Skip to content

fix(core): honor Shell truncation threshold setting - #9014

Merged
yiliang114 merged 6 commits into
QwenLM:mainfrom
cxruan:codex/fix-8922-shell-truncation-threshold
Aug 14, 2026
Merged

fix(core): honor Shell truncation threshold setting#9014
yiliang114 merged 6 commits into
QwenLM:mainfrom
cxruan:codex/fix-8922-shell-truncation-threshold

Conversation

@cxruan

@cxruan cxruan commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This change makes Shell use an explicitly configured tools.truncateToolOutputThreshold at both output-processing boundaries. When the setting is absent, Shell keeps its existing 30,000-character default; explicit lower, higher, and disabled values take precedence.

It also adds regression coverage for configuration provenance and the Shell output path, and updates the design documentation that describes Shell truncation.

Why it's needed

Shell currently hardcodes a 30,000-character threshold, so a 40,000-character result is truncated even when the user configures a 100,000-character threshold. This contradicts the documented setting and unnecessarily replaces model-facing output with a shortened preview and persisted-file reference.

Reviewer Test Plan

How to verify

Configure tools.truncateToolOutputThreshold as 100,000 and tools.truncateToolOutputLines as 100,000, restart Qwen Code, and ask Shell to run node -e "process.stdout.write('x'.repeat(40000))". Confirm that the complete output reaches the model without a truncation marker or persisted-output reference. Then remove the threshold setting and confirm that the same output is still truncated at Shell's existing 30,000-character default. Optionally set the threshold to 10,000 or -1 and confirm that Shell respectively truncates earlier or disables character truncation.

Evidence (Before & After)

Before: Qwen Code 0.21.9 persisted the 40,000-character result to a run_shell_command_*.output file despite an explicit 100,000-character threshold.

After: the current source keeps all 40,000 characters with an explicit 100,000 threshold and creates no artifact. The absent setting still truncates at 30,000; an explicit 10,000 threshold truncates a 20,000-character result; and -1 keeps all 40,000 characters.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS arm64, Node.js v24.14.1. The installed 0.21.9 CLI baseline ran under the macOS seatbelt sandbox; the current source was verified through the real Shell subprocess path plus focused unit and scheduler tests.

Risk & Scope

  • Main risk or tradeoff: An explicitly configured threshold below 30,000 now truncates Shell output earlier, which is the intended consequence of honoring the setting.
  • Not validated / out of scope: Windows and Linux runtime verification; independent aggregate batch and final-response budgets are unchanged.
  • Breaking changes / migration notes: None. Users without the setting retain the existing Shell default.

Linked Issues

Closes #8922

中文说明

本 PR 做了什么

本改动让 Shell 在两层输出处理边界都使用用户显式配置的 tools.truncateToolOutputThreshold。未配置时,Shell 继续保持现有的 30,000 字符默认值;显式配置的更低、更高以及禁用值都会优先生效。

同时补充了配置来源与 Shell 输出路径的回归测试,并更新了描述 Shell 截断行为的设计说明。

为什么需要

Shell 当前硬编码了 30,000 字符阈值,因此即使用户配置为 100,000,40,000 字符的结果仍会被截断。这与设置文档不符,也会不必要地将模型可见输出替换成缩短预览和持久化文件引用。

Reviewer 测试计划

如何验证

tools.truncateToolOutputThresholdtools.truncateToolOutputLines 都配置为 100,000,重启 Qwen Code,然后让 Shell 执行 node -e "process.stdout.write('x'.repeat(40000))"。确认完整输出进入模型上下文,且没有截断标记或持久化输出引用。随后移除阈值配置,确认相同输出仍按 Shell 现有的 30,000 字符默认值截断。也可以将阈值设为 10,000 或 -1,确认 Shell 分别更早截断或禁用字符截断。

证据(修复前后)

修复前:Qwen Code 0.21.9 在显式配置 100,000 字符阈值时,仍会将 40,000 字符结果持久化为 run_shell_command_*.output 文件。

修复后:当前源码在显式配置 100,000 时保留全部 40,000 字符且不创建 artifact。未配置时仍按 30,000 截断;显式配置 10,000 时会截断 20,000 字符结果;配置 -1 时会保留全部 40,000 字符。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS arm64,Node.js v24.14.1。已安装的 0.21.9 CLI 基线在 macOS seatbelt sandbox 下运行;当前源码通过真实 Shell 子进程路径以及定向单测和 scheduler 测试完成验证。

