From 0aeb8330d526ae86b4d7b5fbaae385e7107d928b Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 00:07:34 -0700 Subject: [PATCH 1/4] ci: gate main on spec validity and requirement visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .github/scripts/openspec-visibility.cjs | 205 +++++++++++++++ .github/scripts/openspec-visibility.test.cjs | 252 +++++++++++++++++++ .github/workflows/ci.yml | 11 + openspec/specs/infrastructure/spec.md | 51 +++- 4 files changed, 516 insertions(+), 3 deletions(-) create mode 100644 .github/scripts/openspec-visibility.cjs create mode 100644 .github/scripts/openspec-visibility.test.cjs diff --git a/.github/scripts/openspec-visibility.cjs b/.github/scripts/openspec-visibility.cjs new file mode 100644 index 00000000..995f375b --- /dev/null +++ b/.github/scripts/openspec-visibility.cjs @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Requirement visibility check — catch specs the OpenSpec parser silently + * stops reading part-way through. + * + * WHY THIS EXISTS. `openspec validate --all --strict` answers "is what I read + * well-formed", not "did I read the whole file". A spec's requirements live + * under `## Requirements`, and a second `##` heading inside that section ends + * it: every `### Requirement:` below the intruding heading is no longer a + * requirement to the parser, it is prose. Those requirements are not invalid, + * they are INVISIBLE — so `--strict` reports success having never looked at + * them. Measured before this check existed, `infrastructure` carried 20 + * requirements with 1 visible and `skills` carried 7 with 1, both green the + * whole time. A passing gate is an active claim that the spec was read, which + * makes this failure mode worse than a red check rather than milder. + * + * WHAT IT CHECKS. For each `openspec/specs//spec.md`: the number of + * `### Requirement:` headings in the file must equal the number the parser can + * reach. Any difference fails, naming the spec and the count. + * + * FENCED CODE BLOCKS ARE NOT SPEC CONTENT. Lines inside a ``` or ~~~ fence are + * skipped entirely — they neither count as requirements nor open or close the + * requirements section. Spec files do contain fenced examples, and a fenced + * `### Requirement:` is documentation ABOUT the format, not a requirement the + * parser was ever meant to see; counting it would report a hidden requirement + * that does not exist and push authors to mangle their examples. Skipping + * fences costs nothing in detection power: the `cli-help` defect this family of + * bugs comes from was a LOST OPENING fence, which turns the block's `##` lines + * into real headings — and real headings are exactly what this check reads. + * + * AN UNCLOSED FENCE IS ITSELF A FAILURE. Skipping fenced content has one edge: + * a lost opening fence leaves the block's CLOSING ``` dangling, and a dangling + * fence opens a region that runs to end of file, hiding everything after it + * from this check as well as from the parser. So a fence still open at EOF + * fails outright rather than being tolerated — the one case where "I could not + * read the rest of the file" is the finding. + * + * Usage: + * node .github/scripts/openspec-visibility.cjs [specsDirectory] + * + * Exits non-zero when any spec has requirements the parser cannot see. + */ +const { readdirSync, readFileSync, existsSync } = require("node:fs"); +const { join, relative, resolve } = require("node:path"); + +const REPO_ROOT = join(__dirname, "..", ".."); +const DEFAULT_SPECS_DIRECTORY = join(REPO_ROOT, "openspec", "specs"); + +const REQUIREMENTS_HEADING = "## Requirements"; +const REQUIREMENT_PREFIX = "### Requirement:"; + +/** + * A fence opens on ``` or ~~~ (up to three leading spaces, per CommonMark) and + * closes on a run of the same character at least as long. Tracking the opener's + * length and character is what lets a ```` ```` ```` block contain a ``` line + * without the check losing its place. + */ +function fenceOf(line) { + const match = /^ {0,3}(`{3,}|~{3,})/.exec(line); + return match ? { char: match[1][0], length: match[1].length } : null; +} + +/** + * Count requirements written in a spec against requirements the parser reaches. + * Pure and string-in, so the interesting cases are testable without a fixture + * tree on disk. + */ +function countRequirements(source) { + let total = 0; + let visible = 0; + let inRequirements = false; + let openFence = null; + + for (const line of source.split("\n")) { + const fence = fenceOf(line); + if (openFence) { + if ( + fence && + fence.char === openFence.char && + fence.length >= openFence.length + ) { + openFence = null; + } + continue; + } + if (fence) { + openFence = fence; + continue; + } + + if (line.startsWith("## ")) { + inRequirements = line.trim() === REQUIREMENTS_HEADING; + } else if (line.startsWith(REQUIREMENT_PREFIX)) { + total += 1; + if (inRequirements) { + visible += 1; + } + } + } + + return { + total, + visible, + hidden: total - visible, + unclosedFence: openFence !== null, + }; +} + +/** Every `/spec.md` under the specs directory, sorted for stable output. */ +function findSpecs(specsDirectory) { + if (!existsSync(specsDirectory)) { + return []; + } + return readdirSync(specsDirectory, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => join(specsDirectory, entry.name, "spec.md")) + .filter((path) => existsSync(path)) + .sort(); +} + +/** Repo-relative when the spec lives in the repo, absolute otherwise (tests, ad-hoc runs). */ +function displayPath(path) { + const relativePath = relative(REPO_ROOT, path); + return relativePath.startsWith("..") ? path : relativePath; +} + +function checkSpecs(specsDirectory = DEFAULT_SPECS_DIRECTORY) { + return findSpecs(specsDirectory).map((path) => ({ + path, + ...countRequirements(readFileSync(path, "utf8")), + })); +} + +function main({ argv = process.argv.slice(2) } = {}) { + const specsDirectory = argv[0] + ? resolve(process.cwd(), argv[0]) + : DEFAULT_SPECS_DIRECTORY; + const results = checkSpecs(specsDirectory); + + if (results.length === 0) { + console.error(`No specs found under ${specsDirectory}`); + return { results, ok: false }; + } + + for (const result of results) { + const name = displayPath(result.path); + const status = + result.hidden > 0 + ? "HIDDEN " + : result.unclosedFence + ? "UNCLOSED" + : "ok "; + console.log( + ` ${status} ${name} ${result.visible}/${result.total} requirement(s) visible` + ); + } + + const broken = results.filter( + (result) => result.hidden > 0 || result.unclosedFence + ); + if (broken.length === 0) { + console.log( + `\nEvery requirement in ${results.length} spec(s) is visible to the parser.` + ); + return { results, ok: true }; + } + + console.error(""); + for (const result of broken) { + const name = displayPath(result.path); + if (result.hidden > 0) { + console.error( + `${name}: ${result.hidden} requirement(s) hidden by a '##' heading inside '## Requirements' (${result.visible} of ${result.total} visible)` + ); + } + if (result.unclosedFence) { + console.error( + `${name}: a code fence is still open at end of file, so everything after it is unreadable to this check and to the parser (likely a lost opening fence leaving its closer dangling)` + ); + } + } + if (broken.some((result) => result.hidden > 0)) { + console.error( + "\nA '##' heading ends the requirements section. Use a bold lead-in line for topical grouping instead, so the requirements below it stay readable to the parser." + ); + } + return { results, ok: false }; +} + +// Exported so openspec-visibility.test.cjs can drive the counter over inline +// sources and main() over a temporary spec tree, with nothing on disk to keep. +module.exports = { countRequirements, findSpecs, checkSpecs, main }; + +if (require.main === module) { + try { + if (!main().ok) { + process.exitCode = 1; + } + } catch (error) { + console.error(`\nopenspec-visibility failed: ${error.message}`); + process.exitCode = 1; + } +} diff --git a/.github/scripts/openspec-visibility.test.cjs b/.github/scripts/openspec-visibility.test.cjs new file mode 100644 index 00000000..43400542 --- /dev/null +++ b/.github/scripts/openspec-visibility.test.cjs @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for openspec-visibility.cjs — the check that catches requirements the + * OpenSpec parser stops reading, rather than requirements it reads and rejects. + * + * Most cases run the counter over an inline spec string; the two that exercise + * main() build a throwaway spec tree in a temp directory, so nothing here reads + * or depends on the committed specs. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { mkdtempSync, mkdirSync, writeFileSync, rmSync } = require("node:fs"); +const { tmpdir } = require("node:os"); +const { join } = require("node:path"); + +const { + countRequirements, + checkSpecs, + main, +} = require("./openspec-visibility.cjs"); + +const CLEAN_SPEC = [ + "# capability Specification", + "", + "## Purpose", + "", + "Why this capability exists.", + "", + "## Requirements", + "", + "### Requirement: First thing", + "", + "The system SHALL do the first thing.", + "", + "#### Scenario: It happens", + "", + "- **WHEN** asked", + "- **THEN** it happens", + "", + "### Requirement: Second thing", + "", + "The system SHALL do the second thing.", + "", + "#### Scenario: It also happens", + "", + "- **WHEN** asked", + "- **THEN** it also happens", + "", +].join("\n"); + +/** The clean spec with a stray `## Grouping` inserted before the second requirement. */ +const TRUNCATED_SPEC = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "## Grouping\n\n### Requirement: Second thing" +); + +test("a clean spec has every requirement visible", () => { + assert.deepEqual(countRequirements(CLEAN_SPEC), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("a stray '##' heading hides every requirement below it", () => { + assert.deepEqual(countRequirements(TRUNCATED_SPEC), { + total: 2, + visible: 1, + hidden: 1, + unclosedFence: false, + }); +}); + +test("a bold lead-in line groups topics without hiding anything", () => { + // The sanctioned alternative to a `##` heading, so it must stay clean. + const grouped = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "**Grouping.**\n\n### Requirement: Second thing" + ); + assert.deepEqual(countRequirements(grouped), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("a fenced '### Requirement:' example is not counted at all", () => { + // It is documentation about the format, not a requirement the parser was + // ever meant to see — counting it would report a hidden requirement that + // does not exist. + const withExample = CLEAN_SPEC.replace( + "### Requirement: Second thing", + [ + "### Requirement: Second thing", + "", + "```markdown", + "### Requirement: An illustrative example", + "```", + "", + ].join("\n") + ); + assert.deepEqual(countRequirements(withExample), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("a fenced '## ' line does not close the requirements section", () => { + const withHeadingExample = CLEAN_SPEC.replace( + "### Requirement: Second thing", + [ + "```markdown", + "## Some Other Section", + "```", + "", + "### Requirement: Second thing", + ].join("\n") + ); + assert.deepEqual(countRequirements(withHeadingExample), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("a lost opening fence is caught by the unclosed-fence rule", () => { + // This is the cli-help defect: the opener is lost, so the template's `##` + // becomes a real heading AND the surviving closer opens a region running to + // end of file. The dangling fence hides the requirement below it from the + // hidden-count too, which is exactly why an unclosed fence fails on its own. + const lostOpener = CLEAN_SPEC.replace( + "### Requirement: Second thing", + ["## Goal", "```", "", "### Requirement: Second thing"].join("\n") + ); + const result = countRequirements(lostOpener); + assert.equal(result.unclosedFence, true); + assert.equal(result.hidden, 0, "the dangling fence swallows the evidence"); +}); + +test("main fails and explains an unclosed fence", () => { + const lostOpener = CLEAN_SPEC.replace( + "### Requirement: Second thing", + ["## Goal", "```", "", "### Requirement: Second thing"].join("\n") + ); + const directory = specTree({ gamma: lostOpener }); + 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 }); + } +}); + +test("a tilde fence is honoured, and a longer fence contains a shorter one", () => { + const nested = CLEAN_SPEC.replace( + "### Requirement: Second thing", + [ + "~~~markdown", + "## Not A Heading", + "~~~", + "", + "````", + "```", + "## Also Not", + "````", + "", + "### Requirement: Second thing", + ].join("\n") + ); + assert.deepEqual(countRequirements(nested), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +/** Build a temp `specs//spec.md` tree and return its directory. */ +function specTree(specsByName) { + const directory = mkdtempSync(join(tmpdir(), "openspec-visibility-")); + for (const [name, source] of Object.entries(specsByName)) { + mkdirSync(join(directory, name), { recursive: true }); + writeFileSync(join(directory, name, "spec.md"), source); + } + return directory; +} + +test("checkSpecs reports one row per spec directory", () => { + const directory = specTree({ alpha: CLEAN_SPEC, beta: TRUNCATED_SPEC }); + try { + const results = checkSpecs(directory); + assert.equal(results.length, 2); + assert.deepEqual( + results.map((result) => result.hidden), + [0, 1] + ); + } finally { + rmSync(directory, { recursive: true, force: true }); + } +}); + +test("main fails and names the offending spec and count", () => { + const directory = specTree({ alpha: CLEAN_SPEC, beta: TRUNCATED_SPEC }); + const errors = []; + const logs = []; + const realError = console.error; + const realLog = console.log; + console.error = (message) => errors.push(String(message)); + console.log = (message) => logs.push(String(message)); + try { + const { ok } = main({ argv: [directory] }); + assert.equal(ok, false); + const reported = errors.join("\n"); + assert.match(reported, /beta[/\\]spec\.md/); + assert.match(reported, /1 requirement\(s\) hidden/); + assert.doesNotMatch(reported, /alpha/); + } finally { + console.error = realError; + console.log = realLog; + rmSync(directory, { recursive: true, force: true }); + } +}); + +test("main passes when every spec is clean", () => { + const directory = specTree({ alpha: CLEAN_SPEC, beta: CLEAN_SPEC }); + const realLog = console.log; + console.log = () => {}; + try { + assert.equal(main({ argv: [directory] }).ok, true); + } finally { + console.log = realLog; + rmSync(directory, { recursive: true, force: true }); + } +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7af7bf83..f23939d5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,3 +59,14 @@ jobs: - name: Test workflow scripts run: node --test .github/scripts/*.test.cjs + + # Repo-wide, not changed-files-only: spec rot accumulates in the specs a + # PR does not touch, so a scoped check would never surface it. + - name: Validate specs + run: pnpm openspec validate --all --strict + + # `--strict` validates what the parser read, not that it read the whole + # file. A second `##` inside `## Requirements` ends the section and every + # requirement below it becomes invisible — valid, unread, and green. + - name: Check spec requirement visibility + run: node .github/scripts/openspec-visibility.cjs diff --git a/openspec/specs/infrastructure/spec.md b/openspec/specs/infrastructure/spec.md index 0c3da89d..2a78f93d 100644 --- a/openspec/specs/infrastructure/spec.md +++ b/openspec/specs/infrastructure/spec.md @@ -149,15 +149,27 @@ A GitHub Actions workflow file SHALL exist at `.github/workflows/ci.yml`. - **WHEN** inspecting the repository - **THEN** `.github/workflows/ci.yml` SHALL exist and be valid YAML -### Requirement: Workflow triggers on PRs and main pushes +### Requirement: Workflow triggers on every pull request and main pushes -The workflow SHALL trigger on pull requests targeting the `main` branch and on pushes to the `main` branch. +The workflow SHALL trigger on **every** pull request regardless of its base branch, and on pushes to the `main` branch. The `pull_request` trigger SHALL carry no `branches:` filter, and SHALL name `ready_for_review` alongside the default event types. -#### Scenario: Pull request triggers workflow +Lint, typecheck, and tests have no interest in where a pull request eventually merges, and a `branches: [main]` filter did not reliably reach stacked pull requests: GitHub sometimes resolves a stacked PR's eventual target and sometimes does not, so the filter ran for the lower PRs of a stack and silently stopped for the upper ones — leaving a large change at "ready for review" having never been linted, typechecked, or tested. A filter that works for six PRs and quietly fails on the seventh is worse than one that never worked, because nobody re-checks it. `ready_for_review` is not in the default event set, so without it a draft marked ready gets no fresh run until someone happens to push again. + +#### Scenario: Pull request targeting main triggers workflow - **WHEN** a pull request is opened or updated targeting `main` - **THEN** the CI workflow SHALL run +#### Scenario: Stacked pull request triggers workflow + +- **WHEN** a pull request is opened or updated targeting a branch other than `main` +- **THEN** the CI workflow SHALL run, because the trigger carries no `branches:` filter + +#### Scenario: Draft marked ready triggers workflow + +- **WHEN** a draft pull request is marked ready for review with no new commits +- **THEN** the CI workflow SHALL run + #### Scenario: Push to main triggers workflow - **WHEN** a commit is pushed to `main` @@ -224,6 +236,39 @@ The workflow SHALL run `pnpm test` and the job SHALL fail if any tests fail. - **WHEN** a test suite has failures - **THEN** the test step SHALL fail and the workflow SHALL report failure +### Requirement: Workflow validates the specs + +The workflow SHALL validate `openspec/specs/` on every run, with two checks, and the job SHALL fail if either reports a problem. + +The first check runs `openspec validate --all --strict` across every spec, not only the specs a pull request touches: spec rot accumulates in the files nobody is editing, so a changed-files-only check would never surface it. + +The second check verifies that every `### Requirement:` heading sits under `## Requirements`. This is not redundant with the first. A second `##` heading inside the requirements section ends it, and every requirement below becomes prose to the parser — not invalid, but unread, so `--strict` reports success having never looked at them. Measured before the check existed, `infrastructure` carried 20 requirements with 1 visible and `skills` carried 7 with 1, both passing `--strict` the entire time. A passing gate is an active claim that the spec was read, which makes a silently truncated spec worse than a red check. Topical grouping inside a requirements section therefore uses a bold lead-in line rather than a heading. + +#### Scenario: A malformed spec fails the workflow + +- **WHEN** a spec under `openspec/specs/` does not satisfy `--strict` validation +- **THEN** the validation step SHALL fail and the workflow SHALL report failure + +#### Scenario: An untouched spec is still validated + +- **WHEN** a pull request changes no file under `openspec/specs/` but an existing spec is malformed +- **THEN** the validation step SHALL still fail, because validation is repo-wide + +#### Scenario: A truncated requirements section fails the workflow + +- **WHEN** a spec contains a `### Requirement:` heading that a `##` heading has placed outside the `## Requirements` section +- **THEN** the visibility step SHALL fail, naming the spec and the number of hidden requirements + +#### Scenario: A fenced requirement example does not fail the workflow + +- **WHEN** a spec contains a `### Requirement:` line inside a fenced code block as an illustration of the format +- **THEN** the visibility step SHALL NOT count it, because fenced content is documentation rather than a requirement the parser reads + +#### Scenario: An unclosed code fence fails the workflow + +- **WHEN** a spec reaches end of file with a code fence still open, as happens when an opening fence is lost and its closer is left dangling +- **THEN** the visibility step SHALL fail, because everything after that point is unreadable to the check as well as to the parser + ### Requirement: Workflow uses pnpm matching packageManager field The workflow SHALL install pnpm using a version consistent with the `packageManager` field in the root `package.json`. From 3e0d781faa1a509da607e98d8f2b1d397251b436 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 00:59:31 -0700 Subject: [PATCH 2/4] docs(openspec): correct the Node version the CI spec requires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- openspec/specs/infrastructure/spec.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openspec/specs/infrastructure/spec.md b/openspec/specs/infrastructure/spec.md index 2a78f93d..703178a9 100644 --- a/openspec/specs/infrastructure/spec.md +++ b/openspec/specs/infrastructure/spec.md @@ -278,14 +278,14 @@ The workflow SHALL install pnpm using a version consistent with the `packageMana - **WHEN** the workflow installs dependencies - **THEN** the pnpm version used SHALL match the version specified in `packageManager` -### Requirement: Workflow uses Node 22 +### Requirement: Workflow uses Node 24 -The workflow SHALL use Node.js version 22. +The workflow SHALL use Node.js version 24. -#### Scenario: Node version is 22 +#### Scenario: Node version is 24 - **WHEN** the workflow runs -- **THEN** Node.js 22 SHALL be the active runtime +- **THEN** Node.js 24 SHALL be the active runtime ### Requirement: Workflow does not publish From 3fd230108071d5ecf4956098217c74887a3eb154 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 11:57:02 -0700 Subject: [PATCH 3/4] fix(ci): match OpenSpec's own heading patterns in the visibility check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .github/scripts/openspec-visibility.cjs | 63 +++++++++--- .github/scripts/openspec-visibility.test.cjs | 101 +++++++++++++++---- 2 files changed, 130 insertions(+), 34 deletions(-) diff --git a/.github/scripts/openspec-visibility.cjs b/.github/scripts/openspec-visibility.cjs index 995f375b..29473701 100644 --- a/.github/scripts/openspec-visibility.cjs +++ b/.github/scripts/openspec-visibility.cjs @@ -48,8 +48,35 @@ const { join, relative, resolve } = require("node:path"); const REPO_ROOT = join(__dirname, "..", ".."); const DEFAULT_SPECS_DIRECTORY = join(REPO_ROOT, "openspec", "specs"); -const REQUIREMENTS_HEADING = "## Requirements"; -const REQUIREMENT_PREFIX = "### Requirement:"; +/** + * HEADING RECOGNITION MIRRORS OPENSPEC'S, DELIBERATELY. This check is only + * meaningful where it agrees with the parser it is speaking for, so these + * patterns are copied from OpenSpec's own: + * + * markdown-parser.js /^(#{1,6})\s+(.+)$/ section boundaries + * requirement-blocks.js /^##\s+Requirements\s*$/i the section itself + * /^##\s+/ what ends it + * /^###\s*Requirement:\s*(.+)\s*$/ + * + * Two consequences are worth stating because they look like bugs. + * + * NO LEADING-SPACE TOLERANCE, ON PURPOSE. CommonMark accepts up to three + * spaces before an ATX heading; OpenSpec does not — every one of its patterns + * is anchored at column 0. So an indented ` ## Grouping` is prose to the + * parser and does NOT end the requirements section. Widening this to `^ {0,3}##` + * would make the check stricter than the thing it checks and report hidden + * requirements that are, in fact, read — the opposite of the failure it exists + * to catch. `fenceOf` tolerates leading spaces because CommonMark's fence rule + * is what governs fences here; headings are governed by OpenSpec's rule. + * + * WHITESPACE AFTER THE HASHES IS TOLERATED, ALSO ON PURPOSE. OpenSpec matches + * `\s`, not a literal space, so `##\tGrouping` really does end the section and + * `### Requirement:` really is a requirement. Matching only a single space + * would miss both — a hidden requirement reported visible. + */ +const REQUIREMENTS_HEADING = /^##\s+Requirements\s*$/i; +const SECTION_HEADING = /^##\s/; +const REQUIREMENT_HEADING = /^###\s*Requirement:/; /** * A fence opens on ``` or ~~~ (up to three leading spaces, per CommonMark) and @@ -90,9 +117,9 @@ function countRequirements(source) { continue; } - if (line.startsWith("## ")) { - inRequirements = line.trim() === REQUIREMENTS_HEADING; - } else if (line.startsWith(REQUIREMENT_PREFIX)) { + if (SECTION_HEADING.test(line)) { + inRequirements = REQUIREMENTS_HEADING.test(line); + } else if (REQUIREMENT_HEADING.test(line)) { total += 1; if (inRequirements) { visible += 1; @@ -133,14 +160,24 @@ function checkSpecs(specsDirectory = DEFAULT_SPECS_DIRECTORY) { })); } -function main({ argv = process.argv.slice(2) } = {}) { +/** + * `log`/`error` are injectable so tests can assert on the report without + * reassigning the global console — a mutation that is only safe while + * `node --test` runs this file's tests sequentially, and would start losing or + * crossing output the day anything here gains concurrency. + */ +function main({ + argv = process.argv.slice(2), + log = console.log, + error = console.error, +} = {}) { const specsDirectory = argv[0] ? resolve(process.cwd(), argv[0]) : DEFAULT_SPECS_DIRECTORY; const results = checkSpecs(specsDirectory); if (results.length === 0) { - console.error(`No specs found under ${specsDirectory}`); + error(`No specs found under ${specsDirectory}`); return { results, ok: false }; } @@ -152,7 +189,7 @@ function main({ argv = process.argv.slice(2) } = {}) { : result.unclosedFence ? "UNCLOSED" : "ok "; - console.log( + log( ` ${status} ${name} ${result.visible}/${result.total} requirement(s) visible` ); } @@ -161,28 +198,28 @@ function main({ argv = process.argv.slice(2) } = {}) { (result) => result.hidden > 0 || result.unclosedFence ); if (broken.length === 0) { - console.log( + log( `\nEvery requirement in ${results.length} spec(s) is visible to the parser.` ); return { results, ok: true }; } - console.error(""); + error(""); for (const result of broken) { const name = displayPath(result.path); if (result.hidden > 0) { - console.error( + error( `${name}: ${result.hidden} requirement(s) hidden by a '##' heading inside '## Requirements' (${result.visible} of ${result.total} visible)` ); } if (result.unclosedFence) { - console.error( + error( `${name}: a code fence is still open at end of file, so everything after it is unreadable to this check and to the parser (likely a lost opening fence leaving its closer dangling)` ); } } if (broken.some((result) => result.hidden > 0)) { - console.error( + error( "\nA '##' heading ends the requirements section. Use a bold lead-in line for topical grouping instead, so the requirements below it stay readable to the parser." ); } diff --git a/.github/scripts/openspec-visibility.test.cjs b/.github/scripts/openspec-visibility.test.cjs index 43400542..5297435d 100644 --- a/.github/scripts/openspec-visibility.test.cjs +++ b/.github/scripts/openspec-visibility.test.cjs @@ -75,6 +75,66 @@ test("a stray '##' heading hides every requirement below it", () => { }); }); +test("a tab after '##' ends the section, matching OpenSpec's '\\s'", () => { + // OpenSpec matches /^##\s+/, not a literal "## ", so `##\tGrouping` is a real + // heading that truncates the section. Requiring a space here would report + // those requirements visible while the parser never reads them. + const tabHeading = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "##\tGrouping\n\n### Requirement: Second thing" + ); + assert.deepEqual(countRequirements(tabHeading), { + total: 2, + visible: 1, + hidden: 1, + unclosedFence: false, + }); +}); + +test("an indented '##' is prose, not a section boundary", () => { + // CommonMark allows up to three spaces before an ATX heading; OpenSpec's + // parsers are all anchored at column 0 and do not. Widening this check to + // /^ {0,3}##/ would report a requirement hidden that the parser reads fine. + const indentedHeading = CLEAN_SPEC.replace( + "### Requirement: Second thing", + " ## Grouping\n\n### Requirement: Second thing" + ); + assert.deepEqual(countRequirements(indentedHeading), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("'### Requirement:' spacing is as loose as OpenSpec's", () => { + // REQUIREMENT_HEADER_REGEX is /^###\s*Requirement:/ — no space and several + // spaces both parse, so both must count toward the total. + const looseSpacing = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "###Requirement: Second thing" + ).replace("### Requirement: First thing", "### Requirement: First thing"); + assert.deepEqual(countRequirements(looseSpacing), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + +test("the requirements heading is matched case-insensitively", () => { + // findSection compares titles case-insensitively and extractRequirementsSection + // carries the /i flag, so `## requirements` is the real section — treating it + // as a different heading would report every requirement hidden. + const lowercase = CLEAN_SPEC.replace("## Requirements", "## requirements"); + assert.deepEqual(countRequirements(lowercase), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + test("a bold lead-in line groups topics without hiding anything", () => { // The sanctioned alternative to a `##` heading, so it must stay clean. const grouped = CLEAN_SPEC.replace( @@ -151,20 +211,14 @@ test("main fails and explains an unclosed fence", () => { ["## Goal", "```", "", "### Requirement: Second thing"].join("\n") ); const directory = specTree({ gamma: lostOpener }); - 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); + const { ok, errors } = run(directory); + assert.equal(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 }); } }); @@ -193,6 +247,22 @@ test("a tilde fence is honoured, and a longer fence contains a shorter one", () }); }); +/** + * Drive main() over a directory, collecting what it would have printed. + * The injected logger keeps these assertions off the global console, so no test + * here depends on running sequentially with the others. + */ +function run(directory) { + const logs = []; + const errors = []; + const { ok, results } = main({ + argv: [directory], + log: (message) => logs.push(String(message)), + error: (message) => errors.push(String(message)), + }); + return { ok, results, logs, errors }; +} + /** Build a temp `specs//spec.md` tree and return its directory. */ function specTree(specsByName) { const directory = mkdtempSync(join(tmpdir(), "openspec-visibility-")); @@ -219,34 +289,23 @@ test("checkSpecs reports one row per spec directory", () => { test("main fails and names the offending spec and count", () => { const directory = specTree({ alpha: CLEAN_SPEC, beta: TRUNCATED_SPEC }); - const errors = []; - const logs = []; - const realError = console.error; - const realLog = console.log; - console.error = (message) => errors.push(String(message)); - console.log = (message) => logs.push(String(message)); try { - const { ok } = main({ argv: [directory] }); + const { ok, errors } = run(directory); assert.equal(ok, false); const reported = errors.join("\n"); assert.match(reported, /beta[/\\]spec\.md/); assert.match(reported, /1 requirement\(s\) hidden/); assert.doesNotMatch(reported, /alpha/); } finally { - console.error = realError; - console.log = realLog; rmSync(directory, { recursive: true, force: true }); } }); test("main passes when every spec is clean", () => { const directory = specTree({ alpha: CLEAN_SPEC, beta: CLEAN_SPEC }); - const realLog = console.log; - console.log = () => {}; try { - assert.equal(main({ argv: [directory] }).ok, true); + assert.equal(run(directory).ok, true); } finally { - console.log = realLog; rmSync(directory, { recursive: true, force: true }); } }); From 71f4a5798ef6f7623441db90d52328e7d90afa6c Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 19:42:25 -0700 Subject: [PATCH 4/4] fix(ci): end the requirements section on a '#' heading, as the parser does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visibility check ends `## Requirements` on `/^##\s/`, but the parser `openspec validate` actually runs — MarkdownParser — ends a section on ANY heading of level <= its own, so a level-1 `# Something` truncates it too. Verified directly: MarkdownParser reads 1 requirement from a spec where the check reported 2/2 visible. That is the silent truncation this gate exists to catch, reproduced inside the checker. Also corrects the comment's parser citation. It credited requirement-blocks.js's `/^###\s*Requirement:\s*(.+)\s*$/`, which serves the delta/edit path, not validation. The validating parser promotes every `###` child of the section regardless of title, so a title-less `### Requirement:` is a requirement to it — requiring `\s*\S` would stop counting a heading the parser does read. Both behaviours are now pinned by tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .github/scripts/openspec-visibility.cjs | 40 ++++++++++++++------ .github/scripts/openspec-visibility.test.cjs | 34 +++++++++++++++++ 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/.github/scripts/openspec-visibility.cjs b/.github/scripts/openspec-visibility.cjs index 29473701..9b22d15a 100644 --- a/.github/scripts/openspec-visibility.cjs +++ b/.github/scripts/openspec-visibility.cjs @@ -50,15 +50,28 @@ const DEFAULT_SPECS_DIRECTORY = join(REPO_ROOT, "openspec", "specs"); /** * HEADING RECOGNITION MIRRORS OPENSPEC'S, DELIBERATELY. This check is only - * meaningful where it agrees with the parser it is speaking for, so these - * patterns are copied from OpenSpec's own: + * meaningful where it agrees with the parser it is speaking for — and the + * parser it speaks for is the one `openspec validate` runs: `MarkdownParser` + * (core/validation/validator.js constructs it, core/parsers/markdown-parser.js + * defines it). Its rules are: * - * markdown-parser.js /^(#{1,6})\s+(.+)$/ section boundaries - * requirement-blocks.js /^##\s+Requirements\s*$/i the section itself - * /^##\s+/ what ends it - * /^###\s*Requirement:\s*(.+)\s*$/ + * /^(#{1,6})\s+(.+)$/ what counts as a heading at all + * findSection(..., 'Requirements') the section, matched case-insensitively + * getContentUntilNextHeader(i, 2) any heading of level <= 2 ends it + * parseRequirements(section) EVERY `###` child is a requirement * - * Two consequences are worth stating because they look like bugs. + * `requirement-blocks.js` carries a second, stricter set of patterns + * (`/^###\s*Requirement:\s*(.+)\s*$/` and friends). Those serve the delta/edit + * path, not validation, and where the two disagree this check follows the + * validating parser. Three consequences are worth stating because they look + * like bugs. + * + * A `#` HEADING ENDS THE SECTION TOO. `getContentUntilNextHeader` breaks on any + * heading whose level is <= the section's, so inside `## Requirements` a + * level-1 `# Something` truncates it exactly as a `##` does. Matching only + * `^##\s` here would report every requirement below such a heading as visible + * while the parser never reads one of them — the silent truncation this file + * exists to catch, reproduced in the checker. * * NO LEADING-SPACE TOLERANCE, ON PURPOSE. CommonMark accepts up to three * spaces before an ATX heading; OpenSpec does not — every one of its patterns @@ -69,13 +82,16 @@ const DEFAULT_SPECS_DIRECTORY = join(REPO_ROOT, "openspec", "specs"); * to catch. `fenceOf` tolerates leading spaces because CommonMark's fence rule * is what governs fences here; headings are governed by OpenSpec's rule. * - * WHITESPACE AFTER THE HASHES IS TOLERATED, ALSO ON PURPOSE. OpenSpec matches - * `\s`, not a literal space, so `##\tGrouping` really does end the section and - * `### Requirement:` really is a requirement. Matching only a single space - * would miss both — a hidden requirement reported visible. + * WHITESPACE AFTER THE HASHES IS TOLERATED, AND SO IS A MISSING TITLE. OpenSpec + * matches `\s`, not a literal space, so `##\tGrouping` really does end the + * section and `### Requirement:` really is a requirement. And because + * `parseRequirements` promotes every `###` child of the section, a title-less + * `### Requirement:` is a requirement to the validating parser as well — + * demanding a non-empty title (`\s*\S`) would stop counting a heading the + * parser does read. */ const REQUIREMENTS_HEADING = /^##\s+Requirements\s*$/i; -const SECTION_HEADING = /^##\s/; +const SECTION_HEADING = /^#{1,2}\s/; const REQUIREMENT_HEADING = /^###\s*Requirement:/; /** diff --git a/.github/scripts/openspec-visibility.test.cjs b/.github/scripts/openspec-visibility.test.cjs index 5297435d..1d1c7f20 100644 --- a/.github/scripts/openspec-visibility.test.cjs +++ b/.github/scripts/openspec-visibility.test.cjs @@ -91,6 +91,40 @@ test("a tab after '##' ends the section, matching OpenSpec's '\\s'", () => { }); }); +test("a '#' heading ends the section, matching the parser's level rule", () => { + // getContentUntilNextHeader(startLine, 2) breaks on any heading of level <= 2, + // so a level-1 heading truncates `## Requirements` exactly as a `##` does. + // Verified against MarkdownParser: it reads 1 requirement from this source. + const levelOneHeading = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "# Interlude\n\n### Requirement: Second thing" + ); + assert.deepEqual(countRequirements(levelOneHeading), { + total: 2, + visible: 1, + hidden: 1, + unclosedFence: false, + }); +}); + +test("a title-less '### Requirement:' still counts", () => { + // parseRequirements promotes every `###` child of the section regardless of + // its title, so MarkdownParser reads this as a requirement (verified). The + // stricter /^###\s*Requirement:\s*(.+)\s*$/ in requirement-blocks.js governs + // the delta/edit path, not validation — following it here would stop counting + // a heading the validating parser does read. + const titleless = CLEAN_SPEC.replace( + "### Requirement: Second thing", + "### Requirement:" + ); + assert.deepEqual(countRequirements(titleless), { + total: 2, + visible: 2, + hidden: 0, + unclosedFence: false, + }); +}); + test("an indented '##' is prose, not a section boundary", () => { // CommonMark allows up to three spaces before an ATX heading; OpenSpec's // parsers are all anchored at column 0 and do not. Widening this check to