Skip to content

fix(cli): close three review-comment gaps from the #9027 CI review - #9102

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/review-settings-followups
Aug 14, 2026
Merged

fix(cli): close three review-comment gaps from the #9027 CI review#9102
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/review-settings-followups

Conversation

@wenshao

@wenshao wenshao commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Follow-ups to three Suggestion-level findings from #9027's CI review, all landed on files that belonged to the now-merged #8994:

  • Regime-aware refusal wording. When review.comment authorises the run and the args record is missing, the refusal no longer says "cannot show that --comment was requested" — the real blocker is that no recorded invocation names a pull request to bind the write to, and a plain re-run of the review fixes it, while typing --comment does not. Flag-driven operators keep the flag wording, and the refusal-advice selection routes the new message to the binding remedy.
  • A handler-path refusal test for the operator-scope invariant. The publish-assets suite previously pinned the skipWorkspaceSettings flag only through a success case whose mock returned the same value as the polluted view. The new test runs the handler with the setting off and no --comment in the recorded arguments, expecting exit code 3 and no gh call; dropping the scope flag turns the refusal into a publish (mutation-verified).
  • Symmetric pins for the flag branch of the forcing ternaries. A flag-forced high effort now has positive and negative assertions on both the forcing warning and the resolution text, so collapsing either ternary to the setting-only message reddens the suite (mutation-verified).

Why it's needed

Each gap lets a specific misdirection or regression ship silently: an operator told to type a flag they never needed; a repository-controlled workspace settings file deciding to publish evidence for every reviewer; a warning that sends the operator hunting a setting that is off.

Reviewer Test Plan

How to verify

  • Run the review command suites (cd packages/cli && npx vitest run src/commands/review/submit.test.ts src/commands/review/publish-assets.test.ts src/commands/review/parse-args.test.ts) — the new cases pin both regimes.
  • To see the refusal wording directly: with review.comment: true in user settings and no recorded args, qwen review submit refuses naming the missing invocation rather than a missing flag.

Tested on

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

Risk & Scope

  • Main risk or tradeoff: none user-facing beyond refusal text; the only behavior change is wording in a refusal path and added test coverage.
  • Not validated / out of scope: the R2-3 linearity-guard tightening from the same review was superseded by the bounded-strip version already on main.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to review findings on #9027 (files from #8994).

中文说明

本 PR 处理 #9027 CI review 的三条 Suggestion 级发现,涉及文件均来自已合并的 #8994

  • **拒绝措辞区分授权模式。**当 review.comment 设置授权运行而 args 记录缺失时,拒绝理由不再说"无法证明请求了 --comment"——真正的阻塞是没有任何记录了 PR 的调用可供绑定写入目标,重新运行评审即可解决,键入 --comment 并不能。旗标驱动的用户保留旗标措辞,拒绝建议的选择逻辑会把新文案路由到绑定类补救措施。
  • **operator-scope 不变量的 handler 路径拒绝测试。**publish-assets 套件此前只通过一个 mock 返回值与污染视图相同的成功用例间接覆盖 skipWorkspaceSettings 旗标。新测试在设置关闭、args 无 --comment 时运行 handler,断言退出码 3 且未调用 gh;去掉该旗标会让拒绝变成发布(已变异验证)。
  • **强制提级三元表达式旗标分支的对称断言。**旗标强制 high 时,强制警告和 resolution 文案都有正负断言,折叠任一三元表达式为仅设置分支文案都会使套件变红(已变异验证)。

动机:每个缺口都让一种具体的误导或回归悄悄上线——告诉运营者键入一个他们从未需要的旗标;仓库可控的 workspace 设置文件为每个评审者决定发布证据;警告让运营者去排查一个并未开启的设置。

验证:cd packages/cli && npx vitest run src/commands/review/submit.test.ts src/commands/review/publish-assets.test.ts src/commands/review/parse-args.test.ts 全绿;三条修复均做了变异验证(去掉被钉的机制后新测试变红)。

- authorization: a missing args file no longer tells a setting-driven
  operator to re-run with --comment — with review.comment on, the refusal
  names the real blocker (no recorded invocation names a PR to bind the
  write to), and submit's advice ternary routes it to the binding remedy.
