Filed from the review on #2701, which flagged the ADR spec as contradicting the WARN-only design. Verified — and it is broader than one assertion.
Nothing runs these
blueprint-plugin/hooks/spec/ holds four shellspec files. shellspec appears nowhere in .github/workflows/, the justfile, .pre-commit-config.yaml, or any other runner, and is not installed. So they have never failed, because they have never run.
They describe a validator that #2440 deleted
#2440 replaced the hand-rolled bash field lists with check-schema.py driven by schemas/*.schema.json. The specs still assert the old hook's messages and exit codes. Measured against the live hook today:
Spec assertion (validate_adr_frontmatter_spec.sh) |
Actual behaviour |
Verdict |
missing id → exit 2, ERROR: Missing required frontmatter field: id |
exit 2, SEVERITY=ERROR … AT=/frontmatter DETAIL='id' is a required property |
status ✅, message ❌ |
bad id format → exit 2, ERROR: Invalid ADR id format |
exit 2, schema-violation line |
status ✅, message ❌ |
bad status → exit 2, ERROR: Invalid ADR status |
exit 2, 'Pending' is not one of [...] |
status ✅, message ❌ |
missing ## Decision → exit 2, ERROR: Missing required section: ## Decision |
exit 0, STATUS=WARN |
❌ both |
missing domain → exit 0, WARNING: Missing 'domain' field |
STATUS=OK, no domain warning at all |
status ✅, message ❌ |
Every message assertion is wrong; two of five are wrong on exit status too. validate_prp_frontmatter_spec.sh:90 has the same missing-section shape (ERROR: Missing required section: ## Success Criteria) and prp.schema.json also carries x-blueprint-severity: warning, so it is wrong the same way. validate_prd_frontmatter_spec.sh and check_prp_readiness_spec.sh need the same audit.
One thing the specs get right: the hook writes to stderr, so The stderr should include is the correct stream.
Why this is a question
Two defensible paths, materially different work:
- Delete all four. They assert a deleted design, nothing runs them, and
scripts/tests/test-check-schema.sh already covers this ground properly — 27 assertions, semantic (it executes the validator), mutation-verified, and actually wired into pre-commit. On this reading the specs are not a test suite, they are stale documentation that looks like one, and the cost of keeping them is that the next reader believes hooks are tested when they are not.
- Rewrite them against the structured output and wire shellspec into CI. Recovers genuine hook-level coverage that
test-check-schema.sh does not have: these drive the hook's own stdin/stderr/exit-code contract end to end. Costs a new CI dependency, and a rewrite of every assertion.
Option 1 is the smaller, honester change and is what I would lean to given the duplication with test-check-schema.sh. Option 2 is right if hook-contract coverage is wanted as a distinct layer.
What is not defensible is leaving them as they are — a suite that cannot fail cannot regress, and it misrepresents the coverage.
Not done in #2701
Deliberately. The PR's scope was narrowing the ADR section set; deleting or rewriting four test files is a different change. shellspec is also not installable in that session's sandbox (the GitHub release archive returns 403 through the proxy), so any rewrite there would have been unverifiable — editing tests one cannot run is how this situation arose.
Refs #2440, #2701, #2446
Filed from the review on #2701, which flagged the ADR spec as contradicting the WARN-only design. Verified — and it is broader than one assertion.
Nothing runs these
blueprint-plugin/hooks/spec/holds four shellspec files.shellspecappears nowhere in.github/workflows/, thejustfile,.pre-commit-config.yaml, or any other runner, and is not installed. So they have never failed, because they have never run.They describe a validator that #2440 deleted
#2440 replaced the hand-rolled bash field lists with
check-schema.pydriven byschemas/*.schema.json. The specs still assert the old hook's messages and exit codes. Measured against the live hook today:validate_adr_frontmatter_spec.sh)id→ exit 2,ERROR: Missing required frontmatter field: idSEVERITY=ERROR … AT=/frontmatter DETAIL='id' is a required propertyERROR: Invalid ADR id formatERROR: Invalid ADR status'Pending' is not one of [...]## Decision→ exit 2,ERROR: Missing required section: ## DecisionSTATUS=WARNdomain→ exit 0,WARNING: Missing 'domain' fieldSTATUS=OK, no domain warning at allEvery message assertion is wrong; two of five are wrong on exit status too.
validate_prp_frontmatter_spec.sh:90has the same missing-section shape (ERROR: Missing required section: ## Success Criteria) andprp.schema.jsonalso carriesx-blueprint-severity: warning, so it is wrong the same way.validate_prd_frontmatter_spec.shandcheck_prp_readiness_spec.shneed the same audit.One thing the specs get right: the hook writes to stderr, so
The stderr should includeis the correct stream.Why this is a
questionTwo defensible paths, materially different work:
scripts/tests/test-check-schema.shalready covers this ground properly — 27 assertions, semantic (it executes the validator), mutation-verified, and actually wired into pre-commit. On this reading the specs are not a test suite, they are stale documentation that looks like one, and the cost of keeping them is that the next reader believes hooks are tested when they are not.test-check-schema.shdoes not have: these drive the hook's own stdin/stderr/exit-code contract end to end. Costs a new CI dependency, and a rewrite of every assertion.Option 1 is the smaller, honester change and is what I would lean to given the duplication with
test-check-schema.sh. Option 2 is right if hook-contract coverage is wanted as a distinct layer.What is not defensible is leaving them as they are — a suite that cannot fail cannot regress, and it misrepresents the coverage.
Not done in #2701
Deliberately. The PR's scope was narrowing the ADR section set; deleting or rewriting four test files is a different change.
shellspecis also not installable in that session's sandbox (the GitHub release archive returns 403 through the proxy), so any rewrite there would have been unverifiable — editing tests one cannot run is how this situation arose.Refs #2440, #2701, #2446