风险与范围

  • 主要风险或取舍:显式配置低于 30,000 的阈值后,Shell 会更早截断;这是遵循该设置的预期结果。
  • 未验证或范围外:Windows 与 Linux 运行时验证;独立的聚合 batch 和最终响应预算保持不变。
  • 破坏性变更或迁移说明:无。未配置该设置的用户继续保留现有 Shell 默认行为。

关联 Issue

Closes #8922

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 12, 2026
@cxruan

cxruan commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

E2E test report

Baseline

Using the globally installed Qwen Code 0.21.9 under the macOS seatbelt sandbox, I configured truncateToolOutputThreshold: 100000 and ran node -e "process.stdout.write('x'.repeat(40000))" through Shell. The CLI still persisted the result to a run_shell_command_*.output file, reproducing #8922.

Current source

I ran the same command through the real ShellToolInvocation subprocess path for each configuration:

Configuration Output Effective Shell limit Truncated Persisted artifacts
Explicit 100k 40k 100k No 0
Setting absent 40k 30k Yes 1
Explicit 10k 20k 10k Yes 1
Explicit -1 40k Infinity No 0

The test harness executed the real child process and cleaned all temporary output afterward.

Automated verification

  • Core Config tests: 521 passed
  • Shell tests: 300 passed
  • Background Shell tests: 3 passed
  • Tool auto-classifier tests: 14 passed
  • Settings schema tests: 40 passed
  • Focused scheduler per-tool budget test: passed
  • Core TypeScript typecheck: passed
  • Targeted ESLint, Prettier, and git diff --check: passed

Limitation

