dev-code-review

Dev Code Review

IRON LAW: NO READY VERDICT WITHOUT WIRING EVIDENCE, AND NO COMMIT MESSAGE UNLESS READY.

Focused review on the current git working tree right before a commit. Goal: catch 规范、功能、安全/可靠性风险、闭环、注释、废代码 problems before they enter history.

This is NOT a full architecture review. Stay scoped to what actually changed in this diff.

---

Trigger routing

Use this skill when the user is preparing to commit code or asks to review the current git working tree before committing.

Trigger phrases include:

• 帮我 commit
• 我要 commit
• commit 一下
• commit 这个改动
• 准备 commit
• 准备提交
• pre-commit
• 提交前检查
• 这次改动 review 一下
• 看下这次修改
• commit 前看一下
• I want to commit
• let's commit
• check before commit
• review my changes

Ambiguous commit requests such as 帮我 commit / commit 一下 mean "go through the full commit flow: review first, commit message only if ready". This is the safer default path because team policy requires review before commit.

Only route to dev-commit-writer when the user explicitly says skip review, 跳过 review, 我自审过了, only message, or 只要 message.

For other workflow stages, route to the matching skill:

• Fuzzy requirements before code → dev-grill-docs (dev-spec remains a compatibility alias)
• Spec to implementation plan → dev-plan
• Bug investigation / root cause fix → dev-fix

Optional arguments:

• --staged: limit to staged changes only
• --path=<glob>: limit scope to a path glob

Step 0 — Load baseline

执行前先加载 references/dev-baseline.md。以下行为准则在本 skill 全程生效:不假设、最小代码、外科手术式改动、可验证成功标准。

baseline 与本 skill 的关联点:

• 不假设 —— diff 意图不明确时,在报告里以一句话标注「我对意图的理解可能是 A 或 B,请确认」,而不是默默选一个写进 commit message。
• 外科手术式改动 —— 评审时额外做一道 scope check:diff 是否夹带与主改动无关的「顺手优化」(格式化、邻近重构、无关注释清理)?若有,作为 P1 finding 提示拆 commit。
• 可验证成功标准 —— 「闭环」轴的 grep 验证就是这条准则的具体应用:新符号必须有可见 caller,这是「完成」的可验证判据。

skill 内部规则在与 baseline 冲突时以局部为准(例如严重度 rubric)。

---

Red flags — backtrack if any appear

If any red flag appears, stop and correct the review before reporting:

• You judged 闭环 without running the required git grep evidence command or naming an exemption.
• A finding has no concrete path:line, no clear axis, or no actionable fix.
• You are about to report an architecture/style preference as P0/P1 without a concrete functional, security, reliability, performance, wiring, or maintainability risk.
• The diff is large or mixed-concern, but you have not done the scope check.
• Verdict is not ✅ READY, but you are about to output a Commit section.

---

Step 1 — Gather the change set

Run these read-only commands. They never mutate the working tree:

git status --short
git diff --stat
git diff                  # unstaged
git diff --cached         # staged
git log --oneline -10     # recent commit message style

Scope rules

• If both staged and unstaged changes exist, review both but call out which is which in the report.
• If $ARGUMENTS contains --staged / staged / --cached → restrict to git diff --cached.
• If $ARGUMENTS contains --path=<glob> (or a bare path) → restrict to that path.
• If there are zero changes → stop and tell the user. Do not invent findings.

Diff size / concern grouping

• If the diff is large (>500 changed lines), summarize changed files by module/feature area before judging findings, then review in batches. Keep the final report ≤60 lines; do not downgrade concrete blockers just because the diff is large.
• If the diff mixes unrelated concerns, group your reasoning by logical feature area and apply the baseline scope check. Unrelated drive-by changes that should be split remain a P1 scope finding.
• For generated, lockfile, or pure data-heavy diffs, review the human-authored source of truth first and mention skipped generated/data files in Scope.

File-reading rules

For each modified file, read the full file (not just the hunks) when the diff touches logic, so you can judge whether the change is closed-loop. Skim-only is acceptable for pure config / lockfile / generated code.

Skip / flag patterns

Skip these files' contents (mention they exist in Scope, but don't review):

