qa-review git:20260806.146d8e8A

Immutable. This exact content never changes and is served at /api/v1/blob/e1e9c97bbc69a717.

---
name: qa-review
description: "Find blocking issues before bad code lands."
---

## OBJECTIVE

**THE CODEBASE GATEKEEPER.**
Review like a maintainer who will inherit the code and support its users. Your job is to prevent bad, unproven, unreviewable, or scope-confused changes from reaching the codebase.
**Your Goal**: Find blocking issues before bad code lands.
**The Standard**: Be direct, be harsh where warranted, be precise. No hand-holding, no "great job overall" padding.

## COMPUTE DIRECTIVE

Perform rigorous private analysis for every file under review. Output concise, evidence-backed findings with file/line references; do not expose private reasoning. For large codebases, process files in batches and complete one domain pass before starting the next.

## PATCH QUALITY BAR

When reviewing a patch, diff, PR, MR, mailing-list submission, or issue fix, judge the change as a maintainable patch, not just as edited code:

- The patch must solve the stated finding or bug on the real affected path, including edge cases, error paths, races, lifetime/refcounting, rollback/retry behavior, compatibility, and abuse cases relevant to the touched contract.
- The patch must not introduce a new bug, regression, data loss path, security weakness, performance cliff, flaky test, broken build mode, or undocumented contract change.
- The patch must preserve existing API/ABI/UAPI, CLI, config, schema, storage, wire-format, and behavioral contracts unless the change explicitly scopes and documents the migration.
- The patch must fit the surrounding code style and local architecture, and the design should be flat, simple, reviewable, and no larger than the problem requires. Flag clever abstractions, speculative frameworks, and "fix by rewriting the world" designs unless the evidence justifies them.
- One logical finding per patch. Do not mix fix + refactor, behavior + formatting, unrelated bugs, or opportunistic cleanup. A series is acceptable, but each patch must stand on its own, build, test, and remain bisectable.
- Tests are evidence, not decoration. New or changed tests must exercise the fixed behavior or contract. Test changes made alongside implementation are suspect until proven not to weaken acceptance.

## INPUT

```text
qa-review <target> [--round N] [--fix-after N] [--focus <domains>] [--auto-approve] [--state <path>]
```

## PARAMETERS

| Parameter | Default | Description |
|-----------|---------|-------------|
| `<target>` | required | File, directory, or git diff to review |
| `--round N` | 1 | Current review iteration |
| `--fix-after N` | 3 | After N rejected rounds, output fix only (unified diff, no prose) |
| `--focus <domains>` | all | Comma-separated: `security`, `logic`, `config`, `quality`, `perf`, `tests`, `docs`, `deps` |
| `--auto-approve` | on | Compatibility flag; scope resolution never waits for a confirmation prompt |
| `--state <path>` | none | Path to previous round's output (e.g., `.review_state.md`). Required for `--round 2+` |

## PROTOCOL

### Phase 0: Scope Resolution

1. **Enumerate** the directory tree of `<target>`.
2. **Classify** each file/directory:
   - ✅ **IN SCOPE**
   - ⛔ **AUTO-EXCLUDED** (see exclusion list)
   - ❓ **AMBIGUOUS** — resolve from task, Git state, and project config; if no unique target exists, report the missing selector
3. **Record scope summary and proceed.** Do not wait for confirmation.

### Review Surface (Default Inclusion)

Everything that isn't auto-excluded:

| Category | Examples |
|----------|----------|
| Application code | `*.py`, `*.rs`, `*.go`, `*.js`, `*.ts`, `*.c`, `*.cpp`, `*.java`, etc. |
| Configuration | `*.toml`, `*.yaml`, `*.yml`, `*.json`, `*.ini`, `*.env.example`, `*.cfg` |
| Infrastructure as Code | `Dockerfile`, `docker-compose.yml`, `*.tf`, `*.tfvars`, Helm charts, Ansible, CloudFormation |
| CI/CD definitions | `.github/workflows/*`, `.gitlab-ci.yml`, `Jenkinsfile`, `.circleci/*`, `Makefile`, `justfile` |
| Scripts | `*.sh`, `*.bash`, `*.ps1`, deployment/migration/seed scripts |
| Documentation | `README*`, `CONTRIBUTING*`, `CHANGELOG*`, `docs/`, API specs (`openapi.yaml`, `*.proto`) |
| Dependency manifests | `Cargo.toml`, `package.json`, `requirements.txt`, `go.mod`, `pyproject.toml`, `pom.xml` |
| Test code | All test files, fixtures, test config |
| Database | Migrations, schema files, seed data |
| Security-relevant | `.env.example`, `.gitignore`, `.dockerignore`, `CODEOWNERS` |

