ci: gate main on spec validity and requirement visibility - #114
ci: gate main on spec validity and requirement visibility#114thecodedrift wants to merge 3 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.
|
@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}
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
Review asked whether the '## Requirements' boundary should tolerate up to
three leading spaces, per CommonMark. It should not: every heading pattern
in OpenSpec is anchored at column 0 (markdown-parser's /^(#{1,6})\s+(.+)$/,
requirement-blocks' /^##\s+Requirements\s*$/i and /^###\s*Requirement:/),
so an indented '##' is prose to the parser and widening the match would
report requirements hidden that are read fine.
Checking that did surface three real divergences, all in the direction that
matters — the check being laxer than the parser, so a hidden requirement
reads as visible:
- OpenSpec matches \s after the hashes, not a literal space, so a tab-led
'##\tGrouping' truncates the section while 'startsWith("## ")' missed it.
- '###Requirement:' and '### Requirement:' both parse, but only the
single-space form was counted toward the total.
- The requirements heading is matched case-insensitively by the parser;
'## requirements' would have reported every requirement hidden.
Also injects log/error into main() instead of reassigning the global console
in tests, per Copilot and Claude — safe today under node --test's sequential
default, a flakiness trap the moment anything here gains concurrency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Both inline findings are answered and resolved on their threads, and 019aa46 addresses them: the heading-leniency gap is declined (OpenSpec anchors every heading pattern at column 0, so tolerating leading spaces would report requirements hidden that the parser reads), but checking it surfaced three real divergences in the laxer direction — tab-after- — AI Coding Agent |
Noted, and worth keeping in view — but Vale would be a different check rather than the same one relocated. Vale reads prose against style rules; the thing this script asserts is agreement with OpenSpec's own parser (its heading regexes are now mirrored line for line, see 019aa46), i.e. "did the parser read the whole file". A Vale rule could enforce "no — AI Coding Agent |
|
@claude /review |
|
Claude finished @thecodedrift's task in 4m 13s —— View job Review: ci: gate main on spec validity and requirement visibilityOverall assessment: solid, well-tested change with good design reasoning (and good responsiveness to the earlier review round — 019aa46 closed three real gaps found while checking the leading-space question). One concrete but narrow-trigger correctness gap posted inline; the rest are non-blocking design/efficiency notes. What I checked
Inline comment posted
Non-blocking notes (design/efficiency, not correctness bugs)
Nothing else risked being a security, performance, or test-adequacy issue — the fence-skip/fence-still-open interplay, the |
| */ | ||
| const REQUIREMENTS_HEADING = /^##\s+Requirements\s*$/i; | ||
| const SECTION_HEADING = /^##\s/; | ||
| const REQUIREMENT_HEADING = /^###\s*Requirement:/; |
There was a problem hiding this comment.
The block comment directly above (lines 56-59) cites OpenSpec's own requirement-heading pattern as /^###\s*Requirement:\s*(.+)\s*$/ — which requires at least one character of title text after the colon. The implemented REQUIREMENT_HEADING drops that suffix:
const REQUIREMENT_HEADING = /^###\s*Requirement:/;Tracing both regexes by hand against a bare ### Requirement: line with nothing after the colon: the local pattern matches (nothing after Requirement: is required), but the cited upstream pattern does not (\s* can match empty, but (.+) then has no characters left to consume). Any variant with even one trailing character does match both, so the divergence is narrow — exactly the title-less case.
Failure scenario: a title-less ### Requirement: heading appearing outside ## Requirements (e.g. an incidental heading in a "Design" or prose section) is perfectly valid markdown that openspec validate --all --strict won't flag, since OpenSpec's own parser — per the pattern this file cites for itself — doesn't recognize it as a requirement at all. This script still increments total for it without incrementing visible, producing hidden > 0 and failing this new gate on a spec the real parser reads cleanly. Inside ## Requirements the gap is harmless (total and visible move together), so this only bites as a false-positive failure mode.
Given the PR's own framing — "this check is only meaningful where it agrees with the parser" — this seems worth closing, e.g. /^###\s*Requirement:\s*\S/. Not covered by any existing test in openspec-visibility.test.cjs (the spacing tests around line 110-123 cover missing/extra space before "Requirement", not a missing title).
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