refactor(review): drop deterministic-analysis and autofix steps

[wenshao]

What this PR does

This PR slims the bundled /review skill from 11 steps to 9 by removing Step 3 (deterministic analysis) — the auto-run of the project's linter and type checker (tsc / eslint / ruff / cargo clippy / go vet) plus CI-lint discovery — and Step 8 (autofix) — the PR-worktree auto-fix that committed and pushed fixes back to the PR branch. The remaining steps are renumbered (4→3 … 11→9) and every cross-reference is updated. Agent 7 (Build & Test) stays in the parallel-review step and now always runs build+test.

Since the qwen review deterministic subcommand was the internal backend for the removed Step 3 (review.ts describes these subcommands as "internal helpers used by the /review skill"), it is now orphaned and is removed together with its import/registration in review.ts. Companion docs are updated to match: DESIGN.md, docs/users/features/code-review.md (11→9 steps, removed Deterministic Analysis / Autofix sections + comparison-table rows), and docs/users/features/commands.md.

Why it's needed

The deterministic-analysis and autofix steps add significant prompt size and operational complexity to the skill: the deterministic step carries a multi-language tool matrix and CI-config parsing, while autofix performs worktree commits and pushes. Removing them makes /review a leaner, LLM-review-focused flow. Code correctness is still covered by the LLM review agents (Agents 1–6) and by Agent 7, which compiles and tests the change. This is a deliberate simplification tradeoff — see Risk & Scope.

Reviewer Test Plan

How to verify

The change is the bundled-skill prompt + its docs, plus removal of the now-unused deterministic CLI subcommand.

• Skill still parses with the real loader: cd packages/core && npx vitest run src/skills/bundled-skills.integration.test.ts → 9 passed.
• CLI loader unaffected: cd packages/cli && npx vitest run src/services/BundledSkillLoader.test.ts → 23 passed.
• After deleting deterministic.ts, the CLI still type-checks: npm --prefix packages/core run build && npm --prefix packages/cli run typecheck reports no review/deterministic error (any remaining errors are pre-existing local dist staleness in unrelated packages; CI builds fresh).
• git grep deterministicCommand → no remaining references; deterministic.ts had no dedicated test.
• Formatting: npx prettier --check on the changed files → clean.

Evidence (Before & After)

Before: 11 steps (incl. Step 3 deterministic analysis and Step 8 Autofix) plus the qwen review deterministic subcommand. After: 9 steps, both removed and the rest renumbered, and the subcommand deleted. N/A for screenshots (skill prompt + docs + dead-code removal).

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	N/A
🐧 Linux	N/A

Skill prompt + docs + dead-code removal with no platform-specific behavior; the vitest suites and typecheck run on all three OSes in CI.

Environment (optional)

Unit tests + typecheck (vitest / tsc); no runtime/sandbox needed.

Risk & Scope

• Main risk or tradeoff: /review no longer auto-runs a linter/type checker, so lint/type regressions lose their deterministic safety net — coverage now relies on the LLM review agents plus Agent 7's build (which includes compilation). This is the intended simplification tradeoff and runs counter to the original "deterministic ground-truth" design rationale.
• Behavior change: no deterministic-analysis step and no PR autofix; the qwen review deterministic subcommand is removed (it was the Step 3 backend and became orphaned once Step 3 was gone). The local "fix these issues" interactive edit in the present step is retained.
• Not validated / out of scope: no end-to-end /review run against a live PR; verification is the unit tests + typecheck + format/content checks above. No test was removed (deterministic.ts had none).

Linked Issues

N/A.

中文说明

这个 PR 做了什么

把内置 /review skill 从 11 步精简到 9 步,删除 Step 3(确定性分析)——自动跑 linter/类型检查(tsc / eslint / ruff / cargo clippy / go vet)加 CI-lint 发现——和 Step 8(autofix)——在 PR worktree 里自动修复并 commit、push 回 PR 分支。剩余步骤重编号(4→3 … 11→9),所有交叉引用同步。Agent 7(Build & Test)保留在并行 review 步骤,现在总是跑 build+test。

由于 qwen review deterministic 子命令是被删的 Step 3 的内部后端(review.ts 把这些子命令描述为_"internal helpers used by the /review skill"_),它现在 orphaned,故连同 review.ts 里的 import/注册一并删除。配套文档同步更新:DESIGN.md、docs/users/features/code-review.md(11→9 步、删除 Deterministic Analysis / Autofix 两节 + 对比表行)、docs/users/features/commands.md。

为什么需要