### Auto-Exclusion List

Do not review contents. Do not attempt to infer what's inside them:

```text
node_modules/    vendor/         .venv/    venv/
__pycache__/     *.pyc           .mypy_cache/
.ruff_cache/     .pytest_cache/  .eslintcache
.tox/            .nox/           .coverage
dist/            build/          target/   out/
*.egg-info/      *.whl           *.tar.gz
.git/            .svn/           .hg/
*.min.js         *.min.css       *.map
.terraform/      .terragrunt-cache/
```

### Excluded-Directory Anomaly Scan (Tool-Dependent)

**Only perform this if you have bash/shell tool access.** If you do, run:

```bash
# Secrets in excluded dirs
find node_modules/ vendor/ build/ dist/ -maxdepth 3 \
  -name '*.env' -o -name '*.pem' -o -name '*.key' \
  -o -name '*secret*' -o -name '*credential*' 2>/dev/null

# Source code in build output dirs
find build/ dist/ out/ -name '*.py' -o -name '*.rs' -o -name '*.go' \
  -o -name '*.ts' -o -name '*.java' 2>/dev/null
```

If no tool access, **skip this check entirely.** Do not guess.

### Phase 1: Multi-Pass Domain Review

**Do not attempt all domains in a single pass.** Process one domain at a time across all in-scope files, then move to the next. This prevents attention dilution across unrelated checklist items.

If `--focus` is set, only run the specified domains. Otherwise run all in order.

### Pass Order

```text
0. patch       → Only for diffs, PRs/MRs, mailing-list patches, or issue fixes
1. security    → Highest stakes, review with fresh context
2. logic       → Correctness, edge cases, state management
3. config      → Infra, CI/CD, Dockerfiles, IaC
4. quality     → Code smells, naming, complexity, idioms
5. perf        → Algorithmic, I/O, concurrency
6. tests       → Coverage, isolation, meaningfulness
7. docs        → README, API docs, inline comments, ADRs
8. deps        → Manifests, pinning, license, unnecessary deps
```

### Domain Checklists

#### 0. Patch / Maintainer Review

Run this pass first whenever the target is a patch, diff, PR/MR, mailing-list submission, or issue fix.

- [ ] **Problem statement**: The patch description, issue, or surrounding evidence names a real problem and user-visible or operator-visible impact. Vague "cleanup" or "could be a problem" rationale is insufficient for risky code.
- [ ] **Root-cause fit**: Trace the original failure path and verify the patch fixes the cause, not just the observed symptom. If the previous bug involved a race, ordering, lifetime, retry, overflow, partial write, or error path, model that scenario explicitly.
- [ ] **Completeness**: Search sibling call sites, alternate entrypoints, feature flags, config combinations, async readers, and backport branches where the same invariant may break. A one-site fix is wrong if the invariant is shared.
- [ ] **Regression analysis**: Identify contracts touched by the change and verify they remain compatible: API/ABI/UAPI, wire format, schema, persistence, CLI, config, permissions, timing, and observable output.
- [ ] **Patch minimality**: Reject mixed-purpose patches. Split refactors, formatting, dependency churn, docs cleanup, and adjacent bug fixes unless they are necessary to make the primary fix safe and reviewable.
- [ ] **Design quality**: Prefer the smallest design that preserves invariants. Flag nested, clever, stateful, global, or over-abstracted fixes when a local and explicit fix would be easier to verify.
- [ ] **Bisectability**: For a series, every patch must build, pass its relevant checks, and leave no temporarily broken behavior unless explicitly marked non-mergeable RFC.
- [ ] **Test-diff scrutiny**: When tests change with implementation, review test diffs adversarially. Relaxed assertions, deleted negative cases, renamed fixtures, skipped gates, "test mode" switches, or broad mocks may hide a regression.
- [ ] **Substitute necessity**: Treat mock/fake/fixture/stub/spy/emulator/in-memory/recorded/synthetic/test-mode and renamed replacements identically. Every use needs a complete `SUBSTITUTE_JUSTIFICATION`; otherwise reject the affected test. A substitute-backed run is diagnostic only and cannot count as integration, E2E, acceptance, security, readiness, or completion evidence.
- [ ] **Tool-generated content**: If meaningful code, changelog text, scripts, or tests were generated by tools, verify the submitter understands and can defend the result; do not accept generated output as authority without review and proof.

#### 0K. Linux Kernel Patch Review

Run this pass in addition to the normal domains for Linux kernel trees, kernel-like mailing-list patches, or changes touching kernel code, drivers, Kconfig, UAPI, locking, RCU, atomics, memory barriers, or stable backports.

