Repository navigation
[PR] fix(block-ops): acp_rewrite respects compress's summary min/max bounds and accepts numeric blockId | fix(block-ops): acp_rewrite 遵守 compress 的摘要上下限，并接受数字形式的 blockId - #6315
Open
BBleae wants to merge 2 commits into
Conversation
…ccept a numeric blockId acp_rewrite only checked "non-empty" and "not identical", so it stored summaries compress refuses for the same block: over compress.maxSummaryLength, under compress.minSummaryLength, or larger than the content the block folded. The stored summary rides in the resident view on every request, so a block could end up costing more than the messages it replaced. executeAcpRewrite now judges each rewrite before the kernel call: - compress.minSummaryLength / maxSummaryLength of the resolved config (0 or absent = unbounded, as in compress), with compress's wording; - net shrink: a summary not smaller than the folded content (compressedTokens, defaultCountTokens) is refused, unless it shrinks the summary it replaces; - unknown/inactive ids and blank or identical text keep the kernel's existing reasons; in a batch the other items still land. blockId also accepts a non-negative integer (single form and items), the way acp_retire / acp_restore already do; before, a number was read as a missing argument. Host-side only; kernel/** is untouched. Docs line for compress.blockOps updated in both languages.
ranxianglei
changed the title
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
There are unresolved review findings—including a moderate correctness issue where blockIdArg() returns an empty string for whitespace-only input (leading to inconsistent and harder-to-reason-about id handling), plus documentation mismatches about acp_rewrite’s intended exception behavior.
3 open issues
What changed in this PR
Tightens the host-side acp_rewrite block-op so rewritten summaries are constrained by the same bounds and net-shrink expectations as compress, and extends acp_rewrite to accept numeric blockId inputs (matching other block-ops), with updated docs and regression tests.
Changes:
- Enforce
compress.minSummaryLength/compress.maxSummaryLengthand a “no net shrink” gate foracp_rewriteprior to the kernel mutation while preserving kernel-owned rejection reasons. - Accept non-negative integer numeric
blockIdvalues in both single and batch rewrite forms and thread resolved config into the rewrite executor. - Add tests for rejection reasons + partial success behavior in batches, and update EN/ZH configuration docs to describe the new rejection conditions.
| File | Description |
|---|---|
| tests/issue5765-acp-block-ops.test.ts | Adds regression tests covering acp_rewrite refusals for too-long/too-short/no-net-shrink summaries, batch partial success reporting, and numeric blockId handling (including rejecting non-integer/negative values). |
| src/loop/core.ts | Updates the proxy tool execution path to pass the resolved ctx.config into executeAcpRewrite so rewrite validation uses the effective configuration. |
| src/acp-block-ops.ts | Implements host-side acp_rewrite validation aligned with compress (min/max summary length and net-shrink checks) and expands blockId parsing to accept numeric IDs in single and batch forms. |
| docs/configuration.zh-CN.md | Updates the Chinese configuration documentation for compress.blockOps to describe additional acp_rewrite rejection conditions and related behavior. |
| docs/configuration.md | Updates the English configuration documentation for compress.blockOps to describe additional acp_rewrite rejection conditions and related behavior. |
Review effort: Balanced
Comment on lines
+86
to
+90
| function blockIdArg(v: unknown): string | undefined { | ||
| if (typeof v === "string") return v.trim(); | ||
| if (typeof v === "number" && Number.isInteger(v) && v >= 0) return String(v); | ||
| return undefined; | ||
| } |
| - `acp_retire` — move active compressed blocks out of the resident view (their summaries no longer ride on every request; `acp_restore` / `decompress` / `search_context` still reach them by id). | ||
| - `acp_restore` — put retired blocks back into the resident view. | ||
| - `acp_rewrite` — replace a block's summary in place (one-step rollback slot `previousSummary` kept; identical/blank rewrites rejected). | ||
| - `acp_rewrite` — replace a block's summary in place (one-step rollback slot `previousSummary` kept; identical/blank rewrites rejected, and so are the summaries `compress` would refuse for the block: outside `compress.minSummaryLength` / `compress.maxSummaryLength`, or not smaller than the content the block folded). |
| - `acp_retire` — 把 active 压缩块移出常驻视图（其摘要不再随每个请求发送；仍可按 id 用 `acp_restore` / `decompress` / `search_context` 取回）。 | ||
| - `acp_restore` — 把已退场块放回常驻视图。 | ||
| - `acp_rewrite` — 原地替换块摘要（保留一步回滚位 `previousSummary`；相同/空白摘要拒收）。 | ||
| - `acp_rewrite` — 原地替换块摘要（保留一步回滚位 `previousSummary`；相同/空白摘要拒收；`compress` 对该块会拒收的摘要同样拒收：长度超出 `compress.minSummaryLength` / `compress.maxSummaryLength`，或不比该块折叠的内容小）。 |
BBleae
changed the title
ranxianglei
changed the title
Owner
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) 进度同步（进程曾被强制重启一次，现在继续）：
正在做的最后一步：本地定向跑 |
…vement' exception (ranxianglei#6319) The net-shrink refusal in executeAcpRewrite has an exception: a rewrite that shrinks the summary it replaces is always allowed, even when the stored summary was already larger than the folded content. The PR's two new tests cover the refusals (too long / no net shrink / too short) and the numeric blockId, but not this exception branch. Add a case that pins it so a future tightening of the check cannot silently start refusing bloat cleanup.
Owner
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) 结论先行：可合。 方向对、实现正确、测试有牙；我顺手补了一个 PR 漏掉的测试（已 push 到分支，见下）。作者抛出的两个决策点附推荐，均不阻塞合并。 查重：不重复。本 PR 正是 #6319 的修复（症状/机制一一对应）；#6311 是另一处 blockOps 缺陷（改写后常驻视图仍带旧摘要），症状与机制都不同，不在本 PR 范围；无其他在途 PR 触碰 我不止读 diff，还本地验证了（PR head worktree，
八问
代码质量（⑤b）：命名达意、注释讲"为什么"、错误信息带上下文（字符数/上限）、无死代码、风格一致、未引新依赖。唯一小点： 我补的测试（已 push，commit 两个作者已抛出的决策点（附推荐，均不阻塞合并）
合并判定：方向对、实现正确、测试有牙（本地已验证 fail-on-master / pass-on-fix）、CI 全绿（head |
Owner
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) 补充：我补测试后的新 head 结论不变：可合。合并前待办仍是那三条——rebase 到最新 master、打 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6319
这是 #5765 / PR #5774（压缩块生命周期工具，
compress.blockOps，默认关闭）的跟进。只改宿主侧，没有动kernel/**。问题
acp_rewrite替换的是块里存的摘要，而这份摘要每个请求都会随常驻视图发出去。compress对写进这里的摘要有把关；acp_rewrite只查「非空」和「与原来不同」，所以同一个块上compress会拒收的摘要，acp_rewrite照样存进去。在 master
c26c31ace上，默认配置（compress.maxSummaryLength20000，minSummaryLength50），折叠一段约 6000 字符的范围：compressacp_rewriteSummary too long）b1 summary replaced (… → 25000 chars)no net shrink）Summary too short）工具说明写的是「replace … with shorter text you supply」，但没有任何检查让调用遵守这一点。一个块被改写成 25000 字符后，之后每个请求的开销都比它折叠掉的那些消息还大。
另一处：
acp_retire/acp_restore通过collectBlockIds接受数字形式的blockId: 5，acp_rewrite却只认字符串：{ blockId: 1, summary }回的是provide { blockId, summary } or items: …，就像什么都没传；items: [{ blockId: 1, summary }]也一样。改法
src/acp-block-ops.ts，在调内核之前：compress.minSummaryLength/compress.maxSummaryLength约束，含义和compress里一样（0 = 不限制），回执措辞也一样（Summary too short (N chars, min M)/Summary too long (N chars, max M)）。过短的回执指向acp_retire——把块移出常驻视图本来就该用它。compressedTokens，和src/stream.ts里gate:no-net-shrink的检查一样用内核的defaultCountTokens计数）就拒收。留一个例外，保证这道检查永远不会挡住改进：只要比它替换掉的那份摘要小，改写就一律放行，即使存着的摘要本来就比折叠内容大。blockId在单条形式和items里都接受非负整数，范围和collectBlockIds一致。工具 schema 仍写string；这里只是不再把数字当成「没传」。executeProxyTool把ctx.config传给executeAcpRewrite。文档：
docs/configuration.md和docs/configuration.zh-CN.md里compress.blockOps下acp_rewrite那一行，现在列出了这些拒收情形。对现有调用方有变化的一点：默认配置下，不到 50 字符的改写现在会被拒收。如果你更希望短改写继续合法、只保留上限和净收缩，说一声我就把下限去掉，就是一个
if。不在这个 PR 里：回滚位
每次改写都会写
previousSummary，回执、工具说明、prompt 段和文档也都说上一份摘要「kept for rollback」。但没有任何地方读这个字段：没有工具或参数能把它恢复回来，第二次改写还会把这个位覆盖掉。我不想在没人提的情况下加新动作。你倾向哪种？acp_rewrite({ blockId, rollback: true })对调summary和previousSummary；或者decompress取回，回执里已经这么写了）。哪种都可以作为后续 PR 补上。
测试
tests/issue5765-acp-block-ops.test.ts新增两个用例，在 master 上都失败、改动后都通过：maxSummaryLength20000 /minSummaryLength50，块折叠了约 6000 字符：25000 字符、9000 字符、1 字符各自按对应理由被拒，存着的摘要和previousSummary都不变；批量时被拒的那项写进回执，同批里合法的那项照常生效；blockId在单条形式和items里都能用；1.5和-1仍不算 id。原有的 #5765 用例没改，全部通过。
在 Windows、Node 22 上离线跑（出站网络被挡），
tests/*.test.ts用node --test跑，带的三个--import和npm test一样：tests/issue5765-acp-block-ops.test.ts：改动前 8 过 / 2 败（就是新增的两个），改动后 10 / 10。--test-concurrency=4）：11007 项（master 上 11005 项 + 新增 2 项），10922 过、9 败、9 取消、67 跳过。9 个取消（external-summary-autofold7 个、fork-nudge-deadzone2 个）和其中一个失败（preflight-target-stability）是机器负载高造成的：这几个文件单独跑都过。其余 8 个失败不经过这次改的代码，而且在这台机器上，未改动的 masterc26c31ace也是同样的失败：graceful-close、issue3776-tls-mitm-diagnostics（B）、responses-websocket（TLS 那一段）：本机杀毒软件会重签回环口的 TLS；main-path-transport-retry（主模型请求路径缺少 fail-fast 传输重试：一次 DNS 抖动即打断整轮（acp-loop/preflight 已有 #1263 重试，主路径不对称） #1688 DNS）、upstream-proxy-routing（PR feat: upstream proxy support (firewall / GFW) #67）、mitm（setupMitm e2e）：需要真实 DNS / 非回环网卡，被离线防护挡掉了；log-retention（"out-of-order stamps"）：依赖本地时区；本机不在 UTC 时失败，TZ=UTC下通过；issue3544-stale-proxy-rotation：默认端口上已有另一个实例在监听。tsc --noEmit干净。kernel/下没有文件改动。