确定性分析和 autofix 两步给 skill 增加了相当多的 prompt 体积和操作复杂度:确定性步骤带多语言工具矩阵和 CI 配置解析,autofix 执行 worktree 的 commit 和 push。移除它们让 /review 更精简、聚焦 LLM 评审。代码正确性仍由 LLM review agents(Agent 1–6)和会编译并测试改动的 Agent 7 覆盖。这是有意的简化权衡——见风险与范围。

Reviewer Test Plan

如何验证

改动是内置 skill 的 prompt + 文档,外加删除已不再使用的 deterministic CLI 子命令。

• skill 仍能被真实 loader 解析:cd packages/core && npx vitest run src/skills/bundled-skills.integration.test.ts → 9 passed。
• CLI loader 不受影响:cd packages/cli && npx vitest run src/services/BundledSkillLoader.test.ts → 23 passed。
• 删除 deterministic.ts 后 CLI 仍能类型检查:npm --prefix packages/core run build && npm --prefix packages/cli run typecheck 无 review/deterministic 相关错误(剩余错误是本地无关包 dist 陈旧的既有问题;CI 全新构建)。
• git grep deterministicCommand → 无残留引用;deterministic.ts 无专用测试。
• 格式:对改动文件 npx prettier --check → 干净。

证据(Before & After)

Before:11 步(含 Step 3 确定性分析、Step 8 Autofix)加 qwen review deterministic 子命令。After:9 步、两步删除、其余重编号、子命令删除。截图 N/A(skill prompt + 文档 + 死代码删除)。

测试平台

OS	状态
🍏 macOS	✅
🪟 Windows	N/A
🐧 Linux	N/A

skill prompt + 文档 + 死代码删除,无平台特定行为;vitest 套件和 typecheck 在 CI 三个 OS 上都会跑。

环境(可选)

单元测试 + typecheck(vitest / tsc);不需要运行时/sandbox。

风险与范围

• 主要风险或权衡:/review 不再自动跑 linter/类型检查,lint/type 回归失去确定性安全网——覆盖现在依赖 LLM review agents 加 Agent 7 的 build(含编译)。这是有意的简化权衡,与原本"确定性 ground-truth"设计理念相反。
• 行为变更:没有确定性分析步骤、没有 PR autofix;qwen review deterministic 子命令已删除(它是 Step 3 的后端,Step 3 删了它就 orphaned)。present 步骤里本地的 "fix these issues" 交互式编辑保留。
• 未验证 / 范围外:没有针对真实 PR 端到端跑 /review;验证是上面的单元测试 + typecheck + 格式/内容检查。没有删除任何测试(deterministic.ts 本就没有)。

Linked Issues

N/A。

[qwen-code-ci-bot]

Thanks for the PR! (Re-run after multiple review rounds.)

Template looks good ✓ — all required headings present, PR body updated to match the final shape (deterministic subcommand removed, not retained).

On direction: this is a deliberate simplification — dropping the deterministic-analysis step (auto-running linters/typecheckers) and the autofix step from /review, reducing it from 11 to 9 steps. The motivation is sound: the deterministic step carries a multi-language tool matrix and CI-config parsing that adds significant prompt size and operational complexity, and the autofix step performs worktree commits/pushes that are risky and hard to get right. The tradeoff is clearly stated — lint/type regressions lose their deterministic safety net and rely on LLM agents + Agent 7's build. The exclusion criteria are updated to compensate: substantive linter/typecheck issues (unused variables, unreachable code, type errors) are now explicitly in-scope for LLM review, decoupled from pipeline state. No CHANGELOG reference, but the area is directly relevant to the project's review pipeline.

On approach: the scope is tight and well-contained. Every change in the diff is a necessary consequence of removing the two steps — step renumbering, source tag cleanup ([linter]/[typecheck] removed), dead-code deletion (~740-line deterministic.ts removed), doc updates, and a new regression test. No drive-by refactors, no scope creep. The security hardening in Agent 7's CI config discovery ("read CI config from the base branch, not the worktree — the PR branch is untrusted") is a good carry-over from the deleted Step 3. Moving on to code review. 🔍

中文说明

感谢贡献！（多轮 review 后重新运行。）

模板完整 ✓——所有必要标题齐全，PR body 已更新以反映最终形态（deterministic 子命令已删除，非保留）。

方向：这是一次有意的简化——从 /review 中移除确定性分析步骤（自动运行 linter/类型检查）和 autofix 步骤，从 11 步精简到 9 步。动机合理：确定性步骤带着多语言工具矩阵和 CI 配置解析，增加了大量 prompt 体积和操作复杂度；autofix 步骤执行 worktree 的 commit/push，风险高且难做好。权衡明确——lint/类型回归失去确定性安全网，改由 LLM agents + Agent 7 的 build 覆盖。排除标准已同步更新并解耦：实质性 linter/类型检查问题（未使用变量、不可达代码、类型错误）现在明确 in scope，不依赖 pipeline 状态。CHANGELOG 无直接参考，但该区域与项目的 review 流水线直接相关。