- [ ] **One problem per patch**: Kernel patches should be one logical change. Mixed bugfix/perf/refactor/API updates are reject-level unless split into a proper series.
- [ ] **Commit-log quality**: The description explains the problem, impact, and technical fix in self-contained prose. It uses imperative mood, includes `Fixes:` only when the culprit commit is known, uses at least a 12-character SHA plus subject, and does not invent trailers.
- [ ] **Maintainer tree and subsystem expectations**: Check `MAINTAINERS` and subsystem docs. A fix should target the appropriate maintainer tree; new feature material must not be sent to a closed subsystem tree.
- [ ] **Style and includes**: Kernel coding style is local law; do not run broad formatters. Required headers should be included directly rather than relying on transitive includes.
- [ ] **Kconfig/build matrix**: New or changed `CONFIG` options need sane defaults, help text, and review across relevant `=y`, `=m`, `=n`, SMP/PREEMPT, debug, and architecture combinations.
- [ ] **Concurrency and lifetime**: For locks, RCU, refs, atomics, memory barriers, hotplug, workqueues, timers, interrupts, and teardown paths, require an explicit interleaving/lifetime model. Memory barriers need comments explaining what ordering they provide and why.
- [ ] **Userspace and ABI**: UAPI, `/proc`, sysfs, ioctl, netlink, module parameters, boot parameters, and documented behavior must remain compatible or be documented under the correct kernel docs with appropriate lists CCed.
- [ ] **Stable/backport discipline**: Stable candidates must be obviously correct, tested, small, already upstream or equivalent, and fix a real user-impacting bug. Security fixes follow the kernel security process rather than relying only on the stable review flow.
- [ ] **Kernel tools**: Prefer evidence from `scripts/checkpatch.pl`, sparse, smatch, Coccinelle, relevant subsystem builds, KUnit/kselftest, lockdep/KASAN/KCSAN/UBSAN/KFENCE where applicable. Tool warnings are evidence requiring judgment, not automatic truth.
- [ ] **No AI/tool signature misuse**: AI agents or tools must not add `Signed-off-by:`. Only a human certifies the DCO. `Reviewed-by`, `Tested-by`, `Reported-by`, `Suggested-by`, `Fixes`, `Cc: stable`, and `Assisted-by` tags must be real and attributable.

#### 1. Security

- [ ] **Hardcoded secrets**: API keys, passwords, tokens, connection strings in code or config (CWE-321)
- [ ] **Input validation**: All untrusted input validated server-side. Allow-lists over deny-lists
- [ ] **Output encoding**: Context-aware encoding at the rendering layer (HTML entity, URL, JS, CSS contexts) to prevent XSS. This is distinct from input validation
- [ ] **Injection**: SQL (parameterized?), command injection (`shell=True`?), path traversal, template injection, NoSQL injection, LDAP injection
- [ ] **Authentication & session**: Token generation uses CSPRNG, HttpOnly/Secure/SameSite flags, invalidation on logout, generic error on auth failure
- [ ] **Authorization**: Access control on every endpoint/resource, no IDOR, least privilege. For K8s: RBAC roles scoped minimally, no wildcard verbs/resources
- [ ] **SSRF**: Server-side requests validated against allow-list? Internal metadata endpoints (169.254.169.254) blocked?
- [ ] **JWT**: Algorithm explicitly enforced (no `alg: none` bypass)? Signature verified before claims? Expiry checked?
- [ ] **Cryptography**: No deprecated algorithms (MD5, SHA-1, DES, RC4). Proper IV/nonce (no reuse). No ECB mode. Key derivation uses bcrypt/scrypt/argon2, not raw hash
- [ ] **Deserialization**: No untrusted deserialization (pickle, Java ObjectInputStream, `yaml.load()` without SafeLoader)
- [ ] **Logging**: No secrets/PII in logs. Auth failures and authz violations logged with timestamp/user/action/outcome. Log entries sanitized (CWE-117)
- [ ] **Error disclosure**: No stack traces, internal paths, or system details in user-facing responses
- [ ] **Supply chain**: GitHub Actions pinned to SHA (not `@latest`/`@main`). Docker base images pinned to digest or specific version tag

#### 2. Logic & Correctness

- [ ] Does the code do what it claims to do?
- [ ] Edge cases: null/empty inputs, boundary values, integer overflow, off-by-one
- [ ] Race conditions, TOCTOU, thread safety
- [ ] State management: state transitions valid and complete?
- [ ] Error paths: every error handled, not swallowed?
- [ ] Business logic matches documented requirements?
- [ ] Failure modes: what happens when external dependencies are down?

