fix(cli): wrap long compact tool summaries - #6847
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: Real bug with reproduction. Issue #6814 clearly describes long tool summaries being truncated with Direction: Aligned. Terminal UX improvement for compact tool summaries — keeping full file paths visible rather than losing information to truncation is a clear win for users working with deep directory structures. No CHANGELOG reference, but the area (terminal rendering / tool display) is squarely within scope. Size: Not a core-module change. All 4 files are in Approach: Minimal and focused — one prop change ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：有复现的真实 bug。Issue #6814 明确描述了长工具摘要被 方向：对齐。终端 UX 改进——保留完整文件路径而不是因截断丢失信息，对使用深层目录结构的用户来说是明确的改善。 规模：非核心模块改动。4 个文件均在 方案：最小且聚焦——一个 prop 变更、一个提取的辅助函数、一个高度估算函数来修复父级布局计算，以及回归测试。无范围蔓延。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: change The PR matches this exactly, plus adds a proper Reuse check: Correctness: The height estimator subtracts fixed-width elements (padding, status indicator, elapsed-time label) from Conventions: Clean extraction of No blockers found. Real-Scenario TestingRan the bundled PR build in a 74-column tmux TUI with Before (installed qwen 0.19.10)Truncated with After (PR build, v0.19.9)Wraps to the next line — full path is preserved, no truncation ellipsis. Matches the PR's stated behavior exactly. Unit TestsTypecheck ( 中文说明代码审查：PR 方案与独立方案一致，且更加完善——增加了完整的高度估算函数，处理了状态指示器宽度、耗时标签预留宽度和宽字符（CJK）。所有复用（ 真实场景测试：在 74 列 tmux TUI 中使用 PR 构建测试，确认长路径从截断（ — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — Clean across every stage: real bug with reproduction, minimal focused fix, all tests pass, tmux confirms the before/after behavior. This is exactly the kind of PR the gate should wave through quickly. One prop change fixes the user-visible truncation. The The added tests are well-targeted: wrap behavior, wide characters, active vs completed tool width reservation, and the integration test verifying that Approving. ✅ 中文说明信心度：5/5 — 各阶段均通过：真实 bug 有复现、最小聚焦修复、所有测试通过、tmux 确认修改前后行为差异。 一个 prop 变更修复了用户可见的截断问题。 — Qwen Code · qwen3.7-max Reviewed at |
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.
Reviewed — no blockers. Suggestions are inline.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
Gave this a quick local test-drive with a real model to double-check the fix — built the CLI with these two files' changes on top of
Confirms the fix is load-bearing. Thanks! 🙏 |
|
Thanks for the real tmux test-drive and screenshot, this is very helpful. I also followed up on the later estimator review comments so the timeout-width path is covered now. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
|
|
||
| const COMPACT_GROUP_HORIZONTAL_PADDING = 2; | ||
| const ELAPSED_TIME_MARGIN_LEFT = 1; | ||
| const EXECUTING_ELAPSED_TIME_RESERVED_LABEL = '99h 59m 59s'; |
There was a problem hiding this comment.
[Suggestion] EXECUTING_ELAPSED_TIME_RESERVED_LABEL reserves 13 display columns ('99h 59m 59s') for every executing tool without a timeout, but ToolElapsedTime renders bare elapsed time like '5s' (2–6 columns) and returns null for the first 3 seconds. This shrinks summaryWidth by ~10 unnecessary columns on narrow terminals, potentially causing the summary to wrap one line earlier than the render actually needs. The over-reservation is safe (never starves tool results), but a tighter cap (e.g. '59m 59s' or a dynamic reservation) would reduce unnecessary wrapping at narrow widths.
— qwen3.7-max via Qwen Code /review
| contentWidth: number; | ||
| } | ||
|
|
||
| const COMPACT_GROUP_HORIZONTAL_PADDING = 2; |
There was a problem hiding this comment.
[Suggestion] COMPACT_GROUP_HORIZONTAL_PADDING = 2 correctly matches paddingX={1} in the JSX below, but the JSX uses a bare numeric literal — there is no structural link between the constant and the rendered value. If someone changes paddingX in the future, the height estimator silently underestimates, and tool results below get sized wrong. Consider referencing the same constant from the JSX (e.g. paddingX={COMPACT_GROUP_HORIZONTAL_PADDING / 2}) to make the coupling explicit.
— qwen3.7-max via Qwen Code /review
| const maxElapsedStr = formatDuration(timeoutMs, { | ||
| hideTrailingZeros: true, | ||
| }); | ||
| label = `(${maxElapsedStr} · timeout ${maxElapsedStr})`; |
There was a problem hiding this comment.
[Suggestion] The timeout label format `(${maxElapsedStr} · timeout ${maxElapsedStr})` duplicates the format string from ToolElapsedTime. If the label format changes in one place, the estimator's width reservation silently drifts from the actual rendered width. Consider extracting the format template into a shared constant consumed by both the estimator and the component.
— qwen3.7-max via Qwen Code /review
|
@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. ✅
Local verification at the new head (
|
| Check | Result |
|---|---|
PR test files — CompactToolGroupDisplay.test.tsx / ToolGroupMessage.test.tsx |
33 + 52 = 85 passed |
Whole packages/cli/.../messages/ neighbourhood |
14 files · 293 passed (1 skipped) — no collateral regressions |
Independent invariant (mine): estimate == actual rendered lines |
37/37 cases, widths 12–120, ASCII · long path · CJK |
tsc --noEmit in PR scope |
0 errors in components/messages |
The two behaviours, rendered for real
1. Wrap instead of truncate. At contentWidth=30, main truncates the summary with … and loses the path; this PR wraps it with an indented continuation and keeps the full path.
2. The estimator is exact. The new estimateCompactToolGroupHeight() feeds staticHeight in ToolGroupMessage, which budgets availableTerminalHeight for the sibling tool output. I wrote a separate test (not part of the PR) asserting the estimate equals the number of lines the component actually renders — exact for completed tools, and a safe upper-bound (estimate ≥ actual, never under-reserves) for active ones. It matches across the whole width matrix.
Both halves are load-bearing
I reverted each change in isolation and confirmed a test flips red, then restored the source:
wrap="wrap"→truncate-end⇒wraps long summaries…fails withexpected 1 to be greater than 1.estimateCompactToolGroupHeight(...)→ the old hard-coded1⇒reserves wrapped compact summary height…fails withexpected 10 to be 8. That's the exact 2-line under-reservation the estimator prevents — without it, a wrapped 3-line summary would hand the sibling tool 2 lines of budget it doesn't have and overflow the group.
Notes
- The only
tscerrors in my ad-hoc worktree were pre-existing / cross-package skew in files this PR doesn't touch (serve/*,useExtensionUpdates.ts) — none in the changed files. - The active-state (
Executing) frame can't be captured viaink-testing-library(the animated status spinner produces no synchronous first frame); that path is covered by the unit tests + the invariant's upper-bound assertions, and by the live tmux drive linked above.
Verdict: the fix is correct and load-bearing at e304014e6; both the wrap and the height-estimation paths behave as intended. LGTM from my side. 🙏
中文说明（点击展开）
在新的 HEAD（e304014e6）上完成本地验证 ✅
接续我之前的 tmux 实测——那次覆盖的是首个提交（e93021738）上的换行改动。此后新增的三个提交加入了高度估算逻辑（estimateCompactToolGroupHeight，含超时宽度预留），因此我重新构建并在当前 HEAD 上用真实 Ink 渲染做了复验（被测组件本身不打桩）。
环境： macOS · Node 22.23.1 · Vitest 3.2.4 · 真实 ink-testing-library 渲染 <CompactToolGroupDisplay/>。这同时补齐了 PR 中「🍏 macOS
验证内容
| 检查项 | 结果 |
|---|---|
PR 测试文件 —— CompactToolGroupDisplay.test.tsx / ToolGroupMessage.test.tsx |
33 + 52 = 85 通过 |
整个 messages/ 目录 |
14 个文件 · 293 通过（1 跳过）—— 无连带回归 |
独立不变量（我自建）：estimate == 实际渲染行数 |
37/37，宽度 12–120，ASCII · 长路径 · 中文 |
PR 范围内 tsc --noEmit |
components/messages 中 0 错误 |
两个行为的真实渲染
1. 换行而非截断。 在 contentWidth=30 下，main 会用 … 截断摘要并丢失路径；本 PR 则换行并带缩进续行，完整保留路径。
2. 估算是精确的。 新增的 estimateCompactToolGroupHeight() 会喂给 ToolGroupMessage 的 staticHeight，后者据此为相邻工具输出分配 availableTerminalHeight。我另写了一个测试（不属于本 PR），断言估算值等于组件实际渲染的行数——对已完成工具是精确相等，对进行中工具是安全上界（estimate ≥ actual，绝不低估）。在整个宽度矩阵上都吻合。
（图见上方英文部分：渲染前后对比 + 估算对照表）
两处改动都「承重」
我分别单独回退每处改动并确认对应测试变红，然后恢复源码：
wrap="wrap"→truncate-end⇒wraps long summaries…失败，报expected 1 to be greater than 1。estimateCompactToolGroupHeight(...)→ 旧的硬编码1⇒reserves wrapped compact summary height…失败，报expected 10 to be 8。这正是估算所避免的 2 行低估——若无此改动，一个换行后为 3 行的摘要会给相邻工具多分配 2 行其实并不存在的高度，从而使整个组溢出。
（图见上方英文部分：verbatim vitest 输出）
补充说明
- 我这个临时 worktree 里仅有的
tsc报错都属于既有 / 跨包版本偏差，出现在本 PR 未触及的文件（serve/*、useExtensionUpdates.ts），改动文件本身零报错。 - 进行中（
Executing）状态的帧无法用ink-testing-library捕获（动画状态指示器没有同步首帧）；该路径已由单元测试 + 不变量的上界断言覆盖，并有上方链接的 tmux 实测佐证。
结论： 在 e304014e6 上，修复正确且承重；换行与高度估算两条路径均符合预期。我这边 LGTM。🙏
Built & verified locally on macOS from the PR head — real Ink render, not CI. Images hosted on the pr-assets/6847-verify branch.
… compute the verdict (QwenLM#6892) * fix(review): prove coverage on both topologies, and check the prompt survived the trip Three defects, all measured against the harness's own transcripts of real /review runs against QwenLM/qwen-code PRs QwenLM#6766 (Step 3B) and QwenLM#6579 (Step 3A). 1. Step 3A reviews were told nobody had read them. Coverage was attributed by one question: did an agent whose launch prompt says `chunk N of M` make a successful tool call? No Step 3A prompt says that — there every dimension agent walks the whole diff — so no chunk was ever attributed to anyone. Run against a real 3A review whose twelve agents each opened the diff, walked both chunks and filed findings, check-coverage returned Coverage: 0/2 chunk(s) reviewed. 16 agent(s) ran; 16 did work ERROR: 2 chunk(s) were not reviewed — 1, 2. Nobody read those lines. in one breath. compose-review runs the same computation on the way to the verdict, so a flawless small-PR review was capped away from Approve and the body it would have POSTED to the pull request said nobody had read it. Step 3A is the topology most pull requests get. The only reason it never blew up is that check-coverage lived inside Step 3B and was never reached from 3A — two bugs cancelling. Coverage is now the intersection of two things the harness wrote down: the lines each agent was pointed at (its launch prompt) and the fact that it opened the diff (a successful tool call naming the diff file). Topology-blind. It also no longer credits a chunk to an agent on the strength of any successful call — a glob for test files was enough. 2. The whole-diff agents were still launched blind. agent-prompt built the territory agents' prompts and left the other half of the fan-out to prose. All three whole-diff agents of the 3B run — cross-file tracer, test-coverage matrix, build & test — got a prompt naming no diff file at all. The test-coverage matrix was told to "Read the diff chunks" and given no path to read them from; it read the post-change source instead, which on a deletion shows it nothing. These agents own the classes a chunk agent is structurally blind to, and the gate could not see it: it only asked that question of agents whose prompt said `chunk N of M`. `agent-prompt --whole-diff` now builds their diff-reading block too. 3. The prompt the CLI built was rewritten on the way to the agent. The 3B run invoked agent-prompt correctly for all five chunks and then paraphrased what it printed: the delivered prompt dropped the rule against reciting a stock sentence, dropped the half-read warning, replaced the project's review rules with a three-sentence summary of its own, and invented an instruction that was never in the original. Nothing could see it, because a paraphrase keeps the diff path. So agent-prompt records what it emitted, at a path derived from the plan that the caller is never given; check-coverage reads it back against the launch prompt the harness recorded. Replayed against that run's real transcripts, all five chunk agents are now named. Verified end to end: a fresh /review of QwenLM#6829 (3A) calls `agent-prompt --whole-diff`, passes it verbatim, and Step 3D reports 2/2 chunks reviewed, 12 agents, 12 did work — a gate that path could not reach before, and could not have passed if it had. * fix(review): build every agent's prompt, from a roster the plan derives Two more failures, both measured against the harness's own transcripts of real /review runs. 1. Agent 0 was never launched, and nothing could tell. The skill says issue fidelity runs on every PR review. Dogfooded on QwenLM#6766, it did not run — and every check passed, because every check asks a question of an agent that RAN. An agent that does not run leaves no transcript to ask. An omission is invisible precisely because it is an omission. So `check-coverage` now derives a roster from the plan — which the caller does not write — and names every required agent that never ran, with the exact `agent-prompt` call that builds it. The plan already knew everything the roster turns on: the topology, whether the diff deletes anything, which files were rewritten heavily enough to need invariant agents, whether there is a worktree to build in and a pull request to check an issue against. `agent-prompt --role <role>` builds all of them: 0, 1a, 1b, 1c, 2, 3, 4, 5, 6a/6b/6c, 7, the test matrix, and the three invariant agents per heavy file. The briefs move out of SKILL.md and into code, because a brief the orchestrator retypes is a brief that drifts. 2. A 4 652-character prompt is not a thing an orchestrator will paste twelve times. With the briefs welded into the launch prompt, the first dogfood delivered **2 893** characters of one: it kept the head, added a preamble of its own, and cut nineteen hundred characters out of the middle. The delivery check caught it — and the run then read the check's exit-3, concluded "the agents clearly did their job", skipped `compose-review`, and filed an **Approve it had written itself**. A gate that always fails is a gate that gets talked around. So the brief goes where the diff already goes: on disk, read by the agent that needs it. The launch prompt drops to ~500-800 characters — it names the role, points at the brief file, and lists the diff reads — and whether the agent actually read its brief stops being a hope and becomes a tool call the harness wrote down (`unreadBriefs`). Verified end to end. A fresh /review of QwenLM#6847 built all twelve role prompts, and against the harness's transcripts: 12 of 12 delivered **byte-for-byte verbatim**, 12 of 12 **opened their brief**, 3-19 successful tool calls each — Agent 0 among them. Step 3D: `1/1 chunks reviewed, 12/12 agents did work`, no errors. * fix(review): the verdict is computed, not carried `compose-review` has computed the event and the body since the C/S table stopped being prose. The skill then told the orchestrator to "copy event/body verbatim into the review JSON" — a transcription, into a document the model writes, of a decision the CLI had already made. That is the exact anti-pattern `submit`'s own header repudiates, and it left two ways for a run to author its own verdict: - **The terminal.** Step 6's verdict was composed by the model, from prose rules. Dogfooded, a run read the coverage check's refusal, concluded that "the agents clearly did their job", never called `compose-review` at all, and printed `Review complete — Approve` on a review whose gate had just refused. - **The wire.** `submit` took `{event, body}` as fields. Nothing stopped a run that had skipped the computation from posting the conclusion it preferred. So `submit` composes. It takes the findings — the inline comments and the states Step 6 established — and derives everything that follows, including how many blockers there are: `criticalsInline` and `suggestionsInline` are counted off the `**[Critical]**` / `**[Suggestion]**` prefixes of the comments actually attached, not accepted as numbers beside them. (A number beside a list is a number that can disagree with the list, and one did: the breaching run posted a body reading "Suggestions are inline" next to an empty `comments` array and a summary claiming `0 Suggestion inline`.) A payload carrying `event`/`body` is refused rather than silently overruled — the caller was trying to author a verdict. Two body checks are deleted, not weakened: a body that promises inline comments it does not carry, and a body whose footer is preceded by a literal `\n`. Both were checks on a string the caller built. The caller no longer builds it. `compose-review` now prints the verdict line itself, and Step 6 prints that. There is one place a verdict exists; skipping the command does not get you a different one, it gets you none. Verified: a payload with `event: APPROVE` and an unreviewed dimension is refused at the wire; the same findings without a verdict compose to `COMMENT`. A fresh /review of QwenLM#6788 called `compose-review`, was told `Verdict: Comment`, and showed the user "Comment — downgraded from Approve (CI failing: route)" — the presubmit downgrade applied by code, on a run that did not post. * fix(review): make Step 3B carryable, and stop the delivery check crying wolf Dogfooding the Step 3B path — the one topology none of this had been run against — found two defects, and the second is the more important of the two. 1. Eighty-seven kilobytes of chunk prompts, in one response. The briefs moved onto disk for the dimension agents and not for the territory agents. Measured on PR QwenLM#6606 (5 511 diff lines, 17 chunks): 17 chunk launch prompts of ~5 149 characters each — **87 546 characters** the orchestrator was expected to paste unedited. At a twelfth of that load it had already cut nineteen hundred characters out of a single prompt. Chunk agents get the same split: the brief on disk, and a launch prompt that carries only what cannot live anywhere else — `chunk N of M`, which attributes the territory, and the `offset`/`limit`, which are the lines coverage proves were delivered. 87 546 → 14 789 characters. And `check-coverage` now asks the territory agents the same question it asks the others: did you open your brief? 2. The delivery check failed a correct run — all nine agents of it. It was a substring test: the built prompt had to appear in the launch prompt, contiguously. That is a stricter claim than the skill makes, and both of the differences it fired on were legitimate. The orchestrator had inserted **the one-sentence summary of the change that the skill explicitly tells it to add**, which breaks contiguity by construction — and it had reflowed a hard-wrapped sentence onto one line, which changes not one character of meaning. This is the failure this skill keeps re-learning, and this time it was ours: a gate that fires on a correct run is a gate that gets talked around, and there is a dogfood transcript of a model doing exactly that. The rule the check enforces is now the rule the skill states — **you may add; you may not remove, alter, or reorder** — over whitespace-collapsed lines, in order. Verified against the harness's transcripts of a real Step 3B review of QwenLM#6766: nine agents (five chunks, issue fidelity, cross-file tracer, test matrix, build & test), 9/9 delivered intact, 9/9 opened their brief, 6-22 successful tool calls each. `check-coverage`: 5/5 chunks, every list empty, exit 0. * feat(review): path-scoped rules; and move the briefs out of the skill, where they were never reaching the agents Two changes, and the second found a hole the first would not have. 1. Rules that attach to a path, not to a dimension. The nine dimensions are domain-blind by design — "find security bugs" is a lens, not a syllabus — and that holds until a file's failure modes are not guessable from reading it. A GitHub Actions workflow is the clearest case: it is YAML, so it reads as configuration, and the reviewer who treats it as configuration misses every one of its attack classes. Nothing in this review knew to ask whether a `pull_request_target` job checks out the contributor's head — which is the difference between a CI file and a remote code execution with the repository's write token. This repo runs `qwen-autofix.yml`, which posts to pull requests. `agent-prompt` now appends a checklist for such a file to the brief of every code-reviewing agent **whose territory actually contains one**. Scoped, because a rule that fires on every review is a rule that gets skimmed. `/review` runs on other people's repositories, so the calibration matters as much as the content: the blockers are the six that are unambiguously wrong; the two that shade into taste (SHA-pinning, `permissions:`) are Suggestions, exempt the conventions almost everyone keeps, and are scoped to lines the diff touches. No style rules — a linter owns those, and the Exclusion Criteria already forbid them. 2. The briefs move out of SKILL.md — and three things turned out never to have reached an agent at all. The briefs have been built in code since the roster landed, and SKILL.md still carried 38 KB of the same prose. Duplication is drift, and a 178 KB skill is ~45 000 tokens in the orchestrator's context on every review — which is itself a cause of the failure this whole line of work has been chasing. The skill now keeps what each agent is *for* (a table) and drops what it is *sent* (the command's copy is the one that arrives). Doing that surfaced what the code briefs were missing, because the deleted prose had to go somewhere: - **The Exclusion Criteria had never reached an agent.** The skill states them at the end of the document and tells the orchestrator to "apply" them. The agents do not read the document. The single largest precision control in this review has been governing nobody, in every run, since it was written. - **Nor had the anchor rules.** Agents were asked for a snippet and never told what makes one resolvable: prefer added lines, a removed line cannot be anchored at all, a bare `}` matches everywhere. `resolve-anchors` was downstream of a snippet nobody had given the rules to produce. - **Nor the severity calibration.** `SEVERITY`'s own comment warns that a chunk agent owns test coverage with nothing to calibrate it and will file "zero test coverage" as Critical — and then did not include the calibration. All three are in the briefs now. And two degradations the orchestrator used to be told to add by hand — and can no longer add, because it does not write these prompts — are applied by the builder: in cross-repo lightweight mode there is no tree, so 1b and 1c report at `Confidence: low` rather than asserting a re-establishment is missing. A false Critical blocks a merge. Step 3C (the medium-effort inline pass) now *loads* the briefs it needs rather than carrying them: same text as the high-effort agents get, read when that level actually runs instead of sitting in every review's context. SKILL.md: 171 178 → 153 656 bytes. * fix(review): scope an invariant agent to its own file, and let a blocker's blast radius be part of the blocker Two corrections, both from dogfooding the paths that had never been run. 1. An invariant agent was being handed the whole chunk plan. It owns one heavily-rewritten file. Its brief says so, and gives it that file's own slice of the diff. Its *launch prompt* listed every chunk in the review — on PR QwenLM#6457, all twenty-one reads of a 6 149-line diff, for an agent whose job is one file. The wasted reading is the smaller half. Coverage is computed from the ranges in the launch prompt, so an invariant agent was being credited with having read **every chunk in the review**. One of them could have masked twenty missing chunk agents. It now gets exactly its file's `diffRange`, and nothing else. 2. `permissions: write-all` on a job that runs untrusted code is not a Suggestion. The path rule said it was. Dogfooded against a planted vulnerability, the security agent read that and escalated anyway: "grants maximum token scope to a job that processes untrusted contributor code, amplifying the RCE above". It was right and the flat rule was too coarse. A broad token on a privileged job is not a separate recommendation — it is how far the blocker reaches, and it belongs in that finding, at Critical. On an ordinary job it stays a Suggestion. * fix(review): the six findings this skill filed against its own pull request The repository's own `/review` bot reviewed QwenLM#6892 — this change reviewing the code that changes it — and filed six Suggestions. Every one of them is real, and two are fail-open holes in the gates this pull request exists to build. They are fixed here, each with a test. - **`submit` accepted `state: null`.** `=== undefined` is not `== null`, so the structural check passed it; `compose`'s `?? {}` then collapsed it to an empty state and would have posted a review whose footer named no model and whose caps came from nowhere. - **`wasDeliveredVerbatim` was vacuously true for an empty `built` prompt.** A zero-byte record is what a partial write leaves behind — and `recordPrompt` swallows its write errors by design, so this is reachable. `readRecordedPrompts` stores it as `''`, not `undefined`, so the "no prompt was built" guard did not catch it, and the loop's body never executed. The roster would have credited a required role to whichever transcript it looked at first. It now fails closed. - **A chunk read across two pages got no credit.** The check asked for a *single* range containing the chunk, and reads of 1-200 and 201-400 are two — so it contradicted the paging instruction the same review had just given, on exactly the oversized chunks where paging is not optional. Ranges are coalesced first. - **Agent 7 was handed relative paths it could not resolve.** `worktreePath` and the plan path are repo-relative in the report, and Agent 7's working directory *is* the worktree — so `--worktree .qwen/tmp/review-pr-6457` resolved to `<worktree>/.qwen/tmp/review-pr-6457`, which does not exist. This was already visible and nobody had read it: in the 29-agent dogfood run, Agent 7 spent its time running `find … -name "*6457*fetch*"`, hunting for a plan it had been handed a path to. Absolute now. - **`removePromptRecord` was dead code with a comment claiming a caller it did not have.** `cleanup.ts` sweeps the prompt directory by prefix instead. Deleted. - **`--dry-run` omitted `cappedBy`.** The point of a dry run is to see what would be posted; `"event": "COMMENT"` with no reason leaves the reader to guess why the Approve went away. * docs(review): purge the stale event/body payload examples from Step 7 The reviewer caught a real contradiction it filed as Critical: submit.ts now refuses a payload carrying `event`/`body` (those are computed from `state` and the attached comments), but Step 7's main-path 'Build the review JSON' examples still showed `"event": "REQUEST_CHANGES"` / `"body"` and routed the verdict through a copy-it-verbatim step. An orchestrator following the unchanged instructions would have built exactly the payload submit rejects. The correct `{commit_id, comments, state}` shape existed lower in the section (the no-findings branch), added when submit took over composition — but the main-path examples and the compose-review-then-transcribe bullets above them were never reconciled. They are now: one payload shape, no verdict in it, `state` handed to submit, and the inline counts derived from the comments rather than supplied. Found by the repository's own /review on QwenLM#6892. * fix(review): the three findings from the third self-review The repository's /review passed QwenLM#6892 (no blockers) and filed three Suggestions. All three are real; two are contradictions this PR itself introduced. - **verdictLine printed a dangling colon.** When a would-be Approve was taken away by a presubmit downgrade ALONE — no cap state, `cappedBy` empty, `downgraded` true — the code joined the empty array and produced 'an Approve was NOT available: — downgraded by a presubmit check', a colon over nothing. It now collects the reasons (a cap and a downgrade are both reasons, either can be the only one) and prints the clause only when there is a reason to. The function had no test; it has six now, including this case. - **Step 3D said 'six failures' and listed seven, while check-coverage reports eight.** The count drifted as failure classes were added, and the uncoverable-chunk class had no bullet at all. Now 'eight', with the missing bullet written. - **`submit --review` help still advertised `event` / `body`** as payload fields, which the same command now refuses. Updated to `commit_id / comments / state`. Found by the repository's own /review on QwenLM#6892 — the third pass, the one that turned CHANGES_REQUESTED into no-blockers. * fix(review): a heavy file in a Step-3A diff must not demand invariant agents From a human review of QwenLM#6892 (doudouOUC). `heavy` is decided independently of topology (lib/heavy.ts): a ~300-line source file with ~120 changed lines clears the rewrite-ratio branch while srcDiffLines stays under 500 — a Step 3A review. The invariant-agent loop in requiredAgents ran in both topologies, so it added invariant-a/b/c to the roster of a 3A review that never launches them; check-coverage then reported them as missingRoles and exit-3'd, and compose-review capped the verdict — an otherwise-complete small PR, falsely blocked. Gate the loop on isTerritoryFanOut. Step 3A's dimension agents each walk the whole diff, so one already sees both ends of a rewritten file; invariant agents are a 3B mechanism for when the diff is carved into territories and no single agent holds the whole file. roster.test.ts now pins the 3A-heavy case. Also, same review: merge() in coverage.ts copied its first tuple and pushes copies, so it no longer mutates a tuple owned by rec.diffReads (harmless today, pure now). * fix(review): a downgraded Request changes must not read as a plain Comment Fifth self-review, one behavioural finding among five (the rest are test/doc). verdictLine printed 'Comment — downgraded by a presubmit check' for BOTH a Suggestion-only Comment the presubmit moved and a REQUEST_CHANGES it moved down to Comment. The second is a review with confirmed Criticals posted inline, and 'Comment — downgraded' reads to an operator as 'nothing blocking'. It could not tell them apart from baseEvent alone — a cap may already have softened the RC before the downgrade ran — so ComposeReviewResult now carries downgradedFrom, and verdictLine says 'Request changes, downgraded to Comment … (the blockers are still posted)' for that case. Six verdictLine cases now, including this one. Also from the same review, all confirmed: - agent-prompt.test.ts: the describe block named a function that was renamed (buildRolePrompt -> buildRoleBrief), and the mode-rejection it.each covered 2 of the invalid combinations, not the role-mode ones; now covers all five and drops the stale 'two modes' wording. - SKILL.md Step 7: the review-JSON example used a /* */ comment inside a ```json fence (not valid JSON); switched to ```jsonc with a // comment. * fix(review): eight review-round fixes atop the Step-3A invariant resolution Follows 7c499d1, which resolved the doudouOUC roster finding (a heavy file in a Step-3A diff must not demand invariant agents — gate the loop on the topology) and the merge() purity nit. This carries the rest of the same round: - `roster.ts` requires Agent 0 only for a positive PR number. `!== undefined` let `null`/`0`/`''` through. Note the reviewer's suggested `typeof === 'number'` is wrong for this codebase — `fetch-pr` writes the number as a *string* — so the guard accepts a numeric string too, or every real PR review would lose Agent 0. A table test pins both directions. - `transcripts.ts` matches the diff path as a whole JSON string value, so `…/diff.txt.bak` no longer counts as reading `…/diff.txt`. It also documents why FIFO is right for a chronological transcript. - `agent-prompt.ts` scopes path rules to `--file` only for invariant roles — a whole-diff reviewsCode agent passed `--file` would otherwise lose the rules for every other file — and guards each chunk element in `diffReadingBlock` like `chunkFrom`, so a corrupted chunk errors legibly instead of emitting `offset=NaN`. - `compose-review.ts` stops double-wrapping `cov.missingRoles` / `cov.rewrittenPrompts`, which coverage.ts already writes self-explanatory. - `agent-briefs.ts` JSDoc said "Two do not" read the diff; only Build & Test does not. - The agent-prompt size-bound test now covers `test-matrix`. The empty-prompt guard, the paged-read coverage, the verdictLine dangling-colon and the submit help text were all already handled by earlier commits on the branch; those threads are answered without a code change.
3 participants
What this PR does
This PR makes completed compact tool summary lines wrap within their available width instead of truncating the hidden suffix at the terminal edge. It also adds focused Ink rendering coverage for a long file path in a narrow 30-column display, verifying that the result spans multiple lines, contains no truncation ellipsis, and preserves the complete summary.
Why it's needed
Long file paths and command text currently lose useful information when compact tool summaries exceed the terminal width. Wrapping keeps the full summary visible while preserving the existing status indicator and elapsed-time layout. The change is limited to the compact tool summary line; other intentional single-line
truncate-endusages remain unchanged.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsxwith Node.js 22 or newer and confirm all 27 tests pass, includingwraps long summaries instead of truncating them.ReadFiletool whose description ispackages/cli/src/ui/components/messages/CompactToolGroupDisplay.tsxwithcontentWidth={30}and confirm the summary wraps across lines without…while retaining the complete path.npm run typecheck --workspace=packages/cliand confirm it exits successfully.Evidence (Before & After)
Captured from
ink-testing-libraryusing the same completedReadFiletool and 30-column content width.Before (
wrap="truncate-end"):After (
wrap="wrap"):The focused regression test fails against the previous implementation with
expected 1 to be greater than 1because only one line is rendered, and passes with this change.Tested on
Environment (optional)
Windows local workspace with Node.js 24.14.0, npm 10.8.2, Vitest 3.2.4, and
ink-testing-library. The focused test file passed 27/27 tests; CLI typecheck, Prettier checks for both changed files, and ESLint with zero warnings also passed.Risk & Scope
truncate-endusages elsewhere in the UI.Linked Issues
Fixes #6814
中文说明
本 PR 的改动
本 PR 将已完成的紧凑工具摘要行改为在可用宽度内自动换行，而不是在终端边缘截断并隐藏后续内容。同时增加了一个聚焦的 Ink 渲染测试，使用 30 列窄布局中的长文件路径，验证结果会渲染为多行、不包含截断省略号，并完整保留摘要内容。
修改原因
当紧凑工具摘要超过终端宽度时，较长的文件路径和命令文本目前会丢失有用信息。自动换行可以让完整摘要保持可见，同时保留现有的状态指示器和耗时布局。本次修改仅限于紧凑工具摘要行；其他有意保持单行的
truncate-end用法均不变。Reviewer 测试计划
验证方式
cd packages/cli && npx vitest run src/ui/components/messages/CompactToolGroupDisplay.test.tsx，确认全部 27 个测试通过，其中包括wraps long summaries instead of truncating them。ReadFile工具，将其 description 设置为packages/cli/src/ui/components/messages/CompactToolGroupDisplay.tsx，并设置contentWidth={30}；确认摘要会跨行显示、不包含…，并且完整路径得到保留。npm run typecheck --workspace=packages/cli，确认命令成功退出。修改前后证据
以下内容由
ink-testing-library使用同一个已完成的ReadFile工具和 30 列内容宽度渲染得到。修改前（
wrap="truncate-end"）：修改后（
wrap="wrap"）：针对性的回归测试在旧实现上会失败并报告
expected 1 to be greater than 1，因为输出只有一行；应用本次修改后，该测试通过。测试平台
环境（可选）
Windows 本地工作区，使用 Node.js 24.14.0、npm 10.8.2、Vitest 3.2.4 和
ink-testing-library。目标测试文件的 27/27 个测试全部通过；CLI 类型检查、两个修改文件的 Prettier 检查以及零警告 ESLint 检查也均通过。风险与范围
truncate-end。关联 Issue
Fixes #6814