- publish-assets: a handler-path refusal test pins the operator-scope
  invariant — with the setting off and no --comment in the recorded args,
  the handler exits 3 without touching gh; dropping skipWorkspaceSettings
  reddens it (mutation-verified).
- parse-args: the flag branch of both forcing ternaries gets symmetric
  pins — a flag-forced high names the flag in the forcing warning and in
  the resolution text, never a setting that is off (mutation-verified).

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run on the four commits pushed since the last pass (4c25d406c097af) — each answers a review thread on the first commit, and all five threads are now resolved.

Template: complete — same small note as last time: no ### Evidence (Before & After) subsection; an N/A would have closed it for a wording-plus-test change. Not blocking.

Problem: real and unchanged from the last pass — the three named findings from #9027's CI review, each verified against the code. The four new commits address findings raised while reviewing the fix itself: the TS2698 cast that broke the build (R1-1), an explicit-false pin for the flag branch of the new ternary (R2-1), a contradiction between the new setting-regime refusal and the advice preamble it routes to (R3-1), and a shared handler-invocation shape so the two handler-path legs can't drift (R4-1).

Direction: aligned — unchanged from the last pass; the additional commits harden the same refusal/advice surface rather than widening scope.

Size: 30 production lines (authorization.ts 12, submit.ts 18) + 128 test lines. Not core infrastructure.

Approach: minimal — every commit maps to a named finding or review thread, no drive-by edits. The preamble change in submit.ts ("Nothing recorded authorises binding…") is the right fix for R3-1: the second advice arm now covers both a binding miss and a missing args record without presupposing recorded arguments exist, and the routing key (`--comment` was) still splits the refusal classes correctly — I checked all five refusal messages against it.

Risk: no high-risk path matches (1e).

Moving on to code review. 🔍

中文说明

本轮复审针对上次审查后新推的四个提交(4c25d406c097af)——每条提交都回应首个提交上的一条评审线程,五条线程现已全部解决。

**模板:**完整——与上轮相同的小提醒:缺少 ### Evidence (Before & After) 小节;对文案+测试类改动写 N/A 即可。不阻塞。

**问题:**真实且与上轮一致——#9027 CI review 的三条已点名发现,逐一在代码中核实成立。四个新提交处理的是评审修复本身时发现的问题:破坏构建的 TS2698 转型(R1-1)、为新三元表达式旗标分支补显式 false 钉桩(R2-1)、新的设置模式拒绝文案与其路由到的建议前缀之间的矛盾(R3-1)、以及共享 handler 调用形态以防止两条 handler 路径测试漂移(R4-1)。

**方向:**对齐——与上轮一致;新增提交加固的是同一拒绝/建议面,未扩大范围。

**规模:**30 行生产代码(authorization.ts 12 行、submit.ts 18 行)+ 128 行测试。非核心基础设施。

**方案:**最小化——每条提交对应一条已点名发现或评审线程,无顺手改动。submit.ts 的前缀改动("Nothing recorded authorises binding…")是 R3-1 的正确修复:第二条建议臂现在同时覆盖目标绑定失败与 args 记录缺失,且不再预设存在已记录的参数;路由键(`--comment` was)仍正确划分拒绝类别——已将所有五条拒绝文案逐一比对核实。

**风险:**未命中高风险路径(1e)。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 6c097af8f1822fd670cf15ed35ea956dd446f763 · re-run with @qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

The build blocker is fixed in the house style the previous pass suggested, and the follow-up commits hold up under review:

  • submit.test.tsbase is now a plain object literal; the call sites spread it without a cast, so tsc --build is clean. The added explicit-false leg closes a real hole: with only undefined exercising the flag branch, a ternary mutated to a presence check ('defaultComment' in req / !== undefined) would survive; defaultComment: false takes the flag branch and pins === true. Both production callers pass strict booleans (runSubmit's destructuring default, the handler's resolved setting), so the pin matches what production actually sends.
  • authorization.ts / submit.ts — the regime-aware catch branch and the neutral advice preamble check out end to end. The new setting-regime message avoids the `--comment` was substring, so it routes to the binding-remedy arm; the flag-regime message keeps it. The new preamble ("Nothing recorded authorises binding…") reads correctly for both legs of that arm — a binding miss and a missing args record — where the old "The recorded arguments do not bind" contradicted the missing-args why. No other code or docs match the old preamble or the old refusal string; publish-assets interpolates auth.why verbatim without routing on it.
  • publish-assets.test.ts — the new refusal leg is the mutation pin the wiring leg couldn't be: with the operator-scope mock returning {} (setting off) and the recorded args naming the target PR, dropping skipWorkspaceSettings from the handler makes the workspace-polluted view (comment: true) turn the refusal into a publish — the exit code and both gh call counts redden. The shared runHandler() removes the copy-in-two-places danger the review flagged; the yargs-facing argument shape now lives in one place.
  • parse-args.test.ts — unchanged from the reviewed first commit; both flag-branch pins verified against parse-args.ts:400-402 and :428-431.

