Repository navigation
Conversation
|
Current head: Fresh current-main replacement for #9541. The compression admission changes were ported onto current model-routing code, including endpoint-pinned compaction model selection. @qwen-code /review |
chore: sync current upstream main into aaron/fix-compression-context-admission-v2
|
Current head: Current-main replacement for #9541 after syncing the latest upstream commit; preserves endpoint-pinned model routing and compression admission safeguards. @qwen-code /review |
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review at 490b993 by a maintainer's assistant. This PR appears to have no automated review (the precheck withheld triage), so this is the first review on the thread. Scope checked: ChatCompressionService.compress is the sole LLM-compression entry, so all five callers are covered; the guard has no queue/slot/shared counter, so no check-then-act or leak concerns apply; the rejection path returns newHistory: null without touching history and the breaker latches. Two findings:
| cache_sharing_used: usedCacheSharing, | ||
| }), | ||
| ); | ||
| const newTokenCountIsEstimated = |
There was a problem hiding this comment.
[Critical] The new provenance stamp ignores outputCountIsEstimated, so a locally-estimated output count can be reported as authoritative.
const newTokenCountIsEstimated =
usedEstimatedVisibleDelta ||
Boolean(opts.originalTokenCountIsEstimated) ||
restorationChars > 0;One of the two "provider" counts consumed by the branch above (lines 1535-1566) can be a local estimate: when the provider reports promptTokenCount but omits candidatesTokenCount/totalTokenCount, the code sets outputCountIsEstimated = true and compressionOutputTokenCount = estimateSummaryOutputTokens(...) (lines 1296-1300), and the branch only requires typeof compressionOutputTokenCount === 'number' && > 0. newTokenCountIsEstimated then omits that third estimated source, so with an authoritative baseline and no restoration attachments it computes false and stamps a partly-estimated newTokenCount as exact.
The merge base hardcoded newTokenCountIsEstimated: true on this return, so this is a regression rather than a pre-existing gap. Consequence: tryCompress stores the value as non-estimated (llm-chat.ts:2772-2775), the banner drops the ~ prefix, and the threshold/session-limit gates later trust a number that contains an estimate.
Fix: fold the flag in — usedEstimatedVisibleDelta || outputCountIsEstimated || Boolean(opts.originalTokenCountIsEstimated) || restorationChars > 0; — and add a test with usage: { promptTokenCount: N } only, asserting newTokenCountIsEstimated === true.
| }).length / CHARS_PER_TOKEN, | ||
| ); | ||
| sharedCurrentRouteTokenEstimate = | ||
| estimateContentTokens( |
There was a problem hiding this comment.
[Suggestion] The cache-sharing admission estimate uses the raw char/4 estimator, not the UTF-8-adjusted one the cold path uses.
sharedCurrentRouteTokenEstimate is built from estimateContentTokens(...), while the cold path's sibling term is estimateUtf8AdjustedContentTokens(...) at line 663 (and the pre-hook shared estimate at line 639 has the same raw shape). This is the only local term protecting the unslimmed, largest request when the provider anchor understates the current route — the scenario the new "stale provider anchor" test targets — yet for non-ASCII content it is roughly 5× below the calibrated estimate (0.25 tok/char vs 1.25). That is the multilingual-bypass class this PR set out to fix, and the only test for this term uses ASCII.
Failure is bounded (the shared request 400s, is caught, and the slimmed cold path runs), hence a Suggestion: use estimateUtf8AdjustedContentTokens(sideQueryHistory, slimmingConfig.imageTokenEstimate) for the current-route term at both 639 and 1095.
What this PR does
Rebuilds the compression-admission fix on the current
mainwithout rebasing or force-pushing the old PR branch.The change applies complete request admission to shared-cache and cold compression requests. It combines provider anchors with current-route estimates, includes thought signatures, runs bounded microcompaction before cold requests, and rejects requests locally when they cannot leave a usable output budget.
It also preserves current-main model routing, including endpoint-pinned compaction model selection, while retaining the admission safeguards from #9541.
Why it's needed
Issue #9455 captured compression requests whose estimated input plus reserved output exceeded the receiving model window. Those requests were knowingly sent to the provider and failed with context-window errors instead of being reduced or rejected locally. Additional review found that multilingual inputs, side-query failures, and mixed exact/estimated accounting could still bypass or corrupt that safety boundary.
Reviewer Test Plan
Verify the focused core suites around chat compression, compaction slimming, microcompaction, turn handling, and llm-chat, plus the normal CI build/lint/typecheck gates.
The regression matrix covers same-model cache admission, route overhead, thought signatures, bounded microcompaction, multilingual dense text, compaction-model budget boundaries, side-query exceptions, truncation telemetry, estimated token deltas, and failure rendering.
Risk & Scope
Linked Issues
Fixes #9455.
Supersedes #9541, whose branch no longer merged cleanly with current
main.中文说明
本 PR 在最新
main上重新构建压缩准入修复,不对旧分支 rebase 或 force-push。保留 #9541 的压缩准入保护,同时保留当前 main 已新增的 endpoint-pinned 压缩模型路由。关联并修复 #9455;由于 #9541 已与当前 main 产生真实合并冲突,本 PR 作为其干净替代。