review-artifacts · diff
git:20260823.d7ee0ce to git:20260903.690520b
3 added, 1 removed. Audit A to A.
---
name: review-artifacts
description: Review OpenSpec change artifacts before anyone writes code — architecture quality and design completeness, then spec completeness and testability. Catches a wrong idea at the cheapest possible moment. Use after generating proposal, design, tasks and delta specs, and before implementing.
license: MIT
compatibility: Requires the openspec CLI. Node.js >= 18 for the bundled interlock CLI.
argument-hint: "[change-name]"
allowed-tools: Bash(interlock *) Bash(openspec *) Read Glob Grep
metadata:
type: review
outputs:
- .claude/metrics/review-artifacts-<change>-<timestamp>.json
---
Review the change artifacts before implementation. This is the highest-leverage gate in the whole flow: a blocker caught here costs a paragraph, the same blocker caught after ship costs a day.
Resolve the change:
```bash
interlock validate [change-name] --json
```
If it reports ambiguity, pick the change the user named. If they named none and several are active, review the most recently modified and **say which one you picked** — this skill runs inside automated chains where a question would stall the flow.
Read all of `proposal.md`, `specs/**/*.md`, `design.md`, `tasks.md`.
Detect the stack from the artifacts and the dependency manifest, and skip framework-specific checks that do not apply. Do not ask a Go service about accessibility requirements.
---
## Reviewer 1 — Architecture
**Architecture quality**
- Are layer boundaries placed correctly in `design.md` for this stack?
- Does the structure follow the patterns already in this project?
- Are state and ownership decisions appropriate?
- Is the data flow clear — reads, writes, mutations, side effects?
**Design completeness**
- Does `design.md` specify an error-handling strategy?
- When UI is in scope, are accessibility requirements present?
- Is the approach consistent with the project's own architecture docs (`CLAUDE.md`, `AGENTS.md`, `docs/`)?
**Pattern fitness**
- Does the proposed structure introduce complexity it does not need?
- Is there a simpler alternative that reaches the same outcome?
---
## Reviewer 2 — QA
**Spec completeness**
- Does every requirement in `specs/` have concrete Given/When/Then scenarios?
- Are edge cases covered — empty, error, loading, boundary values, permission denied?
- Which scenarios would a real user hit that the spec does not mention?
**Testability**
- Can every scenario become an automated test?
- Are acceptance criteria specific enough to verify? "Should work properly" is not a criterion.
**Task quality**
- Does `tasks.md` pair a test task with every feature task?
- Is the ordering right — do dependencies precede dependents?
- Is each task small enough for one focused session?
- **Same-file staircase.** Do consecutive checkboxes name the same production path — one per TDD beat, comment move or trap? That is one checkbox, not five. Report it as a warning naming the repeated path. It is not a ship halt: `planWaves` folds the resulting 1-task waves back together, so the cost is implementer count and lost parallelism, not checkpoints.
- Does each numbered section hold work that could share a wave? Independent files belong in one section; a new section is only an output-exists boundary.
**Delta spec quality**
- Are ADDED / MODIFIED / REMOVED sections used correctly?
- Do the deltas describe **behavior**, not implementation?
---
## Invariant check
If the change transforms a shared or derived value — normalizing an identity, deriving a cache or dedup key, canonicalizing a field — then `tasks.md` must contain a task for **every consumer** of that value, not just the one the change is about.
A change that updates one call site and leaves its siblings reading the raw form is a **blocker at this gate**, because it is far cheaper to add the tasks now than to discover the surviving bug after review. See `${CLAUDE_PLUGIN_ROOT}/shared/INVARIANT-SWEEP.md`.
---
## Output
```
── ARCHITECTURE ────────────────────────────────
✓ [passed — brief]
⚠ WARNING: [specific issue, with file reference]
✗ BLOCKER: [critical issue]
── QA ──────────────────────────────────────────
✓ [passed — brief]
⚠ WARNING: [specific issue, with file reference]
✗ BLOCKER: [critical issue]
SUMMARY: N blockers, N warnings, N suggestions
Recommendation: [fix blockers | address warnings | proceed to /interlock:ship]
```
Be specific — name files and sections. Do not pad with generic praise. If the artifacts are genuinely good, say so briefly and recommend proceeding.
---
## Emit structured findings
Write `.claude/metrics/review-artifacts-<change>-<YYYYMMDD-HHMMSS>.json` in this shape:
```json
{ "dimension": "artifacts",
"findings": [
{ "severity": "blocker|warning|suggestion",
"file": "path/to/artifact.md", "line": 42,
"title": "short claim",
"description": "what is wrong and why",
"suggestion": "how to fix it" }
] }
```
`severity`, `file`, `title` and `description` are required; `line` and `suggestion` are optional. Map BLOCKER→`blocker`, WARNING→`warning`, SUGGESTION→`suggestion`. Create `.claude/metrics/` if it does not exist. This is the complete emit contract — do not read `lib/findings.mjs` to rediscover it.
Then let the deterministic gate decide, rather than deciding in prose:
```bash
- interlock gate --findings .claude/metrics/review-artifacts-<change>-<ts>.json
+ interlock gate --findings .claude/metrics/review-artifacts-<change>-<ts>.json --metrics <change>
```
The gate blocks if and only if at least one finding is a `blocker`. Its exit status is the verdict: non-zero means blocked. Do not re-derive the verdict in prose.
+
+ `--metrics <change>` is not optional decoration. It writes the four counts this gate produced — raised, dismissed, dropped by the quality band, surviving — to `.claude/metrics/` in the schema `interlock report` recognizes. Drop it and this review path becomes invisible to the report forever: the review-finding indicators read `unobserved` no matter how many gates ran. It is bookkeeping only and cannot change the verdict or the exit status.