golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 2 Packages Projects Releases Wiki Activity

refactor(#3180): ADR-3180 behavior contract + cross-surface drift guardrails (#3223)

Browse Source 
* 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>
This commit is contained in:
Tom Boucher
2026-08-08 16:05:17 -04:00
committed by GitHub
parent 636ec92107
commit b9f51836e6
22 changed files with 2023 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

131
tests/ci-test-scope.test.cjs
View File

@@ -891,10 +891,16 @@ const path = require('path');
const { createTempDir, cleanup } = require('./helpers.cjs');
const { runNode } = require('./helpers/process-seam.cjs');
const { toLegacyResult } = require('./helpers/git-fixture.cjs');
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const HARNESS = path.join(__dirname, '..', 'scripts', 'run-tests.cjs');
// Exported so the suite-token resolution contract below can be asserted
// in-process rather than through a timed subprocess spawn (see evidence
// comment above describe('bug #641 ...')).
const {
walkTestFiles,
selectExplicitFiles,
selectFiles,
parseArgs,
} = require('../scripts/run-tests.cjs');
const PASS_BODY = `'use strict';
const { test } = require('node:test');
@@ -907,17 +913,42 @@ function seed(dir, names) {
}
}
function runHarness(testDir, args = [], extraEnv = {}) {
const env = { ...process.env, GSD_TEST_DIR: testDir, ...extraEnv };
delete env.NODE_TEST_CONTEXT;
const result = runNode([HARNESS, ...args], {
cwd: path.join(__dirname, '..'),
env,
timeoutMs: PROBE_TIMEOUT_MS,
});
return toLegacyResult(result);
// Mirrors scripts/run-tests.cjs main()'s selection path (parseArgs ->
// walkTestFiles -> selectExplicitFiles/selectFiles) exactly, called
// in-process instead of through a spawned child that then spawns a nested
// `node --test`. See the evidence comment above describe('bug #641 ...').
function selectInProcess(testDir, args) {
const parsed = parseArgs(args);
if (parsed.error) return parsed;
const allFiles = walkTestFiles(testDir, '').sort();
const usingExplicitFiles = parsed.files !== null || parsed.filesFrom !== null;
if (usingExplicitFiles) {
return selectExplicitFiles(allFiles, parsed.files, parsed.filesFrom);
}
return { files: selectFiles(allFiles, parsed.suite) };
}
// Redesign evidence (2026-08-08): these probes originally spawned
// scripts/run-tests.cjs as a real child — which itself spawns a NESTED
// `node --test` — under a fixed PROBE_TIMEOUT_MS=15000 wall-clock budget,
// concurrently with the ~31k-test full suite running in the same container.
// On the remote runner this produced intermittent failures shaped
// `null !== 0` (r.status === null: the child was KILLED at the timeout),
// never a failed assertion about suite-token resolution. Reproduced on
// `next` alone (sha e705652ba): 5 failures on linux-node22, 0 failures on
// linux-node24. The victim subset varied by run and by lane, and the
// failure count went DOWN (7 -> 4 unique failures) as an unrelated diff got
// heavier — a resource collision, not a flake. The subject under test is
// suite-TOKEN RESOLUTION (parseArgs / selectExplicitFiles / selectFiles),
// not test execution; running the seeded trivial fixture files was
// incidental and was the entire timeout surface. These probes now call the
// exported selection functions in-process — removing the wall-clock budget
// around a nested spawn, not raising its number. Real end-to-end coverage of
// run-tests.cjs spawning and running to completion (exit 0 from a real
// harness run) already exists in tests/run-tests-harness.test.cjs (e.g. 'no
// flag runs ALL test files', '--suite unit excludes marked suites',
// 'non-zero from node:test propagates through harness'), so no execution
// coverage is lost by converting the "(tests run successfully)" probe below.
describe('bug #641 — --files-from with bare suite token', () => {
let tmpDir;
@@ -935,57 +966,45 @@ describe('bug #641 — --files-from with bare suite token', () => {
const listPath = path.join(tmpDir, 'ci-selected-tests.txt');
fs.writeFileSync(listPath, 'unit\n', 'utf8');
const r = runHarness(tmpDir, ['--files-from', listPath]);
const r = selectInProcess(tmpDir, ['--files-from', listPath]);
// Must NOT exit 2 with the "not found" error.
assert.notStrictEqual(
r.status,
2,
`Expected exit 0 or 1, got 2.\nstderr: ${r.stderr}\nstdout: ${r.stdout}`,
);
assert.doesNotMatch(
r.stderr,
/requested test file\(s\) not found: unit/,
`Must not emit "not found: unit".\nstderr: ${r.stderr}`,
);
// The unit suite file (a.test.cjs) must appear in the run.
// Must NOT be the "not found" error shape (the old exit-2 crash).
assert.strictEqual(r.error, undefined, `Expected a resolved file list, got error: ${r.error}`);
// The unit suite file (a.test.cjs) must appear in the resolution.
assert.ok(
r.stderr.includes('a.test.cjs'),
`Expected a.test.cjs (unit suite) to be selected.\nstderr: ${r.stderr}`,
r.files.includes('a.test.cjs'),
`Expected a.test.cjs (unit suite) to be selected. Resolved: ${r.files}`,
);
// The security suite file must NOT be included (unit token = unit only).
assert.ok(
!r.stderr.includes('b.security.test.cjs'),
`Expected b.security.test.cjs (security suite) to be excluded.\nstderr: ${r.stderr}`,
!r.files.includes('b.security.test.cjs'),
`Expected b.security.test.cjs (security suite) to be excluded. Resolved: ${r.files}`,
);
});
test('--files-from with bare "unit" token exits 0 (tests run successfully)', () => {
// "Exits 0" is determined entirely by selection succeeding with a
// non-empty list — the seeded fixture is a trivial no-op, so executing
// it contributes nothing this in-process call doesn't already prove.
// End-to-end execution coverage of run-tests.cjs lives in
// tests/run-tests-harness.test.cjs (see the evidence comment above).
seed(tmpDir, ['a.test.cjs']);
const listPath = path.join(tmpDir, 'ci-selected-tests.txt');
fs.writeFileSync(listPath, 'unit\n', 'utf8');
const r = runHarness(tmpDir, ['--files-from', listPath]);
const r = selectInProcess(tmpDir, ['--files-from', listPath]);
assert.strictEqual(
r.status,
0,
`Expected exit 0.\nstderr: ${r.stderr}\nstdout: ${r.stdout}`,
);
assert.strictEqual(r.error, undefined, `Expected a resolved file list, got error: ${r.error}`);
assert.deepStrictEqual(r.files, ['a.test.cjs']);
});
test('--files with bare "unit" token also resolves correctly', () => {
seed(tmpDir, ['a.test.cjs', 'b.security.test.cjs']);
const r = runHarness(tmpDir, ['--files', 'unit']);
const r = selectInProcess(tmpDir, ['--files', 'unit']);
assert.notStrictEqual(
r.status,
2,
`Expected exit 0, got 2.\nstderr: ${r.stderr}`,
);
assert.doesNotMatch(r.stderr, /requested test file\(s\) not found: unit/);
assert.ok(r.stderr.includes('a.test.cjs'), `a.test.cjs must be selected.\nstderr: ${r.stderr}`);
assert.ok(!r.stderr.includes('b.security.test.cjs'), `security file must not be selected.\nstderr: ${r.stderr}`);
assert.strictEqual(r.error, undefined, `Expected exit 0, got error: ${r.error}`);
assert.ok(r.files.includes('a.test.cjs'), `a.test.cjs must be selected. Resolved: ${r.files}`);
assert.ok(!r.files.includes('b.security.test.cjs'), `security file must not be selected. Resolved: ${r.files}`);
});
test('mixed: suite token "unit" alongside an explicit file resolves both', () => {
@@ -994,13 +1013,14 @@ describe('bug #641 — --files-from with bare suite token', () => {
// 'unit' expands to [a.test.cjs, b.test.cjs]; b.test.cjs is explicit too.
fs.writeFileSync(listPath, 'unit\nb.test.cjs\n', 'utf8');
const r = runHarness(tmpDir, ['--files-from', listPath]);
const r = selectInProcess(tmpDir, ['--files-from', listPath]);
assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`);
// Both unit files present; security not.
assert.ok(r.stderr.includes('a.test.cjs'), `a.test.cjs must be selected.\nstderr: ${r.stderr}`);
assert.ok(r.stderr.includes('b.test.cjs'), `b.test.cjs must be selected.\nstderr: ${r.stderr}`);
assert.ok(!r.stderr.includes('c.security.test.cjs'), `c.security.test.cjs must be excluded.\nstderr: ${r.stderr}`);
assert.strictEqual(r.error, undefined, `Expected a resolved file list, got error: ${r.error}`);
// Both unit files present exactly once (the union dedupes them); security not.
assert.ok(r.files.includes('a.test.cjs'), `a.test.cjs must be selected. Resolved: ${r.files}`);
assert.ok(r.files.includes('b.test.cjs'), `b.test.cjs must be selected. Resolved: ${r.files}`);
assert.ok(!r.files.includes('c.security.test.cjs'), `c.security.test.cjs must be excluded. Resolved: ${r.files}`);
assert.strictEqual(r.files.length, 2, `expected no duplicate b.test.cjs. Resolved: ${r.files}`);
});
test('#408 fallback: ci-test-scope "unit" sentinel does not crash run-tests', () => {
@@ -1013,15 +1033,10 @@ describe('bug #641 — --files-from with bare suite token', () => {
const listPath = path.join(tmpDir, '.ci-selected-tests.txt');
fs.writeFileSync(listPath, 'unit\n', 'utf8');
const r = runHarness(tmpDir, ['--files-from', listPath]);
const r = selectInProcess(tmpDir, ['--files-from', listPath]);
assert.strictEqual(
r.status,
0,
`#408 fallback: expected exit 0 but got ${r.status}.\nstderr: ${r.stderr}`,
);
assert.doesNotMatch(r.stderr, /not found: unit/);
assert.ok(r.stderr.includes('a.test.cjs'), `unit test must run.\nstderr: ${r.stderr}`);
assert.strictEqual(r.error, undefined, `#408 fallback: expected a resolved file list, got error: ${r.error}`);
assert.ok(r.files.includes('a.test.cjs'), `unit test must resolve. Resolved: ${r.files}`);
});
});