The repository-wide local CLI entry was not used for the after case because this checkout had unrelated stale ignored package build artifacts (@qwen-code/channel-base was missing a newer export). The real subprocess harness covers the changed Shell execution and result-processing path without requiring a full workspace build.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug, not theory. The linked issue (#8922) carries a concrete reproduction — set tools.truncateToolOutputThreshold to 100,000, run a command producing 40,000 characters, and the output is still truncated at Shell's hardcoded 30,000 and spilled to a file. The issue also pinpoints the mechanism (Shell passes a per-call threshold: 30_000 that overrides the configured value) and the origin (#4880). Maintainers already triaged it as type/bug / priority/P2.

Direction: clearly aligned — this is a documented setting that the Shell path silently ignores, so honoring it fixes a real doc-vs-behavior contradiction for users who tuned the threshold. No CHANGELOG reference upstream for this exact knob, but tool-output truncation configurability is actively maintained territory, and the area is squarely relevant.

Size: core paths are touched (packages/core/src/config/, packages/core/src/tools/, packages/cli/src/config/): 22 production lines (shell.ts 13, config.ts 7, settingsSchema.ts 2) vs. 64 test lines vs. 8 docs lines. Well under any escalation threshold.

Approach: the scope feels right. The interesting design point is provenance: the global default for this setting is 25,000 while Shell's own default is 30,000, so simply reading getTruncateToolOutputThreshold() would have silently shrunk the absent-setting behavior from 30k to 25k. Tracking "was it explicitly set" in Config and keeping the 30k Shell default otherwise is the minimal correct shape, and it matches what I'd have proposed independently. Both truncation boundaries (the producer call in the invocation and the maxOutputChars the scheduler reads) are updated together, and -1 → disabled flows through the existing Infinity handling. Docs (user settings, schema description, design doc) are updated in the same PR rather than left to drift.

Risk: elevated — packages/core/src/tools/shell.ts matches a high-revert path pattern from this repo's revert-history analysis. That doesn't block anything, but it means no enrichment is skipped, CI evidence is required before approval, and a sandboxed verification lane is worth naming (done in the review comment).

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,不是理论问题。关联 issue(#8922)提供了具体复现——将 tools.truncateToolOutputThreshold 设为 100,000,执行一个产生 40,000 字符输出的命令,Shell 仍按硬编码的 30,000 截断并落盘。issue 还定位了机制(Shell 传入的 per-call threshold: 30_000 覆盖了配置值)和来源(#4880)。维护者已将其标记为 type/bug / priority/P2

方向:明确对齐——这是一个被 Shell 路径静默忽略的已文档化设置,修复它解决的是真实的"文档与行为不符"问题。上游 CHANGELOG 没有针对该配置项的直接条目,但工具输出截断的可配置性一直是活跃维护的领域。

规模:触及核心路径(packages/core/src/config/packages/core/src/tools/packages/cli/src/config/):生产代码 22 行(shell.ts 13、config.ts 7、settingsSchema.ts 2),测试 64 行,文档 8 行。远低于任何升级阈值。

方案:范围合理。关键设计点是配置来源(provenance):该设置的全局默认值是 25,000,而 Shell 自身默认是 30,000,如果直接读 getTruncateToolOutputThreshold(),未配置时的行为会从 30k 悄悄缩到 25k。在 Config 中记录"是否显式配置"、未配置时保留 Shell 的 30k 默认值,是最小且正确的形态,也与我独立想到的方案一致。两个截断边界(invocation 中的 producer 调用、scheduler 读取的 maxOutputChars)同步更新,-1 → 禁用经由现有 Infinity 处理自然生效。用户文档、schema 描述与设计文档也在同一 PR 内同步更新。

风险:升级——packages/core/src/tools/shell.ts 命中本仓库 revert 历史分析中的高回滚路径模式。这不阻塞任何东西,但意味着不跳过任何审查项、批准前必须有 CI 证据,并且值得指明沙箱验证通道(见审查评论)。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 1bbada0545519241f03fda7cf3feeec99b6ab9ab · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Working from the stated problem, my independent proposal was exactly what this PR does: stop Shell from passing a hardcoded per-call threshold that overrides the configured one — but with one catch the PR handles correctly. The global default for tools.truncateToolOutputThreshold is 25,000 while Shell's own default is 30,000, so naively reading config.getTruncateToolOutputThreshold() would have silently shrunk the absent-setting behavior from 30k to 25k. The fix instead tracks whether the setting was explicitly provided and only then lets it override Shell's default. That's the minimal correct shape, and the diff matches it.

What I verified statically against the code at this commit:

  • Both truncation boundaries move together. The producer call inside the Shell invocation (the threshold: 30_000 literal) and the maxOutputChars getter the scheduler reads as the per-tool budget now share one helper, so the two can't drift apart. previewChars: 4000 and keep: 'both' are untouched, so the preview shape when truncation does fire is unchanged.
  • The provenance flag is sound. The only production site that builds Config params passes the raw setting value through (undefined when absent), so isTruncateToolOutputThresholdExplicit() genuinely distinguishes "not configured" from "explicitly set to the same value as the default". Every other reference I found is a test mock.
  • -1 (disabled) flows through the existing Infinity handling: the truncation helper's fast path returns content untouched at Infinity, and the scheduler's per-tool pass no-ops too. The independent aggregate batch and final-response budgets are untouched, as the PR claims.
  • Tests pin the load-bearing behavior: a 40,000-character result with an explicit 100,000 threshold stays complete — no truncation marker, no persisted-output file — plus the provenance matrix (unset keeps 30k; 25k/10k/100k/-1 all honored) and maxOutputChars exposure to the scheduler, including Infinity.
  • Docs updated in the same PR: user settings table, schema description, the tool-response-budget design doc, and the bundled review skill's design note.
  • One thing the in-flight CI will answer: any test file that builds a partial Config mock for Shell paths now needs the new method. The two that hit it are updated here; if a third exists, the unit suite will surface it.

No correctness, security, or convention issues found.

Testing evidence (this PR's own CI, read via API — no PR code executed here)

The unit suite on the reviewed commit is still in flight (~30 min runtime; not polling — the finalize workflow updates the table below in place once CI settles). No red checks so far. The macOS/Windows unit jobs and the CLI integration job report "skipped" by workflow design — they only run in the merge queue (merge_group), not on PRs — so that state is expected infrastructure behavior, not a gap introduced by this PR.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ⏳ in progress
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Classify PR ✅ success
precheck-pr / precheck ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped (merge queue only)
Test (windows-latest, Node 22.x) ⏭️ skipped (merge queue only)
Integration Tests (CLI, No Sandbox) ⏭️ skipped (merge queue only)

Sandboxed verification would settle this: @qwen-code /verify — that the full settings-file → CLI → Config → Shell chain honors an explicit threshold end-to-end (the 40k-output, 100k-threshold case) is a behavioural claim the mocked unit tests approximate but cannot prove, and the author's before/after evidence is their own claim, verified on macOS only. The author lacks write access, so this would be a sponsored run: a maintainer's @qwen-code /verify comment approves the head it is written against, and that run carries a pre-execution risk screen (npm lifecycle scripts, off-registry dependency resolutions, package-manager config, model screen — all failing closed) plus a full workspace wipe before any PR code executes. Read the resulting report with the same skepticism as the fork's own CI logs: the code under verification is adversarial input, and a crafted PR can shape what the report says even though the sandbox bounds what it can do.

Real-scenario (tmux) testing: not run — this is an unattended CI pass; live behavior is exercised only by the isolated @qwen-code /tmux job if a maintainer triggers it.

中文说明

代码审查:从问题出发,我独立给出的方案与本 PR 一致,而且它正确处理了其中的关键陷阱——该设置的全局默认值是 25,000,Shell 自身默认是 30,000,如果直接读 getTruncateToolOutputThreshold(),未配置时的行为会从 30k 悄悄缩到 25k。PR 改为记录"是否显式配置",仅在显式配置时覆盖 Shell 默认值。静态核对:两个截断边界(invocation 内的 producer 调用、scheduler 读取的 maxOutputChars)共用同一个 helper,不会漂移;previewChars/keep 未动;唯一的 Config 参数生产构造点原样透传设置值(未配置为 undefined),来源标记可靠;-1(禁用)经由现有 Infinity 处理自然生效,聚合 batch 与最终响应预算不变。测试钉住了关键行为:显式 100k 阈值下 40k 输出完整保留、无截断标记、无落盘文件,另有来源矩阵与 scheduler 暴露测试。文档同步更新。未发现正确性、安全或规范问题。唯一待观察项:若有第三处 Shell 路径的 Config 局部 mock 未补新方法,在跑的单测套件会暴露它。

测试证据:来自该 PR 自身 CI 的 API 读取(此处未执行任何 PR 代码)。评审提交上的 ubuntu 单测套件仍在运行(约 30 分钟;不轮询,finalize 工作流会在 CI 结束后原地更新表格)。目前无红色检查。macOS/Windows 单测与 CLI 集成任务按工作流设计只在合并队列(merge_group)运行,PR 上显示 skipped 属预期,不是本 PR 造成的缺口。

沙箱验证建议:@qwen-code /verify——"完整链路(设置文件 → CLI → Config → Shell)遵循显式阈值"是行为性主张,mock 单测只能近似,作者的 before/after 也仅是其自述(且只在 macOS 验证)。作者无写权限,因此这是一次受助跑(sponsored run):维护者的 @qwen-code /verify 评论批准其书写时的 head,该运行在执行任何 PR 代码前有预执行风险筛查(npm 生命周期脚本、非注册表依赖解析、包管理器配置、模型筛查——全部 fail-closed)和完整工作区清理。请以对待 fork CI 日志的同怀疑态度阅读其报告:被验证代码是对抗性输入,精心构造的 PR 可以影响报告_怎么说_,尽管沙箱限定了它_能做什么_。

真实场景(tmux)测试:未执行——这是无人值守 CI 流程;如需真实 TUI 行为验证,可由维护者触发隔离的 @qwen-code /tmux 任务。

Qwen Code · qwen3.8-max

Reviewed at 1bbada0545519241f03fda7cf3feeec99b6ab9ab · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean review with zero findings; withholding the fifth point only because end-to-end proof of the behavior rests on the author's macOS run and mocked unit tests until the named /verify lane or a maintainer's manual pass settles it.

This one earns its merge. The problem is real and maintainer-triaged (a documented setting silently ignored by the Shell path, P2 bug with a concrete reproduction), and the fix is the minimal correct shape — including the non-obvious part: since the setting's global default (25,000) differs from Shell's own default (30,000), honoring the value required tracking whether it was explicitly configured rather than just reading the getter. The diff does exactly that, updates both truncation boundaries through one shared helper so they can't drift, carries focused regression tests for the load-bearing case (40k output survives a 100k threshold, no marker, no spill file), and keeps every doc that described the old behavior in sync. Nothing in the diff is unrelated; if I maintain this in six months I'll thank the author, not curse them.

The author's other contributions here are few and previously merged — this is merit evaluation of one PR, not volume pressure. The shell.ts path is flagged high-revert-risk in this repo's history, which is why the CI evidence is a hard precondition and the sandboxed lane is named above rather than implied.

Approval deferred until CI lands green on 1bbada0545519241f03fda7cf3feeec99b6ab9ab — the ubuntu unit suite is still in flight for this commit. Once it completes green, the finalize workflow posts the commit-pinned approval; if anything lands red or the head moves, it flags instead.

中文说明

这个 PR 配得上合并。问题真实且已被维护者分诊(文档化的设置被 Shell 路径静默忽略,P2 bug,有具体复现);修复是最小且正确的形态——包括不那么显眼的关键点:该设置的全局默认值(25,000)与 Shell 自身默认值(30,000)不同,因此遵循配置值需要记录"是否显式配置",而不是直接读 getter。diff 正是这么做的:通过一个共享 helper 同时更新两个截断边界使其不会漂移,为关键场景(100k 阈值下 40k 输出完整保留、无标记、无落盘)补了聚焦的回归测试,并把所有描述旧行为的文档同步更新。diff 中没有无关改动。作者在本仓库的过往贡献数量不多且已有合并记录——这是对单个 PR 的择优评估。shell.ts 路径在本仓库历史中属高回滚风险,因此 CI 证据是批准的硬性前提,沙箱验证通道也被明确指名而非含糊带过。

批准将推迟到 CI 在该提交上全绿之后——ubuntu 单测套件仍在运行。全绿后由 finalize 工作流发布绑定该提交的批准;若出现红色检查或 head 移动,则改为标记提示。

Qwen Code · qwen3.8-max

Reviewed at 1bbada0545519241f03fda7cf3feeec99b6ab9ab · re-run with @qwen-code /triage

@cxruan
cxruan force-pushed the codex/fix-8922-shell-truncation-threshold branch from 1bbada0 to 096a3e7 Compare August 12, 2026 15:12
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

Preserve Shell's 30k default only when the global threshold is absent, and use an explicit value for both producer and scheduler truncation.
@cxruan
cxruan force-pushed the codex/fix-8922-shell-truncation-threshold branch from 096a3e7 to a9b2b81 Compare August 12, 2026 15:24

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): PR #9014 makes the Shell tool honor an explicitly configu...: none — all checks above completed; the only item left unverified is the existence of a null -stripping validation layer outside settings.ts (noted in the fin…; You are hunting for gaps the base fan-out missed in PR #9...: none — the walk is complete..

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):PR #9014 makes the Shell tool honor an explicitly configu...:none — all checks above completed; the only item left unverified is the existence of a null -stripping validation layer outside settings.ts (noted in the fin…;You are hunting for gaps the base fan-out missed in PR #9...:none — the walk is complete.

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/config/config.ts Outdated
Comment thread packages/core/src/tools/shell.ts
Comment thread packages/core/src/tools/shell.ts Outdated
@QwenLM QwenLM deleted a comment from danialzivehdadr Aug 12, 2026

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): PR #9014 makes the Shell tool honor an explicitly configu...: did not verify whether the /set dialog / settings writer enforces the number type on truncateToolOutputThreshold input (concluded immaterial — the generic….

