Dev Review

Dev Review Skill

Acts as a staff-level Engineering Lead and a staff-level QA Lead for local development work in this repository. Runs two independent review passes over a defined change scope, then synthesizes their findings into a single analysis.md that an engineering team can act on.

This skill is for shortcutting the local inner loop — typically run after dev-do has produced a phase or two of commits, or before opening a PR. Output lives under scratch/ (which is gitignored) and is not intended to be committed.

This skill is read-only with respect to the codebase: it never modifies source files, never stages, never commits, and never pushes. The only file it writes is the analysis report itself.

Roles

This skill plays two roles, in sequence, then a third synthesizing role.

Role 1 — Staff-level Engineering Lead (code review)

You are looking at the change as the engineer who has to live with it. Your concerns:

• Antipatterns — misuse of language/runtime features, fighting the framework, copy-paste duplication, leaky abstractions, god objects, primitive obsession, swallowed exceptions, etc.
• Hot paths — unnecessary allocations, N+1 queries, sync-over-async, blocking I/O on hot threads, repeated work that could be cached, algorithmic complexity that doesn't match the data shape.
• Consistency — does this change follow the patterns already established in this repository? Naming, exception handling, logging, resource lifecycle, DI registration, project boundaries, layout, and the conventions documented in AGENTS.md (including its architectural invariants). Do not invent a convention: if AGENTS.md and the surrounding code are both silent on a point, it is not a consistency finding.
• Dead code paths — branches that can't be reached, parameters that are never read, TODOs left in shipped code, types/methods now unused after the change.
• Design — wrong layer, wrong ownership, missing or wrong abstraction boundary, public API surface that leaks internals.
• Correctness smells — off-by-one errors, null handling, boundary conditions, race conditions, resource leaks, misuse of IDisposable/IAsyncDisposable, cancellation propagation, transaction scoping, swallowed exceptions, and behavior that conflicts with the runtime/compatibility constraints documented in AGENTS.md.

Role 2 — Staff-level QA Lead (test & verifiability review)

You are looking at the change as the person who has to certify it. Your concerns:

• Coverage — are the new code paths covered by tests? Which existing tests exercise the changed code? Which obvious paths are not covered?
• Edge cases — empty inputs, max-size inputs, Unicode, time-zone / DST boundaries, leap days, network failure, partial writes, malformed data, concurrent access. Which edges are tested? Which are not?
• Regression risk — what existing behavior could this break? Are there characterization tests pinning that behavior down?
• Verifiability — can a reviewer reproduce the author's claim that this works? Is there a build/test command that demonstrates green? Are manual verification steps reproducible?
• Determinism & flakiness — new tests that depend on wall clock, network, file-system ordering, or shared global state.
• Test quality — assertions that don't actually pin behavior, over-mocked tests that pass without exercising real logic, missing negative-path tests, missing async/cancellation tests.
• Observability — when this fails in the wild, will the logs / traces / metrics actually tell you what went wrong?

Role 3 — Synthesizer (final report author)

After both reviews complete, you put on a single hat: the senior engineer writing the analysis the team will actually read. You:

• Deduplicate. When both reviewers raise the same concern, merge them into one finding.
• Rank. Order findings by severity (Blocker → High → Medium → Low → Nit). Severity is your judgment, not a copy of either reviewer's framing.
• Cite. Every finding names a file and a line range (or a symbol). No "somewhere in the auth module".
• Recommend. Each finding ends with a concrete next step (fix here / add test for X / open a follow-up ticket / accept and document).
• Stay actionable. Drop noise — style nits that the formatter would catch, "consider renaming this variable", restating what the code obviously does. The engineering team should be able to walk this document top-to-bottom and act on each item.

Inputs

1. Target (required) — where to write the analysis. One of:

   • A full path (absolute or repo-relative) to a .md file. Used verbatim. Example: scratch/0423-02/analysis.md, C:\path\to\repo\scratch\0501-04\analysis.md.
   • A slot number (one or more digits, e.g. 2, 02, 14). Expands to scratch/<MMDD>-<##>/analysis.md, where:
     • <MMDD> is today's local date (zero-padded month + day).
     • <##> is the slot number, always zero-padded to two digits.
   • When given a number, confirm the resolved path back to the user in your first response.
   • The parent directory is created if missing. The analysis file is overwritten if it already exists, after showing the user a short notice that you're replacing the prior analysis.

2. Scope (optional) — what to review. If the user names a scope, honor it verbatim. Accepted forms:

   • working-tree — staged + unstaged changes vs HEAD.
   • last-commit — HEAD~1..HEAD.
   • since-push — local commits ahead of the upstream branch (@{u}..HEAD if upstream is configured; otherwise fall back to origin/<default-branch>..HEAD).
   • full — the entire repo (use only when explicitly requested; reviews are time-boxed and partitioned in this case).
   • A commit range (<sha>..<sha>), a single SHA, a branch name, or a list of file paths. Used verbatim.
   • plan-slot — the commits produced by the sibling plan.md in the same slot directory (see Scope Resolution below).

3. Optional focus — free-form text. Examples: "focus on the ingestion path", "I'm worried about the new transaction handling", "skip the test files". Use this to weight the review, not to limit it; still surface anything load-bearing you find outside the focus.

4. max_subagents (optional, default 3) — maximum number of sub-agents to run in parallel at any given time. 1 disables parallel fan-out entirely (the Engineering and QA passes still happen, but sequentially in-process or one-at-a-time). Hard upper bound: 8. The cap is a concurrency ceiling, not a total ceiling — you may launch more than max_subagents sub-agents over the life of the task (e.g., when partitioning a large full scope) as long as no more than max_subagents are running at the same time.

Scope Resolution (when Scope is not supplied)

This is the order of operations:

1. Detect a sibling plan.md. If the resolved analysis path is scratch/<MMDD>-<##>/analysis.md and a plan.md exists in the same directory, attempt plan-slot scope:

   • Read plan.md's ## Progress Log and collect the SHA from every COMMIT entry. Ignore PENDING and NOTE entries — a PENDING entry is unfinished work, not a reviewable commit.
   • The review scope is exactly that set of commits. Do not collapse it into an oldest-parent..newest range unless you have verified the commits are contiguous (each one's parent is the previous), because an unverified range silently pulls in unrelated intervening commits. Otherwise inspect each SHA individually with git show <sha> and union the results.
   • Echo the resolved commit list and file set to the user.
   • If the plan exists but no COMMIT entries are recorded yet (e.g. the plan is Draft or Ready-to-execute), fall through to step 2.

2. No plan, or plan with no commits: stop and ask the user to choose. Offer these options exactly:

   • full — review all code in the repo.
   • since-push — local commits not yet on the upstream branch.
   • last-commit — just HEAD.
   • working-tree — uncommitted changes only.

   Do not guess. Wait for the user's choice before starting either review pass.

Always echo the final resolved scope (a concrete set of files and commit SHAs, not just a label) to the user before fanning out the review passes. This is the contract that lets the user catch a mis-scoping before any expensive work happens.