Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

@qwen-code /triage

Thanks for the PR!

Template looks good ✓

On direction: direct fix for #5101 — sequential tool-result turns growing provider history unbounded is a real P1 problem on the context performance roadmap. The issue is well-scoped: per-result truncation alone can't prevent cumulative growth across many rounds, and full compression arrives too late when the compression request itself is already oversized. This is squarely within qwen-code's core mission of context management.

On approach: the scope feels right. Adding a cumulative size-based trigger alongside the existing time-based one is the natural complement — time-based alone can't fire during continuous tool use (no idle gap). 789+/149- across 12 files is focused for what it does. The helper extraction (refKey, buildClearMap, buildKeepRefs, planSizeBasedClearing) is appropriate when adding a second clearing path through the same function. The "pending content as virtual tail" design handles the ToolResult turn edge case correctly. I don't see a materially simpler path that solves the cumulative growth problem.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

✅ Local verification report — build, focused tests, real-execution harness, and a live --debug session

I verified this PR locally three ways: (1) reproduced the author's build/test/typecheck commands, (2) drove the built size-budget algorithm directly with controlled synthetic history to check every edge case, and (3) ran a real interactive session under tmux with --debug and confirmed the new size-triggered microcompaction actually fires. Posting as a merge reference.

Setup

• Branch codex/fix-5101-tool-result-history-budget @ 3f44646 (fresh git worktree + npm install + npm run build), CLI v0.18.0
• Platform 🐧 Linux / Node v22.22.2 — the PR table marks Linux ⚠ untested, so this covers it.

1) Author's commands reproduced — all green

2) Real-execution harness against the built module — 17/17

I imported the compiled microcompactHistory from dist and fed it controlled histories. Every behavior the PR claims holds:

PASS  1. threshold -1 disables size trigger
PASS  2. legacy minutes:-1 (new field unset) disables size trigger
PASS  3. total (1500) <= threshold (3000) → no clearing
PASS  4a. size trigger fired (triggerReason=size)  — {before:8000, after:3000, thr:3000, cleared:5, kept:2}
PASS  4b–4f. before=8000, after≤3000, cleared OLDEST first, recent 2 preserved, history length kept
PASS  5. error results not counted → no clearing despite 30000 raw chars
PASS  6. already-cleared results not re-counted/cleared
PASS  7. non-compactable tool not counted → no clearing
PASS  8. pending tail counts toward virtual total (before=3500) but is NOT itself cleared; history length unchanged
==== 17 passed, 0 failed ====

This independently confirms: oldest-first clearing, total bounded to the threshold, recent-N protected, and correct skipping of error / already-cleared / non-compactable / pending-tail parts — matching the PR's own new tests (size-compacts old tool results…, counts pending content as a virtual tail…, does not clear protected recent results…, does not size-compact errors or already-cleared…, etc.).

3) Live tmux session with --debug — the size trigger fires for real

Test project with context.clearContextOnIdle.toolResultsTotalCharsThreshold: 4000, toolResultsNumToKeep: 2. Launched qwen --yolo --debug, had the model run seq 1 800 (~3,092 chars/result) several times via run_shell_command, accumulating compactable tool history. The new [TOOL-RESULT MC] path fired (debug log):

[CLIENT] [TOOL-RESULT MC] tool result chars 9603 > 4000, cleared 1 tool result(s) (~801 tokens), now 6402, kept 1 tool result(s)

• ✅ Size-triggered microcompaction fires on the ToolResult pre-send path, with the pending result counted as a virtual tail (9603 > 4000).
• ✅ 0 microcompaction failures in the whole session.
• ✅ The idle ([TIME-BASED MC]) path correctly did not fire (the session was active, not idle) — the two triggers are cleanly separated.

Observations (none are blockers)

1. The threshold is a soft bound, by design. When the most-recent toolResultsNumToKeep results alone exceed the threshold, clearing stops with history still above it (the live log shows now 6402 > 4000, kept 1). This is correct — the protected recent tail is never evicted — but worth a one-line doc note so it isn't mistaken for under-clearing.
2. Size trigger accounts for and clears text tool outputs only (functionResponse.response.output strings); media/inline-data parts are intentionally preserved on the size path (mediaKept = 0) and remain bounded only by the idle path. This matches the PR's "media preservation for size-triggered cleanup" note — flagging it purely as a scope reminder.
3. Default 500000 is conservative and the legacy toolResultsThresholdMinutes: -1 compatibility (disables the size trigger too unless explicitly set) is verified, so existing users see no behavior change.

