Repository navigation
fix: Rehook compaction - #55208
fix: Rehook compaction#55208daniel-sc wants to merge 7 commits into
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks correct. I reproduced #54916 on the base with the bot's session-runner scenario: the shrunk compaction request contained text that a compaction hook had redacted. With this PR the request is still shrunk, and the text stays redacted. The closest core test suites (256 tests) and typecheck pass, and the new compaction cases fail on the base.
I checked the reworked deliver loop. Its fallbacks match the old behaviour: before any rejection, a request that can't be reduced is sent whole; after a rejection it fails; and "payload too large" still falls back to the full text version. Two small behaviour changes in native compaction: an auto compaction's Started event is now published before the endpoint check, and a hook-supplied result now wins over a disallowed endpoint rewrite. I don't think either matters, but they're worth knowing. This will likely conflict with #55146, which changes the same prepare() call.
Public API: no signatures change. The compaction and model.request session hooks can now run more than once per compaction (full draft, then a fresh reduced one), and the plugin docs now say so. This stays backwards compatible, except for hooks that assume they run once per compaction or that keep state between calls.
The one thing I'd change is the new test (inline comment).
Related: #50904
|
@rekram1-node could you review this compaction hook fix? Updated for the whole-conversation fallback and v2.1 checkpoint changes. It prevents shrinking from restoring text removed by hooks (#54916), and accounts for tools removed by hooks when budgeting retries. The focused tests and full workspace check pass. |
There was a problem hiding this comment.
Still looks correct after the rework for current dev. I re-ran the #54916 reproduction: on the base the shrunk compaction request contains the text a compaction hook removed; with this head it is shrunk and keeps the hook's redaction. The core compaction, runner and model-request suites (279 tests) and typecheck pass.
The earlier test feedback is addressed: the regression tests are now small standalone cases with no shared mutable state, and the new removed-tools and flattened-draft result cases follow the same pattern. In the reworked deliver, the choices of which request to send and when to give up after a rejection still behave as before.
Public API: no signatures change. The compaction and model.request session hooks can now run more than once per compaction, each time on a fresh draft, as the plugin docs now say. This stays backwards compatible, except for hooks that assume a single call per compaction.
No further concerns.
There was a problem hiding this comment.
The merge of #58015 keeps the fix. I re-ran the #54916 reproduction: it still leaks on the base and passes with this head. The closest core suites (280 tests) and typecheck pass. With the merge, reduced drafts are prepared without tool definitions before the hooks run, so the flattened request still carries no tools (hooks can't add ones that weren't passed in). The room kept for the system prompt also leaves tools out, as it should.
Public API: unchanged since my last review. The compaction and model.request hooks may run more than once per compaction, as documented.
One small thing: the existing "lowers the output limit" test's fixture was changed, but the original passes against this code (inline).
packages/core/test/session-compaction.test.ts:749: This changes the fixture of an existing test (a 250k-token user exchange becomes a 250k-token previous summary plus a new question), but it doesn't look necessary: I ran the base branch's unchangedsession-compaction.test.tsagainst this PR'scompaction.ts, and all 26 tests pass, this one included. Unless there's a case I'm missing, please revert this hunk to keep the diff to the fix.
|
This fixture change comes from upstream #58015 (ac68d6a), and is unchanged relative to the current PR base. Since that change allows user text to be shortened, the test uses a previous summary to exercise the unshrinkable fallback. I reran both fixtures on this head: the current one passes; the old one fails with maxTokens 30000 rather than 1024. I’d keep the upstream fixture. |
This branch has not been deployed
1 participant
Issue for this PR
Closes #54916
Type of change
What does this PR do?
Compaction shrinking can restore checkpoint text removed by a request hook.
Rebuild each reduced draft before rerunning
compactionandmodel.request. Earlier hook edits do not accumulate, and a suppliedevent.resultcompletes compaction immediately. Reduction budgets use the latest hooked system/tool overhead, so tools removed by a hook do not prevent a valid retry.Preserves the whole-conversation request when it fits, the retained-tail fallback, and oldest-first dropping. Updated for v2.1 checkpoint metadata. The plugin documentation describes repeated hook calls.
How did you verify your code works?
40 tests pass across the compaction, native compaction, model-request, and compaction-transport suites. Regression coverage checks redaction through shrinking and overflow, hook-supplied completion, retained-tail fallback, and removed-tool budgeting.
The removed-tool regression failed before the budgeting fix and passed afterward, including 10 consecutive runs. Full
bun run checkpasses (lint and all 34 workspace typecheck tasks).Screenshots / recordings
Not applicable.
Checklist
Added by the opencode bot (AI-generated).
Related: #50904
Public API: compaction and model.request plugin hooks may now run more than once per compaction, on a fresh reduced draft (documented; no signature changes)