diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3a03a197746..d00a8fb609f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,10 @@ - Memoized non-message token totals (system prompt, tool schemas, skills) so the per-turn compaction and context-threshold paths recompute them at most once per input change instead of on every call. `getContextBreakdown` and `#estimateStoredContextTokens` previously re-tokenized the system prompt and every tool's wire schema (per-tool `JSON.stringify`) several times per turn over inputs that change at most once per turn. +### Fixed + +- Fixed late advisor concerns after a terminal primary answer starting a duplicate primary turn when no queued work remains ([#4840](https://github.com/can1357/oh-my-pi/issues/4840)). + ## [16.3.11] - 2026-07-06 ### Changed diff --git a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts index 994a8365914..bb138badba5 100644 --- a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts +++ b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts @@ -1562,6 +1562,20 @@ describe("advisor", () => { } }); + it("preserves a late interrupting note when the primary already ended with a terminal answer", () => { + for (const severity of ["concern", "blocker"] as const) { + expect( + resolveAdvisorDeliveryChannel({ + severity, + autoResumeSuppressed: false, + streaming: false, + aborting: false, + terminalAnswerNoQueuedWork: true, + }), + ).toBe("preserve"); + } + }); + it("routes interrupting notes to the aside queue during immune turns without overriding preservation", () => { expect( resolveAdvisorDeliveryChannel({ diff --git a/packages/coding-agent/src/advisor/advise-tool.ts b/packages/coding-agent/src/advisor/advise-tool.ts index bc24a029c42..91a7beba122 100644 --- a/packages/coding-agent/src/advisor/advise-tool.ts +++ b/packages/coding-agent/src/advisor/advise-tool.ts @@ -94,6 +94,9 @@ export function isAdvisorInterruptImmuneTurnActive(opts: { * - An interrupting `concern`/`blocker` is normally steered into the agent: into * the live turn while one is streaming, or (when idle) a triggered turn so the * advice is acted on immediately. + * - If the primary tail is already a terminal text answer and there is no queued + * work, late interrupting advice is preserved as a visible card instead of + * waking the primary to restate completion. * - After a deliberate user interrupt (`autoResumeSuppressed`) the advisor must * not auto-resume the stopped run. While the agent is idle — or still tearing * the interrupted turn down (`aborting`) — the note is preserved as a visible @@ -103,17 +106,19 @@ export function isAdvisorInterruptImmuneTurnActive(opts: { * run instead strands it (it never reaches the running agent) and the withheld * notes dump as one burst at the next user prompt — the bug this guards. * - During the post-interrupt immune-turn window, further `concern`/`blocker` - * notes are downgraded to asides; suppression preservation still wins. + * notes are downgraded to asides; preservation still wins. */ export function resolveAdvisorDeliveryChannel(opts: { severity: AdvisorSeverity | undefined; autoResumeSuppressed: boolean; streaming: boolean; aborting: boolean; + terminalAnswerNoQueuedWork?: boolean; interruptImmuneTurnActive?: boolean; }): AdvisorDeliveryChannel { if (!isInterruptingSeverity(opts.severity)) return "aside"; if (opts.autoResumeSuppressed && (opts.aborting || !opts.streaming)) return "preserve"; + if (opts.terminalAnswerNoQueuedWork && !opts.streaming && !opts.aborting) return "preserve"; if (opts.interruptImmuneTurnActive) return "aside"; return "steer"; } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index c11d0bf3671..0bdc4e9c959 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -1382,6 +1382,21 @@ function isAdvisorCard(message: AgentMessage): message is CustomMessage { return message.role === "custom" && message.customType === "advisor"; } +function isTerminalTextAssistantAnswer(message: AgentMessage | undefined): message is AssistantMessage { + if (message?.role !== "assistant" || message.stopReason !== "stop") return false; + let hasText = false; + for (const part of message.content) { + if (part.type === "toolCall") return false; + if (part.type === "text") { + if (part.text.trim().length > 0) hasText = true; + continue; + } + if (part.type === "thinking") continue; + return false; + } + return hasText; +} + /** * A queued message the user can restore to the editor / pull back as a draft. * Only genuinely user-authored messages qualify: plain user turns, or custom @@ -2601,13 +2616,21 @@ export class AgentSession { /** * Route one accepted advice note from `advisor` to the primary. Concern and * blocker interrupt the running agent through the steering channel; once the - * loop has yielded, `triggerTurn` resumes it. After a deliberate user interrupt - * auto-resume is suppressed while idle/unwinding (the note becomes a preserved - * card re-entering on resume); a live-streaming turn is steered in directly. A - * plain nit always rides the non-interrupting YieldQueue aside. Suppression by - * the per-advisor emission guard drops the note silently — the model still saw - * `Recorded.`, so it isn't tempted to rephrase the same note past the dedupe. + * loop has yielded, `triggerTurn` resumes it. If the loop already ended with a + * terminal text answer and no queued work remains, the note is preserved as an + * advisor card instead of waking a duplicate completion turn. After a deliberate + * user interrupt auto-resume is suppressed while idle/unwinding (the note + * becomes a preserved card re-entering on resume); a live-streaming turn is + * steered in directly. A plain nit always rides the non-interrupting YieldQueue + * aside. Suppression by the per-advisor emission guard drops the note silently — + * the model still saw `Recorded.`, so it isn't tempted to rephrase the same note + * past the dedupe. */ + #hasTerminalTextAnswerWithoutQueuedWork(): boolean { + if (this.agent.hasQueuedMessages() || this.#pendingNextTurnMessages.length > 0) return false; + return isTerminalTextAssistantAnswer(this.agent.state.messages.at(-1)); + } + #routeAdvice(advisor: ActiveAdvisor, note: string, severity?: AdvisorSeverity): void { if (!advisor.emissionGuard.accept(note)) { logger.debug("advisor advice suppressed by emission guard", { severity, advisor: advisor.name }); @@ -2625,6 +2648,7 @@ export class AgentSession { // loop consumes a steer at its next boundary. streaming: this.agent.state.isStreaming, aborting: this.#abortInProgress, + terminalAnswerNoQueuedWork: this.#hasTerminalTextAnswerWithoutQueuedWork(), interruptImmuneTurnActive: interrupting && this.#isAdvisorInterruptImmuneTurnActive(), }); if (channel === "aside") { diff --git a/packages/coding-agent/test/agent-session-advisor-suppression.test.ts b/packages/coding-agent/test/agent-session-advisor-suppression.test.ts index 2ac1780ed2f..19c9aa44a88 100644 --- a/packages/coding-agent/test/agent-session-advisor-suppression.test.ts +++ b/packages/coding-agent/test/agent-session-advisor-suppression.test.ts @@ -41,6 +41,13 @@ interface ParkedHarness { streamStarted: Promise; } +interface CompletedAdvisorHarness { + session: AgentSession; + sessionManager: SessionManager; + mock: MockModel; + advisorMock: MockModel; +} + describe("AgentSession advisor auto-resume suppression", () => { let tempDir: TempDir; let session: AgentSession; @@ -94,6 +101,50 @@ describe("AgentSession advisor auto-resume suppression", () => { return { session, sessionManager, mock, streamStarted: started.promise }; } + async function createCompletedAdvisorSession(): Promise { + const model = getBundledModel("anthropic", "claude-sonnet-4-5")!; + const mock = createMockModel({ + responses: [ + { content: ["EXACT VERDICT"], stopReason: "stop" }, + { content: ["CHANGED VERDICT"], stopReason: "stop" }, + ], + }); + const advisorMock = createMockModel({ + responses: [ + { + content: [ + { + type: "toolCall", + name: "advise", + arguments: { note: "Fixture verdict confirmed", severity: "concern" }, + }, + ], + }, + ], + }); + const agent = new Agent({ + getApiKey: () => "test-key", + initialState: { model, systemPrompt: ["Test"], tools: [] }, + streamFn: mock.stream, + }); + const sessionManager = SessionManager.inMemory(); + const settings = Settings.isolated({ "compaction.enabled": false, "retry.enabled": false }); + settings.setModelRole("advisor", "anthropic/claude-sonnet-4-5"); + const authStorage = await AuthStorage.create(tempDir.join(`auth-${Snowflake.next()}.db`)); + authStorages.push(authStorage); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + const modelRegistry = new ModelRegistry(authStorage, tempDir.join("models.yml")); + session = new AgentSession({ + agent, + sessionManager, + settings, + modelRegistry, + advisorTools: [], + advisorStreamFn: advisorMock.stream, + }); + return { session, sessionManager, mock, advisorMock }; + } + function advisorCard(content: string) { return { customType: ADVISOR_TYPE, @@ -132,6 +183,27 @@ describe("AgentSession advisor auto-resume suppression", () => { return persisted; } + it("preserves a late advisor concern after a terminal answer without waking the primary", async () => { + const { session, sessionManager, mock, advisorMock } = await createCompletedAdvisorSession(); + const persisted = capturePersistedAdvice(sessionManager); + + await session.prompt("read five fixture files and answer with exactly one line"); + await session.waitForIdle(); + expect(mock.calls.length).toBe(1); + + expect(session.setAdvisorEnabled(true)).toBe(true); + const advisor = session.getAdvisorAgent(); + if (!advisor) throw new Error("Expected advisor agent to be live"); + + await advisor.prompt("inspect the completed turn"); + await session.waitForIdle(); + + const advisorCards = session.agent.state.messages.filter(isAdvisorCard); + expect(advisorCards).toHaveLength(1); + expect(persisted.at(-1)).toContain("Fixture verdict confirmed"); + expect(advisorMock.calls.length).toBeGreaterThanOrEqual(1); + expect(mock.calls.length).toBe(1); + }); it("preserves an advisor concern steered before the user interrupt, without auto-resuming", async () => { const { session, sessionManager, mock, streamStarted } = await createParkedSession(); const persisted = capturePersistedAdvice(sessionManager);