* refactor(#3180): one owner for completion ratio, a prompt-layer drift guard, and a written behavior contract The 2026-08-08 coverage audit on #3180 found the epic's copy counts were a lower bound for the third consecutive time, and that two derivation families had never been named at all. ADR-3180 gains Decision 7 — a normative behavior contract that says what the right answer IS for each derivation, not merely who owns it. A reviewer with no written rule can only ask "does this look like the others", which is how a fifth copy passes review. Decision 4 gains (d) scan surface is every authored surface and an owner FILE is never exempt, only its named functions; and (e) a surface that cannot be consolidated today ships ratcheted, never unguarded. Completion ratio: `clampPercent` sat exported and unused beside six hand-inlined copies of its own body across five modules. All six now route through it; `clampPercentFromFraction` is added for the one caller that already held a fraction. Every migration is behaviour-identical — clampPercent's first line IS the `total > 0 ? … : 0` ternary each copy carried. Guarded by lint-completion-ratio-drift.cjs, which reports zero re-derivations with no file-level exemption. Prompt layer: workflow markdown re-derives live-plan counting in raw shell (#1762), invisible to every `src/`-scoped guard. lint-planning-prompt-drift.cjs scans it with a shrink-only baseline of the 7 sites that exist today — new sites fail, and a baseline entry that stops firing fails too, so an acknowledgment can never outlive the thing it describes. lint-milestone-window-drift.cjs stops exempting its owner file wholesale; only the four named canonical functions are exempt now. The blanket exemption was pointed at the one file most likely to grow the next copy, and it had. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3180): link Phases 6-8 sub-issues (#3216, #3217, #3218) from ADR-3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): address orthogonal review — consumer-output identity tests, count-keyed ratchet, property coverage Five findings from the two orthogonal review passes, all fixed. Decision 4(c) breach: the completion-ratio identity test asserted at the OWNER, which is exactly the bypass that decision exists to close — a consumer can call clampPercent and then post-process locally, leaving both the lint and an owner-level test green. It now drives `roadmap analyze`, `query progress` and `stats` and asserts on their own output, over a fixture containing a `status: superseded` plan so a consumer that re-counted raw files would report 60 where the owner reports 75. Decision 4(e) breach: ratchet entries named the epic (#3180) rather than the issue that removes them. They name Phase 8 (#3218) now. The ratchet keyed on (file, text) alone, so plan-phase.md's two byte-identical sites were one indistinguishable key and migrating either would have left the guard green with the other alive. Entries carry an occurrence count; fewer than acknowledged fails as a partial migration, more fails as a new copy. Adds the missing MAX_REGEX_LITERAL_LEN boundary coverage the sibling guard's test already had, and the fast-check property tests CONTRIBUTING requires for clamp/budget-limit functions. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: stop wrapping a nested double-spawn in a 15s wall-clock budget (bug #641 probes) `tests/ci-test-scope.test.cjs`'s `bug #641` block spawned `run-tests.cjs` under PROBE_TIMEOUT_MS=15000; that child then spawned a nested `node --test`. A fixed wall-clock budget around a double spawn, running inside a container that is concurrently executing the full ~31k-test suite, fails by construction under load. Confirmed against three full matrix runs. Every failure was shaped `null !== 0` — the child was KILLED, never an assertion about the thing under test. One captured probe had already printed the correct resolution (`suite="all" files=2: a.test.cjs b.test.cjs`) and was killed anyway. It reproduces on `next` alone: 5 failures on linux-node22, 0 on linux-node24. The victim subset varies by run and by lane. What these tests are actually about is suite-token RESOLUTION — `unit` as a bare token in --files/--files-from. Executing the seeded trivial files is incidental and is the entire timeout surface, so the assertions move in-process against the same functions `main()` calls, in the same order. `parseArgs`, `selectExplicitFiles`, `selectFiles` and `walkTestFiles` are exported for that; no behavior, signature or logic changed. No coverage lost: `tests/run-tests-harness.test.cjs` already spawns the harness for real and asserts exit codes end to end, on a 120s budget. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: delete the three elapsed-time assertions CLAUDE.md forbids asserting on wall-clock time. Three assertions did, and all three are load-sensitive: on a saturated bench each can fail while the code under test is correct. In every case the load-bearing assertion sits on the line above and the timing line adds no discrimination. run-with-timeout: the stated worry — "was this 124 the cap firing or the 30s harness backstop?" — is already answered by the assertion above it. A backstop kills by signal, which surfaces as status null, never 124. Observed directly this session: three matrix runs produced exactly that null shape from killed children. normalize-test-command and context-predicates: both bounded a ReDoS check. A threshold only ever separates "fast" from "slightly slow", which is bench load, not correctness — catastrophic backtracking on 800 KB of input does not take 251ms, it does not finish at all. A real regression therefore shows up as the suite being killed on that test, which is louder and more reliable than a number. The structural assertions (returned unchanged; cleanly rejected) are what actually carry those tests, and they stay. The sweep now reports zero elapsed-time assertions in tests/. The remaining Date.now() uses are unique-path suffixes, barrier deadlines, fixture timestamps and fake mtimes — none of them assertions. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3180): backfill changeset PR number (#3223) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): key the prompt-drift ratchet on POSIX paths so it works on Windows The baseline keys on (file, trimmed text). `file` came from scanTree's `path.relative()`, which uses NATIVE separators, while the committed baseline stores POSIX. On Windows every violation was therefore unmatched — reported as FRESH — and every baseline entry matched nothing — reported as STALE. The guard failed 100% of the time there, on both CI shards: ✖ scanRepo(repoRoot) matches the baseline exactly: zero fresh AND zero stale + { file: 'gsd-core\\workflows\\execute-plan.md', ... } The remote runner this repo gates on is Linux-only and cannot see this class at all; the GitHub Actions Windows lane is what caught it. Normalization is unconditional — never gated on process.platform. A platform-conditional normalizer makes the POSIX path the special case and leaves the Windows branch unexercised on every other OS, which is the same blind spot in a different place. It is applied at one seam inside findPromptDrift, which builds `file` on every returned violation, so the baseline key, the --update writer, the stderr report and the tests all consume one normalized value. The regression tests drive a Windows-shaped relPath directly and run on every OS rather than skipping off-Windows — a test that only runs on the platform where the bug lives is why this escaped. They include a sanity check that un-normalized input does NOT match, so the assertion cannot pass vacuously. Audited the three sibling guards: none keys against a committed cross-platform baseline, and their exemption keys are path.join-built, so producer and consumer share the native convention. Left correct code alone rather than making them look alike. scripts/lib/drift-scan.cjs is untouched — normalizing there would break those three on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
162 lines
7.1 KiB
JavaScript
162 lines
7.1 KiB
JavaScript
'use strict';
|
|
|
|
/**
|
|
* Regression tests for the test-command normalizer (#1857).
|
|
*
|
|
* A GSD test gate must not hang forever on a watch-mode runner. The normalizer
|
|
* rewrites a resolved test command to a best-effort one-shot form; the gate's
|
|
* wall-clock timeout is the ultimate guarantee (asserted separately as workflow
|
|
* content). These tests cover the two normalization acceptance criteria:
|
|
* (a) an already-one-shot command is invoked UNCHANGED (never double-flagged),
|
|
* (b) a watch-mode command is normalized to a one-shot form.
|
|
*/
|
|
|
|
const { test, describe, beforeEach, afterEach } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('node:fs');
|
|
const os = require('node:os');
|
|
const path = require('node:path');
|
|
|
|
const { cleanup } = require('./helpers.cjs');
|
|
|
|
const normalizer = require('../gsd-core/bin/lib/normalize-test-command.cjs');
|
|
const { normalizeTestCommand, isAlreadyOneShot } = normalizer;
|
|
|
|
function tmpProject(testScript) {
|
|
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1857-'));
|
|
fs.writeFileSync(
|
|
path.join(dir, 'package.json'),
|
|
JSON.stringify({ scripts: { test: testScript } }),
|
|
);
|
|
return dir;
|
|
}
|
|
|
|
describe('normalizeTestCommand: direct vitest (watch by default) → one-shot (#1857)', () => {
|
|
test('bare vitest gains "run"', () => {
|
|
assert.strictEqual(normalizeTestCommand('vitest', '/tmp'), 'vitest run');
|
|
});
|
|
test('npx vitest with flags gains "run" and keeps flags', () => {
|
|
assert.strictEqual(normalizeTestCommand('npx vitest --coverage', '/tmp'), 'npx vitest run --coverage');
|
|
});
|
|
test('vitest --watch is stripped and made one-shot', () => {
|
|
assert.strictEqual(normalizeTestCommand('vitest --watch', '/tmp'), 'vitest run');
|
|
});
|
|
});
|
|
|
|
describe('normalizeTestCommand: already one-shot is a no-op (#1857)', () => {
|
|
for (const cmd of ['vitest run', 'vitest --run', 'npx vitest run -t foo', 'jest --watchAll=false']) {
|
|
test(`"${cmd}" is recognised one-shot and returned unchanged`, () => {
|
|
assert.strictEqual(normalizeTestCommand(cmd, '/tmp'), cmd);
|
|
assert.ok(isAlreadyOneShot(cmd), `"${cmd}" should be recognised as one-shot`);
|
|
});
|
|
}
|
|
// jest is one-shot by default (no --watch); --runInBand doesn't disable watch,
|
|
// so it isn't a one-shot MARKER — but a jest command without a watch flag is
|
|
// still returned unchanged via the jest branch.
|
|
for (const cmd of ['jest', 'jest --runInBand']) {
|
|
test(`"${cmd}" (jest, no watch flag) is returned unchanged`, () => {
|
|
assert.strictEqual(normalizeTestCommand(cmd, '/tmp'), cmd);
|
|
});
|
|
}
|
|
});
|
|
|
|
describe('normalizeTestCommand: direct jest with watch flag → one-shot (#1857)', () => {
|
|
test('jest --watch → --watchAll=false', () => {
|
|
assert.strictEqual(normalizeTestCommand('jest --watch', '/tmp'), 'jest --watchAll=false');
|
|
});
|
|
test('jest --watchAll → --watchAll=false', () => {
|
|
assert.strictEqual(normalizeTestCommand('jest --watchAll', '/tmp'), 'jest --watchAll=false');
|
|
});
|
|
});
|
|
|
|
describe('normalizeTestCommand: package-manager script invocation inspects package.json (#1857)', () => {
|
|
let vitestDir; let vitestRunDir; let jestDir;
|
|
beforeEach(() => {
|
|
vitestDir = tmpProject('vitest'); // watches by default
|
|
vitestRunDir = tmpProject('vitest run'); // already one-shot
|
|
jestDir = tmpProject('jest'); // one-shot by default
|
|
});
|
|
afterEach(() => {
|
|
for (const d of [vitestDir, vitestRunDir, jestDir]) {
|
|
cleanup(d);
|
|
}
|
|
});
|
|
|
|
test('npm test whose script is watch-vitest → CI=true prefix', () => {
|
|
assert.strictEqual(normalizeTestCommand('npm test', vitestDir), 'CI=true npm test');
|
|
});
|
|
test('pnpm test whose script is watch-vitest → CI=true prefix', () => {
|
|
assert.strictEqual(normalizeTestCommand('pnpm test', vitestDir), 'CI=true pnpm test');
|
|
});
|
|
test('npm test whose script is already "vitest run" → unchanged', () => {
|
|
assert.strictEqual(normalizeTestCommand('npm test', vitestRunDir), 'npm test');
|
|
});
|
|
test('npm test whose script is jest (one-shot) → unchanged', () => {
|
|
assert.strictEqual(normalizeTestCommand('npm test', jestDir), 'npm test');
|
|
});
|
|
|
|
test('pnpm --dir <app> test inspects the target package.json (#1857 comment 5)', () => {
|
|
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1857-root-'));
|
|
try {
|
|
fs.mkdirSync(path.join(root, 'app'));
|
|
fs.writeFileSync(path.join(root, 'app', 'package.json'), JSON.stringify({ scripts: { test: 'vitest' } }));
|
|
assert.strictEqual(normalizeTestCommand('pnpm --dir app test', root), 'CI=true pnpm --dir app test');
|
|
// An explicit --run the user already passed is respected (no-op); the
|
|
// gate timeout backstops a project that still watches despite --run.
|
|
assert.strictEqual(normalizeTestCommand('pnpm --dir app test -- --run', root), 'pnpm --dir app test -- --run');
|
|
} finally {
|
|
cleanup(root);
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('normalizeTestCommand: non-JS / unknown runners are untouched (#1857)', () => {
|
|
for (const cmd of ['cargo test', 'go test ./...', 'make test', 'python -m pytest -q', 'true', '']) {
|
|
test(`"${cmd}" is returned unchanged`, () => {
|
|
assert.strictEqual(normalizeTestCommand(cmd, '/tmp'), cmd);
|
|
});
|
|
}
|
|
test('npm test with no package.json is left unchanged (cannot classify)', () => {
|
|
const empty = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1857-empty-'));
|
|
try {
|
|
assert.strictEqual(normalizeTestCommand('npm test', empty), 'npm test');
|
|
} finally {
|
|
cleanup(empty);
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('normalizeTestCommand: security hardening (#1857 review)', () => {
|
|
// "vitest"/"jest" as a token that isn't the invoked binary must NOT be mangled.
|
|
for (const cmd of ['make test-vitest', 'node ./scripts/run-vitest.js', './bin/vitest-wrapper.sh', 'cat vitest.config.js', 'node jest-runner.js']) {
|
|
test(`"${cmd}" is not mangled (word-token, not substring)`, () => {
|
|
assert.strictEqual(normalizeTestCommand(cmd, '/tmp'), cmd);
|
|
});
|
|
}
|
|
|
|
test('an oversized command is returned unchanged (no ReDoS)', () => {
|
|
// The blow-up input from the review: a long "npm " run with no `test` token.
|
|
const huge = 'npm '.repeat(200000); // ~800 KB
|
|
const out = normalizeTestCommand(huge, '/tmp');
|
|
// No elapsed-time bound: catastrophic backtracking on an 800 KB input
|
|
// does not take 251ms, it does not finish at all. A real ReDoS
|
|
// regression manifests as the suite being killed on this test, which is
|
|
// a louder and more reliable signal than a threshold — the threshold
|
|
// only ever distinguished "fast" from "slightly slow" (bench load), not
|
|
// correctness.
|
|
assert.strictEqual(out, huge, 'oversized input must be returned unchanged');
|
|
});
|
|
|
|
test('a package.json that is not a regular file is ignored (no FIFO hang)', () => {
|
|
// resolvePackageDir would point here; a non-regular "package.json" must be skipped.
|
|
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1857-notfile-'));
|
|
try {
|
|
fs.mkdirSync(path.join(dir, 'package.json')); // a DIRECTORY named package.json
|
|
// Classified as "cannot determine runner" → command returned unchanged.
|
|
assert.strictEqual(normalizeTestCommand('npm test', dir), 'npm test');
|
|
} finally {
|
|
cleanup(dir);
|
|
}
|
|
});
|
|
});
|