Skip to content

fix(coding-agent): recover required compaction loops - #679

Merged
code-yeongyu merged 1 commit into
code-yeongyu:mainfrom
realsigridjin:fix/compaction-recovery-loop
Aug 3, 2026
Merged

fix(coding-agent): recover required compaction loops#679
code-yeongyu merged 1 commit into
code-yeongyu:mainfrom
realsigridjin:fix/compaction-recovery-loop

Conversation

@realsigridjin

@realsigridjin realsigridjin commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reset the automatic compaction soft cap per provider turn while preserving the universal session hard cap
  • resume provenance-confirmed required compaction, including missing provider usage anchors, without duplicate threshold errors or synthetic user input
  • reconstruct deterministic recovery from bounded replay-safe retained context, preserving task intent and the latest meaningful request

Verification

  • npm run check
  • 54 affected test files / 396 tests passed on current origin/main
  • npm run build
  • compiled large-skill, malformed replay, no-usage recovery, queue, and all-route hard-cap probes passed
  • independent goal, quality, security, QA, and upstream-context reviews passed

Summary by cubic

Fix required-compaction recovery so long sessions keep progressing and interrupted work resumes correctly. Resets the soft compaction cap per provider turn and adds safe, deterministic suffix recovery even without usage anchors.

  • Bug Fixes
    • Reset the automatic-compaction soft cap at each provider turn; keep the universal session hard cap.
    • After an accepted threshold compaction, resume the exact interrupted continuation (queued steering preserved). No synthetic "continue". Rejected recovery remains terminal.
    • Track provenance for required-compaction errors. Do not retry provider errors that mimic the same text. Persist only one threshold error per recovery sequence.
    • Deterministic recovery measures the reconstructed retained suffix, ignores stale pre-compaction usage, prefers the prepared boundary, and otherwise falls back to the latest meaningful user turn. Validate retained messages and fail closed on unsafe or oversized content.
    • When usage anchors are missing or zero, use the persisted byte-based estimate for recovery.

Written for commit e863482. Summary will update on new commits.

Review in cubic

@realsigridjin

Copy link
Copy Markdown
Contributor Author

@code-yeongyu All CI checks are green. I do not have upstream merge permission; please merge this PR when ready.

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sisyphus-bot] Brutal review of PR #679. CI is green and the headline behaviors are pinned, but five independent review lanes plus lead-owned reproduction found concrete, reproducible defects that must be fixed before merge. Requesting changes.

Blocker 1 (P1, runtime): successful required-compaction recovery still rejects the originating prompt

  • packages/coding-agent/src/core/agent-session.ts:1004-1009 stores _requiredCompactionAdmissionError when a queued continuation hits required compaction, but nothing clears it when the post-agent_end recovery is ACCEPTED and the queued continuation completes successfully. _promptAgent() (:1266-1271) then throws the stale error.
  • Deterministic repro (2/2 runs, faux provider; steer queued mid-turn, inline compaction rejected once, recovery accepted): the queued continuation is persisted ("queued continuation completed", provider calls = 2, both queues empty) yet the originating session.prompt() still rejects with "Context remains above the compaction threshold because compaction did not complete".
  • Requested change: clear the stored admission error when the recovery compaction is accepted and its queued continuation is scheduled/launched, plus a regression test at the post-compaction-recovery-bounds seam that queues the steer BEFORE the failed inline admission.

Blocker 2 (resource safety): deterministic fallback eagerly serializes retained messages unboundedly and executes accessors

  • packages/coding-agent/src/core/extensions/builtin/compaction/deterministic-fallback.ts:45-55 (estimateConservativeTokens) materializes JSON.stringify(message) for every retained message with no size bound and no abort observation. Measured: one 32 MiB retained text message adds ~207 MiB peak memory, synchronously, exactly during required recovery.
  • The same path invokes property getters on retained tool-call arguments (probe observed getterCalls: 2 from a crafted arguments object): persisted, attacker-influenceable data is executed-on-read during recovery sizing.
  • Requested change: bound the estimator with early exit once the contextWindow - reserveTokens budget is exceeded, byte-count strings without whole-message serialization, and reject accessor-bearing/non-plain values instead of reading them.

Blocker 3 (test integrity): shipped guards are not pinned

  • G1: moving the turn_end reset out of finally (skipping it on the lane-policy / degradation-recovery early returns) leaves the whole changed suite GREEN (48 files / 335 tests). The exact regression class this PR fixes is unprotected. Add a route-guards case that trips degradation recovery, fires turn_end, and asserts the next turn still admits compaction.
  • G2: deleting the agent_start provenance clear (agent-session.ts:1593-1595) leaves coverage GREEN; a same-text provider error in a later run can inherit stale required-compaction provenance. Add a guards test: complete one real required-compaction run, then feed the identical provider error text in a fresh run and assert no spurious retry/compaction.
  • C1: continues once after recovery clears the threshold queues only two faux responses while asserting three calls, so the final turn ends in an unasserted No more faux responses queued error. Queue a third response and assert its content.

Lane verdicts

  • behavior (deep): P1 blocker 1, reproduced.
  • safety (deep): blocker 2, measured (~207 MiB / 32 MiB input; accessor execution).
  • test review: blockers G1/G2/C1, with sensitivity proofs that the reset, hard-cap order, and provenance gates ARE load-bearing (each deletion goes RED in the shipped tests).
  • quality: PASS, no criterion-blocking findings.
  • runtime QA lane: BLOCKED by harness-write failure, no runtime evidence; lead will run the isolated real-CLI QA after fixes.

Clean-worktree receipt at review time: git status --porcelain empty in /Users/yeongyu/local-workspaces/senpi/local-ignore/worktrees/pr-679-brutal-review. Evidence: local-ignore/qa-evidence/20260803-pr-679-brutal-review/ (reviewer reports + repro scripts).

@code-yeongyu

Copy link
Copy Markdown
Owner

[sisyphus-bot] Review follow-up: all verified blockers from my request-changes review are now fixed and verified. Since I cannot push to the contributor fork, the fixes landed as a stacked branch fix/compaction-recovery-loop-review-fixes on code-yeongyu/senpi, containing this PR's commit plus three atomic fix commits:

  1. fix(coding-agent): supersede admission error after accepted recovery — clears the stale stored admission rejection when accepted recovery schedules the queued continuation (blocker 1, reproduced 2/2 runs; regression test added at the bounds seam).
  2. fix(coding-agent): bound deterministic fallback sizing — estimator fails closed on accessor/non-plain/cyclic values and exits early over budget (blocker 2; accessor test went RED before the fix).
  3. test(coding-agent): pin recovery guards and supersession changes — pins the turn-end reset across degradation-recovery early returns (mutation-proven RED), closes the unasserted third faux call, records changes.md.

One review item was declined after lead verification: the stale-provenance concern (G2) could not be reproduced as harmful — classification at agent_end also requires a still-over-threshold context, which the successful run-2 compaction eliminates; the mutation stayed GREEN under every formulation. Evidence: 28/28 changed-scope tests, typecheck, npm run check (exit 0), senpi-qa isolated channels 9/9 + 43/43 + 4/4 + 8/8, real auth unchanged.

code-yeongyu added a commit that referenced this pull request Aug 3, 2026
…p-review-fixes

fix(coding-agent): required-compaction review fixes for #679
@code-yeongyu
code-yeongyu merged commit e863482 into code-yeongyu:main Aug 3, 2026
14 checks passed
@realsigridjin
realsigridjin deleted the fix/compaction-recovery-loop branch August 3, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants