From 4394184331c8e0d00746bc3d217293f3cab5bb31 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 28 Jun 2026 19:51:12 +0000 Subject: [PATCH] fix(session): restore overflow turn when compaction skips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the persisted assistant error before #runAutoCompaction so the kept region is clean, then re-append it on COMPACTION_CHECK_NONE without a fresh compaction entry — covers no-model, hook-cancel, and compaction-error paths. Same rollback for response.incomplete recovery. Fixes #3747 --- .../coding-agent/src/session/agent-session.ts | 53 ++++++++++++++++--- ...ion-auto-compaction-progress-guard.test.ts | 36 +++++++++++++ 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index a40ce2414..ee01979fb 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -9502,8 +9502,12 @@ 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 await this.#runRecoveryCompactionWithRollback( + "overflow", + assistantMessage, + allowDefer, + { autoContinue }, + ); } return COMPACTION_CHECK_NONE; } @@ -9532,15 +9536,16 @@ 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, }); - return await this.#runAutoCompaction("incomplete", true, false, allowDefer, { - autoContinue, - triggerContextTokens: calculateContextTokens(assistantMessage.usage), - }); + return await this.#runRecoveryCompactionWithRollback( + "incomplete", + assistantMessage, + allowDefer, + { autoContinue, triggerContextTokens: calculateContextTokens(assistantMessage.usage) }, + ); } // Neither promotion nor compaction is available — surface the dead-end so // the user understands why the turn yielded with nothing. @@ -9816,6 +9821,40 @@ export class AgentSession { this.#discardAssistantTurn(assistantMessage); } + /** + * Drop the failed assistant turn from persisted history, run + * {@link #runAutoCompaction} for an `overflow` / `incomplete` recovery, and + * restore the assistant entry if compaction did not actually commit + * anything (no usable model/preparation, hook cancel, or compaction error). + * + * Compaction has to see a clean branch — otherwise its `prepareCompaction` + * pass would keep the failed turn in the kept region and the retry would + * replay it. But a NONE return that was not paired with a fresh compaction + * summary means no recovery is in progress, and leaving the branch + * reparented would erase the only user-visible explanation for why the turn + * stopped. Reverting the drop in that case preserves the transcript while + * still letting a real recovery path own the rewrite. + */ + async #runRecoveryCompactionWithRollback( + reason: "overflow" | "incomplete", + assistantMessage: AssistantMessage, + allowDefer: boolean, + options: { autoContinue: boolean; triggerContextTokens?: number }, + ): Promise { + const compactionEntryBefore = getLatestCompactionEntry(this.sessionManager.getBranch()); + await this.#dropPersistedAssistantTurn(assistantMessage); + const result = await this.#runAutoCompaction(reason, true, false, allowDefer, options); + const recoveryCommitted = + result.continuationScheduled || result.deferredHandoff || result.automaticContinuationBlocked === true; + if (!recoveryCommitted) { + const compactionEntryAfter = getLatestCompactionEntry(this.sessionManager.getBranch()); + if (compactionEntryAfter === compactionEntryBefore) { + this.sessionManager.appendMessage(assistantMessage); + } + } + return result; + } + /** * Drop an assistant turn from BOTH the live agent context and the persisted * session branch (reparenting the leaf to the turn's parent), so a discarded 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 f866a559f..25ac72ece 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 @@ -443,6 +443,42 @@ describe("AgentSession auto-compaction progress guard", () => { ); }); + it("restores the persisted overflow error when compaction skips without committing", async () => { + // `#runAutoCompaction` returning COMPACTION_CHECK_NONE without writing a + // compaction summary (no available model, hook cancel, compaction error) + // MUST NOT erase the user-visible assistant error: the transcript would + // otherwise lose the only explanation of why the turn stopped. + seedPriorTurns(); + vi.spyOn(modelRegistry, "getAvailable").mockReturnValue([]); + const promptSpy = vi.spyOn(session.agent, "prompt").mockResolvedValue(undefined as never); + const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(); + + const { promise: compactionDone, resolve: onCompactionDone } = Promise.withResolvers(); + session.subscribe(event => { + if (event.type === "auto_compaction_end") onCompactionDone(); + }); + + const assistantMsg = overflowAssistant(); + session.agent.emitExternalEvent({ type: "message_end", message: assistantMsg }); + session.agent.emitExternalEvent({ type: "agent_end", messages: [assistantMsg] }); + + await compactionDone; + 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