From 4b87bc718e6df8152d0e1fcebc3d4096d65b19ec Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:39:38 -0700 Subject: [PATCH] refactor(agent-launch): redefine the agent.launch contract (#20999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(agent-launch): redefine the agent.launch contract `agent.launch` has no clients yet, so the contract is redefined in place rather than versioned. - params require `operation.id`, pinned to the shipped operation-id mint so the host can read the embedded timestamp back. No caller-supplied fingerprint: the host derives its own. - the result carries `disposition` ('created' | 'replayed', the same vocabulary `RuntimeCreateAgentSessionResult` already uses) and a single top-level `warning` instead of one on the terminal arm only. - the prompt receipt becomes an outcome enum, so a receipt can under-claim instead of reporting a bare `delivered: false`. - the dead `customization` field is deleted, and the mode-reason union and receipt are declared once in shared with main re-exporting. - `clientMutationId` joins the reserved create fields, with a test pinning the list to the create schema in both directions. Contract only; no behaviour change and no ledger wiring. * docs(agent-launch): stop calling the stripped set "agent fields" `clientMutationId` joined AGENT_LAUNCH_RESERVED_CREATE_FIELDS, so three comments describing the stripped set as agent fields now teach the wrong model — including a SAFETY rationale, where a reader is trusting it most. The rationale's claim is unchanged and still sound: deleting keys from a parsed object leaves the rest the parsed shape. * refactor(agent-launch): make the attempt id the launch's only idempotency key Review follow-ups on the contract redefinition. `operation: { id }` becomes a flat `clientOperationId`, spelled the way `terminal.createAgentSession` and the structured mutation envelope already spell the same concept, and admitted by the shipped `parseAgentSessionOperationTimestamp` rather than a second copy of its pattern — so `agent-session-host-authority` keeps the regex private. The handler now dedupes on that id instead of the create payload's `clientMutationId`. That field is optional, so keying on it left any launch that omitted one with no idempotency at all, while the required attempt id did nothing. Reserving `clientMutationId` is still right, but for the reason the comments now give: `createManagedWorktree` never reads it, so a copy left in the forwarded payload is inert while still reading as a guarantee. The previous rationale — that it was a second live dedupe key — was not true. `messageId` moves onto the prompt receipt's `journaled` arm so a producer cannot report the text as committed without saying where, and `rpcCallerKey` picks up the `terminal.create` call site it was lifted from instead of shipping with no callers. * docs(agent-launch): record why disposition is two-valued only for now The ledger admits attempts whose outcome was never recorded, and neither `created` nor `replayed` can say "I cannot tell you" — a caller handed `created` for an unresolved attempt starts a second agent. Noted at the type rather than in review, so whoever wires the ledger reads it where they edit. * fix(agent-launch): keep contract within implemented guarantees --- .../agent-launch-worktree-create.test.ts | 24 +------- .../src/tasks/agent-launch-worktree-create.ts | 13 ++-- .../agent-launch-executor.test.ts | 5 +- .../agent-launch/agent-launch-executor.ts | 4 +- src/main/agent-launch/agent-launch-mode.ts | 31 +++------- .../runtime/rpc/methods/agent-launch.test.ts | 13 ++++ src/main/runtime/rpc/methods/agent-launch.ts | 4 +- src/shared/agent-launch-intent.ts | 61 +++++++++++++------ src/shared/protocol-version.ts | 3 +- 9 files changed, 79 insertions(+), 79 deletions(-) diff --git a/mobile/src/tasks/agent-launch-worktree-create.test.ts b/mobile/src/tasks/agent-launch-worktree-create.test.ts index 8b2567919c3..ada5becd4ca 100644 --- a/mobile/src/tasks/agent-launch-worktree-create.test.ts +++ b/mobile/src/tasks/agent-launch-worktree-create.test.ts @@ -88,31 +88,13 @@ describe('readAgentLaunchCreateOutcome', () => { ).toEqual({ worktreeId: 'wt-1' }) }) - it('reads a warning an older host nested on the outcome', () => { - // A host from before the warning moved to the top level nests it on the terminal outcome, and - // advertises the same `agent.launch.v1`, so this route really is taken against one. Its warning - // is legitimate, not a stale shape to defend against: dropping it loses the incomplete-create - // notice the `worktree.create` path already delivered, which is a regression rather than a - // contract cleanup. + it('ignores a warning outside the v2 top-level result contract', () => { expect( readAgentLaunchCreateOutcome({ worktreeId: 'wt-1', - outcome: { kind: 'terminal', handle: 'term-1', warning: ' startup terminal failed ' } + outcome: { kind: 'terminal', handle: 'term-1', warning: 'stale nested warning' } }) - ).toEqual({ worktreeId: 'wt-1', warning: 'startup terminal failed' }) - }) - - it('prefers the top-level warning over a nested one', () => { - // A current host writes only the top level — `AgentLaunchOutcome` has no `warning` on either - // arm, so it cannot nest one — meaning this case cannot arise from one. Pinned anyway so the - // migration fallback can never shadow the fresher value. - expect( - readAgentLaunchCreateOutcome({ - worktreeId: 'wt-1', - outcome: { kind: 'terminal', handle: 'term-1', warning: 'nested' }, - warning: 'top level' - }) - ).toEqual({ worktreeId: 'wt-1', warning: 'top level' }) + ).toEqual({ worktreeId: 'wt-1' }) }) }) diff --git a/mobile/src/tasks/agent-launch-worktree-create.ts b/mobile/src/tasks/agent-launch-worktree-create.ts index a5fc5ef5d5a..26bd55b671a 100644 --- a/mobile/src/tasks/agent-launch-worktree-create.ts +++ b/mobile/src/tasks/agent-launch-worktree-create.ts @@ -59,14 +59,9 @@ export function readAgentLaunchCreateOutcome(result: unknown): AgentLaunchCreate if (typeof worktreeId !== 'string' || !worktreeId.trim()) { return null } - // A current host reports an incomplete create at the top level, the same place `worktree.create` - // puts it, so nothing here branches on which surface the host built to find it. A host that - // predates that move nests the same warning on the terminal outcome instead, and still advertises - // the one `agent.launch.v1` capability, so this route cannot tell the two apart up front — read - // both shapes for as long as such a host can be paired. Top level wins: it is the only place a - // current host writes, so the fallback cannot shadow a fresher value. - const warning = - readTrimmedWarning(result) || readTrimmedWarning('outcome' in result ? result.outcome : null) + // v2 guarantees that an incomplete create is reported at the top level, the same place + // `worktree.create` puts it, so nothing here branches on which surface the host built. + const warning = readTrimmedWarning(result) return { worktreeId, ...(warning ? { warning } : {}) } } @@ -81,7 +76,7 @@ function readTrimmedWarning(source: unknown): string { * Whether the host rejected the method itself rather than the create. * * The `status.get` probe can be stale in one direction that matters: the host advertises - * `agent.launch.v1` but has not yet recorded this client's own capability list, and then refuses + * `agent.launch.v2` but has not yet recorded this client's own capability list, and then refuses * the call. Downgrading to `worktree.create` keeps that race from failing a create outright. */ export function isAgentLaunchUnsupportedRefusal(error: { diff --git a/src/main/agent-launch/agent-launch-executor.test.ts b/src/main/agent-launch/agent-launch-executor.test.ts index 092262470a8..8910ccd1e60 100644 --- a/src/main/agent-launch/agent-launch-executor.test.ts +++ b/src/main/agent-launch/agent-launch-executor.test.ts @@ -232,13 +232,14 @@ describe('an agent with no structured session', () => { }) describe('the prompt receipt', () => { - it('reports a requested prompt as undelivered rather than omitting it', async () => { + it('reports a requested prompt as not delivered rather than omitting it', async () => { const h = harness({}) const result = await h.run({ ...CREATE_INTENT, prompt: { text: 'do the thing', delivery: 'draft' } }) - expect(result.prompt).toEqual({ delivery: 'draft', delivered: false }) + // The executor delivers nothing, so the only honest outcome is the one that under-claims. + expect(result.prompt).toEqual({ delivery: 'draft', outcome: 'not-delivered' }) }) it('omits the receipt when no prompt was requested', async () => { diff --git a/src/main/agent-launch/agent-launch-executor.ts b/src/main/agent-launch/agent-launch-executor.ts index 9b36d91ac05..c65f63061fa 100644 --- a/src/main/agent-launch/agent-launch-executor.ts +++ b/src/main/agent-launch/agent-launch-executor.ts @@ -281,10 +281,10 @@ function existingWorktreeId(target: AgentLaunchTarget): string { /** Prompt delivery is the caller's, not the executor's: a PTY paste is observed by whoever owns * the pane, and a structured first turn is sent through the session. The executor reports the - * requested delivery back undelivered so a caller cannot mistake silence for delivery. */ + * requested delivery back as not delivered so a caller cannot mistake silence for delivery. */ function promptReceipt(intent: AgentLaunchIntent): Pick { if (!intent.prompt) { return {} } - return { prompt: { delivery: intent.prompt.delivery, delivered: false } } + return { prompt: { delivery: intent.prompt.delivery, outcome: 'not-delivered' } } } diff --git a/src/main/agent-launch/agent-launch-mode.ts b/src/main/agent-launch/agent-launch-mode.ts index 2add36df4e6..7533c0f2a01 100644 --- a/src/main/agent-launch/agent-launch-mode.ts +++ b/src/main/agent-launch/agent-launch-mode.ts @@ -17,6 +17,11 @@ * records; every other surface says "chat session" / "terminal agent". */ +import type { + AgentLaunchMode, + AgentLaunchModeReason, + AgentLaunchModeReceipt +} from '../../shared/agent-launch-intent' import type { GlobalSettings } from '../../shared/global-settings-types' import { RUNTIME_CAPABILITIES } from '../../shared/protocol-version' import { @@ -29,29 +34,9 @@ import type { TuiAgent } from '../../shared/tui-agent' import { hasExplicitTuiLaunchCommand } from '../../shared/tui-agent-launch-command-override' import type { OrcaRuntimeService } from '../runtime/orca-runtime' -export type AgentLaunchMode = 'structured' | 'terminal' - -export type AgentLaunchModeReason = - | 'user_default' - | 'remote_execution_host' - | 'reused_terminal' - | 'agent_without_structured_session' - | 'tui_launch_command' - | 'structured_sessions_unavailable' - | 'structured_support_unknown' - | 'wsl_execution_runtime' - | 'codex_on_windows' - | 'structured_unsupported_on_host' - -export type AgentLaunchModeReceipt = { - /** The mode the launch actually ran in. */ - mode: AgentLaunchMode - /** The user's settings default for a new agent tab. */ - preferred: AgentLaunchMode - reason: AgentLaunchModeReason - /** One sentence, always present, so a fallback is never silent. */ - detail: string -} +// The receipt is part of the launch contract, so it is declared with the rest of it; re-exported +// here because this module is where the decision that fills it lives. +export type { AgentLaunchMode, AgentLaunchModeReason, AgentLaunchModeReceipt } /** What this caller calls the thing it is starting, so one decision serves every surface without * a receipt reading "worker" on a phone. */ diff --git a/src/main/runtime/rpc/methods/agent-launch.test.ts b/src/main/runtime/rpc/methods/agent-launch.test.ts index db807f3e7f8..11caa1d3fa1 100644 --- a/src/main/runtime/rpc/methods/agent-launch.test.ts +++ b/src/main/runtime/rpc/methods/agent-launch.test.ts @@ -191,6 +191,19 @@ describe('who may call agent.launch', () => { expect(runtime.createManagedWorktree).toHaveBeenCalled() }) + it('refuses the prior wire contract after the result shape changed', async () => { + const runtime = runtimeStub() + await expect( + launch(CREATE_LAUNCH, runtime, { + clientKind: 'mobile', + pairedDeviceId: 'device-1', + clientCapabilities: ['agent.launch.v1'] + }) + ).rejects.toThrow('agent_launch_unsupported') + expect(AGENT_LAUNCH_RUNTIME_CAPABILITY).toBe('agent.launch.v2') + expect(runtime.createManagedWorktree).not.toHaveBeenCalled() + }) + it('admits an in-process caller, which negotiates nothing', async () => { const runtime = runtimeStub() await launch(CREATE_LAUNCH, runtime, {}) diff --git a/src/main/runtime/rpc/methods/agent-launch.ts b/src/main/runtime/rpc/methods/agent-launch.ts index 54e0e8a76f6..49a8f9e3951 100644 --- a/src/main/runtime/rpc/methods/agent-launch.ts +++ b/src/main/runtime/rpc/methods/agent-launch.ts @@ -23,7 +23,7 @@ import { agentLaunchSurfaceFactory } from './agent-launch-surfaces' import { agentLaunchWorkspaceFactory } from './agent-launch-worktree-creation' /** - * Advertising `agent.launch.v1` is a client's statement that it understands EITHER outcome — a + * Advertising `agent.launch.v2` is a client's statement that it understands EITHER outcome — a * structured session it can open, or a terminal agent. A client that can only render one of the * two must keep using the surface-specific methods instead. In-process callers are the same build * as the host and negotiate nothing. @@ -103,6 +103,8 @@ export const AGENT_LAUNCH_METHODS = [ surfaces: agentLaunchSurfaceFactory(context), workspaces: agentLaunchWorkspaceFactory(context, intent.agent) }) + // Preserve the existing bounded create guard. Complete launch replay needs durable operation + // identity, caller scope and a host-computed payload fingerprint; this cache has none of them. if (params.target.kind === 'create-worktree' && params.target.create.clientMutationId) { return context.runtime.dedupeWorktreeCreate( params.target.create.repo, diff --git a/src/shared/agent-launch-intent.ts b/src/shared/agent-launch-intent.ts index ff02565112b..702add77acd 100644 --- a/src/shared/agent-launch-intent.ts +++ b/src/shared/agent-launch-intent.ts @@ -48,18 +48,6 @@ export type AgentLaunchTarget = * terminal agent: a running PTY keeps its execution transport. */ export type AgentLaunchReusedTerminal = { handle: string } -/** - * Facts that only the calling surface knows and that the route has to see. These are inputs to the - * decision, not requests: a caller states that it is passing custom agent arguments, and the host - * concludes that a terminal is required. - */ -export type AgentLaunchCustomization = { - /** Explicit per-launch agent argv. Only a TUI applies these. */ - agentArgs?: string - /** A subdirectory the agent should start in. Only a TUI applies this. */ - cwd?: string -} - export type AgentLaunchIntent = { agent: TuiAgent target: AgentLaunchTarget @@ -67,19 +55,35 @@ export type AgentLaunchIntent = { /** Seeded launch options, narrowed by the host to what a structured create accepts. */ sessionOptions?: Readonly> reuseTerminal?: AgentLaunchReusedTerminal - customization?: AgentLaunchCustomization } /** The surface the host actually created. */ export type AgentLaunchOutcome = | { kind: 'structured'; sessionId: string; handle: string } | { kind: 'terminal'; handle: string } +/** + * What became of the launch text. + * + * An enum rather than a boolean because "not delivered" and "handed to a surface that delivers it + * out of band" are different answers, and a caller deciding whether to resend needs to tell them + * apart. A receipt may under-claim — reporting a delivery it cannot vouch for as `not-delivered` is + * a wasted resend, while over-claiming loses the text silently. + */ +export type AgentLaunchPromptOutcome = AgentLaunchPromptDisposal['outcome'] + +/** `messageId` hangs off the `journaled` arm rather than sitting optional beside all three: a + * producer must not be able to claim the text was committed and then not say where. */ +type AgentLaunchPromptDisposal = + /** Committed to the session's transcript, which `messageId` names. */ + | { outcome: 'journaled'; messageId: string } + /** Written to a PTY, whose consumption only the pane's owner observes. */ + | { outcome: 'handed-to-terminal' } + /** Not delivered by this call; the caller still owns the text. */ + | { outcome: 'not-delivered' } -/** Whether the launch text was delivered, for a caller that needs to report or retry it. */ export type AgentLaunchPromptReceipt = { delivery: AgentLaunchPromptDelivery - delivered: boolean -} +} & AgentLaunchPromptDisposal export type AgentLaunchResult = { outcome: AgentLaunchOutcome @@ -102,14 +106,31 @@ export type AgentLaunchResult = { prompt?: AgentLaunchPromptReceipt } +export type AgentLaunchMode = 'structured' | 'terminal' + +/** Why a launch ran in the mode it did. `user_default` is the preference being honoured; every + * other member is a reason the preference could not be applied to this launch. */ +export type AgentLaunchModeReason = + | 'user_default' + | 'remote_execution_host' + | 'reused_terminal' + | 'agent_without_structured_session' + | 'tui_launch_command' + | 'structured_sessions_unavailable' + | 'structured_support_unknown' + | 'wsl_execution_runtime' + | 'codex_on_windows' + | 'structured_unsupported_on_host' + /** Restates `WorkerStartModeReceipt` in surface-neutral terms so orchestration's receipt and a * mobile or renderer launch report the same vocabulary. */ export type AgentLaunchModeReceipt = { - mode: 'structured' | 'terminal' + /** The mode the launch actually ran in. */ + mode: AgentLaunchMode /** The user's settings default for a new agent tab. */ - preferred: 'structured' | 'terminal' - reason: string - /** One sentence, always present. */ + preferred: AgentLaunchMode + reason: AgentLaunchModeReason + /** One sentence, always present, so a fallback is never silent. */ detail: string } diff --git a/src/shared/protocol-version.ts b/src/shared/protocol-version.ts index 6a8ee756b3a..11b834f00da 100644 --- a/src/shared/protocol-version.ts +++ b/src/shared/protocol-version.ts @@ -250,7 +250,8 @@ export const NOTIFICATIONS_REMOTE_PUSH_RUNTIME_CAPABILITY = 'notifications.remot * picks: a structured session it can open, or a terminal agent. A client that renders only one of * the two keeps using the surface-specific methods. */ -export const AGENT_LAUNCH_RUNTIME_CAPABILITY = 'agent.launch.v1' as const +// v2 makes prompt delivery an outcome union and top-level warnings the only supported shape. +export const AGENT_LAUNCH_RUNTIME_CAPABILITY = 'agent.launch.v2' as const // Generic native clients include the CLI and must not claim Electron-only page // placement support.