mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
fix(runtime): forward scanChildProcesses through the environment inspection RPC
`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.
This commit is contained in:
@@ -153,7 +153,7 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu
|
||||
|
||||
async inspectTerminalProcess(
|
||||
terminalSelector: string,
|
||||
options?: { expectedIncarnationId?: string }
|
||||
options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean }
|
||||
): Promise<PtyProcessInspection> {
|
||||
const leaf = this.resolveLiveLeafForHandle(terminalSelector)
|
||||
if (!leaf?.ptyId || !this.ptyController) {
|
||||
|
||||
@@ -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<string, unknown>
|
||||
): 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'
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -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',
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -109,7 +109,7 @@ export type RuntimePtyController = {
|
||||
getForegroundProcess(ptyId: string): Promise<string | null>
|
||||
inspectProcess?(
|
||||
ptyId: string,
|
||||
options?: { expectedIncarnationId?: PtyIncarnationId }
|
||||
options?: { expectedIncarnationId?: PtyIncarnationId; scanChildProcesses?: boolean }
|
||||
): Promise<PtyProcessInspection>
|
||||
confirmForegroundProcess?(ptyId: string): Promise<string | null>
|
||||
confirmShellForeground?(ptyId: string): Promise<boolean>
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 }
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user