方案：范围紧凑且自包含。diff 中每一处改动都是删除这两步的必然后果——步骤重编号、source tag 清理、死代码删除（约 740 行 deterministic.ts）、文档更新、新增回归测试。无顺手重构，无范围蔓延。Agent 7 的 CI 配置安全加固（"从 base 分支读 CI 配置，不从 worktree——PR 分支不可信"）是从被删 Step 3 延续过来的好细节。进入代码审查 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review (re-run)

Independent proposal (before reading the diff): to remove deterministic analysis and autofix from /review, I'd delete the deterministic.ts subcommand (~740 lines), remove its import/registration from review.ts, delete the Step 3 and Step 8 sections from SKILL.md, renumber all subsequent steps (4→3 … 11→9), remove [linter]/[typecheck] source tags from the dedup and exclusion criteria sections, update all cross-references in cleanup.ts, presubmit.ts, and docs (code-review.md, DESIGN.md, commands.md). I'd add a test guarding against re-adding the deterministic subcommand.

Comparison with the diff: the PR matches this proposal exactly. Every change is a necessary consequence of removing the two steps. Verified:

• Dangling references: grep for deterministicCommand, deterministic.ts, [linter], [typecheck], Step 8.*Autofix, Step 10, Step 11, pre-autofix — all clean, zero hits across packages/ and docs/.
• Step count: grep -c '## Step' on SKILL.md = 9. Correct.
• Step renumbering: consistent across SKILL.md, DESIGN.md, code-review.md. cleanup.ts (Step 11→9) and presubmit.ts (Step 9→7, comment + yargs describe) cross-references updated.
• Source tag cleanup: [linter] and [typecheck] removed from the dedup section's deterministic source list and from the exclusion criteria. The exclusion criteria now reads: cosmetic style/formatting excluded, but substantive lint/type issues (unused variables, unreachable code, type errors) are in scope — decoupled from pipeline state, no "deterministic tool" wording.
• Security hardening: Agent 7's CI config discovery now includes "read CI config from the base branch, not the worktree" — previously only in the deleted Step 3.
• New test: review.test.ts asserts exactly 5 registered subcommands (no deterministic), describe text doesn't mention "deterministic". Solid regression guard.
• review.ts cleanup: import removed, demandCommand message updated, describe text updated. Clean.

No correctness issues, no AGENTS.md violations. The diff is clean and focused.

Verification

Worktree npm install timed out (large monorepo), so tests were not run locally. CI status from GitHub:

Test (ubuntu-latest, Node 22.x):  pending (running)
Classify PR:                      pass
Remind on force-push:             pass

Dangling-reference checks (run locally on PR code):

grep deterministicCommand/deterministic.ts:  0 hits ✓
grep [linter]/[typecheck] in SKILL.md:       0 hits ✓
grep Step 10/Step 11/pre-autofix:            0 hits ✓
grep -c '## Step' SKILL.md:                  9 ✓

Tmux Testing

N/A — this is a prompt + docs + dead-code removal PR with no user-visible CLI behavior change. The /review skill pipeline changes are prompt-text edits and a deleted TypeScript module, exercised through unit tests, not through interactive CLI usage.

中文说明

代码审查（重新运行）

独立方案（看 diff 前）：要从 /review 中移除确定性分析和 autofix，我会删除 deterministic.ts 子命令（约 740 行）、从 review.ts 移除 import/注册、从 SKILL.md 删除 Step 3 和 Step 8、重编号后续步骤（4→3 … 11→9）、从去重和排除标准中移除 [linter]/[typecheck] source tags，更新 cleanup.ts、presubmit.ts 和文档中的所有交叉引用。我会加一个测试防止重新添加 deterministic。

与 diff 对比： PR 完全匹配这个方案。每处改动都是移除两步的必然后果。已验证：

