Files
msd-core/tests/code-review.test.cjs
Tom Boucher dc3c81e93d chore(#3212): src/pattern.cts is the sole owner of runtime-value regex construction — Phase 1 (#3416)
* test(#3412): failing-first suite for the pattern-construction seam

Phase 1 of epic #3212 (ADR-3212 §1/§2/§7). Tests only — src/pattern.cts
and eslint-rules/no-adhoc-regex-escape.cjs do not exist yet, so both
suites fail with MODULE_NOT_FOUND, which is the intended RED.

Locks the measured behavior rather than the assumed behavior:
RegExp.escape hex-escapes the leading character of nearly every string
("abc" -> "\x61bc"), so the suite asserts match-equivalence against an
inlined historical oracle (the implementation being deleted) rather
than byte-equivalence of pattern text — 200 seeded fast-check runs plus
a fixed corpus, 0 mismatches. Also locks the latent character-class
range bug this phase fixes as a side effect: a hyphen-bearing value
interpolated into [...] currently forms a real range and matches an
unintended character; post-migration it must not.

* chore(#3412): src/pattern.cts owns runtime-value regex construction

Phase 1 of epic #3212 (ADR-3212 §1/§2/§6/§7). Adds the pattern seam
delegating to the built-in RegExp.escape, deletes every hand-rolled
copy, and raises the Node floor to the Active LTS line.

The census was low, three times over. ADR-3212 counted 10 copies; a
graph query found 12; the new lint rule — once live — found 27 more.
The difference is that the census counted named helper FUNCTIONS while
the rule counts the escape SHAPE, so inline .replace(<class>, '\$&')
copies were never in scope. ADR §1's actual requirement is that no
module outside the seam escapes a value for regex use, so all of them
are, and CLAUDE.md's no-defer rule makes them this change's work.
Fourth consecutive epic here whose copy count was low — the argument
for ADR-3180 Amendment 3's "state N found by the guard" rule.

Also corrected mid-implementation: the survey reported phase-id.cts's
escapeRegex had 0 external importers. It had 8 production importers,
making its removal a public-surface change to an ADR-2121-owned module
and requiring an update to that ADR's locked-surface test. Blast
radius revised Medium-High -> High.

RegExp.escape is match-equivalent but NOT text-equivalent: it
hex-escapes the leading char of nearly every string ("abc" ->
"\x61bc"). Equivalence is proven by a seeded fast-check property test
against the deleted implementation as oracle. It also fixes a latent
bug: a hyphen-bearing value interpolated into a character class
previously formed a real range and matched an unintended character.

Node floor 22 -> 24 (RegExp.escape is Node 24+), across engines,
.nvmrc, package-lock, 9 CI matrix entries, and 5 docs. The aggregate
`required-tests` context is unchanged and no job was added or removed,
so branch protection cannot be orphaned by the dropped lanes.

Enforced by eslint-rules/no-adhoc-regex-escape.cjs (shape-matched, with
structural provenance for reviewed pattern-fragment constants rather
than a name heuristic) plus a whole-tree companion guard covering the
directories ESLint's globs miss.

* fix(#3412): close the _SOURCE guard evasion, correct two false claims

Three findings from the orthogonal review pass, all fixed.

1. The ESLint rule's `_SOURCE` provenance fallback was pure identifier-
   name matching with no binding check, so `new RegExp(userInput_SOURCE)`
   — a function parameter — sailed past the guard. That is the same
   rename-evasion class issue #3410 documents, reopened by the very
   fallback meant to complement the structural check. Now bound to the
   identifier's actual binding kind: import, require-derived const, or
   module-scope const; parameters, `let`/`var`, and unresolvable
   bindings fail closed. Four RuleTester cases cover the evasion and
   prove the legitimate cross-module case still passes.

2. src/pattern.cts's own header carried the stale pre-correction counts
   (12 copies / 17 call sites) while CONTEXT.md and the design doc
   carried the corrected ones (~39 / ~44) — a self-contradiction inside
   the PR whose entire purpose is deleting divergent copies. Rewritten,
   preserving the durable lesson: a named-function census cannot see
   inline copies; only a shape-matching guard can.

3. The claim that all deleted copies threw TypeError on non-string was
   false. phase-id.cts's copy — the one with 8 external importers — did
   String(value).replace(...) and never threw. The seam's locked
   signature does not coerce, so this is a real, now-disclosed behavior
   change rather than the pure preservation the tests asserted. Audited
   all 32 invocations across the 8 importers and 6 in-file callers:
   every one is safe by construction (upstream truthy guard or a
   string-producing derivation), verified by runtime probe against the
   compiled modules rather than by TS compilation, which cannot see a
   runtime undefined. Corrected the false claim in both the test comment
   and the design doc, and added it to Known limits.

* docs(#3412): add Changed changeset for the Node 24 floor

The only user-visible break in this phase. The escape-behavior change
is internal and match-equivalent, so it carries no user-facing note.

* fix(#3412): resolve the seam's require graph in script fixtures and packaging

Checkpoint 2 came back red with 90 failures on the node24 lane. Three
distinct defects, all introduced by routing scripts/ through the new
pattern seam, none reproducible by any local gate:

1. ~82 failures — tests/adr-index-gate.test.cjs and
   tests/removed-but-needed-lint.test.cjs copy a scripts/*.cjs into an
   mkdtemp fixture and spawn it there (necessary: those scripts resolve
   their scan root from __dirname/.., so running the real script would
   scan the real repo). Each harness hand-listed the dependencies to
   copy alongside. Adding require('../gsd-core/bin/lib/pattern.cjs') to
   gen-adr-index.cjs made both lists silently incomplete ->
   MODULE_NOT_FOUND, plus 17 downstream 'did not emit parseable JSON'
   failures from the same crash.

   Fixed as a class, not an instance: new tests/helpers/copy-script-
   fixture.cjs walks a script's transitive static relative-require graph
   and copies it, so dependencies are derived and never re-declared. It
   throws (naming the unbuilt artifact) instead of letting the child die
   with a bare MODULE_NOT_FOUND. Verified for all four seam-consuming
   scripts: gen-adr-index, lint-removed-but-needed, gen-loop-host-
   contract, sync-runtime-launcher.

2. 2 failures — scripts/ ships wholesale but eslint-rules/ does not, so
   the new scripts/lint-no-adhoc-regex-escape.cjs would be
   MODULE_NOT_FOUND in a published install (#2858 guard). Excluded from
   the tarball, matching the existing precedent for gen-emitted-
   baseline.cjs, which is excluded for the identical reason, and locked
   with a test modeled on that one. Confirmed against a real npm pack:
   890 files, 0 from eslint-rules/, and gsd-core/bin/lib/pattern.cjs
   present (so the other four scripts' requires are legitimate).

3. 6 failures — tests/phase-id.test.cjs asserted the literal escaped
   source text ('0*29', 'PROJ-42'). RegExp.escape is match-equivalent to
   the retired hand-rolled escaper but NOT text-equivalent: it hex-
   escapes the leading character and all hyphens ('0*\x329',
   '\x50ROJ\x2d42'). Verified NOT a behavior change — 576 match
   decisions across all three real interpolation prefixes, zero
   divergence. Those tests now compile each source into the same heading
   regex src/roadmap.cts's searchPhaseInContent builds and assert what
   matches and what does not, including the 'i'-flag canonicalization
   the hex escape has to preserve. Re-pinning the new literals would
   have rebuilt the same brittleness one layer down. Adds a test for the
   property the escape exists for: a dot in '1.2' must not act as a
   wildcard.

Also shares one definition of 'a require' between the packaging guard
and the fixture copier, so the two cannot disagree about what they scan.

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

* fix(#3412): refuse to copy a fixture dependency outside the fixture root

copyScriptWithDeps resolved each relative require and joined the
repo-relative result onto fixtureRoot. A require resolving OUTSIDE the
repo yields a '../'-prefixed relative path, so path.join climbed out of
the fixture and wrote into the surrounding temp dir (verified:
repoRoot=/repo + depAbs=/etc/passwd wrote /tmp/etc/passwd).

No script in the tree does this today, so this closes an available
escape rather than an active one. Refuses via the existing unresolved-
require path so the failure names the offending specifier. Covered by a
negative proof that the guard fires and that nothing lands outside the
fixture.

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

* fix(#3412): parse requires instead of pattern-matching them; restore the foreign-prefix contract

Applies all findings from the second orthogonal review round, re-run
because real code changed after round 1.

HIGH (security) — extractRequires stripped BLOCK comments before LINE
comments, so a '//' comment containing '/*' opened a phantom block
comment, and a '//' inside a string literal truncated the line. Both
hid real requires: 'const u="http://x"; require("./real.cjs")'
returned [], and four real requires in gsd-core/bin/gsd-tools.cjs were
invisible. Replaced with a real AST parse via espree.

This is ADR-3212's own Decision 4 — tokenizer-first for stateful
grammars — applied to the case it describes; comment/string/regex
nesting is exactly such a grammar, which is why the regex version was
wrong. The function was moved byte-identical out of the #2858 packaging
guard, so the bug PRE-DATES this branch and has been a live blind spot
there: a shipped script could have required an unshipped path
undetected. Fixing it makes that guard strictly stronger than on next.

espree is promoted from a transitive eslint dependency to an explicit
devDependency rather than relying on hoisting. The script parse attempt
sets ecmaFeatures.globalReturn because Node wraps CommonJS bodies in a
function, making a top-level return legal — scripts/check-coverage-gate
.cjs relies on it, and without the flag the guard throws on a file it
is supposed to scan. Verified 0 unparseable across all 324 .cjs/.js
under scripts/, bin/, and gsd-core/bin/, and 0 new violations against a
real npm pack, so the exact extractor does not newly fail the guard.

MEDIUM (security) — the repo-containment check guarded dependencies but
not the entry path. One escapesContainment predicate now guards both.

LOW (security) — containment was lexical while fs follows symlinks, and
a directory symlink could mint a fresh dedupe key per level. realpath
now resolves both repoRoot and each dependency before the decision, and
the realpath-derived path is the dedupe key. Destination layout still
uses the original repo-relative path, so copied trees are unchanged.

MAJOR (standards) — the round-1 behavioral rewrite of phase-id tests
lost the foreign-prefix contract: every assertion was satisfied by an
impl returning [A-Z]+\x2d42, i.e. ANY project code — the exact #3599
bug class the exact-source prevents. The literal assertions it replaced
were catching this. Now asserts the compiled regex REJECTS a different
prefix with the same number.

MAJOR (standards) — the test hand-duplicated production's heading regex
with no parity guard (CLAUDE.md's 'Generative Fix Divergence'). Removed
the parallel surface instead of policing it: src/roadmap.cts exports
buildPhaseHeadingRegex, searchPhaseInContent calls it, the test imports
it. Byte-identical .source and .flags verified for both escaped forms.

MINOR — '..foo' no longer false-flagged as an escape; the inverted
spurious-vs-missing doc claim corrected; the dead allow-test-rule
header removed.

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

* chore(#3412): backfill changeset pr number to 3416

* fix(#3412): make the escape guard's own regex linear, reword an injection-scan collision

Two CI failures on PR #3416, both in code this branch added.

CodeQL js/redos (high) — REPLACE_CALL_RE's outer alternation let a
bracket run be consumed EITHER by the character-class branch OR one
character at a time by the trailing catch-all, so a failing match
explored both parses of every pair. Measured on the real regex:
n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, a clean 2^n. This script
scans repo source, so a file with a long bracket run after '.replace(/'
would hang CI outright — a guard against undisciplined pattern
construction was itself the worst pattern in the diff.

Fixed the way ADR-3212 already prescribes: the catch-all branch now
excludes '[' and ']' so a bracket can only be consumed by the class
branch (this is what makes it linear), and every quantifier is bounded
(the locked bounded-quantifiers decision) as a second line of defense.
Now 0ms at n=2000. Disclosed coverage tradeoff, recorded at the
constant: a regex literal with a BARE unescaped ']' outside a class is
no longer matched by this backstop. No census shape has that form, and
the AST rule remains the primary detector.

Verified the guard did not go blind doing it: a real census-shape
violation is still reported, and an allow-adhoc-regex-escape
suppression comment is still honored.

Regression test drives the exported findViolations on a
2000-repetition adversarial input and asserts the RESULT. It makes no
wall-clock assertion — elapsed-time tests are forbidden — so a
regression surfaces as a harness timeout, which is the correct signal.

Prompt injection scan — 'must not act as a regex wildcard' in a test
comment matched the scanner's jailbreak pattern act\s+as\s+(a|an|if|
my). Reworded to 'behave as'. Deliberately NOT allowlisted: silencing a
whole test file over one phrase would blunt the scanner permanently,
and the comment has nothing to do with injection.

Neither failure was reachable from the remote runner — CodeQL and the
injection scan are not in that matrix, so the sha it passed was green
and still wrong.

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-13 16:19:57 -04:00

797 lines
36 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 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');
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(
/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 = content.match(/```bash[\s\S]*?```/g) || [];
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 = content.match(/```bash[\s\S]*?```/g) || [];
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');
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
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)'
);
});
});
});
}