Verdict

Solid, well-tested fix that does exactly what #5101 needs: a client-side cumulative budget that bounds active tool-result history before the provider request, reusing the existing microcompaction path. Build/typecheck/tests are clean, the algorithm is correct across all edge cases under direct execution, and it fires correctly in a live session. LGTM — recommend merge. The only follow-up I'd suggest is a one-line doc note that the threshold is a soft bound (recent results are always preserved).

==== 17 passed, 0 failed ====

[CLIENT] [TOOL-RESULT MC] tool result chars 9603 > 4000, cleared 1 tool result(s) (~801 tokens), now 6402, kept 1 tool result(s)

Code Review

No critical blockers found. Implementation is clean and well-structured.

Independent proposal: before reading the diff, I'd add a cumulative char-count trigger alongside the existing idle trigger in microcompactHistory, with a new config field toolResultsTotalCharsThreshold, a planSizeBasedClearing helper that collects compactable tool refs, counts chars, and clears oldest-first while protecting recent results. The PR matches this approach exactly, plus the "pending content as virtual tail" design for ToolResult turns.

Correctness: the size-based clearing path correctly filters pending content out of clearRefs (ref.contentIndex >= history.length), skips errors and already-cleared results via getToolOutputChars, and preserves media (size trigger only clears compactable tool outputs). The toolsKept calculation change from Math.min(tool.length, keepRecent) to tool.filter((r) => keepRefs.has(refKey(r))).length is a correctness improvement for the size-trigger case — Math.min overcounts when some tool refs are already-cleared.

Config handling: the legacy compatibility path (toolResultsThresholdMinutes < 0 → disable size trigger when new field unset) is well-thought-out. The getToolResultsTotalCharsThreshold fallback chain is clear. !Number.isFinite(threshold) catches NaN/Infinity.

Trigger separation: the force / idle / size branching in microcompactHistory is clean — force runs full clearing (tool + media + nested-media), idle runs the same full clearing on time-based trigger, size runs tool-only clearing. The sizeOnly flag correctly skips the time-based trigger check.

Client integration: the ToolResult turn in client.ts correctly passes { sizeOnly: true, pendingContent: createUserContent(requestToSend) } to microcompactHistoryBeforeSend, and the debug log format [TOOL-RESULT MC] vs [TIME-BASED MC] makes the two paths easy to distinguish in logs.

Unit Tests

All pass:

• microcompact.test.ts: 44/44 ✓
• config.test.ts: 221/221 ✓
• client.test.ts: 198/198 ✓
• settingsSchema.test.ts: 19/19 ✓
• npm run build: ✓ (0 errors)
• npm run typecheck: ✓ (all packages)

Real-Scenario Testing

Non-UI behavior (internal microcompaction) — no visible TUI difference. Smoke-tested both builds with a tool-using prompt.

Before (installed build)

runner@runnervm1li68:~/work/qwen-code/qwen-code$ qwen -p 'Read package.json and tell me the project name' --max-session-turns 3 -y 2>&1 | tee tmp/triage-test-183537/before.log
Warning: running headless with --yolo / approval-mode=yolo and no sandbox. All tool calls (shell, write, edit) auto-execute at this process's privilege level. Enable a sandbox via --sandbox / QWEN_SANDBOX, or set QWEN_CODE_SUPPRESS_YOLO_WARNING=1 to silence this notice.
The project name is `@qwen-code/qwen-code`.
runner@runnervm1li68:~/work/qwen-code/qwen-code$

After (this PR)

runner@runnervm1li68:~/work/qwen-code/qwen-code$ node /home/runner/work/qwen-code/qwen-code/.qwen/worktrees/triage/dist/cli.js -p 'Read package.json and tell me the project name' --max-session-turns 3 -y 2>&1 | tee tmp/triage-test-183537/after.log
Warning: running headless with --yolo / approval-mode=yolo and no sandbox. All tool calls (shell, write, edit) auto-execute at this process's privilege level. Enable a sandbox via --sandbox / QWEN_SANDBOX, or set QWEN_CODE_SUPPRESS_YOLO_WARNING=1 to silence this notice.
The project name is `@qwen-code/qwen-code` (v0.18.0).
runner@runnervm1li68:~/work/qwen-code/qwen-code$

