Files
msd-core/tests/mutation-workflow-base-ref.test.cjs
Tom Boucher 0408276791 chore(#2797): federate reviewer config keys off the central schema (#2841)
* chore(#2797): federate reviewer config keys off the central schema

Phase 4 of epic #2782 (ADR-2782 D9, config half). Runs AFTER 5a per the
ADR's swap amendment: a federated config slice lives inside a
capabilities/<id>/capability.json, and three of the five key families had
no capability directory until 5a created them.

Four key families move to the lanes that use them; the central-schema
removal and the federated addition land in this one commit because the
exclusivity invariant fails the build on a key present in both.
review.max_prompt_tokens, review.default_reviewers and
review.reviewer_instances describe policy ACROSS lanes and stay central.

Two things the issue did not name, both found while building it:

1. THE EXCLUSIVITY GATE WAS BLIND TO PATTERNS. It compared federated keys
   against manifest.validKeys only, and two of the four families
   (review.models.<slug>, review.max_prompt_tokens_per_reviewer.<slug>)
   were pattern-backed. That is not cosmetic: isCentralConfigKey consults
   those patterns and mergeFederatedConfig skips every key for which it
   returns true, so declaring a slice while the pattern survived would
   have shipped an INERT slice behind a green gate — the exact
   half-migrated shape the invariant exists to prevent. The gate now
   loads the patterns from the same manifest the runtime reads.

2. AN UNSET PER-LANE BUDGET NOW RESOLVES TO 0, NOT NOT-FOUND, because a
   federated key always resolves to its declared default. The three
   fallback guards in review.md checked only empty-or-"null", so a user
   who set the GLOBAL review.max_prompt_tokens would have silently lost
   trimming on the HTTP lanes. The guards now treat 0 as unset.

D9 says review.models.<slug> is owned by "the lane whose slug it names".
That is false for one lane: the shipped key is review.models.agy while
the slug is antigravity. Ownership follows the lane; the key name is
preserved, because renaming would break every config that sets it.

Existing tests updated rather than left asserting the old world:
config-get on a cleared federated key yields empty instead of
not-found (what #2046 actually protects — never persisting the literal
"null" — is unchanged and still asserted); the config-schema dynamic
pattern representative moves to reviewer_instances; the
prototype-pollution guard case moves to a surviving dynamic prefix so
alert #26 keeps its coverage, with a new case asserting the old key is
now rejected earlier; and Phase 2's harvest-widening inertness assertion
becomes an ownership assertion, since Phase 4 is what consumes it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#2797): use a -1 sentinel so an explicit per-lane budget of 0 survives

A federated config key always resolves to its declared default, so an
unset per-lane prompt budget needed a value the workflow could treat as
'not configured'. The first cut used 0 — which is wrong: 0 is already a
LEGITIMATE per-lane budget meaning 'do not trim this lane' (the
early-return guard in prepare_trimmed_prompt_for_reviewer). Treating it
as unset would have silently switched a user who deliberately disabled
trimming for one lane onto the global budget.

The sentinel is now -1, which is not a valid token budget, so all three
states stay distinguishable: unset falls back to global, an explicit 0
disables trimming for that lane, and an explicit N is used. Locked by
three CLI round-trip tests.

Surfaced by the isolated security reviewer before it crashed mid-run;
verified independently against the shipped trim guard rather than taken
on trust.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#2797): update central-registration assertions and stay under the review.md cap

The remote runner caught both; my local sweep missed the files.

1. tests/plan-review-convergence.test.cjs asserted the three local-server
   host keys are in VALID_CONFIG_KEYS. They are federated to their lane
   capabilities now, and the exclusivity invariant forbids a key living
   in both places. What #2306-local actually protects is that config-set
   ACCEPTS them, so that is what is asserted — via isValidConfigKey, the
   predicate config-set itself uses, which spans central and federated.
   A second assertion pins federated ownership, so a silent reversion
   back to the central schema fails too.

2. review.md exceeded the LARGE tier hard cap (62583 > 61440). That cap
   is a red line, not a budget to raise. The three per-lane budget guard
   comments were near-identical; condensed to one terse line each. 61371
   bytes, 69 to spare. Real extraction to workflows/review/modes/ is
   Phase 5b/6 work.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#2797): fail closed on a broken config-schema manifest; reconcile stale docs

Isolated security review findings.

MAJOR — loadCentralConfigPatterns failed OPEN. It swallowed a JSON parse
error and returned [], while its sibling loadCentralConfigKeys, reading
the SAME file, writes to stderr and throws ExitError(1) on that identical
failure class. Fail-open here defeats the gate this function exists to
feed: with zero patterns, validateCrossCapability's pattern-collision
check silently passes and an inert federated slice ships green. It was
masked in the one production call site only because loadCentralConfigKeys
runs first against the same path — a coincidence of ordering, not a
guarantee, and this function is exported and called standalone. The two
now share a contract: ENOENT is the legitimate absent case, anything else
throws loudly. A single unparseable PATTERN is still skipped, which
degrades to "checked less" rather than blocking every build. The branch
had zero coverage; it now has two tests (malformed JSON, EISDIR).

MINOR — docs/CONFIGURATION.md still listed review.models.qwen and
review.models.cursor as settable, ~770 lines below this PR's own new
Ownership section. Those lanes take no model flag, so they declare no
model key and config-set now rejects them. Rows removed; the missing
review.models.agy row added; the per-reviewer budget row corrected to
name only the lanes that own a budget key, and to document that a
per-lane 0 disables trimming for that lane.

Also fixes a shadowed "raw" binding introduced by the fail-closed change,
which made the generator unrequirable — caught immediately by its own
--check.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* chore(#2797): backfill changeset pr number to 2841

* fix(#2452): make the base-ref mutation test hermetic against leaked GIT_* env

tests/mutation-workflow-base-ref.test.cjs fails on PR branches while next
stays green, and it is currently blocking at least three unrelated PRs
(#2841, #2832, #2827) with:

  error: invalid object 100644 <sha> for 'base-N.txt'
  error: Error building trees

The existing loop comment attributes this to `git add .` rehashing O(n^2)
blobs "before the object write had landed" and works around it by staging
one path per iteration. That is not the cause: sequential execFileSync
calls cannot race each other's object writes, and the failure persisted
after that change — it simply moved to a lower commit index.

The cause is that the git() helper inherited the runner's environment. A
leaked GIT_INDEX_FILE makes `git add` write into a DIFFERENT repository's
index; GIT_OBJECT_DIRECTORY / GIT_ALTERNATE_OBJECT_DIRECTORIES send the
blob to another object store; GIT_DIR / GIT_WORK_TREE redirect the whole
operation. In every case `git commit` then cannot resolve a blob it just
staged, which is precisely the error above.

Verified by negative control: with GIT_DIR exported, this test fails on
the unfixed helper (the git commands operate on the wrong repository
entirely); with the helper stripping GIT_* it passes. The single-path
staging is kept — it is genuinely less work — but it is no longer load
bearing.

Found while shipping #2797. Fixed in place rather than deferred: it is a
defect surfaced during the work, and it is blocking other contributors.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#2452): build the base-advance commits empty, removing the lost-object class

The base-ref guard has been failing in CI with:

  error: invalid object 100644 <sha> for 'base-N.txt'
  error: Error building trees

It is currently red on at least three unrelated PRs (#2841, #2832,
#2827) while next stays green.

Two theories have now been tried and neither held. #1881 blamed `git
add .` rehashing O(n^2) blobs and switched to staging one path per
iteration; the failure moved from commit 32 to commit 25 and carried on.
The preceding commit here made the git helper hermetic against leaked
GIT_* environment — that IS a real vulnerability (with GIT_DIR exported
the helper operates on the wrong repository entirely, proven by negative
control) but it produces a different error than CI reports, so it is not
demonstrably the cause either.

Neither trigger reproduces off-CI, so this stops guessing at the trigger
and removes the failure CLASS instead. The loop needs base-branch DEPTH
and nothing else: no assertion reads these commits' contents, and
base-side files cannot appear in `origin/base...HEAD` regardless.
`--allow-empty` writes no blob and no tree, so there is no object for the
index to reference and lose. It is also far less work than 60
write+hash+index cycles.

The guard still proves its mechanism: the test asserts that a --depth=1
base fetch FAILS and a full fetch resolves, so a broken topology would
surface immediately rather than passing vacuously.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Test <test@example.com>
2026-07-30 08:52:56 -04:00

315 lines
14 KiB
JavaScript

'use strict';
/**
* tests/mutation-workflow-base-ref.test.cjs
*
* Regression tests for the mutation-gate base-ref fetch (issue #2452).
*
* Background: `.github/workflows/mutation.yml` checks out with `fetch-depth: 0`
* (full history) and then re-fetched the base branch with `--depth=1`. That
* shallow re-fetch truncates the base ref's ancestry, so the three-dot diff in
* scripts/mutation-matrix.cjs (`git diff --name-only origin/<base>...HEAD`)
* can no longer compute a merge base and aborts with
* `fatal: origin/next...HEAD: no merge base`, exit 2. The `detect` job then
* fails and the `mutate` shards never run — the 80% mutation-score threshold
* goes UNVERIFIED rather than enforced.
*
* The failure is branch-position dependent, which is why it went unnoticed: a
* branch already level with the base incidentally passes (its merge base IS
* the single fetched commit), while a branch that is BEHIND the base fails.
*
* Test 1 is the contract guard (RED on origin/next, GREEN after the fix).
* Test 2 is a real-git mechanism proof: it reconstructs the runner's ref
* topology in a temp repo and demonstrates that the shallow fetch breaks the
* three-dot diff while a full fetch resolves it — proving the fix is both
* necessary and sufficient rather than asserting on YAML text alone.
*/
const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const os = require('os');
const path = require('path');
const { execFileSync } = require('child_process');
const helpers = require('./helpers.cjs');
const WORKFLOWS_DIR = path.resolve(__dirname, '..', '.github', 'workflows');
/**
* Every workflow whose lint step diffs against the base with the three-dot
* form (`origin/<base>...HEAD`). All of them need the BASE REF's ancestry, so
* none may shallow-fetch it. mutation.yml used --depth=1 and failed outright;
* the other two used --depth=50, shrinking the window further still.
*
* This guard covers the base-ref FETCH only. The shallow *checkout* depth on
* changeset-required.yml / docs-required.yml is a separate, deliberate cost
* control with fail-closed semantics, owned by
* tests/policy-lint-shallow-checkout.test.cjs — do not conflate the two.
*/
const THREE_DOT_WORKFLOWS = [
{ file: 'mutation.yml', consumer: 'scripts/mutation-matrix.cjs' },
{ file: 'changeset-required.yml', consumer: 'scripts/changeset/lint.cjs' },
{ file: 'docs-required.yml', consumer: 'scripts/lint-docs-required.cjs' },
];
// Bounded: git subprocesses in tests must never hang a CI lane.
const GIT_TIMEOUT_MS = 30_000;
/**
* Environment for every git call below, with EVERY `GIT_*` variable stripped.
*
* Without this the helper inherits the runner's environment. A leaked
* `GIT_INDEX_FILE` makes `git add` in this temp repo write into a DIFFERENT
* repository's index; a leaked `GIT_OBJECT_DIRECTORY` /
* `GIT_ALTERNATE_OBJECT_DIRECTORIES` sends the blob to a different object store;
* a leaked `GIT_DIR` / `GIT_WORK_TREE` redirects the operation wholesale. In
* each case `git commit` then cannot resolve a blob it just staged, and git
* reports exactly:
*
* error: invalid object 100644 <sha> for 'base-N.txt'
* error: Error building trees
*
* That failure was previously attributed to `git add .` rehashing O(n²) blobs
* "before the object write had landed" (see the loop comment below) and worked
* around by staging one path per iteration. That reduced the churn but not the
* cause: sequential execFileSync calls cannot race each other's object writes,
* and the failure persisted — reappearing at a lower commit index on PR branches
* while `next` stayed green. Making the environment hermetic addresses the
* mechanism rather than the symptom; the single-path staging below is kept
* because it is genuinely less work.
*/
function gitEnv() {
const env = { ...process.env };
for (const key of Object.keys(env)) {
if (key.startsWith('GIT_')) delete env[key];
}
return env;
}
function git(cwd, args) {
return execFileSync('git', args, {
cwd,
encoding: 'utf8',
timeout: GIT_TIMEOUT_MS,
stdio: ['ignore', 'pipe', 'pipe'],
env: gitEnv(),
});
}
/**
* Extract the `run:` body of the named step from the workflow YAML.
* Deliberately a small hand parser rather than a YAML dep: this asserts on the
* literal command line the runner executes, which is the contract at issue.
*
* Handles both inline (`run: git fetch ...`) and block-scalar (`run: |`) forms;
* a block scalar returns its dedented body so a future refactor to multi-line
* `run:` cannot silently degrade the guard into asserting on the literal "|".
* Comment lines are skipped so a commented-out step of the same name cannot
* shadow the real one.
*/
function runBodyForStep(yaml, stepName) {
const lines = yaml.split(/\r?\n/);
const nameIdx = lines.findIndex(
(l) => !/^\s*#/.test(l) && l.includes(`- name: ${stepName}`),
);
if (nameIdx === -1) return null;
for (let i = nameIdx + 1; i < lines.length; i++) {
const line = lines[i];
// Next step begins -> the step had no run: body.
if (/^\s*- name:/.test(line) && !/^\s*#/.test(line)) return null;
const m = line.match(/^\s*run:\s*(.*)$/);
if (!m) continue;
const inline = m[1].trim();
if (!/^[|>][-+]?$/.test(inline)) return inline;
// Block scalar: collect the indented body until the indentation drops.
const runIndent = line.match(/^(\s*)/)[1].length;
const body = [];
for (let j = i + 1; j < lines.length; j++) {
const bodyLine = lines[j];
if (bodyLine.trim() === '') {
body.push('');
continue;
}
const indent = bodyLine.match(/^(\s*)/)[1].length;
if (indent <= runIndent) break;
body.push(bodyLine.trim());
}
return body.join('\n').trim();
}
return null;
}
describe('#2452 CI gates: base-ref fetch must preserve ancestry', () => {
for (const { file, consumer } of THREE_DOT_WORKFLOWS) {
test(`${file}: "Fetch base ref for diff" does not shallow-fetch the base`, () => {
const yaml = fs.readFileSync(path.join(WORKFLOWS_DIR, file), 'utf8');
const runBody = runBodyForStep(yaml, 'Fetch base ref for diff');
assert.ok(
runBody,
`Expected a "Fetch base ref for diff" step with a run: body in ${file}. ` +
'If the step was renamed, update this test to match — do not delete the guard.',
);
assert.ok(
runBody.startsWith('git fetch origin'),
`Expected the step to fetch the base ref, got: ${runBody}`,
);
assert.ok(
!/--depth[=\s]/.test(runBody),
`${file}: the base-ref fetch must NOT be shallow. A --depth fetch truncates ` +
"the base branch's ancestry, so the three-dot diff in " +
`${consumer} cannot compute a merge base and the job dies with ` +
'`fatal: ...: no merge base` (#2452). Offending command: ' +
runBody,
);
});
}
// How far the base branch advances past the branch point. Chosen so the
// merge base sits OUTSIDE the old --depth=50 window, making the boundary
// between "cushion masks the bug" and "cushion exhausted" directly testable.
const BASE_ADVANCE = 60;
// Base chain is: tip … BASE_ADVANCE commits … branch point. So the branch
// point is the (BASE_ADVANCE + 1)-th commit from the tip — the exact depth
// at which a shallow base fetch first contains a usable merge base.
const MERGE_BASE_DEPTH = BASE_ADVANCE + 1;
test('base-ref fetch depth determines whether the three-dot diff resolves', () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2452-'));
try {
// ---- origin: a base branch that advances past a feature branch --------
const origin = path.join(tmp, 'origin');
fs.mkdirSync(origin);
git(origin, ['init', '--quiet', '--initial-branch=base']);
git(origin, ['config', 'user.email', 'test@example.com']);
git(origin, ['config', 'user.name', 'Test']);
// Ambient commit.gpgsign=true would otherwise break these commits in CI.
git(origin, ['config', 'commit.gpgsign', 'false']);
fs.writeFileSync(path.join(origin, 'seed.txt'), 'seed\n');
git(origin, ['add', '.']);
git(origin, ['commit', '--quiet', '-m', 'seed']);
// Feature branch diverges here — this commit is the merge base.
git(origin, ['checkout', '--quiet', '-b', 'feature']);
fs.writeFileSync(path.join(origin, 'covered.cts'), 'export const x = 1;\n');
git(origin, ['add', '.']);
git(origin, ['commit', '--quiet', '-m', 'feature change']);
// Base then advances, leaving `feature` BEHIND — the failing condition.
git(origin, ['checkout', '--quiet', 'base']);
for (let n = 1; n <= BASE_ADVANCE; n++) {
// EMPTY commits: this loop needs base-branch DEPTH, nothing else. No
// assertion reads these commits' contents — the three-dot diff below is
// asserted on `covered.cts`, which lives on the HEAD side, and base-side
// files cannot appear in `origin/base...HEAD` at all.
//
// They used to write and stage a `base-N.txt` per iteration, and that is
// what kept failing in CI:
//
// error: invalid object 100644 <sha> for 'base-N.txt'
// error: Error building trees
//
// The first attempt blamed `git add .` rehashing O(n²) blobs and switched
// to staging one path per iteration (#1881). It did not work — the failure
// simply moved from commit 32 to commit 25, and went on blocking unrelated
// PRs. The trigger for the lost object write was never reproduced off-CI.
//
// So rather than keep guessing at the trigger, this removes the failure
// CLASS: `--allow-empty` writes no blob and no tree for these commits, so
// there is no object for the index to reference and lose. It is also
// dramatically less work than 60 write+hash+index cycles.
git(origin, ['commit', '--quiet', '--allow-empty', '-m', `base advance ${n}`]);
}
// ---- runner: has the PR head, must fetch the base ref separately ------
// Each variant gets its OWN clone, modelling independent workflow runs.
// They must not share a repo: once a shallow fetch writes a .git/shallow
// boundary, a later plain `git fetch` does NOT un-shallow it (that needs
// --unshallow), so repairing in place would test a scenario the workflow
// never encounters.
function runnerDiff(name, baseFetchArgs) {
const dir = path.join(tmp, name);
fs.mkdirSync(dir);
git(dir, ['init', '--quiet']);
git(dir, ['config', 'user.email', 'test@example.com']);
git(dir, ['config', 'user.name', 'Test']);
git(dir, ['config', 'commit.gpgsign', 'false']);
git(dir, ['remote', 'add', 'origin', origin]);
git(dir, ['fetch', '--quiet', 'origin', 'feature']);
git(dir, ['checkout', '--quiet', 'FETCH_HEAD']);
// The FETCH is inside the try, not before it. A base fetch whose shallow
// boundary lands short of the merge base can fail during the FETCH itself
// ("unable to parse commit" — the boundary commit's parent is unavailable)
// rather than succeeding and leaving the DIFF to fail with "no merge base".
// Which of the two git picks is version/transport dependent: this test
// passed on ubuntu-22 and windows-24 and failed on ubuntu-24 for the same
// commit. Both outcomes mean the same thing for what this guard protects —
// a shallow base ref cannot resolve the three-dot diff — so both are
// recorded as ok:false instead of one of them escaping as a crash.
try {
git(dir, ['fetch', '--quiet', 'origin', 'base', ...baseFetchArgs]);
return { ok: true, out: git(dir, ['diff', '--name-only', 'origin/base...HEAD']).trim() };
} catch (err) {
return { ok: false, err: String(err.stderr || err.message) };
}
}
// (a) --depth=1 — mutation.yml's pre-fix command. Always broken.
const depth1 = runnerDiff('runner-depth-1', ['--depth=1']);
assert.equal(
depth1.ok,
false,
'Expected the three-dot diff to FAIL after a --depth=1 base fetch. ' +
'If this stops holding, the #2452 mechanism no longer reproduces and ' +
'this guard needs revisiting.',
);
// Asserting git's exact wording couples this guard to a git version:
// "no merge base" (diff-time) and "unable to parse commit" (fetch-time)
// are the same condition reported at different stages. CONTRIBUTING also
// prohibits raw text matching on subprocess output — the typed outcome
// above (`ok === false`) IS the contract this test exists to pin.
assert.ok(depth1.err.length > 0, 'a failed shallow diff must report a cause');
// (b) BOUNDARY, just below: the merge base is one commit out of reach.
// This is the changeset-required.yml / docs-required.yml --depth=50 case
// generalized — the cushion only ever postponed the same failure.
const below = runnerDiff('runner-depth-below', [`--depth=${MERGE_BASE_DEPTH - 1}`]);
assert.equal(
below.ok,
false,
`Expected FAIL at --depth=${MERGE_BASE_DEPTH - 1} (merge base one commit ` +
'beyond the shallow boundary) — this is why a bounded cushion is not a fix',
);
assert.ok(below.err.length > 0, 'a failed shallow diff must report a cause');
// (c) BOUNDARY, exactly deep enough: the merge base is the last commit in.
const atDepth = runnerDiff('runner-depth-at', [`--depth=${MERGE_BASE_DEPTH}`]);
assert.equal(
atDepth.ok,
true,
`Expected SUCCESS at --depth=${MERGE_BASE_DEPTH} (merge base exactly at the ` +
`shallow boundary), got: ${atDepth.err}`,
);
assert.equal(atDepth.out, 'covered.cts');
// (d) Unbounded — the shipped fix. Correct regardless of how far behind.
const full = runnerDiff('runner-full', []);
assert.equal(full.ok, true, `Expected the full-fetch diff to succeed, got: ${full.err}`);
assert.equal(
full.out,
'covered.cts',
'After a full base fetch the three-dot diff must resolve and report ' +
"exactly the feature branch's changed files",
);
} finally {
helpers.cleanup(tmp);
}
});
});