Skip to content

docs: add spec & test shape-review checklist (presence vs shape gap)#56

Merged
awais786 merged 1 commit into
mainfrom
spec-review-checklist
Jun 3, 2026
Merged

docs: add spec & test shape-review checklist (presence vs shape gap)#56
awais786 merged 1 commit into
mainfrom
spec-review-checklist

Conversation

@awais786

@awais786 awais786 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

The bidirectional audit gate (scripts/check-spec-coverage.sh + tests/meta/playwright-practices.spec.ts) enforces PRESENCE — every requirement has a tag, every contract-bearing test has a tag pointing at a requirement. That's load-bearing.

What the gate does NOT enforce is SHAPE — whether the test's assertion actually fails when the contract breaks. Four distinct shape bugs have shipped through the gate in this repo:

  1. workspace-auto-join-independence — asserted "no UUID collision across 4 independent DBs," structurally impossible to fail.
  2. jwt-algorithm-confusion — toBeGreaterThanOrEqual(200), every HTTP status is >= 200, tautology.
  3. login-logout-flow — @SPEC tag pointed at a contract the file's tests don't exercise.
  4. cookie-bomb-dos — required HTTP 4xx, but the bundle's fail-closed shape is 302-to-IDP; 6/7 entry points failed CI while behaving correctly.

Each passed the automated gate. None would have shipped if the author had walked a structured shape-review checklist.

Adds docs/spec-review-checklist.md:

  • Part A: 5-item checklist for adding/editing a ### Requirement: (strong verb, observable, no implementation coupling, scenario block, failure mode named).
  • Part B: 6-item checklist for adding/editing an @spec-tagged assertion (tag points at exercised contract, counterfactual exists, assertion can fail, range assertions enumerate the acceptable set, per-app generalisation handled, precondition does not bake the contract).
  • Worked examples: each of the four shape bugs walked through the checklist as the author/reviewer would have.
  • Smell tests: three quick questions that catch most cases.

Adds a pointer from CLAUDE.md (so Claude sessions consult it before editing specs / @spec-tagged tests) and from README.md (so human PR authors land on it).

No tooling change. Pure discipline document — the enforcement is "reviewer walks the checklist out loud in the PR comments before approving."

Audit + typecheck remain clean (88 covered/deferred/missing total unchanged).

Motivation / Background

This Pull Request has been created because:

  • Resolves #issue-id

Detail

This Pull Request changes:

Additional information

TIP: Provide additional information such as screenshots, benchmarks, reference to other repositories or alternative solutions

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature
  • CHANGELOG files are updated for the behavior change or additional feature (minor bug fixes and documentation changes should not be included)
  • This PR contains API changes and API documentation is updated accordingly (for critical or behavior change, please inform related parties about them).

The bidirectional audit gate (scripts/check-spec-coverage.sh +
tests/meta/playwright-practices.spec.ts) enforces PRESENCE — every
requirement has a tag, every contract-bearing test has a tag pointing
at a requirement. That's load-bearing.

What the gate does NOT enforce is SHAPE — whether the test's
assertion actually fails when the contract breaks. Four distinct
shape bugs have shipped through the gate in this repo:

  1. workspace-auto-join-independence — asserted "no UUID collision
     across 4 independent DBs," structurally impossible to fail.
  2. jwt-algorithm-confusion — toBeGreaterThanOrEqual(200), every
     HTTP status is >= 200, tautology.
  3. login-logout-flow — @SPEC tag pointed at a contract the
     file's tests don't exercise.
  4. cookie-bomb-dos — required HTTP 4xx, but the bundle's
     fail-closed shape is 302-to-IDP; 6/7 entry points failed CI
     while behaving correctly.

Each passed the automated gate. None would have shipped if the
author had walked a structured shape-review checklist.

Adds docs/spec-review-checklist.md:

  - Part A: 5-item checklist for adding/editing a `### Requirement:`
    (strong verb, observable, no implementation coupling, scenario
    block, failure mode named).
  - Part B: 6-item checklist for adding/editing an @spec-tagged
    assertion (tag points at exercised contract, counterfactual
    exists, assertion can fail, range assertions enumerate the
    acceptable set, per-app generalisation handled, precondition
    does not bake the contract).
  - Worked examples: each of the four shape bugs walked through
    the checklist as the author/reviewer would have.
  - Smell tests: three quick questions that catch most cases.

Adds a pointer from CLAUDE.md (so Claude sessions consult it before
editing specs / @spec-tagged tests) and from README.md (so human
PR authors land on it).

No tooling change. Pure discipline document — the enforcement is
"reviewer walks the checklist out loud in the PR comments before
approving."

Audit + typecheck remain clean (88 covered/deferred/missing total
unchanged).
@awais786 awais786 merged commit f0deb1f into main Jun 3, 2026
2 checks passed
awais786 added a commit that referenced this pull request Jun 3, 2026
CI surfaced one false positive after merging: CLAUDE.md line 21
contains the backtick literal `git log -- tests/`. The path-detection
heuristic was "contains a slash AND ends in .ext or /" — `tests/`
matched that even though the full literal is a shell command, not a
file path.

Tightening the heuristic to also require no whitespace inside the
backtick literal catches this class without losing real coverage:
file paths in prose never contain spaces (they're either single
tokens like `tests/auth/foo.spec.ts` or markdown links). Shell
commands, prose phrases ("`git log -- tests/`", "`cd tests/`"), and
sentence fragments all do contain spaces.

Verified against the CI-surfaced case + the two skills.md references
to docs/spec-review-checklist.md (which now exist on origin/main
after PR #56 merged but not yet on this branch's local main).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant