From 9fd04f277db96a2c243ab9dc3b36c9f8be3a591c Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 11 Jul 2026 14:32:27 -0700 Subject: [PATCH] Fix stale-handle rejection using controller-less status checks and contr - Distinguish `terminal_handle_stale` (PTY rebind mid-check) from `terminal_exited` so callers get an accurate reason instead of a generic `terminal_not_writable`. - Move the PTY binding assertion to always run after the fresh foreground read, even when no controller is present, closing a gap where a rebind during a controller-less check went undetected. - Call `confirmForegroundProcess` through its owning controller instance so the method keeps its `this` receiver instead of being invoked unbound. - Update reliability gates and design doc to reflect the new test coverage and corrected error semantics. --- config/reliability-gates.jsonc | 27 ++++++++++++-- docs/fix-web-annotation-active-agent-send.md | 8 ++--- src/main/runtime/orca-runtime.test.ts | 37 ++++++++++++++++++-- src/main/runtime/orca-runtime.ts | 20 +++++------ 4 files changed, 71 insertions(+), 21 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 01432411788..333eb7e8a14 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -2175,7 +2175,8 @@ "a recognized fresh provider confirmation authorizes explicit hook state over an ordinary shell result", "shell, non-agent, null, thrown, and missing confirmation remain fail-closed", "title blockers and ordinary recognized agent evidence add no confirmation call", - "PTY exit or handle rebind during either foreground read rejects evidence bound to the original PTY" + "PTY exit or handle rebind during either foreground read rejects evidence bound to the original PTY", + "controller-less yields preserve stale-handle errors and confirmation methods retain their controller receiver" ] }, { @@ -2205,6 +2206,19 @@ "fresh confirmation is discarded when its owning local PTY exits" ] }, + { + "file": "src/main/providers/windows-conpty-process-membership.test.ts", + "assertions": [ + "exact ConPTY console membership comes from the fixed node-pty helper", + "malformed, incomplete, timed-out, and spawn-error membership reads fail closed" + ] + }, + { + "file": "src/main/daemon/daemon-foreground-confirmation-protocol.test.ts", + "assertions": [ + "daemons from before the fresh-confirmation RPC are rejected by protocol version" + ] + }, { "file": "src/main/daemon/pty-subprocess.test.ts", "assertions": [ @@ -2216,6 +2230,13 @@ "assertions": [ "selected active-agent note sends retain guarded paste and submit routing" ] + }, + { + "file": "src/renderer/src/components/browser-pane/BrowserAnnotationSendMenuContent.test.tsx", + "assertions": [ + "browser annotation send content routes through review-notes send so existing agent sessions remain selectable", + "both browser annotation send surfaces wire the shared menu content" + ] } ], "evidenceRuns": [ @@ -2225,8 +2246,8 @@ "platform": "macos", "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts src/main/runtime/rpc/terminal-send.test.ts src/main/ipc/pty.test.ts src/main/providers/agent-foreground-process.test.ts src/main/providers/local-pty-provider.test.ts src/main/providers/windows-conpty-process-membership.test.ts src/main/daemon/daemon-foreground-confirmation-protocol.test.ts src/main/daemon/pty-subprocess.test.ts src/renderer/src/lib/active-agent-note-send.test.ts src/renderer/src/components/browser-pane/BrowserAnnotationSendMenuContent.test.tsx", "result": "passed", - "durationSeconds": 9.54, - "summary": "Ten focused test files passed (1181 tests), covering runtime confirmation and PTY revalidation, guarded RPC zero-write behavior, PTY controller routing, local/daemon fresh scans, exact ConPTY membership, and unchanged renderer note routing." + "durationSeconds": 9.58, + "summary": "Ten focused test files passed (1183 tests), covering runtime confirmation and PTY revalidation, guarded RPC zero-write behavior, PTY controller routing, local/daemon fresh scans, exact ConPTY membership, and unchanged renderer note routing." } ], "runtimeBudget": { diff --git a/docs/fix-web-annotation-active-agent-send.md b/docs/fix-web-annotation-active-agent-send.md index 5b51b191022..7dfeb27697b 100644 --- a/docs/fix-web-annotation-active-agent-send.md +++ b/docs/fix-web-annotation-active-agent-send.md @@ -2,13 +2,13 @@ ## Problem -Issue [#8303](https://github.com/stablyai/orca/issues/8303) reports that sending a browser annotation to an already-running agent on Windows can fail with “The selected terminal is not a recognized agent session.” The browser menu reaches the shared explicit-target path at `src/renderer/src/lib/active-agent-note-send.ts:82`; the message is the `no-agent` result at `src/renderer/src/lib/active-agent-note-send-result.ts:28`. +Issue [#8303](https://github.com/stablyai/orca/issues/8303) reports that sending a browser annotation to an already-running agent on Windows can fail with “The selected terminal is not a recognized agent session.” The browser menu reaches the shared explicit-target path in `active-agent-note-send.ts`; the message is the `no-agent` result from `active-agent-note-send-result.ts`. ## Root cause -`getTerminalAgentStatus` trusts fresh pane-scoped hook state unless current title/wait evidence blocks it or `terminalHasShellForegroundProcess` reports a shell (`src/main/runtime/orca-runtime.ts:10229`, `src/main/runtime/orca-runtime.ts:10334`). On Windows, the ordinary foreground read can return the ConPTY shell (for example `powershell.exe`) while a recognized agent remains a live descendant, so readiness and guarded writes incorrectly return `no-agent`. +`getTerminalAgentStatus` trusts fresh pane-scoped hook state unless current title/wait evidence blocks it or `terminalHasShellForegroundProcess` reports a shell. On Windows, the ordinary foreground read can return the ConPTY shell (for example `powershell.exe`) while a recognized agent remains a live descendant, so readiness and guarded writes incorrectly return `no-agent`. -Local and daemon PTY providers already expose `confirmForegroundProcess` (`src/main/providers/types.ts:212`). Their Windows implementations perform a fresh process scan with exact ConPTY membership (`src/main/providers/local-pty-provider.ts:994`, `src/main/daemon/pty-subprocess.ts:1055`), but the send guard does not use that stronger evidence. +Local and daemon PTY providers already expose `confirmForegroundProcess`. Their Windows implementations perform a fresh process scan with exact ConPTY membership, but the send guard does not use that stronger evidence. ## Non-goals @@ -54,7 +54,7 @@ Local and daemon PTY providers already expose `confirmForegroundProcess` (`src/m - Runtime unit: fresh explicit state + ordinary shell + confirmed recognized agent is sendable. - Runtime unit: shell/non-agent/null/throw/missing confirmation remains rejected; confirmation is skipped for permission/wait/title blockers and ordinary recognized foreground evidence. -- Runtime unit: a deferred ordinary read or confirmation followed by PTY exit/rebind cannot authorize the stale or replacement PTY, and both process reads receive the originally captured `ptyId`. +- Runtime unit: a deferred ordinary read, confirmation, or controller-less yield followed by PTY exit/rebind cannot authorize the stale or replacement PTY; both process reads receive the captured `ptyId`, and provider methods retain their controller receiver. - RPC unit: guarded callback checks the exact write `ptyId`; a rebind returns not writable and invokes no write. - PTY controller routing unit: confirmation reaches the provider that owns the captured `ptyId`; an unsupported provider or routing failure returns unavailable evidence. - Provider/daemon regression: existing fresh-scan ordering, ConPTY membership, delayed-exit, mixed-version, and cached-follow-up tests remain green. diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 6a2c597ba60..fdd1096529e 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -8563,6 +8563,25 @@ describe('OrcaRuntimeService', () => { expect(confirmForegroundProcess).toHaveBeenCalledWith('pty-1') }) + it('calls foreground confirmation with its controller receiver', async () => { + const getForegroundProcess = vi.fn(async () => 'powershell.exe') + const confirmForegroundProcess = vi.fn( + async function (this: { getForegroundProcess: typeof getForegroundProcess }) { + return this.getForegroundProcess === getForegroundProcess ? 'codex' : null + } + ) + const { runtime, handle } = await createExplicitAgentStatusHarness({ + getForegroundProcess, + confirmForegroundProcess + }) + + await expect(runtime.getTerminalAgentStatus(handle)).resolves.toMatchObject({ + isRunningAgent: true, + status: 'working' + }) + expect(confirmForegroundProcess).toHaveBeenCalledOnce() + }) + it.each([ ['shell', async () => 'pwsh.exe'], ['non-agent', async () => 'vim'], @@ -8674,10 +8693,22 @@ describe('OrcaRuntimeService', () => { syncPty('pty-2') foreground.resolve('zsh') - await expect(status).rejects.toThrow('terminal_not_writable') + await expect(status).rejects.toThrow('terminal_handle_stale') expect(confirmForegroundProcess).not.toHaveBeenCalled() }) + it('rejects a handle rebind while a controller-less status check yields', async () => { + const { runtime, handle, syncPty } = await createExplicitAgentStatusHarness({ + getForegroundProcess: async () => 'zsh' + }) + runtime.setPtyController(null) + + const status = runtime.getTerminalAgentStatus(handle) + syncPty('pty-2') + + await expect(status).rejects.toThrow('terminal_handle_stale') + }) + it('rejects confirmation evidence when the handle rebinds during the fresh read', async () => { const confirmation = deferred() const confirmForegroundProcess = vi.fn(() => confirmation.promise) @@ -8691,7 +8722,7 @@ describe('OrcaRuntimeService', () => { syncPty('pty-2') confirmation.resolve('codex') - await expect(status).rejects.toThrow('terminal_not_writable') + await expect(status).rejects.toThrow('terminal_handle_stale') }) it('rejects confirmation evidence when the owning PTY exits', async () => { @@ -8707,7 +8738,7 @@ describe('OrcaRuntimeService', () => { runtime.onPtyExit('pty-1', 0) confirmation.resolve('codex') - await expect(status).rejects.toThrow('terminal_not_writable') + await expect(status).rejects.toThrow('terminal_exited') }) it('reports permission from a title-derived action-required agent state', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index d45b352fbb9..5a2ec388866 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -10254,6 +10254,7 @@ export class OrcaRuntimeService { const isRunningAgent = !terminalTitleBlocksExplicitAgentStatus(terminal.title) && !(await this.terminalHasShellForegroundProcess(handle, ptyId)) + this.assertTerminalAgentStatusPtyBinding(handle, ptyId) return { handle, isRunningAgent, @@ -10288,15 +10289,12 @@ export class OrcaRuntimeService { } private assertTerminalAgentStatusPtyBinding(handle: string, expectedPtyId: string): void { - try { - if (this.getTerminalAgentStatusPtyId(handle) === expectedPtyId) { - return - } - } catch { - // Fall through to the stable binding error below. + if (this.getTerminalAgentStatusPtyId(handle) === expectedPtyId) { + return } - // Why: delayed process evidence belongs only to the PTY that started the read. - throw new Error('terminal_not_writable') + // Why: delayed process evidence belongs only to the PTY that started the + // read, while callers still rely on the established stale-handle contract. + throw new Error('terminal_handle_stale') } private getTerminalAgentStatusSnapshot( @@ -10382,13 +10380,13 @@ export class OrcaRuntimeService { if (!foregroundProcess || !isShellProcess(foregroundProcess)) { return false } - const confirmForegroundProcess = this.ptyController.confirmForegroundProcess - if (!confirmForegroundProcess) { + const confirmationController = this.ptyController + if (!confirmationController?.confirmForegroundProcess) { return true } let confirmedProcess: string | null try { - confirmedProcess = await confirmForegroundProcess(ptyId) + confirmedProcess = await confirmationController.confirmForegroundProcess(ptyId) } catch { this.assertTerminalAgentStatusPtyBinding(handle, ptyId) return true