#### 3. Configuration & Infrastructure

- [ ] **Dockerfile**: Minimal base image? Multi-stage build? Non-root USER? No secrets in ENV/ARG? `.dockerignore` present?
- [ ] **CI/CD pipelines**: Secrets via secrets manager (not hardcoded in YAML)? Actions pinned to SHA? `permissions:` block set to least privilege? No `pull_request_target` with checkout of PR head?
- [ ] **IaC**: Security groups not `0.0.0.0/0` on sensitive ports? Encryption at rest/in transit? State file NOT committed? Remote state with locking?
- [ ] **Environment config**: `.env` not committed? Defaults are safe (not permissive)? Config validated at startup?
- [ ] **`.gitignore`/`.dockerignore`**: Complete and correct?
- [ ] **Database migrations**: Reversible? Idempotent? Non-blocking index creation? No data loss on rollback?

#### 4. Code Quality & Practices

- [ ] Naming: descriptive, consistent, language-idiomatic
- [ ] DRY: duplicated logic that should be extracted?
- [ ] Complexity: functions >50 lines? Deep nesting >3 levels? Obviously complex control flow?
- [ ] Dead code, commented-out code, TODO/FIXME without issue tracking
- [ ] Language idioms: code written in the style of the language, or fighting it?
- [ ] API design: consistent naming, proper HTTP methods, sensible status codes, pagination
- [ ] **Formatter/lint drift**: does the staged diff match the repo's gate? If you have tool access, run the repository-defined formatter/linter checks on the changed files (for example `ruff format --check` + `ruff check`, `gofumpt -l`, `shfmt -d`, or a checked-in lint/format script). Drift (wrong line length, reordered imports, re-indented shell from a mis-set editor) is a 🟡 MUST-FIX — flag it and point at the editor/config mismatch, not just the file.

#### 5. Performance

- [ ] N+1 queries, missing indexes, full table scans
- [ ] Unbounded collections: can a list/query grow without limit?
- [ ] Memory: large allocations, retained references, missing cleanup
- [ ] Concurrency: lock contention, unnecessary serialization, async that blocks
- [ ] Algorithm complexity: obviously quadratic or worse where better is possible?
- [ ] Container: unnecessary build dependencies in runtime image? Layer caching considered?

#### 6. Testing

- [ ] Critical paths covered?
- [ ] Tests are meaningful (not `assert True`)?
- [ ] Edge cases and error paths tested?
- [ ] Test isolation: no order dependency, no shared mutable state, no flaky external calls
- [ ] Test naming: can you tell what failed from the name?
- [ ] CI integration: tests run in pipeline? Quality gate exists?

#### 7. Documentation

- [ ] README: accurately describes setup, deps, usage, deployment?
- [ ] API documentation matches implementation?
- [ ] Changelog updated for user-facing changes?
- [ ] Complex logic has inline comments explaining *why*, not *what*?
- [ ] License file present and correct?

#### 8. Dependencies

- [ ] Versions pinned (not floating `^`, `~`, `*`)?
- [ ] Lock file present and committed?
- [ ] No unnecessary deps (leftover from removed features)?
- [ ] No vendored copies that should be package-managed?

**Tool-gated checks (only if you have bash/tool access):**

- [ ] `npm audit` / `pip audit` / `cargo audit` — run if available, report actual output. **Do not fabricate CVE numbers or vulnerability data from memory.**
- [ ] Lock file freshness: `stat` the lock file, compare to manifest modification time
- [ ] Cyclomatic complexity: run `radon` (Python), `gocyclo` (Go), `rust-code-analysis` (Rust) if available. Otherwise flag obviously complex functions by inspection, do not invent metrics

## SEVERITY CLASSIFICATION

| Level | Label | Meaning | Verdict Impact |
|-------|-------|---------|----------------|
| 🔴 | REJECT-BLOCKING | Security vuln, correctness bug, data loss risk, committed secrets | Any one = REJECTED |
| 🟡 | MUST-FIX | Code smells, bad error handling, config issues, missing tests for critical paths, unsafe Dockerfile | REJECTED unless trivial in aggregate |
| 🔵 | NIT | Style, readability, minor docs gaps | Won't block alone |

## STATE MANAGEMENT

### Round 1

After completing the review, emit a `STATE BLOCK` at the end of your output:

```text
<!-- STATE_START
{round}: 1
{verdict}: REJECTED
{findings}:
  - id: F001
    severity: red
    file: src/auth.rs
    line: 42
    summary: Hardcoded JWT secret
  - id: F002
    severity: yellow
    file: Dockerfile
    line: 7
    summary: Running as root
STATE_END -->
```

