fix(1259-01): make real runner cross-platform/version portable (CI ubuntu-24 + windows-24)
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 <cli>' 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.
This commit is contained in:
@@ -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',
|
||||
|
||||
@@ -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 <cli>` (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 <target>` (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 <eslint-cli> --format json <target>` (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'],
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user