From b9e1f511e9565a766086d6ee9ac8be435b30fcf6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:47:18 -0700 Subject: [PATCH] fix(completion): refuse an exit the legacy foreground field contradicts The mirror of the children-axis rule, one axis over. Recognition used to read `recognizeAgentProcess(result.foregroundProcess)`. Moving it onto the evidence union made it read `evidence.foreground.processName`, so a sample whose union reports `{observed, processName: null}` beside a legacy `foregroundProcess: 'codex'` no longer recognizes an agent. Its foreground verdict is still inside the vocabulary, so the out-of-vocabulary guard cannot refuse it either, and the sample falls through to the exit gate and dispatches a process-exit completion for an agent the host still names in the foreground. Only the `null` pole of the legacy scalar is the lossy collapse this batch removes; it conflates "no agent in the foreground" with "could not ask". A recognized non-null name is a positive observation on every producer: buildPtyProcessInspectionWireResult copies it straight off an `observed` foreground verdict, and the ps/pgrep probes only set it from a command line the host actually read back. Discarding a positive liveness observation to honour a union reporting no name is the #16900/#16908 polarity bug on the foreground axis, so the gate now refuses whenever either half still names an agent. The refusal is added at the exit gate rather than by widening recognition, so the arm can only ever withhold a dispatch, never introduce one: reading the legacy name into handleRecognizedProcess would fire a process-replacement completion on a disagreeing name, which is less conservative, not more. No conforming host is affected. To reach the gate the foreground verdict is already 'observed', so buildPtyProcessInspectionWireResult's legacy scalar is byte-identical to the union's processName the gate already found unrecognized, and a host predating the field gets that same scalar through the reader's legacy fallback. Only a peer this client cannot vouch for can disagree, the same premise the children-axis arm is pinned on. Pinned by four monitor tests through the real reader with no mock; all four fail on the previous head, each with an actual false process-exit completion for 'codex'. The four-way ablation keeps the arms independent: reverting only this term reddens its four tests and leaves the vocabulary-guard, positive-exited and legacy-live-children pins green. --- ...completion-legacy-foreground-agent.test.ts | 161 ++++++++++++++++++ .../agent-completion-process-monitor.ts | 9 +- 2 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/components/terminal-pane/agent-completion-legacy-foreground-agent.test.ts diff --git a/src/renderer/src/components/terminal-pane/agent-completion-legacy-foreground-agent.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-legacy-foreground-agent.test.ts new file mode 100644 index 00000000000..6657cfa76dd --- /dev/null +++ b/src/renderer/src/components/terminal-pane/agent-completion-legacy-foreground-agent.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it, vi } from 'vitest' +import { createAgentCompletionCoordinator } from './agent-completion-coordinator' +import { useAgentCompletionCoordinatorLifecycle } from './agent-completion-coordinator-test-harness' +import { POLL_TIER_INTERVAL_MS } from './agent-completion-poll-cadence' +import type { RuntimeTerminalProcessInspection } from '@/runtime/runtime-terminal-inspection' + +// Pins the foreground axis of the legacy-contradiction rule, one axis over from +// agent-completion-legacy-live-children.test.ts. +// +// Recognition used to read `recognizeAgentProcess(result.foregroundProcess)`. +// Moving it onto the evidence union made it read +// `evidence.foreground.processName` and dropped the legacy field, so a sample +// whose union reports `{observed, processName: null}` beside a legacy +// `foregroundProcess: 'codex'` no longer recognizes an agent. The union's +// foreground verdict is still inside the vocabulary, so the out-of-vocabulary +// guard cannot refuse it either, and the sample falls straight through to the +// exit gate and completes the agent that is still in the foreground. +// +// Only the `null` pole of the legacy scalar is the lossy collapse this batch +// removes; it conflates "no agent in the foreground" with "could not ask". A +// recognized non-null name is a positive observation on every producer: +// buildPtyProcessInspectionWireResult copies it straight off an `observed` +// foreground verdict, and the ps/pgrep probes only set it from a command line +// the host actually read back. Discarding a positive liveness observation to +// honour a union that reports no name is the #16900/#16908 polarity bug +// mirrored onto the foreground axis, so the exit gate must refuse whenever +// either half still names a recognized agent. +// +// No mock is needed: readPtyProcessInspectionEvidence never cross-checks the +// legacy scalar against the union, so this shape flows through the real reader +// exactly as its children-axis mirror does. +function unnamedForegroundWithLegacyAgent( + legacyForegroundProcess: string +): RuntimeTerminalProcessInspection { + return { + // The legacy field positively names a recognized agent; the union does not. + foregroundProcess: legacyForegroundProcess, + hasChildProcesses: false, + processEvidence: { + foreground: { verdict: 'observed', processName: null }, + children: { verdict: 'exited' } + } + } +} + +function conforming( + foregroundProcess: string | null, + childrenVerdict: 'live' | 'exited' +): RuntimeTerminalProcessInspection { + return { + foregroundProcess, + hasChildProcesses: childrenVerdict === 'live', + processEvidence: { + foreground: { verdict: 'observed', processName: foregroundProcess }, + children: { verdict: childrenVerdict } + } + } +} + +describe('agent completion refuses an exit the legacy foreground field contradicts', () => { + useAgentCompletionCoordinatorLifecycle() + + function startCoordinator(results: () => RuntimeTerminalProcessInspection) { + const dispatchCompletion = vi.fn() + const pollTimes: number[] = [] + const inspectProcess = vi.fn(async () => { + pollTimes.push(Date.now()) + return results() + }) + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess, + dispatchCompletion, + isLive: () => true + }) + coordinator.startProcessTracking() + return { coordinator, dispatchCompletion, pollTimes } + } + + it('never completes while the legacy field still names the running agent', async () => { + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + // The union's foreground verdict is 'observed' and its children verdict is + // positively 'exited', so neither the out-of-vocabulary guard nor the + // positive-exited arm can refuse this sample, and the legacy children + // boolean agrees with 'exited'. The only half of the inspection that still + // reports the agent is the legacy foreground scalar. + result = unnamedForegroundWithLegacyAgent('codex') + await vi.advanceTimersByTimeAsync(60_000) + + expect(dispatchCompletion).not.toHaveBeenCalled() + }) + + it('refuses on a legacy agent name that differs from the one it is completing', async () => { + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + // A recognized agent in the foreground contradicts "the agent exited" + // whichever agent it is; the client cannot tell which half of a + // disagreeing peer's report is the stale one. + result = unnamedForegroundWithLegacyAgent('claude') + await vi.advanceTimersByTimeAsync(60_000) + + expect(dispatchCompletion).not.toHaveBeenCalled() + }) + + it('still completes once the same host publishes a coherent unnamed foreground', async () => { + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + result = unnamedForegroundWithLegacyAgent('codex') + await vi.advanceTimersByTimeAsync(60_000) + expect(dispatchCompletion).not.toHaveBeenCalled() + + // Refusing the contradicting sample must not wedge the monitor. An + // unrecognized legacy name is the normal post-exit shell, not a positive + // agent observation, so it must still complete. + result = conforming('zsh', 'exited') + await vi.advanceTimersByTimeAsync(30_000) + + expect(dispatchCompletion).toHaveBeenCalledTimes(1) + expect(dispatchCompletion).toHaveBeenCalledWith('codex', { + source: 'process-exit', + quietedHookDone: false, + terminalIdleConfirmed: true + }) + }) + + it('treats the contradicting sample as a readable inspection, not a failed one', async () => { + // Both halves parsed — they only disagree — so refusing must not charge an + // inspection error. Charging one would slide a long-running agent down the + // error backoff (2x the active tier on the first error) for free. Jitter is + // pinned to 1.0 by the shared lifecycle. + const activeTier = POLL_TIER_INTERVAL_MS.active + let result = conforming('codex', 'live') + const { dispatchCompletion, pollTimes } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(4_000) + expect(pollTimes.length).toBeGreaterThan(2) + expect(pollTimes[2] - pollTimes[1]).toBe(activeTier) + + const switchedAt = Date.now() + result = unnamedForegroundWithLegacyAgent('codex') + await vi.advanceTimersByTimeAsync(10_000) + + expect(dispatchCompletion).not.toHaveBeenCalled() + const firstContradiction = pollTimes.findIndex((time) => time > switchedAt) + expect(firstContradiction).toBeGreaterThan(-1) + expect(pollTimes.length).toBeGreaterThan(firstContradiction + 1) + expect(pollTimes[firstContradiction + 1] - pollTimes[firstContradiction]).toBe(activeTier) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts b/src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts index 66cdaf85833..5967e041834 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts @@ -108,7 +108,14 @@ export function createAgentCompletionProcessMonitor({ // its `false` conflates "no children" with "could not ask", while every // producer sets `true` from a positive observation. So a `true` beside a // disagreeing 'exited' must refuse, exactly as the boolean-only gate did. - if (evidence.children.verdict !== 'exited' || result.hasChildProcesses) { + // The legacy foreground scalar is the same rule one axis over: only its + // `null` conflates "no agent" with "could not ask", so a still-recognized + // name beside a union reporting none must refuse too. + if ( + evidence.children.verdict !== 'exited' || + result.hasChildProcesses || + recognizeAgentProcess(result.foregroundProcess) !== null + ) { state.pendingProcessExitAgent = null scheduleNextPoll() return false