• Binary files (images, fonts, archives, compiled artifacts)
• Lockfiles: *.lock, pnpm-lock.yaml, package-lock.json, yarn.lock, Cargo.lock, Gemfile.lock, poetry.lock, go.sum
• Generated code: *.g.dart, *.freezed.dart, *.pb.go, *.pb.ts, *_pb2.py, *.gen.ts
• Pure data files > 500 lines

If the diff includes paths that are usually gitignored (node_modules/, .next/, target/, dist/, build/, .venv/, __pycache__/, .DS_Store), emit a P1 finding (axis: 废码) prompting the user to check .gitignore.

---

Step 2 — Severity rubric (read this BEFORE judging findings)

Use this rubric to keep severity consistent across runs:

• P0 (BLOCK) — any of:
  • 未闭环 (new symbol / route / component / config key with no consumer)
  • Secret / credential / API key / token leaked
  • Clear functional defect that will fail on the first real call (null-deref guaranteed, inverted condition, broken auth check, injection)
  • Security flaw with direct exploitability or data exposure (auth bypass, missing tenant/ownership check, unsafe command/file path from user input)
  • Destructive change that breaks others' working tree (broken migration, deleted public API still in use)
• P1 (FIX) — any of:
  • Unhandled edge case (null / empty / 0 / negative / overflow / concurrent re-entry) where the path is reachable
  • Silently swallowed errors (catch {}, except: pass, _ = err)
  • Residual debug output (console.log, print, debugPrint, println!, fmt.Println, NSLog, debugger)
  • Project lint config violation
  • Naming / style severely inconsistent with surrounding file
  • Missing input validation at a trust boundary
  • Reachable reliability/performance regression (race condition, missing transaction/idempotency, N+1/hot-path loop, unbounded memory/IO)
• P2 (NIT) — any of:
  • Redundant / stale / decorative comments
  • Small duplicated block (one helper would do)
  • Style preference, formatting, ordering
  • TODO / FIXME without owner or ticket

When uncertain, prefer the lower severity. Don't inflate to look thorough.

---

Step 3 — Review along 5 axes

For every finding, cite path:line and tag severity using the rubric above.

SDD artifact check

Before judging the functional axis, scan .claude/artifacts/{designs,plans,fixes}/ when it exists:

• If exactly one relevant artifact or one shared slug exists, use it as the review contract.
• If both designs/<slug>.md and plans/<slug>.md exist, check the diff against both the spec acceptance criteria and the plan ADR.
• If fixes/<slug>.md exists, check the diff against the recorded root cause, fix summary, regression test, and verification evidence.
• If multiple unrelated slugs exist, do not guess; mention that artifact alignment could not be determined.

Clear divergence from an active spec, ADR, or fix artifact is a functional finding. If the implementation is reasonable but the artifact is stale, report artifact drift instead of silently accepting the mismatch.

Risk checklist loading

Load references/risk-checklist.md only when relevant:

• Diff touches auth/authz, tenant or ownership boundaries, network/API handlers, database writes/queries, file paths, shell commands, deserialization, crypto, logging of payloads, dependency/source changes, concurrency, caching, pagination, or hot-path loops.
• Diff adds/removes public endpoints, background jobs, migrations, feature flags, SDK/API clients, or code that handles untrusted input.
• You suspect a security/reliability/performance issue but need a compact checklist to avoid missing common cases.

Keep findings mapped to the existing axes. Security, reliability, and performance problems normally belong to 功能, unless the issue is purely unused/dead code.

1. 规范 (Code style & conventions)

先识别本次 diff 涉及的语言,再用该语言的官方/社区规范评审。详细的逐语言检查清单见 references/lang-conventions.md —— 只加载与本次 diff 实际涉及语言相关的小节,不要把整个文件读进来。

项目配置优先级最高:如果项目根目录存在 lint 配置(.eslintrc*、analysis_options.yaml、pyproject.toml、.golangci.yml、clippy.toml、rustfmt.toml、.editorconfig、tsconfig.json、.swiftlint.yml 等),以项目配置为准,覆盖 references 中的通用条目。

通用项(语言无关,所有语言都查):

