From 699e07fea782cc281dbb057bf6f96d730e712cb6 Mon Sep 17 00:00:00 2001 From: Wolfgang Schoenberger <221313372+wolfiesch@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:17:56 -0700 Subject: [PATCH] fix(coding-agent): separate overflow-retry fit check from auto-continue band MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR #3412 review (roboomp blocking + codex P2 + Copilot nits): - The overflow/incomplete retry no longer reuses the COMPACTION_RECOVERY_BAND hysteresis. Reusing it required residual context below 0.8×threshold, which turned a recoverable overflow (e.g. 150k on a 200k window, under the ~170k threshold) into a manual dead-end. Add #compactionCreatedRetryFit, which measures the rebuilt prompt against the usable fit budget (contextWindow - effectiveReserveTokens) and is evaluated AFTER the failed assistant is dropped, so the just-failed turn is excluded. - The threshold auto-continue keeps the stricter recovery-band check (#compactionCreatedHeadroom) — that path is the snapcompact thrash guard. - Split the no-progress warning so each path warns on its own signal. - Alias errorIsFromBeforeCompaction as assistantPredatesCompaction in the threshold-usage path for clarity; de-duplicate the rationale comment. Adds two regression tests (recoverable overflow retries; non-fitting overflow pauses + warns), mutation-verified against the band-vs-fit split. (cherry picked from commit f8cf45b2740f04d8776a84521ba563b779d65b25) --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/session/agent-session.ts | 94 ++++++++++----- ...ion-auto-compaction-progress-guard.test.ts | 113 ++++++++++++++++++ 3 files changed, 179 insertions(+), 29 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 24a3165de..4125f6ce6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -118,6 +118,7 @@ - Fixed `omp install` of legacy pi extensions failing with `Cannot find module '/$bunfs/root/packages/coding-agent/src/extensibility/typebox.js'` on every released `omp--` binary. Commit `dc5c93462f` removed worker entrypoints from `scripts/ci-release-build-binaries.ts`; the inline comment then claimed the legacy-shim and package-barrel entrypoints (`typebox.ts`, `legacy-pi-{ai,coding-agent}-shim.ts`, `packages/{agent,natives,tui,utils}/...`) were "still" passed to `bun build --compile`, but they had never been re-added. The release binaries shipped without those files in bunfs, so `legacy-pi-compat.ts` redirected `typebox` imports to a bunfs path that didn't exist. `__resolveTypeBoxShimPath` now mirrors `__validateLegacyPiPackageRootOverrides` (#2168) by dropping the override when the shim file is missing, so a missing shim falls through to native `node_modules` resolution instead of emitting a dead bunfs URL ([#3414](https://github.com/can1357/oh-my-pi/issues/3414)). - Fixed every legacy `@(scope)/pi-*` and `@sinclair/typebox` import failing to load on the `omp-darwin-arm64` release binary (and any other `omp` built with Bun 1.3.14). `__validateLegacyPiPackageRootOverrides` and the `rewriteLegacyPiImports` emit path both depended on `--compile` extras being reachable as `/$bunfs/root/...` filesystem entries, but Bun 1.3.14 stopped exposing them through every API (`fs.existsSync`, `Bun.file().exists()`, `Bun.resolveSync`, `await import()` on the bunfs path or its `file://` URL all fail; only `/$bunfs/root/` itself answers). `legacy-pi-compat.ts` now keeps a JS-heap reference to every bundled pi-* surface in a lazy-loaded sibling `legacy-pi-bundled-registry.ts` and serves them through an `omp-legacy-pi-bundled:` virtual namespace whose `Bun.plugin().onLoad` returns synthetic re-exports — no bunfs path ever leaves the module in compiled mode, and dev / source-link / installed-package modes keep the historical `file://` rewrite. The matching `--compile` extras in `scripts/build-binary.ts`, the shared `scripts/binary-entrypoints.ts` list, and the dead `BUNFS_PACKAGE_ROOT` / `bunfsPath` / `__computeBunfsPackageRoot` / `__joinBunfsPath` helpers are gone. ([#3423](https://github.com/can1357/oh-my-pi/issues/3423)) - Fixed auto-compaction thrashing on a session whose single most-recent kept turn already exceeds the compaction threshold. `prepareCompaction` keeps that turn verbatim (`findCutPoint` never cuts at tool results), so the rewritten context stays above threshold; the context-full / snapcompact success tail scheduled the agent-authored auto-continue (and the overflow/incomplete retry) unconditionally, so the next `agent_end` re-entered `#checkCompaction` over the same oversized tail and re-fired forever. This is the residual loop left after #3247 capped snapcompact's own frame projection — once the frame cap drops below one frame, snapcompact is skipped and the context-full summarizer path still made no headroom. `#runAutoCompaction` now gates the continuation/retry on a post-maintenance headroom check (`#compactionCreatedHeadroom`, sharing shake's `COMPACTION_RECOVERY_BAND` hysteresis from #2275); when a pass frees too little it pauses automatic maintenance and emits a single warning instead of looping. The post-turn threshold check also ignores an assistant's stale pre-compaction `usage` so the scheduled auto-continue cannot re-trip on the kept assistant's old high token count. +- Fixed auto-compaction thrashing on a session whose single most-recent kept turn already exceeds the compaction threshold. `prepareCompaction` keeps that turn verbatim (`findCutPoint` never cuts at tool results), so the rewritten context stays above threshold; the context-full / snapcompact success tail scheduled the agent-authored auto-continue (and the overflow/incomplete retry) unconditionally, so the next `agent_end` re-entered `#checkCompaction` over the same oversized tail and re-fired forever. This is the residual loop left after #3247 capped snapcompact's own frame projection — once the frame cap drops below one frame, snapcompact is skipped and the context-full summarizer path still made no headroom. `#runAutoCompaction` now gates the threshold auto-continue on a post-maintenance headroom check (`#compactionCreatedHeadroom`, sharing shake's `COMPACTION_RECOVERY_BAND` hysteresis from #2275) and the overflow/incomplete retry on a separate fit check (`#compactionCreatedRetryFit`, measured after the failed turn is dropped) so a recoverable overflow that fits the window still retries; when a pass frees too little for the relevant path it pauses automatic maintenance and emits a single warning instead of looping. The post-turn threshold check also ignores an assistant's stale pre-compaction `usage` so the scheduled auto-continue cannot re-trip on the kept assistant's old high token count. ## [16.1.17] - 2026-06-24 diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 58dc69f2c..b425b6a1f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -8865,13 +8865,18 @@ export class AgentSession { if (assistantMessage.stopReason === "error") return COMPACTION_CHECK_NONE; const pruneResult = await this.#pruneToolOutputs(); const maintenanceTokensFreed = (supersedeResult?.tokensSaved ?? 0) + (pruneResult?.tokensSaved ?? 0); + // `errorIsFromBeforeCompaction` (computed above) is the general + // "this assistant message predates the latest compaction" predicate here, + // not just an error-specific one; alias it locally so the threshold intent + // reads clearly (#3412 review). + const assistantPredatesCompaction = errorIsFromBeforeCompaction; // An assistant that predates the latest compaction carries stale, pre-rewrite // `usage`: the scheduled auto-continue re-enters this check with the kept // assistant (#promptWithMessage → #checkCompaction), and its old high prompt // count would re-trip the threshold on a freshly compacted history. Drop the // stale provider number for those messages and let the live stored estimate // (the floor applied below) drive the decision instead. - const assistantUsageContextTokens = errorIsFromBeforeCompaction + const assistantUsageContextTokens = assistantPredatesCompaction ? 0 : calculateContextTokens(assistantMessage.usage); const storedContextTokens = this.#estimateStoredContextTokens(); @@ -10163,6 +10168,36 @@ export class AgentSession { return true; } + /** + * Retry-side counterpart to {@link #compactionCreatedHeadroom}. An + * overflow/incomplete recovery only needs the rebuilt prompt to *fit* the + * window again — it does not have to land under the compaction threshold, let + * alone the stricter `COMPACTION_RECOVERY_BAND × threshold` hysteresis the + * auto-continue thrash guard uses. Reusing the band here turned recoverable + * overflows into manual dead-ends: a 200k-window prompt compacted from + * overflow down to ~150k is comfortably retryable, but sits above + * `0.8 × 170k = 136k` and was wrongly refused (PR #3412 review). + * + * Measures residual context against the usable budget + * (`contextWindow - effectiveReserveTokens`). Callers MUST invoke this AFTER + * dropping the failed assistant from `this.messages`, so the just-failed turn + * (which the retry prompt will not include) is excluded from the estimate. + * + * When the model/window is unknown we cannot evaluate the budget, so we + * optimistically allow the retry (preserving prior behavior). + */ + #compactionCreatedRetryFit(): boolean { + const contextWindow = this.model?.contextWindow ?? 0; + if (contextWindow <= 0) return true; + const compactionSettings = this.settings.getGroup("compaction"); + const residualTokens = compactionContextTokens( + this.getContextUsage({ contextWindow })?.tokens ?? 0, + this.#estimateStoredContextTokens(), + ); + const fitBudget = contextWindow - effectiveReserveTokens(contextWindow, compactionSettings); + return residualTokens <= fitBudget; + } + /** * Internal: Run auto-compaction with events. * @@ -10657,12 +10692,16 @@ export class AgentSession { // of recent history verbatim and findCutPoint can only cut at turn // boundaries (never tool results), so a single oversized recent turn (e.g. a // huge tool result) leaves the rewritten context still above threshold. - // Scheduling the auto-continue regardless means the next agent_end re-enters - // #checkCompaction over the same oversized tail and re-fires forever - // (the snapcompact thrash). Mirror the shake recovery-band check: only - // auto-continue when compaction actually created headroom. + // Scheduling the continuation regardless means the next agent_end re-enters + // #checkCompaction over the same oversized tail and re-fires forever. The + // retry and the threshold auto-continue use different progress tests (a + // recoverable overflow only has to fit; the auto-continue thrash needs the + // stricter recovery band), so each branch evaluates its own below. let continuationScheduled = false; - const madeProgress = this.#compactionCreatedHeadroom(options.triggerContextTokens); + // A non-idle pass that wanted to continue (retry or auto-continue) but freed + // too little for that path to proceed is a dead-end: warn once so the user + // understands why maintenance paused instead of silently looping. + let noProgressDeadEnd = false; if (willRetry) { const messages = this.agent.state.messages; @@ -10681,28 +10720,28 @@ export class AgentSession { } } - // Only retry when maintenance actually created headroom. An - // overflow/incomplete recovery whose kept recent turn alone still - // exceeds the window would retry straight back into the same - // overflow/length failure — the recovery-side twin of the threshold - // auto-continue thrash. Pause and surface the dead-end instead. - if (madeProgress) { + // Retry only needs the rebuilt prompt to fit the window again — measured + // AFTER the drop above so the just-failed turn (which the retry prompt + // won't include) is excluded. Reusing the auto-continue recovery band + // here turned recoverable overflows into manual dead-ends (#3412 review), + // so use the looser fit budget. + if (this.#compactionCreatedRetryFit()) { this.#scheduleAgentContinue({ delayMs: 100, generation }); continuationScheduled = true; + } else { + noProgressDeadEnd = true; + } + } else if (reason !== "idle" && shouldAutoContinue) { + // Mirror the shake recovery-band check: only auto-continue when compaction + // landed residual context under `COMPACTION_RECOVERY_BAND × threshold`. + // Re-firing on a history that still sits just over the line is the + // snapcompact thrash, so require genuine headroom, not a bare fit. + if (this.#compactionCreatedHeadroom(options.triggerContextTokens)) { + this.#scheduleAutoContinuePrompt(generation); + continuationScheduled = true; + } else { + noProgressDeadEnd = true; } - } else if (reason !== "idle" && shouldAutoContinue && madeProgress) { - // Post-maintenance progress guard. Snapcompact can project over budget - // and fall back to a context-full summary; the summarizer keeps - // `keepRecentTokens` of recent history verbatim and findCutPoint can - // only cut at turn boundaries (never tool results), so a single - // oversized recent turn (e.g. a huge tool result) leaves the rewritten - // context still above threshold. Scheduling the auto-continue regardless - // means the next agent_end re-enters #checkCompaction over the same - // oversized tail and re-fires forever (the snapcompact thrash). Mirror - // the shake recovery-band check: only auto-continue when compaction - // actually created headroom. - this.#scheduleAutoContinuePrompt(generation); - continuationScheduled = true; } else if (!suppressContinuation && this.agent.hasQueuedMessages()) { // Auto-compaction can complete while follow-up/steering/custom messages are waiting. // Kick the loop so queued messages are actually delivered. @@ -10714,10 +10753,7 @@ export class AgentSession { continuationScheduled = true; } - // A non-idle pass that wanted to continue (auto-continue or retry) but - // made no headroom is a dead-end: warn once so the user understands why - // maintenance paused instead of silently looping. - if (!madeProgress && reason !== "idle" && (willRetry || shouldAutoContinue)) { + if (noProgressDeadEnd) { this.emitNotice( "warning", "Compaction freed too little context to make progress — pausing automatic maintenance to avoid a compaction loop. The most recent turn alone is too large to reduce further; shrink it (e.g. clear large tool output) or switch to a larger-context model.", 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 92911cf17..7308513a7 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 @@ -143,6 +143,27 @@ describe("AgentSession auto-compaction progress guard", () => { timestamp: Date.now(), }; } + /** Build a context-overflow assistant turn (input exceeds the 200k window). */ + function overflowAssistant() { + return { + role: "assistant" as const, + content: [{ type: "text" as const, text: "" }], + api: "anthropic-messages" as const, + provider: "anthropic" as const, + model: "claude-sonnet-4-5", + stopReason: "error" as const, + errorMessage: "prompt is too long: 250000 tokens > 200000 maximum", + usage: { + input: 250000, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 250000, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }; + } function collectNotices() { const notices: { level: string; message: string; source?: string }[] = []; @@ -226,4 +247,96 @@ describe("AgentSession auto-compaction progress guard", () => { const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); expect(noProgress.length).toBe(0); }); + /** + * Seed several large prior turns into the session branch so `prepareCompaction` + * returns a real preparation after the overflow recovery drops the failed + * assistant from active context. The drop only touches agent state, and a + * branch under `keepRecentTokens` (20k) has nothing to summarize, so each + * turn carries enough text (~10k tokens) to push older turns past the cut. + */ + function seedPriorTurns() { + const bigText = "lorem ipsum ".repeat(4000); // ~10k tokens of summarizable text + for (let i = 0; i < 4; i++) { + sessionManager.appendMessage({ + role: "assistant", + content: [{ type: "text", text: bigText }], + api: "anthropic-messages", + provider: "anthropic", + model: "claude-sonnet-4-5", + stopReason: "stop", + usage: { + input: 1000, + output: 50, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 1050, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }); + sessionManager.appendMessage({ role: "user", content: "next", timestamp: Date.now() }); + } + } + + it("retries an overflow recovery that fits the window but stays inside the recovery band", async () => { + // Regression for the band-vs-fit conflation (#3412 review): the overflow + // retry only needs the rebuilt prompt to fit the window, NOT to drop under + // `COMPACTION_RECOVERY_BAND × threshold`. Residual lands at 150k on a 200k + // window — above the 0.8×170k≈136k recovery band, but comfortably under the + // usable budget — so the retry MUST proceed instead of dead-ending. + seedPriorTurns(); + const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(); + vi.spyOn(session.agent, "prompt").mockResolvedValue(undefined as never); + vi.spyOn(session, "getContextUsage").mockReturnValue({ tokens: 150000, contextWindow: 200000, percent: 75 }); + + const notices = collectNotices(); + + 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(continueSpy).toHaveBeenCalledTimes(1); + const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); + expect(noProgress.length).toBe(0); + }); + + it("pauses (single warning) when an overflow recovery still does not fit the window", async () => { + // The genuine dead-end the retry guard must still catch: even after dropping + // the failed turn the rebuilt prompt is over the window, so retrying would + // hit the same overflow. Pause once instead of looping. + seedPriorTurns(); + const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(); + vi.spyOn(session.agent, "prompt").mockResolvedValue(undefined as never); + vi.spyOn(session, "getContextUsage").mockReturnValue({ tokens: 205000, contextWindow: 200000, percent: 102.5 }); + + const notices = collectNotices(); + const startCount = countCompactionStarts(); + + 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(startCount()).toBe(1); + expect(continueSpy).not.toHaveBeenCalled(); + expect(session.isStreaming).toBe(false); + const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); + expect(noProgress.length).toBe(1); + expect(noProgress[0].level).toBe("warning"); + }); });