From 9f79d09a8de52c6bbf3d23ecbb416a4b786074ce Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 28 Jun 2026 19:40:51 +0000 Subject: [PATCH] fix(session): preserve overflow error when no recovery runs Split active-context vs persisted-history removal in #checkCompaction so the persisted assistant error stays on the branch unless context promotion or compaction is actually scheduled. Fixes #3747 --- .../coding-agent/src/session/agent-session.ts | 28 +++++++++++++++-- ...ion-auto-compaction-progress-guard.test.ts | 30 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index b74012f00..a40ce2414 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -9482,11 +9482,18 @@ export class AgentSession { const errorIsFromBeforeCompaction = compactionEntry !== null && assistantMessage.timestamp < new Date(compactionEntry.timestamp).getTime(); if (sameModel && !errorIsFromBeforeCompaction && AIError.isContextOverflow(assistantMessage, contextWindow)) { - await this.#discardRecoverableAssistantTurn(assistantMessage); + // Clear the failed turn from active context so the retry (or the next + // user prompt) does not replay it. The persisted branch entry stays + // for now: when no recovery path runs, the user-facing transcript + // MUST keep the only assistant message explaining why the turn + // stopped. The branch entry is dropped further down, but only on the + // paths that actually schedule a retry/compaction. + this.#removeAssistantMessageFromActiveContext(assistantMessage); // Try context promotion first - switch to a larger model and retry without compacting const promoted = await this.#tryContextPromotion(assistantMessage); if (promoted) { + await this.#dropPersistedAssistantTurn(assistantMessage); // Retry on the promoted (larger) model without compacting this.#scheduleAgentContinue({ delayMs: 100, generation }); return COMPACTION_CHECK_CONTINUATION; @@ -9495,6 +9502,7 @@ export class AgentSession { // No promotion target available fall through to compaction const compactionSettings = this.settings.getGroup("compaction"); if (compactionSettings.enabled && compactionSettings.strategy !== "off") { + await this.#dropPersistedAssistantTurn(assistantMessage); return await this.#runAutoCompaction("overflow", true, false, allowDefer, { autoContinue }); } return COMPACTION_CHECK_NONE; @@ -9507,10 +9515,14 @@ export class AgentSession { // otherwise compaction/handoff. Unlike overflow, the *input* is fine, so we // allow the handoff strategy to actually run. if (sameModel && !errorIsFromBeforeCompaction && assistantMessage.stopReason === "length") { - await this.#discardRecoverableAssistantTurn(assistantMessage); + // Same active-context vs persisted-history split as the overflow path + // above: clear the dead turn from agent state so it cannot be replayed, + // but keep it on the branch unless promotion or compaction actually runs. + this.#removeAssistantMessageFromActiveContext(assistantMessage); const promoted = await this.#tryContextPromotion(assistantMessage); if (promoted) { + await this.#dropPersistedAssistantTurn(assistantMessage); logger.debug("Context promotion triggered by response.incomplete (length stop)", { from: `${assistantMessage.provider}/${assistantMessage.model}`, }); @@ -9520,6 +9532,7 @@ export class AgentSession { const incompleteCompactionSettings = this.settings.getGroup("compaction"); if (incompleteCompactionSettings.enabled && incompleteCompactionSettings.strategy !== "off") { + await this.#dropPersistedAssistantTurn(assistantMessage); logger.debug("Compaction triggered by response.incomplete (length stop, no promotion target)", { model: `${assistantMessage.provider}/${assistantMessage.model}`, strategy: incompleteCompactionSettings.strategy, @@ -9789,7 +9802,16 @@ export class AgentSession { } } - async #discardRecoverableAssistantTurn(assistantMessage: AssistantMessage): Promise { + /** + * Drop a recoverable assistant turn from the persisted session branch once a + * recovery path (context promotion or compaction) is committed. Waits for the + * in-flight `message_end` persistence slot first so the branch entry exists + * before we reparent past it. Active context removal is the caller's + * responsibility — recovery paths clear it eagerly so the retry never + * replays the failed turn, while no-recovery paths leave the persisted entry + * (and the user-visible transcript line) in place. + */ + async #dropPersistedAssistantTurn(assistantMessage: AssistantMessage): Promise { await this.#waitForSessionMessagePersistence(assistantMessage); this.#discardAssistantTurn(assistantMessage); } diff --git a/packages/coding-agent/test/agent-session-auto-compaction-progress-guard.test.ts b/packages/coding-agent/test/agent-session-auto-compaction-progress-guard.test.ts index c49592c7f..f866a559f 100644 --- a/packages/coding-agent/test/agent-session-auto-compaction-progress-guard.test.ts +++ b/packages/coding-agent/test/agent-session-auto-compaction-progress-guard.test.ts @@ -413,6 +413,36 @@ describe("AgentSession auto-compaction progress guard", () => { ); }); + it("keeps the visible overflow error when no recovery path is available", async () => { + // When promotion is off and compaction is disabled there is no retry to run; + // the persisted assistant error MUST stay on the branch so the user (and the + // reloaded transcript) keeps the only explanation of why the turn stopped. + session.settings.set("contextPromotion.enabled", false); + session.settings.set("compaction.enabled", false); + + const promptSpy = vi.spyOn(session.agent, "prompt").mockResolvedValue(undefined as never); + const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(); + + const assistantMsg = overflowAssistant(); + session.agent.emitExternalEvent({ type: "message_end", message: assistantMsg }); + session.agent.emitExternalEvent({ type: "agent_end", messages: [assistantMsg] }); + + await session.waitForIdle(); + + expect(promptSpy).not.toHaveBeenCalled(); + expect(continueSpy).not.toHaveBeenCalled(); + expect(sessionManager.getBranch()).toContainEqual( + expect.objectContaining({ + type: "message", + message: expect.objectContaining({ + role: "assistant", + stopReason: "error", + errorMessage: assistantMsg.errorMessage, + }), + }), + ); + }); + it("retries a small-window overflow when the default reserve exceeds the model window", async () => { // Bundled 4k/8k models can be smaller than the default absolute reserve // (16,384). Retry fit must clamp that reserve; otherwise the budget goes