ci: gate main on spec validity and requirement visibility - #114
ci: gate main on spec validity and requirement visibility#114thecodedrift wants to merge 2 commits into
Conversation
Nothing in CI validated `openspec/specs/`, so spec rot accumulated silently. Add two checks to the Validate job. `pnpm openspec validate --all --strict` runs repo-wide rather than over changed files: the rot lives in the specs a PR does not touch. `--strict` is structurally blind to a second class of failure. A `##` heading inside `## Requirements` ends the section, and every `### Requirement:` below it stops being a requirement to the parser — not invalid, unread. `infrastructure` sat at 1 of 20 visible and `skills` at 1 of 7, both green throughout. So a second check compares requirements written against requirements the parser reaches. Fenced content is skipped: a fenced `### Requirement:` documents the format rather than declaring a requirement. The one edge that opens — a lost opening fence leaving its closer dangling, which swallows the rest of the file — is caught by failing on a fence still open at EOF. Both checks pass on main today: 23 specs, 0 failures, 0 hidden. Also correct the CI trigger requirement in the infrastructure spec. It claimed pull requests targeting `main`; 1c181e2 removed that `branches:` filter because it did not reliably reach stacked PRs. Fixes #105 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
`ci.yml` pins `node-version: 24`; the spec required 22. Neither of the checks this change adds would ever catch it — the requirement is perfectly well-formed, it is just false. Worth noting as the limit of the gate: it raises the floor from "unparseable" to "structurally sound" and says nothing about whether a spec describes reality. Found while adding the gate, and folded in here because it is one line in the file this change already edits.
thecodedrift
added
the
skip-changeset
There was a problem hiding this comment.
Pull request overview
Adds CI enforcement to prevent OpenSpec drift by gating main on both strict OpenSpec validation and a structural “requirement visibility” check that detects truncated ## Requirements sections (a failure mode --strict can’t detect). It also updates the infrastructure spec to document the new CI gates and correct existing workflow requirements (trigger semantics and Node version).
Changes:
- Extend the
Validatejob to runpnpm openspec validate --all --strictrepo-wide and a new visibility checker script. - Add
.github/scripts/openspec-visibility.cjsplus unit tests to detect hidden requirements caused by stray##headings and to fail on unclosed code fences. - Update
openspec/specs/infrastructure/spec.mdto reflect CI trigger behavior, spec-validation requirements, and Node 24.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
openspec/specs/infrastructure/spec.md |
Documents CI triggers more precisely and adds requirements/scenarios for the new spec validation + visibility gates; updates Node requirement to 24. |
.github/workflows/ci.yml |
Adds two new gating steps (strict OpenSpec validation + visibility check) to the Validate job. |
.github/scripts/openspec-visibility.cjs |
New script to count total vs. parser-visible requirements per spec, ignoring fenced blocks and failing on unclosed fences. |
.github/scripts/openspec-visibility.test.cjs |
New unit tests covering visible/hidden counting, fence handling, and main() error reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const errors = []; | ||
| const realError = console.error; | ||
| const realLog = console.log; | ||
| console.error = (message) => errors.push(String(message)); | ||
| console.log = () => {}; | ||
| try { | ||
| assert.equal(main({ argv: [directory] }).ok, false); | ||
| assert.match( | ||
| errors.join("\n"), | ||
| /gamma[/\\]spec\.md: a code fence is still open/ | ||
| ); | ||
| } finally { | ||
| console.error = realError; | ||
| console.log = realLog; | ||
| rmSync(directory, { recursive: true, force: true }); |
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 45s —— View job Review: ci: gate main on spec validity and requirement visibilityOverall assessment: solid, well-tested change. No blocking issues — one plausible correctness gap and one minor test-hygiene note posted inline. What I checked
Inline comments posted
Neither issue blocks merge; the first is worth a quick confidence check against OpenSpec's actual heading parsing, the second is a nice-to-have. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (line.startsWith("## ")) { | ||
| inRequirements = line.trim() === REQUIREMENTS_HEADING; | ||
| } else if (line.startsWith(REQUIREMENT_PREFIX)) { | ||
| total += 1; | ||
| if (inRequirements) { | ||
| visible += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The ## Requirements boundary check requires the heading to be flush-left:
if (line.startsWith("## ")) {
inRequirements = line.trim() === REQUIREMENTS_HEADING;CommonMark (and most Markdown parsers, plausibly including OpenSpec's own) allow up to 3 leading spaces before an ATX heading and still treat it as a real heading. If a stray ## heading is ever written with 1–3 leading spaces inside a ## Requirements section (e.g. from a copy/paste or auto-indent), and OpenSpec's actual parser still treats it as heading that ends the section, this script won't: startsWith("## ") fails on the indented line, so inRequirements never flips to false, and every requirement below stays counted as visible.
That's the exact failure mode this check exists to catch (silently-hidden requirements, passing green) reproduced by a case this specific implementation can't see. Worth confirming against how OpenSpec's own markdown parser recognizes headings — if it also requires column-0, this is moot; if it follows CommonMark's leniency, there's a gap here. Might be worth widening the match to /^ {0,3}##\s/ (mirroring the leading-space tolerance already used in fenceOf) to close it defensively either way.
| const realError = console.error; | ||
| const realLog = console.log; | ||
| console.error = (message) => errors.push(String(message)); | ||
| console.log = () => {}; |
There was a problem hiding this comment.
Minor, and Copilot already flagged this: these tests reassign the global console.error/console.log for the duration of the test, restoring them in finally. With top-level tests in a node --test file running sequentially by default this is safe today, but it's a latent flakiness trap — if concurrency is ever turned on for this file (or a future test is added with { concurrency: true }), one test's captured output can silently swallow or corrupt another's. Since main() already returns { results, ok }, consider having it optionally return the log/error lines it would have printed (or accept an injected logger) instead of relying on global mutation, so assertions don't depend on execution order.
thecodedrift
left a comment
thecodedrift
left a comment
There was a problem hiding this comment.
As an aside, this whole script may go away once we have Vale rules in place, designed to specifically handle this kind of stuff. It's worth noting that when we look at how deep this fix is
Or if we want to do this fix at all
Nothing in CI validated the specs, so spec rot accumulated silently and was only ever found by hand. This adds two checks to the
Validatejob.Two checks, because the first is structurally blind
openspec validate --all --strict— repo-wide, deliberately. The failures this exists to catch live in specs a given PR does not touch, so a changed-files-only check would never surface them.Every
### Requirement:sits under## Requirements— a new.github/scripts/openspec-visibility.cjs, with a unit-test sibling picked up by the existingnode --test .github/scripts/*.test.cjsstep.The second is not redundant. A second
##heading inside the requirements section ends it, and everything below stops being parsed as a requirement — not invalid, invisible. Measured before the check existed:infrastructurehad 20 requirements with 1 visible andskillshad 7 with 1, both passing--strictthe whole time. A passing gate is an active claim that the spec was read, which makes a silently truncated spec worse than a red one.The check nearly shipped with the bug it exists to catch
Fenced code blocks are skipped, because a
### Requirement:inside a fence is documentation about the format rather than a requirement.Writing the test for the lost-opening-fence case — the real defect found in
cli-helpduring #106 — exposed that a surviving closing fence opens a region to EOF, hiding the rest of the file from the check as well as the parser.hiddencame back 0. An unclosed fence now fails on its own terms with its own message.Both gates are green on day one
--strictreports 23 passed / 0 failed, and the visibility check reports all 23 specsok. #106 cleared the backlog that previously blocked this. Turning a gate on red is how a check gets ignored.Deliberate-failure proof: injecting a stray
## Stray Groupinginto a copy ofskills/spec.mdexits 1 with6 requirement(s) hidden by a '##' heading inside '## Requirements' (1 of 7 visible)— reproducing the historical 7→1 figure exactly.Spec changes
Two edits to
openspec/specs/infrastructure/spec.md, made directly rather than through a change proposal:mainbranch"; that stopped being true in1c181e2, which removed thebranches:filter after it silently stopped reaching stacked PRs.ci.ymlpins 24.Worth stating the limit plainly: neither new check would have caught either correction. Both requirements were perfectly well-formed and simply false. This raises the floor from "unparseable" to "structurally sound" and says nothing about whether a spec describes reality.
Verification
pnpm lintclean ·pnpm typecheck1/1 ·pnpm test602 passing ·openspec validate --all --strict23/0 ·node --test .github/scripts/*.test.cjs124/0No changeset: this touches CI, a workflow script, and a spec. Nothing ships, so the PR carries
skip-changesetrather than a release note that would describe nothing.Fixes #105