No correctness, security, or convention issues found.

Test evidence — the PR's own CI

The build failure is gone: Test (ubuntu-latest, Node 22.x) is green on this commit. The job log shows tsc --build clean and every vitest block passing, including the 53-file / 1154-test block and the 12-file / 140-test no-AK integration batch. The macOS and Windows test legs and the CLI integration job show skipped — that is by design, not fallout: ci.yml gates them on merge_group ("Integration tests run only in the merge queue", ci.yml:1035), so on a PR event the ubuntu leg is the suite's gate. The previous pass's table read "skipped after the failure"; the skip is structural and identical across both runs.

The Qwen Code CI run is not fully settled: web-shell E2E Smoke was still in progress at review time. This PR touches no web-shell paths, but I won't predict the result.

Not verified: no separate sandboxed run — none is needed here. The only behavioural claim is the refusal wording itself, pinned by exact-string assertions that this green suite executed; the test-pinning claims are verified statically above, and CI provided the runtime evidence.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success — build + full unit suite + no-AK integration batch
Test (macos-latest, Node 22.x) ⏭️ skipped — merge_group only by design
Test (windows-latest, Node 22.x) ⏭️ skipped — merge_group only by design
Integration Tests (CLI, No Sandbox) ⏭️ skipped — merge queue only by design
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
precheck-pr / precheck ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ⏳ in progress at review time
中文说明

代码审查

构建阻塞已按上一轮建议的既有写法修复,后续提交也经得起核查:

  • submit.test.ts——base 现在是普通对象字面量,调用处直接展开、不再转型,tsc --build 干净。新增的显式 false 分支补上了真实的洞:只有 undefined 走旗标分支时,把三元表达式变异为存在性检查('defaultComment' in req / !== undefined)可以存活;defaultComment: false 走旗标分支,钉住 === true。两个生产调用点都传严格布尔(runSubmit 的解构默认值、handler 的解析后设置),钉桩与生产实际传参一致。
  • authorization.ts / submit.ts——授权模式感知的 catch 分支与中性建议前缀经端到端核实。新的设置模式文案不含 `--comment` was 子串,因此路由到绑定类补救建议臂;旗标模式文案保留该子串。新前缀("Nothing recorded authorises binding…")对该臂的两条腿——目标绑定失败与 args 记录缺失——都通顺,而旧文案 "The recorded arguments do not bind" 与缺 args 的 why 相矛盾。没有其他代码或文档匹配旧前缀或旧拒绝文案;publish-assets 仅原样嵌入 auth.why,不按内容路由。
  • publish-assets.test.ts——新的拒绝测试正是 wiring leg 无法充当的变异钉:operator-scope mock 返回 {}(设置关闭)、记录的 args 指明目标 PR 时,handler 丢掉 skipWorkspaceSettings 会让工作区污染视图(comment: true)把拒绝变成发布——退出码与两个 gh 调用计数全部变红。共享的 runHandler() 消除了评审指出的两处拷贝风险,面向 yargs 的参数形态现在只存于一处。
  • parse-args.test.ts——与已审查的首个提交一致;两条旗标分支钉桩均已对照 parse-args.ts:400-402 与 :428-431 核实。

未发现正确性、安全性或规范问题。

测试证据——PR 自己的 CI

