From 25128f58bf6e1a961bb54f89bda53b4ccfb151c9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 18 Aug 2026 02:44:43 -0700 Subject: [PATCH] fix(agent-status): close four tri-state evidence gaps found in review (STA-4119) Independent review of the snapshot/replay fix found four ways the tri-state could report a positive "no background work" from evidence nobody reported, or stay "never observed" forever. All four are the same failure in different places: the new per-runtime observation marker was not kept in step with the inventory sets it qualifies. - Interrupt: clearing the inventory sets means "stop gating the pane", not "the provider reported an all-clear". The marker survived, so the resolver saw "observed, nothing running" and emitted a positive false over a process that may still be live. Interrupts now drop the marker in all three paths. - Session cron: a scheduled cron is provider-owned work under the same PTY, but the resolver consulted only the background-shell set and a cron-only inventory never set the marker. Such a pane was unknown forever, or reported a false all-clear from stale state. Cron now both counts as an observation and resolves as work. - Remote/relay: a host reports the inventory as a wire boolean, but the receiving runtime never recorded that an observation happened, so every remote pane resolved to unknown and a relayed all-clear could never make one eligible. - Local-hook rollback: the save/restore dance that protects against nested or rejected payloads tracked three fields and not the marker, so a discarded payload could leave the pane marked observed with the sets rolled back. Each is pinned by a test whose corresponding production revert makes it red. --- .../server-hook-http-ingest.test.ts | 6 ++ ...er-status-snapshot-background-work.test.ts | 83 +++++++++++++++++++ src/main/agent-hooks/server.ts | 36 ++++++-- src/shared/agent-hook-listener.ts | 22 ++++- 4 files changed, 140 insertions(+), 7 deletions(-) diff --git a/src/main/agent-hooks/server-hook-http-ingest.test.ts b/src/main/agent-hooks/server-hook-http-ingest.test.ts index 622fc7d428b..43f5a77beb4 100644 --- a/src/main/agent-hooks/server-hook-http-ingest.test.ts +++ b/src/main/agent-hooks/server-hook-http-ingest.test.ts @@ -125,6 +125,12 @@ describe('AgentHookServer listener replay', () => { expect(server.getStatusSnapshot()[0]).toEqual(waiting) expect(server._getStateForTests().claudeRunningNonAgentTaskPaneKeys.has(PANE)).toBe(true) expect(server._getStateForTests().claudeActiveSessionCronPaneKeys.has(PANE)).toBe(true) + // Why: the observation marker rides the same snapshot/restore as the sets above. A rejected + // payload must not be able to move it either, or the tri-state could resolve from evidence + // that was never accepted. + expect(server._getStateForTests().claudeBackgroundInventoryObservedPaneKeys.has(PANE)).toBe( + true + ) } finally { server.stop() } diff --git a/src/main/agent-hooks/server-status-snapshot-background-work.test.ts b/src/main/agent-hooks/server-status-snapshot-background-work.test.ts index 4d1cbd96769..f3645c25271 100644 --- a/src/main/agent-hooks/server-status-snapshot-background-work.test.ts +++ b/src/main/agent-hooks/server-status-snapshot-background-work.test.ts @@ -79,4 +79,87 @@ describe('status snapshot carries provider background-work evidence', () => { server.stop() } }) + + // Why: an interrupt CLEARS the inventory sets to stop gating the pane. That is not the provider + // reporting an all-clear, so it must leave the pane "never observed" (unknown) rather than + // manufacturing a positive `false` over a process that may still be running. + it('reports unknown, not a false all-clear, after an interrupt', async () => { + const server = new AgentHookServer() + await server.start({ env: 'production', userDataPath }) + try { + await postHookEvent( + server, + buildBody({ hook_event_name: 'UserPromptSubmit', prompt: 'start the dev server' }) + ) + await postHookEvent( + server, + buildBody({ hook_event_name: 'Stop', background_tasks: [RUNNING_SHELL] }) + ) + expect( + server.getStatusSnapshot().find((entry) => entry.paneKey === PANE) + ?.providerBackgroundWorkActive + ).toBe(true) + + await postHookEvent( + server, + buildBody({ hook_event_name: 'Stop', is_interrupt: true, background_tasks: [] }) + ) + + expect( + server.getStatusSnapshot().find((entry) => entry.paneKey === PANE) + ?.providerBackgroundWorkActive + ).toBeUndefined() + } finally { + server.stop() + } + }) + + // Why: a scheduled session cron is provider-owned work under this PTY just as a background shell + // is. A cron-only inventory must both COUNT as an observation and resolve to live work. + it('treats a cron-only inventory as observed live background work', async () => { + const server = new AgentHookServer() + await server.start({ env: 'production', userDataPath }) + try { + await postHookEvent( + server, + buildBody({ hook_event_name: 'UserPromptSubmit', prompt: 'schedule a job' }) + ) + await postHookEvent( + server, + buildBody({ hook_event_name: 'Stop', session_crons: [{ id: 'cron-1' }] }) + ) + + const row = server.getStatusSnapshot().find((entry) => entry.paneKey === PANE) + expect(row?.providerBackgroundWorkActive).toBe(true) + } finally { + server.stop() + } + }) + + // Why: a relayed host reports the inventory as a wire boolean. If the receiving runtime does not + // record that an observation happened, every remote pane stays "never observed" forever and a + // relayed all-clear can never make one eligible. + it('records remote/relayed background-work evidence as an observation', async () => { + const server = new AgentHookServer() + await server.start({ env: 'production', userDataPath }) + try { + server.ingestRemote( + { + paneKey: PANE, + tabId: 'tab-1', + worktreeId: 'wt-1', + hookEventName: 'Stop', + claudeRunningNonAgentTask: false, + payload: { state: 'done', prompt: 'remote turn', agentType: 'claude' } + }, + 'conn-remote-1' + ) + + const row = server.getStatusSnapshot().find((entry) => entry.paneKey === PANE) + expect(row?.state).toBe('done') + expect(row?.providerBackgroundWorkActive).toBe(false) + } finally { + server.stop() + } + }) }) diff --git a/src/main/agent-hooks/server.ts b/src/main/agent-hooks/server.ts index 9dec49c803c..24df3ebf758 100644 --- a/src/main/agent-hooks/server.ts +++ b/src/main/agent-hooks/server.ts @@ -128,6 +128,10 @@ type ClaudeBackgroundEvidence = { hasRunningTask: boolean hasUnclassifiedTask: boolean hasActiveCron: boolean + /** Why: the rollback below must restore this too. A nested/duplicate/rejected local payload that + * carried an inventory would otherwise leave the pane marked "observed" with the running sets + * rolled back, and the tri-state would resolve to a positive `false` nobody reported. */ + hasObservedInventory: boolean } type NormalizedLocalHook = { @@ -1554,9 +1558,16 @@ export class AgentHookServer { if (agentType !== 'claude') { return undefined } - return this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey) - ? this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey) - : undefined + if (!this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey)) { + return undefined + } + // Why: a scheduled session cron is provider-owned work under this PTY exactly as a background + // shell is, and the wire definition already treats both as the broad signal. Omitting cron + // here would report an all-clear over a pane that still has work queued. + return ( + this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey) || + this.state.claudeActiveSessionCronPaneKeys.has(paneKey) + ) } /** Stamps the resolved evidence so every accepted row restates it, on the live push and on the @@ -2186,13 +2197,19 @@ export class AgentHookServer { return { hasRunningTask: this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey), hasUnclassifiedTask: this.state.claudeUnclassifiedBackgroundTaskPaneKeys.has(paneKey), - hasActiveCron: this.state.claudeActiveSessionCronPaneKeys.has(paneKey) + hasActiveCron: this.state.claudeActiveSessionCronPaneKeys.has(paneKey), + hasObservedInventory: this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey) } } private setClaudeBackgroundEvidence( paneKey: string, - { hasRunningTask, hasUnclassifiedTask, hasActiveCron }: ClaudeBackgroundEvidence + { + hasRunningTask, + hasUnclassifiedTask, + hasActiveCron, + hasObservedInventory + }: ClaudeBackgroundEvidence ): void { if (hasRunningTask) { this.state.claudeRunningNonAgentTaskPaneKeys.add(paneKey) @@ -2209,6 +2226,11 @@ export class AgentHookServer { } else { this.state.claudeActiveSessionCronPaneKeys.delete(paneKey) } + if (hasObservedInventory) { + this.state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey) + } else { + this.state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey) + } } ingestTerminalStatus(event: { @@ -2558,6 +2580,10 @@ export class AgentHookServer { } else { this.state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey) } + // Why: the host DID report an inventory — that is what put the boolean on the wire. + // Without recording it the tri-state stays "never observed" for every remote pane and + // a relayed `false` can never make one eligible. + this.state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey) } : undefined ) diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index 9de5e9e2392..cce760b49af 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -2663,8 +2663,14 @@ function updateClaudeRunningNonAgentTask( state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey) } // Why: reached only when the payload actually carried background_tasks, so this is the one - // point where "Claude told us what is running" becomes true for the pane. - state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey) + // point where "Claude told us what is running" becomes true for the pane. An interrupt is + // excluded for the same reason the sets above are cleared: it ungates the pane rather than + // reporting an all-clear. + if (interrupted) { + state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey) + } else { + state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey) + } } function resolveClaudePaneState( @@ -2785,6 +2791,10 @@ export function markClaudeLeadTurnInterrupted(state: HookListenerState, paneKey: state.claudeRunningNonAgentTaskPaneKeys.delete(paneKey) state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey) state.claudeActiveSessionCronPaneKeys.delete(paneKey) + // Why: clearing the inventory sets here means "stop gating the pane", NOT "the provider reported + // an all-clear". Leaving the observation behind would let the tri-state resolve to a positive + // `false` from evidence nobody reported, over a process that may still be running. + state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey) } /** Rebuild a pane's working roster from a persisted snapshot; live activity confirms a seed, a complete task inventory may reap an unconfirmed one whose finish hook arrived while Orca was offline. */ @@ -3053,6 +3063,11 @@ function normalizeClaudeEvent( } else { state.claudeActiveSessionCronPaneKeys.delete(paneKey) } + // Why: a cron inventory is background-work evidence too. Without this a cron-only pane stays + // "never observed" forever and can never become hibernatable. + if (interrupted !== true) { + state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey) + } } else if (eventAgentId === undefined && isTurnBoundary && backgroundTasks.present) { // Why: current Claude may omit an empty cron inventory while still emitting background_tasks. state.claudeActiveSessionCronPaneKeys.delete(paneKey) @@ -3160,6 +3175,9 @@ function normalizeClaudeEvent( state.claudeRunningNonAgentTaskPaneKeys.delete(paneKey) state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey) state.claudeActiveSessionCronPaneKeys.delete(paneKey) + // Why: an interrupt ungates the pane; it is not a reported all-clear. See + // markClaudeLeadTurnInterrupted. + state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey) } const effectiveState = resolveClaudePaneState(state, paneKey, {