• 残留引用： grep 检查 deterministicCommand、deterministic.ts、[linter]、[typecheck]、Step 8.*Autofix、Step 10、Step 11、pre-autofix——全部干净，零命中。
• 步骤数： SKILL.md 中 grep -c '## Step' = 9。正确。
• 步骤重编号： SKILL.md、DESIGN.md、code-review.md 一致。cleanup.ts（Step 11→9）和 presubmit.ts（Step 9→7，注释 + yargs describe）交叉引用已更新。
• Source tag 清理： 从去重和排除标准中移除了 [linter] 和 [typecheck]。排除标准现在：cosmetic 风格/格式排除，但实质 lint/类型问题（未使用变量、不可达代码、类型错误）in scope——与 pipeline 状态解耦。
• 安全加固： Agent 7 的 CI 配置发现现在包含"从 base 分支读 CI 配置，不从 worktree"——之前只在被删 Step 3 中有。
• 新测试： review.test.ts 断言恰好 5 个注册子命令（无 deterministic），describe 文本不含 "deterministic"。可靠的回归保护。
• review.ts 清理： import 移除、demandCommand 消息更新、describe 文本更新。干净。

无正确性问题，无 AGENTS.md 违规。diff 干净且聚焦。

验证结果

Worktree npm install 超时（大型 monorepo），未在本地运行测试。CI 状态来自 GitHub：

Test (ubuntu-latest, Node 22.x):  pending（运行中）
Classify PR:                      pass
Remind on force-push:             pass

残留引用检查（在 PR 代码上本地运行）：

grep deterministicCommand/deterministic.ts:  0 命中 ✓
grep [linter]/[typecheck] in SKILL.md:       0 命中 ✓
grep Step 10/Step 11/pre-autofix:            0 命中 ✓
grep -c '## Step' SKILL.md:                  9 ✓

Tmux 测试

N/A——这是一个 prompt + 文档 + 死代码删除的 PR，没有用户可见的 CLI 行为变更。/review skill 流水线的改动是 prompt 文本编辑和一个删除的 TypeScript 模块，通过单元测试覆盖，不需要交互式 CLI 使用来验证。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Reflection (re-run)

Going back to the independent proposal — the PR matches it exactly. Every change is a necessary consequence of removing deterministic analysis and autofix. The diff is focused (9 files, +144 −972), the step renumbering is consistent across all files, the source tag cleanup is thorough, and the new review.test.ts is a good regression guard.

The direction tradeoff is honest and well-reasoned: the deterministic step's multi-language tool matrix and autofix's worktree commit/push add real complexity, and the simplification is worth the loss of the deterministic safety net — especially since Agent 7 still runs build+test, and the exclusion criteria are updated (decoupled from pipeline state) to bring substantive lint/type issues back into LLM scope. The security hardening note carried over from Step 3 to Agent 7 is a thoughtful touch.

Multiple rounds of review feedback have been addressed: the deterministic.ts module was eventually removed (the author reversed their initial call after recognizing it was an internal helper, not a user-facing command), the exclusion criteria contradiction was resolved, DESIGN.md sections were cleaned up, and the PR body was updated to match the final shape. All inline threads are resolved. CI (ubuntu) was green on the prior commit.

Guardrail note: this is a cross-repository fork PR with a refactor title — the approval guardrail applies. Refactors touch structure broadly and a fork author is not a trusted committer, so this needs a human maintainer's eye before merging. Escalating to the maintainer for final decision.

中文说明

反思（重新运行）

回到独立方案——PR 完全匹配。每处改动都是移除确定性分析和 autofix 的必然后果。diff 聚焦（9 个文件，+144 −972），步骤重编号在所有文件中一致，source tag 清理彻底，新的 review.test.ts 是好的回归保护。

方向的权衡诚实且有理：确定性步骤的多语言工具矩阵和 autofix 的 worktree commit/push 确实增加了复杂度，简化值得——尤其是 Agent 7 仍然运行 build+test，排除标准已更新（与 pipeline 状态解耦）将实质 lint/类型问题重新纳入 LLM 范围。从 Step 3 延续到 Agent 7 的安全加固是个用心的细节。

多轮 review 反馈已处理：deterministic.ts 模块最终被删除（作者在认识到它是内部 helper 而非面向用户的命令后推翻了初始判断），排除标准矛盾已解决，DESIGN.md 各节已清理，PR body 已更新以反映最终形态。所有 inline thread 已 resolve。CI（ubuntu）在上一个 commit 上已通过。

护栏说明： 这是一个跨仓库 fork PR 且标题为 refactor——审批护栏适用。重构广泛触及结构，fork 作者不是可信提交者，合并前需要人类维护者的审查。已转交维护者做最终决定。

— Qwen Code · qwen3.7-max

[DragonnZhang]

Stale step reference in lightweight-mode skip list. The old text was:

Skip Steps 2 (no local rules), 3 (no local linter), 8 (no local files to fix), 10 (no local cache)

