From 84880f27115ff68a14c85ffb6fdbf7ec6bf9d7ef Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 19:04:25 -0700 Subject: [PATCH] fix(notifications): gate the Codex lead-Stop banner without downgrading its state The first cut of this gate rewrote a lead Stop's `state` from 'done' to 'working' when the roster still held live children. That silently broke the opposite contract in src/main/agent-hooks/server.ts: a root Stop retires child rows and reports 'done'. Two pre-existing tests in server.test.ts went red. Both contracts are real and can't both live in `state`. The reap is also what destroys the signal: after it, a lead Stop and the final SubagentStop are byte-identical (state=done, subagents=undefined), so no downstream heuristic can tell a mid-turn stop from a turn end. So capture it before the reap and carry it as a live-only field. Normalization keeps reporting 'done'; the completion coordinator reads the flag and suppresses only the notification, which is all #4375 ever asked for. Stripped on persist so a rehydrated row can't re-suppress the first real completion after restart. --- src/main/agent-hooks/server.ts | 6 +++ .../agent-completion-coordinator.ts | 7 ++++ .../codex-lead-stop-subagent-gate.test.ts | 37 +++++++++++++++++++ src/shared/agent-hook-listener.ts | 28 +++++++++----- src/shared/agent-status-types.ts | 10 ++++- 5 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/main/agent-hooks/server.ts b/src/main/agent-hooks/server.ts index 06dfb151560..e8cf6c9ef26 100644 --- a/src/main/agent-hooks/server.ts +++ b/src/main/agent-hooks/server.ts @@ -1927,6 +1927,12 @@ export class AgentHookServer { continue } const { promptInteractionKey: _promptInteractionKey, ...persistedPayload } = payload + // Why: leadStopWithLiveSubagents describes one live hook delivery, not restorable pane + // state — a rehydrated one would re-suppress the first completion after restart (#4375). + if (persistedPayload.payload?.leadStopWithLiveSubagents) { + const { leadStopWithLiveSubagents: _live, ...inner } = persistedPayload.payload + persistedPayload.payload = inner + } entries[paneKey] = persistedPayload as EnrichedAgentHookEventPayload } const file: LastStatusFile = { version: LAST_STATUS_FILE_VERSION, entries } diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts index b948cf43da6..4fa9b3900c2 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts @@ -811,6 +811,13 @@ export function createAgentCompletionCoordinator( return } if (isCompletionHookState(payload.state)) { + if (payload.leadStopWithLiveSubagents) { + // Why: a Codex lead Stop raised while its children still run ends no turn (#4375). + // It stays `done` for the sidebar, but notifying here is the reported spam. Cancel any + // provisional done too — the lead can Stop inside a quiet window opened by an earlier one. + clearPendingHookDone() + return + } // Why: the turn is ending, so a debounced attention from an earlier pause must not fire after completion. clearPendingCodexAttention() if (isRecognizedAgentType(payload.agentType)) { diff --git a/src/renderer/src/components/terminal-pane/codex-lead-stop-subagent-gate.test.ts b/src/renderer/src/components/terminal-pane/codex-lead-stop-subagent-gate.test.ts index 9863447a72e..dea820f72f9 100644 --- a/src/renderer/src/components/terminal-pane/codex-lead-stop-subagent-gate.test.ts +++ b/src/renderer/src/components/terminal-pane/codex-lead-stop-subagent-gate.test.ts @@ -242,6 +242,43 @@ describe('issue #4375 — non-turn-end notification spam', () => { expect(restarted?.payload.state).toBe('working') }) + it('gates notification without downgrading the reported state of a lead Stop', () => { + // Why: an earlier cut of this fix gated by rewriting `state` to 'working', which silently + // broke the sidebar/persistence contract that a root Stop retires child rows and reports + // 'done' (server.test.ts). The suppression signal must ride alongside `done`, not replace it. + const listenerState: HookListenerState = createHookListenerState() + const send = ( + ev: string, + p?: Record + ): ReturnType => + normalizeHookPayload( + listenerState, + 'codex', + { + paneKey: PANE_KEY, + tabId: 'tab-1', + worktreeId: WORKTREE_ID, + hook_event_name: ev, + payload: { hook_event_name: ev, ...p } + }, + 'test-env' + ) + + send('UserPromptSubmit', { prompt: 'go' }) + send('SubagentStart', { agent_id: 'child-1', agent_type: 'reviewer' }) + + const leadStop = send('Stop') + expect(leadStop?.payload.state).toBe('done') + expect(leadStop?.payload.subagents).toBeUndefined() + expect(leadStop?.payload.leadStopWithLiveSubagents).toBe(true) + + // The child's own Stop is a real turn end: same shape, but no suppression flag. + send('PreToolUse', { agent_id: 'child-1', tool_name: 'Bash', tool_input: { command: 'y' } }) + const childStop = send('SubagentStop', { agent_id: 'child-1' }) + expect(childStop?.payload.state).toBe('done') + expect(childStop?.payload.leadStopWithLiveSubagents).toBeUndefined() + }) + // KNOWN-OPEN residual, deliberately out of scope for the roster fix. A bare mid-turn // Stop (no subagent) that resumes AFTER the 1.5s quiet window still notifies. There is // no principled bound to widen the window to, and widening it delays every real Codex diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index 36f60d6308d..1d2630b5a9b 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -3169,7 +3169,11 @@ function buildCodexStatusPayload( promptText: string, paneKey: string, hookPayload: Record, - options: { stateName: 'working' | 'waiting' | 'done'; updateLead: boolean } + options: { + stateName: 'working' | 'waiting' | 'done' + updateLead: boolean + leadStopWithLiveSubagents?: boolean + } ): ParsedAgentStatusPayload | null { const snapshot = options.updateLead ? resolveToolState(state, paneKey, extractToolFields('codex', eventName, hookPayload), { @@ -3189,7 +3193,8 @@ function buildCodexStatusPayload( toolInput: snapshot.toolInput, interactivePrompt: snapshot.interactivePrompt, lastAssistantMessage: snapshot.lastAssistantMessage, - subagents: codexRosterToSnapshots(state.codexSubagentRosterByPaneKey.get(paneKey)) + subagents: codexRosterToSnapshots(state.codexSubagentRosterByPaneKey.get(paneKey)), + leadStopWithLiveSubagents: options.leadStopWithLiveSubagents }) } @@ -3283,12 +3288,14 @@ function normalizeCodexEvent( return buildCodexChildDrivenStatusPayload(state, eventName, paneKey, hookPayload) } - // Why: a lead Stop isn't "done" while children still run, so the roster must gate it before - // being retired. SessionStart is exempt — its roster belongs to the process that just exited. - const effectiveState = - eventName === 'SessionStart' - ? stateName - : codexRosterEffectiveState(state.codexSubagentRosterByPaneKey.get(paneKey), stateName) + // Why: the roster is retired below, which makes a lead Stop indistinguishable from the final + // SubagentStop (both `done` with no children). Capture whether children were still live BEFORE + // the reap so notification can tell a real turn end from a mid-turn one (#4375). SessionStart is + // exempt — its roster belongs to the process that just exited. + const leadStopWithLiveSubagents = + eventName === 'Stop' && + codexRosterEffectiveState(state.codexSubagentRosterByPaneKey.get(paneKey), stateName) !== + stateName if (eventName === 'SessionStart') { // Why: a pane can host a new Codex process after the old one exited without child Stop hooks. state.codexSubagentRosterByPaneKey.delete(paneKey) @@ -3304,8 +3311,9 @@ function normalizeCodexEvent( (eventName === 'SessionStart' ? undefined : previousLead?.model) }) return buildCodexStatusPayload(state, eventName, promptText, paneKey, hookPayload, { - stateName: effectiveState, - updateLead: true + stateName, + updateLead: true, + leadStopWithLiveSubagents }) } diff --git a/src/shared/agent-status-types.ts b/src/shared/agent-status-types.ts index 1968ee0890f..5a5497d2393 100644 --- a/src/shared/agent-status-types.ts +++ b/src/shared/agent-status-types.ts @@ -165,6 +165,11 @@ export type AgentStatusPayload = { interrupted?: boolean /** Live in-process children of the reporting session. See AgentStatusEntry. */ subagents?: AgentSubagentSnapshot[] + /** Live-only: this `done` is a Codex lead Stop raised while subagents were still + * running, so it ends no turn. Set before the roster is retired, which is what + * otherwise makes it identical to the final SubagentStop (#4375). Notification-only — + * `state` stays `done` so sidebar/persistence behavior is unchanged. */ + leadStopWithLiveSubagents?: boolean } /** @@ -361,7 +366,10 @@ function normalizeAgentStatusObject(parsed: unknown): ParsedAgentStatusPayload | ), // Why: only meaningful on `done`; coerce to undefined elsewhere so it can't leak stale truth across transitions. interrupted: obj.interrupted === true && state === 'done' ? true : undefined, - subagents: normalizeSubagentsField(obj.subagents) + subagents: normalizeSubagentsField(obj.subagents), + // Why: like `interrupted`, only meaningful on `done`; anywhere else it would be stale truth. + leadStopWithLiveSubagents: + obj.leadStopWithLiveSubagents === true && state === 'done' ? true : undefined } }