Conversation
Upstream's _checkCompaction gates overflow recovery behind a sameModel check that compares the assistant message's provider/model against this.model. In Auto sessions, this.model is kimchi-dev/auto but the assistant message carries the routed concrete model (e.g. ai-enabler/routed), so sameModel is always false and the session wedges on the context-length 400 instead of compacting and retrying. Extend the existing installCompactionRecoveryPatch wrapper in src/upstream-retry-patch.ts to stamp the throwaway message copy with the session model's provider/id when an Auto session has resolved routing. This makes sameModel pass so upstream's overflow recovery fires. The original message is never mutated; non-Auto sessions are unaffected, preserving the model-switch guard. Add 4 unit tests in src/upstream-compaction-recovery.test.ts covering Auto stamping, unresolved routing, concrete-session no-op, and combined raw-error + model stamping. Add an e2e test in tests/e2e/tui/auto-model.test.ts verifying that Auto sessions compact and recover from a context-window overflow end-to-end. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Kimchi Code Review
Summary📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — The PR adds focused unit tests in 📝 Found 1 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
There was a problem hiding this comment.
📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — The PR adds focused unit tests in src/upstream-compaction-recovery.test.ts covering resolved/unresolved Auto routing, concrete (non-Auto) sessions, and the combined raw-error + model-stamping path. It also adds an end-to-end TUI test in tests/e2e/tui/auto-model.test.ts that exercises the full Auto overflow-recovery flow. Coverage is strong for the changed behavior.
📝 Found 1 issue(s). See inline comments for details.
…tiveModel Address review feedback: reference equality on the resolveEffectiveModel result is fragile against future refactoring that may return a distinct object representing the same Auto model. Compare by provider/id instead. Co-Authored-By: Kimchi <noreply@kimchi.dev>
|
📚 No documentation changes were needed for this PR. |
Problem
Auto (router) sessions cannot recover from context-window overflow. Upstream's
_checkCompactiongates overflow recovery behind asameModelcheck that compares the assistant message'sprovider/modelagainstthis.model. In Auto sessions,this.modeliskimchi-dev/auto(the user-selected model, kept by design), but the assistant message carries the routed concrete model (e.g.ai-enabler/routed) — stamped by the Auto API provider when it delegates to the concrete model'sstream(). SosameModelis alwaysfalse, the overflow recovery branch never fires, and the session wedges on the 400 instead of compacting and retrying.The existing
installCompactionRecoveryPatchwrapper already handles the other half of this problem — restoring the raw provider error text soisContextOverflowcan match it — but it cannot help ifsameModelnever lets execution reach that check.Fix
Extended the existing
installCompactionRecoveryPatchwrapper insrc/upstream-retry-patch.tsto stamp the throwaway message copy with the session model's identity (kimchi-dev/auto) when an Auto session has resolved routing. This makessameModelpass because the copy'sprovider/modelnow matchthis.model. The original message is never mutated — the stamping operates on the same throwaway copy the wrapper already creates for raw-error restoration. Non-Auto sessions are a no-op, preserving the model-switch guard.Alternatives Considered
Temporarily swap
this.agent.state.model— Set the session model to the routed concrete model during_checkCompaction, then restore it in afinallyblock. Rejected because_checkCompactionsetsthis._overflowRecoveryAttempted = trueinternally — the flag would correctly land on the real session, but concurrent reads (e.g. status-line renders) could briefly see the routed model instead ofauto. Stamping the copy avoids mutating session state entirely.Patch upstream
_checkCompactionsource viapatches/— Add a source-level patch to modify thesameModelgate inagent-session.js. Rejected because patches are debt — they require tracking issues and expire when upstream changes. Additionally, the effective (routed) model is tracked in Kimchi's router state (stateBySession), which is not accessible from upstream'sAgentSessioncontext, making it hard to resolve the correct model identity inside a source patch. The existing wrapper approach already intercepts_checkCompactioncleanly and has access to the session viathis, where the router state can be resolved.Tests
src/upstream-compaction-recovery.test.ts: Auto stamping, unresolved routing no-op, concrete-session no-op, combined raw-error + model stamping.tests/e2e/tui/auto-model.test.ts: Auto session routes to a concrete model, hits a context-length 400, runs compaction summary, and retries successfully.