Repository navigation
feat(core): shrink tool results to the headroom left before auto-compaction - #13599
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Local verification for commit Fresh core/CLI builds and typechecks passed, as did changed-file ESLint/Prettier and diff checks. Targeted checks accumulated 1,416 distinct executed cases, each passing at its latest verification. Review repairs cover independent explicit char/line settings, line-only context reduction, producer metadata in speculation/ACP, and truthful no-file JSON previews. This is targeted coverage, not a full repository test run. Terminal excerpts below are genuine captures with anonymous runtime UUIDs redacted. No internal defect URLs, customer identifiers, credentials or private local paths are included. #2566 / #11536 final local native acceptanceAll four frozen-candidate native CLI scenarios completed successfully. Actual injected character lengths agree with the anonymous provider requests; saved complete outputs pass content and digest checks. The explicit shell override is recorded as Executed the real noninteractive Identity: immutable base
Low and high each request the same shell output and 20,021-character / 204-line read fixture. Low injects 24,708 characters total; high injects 18,400. The shell retains its independent 30,000-character producer cap, with a 4,687/4,688-character recovery envelope. High Explicit settings are confirmed by the runtime start event as 12,345 characters / 321 lines. The shell producer and injection report applied budget 12,345 and preserve the complete saved output. The read tool itself returns âShowing lines 1-125 of 204 total linesâ; its measured raw value is that produced 12,406-character result, not the entire 20,021-character file. No context reduction event occurs in this profile. The 321-line value is loaded, but this 204-line read fixture does not independently prove a binding 321-line cutoff. Recovery metadata/envelope can exceed the content threshold. Observed source-label boundary: producer and both actual injection events for MCP uses the actually advertised Each shell full artifact contains the complete known shell payload, including the product command/result wrapper (123,226 / 123,227 / 123,231 characters and 3,008 lines). Wire SHA-256 values match saved output SHA-256. Full recovered files are retained under each profileâs The native request totals include product behavior rather than a fixed assertion: low has main response + tool continuation + managed-memory scan (3); high and explicit have two compression-related requests (including one without tools), followed by a resumed main continuation and managed-memory scan (5); MCP has discovery + invocation + answer + managed-memory scan (4). Repeated injection records correlate with a subsequent managed-memory side query carrying the tool history. Request purposes and timestamps support this attribution; the logs lack a chat-instance identifier, so the native run does not directly prove per-chat deduplication or conclusively exclude duplicate sinks. Retry/history deduplication is covered separately by targeted chat tests. Histograms Actual tmux captures are retained for all four profiles (low 91,033 bytes; high 91,054; explicit 70,764; MCP 70,866). The following blocks are excerpts of captured terminal bytes, preserving their native line wrapping; full capture files are low: high: explicit: mcp: Historical evidence remains separate: Scope verdict: the tested adaptive defaults, explicit configuration loading, native shell/read injection accounting, MCP global reduction, structured spill preview, and saved-output integrity pass. Shell source-label interpretation is the documented boundary above; it is not a failed numeric-budget check. This is bounded controlled-provider acceptance, not full closure of every original #11536 pipeline/ACP/speculation/backend requirement or proof of representative real-model savings. No source, dependency, Git, GitHub, HOME/auth configuration, or provider infrastructure was changed by this verification. äļæčĄĨå
ïžåįŧåéčŋįåŪ CLI â æŽå°åŋå provider įåŪé
čŊ·æąč·ŊåūïžåđķäŋįįåŪ tmux čūåšãéŦååéĒįŪįĄŪåŪåå°åŪé
æģĻå
ĨïžMCP åŪæī JSON å shell/read č―įå
åŪđååŊæĒåĪãæūåž shell įåŪé
åæŪĩæŊ |
|
First CI snapshot for A single job-specific rerun request was rejected by GitHub with HTTP 403 because the containing workflow is still running. No product or workflow workaround was added. Other checks are pending; this PR is ready for review, with CI completion still pending. äļæïžéĶäļŠåĪąčīĨä―äš checkout į Git åŊđ蹥čŊŧåéķæŪĩïžå°æŠåŪčĢ äūčĩææ§čĄæĩčŊïžäļč―æŪæĪåĪåŪ䚧ååå―ãå·ēå°čŊäļæŽĄäŧ éč·čŊĨ jobïžGitHub å æīäļŠ workflow å°åĻčŋčĄčæįŧïž403ïžãæŽå°éŠčŊäļčąæ tmux æĨåč§äļïžPR å·ēåŊ reviewïžä― CI å°æŠåŪæã |
|
â Qwen Triage finished â view run. See the stage comments in this thread for the result. â Qwen Triage å·ēåŪæ ââ æĨįčŋčĄãįŧæč§æŽįšŋįĻäļįåéķæŪĩčŊčŪšã |
|
Re-running the gate at the current head Template â â What / Why / Reviewer Test Plan (How to verify · Evidence · Tested on) / Risk & Scope / Linked Issues all present. Problem â observed, not theoretical. This implements #2566, still open, carrying Direction â aligned with the core context-management mission. The two flags from the previous pass both still stand, and neither is a gate block:
Size â core paths touched ( Approach â the redesign is the right call and I want to say so plainly. The previous mechanism recovered a tool result's only handle back to its spilled artifact by regex-matching rendered text ( The cost is scope. The change now reaches Risk â no Stage 1e high-risk path match (paginated file list checked against the revert-correlated patterns; none of the 11 production files match). One honest caveat the mechanical check cannot see: the regression Stage 2 reports surfaces in Moving on to code review. ð äļæčŊīæåĻå―å head æĻĄæŋ â ââ What / Why / Reviewer Test PlanïžHow to verify · Evidence · Tested onïž/ Risk & Scope / Linked Issues åé―å Ļã éŪéĒ ââ å·ēč§æĩïžäļæŊįčŪšæ§å åšãæŽ PR åŪį°įæŊäŧįķåžæūį #2566ïžåļĶ æđå ââ äļæ ļåŋįäļäļæįŪĄįä―ŋå―äļčīãäļäļč―ŪįäļĪįđæéäūįķæįŦïžäļé―äļæŊéĻæ§éŧæéĄđïž
č§æĻĄ ââ č§Ķåæ ļåŋč·Ŋåūïž æđæĄ ââ čŋæŽĄéæ°čŪūčŪĄæđåæĢįĄŪïžææģæįĄŪčŊīåšæĨãæĪåįæšåķéčŋåŊđå·ēæļēæææŽåæĢååđé
ïž äŧĢäŧ·æŊčåīãæđåĻį°åĻč§Ķå éĢéĐ ââ æŠå―äļ Stage 1e éŦéĢéĐč·Ŋåūïžå·ēįĻåéĄĩæäŧķåčĄĻåŊđį
§äļåæŧįļå
ģįæĻĄåžïž11 äļŠį䚧æäŧķåäļåđé
ïžãä―æäļįđæšæĒ°æĢæĨįäļå°įåĶčŊæéïžStage 2 æĨåįåå―æīéēåĻ čŋå ĨäŧĢį åŪĄæĨãð â Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Code review â read the full production diff at The three standing Criticals are genuinely fixed at this head. I verified each in the code rather than taking the resolved-thread flag (all 81 threads now read resolved) or the author's word:
This closes a root cause rather than another instance, which is what the round-18 convergence note recommended. Critical 1 â a successful tool result is now converted into an error when optional metadata throws. In } catch {
// Optional observation metadata must not change a tool's outcome.
}
return attachToolOutputProvenance(
attachToolOutputRecovery(
convertToFunctionResponseParts(toolName, callId, llmContent),
{
text: producerResult?.recoveryText,
persistedOutputFiles: producerResult?.persistedOutputFiles, // unguarded
},
),At The received string is the test's own Critical 2 â the symbol side-channel does not survive serialization, and R16-1's fix now rests on it. The red assertion is the smaller half. const previousFiles =
entry.persistedOutputFiles ??
entry.responseParts.map(getToolOutputRecovery)
.find((r) => r?.persistedOutputFiles !== undefined)?.persistedOutputFiles;
if (previousFiles !== undefined) { /* keep, do not re-persist */ continue; }It holds in-turn, where Parts are live objects. It does not hold across anything that round-trips through JSON â session restore, replay, rewind, recording. The merge this PR just absorbed landed Two Suggestions, neither blocking:
The new recovery channel is the crux of the redesign, and its leak is the last step: sequenceDiagram
participant P1 as Tool producer
participant P2 as Scheduler conversion
participant P3 as Part with recovery symbol
participant P4 as Response finalizer
participant P5 as Transcript writer
P1->>P2: ToolResult carrying recoveryText and persistedOutputFiles
P2->>P3: attachToolOutputRecovery on every converted part
P3->>P4: send boundary or batch budget pass
P4->>P4: minimumRecoveryBudget prices header plus preserved unit
P4-->>P3: shortened text with recovery re-attached
P3->>P5: recorded as JSON
P5-->>P3: symbols do not survive the round trip
Files changed (21 of 21 shown)
CI evidence â unattended re-run ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / æŊäļŠæĢæĨåäļčĄïžåææ°äļæŽĄčŋčĄïžïžįįĨ skippedïžåĪąčīĨéĄđæåĻæåã This red is PR-caused, not infra noise, and I want to be explicit about why I am calling it that rather than letting it pass as a flake. All three failures are deterministic assertion mismatches whose received values are produced by the new recovery mechanism: two carry the literal message of a throwing Not verified, and why: the macOS and Windows lanes and the CLI integration suite were skipped again, for a change whose new logic is a tool-result contract widening plus path handling â the two POSIX-only Sandboxed verification: a äļæčŊīæäŧĢį åŪĄæĨ ââ å·ēčŊŧåŪ äļäļŠæŠåģ Critical åĻå―å head įĄŪåŪäŋŪåĨ―äšã æéäļåĻäŧĢį éæ ļåŊđïžčäļæŊéäŋĄįšŋįĻ resolved æ čŪ°ïž81 æĄįšŋįĻį°å·ēå ĻéĻ resolvedïžæä―č įčŊīæģïž
čŋå ģéįæŊæ đå čéåäļäļŠåŪäūïžæĢæŊįŽŽ 18 č―Ūæķææč§æåŧščŪŪįã Critical 1 ââ å―åŊéå
æ°æŪæåžåļļæķïžæåįå·Ĩå
·įŧæį°åĻäžčĒŦč―ŽæéčŊŊã åĻ Critical 2 ââ symbol æč·Ŋæ æģč·Ļåšåååæīŧïžč R16-1 įäŋŪåĪį°åĻæĢäūčĩåŪã äļĪæĄåŧščŪŪïžåäļéŧæïž
CI čŊæŪ ââ æ äššåžåŪéč·ïž čŋäļŠįšĒčēįą PR é æïžäļæŊåšįĄčŪūæ―åŠåĢ°ïžææģæįĄŪčŊīæäļšäŧäđčŋæ ·åĪåŪčäļæŊå―ä― flake æūčŋïžäļäļŠåĪąčīĨé―æŊįĄŪåŪæ§įæčĻäļåđé
ïžå
ķ received åžåįąæ°įæĒåĪæšåķ䚧įââäļĪäļŠæšåļĶįæĢæŊæåžåļļ æŠéŠčŊååå ïžmacOS äļ Windows lane äŧĨå CLI éæåĨäŧķåæŽĄčĒŦč·ģčŋïžčæŽæŽĄæđåĻįæ°éŧčūæĢæŊå·Ĩå
·įŧæåĨįšĶįå åŪ―å č·ŊåūåĪįââįŽŽ 17 č―Ūå·ēæåšįäļĪåĪäŧ
POSIX į æēįŪąéŠčŊïžæĪ head äļå·ēæäļæŽĄ â Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 2/5 â the redesign genuinely closes the root cause behind three long-standing Criticals, but it lands with CI red from two new Criticals of its own, one of which is a real production regression on the ACP and speculation paths. Stepping back against my own independent proposal from Stage 2: I still don't have a simpler design to offer for the pressure budget itself. But I do think the recovery half of this PR took a wrong turn that is worth naming, because it is the part that keeps generating findings. Round after round filed the same Critical against The problem is what replaced them. The handle moved from parsed text to a symbol-keyed property on And Critical 1 is not a design question at all â it is a one-line placement error. The catch block directly above the new read states the invariant in a comment; the read went on the other side of it. Two pre-existing tests exist specifically to pin that invariant, and both are red. Why I'm not approving:
The value question from my last two passes is unchanged and still belongs to a human: #2566 is On the standing vote. The PR already carries a @wenshao @zjunothing â you are both assigned, and this needs a human call. Two things: (1) the two Criticals above are small, concrete fixes and should not need another design round â Critical 1 in particular is moving one read inside an existing The gate is not approving. Request-changes is already standing on this PR, so no new review is submitted. äļæčŊīæį―ŪäŋĄåšĶïž2/5 ââ čŋæŽĄéæ°čŪūčŪĄįĄŪåŪå ģéäšäļäļŠéŋææŠåģ Critical čåįæ đå ïžä―åŪč―å°æķ CI æŊįšĒįïžäļåļĶæĨäšäļĪäļŠåąäšåŪčŠå·ąįæ° Criticalïžå ķäļäļäļŠæŊ ACP äļ speculation č·ŊåūäļįåŪįį䚧åå―ã éäļæĨåŊđį §æåĻ Stage 2 åäļįįŽįŦæđæĄïžåŊđååéĒįŪæŽčšŦïžæäŧįķæēĄææīįŪįčŪūčŪĄåŊæãä―æįĄŪåŪčŪĪäļšæŽ PR įæĒåĪéĢäļåčĩ°äšäļæŪĩåžŊč·ŊïžåžåūįđåšæĨïžå äļšåŪæĢæŊäļæäš§åš findings įéĻåã äļč―Ūåäļč―ŪéåŊđ éŪéĒåĻäšæŋäŧĢåŪäŧŽįäļčĨŋãåĨæäŧč§ĢæææŽåæäš č Critical 1 æ đæŽäļæŊčŪūčŪĄéŪéĒââåŪæŊäļčĄä―į―ŪéčŊŊãæ°čŊŧåæĢäļæđį catch åįĻæģĻéåæäšäļåéïžčŊŧåčĒŦæūå°äšåŪįåĶäļäū§ãæäļĪäļŠæĒææĩčŊæĢæŊäļšéä―čŋäļŠäļåéčååĻįïžį°åĻé―æŊįšĒįã äļšä―äļæđåïž
åäļĪč―Ūįäŧ·åžéŪéĒæēĄæååïžäŧåšįąäššæĨåŪïž#2566 æŊ å
ģäšå―åéĢäļįĨĻã æŽ PR å·ēåļĶææĨčŠæŽæšåĻäššį @wenshao @zjunothing ââ ä― äŧŽäļĪä―é―åĻææīūåčĄĻäļïžčŋéčĶäššæĨåŪãäļĪäŧķäšïž(1) äļéĒäļĪäļŠ Critical é―æŊå°čå
·ä―įäŋŪåĪïžäļåščŊĨåéčĶäļč―ŪčŪūčŪĄââå°Īå
ķ Critical 1 åŠæŊæäļåĪčŊŧåį§ŧčŋå·ēæį éĻæ§äļäšæđåãæĪ PR äļå·ēæ request-changes åĻįæïžå æĪäļæäšĪæ°čŊåŪĄã â 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 findings â full reasoning in the stage comments above.
-
CI is red because of this PR.
Lint & Staticfails the settings-schema freshness gate:DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD/..._LINESmoved to 80000 / 2000 butpackages/vscode-ide-companion/schemas/settings.schema.jsonstill says 25000 / 1000. Runnpm run generate:settings-schemaand commit. -
Removing
DiscoveredMCPTool.maxOutputCharssilently applies the global 2000-line cap to MCP results. That override was load-bearing for a char-only exemption documented in two places (coreToolScheduler.tsperToolLines, and themcp-tool.tscomment this diff deletes). MCP goes from 500K chars / no line cap to 80K chars / 2000 lines, and many-short-lines is the dominant shape for structured MCP output. Keep the exemption or state the intent in the description and design doc. -
READ_FILE_CHAR_CAP = 25_000was left behind.packages/cli/src/commands/review/lib/diff-plan.ts:238is a documented mirror of the constant this PR moves, with 11 read sites across 5 files in/reviewâ including themaxLineChars > READ_FILE_CHAR_CAPcheck that decides a chunk cannot be receipted as fully reviewed.
Also worth fixing in the same pass (not blocking on their own): an unconditional SHA-256 now runs on every tool result including untruncated ones; four doc pages still state the old numbers; the managed-agent protocol carries charsExplicit but not the new linesExplicit; the estimatePartChars change affects compaction for every session with no test and no mention; and the truncation stub still says "beginning and end" while substituting a structured sample.
Separately, a question rather than a blocker: the 25Kâ80K / 1000â2000 default bump is the half of #2566 that prior triage declined pending evidence, and need-discussion is still on that issue. The pressure-aware half does not need the bump to work â it scales whatever the default is. Consider splitting it out so it can be reviewed as the product decision it is.
Note the unit suite had not finished on this head (Test (ubuntu-latest) in progress, macOS/Windows skipped), so nothing here reflects a full suite verdict.
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
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>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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
Preserve the verified estimator implementation and portable MCP temp-directory mock from the parallel patrol fix. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
PR #13599 follow-up native acceptancePASS: one actual high-pressure noninteractive CLI run completed naturally. The anonymous provider received the reduced shell/read_file results; injection telemetry retained their producer sizes; both complete spill artifacts passed integrity checks. This is current frozen-candidate local acceptance after the CI corrections, distinct from the earlier four-profile evidence at Candidate commit: Actual controlled-provider resultUsed the real supported The provider issued one real
Shell raw size measures the producer's model-facing result, including its command/status wrapper; it is larger than stdout itself. This report does not claim original file/network-byte telemetry coverage for every producer. Shell's The complete saved shell producer output was 123,212 bytes, contained all 123,022 fixture stdout bytes, and matched the preview's advertised digest:
The read-file spill retained the entire original 20,021-byte file exactly, despite the two-stage preview reduction. Its digest was:
CLI exit was 0; final stream result was Actual request and telemetry attributionFive actual requests were observed: three main-chat requests, one compression request, and one managed-memory request. Classification uses the actual system prompts, including
Compression was actually exercised, not bypassed: telemetry reported Four injection records were observed, two per tool. The first pair at 13:06:56.084 aligns with the post-compression main request 4. The second pair at 13:06:56.126 occurred after the managed-memory subagent started at 13:06:56.112 and immediately before its API request at 13:06:56.127 (which carries the subagent name and managed-memory prompt identity); provider request 5 followed at 13:06:56.145. This supports attribution to main continuation and the side-query chat. Injection events themselves lack a chat identity, so this report does not claim a general same-chat retry/history dedup guarantee from native evidence. The repeated records are not evidence of repeated tool execution. Current local checks (parent receipts)Latest whole-file receipts consolidate 804 core and 661 CLI cases: 1,465 distinct executed cases passed, zero skipped or failed in the latest receipt for each file. This is deduplicated coverage from the latest file results, not one single final full-suite run. The verifier did not rerun tests. The initial core pass had 800 passing and four MCP failures caused by a missing storage getter in the mock; after the fixture correction, the entire 166-case MCP file passed. The initial CLI pass had five old 25k-derived brief-allowance expectations; correcting the fixture/comment from six pages (
The estimator's absent/circular-payload behavior and Source and actual loaded bytesThe read-only Node load hook delegated to the normal loader without transforming source. Eleven watched candidate JavaScript modules were actually loaded, including CLI, Config, scheduler, LlmChat, output-size, estimator, retention, shell and read_file. A loaded MCP module does not establish an MCP invocation. Review diff-plan source/build files were pinned but were not loaded/exercised by this scenario.
Actual tmux captureThis excerpt comes directly from the retained real session's Evidence and cleanupThe owned Cleanup complete: owned CLI/provider tmux sessions removed, anonymous provider listener stopped, and owned profile/runtime/workspace removed. Complete spill copies and all evidence remain. The verifier made no product source, Git/GitHub, auth configuration, dependency installation or build changes. Actual user HOME/CODEX_HOME/auth were untouched. äļæčŊīæå―ååŧįŧåéįäļæŽĄįåŪéŦåå CLI éŠæķéčŋãprovider éĶæŽĄåŪé åæĨ 970,000 prompt tokensïžč·ŊįąæįĄŪé į―Ū 1,000,000 įŠåĢïžæēĄæä―ŋįĻå éĻ token-counter setterãåŪé æ§čĄ shell äļ read_file åäļæŽĄïžprovider æķå°įå·Ĩå ·ææŽååŦæŊ 4,673 äļ 13,727 åįŽĶïžæģĻå Ĩ telemetry äŋįäšåå§ producer åĪ§å° 123,212 äļ 20,021ãread_file įåŪé context budget äļš 18,400ïžshell äŋæå ķå åŧš per_tool 30,000 įįĨãéĒįŪæŊäļéïžäļčĄĻįĪščūåšäļåŪæ°åĨ―įäšéĒįŪã åŪæīæäŧķæĒåĪå·ēæ ļéŠïžread_file äŋåäšå ĻéĻ 20,021 åå§åčïžéåčįäščūå Ĩïžshell äŋåįåŪæī producer čūåšå åŦå ĻéĻ 123,022 stdout åčïžäļåđé éĒč§åĢ°æį SHA-256ãshell į raw metric å åŦå―äŧĪåįķæ wrapperïžäļč―äļåå§ stdout åĪ§å°æ··äļšäļč°ïžäđäļč―æģåäļšææ producer įåå§æäŧķ/į―įŧåččĶįãCLI čŠįķåŪæåđķéåš 0ïžæēĄæ permission denialã åįžĐįĄŪåŪåįïžåŪé čŊ·æąæäļäļŠ mainãäļæŽĄ compressionãäļæŽĄ managed-memory åšåïžåįžĐåå·Ĩå ·įŧæäŧäŋįãæģĻå ĨčŪ°å―įäļĪįŧæķéīååŦåŊđåšåįžĐåįäļŧčŊ·æąäļ managed-memory side queryïžäšäŧķæŽčšŦæēĄæ chat identityïžå æĪæēĄæåŪĢį§° native å·ēčŊæææå chat éčŊ/ååē replay įåŧéčŊäđïžäđæēĄæææšåļĶååēå―ä―éåĪå·Ĩå ·æ§čĄãåæ§åŋåæĻĄåįåšåŪåįäŧ čŊæčŋčĄå wire æšåķïžäļčŊæį䚧æĻĄåææææķįã įķ agent įææ°éæäŧķåæ§åŧéåäļš core 804ãCLI 661ïžå ą 1,465 äļŠåŪé æ§čĄäļéčŋį distinct casesãåŪäŧŽæĨčŠåæŽĄéäļčŋčĄå äŋŪæĢ mock/fixture åįåŊđåšæīæäŧķéč·ïžäļæŊåĢ°į§°åč·čŋäļæŽĄæįŧå ĻåĨãå―å native åŠå shell/read_file äļæĄéŦåååšæŊïžMCPãįĐš/åūŠįŊ payloadãreview allowance įäŧĨåŪåæĩčŊäļšįŽįŦčŊæŪãčŋčĄåå 23 äļŠ production/source/build æäŧķååļäļčīïžåįŧ test-only äŋŪæĢæŠæđååŪé æ§čĄåčãææčŠæäļīæķčŋčĄčĩæšå·ēæļ įïžåå§/čąæ tmuxãwireãtelemetryãåŪæīæĒåĪæäŧķåæĨåäŋįã |
PR #13599 structured JSON preview native acceptancePASS: one real MCP JSON discovery/invocation scenario completed naturally. The next main provider request received the structured-sample instruction, without a beginning/end or CONTENT TRUNCATED claim. The embedded sample parsed as valid JSON, and the complete saved JSON matched the actual raw 120,000 bytes and its advertised digest exactly. Tested candidate core/source: Real discovery, invocation and provider resultUsed the actual supported noninteractive The initial advertised provider tools included The next main request carried a 1,005-character text result (1,007 UTF-8 bytes because the sample includes an ellipsis). The wire text is an explanatory envelope followed by a JSON sample; this report does not claim the entire envelope is a JSON document. The sample retained the marker, actual total row count 2,000 and first two real rows, and parsed successfully. The result text explicitly said:
It contained neither MCP producer/injection telemetry reported raw 120,000 â injected 1,005 characters, The complete JSON spill equaled both the actual stdio-produced raw JSON and the frozen expected bytes, exactly 120,000 bytes. It parsed as complete JSON with all 2,000 original rows. The advertised digest matched the file:
CLI exit was 0; final stream result was Actual request attributionFour provider requests were observed: three main-chat requests and one managed-memory request. The side request is identified by its actual
The MCP result was injected into the next main request and later retained in managed-memory history. Injection records for discovery/MCP were repeated in the memory side-query phase: managed-memory started at Source/executed identity and local checksThe read-only load hook delegated to the normal loader and recorded file paths/hashes without transforming source. All eleven watched candidate build modules loaded, including MCP client/tool, tool_search/tool_call, scheduler, LlmChat, output-size and the new truncation helper. Actual stdio invocation and outgoing provider text establish the reached behavior; loading a module alone would not.
Parent-provided current receipts: fresh core build exited 0; the full truncation test file executed 44 passing cases; source/test ESLint, formatting and diff checks passed. The verifier ran no tests or builds. The earlier 1,465 distinct latest-file cases and high-pressure native result belong to the separate 6ecad6 report; no fresh full-suite claim is made for this text-only additive correction. Actual tmux captureThis excerpt is directly from Evidence and cleanupThe owned Cleanup complete: owned CLI/provider tmux sessions removed, provider listener stopped, no stdio helper remained after CLI exit, and owned profile/runtime/workspace removed. All raw and recovered evidence is preserved. No product source, Git/GitHub/Aone, auth, dependency installation or build mutation was performed by this verifier. äļæčŊīæå―å additive åéįäļæŽĄįåŪ MCP JSON éŠæķéčŋãéĶå
éčŋåŪé
äļäļæŽĄäļŧčŊ·æąæķå°įå·Ĩå ·įŧæäļš 1,005 åįŽĶïžæįĄŪčŊīæäļæđå åŪđæŊ structured JSON sampleïžæēĄæ beginning/end æ CONTENT TRUNCATED åĢ°į§°ãæīäļŠå·Ĩå ·ææŽæŊčŊīæ envelope å JSON æ ·äūïžäļæŊåĢ°į§° envelope æŽčšŦåŊįīæĨč§Ģæäļš JSONãåĩå Ĩæ ·äūåŊæĢåļļč§ĢæïžåđķäŋįįåŪ markerã2,000 čĄæŧæ°ååäļĪæĄčŪ°å―ãtelemetry æūįĪšåå§ 120,000ãæģĻå Ĩ 1,005ãglobal budget 80,000ãåå§ JSON åŠæäļčĄïžå æĪæŽæŽĄåŪé čŊæ char-triggered JSON branchïžæŠįŽįŦéŠčŊ lines æŠæéåžã åŪæī spill äļåŪé stdio åå§ JSONãåŧįŧį expected bytes äļč éåčįļįïžåäļš 120,000 åčïžäŋįå ĻéĻ 2,000 æĄåå§čŪ°å―ïžSHA-256 äļčūåšåĢ°æäļčīãåŪé čŊ·æąäļšäļäļŠ main å äļæŽĄ managed-memoryïžæŠåįåįžĐïžéåĪæģĻå ĨčŪ°å―æ side-query įåŪé prompt äļæķéīå ģįģŧå―åąïžäļåŪĢį§°å·ēéčŋ native čŊæææå chat replay åŧéčŊäđïžäđæēĄææååēæšåļĶå―ä―éåĪå·Ĩå ·č°įĻã å―å core æåŧšåŊđåš be37149ïžCLI ä―ŋįĻ 6ecad6 įæŠåæåŧšäš§įĐïž21 äļŠæšį /æ§čĄæäŧķčŋčĄååååļäļčīïž11 äļŠ watched build modules ååŪé å č――ïžå åŦæ° truncation helperãįķ agent įå―ååŪååæ§æŊåŪæī truncation æäŧķ 44 cases éčŋïžæĪå 1,465 cases äļéŦååįŧæäŧåąäšįŽįŦį 6ecad6 čŊæŪïžæēĄæéæ°č·éŦååæåĢ°į§°ååŪææįŧå ĻåĨãææčŠæčŋįĻãįåŽãtmuxãprofile/runtime/workspace å·ēæļ įïžåå§/čąæåŪé įŧįŦŊãčŊ·æąãtelemetry äļåŪæī JSON äŋįãčĨåįŧäŧ åĒå Git ancestry äļäŋæåŪå Ļįļåįå·ēæĩį䚧/build åčïžåŊäŧĨåĪįĻæŽčŊæŪïžæŽæĨåäļæđåæđååįæįŧæ ã Final ancestry and evidence reuseFinal candidate merge: æįŧ merge äļš |
|
Final pushed head: High-pressure CLI/tmux report · JSON MCP CLI/tmux report. Addressed the concrete review/CI failures in additive local commits
The current CI fixes also correct strict comparisons that inadvertently treated enumerable internal Symbol provenance as part of provider JSON, while retaining live-path provenance and cancellation/status assertions. Latest whole-file receipts cover 1,465 distinct executed passing cases for those repairs, plus the separate 44-case truncation file. Fresh core/CLI builds and typechecks passed; the final small JSON correction has its own fresh core build. Suggestion 4's pre-fallback digest cost and the other nonblocking hot-path optimization notes remain acknowledged performance tradeoffs. The digest contributes to the actual wrapper size/token decision; this follow-up does not introduce a placeholder hash or claim a measured CPU improvement. Raw size also remains explicitly unknown for live paths that lack a producer result/original-size metadata; it is not inferred from a preview. Native evidence is pinned separately: the high-pressure run at äļæčŊīæå·ēįĻåĒéæäšĪäŋŪæĢå ·ä― review/CI éŪéĒïžschema éŧčŪĪåžå·ēįšģå ĨïžMCP åæķč·éåįŽĶåčĄæ°éåķæŊæįĄŪææįčĄäļšïžæĢæåäļčąæčŪūčŪĄå·ēåæļ éŧčŪĪ 2,000 čĄåŊč―å č§Ķåïžreview čŊŧååéĄĩæ 80k čŪĄįŪïžįŽįŦ 20k diff chunk äļåãå―åįĻæ·ææĄĢåååēčŪūčŪĄčŊīæå·ēåæĨ/æ æģĻåįŧ§įšĶåŪã managed provider worker äŧ
æ ļéŠæ§čĄäļäļææčĶåđķįīæĨæ§čĄå·Ĩå
·ïžäļäŧčŊĨčŪ°å―éåŧšååé
į―ŪïžäđäļåéååæįĨ chatïžå æĪäļæ·ŧå æ åŪé
æķčīđč
į CI äŋŪæĢįææ°æīæäŧķåæ§åŧéåčĶį 1,465 äļŠåŪé æ§čĄéčŋįįĻäūïžåĶæ 44 äļŠæŠæįĻäūïžcore/CLI æåŧšãįąŧåæĢæĨéčŋïžæåį JSON å°æđåĻåĶææ° core æåŧšãå éĻ Symbol æĨæšäŋĄæŊäŧåĻåŪæķč·ŊåūäŋįïžäļĨæ žæčĻæåŪé provider JSON æ ļéŠïžåđķäŋįåæķ/įķææĢæĨã éŦååæĨåįŧåŪ |
ðĐš serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
7 |
6 |
â Qwen Code · serve A/B
|
@wenshao @tanzhenxin one product decision is needed before this can merge, separate from the code review. This PR does two things:
The review bot's three blocking items from the Options:
My recommendation is (b) unless the bump was already agreed somewhere I missed. Please say which. äļæčŊīæ@wenshao @tanzhenxin ååđķåéčĶå åäļäļŠäš§ååģåŪïžčŋåäŧĢį review æŊäļĪåäšã čŋäļŠ PR åäšäļĪäŧķäšïž
review bot åĻ åŊéæđæĄïž
éĪéčŋæŽĄč°æīå·ēįŧåĻææēĄįå°įå°æđčūūæäšå ąčŊïžæåŧščŪŪé (b)ãčŊ·åįĨéåŠäļŠã |
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- web_search now builds its llmContent twice (once with applyLimit = false purely to measure it) â already reported (comment 6037332199, 'Notes, not blockers')
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 reverse-audit (round 2)": the test-only change at coreToolScheduler.test.ts:3779 ( { waitForCompletion: true } â { waitForCompletion: true, truncateToolOutputThreshold: 25000 } ) â I dâĶ; "agent reverse-audit (round 2)": a repo-wide check for production structuredClone (or any other structured-copy) of Part objects, which would silently drop the symbol-keyed provenance and sâĶ; "agent reverse-audit (round 2)": did not build the workspace and run packages/cli vitest for useToolScheduler.test.ts / hosted-workspace-tool-turn.test.ts , so the claim that the pre-changâĶ; "agent reverse-audit (round 1)": I did not verify the new design doc's telemetry-shape claims against the code â "RUM mirrors size fields, and character/token histograms distinguish raw/injecteâĶ; "agent reverse-audit (round 1)": I did not verify "Code-mode nested calls retain execution measurements but do not claim independent model injection when only their parent's output is submittedâĶ, and 4 more.
Not reviewed: reverse audit â stopped before round 4 by the review time budget.
[Critical] The PR's own open merge precondition is unresolved. In comment 6040691008 the author asked @wenshao and @tanzhenxin to choose between (a) approving the 25,000 -> 80,000 character / 1,000 -> 2,000 line default bump inside this PR and (b) splitting it into a separate PR, and recommended (b) unless the bump had already been agreed. No maintainer decision is recorded on the thread, and issue #2566 â whose second half this bump is â still carries need-discussion after prior triage declined that half pending evidence. This is a product decision rather than a code defect, and the pressure-aware half of the change works with any default, so it cannot be settled by reading the diff; this review re-checked it against the thread and it still stands.
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã
æŽč―ŪįĄŪčŪĪį 1 æĄåŧščŪŪįš§åį°å·ēåĻ PR äļæĨåčŋïžäļåéåĪååļïžåčĄĻč§äļæđčąæéĻåïžã
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent reverse-audit (round 2)"ïžthe test-only change at coreToolScheduler.test.ts:3779 ( { waitForCompletion: true } â { waitForCompletion: true, truncateToolOutputThreshold: 25000 } ) â I dâĶïž"agent reverse-audit (round 2)"ïža repo-wide check for production structuredClone (or any other structured-copy) of Part objects, which would silently drop the symbol-keyed provenance and sâĶïž"agent reverse-audit (round 2)"ïždid not build the workspace and run packages/cli vitest for useToolScheduler.test.ts / hosted-workspace-tool-turn.test.ts , so the claim that the pre-changâĶïž"agent reverse-audit (round 1)"ïžI did not verify the new design doc's telemetry-shape claims against the code â "RUM mirrors size fields, and character/token histograms distinguish raw/injecteâĶïž"agent reverse-audit (round 1)"ïžI did not verify "Code-mode nested calls retain execution measurements but do not claim independent model injection when only their parent's output is submittedâĶïžåĶæ 4 æĄã
æŠåŪĄæĨïžåååŪĄčŪĄââčŊåŪĄæķéīéĒįŪäļčķģïžæŠč―åžå§įŽŽ 4 č―Ūã
[Critical] The PR's own open merge precondition is unresolved. In comment 6040691008 the author asked @wenshao and @tanzhenxin to choose between (a) approving the 25,000 -> 80,000 character / 1,000 -> 2,000 line default bump inside this PR and (b) splitting it into a separate PR, and recommended (b) unless the bump had already been agreed. No maintainer decision is recorded on the thread, and issue #2566 â whose second half this bump is â still carries need-discussion after prior triage declined that half pending evidence. This is a product decision rather than a code defect, and the pressure-aware half of the change works with any default, so it cannot be settled by reading the diff; this review re-checked it against the thread and it still stands.
â qwen3.8-max via Qwen Code /review (v0.25.0)
|
All 27 Reason: this PR is at +1673/-123, over the patrol's 1500-addition scope fuse, so this round does replies and thread organisation only â no code is added. Deferred, not rejected: #13635 enumerates all 27 by review id, anchor and one-line gist, grouped by structural root (10 of them are already umbrella findings â one root, N confirmed symptoms â and are tracked as one work item each, not N). The 4 None of the 27 Suggestion threads has been resolved. Resolving 27 threads by pointing at a fresh issue is the maintainer's call, not the patrol's. Threads covered by #13635:
Feature tracking stays in #11536 (originally #2566). #11536 is a feature-request issue that closes at merge, so the review debt is filed separately rather than appended to it. äļæčŊīæhead |
âĶ 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
|
The blocking item from review 5646868449 is fixed in adeea4b.
Both re-stamp sites now re-attach the handle after the stamp, matching what The regression test in |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT at head adeea4b9d79be430423b3192bab571d2adf8ec82 â not approving. The single commit added since my last review is a correct fix, but the three Criticals recorded as standing in round 18 are untouched at this head, and my own scan of the full diff is still incomplete.
1. The three standing Criticals are unchanged at this head
Round 18 (review 5641202121, against fcd2443cd26a) recorded three sev: "C" findings, each titled "still standing at the reviewed commit". No review round has run since â the newest ledger on this PR is still round 18, and there are no reviews at all at 84708c5d or adeea4b9 from the review bot. The head has moved four times since fcd2443.
adeea4b9 is one commit ahead of 84708c5d and zero behind, and its scope is exactly two files: packages/core/src/core/coreToolScheduler.ts (+28/â12) and a new packages/core/src/core/coreToolScheduler.test.ts (+60). I compared the blobs of all three files the round-18 findings are anchored in, between 84708c5d and adeea4b9:
| finding | anchor | blob |
|---|---|---|
| R12-1 â the send boundary recovers a tool result's only recovery handle by regex | packages/core/src/tools/truncation.ts:844 |
identical |
R16-1 â the recovery-handle inverses are not idempotent under fitText's own output |
packages/core/src/tools/tool-response-finalizer.ts:132 |
identical |
R18-1 â the preserved transcript handle is charged 1:1 against the same previewBudget whose 1/5 head share carries the composition's reason line |
packages/core/src/tools/agent/transcript-marker.ts:23 |
identical |
Their PR-relative deltas are unchanged too (tool-response-finalizer.ts +224/â49, transcript-marker.ts +20/â0, and truncation.ts still not in this PR's 22-file change set at all). So nothing in the new commit addresses any of the three, and I have no basis on which to call any of them fixed. Per the round-18 record they were carried findings, not fresh ones, which means they have now survived several heads unaddressed.
I am not re-deriving their causal chains here â R18-1 in particular is an A/B measurement claim about headBudget = floor((previewBudget-5)/5) that cannot be settled by reading a commit subject. The point is narrower and does not need that: the code those findings describe is byte-identical to what round 18 assessed, and no reviewer has re-assessed it since.
2. What the new commit does, and it is correct
adeea4b9 fixes a real defect and I verified it rather than taking the subject line for it. tool-output-recovery.ts defines the handle with Object.defineProperty(..., { enumerable: false }), and its own doc comment states the consequence: "Callers that copy a part with a spread drop the handle, so attach it last." attachToolOutputProvenance spreads each part, so it was silently discarding the recovery handle. Both scheduler sites now read the handle from the original call.response.responseParts before the provenance spread and re-attach it afterwards:
const recovery = call.response.responseParts.map(getToolOutputRecovery).find(Boolean) ?? {};
call.response.responseParts = attachToolOutputRecovery(
attachToolOutputProvenance(call.response.responseParts, provenance ?? { âĶ }),
recovery,
);Reading before the spread is the right order â reading after would always find nothing â and the non-enumerable definition is deliberate rather than accidental: it keeps a part comparable to its own serialized form so the transcript assertion holds and the handle cannot leak into a model request. The commit adds 60 lines of tests alongside it. This part needs no change.
3. My own scan remains incomplete
This is the second head in a row where I could not finish the current-code gate. Unread at adeea4b9: packages/core/src/core/llm-chat.ts (+393/â65, the pressure-budget anchor and its invalidation), packages/core/src/tools/tool-response-finalizer.ts (+224/â49, allocation and the sub-notice veto), packages/core/src/tools/agent/agent.ts (+45/â44), microcompact.ts, fileReadCache.ts, tokenEstimation.ts and web-fetch.ts. I read tool-output-recovery.ts in full and the scheduler hunk; that is not coverage of a 22-file, +2399-line diff. I am recording these as unperformed gates rather than reporting findings I cannot prove.
4. CI at this head
14 checks pass, 7 skipped, 1 pending, 1 failed. The failure is the Managed Agent gate job (run 40275374499 / job 121416902625), reported with a 0-second duration and its name still an unevaluated conditional expression over needs.scope.outputs, which reads as the job failing before it resolved its own identity rather than failing an assertion. This PR's change set is entirely packages/core plus two design docs and contains no managed-agent-server code, so I have no evidence tying the failure to it â and per this channel's rules an unattributed failure does not change the verdict. It is worth a look before merge simply because a red gate blocks the lane.
Note, non-blocking
attachToolOutputRecovery defines the symbol unconditionally on every part, so the scheduler's ?? {} fallback attaches an empty handle rather than leaving the part without one. getToolOutputRecovery then returns {}, which is truthy, so a consumer testing if (getToolOutputRecovery(part)) cannot distinguish "no handle" from "empty handle" â while one testing if (recovery?.text) can. I did not read the consumers at this head and am not filing this as a defect; it is worth confirming that every consumer guards on the fields rather than on the object's presence, since find(Boolean) already treats {} as a hit.
Next step
The blocking path is unchanged from my last review: address R12-1, R16-1 and R18-1 at a new head, or reply on each with the code evidence that refutes it and have it dismissed. A fresh review round at this head would also settle them, since none has run since fcd2443.
|
Re-anchoring the three round-18 Criticals at the current head To be explicit about what I am not claiming â I am not saying any of these are fixed. I'm saying the anchors are dead, so "still standing at The baseline hop is the issueRound 18 (review
So a Anchor state at
|
| finding | anchor | file length now | named symbol |
|---|---|---|---|
| R12-1 | tools/truncation.ts:844 |
821 lines â line 844 does not exist | persistedOutputPointerRange: 0 hits repo-wide; matched in 2 files at fcd2443cd2 |
| R18-1 | tools/agent/transcript-marker.ts:23 |
20 lines â line 23 does not exist | file was 43 lines at fcd2443cd2 |
| R16-1 | tools/tool-response-finalizer.ts:132 |
740 lines, so the line resolves â but to recoveryHeader() / recoveryOf(), not the code round 18 meant |
file was 680 lines at fcd2443cd2 |
Positive control, so the zero isn't a broken probe: at the same head git grep isAlreadyTruncated hits coreToolScheduler.ts (3), truncation.test.ts (3) and truncation.ts (1). Case-insensitive PointerRange and persistedOutputPointer are both 0 as well.
Per finding
R12-1 â the mechanism it names is gone, not merely moved. b69dfdd104 deletes persistedOutputPointerRange outright: the import, the return persistedOutputPointerRange(text) ?? transcriptMarkerRange(text) call site, and the export function definition are all in the deletion side of that commit. transcriptMarkerRange is likewise absent now. What replaced it is a structured side channel â tool-output-recovery.ts attaches a non-enumerable Symbol('tool-output-recovery') to the part, read back through getToolOutputRecovery (3 sites each in tool-response-finalizer.ts and coreToolScheduler.ts). Residual text-scraping in the changed files is one unrelated continuationText.match(/^\s+/) in llm-chat.ts. That is the shape this finding was asking for, so I think R12-1 wants withdrawing or restating against the new mechanism rather than being carried again.
R16-1 / R18-1 â anchors dead, substance not re-derived. I'm deliberately not calling these fixed. The budget-charging logic R18-1 describes now lives around minimumRecoveryBudget(slot) at tool-response-finalizer.ts:138-145, applied as a floor at lines 440, 474 and 574. Worth flagging that this floor protects the recovery handle from being shortened â which is closer to the behaviour R18-1 objects to than to a refutation of it, so if anything R18-1 deserves a fresh look at that coordinate. R16-1's idempotence question now belongs at recoveryOf() (:118-128) / recoveryHeader() (:130-136) / fitText() (:304).
One logistics note
The review lane won't settle this on its own. Run 40275374400 (pull_request_target, created 05:39:12Z) has been queued about nine hours with precheck-pr / precheck still queued and review-config skipped, and there is no bot review at all on adeea4b9d7. A fresh round would have to be triggered deliberately rather than waited for.
Not touching review state here â no dismiss, no approve, no /triage.
|
Recovery metadata now survives hook-context appends at dd3a7ab, after merging current main Independent verification of this exact commit passed: 29 tests across four files, zero failures, 996 tests filtered by the explicit name selector. Core and Browser Use rebuilt successfully; core typecheck and scoped lint/format checks passed. The checks comprise five finalizer tests, four AgentTool tests, four scheduler tests, and the complete 16-test hook-context suite. The preserved before/after mechanism witness calls the actual compiled conversion, hook-context and async-finalizer APIs with persistence quota exhausted and a 660-character budget:
The checked-in scheduler cases additionally exercise actual PreToolUse/PostToolUse message-bus composition at budget 660. They use declared known-no-file/no-retry metadata; quota exhaustion is exercised separately by the external witness above. Helper regressions cover both output/error fields through the synchronous send guard. Disposition of the three standing mechanisms:
Older raw CHANGES_REQUESTED reviews also contain R11-1/R11-2, R13-1 and R15-1/R15-2; these remain UNADJUDICATED in this scoped pass. Reviews This evidence covers mechanism composition and scheduler integration; no public CLI/provider E2E, live hook subprocess, full-suite or whole-repository verification is claimed. New CI runs have been triggered on the delivered head; approval and broader PR review remain separate. |
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: build-and-test â Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (win32 exposure independently probed and posted as R19-17).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": none of my checks were cut short, but two verifications are arithmetic-derived rather than executed â I did not run the suite or the two proposed mutations, so âĶ; "agent reverse-audit (round 1)": the telemetry layer inside finalizeToolResponses ( logToolResultSize / ToolResultSizeEvent attribution) â not walked; the emitting lines sit outside chunk âĶ; "agent reverse-audit (round 1)": I did not enumerate every {...part} spread-copy site in packages/core to check which re-attach the recovery handle (only tool-hook-context.ts:109-110 veriâĶ; "agent reverse-audit (round 1)": no vitest run â the pass/fail arithmetic for each new row is hand-traced from allocateTextBudget / fitText , not executed.; "agent reverse-audit (round 1)": did not run packages/core vitest, so I could not confirm whether an existing (non-diff) test in llm-chat.test.ts already reaches the shrink with non-empty âĶ, and 3 more.
Not reviewed: reverse audit â stopped before round 4 by the review time budget.
21 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement â 19 listed, 2 more inside the overflow count).
Deferred under the convergence posture (round 19, 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/core/coreToolScheduler.ts:6458 â [probe] Critical [fails-closed] [new-surface] persistedOutputFiles: [] on recovery handles suppresses the send boundary's own persistence â pointerless notice, output nowhere on disk; [] isâĶdocs/design/pressure-aware-tool-output.md:17 â [review] R19-4: The doc promises an inline recovery handle â naming a spill envelope's Full output saved to: line alongside the subagent transcript marker â is 'kept whole above the preview, âĶpackages/core/src/core/llm-chat.test.ts:14788 â [review] R19-5: .every() over the filtered resultOutputs passes vacuously on an empty array, and the companion resultChars(1) < 8_000 also holds at 0 â this media-charging test can silenâĶpackages/core/src/core/llm-chat.test.ts:14847 â [review] R19-6: The forked-chat side of the worker-invalidation catch â if (!this.isForkedChat) fileReadCache.clear(); (llm-chat.ts:3782) â is untested, and it is a third branch distinct froâĶpackages/core/src/core/llm-chat.test.ts:15219 â [review] R19-7: The only test named for the 'exempt tool error is cut twice' defect asserts a one-sided upper bound plus an equality between two computed sums, so any SMALLER budget â exactly âĶpackages/core/src/core/llm-chat.ts:2958 â [review] R19-8: The new anchor-invalidation set (both usage setters, setSystemInstruction, setTools, setHistory, clearHistory, dispatch, retry restore) misses the third place that discards every tokâĶpackages/core/src/core/llm-chat.ts:3610 â [review] R19-9: These synthetic entries make the send boundary a NEW disk-persisting caller â the guard it replaced never wrote anything â and every artifact and ToolResultPersistedEvent is attribâĶpackages/core/src/core/llm-chat.ts:3630 â [review] R19-10: This pressure-vs-aggregate selection branch â the only code that can RAISE the budget back to the aggregate after a pressure preview â is entered by no test in the diff: it needs pâĶpackages/core/src/core/llm-chat.ts:3758 â [review] R19-11: This block stats every cut path on the LOCAL filesystem unconditionally and then also calls getExecutionEnvironment?.()?.invalidateReadCache(paths) â which for a local environmentâĶpackages/core/src/core/llm-chat.ts:3771 â [review] R19-12: Both new cache-disarm fallback branches are unexercised: the non-fork markAllReadsEvictedFromHistory() (its only production call site â all seven assertions in the new block are âĶpackages/core/src/core/llm-chat.ts:3771 â [review] R19-13: The non-fork branch escalates ONE unresolvable path to a cache-GLOBAL residency disarm while holding that exact path, discarding the per-path invalidateByPath fallback that the foâĶpackages/core/src/core/llm-chat.ts:3793 â [review] R19-14: The main-chat clearLoadedSkillTracking call at the send boundary has no test observing it fire; the only clearLoadedSkills assertion in the pressure block is the forked-chat negâĶpackages/core/src/core/llm-chat.ts:5906 â [review] R19-15: This anchor clear + version bump on the previousGenerationConfig restore has no test: no case in the pressure block dispatches with options.preparedRequest , the only condition uâĶpackages/core/src/core/llm-chat.ts:7828 â [review] R19-16: The new usage anchor derives tokens from the RAW usageMetadata , bypassing the coerceUsageCount sanitisation the same function applies at :7117-7138 and both sibling record sitâĶpackages/core/src/services/microcompaction/microcompact.test.ts:2048 â [review] R19-17: (pattern, location 1 of 2 â the second is llm-chat.test.ts:14978) This expectation hardcodes the POSIX result of unescapePath , which is an intentionalâĶpackages/core/src/core/llm-chat.test.ts:14978 â [review] R19-17: (pattern, location 2 of 2 â the first is microcompact.test.ts:2048) This stat-argument expectation hardcodes the POSIX result of unescapePath ; on win32 the value keeps its lâĶpackages/core/src/tools/agent/transcript-marker.ts:20 â [review] R19-18: This module was extracted to be the marker's single source of truth, and two tests source it (coreToolScheduler.test.ts:4113, tool-hook-context.test.ts:96) â but the sâĶpackages/core/src/tools/tool-response-finalizer.test.ts:854 â [review] R19-19: Inside this it.each over six send budgets the assertion hardcodes 0 , so it is identical in all six rows and pins only the guard's zero-budget early return â âĶpackages/core/src/tools/tool-response-finalizer.test.ts:936 â [review] R19-22: This test pins a broken combination in the production veto it exercises: the 'too small for a meaningful result' veto (tool-response-finalizer.ts:565-576) is evaâĶpackages/core/src/tools/tool-response-finalizer.ts:335 â [review] R19-20: The de-dup strip removes producer-owned strings with a GLOBAL body.split(owned).join('') â correct only while each owned is a unique handle â but two of the threeâĶ- âĶand 6 more (see the run report)
Convergence: round 19 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 3 (1 new). The rate of new findings is not falling. 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.)
Residual risk: this loop is persistently critical â Criticals stood in the previous round's work-list and stand again this round (3 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.
äļæčŊīæ
äŧ åŪæéĻååŪĄæĨïžåŪĄæĨįžšåĢå·ēæŦéēã
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
æŠåŪĄæĨïžåæäļščąæïžïžbuild-and-test â Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally (win32 exposure independently probed and posted as R19-17).
æŠæĒįīĒå°å
ĻéĻæ·ąåšĶïžčūūå°å·Ĩå
·č°įĻéĒįŪïžïž"agent reverse-audit (round 3)"ïžnone of my checks were cut short, but two verifications are arithmetic-derived rather than executed â I did not run the suite or the two proposed mutations, so âĶïž"agent reverse-audit (round 1)"ïžthe telemetry layer inside finalizeToolResponses ( logToolResultSize / ToolResultSizeEvent attribution) â not walked; the emitting lines sit outside chunk âĶïž"agent reverse-audit (round 1)"ïžI did not enumerate every {...part} spread-copy site in packages/core to check which re-attach the recovery handle (only tool-hook-context.ts:109-110 veriâĶïž"agent reverse-audit (round 1)"ïžno vitest run â the pass/fail arithmetic for each new row is hand-traced from allocateTextBudget / fitText , not executed.ïž"agent reverse-audit (round 1)"ïždid not run packages/core vitest, so I could not confirm whether an existing (non-diff) test in llm-chat.test.ts already reaches the shrink with non-empty âĶïžåĶæ 3 æĄã
æŠåŪĄæĨïžåååŪĄčŪĄââčŊåŪĄæķéīéĒįŪäļčķģïžæŠč―åžå§įŽŽ 4 č―Ūã
21 æĄ Suggestion åĻå·ēč§Ģæį critical ååļäļéäđåĪčĒŦčĩ·čäļščĄå čŊčŪšïžCLI å·ēå°å ķį§ŧå Ĩäļæđåŧķåæļ åïžäļéåžšåķæ§čĄââååš 19 æĄïžå ķä― 2 æĄčŪĄå ĨæšĒåščŪĄæ°ïžã
æķæå§ŋæäļåŧķåïžįŽŽ 19 č―Ūïžééŧæïžââå·ēčŪ°å―ïžæŽč―ŪäļčĶæąäŋŪæđïžå ķäļ 1 æĄ Critical æå ķåĪąčīĨæđåäļåŊđį §åšįšŋåŧķåââfails-closed äļ new-surfaceïžæŠčŪĪčŊäŧŧä―éčŊŊįŧæïžäļ merge base æĒæ čŊĨåč―éĒäđæ čŊĨįžšé·ââä―äļšåįŧå·Ĩä―čŪ°å―åĻ findings å·Ĩäŧķäļïžå ą 26 æĄïžåææŠįŋŧčŊïžåčĄĻč§äļæđčąæéĻåïžã
æķææ åĩïžįŽŽ 19 č―Ūååļäš 3 æĄčĄå čŊčŪšïžå ķäļ 3 æĄæŊéĶæŽĄæåšïžäļäļč―Ūååļäš 3 æĄïžå ķäļ 1 æĄéĶæŽĄæåšïžãæ°åį°į䚧åšéåšĶæēĄæäļéãæåĐä―äŋŪåĪææäļæđãéŠčŊååæĻéïžåŊäŧĨéŋå åūŠįŊååĪæĻåŊžåäļįŧåį°ïžæŽ PR įčŊåŪĄå·ēč§Ģæäļš critical ååļäļéãïžäŧ äļšč§åŊââæŽč―ŪčŊåŪĄæŠå æĪæĢįäŧŧä―å åŪđãïž
æŪä―éĢéĐïžæŽåūŠįŊåĪäš persistently-critical å―Ēæââäļäļč―Ūå·Ĩä―æļ
åäļį Critical æŽč―ŪäūįķååĻïžæŽč―Ū 3 æĄ CriticalïžïžéĶæŽĄåį°įéįæēĄæäļéïžæŽč―Ū 3ïžäļäļč―Ū 1ïžïžäļæŠåģ Critical į§ŊåæēĄæåå°ãseverity floor æ æģä―ŋå
ķæķæãåŧščŪŪïžland-with-residual-riskââåšåĢæŊ maintainer įéĢéĐæĨååģåŪïžåå
Ĩåđķæŋæ
æŪä―éĢéĐïžïžčéååžäļč―ŪčŊåŪĄãäūčŊĨåģåŪä―ŋįĻįæŪä―éĢéĐæļ
åïžmaintainer åĄŦåïžïžææŊæĄæŠåģ Critical ååšãæŧåŧéĒ · æŧåŧč
äūčĩæ§ Â· å―ąåčåīãäļæ ãäŧ
äļšåŧščŪŪââäļéŧææŽæŽĄčŊåŪĄã
â qwen3.8-max via Qwen Code /review (v0.26.1)
| const guarded = | ||
| unresolvedRead || (hookRetargetPossible && cutRead) | ||
| ? entries |
There was a problem hiding this comment.
[Critical] R19-1: [fails-closed] [regression] The hookRetargetPossible && cutRead veto is matcher-agnostic and skips the cut ENTIRELY: any configured PreToolUse/PermissionRequest hook â whatever its matcher, even one that cannot rewrite input â plus any would-be-cut read/edit/write/tool_call result waives both the new pressure budget and the pre-existing unconditional getToolOutputBatchBudget() send guard this diff removed.
One formatter hook matching only edit is enough: a ~600k-char batch containing one read_file whose target resolved fine ships uncut (~150k tokens against a 200k-char budget), including unrelated shell output. The #2566 replay also fails to converge for hook sessions â the pressured send trips the veto, effectiveTokens stays at or above auto, and tryCompress runs. The residency-invalidation block this same hunk adds (stat / markReadEvictedFromHistory / invalidateReadCache / clearLoadedSkillTracking) is dead code in exactly that configuration, so the design doc's promise that resolvable shortened reads still disarm their quote-back cache entries is unreachable there.
Witness:
PR PROBE-F1a-hook dispatchedChars=700000 (PreToolUse hook, resolvable read_file)
PR PROBE-F1a-control dispatchedChars=200000 (same batch, no hook)
BASE BASE-PROBE chars=200000 (same batch, hook configured â base has no veto)
PR PROBE-F1b-hook effectiveTokens=856042 (>= auto â tryCompress runs)
PR PROBE-F1b-control effectiveTokens=848670 (< auto â #2566 converges)
CANDIDATE FIX: both arms flipped; exactly the two pinning it.each rows went red
Narrow the second disjunct to the design doc's actual exception: set unresolvedRead inside the loop only when a cut read-ish result has NO derivable path and a retargeting hook is possible; when paths resolved, cut the batch and disarm via paths as the rest of the hunk does. The fix must not violate: hasHooksForEvent is not tool-specific â sessionHooksManager.ts:269 iterates every session's hooks for the event with no matcher check (the matcher-aware getMatchingHooks sits immediately below), and skill hooks register WITH a matcher (registerSkillHooks.ts:97-104), so hasHooksForEvent('PreToolUse') does not imply a hook that can retarget read_file. Acceptance: narrow the existing preserves a cut read batch when a %s hook could have retargeted it rows to the unresolvable-target scenario and add a case with hasHooksForEvent true and a RESOLVABLE cut read_file asserting resultChars(1) < 20_000 and a cache disarm ran â removing the narrowing reddens the new case (probe-verified: the candidate fix flips exactly these).
äļæčŊīæ
[Critical] R19-1ïž[fails-closed] [regression] hookRetargetPossible && cutRead äŋįåæŊäļ matcher æ å
ģïžäļäžæīä―č·ģčŋčĢåŠïžåŠčĶé
į―Ūäšäŧŧæ PreToolUse/PermissionRequest éĐåïžæ čŪšå
ķ matcher æŊäŧäđãæŊåĶč―æđåčūå
ĨïžïžæđæŽĄéåæäŧŧä―äžčĒŦčĢåŠį read/edit/write/tool_call įŧæïžæīæđå°ąåæķčąå
äšæ°įååéĒįŪåæŽ diff å éĪįãåæŽæ æĄäŧķæ§čĄį getToolOutputBatchBudget() åéåŪåŦã
äļäļŠåŠåđé
edit įæ žåžåéĐåå°ąčķģåĪïžå
åŦäļæĄč·ŊåūåŊč§Ģæį read_file įįšĶ 600k åįŽĶæđæŽĄäžåæ ·ååšïžįšĶ 15 äļ tokenïžčéĒįŪæŊ 200k åįŽĶïžïžčŋåæ å
ģį shell čūåšã#2566 įåĪį°č·ŊåūåĻéĐåäžčŊäļäđäļæķæââåååéč§ĶåäŋįåæŊïžeffectiveTokens äŧäļä―äš autoïžtryCompress į
§åļļæ§čĄãåäļ hunk æ°åĒįéĐŧįåĪąæåïžstat / markReadEvictedFromHistory / invalidateReadCache / clearLoadedSkillTrackingïžåĻčŊĨé
į―ŪäļæŊæŧäŧĢį ïžčŪūčŪĄææĄĢãåŊč§ĢæįčĢåŠčŊŧåäŧäžč§ĢéĪå
ķåžįĻįžåæĄįŪãįæŋčŊšåĻæĪäļåŊčūūã
čŊæŪč§äļæđ Witness äŧĢį åïžæĒéæūįĪšéĐåčååš 700000 åįŽĶãåŊđį
§č 200000ãååđķåšįšŋïžé
į―ŪäšéĐåïžäŧčĢå° 200000ïžåéäŋŪåĪïžåŧæ hookRetargetPossible && cutReadïžä―ŋäļĪčįŋŧč―Žïžäļæ°åĨ―åŠæéä―æ§čĄäļšįäļĪäļŠ it.each įĻäūåįšĒã
äŋŪåĪïžæįŽŽäšäļŠæåéĄđæķįŠäļščŪūčŪĄææĄĢįæĢįäūåĪââäŧ
å―čĒŦčĢåŠįčŊŧįąŧįŧææ æģæīūįč·ŊåūäļåŊč―ååĻæđåéĐåæķæį―Ū unresolvedReadïžč·ŊåūåŊč§Ģææķį
§åļļčĢåŠåđķæ paths č§ĢéĪæĶčĢ
ãäŋŪåĪäļåūčŋåįæĒæäšåŪïžhasHooksForEvent äļå·Ĩå
·æ å
ģââsessionHooksManager.ts:269 éåčŊĨäšäŧķäļææäžčŊéĐåãäļæĢæĨ matcherïžmatcher æįĨį getMatchingHooks å°ąåĻå
ķäļæđïžïžäļæč―éĐåæ matcher æģĻåïžregisterSkillHooks.ts:97-104ïžïžå æĪ hasHooksForEvent('PreToolUse') äļč―æĻåšååĻč―æđå read_file įéĐåãéŠæķïžæį°æ preserves a cut read batch when a %s hook could have retargeted it äļĪčĄæķįŠå°ãįŪæ äļåŊč§ĢæãåšæŊïžåđķæ°åĒ hasHooksForEvent äļš trueãread_file č·ŊåūåŊč§ĢæįįĻäūïžæčĻ resultChars(1) < 20_000 äļįžåč§ĢéĪčĒŦč°įĻââį§ŧéĪæķįŠåčŊĨįĻäūåŋ
éĄŧåįšĒïžæĒéå·ēéŠčŊįŋŧįšĒčåīæ°åĨ―æŊčŋäļĪåĪïžã
â qwen3.8-max via Qwen Code /review (v0.26.1)
There was a problem hiding this comment.
Changed in 3587a76baa, but not removed. The veto is now gated on hasHooksMatchingTool per event and tool name and honors getDisableAllHooks, so a formatter hook matching only edit no longer preserves a read batch.
I did not adopt "resolvable original path â cut": updatedInput can execute B while history records A, and invalidating A would leave B resident. For a matching read hook the batch is still preserved whole rather than cut partially, because the finalizer applies one batch budget and a partial preserve would need a new per-entry path. Leaving this open for your call.
Local verification (merge-reference) at
|
| before arm | after arm | |
|---|---|---|
| Revision | 2f3d1b2ff53b6ae45bc022d1ff415e679d944015 (real git merge-base vs origin/main) |
dd3a7abdc8aa0c2d49d09332225654bbcf74bffd (live head) |
| Worktree | detached, independent | detached, independent |
| Install | pnpm install --frozen-lockfile, rc=1 |
pnpm install --frozen-lockfile, rc=0 |
node_modules |
1238 entries, vitest 3.2.7 | 1238 entries, vitest 3.2.7 |
packages/core/dist |
absent | present, then moved aside for the decisive run |
Lockfile is byte-identical across arms (bd920144edd322ad19245bc0ccfb884b), but per the discipline that a shared lockfile does not prove installed-artifact equivalence, each arm got its own install â no hardlinked or donated node_modules.
The base arm's rc=1 is not a dependency failure: it is the prepare hook's tsc --build being SIGKILLed (OOM, two parallel builds), after linking had completed. Both arms run packages/core tests from TypeScript source via a scratch vitest config with globalSetup (the workspace-dist guard) removed. To prove the result does not depend on build artifacts, the after arm was re-run with its 90 MB packages/core/dist moved aside: identical output (10631 chars / [167, 10993] bytes). Both arms were therefore dist-less and symmetric for that comparison.
The harness is byte-identical in both arms (md5 835da7a4441313353141e223bedd45fc) at distinct inodes (1489885 vs 6829812), so neither arm could write through to the other.
What was observed
The probe drives the real LlmChat send path through the real OpenAIContentGenerator / DefaultOpenAICompatibleProvider into a real loopback HTTP sink, then reads back the request bodies the sink actually received. The oracle is bytes on a socket, not the return value of enforceFunctionResponseBudget (whose return value is discarded at all of its call sites, so it cannot be the send boundary).
Scenario: turn 1 seeds a usage anchor and requests the shell tool; turn 2 submits a 20,000-char tool result.
| # | Scenario | Arm | Prompt tokens | Chars on wire | Request bytes | Payload head |
|---|---|---|---|---|---|---|
| 1 | near auto-compaction | before | 846,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 2 | near auto-compaction | after | 846,000 | 10631 | [167, 10993] |
[{"type":"text","text":"Tool output truncated.âĶ |
| 3 | far from auto-compaction (control) | before | 100,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 4 | far from auto-compaction (control) | after | 100,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 5 | near auto, dist removed (identity control) |
after | 846,000 | 10631 | [167, 10993] |
[{"type":"text","text":"Tool output truncated.âĶ |
Causal difference in one sentence: near the trigger the emitted tool result shrinks from 20,027 to 10,631 characters and gains a real Tool output truncated. notice, while far from the trigger both arms emit byte-identical payloads â the pressure path is gated on the usage anchor and leaves static behavior untouched.
Rows 3â4 are the control that rules out "the PR just truncates more aggressively everywhere". Rows 1 and 2 differ, which is the hard indicator that the rig still arms; identical observations in both arms would have meant the harness lost discriminating power rather than the behavior being unchanged.
Regression pin
Head's own packages/core/src/core/tool-hook-context.test.ts (16 cases, including the two preserves producer recovery through context and another budget cases added by dd3a7abdc8) run against base's unfixed tool-hook-context.ts:
base: Tests 2 failed | 14 passed (16)
à preserves producer recovery through context and another budget (output)
à preserves producer recovery through context and another budget (error)
AssertionError: expected 'Tool output truncated.\n\nSubagent diâĶ'
to contain 'Subagent did not complete (terminate âĶ'
head: Tests 16 passed (16)
Collected-count parity holds (16 == 16), so the red is a genuine assertion failure, not a collection failure. Base's tool-hook-context.ts was confirmed unfixed beforehand (grep -c setToolOutputRecovery â 0), and the file was restored to its pristine md5 b01c2f0c530e088636b9aaa65c477187 afterwards.
Controlled hybrid, labelled: that test imports tools/tool-output-recovery.ts and tools/agent/transcript-marker.ts, which this PR adds and which do not exist at the merge-base. Those two additive leaf modules were copied into the base arm so the test could collect; base's own tool-hook-context.ts was left untouched. The pin therefore isolates the recovery-carry behaviour, not the existence of the new modules.
Independent oracle
The PR's own suites at head, run separately from the probe: 10 files / 2017 tests passed (llm-chat 708, coreToolScheduler 539, agent 421, web-fetch 114, microcompact 103, fileReadCache 61, tool-response-finalizer 49, tool-hook-context 16, tool-response-finalizer.integration 3, goal-turn-integration 3). This cross-checks that the mechanism works under the product's own assertions, not only under my harness.
Boundaries and gaps
- The provider is a scripted loopback sink, not a real model. This is real-stack, controlled-provider evidence about what is sent. It is not real-model acceptance, and nothing here proves a provider would accept the shrunk payload.
- No real CLI/TUI process. The send path ran in-process under vitest, not through a spawned
qwenbinary. A full CLI end-to-end leg remains NOT VERIFIED (notN/A) â it needs the workspacedistchain, which currently fails to build on this machine (build --cli-onlydies inbrowser-use;bundlefails to resolve@qwen-code/channel-baseand@qwen-code/acp-bridge/*). - Base arm install
rc=1(OOM inprepare), mitigated by the dist-removed re-run in row 5 rather than by a clean base build. - Scope is the emitted-payload wire boundary. Persistence of full originals, the PreToolUse empty-argument repair path, worker cache residency, and media/framing charging order were not independently exercised this round; the product's own suites cover parts of them, but I am not claiming separate evidence for them.
A local PASS is not a merge approval. reviewDecision is still CHANGES_REQUESTED.
VERDICT: PASS â scoped to the emitted-payload wire boundary at dd3a7abdc8aa0c2d49d09332225654bbcf74bffd, with the CLI end-to-end leg recorded as NOT VERIFIED.
åŪæīäļææĨå
æŽå°éŠčŊïžmerge-referenceïž@ dd3a7abdc8ïžåĻååšč――č·į wire čūđįäļ PASS
čŠæŽ PR äļäļäŧ―æĨåäŧĨæĨ live head å·ēåį§ŧïždd8d472239 â dd3a7abdc8ïžäļäļŠčŠææäšĪ + äļæŽĄååđķ mainïžmerge-base įą 4b136b527f åį§ŧå° 2f3d1b2ff5ïžįļå·Ū 343 äļŠæäšĪïžãčŋä―ŋæ§įåšįšŋčä―åšïžå æĪæŽč―ŪæŊå
Ļæ°įäļĪčéŠčŊïžäļæŊåĪįĻæ§æ°æŪã
äļšäŧäđæŊį°åĻïž čŊĨ head į CI éĶæŽĄč―å°ãæĪååĄåĻ 13/23 æĨãå·ēč· 343 åéį review-pr č
ŋäš 02:20:48Z åäļš completed/successïž24/24 æĨïžjob 121827650289ïžãčŊĨ head äļ 21 æĄ run å
ĻéĻčŋå
Ĩįŧæã0 åĪąčīĨïž86 äļŠ check-run å
ĻéĻ completedïž0 failedã0ãæ æååįŧ§į cancelledããmergeable=trueïžmergeStateStatus=BLOCKED åŠæĨčŠ reviewDecision=CHANGES_REQUESTEDïžäļ CI æ å
ģã
čŋčĄæķčšŦäŧ―
| before č | after č | |
|---|---|---|
| įæŽ | 2f3d1b2ff53b6ae45bc022d1ff415e679d944015ïžåŊđ origin/main įįåŪ git merge-baseïž |
dd3a7abdc8aa0c2d49d09332225654bbcf74bffdïžlive headïž |
| worktree | įŽįŦ detached | įŽįŦ detached |
| åŪčĢ | pnpm install --frozen-lockfileïžrc=1 |
pnpm install --frozen-lockfileïžrc=0 |
node_modules |
1238 éĄđïžvitest 3.2.7 | 1238 éĄđïžvitest 3.2.7 |
packages/core/dist |
įžšåĪą | ååĻïžåģåŪæ§éĢäļč·åå·ēį§ŧåž |
äļĪč lockfile éåčįļåïžbd920144edd322ad19245bc0ccfb884bïžïžä―æãlockfile įļåäļč―åįŽčŊæå·ēåŪčĢ
äūčĩäļčŋčĄäš§įĐįäŧ·ãįįšŠåūïžæŊčåčŠįŽįŦåŪčĢ
ââæēĄæįĄŽéūæĨãæēĄæåįĻ donor į node_modulesã
base čį rc=1 äļæŊäūčĩåĪąčīĨïžæŊ prepare éĐåéį tsc --build čĒŦ SIGKILLïžäļĪäļŠåđķčĄæåŧšåŊžčī OOMïžïžčéūæĨåĻæĪäđåå·ēåŪæãäļĪčé―įĻåŧæ globalSetupïžworkspace dist åŪåŦïžįäļīæķ vitest configïžäŧ TypeScript æšį č· packages/core æĩčŊãäļščŊæįŧæäļäūčĩæåŧšäš§įĐïžafter čåĻæ 90 MB į packages/core/dist į§ŧåžåéč·ïžčūåšåŪå
Ļäļčīïž10631 åįŽĶ / [167, 10993] åčïžãå æĪéĢæŽĄåŊđæŊäļäļĪčé―æŊæ dist äļåŊđį§°įã
čĢ
į―ŪåĻäļĪčéåčįļåïžmd5 835da7a4441313353141e223bedd45fcïžãinode äšäļįļåïž1489885 vs 6829812ïžïžäŧŧäļčé―æ æģåįĐŋå°åĶäļčã
č§æĩå°äŧäđ
æĒééĐąåĻįåŪ LlmChat åéč·ŊåūïžįŧįåŪ OpenAIContentGenerator / DefaultOpenAICompatibleProviderïžæčŋįåŪ loopback HTTP sinkïžåčŊŧå sink åŪé
æķå°įčŊ·æąä―ãoracle æŊ socket äļįåčïžäļæŊ enforceFunctionResponseBudget įčŋååžïžčŊĨå―æ°čŋååžåĻå
ķå
ĻéĻč°įĻįđé―čĒŦäļĒåžïžå æĪåŪäļåŊč―æŊåéčūđįïžã
åšæŊïžįŽŽ 1 č―Ūį§äļįĻééįđåđķčŊ·æą shell å·Ĩå
·ïžįŽŽ 2 č―ŪæäšĪäļäļŠ 20,000 åįŽĶįå·Ĩå
·įŧæã
| # | åšæŊ | č | prompt tokens | äļįšŋåįŽĶæ° | čŊ·æąåč | č――č·åžåĪī |
|---|---|---|---|---|---|---|
| 1 | æĨčŋčŠåĻåįžĐ | before | 846,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 2 | æĨčŋčŠåĻåįžĐ | after | 846,000 | 10631 | [167, 10993] |
[{"type":"text","text":"Tool output truncated.âĶ |
| 3 | čŋįĶŧčŠåĻåįžĐïžæ§åķįŧïž | before | 100,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 4 | čŋįĶŧčŠåĻåįžĐïžæ§åķįŧïž | after | 100,000 | 20027 | [167, 20389] |
[{"type":"text","text":"xxxxâĶ |
| 5 | æĨčŋčŠåĻåįžĐãį§ŧéĪ distïžčšŦäŧ―æ§åķïž |
after | 846,000 | 10631 | [167, 10993] |
[{"type":"text","text":"Tool output truncated.âĶ |
äļåĨčŊå æïžæĨčŋč§Ķåįđæķïžååšįå·Ĩå
·įŧæäŧ 20,027 åįŽĶįžĐå° 10,631 åįŽĶåđķåļĶäļįåŪį Tool output truncated. æįĪšïžčŋįĶŧč§ĶåįđæķäļĪčååšéåčįļåįč――č·ââååč·ŊåūäŧĨįĻééįđäļšéĻæ§ïžéæčĄäļšæŠčĒŦæđåĻã
įŽŽ 3ã4 čĄæŊæ§åķįŧïžįĻæĨæéĪãčŋäļŠ PR åŠæŊå°åĪé―æīæŋčŋå°æŠæããįŽŽ 1ã2 čĄååĻå·ŪåžïžčŋæĢæŊčĢ į―Ūäŧįķ arm čĩ·æĨįįĄŽææ ïžčĨäļĪčč§æĩåžįļåïžéĢčŊīæčĢ į―ŪåĪąåŧåĪåŦåïžčäļæŊčĄäļšäļčīã
åå―é
head čŠåļĶį packages/core/src/core/tool-hook-context.test.tsïž16 äļŠįĻäūïžåŦ dd3a7abdc8 æ°åĒįäļĪæĄ preserves producer recovery through context and another budgetïžč·åĻ base æŠäŋŪåĪį tool-hook-context.ts äļïž
base: Tests 2 failed | 14 passed (16)
à preserves producer recovery through context and another budget (output)
à preserves producer recovery through context and another budget (error)
AssertionError: expected 'Tool output truncated.\n\nSubagent diâĶ'
to contain 'Subagent did not complete (terminate âĶ'
head: Tests 16 passed (16)
äļĪčæķéæ°įļåïž16 == 16ïžïžæäŧĨčŋäļŠįšĒæŊįåŪæčĻåĪąčīĨïžäļæŊæķéåĪąčīĨãäšå
å·ēįĄŪčŪĪ base į tool-hook-context.ts æŊæŠäŋŪåĪįæŽïžgrep -c setToolOutputRecovery â 0ïžïžäšåčŊĨæäŧķå·ēčŋåå°åå§ md5 b01c2f0c530e088636b9aaa65c477187ã
åæ§ hybridïžæįĄŪæ æģĻïž čŊĨæĩčŊ import äš tools/tool-output-recovery.ts äļ tools/agent/transcript-marker.tsïžčŋäļĪäļŠæäŧķįąæŽ PR æ°åĒãåĻ merge-base äļäļååĻãäļšäščŪĐæĩčŊč―åĪæķéïžæčŋäļĪäļŠįšŊčŋ―å įåķåæĻĄåæ·čŋäš base čïžbase čŠå·ąį tool-hook-context.ts æŠåĻãå æĪčŋäļŠééįĶŧįæŊãrecovery æŊåĶčĒŦåļĶčŋåŧãčŋäļčĄäļšïžäļæŊæ°æĻĄåæŊåĶååĻã
įŽįŦ oracle
äļæĒéååžč·į head äū§ PR čŠæåĨäŧķïž10 äļŠæäŧķ / 2017 äļŠįĻäūå
Ļįŧŋïžllm-chat 708ãcoreToolScheduler 539ãagent 421ãweb-fetch 114ãmicrocompact 103ãfileReadCache 61ãtool-response-finalizer 49ãtool-hook-context 16ãtool-response-finalizer.integration 3ãgoal-turn-integration 3ïžãčŋæĄäšĪåéŠčŊčŊīæčŊĨæšåķåĻ䚧åčŠå·ąįæčĻäļäđæįŦïžčäļåŠåĻæįčĢ
į―ŪäļæįŦã
čūđįäļįžšåĢ
- provider æŊčæŽåį loopback sinkïžäļæŊįåŪæĻĄåã čŋæŊįåŪæ ãåæ§ provider įčŊæŪïžčŊæįæŊååšäšäŧäđïžäļæŊįåŪæĻĄåįæĨæķčŪĪåŊïžäđäļč―čŊæäŧŧä― provider äžæĨåįžĐå°åįč――č·ã
- æēĄæįåŪ CLI/TUI čŋįĻã åéč·ŊåūæŊåĻ vitest čŋįĻå
éĐąåĻįïžäļæŊéčŋ spawn åšæĨį
qwenäščŋåķãåŪæī CLI įŦŊå°įŦŊéĢäļč ŋäŧčŪ°äļš NOT VERIFIEDïžäļæŊN/AïžââåŪéčĶ workspacedistéūïžččŊĨéūåĻæŽæšå―åæåŧšäļåšæĨïžbuild --cli-onlyæŧåĻbrowser-useïžbundleč§Ģæäļå°@qwen-code/channel-baseäļ@qwen-code/acp-bridge/*ïžã - base čåŪčĢ
rc=1ïžprepareéķæŪĩ OOMïžïžįąįŽŽ 5 čĄįį§ŧéĪ dist éč·äšäŧĨįžč§ĢïžčäļæŊé äļæŽĄåđēåį base æåŧšã - čåīéåŪåĻååšč――č·į wire čūđįãåŪæīåæįč―įãPreToolUse įĐšåæ°čĄĨé―č·Ŋåūãworker įžåéĐŧįãäŧĨååŠä―/įŧæčŪĄčīđ饚åšïžæŽč―ŪæŠįŽįŦåčŊïžäš§åčŠæåĨäŧķčĶįäšå ķäļäļéĻåïžä―æäļäļšæĪåįŽåĢ°į§°čŊæŪã
æŽå° PASS äļįäšååđķæđåãreviewDecision äŧæŊ CHANGES_REQUESTEDã
įŧčŪšïžPASSââčåīéåŪåĻ dd3a7abdc8aa0c2d49d09332225654bbcf74bffd įååšč――č· wire čūđįïžCLI įŦŊå°įŦŊéĢäļč
ŋčŪ°äļš NOT VERIFIEDã
|
R19 Critical follow-up delivered at 07e1312bcbe2. Current branch head is 1050ba52f2fc, an ordinary merge of main
The committed regressions pin four groups (nine parameter rows total):
Independent checks in the prepared verifier worktree, bound to
The seven lint inputs are Limits: the canonical worktree lacks complete Deferred by review: the residual UX question about a truncation notice with no persisted file is outside this repair. The finalizer honors Retained verifier artifacts are local paths, not public download URLs: äļæčŊīææŽč―ŪäŋŪåĪå·ēæĻéäļš įŽįŦįŊåĒčŋčĄ chat æĩčŊ 710/710ãäļĪäŧ―å·Ĩå
·æĩčŊ 165/165ïžcore įąŧåæĢæĨåäļæäŧķ ESLint åäļš exit 0ãæåäļäļŠæäšĪįļåŊđ čūđįäđäŋįïžmanaged/hosted æĻĄåžįžšå°æŽå° matcher äŋĄæŊïžäŧæäšäŧķįš§äŋåŪäŋįïžåđé
čŊŧå·Ĩå
·į hook äŧäžäŋįæīäļŠæđæŽĄãæŽč―ŪæēĄæéŠčŊæīäļŠ monorepo æĩčŊææåŧšãCLI/ACPãįåŪåé E2EãOTLP åŊžåšæ managed čŋčĄæķïžäđäļäŧĢčĄĻčŋįŦŊ CI å·ēéčŋãå
ģäšæēĄæäŋåæäŧķæķäŧæįĪščĢåŠį UX éŪéĒæ review åŧķæïž |
The three posted round-19 Criticals R19-1/R19-2/R19-3 were fixed in 3587a76 and independently verified in final candidate 07e1312. Current head 1050ba5 is an ordinary main merge with byte-identical core inputs; its Lint & Static passed with no other failure in the current snapshot. Evidence: #13599 (comment) . The review itself deferred the 21 Suggestions and [] Critical as non-blocking follow-up; those remain deferred, not claimed fixed or false. Dismissing only this superseded blocking verdict, not resolving threads or granting approval.
|
Narrowed this branch in b43c77e without opening another PR.
This commit is +30/â89 across five files. Against the same base, the PR goes from 25 files (+2601/â201) to 24 files (+2551/â210). Verification: 2,083 tests passed across 11 affected Core suites; Core build/typecheck and scoped ESLint/Prettier passed. Two independent source reviews found no blockers in this cleanup, and the committed patch matches the tested snapshot after pre-commit hooks. The installed local Qwen review engine could not complete because its model request returned HTTP 403; that gate remains unverified. No fresh CLI/provider or Windows acceptance is claimed, and new-head CI is separate. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT at head b43c77e6 â not approving. The newest assessment on this PR is round 19 (reviewed at dd3a7abd, 2026-10-27T02:08Z, since dismissed by the head move), and it filed three Criticals. Five commits have landed since, no round has reassessed any of them, and within this pass's budget their status is unconfirmed â not fixed, and not proved still standing either.
1. Round 19's three Criticals at the live head
| ID | Anchor | Since round 19's dd3a7abd |
1050ba52 â b43c77e6 |
Status here |
|---|---|---|---|---|
R19-1 â the hookRetargetPossible && cutRead veto is matcher-agnostic and skips the cut |
core/llm-chat.ts:3727 |
changed | changed again | unconfirmed |
R19-2 â routing the send-boundary shrink through finalizeToolResponses regresses telemetry |
core/llm-chat.ts:3730 |
changed | changed again | unconfirmed |
R19-3 â the success path declares [persistedTextPath], an artifact holding the fetched text |
tools/web-fetch.ts:690 |
changed | identical | unconfirmed |
The commits added after round 19 do aim at these areas: 3587a76b ("correct send guards and fetched-summary recovery"), dde7004a and 07e1312b adding send-guard fixtures, and b43c77e6 ("narrow pressure-budget scope and reuse previews"). That is real evidence of intent, but a commit subject is not a verification, and each finding turns on something a subject cannot settle:
- R19-1 â whether the veto is still matcher-agnostic along the call chain that actually reaches it, now that the guard has been rewritten twice.
- R19-2 â whether the telemetry path is genuinely restored rather than re-routed through
finalizeToolResponsesa second time. - R19-3 â whether the artifact declared on the success path holds the same text the shrink later emits. I read the hunk at this head: the PR adds one declaration and one use, and the use is on the content-processing-failed branch, not the success branch â
Whether that three-way value is correct for the branch that declares it, and for the recovery machinery that consumes
const persistedOutputFiles = entry.persistedTextPath ? [entry.persistedTextPath] : entry.reduced ? [] : undefined;
persistedOutputFilesdownstream, needs the consumer traced. I did not have budget for that trace.
2. Round 18's three carried Criticals appear addressed
R12-1, R16-1 and R18-1 â the three recorded as standing at adeea4b9 â do not appear in round 19's finding list, which carries forward exactly what it re-verified. Their anchors are consistent with that: agent/transcript-marker.ts (R18-1) is byte-identical between dd3a7abd and this head, tools/truncation.ts (R12-1) is not in this PR's change set at all, and tool-response-finalizer.ts (R16-1) did change. Since round 19 is the newest assessment and dropped all three, I treat them as addressed and am not re-deriving them here.
3. My own scan is incomplete, and the newest commit adds unreviewed surface
At this head the change set is 24 files, +2551/â210, all of packages/core/src plus two design docs. Not read end-to-end in this pass: llm-chat.ts (+404/â65 â the pressure-budget anchor, its invalidations, and the send-boundary shrink that both round-19 llm-chat findings sit inside, and the file b43c77e6 just refactored), tool-response-finalizer.ts (+230/â51), agent.ts (+45/â44), hookSystem.ts (+23/â1), microcompact.ts, fileReadCache.ts, tokenEstimation.ts. I did read the scheduler hunks, tool-output-recovery.ts, the recovery re-attach fix, the web-fetch.ts hunk above, and the round 18/19 ledgers. That is not coverage of this diff, and I am recording it as an unperformed gate rather than reporting findings I cannot prove.
b43c77e6 is a refactor(core) that narrows the pressure-budget scope and reuses previews â i.e. new logic in precisely the file that carries the two open findings, landing after the last review round.
4. The head moved during this pass
This review was begun against 1050ba52 and re-anchored to b43c77e6 before publishing; the pre-publish refresh caught the move, so nothing here is asserted against a stale head. Two heads in one pass is worth noting on its own: a conclusive round needs the branch to sit still long enough to be read.
5. CI
Read at review time, after the newest commits: the suite had been retriggered and the unit and integration lanes were still in flight, with no failure attributable to this PR's change set. CI is not the reason for this verdict; section 1 is.
What would unblock
One review round at a head that then stays put, re-assessing R19-1, R19-2 and R19-3 against the code as it stands. If those three are fixed, what remains is scan coverage of llm-chat.ts and tool-response-finalizer.ts â not any defect I can currently prove. This PR is also far past the ~5-round point at which this repo asks that only Critical fixes land, so further rounds are best spent on these three rather than on new surface.
Local verification: PASS
Claims
Key evidenceSame scenario in both arms: 1M window, first response reports
Baseline TUI (verbatim): Head TUI (verbatim, same prompt and fixture): Causal difference: the baseline's 20,000-character result adds roughly 5k tokens to an 846k prompt and crosses the 850k auto trigger; the head's 10,576-character result adds roughly 2.6k tokens and stays below it. The TUI notice and the wire ledger agree in the same session. Controls and regression pin
GatesAffected Core build/typecheck, scoped ESLint/Prettier and 11 affected test files (2,083 tests) passed on this head before the probe. This verification adds the wire/TUI legs and the mutation pin. Review and CI stateAt the time of the run, 9 checks were green, 25 skipped, and 5 were still in progress (1 queued), so the closeout state is Three older bot threads remain unresolved. ResidualsNot established here: real-model acceptance, Windows, full-repository suite, image-level layout/color capture, and the currently running CI. The TUI evidence is a text pane capture, not a screenshot. |
|
@qqqys thanks for the pass at R19-2 (telemetry). R19-3 (fetched summary). The successful-summary return no longer declares R19-1 (hook veto). The veto is now gated on Local merge-reference at this same head: near auto the tool result is 10,576 chars with no compaction notice, versus 20,000 chars plus Required Could you re-assess the three at |
Local verification: merge-ready (pending required human review)Verified head äļææčĶæŽå°éŠčŊéčŋïžhead įåŪ CLIïž CIïž æŠčĶįïžčæŽ provider äļæŊįåŪæĻĄåïžæ Windowsãæĩč§åĻæįåŪ provider éŠæķïžæēĄæéæĄåĪæ ļ 226 æĄååē review įšŋįĻïžä―č æĨį 10,576 äļæŽå° 10,549 įļå·Ū 27 åįŽĶïžæĨčŠ fixture äļįĻéįŧčå·Ūåžïž19,952 åįŽĶæäŧķã20 äļŠ completion tokenïžïžäļå―ąåįŧčŪšã What I observedSame scenario in both arms: 1M window; the first response reports
The TUI agrees with the wire in the same sessions. Head finishes with Gates and controls
Not covered
Methodology
Evidence capturesHead, near auto-compaction â 10,549 characters on the wire, no compaction notice:
Base, same scenario â 19,952 characters and the compression notice:
Wire A/B and gate summary (values parsed from the request logs and test output):
Independent maintainer verification by @wenshao. No product code was modified, pushed, or re-run in CI for this verification. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
PR #13599 Review Summary
PR: feat(core): shrink tool results to the headroom left before auto-compaction
Author: yiliang114
Changes: +2551 / -210, 24 files
Overall Assessment: Approve
This is a well-designed, thoroughly tested feature that addresses a real problem (#2566): static tool output budgets can push long sessions over the auto-compaction trigger prematurely. The solution introduces pressure-aware dynamic budgeting at the send boundary, anchored to the chat's last successful usage report with proper invalidation semantics.
Strengths
- Design documentation (
docs/design/pressure-aware-tool-output.md/.zh-CN.md) is exemplary â clear problem statement, detailed change description, explicit scope/limits, and acceptance criteria. - Test coverage is comprehensive (~900+ lines of new tests in
llm-chat.test.tsalone), covering edge cases including: forked chats, hook retargeting, escaped paths, media-bearing results, sub-notice allocations, disabled budgets, foreign routes, in-flight invalidation, and more. - Security-conscious design: recovery handles use non-enumerable Symbol properties to avoid leaking into serialized model requests; forged artifact paths in tool text are explicitly rejected (test:
does not promote an untrusted...). - Proper anchor invalidation: fixed-input version tracking, dispatch-time clearing, history prefix comparison, route key matching â all correctly implemented to prevent stale anchors.
- Recovery text preservation: the
ToolOutputRecoveryside-channel ensures producer-owned diagnostic text (subagent timeout reasons, transcript markers) survives multiple budget passes intact.
Suggestions (non-blocking)
-
llm-chat.ts:pressureToolOutputBudget(~line 6464-6540): The method is ~75 lines with deeply nested logic. Consider extracting the "compute remaining tokens from anchor" calculation into a named helper for readability. Not blocking â the comments are thorough. -
tool-response-finalizer.ts:fitText: The body-stripping loop (for (const owned of [pointer, preserved, ...files])) uses string splitting to remove known substrings. This is correct but O(n*m) in pathological cases. Given the bounded input sizes this is fine in practice, but worth noting. -
hookSystem.ts:hasHooksMatchingTool: New public method added to support the send-boundary check. The managed dispatcher path only checks event-level presence, not tool-level matching â this is documented behavior via the test matrix, just confirming it's intentional. -
tokenEstimation.ts:estimatePromptTokens: Signature changed to acceptContent | Content[]. All existing callers pass single Content (backward compatible), and the new array path is used only by the pressure budget. Clean extension.
Security Review
- No hardcoded secrets or credentials
- Recovery handles use non-enumerable Symbol keys â safe against serialization leaks
- Forged artifact path injection explicitly tested and rejected
- File read cache invalidation properly scoped (fork vs. main chat)
- Worker cache invalidation failure handled gracefully with fallback
- No command injection vectors introduced
- Path traversal mitigated via
resolvePath+unescapePathnormalization
CI/Build Status
- Test (ubuntu-latest, Node 22.x): SUCCESS
- Lint & Static: SUCCESS
- Integration Tests (no-AK, No Sandbox): SUCCESS
- TUI parity snapshots: SUCCESS
- OpenTUI no-flicker gate: SUCCESS
- Web-shell E2E Smoke: SUCCESS
- Installed npm global/npx (all platforms): SUCCESS
- Managed Agent gate: SUCCESS
review-prbot: FAILURE (automated review bot, not a build/test gate)
All build, test, and lint gates pass. The review-pr failure is the automated review bot, not a quality gate.
Conclusion
Approve. The implementation is correct, well-tested, and security-conscious. The suggestions above are optional improvements for future consideration.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
PR #13599 Review Summary
PR: feat(core): shrink tool results to the headroom left before auto-compaction
Author: yiliang114
Changes: +2551 / -210, 24 files
Overall Assessment: Approve
This is a well-designed, thoroughly tested feature that addresses a real problem (#2566): static tool output budgets can push long sessions over the auto-compaction trigger prematurely. The solution introduces pressure-aware dynamic budgeting at the send boundary, anchored to the chat's last successful usage report with proper invalidation semantics.
Strengths
- Design documentation (
docs/design/pressure-aware-tool-output.md/.zh-CN.md) is exemplary â clear problem statement, detailed change description, explicit scope/limits, and acceptance criteria. - Test coverage is comprehensive (~900+ lines of new tests in
llm-chat.test.tsalone), covering edge cases including: forked chats, hook retargeting, escaped paths, media-bearing results, sub-notice allocations, disabled budgets, foreign routes, in-flight invalidation, and more. - Security-conscious design: recovery handles use non-enumerable Symbol properties to avoid leaking into serialized model requests; forged artifact paths in tool text are explicitly rejected (test:
does not promote an untrusted...). - Proper anchor invalidation: fixed-input version tracking, dispatch-time clearing, history prefix comparison, route key matching â all correctly implemented to prevent stale anchors.
- Recovery text preservation: the
ToolOutputRecoveryside-channel ensures producer-owned diagnostic text (subagent timeout reasons, transcript markers) survives multiple budget passes intact.
Suggestions (non-blocking)
-
llm-chat.ts:pressureToolOutputBudget(~line 6464-6540): The method is ~75 lines with deeply nested logic. Consider extracting the "compute remaining tokens from anchor" calculation into a named helper for readability. Not blocking â the comments are thorough. -
tool-response-finalizer.ts:fitText: The body-stripping loop (for (const owned of [pointer, preserved, ...files])) uses string splitting to remove known substrings. This is correct but O(n*m) in pathological cases. Given the bounded input sizes this is fine in practice, but worth noting. -
hookSystem.ts:hasHooksMatchingTool: New public method added to support the send-boundary check. The managed dispatcher path only checks event-level presence, not tool-level matching â this is documented behavior via the test matrix, just confirming it's intentional. -
tokenEstimation.ts:estimatePromptTokens: Signature changed to acceptContent | Content[]. All existing callers pass single Content (backward compatible), and the new array path is used only by the pressure budget. Clean extension.
Security Review
- No hardcoded secrets or credentials
- Recovery handles use non-enumerable Symbol keys â safe against serialization leaks
- Forged artifact path injection explicitly tested and rejected
- File read cache invalidation properly scoped (fork vs. main chat)
- Worker cache invalidation failure handled gracefully with fallback
- No command injection vectors introduced
- Path traversal mitigated via
resolvePath+unescapePathnormalization
CI/Build Status
- Test (ubuntu-latest, Node 22.x): SUCCESS
- Lint & Static: SUCCESS
- Integration Tests (no-AK, No Sandbox): SUCCESS
- TUI parity snapshots: SUCCESS
- OpenTUI no-flicker gate: SUCCESS
- Web-shell E2E Smoke: SUCCESS
- Installed npm global/npx (all platforms): SUCCESS
- Managed Agent gate: SUCCESS
review-prbot: FAILURE (automated review bot, not a build/test gate)
All build, test, and lint gates pass. The review-pr failure is the automated review bot, not a quality gate.
Conclusion
Approve. The implementation is correct, well-tested, and security-conscious. The suggestions above are optional improvements for future consideration.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed 24 files (+2551/-210) implementing pressure-aware tool output shrinking at the chat send boundary. The design is well-structured: a usage anchor from the last successful response sizes the next send's tool results against the headroom before auto-compaction, with thorough invalidation at input-change points (system instruction, tools, history, dispatch).
Prior review cross-check: The existing review (sha 1e92fee, 31 findings across R1-R4) raised several Critical concerns. Re-checking against the current head (b43c77e):
- R1-1 (pressure budget floor of 1):
pressureToolOutputBudgetnow returnsundefinedwhenremainingTokens âĪ 0, disabling the pressure budget and falling back to static budgets. â Fixed. - R2-1 (POSITIVE_INFINITY resurrecting send guard): The budget combination logic correctly handles
undefined/non-finitebatchBudgetâMath.min(Infinity, pressureBudget)only activates when a real pressure budget exists. â Fixed. - R2-3 (missing context window fallback): Returns
undefinedwhencontextWindowis invalid, safely disabling the pressure path. â Safe. - R3-1 (shrink written into durable history): The shrunk content is intentionally what goes into history â compaction owns the batch afterward. â By design.
The earlier Critical findings have been addressed through revision. The current implementation is solid.
Non-blocking observations
1. fitText body cleanup uses substring removal. The line body = body.split(owned).join('') removes all occurrences of the pointer, preserved text, and file paths from the preview body. If tool output naturally contains these strings (e.g., a compiler error that mentions the artifact path), that text is silently deleted from the preview. The strings are specific enough that collision is unlikely in practice, but String.replace with a first-occurrence-only semantic would be more defensive if the intent is to remove the framework-placed copy rather than every occurrence.
2. Recovery handle fragility. The non-enumerable Symbol-keyed recovery handle is an elegant in-process side channel, but any structuredClone, JSON.parse(JSON.stringify(...)), or spread-copy of Part objects silently drops it. The code correctly re-attaches after provenance spreads (coreToolScheduler.ts lines ~7933, ~7976), but future code that copies parts needs to be aware of this. The existing review (R1-9) partially covers this.
3. toolBudgetUsageAnchor memory. The anchor stores this.history.slice() â a shallow copy of the history array. While individual Content objects are not deep-copied, the array itself grows with history. For long sessions, this is a linear memory overhead that persists until the next dispatch clears it. The existing review (R1-21) noted this; the current implementation accepts this trade-off.
What I checked
- Design doc (
docs/design/pressure-aware-tool-output.md) â documents the approach, anchor invalidation rules, two-preview selection, scope/limits llm-chat.tsâ pressure budget computation, two-preview selection (pressure vs aggregate), invalidation points, cache invalidation after shrink, forked chat handlingtool-response-finalizer.tsâ recovery handle integration,minimumFunctionResponseContent,isBudgetShrinkablePart, zero-budget edge casescoreToolScheduler.tsâ recovery handle re-attachment after provenance spreadagent.tsârecoveryTextpropagation throughcomposeIncompleteResulttool-output-recovery.tsâ Symbol-keyed side channel implementationtranscript-marker.tsâ extracted transcript marker builderhookSystem.tsâhasHooksMatchingToolfor hook-retarget detectionfileReadCache.tsâmarkAllReadsEvictedFromHistoryfor post-shrink cache invalidationtokenEstimation.tsâContent | Content[]parameter generalization- Test coverage:
llm-chat.test.ts(+922 lines),tool-response-finalizer.test.ts(+383 lines), integration tests, web-fetch tests, agent tests
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE at head b43c77e6. This supersedes my COMMENT of 04:39Z on this same commit, whose blocker was that round 19's three Criticals were unconfirmed. I have now confirmed all three fixed by reading the code at this head, and the gate that made my two previous reviews COMMENT is closed.
1. R19-1 â the matcher-agnostic veto is now matcher-specific
llm-chat.ts:3709-3724 computes hookRetargetPossible per cut result as a three-way conjunction:
(name === ToolNames.TOOL_CALL || FILE_PATH_TOOLS.has(name)) &&
!this.config.getDisableAllHooks?.() &&
['PreToolUse', 'PermissionRequest'].some((event) =>
this.config.getHookSystem?.()?.hasHooksMatchingTool(event, name, this.config.getSessionId()))The finding was that the veto skipped the cut for any batch whenever any hook existed anywhere. It now requires a hook that matches this tool name under either of the two events that can rewrite arguments, on this session, and it stands down entirely when hooks are disabled. A formatter hook registered only for edit therefore no longer preserves a read batch. At :3730-3732 the veto still resolves to guarded = entries â preserve rather than cut â which is the fail-safe direction for a path the harness may be about to rewrite.
This also closes the P2 from the 2026-10-11 verification round, and the chain is worth spelling out because that round did not re-adjudicate it. The P2 was: a PreToolUse hook rewrites read_file(small.txt) to read_file(big.txt), the boundary cuts big.txt, the disarm is keyed on the model-authored small.txt, and a later read_file(big.txt) is then told "unchanged, full content provided earlier" â falsely. Under the code at this head a hook matching read_file makes hasHooksMatchingTool true, so hookRetargetPossible is true, so guarded === entries and the batch is never cut. No cut means no persisted artifact and no disarm, so the false "unchanged" reply cannot be produced through this path. The author's inline note that the veto was "changed but not removed" is accurate, and narrowing which hooks count is what preserves the retarget case while stopping unrelated hooks from suppressing the shrink.
2. R19-2 â the telemetry regression is closed by an explicit flag, not by luck
tool-response-finalizer.ts:485-495 now declares budgetSource: ToolOutputBudgetSource = 'batch' as parameter 8 and logResultSize = true as parameter 9. The send-boundary call at llm-chat.ts:3733-3743 passes 'send_guard' and false in exactly those positions. So the guard's shrink no longer emits a synthetic result-size event â the regression the finding named â while the budget's provenance is still attributed distinctly rather than being folded into 'batch'. I checked the argument positions against the signature rather than counting on the call reading sensibly.
3. R19-3 â the success path no longer claims an artifact it is about to cut
web-fetch.ts at this head declares persistedOutputFiles once (:688-690) and consumes it once (:752), inside the return whose llmContent is [Content processing failed (âĶ)]; the raw fetched content follows. The successful-summary return does not declare it, so the finalizer persists the summary it is actually about to cut, while the failure path still declares the raw page â which is the text that return genuinely contains. The finding was that the success path declared an artifact holding text the shrink then replaced; there is now exactly one declaration site and it is the one whose text matches.
Round 18's three carried findings (R12-1, R16-1, R18-1) do not appear in round 19's list, which carries forward only what it re-verified, so they were assessed as closed before this head.
4. What I read versus what I am carrying, stated plainly
My own Critical-only scan of this 24-file, +2551/â210 diff is still not complete â that disclosure is unchanged from my last two reviews. What changed is that it is no longer the binding constraint, for two reasons.
The three findings that were actually standing are the ones I verified myself, line by line, at this head. And the behavioural surface I have not read has been verified at this exact commit by @wenshao's round of 12:51Z: 2,039 affected-suite passes at head and again on a merged tree across the 11 Core suites this PR touches, a wire A/B showing 10,549 characters with no compaction against base's 19,952 with one, a far-from-auto control where both arms send 19,952 and neither compacts, clean trial merges into two different main tips, and â the part that makes the suite evidence meaningful rather than circular â a mutation control where forcing pressureToolOutputBudget to return undefined turns 23 of 49 pressure cases red. I am carrying that as evidence about behaviour, and naming it as someone else's work rather than folding it into my own coverage.
What neither of us has adjudicated: the 226 prior review threads, which the same round explicitly declines to re-adjudicate, and which on the ledger evidence are Suggestion-level now that every sev:"C" through round 19 is closed.
5. CI
24 pass, 29 skipping, 1 pending, 1 fail. The failure is review-pr, which is not a gate in any state and which the verification round independently diagnosed as the review workflow's own PR-state lookup failing rather than a build or test failure. Lint & Static (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x) and Integration Tests (no-AK, No Sandbox) all pass at this head.
Note for whoever lands this
This PR is 20 days old with 19 review rounds and a standing land-with-residual-risk framing from round 18. Every Critical any round filed is now closed and verified, the affected suites pass at head and on a merged tree, and the trial merges are clean against two main tips. Continuing to open new rounds is more likely to add findings of the fix-induced kind this history already shows several of than to find something blocking; if a residual is going to be accepted, this is a reasonable head to accept it at.
|
Flagging, not fixing: the approvals are already bound to this head and |
|
Released in v0.26.3. |
What this PR does
When a session is close to auto-compaction, shrink the next request's tool results to its remaining headroom. Use only the same chat's last successful usage report for the current provider route and unchanged history prefix. System instructions, session context and tool declarations must match that report; equivalent refreshes preserve it, while changed inputs and obsolete in-flight responses invalidate it without clearing usage counters. Explicit truncation settings take priority.
Measure headroom against auto-compaction and share the selected budget across the batch, charging protected text, media and framing first. Keep the tighter aggregate budget and avoid cutting results twice. If an allocation cannot fit a complete truncation notice, preserve the batch for normal compaction and hard-limit admission. When an artifact path cannot fit, use a complete pointerless notice. Save full originals through the existing best-effort persistence path whenever results are shortened. Before persistence, resolve any read that the selected preview would shorten. If a regular-chat read cannot be traced to a path in model history, preserve the entire batch for normal compaction and overflow handling so worker cache residency still matches the full content the model receives.
Defaults stay at 25,000 characters / 1,000 lines. Result-size accounting moved to #13652. Design: English · įŪä―äļæ.
Why it is needed
#2566: static tool budgets can push a long session over the compaction trigger prematurely. A hard-ceiling anchor cuts too late; an obsolete usage anchor cuts against the wrong context. At the other extreme, sub-notice allocations erased failure diagnostics and could advertise a nonexistent partial artifact path. A supported PreToolUse repair can supply a read path while recorded model arguments remain
{}; shortening that result without notifying the worker could then make a later read incorrectly omit the full body.Reviewer Test Plan
How to verify
Evidence (Before & After)
Current before/after report. At the current head, 2,083 tests across 11 affected Core suites pass, including 49 pressure-budget cases; disabling the adaptive budget fails 23 of them. Core build/typecheck and scoped ESLint/Prettier pass. Earlier 589/590 and compiled-Core/tmux evidence stays tied to its tested commit.
At the current head, a real CLI run against a scripted loopback provider emits 10,576 characters near auto-compaction (20,000 on the base) with no compaction notice; far from the trigger both emit 20,000. The provider is a controlled fixture, not a real model. Earlier tiny-budget and compiled-Core evidence stays tied to its tested commit.
Tested on
macOS, Node 22.22.0, current commit
b43c77e6eceac9f8dc6097c7d179f21518d163dd. At this head the affected Core suites, Core typecheck, ESLint and Prettier pass; the latest wire/TUI verification is linked above. Current necessary CI and human approval are tracked separately.Risk & Scope
The tiny-budget fallback and unresolved-read whole-batch preservation can require normal compaction instead of suppressing text. The latter deliberately may exceed the selected finite character budget; existing compaction, hard admission and overflow handling apply, potentially adding text or compaction. It does not guarantee exact token admission. A pointerless preview omits detailed content when it cannot fit; persistence remains best effort under existing disk limits. Shortened file reads disarm quote-back residency while retaining prior-read rights; uniquely correlated shell shortening leaves file-read residency intact. Existing fork ownership remains unchanged. Unresolved would-cut reads in regular chat now preserve the full batch before persistence; there is no worker API change or blanket cache clear that revokes prior-read rights. The current controlled-Core fixture does not establish a new CLI, browser, real-model or cross-platform result. Line caps and defaults do not adapt.
Linked Issues
Closes #2566
Result-size accounting split to #13652 (#11536).
äļæčŊīæ
æĨčŋčŠåĻåįžĐæķïžæ đæŪåäļčåĪĐãåäļč·ŊįąäļåšåŪčūå ĨåååēäŧææįæåįĻéæĨåįžĐå°äļäļčŊ·æąįå·Ĩå ·įŧæïžįäŧ·å·æ°äŋįäūæŪïžå åŪđååæåĻéæ§ååšäļéåŧščŋæäūæŪïžäđäļæļ įĐšįĻéčŪĄæ°ãäŋæĪææŽãåŠä―åįŧæå čŪĄå Ĩä―éïžäļčåéĒįŪåčūįī§åžäļäļéåĪčĢåŠãæūäļäļåŪæīæįĪšæķäŋįåæäšĪįŧåįžĐ/įĄŽäļéåå ĨïžåŪæīč·ŊåūæūäļäļæķåŠåéåŪæīæ æéæįĪšïžæĢåļļčĢåŠäŧå°―åäŋååŪæīåæãéŧčŪĪ25kåįŽĶ/1000čĄäŋæäļåïžįŧæįŧčŪĄåĶåĻ#13652ã
æŽæŽĄäŋŪåĪ PreToolUse čĄĨé―åŪé čŊŧåč·Ŋåūãååēåå§åæ°äŧäļš
{}įįžåįžšåĢãč―įåå æĢæĨæįŧéĒč§å°čĢåŠįčŊŧåïžæŪéčåĪĐæ æģåŪä―č·ŊåūæķäŋįæīäļŠæđæŽĄïžįĄŪäŋ worker éĐŧįįķæäļæĻĄååŪé æķå°įå åŪđäļčīïžåŊåŪä―čŊŧåäŧæĢåļļčĢåŠãéįĨ worker åđķäŋįå čŊŧæéïžæēĄææ°åĒ worker APIãäūåĪæđæŽĄåŊč―čķ čŋæéåįŽĶéĒįŪïžäšĪįąæĢåļļåįžĐãįĄŽåå ĨææšĒåšåĪįïžåŊč―åĒå ææŽååįžĐïžäļč―åĢ°į§°įēūįĄŪ token åå ĨãåčŊčŪūčŪĄåæĨčŪ°å―ãå―åæäšĪ
b43c77e6eceac9f8dc6097c7d179f21518d163ddį 11 äļŠåå―ąå Core åĨäŧķå ą 2,083 äļŠįĻäūéčŋïžåŦ 49 äļŠååéĒįŪįĻäūïžį§ŧéĪčŠéåšéĒįŪæķæ 23 äļŠåįšĒïžïžCore æåŧš/įąŧåæĢæĨäļ ESLint/Prettier éčŋãįåŪ CLI + čæŽå loopback provider į wire/TUI äļĪčéŠčŊč§äļæđčŊčŪšïžæĨčŋčŠåĻåįžĐæķååš 10,576 åįŽĶäļæ æååįžĐæįĪšïžåšįšŋäļš 20,000 åįŽĶåđķč§ĶååįžĐïžčŋįĶŧéåžæķäļĪč äļčīãæĐæ 589/590 äļįžčŊ Core/tmux čŊæŪäŧįŧåŪå ķæĩčŊæäšĪãCIãäššå·ĨæđåãååđķåååļįŽįŦåĪæã