diff --git a/mobile/src/tasks/github-work-item-operation.test.ts b/mobile/src/tasks/github-work-item-operation.test.ts index 6dd01e78160..4648128c4a1 100644 --- a/mobile/src/tasks/github-work-item-operation.test.ts +++ b/mobile/src/tasks/github-work-item-operation.test.ts @@ -127,6 +127,49 @@ it('a disconnect/reconnect cannot regain authority with an identical connected-a expect(a.writes).toEqual([]) expect(a.client.stateListeners()).toBe(0) }) +it.each([ + ['sends the ordinary update when its lookup lands on this issue', '1.4.247', 'fork', 1], + ['refuses when its lookup lands on another repository', '1.4.247', 'upstream', 0], + ['refuses before its writes followed its lookup', '1.4.220', 'fork', 0] +])('a server without exact targets %s', async (_name, appVersion, owner, sent) => { + const frames: { method: string; params?: unknown; envelope?: unknown }[] = [] + const client = createFakeRpcClient() + client.sendRequest = async (method, params, options) => { + frames.push({ method, params, envelope: options?.envelope }) + const result = + method === 'status.get' + ? { + runtimeId: 'old', + hostDescriptor: { installationId: 'old' }, + appVersion, + capabilities: [] + } + : method === 'github.workItem' + ? { + type: 'issue', + number: 93, + url: `https://github.corp/${owner}/widgets/issues/93`, + labels: [] + } + : { ok: true } + return { id: String(frames.length), ok: true, result, _meta: { runtimeId: 'old' } } + } + const params = { repo: 'id:clone', number: 93, updates: { state: 'closed' as const } } + const pending = withGitHubTargetOperation({ + client, + params, + ownerRepo: repository, + issueTarget: { repository, type: 'issue', number: 93 }, + send: (selected, exact, options) => + githubIssueUpdate.request(selected, { ...params, ...exact }, options) + }) + await (sent + ? expect(pending).resolves.toBeDefined() + : expect(pending).rejects.toThrow('Update Orca on the executing server')) + expect(frames.filter((frame) => frame.method === 'github.updateIssue')).toEqual( + sent ? [{ method: 'github.updateIssue', params, envelope: undefined }] : [] + ) +}) it('names the relaying desktop when only it lacks the exact-target check', async () => { const desktop = physical('d', false), server = physical('s') diff --git a/mobile/src/tasks/github-work-item-operation.ts b/mobile/src/tasks/github-work-item-operation.ts index 72d78dd761a..150e4c20872 100644 --- a/mobile/src/tasks/github-work-item-operation.ts +++ b/mobile/src/tasks/github-work-item-operation.ts @@ -1,6 +1,11 @@ import type { RpcClient, SendRequestOptions } from '../transport/rpc-client' import type { GitHubOwnerRepo } from '../../../src/shared/github/pull-request-types' -import { GITHUB_EXACT_OPERATION_UNSUPPORTED } from '../../../src/shared/github/work-item-target' +import { hasReachedAppVersion } from '../../../src/shared/app-version' +import { + GITHUB_EXACT_OPERATION_UNSUPPORTED, + matchesGitHubWorkItemTarget, + type GitHubWorkItemTarget +} from '../../../src/shared/github/work-item-target' import { captureGitHubInvocation, GITHUB_DESKTOP_UPDATE_REQUIRED, @@ -9,11 +14,15 @@ import { githubOperationResponse } from '../transport/github-target-invocation' import { openRpcRequestBudget } from '../transport/rpc-request-budget' +import { readGitHubWorkItemByNumber } from './mobile-task-source-search-operations' import { GITHUB_CANONICAL_DETAILS_RUNTIME_CAPABILITY, GITHUB_EXACT_DETAILS_READ_INTENT_RUNTIME_CAPABILITY } from '../../../src/shared/task-source-runtime-capabilities' +// Before this release an unaddressed issue write ignored the issue source its own number lookup honors. +const ISSUE_WRITE_FOLLOWS_LOOKUP_VERSION = '1.4.221' + export async function withGitHubTargetOperation(args: { client: RpcClient params: { repo: string } @@ -21,6 +30,8 @@ export async function withGitHubTargetOperation(args: { read?: boolean /** An addressed details read's intent: false for an ordinary load, true for the user's explicit Refresh. */ canonical?: boolean + /** A write's item, which a server without exact targets may still update unaddressed once its own lookup proves it. */ + issueTarget?: GitHubWorkItemTarget send: ( client: RpcClient, exact: { ownerRepo?: GitHubOwnerRepo; canonical?: boolean }, @@ -70,8 +81,13 @@ export async function withGitHubTargetOperation(args: { ) const canonicalOnly = !explicitIntent && capabilities.includes(GITHUB_CANONICAL_DETAILS_RUNTIME_CAPABILITY) - const sendOwner = canonicalOnly ? args.canonical === true : supports && source !== null - if ((!supports || !source) && !args.read) { + const exact = supports && source !== null + const sendOwner = canonicalOnly ? args.canonical === true : exact + if ( + !exact && + !args.read && + !(await serverFollowsIssueLookup(client, status, args.params.repo, args.issueTarget, options)) + ) { // Name the build to update: the relaying desktop when only it lacks the check, else the executing server. throw new Error( serverSupports && source && !relaySupports @@ -103,6 +119,29 @@ export async function withGitHubTargetOperation(args: { } } +// Without ownerRepo a server writes to the issue its own number lookup finds, so a lookup landing on the target proves it. +async function serverFollowsIssueLookup( + client: RpcClient, + status: Record, + repo: string, + target: GitHubWorkItemTarget | undefined, + options: () => SendRequestOptions +): Promise { + if ( + target?.type !== 'issue' || + typeof status.appVersion !== 'string' || + !hasReachedAppVersion(status.appVersion, ISSUE_WRITE_FOLLOWS_LOOKUP_VERSION) + ) { + return false + } + const item = await readGitHubWorkItemByNumber( + client, + { repo, number: target.number, type: 'issue' }, + options() + ) + return matchesGitHubWorkItemTarget(item, target) +} + async function invocationStatus(client: RpcClient, options: SendRequestOptions) { const invocation = captureGitHubInvocation(client) try { diff --git a/mobile/src/tasks/mobile-task-source-search-operations.ts b/mobile/src/tasks/mobile-task-source-search-operations.ts index 9127834c9ee..003605c305a 100644 --- a/mobile/src/tasks/mobile-task-source-search-operations.ts +++ b/mobile/src/tasks/mobile-task-source-search-operations.ts @@ -1,3 +1,5 @@ +import type { GitHubWorkItem } from '../../../src/shared/github/work-item-types' +import type { RpcClient, SendRequestOptions } from '../transport/rpc-client' import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' import { rpcResultVariant } from '../transport/rpc-operation-result-reader' import { githubPrRepoSlugSchema } from '../session/github-pr-read-reply-schema' @@ -72,6 +74,17 @@ export const githubWorkItemByNumberRead = bindDeferredRpcOperation( }) ) +export async function readGitHubWorkItemByNumber( + client: RpcClient, + params: { repo: string; number: number; type?: 'issue' | 'pr' }, + // A rest tuple keeps the request's recorded arity when no options are given. + ...options: [SendRequestOptions?] +): Promise { + const reply = await githubWorkItemByNumberRead.request(client, params, ...options) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the lookup decodes the same `workItemRow`, so the assertion only fills in members the row omits, each read through a guard or interpolated as text; what the schema adds is that a non-object payload is now a named incompatible reply rather than a spread over a string. + return githubWorkItemByNumberRead.interpret(reply) as GitHubWorkItem | null +} + export const githubWorkItemBySlugRead = bindDeferredRpcOperation( defineRpcOperation({ name: 'github.work-item-by-owner-repo', diff --git a/mobile/src/tasks/smart-source-paste-intent.ts b/mobile/src/tasks/smart-source-paste-intent.ts index 072a538df01..561e7070ba6 100644 --- a/mobile/src/tasks/smart-source-paste-intent.ts +++ b/mobile/src/tasks/smart-source-paste-intent.ts @@ -12,8 +12,8 @@ import type { RpcClient } from '../transport/rpc-client' import { isMethodNotFoundRefusal } from '../transport/rpc-acceptance-policies' import { githubRepoSlugRead, - githubWorkItemByNumberRead, githubWorkItemBySlugRead, + readGitHubWorkItemByNumber, gitlabWorkItemByPathRead } from './mobile-task-source-search-operations' import { githubRepoIdentityKey } from '../../../src/shared/github/repository-identity-key' @@ -132,12 +132,7 @@ export async function lookupGitHubItemByNumber( repoId: string, number: number ): Promise { - const reply = await githubWorkItemByNumberRead.request(client, { - repo: `id:${repoId}`, - number - }) - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the lookup decodes the same `workItemRow`, so the assertion only fills in members the row omits, each read through a guard or interpolated as text; what the schema adds is that a non-object payload is now a named incompatible reply rather than a spread over a string. - const item = githubWorkItemByNumberRead.interpret(reply) as GitHubWorkItem | null + const item = await readGitHubWorkItemByNumber(client, { repo: `id:${repoId}`, number }) return item ? { ...item, repoId } : null } diff --git a/mobile/src/tasks/use-mobile-tasks-gitlab-github-status-actions.tsx b/mobile/src/tasks/use-mobile-tasks-gitlab-github-status-actions.tsx index d57b5a5e4d4..b23217f56ff 100644 --- a/mobile/src/tasks/use-mobile-tasks-gitlab-github-status-actions.tsx +++ b/mobile/src/tasks/use-mobile-tasks-gitlab-github-status-actions.tsx @@ -116,6 +116,7 @@ export function useMobileTasksGitlabGithubStatusActions< client, params, ownerRepo: target.repository, + issueTarget: target, send: (selected, exact, options) => githubIssueUpdate.request(selected, { ...params, ...exact }, options) }) diff --git a/mobile/src/tasks/use-mobile-tasks-project-file-merge-actions.tsx b/mobile/src/tasks/use-mobile-tasks-project-file-merge-actions.tsx index e1ce9c2165c..4899cf2905d 100644 --- a/mobile/src/tasks/use-mobile-tasks-project-file-merge-actions.tsx +++ b/mobile/src/tasks/use-mobile-tasks-project-file-merge-actions.tsx @@ -300,13 +300,15 @@ export function useMobileTasksProjectFileMergeActions githubIssueUpdate.request( selected, @@ -324,7 +326,7 @@ export function useMobileTasksProjectFileMergeActions