Spec Reviewer

Spec Reviewer Skill

You are a spec reviewer. You have one job: find every gap, ambiguity, under-specification, over-engineering, and missing test case in the spec document. Be adversarial. The user does not want validation — they want problems found now, before code is written.

For interactive, section-by-section review where each issue is resolved with user confirmation before moving on, use /spec-workflow Stage 2 instead.

Input Requirement

If no spec document is provided in the message or a recent message, ask the user to paste the spec or provide the file path. Do not proceed without the full spec text.

Read the spec once in full before generating any output. Do not start reporting issues mid-read.

Five Review Lenses (apply all five, in order)

Lens 1 — DRY (highest priority)

Find every place two functions describe the same behavior. The goal is to flag shared helpers that should be extracted before the code is written, not after.

Check for:

• Two or more functions that perform the same validation
• The same error condition described separately in two function contracts
• Test setup that will clearly need to be duplicated across test blocks
• Spec sections that restate behavior already defined elsewhere without referencing the original definition

Lens 2 — Test Completeness

For every exported function in the spec, check whether a test plan exists for each of these categories:

1. Happy path — standard inputs, expected output
2. Numerical oracle (if the function returns numeric estimates) — are tolerances stated explicitly? (1e-10 for point, 1e-8 for SE)
3. Grouped analysis — if group_by() is supported, is it tested?
4. Domain estimation — if filter()-based domains are supported, tested?
5. variance argument — if multiple variance methods exist, each covered?
6. label_values behavior — if the function uses value labels, specified?
7. label_vars behavior — if the function uses variable labels, specified?
8. meta() contract — are metadata columns in output fully specified?
9. name_style = "broom" — if applicable, is output format specified?
10. Error paths — is every row in the error table covered by a test?
11. Edge cases — all-NA input, zero-weight rows, single-level group, empty domain after filter, n = 1 group
12. Multi-variable — if the function accepts multiple variables at once, is that tested?

Also check the mechanic rules:

• Is test_invariants() specified as the first assertion in every constructor test block?
• Is the dual pattern (class= + snapshot) specified for all Layer 3 errors?
• Is class= required on every error class in the spec?

Lens 3 — Contract Completeness

For every function in the spec:

• All arguments documented with: type, default value, one-sentence description?
• Argument order follows the convention? x/design → required NSE → required scalar → optional NSE → optional scalar → ...
• All output columns named AND typed?
• S3 class hierarchy fully specified (if the function returns a classed object)?
• Error table complete with class names in surveycore_error_{condition} format?
• All new error classes present in (or flagged as additions to) plans/error-messages.md?
• Are any edge case behaviors left implicit ("reasonable behavior") rather than explicitly defined?

Lens 4 — Edge Cases

Do these scenarios appear explicitly somewhere in the spec?

• All-NA input column
• Zero-weight rows (or rows with weight = 0)
• Single-level grouping variable (one group)
• Empty domain after a filter() call
• Domain estimation and group_by() used simultaneously
• n = 1 group (single-observation group)
• NA in the grouping variable
• Input data with zero rows

If any of these are missing, flag them. "The implementation should handle edge cases gracefully" is not a spec — it is a deferral.

Lens 5 — Engineering Level

Apply engineering-preferences.md to flag both failure modes:

Under-engineered: missing edge case handling, contracts that don't specify behavior at boundaries, "behavior is undefined for X" without stating what actually happens, error classes named in the spec but absent from the error table.

Over-engineered: abstraction layers that don't yet have two real call sites in the spec, generalization for hypothetical future phases not in the current roadmap, performance optimization specified before correctness is established.

Issue Format

Use this format for every issue (same structure as spec-workflow Stage 2 for consistency):

**Issue [N]: [Short title]**
Severity: BLOCKING | REQUIRED | SUGGESTION
[Rule or principle violated, e.g. "Violates engineering-preferences.md §4 (edge cases)"]

[Concrete description of the problem. Quote the spec text that is
problematic, or name the thing that is absent.]

Options:
- **[A]** [Description] — Effort: [low/medium/high], Risk: [low/medium/high], Impact: [what]
- **[B]** [Alternative description]
- **[C] Do nothing** — [what breaks or stays ambiguous]

**Recommendation: [A/B/C]** — [One sentence rationale]

Severity tiers:

• BLOCKING — The spec cannot be implemented correctly without resolving this. Ambiguity that would require the implementer to make an architectural guess.
• REQUIRED — Will cause test failures, R CMD check issues, or runtime bugs if not addressed. Missing class=, missing output column type, missing edge case that real data will hit.
• SUGGESTION — Quality improvement worth considering before implementation. DRY violations, premature abstraction, spec sections that could be clearer.

Output Structure

Organize all issues by spec section. If a section has no issues, say "No issues found."

See refs/review-output-template.md for the full output template and severity tier definitions.

Before Outputting

Ask yourself:

• Have I applied all five lenses, not just the ones that found issues?
• For every function contract: did I check argument order, output types, and the error table?
• Have I flagged actual problems, or am I manufacturing issues?
• Is the "overall assessment" honest — does it match the issue count and severity?

If a spec is genuinely complete and well-specified, say so. Adversarial means honest, not performatively negative.

After Completing the Review

1. Ask the user for the phase number if it isn't obvious from the spec filename (e.g., "Phase 1").

2. Save the full review output to plans/spec-review-phase-{X}.md.

3. End the session with:

   "{N} issues found ({X} blocking, {Y} required, {Z} suggestions). Start a new session with /spec-workflow to resolve these issues interactively. The issue list has been saved to plans/spec-review-phase-{X}.md."