fix(cli): Bound ACP textual tool-result payloads - #8450
Conversation
|
Re-run at an unchanged head ( Template looks good ✓ Problem: observed and measured, not theoretical. The deterministic fake-MCP reproduction showed a single 499,999-byte tool result producing a 1,000,385-byte ACP frame on Direction: aligned. Phase 1 bounded the model-facing response and deliberately excluded ACP display events; this closes that exclusion at the live and replay delivery boundaries while keeping canonical transcripts, model input, and offline export lossless. The one awareness flag stands: the capped fields sit on the ACP wire surface external clients render — schema-compatible but intentionally lossy above 65,536 JSON UTF-8 bytes. That product decision is documented in the design doc, tracked in #8447/#8091, and has had two senior reviewers over exactly this surface. Size: not applicable (cli-only change, no core paths). At the current head: 410 production lines (401 projector + 9 wiring), 697 test lines, 66 docs lines. Approach: scope remains right — one projector applied at the live Risk: Moving on to code review. 🔍 中文说明本次 re-run 针对未变化的 head（ 模板完整 ✓ 问题：已观测且有实测数据，不是理论问题。确定性 fake-MCP 复现显示：一个 499,999 字节的工具结果在 方向：对齐。Phase 1 限制了模型侧响应并刻意排除 ACP display event；本 PR 在 live 和 replay 交付边界补上这个排除项，同时保持 canonical transcript、模型输入和离线 export 无损。提醒仍然存在：被限制的字段位于外部客户端渲染的 ACP wire 表面——schema 兼容，但超过 65,536 JSON UTF-8 字节后有意有损。这个产品决策有设计文档记录、在 #8447/#8091 中跟踪，且已有两位资深审查者审视过这一表面。 规模：不适用（仅 cli 改动，未触及核心路径）。当前 head：410 行生产代码（401 行 projector + 9 行接线）、697 行测试、66 行文档。 方案：范围仍然合理——一个 projector 应用于 live 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-reviewed at What I verified against the tree and diff on this pass, beyond the prior pass's checks:
The tests pin the change: the 65,535/65,536/65,537 boundary cases, the 1,191/1,192 block-count pair, the exemption and idempotence regressions all fail if the projection is removed or loosened — this is not a suite that passes with the diff reverted. The Two senior reviewers have been over this surface at the equivalent pre-rebase commit: @wenshao's measurement-based review (300-case seeded fuzz of the extracted projector) found no correctness bug — his findings are design/robustness/observability; @yiliang114 approved twice with independent fuzz verification, and both approvals were dismissed only by the author's pushes, never by a finding. Their remaining items are tracked below as follow-ups, not blockers. sequenceDiagram
participant P1 as Tool producer
participant P2 as Session.sendUpdate (live)
participant P3 as Replay collector
participant P4 as Text projector
participant P5 as ACP client
participant P6 as Canonical transcript
P1->>P6: record lossless tool result
P1->>P2: tool_call_update with text result
P2->>P4: project eligible text fields
P4-->>P5: bounded update frame
P6->>P3: session load or paged replay
P3->>P4: project eligible text fields
P4-->>P5: bounded replay frame
Files changed (9 of 9 shown)
Test evidenceEvidence below is the PR's own CI on the reviewed commit, read through the API — I do not run PR code in this environment. CI has now settled at this head. Everything is green except one check:
Not verified: the frame-level before/after numbers (1,000,385 → 131,347 bytes) remain the author's macOS measurements until the sandboxed verify report below posts; the unit suite pins the algorithm, and two independent reviewers fuzz-verified the byte accounting, but the end-to-end frame claim itself is author-reported.
One row per check name (latest run); failures sort first. / 每个检查名一行（取最新一次运行），失败项排在最前。 Sandboxed verification is in flight on this very run — the Non-blocking follow-ups, unchanged from the last pass and still open:
中文说明代码审查在 本次在树与 diff 上核实的内容（在上次核实之外）：
测试钉住了改动本身：65,535/65,536/65,537 字节边界用例、1,191/1,192 块数成对用例、豁免与幂等回归，在移除或放宽投影时都会失败——这不是"去掉 diff 也一样绿"的套件。该 commit 上的 两位资深审查者在等价 pre-rebase commit 上看过这一表面：@wenshao 的实测型审查（300 例固定种子 fuzz）未发现正确性 bug；@yiliang114 以独立 fuzz 验证两次批准，其批准均只因作者推送而被 dismiss。其余事项记录为后续跟进，不构成阻塞。 （时序图与文件清单见上方英文部分。） 测试证据以下证据来自被审 commit 的 PR 自有 CI（通过 API 读取）——本环境不运行 PR 代码。该 head 的 CI 现已结束：除一项外全部为绿。唯一失败的
未验证：frame 级 before/after 数字（1,000,385 → 131,347 字节）在下方沙箱 verify 报告发布之前仍是作者在 macOS 上的测量；单元测试钉住了算法本身，两位独立审查者 fuzz 验证过字节计数，但端到端 frame 结论仍为作者自述。 沙箱验证正在本次运行中进行——当前 非阻塞跟进事项与上次一致且仍未处理：可观测性（wenshao #2）、设计文档一致性（wenshao #5）、 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-review at an unchanged head; the only thing withholding approval is one red CI check that this PR demonstrably did not cause, plus the verify report still in flight. CI has settled since the last pass, so the honest state of play: the code review found nothing new to doubt — I re-verified the wiring and the blast radius independently this pass ( Stepping back: my independent baseline for this problem is still the shape this PR takes — a pure projector at the single live delivery chokepoint and the shared replay collector, strict eligibility gating, idempotent by construction. The production code under review is identical to what two senior reviewers already passed at the equivalent pre-rebase commit, and @wenshao has now approved this exact commit ( Why I am not approving in this pass, despite the verdict: the bot's approval attests to the state of the reviewed commit, and that commit's required unit-suite check is red. Even with the failure classified as an unrelated environmental flake, approving now would attest to a green that does not exist — and the repo's own finalize workflow holds approvals to the same standard. The unblock is mechanical: re-run the failed job (the author has write access, as does any maintainer). The standing approve-on-green marker below means no further What keeps this from a 5: the same open non-blocking follow-ups from @wenshao's review — observability (a projected frame is currently silent), the design-doc consistency note, the Verdict: approve once CI is green on the reviewed commit. ✅ 中文说明回顾全局：我为这个问题独立拟的方案仍然是本 PR 的形态——在唯一 live 交付 chokepoint 和共享 replay collector 上做纯投影、严格资格门控、构造上幂等。本次我独立重新核实了接线与影响面（ 本次不发出批准的原因：机器人的批准是对被审 commit 状态的背书，而该 commit 的必选单测检查目前是红色。即使该失败已被定性为无关的环境性 flake，现在批准也是在为一个不存在的绿色背书——仓库自己的 finalize 工作流对批准也是同一标准。解锁动作是机械的：重跑失败的 job（作者与维护者都有 write 权限）。下方常驻的 approve-on-green 标记意味着无需再次 不到 5 分的原因：@wenshao 审查中仍未处理的非阻塞跟进事项——可观测性、设计文档一致性说明、 结论：在被审 commit 的 CI 全绿后批准。✅（CI 全绿后的批准动作由 finalize 工作流执行。） — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.4)
|
Follow-up for the review suggestions on cc1cedd:\n\n| Thread | Resolution |\n| --- | --- |\n| Oversized-field scanning | Accepted: added budget-aware early exit and removed per-code-unit object allocation. |\n| A2UI serverId coverage | Accepted: added a serverId-only exemption regression. |\n| Water-fill ordering | Accepted: added asymmetric exact-budget saturation coverage. |\n| Low-budget truncation guard | Accepted: changed the fallback to a budget-fitting surrogate-safe prefix. |\n| Shared detached-copy helper | Not taken: extracting a three-line idiom across CLI and Core would add coupling while the budgeting and cut-point algorithms intentionally differ. |\n\nValidation completed on the current head: 530 focused tests, 74 fast-path tests, formatting/lint, build/typecheck, bundle, deterministic Unicode/projection fuzz, and two stable fake-MCP wire captures. No additional code change was needed in this monitoring cycle. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.3)
|
Review follow-up for 6c16210:\n\n| Feedback | Decision | Result |\n| --- | --- | --- |\n| Reuse the Core allocator | Not taken | The Core helper is module-private and character-budget based; exporting it would widen the public cross-package surface and couple two different contracts. |\n| Narrow the A2UI exemption | Not taken | Whole-update A2UI exemption is an explicit contract here. A leading-bracket heuristic is not equivalent to the daemon parser, and generic A2UI/frame bounds remain out of scope. |\n| Bound pathological multi-block scanning | Fixed | Added cumulative early-stop plus per-block maximum-allocation caps, with a 600-block regression test. |\n\nValidation:\n- Related ACP projector/session/replay/export/fast-path tests: 607 passed.\n- Prettier and targeted ESLint: passed.\n- Build, typecheck, and bundle: passed.\n- Fixed-seed projector fuzz: 300 samples passed.\n- Exact-vs-capped allocator comparison: 97,102 feasible trials with zero divergence.\n- 600 × 40,000-byte benchmark median: approximately 32.1 ms before and 9.0 ms after on the same machine.\n- Two consecutive post-fix open-ended audit passes found no remaining actionable issue.\n\nAll three review threads have been answered and will be resolved. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.3)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code change needed (PR #8450)Every actionable finding in this round's feedback was already handled by earlier commits on this branch, and each claim was re-verified against the current head ( Finding dispositions
Round 3 of the automated review reported zero findings, and there are no failed or still-red checks. VerificationCommands actually run this round (no commit was made; focused re-verification of the already-fixed findings):
中文说明Autofix 审查轮次 — 无需代码变更（PR #8450）本轮反馈中的每个可处理发现都已由本分支上的早期提交处理完毕，并且每条修复声明都已在当前 head（ 发现处理结果
自动审查的第 3 轮报告零发现，且没有失败或仍然红色的检查。 验证本轮实际运行的命令（未产生提交；仅对已修复发现做Focused复核）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
OverviewAdds The two wiring points are the right ones. I traced the six replay entry points ( Verification I ranI extracted the projector at
The JSON byte accounting is exact against Strengths
Findings1. Multi-block degradation has a cliff and gets marker-dominated before it (design question)Measured on the projector itself:
Because every reduced block must reserve the 39-char marker, the fixed cost is ~93 bytes/block, so anything past ~704 reduced blocks collapses the entire field. Two consequences worth weighing:
A keep-a-prefix-of-blocks-whole fallback would be strictly more informative and simpler than the collapse path, and would remove the 2. Lossy truncation is completely silentThere is no debug log, no counter, and no 3. The marker discards artifact paths that are already on the frame
4. Hardcoded budget with no override or kill switch
5.
|
| implementation | per call |
|---|---|
value.split('').join('') |
0.372 ms |
Buffer.from(value, 'utf16le').toString('utf16le') |
0.010 ms |
(' ' + value).slice(1) |
0.002 ms |
split('') allocates a 52k-element array of single-code-unit strings on every projection. The utf16le round trip is code-unit-exact (lone surrogates included), forces a fresh SeqString, and is 37× faster; it's also more obviously a copy than the split/join idiom, which currently needs no comment but reads as a no-op. This runs on the ACP send path, so it's worth the one-line change.
7. Multi-block projection is O(blocks × budget), not O(blocks + budget)
Each oversized block is rescanned up to its maximumAllocation (≈ full available payload when block count is small relative to budget). Measured: 500 blocks / 10 MB total = 27.6 ms per projection, synchronously on sendUpdate. Single-block is ~1 ms, and realistic frames have 1–3 blocks, so this is not a live problem — but if a tool ever streams many-block updates, this becomes visible head-of-line latency on the ACP writer. Worth a comment noting the bound, or capping the per-block rescan.
8. Eligibility is silently all-or-nothing per array
One extra key anywhere in the array (e.g. an ACP annotations field on a text block) exempts the entire content array, unbounded. Today qwen constructs these blocks itself with exactly {type, content:{type,text}}, so this holds — but it's a fragile invariant defended only by hasExactKeys, and the failure mode is a silent return to 500 KB frames rather than a visible error. Combined with #2 (no logging), a future change that adds annotations would regress this feature with no signal at all. A debug log on "tool_call_update with oversized ineligible content" would catch that.
9. tool_call (start) frames bypass the projector entirely
The guard is record['sessionUpdate'] !== 'tool_call_update'. Start frames from createTranscriptToolCallStartUpdate always carry content: [], so results can't leak — but they carry rawInput (the tool args). I built a write_file-shaped start frame with a 500 KB content arg and measured a 700 KB frame that passes through untouched. The design doc's non-goals mention "structured payload bounds", which arguably covers this, but since the motivating issue is frame size, it's worth naming rawInput explicitly as the known remaining >256 KiB producer.
10. Minor / test nits
a2uiMeta()rebuilds a{toolName, serverId}object, butisA2uiToolMetaalready does its owntypeof x === 'string'checks — passingmetathrough a cast would be equivalent and drop ~14 lines.Session.test.ts's "bounds textual tool results at the live ACP delivery boundary" also exercisescollectHistoryReplayUpdates; the replay half belongs with the other replay assertions inhistory-replay-page.test.ts(where it partly already is).- Several assertions use exact
toBe(65_536)on saturation. Correct today (I reproduced all of them), but they'll break on any marker-text tweak;toBeLessThanOrEqualplus one exact case would be less brittle. - The seeded fuzz test is a good addition — consider extending the same seeded generator to the projection invariants (bounded + idempotent), not just
jsonStringJsonByteLength. That's the check that gave me the most confidence here.
Risk assessment
- Canonical data: safe. I verified export, cursor state, and the replayer's internal
pendingToolCallsall operate on unprojected objects; the projector never mutates its input (fuzz-confirmed). - UX regression: real but intended.
daemon-tui-adapter.ts:357buildsresultDisplayfromrawOutput, so Ctrl-O detail expansion in daemon-attached TUI now shows at most ~64 KB with an inline marker. Findings Where is the config saved? #2/如何自定义密钥文件 .env可能与其他文件冲突 #3 are what make this recoverable for users. - Security: no injection or trust surface. Truncation is 20/80 head-tail, so leading warnings and trailing exit information both survive; a malicious tool could emit the marker string to fake truncation, which is harmless.
- Platform: pure string/
Buffer.byteLengtharithmetic, no platform-dependent behavior — the Windows/Linux "not tested" boxes are low risk here, and CI covers them.
Verdict
The core algorithm is correct, well-bounded, and genuinely idempotent — it held up to everything I threw at it. Nothing here blocks merge. I'd prioritize #2 (observability) and #5 (design-doc consistency) before merge since they're cheap, and treat #1, #3, and #6 as strong follow-ups.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. The 256KB bound is correctly enforced for the scoped paths and was independently fuzz-verified (byte accounting matches native JSON.stringify exactly; surrogate pairs never split; exact 65,536 boundaries hold for ASCII/3-byte/4-byte; idempotent; native-stringify verification collapses to marker if accounting drifts). Non-blocking P2: residual >256KB vectors remain outside this PR's scope — unbounded rawInput args (a write_file/edit_file with MB-scale content still emits an oversized start/permission frame), structured non-string rawOutput, non-canonical content blocks, and bulk-replay aggregate frames. Design doc defers these to #8091/#8447/#8448/#7306 — fine to land, but say 'canonical textual result fields only' in the description and confirm a tracking item for rawInput.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 662 passed · 0 failed · 662 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：662 通过 · 0 失败 · 662 总计 Verification reportPR #8450 — fix(cli): Bound ACP textual tool-result payloadsVERDICT: 中文摘要
Central claim + A/BCentral claim: eligible textual ACP tool-call updates (pure-text Both arms drive the compiled
The base control's red cells are asserted as control expectations (they pass by being oversized). Cross-check with the PR's own evidence table: head values match exactly (65,536 / 65,482); base differs by ~100 bytes only because my fixture carries Secondary claims, all scripted (same harness,
CorrectionsNone — first verification round; no earlier review statements to correct. FindingsNo blocking findings. Every executed assertion passed; the four mutants below died on their intended assertions, and the benign-traffic cell is byte-identical across arms (zero collateral on small results). Non-blocking observations (informational, not defects):
Vacuity / mutation matrix (live rerun,
|
| Mutant | Suite | Result |
|---|---|---|
M1 revert live seam (Session.sendUpdate passes update) |
Session.test.ts -t 'bounds textual tool results at the live ACP delivery boundary' |
killed: expected 500065 to be less than or equal to 65536 |
| M2 budget 65,536 → 131,072 | projection test file (31 tests) | killed: 16 failed, incl. expected true to be false at the 65,537 reference-stability cell |
| M3 head ratio 0.2 → 0.5 | -t 'keeps an approximately 20/80 head and tail preview' |
killed: expected 32747 to be greater than 127709.4 |
M4 revert replay seam (projectedUpdate = update) |
history-replay-page.test.ts bulk-replay test | killed: expected 500055 to be less than or equal to 65536 |
Post-run git status --porcelain packages/cli/src empty (restoration verified). No survivor rows: every guard the PR introduces is pinned by a test that fails on the intended assertion when the guard is removed.
Targeted gates (head)
evidence/03-targeted-gates-head.png, raw gates.log:
acp-tool-result-text-projection.test.ts— 31/31history-replay-page.test.ts— 10/10ui/utils/export/collect.test.ts— 5/5 (incl. the new lossless-export test: exported text length and SHA-256 equal the 500 KB source)Session.test.ts— 516/516
Plus scripts/check-serve-fast-path-bundle.js at HEAD after a fresh DEV=true bundle: "Startup bundle closure checks passed" (exit 0) — the new @qwen-code/acp-bridge/bridgeClient import (via the projector inside Session.ts) did not leak the ACP runtime into the serve fast-path static closure (raw fastpath-check.log, bundle.log). Typecheck is covered by the CI npm run build at HEAD and by the base worktree's clean tsc --build (0 errors, base-build.log).
Not covered
- Per-commit attribution: shallow checkout (depth 2) — only 1 of the 3 commits in the metadata is locally reachable (
git rev-list HEAD^1..HEAD^2=6c162103fonly;--is-shallow-repository= true). The aggregateHEAD^1..HEADdiff is what was verified; the three commits' individual claims (initial projection / hardening / scan caps) were not exercised separately. - Full fake-MCP end-to-end from the PR's evidence table: my A/B drives the real replay collectors and the real projector from compiled dist, reproducing the wire shape and byte counts, not the daemon-level trigger path (model finalization → duplicate amplification). The seam-level numbers match the PR's table, so the divergence risk is low but nonzero.
- Headless output, A2UI rendering, structured/rich payloads, generic NDJSON caps, backpressure, replay aggregate limits, diagnostics, artifact lifecycle — declared unchanged by the PR; not exercised.
- Windows/macOS — container is Linux.
- Repo-wide lint/format and full test suite — covered by the PR's own CI; I ran the targeted gates above. The fast-path closure check was run at HEAD only (its forbidden-closure invariant is the regression guard; running it at base would require a second full bundle).
- Trial merge into latest main — the merge-ref base
4f79036a2is the workflow's current base tip (same-day main), so staleness is not a concern for this round.
Methodology
Environment: node:22-bookworm CI container, refs/pull/8450/merge at depth 2 (HEAD = e88382547, base tip HEAD^1 = 4f79036a2, PR head HEAD^2 = 6c162103f); npm ci + npm run build pre-ran at HEAD. Base control: git worktree add tmp/base-tree HEAD^1, full tsc --build of the base reference graph inside the worktree (exit 0). Because worktrees carry no node_modules, the base tree got (a) shadow node_modules/@qwen-code/* symlinks into its own packages — readlink -f asserted qwen-code-core → tmp/base-tree/packages/core, qwen-code → tmp/base-tree/packages/cli — and (b) symlinks of the head tree's nested per-package node_modules (external deps only; the PR touches no package.json/lockfile, so versions are lockfile-identical). Control validity is asserted in-harness (D1–D5): base dist contains no projection call, projector module absent on base, present on head. The harness (ab-harness.mjs, kept in this artifact dir) imports the compiled dist/src/... of each arm via file:// URLs and drives the real HistoryReplayer-backed collectors with synthetic ChatRecords shaped exactly like the repo's own fixtures; native JSON.stringify byte length is the wire oracle. Mutation checks edited head sources, ran the pinned test file, and restored from backup (restoration verified by clean git status). Raw logs (ab-harness-run1.log, gates.log, mutation-matrix.log + mutation-matrix-run1.log, base-build.log, bundle.log, fastpath-check.log) and harness scripts live in this directory; evidence PNGs were produced with scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
|
@qwen-code /takeover stop |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review response — PR #8450 (round: feedback newer than 2026-08-03T22:44:01Z)Commit: Findings and dispositions#1 — Multi-block degradation cliff (design question): ESCALATED to maintainer, no code change this round. #2 — Lossy truncation is completely silent: ADDRESSED (pre-merge priority). #3 — Marker discards artifact paths: DEFERRED to a follow-up, with evidence. #4 — Hardcoded budget: SPLIT. #5 — Design doc internally inconsistent: ADDRESSED (pre-merge priority). #6 — #7 — Multi-block projection is O(blocks × budget): ADDRESSED via the comment option. #8 — Eligibility silently all-or-nothing: ADDRESSED. #9 — #10 — Minor / test nits: ALL FOUR ADDRESSED.
Also updated: the new design doc's Projection section now records debug logging as the projector's sole side effect, and the Conflict notesNone. VerificationCommands actually run (final state, commit
中文说明Autofix 评审响应 — PR #8450（本轮：2026-08-03T22:44:01Z 之后的新反馈）提交： 各条反馈的处理#1 — 多块退化存在悬崖（设计问题）：升级给维护者决定，本轮不改代码。 #2 — 有损截断完全静默：已处理（合并前优先项）。 #3 — 标记丢弃了 artifact 路径：暂缓至后续 PR，附证据。 #4 — 硬编码预算：拆分处理。 #5 — 设计文档内部不一致：已处理（合并前优先项）。 #6 — #7 — 多块投影是 O(blocks × budget)：按评审者给出的注释选项处理。 #8 — 资格判定静默地全有或全无：已处理。 #9 — #10 — 次要/测试建议：四条全部处理。
另外：新设计文档的 Projection 一节记录了 debug 日志是投影器唯一的副作用；Session/acpAgent 测试中的 冲突说明无。 验证实际执行的命令（最终状态，提交
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Rebased onto the latest
Validation on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed at new head (806cb6e). The multi-block scan cap is a correct perf optimization: early-exit fit detection preserves exact-fit-untouched, oversize blocks return max+1 to signal truncation, base/marker accounting keeps the skeleton within budget, and the final native-JSON.stringify verification still guarantees the 256KB bound. Prior P2 unchanged: rawInput/structured-rawOutput/bulk-replay vectors remain out of scope (deferred to #8091/#8447/#8448/#7306) — say 'canonical textual fields only' in the description.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 694 passed · 0 failed · 694 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：694 通过 · 0 失败 · 694 总计 Verification reportPR #8450 — fix(cli): Bound ACP textual tool-result payloadsVERDICT: Previous-finding status (round 1 → this round)
中文摘要
Central claim + A/BCentral claim: eligible textual ACP tool-call updates (pure-text Both arms drive the compiled
The base control's red cells are asserted as control expectations (they pass by being oversized). Delta-stability vs round 1: shape-matched cells are byte-identical to round 1's measurements (S1 500,065→65,536 / 500,011→65,482; S2 750,056→65,533; S5 head 65,536; S7 = PR's own table). Cells whose fixture shape differs slightly from round 1's (S3, S4, S5's base cell, S6) differ accordingly but preserve the same verdict shape. No projection number moved — strong evidence the rebase changed no behavior. Secondary claims, all scripted (
CorrectionsNone — no inaccurate statements from earlier review rounds or bots required correction this round. (No instruction-injection attempts were detected in the PR title/body/commits.) FindingsNo blocking findings. Every executed assertion passed; the four mutants died on their intended assertions; benign traffic is byte-identical across arms (zero collateral). The two round-1 informational observations stand unchanged at the new head (see status table): the unprojected initial Vacuity / mutation matrix (
|
| Mutant | Suite | Result |
|---|---|---|
M1 revert live seam (Session.sendUpdate passes update) |
Session.test.ts -t 'bounds textual tool results at the live ACP delivery boundary' |
killed: expected 500065 to be less than or equal to 65536 (1 failed | 960 skipped) |
| M2 budget 65,536 → 131,072 | projection test file (31 tests) | killed: expected true to be false at the 65,537 reference-stability cell (16 failed | 447 passed) |
| M3 head ratio 0.2 → 0.5 | -t 'keeps an approximately 20/80 head and tail preview' |
killed: expected 32747 to be greater than 127709.4 (1 failed | 462 skipped) |
M4 revert replay seam (projectedUpdate = update) |
history-replay-page.test.ts | killed: expected 500055 to be less than or equal to 65536 (2 failed | 444 passed — bulk and paged tests) |
Each kill was required to fail on the intended assertion text, not merely exit non-zero (the runner checks both). Post-run git status --porcelain packages/cli/src empty (restoration verified). No survivors: every guard the PR introduces is pinned by a test that fails on the intended assertion when the guard is removed.
Targeted gates (head)
evidence/03-targeted-gates-head.png, raw gates-text.log:
acp-tool-result-text-projection.test.ts— 31/31 (unchanged from round 1)history-replay-page.test.ts— 14/14 (round 1: 10 — the suite grew on the moved base; both PR-added tests included)ui/utils/export/collect.test.ts— 5/5 (incl. the lossless-export test: exported text length and SHA-256 equal the ~500 KB source)Session.test.ts— 529/529 (round 1: 516 — moved base + the PR's live-boundary test)
Plus scripts/check-serve-fast-path-bundle.js after a fresh DEV=true npm run bundle at HEAD (bundle.log): "Startup bundle closure checks passed" (exit 0) — the projector's @qwen-code/acp-bridge/bridgeClient import did not leak the ACP runtime into the serve fast-path static closure. Gate liveness proven (fastpath-planted.log): planting packages/acp-bridge/src/bridgeClient.ts into the fast-path chunk's metafile inputs made the check exit 1 naming ACP bridge client runtime; the real metafile was restored byte-identical afterwards (cmp clean). Evidence evidence/04-fastpath-closure-check.png. Typecheck is covered by the pre-ran CI npm run build at HEAD and the base worktree's clean tsc --build (0 errors, base-build.log).
Not covered
- Per-commit attribution: shallow checkout (depth 2) —
git rev-list HEAD^1..HEAD^2yields only806cb6ewhile the metadata lists 3 commits (ff0c1a9,81ea3d9,806cb6e); the repository is shallow, so assume unreachable. The aggregateHEAD^1..HEADdiff is what was verified; the three commits' individual claims (initial projection / hardening / scan caps) were not exercised separately. The scan-cap claim was nonetheless probed directly via the P8 ladder. - Full fake-MCP end-to-end from the PR's evidence table: the A/B drives the real replay collectors and the real projector from compiled dist — it reproduces the wire shape and byte counts, not the daemon-level trigger path (model finalization → duplicate amplification). Seam-level numbers match the PR's table exactly (S7), so divergence risk is low but nonzero.
- Live seam driven through a real daemon/socket — covered instead by the PR's own
Session.test.tslive-boundary test (gate) plus the M1 mutation; not exercised over a live ACP connection. - Headless output, A2UI rendering, structured/rich payloads, generic NDJSON caps, backpressure, replay aggregate limits, diagnostics, artifact lifecycle — declared unchanged by the PR; not exercised.
- Windows/macOS — container is Linux.
- Repo-wide lint/format and full test suite — covered by the PR's own CI; targeted gates above. Fast-path closure check ran at HEAD only (its forbidden-closure invariant is the regression guard; a base-side bundle would cost a second full bundle).
- Trial merge into latest main — no network/token in this container; the merge-ref base
a5c637bis the workflow's current base tip and the PR is already rebased onto it, so staleness is not a concern for this round.
Methodology
Environment: node:22-bookworm CI container, node v22.23.2, refs/pull/8450/merge at depth 2 (HEAD = 8628c5f, base tip HEAD^1 = a5c637b, PR head HEAD^2 = 806cb6e); npm ci + npm run build pre-ran at HEAD. Base control: git worktree add tmp/base-tree HEAD^1, then the worktree was wired to the already-installed dependencies: root node_modules entries symlinked from the head tree except the @qwen-code scope, which was re-linked to the base tree's own packages (asserted in-harness, D4/D5: qwen-code-core realpath = tmp/base-tree/packages/core from the base arm, packages/core from the head arm), plus per-package nested node_modules symlinked from head (external deps only — no nested @qwen-code entries exist, and the PR touches no package.json/lockfile, so versions are lockfile-identical). Base built with the repo's own scripts/build_package.js (tsc --build) for core, acp-bridge, cli — exit 0 (base-build.log). The harness (ab-harness.mjs) imports each arm's compiled dist/src/... via file:// URLs and drives the real HistoryReplayer-backed collectors with synthetic ChatRecords shaped exactly like the repo's own test fixtures; native JSON.stringify byte length is the wire oracle. Mutation checks (mutation-matrix.sh) edited head sources, ran the pinned vitest files, and restored via git checkout (clean git status verified). Raw logs (ab-harness-run1.log, ab-harness-run2-capture.log, base-build.log, bundle.log, gates.log, gates-text.log, mutant-M1..M4.log, poscontrol-green.log, fastpath-check.log, fastpath-check-text.log, fastpath-planted.log, esbuild-metafile-backup.json) and harness scripts live in this directory; evidence PNGs were produced with scripts/verify-capture.mjs. Assertion accounting: 107 A/B-harness checks + 579 targeted-gate vitest tests (31+14+5+529) + 6 mutation-matrix checks (2 positive controls + 4 kills) + 2 fast-path closure checks (planted-red + real-green) = 694, all executed, all passed; fail counts only unexpected outcomes and there were none.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Addressed the latest review feedback in 2f7daf6.\n\n| Item | Action |\n| --- | --- |\n| Inner text-content metadata exemption | Added a regression proving extra inner fields keep the update untouched |\n| Maximum canonical block count | Added the paired 1,191-block fit boundary |\n| Legacy A2UI toolName-only detection | Added a projector-level exemption regression |\n| PR scope wording | Clarified that the bound applies to canonical textual fields only |\n\nValidation: projector tests 34/34, Prettier, ESLint, root build, and typecheck all passed. Two consecutive final diff audits were clean.\n\nRejected/deferred: none. Decisions needed: none. |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Review: Bound ACP textual tool-result payloadsOverviewAdds The design is sound and the risky part — the byte accounting — holds up under inspection. Comments below are mostly about gaps that should be named explicitly rather than defects in what's implemented. What I verified on the branch (
|
|
@wenshao Thanks for the thorough review. I checked each point against
This PR has now been through roughly five review rounds. Per the repository's review-convergence rule, I am freezing non-Critical code and documentation churn at this point, so I am deferring these suggestions instead of pushing another revision and resetting CI/review. The current PR body and design already state that structured/rich payloads and universal ACP frame limits remain out of scope; the more specific No user decision is needed for this disposition. |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 683 passed · 0 failed · 683 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：683 通过 · 0 失败 · 683 总计 Verification reportPR #8450 — fix(cli): Bound ACP textual tool-result payloadsVERDICT: 中文摘要 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证容器（
仅作为评审证据，不构成评审、批准或 CI 检查。 Previous-finding status (round 2 → this round)
Central claim + A/BCentral claim: eligible textual ACP tool-call updates (pure-text Both arms drive the compiled
The base control's oversized cells are asserted as control expectations (they pass by being verbatim). Delta-stability vs round 2: shape-matched cells are byte-identical to round 2's measurements (S1 500,065/500,011/1,000,493 → 65,536/65,482/131,435; S7 = PR's own table). Frame overhead differs from the PR's daemon table (1,000,385/131,347) only by the synthetic session-id envelope; the field-level numbers are the contract and match exactly. Secondary claims, all scripted (
CorrectionsNone — no inaccurate statements from earlier review rounds or bots required correction this round. No instruction-injection attempts were detected in the PR title/body/commits. FindingsNo blocking findings. Every executed assertion passed; benign traffic is byte-identical across arms (zero collateral); the new boundary behavior is pinned by the suite (M2/M3b/M6/M7 kills below). Two informational observations: F1 (informational): the new boundary tests pin observable outcomes; the guard layers they name are individually redundant by constructionThe mutation matrix (below) deleted each defensive layer of
The consequence for the new tests this round added: names like "collapses content when the empty structure cannot fit" suggest coverage of a specific guard, but what is actually pinned is the emergent arithmetic — and that is pinned, hard: the budget constant (M2: 16 failed), the rawOutput comparator pair (M3b: exactly 1 failed, the 65,536 reference-stability cell), the content-side comparator (M7: F2 (informational, extends round-2 finding #2): both defensive branches inside `truncateStringPayload` are unreachable from its only call sitesRound 2 established that the Mutation matrix — boundary guards (this round's delta)Positive control first: the four affected suites ran green unmutated (
Survivor adjudication ( Targeted gates (head)
Plus Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-checked at latest head: the new commit is test-only (pins the 1191-block exact-fit, the non-canonical extra-fields exemption, and legacy A2UI-by-toolName exemption); the projection module code is byte-identical to the reviewed version, so the 256KB bound and its verified properties are unchanged. Approval stands. Residual out-of-scope vectors (rawInput, structured rawOutput, non-canonical content passed through, bulk-replay frames) remain deferred per the design doc — keep the tracking items.
|
Released in v0.21.6. |
5 participants
What this PR does
This PR adds an internal ACP transport projection for canonical textual tool-call fields in every status. Eligible pure-text
contentarrays and primitive-stringrawOutputvalues are bounded independently to 65,536 UTF-8 bytes as serialized JSON fields, using exact JSON escape accounting, deterministic 20/80 head-tail previews, and bounded multi-block allocation. Live updates and every replay collector are covered, while A2UI and structured or rich content remain unchanged.The projection is immutable and idempotent. Canonical transcripts, model-facing responses, artifact metadata, ACP schemas, public APIs, and offline exports are not changed. The design documentation records the contract and updates the older statement that immediate ACP display events are always unchanged.
Why it's needed
Phase 1 bounds model-facing tool responses, but ACP display construction can still duplicate a complete tool result in both
contentandrawOutputafter model finalization. A deterministic fake-MCP reproduction produced a 1,000,385-byte ACP frame for one 499,999-byte result onorigin/main. This PR limits the eligible display fields at the actual live and replay delivery boundaries without making transport loss canonical.Reviewer Test Plan
How to verify
Run the focused projector, live Session, replay collector, and offline export tests from
packages/cli. Confirm the JSON-byte threshold cases pass at 65,535, 65,536, and 65,537 bytes; Unicode, escape, surrogate, multi-block, A2UI, rich-content, idempotence, and structural-sharing cases remain correct; live and replay fields are each at most 65,536 bytes; and offline export preserves the original length and SHA-256.Run the root build and typecheck, then bundle and run the serve fast-path closure check. The candidate should build without changing the fast-path bundle closure. Repeating the deterministic fake-MCP exchange should retain both fields, produce parseable JSON, and keep the ordinary textual fixture frame below 256 KiB.
Evidence (Before & After)
The same fake OpenAI response and stdio fake MCP tool returned 499,999 ASCII bytes to both bundles. Only lengths and SHA-256 values were captured; no result body was printed.
origin/main(be4f5e0963)cc1ceddfde)contentJSON UTF-8 bytesrawOutputJSON UTF-8 bytesSource fixture: 499,999 bytes, SHA-256
146e22690b10e8ea73a95f10a7399f26318528775e4479cc9f078f42e79c78a4. The offline export regression preserves the original source length and SHA-256. The candidate frame result applies only to this ordinary textual fixture and is not a universal frame-size contract.Tested on
Environment (optional)
macOS 26.4.1, Node.js 24.12.0, npm 10.9.8, local production bundles, sandbox disabled.
Risk & Scope
Linked Issues
Refs #8447
Refs #7306
Refs #8091
中文说明
本 PR 做了什么
本 PR 为所有状态的 ACP tool-call update 中的规范文本字段增加内部 transport projection。符合条件的纯文本
content数组与 primitive stringrawOutput会分别限制在字段 JSON 序列化后 65,536 UTF-8 字节以内，使用精确的 JSON 转义字节计算、确定性的 20/80 头尾预览以及有界的多 block 预算分配。live update 和所有 replay collector 都受到覆盖，A2UI、结构化或富内容保持不变。投影过程不可变且幂等。canonical transcript、模型侧响应、artifact metadata、ACP schema、公共 API 和离线 export 均不改变。设计文档记录了该契约，并修正了旧文档中“即时 ACP display event 始终不变”的表述。
为什么需要
Phase 1 已限制模型侧工具响应，但 ACP display 会在模型 finalization 之后仍将完整工具结果同时放入
content和rawOutput，形成重复放大。确定性的 fake-MCP 复现在origin/main上仅一个 499,999 字节结果就产生了 1,000,385 字节 ACP frame。本 PR 在实际 live 和 replay 交付边界限制符合条件的 display 字段，同时避免让 transport 有损结果进入 canonical 数据。Reviewer 测试计划
如何验证
在
packages/cli运行 projector、live Session、replay collector 和 offline export 的聚焦测试。确认 65,535、65,536、65,537 字节阈值用例通过；Unicode、转义、代理对、多 block、A2UI、富内容、幂等和结构共享用例保持正确；live 与 replay 字段分别不超过 65,536 字节；offline export 保留原始长度和 SHA-256。运行根目录 build 和 typecheck，然后执行 bundle 与 serve fast-path 闭包检查。候选版本应正常构建且不改变 fast-path bundle 闭包。重复确定性的 fake-MCP 交换后，两个字段都应继续存在，JSON 可解析，普通文本 fixture frame 小于 256 KiB。
证据（修改前后）
两个 bundle 使用完全相同的 fake OpenAI 响应和 stdio fake MCP 工具，返回 499,999 ASCII 字节。只采集长度和 SHA-256，没有打印结果正文。
origin/main(be4f5e0963)cc1ceddfde)contentJSON UTF-8 字节rawOutputJSON UTF-8 字节源 fixture 为 499,999 字节，SHA-256 是
146e22690b10e8ea73a95f10a7399f26318528775e4479cc9f078f42e79c78a4。offline export 回归测试保留原始源长度和 SHA-256。候选 frame 结果只适用于这一普通文本 fixture，不是通用 frame 大小契约。测试平台
环境（可选）
macOS 26.4.1、Node.js 24.12.0、npm 10.9.8、本地 production bundle、未启用 sandbox。
风险与范围
关联 Issue
Refs #8447
Refs #7306
Refs #8091