• Naming 跟随同文件/同模块邻居 —— 改动的命名风格必须和周围一致,不要一个文件里混用 camelCase 和 snake_case。
• Function size sane (< ~50 行)、file size sane (< ~800 行)。仅当本次 diff 让它变得更糟时才 flag。
• No deep nesting (>4) introduced by this change.
• No hardcoded secrets, API keys, tokens, absolute local paths, personal info.
• No 调试输出残留 (console.log / print / debugPrint / println! / fmt.Println / NSLog / debugger 等)。
• Imports / use / require 被实际使用,没有新增未使用的导入。
• Errors handled explicitly at boundaries; no silently swallowed catch {} / except: pass / _ = err。
• Immutability respected where the surrounding codebase uses it.

2. 功能 (Functional correctness)

Read the diff as if you have to ship it:

• Does the change actually do what its commit/PR intent suggests?
• Are edge cases handled? (null / empty / error / loading / 0 / negative / very large)
• Any obvious off-by-one, wrong operator, swapped argument, copy-paste bug?
• Async / await / Future / Promise correctness — no fire-and-forget that should be awaited.
• State updates: are they safe under concurrent calls / re-entry?
• Inputs from network / user / disk validated at the boundary?
• Security boundaries: auth/authz/tenant checks preserved; no injection, SSRF, path traversal, unsafe deserialization, weak crypto, or sensitive logging introduced.
• Reliability/data integrity: transactions/idempotency/retry semantics remain correct; no check-then-act race, lost update, partial write, or resource leak.
• Performance: no new N+1 query, unbounded pagination/load, expensive hot-path loop, cache without invalidation/TTL, or main-thread blocking work.

3. 闭环 (Is the change actually wired up end-to-end?)

This is the most commonly skipped check. Two-valued — there is no "partially closed loop".

For every new public symbol introduced in this diff (function / class / method / route / component / config key / event type / DB column / error type), verify a consumer exists:

git grep -n "<symbol>" -- ':!<defining_file>'

Evidence before claims(必做):必须真正调用 bash 跑 git grep,在报告里展示 command + output(完整粘贴,或简洁引用「N matches in M files: ...」),不能只说「grep 验证通过」。这是 baseline「Evidence before claims」在闭环轴的最严落地 —— 没真跑 grep 就不算闭环检查。

Decision tree:

1. Has external callers → ✓ closed loop
2. No external callers, but symbol is one of the framework-invoked exemptions below → ✓ closed loop
3. Exported / public, 0 callers, not exempt → P0 未闭环 (dangling), name the missing wire-up
4. Private / file-local, 0 callers → P1 废码 (unused private symbol)

Framework-invoked exemptions (count as closed loop even with 0 grep hits):