构建失败已消除:Test (ubuntu-latest, Node 22.x) 在该提交上为绿。任务日志显示 tsc --build 无错误、所有 vitest 块通过,包括 53 文件 / 1154 测试块与 12 文件 / 140 测试的 no-AK 集成批。macOS、Windows 测试腿与 CLI 集成任务显示跳过——这是设计使然,不是失败后果:ci.yml 将其限定于 merge_group("Integration tests run only in the merge queue",ci.yml:1035),PR 事件下 ubuntu 腿即套件关卡。上一轮表格写 "skipped after the failure" 不准确——该跳过是结构性的,两轮完全相同。

Qwen Code CI 运行尚未完全结束:审查时 web-shell E2E Smoke 仍在进行。本 PR 未触及 web-shell 路径,但不预测其结果。

未验证:未另开沙箱运行——此处无需。唯一的行为主张是拒绝文案本身,已由这套绿色套件实际执行的精确字符串断言钉住;测试钉桩类主张在上文静态核实,CI 提供了运行时证据。

(CI 表格见上方标记区域。)

Qwen Code · qwen3.8-max

Reviewed at 6c097af8f1822fd670cf15ed35ea956dd446f763 · re-run with @qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — the previous blocker is fixed in the exact house style suggested, every follow-up commit maps to a resolved review thread, and the suite that previously never ran is green.

Stepping back: the shape of this PR is what makes follow-ups easy to say yes to. The three original changes were already the minimal form of the fix; the four commits since then didn't widen scope — each answers a named review finding, and the answers are the small, structural kind (a cast dropped, an explicit-false leg added, a preamble made neutral, an argument shape shared) rather than feature growth. The R3-1 fix deserves a word: the old preamble would have shipped a refusal that contradicts itself in the setting-driven missing-args case ("no review arguments were recorded…" followed by "The recorded arguments do not bind"), which is exactly the misdirection this PR exists to remove. Catching that in review of the fix, and closing it with a three-word preamble change plus a negative assertion, is the loop working as intended.

What remains is mechanical. The web-shell E2E smoke was still running at review time — this PR touches no web-shell paths, so I expect it to land green, but approval waits for CI to settle on the reviewed commit rather than a prediction. The earlier build-failure change requests are answered by this commit series.

Approval deferred until CI lands green on 6c097af8f1822fd670cf15ed35ea956dd446f763.

中文说明

置信度:5/5——上一个阻塞已按建议的既有写法修复,每条后续提交都对应一条已解决的评审线程,此前从未运行的套件现已全绿。

退一步看:这个 PR 的形态正是让跟进 PR 容易获得肯定的那种。最初的三处改动已是修复的最小形态;此后四个提交没有扩大范围——每条都回应一个已点名的评审发现,且回应都是小而结构性的(去掉转型、补显式 false 分支、前缀改为中性、参数形态共享),而非功能膨胀。R3-1 的修复值得一提:旧前缀会在设置驱动的缺 args 场景下发出自相矛盾的拒绝("no review arguments were recorded…" 之后接 "The recorded arguments do not bind"),恰是本 PR 要消除的误导。在评审修复本身时发现它,并用三个词的前缀改动加一条负断言收掉——这正是评审循环应有的工作方式。

余下是机械性的。审查时 web-shell E2E smoke 仍在运行——本 PR 未触及 web-shell 路径,预期变绿,但批准等待 CI 在被审提交上落定,而非凭预期。此前因构建失败发出的修改请求已由这一系列提交回应。

批准已推迟至 CI 在 6c097af8f1822fd670cf15ed35ea956dd446f763 上变绿后执行。

Qwen Code · qwen3.8-max

Reviewed at 6c097af8f1822fd670cf15ed35ea956dd446f763 · re-run with @qwen-code /triage

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One mechanical fix needed before this can land, @wenshao: the new test in submit.test.ts types const base = { ... } as never and then spreads it (...base), which fails tsc --build with TS2698 — so CI is red and the suite never ran on this commit. The house-style fix (used by authFor in the same file) is to keep the variable plain and cast the literal at the call site: { ...base, defaultComment: true } as never.

