fix(cli): show file path in compact tool summary for single collapsible tools - #6448
Conversation
…le tools buildToolSummary() previously discarded the description field from collapsible tools (ReadFile, Grep, Glob, ListFiles), showing only generic counts like 'Read 1 file'. Now shows the actual file path or search pattern for single tools, while preserving count format for batches of multiple tools of the same type. Falls back to count format when description is unavailable. Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Thanks for the PR! Template looks good ✓ Problem: Real UX issue — when the CLI reads a single file, the compact summary shows "Read 1 file" with no indication of which file. The Direction: Aligned. Showing "Read src/index.ts" instead of "Read 1 file" is strictly more useful with no downside. This is a straightforward TUI enhancement. Size: 3 files changed, +125/-26. Production logic: 47 lines in Approach: Clean and minimal. The Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实的 UX 问题——当 CLI 读取单个文件时，紧凑摘要显示 "Read 1 file"，无法知道读的是哪个文件。 方向：对齐。显示 "Read src/index.ts" 比 "Read 1 file" 更有用，没有副作用。这是一个简单的 TUI 增强。 规模：3 个文件，+125/-26。生产逻辑： 方案：干净且最小化。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: I'd solve this by making Comparison with PR: The PR's approach matches my proposal exactly —
No correctness bugs, no security issues, no AGENTS.md violations. The Reuse check: TestingUnit tests: All 71 tests pass (ran locally in worktree). New tests added (8):
Real-scenario tmux test: Not feasible in this CI environment — the compact tool summary is rendered by the Ink React TUI in interactive mode only, and requires a live model interaction to trigger tool calls. 中文说明代码审查独立方案： 我会让 与 PR 对比： PR 的方案与我的方案完全一致—— 无正确性 bug、无安全问题、无 AGENTS.md 违规。 复用检查： 测试单元测试： 71 个测试全部通过（在 worktree 本地运行）。 tmux 真实场景测试： 在此 CI 环境中不可行——紧凑工具摘要仅在交互模式下由 Ink React TUI 渲染，需要实际的模型交互来触发工具调用。 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped display improvement. The change does exactly what it says — uses the existing The test suite is strong — 71 tests across both files, with 8 new tests that thoroughly pin the new behavior and the fallback paths. @wenshao's real TUI verification with A/B comparison and mutation testing (reverting source while keeping tests → 12 failures) confirms the tests are non-vacuous and the feature works in practice. My independent proposal matched the PR's approach exactly, which is a good sign — this is the natural, minimal way to solve the problem. No simpler path exists. No concerns. Shipping it. 中文说明这是一个干净、范围明确的显示改进。改动完全符合描述——利用已有的 测试套件扎实——两个文件共 71 个测试，8 个新测试全面锁定新行为和回退路径。@wenshao 的真实 TUI A/B 验证和变异测试（回退源码保留测试 → 12 个失败）确认测试非空转、功能在实践中有效。 我的独立方案与 PR 的方案完全一致——这是解决问题的自然、最小方式。不存在更简单的路径。 无顾虑，可以合入。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] Stale JSDoc — The JSDoc comment above buildToolSummary (line ~195) still documents the old count-based behavior (Single tool → "Read 1 file" / "Ran 1 command"), but the function now produces "Read a.ts" / "Ran ls -la" when a description is available. Suggest updating to:
* Single tool (with description) → "Read a.ts" / "Ran ls -la"
* Single tool (no description) → "Read 1 file" / "Ran 1 command"
* Multi same → "Read 3 files"
* Multi mixed → "Read a.ts, ran npm test, edited b.ts"
|
|
||
| if (count === 1) { | ||
| if (tools.length === 1 && tools[0].description) { | ||
| parts.push(`${v} ${tools[0].description}`); |
There was a problem hiding this comment.
[Suggestion] The description field is interpolated directly here without sanitization. Two concerns:
- In
useReactToolScheduler.ts, when a tool call errors,descriptionis set toJSON.stringify(request.args), producing output likeRead {"file_path":"/some/path"}instead of falling back toRead 1 file. - The in-process path applies no ANSI/control-character stripping (unlike the daemon path which uses
sanitizeDisplayText()), so shell commands or file paths with escape sequences would render verbatim.
Consider sanitizing and filtering:
| parts.push(`${v} ${tools[0].description}`); | |
| const desc = tools[0].description?.replace(/[\x00-\x1f\x7f]/g, '') ?? ''; | |
| const isDisplaySafe = desc.length > 0 && !desc.startsWith('{') && !desc.startsWith('['); | |
| if (isDisplaySafe) { | |
| parts.push(`${v} ${desc}`); | |
| } else if (tools.length === 1) { | |
| parts.push(`${v} 1 ${template.singular}`); | |
| } else { |
— qwen3.7-max via Qwen Code /review
… ANSI
When a tool call errors, useReactToolScheduler sets description to
JSON.stringify(args) which produces '{...}' blobs. Strip ANSI escape
sequences and reject JSON-looking descriptions so the summary falls
back to the count format instead of rendering raw JSON.
Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
@qwen-code /triage |
| const stripped = raw.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, ''); | ||
| // Reject control characters (except tab/newline) | ||
| // eslint-disable-next-line no-control-regex | ||
| const cleaned = stripped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]/g, ''); |
There was a problem hiding this comment.
[Suggestion] The control-character regex deliberately preserves \n (0x0a) and \r (0x0d), but shell tool descriptions can contain literal newlines — e.g., heredocs, multi-line bash -c commands, or commands with embedded \n. These survive sanitization and flow into the summary string rendered by <Text wrap="truncate-end">.
Ink's <Text> breaks on embedded newlines, which would render the compact summary across multiple rows — defeating the "compact" layout with no error logged.
| const cleaned = stripped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]/g, ''); | |
| const cleaned = stripped.replace(/[\x00-\x1f\x7f]/g, ''); |
This removes all C0 control characters including tab/newline/CR. Alternatively, replace them with spaces to preserve readability: .replace(/[\r\n]+/g, ' ').
— qwen3.7-max via Qwen Code /review
|
|
||
| // Strip ANSI escape sequences | ||
| // eslint-disable-next-line no-control-regex | ||
| const stripped = raw.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, ''); |
There was a problem hiding this comment.
[Suggestion] The CSI regex \x1b\[[0-9;]*[a-zA-Z] only matches SGR/CSI sequences. Non-CSI ANSI sequences like \x1b(B (charset selection), \x1b7 (save cursor), and OSC \x1b]... pass through unmatched.
The second-pass control-character regex strips the ESC byte (0x1B falls in \x0e-\x1f), but the residual characters remain as garbled text: \x1b(B → (B, \x1b7 → 7. For typical file paths and commands this is unlikely, but terminal output or unusual tool descriptions could trigger it.
Consider broadening the ANSI strip to cover common non-CSI forms, or stripping all ESC-prefixed sequences:
| const stripped = raw.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, ''); | |
| const stripped = raw.replace(/\x1b\][^\x07]*\x07|\x1b[()][A-Z0-9]|\x1b\[[0-9;]*[a-zA-Z]|\x1b./g, ''); |
— qwen3.7-max via Qwen Code /review
…summary Strip all common ANSI escape sequences (OSC, charset, CSI, single-byte ESC) instead of just CSI. Replace all C0 control characters including newlines with spaces so embedded \n in shell descriptions does not break the single-line compact summary layout. Signed-off-by: Alex <alex.tech.lab@outlook.com>
… summary The buildToolSummary change from 'Read 1 file' to 'Read a.ts' broke 4 assertions in ToolGroupMessage.test.tsx. Update all 5 occurrences to use the new description-based format. Signed-off-by: Alex <alex.tech.lab@outlook.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The implementation is clean and well-scoped — safeDescription() handles all common edge cases (ANSI escapes, JSON error blobs, empty descriptions), tests are thorough at 21+ cases, and the count-format fallback preserves backward compatibility for multi-tool groups.
— qwen3.7-max via Qwen Code /review
✅ Verification report — PR #6448 (real TUI build, BASE↔PR A/B)Verified the built Verdict: LGTM — safe to merge. Behavior is correct, fallbacks hold, truncation is graceful. A few non-blocking notes for the record are at the bottom.
Headline: single collapsible tool now shows the path/patternSame session, same mock tool calls; the only variable is the compiled
Edge cases exercised in the live TUIMulti-category, all collapsible in one turn — descriptions joined in category order (
Unit tests & non-vacuity
Notes (non-blocking — for the record)
🔧 How this was verified (method)
🇨🇳 中文版验证报告（点击展开）✅ 验证报告 — PR #6448（真实 TUI 构建，BASE↔PR A/B 对比）在 真实交互式 TUI（tmux，Linux）中验证了本 PR head（ 结论：LGTM — 可以合并。 行为正确、回退逻辑成立、截断处理优雅。文末列了几条不阻塞合并的备注。
核心：单个可折叠工具现在显示路径/模式同一会话、同一批 mock 工具调用，唯一变量是编译后的 （截图见上方英文版 side-by-side 图）
实际 TUI 中验证的边界情况
单元测试与非空验证
备注（不阻塞合并，仅作记录）
🔧 验证方法
|
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
added
category/ui
Resolve conflicts in three files: - CompactToolGroupDisplay.tsx: combine this branch's i18n count-phrase summaries with main's single-tool description display (QwenLM#6448). Kept the localized {{count}} phrases for the multi-tool / no-description paths and added English verb prefixes (pastVerb/activeVerb) for the single-tool "Read a.ts" case, since the description it precedes is a language-neutral path/command and bare-verb i18n keys would collide with existing entries. - resumeHistoryUtils.ts: keep both import groups (isCollapsibleTool and main's history-gap-notice helpers). - AppContainer.tsx: call main's shouldDrainMessageQueue() guard and keep this branch's extra guard that suppresses queue draining while the transcript is open. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
3 participants
What this PR does
Changes the compact tool group summary to display the actual file path or search pattern when a single collapsible tool (read/search/list) is executed, instead of a generic count like "Read 1 file". When multiple tools of the same type are batched together, the count format is preserved. If the tool description is unavailable, it falls back to the count format.
Why it's needed
Collapsible tools (ReadFile, Grep, Glob, ListFiles) include a
descriptionfield containing the file path or search parameters, butbuildToolSummary()only used the tool name for categorization and discarded this information. Users executing a read operation would see "Read 1 file" with no indication of which file was read, making it harder to follow agent actions in the TUI.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsx— all 21 tests passEvidence (Before & After)
Read 1 fileRead src/a.tsSearched 1 patternSearched 'foo' in path './'Read 3 filesRead 3 files(unchanged)Read 1 file, ran 1 commandRead src/a.ts, ran lsRead 1 fileRead 1 file(unchanged)Tested on
Environment
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsx— 21 tests passedRisk & Scope
ink'swrap="truncate-end"handles overflowLinked Issues
N/A
中文说明
此 PR 做了什么
修改了紧凑工具组摘要的显示逻辑：当执行单个可折叠工具（read/search/list）时，显示实际的文件路径或搜索模式，而不是通用的 "Read 1 file" 计数。当多个同类型工具批量执行时，保留计数格式。如果工具描述不可用，则回退到计数格式。
为什么需要
可折叠工具（ReadFile、Grep、Glob、ListFiles）包含一个
description字段，记录了文件路径或搜索参数，但buildToolSummary()只使用工具名称进行分类计数，丢弃了这些信息。用户执行读取操作时只能看到 "Read 1 file"，无法知道读了哪个文件，难以在 TUI 中跟踪 agent 的操作。评审者测试计划
如何验证
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsx— 21 个测试全部通过证据（前后对比）
Read 1 fileRead src/a.tsSearched 1 patternSearched 'foo' in path './'Read 3 filesRead 3 files（不变）Read 1 file, ran 1 commandRead src/a.ts, ran lsRead 1 fileRead 1 file（不变）测试平台
环境
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsx— 21 个测试通过风险与范围
ink的wrap="truncate-end"会处理溢出关联 Issue
无