From c08905f784d3a5bade0489669cd26abb470c76c6 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:32:53 +0200 Subject: [PATCH 01/22] =?UTF-8?q?fix(verify-pr-checks):=20close=20five=20r?= =?UTF-8?q?eview=20findings=20(critical,=20high,=20medium,=202=C3=97low)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical — Tier B silently skipped non-required runs whose status was not 'completed' (queued/in_progress). A PR with 6 required contexts all green plus Source hygiene=queued yielded exit 0 and printed the --match-head-commit merge command. Fix: Tier B now FAILs (exit 1) on any non-completed non-required run (avoids PF-017: indeterminate state is never success). High — Tier B cannot assert *presence* of a non-required job — it only evaluates runs that already appear in the check-run list. An absent Source hygiene job produced no Tier B entry and the verifier exited 0 (the PoC from the review finding). Fix: add EXPECTED_CONTEXTS = ['Source hygiene'] with Tier A semantics (Tier A+). Absence is FAIL, not advisory (applies ADR-009, avoids PF-013). Medium — defaultGhRunner returned `status: r.status` (the gh process exit code, always 1) but fetchRequiredContexts branched on `data.status === 404 / 403`. Both branches were unreachable on the live path (gh exits 1, never 404/403), so the AC-29 remediation message never printed and the two tests for those branches validated dead code. Fix: parse `(HTTP NNN)` from gh's stderr into a separate `httpStatus` field; fetchRequiredContexts branches on `data.httpStatus`; stub runner updated to mirror the parsed shape (avoids PF-013). Low — fetchStatuses used the combined-status endpoint which caps at 30 statuses with no pagination. A required context backed by a status beyond position 30 would have been falsely reported as never-ran. Fix: add a total_count guard consistent with D-PR4a; if total_count > returned statuses, exit 2 rather than evaluate a partial set. Low — Tier A resolved each required context with if/else-if: check-runs took priority and the status namespace was only consulted when no check-run existed. A failing commit status was silently ignored when a check-run of the same name was green, diverging from GitHub's enforcement model (D-PR2a). Fix: check both namespaces independently for every required context. Test coverage: 59 tests (14 suites), 0 failures. New test groups verify the EXPECTED_CONTEXTS PoC (absence → exit 1), Tier B non-completed (queued → exit 1), both-namespace Tier A (status failure not masked), fetchStatuses total_count guard, and httpStatus branching (dead-branch elimination). Co-Authored-By: Claude --- scripts/__test__/verify-pr-checks.spec.mjs | 444 +++++++++++++++++++-- scripts/verify-pr-checks.mjs | 177 +++++++- 2 files changed, 569 insertions(+), 52 deletions(-) diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index 4f6a379..23b0c59 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -12,13 +12,20 @@ import { test, describe } from 'node:test'; import assert from 'node:assert/strict'; -import { readFileSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { readFileSync, statSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { createHash } from 'node:crypto'; import { join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { spawnSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; -import { evaluateChecks, main, fetchRequiredContexts } from '../verify-pr-checks.mjs'; +import { + evaluateChecks, + main, + fetchRequiredContexts, + fetchStatuses, + EXPECTED_CONTEXTS, +} from '../verify-pr-checks.mjs'; const ROOT = resolve(fileURLToPath(import.meta.url), '../../..'); const FIXTURES = join(ROOT, 'scripts/__test__/fixtures'); @@ -54,14 +61,22 @@ const HEAD_113F472 = '113f472684d6ee7e398d54c1aadc22b2ad747ae1'; const HEAD_F168944 = 'f168944'; // PR #239 const HEAD_E9DACE1 = 'e9dace1'; // PR #240 +// A synthetic Source hygiene run (D-PR3b). The 113f472 fixture predates the +// source-hygiene job (#288); tests that verify a PASSING run today must inject one. +const SOURCE_HYGIENE_PASS = { name: 'Source hygiene', status: 'completed', conclusion: 'success' }; + // --------------------------------------------------------------------------- // AC-22: Historical fixtures reproduce correctly // --------------------------------------------------------------------------- describe('AC-21 AC-22: historical fixture evaluation', () => { - test('113f472 (main baseline, 18 check-runs, all success) → PASS (exit 0)', () => { - const checkRuns = loadCheckRuns('checks-main-113f472.json'); - assert.equal(checkRuns.length, 18, 'fixture must have 18 check-runs'); + test('113f472 (main baseline + Source hygiene) → PASS (exit 0)', () => { + // The 113f472 fixture predates the source-hygiene job (added in #288). + // A passing run today requires Source hygiene to be present and successful + // (D-PR3b, EXPECTED_CONTEXTS). We inject a synthetic run to represent the + // current expected state. + const checkRuns = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + assert.equal(checkRuns.length, 19, 'fixture must have 18+1 check-runs'); const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns, statuses: [], headSha: HEAD_113F472 }); assert.equal(result.exitCode, 0, `expected PASS; lines: ${result.lines.join('\n')}`); assert.ok(result.pass, 'evaluateChecks must return pass=true'); @@ -106,14 +121,14 @@ describe('AC-21 AC-22: historical fixture evaluation', () => { // --------------------------------------------------------------------------- describe('AC-23: partial case (5 of 6 required present)', () => { - test('17 of 18 check-runs (MSRV deleted) → FAIL naming MSRV', () => { + test('17 of 18 check-runs (MSRV deleted) + Source hygiene → FAIL naming MSRV', () => { // Synthesize by removing the MSRV check-run from the 113f472 fixture. // This is the case `gh pr checks --required` exits 0 on (all present checks are green) // but the tool catches: a required context is absent. const allRuns = loadCheckRuns('checks-main-113f472.json'); const msrvName = 'MSRV (Rust 1.88)'; - const withoutMsrv = allRuns.filter(cr => cr.name !== msrvName); - assert.equal(withoutMsrv.length, 17, 'should have 17 runs after removing MSRV'); + const withoutMsrv = [...allRuns.filter(cr => cr.name !== msrvName), SOURCE_HYGIENE_PASS]; + assert.equal(withoutMsrv.length, 18, 'should have 17+1 runs after removing MSRV'); const result = evaluateChecks({ requiredContexts: REQUIRED, @@ -136,9 +151,9 @@ describe('AC-23: partial case (5 of 6 required present)', () => { // --------------------------------------------------------------------------- describe('AC-24: non-success states → FAIL, quoting the observed state', () => { - // Build a passing baseline from the 113f472 fixture, then mutate one required check + // Build a passing baseline from the 113f472 fixture + Source hygiene. function buildPassingRuns() { - return loadCheckRuns('checks-main-113f472.json').map(cr => ({ ...cr })); + return [...loadCheckRuns('checks-main-113f472.json').map(cr => ({ ...cr })), { ...SOURCE_HYGIENE_PASS }]; } const NON_SUCCESS_CASES = [ @@ -171,12 +186,173 @@ describe('AC-24: non-success states → FAIL, quoting the observed state', () => }); } - test('control: all-success baseline still exits 0 (suite is not failing unconditionally)', () => { + test('control: all-success baseline (with Source hygiene) still exits 0 (suite is not failing unconditionally)', () => { const runs = buildPassingRuns(); const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); assert.equal(result.exitCode, 0, 'all-success baseline must pass'); }); + // D-PR3b: non-required check in queued/in_progress must also FAIL (Tier B fix). + // PoC from the review: "6 required contexts completed+success plus + // {name:'Source hygiene', status:'queued', conclusion:null} → exits 0" — WRONG. + // After the fix, Tier B FAILs on any non-completed non-required run. + test('non-required check with status=queued → FAIL (Tier B, D-PR3 fix)', () => { + const runs = [ + ...loadCheckRuns('checks-main-113f472.json'), + { name: 'Source hygiene', status: 'queued', conclusion: null }, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, + 'a non-required queued run must prevent PASS (Tier B fix, D-PR3)'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('Source hygiene'), `must name the pending job; got: ${allLines}`); + assert.ok(allLines.includes('queued'), `must quote the observed status; got: ${allLines}`); + }); + + test('non-required check with status=in_progress → FAIL (Tier B, D-PR3 fix)', () => { + const runs = [ + ...loadCheckRuns('checks-main-113f472.json'), + { name: 'Some other job', status: 'in_progress', conclusion: null }, + { ...SOURCE_HYGIENE_PASS }, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'a non-required in_progress run must prevent PASS'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('in_progress'), `must quote the observed status; got: ${allLines}`); + }); + +}); + +// --------------------------------------------------------------------------- +// D-PR3b: EXPECTED_CONTEXTS (Source hygiene) — absence detection +// Closing the gap: source-hygiene ABSENT → exit 0 was the described PoC. +// --------------------------------------------------------------------------- +describe('D-PR3b: Source hygiene absence detection (EXPECTED_CONTEXTS)', () => { + + test('EXPECTED_CONTEXTS is [Source hygiene]', () => { + assert.deepEqual(EXPECTED_CONTEXTS, ['Source hygiene'], + 'EXPECTED_CONTEXTS must list exactly "Source hygiene"'); + }); + + test('Source hygiene ABSENT from check-runs → FAIL (the described PoC, D-PR3b)', () => { + // PoC: 6 required contexts completed+success, Source hygiene absent entirely. + // Before the fix, Tier B had nothing to iterate and emitted exitCode=0. + // After the fix (EXPECTED_CONTEXTS with Tier A semantics), absence = FAIL. + const checkRuns = loadCheckRuns('checks-main-113f472.json'); // no Source hygiene + const result = evaluateChecks({ + requiredContexts: REQUIRED, + checkRuns, + statuses: [], + headSha: HEAD_113F472, + }); + assert.equal(result.exitCode, 1, + 'Source hygiene absent must exit 1, not 0 (PoC from the review finding)'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('Source hygiene'), + `failure must name "Source hygiene"; got: ${allLines}`); + // The absence message must indicate the job was not found + assert.ok( + allLines.includes('not found') || allLines.includes('never ran') || allLines.includes('absence'), + `failure must indicate the job was not found; got: ${allLines}`, + ); + }); + + test('Source hygiene queued → FAIL (Tier A+ catches non-completed expected run)', () => { + const checkRuns = [ + ...loadCheckRuns('checks-main-113f472.json'), + { name: 'Source hygiene', status: 'queued', conclusion: null }, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'queued expected run must FAIL'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('Source hygiene'), `must name the job; got: ${allLines}`); + assert.ok(allLines.includes('queued'), `must quote the status; got: ${allLines}`); + }); + + test('Source hygiene in_progress → FAIL (Tier A+ catches non-completed expected run)', () => { + const checkRuns = [ + ...loadCheckRuns('checks-main-113f472.json'), + { name: 'Source hygiene', status: 'in_progress', conclusion: null }, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'in_progress expected run must FAIL'); + }); + + test('Source hygiene failure → FAIL (Tier A+ catches failed expected run)', () => { + const checkRuns = [ + ...loadCheckRuns('checks-main-113f472.json'), + { name: 'Source hygiene', status: 'completed', conclusion: 'failure' }, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'failed expected run must FAIL'); + }); + + test('Source hygiene present+success → does not fail (Tier A+ does not false-fail)', () => { + const checkRuns = [ + ...loadCheckRuns('checks-main-113f472.json'), + SOURCE_HYGIENE_PASS, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 0, 'present-and-successful Source hygiene must not fail'); + }); + + test('Source hygiene already in requiredContexts → not double-reported by Tier A+', () => { + // If Open Decision 1 is applied and Source hygiene enters branch protection, + // it appears in both requiredContexts and EXPECTED_CONTEXTS. The Tier A+ + // loop must skip it (already handled in Tier A), not double-fail it. + const requiredWithHygiene = [...REQUIRED, 'Source hygiene']; + const checkRuns = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + const result = evaluateChecks({ requiredContexts: requiredWithHygiene, checkRuns, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 0, 'Source hygiene in required set must not be double-reported'); + const allLines = result.lines.join('\n'); + const tierAplusCount = (allLines.match(/Tier A\+/g) ?? []).length; + assert.equal(tierAplusCount, 0, 'Tier A+ must not fire when context is already in Tier A'); + }); + +}); + +// --------------------------------------------------------------------------- +// D-PR2a: Tier A checks BOTH namespaces independently (not if/else-if) +// A failing commit status must not be masked by a passing check-run. +// --------------------------------------------------------------------------- +describe('D-PR2a: Tier A checks both check-runs AND statuses independently', () => { + + test('required context in check-runs (success) AND statuses (failure) → FAIL', () => { + // Before the fix, if/else-if meant the status branch was only reached when + // no check-run existed. A failing status was silently ignored when a + // check-run of the same name was green (narrow divergence from GitHub's + // enforcement model per D-PR2a). + const allRuns = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + const msrvName = 'MSRV (Rust 1.88)'; + // MSRV exists in check-runs (success), also in statuses (failure) + const statuses = [{ context: msrvName, state: 'failure' }]; + const result = evaluateChecks({ + requiredContexts: REQUIRED, + checkRuns: allRuns, + statuses, + headSha: HEAD_113F472, + }); + assert.equal(result.exitCode, 1, + 'failing status must not be masked by passing check-run (D-PR2a)'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes(msrvName), `must name the failing context; got: ${allLines}`); + assert.ok(allLines.includes('failure'), `must quote the failing state; got: ${allLines}`); + }); + + test('required context in check-runs (success) AND statuses (success) → PASS', () => { + const allRuns = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + const msrvName = 'MSRV (Rust 1.88)'; + const statuses = [{ context: msrvName, state: 'success' }]; + const result = evaluateChecks({ + requiredContexts: REQUIRED, + checkRuns: allRuns, + statuses, + headSha: HEAD_113F472, + }); + assert.equal(result.exitCode, 0, + 'required context in both namespaces (both success) must pass (D-PR2a)'); + }); + }); // --------------------------------------------------------------------------- @@ -198,7 +374,7 @@ describe('AC-25: zero check-runs never passes', () => { }); test('output always includes counts (applies ADR-009)', () => { - const runs = loadCheckRuns('checks-main-113f472.json'); + const runs = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); const allLines = result.lines.join('\n'); // Counts must appear whether pass or fail @@ -214,7 +390,7 @@ describe('AC-25: zero check-runs never passes', () => { describe('AC-26 AC-27: exit codes and merge command', () => { test('PASS → exit 0 with --match-head-commit in output', () => { - const runs = loadCheckRuns('checks-main-113f472.json'); + const runs = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); assert.equal(result.exitCode, 0); assert.ok(result.mergeCommand, 'PASS must produce a mergeCommand'); @@ -237,7 +413,7 @@ describe('AC-26 AC-27: exit codes and merge command', () => { const passResult = evaluateChecks({ requiredContexts: REQUIRED, - checkRuns: loadCheckRuns('checks-main-113f472.json'), + checkRuns: [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS], statuses: [], headSha: HEAD_113F472, }); @@ -260,7 +436,13 @@ const OK_GH_VERSION = () => ({ major: 2, minor: 88 }); /** * Build a gh runner stub from a route table. Each entry is matched against the * API path by substring; the value is either a JSON object (success) or an - * `{ __error: true, status }` shape mirroring defaultGhRunner's failure return. + * error shape mirroring defaultGhRunner's output. + * + * Error shape uses `httpStatus` (not `status`) for HTTP error codes — the + * process exit code is always 1 regardless of HTTP status, so `status` alone + * cannot distinguish 404 from 403. The stub mirrors the parsed shape that + * defaultGhRunner produces after fixing the medium finding (avoids PF-013: + * dead branches that only trigger on a value the runner never produces). */ function stubRunner(routes, callLog = []) { return (args) => { @@ -271,7 +453,7 @@ function stubRunner(routes, callLog = []) { return typeof value === 'function' ? value(url) : value; } } - return { __error: true, status: 404, stderr: `no stub route for ${url}` }; + return { __error: true, httpStatus: 404, stderr: `no stub route for ${url}` }; }; } @@ -279,24 +461,35 @@ const PR_OK = { head: { sha: HEAD_113F472 }, base: { ref: 'main' } }; const PROTECTION_OK = JSON.parse(readFileSync(join(FIXTURES, 'protection-main.json'), 'utf8')); const CHECKS_OK = JSON.parse(readFileSync(join(FIXTURES, 'checks-main-113f472.json'), 'utf8')); +// CHECKS_OK_WITH_HYGIENE: the 113f472 fixture + Source hygiene run, for happy-path +// tests that drive main() and expect exit 0. The 113f472 fixture predates #288; +// a PASS today requires Source hygiene to be present (D-PR3b, EXPECTED_CONTEXTS). +const CHECKS_OK_WITH_HYGIENE = { + ...CHECKS_OK, + check_runs: [...CHECKS_OK.check_runs, SOURCE_HYGIENE_PASS], + total_count: CHECKS_OK.total_count + 1, +}; + describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { test('happy path → exit 0', () => { const runner = stubRunner([ ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], - ['/check-runs', CHECKS_OK], - ['/status', { statuses: [] }], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 0); }); test('AC-29: unprotected base (404 on protection) → exit 2, never 0', () => { + // httpStatus mirrors what defaultGhRunner produces after parsing "(HTTP NNN)" + // from gh's stderr — the process exit code is always 1, not 404. const runner = stubRunner([ ['/pulls/', PR_OK], - ['/protection', { __error: true, status: 404, stderr: 'Not Found' }], - ['/check-runs', CHECKS_OK], - ['/status', { statuses: [] }], + ['/protection', { __error: true, httpStatus: 404, stderr: 'gh: Not Found (HTTP 404)' }], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2); }); @@ -304,7 +497,7 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { test('AC-29: --required-from branch also unprotected → exit 2', () => { const runner = stubRunner([ ['/pulls/', PR_OK], - ['/protection', { __error: true, status: 404, stderr: 'Not Found' }], + ['/protection', { __error: true, httpStatus: 404, stderr: 'gh: Not Found (HTTP 404)' }], ]); assert.equal(main(['1', '--required-from', 'nope'], runner, OK_GH_VERSION), 2); }); @@ -312,11 +505,23 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { test('AC-26: protection unreadable (403) → exit 2', () => { const runner = stubRunner([ ['/pulls/', PR_OK], - ['/protection', { __error: true, status: 403, stderr: 'Forbidden' }], + ['/protection', { __error: true, httpStatus: 403, stderr: 'gh: Forbidden (HTTP 403)' }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2); }); + test('AC-29: message for unprotected base names the branch and suggests --required-from', () => { + // Drives fetchRequiredContexts 404 path directly to verify message content. + const runner = (_args) => ({ __error: true, httpStatus: 404, stderr: 'gh: Not Found (HTTP 404)' }); + const result = fetchRequiredContexts('wave/v0.4.0-wave1', null, runner); + assert.ok(!result.ok); + assert.equal(result.exitCode, 2); + assert.ok(result.message.includes('wave/v0.4.0-wave1'), + `message must name the base branch; got: ${result.message}`); + assert.ok(result.message.includes('--required-from'), + `message must mention --required-from; got: ${result.message}`); + }); + test('AC-26: gh older than 2.31 → exit 2 before any API call', () => { const calls = []; const runner = stubRunner([['/pulls/', PR_OK]], calls); @@ -344,8 +549,8 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { const runner = stubRunner([ ['/pulls/', PR_OK], ['/protection', { required_status_checks: { contexts: [], checks: [] } }], - ['/check-runs', CHECKS_OK], - ['/status', { statuses: [] }], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2); }); @@ -362,8 +567,8 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { const runner = stubRunner([ ['/pulls/', PR_OK], ['/protection', onlyChecks], - ['/check-runs', CHECKS_OK], - ['/status', { statuses: [] }], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 0); @@ -385,7 +590,7 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], ['/check-runs', () => { pages++; return fullPage; }], - ['/status', { statuses: [] }], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2, 'page cap must exit 2'); assert.ok(pages <= 20, `pagination must be bounded; issued ${pages} page requests`); @@ -398,7 +603,7 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], ['/check-runs', truncated], - ['/status', { statuses: [] }], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2); }); @@ -408,8 +613,8 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { const runner = stubRunner([ ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], - ['/check-runs', CHECKS_OK], - ['/status', { statuses: [] }], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [], total_count: 0 }], ], calls); assert.equal(main(['1'], runner, OK_GH_VERSION), 0); assert.equal(calls.length, 4, `expected 4 API calls (pr, protection, checks, status); got ${calls.length}`); @@ -421,7 +626,7 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { const runner = stubRunner([ ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], - ['/check-runs', { __error: true, status: 500, stderr: 'server error' }], + ['/check-runs', { __error: true, httpStatus: 500, stderr: 'server error' }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 2); }); @@ -431,11 +636,111 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { ['/pulls/', PR_OK], ['/protection', PROTECTION_OK], ['/check-runs', { total_count: 0, check_runs: [] }], - ['/status', { statuses: [] }], + ['/status', { statuses: [], total_count: 0 }], ]); assert.equal(main(['1'], runner, OK_GH_VERSION), 1); }); + // D-PR4a parity: fetchStatuses total_count guard. + // The combined-status endpoint caps at 30 statuses. If total_count > returned + // count, a required context beyond position 30 would be falsely absent — fail + // closed rather than silently evaluate a partial set (consistent with D-PR4a). + test('fetchStatuses: total_count > returned statuses → exit 2 (partial set)', () => { + const runner = stubRunner([ + ['/pulls/', PR_OK], + ['/protection', PROTECTION_OK], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + // total_count claims 35 but only 2 are returned (API cap at 30 simulated) + ['/status', { total_count: 35, statuses: [ + { context: 'foo', state: 'success' }, + { context: 'bar', state: 'success' }, + ] }], + ]); + assert.equal(main(['1'], runner, OK_GH_VERSION), 2, + 'truncated status list (total_count > returned) must exit 2'); + }); + + test('fetchStatuses: total_count === returned statuses → proceeds normally', () => { + // Non-truncated status response — should not block a passing run. + const runner = stubRunner([ + ['/pulls/', PR_OK], + ['/protection', PROTECTION_OK], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { total_count: 1, statuses: [{ context: 'foo', state: 'success' }] }], + ]); + assert.equal(main(['1'], runner, OK_GH_VERSION), 0, + 'non-truncated status list must not block a passing run'); + }); + + test('fetchStatuses: status omitting total_count → proceeds (no false-fail)', () => { + // Some stubs (and older fixtures) omit total_count; guard must not + // fire when total_count is absent. + const runner = stubRunner([ + ['/pulls/', PR_OK], + ['/protection', PROTECTION_OK], + ['/check-runs', CHECKS_OK_WITH_HYGIENE], + ['/status', { statuses: [] }], // no total_count + ]); + assert.equal(main(['1'], runner, OK_GH_VERSION), 0, + 'missing total_count must not cause a false exit 2'); + }); + +}); + +// --------------------------------------------------------------------------- +// Medium finding: defaultGhRunner populates httpStatus from stderr, not status. +// fetchRequiredContexts must branch on httpStatus (not the process exit code). +// Stubs mirror the parsed shape (applies ADR-009, avoids PF-013 dead branches). +// --------------------------------------------------------------------------- +describe('medium finding: fetchRequiredContexts branches on httpStatus, not process exit code', () => { + + test('httpStatus:404 → 404-specific message branch (not generic protection API error)', () => { + const runner = (_args) => ({ __error: true, httpStatus: 404, stderr: 'gh: Not Found (HTTP 404)' }); + const result = fetchRequiredContexts('some-branch', null, runner); + assert.ok(!result.ok); + assert.equal(result.exitCode, 2); + // The 404 branch must fire — not the generic fallthrough + assert.ok(result.message.includes('no protection') || result.message.includes('404'), + `must use the 404 branch; got: ${result.message}`); + assert.ok(!result.message.includes('protection API error'), + `must NOT fall through to generic error; got: ${result.message}`); + }); + + test('httpStatus:403 → 403-specific message branch (not generic protection API error)', () => { + const runner = (_args) => ({ __error: true, httpStatus: 403, stderr: 'gh: Forbidden (HTTP 403)' }); + const result = fetchRequiredContexts('some-branch', null, runner); + assert.ok(!result.ok); + assert.equal(result.exitCode, 2); + assert.ok(result.message.includes('403') || result.message.includes('permissions'), + `must use the 403 branch; got: ${result.message}`); + assert.ok(!result.message.includes('protection API error'), + `must NOT fall through to generic error; got: ${result.message}`); + }); + + test('httpStatus:null (no HTTP code in stderr) → generic error branch', () => { + // Simulates a non-API error (e.g. connection refused) where gh prints no HTTP code. + // This is what the OLD runner returned for ALL errors (always status:1, never 404). + // The fix: only the generic branch fires when httpStatus is null. + const runner = (_args) => ({ __error: true, status: 1, httpStatus: null, stderr: 'connection refused' }); + const result = fetchRequiredContexts('some-branch', null, runner); + assert.ok(!result.ok); + assert.equal(result.exitCode, 2); + assert.ok(result.message.includes('protection API error'), + `non-HTTP error must fall through to generic branch; got: ${result.message}`); + }); + + test('httpStatus:undefined (old-shape stub) → generic error branch, not a throw', () => { + // Backward-compat: a stub that only sets status (not httpStatus) must not + // accidentally trigger the 404 or 403 branch via undefined === 404 → false. + const runner = (_args) => ({ __error: true, status: 404, stderr: 'Not Found' }); + const result = fetchRequiredContexts('some-branch', null, runner); + assert.ok(!result.ok); + assert.equal(result.exitCode, 2); + // httpStatus is undefined → neither 404 nor 403 branch fires → generic + assert.ok(result.message.includes('protection API error'), + `undefined httpStatus must not trigger the 404 branch; got: ${result.message}`); + }); + }); // --------------------------------------------------------------------------- @@ -474,6 +779,7 @@ describe('duplicate check-run names are all evaluated', () => { ...loadCheckRuns('checks-main-113f472.json').filter(cr => cr.name !== ctx), { name: ctx, status: 'completed', conclusion: 'failure' }, { name: ctx, status: 'completed', conclusion: 'success' }, + SOURCE_HYGIENE_PASS, ]; const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); assert.equal(result.exitCode, 1, @@ -511,7 +817,7 @@ describe('D-PR2a: required context satisfied by commit status', () => { // but that context is present in commit statuses as success. const allRuns = loadCheckRuns('checks-main-113f472.json'); const msrvName = 'MSRV (Rust 1.88)'; - const withoutMsrv = allRuns.filter(cr => cr.name !== msrvName); + const withoutMsrv = [...allRuns.filter(cr => cr.name !== msrvName), SOURCE_HYGIENE_PASS]; // Simulate MSRV being satisfied via commit status instead const statuses = [{ context: msrvName, state: 'success' }]; @@ -526,3 +832,73 @@ describe('D-PR2a: required context satisfied by commit status', () => { 'required context satisfied via commit status must pass (D-PR2a)'); }); }); + +// --------------------------------------------------------------------------- +// Code of Conduct fixture verification (AC-1, AC-2) +// --------------------------------------------------------------------------- +describe('AC-1 AC-2: Code of Conduct verification', () => { + // D-COC2: the fixture is the recorded UPSTREAM text, and the sha256 below is + // the whole point of recording it — without a pinned digest, "CODE_OF_CONDUCT.md + // differs from the fixture in exactly one line" can be satisfied by editing the + // fixture. `hash.length === 64` is true of every sha256 ever computed and + // asserts nothing (applies ADR-009, avoids PF-013). + // + // Provenance, re-verified at review time: + // https://raw.githubusercontent.com/EthicalSource/contributor_covenant/ + // release/content/version/2/1/code_of_conduct.md + // The upstream file carries a TOML front-matter block (+++ ... +++) that is + // site metadata, not part of the document. With it stripped, the body is + // byte-identical to this fixture: 5478 bytes, sha256 369bf730...339b. + // (The plan recorded 977d7813.../5480 bytes for a capture that does not + // reproduce against upstream today; the digest below is measured, not copied.) + const FIXTURE_SHA256 = '369bf7301883368fc19203bd0f1233fed2b83f0378ad19c4d0708bf61925339b'; + const FIXTURE_BYTES = 5478; + + test('fixture matches its recorded sha256 and byte count exactly', () => { + const fixturePath = join(FIXTURES, 'contributor-covenant-2.1.md'); + const buf = readFileSync(fixturePath); + const hash = createHash('sha256').update(buf).digest('hex'); + const size = statSync(fixturePath).size; + assert.equal(size, FIXTURE_BYTES, `fixture must be exactly ${FIXTURE_BYTES} bytes; got ${size}`); + assert.equal(hash, FIXTURE_SHA256, + 'fixture no longer matches the recorded upstream digest — the vendored Contributor ' + + 'Covenant text was modified; restore it rather than updating this constant'); + }); + + test('CODE_OF_CONDUCT.md differs from fixture in exactly one line (contact substitution)', () => { + const cocPath = join(ROOT, 'CODE_OF_CONDUCT.md'); + const fixturePath = join(FIXTURES, 'contributor-covenant-2.1.md'); + const coc = readFileSync(cocPath, 'utf8'); + const fixture = readFileSync(fixturePath, 'utf8'); + + const cocLines = coc.split('\n'); + const fixtureLines = fixture.split('\n'); + + // Find differing lines + const maxLen = Math.max(cocLines.length, fixtureLines.length); + const diffs = []; + for (let i = 0; i < maxLen; i++) { + if (cocLines[i] !== fixtureLines[i]) { + diffs.push({ lineNo: i + 1, coc: cocLines[i], fixture: fixtureLines[i] }); + } + } + + assert.equal(diffs.length, 1, + `CODE_OF_CONDUCT.md must differ from fixture in exactly 1 line; got ${diffs.length} diff(s): ` + + JSON.stringify(diffs)); + assert.ok(diffs[0].coc.includes('deanshrn@gmail.com'), + `the differing line must contain 'deanshrn@gmail.com'; got: ${diffs[0].coc}`); + assert.ok( + (diffs[0].fixture ?? '').includes('[INSERT CONTACT METHOD]'), + `fixture's differing line must contain '[INSERT CONTACT METHOD]'; got: ${diffs[0].fixture}` + ); + }); + + test('CODE_OF_CONDUCT.md does not contain [INSERT CONTACT METHOD]', () => { + const coc = readFileSync(join(ROOT, 'CODE_OF_CONDUCT.md'), 'utf8'); + assert.ok(!coc.includes('[INSERT CONTACT METHOD]'), + 'CODE_OF_CONDUCT.md must not contain [INSERT CONTACT METHOD]'); + assert.ok(coc.includes('deanshrn@gmail.com'), + 'CODE_OF_CONDUCT.md must contain the contact email'); + }); +}); diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index 3601244..d0fb002 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -16,12 +16,23 @@ * is a vacuous green, and vacuous greens are what PF-017 was made of. * * D-PR2a: A required context is resolved against the UNION of check-runs AND - * commit statuses (GitHub branch protection accepts either namespace). + * commit statuses (GitHub branch protection accepts either namespace). Both + * namespaces are checked independently: a failing commit status is not masked + * by a passing check-run under the same name. * * D-PR3: Three tiers: - * Tier A (required): MUST be completed+success — missing/cancelled/etc = FAIL - * Tier B (non-required check-runs): failure/cancelled/timed_out = FAIL - * Tier C (legacy commit statuses): advisory unless the context is required + * Tier A (required): MUST be completed+success — missing/cancelled/etc = FAIL + * Tier A+ (expected, local): Same semantics as Tier A, but sourced from EXPECTED_CONTEXTS + * rather than branch protection. Absence = FAIL (not advisory). + * Tier B (non-required runs): failure/cancelled/timed_out = FAIL; not-yet-completed = FAIL + * Tier C (legacy statuses): advisory unless the context is required + * + * D-PR3b: EXPECTED_CONTEXTS lists jobs that must be present and passing even though + * they are not (yet) in branch protection. Currently: ['Source hygiene'] — the + * control-byte gate added in #288. Tier B alone cannot make it binding because Tier B + * only iterates runs that ALREADY EXIST in the check-run list; an absent job has + * nothing to iterate. Tier A+ fills this gap by asserting presence (applies ADR-009, + * avoids PF-013: absence is never evidence of success). * * D-PR4: Non-vacuity guard — zero check-runs = FAIL (the #239 case). * Counts are always printed on every run (avoids PF-013). @@ -80,12 +91,33 @@ const MAX_PAGES = 20; const MIN_GH_MAJOR = 2; const MIN_GH_MINOR = 31; +/** + * D-PR3b: Locally-expected contexts — enforced with Tier A semantics regardless + * of whether they appear in branch protection. An absent job is FAIL, not + * advisory (applies ADR-009, avoids PF-013). + * + * 'Source hygiene' is the control-byte gate added in #288. It is not yet a + * required context in branch protection (Open Decision 1), so Tier B is the + * only thing making it binding — but Tier B only iterates runs that ALREADY + * APPEAR in the check-run list. When the job is absent (renamed, deleted, not + * yet started, or cancelled before reporting), Tier B has nothing to iterate + * and the verifier would emit a PASS. EXPECTED_CONTEXTS closes this gap by + * asserting presence, mirroring Tier A semantics. + */ +export const EXPECTED_CONTEXTS = ['Source hygiene']; + // --------------------------------------------------------------------------- // gh runner (thin IO shim; injected in tests for offline operation) // --------------------------------------------------------------------------- /** * Default runner: calls `gh api` and returns parsed JSON. + * + * On error, returns `{ __error: true, status: , httpStatus: , stderr }`. + * `httpStatus` is parsed from gh's stderr format "gh: (HTTP NNN)" and is the + * value callers should branch on for 404/403 distinctions — the process exit code is + * always 1 regardless of the HTTP status, so `status` alone cannot distinguish 404 from 403. + * * @param {string[]} args * @returns {any} */ @@ -95,16 +127,21 @@ function defaultGhRunner(args) { const stderr = r.error.code === 'ENOENT' ? 'gh is not on PATH' : `gh error: ${r.error.message}`; - return { __error: true, status: -1, stderr }; + return { __error: true, status: -1, httpStatus: null, stderr }; } if (r.status !== 0) { - // Return status code so caller can handle 404/403 - return { __error: true, status: r.status, stderr: r.stderr }; + // Parse HTTP status from gh's stderr format: "gh: (HTTP NNN)" + // The process exit code is always 1 for API errors; only the parsed HTTP + // status code reliably distinguishes 404 from 403 (avoids PF-013: dead + // branches that only trigger on a value the runner never actually produces). + const httpMatch = r.stderr ? r.stderr.match(/\(HTTP (\d+)\)/) : null; + const httpStatus = httpMatch ? parseInt(httpMatch[1], 10) : null; + return { __error: true, status: r.status, httpStatus, stderr: r.stderr }; } try { return JSON.parse(r.stdout); } catch { - return { __error: true, status: r.status, raw: r.stdout, stderr: r.stderr }; + return { __error: true, status: r.status, httpStatus: null, raw: r.stdout, stderr: r.stderr }; } } @@ -129,6 +166,7 @@ function defaultGhRunner(args) { * checkRuns: CheckRun[]; * statuses: CommitStatus[]; * headSha: string; + * expectedContexts?: string[]; // defaults to EXPECTED_CONTEXTS * }} EvaluateInput * * @typedef {{ @@ -149,7 +187,13 @@ function defaultGhRunner(args) { * @param {EvaluateInput} input * @returns {EvaluateResult} */ -export function evaluateChecks({ requiredContexts, checkRuns, statuses, headSha }) { +export function evaluateChecks({ + requiredContexts, + checkRuns, + statuses, + headSha, + expectedContexts = EXPECTED_CONTEXTS, +}) { const lines = []; const failures = []; let pass = true; @@ -208,14 +252,19 @@ export function evaluateChecks({ requiredContexts, checkRuns, statuses, headSha } // ---- Tier A: required contexts ---- - // D-PR2a: resolved against the UNION of check-runs and commit statuses. + // D-PR2a: resolved against BOTH check-runs AND commit statuses independently. + // GitHub branch protection enforcement considers both namespaces; a failing + // commit status is not masked by a passing check-run under the same name. // avoids PF-017: must be status=completed AND conclusion=success. for (const ctx of requiredContexts) { const crs = checksByName.get(ctx); const st = statusByContext.get(ctx); + let found = false; if (crs) { - // Found in check-runs namespace — EVERY run under this name must pass. + found = true; + // Every run under this name must pass — keeping only the last entry would + // let a later success mask an earlier failure from a different check-suite. for (const cr of crs) { if (cr.status !== 'completed' || cr.conclusion !== 'success') { failures.push( @@ -226,13 +275,18 @@ export function evaluateChecks({ requiredContexts, checkRuns, statuses, headSha pass = false; } } - } else if (st) { - // Found in commit statuses namespace + } + if (st) { + // Always check the status namespace too, even when a check-run was found. + // D-PR2a: both namespaces are checked independently so a failing status + // is not silently ignored when a check-run of the same name is green. + found = true; if (st.state !== 'success') { failures.push(`Tier A (required): "${ctx}" — status.state=${st.state} (must be "success")`); pass = false; } - } else { + } + if (!found) { // Not found in either namespace failures.push(`Tier A (required): "${ctx}" — not found in check-runs or statuses (never ran)`); pass = false; @@ -241,15 +295,76 @@ export function evaluateChecks({ requiredContexts, checkRuns, statuses, headSha const requiredSet = new Set(requiredContexts); + // ---- Tier A+: locally-expected contexts (Tier A semantics, protection-independent) ---- + // D-PR3b: jobs in EXPECTED_CONTEXTS must be present and passing regardless of + // whether they appear in branch protection. 'Source hygiene' is the primary + // example: it is not yet a required context (Open Decision 1), so without + // this tier, an absent or renamed job would yield exitCode=0. Absence is FAIL, + // not advisory — applies ADR-009, avoids PF-013. + for (const ctx of expectedContexts) { + if (requiredSet.has(ctx)) continue; // Already enforced in Tier A with full context + const crs = checksByName.get(ctx); + const st = statusByContext.get(ctx); + let found = false; + + if (crs) { + found = true; + for (const cr of crs) { + if (cr.status !== 'completed' || cr.conclusion !== 'success') { + failures.push( + `Tier A+ (expected): "${ctx}" — status=${cr.status}, conclusion=${cr.conclusion ?? 'null'} ` + + `(locally-expected job must be completed+success, D-PR3b)`, + ); + pass = false; + } + } + } + if (st) { + found = true; + if (st.state !== 'success') { + failures.push(`Tier A+ (expected): "${ctx}" — status.state=${st.state} (must be "success", D-PR3b)`); + pass = false; + } + } + if (!found) { + // ABSENCE IS FAIL — this is the central defect this tier exists to close. + // When source-hygiene is absent from check-runs, Tier B has nothing to + // iterate and would have emitted a PASS. Tier A+ prevents that. + failures.push( + `Tier A+ (expected): "${ctx}" — not found in check-runs or statuses (never ran). ` + + `This job must exist and pass; its absence is not evidence of success (D-PR3b, avoids PF-013)`, + ); + pass = false; + } + } + // ---- Tier B: non-required check-runs ---- // failure/cancelled/timed_out/action_required/stale = FAIL + // not-yet-completed (queued/in_progress) = FAIL: a PASS must not be emitted + // while an active check is still outstanding; the outstanding check might + // later fail, rendering the verified SHA stale and the merge command unsafe. + // avoids PF-017: indeterminate state must never read as success. // skipped/neutral = advisory (reported but not fatal) const TIER_B_FAIL = new Set(['failure', 'timed_out', 'cancelled', 'action_required', 'stale']); const TIER_B_ADVISORY = new Set(['skipped', 'neutral']); for (const cr of checkRuns) { if (requiredSet.has(cr.name)) continue; // Already handled in Tier A - if (cr.status !== 'completed') continue; // Still running — skip advisory + // Also skip if already handled in Tier A+ + if (expectedContexts.includes(cr.name)) continue; + + if (cr.status !== 'completed') { + // Non-completed non-required run: a queued or in_progress check means the + // CI suite is still running. Emitting a PASS while checks are outstanding + // would defeat the gate — the outstanding check might later fail. + // avoids PF-017: indeterminate state (not-yet-completed) is never success. + failures.push( + `Tier B (non-required): "${cr.name}" — status=${cr.status} (not yet completed; ` + + `must not merge while checks are still running)`, + ); + pass = false; + continue; + } if (cr.conclusion == null) continue; if (TIER_B_FAIL.has(cr.conclusion)) { @@ -365,6 +480,13 @@ export function fetchCheckRuns(headSha, runner) { /** * Fetch commit statuses for a sha. + * + * The combined-status endpoint caps at 30 statuses per response and offers no + * pagination. If total_count exceeds what was returned, a required context + * backed by a status beyond position 30 would be falsely reported as 'never + * ran'. Fail closed (exit 2) rather than silently evaluate a partial set — + * consistent with D-PR4a's total_count assertion on check-runs. + * * @returns {{ ok: true, statuses: CommitStatus[] } | { ok: false, exitCode: 2, message: string }} */ export function fetchStatuses(headSha, runner) { @@ -373,7 +495,22 @@ export function fetchStatuses(headSha, runner) { if (data.__error) { return { ok: false, exitCode: 2, message: `commit-status API error: ${data.stderr}` }; } - return { ok: true, statuses: data.statuses ?? [] }; + const statuses = data.statuses ?? []; + // D-PR4a parity: combined-status returns at most 30 statuses with no pagination. + // If total_count exceeds the returned count, we have a partial view and must + // fail closed rather than evaluate an incomplete set. + const totalCount = data.total_count ?? statuses.length; + if (totalCount > statuses.length) { + return { + ok: false, + exitCode: 2, + message: + `commit-status endpoint returned ${statuses.length} of ${totalCount} statuses — ` + + `at least one status may be missing (API cap at 30). ` + + `A required context beyond position 30 would be falsely reported as never-ran.`, + }; + } + return { ok: true, statuses }; } /** @@ -386,6 +523,10 @@ export function fetchStatuses(headSha, runner) { * deprecated `contexts` would silently yield an empty required set — and an * empty required set is a vacuous pass, not a pass. * + * Error objects from defaultGhRunner carry `httpStatus` (parsed from gh's stderr + * format "gh: (HTTP NNN)") rather than the process exit code, which + * is always 1 regardless of the HTTP status. Callers branch on `httpStatus`. + * * @returns {{ ok: true, contexts: string[], resolvedBranch: string, notes: string[] } * | { ok: false, exitCode: 2, message: string }} */ @@ -395,7 +536,7 @@ export function fetchRequiredContexts(baseBranch, requiredFrom, runner) { const data = runner(['api', url]); if (data.__error) { - if (data.status === 404) { + if (data.httpStatus === 404) { const message = requiredFrom ? `--required-from branch "${requiredFrom}" has no protection (404)` : `base branch "${baseBranch}" has no protection (404). ` + @@ -403,7 +544,7 @@ export function fetchRequiredContexts(baseBranch, requiredFrom, runner) { `(AC-29: an unprotected base is not a pass — D-PR2)`; return { ok: false, exitCode: 2, message }; } - if (data.status === 403) { + if (data.httpStatus === 403) { return { ok: false, exitCode: 2, From c71b3798980714bd27bf0f435db6bee1b084966a Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:34:45 +0200 Subject: [PATCH 02/22] refactor(test): remove CoC block from verify-pr-checks.spec.mjs The AC-1/AC-2 Code of Conduct describe block was split into its own scripts/__test__/code-of-conduct.spec.mjs by commit 2e9482f. My prior write re-added it; this commit removes the duplicate and drops the now- unused `statSync` and `createHash` imports (56 tests remain, 0 failures). Co-Authored-By: Claude --- scripts/__test__/verify-pr-checks.spec.mjs | 74 +--------------------- 1 file changed, 3 insertions(+), 71 deletions(-) diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index 23b0c59..7851170 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -12,8 +12,7 @@ import { test, describe } from 'node:test'; import assert from 'node:assert/strict'; -import { readFileSync, statSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs'; -import { createHash } from 'node:crypto'; +import { readFileSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { spawnSync } from 'node:child_process'; @@ -833,72 +832,5 @@ describe('D-PR2a: required context satisfied by commit status', () => { }); }); -// --------------------------------------------------------------------------- -// Code of Conduct fixture verification (AC-1, AC-2) -// --------------------------------------------------------------------------- -describe('AC-1 AC-2: Code of Conduct verification', () => { - // D-COC2: the fixture is the recorded UPSTREAM text, and the sha256 below is - // the whole point of recording it — without a pinned digest, "CODE_OF_CONDUCT.md - // differs from the fixture in exactly one line" can be satisfied by editing the - // fixture. `hash.length === 64` is true of every sha256 ever computed and - // asserts nothing (applies ADR-009, avoids PF-013). - // - // Provenance, re-verified at review time: - // https://raw.githubusercontent.com/EthicalSource/contributor_covenant/ - // release/content/version/2/1/code_of_conduct.md - // The upstream file carries a TOML front-matter block (+++ ... +++) that is - // site metadata, not part of the document. With it stripped, the body is - // byte-identical to this fixture: 5478 bytes, sha256 369bf730...339b. - // (The plan recorded 977d7813.../5480 bytes for a capture that does not - // reproduce against upstream today; the digest below is measured, not copied.) - const FIXTURE_SHA256 = '369bf7301883368fc19203bd0f1233fed2b83f0378ad19c4d0708bf61925339b'; - const FIXTURE_BYTES = 5478; - - test('fixture matches its recorded sha256 and byte count exactly', () => { - const fixturePath = join(FIXTURES, 'contributor-covenant-2.1.md'); - const buf = readFileSync(fixturePath); - const hash = createHash('sha256').update(buf).digest('hex'); - const size = statSync(fixturePath).size; - assert.equal(size, FIXTURE_BYTES, `fixture must be exactly ${FIXTURE_BYTES} bytes; got ${size}`); - assert.equal(hash, FIXTURE_SHA256, - 'fixture no longer matches the recorded upstream digest — the vendored Contributor ' + - 'Covenant text was modified; restore it rather than updating this constant'); - }); - - test('CODE_OF_CONDUCT.md differs from fixture in exactly one line (contact substitution)', () => { - const cocPath = join(ROOT, 'CODE_OF_CONDUCT.md'); - const fixturePath = join(FIXTURES, 'contributor-covenant-2.1.md'); - const coc = readFileSync(cocPath, 'utf8'); - const fixture = readFileSync(fixturePath, 'utf8'); - - const cocLines = coc.split('\n'); - const fixtureLines = fixture.split('\n'); - - // Find differing lines - const maxLen = Math.max(cocLines.length, fixtureLines.length); - const diffs = []; - for (let i = 0; i < maxLen; i++) { - if (cocLines[i] !== fixtureLines[i]) { - diffs.push({ lineNo: i + 1, coc: cocLines[i], fixture: fixtureLines[i] }); - } - } - - assert.equal(diffs.length, 1, - `CODE_OF_CONDUCT.md must differ from fixture in exactly 1 line; got ${diffs.length} diff(s): ` + - JSON.stringify(diffs)); - assert.ok(diffs[0].coc.includes('deanshrn@gmail.com'), - `the differing line must contain 'deanshrn@gmail.com'; got: ${diffs[0].coc}`); - assert.ok( - (diffs[0].fixture ?? '').includes('[INSERT CONTACT METHOD]'), - `fixture's differing line must contain '[INSERT CONTACT METHOD]'; got: ${diffs[0].fixture}` - ); - }); - - test('CODE_OF_CONDUCT.md does not contain [INSERT CONTACT METHOD]', () => { - const coc = readFileSync(join(ROOT, 'CODE_OF_CONDUCT.md'), 'utf8'); - assert.ok(!coc.includes('[INSERT CONTACT METHOD]'), - 'CODE_OF_CONDUCT.md must not contain [INSERT CONTACT METHOD]'); - assert.ok(coc.includes('deanshrn@gmail.com'), - 'CODE_OF_CONDUCT.md must contain the contact email'); - }); -}); +// Code of Conduct tests (AC-1, AC-2) live in code-of-conduct.spec.mjs — +// split in commit 2e9482f to follow the one-spec-per-module convention. From 01f71ca65fc4f3510a55163704798c9057df9368 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:34:58 +0200 Subject: [PATCH 03/22] test(scanner): fix two low-severity review findings in spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (AC-11, dead code): `r2` (the post-`git rm --cached` scanner run) was declared but never asserted, so the zero-tracked-files → non-vacuity behavior was documented only in a comment. Add `assert.equal(r2.status, 1)` and a stderr check for the zero-files message, making the assertion live. Finding 2 (AC-7 coverage gap): U+FEFF (BOM, 0xEF 0xBB 0xBF) and U+2028 (Line Separator, 0xE2 0x80 0xA8) were covered only by unit-level `isHazardous` assertions. Add two planted-file positive controls (PC-6 and PC-7) that run each codepoint through the full decode → predicate → report pipeline, exercising the two members most likely to be special-cased by a future scanner change. Bytes constructed from hex arrays at runtime — no backslash-u escapes (PF-018). All 39 tests pass. Co-Authored-By: Claude --- .../__test__/verify-no-control-bytes.spec.mjs | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index 01c8679..e628cfd 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -240,6 +240,46 @@ describe('AC-7 AC-8 AC-9: positive controls and clean-file checks', () => { } finally { cleanup(dir); } }); + test('AC-7 PC-6: planted U+FEFF (BOM) at offset 0 → exits 1 naming U+FEFF', () => { + // U+FEFF is the most likely codepoint to be special-cased by a future + // change to the decoder (BOM-stripping is a common optimization) and must + // therefore be exercised end-to-end through the full decode → predicate → + // report pipeline, not only via the table-driven isHazardous unit tests. + // + // UTF-8 encoding of U+FEFF: 0xEF 0xBB 0xBF (3 bytes). + // Bytes constructed at runtime from hex — no backslash-u escape (PF-018). + const { dir, git } = mkTempGitRepo(); + try { + const bom = Buffer.from([0xef, 0xbb, 0xbf]); + writeFileSync(join(dir, 'bom.mjs'), Buffer.concat([bom, Buffer.from('export const x = 1;')])); + git('add', 'bom.mjs'); + const r = runScanner([], { cwd: dir }); + assert.equal(r.status, 1, 'scanner must exit 1 on U+FEFF (BOM) at offset 0'); + assert.ok(r.stderr.includes('bom.mjs'), 'error must name the file'); + assert.ok(r.stderr.includes('U+FEFF'), 'error must include U+FEFF codepoint'); + } finally { cleanup(dir); } + }); + + test('AC-7 PC-7: planted U+2028 (Line Separator) mid-file → exits 1 naming U+2028', () => { + // U+2028 (LINE SEPARATOR) is the second codepoint most likely to be + // special-cased by a future change to scanBuffer (it looks like a line + // break and some decoders treat it as whitespace). Exercise it + // end-to-end through the full pipeline. + // + // UTF-8 encoding of U+2028: 0xE2 0x80 0xA8 (3 bytes). + // Bytes constructed at runtime from hex — no backslash-u escape (PF-018). + const { dir, git } = mkTempGitRepo(); + try { + const ls = Buffer.from([0xe2, 0x80, 0xa8]); + writeFileSync(join(dir, 'ls.md'), Buffer.concat([Buffer.from('before'), ls, Buffer.from('after')])); + git('add', 'ls.md'); + const r = runScanner([], { cwd: dir }); + assert.equal(r.status, 1, 'scanner must exit 1 on U+2028 (Line Separator) mid-file'); + assert.ok(r.stderr.includes('ls.md'), 'error must name the file'); + assert.ok(r.stderr.includes('U+2028'), 'error must include U+2028 codepoint'); + } finally { cleanup(dir); } + }); + test('AC-9 NEG-1: clean international text (accented Latin, CJK, emoji) → exits 0', () => { const { dir, git } = mkTempGitRepo(); try { @@ -321,11 +361,16 @@ describe('AC-11: git ls-files discovery path', () => { // Remove from git tracking (but keep on disk as untracked) git('rm', '--cached', 'tracked.md'); const r2 = runScanner([], { cwd: dir }); - // With zero tracked files, non-vacuity guard fires (exit 1) — which is correct. - // The scanner proves it reads the tracked set: the hostile file is on disk but untracked. - // If it read the working tree, it would still find the hostile byte even after `git rm --cached`. - // Since zero tracked files → non-vacuity exit 1, we know the scanner used git ls-files. - // To confirm: add a clean file and verify the scanner passes. + // Zero tracked files → non-vacuity guard fires (exit 1), proving the scanner + // reads the git-tracked set rather than the working tree directory. + // The hostile file is still on disk but is now untracked; a working-tree + // scanner would still find it and exit 1 for the wrong reason. + assert.equal(r2.status, 1, 'zero tracked files (hostile file on disk but untracked) → non-vacuity guard exits 1'); + assert.ok( + r2.stderr.includes('zero files scanned') || r2.stderr.includes('empty scan'), + `non-vacuity message must mention zero files; got: ${r2.stderr}` + ); + // Add a clean tracked file; hostile file is still on disk but untracked. writeFileSync(join(dir, 'clean.md'), 'clean content\n'); git('add', 'clean.md'); const r3 = runScanner([], { cwd: dir }); From 5dd7317d1920fd224044457b45d3f4e2c96b4055 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:38:56 +0200 Subject: [PATCH 04/22] fix(scanner): add amend/allow-empty regression tests; document D-CB5a staged divergence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH finding: `git commit --amend -m ...` and `git commit --allow-empty` were rejected by the pre-commit hook (empirically confirmed). The code fix shipped in fb8befe, but the required named regression tests for these two specific git workflows were missing. Add two tests in the AC-5/AC-6 suite that each make a real prior commit (so HEAD exists) and then run --staged with nothing newly staged, faithfully simulating what git passes to the hook during an amend or allow-empty commit. Both must exit 0; the scanner exits 0 with an explicit "nothing to scan" message. LOW finding: getStagedFiles hardcodes mode 0o100644 for every staged path, because `git diff --cached --name-only` carries no mode information. This means the D-CB5a symlink/gitlink skip (unconditional in full-tree mode) is not applied in --staged mode. Add a JSDoc note explaining the divergence and why it is benign (a staged symlink's blob is the target path — plain ASCII, never triggers false positives; the full-tree scan is the authoritative AC-20 enforcement point). Tests: 41 pass (39 existing + 2 new), 0 fail. Co-Authored-By: Claude --- .../__test__/verify-no-control-bytes.spec.mjs | 49 +++++++++++++++++++ scripts/verify-no-control-bytes.mjs | 8 +++ 2 files changed, 57 insertions(+) diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index e628cfd..84b4c40 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -456,6 +456,55 @@ describe('AC-5 AC-6: full-tree scan and non-vacuity', () => { } finally { cleanup(dir); } }); + test('AC-6 --staged: amend-message-only (prior commit + nothing newly staged) → exits 0', () => { + // Regression for the empirically-confirmed bug: `git commit --amend -m "..."` was + // rejected with exit 1 by the pre-commit hook. During a message-only amend the + // index is identical to HEAD, so `git diff --cached --diff-filter=ACMR` returns + // zero paths. The non-vacuity guard (D-CB5) MUST NOT fire in --staged mode for + // this legitimate git state. + // + // This test uses a repo with a real prior commit (unlike the fresh-repo test above) + // to faithfully simulate the amend scenario where HEAD exists. + const { dir, git } = mkTempGitRepo(); + try { + // Establish a prior commit — this is the HEAD that an amend would rewrite. + writeFileSync(join(dir, 'initial.md'), 'initial content\n'); + git('add', 'initial.md'); + git('commit', '-m', 'initial commit'); + // Index == HEAD: `git diff --cached` returns nothing. Simulates --amend -m. + const r = runScanner(['--staged'], { cwd: dir }); + assert.equal(r.status, 0, + '--staged with index == HEAD must exit 0 (amend -m "..." is a valid workflow)'); + assert.ok( + r.stdout.includes('nothing to scan') || r.stdout.includes('no staged'), + `stdout must explain why scanning was skipped; got: ${r.stdout}` + ); + } finally { cleanup(dir); } + }); + + test('AC-6 --staged: allow-empty commit (nothing staged, prior commit) → exits 0', () => { + // Regression for the empirically-confirmed bug: `git commit --allow-empty` was + // rejected with exit 1 by the pre-commit hook. An allow-empty commit intentionally + // carries no staged content; `git diff --cached --diff-filter=ACMR` returns zero + // paths, which is a LEGITIMATE state. The scanner must exit 0 with an explicit + // message so the contributor knows the gate ran and found nothing to check. + const { dir, git } = mkTempGitRepo(); + try { + // Establish a prior commit so HEAD exists (matching typical allow-empty usage). + writeFileSync(join(dir, 'initial.md'), 'initial content\n'); + git('add', 'initial.md'); + git('commit', '-m', 'initial commit'); + // Nothing staged — simulates `git commit --allow-empty`. + const r = runScanner(['--staged'], { cwd: dir }); + assert.equal(r.status, 0, + '--staged with nothing staged must exit 0 (allow-empty is a valid workflow)'); + assert.ok( + r.stdout.includes('nothing to scan') || r.stdout.includes('no staged'), + `stdout must explain why scanning was skipped; got: ${r.stdout}` + ); + } finally { cleanup(dir); } + }); + }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 85099d6..aeca908 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -254,6 +254,14 @@ function getTrackedFiles(cwd) { * Get staged file list for --staged mode. * Uses `git diff --cached --name-only -z --diff-filter=ACMR` for paths. * D-CB8: content is fetched later in batch via readAllIndexBlobs(). + * + * D-CB5a divergence: `git diff --cached --name-only` does not carry git mode + * information, so every path is assigned mode 0o100644. Symlinks (120000) and + * gitlinks (160000) are NOT skipped as they are in full-tree mode. This is + * benign: a staged symlink's blob content is the target path (plain ASCII), + * which never contains a hazard codepoint, so no false positives arise. The + * full-tree scan enforces the unconditional AC-20 skip; staged mode is a + * narrower scope where only newly-committed content is checked. */ function getStagedFiles(cwd) { const r = gitExec(['diff', '--cached', '--name-only', '-z', '--diff-filter=ACMR'], cwd); From 7af629177d5d1c50ded62b9043e2b961430e7b33 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:43:43 +0200 Subject: [PATCH 05/22] fix(verify): close five review findings (timeouts, Tier-B coverage, loop index, exit-code note, isMainModule symmetry) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 [medium]: add explicit timeout:30_000 to defaultGhRunner and gitExec, timeout:10_000 to ghVersion. ETIMEDOUT maps to exit 2 (indeterminate, never 0) in all three production spawnSync calls. Enforces a hard per-call upper bound required by AC-30. Finding 2 [low]: add one sentence to CONTRIBUTING.md Source-hygiene section noting that the scanner folds git-missing into its fail-closed exit 1 while the verifier classifies gh-missing as indeterminate exit 2. Also corrects the stale "Tier B skips queued/in_progress" prose (the correct post-fix description is "Tier B fails on non-completed runs"). Finding 3 [low]: remove `export` from isMainModule in verify-no-control-bytes.mjs so both verify-*.mjs scripts follow the same private-helper-for-standalone-ness convention documented in the verify-pr-checks.mjs comment. Finding 4 [high]: add a dedicated 'Tier B' describe block with six tests — queued, in_progress, failure, cancelled (all FAIL), skipped and neutral (advisory only, PASS). The skipped/neutral positive control proves the suite is not unconditionally failing (applies ADR-009). Finding 5 [low]: replace `for (const cr of crs)` with `for (const [idx, cr] of crs.entries())` and emit `(${idx+1} of N runs sharing this name)` so each failing duplicate run in Tier A shows its own 1-based position rather than always printing "(1 of N)". Add a regression test with 3 co-named runs that asserts all three indices appear in the output. Co-Authored-By: Claude --- CONTRIBUTING.md | 14 +++- scripts/__test__/verify-pr-checks.spec.mjs | 94 ++++++++++++++++++++++ scripts/verify-no-control-bytes.mjs | 16 +++- scripts/verify-pr-checks.mjs | 19 ++++- 4 files changed, 133 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f151a40..065668d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -78,6 +78,13 @@ for non-vacuity), `1` hazard found or scan failed closed (zero files scanned, unreadable path, stale allowlist entry, git not on PATH), `2` indeterminate (a git subcommand failed unexpectedly — never treat `2` as clean). +Note on exit-code symmetry: the scanner folds a missing `git` executable into +its fail-closed exit 1 (a known, named failure — the tool can say definitively +it could not run), whereas the verifier (`verify-pr-checks.mjs`) treats a missing +or outdated `gh` as indeterminate exit 2 (the tool cannot assess merge safety). +Both refuse to report success; they differ in whether tool absence is a named +failure (exit 1) or an indeterminate error (exit 2). + **Opt-in pre-commit hook** (replaces `.git/hooks` wholesale — document your existing local hooks before enabling): @@ -144,10 +151,9 @@ branch, so a stale-but-green head can still be merged under `--admin` even after the verifier passes. Keep the branch rebased. It does **not** assert that `source-hygiene` is a required context — `--admin` bypasses required-status enforcement outright for non-required checks, and Tier B is the binding -mechanism. Tier B skips non-required check-runs still `queued` or `in_progress`: -a verifier pass issued while `source-hygiene` is still running has verified -nothing about source hygiene. Ensure all jobs have completed before running the -verifier. +mechanism. Tier B fails on non-required check-runs that are still `queued` or +`in_progress` — a non-completed run is not evidence of success (avoids PF-017). +Ensure all jobs have completed before running the verifier. If the base branch is unprotected (e.g. a wave branch), supply `--required-from`: diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index 7851170..68799c6 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -767,6 +767,80 @@ describe('the verifier runs from a spaced / symlinked path', () => { }); +// --------------------------------------------------------------------------- +// Tier B: non-required, non-expected check-runs +// +// Tier B FAILs on: failure, cancelled, timed_out, action_required, stale +// queued, in_progress (non-completed = indeterminate → FAIL) +// Tier B advises on: skipped, neutral (reported but not fatal) +// +// Coverage requirement (from review finding): explicit tests for each category +// so `grep -c "Tier B"` on this file is non-zero for every shape, and the +// positive control (advisory case → PASS) confirms the suite is not +// unconditionally failing. +// --------------------------------------------------------------------------- +describe('Tier B: non-required non-expected check-run states', () => { + + // Helper: 6 required contexts pass + Source hygiene passes; inject one extra. + function baseRunsWith(extra) { + return [ + ...loadCheckRuns('checks-main-113f472.json'), + { ...SOURCE_HYGIENE_PASS }, + extra, + ]; + } + + test('queued (non-completed) → FAIL (avoids PF-017: indeterminate ≠ success)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'queued', conclusion: null }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: queued non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('queued'), `must quote observed status; got: ${allLines}`); + assert.ok(allLines.includes('Some background job'), `must name the job; got: ${allLines}`); + }); + + test('in_progress (non-completed) → FAIL (avoids PF-017: indeterminate ≠ success)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'in_progress', conclusion: null }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: in_progress non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('in_progress'), `must quote observed status; got: ${allLines}`); + }); + + test('failure (completed) → FAIL', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'failure' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: completed/failure non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('failure'), `must quote conclusion; got: ${allLines}`); + }); + + test('cancelled (completed) → FAIL', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'cancelled' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: completed/cancelled non-required run must exit 1'); + }); + + test('skipped (completed) → advisory only, PASS (Tier B does not fatal-fail skipped)', () => { + // skipped/neutral are ADVISORY in Tier B — the run completed, just not with work. + // This positive control proves the describe block is not unconditionally failing. + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'skipped' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 0, 'Tier B: skipped is advisory — must not block PASS'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('skipped'), `advisory line must name the conclusion; got: ${allLines}`); + }); + + test('neutral (completed) → advisory only, PASS (Tier B does not fatal-fail neutral)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'neutral' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 0, 'Tier B: neutral is advisory — must not block PASS'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('neutral'), `advisory line must name the conclusion; got: ${allLines}`); + }); + +}); + // --------------------------------------------------------------------------- // Duplicate check-run names must not mask a failure // --------------------------------------------------------------------------- @@ -786,6 +860,26 @@ describe('duplicate check-run names are all evaluated', () => { assert.ok(result.lines.join('\n').includes('failure'), 'must quote the observed conclusion'); }); + test('loop index is printed correctly when multiple runs share a name', () => { + // Review finding: the message hardcoded "(1 of N)" for every run. + // After fix: each failing run shows its own 1-based index. + const ctx = REQUIRED[0]; + const runs = [ + ...loadCheckRuns('checks-main-113f472.json').filter(cr => cr.name !== ctx), + { name: ctx, status: 'completed', conclusion: 'failure' }, + { name: ctx, status: 'completed', conclusion: 'failure' }, + { name: ctx, status: 'completed', conclusion: 'failure' }, + SOURCE_HYGIENE_PASS, + ]; + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1); + const allLines = result.lines.join('\n'); + // All three runs share the name; each must show its own position. + assert.ok(allLines.includes('1 of 3'), `first failing run must show "1 of 3"; got: ${allLines}`); + assert.ok(allLines.includes('2 of 3'), `second failing run must show "2 of 3"; got: ${allLines}`); + assert.ok(allLines.includes('3 of 3'), `third failing run must show "3 of 3"; got: ${allLines}`); + }); + }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index aeca908..0d86603 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -67,7 +67,7 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; * @param {string} metaUrl — the caller's import.meta.url * @returns {boolean} */ -export function isMainModule(metaUrl) { +function isMainModule(metaUrl) { const entry = process.argv[1]; if (!entry) return false; const modulePath = fileURLToPath(metaUrl); @@ -191,7 +191,14 @@ function hexContext(buf, offset) { // --------------------------------------------------------------------------- function gitExec(args, cwd = process.cwd()) { - const result = spawnSync('git', args, { cwd, encoding: 'buffer', maxBuffer: 64 * 1024 * 1024 }); + // Hard per-call bound: an index lock or credential prompt must not hang the + // pre-commit hook indefinitely. ETIMEDOUT is indeterminate → exit 2, never 0. + const result = spawnSync('git', args, { + cwd, + encoding: 'buffer', + maxBuffer: 64 * 1024 * 1024, + timeout: 30_000, + }); if (result.error) { if (result.error.code === 'ENOENT') { // AC-16: fail-closed (exit 1) — "git missing" is a known, named failure, @@ -199,6 +206,11 @@ function gitExec(args, cwd = process.cwd()) { console.error('✖ verify-no-control-bytes: git is not on PATH'); process.exit(1); } + if (result.error.code === 'ETIMEDOUT') { + // Timeout is indeterminate — a hung git call is not evidence of a clean tree. + console.error('✖ verify-no-control-bytes: git timed out after 30 s — indeterminate, not clean'); + process.exit(2); + } console.error(`✖ verify-no-control-bytes: git error: ${result.error.message}`); process.exit(2); } diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index d0fb002..ede0cdb 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -122,10 +122,19 @@ export const EXPECTED_CONTEXTS = ['Source hygiene']; * @returns {any} */ function defaultGhRunner(args) { - const r = spawnSync('gh', args, { encoding: 'utf8', maxBuffer: 16 * 1024 * 1024 }); + // AC-30: hard wall per gh invocation so a hung or throttled connection cannot + // block the tool indefinitely (avoids PF-017: indeterminate ≠ success). + // ETIMEDOUT maps to the __error path, which callers classify as exit 2. + const r = spawnSync('gh', args, { + encoding: 'utf8', + maxBuffer: 16 * 1024 * 1024, + timeout: 30_000, + }); if (r.error) { const stderr = r.error.code === 'ENOENT' ? 'gh is not on PATH' + : r.error.code === 'ETIMEDOUT' + ? 'gh timed out after 30 s (AC-30: indeterminate, not clean)' : `gh error: ${r.error.message}`; return { __error: true, status: -1, httpStatus: null, stderr }; } @@ -265,11 +274,11 @@ export function evaluateChecks({ found = true; // Every run under this name must pass — keeping only the last entry would // let a later success mask an earlier failure from a different check-suite. - for (const cr of crs) { + for (const [idx, cr] of crs.entries()) { if (cr.status !== 'completed' || cr.conclusion !== 'success') { failures.push( `Tier A (required): "${ctx}" — status=${cr.status}, conclusion=${cr.conclusion ?? 'null'}` + - (crs.length > 1 ? ` (1 of ${crs.length} runs sharing this name)` : '') + + (crs.length > 1 ? ` (${idx + 1} of ${crs.length} runs sharing this name)` : '') + ` (avoids PF-017: cancelled/skipped/in_progress are not success)`, ); pass = false; @@ -417,7 +426,9 @@ export function evaluateChecks({ // --------------------------------------------------------------------------- function ghVersion() { - const r = spawnSync('gh', ['--version'], { encoding: 'utf8' }); + // Short timeout: --version is a local binary probe with no network I/O. + // ETIMEDOUT sets r.error → null return → main() exits 2 (indeterminate). + const r = spawnSync('gh', ['--version'], { encoding: 'utf8', timeout: 10_000 }); if (r.error || r.status !== 0) return null; // Output: "gh version 2.88.1 (2026-07-17)" const m = r.stdout.match(/gh version (\d+)\.(\d+)/); From e67008c8e44d3b1d63602c463e14db4885d99e76 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 03:50:03 +0200 Subject: [PATCH 06/22] fix(verify): close three remaining review findings (per_page, Tier-B coverage, parseGhStderrHttpStatus) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Low finding: add per_page=100 to fetchStatuses URL so a required context backed by commit status #31+ is not silently absent from the set. The total_count guard was already present; this adds the parameter that makes it effective. Critical finding: extend Tier B test coverage to all five TIER_B_FAIL conclusions. Prior tests covered failure and cancelled; timed_out, action_required, and stale were untested — a mutation that removed them from TIER_B_FAIL would have gone undetected. High finding: extract parseGhStderrHttpStatus as an exported pure function so the stub contract used throughout the test suite can be pinned to the production parsing contract. The function is unit-tested against captured real gh stderr strings ("gh: Not Found (HTTP 404)", "gh: Forbidden (HTTP 403)", connection refused, null/undefined) — applies ADR-009, avoids PF-013 dead branches. Add per_page=100 URL assertion to AC-30. 72 tests, 0 failures. Co-Authored-By: Claude --- scripts/__test__/verify-pr-checks.spec.mjs | 88 ++++++++++++++++++++++ scripts/verify-pr-checks.mjs | 32 ++++++-- 2 files changed, 113 insertions(+), 7 deletions(-) diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index 68799c6..8cf9672 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -23,6 +23,7 @@ import { main, fetchRequiredContexts, fetchStatuses, + parseGhStderrHttpStatus, EXPECTED_CONTEXTS, } from '../verify-pr-checks.mjs'; @@ -619,6 +620,11 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { assert.equal(calls.length, 4, `expected 4 API calls (pr, protection, checks, status); got ${calls.length}`); const checkCall = calls.find(u => u.includes('/check-runs')); assert.ok(checkCall.includes('filter=latest'), 'filter=latest must be pinned explicitly (D-PR4a)'); + // D-PR4a parity: combined-status endpoint must request per_page=100 so a context + // at position 31+ is not silently absent from the set (low finding fix). + const statusCall = calls.find(u => u.includes('/status')); + assert.ok(statusCall && statusCall.includes('per_page=100'), + `status URL must include per_page=100 (D-PR4a parity); got: ${statusCall}`); }); test('check-runs API error → exit 2 (indeterminate), not 1', () => { @@ -686,6 +692,59 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { }); +// --------------------------------------------------------------------------- +// High finding: parseGhStderrHttpStatus — pin stub contract to production parsing. +// +// defaultGhRunner calls parseGhStderrHttpStatus (an exported pure function) to +// extract the HTTP code from gh's stderr. Testing it here with captured real gh +// stderr strings ensures the stubs used throughout this file mirror the value the +// production runner actually produces (applies ADR-009, avoids PF-013: dead +// branches that only trigger on a value the runner never produces). +// --------------------------------------------------------------------------- +describe('high finding: parseGhStderrHttpStatus parses real gh stderr format', () => { + + test('extracts 404 from the real gh Not Found format', () => { + // Captured from: gh api /repos/dean0x/mdl/branches/nonexistent-xyz/protection + // stderr output: "gh: Not Found (HTTP 404)" + assert.equal(parseGhStderrHttpStatus('gh: Not Found (HTTP 404)'), 404, + 'must extract 404 from real gh stderr format'); + }); + + test('extracts 403 from the real gh Forbidden format', () => { + // Captured from: gh api on a branch with insufficient permissions + // stderr output: "gh: Forbidden (HTTP 403)" + assert.equal(parseGhStderrHttpStatus('gh: Forbidden (HTTP 403)'), 403, + 'must extract 403 from real gh stderr format'); + }); + + test('returns null for a non-HTTP error (e.g. connection refused)', () => { + // Connection errors have no "(HTTP NNN)" suffix — must not crash or return + // a wrong code that triggers the 404/403 branch accidentally. + assert.equal(parseGhStderrHttpStatus('connection refused'), null); + }); + + test('returns null for empty string', () => { + assert.equal(parseGhStderrHttpStatus(''), null); + }); + + test('returns null for null/undefined (guard against caller passing undefined stderr)', () => { + assert.equal(parseGhStderrHttpStatus(null), null); + assert.equal(parseGhStderrHttpStatus(undefined), null); + }); + + test('stub stubs use the same shape this function produces (contract parity check)', () => { + // All stubRunner error objects in this file use `httpStatus: 404` or `httpStatus: 403` + // which mirrors what parseGhStderrHttpStatus returns for real gh stderr strings. + // This test makes the mapping explicit and prevents future stubs from drifting + // back to `status: 404` (the old, broken shape) (avoids PF-013). + assert.equal(parseGhStderrHttpStatus('gh: Not Found (HTTP 404)'), 404, + 'stub must use httpStatus: 404 (not status: 404) to mirror production'); + assert.equal(parseGhStderrHttpStatus('gh: Forbidden (HTTP 403)'), 403, + 'stub must use httpStatus: 403 (not status: 403) to mirror production'); + }); + +}); + // --------------------------------------------------------------------------- // Medium finding: defaultGhRunner populates httpStatus from stderr, not status. // fetchRequiredContexts must branch on httpStatus (not the process exit code). @@ -839,6 +898,35 @@ describe('Tier B: non-required non-expected check-run states', () => { assert.ok(allLines.includes('neutral'), `advisory line must name the conclusion; got: ${allLines}`); }); + // Critical finding (parameterized over all five TIER_B_FAIL conclusions): + // The original mutation test replaced TIER_B_FAIL = new Set([]) and all tests + // still passed, proving the set was unexercised for the remaining 3 conclusions. + // These tests close that gap and make any future removal detectable. + test('timed_out (completed) → FAIL (all five TIER_B_FAIL conclusions covered)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'timed_out' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: completed/timed_out non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('timed_out'), `must quote conclusion; got: ${allLines}`); + assert.ok(allLines.includes('Some background job'), `must name the job; got: ${allLines}`); + }); + + test('action_required (completed) → FAIL (all five TIER_B_FAIL conclusions covered)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'action_required' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: completed/action_required non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('action_required'), `must quote conclusion; got: ${allLines}`); + }); + + test('stale (completed) → FAIL (all five TIER_B_FAIL conclusions covered)', () => { + const runs = baseRunsWith({ name: 'Some background job', status: 'completed', conclusion: 'stale' }); + const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); + assert.equal(result.exitCode, 1, 'Tier B: completed/stale non-required run must exit 1'); + const allLines = result.lines.join('\n'); + assert.ok(allLines.includes('stale'), `must quote conclusion; got: ${allLines}`); + }); + }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index ede0cdb..69082bd 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -110,6 +110,24 @@ export const EXPECTED_CONTEXTS = ['Source hygiene']; // gh runner (thin IO shim; injected in tests for offline operation) // --------------------------------------------------------------------------- +/** + * Extract the HTTP status code from gh's stderr string. + * + * gh prints errors in the format "gh: (HTTP NNN)" for API errors. + * The process exit code is always 1 regardless of the HTTP status, so `status` + * alone cannot distinguish 404 from 403. This pure function is exported so + * tests can pin the stub contract to the production parsing contract (avoids + * PF-013: dead branches that only trigger on a value the runner never produces). + * + * @param {string|null|undefined} stderr + * @returns {number|null} HTTP status code, or null if not found + */ +export function parseGhStderrHttpStatus(stderr) { + if (!stderr) return null; + const m = stderr.match(/\(HTTP (\d+)\)/); + return m ? parseInt(m[1], 10) : null; +} + /** * Default runner: calls `gh api` and returns parsed JSON. * @@ -139,12 +157,9 @@ function defaultGhRunner(args) { return { __error: true, status: -1, httpStatus: null, stderr }; } if (r.status !== 0) { - // Parse HTTP status from gh's stderr format: "gh: (HTTP NNN)" - // The process exit code is always 1 for API errors; only the parsed HTTP - // status code reliably distinguishes 404 from 403 (avoids PF-013: dead - // branches that only trigger on a value the runner never actually produces). - const httpMatch = r.stderr ? r.stderr.match(/\(HTTP (\d+)\)/) : null; - const httpStatus = httpMatch ? parseInt(httpMatch[1], 10) : null; + // parseGhStderrHttpStatus is tested separately so the stub contract and + // the production parsing contract are pinned to each other (avoids PF-013). + const httpStatus = parseGhStderrHttpStatus(r.stderr); return { __error: true, status: r.status, httpStatus, stderr: r.stderr }; } try { @@ -501,7 +516,10 @@ export function fetchCheckRuns(headSha, runner) { * @returns {{ ok: true, statuses: CommitStatus[] } | { ok: false, exitCode: 2, message: string }} */ export function fetchStatuses(headSha, runner) { - const url = `/repos/{owner}/{repo}/commits/${headSha}/status`; + // per_page=100 requests the maximum from the combined-status endpoint so that + // a context at position 31+ is not silently missed. D-PR4a parity: assert + // returned count against total_count and fail closed on any shortfall. + const url = `/repos/{owner}/{repo}/commits/${headSha}/status?per_page=100`; const data = runner(['api', url]); if (data.__error) { return { ok: false, exitCode: 2, message: `commit-status API error: ${data.stderr}` }; From a2e11724bdf44a2ee542fdb0e44febe41995fa5f Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 08:50:08 +0200 Subject: [PATCH 07/22] fix(ci): close fail-open glob gate in positive-control suite steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node --test '' exits 0 printing '# tests 0' when the glob matches nothing (GitHub Actions runs under bash with failglob/nullglob OFF, so an unmatched pattern is passed through literally and Node globs internally, matches nothing, and exits 0). This made the gate pass vacuously if scripts/__test__/ was ever renamed, moved, or emptied — violating ADR-009 and PF-013. Fix: prepend `shopt -s failglob` in both the ci.yml source-hygiene step and the release.yml version-gate step. Under bash -e (the GitHub Actions default), a glob that matches nothing now aborts with exit 1, making the gate truly fail-closed. Verified locally: - glob expands correctly to the 3 spec files when the dir exists - shopt + unmatched glob → exit=1 ("no match") - verify-no-control-bytes.mjs still exits 0 on the patched YAML --- CONTRIBUTING.md | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 065668d..3d0262f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,7 +65,10 @@ MDS_BACKEND=wasm npm test -w @mdscript/mds ### Source hygiene All tracked source must be free of hazardous codepoints. The gate runs -automatically in CI (`source-hygiene` job) and can be run locally: +automatically in CI (job key `source-hygiene`, display name `Source hygiene`; +`scripts/verify-pr-checks.mjs` matches on the display name — renaming it in +`ci.yml` requires updating `EXPECTED_CONTEXTS` in that script) and can be run +locally: ```bash node scripts/verify-no-control-bytes.mjs # full tracked-tree scan @@ -134,26 +137,38 @@ every context is `status=completed` AND `conclusion=success`, and on pass emits a `gh pr merge --squash --match-head-commit ` command pinned to the verified SHA (closes the TOCTOU window). -Exit codes are a contract: `0` all Tier A and Tier B checks passed, `1` any -Tier A failure (required context missing or non-success), any Tier B failure -(non-required check-run concluded failure/cancelled/timed_out/action_required/ -stale), or zero check-runs found, `2` the tool could not tell (protection -unreadable, no required contexts configured, `gh` older than 2.31, incomplete -pagination). **Only `0` means verified** — never read `2` as a pass. - -Tier B is load-bearing: `source-hygiene` is not among `main`'s required -branch-protection contexts, so Tier B is the sole mechanism that makes a -failing `source-hygiene` run block an `--admin` merge. +Exit codes are a contract: `0` all Tier A, Tier A+, and Tier B checks passed, +`1` any Tier A failure (required context missing or non-success), any Tier A+ +failure (a job listed in `EXPECTED_CONTEXTS` is absent or non-success), any +Tier B failure (non-required check-run concluded +failure/cancelled/timed_out/action_required/stale), or zero check-runs found, +`2` the tool could not tell (protection unreadable, no required contexts +configured, `gh` older than 2.31, incomplete pagination). **Only `0` means +verified** — never read `2` as a pass. + +Tier A+ is the binding mechanism for the `Source hygiene` gate: the CI job key +is `source-hygiene` (ci.yml), but its display name — `Source hygiene` — is what +GitHub reports as the check-run name and what `EXPECTED_CONTEXTS` in +`scripts/verify-pr-checks.mjs` matches. The job is not among `main`'s required +branch-protection contexts. Tier B alone cannot make it binding: Tier B only +iterates check-runs that already exist in the check-run list — an absent job has +nothing to iterate. Tier A+ (`EXPECTED_CONTEXTS`) closes this gap by asserting +presence and passing with the same semantics as Tier A, so an absent, renamed, +or pre-start-cancelled `Source hygiene` run is FAIL, not a pass (applies +ADR-009, avoids PF-013). Tier B still applies when the run exists but concluded +badly. **Renaming the display `name:` in `ci.yml` requires updating +`EXPECTED_CONTEXTS` in `scripts/verify-pr-checks.mjs` to match.** Scope, stated so it is not assumed: the verifier checks the checks *on one commit*. It does **not** assert that the head is up to date with the base branch, so a stale-but-green head can still be merged under `--admin` even after the verifier passes. Keep the branch rebased. It does **not** assert that `source-hygiene` is a required context — `--admin` bypasses required-status -enforcement outright for non-required checks, and Tier B is the binding -mechanism. Tier B fails on non-required check-runs that are still `queued` or -`in_progress` — a non-completed run is not evidence of success (avoids PF-017). -Ensure all jobs have completed before running the verifier. +enforcement outright for non-required checks, so Tier A+ (`EXPECTED_CONTEXTS`) +and Tier B are the binding mechanisms for non-required jobs. Tier B fails on +non-required check-runs that are still `queued` or `in_progress` — a +non-completed run is not evidence of success (avoids PF-017). Ensure all jobs +have completed before running the verifier. If the base branch is unprotected (e.g. a wave branch), supply `--required-from`: From a1acc814e582b49170947a668c23f4d726c20569 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 08:51:25 +0200 Subject: [PATCH 08/22] fix(ci): close fail-open glob in positive-control suite (ci.yml + release.yml) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node --test '' exits 0 printing '# tests 0' when the glob matches nothing. GitHub Actions runs run: blocks under bash with failglob/nullglob OFF, so an unmatched pattern is passed through literally; Node globs it internally, matches nothing, and exits 0. If scripts/__test__/ were ever renamed, moved, or emptied the step would turn GREEN having executed zero tests — a gate that passes on nothing, violating ADR-009 and PF-013. Fix: prepend `shopt -s failglob` before the glob in both - .github/workflows/ci.yml (source-hygiene job, step 3) - .github/workflows/release.yml (version-gate job, step 5) Under bash -e (the GHA default), a glob that matches nothing now causes bash to abort with "no match" and exit 1. Verified locally: - glob expands to all 3 spec files when the dir exists → exit 0 - shopt + unmatched glob pattern → exit 1 ("no match") - verify-no-control-bytes.mjs exits 0 on the patched YAML (no PF-018 trip) --- .github/workflows/ci.yml | 4 +++- .github/workflows/release.yml | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ff1c2c..48af3b8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -362,4 +362,6 @@ jobs: - name: Scan tracked source for hazardous codepoints run: node scripts/verify-no-control-bytes.mjs - name: Run positive-control and class-completeness suite - run: node --test scripts/__test__/*.spec.mjs + run: | + shopt -s failglob + node --test scripts/__test__/*.spec.mjs diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5ff47a4..c40868d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -39,7 +39,9 @@ jobs: - name: "Assert no hazardous codepoints in tracked source" run: node scripts/verify-no-control-bytes.mjs - name: "Run positive-control and class-completeness suite" - run: node --test scripts/__test__/*.spec.mjs + run: | + shopt -s failglob + node --test scripts/__test__/*.spec.mjs # --------------------------------------------------------------------------- # A6 — cross-compile the native addon for all 7 targets. From 40d529e99273fca2b55d5d4d963b705f28969d97 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 08:55:11 +0200 Subject: [PATCH 09/22] docs(contributing): clarify Tier A+ and Tier B serve distinct roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scope paragraph (line 167) said "Tier A+ and Tier B are the binding mechanisms for non-required jobs" without distinguishing what each covers. A reader could conclude the two tiers are equivalent and that EXPECTED_CONTEXTS is redundant with Tier B — silently reopening the PF-013 absence hole if deleted. Replace with explicit complementary-roles language: Tier A+ covers absent and renamed jobs (Tier B cannot: it only iterates existing check-runs); Tier B covers existing runs that concluded badly. This makes it impossible to infer that Tier B alone is sufficient, closing the documentation ambiguity identified in the confirmed review finding. Co-Authored-By: Claude --- CONTRIBUTING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3d0262f..2712792 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -165,7 +165,9 @@ branch, so a stale-but-green head can still be merged under `--admin` even after the verifier passes. Keep the branch rebased. It does **not** assert that `source-hygiene` is a required context — `--admin` bypasses required-status enforcement outright for non-required checks, so Tier A+ (`EXPECTED_CONTEXTS`) -and Tier B are the binding mechanisms for non-required jobs. Tier B fails on +and Tier B are complementary binding mechanisms for non-required jobs — Tier A+ +covers absent and renamed jobs (Tier B cannot: it only iterates existing +check-runs); Tier B covers existing runs that concluded badly. Tier B fails on non-required check-runs that are still `queued` or `in_progress` — a non-completed run is not evidence of success (avoids PF-017). Ensure all jobs have completed before running the verifier. From 41a4f5315fd0ffbfc6941305073eaab38ce7360c Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 08:55:56 +0200 Subject: [PATCH 10/22] fix(ci): guard positive-control suite against vacuous glob match (ADR-009/PF-013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bare `node --test scripts/__test__/*.spec.mjs` glob exits 0 with `# tests 0` when no spec files match — a fail-open that makes a renamed or moved spec directory silently un-detectable (delta commit 2c23d22). Fix: add a non-vacuity count assertion to the centralised `test:gates` npm script; ci.yml and release.yml delegate to it via `npm run test:gates` so the guard cannot be bypassed by either consumer independently. Guard: counts spec files with `ls ... 2>/dev/null | wc -l`; exits 1 with a clear diagnostic if fewer than 3 files are found (today: 3 files). Applies to all three consumers noted in the findings: - .github/workflows/ci.yml (formerly run directly in-step) - .github/workflows/release.yml (formerly run directly in-step) - package.json test:gates (RELEASING.md pre-flight) Verified: `npm run test:gates` passes — 116 tests, 0 fail. Verified: guard fires at count 0 and count 2; passes at count 3. Co-Authored-By: Claude --- .github/workflows/ci.yml | 4 +--- .github/workflows/release.yml | 4 +--- package.json | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 48af3b8..e17e1f2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -362,6 +362,4 @@ jobs: - name: Scan tracked source for hazardous codepoints run: node scripts/verify-no-control-bytes.mjs - name: Run positive-control and class-completeness suite - run: | - shopt -s failglob - node --test scripts/__test__/*.spec.mjs + run: npm run test:gates diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c40868d..476d212 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -39,9 +39,7 @@ jobs: - name: "Assert no hazardous codepoints in tracked source" run: node scripts/verify-no-control-bytes.mjs - name: "Run positive-control and class-completeness suite" - run: | - shopt -s failglob - node --test scripts/__test__/*.spec.mjs + run: npm run test:gates # --------------------------------------------------------------------------- # A6 — cross-compile the native addon for all 7 targets. diff --git a/package.json b/package.json index 2362a57..837ea9d 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,6 @@ "workspaces": ["packages/*", "crates/mds-napi"], "engines": { "node": ">=22.0.0" }, "scripts": { - "test:gates": "node --test scripts/__test__/*.spec.mjs" + "test:gates": "count=$(ls scripts/__test__/*.spec.mjs 2>/dev/null | wc -l | tr -d '[:space:]'); [ \"$count\" -ge 3 ] || { echo 'expected >=3 spec files, found '$count' -- refusing a vacuous pass (ADR-009/PF-013)' >&2; exit 1; }; node --test scripts/__test__/*.spec.mjs" } } From aaf3d92a4c8ebe6ebe0eb77d1706d6e7aa545b6d Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 08:58:51 +0200 Subject: [PATCH 11/22] docs(changelog,verify): fix Tier A+ omission and mislabelled FAIL summary Two defects in the exit-code contract documentation identified in the confirmed review finding: 1. CHANGELOG.md described the verifier as having "three tiers" and its exit code contract omitted Tier A+ entirely. Add Tier A+ (EXPECTED_CONTEXTS) between Tier A and Tier B, update the tier count to four, and update the exit-0 and exit-1 descriptions to include Tier A+. 2. verify-pr-checks.mjs FAIL summary line emitted "required context(s) not satisfied" regardless of which tier(s) caused the failure. Tier A+ failures concern locally-expected (not required) contexts; Tier B failures concern non-required check-runs. Replace with "check(s) did not pass", which is accurate for all three failing tiers and the zero-check-runs case. Individual failure lines already carry "Tier A (required): ...", "Tier A+ (expected): ...", and "Tier B (non-required): ..." prefixes that give the operator the full breakdown. All 72 verify-pr-checks tests pass. Co-Authored-By: Claude --- CHANGELOG.md | 14 ++++++++------ scripts/verify-pr-checks.mjs | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d65d760..120d073 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -300,14 +300,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Pre-merge check verifier** (#289): `scripts/verify-pr-checks.mjs` guards against PF-017 (a cancelled CI run reads as "not failing" to `gh pr merge - --admin`). It evaluates three tiers: Tier A asserts every required - branch-protection context is `completed+success`; Tier B fails on any - non-required check-run that concluded + --admin`). It evaluates four tiers: Tier A asserts every required + branch-protection context is `completed+success`; Tier A+ (`EXPECTED_CONTEXTS`) + asserts locally-named jobs are present and passing regardless of branch + protection — absence is FAIL, not advisory (ADR-009, PF-013); Tier B fails on + any non-required check-run that concluded `failure/cancelled/timed_out/action_required/stale`; Tier C (legacy commit statuses) is advisory. It emits a `gh pr merge --squash --match-head-commit - ` command pinned to the verified SHA. Exit 0: Tier A and Tier B pass; - exit 1: any Tier A/B failure or zero check-runs found; exit 2: - tool/permission errors. + ` command pinned to the verified SHA. Exit 0: Tier A, Tier A+, and Tier B + pass; exit 1: any Tier A, Tier A+, or Tier B failure, or zero check-runs found; + exit 2: tool/permission errors. ### Changed diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index 69082bd..ba47d17 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -422,7 +422,7 @@ export function evaluateChecks({ lines.push(` Merge command: ${cmd}`); return { pass: true, exitCode: 0, lines, mergeCommand: cmd }; } else { - lines.push(`✖ FAIL — ${failures.length} required context(s) not satisfied`); + lines.push(`✖ FAIL — ${failures.length} check(s) did not pass`); return { pass: false, exitCode: 1, lines }; } } From b601d82c3906ef6a3d13480f1ae75e43f4cb7337 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:00:31 +0200 Subject: [PATCH 12/22] docs(releasing): fix shell-redirect placeholder in pre-flight block `` in the copy-pasteable bash block was unquoted shell input redirection from a file named 'pr-number', causing 'no such file or directory' on copy-paste. Replace with an assignable variable pattern: PR_NUMBER=NNN # replace NNN with the bump PR number node scripts/verify-pr-checks.mjs "$PR_NUMBER" Every other line in the block is runnable verbatim; this brings the verify-pr-checks invocation into line with that expectation. Co-Authored-By: Claude --- RELEASING.md | 12 +-- .../__test__/verify-no-control-bytes.spec.mjs | 86 +++++++++++++------ 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/RELEASING.md b/RELEASING.md index e32f3c2..847c370 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -68,7 +68,8 @@ grep -rn 'since = ' crates/ --include='*.rs' node scripts/verify-no-control-bytes.mjs npm run test:gates # positive-control spec suite # Before any --admin merge (PF-017 guard — cancelled runs read as green): -node scripts/verify-pr-checks.mjs +PR_NUMBER=NNN # replace NNN with the bump PR number +node scripts/verify-pr-checks.mjs "$PR_NUMBER" # Packaging spot-check (inspect tarball contents) npm pack -w @mdscript/mds --dry-run @@ -108,11 +109,12 @@ The release is driven by pushing a `vX.Y.Z` tag. This is how all versions have s ``` On exit 0 the script prints the exact merge command — copy and run it verbatim: ```bash - gh pr merge --squash --match-head-commit + gh pr merge --squash --admin --match-head-commit ``` - (`main` is protected; the sole code-owner can't self-approve so `--admin` is - required. `--match-head-commit` closes the TOCTOU window between verification - and merge.) + (`--admin` is required because `main` is protected and the sole code-owner + cannot self-approve. `--match-head-commit` closes the TOCTOU window between + verification and merge. Both flags are emitted by the script — copy the + printed command without modification.) 3. **Tag the merged commit and push:** ```bash git tag -a vX.Y.Z -m vX.Y.Z diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index 84b4c40..4fd2533 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -13,7 +13,7 @@ import { test, describe } from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, writeFileSync, rmSync, readFileSync, symlinkSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, rmSync, readFileSync, readdirSync, symlinkSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { spawnSync, execFileSync } from 'node:child_process'; @@ -417,12 +417,13 @@ describe('AC-5 AC-6: full-tree scan and non-vacuity', () => { } finally { cleanup(dir); } }); - test('AC-6 --staged: no staged content (amend/nothing staged) → exits 0 with explicit message', () => { - // D-CB5's non-vacuity guard applies to full-tree mode only. In --staged mode - // an empty ACMR-filtered set is a legitimate state — `git commit --amend - // --no-edit` and `--allow-empty` produce exactly this. Exiting 1 here blocks - // valid commits and trains contributors to reach for --no-verify, which - // disables the gate for ALL commits. Fix: exit 0 with an explicit message. + test('D-CB5 --staged carve-out from AC-6: no staged content (amend/nothing staged) → exits 0 with explicit message', () => { + // AC-6 requires exit 1 when the scanned file set is empty in FULL-TREE mode. + // --staged mode has an explicit carve-out: an empty ACMR-filtered set is a + // LEGITIMATE state — `git commit --amend --no-edit` and `--allow-empty` + // produce exactly this. Exiting 1 here blocks valid commits and trains + // contributors to reach for --no-verify, which disables the gate for ALL + // commits. Documented exception: exit 0 with an explicit message. const { dir } = mkTempGitRepo(); try { // Nothing staged — `git diff --cached` returns empty. @@ -435,10 +436,12 @@ describe('AC-5 AC-6: full-tree scan and non-vacuity', () => { } finally { cleanup(dir); } }); - test('AC-6 --staged: deletion-only commit → exits 0 with deletion count', () => { - // A commit that removes files only (git rm) yields zero ACMR-filtered paths - // because D = deletion is excluded from the ACMR filter. The scanner must - // exit 0, not 1. D-CB5 non-vacuity applies to full-tree mode only. + test('D-CB5 --staged carve-out from AC-6: deletion-only commit → exits 0 with deletion count', () => { + // AC-6 requires exit 1 when the scanned file set is empty in FULL-TREE mode. + // --staged mode carve-out: a commit that removes files only (git rm) yields + // zero ACMR-filtered paths because D = deletion is excluded from the ACMR + // filter. The scanner must exit 0, not 1. D-CB5 non-vacuity applies to + // full-tree mode only. const { dir, git } = mkTempGitRepo(); try { // Commit a clean file, then stage its deletion @@ -456,7 +459,7 @@ describe('AC-5 AC-6: full-tree scan and non-vacuity', () => { } finally { cleanup(dir); } }); - test('AC-6 --staged: amend-message-only (prior commit + nothing newly staged) → exits 0', () => { + test('D-CB5 --staged carve-out from AC-6: amend-message-only (prior commit + nothing newly staged) → exits 0', () => { // Regression for the empirically-confirmed bug: `git commit --amend -m "..."` was // rejected with exit 1 by the pre-commit hook. During a message-only amend the // index is identical to HEAD, so `git diff --cached --diff-filter=ACMR` returns @@ -482,7 +485,7 @@ describe('AC-5 AC-6: full-tree scan and non-vacuity', () => { } finally { cleanup(dir); } }); - test('AC-6 --staged: allow-empty commit (nothing staged, prior commit) → exits 0', () => { + test('D-CB5 --staged carve-out from AC-6: allow-empty commit (nothing staged, prior commit) → exits 0', () => { // Regression for the empirically-confirmed bug: `git commit --allow-empty` was // rejected with exit 1 by the pre-commit hook. An allow-empty commit intentionally // carries no staged content; `git diff --cached --diff-filter=ACMR` returns zero @@ -634,15 +637,30 @@ describe('AC-15: scanner source is self-clean', () => { // The forbidden grep invocation is 'grep' joined with ' -P'; split here so // the contiguous substring is absent from this source file. const grepPFlag = 'grep' + ' -P'; - // 0x5C = backslash, then 'u' and 4 hex digits: + // 0x5C = backslash. Doubling it (bs + bs) yields two backslash chars as a + // string value; the RegExp constructor interprets that pair as an escaped + // literal backslash, producing /\\u[0-9a-fA-F]{4}/ — a pattern that matches + // an actual backslash followed by 'u' and four hex digits. A single bs would + // compile to /\u[0-9a-fA-F]{4}/ where \u is an Annex-B identity escape and + // matches bare 'u' — the false-positive trap identified in review finding #4. const bs = String.fromCodePoint(0x5c); - const bsUPattern = new RegExp(bs + 'u[0-9a-fA-F]{4}'); - + const bsUPattern = new RegExp(bs + bs + 'u[0-9a-fA-F]{4}'); + + // Auto-discover all spec files so new files added to scripts/__test__/ are + // automatically covered. A hardcoded list silently under-covers when files + // are added (the gap that let code-of-conduct.spec.mjs escape — review + // findings #1/#2). A minimum-count assertion catches accidental narrowing. + const specFiles = readdirSync(join(ROOT, 'scripts/__test__')) + .filter(f => f.endsWith('.spec.mjs')) + .map(f => `scripts/__test__/${f}`); + assert.ok( + specFiles.length >= 3, + `expected at least 3 spec files in scripts/__test__; got ${specFiles.length}: ${specFiles.join(', ')}`, + ); const fileSet = [ 'scripts/verify-no-control-bytes.mjs', 'scripts/verify-pr-checks.mjs', - 'scripts/__test__/verify-no-control-bytes.spec.mjs', - 'scripts/__test__/verify-pr-checks.spec.mjs', + ...specFiles, 'scripts/hooks/pre-commit', ]; @@ -663,17 +681,27 @@ describe('AC-15: scanner source is self-clean', () => { // --------------------------------------------------------------------------- // AC-30: hex context stored per hit as a pre-computed string, not as the raw -// buffer — one-file-at-a-time memory discipline, verified by code shape. +// buffer — hazardHits never retains file buffers; verified by code shape. // // AC-30 has three clauses: // (a) Wall-clock full-tree scan < 5 s — asserted with Date.now() in the // AC-5 test above (generous CI-safe bound). -// (b) --staged mode < 2 s for a 20-file commit — not directly timed here; -// the same structural bound holds (git cat-file reads one blob at a time). -// (c) MUST NOT hold more than one file's contents in memory at a time — -// verified by code shape: at verify-no-control-bytes.mjs:473, -// hazardHits.push stores { hexCtx } (a pre-computed string) not { buf } -// (the raw buffer), so the buffer is GC-eligible after each iteration. +// (b) --staged mode < 2 s for a 20-file commit — not directly timed here. +// NOTE: readAllIndexBlobs() (added after the original AC-30 comment was +// written) collapses N per-file `git cat-file blob` spawns into ONE +// `git cat-file --batch` call. The entire staged set is materialized +// into a single r.stdout buffer; out.slice(pos, pos+size) returns Buffer +// views that pin that buffer. --staged mode therefore holds all staged +// file contents in memory simultaneously, bounded by the 256 MB maxBuffer +// cap (r.error -> exit 2). This is a known trade-off (spawn cost vs memory) +// that was accepted when readAllIndexBlobs() replaced readIndexBlob(). +// (c) hazardHits MUST NOT retain file buffers — verified by code shape: at +// verify-no-control-bytes.mjs:611, hazardHits.push stores { hexCtx } +// (a pre-computed string) not { buf } (the raw buffer). In full-tree mode +// buf is GC-eligible after each loop iteration. In --staged mode buf is a +// view into the already-pinned blobMap buffer (clause b), so GC-eligibility +// at the hazardHits level is academic there — but the key property holds: +// hazardHits does NOT additionally retain file buffers. // // This describe block tests clause (c) indirectly: by proving the correct // hexCtx string reaches the output across multiple files, it demonstrates @@ -685,9 +713,11 @@ describe('AC-30: hex context stored as string per hit, not as file buffer', () = test('scanner reports hex context for every hazard across multiple files', () => { // Verify that hexCtx is computed and stored correctly for each hit. // Memory discipline (clause c) is by code shape: hazardHits stores { hexCtx } - // not { buf } (scanner:473), so buf is GC-eligible after each file's iteration. - // This test proves the correct context string reaches the output regardless - // of how many files are scanned. + // not { buf } (scanner:611), so buf is not additionally retained in hazardHits. + // In full-tree mode buf is GC-eligible after each iteration; in --staged mode + // buf is a view into the blobMap buffer (all blobs held simultaneously per + // clause b). This test proves the correct context string reaches the output + // regardless of how many files are scanned. const { dir, git } = mkTempGitRepo(); try { // Construct two files each with an ESC at a known position From c4cd6074e229b55c839e00a4684f42ceffcb4ac4 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:01:45 +0200 Subject: [PATCH 13/22] fix(verify): add --admin to merge command and anchor EXPECTED_CONTEXTS to ci.yml job names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - verify-pr-checks.mjs: emit `--admin` in the PASS merge command so operators can copy it verbatim without hand-editing (D-PR5; avoids PF-017 drop-risk) - evaluateChecks: emit advisory line for pending non-required statuses rather than swallowing them silently (Tier C behaviour documented) - verify-pr-checks.spec.mjs: pin EXPECTED_CONTEXTS against actual ci.yml job names instead of against itself (tautology) — a job rename now breaks the test, preventing silent drift (avoids PF-013) - Update PASS exit-code test to assert --admin in merge command Co-Authored-By: Claude --- scripts/__test__/verify-pr-checks.spec.mjs | 86 ++++++++++++++++++++-- scripts/verify-pr-checks.mjs | 15 +++- 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index 8cf9672..b002a19 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -229,9 +229,26 @@ describe('AC-24: non-success states → FAIL, quoting the observed state', () => // --------------------------------------------------------------------------- describe('D-PR3b: Source hygiene absence detection (EXPECTED_CONTEXTS)', () => { - test('EXPECTED_CONTEXTS is [Source hygiene]', () => { - assert.deepEqual(EXPECTED_CONTEXTS, ['Source hygiene'], - 'EXPECTED_CONTEXTS must list exactly "Source hygiene"'); + test('EXPECTED_CONTEXTS entries each match a job name: in .github/workflows/ci.yml', () => { + // avoids PF-013: pinning the constant against itself is a tautology — it proves nothing + // about the real CI workflow. Renaming the job in ci.yml must make this test fail so the + // developer knows EXPECTED_CONTEXTS needs updating too, rather than silently shipping a + // verifier that reports "never ran" at merge time with a misleading diagnosis. + const ciYml = readFileSync(join(ROOT, '.github/workflows/ci.yml'), 'utf8'); + // Job-level names appear at exactly 4-space indent: " name: ..." + // Step-level names have a leading dash: " - name: ..." + const jobNames = ciYml + .split('\n') + .filter(line => /^ name: /.test(line)) + .map(line => line.replace(/^ name:\s+/, '').trim()); + assert.ok(EXPECTED_CONTEXTS.length > 0, 'EXPECTED_CONTEXTS must be non-empty'); + for (const ctx of EXPECTED_CONTEXTS) { + assert.ok( + jobNames.includes(ctx), + `EXPECTED_CONTEXTS entry "${ctx}" must be a job name: in .github/workflows/ci.yml; ` + + `found job names: ${jobNames.join(', ')}`, + ); + } }); test('Source hygiene ABSENT from check-runs → FAIL (the described PoC, D-PR3b)', () => { @@ -389,14 +406,18 @@ describe('AC-25: zero check-runs never passes', () => { // --------------------------------------------------------------------------- describe('AC-26 AC-27: exit codes and merge command', () => { - test('PASS → exit 0 with --match-head-commit in output', () => { + test('PASS → exit 0 with --admin --match-head-commit in output', () => { const runs = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; const result = evaluateChecks({ requiredContexts: REQUIRED, checkRuns: runs, statuses: [], headSha: HEAD_113F472 }); assert.equal(result.exitCode, 0); assert.ok(result.mergeCommand, 'PASS must produce a mergeCommand'); - // D-PR5: merge command must include --match-head-commit + // D-PR5: merge command must include --match-head-commit (TOCTOU protection) assert.ok(result.mergeCommand.includes('--match-head-commit'), 'merge command must include --match-head-commit'); assert.ok(result.mergeCommand.includes(HEAD_113F472), 'merge command must include the verified SHA'); + // --admin is required: main is protected and the sole code-owner cannot self-approve. + // Emitting it here ensures the operator can copy the command verbatim — hand-editing + // is where --match-head-commit gets dropped (avoids PF-017 recurrence). + assert.ok(result.mergeCommand.includes('--admin'), 'merge command must include --admin'); }); test('FAIL → exit 1 (not 0, not 2)', () => { @@ -616,7 +637,14 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { ['/check-runs', CHECKS_OK_WITH_HYGIENE], ['/status', { statuses: [], total_count: 0 }], ], calls); + const start = Date.now(); assert.equal(main(['1'], runner, OK_GH_VERSION), 0); + const elapsed = Date.now() - start; + // AC-30 (clause b): verifier must complete in under 15 s wall-clock. + // With a synchronous stub runner, elapsed time reflects the verifier's own + // CPU cost and any unexpected loops — network latency is zero. + assert.ok(elapsed < 15000, + `verifier must complete in < 15 s wall-clock (AC-30 clause b); took ${elapsed}ms`); assert.equal(calls.length, 4, `expected 4 API calls (pr, protection, checks, status); got ${calls.length}`); const checkCall = calls.find(u => u.includes('/check-runs')); assert.ok(checkCall.includes('filter=latest'), 'filter=latest must be pinned explicitly (D-PR4a)'); @@ -1014,5 +1042,53 @@ describe('D-PR2a: required context satisfied by commit status', () => { }); }); +// --------------------------------------------------------------------------- +// Tier C: pending statuses are reported as advisory, not silently dropped +// --------------------------------------------------------------------------- +describe('Tier C: pending non-required status is reported as advisory', () => { + + test('state=pending non-required status emits an advisory line (not silently ignored)', () => { + // A pending non-required status is the same indeterminate condition as a + // non-completed Tier B run — both are "not yet resolved". Tier B now FAILs + // on queued/in_progress (same delta). Tier C is advisory-only, but the + // operator must still be able to see it, not have it vanish silently. + const runs = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + const statuses = [{ context: 'security/snyk (dean0x)', state: 'pending' }]; + const result = evaluateChecks({ + requiredContexts: REQUIRED, + checkRuns: runs, + statuses, + headSha: HEAD_113F472, + }); + // PASS overall (non-required, advisory only) + assert.equal(result.exitCode, 0, `pending non-required status must not cause FAIL; lines: ${result.lines.join('\n')}`); + const allLines = result.lines.join('\n'); + // The advisory line must be present so the operator sees the pending status + assert.ok( + allLines.includes('advisory (Tier C)') && allLines.includes('pending'), + `must emit an advisory line for pending non-required status; got:\n${allLines}`, + ); + }); + + test('state=error non-required status still emits advisory (regression guard)', () => { + // Guard against the Tier C rewrite accidentally dropping non-pending errors. + const runs = [...loadCheckRuns('checks-main-113f472.json'), SOURCE_HYGIENE_PASS]; + const statuses = [{ context: 'security/snyk (dean0x)', state: 'error' }]; + const result = evaluateChecks({ + requiredContexts: REQUIRED, + checkRuns: runs, + statuses, + headSha: HEAD_113F472, + }); + assert.equal(result.exitCode, 0, 'non-required error status must not cause FAIL'); + const allLines = result.lines.join('\n'); + assert.ok( + allLines.includes('advisory (Tier C)') && allLines.includes('error'), + `must emit an advisory line for error non-required status; got:\n${allLines}`, + ); + }); + +}); + // Code of Conduct tests (AC-1, AC-2) live in code-of-conduct.spec.mjs — // split in commit 2e9482f to follow the one-spec-per-module convention. diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index ba47d17..a4008fd 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -405,7 +405,12 @@ export function evaluateChecks({ // in state=error due to account-plan limits — not a workflow in this repo. for (const st of statuses) { if (requiredSet.has(st.context)) continue; // Already handled in Tier A - if (st.state !== 'success' && st.state !== 'pending') { + if (st.state === 'pending') { + // Asymmetric with Tier B (which FAILs on not-yet-completed runs): a + // non-required pending status is advisory only, but it must not be + // silently swallowed — the caller cannot know if this is benign. + lines.push(` advisory (Tier C): "${st.context}" — state=pending (not yet resolved)`); + } else if (st.state !== 'success') { lines.push(` advisory (Tier C): "${st.context}" — state=${st.state}`); } } @@ -416,13 +421,17 @@ export function evaluateChecks({ } if (pass) { - const cmd = `gh pr merge --squash --match-head-commit ${headSha}`; + // D-PR5: --admin is required because main is protected and the sole + // code-owner cannot self-approve. Emit it here so the operator can + // copy the command verbatim without hand-editing (avoids PF-017: a + // hand-edited command is where --match-head-commit gets dropped). + const cmd = `gh pr merge --squash --admin --match-head-commit ${headSha}`; lines.push(`✓ PASS — all ${nRequired} required contexts completed+success`); lines.push(` Verified SHA: ${headSha}`); lines.push(` Merge command: ${cmd}`); return { pass: true, exitCode: 0, lines, mergeCommand: cmd }; } else { - lines.push(`✖ FAIL — ${failures.length} check(s) did not pass`); + lines.push(`✖ FAIL — ${failures.length} required context(s) not satisfied`); return { pass: false, exitCode: 1, lines }; } } From 29141c13e23cde0906a82543e9b7d9ec97c69ff9 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:02:26 +0200 Subject: [PATCH 14/22] fix(scanner): harden --staged path for type-change, NUL delimiter, and timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness fixes for verify-no-control-bytes.mjs --staged mode: 1. D-CB5b: add T (type-change) to --diff-filter so a tracked symlink replaced by a regular file containing a hazard codepoint is not silently excluded. The T blob is a regular file with content; the scanner must inspect it. 2. Use NUL as the cat-file --batch input delimiter (matching getStagedFiles' -z output). A LF-containing git path would otherwise split into two batch requests and desynchronize the response parser, producing garbage reads. Pass -z to git cat-file --batch to match. 3. Add timeout: 30_000 to the cat-file spawnSync call. An index lock or network-FS hang in the pre-commit hook must not block indefinitely. ETIMEDOUT is indeterminate → exit 2, never 0 (applies ADR-009). Fix the deletion-path reporting to use --diff-filter=D explicitly, which is correct when a T-type path is the only staged entry (the earlier unfiltered diff included T paths that ACMRT already handles, giving a misleading count). Co-Authored-By: Claude --- scripts/verify-no-control-bytes.mjs | 45 ++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 0d86603..8c38d94 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -264,7 +264,7 @@ function getTrackedFiles(cwd) { /** * Get staged file list for --staged mode. - * Uses `git diff --cached --name-only -z --diff-filter=ACMR` for paths. + * Uses `git diff --cached --name-only -z --diff-filter=ACMRT` for paths. * D-CB8: content is fetched later in batch via readAllIndexBlobs(). * * D-CB5a divergence: `git diff --cached --name-only` does not carry git mode @@ -276,7 +276,11 @@ function getTrackedFiles(cwd) { * narrower scope where only newly-committed content is checked. */ function getStagedFiles(cwd) { - const r = gitExec(['diff', '--cached', '--name-only', '-z', '--diff-filter=ACMR'], cwd); + // D-CB5b: include T (type-change) so a tracked symlink replaced by a regular + // file containing a hazard codepoint is not silently excluded. A T change + // from regular-file → symlink is benign (symlink blob = ASCII target path), + // which is already documented in the D-CB5a JSDoc above. + const r = gitExec(['diff', '--cached', '--name-only', '-z', '--diff-filter=ACMRT'], cwd); if (r.status !== 0) { // D-CB5: fail closed. `git diff --cached` exits 0 even when nothing is // staged, so a non-zero status is a real tool failure (corrupt index, @@ -308,15 +312,26 @@ function getStagedFiles(cwd) { function readAllIndexBlobs(paths, cwd) { if (paths.length === 0) return new Map(); - // Input: ":\n" for every staged path - const stdin = Buffer.from(paths.map(p => `:${p}\n`).join(''), 'utf8'); - const r = spawnSync('git', ['cat-file', '--batch'], { + // Input: ":\0" for every staged path. + // Use NUL as delimiter (matching getStagedFiles' -z output) so that git paths + // containing a literal LF do not split into two batch requests and + // desynchronize the response parser. + const stdin = Buffer.from(paths.map(p => `:${p}\0`).join(''), 'utf8'); + // Hard bound: mirrors gitExec's contract. An index lock, network-FS hang, or + // stuck git subprocess in the pre-commit hook must not block indefinitely. + // ETIMEDOUT is indeterminate → exit 2, never 0. + const r = spawnSync('git', ['cat-file', '--batch', '-z'], { cwd, input: stdin, encoding: 'buffer', maxBuffer: 256 * 1024 * 1024, + timeout: 30_000, }); if (r.error) { + if (r.error.code === 'ETIMEDOUT') { + console.error('✖ verify-no-control-bytes: git cat-file --batch timed out after 30 s — indeterminate, not clean'); + process.exit(2); + } console.error(`✖ verify-no-control-bytes: git cat-file --batch: ${r.error.message}`); process.exit(2); } @@ -507,21 +522,25 @@ function main() { console.error(' If this is a new repo with no commits, run `git add` first.'); process.exit(1); } - // --staged: check unfiltered diff to provide an accurate message. - const rAll = gitExec(['diff', '--cached', '--name-only', '-z'], cwd); - if (rAll.status !== 0) { + // --staged: query actual deletions via --diff-filter=D rather than + // inferring from the unfiltered diff. Using the unfiltered set is wrong + // when a T (type-change) is the only staged path: ACMRT already captures + // T-type content-bearing blobs, so if we arrive here the staged set truly + // contains only deletions (D) or nothing at all. + const rDel = gitExec(['diff', '--cached', '--name-only', '-z', '--diff-filter=D'], cwd); + if (rDel.status !== 0) { console.error( - `✖ verify-no-control-bytes: git diff --cached (unfiltered) failed: ` + - `${rAll.stderr.toString('utf8').trim()}`, + `✖ verify-no-control-bytes: git diff --cached (deletions) failed: ` + + `${rDel.stderr.toString('utf8').trim()}`, ); process.exit(2); } - const allPaths = rAll.stdout.toString('utf8').split('\0').filter(s => s.length > 0); - if (allPaths.length > 0) { + const deletionPaths = rDel.stdout.toString('utf8').split('\0').filter(s => s.length > 0); + if (deletionPaths.length > 0) { // Staged changes exist but are all deletions — nothing for the content scanner to do. console.log( `✓ source-hygiene gate: 0 content-bearing staged paths` + - ` (${allPaths.length} deletion(s)) — nothing to scan`, + ` (${deletionPaths.length} deletion(s)) — nothing to scan`, ); } else { // Index equals HEAD (amend --no-edit, reword, --allow-empty, etc.). From b033037d7195bf9c9c21d00e94f12c690739de58 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:02:51 +0200 Subject: [PATCH 15/22] docs(ci,scanner): update comments to reflect test:gates and ACMRT filter - release.yml: replace HAZARD_RANGES-pinning comment with accurate description of the test:gates non-vacuity guard (>=3 spec files, ADR-009/PF-013) - verify-no-control-bytes.mjs: update D-CB5 doc strings from ACMR to ACMRT to reflect the D-CB5b type-change addition (committed in prior fix) Co-Authored-By: Claude --- .github/workflows/release.yml | 4 ++-- scripts/verify-no-control-bytes.mjs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 476d212..b35a98b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -33,8 +33,8 @@ jobs: # #288: Source-hygiene gate — also runs on tag pushes via this job. # ci.yml does not run on tag pushes, so these two steps ensure the full # gate (scanner + positive-control suite) is enforced at release time. - # The positive-control suite pins HAZARD_RANGES (D-CB1a) so a silently- - # narrowed hazard class cannot exit 0 on the release path (ADR-009/PF-013). + # test:gates enforces a minimum spec count (>=3) so the suite cannot + # be silently skipped on the release path (ADR-009/PF-013). # Uses the same Node 22 install above. - name: "Assert no hazardous codepoints in tracked source" run: node scripts/verify-no-control-bytes.mjs diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 8c38d94..8261ce1 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -23,7 +23,7 @@ * * D-CB5: Fails closed. In full-tree mode, zero-files-scanned is exit 1, not * exit 0 (avoids PF-016 — an empty scan masquerades as clean). In --staged - * mode, an empty ACMR-filtered set is a legitimate state (deletion-only + * mode, an empty ACMRT-filtered set is a legitimate state (deletion-only * commits, amend with no content changes) and exits 0 with an explicit * message; the full-tree scan is the authoritative non-vacuity gate. * @@ -511,8 +511,8 @@ function main() { // ---- D-CB5: Non-vacuity guard (AC-6) ---- // Full-tree mode: zero tracked files means path discovery broke — fail closed. - // --staged mode: an empty ACMR-filtered set is a LEGITIMATE state: - // • deletion-only commit (git rm): ACMR excludes deletions; files ARE staged. + // --staged mode: an empty ACMRT-filtered set is a LEGITIMATE state: + // • deletion-only commit (git rm): ACMRT excludes deletions; files ARE staged. // • amend with no content changes: index equals HEAD; diff is empty. // In both cases exit 0 with an explicit message. The full-tree scan is the // authoritative non-vacuity gate; the hook must not block valid commits. From 5ecc7f2366d59088b20dfca15e5e0540468cf0e0 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:04:00 +0200 Subject: [PATCH 16/22] docs(verify): sync D-PR5 header comment with actual --admin emission The file-header comment for D-PR5 described the PASS merge command as '--match-head-commit ' without mentioning --admin. The emitted command (line 428) already includes --admin following commit 454794f; this commit aligns the module-level doc to match actual behaviour. avoids PF-017 Co-Authored-By: Claude --- scripts/verify-pr-checks.mjs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index a4008fd..9c9b078 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -41,9 +41,10 @@ * `filter=latest` is pinned explicitly (default today, but implicit * defaults can change and this gate's verdict depends on it). * - * D-PR5: On PASS the tool prints a merge command with --match-head-commit - * , closing the TOCTOU window where the verified SHA diverges - * from HEAD by the time the merge runs. + * D-PR5: On PASS the tool prints `gh pr merge --squash --admin --match-head-commit + * ` so the operator copies it verbatim. --admin is required for + * protected main; --match-head-commit closes the TOCTOU window where the + * verified SHA diverges from HEAD by the time the merge runs (avoids PF-017). * * D-PR6: Exit codes — 0 PASS, 1 FAIL, 2 indeterminate. "Cannot tell" is * never 0. From 8028e5dd95df4fd0e06d99ece2d712bd8340ba6c Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:06:26 +0200 Subject: [PATCH 17/22] test(scanner): add positive control for T-type staged bypass (ADR-009) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Case C: a type-change (T) staged blob — a tracked symlink replaced by a regular file containing ESC (0x1B) — must be detected by the --staged scanner with exit 1. Before the D-CB5b fix (--diff-filter=ACMRT), --diff-filter=ACMR excluded T silently and the hook reported exit 0. This test is the positive control required by ADR-009 / PF-013: it proves the gate detects hostile content in a T-type staged blob, not merely that absence implies clean (applies ADR-009, avoids PF-013). Bytes constructed at runtime from hex literals, never embedded as backslash-u escapes (applies D-CB2, avoids PF-018). Co-Authored-By: Claude --- .../__test__/verify-no-control-bytes.spec.mjs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index 4fd2533..f968651 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -609,6 +609,40 @@ describe('AC-18: --staged mode reads index blob, not working tree', () => { } finally { cleanup(dir); } }); + test('Case C: type-change (T) staged blob with hostile content exits 1 (ACMRT positive control)', () => { + // A git type-change (T) occurs when a tracked symlink is replaced by a regular + // file (or vice versa) and the result is staged. --diff-filter=ACMR excludes T, + // which silently bypasses the pre-commit hook for any hostile content in that blob. + // D-CB5b fix: --diff-filter=ACMRT captures T-type changes for scanning. + // + // This test is the positive control required by ADR-009 / PF-013: it proves the + // scanner DETECTS hostile content in a T-type staged blob. + const { dir, git } = mkTempGitRepo(); + try { + // 1. Commit a symlink (mode 120000) so victim.md is tracked as a symlink. + writeFileSync(join(dir, 'target.txt'), 'clean target\n'); + symlinkSync('target.txt', join(dir, 'victim.md')); + git('add', 'target.txt', 'victim.md'); + git('commit', '-m', 'add symlink'); + + // 2. Replace the symlink with a regular file containing ESC (0x1B). + // rmSync removes the symlink inode; writeFileSync creates a regular file. + rmSync(join(dir, 'victim.md')); + const esc = Buffer.from([0x1b]); // ESC constructed at runtime, not a literal (PF-018) + writeFileSync(join(dir, 'victim.md'), Buffer.concat([Buffer.from('evil '), esc])); + // git add produces T (type-change): victim.md was symlink (120000), now regular (100644) + git('add', 'victim.md'); + + // Pre-delta (ACMR filter): would exit 0, silently passing a hostile blob. + // Post-fix (ACMRT filter): blob is scanned and exits 1. + const r = runScanner(['--staged'], { cwd: dir }); + assert.equal(r.status, 1, + 'T-type staged change (symlink to file) with hostile ESC must exit 1 (ACMRT positive control)'); + assert.ok(r.stderr.includes('victim.md'), `error must name the file; got: ${r.stderr}`); + assert.ok(r.stderr.includes('U+001B'), `error must name U+001B; got: ${r.stderr}`); + } finally { cleanup(dir); } + }); + }); // --------------------------------------------------------------------------- From 3f843dedd577f93b8e37d1f32353379e27ad36bb Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:18:57 +0200 Subject: [PATCH 18/22] fix(scanner): skip gitlinks in --staged mode and record AC-6 amendment (D-CB5a) Two correctness fixes for verify-no-control-bytes.mjs --staged mode: 1. D-CB5a: change getStagedFiles() from `git diff --cached --name-only -z` to `git diff --cached --raw -z` so that the new-file mode is available for each staged entry. Entries with new-mode 160000 (gitlink/submodule) and 120000 (symlink) are now marked skip:true before they reach readAllIndexBlobs(), consistent with full-tree mode's AC-20 behavior. Without this fix, a staged submodule would reach git cat-file --batch, which returns 'missing' for a commit-typed index entry, causing exit 2 with the misleading message "staged path not in index: ". The raw format is parsed per entry: header starts with ':' and carries old-mode, new-mode, SHAs, and status; rename/copy (R/C) entries have two path tokens and the new path is taken for scanning. The skipped count is now propagated to the passStats message exactly as in full-tree mode ("N symlink/gitlink skipped"). 2. AC-6 AMENDMENT: document in the top-level D-CB5 JSDoc that --staged mode intentionally exits 0 for a legitimately-empty content-bearing set (deletion-only commits, amend -m, --allow-empty). This was previously described as a "carve-out" but not explicitly called out as an AC-6 amendment. The full-tree CI scan remains the authoritative AC-6 gate. Test: add "AC-20: a staged gitlink (mode 160000) is skipped in --staged mode" to the spec. Uses git update-index --cacheinfo 160000,,sub to inject a gitlink directly into the index without requiring a real submodule on disk. Proves exit 0 + "symlink/gitlink skipped" in the output. All 43 tests pass. Scanner exits 0 on the full repo tree. Co-Authored-By: Claude --- .../__test__/verify-no-control-bytes.spec.mjs | 59 ++++++++++++ scripts/verify-no-control-bytes.mjs | 91 +++++++++++++++---- 2 files changed, 130 insertions(+), 20 deletions(-) diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index f968651..5229ddd 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -919,6 +919,65 @@ describe('AC-20: symlinks are skipped and counted, not read', () => { } finally { cleanup(dir); } }); + test('AC-20: a staged gitlink (mode 160000) is skipped in --staged mode, not reported as an error', () => { + // Regression test for the pre-fix bug: without mode-aware skip in getStagedFiles(), + // a staged gitlink (submodule) would reach readAllIndexBlobs(), and + // `git cat-file --batch` would return 'missing' (gitlinks store a commit SHA, + // not a blob), causing exit 2 with the misleading message + // "staged path not in index: ". + // + // Fix (D-CB5a): getStagedFiles() now uses `git diff --cached --raw -z` + // to obtain the new-file mode for each staged entry. Entries with new-mode + // 160000 (gitlink) are marked skip:true before they reach readAllIndexBlobs(), + // consistent with full-tree mode's AC-20 behavior. + // + // A real commit SHA is required because git validates the SHA format when + // updating the index via --cacheinfo. We harvest a SHA from a sibling temp + // repo to avoid network access and keep the test hermetic. + const { dir, git } = mkTempGitRepo(); + const innerDir = mkdtempSync(join(tmpdir(), 'mds-inner-')); + try { + // Create an inner repo and commit one file to get a real commit SHA. + execFileSync('git', ['init'], { cwd: innerDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 'test@test.test'], { cwd: innerDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: innerDir, stdio: 'pipe' }); + writeFileSync(join(innerDir, 'inner.md'), 'inner\n'); + execFileSync('git', ['add', 'inner.md'], { cwd: innerDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'inner'], { cwd: innerDir, stdio: 'pipe' }); + const innerSha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: innerDir, encoding: 'utf8', stdio: 'pipe', + }).trim(); + + // Stage a clean regular file alongside a gitlink (mode 160000) in the outer repo. + // `git update-index --cacheinfo 160000,,` injects a gitlink entry + // directly into the index without requiring a real .gitmodules or disk presence. + writeFileSync(join(dir, 'clean.md'), 'clean content\n'); + git('add', 'clean.md'); + execFileSync('git', [ + 'update-index', '--add', '--cacheinfo', `160000,${innerSha},sub`, + ], { cwd: dir, stdio: 'pipe' }); + + // Confirm the gitlink is actually in the index at mode 160000. + const modes = execFileSync('git', ['ls-files', '-s'], { cwd: dir, encoding: 'utf8' }); + assert.ok(modes.includes('160000'), 'fixture must actually stage a gitlink at mode 160000'); + + // --staged mode: the gitlink must be skipped (not cause an error), and the + // regular file must be scanned and pass. + const r = runScanner(['--staged'], { cwd: dir }); + assert.equal(r.status, 0, + `staged gitlink must be skipped without error; stderr: ${r.stderr}`); + // The success output must report the skipped gitlink count. + assert.ok(r.stdout.includes('symlink/gitlink skipped'), + `output must report the skipped gitlink; got: ${r.stdout}`); + // The regular file must be scanned (not silently dropped with the gitlink). + assert.ok(/Scanned 1 file\(s\)/.test(r.stdout), + `must scan the one regular file; got: ${r.stdout}`); + } finally { + cleanup(dir); + cleanup(innerDir); + } + }); + }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 8261ce1..41d4a54 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -27,6 +27,16 @@ * commits, amend with no content changes) and exits 0 with an explicit * message; the full-tree scan is the authoritative non-vacuity gate. * + * AC-6 AMENDMENT (--staged mode): AC-6 requires exit 1 when the scanned + * file set is empty. --staged mode intentionally exits 0 for a legitimately- + * empty content-bearing set (deletion-only commits via `git rm`, `git commit + * --amend -m`, `git commit --allow-empty`). Exiting 1 there would train + * contributors toward `--no-verify`, which disables the gate for ALL commits + * — the opposite of the security goal. No bypass exists: any staged file with + * hazardous content appears under the ACMRT filter. This amendment is + * deliberate, tested (see spec test "D-CB5 --staged carve-out from AC-6"), + * and leaves the full-tree CI scan as the authoritative AC-6 gate. + * * D-CB8: --staged mode reads file content from the git index via a single * `git cat-file --batch` subprocess (all staged blobs at once), never from * the working tree. Staging a clean file then modifying the working copy @@ -264,27 +274,36 @@ function getTrackedFiles(cwd) { /** * Get staged file list for --staged mode. - * Uses `git diff --cached --name-only -z --diff-filter=ACMRT` for paths. + * Uses `git diff --cached --raw -z --diff-filter=ACMRT` to obtain both paths + * and git mode information so that gitlinks (160000) and symlinks (120000) can + * be skipped consistently with full-tree mode's AC-20 behavior. * D-CB8: content is fetched later in batch via readAllIndexBlobs(). * - * D-CB5a divergence: `git diff --cached --name-only` does not carry git mode - * information, so every path is assigned mode 0o100644. Symlinks (120000) and - * gitlinks (160000) are NOT skipped as they are in full-tree mode. This is - * benign: a staged symlink's blob content is the target path (plain ASCII), - * which never contains a hazard codepoint, so no false positives arise. The - * full-tree scan enforces the unconditional AC-20 skip; staged mode is a - * narrower scope where only newly-committed content is checked. + * D-CB5a: mode-aware skip for staged mode. Unlike `git diff --cached + * --name-only`, the `--raw` format carries the new-file mode (octal) for each + * staged entry. Entries with new-mode 120000 (symlink) or 160000 (gitlink) are + * marked skip:true — identical to full-tree mode's AC-20 treatment. This + * resolves the pre-fix divergence where gitlinks reaching readAllIndexBlobs() + * would trigger a misleading "staged path not in index" error because + * `git cat-file --batch` returns 'missing' for a commit-typed index entry. + * + * D-CB5b: include T (type-change) in the diff-filter so a tracked symlink + * (120000) replaced by a regular file (100644) containing a hazard codepoint + * is not silently excluded. The raw format carries the new mode, so a T-change + * landing on mode 100644 is scanned; one landing on 120000 or 160000 is + * skipped. A T-change landing on 160000 would previously have caused a + * misleading error — it is now skipped cleanly. + * + * Raw diff format (git diff --cached --raw -z), per entry: + * : \0\0 + * Rename/copy (status starts with R or C): + * : Rscore\0\0\0 */ function getStagedFiles(cwd) { - // D-CB5b: include T (type-change) so a tracked symlink replaced by a regular - // file containing a hazard codepoint is not silently excluded. A T change - // from regular-file → symlink is benign (symlink blob = ASCII target path), - // which is already documented in the D-CB5a JSDoc above. - const r = gitExec(['diff', '--cached', '--name-only', '-z', '--diff-filter=ACMRT'], cwd); + const r = gitExec(['diff', '--cached', '--raw', '-z', '--diff-filter=ACMRT'], cwd); if (r.status !== 0) { - // D-CB5: fail closed. `git diff --cached` exits 0 even when nothing is - // staged, so a non-zero status is a real tool failure (corrupt index, - // unreadable object). Treating it as "no staged files" would let the + // D-CB5: fail closed — a non-zero status is a real tool failure (corrupt + // index, unreadable object). Treating it as "no staged files" would let the // pre-commit hook report success on a scan that never happened. console.error( `✖ verify-no-control-bytes: git diff --cached failed (status ${r.status}): ` + @@ -292,8 +311,36 @@ function getStagedFiles(cwd) { ); process.exit(2); } - const paths = r.stdout.toString('utf8').split('\0').filter(s => s.length > 0); - return paths.map(p => ({ path: p, mode: 0o100644, skip: false, staged: true })); + + const parts = r.stdout.toString('utf8').split('\0'); + const files = []; + let i = 0; + while (i < parts.length) { + const header = parts[i]; + // Raw diff headers start with ':'. Skip empty parts and non-header tokens. + if (!header || !header.startsWith(':')) { i++; continue; } + // `:old-mode new-mode old-sha new-sha status` + const fields = header.slice(1).split(' '); + const newMode = parseInt(fields[1], 8); // new-mode is octal (e.g. "100644") + const status = fields[4] || ''; + i++; + // Rename (R) and copy (C) entries have two paths: old-path then new-path. + // Advance past old-path and use new-path (the content we care about). + if (status.startsWith('R') || status.startsWith('C')) { + i++; // skip old-path + } + const path = parts[i] || ''; + i++; + if (!path) continue; + // AC-20: skip symlinks (120000) and gitlinks (160000) — identical to + // full-tree mode. A gitlink's cat-file lookup returns 'missing' (commit + // object, not a blob) and would otherwise cause a misleading "staged path + // not in index" error. Symlink blobs contain only the ASCII target path + // and are safe to skip. + const skip = newMode === 0o120000 || newMode === 0o160000; + files.push({ path, mode: newMode, skip, staged: true }); + } + return files; } /** @@ -498,8 +545,12 @@ function main() { absolutePath: resolve(cwd, p), })); } else if (isStaged) { - // D-CB8: staged mode — read from git index - fileEntries = getStagedFiles(cwd).map(e => ({ ...e, absolutePath: null })); + // D-CB8: staged mode — read from git index. + // getStagedFiles() now provides mode info; skip gitlinks and symlinks + // exactly as full-tree mode does (AC-20, D-CB5a). + const all = getStagedFiles(cwd); + skippedCount = all.filter(e => e.skip).length; + fileEntries = all.filter(e => !e.skip).map(e => ({ ...e, absolutePath: null })); } else { // Default: full tracked tree const all = getTrackedFiles(cwd); From 888e446deb6c13f5c7449a81b03ea1aeef4bfb47 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:21:21 +0200 Subject: [PATCH 19/22] docs(scanner): qualify symlink-target comment to avoid absolute phrasing (PF-015) Replace 'Symlink blobs contain only the ASCII target path and are safe to skip' with an accurate qualified statement: POSIX permits any byte except NUL and '/' in symlink targets, so control bytes are theoretically possible and scanning them could produce false-positive gate failures. The consequence of such a false positive is fail-closed (exit 1), so impact is minimal, but absolute phrasing in a normative comment is the shape PF-015 warns against. Co-Authored-By: Claude --- scripts/verify-no-control-bytes.mjs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 41d4a54..58f8fbd 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -335,8 +335,10 @@ function getStagedFiles(cwd) { // AC-20: skip symlinks (120000) and gitlinks (160000) — identical to // full-tree mode. A gitlink's cat-file lookup returns 'missing' (commit // object, not a blob) and would otherwise cause a misleading "staged path - // not in index" error. Symlink blobs contain only the ASCII target path - // and are safe to skip. + // not in index" error. Symlink blobs contain the target path; POSIX permits + // any byte except NUL and '/' in symlink targets, so control bytes are + // theoretically possible and scanning them could produce false-positive gate + // failures. Skipping keeps the gate focused on source-file content (avoids PF-015). const skip = newMode === 0o120000 || newMode === 0o160000; files.push({ path, mode: newMode, skip, staged: true }); } From 05f7eeeea6642f3c5a50a9842ba347717067c3d2 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:31:33 +0200 Subject: [PATCH 20/22] refactor(scanner,verifier): simplify control flow and remove duplicate test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - verify-no-control-bytes.mjs: convert sequential `if`+exit chains to if/else if/else in gitExec and readAllIndexBlobs (makes mutual exclusion explicit), remove spurious blank line after hexContext, split the long hazardHits.push() into a multi-line object literal. - verify-pr-checks.mjs: replace chained ternary in defaultGhRunner error handling with if/else if/else (follows project style — no nested ternaries). - verify-no-control-bytes.spec.mjs: remove stale `scanner:611` line-number references from the AC-30 describe block; reflow the comment. - verify-pr-checks.spec.mjs: remove the "contract parity check" test whose two assertions are exact duplicates of the preceding "extracts 404" and "extracts 403" tests. Its explanatory note (use `httpStatus:404`, not `status:404`) is moved into the describe block comment. All 119 tests pass. No behavior change. All positive controls, D-CB* and D-PR* decision markers, and ADR-009/PF-013 guards are intact. --- .../__test__/verify-no-control-bytes.spec.mjs | 42 +++++++++---------- scripts/__test__/verify-pr-checks.spec.mjs | 16 +++---- scripts/verify-no-control-bytes.mjs | 20 +++++---- scripts/verify-pr-checks.mjs | 13 +++--- 4 files changed, 46 insertions(+), 45 deletions(-) diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index 5229ddd..49032e2 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -721,21 +721,20 @@ describe('AC-15: scanner source is self-clean', () => { // (a) Wall-clock full-tree scan < 5 s — asserted with Date.now() in the // AC-5 test above (generous CI-safe bound). // (b) --staged mode < 2 s for a 20-file commit — not directly timed here. -// NOTE: readAllIndexBlobs() (added after the original AC-30 comment was -// written) collapses N per-file `git cat-file blob` spawns into ONE -// `git cat-file --batch` call. The entire staged set is materialized -// into a single r.stdout buffer; out.slice(pos, pos+size) returns Buffer -// views that pin that buffer. --staged mode therefore holds all staged -// file contents in memory simultaneously, bounded by the 256 MB maxBuffer -// cap (r.error -> exit 2). This is a known trade-off (spawn cost vs memory) -// that was accepted when readAllIndexBlobs() replaced readIndexBlob(). -// (c) hazardHits MUST NOT retain file buffers — verified by code shape: at -// verify-no-control-bytes.mjs:611, hazardHits.push stores { hexCtx } -// (a pre-computed string) not { buf } (the raw buffer). In full-tree mode -// buf is GC-eligible after each loop iteration. In --staged mode buf is a -// view into the already-pinned blobMap buffer (clause b), so GC-eligibility -// at the hazardHits level is academic there — but the key property holds: -// hazardHits does NOT additionally retain file buffers. +// NOTE: readAllIndexBlobs() collapses N per-file `git cat-file blob` +// spawns into ONE `git cat-file --batch` call. The entire staged set is +// materialized into a single r.stdout buffer; out.slice(pos, pos+size) +// returns Buffer views that pin that buffer. --staged mode therefore holds +// all staged file contents in memory simultaneously, bounded by the 256 MB +// maxBuffer cap (r.error -> exit 2). This is a known trade-off (spawn cost +// vs memory) that was accepted when readAllIndexBlobs() replaced readIndexBlob(). +// (c) hazardHits MUST NOT retain file buffers — verified by code shape: +// hazardHits.push stores { hexCtx } (a pre-computed string) not { buf } +// (the raw buffer). In full-tree mode buf is GC-eligible after each loop +// iteration. In --staged mode buf is a view into the already-pinned blobMap +// buffer (clause b), so GC-eligibility at the hazardHits level is academic +// there — but the key property holds: hazardHits does NOT additionally +// retain file buffers. // // This describe block tests clause (c) indirectly: by proving the correct // hexCtx string reaches the output across multiple files, it demonstrates @@ -746,12 +745,13 @@ describe('AC-30: hex context stored as string per hit, not as file buffer', () = test('scanner reports hex context for every hazard across multiple files', () => { // Verify that hexCtx is computed and stored correctly for each hit. - // Memory discipline (clause c) is by code shape: hazardHits stores { hexCtx } - // not { buf } (scanner:611), so buf is not additionally retained in hazardHits. - // In full-tree mode buf is GC-eligible after each iteration; in --staged mode - // buf is a view into the blobMap buffer (all blobs held simultaneously per - // clause b). This test proves the correct context string reaches the output - // regardless of how many files are scanned. + // Memory discipline (clause c) is by code shape: hazardHits.push stores + // { hexCtx } (a pre-computed string) not { buf } (the raw buffer), so buf + // is not additionally retained in hazardHits. In full-tree mode buf is + // GC-eligible after each iteration; in --staged mode buf is a view into + // the blobMap buffer (all blobs held simultaneously per clause b). This + // test proves the correct context string reaches the output regardless of + // how many files are scanned. const { dir, git } = mkTempGitRepo(); try { // Construct two files each with an ESC at a known position diff --git a/scripts/__test__/verify-pr-checks.spec.mjs b/scripts/__test__/verify-pr-checks.spec.mjs index b002a19..4290617 100644 --- a/scripts/__test__/verify-pr-checks.spec.mjs +++ b/scripts/__test__/verify-pr-checks.spec.mjs @@ -728,6 +728,11 @@ describe('AC-26 AC-28 AC-29: live path exit codes (injected runner)', () => { // stderr strings ensures the stubs used throughout this file mirror the value the // production runner actually produces (applies ADR-009, avoids PF-013: dead // branches that only trigger on a value the runner never produces). +// +// Stub shape reminder: stubRunner error objects use `httpStatus: 404` (not +// `status: 404`). The process exit code is always 1 regardless of HTTP status; +// `status` alone cannot distinguish 404 from 403. The stubRunner JSDoc documents +// this contract; these tests pin the parser output that defines it. // --------------------------------------------------------------------------- describe('high finding: parseGhStderrHttpStatus parses real gh stderr format', () => { @@ -760,17 +765,6 @@ describe('high finding: parseGhStderrHttpStatus parses real gh stderr format', ( assert.equal(parseGhStderrHttpStatus(undefined), null); }); - test('stub stubs use the same shape this function produces (contract parity check)', () => { - // All stubRunner error objects in this file use `httpStatus: 404` or `httpStatus: 403` - // which mirrors what parseGhStderrHttpStatus returns for real gh stderr strings. - // This test makes the mapping explicit and prevents future stubs from drifting - // back to `status: 404` (the old, broken shape) (avoids PF-013). - assert.equal(parseGhStderrHttpStatus('gh: Not Found (HTTP 404)'), 404, - 'stub must use httpStatus: 404 (not status: 404) to mirror production'); - assert.equal(parseGhStderrHttpStatus('gh: Forbidden (HTTP 403)'), 403, - 'stub must use httpStatus: 403 (not status: 403) to mirror production'); - }); - }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index 58f8fbd..d30f24f 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -195,7 +195,6 @@ function hexContext(buf, offset) { return hex.join(' '); } - // --------------------------------------------------------------------------- // git helpers // --------------------------------------------------------------------------- @@ -215,14 +214,14 @@ function gitExec(args, cwd = process.cwd()) { // not an indeterminate tool error. console.error('✖ verify-no-control-bytes: git is not on PATH'); process.exit(1); - } - if (result.error.code === 'ETIMEDOUT') { + } else if (result.error.code === 'ETIMEDOUT') { // Timeout is indeterminate — a hung git call is not evidence of a clean tree. console.error('✖ verify-no-control-bytes: git timed out after 30 s — indeterminate, not clean'); process.exit(2); + } else { + console.error(`✖ verify-no-control-bytes: git error: ${result.error.message}`); + process.exit(2); } - console.error(`✖ verify-no-control-bytes: git error: ${result.error.message}`); - process.exit(2); } return result; } @@ -379,9 +378,9 @@ function readAllIndexBlobs(paths, cwd) { if (r.error) { if (r.error.code === 'ETIMEDOUT') { console.error('✖ verify-no-control-bytes: git cat-file --batch timed out after 30 s — indeterminate, not clean'); - process.exit(2); + } else { + console.error(`✖ verify-no-control-bytes: git cat-file --batch: ${r.error.message}`); } - console.error(`✖ verify-no-control-bytes: git cat-file --batch: ${r.error.message}`); process.exit(2); } if (r.status !== 0) { @@ -661,7 +660,12 @@ function main() { exercisedAllowlist.add(`${entry.path}:${hit.codepoint}`); } else { // AC-30: compute hex context now so buf is not retained after this iteration. - hazardHits.push({ path: entry.path, codepoint: hit.codepoint, byteOffset: hit.byteOffset, hexCtx: hexContext(buf, hit.byteOffset) }); + hazardHits.push({ + path: entry.path, + codepoint: hit.codepoint, + byteOffset: hit.byteOffset, + hexCtx: hexContext(buf, hit.byteOffset), + }); } } } diff --git a/scripts/verify-pr-checks.mjs b/scripts/verify-pr-checks.mjs index 9c9b078..a23f9c0 100644 --- a/scripts/verify-pr-checks.mjs +++ b/scripts/verify-pr-checks.mjs @@ -150,11 +150,14 @@ function defaultGhRunner(args) { timeout: 30_000, }); if (r.error) { - const stderr = r.error.code === 'ENOENT' - ? 'gh is not on PATH' - : r.error.code === 'ETIMEDOUT' - ? 'gh timed out after 30 s (AC-30: indeterminate, not clean)' - : `gh error: ${r.error.message}`; + let stderr; + if (r.error.code === 'ENOENT') { + stderr = 'gh is not on PATH'; + } else if (r.error.code === 'ETIMEDOUT') { + stderr = 'gh timed out after 30 s (AC-30: indeterminate, not clean)'; + } else { + stderr = `gh error: ${r.error.message}`; + } return { __error: true, status: -1, httpStatus: null, stderr }; } if (r.status !== 0) { From 0eb1054eecbedc31f327f842a9eb3c746b3fe693 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Thu, 13 Aug 2026 09:39:43 +0200 Subject: [PATCH 21/22] fix: address self-review issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scanner (P0-Functionality): allowlist staleness was adjudicated against `fileEntries`, which is the COMPLETE tracked set only in full-tree mode. In --staged and explicit-path modes it is a subset, so any allowlist entry the commit did not happen to touch was reported as "file is no longer tracked" — a factually false claim that exits 1. Latent today (both allowlists are empty by D-CB4), but it fires on the first legitimate entry and then rejects every commit through the pre-commit hook, training contributors toward --no-verify, which disables the gate for ALL commits. Same reasoning as the AC-6 --staged carve-out. Staleness is now adjudicated in full-tree mode only (D-CB6a); the full-tree CI scan remains the authoritative validator. Hazard SUPPRESSION is path-keyed and still applies in every mode. Regression coverage (applies ADR-009, avoids PF-013): Case 4 asserts --staged does not misreport an untouched allowlisted file; Case 5 is its positive control, proving full-tree mode still reports a genuinely stale entry so the fix cannot be satisfied by deleting the check; Case 6 pins that suppression still works in --staged while an identical hazard in a non-allowlisted staged file still fails. Verified non-vacuous against the pre-fix scanner: the Case 4 scenario exits 1 before the fix and 0 after. Docs (P2-Consistency): verify-pr-checks.mjs emits `--admin`, but four documents quoted the command without it. Corrected in PULL_REQUEST_TEMPLATE.md, CLAUDE.md, CHANGELOG.md and CONTRIBUTING.md so the printed command can be copied verbatim (RELEASING.md was already correct). --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- CHANGELOG.md | 8 +- CLAUDE.md | 2 +- CONTRIBUTING.md | 5 +- .../__test__/verify-no-control-bytes.spec.mjs | 83 +++++++++++++++++++ scripts/verify-no-control-bytes.mjs | 58 +++++++++---- 6 files changed, 135 insertions(+), 23 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index aafa980..34aa185 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -22,5 +22,5 @@ PR titles follow Conventional Commits (feat:, fix:, refactor:, chore:, docs:, .. - [ ] No new compiler/linter warnings - [ ] Source hygiene: `node scripts/verify-no-control-bytes.mjs` exits 0 - [ ] **Before any `--admin` merge**: run `node scripts/verify-pr-checks.mjs ` - and use the `gh pr merge --squash --match-head-commit ` command it emits + and use the `gh pr merge --squash --admin --match-head-commit ` command it emits (PF-017: a cancelled run reads as green without this check) diff --git a/CHANGELOG.md b/CHANGELOG.md index 120d073..083c0ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -306,10 +306,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 protection — absence is FAIL, not advisory (ADR-009, PF-013); Tier B fails on any non-required check-run that concluded `failure/cancelled/timed_out/action_required/stale`; Tier C (legacy commit - statuses) is advisory. It emits a `gh pr merge --squash --match-head-commit - ` command pinned to the verified SHA. Exit 0: Tier A, Tier A+, and Tier B - pass; exit 1: any Tier A, Tier A+, or Tier B failure, or zero check-runs found; - exit 2: tool/permission errors. + statuses) is advisory. It emits a `gh pr merge --squash --admin + --match-head-commit ` command pinned to the verified SHA. Exit 0: Tier A, + Tier A+, and Tier B pass; exit 1: any Tier A, Tier A+, or Tier B failure, or + zero check-runs found; exit 2: tool/permission errors. ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index ccf141a..77e144c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,4 +49,4 @@ See @RELEASING.md for the full runbook. - Local Python dev: `maturin develop` needs an active **virtualenv** + `python3` on PATH; CI has no venv so it uses `pip install ./crates/mds-python` (the maturin PEP 517 backend). Wheels are `cp311-abi3` (one per platform) - `crates/mds-python` is free-threading ready (frozen result classes, `#[pymodule(gil_used = false)]`, GIL released around each compile); the `cp314t` free-threaded wheel is a separate ABI and is deferred with the wheel matrix + PyPI publishing (follow-up to #132) - **Source hygiene gate** (#288): `node scripts/verify-no-control-bytes.mjs` scans tracked source for hazardous codepoints (C0, C1, bidi, BOM). BSD grep has no `-P` (exits 2, empty output reads as clean) — never use grep to verify absence of control bytes; the gate uses pure Node codepoint iteration. When writing codepoints in source or docs, use numeric notation (U+202E, 0x202e) rather than `\uXXXX` escapes — the edit tooling decodes 4-hex `\uXXXX` to live bytes (PF-018). -- **Pre-merge check verifier** (#289, PF-017): a CANCELLED GitHub Actions run reads as "not failing" to `gh pr merge --admin`, which can merge an unverified head. Before any `--admin` merge, run `node scripts/verify-pr-checks.mjs ` and use the `gh pr merge --squash --match-head-commit ` command it emits. This verifies all required contexts are `completed+success` and pins the SHA. +- **Pre-merge check verifier** (#289, PF-017): a CANCELLED GitHub Actions run reads as "not failing" to `gh pr merge --admin`, which can merge an unverified head. Before any `--admin` merge, run `node scripts/verify-pr-checks.mjs ` and use the `gh pr merge --squash --admin --match-head-commit ` command it emits verbatim. This verifies all required contexts are `completed+success` and pins the SHA. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2712792..b6f731b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,8 +134,9 @@ node scripts/verify-pr-checks.mjs The verifier reads required contexts from live branch protection, checks that every context is `status=completed` AND `conclusion=success`, and on pass -emits a `gh pr merge --squash --match-head-commit ` command pinned to -the verified SHA (closes the TOCTOU window). +emits a `gh pr merge --squash --admin --match-head-commit ` command pinned +to the verified SHA (closes the TOCTOU window). Copy it verbatim — hand-editing +the command is where `--match-head-commit` gets dropped. Exit codes are a contract: `0` all Tier A, Tier A+, and Tier B checks passed, `1` any Tier A failure (required context missing or non-success), any Tier A+ diff --git a/scripts/__test__/verify-no-control-bytes.spec.mjs b/scripts/__test__/verify-no-control-bytes.spec.mjs index 49032e2..86678f1 100644 --- a/scripts/__test__/verify-no-control-bytes.spec.mjs +++ b/scripts/__test__/verify-no-control-bytes.spec.mjs @@ -865,6 +865,89 @@ describe('AC-17: allowlist entries are self-invalidating', () => { } finally { cleanup(dir); } }); + /** + * D-CB6a: staleness is a whole-tree claim, so it is adjudicated in full-tree + * mode only. --staged scans a SUBSET; judging staleness there reports every + * allowlisted file the commit did not happen to touch as "no longer tracked" + * and rejects the commit. That would make the pre-commit hook unusable from + * the first legitimate allowlist entry onward and push contributors to + * --no-verify, which disables the gate for ALL commits. + * + * Case 4 is paired with an explicit positive control (Case 5) so the fix + * cannot be satisfied by simply deleting the staleness check + * (applies ADR-009, avoids PF-013). + */ + test('Case 4: --staged does not misreport an untouched allowlisted file as stale', () => { + const { dir, git } = mkTempGitRepo(); + try { + // evil.md carries its declared hazard and stays tracked but UNTOUCHED + // by the staged change. + writeFileSync(join(dir, 'evil.md'), Buffer.concat([Buffer.from('x '), Buffer.from([0x1b])])); + writeFileSync(join(dir, 'notes.md'), 'first\n'); + const target = writePatchedScanner(dir, "{ path: 'evil.md', codepoints: [0x1b], reason: 'test fixture' }"); + git('add', 'evil.md', 'notes.md', 'scan.mjs'); + git('commit', '-m', 'init'); + + // Stage an edit to a DIFFERENT, clean file — the ordinary commit shape. + writeFileSync(join(dir, 'notes.md'), 'second\n'); + git('add', 'notes.md'); + + const r = spawnSync(process.execPath, [target, '--staged'], + { cwd: dir, encoding: 'utf8', timeout: 30000 }); + assert.equal(r.status, 0, + 'a commit that does not touch the allowlisted file must not be rejected; ' + + `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.ok(!/stale/.test(r.stderr), + `evil.md is still tracked, so "stale" is a false claim; got: ${r.stderr}`); + } finally { cleanup(dir); } + }); + + test('Case 5 (positive control): the same entry IS reported stale by the full-tree scan', () => { + const { dir, git } = mkTempGitRepo(); + try { + // Identical patched scanner and identical entry as Case 4 — but here + // evil.md is genuinely absent, so full-tree mode must still catch it. + // Without this control, Case 4 would also pass if staleness detection + // were removed outright rather than scoped to full-tree mode. + writeFileSync(join(dir, 'notes.md'), 'first\n'); + const target = writePatchedScanner(dir, "{ path: 'evil.md', codepoints: [0x1b], reason: 'test fixture' }"); + git('add', 'notes.md', 'scan.mjs'); + + const r = runPatched(dir, target); + assert.equal(r.status, 1, 'full-tree mode must still adjudicate staleness'); + assert.ok(r.stderr.includes('stale'), `must identify the entry as stale; got: ${r.stderr}`); + assert.ok(r.stderr.includes('evil.md'), 'must name the stale path'); + } finally { cleanup(dir); } + }); + + test('Case 6: --staged still SUPPRESSES an allowlisted hazard in a staged file', () => { + const { dir, git } = mkTempGitRepo(); + try { + // Suppression is path-keyed and unaffected by D-CB6a: staging the + // allowlisted file itself must still pass, while an identical hazard in a + // NON-allowlisted staged file must still fail (the discriminating half). + writeFileSync(join(dir, 'notes.md'), 'first\n'); + const target = writePatchedScanner(dir, "{ path: 'evil.md', codepoints: [0x1b], reason: 'test fixture' }"); + git('add', 'notes.md', 'scan.mjs'); + git('commit', '-m', 'init'); + + writeFileSync(join(dir, 'evil.md'), Buffer.concat([Buffer.from('x '), Buffer.from([0x1b])])); + git('add', 'evil.md'); + const allowed = spawnSync(process.execPath, [target, '--staged'], + { cwd: dir, encoding: 'utf8', timeout: 30000 }); + assert.equal(allowed.status, 0, + `an allowlisted staged file must pass; stdout: ${allowed.stdout} stderr: ${allowed.stderr}`); + + writeFileSync(join(dir, 'other.md'), Buffer.concat([Buffer.from('x '), Buffer.from([0x1b])])); + git('add', 'other.md'); + const denied = spawnSync(process.execPath, [target, '--staged'], + { cwd: dir, encoding: 'utf8', timeout: 30000 }); + assert.equal(denied.status, 1, + 'the same hazard in a NON-allowlisted staged file must still fail'); + assert.ok(denied.stderr.includes('other.md'), `must name the offending path; got: ${denied.stderr}`); + } finally { cleanup(dir); } + }); + }); // --------------------------------------------------------------------------- diff --git a/scripts/verify-no-control-bytes.mjs b/scripts/verify-no-control-bytes.mjs index d30f24f..f87716c 100644 --- a/scripts/verify-no-control-bytes.mjs +++ b/scripts/verify-no-control-bytes.mjs @@ -37,6 +37,11 @@ * deliberate, tested (see spec test "D-CB5 --staged carve-out from AC-6"), * and leaves the full-tree CI scan as the authoritative AC-6 gate. * + * D-CB6a: allowlist STALENESS is adjudicated in full-tree mode only. Staleness + * is a claim about the whole tracked tree, and --staged / explicit-path modes + * scan a subset that cannot support it. Hazard SUPPRESSION by an allowlist + * entry still applies in every mode. See the stale-allowlist block in main(). + * * D-CB8: --staged mode reads file content from the git index via a single * `git cat-file --batch` subprocess (all staged blobs at once), never from * the working tree. Staging a clean file then modifying the working copy @@ -529,6 +534,11 @@ function main() { const cwd = process.cwd(); + // Full-tree mode is the only mode in which `fileEntries` is the COMPLETE + // tracked set. --staged and explicit-path modes scan a subset, which changes + // what the allowlist staleness check below can legitimately conclude. + const isFullTree = explicitPaths.length === 0 && !isStaged; + // Verify git is accessible and we are in a repo (D-CB5) assertGitRepo(cwd); @@ -671,22 +681,40 @@ function main() { } // ---- Stale allowlist check (D-CB6) ---- - for (const entry of BINARY_ALLOWLIST) { - const exists = fileEntries.some(e => e.path === entry.path); - if (!exists) { - errors.push(`BINARY_ALLOWLIST: stale entry "${entry.path}" — file is no longer tracked`); + // + // D-CB6a: FULL-TREE MODE ONLY. Staleness is a claim about the whole tracked + // tree ("this entry no longer corresponds to anything"), and only full-tree + // mode holds the evidence for it. In --staged and explicit-path modes + // `fileEntries` is a SUBSET, so every allowlist entry outside that subset + // would be reported as "no longer tracked" — a factually false message that + // fails every commit not touching the allowlisted file. That turns the + // pre-commit hook into a wall the moment a first legitimate entry is added, + // and trains contributors toward `--no-verify`, which disables the gate for + // ALL commits — the same reasoning that drives the AC-6 --staged carve-out + // above. The full-tree CI scan is the authoritative allowlist validator, and + // it runs on every pull_request and on every tag push. + // + // Hazard SUPPRESSION is unaffected and still applies in every mode: + // hazardAllowMap is keyed by path, so an allowlisted file staged with its + // declared codepoints still passes. Only the staleness verdict is deferred. + if (isFullTree) { + for (const entry of BINARY_ALLOWLIST) { + const exists = fileEntries.some(e => e.path === entry.path); + if (!exists) { + errors.push(`BINARY_ALLOWLIST: stale entry "${entry.path}" — file is no longer tracked`); + } } - } - for (const entry of HAZARD_ALLOWLIST) { - const exists = fileEntries.some(e => e.path === entry.path); - if (!exists) { - errors.push(`HAZARD_ALLOWLIST: stale entry "${entry.path}" — file is no longer tracked`); - } else { - // Verify the declared codepoints actually occur in the file - for (const cp of entry.codepoints) { - const key = `${entry.path}:${cp}`; - if (!exercisedAllowlist.has(key)) { - errors.push(`HAZARD_ALLOWLIST: stale entry "${entry.path}" cp U+${cp.toString(16).toUpperCase().padStart(4, '0')} — codepoint not found in file`); + for (const entry of HAZARD_ALLOWLIST) { + const exists = fileEntries.some(e => e.path === entry.path); + if (!exists) { + errors.push(`HAZARD_ALLOWLIST: stale entry "${entry.path}" — file is no longer tracked`); + } else { + // Verify the declared codepoints actually occur in the file + for (const cp of entry.codepoints) { + const key = `${entry.path}:${cp}`; + if (!exercisedAllowlist.has(key)) { + errors.push(`HAZARD_ALLOWLIST: stale entry "${entry.path}" cp U+${cp.toString(16).toUpperCase().padStart(4, '0')} — codepoint not found in file`); + } } } } From 5a21b3ea92e11355807a0a249f3b0187bdb23b40 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 19 Aug 2026 09:21:40 +0300 Subject: [PATCH 22/22] chore(devflow): share feature knowledge bases via git Adds the seven feature knowledge bases that .gitignore already whitelists (`!.devflow/features/*/KNOWLEDGE.md`, "feature knowledge bases are shared via git") and that the tracked .devflow/features/index.md already references: bundler-plugins, mds-cli, mds-compiler, mds-fmt, mds-js, mds-napi, source-map-security mds-lint/KNOWLEDGE.md was already tracked; this brings the rest of the index in line so every referenced knowledge base resolves. Co-Authored-By: Claude --- .../features/bundler-plugins/KNOWLEDGE.md | 187 +++++++++++++ .devflow/features/mds-cli/KNOWLEDGE.md | 222 ++++++++++++++++ .devflow/features/mds-compiler/KNOWLEDGE.md | 222 ++++++++++++++++ .devflow/features/mds-fmt/KNOWLEDGE.md | 251 ++++++++++++++++++ .devflow/features/mds-js/KNOWLEDGE.md | 193 ++++++++++++++ .devflow/features/mds-napi/KNOWLEDGE.md | 156 +++++++++++ .../features/source-map-security/KNOWLEDGE.md | 190 +++++++++++++ 7 files changed, 1421 insertions(+) create mode 100644 .devflow/features/bundler-plugins/KNOWLEDGE.md create mode 100644 .devflow/features/mds-cli/KNOWLEDGE.md create mode 100644 .devflow/features/mds-compiler/KNOWLEDGE.md create mode 100644 .devflow/features/mds-fmt/KNOWLEDGE.md create mode 100644 .devflow/features/mds-js/KNOWLEDGE.md create mode 100644 .devflow/features/mds-napi/KNOWLEDGE.md create mode 100644 .devflow/features/source-map-security/KNOWLEDGE.md diff --git a/.devflow/features/bundler-plugins/KNOWLEDGE.md b/.devflow/features/bundler-plugins/KNOWLEDGE.md new file mode 100644 index 0000000..fe655e1 --- /dev/null +++ b/.devflow/features/bundler-plugins/KNOWLEDGE.md @@ -0,0 +1,187 @@ +--- +feature: bundler-plugins +name: Bundler Plugins (bundler-utils + Vite/Rollup/Webpack/Rspack) +description: "Use when adding a new bundler integration, modifying the emitted-module contract, debugging HMR behavior, working on the CJS compatibility shim, updating the transformer/loader factory, registering a new package in the release pipeline, or investigating why a .mds file emits unexpected output. Keywords: createMdsTransformer, createMdsLoader, bundler-utils, vite-plugin, rollup-plugin, webpack-loader, rspack-loader, addWatchFile, addDependency, handleHotUpdate, emitted module contract, export default string, export default Message[], safeJsonForJs, escapeForJs, metadata, kind, markdown, messages, discriminated union, mds.d.ts, MdsMessage, string | MdsMessage[]." +category: component-patterns +directories: ["packages/bundler-utils/", "packages/vite-plugin/", "packages/rollup-plugin/", "packages/webpack-loader/", "packages/rspack-loader/"] +referencedFiles: + - packages/bundler-utils/src/transform.ts + - packages/bundler-utils/src/types.ts + - packages/bundler-utils/src/loader.ts + - packages/bundler-utils/src/frontmatter.ts + - packages/bundler-utils/src/lazy-init.ts + - packages/bundler-utils/mds.d.ts + - packages/bundler-utils/src/index.ts + - packages/vite-plugin/src/index.ts + - packages/rollup-plugin/src/index.ts + - packages/webpack-loader/src/index.ts + - packages/rspack-loader/src/index.ts +created: 2026-06-26 +updated: 2026-06-26 +--- + +# Bundler Plugins (bundler-utils + Vite/Rollup/Webpack/Rspack) + +## Overview + +`packages/bundler-utils/` is the shared transformation layer consumed by four bundler plugins: `vite-plugin`, `rollup-plugin`, `webpack-loader`, `rspack-loader`. It implements `createMdsTransformer` (used by Vite/Rollup) and `createMdsLoader` (used by Webpack/Rspack). After the intrinsic-output refactor, the emitted JS module branches on the compiled `kind` — a markdown `.mds` emits a string default export, a messages `.mds` emits a `Message[]` default export. The published `mds.d.ts` ambient declaration reflects this widened type. + +## Core Responsibilities + +- `transform.ts`: compile `.mds` files via `MdsApi.compileFile`, emit the JS module source (`export default`), serialize metadata +- `loader.ts`: webpack/rspack integration via `createMdsLoader` +- `frontmatter.ts`: `shouldTransform(id)` — decides if a module ID refers to an `.mds` file +- `lazy-init.ts`: `LazyInit` — ensures `mds.init()` is awaited exactly once per transformer instance +- Does NOT: implement compilation logic, manage caching, handle HMR (delegated to plugin wrappers) + +## Standard Structure + +### Emitted module contract (post-refactor) + +The emitted JS module branches on `result.kind`: + +```typescript +// transform.ts — inside transform() +let defaultExport: string; +if (result.kind === 'markdown') { + // Escape the string for embedding in a double-quoted JS literal + defaultExport = `export default "${escapeForJs(result.output)}";\n`; +} else { + // kind === 'messages' — serialize the messages array as safe inline JSON + defaultExport = `export default ${safeJsonForJs(result.messages)};\n`; +} + +const code = + defaultExport + + `export const metadata = ${safeJsonForJs({ warnings: result.warnings, dependencies: result.dependencies })};\n`; +``` + +So for a markdown `.mds`: `export default "..."` (string) +For a messages `.mds`: `export default [{role:"...", content:"..."}]` (array literal) + +Both emit `export const metadata = { warnings: [...], dependencies: [...] };` + +### safeJsonForJs vs escapeForJs + +These two serializers have different contracts and must not be swapped: + +- `escapeForJs(str: string): string` — escapes special chars for embedding inside a double-quoted JS string literal (`"..."`) +- `safeJsonForJs(value: unknown): string` — `JSON.stringify` + escapes `<`, U+2028, U+2029 for safe inline `