From 899a126483def08d152bb02b25b40c96bc0d36fa Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:51:57 -0700 Subject: [PATCH] fix(runtime): forward scanChildProcesses through the environment inspection RPC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `guardRunningTerminalClose` asks the host to pay for a real child-process read, but the environment path dropped the option before it reached the wire: the renderer sent only `expectedIncarnationId`, and the RPC schema — the shared `TerminalHandle` — silently stripped anything else. A host routing that pane through an SSH relay then declined to scan and answered `unverifiable`, which `inspectionReportsRunningWork` reads as running work. The result was a close confirmation on an idle pane, which is the nag this PR exists to avoid. Forwarded through all four layers: renderer payload, RPC schema, method handler, and the runtime/controller signatures. The schema is a dedicated extension rather than a field on `TerminalHandle`, so `clearBuffer`/`agentStatus`/`isRunningAgent` keep refusing an option they have no use for. The silent strip is not itself the defect — it is what makes a new optional member safe to send to an old host, per docs/reference/remote-wire-compatibility.md. The defect was the schema and its caller drifting inside one version, so the tests pin the registered method rather than the schema alone: pointing it back at `TerminalHandle` compiles, parses, and drops the option. Found by review on #18591. --- ...tore-structured-agent-session-tabs-once.ts | 2 +- .../terminal-inspect-process-params.test.ts | 92 +++++++++++++++++++ .../terminal/terminal-query-methods.ts | 23 +++-- .../rpc/methods/terminal/unary-schemas.ts | 11 +++ .../runtime-pty-controller-contract.ts | 2 +- .../runtime-terminal-inspection.test.ts | 52 +++++++++++ .../runtime/runtime-terminal-inspection.ts | 6 +- 7 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts diff --git a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts index 0ed3700ba99..4466594b0dc 100644 --- a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts +++ b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts @@ -153,7 +153,7 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu async inspectTerminalProcess( terminalSelector: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ): Promise { const leaf = this.resolveLiveLeafForHandle(terminalSelector) if (!leaf?.ptyId || !this.ptyController) { diff --git a/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts b/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts new file mode 100644 index 00000000000..48649bc372c --- /dev/null +++ b/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts @@ -0,0 +1,92 @@ +// The host half of the same contract: an RPC schema silently strips keys it does not declare, which +// is exactly what forward compatibility needs and exactly how a caller's option can vanish inside +// one version. `scanChildProcesses` has to be declared here, and only here -- the sibling handle +// methods have no use for it and must keep refusing it. +import { describe, expect, it, vi } from 'vitest' +import type { ZodType } from 'zod' +import { TERMINAL_QUERY_METHODS } from './terminal-query-methods' +import { TerminalHandle, TerminalInspectProcess } from './unary-schemas' + +/** The method as registered, so a schema swap on the definition cannot pass unseen. */ +function inspectProcessMethod() { + const method = TERMINAL_QUERY_METHODS.find((entry) => entry.name === 'terminal.inspectProcess') + if (!method) { + throw new Error('terminal.inspectProcess is not registered') + } + return method +} + +async function callRegisteredHandler( + params: Record +): Promise<{ terminal: string; options: unknown }> { + const method = inspectProcessMethod() + const parsed = (method.params as ZodType).parse(params) + const inspectTerminalProcess = vi.fn(async () => ({ + foregroundProcess: null, + hasChildProcesses: false + })) + await method.handler(parsed, { runtime: { inspectTerminalProcess } } as never, undefined as never) + const [terminal, options] = inspectTerminalProcess.mock.calls[0] as unknown as [string, unknown] + return { terminal, options } +} + +describe('terminal.inspectProcess registration', () => { + // The half the schema test alone cannot see: pointing the method back at the shared handle schema + // compiles, parses, and silently drops the option. This exercises the registered definition. + it('carries scanChildProcesses from the wire into the runtime call', async () => { + await expect( + callRegisteredHandler({ terminal: 'term_1', scanChildProcesses: true }) + ).resolves.toEqual({ terminal: 'term_1', options: { scanChildProcesses: true } }) + }) + + it('carries it alongside the incarnation fence', async () => { + await expect( + callRegisteredHandler({ + terminal: 'term_1', + expectedIncarnationId: 'inc-1', + scanChildProcesses: true + }) + ).resolves.toEqual({ + terminal: 'term_1', + options: { expectedIncarnationId: 'inc-1', scanChildProcesses: true } + }) + }) + + it('keeps the legacy one-argument shape for a bare poll', async () => { + await expect(callRegisteredHandler({ terminal: 'term_1' })).resolves.toEqual({ + terminal: 'term_1', + options: undefined + }) + }) +}) + +describe('terminal.inspectProcess params', () => { + it('preserves scanChildProcesses', () => { + expect(TerminalInspectProcess.parse({ terminal: 'term_1', scanChildProcesses: true })).toEqual({ + terminal: 'term_1', + scanChildProcesses: true + }) + }) + + it('preserves it alongside the incarnation fence', () => { + expect( + TerminalInspectProcess.parse({ + terminal: 'term_1', + expectedIncarnationId: 'inc-1', + scanChildProcesses: true + }) + ).toEqual({ terminal: 'term_1', expectedIncarnationId: 'inc-1', scanChildProcesses: true }) + }) + + it('leaves it absent for a polling caller', () => { + expect(TerminalInspectProcess.parse({ terminal: 'term_1' })).toEqual({ terminal: 'term_1' }) + }) + + // The shape that produced the bug, pinned so nobody "simplifies" the method back onto the shared + // handle schema: TerminalHandle drops the option on the floor without complaining. + it('shows why the shared handle schema could not carry it', () => { + expect(TerminalHandle.parse({ terminal: 'term_1', scanChildProcesses: true })).toEqual({ + terminal: 'term_1' + }) + }) +}) diff --git a/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts b/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts index a8aec9ff573..bf7b4a5bd87 100644 --- a/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts +++ b/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts @@ -1,6 +1,7 @@ import { defineMethod, type RpcAnyMethod } from '../../core' import { TerminalHandle, + TerminalInspectProcess, TerminalListParams, TerminalRead, TerminalRecoverPane, @@ -65,15 +66,21 @@ export const TERMINAL_QUERY_METHODS: RpcAnyMethod[] = [ }), defineMethod({ name: 'terminal.inspectProcess', - params: TerminalHandle, - handler: async (params, { runtime }) => ({ - process: await runtime.inspectTerminalProcess( - params.terminal, - params.expectedIncarnationId + params: TerminalInspectProcess, + handler: async (params, { runtime }) => { + const options = { + ...(params.expectedIncarnationId ? { expectedIncarnationId: params.expectedIncarnationId } - : undefined - ) - }) + : {}), + ...(params.scanChildProcesses === true ? { scanChildProcesses: true } : {}) + } + return { + process: await runtime.inspectTerminalProcess( + params.terminal, + Object.keys(options).length > 0 ? options : undefined + ) + } + } }), defineMethod({ name: 'terminal.isRunningAgent', diff --git a/src/main/runtime/rpc/methods/terminal/unary-schemas.ts b/src/main/runtime/rpc/methods/terminal/unary-schemas.ts index 323b34a7544..afe0a3bb486 100644 --- a/src/main/runtime/rpc/methods/terminal/unary-schemas.ts +++ b/src/main/runtime/rpc/methods/terminal/unary-schemas.ts @@ -13,6 +13,17 @@ export const TerminalFocus = TerminalHandle.extend({ navigation: z.enum(['caller', 'host']).optional() }) +/** + * `terminal.inspectProcess` carries one member the sibling handle methods must not: whether the + * caller's answer decides something once, which is what licenses the host to pay for a process-table + * read. Extended rather than added to `TerminalHandle` so `clearBuffer`/`agentStatus`/`isRunningAgent` + * keep refusing an option they have no use for. + */ +export const TerminalInspectProcess = TerminalHandle.extend({ + // Additive request member understood by newer hosts; legacy hosts safely ignore it. + scanChildProcesses: z.boolean().optional() +}) + export const TerminalListParams = z.object({ worktree: OptionalString, limit: OptionalFiniteNumber, diff --git a/src/main/runtime/runtime-pty-controller-contract.ts b/src/main/runtime/runtime-pty-controller-contract.ts index 194d0bb665c..665c6fdb609 100644 --- a/src/main/runtime/runtime-pty-controller-contract.ts +++ b/src/main/runtime/runtime-pty-controller-contract.ts @@ -109,7 +109,7 @@ export type RuntimePtyController = { getForegroundProcess(ptyId: string): Promise inspectProcess?( ptyId: string, - options?: { expectedIncarnationId?: PtyIncarnationId } + options?: { expectedIncarnationId?: PtyIncarnationId; scanChildProcesses?: boolean } ): Promise confirmForegroundProcess?(ptyId: string): Promise confirmShellForeground?(ptyId: string): Promise diff --git a/src/renderer/src/runtime/runtime-terminal-inspection.test.ts b/src/renderer/src/runtime/runtime-terminal-inspection.test.ts index 52609b75590..7f54f5873bc 100644 --- a/src/renderer/src/runtime/runtime-terminal-inspection.test.ts +++ b/src/renderer/src/runtime/runtime-terminal-inspection.test.ts @@ -146,6 +146,58 @@ describe('runtime terminal owner routing', () => { expect(localHasChildren).not.toHaveBeenCalled() }) + // Why these exist: the close guards ask the host to pay for a real child-process read, and the + // environment path used to drop the option before it reached the wire. The host then declined to + // scan and answered `unverifiable`, which the guard reads as running work -- a confirmation + // dialog on every idle close of a remote Windows pane. Found by review on #18591. + it('forwards scanChildProcesses to the PTY owning environment', async () => { + await inspectRuntimeTerminalProcess( + { activeRuntimeEnvironmentId: 'env-2' }, + 'remote:env-1@@terminal-1', + { scanChildProcesses: true } + ) + + expect(runtimeCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'terminal.inspectProcess', + params: { terminal: 'terminal-1', scanChildProcesses: true }, + timeoutMs: 15_000 + }) + }) + + it('forwards scanChildProcesses alongside the incarnation fence', async () => { + await inspectRuntimeTerminalProcess( + { activeRuntimeEnvironmentId: 'env-2' }, + 'remote:env-1@@terminal-1', + { expectedIncarnationId: 'incarnation-1', scanChildProcesses: true } + ) + + expect(runtimeCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'terminal.inspectProcess', + params: { + terminal: 'terminal-1', + expectedIncarnationId: 'incarnation-1', + scanChildProcesses: true + }, + timeoutMs: 15_000 + }) + }) + + it('omits scanChildProcesses when the caller is only polling', async () => { + await inspectRuntimeTerminalProcess( + { activeRuntimeEnvironmentId: 'env-2' }, + 'remote:env-1@@terminal-1' + ) + + expect(runtimeCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'terminal.inspectProcess', + params: { terminal: 'terminal-1' }, + timeoutMs: 15_000 + }) + }) + it('maps an old host inspection to client-only unverifiable', async () => { runtimeCall.mockResolvedValue({ ok: true, diff --git a/src/renderer/src/runtime/runtime-terminal-inspection.ts b/src/renderer/src/runtime/runtime-terminal-inspection.ts index 1dbceaad832..8c354cdc415 100644 --- a/src/renderer/src/runtime/runtime-terminal-inspection.ts +++ b/src/renderer/src/runtime/runtime-terminal-inspection.ts @@ -169,7 +169,11 @@ export async function inspectRuntimeTerminalProcess( terminal, ...(options?.expectedIncarnationId ? { expectedIncarnationId: options.expectedIncarnationId } - : {}) + : {}), + // Why forwarded: the close guards pass this so the host pays for a real child-process read. + // Dropped here, the host declines to scan and answers `unverifiable`, which the guard reads + // as running work -- a confirmation dialog on every idle close of a remote Windows pane. + ...(options?.scanChildProcesses === true ? { scanChildProcesses: true } : {}) }, { timeoutMs: 15_000 } )