fix(core): Clamp compression output budget to remaining context window - #9109
Conversation
The compression side-query requested a fixed 20K output budget regardless of the room left in the window, so on small-window deployments a long session could push prompt + 20K over the window and the backend rejected the request with a 400 before generating (issue QwenLM#7960). Clamp the budget to window minus the estimated input and a small safety margin, floored at 1 so the request stays valid even when the estimate exhausts the window.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, well evidenced. #7960 reports the exact failure with a curl-level reproduction against a real vLLM backend (65,536-token window, ~45,537 prompt tokens + the fixed 20K output budget = 65,537 → 400 before generation), independently reproduced on a second deployment. Not theoretical. Direction: aligned — compression is the recovery mechanism for saturated sessions, and it failing precisely when the window is tight defeats its purpose. The change is what the issue itself proposed: clamp the output budget to the remaining window. Size: core path ( Approach: minimal — one exported pure function ( Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，证据充分。#7960 报告了精确的失败场景，并有对真实 vLLM backend 的 curl 级复现（65,536 token 窗口、约 45,537 prompt token + 固定 20K 输出预算 = 65,537 → 生成前被 400 拒绝），且在第二个独立部署上复现。不是理论性问题。 方向：对齐——压缩是会话接近窗口上限时的恢复机制，窗口紧张时它反而失败，等于失去了存在意义。修复方案正是 issue 本身建议的：把输出预算夹取到窗口剩余空间。 规模：核心路径（ 方案：最小化——一个导出的纯函数（ 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach matches what I'd have proposed independently: clamp the cold side-query's output budget to
Two non-blocking notes:
CI test evidenceThe PR's own CI on the reviewed commit: the ubuntu unit suite — the required check for PRs; macOS/Windows jobs run only in the merge queue and are expected-skipped here — is still in progress. This pass reports the snapshot as fetched; no polling. Prechecks, classification, labeling, and both Desktop Shell jobs passed; nothing red so far. Not verified: end-to-end behavior against a real small-window vLLM deployment (author states this is out of scope; the backend preflight is simulated in the test mock). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining gap as a sponsored run (the author lacks write access): 中文说明代码审查方案与我独立设想的一致：把冷 side-query 的输出预算夹取到「窗口 − 估算输入 − 安全余量」，下限 1，20K 上限保留为天花板。已对照代码核实：
两个非阻塞备注：
CI 测试证据被审 commit 上 PR 自身的 CI：ubuntu 单测套件（PR 的必需检查；macOS/Windows 只在 merge queue 运行，此处 skipped 属预期）仍在进行中。本次只报告抓取时的快照，不轮询。预检、分类、打标签、两个 Desktop Shell 任务均通过；目前没有红色。未验证：针对真实小窗口 vLLM 部署的端到端行为（作者声明超出范围；backend 预检由测试 mock 模拟）。 沙盒验证可以作为 sponsored run 补足剩余缺口（作者无写权限）： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a clean, minimal fix for a confirmed bug with a pinning regression test; two non-blocking nits (the truncation guard goes dead under clamped budgets, and the PR body cites two test files that don't exist). This is exactly the kind of PR the gate should wave through: a real reported failure with a curl-level reproduction, the fix the issue itself proposed, implemented as one pure function and one call site, behavior unchanged on large windows, and a regression test that simulates the backend preflight and fails without the clamp. The scope never grows beyond the problem. My honest reservations, neither blocking: under a clamped budget the summary-truncation guard can no longer fire, and the cold path has no XML backstop — contrary to the PR's risk note — so a truncated summary can be persisted on small windows. That is still strictly better than today's guaranteed 400, and the guard's own TODO already points at Approval is deferred until CI lands green on 中文说明置信度：4/5 —— 对已确认 bug 的干净、最小修复，带承重回归测试；两个非阻塞瑕疵（夹取预算下截断守卫失效、PR 正文引用了两个不存在的测试文件）。 这正是门禁应该放行的 PR：真实报告的失败、curl 级复现、issue 本身提议的修复方案、实现为一个纯函数加一个调用点、大窗口行为不变、回归测试模拟 backend 预检且去掉夹取就会失败。范围始终没有超出问题本身。 我如实保留的顾虑（均不阻塞）：夹取预算下摘要截断守卫无法再触发，且冷路径没有 XML 兜底——与 PR 风险说明相反——小窗口下截断的摘要可能被持久化。这仍严格优于现状（必然 400），守卫自身的 TODO 也已指出 批准延迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "PR #9109 (QwenLM/qwen-code) fixes #7960 by clamping the…": none — all candidate checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："PR #9109 (QwenLM/qwen-code) fixes #7960 by clamping the…"：none — all candidate checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Compare the summary-truncation guard against the budget actually requested instead of the fixed 20K ceiling, so it can still fire on clamped requests (the output can never exceed what was requested). Exclude the floor regime where the comparison degenerates. Key the budget to the receiving model's window: when the guard keeps a distinct compaction model, clamp against that model's window instead of the main model's, restoring the pre-clamp ceiling there. Also deduplicate the payload estimate shared by the compaction-model guard and the budget clamp (memoized and lazy), log the budget when it is clamped, correct the floor-at-1 docstring rationale, and cross-reference the send-path clamp in core/tokenLimits.ts.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "PR #9109 fixes issue #7960 by clamping the compression…": none — all checks I started completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget..
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）："PR #9109 fixes issue #7960 by clamping the compression…"：none — all checks I started completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.。
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…lamp - Drop the budget-1 guard exclusion: any output at a 1-token cap is definitionally truncated and must be dropped, not persisted - Key the truncation threshold to the count's provenance: provider counts compare against the clamped budget, local estimates keep the fixed 20K ceiling to avoid estimator false positives - Update CompressionStatus.COMPRESSION_FAILED_OUTPUT_TRUNCATED doc - Bump degenerate window=1000 fixtures to realistic windows - Add a regression test driving a floored budget of 1 through compress()
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "PR #9109 fixes issue #7960 by clamping the compression…": did not run the new test file or the existing suite to confirm the issue-7960 tests pass as written (review-only scope, evidence gathered by reading).; "PR #9109 fixes issue #7960 by clamping the compression…": did not quantify real-world Qwen tokenizer chars/token ratios for CJK input beyond verifying the codebase's own two estimators disagree by up to 6× (Finding 2's…; "PR #9109 fixes issue #7960 by clamping the compression…": none (finished well under the tool ceiling).; "PR #9109 fixes issue #7960 by clamping the compression…": did not run the new/modified vitest files or typecheck (static review only); "PR #9109 fixes issue #7960 by clamping the compression…": did not trace resolveModelId internals beyond confirming its export and call shape, and 2 more.
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）："PR #9109 fixes issue #7960 by clamping the compression…"：did not run the new test file or the existing suite to confirm the issue-7960 tests pass as written (review-only scope, evidence gathered by reading).；"PR #9109 fixes issue #7960 by clamping the compression…"：did not quantify real-world Qwen tokenizer chars/token ratios for CJK input beyond verifying the codebase's own two estimators disagree by up to 6× (Finding 2's…；"PR #9109 fixes issue #7960 by clamping the compression…"：none (finished well under the tool ceiling).；"PR #9109 fixes issue #7960 by clamping the compression…"：did not run the new/modified vitest files or typecheck (static review only)；"PR #9109 fixes issue #7960 by clamping the compression…"：did not trace resolveModelId internals beyond confirming its export and call shape，另有 2 条。
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Close the floor-regime hole on the usage-missing path: at a floored budget of 1 the truncation guard now drops locally estimated output too, since no complete summary can exist at a 1-token cap. Add regression coverage for both the floor + missing-usage combination and the provenance split (clamped budget + complete estimated summary). Correct the safety-margin docstring to not overstate its guarantee against proportional estimator error. Fold the issue-7960 regression suite into the colocated main test file per repo convention.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the 5-round cap with one dry round; two-consecutive-dry convergence was not reached.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget..
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the 5-round cap with one dry round; two-consecutive-dry convergence was not reached。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.。
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Close the clamped-budget hole on the usage-missing path: when the output budget was clamped and the count is a local estimate, the threshold comparison cannot detect cap-hits (output never exceeds the requested budget and the estimator tops out at ~1.5x actual tokens), so gate acceptance on snapshot well-formedness instead — a cap-hit fragment lacks the closed state_snapshot tag.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
The docstring claimed the residual 400 means compression 'fails safely', but the setup-phase side-query has no catch around it: the error propagates to the caller without advancing the consecutive-failure breaker. Correct the wording; mapping the 400 to a breaker-counted failure status is follow-up issue QwenLM#9115's scope.
…#9175) * fix(review): repair seven pipeline defects found by live runs Four full reviews (PRs QwenLM#9113, QwenLM#9094, QwenLM#9109, QwenLM#9106) were run headless against qwen3.8-max and watched step by step. Every one of them reached a verdict, and every one of them also exposed a defect in the pipeline itself. Each fix below carries the measurement that found it. The incremental anchor was being withheld for a reason that says nothing about which lines were read. A dimension nobody can run — on this repo, the integration suite CI skips and the local budget cannot fit — capped the verdict, the cap withheld the anchor, and the missing anchor sent the next round over the full diff again: 119 minutes and 34M input tokens on a pull request whose code had not changed a line. The anchor now answers to the coverage evidence alone, recomputed from the harness's own transcripts, so a depth gap no longer costs a range. Two of the four runs had capped and two had not, on identical mechanical facts, which is the second reason this could not stay a judgment call. The build-and-test dimension could not finish, and no amount of reallocation would have made it. One shell call is capped at ten minutes by the tool itself; this repo needs more than that (install, the scoped builds, then a core suite at 106s and a CLI suite measured at 401s). Since the ceiling is per call, the run now continues across calls: a resumed call reuses the installed and compiled tree, runs only the suites the previous call could not reach, and merges into the same report. A suite killed on a deadline the budget had shortened is recorded as provisional rather than as a timeout, so the continuation knows to give it a full window instead of reproducing the kill. The per-command deadline is sized to the slowest measured suite, and the whole-call budget is now derived from the tool's ceiling rather than from the deadline. A machine ledger posted by another account was invisible, which turned off cross-round recovery in precisely the case it was built for: continuous integration posts as a bot and a maintainer runs as themselves. The two halves of that ledger are not the same claim — the findings are a work list every round re-rules against the code, while the reviewed-at commit decides which lines the next round skips — so the list now travels across accounts and the anchor never does. The attribution of test failures was reading a truncated record. The failing file set for the pull request side was re-parsed from a bounded report, which on a live run recovered one file out of eleven; the same set is now measured where the raw output still exists, before the bound is applied. The loss ran in the direction that matters: a file only the pull request side fails is exactly the one that should have been reported. Three smaller repairs round it out. A review launched from the bundle never learned which build it belonged to, so every helper command it shelled out to resolved whatever happened to be installed on the path — one live run died on its second command. A "nothing to disclose" answer written in Chinese was classified as a real gap and published as one, in a body whose own evidence said the opposite. And a count field surrounded by list fields was sent a list twice in four runs, so it now says what to send instead. * fix(review): tell a continuation that ended before its test phase apart from one that finished A run that never reached its test phase — a failed install, the disk-space gate, a budget spent during the build, a deliberate build-only probe — carries neither a test scope nor a test result, so a continuation finds nothing to do and said the run had reached every suite in scope. That is the same shape this branch fixes elsewhere: prose asserting the opposite of the evidence beside it. It now says no suite ran and names the fix, and a test pins the two facts apart. * fix(review): close the nine findings the pipeline raised on this branch The review this branch changes was pointed at this branch, and it filed three blockers and six suggestions. All of them hold; each fix carries the probe that found it. The Chinese placeholder classifier was dropping real disclosures. It claimed to mirror the English branch's narrowing and did not: without an all-done head the span before the completion word swallowed a gap clause, the single-character negation lookbehind let "not yet finished" read as finished, and forty free characters after the completion word swallowed the clause carrying the gap. Eight real sentences were classified as no-answers, which is the direction that certifies depth nobody reached. The branch now requires the same head the English side has always required, extends the lookbehinds, and ends at the completion word with only a budget adverbial after it. A continuation was declaring victory it had not won. A retry admitted late is killed again — the ordinary outcome when an expensive suite gets the tail of a budget — and the merged note still said every suite in scope had run, while a provisional result sat in the report. Retries the budget never reached were dropped from the accounting altogether, because a retry is a command and the not-run list holds workspaces. Both now count, and the note names what is still provisional and why another continuation is worth it. A continuation also dropped the framing an install failure owes the reader. The fresh path prepends it on every return precisely because the structured field alone was judged insufficient; the merge replaced the note wholesale, so a report carrying a non-zero install arrived with nothing saying that failure is infrastructure rather than something to file against the pull request. The framing is now shared between both paths. The rest are smaller. The budget help text described an admission rule this branch had already replaced, contradicting the code, the tests, and the sibling flag's own description. The continuation test could not fail: it searched the whole prompt, so text from the first invocation block and the sibling brief satisfied every assertion, and deleting the block it guards left it green — it now isolates that block, and a mutation confirms it fails when the block goes. The report guard checked one of the three arrays the merge walks, so a partial report died on a raw type error instead of the named refusal that is the guard's whole purpose. Two prose copies of a constant this branch consolidated now interpolate it. And the comment at the ledger recovery site still described the own-account-only model this branch removes, at exactly the boundary a reader auditing the change would consult. * fix(review): answer the second review round's blockers Ten more blockers, from the pipeline reviewing this branch a second time. The sharpest one is about the change this branch makes to the anchor. Exempting an unreviewed dimension from withholding the anchor was too wide. The field carries two different claims: a dimension nobody could RUN, which says nothing about which lines were read, and a lens that whiffed — made some tool calls, opened some files, returned nothing substantive twice — which is a claim about lines that no machine detector produces. The first cut exempted both, so a twice-whiffed security pass could advance the range past the lines it never reviewed, and the fixture that pinned exactly that was deleted along the way. The exemption now rests on a fact the code can check rather than on the field's name: exactly one dimension declares that it reads no diff, and only its gaps are depth. The deleted fixture is restored, and the other direction is pinned beside it. The Chinese no-answer classifier is rewritten as a closed vocabulary. Spelling the completion clause as bounded spans that merely refuse to cross an exception word invited exactly what two review rounds then found: negations the single-character lookbehind could not see, inability modifiers, hedged completions, gap clauses swallowed on either side of the completion word, and a span that slid past a negated completion to a later affirmed one. Every one of those drops a real disclosure, which is the direction this module's own header calls the worse of the two. A closed vocabulary cannot be walked through, because there is nothing to walk: a sentence carrying a gap is built from pieces the clause does not contain, so it fails to match and is kept. All thirteen evasions are now keep-tested against it. A continuation could destroy the report it was asked to continue. When no toolchain applied at the worktree root, the early returns built a fresh report with no reference to the previous one, and the handler wrote it over the file the run had just read — a wrong or pruned worktree path would replace an in-flight report with an empty one, and the chain stayed dead after the path was fixed. A continuation now refuses instead, and the refusal leaves the file untouched. Recovery across accounts could also run the round counter backward. Rounds are an id space, so a recovered round that goes down re-issues ids the pull request already carries against different findings. A bot whose own recovery failed transiently posts a round-one marker after a round-seven one, and ordering by timestamp alone hands the next round a two. The counter only ever advances, so preferring the highest round cannot lose a newer work list. The rest: the report guard now validates the scope the merge walks, not only its arrays; the ledger's one uncapped field is capped now that the read path takes text any account can post; the build-and-test brief names the third shape of unfinished work, a single-package repo whose budget stopped before its only suite; and the continuation test builds its paths the way its neighbours do instead of spelling them for one platform. * fix(review): answer the third round, whose blockers were all mine Every blocker this round came from last round's fixes. That is worth saying plainly: the repairs were made under review pressure and shipped their own defects, and the pipeline caught each one. Two were outright bugs in the report guard. It read a field off the parsed value before checking that value was an object, so a file containing the literal `null` produced the raw type error the guard exists to replace — and it checked that the command lists were lists without checking what was in them, so a list holding a null cleared the gate and died one layer deeper. Both refuse now, with the recovery instruction the sibling branches already carried. The Chinese clause had reintroduced the overlapping-quantifier shape this module's header bans and its linearity test exists for: four optional groups chained across whitespace matchers, in a language that does not put whitespace between those tokens. The whitespace bought nothing but the backtracking. It is gone, and the pathological input that walks that shape is now in the linearity test beside the others. The anchor decision was reading the dimension list after a pre-existing splice had already removed every entry mentioning the review time budget — a splice that exists so the body does not say one gap twice, and that matches on a phrase. An entry whose free-form reason merely mentions the budget was therefore invisible to the decision, and that entry is exactly the line-coverage claim the decision must respect. It now reads the list as disclosed and renders from the spliced one. And the two copies of the continuation rule disagreed with each other inside one prompt about the shape that cannot be continued at all. They now say the same thing: report the dimension unfinished, and do not spend a continuation on a report that has no scope for one to read. * fix(review): answer the fourth round; the anchor list has three writers The dimension list the anchor decision reads is written at three different points — the caller's own entries, the budget-phrase splice that removes some of them, and the deterministic gates that push their machine-owed debts in later — and the last two rounds each fixed one end by breaking the other. The first version read after the splice and missed the entries it removed; the second read before the gates and missed an unlinted script or an unwalked defect layer, either of which is a line-coverage claim, not a dimension nobody could run. It now reads the live list at the decision point plus the entries the splice removed, which is the only view that sees every writer. Two more from making foreign markers authoritative. The round a marker claims is the id space itself, so preferring the highest round hands an unbounded one from any poster a permanent win — and past 2^53 the increment stops advancing, so every later round re-stamps the same ids against different findings. Rounds are now capped on read and mirrored on write, fail-quiet like every other malformation. And a transient identity lookup no longer costs the recovery: it used to degrade to "no ledger", which leaves this machine's side file at the round it last wrote while other accounts post past it, so the next verdict re-issues ids the pull request already carries. The lookup is isolated now, and a failure recovers the work list as foreign — no anchor rides on an identity the run could not confirm. * fix(review): give the report a run identity; keep foreign ids out of the pipeline's namespace The fifth round found the two holes still open in the two boundaries this branch loosened, and both fixes are about identity rather than shape. A continuation trusted any well-shaped report at the out path, and that path is stable across review rounds while nothing sweeps it on an interrupted one — so the report a crashed round leaves behind is exactly what the next round's resume finds. Continuing it keeps the old commit's passing entries on the new round's tree, certifying old-commit passes for the new commit, and skips the install the fresh worktree never had. Every report now records the run it belongs to — the tree it ran in, and the commit its plan fetched — and a continuation refuses anything else, a report with no identity included: one that cannot prove it belongs here reads the same as one that provably does not. And the trust split stripped the anchor from a foreign marker but let its finding ids through, which hands any account the pipeline's own namespace: a marker at round N carrying ids from round N+1 pre-claims exactly the prefix the next compose stamps, splitting one claim across two ids and renumbering every genuinely new finding past the squatted block. A legitimate marker cannot claim an id from a future round — a round stamps its own ids and carries older ones forward — so the parser now drops any finding that does, read-side only, because the writer cannot produce the violation. * fix(review): refuse element-level corruption instead of crashing on it Two crash shapes from the sixth round, both the same lesson one level deeper than the round-three fix that checked the lists: the elements are the payload. A continuation's not-run entries become shell commands, so a list holding a null cleared an arrays-only guard and crashed in the escaper instead of refusing with the named fix. And the failing-file set rides a report file anything may have edited, with exactly one consumer — a set that is not a string array reached the set arithmetic as-is, where the honest reading is the one an absent field has always taken: this seam supplied no measurement, fall back to re-parsing the stored output. * fix(review): answer the seventh round — instance identity, relay independence, and the cap round-trip Five blockers, all in this branch's own additions, three of them in the fixes earlier rounds bought. The run identity was strings where it needed an instance. The path and the sha are identical after fetch-pr destroys and recreates the worktree — which it does every round — so the identity check admitted a continuation onto a bare tree with no installed or compiled state, whose every suite then failed with resolution errors framed as candidate findings against the pull request. The report now records the worktree root's inode and birth time, which a recreated directory cannot keep, and a continuation onto a previous instance refuses with the reason spelled out. No legitimate continuation crosses a recreation; the valid resumes all happen inside one round. The anchor decision was relay-dependent for a budget or round-cap stop: identical machine state carried the anchor when the orchestrator dropped the mandated stop entry and withheld it when the entry was relayed — and the stderr instruction mandates the relay, so every compliant run paid the full-diff re-review this exemption exists to end. The stop's relayed entry now classifies as depth, and only against the marker the machine itself wrote: no marker, no exemption, so stop-shaped prose cannot buy an anchor the state does not support, and a lens entry that mentions the phrase in its reason withholds as before. The caveat gets the cure the note already had. A resume appended to it, so a completed chain still read "still to run" over suites that had just passed, and the dimension brief tells the agent to quote a present caveat as possibly-incomplete scope. Superseded budget-stop and earlier resume segments are retired, live limitations survive verbatim, and a chain that finishes with none ends with the caveat absent — the field's own contract for full coverage. The round cap broke its own round-trip: the stamp was uncapped while the serializer clamped, so at exactly the cap the writer produced a marker whose own parser dropped every finding — invisibly, with the anchor still riding. The one writer now caps its stamp, which also makes the squat filter's premise true again. And the bare "check" noun in the Chinese token made the classifier drop standalone "did not check" lines — a live gap under the brief's own rule that the line is only written when something was cut short. The noun group keeps the two documented placeholder nouns and nothing else. * fix(review): answer the eighth round — exact machine text, anchored retirement, and the local round's edge Four blockers; three sit in the seventh round's fixes and one is the second round's finding still alive under its replacement. The stop exemption matched a head plus a phrase, and that shape also covers a genuine line-coverage claim whose whiffed scope IS the reverse audit — an entry the phrase splice then removes from the rendered body, so the anchor rode past a whiffed audit while the posted review showed only the benign disclosure. The exemption now matches the exact entries the machinery mints, nothing looser: marker-anchored and text-anchored, and an edited or paraphrased relay withholds, which is the safe direction. The caveat retirement had two holes with one shape. Its regex matched the marker phrases anywhere in a segment, and segments interpolate file names from the reviewed diff — so a file named after the phrase retired the live limitation quoting it, untrusted input silently certifying scope. And the resume clause itself emitted the segment separator inside one clause, so a second continuation cut it in half: the head retired, the tail — "N still to run" — survived into a report whose note says everything ran. Retirement now anchors on the producers' own grammar at the segment start, and the clause is one segment. The run identity had no edge a local round crosses. Local plans carry no sha and the project root is never recreated, so every clause compared equal across rounds and an interrupted round's report certified pre-edit results for the edited tree. The plan file is the one thing every round writes afresh in both modes; its mtime now rides the identity, and a report stamped against a previous round's plan refuses with the reason named. The report gate and the delta seam also stop trusting vacuous content: workspace names and commands must be non-empty — an empty workspace resolves npm to the root suite, a different measurement wearing the requested one's name — timed-out entries are strings, and an empty failing-file set reads as no measurement, since the producer omits the field rather than writing one. * fix(review): answer the ninth round — structural caveats, bounded foreign rounds, and the starved suite Four blockers. One had stood since round one, and two survive earlier fixes in sharper forms; the shape of the repairs is the lesson of the round. The caveat is no longer parsed at all. Two attempts at retiring superseded clauses by re-reading rendered prose both lost to PR-authored names — first a filename matching the phrase, then a workspace dir embedding the segment separator plus the clause grammar, which fabricated a boundary and retired the live limitation's honest tail with the fake clause. The fresh run now records the scope's own caveat in a separate field whenever it appends a machine clause; a continuation carries that string through untouched and rebuilds the joined prose from it plus its own current clause. Absent means nothing was appended and the whole caveat is live. Nothing content-matches, so nothing can be talked out of a limitation by a name in the reviewed diff. The resume path now honors the ordering invariant the fresh path documents. Not-run suites are stored alphabetically, and a continuation that consumed them verbatim starved the changed workspace's suite to the budget's worst tail on every continuation — a chain could hit its cap with the one suite the diff changed never run, disclosed but never measured. Retries first, then the affected suites, then the dependents. A foreign round implausibly far past this account's own is no longer adopted. Round-first selection had a fixed point an attacker could pin with one post: a round at the cap outranks every real round forever, the capped stamp holds the counter there, and every later round re-issues the same ids against different findings. Rounds advance one per posted review, so a legitimate interleave sits a handful ahead at most; beyond our own highest plus a headroom of sixty-four, a foreign marker is not a newer work list — it is not a work list at all. Inside the bound, a hostile post can only inflate the counter by a bounded step, which costs numbers and nothing else. And the stop-entry splice reads both languages, because the exemption already admits the Chinese pair as a compliant relay: with an English-only phrase the relayed Chinese entry survived into the whiffed-dimension rendering beside the structural stop line — the same gap said twice, one copy under the wrong cause. The relay-independence test now asserts the strongest form available: the body is byte-identical whether the entry was relayed in English, relayed in Chinese, or dropped. * fix(review): answer the tenth round — the cache reads the marker, and a typecheck slip Two blockers. One is mine in the plainest sense: a test added late in round nine called a helper with too few arguments, and the local gate that would have caught it — the typecheck — was the one step that round skipped. Fixed, and the same test now also relays the Chinese round-cap pair, pinning byte identity across all three relay states for that branch too. The other is the first round's finding come back through the document: the skill's cache rule was a hand-copied condition list, and it aged out of sync with the module the moment the anchor net grew the depth-only distinction — a whiffed-lens round had its sha withheld by the marker and cached by the prose. The rule is now mechanical instead of descriptive: the cache advances exactly when the composed body's marker carries a sha, because the module already computed the net and two copies of it is how the two anchors came to disagree about what a clean round is. From the suggestions: resume with build-only now refuses instead of silently ignoring the flag — the pair names no work at all; the lockfile's merge-collateral peer-flag churn is reverted to main's copy; and three comments now say what is true — the inode does not separate instances on a filesystem that reuses them (the plan mtime is the cross-round floor there), an inside-bound hostile round can still win one recovery's work list (which is re-ruled, like every foreign work list), and a failed identity lookup bounds recovery to the headroom rather than recovering everything. * fix(review): answer the eleventh round — a proven identity, a validated identity, and the bullet the merge ate Three blockers, and the middle one is a regression my own round-four fix introduced. The isolated identity lookup turned a rate-limit blip into proof of absence. With the lookup's failure swallowed into a null login, the recovery walk had no name to look for, recorded "no own review exists" about an identity it never knew, and the deletion arm removed the side file and reset the round counter — the id-space collision this whole recovery redesign exists to prevent, delivered by a transient network error. The pre-isolation code got this right by accident, because the throw reached the outer catch and took the conservative strip path. Deletion now requires a CONFIRMED identity, and the handler-level pin drives the real handler with a throwing lookup and asserts nothing is removed. The identity gate crashed on the one corrupt shape it did not name: the report's run field was never validated, so `tree: null` slipped past a presence-only check and died on a null dereference inside the very gate that exists to refuse with a named fix. The gate now validates everything the identity check walks — root, sha, plan, and the tree fingerprint's own fields — and the crash shapes joined the refusal table. And the merge resolution had eaten the skill's documentation for a state field the module still consumes: the duplicate-drop account's bullet was adjacent to a bullet this branch rewrote, and the conflict resolution kept the rewrite and dropped the neighbour — leaving a channel the verdict arithmetic counts with no documented way to reach it. Restored verbatim. From the suggestions: the lockfile's merge-collateral churn is reverted again — this time after the last npm operation rather than before the next one — and the seven comments the sweep named now describe the shipped mechanisms: the structural caveat carry-through that replaced both parses, the stamp that writes this build's own entry instead of blanking the slot, the truncation flag that fires only on the re-parse fallback and loses in both directions, the depth exemption's full exception set, and the continuation contract's retry-first half. * fix(review): answer the twelfth round — the union that ends displacement, and the gate's last fields Two blockers. A foreign marker one round ahead — deep inside the plausibility bound — could displace this account's entire work list, and displaced entries owed no ruling, so one drive-by comment retired a certified Critical from the marker chain for the rest of the PR's life; the doctored variant copies the own list minus the entry to suppress. A foreign winner is now MERGED over the own latest findings instead of replacing them: own entries are authoritative on an id collision, foreign entries with new ids join after, the merged list re-caps with an honest dropped count, and the round number still advances — the counter is a shared id space, the work list is not a prize. The headroom comment now says what is true because of this, rather than what was hoped. And the continuation's shape gate now validates the last fields the merge walks: a non-iterable affected crashed the ordering seed, a string notBuilt crashed the refusal's join, and a bare true notBuilt — the worst shape — skipped the unbuilt-tree refusal silently and ran suites against packages that were never compiled, manufacturing the exact failures the refusal exists to prevent. The caveat strings are typed too, since the brief quotes them. From the suggestions: the resumed note carries the caveat exactly as the fresh path does, counts build failures into its passed/failed sentence so it cannot contradict the recomputed ok beside it, and the stray duplicated doc-comment opener is gone. * fix(review): answer the thirteenth round — exec evidence, a command grammar, and an empty login Three blockers from the automatic review, all probe-confirmed on the merge head. R1-11 (isUnusableScriptEntry): the gate answered 'usable' by extension enumeration — anything outside .js/.mjs/.cjs returned usable with no isFile/X_OK/shebang check — so the childEnv stamp could hand skill subcommands an entry no shell can exec: a tsx dev launch stamps the 0644 index.ts, and 'node <pkg-dir>' stamps the DIRECTORY (which passes an X_OK probe as search permission). Every '"${QWEN_CODE_CLI:-qwen}"' then died on exit 126 where empty would have fallen back. The gate now demands positive evidence: a regular file with the execute bit, a '#!' header for any known script extension (the TS family included), no shebang needed for native binaries. Both entrances are pinned at the childEnv level and the primitive's own suite covers the three shapes. R3-38 (--resume injection): the continuation re-executed report-stored test[].command strings verbatim under shell: true, while the identity check pins a report to this run's tree — not to this program's authorship — and the gate's own corrupt-report fixtures declare the edited-in-place report the accepted threat model. Stored commands are now held to the emitter's grammar (npm test [--workspace="<dir>"]), the same policy test-delta already applies before re-running report commands; an alien command refuses the resume with a named fix, before anything runs. identityKnown empty-login: currentUser() answering empty-with-exit-0 (a stubbed or proxied gh) set identityKnown=true while recoverLedger reads '' as unknown, so the deletion arm could remove the prev-ledger side file — resetting the round counter — over an identity that was never proven. Empty is now unknown, matching presubmit's own '' handling. Each fix is pinned by a test its reverted mutant fails. * fix(review): answer the fourteenth round — an anonymous recovery cannot swap the list R13-1: during an identity-lookup outage (currentUser throws, or answers empty) every marker walks as FOREIGN — there is no me — so the union that protects the certified work list never has an own side to merge over, and the recovered winner was written to the side file WHOLESALE: a drive-by marker posted at this account's current round (visible to any authenticated user, inside the zero-base headroom) replaced this machine's last known-good list on equal round + larger review id, and the swap persisted — the marker stays on the PR, so every later outage reopened it. The suppression class the union merge exists to kill, reopened through the anonymous path. persistRecoveredLedger now takes identityKnown and gives the anonymous recovery its own outcome: with a readable existing file, a same-round winner changes NOTHING, and a strictly-higher round advances only the round counter (refusing that too would re-expose R4-2's id-space collision — a lagging counter re-issues ids the PR already carries) plus the reviewId tiebreak; the findings stay this machine's own, and sha/commitId are dropped — an anonymous round cannot be re-vouched, and an anchor superseded by rounds this account never certified must not scope the next review (the healthy foreign-winner path strips it at the recovery seam for the same reason). With no readable file there is nothing to protect and the write stays wholesale. The new suite pins the drive-by (file byte-identical), the advance (counter moves, list survives, anchor and age reference gone — with noOwnReview deliberately true so a positional swap of the two booleans deletes the file and fails the test), and the no-file arm; the guard mutant fails exactly the first two.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/services/chatCompressionService.ts:984 — [probe] early-closed snapshot + trailing generation to the cap persists truncated trailing text as COMPRESSED
中文说明
无阻断问题。LGTM！✅
Test Plan（非阻断）：packages/core/src/services/chatCompressionService.issue-7960.test.ts — no such file or directory; packages/core/src/services/chatCompressionService.test-turn-2-fix.test.ts — no such file or directory; packages/core/src/services/contextCompressionService.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the full diff at a1b35332; CI is fully green on this head.
What I checked:
computeCompactionOutputBudgetis a pure clamp (max(1, min(20K, window - estimate - margin))), andbudgetWindowonly switches to the compaction model's window on the branch where that model is actually kept — so the budget always clamps against the window of the model that receives the side-query.- The truncation guard is correctly keyed to count provenance: provider-reported counts compare against the clamped budget (comparing against the fixed ceiling would make the guard unreachable on every clamped request), while local estimates keep the fixed-ceiling threshold to avoid converting ±30% estimator variance into false truncation verdicts — with the budget-1 floor regime as the deliberate exception on both paths.
- The new well-formedness guard covers the one gap the threshold comparison can't see (clamped budget + local estimate), and
</state_snapshot>presence is a sound truncation signal given the directive mandates the XML. - Memoized estimate keeps the compaction-model guard and the budget clamp sharing the same leading terms, so they can't drift; large-window behavior resolves to the same 20K as before.
One acknowledged residual (documented in code, not blocking): proportional tokenizer error beyond the fixed 1K margin can still push prompt + max_tokens over the window and surface as a backend 400 — strictly better than today's guaranteed 400 on small windows.
LGTM.
|
Released in v0.21.14. |
3 participants
What this PR does
The compression side-query previously requested a fixed 20K-token output budget regardless of how much room the session's prompt actually left in the model's context window. This PR computes the budget per request instead: the fixed ceiling clamped to the receiving model's remaining window (window minus the estimated prompt size minus a small safety margin for tokenizer estimation error), floored at 1 so the request parameters stay valid even when the estimate already exhausts the window. On large windows the budget resolves to the same 20K as before — behavior is unchanged there. The summary-truncation guard now compares the reported output count against the budget actually requested (output can never exceed what was requested, so comparing against the fixed ceiling would make the guard unreachable on every clamped request), and a debug log records the budget whenever it is clamped.
Why it's needed
Providers validate that prompt tokens plus the requested output budget fit within the model's context window before generating anything. On small-window deployments (e.g. a local vLLM server started with a reduced max model length), a long session can leave less than the fixed 20K of headroom. The compression request then exceeds the window and the backend rejects it with a 400 before the model ever runs — so compression, the very mechanism meant to recover the session, fails instead. Reported in #7960: with a 65,536-token window and a prompt of roughly 45,537 tokens, 45,537 + 20,000 = 65,537 exceeded the window by exactly one token and every compaction attempt failed. The same unclamped ceiling also left reactive rescue compaction unable to fire when a user prompt pushes the main request over the window. The existing compaction-model guard rejects oversized compaction models against a default window, but never clamped the real limit — this closes that gap.
Reviewer Test Plan
How to verify
The core scenario is covered by a regression test that simulates a backend enforcing the window preflight: with a 65,536-token window and a ~45.5K-token session history, compression before this change fails with the exact "exceeds the model's context window" 400 from the issue, and after the change succeeds with a budget clamped to ~19K so that prompt plus budget fits the window. Behaviors worth confirming: on a large window (128K) the requested budget stays at the full 20K ceiling, i.e. no behavior change for typical deployments; when the model output hits the clamped budget, the truncation guard drops the potentially-truncated summary and reports the distinct truncation-failure status instead of persisting it; when a distinct compaction model with a larger window is kept, the budget keys to that model's window so the summary ceiling does not regress; and when the estimated input already exceeds the window, the budget floors at 1 keeping the request valid — a regime where the backend normally rejects the oversized prompt anyway.
Evidence (Before & After)
N/A — no user-visible / TUI change. Unit test evidence:
Tested on
Environment (optional)
Unit tests only (
npm install,npx vitest,npm run typecheck,npm run lint). No live backend was used; the backend preflight is simulated in the test mock.Risk & Scope
finish_reason) is not surfaced by the side-query plumbing yet (tracked by the existing TODO).Linked Issues
Fixes #7960
Follow-up: #9115
中文说明
本 PR 做了什么
压缩 side-query 之前无论会话 prompt 在模型上下文窗口中实际还剩多少空间，都固定请求 20K token 的输出预算。本 PR 改为按请求动态计算：以固定上限为天花板，夹取到接收请求的模型的窗口剩余空间（窗口减去估算的 prompt 大小，再减去一小段用于补偿 tokenizer 估算误差的安全余量），并设下限 1，保证即使估算已经占满窗口，请求参数依然合法。在大窗口下预算仍然收敛为原来的 20K——这些场景行为完全不变。摘要截断守卫现在对比实际请求的预算而非固定上限（输出永远不会超过请求的预算，若仍对比固定上限，被夹取的请求上该守卫永远不可能触发），预算被夹取时还会记录一条 debug 日志。
为什么需要
提供商在生成任何内容之前，会校验 prompt token 数加上请求的输出预算是否能装进模型的上下文窗口。在小窗口部署（例如以降低的最大模型长度启动的本地 vLLM 服务）上，长会话可能只剩不到固定 20K 的空间。此时压缩请求会超出窗口，backend 在模型还没开始运行前就用 400 拒绝——结果压缩这个本应拯救会话的机制反而失败了。#7960 报告的场景：65,536 token 窗口、prompt 约 45,537 token，45,537 + 20,000 = 65,537 恰好超出窗口 1 个 token，每次压缩都失败。同样的未夹取上限还导致：当用户 prompt 把主请求推过窗口时，反应式救援压缩无法生效。既有的压缩模型守卫会基于默认窗口拒绝过大的压缩模型，但从不对真实窗口做夹取——本 PR 补上了这个缺口。
评审验证计划
如何验证
核心场景由一个模拟 backend 窗口预检的回归测试覆盖：65,536 token 窗口、约 45.5K token 的会话历史，修复前压缩以 issue 中完全一致的 "exceeds the model's context window" 400 失败，修复后成功，预算被夹取到约 19K，使 prompt 加预算装进窗口。值得确认的行为：大窗口（128K）下请求的预算仍是完整的 20K 上限，即典型部署行为不变；模型输出达到夹取后的预算时，截断守卫会丢弃可能被截断的摘要并报告专门的截断失败状态，而不是将其持久化；当保留了窗口更大的独立压缩模型时，预算按该模型的窗口定界，摘要上限不会退化；当估算输入已经超过窗口时，预算落到下限 1 保持请求合法——该区间下 backend 通常会因 prompt 本身超窗而直接拒绝。
证据（修复前后对比）
不适用——无用户可见 / TUI 变化。单测证据见上方英文部分的命令。
测试环境
环境（可选）
仅单元测试（
npm install、npx vitest、npm run typecheck、npm run lint）。未使用真实 backend，backend 预检由测试 mock 模拟。风险与范围
finish_reason）目前 side-query 管道尚未透传（既有 TODO 跟踪中）。关联 Issue
Fixes #7960
后续跟踪：#9115