Files
msd-core/tests/code-review-pipeline-regression.test.cjs
Jakub Zych a9a7a328e6 refactor: hard-fork GSD -> MSD (Make Software Done)
Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD
across contents and paths, upstream package/repo coordinates -> @golem15/msd-core
and golem15com/msd-core. Deep links into upstream history, sibling upstream
packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is.

Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line,
package/plugin identity, regenerated lockfile, install-tree fixtures, derived
registries and benchmark baseline; migration checksum baseline re-locked
(MSD keeps its own install state, so no install had applied the old sums);
sort-order and regex-escaped expectations in tests adjusted.
2026-10-06 01:47:40 +02:00

4997 lines
284 KiB
JavaScript

// docs-guard-exempt: 'docs/DEVELOPMENT.md' is a synthetic files-list fixture entry, not read as content.
// allow-test-rule: source-text-is-the-product
// The workflow and agent .md files ARE the product: their text is loaded and
// executed/interpreted at runtime by the agent host. Testing that specific
// strings exist within these files tests the deployed contract, not an
// implementation detail. No runtime API exists to enumerate the label accept-
// list or filter-set definitions — the text IS the specification.
//
// Bug 1 (compute_file_scope) — The inline Node.js script embedded in the
// workflow .md is the parser. The test implements the identical parse logic as
// a pure JS function (mirroring lines 172-184 of code-review.md exactly) and
// asserts on its structured output. A separate docs-parity assertion checks
// that the workflow .md contains the hyphen-aware boundary regex and the
// em-dash/parenthetical stripping — both of which are the deployed contract.
//
// Bug 2 (present_results) — Tested both behaviourally (pure JS helper that
// mimics the grep|cut pipeline) and via docs-parity on the workflow .md text.
//
// Bugs 3 and reviewer contract — docs-parity only on agents/*.md: the filter-
// set definition and label-equivalence contract exist only as text in those
// files; there is no runtime enumeration API.
'use strict';
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const fc = require('fast-check');
const { runHook, runNode, OUTCOME } = require('./helpers/process-seam.cjs');
const { toLegacyResult, gitOrThrow } = require('./helpers/git-fixture.cjs');
const { PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, HOOK_FANOUT_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const { createTempDir, createTempGitProject, cleanup, readFileNormalized } = require('./helpers.cjs');
const {
foldShellContinuations,
findShellFencedMatches,
} = require('./helpers/shell-doc-scan.cjs');
const os = require('node:os');
/**
* A single invocation of the external `fallow` binary's `audit`
* subcommand against the REAL current repo tree (CI-only) -- a heavier
* third-party tool operation distinct from any existing shared constant.
*/
const FALLOW_AUDIT_TIMEOUT_MS = 120000;
const ROOT = path.resolve(__dirname, '..');
// HOISTED. `const` is in the temporal dead zone until its declaration executes, and a
// `{ skip: !HAS_BASH }` option object is evaluated EAGERLY when its describe body runs --
// so a bash-gated test added ABOVE the old mid-file declaration threw a ReferenceError
// that aborted the whole describe and CANCELLED its siblings. It caught three separate
// additions in this round alone, and the cancellation reads as a passing run in the
// summary line. Declared with the other file-level constants so placement stops mattering.
// AND NOT A PROBE — but the honest reason is narrower than the first draft of this comment claimed,
// and the correction is worth keeping. Round 5 flagged that every test exercising the shipped bash
// fences is `{ skip: !HAS_BASH }`, so block 1's severity-reporting path has no Windows-lane coverage.
// The gap is real and the count is 37 — 22 was the number of `{ skip: !HAS_BASH }` CALL SITES, and a
// skip on a `describe` cancels its subtests.
//
// This comment first justified the skip by citing `local/no-unguarded-nonportable-exec` as REQUIRING
// this exact guard. That was checked and is wrong on both halves: the rule only fires on a file that
// also chmods an exec bit with an octal literal (this file has none, so it never fires here), and
// `eslint-rules/lib/platform-guard.cjs` accepts four guard shapes plus `os.platform()`, not one. A
// constraint that exists is not a constraint that applies.
//
// What holds — and this is now MEASURED, not assumed. The measurement the previous version of this
// comment deferred has been made, on native Windows (not WSL) with Git Bash 5.2.37 / MINGW64 first on
// PATH, node v25.2.1 — the same shell family the repo's `windows-latest` lane runs:
//
// HAS_BASH left alone: 179 tests, 127 pass, 0 fail, 52 skipped
// HAS_BASH forced true: 179 tests, 140 pass, 24 fail, 15 skipped
//
// So 37 of the skips are this guard's (52 - 15; the other 15 skip for unrelated reasons), and
// unskipping them does NOT reveal a clean win: 13 pass and 24 fail. The failures cluster on exactly
// the divergence the eslint rule's subject line names — `bash -c` quoting (one surfaces as
// `unexpected EOF while looking for matching '"'`), empty captured output, and two outright
// `spawn_failed`. Flipping this constant to a runtime probe today would red the Windows lane with 24
// failures, so the guard STAYS; what changes is that it now documents a measured gap instead of an
// assumed one. Closing it means porting the fences themselves, which is a change of its own, not a
// line in a review round.
//
// NOTE for anyone re-running this: on a WSL host `bash` on the Windows PATH resolves to
// C:\Windows\system32\bash.exe, the WSL bridge — measuring through that runs real Linux bash and
// reports a false clean. Put `C:\Program Files\Git\bin` first.
const HAS_BASH = process.platform !== 'win32';
const WORKFLOW_PATH = path.join(ROOT, 'msd-core', 'workflows', 'code-review.md');
const PRE_PASS_STEP_PATH = path.join(ROOT, 'msd-core', 'workflows', 'code-review', 'steps', 'structural-pre-pass.md');
const FIXER_PATH = path.join(ROOT, 'agents', 'msd-code-fixer.md');
const REVIEWER_PATH = path.join(ROOT, 'agents', 'msd-code-reviewer.md');
const EXECUTE_PHASE_PATH = path.join(ROOT, 'msd-core', 'workflows', 'execute-phase.md');
const DISPOSITION_STEP_PATH = path.join(ROOT, 'msd-core', 'workflows', 'execute-phase', 'steps', 'code-review-disposition.md');
// ---------------------------------------------------------------------------
// #4259: the T6 docs-parity site scan, hoisted out of the assertion so it can
// be driven directly by the negative controls below.
//
// The scan is `$`-anchored with `[^\n]*` on both sides of `--grep=`, so it only
// ever matched when `git log` and `--grep=` sat on the SAME physical line. A
// shell line-continuation made a semantically identical derivation invisible:
// zero hits, and T6 passed. Both generations of the assertion were defeated by
// it — the current anti-revert ban let a wrapped site through outright, and at
// v1.12.0 a wrapped site was silently exempted from the pattern-conformance
// checks written to catch the macOS `\b`-no-op class, so it could have carried
// exactly the malformed pattern T6 exists to reject. That is not a hypothetical
// shape: wrapping is the natural way to write a `git log` carrying a long ERE,
// and a real candidate implementation for #3926 did it, passed T6, and was
// caught only by later manual review.
//
// Folding continuations BEFORE matching is the repair, rather than widening the
// regex in place: the assertion's message and its `PHASE_SCOPE_NUM` filter both
// assume one site is one string, and a `[\s\S]*?` would happily run the scan
// across unrelated statements. The fold corrects the input, so everything built
// on the scan is fixed at once.
//
const GREP_SITE_RE = /^\s*[A-Z_]+=\$\(git log[^\n]*--grep=[^\n]*$/gm;
const findGrepSites = (src) => findShellFencedMatches(src, GREP_SITE_RE);
// ---------------------------------------------------------------------------
// Pure-function implementation of the compute_file_scope Node script body.
// This mirrors the logic in code-review.md lines 172-184 exactly.
// If those lines change, this function must be updated in tandem (and the
// docs-parity assertions below will catch a mismatch at the regex level).
//
// #2666: the acceptance predicate accepts root-level paths (no `/`) and known
// extensionless build files (Dockerfile/Makefile/etc.), not only nested paths
// with a trailing extension. Prose bullets are rejected by the known-filename /
// has-extension distinction (plus the post-processing existence check backstop
// in the shipped workflow).
const KNOWN_EXTENSIONLESS_BUILD_FILES = new Set([
'dockerfile', 'containerfile', 'makefile', 'justfile', 'procfile',
]);
function isAcceptablePath(raw) {
// A trailing `.`+alphanumerics extension qualifies (root-level OR nested):
// package.json, renovate.json, .gitlab-ci.yml, AGENTS.md, app/foo.tsx
if (/\.[A-Za-z0-9]+$/.test(raw)) return true;
// Known extensionless build filename (basename, case-insensitive): Dockerfile, Makefile, …
const base = raw.split('/').pop();
if (KNOWN_EXTENSIONLESS_BUILD_FILES.has(base.toLowerCase())) return true;
return false;
}
function parseKeyFiles(yaml) {
const files = [];
let inSection = null;
for (const line of yaml.split('\n')) {
if (/^\s+created:/.test(line)) { inSection = 'created'; continue; }
if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; }
// Hyphen-aware boundary: reset inSection for ANY key: line (including key-decisions:, etc.)
if (/^\s*[\w-]+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; }
if (inSection && /^\s+-\s+(.+)/.test(line)) {
let raw = line.match(/^\s+-\s+(.+)/)[1].trim();
raw = raw.replace(/^['"]|['"]$/g, '');
// Order matters: parens BEFORE em-dash because em-dashes can appear inside parens
raw = raw.replace(/\s+\([^)]*\)\s*$/, '');
raw = raw.split(/\s+—\s/)[0].trim();
if (isAcceptablePath(raw)) {
files.push(raw);
}
}
}
return files;
}
// ---------------------------------------------------------------------------
// Pure-function implementation of the present_results severity-label parser.
// Mirrors the grep -E "^\s*(critical|blocker):" | head -1 | cut -d: -f2 | xargs
// pipeline from code-review.md.
// ---------------------------------------------------------------------------
function parseFrontmatterCritical(frontmatter) {
const lines = frontmatter.split('\n');
const match = lines.find((l) => /^\s*(critical|blocker):/.test(l));
if (!match) return { critical: 0 };
const value = match.split(':').slice(1).join(':').trim();
return { critical: parseInt(value, 10) || 0 };
}
// ---------------------------------------------------------------------------
// BUG 1 — SUMMARY parser: compute_file_scope must not bleed prose from
// hyphenated sections (key-decisions:, patterns-established:, etc.) into the
// file list, and must strip em-dash descriptions and parentheticals.
// ---------------------------------------------------------------------------
describe('Bug 1 — compute_file_scope SUMMARY parser', () => {
test('#4461: the shipped compute_file_scope bash fence parses verbatim', () => {
const src = readFileNormalized(WORKFLOW_PATH);
const stepStart = src.indexOf('<step name="compute_file_scope">');
assert.notStrictEqual(stepStart, -1, 'compute_file_scope step must exist');
const marker = src.indexOf('EXTRACTED=$(', stepStart);
assert.notStrictEqual(marker, -1, 'Tier-2 SUMMARY extractor must exist');
const fenceStart = src.lastIndexOf('```bash\n', marker);
const fenceEnd = src.indexOf('\n```', marker);
assert.ok(fenceStart !== -1 && fenceEnd !== -1, 'Tier-2 SUMMARY bash fence must be complete');
const script = src.slice(fenceStart + '```bash\n'.length, fenceEnd);
const result = runHook('-n', ['-c', script], {
interpreter: 'bash',
timeoutMs: PROBE_TIMEOUT_MS,
});
assert.equal(result.exitCode, 0, `bash rejected the shipped fence:\n${result.stderr}`);
});
test('#4461: the shipped extractor helper treats adversarial SUMMARY text and argv as inert data', () => {
const src = readFileNormalized(WORKFLOW_PATH);
const helperStart = src.indexOf(' extract_summary_files() {');
// Anchor on the real loop gate, not the whitespace-only separator above it:
// editors are entitled to trim trailing spaces without changing behavior.
const helperEnd = src.indexOf('\n if [ -n "$SUMMARIES" ]; then', helperStart);
assert.ok(helperStart !== -1 && helperEnd !== -1, 'extract_summary_files helper must be extractable');
const helper = src.slice(helperStart, helperEnd);
const dir = createTempDir('msd-4461-adversarial-');
try {
const sentinel = path.join(dir, 'MUST-NOT-EXIST');
// This directly executes the shipped helper's argv boundary. It does
// not claim the workflow's outer SUMMARY-list iteration preserves
// whitespace; that pre-existing shell-word-splitting behavior remains
// tracked separately by #4109.
// Double quotes are not legal in Windows filenames. Keep the argv
// adversarial with shell syntax, whitespace, and a quote that is valid
// on every supported filesystem; the SUMMARY payload below exercises
// a literal double quote independently.
const summary = path.join(dir, "SUMMARY $(not-a-command) 'quoted'.md");
// Git Bash launches the native Windows Node binary in CI. Forward-slash
// absolute paths survive that argv boundary on both platforms, while a
// raw drive path's backslashes are MSYS quoting syntax rather than data.
const shellSentinel = sentinel.replace(/\\/g, '/');
const shellSummary = summary.replace(/\\/g, '/');
const dollarPath = `src/$(touch ${shellSentinel}).js`;
const backtickPath = `src/\`touch ${shellSentinel}\`.js`;
const quotePath = 'src/"quoted".js';
fs.writeFileSync(summary, [
'---',
'key-files:',
' created:',
` - ${dollarPath}`,
' modified:',
` - ${backtickPath}`,
` - ${quotePath}`,
'---',
'',
].join('\n'));
const script = ['set -eu', helper, 'extract_summary_files "$SUMMARY_PATH"'].join('\n');
const result = toLegacyResult(runHook('-c', [script, 'bash'], {
interpreter: 'bash',
env: { ...process.env, SUMMARY_PATH: shellSummary },
timeoutMs: PROBE_TIMEOUT_MS,
}));
assert.equal(result.status, 0, result.stderr);
assert.deepStrictEqual(result.stdout.trim().split('\n'), [dollarPath, backtickPath, quotePath]);
assert.ok(!fs.existsSync(sentinel), 'SUMMARY payload must never execute command substitutions');
} finally {
cleanup(dir);
}
});
test('extracts only key-files.created and key-files.modified entries', () => {
const yaml = [
'key-files:',
' created:',
' - app/foo.tsx',
' modified:',
' - lib/bar.ts',
'key-decisions:',
' - We chose RSC for performance reasons',
'patterns-established:',
' - Always validate at the boundary',
'requirements-completed:',
' - REQ-01 done',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files.sort(), ['app/foo.tsx', 'lib/bar.ts'].sort());
});
test('strips em-dash narrative from bullet: "app/foo.tsx — RSC catalogue with filters"', () => {
const yaml = [
'key-files:',
' created:',
' - app/foo.tsx — RSC catalogue with topic/mode/date filters',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files, ['app/foo.tsx']);
});
test('strips parenthetical from bullet: "tests/bar.test.ts (122 lines — 17 assertions)"', () => {
const yaml = [
'key-files:',
' created:',
' - tests/bar.test.ts (122 lines — 17 assertions)',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files, ['tests/bar.test.ts']);
});
test('hyphenated sections in any order produce identical results', () => {
const yamlA = [
'key-decisions:',
' - Some decision',
'key-files:',
' created:',
' - src/index.ts',
'patterns-established:',
' - Some pattern',
].join('\n');
const yamlB = [
'patterns-established:',
' - Some pattern',
'key-files:',
' created:',
' - src/index.ts',
'key-decisions:',
' - Some decision',
].join('\n');
assert.deepStrictEqual(parseKeyFiles(yamlA), parseKeyFiles(yamlB));
assert.deepStrictEqual(parseKeyFiles(yamlA), ['src/index.ts']);
});
test('prose-only bullets from key-decisions are never included in file list', () => {
const yaml = [
'key-decisions:',
' - We chose RSC for performance reasons',
' - Deferred auth to Phase 3',
'key-files:',
' created:',
' - app/page.tsx',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files, ['app/page.tsx']);
});
// #2666 — the Tier-2 extractor must NOT drop repository-root files (no `/`)
// or known extensionless build files. Pre-fix the buggy predicate
// `/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)` dropped every root-level
// path and every extensionless build file anywhere in the tree.
test('#2666 RED: root-level files with extensions are accepted (package.json, renovate.json, .gitlab-ci.yml, AGENTS.md)', () => {
const yaml = [
'key-files:',
' modified:',
' - package.json',
' - renovate.json',
' - .gitlab-ci.yml',
' - AGENTS.md',
' - CLAUDE.md',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(
files.sort(),
['.gitlab-ci.yml', 'AGENTS.md', 'CLAUDE.md', 'package.json', 'renovate.json'],
'root-level files with extensions must not be dropped for lacking a directory separator',
);
});
test('#2666: nested extensionless build files are accepted (docker/Dockerfile, web/Makefile)', () => {
const yaml = [
'key-files:',
' modified:',
' - docker/Dockerfile',
' - web/Makefile',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files.sort(), ['docker/Dockerfile', 'web/Makefile']);
});
test('#2666: root-level extensionless build files are accepted (Dockerfile, Makefile, Justfile, Containerfile, Procfile)', () => {
const yaml = [
'key-files:',
' created:',
' - Dockerfile',
' - Makefile',
' - Justfile',
' - Containerfile',
' - Procfile',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(
files.sort(),
['Containerfile', 'Dockerfile', 'Justfile', 'Makefile', 'Procfile'],
);
});
test('#2666 acceptance #1: the reporter 10-file Docker+CI phase yields all 10 paths', () => {
const yaml = [
'key-files:',
' created:',
' - Dockerfile',
' - .gitlab-ci.yml',
' - renovate.json',
' - AGENTS.md',
' - CLAUDE.md',
' - docs/DEVELOPMENT.md',
' - scripts/version-consistency-gate.mjs',
' - web/package.json',
' - web/version_management.md',
' - web/update-version.cjs',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(
files.sort(),
[
'.gitlab-ci.yml', 'AGENTS.md', 'CLAUDE.md', 'Dockerfile',
'docs/DEVELOPMENT.md', 'renovate.json', 'scripts/version-consistency-gate.mjs',
'web/package.json', 'web/update-version.cjs', 'web/version_management.md',
],
'the full reporter phase must scope all 10 files, including Dockerfile + root files',
);
});
test('#2666 negative-space: a path-like prose bullet with no extension and unknown basename is rejected', () => {
// `topic/mode/date filters` has a `/` but no extension and an unknown basename —
// the pre-fix predicate dropped it (good), the relaxed predicate must STILL drop it.
const yaml = [
'key-decisions:',
' - topic/mode/date filters',
'key-files:',
' created:',
' - app/page.tsx',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files, ['app/page.tsx']);
});
test('#2666 negative-space: em-dash/parenthetical stripping still works on an accepted root file', () => {
const yaml = [
'key-files:',
' modified:',
' - Dockerfile — multi-stage build',
].join('\n');
const files = parseKeyFiles(yaml);
assert.deepStrictEqual(files, ['Dockerfile']);
});
// Docs-parity: the workflow .md must contain the hyphen-aware boundary regex
// so what we tested above is actually what is deployed.
test('code-review.md contains hyphen-aware boundary regex [\\w-]+', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
// Locate the Node script block in the compute_file_scope step
const scriptStart = src.indexOf('const files = [];');
assert.ok(scriptStart !== -1, 'compute_file_scope script must contain "const files = [];"');
const scriptEnd = src.indexOf('if (files.length)', scriptStart);
const scriptSection = src.slice(scriptStart, scriptEnd);
// Must use [\\w-]+ (hyphen-aware) not \\w+ only
const hasHyphenAwareRegex = scriptSection.includes('[\\\\w-]') || scriptSection.includes('[\\w-]');
assert.ok(
hasHyphenAwareRegex,
'compute_file_scope boundary regex must be hyphen-aware ([\\w-]+), found section:\n' + scriptSection
);
});
// Docs-parity: the workflow .md must contain the em-dash and parenthetical stripping.
test('code-review.md contains em-dash split and parenthetical strip in script body', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const scriptStart = src.indexOf('const files = [];');
const scriptEnd = src.indexOf('if (files.length)', scriptStart);
const scriptSection = src.slice(scriptStart, scriptEnd);
assert.ok(
scriptSection.includes('replace(/\\s+\\([^)]*\\)\\s*$/, \'\')'),
'Script must strip parentheticals with replace(/\\s+\\([^)]*\\)\\s*$/, \'\')'
);
assert.ok(
scriptSection.includes('split(/\\s+—\\s'),
'Script must split on em-dash to strip narrative'
);
});
// #2666 docs-parity: the shipped workflow must NOT still carry the buggy
// AND-joined predicate that required BOTH a `/` and a trailing extension —
// that predicate dropped every root-level file and every extensionless build
// file. Catches a revert of the #2666 fix.
test('#2666 docs-parity: compute_file_scope does not contain the buggy slash-and-extension predicate', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const scriptStart = src.indexOf('const files = [];');
const scriptEnd = src.indexOf('if (files.length)', scriptStart);
const scriptSection = src.slice(scriptStart, scriptEnd);
assert.ok(
!scriptSection.includes('/\\//.test(raw) && /\\.[A-Za-z0-9]+$/.test(raw)'),
'compute_file_scope must not use the buggy AND-joined /\\//.test(raw) && /\\.[A-Za-z0-9]+$/.test(raw) ' +
'predicate (#2666) — it drops every root-level and extensionless build file. Found section:\n' +
scriptSection
);
});
// #2666 docs-parity: the shipped workflow must reference the known
// extensionless build filenames so Dockerfile/Makefile/etc. are accepted.
test('#2666 docs-parity: compute_file_scope accepts known extensionless build files (Dockerfile)', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const scriptStart = src.indexOf('const files = [];');
const scriptEnd = src.indexOf('if (files.length)', scriptStart);
const scriptSection = src.slice(scriptStart, scriptEnd);
assert.ok(
/dockerfile/i.test(scriptSection),
'compute_file_scope must reference known extensionless build filenames (e.g. Dockerfile) ' +
'so they are not dropped (#2666). Found section:\n' + scriptSection
);
});
// #2666 docs-parity: the Tier-3 git-diff fallback must intersect with the
// SUMMARY scope and warn on dropped files (not only fire on zero Tier-2 hits).
test('#2666 docs-parity: Tier-3 intersects/warns against git diff --name-only', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
// The shipped workflow must compute git diff --name-only AND emit a warning
// when the diff contains files the SUMMARY extractor did not surface.
assert.ok(
src.includes('git diff --name-only'),
'code-review.md must run `git diff --name-only` to cross-check the SUMMARY scope (#2666)'
);
assert.ok(
/warn|missing|not surfaced|did not|not in/i.test(src),
'code-review.md must warn when git diff contains files the SUMMARY extractor dropped (#2666)'
);
});
// #2666 docs-parity: the membership test must be EXACT whole-line matching
// (grep -Fxq), not an unanchored `case` substring match — otherwise a short
// basename in the diff (root `Dockerfile`) substring-matches a longer scoped
// path (`docker/Dockerfile`) and is silently skipped, reintroducing the bug.
test('#2666 docs-parity: Tier-3 cross-check uses exact whole-line matching (grep -Fxq), not substring case', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
assert.ok(
src.includes('grep -Fxq'),
'code-review.md Tier-3 cross-check must use grep -Fxq (exact whole-line match) for membership ' +
'testing, not an unanchored `case` substring match that would skip a root `Dockerfile` ' +
'whose name appears as a suffix of an already-scoped `docker/Dockerfile` (#2666)'
);
// The unanchored substring `case "$IN_SCOPE" in` membership test must NOT be
// present — it would false-match a basename suffix. Plain substring check (no
// regex, so no CRLF-fragility): the grep -Fxq positive guard above proves the
// correct mechanism; this negative guard catches a revert to the `case` form.
assert.ok(
!src.includes('case "$IN_SCOPE"'),
'code-review.md Tier-3 must not use the unanchored `case "$IN_SCOPE"` substring membership ' +
'test (#2666) — use grep -Fxq for exact whole-line matching'
);
});
});
// ---------------------------------------------------------------------------
// BUG 2 — severity-label parser: present_results must accept both `critical:`
// and `blocker:` as Critical-tier frontmatter keys.
// ---------------------------------------------------------------------------
describe('Bug 2 — present_results severity-label parser', () => {
test('frontmatter with blocker: 8 is parsed as critical: 8', () => {
const frontmatter = [
'phase: 03-courses',
'reviewed: 2025-01-01T00:00:00Z',
'findings:',
' blocker: 8',
' warning: 2',
' info: 0',
' total: 10',
'status: issues_found',
].join('\n');
const result = parseFrontmatterCritical(frontmatter);
assert.strictEqual(result.critical, 8);
});
test('frontmatter with critical: 5 is parsed as critical: 5', () => {
const frontmatter = [
'phase: 03-courses',
'reviewed: 2025-01-01T00:00:00Z',
'findings:',
' critical: 5',
' warning: 1',
' info: 0',
' total: 6',
'status: issues_found',
].join('\n');
const result = parseFrontmatterCritical(frontmatter);
assert.strictEqual(result.critical, 5);
});
test('frontmatter with neither critical nor blocker returns 0', () => {
const frontmatter = [
'phase: 03-courses',
'findings:',
' warning: 3',
' info: 1',
' total: 4',
'status: issues_found',
].join('\n');
const result = parseFrontmatterCritical(frontmatter);
assert.strictEqual(result.critical, 0);
});
// Docs-parity: the workflow .md must contain the updated grep pattern.
test('code-review.md present_results grep accepts both critical and blocker labels', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
assert.ok(
src.includes('grep -E "^[[:space:]]*(critical|blocker):"'),
'code-review.md present_results must grep for both critical: and blocker: labels'
);
});
// Docs-parity: the workflow .md must contain the updated grep for BL- headings.
test('code-review.md present_results grep includes BL- headings alongside CR- and WR-', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
assert.ok(
src.includes('### BL-') && src.includes('### CR-') && src.includes('### WR-'),
'code-review.md present_results must grep for BL- alongside CR- and WR- headings'
);
});
});
// ---------------------------------------------------------------------------
// BUG 3 — fixer agent ID alphabet and filter sets must include BL-* alongside CR-*.
// ---------------------------------------------------------------------------
describe('Bug 3 — msd-code-fixer BL-* inclusion in filter sets', () => {
test('finding_parser documents BL-\\d+ as Critical-tier-equivalent', () => {
const src = fs.readFileSync(FIXER_PATH, 'utf8');
const parserStart = src.indexOf('<finding_parser>');
const parserEnd = src.indexOf('</finding_parser>');
assert.ok(parserStart !== -1, 'msd-code-fixer.md must have a <finding_parser> block');
const parserSection = src.slice(parserStart, parserEnd);
assert.ok(
parserSection.includes('BL-'),
'finding_parser block must document BL-* as a Critical-tier-equivalent ID prefix'
);
});
test('parse_findings step documents severity as "Critical (CR-* or BL-*)"', () => {
const src = fs.readFileSync(FIXER_PATH, 'utf8');
const stepStart = src.indexOf('<step name="parse_findings">');
const stepEnd = src.indexOf('</step>', stepStart);
assert.ok(stepStart !== -1, 'msd-code-fixer.md must have a parse_findings step');
const stepSection = src.slice(stepStart, stepEnd);
assert.ok(
stepSection.includes('CR-* or BL-*') || stepSection.includes('CR-* and BL-*'),
'parse_findings step must describe Critical severity as "CR-* or BL-*"'
);
});
test('critical_warning filter set includes BL-* alongside CR-* and WR-*', () => {
const src = fs.readFileSync(FIXER_PATH, 'utf8');
const stepStart = src.indexOf('<step name="parse_findings">');
const stepEnd = src.indexOf('</step>', stepStart);
const stepSection = src.slice(stepStart, stepEnd);
const critWarningIdx = stepSection.indexOf('critical_warning');
assert.ok(critWarningIdx !== -1, 'parse_findings must define critical_warning filter');
const lineStart = stepSection.lastIndexOf('\n', critWarningIdx);
const lineEnd = stepSection.indexOf('\n', critWarningIdx);
const filterLine = stepSection.slice(lineStart, lineEnd);
assert.ok(
filterLine.includes('BL-'),
'critical_warning filter line must include BL-*: ' + filterLine.trim()
);
});
test('sort order description mentions both CR-* and BL-* for Critical tier', () => {
const src = fs.readFileSync(FIXER_PATH, 'utf8');
const stepStart = src.indexOf('<step name="parse_findings">');
const stepEnd = src.indexOf('</step>', stepStart);
const stepSection = src.slice(stepStart, stepEnd);
assert.ok(
stepSection.includes('BL-'),
'parse_findings sort-order description must mention BL-* as Critical-tier alongside CR-*'
);
});
});
// ---------------------------------------------------------------------------
// REVIEWER CONTRACT — msd-code-reviewer.md must acknowledge BL-/blocker: as
// an accepted alternative to CR-/critical: (tier-equivalent).
// ---------------------------------------------------------------------------
describe('Reviewer contract — msd-code-reviewer.md label-equivalence', () => {
test('write_review step documents blocker: as accepted alternative to critical:', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepStart = src.indexOf('<step name="write_review">');
const stepEnd = src.indexOf('</step>', stepStart);
assert.ok(stepStart !== -1, 'msd-code-reviewer.md must have a write_review step');
const stepSection = src.slice(stepStart, stepEnd);
assert.ok(
stepSection.includes('blocker'),
'write_review step must acknowledge blocker: as a tier-equivalent alternative to critical:'
);
});
test('write_review step acknowledges BL- finding ID prefix as Critical-tier-equivalent', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepStart = src.indexOf('<step name="write_review">');
const stepEnd = src.indexOf('</step>', stepStart);
const stepSection = src.slice(stepStart, stepEnd);
assert.ok(
stepSection.includes('BL-'),
'write_review step must acknowledge BL- as a Critical-tier-equivalent finding ID prefix'
);
});
});
// ---------------------------------------------------------------------------
// BUG 4 (#2352) — compute_file_scope must tilde-expand `~/...`-prefixed
// SUMMARY.md key-files entries BEFORE the "Filter deleted files" existence
// check. Bash only tilde-expands a literal `~` written in source text, never
// one arriving as the value of an already-expanded variable — so a real file
// recorded as `~/.claude/msd-core/workflows/verify-phase.md` was silently
// misclassified as deleted and dropped from REVIEW_FILES, and a phase whose
// every recorded file used a `~/...` path hit the empty-scope skip
// ("No source files changed ... Skipping review.") as a false negative.
//
// Tested both ways: a docs-parity assertion (cross-platform, pure fs read)
// that the normalization block exists in the deployed workflow text, and a
// behavioral test that extracts the actual "Expand tilde paths" +
// "Filter deleted files" bash blocks from code-review.md and executes them
// via a real bash subprocess against planted files under a fresh HOME.
// ---------------------------------------------------------------------------
describe('Bug 4 (#2352) — compute_file_scope tilde-path expansion', () => {
// Docs-parity: the workflow .md must contain the tilde-normalization block
// as step 1 of "Post-processing (all tiers)", ahead of the deleted-file
// filter, so what we behaviorally test below is what is actually deployed.
test('code-review.md contains a tilde-expansion block ahead of the deleted-file filter', () => {
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const postProcessingIdx = src.indexOf('**Post-processing (all tiers):**');
assert.ok(postProcessingIdx !== -1, 'code-review.md must have a "Post-processing (all tiers)" section');
const expandIdx = src.indexOf('EXPANDED_FILES=()', postProcessingIdx);
assert.ok(expandIdx !== -1, 'Post-processing must contain an EXPANDED_FILES=() tilde-expansion loop');
const caseIdx = src.indexOf('case "$file" in', postProcessingIdx);
assert.ok(caseIdx !== -1 && caseIdx < expandIdx + 400, 'tilde-expansion loop must use a case "$file" in match');
assert.ok(
src.slice(caseIdx, caseIdx + 200).includes('"~/"*)') &&
src.slice(caseIdx, caseIdx + 200).includes('${HOME}${file#\\~}'),
'tilde-expansion loop must rewrite a leading ~/ to ${HOME}/... via ${file#\\~}'
);
const deletedFilterIdx = src.indexOf('DELETED_COUNT=0', postProcessingIdx);
assert.ok(deletedFilterIdx !== -1, 'Post-processing must still contain the deleted-file filter');
assert.ok(
expandIdx < deletedFilterIdx,
'tilde-expansion loop must run BEFORE the deleted-file filter, not after'
);
});
// Extract the tilde-expansion fence and the (non-adjacent — the exclusions
// filter sits between them) deleted-file-filter fence from the
// "Post-processing (all tiers)" section of code-review.md — the exact
// snippets the runtime executes, located by content anchor rather than
// position so an intervening step doesn't silently swap in the wrong
// block — and glue them behind a synthetic REVIEW_FILES=("$@") seed for
// direct execution. The exclusions filter itself is intentionally skipped
// here: it only matches relative planning-artifact paths and is orthogonal
// to tilde expansion (see code-review.md step 2, "Apply exclusions").
function extractPostProcessingScript() {
// readFileNormalized() strips \r\n -> \n before either fence below is
// sliced out and later spawned via spawnSync('bash', ...) in
// runPostProcessing() — an un-normalized read on a Windows checkout would
// break bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
const src = readFileNormalized(WORKFLOW_PATH);
const postProcessingIdx = src.indexOf('**Post-processing (all tiers):**');
assert.ok(postProcessingIdx !== -1, 'code-review.md must have a "Post-processing (all tiers)" section');
function fenceContaining(marker) {
const markerIdx = src.indexOf(marker, postProcessingIdx);
assert.ok(markerIdx !== -1, `expected to find "${marker}" in the Post-processing section`);
const fenceStart = src.lastIndexOf('```bash', markerIdx);
assert.ok(fenceStart !== -1 && fenceStart > postProcessingIdx, `no \`\`\`bash fence before "${marker}"`);
const bodyStart = src.indexOf('\n', fenceStart) + 1;
const fenceEnd = src.indexOf('\n```', bodyStart);
assert.ok(fenceEnd !== -1, `unterminated \`\`\`bash fence containing "${marker}"`);
return src.slice(bodyStart, fenceEnd);
}
const tildeBlock = fenceContaining('EXPANDED_FILES=()');
const deletedBlock = fenceContaining('DELETED_COUNT=0');
return [
'REVIEW_FILES=("$@")',
tildeBlock,
deletedBlock,
'printf "%s\\n" "${REVIEW_FILES[@]}"',
'echo "REVIEW_FILES_COUNT=${#REVIEW_FILES[@]}"',
'echo "DELETED_COUNT=$DELETED_COUNT"',
].join('\n');
}
function runPostProcessing(homeDir, files) {
const script = extractPostProcessingScript();
// "bash" as $0 so the real REVIEW_FILES entries land in "$@" from $1.
return toLegacyResult(
runHook('-c', [script, 'bash', ...files], {
interpreter: 'bash',
env: { ...process.env, HOME: homeDir },
timeoutMs: PROBE_TIMEOUT_MS,
})
);
}
let tmpHome;
test('setup: plant a fresh HOME with a real file', { skip: process.platform === 'win32' }, () => {
tmpHome = createTempDir('msd-2352-home-');
fs.mkdirSync(path.join(tmpHome, '.claude', 'msd-core', 'workflows'), { recursive: true });
fs.writeFileSync(
path.join(tmpHome, '.claude', 'msd-core', 'workflows', 'verify-phase.md'),
'# real file\n',
'utf8'
);
});
test(
'AC1: a ~/-prefixed path to a real file survives and is not counted deleted',
{ skip: process.platform === 'win32' },
() => {
const result = runPostProcessing(tmpHome, ['~/.claude/msd-core/workflows/verify-phase.md']);
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
assert.match(
result.stdout,
new RegExp(path.join(tmpHome, '.claude', 'msd-core', 'workflows', 'verify-phase.md').replace(/[/\\.]/g, '\\$&')),
`expected expanded absolute path in surviving REVIEW_FILES; got: ${JSON.stringify(result.stdout)}`
);
assert.match(result.stdout, /DELETED_COUNT=0/, `expected DELETED_COUNT=0; got: ${JSON.stringify(result.stdout)}`);
assert.match(
result.stdout,
/REVIEW_FILES_COUNT=1/,
`expected the tilde path to survive into REVIEW_FILES; got: ${JSON.stringify(result.stdout)}`
);
}
);
test(
'AC2: a ~/-prefixed path to a non-existent file is still correctly excluded as deleted',
{ skip: process.platform === 'win32' },
() => {
const result = runPostProcessing(tmpHome, ['~/.claude/msd-core/workflows/does-not-exist.md']);
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
assert.match(result.stdout, /DELETED_COUNT=1/, `expected DELETED_COUNT=1; got: ${JSON.stringify(result.stdout)}`);
assert.match(
result.stdout,
/REVIEW_FILES_COUNT=0/,
`expected the missing tilde path to be dropped; got: ${JSON.stringify(result.stdout)}`
);
}
);
test(
'AC3: a phase where every recorded file is a real ~/-prefixed path does not empty the scope',
{ skip: process.platform === 'win32' },
() => {
const result = runPostProcessing(tmpHome, ['~/.claude/msd-core/workflows/verify-phase.md']);
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
const countMatch = result.stdout.match(/REVIEW_FILES_COUNT=(\d+)/);
assert.ok(countMatch, `expected a REVIEW_FILES_COUNT line; got: ${JSON.stringify(result.stdout)}`);
assert.ok(
Number(countMatch[1]) > 0,
'an all-tilde real-file scope must not reduce to zero (would trigger the empty-scope skip)'
);
}
);
test(
'AC4: mixed tilde + missing ordinary relative path resolve independently',
{ skip: process.platform === 'win32' },
() => {
const result = runPostProcessing(tmpHome, [
'~/.claude/msd-core/workflows/verify-phase.md',
'this/relative/path/does-not-exist.md',
]);
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
assert.match(result.stdout, /DELETED_COUNT=1/, `expected exactly 1 deleted; got: ${JSON.stringify(result.stdout)}`);
assert.match(
result.stdout,
/REVIEW_FILES_COUNT=1/,
`expected only the tilde path to survive; got: ${JSON.stringify(result.stdout)}`
);
assert.doesNotMatch(
result.stdout,
/this\/relative\/path\/does-not-exist\.md/,
'the missing ordinary relative path must not survive into REVIEW_FILES'
);
}
);
test('teardown: remove the temp HOME', { skip: process.platform === 'win32' }, () => {
cleanup(tmpHome);
});
});
// ---------------------------------------------------------------------------
// Shared diff-base extraction/execution helpers (Bug 5 #3191, Bug 6 #3503).
//
// The workflow computes "the phase's base commit" in three independent bash
// invocations (each <step> is its own shell): the Tier-3 file-scope fallback
// (compute_file_scope), the agent-context DIFF_BASE (spawn_reviewer), and the
// fallow pre-pass's --changed-since base (structural-pre-pass.md).
//
// Behavioral style follows Bug 4: extract the SHIPPED bash from the workflow
// .md files by content anchor and execute it via a real bash subprocess
// against a git fixture — so the assertion binds the deployed text, not a
// JS reimplementation. Running the real `git log` (not a regex shim) is what
// makes platform-level regex holes (the #3191 macOS `\b` no-op) visible.
// ---------------------------------------------------------------------------
// The ```bash fence containing `marker`, located after `fromIdx`.
function fenceContaining(src, marker, fromIdx = 0) {
const markerIdx = src.indexOf(marker, fromIdx);
assert.ok(markerIdx !== -1, `expected to find "${marker}" in workflow source`);
const fenceStart = src.lastIndexOf('```bash', markerIdx);
assert.ok(fenceStart !== -1, `no \`\`\`bash fence before "${marker}"`);
const bodyStart = src.indexOf('\n', fenceStart) + 1;
const fenceEnd = src.indexOf('\n```', bodyStart);
assert.ok(fenceEnd !== -1, `unterminated \`\`\`bash fence containing "${marker}"`);
return src.slice(bodyStart, fenceEnd);
}
// The Tier-3 derivation prefix: fence start up to the REVIEW_FILES branch.
function extractTier3Derivation() {
const src = readFileNormalized(WORKFLOW_PATH);
const fence = fenceContaining(src, '# Compute diff base from phase commits');
const cut = fence.indexOf('if [ ${#REVIEW_FILES[@]} -eq 0 ]');
assert.ok(cut !== -1, 'Tier-3 fence must contain the REVIEW_FILES empty-scope branch');
return fence.slice(0, cut);
}
// spawn_reviewer no longer derives its own DIFF_BASE (#4209 B3 fix: a second,
// divergent recomputation there made the external reviewer lane and the
// internal reviewer diff against different base SHAs on any re-review). It
// now reuses the value compute_file_scope's Tier-3 derivation already
// computed, so this is the SAME snippet as extractTier3Derivation() — kept
// as a distinct name so T2/T5 below still read as testing spawn_reviewer's
// contract, not just Tier 3's.
function extractSpawnReviewerDerivation() {
return extractTier3Derivation();
}
// The fallow phase-scope derivation, from the step fragment. The fragment
// carries markdown-escaped quotes (\") in this fence — an authoring
// artifact that survived #2994 fragmentization verbatim; the runtime agent
// normalizes them when transcribing, so the test does the same before
// executing. Sliced from FALLOW_SCOPE_ARGS=() (skipping the msd-tools
// runtime resolver line above it, which exits 1 on machines without an
// installed msd-tools and is orthogonal to the base-derivation under test)
// to just before the msd_run invocation (which needs the real binary).
function extractFallowDerivation() {
const src = readFileNormalized(PRE_PASS_STEP_PATH);
const fence = fenceContaining(src, 'FALLOW_PHASE_START=$(git log');
const scopeStart = fence.indexOf('FALLOW_SCOPE_ARGS=()');
assert.ok(scopeStart !== -1, 'fallow fence must define FALLOW_SCOPE_ARGS=()');
const cut = fence.indexOf('msd_run run-with-timeout');
assert.ok(cut !== -1, 'fallow fence must contain the msd_run run-with-timeout call');
assert.ok(scopeStart < cut, 'FALLOW_SCOPE_ARGS must precede the msd_run invocation');
return fence.slice(scopeStart, cut).replace(/\\"/g, '"');
}
// Execute a derivation snippet with PADDED_PHASE (and the fallow scope gate)
// set, echoing the values it computes between sentinels so multi-line
// PHASE_COMMITS parse cleanly.
function runDerivation(repo, snippet, phase) {
const script = [
`PADDED_PHASE=${phase}`,
// #3995: the derivations anchor on the phase's own directory, not a
// commit-subject grep — the fixture commits each phase's directory at
// its first scope commit.
`PHASE_DIR=${repo}/.planning/phases/${phase}-ctx`,
'FALLOW_SCOPE=phase',
snippet,
'echo "===PHASE_START==="',
'printf \'%s\\n\' "$PHASE_START"',
'echo "===DIFF_BASE==="',
'printf \'%s\\n\' "$DIFF_BASE"',
'echo "===FALLOW_BASE==="',
'printf \'%s\\n\' "$FALLOW_BASE"',
'echo "===END==="',
].join('\n');
// Bash FAN-OUT: the extracted snippet runs `git log` plus an `echo | tail`
// pipe — the wrong class for `PROBE_TIMEOUT_MS` (a single short CLI
// probe). Same class as the observed CI failures in
// tests/quick-branching.test.cjs (PR #3787 run 32668773524) and
// tests/worktree-safety.test.cjs (`next` run 32608945654). See
// HOOK_FANOUT_TIMEOUT_MS in ./helpers/timeouts.cjs for the class
// rationale.
return toLegacyResult(
runHook('-c', [script, 'bash'], {
interpreter: 'bash',
cwd: repo,
timeoutMs: HOOK_FANOUT_TIMEOUT_MS,
})
);
}
function parseSentinel(stdout, name) {
const m = stdout.match(new RegExp(`===${name}===\\n([\\s\\S]*?)\\n===`));
if (!m) return null;
return m[1].split('\n').map((l) => l.trim()).filter((l) => l.length > 0);
}
// ---------------------------------------------------------------------------
// Bug 5 (#3191) — EVERY diff-base derivation must use the same anchored,
// portable derivation.
//
// The workflow computes "the phase's base commit" in three independent bash
// invocations (each <step> is its own shell): the Tier-3 file-scope fallback
// (compute_file_scope), the agent-context DIFF_BASE (spawn_reviewer), and the
// fallow pre-pass's --changed-since base (structural-pre-pass.md). #2989
// anchored only the Tier-3 copy — and did so with `\b`, which is not a POSIX
// ERE token, so on macOS (regex(3)) that grep matches NOTHING and Tier 3
// always fails closed. The other two sites kept the original unanchored
// `--grep="${PADDED_PHASE}"`, whose oldest substring match is routinely a
// version-string/date commit from months before the phase existed.
// (#3503 later replaced the anchor itself — a subject-line conventional-
// commit scope match instead of the "[Pp]hase N" prose phrase, which MSD's
// own commits never contain; see Bug 6. The lockstep + portability +
// fail-closed contract THIS block verifies is unchanged.)
//
// Behavioral style follows Bug 4: extract the SHIPPED bash from the workflow
// .md files by content anchor and execute it via a real bash subprocess
// against a git fixture — so the assertion binds the deployed text, not a
// JS reimplementation. Running the real `git log` (not a regex shim) is what
// keeps platform-level regex holes (the #3191 macOS `\b` no-op) visible.
// ---------------------------------------------------------------------------
// Shared: the fixture phase directory every derivation anchors on (#3995).
const PHASE06_PLAN_REL = path.join('.planning', 'phases', '06-ctx', '06-PLAN.md');
// Shared history builder (was local to the #3503 describe; the #3995 rows
// reuse it). Each entry is [relPath, subject, body?]; parent dirs are created.
function buildHistory(prefix, commits) {
const repo = createTempGitProject(prefix);
const hashes = {};
for (const [file, message, body] of commits) {
fs.mkdirSync(path.dirname(path.join(repo, file)), { recursive: true });
fs.writeFileSync(path.join(repo, file), `${message}\n`);
gitOrThrow(['add', file], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['commit', '-m', message, ...(body ? ['-m', body] : [])], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
hashes[file] = gitOrThrow(['rev-parse', 'HEAD'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim();
}
return { repo, hashes };
}
describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all three diff-base sites', () => {
const SKIP_WIN32 = { skip: process.platform === 'win32' };
// Fixture: five commits whose messages exercise every false-match class
// from the issue — version string + date, another phase's plan whose scope
// number is a digit-superset, a prose "Phase N" mention in another phase's
// subject — plus the phase's real first scope commit and an unrelated HEAD.
function buildFixture(prefix, phaseCommitMessage, opts = {}) {
// opts.skipPhaseDir: the fail-closed row (T5) commits NO phase directory,
// so the directory anchor must resolve nothing.
const commitPhaseDir = opts.skipPhaseDir !== true;
const repo = createTempGitProject(prefix);
const phaseDir = path.join(repo, '.planning', 'phases', '06-ctx');
fs.mkdirSync(phaseDir, { recursive: true });
const commits = [
['c1.txt', 'chore: bump to v2.06.0 on 2026-01-05'],
['c2.txt', 'docs(60-01): unrelated phase-plan work'],
[commitPhaseDir ? PHASE06_PLAN_REL : 'c3.txt', phaseCommitMessage],
['c4.txt', 'chore: Phase 60 cleanup'],
['c5.txt', 'docs: touch README'],
];
const hashes = {};
for (const [file, message] of commits) {
fs.mkdirSync(path.dirname(path.join(repo, file)), { recursive: true });
fs.writeFileSync(path.join(repo, file), `${message}\n`);
gitOrThrow(['add', file], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['commit', '-m', message], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
hashes[file] = gitOrThrow(['rev-parse', 'HEAD'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim();
}
return { repo, hashes };
}
test(
'T1 + T4: Tier-3 derivation matches ONLY the phase\'s real scope commit — never a digit-substring or superset hit',
SKIP_WIN32,
() => {
const { repo, hashes } = buildFixture('msd-3191-tier3-', 'docs(06): capture phase context');
try {
const result = runDerivation(repo, extractTier3Derivation(), '06');
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
const phaseStart = parseSentinel(result.stdout, 'PHASE_START');
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
// AC: the phase's real commits are a small minority of digit-containing
// commits; the derivation must resolve to an ancestor near the phase's
// actual first commit (c3^) — never the older v2.06.0/docs(06-01) hits.
assert.deepStrictEqual(
phaseStart,
[hashes[PHASE06_PLAN_REL]],
`Tier-3 anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}`
);
assert.deepStrictEqual(
diffBase,
[`${hashes[PHASE06_PLAN_REL]}^`],
'Tier-3 DIFF_BASE must be the phase first-commit parent'
);
} finally {
cleanup(repo);
}
}
);
test(
'T2: spawn_reviewer DIFF_BASE derivation uses the same anchored grep (not the bare digit)',
SKIP_WIN32,
() => {
const { repo, hashes } = buildFixture('msd-3191-spawn-', 'docs(06): capture phase context');
try {
const result = runDerivation(repo, extractSpawnReviewerDerivation(), '06');
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
const phaseStart = parseSentinel(result.stdout, 'PHASE_START');
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
// Pre-fix this matches c1 and c2 as well and tail -1 picks c1 — the
// oldest unrelated match — feeding a bogus diff_base to the reviewer
// agent exactly when files: is empty (the fail-closed scenario).
assert.deepStrictEqual(
phaseStart,
[hashes[PHASE06_PLAN_REL]],
`spawn_reviewer anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}`
);
assert.deepStrictEqual(
diffBase,
[`${hashes[PHASE06_PLAN_REL]}^`],
'spawn_reviewer DIFF_BASE must be the phase first-commit parent'
);
} finally {
cleanup(repo);
}
}
);
test(
'T3: fallow phase scope derives --changed-since from the anchored grep, never an old substring match',
SKIP_WIN32,
() => {
const { repo, hashes } = buildFixture('msd-3191-fallow-', 'docs(06): capture phase context');
try {
const result = runDerivation(repo, extractFallowDerivation(), '06');
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
// Pre-fix the unanchored grep's oldest match is the v2.06.0 commit, so
// FALLOW_SCOPE_ARGS resolves to --changed-since <old-unrelated-commit>
// and widens the structural pre-pass far beyond the phase.
assert.deepStrictEqual(
fallowBase,
[`${hashes[PHASE06_PLAN_REL]}^`],
`FALLOW_BASE must be the phase first-commit parent, got: ${JSON.stringify(fallowBase)}`
);
} finally {
cleanup(repo);
}
}
);
test(
'root commit: fallow phase scope uses a resolvable root SHA',
SKIP_WIN32,
() => {
const repo = createTempDir('msd-4183-fallow-root-');
try {
gitOrThrow(['init', '-b', 'main'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['config', 'user.name', 'Test'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
const phaseFile = path.join(repo, '.planning', 'phases', '06-ctx', 'PLAN.md');
fs.mkdirSync(path.dirname(phaseFile), { recursive: true });
fs.writeFileSync(phaseFile, '# phase context\n');
gitOrThrow(['add', '.planning'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['commit', '-m', 'docs(06): initial phase context'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
const rootSha = gitOrThrow(['rev-parse', 'HEAD'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim();
fs.writeFileSync(path.join(repo, 'index.js'), 'module.exports = 1;\n');
gitOrThrow(['add', 'index.js'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
gitOrThrow(['commit', '-m', 'feat: add source'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS });
const result = runDerivation(repo, extractFallowDerivation(), '06');
assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`);
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
assert.deepStrictEqual(
fallowBase,
[rootSha],
`root-parent FALLOW_BASE regression: expected ${rootSha}, got ${JSON.stringify(fallowBase)}`,
);
assert.equal(
gitOrThrow(['rev-parse', '--verify', `${fallowBase[0]}^{commit}`], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim(),
rootSha,
'FALLOW_BASE must resolve to the root commit',
);
if (process.env.CI) {
const { requireFallowBinary } = require('../msd-core/bin/lib/fallow-runner.cjs');
const { execTool } = require('../msd-core/bin/lib/shell-command-projection.cjs');
const audit = execTool(
requireFallowBinary({ cwd: ROOT, envPath: '' }),
['audit', '--changed-since', fallowBase[0], '--format', 'json'],
{ cwd: repo, timeout: FALLOW_AUDIT_TIMEOUT_MS },
);
assert.ok([0, 1].includes(audit.exitCode), `fallow root audit exit=${audit.exitCode}; stderr=${audit.stderr}`);
console.log(`fallow-root-audit normal-exit=${audit.exitCode}`);
}
} finally {
cleanup(repo);
}
},
);
test(
'T5: with no genuine phase scope commit, every derivation yields NO base (fail-closed preserved)',
SKIP_WIN32,
() => {
const { repo } = buildFixture('msd-3191-closed-', 'feat: scanner core', { skipPhaseDir: true }); // no committed phase dir anywhere
try {
for (const [label, snippet] of [
['tier3', extractTier3Derivation()],
['spawn_reviewer', extractSpawnReviewerDerivation()],
['fallow', extractFallowDerivation()],
]) {
const result = runDerivation(repo, snippet, '06');
assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`);
const phaseStart = parseSentinel(result.stdout, 'PHASE_START');
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
assert.deepStrictEqual(phaseStart, [], `${label}: no phase dir committed — anchor must stay empty`);
assert.deepStrictEqual(diffBase, [], `${label}: DIFF_BASE must stay empty (no bogus base)`);
assert.deepStrictEqual(fallowBase, [], `${label}: FALLOW_BASE must stay unset`);
}
} finally {
cleanup(repo);
}
}
);
// T6 docs-parity anti-revert (#3191/#3995): every diff-base derivation in
// both files must use the SAME phase-directory anchor — and no message-grep
// derivation may return (a subject carries no milestone bound; that class
// failed five times: #2989/#3191/#3503/#3995).
// #4259: the T6 scan drives itself off the live workflow files, which are
// clean — so its matching branch is exercised only by whatever those files
// happen to contain, and the hole it had was invisible for exactly that
// reason. These fixtures drive findGrepSites directly, in both directions.
test('#4259 T6 site scan sees a backslash-continued derivation, and still ignores what it should', () => {
const sameLine = [
'```bash',
'PHASE_START=$(git log --extended-regexp --grep="^(feat|fix)\\(phase-${PHASE_SCOPE_NUM}" --format="%H")',
'```',
].join('\n');
// Semantically identical to the row above. The only difference is two
// continued physical lines, and that used to be enough to vanish.
const continued = [
'```bash',
'PHASE_START=$(git log \\',
' --extended-regexp \\',
' --grep="^(feat|fix)\\(phase-${PHASE_SCOPE_NUM}" --format="%H")',
'```',
].join('\n');
assert.equal(findGrepSites(sameLine).length, 1, 'the same-line form must stay caught');
assert.equal(findGrepSites(continued).length, 1, 'the continued form must now be caught (#4259)');
// The filter T6 actually asserts on has to see the marker too. Before the
// fold this failed twice over: the scan returned nothing, AND
// PHASE_SCOPE_NUM sat on a different physical line from the one the scan
// would have captured, so even a matching scan would have filtered it out.
for (const src of [sameLine, continued]) {
assert.equal(
findGrepSites(src).filter((l) => l.includes('PHASE_SCOPE_NUM')).length,
1,
'the captured site must carry the marker T6 filters on',
);
}
// Negative control that DOES exercise the fold: a continued, non-phase
// grep site remains a site, but must not become a phase-scope finding.
const benign = [
'```bash',
'RELEASE_NOTES=$(git log \\',
' --grep="^chore" --format="%s")',
'```',
].join('\n');
assert.equal(
findGrepSites(benign).filter((l) => l.includes('PHASE_SCOPE_NUM') || /phase-\)?\(/.test(l)).length,
0,
'a non-phase-scope --grep must stay clean',
);
// A wrapped git-log assignment that merely sits near a --grep string must
// not be glued into one logical line with it. This exercises the fold and
// still contains every keyword the site regex looks for.
const unrelated = [
'```bash',
'SOME_VAR=$(git log \\',
' --format="%H")',
'echo "--grep=$SOME_VAR"',
'```',
].join('\n');
assert.deepStrictEqual(findGrepSites(unrelated), [], 'a wrapped unrelated assignment must not glue into a hit');
// A continuation must not reach across a blank line — the reason this
// folds [ \t]* rather than \s* after the newline.
const acrossBlank = [
'```bash',
'SOME_VAR=$(git log \\',
'',
'FOO=--grep=x',
'```',
].join('\n');
assert.deepStrictEqual(findGrepSites(acrossBlank), [], 'the fold must stop at a blank line');
// Two trailing backslashes represent one literal backslash followed by a
// real newline. Folding this would invent a site the shell does not have.
const evenBackslashes = [
'```bash',
'PHASE_START=$(git log \\\\',
' --grep="phase-${PHASE_SCOPE_NUM}")',
'```',
].join('\n');
assert.deepStrictEqual(
findGrepSites(evenBackslashes),
[],
'an even trailing-backslash run is not a shell continuation',
);
// Three trailing backslashes retain one literal pair while the final
// backslash continues the command. This pins the non-trivial odd boundary.
const oddBackslashes = [
'```bash',
`PHASE_START=$(git log ${'\\'.repeat(3)}`,
' --grep="phase-${PHASE_SCOPE_NUM}")',
'```',
].join('\n');
assert.deepStrictEqual(
findGrepSites(oddBackslashes),
[`PHASE_START=$(git log ${'\\'.repeat(2)} --grep="phase-\${PHASE_SCOPE_NUM}")`],
'an odd trailing-backslash run keeps its literal pairs and continues the line',
);
});
test('#4259 continuation folding preserves every odd/even backslash-run boundary', () => {
fc.assert(fc.property(
fc.integer({ min: 0, max: 31 }),
fc.array(fc.constantFrom(' ', '\t'), { maxLength: 8 }).map((chars) => chars.join('')),
(runLength, indentation) => {
const slashes = '\\'.repeat(runLength);
const source = `cmd ${slashes}\n${indentation}tail`;
const expected = runLength % 2 === 1
? `cmd ${'\\'.repeat(runLength - 1)} tail`
: source;
assert.strictEqual(foldShellContinuations(source), expected);
},
), { numRuns: 200 });
});
test('#4259 T6 site scan reports nothing on the live workflow files', () => {
// The adoption check: only shell code fences are scanned, so markdown hard
// breaks and examples in other languages cannot be folded into fake shell
// sites. Distinct from T6 itself, this asserts the live scan is quiet.
for (const src of [
readFileNormalized(WORKFLOW_PATH),
readFileNormalized(PRE_PASS_STEP_PATH).replace(/\\"/g, '"'),
]) {
assert.deepStrictEqual(findGrepSites(src), []);
}
});
test('T6 docs-parity: all diff-base derivations use the identical phase-directory anchor; no --grep site remains', () => {
const sources = [
readFileNormalized(WORKFLOW_PATH),
readFileNormalized(PRE_PASS_STEP_PATH).replace(/\\"/g, '"'),
];
for (const src of sources) {
assert.ok(
src.includes('PHASE_START=$(git log --format="%H" --diff-filter=A -- "${PHASE_DIR}"'),
'each file must derive the base from the phase directory\'s first commit (#3995)'
);
}
const grepSites = [];
for (const src of sources) {
// #4259: findGrepSites folds backslash continuations first, so a wrapped
// assignment presents as one logical line and cannot slip the scan.
grepSites.push(...findGrepSites(src));
}
assert.deepStrictEqual(
grepSites.filter((l) => l.includes('PHASE_SCOPE_NUM') || /phase-\)?\(/.test(l)),
[],
'no phase-scope message-grep derivation may remain — subjects carry no milestone bound (#3995)'
);
});
});
describe('Bug 6 (#3503/#3995) — diff base keys on the phase directory, not commit subjects', () => {
const SKIP_WIN32 = { skip: process.platform === 'win32' };
const REPRO_HISTORY = [
['c1.txt', 'chore: bump to v2.06.0 on 2026-01-05'],
['c2.txt', 'feat(60-01): probe wiring', 'The EF path still uses it, fenced to Phase 06 per D-09.'],
['c3.txt', 'docs: commit message format', 'Phase headers use the form:\n\n### Phase 06 (Cluster B): Title\n\nin ROADMAP detail sections.'],
[PHASE06_PLAN_REL, 'docs(06): capture phase context'],
['c5.txt', 'feat(06-01): implement scanner core'],
['c6.txt', 'docs(phase-6): update tracking after wave 1'],
['c7.txt', 'docs: touch README'],
];
test(
'T1: prose forward-references and doc-format examples never capture the base — it resolves to the phase dir first commit at all three sites',
SKIP_WIN32,
() => {
const { repo, hashes } = buildHistory('msd-3503-scope-', REPRO_HISTORY);
try {
const sites = [
['tier3', extractTier3Derivation()],
['spawn_reviewer', extractSpawnReviewerDerivation()],
['fallow', extractFallowDerivation()],
];
for (const [label, snippet] of sites) {
const result = runDerivation(repo, snippet, '06');
assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`);
const phaseStart = parseSentinel(result.stdout, 'PHASE_START');
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
const dirFirst = hashes[PHASE06_PLAN_REL];
if (label !== 'fallow') {
assert.deepStrictEqual(
phaseStart,
[dirFirst],
`${label}: anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}`
);
}
const expected = [`${dirFirst}^`];
if (label === 'fallow') {
assert.deepStrictEqual(fallowBase, expected, `${label}: base must be the phase dir first commit's parent`);
} else {
assert.deepStrictEqual(diffBase, expected, `${label}: base must be the phase dir first commit's parent`);
}
}
} finally {
cleanup(repo);
}
}
);
test(
'T2: subject spellings are irrelevant to the directory anchor — unpadded and padded histories resolve identically',
SKIP_WIN32,
() => {
const { repo, hashes } = buildHistory('msd-3503-unpadded-', [
['c1.txt', 'feat(60-01): probe wiring', 'Deferred to Phase 06 per D-09.'],
[PHASE06_PLAN_REL, 'docs(phase-6): capture phase context'],
['c3.txt', 'feat(6-01): implement scanner core'],
['c4.txt', 'test(6): persist human verification items as UAT'],
['c5.txt', 'docs: touch README'],
]);
try {
for (const [label, snippet] of [
['tier3', extractTier3Derivation()],
['spawn_reviewer', extractSpawnReviewerDerivation()],
['fallow', extractFallowDerivation()],
]) {
const result = runDerivation(repo, snippet, '06');
assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`);
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
const expected = [`${hashes[PHASE06_PLAN_REL]}^`];
if (label === 'fallow') {
assert.deepStrictEqual(fallowBase, expected, `${label}: base must be the phase dir first commit's parent`);
} else {
assert.deepStrictEqual(diffBase, expected, `${label}: base must be the phase dir first commit's parent`);
}
}
} finally {
cleanup(repo);
}
}
);
test(
'T3: no committed phase dir fails closed (no silent arbitrary base)',
SKIP_WIN32,
() => {
const { repo } = buildHistory('msd-3503-closed-', [
['c1.txt', 'chore: bump to v2.06.0'],
['c2.txt', 'feat(60-01): probe wiring', 'Deferred to Phase 06 per D-09.'],
['c3.txt', 'docs(06): capture phase context'],
['c4.txt', 'docs: touch README'],
]);
try {
for (const [label, snippet] of [
['tier3', extractTier3Derivation()],
['spawn_reviewer', extractSpawnReviewerDerivation()],
['fallow', extractFallowDerivation()],
]) {
const result = runDerivation(repo, snippet, '06');
assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`);
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
assert.deepStrictEqual(diffBase, [], `${label}: DIFF_BASE must stay empty without a committed phase dir`);
assert.deepStrictEqual(fallowBase, [], `${label}: FALLOW_BASE must stay unset`);
}
} finally {
cleanup(repo);
}
}
);
// #3995: the milestone-blind repro. A PREVIOUS milestone's phase-02 commit
// exists in history with a perfectly anchored subject; the current
// milestone's phase 02 has its own directory. The old derivation's
// unbounded grep + tail -1 selected the archived milestone's commit and
// took a 7-file phase to a 3388-file scope; the directory anchor cannot.
test(
"T4 (#3995): a previous milestone's same-numbered phase commit never captures the base",
SKIP_WIN32,
() => {
const oldMilestonePhase = path.join('.planning', 'milestones', 'v1.1-phases', '02-old', '02-PLAN.md');
const currentPhase = path.join('.planning', 'phases', '02-ctx', '02-PLAN.md');
const { repo, hashes } = buildHistory('msd-3995-milestone-', [
[oldMilestonePhase, 'feat(02-01): research-project command, workflow, and template'],
['mid.txt', 'chore: close milestone v1.1'],
[currentPhase, 'feat(02-01): current milestone phase 02 plan 01'],
['c4.txt', 'docs: touch README'],
]);
try {
for (const [label, snippet] of [
['tier3', extractTier3Derivation()],
['spawn_reviewer', extractSpawnReviewerDerivation()],
['fallow', extractFallowDerivation()],
]) {
const result = runDerivation(repo, snippet, '02');
assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`);
const diffBase = parseSentinel(result.stdout, 'DIFF_BASE');
const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE');
const expected = [`${hashes[currentPhase]}^`];
if (label === 'fallow') {
assert.deepStrictEqual(fallowBase, expected,
`${label}: base must be the CURRENT phase dir's first commit, never the archived milestone's (#3995)`);
} else {
assert.deepStrictEqual(diffBase, expected,
`${label}: base must be the CURRENT phase dir's first commit, never the archived milestone's (#3995)`);
}
}
} finally {
cleanup(repo);
}
}
);
});
// ---------------------------------------------------------------------------
// #4209 Phase 1 Plan 3 (Task 2) — external reviewer evidence consolidation.
// msd-code-reviewer.md must treat <external_reviewer_evidence> as untrusted
// input: independently re-verify every claim against the actual current
// source before it can appear in REVIEW.md, fold a verified claim into the
// SAME Narrative Findings section (no second schema), and never let text
// embedded inside an evidence file act as an instruction. code-review.md's
// EXTERNAL_EVIDENCE_BLOCK must keep restating the four fixed prohibitions.
// ---------------------------------------------------------------------------
describe('CONS-01..03 — external reviewer evidence consolidation (#4209)', () => {
function loadStep(src, stepName) {
const stepStart = src.indexOf(`<step name="${stepName}">`);
assert.ok(stepStart !== -1, `agent must have a ${stepName} step`);
const stepEnd = src.indexOf('</step>', stepStart);
return src.slice(stepStart, stepEnd);
}
test('load_context parses <external_reviewer_evidence> and marks it untrusted', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepSection = loadStep(src, 'load_context');
assert.ok(stepSection.includes('external_reviewer_evidence'),
'load_context must parse the external_reviewer_evidence block');
assert.ok(/untrusted/i.test(stepSection),
'load_context must explicitly mark external reviewer evidence as untrusted data');
});
test('load_context requires independent re-verification against actual source before accepting a claim', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepSection = loadStep(src, 'load_context');
assert.ok(/re-open|reopen/i.test(stepSection) && /re-read/i.test(stepSection),
'load_context must require re-opening and re-reading the actual cited source before accepting an external claim');
assert.ok(/REJECTED|reject/i.test(stepSection),
'load_context must state that an unverifiable external claim is rejected, not included');
});
test('load_context resists prompt injection embedded inside evidence text', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepSection = loadStep(src, 'load_context');
assert.ok(/prompt-injection|prompt injection/i.test(stepSection),
'load_context must name prompt injection as a threat from evidence content');
assert.ok(/never a command|not a command/i.test(stepSection),
'load_context must state evidence text is data, never a command');
});
test('a verified external claim folds into Narrative Findings with no separate schema (CONS-03)', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const stepSection = loadStep(src, 'load_context');
assert.ok(/Narrative Findings/.test(stepSection),
'load_context must route a verified external claim into the existing Narrative Findings section');
const writeReviewSection = loadStep(src, 'write_review');
assert.ok(/external:/.test(writeReviewSection),
'write_review must document the (external: {slug}) provenance tag for a verified external finding');
assert.ok(!/## External/i.test(writeReviewSection),
'write_review must not introduce a separate External Findings section — one REVIEW.md schema only');
});
test('critical_rules restates the untrusted-evidence contract', () => {
const src = fs.readFileSync(REVIEWER_PATH, 'utf8');
const rulesStart = src.indexOf('<critical_rules>');
const rulesEnd = src.indexOf('</critical_rules>');
assert.ok(rulesStart !== -1 && rulesEnd !== -1, 'msd-code-reviewer.md must have a critical_rules section');
const rulesSection = src.slice(rulesStart, rulesEnd);
assert.ok(/external_reviewer_evidence|external reviewer/i.test(rulesSection),
'critical_rules must restate the external-evidence-is-untrusted contract');
});
test('code-review.md restates the four fixed source-review prohibitions when handing off evidence', () => {
const workflowSrc = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const blockStart = workflowSrc.indexOf('EXTERNAL_EVIDENCE_BLOCK=$(printf');
assert.ok(blockStart !== -1, 'code-review.md must build an EXTERNAL_EVIDENCE_BLOCK');
const blockEnd = workflowSrc.indexOf('\n', workflowSrc.indexOf(')', blockStart));
const blockText = workflowSrc.slice(blockStart, blockEnd);
for (const prohibition of ['no source mutation', 'no test execution', 'no background processes', 'no active polling']) {
assert.ok(blockText.includes(prohibition),
`EXTERNAL_EVIDENCE_BLOCK must restate "${prohibition}" (SAFE-03..06)`);
}
});
});
// #3829 — execute-phase `code_review_gate`: surface the severity counts it
// already parses, and record a per-finding disposition.
//
// Before this change the gate extracted `status:` from REVIEW.md's frontmatter,
// discarded the `critical`/`warning`/`info`/`total` values sitting in the same
// `sed` range, and printed a message that was byte-identical for a review with
// one `info` finding and a review with a Critical. Nothing recorded what
// happened to any finding, so a phase reached `phase.complete` with Criticals
// standing and no trace they had been seen.
//
// Tested the way this file tests every other embedded parser: a pure-JS mirror
// of the shipped `node -e` script (validated against the shipped block during
// implementation), plus docs-parity assertions on the workflow .md text, which
// is itself the deployed contract.
// ---------------------------------------------------------------------------
// Mirror of the gate's frontmatter scalar reads. The shipped block strips CR, then takes ONLY
// the first `---` ... `---` block — it does NOT use a sed range, because a sed range re-opens on
// a body `---` and runs to EOF, which leaks body lines into an OPTIONAL key's read. This mirror
// must model that extraction, not a whole-document scan: a mirror that scans the document passes
// on a fixture the shipped code fails, which is the drift this comment exists to prevent.
function parseGateCounts(reviewText) {
const lines = String(reviewText).replace(/\r/g, '').split('\n');
// Only a CLOSED frontmatter block counts: an unterminated one must yield nothing rather than
// hand the whole review body to the reads below.
let fm = [];
if (lines[0] === '---') {
const buf = [];
let closed = false;
for (let i = 1; i < lines.length; i++) {
if (lines[i] === '---') { closed = true; break; }
buf.push(lines[i]);
}
if (closed) fm = buf;
}
// The shipped reads are `grep -m1 <key> | cut -d: -f2- | sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//'`,
// and BOTH stages matter. This mirror previously modelled `cut -d: -f2 | tr -d ' '`, which REPAIRED a
// malformed scalar into a number twice over -- `tr -d` deleting INTERNAL spaces (`1 0` -> `10`) and
// `-f2` keeping only the second colon-field (`1: junk` -> `1`). Both were retired from the shipped
// reads when an adversarial pass showed a repaired number deciding whether a shortfall was reported.
// `-f2-` keeps the whole scalar and only the ends are trimmed. The class below is POSIX
// [[:space:]] AS THE C LOCALE DEFINES IT -- space, tab, newline, VT, FF, CR -- spelled out
// literally rather than as JS `\s`, which also matches the unicode spaces C does not. That is an
// equivalence, not an approximation, ONLY because the shipped reads pin LC_ALL=C: glibc's C.UTF-8
// classifies U+2003 as [[:space:]] and as [[:blank:]], so an unpinned shipped read trimmed a
// character this mirror keeps, and the divergence was invisible here because both sides still
// landed on the same arm for every fixture that existed. The anchors below are spelled with the
// same literal class, for the same reason -- `\s` there would match a U+2003 indent the pinned
// shipped grep does not.
const TRIM = /^[ \t\n\v\f\r]+|[ \t\n\v\f\r]+$/g;
const first = (re) => {
for (const line of fm) {
const m = line.match(re);
if (m) return line.slice(line.indexOf(':') + 1).replace(TRIM, '');
}
return '';
};
// The four counts belong to the `findings:` MAPPING, so the mirror scopes to it exactly as the
// shipped awk does: select the findings: block, stop at the next column-0 key. Without this a
// top-level key later named `total:` / `info:` / `critical:` is picked up ahead of the nested
// one. `status:` stays anchored at column 0 because it IS top-level.
const findingsBlock = [];
{
let inBlock = false;
for (const line of fm) {
// The BLOCK BOUNDARY takes the literal class too, and it is not decoration: the shipped
// selector is an `awk` whose `/^findings:[[:space:]]*$/` and `/^[^[:space:]]/` resolve
// through the ambient locale exactly as grep's and sed's did. Pass 6 found the awk had been
// left unpinned while grep and sed were fixed -- `findings:<U+2003>` opened the block under
// C.UTF-8 and did not under C, so the SAME review reported a breakdown on one machine and
// the countless message on another. `\s`/`\S` here would model neither pinned side.
if (/^findings:[ \t\n\v\f\r]*$/.test(line)) { inBlock = true; continue; }
if (inBlock && /^[^ \t\n\v\f\r]/.test(line)) break;
if (inBlock) findingsBlock.push(line);
}
}
// SAME pipeline as `first` above, and the duplication is the point: BOTH helpers model the shipped
// reads, and the counts go through THIS one. A previous edit updated `first` and the comment above
// it while leaving this body on the retired `split(':')[1].replace(/ /g,'')` — the shipped block had
// moved to `-f2-` + end-trim and the mirror had not, which the parity fixtures could not see because
// both parsers landed on the countless arm for every fixture that then existed. The
// `a self-consistent repaired breakdown` fixture below is what separates them.
const firstIn = (lines, re) => {
for (const line of lines) {
const m = line.match(re);
if (m) return line.slice(line.indexOf(':') + 1).replace(TRIM, '');
}
return '';
};
return {
status: first(/^status:(.*)$/),
critical: firstIn(findingsBlock, /^[ \t\n\v\f\r]*(?:critical|blocker):(.*)$/),
warning: firstIn(findingsBlock, /^[ \t\n\v\f\r]*warning:(.*)$/),
info: firstIn(findingsBlock, /^[ \t\n\v\f\r]*info:(.*)$/),
total: firstIn(findingsBlock, /^[ \t\n\v\f\r]*total:(.*)$/),
};
}
// ── The SHIPPED disposition builder, executed — not modelled ──────────────────
//
// Rounds 1 and 2 of the pre-filing review both refuted "the mirrors are faithful": a
// hand-written model of a shell-embedded script drifts, and when it drifts the behavioural
// tests pass while the shipped block is broken. Mutation-testing confirmed it — deleting the
// carried-row logic from the shipped file turned nothing red.
//
// So the disposition tests below run the ACTUAL script. It is pure Node (every input arrives
// through process.env), so extracting it and executing it needs no shell and works on Windows.
// The only transformation is undoing the two shell escapes the surrounding double-quoted
// `node -e "…"` string requires: \` -> ` and \$ -> $.
function shippedDispositionScript() {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8').replace(/\r\n/g, '\n');
const open = src.indexOf('node -e "');
assert.ok(open !== -1, 'the disposition step must still embed a node -e script');
const body = src.slice(open + 'node -e "'.length);
const end = body.indexOf('\n" || echo ');
assert.ok(end !== -1, 'the node -e script must still be closed by its || echo fallback');
// Undo exactly what the surrounding double-quoted shell string does, in ONE left-to-right
// pass: inside "..." a backslash is special only before $ ` " or \\, and everything else is
// literal. Doing these as separate passes (or missing \\\\ -> \\) silently hands the test a
// DIFFERENT regex from the one that ships — which is how an escaped-pipe case passed here
// while failing in the shell.
return body.slice(0, end).replace(/\\([\\$`"])/g, '$1');
}
// Run the shipped script against a temp phase dir and return the ledger it wrote (or null).
function runShippedDisposition({ reviewText, priorText, fixText, iterFixText, padded = '01', reviewTotal }) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3829-'));
try {
const reviewPath = path.join(dir, padded + '-REVIEW.md');
const dispPath = path.join(dir, padded + '-REVIEW-DISPOSITION.md');
const fixPath = path.join(dir, padded + '-REVIEW-FIX.md');
fs.writeFileSync(reviewPath, reviewText);
if (priorText !== undefined) fs.writeFileSync(dispPath, priorText);
if (fixText !== undefined) fs.writeFileSync(fixPath, fixText);
// The --auto loop's per-iteration backups (<NN>-REVIEW-FIX.iterN.md). Keyed by iteration
// number so a test can drive the ordering the script relies on (newest wins).
for (const [n, text] of Object.entries(iterFixText || {})) {
fs.writeFileSync(path.join(dir, padded + '-REVIEW-FIX.iter' + n + '.md'), text);
}
// Bounded by construction via the process seam — an unbounded spawn is an indefinite hang.
const res = runNode(['-e', shippedDispositionScript()], {
timeoutMs: PROBE_TIMEOUT_MS,
env: {
...process.env,
REVIEW_FILE: reviewPath,
DISPOSITION_FILE: dispPath,
FIX_REPORT_FILE: fixPath,
PADDED: padded,
// The frontmatter total block 2 derives and hands to the script, so the two parsers can
// be reconciled. Passed through here so a test can drive the shortfall path.
REVIEW_TOTAL: reviewTotal === undefined ? '' : String(reviewTotal),
},
});
assert.strictEqual(res.outcome, OUTCOME.EXITED, 'the shipped script must run to completion');
assert.strictEqual(res.exitCode, 0, 'the shipped script must exit 0: ' + res.stderr);
const ledger = fs.existsSync(dispPath) ? fs.readFileSync(dispPath, 'utf8') : null;
return { ledger, stdout: res.stdout, wroteNothing: /disposition unchanged/.test(res.stdout) };
} finally {
cleanup(dir);
}
}
// Parse the rows out of a rendered ledger, so assertions read against real output.
function ledgerRows(ledger) {
if (ledger === null) return null;
const rows = [];
for (const line of ledger.split('\n')) {
const m = line.match(/^\|\s*((?:CR|BL|WR|IN)-\d+)\s*\|\s*([a-z]+)\s*\|\s*([a-z]+)\s*\|\s*(.*?)\s*\|$/);
if (m) rows.push({ id: m[1], severity: m[2], disposition: m[3], source: m[4] });
}
return rows;
}
// buildDisposition is NOT a mirror any more — it drives the SHIPPED script.
//
// Three pre-filing review rounds each refuted "the mirror is faithful", and mutation testing
// confirmed the cost: a hand-written model of a shell-embedded script drifts, and when it drifts
// the tests pass while the shipped block is broken. The model is gone; this adapter runs the real
// thing and returns the same shape the assertions below already expect.
function buildDisposition({ reviewText, priorText, fixText, padded = '01', reviewTotal }) {
const rows = ledgerRows(runShippedDisposition({ reviewText, priorText, fixText, padded, reviewTotal }).ledger);
if (rows === null) return null;
return {
rows: rows.map((r) => ({
...r,
carried: / \(not in the current review\)$/.test(r.source) || undefined,
source: r.source.replace(/ \(not in the current review\)$/, ''),
})),
open: rows.filter((r) => r.disposition === 'open').length,
total: rows.length,
};
}
const REVIEW_WITH_FINDINGS = [
'---',
'phase: 01',
'findings:',
' critical: 1',
' warning: 2',
' info: 1',
' total: 4',
'status: issues_found',
'---',
'',
'## Critical Issues',
'',
'### CR-01: SQL injection in auth',
'',
'## Warnings',
'',
'### WR-01: missing null check',
'',
'### WR-02: unused import',
'',
'## Info',
'',
'### IN-01: stale TODO',
].join('\n');
describe('#3829 — code_review_gate severity surfacing', () => {
test('the gate reads the counts that sit beside the status it already parsed', () => {
const parsed = parseGateCounts(REVIEW_WITH_FINDINGS);
assert.deepStrictEqual(parsed, {
status: 'issues_found', critical: '1', warning: '2', info: '1', total: '4',
});
});
test('blocker: is accepted as the Critical tier-equivalent of critical:', () => {
const parsed = parseGateCounts(REVIEW_WITH_FINDINGS.replace(' critical: 1', ' blocker: 1'));
assert.strictEqual(parsed.critical, '1');
});
test('a status:/total: line in the review BODY never displaces the frontmatter value', () => {
// The `sed -n '/^---$/,/^---$/p'` range re-opens on a body `---` and runs to
// EOF, so first-match (`grep -m1`) is what makes this correct — not the range.
const poisoned = REVIEW_WITH_FINDINGS + '\n\n---\n\nstatus: clean\ntotal: 999\n';
const parsed = parseGateCounts(poisoned);
assert.strictEqual(parsed.status, 'issues_found');
assert.strictEqual(parsed.total, '4');
});
test('a REVIEW.md with no findings: block yields no counts, so the gate can fall back', () => {
const legacy = ['---', 'phase: 02', 'status: issues_found', '---', '', '# Phase 02'].join('\n');
const parsed = parseGateCounts(legacy);
assert.strictEqual(parsed.status, 'issues_found');
assert.strictEqual(parsed.total, '');
assert.strictEqual(parsed.critical, '');
});
test('docs-parity: the gate states the counts rather than the countless message alone', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes('Code review: ${REVIEW_TOTAL} findings — ${REVIEW_CRITICAL} critical, ${REVIEW_WARNING} warning, ${REVIEW_INFO} info.'),
'code_review_gate must display the per-severity breakdown it parsed'
);
});
test('docs-parity: every frontmatter scalar read carries a first-match guard', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes('grep -m1 "^status:"'),
'the status: read must keep its single-match guard (DEFECT.FRONTMATTER-SCALAR-BROAD-GREP)'
);
});
});
describe('#3829 — code_review_gate per-finding disposition record', () => {
test('every finding is recorded, defaulting to open', () => {
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, padded: '01' });
assert.strictEqual(d.total, 4);
assert.strictEqual(d.open, 4);
assert.deepStrictEqual(d.rows.map((r) => r.id), ['CR-01', 'WR-01', 'WR-02', 'IN-01']);
assert.deepStrictEqual(d.rows.map((r) => r.severity), ['critical', 'warning', 'warning', 'info']);
});
test('BL- findings are recorded at the Critical tier alongside CR-', () => {
const d = buildDisposition({
reviewText: REVIEW_WITH_FINDINGS.replace('### CR-01:', '### BL-01:'),
padded: '01',
});
assert.strictEqual(d.rows[0].id, 'BL-01');
assert.strictEqual(d.rows[0].severity, 'critical');
});
test('--fix outcomes are reconciled from REVIEW-FIX.md with provenance', () => {
const fixText = [
'# Phase 01: Code Review Fix Report',
'',
'## Fixed Issues',
'',
'### WR-01: missing null check',
'',
'## Skipped Issues',
'',
'### WR-02: unused import',
].join('\n');
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, fixText, padded: '01' });
const byId = Object.fromEntries(d.rows.map((r) => [r.id, r]));
assert.strictEqual(byId['WR-01'].disposition, 'fixed');
assert.strictEqual(byId['WR-01'].source, '01-REVIEW-FIX.md');
assert.strictEqual(byId['WR-02'].disposition, 'skipped');
assert.strictEqual(byId['CR-01'].disposition, 'open');
assert.strictEqual(d.open, 2);
});
test('a finding heading outside the Fixed/Skipped sections is not a disposition', () => {
const fixText = [
'## Fixed Issues',
'',
'### WR-01: missing null check',
'',
'## Verification',
'',
'### IN-01: mentioned while describing how the fix was verified',
].join('\n');
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, fixText, padded: '01' });
const byId = Object.fromEntries(d.rows.map((r) => [r.id, r]));
assert.strictEqual(byId['WR-01'].disposition, 'fixed');
assert.strictEqual(byId['IN-01'].disposition, 'open');
});
test('a recorded decision survives a re-run — open never overwrites deferred', () => {
const priorText = [
'| Finding | Severity | Disposition | Source |',
'|---------|----------|-------------|--------|',
'| CR-01 | critical | deferred | ships next phase |',
'| WR-01 | warning | open | - |',
].join('\n');
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, priorText, padded: '01' });
const byId = Object.fromEntries(d.rows.map((r) => [r.id, r]));
assert.strictEqual(byId['CR-01'].disposition, 'deferred');
// The source cell carries the human's stated reason and is preserved, not replaced.
assert.strictEqual(byId['CR-01'].source, 'ships next phase');
assert.strictEqual(d.open, 3);
});
test('an applied --fix outcome wins over an earlier deferral', () => {
const priorText = '| CR-01 | critical | deferred | later |';
const fixText = ['## Fixed Issues', '', '### CR-01: SQL injection in auth'].join('\n');
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, priorText, fixText, padded: '01' });
assert.strictEqual(d.rows[0].disposition, 'fixed');
});
test('a review with no finding headings produces no record at all', () => {
const legacy = ['---', 'phase: 02', 'status: issues_found', '---', '', 'prose only'].join('\n');
assert.strictEqual(buildDisposition({ reviewText: legacy, padded: '02' }), null);
});
test('docs-parity: the gate writes a REVIEW-DISPOSITION sibling, not into REVIEW.md', () => {
// Eighth src.includes() converted. It pinned the literal `${PHASE_DIR}` interpolation, so it
// went red when path construction moved to a VALIDATED local -- while the property it names,
// "a REVIEW-DISPOSITION sibling", was untouched. Assert the property: the ledger lands beside
// the review, under the derived name, and REVIEW.md itself is not written.
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-sib-'));
try {
const reviewPath = path.join(dir, '02-REVIEW.md');
const reviewText = ['---', 'status: issues_found', '---', '', '### CR-01: a finding'].join('\n');
fs.writeFileSync(reviewPath, reviewText);
const res = runNode(['-e', shippedDispositionScript()], {
timeoutMs: PROBE_TIMEOUT_MS,
env: {
...process.env,
REVIEW_FILE: reviewPath,
DISPOSITION_FILE: path.join(dir, '02-REVIEW-DISPOSITION.md'),
FIX_REPORT_FILE: path.join(dir, '02-REVIEW-FIX.md'),
PADDED: '02',
},
});
assert.strictEqual(res.exitCode, 0, res.stderr);
assert.ok(fs.existsSync(path.join(dir, '02-REVIEW-DISPOSITION.md')),
'code_review_gate must write the disposition record to a REVIEW-DISPOSITION sibling');
assert.strictEqual(fs.readFileSync(reviewPath, 'utf8'), reviewText,
'and must not write into REVIEW.md — msd-code-reviewer is its single writer');
} finally {
cleanup(dir);
}
// Sixth src.includes() converted. It pinned the exact SOURCE LINE of the enumeration loop, so
// it went red when M3 reformatted that loop to track the enclosing section -- while the
// property it names, "enumerate every finding ID", was strictly widened rather than broken.
// A pin on a code shape reports every refactor as a regression and no regression as one.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: a', '### WR-01: b', '### IN-01: c',
'### CR-01: a duplicate id, which must not produce a second row'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review }).ledger);
assert.deepStrictEqual(
rows.map((r) => r.id), ['CR-01', 'WR-01', 'IN-01'],
'code_review_gate must enumerate every finding ID from REVIEW.md, in order, once each'
);
});
test('docs-parity: code_review_gate actually reaches the extracted step', () => {
// The step is only reachable because the parent says to read and execute it. Without this,
// every other docs-parity assertion here could pass against a file nothing loads.
const parent = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf8');
assert.ok(
/Read and execute\s+`msd-core\/workflows\/execute-phase\/steps\/code-review-disposition\.md`/.test(parent),
'code_review_gate must read and execute the disposition step'
);
assert.ok(
parent.indexOf('code-review-disposition.md') < parent.indexOf('**TDD review escalation'),
'and must do so before the TDD escalation can stop the phase'
);
});
test('docs-parity: the disposition write is non-blocking', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes('Code review disposition record skipped (non-blocking).'),
'the disposition write must never block execution flow'
);
});
});
// ---------------------------------------------------------------------------
// #3829 review round — the seven defects a cross-AI adversarial pass on the fix
// diff refuted before filing. Each is pinned here so the round's work survives.
// ---------------------------------------------------------------------------
describe('#3829 review round — frontmatter scoping, section anchoring, ledger durability', () => {
test('an OPTIONAL count absent from frontmatter is not supplied by a leaked body value', () => {
// A sed range re-opens on a body `---` and runs to EOF. First-match protects a key the
// frontmatter DOES carry; it cannot protect one it does not. Extraction must stop at the
// closing delimiter, or a body `total:` becomes the reported finding count.
const leaky = [
'---', 'phase: 01', 'status: issues_found', '---',
'', '# Report', '', '---', '', 'total: 7', 'critical: 9',
].join('\n');
const parsed = parseGateCounts(leaky);
assert.strictEqual(parsed.status, 'issues_found');
assert.strictEqual(parsed.total, '', 'a body total: must not become the finding count');
assert.strictEqual(parsed.critical, '', 'a body critical: must not become the critical count');
});
test('a CRLF-authored review parses to bare values, with no carriage return riding along', () => {
const crlf = [
'---', 'phase: 01', 'findings:', ' critical: 1', ' warning: 0', ' info: 2',
' total: 3', 'status: issues_found', '---', '', '### CR-01: a',
].join('\r\n');
const parsed = parseGateCounts(crlf);
assert.deepStrictEqual(parsed, {
status: 'issues_found', critical: '1', warning: '0', info: '2', total: '3',
});
for (const v of Object.values(parsed)) assert.ok(!/\r/.test(v), 'no value may carry a CR');
});
test('docs-parity: the gate stops at the closing delimiter and strips CR before parsing', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes(`awk 'NR==1{if($0!="---") exit; next} /^---$/{closed=1; exit}`),
'the gate must extract only the FIRST frontmatter block, not a re-opening sed range'
);
assert.ok(
src.includes(`tr -d '\\r' < "$REVIEW_FILE"`),
'the gate must strip CR so a CRLF review cannot inject one into the message'
);
});
test('a section heading that merely STARTS with "Fixed Issues" does not classify findings', () => {
const fixText = [
'# Fix report', '', '## Fixed Issues Verification', '', '### IN-01: named while verifying',
].join('\n');
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, fixText, padded: '01' });
const byId = Object.fromEntries(d.rows.map((r) => [r.id, r]));
assert.strictEqual(byId['IN-01'].disposition, 'open');
});
test('a deferral reason written into the Source cell survives a re-run verbatim', () => {
const priorText = '| CR-01 | critical | deferred | ships next phase, see ADR-99 |';
const d = buildDisposition({ reviewText: REVIEW_WITH_FINDINGS, priorText, padded: '01' });
const cr = d.rows.find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred');
assert.strictEqual(cr.source, 'ships next phase, see ADR-99');
});
test('a decided finding the current review no longer reports is carried, not dropped', () => {
// --auto re-reviews and rewrites REVIEW.md; a fixed or deferred finding can vanish from it.
// Dropping the row would erase the record that it was seen — the exact failure #3829 is about.
const priorText = [
'| CR-01 | critical | deferred | ships next phase |',
'| IN-01 | info | open | - |',
].join('\n');
const shrunk = ['---', 'status: issues_found', '---', '', '### WR-01: still here'].join('\n');
const d = buildDisposition({ reviewText: shrunk, priorText, padded: '01' });
const byId = Object.fromEntries(d.rows.map((r) => [r.id, r]));
assert.ok(byId['CR-01'], 'a deferred finding absent from the review must still be recorded');
assert.strictEqual(byId['CR-01'].disposition, 'deferred');
assert.strictEqual(byId['CR-01'].carried, true);
assert.strictEqual(byId['CR-01'].source, 'ships next phase');
// M1: the untriaged row is carried TOO, and marked. Dropping it erased the record that the
// finding was ever seen -- which is #3829's complaint verbatim, reproduced by the artifact
// built to prevent it. 'Nothing was decided about it' is precisely the state that must leave
// a trace. The marker keeps it honest: the row does not claim the finding is live.
assert.ok(byId['IN-01'], 'an untriaged row for a vanished finding is carried, not deleted');
assert.strictEqual(byId['IN-01'].disposition, 'open');
assert.strictEqual(byId['IN-01'].carried, true);
});
test('the carried marker is rendered, never stored — it cannot accumulate across runs', () => {
// The marker lives in the Source cell, which is re-parsed on the next run. Storing it would
// re-append it every time: the cell grows without bound AND the file changes on every run,
// which silently defeats the unchanged-run check and restores the empty-docs-commit churn.
const shrunk = ['---', 'status: issues_found', '---', '', '### WR-01: b'].join('\n');
let priorText = '| CR-01 | critical | deferred | ships next phase |';
let cr;
for (let i = 0; i < 3; i++) {
const d = buildDisposition({ reviewText: shrunk, priorText, padded: '01' });
cr = d.rows.find((r) => r.id === 'CR-01');
// Re-render the row the way the shipped builder does, and feed it back in.
priorText = '| ' + cr.id + ' | ' + cr.severity + ' | ' + cr.disposition + ' | ' +
cr.source + (cr.carried ? ' (not in the current review)' : '') + ' |';
}
assert.strictEqual(cr.source, 'ships next phase', 'the stored source must stay canonical');
assert.strictEqual(
(priorText.match(/\(not in the current review\)/g) || []).length, 1,
'the marker must appear exactly once no matter how many times the gate re-runs'
);
});
test('docs-parity: the ledger is rewritten only on a real change, timestamp excluded', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes("const stripTs = (t) => t.replace(/^recorded:.*\\$/m, 'recorded:');"),
'the gate must compare ignoring the timestamp so an unchanged run writes nothing'
);
assert.ok(
src.includes('Code review disposition unchanged: '),
'an unchanged run must say so rather than producing an empty docs commit'
);
});
test('docs-parity: section headings are anchored whole and the prior source cell is captured', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes('/^##\\s+Fixed Issues\\s*\\$/') && src.includes('/^##\\s+Skipped Issues\\s*\\$/'),
'fix-report section headings must be matched whole, never by prefix'
);
// Eighth src.includes() converted -- it pinned the exact source-cell CAPTURE, and that
// capture was the round-3 defect (a bare | failed the whole match). The property it named
// is asserted behaviourally below: 'an escaped pipe in a deferral reason survives whole'
// and 'a bare pipe in a deferral reason is kept as prose'.
// Seventh src.includes() converted -- it pinned the exact comparison EXPRESSION, so it went
// red when the comparison gained whitespace normalization while the property it names was
// unchanged. Assert the property instead: a fix report naming a different finding under a
// reused id does not decide the row.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a new finding'].join('\n');
const stale = ['## Fixed Issues', '', '### CR-01: what this id used to mean'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review, fixText: stale }).ledger);
assert.strictEqual(rows[0].disposition, 'open',
'a fix report must name the SAME finding before its outcome is applied');
});
});
describe('#3861 round 2 — the disposition ledger is reachable in a shipped path (B1c/B1d)', () => {
const FIX_WORKFLOW = path.join(ROOT, 'msd-core', 'workflows', 'code-review-fix.md');
test('code-review-fix.md reads and executes the disposition step after committing the fix report', () => {
// Without this the reconciliation logic -- the bulk of the step and nearly all of its test
// surface -- is reachable only on a RE-EXECUTION of the phase, and REQ-REVIEW-10 is unmet in
// every shipped path. execute-phase.md's gate invokes review with neither --fix nor --auto,
// so REVIEW-FIX.md cannot exist there and every row it writes is `open` by construction.
const src = fs.readFileSync(FIX_WORKFLOW, 'utf8');
assert.match(src, /<step name="record_disposition">/,
'the fix workflow must carry a step that records the disposition');
assert.match(src, /msd-core\/workflows\/execute-phase\/steps\/code-review-disposition\.md/,
'and it must invoke the SAME step, not a second copy of the logic');
// Ordering is load-bearing: the fix report must be on disk before the ledger claims anything
// about it. Assert the step positions rather than merely their presence.
const commitAt = src.indexOf('<step name="commit_fix_report">');
const recordAt = src.indexOf('<step name="record_disposition">');
const presentAt = src.indexOf('<step name="present_results">');
assert.ok(commitAt > -1 && recordAt > -1 && presentAt > -1, 'all three steps must exist');
assert.ok(commitAt < recordAt, 'the ledger is reconciled AFTER the fix report is written');
assert.ok(recordAt < presentAt, 'and before results are presented');
});
test('the iteration backups are removed AFTER the ledger has read them, not inside the loop', () => {
// #3861 round 5. The .iterN.md backups are the only surviving record of what an earlier --auto
// iteration fixed -- this workflow keeps ONE final version of each artifact, and the re-review
// drops a finding once it is fixed, so neither final artifact carries it. Deleting them at the
// end of the loop meant record_disposition reached a CONVERGED run with every early fix already
// erased and recorded those findings as open.
const src = fs.readFileSync(FIX_WORKFLOW, 'utf8');
const recordAt = src.indexOf('<step name="record_disposition">');
const cleanupAt = src.indexOf('<step name="cleanup_iteration_backups">');
assert.ok(cleanupAt > -1, 'the backups must be removed by a named step, not inline in the loop');
assert.ok(recordAt < cleanupAt, 'the ledger reads the backups BEFORE they are removed');
// And the removal must not have been left behind in the loop as well.
const loopAt = src.indexOf('<step name="auto_iteration_loop">');
const loopBody = src.slice(loopAt, src.indexOf('<step name="commit_fix_report">'));
assert.ok(!/rm -f "\$\{REVIEW_PATH%\.md\}\.iter"/.test(loopBody),
'the loop must no longer delete the backups it just wrote');
});
test('the reconciliation is reachable: gate writes all-open, the fix path resolves it', () => {
// The two call sites driven in sequence, which is the shipped order. This is the review's own
// input -> wrong output case, inverted: a phase whose findings are all fixed by a subsequent
// --fix run used to end at `open: N / total: N`, asserting that every triaged finding was
// forgotten -- worse than recording nothing, because it looks authoritative and is inverted.
const review = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 1', ' info: 0', ' total: 2', '---', '',
'## Critical Issues', '', '### CR-01: the critical one', '',
'## Warnings', '', '### WR-01: the warning one'].join('\n');
const fixReport = ['---', 'status: complete', '---', '',
'## Fixed Issues', '', '### CR-01: the critical one', '',
'## Skipped Issues', '', '### WR-01: the warning one'].join('\n');
// Call site 1 -- execute-phase.md's code_review_gate. No fix report exists yet.
const gate = runShippedDisposition({ reviewText: review, reviewTotal: 2 });
const gateRows = ledgerRows(gate.ledger);
assert.deepStrictEqual(gateRows.map((r) => r.disposition), ['open', 'open'],
'at the gate there is no fix report, so every row is open -- correctly');
// Call site 2 -- code-review-fix.md's record_disposition, with the report on disk and the
// gate's ledger carried in as prior state.
const after = runShippedDisposition({
reviewText: review, priorText: gate.ledger, fixText: fixReport, reviewTotal: 2,
});
const rows = ledgerRows(after.ledger);
assert.deepStrictEqual(
rows.map((r) => [r.id, r.disposition]),
[['CR-01', 'fixed'], ['WR-01', 'skipped']],
'the fix outcomes reach the ledger'
);
assert.match(after.ledger, /^open: 0$/m, 'and the headline agrees');
assert.match(after.ledger, /^total: 2$/m);
});
});
describe('#3861 round 2 — severity comes from the section, not just the id prefix (M3)', () => {
const sevOf = (reviewText, id) => {
const rows = ledgerRows(runShippedDisposition({ reviewText }).ledger);
const row = rows.find((r) => r.id === id);
assert.ok(row, id + ' must have a row');
return row.severity;
};
test("a Critical mis-numbered as WR- is recorded critical when it sits under '## Critical Issues'", () => {
// The section heading is the reviewer's OWN statement of severity, and the walker already
// visits it. Deriving from the prefix alone puts the ledger's Severity column -- the whole
// basis for triaging it -- in disagreement with the review it summarizes and with the
// frontmatter count line block 1 prints.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Critical Issues', '', '### CR-01: properly numbered', '',
'### WR-04: a critical the reviewer mis-numbered', '',
'## Info', '', '### IN-01: an info item'].join('\n');
assert.strictEqual(sevOf(review, 'WR-04'), 'critical', 'the section outranks the prefix');
assert.strictEqual(sevOf(review, 'CR-01'), 'critical');
assert.strictEqual(sevOf(review, 'IN-01'), 'info');
});
test('an IN- finding under ## Warnings is recorded warning', () => {
// The other direction, so the rule is not one-way: the section governs whichever way the
// prefix disagrees with it.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Warnings', '', '### IN-02: mis-numbered the other way'].join('\n');
assert.strictEqual(sevOf(review, 'IN-02'), 'warning');
});
test('the id prefix still governs when no recognized section encloses the finding', () => {
// Fallback control. A review that does not use the documented headings -- and every carried
// row from an earlier review -- must keep the prefix mapping, BL- included.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: a', '### BL-02: b', '### WR-03: c', '### IN-04: d'].join('\n');
assert.strictEqual(sevOf(review, 'CR-01'), 'critical');
assert.strictEqual(sevOf(review, 'BL-02'), 'critical', 'BL- stays Critical-tier-equivalent');
assert.strictEqual(sevOf(review, 'WR-03'), 'warning');
assert.strictEqual(sevOf(review, 'IN-04'), 'info');
});
test('a lookalike section heading does not re-tier the findings under it', () => {
// Matched WHOLE, exactly as the fix-report sections are. A prefix match would let
// '## Critical Issues Verification' promote everything beneath it.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Critical Issues Verification', '', '### IN-05: not actually critical'].join('\n');
assert.strictEqual(sevOf(review, 'IN-05'), 'info', 'an unrecognized section falls back to the prefix');
});
test('a section heading inside a fenced example does not govern', () => {
// The fence walker already skips fenced content; this pins that the new section tracking
// honours it rather than reading an illustration as document structure.
const review = ['---', 'phase: 01', 'status: issues_found', '---', '',
'```markdown', '## Critical Issues', '```', '', '### IN-06: outside the fence'].join('\n');
assert.strictEqual(sevOf(review, 'IN-06'), 'info');
});
});
describe('#3861 round 11 — a carried row keeps the severity the ledger recorded', () => {
// The ledger always WROTE a severity (table cell + frontmatter key) and nothing read it back:
// the row regex skipped the cell as [^|]*, the frontmatter walk collected only titles, and a
// carried row was rebuilt from the id prefix because sectionSev holds only the CURRENT review's
// findings. So the one case round 2's M3 fix exists for -- a Critical the reviewer mis-numbered
// WR-04 -- was recorded critical on run 1 and silently re-recorded warning on run 2, once the
// review stopped reporting it. Every carried-row test used CR-01/IN-01, whose prefix already
// matched, so the fallback returned the right answer by coincidence and no mutation could tell
// the two paths apart. Reproduced by the round-11 review by executing the shipped script twice.
const REVIEW_1 = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Critical Issues', '', '### CR-01: properly numbered', '',
'### WR-04: a critical the reviewer mis-numbered', ''].join('\n');
// Run 2: the review no longer reports WR-04 at all.
const REVIEW_2_DROPPED = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Critical Issues', '', '### CR-01: properly numbered', ''].join('\n');
const rowOf = (ledger, id) => {
const row = ledgerRows(ledger).find((r) => r.id === id);
assert.ok(row, id + ' must have a row');
return row;
};
// Run 1's ledger, with WR-04 deferred BY HAND, the way the ledger's own legend instructs. Edited
// in place so the frontmatter (and its title) is present -- a bare row would carry no title and
// sameFinding() would pass through its back-compat arm, never consulting identity at all.
const deferredLedger = () => {
const first = runShippedDisposition({ reviewText: REVIEW_1 }).ledger;
assert.strictEqual(rowOf(first, 'WR-04').severity, 'critical', 'run 1 records the section severity');
const edited = first.replace(/^\| WR-04 \| critical \| open \| - \|$/m, '| WR-04 | critical | deferred | waiting on team A |');
assert.notStrictEqual(edited, first, 'the hand edit must have landed');
return edited;
};
test("a deferred WR-* filed under '## Critical Issues' stays critical once the review stops reporting it", () => {
const after = runShippedDisposition({ reviewText: REVIEW_2_DROPPED, priorText: deferredLedger() });
const row = rowOf(after.ledger, 'WR-04');
assert.strictEqual(row.severity, 'critical', 'the recorded severity survives the carry -- the prefix would say warning');
assert.strictEqual(row.disposition, 'deferred', 'and so does the decision');
assert.match(row.source, /^waiting on team A/, 'and the reason');
assert.match(row.source, /\(not in the current review\)$/, 'and the row is marked carried');
assert.match(after.ledger, /^ {4}severity: critical$/m, 'the frontmatter agrees with the table');
});
test('the recorded severity also outranks the prefix while the review still reports the finding under no recognized section', () => {
// A re-review that dropped the documented headings. The current review gives no section, the
// ledger remembers what the last one said, and that is a better source than the id alone.
const plain = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: properly numbered', '### WR-04: a critical the reviewer mis-numbered'].join('\n');
const after = runShippedDisposition({ reviewText: plain, priorText: deferredLedger() });
assert.strictEqual(rowOf(after.ledger, 'WR-04').severity, 'critical');
});
test("the current review's section still wins over the recorded value", () => {
// Precedence control: a reviewer who re-tiers a finding on re-review is the newer statement.
const retiered = ['---', 'phase: 01', 'status: issues_found', '---', '',
'## Critical Issues', '', '### CR-01: properly numbered', '',
'## Warnings', '', '### WR-04: a critical the reviewer mis-numbered'].join('\n');
const after = runShippedDisposition({ reviewText: retiered, priorText: deferredLedger() });
assert.strictEqual(rowOf(after.ledger, 'WR-04').severity, 'warning', 'the section is the current statement');
});
test('a REUSED id does not inherit the old finding\'s severity', () => {
// Identity control, the same rule the disposition takes: ids are reused across re-reviews, so
// a brand-new WR-04 under no section starts from its own prefix, not from the finding it
// replaced. Inheriting here would put a Critical tier on an ordinary warning.
const reused = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: properly numbered', '### WR-04: an entirely different finding'].join('\n');
const after = runShippedDisposition({ reviewText: reused, priorText: deferredLedger() });
const row = rowOf(after.ledger, 'WR-04');
assert.strictEqual(row.severity, 'warning', 'a different finding under a reused id is inferred from its prefix');
assert.strictEqual(row.disposition, 'open', 'and, as before, does not inherit the decision either');
});
test('a hand-mangled Severity cell falls back to the frontmatter copy, and a mangled pair to the prefix', () => {
// Both sources are enum-validated (ADR-227): a value outside critical|warning|info is not a
// severity. The table is read first because it is the surface a human edits; the frontmatter
// is the copy a human is not invited to touch.
const base = deferredLedger();
const cellMangled = base.replace('| WR-04 | critical | deferred |', '| WR-04 | critcal | deferred |');
assert.notStrictEqual(cellMangled, base);
let after = runShippedDisposition({ reviewText: REVIEW_2_DROPPED, priorText: cellMangled });
assert.strictEqual(rowOf(after.ledger, 'WR-04').severity, 'critical', 'the frontmatter still knows');
assert.strictEqual(rowOf(after.ledger, 'WR-04').disposition, 'deferred', 'a bad severity cell does not cost the decision');
const bothMangled = cellMangled.replace(/^( {2}- id: WR-04\n {4}severity: )critical$/m, '$1critcal');
assert.notStrictEqual(bothMangled, cellMangled, 'the frontmatter mangling must have landed');
after = runShippedDisposition({ reviewText: REVIEW_2_DROPPED, priorText: bothMangled });
assert.strictEqual(rowOf(after.ledger, 'WR-04').severity, 'warning', 'nothing recorded is usable, so the prefix is all that is left');
});
test('a pre-severity ledger (bare rows, no frontmatter) still infers from the prefix', () => {
// Back-compat control. A ledger written by hand with no severity cell to speak of is not
// rejected; it just has nothing to carry, so the prefix governs as it always did.
const bare = '| WR-04 | | deferred | by hand |\n';
const after = runShippedDisposition({ reviewText: REVIEW_2_DROPPED, priorText: bare });
const row = rowOf(after.ledger, 'WR-04');
assert.strictEqual(row.severity, 'warning');
assert.strictEqual(row.disposition, 'deferred', 'an empty severity cell does not cost the decision');
});
});
describe('#3861 round 2 — a finding the heading parser cannot match is SURFACED, not dropped (B4)', () => {
// Two independent parsers produce two numbers one paragraph apart: the counts come from
// REVIEW.md's frontmatter, the rows from `### <ID>:` heading matches against a CLOSED
// CR|BL|WR|IN alternation. Nothing reconciled them, so a finding the alternation cannot reach
// contributed no row, no note and no diagnostic -- and the ledger asserted `open: N of N` over
// a set strictly smaller than the console line had just reported.
const REVIEW_5 = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 2', ' info: 2', ' total: 5', '---', '',
'### CR-01: a conforming finding',
'### WR-01: another conforming one',
'### WR-02: a third',
'### SEC-01: a prefix the alternation does not carry',
'#### IN-09: a heading one level too deep'].join('\n');
test('the shortfall is stated in the ledger frontmatter and on the console', () => {
const out = runShippedDisposition({ reviewText: REVIEW_5, reviewTotal: 5 });
assert.match(out.ledger, /^unparsed: 2$/m,
'the ledger must record that two findings reached no row');
assert.match(out.ledger, /^total: 3$/m, 'and must still report the rows it does have');
assert.match(out.stdout, /2 finding\(s\) recorded NOWHERE/,
'the console line must say so too -- the ledger is not the only surface a human reads');
assert.match(out.stdout, /the review reports 5, but only 3 matched/,
'and must name both numbers, so the shortfall is checkable rather than asserted');
});
test('a review whose findings all parse gains no unparsed key at all', () => {
// Negative control for the key itself. An ordinary ledger must not grow a noise key, or the
// unchanged-run check starts rewriting the file on every phase.
const clean = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---', '',
'### CR-01: the only finding'].join('\n');
const out = runShippedDisposition({ reviewText: clean, reviewTotal: 1 });
assert.doesNotMatch(out.ledger, /^unparsed:/m, 'nothing was dropped, so nothing is reported');
assert.doesNotMatch(out.stdout, /recorded NOWHERE/);
});
test('an absent or non-numeric total reconciles nothing rather than inventing a shortfall', () => {
// The reconciliation needs a number on BOTH sides. A legacy review with no findings: block
// has no total to compare against, and reporting `unparsed: 3` there would be a fabrication.
const legacy = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: a', '### WR-01: b'].join('\n');
const out = runShippedDisposition({ reviewText: legacy, reviewTotal: '' });
assert.doesNotMatch(out.ledger, /^unparsed:/m);
assert.match(out.ledger, /^total: 2$/m);
});
test('a total SMALLER than the rows is not reported as a negative shortfall', () => {
// Boundary in the other direction: the subtraction is clamped, so a review under-reporting
// its own total cannot produce `unparsed: -1`.
const out = runShippedDisposition({
reviewText: ['---', 'phase: 01', 'status: issues_found', 'findings:', ' total: 1', '---', '',
'### CR-01: a', '### WR-01: b'].join('\n'),
reviewTotal: 1,
});
assert.doesNotMatch(out.ledger, /^unparsed:/m, 'no shortfall when more parsed than declared');
assert.doesNotMatch(out.ledger, /unparsed: -/);
});
test('a review NONE of whose findings parse still records the shortfall on a first run', () => {
// The corner every case above misses, and the one carrying the LEAST evidence anywhere else:
// every finding in a heading shape the alternation cannot reach, on a phase with no prior
// ledger and no fix report. `order` is empty, so the early exit that stands down for "nothing
// to record" fired BEFORE the shortfall was computed -- no ledger, no console line, no
// diagnostic, for a review that declared two Criticals. A PARTIAL shortfall always reported,
// which is exactly why the total one read as covered. The shortfall is now derived above that
// exit and both exits decline to fire while one is outstanding.
const allUnmatched = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 2', ' warning: 0', ' info: 0', ' total: 2', '---', '',
'## Critical Issues', '',
'### SEC-01: a prefix the alternation does not carry',
'### SEC-02: and a second one'].join('\n');
const out = runShippedDisposition({ reviewText: allUnmatched, reviewTotal: 2 });
assert.notStrictEqual(out.ledger, null,
'a review whose findings NONE parsed must still leave a record — silence here is the exact silent drop this reconciliation exists to close');
assert.match(out.ledger, /^unparsed: 2$/m, 'and must state how many findings reached no row');
assert.match(out.ledger, /^total: 0$/m, 'while reporting honestly that it carries no rows');
assert.match(out.stdout, /2 finding\(s\) recorded NOWHERE/,
'the console line must say so too — the ledger is not the only surface a human reads');
});
test('a fix report present, nothing parsed, no prior ledger — the SECOND exit records it too', () => {
// The two exits that discarded the shortfall are reached by DIFFERENT inputs, so one test
// cannot pin both. The earlier one stands down as soon as a fix report exists; this input
// therefore sails past it and lands on the later `rows.length === 0` return, which had the
// identical hole. Without this case, deleting the later guard's conjunct leaves the pair
// above green and the drop returns by the other road — a surviving mutant, not a covered one.
const allUnmatched = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 2', ' warning: 0', ' info: 0', ' total: 2', '---', '',
'### SEC-01: a prefix the alternation does not carry',
'### SEC-02: and a second one'].join('\n');
// The fix report must contribute NO row, or it never reaches the later exit: an id the
// alternation CAN match becomes a carried row, rows.length is 1, and the guard under test is
// not the one that decides. Mutation-controlled — with a CR-NN id here the later guard's
// conjunct could be deleted and this test stayed green.
const fixReport = ['## Fixed Issues', '', '### SEC-03: an unmatched id in the fix report too'].join('\n');
const out = runShippedDisposition({ reviewText: allUnmatched, fixText: fixReport, reviewTotal: 2 });
assert.notStrictEqual(out.ledger, null,
'a fix report does not excuse the drop — the shortfall is still two findings recorded nowhere');
assert.match(out.ledger, /^unparsed: 2$/m, 'and the count must survive this path too');
});
test('a genuinely clean review still writes nothing — the relaxed exit is scoped to a shortfall', () => {
// Negative control for the fix itself, and the reason it is scoped rather than removed:
// relaxing that exit unconditionally would grow a zero-row ledger on every clean phase. With
// `total: 0` there is no shortfall to outstand, so both exits still fire exactly as before.
const clean = ['---', 'phase: 01', 'status: clean', 'findings:',
' critical: 0', ' warning: 0', ' info: 0', ' total: 0', '---', '', 'No issues.'].join('\n');
const out = runShippedDisposition({ reviewText: clean, reviewTotal: 0 });
assert.strictEqual(out.ledger, null, 'no findings declared and none parsed — nothing to record');
assert.strictEqual(out.stdout.trim(), '', 'and nothing to say');
});
test('a clean review with an unparseable fix report still writes nothing — the later exit must keep firing', () => {
// The fourth cell of the matrix, and the only one the three tests above leave open: they pin
// the LATER exit's under-fire (it must not swallow a shortfall) and the EARLIER exit's
// over-fire (a clean review must stay unrecorded), but nothing pins the later exit's OWN
// over-fire. It is reachable because a fix report — even one naming no matchable id — makes
// the earlier exit stand down, so control arrives at the later exit with a genuinely clean
// review and no shortfall. Neutering only that exit then writes a zero-row ledger reading
// "0 of 0 finding(s) open" for a phase that had nothing to report, and all three tests above
// stay green through it.
const clean = ['---', 'phase: 01', 'status: clean', 'findings:',
' critical: 0', ' warning: 0', ' info: 0', ' total: 0', '---', '', 'No issues.'].join('\n');
const unparseableFix = ['## Fixed Issues', '', '### SEC-03: an unmatched id in the fix report'].join('\n');
const out = runShippedDisposition({ reviewText: clean, fixText: unparseableFix, reviewTotal: 0 });
assert.strictEqual(out.ledger, null,
'a fix report that decides nothing is not a reason to record a ledger for a clean review');
assert.strictEqual(out.stdout.trim(), '', 'and nothing to say about it');
});
});
describe('#3829 review round 2 — a review that reports nothing still reconciles the ledger', () => {
const EMPTY_REVIEW = ['---', 'phase: 01', 'status: issues_found', '---', '', 'no findings'].join('\n');
test('a decided row is carried when the review reports no findings at all', () => {
// Exiting early on an empty review would freeze a stale ledger showing findings as open that
// the review no longer reports — the opposite of what this record exists to do.
const priorText = [
'| CR-01 | critical | deferred | ships next phase |',
'| WR-01 | warning | open | - |',
].join('\n');
const d = buildDisposition({ reviewText: EMPTY_REVIEW, priorText, padded: '01' });
// M1: BOTH rows survive. The decided one because it was triaged, the untriaged one because
// it was not -- and an untriaged finding vanishing without trace is the defect #3829 names.
assert.strictEqual(d.total, 2, 'both rows survive; neither is silently deleted');
assert.deepStrictEqual(d.rows.map((r) => r.id), ['CR-01', 'WR-01']);
assert.strictEqual(d.rows[0].disposition, 'deferred');
assert.strictEqual(d.rows[0].carried, true);
assert.strictEqual(d.rows[1].disposition, 'open');
assert.strictEqual(d.rows[1].carried, true);
assert.strictEqual(d.open, 1, 'the untriaged carried row is still open, and the count says so');
});
test('a review with no findings and no prior ledger produces nothing at all', () => {
assert.strictEqual(buildDisposition({ reviewText: EMPTY_REVIEW, padded: '01' }), null);
});
// Ninth src.includes() retired (round 3): it pinned the guard's exact line, including the
// process.exit(0) the script no longer calls, so it went red on a change that left the property
// untouched. The property -- an empty review still reconciles an EXISTING ledger -- is asserted
// behaviourally by the round-2 describe '#3829 review round 2 — a review that reports nothing
// still reconciles the ledger' below, which drives the shipped script against a prior ledger.
});
// ---------------------------------------------------------------------------
// #3829 — the SHIPPED disposition script, driven. These tests execute the real
// embedded script (see shippedDispositionScript), so a regression in the
// workflow file turns them red. The mirror-based tests above are kept for the
// pure parsing shapes; these are the ones that hold the contract.
// ---------------------------------------------------------------------------
describe('#3829 — shipped disposition script (executed, not mirrored)', () => {
const REVIEW = ['---', 'phase: 01', 'status: issues_found', '---', '',
'### CR-01: a', '', '### WR-01: b', '', '### IN-01: c'].join('\n');
test('every finding is recorded, defaulting to open, at the right severity', () => {
const rows = ledgerRows(runShippedDisposition({ reviewText: REVIEW }).ledger);
assert.deepStrictEqual(rows.map((r) => [r.id, r.severity, r.disposition]), [
['CR-01', 'critical', 'open'], ['WR-01', 'warning', 'open'], ['IN-01', 'info', 'open'],
]);
});
test('--fix outcomes are reconciled, and a lookalike section heading is not one', () => {
const fixText = ['## Fixed Issues', '', '### WR-01: b', '',
'## Fixed Issues Verification', '', '### IN-01: c'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: REVIEW, fixText }).ledger);
const by = Object.fromEntries(rows.map((r) => [r.id, r]));
assert.strictEqual(by['WR-01'].disposition, 'fixed');
assert.strictEqual(by['WR-01'].source, '01-REVIEW-FIX.md');
assert.strictEqual(by['IN-01'].disposition, 'open', 'a lookalike heading must classify nothing');
});
test("a human's deferral reason survives, and the carried marker never accumulates", () => {
// Render -> re-parse -> render, four times, exactly as consecutive phase runs would.
const shrunk = ['---', 'status: issues_found', '---', '', '### WR-01: b'].join('\n');
let prior = ['| Finding | Severity | Disposition | Source |',
'|---------|----------|-------------|--------|',
'| CR-01 | critical | deferred | ships next phase, see ADR-99 |'].join('\n');
let ledger;
for (let i = 0; i < 4; i++) {
ledger = runShippedDisposition({ reviewText: shrunk, priorText: prior }).ledger;
prior = ledger;
}
const cr = ledgerRows(ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred');
assert.strictEqual(cr.source, 'ships next phase, see ADR-99 (not in the current review)');
assert.strictEqual(
(ledger.match(/\(not in the current review\)/g) || []).length, 1,
'the carried marker must appear exactly once however many times the gate runs'
);
});
test('the ledger is a fixed point: a run that changes nothing rewrites nothing', () => {
const first = runShippedDisposition({ reviewText: REVIEW }).ledger;
const again = runShippedDisposition({ reviewText: REVIEW, priorText: first });
assert.strictEqual(again.wroteNothing, true, 'an unchanged run must report unchanged');
assert.strictEqual(again.ledger, first, 'and must leave the bytes alone');
});
test('a finding the review no longer reports is carried whether or not it was triaged', () => {
const prior = ['| CR-01 | critical | deferred | later |', '| IN-01 | info | open | - |'].join('\n');
const shrunk = ['---', 'status: issues_found', '---', '', '### WR-01: b'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: shrunk, priorText: prior }).ledger);
const ids = rows.map((r) => r.id);
assert.ok(ids.includes('CR-01'), 'a deferred finding must survive leaving the review');
assert.ok(ids.includes('IN-01'), 'and so must an untriaged one -- that is the whole record');
});
test('a renumbered finding leaves its old row behind, marked, rather than vanishing', () => {
// The concrete case M1 names: run 1 records CR-01 open, the re-review renumbers it to CR-02,
// and run 2's ledger used to contain neither. The old row is now carried and marked, so the
// double-count is legible rather than a silent delete -- the stated cost of the fix.
const prior = '| CR-01 | critical | open | - |';
const renumbered = ['---', 'status: issues_found', '---', '', '### CR-02: the same finding, renumbered'].join('\n');
const ledger = runShippedDisposition({ reviewText: renumbered, priorText: prior }).ledger;
const rows = ledgerRows(ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-02', 'CR-01']);
assert.match(ledger, /\| CR-01 \|.*\(not in the current review\) \|/,
'the carried row is MARKED, so it does not claim the finding is live');
});
test('a review reporting nothing still reconciles an existing ledger', () => {
const empty = ['---', 'status: issues_found', '---', '', 'no findings'].join('\n');
const prior = ['| CR-01 | critical | deferred | later |', '| WR-01 | warning | open | - |'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: empty, priorText: prior }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01', 'WR-01'], 'reconciled, not truncated');
});
test('a review reporting nothing with no prior ledger writes no ledger at all', () => {
const empty = ['---', 'status: issues_found', '---', '', 'no findings'].join('\n');
assert.strictEqual(runShippedDisposition({ reviewText: empty }).ledger, null);
});
test('an applied outcome outranks an earlier deferral', () => {
const prior = '| CR-01 | critical | deferred | later |';
const fixText = ['## Fixed Issues', '', '### CR-01: a'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: REVIEW, priorText: prior, fixText }).ledger);
assert.strictEqual(rows.find((r) => r.id === 'CR-01').disposition, 'fixed');
});
test('a deferral on a finding STILL in the review is not reset to open', () => {
// Distinct from the carried case: this finding is present in the current review, so it takes
// the ordinary path. 'open' must never overwrite a recorded decision on that path either.
const prior = '| CR-01 | critical | deferred | ships next phase |';
const rows = ledgerRows(runShippedDisposition({ reviewText: REVIEW, priorText: prior }).ledger);
const cr = rows.find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred');
assert.strictEqual(cr.source, 'ships next phase');
assert.ok(!/not in the current review/.test(cr.source), 'it is present, so it is not carried');
});
test('BL- is recorded at the Critical tier, as the documented CR- equivalent', () => {
const withBlocker = ['---', 'status: issues_found', '---', '',
'### BL-01: a', '', '### WR-01: b'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: withBlocker }).ledger);
assert.deepStrictEqual(rows.map((r) => [r.id, r.severity]), [
['BL-01', 'critical'], ['WR-01', 'warning'],
]);
});
test('a finding id whose prefix is a JS object property name produces no row', () => {
const hostile = ['---', 'status: issues_found', '---', '',
'### constructor-01: x', '', '### __proto__-02: y', '', '### CR-01: real'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: hostile }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01']);
});
});
describe('#3829 review round 3 — hostile frontmatter and hand-edited ledgers', () => {
test('an UNTERMINATED frontmatter block yields no values, not the whole review body', () => {
// Stopping at "the next ---" is not enough: with no closing delimiter the scan would run to
// EOF and hand body text to every read, undoing the scoping fix entirely.
const unterminated = [
'---', 'phase: 01', 'status: issues_found', '',
'# body', '', 'total: 777', 'critical: 66',
].join('\n');
const parsed = parseGateCounts(unterminated);
assert.deepStrictEqual(parsed, { status: '', critical: '', warning: '', info: '', total: '' });
});
test('docs-parity: the frontmatter scan emits only when the closing delimiter was seen', () => {
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.ok(
src.includes('END{if (closed) printf "%s", buf}'),
'an unterminated frontmatter block must yield nothing'
);
});
test('a hand-edited row missing its trailing pipe still preserves the decision', () => {
// A mangled table is already broken; silently dropping the row would lose a deferral, which
// is the exact class of loss this record exists to prevent.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a'].join('\n');
const prior = '| CR-01 | critical | deferred | see issue 42';
const rows = ledgerRows(runShippedDisposition({ reviewText: review, priorText: prior }).ledger);
const cr = rows.find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred');
assert.strictEqual(cr.source, 'see issue 42');
});
});
describe('#3829 review round 3 — stale fix reports, fenced examples, hostile REVIEW.md', () => {
test('a STALE fix report does not mark a new finding of the same id as fixed', () => {
// Finding ids are reused across re-reviews. Matching on the id alone would let a fix report
// from an earlier review declare a brand-new CR-01 already fixed — the worst possible lie
// for a record whose whole job is saying what happened to a finding.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: NEW authentication bypass'].join('\n');
const fixText = ['## Fixed Issues', '', '### CR-01: OLD null dereference'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review, fixText }).ledger);
assert.strictEqual(rows[0].disposition, 'open');
});
test('a fix report naming the same finding still applies', () => {
const review = ['---', 'status: issues_found', '---', '', '### CR-01: same title'].join('\n');
const fixText = ['## Fixed Issues', '', '### CR-01: same title'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review, fixText }).ledger);
assert.strictEqual(rows[0].disposition, 'fixed');
});
test('a finding heading inside a fenced block is an example, not a finding', () => {
const review = ['---', 'status: issues_found', '---', '', '### CR-01: real',
'', '```', '### CR-77: an illustration', '```'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01']);
});
test('an id listed under BOTH Fixed and Skipped is not decided by row order', () => {
const review = ['---', 'status: issues_found', '---', '', '### WR-01: dup'].join('\n');
const fixText = ['## Fixed Issues', '', '### WR-01: dup', '',
'## Skipped Issues', '', '### WR-01: dup'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review, fixText }).ledger);
assert.strictEqual(rows[0].disposition, 'fixed', 'first occurrence wins, deterministically');
});
test('an escaped pipe in a deferral reason survives whole', () => {
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a'].join('\n');
const prior = '| CR-01 | critical | deferred | wait \\| see ADR-9 |';
const rows = ledgerRows(runShippedDisposition({ reviewText: review, priorText: prior }).ledger);
assert.strictEqual(rows[0].source, 'wait \\| see ADR-9');
});
// #3861 round 3. The Source cell is the one field this ledger asks a human to hand-edit, and
// "waiting on team A | team B to align" is an ordinary thing to type there. Under the previous
// capture a bare | failed the WHOLE prior-row match: prior.get() was undefined, the row fell
// through to open with an empty Source, and the console line read "1 of 1 finding(s) open" --
// a Critical a human explicitly deferred, with a documented reason, rendered indistinguishable
// from one never triaged, and the reason gone. Exactly the "was this ever seen" ambiguity
// #3829 exists to remove, reachable by one missing backslash. The cell is the LAST column, so
// it is now captured to the end of the line and a bare pipe is prose; the render escapes it
// so the table stays a table, and the second run converges.
test('a bare pipe in a deferral reason is kept as prose: the decision and the reason survive', () => {
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a'].join('\n');
const prior = '| CR-01 | critical | deferred | waiting on team A | team B to align |';
const first = runShippedDisposition({ reviewText: review, priorText: prior });
const rows = ledgerRows(first.ledger);
assert.strictEqual(rows[0].disposition, 'deferred', 'a bare pipe must not revert the decision');
assert.strictEqual(rows[0].source, 'waiting on team A \\| team B to align', 'the reason survives, escaped');
assert.match(first.ledger, /^open: 0$/m, 'and the headline count agrees with the row');
// Fixed point: the escaped form re-parses to itself, so the second run rewrites nothing.
const second = runShippedDisposition({ reviewText: review, priorText: first.ledger });
assert.strictEqual(ledgerRows(second.ledger)[0].source, 'waiting on team A \\| team B to align');
assert.match(second.stdout, /disposition unchanged/, 'the second run must converge');
});
// Round-3 adversarial pass on the fix above. The first escape used /(^|[^\\])\|/g, which CONSUMES
// the character before the pipe: adjacent bare pipes were escaped one per run (A||B -> A\||B ->
// A\|\|B, a third run to converge, breaking the advertised second-run fixed point), and an escaped
// backslash before a pipe (A\\|B) hid the pipe behind the wrong parity and left it bare in the
// rendered table. The generator then emitted at most one bare pipe, so no property reached either; it does now.
// Round-3 adversarial pass. The script printed its verdict and then called process.exit(0) on the
// 'unchanged' branch only. Node documents process.stdout writes to pipes as ASYNCHRONOUS on
// POSIX, and an explicit exit can pre-empt a pending write, so on those lanes the caller can see
// exit 0 with no verdict line -- a hardening, not a reproduced defect: the reviewer's empty-stdout
// observation turned out to be its own sandbox (a bare console.log child printed nothing there
// either), which is stated so nobody re-reads this as evidence the drop was seen. The script now
// runs inside main() and leaves by return, so the loop drains stdout before exit.
// Shape-pinned deliberately: the property IS the absence of the call, and a behavioural test would
// have to race a pipe to fail. Comments are stripped first so a mention is not a match, and the
// match covers the dotted, bracketed and whitespace-split spellings; a call built by any other
// indirection is outside this pin and is what code review is for.
test('the shipped script never calls process.exit -- it returns, so its verdict line is never lost', () => {
const code = shippedDispositionScript().replace(/^\s*\/\/.*$/gm, '');
assert.doesNotMatch(code, /process\s*(?:\.\s*exit\b|\[\s*['"]exit['"]\s*\])/,
'leave main() by return; an explicit exit can drop the verdict line on a piped stdout');
});
test('adjacent bare pipes and a backslash-then-pipe are escaped in ONE write, then converge', () => {
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a'].join('\n');
const prior = '| CR-01 | critical | deferred | A||B and C\\\\|D |';
const first = runShippedDisposition({ reviewText: review, priorText: prior });
assert.strictEqual(ledgerRows(first.ledger)[0].source, 'A\\|\\|B and C\\\\\\|D',
'every bare pipe is escaped on the first write, whatever precedes it');
const second = runShippedDisposition({ reviewText: review, priorText: first.ledger });
assert.strictEqual(second.ledger, first.ledger, 'and the escaped form is a fixed point');
assert.match(second.stdout, /disposition unchanged/);
});
});
// ---------------------------------------------------------------------------
// #4665 — `--fix` must be able to act on an existing REVIEW.md even when the
// incremental scope for a FRESH review is empty. check_empty_scope used to
// exit the entire workflow whenever REVIEW_FILES was empty — before
// dispatch-fix — so the documented contract ("after review completes (or if
// REVIEW.md already exists), auto-apply findings found") was unreachable for
// any phase whose post-review changes are planning artifacts only.
// ---------------------------------------------------------------------------
describe('#4665 — check_empty_scope --fix recovery onto an existing REVIEW.md', () => {
const readWorkflow = () => fs.readFileSync(WORKFLOW_PATH, 'utf8');
test('check_empty_scope recovers --fix onto an existing REVIEW.md instead of exiting (#4665)', () => {
const src = readWorkflow();
const stepIdx = src.indexOf('<step name="check_empty_scope">');
const nextStepIdx = src.indexOf('<step name="structural_pre_pass">');
assert.ok(stepIdx !== -1, 'check_empty_scope step must exist');
assert.ok(nextStepIdx > stepIdx, 'structural_pre_pass must follow check_empty_scope');
const block = src.slice(stepIdx, nextStepIdx);
// The recovery computes the phase's REVIEW.md path with the same
// expression spawn_reviewer uses.
assert.ok(
block.includes('REVIEW_PATH="${PHASE_DIR}/${PADDED_PHASE}-REVIEW.md"'),
'check_empty_scope must compute REVIEW_PATH exactly as spawn_reviewer does'
);
// The skip is now guarded: it fires only when --fix is absent OR no
// REVIEW.md exists on disk.
assert.ok(
block.includes('FIX_FLAG') && block.includes('-f "${REVIEW_PATH}"'),
'the empty-scope skip must be guarded on FIX_FLAG and the existing REVIEW.md file check'
);
// The fence must be self-contained about its own precondition: an explicit
// emptiness check, so a literal-minded execution cannot read the recovery
// paragraph as skipping a needed fresh review on a non-empty scope.
assert.ok(
block.includes("\"${#REVIEW_FILES[@]}\" -ne 0"),
'check_empty_scope must assert REVIEW_FILES emptiness explicitly, not only in prose'
);
});
test('the fix-recovery path routes to dispatch-fix past the fresh-review steps (#4665)', () => {
const src = readWorkflow();
const stepIdx = src.indexOf('<step name="check_empty_scope">');
const nextStepIdx = src.indexOf('<step name="structural_pre_pass">');
const block = src.slice(stepIdx, nextStepIdx);
assert.match(block, /dispatch-fix/, 'the recovery must route to dispatch-fix');
assert.match(
block, /structural_pre_pass[\s\S]*dispatch_reviewer_lanes[\s\S]*spawn_reviewer[\s\S]*commit_review/,
'the recovery must name the fresh-review steps it skips (no reviewer spawn, nothing to commit)'
);
assert.doesNotMatch(
block, /spawn the (reviewer|agent)|msd-code-reviewer/,
'the recovery must not contain an instruction to spawn a fresh reviewer'
);
});
test('the plain empty-scope skip survives for the non-fix path (#4665)', () => {
const src = readWorkflow();
const stepIdx = src.indexOf('<step name="check_empty_scope">');
const nextStepIdx = src.indexOf('<step name="structural_pre_pass">');
const block = src.slice(stepIdx, nextStepIdx);
assert.ok(
block.includes('No source files changed in phase ${PHASE_ARG}. Skipping review.'),
'the plain skip text must remain for invocations without --fix or without an existing REVIEW.md'
);
assert.match(block, /Exit workflow/i, 'the non-recovery path must still end the workflow');
});
});
// ---------------------------------------------------------------------------
// #3861 round 1 — the two blockers, and the structural properties that catch them
//
// Both were invisible to every behavioural test above, for the same reason: those
// tests execute the node script through the process seam with the environment
// handed to it, so they never see the SHELL that is supposed to build that
// environment, nor the prose that tells the agent what to read.
// ---------------------------------------------------------------------------
// Every ```bash fence in a step file, in order.
function bashFences(src) {
const out = [];
const lines = src.replace(/\r\n/g, '\n').split('\n');
let start = -1;
for (let i = 0; i < lines.length; i++) {
if (start === -1 && /^```bash\s*$/.test(lines[i])) { start = i + 1; continue; }
if (start !== -1 && /^```\s*$/.test(lines[i])) { out.push(lines.slice(start, i).join('\n')); start = -1; }
}
return out;
}
describe('#3861 round 1 — step-file structural contract', () => {
test('the step file never instructs the agent to read and execute ITSELF', () => {
// A step file that names its own path as something to "read and execute" is
// unbounded self-recursion at runtime, and nothing downstream bounds it.
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
const self = 'execute-phase/steps/' + path.basename(DISPOSITION_STEP_PATH);
assert.ok(
!src.includes(self),
'code-review-disposition.md must not reference its own path — execute-phase.md is what points here'
);
});
test('the disposition instruction appears exactly once', () => {
// The self-reference above arrived as a duplicated paragraph; the duplicate is
// the tell, and no linter scores markdown prose.
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
const n = src.split(/\r?\n/).filter((l) => l.startsWith('**Record a per-finding disposition.**')).length;
assert.strictEqual(n, 1, 'exactly one disposition instruction, not a pointer plus a body');
});
test('every shell block derives the variables it reads — blocks do not share a shell', () => {
// Each fenced block is dispatched as its own Bash call, so a variable derived in
// block 1 is EMPTY in block 2 — and empty is silent: the write lands on a bare
// `-REVIEW-DISPOSITION.md` and the step still reports success. The step's own
// inputs (PHASE_DIR, PHASE_NUMBER) are the only values a block may inherit.
const INPUTS = new Set(['PHASE_DIR', 'PHASE_NUMBER']);
const DERIVED = ['PADDED', 'REVIEW_FILE', 'DISPOSITION_FILE'];
const fences = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'));
assert.ok(fences.length >= 2, 'the step must still carry more than one shell block');
for (const [i, fence] of fences.entries()) {
for (const v of DERIVED) {
if (INPUTS.has(v)) continue;
// The derivation may be INDENTED or sit inside a `case` arm -- PADDED is derived by a
// case split so a dotted phase number does not reach `printf %02d`. Anchoring the
// detector at column 0 made a real derivation invisible and the guard fired on it. The
// property being checked is unchanged: the block must ASSIGN what it reads.
// A SELF-REFERENTIAL assignment is a PASS-THROUGH, not a derivation. Block 2 prefixes its
// `node -e` with `REVIEW_FILE="${REVIEW_FILE}" DISPOSITION_FILE="${DISPOSITION_FILE}" ...`
// to put them in the child's environment, and the detector counted that as deriving them.
// So deleting block 2's REAL derivation left this guard GREEN -- on the exact defect it
// was written for. Found by negative-controlling the guard rather than by reading it.
// (Pre-existing: the original column-0 anchor matched that same line, which is at column 0.)
const fenceNoPassthrough = fence.replace(
new RegExp('(?:^|[\\s;])' + v + '="\\$\\{' + v + '\\}"', 'gm'), ' ');
// Non-empty RHS required. `REVIEW_FILE=` is an assignment TOKEN and not a derivation, and
// the reviewer evaded the earlier predicate with exactly that. This is a cheap fast-fail,
// NOT the guard's authority -- see the executed guard below, which is.
const assigned = new RegExp('(?:^|[\\s;])' + v + '=(?![\\s;#]|$)', 'm');
const reads = new RegExp('\\$\\{?' + v + '\\b').test(fence.replace(new RegExp('(?:^|[\\s;])' + v + '=', 'gm'), ''));
if (!reads) continue;
assert.ok(
assigned.test(fenceNoPassthrough),
'block ' + (i + 1) + ' reads ' + v + ' without deriving it — it is empty in a fresh shell'
);
}
}
});
});
describe('#3861 round 16 — the embedded script fits a Windows command line', () => {
// Block 2 runs the record-builder as `node -e "<script>"`, so the whole script is ONE
// argv entry. Windows caps a command line at 32767 characters (CreateProcess), and Node
// surfaces the overflow as ENAMETOOLONG from spawn — the process never starts. Linux's
// ~2 MB ARG_MAX means the Linux lane cannot see this at all: when the script crossed the
// cap it stayed green on ubuntu and took out 83 tests on windows-latest in one push,
// every one of them reporting `spawn_failed` rather than anything about length.
//
// Measured on native Windows (node v25.2.1): the largest `-e` argument that still spawns
// is 32728 characters; 32729 fails. The budget below sits well under that so the next
// addition to the script has somewhere to go — long rationale belongs in the step file's
// prose, which costs the command line nothing.
//
// What this guard does NOT establish, stated so it is not read as more than it is. It counts
// the extracted JavaScript, not the command line Windows finally serializes: the 32767 cap
// applies to the whole line — executable path, quoting and backslash escaping included — so a
// quote-heavy payload expands on the way out, and the 32728 figure above is one host's
// threshold rather than the CI runner's. The budget is a practical margin with room for both
// effects, not a proof that everything it admits will spawn.
const WINDOWS_CMDLINE_CAP = 32767;
const CMDLINE_BUDGET = 24576; // 24 KiB — the cap less ~8 KiB of deliberate headroom
test('the extracted node -e script stays well under the Windows command-line cap', () => {
const script = shippedDispositionScript();
// Anti-vacuity: a length assertion alone passes when the extractor returns '' — which is
// exactly what a moved fence or a renamed delimiter would produce. Bound it from BELOW
// first, so a broken extractor fails here instead of reporting a comfortable 0 bytes.
assert.ok(
script.length > 4096,
'the extractor returned ' + script.length + ' bytes — it is not reading the shipped script'
);
assert.ok(
script.length <= CMDLINE_BUDGET,
'the embedded node -e script is ' + script.length + ' characters; the budget is ' +
CMDLINE_BUDGET + ' and the hard Windows limit is ' + WINDOWS_CMDLINE_CAP +
'. Move long rationale out of the script and into the step file prose — it reads the ' +
'same there and costs the command line nothing.'
);
});
});
describe('#3861 round 2 — the shell-sharing guard, EXECUTED', () => {
// The textual guard above is a fast-fail, not the authority. An adversarial pass evaded it
// three ways -- an empty `REVIEW_FILE=`, a self-referential `REVIEW_FILE=$REVIEW_FILE`, and a
// commented assignment -- because a structural predicate recognises assignment TOKENS, never
// assignments that derive a usable value. No amount of regex fixes that class.
//
// So the authority moves to execution, which is the lesson this PR has now learned three times:
// run the real second fence in a FRESH shell with nothing but the step's two declared inputs,
// and require it to write the ledger at the correct derived path. Every derivation in that
// fence is load-bearing for that outcome, so no textual dodge survives it.
// RANDOM PHASES, and the randomness is the mechanism rather than decoration. Fixed fixtures
// cannot establish derivation: an adversarial pass defeated the one-phase version with
// `case ... in 7) PADDED=07 ;; *) PADDED=07 ;; esac`, and then defeated the two-phase version
// by simply adding `3.1) PADDED=03.1` to the same case. Any finite sample loses that race --
// the enumeration just grows to cover whatever the test happens to name.
//
// A phase picked at RUN TIME raises the cost of a hardcode from two arms to the whole drawn
// domain, so an accidental loss of derivation fails on some run rather than never.
//
// STATED HONESTLY, because the first version of this comment overclaimed and was refuted: the
// domain is FINITE -- 88 integer values and 792 dotted ones -- so a mutation enumerating all
// 880 passes forever, and one covering 90 or 12345678.1 or 1.10 would still break real phases
// the draw cannot reach. This raises the bar; it does not prove derivation, and nothing short
// of reading the fence can. `Math.random()` is also unseeded, so a failure is reproducible only
// in the sense that the drawn values are printed in every assertion message below -- re-running
// draws different ones. That is the honest description of what this buys.
//
// One integer and one dotted phase per run: the dotted one additionally pins the integer-part
// split a naive `%02d` cannot express.
const rnd = (lo, hi) => lo + Math.floor(Math.random() * (hi - lo + 1));
const intPhase = String(rnd(2, 89));
const dotPhase = rnd(2, 89) + '.' + rnd(1, 9);
const pad = (v) => {
const [i, sub] = String(v).split('.');
return String(Number(i)).padStart(2, '0') + (sub === undefined ? '' : '.' + sub);
};
for (const [phaseNumber, padded] of [[intPhase, pad(intPhase)], [dotPhase, pad(dotPhase)]]) {
test('block 2 derives its own paths and writes the ledger for phase ' + phaseNumber,
{ skip: !HAS_BASH }, () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-blk2-'));
const drawn = ' (drawn phase ' + phaseNumber + ' -> ' + padded + ')';
try {
fs.writeFileSync(path.join(dir, padded + '-REVIEW.md'),
['---', 'phase: ' + padded, 'status: issues_found', '---', '',
'### CR-01: a finding'].join('\n'));
const fence = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'))[1];
assert.ok(fence && fence.includes('DISPOSITION_FILE'), 'the disposition block must be fence 2');
// Scrub the DERIVED names from the inherited environment, so the claim "given only the
// declared inputs" is true rather than merely intended. `...process.env` is still needed
// for PATH/HOME, and an adversarial pass was right to call out the earlier wording.
const env = { ...process.env, PHASE_DIR: dir, PHASE_NUMBER: phaseNumber, RUNTIME_DIR: ROOT };
for (const k of ['PADDED', 'REVIEW_FILE', 'DISPOSITION_FILE', 'FIX_REPORT_FILE']) delete env[k];
const res = runHook('-c', ['set -euo pipefail\n' + fence + '\n'], {
interpreter: 'bash', timeoutMs: PROBE_TIMEOUT_MS, env,
});
assert.strictEqual(res.outcome, OUTCOME.EXITED, 'the block must run to completion');
assert.strictEqual(res.exitCode, 0, 'advisory: it must not abort' + drawn + ': ' + res.stderr);
assert.ok(fs.existsSync(path.join(dir, padded + '-REVIEW-DISPOSITION.md')),
'the ledger must land at the DERIVED path — a lost derivation writes elsewhere or nowhere' + drawn);
const rows = ledgerRows(fs.readFileSync(path.join(dir, padded + '-REVIEW-DISPOSITION.md'), 'utf8'));
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01'], 'and it must have read the review' + drawn);
assert.ok(!fs.existsSync(path.join(dir, '-REVIEW-DISPOSITION.md')),
'an empty PADDED must never produce a bare-named ledger' + drawn);
} finally {
cleanup(dir);
}
});
}
});
// ---------------------------------------------------------------------------
// #3861 round 1, Major 4 + Minor 9 — the frontmatter reads, EXECUTED
//
// The disposition builder stopped being a mirror three rounds ago, for a reason
// this file already states: a hand model of a shell-embedded script drifts, and
// when it drifts the tests pass while the shipped block is broken. The counts
// parser kept its mirror anyway, and the argument against mirrors does not stop
// applying at the boundary between the two blocks.
//
// So the mirror loses its authority: it is now asserted AGAINST the shipped awk
// and greps, run under `set -euo pipefail` in a real shell, over every fixture
// the mirror is tested on. Divergence in either direction fails.
//
// Running the block also makes its advisory guards behavioural rather than
// textual, which retires the two `src.includes()` docs-parity assertions that
// stood in for them — Minor 9's anti-pattern, reduced by executing the thing
// the assertions were describing.
// ---------------------------------------------------------------------------
// Run the step's FIRST shell block — the frontmatter reads — and report WHAT IT PRINTS.
//
// This harness used to append its own `printf` of the six internal variables to the fence before
// running it, and every assertion below then read those six lines. That is a test manufacturing the
// observable it asserts on: the shipped fence emitted nothing, the tested fence emitted six lines
// because the test added them, and the whole group was green against a script that did not exist
// outside this process. It is why the "the gate reports the counts" defect shipped past a suite that
// looks like it covers exactly that surface — the green was structurally incapable of turning red
// for it. The emitter now lives in the fence (see the step file), so the harness reads the fence's
// own stdout and nothing is synthesized here.
// `plantDir` stands a DIRECTORY at the review path. It is the root-immune half of the
// unreadable-review contract: the fence's guard is `[ -f "$REVIEW_FILE" ] && [ -r ... ]`,
// and only the `-r` leg is defeated by running as root. A directory fails the `-f` leg on
// every lane and euid, so it reaches the same non-reporting arm without depending on
// permission bits at all. See the two tests at the end of this describe.
function runShippedGateCounts({ reviewText, padded = '01', writeReview = true, mode, phaseNumber, plantDir = false, extraEnv = {} }) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-'));
try {
const reviewPath = path.join(dir, padded + '-REVIEW.md');
if (plantDir) fs.mkdirSync(reviewPath);
else if (writeReview) fs.writeFileSync(reviewPath, reviewText);
if (mode !== undefined) fs.chmodSync(reviewPath, mode);
const fence = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'))[0];
assert.ok(fence && fence.includes('REVIEW_COUNTS_OK'), 'the counts block must still be the first fence');
// `set -euo pipefail` is the point, not decoration: the step is advisory, so a
// non-matching grep or an unreadable review must not take the block down.
const script = 'set -euo pipefail\n' + fence + '\n';
const res = runHook('-c', [script], {
interpreter: 'bash',
timeoutMs: PROBE_TIMEOUT_MS,
// `Number(padded)` is deliberate for the integer case (strips the leading zero the way a
// caller's parsed phase_number does) but must not mangle a DOTTED phase: Number('03.1') is
// 3.1, which is what we want, while a non-numeric padded value would become NaN.
// `phaseNumber` overrides the derivation so a test can drive a value `padded` cannot
// express -- an unusable one. Otherwise PHASE_NUMBER is derived from `padded` as a caller's
// parsed phase_number would be.
env: {
...process.env,
PHASE_DIR: dir,
PHASE_NUMBER: phaseNumber === undefined ? String(Number(padded)) : phaseNumber,
// `extraEnv` exists for ONE property: the reads pin LC_ALL=C, and a test that cannot set
// the ambient locale cannot prove the pin is doing anything.
...extraEnv,
},
});
assert.strictEqual(res.outcome, OUTCOME.EXITED, 'the counts block must run to completion');
return { exitCode: res.exitCode, stdout: res.stdout, stderr: res.stderr };
} finally {
cleanup(dir);
}
}
// Read the shipped message back into the facts it asserts. This parses the OBSERVABLE the operator
// sees — it does not reach into the fence — so a value the gate declines to report is `reported:
// false` here rather than a number this helper invented.
function readGateMessage(stdout) {
const full = /^Code review: (\d+) findings — (\d+) critical, (\d+) warning, (\d+) info\.$/m.exec(stdout);
if (full) {
return { reported: true, countsOk: '1', total: full[1], critical: full[2], warning: full[3], info: full[4] };
}
if (/^Code review found issues\.$/m.test(stdout)) return { reported: true, countsOk: '0' };
// The third arm (round 11): the file was READ and yielded no status. Reported -- the operator is
// told something -- but no counts and no verdict, which is what `unparsed` records.
if (/^Code review status unparsed: /m.test(stdout)) return { reported: true, countsOk: '0', unparsed: true };
return { reported: false };
}
// The mirror's other half: render the two arms exactly as the shipped fence does, from the mirror's
// own parsed counts. Parity is asserted over this WHOLE STRING rather than over five intermediate
// values, which is what makes the assertion bind to something a user can see. The countsOk gate is
// modelled here because the shipped fence gates on it — digit-only, at most 8 digits, and the three
// severities must sum to the total.
function renderGateMessage(counts, phaseNumber) {
const numeric = (v) => v !== '' && /^[0-9]+$/.test(v) && v.length <= 8;
const all = [counts.total, counts.critical, counts.warning, counts.info];
let ok = all.every(numeric);
if (ok) {
const n = (v) => parseInt(v, 10);
if (n(counts.critical) + n(counts.warning) + n(counts.info) !== n(counts.total)) ok = false;
}
// A mirror is always handed a TEXT, so the file was read by construction: an empty status here
// is the read-but-unparseable arm, never the absent one (round 11 Minor -- a malformed report
// must not read as clean). The absent arm is driven directly, not through the mirror.
if (counts.status === '') {
return 'Code review status unparsed: REVIEW.md is present but its frontmatter has no parseable status; severity counts unavailable.\n';
}
if (counts.status === 'clean' || counts.status === 'skipped') return '';
const head = ok
? `Code review: ${counts.total} findings — ${counts.critical} critical, ${counts.warning} warning, ${counts.info} info.`
: 'Code review found issues.';
return head + '\n' + `Consider running: /msd:code-review ${phaseNumber} --fix` + '\n';
}
describe('#3861 round 2 — the ledger write refuses a non-regular file', () => {
// The write-safety behaviour shipped with NO regression control at all -- I hand-drove it and
// did not pin it, which the round review caught by grepping for the words. Three shapes, each
// a distinct failure mode, all driven before this test existed:
// symlink -> writeFileSync FOLLOWS it and replaced the target's contents, outside the
// phase directory, leaving the link intact so nothing looked wrong;
// FIFO -> readFileSync BLOCKED FOREVER, in a gate documented as never blocking;
// directory -> the write throws.
const mkPhase = () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-nrf-'));
fs.writeFileSync(path.join(dir, '03-REVIEW.md'),
['---', 'status: issues_found', '---', '', '### CR-01: a finding'].join('\n'));
return dir;
};
const runAt = (dir) => runNode(['-e', shippedDispositionScript()], {
timeoutMs: PROBE_TIMEOUT_MS,
env: {
...process.env,
REVIEW_FILE: path.join(dir, '03-REVIEW.md'),
DISPOSITION_FILE: path.join(dir, '03-REVIEW-DISPOSITION.md'),
FIX_REPORT_FILE: path.join(dir, '03-REVIEW-FIX.md'),
PADDED: '03',
},
});
// Skipped on win32 per the repo's existing convention for symlink-planting tests:
// tests/settings-jsonc.test.cjs:389 skips the same class, and
// tests/unreachable-guard-drift.test.cjs:726 records why -- symlink creation requires elevated
// privileges on Windows CI. It happened to be available on the lane this round; the convention
// exists because it is not guaranteed.
test('a symlink at the ledger path is refused, and its target is untouched',
{ skip: process.platform === 'win32' }, () => {
const dir = mkPhase();
try {
const outside = path.join(dir, 'outside.txt');
fs.writeFileSync(outside, 'ORIGINAL');
fs.symlinkSync(outside, path.join(dir, '03-REVIEW-DISPOSITION.md'));
const res = runAt(dir);
assert.strictEqual(res.exitCode, 0, 'advisory: it refuses, it does not fail');
assert.match(res.stdout, /not a regular file/, 'and says why');
assert.strictEqual(fs.readFileSync(outside, 'utf8'), 'ORIGINAL',
'the symlink target must not be overwritten');
assert.ok(fs.lstatSync(path.join(dir, '03-REVIEW-DISPOSITION.md')).isSymbolicLink(),
'and the link itself is left alone');
} finally { cleanup(dir); }
});
test('a symlink whose target ALREADY matches is refused too — the fast path does not bypass it',
{ skip: process.platform === 'win32' }, () => {
// The unchanged-run fast path read the ledger before the check, so a link whose target
// happened to match slipped through reporting `unchanged`. The check is first now.
const dir = mkPhase();
try {
const outside = path.join(dir, 'outside.txt');
fs.writeFileSync(outside, 'ORIGINAL');
fs.symlinkSync(outside, path.join(dir, '03-REVIEW-DISPOSITION.md'));
runAt(dir);
const res = runAt(dir);
assert.match(res.stdout, /not a regular file/);
assert.doesNotMatch(res.stdout, /unchanged/, 'the fast path must not run ahead of the check');
assert.strictEqual(fs.readFileSync(outside, 'utf8'), 'ORIGINAL');
} finally { cleanup(dir); }
});
test('a FIFO at the ledger path is refused rather than blocking the phase forever', () => {
const dir = mkPhase();
try {
// Through the process seam, like every other spawn in this file.
const fifoPath = path.join(dir, '03-REVIEW-DISPOSITION.md');
runHook('-c', ['mkfifo "$1"', '_', fifoPath], { interpreter: 'bash', timeoutMs: PROBE_TIMEOUT_MS });
// GATE ON WHAT WAS CREATED, never on mkfifo's exit code. On the Windows lane mkfifo EXISTS
// and exits 0 while producing something that is not a FIFO, so an exit-code guard let this
// test run against an ordinary path: the ledger wrote normally and the assertion below
// failed for a reason that had nothing to do with the behaviour under test. Caught by CI,
// not by the local suite or five review passes -- every one of which ran on Linux.
let isFifo = false;
try { isFifo = fs.lstatSync(fifoPath).isFIFO(); } catch { isFifo = false; }
if (!isFifo) return; // no real FIFO on this platform; nothing to assert
const res = runAt(dir);
assert.strictEqual(res.outcome, OUTCOME.EXITED,
'a FIFO must not hang the step — readFileSync blocks on one forever');
assert.strictEqual(res.exitCode, 0);
assert.match(res.stdout, /not a regular file/);
} finally { cleanup(dir); }
});
test('a directory at the ledger path is refused', () => {
const dir = mkPhase();
try {
fs.mkdirSync(path.join(dir, '03-REVIEW-DISPOSITION.md'));
const res = runAt(dir);
assert.strictEqual(res.exitCode, 0);
assert.match(res.stdout, /not a regular file/);
} finally { cleanup(dir); }
});
test('an ordinary ledger is still written — the refusal is not a blanket refusal', () => {
const dir = mkPhase();
try {
const res = runAt(dir);
assert.strictEqual(res.exitCode, 0);
assert.doesNotMatch(res.stdout, /not a regular file/);
assert.ok(fs.existsSync(path.join(dir, '03-REVIEW-DISPOSITION.md')));
} finally { cleanup(dir); }
});
});
describe('#3861 round 2 — a DOTTED phase number does not break the step', () => {
// Found by the round's own adversarial review, in its MISSED section -- no finding asked about
// it. Both of this step's call sites accept `03.1` -- `code-review-fix.md`'s PADDED_PHASE validator accepts
// ^[0-9]+(\.[0-9]+)*$ (widened from `?` to `*` by #4568; this comment named the pre-#4568 form
// until round 11) and `execute-phase.md` applies no shape gate at all. (`code-review.md`'s
// own PADDED_PHASE validator is identical but never dispatches this step; it was cited here as a caller
// for several rounds and is not one.) The step reconstructed the path with `printf "%02d"`, which cannot
// format one: bash prints `invalid number` and exits 1. Under `set -euo pipefail` that aborts
// the step on its FIRST line -- the loudest possible failure from a gate that promises never to
// block, and it takes the phase's whole review report with it.
// #3861 round 5, minor 1. The PADDED derivation -- the traversal fence between an
// attacker-influenceable phase number and a file path, plus the per-component length bound -- is
// duplicated verbatim across both fences, because each fenced block runs in a fresh shell and must
// derive what it reads. Each copy is independently tested, but nothing asserted they stay in step,
// and a future edit to one could silently desync the other with the suite still green. That is the
// shared-parallel-surface shape CLAUDE.md requires a parity test for, and it is security-relevant
// validation logic rather than incidental repetition.
//
// Compared LINE BY LINE rather than through a normalizing rewrite: a normalizer would have to be
// told what may differ, and anything it was told to tolerate would stop being asserted. Exactly one
// line may differ, and the test names both of its forms.
test('the two fences derive PADDED identically, and only the refusal message may differ', () => {
const fences = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'));
assert.strictEqual(fences.length, 2, 'the step must still carry exactly two bash fences');
const derivationOf = (fence, which) => {
const start = fence.indexOf('_pd="${PHASE_DIR:-}"');
const end = fence.indexOf('DISPOSITION_FILE="${_pd}/${PADDED}-REVIEW-DISPOSITION.md"');
assert.ok(start > -1, 'block ' + which + ' must still open the derivation with _pd');
assert.ok(end > start, 'block ' + which + ' must still close it by building the ledger path');
return fence.slice(start, end).split('\n');
};
const a = derivationOf(fences[0], 1);
const b = derivationOf(fences[1], 2);
// A parity test over an empty or trivial slice passes vacuously and pins nothing.
assert.ok(a.length > 20, 'the derivation must still be the substantial block this pins');
assert.strictEqual(a.length, b.length, 'the two derivations must have the same shape');
const differing = a.map((line, i) => [i, line, b[i]]).filter((e) => e[1] !== e[2]);
assert.strictEqual(differing.length, 1,
'exactly one line may differ between the two derivations; got ' + differing.length + ': ' +
JSON.stringify(differing.map((e) => [e[1], e[2]])));
assert.match(differing[0][1], /Code review reporting skipped/, 'block 1 refuses by its own name');
assert.match(differing[0][2], /Code review disposition skipped/, 'block 2 refuses by its own name');
});
test('block 1 reports a dotted phase instead of aborting', { skip: !HAS_BASH }, () => {
const review = ['---', 'phase: 03.1', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---', '',
'### CR-01: a real finding'].join('\n');
const out = runShippedGateCounts({ reviewText: review, padded: '03.1' });
assert.strictEqual(out.exitCode, 0, 'advisory: a dotted phase must not abort the step');
assert.doesNotMatch(out.stderr, /invalid number/,
'the phase number must never reach printf %02d unsplit');
assert.match(out.stdout, /^Code review: 1 findings — 1 critical, 0 warning, 0 info\.$/m,
'and the review is actually found and reported');
});
test('a phase number outside the documented shape builds NO path — traversal fence', { skip: !HAS_BASH }, () => {
// PHASE_NUMBER is interpolated into a file path. The first draft of the dotted-phase fix
// carried an unusable value VERBATIM, which made `${PHASE_DIR}/../../etc/passwd-REVIEW.md`
// reachable where the old `printf "%02d"` had at least mangled it to `00` -- a regression
// introduced by the fix, found by adversarially reviewing it. Both call sites accept
// ^[0-9]+(\.[0-9]+)*$ -- an UNBOUNDED segment count since #4568, anchored at
// `code-review-fix.md`'s PADDED_PHASE validator and ungated at `execute-phase.md` -- and this step has two
// call sites and validates for itself.
// `1.2.3` LEFT THIS LIST in round 11. It is a legal N-segment id at the current base, and
// asserting its refusal here is precisely what held the step narrower than both of them;
// the positive case is its own test below. What remains here is SHAPE, not arity, so the
// two malformed-dot cases that the arity guard used to mask are added explicitly.
// The LETTER AXIS joined in round 12 (#4744 / #4660): a single uppercase letter is legal only as
// the last character of the integer part, so every other placement is shape, not arity.
for (const bad of ['../../etc/passwd', 'abc', '', '-1', '3.', '.1', '+1', '3 1', '1..2', '1.2.',
'23a', 'A23', '2A3', '23AB', '23.1A']) {
const out = runShippedGateCounts({ reviewText: '', writeReview: false, phaseNumber: bad });
assert.strictEqual(out.exitCode, 0, 'advisory: `' + bad + '` must not abort the step');
assert.match(out.stdout, /skipped \(unusable phase number/,
'`' + bad + '` must be refused by name, not silently coerced');
assert.doesNotMatch(out.stdout, /Code review: /,
'`' + bad + '` must not report counts read from a path built out of it');
}
});
test('an N-SEGMENT phase number reports counts, exactly as its callers accept it', { skip: !HAS_BASH }, () => {
// #3861 round 11. Found by this round's own adversarial review, not by the maintainer's.
// The base range widened `code-review-fix.md`'s PADDED_PHASE validator to `^[0-9]+(\\.[0-9]+)*$` (#4568), matching the
// segment-count freedom the canonical grammar in src/phase-id.cts has carried since
// #2128. This step still carried
// `*.*.*) _ok=0` -- "more than one dot: not the documented shape" -- so `23.1.2` took the
// refusal arm, printed `skipped (unusable phase number ...)` and wrote NO ledger, for a
// phase id its own dispatcher had just produced.
//
// It degraded LOUDLY, not silently, which is exactly why nothing caught it: the
// traversal-fence test above asserted that refusal as CORRECT. An arity bound and a shape
// bound had been folded into one arm, so the test that should have failed was the test
// that encoded the bug.
//
// THREE segments and FOUR, deliberately: the retired guard was arity-shaped, so a fix that
// merely moved the bound from two dots to three would pass a three-segment-only test.
for (const phase of ['23.1.2', '1.2.3.4']) {
const review = ['---', 'phase: ' + phase, 'status: issues_found', 'findings:',
' critical: 1', ' warning: 2', ' info: 0', ' total: 3', '---', '',
'### CR-01: a finding'].join('\n');
const padded = phase.replace(/^[0-9]+/, (m) => m.padStart(2, '0'));
const out = runShippedGateCounts({ reviewText: review, padded, phaseNumber: phase });
assert.strictEqual(out.exitCode, 0, phase + ': advisory -- must not abort the step');
assert.doesNotMatch(out.stdout, /skipped \(unusable phase number/,
phase + ': must NOT be refused -- both of this step\'s callers accept it');
assert.match(out.stdout, /^Code review: 3 findings — 1 critical, 2 warning, 0 info\.$/m,
phase + ': the review must be found at the N-segment path and reported');
assert.doesNotMatch(out.stderr, /invalid number/,
phase + ': the phase number must never reach printf %02d unsplit');
}
});
test('a LETTER-VARIANT phase number reports counts, padded as the canonical grammar pads it', { skip: !HAS_BASH }, () => {
// #3861 round 12. #4744 (#4660) widened the six shell/markdown phase mirrors to the canonical
// grammar's letter axis after this branch was cut, and its `lint-phase-id-drift` ratchet then
// flagged this step as the one digit-only mirror left -- found by running the base range's
// modified gates against the rebased tree, not by the review. `12A` and `23A.1.2` were refused
// by name; `3A` must pad to `03A`, the letter carried verbatim after the padded digits exactly as
// src/phase-id.cts pads it.
for (const [phase, padded] of [['12A', '12A'], ['3A', '03A'], ['23A.1.2', '23A.1.2'], ['12345678A', '12345678A']]) {
const review = ['---', 'phase: ' + phase, 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---', '',
'### CR-01: a finding'].join('\n');
const out = runShippedGateCounts({ reviewText: review, padded, phaseNumber: phase });
assert.strictEqual(out.exitCode, 0, phase + ': advisory -- must not abort the step');
assert.doesNotMatch(out.stdout, /skipped \(unusable phase number/,
phase + ': must NOT be refused -- its dispatcher accepts it since #4744');
assert.match(out.stdout, /^Code review: 1 findings — 1 critical, 0 warning, 0 info\.$/m,
phase + ': the review must be found at the ' + padded + ' path and reported');
}
});
test('the fence agrees with its callers across a probed set spanning both boundaries', { skip: !HAS_BASH }, () => {
// #3861 round 11. The first cut of this test was named "congruence, not merely wider" and
// probed 14 ids, none of them near the length bound. It passed, and the property it named
// was false: the shipped fence is deliberately NARROWER than the callers' regex, because
// the two `?????????*` checks bound the integer part and the suffix to 8 characters each.
// That overclaim was caught by this round's second adversarial review, which drove
// `123456789` and `1.1234567.1` — both caller-valid, both fence-refused.
//
// EXAMPLE-BASED, and the name says so rather than promising a language-level invariant.
// A finite probe set cannot prove congruence over an infinite language: the third review
// pass demonstrated this by injecting a `2) _ok=0` arm into the fence, which this test
// still passed because `2` is not in the list below. Read it as a regression pin over the
// values that actually broke, not as an exhaustive equivalence proof.
//
// It asserts two things, in two parts:
// (1) WITHIN the length bound, the fence and the callers agree exactly -- that is what
// round 11's shape fix bought, and the regression worth pinning.
// (2) BEYOND it, the fence refuses ids the callers accept. That divergence is
// PRE-EXISTING and untouched by this round (the bound predates the N-segment work
// and guards `$((10#...))` against bash's 2^64 wrap); it is pinned here so it stays
// a KNOWN narrowing rather than drifting back into an accidental one.
// The callers' regex since #4744: the letter axis is part of the agreement now.
const CALLER_RE = /^[0-9]+[A-Z]?(\.[0-9]+)*$/;
const refused = (v) => {
const out = runShippedGateCounts({ reviewText: '', writeReview: false, phaseNumber: v });
assert.strictEqual(out.exitCode, 0, v + ': advisory -- must not abort');
return /skipped \(unusable phase number/.test(out.stdout);
};
// (1) agreement, for every id whose components are each within the bound
for (const v of ['1', '03', '03.1', '23.1.2', '1.2.3.4', '12345678', '1.12345678',
'1.1234567.1', '12345678.12345678', '1.1.1.1.1.1.1.1.1.1',
'12A', '3A', '23A.1.2', '12345678A', '23a', 'A23', '2A3', '23AB', '23.1A',
'1..2', '1.2.', '.1', '3.', 'abc', '-1', '+1', '3 1', '../../etc/passwd']) {
assert.strictEqual(refused(v), !CALLER_RE.test(v),
v + ': with every component within the bound, step and callers must agree');
}
// (2) the remaining deliberate narrowing: a SINGLE component over 8 characters.
// This is the `$((10#...))` overflow guard and it is NOT a congruence defect -- bash
// integers wrap at 2^64, so an unbounded integer segment silently becomes a negative
// padded phase. Pinned so the narrowing stays known rather than drifting back.
for (const v of ['123456789', '1.999999999', '123456789A']) {
assert.ok(CALLER_RE.test(v), v + ': precondition -- the callers do accept this');
assert.ok(refused(v),
v + ': the per-component 8-char bound must keep refusing this; if this flips, the '
+ 'bound changed and `$((10#...))` overflow protection needs re-deriving');
}
});
test('the length bound is PER COMPONENT, not over the whole tail after the first dot', { skip: !HAS_BASH }, () => {
// #3861 round 11, C1. The bound used to read `${_pn#*.}` -- the entire suffix -- which is
// one component only while an id has at most two. The moment N-segment ids were accepted,
// that form rejected `1.1234567.1`: every component is a legal 7 digits, but the tail
// measures 9 characters. The comment above the check had promised per-component bounding
// since before this PR; the code only became untrue of it when the arity arm came out.
//
// Drives the boundary from both sides on a LATER segment, which is the part the old form
// got wrong -- an 8-char middle segment must pass and a 9-char one must fail, with the
// total length in both cases well past what the old whole-tail bound allowed.
const cases = [
['1.12345678.1', false, 'an 8-char middle segment is within the per-component bound'],
['1.123456789.1', true, 'a 9-char middle segment exceeds it'],
['1.1234567.1', false, 'the id the whole-tail bound rejected for its total length'],
['12345678.12345678', false, 'two 8-char components, 17 characters total'],
];
for (const [phase, mustRefuse, why] of cases) {
const out = runShippedGateCounts({ reviewText: '', writeReview: false, phaseNumber: phase });
assert.strictEqual(out.exitCode, 0, phase + ': advisory -- must not abort');
assert.strictEqual(/skipped \(unusable phase number/.test(out.stdout), mustRefuse,
phase + ': ' + why);
}
});
test('an integer phase is still zero-padded exactly as before', { skip: !HAS_BASH }, () => {
// Negative control for the split: the ordinary path must be untouched.
const review = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---', '',
'### CR-01: a finding'].join('\n');
const out = runShippedGateCounts({ reviewText: review, padded: '01' });
assert.strictEqual(out.exitCode, 0);
assert.match(out.stdout, /^Code review: 1 findings/m);
});
});
describe('#3861 round 1 — the counts mirror is asserted against the shipped shell', () => {
// Every fixture the mirror is exercised on above, plus the count edges.
const FIXTURES = {
'the documented review': REVIEW_WITH_FINDINGS,
'blocker: as the critical tier-equivalent': REVIEW_WITH_FINDINGS.replace(' critical: 1', ' blocker: 1'),
'a body ---, status: and total: after the frontmatter':
REVIEW_WITH_FINDINGS + '\n\n---\n\nstatus: clean\ntotal: 999\n',
'a legacy review with no findings: block':
['---', 'phase: 02', 'status: issues_found', '---', '', '# Phase 02'].join('\n'),
'CRLF line endings': REVIEW_WITH_FINDINGS.replace(/\n/g, '\r\n'),
'unterminated frontmatter': ['---', 'status: issues_found', 'total: 4', '', '## Body'].join('\n'),
'no frontmatter at all': '# Phase 01\n\nnothing here\n',
'a zero-finding review': ['---', 'phase: 01', 'findings:', ' critical: 0', ' warning: 0',
' info: 0', ' total: 0', 'status: issues_found', '---'].join('\n'),
// Both from the round-1 adversarial pass: the shipped pipeline collapses `1 0` to `10` where
// a trim keeps `1 0`, and truncates at a second colon where a tail capture keeps it. Neither
// is reachable from the well-formed fixtures above, which is exactly why they are here.
'a count with an internal space': REVIEW_WITH_FINDINGS.replace(' critical: 1', ' critical: 1 0'),
// The fixture that can actually SEE a mirror/shipped parser divergence. The two above cannot:
// on them both parsers reach the countless arm, so the parity assertion holds either way. Here
// the repaired numbers are SELF-CONSISTENT (10 == 10 + 0 + 0), so the retired parser renders a
// full breakdown from a `findings:` block containing no such numbers while the shipped one
// withholds it.
'a self-consistent repaired breakdown':
['---', 'phase: 01', 'status: issues_found', 'findings:', ' critical: 1 0',
' warning: 0', ' info: 0', ' total: 1 0', '---', '', '### CR-01: a'].join('\n'),
// Tab-separated scalars are valid YAML. The retired `tr -d ' '` left the tab in place and made
// every count non-numeric; the shipped trim reads them, so this fixture also pins that change.
'tab-separated counts':
['---', 'phase: 01', 'status: issues_found', 'findings:', ' critical:\t1',
' warning:\t1', ' info:\t1', ' total:\t3', '---', '', '### CR-01: a'].join('\n'),
// The STATUS axis of the same class, and the fixture that pins the `first` helper. Every
// pre-existing status fixture left both parsers on the SAME arm -- `issues:found` truncates to
// `issues`, which is no more `clean` than `issues:found` is -- so the mirror's status read could
// drift from the shipped one unseen, exactly as `firstIn` did. Here truncation FLIPS the arm:
// `clean:junk` cut at the second colon is the silent `clean`, whole it reports.
'a status whose truncation would flip the arm':
REVIEW_WITH_FINDINGS.replace('status: issues_found', 'status: clean:junk'),
// Unicode whitespace is NOT trimmed, because the reads pin LC_ALL=C. Unpinned under glibc's
// C.UTF-8 the shipped sed trimmed U+2003 and this took the silent clean arm on some machines
// and not others; the mirror never trims it. Same class as the fixture above, locale axis.
'a status with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace('status: issues_found', 'status: clean\u2003'),
'a count with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace(' critical: 1', ' critical: 1\u2003'),
'a findings: opener with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace('findings:', 'findings:\u2003'),
'a value containing a second colon': REVIEW_WITH_FINDINGS.replace('status: issues_found', 'status: issues:found'),
// POSIX [[:space:]] covers form feed and vertical tab; a [ \t] mirror does not, so the
// shipped grep matches a line the mirror rejects outright. Third counterexample, same class.
'a key indented with a form feed': REVIEW_WITH_FINDINGS.replace(' critical: 1', '\fcritical: 1'),
// Minor 1: a TOP-LEVEL key sharing a name with a nested count. The reads were scoped to the
// frontmatter but not to the `findings:` mapping the values belong to, so `^[[:space:]]*total:`
// matched this one first and the gate reported a number from outside the breakdown.
'a top-level total: ahead of the nested one':
['---', 'phase: 02', 'total: 999', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---'].join('\n'),
'a top-level info: and critical: ahead of the nested ones':
['---', 'critical: 42', 'info: 7', 'status: issues_found', 'findings:',
' critical: 1', ' warning: 0', ' info: 0', ' total: 1', '---'].join('\n'),
};
for (const [name, reviewText] of Object.entries(FIXTURES)) {
test('shipped shell and mirror agree on ' + name, { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({ reviewText });
// Parity over the WHOLE emitted message, not over five intermediate variables the test
// used to print for itself. A drift in any parsed value changes this string or the arm
// it selects, so the assertion binds to what an operator actually sees.
assert.strictEqual(
shipped.stdout,
renderGateMessage(parseGateCounts(reviewText), 1),
'the mirror has drifted from the shipped awk/grep block'
);
});
}
// The parity fixtures above run under whatever locale the suite inherits, so they can only catch a
// locale bug on a machine that happens to have it. This drives the SAME input under both locales and
// asserts the shipped fence does not care -- which is the actual property LC_ALL=C buys. Found by the
// round's fifth adversarial pass: glibc's C.UTF-8 classifies U+2003 as [[:space:]] AND [[:blank:]]
// where C and en_US.UTF-8 classify it as neither, so before the pin `status: clean<U+2003>` trimmed
// to the silent `clean` on some machines and reported on others.
for (const [name, reviewText] of Object.entries({
'a status with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace('status: issues_found', 'status: clean\u2003'),
'a count with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace(' critical: 1', ' critical: 1\u2003'),
'a unicode-space indented count key':
REVIEW_WITH_FINDINGS.replace(' critical: 1', '\u2003critical: 1'),
// Pass 6's finding, and the one input that proves the awk was missed: the BLOCK OPENER. Under
// C.UTF-8 `findings:<U+2003>` matched `/^findings:[[:space:]]*$/` and opened the mapping, under
// C it did not -- so the same review rendered a full breakdown on one machine and the countless
// message on another, with every grep and sed already pinned.
'a findings: opener with a trailing unicode space':
REVIEW_WITH_FINDINGS.replace('findings:', 'findings:\u2003'),
'the documented review': REVIEW_WITH_FINDINGS,
})) {
test('the shipped reads are locale-invariant on ' + name, { skip: !HAS_BASH }, () => {
const c = runShippedGateCounts({ reviewText, extraEnv: { LC_ALL: 'C', LANG: 'C' } });
const utf8 = runShippedGateCounts({ reviewText, extraEnv: { LC_ALL: 'C.UTF-8', LANG: 'C.UTF-8' } });
assert.strictEqual(c.exitCode, 0, 'advisory: must not abort under any locale');
assert.strictEqual(utf8.exitCode, 0, 'advisory: must not abort under any locale');
assert.strictEqual(utf8.stdout, c.stdout,
'the shipped reads must not depend on the ambient locale; drop LC_ALL=C and this reds');
// And the mirror predicts that one locale-independent answer.
assert.strictEqual(c.stdout, renderGateMessage(parseGateCounts(reviewText), 1),
'the mirror must model the locale-pinned shipped read');
});
}
test('every locale-sensitive tool in the step is pinned to LC_ALL=C', () => {
// THE CHECK THAT WOULD HAVE CAUGHT THE LAST MISS. `grep`, `sed` and `awk` all resolve
// [[:space:]] / [[:blank:]] through the ambient locale, and glibc's C.UTF-8 classifies U+2003
// as both where C and en_US.UTF-8 classify it as neither. Pass 5 pinned the grep and sed reads
// and the round then CLAIMED the parser was locale-independent; pass 6 found the two `awk`
// mapping selectors still unpinned, because that census searched for the tools it expected
// rather than the tools that were there. Asserting the invariant over the file is that census
// in a form a future edit is far less likely to slip past.
//
// WHAT THIS IS, AND WHAT IT IS NOT. It is a REGRESSION GUARD against the accident that has now
// happened twice — a read added or edited without its pin, in a file where every existing read
// has one. It is NOT a proof, and it is deliberately not written as one. It scans TEXT, so it
// cannot see shell or JS command structure: a name that is not written literally (`$AWK "$f"`,
// a command composed as `a''wk`, a command name computed inside the embedded `node -e` block)
// is invisible to it, and so is an executable command substitution on a physical line that
// begins with `#` inside a multiline quoted argument, which the comment exemption below skips.
//
// Earlier versions of this comment tried to ENUMERATE those residuals. Three adversarial passes
// in a row then found one more each time, which is the actual lesson: the list cannot be closed,
// so a comment promising a closed list is false the moment someone is cleverer than it. The
// examples above are illustrations, not an inventory. Its two directions are NOT symmetric, and
// that asymmetry is the whole operating instruction: a report is ADJUDICABLE — read the reported
// line together with what precedes it, since the same text can be a command or an argument to one
// (` grep` is a call after `:` and a string after `printf '%s\n' \`), so the line alone does not
// always settle it — whereas SILENCE proves nothing at all,
// because the evasions above are silent and so is any evasion no one has thought of yet. So:
// investigate every report, and never read silence as proof that a new read is pinned.
//
// Driven, it does catch: unpinned, `env`-prefixed, wrongly-pinned (`LC_ALL=C.UTF-8`),
// path-qualified, line-initial, and literal-in-Node calls. Its false answers run loud rather
// than quiet — a trailing comment naming a tool, a tool name inside an awk program, or a path
// whose component starts with one (`bin/grep-wrapper`) would all trip it. That direction is the
// right one for a guard, and none of those shapes exists in the step today. (The illustration
// is deliberately not a `docs/`-prefixed path: lint-docs-guard-registration reads one of those
// as a real docs reference from this file and demands a baseline entry for it.)
// `splitLines`, not `split('\n')`: the repo's own lint bans the latter on readFileSync content
// (DEFECT.WINDOWS-CRLF-TEST-PORTABILITY), and a CRLF checkout would otherwise leave a stray
// `\r` on every line here.
const { splitLines } = require('../msd-core/bin/lib/text-lines.cjs');
const step = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
const offenders = [];
splitLines(step).forEach((line, i) => {
if (/^\s*#/.test(line)) return; // prose may name an unpinned form while explaining it
// Blank out the PINNED calls first, then anything left naming one of these tools is unpinned.
// Deliberately NOT keyed on a leading `|`: every call is piped today, but a guard that only
// sees pipes would wave through `awk '...' < "$f"` or `$(grep ...)`, and "correct for the
// shapes that happen to exist right now" is the exact property that let the awk selectors sit
// unpinned through a whole commit that claimed otherwise.
const rest = line.replace(/LC_ALL=C\s+(?:grep|sed|awk)\b/g, '');
// The preceding-char class deliberately does NOT shield `/` or `.`: an earlier version did,
// and `/usr/bin/awk '...' < "$f"` sailed through it. A path-qualified call is still a call.
// `parsed`, `passed` and `awkward` stay unmatched, because their tool substring is preceded
// or followed by a word character.
const leftover = rest.match(/(?:^|[^A-Za-z0-9_-])(?:grep|sed|awk)\b/g) || [];
for (const c of leftover) offenders.push(`${i + 1}: ${c.trim()} (unpinned)`);
});
assert.deepStrictEqual(offenders, [],
'every grep/sed/awk in the step must be LC_ALL=C-pinned; an unpinned one makes the parse '
+ 'depend on the machine, which is what pass 5 and pass 6 each found');
// `cut -d: -f2-` and `tr -d '\r'` are deliberately NOT pinned, and the exemption is principled
// rather than an oversight: neither resolves a character class or a collation. One splits on a
// single ASCII byte, the other deletes one literal byte.
assert.ok(/\|\s*cut -d: -f2-/.test(step), 'the cut reads are still the class-free shape');
});
test('a zero-finding review reports a real breakdown, not the countless fallback', { skip: !HAS_BASH }, () => {
// Minor 6. `0` is a number, so the gate must state `0 findings — 0 critical, …`
// rather than fall back. The `case` guard rejects the empty string and non-digits;
// a guard written against truthiness would reject this and say nothing at all.
const shipped = runShippedGateCounts({ reviewText: FIXTURES['a zero-finding review'] });
assert.strictEqual(shipped.stdout,
'Code review: 0 findings \u2014 0 critical, 0 warning, 0 info.\n'
+ 'Consider running: /msd:code-review 1 --fix\n',
'all four counts are numeric, so the breakdown is reported rather than withheld');
});
test('a partial findings: block makes the whole breakdown unavailable', { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({
reviewText: ['---', 'findings:', ' total: 4', 'status: issues_found', '---'].join('\n'),
});
assert.strictEqual(readGateMessage(shipped.stdout).countsOk, '0',
'a total without the three severities is not a breakdown');
assert.match(shipped.stdout, /^Code review found issues\.$/m,
'the countless form is what reaches the operator');
});
test('a missing REVIEW.md leaves the counts empty and does not abort', { skip: !HAS_BASH }, () => {
// Behavioural replacement for the `src.includes('if [ -f "$REVIEW_FILE" ] …')`
// assertion: under `set -e` an aborting block is what actually breaks the phase.
const shipped = runShippedGateCounts({ reviewText: '', writeReview: false });
assert.strictEqual(shipped.exitCode, 0, 'advisory: a missing review must not abort the step');
// An absent review yields an empty status, which is a NON-REPORTING arm: the gate says
// nothing at all rather than claiming a countless review. Asserting on the observable is
// what makes that distinction visible; the old six-line probe could not express it.
assert.strictEqual(shipped.stdout, '', 'no review, no message');
assert.strictEqual(readGateMessage(shipped.stdout).reported, false);
});
test('the countless fallback requires all four counts, not just the total', { skip: !HAS_BASH }, () => {
// Fifth `src.includes()` assertion converted to a behavioural one (round 1 retired four).
// It pinned the PROSE that stated the condition, so it went red the moment the emitter moved
// into the fence and the prose was rewritten — while the behaviour it named was untouched.
// That is the pin arguing for its own conversion: the arm is now executed and observable, so
// assert the arm. Contrast is the point — a bare total takes the countless arm, the full set
// takes the breakdown arm — which a one-sided assertion could not express.
const fm = (rows) => ['---', 'findings:', ...rows, 'status: issues_found', '---'].join('\n');
const totalOnly = runShippedGateCounts({ reviewText: fm([' total: 4']) });
assert.match(totalOnly.stdout, /^Code review found issues\.$/m,
'a numeric total with missing severities must not emit a half-filled breakdown');
assert.doesNotMatch(totalOnly.stdout, /findings —/,
'and must not emit the breakdown form at all');
const allFour = runShippedGateCounts({
reviewText: fm([' critical: 1', ' warning: 2', ' info: 1', ' total: 4']),
});
assert.match(allFour.stdout, /^Code review: 4 findings — 1 critical, 2 warning, 1 info\.$/m,
'all four present and consistent is what the breakdown arm requires');
});
// The guard this pair covers is `[ -f "$REVIEW_FILE" ] && [ -r "$REVIEW_FILE" ]`, and the two
// legs need different fixtures because only one of them survives root.
//
// `chmod 0o000` does NOT make a file unreadable to root: root bypasses POSIX read permission
// bits, so `[ -r ]` stays true, the fence reads the fixture, and the assertion below sees the
// real breakdown instead of silence. That is not hypothetical here — the bench runs this suite
// as root under Docker, where this test failed with
// actual: 'Code review: 4 findings — 1 critical, 2 warning, 1 info.\n...'
// against an expected ''. CLAUDE.md names the mode-bit trick as the wrong tool for injecting an
// IO failure, and the usual remedy — monkeypatch the read to throw EACCES — does not reach this
// site: the read is performed by a spawned `bash`, not by this process, so stubbing node's `fs`
// is not on the code path at all.
//
// So the `-r` leg keeps the mode-bit fixture and declares the lanes it cannot bind on, exactly
// as tests/plan-review-convergence.test.cjs does for its own shell-side `-r` arm, and the `-f`
// leg below carries the contract on every lane INCLUDING root. Skipping the first without
// adding the second would have traded a false failure for lost coverage.
const SKIP_MODE_BITS = !HAS_BASH
? 'POSIX-only bash fragment'
: typeof process.getuid === 'function' && process.getuid() === 0
? 'root bypasses the read permission bit'
: false;
test('an unreadable REVIEW.md leaves the counts empty and does not abort', { skip: SKIP_MODE_BITS }, () => {
const shipped = runShippedGateCounts({ reviewText: REVIEW_WITH_FINDINGS, mode: 0o000 });
assert.strictEqual(shipped.exitCode, 0, 'advisory: an unreadable review must not abort the step');
assert.strictEqual(shipped.stdout, '', 'an unreadable review reports nothing, and does not guess');
});
// Root-immune companion: a directory fails `-f` for every euid, so this binds on the bench lane
// where the test above is skipped. Same arm, same observable — silence and a zero exit.
test('a directory standing in for REVIEW.md leaves the counts empty and does not abort', { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({ reviewText: REVIEW_WITH_FINDINGS, plantDir: true });
assert.strictEqual(shipped.exitCode, 0, 'advisory: a non-regular review path must not abort the step');
assert.strictEqual(shipped.stdout, '', 'a review path that is not a regular file reports nothing, and does not guess');
assert.strictEqual(readGateMessage(shipped.stdout).reported, false);
});
// An EMPTY REGULAR FILE is a third arm, and it is NOT the missing case: `-f` and `-r` both pass,
// so the fence opens and reads the file where the missing case never gets past the guard. The
// missing-file test above cannot reach it — it passes `writeReview: false`, so no file exists at
// all — and until this test every `reviewText: ''` call in this file did the same. The PR body
// has claimed this case since round 1; it was documented as covered and was not covered.
//
// WHY the scan yields nothing, stated precisely because the obvious reading is wrong: it is NOT
// the `NR==1{if($0!="---") exit}` guard. A zero-byte file supplies awk no record at all, so that
// action never executes (NR stays 0). The output is empty because `closed` is never set and the
// END block therefore prints nothing.
//
// Since round 11 the observable is NOT the missing-file case's any more, and that is the point:
// a zero-byte REVIEW.md was read and has no parseable status, so the fence says so rather than
// staying silent -- silence is what a clean review looks like. The counts are still empty and
// nothing is guessed; what changed is that the operator is told the report could not be read.
test('an EMPTY REVIEW.md leaves the counts empty, reports the unparseable status, and does not abort', { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({ reviewText: '' }); // writeReview defaults true: a real, empty file
assert.strictEqual(shipped.exitCode, 0, 'advisory: an empty review must not abort the step');
assert.match(shipped.stdout, /^Code review status unparsed: /m, 'an empty file was read, and says so');
assert.doesNotMatch(shipped.stdout, /^Code review: \d+ findings/m, 'and no breakdown is invented');
assert.strictEqual(readGateMessage(shipped.stdout).unparsed, true);
});
});
describe('#3861 round 11 — a malformed report does not read as clean in the counting arm', () => {
// A REVIEW.md with three criticals and an UNTERMINATED frontmatter yielded REVIEW_STATUS='',
// and the counting arm then printed nothing -- byte-identical to a clean review. Block 2 still
// said `status: none` rather than `clean`, so a careful reader could separate them downstream,
// which is the only reason this was Minor. The counting arm now distinguishes 'no findings'
// from 'could not parse', and block 2 names the same distinction ('unparsed' vs 'none').
const UNTERMINATED = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 3', ' warning: 0', ' info: 0', ' total: 3', '',
'## Critical Issues', '', '### CR-01: a', '### CR-02: b', '### CR-03: c'].join('\n');
test('an unterminated frontmatter is reported as unparsed, not passed over in silence', { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({ reviewText: UNTERMINATED });
assert.strictEqual(shipped.exitCode, 0, 'advisory: a malformed review must not abort the step');
assert.match(shipped.stdout, /^Code review status unparsed: REVIEW\.md is present but its frontmatter has no parseable status; severity counts unavailable\.$/m);
assert.doesNotMatch(shipped.stdout, /^Code review: \d+ findings/m, 'a breakdown read from an unterminated block would be the body leak this scan prevents');
assert.doesNotMatch(shipped.stdout, /Consider running/, 'nothing here proves there are findings to fix');
assert.strictEqual(readGateMessage(shipped.stdout).unparsed, true);
});
test('a closed frontmatter with no status: key is the same arm', { skip: !HAS_BASH }, () => {
const noStatus = ['---', 'phase: 01', 'findings:', ' critical: 3', ' warning: 0', ' info: 0',
' total: 3', '---', '', '### CR-01: a'].join('\n');
const shipped = runShippedGateCounts({ reviewText: noStatus });
assert.match(shipped.stdout, /^Code review status unparsed: /m);
});
test('a review with no frontmatter at all is the same arm', { skip: !HAS_BASH }, () => {
const shipped = runShippedGateCounts({ reviewText: '# Phase 01\n\n### CR-01: a finding\n' });
assert.match(shipped.stdout, /^Code review status unparsed: /m);
});
test('the absent, directory and unreadable cases stay silent — nothing was read, so nothing is described', { skip: !HAS_BASH }, () => {
// The distinction is READ-ness, not emptiness: the three guard-refused shapes have no file
// content to describe, and describing one would be the guess the guard exists to prevent.
// (The mode-bit case is pinned by its own root-aware test above.)
assert.strictEqual(runShippedGateCounts({ reviewText: '', writeReview: false }).stdout, '', 'absent');
assert.strictEqual(runShippedGateCounts({ reviewText: UNTERMINATED, plantDir: true }).stdout, '', 'directory');
});
test('block 2 names the same distinction: unparsed for a read file, none for an absent one', { skip: !HAS_BASH }, () => {
const script = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'))[1];
const runBlock2 = (dir, padded) => runHook('-c', ['set -euo pipefail\n' + script + '\n'], {
interpreter: 'bash', timeoutMs: PROBE_TIMEOUT_MS,
env: { ...process.env, PHASE_DIR: dir, PHASE_NUMBER: String(Number(padded)) },
});
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-unparsed-'));
try {
// No REVIEW.md, no ledger, no fix report -> skipped (status: none)
let res = runBlock2(dir, '01');
assert.strictEqual(res.exitCode, 0);
assert.match(res.stdout, /^Code review disposition skipped \(status: none\)$/m, 'absent reads as none');
fs.writeFileSync(path.join(dir, '01-REVIEW.md'), UNTERMINATED);
res = runBlock2(dir, '01');
assert.strictEqual(res.exitCode, 0);
assert.match(res.stdout, /^Code review disposition skipped \(status: unparsed\)$/m, 'a read-but-unparseable review reads as unparsed');
assert.doesNotMatch(res.stdout, /status: none/);
} finally {
cleanup(dir);
}
});
});
describe('#3861 round 12 — block 2 does not compute a shortfall from a self-contradicting findings block', () => {
// Round 12, Minor. Block 1 withholds the severity breakdown unless the four counts are numeric
// AND `critical + warning + info == total`; block 2 bounded `total` for digits and length only
// and then handed it to the `unparsed:` reconciliation. So a REVIEW.md whose `findings:` block
// disagrees with itself made block 1 print the countless form -- breakdown suppressed as
// untrustworthy -- while block 2 still computed a shortfall from that same untrusted number.
// Two trust models for one field, one fence apart, with the weaker one downstream.
//
// The fix is NARROWER than "re-apply block 1's check", deliberately: block 1 demands all four
// counts because it DISPLAYS all four. Block 2 uses `total` alone. Applying the all-four rule
// here would blank a perfectly usable `total: 5` on a review carrying no severity keys and
// SILENTLY DROP a shortfall the step reports correctly today -- trading a safe-direction
// over-report for a silent under-report. Only the CONTRADICTION ports.
const headings = ['', '### CR-01: a conforming finding', '### WR-01: another', '### WR-02: a third'];
const review = (fm) => ['---', 'phase: 01', 'status: issues_found', ...fm, '---', ...headings].join('\n');
const runBlock2 = (dir) => {
const script = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'))[1];
return runHook('-c', ['set -euo pipefail\n' + script + '\n'], {
interpreter: 'bash', timeoutMs: PROBE_TIMEOUT_MS,
env: { ...process.env, PHASE_DIR: dir, PHASE_NUMBER: '1' },
});
};
const drive = (fm, status) => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-r12-'));
try {
const body = status === undefined ? review(fm)
: review(fm).replace('status: issues_found', 'status: ' + status);
fs.writeFileSync(path.join(dir, '01-REVIEW.md'), body);
const res = runBlock2(dir);
assert.strictEqual(res.exitCode, 0, 'the fence is advisory and must never abort: ' + res.stderr);
const p = path.join(dir, '01-REVIEW-DISPOSITION.md');
return { stdout: res.stdout, ledger: fs.existsSync(p) ? fs.readFileSync(p, 'utf8') : null };
} finally {
cleanup(dir);
}
};
test('a contradicting findings block yields no shortfall (fails before the fix)', { skip: !HAS_BASH }, () => {
// total: 10 against 1 + 1 + 1. Three headings parse. Before the fix this rendered
// `unparsed: 7` from a number block 1 had already judged untrustworthy.
const out = drive(['findings:', ' critical: 1', ' warning: 1', ' info: 1', ' total: 10']);
assert.doesNotMatch(out.ledger, /^unparsed:/m, 'a self-contradicting total is not a number to reconcile against');
assert.doesNotMatch(out.stdout, /recorded NOWHERE/);
assert.match(out.ledger, /^total: 3$/m, 'and the rows it does have are still reported');
});
test('a CONSISTENT total still reconciles — the check narrows nothing it should not', { skip: !HAS_BASH }, () => {
// 1 + 2 + 2 == 5, three headings parse, so two are recorded nowhere and must be said.
const out = drive(['findings:', ' critical: 1', ' warning: 2', ' info: 2', ' total: 5']);
assert.match(out.ledger, /^unparsed: 2$/m);
assert.match(out.stdout, /2 finding\(s\) recorded NOWHERE/);
});
test('a total with NO severity keys still reconciles — absent is not a contradiction', { skip: !HAS_BASH }, () => {
// The over-reach control. There is nothing for `total: 5` to disagree WITH here, so the
// shortfall this step reports correctly today must survive the new check.
const out = drive(['findings:', ' total: 5']);
assert.match(out.ledger, /^unparsed: 2$/m, 'absent counts must not suppress a real shortfall');
});
test('a partial or non-numeric breakdown is likewise not a contradiction', { skip: !HAS_BASH }, () => {
for (const fm of [
['findings:', ' critical: 1', ' total: 5'],
['findings:', ' critical: x', ' warning: 1', ' info: 1', ' total: 5'],
]) {
assert.match(drive(fm).ledger, /^unparsed: 2$/m, JSON.stringify(fm));
}
});
test('`blocker:` is read as the critical tier, exactly as block 1 reads it', { skip: !HAS_BASH }, () => {
// A mirror that dropped the documented alternation would diverge from block 1 on precisely
// the reviews that use it: 1 + 1 + 1 == 3 here, so the sum agrees and nothing is suppressed.
const out = drive(['findings:', ' blocker: 1', ' warning: 1', ' info: 1', ' total: 3']);
assert.doesNotMatch(out.ledger, /^unparsed:/m, 'blocker counted as critical => the sum agrees');
// And the contradicting twin, to prove the alternation is load-bearing rather than inert.
const bad = drive(['findings:', ' blocker: 1', ' warning: 1', ' info: 1', ' total: 9']);
assert.doesNotMatch(bad.ledger, /^unparsed:/m);
});
test('a severity with an INTERNAL space is malformed, and does not suppress the shortfall', { skip: !HAS_BASH }, () => {
// Found by the round's own adversarial pass, against the first version of this fix. The sibling
// reads use `cut -d: -f2 | tr -d ' '`, which deletes INTERNAL spaces too, so `critical: 1 0`
// arrives as the perfectly numeric `10`. That is long-standing in those reads and was INERT here
// until this block started reading the severities -- at which point a repaired number could
// satisfy the sum test and SUPPRESS a real `unparsed:` shortfall. The severity reads now trim the
// ends only, so the space survives, the digit check rejects it, and nothing is suppressed.
const out = drive(['findings:', ' critical: 1 0', ' warning: 0', ' info: 0', ' total: 5']);
assert.match(out.ledger, /^unparsed: 2$/m, 'a malformed severity must not license suppression');
});
test('a scalar with a SECOND COLON is malformed, and the well-formed total still reconciles', { skip: !HAS_BASH }, () => {
// Second adversarial pass. `cut -d: -f2` takes only the SECOND FIELD, so `critical: 1: junk`
// arrived as the perfectly numeric `1` -- 1+0+0 != 5 read as a contradiction and SUPPRESSED a
// shortfall that was genuinely owed. `-f2-` keeps everything after the first colon, so the
// malformed scalar stays malformed, no contradiction is claimed, and `total: 5` still reconciles.
const out = drive(['findings:', ' critical: 1: junk', ' warning: 0', ' info: 0', ' total: 5']);
assert.match(out.ledger, /^unparsed: 2$/m, 'a malformed severity must not license suppression');
});
test('a malformed TOTAL is not repaired into a fabricated shortfall', { skip: !HAS_BASH }, () => {
// Second adversarial pass, and the sharper of the two. The first version of this fix parsed the
// severities strictly and left `total` lenient -- so `critical: 5 0` with `total: 1 0` repaired
// ONLY the total to `10`, rejected the severity, skipped the contradiction check, and INVENTED
// `unparsed: 7` against three parsed headings. A field is either trustworthy or it is not:
// parsing one leniently and its sibling strictly is the shape that fabricates. Every count this
// block reads now goes through one parser.
const out = drive(['findings:', ' critical: 5 0', ' warning: 0', ' info: 0', ' total: 1 0']);
assert.doesNotMatch(out.ledger, /^unparsed:/m, 'a malformed total is not a number to reconcile against');
assert.doesNotMatch(out.stdout, /recorded NOWHERE/);
});
test('BLOCK 1 withholds a breakdown built from REPAIRED counts', { skip: !HAS_BASH }, () => {
// The one input that distinguishes block 1's old parser from its new one, and the reason this
// test exists: the round's first negative control for the block-1 change was VACUOUS. Reverting
// block 1 to `cut -d: -f2 | tr -d ' '` left the whole suite green, because on every fixture that
// existed both parsers landed on the SAME arm -- the countless form -- so a parity assertion
// could not see the difference.
//
// A SELF-CONSISTENT repaired breakdown separates them. `critical: 1 0` / `total: 1 0` repairs to
// 10 and 10, which SUM, so the old parser reported `10 findings -- 10 critical, 0 warning,
// 0 info.` from a `findings:` block that contains no such numbers. The new parser rejects the
// scalars and takes the countless arm, which is also what block 2 does with the same bytes --
// a console line and a ledger can no longer contradict each other on this input.
const repaired = ['---', 'phase: 01', 'status: issues_found', 'findings:',
' critical: 1 0', ' warning: 0', ' info: 0', ' total: 1 0', '---', '',
'### CR-01: a', '### WR-01: b', '### WR-02: c'].join('\n');
const shipped = runShippedGateCounts({ reviewText: repaired });
assert.strictEqual(shipped.exitCode, 0, 'advisory: never abort');
assert.doesNotMatch(shipped.stdout, /10 findings/, 'a repaired number must not be reported as a count');
assert.doesNotMatch(shipped.stdout, /10 critical/);
assert.match(shipped.stdout, /^Code review found issues\./m, 'the countless arm is the honest one here');
});
test('a status scalar carrying a second colon does not silently take the clean arm', { skip: !HAS_BASH }, () => {
// Fourth adversarial pass, MISSED. `status:` kept the retired `cut -d: -f2` after the counts moved
// off it, and it is the read where truncation costs most: the valid YAML scalar `status: clean:junk`
// arrived as the bare `clean`, so an unusable status took the CLEAN arm and suppressed both the
// report and the ledger. The whole scalar matches no arm, so the step reports instead.
const out = drive(['findings:', ' critical: 1', ' warning: 0', ' info: 0', ' total: 1'], 'clean:junk');
assert.ok(out.ledger !== null, 'an unusable status must not suppress the ledger');
});
test('a status with trailing unicode whitespace does not silently take the clean arm', { skip: !HAS_BASH }, () => {
// Fifth adversarial pass. The ledger half of the locale finding: unpinned, glibc's C.UTF-8 trimmed
// U+2003 and `status: clean<U+2003>` suppressed the ledger on exactly the machines whose locale
// said so. Pinned to C the scalar stays unusable, matches no arm, and the ledger is written.
const out = drive(['findings:', ' critical: 1', ' warning: 0', ' info: 0', ' total: 1'], 'clean\u2003');
assert.ok(out.ledger !== null, 'an unusable status must not suppress the ledger under any locale');
});
test('the parser is symmetric across all four count fields', { skip: !HAS_BASH }, () => {
// Third adversarial pass, MISSED: the fixtures exercised malformed `critical` and `total` only,
// so they did not actually pin the four-field symmetry the fix claims. Each field in turn gets
// each malformed shape; none may license suppression, because a malformed severity is not a
// disagreement and `total: 5` stays usable.
for (const key of ['critical', 'blocker', 'warning', 'info']) {
for (const bad of ['1 0', '1: junk']) {
const fm = ['findings:', ' total: 5'];
for (const k of ['critical', 'warning', 'info']) {
if (k === key || (key === 'blocker' && k === 'critical')) continue;
fm.push(` ${k}: 0`);
}
fm.push(` ${key}: ${bad}`);
assert.match(drive(fm).ledger, /^unparsed: 2$/m, `${key}: '${bad}' must not license suppression`);
}
}
});
test('an absent severity still proves a contradiction when the known ones OVERSHOOT', { skip: !HAS_BASH }, () => {
// Third adversarial pass. Counts are non-negative, so a missing one can only ADD: when the
// present severities already sum to MORE than `total`, the block disagrees with itself whatever
// the absent value is. Requiring all three before comparing reconciled against a total the
// present counts had already refuted.
const over = drive(['findings:', ' critical: 4', ' warning: 4', ' total: 5']);
assert.doesNotMatch(over.ledger, /^unparsed:/m, '4 + 4 > 5 is a contradiction with or without info:');
// The other direction stays reconcilable: an UNDERshoot is exactly what the absent count explains.
const under = drive(['findings:', ' critical: 1', ' warning: 1', ' total: 5']);
assert.match(under.ledger, /^unparsed: 2$/m, 'an undershoot is the absent count\'s job, not a contradiction');
});
test('a zero-padded breakdown does not take the advisory step down', { skip: !HAS_BASH }, () => {
// `10#` on every operand: bash reads a leading zero as octal, so `critical: 08` would make
// $(( )) fail with "value too great for base" and, under `set -e`, abort a step that
// promises never to block. 8 + 1 + 1 == 10, so the sum agrees and the shortfall is reported.
const out = drive(['findings:', ' critical: 08', ' warning: 01', ' info: 01', ' total: 10']);
assert.match(out.ledger, /^unparsed: 7$/m, 'octal-looking counts are read as decimal, not as an abort');
});
});
// ---------------------------------------------------------------------------
// #3861 round 1 — Minor 5, and the finding-id census the review did not ask for
// ---------------------------------------------------------------------------
const REVIEWER_AGENT_PATH = path.join(ROOT, 'agents', 'msd-code-reviewer.md');
// The alternations the shipped script uses to recognise a finding id. There are three —
// the heading matcher, the ledger row re-parser, and the frontmatter `- id:` matcher —
// and adding a prefix to only some of them is silent. The severity map below is a fourth
// copy of the same set; it is not an alternation, so it is extracted separately.
// This scan is by PATTERN, never a fixed list of sites, which is why round 5's new
// matcher was absorbed with no edit here. Do not convert it to an enumeration.
function idAlternations() {
const script = shippedDispositionScript();
return [...script.matchAll(/\(\?:((?:[A-Z]{2}\|)+[A-Z]{2})\)-/g)].map((m) => m[1].split('|').sort().join('|'));
}
// The FOURTH copy: the severity map's keys. It is not an alternation, so the extractor above cannot
// see it — and a set that agrees in the three regexes while mis-tiering in the map is the drift the
// guard would otherwise miss entirely.
// (Said THIRD until round 6. The extractor finds three alternations — round 5's frontmatter `- id:`
// matcher is the third — so the map has been the fourth copy since then. The count is prose only;
// nothing below reads it.)
function severityMapKeys() {
const script = shippedDispositionScript();
const m = script.match(/\{([^}]*?)\}\[id\.split/);
assert.ok(m, 'the severity map must still be an inline object literal indexed by the id prefix');
return [...m[1].matchAll(/([A-Z]{2}):/g)].map((x) => x[1]);
}
describe('#3861 round 1 — stale fix reports are stated, not silently ignored', () => {
test('a fix report naming a different finding under a reused id says so', () => {
// Exact-title coupling is deliberate — ids are reused across re-reviews, so a
// stale REVIEW-FIX.md must not mark a brand-new CR-01 fixed. But failing it
// silently leaves 'open' indistinguishable from 'the report never named it',
// which is the one thing the ledger exists to tell apart.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a genuinely new finding'].join('\n');
const fixText = ['## Fixed Issues', '', '### CR-01: the finding this id used to mean'].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText });
const rows = ledgerRows(out.ledger);
assert.strictEqual(rows[0].disposition, 'open', 'a stale report must not decide the row');
assert.match(out.stdout, /titles its finding differently from the review/,
'and the mismatch must be reported, not swallowed');
assert.match(out.stdout, /a stale report, or a re-titled one/,
'stated as the observation it is -- the step cannot tell the two causes apart');
assert.match(out.stdout, /CR-01/, 'naming the finding it could not reconcile');
});
test('a title differing only in INTRA-LINE whitespace still reconciles (m2)', () => {
// msd-code-fixer.md writes '### {finding_id}: {title}' under no contract that the title is
// copied byte-for-byte, so a fixer that re-spaces a title used to produce a spurious note and
// leave a genuinely-fixed row 'open'. Runs of spaces carry no information; they are collapsed.
// NAMED PRECISELY. An earlier version of this test called itself the "reflowed" case while
// substituting triple spaces, which is not a reflow -- see the bound pinned below.
const title = 'a long finding title a fixer might re-space';
const review = ['---', 'status: issues_found', '---', '', '### CR-01: ' + title].join('\n');
const respaced = ['## Fixed Issues', '', '### CR-01: ' + title.replace(/ /g, ' ')].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText: respaced });
assert.strictEqual(ledgerRows(out.ledger)[0].disposition, 'fixed',
'a re-spaced title is the same title, and the fix outcome must reach the ledger');
assert.doesNotMatch(out.stdout, /titles its finding differently/,
'and no spurious mismatch is reported');
});
test('a title WRAPPED across lines is not reconciled — the bound, pinned deliberately', () => {
// The limit of the m2 fix, stated rather than left to be discovered. A `###` heading is ONE
// line by definition: if a fixer wraps a long title, the continuation is a separate paragraph
// and the heading parser -- correctly -- captures only the first line. Whitespace collapsing
// cannot reach across that boundary.
//
// NOT widened, and the reason is that widening is the worse defect: to reconcile a wrapped
// title the parser would have to absorb whatever follows a heading into the title, which
// silently swallows arbitrary prose and would make the stale-report check meaningless. The
// failure mode kept here is the SAFE one -- a visible mismatch note and a row left open,
// never a wrong 'fixed'.
const review = ['---', 'status: issues_found', '---', '',
'### CR-01: a long finding title that wraps'].join('\n');
const wrapped = ['## Fixed Issues', '', '### CR-01: a long', 'finding title that wraps'].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText: wrapped });
assert.strictEqual(ledgerRows(out.ledger)[0].disposition, 'open',
'a wrapped heading does not reconcile -- and fails in the safe direction');
assert.match(out.stdout, /titles its finding differently/,
'the mismatch is reported rather than swallowed');
});
test('a RE-CASED or truncated title still reports a mismatch — the strict half is kept', () => {
// The deliberate residual. Case changes and truncation are the shapes a genuinely different
// finding takes, so widening to them would trade a visible false positive for a silent false
// negative -- a stale report marking a brand-new CR-01 fixed, which is the worse direction.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: The Finding'].join('\n');
const recased = ['## Fixed Issues', '', '### CR-01: the finding'].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText: recased });
assert.strictEqual(ledgerRows(out.ledger)[0].disposition, 'open');
assert.match(out.stdout, /titles its finding differently/);
});
test('a matching fix report reports no mismatch', () => {
// Negative control for the note itself: it must not fire on the ordinary path.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: same title'].join('\n');
const fixText = ['## Fixed Issues', '', '### CR-01: same title'].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText });
assert.strictEqual(ledgerRows(out.ledger)[0].disposition, 'fixed');
assert.doesNotMatch(out.stdout, /reused id/);
});
});
describe('#3861 round 1 — finding-id prefix census', () => {
test('every copy of the prefix set agrees with every other', () => {
// The set is written out FOUR times in one script — the heading matcher, the ledger
// row re-parser, the frontmatter `- id:` matcher the title tracking added, and (by its
// keys) the severity map. Adding a prefix to some of them does not error; it drops
// carried rows on the next run.
// The count is stated for the reader; nothing below depends on it. idAlternations()
// scans the script by PATTERN rather than walking a fixed site list, which is why the
// fourth site was absorbed without a change here — this comment is the only thing that
// fell behind, and a guard whose population is hand-listed is the defect it would have
// been. Do not convert this to an enumeration.
const alts = idAlternations();
assert.ok(alts.length >= 2, 'the script must still enumerate finding-id prefixes');
assert.strictEqual(new Set(alts).size, 1, 'the prefix enumerations have drifted apart: ' + alts.join(' vs '));
// And the fourth copy, which is not an alternation: every prefix the regexes admit must either
// carry an explicit tier in the severity map or fall to `info` by the documented default.
// Without this, the three regexes can gain a prefix while the map silently mis-tiers it.
const mapped = new Set(severityMapKeys());
const admitted = alts[0].split('|');
const unmapped = admitted.filter((p) => !mapped.has(p));
assert.deepStrictEqual(
unmapped, ['IN'],
'only IN may rely on the info default; every other admitted prefix needs an explicit tier'
);
// And the other direction, which a one-way check leaves open: a tier for a prefix the
// regexes never admit is dead code that reads as coverage.
const unadmitted = [...mapped].filter((p) => admitted.indexOf(p) === -1);
assert.deepStrictEqual(
unadmitted, [],
'the severity map tiers prefixes the id regexes do not admit: ' + unadmitted.join(',')
);
});
test('the prefix set covers every id shape the reviewer agent emits', () => {
// The DOMAIN is owned elsewhere — msd-code-reviewer.md's body template and its
// Label-equivalence paragraph — so it can acquire a member without this script
// changing. An unlisted prefix is not mis-tiered, it is INVISIBLE: the finding
// never enters the order list and gets no row at all.
const agent = fs.readFileSync(REVIEWER_AGENT_PATH, 'utf8');
// BOTH surfaces. The body template writes `### CR-01:` headings; the Label-equivalence
// paragraph defines BL in PROSE and appears in no heading at all (`### BL-` occurs zero
// times). A heading-only scan therefore passes today purely because BL happens to be
// hard-coded, and would miss the next prose-defined prefix exactly as it would miss BL.
const emitted = new Set([
...[...agent.matchAll(/^###\s+([A-Z]+)-\d+:/gm)].map((m) => m[1]),
...[...agent.matchAll(/\b([A-Z]+)-\s*(?:IDs?|prefix)/g)].map((m) => m[1]),
...[...agent.matchAll(/IDs? beginning with\s+`?([A-Z]+)-/g)].map((m) => m[1]),
]);
assert.ok(emitted.size > 0, 'the reviewer agent must still declare its finding-id shapes');
const known = new Set(idAlternations()[0].split('|'));
for (const prefix of emitted) {
assert.ok(known.has(prefix), prefix + '- findings would get no disposition row at all');
}
});
});
// ---------------------------------------------------------------------------
// #3861 round 1, second pass — defects found by adversarially reviewing the
// round's OWN fixes before pushing them. Every one of these was invisible to
// the maintainer's review and to the first pass above.
// ---------------------------------------------------------------------------
describe('#3861 round 1 — fence tracking, status gating, count consistency', () => {
test('a foreign fence marker inside a fenced example does not swap example for finding', () => {
// The worst shape this file has carried: a bare fenced/not-fenced toggle treats ``` and ~~~
// as interchangeable, so a ~~~ line inside a ``` example CLOSES the fence and the example's
// real close REOPENS one. The ledger then records the ILLUSTRATION and drops the finding —
// a confidently-written artifact that is wrong in both directions at once.
const review = ['---', 'status: issues_found', '---', '', '```', '~~~',
'### CR-77: an example inside a fence', '```', '', '### CR-01: a real finding'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01'], 'the real finding, and only it');
});
test('a longer close does not require an exact-length match, per CommonMark', () => {
const review = ['---', 'status: issues_found', '---', '', '```',
'### CR-77: fenced', '````', '', '### CR-01: real'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01']);
});
test('the disposition block gates on review status in shell, not in prose', { skip: !HAS_BASH }, () => {
// Block 1 computes REVIEW_STATUS and emits nothing, and its shell is discarded — so a
// condition stated only in the prose between the blocks is not available to anything. A
// clean re-review would otherwise rewrite a ledger it was never meant to touch.
const fences = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'));
assert.ok(fences.length >= 2, 'the step must still carry more than one shell block');
assert.match(fences[1], /REVIEW_STATUS/, 'block 2 must re-derive the status it is gated on');
assert.match(fences[1], /clean\|skipped/, 'and gate on the documented clean/skipped/empty set');
});
test('an internally inconsistent breakdown is withheld, not half-rendered', { skip: !HAS_BASH }, () => {
// `total: 0` beside `critical: 1` is four valid numbers producing a self-contradicting
// line. Numeric is necessary, not sufficient.
const shipped = runShippedGateCounts({
reviewText: ['---', 'findings:', ' critical: 1', ' warning: 0', ' info: 0',
' total: 0', 'status: issues_found', '---'].join('\n'),
});
assert.strictEqual(readGateMessage(shipped.stdout).countsOk, '0',
'the counts do not sum to the total, so no breakdown');
});
test('a consistent breakdown is still reported', { skip: !HAS_BASH }, () => {
// Negative control for the sum check — it must not withhold a correct breakdown.
const shipped = runShippedGateCounts({ reviewText: REVIEW_WITH_FINDINGS });
assert.strictEqual(readGateMessage(shipped.stdout).countsOk, '1');
});
test('a carried finding that REAPPEARS loses the carried marker', () => {
// The defect that storing the cell verbatim introduced, and the reason the strip is back.
// Run 1 carries CR-01 and marks it; run 2 reports CR-01 again. If the marker were permanent
// the ledger would state 'not in the current review' about a finding plainly in it —
// an artifact confidently wrong about its own contents.
const absent = ['---', 'status: issues_found', '---', '', '### WR-01: other'].join('\n');
const back = ['---', 'status: issues_found', '---', '', '### CR-01: it came back'].join('\n');
const prior = '| CR-01 | critical | deferred | waiting on ADR-9 |';
const run1 = runShippedDisposition({ reviewText: absent, priorText: prior });
assert.strictEqual(
ledgerRows(run1.ledger).find((r) => r.id === 'CR-01').source,
'waiting on ADR-9 (not in the current review)', 'carried, and marked as such'
);
const run2 = runShippedDisposition({ reviewText: back, priorText: run1.ledger });
const row = ledgerRows(run2.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(row.source, 'waiting on ADR-9', 'the marker goes when the finding returns');
assert.strictEqual(row.disposition, 'deferred', 'and the decision itself is still preserved');
});
test('a carried row still does not grow its marker across runs', () => {
// The property the strip exists for, re-pinned now that it is bounded.
const review = ['---', 'status: issues_found', '---', '', '### WR-01: unrelated'].join('\n');
const prior = '| CR-01 | critical | deferred | waiting on ADR-9 (not in the current review) |';
const first = runShippedDisposition({ reviewText: review, priorText: prior });
const carried = ledgerRows(first.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(carried.source, 'waiting on ADR-9 (not in the current review)');
const second = runShippedDisposition({ reviewText: review, priorText: first.ledger });
assert.strictEqual(
ledgerRows(second.ledger).find((r) => r.id === 'CR-01').source,
'waiting on ADR-9 (not in the current review)',
'exactly one marker, however many times the gate runs'
);
});
});
// ---------------------------------------------------------------------------
// #3861 round 1, third pass — the fidelity gap that let the second pass ship a
// regression the whole suite was green over.
//
// Every test above extracts the embedded script as TEXT and runs it. Bash does
// not: it expands the double-quoted `node -e "..."` argument first, so an
// unescaped backtick is COMMAND SUBSTITUTION and the script Node receives is
// not the script the tests read. That is not a hypothetical — the previous
// commit shipped exactly that, in a code comment, and 122 green tests said
// nothing because none of them ever asked bash what it would actually pass.
// ---------------------------------------------------------------------------
describe('#3861 round 1 — the tests must run what BASH would run', () => {
test('bash expansion of the node -e argument matches what the tests extract', { skip: !HAS_BASH }, () => {
// Ask bash for the literal argument it would hand node, and compare. This is the general
// guard: it catches an unescaped backtick, an unescaped $, and any other expansion the
// extractor's two-escape undo cannot model — none of which the behavioural tests can see.
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8').replace(/\r\n/g, '\n');
const open = src.indexOf('node -e "');
const body = src.slice(open + 'node -e "'.length);
const end = body.indexOf('\n" || echo ');
assert.ok(end !== -1, 'the node -e script must still be closed by its || echo fallback');
const quoted = body.slice(0, end);
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-fid-'));
try {
const out = path.join(dir, 'arg.txt');
// `printf %s` with the SAME double-quoted string the step uses: whatever bash does to it
// on the way to node, it does here too.
const probe = 'printf %s "' + quoted + '" > ' + JSON.stringify(out) + '\n';
const res = runHook('-c', [probe], { interpreter: 'bash', timeoutMs: PROBE_TIMEOUT_MS });
assert.strictEqual(res.outcome, OUTCOME.EXITED, 'the probe must run to completion');
assert.strictEqual(
res.stderr.trim(), '',
'bash emitted diagnostics expanding the node -e argument — an unescaped backtick or $: ' + res.stderr
);
assert.strictEqual(
fs.readFileSync(out, 'utf8'), shippedDispositionScript(),
'bash hands node a DIFFERENT script than the tests exercise'
);
} finally {
cleanup(dir);
}
});
// Run ONLY the status guard at the head of the disposition block — everything up to the shim
// preamble. runShippedDisposition drives the node script directly and never sees this shell at
// all, so a test written against it says nothing about the guard: it passed unchanged with the
// guard made unconditional, which is exactly the vacuity this helper exists to remove.
function runDispositionGuard({ reviewText, withLedger, withFix, withIterFix }) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-grd-'));
try {
fs.writeFileSync(path.join(dir, '01-REVIEW.md'), reviewText);
if (withLedger) fs.writeFileSync(path.join(dir, '01-REVIEW-DISPOSITION.md'), '| CR-01 | critical | deferred | x |\n');
if (withFix) fs.writeFileSync(path.join(dir, '01-REVIEW-FIX.md'), '## Fixed Issues\n\n### CR-01: a thing\n');
if (withIterFix) fs.writeFileSync(path.join(dir, '01-REVIEW-FIX.iter2.md'), '## Fixed Issues\n\n### CR-01: a thing\n');
const fence = bashFences(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'))[1];
const cut = fence.indexOf('_MSD_SHIM_NAME=');
assert.ok(cut > 0, 'the disposition block must still open with its guard, then the shim');
const script = 'set -euo pipefail\n' + fence.slice(0, cut) + '\nprintf "PROCEEDED\\n"\n';
const res = runHook('-c', [script], {
interpreter: 'bash',
timeoutMs: PROBE_TIMEOUT_MS,
env: { ...process.env, PHASE_DIR: dir, PHASE_NUMBER: '1' },
});
assert.strictEqual(res.outcome, OUTCOME.EXITED, 'the guard must run to completion');
return { proceeded: /PROCEEDED/.test(res.stdout), stdout: res.stdout, exitCode: res.exitCode };
} finally {
cleanup(dir);
}
}
test('a clean review with no ledger is skipped', { skip: !HAS_BASH }, () => {
const out = runDispositionGuard({ reviewText: ['---', 'status: clean', '---'].join('\n') });
assert.strictEqual(out.proceeded, false, 'nothing to record and nothing to reconcile');
assert.match(out.stdout, /skipped \(status: clean\)/);
});
test('a clean review with an EXISTING ledger still proceeds, to reconcile it', { skip: !HAS_BASH }, () => {
// Freezing the ledger here would leave findings showing as open that the review no longer
// reports — the case the embedded script's own reconciliation path is written for. A guard
// that skipped unconditionally would make that path unreachable on exactly the run needing it.
const out = runDispositionGuard({ reviewText: ['---', 'status: clean', '---'].join('\n'), withLedger: true });
assert.strictEqual(out.proceeded, true, 'an existing ledger must still be reconciled');
assert.match(out.stdout, /reconciling the fix report and any existing disposition ledger/);
});
test('a review reporting issues always proceeds', { skip: !HAS_BASH }, () => {
const out = runDispositionGuard({ reviewText: ['---', 'status: issues_found', '---'].join('\n') });
assert.strictEqual(out.proceeded, true);
});
// #3861 round 5 — a converged `--auto` run reaches the guard with a CLEAN review and, on a direct
// /msd-code-review invocation, no gate-written ledger. Keying the skip on the ledger alone meant a
// fully successful multi-iteration run — every finding fixed and committed — recorded nothing at all.
test('a clean review with NO ledger but a fix report still proceeds', { skip: !HAS_BASH }, () => {
const out = runDispositionGuard({ reviewText: ['---', 'status: clean', '---'].join('\n'), withFix: true });
assert.strictEqual(out.proceeded, true, 'a fix report is a decision to record, ledger or not');
assert.match(out.stdout, /reconciling the fix report/);
});
test('a clean review with NO ledger but only an ITERATION fix report still proceeds', { skip: !HAS_BASH }, () => {
// The converged loop's earlier iterations survive only as <NN>-REVIEW-FIX.iterN.md, so the
// backups have to count toward the guard exactly as the final report does.
const out = runDispositionGuard({ reviewText: ['---', 'status: clean', '---'].join('\n'), withIterFix: true });
assert.strictEqual(out.proceeded, true, 'an iteration backup is a decision to record too');
});
test('a clean review with neither a ledger nor any fix report is still skipped', { skip: !HAS_BASH }, () => {
// The widening must not become "always proceed" — the original skip is still correct when
// there is genuinely nothing to record.
const out = runDispositionGuard({ reviewText: ['---', 'status: clean', '---'].join('\n') });
assert.strictEqual(out.proceeded, false);
assert.match(out.stdout, /skipped \(status: clean\)/);
});
// ── #3861 round 5 — the --auto multi-iteration reconciliation gap ──────────────────────────
//
// code-review-fix.md overwrites REVIEW-FIX.md on every iteration and DELETES the .iterN.md
// backups on convergence, so a finding fixed in iteration 1 was absent from the final fix report
// AND from the final review (it was fixed, so the re-review stopped reporting it). The step then
// fell back to the gate's `open` row and rendered `open ... (not in the current review)` — the
// same bytes a finding that vanished for an unrelated reason produces.
test('a fix report naming a finding the review no longer reports records it FIXED, not open', () => {
// The precise site: sameTitle(undefined, h.title) is false and title.has(id) is false too, so
// the entry entered NEITHER applied NOR staleFix and was dropped in silence.
const review = ['---', 'status: issues_found', '---', '', '### WR-09: something else'].join('\n');
const fix = ['## Fixed Issues', '', '### CR-01: the one fixed earlier'].join('\n');
const out = runShippedDisposition({ reviewText: review, fixText: fix });
const rows = ledgerRows(out.ledger);
const cr = rows.find((r) => r.id === 'CR-01');
assert.ok(cr, 'the fixed finding must have a row at all');
assert.strictEqual(cr.disposition, 'fixed', 'a committed fix must not render as open');
assert.match(cr.source, /not in the current review/, 'and it must be marked as no longer reported');
});
test('an ITERATION fix report is reconciled even when the final report has moved on', () => {
// The reviewer's scenario end to end: iteration 1 fixed CR-01, iteration 3 fixed WR-09, and the
// final REVIEW-FIX.md carries only the last iteration's scope.
const review = ['---', 'status: issues_found', '---', '', '### IN-07: still open'].join('\n');
const fix = ['## Fixed Issues', '', '### WR-09: fixed last'].join('\n');
const iter = { 2: ['## Fixed Issues', '', '### CR-01: fixed in iteration one'].join('\n') };
const out = runShippedDisposition({ reviewText: review, fixText: fix, iterFixText: iter });
const rows = ledgerRows(out.ledger);
assert.strictEqual(rows.find((r) => r.id === 'CR-01').disposition, 'fixed');
assert.strictEqual(rows.find((r) => r.id === 'WR-09').disposition, 'fixed');
assert.strictEqual(rows.find((r) => r.id === 'IN-07').disposition, 'open');
});
test('the NEWEST fix report wins when two iterations decide the same id differently', () => {
// Reports are read newest-first, so first-occurrence-wins gives the most recent statement —
// the same precedence a duplicate id already gets WITHIN one report.
const review = ['---', 'status: issues_found', '---', '', '### IN-07: unrelated'].join('\n');
const fix = ['## Skipped Issues', '', '### CR-01: contested'].join('\n');
const iter = { 2: ['## Fixed Issues', '', '### CR-01: contested'].join('\n') };
const out = runShippedDisposition({ reviewText: review, fixText: fix, iterFixText: iter });
assert.strictEqual(ledgerRows(out.ledger).find((r) => r.id === 'CR-01').disposition, 'skipped');
});
test('a REUSED id whose title differs is NOT inherited as fixed — the stale-report arm still rules', () => {
// The negative control for the arm added above. Re-review renumbers, so an earlier iteration's
// CR-01 and the current review's CR-01 can be different findings; carrying the decision across
// that boundary would render a false `fixed`, which is worse than the `open` it replaced.
const review = ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n');
const iter = { 2: ['## Fixed Issues', '', '### CR-01: an older, different finding'].join('\n') };
const out = runShippedDisposition({ reviewText: review, iterFixText: iter });
const cr = ledgerRows(out.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'open', 'a different finding under a reused id must stay open');
assert.match(out.stdout, /title their finding differently|titles its finding differently/,
'and the mismatch must be stated, not swallowed');
});
// ── #3861 round 5 rework — a REUSED finding id must not inherit the old finding's decision ──
//
// Found by the round's own adversarial review, which drove it: ids are reused across re-reviews
// (the --auto loop renumbers), and row() inherited a prior decision on an id match alone. A prior
// 'CR-01 fixed' against a review reporting a brand-new CR-01 rendered the NEW finding `fixed` — a
// false decision in the artifact whose entire purpose is telling triaged from forgotten.
test('the ledger does not promise a preservation it no longer makes', () => {
// The rendered text said "preserves every row and every disposition" while the step had gained
// an intentional drop for a reused id — shipped, user-facing text asserting something false.
// And the console must not point at git: committing is gated on commit_docs and a failed commit
// is swallowed, so under commit_docs=false the overwritten decision may exist nowhere.
// Round 5 found this test VACUOUS: it ran with no prior ledger, so no reuse ever occurred and
// the `is in git` assertion could not have failed however the console was worded. Driven through
// a real drop now, so the negative assertion is made against a console line that actually exists.
const seed = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: the original finding'].join('\n'),
priorText: '| CR-01 | critical | deferred | waiting on the vendor |\n',
});
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: seed.ledger,
});
assert.match(out.stdout, /decision\(s\) DROPPED/, 'precondition: this run must actually drop a decision');
assert.doesNotMatch(out.ledger, /preserves every row and every disposition/,
'the unqualified preservation promise must not return');
assert.match(out.ledger, /id is REUSED/, 'and the one exception must be stated where a reader meets it');
assert.doesNotMatch(out.stdout, /is in git/, 'the console must not assert a recovery path that may not exist');
});
test('an `open` prior is replaced SILENTLY, and the shipped text says so', () => {
// Round 5: the legend and both feature docs claimed the drop is named on the console
// unconditionally. It is not — `row()` reports only a RECORDED decision (`was.d !== 'open'`).
// The behaviour is deliberate (an `open` row records no decision to lose); the text was wrong.
const seed = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: the original finding'].join('\n'),
});
assert.strictEqual(ledgerRows(seed.ledger).find((r) => r.id === 'CR-01').disposition, 'open');
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: seed.ledger,
});
assert.doesNotMatch(out.stdout, /DROPPED/, 'an untriaged prior row is replaced without a report');
assert.match(out.ledger, /a row still at `open` is replaced silently/,
'and the legend must state that exception rather than promising an unconditional report');
});
test('a dropped decision is REPORTED on the console, not lost quietly', () => {
// The `superseded:` block that preserved these was tried and withdrawn — it produced a fresh
// defect on each of three review passes. What survives is the guard (no false `fixed`) plus an
// explicit report; the prior ledger row remains in git.
const seed = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: the original finding'].join('\n'),
priorText: '| CR-01 | critical | deferred | waiting on the vendor |\n',
});
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: seed.ledger,
});
assert.strictEqual(ledgerRows(out.ledger).find((r) => r.id === 'CR-01').disposition, 'open');
assert.match(out.stdout, /decision\(s\) DROPPED/, 'the drop must be stated');
assert.match(out.stdout, /CR-01=deferred/, 'naming the id and what was decided');
assert.doesNotMatch(out.ledger, /^superseded:/m, 'and no preservation block is written');
});
test('a prior decision is NOT inherited when the id now names a different finding', () => {
const first = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: the original finding'].join('\n'),
fixText: ['## Fixed Issues', '', '### CR-01: the original finding'].join('\n'),
});
assert.strictEqual(ledgerRows(first.ledger).find((r) => r.id === 'CR-01').disposition, 'fixed');
assert.match(first.ledger, /title: "the original finding"/, 'the title must be recorded to make reuse detectable');
const second = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: first.ledger,
});
const cr = ledgerRows(second.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'open', 'a different finding under a reused id must be untriaged');
});
test('an EMPTY title is recorded, so it cannot read back as a pre-format ledger', () => {
// The leak that came back three passes running: while an empty title emitted no `title:` key,
// an empty-titled finding read back as legacy and inherited a decision across a reused id.
const first = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### IN-07: unrelated'].join('\n'),
iterFixText: { 2: ['## Fixed Issues', '', '### CR-01:'].join('\n') },
});
assert.match(first.ledger, /title: ""/, 'an empty title is still recorded, explicitly');
const second = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: first.ledger,
});
assert.strictEqual(ledgerRows(second.ledger).find((r) => r.id === 'CR-01').disposition, 'open',
'an empty recorded title is a title, not an absent one');
});
test('a pre-format ledger whose title merely LOOKS like JSON keeps its quotes', () => {
// Without the `titles: json` marker, JSON.parse ran on every value — so a legacy bare title
// written as "quoted whole title" lost its quotes, stopped matching, and flipped to open.
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: "quoted whole title"'].join('\n'),
priorText: [
'---', 'phase: 01', 'review: 01-REVIEW.md', 'findings:',
' - id: CR-01', ' severity: critical', ' disposition: deferred',
' title: "quoted whole title"', 'open: 0', 'total: 1', 'recorded: x', '---', '',
'| CR-01 | critical | deferred | a reason |',
].join('\n'),
});
const cr = ledgerRows(out.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred', 'the legacy title must still identify the finding');
assert.doesNotMatch(out.stdout, /DROPPED/, 'and no drop may be reported');
});
test('a prior ledger with NO recorded title still inherits its decision (back-compat)', () => {
// A ledger written before titles were recorded carries none. Refusing to inherit there would
// reset every decision in it — the loss this guard exists to prevent, caused by the guard.
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: whatever it is called now'].join('\n'),
priorText: '| CR-01 | critical | deferred | a reason from an older ledger |\n',
});
const cr = ledgerRows(out.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'deferred', 'an absent prior title must inherit, not reset');
assert.strictEqual(cr.source, 'a reason from an older ledger');
});
test('an iteration-derived decision cites the report it actually came from', () => {
// The Source cell hard-coded the unsuffixed <NN>-REVIEW-FIX.md, so a decision read out of an
// iteration backup cited a file that may not exist. A citation the reader cannot follow is
// worse than none.
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### IN-07: unrelated'].join('\n'),
iterFixText: { 2: ['## Fixed Issues', '', '### CR-01: fixed in iteration one'].join('\n') },
});
const cr = ledgerRows(out.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(cr.disposition, 'fixed');
assert.match(cr.source, /01-REVIEW-FIX\.iter2\.md/, 'the cited report must be the one that decided it');
});
// ── #3861 round 5, second rework pass — four defects the review drove out of the FIRST fix ──
test('an iteration-only decision records the title it was decided under', () => {
// Without this the row was written with NO title -- the current review does not report the
// finding, so nothing else knows one -- and the next review reusing that id then hit the
// title-ABSENT back-compat exception and inherited the old `fixed`. The very defect the title
// machinery exists to close, surviving through the hole opened for legacy ledgers.
const first = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### IN-07: unrelated'].join('\n'),
iterFixText: { 2: ['## Fixed Issues', '', '### CR-01: the original finding'].join('\n') },
});
assert.match(first.ledger, /title: "the original finding"/, 'the deciding title must be recorded');
const second = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: a brand new finding'].join('\n'),
priorText: first.ledger,
});
assert.strictEqual(ledgerRows(second.ledger).find((r) => r.id === 'CR-01').disposition, 'open',
'a reused id must not inherit a decision recorded for a different finding');
});
test('the ledger frontmatter is valid YAML even when a title contains a colon', () => {
// `title: Parser: loses data` is not YAML — a real reader returns 'bad indentation of a mapping
// entry'. The values are emitted as JSON scalars, which YAML 1.2 reads as double-quoted strings.
const yaml = require('js-yaml');
const out = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: Parser: loses data'].join('\n'),
});
const fm = yaml.load(out.ledger.split('---')[1]);
assert.strictEqual(fm.findings[0].title, 'Parser: loses data', 'the colon must survive the round trip');
// And the title still round-trips through the parser as an identity, so a re-run is unchanged.
const again = runShippedDisposition({
reviewText: ['---', 'status: issues_found', '---', '', '### CR-01: Parser: loses data'].join('\n'),
priorText: out.ledger,
});
assert.strictEqual(again.wroteNothing, true, 'a second run must still report unchanged');
});
test('a carried human reason ending in the marker is preserved, and not doubled', () => {
// Neither stripped (the previous fix's residue) nor appended twice.
const review = ['---', 'status: issues_found', '---', '', '### WR-01: unrelated'].join('\n');
const prior = '| CR-01 | critical | deferred | defer because (not in the current review) |';
const first = runShippedDisposition({ reviewText: review, priorText: prior });
const carried = ledgerRows(first.ledger).find((r) => r.id === 'CR-01');
assert.strictEqual(carried.source, 'defer because (not in the current review)');
const second = runShippedDisposition({ reviewText: review, priorText: first.ledger });
assert.strictEqual(
ledgerRows(second.ledger).find((r) => r.id === 'CR-01').source,
'defer because (not in the current review)',
'stable across runs — the render appends nothing it can already see'
);
});
test('a leading-zero count is evaluated, not silently skipped', { skip: !HAS_BASH }, () => {
// Bash infers the base from a leading zero, so `critical: 08` made $(( )) fail. It does NOT
// abort — the expansion sits in an `if` condition, where set -e does not fire — which is
// worse: the sum check silently does not run, an inconsistent breakdown passes, and the
// only trace is a stray diagnostic on stderr. Asserting exit 0 alone is vacuous here; it
// was true before the fix too. Assert the check's OUTCOME and the absent diagnostic.
const padded = (total) => ['---', 'findings:', ' critical: 08', ' warning: 0',
' info: 0', ' total: ' + total, 'status: issues_found', '---'].join('\n');
const inconsistent = runShippedGateCounts({ reviewText: padded('9') });
assert.strictEqual(readGateMessage(inconsistent.stdout).countsOk, '0',
'08 + 0 + 0 is 8, not 9 \u2014 the check must FIRE');
assert.doesNotMatch(inconsistent.stderr, /value too great for base/,
'the padded count must be read as decimal, not left to base inference');
const consistent = runShippedGateCounts({ reviewText: padded('8') });
assert.strictEqual(consistent.exitCode, 0, 'an advisory gate must not abort on a padded count');
assert.strictEqual(readGateMessage(consistent.stdout).countsOk, '1',
'08 + 0 + 0 is 8, so the breakdown is consistent');
});
test('a fence indented past three spaces is not a fence', { skip: !HAS_BASH }, () => {
// CommonMark: at most three leading spaces open a fence; four is an indented code block.
const src = fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8');
assert.match(src, /\^ \{0,3\}\(/, 'the fence matcher must bound its leading whitespace');
});
});
// Placement here is now incidental. This block used to be pinned to the end of the file because
// HAS_BASH was declared mid-file and a `skip` option referencing it from an earlier block hit the
// temporal dead zone -- cancelling its neighbours rather than failing visibly. HAS_BASH is
// declared with the file's other top-level constants now, so that constraint is gone and this
// block may be moved beside its siblings whenever someone is tidying.
describe('#3861 round 1 — count validation, executed', () => {
test('all four counts are validated before the breakdown is shown', { skip: !HAS_BASH }, () => {
// Was a `src.includes()` assertion on the exact `case` line, which broke the moment that
// line grew a length bound. The behaviour is what matters and is now executed directly.
const counts = (c, w, i, t) => ['---', 'findings:', ' critical: ' + c, ' warning: ' + w,
' info: ' + i, ' total: ' + t, 'status: issues_found', '---'].join('\n');
const ok = (t) => readGateMessage(runShippedGateCounts({ reviewText: t }).stdout).countsOk;
assert.strictEqual(ok(counts('x', '0', '0', '0')), '0',
'a non-numeric count withholds the whole breakdown');
assert.strictEqual(ok(counts('1', '2', '1', '4')), '1');
// Bash integers wrap at 2^64, so a 20-digit count reaches the sum as 0 and an inconsistent
// breakdown passes. Length-bounded, because no review reports nine digits of findings.
assert.strictEqual(ok(counts('18446744073709551616', '0', '0', '0')), '0',
'a count long enough to wrap the sum is not a count');
});
test('the count-length threshold is covered at limit-1, limit and limit+1', { skip: !HAS_BASH }, () => {
// M2. The guard is `?????????*` -- nine or more characters -- so the limit is 8 digits
// ACCEPTED, 9 REJECTED. The only cases here were 'x', single digits and a 20-digit value,
// none of which pins the boundary: dropping one `?` moves the limit to 7 digits and no test
// would have noticed. All three points are asserted, and the sum is kept consistent at each
// so the length rule is what decides the verdict rather than the sum check.
const counts = (c, w, i, t) => ['---', 'findings:', ' critical: ' + c, ' warning: ' + w,
' info: ' + i, ' total: ' + t, 'status: issues_found', '---'].join('\n');
const ok = (t) => readGateMessage(runShippedGateCounts({ reviewText: t }).stdout).countsOk;
const d = (n) => '1'.padEnd(n, '0'); // n digits, leading 1 so the value is exact
assert.strictEqual(d(7).length, 7);
assert.strictEqual(d(8).length, 8);
assert.strictEqual(d(9).length, 9);
assert.strictEqual(ok(counts(d(7), '0', '0', d(7))), '1', 'limit-1: 7 digits is accepted');
assert.strictEqual(ok(counts(d(8), '0', '0', d(8))), '1', 'limit: 8 digits is accepted');
assert.strictEqual(ok(counts(d(9), '0', '0', d(9))), '0', 'limit+1: 9 digits is rejected');
});
});
describe('#3861 round 1 — an absent review still reconciles', () => {
test('a missing REVIEW.md does not abandon an existing ledger', () => {
// The status guard proceeds when a ledger exists, so the script must tolerate the review
// being gone: reading it unconditionally threw, the trailing fallback swallowed it, and the
// ledger was left frozen — the exact freeze the reconciliation path exists to prevent.
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'msd-3861-abs-'));
try {
const dispPath = path.join(dir, '01-REVIEW-DISPOSITION.md');
fs.writeFileSync(dispPath, ['| CR-01 | critical | deferred | waiting on ADR-9 |',
'| WR-09 | warning | open | - |'].join('\n'));
const res = runNode(['-e', shippedDispositionScript()], {
timeoutMs: PROBE_TIMEOUT_MS,
env: {
...process.env,
REVIEW_FILE: path.join(dir, '01-REVIEW.md'), // deliberately absent
DISPOSITION_FILE: dispPath,
FIX_REPORT_FILE: path.join(dir, '01-REVIEW-FIX.md'),
PADDED: '01',
},
});
assert.strictEqual(res.outcome, OUTCOME.EXITED);
assert.strictEqual(res.exitCode, 0, 'an absent review is not an error: ' + res.stderr);
const rows = ledgerRows(fs.readFileSync(dispPath, 'utf8'));
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01', 'WR-09'],
'an absent review reconciles the ledger; it does not delete rows from it');
assert.match(rows[0].source, /not in the current review/);
} finally {
cleanup(dir);
}
});
});
describe('#3861 round 1 — fence closers and one-letter prefixes', () => {
test('a line with an info string is an opener shape, never a closer', () => {
// CommonMark: a closing fence carries only whitespace after its marker. Treating an
// info-string line as a close ends the fence early and admits the example headings under it.
const review = ['---', 'status: issues_found', '---', '', '```',
'```js', '### CR-77: still inside the fence', '```', '', '### CR-01: real'].join('\n');
const rows = ledgerRows(runShippedDisposition({ reviewText: review }).ledger);
assert.deepStrictEqual(rows.map((r) => r.id), ['CR-01']);
});
test('a one-letter finding prefix in the agent template is not invisible', () => {
// The domain scan required [A-Z]{2,}, so a template heading like `### C-01:` — explicit and
// parseable, not prose — contributed nothing and the guard passed over a finding shape the
// ledger would drop entirely.
const agent = fs.readFileSync(REVIEWER_AGENT_PATH, 'utf8');
const emitted = [...agent.matchAll(/^###\s+([A-Z]+)-\d+:/gm)].map((m) => m[1]);
assert.ok(emitted.length > 0, 'the reviewer agent must still declare its finding-id shapes');
const oneLetter = [...'### C-01: x'.matchAll(/^###\s+([A-Z]+)-\d+:/gm)].map((m) => m[1]);
assert.deepStrictEqual(oneLetter, ['C'], 'the scan must admit a single-letter prefix');
});
});
// ---------------------------------------------------------------------------
// The REVIEW.md lookup's phase-id handling, tested at the step file that owns it.
//
// These four tests used to live in `tests/nsegment-phase-grammar.test.cjs`, inside the
// `#4748` describe block, because that is where the lookup's gate was when #3829 moved the
// lookup out of `execute-phase.md`. #4781 (#4628) then removed #4748's letter-axis work from
// the grammar file, so the block that hosted them no longer exists. They are re-homed here
// unchanged in substance: they assert properties of THIS PR's step file, not of #4748's sites,
// and keeping them beside the step they guard is what stops an unrelated upstream revert from
// silently deleting this PR's own coverage.
//
// One test did NOT come along: the assertion that `execute-phase.md`'s init parse list names
// `padded_phase`. That is a property of #4748's site, not of this step — #4781 removed the
// field from that list, and carrying the assertion here would only pin someone else's revert.
//
// The MECHANISM differs from the pre-move one and the PROPERTY does not. `execute-phase.md`
// bound init's `{padded_phase}`; the step validates PHASE_NUMBER for shape and traversal and
// then pads the DIGIT RUN as a STRING, carrying an optional letter and any dot segments verbatim.
// Both refuse exactly the two shapes #4748 named: `printf "%02d"` cannot pad `03A` (prints `03`,
// exits 1) and reads an already-padded `08` as octal (prints `00`) -- and the string pad refuses
// them by not doing arithmetic at all, which also fixed a THIRD shape the arithmetic form got
// wrong (`008` -> `08`). The last two tests drive that equivalence against the canonical
// normalizer rather than asserting it.
// ---------------------------------------------------------------------------
describe('#3829 — the step\'s REVIEW.md lookup resolves a letter-suffixed phase without a shell re-pad', () => {
const { execFileSync } = require('node:child_process');
const { splitLines } = require('../msd-core/bin/lib/text-lines.cjs');
const { normalizePhaseName } = require('../msd-core/bin/lib/phase-id.cjs');
/**
* Pure: the indexes of every line containing `anchor`. Asserts the count so a site that is
* added, removed or renamed breaks this test loudly instead of silently narrowing what it
* covers (the step carries each anchor TWICE — one per markdown fence, each a fresh shell).
*/
function findAnchoredLineIndexes(lines, anchor, expectedCount) {
const idx = [];
lines.forEach((l, i) => { if (l.includes(anchor)) idx.push(i); });
assert.equal(
idx.length,
expectedCount,
`expected ${expectedCount} line(s) containing ${JSON.stringify(anchor)}, found ${idx.length}`,
);
return idx;
}
/** Run `script` in bash with `env` merged in; never throws — returns { status, stdout, stderr }. */
function runBash(script, env) {
try {
const stdout = execFileSync('bash', [], {
input: script,
encoding: 'utf8',
timeout: PROBE_TIMEOUT_MS,
env: { ...process.env, ...env },
stdio: ['pipe', 'pipe', 'pipe'],
});
return { status: 0, stdout: stdout.trim(), stderr: '' };
} catch (e) {
return { status: e.status, stdout: String(e.stdout || '').trim(), stderr: String(e.stderr || '').trim() };
}
}
const stepLines = splitLines(fs.readFileSync(DISPOSITION_STEP_PATH, 'utf8'));
// TWO fences — each markdown fence is a fresh shell, so each derives and looks up for itself.
const lookups = findAnchoredLineIndexes(stepLines, 'REVIEW_FILE="${_pd}/${PADDED}-REVIEW.md"', 2);
// The derivation slice each fence runs before its lookup, taken by CONTENT rather than by a
// line number that every edit to the file would drift.
const derivStarts = findAnchoredLineIndexes(stepLines, '_pd="${PHASE_DIR:-}"', 2);
const derivations = derivStarts.map((d, n) => stepLines.slice(d, lookups[n] + 1).join('\n'));
test('no fence re-pads the phase number with printf (fails before the fix)', () => {
// The defect shape, not the remedy: `printf "%0Nd"` applied to PHASE_NUMBER itself.
const offenders = stepLines
.map((l, n) => [n + 1, l])
.filter(([, l]) => !/^\s*#/.test(l) && /printf\s+"%0\d*d"\s+"?\$\{?PHASE_NUMBER/.test(l));
assert.deepEqual(offenders, [], `the step must not re-pad PHASE_NUMBER in shell: ${JSON.stringify(offenders)}`);
// And the pad it does perform is a STRING pad, not arithmetic: the canonical normalizer
// left-pads to a MINIMUM of two and otherwise preserves the run, so `$(( ))` is wrong by
// construction -- it collapsed `008` to `08` until round 14.
// Asserted as the PROPERTY, not as one spelling of it: an equivalent multi-line string pad must
// pass. What must not pass is arithmetic, or a pad that drops either carried part.
for (const d of derivations) {
const bound = d.split('\n').find((l) => /PADDED=/.test(l) && !/PADDED=""/.test(l));
assert.ok(bound, 'the derivation must bind PADDED');
assert.doesNotMatch(bound, /printf|\$\(\(/, `the pad must not be arithmetic: ${bound.trim()}`);
// What the pad must PRODUCE is asserted by the matrix and the property below, which execute
// it. Requiring a particular variable to appear HERE false-positives on an equivalent pad
// that routes the digit run through an intermediate -- driven, and it is why this stops at
// the defect shape.
}
});
test('regression control: both lookup lines are unchanged', () => {
for (const i of lookups) {
assert.equal(stepLines[i].trim(), 'REVIEW_FILE="${_pd}/${PADDED}-REVIEW.md"');
}
});
test('nothing REBINDS REVIEW_FILE after the lookup', () => {
// The slices above stop AT the first anchored assignment, so on their own they cannot see a
// later line overwriting the path the fence actually consumes. Driven by an adversarial pass
// on this very test: inserting the expected lookup and then overriding it with
// REVIEW_FILE="${_pd}/WRONG-REVIEW.md" left every other assertion here green. Pin the whole
// file rather than the slice -- the only REVIEW_FILE= bindings permitted are the canonical
// lookup (once per fence) and the identity pass-through that hands it to the embedded node
// script as an env prefix.
// The predicate is deliberately wider than `^REVIEW_FILE=`: a second adversarial pass drove an
// INDENTED assignment and an `export REVIEW_FILE=...` straight through that anchor, both of
// which execute exactly like a bare one. Leading whitespace and an optional `export` are
// absorbed here so the accept-list below is what decides, not the spelling of the line.
// `+=` too: `REVIEW_FILE+=-wrong` APPENDS and executes (driven: `REVIEW_FILE=good;
// REVIEW_FILE+=-wrong` prints `good-wrong`), so an assignment-operator match that only sees
// `=` lets a real rebinding through. Third spelling found by a third adversarial pass.
const BIND_RE = /^\s*(?:export\s+)?REVIEW_FILE\+?=/;
const binds = stepLines.filter((l) => BIND_RE.test(l)).map((l) => l.replace(/^\s*(?:export\s+)?/, ''));
assert.equal(binds.filter((l) => l.startsWith('REVIEW_FILE="${_pd}/${PADDED}-REVIEW.md"')).length, 2,
'each fence must bind the canonical lookup exactly once');
for (const b of binds) {
const ok = b.startsWith('REVIEW_FILE="${_pd}/${PADDED}-REVIEW.md"')
|| b.startsWith('REVIEW_FILE="${REVIEW_FILE}"');
assert.ok(ok, `REVIEW_FILE is rebound to something other than the canonical lookup: ${b}`);
}
});
test('composition: the live derivation and lookup resolve the letter phase\'s own REVIEW.md', (t) => {
// The executable half: run the SHIPPED lines against a fixture so the validation, the padding,
// the letter carry and the path construction are exercised together.
const dir = createTempDir();
t.after(() => cleanup(dir));
for (const [id, status] of [['3A', 'clean'], ['8', 'issues'], ['9', 'skipped']]) {
const emitted = normalizePhaseName(id);
fs.writeFileSync(path.join(dir, `${emitted}-REVIEW.md`), `---\nstatus: ${status}\n---\n# review\n`);
for (const deriv of derivations) {
const script = [
'set -e',
deriv,
'test -f "$REVIEW_FILE" || { echo "MISSING $REVIEW_FILE"; exit 3; }',
'printf \'%s %s\' "$PADDED" "$(grep -m1 "^status:" "$REVIEW_FILE" | cut -d: -f2 | tr -d " ")"',
].join('\n');
const r = runBash(script, { PHASE_DIR: dir, PHASE_NUMBER: id });
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stdout} ${r.stderr}`);
assert.equal(r.stdout, `${emitted} ${status}`);
}
}
});
test('the step\'s padding agrees with the canonical normalizer across the letter matrix', (t) => {
// Driven, not argued: every shape #4748's own tests named, plus its stated regression controls.
const dir = createTempDir();
t.after(() => cleanup(dir));
for (const id of ['3A', '8', '9', '08', '09', '12A', '4B', '23.1.2', '03A.1.2', '1', '06', '36.14', '08.5']) {
for (const deriv of derivations) {
const r = runBash(`set -e\n${deriv}\nprintf '%s' "$PADDED"`, { PHASE_DIR: dir, PHASE_NUMBER: id });
assert.equal(r.status, 0, `bash exited ${r.status} on ${id}: ${r.stderr}`);
assert.equal(r.stdout, normalizePhaseName(id), `padding drifted from the canonical normalizer on ${id}`);
}
}
});
test('property: the step\'s padding agrees with the canonical normalizer on any id it accepts', (t) => {
// The matrix above samples 13 shapes. #4748's guarantee at its ORIGINAL site was a DATAFLOW
// pin — `execute-phase.md` bound init's own `{padded_phase}`, so the lookup could not disagree
// with the canonical normalizer because it never computed anything. This step reconstructs the
// value in shell instead, so that pin is not available here and AGREEMENT is what replaces it.
// A 13-point sample cannot see a future canonical-grammar change land outside those 13 points;
// a generator can, and this is the exact blind spot rounds 3 and 5 of this PR were both about.
// Scoped to the ids the step's own guard ACCEPTS: a digit run within its 8-digit bound, an
// optional single A-Z, and optional dot segments. Milestone `N-N` forms are outside the step's
// domain, and are asserted nowhere here rather than silently passed.
const dir = createTempDir();
t.after(() => cleanup(dir));
// The digit run is generated as a STRING of digits, never as an integer: `String(fc.integer())`
// can never produce a LEADING ZERO, so an integer-sourced generator silently loses the `08`/`09`
// cases the finite matrix above already covered, and could never have reached `008`. That was
// this property's own first cut, and the review that caught it is why the shape is spelled out.
// Segment depth goes to four because the repo itself exercises `1.2.3.4`. The step's grammar is
// unbounded in depth; four is a bound, stated rather than implied, and it is the residual here.
const digitRun = fc.array(fc.integer({ min: 0, max: 9 }), { minLength: 1, maxLength: 8 })
.map((ds) => ds.join(''));
const PHASE_ID = fc.tuple(
digitRun,
fc.option(fc.constantFrom(...'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), { nil: '' }),
fc.array(digitRun, { maxLength: 4 }),
).map(([run, letter, segs]) => run + letter + segs.map((s) => '.' + s).join(''));
fc.assert(fc.property(PHASE_ID, (id) => {
for (const deriv of derivations) {
const r = runBash(`set -e\n${deriv}\nprintf '%s' "$PADDED"`, { PHASE_DIR: dir, PHASE_NUMBER: id });
assert.equal(r.status, 0, `bash exited ${r.status} on ${id}: ${r.stderr}`);
assert.equal(r.stdout, normalizePhaseName(id), `padding drifted from the canonical normalizer on ${id}`);
}
}), { numRuns: 25 });
});
});