Files
msd-core/tests/code-review.test.cjs
Tom Boucher 15af0f5536 enhance(#3951): B6+B7 — widen two unreachable lint rules and make the guard ledger true (#3965)
* fix(#3951): two lint rules that could not reach the code they govern

B6 names two widenings. Measuring them first turned up a defect the criterion did
not know about, and refuted the reason it gave for one of them.

1. no-adhoc-markdown-parsing self-gates on its own filename.

   Lines 107-110 short-circuit create() to {} unless the path matches
   /(?:^|\/)src\/[^/]+\.cts$/. B6 says to widen the files: glob in
   eslint.config.mjs - but doing only that ships an INERT rule, because the gate
   still returns {} for every new path. Both halves have to change, and the gate
   is the load-bearing one.

   That same regex hides a live hole: [^/]+ is FLAT-ONLY, so it requires the file
   to sit directly in src/. The registered glob is src/**/*.cts, which includes
   subdirectories. 28 .cts files - health-diagnostic-rules/ (10),
   installer-migrations/ (11), observability/ (3), host-integration-adapters/ (2),
   vendor/ (2) - are inside the registered glob and silently skipped.

   Measured with the gate neutralized: 0 violations there today. The hole is
   hiding nothing right now, and is fixed anyway, because "no violations today" is
   not a property that keeps holding.

   The fix is not invented: require-subprocess-timeout.cjs:196 already carries the
   correct form of this guard, /(?:^|\/)src\/.*\.cts$/ with .*, one directory over.
   Checked the other 21 rules for the same bug - no-adhoc-regex-escape and
   no-private-binary-resolution short-circuit only to exempt their own seam file,
   which is the right shape, and no-crlf-fragile-split has no filename gate at
   all. This bug is unique to the one rule.

2. no-adhoc-regex-escape could not see the shape that actually occurs.

   Line 396 gated the whole UNSAFE-NEW-REGEXP arm on arg.type === 'Identifier'.
   Every check below it - the _SOURCE provenance check, the
   isSoleReturnOfOwnParameter shape - lives inside that branch, so
   new RegExp(obj['key']) and new RegExp(cfg.pattern) were never examined at all.
   Runtime data arrives as a property access far more often than as a bare
   identifier, which is exactly why this rule never fired on the #3477 ReDoS.

   Widened to MemberExpression, measured by AST walk across all five registered
   blocks rather than by grep. 27 sites, zero TSAsExpression:

     18  safe new RegExp(X.source, flags)  -> exempted, keyed strictly on the
         PROPERTY being `source`, never on the object. Keying on the object would
         wave through X.anything and buy nothing. B6 estimated ~10; that was an
         undercount.
      3  _SOURCE-suffixed constants reached through a required module namespace
         (phaseId.BRACKET_PHASE_TOKEN_SOURCE) -> the same provenance-exempt class
         the rule already recognizes for bare identifiers, extended to reach them.
         Without this the widening produces 3 false flags.
      6  real findings -> marked, each a test extracting a pattern from a shipped
         file at test time, where the runtime contract IS the product.

   Deliberately the NARROW MemberExpression form. The rule's own
   isSoleReturnOfOwnParameter doc comment records that an earlier broad
   "any non-literal identifier" heuristic produced ~25 false positives and was
   rejected; a re-run of the census after this change flags exactly the 6 above
   and nothing else.

Verified by execution, not by reading: the gate now accepts src/<subdir>/x.cts,
still accepts flat src/x.cts, and still exempts paths outside src/ - each pinned
by a test proven to fail against the old regex. build:lib, lint and lint:ci all
exit 0.

Refs #3951

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

* fix(#3951): give no-adhoc-markdown-parsing its reach, and fix the 80 parses it finds

The rule self-gates on filename AND is registered on one glob, so widening either
half alone is inert. Both move here: the gate now accepts tests/**/*.cjs and
scripts/**/*.cjs alongside src/**/*.cts, and eslint.config.mjs registers it on the
same two.

A test pins that the gate and the registration AGREE, in both directions. The
original defect was a gate narrower than its registration; the failure mode of
this fix is a gate wider than its registration. Both are silent, so the test
asserts the pair rather than either half.

80 violations across 43 files, all in tests/, zero in scripts/. 70 are routed
through the existing seams - scanFencedBlocks, collectSection, stripFencedCode,
tokenizeHeadings from markdown-sectionizer; splitTableRow, parseMarkdownTable,
findTableWithColumns from markdown-table. Headerless STATE.md tables use
splitTableRow per line, because parseMarkdownTable needs a real delimiter row.

10 are suppressed, 12.5%, well under the third that would have meant the rule is
mis-scoped for tests/ rather than the tests carrying debt. Each names its reason:
three regression guards (#3873 / bug-#21) are deliberately independent of the
generator's own fence handling, and routing them through the seam would have them
test the generator against itself; one is a negative-text probe that extracts
nothing; six are a shell-pipe-to-jq detector whose regex coincidentally matches the
table fingerprint and is not markdown parsing at all.

All ten sit in tests whose subject is .md content, which is normally a reason to
prefer the seam. The marker used is allow-adhoc-markdown, distinct from
no-source-grep's allow-test-rule, and lint:ci's lint-allow-test-rule-refs reports
the same 280/280 unverified count as before - checked rather than assumed, because
those two markers are easy to conflate.

The widening earned its keep immediately: it found a test that passed for the
wrong reason.

  tests/config-field-docs.test.cjs asserted notEqual(<cell>, '600') against the
  TYPE column instead of the DEFAULT column. notEqual('number', '600') is true
  forever, so the guard against workflow.subagent_timeout regressing to the old
  seconds default could never fire. docs/CONFIGURATION.md:434 is
  `| workflow.subagent_timeout | number | 300000 | ... |`, so the default is cell
  index 2; the assertion is now row-scoped through splitTableRow and reads 300000.

That is the argument for the widening in one case: the violation was invisible to
lint, the suite was green, and the assertion was vacuous. A rule that cannot reach
a file cannot tell you the file is lying.

Not fixed here, and recorded rather than assumed: #3426/#3239 are NOT reachable by
this widening. tests/package-legitimacy-gate.test.cjs yields zero violations even
with the gate bypassed - its hand-rolled scans are real, but built from line
filters and split('|') rather than the regex-literal fingerprints this rule
detects. They need new detectors. The epic assumed a wider glob would catch them.

build:lib, lint and lint:ci all exit 0; the post-fix census across tests/** and
scripts/** is 0 violations.

Refs #3951

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

* fix(#3951): B7 — and #3356's defects were still live in the code

B7 asks that each closed child be driven fail-first with a behavioral identity
test at the CONSUMER's output. Four of eleven children had no test citing their
issue number. Auditing them by BEHAVIOR rather than by number-grep changed the
answer for three of the four.

#3364 and #2540 — traceability only. Both were implemented by #3941 and their
consumer-output tests exist and were shown failing-first; neither cited its
originating issue, so an audit that greps for the number reports them uncovered.
Tagged the specific asserting test in each file, following the citation form those
files already use.

#3372 — covered, but only at helper level, and the triage narrowed it. Of the four
commands the issue names, only estimate-cli's collectCalibrationSamples actually
enumerates phase dirs from disk; smart-entry, audit and roadmap-upgrade derive from
ROADMAP/body text and never reach the sentinel path, so they are benign by
construction and were left alone rather than "fixed" into churn. The existing #3882
rows asserted the helper's return value. Added a consumer-output test driving
`query estimate-calibrate` and asserting sample_count and the persisted document.
RED proof: reverted collectCalibrationSamples to a raw readdirSync and ran the real
CLI - sample_count 3, sentinel leaked; restored - sample_count 2.

#3356 — NOT covered, and BOTH halves of the defect were still live in source. The
issue is closed; the bug was not fixed. Fixed here rather than writing tests that
document a bug as correct.

  Defect 1, the contradicted row. quick.md:627 claimed
  `quick-tasks-append` performs "the equivalent write" to the Step 7c row. It did
  not: the `#` cell was a positional ordinal and `Directory` read `—`, because the
  route had no way to receive a quick id or task directory. Added OPTIONAL
  `--quick-id` / `--slug` / `--directory`. A caller with neither - fast.md, the
  original #2133 caller - omits them and gets the byte-identical prior row, so
  nothing existing changes. A caller that HAS a real id and directory now gets the
  canonical row quick.md:632 renders. The false-equivalence sentence itself is
  corrected rather than left to mislead the next reader.

  Defect 2, the forced re-derive. The route called readModifyWriteStateMd with no
  options, so a body-only append to the Quick Tasks table triggered a full
  re-derive of the disk-derived progress.* frontmatter. Every other body-only
  writer passes { resync: false } - src/state.cts's own docstring prescribes it -
  and this route was the lone outlier. RED proof: reverted the option, seeded a
  project with 2 real phase dirs and a curated total_phases of 25, ran
  quick-tasks-append; total_phases collapsed to 2. Restored; it stayed 25.

That second one is the shape this epic exists to close: a silent write that
replaces curated state with a re-derivation nobody asked for, exit 0 throughout.

build:lib, lint and lint:ci all exit 0.

Refs #3951

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

* docs(#3951): amend B6's ledger to what was measured, and document the new flags

The ADR gains a ledger amendment in its own correction style - the sixth wrong
premise it records, found the same way as the other five, by measuring before
building.

B6 says the net guard count must fall. It rose: 62 -> 69, +7, measured from the
epic's filing commit to origin/next. The attribution is the point, though. Five of
the seven came from PRs unrelated to this epic, one was added by a phase of it, and
the epic did retire something sub-file - #3884 removed a detector with an explicit
"net: -1 detector, 0 added" ledger. Every named casualty is load-bearing, two
already carry retractions in this same document, and a sweep of all 22 rules plus
every scripts/lint-* found no provably dead guard. There is no honest way to make
the count fall; forcing it would trade coverage for a number, which is the Goodhart
outcome Decision 6 exists to prevent.

The amendment also records that B6's own prescribed fix for one widening was inert.
no-adhoc-markdown-parsing self-gates on its filename, so widening only the files:
glob - which is what the criterion says to do - ships a rule that still returns {}
for every new path. And #3426/#3239 are not reachable by that widening at all;
their scans use line filters and split('|'), not the regex fingerprints the rule
detects. The roster row tracked them against the wrong mechanism.

Three roster rows updated from aspiration to fact: the two widenings are DONE with
their measured counts, and lint-phase-enumeration-drift is marked RETAINED rather
than "expected casualty - verify before retiring", because Phase 5 verified it and
kept it.

The rule Decision 6 should carry forward is stated plainly: a guard ledger is a
claim about COVERAGE, not about COUNT. "Net count must fall" is measurable and
wrong. "Every guard is reachable, and each retirement names what makes its defect
unrepresentable" is the property that was actually wanted.

CLI-TOOLS.md documents the optional --quick-id/--slug/--directory flags and says
plainly that omitting them keeps the pre-#3356 row byte-identical, plus that the
append no longer re-derives progress frontmatter.

New features fragment (id 3951); FEATURES.md regenerated rather than hand-edited.
Changeset is Changed, pr:0 pending backfill.

Refs #3951

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

* test(#3951): correct four rows that pinned the lint rule's old narrow reach

The remote suite came back RED with 5 failures, all in tests/eslint-rules.test.cjs.
They are stale tests, not a regression: four rows assert that
no-adhoc-markdown-parsing is inert outside src/*.cts, which is exactly the
contract this deliverable changes.

Confirmed by reading rather than inferred from the names - the row at :1981 used
filename: 'tests/some.test.cjs' and filename: 'scripts/helper.cjs', the two roots
the rule now covers on purpose.

Worth recording WHY local gates missed this. npm run lint and lint:ci were green,
and the touched test files passed standalone. Lint only reports violations in real
files; these rows assert the rule's REACH using synthetic RuleTester filenames, so
nothing but the full suite could see them. Local green on a rule change says
nothing about the rule's own tests.

Each row is rewritten with BOTH halves rather than flipped from valid to invalid:

  - the same fingerprint under tests/ or scripts/ is now flagged, with the right
    messageId
  - the negative space is preserved - the same fingerprint under a path outside
    all three roots (gsd-core/bin/lib/foo.cjs) is still NOT flagged

The second half is the one that matters. Without it the rule has no boundary and
nothing would catch an over-wide gate later, which is the mirror image of the bug
this deliverable just fixed.

Each row is renamed to state the current contract; the old names said
"non-src/*.cts ... is not flagged" and would have been actively misleading once
the bodies changed.

Proven to test the widening rather than restate it: every flagged half was run
against HEAD~2's pre-widening rule and does NOT fire there, then against the
current rule and does. 12/12 on that probe; the full file is 178/178.

Swept for the same staleness elsewhere and found none.
require-subprocess-timeout's own "inert outside src/*.cts" row is untouched -
that rule's gate was not widened here - and no-adhoc-regex-escape's test file
already carries correctly-targeted rows.

Refs #3951

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

* test(#3951): acknowledge the quick.md growth the attribution guard reported

The full suite came back RED with one failure, and it is mine:

  1 file(s) grew without an acknowledgment:
    quick.md grew 364 bytes

gsd-core/workflows/quick.md is runtime-loaded emitted content, so correcting
its false 'performs the equivalent write' claim trips emitted-attribution by
construction. This is the acknowledgment, not a workaround - there is nothing
to regenerate.

The fragment names ONE path, which is the only one the guard reported. The four
spent acknowledgments it also listed (audit-uat, plan-phase, progress, review)
belong to other fragments whose ripple the base already absorbs; they are inert,
not failures, and are deliberately NOT copied here - naming paths I did not
change would make this record false in the other direction.

Byte figure corrected before committing: the guard reported 37220 -> 37584
(+364), but origin/next has since moved and quick.md is 37232 there now, so the
measured delta is +352. The reason text says so and names the base as a moving
figure rather than pinning a number that is already stale.

Refs #3951

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

* test(#3951): move the quick.md growth ack to a trailer, delete the obsolete fragment

The acknowledgment mechanism changed under this branch. Merging next brought in
the redesign - it also deleted .github/workflows/ack-fragment-sweep.yml, which
was in the merge status and which I did not register at the time - and the guard
now says so directly:

  Add a trailer to a commit in this PR (never a new file).
    Emitted-Drift-Ack-Growth: quick.md - <why this growth is deliberate>

So tests/emitted-drift-acks/3951-quick-append-equivalence.json is obsolete on
arrival. A fragment file is no longer read by anything, and leaving it would be a
dead record that looks like an active one. It is deleted here rather than kept
"just in case".

The byte figure moved again with the merge: 37232 -> 37596, +364. The earlier
fragment said +352, measured before the merge auto-merged quick.md itself. The
trailer carries no number, which is the better design - the figure was stale
twice in two attempts.

Refs #3951

Emitted-Drift-Ack-Growth: quick.md — #3356/#3951 replaces a false claim with an accurate one. Line 627 said the `quick-tasks-append` shortcut "performs the equivalent write" to the Step 7c row rendered above it; it did not, and that was the documented half of #3356 — with no quick id or task directory the route emitted a positional ordinal in `#` and an em-dash in `Directory`, a visibly different row. The corrected sentence has to carry three facts the original elided: what the shortcut actually writes when it has neither input, that this is honest behavior for its real caller (`fast.md`, which has neither), and how a caller with both now gets the byte-identical canonical row via the new optional `--quick-id`/`--slug`/`--directory` flags. Prose is the product here — an executing agent reads this line to decide whether the shortcut is safe for its case, and a shorter correction would either drop the flags (leaving the reader unable to act on the fix) or drop the limitation (recreating the false claim in gentler words).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* chore(#3951): backfill changeset pr number

Refs #3951

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

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-08-27 23:10:49 -04:00

814 lines
37 KiB
JavaScript

// allow-test-rule: source-text-is-the-product
// Reads .md/.json/.yml product files whose deployed text IS what the
// runtime loads — testing text content tests the deployed contract.
/**
* GSD Code Review Tests
*
* Validates all code review artifacts from Phases 1-4:
* - Agent frontmatter (gsd-code-reviewer, gsd-code-fixer)
* - Command structure (code-review.md, code-review-fix.md)
* - Workflow structure (code-review.md, code-review-fix.md)
* - Config key registration (workflow.code_review, workflow.code_review_depth)
* - Workflow integration points (execute-phase, quick, autonomous)
*
* Test structure:
* - CR-AGENT: Hermetic agent tests (repo files only)
* - CR-CMD: Hermetic command tests (repo files only)
* - CR-WORKFLOW: Hermetic workflow tests (repo files only)
* - CR-CONFIG: Hermetic config tests (repo files only)
* - CR-INTEGRATION: Conditional integration tests (skip if plugin dir absent)
*/
const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const { scanFencedBlocks } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs');
/** Return the raw text of every ```bash fenced block in `content`. */
function extractBashBlocks(content) {
const lines = content.split(/\r?\n/);
const blocks = [];
for (const block of scanFencedBlocks(lines)) {
if (block.closeLineIdx === -1) continue;
if ((block.infoString || '').trim().toLowerCase() !== 'bash') continue;
blocks.push(lines.slice(block.openLineIdx, block.closeLineIdx + 1).join('\n'));
}
return blocks;
}
const os = require('os');
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs');
// --- Test Environment Setup ---
const AGENTS_DIR = path.join(__dirname, '..', 'agents');
const COMMANDS_DIR = path.join(__dirname, '..', 'commands', 'gsd');
const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows');
/**
* Parse top-level (non-nested, non-escaped) Skill() invocations from a workflow .md file.
*
* Returns an array of structured objects: [{ skill, args }]
* - `skill` is the value of the `skill="..."` keyword argument
* - `args` is the value of the `args="..."` keyword argument (or null if absent)
*
* Skips occurrences inside escaped string contexts like
* prompt="... Skill(skill=\"x\", args=\"y\") ..."
* by walking the file character-by-character and tracking whether we are inside
* a double-quoted string. Escaped quotes (\") are treated as literal content.
*
* This avoids regex/.includes() text-matching: callers receive a structured list
* and assert against fields and tokenized args.
*/
function parseWorkflowSkillInvocations(content) {
const invocations = [];
let i = 0;
let inString = false;
while (i < content.length) {
const ch = content[i];
if (inString) {
if (ch === '\\' && i + 1 < content.length) {
// Skip escape sequence (e.g. \" or \\)
i += 2;
continue;
}
if (ch === '"') {
inString = false;
}
i += 1;
continue;
}
if (ch === '"') {
inString = true;
i += 1;
continue;
}
// Look for top-level "Skill(" at this position
if (content.startsWith('Skill(', i)) {
const callStart = i + 'Skill('.length;
// Find the matching close paren, respecting strings/escapes inside the call
let j = callStart;
let depth = 1;
let innerInString = false;
while (j < content.length && depth > 0) {
const c = content[j];
if (innerInString) {
if (c === '\\' && j + 1 < content.length) {
j += 2;
continue;
}
if (c === '"') innerInString = false;
j += 1;
continue;
}
if (c === '"') {
innerInString = true;
} else if (c === '(') {
depth += 1;
} else if (c === ')') {
depth -= 1;
if (depth === 0) break;
}
j += 1;
}
const callBody = content.slice(callStart, j);
const parsed = parseSkillCallBody(callBody);
if (parsed) invocations.push(parsed);
i = j + 1;
continue;
}
i += 1;
}
return invocations;
}
/**
* Parse the body of a Skill(...) call into { skill, args }.
* Body looks like: skill="name", args="value" (args optional).
* Returns null if no skill keyword is found.
*/
function parseSkillCallBody(body) {
const kwargs = {};
const isIdentChar = (c) => /[A-Za-z0-9_]/.test(c);
const isWs = (c) => /\s/.test(c);
let i = 0;
while (i < body.length) {
// Skip whitespace and commas
while (i < body.length && (isWs(body[i]) || body[i] === ',')) i += 1;
if (i >= body.length) break;
// Read identifier key
const keyStart = i;
while (i < body.length && isIdentChar(body[i])) i += 1;
const key = body.slice(keyStart, i);
if (!key) break;
// Expect '='
while (i < body.length && isWs(body[i])) i += 1;
if (body[i] !== '=') break;
i += 1;
while (i < body.length && isWs(body[i])) i += 1;
// Expect quoted value
if (body[i] !== '"') break;
i += 1;
let value = '';
while (i < body.length) {
const c = body[i];
if (c === '\\' && i + 1 < body.length) {
value += body[i + 1];
i += 2;
continue;
}
if (c === '"') {
i += 1;
break;
}
value += c;
i += 1;
}
kwargs[key] = value;
}
if (!('skill' in kwargs)) return null;
return { skill: kwargs.skill, args: 'args' in kwargs ? kwargs.args : null };
}
// Plugin directory resolution (cross-platform safe)
const PLUGIN_WORKFLOWS_DIR = process.env.GSD_PLUGIN_ROOT || path.join(os.homedir(), '.claude', 'gsd-core', 'workflows');
const PLUGIN_AVAILABLE = fs.existsSync(PLUGIN_WORKFLOWS_DIR);
// --- CR-AGENT: code review agent frontmatter ---
describe('CR-AGENT: code review agent frontmatter', () => {
test('gsd-code-reviewer.md has required frontmatter fields', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-reviewer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('name:'), 'gsd-code-reviewer missing name:');
assert.ok(frontmatter.includes('description:'), 'gsd-code-reviewer missing description:');
assert.ok(frontmatter.includes('tools:'), 'gsd-code-reviewer missing tools:');
assert.ok(frontmatter.includes('color:'), 'gsd-code-reviewer missing color:');
});
test('gsd-code-fixer.md has required frontmatter fields', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('name:'), 'gsd-code-fixer missing name:');
assert.ok(frontmatter.includes('description:'), 'gsd-code-fixer missing description:');
assert.ok(frontmatter.includes('tools:'), 'gsd-code-fixer missing tools:');
assert.ok(frontmatter.includes('color:'), 'gsd-code-fixer missing color:');
});
test('gsd-code-reviewer.md has Read, Bash, Glob, Grep, Write tools', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-reviewer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('Read'), 'gsd-code-reviewer missing Read tool');
assert.ok(frontmatter.includes('Bash'), 'gsd-code-reviewer missing Bash tool');
assert.ok(frontmatter.includes('Glob'), 'gsd-code-reviewer missing Glob tool');
assert.ok(frontmatter.includes('Grep'), 'gsd-code-reviewer missing Grep tool');
assert.ok(frontmatter.includes('Write'), 'gsd-code-reviewer missing Write tool');
});
test('gsd-code-fixer.md has Read, Edit, Write, Bash, Grep, Glob tools', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('Read'), 'gsd-code-fixer missing Read tool');
assert.ok(frontmatter.includes('Edit'), 'gsd-code-fixer missing Edit tool');
assert.ok(frontmatter.includes('Write'), 'gsd-code-fixer missing Write tool');
assert.ok(frontmatter.includes('Bash'), 'gsd-code-fixer missing Bash tool');
});
test('gsd-code-reviewer.md does not have skills: in frontmatter', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-reviewer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(!frontmatter.includes('skills:'),
'gsd-code-reviewer has skills: in frontmatter — breaks Gemini CLI');
});
test('gsd-code-fixer.md does not have skills: in frontmatter', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(!frontmatter.includes('skills:'),
'gsd-code-fixer has skills: in frontmatter — breaks Gemini CLI');
});
test('gsd-code-fixer.md rollback uses git checkout (not Write tool)', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
assert.ok(content.includes('git checkout --'),
'gsd-code-fixer rollback should use git checkout -- {file} for atomic rollback');
assert.ok(!content.includes('PRE_FIX_CONTENT'),
'gsd-code-fixer should not use PRE_FIX_CONTENT in-memory capture (use git checkout instead)');
});
test('gsd-code-fixer.md success_criteria consistent with rollback strategy (git checkout)', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
// eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own agent .md content, fixed-size author-controlled content
const successCriteria = content.match(/<success_criteria>([\s\S]*?)<\/success_criteria>/)?.[1] || '';
assert.ok(successCriteria.includes('git checkout'),
'gsd-code-fixer success_criteria must reference git checkout rollback');
assert.ok(!successCriteria.includes('Write tool with captured'),
'gsd-code-fixer success_criteria must not say Write tool for rollback');
});
test('gsd-code-fixer.md flags logic-bug fixes for human review', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
assert.ok(content.includes('requires human verification'),
'gsd-code-fixer should flag logic-bug fixes as requiring human verification');
});
test('gsd-code-reviewer.md REVIEW.md spec includes files_reviewed_list field', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-reviewer.md'), 'utf-8');
assert.ok(content.includes('files_reviewed_list'),
'gsd-code-reviewer REVIEW.md frontmatter spec must include files_reviewed_list for --auto scope persistence');
});
// #2825: gsd-code-fixer is the only writer that hand-rolls a git worktree; it
// must honor workflow.use_worktrees (the documented opt-out) like its four
// sibling writer workflows, and never rm -rf a possible Windows reparse point.
test('#2825 gsd-code-fixer.md reads workflow.use_worktrees and gates git worktree add on it', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
assert.ok(
content.includes('workflow.use_worktrees'),
'gsd-code-fixer setup_worktree must read the workflow.use_worktrees config flag (#2825)',
);
// The git worktree add must be CONDITIONAL on the flag, not unconditional.
// Locate the worktree-add line and confirm a USE_WORKTREES gate precedes it.
assert.ok(
/USE_WORKTREES=.false./.test(content) || content.includes('if [ "$USE_WORKTREES" = "false" ]'),
'gsd-code-fixer must gate worktree creation on USE_WORKTREES=false (skip when opted out) (#2825)',
);
});
test('#2825 gsd-code-fixer.md forbids rm -rf on a possible reparse point (Windows junction safety)', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
assert.ok(
/rm -rf.*reparse point|reparse point.*rm -rf|NEVER .rm -rf.|never use .rm -rf/i.test(content),
'gsd-code-fixer must forbid rm -rf on a possible reparse point/junction (#2825) — on Windows that is the delete-the-target path',
);
});
test('#2825 gsd-code-fixer.md records where verification ran (main checkout vs worktree)', () => {
const content = fs.readFileSync(path.join(AGENTS_DIR, 'gsd-code-fixer.md'), 'utf-8');
assert.ok(
// eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own agent .md content, fixed-size author-controlled content
/verification[\s\S]*(main checkout|worktree)|(main checkout|worktree)[\s\S]*verification/i.test(content),
'gsd-code-fixer REVIEW-FIX.md must record where verification ran (main checkout vs worktree) so a reader knows if the numbers are reproducible (#2825)',
);
});
});
// --- CR-CMD: code review command structure ---
describe('CR-CMD: code review command structure', () => {
test('code-review.md has correct frontmatter name: gsd:code-review', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('name: gsd:code-review'),
'code-review.md missing correct name in frontmatter');
});
// #2790: code-review-fix.md was consolidated into code-review.md as the --fix flag.
test('code-review.md has --fix flag absorbing code-review-fix (#2790)', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('--fix'),
'code-review.md must document --fix flag (absorbed code-review-fix)');
});
test('code-review.md references workflow: code-review.md', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('code-review.md'),
'code-review.md does not reference its workflow');
});
test('code-review.md references code-review-fix workflow via --fix (#2790)', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('code-review-fix') || content.includes('--fix'),
'code-review.md must reference code-review-fix workflow or --fix flag');
});
test('code-review.md has argument-hint in frontmatter', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('argument-hint:'),
'code-review.md missing argument-hint');
});
test('code-review.md argument-hint includes --fix flag (#2790: absorbed code-review-fix)', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('argument-hint:') && content.includes('--fix'),
'code-review.md must have argument-hint with --fix');
});
test('code-review.md has allowed-tools in frontmatter', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('allowed-tools:'),
'code-review.md missing allowed-tools');
});
test('code-review.md has allowed-tools in frontmatter (covers fix too, #2790)', () => {
const content = fs.readFileSync(path.join(COMMANDS_DIR, 'code-review.md'), 'utf-8');
const frontmatter = content.split('---')[1] || '';
assert.ok(frontmatter.includes('allowed-tools:'),
'code-review.md missing allowed-tools');
});
});
// --- CR-WORKFLOW: code review workflow structure ---
describe('CR-WORKFLOW: code review workflow structure', () => {
test('code-review.md workflow has <step name="initialize">', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('<step name="initialize">'),
'code-review.md workflow missing initialize step');
});
test('code-review.md workflow has <step name="check_config_gate">', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('<step name="check_config_gate">'),
'code-review.md workflow missing check_config_gate step');
});
test('code-review.md workflow references gsd-code-reviewer agent', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8');
assert.ok(content.includes('gsd-code-reviewer'),
'code-review.md workflow does not reference gsd-code-reviewer agent');
});
test('code-review-fix.md workflow has <step name="initialize">', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review-fix.md'), 'utf-8');
assert.ok(content.includes('<step name="initialize">'),
'code-review-fix.md workflow missing initialize step');
});
test('code-review-fix.md workflow references gsd-code-fixer agent', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review-fix.md'), 'utf-8');
assert.ok(content.includes('gsd-code-fixer'),
'code-review-fix.md workflow does not reference gsd-code-fixer agent');
});
test('code-review-fix.md workflow has iteration cap', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review-fix.md'), 'utf-8');
// Check for iteration logic with cap
assert.ok(content.includes('MAX_ITERATIONS') || (content.includes('3') && content.includes('iteration')),
'code-review-fix.md workflow missing iteration cap logic');
});
test('code-review.md --files path traversal guard rejects paths outside repo', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8');
// Guard must resolve and compare against REPO_ROOT
assert.ok(content.includes('REPO_ROOT') && content.includes('realpath'),
'code-review.md missing path traversal guard (realpath + REPO_ROOT check)');
assert.ok(content.includes('File path outside repository'),
'code-review.md missing rejection message for paths outside repo');
});
test('code-review.md uses portable while-read loop for array dedup (not mapfile)', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review.md'), 'utf-8');
// mapfile is bash 4+ only; macOS ships bash 3.2. Dedup must use portable while-read.
// Note: 'mapfile' may appear in platform_notes documentation — check bash code blocks only
const codeBlocks = extractBashBlocks(content);
const hasMapfileInCode = codeBlocks.some(block => block.includes('mapfile -t'));
assert.ok(!hasMapfileInCode,
'code-review.md bash code blocks use mapfile which is bash 4+ only — breaks macOS default bash 3.2');
assert.ok(content.includes('while IFS= read -r'),
'code-review.md should use portable while-read loop instead of mapfile');
});
test('code-review-fix.md uses portable while-read loop for array construction (not mapfile)', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'code-review-fix.md'), 'utf-8');
const codeBlocks = extractBashBlocks(content);
const hasMapfileInCode = codeBlocks.some(block => block.includes('mapfile -t'));
assert.ok(!hasMapfileInCode,
'code-review-fix.md bash code blocks use mapfile which is bash 4+ only — breaks macOS default bash 3.2');
assert.ok(content.includes('while IFS= read -r'),
'code-review-fix.md should use portable while-read loop instead of mapfile');
});
});
// --- CR-CONFIG: config key registration ---
describe('CR-CONFIG: config key registration', () => {
test('config-set accepts workflow.code_review', () => {
const tmpDir = createTempProject();
try {
const result = runGsdTools('config-set workflow.code_review true', tmpDir);
assert.ok(result.success, `config-set should accept workflow.code_review: ${result.error}`);
} finally {
cleanup(tmpDir);
}
});
test('config-set accepts workflow.code_review_depth', () => {
const tmpDir = createTempProject();
try {
const result = runGsdTools('config-set workflow.code_review_depth standard', tmpDir);
assert.ok(result.success, `config-set should accept workflow.code_review_depth: ${result.error}`);
} finally {
cleanup(tmpDir);
}
});
test('config-get workflow.code_review returns value set via config-set', (t) => {
const tmpDir = createTempProject();
t.after(() => cleanup(tmpDir));
const setResult = runGsdTools(['config-set', 'workflow.code_review', 'true'], tmpDir);
assert.ok(setResult.success, `config-set workflow.code_review failed: ${setResult.error}`);
const getResult = runGsdTools(['config-get', 'workflow.code_review'], tmpDir);
assert.ok(getResult.success, `config-get workflow.code_review failed: ${getResult.error}`);
assert.strictEqual(getResult.output, 'true',
`workflow.code_review should return "true", got ${getResult.output}`);
});
test('config-get workflow.code_review_depth returns value set via config-set', (t) => {
const tmpDir = createTempProject();
t.after(() => cleanup(tmpDir));
const setResult = runGsdTools(['config-set', 'workflow.code_review_depth', 'standard'], tmpDir);
assert.ok(setResult.success, `config-set workflow.code_review_depth failed: ${setResult.error}`);
const getResult = runGsdTools(['config-get', 'workflow.code_review_depth'], tmpDir);
assert.ok(getResult.success, `config-get workflow.code_review_depth failed: ${getResult.error}`);
assert.strictEqual(getResult.output, '"standard"',
`workflow.code_review_depth should return '"standard"', got ${getResult.output}`);
});
});
// --- CR-INTEGRATION: workflow integration points ---
describe('CR-INTEGRATION: workflow integration points', () => {
test('execute-phase.md contains code_review_gate step', { skip: !PLUGIN_AVAILABLE ? 'Plugin dir not installed' : false }, () => {
const content = fs.readFileSync(path.join(PLUGIN_WORKFLOWS_DIR, 'execute-phase.md'), 'utf-8');
assert.ok(content.includes('code_review_gate'),
'execute-phase.md missing code_review_gate step name');
});
test('execute-phase.md resolves code-review capability hook', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'execute-phase.md'), 'utf-8');
// eslint-disable-next-line local/no-unbounded-quantifier -- parses maintainer-authored workflow markdown, bounded prose, not adversarial input
const gateMatch = content.match(/<step name="code_review_gate"[^>]*>([\s\S]*?)<\/step>/);
assert.ok(gateMatch, 'execute-phase.md missing code_review_gate step');
const gateContent = gateMatch[1];
assert.ok(gateContent.includes('loop render-hooks execute:post'),
'execute-phase.md code_review_gate must resolve execute:post capability hooks');
assert.ok(gateContent.includes('ref.skill == "code-review"'),
'execute-phase.md code_review_gate must identify the code-review capability hook');
assert.ok(!gateContent.match(/config-get\s+workflow\.code_review/),
'execute-phase.md code_review_gate must not read workflow.code_review directly');
});
test('execute-phase.md does NOT contain ls.*REVIEW.md.*head pattern', { skip: !PLUGIN_AVAILABLE ? 'Plugin dir not installed' : false }, () => {
const content = fs.readFileSync(path.join(PLUGIN_WORKFLOWS_DIR, 'execute-phase.md'), 'utf-8');
// Extract code_review_gate section to check
// eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own workflow .md content, fixed-size author-controlled content
const gateMatch = content.match(/<step name="code_review_gate">([\s\S]*?)<\/step>/);
if (gateMatch) {
const gateContent = gateMatch[1];
assert.ok(!gateContent.match(/ls.*REVIEW\.md.*head/),
'execute-phase.md code_review_gate uses non-deterministic glob pattern (ls | head)');
}
});
test('quick.md contains code-review invocation', { skip: !PLUGIN_AVAILABLE ? 'Plugin dir not installed' : false }, () => {
const content = fs.readFileSync(path.join(PLUGIN_WORKFLOWS_DIR, 'quick.md'), 'utf-8');
assert.ok(content.includes('code-review') || content.includes('code_review'),
'quick.md missing code-review invocation');
});
test('quick.md resolves code-review capability hook', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'quick.md'), 'utf-8');
const start = content.indexOf('**Step 6.25: Code review (auto)**');
// #2994 (pre-existing since #2994's earlier quick-verification.md extraction,
// 18ff35d20): Step 6.5's content moved into
// gsd-core/workflows/quick/steps/quick-verification.md behind a
// `<!-- gsd:section id="quick-verification" -->` marker, so the literal
// "**Step 6.5: Verification" heading text no longer follows Step 6.25 in
// this file — the marker is the correct end-of-step delimiter now (mirrors
// phase6-review-capabilities.test.cjs's identical retarget for the same move).
const end = content.indexOf('<!-- gsd:section id="quick-verification"', start);
assert.ok(start !== -1 && end !== -1, 'quick.md missing Step 6.25 code review section');
const reviewContent = content.slice(start, end);
assert.ok(reviewContent.includes('loop render-hooks execute:post'),
'quick.md code review step must resolve execute:post capability hooks');
assert.ok(reviewContent.includes('ref.skill == "code-review"'),
'quick.md code review step must identify the code-review capability hook');
assert.ok(!reviewContent.match(/config-get\s+workflow\.code_review/),
'quick.md code review step must not read workflow.code_review directly');
});
// autonomous.md tests read from the repo's canonical workflow source (WORKFLOWS_DIR),
// not the user-installed plugin dir. The plugin dir can lag behind the repo until the
// user re-installs, so asserting against it produces false negatives. The repo file
// is the source of truth and is always present in CI checkouts.
test('autonomous.md contains gsd-code-review skill invocation', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'autonomous.md'), 'utf-8');
// Parse Skill(...) invocations into structured objects and assert canonical
// hyphen form is referenced. Canonical command form is hyphen
// (gsd-code-review); colon form (gsd:code-review) is the legacy
// frontmatter-name form removed in PR #2819.
const invocations = parseWorkflowSkillInvocations(content);
const skillNames = invocations.map(inv => inv.skill);
assert.ok(skillNames.includes('gsd-code-review'),
`autonomous.md must invoke Skill(skill="gsd-code-review", ...); found skills: ${JSON.stringify(skillNames)}`);
assert.ok(!skillNames.includes('gsd:code-review'),
'autonomous.md must not use legacy colon form gsd:code-review (canonical is hyphen form)');
});
test('autonomous.md auto-fix uses consolidated gsd-code-review --fix invocation (#2790)', () => {
// After #2790, gsd-code-review-fix was absorbed into gsd-code-review as
// the --fix flag. The autonomous workflow must invoke the consolidated
// form, not the deleted gsd-code-review-fix skill.
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'autonomous.md'), 'utf-8');
const invocations = parseWorkflowSkillInvocations(content);
const skillNames = invocations.map(inv => inv.skill);
assert.ok(!skillNames.includes('gsd-code-review-fix'),
`autonomous.md must not invoke deleted gsd-code-review-fix skill (consolidated into --fix); found: ${JSON.stringify(skillNames)}`);
assert.ok(!skillNames.includes('gsd:code-review-fix'),
'autonomous.md must not use legacy colon form gsd:code-review-fix');
// Find a gsd-code-review invocation that carries the --fix flag (the
// consolidated auto-fix entry point).
const fixInvocation = invocations.find(inv => {
if (inv.skill !== 'gsd-code-review') return false;
const tokens = new Set((inv.args ?? '').split(/\s+/).filter(Boolean));
return tokens.has('--fix');
});
assert.ok(fixInvocation,
`autonomous.md must invoke Skill(skill="gsd-code-review", args="... --fix ...") for auto-fix; found: ${JSON.stringify(invocations)}`);
});
test('autonomous.md contains --auto flag on consolidated --fix invocation (#2790)', () => {
const content = fs.readFileSync(path.join(WORKFLOWS_DIR, 'autonomous.md'), 'utf-8');
// Find the gsd-code-review invocation that carries --fix (the consolidated
// auto-fix entry point), then assert --auto is one of its arg tokens.
// Tokenize via whitespace-split to avoid substring matches that could
// conflate --auto with --auto-foo.
const invocations = parseWorkflowSkillInvocations(content);
const fixInvocation = invocations.find(inv => {
if (inv.skill !== 'gsd-code-review') return false;
const tokens = new Set((inv.args ?? '').split(/\s+/).filter(Boolean));
return tokens.has('--fix');
});
assert.ok(fixInvocation, 'autonomous.md missing Skill(skill="gsd-code-review", args="... --fix ...") invocation');
const argTokens = new Set((fixInvocation.args ?? '').split(/\s+/).filter(Boolean));
assert.ok(argTokens.has('--auto'),
`autonomous.md gsd-code-review-fix args missing --auto flag; got args="${fixInvocation.args}"`);
});
});
// ────────────────────────────────────────────────────────────────────────
// Folded from tests/bug-2839-review-fix-transactional-cleanup.test.cjs — consolidation epic #1969 (B8 #1977)
// ────────────────────────────────────────────────────────────────────────
{
const { describe: __foldDescribe } = require('node:test');
__foldDescribe("folded:bug-2839-review-fix-transactional-cleanup (consolidation epic #1969 B8 #1977)", () => {
/**
* Regression test for bug #2839
*
* /gsd-code-review-fix cleanup tail is non-transactional. If the agent is
* interrupted (system restart, OOM kill) AFTER the last fix commit but
* BEFORE `git worktree remove`, the worktree is orphaned in
* `git worktree list`, the agent's branch is left with unmerged commits,
* and STATE.md is never advanced. To anyone reading main only, the phase
* looks "ready to plan" while critical fixes sit on a dangling branch.
*
* Fix: introduce a recovery sentinel JSON at
* ${PHASE_DIR}/.review-fix-recovery-pending.json
* The sentinel is written AFTER `git worktree add` succeeds and
* REMOVED only after `git worktree remove` completes, so the cleanup
* tail is transactional from the orchestrator's perspective. If the
* process dies in between, the sentinel is left behind pointing at the
* orphan worktree and branch — a future run, /gsd-resume-work, or
* /gsd-progress can detect and complete the recovery.
*/
'use strict';
// allow-test-rule: source-text-is-the-product (see #2839)
// The gsd-code-fixer agent's working instructions ARE the product — Claude
// follows them at runtime. Structural assertions over the markdown source
// test the deployed contract. See bug-2686 for the same pattern.
const { describe, test, before } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const { parseFrontmatter } = require('./helpers.cjs');
const SENTINEL_NAME = '.review-fix-recovery-pending.json';
function extractStep(content, stepName) {
const re = new RegExp(`<step\\s+name="${stepName}">([\\s\\S]*?)</step>`);
const m = content.match(re);
return m ? m[1] : null;
}
describe('bug-2839: /gsd-code-review-fix cleanup is transactional', () => {
let agentPath;
let agentContent;
let frontmatter;
before(() => {
agentPath = path.join(__dirname, '..', 'agents', 'gsd-code-fixer.md');
assert.ok(fs.existsSync(agentPath), 'agents/gsd-code-fixer.md must exist');
agentContent = fs.readFileSync(agentPath, 'utf-8');
frontmatter = parseFrontmatter(agentContent);
assert.ok(frontmatter, 'agent must have YAML frontmatter');
});
test('agent declares a recovery sentinel filename', () => {
assert.ok(
agentContent.includes(SENTINEL_NAME),
`gsd-code-fixer.md must reference the recovery sentinel ${SENTINEL_NAME} so an interrupted cleanup tail is discoverable (#2839)`
);
});
test('sentinel is written inside setup_worktree, after git worktree add', () => {
const setupStep = extractStep(agentContent, 'setup_worktree');
assert.ok(setupStep, 'setup_worktree step must exist');
assert.ok(
setupStep.includes(SENTINEL_NAME),
`setup_worktree must reference ${SENTINEL_NAME} so the sentinel is created at the start of the run (#2839)`
);
const addPos = setupStep.indexOf('git worktree add');
assert.ok(addPos !== -1, 'setup_worktree must contain `git worktree add`');
// The sentinel WRITE (not just a reference) must come after `git worktree add`.
// Earlier references are allowed (e.g. recovery check for a stale sentinel
// from a prior interrupted run). Look for an explicit write — either a
// shell `>`/`>>` redirection, a `node -e` invocation that uses
// `fs.writeFileSync(...sentinel...)`, or a `Write` tool reference.
const writeIdx = (() => {
const candidates = [
/fs\.writeFileSync\([^)]*sentinel/,
/>\s*"?\$sentinel/,
/>\s*"?\$\{sentinel\}/,
/Write the recovery sentinel/i,
];
let earliest = -1;
for (const re of candidates) {
const m = re.exec(setupStep);
if (m && (earliest === -1 || m.index < earliest)) earliest = m.index;
}
return earliest;
})();
assert.ok(
writeIdx !== -1,
'setup_worktree must explicitly describe writing the sentinel (#2839)'
);
assert.ok(
addPos < writeIdx,
'sentinel must be written AFTER `git worktree add` succeeds (#2839)'
);
});
test('sentinel records worktree path, branch, and padded_phase as JSON fields', () => {
for (const key of ['worktree_path', 'branch', 'padded_phase']) {
assert.ok(
agentContent.includes(key),
`recovery sentinel must record \`${key}\` so a future /gsd-resume-work or /gsd-progress can locate the orphan state (#2839)`
);
}
});
test('sentinel removal happens only AFTER git worktree remove succeeds', () => {
const setupStep = extractStep(agentContent, 'setup_worktree');
assert.ok(setupStep, 'setup_worktree step must exist');
const cleanupAnchor = setupStep.lastIndexOf('Cleanup tail (transactional');
assert.ok(cleanupAnchor !== -1, 'setup_worktree must document cleanup-tail section');
const cleanupSection = setupStep.slice(cleanupAnchor);
const removeIdx = cleanupSection.indexOf('git worktree remove "$wt" --force');
assert.ok(removeIdx !== -1, 'cleanup-tail must remove worktree');
// Within the cleanup-tail section, accept either a literal-filename form
// (`rm -f .../.review-fix-recovery-pending.json`) or a shell-variable form
// referring to the previously-declared `sentinel` variable
// (`rm -f "$sentinel"` / `rm -f "${sentinel}"`).
const escapedName = escapeRegex(SENTINEL_NAME);
const sentinelRemovalRe = new RegExp(
`(rm\\s+(?:-f\\s+)?[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?)|unlink[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?))`
);
const sentinelRemovalMatch = sentinelRemovalRe.exec(cleanupSection);
assert.ok(
sentinelRemovalMatch,
`agent must remove the sentinel file (rm or unlink ${SENTINEL_NAME}) as part of the cleanup tail (#2839)`
);
const sentinelRemovalIdx = sentinelRemovalMatch.index;
assert.ok(
removeIdx < sentinelRemovalIdx,
'cleanup ordering must be: `git worktree remove` BEFORE sentinel removal (#2839)'
);
});
test('agent documents detection of pre-existing sentinel from a prior interrupted run', () => {
const lower = agentContent.toLowerCase();
const mentionsRecovery =
lower.includes('stale sentinel') ||
lower.includes('existing sentinel') ||
lower.includes('previous sentinel') ||
lower.includes('prior run') ||
lower.includes('pre-existing sentinel') ||
lower.includes('recovery');
assert.ok(
mentionsRecovery,
'agent must describe how it handles a pre-existing sentinel from a previous interrupted run (#2839)'
);
});
test('cleanup-tail obligation is documented as transactional / atomic', () => {
const lower = agentContent.toLowerCase();
const mentionsTransactional =
lower.includes('transactional') ||
lower.includes('atomic cleanup') ||
lower.includes('cleanup tail');
assert.ok(
mentionsTransactional,
'agent must document the cleanup tail as transactional/atomic (#2839)'
);
});
});
});
}