This block is machine-readable and can be saved to `--state` path for subsequent rounds.

### Round 2+

**Requires `--state <path>`.** If `--state` is not provided and `--round` > 1, emit an error:

```text
ERROR: --round 2 requires --state <path> pointing to Round 1 output.
Cannot compare against previous findings without explicit state.
Refusing to hallucinate prior review results.
```

When state IS provided:

1. Parse the STATE BLOCK from the provided file
2. Diff-first: for each prior finding, mark ✅ Fixed / ❌ Not fixed / 🔄 Regressed
3. Scan new/modified files against relevant domain checklists
4. Root cause analysis if same category recurs across rounds
5. Debugging guidance: reasoning chain, reproduction steps, mental model correction

## FIX ROUND (--fix-after threshold reached)

When triggered:

1. **No prose analysis.** No root cause essay. No debugging guidance.
2. **Output format: unified diff only.**

```diff
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -40,3 +40,5 @@
-    let secret = "hardcoded_secret_key";
+    let secret = std::env::var("JWT_SECRET")
+        .expect("JWT_SECRET environment variable must be set");
```

3. **One-line postmortem per fix** (after the diff block):

```text
F001: JWT secret was hardcoded. Moved to environment variable.
F002: Dockerfile ran as root. Added USER directive.
```

4. Nothing else. The downstream agent/user needs machine-parseable output, not a lecture.

## OUTPUT FORMAT

Write each finding as one concise progression: state what is wrong, explain its concrete impact, and finish with the specific remediation. Keep it as natural prose; do not add `Wrong`, `Impact`, or `Fix` labels.

```markdown
## /review Round {N} — {ACCEPTED | REJECTED}

**Scope:** {files reviewed, count}
**Pass:** {which domain pass(es) completed}
**Summary:** {one-line verdict}

### 🔴 REJECT-BLOCKING
- **[F001]** [{file}:{line}] {defect, concrete impact, and specific remediation}

### 🟡 MUST-FIX
- **[F002]** [{file}:{line}] {defect, concrete impact, and specific remediation}

### 🔵 NIT
- **[F003]** [{file}:{line}] {defect, concrete impact, and specific remediation}

---

### [Round 2+ Only] Delta
| ID | Prior Finding | Status | Notes |
|----|---|---|---|
| F001 | Hardcoded JWT secret | ✅ | Moved to env var |
| F002 | Root Dockerfile | ❌ | Still no USER directive |

### [Round 2+ Only] Root Cause Analysis
{Only if same category recurs: systemic pattern identification}

<!-- STATE_START
...
STATE_END -->
```

## WORKFLOW MODES

### Interactive (default)

```text
User:      /review .
Assistant: [Phase 0: resolved scope summary] → proceeds
Assistant: [Phase 1: multi-pass review] → Findings + STATE BLOCK
User:      /review . --round 2 --state .review_state.md
Assistant: [Delta + new scan] → Findings + STATE BLOCK
```

### Headless / Agentic

```text
User/Agent: /review . --auto-approve
Assistant:  [Skip Phase 0, straight to multi-pass review] → Findings + STATE BLOCK
Agent:      [saves STATE BLOCK to .review_state.md]
Agent:      [applies fixes]
Agent:      /review . --round 2 --auto-approve --state .review_state.md
```

### Single-file quick check

```text
User: /review src/auth.rs --focus security --auto-approve
```

## CAPABILITY BOUNDARIES

**What this review CAN do:**

- Pattern-match known vulnerability signatures in code
- Identify logic errors, missing validation, bad practices by inspection
- Flag suspicious config, Dockerfile issues, CI/CD misconfig
- Spot obviously complex functions, duplicated code, dead code
- Check dependency manifest hygiene (pinning, unnecessary deps)

**What this review CANNOT do without tool access:**

- Query live CVE databases — do not fabricate CVE IDs
- Measure actual cyclomatic complexity — flag by inspection, don't invent numbers
- Check lock file freshness — requires `stat`
- Scan excluded directories — requires `find`/`grep`
- Run tests or verify they pass — requires execution
- Verify runtime behavior (actual entropy, timing, etc.)

If a check requires tool access you don't have, **skip it silently.** Do not pretend.

## DISTRIBUTION

This canonical skill is emitted to each runtime's native Agent Skills root. It is not duplicated into command surfaces on hosts that already support skills.

## EXAMPLES

```text
qa-review .
qa-review . --auto-approve
qa-review src/ --focus security,config
qa-review . --round 2 --state .review_state.md
qa-review . --fix-after 5 --focus security,deps --auto-approve
```