From ab128cb13b7d9ded9d89d042ace319b247a26e1e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:48:36 -0700 Subject: [PATCH] fix(daemon): keep agent-completion detection alive on pre-v27 daemons (#10478) * fix(daemon): keep agent-completion detection alive on pre-v27 daemons DaemonPtyAdapter.inspectProcess() threw terminal_liveness_unavailable when the connected daemon predated protocol v27. The intended provider-level fallback only fires when a provider lacks inspectProcess, so for the daemon adapter the throw propagated: agent-completion-coordinator swallowed it into consecutiveInspectionErrors and retried forever, killing process-exit completions and pending-title validation. Daemons intentionally survive app updates, so updating in place with agent terminals open routes those PTYs to a legacy adapter and permanently disables agent-finished notifications until the terminal is recreated. Compose the inspection client-side from getForegroundProcess, which v26 fully supports. No new wire traffic and no new daemon capability. * test(daemon): pin null-foreground semantics on the pre-v27 inspect fallback The legacy composition had no coverage for a null foreground, which is the one daemon response shape whose semantics diverge from v27: there inspectProcess goes through getAliveSession() and throws for a vanished session, while getForegroundProcess is deliberately null-not-throw. It is also the only shape that reaches a user-visible completion, so reading it as idle is a deliberate choice that should not change silently. (cherry picked from commit 6592c01592d993df37c169c8602271e6c8cb0e16) --- src/main/daemon/daemon-pty-adapter.test.ts | 90 ++++++++++++++++++++++ src/main/daemon/daemon-pty-adapter.ts | 21 ++++- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 089d1a59910..2ad8ef28fce 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -6,6 +6,7 @@ import { mkdtempSync, mkdirSync, rmSync, existsSync, readFileSync, writeFileSync import { DaemonClient } from './client' import { DaemonProtocolError } from './daemon-errors' import { DaemonPtyAdapter } from './daemon-pty-adapter' +import { COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION } from './daemon-protocol-version' import { DaemonServer } from './daemon-server' import { HeadlessEmulator } from './headless-emulator' import { getHistorySessionDirName } from './history-paths' @@ -927,6 +928,95 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) }) + describe('inspectProcess on pre-inspection daemon protocols', () => { + // Why: daemons outlive an in-place app update, so a v26 daemon must still answer inspections + // client-side; throwing here leaves agent-completion detection permanently retrying. + type ClientInternals = { + client: { request: ReturnType; disconnect: ReturnType } + } + + function createLegacyAdapter(request: ReturnType): DaemonPtyAdapter { + const legacy = new DaemonPtyAdapter({ + socketPath, + tokenPath, + protocolVersion: COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION - 1 + }) + ;(legacy as unknown as ClientInternals).client = { request, disconnect: vi.fn() } + return legacy + } + + it('composes a real inspection from the foreground call the daemon does support', async () => { + const request = vi.fn(async () => ({ foregroundProcess: 'codex' })) + const legacy = createLegacyAdapter(request) + + expect(await legacy.inspectProcess('sess-a')).toEqual({ + foregroundProcess: 'codex', + hasChildProcesses: true + }) + // Why: the fallback must not depend on a capability the legacy daemon lacks. + expect(request).toHaveBeenCalledWith('getForegroundProcess', { sessionId: 'sess-a' }) + expect(request).not.toHaveBeenCalledWith('inspectProcess', expect.anything()) + + legacy.dispose() + }) + + it('reports an idle shell as having no child processes', async () => { + const legacy = createLegacyAdapter(vi.fn(async () => ({ foregroundProcess: 'bash' }))) + + expect(await legacy.inspectProcess('sess-a')).toEqual({ + foregroundProcess: 'bash', + hasChildProcesses: false + }) + + legacy.dispose() + }) + + it('reports a null foreground as idle, matching what the legacy daemon can report', async () => { + // Why: pins the one response shape whose semantics differ from v27, where inspectProcess goes + // through getAliveSession() and throws for a vanished session while getForegroundProcess stays + // null-not-throw. Reading it as idle is deliberate — this is also the only shape that reaches a + // user-visible completion — so the divergence must not change silently. + const legacy = createLegacyAdapter(vi.fn(async () => ({ foregroundProcess: null }))) + + expect(await legacy.inspectProcess('sess-a')).toEqual({ + foregroundProcess: null, + hasChildProcesses: false + }) + + legacy.dispose() + }) + + it('rejects rather than reading as idle when the daemon call fails', async () => { + // Why: getForegroundProcess swallows errors into null; composing through it would turn a dead + // socket into a false "agent exited" completion, the mirror of the bug this path fixes. + const legacy = createLegacyAdapter( + vi.fn(async () => { + throw new Error('socket_closed') + }) + ) + + await expect(legacy.inspectProcess('sess-a')).rejects.toThrow('socket_closed') + + legacy.dispose() + }) + + it('still delegates to the daemon once the protocol supports inspectProcess', async () => { + const request = vi.fn(async () => ({ foregroundProcess: 'codex', hasChildProcesses: true })) + const current = new DaemonPtyAdapter({ + socketPath, + tokenPath, + protocolVersion: COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION + }) + ;(current as unknown as ClientInternals).client = { request, disconnect: vi.fn() } + + await current.inspectProcess('sess-a') + + expect(request).toHaveBeenCalledWith('inspectProcess', { sessionId: 'sess-a' }) + + current.dispose() + }) + }) + describe('serialize / revive', () => { it('serialize returns JSON', async () => { const { id } = await adapter.spawn({ cols: 80, rows: 24 }) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 5127354cc97..8d0cdf796fa 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -818,17 +818,30 @@ export class DaemonPtyAdapter implements IPtyProvider { // No flow control for daemon-backed terminals } - async hasChildProcesses(id: string): Promise { - const foregroundProcess = await this.getForegroundProcess(id) - // Why: daemon-backed PTYs can host long-lived agents while detached; cleanup prompts must not treat them as idle shells. + // Why: daemon-backed PTYs can host long-lived agents while detached; cleanup prompts must not treat them as idle shells. + private hasChildProcessesFromForeground(foregroundProcess: string | null): boolean { return foregroundProcess !== null && !isShellProcess(foregroundProcess) } + async hasChildProcesses(id: string): Promise { + return this.hasChildProcessesFromForeground(await this.getForegroundProcess(id)) + } + async inspectProcess( id: string ): Promise<{ foregroundProcess: string | null; hasChildProcesses: boolean }> { if (this.protocolVersion < COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION) { - throw new Error('terminal_liveness_unavailable') + // Why: pre-v27 daemons survive an in-place app update; compose the inspection client-side from the + // one call they do support instead of throwing, or completion detection stays dead until recreate. + // Requests directly (not via getForegroundProcess) so a dead socket still rejects rather than + // reading as an idle foreground and dispatching a false completion. + const { foregroundProcess } = await this.client.request<{ + foregroundProcess: string | null + }>('getForegroundProcess', { sessionId: id }) + return { + foregroundProcess, + hasChildProcesses: this.hasChildProcessesFromForeground(foregroundProcess) + } } return this.client.request<{ foregroundProcess: string | null