Everything else checks out — direction, minimal scope, advice routing, and the mutation pins are all sound. Details in my review comments above. 🙏

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not explored to full depth (tool budget reached): "This PR is a follow-up to three Suggestion-level findings…": running the 3 touched test files (npm ci in worktree fails repeatedly: ENOTEMPTY/TAR_ENTRY_ERROR during install, consistent with concurrent use of the shared wo…; "This PR is a follow-up to three Suggestion-level findings…": running the three touched test files (parse-args/publish-assets/submit) — npm ci in the worktree failed repeatedly with ENOTEMPTY/TAR_ENTRY_ERROR during extra…; "This PR is a follow-up to three Suggestion-level findings…": npm run typecheck — blocked by the same install failure (low risk: the production change is two string literals inside an existing ternary, and tests use as …; "This PR is a follow-up to three Suggestion-level findings…": none — the planned run of the three affected test files completed successfully.; "This PR is a follow-up to three Suggestion-level findings…": could not execute the three test files — the worktree's node_modules was corrupted on arrival, and three reinstall attempts failed (partial-install state, an ….

中文说明

未探索到全部深度(达到工具调用预算):"This PR is a follow-up to three Suggestion-level findings…"running the 3 touched test files (npm ci in worktree fails repeatedly: ENOTEMPTY/TAR_ENTRY_ERROR during install, consistent with concurrent use of the shared wo…"This PR is a follow-up to three Suggestion-level findings…"running the three touched test files (parse-args/publish-assets/submit) — npm ci in the worktree failed repeatedly with ENOTEMPTY/TAR_ENTRY_ERROR during extra…"This PR is a follow-up to three Suggestion-level findings…"npm run typecheck — blocked by the same install failure (low risk: the production change is two string literals inside an existing ternary, and tests use as …"This PR is a follow-up to three Suggestion-level findings…"none — the planned run of the three affected test files completed successfully."This PR is a follow-up to three Suggestion-level findings…"could not execute the three test files — the worktree's node_modules was corrupted on arrival, and three reinstall attempts failed (partial-install state, an …

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

Comment thread packages/cli/src/commands/review/submit.test.ts Outdated
Comment thread packages/cli/src/commands/review/publish-assets.test.ts
Spreading an 'as never' object literal is a tsc --build error (spread of a
non-object type), which the CI Test job caught; the cast was unnecessary —
the partial object literal typechecks against ReviewWriteRequest directly.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "This PR is a follow-up to three Suggestion-level findings…": none — all checks completed within budget.; "This PR is a follow-up to three Suggestion-level findings…": I did not execute the test suites ( vitest ) myself; verification was by source-tracing the asserted strings and harness behavior, which for wording-pinning tes…; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget..

中文说明

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

未探索到全部深度(达到工具调用预算):"This PR is a follow-up to three Suggestion-level findings…"none — all checks completed within budget."This PR is a follow-up to three Suggestion-level findings…"I did not execute the test suites ( vitest ) myself; verification was by source-tracing the asserted strings and harness behavior, which for wording-pinning tes…"You are review agent reverse-audit — Reverse audit agent…"none — all planned checks completed within budget.

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

Comment thread packages/cli/src/commands/review/submit.test.ts
…he mock comments

- The flag-wording branch of the missing-args-file refusal is now exercised
  with the explicit defaultComment:false both production callers pass — a
  presence-check mutation of the ternary no longer survives.
- The publish-assets mock-setup comments claimed this file's refusals never
  touch the loadSettings mock; the handler-path refusal test added in this
  PR does, so the comments and the guard enumeration now say so.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "PR #9102 is a follow-up to three Suggestion-level findings…": none — all planned checks completed within budget.; "PR #9102 is a follow-up to three Suggestion-level findings…": did not execute the three changed test files (review worktree has no node_modules; every asserted string was instead verified character-for-character against th….

中文说明

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

未探索到全部深度(达到工具调用预算):"PR #9102 is a follow-up to three Suggestion-level findings…"none — all planned checks completed within budget."PR #9102 is a follow-up to three Suggestion-level findings…"did not execute the three changed test files (review worktree has no node_modules; every asserted string was instead verified character-for-character against th…

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

Comment thread packages/cli/src/commands/review/lib/authorization.ts
A setting-driven missing-args refusal lands in the advice switch's else arm,
whose preamble 'The recorded arguments do not bind this target' presupposed
recorded arguments exist — contradicting the refusal's own 'no review
arguments were recorded'. The preamble is now neutral ('Nothing recorded
authorises binding this target'), covering both a binding miss and a missing
record; the remediation half was already correct.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "PR #9102 is a follow-up to three Suggestion-level findings…": none — all checks above completed within budget..

中文说明

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

未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…"none — all checks above completed within budget."PR #9102 is a follow-up to three Suggestion-level findings…"none — all checks above completed within budget.

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

Comment thread packages/cli/src/commands/review/publish-assets.test.ts Outdated
… legs

The refusal leg hand-copied the wiring leg's 9-line handler argument
literal; a one-leg-only key rename (silenced by the as-never cast) would
leave the suite green while the handler refused for the missing-args class
instead of the documented one. Both legs now call a shared runHandler()
helper, so that drift is structurally impossible.
@wenshao

wenshao commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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: 50 passed · 0 failed · 50 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:50 通过 · 0 失败 · 50 总计

Verification report

PR 9102 — fix(cli): close three review-comment gaps from the #9027 CI review

Verdict: merge-ready — 50/50 scripted assertions passed, 0 unexpected failures. Verified head 6c097af8f1822fd670cf15ed35ea956dd446f763 (merge f27c3815, base tip 8e0033d6).

中文摘要
  • 结论: merge-ready。50/50 脚本化断言通过,0 个意外失败。
  • A/B 结论(见 "Central claim" 表与 01-ab-refusal-wording-head-vs-base.png):设置授权(review.comment: true)且 args 记录缺失时,head 的拒绝文案指明"没有任何记录在案的调用命名 PR",建议走中性的绑定补救;base 则把设置驱动的运营者引向 --comment(端到端经真实 yargs handler + 真实用户 settings 文件复现,base 的 stderr 含 Re-run with --comment,head 为 0 次)。旗标授权措辞在两个 build 间逐字节不变。
  • 变异矩阵 6/6 杀死(见 02-mutation-matrix-six-of-six-killed.png):回退措辞三元、存在性检查变异、建议前言回退、skipWorkspaceSettings 丢弃、两个强制提级三元折叠,均被 PR 声称钉住的断言精确杀死;未变异对照全绿。
  • :Reviewer Test Plan 三套件 180/180;整个 review 目录 2496 通过 / 4 跳过 / 0 失败;仓库 typecheck 通过。
  • Findings:无阻塞项。
  • 未覆盖:逐 commit 归因(shallow depth-2,元数据 5 个 commit 本地仅 1 个可达,只验证聚合 diff);编译后 bundle 路径(用 tsx 驱动同源 TS + 真实 handler,逻辑同源);Windows/macOS(纯字符串逻辑,风险低)。

Scope

  • Central claim: regime-aware refusal wording — with review.comment authorising and no args record, the refusal names the missing invocation ("no recorded invocation names a pull request to bind this write to") instead of telling a setting-driven operator to type --comment; flag-driven operators keep the flag wording; and runSubmit's advice ternary routes the new refusal to the binding remedy with a non-contradictory preamble ("Nothing recorded authorises binding this target" instead of "The recorded arguments do not bind", which presupposed recorded arguments that a missing-args refusal says do not exist).
  • Secondary 1: the new handler-path refusal test in publish-assets.test.ts pins the operator-scope invariant (skipWorkspaceSettings on the handler's settings read).
  • Secondary 2: the new/extended parse-args.test.ts assertions pin the flag branch of both forcing ternaries symmetrically.

Central claim — A/B (runSubmit level, real stderr, no mocks)

Harness run-ab.mjs + ab-cell.ts drives the real runSubmit from each tree (head = repo, base = scratch worktree at 8e0033d6); the refusal path returns before any gh call, so nothing is mocked. Witness: evidence/01-ab-refusal-wording-head-vs-base.png (34/34).

cell regime base (8e0033d) head (6c097af) flip?
S1 setting on, args file missing cannot show that --comment was requested + advice Re-run with --comment (the misdirection) no recorded invocation names a pull request + Nothing recorded authorises binding; Re-run with --comment absent yes
S2 flag (option omitted → false), args missing flag wording + flag advice identical no (invariant)
S3 flag (explicit false, as both callers pass), args missing flag wording + flag advice identical no (invariant)
S4 binding miss (args name #9999, target #123), setting on The recorded arguments do not bind this target Nothing recorded authorises binding this target yes (preamble neutralised)

All cells exit 3 on both builds. The S1 base cell is the control proving the bug the PR fixes: a setting-driven operator was advised to re-run with a flag that would not have helped; reproduced end-to-end through the real yargs handler with a real user settings file (review.comment: true in a scratch QWEN_HOME) — handler-e2e.ts + assert-handler-e2e.mjs, 6/6, logs in logs/handler-e2e-*.log:

HEAD: REFUSED … so no recorded invocation names a pull request to bind this write to — re-run the review naming the pull request.
      advice: Nothing recorded authorises binding this target …   (0× "Re-run with `--comment`")
BASE: REFUSED … so this run cannot show that `--comment` was requested.
      advice: … Re-run with `--comment` …                          (the misdirection)

Reviewer Test Plan walk-through

step result
1. npx vitest run src/commands/review/submit.test.ts src/commands/review/publish-assets.test.ts src/commands/review/parse-args.test.ts ✅ 180/180 (65 + 36 + 79)
2. with review.comment: true in user settings and no recorded args, submit refuses naming the missing invocation ✅ stronger form: real handler + real settings file, head vs base (table above)

Mutation matrix (vacuity + the PR's mutation-verified claims)

Witness: evidence/02-mutation-matrix-six-of-six-killed.png. Unmutated control green (180/180) in the same run.

mutation suite result killed by
M1 revert central wording ternary to base's single string submit KILLED 2/65 red a missing args file names the missing invocation… (Expected "no recorded invocation names a pull request", Received the base string) + the advice test
M2 presence-check === true!== undefined submit KILLED 1/65 red the explicit-false leg (byFlagExplicit expects the flag wording)
M3 advice preamble reverted to "The recorded arguments do not bind" submit KILLED 1/65 red matches the refusal advice to the refusal class (toContain('Nothing recorded authorises binding'))
M4 drop skipWorkspaceSettings from operatorReviewSettings publish-assets KILLED 1/36 red the new handler-path refusal test (expected undefined to be 3 — refusal became a publish)
M5 collapse forcing-warning ternary to the setting wording parse-args KILLED 1/79 red a flag-forced high names the flag in the forcing warning, not the setting
M6 collapse resolution ternary to the setting wording parse-args KILLED 1/79 red the extended --effort warning assertions

Every kill landed on the intended assertion (red test names printed in the capture; expected-vs-actual verified in the live runs behind it), not on an import/compile failure; no survivors. M1 additionally proves the new central test non-vacuous; M4 proves the new publish-assets test load-bearing for the operator-scope invariant.

Targeted gates

  • Reviewer Test Plan suites: 180 passed / 0 failed (3 files).
  • Full src/commands/review/ directory: 68 files, 2496 passed, 4 skipped, 0 failed.
  • Repo npm run typecheck: exit 0 (the gate that caught commit 2's illegal cast).

Gate liveness is proven by the matrix: six single-point mutations each reddened the targeted suite.

Findings

No blocking findings. Non-blocking observations:

  1. Premise confirmed end-to-end. The base build's misdirection is not just a code read — the real handler with a real review.comment: true settings file advised Re-run with --comment to an operator who never needed the flag (logs/handler-e2e-base.log). The PR's premise holds exactly as described.
  2. Type-boundary census of the new ternary. req.defaultComment is boolean | undefined at the interface; both production callers pass strict booleans (operatorReviewSettings().comment is review?.comment === true; runSubmit destructures defaultComment = false), so the reachable shapes are exactly true / false / undefined — all three pinned (S1, S3, S2 and the byFlagExplicit leg). No unguarded shape.

Not covered

  • Per-commit attribution. The metadata snapshot lists 5 commits; the depth-2 merge-ref checkout makes only 1 reachable (git rev-list HEAD^1..HEAD^2 = 1). The aggregate HEAD^1..HEAD diff was verified; individual commit claims (e.g. commit 4's advice-preamble rationale) were exercised only through the aggregate.
  • Compiled-bundle path. Harnesses drive the TS source via tsx (including the real yargs handler); the esbuild-bundled dist/cli.js path was not booted. The refusal logic is identical source; residual risk is wiring, which the handler-level E2E covers.
  • Base drift note. The snapshot's baseRefOid (97ec96ec) predates the local merge base (8e0033d6) — the workflow re-synced the merge ref; the verified diff is HEAD^1..HEAD as checked out.
  • Repo-wide lint/test beyond the review workspace (CI covers these).
  • Windows/macOS — Linux container only; the change is string logic with no platform surface.

Methodology

Environment: node:22-bookworm CI container, merge-ref checkout (HEAD f27c3815 = refs/pull/9102/merge, base 8e0033d6, PR head 6c097af8), npm ci + npm run build pre-run. A/B base side: scratch git worktree at 8e0033d6 with no own node_modules — bare imports walk up to the root install; readlink -f node_modules/@qwen-code/qwen-code-core = head tree's packages/core, a non-confound because the PR diff touches zero files outside packages/cli (and package.json/lockfile are untouched, LOCKFILE-CLEAN). Harnesses (ab-cell.ts, run-ab.mjs, handler-e2e.ts, assert-handler-e2e.mjs, run-matrix.mjs in this dir) drive the real runSubmit / submitCommand.handler unmocked — the refusal path returns before any gh spawn; settings come from a scratch QWEN_HOME settings.json. Mutations are single-point edits applied, tested, and reverted via git checkout (tree verified clean after). Raw per-cell logs in logs/; captures in evidence/.

Evidence images

01-ab-refusal-wording-head-vs-base

02-mutation-matrix-six-of-six-killed

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot has neither a verdict nor a deferral on 6c097af8f1822fd670cf15ed35ea956dd446f763 — no APPROVED, CHANGES_REQUESTED, or COMMENTED review of its own. A DISMISSED one does not count: dismiss_stale_reviews voids the bot's approval on every push, which is exactly when a fresh one is needed. If this re-run was meant to review or approve, it did not, and an approval left by another account is a separate vote that does not count as the bot's own.

⚠️ 机器人在 6c097af8f1822fd670cf15ed35ea956dd446f763既没有裁决也没有 defer —— 没有属于它自己的 APPROVEDCHANGES_REQUESTEDCOMMENTED 评审。DISMISSED 不算:dismiss_stale_reviews 会在每次推送时作废机器人的批准,而那恰恰是需要一次新批准的时刻。如果这次重跑本应评审或批准,那么它没有做到;而其他账号留下的批准是另一张票,不能算作机器人自己的。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅

Not explored to full depth (tool budget reached): "PR #9102 is a follow-up to three Suggestion-level findings…": could not execute the three changed test files — no node_modules in the worktree or parent checkout and a fresh monorepo install was not run; string/predicate…; "PR #9102 is a follow-up to three Suggestion-level findings…": run the four affected vitest suites — no node_modules in the review worktree and no vitest in the parent checkout, so all string/behavior verification above w…; "PR #9102 is a follow-up to three Suggestion-level findings…": none — all checks I opened were completed within budget.; "PR #9102 is a follow-up to three Suggestion-level findings…": none — all checks completed within budget..

中文说明

未发现问题。LGTM!✅

未探索到全部深度(达到工具调用预算):"PR #9102 is a follow-up to three Suggestion-level findings…"could not execute the three changed test files — no node_modules in the worktree or parent checkout and a fresh monorepo install was not run; string/predicate…"PR #9102 is a follow-up to three Suggestion-level findings…"run the four affected vitest suites — no node_modules in the review worktree and no vitest in the parent checkout, so all string/behavior verification above w…"PR #9102 is a follow-up to three Suggestion-level findings…"none — all checks I opened were completed within budget."PR #9102 is a follow-up to three Suggestion-level findings…"none — all checks completed within budget.

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

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — build-cli was skipped in CI and its suite did not run locally because packages/core dist cleanup failed with ENOTEMPTY.

中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — build-cli was skipped in CI and its suite did not run locally because packages/core dist cleanup failed with ENOTEMPTY。

— gpt-5.6-sol via Qwen Code /review (v0.21.11)

@wenshao
wenshao enabled auto-merge August 14, 2026 00:07

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved as requested.

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

Copy link
Copy Markdown
Collaborator

Released in v0.21.12.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants