fix(compaction): allow manual recovery on SDK-owned sessions - #1423
Conversation
🤖 ulw-loop engaged on this PR\n\nRunning an automated brutal review → fix → CI green → merge loop. Stages and who does what:\n\n| Stage | Agent | Output |\n| --- | --- | --- |\n| 1. Adversarial review | | review via the GitHub review API, file:line anchored |\n| 2. Fix | | commits on this branch addressing every blocking concern |\n| 3. CI + merge | orchestrator | required checks green, then merge to |\n\nBaseline captured before any change (RED):\n- — FAIL: › does not exit while a turn is active even with no connections → \n- — FAIL, aggregator only ()\n- Everything else green: Static checks, Test (coding-agent 1-3), Test (workspaces + scripts), Terminal tools ×3, Inspector handoff ×3, Changelog gate.\n\nI'll comment at each stage transition with evidence. |
📋 Stage 1 progress — CI triage + conflict located\n\nThe red CI is not this PR's fault. Evidence:\n\n| Run | Branch | Failing job | Failing test |\n| --- | --- | --- | --- |\n| 34077470542 | (this PR) | RPC named pipes (Windows) | does not exit while a turn is active even with no connections → |\n| 34076954251 | **** | RPC named pipes (Windows) | does not exit while a client is attached, then exits after it detaches → |\n\nSame job, same on the Windows named pipe, two different test cases, one of them on with none of this PR's code. That is a pre-existing Windows named-pipe race in , unrelated to the compaction guard. is red only because it aggregates that job ().\n\nMerge conflict located — :\n\nSingle file, the block under Unreleased. has advanced → since this PR was branched, so a rebase is needed regardless.\n\n🧠 is currently reading the full compaction lane to produce the blocking review. Next comment will carry its verdict. |
📋 Stage 1 progress — local baseline GREEN, review lane retrying\n\nLocal baseline at PR head ** (worktree, clean):\n\n\nFindings from an independent source read** (these will anchor the formal review):\n\n1. Stacked bypass. With this change, on an SDK-owned session now clears three layers, not one:\n - the SDK-owner lane guard — (this PR)\n - the circuit breaker — \n - the per-turn cap — is a no-op, it always returns \n\n So a repeated on an SDK-owned session has no rate or failure protection at all, while the SDK concurrently owns automatic compaction over the same transcript. The PR body says "Threshold, overflow, speculative ownership, and model-capacity admission remain unchanged" — true — but it does not address that the manual lane it opens is itself unguarded.\n\n2. Short-circuit skips the lane call. means is not invoked for manual requests. Worth confirming that function is side-effect-free/memo-free before relying on the short-circuit.\n\n3. Reason coverage is partial. () is . The new omits and .\n\n4. The test helper defeats type checking. ends in (alignment test :181-194), so an invalid in the would not be caught by Version 5.9.3tsc: The TypeScript Compiler - Version 5.9.3 COMMON COMMANDS tsc tsc app.ts util.ts tsc -b tsc --init tsc -p ./path/to/tsconfig.json tsc --help --all tsc --noEmit COMMAND LINE FLAGS --help, -h --watch, -w --all --version, -v --init --project, -p --showConfig --build, -b COMMON COMPILER OPTIONS --pretty --declaration, -d Image Settings: Image Operators: Image Stack Operators: Miscellaneous Options: By default, the image format of 'file' is determined by its magic --declarationMap --emitDeclarationOnly --sourceMap --noEmit --target, -t --module, -m --lib --allowJs --checkJs --jsx --outFile --outDir --removeComments --strict --types --esModuleInterop You can learn about all of the compiler options at https://aka.ms/tsc.\n\n5. No coverage lost from the deleted test — its default reason was , still covered as the first case. ✅\n\n |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
🔴 Request changes — the one-line exemption is not the whole fix
The intent is right and the SDK case is genuinely covered (I proved the RED myself, see below). But the change exempts manual at one of three ownership call sites and lands it in a lane where every remaining safety valve is already inert for manual. As written it opens an unguarded compaction path on SDK-owned sessions.
Evidence I captured before writing this
Reverted only the production hunk (packages/coding-agent/src/core/extensions/builtin/compaction/index.ts:559 back to if (lanePolicy.disablesSenpiCompaction(ctx))) in a clean worktree at head 369eb0330 and ran the new test:
FAIL test/suite/sdk-manual-compaction-recovery.test.ts > persists a usable manual summary on claude-sdk-oauth
AssertionError: [{"reason":"manual","aborted":true,"accepted":false,"rejectionCause":"external-owner",
"errorMessage":"Compaction rejected: the Claude Agent SDK owns compaction for this session"}]
expected { status: 'rejected' } to match object { status: 'compacted' }
Test Files 1 failed (1)
Tests 1 failed | 1 passed (2)
Good: the claude-sdk-oauth case is real coverage. Bad: 1 passed — the faux case passes with the fix reverted, so it is vacuous and proves nothing about this change.
BLOCKING: 1 — the exemption covers 1 of 3 ownership sites, and the other two now misbehave for manual
lanePolicy.disablesSenpiCompaction(ctx) is consulted at packages/coding-agent/src/core/extensions/builtin/compaction/index.ts lines 559, 766, and 1008. This PR reason-gates only 559. Site 766:
if (compactEvent.rejectionCause === "external-owner") return;
if (!lanePolicy.disablesSenpiCompaction(ctx)) {
state = breaker.recordFailure(state, Date.now(), { route: compactEvent.reason });
}Before this PR a manual compaction on an SDK-owned session always rejected with external-owner and returned at line 1. Now it proceeds — so when it fails, rejectionCause is no longer external-owner, execution reaches the second check, disablesSenpiCompaction is still true, and recordFailure is never called. Failed manual compactions on SDK lanes are invisible to the circuit breaker, permanently.
Required fix: make lane ownership reason-aware in a single helper (e.g. lanePolicy.ownsCompaction(ctx, reason)) and use it at all three sites, instead of an inline event.reason !== "manual" && negation at one. A reader cannot currently tell which sites are manual-exempt.
BLOCKING: 2 — manual now clears every guard simultaneously
With 559 exempted, a manual compaction on an SDK-owned session is gated by nothing:
| Guard | State for manual |
Where |
|---|---|---|
| SDK lane ownership | bypassed by this PR | packages/coding-agent/src/core/extensions/builtin/compaction/index.ts:559 |
| Circuit breaker | shouldBypass() returns true |
compaction/circuit-breaker.ts:59 |
| Per-turn cap | no-op, always {cancel:false} |
compaction/per-turn-cap.ts:23 |
Combined with BLOCKING 1 (failures never recorded), repeated /compact on an SDK-owned session can drive unbounded summarization requests with no backoff, while the SDK concurrently owns automatic compaction of the same transcript. The PR body's "Threshold, overflow, speculative ownership, and model-capacity admission remain unchanged" is accurate but does not address that the lane being opened is itself unguarded.
Required fix: either keep the breaker live for manual on SDK-owned lanes, or add an explicit guard, plus a test proving repeated manual compaction cannot spin unbounded.
BLOCKING: 3 — no evidence of safety when SDK auto-compaction races an in-flight manual one
The handler now runs capturePendingMetadata, runOpenAiRemoteCompaction, the warm-job consume path and the speculative snapshot for manual on a lane whose transcript the SDK also compacts. I see no lock or generation guard covering that interleaving, and no test exercises it. Show the guard, or add the test.
BLOCKING: 4 — the faux parameterization is vacuous
Proven above: packages/coding-agent/test/suite/sdk-manual-compaction-recovery.test.ts:15 passes with the production change reverted. It contributes a green tick and no signal. Either make it a real control that asserts the non-SDK path is unaffected in a way that could fail, or drop it and say so.
BLOCKING: 5 — the PR does not merge
git merge-tree origin/main 369eb0330 → CONFLICT (content): Merge conflict in packages/coding-agent/CHANGELOG.md. main has advanced 5a23f6edf → 4a343b168. Rebase required.
Notes (non-blocking)
packages/coding-agent/test/claude-sdk-oauth-compaction-alignment.test.ts:218—it.each(["threshold","overflow","pre_prompt"])omitsbranchandextensionfromCompactionReason(core/extensions/types.ts:99). They still cancel correctly; the lane assertion is just incomplete.packages/coding-agent/test/claude-sdk-oauth-compaction-alignment.test.ts:181-194—beforeCompactEvent()ends inas unknown as SessionBeforeCompactEvent, so an invalidreasonin thatit.eachwould not be caught bytsc. The cast is pre-existing, but this PR now depends on it for correctness.packages/coding-agent/test/suite/sdk-manual-compaction-recovery.test.ts:17—vi.spyOn(Date,"now").mockReturnValue(...)freezes time for the whole case; fine here since the breaker cooldown is not under test, but it does mean this test could never catch a cooldown regression.- ✅ No coverage was lost by deleting "cancels a requested senpi compaction with the lane reason" — its default reason was
threshold, still the firstit.eachcase. - ✅ The red Windows CI is not this PR's fault.
RPC named pipes (Windows)fails identically onmain(run 34076954251) withconnect ENOENT \\.\pipe\senpi-rpc-...on a different test case.Check and testis red only as its aggregator. Do not chase it here.
Automated adversarial review. Every claim above was verified against source at 369eb0330; the RED capture is reproducible by reverting the single production hunk.
| invalidateSpeculativeCompaction(ctx); | ||
| try { | ||
| if (lanePolicy.disablesSenpiCompaction(ctx)) { | ||
| if (event.reason !== "manual" && lanePolicy.disablesSenpiCompaction(ctx)) { |
There was a problem hiding this comment.
BLOCKING: This reason-gates ownership at one of three disablesSenpiCompaction call sites (559, 766, 1008).
Consequence at line 766: a manual compaction that now proceeds and then fails no longer carries rejectionCause === "external-owner", so it reaches if (!lanePolicy.disablesSenpiCompaction(ctx)) — still true on this lane — and breaker.recordFailure() is never called. Failed manual compactions on SDK-owned sessions become invisible to the circuit breaker.
Combined with circuit-breaker.ts:59 (shouldBypass returns true for manual) and per-turn-cap.ts:23 (shouldRejectByCap is a no-op), manual compaction here is guarded by nothing.
Replace the inline negation with a reason-aware helper used at all three sites.
| }); | ||
|
|
||
| describe("explicit compaction recovers a rejected model downswitch", () => { | ||
| it.each(["claude-sdk-oauth", "faux"])("persists a usable manual summary on %s", async (provider) => { |
There was a problem hiding this comment.
BLOCKING: the faux case is vacuous. I reverted the production hunk and ran this file:
Tests 1 failed | 1 passed (2)
The claude-sdk-oauth case correctly went RED; faux passed without the fix. It cannot fail for the regression it appears to name, so it is a green tick with no signal. Make it a control that could actually fail, or drop it.
| describe("explicit compaction recovers a rejected model downswitch", () => { | ||
| it.each(["claude-sdk-oauth", "faux"])("persists a usable manual summary on %s", async (provider) => { | ||
| // Given the real builtin, no compaction-model override, and an oversized live transcript. | ||
| vi.spyOn(Date, "now").mockReturnValue(1_800_000_000_000); |
There was a problem hiding this comment.
Note: freezing Date.now for the whole case is fine for this assertion, but it guarantees this test can never catch a circuit-breaker cooldown regression — relevant because this PR routes manual compaction past the breaker.
| rejectionCause: "external-owner", | ||
| }); | ||
| }); | ||
| it.each(["threshold", "overflow", "pre_prompt"] as const)( |
There was a problem hiding this comment.
Note: CompactionReason (core/extensions/types.ts:99) is "manual" | "threshold" | "overflow" | "pre_prompt" | "branch" | "extension". This omits branch and extension.
They still cancel correctly today, so this is not blocking — but the helper this feeds (beforeCompactEvent(), :181-194) ends in as unknown as SessionBeforeCompactEvent, so tsc would not catch a bogus reason here either. The lane assertion is weaker than it looks.
| - `packages/coding-agent/src/core/extensions/builtin/compaction/index.ts` exempts manual requests from the SDK-native `session_before_compact` cancellation. Automatic threshold, overflow, pre-prompt, and speculative ownership remain SDK-owned. | ||
|
|
||
| ### Why | ||
|
|
There was a problem hiding this comment.
BLOCKING (doc accuracy): this says automatic lanes "remain SDK-owned", which is true, but omits that the manual lane being opened has no breaker and no cap (circuit-breaker.ts:59, per-turn-cap.ts:23). Document the guard situation for the path you are enabling, not only the ones you left alone.
|
|
||
| ### Fixed | ||
|
|
||
| - Explicit `/compact` now works on Claude SDK-owned sessions after a rejected smaller-model switch, while automatic SDK compaction ownership remains unchanged ([#1423](https://github.com/code-yeongyu/senpi/pull/1423) by [@realsigridjin](https://github.com/realsigridjin)). |
There was a problem hiding this comment.
BLOCKING (mergeability): this file is the sole merge conflict against main — git merge-tree origin/main 369eb0330 → CONFLICT (content). main moved 5a23f6edf → 4a343b168. Rebase and re-place this entry.
✅ Stage 1 complete → Stage 2 started\n\n**REQUEST_CHANGES review #5127971553** posted via the review API — , 6 inline comments.\n\n5 blockers, each verified against source at :\n\n| # | Blocker | Anchor |\n| --- | --- | --- |\n| 1 | Exemption covers 1 of 3 sites; failed manual compactions never reach | / |\n| 2 | Manual clears every guard at once — lane, breaker (), cap ( is a no-op) | — |\n| 3 | No evidence of safety when SDK auto-compaction races an in-flight manual one | — |\n| 4 | The case is vacuous — proven | |\n| 5 | Sole merge conflict vs | |\n\nOn blocker 4 — the RED capture. I reverted only the production hunk in a clean worktree and re-ran the new test:\n\n\n\nThe case is genuine coverage ✅. The case passed with the fix reverted — it cannot fail for the regression it names.\n\n>
|
369eb03 to
261662a
Compare
🔍 Stage 2 — fixes landed, then audited and corrected\n\n pushed (PR head updated, ). I audited that diff rather than trusting it, and four things were wrong. All corrected:\n\n| # | Problem in | Correction |\n| --- | --- | --- |\n| A | The rebase destroyed ** — the whole released section (#1422, the session-scoping fix, the goal re-prompt fix) and the header were deleted, an unrelated #7884 entry got a stray typo, and this PR's own #1423 entry vanished (which would fail the Changelog gate) | Restored the file from and re-added the #1423 entry under |\n| B | ** became dead code — both params ignored, always | Restored the manual escape hatch with a comment explaining why |\n| C | Blocker 4 was never addressed — the case was untouched | Documented it as an explicit control for the non-SDK lane |\n| D | collapses to , and used | Signature is now ; the predicate is a local, so destructuring cannot unbind it |\n\nOn (B) — I'm overruling my own blocker 2, deliberately. resolved it by removing the manual breaker bypass entirely, which means a tripped breaker refuses . That defeats this PR's whole purpose: a user whose downswitch was rejected is told to compact and retry, and during a window they would have had no recourse. It also changed behavior for every non-SDK lane, well outside this PR's scope.\n\nThe real substance of blocker 2 was that failed manual compactions were invisible to the breaker — and that is fixed by blocker 1's change at the failure-accounting site: manual failures are now recorded and count toward the trip that protects the automatic routes. The escape hatch stays open; the accounting is no longer blind. Manual compaction is user-initiated and self-limiting, so "unbounded automatic spin" was never the exposure there.\n\nBlocker status\n\n- ✅ 1 — reason-aware at admission + failure-accounting sites; left automatic-only\n- ✅ 2 — answered above: failures now recorded; escape hatch intentionally preserved\n- ✅ 3 — existing speculative-generation and message-revision checks documented in \n- ✅ 4 — case documented as a real control that now guards the shared predicate's non-SDK branch\n- ✅ 5 — rebased onto ; conflict resolved correctly this time\n\nVerification is running now (vitest scope + Version 5.9.3tsc: The TypeScript Compiler - Version 5.9.3 COMMON COMMANDS tsc tsc app.ts util.ts tsc -b tsc --init tsc -p ./path/to/tsconfig.json tsc --help --all tsc --noEmit COMMAND LINE FLAGS --help, -h --watch, -w --all --version, -v --init --project, -p --showConfig --build, -b COMMON COMPILER OPTIONS --pretty --declaration, -d Image Settings: Image Operators: Image Stack Operators: Miscellaneous Options: By default, the image format of 'file' is determined by its magic --declarationMap --emitDeclarationOnly --sourceMap --noEmit --target, -t --module, -m --lib --allowJs --checkJs --jsx --outFile --outDir --removeComments --strict --types --esModuleInterop You can learn about all of the compiler options at https://aka.ms/tsc + biome). Results and the pushed SHA land in the next comment. |
The rebase resolution removed the released 2026.9.7 section and this PR's own entry, and left a stray parenthesis in the restart-continuity entry. Restore the file from main and re-add the code-yeongyu#1423 entry under Unreleased.
…pped A tripped breaker must halt automatic compaction only. Refusing an explicit /compact during the cooldown strands the very session this recovery exists for, since a rejected downswitch tells the user to compact and retry. Manual failures are still recorded through the reason-aware ownership predicate, so they keep counting toward the trip that protects the automatic routes.
… binding ownsCompaction took 'manual' | string, which collapses to string and loses every compile-time guarantee; it now takes CompactionReason. The predicate is a local, so destructuring the policy object cannot unbind the receiver.
The faux case passes with the production change reverted, so it is not regression coverage. It is a control for the lane the exemption must not change, and it now guards the shared ownsCompaction predicate's non-SDK branch.
✅ Corrections pushed — → \n\nFour atomic commits on top of the fix:\n\n| SHA | Subject |\n| --- | --- |\n| | |\n| — | |\n| — | |\n| | |\n\nLocal verification at :\n\n\n\nA local error TS5058: The specified path does not exist: 'tsconfig.build.json'. reports for across ~20 pre-existing files (, , …) because the sibling workspace packages are not built in a bare worktree. Zero errors reference any file changed here — the grep explicitly included , , , and and matched none. The Static checks CI job typechecks against built siblings and is authoritative.\n\nPR is now (the earlier was API propagation lag, not a real conflict). CI is running at ; I'll report the result and merge once required checks are green. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
✅ Approving — all five blockers resolved
Re-verified against the final tree at 922b5e219:
| # | Blocker | Resolution (verified) |
|---|---|---|
| 1 | Exemption covered 1 of 3 ownership sites; failed manual compactions never reached recordFailure |
index.ts:559 and :766 both route through lanePolicy.ownsCompaction(ctx, reason). The message_end site (:1008) still uses disablesSenpiCompaction — correct, it is automatic-only. |
| 2 | Manual cleared every guard at once | Failures are now recorded via the accounting site, so they count toward the trip. The manual bypass of a tripped breaker is intentionally kept (circuit-breaker.ts:67) — removing it would refuse the /compact this PR exists to enable. Rationale in this comment. |
| 3 | No concurrency evidence | Existing speculative-generation and message-revision guards documented in changes.md. |
| 4 | faux case vacuous |
Documented as an explicit control for the non-SDK lane; it now guards the shared predicate's !disablesSenpiCompaction branch. |
| 5 | Merge conflict | Rebased onto 4a343b168; CHANGELOG.md is a clean 1 +. |
CI at 922b5e219: 20 pass, 0 fail.
RPC named pipes (Windows) — the job that was red when this started — passed in 4m30s. As called out earlier, it was a pre-existing named-pipe race that also failed on main (run 34076954251), never a defect in this PR. Static checks, Changelog gate, all four Test shards, Terminal tools ×3, Inspector handoff ×3, and Hooks trust storage are green.
Merging.
🎉 Merged — \n\n → at .\n\nFinal CI at : 20 pass, 0 fail.\n\nThe loop end to end:\n\n1. Brutal review — , 6 inline comments, 5 blockers.\n2. RED proof — reverted only the production hunk; the new test failed with while the case passed, proving that half vacuous.\n3. Fixes — reason-aware at the admission and failure-accounting sites, so failed manual compactions are no longer invisible to the circuit breaker.\n4. Audit — the first fix pass destroyed the released changelog section and reduced to dead code; both corrected before merge.\n5. Approval + merge — .\n\n, red when this started, passed in 4m30s — confirming the early call that it was a pre-existing named-pipe race that also failed on , not a defect here.\n\nThanks @realsigridjin — the core insight was right; it just needed the other two ownership sites to come along with it. |
2 participants
Summary
Allow explicit manual compaction while the Claude Agent SDK owns automatic compaction.
A large SDK session can correctly fail admission to a smaller-context model. The error tells the user to compact and retry, but the builtin ownership guard also cancelled explicit
/compactrequests withexternal-owner. Exempting onlyreason === "manual"makes that recovery path usable. Threshold, overflow, speculative ownership, and model-capacity admission remain unchanged.Verification
external-ownercancellation.Based on current
mainat5a23f6edf, containing latest releasev2026.9.6.Related work
This is a narrower, independently tested alternative to the manual
/compactportion of open #1268, without its fallback-restore commands. #1263 addresses oversized resumes; #1338 addresses incompatible retry candidates. Users still retry model selection after compaction; this does not introduce automatic downsizing.Summary by cubic
Explicit manual compaction is now allowed on SDK-owned sessions, fixing recovery after a failed model downswitch. Previously, the ownership guard cancelled manual
/compactrequests withexternal-owner; now only manual requests bypass that guard, while automatic threshold, overflow, and speculative compaction remain SDK-owned. Manual failures still trip the circuit breaker, but a tripped breaker no longer blocks an explicit/compact— it stays the user's escape hatch during the cooldown.Verification
threshold,overflow,pre_prompt) still get cancelled./compactstill succeeds while the circuit breaker is tripped.Written for commit 922b5e2. Summary will update on new commits.