code-review

Files (1)

• SKILL.md

SKILL.md (first lines)

# Code Review — Agent Output Gate

Review agent-generated code before merging. Focus on four areas specific to agentic output.

## Process

### 1. Read context

- The issue or PRD this branch addresses
- `CONTEXT.md` — domain vocabulary and conventions
- Recent ADRs in `docs/adr/`
- The diff (`git diff main...HEAD` or `git log main..HEAD --oneline`)

### 2. Review against checklist

**Spec compliance**
- Does the code solve the stated problem? (Not a different problem, not a subset.)
- Are there edge cases from the spec that are unhandled?
- Are there features NOT in the spec that were added? (Speculative feature = flag.)

**Test quality**
- Do tests verify behavior through public interfaces? (Not implementation details.)
- Would tests survive a refactor that doesn't change behavior?
- Are there tests for critical paths, or only happy paths?
- Do the tests actually fail when the behavior is broken? (Try the deletion test on the implementation.)

**Architectural alignment**
- Are new types/functions named consistently with `CONTEXT.md`?
- Do architectural patterns match existing code? (Or is there a novel pattern without a justifying ADR?)
- Are there new dependencies that should be flagged?

**Agent anti-patterns**
- Dead code: stubs, unused exports, commented-out code, variables declared+mutated but never read
- Over-engineering: abstractions for hypothetical future needs
- Hallucinated APIs: functions/types that don't exist in the project
- Inconsistent style: formatting, naming, import conventions that drift from the rest of the codebase
- **XSS via escapeHtml fail**: `escapeHtml` defined with `div.innerHTML = s` (decodes, not escapes) instead of `div.textContent = s`. Or escapeHtml defined but never called on untrusted data injected into innerHTML. Flag BOTH: wrong implementation + missing call sites. The fix uses textContent for true escaping, and separates trusted HTML (authored by us) from untrusted data.

### 3. Report

Classify each finding:

| Severity | Meaning | Action |
|----------|---------|--------|
| 🔴 **Critical** | Breaks spec, test doesn't verify behavior, or hallucinated API | Must fix before merge |
| 🟡 **Warning** | Over-engineering, naming drift, missing edge case | Should fix, discuss if unsure |
| ⚪ **Nit** | Style preference, minor cleanup | Optional, up to author |

### 4. Outcome

- **No criticals** → "Approved. Address warnings at your discretion."
- **Criticals found** → "Blocked. Fix [N] critical issues before merge."
- **Mixed** → "Fix criticals, discuss warnings."

From atalariq/skills, MIT.

More from atalariq/skills