diff --git a/.github/workflows/quality-resolve-probe.yml b/.github/workflows/quality-resolve-probe.yml index 35bdbe1..af44718 100644 --- a/.github/workflows/quality-resolve-probe.yml +++ b/.github/workflows/quality-resolve-probe.yml @@ -96,6 +96,76 @@ jobs: - name: "Assert every verdict-producing job is in Quality Report's needs" run: python3 scripts/assert-quality-report-gates-every-leg.py .github/workflows/quality.yml + # ── THE OTHER DIRECTION (#194) ──────────────────────────────────────── + # + # The two checks above close direction 1: a job's failure must be able to + # reach the required check. They say nothing about direction 2: whether + # the job RUNS AT ALL. A job deleted by an upstream result is `skipped`, + # `skipped` is not `failure`, and `contains(needs.*.result, 'failure')` + # reads straight past it — so a job can be perfectly wired into the gate + # and still contribute nothing, silently. That is #194: four test jobs + # carried `needs.security.result != 'failure'`, and one advisory against + # a dev-only formatter deleted PHPUnit, Newman and E2E in sixteen repos + # at once without turning anything red. + # + # Positive control first, same discipline as above. + - name: "Positive control — a producer that deletes a verdict must be detected" + run: python3 scripts/assert-no-producer-deletes-a-verdict.py --positive-control .github/workflows/quality.yml + + - name: "Assert no job can be DELETED by a producer's result" + run: python3 scripts/assert-no-producer-deletes-a-verdict.py .github/workflows/quality.yml + + coverage-gate-can-fail: + name: "The spec-coverage threshold can fail a run" + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + + # #189: `playwright-coverage-threshold` had NEVER gated. Below-threshold + # emitted `::warning::` and returned zero, so the knob was decorative — + # pipelinq declared 75, its run printed 28%, and the run passed. + # + # POSITIVE CONTROL FIRST, and it is the whole point here: the suite is + # re-run with the gate neutered back to warning-only, and MUST go red. A + # suite that stays green against the known-bad program is not measuring + # enforcement, which is exactly how the defect survived this long. + - name: "Positive control — a warning-only gate must fail this suite" + run: python3 scripts/test-spec-coverage-gate.py --positive-control .github/workflows/quality.yml + + # THE MEASUREMENT. The Node program is EXTRACTED from quality.yml's + # heredoc and executed against fixtures — below-threshold (must exit + # non-zero), at-threshold (must exit zero), zero-scenarios (must be NOT + # MEASURABLE rather than the old 100%), and a fixture proving ten + # unrelated `test()` calls no longer move the number. + # + # Running the shipped text rather than a transcription is deliberate: a + # test against a copy passes happily while the workflow does something + # else. + - name: "Exercise the shipped spec-coverage program against fixtures" + run: python3 scripts/test-spec-coverage-gate.py .github/workflows/quality.yml + + # MUTATION BATTERY. A green suite proves nothing on its own — the question + # is whether it would have NOTICED. Each mutant reintroduces one specific + # defect (enforcement removed, threshold that never fires, zero scenarios + # scoring 100, exclusions folded back into the denominator, …) and the + # suite must go red for every one. A mutant that SURVIVES means the + # fixture cannot reach that branch, and the fix is a better fixture. + # + # This is not decorative: it caught a real hole on its first run. The + # obvious fixture for `@e2e exclude` in a test file could not tell the + # guarded regex from the unguarded one — both capture `exclude`, which + # resolves to no slug — so that assertion passed while proving nothing. + # The separator forms (`exclude::`, `exclude#`) are where the + # guard is load-bearing, and the fixture now uses them. + # + # The last mutant is an ANTI-WIDENING control: it reworks a log string + # nothing asserts on, and the suite must STAY GREEN. Without it, a suite + # that failed on any edit at all would score a perfect kill rate while + # being worthless. + - name: "Mutation battery — every known defect must be caught" + run: python3 scripts/test-spec-coverage-gate.py --mutation-battery .github/workflows/quality.yml + seed-semantics: name: "A failing seed fails the job" runs-on: ubuntu-latest diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 985a2ed..2a39047 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -138,15 +138,47 @@ on: type: string default: "" enable-playwright-coverage: - description: "Collect V8 code coverage during Playwright tests and enforce threshold" + description: "Measure spec-to-test scenario coverage after the Playwright suite (and set COLLECT_COVERAGE for the suite itself). Produces playwright-coverage.json and enforces playwright-coverage-threshold." required: false type: boolean default: false playwright-coverage-threshold: - description: "Minimum line coverage percentage for Playwright tests (0-100). Only enforced when enable-playwright-coverage is true." + # #189. Three things were wrong with this knob and all three are fixed + # in the step that reads it ("Generate spec-to-test coverage report"): + # + # 1. It never enforced. Below-threshold emitted `::warning::` and the + # job stayed green, so the value was decorative. pipelinq declared + # 75, its run printed 28%, and the run passed. It now `::error::`s + # and exits non-zero. + # 2. The number was not coverage. It was + # `count(test() calls) / count(scenario headings)` — two + # independent totals, never compared to each other. Ten unrelated + # tests raised it exactly as much as covering ten scenarios, and it + # could exceed 100% while covering nothing. It is now real + # per-scenario matching against `@e2e` references, using gate-19's + # dialect. The old ratio is still reported, under the honest name + # `testsPerScenarioPercent`, and is not gated on. + # 3. Zero scenarios scored 100% — the worst-covered repo and the best + # produced the same number. Zero enforceable scenarios is now a + # FAILURE TO MEASURE and fails. + # + # The description below is also corrected. It used to read "Only + # enforced when enable-playwright-coverage is true", which told a reader + # that setting the flag switched enforcement on. It did not. + # + # DEFAULT CHANGED 75 -> 0, deliberately, to bound the blast radius of + # making a dead gate live. Measured 2026-08-08 across all 31 fleet + # callers: exactly ONE repo (pipelinq) sets `enable-playwright-coverage: + # true`, and it also sets its own threshold explicitly. Every other + # caller leaves the flag false, so the step does not run at all. A + # default of 0 means a repo that switches measurement on gets the + # NUMBER without being turned red by a floor it never chose; gating is + # opt-in by setting a value, which is what a threshold input should + # mean. + description: "Minimum percentage of non-excluded openspec scenarios that must be referenced by an @e2e annotation in a Playwright test (0-100). ENFORCED — below this the Playwright job fails. Only evaluated when enable-playwright-coverage is true. Default 0 means measure and report, do not gate." required: false type: number - default: 75 + default: 0 enable-axe: # Same shape and the same reasoning as enable-hydra-gates below: a # control that goes red on inherited debt the moment it is switched on @@ -984,6 +1016,57 @@ jobs: path: quality-results/ # ── Group 3: Security ────────────────────────── + # + # ══ THIS JOB NO LONGER GATES THE TEST TIER (#194) ═════════════════════════ + # + # `phpunit`, `newman`, `playwright` and `journeydoc-capture` each used to + # carry `&& needs.security.result != 'failure'` in their `if:`. That single + # clause answered TWO different questions with one conditional: + # + # 1. should a security failure block the MERGE? — yes, and it still does + # 2. should a security failure DELETE the test evidence? — no, and it did + # + # WHAT HAPPENED, 2026-08-06. CVE-2026-67434 was published against + # `squizlabs/php_codesniffer <3.13.6`. `composer audit` queries the LIVE + # Packagist advisory feed on every run, so the moment the advisory began + # being served, `Security (composer)` went red in every repo in the fleet + # simultaneously — with no commit, no push and no code change anywhere. The + # clause above then turned PHPUnit, Newman and E2E into `skipped` across 16 + # repositories at once. + # + # A SKIPPED JOB RENDERS AS A GREY TICK, NOT A RED X. Dashboards, `gh pr + # checks` tallies and any count of failures read zero and report success. The + # runs did not look broken; the tests simply had not run. It fooled a review + # into filing a "fully green" report over a fleet with no test coverage at + # all. + # + # The coupling was never justified by the risk it managed. The dependency in + # question is a CODE FORMATTER that never executes in production. There is no + # reading of "phpcs has a command-injection bug" that implies "we can no + # longer trust what PHPUnit reports". And the shape inverts the purpose of + # CI: at the exact moment a security problem appears — when you most want to + # know whether the code still works — it deleted the evidence that would tell + # you. + # + # WHAT REPLACES IT (option E of #194 — decouple, and render loudly): + # + # * The test jobs no longer read `needs.security.result`. They always run + # and always produce a verdict. `needs: [security]` is kept on some of + # them for ORDERING only — every one of them carries `!cancelled()`, + # which suppresses the implicit `success()` on `needs`. + # + # * Security still blocks the merge, unchanged: this job is in the Quality + # Report's `needs:`, and `quality / Quality Report` is a required context + # on `main` and `beta` in all 25 fleet repos. NOTHING IS WEAKENED AT THE + # MERGE GATE. The cost is a few CI minutes spent on a PR that could not + # have merged anyway. + # + # * The Quality Report gained a third gate — "a test-tier job that was + # ENABLED but did not run" — so that if any future condition deletes the + # test tier again, the report says so in words and fails, instead of + # tallying the absence of a verdict as a pass. See that step for detail. + # + # ══════════════════════════════════════════════════════════════════════════ security: runs-on: ubuntu-latest name: "Security (${{ matrix.ecosystem }})" @@ -1455,13 +1538,24 @@ jobs: # suppressing 73 real findings: whether the test suite ran at all was # contingent on a suppression file. # - # The security gate is kept — a repo with a known-vulnerable dependency - # should not spin up servers. This also aligns PHPUnit with the playwright - # job below, which already deliberately declines to gate on php-quality. + # NOT gated on security either, as of #194 — and that clause used to be + # here, justified as "a repo with a known-vulnerable dependency should not + # spin up servers". The justification did not survive contact with a real + # advisory. See the block on the `security` job for the full argument and + # the 2026-08-06 incident; the short version is that `composer audit` + # queries the LIVE Packagist feed, so an advisory published against any + # shared dev dependency deleted the entire fleet's test tier at once, with + # no commit and no red. A skipped job is a grey tick, not a failure. + # + # Security still blocks the MERGE: `security` is in the Quality Report's + # `needs:`, and `quality / Quality Report` is the required context on main + # and beta in all 25 fleet repos. Decoupling costs a few CI minutes on a PR + # that cannot merge anyway; it buys a verdict that exists. # # `needs:` is retained for ordering only; with `!cancelled()` and no result - # condition on php-quality, PHPUnit runs regardless of static-analysis colour. - if: ${{ inputs.enable-php && inputs.enable-phpunit && !cancelled() && needs.security.result != 'failure' }} + # condition on either producer, PHPUnit runs regardless of static-analysis + # colour and regardless of the advisory feed. + if: ${{ inputs.enable-php && inputs.enable-phpunit && !cancelled() }} runs-on: ubuntu-latest name: "PHPUnit (PHP ${{ matrix.php-version }}, NC ${{ matrix.nextcloud-ref }})" needs: [php-quality, security] @@ -1711,13 +1805,20 @@ jobs: # left behind. playwright already declines to gate on php-quality too, so # this brings the last server-spinning job into line with the other two. # - # The security gate is KEPT, matching phpunit and playwright: a repo with a - # known-vulnerable dependency should not spin up servers. + # The security gate is NOT kept, as of #194 — it was removed from phpunit, + # playwright and journeydoc-capture at the same time, for the same reason. + # See the `security` job for the argument and the 2026-08-06 incident. + # Short version: the clause below used to read `&& needs.security.result != + # 'failure'`, and one advisory against one shared DEV dependency turned + # this job into `skipped` in every repo at once. That is the identical + # defect this comment block already describes for php-quality, arriving + # through a second door. # # `needs:` is retained for ordering only; with `!cancelled()` and no result - # condition on php-quality, Newman now runs regardless of static-analysis - # colour and reaches its own verdict. - if: ${{ inputs.enable-newman && !cancelled() && needs.security.result != 'failure' }} + # condition on either producer, Newman now runs regardless of + # static-analysis colour, regardless of the advisory feed, and reaches its + # own verdict. + if: ${{ inputs.enable-newman && !cancelled() }} runs-on: ubuntu-latest name: "Integration Tests (Newman)" needs: [php-quality, security] @@ -2007,7 +2108,11 @@ jobs: tail -50 server/data/nextcloud.log 2>/dev/null | python3 -m json.tool --no-ensure-ascii 2>/dev/null || tail -50 server/data/nextcloud.log 2>/dev/null || echo "No log found" playwright: - if: ${{ inputs.enable-playwright && !cancelled() && needs.security.result != 'failure' }} + # `needs.security.result != 'failure'` was removed here by #194. E2E is the + # most expensive evidence this workflow produces and it was the easiest to + # delete: one advisory against a dev-only formatter took it out fleet-wide + # on 2026-08-06 and rendered as a grey tick. See the `security` job. + if: ${{ inputs.enable-playwright && !cancelled() }} runs-on: ubuntu-latest name: "E2E Tests (Playwright)" # `frontend-build` is a dependency purely so its artifact is available to @@ -2650,80 +2755,303 @@ jobs: retention-days: 14 if-no-files-found: error + # ══ Spec-to-test coverage (#189) ═══════════════════════════════════════ + # + # This step used to compute, warn about, and ignore a number that was not + # coverage. All three halves of that are fixed here; the argument is + # recorded on the `playwright-coverage-threshold` input at the top of the + # file, and the short version is: + # + # * `::warning::` with no non-zero exit -> the threshold could not fail + # anything, ever. It now `::error::`s and exits 1. + # * `tests.length / scenarios.length` is a ratio of two totals that are + # never compared to each other. It is replaced by real per-scenario + # matching. The old ratio survives as `testsPerScenarioPercent`, + # reported and never gated on, so nobody loses a number they were + # watching. + # * `scenarios.length === 0 ? … : 100` scored a repo with no parseable + # scenarios at 100%. Zero enforceable scenarios is now NOT MEASURABLE + # and fails, because a measurement that could not be taken is not a + # pass. + # + # WHAT "COVERED" MEANS NOW. A scenario is covered when some Playwright + # test file carries an `@e2e` reference that resolves to its slug — + # `@e2e ::` or `@e2e openspec/specs//spec.md#` + # — and a scenario carrying `@e2e exclude ` (at scenario, + # requirement or whole-spec level) leaves the denominator entirely. That + # is deliberately the SAME dialect as hydra-gates gate-19 + # (`check_e2e_coverage.py`), so a repo annotates once and both numbers + # move together. The division of labour: gate-19 is DIFF-SCOPED and blocks + # a PR that adds an unannotated scenario; this is the WHOLE-REPO figure + # and blocks on the standing floor. gate-19 is not invoked from here on + # purpose — this job has no hydra-gates checkout, and adding one would + # make the E2E job depend on a second repository to report a number. + # + # KNOWN GAP, stated here rather than discovered later. gate-19 was + # rewritten in #249 to read test files with a real JS parser, so it can + # tell that an `@e2e` reference sits inside a `describe.skip` or an empty + # test body and refuse to count it. This step reads the annotation as + # TEXT, so it counts such a reference as covered. That makes this number + # an UPPER BOUND on real coverage, and the two figures can legitimately + # disagree in that one direction. It is a deliberate trade: closing the + # gap means shipping a JS parser into this heredoc, and gate-19 already + # catches the case on every PR that touches a spec. DO NOT read a passing + # threshold here as evidence that gate-19 would also pass. + # + # WHY A HEREDOC RATHER THAN `node -e`. The program is now long enough to + # need a test, and a `node -e` string with `${{ }}` interpolated into its + # middle cannot be executed anywhere except inside a GitHub runner. As a + # file fed only by environment variables it is a plain Node program, and + # `scripts/test-spec-coverage-gate.py` extracts THIS text out of THIS file + # and runs it against fixtures — including a fixture that proves the gate + # can fail, which #189 correctly points out no run could previously show. - name: Generate spec-to-test coverage report if: ${{ inputs.enable-playwright-coverage && !cancelled() }} + env: + SPEC_COVERAGE_THRESHOLD: ${{ inputs.playwright-coverage-threshold }} + PLAYWRIGHT_TEST_PATH: ${{ inputs.playwright-test-path }} run: | cd server/apps/${{ inputs.app-name }} - node -e " - const fs = require('fs'); - const path = require('path'); - - // Collect all spec scenarios from openspec - const specDir = 'openspec/specs'; - const changeDir = 'openspec/changes'; - let scenarios = []; - - function extractScenarios(dir) { - if (!fs.existsSync(dir)) return; - for (const entry of fs.readdirSync(dir, { recursive: true })) { - const file = path.join(dir, entry); - if (!file.endsWith('spec.md') && !file.endsWith('spec.md')) continue; - if (!fs.statSync(file).isFile()) continue; - const content = fs.readFileSync(file, 'utf8'); - const matches = content.match(/^###?\s+(S\d+|Scenario[:\s]|REQ-)[^\n]*/gm) || []; - for (const m of matches) { - scenarios.push({ file: file.replace(/\\\\/g, '/'), scenario: m.replace(/^#+\s+/, '').trim() }); - } + cat > /tmp/spec-coverage.js <<'SPEC_COVERAGE_JS' + // Spec-to-test coverage — real per-scenario matching. + // + // Env: SPEC_COVERAGE_THRESHOLD, PLAYWRIGHT_TEST_PATH, SPEC_COVERAGE_OUT + // Exit: 0 measured and at/above threshold; 1 below threshold, or the + // measurement could not be taken at all. + 'use strict'; + + var fs = require('fs'); + var path = require('path'); + + var THRESHOLD = Number(process.env.SPEC_COVERAGE_THRESHOLD || '0'); + var TEST_DIR = process.env.PLAYWRIGHT_TEST_PATH || 'tests/e2e'; + var OUT = process.env.SPEC_COVERAGE_OUT || 'playwright-coverage.json'; + var SPEC_ROOT = 'openspec/specs'; + + function slugify(text) { + var t = String(text).toLowerCase(); + t = t.replace(/[^a-z0-9\s-]/g, ''); + t = t.replace(/[\s_]+/g, '-'); + t = t.replace(/-{2,}/g, '-'); + return t.replace(/^-+|-+$/g, ''); + } + + function walk(dir, acc) { + acc = acc || []; + if (!fs.existsSync(dir)) return acc; + var entries; + try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch (e) { return acc; } + for (var i = 0; i < entries.length; i++) { + var e = entries[i]; + var full = path.join(dir, e.name); + if (e.isDirectory()) walk(full, acc); + else if (e.isFile()) acc.push(full); } + return acc; + } + + var EXCLUDE_RE = /@e2e\s+exclude\b[ \t]*(.*?)\s*$/; + // A WHOLE-SPEC exclusion must be a standalone directive line (after + // optional markdown bullet/quote/heading markers), so prose that + // merely mentions the token does not silently void an entire spec. + var WHOLE_SPEC_EXCLUDE_RE = /^[ \t>*#-]*@e2e\s+exclude\b[ \t]*(.*?)\s*$/; + + function parseExclusion(line) { + var m = EXCLUDE_RE.exec(line); + if (!m) return null; + return { reason: m[1].trim() || null }; } - extractScenarios(specDir); - extractScenarios(changeDir); - - // Collect all test descriptions from Playwright specs - const testDir = '${{ inputs.playwright-test-path }}'; - let tests = []; - if (fs.existsSync(testDir)) { - for (const entry of fs.readdirSync(testDir, { recursive: true })) { - const file = path.join(testDir, entry); - if (!file.endsWith('.spec.ts') && !file.endsWith('.spec.js')) continue; - if (!fs.statSync(file).isFile()) continue; - const content = fs.readFileSync(file, 'utf8'); - const matches = content.match(/test\(['\x60]([^'\x60]+)/g) || []; - for (const m of matches) { - tests.push({ file: file.replace(/\\\\/g, '/'), test: m.replace(/test\(['\x60]/, '').trim() }); + + // Format A: `#### Scenario: ` + var SCENARIO_RE = /^#{4}\s+Scenario:\s*(.+)$/i; + // Format B parent: `### Requirement: …` or `### REQ-xxx: …` + var REQUIREMENT_RE = /^#{3}\s+(?:Requirement:|REQ-[A-Za-z0-9_-]+:)\s*(.*)$/; + var ALT_MARKER_RE = /^\*\*Scenarios?:\*\*\s*$/i; + var ALT_ITEM_RE = /^(\d+)\.\s+\*\*(?:GIVEN|WHEN)\b/i; + + function parseSpecFile(file) { + var specName = path.basename(path.dirname(file)); + var lines = fs.readFileSync(file, 'utf8').split(/\r?\n/); + var out = []; + + var wholeExcluded = false, wholeReason = null; + for (var h = 0; h < lines.length; h++) { + if (REQUIREMENT_RE.test(lines[h]) || SCENARIO_RE.test(lines[h])) break; + var wm = WHOLE_SPEC_EXCLUDE_RE.exec(lines[h]); + if (wm) { wholeExcluded = true; wholeReason = wm[1].trim() || null; break; } + } + + var reqSlug = null, reqExcluded = false, reqReason = null; + var i = 0; + while (i < lines.length) { + var line = lines[i]; + + var rm = REQUIREMENT_RE.exec(line); + if (rm) { + var reqText = rm[1].trim() || line.replace(/^#+\s+/, '').trim(); + reqSlug = slugify(reqText); + var rex = parseExclusion(line); + reqExcluded = !!rex; + reqReason = rex ? rex.reason : null; + for (var k = i + 1; k < lines.length && k <= i + 3; k++) { + if (REQUIREMENT_RE.test(lines[k]) || SCENARIO_RE.test(lines[k])) break; + var rex2 = parseExclusion(lines[k]); + if (rex2) { reqExcluded = true; reqReason = reqReason || rex2.reason; break; } + } + i++; + continue; + } + + var sm = SCENARIO_RE.exec(line); + if (sm) { + var title = sm[1].trim(); + var block = [line]; + var j = i + 1; + while (j < lines.length && !/^#{1,4}\s/.test(lines[j])) { block.push(lines[j]); j++; } + var sEx = null; + for (var b = 0; b < block.length; b++) { sEx = sEx || parseExclusion(block[b]); } + var excluded = wholeExcluded || reqExcluded || !!sEx; + var reason = wholeExcluded ? wholeReason : (sEx ? sEx.reason : (reqExcluded ? reqReason : null)); + out.push({ + spec: specName, file: file, label: 'Scenario: ' + title, + slug: slugify(title), excluded: excluded, reason: reason + }); + i = j; + continue; } + + if (ALT_MARKER_RE.test(line) && reqSlug) { + var j2 = i + 1; + while (j2 < lines.length && !/^#{1,4}\s/.test(lines[j2])) { + var am = ALT_ITEM_RE.exec(lines[j2]); + if (am) { + var itemEx = parseExclusion(lines[j2]); + var ex2 = wholeExcluded || reqExcluded || !!itemEx; + var rsn2 = wholeExcluded ? wholeReason : (itemEx ? itemEx.reason : (reqExcluded ? reqReason : null)); + out.push({ + spec: specName, file: file, label: reqSlug + ' scenario ' + am[1], + slug: reqSlug + '-scenario-' + am[1], excluded: ex2, reason: rsn2 + }); + } + j2++; + } + i = j2; + continue; + } + + i++; } + return out; } - // Collect test flow files - const flowDir = 'tests/flows'; - let flows = []; - if (fs.existsSync(flowDir)) { - for (const entry of fs.readdirSync(flowDir)) { - if (entry.endsWith('.md') && entry !== 'README.md') flows.push(entry); + // `@e2e <path>#<slug>` / `@e2e <spec>::<slug>`. `exclude` is a + // directive, not a reference, so it must not be read as one. + var REF_RE = /@e2e\s+(?!exclude\b)(\S+)/g; + + function parseTestFile(file) { + var content = fs.readFileSync(file, 'utf8'); + var refs = []; + var m; + REF_RE.lastIndex = 0; + while ((m = REF_RE.exec(content)) !== null) { + var raw = m[1]; + var slug = null; + if (raw.indexOf('#') !== -1) slug = raw.split('#').pop(); + else if (raw.indexOf('::') !== -1) slug = raw.split('::').pop(); + if (slug) refs.push(slugify(slug)); } + var titles = content.match(/test\(['"`]([^'"`]+)/g) || []; + return { refs: refs, tests: titles.length }; } - const report = { - specScenarios: scenarios.length, - playwrightTests: tests.length, - testFlows: flows.length, - coverage: scenarios.length > 0 ? Math.round((tests.length / scenarios.length) * 100) : 100, - scenarios, - tests, - flows, + var specFiles = walk(SPEC_ROOT).filter(function (f) { + var b = path.basename(f); + return b === 'spec.md' || b === 'specs.md'; + }); + + var scenarios = []; + for (var s = 0; s < specFiles.length; s++) { + scenarios = scenarios.concat(parseSpecFile(specFiles[s])); + } + + var testFiles = walk(TEST_DIR).filter(function (f) { + return /\.(spec|test)\.(ts|js)$/.test(f); + }); + + var referenced = Object.create(null); + var testCount = 0; + var refCount = 0; + for (var t = 0; t < testFiles.length; t++) { + var parsed = parseTestFile(testFiles[t]); + testCount += parsed.tests; + for (var r = 0; r < parsed.refs.length; r++) { + refCount++; + (referenced[parsed.refs[r]] = referenced[parsed.refs[r]] || []).push(testFiles[t]); + } + } + + var total = scenarios.length; + var excluded = 0, covered = 0; + var uncoveredList = []; + for (var x = 0; x < scenarios.length; x++) { + var sc = scenarios[x]; + if (sc.excluded) { excluded++; sc.status = 'excluded'; continue; } + if (referenced[sc.slug]) { covered++; sc.status = 'covered'; sc.coveredBy = referenced[sc.slug]; } + else { sc.status = 'uncovered'; uncoveredList.push(sc.spec + '#' + sc.slug); } + } + + var enforceable = total - excluded; + var measurable = enforceable > 0; + var coverage = measurable ? Math.round((covered / enforceable) * 100) : null; + + var report = { + metric: 'scenario-coverage', + metricDescription: 'percent of non-excluded openspec scenarios referenced by an @e2e annotation in a Playwright test', + specScenarios: total, + excludedScenarios: excluded, + enforceableScenarios: enforceable, + coveredScenarios: covered, + uncoveredScenarios: enforceable - covered, + playwrightTests: testCount, + e2eReferences: refCount, + // The OLD number, kept under a name that says what it is: two + // independent totals divided by each other. Reported, never gated. + testsPerScenarioPercent: total > 0 ? Math.round((testCount / total) * 100) : null, + measurable: measurable, + coverage: coverage, + threshold: THRESHOLD, + uncovered: uncoveredList.slice(0, 200), + scenarios: scenarios }; - fs.writeFileSync('playwright-coverage.json', JSON.stringify(report, null, 2)); - console.log('Spec scenarios: ' + report.specScenarios); - console.log('Playwright tests: ' + report.playwrightTests); - console.log('Test flows: ' + report.testFlows); - console.log('Spec-to-test coverage: ' + report.coverage + '%'); + fs.writeFileSync(OUT, JSON.stringify(report, null, 2)); + + console.log('Spec scenarios found: ' + total); + console.log(' @e2e-excluded: ' + excluded); + console.log(' enforceable: ' + enforceable); + console.log(' covered by an @e2e ref: ' + covered); + console.log('Playwright test() calls: ' + testCount + ' (reported, NOT gated on)'); + console.log('@e2e references found: ' + refCount); + console.log('tests-per-scenario: ' + report.testsPerScenarioPercent + '% (reported, NOT gated on)'); - if (report.coverage < ${{ inputs.playwright-coverage-threshold }}) { - console.log('::warning::Spec-to-test coverage ' + report.coverage + '% is below threshold ' + ${{ inputs.playwright-coverage-threshold }} + '%'); + if (!measurable) { + console.log('::error::NOT MEASURABLE - no enforceable scenarios found under ' + SPEC_ROOT + '. This is a FAILURE TO MEASURE, not 100% coverage: a repo with no parseable scenarios has proved nothing. Expected `#### Scenario: <title>` headings, or `### REQ-x:`/`### Requirement:` followed by `**Scenarios:**` and numbered `N. **GIVEN/WHEN**` items, in openspec/specs/*/spec.md.'); + process.exit(1); } - " + + console.log('Scenario coverage: ' + coverage + '% (threshold ' + THRESHOLD + '%)'); + + if (coverage < THRESHOLD) { + console.log('::error::Scenario coverage ' + coverage + '% is below the configured threshold of ' + THRESHOLD + '%. ' + (enforceable - covered) + ' of ' + enforceable + ' enforceable scenarios have no @e2e reference in any Playwright test.'); + var show = uncoveredList.slice(0, 25); + for (var u = 0; u < show.length; u++) console.log(' uncovered: ' + show[u]); + if (uncoveredList.length > show.length) console.log(' ... and ' + (uncoveredList.length - show.length) + ' more (full list in ' + OUT + ')'); + process.exit(1); + } + + console.log('Scenario coverage meets the threshold.'); + SPEC_COVERAGE_JS + node /tmp/spec-coverage.js - name: Upload Playwright report if: always() @@ -2800,7 +3128,11 @@ jobs: # ╚══════════════════════════════════════════════╝ journeydoc-capture: - if: ${{ inputs.enable-journeydoc-capture && !cancelled() && needs.security.result != 'failure' }} + # `needs.security.result != 'failure'` removed by #194, with the rest of the + # test tier. `needs: [security]` is kept for ordering only — `!cancelled()` + # suppresses the implicit `success()`, so this job reaches its own verdict + # whatever the advisory feed says. See the `security` job. + if: ${{ inputs.enable-journeydoc-capture && !cancelled() }} runs-on: ubuntu-latest name: "Journeydoc Capture (screenshots)" needs: [security] @@ -3827,6 +4159,26 @@ jobs: esac } + # Test-tier icon (#194). `skipped` means two completely different + # things depending on whether the caller asked for the job: + # + # enabled=false → the repo does not have this kind of test. ⏭️. + # enabled=true → the job was asked for and DID NOT RUN. That is + # the ABSENCE OF A VERDICT, and it must not share a + # glyph with "no tests configured" — which is + # exactly how a fleet-wide advisory deleted the test + # tier on 2026-08-06 while every report looked + # ordinary. Rendered in words, not a tick. + # + # $1 = needs.<job>.result, $2 = "true"/"false" for enabled + test_icon() { + if [ "$2" = "true" ] && [ "$1" = "skipped" ]; then + echo "🚨 **NO VERDICT — enabled but never ran**" + else + icon "$1" + fi + } + # Build license info strings license_info() { local eco="$1" @@ -3883,10 +4235,15 @@ jobs: sec_npm=$(read_result "results/result-security-npm/npm.txt") echo "| npm | | | $(icon "$sec_npm") | $(license_info "npm") | |" - # Test rows - echo "| PHPUnit | | | | | $(icon '${{ needs.phpunit.result }}') |" - echo "| Newman | | | | | $(icon '${{ needs.newman.result }}') |" - echo "| Playwright | | | | | $(icon '${{ needs.playwright.result }}') |" + # Test rows. `test_icon` rather than `icon`: see its definition — + # an enabled-but-skipped test job is the absence of a verdict and + # says so in words. + echo "| PHPUnit | | | | | $(test_icon '${{ needs.phpunit.result }}' '${{ inputs.enable-php && inputs.enable-phpunit }}') |" + echo "| Newman | | | | | $(test_icon '${{ needs.newman.result }}' '${{ inputs.enable-newman }}') |" + echo "| Playwright | | | | | $(test_icon '${{ needs.playwright.result }}' '${{ inputs.enable-playwright }}') |" + if [ '${{ inputs.enable-journeydoc-capture }}' = 'true' ]; then + echo "| Journeydoc capture | | | | | $(test_icon '${{ needs.journeydoc-capture.result }}' 'true') |" + fi # The Hydra gates were in this job's `needs:` and in its failure # gate below, so a red gates job did red the Quality Report — but # the report itself never mentioned them, in any state. The one @@ -3941,12 +4298,26 @@ jobs: fi # Playwright spec-to-test coverage (if available) + # #189: this used to render "<coverage>% (<tests> tests / <specs> + # specs)", which stated the broken model out loud — a ratio of two + # totals that were never compared. The producer now emits real + # per-scenario matching, so the numerator and denominator here are + # the same population and the sentence means what it says. `.coverage` + # is null when the measurement could not be taken, and that case says + # so rather than printing "null%". COVERAGE_FILE="results/playwright-report/playwright-coverage.json" if [ -f "$COVERAGE_FILE" ]; then - SPEC_COUNT=$(jq '.specScenarios' "$COVERAGE_FILE") - TEST_COUNT=$(jq '.playwrightTests' "$COVERAGE_FILE") - COVERAGE=$(jq '.coverage' "$COVERAGE_FILE") - echo "**Spec coverage:** ${COVERAGE}% ($TEST_COUNT tests / $SPEC_COUNT specs)" >> "$REPORT" + MEASURABLE=$(jq -r '.measurable // "unknown"' "$COVERAGE_FILE") + if [ "$MEASURABLE" = "false" ]; then + echo "**Spec coverage:** ⚠️ NOT MEASURABLE — no enforceable openspec scenarios were found. This is a failure to measure, not full coverage." >> "$REPORT" + else + COVERED=$(jq -r '.coveredScenarios // 0' "$COVERAGE_FILE") + ENFORCEABLE=$(jq -r '.enforceableScenarios // 0' "$COVERAGE_FILE") + EXCLUDED=$(jq -r '.excludedScenarios // 0' "$COVERAGE_FILE") + COVERAGE=$(jq -r '.coverage // 0' "$COVERAGE_FILE") + THRESH=$(jq -r '.threshold // 0' "$COVERAGE_FILE") + echo "**Spec coverage:** ${COVERAGE}% — $COVERED of $ENFORCEABLE enforceable scenarios carry an \`@e2e\` reference (threshold ${THRESH}%, $EXCLUDED excluded with a reason)." >> "$REPORT" + fi echo "" >> "$REPORT" fi @@ -4016,6 +4387,128 @@ jobs: echo "::error::One or more quality jobs failed — see the report above." exit 1 + # ══ Gate 2 — an ENABLED test job that never ran (#194) ═════════════════ + # + # The invariant, stated once: **`skipped` in the test tier must never be + # summarised as a pass.** `skipped` is not `failure`, so the gate above — + # `contains(needs.*.result, 'failure')` — reads straight past it, and the + # check renders in the same colour family as a success. On 2026-08-06 a + # single advisory against a dev-only formatter turned PHPUnit, Newman and + # E2E into `skipped` in 16 repositories at once and every one of those + # runs read as unremarkable. The absence of a verdict was indistinguishable + # from a good one, and that — not the security failure — is what cost us + # the morning. + # + # The cause of that particular skip is removed (the test jobs no longer + # read `needs.security.result`), but a cause is not an invariant. Any + # future `if:` clause, matrix collapse or producer edge can delete the + # same evidence the same way. This step is the invariant, so the next one + # is loud on arrival instead of being reconstructed from a timeline. + # + # WHY `enabled` IS PART OF THE TEST. `skipped` legitimately means "this + # repo has no Newman collections" for most of the fleet. The defect is + # only a defect when the CALLER ASKED FOR THE JOB and did not get it, so + # each row pairs `needs.<job>.result` with the input(s) that switch it on. + # That keeps the gate silent for the 20-odd repos that run no E2E at all. + # + # WHY THE SUPERSESSION CHECK IS REPEATED HERE. A run cancelled by + # `cancel-in-progress` can leave a not-yet-started job reporting `skipped` + # rather than `cancelled`. Failing on that would turn every rapid push + # red — the same wall of red the gate below exists to avoid — so this + # borrows its discriminator: a run whose head SHA is no longer the branch + # tip has been superseded, and the newer run is the one whose verdict + # counts. + - name: Gate — a test job that was ENABLED but never ran has no verdict + if: ${{ !cancelled() }} + env: + GH_TOKEN: ${{ github.token }} + RUN_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + RUN_REF: ${{ github.head_ref || github.ref_name }} + SECURITY_RESULT: ${{ needs.security.result }} + PHPUNIT_ENABLED: ${{ inputs.enable-php && inputs.enable-phpunit }} + PHPUNIT_RESULT: ${{ needs.phpunit.result }} + NEWMAN_ENABLED: ${{ inputs.enable-newman }} + NEWMAN_RESULT: ${{ needs.newman.result }} + PLAYWRIGHT_ENABLED: ${{ inputs.enable-playwright }} + PLAYWRIGHT_RESULT: ${{ needs.playwright.result }} + JOURNEYDOC_ENABLED: ${{ inputs.enable-journeydoc-capture }} + JOURNEYDOC_RESULT: ${{ needs['journeydoc-capture'].result }} + run: | + set -uo pipefail + + MISSING="" + RAN="" + + # $1 = human label, $2 = enabled ("true"/"false"), $3 = job result + classify() { + if [ "$2" != "true" ]; then + echo " - $1: not enabled by this caller — nothing expected." + return + fi + case "$3" in + skipped) + echo " - $1: ENABLED but result=skipped — NO VERDICT EXISTS." + MISSING="${MISSING}${1}"$'\n' + ;; + success) + echo " - $1: ran, passed." + RAN="${RAN}${1}"$'\n' + ;; + *) + echo " - $1: ran, result=$3." + RAN="${RAN}${1}"$'\n' + ;; + esac + } + + echo "Test-tier verdict census:" + classify "PHPUnit" "$PHPUNIT_ENABLED" "$PHPUNIT_RESULT" + classify "Newman" "$NEWMAN_ENABLED" "$NEWMAN_RESULT" + classify "Playwright" "$PLAYWRIGHT_ENABLED" "$PLAYWRIGHT_RESULT" + classify "Journeydoc capture" "$JOURNEYDOC_ENABLED" "$JOURNEYDOC_RESULT" + + if [ -z "$MISSING" ]; then + echo "Every enabled test job produced a verdict." + # State the distinction the 2026-08-06 reports could not make. + if [ "$SECURITY_RESULT" = "failure" ] && [ -n "$RAN" ]; then + echo "::notice::Security failed, and the test tier still ran and reported. This is 'tests produced a verdict, security did not pass' — NOT 'tests never ran'. The merge is blocked by the security failure, and the test results above are real." + fi + exit 0 + fi + + TIP="$(gh api "repos/${{ github.repository }}/commits/${RUN_REF}" --jq .sha 2>/dev/null || true)" + if [ -n "$TIP" ] && [ "$TIP" != "$RUN_SHA" ]; then + echo "::notice::Test job(s) did not run, but this run has been SUPERSEDED (${RUN_REF} is now at ${TIP:0:7}, this run is ${RUN_SHA:0:7}). The newer run is the one whose verdict counts — staying quiet." + exit 0 + fi + + echo "::error::TEST TIER NOT EXECUTED — NO VERDICT EXISTS for one or more enabled test jobs. A skipped test job is not a pass; it is the absence of a result. This gate cannot pass." + { + echo "" + echo "## 🚨 Gate failed — NO VERDICT EXISTS for the test tier" + echo "" + echo "These jobs were **enabled by this repository's caller** and did **not run**:" + echo "" + # `|| true` on the loop: the default shell is `bash -e`, and a final + # iteration whose `[ -n ]` test is false would make the pipeline + # return 1 and abort the rest of this summary block. + printf '%s' "$MISSING" | while IFS= read -r j; do + if [ -n "$j" ]; then echo "- \`$j\` — result \`skipped\`"; fi + done || true + echo "" + echo "**This is not a test failure. It is the absence of a test result.**" + echo "Nothing above should be read as evidence that the code works, because" + echo "no test was executed to produce that evidence." + echo "" + if [ "$SECURITY_RESULT" = "failure" ]; then + echo "\`Security\` also failed in this run. Note the two are now independent" + echo "(#194) — a security failure no longer skips the test tier, so if the" + echo "test tier is missing, something else deleted it. Investigate the" + echo "skipped job's \`if:\` condition." + fi + } >> "$GITHUB_STEP_SUMMARY" + exit 1 + # A CANCELLED job renders NO verdict, so treating it as merely # "not a failure" lets this gate go green over a leg that never ran. # Observed live on pipelinq run 30903070948: `E2E Tests (Playwright)` diff --git a/.github/workflows/test-tier-gating-fixture.yml b/.github/workflows/test-tier-gating-fixture.yml new file mode 100644 index 0000000..578808e --- /dev/null +++ b/.github/workflows/test-tier-gating-fixture.yml @@ -0,0 +1,135 @@ +name: Fixture — security-gated test tier (#194) + +# THIS WORKFLOW EXISTS TO BE FAILED. It is the executable evidence for #194. +# +# #194 claims that gating the test tier on the security tier does not merely +# *skip* tests — it DELETES THE EVIDENCE, and the deletion does not render as a +# failure. That claim is easy to assert and easy to get wrong, so it is measured +# here on real GitHub Actions rather than argued from the expression grammar. +# +# The three jobs below differ in ONE line each. Everything else — the runner, +# the steps, the "test" that fails — is identical, so any difference in outcome +# is attributable to the `if:` and to nothing else. +# +# security always fails, standing in for `composer audit` picking up +# a fresh advisory against a dev-only dependency. +# +# test-gated the shape that SHIPPED until this PR: +# if: !cancelled() && needs.security.result != 'failure' +# Contains a test that FAILS. Expected outcome: `skipped`. +# Its failure is never reported, because it never runs. +# +# test-decoupled the shape this PR ships: +# if: !cancelled() +# Byte-for-byte the same failing test. Expected outcome: +# `failure`. The verdict exists. +# +# aggregate-old the tally quality.yml used: `contains(needs.*.result, +# 'failure')` over the test tier. Expected: GREEN, over a +# test tier that produced nothing. +# +# aggregate-new the invariant this PR adds to the Quality Report: an +# ENABLED test job in state `skipped` is the absence of a +# verdict. Expected: RED, naming the missing job. +# +# Runs only on `fixture/**` branches, so it costs the fleet nothing and can be +# re-run whenever someone wants to see the mechanism rather than read about it. + +on: + push: + branches: + - 'fixture/**' + workflow_dispatch: + +permissions: + contents: read + +jobs: + security: + name: "fixture: security (always FAILS)" + runs-on: ubuntu-latest + steps: + - name: Stand in for a fresh advisory against a dev-only dependency + run: | + echo "Simulating: composer audit picks up CVE-2026-67434 against" + echo "squizlabs/php_codesniffer — a code FORMATTER that never runs in" + echo "production. No commit was made; the live advisory feed changed." + echo "::error::Security (composer): 1 advisory found" + exit 1 + + # ── BEFORE: the shape that shipped ────────────────────────────────────────── + test-gated: + name: "fixture: BEFORE — test tier gated on security" + needs: [security] + if: ${{ !cancelled() && needs.security.result != 'failure' }} + runs-on: ubuntu-latest + steps: + - name: A test suite that FAILS + run: | + echo "TEST EVIDENCE PRODUCED: 3 passed, 1 FAILED" + echo "::error::PHPUnit: 1 test failed" + exit 1 + + # ── AFTER: the shape this PR ships ────────────────────────────────────────── + test-decoupled: + name: "fixture: AFTER — test tier decoupled" + needs: [security] + if: ${{ !cancelled() }} + runs-on: ubuntu-latest + steps: + - name: A test suite that FAILS + run: | + echo "TEST EVIDENCE PRODUCED: 3 passed, 1 FAILED" + echo "::error::PHPUnit: 1 test failed" + exit 1 + + # ── The tally that could not see the deletion ─────────────────────────────── + aggregate-old: + name: "fixture: OLD tally over the test tier" + needs: [test-gated] + if: always() + runs-on: ubuntu-latest + steps: + - name: Report what the old tally saw + run: | + echo "test-gated result: ${{ needs.test-gated.result }}" + echo "" + echo "The old gate is: contains(needs.*.result, 'failure')" + echo "'skipped' is not 'failure', so this reads ZERO failures." + - name: Old gate — fail when any upstream job failed + if: ${{ always() && contains(needs.*.result, 'failure') }} + run: | + echo "::error::a failure was seen" + exit 1 + - name: What this job concluded + run: | + echo "CONCLUSION: green. A test suite that would have FAILED was" + echo "deleted, and the tally reports nothing wrong. This is the" + echo "silent-green #194 describes." + + # ── The invariant this PR adds ────────────────────────────────────────────── + aggregate-new: + name: "fixture: NEW invariant over the test tier" + needs: [test-gated] + if: always() + runs-on: ubuntu-latest + steps: + # Same logic as the Quality Report's "Gate — a test job that was ENABLED + # but never ran has no verdict" step, reduced to the one job under test. + - name: New gate — an ENABLED test job that never ran has no verdict + env: + PHPUNIT_ENABLED: 'true' + PHPUNIT_RESULT: ${{ needs.test-gated.result }} + run: | + set -uo pipefail + MISSING="" + if [ "$PHPUNIT_ENABLED" = "true" ] && [ "$PHPUNIT_RESULT" = "skipped" ]; then + MISSING="PHPUnit" + fi + if [ -z "$MISSING" ]; then + echo "Every enabled test job produced a verdict." + exit 0 + fi + echo "::error::TEST TIER NOT EXECUTED — NO VERDICT EXISTS for one or more enabled test jobs. A skipped test job is not a pass; it is the absence of a result. This gate cannot pass." + echo "Missing: $MISSING (result=$PHPUNIT_RESULT)" + exit 1 diff --git a/scripts/assert-no-producer-deletes-a-verdict.py b/scripts/assert-no-producer-deletes-a-verdict.py new file mode 100755 index 0000000..e975f0c --- /dev/null +++ b/scripts/assert-no-producer-deletes-a-verdict.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""Fail when one job's result can DELETE another job instead of failing it. + +WHY +--- +`assert-quality-report-gates-every-leg.py` closes one direction: every job that +renders a verdict must be in the Quality Report's `needs:`, or its failure +cannot block a merge (#190/#229). This closes the OTHER direction, which cost us +2026-08-06 (#194): a job can be removed from the run entirely by an upstream +result, and a removed job does not fail — it goes `skipped`, which renders as a +grey tick and is counted by nothing. + +Two shapes produce that, and both are checked here. + +SHAPE 1 — the implicit `success()` trap. + A job with `needs:` and either no `if:` at all, or an `if:` containing no + status-check function, keeps GitHub's default `success()` over its `needs`. + Any producer that fails or skips then deletes it silently. The fix is to + include a status function — conventionally `!cancelled()` in this file — + which suppresses the default and lets the job reach its own verdict. + +SHAPE 2 — an explicit result gate. + `if: … && needs.<producer>.result != 'failure'` is the same deletion, + written on purpose. This is exactly what #194 is about: four test jobs + carried `needs.security.result != 'failure'`, and one advisory published + against a dev-only code formatter turned PHPUnit, Newman and E2E into + `skipped` across sixteen repositories at once, with no commit and no red. + + The distinction that matters: blocking a MERGE on a producer is correct and + is done by putting the producer in the Quality Report's `needs:`. DELETING a + consumer is never the way to express it, because the deletion destroys the + consumer's evidence at exactly the moment you want it. + +An exemption requires an entry in ALLOWLIST with a stated reason, so the list +can only shrink and it says why. + +Usage: assert-no-producer-deletes-a-verdict.py <workflow.yml> + assert-no-producer-deletes-a-verdict.py --positive-control <workflow.yml> +Exit: 0 no job can be deleted by a producer, 1 at least one can. +""" + +from __future__ import annotations + +import argparse +import re +import sys + +import yaml + +# A status-check function suppresses the implicit `success()` over `needs`. +STATUS_FN_RE = re.compile(r"\b(?:success|failure|cancelled|always)\s*\(") + +# `needs.foo.result == 'x'` / `needs['foo'].result != 'x'` in a JOB-level `if:`. +# Step-level conditions are fine and are not inspected: a step that does not run +# leaves its job's verdict intact, whereas a job that does not run has none. +RESULT_GATE_RE = re.compile( + r"needs(?:\.[A-Za-z0-9_-]+|\[['\"][A-Za-z0-9_-]+['\"]\])\.result\s*(?:!=|==)" +) + +# job-id -> reason. Empty on purpose. +ALLOWLIST: dict[str, str] = {} + + +def check(workflow: dict, injected: dict | None = None) -> list[str]: + jobs = dict(workflow.get("jobs") or {}) + if injected: + jobs.update(injected) + + problems: list[str] = [] + for job_id, job in jobs.items(): + if not isinstance(job, dict): + continue + if job_id in ALLOWLIST: + continue + needs = job.get("needs") or [] + if isinstance(needs, str): + needs = [needs] + cond = str(job.get("if", "")) + + if needs and not STATUS_FN_RE.search(cond): + problems.append( + f"{job_id}: has `needs: {needs}` and no status-check function in " + f"`if:` — the implicit success() means ANY producer failing or " + f"skipping DELETES this job instead of failing it. Add " + f"`!cancelled()` to the condition." + ) + + m = RESULT_GATE_RE.search(cond) + if m: + problems.append( + f"{job_id}: `if:` gates on `{m.group(0)}…` — a producer's result " + f"SKIPS this job, and a skipped job renders no verdict and no " + f"red (#194). Block the merge by listing the producer in the " + f"Quality Report's `needs:` instead." + ) + + return problems + + +def main(argv: list[str]) -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("workflow") + ap.add_argument( + "--positive-control", + action="store_true", + help="Inject the exact #194 shape and assert this check detects it. A " + "check that cannot fail is worth nothing.", + ) + args = ap.parse_args(argv) + + with open(args.workflow) as fh: + workflow = yaml.safe_load(fh) + + if args.positive_control: + injected = { + "__probe_result_gate": { + "needs": ["security"], + "if": "${{ !cancelled() && needs.security.result != 'failure' }}", + }, + "__probe_implicit_success": { + "needs": ["security"], + "if": "${{ inputs.enable-php }}", + }, + } + problems = check(workflow, injected) + found = {p.split(":")[0] for p in problems} + if {"__probe_result_gate", "__probe_implicit_success"} <= found: + print( + "OK — the check fails when it should: both an explicit " + "`needs.*.result` gate and an implicit success() trap are " + "detected. Its clean pass is a verdict." + ) + return 0 + print(f"FAIL — positive control not detected. Found: {sorted(found)}") + return 1 + + problems = check(workflow) + if problems: + print("A producer can DELETE a verdict-producing job:\n") + for p in problems: + print(f" - {p}") + print( + "\nA deleted job is `skipped`. `skipped` is not `failure`, so no " + "tally counts it and it renders in the same colour family as a " + "pass. See #194." + ) + return 1 + + n = sum( + 1 + for j in (workflow.get("jobs") or {}).values() + if isinstance(j, dict) and j.get("needs") + ) + print( + f"OK — {n} job(s) declare `needs:` and none of them can be deleted by a " + f"producer's result; each reaches its own verdict." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/test-spec-coverage-gate.py b/scripts/test-spec-coverage-gate.py new file mode 100755 index 0000000..0d1ed77 --- /dev/null +++ b/scripts/test-spec-coverage-gate.py @@ -0,0 +1,617 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""Prove the spec-to-test coverage gate CAN FAIL, and fails for the right reasons. + +WHY +--- +#189: `playwright-coverage-threshold` had never gated anything. Below-threshold +emitted `::warning::` and returned zero, so the job stayed green. pipelinq +declared a floor of 75, its run printed 28%, and the run passed. The issue's +closing requirement is the reason this file exists: + + "it needs a test that shows the gate CAN fail — a fixture with coverage + below the threshold that turns the job red. Right now no such run can + exist." + +Two further defects are covered here because they are the reason the number was +not worth gating on in the first place: + + * the metric was `count(test() calls) / count(scenario headings)` — two + independent totals. Ten unrelated tests raised it exactly as much as + covering ten scenarios. `test_unrelated_tests_do_not_raise_coverage` + pins that they no longer do. + * zero scenarios scored **100%**, so the worst-covered repo and the best + produced the same number. `test_zero_scenarios_is_not_a_pass` pins that + it is now a failure to measure. + +WHAT IS UNDER TEST +------------------ +Not a copy of the program — THE program. The Node source is extracted out of +`.github/workflows/quality.yml` at run time (the `cat > … <<'SPEC_COVERAGE_JS'` +heredoc inside the "Generate spec-to-test coverage report" step) and executed. +A test against a transcription would pass happily while the shipped workflow +did something else; that is the failure mode this whole issue is about. + +Usage: test-spec-coverage-gate.py [workflow.yml] + test-spec-coverage-gate.py --positive-control [workflow.yml] +Exit: 0 all assertions hold, 1 at least one failed. +""" + +from __future__ import annotations + +import argparse +import json +import os +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path + +import yaml + +STEP_NAME = "Generate spec-to-test coverage report" +HEREDOC_OPEN = "<<'SPEC_COVERAGE_JS'" +HEREDOC_CLOSE = "SPEC_COVERAGE_JS" + +FAILURES: list[str] = [] +PASSES: list[str] = [] + + +# --------------------------------------------------------------------------- +# Extraction +# --------------------------------------------------------------------------- +def extract_program(workflow: Path) -> str: + """Pull the Node program out of the shipped workflow's heredoc.""" + data = yaml.safe_load(workflow.read_text()) + steps = data["jobs"]["playwright"]["steps"] + run = None + for step in steps: + if step.get("name") == STEP_NAME: + run = step.get("run") + break + if run is None: + raise SystemExit(f"FATAL: no step named {STEP_NAME!r} in the playwright job") + if HEREDOC_OPEN not in run: + raise SystemExit( + f"FATAL: step {STEP_NAME!r} no longer opens a {HEREDOC_OPEN} heredoc. " + "If the program moved, move this extractor with it — a test that " + "cannot find its subject must not report success." + ) + + lines = run.split("\n") + start = next(i for i, ln in enumerate(lines) if HEREDOC_OPEN in ln) + 1 + end = next(i for i in range(start, len(lines)) if lines[i].strip() == HEREDOC_CLOSE) + program = "\n".join(lines[start:end]) + if "process.exit(1)" not in program: + raise SystemExit( + "FATAL: the extracted program contains no `process.exit(1)`. A " + "coverage gate with no non-zero exit is exactly the defect #189 " + "reports; refusing to pretend this is testable." + ) + return program + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- +def write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def make_repo(root: Path, specs: dict[str, str], tests: dict[str, str]) -> None: + for name, body in specs.items(): + write(root / "openspec" / "specs" / name / "spec.md", body) + for name, body in tests.items(): + write(root / "tests" / "e2e" / name, body) + + +def run_program(program: str, repo: Path, threshold: str) -> tuple[int, str]: + js = repo / "_spec-coverage.js" + js.write_text(program) + env = dict(os.environ) + env["SPEC_COVERAGE_THRESHOLD"] = threshold + env["PLAYWRIGHT_TEST_PATH"] = "tests/e2e" + env["SPEC_COVERAGE_OUT"] = "playwright-coverage.json" + proc = subprocess.run( + ["node", str(js)], + cwd=repo, + env=env, + capture_output=True, + text=True, + ) + return proc.returncode, proc.stdout + proc.stderr + + +def report_json(repo: Path) -> dict: + return json.loads((repo / "playwright-coverage.json").read_text()) + + +def check(label: str, condition: bool, detail: str = "") -> None: + if condition: + PASSES.append(label) + print(f" PASS {label}") + else: + FAILURES.append(f"{label}{(' — ' + detail) if detail else ''}") + print(f" FAIL {label}{(' — ' + detail) if detail else ''}") + + +# Four scenarios in one spec; exactly ONE carries an @e2e reference. +SPEC_FOUR = """# Widget spec + +## Purpose +Four scenarios, one of them referenced from a test. + +### Requirement: Widget behaviour + +#### Scenario: Widget renders on the dashboard +- WHEN the dashboard loads +- THEN the widget is visible + +#### Scenario: Widget refreshes on demand +- WHEN the refresh button is pressed +- THEN the widget reloads + +#### Scenario: Widget shows an empty state +- WHEN there is no data +- THEN an empty state is shown + +#### Scenario: Widget survives a failed request +- WHEN the request fails +- THEN an error is shown +""" + +TEST_ONE_REF = """import { test, expect } from '@playwright/test'; + +// @e2e widget::widget-renders-on-the-dashboard +test('widget renders', async ({ page }) => { + await page.goto('/'); +}); +""" + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- +def test_below_threshold_fails(program: str, tmp: Path) -> None: + """THE headline assertion of #189: below-threshold turns the job red.""" + repo = tmp / "below" + make_repo(repo, {"widget": SPEC_FOUR}, {"widget.spec.ts": TEST_ONE_REF}) + code, out = run_program(program, repo, "75") + rep = report_json(repo) + + check("below-threshold: exits NON-ZERO (the gate can fail)", code == 1, f"exit={code}") + check("below-threshold: emits ::error:: not ::warning::", + "::error::" in out and "::warning::" not in out) + check("below-threshold: coverage is 25% (1 of 4 scenarios referenced)", + rep["coverage"] == 25, f"got {rep['coverage']}") + check("below-threshold: names the uncovered scenarios", + "widget#widget-refreshes-on-demand" in out) + + +def test_at_threshold_passes(program: str, tmp: Path) -> None: + """The same fixture must PASS under a floor it meets — or the gate is just + a way of always failing, which is not a gate either.""" + repo = tmp / "at" + make_repo(repo, {"widget": SPEC_FOUR}, {"widget.spec.ts": TEST_ONE_REF}) + code, out = run_program(program, repo, "25") + check("at-threshold: exits ZERO", code == 0, f"exit={code}") + check("at-threshold: says it meets the threshold", + "meets the threshold" in out) + + +def test_zero_scenarios_is_not_a_pass(program: str, tmp: Path) -> None: + """#189 defect 3: `scenarios.length === 0 ? … : 100` scored an unmeasured + repo at 100%. It must now be a failure to measure.""" + repo = tmp / "zero" + make_repo(repo, {}, {"smoke.spec.ts": "test('smoke', async () => {});\n"}) + (repo / "openspec" / "specs").mkdir(parents=True, exist_ok=True) + code, out = run_program(program, repo, "0") + rep = report_json(repo) + + check("zero-scenarios: exits NON-ZERO", code == 1, f"exit={code}") + check("zero-scenarios: does NOT score 100", rep["coverage"] != 100, + f"coverage={rep['coverage']}") + check("zero-scenarios: coverage is null, not a number", rep["coverage"] is None) + check("zero-scenarios: reports NOT MEASURABLE in words", + "NOT MEASURABLE" in out) + check("zero-scenarios: fails even at threshold 0", + code == 1, + "a threshold of 0 must not turn 'could not measure' into a pass") + + +def test_unrelated_tests_do_not_raise_coverage(program: str, tmp: Path) -> None: + """#189 defect 2: under the old formula, ten unrelated `test()` calls raised + the number exactly as much as covering ten scenarios. Pin that they do not.""" + baseline = tmp / "unrelated-a" + make_repo(baseline, {"widget": SPEC_FOUR}, {"widget.spec.ts": TEST_ONE_REF}) + run_program(program, baseline, "0") + before = report_json(baseline)["coverage"] + + padded = tmp / "unrelated-b" + noise = "\n".join( + f"test('unrelated noise {i}', async () => {{}});" for i in range(10) + ) + make_repo( + padded, + {"widget": SPEC_FOUR}, + {"widget.spec.ts": TEST_ONE_REF, "noise.spec.ts": noise + "\n"}, + ) + run_program(program, padded, "0") + after_rep = report_json(padded) + + check("unrelated tests: real coverage is UNCHANGED by 10 noise tests", + before == after_rep["coverage"] == 25, + f"before={before} after={after_rep['coverage']}") + check("unrelated tests: the old ratio DID move (proving the fixture bites)", + after_rep["testsPerScenarioPercent"] > 25, + f"testsPerScenarioPercent={after_rep['testsPerScenarioPercent']}") + check("unrelated tests: the old ratio is reported under an honest name", + "testsPerScenarioPercent" in after_rep and "coverage" in after_rep) + + +def test_exclusions_leave_the_denominator(program: str, tmp: Path) -> None: + """A reason-bearing `@e2e exclude` is the documented way out (gate-19's + dialect). It must remove the scenario from the denominator, not count as + covered — otherwise excluding everything would read as 100% tested.""" + spec = SPEC_FOUR.replace( + "#### Scenario: Widget refreshes on demand\n", + "#### Scenario: Widget refreshes on demand\n<!-- @e2e exclude server-side, covered by PHPUnit -->\n", + ) + repo = tmp / "excl" + make_repo(repo, {"widget": spec}, {"widget.spec.ts": TEST_ONE_REF}) + run_program(program, repo, "0") + rep = report_json(repo) + + check("exclusions: one scenario is excluded", rep["excludedScenarios"] == 1, + f"got {rep['excludedScenarios']}") + check("exclusions: denominator drops to 3", rep["enforceableScenarios"] == 3, + f"got {rep['enforceableScenarios']}") + check("exclusions: coverage rises to 33% (1 of 3), not to 'covered'", + rep["coverage"] == 33, f"got {rep['coverage']}") + + +def test_whole_spec_exclusion(program: str, tmp: Path) -> None: + """A whole-spec exclusion voids every scenario in the file — and with only + that spec present, the result must be NOT MEASURABLE rather than 100%.""" + spec = SPEC_FOUR.replace( + "## Purpose\n", + "## Purpose\n\n@e2e exclude pure backend spec, covered by PHPUnit\n", + ) + repo = tmp / "whole" + make_repo(repo, {"widget": spec}, {"widget.spec.ts": TEST_ONE_REF}) + code, out = run_program(program, repo, "0") + rep = report_json(repo) + + check("whole-spec exclude: all 4 scenarios excluded", + rep["excludedScenarios"] == 4, f"got {rep['excludedScenarios']}") + check("whole-spec exclude: nothing enforceable left is NOT MEASURABLE", + rep["measurable"] is False and code == 1 and "NOT MEASURABLE" in out) + + +def test_format_b_numbered_scenarios(program: str, tmp: Path) -> None: + """gate-19's Format B: numbered GIVEN/WHEN items under a `**Scenarios:**` + marker, addressed as `<req-slug>-scenario-<n>`.""" + spec = """# Decomposition spec + +## Purpose +Format B scenarios. + +### REQ-DECOMP-001: Settings controller decomposition + +**Scenarios:** + +1. **GIVEN** a fat controller **WHEN** it is split **THEN** each part is testable +2. **GIVEN** a split controller **WHEN** routes resolve **THEN** behaviour is unchanged +""" + test = """import { test } from '@playwright/test'; +// @e2e decomp::settings-controller-decomposition-scenario-1 +test('decomp', async () => {}); +""" + repo = tmp / "fmtb" + make_repo(repo, {"decomp": spec}, {"decomp.spec.ts": test}) + run_program(program, repo, "0") + rep = report_json(repo) + + check("format B: two numbered scenarios found", rep["specScenarios"] == 2, + f"got {rep['specScenarios']}") + check("format B: the referenced one is covered", rep["coveredScenarios"] == 1, + f"got {rep['coveredScenarios']}") + check("format B: coverage is 50%", rep["coverage"] == 50, f"got {rep['coverage']}") + + +def test_exclude_is_not_a_reference(program: str, tmp: Path) -> None: + """`@e2e exclude …` inside a TEST file is a directive, not a scenario + reference. Reading it as one lets an exclusion silently mark a scenario + COVERED — the opposite of what it says. + + THE FIXTURE FORM MATTERS, and the mutation battery is what proved it. The + obvious fixture — `@e2e exclude some-slug`, space-separated — cannot + distinguish the guarded regex from the unguarded one: both capture + `exclude`, which contains neither `#` nor `::`, so neither resolves to a + slug. Written that way this test passed while proving nothing, and the + `exclude-directive-read-as-reference` mutant SURVIVED. + + The separator forms are where the guard is load-bearing: + + @e2e exclude::<slug> guarded -> no ref unguarded -> marks <slug> COVERED + @e2e exclude#<slug> guarded -> no ref unguarded -> marks <slug> COVERED + + Both are plausible as a typo or shorthand for the documented + `@e2e exclude <reason>`, which is exactly why the guard exists. + """ + test = """import { test } from '@playwright/test'; +// @e2e exclude widget-refreshes-on-demand — server-side, covered by PHPUnit +// @e2e exclude::widget-renders-on-the-dashboard +// @e2e exclude#widget-shows-an-empty-state +test('nothing', async () => {}); +""" + repo = tmp / "excl-ref" + make_repo(repo, {"widget": SPEC_FOUR}, {"widget.spec.ts": test}) + run_program(program, repo, "0") + rep = report_json(repo) + check("exclude-in-test: contributes no @e2e reference", + rep["e2eReferences"] == 0, f"got {rep['e2eReferences']}") + check("exclude-in-test: nothing is marked covered", + rep["coveredScenarios"] == 0, f"got {rep['coveredScenarios']}") + check("exclude-in-test: all 4 scenarios remain enforceable and uncovered", + rep["enforceableScenarios"] == 4 and rep["uncoveredScenarios"] == 4, + f"enforceable={rep['enforceableScenarios']} uncovered={rep['uncoveredScenarios']}") + + +TESTS = [ + test_below_threshold_fails, + test_at_threshold_passes, + test_zero_scenarios_is_not_a_pass, + test_unrelated_tests_do_not_raise_coverage, + test_exclusions_leave_the_denominator, + test_whole_spec_exclusion, + test_format_b_numbered_scenarios, + test_exclude_is_not_a_reference, +] + + +# --------------------------------------------------------------------------- +# Mutation battery +# --------------------------------------------------------------------------- +# A test suite that passes proves nothing on its own — the question is whether +# it would have NOTICED the bug. Each mutant below reintroduces one specific +# defect into the shipped program; the suite must go RED for every one. +# +# A mutant that SURVIVES means the suite cannot reach that branch, and the fix +# is a better fixture, not a shrug. +# +# The last entry is an ANTI-WIDENING CONTROL and inverts the expectation: it +# perturbs a log string nothing asserts on, and the suite must stay GREEN. +# Without it, a suite that failed on *any* edit would score a perfect kill rate +# while being worthless. +# +# (name, find, replace, must_be_caught, why) +MUTANTS: list[tuple[str, str, str, bool, str]] = [ + ( + "enforcement-removed", + "process.exit(1)", + "process.exit(0)", + True, + "the #189 defect verbatim: compute a verdict, then decline to act on it", + ), + ( + "threshold-never-fires", + "if (coverage < THRESHOLD) {", + "if (coverage < -1) {", + True, + "a comparison that no real coverage value can satisfy", + ), + ( + "zero-scenarios-scores-100", + "var coverage = measurable ? Math.round((covered / enforceable) * 100) : null;", + "var coverage = measurable ? Math.round((covered / enforceable) * 100) : 100;", + True, + "#189 defect 3: the unmeasured repo and the perfect one report the same number", + ), + ( + "not-measurable-treated-as-pass", + "if (!measurable) {", + "if (false) {", + True, + "a failure to measure silently becomes a passing run", + ), + ( + "exclusions-counted-as-covered", + "if (sc.excluded) { excluded++; sc.status = 'excluded'; continue; }", + "if (false) { excluded++; sc.status = 'excluded'; continue; }", + True, + "excluded scenarios re-enter the denominator, moving every ratio", + ), + ( + "exclude-directive-read-as-reference", + "var REF_RE = /@e2e\\s+(?!exclude\\b)(\\S+)/g;", + "var REF_RE = /@e2e\\s+(\\S+)/g;", + True, + "`@e2e exclude x` in a TEST would mark scenario x covered", + ), + ( + "scenario-headings-not-found", + "var SCENARIO_RE = /^#{4}\\s+Scenario:\\s*(.+)$/i;", + "var SCENARIO_RE = /^#{9}\\s+Scenario:\\s*(.+)$/i;", + True, + "the denominator collapses to zero — the shape that scored 100% before", + ), + ( + "ANTI-WIDENING benign log rewording", + "console.log('Playwright test() calls: '", + "console.log('Playwright test invocations: '", + False, + "no assertion depends on this wording; the suite must NOT fail here", + ), +] + + +def run_suite(program: str) -> tuple[int, int]: + """Run the whole suite against `program`, quietly. Returns (passes, fails).""" + PASSES.clear() + FAILURES.clear() + import contextlib + import io + + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + with tempfile.TemporaryDirectory() as td: + tmp = Path(td) + for fn in TESTS: + try: + fn(program, tmp) + except Exception as exc: # noqa: BLE001 + FAILURES.append(f"{fn.__name__} raised {exc!r}") + return len(PASSES), len(FAILURES) + + +def mutation_battery(program: str) -> int: + print("Mutation battery — each mutant reintroduces one defect; the suite " + "must notice.\n") + baseline_pass, baseline_fail = run_suite(program) + print(f"baseline (unmutated): {baseline_pass} passed, {baseline_fail} failed") + if baseline_fail: + print("FATAL: the suite is not green against the shipped program; " + "mutation results would be meaningless.") + return 1 + print() + + survivors: list[str] = [] + wiring: list[str] = [] + widened: list[str] = [] + + for name, find, replace, must_catch, why in MUTANTS: + if find not in program: + # A mutant that cannot be applied is a WIRING failure. Reporting it + # as a kill would be the same lie this whole file exists to stop. + wiring.append(name) + print(f" SKIPPED (wiring) {name}\n" + f" anchor not found in the program: {find!r}") + continue + + mutated = program.replace(find, replace) + _, fails = run_suite(mutated) + + if must_catch: + if fails: + print(f" KILLED {name} ({fails} assertion(s) flipped) — {why}") + else: + survivors.append(name) + print(f" SURVIVED {name} — SUITE STAYED GREEN. {why}") + else: + if fails: + widened.append(name) + print(f" OVER-BROAD {name} ({fails} flipped) — {why}") + else: + print(f" (control) {name}: suite correctly stayed GREEN — {why}") + + print() + killable = [m for m in MUTANTS if m[3]] + print(f"{len(killable) - len(survivors) - len([w for w in wiring if w])} " + f"of {len(killable)} defect-mutants killed; " + f"{len(survivors)} survived; {len(wiring)} unapplied (wiring).") + + if wiring: + print("\nFAIL — a mutant could not be applied. The anchors have drifted " + "from the program; the battery is measuring less than it claims.") + return 1 + if survivors: + print("\nFAIL — mutant(s) survived. The suite cannot reach that " + "behaviour, so it proves nothing about it. Fix the FIXTURE:") + for s in survivors: + print(f" - {s}") + return 1 + if widened: + print("\nFAIL — the anti-widening control was caught. The suite fails " + "on changes it should not care about, so its kills are not " + "evidence of anything specific:") + for w in widened: + print(f" - {w}") + return 1 + + print("\nOK — every defect-mutant is caught, and the benign control is not.") + return 0 + + +def main(argv: list[str]) -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("workflow", nargs="?", default=".github/workflows/quality.yml") + ap.add_argument( + "--positive-control", + action="store_true", + help="Neuter the gate's non-zero exit and assert THIS SUITE then fails. " + "A suite that cannot fail is the same defect it is testing for.", + ) + ap.add_argument( + "--mutation-battery", + action="store_true", + help="Reintroduce each known defect one at a time and require the suite " + "to notice every one, plus a benign control it must NOT notice.", + ) + args = ap.parse_args(argv) + + workflow = Path(args.workflow) + if not workflow.is_file(): + print(f"FATAL: {workflow} not found") + return 1 + + if shutil.which("node") is None: + print("FATAL: node is not on PATH — cannot execute the program under test") + return 1 + + program = extract_program(workflow) + print(f"Extracted {len(program.splitlines())} lines of Node from " + f"{workflow}:jobs.playwright.steps[{STEP_NAME!r}]") + + if args.mutation_battery: + return mutation_battery(program) + + if args.positive_control: + # Turn every hard exit back into the warning-only behaviour #189 + # describes. The suite MUST go red; if it stays green it is not + # measuring the exit status at all. + program = program.replace("process.exit(1)", "process.exit(0)") + program = program.replace("::error::", "::warning::") + print("POSITIVE CONTROL: gate neutered to warning-only (the #189 behaviour).") + + print() + with tempfile.TemporaryDirectory() as td: + tmp = Path(td) + for fn in TESTS: + print(f"{fn.__name__}:") + try: + fn(program, tmp) + except Exception as exc: # noqa: BLE001 + FAILURES.append(f"{fn.__name__} raised {exc!r}") + print(f" FAIL raised {exc!r}") + print() + + print(f"{len(PASSES)} passed, {len(FAILURES)} failed") + + if args.positive_control: + if FAILURES: + print( + "\nOK — the suite FAILS when the gate is neutered. " + "Its clean pass is a verdict." + ) + return 0 + print( + "\nFAIL — the suite stayed GREEN with the gate neutered. It is not " + "measuring enforcement." + ) + return 1 + + if FAILURES: + print("\nFailures:") + for f in FAILURES: + print(f" - {f}") + return 1 + + print("\nOK — the spec-to-test coverage gate enforces, and can fail.") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:]))