The PR removed Step 3 (Deterministic analysis — deleted) and Step 8 (Autofix — deleted) from the skip list and renumbered 10→8. But the removed Step 3 (no local linter) was relabeled as Step 2 (no local rules) instead of being dropped entirely. In the new numbering, Step 2 is "Load project review rules" — which IS active in lightweight mode (the loaded rules are injected into review agents in Step 3, including lightweight mode which still runs Agents 1–6).

Suggested fix: remove "Skip Step 2 (no local rules) and" so the line reads:

Skip Step 8 (no local cache). In Step 9, skip worktree removal …

[wenshao]

Thanks for looking, but I think this is a misread of the diff. The original line on main already skipped Step 2: Skip Steps 2 (no local rules), 3 (no local linter), 8 (...), 10 (...). In lightweight (cross-repo) mode there is no local checkout of the target repo, so qwen review load-rules has no local .qwen/review-rules.md / AGENTS.md to read — it loads rules from the base branch, which isn't present locally in cross-repo mode. So Skip Step 2 (no local rules) preserves the original design; it isn't a relabel of the removed Step 3. Step 2 is genuinely skipped in lightweight mode, so I've kept the line as-is.

中文

感谢 review,但我认为这是对 diff 的误读。main 上的原文本来就跳过 Step 2:Skip Steps 2 (no local rules), 3 (no local linter), 8 (...), 10 (...)。lightweight(跨仓库)模式下本地没有目标仓库的 checkout,所以 qwen review load-rules 没有本地 .qwen/review-rules.md / AGENTS.md 可读——它从 base 分支加载 rules,而跨仓库模式下本地没有该 base 分支。因此 Skip Step 2 (no local rules) 是保留原设计,不是把已删的 Step 3 重命名。lightweight 模式确实跳过 Step 2,所以这行我保持原样。

[qwen-code-ci-bot]

Additional suggestions (not inline-annotatable):

[Suggestion] Exclusion criteria creates coverage blind spot (SKILL.md:577) — The exclusion criteria tells LLM agents: "do NOT flag issues that a linter or type checker would catch automatically". But with Step 3 (deterministic analysis) removed, no step runs linters or standalone type checkers anymore. Agent 7's build catches compilation failures, but lint warnings, unused variables, and strict-mode type issues that tsc --noEmit / eslint / ruff would catch are now in a void — agents are told to skip them, but no tool catches them. Consider narrowing the exclusion to purely stylistic issues.

[Suggestion] Orphaned qwen review deterministic CLI subcommand — packages/cli/src/commands/review/deterministic.ts (743 lines) is still imported and registered in review.ts (line 15, 28, 33), but the skill no longer calls it. Consider removing it or adding a deprecation note.

[Suggestion] Documentation not updated — docs/users/features/code-review.md still has full "Deterministic Analysis" and "Autofix" sections, old step numbering, and a comparison table. docs/users/features/commands.md line 132 still mentions "deterministic analysis". These should be updated to match.

[Suggestion] Stale step-number comments in CLI source — packages/cli/src/commands/review/cleanup.ts:7 says "Step 11" (now Step 9) and packages/cli/src/commands/review/presubmit.ts:7,260 says "Step 9" (now Step 7).

— qwen3.7-max via Qwen Code /review

[wenshao]

Thanks for the thorough review — pushed 0d56709 addressing the inline and body suggestions.

Applied:

• [Critical] CI-config base-branch protection — re-added to Agent 7 (see inline reply). For PR reviews it now discovers CI-config commands from the base branch (git show <base>:<path>), not the untrusted worktree.
• Exclusion-criteria blind spot — narrowed the exclusion to purely cosmetic formatting a formatter would normalize. Substantive lint/type issues (unused variables, unreachable code, type errors) are now explicitly not excluded, since no deterministic tool catches them anymore.
• Documentation — updated docs/users/features/code-review.md (11→9 steps, removed the Deterministic Analysis and Autofix sections + the two comparison-table rows, renumbered the Token-efficiency table) and docs/users/features/commands.md (agent count).
• Stale CLI step-number comments — fixed cleanup.ts (Step 11→9) and presubmit.ts (Step 9→7, comment + yargs describe).

Not applied (with reason):

• Orphaned qwen review deterministic subcommand — kept intentionally. qwen review deterministic is a standalone CLI command a user can still invoke directly, so it isn't dead code — just no longer auto-called by the skill. Removing the 743-line module + its registration is out of scope for this doc-focused change; happy to do it as a separate cleanup PR if maintainers prefer.

Rebuttal:

• Lightweight-mode Step 2 note (@DragonnZhang) — replied inline. The original main text already skipped Step 2, and lightweight mode has no local rules to load, so keeping Skip Step 2 (no local rules) is correct, not a stale relabel.

中文

