mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 16:02:55 +00:00
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.
This commit is contained in:
@@ -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": {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<string | null>()
|
||||
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 () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user