docs(review): reject self-validating/vacuous tests as [MAJOR] (bookshelf-s957) #1417
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-s957"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a 'reject self-validating / vacuous tests' section to .claude/rules/review-standard.md (graded [MAJOR]) and a matching prohibition to tdd-workflow.md. Motivated by the can't-fail tests caught 6x in epic ukqt + reader work, and the two gate-gaming attempts this session. Docs/rules only — no code. Closes bead bookshelf-s957 on merge.
Security Review — PR #1417
Verified the diff is documentation-only: two files changed, both
.claude/rules/*.md(review-standard.md,tdd-workflow.md). Confirmed viapulls/1417.diff— no.go,.sql,.yml/.yaml(no.forgejo/workflows/changes),Makefile, template, JS/CSS, or any other executable/config file is touched. The change is purely additive prose adding a new review rule ("reject self-validating/vacuous tests") plus a one-line cross-reference intdd-workflow.md. No secrets, credentials, URLs, or scripts are introduced.No multi-user scoping, authZ, injection/SSRF, input-bounds, architecture-boundary, secrets/PII, or CSP surface is present in this diff — none of those categories apply to markdown-only rule files with no runtime consumption.
No findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review — PR #1417 (bd-bookshelf-s957)
Diff-review only (markdown-only docs/rules change, CI green).
Verification performed:
bookshelf-dfknwcitation is a real, closed bead whose description matches the cited example (OIDC valid-library-IDs test that passes only because the stub returns nil).[MAJOR]severity floor is consistent with every other section's pattern in the file ("at least a [MAJOR]" — Flake prevention, Linter baseline integrity, Workflow versioning all use this phrasing).tdd-workflow.md's new prohibition bullet correctly cross-referencesreview-standard.md.Findings:
[MINOR] .claude/rules/review-standard.md:110 — Duplicate adjacent section-header prefix
The new
## Test hygiene (reject self-validating / vacuous tests)header sits immediately after the pre-existing## Test hygiene (reject white-box / internal tests)header. Two consecutive H2s sharing the same "Test hygiene" prefix is a minor scannability nit (grep-by-heading / flat TOC). Consider merging under one## Test hygieneparent with subheadings, or renaming the new one (e.g.## Vacuous / self-validating tests). Cosmetic only.[MINOR] .claude/rules/review-standard.md:117-126 — "Self-validating" bullet conflates two distinct failure modes
The bullet mixes "asserting state the test itself created/set" (test never touches production output) with "a value echoed straight back from a stub" (production path runs but the stub makes the assertion trivially true). Both are real and worth banning, but splitting into two clauses would make the rule slightly easier to pattern-match against in the wild. Not required — guidance is still correct and enforceable as written.
No blockers or majors — this is a pure documentation diff with no code, factually accurate to its cited precedent, and internally consistent with the file's existing severity-grading conventions.
REVIEW VERDICT: 0 blocker, 0 major, 2 minor