感谢细致的 review——已推送 0d56709 处理 inline 和正文的建议。

已采纳:

• [Critical] CI-config base-branch 保护 — 重新加到 Agent 7(见 inline 回复)。PR review 现在从 base 分支(git show <base>:<path>)发现 CI-config 命令,而非不可信的 worktree。
• Exclusion 覆盖盲区 — 把排除项收窄为_纯粹的、formatter 会自动规范化的格式问题_。实质的 lint/type 问题(未使用变量、不可达代码、类型错误)现在明确不排除,因为已经没有确定性工具去抓它们了。
• 文档 — 更新了 docs/users/features/code-review.md(11→9 步、删除 Deterministic Analysis 和 Autofix 两节 + 对比表两行、重编号 Token 表)和 docs/users/features/commands.md(agent 数)。
• CLI 过时步骤号注释 — 修了 cleanup.ts(Step 11→9)和 presubmit.ts(Step 9→7,注释 + yargs describe)。

未采纳(附理由):

• 孤立的 qwen review deterministic 子命令 — 有意保留。qwen review deterministic 是用户仍可直接调用的独立 CLI 命令,所以不是死代码——只是 skill 不再自动调用它。删掉这个 743 行模块 + 其注册超出本次以文档为主的改动范围;如果维护者希望,我可以另开一个清理 PR。

反驳:

• Lightweight 模式 Step 2(@DragonnZhang) — 已 inline 回复。main 原文本来就跳过 Step 2,且 lightweight 模式没有本地 rules 可加载,所以保留 Skip Step 2 (no local rules) 是正确的,不是过时的重命名。

[wenshao]

⚠️ Downgraded from Approve to Comment: self-PR; CI still running.

Additional observations (not inline-annotatable):

[Suggestion] Dead code: deterministic.ts still registered in review.ts — qwen review deterministic is still imported and registered in review.ts:15-34, appearing in qwen review --help with "deterministic analysis" in the describe text. The skill prompt no longer invokes it. The PR acknowledges this is intentional, but the stale describe text is actively misleading. Consider removing the import/registration or updating the describe text.

[Suggestion] No test validates new 9-step structure — bundled-skills.integration.test.ts only verifies frontmatter parsing. No test asserts the absence of autofix, [linter], [typecheck], Step 10, Step 11 or the presence of Steps 1-9. Consider adding content assertions to prevent accidental reintroduction.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The qwen review deterministic subcommand (packages/cli/src/commands/review/deterministic.ts, ~740 lines) and its registration in review.ts are now orphaned — no SKILL.md step invokes it. The PR description explicitly notes this was left in place, but it still ships in the CLI bundle and appears in qwen review --help. If a future SKILL.md edit accidentally references it, the subcommand will produce findings tagged [linter]/[typecheck] that the downstream pipeline no longer recognizes as pre-confirmed (Step 4 deduplication only recognizes [build]/[test]). Consider removing it in a follow-up or adding a @deprecated note.

— qwen3.7-max via Qwen Code /review

[yiliang114]

Thanks for the follow-up. I did a light pass over the latest revision: the /review flow is consistently reduced to 9 steps, the main user docs and CLI comments are aligned, the base-branch CI-config protection is restored, and the formatting/CI checks are clean. I only see a couple of non-blocking DESIGN.md wording nits that can be handled separately, so this looks good to merge from my side.

[wenshao]

Pushed da71ce481 addressing the second review round.

Reversed my earlier call — removed deterministic.ts. Last round I argued for keeping it as a standalone command. On closer look, review.ts describes these subcommands as "internal helpers used by the /review skill" — deterministic isn't a user-facing command, it's the skill's Step 3 backend. With Step 3 gone it has no caller, so it was genuinely orphaned. Deleted the ~740-line module and its import / registration / describe / subcommand-list wiring in review.ts. git grep confirms no remaining reference and there was no dedicated test. Thanks for pushing on this — you were right.

Other suggestions applied (all inline-resolved):

• Exclusion criterion decoupled from pipeline state — substantive lint/type issues are now "in scope", not conditioned on "no deterministic tool catches them".
• SKILL.md worktree install note: dropped the stale "linting".
• DESIGN.md: section renamed to "presubmit and cleanup", "lint" dropped from the review-tools alternative, and the "we already have those" cell fixed.

