feat(review): add review run — headless review with a machine-readable verdict - #7983
Conversation
…ble verdict The review pipeline already runs non-interactively: `qwen --prompt "/review …"` expands the bundled skill, launches the dimension agents, and honors the approval mode. What that path lacks is a contract. The verdict lives in the model's prose and in files whose names the caller must simply know, the exit code says nothing about the outcome, and piped stdin silently defeats slash-command detection (the runner prepends piped input, so the leading `/` is no longer first). Anyone who wants "run a review, tell me what it decided" ends up scraping a terminal. `qwen review run [target]` is that contract and nothing more. It assembles the /review invocation from typed flags (--effort, --comment), re-enters this build's own CLI in a child process with stdin closed, streams the child's progress to stderr, and then reads the verdict from the artifact compose-review wrote — the same JSON the skill treats as the verdict authority — never from anything the model printed. stdout carries only the result (human lines, or the full JSON with --json). Exit codes make the outcome scriptable without parsing: 0 = the review completed (whatever it decided), 1 = it never reached a verdict (child failure, timeout, or no composed artifact — a clean child exit without one is a run that wandered off, not an approve), 3 = completed AND --fail-on request-changes AND the event is REQUEST_CHANGES, so a CI gate can tell "blocking verdict" from "the tool broke". Artifact discovery is scoped to this run (mtime cutoff with a small slack for coarse filesystem clocks): a stale composed JSON from an earlier review says whatever THAT review decided, which is exactly the wrong thing to republish.
|
Thanks for the PR — re-run after 5 autofix rounds. Template: the body uses its own headings rather than the template's exact ones, but the content is substantively complete — what, why, how, test evidence, and the linked issue (#7981) are all there. Not blocking. Problem: this is a feature addition (part of #7981 P2-8, "first-class headless mode"), not a bug fix. The gap is real and observable: the review pipeline already runs headless via the prompt flag, but the verdict lives in model prose, the exit code is meaningless, and piped stdin silently breaks slash-command detection. Every CI consumer has been scraping a terminal to recover what the tool already knows. That's a concrete contract gap, not a theoretical concern. Direction: aligned. Headless review with a machine-readable verdict is squarely within the #7981 roadmap item. CHANGELOG: no direct reference to this command, but the headless/CI direction is well-established. Size: 1137 additions / 3 deletions across 6 files. No core module paths touched (packages/cli/src/commands/review/ only). Production logic: ~515 lines. Test code: ~616 lines. Not applicable for Stage 0. Approach: the scope is tight — a thin wrapper that assembles the /review invocation, spawns the CLI with stdin closed, polls for the composed verdict artifact (surviving the Step 9 cleanup sweep), and maps outcomes onto exit codes. Every edit in the diff serves the stated goal; no drive-by refactors or unrelated changes. The docs update is appropriate for a new user-facing command. Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献——这是经过 5 轮 autofix 后的重新审查。 模板：PR body 使用了自定义标题而非模板的精确标题，但内容实质完整——做了什么、为什么、怎么做、测试证据、关联 issue (#7981) 均有覆盖。不阻断。 问题：这是功能新增（#7981 P2-8 "一等公民 headless 模式"），不是 bug 修复。差距是真实可观测的：评审流水线已经能 headless 运行，但裁决藏在模型文字里、退出码无意义、管道 stdin 会静默破坏斜杠命令识别。所有 CI 消费者都在刮终端输出来还原工具已知的信息。这是具体的契约缺口，不是理论性问题。 方向：对齐。Headless 评审 + 机器可读裁决完全在 #7981 路线图内。 规模：6 个文件，1137 行新增 / 3 行删除。未触及核心模块路径。生产逻辑约 515 行，测试代码约 616 行。Stage 0 不适用。 方案：范围紧凑——薄包装层，组装 /review 调用、关闭 stdin 生成子进程、轮询 composed 产物（绕过 Step 9 清理）、映射退出码。diff 中每个编辑都服务于既定目标，无顺手重构或无关改动。 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal first: given "run a /review headlessly and return a machine-readable verdict with meaningful exit codes", I would: (1) add a review run subcommand under the existing review parent; (2) build the /review prompt from typed flags, validating the target against re-tokenization injection; (3) spawn the CLI's own entry point with the prompt flag, stdin closed, detached for process-group control; (4) poll for the composed verdict artifact during the child's lifetime (since Step 9 cleanup sweeps it before exit); (5) map outcomes onto 0/1/3 exit codes; (6) handle timeout with group kill and forward parent signals. Comparison with the diff: the PR's approach matches this proposal almost exactly. No simpler path was missed. Findings — no critical blockers. Specific observations:
No AGENTS.md violations found. Testing
Ubuntu unit suite green. macOS/Windows tests skipped (fork PR CI limitation). Integration tests skipped. No failures. Sandboxed verification would settle this: @qwen-code /verify — the central claim is that review run correctly captures the composed verdict before Step 9 cleanup sweeps it and maps it onto the 0/1/3 exit contract. The unit tests mock spawn and exercise the logic, but an A/B run against the base build would confirm the end-to-end spawn, poll, verdict, exit path works with a real CLI child process. This is a sponsored run (fork PR): a maintainer's @qwen-code /verify comment approves the head it was written against, and the run carries a pre-execution risk screen and full workspace wipe — read the resulting report with the same skepticism as the fork's CI logs. 中文说明代码审查独立方案：给定"headless 运行 /review 并返回机器可读裁决 + 有意义的退出码"，我会：(1) 在现有 review 父命令下添加 review run 子命令；(2) 从类型化参数构建 /review prompt，验证 target 防止重分词注入；(3) 以关闭 stdin、detached 方式生成 CLI 自身入口的子进程；(4) 在子进程存活期间轮询 composed 产物（因为 Step 9 清理会在退出前扫除它）；(5) 映射 0/1/3 退出码；(6) 超时组杀 + 父信号转发。 与 diff 对比：PR 方案与独立提案几乎完全一致。未发现更简路径。 发现——无关键阻断项：
测试Ubuntu 单元测试绿色。macOS/Windows 跳过（fork PR CI 限制）。集成测试跳过。无失败。 沙盒验证可确认：@qwen-code /verify——核心声明是 review run 在 Step 9 清理前正确捕获 composed 裁决并映射到 0/1/3 退出契约。单元测试 mock 了 spawn，A/B 运行可确认端到端路径。这是赞助运行（fork PR）：maintainer 的 @qwen-code /verify 评论批准其写入的 head，运行带有预执行风险筛查和完整工作区清除。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal wrapper that solves a real contract gap; the only reservation is that end-to-end behavior rests on mocked-spawn unit tests rather than a live CLI child. Stepping back: this PR does exactly what it says — wraps the existing headless review path in a typed contract (stdout result, stderr progress, 0/1/3 exit codes) without adding any new review logic. The implementation is 495 lines of production code, most of which is process lifecycle management (spawn, poll, timeout, signal forwarding) that genuinely needs to exist for the contract to be trustworthy. The test suite is thorough — 12+ tests covering the full contract including the two race conditions (cleanup sweep, timeout-after-verdict) that would silently break a naive implementation. Going back to my independent proposal: the PR matches it almost exactly. I didn't find a simpler path it missed. The capture-poll design is the non-obvious piece, and it's well-motivated — without it, every completed review would read as a failure because Step 9 cleanup sweeps the verdict before the child exits. Is every change necessary? Yes — the docs update documents a new user-facing command, the cli.ts/review.ts changes are minimal registration, and the two new files (run.ts, run.test.ts) are the feature itself. No scope creep. Would I maintain this in six months? Yes — the code is straightforward, the comments explain the non-obvious "why" (version-skew rule, cleanup race, EPIPE safety, process group semantics), and the test suite pins the contract clearly. The 4/5 (not 5/5) is honest: the unit tests mock spawn, so the end-to-end path (real CLI child, real compose-review, real cleanup, verdict capture) is not exercised by the suite. The /verify lane named in Stage 2 would close that gap. But the logic is sound, the mocking is careful, and the CI suite is green. Approving. ✅ 中文说明置信度：4/5 —— 干净、最小化的包装层，解决了真实的契约缺口；唯一的保留是端到端行为依赖 mock-spawn 单元测试而非真实 CLI 子进程。 退一步看：这个 PR 完全做到了它声称的——将现有 headless 评审路径包装为类型化契约（stdout 结果、stderr 进度、0/1/3 退出码），没有添加任何新的评审逻辑。实现是 495 行生产代码，大部分是进程生命周期管理（生成、轮询、超时、信号转发），这些对于契约的可信度是真正必要的。测试套件全面——12+ 个测试覆盖完整契约，包括两个竞态条件（清理扫除、超时后裁决）。 与独立提案对比：PR 几乎完全匹配。未发现更简路径。capture-poll 设计是非显而易见的关键部分。 每个改动都是必要的吗？是的——文档更新记录新的用户命令，cli.ts/review.ts 是最小注册，两个新文件是功能本身。无范围蔓延。 六个月后维护？可以——代码直白，注释解释了非显而易见的"为什么"，测试套件清晰固定契约。 4/5（而非 5/5）是诚实的：单元测试 mock 了 spawn，端到端路径未被套件执行。Stage 2 中命名的 /verify 通道可弥合该差距。但逻辑合理，mock 仔细，CI 套件绿色。 批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
| const composedPath = newestArtifactSince( | ||
| REVIEW_TMP_DIR, | ||
| /^qwen-review-.*composed\.json$/, | ||
| cutoffMs, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The artifact discovery regex matches every target's composed verdict file in the shared .qwen/tmp/ directory. Two concurrent review run invocations for different PRs in the same workspace can have their verdicts misattributed — the slower one picks up the faster one's artifact.
Concrete cost: CI pipeline reviewing two PRs in the same checkout — the CI gate records PR #A's verdict against PR #B.
Suggested fix: scope the regex to the current target using safeTarget(args.target ?? 'local').
中文说明
产物发现正则表达式匹配所有目标的 composed 裁决文件。在同一工作区并发运行两个 review run 时，较慢的调用会拾取较快调用的裁决，导致 CI 门禁将 PR #A 的裁决错误地记录到 PR #B。建议将正则表达式限定为当前目标。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Declined for this round. The {target} token in the composed filename is chosen by the skill orchestrator, not by this command: a PR review writes qwen-review-pr-<number>-composed.json and a local review writes qwen-review-local-composed.json (see SKILL.md Step 7). That token is not equal to safeTarget(args.target) — for qwen review run 7724 the run command's target is 7724 while the file is qwen-review-pr-7724-composed.json. Scoping the regex to safeTarget(args.target ?? 'local') would therefore never match a PR review's verdict and would silently make completed always false on exactly the path this command exists for.
The misattribution this targets is already mitigated: newestArtifactSince only considers files whose mtime is at or after the run's start (minus a 2s clock-slack), so a previous review's composed JSON is invisible. The residual case is two concurrent runs in the same checkout whose start times fall inside each other's slack window; a correct fix needs the run command and the skill to agree on the filename token first, so it is deferred rather than guessed here.
中文说明
本轮予以拒绝。composed 文件名中的 {target} token 由 skill 编排器决定，而非本命令：PR 评审写 qwen-review-pr-<number>-composed.json，本地评审写 qwen-review-local-composed.json（见 SKILL.md Step 7）。该 token 并不等于 safeTarget(args.target) —— 对 qwen review run 7724，run 命令的 target 是 7724，而文件是 qwen-review-pr-7724-composed.json。因此把正则限定为 safeTarget(args.target ?? 'local') 永远不会匹配到 PR 评审的裁决，会在本命令存在的核心路径上静默地使 completed 恒为 false。
该发现针对的误认问题已被缓解：newestArtifactSince 只考虑 mtime 不早于本次运行起始时间（减去 2 秒时钟余量）的文件，因此上一次评审的 composed JSON 是不可见的。残留场景是同一 checkout 中两个并发运行的起始时间互相落在对方的余量窗口内；正确的修复需要先让 run 命令与 skill 就文件名 token 达成一致，故此处推迟而非猜测。
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
QwenLM#7983) - Use writeStderrLineSafe in the timeout and spawn-error handlers and guard the progress stream, so an EPIPE on stderr can no longer skip the child kill, hang the promise, or orphan the review. - Reject a review target carrying whitespace or a leading dash before it is re-tokenized by the child CLI (e.g. `123 --comment` silently authorising posting). - Constrain --approval-mode to the same choices as the top-level CLI. - Capture the child's exit signal and surface it (OOM/SIGKILL vs spawn fail). - Sync the top-level `qwen --help` review description with the command. - Register `run` in the review.test.ts subcommand expectation and add tests for the timeout branch, the readComposed guard, and target rejection.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #7983Addressed all three Critical findings and five of the six Suggestions; declined Critical
Suggestions
Verification
中文说明Autofix 评审轮次 — PR #7983已处理全部三个 Critical 发现以及六个 Suggestion 中的五个；对一个 Suggestion 给出记录在案的理由后予以拒绝。无需 base 冲突合并（ Critical
Suggestions
验证
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/模型 |
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.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
Local verification report — real build, real CLI, real
|
review run <target> |
result |
|---|---|
7724 --effort high |
<skill-args>7724 --effort high</skill-args> |
7724 --effort high --comment |
<skill-args>7724 --effort high --comment</skill-args> |
"123 --comment" |
refused, exit 1, 0 model requests — the child is never launched |
"123 --comment", guard neutralised |
<skill-args>123 --comment</skill-args> — posting authorised by an argument nobody passed |
5. Everything else
--jsonstdout parses as JSON with the child's progress on stderr; without--json, stdout is the verdict line + report path.packages/cli/src/commands/review/run.test.ts: 15 passed. Wholesrc/commands/reviewsuite: 40 files / 1013 tests passed (--retry=0).eslint --max-warnings 0clean on the four touched source files;tsc --noEmit -p packages/cliclean.
Blocker: --timeout-minutes neither stops the review nor bounds the command
gemini.tsx always calls relaunchAppInChildProcess() with stdio: 'inherit' ("so we always have a child process that can be internally restarted"). So the process review run spawns is a thin relaunch wrapper; the actual review is its child:
node dist/cli.js review run --timeout-minutes 0.08 <- this command
└─ node dist/cli.js --prompt /review … <- relaunch wrapper (the only pid child.kill() reaches)
└─ node dist/cli.js --prompt /review … <- the real review
child.kill('SIGTERM') kills the wrapper. The review is reparented to PPID 1 and keeps running — and it still holds the stdout/stderr pipes this command created, so 'close' does not fire either. The setTimeout(() => child.kill('SIGKILL'), 10_000) escalation is a no-op: it re-signals a pid that is already dead.
Measured with --timeout-minutes 0.08 (4.8 s) and a model scripted to answer 12 s late:
review runprintedtimeout … — terminating the review, reported"timedOut": true, "childSignal": "SIGTERM"— and returned after 13.4 s.- The "terminated" review went on to receive the model's answer and execute a
run_shell_command, which left a timestamped marker on disk after the run had reported itself dead. - With a model that never answers at all, the same 4.8 s timeout returned after 484 s.
Both halves of the flag's purpose fail: the wandering run is not stopped (it keeps burning model API calls — exactly what the code comment says must not happen), and review run's own wall clock is unbounded, which is the property a CI job actually depends on.
Two fixes, both verified locally on this build:
-
Kill the process group (preferred — keeps the relaunch, which carries the
--max-old-space-sizeargs):const child = spawn(process.execPath, [...], { stdio: ['ignore','pipe','pipe'], detached: true }); // on timeout: process.kill(-child.pid, 'SIGTERM'); setTimeout(() => { try { process.kill(-child.pid, 'SIGKILL'); } catch {} }, 10_000).unref();
Measured:
'close'fires at 4836 ms for a 4800 ms timeout, no survivors, no post-timeout tool execution. -
Suppress the relaunch in the child env —
QWEN_CODE_NO_RELAUNCH: 'true'— so there is a single pid forchild.kill()to reach. Measured: one process, SIGTERM stops it, no post-timeout tool execution. Simpler, but the child then loses the memory-args relaunch.
A belt-and-braces third option: resolve the outcome promise on 'exit' rather than 'close', so a lingering grandchild can never hold the command open regardless.
Suggestion: artifact discovery is scoped by mtime, but not by target
newestArtifactSince(REVIEW_TMP_DIR, /^qwen-review-.*composed\.json$/, cutoff) accepts any target's artifact. Reproduced: review run 7724 where the run wrote qwen-review-pr-999-composed.json republished that file as 7724's verdict and exited 0.
The mtime cutoff already handles the "last week's review" case; this is the "two reviews sharing one checkout" case (and the "the model composed for the wrong target" case). review run knows its target, so the pattern could be narrowed to it — falling back to the wide pattern only when the target is unknown, since the skill's own suffix (pr-<n> / local / filename) is derived, not passed in.
Nothing else blocking from my side — the design is right and the guards that matter are real. Happy to approve once the timeout actually terminates the review.
中文版
本地验证报告 —— 真实构建、真实 CLI、真实 /review skill
我在本地把这个 PR 完整跑了一遍。环境：PR HEAD（ea935cc3，相对 main 2 个非 merge 提交，5 个文件，+646/−3）的独立 worktree，真实 npm ci + bundle，隔离的 HOME，以及一个 OpenAI 兼容的 mock 端点——子进程评审是真实的 CLI 跑真实的 bundled /review skill，只是模型回合是确定性的。下面所有 composed 裁决都由真实的 qwen review compose-review 写出，没有一处是手写的。
结论：契约成立，但 --timeout-minutes 有一个阻断性缺陷。
1. 动机中的 bug 真实存在，关闭 stdin 确实修好了它
PR 说管道 stdin 会破坏斜杠命令识别。确实如此——直接在网络层记录模型实际收到了什么（截图 1）：同一构建、同一 prompt、三种调用方式。stdin 是管道时，模型收到的是 13,287 字符的散文——字面就是 "please review\n\n\n/review --effort low"，而不是 200,442 字符的展开 skill。review run 在同样的管道下仍然拿到 200,560 字符：stdio[0] = 'ignore' 正如头部注释所说地在起作用。
2. 退出码契约，端到端
六个用例全部符合文档（截图 2）。两点值得单独说：
COMMENT+--fail-on request-changes→ 0：门禁看的是 event，不是"有没有发现问题"。- 子进程干净退出但没有 composed 产物 → 1，即使带
--fail-on：「工具坏了」永远不会被读成 approve。用一个只回「The changes look fine to me.」就停的模型验证过。
退出码 3 能穿过整条启动链（scripts/cli-entry.js → spawnSync → process.exit(status)），CI 里 if [ $? -eq 3 ] 如宣称般可用。
被重新发布的字段是真的：event、verdictLine、baseEvent、cappedBy 逐字来自 compose-review 落盘的产物，其中包含一个真正未被 cap 的 REQUEST_CHANGES（用确定性的 [build] body Critical 构造，它绕开验证 cap）。
3. mtime 界限是承重的 —— 同一构建上的 A/B
我预置了一个 10 分钟前、写着 Verdict: Approve 的 qwen-review-local-composed.json，然后让本次运行的模型跑偏、什么都不写（截图 3）。
把 dist/chunks/ 里那一行 if (mtime < startMs) continue; 置空，结果就从 exit 1、无裁决 翻转为 exit 0、"event": "APPROVE"——上周的批准被当成本次结果重新发布。这道防线是「跑偏的运行」和「CI 绿灯」之间唯一的东西。
4. target 校验同样是承重的
review run <target> |
结果 |
|---|---|
7724 --effort high |
<skill-args>7724 --effort high</skill-args> |
7724 --effort high --comment |
<skill-args>7724 --effort high --comment</skill-args> |
"123 --comment" |
拒绝，exit 1，0 次模型请求——子进程根本没启动 |
"123 --comment"，校验被置空 |
<skill-args>123 --comment</skill-args>——没人传过的参数授权了发帖 |
5. 其他
--json时 stdout 可被 JSON 解析，子进程进度在 stderr；不带--json时 stdout 是裁决行 + 报告路径。run.test.ts：15 通过；整个src/commands/review套件：40 文件 / 1013 测试通过（--retry=0）。- 四个改动源文件
eslint --max-warnings 0干净；tsc --noEmit -p packages/cli干净。
阻断问题：--timeout-minutes 既不终止评审，也不约束命令时长
gemini.tsx 总是以 stdio: 'inherit' 调用 relaunchAppInChildProcess()（「这样我们总有一个可以被内部重启的子进程」）。于是 review run 拉起的那个进程只是一层重启包装，真正的评审是它的子进程：
node dist/cli.js review run --timeout-minutes 0.08 <- 本命令
└─ node dist/cli.js --prompt /review … <- 重启包装（child.kill() 唯一能触到的 pid）
└─ node dist/cli.js --prompt /review … <- 真正的评审
child.kill('SIGTERM') 杀掉的是包装层。评审进程被 reparent 到 PPID 1 并继续运行——而且它仍持有本命令创建的 stdout/stderr 管道，所以 'close' 也不会触发。setTimeout(() => child.kill('SIGKILL'), 10_000) 这一层升级是空转：它对着一个已经死掉的 pid 再发一次信号。
用 --timeout-minutes 0.08（4.8 秒）+ 一个延迟 12 秒才回答的模型实测（截图 4）：
review run打印了timeout … — terminating the review，报告"timedOut": true, "childSignal": "SIGTERM"，然后在 13.4 秒后才返回。- 那个"已被终止"的评审继续收到了模型回复，并执行了一次
run_shell_command——在本次运行已宣告自己死亡之后，往磁盘上留下了带时间戳的标记文件。 - 换成永不回答的模型，同样 4.8 秒的超时在 484 秒后才返回。
这个 flag 的两半用途都落空了：跑飞的评审没有被停下（继续烧模型 API 调用——正是代码注释说必须避免的），而 review run 自身的墙钟时间不受约束——恰恰是 CI 任务真正依赖的那个性质。
两种修法，均已在本构建上验证：
-
杀进程组（推荐——保留 relaunch，它携带
--max-old-space-size参数）：const child = spawn(process.execPath, [...], { stdio: ['ignore','pipe','pipe'], detached: true }); // 超时时： process.kill(-child.pid, 'SIGTERM'); setTimeout(() => { try { process.kill(-child.pid, 'SIGKILL'); } catch {} }, 10_000).unref();
实测：4800 ms 的超时下
'close'在 4836 ms 触发，无残留进程，超时后没有工具执行。 -
抑制 relaunch：子进程环境里设
QWEN_CODE_NO_RELAUNCH: 'true'，这样只有一个 pid 给child.kill()触达。实测：单进程，SIGTERM 能停住，超时后没有工具执行。更简单，但子进程会失去 memory-args 重启。
第三个可叠加的保险：把结果 promise 改为在 'exit' 而非 'close' 上 resolve，这样无论如何都不会有孙进程把命令挂住。
建议：产物发现按 mtime 划界，但没有按 target 划界
newestArtifactSince(REVIEW_TMP_DIR, /^qwen-review-.*composed\.json$/, cutoff) 接受任意 target 的产物。已复现：review run 7724 的运行写出了 qwen-review-pr-999-composed.json，结果那份文件被当作 7724 的裁决发布，退出码 0。
mtime 界限已经覆盖了「上周那次评审」的情形；这里是「两次评审共用一个 checkout」以及「模型给错 target 组装了裁决」的情形。review run 知道自己的 target，可以把 pattern 收窄到它——只在 target 未知时退回宽 pattern，因为 skill 内部的后缀（pr-<n> / local / 文件名）是推导出来的、不是传入的。
除此之外我这边没有别的阻断点——设计方向是对的，关键的几道防线都是真的在起作用。等超时真的能终止评审，我就批准。
…retrying The review-address push is one-shot: when anything pushes to the PR head during the agent's ~50-minute window, the final push dies 'fetch first' and the entire verified agent run is discarded. The per-PR head-write concurrency group cannot prevent this — it serialises this repo's workflows, not the PR author or the fork side. Observed twice in one day (#7983 after a 56-minute run, #7985 after 43 minutes). On rejection, fetch the moved head, merge it into the local line, and retry (bounded at 3 attempts). Merge rather than rebase: the agent's own conflict-resolution rounds create merge commits, and a rebase would flatten them and can silently re-introduce the conflicts they resolved. The merge result descends from the remote head, so the retried push is a fast-forward and rewrites nothing. A genuine content conflict aborts the merge and falls through to the existing failure path unchanged. When a salvage merge happened, the round report discloses that the round's verification predates the merge so mid-run commits get re-checked by a human.
…QwenLM#7983) The CLI relaunches itself in a child process (for --max-old-space-size), so child.kill() only reached the relaunch wrapper — the real review was reparented to PID 1 and kept burning API calls. Spawn with detached:true and kill the process group (-pid) so the timeout actually terminates the review. Also: clamp negative --timeout-minutes to a 1-minute floor, distinguish a corrupt composed artifact from a missing one in human-readable output, and add test coverage for the default (non-JSON) output path.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round summary (verification repair)The previous commit ( Fix: replaced the annotation with Inline: rc:3672648644 — human-readable output path has zero coverageDecision: Implemented (previous commit, re-verified). The test Inline: rc:3672648652 — negative
|
Review:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedAddressed all seven inline findings and every point in @wenshao's issue-level review. The centerpiece is the Critical: a review that completed was reported as failed because its verdict artifact was deleted before the parent read it. One commit: Critical
Suggestions (all implemented)
Issue-level review (@wenshao)
Review verdict (@qwen-code-ci-bot)
Verification
中文说明已处理的评审反馈处理了全部七条行内发现，以及 @wenshao 在 issue 级评审中的每一点。核心是那个 Critical：一次实际完成的评审被报告为失败，因为其裁决产物在父进程读取之前就被删掉了。单次提交： Critical（阻断）
Suggestion（建议，全部实现）
issue 级评审（@wenshao）
评审核验结论（@qwen-code-ci-bot）
验证（Verification）
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #7983All four inline findings are addressed in code (commit Findings
Conflict notesNone — Verification
中文说明Autofix 审查轮次 — PR #7983四条行内发现均已在代码中处理（提交 发现
冲突说明无 — 验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedOne inline suggestion was raised this round; it is a genuine correctness defect [rc:3677334987]
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededNo new actionable feedback was found for PR #7983 in this round.
The only feedback present is in the Deferred non-Critical feedback section (critical-only mode is active after 5 change-producing rounds). Per policy, those items remain open for human follow-up and are not acted on by this bot. 中文说明Autofix 审查轮次 — 无需操作本轮未在 PR #7983 中发现新的可操作反馈。
唯一的反馈位于 已延迟的非 Critical 反馈 部分（经过 5 个产生改动的轮次后，已进入仅处理 Critical 的模式）。按照策略，这些条目保持开放，留待人工跟进，本机器人不会对其执行操作。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. Any items listed below stay open for human follow-up; do not modify code, resolve threads, or reply on their behalf.
中文说明完成 5 个产生改动的轮次后，进入仅处理 Critical 的模式。以上内容保持开放，留待人工跟进；不要为其修改代码、解决线程或代为回复。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — capture-poll design is the key insight, exit code contract is clean (0=completed, 1=no verdict, 3=fail-on match, avoiding yargs/shell collisions). EPIPE resilience, timeout floor, signal forwarding, and Windows taskkill all solid.
Non-blocking: validate effort against EFFORT_LEVELS inside the exported buildReviewPrompt to close the programmatic-caller gap.
…retrying The review-address push is one-shot: when anything pushes to the PR head during the agent's ~50-minute window, the final push dies 'fetch first' and the entire verified agent run is discarded. The per-PR head-write concurrency group cannot prevent this — it serialises this repo's workflows, not the PR author or the fork side. Observed twice in one day (#7983 after a 56-minute run, #7985 after 43 minutes). On rejection, fetch the moved head, merge it into the local line, and retry (bounded at 3 attempts). Merge rather than rebase: the agent's own conflict-resolution rounds create merge commits, and a rebase would flatten them and can silently re-introduce the conflicts they resolved. The merge result descends from the remote head, so the retried push is a fast-forward and rewrites nothing. A genuine content conflict aborts the merge and falls through to the existing failure path unchanged. When a salvage merge happened, the round report discloses that the round's verification predates the merge so mid-run commits get re-checked by a human.
…retrying (QwenLM#8042) * fix(autofix): salvage race-lost pushes by merging the moved head and retrying The review-address push is one-shot: when anything pushes to the PR head during the agent's ~50-minute window, the final push dies 'fetch first' and the entire verified agent run is discarded. The per-PR head-write concurrency group cannot prevent this — it serialises this repo's workflows, not the PR author or the fork side. Observed twice in one day (QwenLM#7983 after a 56-minute run, QwenLM#7985 after 43 minutes). On rejection, fetch the moved head, merge it into the local line, and retry (bounded at 3 attempts). Merge rather than rebase: the agent's own conflict-resolution rounds create merge commits, and a rebase would flatten them and can silently re-introduce the conflicts they resolved. The merge result descends from the remote head, so the retried push is a fast-forward and rewrites nothing. A genuine content conflict aborts the merge and falls through to the existing failure path unchanged. When a salvage merge happened, the round report discloses that the round's verification predates the merge so mid-run commits get re-checked by a human. * fix(autofix): address salvage-loop review findings - Gate the PUSH_RACE_MERGED disclosure on HEAD actually advancing: a transient push failure (upload timeout, 503) on an unmoved branch no-ops the merge ('Already up to date') and must not tell the reviewer to re-check mid-run commits that never existed. - Annotate the salvage fetch failure with ::error:: like the two adjacent failure paths, so a deleted fork branch or network error does not kill the step with an unannotated exit 128 under bash -e. - Re-pin the same-repo push URL construction in tests: it lost its old 'origin "${BRANCH}"' pin in this rework, leaving a ${REPO}→${HEAD_REPO} mutation (malformed remote in the same-repo case) unkillable. * test(autofix): restore dropped mutation-killing pins and add structural assertions (QwenLM#8042) * test(autofix): pin exit 1 in the give-up guard regex to kill the deletion mutation (QwenLM#8042) * test(autofix): pin exit 1 in the fetch-failure and merge-conflict salvage paths (QwenLM#8042) * test(autofix): strengthen salvage-test pins to kill init-value and capture-order mutations (QwenLM#8042) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
Released in v0.21.2. |
5 participants
Part of #7981 (item P2-8, "first-class headless mode").
What
qwen review run [target]— run a full/reviewnon-interactively and get the verdict back as a contract: machine-readable output on stdout, progress on stderr, and exit codes a CI gate can branch on.Why
The review pipeline already runs headless —
qwen --prompt "/review …"expands the bundled skill, launches the dimension agents, and honors the approval mode. What that path lacks is a contract:/is no longer the first character and the skill never expands).Every consumer that wants "run a review, tell me what it decided" — benchmarks, CI, cron — has been re-deriving those facts by scraping a terminal.
How
review runis a thin wrapper and nothing more:/reviewinvocation from typed flags (--effort,--comment), so callers never hand-type prompt strings;process.execPath+ the running entry — the same version-skew rule the skill's subprocesses follow viaQWEN_CODE_CLI), with stdin closed so piped input cannot break slash detection;--quiet); stdout carries only the result (--jsonfor the full object);compose-reviewwrote — the same JSON the skill treats as the verdict authority — never from anything the model printed. Artifact discovery is scoped to this run by an mtime cutoff (with slack for coarse filesystem clocks), so a stale composed JSON from an earlier review can never be republished as this run's outcome;--timeout-minutes(default 120) terminates a wandering run;--approval-modedefaults toyolobecause a headless run cannot answer confirmation prompts (anything unapproved would be auto-denied mid-review) — overridable.Exit codes:
0= the review completed (whatever it decided);1= it never reached a verdict (child failure, timeout, or no composed artifact — a clean child exit without one is a run that wandered off, not an approve);3= completed AND--fail-on request-changesAND the event isREQUEST_CHANGES. 3 rather than 2, so "the review is blocking" is distinguishable from yargs usage errors and shell-reserved codes without parsing anything.Tests
run.test.tspins the contract: prompt assembly; artifact discovery (stale artifacts ignored, newest-of-this-run wins, missing directory tolerated); the 0/1/3 exit split including "incomplete run is 1 even under--fail-on"; spawn wiring (stdin closed,--promptpassed, approval mode threaded); and the wandered-off case (clean child exit, no composed artifact → failure). 12 tests, all green; eslint--max-warnings 0clean; tsc clean for the touched files.中文说明
关联 #7981(P2-8 "一等公民 headless 模式")。
做了什么
qwen review run [target]—— 非交互地执行完整/review,并以契约形式返回结果:stdout 输出机器可读裁决、stderr 输出进度、退出码可直接供 CI 分支判断。为什么
评审流水线本来就能 headless 跑——
qwen --prompt "/review …"会展开 bundled skill、启动各维度 agent、遵循审批模式。缺的是契约:裁决藏在模型的文字叙述和调用方必须"恰好知道"文件名的产物里;进程退出码不反映评审结果;管道 stdin 会静默破坏斜杠命令识别(runner 会把管道输入拼在前面,/不再是第一个字符,skill 不会展开)。所有想要"跑一次评审,告诉我结论"的消费者(基准测试、CI、定时任务)都在靠刮终端输出自行还原这些事实。怎么做
review run是一层薄包装,仅此而已:--effort、--comment)组装/review调用,调用方无需手拼 prompt;process.execPath+ 正在运行的入口——与 skill 子进程通过QWEN_CODE_CLI遵循的同一条"防版本漂移"规则),并关闭 stdin,管道输入无法破坏斜杠识别;--quiet可静音);stdout 只承载结果(--json输出完整对象);compose-review落盘的产物——即 skill 视为裁决权威的那份 JSON——绝不读模型打印的内容。产物发现以本次运行的 mtime 为界(对粗粒度文件系统时钟留余量),上一次评审的陈旧 composed JSON 绝不会被当作本次结果重新发布;--timeout-minutes(默认 120)终止跑飞的评审;--approval-mode默认yolo(headless 无法应答确认弹窗,未预批准的工具会在评审中途被自动拒绝),可覆盖。退出码:
0= 评审完成(无论结论);1= 未达成裁决(子进程失败、超时、或无 composed 产物——子进程干净退出但没有产物属于"跑偏",不是 approve);3= 完成且--fail-on request-changes且事件为REQUEST_CHANGES。用 3 而非 2,使"评审给出阻断结论"与 yargs 用法错误、shell 保留码可区分,无需解析任何输出。测试
run.test.ts固定契约:prompt 组装;产物发现(忽略陈旧产物、取本次最新、目录缺失容忍);0/1/3 退出码划分(含"未完成时即使--fail-on命中也返回 1");spawn 接线(stdin 关闭、--prompt传入、审批模式透传);以及跑偏场景(子进程干净退出但无 composed 产物 → 失败)。12 个测试全绿;eslint--max-warnings 0干净;涉及文件 tsc 干净。