Both builds start, execute tool calls, and produce correct output. No crashes or regressions observed.

— Qwen Code · qwen3.7-max

Stepping back: this is a clean, focused fix for a real P1 problem. Sequential tool-result turns growing provider history unbounded is the kind of creeping issue that causes pain at scale — full compression arrives too late because the compression request itself is already oversized. The size-based trigger is the natural complement to the time-based one, and the default of 500K chars is a conservative starting point.

The implementation is straightforward — no over-abstraction, no speculative features. The helper extraction is what you'd want when adding a second clearing path through the same function. The "pending content as virtual tail" design handles the ToolResult turn edge case correctly without complicating the common path. The force/idle/size trigger separation is clean and easy to reason about.

Tests are thorough — 7 new focused tests covering the important edge cases (disabled threshold, protected recent results, errors, already-cleared, media preservation, pending content counting, legacy compatibility). All 482 tests pass. Build and typecheck clean.

My independent proposal matched the PR's approach almost exactly. I don't see a materially simpler path that solves the cumulative growth problem. The legacy compatibility path (toolResultsThresholdMinutes < 0 inheriting to disable the size trigger) is a thoughtful touch that avoids surprising existing users.

Verdict: Approve ✅

— Qwen Code · qwen3.7-max

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

@qwen-code /triage

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

✅ Local runtime verification — active tool-result history is bounded on the wire (A/B)

I drove the real CLI through an agentic tool loop and captured the outbound provider payload to confirm this PR clears older compactable tool results once the cumulative-char budget is exceeded — with a before/after A/B that uses the PR's own on/off setting. Tested at PR head aad757e50 on 🐧 Linux, Node v22.22.2.

How I tested

• Built the PR in an isolated worktree (npm install → full workspace tsc build + bundle, exit 0). The fetched head is exactly the 12 PR files (no unrelated commits bundled).
• Ran the real CLI non-interactively (-p … --approval-mode yolo, which routes tool results through SendMessageType.ToolResult → the new size-clear checkpoint in client.ts before each provider request) against a mock OpenAI provider that captures every outbound messages[].
• The mock drives a 2-round run_shell_command loop; each call emits ~22k chars of compactable output (under the 30k shell cap), tagged MARKER_R1 / MARKER_R2.
• A/B is a pure settings flip (no code edit — this is the PR's own switch), with QWEN_MC_KEEP_RECENT=1 so only the 1 most-recent result is protected:
  • ON: context.clearContextOnIdle.toolResultsTotalCharsThreshold = 30000
  • OFF: … = -1 (documented disable)

Result — REQ#3 (the provider request after round 2's tool result)

With the budget active, once R1 + pending R2 exceeded 30000 chars the older result was replaced by the placeholder before the request left the client, while the most-recent result was protected — exactly the cumulative client-side bound #5101 asked for. With the size trigger disabled (-1), both results stay full and history grows unbounded across rounds (the bug). The only variable changed between the two runs is toolResultsTotalCharsThreshold.

Result — PR's automated coverage + build

Notes for review

• The clear happens on ToolResult turns before the provider dispatch (client.ts microcompactHistoryBeforeSend({sizeOnly:true, pendingContent})), and the pending (not-yet-in-history) tool result is counted as a virtual tail toward the budget but is itself protected — I confirmed R2 (the pending tail) was never cleared.
• Recent protection is count-based (toolResultsNumToKeep, default 5; I used 1 for a 2-round demo via QWEN_MC_KEEP_RECENT). It's a soft threshold — protected recents can keep the total above it by design.
• Compactable tools are read_file, run_shell_command, grep, glob, web_fetch, replace, write_file; error / already-cleared / non-string (media) outputs are not counted or cleared on the size path.
• Disable semantics verified as documented: toolResultsTotalCharsThreshold: -1 disables only the size trigger; legacy toolResultsThresholdMinutes: -1 (with the new key unset) disables both idle and size.
• Risk noted in the PR holds: very old tool output beyond the protected window is replaced by the placeholder and no longer visible to the model — acceptable trade for bounding provider history.

Verdict

Confirmed end-to-end on the wire: older compactable tool results are cleared once the cumulative char budget is exceeded, recent results are preserved, and the change is fully gated by the new (and -1-disable) setting. Focused core/cli suites + typecheck + build all green. Resolves #5101. LGTM from a verification standpoint. 👍