feat(review): report findings to clients as a typed contract - #9794
Conversation
Add a report_findings core tool: one {level, findings[]} call whose
field names and enum spellings match the review findings artifact, so
/review hands live clients (TUI, Web Shell transcript, ACP hosts) the
findings as data instead of a Markdown restatement. The tool sorts by
severity/confidence/location, derives shortSummary (<= 60 chars) for
compact list UIs, and refuses duplicate ids, control characters, and a
partial outcome set - after --fix the skill re-issues the call with a
fixed/skipped/no_change_needed outcome per finding, mirroring the
artifact's own --outcomes completeness rule, and the rule stays live
for later in-session disposition changes.
The finding enums now live in core; packages/cli/utils/findings.ts
re-exports them under its historical names (the Web Shell renderer
keeps its deliberate browser-side copy). The TUI renders the new
findings_list display as per-finding rows with severity color, id,
file:line, confidence marker and outcome badge; the daemon TUI adapter
passes it through and history/recording compaction truncates only the
free-text fields.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Stopping at the template gate on this one, @wenshao — the body doesn't follow the PR template. It reads as free-form ## Summary / ## Verification sections; none of the required headings are present: ## What this PR does, ## Why it's needed, ## Reviewer Test Plan (with ### How to verify, ### Evidence (Before & After), ### Tested on), ## Risk & Scope, ## Linked Issues.
The content itself is largely there — the summary covers what and why, and the tmux captures stand in for evidence — so this should be a restructure rather than a rewrite. Two pieces are genuinely absent rather than just misnamed: the Tested on OS matrix (the tmux verification doesn't say which platform it ran on) and Risk & Scope (main risk / not validated / breaking-change notes — worth stating explicitly for a PR that adds a new core tool and rewires the bundled review skill).
Please reflow the body into the template headings and we'll take it from there. 🙏
中文说明
这一单先停在模板门禁上，@wenshao——PR 正文没有按 PR 模板 组织：目前是自由格式的 ## Summary / ## Verification，必需标题均缺失：## What this PR does、## Why it's needed、## Reviewer Test Plan（含 ### How to verify、### Evidence (Before & After)、### Tested on）、## Risk & Scope、## Linked Issues。
内容本身大体齐备——Summary 覆盖了做什么和为什么，tmux capture 可充作证据——所以只需重排结构，不必重写。有两块是真的缺失而非标题不同名：Tested on 操作系统矩阵（tmux 验证未说明在哪个平台运行）和 Risk & Scope（主要风险/未验证项/破坏性变更说明——对一个新增 core 工具并改动内置 review skill 的 PR 来说值得写明）。
请把正文重排为模板标题格式，之后我们会继续走门禁流程。🙏
— Qwen Code · qwen3.8-max
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
The two CI drift gates caught what the feature commit missed: the CLI requires a zh translation for every core tool display name, and the web-shell requires a display-name entry (and its own zh translation) for every core wire tool name. Add toolDisplayName.ReportFindings to the zh/zh-TW/ca/en locales, report_findings to the web-shell TOOL_DISPLAY_NAMES map, and toolName.report_findings to the web-shell zh strings.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Re-run on Template looks good ✓ Problem: Unchanged — still an observable gap rather than a theory: Direction: Aligned — a typed in-band findings contract is where review pipelines are heading (Claude Code's changelog points the same way: machine-readable findings, per-finding fix status), and the review skill is a first-class qwen-code surface. Size: Production logic ~1100 lines, tests ~1637, design doc 74. Core paths touched, but the author is a maintainer (@wenshao owns Approach: Scope feels right, and the delta since the last reviewed head is convergence, not churn: round 5 hardened the daemon boundary shape check, terminal-safe rendering, and the outcome-identity rule; round 6 implemented true replacement semantics across every transcript surface plus the retained-display budget; the final commit closed the permissive-arm bypass and rewind recovery. The one main merge ( Risk: No elevated risk signals — the revert-correlated path scan matched nothing. Moving on to code review. 🔍 中文说明在 模板完整 ✓ 问题：不变——仍是可观察的缺口而非理论假设： 方向：对齐——带内 typed findings 契约正是评审管线的演进方向（Claude Code 的 changelog 印证了同样趋势），评审技能是 qwen-code 的一等功能面。 规模：生产逻辑约 1100 行，测试约 1637 行，设计文档 74 行。触及核心路径，但作者是维护者（@wenshao 是 CODEOWNERS 中 方案：范围合理，且上一轮之后的增量是收敛而非发散：round 5 加固 daemon 边界形状检查、终端安全渲染与 outcome 身份规则；round 6 在全部转写面实现真正的替换语义并加上保留显示预算；最后一个提交关闭了宽松分支绕过与 rewind 恢复。唯一一次 main 合并（ 风险：无升级风险信号——与 revert 相关的高风险路径扫描无命中。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the full delta since the last pass — three branch-authored commits (~1470 lines, ~930 of them tests) plus one main merge. Each commit closes a named gap, and I read all three patches end to end:
No critical issues. Two non-blocking notes: the compaction comment's "a third of the budget" phrasing slightly overstates (worst-case finding ≈ 11.4 KB, a bit over one third of 32 KB — the invariant that matters, one finding always fits, holds); and the PR body still says The delivery flow the skill drives: sequenceDiagram
participant P1 as Review skill
participant P2 as report_findings tool
participant P3 as Live clients (TUI, Web Shell, ACP)
P1->>P2: call with level and findings copied from the artifact
P2-->>P3: findings_list display (sorted, compressed summaries)
P3-->>P1: render per-finding rows
Note over P1: after --fix or fix these issues
P1->>P2: re-issue call, every finding carries its outcome
P2-->>P3: same list with fixed / skipped / no change needed badges
Files changed (30 of 35 shown)
Test evidence — the PR's own CIRead from the check-runs on the reviewed commit; nothing re-run locally, per the CI-path rule. Every check on
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed lanes. The live-rendering claim still rests on the isolated 中文说明代码审查完整审查了上一轮之后的增量——三个分支自写提交（约 1470 行，其中约 930 行为测试）加一次 main 合并。三个补丁逐行读完，每个提交都在关闭一个具名缺口：
无阻塞问题。两条非阻塞备注：压缩注释中"预算的三分之一"表述略有夸大（最坏单条约 11.4 KB，略超 32 KB 的三分之一——真正重要的不变量"单条总能容纳"成立）；PR 正文仍写 测试证据从被审提交的 check-runs 读取，按 CI 路径规则未本地重跑。 沙箱通道。 实时渲染结论仍基于 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — every gap the prior rounds raised was closed with a pinned test, the merge is verified from both parents, every check green on the exact reviewed head; the remaining notes are non-blocking and the one live-rendering re-pin is already in motion. Stepping back: the question this pass had to answer was whether three rapid review-driven commits and a main merge damaged or diluted the contract. They strengthened it. My independent baseline for this problem — a UI-delivery-only tool mirroring If I had to maintain this in six months, the shape still reads right: the artifact stays the oracle, each call replaces the whole list, a partial outcome set is refused, identity is held to what the client actually received, a failed delivery is a disclosed no-op that never touches the verdict, and compaction can only shorten free text or count an evicted tail — never alter a severity or an outcome. The delivery dependency on model compliance degrades to today's behavior by design. Standing notes, none blocking: the PR body's stale 中文说明置信度：4/5 —— 此前各轮提出的每个缺口都已关闭并有测试锁定，合并已从双亲核实，被审 head 上所有检查全绿；剩余备注均不阻塞，唯一的实时渲染复证已在进行中。 退一步看：本轮要回答的问题是——三个快速的评审驱动提交加一次 main 合并，是否损害或稀释了契约？答案是它们强化了契约。我对这个问题的独立基线——一个镜像 若六个月后由我维护，这个形态依然经得起读：工件保持 oracle 地位、每次调用整表替换、部分 outcome 集合被拒绝、身份对齐客户端实际收到的内容、交付失败只是披露后跳过绝不触碰裁决、压缩只能缩短自由文本或计数被驱逐的尾部——永不改动严重度或 outcome。交付对模型遵从性的依赖按设计退化为现状。 长期备注，均不阻塞：PR 正文中过时的 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @wenshao — pure policy cap: ~655 production lines across core paths on a |
Stage-2 review observation: FindingsDisplay had direct render tests, but nothing pinned the ToolMessage discriminator, so removing the routing branch kept every test green while findings fell through to the JSON-string fallback. The new case asserts the joined file:line row and the low-confidence marker, which the fallback never produces; verified by mutation (disabling the branch turns the test red).
|
Addressed the stage-2 findings:
Triggering the sandboxed tmux verification next so the live-rendering claim doesn't rest on my own captures. 中文说明已处理 stage-2 的发现:
接下来触发沙箱 tmux 验证,让真实渲染的结论不依赖我自己的 capture。 🤖 Generated with Claude Code — Claude Fable 5 |
|
@qwen-code /tmux |
|
tmux real-user testing: pass - workflow run Launched the changed app in a real tmux session and exercised the affected flow. No report.md or tmux-readable-full.log was found in tmux-results, so detailed report sections are omitted. — Qwen Code · tmux real-user testing |
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.
Test Plan (not a blocker): src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Two behavior fixes: compressFindingSummary backs its hard cut off a surrogate pair instead of emitting an unpaired high surrogate, and sortReportedFindings now matches the artifact's sortFindings exactly (code-unit file/id comparison, missing line ranked first) as its doc comment already claimed. SKILL.md Step 6 gains the bounded-contract rule: the tool refuses over-cap calls whole, so an artifact past 50 findings reports the most-severe 50 with the cut disclosed, and over-cap prose is shortened rather than dropped. The rest closes the mutation gaps the review demonstrated: fixtures where shortSummary differs from summary (pinning that rows render the compact label), the summary blank-guard case, exact-value assertions for shortSummary derivation and the word-boundary cut, per-field control-character coverage, line/id passthrough, and the artifact-order tiebreaks (missing line first, id by code units under a stable sort). Every new assertion was mutation-verified: each documented mutant now turns at least one test red.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No code changes this round. The ten round-1 inline findings were already answered in Round-1 findings — verified in code, threads marked resolvedEach fix was verified by removing the responsible guard/branch (the exact mutant named in the finding), watching the focused test turn red, restoring, and re-running green:
One finding stays deliberately open: rc:3838651416 (the findings compaction branch being a structural no-op at today's schema caps). The round-1 reply declined it as deliberate defense in depth — the Template gate (rv:5002179863) — prepared, needs a GitHub writeThe CHANGES_REQUESTED review asks for the PR body to be reflowed into the repository template's headings. That is PR metadata, not a code change, and this mode has no PR-edit authority or workflow hook. To make it a one-command step, a template-compliant body was prepared in this run's artifacts ( Everything else
Verification
中文说明本轮没有代码改动。十条第一轮行内发现在本轮运行之前已由 第一轮发现——已在代码中验证，线程标记为已解决每条修复的验证方式：移除对应的守卫/分支（即发现中点名的变异体），确认聚焦测试变红，恢复后再跑回绿：
有一条发现刻意保持开放：rc:3838651416（findings 压缩分支在今天的 schema 上限下结构性空操作）。第一轮回复以「刻意的纵深防御」为由拒绝—— 模板门禁（rv:5002179863）——已备好，需要一次 GitHub 写操作这条 CHANGES_REQUESTED 评审要求把 PR 正文重排为仓库模板的标题。那是 PR 元数据而非代码改动，本模式没有编辑 PR 的权限，工作流也没有对应钩子。为把它变成一步操作，本次运行的产物中备好了一份符合模板的正文（ 其余各项
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
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.
Test Plan (not a blocker): src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/report-findings.ts:264 — [probe] absent-confidence sort rank pinned by no testpackages/core/src/tools/report-findings.ts:261 — [review] sort comparator hand-duplicated across packagespackages/core/src/tools/report-findings.ts:249 — [probe] outcomeNote/source/category pass-through unassertedpackages/core/src/tools/report-findings.test.ts:179 — [review] no_change_needed outcome exercised by no testpackages/core/src/tools/report-findings.ts:158 — [probe] per-field maxLength caps exercised by no testpackages/core/src/skills/bundled/review/SKILL.md:586 — [review] low-tier re-issue rules presuppose a missing artifactpackages/core/src/tools/report-findings.ts:184 — [probe] outcomeNote without outcome passes the all-or-none guardpackages/core/src/tools/report-findings.ts:144 — [probe] schema promises a source default the code never applies
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…typed-contract # Conflicts: # packages/cli/src/commands/review/findings.ts
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9794 (round 3)Base-conflict resolution (
|
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. ✅
…9794) - Align the tool's `file` cap with the artifact path domain (PATH_MAX, 4096) and refuse line numbers outside JavaScript's safe integer range. - Refuse `outcome: "skipped"` without a non-empty `outcomeNote`, in the tool and in the `review findings --outcomes` ledger that feeds it. - Hold an outcome re-report to the active report's identity — same ids, none dropped, none added — so a partial fix run cannot silently shorten the client's list. - Render the report-level `level: "low"` state in FindingsDisplay and sanitize every interpolated row value to one terminal-safe line. - Validate the full findings_list shape at the daemon TUI boundary and fall back to plain text for malformed payloads instead of crashing. - SKILL.md: extend the sanctioned over-cap shortening to `outcomeNote` on the Step 6B re-report; the artifact keeps the full-length text.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9794 (address-review, round 6)Round 5 posted 11 Critical inline findings. This round reproduces, fixes, and Resolved findings (8)
Deferred to the next round (3)Deferred only under the ~8-finding batch bound — all three are verified real
VerificationCommands actually run (final state, commit
中文说明Autofix 本轮总结 — PR #9794（address-review，第 6 轮）第 5 轮共发布 11 条 Critical 行内发现。本轮复现、修复并为其中 8 条补齐测试见证（受每轮批量上限约束），其余 3 条已在各自线程回复中明确延后到下一轮。无需合并 base（ 已解决的发现（8 条）
延后到下一轮（3 条）仅因 ~8 条批量上限延后——三条均已确认真实存在，并将在下一轮优先处理，且已在各自线程回复：
验证实际执行的命令（最终状态，commit
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Requesting changes at exact head 01e5f2b0bbd0213f3471b6d00d008dbef19f341f.
The existing replacement-semantics blocker still stands after round 5. A complete initial report followed by a complete outcome report creates two distinct tool/history entries in the in-process TUI and two distinct calls in the daemon reducer. The actual renderer shows the stale ● Critical ... row beside the new ✓ Critical ... (fixed) row, so the core promise that a later call replaces the list is not implemented. Please add a stable logical report identity and replacement across live history, restored history, recording/resume, and the daemon projection.
Round 5 also leaves two correctness gaps in the new fixes:
activeReportIdsis process-local. The same tool instance rejects a 3→1 outcome subset, but a fresh instance after cold session resume accepts it (COLD_RESUME_SUBSET=accepted). Persist/reconstruct the active identity or make it explicit in the contract, and add a cold-resume test.FindingsDisplayreturns early for an empty list before rendering the new low-effort banner, so{ level: "low", findings: [] }still displays only the unqualifiedNo findings.. Keep the unverified marker for empty quick passes too.
The acknowledged cleanup/recovery and aggregate-retention blockers also remain open in their existing threads; I have not duplicated them inline. The focused core and CLI suites pass, but they do not cover the replacement or cold-resume paths above.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": did not trace whether any daemon-mode session keeps a single shared Config/tool instance across concurrent sessions (cross-session identity bleed) — the cached-…; chunk 4: executing packages/core/src/tools/report-findings.test.ts with vitest to confirm the suite passes — the shared worktree has no node_modules or built dist, and….
Test Plan (not a blocker): src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/report-findings.ts:271 — [probe] absent-confidence sort rank pinned by no testpackages/core/src/tools/report-findings.test.ts:483 — [probe] identity-removal transition of activeReportIds pinned by no testpackages/core/src/tools/report-findings.ts:300 — [probe] outcome-identity gate lost across --resume/process restartpackages/cli/src/ui/components/FindingsDisplay.tsx:31 — [probe] terminalSafe misses bidi/zero-width classes on the resume re-entry path
Convergence: round 6 posted 3 inline comment(s), 1 of them reported for the first time; the previous round posted 0 (0 new). Findings keep coming back to the same files: packages/core/src/tools/report-findings.ts (findings in round 5; 1 more now). (Evidence: the previous round was recovered from a marker this account did not post, so those rounds and its counts may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：did not trace whether any daemon-mode session keeps a single shared Config/tool instance across concurrent sessions (cross-session identity bleed) — the cached-…；chunk 4：executing packages/core/src/tools/report-findings.test.ts with vitest to confirm the suite passes — the shared worktree has no node_modules or built dist, and…。
Test Plan（非阻断）：src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 3 条行内评论，其中 1 条是首次提出；上一轮发布了 0 条（其中 0 条首次提出）。发现反复回到同一批文件：packages/core/src/tools/report-findings.ts（第 5 轮已出过发现，本轮又有 1 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，上述轮次与其计数可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…e round 6 contract gaps (#9794)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round 6 — address-review summary (PR #9794)Commit: Feedback points and dispositions1. R6-1 — identity committed at build time (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/utils/resumeHistoryUtils.test.ts:1859 — [review] new describe nested inside the wrong parent describepackages/core/src/tools/report-findings.ts:415 — [probe] outcome-identity gate bypassed by a same-batch report+outcome pairpackages/core/src/skills/bundled/review/SKILL.md:14 — [review] report_findings in skill allowedTools pinned by no testpackages/core/src/tools/report-findings.ts:264 — [probe] source carry-through pinned by no test; mutation ships greenpackages/cli/src/ui/daemon/daemon-tui-adapter.ts:280 — [probe] daemon isFindingsListDisplay never checks the type discriminatorpackages/cli/src/ui/components/messages/ToolMessage.tsx:222 — [probe] resume ingestion casts findings_list on the discriminator alonepackages/core/src/utils/toolResultDisplayCompaction.ts:414 — [probe] findingRetainedSize +20 allowance undercounts JSON shape by ~166 charspackages/core/src/tools/report-findings.test.ts:65 — [probe] absent-confidence sort rank pinned by no testpackages/core/src/tools/tools.ts:854 — [review] schema promises a source default the code never appliespackages/core/src/skills/bundled/review/SKILL.md:998 — [review] low-tier outcome re-issue rule presupposes an artifact the tier never haspackages/core/src/tools/report-findings.ts:146 — [review] confidence schema description contradicts the SKILL.md low-tier rulepackages/core/src/tools/report-findings.test.ts:229 — [probe] outcomeNote carry-through pinned by no testpackages/core/src/skills/bundled/review/SKILL.md:915 — [review] post-delivery severity hold never re-issues report_findings
Convergence: round 7 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 3 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 13 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 3 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…reports on rewind (#9794)
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9794 (Critical-only window)Growth audit: Feedback points and dispositions[rc:3846005471] R7-1 (Critical) — findings trust boundary bypassable via
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed.
Not reviewed: reverse audit — its prompt was built, but no agent was launched with it — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries, and cannot be certified.
中文说明
已审查。
未审查：反向审计——它的 prompt 已构建，但没有 agent 用它启动——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法，无法作证。
— qwen-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — but one check was deliberately not deepened: I did not trace the full daemon producer chain ( ToolCallEmitter.emitResult , transcript-replay, SDK reconne….
Test Plan (not a blocker): src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/utils/findings-coalescing.ts:25 — [review] same-named isFindingsListDisplay predicates differ in strictnesspackages/cli/src/ui/daemon/daemon-tui-adapter.ts:340 — [review] rejected findings_list payload degrades to raw JSON with no logpackages/cli/src/ui/hooks/useHistoryManager.ts:93 — [probe] commit-time coalesce leaves the classic <Static> scrollback stale
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — but one check was deliberately not deepened: I did not trace the full daemon producer chain ( ToolCallEmitter.emitResult , transcript-replay, SDK reconne…。
Test Plan（非阻断）：src/foo.ts:42 — no such file or directory; src/bar.ts:7 — no such file or directory; src/baz.ts — no such file or directory; src/tools/report-findings.test.ts — no such file or directory; src/utils/toolResultDisplayCompaction.test.ts — no such file or directory; and 6 more。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round (no code changes): every requested finding was already resolved at the current head Dispositions — maintainer review rv:5005393579 (@doudouOUC, CHANGES_REQUESTED at
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 1974 passed · 0 failed · 1974 total Flakiness gate: ✅ 11 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1974 通过 · 0 失败 · 1974 总计 抖动门：✅ 11 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9794 Deep Verification (round 3) —
|
| # | Previous finding | Severity | Status at 6e242c3 |
|---|---|---|---|
| 1 | Unrelated pure-Prettier reformat of scripts/tests/cua-driver-release-workflow.test.js (round 1) |
nit | fixed — still fixed: git diff HEAD^1..HEAD -- <file> empty at the new head (harness h5, PF-1) |
| 2 | shortSummary has no schema maxLength while the other six string fields are capped (round 1) |
informational | stands — re-measured: property still uncapped, the other six fields still carry caps, and the behavioral bound holds (100× compression of a 100 KB input → ≤ 60 chars in 72.6 ms, linear). I continue to agree with the accepted rationale |
| 3 | Color-sensitive assertions in the PR's new FindingsDisplay.test.tsx (round 2: 1 test red under FORCE_COLOR=1) |
non-blocking | worsened — re-measured at the new head: 6/13 red under FORCE_COLOR=1 (capture 06-force-color-sensitive-assertions.png, exit 1). The five new control-character-sanitization tests added by this delta inherit the same style: their not.toMatch(/[\u0000-\u0008\u000b-\u001f\u007f-\u009f]/) scans trip on the terminal's own \u001b[…m color codes, and the substring assertion crosses ANSI-split children. Still non-blocking: CI vitest runs colorless (baseline 13/13 green, plus the workflow's flakiness gate) and production rendering is correct. The candidate fix is now measured: wrapping lastFrame() in strip-ansi in a scratch copy turned both regimes green (13/13 under FORCE_COLOR=1, 13/13 colorless) without weakening any assertion — see Findings #1 |
| 4 | agent-core.ts carries a formatting-only delta (+4/−2 union rewraps) (round 2) |
nit | stands — re-measured: still the only two hunks, both type positions, and base fails prettier --check while head passes (canonicalizing). Inert; no action required |
Corrections
- Round 2's correction still holds: the enum/compress single-sourcing lives in
packages/cli/src/commands/review/findings.ts(re-exporting core's enums), not the oldutils/findings.tspath named in earlier prose. Its suite (now +60 lines:--inputwrapper unwrap, skipped-without-note ledger refusal) ran green in the cli gate — it is the behavior-preservation pin for the re-export. - No new corrections this round. One self-correction from the harness bench, recorded so the counts are auditable: my first h1 run bound the identity-gate rejection cells to the wrong tool instance (the gate is per-instance; the module-level instance never delivered the initial report). Re-bound to the delivering instance — the gate then refused exactly as contracted. All 3 initially-red cells were harness bugs, not PR defects.
Central claim and A/B proof
Central claim (re-proven at the new head): report_findings hands /review findings to clients as typed data — validating (control chars, duplicate ids, partial outcome sets, schema caps, and the round-5 additions: skipped requires outcomeNote, line within the safe-integer range, file capped at PATH_MAX, outcome re-reports held to the active report's identity), sorting exactly like the findings artifact, compressing shortSummary to ≤ 60 chars, registering in headless sessions, and emitting a findings_list display the TUI/daemon/compaction layers handle.
This round's delta claims (the four commits after fd41284): (D1) a findings_list record at the daemon boundary either passes the full shape check or falls back to plain text, discriminator-first — no ansiOutput/fileDiff key can smuggle a malformed payload past the guard; (D2) replacement semantics — every transcript surface keeps only the last delivered list and stashes the superseded one; (D3) a rewind past the replacing call restores the superseded report; (D4) the outcome identity gate is a live-process contract committed on delivery.
A/B cells — base absence re-anchored at this round's base (harness h1, capture 01-contract-cells-base-vs-head.png)
Base side = scratch worktree at HEAD^1 (22bb5e8b9f), core rebuilt there. Lockfiles untouched by the PR (verified: git diff HEAD^1..HEAD -- package.json package-lock.json empty), so external deps are a clean control; one dependency note in Methodology (nested ajv resolution required linking head's packages/core/node_modules into the base tree — quoted realpath checks, and the base B-cells only assert absence of files the PR adds).
| Cell | Arm | Oracle | Result |
|---|---|---|---|
| B-1 | base | ToolNames.REPORT_FINDINGS |
✅ undefined |
| B-2 | base | import report-findings.js |
✅ ERR_MODULE_NOT_FOUND (file absent from rebuilt base dist) |
| B-3 | base | index exports | ✅ no ReportFindingsTool / FINDING_SEVERITIES / compressFindingSummary |
| B-4 | base | config.ts source |
✅ zero REPORT_FINDINGS occurrences |
| H-3 | head | registration position | ✅ registerLazy(REPORT_FINDINGS) before the resolveInteractionMode(...) !== 'headless' gate |
A/B — daemon boundary bypass (harness h4, capture 04-daemon-boundary-ab.png)
Control arm = the shipped dist daemon-tui-adapter.js with exactly one hunk reverted: the findings_list branch swapped AFTER the generic fileDiff/ansiOutput/todo_list/… chain (the pre-fix order the commit title closes). Same-directory control file, removed afterward.
| Cell | Head arm | Control arm (pre-fix order) |
|---|---|---|
| C1 well-formed payload | structured findings_list ✅ |
structured ✅ (sanity: control not broken overall) |
C2 five malformed shapes (missing array, string findings, missing fields, bad level enum, bad outcome enum) |
all 5 → plain-text fallback ✅ | — |
C3 malformed + ansiOutput:'SMUGGLED' key |
text fallback containing CORRUPT, no structured display ✅ |
bypass reproduces: structured ansiOutput display escapes quarantine, and the renderer read (findings.length then .map on 'CORRUPT') is a TypeError — the crash the boundary exists to prevent ✅ |
C4 valid findings + extra fileDiff key |
findings rendered ✅ | parity measured: sanitizeDaemonValue keeps all keys in BOTH arms — no arm difference for valid payloads; the substitution the pre-fix order enabled exists only for malformed ones (C3) ✅ |
| C5 second report supersedes first in projection | marker + new list ✅ | — |
11/11 assertions. Sibling sweep on the same boundary: formatToolResultDisplay is the single path rawOutput takes into the daemon projection (toolUpdateToHistoryItem); the content-text path is separately sanitized.
A/B — replacement semantics and rewind restore (harness h3, capture 03-coalesce-rewind-ab.png)
Controls are scratch copies of the shipped dist findings-coalescing.js (zero runtime imports — type-only source imports, erased at compile): control-0 = identity coalescer (the pre-delta state), control-1 = recoalesce with the restore phase reverted.
| Scenario | Head | Control |
|---|---|---|
| Initial report + outcome re-report | exactly one delivered list; superseded tool carries the marker, original stashed | control-0 renders two checklists side by side ✅ |
| Three-report chain | last renders; ORIGINAL stash preserved through double supersede | — |
| Rewind past the replacing call (AppContainer slice semantics) | superseded checklist restored, stash cleared, one list ✅ | control-1 leaves the marker, checklist lost ✅ |
| Rewind keeping two of three reports | re-coalesces: last survivor renders, first re-superseded with original stash ✅ | control-1 leaves both survivors as markers (zero lists) ✅ |
| Compacted (cleared) tool | stays cleared through recoalesce — privacy clear is not resurrected ✅ | — |
| No-op inputs (single list / none / marker without stash) | input array returned by identity ✅ | — |
22/22 assertions.
Contract + identity gate (harness h1, 69/69; capture 01-contract-cells-base-vs-head.png)
Accept path: sort order with absent-confidence ranking between high/low (exact id sequence), shortSummary derivation exact values (word-boundary cut, surrogate-pair backoff exact values at both cap positions), outcome-count line, level carry/omission, empty list. Reject path: partial outcomes, trim-normalized duplicate ids, per-field control characters (TAB/LF/CR prose split, BIDI U+202E), blank required fields, line: 0, line: 2^53 refused vs MAX_SAFE_INTEGER accepted, file 4097 refused vs 4096 (PATH_MAX) accepted, skipped without/with-blank outcomeNote refused (round 5), enum violations, additionalProperties at both levels, 51 refused / 50 accepted.
Identity gate (D4) — 8 flow cells, all per-instance: drops-a-finding refused with the missing id quoted; unknown id refused; missing-id finding in an outcome call refused ((missing id)); exact-id set accepted; a fresh non-outcome report replaces the identity (gate then refuses against the NEW set — measured message names B-1/B-2, proving the gate tracks the latest delivery, not the first); an id-less report commits no identity; a built-but-never-executed report commits no identity (delivery, not build, commits); a rejected call does not move the identity.
Sort-equivalence oracle (harness h2, capture 02-sort-equivalence-oracle.png)
Reference = the artifact's own sortFindings lifted from the built CLI dist, fed artifact-shaped entries (locations[], mandatory confidence); candidate = the tool observed through build()+execute() output order. 16 seeded batches × 35 findings + crafted tie batch = 566 findings, 0 divergent pairs. Crafted order pinned exactly: code-unit file order B.ts < Z.ts < a.ts < z.ts, missing line first, R1-1 < R1-10 by code units.
Mutation matrix — 10/10 killed (harness mut-runner, capture 05-mutation-matrix-10-of-10-killed.png)
One row per delta guard; each mutant reverts exactly one hunk in source (single-occurrence patch), runs the suite collocated with the mutated file, and is restored byte-identical (sha-verified, restored=true all ten; git status clean after). Color normalized off for the vitest children.
| Mutant | Suite | Result (first red assertion) |
|---|---|---|
| M1 findings branch after the generic chain (the bypass order) | daemon-tui-adapter | KILLED — falls back to text for malformed findings_list payloads even when permissive [keys present] |
| M2 supersede loop removed | daemon-tui-adapter | KILLED — replaces the previous findings list in the projection |
| M3 live coalesce-on-commit removed | useHistoryManager | KILLED — replaces earlier findings displays when a new report_findings group commits |
| M4 rewind recoalesce removed | AppContainer | KILLED — restores a superseded findings list when rewinding past its replacing call |
| M5 resume coalesce removed | resumeHistoryUtils | KILLED — keeps only the latest delivered findings list in the restored transcript |
| M6 recoalesce restore phase removed | findings-coalescing | KILLED — restores a superseded display whose replacing call was truncated away |
| M7 skipped-without-note refusal removed | core report-findings | KILLED — requires a non-empty outcomeNote for every skipped outcome |
| M8 identity gate removed | core report-findings | KILLED — refuses an outcome replacement that does not match the active report |
| M9 aggregate retained-display budget removed | core compaction | KILLED — applies an aggregate budget across the list, keeping the most severe prefix |
M10 terminalSafe removed from shortSummary render |
FindingsDisplay | KILLED — renders control characters in other fields inertly |
Unmutated baselines green (cli 281/281 across the six delta suites, core suites green in the same runner). Layered-guard check: the replacement invariant is defended on four distinct surfaces (live history M3, restored history M5, daemon projection M2, rewind repair M4+M6) — each killed alone against its own surface's collocated suite, so no combination row is masked; M4 and M6 (rewind repair = AppContainer call + restore phase) are separate hunks and each independently load-bearing. Positive control: every killed row names the behavioral assertion that went red with expected-vs-actual values, in the file collocated with the mutant.
Targeted gates
| Gate | Result |
|---|---|
| core vitest: report-findings + toolResultDisplayCompaction + config (incl. headless registration) | 621/621 |
| cli vitest: FindingsDisplay, ToolMessage, daemon-tui-adapter, useHistoryManager, findings-coalescing, resumeHistoryUtils, AppContainer, commands/review/findings, run-skill-parity, i18n/index | 489/489 |
web-shell vitest client/components/messages/ |
724/724 |
npm run check-i18n |
✅ |
npm run typecheck (all workspaces incl. integration-tests) |
✅ |
eslint --max-warnings 0 on all 18 changed production files |
✅ clean; liveness proven: planted unused var → no-unused-vars error → restored byte-identical → clean |
Prettier --check on all changed files |
✅ |
Reviewer Test Plan walk-through
All plan-named unit suites ran green at the counts above (the plan's cli list maps 1:1 to the gate; utils/findings.test.ts from the plan text no longer exists — commands/review/findings.test.ts is its successor and ran). The plan's tmux E2E was substituted by the unit-level wire re-verification (h1 exact llmContent/returnDisplay values + H-3 + config gate); see Not covered.
Findings (non-blocking)
- Carried from round 2, widened: the new
FindingsDisplay.test.tsxassertions are color-sensitive — now 6/13 red underFORCE_COLOR=1. The originalrenders outcomes with the skip reasoncase is joined by the five sanitization tests this delta added: theirexpect(frame).not.toMatch(/[\u0000-\u0008\u000b-\u001f\u007f-\u009f]/)scans legitimately trip on the terminal's own\u001b[33m…color codes, and the(skipped: …)substring crosses ANSI-split JSX children. Measured: vitest exit 1,Tests 6 failed | 7 passedunderFORCE_COLOR=1 CLICOLOR_FORCE=1(capture06-force-color-sensitive-assertions.png); 13/13 green colorless, so CI and production are unaffected — this is a test-portability defect only. Measured suggested fix (applied in a scratch copy, then removed): wrap the eightlastFrame()!reads instrip-ansi(already a dependency) — result 13/13 green under FORCE_COLOR=1 AND 13/13 green colorless, i.e. the fix kills the color sensitivity without weakening any assertion (the scans and substrings keep their intent once the terminal's own codes are removed). Suggestion ships with its measurement this round; the mutation matrix's M10 row pins the underlying rendering behavior in the colorless regime. - Carried, informational —
shortSummarystill has no schemamaxLengthwhile id/file/summary/failureScenario/category/outcomeNote all carry caps. The behavioral bound (compression to ≤ 60, linear: 100 × 100 KB in 72.6 ms) makes this self-describing-schema polish, not a behavior gap. Stands, agreed tradeoff. - Carried, nit —
agent-core.tsformatting-only delta (+4/−2 union rewraps, type positions only; base failsprettier --check, head passes). Inert.
Not covered
- Per-commit attribution — depth-2 checkout:
git rev-list HEAD^1..HEAD^2returns 1 reachable commit vs the snapshot's 15;git rev-parse --is-shallow-repository= true. Only the aggregateHEAD^1..HEADdiff was verified. The delta was nonetheless scoped precisely from the snapshot's commit metadata (round-5 / round-6 / merge / boundary-bypass commits) and each delta surface A/B'd. - Web Shell live consumption of
findings_list— PR-declared out of scope; only its display-name/i18n gates ran (724/724). - ACP host rendering — generic by design; not exercised.
- Full headless wire E2E (round 2's scenarios A–C over the bundled CLI + fake OpenAI server) was not re-run: its input closure was re-verified at unit level this round (exact
llmContent/returnDisplayvalues in h1, registration position H-3, config gate). Real-tmux color capture likewise not covered — severity colors reviewed in code only. - Daemon E2E against a live daemon process — the boundary was driven through the exported
reduceDaemonEventToTuiUpdateswith the exact event shapes the daemon emits (taken from the PR's own adapter tests), not over live RPC. - macOS/Windows — platform-neutral TS; CI lanes cover.
- SKILL.md model-side execution — the skill wiring is pinned by the run-skill-parity gate (green); no model ran the skill this round.
- Diagnostic runs not counted in the tally: unmutated matrix baselines (subsumed by the gates), the FORCE_COLOR red-measurement itself (finding evidence), and the fix-probe's 26 test-level passes (counted as 2 regime-level checks).
Methodology
Environment: CI verify container (node v22.23.2, node:22-bookworm lane runtime), merge-ref checkout (HEAD = 83bf118e, HEAD^1 = base tip 22bb5e8b9f, HEAD^2 = PR head 6e242c3e), npm ci + npm run build pre-run at HEAD. Base arm: scratch worktree at HEAD^1 with core rebuilt there; the root node_modules hoists ajv@6 while core compiles against nested ajv@8.20.0, so the base tree linked head's packages/core/node_modules (lockfile untouched by the PR — verified empty diff — making the nested deps identical); the head-side harnesses import explicit per-arm dist paths and assert the @qwen-code/qwen-code-core realpath resolves into the head tree (no workspace-symlink aliasing). Harnesses h1–h5, mut-runner, and the strip-ansi probe live in this artifact dir and drive compiled dist/ output, the exported daemon reducer with real event shapes, or source-level single-hunk reverts — no stubs of code under test; raw logs in logs/, six captures in evidence/. Assertion tally: h1 69 + h2 2 + h3 22 + h4 11 + h5 8 + mutations 20 (10 kills + 10 byte-identical restores) + gates 621+489+724 + repo checks 6 (check-i18n, typecheck, eslint, 2 lint-liveness, prettier) + fix-probe regime checks 2 = 1974 pass / 0 fail.
Flakiness gate log
rounds=5 files=11 skipped=0
file packages/cli/src/commands/review/findings.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/findings.test.ts
file packages/cli/src/ui/AppContainer.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/AppContainer.test.tsx
file packages/cli/src/ui/components/FindingsDisplay.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/FindingsDisplay.test.tsx
file packages/cli/src/ui/components/messages/ToolMessage.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/messages/ToolMessage.test.tsx
file packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/daemon/daemon-tui-adapter.test.ts
file packages/cli/src/ui/hooks/useHistoryManager.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useHistoryManager.test.ts
file packages/cli/src/ui/utils/findings-coalescing.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/utils/findings-coalescing.test.ts
file packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/utils/resumeHistoryUtils.test.ts
file packages/core/src/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.test.ts
file packages/core/src/tools/report-findings.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/report-findings.test.ts
file packages/core/src/utils/toolResultDisplayCompaction.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/toolResultDisplayCompaction.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/findings.test.ts: PPPPP
packages/cli/src/ui/AppContainer.test.tsx: PPPPP
packages/cli/src/ui/components/FindingsDisplay.test.tsx: PPPPP
packages/cli/src/ui/components/messages/ToolMessage.test.tsx: PPPPP
packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: PPPPP
packages/cli/src/ui/hooks/useHistoryManager.test.ts: PPPPP
packages/cli/src/ui/utils/findings-coalescing.test.ts: PPPPP
packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: PPPPP
packages/core/src/config/config.test.ts: PPPPP
packages/core/src/tools/report-findings.test.ts: PPPPP
packages/core/src/utils/toolResultDisplayCompaction.test.ts: PPPPP
verdict: pass
summary: 11 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/AppContainer.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/FindingsDisplay.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/hooks/useHistoryManager.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/utils/findings-coalescing.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: P (exit 0)
round 1 · packages/core/src/config/config.test.ts: P (exit 0)
round 1 · packages/core/src/tools/report-findings.test.ts: P (exit 0)
round 1 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/AppContainer.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/components/FindingsDisplay.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/useHistoryManager.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/utils/findings-coalescing.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: P (exit 0)
round 2 · packages/core/src/config/config.test.ts: P (exit 0)
round 2 · packages/core/src/tools/report-findings.test.ts: P (exit 0)
round 2 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/AppContainer.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/components/FindingsDisplay.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/useHistoryManager.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/utils/findings-coalescing.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: P (exit 0)
round 3 · packages/core/src/config/config.test.ts: P (exit 0)
round 3 · packages/core/src/tools/report-findings.test.ts: P (exit 0)
round 3 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/AppContainer.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/components/FindingsDisplay.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/hooks/useHistoryManager.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/utils/findings-coalescing.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: P (exit 0)
round 4 · packages/core/src/config/config.test.ts: P (exit 0)
round 4 · packages/core/src/tools/report-findings.test.ts: P (exit 0)
round 4 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/findings.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/AppContainer.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/components/FindingsDisplay.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/hooks/useHistoryManager.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/utils/findings-coalescing.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: P (exit 0)
round 5 · packages/core/src/config/config.test.ts: P (exit 0)
round 5 · packages/core/src/tools/report-findings.test.ts: P (exit 0)
round 5 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved. report_findings ships a strict typed contract: enum-validated severity/confidence/source/outcome, control-character and empty-field checks, safe-integer line guard, outcomeNote required for skipped, partial-outcome refusal and active-report identity join. One open Suggestion about scripts-test collection noted, non-blocking.
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. ✅
Post-merge Critical: commit the active report only after scheduler delivery succeedsAt exact head Concrete failure sequence:
This means the comment and state invariant that Suggested fix: commit the report identity at the scheduler's final successful-delivery point, alongside |
5 participants
What this PR does
Adds a
report_findingscore tool so/reviewhands its findings to clients as typed data instead of a Markdown convention. One call carries{level, findings[]}with field names and enum spellings matching the review findings artifact exactly (id,severity,confidence,source,file/line,summary,shortSummary,failureScenario,category,outcome,outcomeNote), so the model copies values out of the artifact instead of translating them. The tool sorts by severity → confidence → location, derives/compressesshortSummaryto ≤ 60 characters for compact list UIs, rejects control characters and duplicate ids, persists nothing, and decides no verdict. It is registered in headless sessions too (review rundepends on it) and deferred behindtool_searchlikerecord_artifact.After
--fix, the review skill re-issues the call with every finding carryingfixed/skipped/no_change_needed(plusoutcomeNote). A partial outcome set is refused, mirroringreview findings --outcomes: a fixer that applies six of nine findings and reports six has silently shortened the list. The rule outlives Step 6B — any later in-session disposition change (fix these issues, a finding established wrong) records outcomes into the artifact and re-issues the call.The TUI renders the new
findings_liststructured display as per-finding rows (severity color, artifact id,file:line, short summary,(low confidence)marker, outcome badge); the daemon TUI adapter passes it through, history/recording compaction truncates only the free-text fields, and the tool display name is covered in the CLI zh/zh-TW/ca/en locales and the Web Shell display-name map. The severity/confidence/outcome/source enums now live in core;packages/cli/src/utils/findings.tsre-exports them under its historical names (the Web Shell review renderer keeps its deliberate browser-side copy). The bundled review skill wires the calls in Step 6 (fields copied from the artifact — the artifact is the oracle), Step 3C (level: "low"for the unverified list), Step 6B, and the interactivefix these issuespath; the call is UI delivery — a failure is disclosed and never alters artifacts or the verdict, same rule asrecord_artifact.Why it's needed
/reviewalready canonicalizes findings as data twice — theqwen review findingsartifact, and the Step 8save-artifact+record_artifactdocument the Web Shell renders — but both are files registered after the fact. Every client rendering the session live (the TUI, the Web Shell transcript, ACP hosts) sees only the Markdown restatement of the same list, and after--fixnothing in-band tells a client which findings are closed. A typed in-band contract gives every client a per-finding checklist whose statuses can be trusted, without parsing prose.Reviewer Test Plan
How to verify
Unit:
packages/core—npx vitest run src/tools/report-findings.test.ts src/utils/toolResultDisplayCompaction.test.ts src/config/config.test.ts(tool validation/sorting/outcome rules, compaction, headless registration).packages/cli—npx vitest run src/ui/components/FindingsDisplay.test.tsx src/ui/daemon/daemon-tui-adapter.test.ts src/utils/findings.test.ts src/commands/review/run-skill-parity.test.ts src/i18n/index.test.ts(TUI rows, adapter passthrough, enum re-exports, SKILL parity, zh coverage).packages/web-shell—npx vitest run client/components/messages/(display-name drift + zh coverage). Changed files passeslint --max-warnings 0and Prettier;npm run check-i18npasses.End-to-end: build the bundle (
npm run bundle), point the CLI at a scripted OpenAI-compatible fake model that answers with areport_findingstool call (isolatedQWEN_HOME,security.auth.selectedType=openai, dummy key), run it in tmux, and confirm the tool result renders as the per-finding list below — then repeat with the fake model emittingoutcomeon every finding and confirm the outcome badges. Expected: severity-colored rows with id,file:line, compressed short summary; on the outcome run,(fixed)/(skipped: reason)/(no change needed)badges.Evidence (Before & After)
Before (main): findings reach live clients only as the Markdown report prose — no structured tool result, nothing machine-readable in-band, and no per-finding status after
--fix.After — first report, real TUI (bundled CLI in tmux against the scripted model):
After — outcome re-report:
Tested on
Environment (optional)
Unit tests via vitest per package; E2E via the bundled
dist/cli.jsin tmux (Linux) against a local fake OpenAI-compatible SSE server, no sandbox.Risk & Scope
packages/cli/src/utils/findings.tsfrom local definitions to core re-exports; values and types are identical and the existing findings suites pin them.findings_listdisplay (the Web Shell keeps its existing artifact-based review view; only its display-name map learned the new tool); ACP hosts render the result generically; macOS/Windows local runs (CI covers them; the change is platform-neutral TS).Linked Issues
None — implements the "typed findings contract" recommendation from the maintainer-side review-pipeline comparison notes.
中文说明
这个 PR 做了什么
新增
report_findingscore 工具,让/review以 typed 数据而非 Markdown 约定把 findings 交给客户端。一次调用携带{level, findings[]},字段名与枚举拼写与 review findings 工件完全一致(id、severity、confidence、source、file/line、summary、shortSummary、failureScenario、category、outcome、outcomeNote),模型直接从工件抄值而不是翻译。工具按 严重度 → 置信度 → 位置 排序,派生并压缩shortSummary到 ≤ 60 字符供紧凑列表 UI 使用,拒绝控制字符与重复 id,不落盘、不裁决;无头会话同样注册(review run依赖),与record_artifact一样走tool_search延迟加载。--fix之后,review skill 再调一次,每条 finding 携带fixed/skipped/no_change_needed(以及outcomeNote)。部分覆盖会被拒绝,与review findings --outcomes的完整性规则一致:修了九条中的六条却只报六条,等于悄悄缩短了清单。该规则在 Step 6B 之后长期有效——会话内任何时候 finding 处置发生变化(fix these issues、确认误报),都要把 outcome 写回工件并重发调用。TUI 将新的
findings_list结构化 display 渲染为逐条行(严重度配色、工件 id、file:line、短摘要、(low confidence)标记、outcome 徽标);daemon TUI 适配器透传,历史/录制压缩只截断自由文本字段,工具显示名覆盖 CLI 的 zh/zh-TW/ca/en locale 与 Web Shell 的显示名映射。severity/confidence/outcome/source 枚举移入 core,packages/cli/src/utils/findings.ts以原名 re-export(Web Shell 评审渲染器保留其刻意的浏览器端副本)。内置 review skill 在 Step 6(字段从工件复制——工件是 oracle)、Step 3C(未验证清单以level: "low"上报)、Step 6B 与交互式fix these issues路径接线;该调用属于 UI 交付——失败只披露、绝不改动工件或裁决,与record_artifact同一条规则。为什么需要
/review的 findings 此前已两次数据化——qwen review findings工件,以及 Step 8 由save-artifact+record_artifact注册、Web Shell 渲染的文档——但两者都是事后注册的磁盘文件。所有实时渲染会话的客户端(TUI、Web Shell 转写、ACP 宿主)看到的只是同一清单的 Markdown 复述;--fix之后也没有任何带内信号告诉客户端哪些 finding 已经关闭。带内 typed 契约让每个客户端都能得到逐条、状态可信的 findings 清单,无需解析 prose。评审验证方案
如何验证
单测:
packages/core—npx vitest run src/tools/report-findings.test.ts src/utils/toolResultDisplayCompaction.test.ts src/config/config.test.ts(工具校验/排序/outcome 规则、压缩、无头注册)。packages/cli—npx vitest run src/ui/components/FindingsDisplay.test.tsx src/ui/daemon/daemon-tui-adapter.test.ts src/utils/findings.test.ts src/commands/review/run-skill-parity.test.ts src/i18n/index.test.ts(TUI 行渲染、适配器透传、枚举 re-export、SKILL parity、zh 覆盖)。packages/web-shell—npx vitest run client/components/messages/(显示名漂移 + zh 覆盖)。改动文件通过eslint --max-warnings 0与 Prettier;npm run check-i18n通过。端到端:构建 bundle(
npm run bundle),让 CLI 指向一个以report_findings工具调用应答的脚本化 OpenAI 兼容假模型(隔离QWEN_HOME,security.auth.selectedType=openai,dummy key),在 tmux 中运行,确认工具结果渲染为下方的逐条清单;再让假模型为每条 finding 附带outcome重跑,确认 outcome 徽标。预期:严重度配色的行,含 id、file:line、压缩短摘要;outcome 轮次出现(fixed)/(skipped: 原因)/(no change needed)徽标。证据(前后对比)
之前(main):findings 只以 Markdown 报告 prose 到达实时客户端——没有结构化工具结果,带内无机器可读数据,
--fix后也没有逐条状态。之后——首报,真实 TUI(tmux 中的打包 CLI 对脚本化模型):见上方英文部分第一段 capture。
之后——outcome 二次上报:见上方英文部分第二段 capture。
测试平台
见上方表格:Linux 已验证(✅),macOS 与 Windows 未在本地验证(⚠️ ,CI 覆盖)。
环境(可选)
单测按包用 vitest;端到端在 Linux 上用打包后的
dist/cli.js+ tmux,对接本地假 OpenAI 兼容 SSE 服务,无沙箱。风险与范围
packages/cli/src/utils/findings.ts从本地定义改为 core re-export;值与类型完全一致,既有 findings 套件已锁定。findings_listdisplay 的实时消费(Web Shell 保留其基于工件的评审视图,本 PR 只补了显示名映射);ACP 宿主以通用形式渲染该结果;macOS/Windows 本地运行(CI 覆盖;改动为平台无关的 TS)。关联 Issue
无——实现维护者侧评审管线对比笔记中的「typed findings 契约」建议。