version: "1.0.0" name: dev-review description: "Performs a two-track code-quality and QA review in the roles of a staff-level Engineering Lead and QA Lead, then synthesizes both critiques into a single analysis.md. USE FOR: pre-PR self-review, post-dev-do quality gates, ad-hoc deep reviews of a change set. Accepts either a full path to the analysis file or a short slot number that expands to scratch/[MMDD]-[##]/analysis.md. Optional max_subagents (default 3) caps parallel sub-agent fan-out. Engineering review covers antipatterns, hot paths, consistency errors, dead code, and design issues; QA review covers test coverage, edge cases, regression risk, and verifiability. Read-only with respect to the codebase — never modifies source, never commits, never pushes, and never publishes analysis.md to GitHub. Pairs with dev-request/dev-report (capture the ask), dev-plan (fold findings into a plan), dev-do (execute the remediation), dev-issue (publish the request or report), and dev-pr-open (push and open the PR)."

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.

1. 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).

1. 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.

1. `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.

1. 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.

Workflow

1. Resolve the analysis path. Echo it.
2. Resolve the review scope as described above. Echo the concrete

file list and (where applicable) commit list. If analysis.md already exists, note that you'll overwrite it.

1. Pre-flight.

• Confirm the working tree state with git status so you know

whether working-tree scope would actually contain anything.

• Read AGENTS.md at the repository root for the canonical build

and test commands, code style, and architectural invariants. If it is absent, fall back to README.md / CONTRIBUTING.md and note in the report which source you used. You will not run these commands, but you will reference them in the QA review, so they must be real. Never invent one.

• Identify the affected project(s) so the commands you cite are

correctly scoped.

1. Run the two review passes. Prefer running them in parallel as

sub-agents (one general-purpose or code-review agent per role) so they can't anchor on each other. Each sub-agent:

• Receives the same resolved scope and focus text.
• Receives an explicit role brief (Engineering Lead or QA Lead,

with the bullet list from "Roles" above).

• Returns a structured list of findings with file paths, line

ranges, severity, and a recommendation per finding.

• Is read-only — explicitly forbidden from editing source or

running mutating commands.

1. Synthesize. Put on the synthesizer hat. Merge duplicates,

re-rank by severity, drop noise, write the final report using the format below.

1. Sanity-check a report containing any Blocker, High, or

architecture-level recommendation with a registered review specialist when available. Otherwise use a fresh general-purpose sub-agent explicitly prompted to act as an adversarial/rubber-duck reviewer. Adopt critique findings that prevent miscommunication; set aside findings that bloat the report. Briefly note in your reply what (if anything) changed.

1. Write `analysis.md`. Overwrite if present.
2. Report back with: the resolved analysis path, the resolved

scope, finding counts by severity, and the top 3 findings (one line each).

Report Format

# Code & QA Review: {short title — what was reviewed}
 
| | |
|-|-|
| Slot | `scratch/<MMDD>-<##>/` (or full path) |
| Issue | [#N](<url>) — or `not published` |
| Scope | {label + concrete description, e.g., `plan-slot` (3 commits, 14 files)} |
| Status | Draft / Ready-for-team |
| Created | {YYYY-MM-DD} |
| Reviewers | Engineering Lead + QA Lead (synthesized) |
 
## TL;DR
 
{3–5 sentences. What was reviewed, the overall health verdict
(Ship / Ship-with-fixes / Do-not-ship), and the single most important
thing the team should do next.}
 
## Scope
 
-**Commits:** {list of SHAs + subjects, oldest → newest, or "n/a"}
-**Files:** {bulleted list of files reviewed, grouped by project}
-**Excluded:** {anything intentionally not reviewed, with reason}
-**Focus:** {echo of the user's focus text, or "general review"}
 
## Findings
 
Findings are **synthesized** from both reviews and ranked by severity.
Each finding is independently actionable.
 
### Blocker
 
#### B1. {Short title}
 
-**Where:** `<path/to/source-file>:120-138` (or symbol name)
-**Source:** Engineering / QA / Both
-**What:** {1–3 sentences. The problem, in observable terms.}
-**Why it matters:** {1–2 sentences. Concrete risk if shipped as-is.}
-**Recommendation:** {Concrete next step. "Add test for X.",
  "Hoist allocation out of the loop.", "Open follow-up issue and
  document the limitation in `ABC.md`."}
 
### High
 
#### H1. {…}
 
{Same shape.}
 
### Medium
 
#### M1. {…}
 
### Low
 
#### L1. {…}
 
### Nit
 
#### N1. {…} (optional — drop the entire Nit section if empty)
 
## Test Coverage Summary
 
-**Covered well:** {areas of the change with strong test coverage}
-**Thin coverage:** {areas with weak coverage; what's missing}
-**Suggested new tests:** {bullet list, each naming the test name,
  the project it belongs in, and the behavior it pins down}
 
## Verification Steps the Team Should Run
 
-{Specific commands, taken verbatim from `AGENTS.md`. Prefer the
  scoped command for the affected project, or the focused filter for a
  single test class/method.}
-{Any sanctioned verification that could **not** be cited as runnable
  without setup `AGENTS.md` documents as a prerequisite, and why.}
-{Manual steps if applicable}
 
## Out of Scope / Deferred
 
-{Things the reviewers noticed but consciously did not chase, with
  why. Useful follow-ups go here.}
 
## Next Steps
 
How these findings re-enter the loop:
 
-**Blocker / High** — when this review has a sibling slot containing a
  `plan.md` (and its source request), re-invoke `dev-plan` on that slot
  with this analysis as input; it folds them in as new remediation
  phases and `dev-do` executes them. For an ad-hoc review with no such
  slot, say so and recommend the user open one with
  `dev-request` / `dev-report` first. Do not hand-patch them outside
  the loop.
-**Medium** — fix now if the change is still in flight, otherwise
  record as a follow-up.
-**Low / Nit** — record and move on. Do not block on these.
-**Never to GitHub.** This analysis is an internal artifact and is
  never published as an issue, a comment, or a quotation. Findings
  re-enter the loop as a new `dev-request` / `dev-report`, which get
  their own issue.
-**After a clean analysis**, `dev-pr-open` is the recommended next
  step — a recommendation, not a gate.
-{Name the concrete next action here, e.g., "Run `dev-plan` on
  `scratch/0423-02/` to add remediation phases for B1 and H2."}
 
## Notes
 
{Free-form. Links to related plans, prior reviews, design docs.}

Sub-Agent Use

• The two role passes (Engineering Lead, QA Lead) should run in

parallel sub-agents. They must not see each other's findings until the synthesizer step. This is the whole point of doing two passes — if they collapse into one, you get one set of findings with the illusion of two reviewers.

• Both sub-agents must be told explicitly that they are read-only:

no edits, no commits, no mutating commands. They may run git diff, git log, git show, view, grep, glob, lsp, and similar read-only inspections.

• The synthesizer step is always done in-process, not delegated.

You own the final ranking and recommendations.

• For very large scopes (full or a multi-hundred-file diff), you

may partition the file set across multiple Engineering or QA sub-agents. If you do, give each sub-agent a non-overlapping slice and aggregate before synthesizing.

• Honor `max_subagents`. Never run more than max_subagents

sub-agents concurrently. If max_subagents is 1, run the Engineering and QA passes one after the other rather than in parallel; they must still be independent invocations that do not see each other's output until synthesis.

Iteration Mode

analysis.md is a snapshot, not a living document. When invoked against a slot whose analysis.md already exists:

• Treat it as a re-review. Read the prior analysis for context

(especially "Out of Scope / Deferred"), then re-run the two passes against the current scope.

• Overwrite analysis.md with the fresh report. Mention in your

reply that you replaced it and call out any findings that have been closed since the prior analysis (with one-line evidence, e.g., "B1 from prior analysis is now resolved by commit abc1234").

• Do not edit plan.md, featurerequest.md, or bugreport.md in

the same slot — those are owned by their respective skills.

Important Rules

• Read-only. This skill never modifies source, never stages,

never commits, never pushes. The only file it writes is analysis.md (and the parent directory if missing).

• `analysis.md` is never published to GitHub. Not as an issue, not

as a comment, not as a quotation in a PR body. It is an internal artifact. Findings re-enter the loop as a new dev-request / dev-report, which get their own issue via dev-issue.

• Populate the `Issue` row, never invent it. Read it from the

sibling plan.md, or from the source artifact when no plan exists, under the same no-downgrade ratchet the other skills use: never replace an existing #N with not published. Report a disagreement rather than resolving it — that belongs to dev-issue under its § The Issue Binding. This skill never calls a writing gh command.

• Two independent passes, then synthesize. Do not skip a pass

because "the other one will catch it". Do not let one pass see the other's draft before synthesis.

• Today's date governs slot expansion. Never reuse a previous

day's <MMDD> for a numeric slot. For an earlier slot, the user must give a full path.

• Cite every finding. File path + line range or symbol. No

"somewhere in the auth module". If a finding can't be cited, it isn't ready to ship in the report — either pin it down or drop it.

• Drop noise. Anything a formatter, linter, or trivial rename

would catch does not deserve a finding number. Mention it once in a single Nit line at most, or omit it entirely.

• Honor repo conventions. Use AGENTS.md at the repository root as

the baseline for "consistency" findings, falling back to README.md / CONTRIBUTING.md if it is absent. Verify any applicable stored memory against the repository before using it. A change that violates a documented convention or architectural invariant is at least a Medium finding unless explicitly justified. A change that merely differs from your personal preference is not a finding at all — do not import conventions from other repositories.

• Severity is the synthesizer's call. Do not pass through the

reviewers' severities verbatim if you disagree. The team reads your synthesized ranking.

• Stay in scope. If you spot a serious issue outside the

reviewed scope, record it under "Out of Scope / Deferred" with a one-line description — do not promote it into the main findings list.

• Concurrency cap is a hard ceiling. Do not spin up more than

max_subagents sub-agents in parallel.

• Do not commit. Files under scratch/ are gitignored on

purpose.