中文说明

已审查。 建议见行内评论。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):PR #9014 makes the Shell tool honor an explicitly configu...:did not verify whether the /set dialog / settings writer enforces the number type on truncateToolOutputThreshold input (concluded immaterial — the generic…

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/tools/shell.test.ts
Comment thread packages/core/src/tools/shell.ts Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): PR #9014 makes the Shell tool honor an explicitly configu...: none — all checks above completed within budget.; PR #9014 makes the Shell tool honor an explicitly configu...: full packages/core suite run (I ran only the two directly affected shell test files; config.test.ts additions were code-reviewed but not executed).; PR #9014 makes the Shell tool honor an explicitly configu...: none — all planned checks completed within budget..

中文说明

已审查。 建议见行内评论。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):PR #9014 makes the Shell tool honor an explicitly configu...:none — all checks above completed within budget.;PR #9014 makes the Shell tool honor an explicitly configu...:full packages/core suite run (I ran only the two directly affected shell test files; config.test.ts additions were code-reviewed but not executed).;PR #9014 makes the Shell tool honor an explicitly configu...:none — all planned checks completed within budget.

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/config/config.ts

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed.

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): PR #9014 makes the Shell tool honor an explicitly configu...: none — all checks I started were completed within budget..

中文说明

已审查。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):PR #9014 makes the Shell tool honor an explicitly configu...:none — all checks I started were completed within budget.

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

