senior-code-review

name

senior-code-review

description

Panel code review — 18 single-concern reviewer agents (correctness, security, performance, reuse, tests, contracts, style, error-handling, concurrency, data-integrity, observability, dependencies, accessibility, privacy, config-secrets, docs, complexity, scope-creep) fanned out inside a Workflow, every candidate adversarially verified, then deduped into one severity-ranked report. Use for reviewing a diff, branch, or PR when broad or targeted single-concern coverage is wanted. Trigger on 'senior-code-review', 'senior review', 'panel review', 'full review', 'review for security and concurrency', or naming specific reviewer concerns. Do NOT trigger for the built-in /code-review quick pass, for fixing code, or for reviewing prose.

senior-code-review

A conductor over a panel of single-concern reviewers. Each reviewer has a charter file in reviewers/ defining its one concern, what to look for, and what to IGNORE. The panel runs inside the review-panel Workflow: one finder agent per reviewer, one independent skeptic per candidate finding, then cross-reviewer dedup into a capped, severity-ranked report.

The panel

reviewer concern notes correctness logic errors, off-by-ones, unhandled branches, broken invariants, null/empty/error cases always fires security injection, authz/authn, secrets, deserialization, SSRF/path traversal performance N+1, unbounded loops/allocations, missing indexes, hot-path blocking, re-renders reuse reimplementation of existing code repo-wide search, not just the diff tests new-branch coverage, tautological assertions, missing edge cases contracts public API/schema/signature changes, backward-compat, migration safety style naming, structure, convention adherence cheap, autofixable, unverified, low-severity error-handling failure paths, retries, timeouts, swallowed exceptions, rollback always fires concurrency races, deadlocks, shared-state mutation, atomicity, ordering data-integrity migrations, backfills, destructive ops, reversibility, data-loss migration diffs observability logging, metrics, tracing, alertability — debuggable in prod? dependencies new/updated deps, license, transitive risk, pinning, lockfile hygiene manifest diffs accessibility semantics, contrast, keyboard nav, ARIA, touch targets UI diffs only privacy PII handling, retention, consent, sensitive-field logging config-secrets hardcoded secrets, env/config drift, feature flags, safe defaults docs docstrings/README/ADR/changelog match the change cheap, unverified, low-severity complexity cyclomatic complexity, dead code, over-abstraction, coupling scope-creep does the diff match the PR's stated intent always fires

Invocation

Parse the user's args into three things:

1. reviewers — "auto" (default: the workflow's Scope agent selects the panel from the diff), "all" (all 18), or an explicit list of names from the table (security concurrency → ["security","concurrency"]). Fuzzy-match reasonable aliases (perf→performance, a11y→accessibility, dupes/duplication→reuse, api→contracts, secrets→config-secrets, migration→data-integrity, o11y→observability, deps→dependencies, test/testing→tests, intent→scope-creep). A name with no fuzzy match: list the 18 valid names and ask — never guess.
2. effort — quick (4 candidates/reviewer, 10-finding cap), standard (6/20, default), thorough (8/30). Map "quick look"→quick, "thorough/comprehensive/audit"→thorough.
3. target — everything else, verbatim: a PR number, ref range, file paths, or free-form focus instructions.

Single-reviewer shortcut: exactly ONE reviewer named + quick effort → skip the workflow. Dispatch one Agent (dev or general-purpose) with: read the charter at <this skill's base directory>/reviewers/<name>.md (the base directory is announced when this skill loads), scope the diff yourself, review through that lens only, return findings with file/line/severity/failure_scenario. One subagent is not a workflow.

Worked parses:

raw args parsed (reviewers, effort, target) "quick security pass on PR 42" single-reviewer shortcut — (security, quick, PR 42) "thorough review of src/auth for races and perf" workflow — (["concurrency","performance"], thorough, "src/auth") (bare invocation) workflow — (auto, standard, working diff)

Pre-launch echo: before calling Workflow, state the parsed reviewers + effort + target in one line. This is the launch announcement, not a permission gate — proceed unless the parse required guessing.

Otherwise launch the engine:

Workflow({
  name: "review-panel",
  args: {
    reviewers: "auto" | "all" | ["security", ...],
    target: "<verbatim>",
    effort: "standard",
    charterDir: "<this skill's base directory>/reviewers"
  }
})

The workflow is headless and returns once. Always pass charterDir as this skill's base directory + /reviewers — the charters travel with the plugin; the engine's built-in fallback path exists only on the author's machine.

First-run setup: if the Workflow call fails with "review-panel" not found, the engine isn't installed yet (plugins can't bundle workflows natively). Run bash "${CLAUDE_PLUGIN_ROOT}/bin/install.sh" (the plugin root is two directories above this skill's base directory), then retry — the workflow registers without a session restart.

Rendering the report

The workflow returns { panel, skipped, selectionReasons, summary, findings[], refuted[], stats }. Render for the user:

1. Summary line first, then panel composition — which reviewers ran, which were skipped and why (auto mode).
2. Findings grouped by severity (critical → high → medium → low), each as file:line (clickable file:// link) — [reviewer, verdict] summary + failure scenario. Include autofix suggestions where present.
3. Stats footer: candidates → verified → refuted → reported. If capDropped > 0, say so — silent truncation reads as full coverage.
4. Do NOT re-litigate verdicts — the skeptics already ruled. If the user disputes a finding, re-verify that one finding with a fresh agent, not by arguing from memory.
5. Offer --fix-style follow-up: on request, apply autofixable findings and/or dispatch dev agents for confirmed defects. Never auto-apply.

Shape of the rendered report (one finding shown — this is a shape, not a sample dataset):

Review: 2 high, 1 medium across src/auth — auth flow needs attention before merge.
Panel: correctness, security, error-handling ran; accessibility skipped (no UI diff).

## High
- file:///abs/path/src/auth/session.ts:42 — [security, confirmed] Session token logged at
  debug level. Failure: prod log aggregation exposes live tokens to anyone with log access.
  Autofix: redact token field in the log call.

Stats: 14 candidates → 9 verified → 5 refuted → 3 reported (0 dropped by cap)

When it breaks

error cause resolution workflow launch/return fails engine error, budget, crash surface the error verbatim — never fake findings target resolves to empty diff bad ref, already merged, wrong path say so and stop; do not review the whole repo instead reviewer name has no fuzzy match typo, unknown concern list the 18 valid names and ask — never guess charter file unreadable missing/moved reviewers/ file skip that reviewer and report the skip; running it charterless is WRONG

Cost note

all × thorough is the expensive corner: 18 finders + up to ~140 verifiers. Default auto/standard panels typically run 5–9 finders.