feat(review): dispatch Step 3A's fan-out from a generated workflow script, and route to it - #8943
feat(review): dispatch Step 3A's fan-out from a generated workflow script, and route to it#8943qqqys wants to merge 24 commits into
Conversation
|
Closing for now — parking this rather than abandoning it. The branch is preserved and this can be reopened as-is. |
…ript Supersedes QwenLM#8846, whose handoff layer did not work: it wrote the script to an arbitrary `--out` directory and told the caller to pass `args: <path>`. Neither is a form the Workflow tool accepts. `readWorkflowFileSecurely` realpaths `scriptPath` and refuses anything outside the saved-workflow directories, and `args` is inline JSON with no path form. Both were verified against the tool's own contract this time, not just the sandbox's. The sandbox has no filesystem — its globals are agent/parallel/pipeline/ phase/log/console/args/budget/workflow and nothing that opens a file — so a roster carried in `args` is a roster the model has to retype into its tool call. That is the failure this change exists to remove, reintroduced one layer up. The roster is therefore baked into the generated script, and the model's call carries one path and no payload. `--roster` and `emit-workflow` still build the same prompts from the same plan through the same `buildLaunch`. What differs is who launches them: `--roster` asks the orchestrator to copy ~13 blocks into agent calls, in one response, unedited; this writes them into a file the runtime reads. The generated file is a fixed body plus one JSON literal. No logic is generated, so the part that can be wrong is the part the tests execute — and they execute the real output of the generator, including a roster with backticks, `${`, backslashes and newlines in the prompts. Not a one-variable change, and the difference is now handled rather than claimed away: workflow dispatch substitutes its own terse subagent persona unless an agentType is given, so the script passes `agentType: 'general-purpose'` — the subagent type SKILL.md requires of the hand-launched path. Otherwise the two paths would run different agents over identical prompts and an A/B between them would not mean anything. Also from the QwenLM#8846 review: - Prompts are recorded. `check-coverage` compares each launch against what the CLI recorded handing out, so without this the whole roster read as unlaunched. - The workflows gate is checked before anything is written, so a run that cannot execute what this emits leaves no script and no prompt records implying it did. - A fan-out where every agent failed throws instead of returning a value. Returned, it would let the caller aggregate over a diff no agent read. - Cleanup sweeps the generated script. It has to live in the user's saved-workflow dir, where every file is a `/<name>` slash command, so a review that left one behind would hand the user a permanent command for a diff that no longer exists. - The dead `label` / `version` / `mode` / `plan` fields are gone with the args file, and the unreachable chunk guard with them. - `readPlanReport` is one definition in lib/report.ts rather than a sixth copy of the try/parse/rethrow block; moving the existing five onto it is mechanical and left to its own change. Still refused, unchanged: a territory fan-out (3B), whose chunk agents carry a per-chunk contract this script does not express, and a worktree review, which needs every agent pinned to the PR worktree. Draft: this depends on QwenLM#8839 for coverage evidence — workflow-dispatched agents write no subagent transcript without it — and nothing routes through this yet, so review behaviour is unchanged. Part of QwenLM#8769. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hind a switch
The generated fan-out has had nothing routed through it: `emit-workflow`
landed as a command nobody called, so `/review` behaved exactly as before
and the migration had no way to be exercised or measured. This wires it
in, opt-in, with the legacy path unchanged and one switch to go back.
**The skill asks; it does not decide.** Step 3A now runs `emit-workflow`
first and branches on its exit code — 6 means this review takes the
legacy path (and nothing was written), 0 means a script exists and the
fan-out is one `Workflow({scriptPath})` call with no args. Eligibility
depends on facts spread across the environment and the plan, and a caller
that decides for itself can decide wrong in the one direction that
matters: taking the workflow path for a worktree review would point every
agent at the main checkout and produce findings that describe the wrong
tree. Exit 6 mirrors the reverse-audit builder's exit 4 and 5 — a
termination verdict, not a failure, and explicitly not something to
repair or retry.
**Two switches, not one.** `QWEN_CODE_ENABLE_WORKFLOWS` says the runtime
may run scripts at all; `QWEN_REVIEW_WORKFLOW` says `/review` may route
its fan-out through it. Folding them would opt a project's reviews in as
a side effect of enabling workflows for anything else, and rolling
reviews back would take the runtime down with them. Unsetting the second
is the one-switch rollback and changes nothing else.
**One predicate.** `resolveOrchestration` decides, and `emit-workflow`'s
own refusals now read from the same function rather than re-deriving the
same facts — two lists kept in step is how a run gets told "use the
workflow" and then refused by the emitter. The half that depends only on
the plan (`structuralBlocker`: a territory fan-out, a PR worktree) is
split out so the roster builder can assert against it and stay a pure
function instead of reaching for `process.env`.
What the two paths share is everything that decides quality: the same
plan, roster, briefs, and prompts built by the same function, the same
`general-purpose` subagent type, and the same coverage gate reading the
same transcripts. What differs is who launches them — which is what makes
the A/B readable.
Part of QwenLM#8769.
681c330 to
c8065a5
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qqqys
changed the title
|
Re-run on the current head after @wenshao's trigger. Template looks good ✓ Problem: observed, not theoretical. The orchestrator conventions this PR removes have each been broken in real runs — the skill's own gate list is the evidence trail (an approval with 22 zero-tool-call agents, chunk prompts naming no diff file, a whole-diff agent launched with no diff). It is the first slice of the proposal in #8769 and builds on #8971 (merged), so the problem statement has a maintainer owner. Direction: aligned. Opt-in behind two env switches, default off, one-switch rollback, legacy path untouched — nothing changes for any review unless both switches are set, and the two paths share everything that decides review quality (plan, roster, briefs, prompts, subagent type, coverage gate), which is what makes the A/B readable. Size: the only core-path file is the bundled review skill's Approach: the scope feels right. Reuse is structural, not claimed: both paths build through the same Risk: none of the 15 changed files matches the revert-correlated high-risk paths — no elevated risk signals. Moving on to code review. 🔍 中文说明应 @wenshao 的触发，在当前 head 上重跑。 模板完整 ✓ **问题：**已观测、非理论。本 PR 要移除的编排器约定，每一条都在真实运行中被打破过 —— 技能自带的门禁清单就是证据链（22 个零工具调用的 agent 下 approve、chunk prompt 不提 diff 文件、whole-diff agent 发射时没有 diff）。它是 #8769 提案的第一片，建立在已合入的 #8971 之上，问题陈述有维护者背书。 **方向：**对齐。两个环境开关后面选择性启用、默认关闭、一键回退、legacy 路径原样未动 —— 两个开关都设置之前任何 review 都不受影响，且两条路径共享一切决定 review 质量的东西（plan、roster、brief、prompt、subagent 类型、覆盖率门禁），这正是 A/B 可读的原因。 **规模：**唯一的核心路径文件是 bundled review 技能的 **方案：**范围合理。复用是结构性的而非口头声明：两条路径都经同一个 **风险：**15 个变更文件均未命中与 revert 相关的高风险路径 —— 无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal before reading the diff: if prompts must not transit the model, the CLI writes a dispatch script the runtime executes, and the model's call carries one path and no payload; the script must live where the Workflow loader already trusts, and the fail-closed accounting has to happen where the results land. The PR does exactly this, and I found no simpler path it missed — the Both round-13 Criticals are resolved on this head — verified against the code, not the commit message:
No new blockers in my pass. The static surface holds up: routing is one predicate shared by the emitter and the skill (no drift possible), the legacy verdict is decided before anything is written (no orphan records that would read as a roster that ran and returned nothing), writes are symlink-checked and temp+rename, cleanup removes the exact derived paths plus a bounded sweep of their temp orphans, and the two env gates plus kill switch are tested in both directions. The sequenceDiagram
participant P1 as Orchestrator (model)
participant P2 as qwen review emit-workflow
participant P3 as Workflow runtime
participant P4 as Step 3D coverage gate
P1->>P2: run with the plan
P2-->>P1: exit 6 legacy, or exit 0 with scriptPath
P1->>P3: one Workflow call, scriptPath only, no args
P3->>P3: parallel fan-out, roster baked in the script
P3-->>P1: delivered list plus missingRoles
P1->>P1: non-empty missingRoles, relaunch named roles or stop
P1->>P4: proceed only after every role delivered
Testing — the PR's own CI at the reviewed headEverything that ran is green; the skips are fork-PR gating (the macOS/Windows unit matrices and the integration suite need maintainer approval to run on fork PRs), pre-existing infrastructure policy and not caused by this PR — same shape as every prior head, re-fetched on the 2026-08-24 re-run. No failing check, so no log excerpt.
The tests execute the generator's real output (rosters with backticks, 中文说明代码审查读 diff 之前的独立方案：既然 prompt 不得经过模型，就由 CLI 写出分派脚本、运行时执行，模型的调用只带一个路径、零负载；脚本必须住在 Workflow 加载器已信任的目录，失败关闭的记账必须发生在结果落地的地方。本 PR 正是如此，我没有找到它遗漏的更简路径 —— 第 13 轮的两个 Critical 已在本 head 上解决 —— 对照代码核实，而非只看提交说明：
本轮审查未发现新的阻断项。 静态面成立：路由是发射器与技能共享的单一谓词（不可能漂移）；legacy 裁决在任何写入之前做出（不会留下「roster 跑过但什么都没返回」的孤儿记录）；写入经符号链接检查与临时文件+重命名；cleanup 只删精确推导路径及其临时孤儿的有界清扫；两个环境开关与 kill switch 在两个方向上都有测试。 （上方时序图：编排器运行 emit-workflow → exit 6 走 legacy 或 exit 0 得到 scriptPath → 一次不带 args 的 Workflow 调用 → 运行时并行扇出（roster 烘在脚本里）→ 返回 delivered 与 missingRoles → 非空则重启被点名角色或停止 → 全部角色交付后才进 Step 3D。） 测试 —— 本 PR 自己的 CI（审查 head 上）所有实际运行的检查全绿；跳过项是 fork PR 门禁（macOS/Windows 单测矩阵与集成测试需要维护者批准才会在 fork PR 上运行），属于既有基础设施策略、非本 PR 所致 —— 与之前每个 head 形态相同，2026-08-24 重跑时重新拉取核对。无失败检查，故无日志摘录。（上方表格：ubuntu 单测、CVE 审计、密钥扫描、桌面壳、web-shell E2E、precheck、分类均 success；三个平台/集成检查 skipped。） 测试执行的是生成器的真实输出（带反引号、 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review with both round-13 Criticals resolved and every evidence lane landed green; capped at defer because the Stage-0 escalation on this core-path fork PR needs a maintainer's sign-off, not because of doubt about the code. Stepping back: my independent proposal before reading the diff was "CLI writes the dispatch script, the model's call carries one path and no payload", and the PR matches it — I found no simpler path it missed. What I'd thank the author for in six months is unchanged from the last pass and reinforced by this one: the honesty of the scope (legacy untouched, one-switch rollback, territory fan-outs stay legacy for a named delivery reason), and the way the two round-13 Criticals were answered — not with reassurance but with the exact fix, a test executing the generated script, and a second test that pins the skill's prose to the script's field so the gate cannot be silently edited away later. Everything the previous defer asked for is now done, and the evidence that was in flight has landed — all of it green on this head. The sandbox verification lane (run 32641093610) concluded merge-ready: 145/145 scripted assertions — A/B byte-parity against the base build, the generated script executed in the real workflow sandbox, a 9-cell fail-closed routing matrix, 11/11 mutants killed by their semantically matching tests, +55 passing / +0 failing versus base, and a clean flakiness gate. A maintainer's independent local round in a credential-free container reached the same verdict (78/78 assertions, no blocking findings). Why I still don't approve: this is a fork PR whose core-path file is the bundled review skill's Step 3A dispatch contract — the orchestration playbook maintainers run daily — at 821 production lines, past the Stage 0 awareness mark, and the gate does not approve an escalated PR alone no matter how clean the stages land. What this run did clear: the stale round-13 ⏸️ Not approving — escalating to a maintainer below. 中文说明置信度：3/5 —— 审查干净、第 13 轮两个 Critical 均已解决、所有证据通道全绿落地；封顶为 defer 是因为核心路径 fork PR 的 Stage 0 升级需要维护者签字，而非对代码有疑虑。 退后一步看：我读 diff 之前的独立方案是「CLI 写出分派脚本，模型的调用只带一个路径、零负载」，本 PR 与之吻合 —— 我没有找到它遗漏的更简路径。六个月后会感谢作者的地方与上一轮相同、且本轮更甚：范围的诚实（legacy 原样未动、一键回退、territory 扇出因明确的交付原因留在 legacy），以及回应第 13 轮两个 Critical 的方式 —— 不是口头保证，而是精确的修复、一个执行生成脚本的测试，以及第二个把技能行文钉在脚本字段上的测试，使该门禁日后不可能被悄悄删掉。 上一次 defer 要求的一切现已完成，且在路上的证据已全部落地 —— 在本 head 上全绿。沙箱验证通道（run 32641093610）结论 merge-ready：145/145 条脚本断言 —— 与 base 构建 A/B 字节对等、生成脚本在真实 workflow 沙箱中执行、9 格失败关闭路由矩阵、11/11 变异全部被语义对应测试杀死、较 base +55 通过 / +0 失败、抖动门干净。维护者在无凭证容器中的独立本地轮次得出相同结论（78/78 断言，无阻塞发现）。 仍不 approve 的原因：这是一个 fork PR，其核心路径文件是 bundled review 技能的 Step 3A 分派契约 —— 维护者日常使用的编排手册 —— 且 821 行生产代码超过 Stage 0 关注线，无论各阶段多干净，门禁都不会独自 approve 一个被升级的 PR。本次运行清理掉的：过期的第 13 轮 ⏸️ 不予 approve —— 已在下方转交维护者。 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @wenshao — 2026-08-24 re-run, head still The evidence that was in flight has landed, all of it green on this head: the sandbox verification lane (run 32641093610) concluded merge-ready — 145/145 scripted assertions, A/B byte-parity against the base build, the generated script executed in the real workflow sandbox, 11/11 mutants killed, +55 passing / +0 failing versus base, clean flakiness gate — and your own independent local round in a credential-free container reached the same verdict (78/78 assertions, no blocking findings). What still needs a human call:
Nothing else is outstanding from the gate's side. 中文说明⏸️ 转交 @wenshao —— 2026-08-24 重跑，head 仍为 此前在路上的证据已全部落地，且在本 head 上全绿：沙箱验证通道（run 32641093610）结论 merge-ready —— 145/145 条脚本断言、与 base 构建 A/B 字节对等、生成脚本在真实 workflow 沙箱中执行、11/11 变异全部被杀死、较 base +55 通过 / +0 失败、抖动门干净；你本人在无凭证容器中的独立本地轮次得出相同结论（78/78 断言，无阻塞发现）。 仍需人工决定的：
门禁侧再无其他未决事项。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory.
中文说明
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('still routes to legacy when the runtime is on but /review is not opted in', () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); | ||
| const priorExit = process.exitCode; | ||
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); |
There was a problem hiding this comment.
[Suggestion] The two routing-verdict tests do not pin the full gate environment — and unlike the "what it writes" block, they never process.chdir into their tmpdir.
Failure scenario: this test leaves QWEN_REVIEW_WORKFLOW at its ambient value, and 'routes to legacy without writing anything when workflows are disabled' pins neither variable. In an environment with both QWEN_CODE_ENABLE_WORKFLOWS=1 and QWEN_REVIEW_WORKFLOW=1 exported — the exact dogfood configuration for this feature — resolveOrchestration flips to workflow mode: besides the misleading exit-code failures, the handler proceeds past the legacy early-return and writeFileSync(reviewWorkflowScriptPath(plan), …) writes a generated fan-out script into the real repository's .qwen/workflows/, because reviewWorkflowScriptPath is cwd-relative and these tests never chdir.
Witness (probe):
QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1 npx vitest run src/commands/review/emit-workflow.test.ts
× routes to legacy without writing anything when workflows are disabled → expected undefined to be 6
× still routes to legacy when the runtime is on but /review is not opted in → expected undefined to be 6
Tests 2 failed | 11 passed (13)
and two stray generated scripts (qwen-review-e8f03e68b4.js, qwen-review-7252b4d4e7.js, full rosters baked in) were created in the repo's packages/cli/.qwen/workflows/ by the run.
Suggested fix: pin both halves of the gate in every routing-verdict test — vi.stubEnv('QWEN_REVIEW_WORKFLOW', '') here (and vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '') in the disabled test) with the existing vi.unstubAllEnvs() cleanup — and chdir into the tmpdir like the "what it writes" block does.
中文说明
两个路由裁决测试没有钉住完整的门禁环境 —— 而且与 "what it writes" 块不同，它们从未 process.chdir 进自己的 tmpdir。
失败场景：本测试把 QWEN_REVIEW_WORKFLOW 留在环境原值，而 'routes to legacy without writing anything when workflows are disabled' 两个变量都不钉。在同时导出 QWEN_CODE_ENABLE_WORKFLOWS=1 与 QWEN_REVIEW_WORKFLOW=1 的环境里 —— 正是本功能的 dogfood 配置 —— resolveOrchestration 翻转为 workflow 模式：除了误导性的退出码断言失败之外，handler 会越过 legacy 提前返回，writeFileSync(reviewWorkflowScriptPath(plan), …) 把一个生成的扇出脚本写进真实仓库的 .qwen/workflows/ —— 因为 reviewWorkflowScriptPath 是 cwd 相对路径，而这两个测试从未 chdir。
证据：上述环境运行该套件得到 2 failed | 11 passed，且运行本身在仓库 packages/cli/.qwen/workflows/ 里创建了两个游离的生成脚本（内含完整 roster）。
建议修复：在每个路由裁决测试里钉住门禁的两半 —— 本测试加 vi.stubEnv('QWEN_REVIEW_WORKFLOW', '')（disabled 测试加 vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '')），配合现有 vi.unstubAllEnvs() 清理；并像 "what it writes" 块一样 chdir 进 tmpdir。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| (emitWorkflowCommand.handler as (a: unknown) => void)({ plan }); | ||
|
|
||
| const scriptPath = reviewWorkflowScriptPath(plan); |
There was a problem hiding this comment.
[Suggestion] The happy path asserts the script file but never the exit-code/stdout contract SKILL.md's new routing section branches on.
Failure scenario: every process.exitCode reference in this file is inside the two legacy routing tests asserting EXIT_LEGACY_ORCHESTRATION; no happy-path test checks that the exit code stays unset (0), and none captures stdout — the scriptPath: <abs path> line the skill parses, and the exit-0-vs-6 branch it keys on. If a refactor hoists process.exitCode = EXIT_LEGACY_ORCHESTRATION past the legacy early return, or the success branch leaves any non-zero code set, these tests stay green while the skill either hand-launches a roster for a review whose script was already emitted, or aborts a healthy run.
Suggested fix: in one happy-path test assert expect(process.exitCode ?? 0).toBe(0) and capture stdout, asserting it contains scriptPath: followed by an absolute path.
中文说明
happy path 只断言了脚本文件，从未钉住 SKILL.md 新路由小节所依赖的退出码/stdout 契约。
失败场景：本文件中所有 process.exitCode 引用都在两个 legacy 路由测试里（断言 EXIT_LEGACY_ORCHESTRATION）；没有任何 happy-path 测试检查退出码保持未设置（0），也没有测试捕获 stdout —— 即技能要解析的 scriptPath: <绝对路径> 一行与据以分支的 0/6 退出码。若某次重构把 process.exitCode = EXIT_LEGACY_ORCHESTRATION 提升到 legacy 提前返回之前，或成功分支遗留了任何非零退出码，这些测试依旧全绿，而技能要么为一个已生成脚本的 review 手工发射 roster，要么中止一次健康的运行。
建议修复：在某个 happy-path 测试中断言 expect(process.exitCode ?? 0).toBe(0) 并捕获 stdout，断言其包含 scriptPath: 及一个绝对路径。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| let rules: string | undefined; | ||
| if (args.rules) { |
There was a problem hiding this comment.
[Suggestion] The new --rules option has no test on this command's path — neither the success wiring nor the unreadable-path refusal.
Failure scenario: no emit-workflow test passes rules, so neither rules reaching buildLaunch via buildFanOutRoster nor the unreadable-path refusal is exercised here, even though the sibling command has exactly that test (agent-prompt.test.ts asserts /cannot read the rules/). This block is a fresh copy of agent-prompt's pattern: an edit to the emit-workflow copy that swallows the read failure (or reads the wrong path) silently runs a workflow-mode review without the project rules it was told to enforce — every agent prompt missing the rules section — while agent-prompt's test still passes green because it tests the other copy.
Suggested fix: add two cases — --rules pointing at a temp file (assert the file's content appears in each baked prompt), and --rules pointing at a nonexistent path (assert the handler throws /cannot read the rules/).
中文说明
新的 --rules 选项在本命令路径上没有任何测试 —— 成功接线与不可读路径的拒绝都没有。
失败场景：没有任何 emit-workflow 测试传入 rules，因此无论是 rules 经 buildFanOutRoster 到达 buildLaunch 的成功接线，还是不可读路径的拒绝，都未被本文件演练；尽管姊妹命令恰有同样的测试（agent-prompt.test.ts 断言 /cannot read the rules/）。此代码块是 agent-prompt 模式的一份新拷贝：若对 emit-workflow 这份拷贝的修改吞掉了读取失败（或读错路径），workflow 模式的 review 会在未加载项目规则的情况下静默运行 —— 每个 agent 的 prompt 都缺失规则小节 —— 而 agent-prompt 的测试依然绿，因为它测的是另一份拷贝。
建议修复：补两个用例 —— --rules 指向一个临时文件（断言其内容出现在每个烘进的 prompt 中）；--rules 指向不存在的路径（断言 handler 抛出 /cannot read the rules/）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| }); | ||
|
|
||
| describe('the env predicates', () => { | ||
| it('workflowsEnabled: opt-in, with the kill switch winning', () => { |
There was a problem hiding this comment.
[Suggestion] The suite pins exact-'1' semantics for reviewWorkflowEnabled ('true'/'0' cases) but none for workflowsEnabled, whose stated reason to exist is mirroring Config.isWorkflowsEnabled's env half — strict === '1' on BOTH vars (config.ts).
Failure scenario: a refactor of workflowsEnabled to truthy parsing (return !!env['QWEN_CODE_ENABLE_WORKFLOWS'], or truthy kill-switch handling) keeps every test green. With QWEN_CODE_ENABLE_WORKFLOWS=true (a common convention value) and workflows not enabled in settings, Config.isWorkflowsEnabled() is false so the Workflow tool is never registered — but resolveOrchestration returns workflow, emit-workflow exits 0, writes the script, and instructs the skill to make ONE Workflow call the runtime cannot execute, wedging Step 3A. The kill-switch side diverges in the opposite direction: truthy DISABLE=true routes legacy while the runtime stays enabled.
Witness (probe, three arms): truthy refactor + existing suites → Tests 24 passed (24); truthy refactor + the suggested assertions → AssertionError: expected true to be false (the flip); correct implementation + the suggested assertions → Tests 12 passed (12) (safe to add).
Suggested fix: add beside the existing pins — expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false), { QWEN_CODE_ENABLE_WORKFLOWS: '0' } → false, and { QWEN_CODE_ENABLE_WORKFLOWS: '1', QWEN_CODE_DISABLE_WORKFLOWS: 'true' } → true.
中文说明
套件为 reviewWorkflowEnabled 钉住了精确 '1' 语义（'true'/'0' 用例），却没为 workflowsEnabled 钉 —— 而后者存在的理由就是镜像 Config.isWorkflowsEnabled 的环境变量半边：对两个变量都做严格 === '1'（config.ts）。
失败场景：把 workflowsEnabled 重构为 truthy 解析（return !!env['QWEN_CODE_ENABLE_WORKFLOWS']，或 truthy 的 kill switch 处理）后所有测试保持绿。当 QWEN_CODE_ENABLE_WORKFLOWS=true（常见约定值）且 settings 未启用 workflow 时，Config.isWorkflowsEnabled() 为 false，Workflow 工具根本不会注册 —— 但 resolveOrchestration 返回 workflow，emit-workflow 退出 0、写出脚本并指示技能发出唯一一次 Workflow 调用，而运行时无法执行它 —— Step 3A 被卡死。kill switch 一侧向相反方向偏离：truthy 的 DISABLE=true 会路由到 legacy，而运行时仍保持启用。
证据（探针，三臂）：truthy 重构 + 现有套件 → 24 passed；truthy 重构 + 建议断言 → AssertionError: expected true to be false（翻转）；正确实现 + 建议断言 → 12 passed（可安全添加）。
建议修复：在现有钉法旁补 —— expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false)、{ QWEN_CODE_ENABLE_WORKFLOWS: '0' } → false、以及 { QWEN_CODE_ENABLE_WORKFLOWS: '1', QWEN_CODE_DISABLE_WORKFLOWS: 'true' } → true。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * outside the saved-workflow directories. A script written beside the plan in | ||
| * `.qwen/tmp` is a script the tool will not open. | ||
| */ | ||
| export const REVIEW_WORKFLOWS_DIR = join('.qwen', 'workflows'); |
There was a problem hiding this comment.
[Suggestion] Write side of the symlinked-root boundary: emit-workflow's mkdirSync(dirname(scriptPath), {recursive: true}); writeFileSync(scriptPath, …) writes the generated script THROUGH a symlinked .qwen/workflows root with no guard, while every other actor in this feature's boundary model refuses that configuration.
Failure scenario: readWorkflowFileSecurely excludes symlinked roots from the load boundary and saveWorkflowScript explicitly refuses to write through one (both in workflow-saved.ts); the cleanup deletion side is the other comment on cleanup.ts. In a repo where .qwen/workflows symlinks to an external/shared directory, with both gates set and an eligible review, emit succeeds — writes the script outside the project through the link, records prompts, prints "make ONE Workflow call with the scriptPath below". The subsequent Workflow({scriptPath}) call is guaranteed to throw "refusing to load a workflow file outside the saved-workflow directories", so Step 3 fails after emit declared it dispatchable, and a self-generated file persists outside the boundary that read and save both protect.
Witness (probe): unmodified PR, symlinked root — handler exited 0 and printed scriptPath: /tmp/probe-proj-…/.qwen/workflows/qwen-review-e0b56d8472.js; the external dir gained the script (AssertionError: expected [ 'qwen-review-2ecff4260e.js' ] to deeply equal []). With an lstat refusal before the write the probe passes; the core loader refusal suite runs 33/33 green.
Suggested fix: before writing, mirror the loader — lstatSync(REVIEW_WORKFLOWS_DIR) (after mkdirSync) and refuse with a stderr error when isSymbolicLink(), same message shape as saveWorkflowScript — or route the write through a shared helper so the guard lives in one place. Note: a guard placed after buildFanOutRoster still leaves prompt records behind (they are written inside the roster build), so refuse before the build or sweep the records on refusal.
中文说明
符号链接根边界的写入侧：emit-workflow 的 mkdirSync(dirname(scriptPath), {recursive: true}); writeFileSync(scriptPath, …) 会在没有任何守卫的情况下穿过符号链接的 .qwen/workflows 根写入生成脚本，而本功能边界模型中的其他所有参与者都拒绝该配置。
失败场景：readWorkflowFileSecurely 把符号链接根排除在加载边界之外，saveWorkflowScript 明确拒绝穿过它写入（均在 workflow-saved.ts）；删除侧是 cleanup.ts 上的另一条评论。在 .qwen/workflows 符号链接到外部/共享目录的仓库里，双门禁开启且 review 合格时，emit 成功 —— 穿过链接把脚本写到项目之外、记录 prompt、打印 "make ONE Workflow call with the scriptPath below"。随后的 Workflow({scriptPath}) 调用必然抛出 "refusing to load a workflow file outside the saved-workflow directories" —— emit 宣称可分派之后 Step 3 失败，且一个自生成文件滞留在读取与保存都在保护的边界之外。
证据（探针）：未修改的 PR、符号链接根 —— handler 退出 0 并打印 scriptPath；外部目录出现了该脚本（断言外部目录为空失败）。写入前加 lstat 拒绝后探针通过；核心加载器拒绝套件 33/33 绿。
建议修复：写入前镜像加载器 —— mkdirSync 之后 lstatSync(REVIEW_WORKFLOWS_DIR)，若 isSymbolicLink() 则以 stderr 错误拒绝，消息形态与 saveWorkflowScript 一致；或把写入 routed 到一个共享 helper，让守卫只存在一处。注意：放在 buildFanOutRoster 之后的守卫仍会留下 prompt 记录（它们在 roster 构建过程中写入），所以应在构建之前拒绝，或在拒绝时清扫记录。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const scriptPath = reviewWorkflowScriptPath(plan); | ||
| expect(scriptPath.startsWith(join('.qwen', 'workflows'))).toBe(true); |
There was a problem hiding this comment.
[Suggestion] This assertion re-derives from the writer's own constant — it cannot fail for any loader-side reason, so writer and loader can drift with zero red tests.
Failure scenario: reviewWorkflowScriptPath is literally join(REVIEW_WORKFLOWS_DIR, …), so scriptPath.startsWith(join('.qwen', 'workflows')) asserts the constant against itself. The PR's review code has zero imports of core's acceptance layer — readWorkflowFileSecurely/resolveSavedWorkflowScript (allowed roots, symlink refusal) and validateWorkflowName/WORKFLOW_NAME_PATTERN — and .qwen/workflows exists as two independent literals, one per package, with no shared constant or pairing test. A core-side rename/restructure of the project saved-workflow root leaves all 13 tests green; then every workflow-eligible review emits exit 0 + "make ONE Workflow call", the loader refuses, and Step 3A wedges after prompt records were already written — downstream gates read a dispatched roster that never ran. (A name-pattern tightening takes the other branch: it orphan-slashes the generated /qwen-review-… command.) Same vacuous-pin shape as the exit-code comment above.
Witness (probe, drift simulation): renamed getProjectWorkflowsDir()'s 'workflows' → 'workflows2' in core, re-ran this file + workflow-script.test.ts → 24 passed (24) with the loader boundary moved out from under the writer.
Suggested fix: pin the core half of the contract in the same test — expect(validateWorkflowName(reviewWorkflowName(plan))).toBeNull() and resolve the written script through the loader's exported entry point (resolveSavedWorkflowScript), asserting it is accepted (both are exported from the core package root); or add a core-side test pinning that the loader's allowed roots include the exact relative dir emit-workflow writes to.
中文说明
该断言是从写入方自己的常量推导出来的 —— 它不可能因加载器侧的任何原因失败，因此写入方与加载器可以在零测试变红的情况下漂移。
失败场景：reviewWorkflowScriptPath 就是 join(REVIEW_WORKFLOWS_DIR, …)，所以 scriptPath.startsWith(join('.qwen', 'workflows')) 是常量对自身的断言。本 PR 的 review 代码对核心接受层零导入 —— readWorkflowFileSecurely/resolveSavedWorkflowScript（允许的根、符号链接拒绝）与 validateWorkflowName/WORKFLOW_NAME_PATTERN —— 而 .qwen/workflows 以两个独立字面量存在，每包一个，没有共享常量也没有配对测试。核心侧对项目 saved-workflow 根的一次改名/重构后 13 个测试全绿；于是每个 workflow 合格的 review 都 emit 退出 0 + "make ONE Workflow call"，加载器拒绝，Step 3A 在 prompt 记录已写之后卡死 —— 下游门禁读到一个"已分派"却从未运行的 roster。（名字模式收紧则走另一分支：生成的 /qwen-review-… 命令成为孤儿。）与上方退出码评论同属"空钉"形状。
证据（探针，漂移模拟）：把核心 getProjectWorkflowsDir() 的 'workflows' 改为 'workflows2'，重跑本文件 + workflow-script.test.ts → 加载器边界已移走而 24 passed (24)。
建议修复：在同一测试里钉住契约的核心半边 —— expect(validateWorkflowName(reviewWorkflowName(plan))).toBeNull()，并把写出的脚本经加载器导出的入口（resolveSavedWorkflowScript）解析、断言被接受（两者均从核心包根导出）；或在核心侧加一个测试，钉住加载器允许的根包含 emit-workflow 写入的确切相对目录。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| } | ||
|
|
||
| /** Absolute-or-relative path of the generated fan-out script for a plan. */ | ||
| export function reviewWorkflowScriptPath(planPath: string): string { |
There was a problem hiding this comment.
[Suggestion] The generated script's destination is cwd-relative, but the loader boundary is rooted at the session's project root (Storage.getProjectWorkflowsDir()) plus user scope, and cleanup sweeps only the cwd-relative dir — so emit-workflow run from any cwd other than the project root writes a script the runtime either rejects or can never clean up.
Failure scenario: a maintainer runs qwen review emit-workflow --plan <abs plan path> from $HOME (or a repo subdirectory). (a) cwd = $HOME: the script lands in ~/.qwen/workflows — the user-scope saved-workflow dir, which readWorkflowFileSecurely explicitly allows — so it loads and runs, but cleanup only ever reads REVIEW_WORKFLOWS_DIR under its own cwd and never user scope: the script becomes a permanent /qwen-review-<hex> slash command in every project on the machine — precisely the orphan the cleanup sweep's comment says it exists to prevent. (b) any other cwd: the script is inside neither allowed directory, so the instructed Workflow({scriptPath}) throws "refusing to load a workflow file outside the saved-workflow directories" — Step 3A wedges after emit declared it dispatchable, and a stray .qwen/workflows/ persists where no cleanup target ever looks. Aggravating: reviewWorkflowName digests resolve(planPath) against cwd, so a re-run from a different cwd also breaks the documented "replaces its own file rather than accumulating" invariant. (The trigger is outside the command family's documented cwd convention, but the harms are concrete, and (a) defeats the very invariant the sweep was added for.)
Witness (probe, real handler + real loader): scenario (b) — handler exited 0, printed scriptPath: /tmp/probe-other-…/.qwen/workflows/qwen-review-f2c8e2dfda.js, then the real resolveSavedWorkflowScript rejected with /outside the saved-workflow directories/; scenario (a) — the loader resolved the <home>/.qwen/workflows/qwen-review-90909b1cc2.js script successfully while <proj>/.qwen/workflows did not exist.
Suggested fix: preflight that the write will land where the loader reads — the plan always lives under the project's .qwen/tmp, so refuse when resolve(planPath) is not inside process.cwd(), with an error naming that the command must run from the project root; alternatively resolve REVIEW_WORKFLOWS_DIR against a discovered project root instead of cwd.
中文说明
生成脚本的落点是 cwd 相对路径，但加载器边界以会话的项目根（Storage.getProjectWorkflowsDir()）加用户作用域为根，而 cleanup 只清扫 cwd 相对目录 —— 因此在项目根之外的任何 cwd 运行 emit-workflow，写出的脚本要么被运行时拒绝，要么永远无法被清扫。
失败场景：维护者从 $HOME（或仓库子目录）运行 qwen review emit-workflow --plan <绝对 plan 路径>。(a) cwd = $HOME：脚本落入 ~/.qwen/workflows —— 加载器明确允许的用户级 saved-workflow 目录 —— 于是能加载运行，但 cleanup 只读自己 cwd 下的 REVIEW_WORKFLOWS_DIR、从不看用户作用域：该脚本成为机器上每个项目里的永久 /qwen-review-<hex> 斜杠命令 —— 恰是 cleanup 清扫注释声称要防止的孤儿。(b) 其他任何 cwd：脚本不在任何允许目录之内，指示中的 Workflow({scriptPath}) 抛出 "outside the saved-workflow directories" —— emit 宣称可分派之后 Step 3A 卡死，且一个游离的 .qwen/workflows/ 滞留在任何 cleanup target 都不会看的地方。加重因素：reviewWorkflowName 对 resolve(planPath) 按 cwd 求摘要，从不同 cwd 重跑还会破坏文档化的"替换自己的文件而非累积"不变量。（触发条件超出本命令族文档化的 cwd 约定，但危害具体，且 (a) 击败的正是清扫存在的理由。）
证据（探针，真实 handler + 真实加载器）：场景 (b) —— handler 退出 0 并打印 scriptPath，随后真实 resolveSavedWorkflowScript 以 /outside the saved-workflow directories/ 拒绝；场景 (a) —— 加载器成功解析 <home>/.qwen/workflows/qwen-review-90909b1cc2.js，而 <proj>/.qwen/workflows 不存在。
建议修复：写入前预检落点就是加载器的读取处 —— plan 恒在项目 .qwen/tmp 之下，因此当 resolve(planPath) 不在 process.cwd() 之内时拒绝，并在错误中说明命令必须从项目根运行；或者把 REVIEW_WORKFLOWS_DIR 解析到探测出的项目根而非 cwd。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ]; | ||
|
|
||
| describe('the generated Step 3A fan-out script', () => { | ||
| it('opens with a meta block the sandbox will accept as a pure literal', () => { |
There was a problem hiding this comment.
[Suggestion] The generated meta block's shape-validity is pinned by no test: this harness converts export const meta = to const meta = instead of exercising the sandbox's real strip/extract path, and the regex only checks lexical purity — while the runtime enforces a strict meta contract by throwing.
Failure scenario: the workflow runtime loads the emitted script through extractAndStripMeta → validateMeta (workflow-sandbox.ts, untouched by this PR), which throws meta.name must be a non-empty string, meta.description must be a non-empty string, or meta.phases[].title must be a non-empty string on a shape violation. An edit to the five-line meta constant in buildReviewWorkflowScript — dropping or emptying description, restructuring phases — leaves all 11 tests in this file green (emit-workflow.test.ts only asserts toContain('export const meta'); no test in packages/cli runs the block through the real contract). Every workflow-path review then fails at load — Workflow({scriptPath}) throws before dispatching a single agent — silently breaking the exact path this change exists to serve, with no test in the PR catching the regression.
Witness (probe): applied description: '' to buildReviewWorkflowScript → the PR's workflow-script.test.ts + emit-workflow.test.ts run 24 passed (24); the real extractAndStripMeta on the same regression throws meta.description must be a non-empty string. (Today's meta is accepted by the real extractor — this is a missing-pin finding, not a current defect.)
Suggested fix: pin the contract fields in the meta-block test — non-empty name and description string values, and a phases array whose entry carries a non-empty title. Note: extractAndStripMeta is not exported from the core package root, so either pin the fields directly or add the export and run the generated script through it.
中文说明
生成的 meta 块的形状合法性没有任何测试钉住：本 harness 用 export const meta = → const meta = 的替换代替沙箱真实的剥离/提取路径，正则也只检查词法纯净度 —— 而运行时是以抛错来强制执行严格的 meta 契约的。
失败场景：workflow 运行时经 extractAndStripMeta → validateMeta（workflow-sandbox.ts，本 PR 未改动）加载发出的脚本，形状违规时抛出 meta.name must be a non-empty string、meta.description must be a non-empty string 或 meta.phases[].title must be a non-empty string。对 buildReviewWorkflowScript 中五行 meta 常量的一次修改 —— 删掉或清空 description、重构 phases —— 本文件 11 个测试全绿（emit-workflow.test.ts 只断言 toContain('export const meta')；packages/cli 中没有任何测试让该块过真实契约）。于是每个 workflow 路径的 review 都在加载时失败 —— Workflow({scriptPath}) 在分派任何 agent 之前抛错 —— 悄悄弄坏本变更存在所要服务的那条路径，而 PR 中没有测试能捕获该回归。
证据（探针）：对 buildReviewWorkflowScript 应用 description: '' → PR 的两个套件 24 passed (24)；真实 extractAndStripMeta 对同一回归抛出 meta.description must be a non-empty string。（当前 meta 被真实提取器接受 —— 这是缺钉发现，不是当前缺陷。）
建议修复：在 meta 块测试里钉住契约字段 —— 非空的 name 与 description 字符串值，以及条目含非空 title 的 phases 数组。注意：extractAndStripMeta 未从核心包根导出，因此要么直接钉字段，要么新增导出并让生成脚本过一遍它。
— qwen3.8-max via Qwen Code /review (v0.21.13)
已被后续 commit 取代，当前 head 需重新 review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 21082, 20210, 1471, 1575, 494, 3686, 507 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/report.ts:81 — [review] readPlanReport insertion orphans the PlanReport interface's doc comment (two JSDoc blocks stack above the function)packages/cli/src/commands/review/emit-workflow.test.ts:208 — [review] legacy-routing tests claim 'without writing anything' but never assert that no script file was writtenpackages/cli/src/commands/review/emit-workflow.test.ts:300 — [review] vacuous re-run-stability assertion: expect(reviewWorkflowScriptPath(a)).toBe(reviewWorkflowScriptPath(a)) cannot failpackages/cli/src/commands/review/emit-workflow.test.ts:165 — [review] refusal tests assert only toThrow; a refused build could leave briefs/prompt records behindpackages/cli/src/commands/review/emit-workflow.ts:62 — [review] workflowsEnabled re-export has no production consumer; its only test duplicates orchestration.test.ts's canonical pinpackages/core/src/skills/bundled/review/SKILL.md:327 — [review] workflow path silently drops the optional Agent 8 finders while the hunk claims the paths share everything that decides qualitypackages/cli/src/commands/review/emit-workflow.ts:139 — [review] legacy-verdict stderr notices use throwing writeStderrLine before exit 6 is set (match-remote uses writeStderrLineSafe here)packages/cli/src/commands/review/emit-workflow.test.ts:66 — [review] localPlan fixture uses nonexistent budget.toolCalls field (real field is agentToolBudget) — prompts bake no budget sectionpackages/cli/src/commands/review/lib/paths.ts:55 — [review] REVIEW_WORKFLOW_PREFIX comment asserts 'the cleanup sweep globs it' — no such glob exists; misdirects future orphan fixespackages/cli/src/commands/review/emit-workflow.test.ts:93 — [review] roster-parity test derives its expected side from the same in-memory object it passes — plan mutations stay invisiblepackages/cli/src/commands/review/lib/report.ts:84 — [review] readPlanReport doc lists base-tree among try/parse/rethrow copies, but base-tree recovers gracefully (available:false)
中文说明
已审查。 建议见行内评论。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 21082, 20210, 1471, 1575, 494, 3686, 507 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('still routes to legacy when the runtime is on but /review is not opted in', () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); | ||
| const priorExit = process.exitCode; | ||
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); |
There was a problem hiding this comment.
[Suggestion] R1-3: Still stands — the two routing-verdict tests still do not pin the full gate environment, and still never process.chdir into their tmpdir: 'routes to legacy…when workflows are disabled' pins neither variable, and its sibling stubs only QWEN_CODE_ENABLE_WORKFLOWS, leaving QWEN_REVIEW_WORKFLOW ambient.
Failure scenario: in the dogfood environment (QWEN_CODE_ENABLE_WORKFLOWS=1 + QWEN_REVIEW_WORKFLOW=1 exported), resolveOrchestration flips to workflow mode → the exit-code assertions fail spuriously and, because reviewWorkflowScriptPath is cwd-relative and these tests never chdir, a generated fan-out script is written into the real repository's .qwen/workflows/.
Suggested fix: pin both halves of the gate in every routing-verdict test — vi.stubEnv('QWEN_REVIEW_WORKFLOW', '') here and vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '') in the disabled test, with the existing vi.unstubAllEnvs() cleanup — and chdir into the tmpdir like the "what it writes" block does.
中文说明
仍未修复 —— 两个路由裁决测试依旧没有钉住完整的门禁环境，也仍未 process.chdir 进自己的 tmpdir：'routes to legacy…when workflows are disabled' 两个变量都不钉，另一个测试只 stub 了 QWEN_CODE_ENABLE_WORKFLOWS，QWEN_REVIEW_WORKFLOW 留在环境原值。
失败场景：在 dogfood 环境（同时导出 QWEN_CODE_ENABLE_WORKFLOWS=1 与 QWEN_REVIEW_WORKFLOW=1）下，resolveOrchestration 翻转为 workflow 模式 → 退出码断言假失败，且因 reviewWorkflowScriptPath 是 cwd 相对路径而测试从未 chdir，生成的扇出脚本被写进真实仓库的 .qwen/workflows/。
建议修复：在每个路由裁决测试里钉住门禁两半 —— 本测试加 vi.stubEnv('QWEN_REVIEW_WORKFLOW', '')、disabled 测试加 vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '')，配合现有 vi.unstubAllEnvs() 清理；并像 "what it writes" 块一样 chdir 进 tmpdir。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); | ||
| (emitWorkflowCommand.handler as (a: unknown) => void)({ plan }); | ||
|
|
||
| const scriptPath = reviewWorkflowScriptPath(plan); |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands — the happy path asserts the script file but never the exit-code/stdout contract SKILL.md's routing section branches on: no happy-path test checks that process.exitCode stays 0, and none captures the scriptPath: <abs path> stdout line.
Failure scenario: a refactor hoisting process.exitCode = EXIT_LEGACY_ORCHESTRATION past the legacy early return, or a success branch leaving any non-zero code set, keeps these tests green while the skill either hand-launches a roster for a review whose script was already emitted, or aborts a healthy run.
Suggested fix: in one happy-path test assert expect(process.exitCode ?? 0).toBe(0) and capture stdout, asserting it contains scriptPath: followed by an absolute path.
中文说明
仍未修复 —— happy path 只断言了脚本文件，从未钉住 SKILL.md 路由小节所依赖的退出码/stdout 契约：没有 happy-path 测试检查 process.exitCode 保持 0，也没有测试捕获 scriptPath: <绝对路径> 这行 stdout。
失败场景：若某次重构把 process.exitCode = EXIT_LEGACY_ORCHESTRATION 提升到 legacy 提前返回之前，或成功分支遗留任何非零退出码，这些测试依旧全绿，而技能要么为一个已生成脚本的 review 手工发射 roster，要么中止一次健康的运行。
建议修复：在某个 happy-path 测试中断言 expect(process.exitCode ?? 0).toBe(0) 并捕获 stdout，断言其包含 scriptPath: 及一个绝对路径。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // does not resolve would silently review without the project rules the run | ||
| // was told to enforce. | ||
| let rules: string | undefined; | ||
| if (args.rules) { |
There was a problem hiding this comment.
[Suggestion] R1-5: Still stands — the --rules option has no test on this command's path: neither the success wiring (rules content reaching the baked prompts via buildFanOutRoster/buildLaunch) nor the unreadable-path refusal is exercised here, even though the sibling agent-prompt command has exactly that test.
Failure scenario: this block is a fresh copy of agent-prompt's pattern — an edit to the emit-workflow copy that swallows the read failure (or reads the wrong path) silently runs a workflow-mode review without the project rules it was told to enforce (every agent prompt missing the rules section), while agent-prompt's test stays green because it tests the other copy.
Suggested fix: add two cases — --rules pointing at a temp file (assert the file's content appears in each baked prompt), and --rules pointing at a nonexistent path (assert the handler throws /cannot read the rules/).
中文说明
仍未修复 —— --rules 选项在本命令路径上没有任何测试：成功接线（rules 内容经 buildFanOutRoster/buildLaunch 到达烘进的 prompt）与不可读路径的拒绝都未被演练，尽管姊妹命令 agent-prompt 恰有同样的测试。
失败场景：此代码块是 agent-prompt 模式的一份新拷贝 —— 若对 emit-workflow 这份拷贝的修改吞掉了读取失败（或读错路径），workflow 模式的 review 会在未加载项目规则的情况下静默运行（每个 agent 的 prompt 都缺失规则小节），而 agent-prompt 的测试依然绿，因为它测的是另一份拷贝。
建议修复：补两个用例 —— --rules 指向一个临时文件（断言其内容出现在每个烘进的 prompt 中）；--rules 指向不存在的路径（断言 handler 抛出 /cannot read the rules/）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| emitWorkflowCommand, | ||
| workflowsEnabled, | ||
| EXIT_LEGACY_ORCHESTRATION, | ||
| } from './emit-workflow.js'; |
There was a problem hiding this comment.
[Suggestion] R1-7: Still stands — nothing anywhere pins the literal exit code 6 that SKILL.md's routing branch keys on; both gate tests assert against the imported EXIT_LEGACY_ORCHESTRATION constant, a tautological pin.
Failure scenario: a future revalue of the constant (the review command family already spans exit codes 0/1/3/4/5/6) moves both assertions and stays green — then every ineligible review lands in the skill's "real failure" branch and halts, or, revalued to 0, the skill takes the workflow branch and looks for a scriptPath: line the legacy path never printed. The sibling contract IS pinned by literal: match-remote.test.ts asserts expect(process.exitCode).toBe(6).
Suggested fix: assert the literal in at least one gate test, e.g. expect(process.exitCode).toBe(6); beside the constant assertion, or add a SKILL.test.ts pin for the emit-workflow "Exit 6" sentence.
中文说明
仍未修复 —— 没有任何测试钉住 SKILL.md 路由分支所依赖的字面退出码 6；两个门禁测试都对着导入的 EXIT_LEGACY_ORCHESTRATION 常量断言，属于同义反复式钉法。
失败场景：未来对该常量的重新赋值（review 命令族已横跨退出码 0/1/3/4/5/6）会让两个断言随之移动且保持全绿 —— 于是每个不合条件的 review 都落入技能的"真实故障"分支而中止；或者若被改为 0，技能走 workflow 分支去寻找 legacy 路径从未打印的 scriptPath: 一行。姊妹契约是字面钉住的：match-remote.test.ts 断言 expect(process.exitCode).toBe(6)。
建议修复：至少在一个门禁测试里断言字面值，例如在常量断言旁加 expect(process.exitCode).toBe(6);；或在 SKILL.test.ts 为 emit-workflow 的 "Exit 6" 句加钉。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const logs: string[] = []; | ||
| const phases: string[] = []; | ||
|
|
||
| const agent = async (prompt: string, opts: unknown) => { |
There was a problem hiding this comment.
[Suggestion] R1-8: Still stands — the executable harness's fake agent() accepts opts opaquely, but the real vm-realm agent() throws on any key outside KNOWN_AGENT_OPTS — no test in this suite can catch an opts-key mutation in FAN_OUT_BODY.
Failure scenario: an edit to the generated dispatch opts — adding description (mandated by the SKILL.md dimensions section for agent calls, making the addition tempting) or renaming agentType toward the skill's subagent_type vocabulary — leaves all 11 tests green, while at runtime every dispatch throws, parallel() settles every slot to null, and FAN_OUT_BODY throws "all N agents failed to deliver" — every workflow-mode fan-out fails while the suite stays green.
Suggested fix: in the existing agentType/label test, also pin the exact opts key set: expect(Object.keys(d.opts).sort()).toEqual(['agentType', 'label', 'phase']) for every dispatch.
中文说明
仍未修复 —— 可执行 harness 里的假 agent() 对 opts 照单全收，但真实 vm 域中的 agent() 会对 KNOWN_AGENT_OPTS 之外的任何键抛错 —— 本套件没有任何测试能捕获 FAN_OUT_BODY 中的 opts 键变异。
失败场景：对生成分派 opts 的一次修改 —— 比如加上 description（SKILL.md 的 dimensions 小节要求每次 agent 调用都带 description，这一加法很诱人），或把 agentType 改名为技能里的 subagent_type 词汇 —— 11 个测试全部保持绿，而运行时每次分派都抛错，parallel() 把每个槽位置为 null，FAN_OUT_BODY 抛出 "all N agents failed to deliver" —— 每个 workflow 模式的扇出都失败，套件却全绿。
建议修复：在现有 agentType/label 测试里同时钉住 opts 键集合：对每次分派断言 expect(Object.keys(d.opts).sort()).toEqual(['agentType', 'label', 'phase'])。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('removes only this target generated fan-out scripts', () => { | ||
| mocks.execFileSync.mockReturnValue(Buffer.from('')); |
There was a problem hiding this comment.
[Suggestion] The new generated-workflow sweep has zero non-PR test coverage: both workflow-sweep tests run runCleanup('pr-123'), while the non-PR calls ('local', 'lease') assert nothing about its rmSync calls.
Failure scenario: a maintainer "tidies" by moving the workflow block inside the if (prMatch) branch (or gating it on the same /^pr-\d+$/ regex) — both workflow tests use 'pr-123', no other test asserts a workflow removal, and the suite stays green. Thereafter any local or file-path review that took the workflow path (structuralBlocker admits those — only territory fan-outs and pr-worktree plans are refused) runs Step 9 cleanup that no longer removes its script, leaving a permanent /qwen-review-<digest> slash command in the user's saved-workflow directory — exactly the residue this block's comment says it exists to prevent.
witness (probe): wrapped the workflow block in if (/^pr-\d+$/u.test(target)) → suite still 38/38 green; with the suggested non-PR assertion added → 1 failed; the same assertion against the unmutated code → green.
Suggested fix: extend one non-PR test (e.g. 'still sweeps side files that match the target prefix', which already calls runCleanup('local') with existsSync true) with:
expect(mocks.rmSync).toHaveBeenCalledWith(
'/repo/.qwen/workflows/qwen-review-local-plan.js',
{ force: true },
);
plus a negative assertion that no fetch-shaped script is attempted for a non-PR target.
中文说明
新增的生成脚本清扫没有任何非 PR 覆盖：两个 workflow 清扫测试都跑 runCleanup('pr-123')，而非 PR 调用（'local'、'lease'）对其 rmSync 调用没有任何断言。
失败场景：维护者"顺手整理"，把 workflow 块挪进 if (prMatch) 分支（或用同一个 /^pr-\d+$/ 正则门禁）—— 两个 workflow 测试都用 'pr-123'，没有其他测试断言 workflow 删除，套件保持全绿。此后任何走了 workflow 路径的 local 或 file review（structuralBlocker 允许它们 —— 只拒绝 territory 扇出和 pr-worktree plan）运行 Step 9 cleanup 时不再删除脚本，用户的 saved-workflow 目录里留下永久的 /qwen-review-<digest> 斜杠命令 —— 恰是本块注释声称要防止的残留。
证据（探针）：把 workflow 块包进 if (/^pr-\d+$/u.test(target)) → 套件仍 38/38 全绿；加上下面的非 PR 断言 → 1 失败；该断言对未变异代码 → 绿。
建议修复：扩展某个非 PR 测试（例如已经以 existsSync true 调用 runCleanup('local') 的 'still sweeps side files that match the target prefix'），加上对 '/repo/.qwen/workflows/qwen-review-local-plan.js' 的 toHaveBeenCalledWith 断言，并补一条负断言：非 PR target 不会尝试 fetch 形状的脚本。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(mocks.rmSync).not.toHaveBeenCalledWith( | ||
| '/repo/.qwen/workflows/qwen-review-a749ec7145.js', | ||
| expect.anything(), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] 'removes only this target generated fan-out scripts' cannot catch over-deletion: the negative-assertion paths are reachable by no code path the fixture exercises — mocks.readdirSync returns [] and existsSync is true only for the dir and the two exact target paths, so any enumeration- or probe-based mutant sees an empty directory listing and stays green. The sweep's blast radius ("only this target") is unpinned.
Failure scenario: paths.ts's REVIEW_WORKFLOW_PREFIX comment ("the cleanup sweep globs it") invites a future orphan fix that replaces the two exact-path removals with a readdirSync(REVIEW_WORKFLOWS_DIR) enumeration; a version of that sweep scoped by prefix only (qwen-review-*.js) instead of per-target would delete a concurrent review's generated script for another PR/file in the same repo. In this test the mutant never calls rmSync on the negative-assertion paths and the suite stays green; the other review's instructed Workflow({scriptPath}) call then fails ENOENT at load mid-run.
witness (probe): unmodified fixture + prefix-scoped over-deleting mutant → test PASSED (mutant invisible); suggested fixture + same mutant → FAILED at the negative assertion; suggested fixture + unmodified implementation → 38/38 passed.
Suggested fix: populate the fixture with sibling entries so any future enumeration that touches another target's script turns the negative assertions red:
mocks.readdirSync.mockReturnValue([
'qwen-review-a749ec7145.js',
'qwen-review-checklist.js',
]);
plus existsSync true for them.
中文说明
'removes only this target generated fan-out scripts' 捕获不了过度删除：负断言路径对 fixture 演练的任何代码路径都不可达 —— mocks.readdirSync 返回 []，existsSync 只对目录和两个精确 target 路径为 true，于是任何基于枚举/探测的变异看到的是空目录列表，保持全绿。清扫的爆炸半径（"仅当前 target"）未被钉住。
失败场景：paths.ts 的 REVIEW_WORKFLOW_PREFIX 注释（"the cleanup sweep globs it"）诱使未来的孤儿修复用 readdirSync(REVIEW_WORKFLOWS_DIR) 枚举替换两处精确路径删除；若该清扫只按前缀（qwen-review-*.js）而非按 target 限定作用域，就会删掉同仓库中另一个 PR/文件并发 review 的生成脚本。在本测试中变异体从不对负断言路径调用 rmSync，套件保持全绿；而另一个 review 被指示的 Workflow({scriptPath}) 调用会在运行中加载时 ENOENT 失败。
证据（探针）：未修改 fixture + 按前缀过度删除的变异体 → 测试通过（变异不可见）；建议 fixture + 同一变异体 → 在负断言处失败；建议 fixture + 未修改实现 → 38/38 通过。
建议修复：给 fixture 填入同目录的其他条目（mocks.readdirSync.mockReturnValue(['qwen-review-a749ec7145.js', 'qwen-review-checklist.js'])，并令 existsSync 对它们为 true），使任何触碰其他 target 脚本的未来枚举都让负断言变红。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('does not remove a workflow through a symlinked root', () => { | ||
| mocks.execFileSync.mockReturnValue(Buffer.from('')); | ||
| mocks.existsSync.mockImplementation( | ||
| ((p: string) => p === '/repo/.qwen/workflows') as never, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This symlinked-root test's two negative rmSync assertions are vacuous: the existsSync mock returns true only for the directory, never for the two script paths, so if (!existsSync(workflowPath)) continue; blocks removal regardless of the guard — the property the test name claims is pinned only by the stderr-message assertion, and a warn-only guard mutation survives.
Failure scenario: refactor the cleanup guard to log-but-not-gate — e.g. set workflowRootSafe = true unconditionally while keeping the Skipping workflow cleanup: … print. In this test the script paths still read existsSync === false, so the loop continues before rmSync, both not.toHaveBeenCalledWith assertions pass, and the suite stays green. The guard has silently become decorative: on a machine where .qwen/workflows is a symlink to a directory (the configuration the loader, saveWorkflowScript, and this block's own message all declare illegal), cleanup now rmSynces the generated-script names through the symlink into whatever the user's link points at — the exact boundary escape R1-2's fix was added to prevent.
witness (probe, flipped both ways): guard broken + original test → 1 passed (negative assertions hold though cleanup would proceed through the symlink); guard broken + the fix below → 1 failed (rmSync called with both script paths through the symlinked root); correct guard + the fix → 1 passed.
| it('does not remove a workflow through a symlinked root', () => { | |
| mocks.execFileSync.mockReturnValue(Buffer.from('')); | |
| mocks.existsSync.mockImplementation( | |
| ((p: string) => p === '/repo/.qwen/workflows') as never, | |
| ); | |
| it('does not remove a workflow through a symlinked root', () => { | |
| mocks.execFileSync.mockReturnValue(Buffer.from('')); | |
| mocks.existsSync.mockImplementation( | |
| ((p: string) => | |
| p === '/repo/.qwen/workflows' || | |
| p === '/repo/.qwen/workflows/qwen-review-pr-123-plan.js' || | |
| p === '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js') as never, | |
| ); |
so the scripts exist in the symlink scenario and any mutation that lets workflowRootSafe be true under a symlinked root actually calls rmSync and fails the test.
中文说明
本符号链接根测试的两个负 rmSync 断言是空的：existsSync mock 只对目录返回 true，从不对两个脚本路径返回 true，于是 if (!existsSync(workflowPath)) continue; 无论守卫是否存在都会阻止删除 —— 测试名所声称的性质只由 stderr 消息断言钉住，warn-only 守卫变异可以存活。
失败场景：把 cleanup 守卫重构为"记录但不门禁"—— 例如无条件设 workflowRootSafe = true 同时保留 Skipping workflow cleanup: … 打印。在本测试中脚本路径依旧读到 existsSync === false，循环在 rmSync 之前 continue，两个 not.toHaveBeenCalledWith 断言通过，套件保持全绿。守卫已悄悄变成装饰：在 .qwen/workflows 符号链接到某个目录的机器上（加载器、saveWorkflowScript、本块自己的消息都宣称非法的配置），cleanup 现在会穿过符号链接对生成脚本名执行 rmSync，落到用户链接指向的任何地方 —— 恰是 R1-2 修复要防止的边界逃逸。
证据（探针，双向翻转）：守卫破坏 + 原测试 → 1 通过（负断言成立，尽管 cleanup 会穿过符号链接继续）；守卫破坏 + 下方修复 → 1 失败（rmSync 穿过符号链接根对两个脚本路径都被调用）；正确守卫 + 修复 → 1 通过。
建议修复（见上方 suggestion 块）：让脚本在符号链接场景中"存在"，使任何让 workflowRootSafe 在符号链接根下为 true 的变异真正调用 rmSync 并让测试失败。
— qwen3.8-max via Qwen Code /review (v0.21.13)
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: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 dry after reporting round 4).
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 20210, 1471, 1575, 494, 3686, 507 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/cleanup.ts:604 — [probe] writer↔sweeper digest divergence for safeTarget-transformed targets — dotfile file-reviews leak their generated script permanentlypackages/cli/src/commands/review/cleanup.test.ts:106 — [probe] vi.mock('./lib/paths.js') fabricates a divergent naming scheme — writer↔sweeper parity untestedpackages/cli/src/commands/review/cleanup.ts:641 — [probe] the workflow-sweep rmSync failure branch (Failed to remove + failedAny) is unreachable from any testpackages/cli/src/commands/review/emit-workflow.test.ts:211 — [probe] legacy-routing tests never assert that no script file was written (round-2 deferred, re-confirmed)packages/cli/src/commands/review/workflow-script.test.ts:36 — [probe] the generated script's two log() lines are unpinned — deletion mutants ship green
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 dry after reporting round 4)。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 20210, 1471, 1575, 494, 3686, 507 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('routes to legacy without writing anything when workflows are disabled', () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); | ||
| const priorExit = process.exitCode; | ||
| try { | ||
| const plan = join(dir, 'plan.json'); | ||
| writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); | ||
| (emitWorkflowCommand.handler as (a: unknown) => void)({ plan }); | ||
| expect(process.exitCode).toBe(EXIT_LEGACY_ORCHESTRATION); |
There was a problem hiding this comment.
[Suggestion] R1-7: Still stands — nothing anywhere pins the literal exit code 6 that SKILL.md's routing branch keys on; both gate tests assert against the imported EXIT_LEGACY_ORCHESTRATION constant, a tautological pin (grep across the review suites finds no literal toBe(6) for this command). Failure scenario: SKILL.md branches on the literal 6; if the constant is ever renumbered, every gate test moves with it and stays green while the skill's routing silently breaks. Suggested fix: pin the literal once, e.g. expect(EXIT_LEGACY_ORCHESTRATION).toBe(6), beside the existing constant-based assertions.
中文说明
R1-7：仍然成立 —— 没有任何测试钉住 SKILL.md 路由分支所依赖的字面退出码 6；两个 gate 测试都只对导入的 EXIT_LEGACY_ORCHESTRATION 常量断言，属于同义反复式钉法（review 相关测试套件中 grep 不到该命令的 toBe(6)）。失败场景：SKILL.md 按字面 6 分支；若常量被改号，所有 gate 测试会跟着一起移动而继续绿灯，skill 的路由则悄然失效。建议修复：在常量断言旁补一条对字面量的钉法，如 expect(EXIT_LEGACY_ORCHESTRATION).toBe(6)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // `check-coverage` matches launches against, so records left by a fan-out | ||
| // that never dispatched would read as a roster that ran and returned | ||
| // nothing — a review reported as covered by agents that do not exist. | ||
| it('routes to legacy without writing anything when workflows are disabled', () => { |
There was a problem hiding this comment.
[Suggestion] R1-3: Still stands — the two legacy-routing tests stub no gate env vars, but runEmitWorkflow → resolveOrchestration defaults to process.env, and neither test-setup.ts nor vitest.config.ts clears them. The sibling test stubs only the runtime gate and assumes ambient QWEN_REVIEW_WORKFLOW is unset. Failure scenario (probe-reproduced): with QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1 exported — the author's own dev state for the happy path — the handler resolves workflow mode, writes the script and records, never sets exitCode 6, and both tests fail spuriously (the probe run also left two real scripts in packages/cli/.qwen/workflows/). Suggested fix: delete/stub both gate vars explicitly in the first test (with restore), and additionally remove QWEN_REVIEW_WORKFLOW before stubbing the runtime gate in the second.
中文说明
R1-3：仍然成立 —— 两个 legacy 路由测试都没有 stub 任何 gate 环境变量，而 runEmitWorkflow → resolveOrchestration 默认读取 process.env，test-setup.ts 与 vitest.config.ts 也都不清理它们。相邻的第二个测试只 stub 了运行时 gate，默认环境中 QWEN_REVIEW_WORKFLOW 未设置。失败场景（已用探针复现）：当导出 QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1（作者开发 happy path 时的自身环境）时，handler 解析为 workflow 模式、写入脚本与记录、从不设置 exitCode 6，两个测试都会假失败（探针运行还在 packages/cli/.qwen/workflows/ 留下了两个真实脚本）。建议修复：第一个测试显式删除/stub 两个 gate 变量（并恢复），第二个测试在 stub 运行时 gate 前先删除 QWEN_REVIEW_WORKFLOW。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); | ||
| vi.stubEnv('QWEN_REVIEW_WORKFLOW', '1'); |
There was a problem hiding this comment.
[Suggestion] R1-9: Still stands — this describe pins only two of the three gate variables: QWEN_CODE_DISABLE_WORKFLOWS is left at its ambient value, so the documented kill-switch rollback configuration breaks the suite. Failure scenario: a developer or CI exporting QWEN_CODE_DISABLE_WORKFLOWS=1 (the documented one-switch rollback) runs this file — every write test resolves legacy and fails spuriously. Suggested fix: vi.stubEnv('QWEN_CODE_DISABLE_WORKFLOWS', '0') — or delete the var — in this beforeEach, mirroring the two existing stubs.
中文说明
R1-9：仍然成立 —— 该 describe 只钉住三个 gate 变量中的两个：QWEN_CODE_DISABLE_WORKFLOWS 保持环境原值，因此文档中记载的 kill-switch 回滚配置会直接打破整个套件。失败场景：导出 QWEN_CODE_DISABLE_WORKFLOWS=1（文档记载的一键回滚配置）的开发者或 CI 运行该文件时，所有写入类测试都会解析为 legacy 而假失败。建议修复：在此 beforeEach 中镜像现有两条 stub，补上 vi.stubEnv('QWEN_CODE_DISABLE_WORKFLOWS', '0') 或删除该变量。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // `readWorkflowFileSecurely`, which realpaths the file and refuses anything | ||
| // outside the saved-workflow directories. A script beside the plan is a | ||
| // script the tool will not open. | ||
| it('writes the script where the Workflow loader will accept it', () => { |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands — the happy path asserts the script file but never the exit-code/stdout contract SKILL.md's routing section branches on: no success-path test checks that process.exitCode stays 0, and none captures the scriptPath: <abs path> stdout line the new SKILL.md branch consumes for its one Workflow({ scriptPath }) call. Failure scenario (mutation-probed): setting process.exitCode = EXIT_LEGACY_ORCHESTRATION after the successful write kept the suite 14/14 green while every opted-in review would silently route to legacy with script and records already on disk; a dropped or reworded scriptPath: line leaves the orchestrator with no path to dispatch while the records read as a roster that ran. Suggested fix: assert expect(process.exitCode ?? 0).toBe(0) in the success tests, and mock ../../utils/stdioHelpers.js (as cleanup.test.ts does) to assert a call matching /^scriptPath: \// with an absolute argument.
中文说明
R1-4：仍然成立 —— happy path 只断言了脚本文件，从未断言 SKILL.md 路由分支所依赖的退出码/stdout 契约：没有成功路径测试检查 process.exitCode 保持 0，也没有任何测试捕获新版 SKILL.md 分支用来发起唯一一次 Workflow({ scriptPath }) 调用的 scriptPath: <abs path> 标准输出行。失败场景（已做变异探针）：在成功写入后设置 process.exitCode = EXIT_LEGACY_ORCHESTRATION，套件仍 14/14 全绿，而所有已 opt-in 的 review 都会带着已落盘的脚本与记录被静默路由回 legacy；scriptPath: 行被删除或改写时，编排器拿不到可分派的路径，而记录读起来却像 roster 已经运行过。建议修复：成功路径测试中断言 expect(process.exitCode ?? 0).toBe(0)，并像 cleanup.test.ts 一样 mock ../../utils/stdioHelpers.js，断言收到匹配 /^scriptPath: \// 且参数为绝对路径的调用。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| }); | ||
|
|
||
| describe('the env predicates', () => { | ||
| it('workflowsEnabled: opt-in, with the kill switch winning', () => { |
There was a problem hiding this comment.
[Suggestion] R1-11: Still stands — the suite pins exact-'1' semantics for reviewWorkflowEnabled ('true'/'0' cases) but none for workflowsEnabled, whose stated reason to exist is mirroring Config.isWorkflowsEnabled's env half (strict === '1' on both enable and disable). Failure scenario: relaxing workflowsEnabled to a truthy check ships green while diverging from the runtime gate it mirrors, enabling the workflow verdict for configurations the runtime refuses. Suggested fix: add expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false) (and the kill-switch mirror) beside the existing cases.
中文说明
R1-11：仍然成立 —— 套件为 reviewWorkflowEnabled 钉住了精确 '1' 语义（'true'/'0' 用例），却没有为 workflowsEnabled 钉住 —— 而后者存在的理由就是镜像 Config.isWorkflowsEnabled 的环境变量半边（enable 与 disable 都严格 === '1'）。失败场景：把 workflowsEnabled 放宽为 truthy 判断会在绿灯下与它所镜像的运行时 gate 漂移，使运行时拒绝的配置也能拿到 workflow 裁决。建议修复：在现有用例旁补上 expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false)（以及 kill-switch 的镜像用例）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| } | ||
|
|
||
| /** Absolute-or-relative path of the generated fan-out script for a plan. */ | ||
| export function reviewWorkflowScriptPath(planPath: string): string { |
There was a problem hiding this comment.
[Suggestion] R1-14: Still stands — the generated script's destination is cwd-relative, but the loader boundary is rooted at the session's project root (Storage.getProjectWorkflowsDir()) plus user scope, and cleanup sweeps only the cwd-relative dir. Failure scenario: any invocation with cwd ≠ project root (a subshell, a wrapper, a future skill recipe) emits a script the Workflow loader refuses — the fan-out fails at dispatch after a successful emit — and leaves it unswept by a root-run cleanup. Suggested fix: root the destination at the resolved project directory (the way Storage.getProjectWorkflowsDir() does) on both the write and sweep sides, or assert cwd containment in runEmitWorkflow.
中文说明
R1-14：仍然成立 —— 生成脚本的落点是相对 cwd 的，但加载器边界以会话的项目根（Storage.getProjectWorkflowsDir()）加用户作用域为根，而 cleanup 只清扫 cwd 相对目录。失败场景：任何 cwd ≠ 项目根 的调用（子 shell、包装脚本、未来的 skill 配方）都会产出一份 Workflow 加载器拒绝的脚本 —— emit 成功后分派才失败 —— 并且从项目根运行的 cleanup 也扫不到它。建议修复：写入与清扫两侧都把落点钉到解析后的项目目录（与 Storage.getProjectWorkflowsDir() 一致），或在 runEmitWorkflow 中断言 cwd 包含关系。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const logs: string[] = []; | ||
| const phases: string[] = []; | ||
|
|
||
| const agent = async (prompt: string, opts: unknown) => { |
There was a problem hiding this comment.
[Suggestion] R1-8: Still stands — the executable harness's fake agent() accepts opts opaquely, but the real vm-realm agent() throws on any key outside KNOWN_AGENT_OPTS — no test in this suite can catch an opts-key mutation in FAN_OUT_BODY. Failure scenario: a mutation adding or renaming an opts key in FAN_OUT_BODY ships green here and then throws in the real vm realm for every dispatched agent — total fan-out failure discovered only in a live run. Suggested fix: have the fake agent() assert opts keys against the real KNOWN_AGENT_OPTS allowlist (import it from the sandbox module), or pin the exact opts object the script sends.
中文说明
R1-8：仍然成立 —— 可执行 harness 里的假 agent() 对 opts 照单全收，而真实 vm 域的 agent() 会对 KNOWN_AGENT_OPTS 之外的任何键抛错 —— 本套件没有任何测试能捕获 FAN_OUT_BODY 中 opts 键的变异。失败场景：在 FAN_OUT_BODY 中新增或改名一个 opts 键，这里绿灯通过，随后在真实 vm 域中每个被分派的 agent 都抛错 —— 只有实际运行时才会发现整个扇出失败。建议修复：让假 agent() 对照真实的 KNOWN_AGENT_OPTS 白名单断言 opts 键（从 sandbox 模块导入），或直接钉住脚本发送的精确 opts 对象。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ]; | ||
|
|
||
| describe('the generated Step 3A fan-out script', () => { | ||
| it('opens with a meta block the sandbox will accept as a pure literal', () => { |
There was a problem hiding this comment.
[Suggestion] R1-15: Still stands — the generated meta block's shape-validity is pinned by no test: this harness converts export const meta = itself instead of exercising the sandbox's real strip/extract path, and the assertions check lexical shape only. Failure scenario: a meta-block mutation the sandbox's stripper rejects (a comment before the block, a computed field) ships green here and fails at Workflow load time for every dispatched review. Suggested fix: run the emitted script through the sandbox's real meta extraction/validation (imported from workflow-sandbox.ts) in at least one test, instead of the harness's own regex strip.
中文说明
R1-15：仍然成立 —— 生成的 meta 块的形状有效性没有任何测试钉住：本 harness 自己转换 export const meta =，而不是走 sandbox 真实的剥离/提取路径，断言也只检查词法形状。失败场景：一个会被 sandbox 剥离器拒绝的 meta 块变异（块前带注释、计算字段等）在这里绿灯通过，然后在每次分派的 Workflow 加载时失败。建议修复：至少在一个测试里让产出的脚本通过 sandbox 真实的 meta 提取/校验（从 workflow-sandbox.ts 导入），而不是 harness 自己的正则剥离。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // that dir is also the user's own saved workflows, where each file is a | ||
| // `/<name>` slash command. A review that left one behind would hand the user | ||
| // a permanent command for a diff that no longer exists. | ||
| it('removes only this target generated fan-out scripts', () => { |
There was a problem hiding this comment.
[Suggestion] R2-1: Still stands — the new generated-workflow sweep has zero non-PR test coverage against the rewritten deterministic-path design: all six workflow tests run runCleanup('pr-123'), while the non-PR calls ('local', filename targets) assert nothing about it. Failure scenario: a regression in the non-PR derivation path — the exact divergence class the round-3 probe demonstrated for dotfile targets (/review .eslintrc: safeTarget strips the leading dot, cleanup's digest disagrees with emit-workflow's, the sweep silently no-ops) — ships green and leaks permanent /qwen-review-<digest> slash commands for every local/file review. Suggested fix: add sweep tests for at least one 'local' and one filename target, asserting rmSync receives the paths emit-workflow would actually write for those plans.
中文说明
R2-1：仍然成立 —— 针对重写后的确定性路径设计，新的生成 workflow 清扫依然没有任何非 PR 测试覆盖：六个 workflow 测试全部运行 runCleanup('pr-123')，而非 PR 调用（'local'、文件名 target）对其没有任何断言。失败场景：非 PR 推导路径上的回归 —— 正是第 3 轮探针在点文件 target 上演示的那类分歧（/review .eslintrc：safeTarget 剥掉前导点，cleanup 的 digest 与 emit-workflow 的不一致，清扫静默空转）—— 会绿灯放行，使每次 local/file review 都泄漏一条永久的 /qwen-review-<digest> 斜杠命令。建议修复：至少为一个 'local' 和一个文件名 target 增加清扫测试，断言 rmSync 收到的是 emit-workflow 实际会写入的路径。
— qwen3.8-max via Qwen Code /review (v0.21.13)
已被后续 commit 取代，当前 head 需重新 review
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. Suggestions are inline.
Not reviewed: reverse audit — stopped at the 5-round cap without two certified dry rounds (round 5's sole finding was rejected in verification, but the builder's retirement ledger never certified the dry receipts).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": verify SKILL.md's routing claims (exit 6 = legacy, exit 0 = workflow, scriptPath: stdout format, prompt-record-on-disk claim) and env-var names ( QWEN_CODE_EN…; "agent reverse-audit (round 1)": check DEFAULT_MAX_AGENTS_PER_RUN in workflow.ts against the 14-agent full roster to rule out over-cap dispatches degrading to missingRoles at runtime.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 20210, 1471, 1575, 494, 3686, 507 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/paths.ts:69 — [review] REVIEW_WORKFLOW_PREFIX comment claims 'the cleanup sweep globs it' — no such glob exists; cleanup removes only exact per-target pathspackages/cli/src/commands/review/lib/report.ts:80 — [probe] readPlanReport insertion orphans the PlanReport interface's doc comment (TSDoc probe: PlanReport's doc resolves empty)packages/cli/src/commands/review/cleanup.test.ts:108 — [review] vi.mock('./lib/paths.js') fabricates a divergent naming scheme — writer↔sweeper script-path parity untestedpackages/cli/src/commands/review/emit-workflow.test.ts:211 — [probe] legacy-routing tests never assert that no script file was written (mutation probe passes both tests green)packages/cli/src/commands/review/cleanup.ts:605 — [review] fetch.json-derived workflow cleanup entry targets a script nothing ever writes (dead arm, undocumented as forward-compat)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — stopped at the 5-round cap without two certified dry rounds (round 5's sole finding was rejected in verification, but the builder's retirement ledger never certified the dry receipts)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：verify SKILL.md's routing claims (exit 6 = legacy, exit 0 = workflow, scriptPath: stdout format, prompt-record-on-disk claim) and env-var names ( QWEN_CODE_EN…；"agent reverse-audit (round 1)"：check DEFAULT_MAX_AGENTS_PER_RUN in workflow.ts against the 14-agent full roster to rule out over-cap dispatches degrading to missingRoles at runtime。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 20210, 1471, 1575, 494, 3686, 507 passed。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const temporaryPath = `${scriptPath}.${randomUUID()}.tmp`; | ||
| writeFileSync(temporaryPath, buildReviewWorkflowScript(agents), { |
There was a problem hiding this comment.
[Suggestion] The cleanup finally wraps only renameSync; a writeFileSync failure after the temp file is created orphans .qwen/workflows/qwen-review-<digest>.js.<uuid>.tmp — a path shape the cleanup sweep never covers (it removes only the two deterministic script paths). A kill between temp-create and rename leaves the same orphan shape.
Failure scenario: writeFileSync fails after creating the file (ENOSPC/EIO mid-write) → it throws before the try block → rmSync(temporaryPath) in finally never runs → the orphan persists in the saved-workflow directory forever, surviving every future cleanup of that target.
Witness (probe on the real handler, fs mocked for ENOSPC-mid-write):
PR: PROBE LEFT-BEHIND: ["qwen-review-9a205d8d01.js.8095c532-….tmp"]
FIXED: PROBE LEFT-BEHIND: [] (writeFileSync moved inside the try)
Suggested fix — wrap write and rename in one try/finally:
| const temporaryPath = `${scriptPath}.${randomUUID()}.tmp`; | |
| writeFileSync(temporaryPath, buildReviewWorkflowScript(agents), { | |
| const temporaryPath = `${scriptPath}.${randomUUID()}.tmp`; | |
| try { | |
| writeFileSync(temporaryPath, buildReviewWorkflowScript(agents), { | |
| encoding: 'utf8', | |
| flag: 'wx', | |
| }); | |
| renameSync(temporaryPath, scriptPath); | |
| } finally { | |
| rmSync(temporaryPath, { force: true }); | |
| } |
中文说明
清理用的 finally 只包住了 renameSync；若 writeFileSync 在临时文件创建后失败，会遗留 .qwen/workflows/qwen-review-<digest>.js.<uuid>.tmp 孤儿文件 —— cleanup 清扫只删两条确定性脚本路径，永远不会覆盖这个形状。在临时文件创建与 rename 之间被杀死也会留下同样的孤儿。
失败场景：writeFileSync 在建文件后失败（写到一半 ENOSPC/EIO）→ 在进入 try 之前抛出 → finally 里的 rmSync(temporaryPath) 不会执行 → 孤儿永久留在 saved-workflow 目录，之后该 target 的每次 cleanup 都清不掉它。
证据（对真实 handler 的探针，mock fs 模拟写到一半 ENOSPC）：PR 代码遗留孤儿文件；把 writeFileSync 移入 try 后遗留为空（翻转）。
建议修复：把写入与 rename 包进同一个 try/finally（见上方 suggestion 块）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // `check-coverage` matches launches against, so records left by a fan-out | ||
| // that never dispatched would read as a roster that ran and returned | ||
| // nothing — a review reported as covered by agents that do not exist. | ||
| it('routes to legacy without writing anything when workflows are disabled', () => { |
There was a problem hiding this comment.
[Suggestion] R1-3: Still stands — the two legacy-routing handler tests read the routing gates from ambient process.env: this test stubs nothing, the sibling stubs only QWEN_CODE_ENABLE_WORKFLOWS; runEmitWorkflow → resolveOrchestration defaults to process.env, and neither test-setup.ts nor vitest.config.ts clears these variables. The sibling orchestration.test.ts avoids the class by injecting explicit env objects.
Failure scenario: in a shell with QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1 (the feature's own documented dogfood/A-B environment) this test takes the workflow route — since the describe never chdirs, it mkdirs .qwen/workflows/ under the ambient cwd and writes a stray generated script into the working tree before the exit-code assertion fails; an ambient QWEN_CODE_DISABLE_WORKFLOWS=1 conversely survives the stubs and routes the what it writes block to legacy.
Witness (probe on the unmodified PR, same file): base with env scrubbed 16 passed; with both gates set 2 failed | 14 passed and stray scripts written into the working tree (.qwen/workflows/qwen-review-d745bf14a1.js, qwen-review-907e53792a.js); with the kill switch set 5 failed | 11 passed.
Suggested fix: pin all three gates deterministically — vi.stubEnv('QWEN_CODE_DISABLE_WORKFLOWS', '1') here (the kill switch wins over every opt-in), and additionally vi.stubEnv('QWEN_REVIEW_WORKFLOW', '') in the sibling; vi.unstubAllEnvs() already runs in both finally blocks.
中文说明
R1-3：仍然成立 —— 两个 legacy 路由 handler 测试从环境 process.env 读取路由开关：本测试什么也不 stub，兄弟测试只 stub 了 QWEN_CODE_ENABLE_WORKFLOWS；runEmitWorkflow → resolveOrchestration 默认使用 process.env，而 test-setup.ts 与 vitest.config.ts 都不清理这些变量。兄弟文件 orchestration.test.ts 通过注入显式 env 对象避免了这一类问题。
失败场景：在设置了 QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1 的 shell（本功能自己文档化的 dogfood/A-B 环境）里，该测试会走 workflow 路径 —— 由于这个 describe 从不 chdir，它会在环境 cwd 下创建 .qwen/workflows/ 并往工作树里写入一个游离的生成脚本，然后才在退出码断言上失败；反过来，环境中的 QWEN_CODE_DISABLE_WORKFLOWS=1 会穿透 stub，把 what it writes 整块路由到 legacy。
证据（对未修改 PR 的探针）：清空 env 的基线 16 passed；设置两个开关后 2 failed | 14 passed 且工作树中出现游离脚本；设置 kill switch 后 5 failed | 11 passed。
建议修复：确定性地钉住全部三个开关 —— 此处 vi.stubEnv('QWEN_CODE_DISABLE_WORKFLOWS', '1')（kill switch 胜过一切 opt-in），兄弟测试再加 vi.stubEnv('QWEN_REVIEW_WORKFLOW', '')；两个 finally 块已有 vi.unstubAllEnvs()。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| beforeEach(() => { | ||
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); | ||
| vi.stubEnv('QWEN_REVIEW_WORKFLOW', '1'); |
There was a problem hiding this comment.
[Suggestion] R1-9: Still stands — this describe pins only two of the three gate variables: QWEN_CODE_DISABLE_WORKFLOWS is left at its ambient value, so the documented kill-switch rollback configuration breaks the suite. vi.unstubAllEnvs() restores only stubbed vars, so the un-stubbed kill switch survives.
Failure scenario: a shell carrying QWEN_CODE_DISABLE_WORKFLOWS=1 — the one-flip rollback this very PR ships — routes every handler-driven test in the describe to legacy; the whole block fails red for reasons unrelated to the change under test.
Witness (probe): QWEN_CODE_DISABLE_WORKFLOWS=1 on the PR as-is → Tests 5 failed | 11 passed (16), all 5 failures in what it writes; same env with the one-line stub below → Tests 16 passed (16).
| beforeEach(() => { | |
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); | |
| vi.stubEnv('QWEN_REVIEW_WORKFLOW', '1'); | |
| beforeEach(() => { | |
| vi.stubEnv('QWEN_CODE_ENABLE_WORKFLOWS', '1'); | |
| vi.stubEnv('QWEN_REVIEW_WORKFLOW', '1'); | |
| vi.stubEnv('QWEN_CODE_DISABLE_WORKFLOWS', ''); |
中文说明
R1-9：仍然成立 —— 这个 describe 只钉住了三个门禁变量中的两个：QWEN_CODE_DISABLE_WORKFLOWS 保持环境值，于是文档化的 kill-switch 回滚配置会弄坏整个测试套件。vi.unstubAllEnvs() 只恢复被 stub 过的变量，未 stub 的 kill switch 会穿透。
失败场景：带有 QWEN_CODE_DISABLE_WORKFLOWS=1 的 shell —— 正是本 PR 自带的一键回滚配置 —— 会把 describe 里所有 handler 驱动的测试路由到 legacy；整块测试因与被测变更无关的原因全红。
证据（探针）：PR 原样 + kill switch → 5 failed | 11 passed（5 个失败全在 what it writes）；加上下方一行 stub 后 → 16 passed。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // `readWorkflowFileSecurely`, which realpaths the file and refuses anything | ||
| // outside the saved-workflow directories. A script beside the plan is a | ||
| // script the tool will not open. | ||
| it('writes the script where the Workflow loader will accept it', () => { |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands — the happy path asserts the script file but never the exit-code/stdout contract SKILL.md's routing branch keys on: no success-path test checks that process.exitCode stays 0/undefined; the exit-code assertions exist only in the two legacy-routing tests.
Failure scenario: a refactor leaking process.exitCode = EXIT_LEGACY_ORCHESTRATION into the success path (the assignment is one branch away in runEmitWorkflow) makes an eligible review with a perfectly written script silently run the legacy path — corrupting the A/B this change exists to run — with every current test green.
Suggested fix: in this test, save/restore process.exitCode around the handler call and assert it is undefined (or 0) afterwards.
中文说明
R1-4：仍然成立 —— happy path 断言了脚本文件，却从不断言 SKILL.md 路由分支所依赖的退出码/stdout 契约：没有任何成功路径测试检查 process.exitCode 保持 0/undefined；退出码断言只存在于两个 legacy 路由测试中。
失败场景：一次把 process.exitCode = EXIT_LEGACY_ORCHESTRATION 泄漏进成功路径的重构（该赋值在 runEmitWorkflow 里只隔一个分支），会让一个脚本已正确写好的合格 review 静默走 legacy 路径 —— 破坏本变更存在意义所在的 A/B —— 而现有测试全绿。
建议修复：在本测试中于 handler 调用前后保存/恢复 process.exitCode，并断言其后为 undefined（或 0）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| let rules: string | undefined; | ||
| if (args.rules) { |
There was a problem hiding this comment.
[Suggestion] R1-5: Still stands — the --rules option has no test on this command's path: neither the success wiring (readFileSync(args.rules) → buildFanOutRoster(report, planPath, rules) → buildLaunch(…, rules)) nor the unreadable-path refusal is exercised — grep for rules in emit-workflow.test.ts returns zero matches, and the legacy --roster path's rules handling is a different call site.
Failure scenario: if the wiring regresses (rules dropped before buildLaunch, or the read moves after the roster build), every workflow-path review silently runs without the project rules it was told to enforce — exactly the failure the error message says the refusal exists to prevent — and no test fails.
Suggested fix: add a test that writes a rules file, calls the handler with { plan, rules } under both gates, and asserts each recorded prompt contains the rules text; plus one asserting an unreadable --rules path throws naming the path, before any script/record is written.
中文说明
R1-5：仍然成立 —— --rules 选项在本命令路径上没有任何测试：成功接线（readFileSync(args.rules) → buildFanOutRoster(report, planPath, rules) → buildLaunch(…, rules)）与不可读路径的拒绝都未被执行 —— 在 emit-workflow.test.ts 中 grep rules 零匹配，而 legacy --roster 路径的 rules 处理是另一个调用点。
失败场景：若接线退化（rules 在 buildLaunch 前被丢弃，或读取挪到 roster 构建之后），每一次 workflow 路径的 review 都会静默地不带项目规则运行 —— 正是错误信息声称该拒绝存在所要防止的失败 —— 且没有测试会失败。
建议修复：新增一个测试，写入规则文件、在两个开关都打开时以 { plan, rules } 调用 handler，并断言每条记录的 prompt 都含规则文本；再断言不可读的 --rules 路径会在写入任何脚本/记录前抛出并点名该路径。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // The reason a run gives should be the one its reader can act on: "you have | ||
| // not enabled workflows" beats "this plan is a territory fan-out" when both | ||
| // are true, because the first is what stands between them and any workflow. | ||
| it('reports the most global blocker first', () => { |
There was a problem hiding this comment.
[Suggestion] R1-10: Still stands — no test combines open env gates with a structurally blocked plan, so resolveOrchestration's delegation to structuralBlocker is an unpinned composition (the blocked-plan tests either call structuralBlocker directly or run resolveOrchestration with closed gates).
Failure scenario: deleting or reordering the structuralBlocker branch from resolveOrchestration would let an eligible-gates territory/worktree plan through as workflow — dispatching a 3A-shaped fan-out for a 3B review — with every current test green.
Suggested fix: add expect(resolveOrchestration(localPlan({ srcDiffLines: 2000, diffLines: 6000 }), ON).mode).toBe('legacy') (and the worktree variant), asserting the structural reason text.
中文说明
R1-10：仍然成立 —— 没有测试把打开的 env 门禁与结构性阻塞的 plan 组合起来，因此 resolveOrchestration 对 structuralBlocker 的委托是一个未被钉住的组合（被阻塞 plan 的测试要么直接调用 structuralBlocker，要么在门禁关闭时运行 resolveOrchestration）。
失败场景：从 resolveOrchestration 中删除或重排 structuralBlocker 分支，会让门禁打开的 territory/worktree plan 以 workflow 放行 —— 为 3B review 分派 3A 形状的扇出 —— 而现有测试全绿。
建议修复：补充 expect(resolveOrchestration(localPlan({ srcDiffLines: 2000, diffLines: 6000 }), ON).mode).toBe('legacy')（以及 worktree 变体），并断言结构性原因文本。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| }); | ||
|
|
||
| describe('the env predicates', () => { | ||
| it('workflowsEnabled: opt-in, with the kill switch winning', () => { |
There was a problem hiding this comment.
[Suggestion] R1-11: Still stands — the suite pins exact-'1' semantics for reviewWorkflowEnabled ('true'/'0' cases) but none for workflowsEnabled, whose stated reason to exist is mirroring Config.isWorkflowsEnabled's env half — strict === '1' on boolean-ish values is the very behaviour under test.
Failure scenario: relaxing workflowsEnabled to truthy-string semantics (or tightening Config's side) silently diverges the CLI's routing verdict from the runtime's own gate: reviews route to a workflow path the runtime refuses, or vice versa, with no red test.
Suggested fix: mirror the sibling test — expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false) and the '0' case.
中文说明
R1-11：仍然成立 —— 套件为 reviewWorkflowEnabled 钉住了精确 '1' 语义（'true'/'0' 用例），却没有为 workflowsEnabled 钉住 —— 而后者的存在理由就是镜像 Config.isWorkflowsEnabled 的 env 半边 —— 对布尔风格取值的严格 === '1' 正是被测行为本身。
失败场景：把 workflowsEnabled 放宽为任意真值字符串语义（或收紧 Config 一侧），会让 CLI 的路由裁决与运行时自己的门禁静默背离：review 被路由到运行时拒绝的 workflow 路径，或相反，且无红测试。
建议修复：镜像兄弟测试 —— expect(workflowsEnabled({ QWEN_CODE_ENABLE_WORKFLOWS: 'true' })).toBe(false) 以及 '0' 用例。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| } | ||
|
|
||
| /** Absolute-or-relative path of the generated fan-out script for a plan. */ | ||
| export function reviewWorkflowScriptPath(planPath: string): string { |
There was a problem hiding this comment.
[Suggestion] R1-14: Still stands — the generated script's destination is cwd-relative (join('.qwen', 'workflows')), but the loader boundary is rooted at the session's project root (Storage.getProjectWorkflowsDir()) plus user scope, and cleanup sweeps only the cwd-relative dir — so emit-workflow invoked from a cwd other than the project root writes a script the loader may refuse and cleanup will never find.
Failure scenario: a caller that runs emit-workflow from a subdirectory (a tool harness, a nested workspace, an aliased wrapper) lands the script in <subdir>/.qwen/workflows/; Workflow({scriptPath}) resolves against the session root and refuses it (or loads a different file), and cleanup <target> recomputes the same wrong-relative path, so the orphan persists in both directions.
Suggested fix: root REVIEW_WORKFLOWS_DIR at the project root the review commands already resolve (or assert cwd === project root at command entry), and add a test pinning the emit/cleanup/loader triple against a non-root cwd.
中文说明
R1-14：仍然成立 —— 生成脚本的落点是 cwd 相对的（join('.qwen', 'workflows')），但加载器边界以会话的项目根（Storage.getProjectWorkflowsDir()）为根并含用户作用域，而 cleanup 只清扫 cwd 相对目录 —— 于是从项目根之外的 cwd 调用 emit-workflow 会写出一个加载器可能拒绝、cleanup 永远找不到的脚本。
失败场景：从子目录运行 emit-workflow 的调用方（工具 harness、嵌套工作区、别名包装）会把脚本落在 <subdir>/.qwen/workflows/；Workflow({scriptPath}) 按会话根解析并拒绝它（或加载到另一个文件），而 cleanup <target> 重新计算出同样错误的相对路径，孤儿在两个方向上都持续存在。
建议修复：把 REVIEW_WORKFLOWS_DIR 钉在 review 命令已解析的项目根上（或在命令入口断言 cwd === 项目根），并补一个测试在非根 cwd 下钉住 emit/cleanup/loader 三元组。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ]; | ||
|
|
||
| describe('the generated Step 3A fan-out script', () => { | ||
| it('opens with a meta block the sandbox will accept as a pure literal', () => { |
There was a problem hiding this comment.
[Suggestion] R1-15: Still stands — the generated meta block's shape-validity is pinned by no test against the real validator: this harness converts export const meta = to const meta = itself instead of exercising the sandbox's real strip/extract path, and the assertions check only lexical purity (no ${/function/spread) — not validateMeta's requirements (non-empty name/description, phases[].title).
Failure scenario: a mutation that empties meta.description, drops phases, or makes the meta block impure in a way the lexical regex does not see ships green; the real sandbox then refuses the script at load time for every workflow-path review.
Suggested fix: assert the generated meta against the real contract — export validateMeta/findMetaBlockBounds from core for the test, or pin a golden meta object and run the harness through the actual strip path.
中文说明
R1-15：仍然成立 —— 生成的 meta 块的形状合法性没有任何测试对着真实校验器钉住：本 harness 自己把 export const meta = 转成 const meta =，而非执行沙箱真实的剥离/提取路径；断言只检查词法纯净度（无 ${/function/spread）—— 并不覆盖 validateMeta 的要求（非空 name/description、phases[].title）。
失败场景：把 meta.description 置空、删掉 phases、或以词法正则看不到的方式弄脏 meta 块的变异都能全绿通过；随后真实沙箱在加载时拒绝该脚本，波及每一次 workflow 路径的 review。
建议修复：让生成的 meta 对着真实契约断言 —— 从 core 导出 validateMeta/findMetaBlockBounds 供测试使用，或钉一个黄金 meta 对象并让 harness 走真实的剥离路径。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // that dir is also the user's own saved workflows, where each file is a | ||
| // `/<name>` slash command. A review that left one behind would hand the user | ||
| // a permanent command for a diff that no longer exists. | ||
| it('removes only this target generated fan-out scripts', () => { |
There was a problem hiding this comment.
[Suggestion] R2-1: Still stands — the new generated-workflow sweep has zero non-PR test coverage against the rewritten deterministic-path design: all six workflow tests run runCleanup('pr-123'), while the non-PR calls ('local', 'lease') assert nothing about its rmSync calls.
Failure scenario: the pr-\d+-gated fetch.json arm is the only target-shape branch in the block; a regression in the plan-path derivation for non-PR targets (local/file reviews — the only shapes that can actually take the workflow path today, since PR-worktree reviews are structurally refused) leaves their scripts unswept with no red test, accumulating permanent /qwen-review-<digest> slash commands.
Suggested fix: add a runCleanup('local') workflow test asserting rmSync of reviewWorkflowScriptPath(tmpFile('local', 'plan.json')) and non-removal of a foreign digest name.
中文说明
R2-1：仍然成立 —— 重写为确定性路径设计后，新的生成脚本清扫在非 PR 目标上测试覆盖为零：全部六个 workflow 测试都运行 runCleanup('pr-123')，而非 PR 调用（'local'、'lease'）对其 rmSync 调用毫无断言。
失败场景：pr-\d+ 门控的 fetch.json 分支是该块中唯一的目标形状分支；非 PR 目标（local/file review —— 由于 PR-worktree review 被结构性拒绝，它们是当前唯一能走 workflow 路径的形状）在 plan 路径推导上的退化会使其脚本不被清扫且无红测试，累积出永久的 /qwen-review-<digest> 斜杠命令。
建议修复：补一个 runCleanup('local') 的 workflow 测试，断言 rmSync 了 reviewWorkflowScriptPath(tmpFile('local', 'plan.json')) 且未删除外部 digest 名字。
— qwen3.8-max via Qwen Code /review (v0.21.13)
PR #8943 — maintainer local verification round (sandboxed container)Verdict: 中文摘要(点击展开)结论:
Central claim + A/B tableCentral claim: with both Driven through the built CLI (
The C1 head cell and the base control are witnessed below. 01 — head arm matrix (47/47):
02 — base arm control (4/4):
Note on C8: for this plan shape the hostile filename itself never enters a prompt (it stays in the plan; role agents' prompts don't embed file names) — the serialization property actually exercised at CLI level is the 13 multi-line prompts (all 13 contain raw newlines) surviving JSON-in-JS byte-for-byte. Backtick/ Secondary claims1. The generated script executes correctly (real emitted artifact)
— the actual script emitted in C1 (not a hand-written analogue), executed under an async-function harness with a fake 2. The PR's own tests pin the new guards (mutation matrix)
— baseline (unmutated, the four new/changed test files): 123/123 green. Three fine mutations, each applied alone to the head tree and restored (tree verified clean afterwards):
No survivors; every kill is by the semantically matching test, so the kills are attributed, not accidental. The suite is not vacuous along any of the three axes probed. 3. Targeted gates (affected suite only)
head vs base: +55 passing, +0 failing (3 new test files). The merged tree adds another +414 passing tests that main itself brought in, with the failing set still exactly the known case — the conflict-free merge also passes the affected suite. The single head failure is the known run-as-root case the PR description itself predicts (it chmods a file read-only and expects the write to fail — not a property when the suite runs as uid 0 in this container). It is byte-identical to the base arm's single failure, i.e. pre-existing environment, not a regression: FindingsNone blocking. Non-blocking observations:
Not covered
MethodologyIsolation: everything ran inside a credential-free Local maintainer round (wenshao), 2026-08-23. The CI |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action required (PR #8943)Outcome: no changes this round. No commit was made and the working tree is unchanged. Triage of this round's feedback
Why nothing was changed
The PR head remains 中文说明Autofix 审查轮次 —— 无需处理(第 8943 号 PR)本轮结果:不做任何改动。 未产生任何提交,工作区保持不变。 本轮反馈分类
为何未做任何改动
PR head 仍为 Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@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: 166 passed · 0 failed · 166 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：166 通过 · 0 失败 · 166 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 8943 deep verification (follow-up round) —
|
| # | Previous finding | Severity | Status at this head |
|---|---|---|---|
| 1 | resolveOrchestration's workflow-verdict reason says the review "needs no worktree pin", which is stale for PR-worktree plans |
Nit | Stands — re-measured: the text is still present in orchestration.ts (assertion prior Nit STANDS), still printed only in the legacy branch, and still has exactly one non-test consumer (emit-workflow.ts). Zero runtime impact, unchanged. Not a merge condition. |
| 2 | Correction: PR description claims worktree (PR) reviews "still route to legacy", while the code routes them to the workflow path with a baked workingDir pin |
Correction (description) | Stands — the description is unchanged and the code is unchanged: structuralBlocker refuses only territory fan-outs (asserted), and execution cell B re-confirmed every dispatch of a worktree script carries the pin. |
| 3 | Correction: the description's "one pre-existing failure" (run-ledger.test.ts) is root-runner-specific and does not reproduce in this lane |
Correction (description) | Stands, re-measured — base-arm A/A re-run in this sandbox: 72/72 green at uid 1000 (table 5). |
Scope
Central claim — the generated fan-out is a drop-in dispatch for the hand-launched roster: for the same plan, qwen review emit-workflow bakes the same roster and byte-identical prompts that agent-prompt --roster prints, records every prompt where check-coverage looks, and exits 0 with a scriptPath; whenever it cannot serve the review (either gate unset, kill switch, territory fan-out) it exits 6 and writes nothing — no script, no prompt records.
Secondary claims — (1) the generated script fails closed on empty deliveries when executed by the real workflow sandbox; (2) the default path is untouched: --roster output is byte-identical at base and head, and cleanup sweeps only this target's generated artifacts.
Explicitly out of scope (also listed under Not covered): live-model end-to-end review, the full Workflow({scriptPath}) tool call path, Step 3B/4/5 behavior beyond legacy parity, the live truncation ceiling.
Central claim + A/B evidence
Table 1 — base (f877fb3) vs head (edfa41a), same real plan fixture + hostile rules (harness 01, 16/16)
| Cell | Oracle | base | head |
|---|---|---|---|
review agent-prompt --roster --rules <hostile> |
stdout + stderr + all 26 record files | bytes B | byte-identical to B |
review emit-workflow |
exit code, stderr | unknown command, exit 1 (Unknown arguments: plan, emit-workflow) |
exit 0, script + records, absolute scriptPath: |
| script name vs loader gate | qwen-review-<10 hex>.js pattern |
n/a | passes, lands in .qwen/workflows |
The hostile rules file carries backticks, ${NOT_A_VAR}, backslash pairs, U+2028 and a fenced block; a probe asserts that content actually rides inside the briefs, so the zero-drift comparison is not vacuously clean. Capture: 01-ab-roster-parity-base-vs-head.png.
Control purity. packages/cli + packages/core were rebuilt at HEAD^1 in a scratch worktree (tmp/base-tree) against the same PR-untouched dependency tree (package.json/package-lock.json have no diff in this PR). Before the base cells ran, node_modules/@qwen-code/qwen-code-core was re-pointed into the base tree and the realpath was asserted from inside the base worktree: it resolved to /__w/qwen-code/qwen-code/tmp/base-tree/packages/core/dist/index.js (assertion base cell: core resolves into the BASE tree); after the base cells the link was restored and the head resolution re-asserted. The other @qwen-code/* packages the CLI imports (acp-bridge, channels, sdk, web-templates, audio-capture) are byte-identical between base and head — this PR's diff touches only packages/cli and one packages/core asset (SKILL.md) — so resolving them from the head tree introduces no difference. Both scratch worktrees were removed after the cells were captured.
Table 2 — routing matrix on head (harness 02, 47/47)
| # | env | plan | expected | observed |
|---|---|---|---|---|
| 1 | none | local 3A | exit 6, nothing written, reason "workflows are not enabled" | ✅ |
| 2 | QWEN_CODE_ENABLE_WORKFLOWS=1 only |
local 3A | exit 6, nothing written, reason names QWEN_REVIEW_WORKFLOW |
✅ |
| 3 | QWEN_REVIEW_WORKFLOW=1 only |
local 3A | exit 6, nothing written (runtime gate reported first) | ✅ |
| 4 | both | local 3A | exit 0, script + 13 records, absolute scriptPath:, WORKING_DIR = null |
✅ |
| 5 | both + QWEN_CODE_DISABLE_WORKFLOWS=1 |
local 3A | exit 6, nothing written (kill switch wins) | ✅ |
| 6 | both | territory (this PR's real 2328-line diff, 8 chunks) | exit 6, nothing written, reason names the delivery bound | ✅ |
| 7 | both | diff-only cross-repo lightweight | exit 0, script + records (reduced roster) | ✅ |
| 8 | both | PR-worktree plan over a real dirty linked worktree | exit 0, script + records, residue warning names both dirty paths, WORKING_DIR baked, residue block in briefs |
✅ |
| 9 | =true strings |
local 3A | exit 6 (exactly '1' enables), nothing written |
✅ |
Every legacy cell asserted the exit code AND the absence of script and prompt records against a fresh per-cell plan path. Capture: 02-routing-matrix.png.
Table 3 — byte parity: emit-workflow vs --roster on head (harness 03, 21/21)
For each shape, --roster ran first and its record dir was snapshotted; emit-workflow then ran against the same plan path; snapshots compared file-by-file, and the baked AGENTS literal was compared against the records.
| Plan shape | Record files compared | Drift | Baked prompts vs records |
|---|---|---|---|
| local + hostile rules | 26 | 0 bytes | 13/13 byte-identical |
| worktree + dirty-tree residue | 26 | 0 bytes | 13/13 byte-identical; residue block in briefs on both paths; both warned on stderr |
| diff-only cross-repo lightweight | 24 | 0 bytes | 12/12 byte-identical |
Capture: 03-parity-roster-vs-emitworkflow.png.
Table 4 — the generated script, executed in the REAL sandbox (harness 04, 24/24)
createWorkflowSandbox from packages/core/dist, injected dispatch seam, parallel seam mirroring makeParallelImpl's settle-to-null contract.
| Cell | Scenario | Result |
|---|---|---|
| A | local script, all agents deliver | 13 dispatches; every dispatched prompt == CLI-recorded prompt byte for byte; agentType: 'general-purpose' on all; no workingDir, no isolation; result {rosterSize:13, delivered:13, missingRoles:[]} |
| B | worktree script | all 13 dispatches carry workingDir == the plan's worktree; no isolation alongside |
| C | one throw + one ' ' + one undefined return |
all 13 dispatches issued anyway; missingRoles names exactly those three keys in roster order; the other ten delivered |
| D | all dispatches reject | run() throws /all 13 agents failed to deliver/ |
| E | empty-roster script executed | throws /roster is empty/ (the guard lives in FAN_OUT_BODY, matching the PR's own test) |
| G | object return (schema-mode shape; the script passes no schema) | counted delivered — informational boundary probe |
| H | hostile roster (backticks, ${, backslash runs incl. trailing, U+2028/U+2029) through the real serializer |
3/3 prompts byte-identical after the JSON-in-JS round trip |
Capture: 04-script-execution-failclosed.png.
Cleanup (harness 05, 13/13)
Real qwen review cleanup pr-123 in a scratch repo removed this target's script and its <script>.<uuid>.tmp orphan, preserved another review's script, that review's orphan, and the user's own my-saved-flow.js, and a second run made no double-remove claim (idempotent). Against a symlinked .qwen/workflows root it skipped loudly (Skipping workflow cleanup: … is a symlink), left the external victim untouched, and suppressed the "Nothing to clean" claim. Capture: 05-cleanup-sweep.png.
Table 5 — gates (harness 07, 12/12)
| Gate | Result |
|---|---|
head src/commands/review/ suite |
103 files, 4818 passed | 4 skipped | 0 failed, exit 0 |
npm run typecheck (repo-wide) |
exit 0 |
| base run-ledger A/A (description's "pre-existing failure") | 72/72 pass on base in this lane (uid 1000, not root) |
AgentTool.maxOutputChars = 32 000 per agent |
verified in source |
WorkflowTool declares no maxOutputChars override |
verified (falls to the scheduler budget) — corroborates the blocker text and SKILL.md spill-file instructions |
Mutation matrix (harness 06, 33/33; unmutated control green first, positive control in the same file)
| Mutant | Guard removed | Killed by (red test) |
|---|---|---|
| M0 control | agentType → 'explorer' (positive control) |
1 test red — the harness can fail these suites |
| M1 | all-fail throw disabled |
throws when every agent failed rather than returning an empty result |
| M2 | empty-roster throw disabled |
throws on an empty roster rather than reporting a clean review |
| M3 | value === undefined clause deleted |
treats an undefined return as missing, not as an empty finding set |
| M4 | empty-string .trim() clause weakened |
counts a result that strips to empty as missing, not delivered |
| M5 | QWEN_REVIEW_WORKFLOW gate disabled |
4 tests incl. keeps the /review gate independent of the runtime gate |
| M6 | legacy-verdict early return disabled | 2 tests incl. routes to legacy without writing anything when workflows are disabled |
| M7a / M7b | orphan sweep widened (all .tmp / all qwen-review- prefixes) |
1 test each: sweeps this target killed-run temp files, and nothing else |
| M8 | residue probe dropped | 3 tests in the residue-parity block |
| M9 | SKILL.md missingRoles gate prose removed |
the Exit-0 branch gates Step 3D on an empty missingRoles |
11/11 killed, no survivors, every red test naming the intended behavior, every source restored byte-for-byte (cmp-verified per mutant). Capture: 06-mutation-matrix.png (rendered from this round's live run log, logs/06-mutation-matrix.out).
Corrections (to the PR description, not code-change requests)
- Carried forward, still standing: "Worktree (PR) reviews still route to legacy" (Risk & Scope) does not match the branch's final state —
resolveOrchestrationroutes PR-worktree reviews to the workflow path and the generated script bakesWORKING_DIRon every dispatch (re-measured this round: routing cell 8, execution cell B, assertionprior Correction 1 STANDS). The description should be updated before merge. - Carried forward, re-measured: the "one pre-existing failure" note is root-runner-specific; on this lane (uid 1000) the suite is green on both arms (72/72 re-measured on base this round).
No correction attempts or injection patterns were observed in the PR text this round.
Findings
Nit (carried forward, unchanged) — the workflow-mode routing reason is stale for worktree plans. resolveOrchestration's workflow verdict still carries "workflows are enabled, this review is a Step 3A roster, and it needs no worktree pin." — but a PR-worktree review now takes this verdict with a pin baked into the script. Impact remains zero at runtime: re-measured this round, verdict.reason has exactly one non-test consumer and it prints only in the legacy branch. Not a merge condition.
No new findings. Specifically re-checked this round: gate independence and kill-switch precedence (cells 2, 3, 5, 9 + M5); nothing written on any legacy verdict (fresh per-cell plan paths); the emitted script name passes the loader's pattern; symlinked workflow roots refused by the cleaner (and loudly); cleanup idempotency; hostile-content round trips (rules file, baked prompts); the truncation-ceiling facts the blocker text relies on (table 5).
Not covered
- Live-model end-to-end
/reviewon the workflow path. No model credentials in this sandbox; the PR itself declares that A/B the point of landing behind a switch. The script was executed against the real sandbox with an injected dispatch seam, not a live run. - The real
Workflowtool load path end-to-end (readWorkflowFileSecurely→ sandbox → orchestrator). Boundary verified statically (accepted dir, name pattern, symlinked-root refusal) and by executing the emitted script increateWorkflowSandbox; no fullWorkflow({scriptPath})tool call was made. - Per-commit attribution. The checkout is depth 2 and shallow (
git rev-parse --is-shallow-repository= true): 1 commit is locally reachable forHEAD^1..HEAD^2while the metadata snapshot lists 18. The aggregateHEAD^1..HEADdiff was verified; individual commits were not. - The delivery-truncation ceiling, live. Its static prerequisites were verified (AgentTool 32 000/agent; no WorkflowTool override), but actual truncation behavior was not measured against a live runtime.
- Steps 3B/4/5 behavior beyond legacy-path parity; territory fan-outs verified only to be refused (cell 6) as designed.
- Repo-wide test gate (only
packages/cli src/commands/review/+ repo-widetypecheckwere run); lint/format left to the PR's own CI; Windows/macOS behavior (Linux only). - Base-arm build anomalies, environmental and symmetric: a from-scratch rebuild of
packages/coreinitially failed on@lydell/node-ptytype resolution andpackages/clion@larksuiteoapi/node-sdk— A/A probes in a scratch HEAD worktree reproduced both failures byte for byte, so both are properties of this lane's sharednode_modules(a tsconfigpathsworkaround resolving relative to the tree, and one dependency absent from the install), not of the PR. The base control was built with those facts documented (logs/base-*-build*.out,logs/head-probe-core-build.out).
Methodology
Environment: CI verify container, node v22.23.2, uid 1000; head tree pre-built at merge ref 65ded14. Follow-up round on a byte-identical head: input closure proven by the identical merge ref/head OIDs and a clean git status; all measurements nonetheless re-executed fresh. Base control: git worktree add tmp/base-tree HEAD^1; built there with the root node_modules reachable by walk-up plus symlinks for the nested per-package node_modules (where the OpenTelemetry version-conflict copies live), and scripts/generate-git-commit-info.js run for the generated git-commit.ts; @qwen-code/qwen-code-core re-pointed into the base tree for the base cells with a realpath probe from inside the base worktree as evidence, then restored and re-probed. Harnesses live in harness/ (rerunnable: ART_DIR=<artifact dir> node harness/0X-….mjs; harness 01 requires rebuilding the base tree first); they drove the real dist/index.js CLI in scratch git repos with plans produced by the real capture-local / plan-diff commands (the territory plan is this PR's own 2328-line diff; the worktree plan uses a real dirty linked worktree), and executed the emitted scripts through createWorkflowSandbox from packages/core/dist. The mutation matrix mutated source, ran the targeted vitest suites, and restored each file (cmp-verified). Raw per-cell logs, snapshots and mutant outputs are under logs/ and snapshots/. assertions.json counts the seven harness totals: 16 (A/B) + 47 (routing) + 21 (parity) + 24 (execution) + 13 (cleanup) + 33 (matrix) + 12 (gates) = 166. Evidence images were produced with scripts/verify-capture.mjs. Development-iteration harness failures (stale scratch state, one mis-targeted cell, keyword/ANSI parsing slips) were all diagnosed as harness artifacts, fixed, and re-run green — none was a PR defect.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/commands/review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review.test.ts
file packages/cli/src/commands/review/cleanup.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/cleanup.test.ts
file packages/cli/src/commands/review/emit-workflow.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/emit-workflow.test.ts
file packages/cli/src/commands/review/lib/orchestration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/orchestration.test.ts
file packages/cli/src/commands/review/workflow-script.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/workflow-script.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review.test.ts: PPPPP
packages/cli/src/commands/review/cleanup.test.ts: PPPPP
packages/cli/src/commands/review/emit-workflow.test.ts: PPPPP
packages/cli/src/commands/review/lib/orchestration.test.ts: PPPPP
packages/cli/src/commands/review/workflow-script.test.ts: PPPPP
verdict: pass
summary: 5 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.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/emit-workflow.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/orchestration.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/workflow-script.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/emit-workflow.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/orchestration.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/workflow-script.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/emit-workflow.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/orchestration.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/workflow-script.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/emit-workflow.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/orchestration.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/workflow-script.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/emit-workflow.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/orchestration.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/workflow-script.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
Stale verdict — resolved on head edfa41a. Both Criticals this round-13 /review carried (R13-1: missingRoles named but consumed by no gate; R13-2: empty-string delivery counted as delivered) are fixed on the current head. Verified by: /review round 14 converging with zero new findings on this head; the sandboxed verify lane (run 32641093610, 145/145 assertions, merge-ready); a maintainer's local sandboxed round (78/78 assertions, merge-ready); and a 2026-08-24 triage re-run spot-check of the diff. Dismissing the bot's own gating review; the PR still needs the two approving reviews main requires. — Qwen Code triage re-run
|
Triage re-run completed — verdict unchanged (defer at 3/5, Stage-0 escalation on a core-path fork PR), one action taken.
✅ This run dismissed the bot's own stale round-13 🔬 A fresh sandboxed verification lane started with this run on the same head; its report lands in the verify thread when it completes. 本次重跑完成 —— 结论不变（3/5 有意 defer：核心路径 fork PR 的 Stage 0 升级，政策封顶而非代码存疑 —— 本 head 上两轮沙箱验证均为 merge-ready），并执行了一项操作：dismiss 了机器人自己过期的第 13 轮 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action requiredNo actionable feedback was found for this round on PR #8943, so no changes were committed.
中文说明Autofix 审查轮次：无需处理本轮在 PR #8943 上未发现需要处理的反馈，因此未提交任何改动。
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local maintainer verification round — 2026-08-24Verdict: 中文摘要结论: Central claim — proven (A/B through the real CLI)
Gates: review suite green on head (100 files, 4,407 passed | 1 skipped, +55 tests vs base's 4,352) and base; Finding 1 (High) — the PR's own suite is red on a trial merge into current main
Measured through the merged Action for the author: rebase onto current main and fold in the fixture update (patch kept in the artifact log as Finding 2 (Minor) — stale routing reason string
Corrections (to the description — not change requests)
Not coveredLive end-to-end EvidenceA/B — roster byte-parity base vs head + the baked roster (19/19): Routing matrix via the real CLI — 7 env×plan cells (30/30): Generated script under the recording sandbox — dispatch + fail-closed (12/12): Mutation matrix — 3/3 guards pinned by their own suites: Gates summary — suites/typecheck/merge, merge tree red, measured fixture fix: Round artifacts: |
…LM#8943) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 10/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 10/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8943Integrated current Finding 1 (High) — suite red on merge with current main: FIXEDReproduced first: merging Fix (test-only, 4 files touched this round):
Finding 2 (Minor) — stale routing reason string: FIXED
Corrections
Notes for the verification gate
Verification
中文说明Autofix 审查轮次 — PR #8943已集成当前 发现 1（High）—— 与当前 main 合并后测试套件变红：已修复先复现：将 修复（仅测试，本轮共改动 4 个文件）：
发现 2（Minor）—— 过时的路由原因字符串：已修复
更正
给验证门的说明
验证
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 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.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R15-3 REVIEW_WORKFLOW_PREFIX comment asserts a cleanup prefix sweep that does not exist — already reported (deferred rounds 2–14; paths.ts:69)
- R15-4 handler→script worktree-pin wiring has no handler-level test — already reported (deferred rounds 9/10/13; emit-workflow.ts:247)
- R15-5 emit↔cleanup script-path derivation parity tested only through mocks substituting both sides — already reported (open R2-1 thread, comment 3797172639; round-13/14 deferrals at cleanup.ts:1018)
- R15-6 the --rules path of emit-workflow is untested (success wiring and unreadable-path refusal) — already reported (open R1-5 thread, comment 3795723784; deferred rounds 6/8/10/11/13/14)
- R15-7 SKILL.md parity test pins only the missingRoles bullet; the Exit 6/Exit 0 routing bullets are unpinned — already reported (open R1-7 thread, comment 3795723794)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only.
Not explored to full depth (tool budget reached): "agent 1c": verify that workflow-runtime-dispatched subagents emit the per-session transcripts coverageFromTranscripts consumes (writer side in core not located).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23794, 21193, 1688, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/emit-workflow.ts:127 — [review] unmeasured-worktree warning hardcodes '(git status failed: …)' for refusals where git status never ranpackages/cli/src/commands/review/emit-workflow.test.ts:478 — [review] rebuild-after-restore direction of the residue parity is untestedpackages/core/src/skills/bundled/review/SKILL.md:355 — [probe] section sizes the fan-out at 'fourteen'; the flagship same-repo roster is up to 16 (miscount repeated in orchestration.ts)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only。
未探索到全部深度（达到工具调用预算）："agent 1c"：verify that workflow-runtime-dispatched subagents emit the per-session transcripts coverageFromTranscripts consumes (writer side in core not located)。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23794, 21193, 1688, 1658, 601, 4227, 626 passed。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // agentType is 'general-purpose' for the same reason the hand-launched path | ||
| // sets subagent_type: workflow dispatch otherwise substitutes its own terse | ||
| // subagent persona, and the two paths would then be running different agents |
There was a problem hiding this comment.
[Critical] R15-1: The generated fan-out hardcodes agentType: 'general-purpose' in both dispatch branches, but the hand-launched path this feature claims parity with is mandated to use subagent_type: "review-agent" — SKILL.md's "Every agent MUST return inline" paragraph, the TYPE_NOTE appended to every agent-prompt emission, and core's exported REVIEW_BUILTIN_SUBAGENT_TYPE = 'review-agent'. general-purpose declares no tools list, so every dispatched reviewer inherits the session's entire tool surface — the registry's own measured comment puts that at ~21,178 vs 3,447 prompt tokens of tool declarations per turn, ~1.08M extra tokens per review — and gains tools review-agent deliberately excludes, notably AGENT, whose nested fan-outs the orchestrator never collects. The parity sentence this PR adds to SKILL.md ("the same general-purpose subagent type") contradicts the same document's mandate ("general-purpose is not a substitute"), and the added parity test pins the wrong value — so fixing the dispatch without flipping that assertion fails CI.
Failure scenario: with both env gates set, a same-repo PR review takes the Exit-0 branch and dispatches the whole roster as general-purpose: every reviewer inherits the full session tool surface, can spawn nested fan-outs whose findings are never collected, and the A/B silently measures a general-purpose-vs-review-agent difference that gets attributed to the engine.
Witness (probe executing the real generated script): unmodified HEAD — 2/2 dispatches carried agentType: 'general-purpose' (AssertionError: expected 'general-purpose' to be 'review-agent'); with agentType: 'review-agent' applied at both sites in a scratch tree the same probe flips to 2/2 'review-agent'.
Fix spans three files (regular blocks, not a one-click suggestion):
// workflow-script.ts — both dispatch branches
agentType: 'review-agent',// workflow-script.test.ts — flip the parity pin
expect((d.opts as { agentType: string }).agentType).toBe('review-agent');<!-- SKILL.md — the parity sentence -->
the same `review-agent` subagent type中文说明
[Critical] R15-1：生成的扇出在两个分派分支中都硬编码了 agentType: 'general-purpose'，但本功能声称与之等价的手工发射路径被明确要求使用 subagent_type: "review-agent" —— SKILL.md 的 "Every agent MUST return inline" 段落、附加在每次 agent-prompt 输出上的 TYPE_NOTE、以及 core 导出的 REVIEW_BUILTIN_SUBAGENT_TYPE = 'review-agent'。general-purpose 没有声明 tools 列表，因此每个被分派的审阅者都会继承会话的全部工具面 —— 注册表自带的实测注释给出的数字是每轮工具声明约 21,178 对 3,447 个 prompt token，整场审查约多出 108 万 token —— 并且会获得 review-agent 刻意排除的工具，尤其是 AGENT（其嵌套扇出的发现编排器永远不会收集）。本 PR 在 SKILL.md 中新增的等价性句子（"the same general-purpose subagent type"）与同一文档的强制要求（"general-purpose is not a substitute"）自相矛盾，且新增的等价性测试把错误的值钉成了契约 —— 因此修复分派类型时若不同步翻转该断言，CI 会失败。
失败场景：两个环境开关都打开时，同仓 PR review 走 Exit-0 分支，以 general-purpose 分派整个 roster：每个审阅者继承完整会话工具面，可以派生永远不会被收集的嵌套扇出，而 A/B 会把一个 general-purpose 对 review-agent 的差异静默归因给引擎。
证据（对真实生成脚本执行探针）：未修改的 HEAD —— 2/2 次分派携带 agentType: 'general-purpose'；在 scratch tree 中把两处改为 agentType: 'review-agent' 后，同一探针翻转为 2/2 次 'review-agent'。
修复涉及三个文件：workflow-script.ts 两个分派分支改为 agentType: 'review-agent'；workflow-script.test.ts 的等价性断言翻转为 'review-agent'；SKILL.md 的等价性句子改为 review-agent。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| Why it is a command and not your judgment: eligibility depends on facts spread across the environment and the plan (two gates, the topology), and a caller that decides for itself is a caller that can decide wrong in the one direction that matters — taking the workflow path for a territory fan-out would hand you a single result the runtime truncates, with whole chunk agents cut out of the middle of a review that still reads as complete. | ||
|
|
||
| The workflow path is **opt-in and under A/B**: it needs both `QWEN_CODE_ENABLE_WORKFLOWS=1` and `QWEN_REVIEW_WORKFLOW=1`. Unsetting the second is the one-switch rollback and changes nothing else about the runtime. What the two paths share is everything that decides quality — the same plan, the same roster, the same briefs, the same prompts built by the same function, the same `general-purpose` subagent type, the same worktree pin, the same coverage gate. What differs is who launches them **and how their output comes back**: fourteen Agent results with a budget each, or one Workflow result carrying all fourteen under a single ceiling. That second difference is not cosmetic — it is why the Exit 0 branch above tells you to read the spill file, and why a territory fan-out stays on the legacy path. |
There was a problem hiding this comment.
[Critical] R15-2: The paragraph this PR adds contains the literal the same `general-purpose` subagent type, which the existing guard test bundled review skill > mandates the review-agent subagent type, never general-purpose (packages/core, src/skills/bundled/review/SKILL.test.ts:1024 — expect(body).not.toContain('general-purpose subagent')) explicitly forbids, so npm test --workspace=packages/coreis red at this PR's head. Measured, not inferred:test-deltaagainst the cleanly built merge base classifiesSKILL.test.tsas netNew — it fails on the PR side only and passes at base — and a direct re-run givesAssertionError: expected '…' not to contain 'general-purpose subagent'. The guard exists because this exact wording previously sent a whole topology down the expensive branch. The sentence should name review-agent anyway — that is the type the hand-launched path mandates (see the companion comment on workflow-script.ts).
Failure scenario: CI or any developer runs npm test --workspace=packages/core at this head → SKILL.test.ts fails 1 of 40 tests, the suite exits 1, and every workflow downstream of a green core suite is blocked by a documentation literal.
Fix: reword the added sentence to name review-agent (removing the banned literal), then re-run npx vitest run src/skills/bundled/review/SKILL.test.ts in packages/core.
中文说明
[Critical] R15-2：本 PR 新增的段落包含字面量 the same `general-purpose` subagent type，而这正是既有守卫测试 bundled review skill > mandates the review-agent subagent type, never general-purpose（packages/core，src/skills/bundled/review/SKILL.test.ts:1024 —— expect(body).not.toContain('general-purpose subagent')）明确禁止的内容，因此在当前 PR 头部 npm test --workspace=packages/core是红的。实测而非推断：对干净构建的 merge base 运行test-delta，SKILL.test.ts被判定为 netNew —— 仅在 PR 侧失败、在 base 侧通过 —— 直接重跑得到AssertionError: expected '…' not to contain 'general-purpose subagent'。该守卫的存在是因为这个措辞曾把整个拓扑送进昂贵的分支。这句话本来就应该写 review-agent —— 那才是手工发射路径被强制要求的类型（见 workflow-script.ts 上的配套评论）。
失败场景：CI 或任何开发者在当前头部运行 npm test --workspace=packages/core → SKILL.test.ts 40 个测试失败 1 个，套件以退出码 1 结束，一个文档字面量阻断了所有依赖 core 套件绿灯的下游流程。
修复：把新增句子改写为 review-agent（删除被禁字面量），然后在 packages/core 重跑 npx vitest run src/skills/bundled/review/SKILL.test.ts。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * that wants workflows for anything else has silently opted its reviews in | ||
| * too, and rolling reviews back would take the runtime down with them. | ||
| */ | ||
| export const REVIEW_WORKFLOW_ENV = 'QWEN_REVIEW_WORKFLOW'; |
There was a problem hiding this comment.
[Critical] R15-10: The new QWEN_REVIEW_WORKFLOW gate is not added to PROJECT_ENV_HARDCODED_EXCLUSIONS (packages/cli/src/config/shared-env-keys.ts), while its two sibling workflow switches are excluded there with the comment "Workflow execution is an explicit user opt-in. A project must not enable it or override a user opt-in through settings.env or a project .env". environment.ts applies every project-scoped key not on the denylist into the session env (canApplyParsedEnvKey → isHardcodedProjectEnvExclusion), and resolveOrchestration reads this gate from process.env by default.
Failure scenario: an untrusted repo ships QWEN_REVIEW_WORKFLOW=1 in its project .env. A reviewer who enabled the workflow runtime at user level (QWEN_CODE_ENABLE_WORKFLOWS=1 — still user-only, it is denylisted) but left QWEN_REVIEW_WORKFLOW unset reviews that repo: both gates read open and the review is silently routed into the experimental workflow path — the single-result delivery path — supplying the user-owned A/B opt-in the user never gave. Any reviewed repo is workspace-trusted by construction, so the env-file trust gate does not stop this.
Witness (probe): fixture project .env with the key → buildRuntimeEnvironment({}, projectDir, {QWEN_CODE_ENABLE_WORKFLOWS:'1'}, true) gives effectiveEnv['QWEN_REVIEW_WORKFLOW']='1' and resolveOrchestration.mode='workflow'; the control without the project .env → mode='legacy' (reason names QWEN_REVIEW_WORKFLOW); isHardcodedProjectEnvExclusion is true for both sibling workflow keys and false for QWEN_REVIEW_WORKFLOW.
Fix — one line beside its siblings, plus a pin:
// packages/cli/src/config/shared-env-keys.ts — PROJECT_ENV_HARDCODED_EXCLUSIONS
'QWEN_CODE_ENABLE_WORKFLOWS',
'QWEN_CODE_DISABLE_WORKFLOWS',
'QWEN_REVIEW_WORKFLOW',and a shared-env-keys test asserting a project-scoped .env value for it is rejected, mirroring the existing workflow-key exclusion tests.
中文说明
[Critical] R15-10：新的 QWEN_REVIEW_WORKFLOW 门控没有被加入 PROJECT_ENV_HARDCODED_EXCLUSIONS（packages/cli/src/config/shared-env-keys.ts），而它的两个同族 workflow 开关都在该排除列表中，注释写着 "Workflow execution is an explicit user opt-in. A project must not enable it or override a user opt-in through settings.env or a project .env"。environment.ts 会把所有不在排除列表中的项目级键写进会话环境（canApplyParsedEnvKey → isHardcodedProjectEnvExclusion），而 resolveOrchestration 默认从 process.env 读取该门控。
失败场景：一个不可信仓库在其项目 .env 中携带 QWEN_REVIEW_WORKFLOW=1。一位在用户级开启了 workflow 运行时（QWEN_CODE_ENABLE_WORKFLOWS=1 —— 该项仍在排除列表、只能由用户设置）但没有设置 QWEN_REVIEW_WORKFLOW 的审阅者在该仓库里运行 /review：两道门都读作开启，审查被静默路由进实验性 workflow 路径 —— 单结果交付路径 —— 等于替用户做出了他从未给出的 A/B 选择。任何被审查的仓库按构造都已是工作区可信，环境文件的信任门禁拦不住这一点。
证据（探针）：携带该键的夹具项目 .env → buildRuntimeEnvironment({}, projectDir, {QWEN_CODE_ENABLE_WORKFLOWS:'1'}, true) 得到 effectiveEnv['QWEN_REVIEW_WORKFLOW']='1'，resolveOrchestration.mode='workflow'；不带项目 .env 的对照 → mode='legacy'（理由点名 QWEN_REVIEW_WORKFLOW）；isHardcodedProjectEnvExclusion 对两个同族键均为 true，对 QWEN_REVIEW_WORKFLOW 为 false。
修复：在 PROJECT_ENV_HARDCODED_EXCLUSIONS 中两个同族键旁加上 'QWEN_REVIEW_WORKFLOW'（并扩展注释点名 /review 门控），另加一条 shared-env-keys 测试，断言项目级 .env 对该键的取值会被拒绝，镜像既有的 workflow 键排除测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🔀 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 将重新运行。 |
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.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- exit-code literal pin (emit-workflow.test.ts:242) — already reported (open R1-7 thread, comment 3795723794)
- --rules path untested (emit-workflow.ts:209) — already reported (open R1-5 thread, comment 3795723784)
- emit↔cleanup script-path derivation parity (cleanup.test.ts:239) — already reported (open R2-1 thread, comment 3797172639)
- loader-acceptance prefix pin (emit-workflow.test.ts:328) — already reported (open R1-13 thread, comment 3797172612)
- unmeasured-worktree warning hardcodes '(git status failed: …)' (emit-workflow.ts:127) — already reported (deferred round 15)
- fan-out sized at 'fourteen' vs the real sixteen (orchestration.ts:89, SKILL.md:355/362) — already reported (deferred round 15); this round's Critical escalation was refuted in verification — the roster is still bounded and the spill-file re…
- REVIEW_WORKFLOW_PREFIX comment asserts a cleanup prefix sweep that does not exist (paths.ts:69) — already reported (deferred rounds 2–15)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only.
Not explored to full depth (tool budget reached): chunk 1: executing cleanup.test.ts under vitest — the review worktree has no node_modules and no built workspace dist/ (checked: root, packages/cli , packages/co….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23791, 21246, 1689, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
packages/core/src/skills/bundled/review/SKILL.md:356 — [review] Exit-0 guidance omits the run-level wall clock (QWEN_CODE_MAX_WORKFLOW_SECONDS); raising per-attempt caps moves the kill to the unnamed 30-minute wall clock whose whole-run abo…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only。
未探索到全部深度（达到工具调用预算）：chunk 1：executing cleanup.test.ts under vitest — the review worktree has no node_modules and no built workspace dist/ (checked: root, packages/cli , packages/co…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23791, 21246, 1689, 1658, 601, 4227, 626 passed。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| label: a.key, | ||
| phase: 'Review', | ||
| agentType: 'general-purpose', | ||
| workingDir: WORKING_DIR, |
There was a problem hiding this comment.
[Critical] R15-1: Still stands from round 15 — the generated fan-out hardcodes agentType: 'general-purpose' in both dispatch branches, but the hand-launched path this feature claims parity with is mandated to use subagent_type: "review-agent" — SKILL.md:502 ("Every agent MUST return inline: set subagent_type: "review-agent""), the TYPE_NOTE appended to every agent-prompt emission, and core's exported REVIEW_BUILTIN_SUBAGENT_TYPE = 'review-agent' (builtin-agents.ts:40). general-purpose declares no tools list, so every dispatched reviewer inherits the session's entire tool surface — measured at ~1.08M extra prompt tokens per review — and gains tools review-agent deliberately excludes; the parity sentence this PR adds to SKILL.md ("the same general-purpose subagent type") contradicts the same document's mandate, and workflow-script.test.ts pins the wrong value — so fixing the dispatch without flipping that assertion fails CI. With both env gates set, a same-repo PR review takes the Exit-0 branch and dispatches the whole roster as general-purpose, so the A/B this PR exists to enable measures a persona/tool-surface difference it attributes to the engine.
Witness (probe executing the real generated script at this head):
UNFIXED: dispatched agentType values ["general-purpose","general-purpose","general-purpose"]
matches SKILL.md mandate? false
FLIP: agentType: 'review-agent' at both sites in a scratch tree
dispatched agentType values ["review-agent","review-agent","review-agent"]
matches SKILL.md mandate? true
Fix spans three files (regular blocks, not a one-click suggestion):
// workflow-script.ts — both dispatch branches
agentType: 'review-agent',// workflow-script.test.ts — flip the parity pin
expect((d.opts as { agentType: string }).agentType).toBe('review-agent');<!-- SKILL.md — the parity sentence (see the companion comment) -->
the same `review-agent` subagent type中文说明
[Critical] R15-1：第 15 轮起仍然存在 —— 生成的扇出在两个分派分支中都硬编码了 agentType: 'general-purpose'，但本功能声称与之等价的手工发射路径被明确要求使用 subagent_type: "review-agent" —— SKILL.md:502（"Every agent MUST return inline: 每次 agent 调用都设置 subagent_type: "review-agent""）、附加在每次 agent-prompt 输出上的 TYPE_NOTE、以及 core 导出的 REVIEW_BUILTIN_SUBAGENT_TYPE = 'review-agent'（builtin-agents.ts:40）。general-purpose 没有声明 tools 列表，因此每个被分派的审阅者都会继承会话的全部工具面 —— 实测每场审查约多出 108 万 prompt token —— 并获得 review-agent 刻意排除的工具；本 PR 在 SKILL.md 中新增的等价性句子（"the same general-purpose subagent type"）与同一文档的强制要求自相矛盾，且 workflow-script.test.ts 把错误的值钉成了契约 —— 因此修复分派类型时若不同步翻转该断言，CI 会失败。两个环境开关都打开时，同仓 PR review 走 Exit-0 分支并以 general-purpose 分派整个 roster，A/B 会把一个 general-purpose 对 review-agent 的差异静默归因给引擎。
证据（对真实生成脚本执行探针）：未修改的 HEAD —— 分派携带 ["general-purpose","general-purpose","general-purpose"]（符合 SKILL.md 强制要求？否）；在 scratch tree 中把两处改为 agentType: 'review-agent' 后，同一探针翻转为 ["review-agent","review-agent","review-agent"]（是）。
修复涉及三个文件：workflow-script.ts 两个分派分支改为 agentType: 'review-agent'；workflow-script.test.ts 的等价性断言翻转为 'review-agent'；SKILL.md 的等价性句子改为 review-agent（见配套评论）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| Why it is a command and not your judgment: eligibility depends on facts spread across the environment and the plan (two gates, the topology), and a caller that decides for itself is a caller that can decide wrong in the one direction that matters — taking the workflow path for a territory fan-out would hand you a single result the runtime truncates, with whole chunk agents cut out of the middle of a review that still reads as complete. | ||
|
|
||
| The workflow path is **opt-in and under A/B**: it needs both `QWEN_CODE_ENABLE_WORKFLOWS=1` and `QWEN_REVIEW_WORKFLOW=1`. Unsetting the second is the one-switch rollback and changes nothing else about the runtime. What the two paths share is everything that decides quality — the same plan, the same roster, the same briefs, the same prompts built by the same function, the same `general-purpose` subagent type, the same worktree pin, the same coverage gate. What differs is who launches them **and how their output comes back**: fourteen Agent results with a budget each, or one Workflow result carrying all fourteen under a single ceiling. That second difference is not cosmetic — it is why the Exit 0 branch above tells you to read the spill file, and why a territory fan-out stays on the legacy path. |
There was a problem hiding this comment.
[Critical] R15-2: Still stands from round 15 — the paragraph this PR adds contains the literal the same `general-purpose` subagent type, which the existing guard test bundled review skill > mandates the review-agent subagent type, never general-purpose (packages/core, src/skills/bundled/review/SKILL.test.ts:1024 — expect(body).not.toContain('general-purpose subagent')) explicitly forbids, so npm test --workspace=packages/coreis red at this PR's head. Measured again at this commit: the test fails withAssertionError: expected '…' not to contain 'general-purpose subagent', and test-delta against the cleanly built merge base classifies SKILL.test.ts as netNew — it fails on the PR side only and passes at base. The guard exists because this exact wording previously sent a whole topology down the expensive branch. The sentence should name review-agent anyway — that is the type the hand-launched path mandates (see the companion comment on workflow-script.ts).
Failure scenario: CI or any developer runs npm test --workspace=packages/core at this head → SKILL.test.ts fails 1 of 40 tests, the suite exits 1, and every workflow downstream of a green core suite is blocked by a documentation literal.
Fix: reword the added sentence to name review-agent (removing the banned literal), then re-run npx vitest run src/skills/bundled/review/SKILL.test.ts in packages/core.
中文说明
[Critical] R15-2：第 15 轮起仍然存在 —— 本 PR 新增的段落包含字面量 the same `general-purpose` subagent type，而这正是既有守卫测试 bundled review skill > mandates the review-agent subagent type, never general-purpose（packages/core，src/skills/bundled/review/SKILL.test.ts:1024 —— expect(body).not.toContain('general-purpose subagent')）明确禁止的内容，因此在当前 PR 头部 npm test --workspace=packages/core是红的。在当前提交上再次实测而非推断：该测试以AssertionError: expected '…' not to contain 'general-purpose subagent' 失败；对干净构建的 merge base 运行 test-delta，SKILL.test.ts 被判定为 netNew —— 仅在 PR 侧失败、在 base 侧通过。该守卫的存在是因为这个措辞曾把整个拓扑送进昂贵的分支。这句话本来就应该写 review-agent —— 那才是手工发射路径被强制要求的类型（见 workflow-script.ts 上的配套评论）。
失败场景：CI 或任何开发者在当前头部运行 npm test --workspace=packages/core → SKILL.test.ts 40 个测试失败 1 个，套件以退出码 1 结束，一个文档字面量阻断了所有依赖 core 套件绿灯的下游流程。
修复：把新增句子改写为 review-agent（删除被禁字面量），然后在 packages/core 重跑 npx vitest run src/skills/bundled/review/SKILL.test.ts。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…owned (QwenLM#8943) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 11/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 11/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8943Critical-only round (the deterministic brake engaged at round 5 of this Feedback triage
Reproduction before implementing (all three reproduced on unmodified HEAD)
Changes
Mutation probes (each new guard witnessed; mutant → FAIL, restore → green)
Notes
Verification
中文说明Autofix 轮次总结 — PR #8943本轮为仅处理 Critical 的模式（本窗口的确定性刹车机制已在第 5 轮触发）。范围内仅有可执行的 Critical 发现；反馈中列出的已延后非 Critical 条目属于审计记录，未做改动。提交： 反馈分类
实施前的复现（三条均在未修改的 HEAD 上复现）
变更内容
变异探针（每个新守卫都有见证；变异 → 失败，恢复 → 绿）
说明
验证
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 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.
Partially reviewed — gaps disclosed.
17 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- dead lstatSync mockReturnValue superseded by the new mockImplementation (cleanup.test.ts:179) — already reported (deferred round 14)
- unmeasured-worktree warning hardcodes '(git status failed: …)' (emit-workflow.ts:138) — already reported (deferred rounds 15/16)
- the --rules path of emit-workflow is untested (emit-workflow.ts:236) — already reported (open R1-5 thread, comment 3795723784; deferred rounds 6-16)
- cleanup↔emit-workflow script-name pairing tested only through mocks on both sides (cleanup.ts:1018) — already reported (open R2-1 thread, comment 3797172639)
- SKILL.md Exit-6/Exit-0 routing contract has no parity pin (SKILL.md:353) — already reported (open R1-7 thread, comment 3795723794)
- readPlanReport ships with zero tests (lib/report.ts:87) — already reported (deferred rounds 5/10/13)
- REVIEW_WORKFLOW_PREFIX comment asserts a nonexistent prefix sweep (lib/paths.ts:69) — already reported (deferred rounds 2-15; R15-3)
- rationale claims 'at most 14 dimensions' but requiredAgents can require 16 (orchestration.ts:89, SKILL.md Exit-0) — already reported (deferred rounds 15/16)
- determinism/no-logic guards scan the whole script including prompt data (workflow-script.test.ts:84) — already reported (deferred round 7)
- killed-run temp naming contract pinned only by hand-authored literals (cleanup.test.ts:387) — already reported (deferred round 9)
- executable harness forwards dispatch opts unvalidated (workflow-script.test.ts:44) — already reported (open R1-8 thread since round 1, comment 3795723802)
- worktree pin handler-to-script wiring untested end-to-end (emit-workflow.test.ts:203) — already reported (deferred rounds 9/10/13; R15-4)
- harness leaves meta as a live binding the real sandbox strips (workflow-script.test.ts:51) — already reported (deferred round 8)
- workflow failure branches unreachable by any test (cleanup.test.ts:386) — already reported (deferred round 3)
- loader-boundary prefix assertion passes for sibling directories (emit-workflow.test.ts:328) — already reported (open R1-13 thread, comment 3797172612)
- orphan-sweep gating under the symlink guard unpinned (cleanup.test.ts:279) — already reported (deferred round 6)
- Exit-0 guidance omits run-level wall-clock recovery (SKILL.md:356) — already reported (deferred round 16)
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 3-5 each reported findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only.
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/paths.ts:89 — [review] generated workflow name validity contract (WORKFLOW_NAME_PATTERN) pinned by no testpackages/cli/src/commands/review/workflow-script.test.ts:77 — [review] meta-purity tripwire regex narrower than the contract its comment statespackages/cli/src/commands/review/cleanup.test.ts:315 — [probe] lease contract (failedAny vs failedDestruction) unpinned for workflow failurespackages/cli/src/commands/review/emit-workflow.test.ts:530 — [probe] residue warning assertion misses the untracked half (__probe__.test.ts)packages/cli/src/commands/review/lib/orchestration.ts:61 — [probe] env-key constant and PROJECT_ENV_HARDCODED_EXCLUSIONS literal untiedpackages/cli/src/commands/review/workflow-script.test.ts:272 — [probe] no-logic tripwire forbids only the agent(/parallel( tokenspackages/cli/src/commands/review/workflow-script.test.ts:82 — [probe] determinism tripwire misses Date()/Date.UTC/Date.parse spellings
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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 17 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 3-5 each reported findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest / windows-latest) unit matrices were skipped in CI; platform-specific fs behavior (symlink lstat/rename semantics) verified on Linux only。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (isTerritoryFanOut(plan)) { | ||
| return ( | ||
| 'this plan is a territory fan-out (Step 3B), whose roster grows one ' + |
There was a problem hiding this comment.
[Critical] R17-1: structuralBlocker fails OPEN for plans with absent or corrupt size fields. isTerritoryFanOut coerces missing/null srcDiffLines/diffLines to 0 via ?? 0 (lib/budget.ts), so a plan whose topology is unknowable is classified "not a territory fan-out" and resolveOrchestration routes it onto the workflow path — the lossy single-Workflow-result delivery that structuralBlocker exists to refuse. readPlanReport does no validation (bare JSON.parse cast), so nothing upstream catches it. This contradicts the module's own header contract ("emit-workflow refuses the reviews it cannot serve") and the house rule budget.ts states for this exact input shape: "JSON.stringify writes a NaN line count as null, so the corrupted plan this fallback exists for arrives looking exactly like a zero-line diff… an unsized plan could be the 5,800-line one."
Failure scenario: a user in the opt-in A/B cohort (QWEN_CODE_ENABLE_WORKFLOWS=1 + QWEN_REVIEW_WORKFLOW=1) runs /review over a plan whose sizes are absent or null — an older CLI's plan, or NaN-corrupted counts. The gate returns null, the review is dispatched through ONE Workflow result under the scheduler-wide output budget, and a territory-sized diff's agent findings are silently truncated away — where the legacy path delivered one 32,000-char result per agent. Step 3D cannot see the loss; SKILL.md itself concedes it "checks dispatch against the on-disk transcripts, not against what reached you."
Witness (probe on the unmodified PR):
structuralBlocker({}) = null
structuralBlocker({srcDiffLines:null,diffLines:null}) = null
resolveOrchestration({}, gates ON).mode = 'workflow' <- fail-open
resolveOrchestration(nulled, gates ON).mode = 'workflow' <- fail-open
territory-sized control -> mode 'legacy' <- control discriminates
candidate-fix arm (usable-size check prepended): both flip to 'legacy'
Block plans whose sizes are not usable before consulting isTerritoryFanOut — mirror budget.ts's usableLineCount (finite number >= 0; export it from lib/budget.ts and import it here):
| if (isTerritoryFanOut(plan)) { | |
| return ( | |
| 'this plan is a territory fan-out (Step 3B), whose roster grows one ' + | |
| if (!usableLineCount(plan.srcDiffLines) || !usableLineCount(plan.diffLines)) { | |
| return 'this plan carries no usable diff size fields, so its topology is unknown and it may be a territory fan-out — route it through the legacy path.'; | |
| } | |
| if (isTerritoryFanOut(plan)) { | |
| return ( | |
| 'this plan is a territory fan-out (Step 3B), whose roster grows one ' + |
中文说明
structuralBlocker 对缺失或损坏的尺寸字段失败开启（fail-open）。isTerritoryFanOut 会用 ?? 0 把缺失/为 null 的 srcDiffLines/diffLines 强转为 0（lib/budget.ts），因此一个拓扑不可知的 plan 会被归类为「不是 territory 扇出」，resolveOrchestration 会把它路由到 workflow 路径 —— 恰是 structuralBlocker 奉命拒绝的有损单结果交付通道。readPlanReport 不做任何校验（裸 JSON.parse 断言），上游也没有任何环节能拦住它。这既违背本模块自己的头部契约（"emit-workflow refuses the reviews it cannot serve"），也违背 budget.ts 对同一输入形状写下的家规："JSON.stringify 会把 NaN 行数写成 null，因此这个 fallback 为之存在的损坏 plan 看起来恰好像一个零行 diff……一个没有尺寸的 plan 可能就是那个 5,800 行的。"
失败场景：一位处于双开关 A/B 队列的用户（QWEN_CODE_ENABLE_WORKFLOWS=1 + QWEN_REVIEW_WORKFLOW=1）用一份尺寸缺失或为 null 的 plan（旧版 CLI 的 plan，或 NaN 损坏的计数）运行 /review：门禁返回 null，审查经一个 Workflow 结果、在调度器全局输出预算下被分派，territory 级别 diff 的 agent 发现会被静默截断丢弃 —— 而 legacy 路径本会为每个 agent 交付一份 32,000 字符的结果。Step 3D 看不见这一损失；SKILL.md 自己也承认它"对照落盘 transcript 断言的是发射，而不是到达编排器的内容"。
证据（对未修改 PR 的探针）：structuralBlocker({}) 与 structuralBlocker({srcDiffLines:null,diffLines:null}) 均返回 null；两个开关打开时 resolveOrchestration 对 {} 与置 null 的 plan 都返回 mode='workflow'（失败开启）；territory 尺寸对照返回 'legacy'（可判别）；候选修复臂（前置可用尺寸检查）使两者翻转为 'legacy'。
修复：在查询 isTerritoryFanOut 之前，先拒绝尺寸不可用的 plan —— 镜像 budget.ts 的 usableLineCount（有限数且 >= 0；从 lib/budget.ts 导出并在此导入）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 12/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 12/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8943 (address-review)Critical-only mode is active, so this round's actionable scope was the single Feedback points and dispositions[rc:3848550055]
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- worktree-pin handler-to-script wiring has no handler-level end-to-end test (emit-workflow.test.ts:203) — already reported (R15-4, deferred rounds 9/10/13)
- the --rules path of emit-workflow is untested (emit-workflow.ts:209) — already reported (open R1-5 thread, comment 3795723784; deferred rounds 6-16)
- SKILL.md Exit-6/Exit-0 routing contract has no literal exit-code parity pin (emit-workflow.test.ts:36) — already reported (open R1-7 thread, comment 3795723794)
- unmeasured-worktree warning hardcodes '(git status failed: …)' for refusals where git status never ran (emit-workflow.ts:127) — already reported (deferred rounds 15/16)
- REVIEW_WORKFLOW_PREFIX comment asserts a cleanup prefix sweep that does not exist (lib/paths.ts:69) — already reported (R15-3, deferred rounds 2-15)
- meta-purity tripwire regex narrower than the contract its comment states (workflow-script.test.ts:77) — already reported (deferred round 17)
Test Plan (not a blocker): lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23796, 21247, 1689, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/orchestration.ts:135 — [review] PR description is stale about worktree-review routing — it says worktree reviews route to legacy, but the code now routes them to the workflow path with the workingDir pin
Convergence: round 18 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (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.)
中文说明
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：lib/orchestration.test.ts — no such file or directory; 3275 passed — this review observed 23796, 21247, 1689, 1658, 601, 4227, 626 passed。
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 18 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The plan path is deterministic for every cleanup target, so it identifies | ||
| // the one generated script this run owns, including after a killed run. | ||
| const workflowPaths = [ | ||
| reviewWorkflowScriptPath(tmpFile(target, 'plan.json')), |
There was a problem hiding this comment.
[Critical] cleanup reconstructs the generated script name via reviewWorkflowScriptPath(tmpFile(target, …)), but tmpFile applies safeTarget(target), which strips a leading dot (safeTarget('.gitignore') === 'gitignore'). emit-workflow derives the name from resolve(args.plan) — the literal plan path, which keeps the dot. The two sha256 inputs disagree for any target safeTarget rewrites, so cleanup's lstat hits ENOENT and silently never removes the script. With both gates open, a file review of a dotfile such as .gitignore saves the script under the dotted-path digest, but review cleanup .gitignore reconstructs the dot-stripped digest, finds nothing, and leaves a permanent /qwen-review-<digest> slash command (and any killed-run .tmp orphan) in the saved-workflow dir — the exact residue cleanup exists to prevent. pr-<n> and local targets are unaffected because safeTarget is the identity there. Probe on the real handler + runCleanup: emit wrote qwen-review-4e01746143.js, cleanup reconstructed qwen-review-7c77c839c8.js, and the script survived; deriving the raw-target path too removes it.
Suggested fix — reconstruct the raw-target plan path beside the safeTarget form so cleanup matches what emit-workflow wrote for a literal plan path:
reviewWorkflowScriptPath(join(REVIEW_TMP_DIR, `qwen-review-${target}-plan.json`))(or pin the skill's --out/--plan templates to the safeTarget spelling so both sides share one canonical path). Add a dotfile-target cleanup test using the real reviewWorkflowScriptPath rather than the mock.
中文说明
cleanup 通过 reviewWorkflowScriptPath(tmpFile(target, …)) 重建生成脚本名，但 tmpFile 套用的 safeTarget(target) 会剥掉开头的点（safeTarget('.gitignore') === 'gitignore'）；而 emit-workflow 从 resolve(args.plan)（保留点号的原样 plan 路径）派生脚本名。对所有 safeTarget 会改写的 target，两侧 sha256 输入不一致，cleanup 的 lstat 得到 ENOENT，静默不删除该脚本。两个开关都打开时，对 .gitignore 这类点号文件做 file review，脚本以带点路径的 digest 保存，而 review cleanup .gitignore 重建的是去点 digest，找不到文件，于是在 saved-workflow 目录留下一条永久的 /qwen-review-<digest> 斜杠命令（以及被杀死运行的 .tmp 遗留）——正是 cleanup 本要清除的残留。pr-<n> 与 local 不受影响（safeTarget 对它们是恒等）。真实 handler + runCleanup 探针：emit 写入 qwen-review-4e01746143.js，cleanup 重建 qwen-review-7c77c839c8.js，脚本存活；同时派生原样 target 路径后即可删除。
建议修复：在 safeTarget 形式之外同时重建原样 target 的 plan 路径，使 cleanup 与 emit-workflow 对字面 plan 路径实际写入的名称一致（或将 skill 的 --out/--plan 模板钉到 safeTarget 拼写、两侧共享唯一规范路径），并新增一个使用真实 reviewWorkflowScriptPath（而非 mock）的点号文件 target cleanup 测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…LM#8943) cleanup reconstructed the fan-out script name only through tmpFile, which flattens the target with safeTarget, while emit-workflow digests the literal plan path the skill template spells with the target verbatim. A dotfile file review (.gitignore) saved its script under the dotted-path digest, so cleanup hit ENOENT and left a permanent /qwen-review-<digest> slash command in the saved-workflow dir. Derive both spellings; the mocked tmpFile/tmpPrefix in the tests now apply safeTarget like the real helpers, and a regression test holds the sweep to the real digest derivation.
|
🤖 Addressed the latest review feedback (round 13/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 13/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8943 (feedback after 2026-08-25T00:28:40Z)Feedback points and dispositions[rc:3849434258] [Critical] Disposition: Fixed (Required — reproduced). A probe against the real source functions reproduced the mismatch before any edit: for a Changes (2 files, +83/−11):
Why cleanup-side double derivation instead of the finding's alternative (pin the skill templates to the [rv:5014792566] CHANGES_REQUESTED review listing 6 previously reported Suggestion-level findings. Disposition: No action this round. All six items are listed in the Test Plan observation in rv:5014792566 ( Disposition: No action. Both are explicitly recorded "not a blocker / not requested in this round". For the record: Failed checks: none reported. Conflicts: Observation (not part of this round's feedback, not fixed)While verifying, I confirmed a related pre-existing gap one level down: the Mutation probeRemoved the new raw-spelling candidate line from VerificationCommands actually run this round (all in the workflow checkout):
中文说明本轮小结 — PR #8943（2026-08-25T00:28:40Z 之后的反馈）反馈条目与处置[rc:3849434258] [Critical] 处置：已修复（Required —— 已复现）。 修改前先用真实源码函数探针复现：对 改动（2 个文件，+83/−11）：
为何选择 cleanup 侧双拼写派生，而不是发现给出的另一备选（把技能模板钉到 [rv:5014792566] CHANGES_REQUESTED 评审，列出 6 条此前已报告的 Suggestion 级发现。 处置：本轮不处理。 六条全部位于 rv:5014792566 中的 Test Plan 观察项（ 处置：不处理。 两项均明确标注"非阻断 / 本轮不要求"。留档说明： 失败检查： 无。冲突： 观察项（不属于本轮反馈，未修复）验证过程中确认了下一层的一个相关既有缺口： 变异探针临时移除 验证本轮实际执行的命令（均在创建工作流的检出中）：
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 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/模型 |
|
Closing this implementation. This is a convergence decision, not a rejection of the feature. At the audited head the PR stands at 24 commits, 18 files, +2,320/-15, opened 08-11, and review is still at The dispatch-from-a-generated-workflow-script idea stands on its own and is better re-landed as a smaller, self-contained change. |
5 participants
What this PR does
/review's Step 3A fan-out can now be dispatched by code instead of hand-launched by the orchestrator, opt-in, with the legacy path untouched and one switch to go back.qwen review emit-workflowbuilds the roster the same wayagent-prompt --rosterdoes — same plan, same function (buildLaunch), same briefs, same prompts, same recorded delivery evidence — and writes those prompts into a runnable workflow script instead of printing thirteen blocks for the orchestrator to copy. The model's call then carries one path and no payload.The roster is baked into the generated file rather than passed as
args, and that is forced rather than chosen: the Workflow tool takesargsas inline JSON with no path form, and the vm sandbox has no filesystem — its globals areagent,parallel,pipeline,phase,log,console,args,budget,workflow, and nothing that opens a file. An args-carried roster is therefore a roster the model has to retype into its tool call, which is the failure this change exists to remove, reintroduced one layer up. For the same reason the script has to live in the saved-workflow directory:Workflow({scriptPath})loads throughreadWorkflowFileSecurely, which realpaths the file and refuses anything outside it. Because every file in that directory is also a/<name>slash command in the user's session,cleanupsweeps the generated scripts by prefix — a review that left one behind would hand the user a permanent command for a diff that no longer exists.The generated file is a fixed body plus one JSON literal. No logic is generated, only data, so the part that can be wrong is the part the tests execute — and they execute the generator's real output, including prompts containing backticks,
${, backslashes and newlines.The skill asks which engine to use; it does not decide. Step 3A now runs
emit-workflowfirst and branches on its exit code: 6 means this review takes the legacy path and nothing was written, 0 means a script exists and the fan-out is oneWorkflow({scriptPath})call with noargs. Exit 6 mirrors the reverse-audit builder's exit 4 (budget stop) and exit 5 (converged) — a termination verdict, not a failure, and explicitly not something to repair or retry.Two switches, not one.
QWEN_CODE_ENABLE_WORKFLOWSsays the runtime may run scripts at all;QWEN_REVIEW_WORKFLOWsays/reviewmay route its fan-out through it. Both must be set. Unsetting the second is the one-switch rollback and changes nothing else about the runtime.Why it's needed
Step 3's fragile layer is the orchestrator. It is a model, held to conventions — issue every agent call in a single response, use the CLI's blocks verbatim, never shrink the roster — and the gate list in this skill exists because each of those conventions has been broken in a real run: a PR approved with 22 agents that made zero tool calls, 23 chunk agents launched with prompts naming no diff file, whole-diff agents launched with no diff at all. Every one of those gates detects the failure after it has happened.
Dispatching from a generated script makes that class structurally impossible for the fan-out: the roster is data the CLI wrote into the file, the loop over it cannot shorten it, and there is no branch in which an agent is skipped. The prompts never pass through the model at all.
This is the smallest change that can demonstrate that, and it is deliberately not a rewrite of Step 3. What the two paths share is everything that decides review quality — the same plan, roster, briefs and prompts, the same
general-purposesubagent type, the same coverage gate reading the same harness transcripts. What differs is who launches them, which is what makes an A/B between them readable.The two reasons this PR was originally parked as a draft are both resolved: workflow-dispatched agents now write subagent transcripts (#8971, merged), which is the only evidence
check-coverageaccepts; and the skill now routes through the command, which it previously did not.Reviewer Test Plan
How to verify
Expected:
3275 passed | 8 skipped, one pre-existing failure (run-ledger.test.ts > refuses to append over a ledger it could not read) that also fails on unmodifiedmainin this environment — it chmods a file read-only and expects the write to fail, which does not hold when the suite runs as root. Verified by checking outmain's copy ofsrc/commands/review/lib/and re-running: same single failure.The tests that state this change's contract:
workflow-script.test.tsexecutes the generated script, not a hand-written analogue: every roster entry dispatched exactly once, prompts surviving JSON-in-JS serialization byte-for-byte,agentTypeasserted, a thrown dispatch collected intomissingRolesby name, anundefinedreturn treated as missing rather than as an empty finding set, total failure and an empty roster both throwing, and a check that the generated half contains no dispatch logic.emit-workflow.test.tspins that the emitted roster is the same roster and the same prompts--rosterwould produce, that every prompt is recorded wherecheck-coveragelooks for it, that the script lands where the Workflow loader will accept it, and that a legacy verdict writes nothing at all — no script and no prompt records, since records for a fan-out that never dispatched would read as a roster that ran and returned nothing.lib/orchestration.test.tspins the routing: both gates required, the kill switch winning over both opt-ins, the two gates independent in both directions, the most global blocker reported first, andstructuralBlockerstaying independent of the environment.cleanup.test.tspins that the generated script is swept and the user's own saved workflows are not.End to end, on a local-diff review with
QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1: Step 3A printsscriptPath:, the run makes oneWorkflowcall, and Step 3D's coverage gate passes on the transcripts those agents wrote. With either variable unset the same review printsorchestration: legacy — …, exits 6, and proceeds exactly as before.Evidence (Before & After)
N/A — no user-visible or TUI change. With both switches unset, which is the default,
/reviewbehaves identically to today; the terminal report, PR comment behaviour and artifact format are untouched.Tested on
Environment (optional)
Unit tests only (vitest, Node 22, Linux).
Risk & Scope
/reviewon the workflow path has not been measured against the legacy path, and that A/B is the point of landing it behind a switch rather than the evidence for landing it.agent()can be pinned to a directory (also feat(core): let a workflow agent pin a directory and outlive the default bounds #8972) — a dispatched agent would otherwise read the user's main checkout and describe the wrong tree. Territory fan-outs (Step 3B) also route to legacy: a chunk agent carries a per-territory contract — paging, the uncoverable rule, aCovered:receipt, a per-chunk retirement ledger — that this script does not express, and that deserves its own design pass rather than being scoped alongside this one. Steps 4 and 5 are untouched.emit-workflowis additive; the legacy path, its gates and its evidence are all untouched.Linked Issues
Part of #8769. Builds on #8971 (merged). Blocked for worktree reviews on #8972.
中文说明
这个 PR 做了什么
/review的 Step 3A 扇出现在可以由代码分派，而不是由编排器手工发射；选择性启用，legacy 路径原样保留，一个开关即可回退。qwen review emit-workflow用与agent-prompt --roster完全相同的方式构建 roster —— 同一份 plan、同一个函数（buildLaunch）、同样的 brief、同样的 prompt、同样落盘的交付证据 —— 只是把这些 prompt 写进一个可运行的 workflow 脚本，而不是打印十三个块让编排器逐个复制。于是模型的调用只带一个路径、零负载。roster 被烘进生成的文件而不是通过
args传递，这是被迫而非选择：Workflow 工具的args只接受内联 JSON、没有路径形式，而 vm 沙箱没有文件系统 —— 它的全局只有agent/parallel/pipeline/phase/log/console/args/budget/workflow，没有任何能打开文件的东西。因此放在args里的 roster 就是需要模型逐字敲进工具调用的 roster，正是本变更要消灭的那个失败，只是上移了一层。同理，脚本必须住在 saved-workflow 目录：Workflow({scriptPath})经readWorkflowFileSecurely加载，该函数对 realpath 做边界检查并拒绝目录之外的一切。又因为该目录里每个文件同时是用户会话中的一条/<name>斜杠命令，cleanup会按前缀清扫生成的脚本 —— 留下一个就等于给用户一条指向已不存在 diff 的永久命令。生成的文件是固定主体加一个 JSON 字面量。不生成逻辑、只生成数据，所以可能出错的部分正是测试所执行的部分 —— 而且测试执行的是生成器的真实输出，包含 prompt 里带反引号、
${、反斜杠和换行的 roster。技能询问该用哪个引擎，而不自行判断。 Step 3A 现在先运行
emit-workflow并按其退出码分支：6 表示本次 review 走 legacy 路径且什么都没写，0 表示脚本已生成、扇出就是一次不带args的Workflow({scriptPath})调用。退出码 6 对齐 reverse-audit 构建器的 4（预算停止）与 5（已收敛）—— 是终止裁决而非失败，明确不需要修复或重试。两个开关，不是一个。
QWEN_CODE_ENABLE_WORKFLOWS表示运行时是否可以跑脚本；QWEN_REVIEW_WORKFLOW表示/review是否可以把它的扇出路由过去。两者都必须设置。取消第二个就是一键回退，且不影响运行时的其余部分。为什么需要
Step 3 最脆的一层是编排器。它是一个模型，只能靠约定约束 —— 在单条 response 里发出全部 agent 调用、逐字使用 CLI 的块、不得缩编 roster —— 而这个技能里的整套门禁之所以存在，正是因为每一条约定都在真实运行中被打破过：一个 PR 被 approve 时 22 个 agent 零工具调用、23 个 chunk agent 的 prompt 里没有一个提到 diff 文件、whole-diff agent 被发射时根本没有 diff。所有这些门禁都是在失败发生之后才发现它。
由生成的脚本分派，使这一类失败在结构上不可能发生于扇出环节：roster 是 CLI 写进文件的数据，遍历它的循环无法缩短它，也不存在跳过某个 agent 的分支。prompt 自始至终不经过模型。
这是能证明该论点的最小变更，并且刻意不是对 Step 3 的重写。两条路径共享的是一切决定 review 质量的东西 —— 同一份 plan、roster、brief 与 prompt，同一个
general-purpose子 agent 类型，同一个读取同样 harness transcript 的覆盖率门禁。差别只在谁发射，这正是让两者之间的 A/B 可读的原因。本 PR 最初被 park 为 draft 的两条理由现已解除：workflow 分派的 agent 现在会写 subagent transcript（#8971，已合入），而那是
check-coverage唯一接受的证据；技能现在也确实路由到了这个命令，此前并没有。审阅者验证方案
如何验证
预期：
3275 passed | 8 skipped，外加一个既有失败（run-ledger.test.ts > refuses to append over a ledger it could not read），该失败在本环境下的未修改main上同样出现 —— 它把文件 chmod 为只读并期望写入失败，而以 root 运行测试时这一前提不成立。已通过检出main版本的src/commands/review/lib/重跑核对：同一个失败。陈述本变更契约的测试：
workflow-script.test.ts执行生成的脚本而非手写类比：每条 roster 恰好分派一次、prompt 穿过 JSON-in-JS 序列化后字节不变、断言agentType、抛错的 dispatch 按名字收进missingRoles、undefined返回算缺失而不是空结果集、全员失败与空 roster 都抛错，以及一条断言「生成的那一半不含任何分派逻辑」。emit-workflow.test.ts钉住：emit 出的 roster 与 prompt 和--roster会产出的完全一致、每条 prompt 都记录在check-coverage会去查的位置、脚本落在 Workflow 加载器会接受的地方，以及 legacy 裁决下什么都不写 —— 没有脚本也没有 prompt 记录，因为一个从未分派的扇出留下的记录会被读作「roster 发射过但什么都没返回」。lib/orchestration.test.ts钉住路由：两个开关缺一不可、kill switch 胜过两个 opt-in、两个开关在两个方向上互相独立、最全局的阻塞原因优先报告，以及structuralBlocker不依赖环境。cleanup.test.ts钉住生成的脚本会被清扫，而用户自己的 saved workflow 不会。端到端：在设置
QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_REVIEW_WORKFLOW=1的 local-diff review 上，Step 3A 打印scriptPath:，运行发出一次Workflow调用，Step 3D 的覆盖率门禁在这些 agent 写下的 transcript 上通过。任一变量未设置时，同一个 review 打印orchestration: legacy — …、退出码 6，并与此前完全一致地继续。证据（前后对比）
N/A —— 没有用户可见或 TUI 变化。两个开关默认都不设置，此时
/review的行为与今天完全相同；终端报告、PR 评论行为与产物格式均未改动。测试环境
环境（可选）
仅单元测试（vitest，Node 22，Linux）。
风险与范围
/review走 workflow 路径与 legacy 路径的对比尚未测量，而那个 A/B 正是「放在开关后面合入」的目的，而不是合入它的证据。agent()能被钉在某个目录（同样是 feat(core): let a workflow agent pin a directory and outlive the default bounds #8972）—— 否则被分派的 agent 会去读用户的主 checkout 并描述错误的树。territory 扇出（Step 3B）同样走 legacy：chunk agent 带着本脚本未表达的 per-territory 契约 —— 分页、uncoverable 规则、Covered:回执、per-chunk retirement 账本 —— 那值得单独一轮设计，而不是与本次一起划范围。Step 4 与 Step 5 未触碰。emit-workflow是增量的；legacy 路径、它的门禁与证据全部未动。关联 Issue
Part of #8769。基于 #8971（已合入）。worktree review 部分阻塞于 #8972。