Not explored to full depth (tool budget reached): "You are hunting for gaps in the review of PR #9014, which…": none — all checks above completed within budget.; "PR #9014 makes the Shell tool honor a user-configured…": could not execute coreToolScheduler.test.ts to confirm the promote test reaches the new getter read (no node_modules in the review worktree or parent checkout…; "You are hunting for gaps in the review of PR #9014, which…": none — all checks above completed within budget.; "PR #9014 makes the Shell tool honor a user-configured…": did not execute the test suite** — the review worktree has no node_modules (vitest binary missing); verification above is static (test-vs-implementation mutat….

中文说明

未发现问题。LGTM!✅

未探索到全部深度(达到工具调用预算):"You are hunting for gaps in the review of PR #9014, which…"none — all checks above completed within budget."PR #9014 makes the Shell tool honor a user-configured…"could not execute coreToolScheduler.test.ts to confirm the promote test reaches the new getter read (no node_modules in the review worktree or parent checkout…"You are hunting for gaps in the review of PR #9014, which…"none — all checks above completed within budget."PR #9014 makes the Shell tool honor a user-configured…"did not execute the test suite** — the review worktree has no node_modules (vitest binary missing); verification above is static (test-vs-implementation mutat…

— qwen3.8-max via Qwen Code /review (v0.21.11)

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

Not explored to full depth (tool budget reached): PR #9014 makes the Shell tool honor the configured tools...: 无。`.

中文说明

已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。

未探索到全部深度(达到工具调用预算):PR #9014 makes the Shell tool honor the configured tools...:无。`。

— deepseek-v4-flash via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/tools/shell.ts
water-in-stone pushed a commit to water-in-stone/qwen-code that referenced this pull request Aug 14, 2026
…LM#9086)

* fix(review): harden the pipeline against four live-run failures

Measured on three parallel PR reviews (qwen3.8-max, 2026-08-13, PRs
QwenLM#9013/QwenLM#9014/QwenLM#9045) run via `qwen review run`:

- run.ts: pin the composed-verdict and report scans to the run's own
  target. The generic newest-composed scan captured a concurrent run's
  artifact — two of the three runs republished a neighbour PR's verdict
  (one reported REQUEST_CHANGES for a review whose own report said
  Comment). Also keep re-reading while the child runs: a coverage
  re-check legitimately recomposed a verdict 12 minutes after the first
  write, and the first-snapshot capture would republish the superseded
  one.

- budget.ts: drop placeholder gaps whose completion word carries a
  trailing budget adverbial. Three "none — all checks … completed
  within budget" non-answers reached two posted bodies because the
  completion idiom required the completion word to end the text.

- coverage.ts: label a non-chunk agent by the brief codename found
  anywhere in its launch prompt. Launchers prepend context lines, so the
  first-line label gave twelve finders one shared PR-summary sentence,
  and every budget-gap disclosure rendered as the same truncated PR
  quote instead of a name.

- copy_bundle_assets.js: emit dist/cli.js with a shebang and the execute
  bit. shellContextEnv blanks a QWEN_CODE_CLI a POSIX shell cannot exec,
  so every review subcommand issued from a session launched off the
  bundle silently fell back to the PATH's global install — all three
  runs executed the machine's auto-updated release instead of the tree
  they were launched from.

* test(review): follow the codename label into compose-review's fixtures

The backtick-collapse fixture's first line was itself the brief codename
shape, so the new codename extraction labels it `agent security` and the
first-line assertion no longer holds. Keep the sanitization intent on a
prose-only first line, and pin the codename behaviour — a prepended
context line must not reintroduce the shared-PR-quote label — as its own
case.

* fix(review): classify the run target with the child's own parser

Review feedback on the target pin: prNumberFromTarget re-derived PR
classification with a narrower regex than parse-args — /pull/<n>/files
URLs went unpinned, 0042 pinned pr-0042- while the child writes pr-42-,
and docs/pull/42 pinned a file target as a PR — so a completed (and
posted) review could be reported as one that produced no verdict.
Delegate to parseReviewArgs, whose verdict is what the child names its
artifacts from, and pin the divergent shapes as tests.

Also gate the bundle's shebang/exec-bit block with a package-assets
case (mode asserted off-win32, double-run must not stack shebangs), and
document the accepted same-target residual race on composedPatternFor.

* fix(review): pin run artifacts by exact target identity, share the identity-line parser

Round-2 review feedback, all six findings:

- run.ts: replace name-shape pins with the exact composed filename each
  target class produces (pr-<n> / file basename / the fixed 'local' token,
  per the skill's --out template). The (?!pr-\d+-) lookahead rejected a
  file run's own artifact whenever the reviewed file was named
  pr-<digits>-…, the PR branch's .* wildcard claimed that same artifact,
  and the pooled null class let concurrent file and no-target runs
  cross-capture each other's verdicts. Target classification now comes
  from classifyRunTarget (parse-args' verdict, basename for files).

- run.ts: newestArtifactSince returns {path, mtime}, so the capture poll
  reuses the scan's own stat instead of re-statting the path — the
  scan-vs-sweep window (and its untestable catch branch) is gone
  structurally.

- lib/agent-identity.ts: one parser for the identity line agent-prompt
  bakes into every launch, shared by cost-ledger's row labels and
  coverage's disclosure labels — the two hand-rolled copies could drift,
  and coverage's copy dropped the (round N) and owned-file suffixes,
  folding reverse-audit rounds into indistinguishable disclosure lines.
  cost-ledger still feeds it only the first line (quoted identity lines
  below must never be credited); coverage scans for the first
  line-anchored identity line (launchers prepend context lines).

- lib/budget.ts: one vocabulary for the budget-idiom family — 'below'
  joins the completion tail's position words, and the stayed idiom takes
  the same qualifiers ('stayed inside the tool-call budget').

- run.test.ts: handler-level assertion that the report scan is pinned
  (a strictly newer neighbour report must not become this run's
  reportPath), alongside the pattern-level cases for every shape the
  review probed.

* fix(review): round-3 review polish — CRLF identity lines, producer-side no-gap rule, edge-case pins

- agent-identity: tolerate a trailing CR (CRLF-recorded prompts fed
  through \n-splitting callers failed every parse and fell back to
  first-line prose); scan the launch prompt with one multiline match
  instead of materializing a line array per agent record; pin the
  round-over-file precedence with a both-suffixes test.

- agent-prompt: state the no-gap rule at the producer — write NO
  'Budget gap:' line when nothing was cut short — instead of leaving
  each agent to improvise a 'none' phrasing the consumer-side
  placeholder filter must chase forever.

- run.ts: strip trailing path separators before taking a file target's
  basename (a tab-completed 'src/' pinned 'qwen-review--composed.json',
  which no child artifact carries — fail-closed exit 1 on a completed
  review); pin file-run reports by their filename slot so a file named
  'pr-1234.md' claims its own report; document the two collision
  classes the basename-keyed pin defines (same-basename files,
  basenames spelling 'local'/'pr-<n>').

- budget.test: pin the parenthesis-form exception keep case beside the
  dash form.

* fix(review): round-4 polish — one budget vocabulary, named pin expectation, honest prompt claim

- budget.ts: spell the budget-position vocabulary once (BUDGET_QUALIFIED /
  COMPLETION_TAIL) and build PLACEHOLDER_GAP_RE from it — the literal
  carried three hand-copies that had already drifted twice in two review
  rounds; the space-separated 'tool call' form is pinned in both branches.

- agent-prompt.ts: the no-gap rule now states what actually happens to a
  'none' disclosure — at best filtered, and any unrecognized wording is
  published as a phantom coverage gap — instead of claiming the parser
  treats it as a gap, which was the negation of the filter shipped beside
  it.

- run.ts: derive the composed pin from composedNameFor and name the
  expected filename in both the no-verdict prose and the JSON result
  (expectedComposedName) — a naming drift between the pin and the skill's
  template was undiagnosable once Step 9 swept the near-miss.

- compose-review.ts: publicAgentSubject's provenance note now describes
  the codename labels coverage prefers, with first-line prose as the
  fallback.

* test(review): guard the no-verdict diagnostic and the pinned capture; align the chunk-role grammar

- run.test.ts: assert the no-verdict report names the artifact it waited
  for, in prose and as expectedComposedName — mutation-verified: dropping
  the suffix now fails.

- run.test.ts: force the neighbour's composed artifact strictly NEWER in
  the concurrent-run fixture. With it older, an unpinned newest-composed
  scan landed on the right file anyway and the regression passed;
  mutation-verified: reverting composedPatternFor to the generic scan now
  fails the handler test, not only the pattern units.

- agent-identity.ts: CHUNK_ROLE_RE takes coverage's CHUNK_RE shape
  (whitespace-tolerant, case-insensitive) so a hand-edited 'Chunk 3 of 7'
  cannot resolve as a chunk owner in the posted body and a role agent in
  the ledger row.

* fix(review): keep the bundle's write time across the shebang rewrite; hold the pins to the skill

Round-6 review feedback, all five findings:

- copy_bundle_assets.js: preserve dist/cli.js's atime/mtime across the
  shebang rewrite. stampReviewSourceDigest reads that mtime as the build
  time, so a bumped one certifies a bundle as newer than review sources
  edited before it and the staleness warning the skill's Step 0 stops on
  never fires. A full bundle stamps before reaching here, but a
  standalone run of this script — a flow the gate's own comment
  contemplates — was exposed.

- package-assets.test.js: pin both halves the block owes. The mtime is
  asserted against a fixture built 60s in the past, and the second run
  now arrives at mode 0644 so the exec bit must be re-set — demoting the
  chmod inside the shebang guard previously stayed green.

- run-skill-parity.test.ts: new. composedNameFor and reportPatternFor
  encode the bundled skill's Step 6 --out template and Step 8 report
  stems, and were pinned only against self-referential literals. This
  reads the templates out of SKILL.md and renders them per target class,
  so a skill-side edit fails next to the code that must follow it
  instead of silently in a later review.

- cost-ledger.test.ts: pin the first-line-only invariant — a launch
  whose prepended context sits above the identity line keeps the
  transcript's own id, never a label lifted from below. Consolidating
  both callers on labelFromLaunchPrompt now fails.

- agent-identity.test.ts: assert the two entry points genuinely differ
  on that prompt, so neither caller's policy can be collapsed into the
  other unnoticed.

* fix(review): stop the mtime assertion from pinning libuv's timespec truncation

The assertion compared the recorded mtime against the Date handed to
utimesSync, so it also pinned libuv's double-seconds → timespec
conversion: about half of all millisecond values read back 1 ns low
(X - 0.001), and builtAt is a fresh Date.now() - 60_000 every run — a
~50% coin flip that would have landed intermittent reds on unrelated
PRs through test:ci.

Capture what the filesystem actually recorded after the setup and
compare against that; the invariant under test is only whether the
shebang rewrite moves the stored time. 10/10 green through the CI entry
point, and it keeps its teeth: removing the production
fs.utimesSync(cliEntry, atime, mtime) restore fails it 3/3.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on 120f56f. Verified end to end:

  • Wiring is correct and capture-safe: getShellOutputThreshold(config) reads the Config at invocation time and is shared by both consumers — the producer truncateToolOutput call site (shell.ts execute path) and the scheduler-facing maxOutputChars getter — so the two layers cannot diverge and there is no stale capture.
  • Back-compat verified: with the setting absent, isTruncateToolOutputThresholdExplicit() is false and Shell keeps its 30K trigger + 4K preview exactly as before. The != null flag also treats a runtime null as unset (regression test in config.test.ts), and both the getter default and the call-site default are pinned by execute-path tests.
  • Default-injection risk ruled out: the only production Config producer is packages/cli/src/config/config.ts:2311, which passes the raw settings.tools?.truncateToolOutputThreshold with no ?? default. The schema's default: 25_000 is consumed only by the settings-dialog/config-command UI helper (getEffectiveValue), not the Config build chain — so the Shell 30K default is not silently flipped to 25K for users who never configured the setting. This addresses the open R3-1 / doudouOUC concern; the author's rebuttal checks out.
  • Invalid values are safe: getTruncateToolOutputThreshold() maps <= 0 to Infinity (matches the documented "Set to -1 to disable"), so 0/negative values disable truncation instead of crashing. Units are characters everywhere, consistent with settings.md and the existing char-based Shell trigger; lines: Infinity keeps the line cap from undercutting the char budget as before.
  • Sub-4K thresholds honored: previewChars: Math.min(4000, outputThreshold) closes the earlier gap where a low explicit threshold was undercut by the fixed 4K preview; truncation-marker and persisted-output-file propagation are covered by the new 10K execute-path test.
  • All prior review rounds' findings were addressed with tests (author replies cite 3b6e604, 27d9a62, 64ce61e; the final bot round landed on LGTM).

Nits (non-blocking, P3):

  1. The "producers must not pass a defaulted value" invariant lives only in the config.ts constructor comment. Today there is exactly one producer, but a future Config producer that applies schema defaults would silently move Shell from 30K to 25K. A one-line reminder near cli config.ts:2311 would harden it.
  2. docs/users/configuration/settings.md reconciliation stays out of scope per the author; note the doc already claims this setting applies to Shell, so runtime now matches doc text — only the Shell-default-30K-vs-25K nuance remains undocumented.

CI: 110/110 check runs completed, zero failures. Test (ubuntu-latest, Node 22.x) and web-shell E2E Smoke passed; macOS/Windows matrix and Integration Tests (CLI, No Sandbox) were skipped under the fork CI-approval gate — small residual risk, covered by the added unit tests.

@yiliang114
yiliang114 added this pull request to the merge queue Aug 14, 2026
Merged via the queue into QwenLM:main with commit 0e0f35e Aug 14, 2026
127 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): Shell ignores tools.truncateToolOutputThreshold

4 participants