From 2b56c94149cbbb3454656ee003b27a7c5f10ff72 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:11:11 -0700 Subject: [PATCH] test(completion): pin the positive-exited arm independently of the vocabulary guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two hardening arms were mutually redundant in coverage: reverting either alone left the suite green. The out-of-vocabulary guard is now pinned on its own; this pins the other half. Given the vocabulary guard, every sample reaching the exit gate already carries 'live' or 'exited'. On a host that builds its wire result with buildPtyProcessInspectionWireResult the legacy `hasChildProcesses` boolean is exactly `children.verdict === 'live'`, so reading the boolean and requiring a positive 'exited' agree — the arm is invisible there. They part on the shape the reader is written to survive: a peer this client cannot vouch for, whose two fields disagree. readPtyProcessInspectionEvidence never cross-checks the legacy scalar against the evidence union, so `hasChildProcesses: false` can arrive beside a positively observed `children: 'live'`. That boolean's `false` is the lossy collapse conflating "no children" with "could not tell" (#16900/#16908). The evidence union is authoritative, so 'live' must refuse even though the legacy field waves it through. Reverting the positive-exited arm alone turns two of these red with a false `process-exit` completion; reverting the vocabulary guard alone leaves all three green. --- ...mpletion-positive-exit-requirement.test.ts | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 src/renderer/src/components/terminal-pane/agent-completion-positive-exit-requirement.test.ts diff --git a/src/renderer/src/components/terminal-pane/agent-completion-positive-exit-requirement.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-positive-exit-requirement.test.ts new file mode 100644 index 00000000000..e49824d1502 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/agent-completion-positive-exit-requirement.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, it, vi } from 'vitest' +import { createAgentCompletionCoordinator } from './agent-completion-coordinator' +import { useAgentCompletionCoordinatorLifecycle } from './agent-completion-coordinator-test-harness' +import type { RuntimeTerminalProcessInspection } from '@/runtime/runtime-terminal-inspection' + +// Pins the positive-exited arm on its own, independently of the +// out-of-vocabulary guard. +// +// Given the vocabulary guard, every sample that reaches the exit gate already +// carries a children verdict of 'live' or 'exited'. Against a host that builds +// its wire result with buildPtyProcessInspectionWireResult the legacy +// `hasChildProcesses` boolean is exactly `children.verdict === 'live'`, so on +// that host reading the boolean and requiring a positive 'exited' agree — the +// arm is invisible. +// +// They part company on the one shape the reader is explicitly written to +// survive: a peer this client cannot vouch for, whose two fields disagree. +// `readPtyProcessInspectionEvidence` never cross-checks the legacy scalar +// against the evidence union, so `hasChildProcesses: false` can arrive beside a +// positively observed `children: 'live'`. The boolean's `false` is the lossy +// collapse that conflates "no children" with "could not tell" — the polarity +// bug from #16900/#16908. The evidence union is the authoritative vocabulary, +// so `live` must refuse completion even though the legacy field would wave it +// through. No mock is needed: this shape flows through the real reader. +function liveChildrenWithLegacyFalse( + foregroundProcess: string | null +): RuntimeTerminalProcessInspection { + return { + foregroundProcess, + // The lossy legacy collapse disagrees with the authoritative evidence. + hasChildProcesses: false, + processEvidence: { + foreground: { verdict: 'observed', processName: foregroundProcess }, + children: { verdict: 'live' } + } + } +} + +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 requires a positively matched exited verdict', () => { + useAgentCompletionCoordinatorLifecycle() + + function startCoordinator(results: () => RuntimeTerminalProcessInspection) { + const dispatchCompletion = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(async () => results()), + dispatchCompletion, + isLive: () => true + }) + coordinator.startProcessTracking() + return { coordinator, dispatchCompletion } + } + + it('never completes while children are positively live, even when the legacy field says otherwise', async () => { + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + // Foreground is observed and inside the vocabulary, and so is the children + // verdict — the vocabulary guard has nothing to refuse here. The only arm + // that can still refuse is the one that demands a positive 'exited', and + // it must, because the host positively observed live children. + result = liveChildrenWithLegacyFalse('zsh') + await vi.advanceTimersByTimeAsync(60_000) + + expect(dispatchCompletion).not.toHaveBeenCalled() + }) + + it('still completes once the same host positively reports exited', async () => { + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + result = liveChildrenWithLegacyFalse('zsh') + await vi.advanceTimersByTimeAsync(60_000) + expect(dispatchCompletion).not.toHaveBeenCalled() + + // Refusing the disagreeing sample must not wedge the monitor. + 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 a positively live sample as a readable inspection, not a failed one', async () => { + // The vocabulary guard re-arms with an inspection-error backoff because the + // sample could not be read. A 'live' sample was read fine — it just is not + // an exit — so the exit arm must refuse without charging an error, or a + // long-running agent would slide down the error backoff for free. + let result = conforming('codex', 'live') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + result = liveChildrenWithLegacyFalse('zsh') + await vi.advanceTimersByTimeAsync(20_000) + + // A single conforming exit sample confirms against the pending exit that + // the live samples left standing; had they been charged as errors, the + // pending exit would have been cleared and re-armed instead. + result = conforming('zsh', 'exited') + await vi.advanceTimersByTimeAsync(30_000) + + expect(dispatchCompletion).toHaveBeenCalledTimes(1) + }) +})