• Test functions in test files (*_test.*, *.test.*, *.spec.*, test_*.py)
• Framework lifecycle / override methods (Flutter build() / initState() / dispose(), React component default exports, Vue setup(), Android onCreate(), iOS viewDidLoad(), etc.)
• Symbols invoked via reflection / annotations / DI (@Override, @Autowired, @Component, @Injectable, Spring beans, etc.)
• CLI entry points (main(), files declared as bin in package.json / pubspec.yaml)
• Public API surface of a library package (anything re-exported from the package's index file)

Other things to verify in this axis:

• New API endpoints / routes / handlers are registered in the router/module.
• New UI components are rendered by a parent (or are top-level routes).
• New config keys are read by the consuming code.
• New event/message types are handled on the receiving side.
• New DB migrations are used by queries and have a working rollback.
• Tests for new logic exist, OR the user explicitly opted out.

4. 多余注释 (Excess / stale comments)

Comments should explain why, not what. Flag:

• Comments that just restate the next line (// increment counter above i++).
• Commented-out code blocks. (Remove — git already has the history.)
• AI-style narrating comments (// We will now loop through..., // This function...).
• TODO / FIXME without an owner or ticket.
• Stale comments that no longer match the code below them.
• Boilerplate banners / decorative separators added by this diff.
• Translation drift (e.g. a Chinese comment describing English code that no longer matches).

Keep: comments that document a non-obvious why, an invariant, a known bug workaround, or a subtle constraint.

5. 多余/无用代码 (Dead code introduced or left behind)

• Unused variables, parameters, private methods, helper functions added by this diff.
• Unreachable branches (if (false), code after return).
• Duplicated logic — same block copied across files when one helper would do.
• Re-exports / shims / "for backwards compat" wrappers added without a real consumer.
• Empty files, empty classes, placeholder stubs that this diff didn't actually fill in.
• Old code path the new code replaced — should it be deleted in this same commit?
• Deletion candidates: distinguish safe delete now from defer with plan.
  • Safe delete now → list in Cleanup only when evidence shows no active consumers and deletion is within this commit's scope.
  • Defer with plan → report as a P2/P1 finding instead of Cleanup, naming the missing evidence/precondition (telemetry, migration, external consumer check, rollback path).

---

Step 4 — Report

输出语言跟随用户提问语言(中文提问出中文报告)。严格按下列模板,不要加寒暄、不要复述用户原话、不要解释自己在做什么。

Before output, run this self-check:

• Verdict matches the rubric: P0 / 闭环 ✗ / secret → ❌ BLOCK; P1 only → ⚠ FIX P1; no P0/P1/debug/dead-code → ✅ READY.
• Scope includes file count, added/removed lines, and staged/unstaged/both.
• Every new public symbol has git grep evidence or an explicit framework/public-API exemption.
• Every finding has path:line, one of 规范 / 功能 / 闭环 / 注释 / 废码, and one actionable fix sentence.
• Commit is omitted unless Verdict is ✅ READY.
• Full report stays ≤60 lines.

━━━ Dev Code Review ━━━
Verdict   : ✅ READY  /  ⚠ FIX P1  /  ❌ BLOCK
Scope     : <N> files · +<added> / −<removed> · <staged|unstaged|both>
Intent    : <一句话,基于 diff 自己判断改动在做什么>
Languages : <按 diff 行数降序,例如:Dart, TypeScript>   Lint config : <analysis_options.yaml | none>

Axis Check
  规范   ✓ / ⚠ / ✗   <一行说明,仅在非 ✓ 时填>
  功能   ✓ / ⚠ / ✗   ...
  闭环   ✓ / ✗        ...   ← 二值,不存在"部分闭环"
  注释   ✓ / ⚠
  废码   ✓ / ⚠

Findings  (P0 阻塞 · P1 应修 · P2 可选)
  [P0] path/to/file.dart:42         <axis>   <一句话问题>
       → <一句话修复建议>
  [P1] path/to/other.ts:117         <axis>   ...
       → ...
  [P2] ...

Cleanup   (建议在本次 commit 内一并删除)
  - path:line  <注释 / 死代码 / 调试输出>  — <一句话原因>

Commit
  <type>(<scope>): <subject ≤72 chars>
  <空行>
  <body 可选,列要点;无要点则省略 body>
  <空行>
  Refs: spec/<slug>      ← 自动追溯,见规则 11
  Refs: plan/<slug>      ← 同 slug 的 spec + plan 都列

模板填写规则(非常重要)

1. Verdict 三选一,判定逻辑:
   • ❌ BLOCK ← 闭环 ✗ 或 存在 P0 或 检出 secret/credential
   • ⚠ FIX P1 ← 闭环 ✓ 且 无 P0 且 存在 P1
   • ✅ READY ← 闭环 ✓ 且 无 P0/P1 且 无残留调试输出 且 无明显废码
2. Languages 排序:按本次 diff 中各语言的修改行数降序排列,让用户一眼看出主要改动在哪种语言。
3. Axis Check 用 ✓ / ⚠ / ✗ 单字符,仅在非 ✓ 时追加一行说明。全部 ✓ 时整张表保留但说明列留空。
4. Findings 严格按 [P?] path:line <axis> <问题> 单行 + 缩进 → <修复> 单行 的两行结构。axis 取值固定:规范 / 功能 / 闭环 / 注释 / 废码。
5. 没有 P0/P1/P2 时,整段 Findings 写一行 none 即可,不要写"无"、"暂无问题"这类废话。
6. Cleanup 段 仅列出可一次性删除的内容。如无可清理项,整段省略(连标题一起省)。需要迁移、遥测、外部消费者确认或 rollback plan 的删除项不要放 Cleanup,改写成 Finding。
7. Commit 段 仅在 Verdict = ✅ READY 时输出。其他 Verdict 下整段省略 —— 还没到提交时机。
8. Commit message 格式:跟随仓库 git log --oneline -10 观察到的风格;仓库无明显风格时退回 conventional commits(feat|fix|refactor|docs|test|chore|perf|ci|style|build)。subject 不加句号,祈使语气,≤72 字符。
9. 同一文件多个 finding 按行号升序排列。跨文件按 P0 → P1 → P2 排列,同级别内按文件路径字典序。
10. 报告全文 ≤ 60 行;超过则压缩 Findings 描述,不要省略 Verdict / Axis / Cleanup 结构。
11. Refs 自动追溯(仅 Verdict = ✅ READY 时):扫 .claude/artifacts/{designs,plans,fixes}/,按以下规则在 commit message footer 自动加 Refs: 行:
    • 0 个 in-flight artifact → 不加 Refs
    • 1 个 → 自动加(Refs: <type>/<slug>,type ∈ {spec, plan, fix})
    • 同 slug 的 spec + plan 都存在 → 两条都加
    • 多个不同 slug → 不擅自选,在 Commit 段下方加一行 > 检测到多个 in-flight artifact: [...],请告诉我这次 commit 关联哪个,我会把 Refs 加进去
    • .claude/artifacts/ 目录不存在 → 不加(用户没用 dev-skills 流程)

极简示例(用户参考用,实际填真实数据)

━━━ Dev Code Review ━━━
Verdict   : ⚠ FIX P1
Scope     : 3 files · +84 / −12 · staged
Intent    : 给 prepAll transfer 增加 aspirate 流速参数并透传到底层 SDK
Languages : Dart                    Lint config : analysis_options.yaml

Axis Check
  规范   ✓
  功能   ⚠   新参数未做 null/负值校验
  闭环   ✓
  注释   ⚠   保留了一行 // TODO: refine later 无 owner
  废码   ✓

Findings
  [P1] lib/.../prepAll_transfer_aspirate_setting.dart:88   功能   flowRate 允许传入负值,会让底层抛异常
       → 在 setter 里 clamp 到 [0, maxFlow] 或 throw ArgumentError
  [P2] lib/.../prepAll_transfer_aspirate_setting.dart:14   注释   `// TODO: refine later` 无 owner/ticket
       → 删除该 TODO 或改成 `// TODO(jason, #123): ...`

Cleanup
  - lib/.../prepAll_transfer_aspirate_setting.dart:14  TODO 注释  — 无 owner/ticket,应删除

---

Hard rules

• 不要 mutate working tree(不要 git add / git commit / git stash / 改文件),除非用户在确认报告后明确说"帮我修"。
• 不要 编造 line number —— 没把握就只引用文件名。
• 不要 把"代码能跑"等同于"闭环" —— 必须验证调用方存在(或属于豁免清单)。
• 不要 把架构洁癖当成阻塞问题 —— 只有造成具体功能、安全、可靠性、性能、闭环或维护风险时才报 finding。
• 不要 输出超长报告。没问题的轴线一句话带过即可,把篇幅留给真问题。
• 不要 一次性把整个 references/lang-conventions.md 读进来,只读本次 diff 涉及的语言小节。
• 不要 默认加载 references/risk-checklist.md;只有命中 Risk checklist loading 条件时才读。
• 用户说"只看 staged" / "只看某路径"时严格遵守,不要扩大范围。
• 评审与实现分开:本 skill 只评审,不写实现代码。如需修复,由用户在下一轮明确指示后进行。

---

Multi-Agent Profile

Recommended agent_type: worker

Use when:

• A diff is ready for independent review.
• The reviewer did not author the implementation being reviewed.
• The main agent needs a verdict before commit or handoff.

Do:

• Stay read-only.
• Review only the requested scope.
• Prioritize bugs, regressions, missing wiring, missing tests, secrets, and dead code.
• Apply reviewer independence from ../../docs/multi-agent-policy.md.

Do not:

• Mutate the working tree.
• Re-review implementation choices already outside the requested scope unless they create concrete risk.
• Emit a commit message unless the verdict is READY.

Output:

• Verdict
• Scope
• Findings ordered by severity
• Test gaps
• Commit message only when READY