| name | review-spec |
| description | Use when reviewing a ticket's scenarios (`.feature` source, with legacy test-definitions.md fallback) — auto-fired by the bdd scenario-gate and re-invokable after scenario edits. Runs vacuous-pass, AODI, determinism, negative-case, and cross-cutting checks and produces a structured findings report. NOT for spec.md JTBD/criteria/persona framing — that is self-review. |
| allowed-tools | * |
Review Spec — Scenario Quality Gate
Adversarially review a ticket's scenarios: treat them as if you're trying to break them — find the one that passes for the wrong reason, the missing rejection path, the flaky assertion. This is the bdd scenario-gate procedure, extracted so it runs two ways:
- Auto-fire — the bdd flow invokes this on entering the
scenario-gate phase.
- Manual re-run — invoke
/review-spec anytime after define-behavior (e.g., scenarios changed during implement and you want to re-validate). Allowed on a closed ticket too — a post-hoc audit is still readable.
Read the active ticket's .feature source first; use test-definitions.md only as the R/G/R ledger and as a legacy scenario fallback when no feature source exists. test-definitions.md is the R/G/R ledger. Run every check below against the scenarios, and present findings in the Findings format at the end. (Not a spec.md framing review — JTBD/criteria/persona checks live in self-review.)
Vacuous-pass test
Run this first — a scenario that would pass without the feature invalidates every check below it. Mentally delete the implementation and ask: could this scenario still pass? If yes, it is vacuous: flag it and propose a stronger Then, not just a warning. (A good test is behavioral — if the behavior changed, the result should change; a scenario that survives a deleted feature tests nothing.)
Common vacuous patterns, each with its fix:
- Existence-only
Then ("a response is returned") → assert the actual value, not that something came back.
- Given-echo ("Given a row with X exists … Then a read returns X") → that exercises the store, not the feature; assert something the feature must compute or change.
- Trivially-true setup — the
Given already makes the Then true regardless of the When → move the real precondition out of the assertion.
- Non-claim
Then ("the system remains running") → assert a falsifiable outcome the feature produces.
AODI validation
Validate each scenario against four criteria:
| Criterion | Check | Red flag |
|---|
| Atomic | Tests ONE behavior | Multiple When/Then pairs |
| Observable | Has externally visible outcome | Internal state only |
| Deterministic | Same result on repeated runs | Time/random/external dependency |
| Independent | No ordering dependency | "After Scenario 2 runs..." |
Determinism risks
Sharpen AODI's Deterministic check with the patterns that actually flake in CI — each with its fix:
- Time without a wait — a
Then that depends on elapsed time, or asserts an async result after a fixed delay → wait on an observable condition (poll/await the state), never a bare sleep.
- Order-dependent comparison — asserting an unordered collection as if it were ordered → sort, or compare as a set, before asserting.
- Unsequenced concurrency — a
Then over concurrent operations with no stated ordering → assert on the settled end-state, or name the ordering guarantee the scenario relies on.
Assertion strength (weak vs strong Then) isn't repeated here — it is testing Iron Law 2, and the vacuous-pass check above already coaches a stronger Then.
Adversarial pass
After AODI validation, argue against your own scenario list: "What breaks that none of these scenarios catch?" Present any findings to the user.
One lens to always run — negative-case coverage: for each happy-path scenario, is there a rejection-path counterpart? Partitioning should already have produced the invalid-input classes (equivalence partitioning covers invalid ranges, not only valid ones); this pass is the backstop. Common pairs — create ↔ duplicate, read ↔ not-found, update ↔ not-allowed, act ↔ precondition-failed. Treat a gap as should-strengthen, not must-fix — a sibling AC often already covers the rejection: "Happy path X has no rejection counterpart — add a scenario for path Z?" For one behavior across many inputs, use a Scenario Outline.
Cross-cutting checks
Six lenses across the whole scenario set (not per scenario) — each asks "what's missing?":
- Conflict — do two scenarios contradict (one allows X, another rejects it) with no distinguishing precondition?
- Boundary — zero / one / max / empty / null covered where they apply?
- Failure — external-dependency failures covered (timeout, 5xx, malformed, partition)? Distinct from the feature's own rejections (the negative-case lens above).
- Security — authn/authz failures and abuse vectors covered?
- Persona consistency — is each scenario's triggering persona clear, and would another persona experience it differently?
- Surface coverage — if
spec.md lists affected surfaces, does each affected surface have a matching @surface.<slug> scenario tag or an explicit skip: reason, and are any @surface.* tags stale?
- Wiring — for each behavior that crosses a module/command boundary, is there a scenario exercised end-to-end through the real entry point (real config → real collaborators, mocking only the process boundary), not only via injected internals? A path reachable solely through a
provider: none-style short circuit has no wiring coverage (see testing/SKILL.md → Wiring Tests).
Findings format
Report findings the way safeword talks to the user — lead with the answer, structure only because a multi-finding review earns it, end with the call:
- Lead with a tally —
**Findings:** N must-fix, M should-strengthen, P looks-good.
- Three tiers — Must Fix (correctness/structure), Should Strengthen (clarity/specificity), Looks Good (specific acknowledgement, never padding).
- One
#### per finding with the scenario id + a short issue; under it, Current (quote the G/W/T, bold the offending phrase) → why → Proposed (the rewrite). Fix last, so the explanation reads as the answer, not justification.
- Bulk — when one pattern hits ≥3 scenarios: one header, an Affected id list, one Representative quote, one Proposed pattern.
- End with
**Next:** — the single fix to start.
**Findings:** 1 must-fix, 0 should-strengthen.
#### oauth.PO1.AC2.change_applies — Then joins two assertions with "and"
Current: "Then the config shows B and later auths use B" — two independent observables.
Proposed: "Then later authentications use User Source B."
**Next:** split the AC2 scenario, then re-run the gate.
After the review
When invoked auto-fire from the bdd scenario-gate, hand control back to bdd/SCENARIOS.md for the Scenario Gate Exit (advance the phase to plan-implementation — the proof plan is recorded there per bdd/PLAN_IMPLEMENTATION.md; work-log). When invoked manually, stop after presenting findings — the driver decides what to fix and whether to re-run.