Verified: cli typecheck is clean of any review/deterministic error (the remaining errors are pre-existing local dist staleness in unrelated packages — CI builds fresh, and the prior commit's ubuntu CI was green), integration + loader tests pass, prettier clean, dist synced.

中文

已推送 da71ce481 处理第二轮 review。

推翻我上一轮的判断——删除了 deterministic.ts。 上一轮我主张保留它作为独立命令。再仔细看,review.ts 把这些子命令描述为 "internal helpers used by the /review skill"——deterministic 不是面向用户的命令,而是 skill 的 Step 3 后端。Step 3 删了之后它没有调用者,所以确实 orphaned。删掉了约 740 行的模块及其在 review.ts 里的 import / 注册 / describe / 子命令列表接线。git grep 确认无残留引用,且没有专用测试。感谢坚持——你是对的。

其他建议已采纳(均已 inline resolve):

• Exclusion 判据与 pipeline 状态解耦——实质 lint/type 问题现在"in scope",不再以"没有确定性工具抓它们"为条件。
• SKILL.md worktree 安装说明:删掉过时的"linting"。
• DESIGN.md:节标题改为"presubmit and cleanup"、review-tools 备选去掉"lint"、"we already have those"单元格修正。

已验证:cli typecheck 无任何 review/deterministic 相关错误(剩余错误是本地无关包 dist 陈旧的既有问题——CI 全新构建不受影响,且上一个 commit 的 ubuntu CI 已通过)、integration + loader 测试通过、prettier 干净、dist 已同步。

[yiliang114]

Thanks for the follow-up. One small housekeeping item before merge: the PR body still reflects the earlier revision. It says this is a prompt/docs-only change with no TypeScript logic change, and that qwen review deterministic is left in place, but the latest commit deletes the deterministic helper and unregisters it from review.ts. Could you update the test plan / risk scope to match the final shape?

While there, docs/users/features/code-review.md still says dependencies are installed so “linting and build/test work”; now that standalone linting is gone, “build/test work” would be more accurate.

The code diff itself looks clean from my side. Ubuntu CI is green; only the review job is still pending.

[yiliang114]

LGTM

[wenshao]

@qwen-code /triage

[wenshao]

@yiliang114 thanks — updated the PR body to match the current state: it now says the qwen review deterministic subcommand is removed, drops the "no TypeScript logic change" claim, and adds the typecheck step (with the note that the only remaining local typecheck errors are pre-existing dist staleness in unrelated packages, which CI builds fresh) to the test plan.

Also pushed 5172f9d8d for the latest ci-bot round (all inline threads resolved):

• Exclusion-criteria contradiction — merged into a single bullet so cosmetic style/formatting/naming stays excluded while substantive linter/type-checker issues (unused vars, unreachable code, type errors) are in scope even where the surrounding code tolerates them. Kept decoupled from pipeline state (no "deterministic tool" wording), and applied the same merge in code-review.md.
• Lightweight skip — (no local reports or cache) to match the Step 8 title.
• DESIGN.md — CI-config section renamed to "auto-discover build/test commands".

CI (ubuntu) is green.

中文

@yiliang114 感谢——已更新 PR body 以反映当前状态:现在写明 qwen review deterministic 子命令已删除、去掉"无 TypeScript 逻辑改动"的表述,并在 test plan 里加了 typecheck 步骤(并注明本地仅剩的 typecheck 报错是无关包 dist 陈旧的既有问题,CI 全新构建不受影响)。

另外推送了 5172f9d8d 处理最新一轮 ci-bot(所有 inline thread 已 resolve):

• Exclusion 判据矛盾 — 合并为一条:cosmetic 风格/格式/命名仍排除,而实质 linter/type 问题(未使用变量、不可达代码、类型错误)in scope,即使周围代码容忍它们。保持与 pipeline 状态解耦(不写"deterministic tool"),并在 code-review.md 做同样合并。
• Lightweight skip — 改为 (no local reports or cache) 以匹配 Step 8 标题。
• DESIGN.md — CI-config 节标题改为"auto-discover build/test commands"。

CI(ubuntu)已通过。

[wenshao]

⚠️ Downgraded from Approve to Comment: self-PR.

[Suggestion] Orphaned "linting" reference in Worktree Isolation section

The deleted Deterministic Analysis section previously gave "linting" its meaning here. The line at docs/users/features/code-review.md:76 still reads:

Dependencies are installed in the worktree (npm ci, etc.) so linting and build/test work

But no linting step remains. The SKILL.md equivalent (line 62) was correctly updated to "agents, build/test" — this doc should match.

- Dependencies are installed in the worktree (`npm ci`, etc.) so build/test work

— qwen3.7-max via Qwen Code /review

[wenshao]

Pushed 39f843547 for the latest round:

• Orphaned "linting" in the Worktree Isolation section (code-review.md:76) — dropped; only build/test remain now that Step 3 is gone.
• CLI subcommand-surface test — added packages/cli/src/commands/review.test.ts guarding that qwen review registers exactly the five remaining helpers (fetch-pr, pr-context, load-rules, presubmit, cleanup), never re-adds deterministic, and that describe stays clean. review.ts had no test before.
• The "changelog-style phrasing" note on code-review.md:235 was anchored to a pre-5172f9d8d line that had already been merged away — replied inline.

All ci-bot inline threads are now resolved. The only remaining unresolved thread is DragonnZhang's lightweight-mode Step 2 note, which I rebutted earlier (the original main text already skipped Step 2, and lightweight has no local rules to load).

中文

推送了 39f843547 处理最新一轮:

• Worktree Isolation 一节里孤立的 "linting"(code-review.md:76)——已删除;Step 3 没了之后只剩 build/test。
• CLI 子命令面测试 — 新增 packages/cli/src/commands/review.test.ts,断言 qwen review 恰好注册五个剩余 helper(fetch-pr、pr-context、load-rules、presubmit、cleanup)、绝不重新加入 deterministic、且 describe 保持干净。review.ts 之前没有测试。
• code-review.md:235 的 "changelog 措辞" 意见锚定的是 5172f9d8d 之前、已被合并掉的旧行——已 inline 回复。

所有 ci-bot 的 inline thread 现已全部 resolve。唯一剩下的 unresolved 是 DragonnZhang 的 lightweight Step 2 意见,我此前已反驳(main 原文本就跳过 Step 2,且 lightweight 无本地 rules 可加载)。

[wenshao]

@qwen-code /triage

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

No review findings. Downgraded from Approve to Comment: CI still running.

LGTM — clean removal of deterministic analysis and autofix steps. Step renumbering is consistent across all files, deterministic.ts deletion is complete with no dangling imports, build and all relevant tests pass.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

Qwen Code review did not complete successfully. Qwen review aborted with an API error before posting comments. See workflow logs.

[yiliang114]

LGTM. I reviewed the latest head (39f8435): the scope is focused, the removed deterministic/autofix flow is consistently reflected in the CLI registration, bundled skill prompt, and user docs, and I did not find blocking issues.

Verification run locally in an isolated worktree:

• cd packages/cli && npx vitest run src/commands/review.test.ts (3 passed)
• cd packages/core && npx vitest run src/skills/bundled-skills.integration.test.ts (9 passed)
• npx prettier --check on the changed files (clean)

GitHub's main Qwen Code CI check is green; one automatic review workflow job is still pending, but no blockers from this review.

Stale automated review from an earlier commit; the requested items were addressed in later commits and the latest head has been reviewed and approved.

[qwen-code-ci-bot]

[Suggestion] The block comment (line 16) says this test guards against "the describe / demand text drift," but demandCommand: () => stub discards all arguments — the demand message string in review.ts:31 ('Specify a subcommand: fetch-pr, pr-context, load-rules, presubmit, or cleanup.') is never captured or asserted. A future edit could reword that message (e.g., re-add "deterministic") and all three tests would still pass.

Consider capturing the message and asserting on it:

let demandMsg = '';
const stub = {
  command: (m: CommandModule) => {
    names.push(String(m.command).split(' ')[0]);
    return stub;
  },
  demandCommand: (_min: number, msg: string) => { demandMsg = msg; return stub; },
  version: () => stub,
} as unknown as Argv;

Then add:

it('demandCommand message does not mention deterministic', () => {
  registeredSubcommands();
  expect(demandMsg).not.toMatch(/deterministic/i);
});

Alternatively, trim the comment to match reality: "…so a future edit can't silently re-add deterministic or drop one of the others."

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The "Original behavior" paragraph was rewritten to use the new step numbers (Step 7 / Step 9) instead of preserving the historical step numbers (Step 9 / Step 11). On main, this line reads Step 9's three pre-submission checks … and Step 11's cleanup — the before/after contrast with the "Current behavior" paragraph is now lost.

Same issue at line 132: Step 6 resolved verdict to APPROVE was Step 7 on main.

"Original behavior" sections should preserve the historical numbering so readers can see what changed:

Suggested change

	**Original behavior:** Step 7's three pre-submission checks (self-PR detection, CI status, existing-comment classification) and Step 9's cleanup were inlined in SKILL.md as `gh api` / `git` shell commands. The LLM ran each command itself, parsed the output, and applied the classification logic.
	**Original behavior:** Step 9's three pre-submission checks (self-PR detection, CI status, existing-comment classification) and Step 11's cleanup were inlined in SKILL.md as `gh api` / `git` shell commands. The LLM ran each command itself, parsed the output, and applied the classification logic.

— qwen3.7-max via Qwen Code /review