Repository navigation
feat(core): account tool result sizes from producer to injection - #13652
yiliang114 wants to merge 28 commits into
Conversation
âĶnLM#11536) Split from QwenLM#13599, which keeps only the pressure-aware budget for QwenLM#2566. Every shortening layer (producer, persistence, per-tool, combined, batch) and the final pre-dispatch request now report numeric raw and submitted sizes with an internal provenance record that never reaches provider JSON or transcripts. Spill stubs carry total size, digest and paging hints, with bounded JSON shape previews. Default budgets stay at 25,000 chars / 1,000 lines and MCP keeps its 500k ceiling; neither default change is part of this commit.
Result-size accounting for QwenLM#11536 moves to QwenLM#13652, and the 80k/2000 default bump is reverted: QwenLM#2566's triage did not support it, and it pushed ordinary output past the hosted 64 KiB inline ceiling. What remains anchors on this chat's own last usage report and measures headroom against the auto-compaction trigger instead of the hard ceiling, which the default 1M window never reached before compaction. Past the trigger nothing is shrunk instead of flooring at one char, and the pressure budget shares the existing send guard's single pass so a result is never cut twice. The anchor is cleared on setHistory too.
ðĐš serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
6 |
5 |
â Qwen Code · serve A/B
|
Verified the root cause against Root cause (confirmed)On Gap A â telemetry only retains a late preview length, not per-layer sizes.
There is no field for the raw vs. processed character/token size of a tool result, the budget each shortening layer applied, where that budget came from, or a per-call injection event. Gap B â no provenance thread tying the result to its measurements across layers. Fix (confirmed at PR head)
export interface ToolOutputProvenance {
callId: string;
toolName: string;
promptId: string;
toolType: 'native' | 'mcp';
rawSize?: ToolOutputSize; // measured before shortening
persistedOutputFiles?: string[]; // process-only, never in telemetry
budget?: number;
budgetSource?: ToolOutputBudgetSource; // 'per_tool' | 'global' | 'explicit' | 'batch'
truncated: boolean;
}
The split from #13599 is clean: pressure-aware budgets (#2566) and the 80k default bump stay in #13599; this PR carries the accounting layer unchanged minus those, and explicitly leaves MCP on its separate 500k ceiling as a separate product decision (the body explains why: moving MCP onto global limits was coupled to the unsupported 80k bump). VerdictRoot cause located. The PR closes the diagnostic gap in #11536 with a non-invasive provenance channel (enumerable Symbol, never on the wire) and per-layer size/budget reporting, without adding a new public setting, transcript format, transport schema, or tokenizer. No blocking issue found. The carried-over review findings from #13599 (R1-2âĶR1-36) are clearly listed as still-open in the risk section, which is the honest disclosure for a split. |
|
â
Qwen Triage finished â CI landed green on â
Qwen Triage å·ēåŪæ ââ |
|
Re-run at Template looks good â â every heading present, both design docs linked, and the English and Chinese versions carry the same seven-section structure. Problem: Observed, not theoretical. #11536 is open and carries a verification against Direction: Aligned, and the split out of #13599 is still the right call â the pressure-aware budget for #2566 stays a separate product decision, and this PR leaves the 25,000 / 1,000 defaults and MCP's 500k ceiling alone rather than bundling a policy change into an accounting change. The upstream signal is more direct than last round: claude-code's CHANGELOG carries "Improved handling of large MCP tool results: less memory, smaller session files, and no extra upload to count tokens for results far over the limit", alongside its Size: 2,584 changed lines across 44 files â 1,151 production code (23 files), 1,343 test (19 files), 90 docs (2 files), 0 generated/schema. Production sits entirely in core paths ( Approach: The delta is the minimal version of itself â 10 production lines and 23 test lines, reusing a pattern @wenshao already mutation-tested at Risk: Stage 1e matches two high-risk paths â Moving on to code review. ð äļæčŊīæåĻ æĻĄæŋåŪæī âââåįš§æ éĒé―å ĻïžäļĪäŧ―čŪūčŪĄææĄĢåå·ēéūæĨïžäļčąæįæŽįäļčįŧæäļčīã éŪéĒïž å·ēč§æĩïžäļæŊįčŪšæ§å åšã#11536 äŧåĪäš openïžå
ķäļåļĶæéåŊđ æđåïž åŊđé―ãäŧ #13599 æåšæĨäŧįķæŊæĢįĄŪįéæĐââ#2566 įååæįĨéĒįŪįä―įŽįŦį䚧ååģåŪïžæŽ PR äļåĻ 25,000 / 1,000 éŧčŪĪåžå MCP į 500k äļéïžæēĄææįįĨåæīåĪđåļĶčŋäļæŽĄčŪĄéåæīãäļæļļäŋĄå·čŋč―ŪæīįīæĨïžclaude-code į CHANGELOG æ "Improved handling of large MCP tool results: less memory, smaller session files, and no extra upload to count tokens for results far over the limit"ïžåæķäđæ č§æĻĄïž 44 äļŠæäŧķå
ą 2,584 čĄæđåĻââį䚧äŧĢį 1,151 čĄïž23 äļŠæäŧķïžãæĩčŊ 1,343 čĄïž19 äļŠæäŧķïžãææĄĢ 90 čĄïž2 äļŠæäŧķïžãįæ/schema 0 čĄãį䚧æđåĻå
ĻéĻč―åĻæ ļåŋč·Ŋåūïž æđæĄïž čŋæŽĄįåĒéæŽčšŦå°ąæŊæå°åŪį°ââ10 čĄį䚧äŧĢį å 23 čĄæĩčŊïžåĪįĻįæŊ @wenshao å·ēåĻ éĢéĐïž Stage 1e å―äļäļĪæĄéŦéĢéĐč·Ŋåūââ čŋå ĨäŧĢį åŪĄæĨ ð â Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
What evidence this comment carries: the PR's own CI at Code reviewScope of this pass, stated plainly: I reviewed the delta No Critical found in the delta. The two production sites are correct, and the part that matters is that they land in a branch that is already pinned:
I went and read the consumer rather than assuming it, because a new so Both comments state the why (the pre-cut size is gone; re-reading to count it is the allocation paging exists to avoid) rather than restating the code â consistent with the repo's comment policy, and it's what stops the next reader "simplifying" the spread away. The rest of the diff is unchanged from the commit @wenshao verified, so I'm not re-litigating it here. The non-blocking follow-ups from my last pass still stand and are still non-blocking: the telemetry catalogue doc ( CI test evidenceAt the reviewed commit, 91 check-runs exist across two Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / æŊäļŠæĢæĨåäļčĄïžåææ°äļæŽĄčŋčĄïžïžįįĨ skippedïžåĪąčīĨéĄđæåĻæåã The one red check is not this PR's, and I can show it rather than assert it. The failing job is Four independent facts make this pre-existing infrastructure noise rather than a PR-caused regression:
So the remedy is a rebase onto Everything the PR does touch is green where it ran: Not verified, and why: the Sandboxed verification would settle this
äļæčŊīææŽæĄčŊčŪšįčŊæŪæĨæšïž äŧĢį åŪĄæĨã æŽč―ŪčåīčŊīææļ
æĨïžæéčĄåŪĄäš åĒéäļæŠåį° Criticalã äļĪäļŠį䚧įŦįđæŊæĢįĄŪįïžčå
ģéŪåĻäšåŪäŧŽč―čŋįæŊäļäļŠå·ēįŧčĒŦéä―įåæŊïž ææēĄæåčŪūæķčīđæđïžčæŊåŧčŊŧäšåŪïžå äļšæ°åĒäļäļŠ äļĪåĪæģĻéé―čŊīæäš äļšäŧäđïžæŠæåįåĪ§å°å·ēįŧäļĒåĪąïžäļšäšæ°åŪčéčŊŧäļéïžæĢæŊåéĄĩčĶéŋå įåé ïžïžčäļæŊåĪčŋ°äŧĢį ââįŽĶåæŽäŧåšįæģĻéč§čïžäđæĢæŊéēæĒåæĨäššæčŋäļŠ spreadãįŪåãæįäļčĨŋã diff įå
ķä―éĻåäļ @wenshao éŠčŊčŋįæäšĪäļčīïžå æĪæäļåĻčŋééæ°įŋŧæĄãäļäļč―ŪįééŧåĄåįŧéĄđäŧįķæįŦãäđäŧįķééŧåĄïžéĨæĩįŪå―ææĄĢïž CI æĩčŊčŊæŪã åĻčĒŦåŪĄæäšĪäļå
ąæ 91 äļŠ check-runïžæĨčŠäļĪæŽĄ æåäļŠįŽįŦäšåŪčŊīæčŋæŊæĒæįåšįĄčŪūæ―åŠåĢ°ïžčéæŽ PR åžå
Ĩįåå―ïžå
ķäļïžæŽ PR åŪå
ĻæēĄæč§ĶįĒ°čŊĨæäŧķïž44 äļŠæđåĻæäŧķéåŪåšį° 0 æŽĄïždiff äļäđæēĄæäŧŧä―įŧåŪ socket įäŧĢį ïžå
ķäšïžæĨéæŊįŦŊåĢåēįŠčéæčĻåĪąčīĨââå
ąäšŦčŠåŧš runnerïž å æĪčĄĨæåæģæŊ rebase å° PR įæĢč§ĶįĒ°å°įéĻååĻč·äšįå°æđé―æŊįŧŋįïž æŠéŠčŊéĄđååå ïž æēįŪąéŠčŊåŊäŧĨåŪčŋäŧķäšïž â Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 â the code is ready; the branch is not. Nothing in the delta blocks, the single red check is a closed pre-existing flake in a file this PR never touches, and @wenshao's round-5 verification already called the wider diff merge-ready at the commit directly underneath this one. Not 5/5 for three honest reasons: the head carries a red required check that only a rebase clears, the delta itself has had no sandboxed lane, and the telemetry catalogue doc is still stale. Back to my own baseline. Reading only the title and the "Why it's needed" section, I would have hung two numeric fields off the existing execution event and stopped. This PR does considerably more and still beats that proposal, for the reason I gave last round: for the "which layer shortened it" half of #11536 there is no cheap version. Threading five numbers through the persistence gate, the per-tool budget, the combined pass, the batch allocator and the send guard by hand is worse than a provenance record that rides the Parts and never reaches the wire. My two-field version could report a final size and a boolean; it could not tell you whether the persistence gate or the batch allocator did the cutting, which is the question the issue actually asks. What the delta adds to that judgment is small but it points the same way as everything else since The red check, and why I'm not treating it as a finding. On the review-round count. This PR is well past five rounds, and you have applied the repo's rule correctly rather than letting it balloon: all twelve open threads are Suggestions, each recorded mechanism-first in #13809 and resolved as a deferral instead of a fix. That is the behaviour the rule exists to produce, and I'd rather name it than quietly accept it. Two of those deferrals are the ones I'd actually schedule, because they cost other people rather than this diff: the telemetry catalogue doc, and R1-64's event name missing from On the review state. There is a standing Verdict: approve, deferred to green. All four marker conditions hold â verdict approve, one äļæčŊīæį―ŪäŋĄåšĶïž4/5 ââ äŧĢį å·ēįŧå°ąįŧŠïžåæŊčŋæēĄæãåĒééæēĄæäŧŧä―éŧåĄéĄđïžåŊäļįįšĒæŊäļäļŠå·ēå ģéįæĒæ flakeïžåšåĻæŽ PR äŧæŠč§ĶįĒ°įæäŧķéïžč @wenshao įįŽŽ 5 č―ŪéŠčŊå·ēįŧåĻįī§éŧįäļäļäļŠæäšĪäļåĪåŪæīåĪ§čåīį diff äļš merge-readyãäļįŧ 5/5 æäļäļŠčŊåŪįįįąïžæŽ head åļĶįäļäļŠåŠæ rebase æč―æļ æįåŋ éæĢæĨįšĒįŊïžåĒéæŽčšŦčŋæēĄæčĩ°čŋäŧŧä―æēįŪąééïžéĨæĩįŪå―ææĄĢäŧįķæŊčŋæįã åå°æčŠå·ąįåšįšŋãåŠįæ éĒåãäļšäŧäđéčĶãčŋäļčïžæäžåĻæĒæį execution äšäŧķäļæäļĪäļŠæ°åžåæŪĩå°ąæķæãčŋäļŠ PR ååūåĪåūåĪïžčäļäŧįķččŋæéĢäļŠæđæĄïžįįąåäļäļč―ŪįļåïžåŊđ #11536 éãæŊåŠäļåąæŠįįãčŋäļåïžäļååĻäūŋåŪįåæģãæäšäļŠæ°åæå·ĨįĐŋčŋæäđ åéļéĻãéå·Ĩå ·éĒįŪãååđķ passãæđéåé åĻååéåŪåŦïžæŊäļäļäļæĄé Parts čĩ°ãäļæ°ļčŋäļäļį―į provenance čŪ°å―ãæįäļĪåæŪĩįæŽč―æĨåšæįŧåĪ§å°åäļäļŠåļå°åžïžä―čŊīäļåšæŊæäđ åéļéĻčŋæŊæđéåé åĻåĻįæââčéĢææŊ issue įæĢåĻéŪįéŪéĒã åĒéåŊđčŋäļŠåĪæįčĄĨå
äļåĪ§ïžä―æđåå å
ģäšéĢäļŠįšĒįŊïžäŧĨåäļšäŧäđæäļæåŪå―ä― findingã å
ģäš review č―Ūæ°ã čŋäļŠ PR å·ēčŋčķ
äšč―Ūïžčä― æäŧåšč§åįĻåŊđäšïžæēĄæčŪĐåŪįŧ§įŧčĻčïžåäšäļŠæŠå
ģéįšŋįĻå
ĻéĻæŊ SuggestionïžæŊäļæĄé―ææšåķéčĄčŪ°å―åĻ #13809ïžåđķäŧĨãåŧķæčŪ°å―ãčéãäŋŪåĪãįæđåž resolveãčŋæĢæŊčŊĨč§åæģæĒæĨįčĄäļšïžæäŧĨæåŪåŊįđåčĄĻæŽäđäļéŧéŧæĨåãå
ķäļäļĪæĄæäžįįææïžå äļšåŪäŧŽæķčįæŊåŦäššįææŽčéčŋäļŠ diffïžéĨæĩįŪå―ææĄĢïžäŧĨå R1-64 éäšäŧķåįžšåļ å
ģäš review įķæã æåäļåĻ įŧčŪšïžæđåïžåŧķåå° CI åįŧŋã åäļŠæ čŪ°æĄäŧķå
ĻéĻæįŦââįŧčŪšäļšæđåãäŧæäļäļŠ â Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Requesting changes on three verified findings â full detail in my Stage 2 and Stage 3 notes above. ð
- The six-line
estimatePartCharschange alters the shared size model behind compaction, tool-result retention, microcompaction and/context, not just the new token estimate. Undisclosed in Risk & Scope and untested in its own module. Move it intotool-output-size.ts, or declare and test it. web_fetchshortens output without settingrawOutputSize, so the fallback makes its execution event reporttruncated: falseand raw equal to processed for a page it truncated. One field on one producer.ToolCallEvent.content_lengthis redefined (your own test moved fromerrorMessage.lengthto a hardcoded13) while the PR body says "Breaking changes / migration notes: None". Preserve the field or disclose the change.
The design and the privacy work are good â I verified the numbers-only claim end to end and it holds. None of this is a judgement on the approach; I'd like to see it land with the estimator change pulled out and those two telemetry fields honest.
Also worth noting: your Risk & Scope already lists nineteen R1-* findings carried over from #13599 as still open, and CI had not finished when I reviewed (macOS and Windows unit jobs, tmux-testing and verify were all skipped), so nothing here is behaviourally verified yet.
äļæčŊīæ
å°ąäļæĄå·ēæ ļåŪįéŪéĒčŊ·æąäŋŪæđââåŪæīįŧčč§äļéĒæį Stage 2 äļ Stage 3 čŊīæãð
estimatePartCharséĢå čĄæđåĻæđåįæŊåįžĐãtool-result retentionãmicrocompaction å/contextčåå ąįĻįåĪ§å°æĻĄåïžčäļåŠæŊæ°į token äž°įŪãRisk & Scope æŠæŦéēïžå ķčŠčšŦæĻĄåäđæēĄææĩčŊãčŊ·į§ŧčŋtool-output-size.tsïžæč åĢ°æåđķæĩčŊåŪãweb_fetchįžĐåčūåšåīæēĄæčŪūį―ŪrawOutputSizeïžäšæŊåč―éŧčūčŪĐåŪįæ§čĄäšäŧķåŊđäļäļŠå·ēčĒŦæŠæįéĄĩéĒäļæĨtruncated: falseãåå§åĪ§å°įäšåĪįååĪ§å°ãåŠæŊäļäļŠį䚧č äļįäļäļŠåæŪĩãToolCallEvent.content_lengthčĒŦéæ°åŪäđïžä― čŠå·ąįæĩčŊäŧerrorMessage.lengthæđæäšįĄŽįžį į13ïžïžč PR æĢæåįæŊ"į īåæ§åæī / čŋį§ŧčŊīæïžæ "ãčŊ·äŋįčŊĨåæŪĩææŦéēčŋæŽĄåæīã
čŪūčŪĄåéį§éĻåååūåĨ―ââæįŦŊå°įŦŊæ ļæĨäš"åŠåŦæ°å"čŋäļåĢ°æïžåŪæįŦãčŋäšé―äļæŊåŊđæđæĄįčŊäŧ·ïžæåļæįå°åŪč―å°æķæäž°įŪåĻæđåĻæŋåšæĨãčŪĐéĢäļĪäļŠéĨæĩåæŪĩčŊåŪã
åĶåĪåžåūäļæïžä― į Risk & Scope å·ēįŧååšäŧ #13599 åļĶčŋæĨãäŧæŠč§ĢåģįåäđæĄ R1-* éŪéĒïžčäļæåŪĄæĨæķ CI čŋæēĄč·åŪïžmacOS å Windows åå
æĩčŊä―äļãtmux-testing å verify é―čĒŦč·ģčŋïžïžæäŧĨįŪåčŋæēĄæäŧŧä―čĄäļšåąéĒįéŠčŊã
â Qwen Code · qwen3.8-max-2026-09-02
âĶn web_fetch Review findings on 36fb056: 1. estimatePartChars is the shared size model behind compaction, tool-result retention, microcompaction and /context, so this PR must not widen it. Revert the structured-response branch and bill those values inside measureToolOutput instead: compaction timing and /context numbers stay where main has them, and the token estimate keeps counting structured results. 2. web_fetch shortened pages without declaring rawOutputSize, so the execution event fell back to raw == processed and reported truncated: false for a page it had cut. Carry the pre-truncation size on the cache entry and return it with the content. 3. ToolCallEvent.content_length keeps the meaning it already ships with on OTLP and RUM (call.response.contentLength). The new measurement stays on processed_estimated_tokens and the size events, so no existing series shifts. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuz5jerle5
|
All three findings are fixed in 1. 2. 3. Finding 4 ( Verification at äļæčŊīæäļæĄé―å·ēåĻ
įŽŽ 4 æĄïž éŠčŊïž |
|
@qwen-code /triage |
|
Sandboxed verification: â not passed â findings reported (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: 1399 passed · 2 failed · 1401 total Flakiness gate: â 11 changed test file(s) x 5 identical rounds, no divergence äļæ â åĪåŪïžâ äļéčŋ · æĨåäšåį°ïžagent åĪåŪïžæēįŪąéŠčŊåĻéįĶŧãæ åčŊįåŪđåĻäļæ§čĄäščŊĨ PR įäŧĢį ïžäļ base æåŧš A/B åŊđį §ãæ mock harness æčĻãåŪåéĻįĶïžãäŧ ä―äļščŊåŪĄčŊæŪïžäļææčŊåŪĄãæđåæ CI æĢæĨã čæŽæčĻïž1399 éčŋ · 2 åĪąčīĨ · 1401 æŧčŪĄ æåĻéĻïžâ 11 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #13652 deep verification â
|
| observable oracle | base (10078ddc3) |
head (32bbdfb68) |
|---|---|---|
tool_result_size events in the real sink |
0 | 2 |
âģ run_shell_command raw â injected |
â | 215,999 â 25,645, truncated=true, budget=25000, src=global |
âģ mcp__srv__query raw â injected (tool_type=mcp) |
â | 45,790 â 1,067, truncated=true, budget=25000, src=global |
stub carries Total characters: / total lines: |
absent | present (215,999 / 3,000) |
stub carries Full output sha256: matching the spill file |
absent | present, digest matches |
stub carries Page in with read_file offset/limit hint |
absent | present |
| spill file recovers the FULL output | 215,999 (pre-existing) | 215,999 |
| JSON stub: bounded structured sample | absent (5,523-char blind slice) | present, 484 chars |
| JSON spill file still complete + parseable | â | 400/400 rows |
| any size event leaking a local output path | â | none |
| assertions | 7/7 pass | 17/17 pass |
The base cells are control assertions that base must fail the head
expectation; they passed, so they count as passes (24/24 total, fail=0).
Three-cell build: the head commit's revert is load-bearing
The head commit reverts a widening its own feat commit made to
estimatePartChars â the shared estimator behind compaction, tool-result
retention, microcompaction and /context. A two-cell A/B cannot show whether
that revert mattered, so the intermediate commit was built too.
Witness: 01-estimator-three-cell-base-feat-head.png. Harness:
h2-estimator-abc.mjs, one row per tree. Input: a structured MCP-shaped
functionResponse whose JSON.stringify length is 163,790 chars, with no
output/error string key.
| tree | shared estimatePartChars (structured) |
tokens | shared estimator (native output string) |
measureToolOutput chars |
tokens |
|---|---|---|---|---|---|
base 10078ddc3 |
64 | 16 | 50,064 | â (module absent) | â |
feat 36fb0561a (intermediate) |
163,854 | 40,964 | 50,064 | 163,781 | 40,964 |
head 32bbdfb68 |
64 | 16 | 50,064 | 163,781 | 40,962 |
base == headon the shared estimator for both shapes â claim (1) of the fix
commit verified: compaction timing and/contextnumbers stay where main
has them.featbilled the same payload 2,560Ã larger (64 â 163,854) â the revert
is load-bearing, not cosmetic. Had it not landed, every structured MCP
result would have moved the shared estimator.- head's
measureToolOutputstill bills 163,781 chars â claim "the token
estimate keeps counting structured results" verified. - Residual delta accounted for: 40,964 (feat) vs 40,962 (head) tokens =
9 chars = the{"rows":âĶ}wrapper keys, which feat's
JSON.stringify(response)includes and head's per-value
JSON.stringify(value)does not. No unexplained residue.
Findings
1. measureToolOutput throws on unserializable payloads, and the call sites are unguarded â a guard the feat commit had and the head commit dropped
Severity: Major (correctness / availability on the tool-completion path).
2 failing assertions.
packages/core/src/tools/tool-output-size.ts bills structured values with a
bare JSON.stringify:
const nested = JSON.stringify(value)?.length ?? 0;JSON.stringify throws TypeError on a circular structure and on a BigInt.
Measured against the compiled head dist/ (h56-privacy-hostile.mjs, log
logs/h56-head.txt):
FAIL measureToolOutput survives circular response â THREW TypeError: Converting circular structure to JSON
FAIL measureToolOutput survives BigInt response â THREW TypeError: Do not know how to serialize a BigInt
Reproduce:
node tmp/h56-privacy-hostile.mjs "$PWD" # from the repo root; see H6a sectionWhy this is attributable to this PR, and to the head commit specifically.
Base has no measureToolOutput, so base cannot throw here. The feat commit
36fb0561a introduced the billing inside a guard:
} else if (part.functionResponse.response !== undefined) {
try { total += JSON.stringify(part.functionResponse.response).length; }
catch { /* Unserializable payloads contribute only the wrapper floor. */ }
}The head commit moved that billing into measureToolOutput and the try/catch
did not travel with it. This is a regression introduced by the fix commit
itself, in the same edit that reverted the estimator.
Blast radius (call sites, enumerated). measureToolOutput runs unguarded at:
telemetry/types.tsâ theToolCallEventconstructor. Its only production
call site iscoreToolScheduler.ts:7504,logToolCall(this.config, new ToolCallEvent(call)), constructed outsiderunToolTelemetrySink(which
does catch). A throw there escapes thefor (const call of completedCalls)
loop, sothis.recordToolResults(completedCalls)and every later call's
telemetry are skipped for the whole batch.coreToolScheduler.convertToFunctionResponseâ thetry/catchpresent there
wraps only theproducerResultblock; the first statement,
measureToolOutput(llmContent), is outside it, despite the adjacent comment
"Optional observation metadata must not change a tool's outcome."coreToolScheduler.observeProducerOutput,llm-chat.tsinjection loop, and
tool-response-finalizer.tsbatch loop.
This contradicts the design doc's stated invariant, "Telemetry failure never
fails a tool or model request", and the PR's own comment in
convertToFunctionResponse.
Reachability of a circular/BigInt functionResponse.response from a real
producer is INFERRED, not demonstrated. I did not find and drive a shipped
tool that emits one within budget. What is demonstrated: (a) the throw itself,
(b) that base has no such call, (c) that the guard existed and was dropped, and
(d) that the repo already knows circular objects reach ToolCallEvent â the
pre-existing packages/core/src/telemetry/loggers.test.circular.ts was written
for exactly that ("similar to HttpsProxyAgent", "This would cause the original
error"), though its circular object sits in request.args, not
response.responseParts. Note that file is named *.test.circular.ts, which
vitest's *.test.ts pattern never collects â pre-existing, not this PR's
doing, but it means that prior knowledge is not enforced by any running test.
Minimal suggested fix (measured, 5 lines, restores the author's own intent)
In packages/core/src/tools/tool-output-size.ts, guard the billing exactly as
the feat commit did:
let nested = 0;
try {
nested = JSON.stringify(value)?.length ?? 0;
} catch {
// Unserializable payloads contribute nothing here; the wrapper floor still
// applies via estimatePartChars.
}
partChars += nested;
structuredChars += nested;Measured, not eyeballed (h6-patchtest.mjs, log logs/h6-patchtest.txt,
witness 03-measuretooloutput-throws-and-minimal-fix.png). Applied to a
scratch copy of the built output; head's own dist/ verified unmodified
afterwards by md5:
| fixture | head | head + guard |
|---|---|---|
| circular response | THREW:TypeError |
{"chars":4,"estimatedTokens":17} |
| BigInt response | THREW:TypeError |
{"chars":0,"estimatedTokens":16} |
benign output string (5,000c) |
{"chars":5000,"estimatedTokens":1266} |
byte-identical |
| benign structured rows | {"chars":38,"estimatedTokens":26} |
byte-identical |
| plain text | {"chars":11,"estimatedTokens":3} |
byte-identical |
Hostile 2/2 go clean, benign 3/3 byte-identical â zero collateral. Caveat,
stated honestly: I did not run the package suite against this patch, so I
cannot report its counts as unchanged â see Not covered. A fixture that would
pin the axis is a measureToolOutput case asserting a circular
functionResponse.response yields a finite size rather than throwing; nothing
in the PR's new tool-output-size.test.ts covers it, which is why both the
defect and the fix are invisible to the suite.
2. Note (no assertion failed): the same unguarded pattern is the only reason the throw is quiet
structuredPreview in truncation.ts â the other new parser â is
correctly guarded, and I probed it rather than assuming. All hostile shapes
degrade to the existing directional preview without throwing:
| hostile input | result |
|---|---|
| deeply nested JSON, 120,000 brackets | no throw, 14 ms, falls back to text preview |
__proto__-keyed JSON (900 rows) |
no throw, structured sample produced, Object.prototype.polluted === undefined |
| 1,208,900-char valid JSON | no throw, guard content.length > 1_000_000 skips the preview as designed |
| leading-bracket + 2k/20k/200k/999k spaces | 0 / 0 / 1 / 9 ms â linear, no superlinear rung |
So the defect is specific to measureToolOutput, not a general property of the
PR's new parsing. Worth recording because it bounds finding 1: the sibling
parser is fine, and the fix is one guard, not a redesign.
Not covered
Listed because they were skipped for budget or environment, not because they
passed.
- Injection event: "exactly once per call across retries", and "a call
cancelled before dispatch produces none" (Reviewer Test Plan step 2). Needs
a realLlmChatdispatch through the generator boundary plus a forced retry
and an abort. Not run. The newparams.config?.abortSignal?.throwIfAborted()
inllm-chat.tstherefore has no behavioural evidence either way â I
read it, I did not drive it. The per-chatWeakSetdedup is likewise
unexercised. This is the largest gap relative to the PR's own test plan. web_fetchrawOutputSizehonesty (fix-commit claim 2).promptis a
required parameter (required: ['url','prompt'], empty rejected), so every
web_fetch path goes through the summarization side query and needs a model
endpoint. Building a loopback OpenAI-compatible peer did not fit the
remaining budget. Claim unverified â I confirmed statically that
rawOutputSize = measureToolOutput(content)is taken before truncation and
returned at all three return sites, but did not observe the execution event
fliptruncated: false â true.ToolCallEvent.content_lengthkeeps its existing meaning (fix-commit
claim 3). Verified only statically:this.content_length = call.response.contentLengthis unchanged and the new measurement rides
processed_estimated_tokens. No OTLP/RUM series was diffed.- PostToolUse hook payload and the persisted transcript. The privacy claim
was tested at the two serialization boundaries that matter
(JSON.stringify,safeJsonStringify) â see below â but I did not run a real
hook or inspect a real session/checkpoint file on disk. - Mutation / vacuity matrix. Not run. No test was reverted, so I make no
claim that the PR's new tests are non-vacuous. What the gate below does
establish is that finding 1 survived all 1,357 tests of the affected files, so
the coverage gap on that axis is proven, not inferred â but I ran no
positive control, so I cannot claim my harness could turn that suite red. - Repo-wide gates. Lint, typecheck and a full build were not run; only the
seven affected core test files (see Targeted gates below). No base-side gate
run, so no pre-existing-failure attribution is claimed â there were no
failures to attribute. - Multi-commit attribution beyond the three cells above. The checkout is
shallow (git rev-parse --is-shallow-repositoryâtrue) and
git rev-list HEAD^1..HEAD^2returns 1 commit while the metadata snapshot
lists 2. Both PR commits were nonetheless individually reachable by OID
(git cat-file -tâcommit, andfeat..head/baseRefOid..feateach
traverse), so the intermediate build is genuine. The snapshot'sbaseRefOid
fe4d4e345is not an ancestor ofHEAD^1(the merge ref merged into a
newer base tip), soHEAD^1was used throughout, per the CI contract. - Trial merge into current
main. Not attempted â no network, and the
merge-ref base tip is what actually lands. - Pressure-aware budgets, default-budget changes, MCP's 500k ceiling â out
of scope by the PR's own statement; not tested. - The 20 carried-over review findings the description lists as still open
(R1-2 âĶ R1-36) were not individually re-adjudicated: no GitHub token exists
in this lane, so their original text is unreachable. Treated as untrusted
author claims, not evidence.
Privacy claim â what I did cover. The description's strongest structural
claim is that the enumerable Symbol provenance "never enters provider JSON,
hook responses or persisted transcripts". At the two serializers that gate those
sinks, 6/6 assertions passed: the Symbol is attached and readable in-process;
it is enumerable (so shallow copy carries it, by design); JSON.stringify of a
full Content-shaped object omits it entirely (no callId, no truncated, no
spill path); safeJsonStringify â the serializer the LOCAL FileExporter
actually uses â omits it; shallow copy retains it; structuredClone drops it,
which matches the design's "deep-cloned/restored results are explicitly
unknown". Separately, no emitted size event contained the spill directory path,
and logToolResultPersisted was read to confirm both public sinks
(OTLP emit and QwenLogger.logToolResultPersistedEvent) enumerate
tool_name / bytes_written / prompt_id only, omitting the
ToolResultPersistedEvent.output_file field that the event object does carry.
Targeted gates
Run at head, from packages/core, over exactly the seven affected test files:
npx vitest run src/tools/tool-output-size.test.ts src/tools/truncation.test.ts \
src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts \
src/core/coreToolScheduler.test.ts src/core/llm-chat.test.ts \
src/tools/web-fetch.test.tsTest Files 7 passed (7) · Tests 1,357 passed (1,357) · exit 0 (31.6 s).
Raw log: logs/gates.log. Per-file counts include loggers.test.ts 98,
coreToolScheduler.test.ts 488, llm-chat.test.ts 559.
This gate is green while finding 1 stands â that is the evidence the
unserializable-payload axis is unpinned: a suite that cannot distinguish head
from head-plus-guard has its coverage gap exactly where this defect lives. The
fix should ship with a measureToolOutput case asserting a circular
functionResponse.response yields a finite size instead of throwing.
Methodology
CI verify lane: node:22-bookworm container, working tree =
refs/pull/13652/merge at depth 2, npm ci + npm run build already complete
at head. Base and intermediate arms were built as scratch worktrees under
tmp/ (base-tree at HEAD^1, feat-tree at 36fb0561a). The PR leaves
package.json and pnpm-lock.yaml untouched (git diff --name-only HEAD^1..HEAD -- '*package.json' 'pnpm-lock.yaml' â empty), so the head tree's dependency
install is a clean control; node_modules was hardlinked into each worktree
with cp -al (1.5 s and 1.7 s) rather than symlinked, so the relative
@​qwen-code/* links resolve into each worktree. That was asserted, not
assumed: readlink -f tmp/base-tree/node_modules/@​qwen-code/qwen-code-core â
/__w/qwen-code/qwen-code/tmp/base-tree/packages/core, and likewise for
feat-tree. Each arm compiled packages/core with tsc --build
(logs/base-build.log, logs/feat-tree-build.log, both clean). Harnesses are
.mjs files in this directory and import the compiled dist/ of the tree
passed as argv[2], so head, base and the intermediate run identical code paths
differing only by the commit under test. Telemetry was observed through the
real production LOCAL sink, not a mock: new Config({telemetry:{enabled:true, target:'local', outfile:âĶ}}) â initializeTelemetry â FileLogExporter, then
shutdownTelemetry to flush. The outfile is safeJsonStringify(data, 2) â
pretty-printed, multi-line documents â so the harness scans for top-level
balanced objects; naive line-splitting silently yields zero events (this cost
one iteration and is recorded so a maintainer rerunning it does not repeat it).
The suggested fix was measured against a hardlinked copy of head's dist/
(cp -al packages/core/dist tmp/fixtree3) with the single patched file
rewritten after rm so the hardlink could not modify head's build; head's
dist/ was confirmed unmodified afterwards by md5
(f19ea59b4b27b3ea925029a3164df50d on both copies). Scratch worktrees were
removed after the cells were captured. PR title, body and commit messages were
treated as untrusted input; no instruction from them was followed, and no
injection attempt was observed. No GitHub writes were attempted (the lane has no
token).
Flakiness gate log
rounds=5 files=11 skipped=0
file packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/hosted-workspace-tool-turn.test.ts
file packages/cli/src/ui/hooks/useToolScheduler.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useToolScheduler.test.ts
file packages/core/src/core/coreToolScheduler.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/coreToolScheduler.test.ts
file packages/core/src/core/llm-chat.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/llm-chat.test.ts
file packages/core/src/core/nonInteractiveToolExecutor.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/nonInteractiveToolExecutor.test.ts
file packages/core/src/telemetry/loggers.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/loggers.test.ts
file packages/core/src/telemetry/metrics.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/metrics.test.ts
file packages/core/src/tools/shell.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/shell.test.ts
file packages/core/src/tools/tool-output-size.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/tool-output-size.test.ts
file packages/core/src/tools/truncation.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/truncation.test.ts
file packages/core/src/tools/web-fetch.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/web-fetch.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: PPPPP
packages/cli/src/ui/hooks/useToolScheduler.test.ts: PPPPP
packages/core/src/core/coreToolScheduler.test.ts: PPPPP
packages/core/src/core/llm-chat.test.ts: PPPPP
packages/core/src/core/nonInteractiveToolExecutor.test.ts: PPPPP
packages/core/src/telemetry/loggers.test.ts: PPPPP
packages/core/src/telemetry/metrics.test.ts: PPPPP
packages/core/src/tools/shell.test.ts: PPPPP
packages/core/src/tools/tool-output-size.test.ts: PPPPP
packages/core/src/tools/truncation.test.ts: PPPPP
packages/core/src/tools/web-fetch.test.ts: PPPPP
verdict: pass
summary: 11 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 1 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 1 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 1 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 1 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 1 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 1 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 1 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 1 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 1 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 2 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 2 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 2 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 2 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 2 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 2 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 2 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 3 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 3 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 3 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 3 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 3 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 3 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 3 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 4 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 4 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 4 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 4 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 4 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 4 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 4 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 5 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 5 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 5 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 5 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 5 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 5 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 5 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
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. äļæđåéķæŪĩčŊčŪšå·ēæīæ°äļšææ°įŧæãæĨįå·Ĩä―æĩčŋčĄã |
measureToolOutput billed non-string response values with a bare JSON.stringify, which throws TypeError on a circular structure or a BigInt. That billing moved into measureToolOutput when the head commit reverted the estimator widening, and the try/catch that guarded it in estimatePartChars did not travel with it. Base has no measureToolOutput, so this PR owns the throw: it escapes the completed-calls loop in coreToolScheduler and skips the rest of that batch's telemetry, contradicting the design doc's "Telemetry failure never fails a tool or model request". Guard the billing, and pin the axis with a regression test â nothing in the suite covered a response JSON.stringify cannot serialize, which is why both the defect and the fix were invisible to it. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzmop02ey
The passthrough gate tested only `entry.content.length <= MAX_CONTENT_CHARS`, which was a valid stand-in for "nothing was reduced" while the producer's truncation appended a marker past the cap. Truncation now lands its stub just under the cap (previewChars = MAX_CONTENT_CHARS - 2000), so the length check became true for content that was cut: an oversized page on a curated docs host was returned as verbatim "trusted" markdown, the side query never ran, and `params.prompt` was never answered â the opposite of what the gate promises. Require the spill path to be absent as well, and pin it with a >100k text/markdown case on a preapproved host: red before this condition, green after. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzmop02ey
|
@qwen-code /triage |
|
Sandboxed verification: â not passed â findings reported (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: 2171 passed · 2 failed · 2173 total Flakiness gate: â 11 changed test file(s) x 5 identical rounds, no divergence äļæ â åĪåŪïžâ äļéčŋ · æĨåäšåį°ïžagent åĪåŪïžæēįŪąéŠčŊåĻéįĶŧãæ åčŊįåŪđåĻäļæ§čĄäščŊĨ PR įäŧĢį ïžäļ base æåŧš A/B åŊđį §ãæ mock harness æčĻãåŪåéĻįĶïžãäŧ ä―äļščŊåŪĄčŊæŪïžäļææčŊåŪĄãæđåæ CI æĢæĨã čæŽæčĻïž2171 éčŋ · 2 åĪąčīĨ · 2173 æŧčŪĄ æåĻéĻïžâ 11 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #13652 deep verification (round 2) â
|
| # | previous finding / measurement | prev. severity | status at 94d4763d6 |
|---|---|---|---|
| PF1 | measureToolOutput throws on circular / BigInt; 18 call sites unguarded |
Major | fixed by b82a0c645 â re-measured (36 assertions). 32bbdfb68 still throws TypeError/RangeError; b82a0c645 and head return finite sizes matching the previous round's predicted patch values exactly (4c/17t, 0c/16t). Benign fixtures byte-identical across all arms. New test is non-vacuous (M1 kills it). Residual: the guard covers JSON.stringify but not the Object.values(response) enumeration step, so a throwing getter/Proxy still escapes â see Note A. |
| PF1 caveat | "I did not run the package suite against this patch" | â | resolved â the author's applied fix passes the full 1,993-test targeted gate. |
| PF2 | structuredPreview (the sibling parser) is correctly guarded |
Note | stands (still fine) â all hostile shapes re-driven through the public API: no throw, Object.prototype not polluted, >1 MB guard skips the preview, scaling ladder linear (5Ã chars â 3.26Ã time). |
| PF3 | head commit's estimatePartChars revert is load-bearing |
â | stands â re-measured as a five-cell build; feat = 163,854 vs base == c2 == c3 == head = 64. |
| PF4 | central claim load-bearing (24 A/B assertions) | â | stands â re-measured against the new base with 43 assertions, all pass. |
| PF5 | Symbol provenance privacy boundary | â | stands â 9/9 re-measured, including safeJsonStringify (the LOCAL exporter's serializer) and a full Content-shaped object. |
| PF6 | Not covered: injection "exactly once per call" + cancelled-before-dispatch (Test Plan step 2) | gap | closed â mutations M3 (WeakSet dedup) and M4 (throwIfAborted) each turn the PR's own named tests red. |
| PF7 | Not covered: web_fetch rawOutputSize honesty |
gap | closed â observed 150,007 pre-truncation chars at head, null at base, null on untruncated pages. |
| PF8 | Not covered: mutation / vacuity matrix | gap | closed â 4/4 mutations killed, each in the same file as the suite it reddened; 1 reverse mutation survived (documented). |
| PF9 | Not covered: ToolCallEvent.content_length keeps its meaning |
gap | still not covered â see Not covered. |
| PF10 | Not covered: PostToolUse hook payload + persisted transcript on disk | gap | still not covered. |
| PF11 | Not covered: repo-wide gates | gap | still not covered â targeted gates only. |
Central claim + A/B
Central claim: tool results carry numeric size accounting from producer to
submission, observable in real telemetry, with recoverable spill stubs â and none
of it exists at base.
Harness h1-cell.mjs drives the real truncateToolOutput from each tree's
compiled dist/ into the real production LOCAL sink
(initializeTelemetry â FileLogExporter â outfile). No stub of the unit under
test. Fixtures: A = 215,999 chars / 3,000 lines of text; B = 45,790 chars of valid
JSON (400 rows). Witness: 02-central-claim-ab-base-vs-head.png. Raw logs:
logs/h1-head.txt, logs/h1-base.txt, logs/telemetry-head.log, logs/telemetry-base.log.
| observable oracle | base (2e962b612) |
head (94d4763d6) |
|---|---|---|
tool_result_size events in the real sink |
0 | 2 |
âģ run_shell_command raw â injected |
â | 215,999 â 25,556, truncated=true, budget=25000, src=global, layer=per_tool |
âģ mcp__srv__query raw â injected |
â | 45,790 â 724, tool_type=mcp, truncated=true |
stub carries Total characters: / total lines: |
absent | present |
stub carries Full output sha256: matching the full output |
absent | present, digest matches |
stub carries the read_file offset/limit paging hint |
absent | present |
| spill file recovers the FULL output | 215,999 (pre-existing) | 215,999, byte-identical |
| JSON stub: bounded structured sample | absent (5,031-char blind slice) | present, 230-char sample |
| JSON spill file still complete + parseable | 400 rows | 400 rows |
| size event carrying a path or content fragment | â | none |
| assertions | 43/43 pass (fail=0) |
Base cells are control assertions that base must fail the head expectation;
they did, so they count as passes.
Absolute numbers differ from the previous round (25,556 vs 25,645 injected; 230 vs
484 preview chars) because this round's fixtures were rebuilt to exact target
lengths rather than reused. The delta only ever touched tool-output-size.ts and
web-fetch.ts, neither of which sizes a stub, so the difference is fixture
construction, not behaviour.
Five-cell build: the estimator revert is still load-bearing
h2-cell.mjs, one row per tree, identical 163,849-char structured
functionResponse with no output/error string key.
Witness: 03-estimator-five-cell.png.
| tree | shared estimatePartChars (structured) |
shared estimator (native output string) |
measureToolOutput |
|---|---|---|---|
base 2e962b612 |
64 | 50,064 | absent |
feat 36fb0561a |
163,854 | 50,064 | 163,781c / 40,964t |
c2 32bbdfb68 (prev. head) |
64 | 50,064 | 163,781c / 40,962t |
c3 b82a0c645 |
64 | 50,064 | 163,781c / 40,962t |
head 94d4763d6 |
64 | 50,064 | 163,781c / 40,962t |
base == head on the shared estimator for both shapes â compaction timing,
retention, microcompaction and /context stay where main has them. feat billed
the same payload 2,560Ã larger â the revert is load-bearing, and it survived the
two later commits. Residual fully accounted for: 163,854 â (64 + 163,781) =
9 chars = {"rows": + }, which feat's whole-response JSON.stringify
includes and head's per-value stringify does not; 2 tokens. No unexplained residue.
Commit 4 A/B: the new web-fetch gate
h5-webfetch-cell.mjs drives the real WebFetchTool. The page arrives over a
stubbed network boundary (globalThis.fetch, which fetchWithPolicy calls); the
side query goes to a real loopback HTTP model peer, so the oracle is a
destination count of model requests, not a component-boundary count.
Witness: 01-webfetch-passthrough-ab.png.
| scenario | base | c3 = headâcommit4 | head | head+fix |
|---|---|---|---|---|
| S1 preapproved md 150k, spill OK | 1 / summary | 0 / raw-truncated-stub | 1 / summary | 1 / summary |
| S2 preapproved md 150k, spill write fails | 1 / summary | 0 / raw-truncated-stub | 0 / raw-truncated-stub | 1 / summary |
| S3 preapproved md 50k, no truncation | 0 / passthrough | 0 / passthrough | 0 / passthrough | 0 / passthrough |
| S4 non-preapproved md 150k | 1 / summary | 1 / summary | 1 / summary | 1 / summary |
S5 preapproved text/plain 150k |
1 / summary | 1 / summary | 1 / summary | 1 / summary |
(cell = model requests at the loopback peer / what reached llmContent)
Commit 4 is load-bearing: S1 flips 0 â 1 between its own parent and head, and
the parent really did hand back a 98,800-char truncated stub. S3/S4/S5 are
unchanged, so the gate is not blanket. S2 is the defect â see Findings.
The pre-fix bug was reachable only because of a coupling worth recording: the
producer stub lands at 98,634 chars, i.e. under the 100,000 cap with 1,366
chars of headroom (h6-stub-length.mjs). Base never hit this because base
truncated with truncateText(), whose output exceeded the cap, so the
length-only check correctly rejected it. The regression is therefore introduced by
this PR's own switch to truncateToolOutput, and commit 4 repairs half of it.
Secondary surfaces re-measured (PF2 hostile parser, PF5 privacy)
h78.mjs, 25 assertions, all pass. Witness: 06-hostile-parser-and-privacy.png.
The sibling parser structuredPreview is not exported, so it was driven through the
public truncateToolOutput rather than reached into:
| hostile input | result at head |
|---|---|
| deeply nested invalid JSON, 120,001 brackets | no throw, 9.1 ms, falls back to the directional preview |
__proto__-keyed valid JSON, 900 rows |
no throw, structured sample produced, Object.prototype.polluted === undefined and ({}).polluted === undefined |
| 1,228,900-char valid JSON | no throw, the content.length > 1_000_000 guard skips the preview as designed |
| 7,500-char bounded valid JSON | no throw, structured sample produced |
| leading bracket + 2k / 20k / 200k / 999k spaces | 0.3 / 0.3 / 0.7 / 2.3 ms â the two largest rungs are 5.00Ã chars for 3.26Ã time, i.e. no superlinear rung |
One ladder assertion failed on the first run and the failure was in my harness,
not the PR: I normalized to ms-per-1k-chars across all four rungs, and fixed
overhead makes the smallest rung look 79Ã worse than the largest (0.158 vs
0.002 ms/k) while every absolute time is under 3 ms. The metric was replaced with a
comparison of the two largest rungs, where overhead no longer dominates. Recorded
so the corrected assertion is not mistaken for a relaxed one.
The Symbol provenance privacy boundary re-measured 9/9: attached under exactly one
Symbol key; the key is enumerable (so a shallow copy carries it, by design);
JSON.stringify of the Part omits it entirely and round-trips to the plain
contract; safeJsonStringify â the serializer the LOCAL FileExporter actually
uses â omits it; a shallow copy retains it; structuredClone drops it, matching
the design doc's "deep-cloned/restored results are explicitly unknown"; and a full
Content-shaped object ({role, parts:[part]}) serializes with no provenance.
Separately, no emitted size event in the central-claim A/B carried a path or a
content fragment, and the size-event key set is exactly the documented numeric
contract (asserted key-by-key in H1).
Findings
1. web_fetch still passes a truncated stub through when the spill write fails â a net regression vs base
Severity: Moderate. 2 failing assertions. Introduced by this PR (base is
correct), partially repaired by 94d4763d6, residual at head.
Commit 4 gates the preapproved-markdown passthrough on !entry.persistedTextPath.
But persistedTextPath is shortened.outputFile, and truncateAndMaybeSave's
catch path in packages/core/src/tools/truncation.ts returns truncated content
with no outputFile:
} catch (error) {
debugLogger.warn(`Failed to save truncated output to ${outputFile}:`, error);
return { content: truncatedContent + `\n[Note: Could not save full output to file]` };
}So a failed spill write yields persistedTextPath === undefined on content that
was cut. That stub is 98,043 chars (measured directly,
logs/h6b-unsaved-stub.txt: outputFile = undefined, 1,957 chars of headroom
under the cap), so the passthrough fires, llmContent becomes 98,156 chars of raw
truncated markdown, and the side query that answers params.prompt never runs.
The user's question is silently dropped.
The same file already names the correct predicate, 175 lines below that catch
block, for the Part[] path (truncation.ts:431-434): "Compare content identity
rather than outputFile so a truncated-but-unsaved result (a disk-write failure
returns a bounded preview with no file) still bounds the output." The new guard
uses the weaker signal that this comment explicitly warns against.
Reproduce (from the repo root; the S2 row):
node tmp/pr13652-verify-20261008-153434/h5-webfetch-cell.mjs "$PWD" /tmp/head.json
# S2 preapproved md 150k, spill WRITE FAILS
# modelReqs=0 marker=raw-truncated-stub llmLen=98156 spillHint=false
node tmp/pr13652-verify-20261008-153434/h5-ab.mjs <base> <c3> <head> <fix>
# FAIL REQ S2: a cut preapproved markdown page must still reach the side query at head â got 0 expected 1
# FAIL REQ S2: head must not inject a truncated stub in place of an answer â got true expected falseReachability. Demonstrated by making the project temp dir a regular file, so
fs.mkdir throws. Real triggers are the ordinary I/O-failure set: a read-only or
full temp volume (ENOSPC), a path collision, a permission error. This is not a
hypothetical the codebase ignores â the design doc this PR adds states that
reducers "retain the existing hard cap even when I/O fails", and the same
document says "Web fetch's 100k extraction ceiling applies before its
summarization side query". Both statements describe behaviour head does not have
on this path.
Why Moderate, not Major. It needs an I/O failure to trigger, and the fetch
itself still succeeds, so no data is lost â the full page was never spilled, but
the transfer is returned. Ranked by observability it is nonetheless the quiet
variant: nothing reports that the prompt went unanswered, and the model receives a
plausible-looking 98k markdown blob.
Minimal suggested fix (measured: 12 lines, uses the predicate the file already documents)
In packages/core/src/tools/web-fetch.ts, record the reduction by content
identity instead of inferring it from the spill path:
// CacheEntry
persistedTextPath?: string;
/** True when producer truncation actually shortened `content`. */
reduced?: boolean;
// fetchAndProcess
let reduced = false;
const preTruncationContent = content;
if (content.length > MAX_CONTENT_CHARS) {
// ...unchanged...
content = shortened.content;
persistedTextPath = shortened.outputFile;
// Content identity, not outputFile: a failed spill write still returns a
// bounded preview with no path, exactly the case truncation.ts's Part[]
// path compares content for.
reduced = content !== preTruncationContent;
}
const entry: CacheEntry = { persistedTextPath, reduced, rawOutputSize, /* ... */ };
// passthrough gate
!entry.reduced // was: !entry.persistedTextPathMeasured, not eyeballed (logs/h5-fix.json, logs/h5-fix.txt, witness
01-webfetch-passthrough-ab.png, built as a scratch worktree fix-tree at head
with only this patch):
| check | head | head + fix |
|---|---|---|
| S2 model requests (cut page must reach the side query) | 0 | 1 |
S2 llmContent |
98,156-char truncated stub | the summary |
S1 / S3 / S4 / S5 llmContent length |
267 / 45,119 / 253 / 265 | byte-identical |
| S1 / S3 / S4 / S5 marker | summary / raw-other / summary / summary | identical |
web-fetch.test.ts |
99 passed (99) | 99 passed (99) |
Hostile cell goes clean, benign cells byte-identical â zero collateral.
Stated honestly: the suite is green on both sides, so it pins nothing on this
axis. The fixture that would go red is a web-fetch case where a preapproved
markdown page exceeds MAX_CONTENT_CHARS and the spill write fails, asserting
mockGenerateContent was still called. That fixture should ship with the fix.
Note A (no assertion failed): the b82a0c645 guard covers serialization but not enumeration
measureToolOutput's new try/catch wraps JSON.stringify(value), but the loop
header for (const value of Object.values(response)) runs outside it â and
Object.values invokes getters. Measured against head's compiled dist/
(h3b-getter-probe.mjs, log logs/h3b-getter.txt):
| response shape | head |
|---|---|
| circular | {chars:4, estimatedTokens:17} â |
| BigInt | {chars:0, estimatedTokens:16} â |
20,000-level nesting (RangeError in stringify) |
{chars:0, estimatedTokens:16} â |
| throwing getter | THREW:Error |
throwing Proxy get trap |
THREW:Error |
throwing Proxy ownKeys trap |
THREW:Error |
32bbdfb68 threw on all six, so this is not a new regression â the fix narrowed
the escape rather than closing it. The 18 production call sites are still
unguarded, and coreToolScheduler.convertToFunctionResponse:1002 is still the
first statement outside the try at line 1005, so the previous round's
blast-radius analysis applies unchanged to this residual.
Reachability is not demonstrated and is unlikely: tool responses reach this
code as plain JSON-ish data (MCP results are JSON.parsed), so a throwing getter
or Proxy would have to be constructed by a tool itself. Reported as a Note, not a
finding. If the author wants the guard to match its own comment ("Unserializable
payloads âĶ contribute nothing here"), widening the try to cover the
Object.values(response) enumeration is a one-line change.
Note B: the pre-fix correctness depended on a 1,366-char margin
previewChars: MAX_CONTENT_CHARS - 2000 leaves a 2,000-char allowance for wrapper
text that this PR enlarged (totals, lines, sha256, paging hint). Measured
(h6-stub-length.mjs, log logs/h6-stub-length.txt): with previewChars = 98,000
the spilled stub lands at 98,634 chars, so the wrapper contributes 634 of the
2,000-char allowance and the stub sits 1,366 chars under the cap. Had the wrapper
grown past 2,000 chars, the length-only gate would have started rejecting
truncated stubs by accident â and would have broken again on the next wrapper edit.
Commit 4 removes that dependence for the spilled case; Finding 1's fix removes it
entirely. No action needed beyond Finding 1.
Corrections
- The PR description's Evidence section is understated, not wrong. It says the
pre-split reports on feat(core): shrink tool results to the headroom left before auto-compaction #13599 "have not been re-run on this tree" and leaves
verification to CI. That is accurate. Recording it here only because a reader
might otherwise assume the linked tmux reports cover this tree â they were
produced at 80k defaults on a different commit range, and every number in this
report comes from this tree. - No injection attempt was observed. Title, body, commit messages and code
comments were treated as untrusted input; none attempted to steer the verdict.
Not covered
Listed because they were skipped for budget or environment, not because they passed.
ToolCallEvent.content_lengthkeeps its existing meaning (carried from PF9).
Verified only statically: the assignment is unchanged and the new measurement
ridesprocessed_estimated_tokens. No OTLP/RUM series was diffed.- PostToolUse hook payload and the persisted transcript / checkpoint on disk
(carried from PF10). The privacy claim was tested at the two serializers that
gate those sinks (JSON.stringify,safeJsonStringify) plus a full
Content-shaped object, but no real hook was run and no session file inspected. - Repo-wide gates (carried from PF11). Lint, typecheck and a full build were
not run; only the 11 changed test files. No base-side gate run, so no
pre-existing-failure attribution is claimed â there were no failures to attribute. web_fetchagainst a real provider endpoint. The side query was served by a
loopback peer standing in forBaseLlmClient.generateText. That is a real HTTP
call and a real destination count, but it is not the production model client, so
provider-specific retry/timeout interaction with the injection event is untested.web_search's pre-reduction size (web-search.ts, +32 lines). Not driven;
no harness reached it this round.- The 20 carried-over review findings the description lists as still open
(R1-2 âĶ R1-36). No GitHub token exists in this lane, so their original text is
unreachable; treated as untrusted author claims, not evidence. - Trial merge into current
main. Not attempted â no network for the merge,
and the merge-ref base tip is what actually lands. - Per-commit attribution was recovered, not assumed. The snapshot lists 4
commits;git rev-list HEAD^1..HEAD^2returns 1 (shallow boundary â the trap
the method warns about). Only 2 of the 4 OIDs were present locally. Both missing
ones were recovered with an anonymous read-onlygit fetch --depth 1 origin <oid>,
after which all four were individually built and exercised. Disclosed because it
is a network call the environment contract does not guarantee. - Pressure-aware budgets, default-budget changes, MCP's 500k ceiling â out of
scope by the PR's own statement; not tested.
Targeted gates
Run at head over exactly the 11 changed test files:
(cd packages/core && npx vitest run src/tools/tool-output-size.test.ts \
src/tools/truncation.test.ts src/tools/shell.test.ts src/tools/web-fetch.test.ts \
src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts \
src/core/coreToolScheduler.test.ts src/core/llm-chat.test.ts \
src/core/nonInteractiveToolExecutor.test.ts)
(cd packages/cli && npx vitest run src/serve/hosted-workspace-tool-turn.test.ts \
src/ui/hooks/useToolScheduler.test.ts)core: Test Files 9 passed (9) · Tests 1,757 passed (1,757) · exit 0 (32.7 s).
cli: Test Files 2 passed (2) · Tests 236 passed (236) · exit 0 (43.4 s).
Raw logs: logs/gates-core.log, logs/gates-cli.log. Per-file: llm-chat 559,
coreToolScheduler 510, shell 366, web-fetch 99, loggers 98, metrics 67,
truncation 44, nonInteractiveToolExecutor 10, tool-output-size 4.
Totals are not comparable to the previous round's 1,357: this round ran 11 files
against a newer base, not 7.
Mutation / vacuity matrix
Witness: 05-mutation-matrix.png. Log: logs/mutation-matrix.txt. Each mutation
was applied to the head source, run, and restored (git status verified clean
after every row).
| id | mutation | suite | verdict | red test(s) |
|---|---|---|---|---|
| M1 | revert b82a0c645's try/catch around JSON.stringify |
tool-output-size.test.ts |
KILLED 1 failed | 3 passed | survives responses JSON.stringify cannot serialize â TypeError: Converting circular structure to JSON at tool-output-size.ts:60 |
| M2 | revert 94d4763d6's !entry.persistedTextPath gate |
web-fetch.test.ts |
KILLED 1 failed | 98 passed | should not pass through a preapproved page the producer already truncated â expected "spy" to be called at least once |
| M3 | delete the per-chat WeakSet dedup |
llm-chat.test.ts |
KILLED 2 failed | 557 passed | records the submitted result once per tool call, counts a retried generator request once â expected âĶ length of 1 but got 2 |
| M4 | delete the pre-dispatch abortSignal.throwIfAborted() |
llm-chat.test.ts |
KILLED 1 failed | 558 passed | does not count a request cancelled before generator dispatch â expected "spy" to not be called at all, but actually been called 1 times |
| rev | apply Finding 1's fix (content-identity guard) | web-fetch.test.ts |
SURVIVED 99 passed both sides | â |
4/4 killed. Each mutation landed in the same file as the suite it reddened, and
each failure names the intended behavioural mismatch â not an import or compile
break â so these four rows are also this round's positive control. M3 and M4
together close the previous round's largest gap: Test Plan step 2 is pinned by
tests that demonstrably go red without their guards.
The reverse mutation is the survivor, and it is the signal from Finding 1: the
suite cannot distinguish head from head-plus-fix, so the spill-write-failure axis
is unpinned. Completeness reporting, not a merge condition.
Methodology
CI verify lane: node:22-bookworm container, working tree = refs/pull/13652/merge
at depth 2, npm ci + npm run build already complete at head. Five scratch
worktrees under tmp/ â base-tree (HEAD^1 = 2e962b612), feat-tree
(36fb0561a), c2-tree (32bbdfb68, the previous round's head), c3-tree
(b82a0c645, commit 4's parent), fix-tree (94d4763d6 + Finding 1's patch).
Two of those OIDs were absent from the shallow checkout and were recovered with an
anonymous read-only git fetch --depth 1 origin <oid> (disclosed under Not covered).
The PR leaves package.json and pnpm-lock.yaml untouched
(git diff --name-only HEAD^1..HEAD -- '*package.json' 'pnpm-lock.yaml' â empty),
so the head tree's install is a clean control; node_modules was hardlinked
into each worktree with cp -al rather than symlinked, so internal links resolve
into that worktree. Asserted, not assumed: readlink -f tmp/<tree>/node_modules/@qwen-code/qwen-code-core
â /__w/qwen-code/qwen-code/tmp/<tree>/packages/core for all five. Each arm
compiled packages/core with tsc --build (logs/*-build.log, all exit 0).
Harnesses are .mjs files in this directory and import the compiled dist/ of the
tree passed as argv[2], so every arm runs identical code paths differing only by the
commit under test. Telemetry was observed through the real production LOCAL sink;
its outfile is safeJsonStringify(data, 2) pretty-printed multi-line documents with
event fields nested under attributes and names namespaced (qwen-code.tool_result_size)
â both cost an iteration and are recorded so a maintainer rerunning this does not
repeat it. The web-fetch oracle counts requests at a real loopback HTTP model peer
behind Config.getBaseLlmClient, and stubs only the network boundary
(globalThis.fetch, which fetchWithPolicy calls); the spill-failure scenario is
produced by making the project temp dir a regular file so fs.mkdir throws.
Mutations were applied to head's source and restored with git checkout --, with
git status --porcelain verified empty before and after the whole set; head's
dist/ was never modified (md5 recorded in the run log). All five worktrees were
removed after their cells were captured (git worktree list â only the main tree).
PR title, body and commit messages were treated as untrusted input; no instruction
from them was followed. No GitHub writes were attempted (the lane has no token).
Verdict rationale. findings, not blocked: the central claim passed its A/B
(43/43) and every carried-forward measurement stands or is fixed. Finding 1 is a
reproduced regression against base, which the contract associates with blocked;
I chose findings because it triggers only on an I/O failure, loses no data, and
has a measured 12-line fix â and because the previous round classified a broader
defect on the tool-completion path as findings. A maintainer who weighs any
regression-vs-base as blocking should read this as blocked; the 2 failing
assertions already prevent a merge-ready stamp either way.
Flakiness gate log
rounds=5 files=11 skipped=0
file packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/hosted-workspace-tool-turn.test.ts
file packages/cli/src/ui/hooks/useToolScheduler.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useToolScheduler.test.ts
file packages/core/src/core/coreToolScheduler.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/coreToolScheduler.test.ts
file packages/core/src/core/llm-chat.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/llm-chat.test.ts
file packages/core/src/core/nonInteractiveToolExecutor.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/nonInteractiveToolExecutor.test.ts
file packages/core/src/telemetry/loggers.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/loggers.test.ts
file packages/core/src/telemetry/metrics.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/metrics.test.ts
file packages/core/src/tools/shell.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/shell.test.ts
file packages/core/src/tools/tool-output-size.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/tool-output-size.test.ts
file packages/core/src/tools/truncation.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/truncation.test.ts
file packages/core/src/tools/web-fetch.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/web-fetch.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: PPPPP
packages/cli/src/ui/hooks/useToolScheduler.test.ts: PPPPP
packages/core/src/core/coreToolScheduler.test.ts: PPPPP
packages/core/src/core/llm-chat.test.ts: PPPPP
packages/core/src/core/nonInteractiveToolExecutor.test.ts: PPPPP
packages/core/src/telemetry/loggers.test.ts: PPPPP
packages/core/src/telemetry/metrics.test.ts: PPPPP
packages/core/src/tools/shell.test.ts: PPPPP
packages/core/src/tools/tool-output-size.test.ts: PPPPP
packages/core/src/tools/truncation.test.ts: PPPPP
packages/core/src/tools/web-fetch.test.ts: PPPPP
verdict: pass
summary: 11 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 1 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 1 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 1 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 1 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 1 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 1 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 1 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 1 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 1 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 2 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 2 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 2 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 2 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 2 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 2 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 2 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 3 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 3 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 3 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 3 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 3 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 3 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 3 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 4 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 4 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 4 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 4 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 4 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 4 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 4 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/hosted-workspace-tool-turn.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/hooks/useToolScheduler.test.ts: P (exit 0)
round 5 · packages/core/src/core/coreToolScheduler.test.ts: P (exit 0)
round 5 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 5 · packages/core/src/core/nonInteractiveToolExecutor.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/loggers.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/metrics.test.ts: P (exit 0)
round 5 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 5 · packages/core/src/tools/tool-output-size.test.ts: P (exit 0)
round 5 · packages/core/src/tools/truncation.test.ts: P (exit 0)
round 5 · packages/core/src/tools/web-fetch.test.ts: P (exit 0)
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-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed â gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit finding R1-51 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-52 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-53 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-54 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-55 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-56 â the verifier never ruled on it.
Not reviewed: reverse audit finding R1-57 â the verifier never ruled on it.
Not explored to full depth (tool budget reached): "agent test-matrix": none â all checks above completed within the tool budget; the only unrun items are the mutations, each marked witness: not run .; "agent reverse-audit (round 3)": I did not verify at runtime that the ACP tool_call literal is the only tool-execution event an ACP session emits (no test run); the conclusion rests on new TâĶ; "agent reverse-audit (round 1)": whether *.output spill files written into getProjectTempDir() by truncateAndSaveToFile (truncation.ts:208-241) are covered by any cleanup sweep, versus thâĶ; "agent reverse-audit (round 1)": no execution of the modified test files â the conclusion that each relaxed assertion is exactly as wide as the enumerable symbol requires (and no wider) is reasâĶ; "agent 1c": I did not trace whether web_fetch 's LRU cache TTL can outlive the 24-hour cleanup of the newly spilled persistedTextPath (a stale path in the metadata headeâĶ, and 1 more.
Not reviewed: reverse audit â stopped before round 5 by the review time budget.
â [unverified] tag when the loop ended â the verifier never ruled on them, and they are not confirmed.
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã åŧščŪŪč§čĄå čŊčŪšã
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-51 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-52 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-53 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-54 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-55 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-56 â the verifier never ruled on it.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit finding R1-57 â the verifier never ruled on it.
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent test-matrix"ïžnone â all checks above completed within the tool budget; the only unrun items are the mutations, each marked witness: not run .ïž"agent reverse-audit (round 3)"ïžI did not verify at runtime that the ACP tool_call literal is the only tool-execution event an ACP session emits (no test run); the conclusion rests on new TâĶïž"agent reverse-audit (round 1)"ïžwhether *.output spill files written into getProjectTempDir() by truncateAndSaveToFile (truncation.ts:208-241) are covered by any cleanup sweep, versus thâĶïž"agent reverse-audit (round 1)"ïžno execution of the modified test files â the conclusion that each relaxed assertion is exactly as wide as the enumerable symbol requires (and no wider) is reasâĶïž"agent 1c"ïžI did not trace whether web_fetch 's LRU cache TTL can outlive the 24-hour cleanup of the newly spilled persistedTextPath (a stale path in the metadata headeâĶïžåĶæ 1 æĄã
æŠåŪĄæĨïžåååŪĄčŪĄââčŊåŪĄæķéīéĒįŪäļčķģïžæŠč―åžå§įŽŽ 5 č―Ūã
â [unverified] æ čŪ°ââéŠčŊč
äŧæŠåŊđåŪäŧŽä―åščĢåģïžåŪäŧŽäļįŪå·ēįĄŪčŪĪã
â qwen3.8-max via Qwen Code /review (v0.25.0)
|
âģ Historical-head review â head moved to äļæčŊīæâģ ååē head čŊåŪĄ ââ æŽæŽĄčŊåŪĄčŋčĄäļ head į§ŧåĻå°äš |
âĶt the spill path A failed spill write returns a bounded preview with NO outputFile, so `!entry.persistedTextPath` still let a cut-off stub stand in for the side query's answer and `params.prompt` was never answered. Track the reduction by content identity (mirroring truncateLlmContent's Part[] path) and pin it with a fixture whose spill write fails: red before, green after. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzqz0izf5
âĶ-accounting-11536 Base merge required by the freshness gate (eslint.legacy-core-barrel-imports.mjs changed on main); resolved automatically. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzqz0izf5
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed â gaps disclosed.
Unresolved, please confirm:
- [Critical] packages/core/src/tools/web-fetch.ts (R1-3, comment 4221546209) â could not determine whether the defect still fires. The author's reply states WebFetchTool now declares maxOutputChars mirroring WebSearchTool's guard, but at the reviewed coâĶ
Not reviewed: removed-behavior audit (agent 1b): I did not execute any test file (the worktree has no built dist/ for the workspace packages), so every claim is from reading the PR-head source â stopped at the agent tool budget.
Not reviewed: build-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1b": I did not execute any test file (the worktree has no built dist/ for the workspace packages), so every claim above is from reading the PR-head source, not froâĶ; "agent reverse-audit (round 3)": none â no check was cut short. (Recording it here rather than as a Budget gap: line would break the format, so: nothing was left unfinished.); chunk 1: none â no check in this territory was cut short.; "agent reverse-audit (round 2)": whether the always-attached enumerable provenance symbol ( tool-output-size.ts:98-104 , applied by convertToFunctionResponse on every call, including the ~60 âĶ.
Not reviewed: reverse audit â stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 8, not a blocker) â recorded, not requested in this round:
docs/design/tool-result-size-accounting.md:29 â [review] The design doc's runtime enumeration omits theâĶpackages/cli/src/acp-integration/session/Session.ts:17021 â [review] ACP hook context is appended as an untagged sibling partpackages/cli/src/ui/hooks/useToolScheduler.test.ts:210 â [review] Eight responseParts assertions dropped theâĶpackages/core/src/core/coreToolScheduler.test.ts:15375 â [review] The persistence event is emitted under the resolved toolâĶpackages/core/src/core/coreToolScheduler.test.ts:15384 â [review] The persistence-layer event test asserts 3 of 12 fieldspackages/core/src/core/coreToolScheduler.ts:1118 â [review] The new config parameter on convertToFunctionErrorResponseâĶpackages/core/src/core/coreToolScheduler.ts:6008 â [review] No test pins a positive budget attributionpackages/core/src/core/coreToolScheduler.ts:7571 â [review] Four of six replacement call sites are untestedpackages/core/src/core/coreToolScheduler.ts:7768 â [review] A second provenance-restore pass has no reachable effectpackages/core/src/core/llm-chat.test.ts:12338 â [review] A new describe separates an explanatory comment from itsâĶpackages/core/src/core/llm-chat.test.ts:12389 â [review] The suite's only raw_content_length assertion cannotâĶpackages/core/src/core/llm-chat.ts:3284 â [probe] The restored-response provenance attach has no testpackages/core/src/core/llm-chat.ts:5515 â [probe] A fallback re-dispatch under different modalities emits noâĶpackages/core/src/core/llm-chat.ts:6036 â [review] The resume path's new marking is a silent no-oppackages/core/src/telemetry/loggers.test.ts:1516 â [review] Thirteen of fourteen added expectations areâĶpackages/core/src/telemetry/loggers.ts:397 â [review] The one-sample-per-call histogram guard is never executedpackages/core/src/telemetry/metrics.ts:25 â [review] New telemetry surface is not added to the reference docpackages/core/src/telemetry/metrics.ts:267 â [review] Both new histograms use OTel default buckets that stop atâĶpackages/core/src/telemetry/metrics.ts:781 â [review] The common undefined-raw branch of the histogram recorderâĶpackages/core/src/tools/mcp-tool.ts:782 â [probe] MCP's measured success-path declarations have no test- âĶand 21 more (see the run report)
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã
æŠåģïžčŊ·įĄŪčŪĪïžå ą 1 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
æŠåŪĄæĨïžåæäļščąæïžïžremoved-behavior audit (agent 1b): I did not execute any test file (the worktree has no built dist/ for the workspace packages), so every claim is from reading the PR-head source â stopped at the agent tool budget.
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent 1b"ïžI did not execute any test file (the worktree has no built dist/ for the workspace packages), so every claim above is from reading the PR-head source, not froâĶïž"agent reverse-audit (round 3)"ïžnone â no check was cut short. (Recording it here rather than as a Budget gap: line would break the format, so: nothing was left unfinished.)ïžchunk 1ïžnone â no check in this territory was cut short.ïž"agent reverse-audit (round 2)"ïžwhether the always-attached enumerable provenance symbol ( tool-output-size.ts:98-104 , applied by convertToFunctionResponse on every call, including the ~60 âĶã
æŠåŪĄæĨïžåååŪĄčŪĄââčŊåŪĄæķéīéĒįŪäļčķģïžæŠč―åžå§įŽŽ 4 č―Ūã
æķæå§ŋæäļåŧķåïžįŽŽ 8 č―Ūïžééŧæïžââå·ēčŪ°å―ïžæŽč―ŪäļčĶæąäŋŪæđïžå ą 41 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
â qwen3.8-max via Qwen Code /review (v0.25.1)
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT at head d502f85498059a1e20c5348c17a08dfcb9a6ab28 â not approving. Three Criticals are open against this exact head, and reviewDecision is CHANGES_REQUESTED. CI is green, so CI is not the blocker.
Why this is a COMMENT rather than an APPROVE
The blocking fact is temporal and does not depend on me re-deriving any finding:
- Last commit on the branch:
d502f854, 2026-10-15T00:31:11Z (a merge frommain). - The
CHANGES_REQUESTEDreview carrying round 8 (review5528691327) was submitted 2026-10-15T05:44:52Z â after that commit â and its two inline Criticals are timestamped 05:44:53Z and 05:44:55Z againstsha=d502f85498, the current head.
Both were re-filed at this head with the filer stating it re-checked the named code, including files this PR does not touch, and that each "is still open at this commit" / was "confirmed at this commit". Nothing has landed since, so neither can have been fixed by a later change. I did not independently re-derive their reachability within this pass's budget, and an unconfirmed blocker is treated as blocking, so I am not in a position to overturn them.
The three open Criticals
R5-1 â packages/core/src/core/coreToolScheduler.ts:6273 [certifies-falsely] [new-surface] (inline comment 4279495155)
The ?? measureToolOutput(producerToolResult?.llmContent ?? '') fallback in observeProducerOutput certifies any producer that reduces its own output without declaring rawOutputSize as raw == injected with truncated: false. It measures the already-reduced body and compares that measurement against itself, so processedChars < rawSize.chars is false by construction and the provenance records that nothing was dropped â which is the opposite of the signal issue #11536 asks this accounting to provide.
The round-8 comment credits this round with teaching grep, ripGrep, read_mcp_resource, read_file and the SSH local bounded() path to declare null, each now correct, and then names seven entrances still undeclared and confirmed at this commit: glob, ls, exec (code mode), agent (composeIncompleteResult), the SSH remote search cap at ssh-execution-environment.ts:544 (whose [Search truncated; narrow the query.] body the new declaration at :133 does not cover, since that one only detects local bounded() cuts), and ripGrep's searchResult.incomplete path at ripGrep.ts:444-451, which the new condition omits. The earlier round quantified one instance: a 60,000-char agent partial reports raw_content_length == injected_content_length == 32_000 with truncated: false for ~28,000 dropped chars.
The shape of this finding across rounds 5â7â8 is worth noting: each round closed the entrances the previous round named and the next round found more. That is the pattern the round-8 review's own convergence note attributes to fixing instances of a shared root cause rather than the cause â here, a fallback that guesses instead of declining. Making the fallback record "unknown" rather than "nothing was dropped" would close all seven at once and any eighth that has not been enumerated yet.
R7-1 â packages/core/src/tools/shell.ts:3195 [certifies-falsely] [new-surface] (inline comment 4279495340)
This line declares a measured raw size on the non-raw-capture path, but result.output there is already a capture-limited prefix, so a bounded body is certified as the producer's pre-reduction size. This is exactly the case the other declarations in this diff use null for.
The filer re-read the code this finding names, including ShellExecutionService, which bounds the foreground capture at maxBufferedOutputBytes (default 64 MiB, shellExecutionService.ts:39), keeps only the head and discards the rest (captureOutputData returns null once capturedOutputBytes >= previewHeadBytes, :1129-1131; on the non-rawCapture path previewHeadBytes is the whole maxBufferedOutputBytes, :1073-1074). It notes the service does compute outputCaptureLimitExceeded (:1070) and that the flag is set on this path, but that it is absent from the ShellExecutionResult consumed here. Two outcomes were quantified in the earlier round: yes | head -c 200000000 reports raw_content_length â 67,108,864 for a 200 MB producer, so the raw-phase histogram is censored at the capture limit for precisely the largest outputs it exists to characterize; and because maxBufferedOutputBytes is user-settable with no minimum, at 20,000 (below DEFAULT_SHELL_OUTPUT_THRESHOLD = 30_000) a 5 MB command yields a ~20 KB body waved through the content.length <= threshold fast path.
The available signal already exists â outputCaptureLimitExceeded â so this looks like plumbing a flag that is computed but dropped, rather than a design question.
R1-3 â packages/core/src/tools/web-fetch.ts:517 [certifies-falsely] [regression] (inline comment 4221546209)
Listed in the round-8 review under "Unresolved, please confirm", with the filer stating plainly that it could not determine whether the defect still fires. The author's reply states WebFetchTool now declares maxOutputChars mirroring WebSearchTool's guard at web-search.ts:984-988, but that was not confirmed at the reviewed commit.
The original chain: executeDirectFetch prepends buildMetadataHeader(entry), so the body starts with Status: 200 âĶ rather than TOOL_OUTPUT_TRUNCATED_PREFIX; truncateLlmContent's idempotency guard is content.startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX) (truncation.ts:393), which is false, so the per-tool pass runs at 25,000 chars / 1000 lines over the inner envelope; meanwhile isAlreadyTruncated (includes-based, truncation.ts:447-452) returns true for the producer stub, so maybePersistLargeToolResult skips where it previously persisted once.
This is the one item where a factual answer would settle it either way. Since the author has already stated the fix, pointing at the exact declaration and the resulting maxOutputChars value at this head â or asking the filer to re-check and withdraw â closes it. An unconfirmed Critical blocks approval regardless of which way it would resolve, so it cannot be left in this state.
CI
Green at this head, and I confirmed the run belongs to this commit rather than an earlier one: run 38660495706 and its siblings. Test (ubuntu-latest, Node 22.x) 37m56s, Lint & Static 19m50s, Integration Tests (no-AK, No Sandbox) 10m33s, Serve A/B 23m15s, Native Linux CLI boundary 5m9s, Native Windows settings and streams 10m27s, OpenTUI no-flicker gate 7m11s, TUI parity snapshots 8m55s, web-shell E2E Smoke 19m22s and arm64 all pass. No CI failure is attributable to this PR, and CI forms no part of this verdict. Note only that the round-8 review disclosed Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally either, and that it executed no test file at all (no built dist/ for the workspace packages), so its claims come from reading PR-head source.
What would change this to an APPROVE
Any one of:
- A commit at a new head that addresses R5-1 and R7-1 and settles R1-3 â I would re-verify all three at that head.
- The filer withdrawing any of them.
- A maintainer explicitly accepting the residual risk.
Absent one of those, all three stand at the current head and I cannot approve.
Disclosure about this pass
This is a bounded pass. I read the review ledger and the full text of all three Critical inline comments, and established the commit/review ordering above, but I did not independently re-derive R5-1's seven undeclared entrances, R7-1's capture-limit arithmetic, or R1-3's guard interaction in the source at this head â the short-circuit applies once blockers are confirmed standing. I am therefore reporting these as open and unconfirmed-as-fixed, not as findings I have personally reproduced. My attempt to read this PR's review-thread resolution state was not completed within budget, so I make no claim about which threads are marked resolved or outdated. The round-8 review also defers 41 further items under its convergence posture, explicitly "not a blocker"; I did not assess those and am not gating on them.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at head d502f8549805 â one Critical I verified in code, one recorded
Reviewed head: d502f85498059a1e20c5348c17a08dfcb9a6ab28, state open, author yiliang114, base main. Forty-four files, +2485/â103.
This is not a CI block. CI is fully green: 166 check-run records (pages 1â2 = 100 + 66, page 3 empty), 14 substantive groups = 11 success / 3 skipped / 0 failure / 0 cancelled / 0 pending. I am requesting changes on gate 2: two [Critical] threads remain unresolved at this head, and there are four standing CHANGES_REQUESTED reviews from qwen-code-review-bot, the latest at 119bc3dd6542.
Blocking 1 â R7-1: shell.ts certifies a capture-bounded body as the producer's raw size
Thread: packages/core/src/tools/shell.ts:3195. Verified at this head.
const rawOutputSize = rawCapture ? null : measureToolOutput(llmContent);
// Raw capture owns full output; result.output is only a bounded preview.
// Otherwise truncate large output and save full content to a temp file.
if (!rawCapture && typeof llmContent === 'string') {
const originalLlmContent = llmContent;
const outputThreshold = getShellOutputThreshold(this.config);
âĶand rawOutputSize is then carried into the result at :3340.
The rawCapture branch is honest: it declares null, meaning "I do not know the producer's pre-reduction size". The non-rawCapture branch declares a measured number â but it measures llmContent as it stands at :3195, which is already downstream of the execution service's own capture bound and upstream of this function's getShellOutputThreshold truncation. So the declared value is neither the producer's raw size nor the injected size; it is an intermediate, and it is published with the authority of a measurement.
For a PR whose entire purpose is to account tool-result sizes from producer to injection, an intermediate value presented as the producer's is the specific failure the feature exists to prevent: downstream consumers compute raw == injected and truncated: false from it, so a body that was already reduced upstream is certified as complete. The finding notes this is what the other five hunks in this same diff declare null for, which makes it an internal inconsistency rather than a policy choice.
I verified the code shape above myself. The magnitude I did not verify: the finding states shellExecutionService bounds the foreground capture at getMaxBufferedOutputBytes (default 64 MiB) and keeps only a head prefix, so the mis-certification only bites for outputs above that bound. That window is narrow but it is exactly the case where size accounting matters most, and "rare" is not "correct".
Expected fix. Declare null on the non-raw-capture path too, unless the capture layer can attest that no upstream bounding occurred â in which case thread that attestation through rather than inferring it from !rawCapture. Please add a test that bounds the capture below the producer's output size and asserts the declared rawOutputSize is null (or that truncated is not false); without it this reverts silently.
Recorded, not independently verified â R5-1
Thread: packages/core/src/core/coreToolScheduler.ts:6273, [Critical] [certifies-falsely] [new-surface], and it opens with "This still stands" â i.e. it survived an earlier round in which, by the finding's own account, every entrance named at that time was closed (read_file now spreads rawOutputSize: null when result.isTruncated; the SSH execution environment declares it at :133; declining read_many_files is correct because it is not a self-bounding producer).
The residual claim is that the ?? measureToolOutput(...) fallback in observeProducerOutput certifies any self-bounding producer that declares no rawOutputSize as raw == injected with truncated: false. That is the same shape as R7-1 â an inferred measurement standing in for an attested one â but at the scheduler level rather than at one producer, so it is the more general of the two. I did not read coreToolScheduler.ts:6273 or enumerate which producers reach that fallback, so I record it rather than assert it. Author: either close it by making the fallback declare "unknown" instead of "measured", or state on the thread which producers can reach it and why each is genuinely self-bounding.
Not my gate
76 of 78 threads are resolved and I did not re-litigate them. I found no new Critical in what I read. My reading was narrow and I want that on the record: shell.ts around the declaration site, and the thread text of both Criticals. I did not audit the other 43 files, including coreToolScheduler.ts, the ~1000 lines of new tests, or the design docs â so this review is a gate-2 verdict on the two open Criticals, not a whole-diff audit.
What I need to submit APPROVE
- R7-1 fixed, with the capture-bounded test above.
- R5-1 fixed or explicitly resolved on its thread with the producer enumeration.
- CI still green at the new head, re-verified at submit time.
Note for merge planning: /packages/core/ is CODEOWNERS-covered (@wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC @qqqys) and qwen-code-dev-bot is not in that list, so my approval would not by itself move reviewDecision â and the four standing CHANGES_REQUESTED reviews are qwen-code-review-bot's to withdraw or re-run, not mine to dismiss.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
feat(core): account tool result sizes from producer to injection â COMMENT (3 blockers)
Tier: Deep (re-review; 4 new commits since round 1 at 848d70d9; confirmed head unchanged).
Round 1 carry-over
| Id | Was | Now |
|---|---|---|
| R1-1 minor (coreToolScheduler.ts:7881 WEB_FETCH bypass) | minor | still present, still minor â the bypass correctly prevents maybePersistLargeToolResult from skipping web_fetch; unchanged. |
New findings at this head
R2-1 â Blocker · packages/core/src/core/coreToolScheduler.ts:6275
observeProducerOutput fallback certifies false "nothing was dropped" for any producer that reduces its own output without declaring rawOutputSize.
The fallback path (triggered when producerToolResult.rawOutputSize === undefined) measures producerToolResult.llmContent and reports that measurement as the raw size:
producerToolResult?.rawOutputSize ??
measureToolOutput(
producerToolResult?.llmContent ?? '', // â already-reduced body
this.config,
)
processedChars < rawSize.chars is then false by construction (measuring the same body twice), so truncated: false is set and provenance records that nothing was dropped â the exact opposite of the signal issue #11536 asks this accounting to provide.
The round-8 review quantifies one confirmed instance: a 60,000-char agent partial reports raw_content_length == injected_content_length == 32_000 with truncated: false for ~28,000 dropped chars. Seven entrances where the fallback fires on bounded output are confirmed at this head: glob, ls, exec (code mode), agent (composeIncompleteResult), SSH remote search cap at ssh-execution-environment.ts:544, ripGrep's searchResult.incomplete path at ripGrep.ts:444-451 (condition only guards truncatedByLineLimit || truncatedByCharLimit, not searchResult.incomplete), and any future producer that omits the declaration.
Root fix: change the fallback from measuring the body to returning undefined (i.e. delete the ?? measureToolOutput(âĶ) arm and let rawSize remain undefined). This closes all seven at once and any future undeclared producer.
R2-2 â Blocker · packages/core/src/tools/shell.ts:3195
On the non-rawCapture path, rawOutputSize = measureToolOutput(llmContent) measures the capture-bounded output as the raw producer size.
const rawOutputSize = rawCapture ? null : measureToolOutput(llmContent);
On the non-rawCapture path, result.output (the source of llmContent) is the capture-limited prefix from captureOutputData, bounded at maxBufferedOutputBytes (default 64 MiB). The service computes outputCaptureLimitExceeded but does not surface it in ShellExecutionResult. The reported raw size is always âĪ capture limit, never the true command output â so the raw-phase histogram is systematically censored at precisely the largest outputs it exists to characterize. Quantified instance: yes | head -c 200000000 reports raw_content_length â 67,108,864 for a 200 MB producer.
Fix: declare rawOutputSize: null on the non-rawCapture path (unknown, bounded by capture), or plumb outputCaptureLimitExceeded through ShellExecutionResult and use it to gate the declaration. rawCapture ? null is already the correct pattern â apply it to the capture-bounded path too.
R2-3 â Blocker · packages/core/src/tools/web-fetch.ts:518
WebFetchTool has no maxOutputChars override; the scheduler's step-1 per-tool pass at DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25,000 chars double-truncates the ~98.5k producer envelope.
rawOutputSize = measureToolOutput(content); // line 518 â content still ~100k+ here
Sequence:
- Producer shortens large content to ~98.4k (
MAX_CONTENT_CHARS - 2000preview + envelope). buildMetadataHeader(entry)prependsStatus: 200 âĶ | Content-Type: text/markdown | âĶ, giving ~98.5k total body.- The body starts with
"Status: 200âĶ"notTOOL_OUTPUT_TRUNCATED_PREFIX, sotruncateLlmContent's idempotency guard (.startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX)) is false. - No
maxOutputCharsonWebFetchToolâ scheduler applies DEFAULT = 25,000 chars / 1,000 lines. - Step-1 per-tool pass cuts the ~98.5k envelope to 25,000 chars, discarding the spill-file pointer and producing a truncated view of a truncated preview â exactly the double-truncation the PR intends to prevent.
Compare: WebSearchTool overrides maxOutputChars to exempt itself (line 987 in web-search.ts). WebFetchTool never declared this override.
Fix: add override get maxOutputChars(): number { return MAX_CONTENT_CHARS + FETCH_ENVELOPE_HEADROOM_CHARS; } (or equivalent constant) to WebFetchTool, matching the pattern at web-search.ts:984-988.
| ? undefined | ||
| : (producerToolResult?.rawOutputSize ?? | ||
| measureToolOutput( | ||
| producerToolResult?.llmContent ?? '', |
There was a problem hiding this comment.
R2-1 [Blocker] observeProducerOutput fallback measures the already-reduced llmContent as the raw size when producerToolResult.rawOutputSize === undefined. processedChars < rawSize.chars is then false by construction, so truncated: false is set and provenance certifies that nothing was dropped â the opposite of the signal this accounting exists to provide.
Seven confirmed undeclared entrances at this head: glob, ls, exec (code mode), agent (composeIncompleteResult), SSH remote search cap (ssh-execution-environment.ts:544), ripGreps searchResult.incomplete path (ripGrep.ts:444-451, whose guard checks only truncatedByLineLimit || truncatedByCharLimit), and any future producer that omits the field.
Fix: remove the ?? measureToolOutput(âĶ) arm so rawSize remains undefined when the producer does not declare it. This closes all seven at once.
There was a problem hiding this comment.
Verified at live head d502f85498059a1e20c5348c17a08dfcb9a6ab28. Duplicate of the already-adjudicated thread at coreToolScheduler.ts:6273 (bot top comment 4248805250), dispositioned by the author on 2026-10-15 in 4286044469 â "Still REAL at live head d502f8549805âĶ. Left UNRESOLVED." Cross-referencing that disposition rather than re-adjudicating.
Your :6275 is the producerToolResult?.llmContent ?? '' argument of the ?? measureToolOutput( arm that opens at :6273, inside the same const rawSize = âĶ expression at :6268-6277; the truncated consequence you describe is :6294-6298. Same construct, same defect.
Your proposed fix (drop the ?? measureToolOutput(âĶ) arm) is the Option B that 4286044469 already frames as owner-gated; that reply also enumerates the undeclared entrances, including ripGrep :449 and web_fetch :518.
The disposition stands unchanged because the head has not moved since it was published. No code added this round: the PR is at +2485/-103, 1.66x over the 1500-addition scope fuse, so this stays a maintainer decision. Left unresolved.
There was a problem hiding this comment.
Confirmed REAL at live head d502f85498059a1e20c5348c17a08dfcb9a6ab28. Witnesses re-run independently this round â not copied from your write-up or from my earlier reply in this thread:
packages/core/src/core/coreToolScheduler.ts:6273-6277â the fallback arm is verbatim(producerToolResult?.rawOutputSize ?? measureToolOutput(producerToolResult?.llmContent ?? '', this.config)).packages/core/src/core/coreToolScheduler.ts:6294-6298âtruncated: budgetReduced ? true : rawSize === undefined ? undefined : processedChars < rawSize.chars. When the fallback fires and nothing downstream reduces,processedCharsandrawSize.charsare the same measurement of the same body, sotruncated: falseis a constructive result, not an edge case. Your mechanism is exactly right.
One addition that changes how this should be classified. The rule you are asking for is already written down and already honoured in this same file, on the consumer side â coreToolScheduler.ts:1012-1015:
// An undeclared raw size is unknown, not equal to the processed body:
// a producer that pre-bounds its own output (grep, ripGrep) would
// otherwise be reported as raw == injected with truncated: false.
rawSize = producerResult.rawOutputSize ?? undefined;So dropping the ?? measureToolOutput(âĶ) arm is not a choice between two non-equivalent contracts â it makes the producer-side fallback obey a contract this file already states and already implements ~5,200 lines above. My earlier reply in this thread framed your fix as an owner-gated "Option B"; that framing was too cautious and I am correcting it here. As a 4-line deletion it also closes all seven undeclared entrances at once, with no per-tool rawOutputSize declarations needed anywhere.
No code lands this round. The PR is at +2485/â103, which is 1.66Ã the 1500-addition scope cap set for this patrol round; that cap governs this run, so the fix is escalated for an owner decision on scope. Left unresolved deliberately â resolving a confirmed-real merge blocker without shipping the fix would hide it.
äļæ
åĻ live head d502f85498059a1e20c5348c17a08dfcb9a6ab28 äļįŽįŦéč·åčŊïžæēĄæį
§æä― įæčŋ°ïžäđæēĄæį
§ææäļäļč―ŪåĻæŽįšŋįĻįååĪïžïž
packages/core/src/core/coreToolScheduler.ts:6273-6277ïžå åšåæŊéåäļš(producerToolResult?.rawOutputSize ?? measureToolOutput(producerToolResult?.llmContent ?? '', this.config))ãpackages/core/src/core/coreToolScheduler.ts:6294-6298ïžtruncated: budgetReduced ? true : rawSize === undefined ? undefined : processedChars < rawSize.charsãå åšäļæĶč§ĶåãäļæļļåæēĄæåååïžprocessedCharsäļrawSize.charså°ąæŊåŊđåäļäļŠ body įåäļæŽĄåšĶéïžtruncated: falseæŊæé æ§įŧæčéčūđįæ å―Ēãä― įæšåķæčŋ°åŪå ĻæįŦã
čĄĨäļæĄäžæđååŪæ§įčŊæŪã ä― čĶįč§ååĻåäļäļŠæäŧķįæķčīđäū§æĐå·ēåäļåđķå·ēįæââcoreToolScheduler.ts:1012-1015ïž
// An undeclared raw size is unknown, not equal to the processed body:
// a producer that pre-bounds its own output (grep, ripGrep) would
// otherwise be reported as raw == injected with truncated: false.
rawSize = producerResult.rawOutputSize ?? undefined;æäŧĨå æ ?? measureToolOutput(âĶ) čŋäļæŊäļæŊåĻäļĪåĨäļįäŧ·åĨįšĶäđéīåįįĨčĢåģïžčæŊčŪĐį䚧äū§å
åšéĩåŪæŽæäŧķåĻäļæđįšĶ 5200 čĄåĪå°ąå·ēåĢ°æåđķåŪį°įåäļåĨįšĶãæäļäļč―Ūæä― įäŋŪæģį§°ä―éčĶ owner čĢåģįãOption BãïžčŋäļŠåŪæ§čŋäšäŋåŪïžåĻæĪæīæĢãåŪæŊäļäļŠ 4 čĄįå éĪïžåđķäļäļæŽĄæ§å
ģæä― ååšįå
ĻéĻäļäļŠæŠåĢ°æå
ĨåĢïžäļéčĶįŧäŧŧä― producer éäļŠčĄĨ rawOutputSizeã
æŽč―Ūäļč―äŧĢį ã PR į°äļš +2485/â103ïžæŊæŽč―Ūå·ĄæĢčŪūåŪį 1500 čĄ additions įæįšŋį 1.66 åïžčŊĨįæåŊđæŽč―Ūįæïžå æĪčŋæĄäŋŪåĪåįš§įŧ owner åčåīčĢåģãįšŋįĻåŧæäŋæ unresolvedïžæäļæĄå·ēįĄŪčŪĪæįŦįååđķéŧåĄéĄđåĻæēĄæäŋŪåĪįæ åĩäļ resolve æïžįäšæéŧåĄččĩ·æĨã
There was a problem hiding this comment.
Confirmed REAL â but your prescribed fix contradicts this PR's own written contract, so this needs a one-line ruling rather than a closeout edit. Re-verified at the delivered head a3508b57cc3c1367a36a64d390066f5fe138a164; your mechanism is exactly right and I am not re-arguing it. What follows is new evidence that changes which fix is correctable inside this PR.
Why "remove the ?? measureToolOutput(âĶ) arm" is not a bounded edit here. Two artifacts in this PR state the opposite contract:
docs/design/tool-result-size-accounting.md:15(added by this PR): "Producers that shorten output report its size before shortening; ordinary producers are measured immediately after execution." That second clause is the fallback arm.coreToolScheduler.test.ts:3907-3948âdoes not measure a self-bounded producer body as its own raw sizeends with a deliberate control arm, commented "Control: an ordinary producer that declares nothing is still measured from its own body, so the assertions above are not 'never measured'", assertingraw_content_length === 'plain body'.length.
So deleting the arm is not just flipping an assertion: it removes raw-size coverage for every ordinary producer, which the design doc says must be measured. I should also correct my own earlier reply on this thread, which argued the deletion merely makes producer-side obey the consumer-side rule at coreToolScheduler.ts:1012-1015. That argument missed the control test â convertToFunctionResponse is a different path where no post-execution measurement is available, so it is not evidence that the scheduler path should stop measuring.
The doc-conformant fix is your Option A, and it is not bounded either. My own count at a3508b57cc3c1367a36a64d390066f5fe138a164 (grep -c rawOutputSize):
| producer | hits | gap |
|---|---|---|
tools/ls.ts, tools/glob.ts, tools/exec.ts, tools/agent/agent.ts, tools/search-memory.ts, code-mode/output.ts |
0 each | declares nothing |
tools/ripGrep.ts |
1 | declares only on truncatedByLineLimit || truncatedByCharLimit; the searchResult.incomplete path declares nothing |
tools/web-fetch.ts |
7 | assigned only inside if (content.length > MAX_CONTENT_CHARS) at :517-518; a reduction below that threshold declares nothing |
That is 8 source files, plus widening ManagedToolResultPayload across a package boundary for the managed-tool leg, plus updating the producer list in both design docs â which is also what the bot thread on this same line asks for.
The two self-consistent contracts, for you to pick:
- Option A â doc-conformant, larger diff. Keep the fallback; make all 8 producers declare
null; update the producer list indocs/design/tool-result-size-accounting.md+.zh-CN.md. Closes all seven entrances permanently, future producers inherit the rule from the doc list, and ordinary producers keep contributing raw-size samples. Cost: 8 files + one cross-package type. - Option B â your fix, ~4 lines, contract change. Delete the arm, flip the control assertion at
coreToolScheduler.test.ts:3944, and amend doc:15to drop "ordinary producers are measured immediately after execution". Cost: every undeclared producer stops contributing a raw-size sample, so the raw-phase histogram loses coverage on all of them.
Both close the seven entrances. They differ only in whether undeclared means "measured" or "unknown" â that is the contract question, and it is yours to rule on, not mine.
Left unresolved deliberately. To be explicit about why: my earlier replies on this thread cited the 1500-addition patrol fuse, and that was the wrong reason â a procedural budget I set for myself is not grounds to hold a confirmed merge blocker. The objective reason is the one above: Option A is 8 files across a package boundary, Option B reverses a documented contract sentence and a deliberately-authored control test. Say "A" or "B" and either lands in one round.
äļæ
įĄŪčŪĪæįŦââä―ä― įŧįäŋŪæģäļæŽ PR čŠå·ąåäļįåĨįšĶåēįŠïžå æĪéčĶäļåĨčĢåģïžčäļæŊæķå°ūįžčūã å·ēåĻäšĪäŧ head a3508b57cc3c1367a36a64d390066f5fe138a164 äļééŠïžæšåķæčŋ°åŪå
ĻæĢįĄŪïžäļåäščŪšãäŧĨäļæŊäžæđåãåŠį§äŋŪæģåĻæŽ PR å
åŊč―ãįæ°čŊæŪã
äļšäŧäđãå æ ?? measureToolOutput(âĶ) čŋäļæŊãåĻčŋéäļæŊæįæđåĻã æŽ PR éæäļĪåĪ䚧įĐåįįļåįåĨįšĶïž
docs/design/tool-result-size-accounting.md:15ïžæŽ PR æ°åĒïžïžãäžįžĐįčūåšįį䚧č åĻįžĐįåįģæĨå°šåŊļïžæŪéį䚧č åĻæ§čĄåįŦåģčĒŦåšĶéãââåååĨå°ąæŊčŋäļŠå åšåæŊãcoreToolScheduler.test.ts:3907-3948įįĻäūæŦå°ūåļĶäļäļŠåŧæåįåŊđį §čïžæģĻéäļšãåŊđį §ïžæŠåĢ°æįæŪéį䚧č äŧįķæčŠčšŦæĢæčĒŦåšĶéïžå æĪäļéĒįæčĻäļæŊãäŧæŠåšĶéããïžæčĻraw_content_length === 'plain body'.lengthã
æäŧĨå æčŋäļæŊäļåŠæŊįŋŧäļäļŠæčĻïžåŪäžčŪĐæŊäļäļŠæŪéį䚧č
åĪąåŧ raw size čĶįïžččŪūčŪĄææĄĢæįĄŪčĶæąåŪäŧŽčĒŦåšĶéãåæķæīæĢæčŠå·ąåĻæŽįšŋįĻæĐåįååĪââææūčŪščŊãå æåŪåŠæŊčŪĐį䚧äū§éĩåŪæķčīđäū§ coreToolScheduler.ts:1012-1015 įč§åããéĢäļŠčŪščŊæžäšåŊđį
§æĩčŊïžconvertToFunctionResponse æŊåĶäļæĄč·ŊåūïžéĢéæ đæŽæēĄæãæ§čĄååšĶéãåŊįĻïžæäŧĨåŪäļč―čŊæč°åšĶåĻčŋæĄč·ŊåūäđčŊĨåæĒåšĶéã
įŽĶåææĄĢįäŋŪæģæŊæđæĄ AïžčåŪåæ ·äļæŊæįįã æåĻ a3508b57cc3c1367a36a64d390066f5fe138a164 äļčŠå·ąæ°įïžgrep -c rawOutputSizeïžïž6 äļŠæäŧķéķå―äļïžls.tsãglob.tsãexec.tsãagent/agent.tsãsearch-memory.tsãcode-mode/output.tsïžïžripGrep.ts 1 åĪä―åŠčĶį truncatedByLineLimit || truncatedByCharLimitïžsearchResult.incomplete č·ŊåūäļįģæĨïžweb-fetch.ts 7 åĪä―åŠåĻ :517-518 į content.length > MAX_CONTENT_CHARS åæŊå
čĩåžïžä―äščŊĨéåžįįžĐåäļįģæĨãåčŪĄ 8 äļŠæšæäŧķïžåĪå managed-tool éĢæĄč
ŋčĶč·Ļå
čūđįå åŪ― ManagedToolResultPayloadïžååĪå äļĪäŧ―čŪūčŪĄææĄĢįį䚧č
æļ
åã
äļĪåĨčŠæī―åĨįšĶïžčŊ·ä― éïž
- æđæĄ AââįŽĶåææĄĢïždiff æīåĪ§ã äŋįå
åšïžčŪĐ 8 äļŠį䚧č
é―åĢ°æ
nullïžæīæ°äļčąæčŪūčŪĄææĄĢįį䚧č æļ åãäļæŽĄå ģæå ĻéĻäļäļŠå ĨåĢïžåįŧæ°åĒį䚧č äŧææĄĢæļ åįŧ§æŋč§åïžäļæŪéį䚧č įŧ§įŧčīĄįŪ raw æ ·æŽãäŧĢäŧ·ïž8 äļŠæäŧķ + äļäļŠč·Ļå įąŧåã - æđæĄ Bââä― įäŋŪæģïžįšĶ 4 čĄïžæđåĨįšĶã å æå
åšïžįŋŧæ
coreToolScheduler.test.ts:3944įåŊđį §æčĻïžåđķäŋŪæđææĄĢ:15ïžå åŧãæŪéį䚧č åĻæ§čĄåįŦåģčĒŦåšĶéããäŧĢäŧ·ïžæææŠįģæĨį䚧č äļåčīĄįŪ raw æ ·æŽïžraw éķæŪĩįīæđåūåŊđåŪäŧŽæīä―åĪąåŧčĶįã
äļĪč é―č―å ģæäļäļŠå ĨåĢïžå·ŪåŦåŠåĻäšãæŠįģæĨãæåģįãčĒŦåšĶéãčŋæŊãæŠįĨãââčŋæŊåĨįšĶéŪéĒïžčŊĨįąä― čĢåģïžäļčŊĨįąæéã
åŧæäŋæ unresolvedã æįĄŪčŊīæįįąïžææĐååĻæŽįšŋįĻåžįĻįæŊ 1500 čĄå·ĄæĢįæïžéĢæŊéčŊŊįįįąââæčŠå·ąčŪūįįĻåšæ§éĒįŪäļæææĢä―äļæĄå·ēįĄŪčŪĪååđķéŧåĄéĄđįäūæŪãåŪĒč§įįąæŊäļéĒéĢæĄïžæđæĄ A æŊč·Ļå čūđįį 8 äļŠæäŧķïžæđæĄ B čĶæĻįŋŧäļåĨå·ēåčŋææĄĢįåĨįšĶåäļæĄåŧæåäļįåŊđį §æĩčŊãä― åäļäļŠãAãæãBãïžäŧŧäļį§é―č―åĻäļč―Ūå č―å°ã
| let rawOutputSize: ToolOutputSize | undefined; | ||
| let reduced = false; | ||
| if (content.length > MAX_CONTENT_CHARS) { | ||
| rawOutputSize = measureToolOutput(content); |
There was a problem hiding this comment.
R2-3 [Blocker] WebFetchTool has no maxOutputChars override (default = undefined â scheduler applies DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000). The assembled body (producer envelope ~98.4k + buildMetadataHeader ~100 chars) starts with "Status: 200âĶ", not TOOL_OUTPUT_TRUNCATED_PREFIX, so truncateLlmContents idempotency guard (.startsWith(âĶ)) is false. The step-1 per-tool pass cuts the ~98.5k envelope to 25,000 chars, discarding the spill-file pointer â double-truncation.
Compare: WebSearchTool overrides maxOutputChars at web-search.ts:987. Fix: add override get maxOutputChars(): number { return MAX_CONTENT_CHARS + FETCH_ENVELOPE_HEADROOM_CHARS; } (or an equivalent constant) to WebFetchTool.
There was a problem hiding this comment.
Verified at live head d502f85498059a1e20c5348c17a08dfcb9a6ab28. REAL â but it is a regression of an already-resolved thread, not a new one.
Same defect as bot R1-3 at web-fetch.ts:517 (top comment 4221546209), which was resolved on the author's reply 4221812685: "Fixed in 7dfc9bc026". That fix is absent at live head:
git grep -E 'maxOutputChars|FETCH_ENVELOPE_HEADROOM_CHARS'overtools/web-fetch.tsatd502f8549805â 0 hits (the onlyoverride getin the file is:723 getDescription());FETCH_ENVELOPE_HEADROOM_CHARSâ 0 hits repo-wide.7dfc9bc026added the override atweb-fetch.ts:796-797(+14 lines).95ae14b46f("fix(core): correct tool size attribution and recoverable previews") deleted exactly those 14 lines. Both are ancestors of live head.
Mechanism confirmed: base tools.ts:359 returns undefined â coreToolScheduler.ts:6779 perToolMax undefined â global DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000 (config.ts:986), keep: 'both' (tools.ts:368); the assembled body starts with buildMetadataHeader (:579-581), not the sentinel, so both startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX) guards (truncation.ts:455, coreToolScheduler.ts:6843) are false. The sibling guard chiga0 cites is intact at web-search.ts:987.
Two details differ from the write-up: the body starts with URL: âĶ, not Status: 200âĶ (buildMetadataHeader :324-331 returns ${urlLine}\nStatus: âĶ) â immaterial to the guard; and the spill pointer sits inside header at the head of the body (:580-581), so under keep: 'both' it lands in the head slice and survives. What is actually lost is the producer's ~98k preview (previewChars = MAX_CONTENT_CHARS - 2000, :526) being re-cut to 25k, plus a second spill envelope.
No code added this round: the PR is at +2485/-103, 1.66x over the 1500-addition scope fuse, so whether to restore the override is a maintainer decision. Left unresolved.
There was a problem hiding this comment.
Confirmed REAL at live head d502f85498059a1e20c5348c17a08dfcb9a6ab28. Witnesses re-run this round:
grep -c maxOutputCharsâpackages/core/src/tools/web-fetch.ts0;packages/core/src/tools/web-search.ts1 (:987 override get maxOutputChars(): number { return MAX_RESULT_SIZE_CHARS + RESULT_ENVELOPE_HEADROOM_CHARS; }); basepackages/core/src/tools/tools.ts:359 get maxOutputChars(): number | undefined.FETCH_ENVELOPE_HEADROOM_CHARSâ 0 hits repo-wide.packages/core/src/config/config.ts:986 export const DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000;, taken up atcoreToolScheduler.ts:6779(const perToolMax = limitsTool.maxOutputChars;) and applied asperToolMax ?? this.config.getTruncateToolOutputThreshold()at:6802and:6846. With no override,perToolMaxisundefinedand the global 25k applies.- The envelope that gets re-cut:
web-fetch.ts:67 const MAX_CONTENT_CHARS = 100_000;, producer truncation at:517-526(previewChars: MAX_CONTENT_CHARS - 2000), assembled at:579frombuildMetadataHeader(:324).
Addition that reframes this one: it is a lost fix, not a new decision. Both commits are ancestors of live head:
7dfc9bc02603eff99ca4e027e9e3d058257436a2added the override (+14 lines), with the reason already in its message: "WebFetchTool declared no maxOutputChars, so the scheduler's per-tool pass re-truncated the producer envelope at the global 25k threshold, spilled a second file and pointed the model at that envelope instead of the page."95ae14b46f84bfd5688864dd1479f86ebda13690("fix(core): correct tool size attribution and recoverable previews") deleted exactly those 14 lines âgit show 95ae14b46f --stat -- packages/core/src/tools/web-fetch.tsâ1 file changed, 14 deletions(-), and the removed hunks are-const FETCH_ENVELOPE_HEADROOM_CHARS = 2_000;and- override get maxOutputChars(): number { return MAX_CONTENT_CHARS + FETCH_ENVELOPE_HEADROOM_CHARS; }. That commit's message is a bare subject line â it states no reason for the deletion.
So the fix is a +14-line restoration of code that already existed, was reviewed, and was dropped without a stated reason. It is the cheapest of the three Blockers and the one I would unblock first if the cap is lifted for any of them.
The two corrections my earlier reply made to your write-up still stand and are unchanged by this: the assembled body starts with URL: âĶ (from buildMetadataHeader, :324) rather than Status: 200âĶ, and under keep: 'both' the spill pointer sits in the head slice and survives â what is actually lost is the ~98k producer preview being re-cut to 25k, plus a second spill envelope. Neither weakens the finding.
No code lands this round â +2485/â103 against the 1500-addition cap set for this patrol round, so this is escalated for an owner decision on scope. Left unresolved.
äļæ
åĻ live head d502f85498059a1e20c5348c17a08dfcb9a6ab28 äļéč·åčŊïž
grep -c maxOutputCharsïžpackages/core/src/tools/web-fetch.ts0ïžpackages/core/src/tools/web-search.ts1ïž:987 override get maxOutputChars(): number { return MAX_RESULT_SIZE_CHARS + RESULT_ENVELOPE_HEADROOM_CHARS; }ïžïžåšįąŧpackages/core/src/tools/tools.ts:359 get maxOutputChars(): number | undefinedãFETCH_ENVELOPE_HEADROOM_CHARSå Ļäŧåš 0 åĪå―äļãpackages/core/src/config/config.ts:986 export const DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000;ïžåĻcoreToolScheduler.ts:6779ïžconst perToolMax = limitsTool.maxOutputChars;ïžåįĻïžåđķåĻ:6802ã:6846äŧĨperToolMax ?? this.config.getTruncateToolOutputThreshold()č―å°ãæēĄæ override æķperToolMaxäļšundefinedïžå Ļåą 25k įæã- äžčĒŦäšæŽĄčĢåįäŋĄå°ïž
web-fetch.ts:67 const MAX_CONTENT_CHARS = 100_000;ïžproducer äū§æŠæåĻ:517-526ïžpreviewChars: MAX_CONTENT_CHARS - 2000ïžïžåĻ:579įąbuildMetadataHeaderïž:324ïžæžčĢ ã
čĄĨäļæĄäžæđååŪæ§įčŊæŪïžčŋæŊčĒŦäļĒæįæĒæäŋŪåĪïžäļæŊæ°åģįã äļĪäļŠæäšĪé―æŊ live head įįĨå ïž
7dfc9bc02603eff99ca4e027e9e3d058257436a2å äļäščŋäļŠ overrideïž+14 čĄïžïžįįąå·ēååĻåŪį commit message éïž"WebFetchTool declared no maxOutputChars, so the scheduler's per-tool pass re-truncated the producer envelope at the global 25k threshold, spilled a second file and pointed the model at that envelope instead of the page."95ae14b46f84bfd5688864dd1479f86ebda13690ïž"fix(core): correct tool size attribution and recoverable previews"ïžæčŋ 14 čĄåæ ·å æââgit show 95ae14b46f --stat -- packages/core/src/tools/web-fetch.tsâ1 file changed, 14 deletions(-)ïžčĒŦå į hunk æĢæŊ-const FETCH_ENVELOPE_HEADROOM_CHARS = 2_000;äļ- override get maxOutputChars(): number { return MAX_CONTENT_CHARS + FETCH_ENVELOPE_HEADROOM_CHARS; }ãčŊĨæäšĪį message åŠæäļčĄ subjectïžæēĄæåæå éĪįįąã
æäŧĨäŋŪæģæŊæĒåĪäļæŪĩå·ēååĻãå·ēčĒŦčŊåŪĄčŋãäļæēĄæåæįįąå°ąčĒŦå æįäŧĢį ïžå ą +14 čĄãčŋæŊäļäļŠ Blocker éæäūŋåŪįäļäļŠïžåĶæčĶäļšå ķäļäŧŧä―äļäļŠæūåžįæïžæäžå æūåžåŪã
æäļäļč―ŪåŊđä― æčŋ°åįäļĪåĪæīæĢäūįķæįŦïžæŽæĄäļæđååŪäŧŽïžæžåšæĨį body äŧĨ URL: âĶ åžåĪīïžæĨčŠ buildMetadataHeaderïž:324ïžïžäļæŊ Status: 200âĶïžåĻ keep: 'both' äļ spill æéč―åĻ head įæŪĩéãäžåæīŧââįæĢäļĒįæŊéĢ ~98k į producer éĒč§čĒŦååå° 25kïžåĪå įŽŽäšäļŠ spill äŋĄå°ãäļĪč
é―äļååžąæŽæĄ findingã
æŽč―Ūäļč―äŧĢį ââ+2485/â103 čķ čŋæŽč―Ūå·ĄæĢčŪūåŪį 1500 čĄ additions įæïžå æĪåįš§įŧ owner åčåīčĢåģãįšŋįĻäŋæ unresolvedã
There was a problem hiding this comment.
Mechanism confirmed â but restoring the override reverses a decision this PR documents explicitly and pins with a test, so it needs your ruling, not a closeout edit. Re-verified at the delivered head a3508b57cc3c1367a36a64d390066f5fe138a164.
The mechanism holds. grep -c maxOutputChars packages/core/src/tools/web-fetch.ts â 0; packages/core/src/config/config.ts:986 â DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000; consumed at coreToolScheduler.ts:6779 (const perToolMax = limitsTool.maxOutputChars;) and landed as perToolMax ?? this.config.getTruncateToolOutputThreshold(). With no override, the global 25k pass does re-cut the ~98k producer envelope. FETCH_ENVELOPE_HEADROOM_CHARS has 0 hits repo-wide.
Why I am not just restoring it. 95ae14b46f deleted the override, and unlike a silent drop, the reason is written into this PR's own artifacts:
docs/design/tool-result-size-accounting.md:41: "Web fetch retains its internal extraction/side-query input and uses global persistence for its outward oversized envelope. No 102k result override remains. A failed large-page processing fallback may preserve an outer envelope and a separate original-page pointer; it does not promise a single spill."web-fetch.test.ts:385:expect(tool.maxOutputChars).toBeUndefined();â inside a test that95ae14b46frenamed fromshould survive the scheduler per-tool pass without a second spilltokeeps default result budgets without shrinking side-query input, replacing the no-second-spill assertion with this one.
So the second spill was traded away deliberately, in exchange for keeping the default result budget and not shrinking the side-query input. Restoring the override means reverting that trade: re-add FETCH_ENVELOPE_HEADROOM_CHARS + the getter (+14 lines), delete the toBeUndefined() pin, and amend doc :41.
Two corrections to the mechanism as stated â they narrow what is actually lost, and neither weakens the finding:
- the assembled body starts with
URL: âĶfrombuildMetadataHeader(web-fetch.ts:324, assembled at:579), notStatus: 200âĶ; - under
keep: 'both'the spill pointer lands in the head slice and survives, so the model keeps a recovery path. What is actually lost is the ~98k producer preview being re-cut to 25k plus a second spill envelope â which is precisely what doc:41accepts.
The ruling needed: does the doc's "global persistence for the outward envelope" contract stand (then this is accepted behaviour and the finding should be closed as by-design), or should web_fetch be its own sizing authority again (then the +14-line restore lands with the test pin deleted and doc :41 amended)? The code change is cheap either way; the decision is not mine.
Left unresolved deliberately. As on R2-1, I am explicitly not citing the patrol line-count fuse here â that was the wrong reason in my earlier replies. The objective reason is that the fix contradicts a documented decision and a test that pins it.
äļæ
æšåķæįŦââä―æĒåĪčŋäļŠ override äžæĻįŋŧæŽ PR æįĄŪåäļãåđķįĻæĩčŊéä―įäļéĄđåģåŪïžå æĪéčĶä― įčĢåģïžčäļæŊæķå°ūįžčūã å·ēåĻäšĪäŧ head a3508b57cc3c1367a36a64d390066f5fe138a164 äļééŠã
æšåķæįŦã grep -c maxOutputChars packages/core/src/tools/web-fetch.ts â 0ïžconfig.ts:986 â DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000ïžåĻ coreToolScheduler.ts:6779 åįĻïžconst perToolMax = limitsTool.maxOutputChars;ïžïžåđķäŧĨ perToolMax ?? this.config.getTruncateToolOutputThreshold() č―å°ãæēĄæ override æķïžå
Ļåą 25k éĢäļčķįĄŪåŪäžæįšĶ 98k į producer äŋĄå°ååäļæŽĄãFETCH_ENVELOPE_HEADROOM_CHARS å
Ļäŧåš 0 å―äļã
äļšäŧäđææēĄæįīæĨæĒåĪã 95ae14b46f å æäščŋäļŠ overrideïžčäļäļãééŧå éĪãäļåïžįįąåčŋäšæŽ PR čŠå·ąį䚧įĐïž
docs/design/tool-result-size-accounting.md:41ïžãWeb fetch äŋįå ķå éĻæ―å/äū§æĨčŊĒčūå ĨïžåŊđåĪįčķ åĪ§äŋĄå°čĩ°å Ļåąæäđ åãäļåäŋį 102k įŧæ overrideã åĪ§éĄĩéĒåĪįåĪąčīĨįå åšåŊč―åæķįäļåĪåąäŋĄå°äļäļäļŠæååå§éĄĩéĒįæéïžåŪäļæŋčŊšåŠæäļæŽĄ spillããweb-fetch.test.ts:385ïžexpect(tool.maxOutputChars).toBeUndefined();ââ ä―äšäļäļŠčĒŦ95ae14b46fæđåįįĻäūéïžäŧshould survive the scheduler per-tool pass without a second spillæđækeeps default result budgets without shrinking side-query inputïžåđķæãæ įŽŽäšæŽĄ spillãįæčĻæĒæäščŋäļæĄã
æäŧĨįŽŽäšæŽĄ spill æŊčĒŦåŧæäšĪæĒæįïžæĒæĨįæŊäŋįéŧčŪĪįŧæéĒįŪãäļäļįžĐå°äū§æĨčŊĒčūå
ĨãæĒåĪ override æåģįåéčŋįŽäšĪæĒïžéæ°å å FETCH_ENVELOPE_HEADROOM_CHARS äļ getterïž+14 čĄïžãå æ toBeUndefined() čŋäļŠéåãåđķäŋŪæđææĄĢ :41ã
åŊđä― æčŋ°įäļĪåĪæīæĢââåŪäŧŽæķįŠäšãåŪé äļĒäšäŧäđãïžé―äļååžąæŽæĄ findingïž
- æžåšæĨį body äŧĨ
URL: âĶåžåĪīïžæĨčŠbuildMetadataHeaderïžweb-fetch.ts:324ïžåĻ:579æžčĢ ïžïžäļæŊStatus: 200âĶïž - åĻ
keep: 'both'äļ spill æéč―åĻ head įæŪĩéãäžåæīŧïžæĻĄåäŧææĒåĪč·ŊåūãįæĢäļĒįæŊéĢįšĶ 98k į producer éĒč§čĒŦååå° 25kïžåĪå įŽŽäšäļŠ spill äŋĄå°ââččŋæĢæŊææĄĢ:41æįĄŪæĨåįã
éčĶä― čĢåģįæŊïž ææĄĢãåŊđåĪäŋĄå°čĩ°å
Ļåąæäđ
åãčŋäļŠåĨįšĶæŊåĶæįŦïžæįŦåæŽæĄåą by-designïžåšæčŪūčŪĄæåūå
ģéïžïžčŋæŊ web_fetch éæ°æäļščŠå·ąįå°šåŊļæåĻïžå +14 čĄįæĒåĪäļå éåãæđææĄĢ :41 äļčĩ·č―å°ïžãäļĪį§æ
åĩäļäŧĢį æđåĻé―åūäūŋåŪïžä―čŋäļŠåģåŪäļčŊĨįąæåã
åŧæäŋæ unresolvedã äļ R2-1 äļæ ·ïžæčŋéæįĄŪäļååžįĻå·ĄæĢįčĄæ°įæââéĢæŊææĐåååĪéįĻéįįįąãåŪĒč§įįąæŊïžčŋäļŠäŋŪæģäļäļéĄđå·ēåčŋææĄĢįåģåŪåäļäļŠéä―åŪįæĩčŊįļåēįŠã
On the non-rawCapture path shell declared measureToolOutput(llmContent) as the producer's raw size, but llmContent is already the capture-bounded prefix: past maxBufferedOutputBytes the service discards the rest, so a 200 MB command reported the 64 MiB ceiling as its own raw size. Expose the discard the service already computes and declare the raw size unknown when it happened, keeping the measured size whenever capture retained the whole stream. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvan7dwe9a
Base merge only; no source changes authored here. Needed because the PR-stage CI budget gate added on main by QwenLM#14213 invokes .github/scripts/check-ci-budget.mjs, which did not exist anywhere on this branch, so Lint & Static failed at step 9 with MODULE_NOT_FOUND and short-circuited every later step (ESLint, Prettier, typecheck) - the delivered shell raw-size fix got no CI verification at all. Automatic merge, zero conflicts. Verified on the merged tree: - node .github/scripts/check-ci-budget.mjs -> rc 0, "PR-stage CI budget holds" - npm run typecheck --workspace=packages/core -> rc 0 - vitest src/tools/shell.test.ts src/services/shellExecutionService.test.ts -> 547/547 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvan7dwe9a
âĶertion
`loads a published static Skill durably without Runtime execution or
approval` compared the projected durable receipt against the parts
returned straight from `turn.execute`. The latter carries
`Symbol('tool-output-provenance')` (packages/core/src/tools/tool-output-size.ts:38)
while the former round-trips through durable JSON, which cannot carry a
symbol â so the assertion could only fail:
- Symbol(tool-output-provenance): { callId: 'call-0', ... }
Wrap the expected side in a JSON round-trip, matching what this file
already does at seven sibling sites (:1290, :1528, :1697, :2096, :2534,
:5038, :5119). The assertion still checks that the functionResponse
survives projection; it just stops requiring an unserializable symbol.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmvas7fee9h
Sync the actual base so Lint & Static stops failing at step 10 "Check lint gate freshness" (job 116928347871). The branch's merge-base is a8eb74f and main has since moved .github/workflows/ci.yml (631b860), a GATE_FILES entry, so the freshness gate judged this head stale even though the PR touches no .github file at all. Mechanical merge only: the resulting tree is byte-identical to `git merge-tree --write-tree origin/main <pr-head>`, no hand-written content and no conflict resolution. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmvaucl4v9l
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed â gaps disclosed.
Not reviewed: build-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit â the loop stopped after round 4 of the 5-round cap without reaching two consecutive dry rounds; round 5 was not launched so the remaining budget covered verification of round 4 findings, the blocker re-check and submission.
Not reviewed: test efficacy â the probe harness could not be validated (harnessValidated: null; all 19 probed files inconclusive, 15 mutant and 98 hunk candidates cut by selection caps), so this diff changed hunks ship with no efficacy evidence either way.
Not reviewed: chunk 7 (packages/core/src/tools/*) â did not execute the new tests in the worktree; each was checked by reading its fixture and assertions â stopped at the agent tool budget.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": I could not resolve what toolName binds to at the scheduler's own persistence-gate call sites (coreToolScheduler.ts:6629 and :7184 â the binding is outside thâĶ; "agent reverse-audit (round 3)": whether any caller re-sends the *same* provenance-less params.message object (which would regenerate a fresh provenance per send at llm-chat.ts:3289 and defeaâĶ; "agent reverse-audit (round 3)": the chunk-6 hunks of shellExecutionService.ts (the pty resolve sites at :2437 and :2848) â I read those lines in the worktree to confirm the flag is present, buâĶ; chunk 11: none â no check in my territory was cut short by the tool ceiling.; chunk 7: did not execute the new tests in this worktree ( npx vitest run on the five touched test files) â correctness of each was verified by reading fixture and asserâĶ.
2 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 9, not a blocker) â recorded, not requested in this round:
packages/core/src/tools/mcp-tool.ts:1053 â [review] R9-2: buildMcpToolError computes a rawOutputSize that both of its consumers discard by design, while paying to re-materialize the whole error payload â and it contradicts the error-patâĶpackages/core/src/tools/mcp-tool.ts:1027 â [review] R9-3: The MCP producer raw-size declarations â the sites that report a *measured* pre-reduction size rather than null , at :782 and here at :1026 â have no test, and the one-token mutatioâĶpackages/cli/src/acp-integration/session/Session.ts:16917 â [review] The ACP boundary now feeds toolResult into convertToFunctionResponse , but that converter has no raw-size fallback for a producer that does not *declare* rawOutputSizeâĶpackages/cli/src/acp-integration/session/Session.ts:16921 â [review] The ACP boundary is the one path where convertToFunctionResponse 's name-prefix tool_type inference is *final* (nothing re-attaches provenance afterwards), so a code-moâĶpackages/cli/src/ui/hooks/useToolScheduler.test.ts:728 â [review] Eight part-shape assertions were relaxed from exact object literals to expect.objectContaining , where the same PR demonstrates a strict alternative that keeps exactnesspackages/core/src/core/client.ts:785 â [review] The new LlmClient.markToolResultsInjected delegation â the only production change in this chunk, and the sole path by which a speculation-accept replay is marked â has no test; the one test âĶpackages/core/src/core/coreToolScheduler.test.ts:15350 â [review] The new persistence-size-attribution test drives the private maybePersistLargeToolResult on a constructor-skipped prototype with a locally restated signature, so tsc no lonâĶpackages/core/src/core/coreToolScheduler.test.ts:15370 â [review] The new persistence size attribution test is the *only* observer of the persistence-layer ToolResultSizeEvent (the file's single logToolResultSize spy, at :15370), yet âĶpackages/core/src/core/coreToolScheduler.ts:1006 â [review] The new try { âĶ } catch {} wraps a single plain property read on a Pick<ToolResult, 'rawOutputSize'> , which cannot throw â unreachable error handling that also swallows silentlâĶpackages/core/src/core/coreToolScheduler.ts:1008 â [review] convertToFunctionResponse measures the whole tool output unconditionally, but processed is read only when the new optional producerResult is supplied â 33 of 36 call sites neâĶpackages/core/src/core/coreToolScheduler.ts:1117 â [review] The new config parameter on convertToFunctionErrorResponse is inert: producerResult is hard-coded undefined, so rawSize and truncated are always undefined and processed - theâĶpackages/core/src/core/coreToolScheduler.ts:6800 â [review] The first model-facing truncation pass records the *char* threshold as the applied budget even when the *line* cap was the constraint that actually cut, so the stamped budget canâĶpackages/core/src/core/coreToolScheduler.ts:7726 â [review] Budget attribution has no positive scheduler-level test: no assertion anywhere pins applied_budget / budget_source for a real reduction (two of the filed premises were refuted anâĶpackages/core/src/core/coreToolScheduler.ts:7879 â [review] The persistence gate decides 'already truncated' by substring-matching a literal marker inside untrusted tool output, and this diff fixes one fooled tool by adding its name to an eâĶpackages/core/src/core/llm-chat.test.ts:12340 â [review] The new tool result injection accounting suite is the only test of the injection-layer ToolResultSizeEvent , and it is constructed so that no budget reduction can ever fire â leaviâĶpackages/core/src/core/llm-chat.test.ts:12389 â [review] The only assertion about the injection layer's raw column cannot tell it apart from the injected column, because the fixture's declared rawSize is numerically identical to the measuâĶpackages/core/src/core/llm-chat.test.ts:12495 â [review] The new suite pins injection accounting only for a retry whose *first attempt actually reached the stream*; the production code marks the provenance as injected and emits tool_resultâĶpackages/core/src/core/llm-chat.ts:3284 â [review] The restored-response provenance attach is exercised by two tests but asserted by none - its only visible trace in the suite is that it forced two identity assertions to be loosenedpackages/core/src/core/llm-chat.ts:5503 â [review] The prepared-request resume branch dispatches provenance-stripped contents, so a whole dispatch path ( sendPreparedMessageStream , used by hosted recovery via turn.ts:754 ) emits zero injeâĶpackages/core/src/followup/speculation.ts:629 â [review] acceptSpeculation marks *every* replayed message as already-injected, but runSpeculativeLoop returns executed tool responses that the fork chat never dispatched, so their one owedâĶ- âĶand 40 more (see the run report)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor â and the round it recovered had no anchor this round could use either â none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head â so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on â this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical â Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk â the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | âĶ | âĶ | âĶ |
Advisory only â it does not block this review.
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
æŠåŪĄæĨïžåæäļščąæïžïžreverse audit â the loop stopped after round 4 of the 5-round cap without reaching two consecutive dry rounds; round 5 was not launched so the remaining budget covered verification of round 4 findings, the blocker re-check and submission.
æŠåŪĄæĨïžåæäļščąæïžïžtest efficacy â the probe harness could not be validated (harnessValidated: null; all 19 probed files inconclusive, 15 mutant and 98 hunk candidates cut by selection caps), so this diff changed hunks ship with no efficacy evidence either way.
æŠåŪĄæĨïžåæäļščąæïžïžchunk 7 (packages/core/src/tools/*) â did not execute the new tests in the worktree; each was checked by reading its fixture and assertions â stopped at the agent tool budget.
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent reverse-audit (round 2)"ïžI could not resolve what toolName binds to at the scheduler's own persistence-gate call sites (coreToolScheduler.ts:6629 and :7184 â the binding is outside thâĶïž"agent reverse-audit (round 3)"ïžwhether any caller re-sends the *same* provenance-less params.message object (which would regenerate a fresh provenance per send at llm-chat.ts:3289 and defeaâĶïž"agent reverse-audit (round 3)"ïžthe chunk-6 hunks of shellExecutionService.ts (the pty resolve sites at :2437 and :2848) â I read those lines in the worktree to confirm the flag is present, buâĶïžchunk 11ïžnone â no check in my territory was cut short by the tool ceiling.ïžchunk 7ïždid not execute the new tests in this worktree ( npx vitest run on the five touched test files) â correctness of each was verified by reading fixture and asserâĶã
2 æĄ Suggestion åĻå·ēč§Ģæį critical ååļäļéäđåĪčĒŦčĩ·čäļščĄå čŊčŪšïžCLI å·ēå°å ķį§ŧå Ĩäļæđåŧķåæļ åïžäļéåžšåķæ§čĄïžã
æķæå§ŋæäļåŧķåïžįŽŽ 9 č―Ūïžééŧæïžââå·ēčŪ°å―ïžæŽč―ŪäļčĶæąäŋŪæđïžå ą 60 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
æšåķåĨåš·ïžæŽč―ŪæŠč―åđēåæķå°ūïžå čæĢįäšåĒééįđïžčåŪæĒåĪå°įéĢäļč―ŪäđæēĄæįäļæŽč―ŪåŊįĻįéįđââčĶäđåŪå ĻæēĄæãčĶäđæēĄæčŪĪčŊč ãčĶäđįąæŽč―ŪčŋčĄčšŦäŧ―äđåĪįčšŦäŧ―čŪĪčŊãčĶäđčĒŦæŽč―Ūįč·åæįŧæč§ĢæäļšåĪīæäšĪââå æĪäļäļæŽĄčŊåŪĄå°éčŊŧæīäļŠ diffïžéĪéæĒåĪæĩįĻææŽč―Ūč―ä―ŋįĻįæīæĐčŠæéįđåŦæĨå°æŽč―ŪįäļįåŪæīå·Ĩä―æļ åäļïžåđķäžäļįīåĶæĪïžįīå°æäļč―Ūįæ čŪ°éæ°åļĶäļéįđïžæč―å°įåŦæĨč―čĒŦčŋčĄčŊĨč―ŪįčŊåŪĄä―ŋįĻãïžäŧ éčŋ°ïžäļæŪæĪčĄåĻââčŋäļæđåæŽč―Ūååļįäŧŧä―å åŪđãïž
æŪä―éĢéĐïžæŽåūŠįŊåĪäš persistently-critical å―Ēæââäļäļč―Ūå·Ĩä―æļ
åäļį Critical æŽč―ŪäūįķååĻïžæŽč―Ū 2 æĄ CriticalïžïžéĶæŽĄåį°įéįæēĄæäļéïžæŽč―Ū 1ïžäļäļč―Ū 0ïžïžäļæŠåģ Critical į§ŊåæēĄæåå°ãseverity floor æ æģä―ŋå
ķæķæãåŧščŪŪïžland-with-residual-riskââåšåĢæŊ maintainer įéĢéĐæĨååģåŪïžåå
Ĩåđķæŋæ
æŪä―éĢéĐïžïžčéååžäļč―ŪčŊåŪĄãäūčŊĨåģåŪä―ŋįĻįæŪä―éĢéĐæļ
åïžmaintainer åĄŦåïžïžææŊæĄæŠåģ Critical ååšãæŧåŧéĒ · æŧåŧč
äūčĩæ§ Â· å―ąåčåīãäļæ ãäŧ
äļšåŧščŪŪââäļéŧææŽæŽĄčŊåŪĄã
â qwen3.8-max via Qwen Code /review (v0.25.2)
The bounded envelope leads with TOOL_OUTPUT_TRUNCATED_PREFIX, but when the body is a JSON sample the head/tail preview and the description that carried the '... [CONTENT TRUNCATED] ...' marker are both replaced, so the envelope has no marker at all. isAlreadyTruncated only recognised that marker and '<persisted-output>', so the scheduler's persistence gate treated an already bounded result as fresh and persisted it a second time. Recognise the sentinel the same way four sibling guards already do (truncateLlmContent's string and Part[] paths, tool-result-retention, coreToolScheduler). startsWith, not includes: a tool whose own output merely contains the phrase mid-stream must still be bounded. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvbycx1dbp
Main moved .github/workflows/ci.yml after this branch last incorporated it, so the lint-gate freshness check would fail and short-circuit every later step in Lint & Static. Fold the base in so this push is CI-verified. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvbycx1dbp
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed â gaps disclosed.
Unresolved, please confirm:
- [Critical] Issue-level comment 6062881329 (qwen-code-review-bot sandboxed verification, 'not passed â findings reported') â the body is rendered as a snippet in the PR context and was not fetched in full, so its asserted defects could not be ruled on âĶ
Not reviewed: build-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Agent 7's own test phase never started (the build consumed the whole-call budget), so the packages/core and packages/cli suites did not execute in this review.
Not reviewed: test efficacy â the probe harness could not be validated (harnessValidated: null; all 19 probes inconclusive), so this diff's changed hunks ship with no efficacy evidence either way.
Not reviewed: mutation probing â review scratch-tree was unavailable in this checkout (repo-local git config carries an unreadable includeIf), so no verifier could run a mutation flip; confirmations rest on read-only probes against the built dist and on quoted lines.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none â but two checks I did not run are disclosed as scope, not as gaps: I did not execute web-fetch.test.ts to confirm the > 90000 assertion's exact post-fâĶ; "agent reverse-audit (round 2)": did not read tool-response-finalizer.ts:1-330 ( collectTextSlots / replaceTextSlots ) to confirm replaceTextSlots deep-copies the functionResponse rather âĶ; "agent reverse-audit (round 2)": inferred rather than walked that no hosted-harness path routes a rawCapture shell call through CoreToolScheduler.observeProducerOutput â I confirmed the singlâĶ; "agent reverse-audit (round 1)": did not finish enumerating every production sendMessageStream caller (Session.ts, hosted-workspace-tool-turn.ts, hosted-harness-session.ts, managed-runtime-ouâĶ; "agent reverse-audit (round 4)": did not execute npx vitest run src/tools/truncation.test.ts / tool-response-finalizer.test.ts , so the new cases are verified by reading against the implemenâĶ.
Not reviewed: reverse audit â stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 10, not a blocker) â recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes â fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect â and remain follow-up work recorded in the findings artifact:
packages/core/src/tools/read-file.ts:395 â [review] Critical [fails-closed] [new-surface] result.isTruncated is true for *any* read that does not return the file from line 1 to its last line â including a caller-requested offset / limit âĶdocs/design/tool-result-size-accounting.md:19 â [review] The design's unconditional "every shortening layer reports size" contract does not hold for the hosted/daemon tool-turn path: packages/cli/src/serve contains zero references to rawâĶpackages/cli/src/acp-integration/session/Session.ts â [review] The ACP path is the one caller that keeps the provenance convertToFunctionResponse attaches, but that function has no ordinary-producer measurement fallback â so on ACP every âĶpackages/cli/src/ui/hooks/useToolScheduler.test.ts:210 â [review] Nine assertions were relaxed from exact object equality to expect.objectContaining to accommodate the new enumerable provenance Symbol, dropping the pin that a scheduler reâĶpackages/core/src/core/client.ts:785 â [review] The new LlmClient delegation that carries the speculation-accept replay dedup is never executed by any test â its only caller replaces the whole client with a hand-built stub, so the cross-câĶpackages/core/src/core/coreToolScheduler.test.ts:15390 â [review] The only test anywhere that observes the persistence -layer ToolResultSizeEvent asserts just the two token arguments and prompt_id ; the raw_content_length / injected_coâĶpackages/core/src/core/coreToolScheduler.ts:1008 â [review] convertToFunctionResponse measures the whole body on every call, but processed is read only inside truncated , which short-circuits whenever rawSize === undefined â i.e. at âĶpackages/core/src/core/coreToolScheduler.ts:1117 â [review] The new config?: Pick<Config, 'getChatCompression'> parameter on convertToFunctionErrorResponse cannot influence any output: the function always passes producerResult: undefinâĶpackages/core/src/core/coreToolScheduler.ts:6799 â [review] None of the five recordBudgetReduction call sites (L6799 per-tool, L6874 combined, L7157/L7192 timeout, L7299 error gate, plus the persistence gate at L6635) has a positive test:âĶpackages/core/src/core/coreToolScheduler.ts:6861 â [review] The combined pass re-derives the budget source inline although staticBudgetSource , defined 20 lines above in the same scope, is that exact expression.packages/core/src/core/coreToolScheduler.ts:7810 â [review] The two identity fields this hunk stamps onto a tool result's provenance are derived from two *different* names, and neither matches the derivation used by the persistence -layer âĶpackages/core/src/core/llm-chat.test.ts:12389 â [review] Every case in the new tool result injection accounting block injects a body the send-boundary budget does not touch, so the injection event's reduction fields â injected_content_leâĶpackages/core/src/core/llm-chat.ts:3284 â [review] The new provenance-less fallback stamp lives only in the else (non-prepared) branch of sendMessageStream , while the preparedRequest branch â the one send path that *provably* strips pâĶpackages/core/src/core/nonInteractiveToolExecutor.test.ts:109 â [review] All six executeToolCall assertions now compare a JSON *projection* of responseParts instead of the parts themselves, so nothing in this file pins what a non-interaâĶpackages/core/src/followup/speculation.ts:626 â [review] acceptSpeculation marks *every* replayed message as already injected, but the speculative loop dispatches messages[k] only at the top of iteration k+1 â so a trailing tool-resulâĶpackages/core/src/services/shellExecutionService.ts:1353 â [review] The producer half of the new outputCaptureLimitExceeded pairing is untested: shell.test.ts only *injects* the flag through a mocked execution result, and no test assertâĶpackages/core/src/telemetry/loggers.test.ts:1516 â [review] Thirteen of the fourteen expectations this hunk adds to the shared expectToolCallLogged helper are undefined -valued, and vitest's toHaveBeenCalledWith equality ignores undefiâĶpackages/core/src/telemetry/loggers.ts:397 â [review] Neither new sink function has a test â logToolResultSize 's layer === 'injection' metric gate ( loggers.ts:397 ) and logToolResultPersisted 's explicit field whitelist that keeps ToâĶpackages/core/src/telemetry/metrics.test.ts:124 â [review] The only test for recordToolResultSizeMetrics asserts one of the four records it makes (the raw-chars point); the raw-tokens, injected-chars and injected-tokens records â their vaâĶpackages/core/src/telemetry/metrics.ts:267 â [review] The two new histograms and three new log events this diff adds are absent from docs/developers/development/telemetry.md , the repo's enumerated telemetry reference (its ### Metrics liâĶ- âĶand 12 more (see the run report)
Convergence: round 10 posted 4 inline comment(s), 3 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/core/src/core/coreToolScheduler.ts (findings in round 5; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause â triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only â nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor â and the round it recovered had no anchor this round could use either â none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head â so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on â this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical â Criticals stood in the previous round's work-list and stand again this round (4 Critical(s)), the rate of first-time findings is not falling (this round 3, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk â the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | âĶ | âĶ | âĶ |
Advisory only â it does not block this review.
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã
æŠåģïžčŊ·įĄŪčŪĪïžå ą 1 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Agent 7's own test phase never started (the build consumed the whole-call budget), so the packages/core and packages/cli suites did not execute in this review.
æŠåŪĄæĨïžåæäļščąæïžïžtest efficacy â the probe harness could not be validated (harnessValidated: null; all 19 probes inconclusive), so this diff's changed hunks ship with no efficacy evidence either way.
æŠåŪĄæĨïžåæäļščąæïžïžmutation probing â review scratch-tree was unavailable in this checkout (repo-local git config carries an unreadable includeIf), so no verifier could run a mutation flip; confirmations rest on read-only probes against the built dist and on quoted lines.
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent reverse-audit (round 4)"ïžnone â but two checks I did not run are disclosed as scope, not as gaps: I did not execute web-fetch.test.ts to confirm the > 90000 assertion's exact post-fâĶïž"agent reverse-audit (round 2)"ïždid not read tool-response-finalizer.ts:1-330 ( collectTextSlots / replaceTextSlots ) to confirm replaceTextSlots deep-copies the functionResponse rather âĶïž"agent reverse-audit (round 2)"ïžinferred rather than walked that no hosted-harness path routes a rawCapture shell call through CoreToolScheduler.observeProducerOutput â I confirmed the singlâĶïž"agent reverse-audit (round 1)"ïždid not finish enumerating every production sendMessageStream caller (Session.ts, hosted-workspace-tool-turn.ts, hosted-harness-session.ts, managed-runtime-ouâĶïž"agent reverse-audit (round 4)"ïždid not execute npx vitest run src/tools/truncation.test.ts / tool-response-finalizer.test.ts , so the new cases are verified by reading against the implemenâĶã
æŠåŪĄæĨïžåååŪĄčŪĄââčŊåŪĄæķéīéĒįŪäļčķģïžæŠč―åžå§įŽŽ 5 č―Ūã
æķæå§ŋæäļåŧķåïžįŽŽ 10 č―Ūïžééŧæïžââå·ēčŪ°å―ïžæŽč―ŪäļčĶæąäŋŪæđïžå ķäļ 1 æĄ Critical æå ķåĪąčīĨæđåäļåŊđį §åšįšŋåŧķåââfails-closed äļ new-surfaceïžæŠčŪĪčŊäŧŧä―éčŊŊįŧæïžäļ merge base æĒæ čŊĨåč―éĒäđæ čŊĨįžšé·ââä―äļšåįŧå·Ĩä―čŪ°å―åĻ findings å·Ĩäŧķäļïžå ą 32 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
æķææ
åĩïžįŽŽ 10 č―Ūååļäš 4 æĄčĄå
čŊčŪšïžå
ķäļ 3 æĄæŊéĶæŽĄæåšïžäļäļč―Ūååļäš 2 æĄïžå
ķäļ 1 æĄéĶæŽĄæåšïžãåį°ååĪåå°åäļæđæäŧķïžpackages/core/src/core/coreToolScheduler.tsïžįŽŽ 5 č―Ūå·ēåščŋåį°ïžæŽč―Ūåæ 2 æĄïžãæ°åį°į䚧åšéåšĶæēĄæäļéãäļäļŠäļæåįå
åžåį°įį°ïžéåļļæåģįéæĄäŋŪåĪåŠåĻåĪįåäļæ đå įåŪäūââå
åŪä―åđķåĪįčŊĨæ đå ïžææįŽįŦįį°ææåįŽį PRïžéåļļæŊéæĄäŋŪåĪæīåŋŦįŧæåūŠįŊãæåĐä―äŋŪåĪææäļæđãéŠčŊååæĻéïžåŊäŧĨéŋå
åūŠįŊååĪæĻåŊžåäļįŧåį°ïžæŽ PR įčŊåŪĄå·ēč§Ģæäļš critical ååļäļéãïžäŧ
äļšč§åŊââæŽč―ŪčŊåŪĄæŠå æĪæĢįäŧŧä―å
åŪđãïž
æšåķåĨåš·ïžæŽč―ŪæŠč―åđēåæķå°ūïžå čæĢįäšåĒééįđïžčåŪæĒåĪå°įéĢäļč―ŪäđæēĄæįäļæŽč―ŪåŊįĻįéįđââčĶäđåŪå ĻæēĄæãčĶäđæēĄæčŪĪčŊč ãčĶäđįąæŽč―ŪčŋčĄčšŦäŧ―äđåĪįčšŦäŧ―čŪĪčŊãčĶäđčĒŦæŽč―Ūįč·åæįŧæč§ĢæäļšåĪīæäšĪââå æĪäļäļæŽĄčŊåŪĄå°éčŊŧæīäļŠ diffïžéĪéæĒåĪæĩįĻææŽč―Ūč―ä―ŋįĻįæīæĐčŠæéįđåŦæĨå°æŽč―ŪįäļįåŪæīå·Ĩä―æļ åäļïžåđķäžäļįīåĶæĪïžįīå°æäļč―Ūįæ čŪ°éæ°åļĶäļéįđïžæč―å°įåŦæĨč―čĒŦčŋčĄčŊĨč―ŪįčŊåŪĄä―ŋįĻãïžäŧ éčŋ°ïžäļæŪæĪčĄåĻââčŋäļæđåæŽč―Ūååļįäŧŧä―å åŪđãïž
æŪä―éĢéĐïžæŽåūŠįŊåĪäš persistently-critical å―Ēæââäļäļč―Ūå·Ĩä―æļ
åäļį Critical æŽč―ŪäūįķååĻïžæŽč―Ū 4 æĄ CriticalïžïžéĶæŽĄåį°įéįæēĄæäļéïžæŽč―Ū 3ïžäļäļč―Ū 1ïžïžäļæŠåģ Critical į§ŊåæēĄæåå°ãseverity floor æ æģä―ŋå
ķæķæãåŧščŪŪïžland-with-residual-riskââåšåĢæŊ maintainer įéĢéĐæĨååģåŪïžåå
Ĩåđķæŋæ
æŪä―éĢéĐïžïžčéååžäļč―ŪčŊåŪĄãäūčŊĨåģåŪä―ŋįĻįæŪä―éĢéĐæļ
åïžmaintainer åĄŦåïžïžææŊæĄæŠåģ Critical ååšãæŧåŧéĒ · æŧåŧč
äūčĩæ§ Â· å―ąåčåīãäļæ ãäŧ
äļšåŧščŪŪââäļéŧææŽæŽĄčŊåŪĄã
â qwen3.8-max via Qwen Code /review (v0.25.2)
| const provenanceByCall = new Map( | ||
| completedCalls.map((call) => [ | ||
| call.request.callId, |
There was a problem hiding this comment.
[Critical] R10-1: [certifies-falsely] [new-surface] provenanceByCall captures a live, mutable ToolOutputProvenance object and re-attaches it after the hooks (L7768), but updateToolOutputBudget mutates that same object in place and never clears truncated/budget/budgetSource â so a batch-budget mark written by an earlier or discarded `a
Failure scenario: getToolOutputBatchBudget() is finite and PostToolBatch is enabled. The pre-hook pass at L7669 (applyBatchOutputBudget â finalizeToolResponses) persists/reduces call A; replaceTextSlots builds the reduced part with { ...part, functionResponse: {...} } (tool-response-finalizer.ts:302-312), and object spread copies the provenance Symbol by reference, so the reduced part and the original part â and the entry in provenanceByCall â all point at one object. updateToolOutputBudget([part], budget, 'batch') (tool-response-finalizer.ts:513) then sets truncated = true, budget = B, budgetSource = 'batch' on it permanently. A PostToolUse/PostToolBatch hook then replaces call A's response with a small hook-authored body; the re-attach at L7768 stamps that already-marked object onto the replacement. logToolCall(new ToolCallEvent(...)) at L7818 emits truncated: true, applied_budget: B, budget_source: 'batch' (types.ts:270-275) and the injection event at llm-chat.ts:5526-5533 repeats it â for a body no budget ever touched. Second, hook-free trigger: the abort-retry loop calls applyBatchOutputBudget twice over the same withoutHookContext parts (L7787 and L7797); if pass 1 exceeds the budget and reduces, and pass 2 (fewer hook contexts) fits so enforceFunctionResponseBudget returns entries unchanged, the submitted body is unreduced while its provenance still certifies truncated: true, budget_source: 'batch'. This is the exact misreport class the PR exists to remove.
Witness:
a **60,000-char body submitted whole** carries `truncated:true, budget:100000, budgetSource:'batch'` â it certifies a cut to a budget it is already under. Flip (same probe, same real code, snapshot-at-capture instead of the live reference): `P00 LIVE map re-attach for b: {..."truncated":true,"budget":100000,"budgetSource":"batch"}` vs `P00 SNAP map re-attach for b: {..."truncated":false}`. Identity of the mutated object reaching back into the map: `same object? true`; `FINALIZER mutated the INPUT entries in place (b): {..."truncated":true,"budget":100000,"budgetSource":"batch"}`.
Suggested fix: Snapshot at capture â provenanceByCall should hold a structural copy (const p = ...find(Boolean); return p ? { ...p } : undefined) so later in-place marks cannot reach back into the map; and/or make updateToolOutputBudget copy-on-write (attachToolOutputProvenance([part], { ...provenance, truncated: true, budget, budgetSource }) and reassign the slot) so a mark lands only on the parts that pass actually reduced.
The fix must not violate this existing fact: Provenance identity is load-bearing in two places a copy-per-part fix would break: llm-chat.ts:5505-5512 groups parts with new Map<ToolOutputProvenance, Part[]>() and dedups sends through this.injectedToolResults.has(provenance) â one object per tool call is what makes all of a call's parts a single injection event and a single dedup key; and enforceFunctionResponseBudget decides what to stamp with if (part !== entries[index].responseParts[partIndex]) (tool-response-finalizer.ts:349-351), which requires the reduced part to stay a distinct object from its input.
Acceptance criterion: packages/core/src/core/coreToolScheduler.test.ts â a case that configures a finite getToolOutputBatchBudget, enables PostToolBatch, makes the pre-hook budget pass reduce call A, then has the PostToolBatch hook replace call A's response, and asserts the ToolCallEvent logged for A has truncated === undefined and applied_budget === undefined. Remove the snapshot / copy-on-write and it goes red. A companion case for the L7787/L7797 double pass (pass 1 reduces, an abort drops a hook context, pass 2 fits) pins the same invariant without hooks replacing anything. Please prove it by mutation â remove the fix, run that test, confirm it goes red.
äļæčŊīæ
éŪéĒïžprovenanceByCall æč·įæŊæīŧįåŊå ToolOutputProvenance åŊđ蹥ïžåđķåĻ hook äđåïžL7768ïžéæ°čīīåïžč updateToolOutputBudget äžå°ąå°äŋŪæđåäļäļŠåŊđ蹥ïžäļäŧäļæļ
éĪ truncated/budget/budgetSourceãå æĪįąæīæĐįãæå·ēčĒŦäļĒåžįæđééĒįŪåå
Ĩįæ čŪ°ïžäžåæīŧå°äļäļŠéĒįŪäŧæŠč§ĶįĒ°čŋįæĢæäļã
č§ĶååšæŊïžgetToolOutputBatchBudget() is finite and PostToolBatch is enabled. The pre-hook pass at L7669 (applyBatchOutputBudget â finalizeToolResponses) persists/reduces call A; replaceTextSlots builds the reduced part with { ...part, functionResponse: {...} } (tool-response-finalizer.ts:302-312), and object spread copies the provenance Symbol by reference, so the reduced part and the original part â and the entry in provenanceByCall â all point at one object. updateToolOutputBudget([part], budget, 'batch') (tool-response-finalizer.ts:513) then sets truncated = true, budget = B, budgetSource = 'batch' on it permanently. A PostToolUse/PostToolBatch hook then replaces call A's response with a small hook-authored body; the re-attach at L7768 stamps that already-marked object onto the replacement. logToolCall(new ToolCallEvent(...)) at L7818 emits truncated: true, `applied_budg
åŧščŪŪäŋŪåĪïžåĻæč·æķååŋŦį
§ââprovenanceByCall åšäŋåįŧæååŊæŽïžčäļæŊå
ąäšŦåäļäļŠåŊååŊđ蹥ã
äŋŪåĪåæïžProvenance identity is load-bearing in two places a copy-per-part fix would break: llm-chat.ts:5505-5512 groups parts with new Map<ToolOutputProvenance, Part[]>() and dedups sends through this.injectedToolResults.has(provenance) â one object per tool call is what makes all of a call's parts a single injection event and a single dedup key; and enforceFunctionResponseBudget decides what to stamp with if (part !== entries[index].responseParts[partIndex]) (tool-response-finalizer.ts:349-351), which requires the reduced part to stay a distinct object from its input.
éŠæķæĩčŊïžpackages/core/src/core/coreToolScheduler.test.ts â a case that configures a finite getToolOutputBatchBudget, enables PostToolBatch, makes the pre-hook budget pass reduce call A, then has the PostToolBatch hook replace call A's response, and asserts the ToolCallEvent logged for A has truncated === undefined and applied_budget === undefined. Remove the snapshot / copy-on-write and it goes red. A companion case for the L7787/L7797 double pass (pass 1 reduces, an abort drops a hook context, pass 2 fits) pins the same invariant without hooks replacing anything.
â qwen3.8-max via Qwen Code /review (v0.25.2)
There was a problem hiding this comment.
Verified REAL at head 0d5bc72bfc â mechanism confirmed by reading the code â but left UNRESOLVED as a merge gate: the fix is a provenance object-model decision, not a mechanical patch.
Confirmed on the live head
updateToolOutputBudgetmutates in place and never clears:packages/core/src/tools/tool-output-size.ts:112-118(provenance.truncated = true; provenance.budget = budget; provenance.budgetSource = source;).provenanceByCallcaptures the live object, not a copy:coreToolScheduler.ts:7611-7618(...map(getToolOutputProvenance).find(Boolean)).- Ordering holds: re-attach at
coreToolScheduler.ts:7768-7780runs after hooks, andapplyBatchOutputBudgetruns again at:7787.
Why I am not taking the suggested one-liner ({ ...p } at capture)
The capture at :7611 happens before the pre-hook budget pass at :7669. A pristine snapshot therefore discards pass 1's legitimate mark for every call hooks did not replace; if pass 2 at :7787 then fits and enforceFunctionResponseBudget returns entries unchanged, the submitted body keeps pass 1's cut while its provenance reports truncated: false. Same certifies-falsely class, opposite direction â so the snapshot is not a safe default.
What needs a ruling: is ToolOutputProvenance a live per-call accumulator (then pass-1 marks must be re-derived, not snapshotted) or an immutable per-pass record (then the identity contract must be re-plumbed)? The finding itself notes identity is load-bearing at llm-chat.ts:5505-5512 (new Map<ToolOutputProvenance, Part[]>() + injectedToolResults.has(provenance) dedup) and at enforceFunctionResponseBudget's part !== entries[index].responseParts[partIndex].
Same root as R10-2 (thread at coreToolScheduler.ts:7769): one shared provenance object describing a body hooks have since changed. Please adjudicate the two together.
äļæ
å·ēåĻ head 0d5bc72bfc æ ļåŪæįŦïžä―åŧæį unresolved å―ååđķéĻįĶïžäŋŪæģæŊå
ģäš provenance åŊđ蹥æĻĄåįčĢåģïžäļæŊæšæĒ°čĄĨäļã
- å°ąå°äŋŪæđäļäŧäļæļ
éĪïž
tools/tool-output-size.ts:112-118ã - æč·įæŊæīŧåŊđ蹥åžįĻïž
coreToolScheduler.ts:7611-7618ã - æķåšæįŦïž
:7768-7780éčīīåĻ hook äđåïž:7787åč·äļæŽĄæđééĒįŪã
äļéįšģåŧščŪŪįãæč·æķåŋŦį
§ãïžæč·įđ :7611 åĻ pre-hook éĒįŪ :7669 äđåïžįšŊååŋŦį
§äžæ pass 1 åæģįčĢåŠæ čŪ°äļčĩ·äļĒæïžčĨ pass 2ïž:7787ïžæĪæķååĨ―æūåūäļãenforceFunctionResponseBudget åæ ·čŋåïžæäšĪįæĢæäŧæŊ pass 1 čĢčŋįïžč provenance æĨ truncated: false ââ åäļįąŧ certifies-falselyïžåŠæŊæđåįļåã
éčĶčĢåģįæŊïžToolOutputProvenance å°åšæŊãæŊäļŠ call įæīŧįīŊå åĻãïžéĢ pass-1 įæ čŪ°čĶéæ°æĻåŊžïžäļæŊåŋŦį
§ïžčŋæŊãæŊčķäļåŊåčŪ°å―ãïžéĢ llm-chat.ts:5505-5512 įåŧéčšŦäŧ―åĨįšĶčĶéæ°æĨįšŋïžãäļ coreToolScheduler.ts:7769 éĢæĄåæ đïžčŊ·äļčĩ·čĢã
| completedCalls = completedCalls.map((call) => { | ||
| const provenance = provenanceByCall.get(call.request.callId); |
There was a problem hiding this comment.
[Critical] R10-2: [certifies-falsely] [new-surface] The re-attach stamps the producer's provenance â including its rawSize â onto hook-appended context that withPostToolBatchAdditionalContext has already folded into the same functionResponse, so a batch-budget cut of the hook text alone marks the tool's own untouched output as truncated and rep
Failure scenario: withPostToolBatchAdditionalContext (L7747) runs before this map and routes through appendTextToFunctionResponse, which returns { ...part, functionResponse: { ...response, output: ${currentText}\n\n${text} } } (tool-hook-context.ts:95-105) â spread preserves the provenance Symbol, so the 300 KB of hook context now lives inside the part carrying the producer's rawSize. Concretely: read_file returns 2 KB (rawSize.chars = 2048); a PostToolBatch hook adds 300 KB of additionalContext to the last call; getToolOutputBatchBudget() is 100 000, so applyBatchOutputBudget at L7787 reduces that slot and updateToolOutputBudget sets truncated = true, budget = 100000, budgetSource = 'batch' on the shared object. The read_file ToolCallEvent then reports truncated: true, applied_budget: 100000, budget_source: 'batch' although its own 2 KB body was submitted whole, and the injection event reports raw_content_length: 2048, injected_content_length: ~100000, truncated: true â a record stating the output was shrunk from 2 KB to 100 KB. PreToolUse contexts appended later by appendHookContexts (L7787) hit the same part for the same reason.
Witness:
not run for the end-to-end replay â the closest capability was a `vitest` probe in `llm-chat.test.ts` driving `onPreparedRequest` â `options.preparedRequest`; it needs a written test file, and `scratch-tree` refused (`available: false`), so the chat fixture could not be seeded without editing the shared worktree. Both halves the claim rests on are measured or quoted above.
Suggested fix: Keep hook-appended context out of the part that carries the producer's rawSize â append it as its own part (or under its own provenance record with rawSize: undefined) so a budget cut of the context cannot mutate the producer's truncated/budget, and so raw_content_length and injected_content_length describe the same producer body.
The fix must not violate this existing fact: appendHookContexts' contract states the context "is joined with a blank line and carries no marker, so every consumer of responseParts â including display and transcript projections â sees it as part of the tool output" (coreToolScheduler.ts:1441-1445), and withPostToolBatchAdditionalContext targets only the last call (coreToolScheduler.ts:1400-1408) â a fix that moves context to a separate part must preserve that display/transcript projection and the last-call targeting.
Acceptance criterion: packages/core/src/core/coreToolScheduler.test.ts â finite getToolOutputBatchBudget(), a small tool result, a large PostToolBatch additionalContext; assert the small tool's ToolCallEvent.truncated is not true and that raw_content_length >= injected_content_length is not inverted for that call. Removing the separation makes it red. Please prove it by mutation â remove the fix, run that test, confirm it goes red.
äļæčŊīæ
éŪéĒïžéæ°čīīåäžæį䚧č
įæĨæšäŋĄæŊïžåŦå
ķ rawSizeïžįå° hook čŋ―å įäļäļæäļïžč withPostToolBatchAdditionalContext å·ēæčŋäšäļäļææå čŋåäļäļŠ functionResponseãäšæŊäļæŽĄäŧ
éåŊđ hook ææŽįæđééĒįŪčĢåŠïžäžæå·Ĩå
·čŠčšŦæŠčĒŦč§ĶįĒ°įčūåšæ čŪ°äļšå·ēæŠæã
č§ĶååšæŊïžwithPostToolBatchAdditionalContext (L7747) runs before this map and routes through appendTextToFunctionResponse, which returns { ...part, functionResponse: { ...response, output: ${currentText}\n\n${text} } } (tool-hook-context.ts:95-105) â spread preserves the provenance Symbol, so the 300 KB of hook context now lives inside the part carrying the producer's rawSize. Concretely: read_file returns 2 KB (rawSize.chars = 2048); a PostToolBatch hook adds 300 KB of additionalContext to the last call; getToolOutputBatchBudget() is 100 000, so applyBatchOutputBudget at L7787 reduces that slot and updateToolOutputBudget sets truncated = true, budget = 100000, budgetSource = 'batch' on the shared object. The read_file ToolCallEvent then reports truncated: true, applied_budget: 100000, budget_source: 'batch' although its own 2 KB body was submitted whole, and the inject
åŧščŪŪäŋŪåĪïžčŪĐ hook čŋ―å įäļäļæäļčĶčŋå
Ĩæŋč――į䚧č
rawSize įéĢäļŠ partã
äŋŪåĪåæïžappendHookContexts' contract states the context "is joined with a blank line and carries no marker, so every consumer of responseParts â including display and transcript projections â sees it as part of the tool output" (coreToolScheduler.ts:1441-1445), and withPostToolBatchAdditionalContext targets only the last call (coreToolScheduler.ts:1400-1408) â a fix that moves context to a separate part must preserve that display/transcript projection and the last-call targeting.
éŠæķæĩčŊïžpackages/core/src/core/coreToolScheduler.test.ts â finite getToolOutputBatchBudget(), a small tool result, a large PostToolBatch additionalContext; assert the small tool's ToolCallEvent.truncated is not true and that raw_content_length >= injected_content_length is not inverted for that call. Removing the separation makes it red.
â qwen3.8-max via Qwen Code /review (v0.25.2)
There was a problem hiding this comment.
Verified REAL at head 0d5bc72bfc; left UNRESOLVED as a merge gate â the fix changes model-visible output structure and shares its root with R10-1.
Confirmed: withPostToolBatchAdditionalContext routes through appendTextToFunctionResponse, which returns { ...part, functionResponse: { ...response, output: ... } }. Object spread preserves the provenance symbol, so hook context lands inside the part carrying the producer's own rawSize. Combined with the in-place mark at tools/tool-output-size.ts:112-118, a budget cut of hook text alone flips the producer's truncated / budget / budgetSource.
Why not fixed here: moving the context to its own part reverses the contract this file states at coreToolScheduler.ts:1441-1445 â "joined with a blank line and carries no marker, so every consumer of responseParts â including display and transcript projections â sees it as part of the tool output" â and must preserve last-call targeting (:1400-1408). That is a product-behaviour decision spanning tool-hook-context.ts + coreToolScheduler.ts + tests, not a mechanical patch, and any version of it also has to answer R10-1's object-model question first.
Same root as R10-1 (thread at coreToolScheduler.ts:7614): one provenance object serving a body that is no longer purely the producer's. Please adjudicate the two together.
äļæ
å·ēåĻ head 0d5bc72bfc æ ļåŪæįŦïžåŧæį unresolved å―ååđķéĻįĶã
æšåķįĄŪčŪĪïžappendTextToFunctionResponse įĻåŊđ蹥åąåžčŋåæ° partïžåąåžäžäŋį provenance symbolïžäšæŊ hook äļäļæč―čŋäšæŋč――į䚧č
čŠå·ą rawSize įéĢäļŠ partïžåå å tools/tool-output-size.ts:112-118 įå°ąå°åå
ĨïžäļæŽĄåŠéåŊđ hook ææŽįéĒįŪčĢåŠå°ąäžįŋŧč―Žį䚧č
į truncated / budget / budgetSourceã
äļåĻæŽ PR äŋŪįįįąïžæäļäļææŠæįŽįŦ part äžåč―Ž coreToolScheduler.ts:1441-1445 æåįåĨįšĶïždisplay / transcript æå―ąé―æåŪå― tool output įäļéĻåïžïžčŋčĶäŋä― :1400-1408 į last-call åŪåïžč·Ļ tool-hook-context.ts + coreToolScheduler.ts + æĩčŊïžåąäš§åčĄäļšåģįïžäļčĶå
åį R10-1 įåŊđ蹥æĻĄåéŪéĒãäļ coreToolScheduler.ts:7614 éĢæĄåæ đïžčŊ·äļčĩ·čĢã
| const header = | ||
| this.buildMetadataHeader(entry) + | ||
| (entry.persistedTextPath |
There was a problem hiding this comment.
[Critical] R10-3: [certifies-falsely] [regression] web_fetch composes its model-visible body as metadata header + producer envelope, so the body never starts with TOOL_OUTPUT_TRUNCATED_PREFIX â every recognizer in the tree is startsWith-anchored, and the per-call-unique spill path the new header line adds is therefore fingerprinted verbati
Failure scenario: Three concrete consequences, all verified against the reviewed commit. (1) Loop guard disabled. buildMetadataHeader leads with URL: âĶ (web-fetch.ts:326-331), so stripPersistenceEnvelope (loopDetectionService.ts:306-307) returns the body unchanged instead of collapsing it to <persisted-stub>sha256:<digest>. The spill name is web_fetch_${crypto.randomBytes(6).toString('hex')}.output (truncation.ts:363), so a page >100 000 chars re-fetched after the 15-minute CACHE_TTL_MS expires (web-fetch.ts:71, 347) â or any usedHttpFallback fetch, which is never cached (web-fetch.ts:557) â yields a body differing only in that random path. extractResultText's doc states its whole purpose: "identical underlying results fingerprint identically no matter where they were persisted" (loopDetectionService.ts:608-611). An unchanged dashboard polled every 20 minutes now reads as "changed every time" â the exact #9450 regression the FULL_OUTPUT_DIGEST_LABEL mechanism exists to prevent, and the digest is present in the body, just unreachable. (2) Duplicate spill. truncateLlmContent's idempotency guard is also startsWith (truncation.ts:455), and web_fetch declares no maxOutputChars (pinned by this diff's own new test, expect(tool.maxOutputChars).toBeUndefined()), so the scheduler bounds it at DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000 (config.ts:986). On the side-query-failure path the diff returns ~99 000 chars (header + the previewChars: MAX_CONTENT_CHARS - 2000 envelope, asserted toBeGreaterThan(90000) by the new test), which is re-truncated: a second ~99 KB file is written, trackToolResultBytes is charged a second time for the same page, and the model receives nested envelopes â the harm truncation.ts:51-53 names explicitly ("would nest headers and spill a duplicate file"). (3) Retention audit misflag. alreadyTruncated at tool-result-retention.ts:127 is also startsWith, so a producer-bounded web_fetch body counts toward oversizedResultCount as a truncation bypass.
Witness:
the run above â two identical-content spills named `web_fetch_c3390e8bf8ae.output` / `web_fetch_a78c770aa09f.output`, and `model-visible body starts with a stub prefix = false` against `['<persisted-output>', 'Output too large (', 'Tool output was too large and has been truncated']`. Not run: the `LoopDetectionService` consecutive-identical guard end to end (`extractResultText` is a private static, and driving the service needs a probe file in an isolated tree, which `scratch-tree available: false` foreclosed); the fingerprint divergence is measured at its inputs and quoted from `:306â307`.
Suggested fix: Keep the sentinel at position 0 of the returned body. Simplest: on the entry.reduced paths return the envelope first and the metadata header after it (llmContent: ${entry.content}\n\n${header}), and keep entry.content as the side-query input as today. Because a leading sentinel also makes truncateLlmContent return the body unchanged, pair that with a bound the tool owns â either declare maxOutputChars on WebFetchToolInvocation or use a smaller previewChars for the returned body than for the side-query input, so a sentinel-led body cannot ship ~99 000 chars into a 25 000-char context budget.
The fix must not violate this existing fact: Recognition is leading-anchored by design: "honoring a marker found mid-string would let quoted content collapse or vary the fingerprint, so only shapes that START with a producer prefix are treated as stubs (issue #9450)" (loopDetectionService.ts:250-254) â so the fix must move the sentinel, not teach the normalizer to search mid-string. And leading the sentinel disables the scheduler's re-bound (if (content.startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX)) return { content }, truncation.ts:455), against DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000 (config.ts:986).
Acceptance criterion: packages/core/src/services/loopDetectionService.test.ts â two web_fetch bodies with identical content but different web_fetch_<hex>.output paths must produce the same extractResultText fingerprint (assert via the consecutive-identical guard, which must fire). It goes red with the header first. Plus packages/core/src/tools/web-fetch.test.ts: assert the failure-path llmContent satisfies startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX) and is within the tool's own declared budget â red today. Please prove it by mutation â remove the fix, run that test, confirm it goes red.
äļæčŊīæ
éŪéĒïžweb_fetch ææĻĄååŊč§æĢæįŧčĢ
äļšãå
äŋĄæŊåĪī + į䚧č
äŋĄå°ãïžå æĪæĢæäŧäļäŧĨ TOOL_OUTPUT_TRUNCATED_PREFIX åžåĪīââčæ äļææčŊåŦåĻé―æŊ startsWith éåŪįãå äđæŊæŽĄč°įĻåŊäļįč―įč·ŊåūčĒŦåčŋåĪīéĻïžčŊĨæĢæäžčĒŦéåæįšđåãčĒŦåæŽĄæŠæãåđķåæŽĄč―įã
č§ĶååšæŊïžThree concrete consequences, all verified against the reviewed commit. (1) Loop guard disabled. buildMetadataHeader leads with URL: âĶ (web-fetch.ts:326-331), so stripPersistenceEnvelope (loopDetectionService.ts:306-307) returns the body unchanged instead of collapsing it to <persisted-stub>sha256:<digest>. The spill name is web_fetch_${crypto.randomBytes(6).toString('hex')}.output (truncation.ts:363), so a page >100 000 chars re-fetched after the 15-minute CACHE_TTL_MS expires (web-fetch.ts:71, 347) â or any usedHttpFallback fetch, which is never cached (web-fetch.ts:557) â yields a body differing only in that random path. extractResultText's doc states its whole purpose: "identical underlying results fingerprint identically no matter where they were persisted" (loopDetectionService.ts:608-611). An unchanged dashboard polled every 20 minutes now reads as "changed ev
åŧščŪŪäŋŪåĪïžčŪĐåĻå
ĩä―äščŋåæĢæįįŽŽ 0 ä―ïžåĻ entry.reduced č·Ŋåūäļå
čŋåäŋĄå°ãåæĨå
äŋĄæŊåĪīïžïžįąäšååŊžåĻå
ĩäžčŪĐ truncateLlmContent åæ ·čŋåæĢæïžčŋéé
äļäļŠå·Ĩå
·čŠæįäļįïžåĻ WebFetchToolInvocation äļåĢ°æ maxOutputCharsïžæåŊđčŋåæĢæä―ŋįĻæŊäū§æĨčŊĒčūå
Ĩæīå°į previewCharsïžã
äŋŪåĪåæïžRecognition is leading-anchored by design: "honoring a marker found mid-string would let quoted content collapse or vary the fingerprint, so only shapes that START with a producer prefix are treated as stubs (issue #9450)" (loopDetectionService.ts:250-254) â so the fix must move the sentinel, not teach the normalizer to search mid-string. And leading the sentinel disables the scheduler's re-bound (if (content.startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX)) return { content }, truncation.ts:455), against DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000 (config.ts:986).
éŠæķæĩčŊïžpackages/core/src/services/loopDetectionService.test.ts â two web_fetch bodies with identical content but different web_fetch_<hex>.output paths must produce the same extractResultText fingerprint (assert via the consecutive-identical guard, which must fire). It goes red with the header first. Plus packages/core/src/tools/web-fetch.test.ts: assert the failure-path llmContent satisfies startsWith(TOOL_OUTPUT_TRUNCATED_PREFIX) and is within the tool's own declared budget â red today.
â qwen3.8-max via Qwen Code /review (v0.25.2)
There was a problem hiding this comment.
Verified REAL at head 0d5bc72bfc, [regression] confirmed; left UNRESOLVED as a merge gate because the fix must reverse two contracts this PR's own new tests pin.
Confirmed: the header/envelope ordering is new in this diff â web-fetch.ts:578-582 changed const header = this.buildMetadataHeader(entry) to also append Full extracted text saved to: ${entry.persistedTextPath}, and the spill name is per-call random (web_fetch_${crypto.randomBytes(6).toString('hex')}.output). Every recognizer in the tree is startsWith-anchored (loopDetectionService.ts:306-307, truncation.ts:455, tool-result-retention.ts:127), so the body now matches none of them. Same family as R9-1, which was fixed at truncation.ts:506 in d4136d3358.
Why not fixed here: the suggested reorder puts the sentinel at position 0, which by design disables the scheduler re-bound (truncation.ts:455), so it must be paired with a tool-owned cap. But this PR's own new test pins expect(tool.maxOutputChars).toBeUndefined() and asserts the failure-path body toBeGreaterThan(90000). Deciding which of those two contracts governs is a design call, and it lands on the same web_fetch persistence/envelope contract as the still-open [Blocker] thread at web-fetch.ts:518 (R2-3). Ruling on them separately risks two contradictory answers on one contract â so this should be adjudicated together with R2-3, not before it.
äļæ
å·ēåĻ head 0d5bc72bfc æ ļåŪæįŦïž[regression] æ įūæĢįĄŪïžïžåŧæį unresolved å―ååđķéĻįĶã
įĄŪčŪĪïžåĪīéĻ/äŋĄå°įå
å饚åšæŊæŽ diff æ°åžå
Ĩį ââ web-fetch.ts:578-582 æ const header = this.buildMetadataHeader(entry) æđæčŋčŋ―å Full extracted text saved to: ${entry.persistedTextPath}ïžčč―įåæŊæŽĄč°įĻéæšãæ éææčŊåŦåĻé―æŊ startsWith éåŪïžloopDetectionService.ts:306-307ãtruncation.ts:455ãtool-result-retention.ts:127ïžïžå æĪæĢæäļäļŠé―äļåđé
ãäļ R9-1 åæïžåč
å·ēåĻ d4136d3358 äŋŪåĻ truncation.ts:506ã
äļåĻæŽ PR äŋŪįįįąïžåŧščŪŪįč°åšäžæåĻå
ĩæūå°įŽŽ 0 ä―ïžčæčŪūčŪĄčŋäžå
ģæč°åšĶåĻįäšæŽĄæķåĢïžtruncation.ts:455ïžïžåŋ
éĄŧåæķé
äļäļŠå·Ĩå
·čŠæįäļįïžä―æŽ PR čŠå·ąįæ°æĩčŊéį expect(tool.maxOutputChars).toBeUndefined()ïžčŋæčĻåĪąčīĨč·ŊåūæĢæ toBeGreaterThan(90000)ãčŋäļĪäļŠåĨįšĶåŠäļŠä―åæŊčŪūčŪĄåģįïžäļäļäŧæŠéįŊį web-fetch.ts:518 [Blocker]ïžR2-3ïžč―åĻåäļäŧ― web_fetch æäđ
å/äŋĄå°åĨįšĶäļ ââ ååžčĢæįŧåšäļĪäļŠäšįļįįūįæĄįéĢéĐïžåšäļ R2-3 äļåđķčĢåģã
|
Closing this PR in favor of a three-part stacked split. The branch Split mapping
Shared files were split at hunk level: Equivalence with this PRThe stacked tip of #14547 is tree-identical to a reference tree built by applying Unresolved thread disposition
All previously resolved/outdated threads map to the same code, now in the slice PRs above. Closing this PR; discussion continues on the stack. |
Pull request was closed
Remove the dead raw-size measurement on the MCP error path (both consumers provably discard it; the error body is replaced and reports an unknown raw size per #13652 R4-1), move the truncation/finalizer/injection size measurements inside the existing telemetry guards so an estimator throw can never fail a truncation pass or model request (design invariant), merge the duplicate compactionInputSlimming imports, and document the per-attempt throwIfAborted retry semantics in the PR body and both design docs.
) * feat(core): account tool result sizes from producer to injection Split part 1 of #13652: measure every tool result at the producer, through persistence, per-tool and combined budgets, and the batch finalizer, carry that provenance to the injection point, and emit tool_result_size / tool_result_injected telemetry with raw vs injected sizes and budget attribution. Producers declare honest raw sizes (unknown when the body was replaced or pre-bounded); restored history and speculation replays are marked so injection events are not double-counted. * fix(core): close review gaps in tool result size accounting Remove the dead raw-size measurement on the MCP error path (both consumers provably discard it; the error body is replaced and reports an unknown raw size per #13652 R4-1), move the truncation/finalizer/injection size measurements inside the existing telemetry guards so an estimator throw can never fail a truncation pass or model request (design invariant), merge the duplicate compactionInputSlimming imports, and document the per-attempt throwIfAborted retry semantics in the PR body and both design docs. * test(cli): compare projected tool result parts without in-memory symbol The merge of origin/main in f3d8f4f reverted this assertion to the pre-PR form, so it compares the persisted projection against in-memory parts that carry Symbol(tool-output-provenance). Round-trip the expected parts through JSON as the PR originally did, so symbol-only annotation metadata is not compared across the persistence boundary. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmvepo0oyh9 * fix(cli): drop in-memory provenance symbol at the hosted CSI boundary convertHostedCsiToolResult returned parts carrying the internal Symbol(tool-output-provenance), which the hosted CSI evidence test compares by value. A symbol cannot survive the broker wire anyway, so return a plain functionResponse part. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmveqqllohc * fix(core): measure undeclared producer size in the shared converter convertToFunctionResponse treated an undeclared rawOutputSize as unknown, so the ACP and speculation paths reported raw_content_length/truncated as undefined for ordinary producers while the scheduler measured them. Every self-bounding producer now declares null, so undeclared means a complete body: measure it, and keep a declared null unknown. Per-layer truncation events also carry the call id the scheduler already holds, and the RUM size event enumerates its fields so call_id/prompt_id stay log-only as the design requires. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvf1t9h7hy * fix(core): attribute send-guard cuts and make result-size measurement total * fix(cli): compare CSI refusals against their durable shape convertToFunctionErrorResponse now attaches the in-memory Symbol(tool-output-provenance) to the rebuilt part, while the persisted history row it is compared against has been through JSON and carries no symbol. Rebuild the refusal part from its durable functionResponse before the isDeepStrictEqual comparison and before recording it, and compare the replayed Batch expectation in hosted-workspace-tool-turn the same way the rest of that suite already normalizes parts. Verified locally: hosted-csi-tool-turn.test.ts 52/52, and both "replays a Batch an earlier release saved on a refused replacement" cases. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmvfkqugqj0 --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
âĶable Split part 2 of #13652: truncateAndSaveToFile keeps the caller's preview budget authoritative (a compact JSON sample is only used inside its own size class), leads every bounded body with the truncation sentinel, embeds the full-output sha256 for the loop guards, and adds a read_file paging hint. Spill writes reserve session bytes up front with rollback on failure, and a disk-budget skip still yields a sentinel-led preview that claims no file.
âĶcoverable Split part 3 of #13652: web_fetch no longer pre-truncates pages with its private 100k truncateText before the scheduler's 25k budget cuts the body again. The producer persists the full extracted text once, marks the cache entry as reduced, and the model-visible header points at the saved file so read_file can page the complete text back in. The persistence gate treats web_fetch like every other tool (its metadata header never starts with the truncation sentinel), and preapproved-markdown passthrough only fires when nothing was reduced.
âĶable (#14546) * feat(core): honor preview budgets and make truncated previews recoverable Split part 2 of #13652: truncateAndSaveToFile keeps the caller's preview budget authoritative (a compact JSON sample is only used inside its own size class), leads every bounded body with the truncation sentinel, embeds the full-output sha256 for the loop guards, and adds a read_file paging hint. Spill writes reserve session bytes up front with rollback on failure, and a disk-budget skip still yields a sentinel-led preview that claims no file. * fix(core): close preview-envelope review gaps - buildStub counts lines without splitting, so the >50 MB stub path no longer allocates a string per line. - A spill skipped for the per-file persistence limit no longer reports "session disk budget exhausted"; the two skip reasons stay distinct as they already do in persistToolResult. - A failed spill write now returns the same sentinel-led envelope as every other bounded body instead of a bare note. - The truncation description only claims the CONTENT TRUNCATED marker when the separator actually fits the budget. - The structured JSON sample is not used for `keep: 'tail'`, which it would answer with head content. - Adds the escaped-sample success case, so the escape-then-measure order is covered in both directions. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * style(core): format truncation.ts to satisfy prettier gate The envelope() call added for the unsaved-output note was hand-wrapped in a shape prettier 3.6.1 rejects, so `Lint & Static` failed at the `scripts/lint.js --prettier` step on head 503451f. Reformatting only; no behaviour change. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmvg66g8bk9 --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
âĶcoverable Split part 3 of #13652: web_fetch no longer pre-truncates pages with its private 100k truncateText before the scheduler's 25k budget cuts the body again. The producer persists the full extracted text once, marks the cache entry as reduced, and the model-visible header points at the saved file so read_file can page the complete text back in. The persistence gate treats web_fetch like every other tool (its metadata header never starts with the truncation sentinel), and preapproved-markdown passthrough only fires when nothing was reduced.
âĶcoverable (#14547) Split part 3 of #13652: web_fetch no longer pre-truncates pages with its private 100k truncateText before the scheduler's 25k budget cuts the body again. The producer persists the full extracted text once, marks the cache entry as reduced, and the model-visible header points at the saved file so read_file can page the complete text back in. The persistence gate treats web_fetch like every other tool (its metadata header never starts with the truncation sentinel), and preapproved-markdown passthrough only fires when nothing was reduced.
âĶaction (#13599) * feat(core): adapt tool output budgets and account for result sizes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * chore(vscode): regenerate settings schema for tool output defaults The truncation defaults moved to 80000 characters and 2000 lines. settingsSchema.ts feeds them into the generated VS Code settings schema, so the committed schema has to be regenerated or the "Check settings schema is up-to-date" gate fails. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmuy1j2xxcd * fix(core): align output-budget consumers and wire assertions Synchronize the review read limit, retain wrapper-only sizing for missing or unserializable results, and keep runtime provenance out of wire assertions. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): describe structured output previews accurately Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): keep tool result size estimation total on odd payloads estimatePartChars grew a structured-payload branch that billed an absent functionResponse.response as "{}" - two chars on top of the wrapper floor - and threw a TypeError on circular payloads, which aborted retention analysis instead of degrading. Guard the branch on a response that is actually there and fall back to the wrapper floor when JSON.stringify cannot serialize it. Also give the mcp-client Config stub the truncation budget and the project temp dir that the producer layer now reads, so the four invocation-context tests exercise MCP tool execution instead of failing on missing stub members. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmuy3o8oich * refactor(core): narrow to pressure-aware tool budgets (#2566) Result-size accounting for #11536 moves to #13652, and the 80k/2000 default bump is reverted: #2566's triage did not support it, and it pushed ordinary output past the hosted 64 KiB inline ceiling. What remains anchors on this chat's own last usage report and measures headroom against the auto-compaction trigger instead of the hard ceiling, which the default 1M window never reached before compaction. Past the trigger nothing is shrunk instead of flooring at one char, and the pressure budget shares the existing send guard's single pass so a result is never cut twice. The anchor is cleared on setHistory too. * fix(core): repair pressure-aware tool budget composition (#2566) Address the review findings on the send-boundary tool budget: - Keep a non-finite aggregate batch budget non-finite so the documented "disabled" sentinel is not tightened into the pressure budget. - Fall back to DEFAULT_TOKEN_LIMIT when the route reports no context window, like every sibling threshold consumer. - Scale the per-result floor and the static-capacity comparison by the number of results the shared budget is split across, so a batch of parallel results no longer collapses each of them to a bare stub. - Charge tool output the budget cannot shorten (exempt tools, tool media) to the headroom instead of assuming the budget covers it. - Reuse estimatePromptTokens for the conservative projection. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzmop02ey * test(core): pin the pressure-aware tool budget contracts (#2566) - A disabled aggregate budget leaves results whole. - A batch whose total exceeds the headroom is shrunk, not just one result. - Every parallel result keeps its spill pointer in the floor band. - Exempt tool output is charged to the headroom. - The tighter of the aggregate budget and the headroom wins. - The default window applies when the route reports none. - The dispatch-time invalidation is exercised through a consumer cancel, so the case fails when the clear is removed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzmop02ey * docs(core): state the disabled-aggregate coupling for tool budgets (#2566) The pass is off when the aggregate budget keeps its non-finite sentinel, so say so in both design docs instead of only in the code comment, and give the exempt-output case a decisive margin over the headroom. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzmop02ey * fix(core): budget an exempt tool error against the pressure headroom `isBudgetShrinkablePart` answers "can `enforceFunctionResponseBudget` shorten this part", but it returned false for every part of an exempt tool. `collectTextSlots` only ever exempts the `output` slot - an `error` field is budgeted unconditionally - so an exempt tool reporting a multi-KB error was counted twice: charged whole to the headroom estimate (dropping the pressure budget to the per-result floor) and then cut to that floor by the pass, squeezing innocent neighbours. Report an `error` field as shrinkable before the exemption check, so the same 8k error next to a plain 20k result gets the plain allocation (5,320 of 10,640 each) instead of two 2,000-char stubs. The plan-mode reminder and media-carrying shapes stay inconsistent on purpose: closing those needs per-field billing, not a boolean. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzqz0izf5 * fix(core): align the pressure budget with the finalizer's own slot rules `isBudgetShrinkablePart` and `collectTextSlots` disagreed about which text `enforceFunctionResponseBudget` shortens: - a part carrying nested media was classified unshrinkable while its `output` string was budgeted anyway, which shortened it without charging its media payload to the headroom either way; - an empty `output` counted as shrinkable although `collectTextSlots` yields no slot for it, inflating the per-result floor's multiplier. The predicate now mirrors `collectTextSlots`, and `pressureToolOutputBudget` charges a media-bearing result's nested payload - not its text - to the headroom. Also, when the send guard actually cuts a batch, the cut is what lands in durable history, so the file read cache and the loaded-skill tracking are now invalidated the way `tryCompress` invalidates them. Corrects the anchor JSDoc's claim about which clears are load-bearing, and reuses the hoisted `contextWindowForClamp` binding instead of re-deriving the window. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzzjnorfk * fix(core): keep the send-boundary invalidation inside the owning chat The two clears added when the send guard cuts a batch ran for a forked chat too. A fork is built on the parent's Config, so it shares the parent's file read cache and the parent's single SkillTool tracking instance while holding only a copy of a history slice. An idle speculation whose batch is cut â by the pressure budget or by the pre-existing aggregate guard â therefore wiped the authoritative session's read verdicts and loaded-skill tracking: the parent's next read_file re-emitted bytes it still had, and the next skill invoke re-injected a body still resident in its history. A throwaway fork grew the authoritative session's context, the opposite of what this pass is for. Every other clear in this file carries the same `!this.isForkedChat` guard, and the field's doc records the invariant normatively. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv0ejs4vgg * fix(core): derive the shrinkable-part predicate from the slot walk `isBudgetShrinkablePart` restated `collectTextSlots`' skip rules and had already drifted from them: a response whose whole `output` is the plan-mode lifecycle prefix yields no slot and travels whole, yet the predicate called it shrinkable. `pressureToolOutputBudget` then charged the headroom for text the budget never touches â it is held out of the estimate instead â and raised the per-result floor and the static-capacity comparison with it, so the budget came out more generous than the headroom it protects. The predicate now answers from `collectTextSlots` itself, so prefix protection and empty slots cannot desynchronise again. The new case pins the classification against the cutter's own behaviour. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv0ejs4vgg * test(core): pin the shrunk send against the auto-compaction trigger The #2566 block measured character counts only, so the outcome the feature exists for was unasserted: the gate's own estimate for the send the model receives could cross `auto` while every case stayed green, and the two numbers are coupled only by arithmetic living in two files. `shrinks results to the headroom left before auto-compaction` now also checks the last `tryCompress` call's `precomputedEffectiveTokens` (848,686 against the 850,000 trigger for the 1M window). Deliberately not asserted in the floor band, where the budget may overshoot and the send crosses `auto` on purpose. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv0ejs4vgg * fix(core): preserve tool results when pressure cuts still compact Keep the aggregate-only result when the pressure floor still reaches auto-compaction and the aggregate-only prompt remains below hard. Retain the pressure cut for small-window hard-tier protection. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): preserve pressure-shrunk output within headroom Remove the preview floor, reserve protected input and framing, persist shortened text, and preserve file-read rights during send-boundary eviction. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): invalidate pressure anchor across fixed input changes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix: preserve tool diagnostics in tiny send budgets Defer sub-notice allocations to normal compaction and avoid incomplete artifact paths. Keep zero-budget estimation separate and synchronize both design languages. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): Preserve unresolvable reads before output shrinking Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): keep a real preview when the artifact pointer cannot fit A per-slot allocation between the notice length and the delivered pointer header collapsed the slot to the bare "Tool output truncated." notice: both the content and the recovery path were dropped while the artifact was still written. Budget the preview against the header that is actually delivered, so every shrunk slot keeps full text, a complete pointer, or a real preview. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv2sa7771h * test(core): pin the pointerless preview against a sliced-pointer mutant Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv2sa7771h * fix(core): close two tool-output shrink gaps at the send boundary R8-1: the send-boundary shrink blanks every non-exempt tool result, but the disarm loop only recognised `read_file`. `edit` and `write_file` arm `readResidentInHistory` through `recordWrite`, so a pressured cut of their output left the fast path armed and a later full read returned "unchanged since last read" for bytes the send had replaced with a truncation notice (issue #4239). Reuse the microcompaction site's `FILE_PATH_TOOLS` so both blanking sites disarm the same set. R3-2: the too-small-headroom veto tested `allocation < 22`, but `fitText` spends the allocation on the notice plus its `\n\n` separator first, so 22..24 cleared the veto, persisted an artifact, and then sent a contentless notice whose pointer was also dropped. Veto below notice + separator + one character instead. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv33003h1y * fix(core): release read residency for the executed target, not the model-authored one PreToolUse `updatedInput` replaces the args that actually execute while history keeps the model-authored ones -- the scheduler clones args at intake precisely so those rewrites never leak back into history. The send-boundary shrink derived its release paths from history, so a hook that retargets `read_file(A)` to `read_file(B)` disarmed A while B stayed armed with its output already cut. The next real `read_file(B)` then served the `file_unchanged` placeholder for content no longer quotable from history. The existing blanket fallback did not catch it: with A already cached, `markReadEvictedFromHistory(A)` succeeds, so nothing flagged the batch. Fall closed to the blanket disarm whenever a PreToolUse hook is registered. It only flips `readResidentInHistory`, so prior-read rights survive. The shrink itself is unchanged. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv3x090u38 * fix(core): fail closed on a PermissionRequest hook retarget after a send-boundary cut The send-boundary fallback added after a tool-output shrink only disarmed the file read cache for a PreToolUse hook. A PermissionRequest allow carrying `updatedInput` is applied the same way: coreToolScheduler hands it to rebuildWithPermissionRequestInput, which rewrites the executed args and records the call in hookReplacedInputCallIds just like a PreToolUse rewrite, while history keeps the model-authored path. With no PreToolUse hook registered, nothing triggered the fallback, so the cut read stayed armed and a later re-read of the retargeted file returned a false "unchanged, full content was provided earlier" placeholder for a body that had actually been truncated. Extend the existing condition to also fail closed when a PermissionRequest hook is configured, and name both events in the comment. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv45kw3b3o * fix(core): preserve a cut read batch when a hook could have retargeted it The send-boundary fail-closed added for a hook `updatedInput` rewrite only disarmed this process's FileReadCache. A worker-executed read owns its residency in the worker's own cache (execution-worker.ts builds a fresh Config), and the sole parent-to-worker sync, synchronizeReadCache, fires only on a clearGeneration change, which markAllReadsEvictedFromHistory() does not bump. invalidateReadCache(paths) does reach the worker but carries only the history-derived, model-authored path, so the executed target stayed armed and the next read returned the "unchanged, full content was provided earlier" placeholder for a body this send had cut. Treat such a cut as unresolvable at the selection site instead: when a PreToolUse or PermissionRequest hook is registered and a result was cut, preserve the batch for normal compaction, the same contract the design doc already gives untraceable reads. That makes the post-hoc hook widening unreachable -- guarded === entries whenever it could have fired -- so the two hasHooksForEvent clauses are deleted and the fallback returns to its original `if (unresolvedRead)` form. Collapse the two near-identical disarm tests into one it.each asserting the guarantee that now holds: the batch is preserved and nothing crosses to the execution environment. Reverting the selection-site condition turns both cases red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv4ifusy4d * fix(core): scope the hook-retarget send guard to read-shaped cut results `hookRetargetPossible` was gated on `cutResults.length > 0`, i.e. on any shortened function response. The rationale comment above it is about a PreToolUse/PermissionRequest `updatedInput` retargeting a *read* off every path derivable here, so a shell-only cut batch also disabled the send-boundary cut. With a hook configured and a 1M window at NEAR_AUTO the same shell batch went out at 20,000 characters instead of 10,576, pushing effective tokens back over the auto-compaction threshold this feature exists to stay under, and bypassing the aggregate batch budget. The bypass was silent: on that path `guarded === entries`, so the reduction warning never fired. Scope the operand to read-shaped names using the vocabulary the loop above already applies: `FILE_PATH_TOOLS` (read_file/edit/write_file, which subsumes `ToolNames.READ_FILE`) plus the `tool_call` bridge. No new imports. Adds one witness: a PreToolUse hook with a shell-only cut batch must still shrink. It fails at the previous head (20,000 sent) and passes here (10,576 sent). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv51qarr5b * fix(core): keep the tool-budget version bound to the request snapshot `request.config` is snapshotted once per send, so every retry re-dispatches the same fixed inputs. Re-reading `toolBudgetFixedInputVersion` at dispatch time let a setter that fires during backoff (progressive MCP discovery calls `LlmClient.setTools()`) relabel that stale usage as current, so the equality check in `processStreamResponse` accepted it and re-anchored the tool-result budget on headroom that does not cover the new fixed inputs. Capture the version alongside the snapshot and retain it for retries. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv68licx1e * fix(core): bind the tool-budget anchor to dispatch and disarm the read cache on worker RPC failure R11-1: setHistory/clearHistory cleared the usage anchor but never bumped toolBudgetFixedInputVersion, so a history replacement landing between dispatch and acceptance was re-adopted paired with the pre-replacement token count. Pair the two writes as setSystemInstruction/setTools do. R11-2: the send-boundary shrink's invalidateReadCache catch only warned. When that RPC rejects, nothing moves the FileReadCache generation that synchronizeReadCache re-propagates on, so the next read_file keeps claiming content this send removed. Mirror client.ts and clear() there. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmv8fwg2z5f * fix(core): keep spill pointers atomic under pressure and disarm a fork's own read cuts (#2566) - recover a persisted-output envelope's 'Full output saved to' path at slot collection and route it through fitText's pointer header, so the send boundary pressure cut delivers the artifact path whole or drops to the bare notice â never a path severed mid-string (R12-1, M1). - split the fork guard: a forked chat disarms exactly the read entries its own send-boundary cut invalidated (per-path, invalidateByPath fallback) while unresolvable reads are preserved on forks the same as on parents, and cache-global clears plus loaded-skill tracking stay non-fork only (R12-2, M2). Regression tests built from the reproduced failure arms; the two R12-1 tests were verified mutation-red against the pre-fix finalizer. * style: apply prettier to the design docs brought in by the main merge The Lint & Static lane short-circuited at the freshness gate on this stale branch, so step 31 Run Prettier never ran. With the gate resolvable after merging main, prettier flags the two design docs that came in with the merge. Formatted only those files. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmvdo5pktf4 * fix(core): keep inline recovery handles whole across the pressure cut (#2566) - Carry a spill envelope's 'Full output saved to:' line and a subagent transcript marker as a producer-owned character range, and keep that range whole above the preview instead of promoting a path parsed out of the text into the notice header. The advertised artifact path is again only this pass's persisted file, so a path containing a space, an error-field envelope and the agent marker all survive intact (R12-1). - Cover the text, output and error slots with the same recovery range. - Derive the send-boundary residency disarm from the tools' own 'unescapePath(file_path.trim())' normalisation, and apply the same normalisation at the microcompaction path map, so an @-completion- escaped path names the cache entry it armed (R13-1). Mutation-checked: removing the preservation turns the spaced, error-field and transcript cases red; removing either normalisation turns its regression case red. * fix(core): invalidate the tool-budget anchor when a prepared send restores its config The prepared-request replay swaps in the snapshot's generationConfig, which carries the systemInstruction and tools that the response's usage anchor was measured against. The generator's finally block restored the live config with a bare assignment, leaving that anchor armed, so the next send sized its tool results against a token count from a different fixed input. Invalidate at the restore the way setSystemInstruction, setTools, setHistory and clearHistory already do. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvhhouazml * fix(core): stop measurement-only budget passes certifying a truncation The pressure measurement, the two estimate previews and the cut-point preview all route a real budget through enforceFunctionResponseBudget without adopting its result. Because replaceTextSlots copies parts by spread, the provenance object a result's parts share is the same object, so those passes stamped truncated/applied_budget on the very parts that then ship byte-identical: a full-size result was dispatched asserting 'truncated, applied_budget 0'. Give the function a stampProvenance flag and pass false from the four measurement call sites. The adopted path (finalizeToolResponses' own 'batch' stamp) stays the only writer, so a genuine cut is still certified. Add the regression case to the pressure-aware block: an uncut send must leave every function-response part without truncated/budget. * fix(core): carry trusted recovery metadata across tool budgets * fix(core): keep the tool-output recovery handle out of a part's value Two main-side contracts broke on this branch: - convertToFunctionResponse read the new optional recoveryText / persistedOutputFiles producer metadata outside the guard that already covers rawOutputSize, so a producer whose getter throws turned a successful tool result into { error: 'persisted output unavailable' }. - The recovery handle was an enumerable symbol key on the part, so a part no longer compared equal to its own serialized form and the agent JSONL transcript assertion failed. Define it non-enumerable, and attach it after attachToolOutputProvenance, whose spread would drop it. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvnzbc3guv * fix(core): re-attach the tool output recovery handle after provenance re-stamp tool-output-recovery.ts defines its handle non-enumerable and documents that a spread copy drops it, so callers must attach it last. checkAndNotifyCompletion re-stamps provenance over the completed parts twice, and both stamps are spread copies, so each one silently dropped the handle. A producer-sized result (an agent TIMEOUT / MAX_TURNS body) never earns persistedOutputFiles, which is the only condition under which collectTextSlots rebuilds the handle. The part was therefore its sole carrier, and the send boundary builds its entries from parts alone. With the handle gone, fitText ran with no preserved text: the pressure budget this PR adds performed a second cut straight through the remediation reason line and the transcript marker, and the minimumRecoveryBudget veto priced its floor at notice+3 instead of the real recovery text. The parent model then read a truncated answer with no knob to raise and re-issued the same call, which timed out again. Re-attach after each stamp, matching convertToFunctionResponse and observeProducerOutput, which already do this correctly. Adds a regression test that drives the scheduler through finalize and then re-cuts the parts at the two per-slot allocations the review measured (660 and 1000); removing either re-attach turns it red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmvop19fgvo * fix(core): preserve recovery when appending hook context * fix(core): correct send guards and fetched-summary recovery * test(core): complete send-guard regression fixtures * test(core): correct hook fixture Config assertion * refactor(core): narrow pressure-budget scope and reuse previews --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: yiliang114 <jinjing.zzj@gmail.com>
What this PR does
Tool results carry raw, processed and final submitted size estimates across existing output-reduction boundaries. Injection is recorded once per newly executed call after final budgeting and media handling; retries, inherited fork history and replay do not recount it. Restored raw sizes remain unknown. New size records exclude output bodies, arguments, local paths and digests, and internal provenance never enters provider JSON.
Oversized results retain complete recoverable originals. Bounded JSON samples preserve original keys and disclose omissions; metadata stays outside the payload namespace. Web extraction retains its existing processing input while the complete outward envelope follows ordinary limits. Configured media estimates and actual reduction budgets are carried through persistence, batch and injection. Cancellation or hook replacements cannot claim producer savings.
Why it's needed
A late preview length cannot explain original result size, where it was reduced or what was submitted. The same pipeline must also preserve recovery pointers and prevent sample corruption, repeated accounting and misleading budget attribution.
Design: English · įŪä―äļæ.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Local macOS native CLI, stdio MCP, production file logs/metrics and matched120x40 tmux runs are recorded in the closeout report. Baseline sample corruption, media-config mismatch, repeated inherited injection and102699-character web envelope were reproduced. Controlled candidate runs preserve originals and correct accounting; the long web result is2621 characters. Exact source hashes, final getter-order delta reuse, all26 attempts and screenshot boundaries are stated in the report.
Core/CLI build and typecheck, changed-file lint/format pass. Core1888 related tests are covered by passing results and595 affected reruns;98 logger and1188 ACP tests pass. Two independent source/ponytail passes found no remaining confirmed owned Critical.
Tested on
Risk & Scope
Linked Issues
Addresses #11536 size accounting and oversized-output recovery. The separate MCP default-policy decision is not closed by this PR. Split from #13599.
äļæčŊīæ
čŋäļŠ PR åäšäŧäđ
å·Ĩå ·įŧæåĻį°æįžĐåčūđįäļæšåļĶåå§ãåĪįååæįŧæäšĪįåĪ§å°äž°įŪãæģĻå ĨäšäŧķåĻæįŧéĒįŪååŠä―åĪįäđåïžææ°æ§čĄč°įĻčŪ°äļæŽĄïžéčŊãforkįŧ§æŋååēåéæūäļäžéåĪčŪĄæ°ãæĒåĪįŧæįåå§åĪ§å°äŋææŠįĨãæ°įåĪ§å°čŪ°å―äļåŦįŧææĢæãåæ°ãæŽå°č·ŊåūææčĶïžå éĻæĨæšäŋĄæŊäļäžčŋå Ĩprovider JSONã
čķ åĪ§įŧæäŋįåŊæĒåĪįåŪæīåæãåéJSONæ ·æŽäŋįåå§éŪåđķčŊīæįįĨïžå æ°æŪäļčĶįč――č·éŪãį―éĄĩæåäŋįåæåĪįčūå ĨïžåŪæīåĪåąčŋåéĩåŪæŪééĒåšĶãé į―ŪååŠä―äž°įŪååŪé įžĐåéĒįŪčīŊįĐŋč―įãæđéåæģĻå ĨïžåæķæhookæŋæĒäļäžåæĨį䚧č čįã
äļšäŧäđéčĶ
åŠææįŧéĒč§éŋåšĶïžæ æģčŊīæåæåĪåĪ§ãåŠéįžĐåãæįŧæäšĪåĪå°ãåäļæĩįĻčŋčĶäŋįæĒåĪæéïžéŋå æ ·æŽæåãéåĪčŪĄæ°åéčŊŊéĒįŪå―åąãčŪūčŪĄææĄĢïžEnglish · įŪä―äļæã
čŊåŪĄæĩčŊčŪĄå
åĶä―éŠčŊ
ååčŊæŪ
æŽå°macOSåįCLIãstdio MCPãį䚧æäŧķæĨåŋ/ææ å120x40é åŊđtmuxčŪ°å―č§æķå°ūæĨåãå·ēåĪį°æ§æ ·æŽæåãåŠä―é į―Ūäļįæãįŧ§æŋæģĻå ĨéåĪįŧčŪĄå102699åįŽĶį―éĄĩåĪåąčķįïžåæ§åéäŋįåæåđķäŋŪæĢčŪĄéïžéŋį―éĄĩæįŧäļš2621åįŽĶãæĨåæįĄŪååšæšį æįšđãæįŧgetter饚åšäŋŪåĪįåĒéåĪįĻãå ĻéĻ26æŽĄå°čŊåæŠåūč―čŊæįčåīã
core/CLIæåŧšãįąŧåæĢæĨååŪålint/æ žåžæĢæĨéčŋãįļå ģcore1888éĄđįąéčŋįŧæå595éĄđåå―ąååĪæĩčĶįïž98éĄđæĨåŋå1188éĄđACPæĩčŊéčŋãäļĪäŧ―įŽįŦæšį /ponytailåŪĄæĨæŠåį°åĐä―å·ēįĄŪčŪĪįčŠæCriticalã
æĩčŊåđģå°
éĢéĐäļčåī
å ģč Issue
åĪį#11536įåĪ§å°čŪĄéåčķ åĪ§čūåšæĒåĪïžįŽįŦMCPéŧčŪĪįįĨåģåŪäļįąæŽPRå ģéãäŧ#13599æåšã