Files
msd-core/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs
Tom Boucher ef43f5161f fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches (#2972)
* fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches

The prior Step 5 "Hunk Verification Gate" was prescribed correctly in the
workflow text — but executed laxly by the LLM, which filled in `verified: yes`
without actually checking content presence. The reporter observed three
distinct files (skills/gsd-discuss-phase/SKILL.md, skills/gsd-autonomous/
SKILL.md, get-shit-done/workflows/new-project.md) where archives contained
substantive user-added blocks that did not survive into the merged result, yet
the gate reported clean.

Move verification from LLM-driven prose into a deterministic Node script the
workflow calls. The script can't be shortcut.

Changes:

- scripts/verify-reapply-patches.cjs (new): pure Node, no external deps.
  For each file in the patches dir, computes user-added significant lines as
  the line-set diff between backup and pristine baseline (when available;
  falls back to "every significant backup line" when no pristine — over-broad
  but the safe direction for this bug class). Asserts each line appears
  literally in the merged installed file via String.prototype.includes.
  Filters trivial lines (length < 12 chars, pure punctuation, decorative
  comments) so harmless drift doesn't trigger false failures. Exits 0 on
  pass, 1 on any miss with per-file diagnostic, 2 on usage error.
  Supports --json for workflow consumption.

- get-shit-done/workflows/reapply-patches.md: rewrite Step 5 to call the
  script and parse its JSON output. The Step 4 Hunk Verification Table
  remains as advisory Claude-readable summary, but the gate is now the
  script's exit code.

- tests/bug-2969-verify-reapply-patches.test.cjs (new): 6 tests covering
  (a) pass when every line survives, (b) fail when a line is missing,
  (c) fail when the merged file is deleted entirely, (d) --json structured
  report shape, (e) backup-meta.json is correctly skipped as metadata,
  (f) no-pristine-dir fallback exercises the safe over-broad path. All pass.

Out of scope: the manifest-baseline tightening described in #2969 Failure 1
(saveLocalPatches comparing against the wrong baseline so prior silent wipes
poison subsequent updates). That's a separate, bigger architectural change
involving pristine-content infrastructure; this PR addresses the gate fidelity
half so users at least see the diagnostic when content goes missing.

Closes #2969 (partial — Failure 2 only)

* fix(#2969): preserve #1999 Hunk Verification Table assertions alongside new script gate

CI failure on PR #2972 surfaced that tests/reapply-patches.test.cjs (the
#1999 contract) asserts Step 5 references:
  - "Hunk Verification Table"
  - `verified: no` failure condition
  - explicit STOP/halt/abort directive
  - "table absent / missing" halt path

My initial Step 5 rewrite for #2969 substituted the deterministic script
for the table-based gate entirely, stripping those references. The script
is the strictly stronger gate, but the existing #1999 test enforces the
table-based safety net as a defense-in-depth contract.

Restore both gates as a layered Step 5:

  - 5a (binding): deterministic verifier script — script gate, exits
    non-zero on any miss, cannot be shortcut by the LLM
  - 5b (advisory): Hunk Verification Table review — preserved as
    redundant safety net for the case where the script has a bug or the
    pristine baseline is unavailable

Both gates must pass. Verified: tests/reapply-patches.test.cjs (5 tests
in the #1999 suite) and tests/bug-2969-verify-reapply-patches.test.cjs
(6 tests in the #2969 suite) all pass — 21/21 total in this fixture.

* fix(#2969): address CodeRabbit findings on workflow + script

Five CR findings on PR #2972, all valid; addressed in this commit:

1. (Major) Stderr was merged into VERIFY_OUTPUT via `2>&1`, so any Node
   warning, deprecation notice, or stack trace would corrupt the JSON
   parse downstream. Capture stdout only; stderr remains on the
   controlling terminal for operator visibility.

2. (Major) verifyFile() crashed with EISDIR/EACCES instead of producing
   a structured diagnostic when the installed path was a directory or
   unreadable. Wrap statSync/readFileSync in try/catch and emit a
   per-file fail row; the whole-run gate continues with structured
   output. Added test case asserting the directory-at-installed-path
   case fails with `not a regular file` diagnostic instead of crashing.

3. (Minor) PRISTINE_FLAG built as a single string + unquoted expansion
   would split paths with spaces. Switched to a bash array (VERIFY_ARGS)
   that preserves whitespace through expansion.

4. (Minor) Fenced code block missing language tag (markdownlint MD040).
   Added `text` tag to the error message block.

5. (Minor) Usage comment said pristine fallback was "backup-meta lookup"
   but the actual code path falls back to significant-line checks from
   backup content. Corrected the comment to match implementation.

Verified all 21 tests in tests/reapply-patches.test.cjs (#1999 contract)
+ tests/bug-2969-verify-reapply-patches.test.cjs (now 7 tests with the
new directory case) pass.

* test(#2969): structured JSON assertions, no substring matching on script output

Replace every assert.match(r.stdout, /pattern/) call with structured
assertions on the parsed JSON report from the script's own --json mode.
The script's --json contract IS the structured shape we test against —
the test author should never depend on the human-readable formatter
output, just as no test should depend on substring presence in source.

Changes:

  - All 7 tests now run the verifier with --json (via a runVerifier()
    helper) and parse the resulting JSON document into { status, report,
    stderr }. Diagnostic stderr is preserved as a separate channel for
    debug output but is not used for assertions.
  - Each previously substring-matched diagnostic ("Failures: 1",
    "not a regular file", "installed file missing after merge",
    file path, dropped line) is now a deepEqual / equal / Array.includes
    against typed report fields: report.failures, report.results[i].status,
    report.results[i].reason, report.results[i].file,
    report.results[i].missing[].
  - Added an explicit "documented shape" test asserting the JSON output
    has exactly the keys { file, missing, reason, status } per result —
    locks the public contract of the --json mode.
  - DRY'd up fixture reset into a resetFixture() helper since every test
    starts with a fresh patches/installed/pristine triple.

Linter: scripts/lint-no-source-grep.cjs reports 0 violations across 348
test files. Combined run of bug-2969-...test.cjs (7 tests) +
reapply-patches.test.cjs (5 tests in the #1999 suite) all pass —
22/22 in the relevant fixture.

* fix(#2969): typed REASON enum + raw-text-matching rule shipped repo-wide

This commit closes the loop on the no-source-grep discipline:

1. scripts/verify-reapply-patches.cjs:
   - Frozen REASON enum exposes the diagnostic surface as stable codes:
     OK_NO_USER_LINES_VS_PRISTINE, OK_NO_SIGNIFICANT_BACKUP_LINES,
     FAIL_INSTALLED_MISSING, FAIL_INSTALLED_NOT_REGULAR_FILE,
     FAIL_READ_ERROR, FAIL_USER_LINES_MISSING.
   - Each result.reason is now a code from this enum, not free text.
     Tests assert via REASON.X equality, not regex on prose.
   - REASON exported from module.exports.

2. tests/bug-2969-verify-reapply-patches.test.cjs:
   - Full rewrite. Every assertion on typed structured fields:
     report.results[0].status === 'fail',
     report.results[0].reason === REASON.FAIL_INSTALLED_NOT_REGULAR_FILE,
     report.results[0].missing.includes(droppedLine) (Array set membership,
     not String substring).
   - Locks the REASON enum surface via Object.keys(REASON).sort() deepEqual.
   - Locks the JSON report shape via Object.keys(report).sort() deepEqual.
   - Zero regex, zero String#includes, zero startsWith/endsWith on text.

3. CONTRIBUTING.md:
   - New section "Prohibited: Raw Text Matching on Test Outputs" with
     concrete BAD/GOOD examples (substring on file content; assert.match
     on stdout; "structured parser" hiding string ops; regex on free-form
     reason fields).
   - The rule statement: "Tests assert on typed structured values. If
     the code under test produces text, the code under test must also
     expose a structured intermediate representation, and the test must
     assert on that IR — never on the rendered text."
   - Required structured-surface table: file IR, --json mode, frozen
     enum, fs facts.
   - "Hiding grep behind a function is still grep" callout — the
     parser-wrapper anti-pattern.
   - New `pre-existing-text-matching` exemption category for the 8
     grandfathered files. Marked Transitional; new tests cannot use it.

4. scripts/lint-no-source-grep.cjs:
   - Three new patterns enforced (in addition to the existing .cjs-source
     readFileSync rule):
     - assert.match/doesNotMatch on .stdout/.stderr
     - .stdout/.stderr.<includes|startsWith|endsWith>(
     - readFileSync(...).<includes|startsWith|endsWith>(
   - Aggregated violations per file (multiple findings now report together).
   - Updated diagnostic message references both CONTRIBUTING.md sections.

5. 8 pre-existing tests annotated with `// allow-test-rule:
   pre-existing-text-matching` so the lint passes on this commit; each
   carries the prose "Tracked for migration to typed-IR assertions; do
   not copy this pattern." Files: bug-2649, bug-2687, bug-2796, bug-2838,
   bug-2943, graphify, hooks-opt-in, security-scan.

Verification: lint 0 violations across 348 test files; full suite passes.

* fix(#2969): rename exemption category to pending-migration-to-typed-ir + cite tracking issue

Per maintainer feedback:
1. "Grandfathered" / "legacy" framing is wrong — both terms imply
   permanent or condoned exemption. The 8 files are tracked for
   correction, not exempted.
2. Each annotated file must cite the tracking issue so the migration
   work is auditable.

Changes:
- CONTRIBUTING.md: rename exemption category from
  `pre-existing-text-matching` to `pending-migration-to-typed-ir`. Update
  prose to "Tracked for correction, not exempted" and require each
  annotation to cite the open migration issue (e.g.
  `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`).
- 8 test files: update annotation to cite #2974 (the tracking issue
  opened for migrating these files to typed-IR assertions).
2026-05-01 16:14:39 -04:00

223 lines
8.9 KiB
JavaScript

/**
* Regression tests for #2838: SUMMARY rescue silently fails when .planning/
* is gitignored.
*
* The pre-fix rescue used `git ls-files --modified --others --exclude-standard`
* to detect uncommitted SUMMARY.md files. When projects gitignore .planning/
* (a common policy), --exclude-standard filters out the very files the rescue
* was meant to save, producing an empty result and skipping the rescue branch.
* The next line `git worktree remove --force` then permanently deleted the
* SUMMARY.
*
* The fix replaces git ls-files with a filesystem-level `find` + `cp` rescue
* that bypasses gitignore entirely.
*
* This test file:
* 1. Extracts the rescue block from each workflow file (parsed structurally
* by locating the labeled comment + closing fence — not free-form regex
* over file contents).
* 2. Runs the extracted block against a real temp repo whose .planning/
* directory is gitignored.
* 3. Asserts the SUMMARY is rescued into the main repo before worktree
* removal.
*/
'use strict';
// allow-test-rule: pending-migration-to-typed-ir [#2974]
// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md
// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern.
const { describe, test, before, after } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const os = require('os');
const { execFileSync, spawnSync } = require('child_process');
const REPO_ROOT = path.join(__dirname, '..');
const EXECUTE_PHASE_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'execute-phase.md');
const QUICK_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'quick.md');
/**
* Extract the rescue block (the bash lines that detect+rescue the
* uncommitted SUMMARY.md). We locate it by:
* - Finding the line that contains "Safety net" AND "SUMMARY"
* - Reading forward until the indent drops back to the surrounding level
* OR we hit a blank line followed by a non-comment, non-rescue line.
*
* To keep this robust, we scan from the safety-net comment forward until we
* either reach `done` (new fix) or `fi` (old fix) followed by a blank line.
*/
function extractRescueBlock(filePath) {
const lines = fs.readFileSync(filePath, 'utf-8').split('\n');
let startIdx = -1;
for (let i = 0; i < lines.length; i++) {
if (/Safety net/.test(lines[i]) && /SUMMARY/.test(lines[i])) {
startIdx = i;
break;
}
}
assert.notStrictEqual(startIdx, -1, `Could not find Safety net SUMMARY rescue block in ${filePath}`);
// Capture from comment through the terminator. The new fix ends with
// `done < <(find ... )`. The old fix ended with `fi` followed by a blank
// line. We accumulate until we find one of those terminators; if we see
// `done < <(find` we always stop there (it can only appear after `fi`).
const collected = [];
let sawFi = false;
for (let i = startIdx; i < lines.length; i++) {
collected.push(lines[i]);
const trimmed = lines[i].trim();
if (/^done\s*<\s*<\(find/.test(trimmed)) return collected.join('\n');
if (/^fi\s*$/.test(trimmed) && i > startIdx + 3) {
sawFi = true;
// Peek next line — if it's blank and the line after is not part of
// the new-fix `done`, treat fi as terminator (old block).
const next = (lines[i + 1] || '').trim();
const next2 = (lines[i + 2] || '').trim();
const newFixContinues = /^done\s*<\s*<\(find/.test(next) || /^done\s*<\s*<\(find/.test(next2);
if (!newFixContinues) {
return collected.join('\n');
}
}
}
assert.fail(`No terminator found in rescue block of ${filePath}`);
return collected.join('\n');
}
function sh(cwd, cmd) {
const r = spawnSync('bash', ['-c', cmd], { cwd, encoding: 'utf-8' });
if (r.status !== 0) {
throw new Error(`Command failed (${r.status}): ${cmd}\nstdout: ${r.stdout}\nstderr: ${r.stderr}`);
}
return r.stdout;
}
/**
* Build a temp repo that mirrors the bug repro from issue #2838 and run
* the rescue block against it. Returns { tmp, wt, summaryFinalPath }.
*/
function runRescueScenario(rescueBlock) {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2838-'));
const wt = path.join(tmp, 'wt');
sh(tmp, 'git init -q -b main');
sh(tmp, 'git config user.email test@example.com && git config user.name test');
fs.writeFileSync(path.join(tmp, '.gitignore'), '.planning/\n');
fs.writeFileSync(path.join(tmp, 'README.md'), 'init\n');
sh(tmp, 'git add .gitignore README.md && git commit -q -m init');
// Create worktree on a feature branch
sh(tmp, `git worktree add -q -b sim-executor "${wt}"`);
// Simulate the executor: untracked SUMMARY.md under gitignored .planning/
const summaryDir = path.join(wt, '.planning', 'quick', '260429-repro');
fs.mkdirSync(summaryDir, { recursive: true });
const summarySrc = path.join(summaryDir, '260429-repro-SUMMARY.md');
fs.writeFileSync(summarySrc, 'status: complete\nrescued: yes\n');
// Confirm precondition: --exclude-standard misses the file (this is the bug)
const ls = sh(tmp, `git -C "${wt}" ls-files --modified --others --exclude-standard -- "*SUMMARY.md" || true`);
assert.strictEqual(ls.trim(), '', 'Precondition: --exclude-standard must hide gitignored SUMMARY');
// Run the rescue block. It expects WT, WT_BRANCH to be set, and to be run
// from the main repo root.
const script = `
set -u
WT="${wt}"
WT_BRANCH="sim-executor"
cd "${tmp}"
${rescueBlock}
`;
const r = spawnSync('bash', ['-c', script], { cwd: tmp, encoding: 'utf-8' });
// Don't fail on non-zero — original block has || true everywhere; we
// judge by outcome.
// Now do the worktree removal that would have lost the file
sh(tmp, `git worktree remove "${wt}" --force`);
const summaryFinalPath = path.join(tmp, '.planning', 'quick', '260429-repro', '260429-repro-SUMMARY.md');
return { tmp, summaryFinalPath, rescueOut: r.stdout + r.stderr };
}
function cleanup(tmp) {
try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {}
}
describe('bug-2838: SUMMARY rescue handles gitignored .planning/', () => {
test('execute-phase.md rescue block recovers SUMMARY when .planning/ is gitignored', () => {
const block = extractRescueBlock(EXECUTE_PHASE_PATH);
const { tmp, summaryFinalPath, rescueOut } = runRescueScenario(block);
try {
assert.ok(
fs.existsSync(summaryFinalPath),
`SUMMARY was lost — rescue did not surface the file into main repo.\nRescue output:\n${rescueOut}`
);
const content = fs.readFileSync(summaryFinalPath, 'utf-8');
assert.match(content, /rescued: yes/);
} finally {
cleanup(tmp);
}
});
test('quick.md rescue block recovers SUMMARY when .planning/ is gitignored', () => {
const block = extractRescueBlock(QUICK_PATH);
const { tmp, summaryFinalPath, rescueOut } = runRescueScenario(block);
try {
assert.ok(
fs.existsSync(summaryFinalPath),
`SUMMARY was lost — rescue did not surface the file into main repo.\nRescue output:\n${rescueOut}`
);
const content = fs.readFileSync(summaryFinalPath, 'utf-8');
assert.match(content, /rescued: yes/);
} finally {
cleanup(tmp);
}
});
test('rescue is idempotent when SUMMARY already present in main repo', () => {
const block = extractRescueBlock(EXECUTE_PHASE_PATH);
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2838-idem-'));
const wt = path.join(tmp, 'wt');
try {
sh(tmp, 'git init -q -b main');
sh(tmp, 'git config user.email test@example.com && git config user.name test');
fs.writeFileSync(path.join(tmp, '.gitignore'), '.planning/\n');
fs.writeFileSync(path.join(tmp, 'README.md'), 'init\n');
sh(tmp, 'git add .gitignore README.md && git commit -q -m init');
sh(tmp, `git worktree add -q -b sim-executor "${wt}"`);
const dir = path.join(wt, '.planning', 'quick', 'x');
fs.mkdirSync(dir, { recursive: true });
const body = 'identical\n';
fs.writeFileSync(path.join(dir, 'x-SUMMARY.md'), body);
// Pre-place the same content in main repo
const mainDir = path.join(tmp, '.planning', 'quick', 'x');
fs.mkdirSync(mainDir, { recursive: true });
fs.writeFileSync(path.join(mainDir, 'x-SUMMARY.md'), body);
const script = `
set -u
WT="${wt}"
WT_BRANCH="sim-executor"
cd "${tmp}"
${block}
`;
const r = spawnSync('bash', ['-c', script], { cwd: tmp, encoding: 'utf-8' });
assert.strictEqual(
r.status,
0,
`Rescue block failed unexpectedly.\nstdout: ${r.stdout}\nstderr: ${r.stderr}`
);
// No "Rescued" message expected because cmp -s matches.
assert.doesNotMatch(r.stdout + r.stderr, /Rescued .*x-SUMMARY\.md/);
assert.strictEqual(fs.readFileSync(path.join(mainDir, 'x-SUMMARY.md'), 'utf-8'), body);
} finally {
try { sh(tmp, `git worktree remove "${wt}" --force`); } catch (_) {}
cleanup(tmp);
}
});
});