From 2af7630642ef5ac438534f58b5787e94a5443384 Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 15 Jun 2026 14:19:35 -0400 Subject: [PATCH] fix(1259-01): make real runner cross-platform/version portable (CI ubuntu-24 + windows-24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed on node 24 + Windows in the real-runner E2E tests — real portability bugs in the shipping runner, not test flakiness: - node 24 names a zero-test file's TAP entry by ABSOLUTE path (node 22 used the basename), so the exact-string vacuity discriminator misfired -> empty file falsely greened. Now compares BASENAMES (separator-agnostic) -> robust across OS + node version. Verified: empty file is non-green under node 24 locally. - the lint-rule runner spawned 'npx', which execFileSync can't launch on Windows (and failed fast on ubuntu-24). Now resolves the project's eslint CLI via eslint/package.json and runs 'node ' through process.execPath (portable; no shell -> no injection). eslint absent -> fail-closed. - node-test also spawns via process.execPath, not bare 'node'. Also folds in round-3 WR-01: the probe-core inline comments still said enforcement was 'deferred to a follow-up PR' — corrected (landed in #1259). Strengthened the isNonVacuousNodeTestPass unit test to pin the basename clause (absolute-path target). Verified 71/71 green under BOTH node 22 and node 24. --- src/probe-core.cts | 10 ++--- src/prohibition-enforcement.cts | 59 ++++++++++++++++++++------ tests/prohibition-enforcement.test.cjs | 10 +++-- 3 files changed, 57 insertions(+), 22 deletions(-) diff --git a/src/probe-core.cts b/src/probe-core.cts index f9a419759..85a39f395 100644 --- a/src/probe-core.cts +++ b/src/probe-core.cts @@ -397,9 +397,9 @@ export function dispositionForProhibition( const hasEnforcement = evidence.length > 0; // FAIL CLOSED: no wired enforcement evidence -> flagged unverified, never green. This holds for - // every tier today (the real enforcement mechanism that could flip a test-tier item to green is - // deferred to a follow-up PR). The guard the safety assertion proves: an unwired item can never - // be silently skipped. + // every tier (the producer that builds enforcement evidence for a test-tier item — the + // `prohibition-enforcement` module — landed in #1259). The guard the safety assertion proves: an + // unwired item can never be silently skipped. if (!hasEnforcement) { return { status: 'unverified', @@ -414,8 +414,8 @@ export function dispositionForProhibition( // D4 GUARD: a judgment-tier (or unknown-tier) prohibition is NEVER a silent green from this // deterministic helper — it always routes to human/LLM judgment review (ADR-550 D4; verify-phase.md). - // Only a test-tier item with wired enforcement evidence may go green, and even that is the deferred - // heavy half until the real negative-test enforcement mechanism lands (no #644 caller passes evidence). + // Only a test-tier item with wired enforcement evidence may go green; the producer that supplies + // that evidence (`prohibition-enforcement`, #1259) runs the wired check and requires a genuine pass. if (tier === 'test') { return { status: 'green', diff --git a/src/prohibition-enforcement.cts b/src/prohibition-enforcement.cts index f4a5d6b36..c63afac21 100644 --- a/src/prohibition-enforcement.cts +++ b/src/prohibition-enforcement.cts @@ -36,6 +36,7 @@ */ import fs from 'node:fs'; +import path from 'node:path'; import { execFileSync } from 'node:child_process'; // eslint-disable-next-line @typescript-eslint/no-require-imports import core = require('./core.cjs'); @@ -104,12 +105,34 @@ export function buildNodeTestArgs(check: CheckDescriptor): string[] { return ['--test', '--test-reporter=tap', check.target]; } -/** eslint argv (the args AFTER `npx`). Runs the project flat config so plugin rules (e.g. `local/*`) - * load — `--rule` CANNOT load a plugin, so we lint the TARGET path as JSON and filter by rule id. - * `--no-warn-ignored` makes an eslint-IGNORED target return `[]` (not a length-1 "File ignored" - * warning result) so an ignored path fails closed via the vacuity guard instead of falsely greening. */ +/** eslint argv (the args AFTER the eslint CLI path). Runs the project flat config so plugin rules + * (e.g. `local/*`) load — `--rule` CANNOT load a plugin, so we lint the TARGET path as JSON and + * filter by rule id. `--no-warn-ignored` makes an eslint-IGNORED target return `[]` (not a length-1 + * "File ignored" result) so an ignored path fails closed via the vacuity guard, not a false green. */ export function buildLintArgs(check: CheckDescriptor): string[] { - return ['eslint', '--no-warn-ignored', '--format', 'json', check.target]; + return ['--no-warn-ignored', '--format', 'json', check.target]; +} + +/** + * Resolve the project's eslint CLI entry portably (no `npx` — not spawnable via `execFileSync` on + * Windows). Resolves eslint's package.json from the target project's `node_modules` and derives + * `bin/eslint.js`, so it is run as `node ` (portable). Returns null if eslint is not installed + * (→ the lint-rule check fails closed, never throws). + */ +function resolveEslintCli(cwd: string): string | null { + try { + const pkg = require.resolve('eslint/package.json', { paths: [cwd] }); + const cli = path.join(path.dirname(pkg), 'bin', 'eslint.js'); + return fs.existsSync(cli) ? cli : null; + } catch { + return null; + } +} + +/** Basename of a path, separator-agnostic (handles `\` and `/` so node-test names compare stably + * across OSes / node versions that report the file-test by differing path forms). */ +function baseOf(p: string): string { + return typeof p === 'string' ? (p.split(/[\\/]/).pop() ?? p) : ''; } /** @@ -156,8 +179,11 @@ export function tapTestNames(out: string): string[] { export function isNonVacuousNodeTestPass(out: string, target: string): boolean { const s = parseNodeTestSummary(out); if (!(s.tests >= 1 && s.pass >= 1 && s.fail === 0)) return false; - const base = typeof target === 'string' ? (target.split(/[\\/]/).pop() ?? target) : ''; - return tapTestNames(out).some((n) => n !== base && n !== target); + // Compare BASENAMES: node reports the file-test by varying path forms across OS / node version + // (absolute, relative, normalized), so an exact-string compare misfires. A real test name (e.g. + // "guards the must-NOT") has no separators, so its basename never equals the target file's. + const tgtBase = baseOf(target); + return tapTestNames(out).some((n) => baseOf(n) !== tgtBase); } /** Number of file results in an eslint `--format json` report (0 if unparseable / not an array). */ @@ -199,10 +225,13 @@ export function eslintJsonHasRule(jsonText: string, rule: string): boolean { * AND a reported test named distinctly from the file). A bare exit 0 for an empty/zero-test file * — which `node --test` counts as one passing "test" named after the file — is NOT a pass (the * #1259 BL-01 false-green fix). - * - lint-rule: runs the project `eslint --format json ` (flat config loads `local/*` - * plugins) and requires the target to actually lint (>=1 file result) AND ZERO messages for the - * specific rule id. `--rule` cannot load a plugin rule, so we filter the structured report by - * `ruleId` instead (the #1259 SF-01 fix). + * - lint-rule: runs the project eslint as `node --format json ` (flat config + * loads `local/*` plugins) and requires the target to actually lint (>=1 file result) AND ZERO + * messages for the specific rule id. `--rule` cannot load a plugin rule, so we filter the + * structured report by `ruleId` instead (the #1259 SF-01 fix). + * + * Both kinds spawn via `process.execPath` (never bare `node`/`npx` — not portably spawnable via + * `execFileSync` on Windows) with arg arrays (no shell → no injection from a caller-supplied target). */ /** * Env for spawned checks: strip `NODE_TEST_CONTEXT` and `NODE_OPTIONS` so an AMBIENT test-runner @@ -222,7 +251,7 @@ function defaultRunCheck(check: CheckDescriptor, cwd: string): CheckRunResult { if (check.kind === 'node-test') { let out = ''; try { - out = execFileSync('node', buildNodeTestArgs(check), { + out = execFileSync(process.execPath, buildNodeTestArgs(check), { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], @@ -231,16 +260,18 @@ function defaultRunCheck(check: CheckDescriptor, cwd: string): CheckRunResult { }); } catch (e) { // A failing test run exits non-zero (TAP still on stdout). Parse it: a real failure has - // `# fail >= 1` -> non-vacuous check returns false. Missing `node` -> no stdout -> false. + // `# fail >= 1` -> non-vacuous check returns false. Missing node -> no stdout -> false. const stdout = e && typeof e === 'object' && 'stdout' in e ? (e as { stdout?: unknown }).stdout : ''; out = typeof stdout === 'string' ? stdout : ''; } return { passed: isNonVacuousNodeTestPass(out, check.target) }; } if (check.kind === 'lint-rule') { + const eslintCli = resolveEslintCli(cwd); + if (!eslintCli) return { passed: false }; // eslint not installed -> fail closed, never throw let json = ''; try { - json = execFileSync('npx', buildLintArgs(check), { + json = execFileSync(process.execPath, [eslintCli, ...buildLintArgs(check)], { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], diff --git a/tests/prohibition-enforcement.test.cjs b/tests/prohibition-enforcement.test.cjs index 41a3b70d7..607e54e8f 100644 --- a/tests/prohibition-enforcement.test.cjs +++ b/tests/prohibition-enforcement.test.cjs @@ -99,7 +99,6 @@ describe('prohibition-enforcement: deterministic test-tier producer (#1259 / ADR 'must export buildLintArgs — the eslint argv builder for the lint-rule real runner'); const argv = enforce.buildLintArgs({ kind: 'lint-rule', rule: 'local/no-source-grep', target: 'tests/' }); assert.ok(Array.isArray(argv), 'argv is an array'); - assert.equal(argv[0], 'eslint'); const fmtIdx = argv.indexOf('--format'); assert.ok(fmtIdx !== -1 && argv[fmtIdx + 1] === 'json', 'emits --format json so the report can be filtered by ruleId'); @@ -241,9 +240,14 @@ describe('prohibition-enforcement real-runner helpers (#1259)', () => { const empty = 'ok 1 - empty.test.cjs\n1..1\n# tests 1\n# pass 1\n# fail 0\n'; assert.equal(enforce.isNonVacuousNodeTestPass(empty, 'empty.test.cjs'), false, 'a file-named-only result is vacuous — the BL-01 false-green guard'); + // BASENAME-NORMALIZED: node may report the file-test by an ABSOLUTE/normalized path while the + // descriptor target is relative (cross-OS / node-version). The basenames must still match → vacuous. + const emptyAbs = 'ok 1 - /tmp/x/empty.test.cjs\n1..1\n# tests 1\n# pass 1\n# fail 0\n'; + assert.equal(enforce.isNonVacuousNodeTestPass(emptyAbs, 'empty.test.cjs'), false, + 'an absolute-path file-test name must still be recognized as vacuous (basename compare, WR-02)'); const real = 'ok 1 - guards the must-NOT\n1..1\n# tests 1\n# pass 1\n# fail 0\n'; - assert.equal(enforce.isNonVacuousNodeTestPass(real, 'neg.test.cjs'), true, - 'a real named test distinct from the file is a genuine pass'); + assert.equal(enforce.isNonVacuousNodeTestPass(real, '/abs/path/neg.test.cjs'), true, + 'a real named test distinct from the file is a genuine pass (even vs an absolute target)'); const failing = 'not ok 1 - guards\n# tests 1\n# pass 0\n# fail 1\n'; assert.equal(enforce.isNonVacuousNodeTestPass(failing, 'neg.test.cjs'), false, 'any failure means not a pass');