* fix(#4259): fold shell continuations before the T6 docs-parity site scan The scan is $-anchored with [^\n]* on both sides of --grep=, so `git log` and `--grep=` had to share a physical line. A backslash-continued derivation — the natural way to write a git log carrying a long ERE — produced zero hits and T6 passed on it. Both generations of the assertion were defeated. The current anti-revert ban let a wrapped site through outright; at v1.12.0, where T6 instead asserted pattern conformance, a wrapped site was silently exempted from the very checks written to catch the macOS \b-no-op class, so it could have carried exactly the malformed pattern T6 exists to reject. A real candidate implementation for #3926 wrapped its derivation, passed T6, and was caught only by later manual review. Fold the continuations before matching rather than widening the regex: the assertion's message and its PHASE_SCOPE_NUM filter both assume one site is one string, and a [\s\S]*? would run the scan across unrelated statements. Correcting the input repairs everything built on the scan at once. The fold uses [ \t]* after the newline rather than \s* — the shell's own rule, and it cannot swallow a blank line and glue two unrelated statements. The scan is hoisted to findGrepSites so the controls can drive it directly: the continued form is caught, the same-line form still is, a benign --grep stays clean, a wrapped unrelated assignment is not glued, a continuation does not cross a blank line, and the live workflow files still report nothing — so this lands without editing any workflow to appease it. * fix(#4259): honor shell continuation boundaries * test(#4259): use canonical fenced-block scanner * test(#4259): centralize shell continuation scanning --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Michel Moreira
@@ -26,10 +26,15 @@ const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const fc = require('fast-check');
|
||||
const { runHook } = require('./helpers/process-seam.cjs');
|
||||
const { toLegacyResult, gitOrThrow } = require('./helpers/git-fixture.cjs');
|
||||
const { PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, HOOK_FANOUT_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
const { createTempDir, createTempGitProject, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
const {
|
||||
foldShellContinuations,
|
||||
findShellFencedMatches,
|
||||
} = require('./helpers/shell-doc-scan.cjs');
|
||||
|
||||
const ROOT = path.resolve(__dirname, '..');
|
||||
const WORKFLOW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review.md');
|
||||
@@ -37,6 +42,32 @@ const PRE_PASS_STEP_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review
|
||||
const FIXER_PATH = path.join(ROOT, 'agents', 'gsd-code-fixer.md');
|
||||
const REVIEWER_PATH = path.join(ROOT, 'agents', 'gsd-code-reviewer.md');
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// #4259: the T6 docs-parity site scan, hoisted out of the assertion so it can
|
||||
// be driven directly by the negative controls below.
|
||||
//
|
||||
// The scan is `$`-anchored with `[^\n]*` on both sides of `--grep=`, so it only
|
||||
// ever matched when `git log` and `--grep=` sat on the SAME physical line. A
|
||||
// shell line-continuation made a semantically identical derivation invisible:
|
||||
// zero hits, and T6 passed. Both generations of the assertion were defeated by
|
||||
// it — the current anti-revert ban let a wrapped site through outright, and at
|
||||
// v1.12.0 a wrapped site was silently exempted from the pattern-conformance
|
||||
// checks written to catch the macOS `\b`-no-op class, so it could have carried
|
||||
// exactly the malformed pattern T6 exists to reject. That is not a hypothetical
|
||||
// shape: wrapping is the natural way to write a `git log` carrying a long ERE,
|
||||
// and a real candidate implementation for #3926 did it, passed T6, and was
|
||||
// caught only by later manual review.
|
||||
//
|
||||
// Folding continuations BEFORE matching is the repair, rather than widening the
|
||||
// regex in place: the assertion's message and its `PHASE_SCOPE_NUM` filter both
|
||||
// assume one site is one string, and a `[\s\S]*?` would happily run the scan
|
||||
// across unrelated statements. The fold corrects the input, so everything built
|
||||
// on the scan is fixed at once.
|
||||
//
|
||||
const GREP_SITE_RE = /^\s*[A-Z_]+=\$\(git log[^\n]*--grep=[^\n]*$/gm;
|
||||
|
||||
const findGrepSites = (src) => findShellFencedMatches(src, GREP_SITE_RE);
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Pure-function implementation of the compute_file_scope Node script body.
|
||||
// This mirrors the logic in code-review.md lines 172-184 exactly.
|
||||
@@ -1076,6 +1107,135 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre
|
||||
// both files must use the SAME phase-directory anchor — and no message-grep
|
||||
// derivation may return (a subject carries no milestone bound; that class
|
||||
// failed five times: #2989/#3191/#3503/#3995).
|
||||
// #4259: the T6 scan drives itself off the live workflow files, which are
|
||||
// clean — so its matching branch is exercised only by whatever those files
|
||||
// happen to contain, and the hole it had was invisible for exactly that
|
||||
// reason. These fixtures drive findGrepSites directly, in both directions.
|
||||
test('#4259 T6 site scan sees a backslash-continued derivation, and still ignores what it should', () => {
|
||||
const sameLine = [
|
||||
'```bash',
|
||||
'PHASE_START=$(git log --extended-regexp --grep="^(feat|fix)\\(phase-${PHASE_SCOPE_NUM}" --format="%H")',
|
||||
'```',
|
||||
].join('\n');
|
||||
|
||||
// Semantically identical to the row above. The only difference is two
|
||||
// continued physical lines, and that used to be enough to vanish.
|
||||
const continued = [
|
||||
'```bash',
|
||||
'PHASE_START=$(git log \\',
|
||||
' --extended-regexp \\',
|
||||
' --grep="^(feat|fix)\\(phase-${PHASE_SCOPE_NUM}" --format="%H")',
|
||||
'```',
|
||||
].join('\n');
|
||||
|
||||
assert.equal(findGrepSites(sameLine).length, 1, 'the same-line form must stay caught');
|
||||
assert.equal(findGrepSites(continued).length, 1, 'the continued form must now be caught (#4259)');
|
||||
|
||||
// The filter T6 actually asserts on has to see the marker too. Before the
|
||||
// fold this failed twice over: the scan returned nothing, AND
|
||||
// PHASE_SCOPE_NUM sat on a different physical line from the one the scan
|
||||
// would have captured, so even a matching scan would have filtered it out.
|
||||
for (const src of [sameLine, continued]) {
|
||||
assert.equal(
|
||||
findGrepSites(src).filter((l) => l.includes('PHASE_SCOPE_NUM')).length,
|
||||
1,
|
||||
'the captured site must carry the marker T6 filters on',
|
||||
);
|
||||
}
|
||||
|
||||
// Negative control that DOES exercise the fold: a continued, non-phase
|
||||
// grep site remains a site, but must not become a phase-scope finding.
|
||||
const benign = [
|
||||
'```bash',
|
||||
'RELEASE_NOTES=$(git log \\',
|
||||
' --grep="^chore" --format="%s")',
|
||||
'```',
|
||||
].join('\n');
|
||||
assert.equal(
|
||||
findGrepSites(benign).filter((l) => l.includes('PHASE_SCOPE_NUM') || /phase-\)?\(/.test(l)).length,
|
||||
0,
|
||||
'a non-phase-scope --grep must stay clean',
|
||||
);
|
||||
|
||||
// A wrapped git-log assignment that merely sits near a --grep string must
|
||||
// not be glued into one logical line with it. This exercises the fold and
|
||||
// still contains every keyword the site regex looks for.
|
||||
const unrelated = [
|
||||
'```bash',
|
||||
'SOME_VAR=$(git log \\',
|
||||
' --format="%H")',
|
||||
'echo "--grep=$SOME_VAR"',
|
||||
'```',
|
||||
].join('\n');
|
||||
assert.deepStrictEqual(findGrepSites(unrelated), [], 'a wrapped unrelated assignment must not glue into a hit');
|
||||
|
||||
// A continuation must not reach across a blank line — the reason this
|
||||
// folds [ \t]* rather than \s* after the newline.
|
||||
const acrossBlank = [
|
||||
'```bash',
|
||||
'SOME_VAR=$(git log \\',
|
||||
'',
|
||||
'FOO=--grep=x',
|
||||
'```',
|
||||
].join('\n');
|
||||
assert.deepStrictEqual(findGrepSites(acrossBlank), [], 'the fold must stop at a blank line');
|
||||
|
||||
// Two trailing backslashes represent one literal backslash followed by a
|
||||
// real newline. Folding this would invent a site the shell does not have.
|
||||
const evenBackslashes = [
|
||||
'```bash',
|
||||
'PHASE_START=$(git log \\\\',
|
||||
' --grep="phase-${PHASE_SCOPE_NUM}")',
|
||||
'```',
|
||||
].join('\n');
|
||||
assert.deepStrictEqual(
|
||||
findGrepSites(evenBackslashes),
|
||||
[],
|
||||
'an even trailing-backslash run is not a shell continuation',
|
||||
);
|
||||
|
||||
// Three trailing backslashes retain one literal pair while the final
|
||||
// backslash continues the command. This pins the non-trivial odd boundary.
|
||||
const oddBackslashes = [
|
||||
'```bash',
|
||||
`PHASE_START=$(git log ${'\\'.repeat(3)}`,
|
||||
' --grep="phase-${PHASE_SCOPE_NUM}")',
|
||||
'```',
|
||||
].join('\n');
|
||||
assert.deepStrictEqual(
|
||||
findGrepSites(oddBackslashes),
|
||||
[`PHASE_START=$(git log ${'\\'.repeat(2)} --grep="phase-\${PHASE_SCOPE_NUM}")`],
|
||||
'an odd trailing-backslash run keeps its literal pairs and continues the line',
|
||||
);
|
||||
});
|
||||
|
||||
test('#4259 continuation folding preserves every odd/even backslash-run boundary', () => {
|
||||
fc.assert(fc.property(
|
||||
fc.integer({ min: 0, max: 31 }),
|
||||
fc.array(fc.constantFrom(' ', '\t'), { maxLength: 8 }).map((chars) => chars.join('')),
|
||||
(runLength, indentation) => {
|
||||
const slashes = '\\'.repeat(runLength);
|
||||
const source = `cmd ${slashes}\n${indentation}tail`;
|
||||
const expected = runLength % 2 === 1
|
||||
? `cmd ${'\\'.repeat(runLength - 1)} tail`
|
||||
: source;
|
||||
assert.strictEqual(foldShellContinuations(source), expected);
|
||||
},
|
||||
), { numRuns: 200 });
|
||||
});
|
||||
|
||||
test('#4259 T6 site scan reports nothing on the live workflow files', () => {
|
||||
// The adoption check: only shell code fences are scanned, so markdown hard
|
||||
// breaks and examples in other languages cannot be folded into fake shell
|
||||
// sites. Distinct from T6 itself, this asserts the live scan is quiet.
|
||||
for (const src of [
|
||||
readFileNormalized(WORKFLOW_PATH),
|
||||
readFileNormalized(PRE_PASS_STEP_PATH).replace(/\\"/g, '"'),
|
||||
]) {
|
||||
assert.deepStrictEqual(findGrepSites(src), []);
|
||||
}
|
||||
});
|
||||
|
||||
test('T6 docs-parity: all diff-base derivations use the identical phase-directory anchor; no --grep site remains', () => {
|
||||
const sources = [
|
||||
readFileNormalized(WORKFLOW_PATH),
|
||||
@@ -1089,9 +1249,9 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre
|
||||
}
|
||||
const grepSites = [];
|
||||
for (const src of sources) {
|
||||
for (const m of src.matchAll(/^\s*[A-Z_]+=\$\(git log[^\n]*--grep=[^\n]*$/gm)) {
|
||||
grepSites.push(m[0]);
|
||||
}
|
||||
// #4259: findGrepSites folds backslash continuations first, so a wrapped
|
||||
// assignment presents as one logical line and cannot slip the scan.
|
||||
grepSites.push(...findGrepSites(src));
|
||||
}
|
||||
assert.deepStrictEqual(
|
||||
grepSites.filter((l) => l.includes('PHASE_SCOPE_NUM') || /phase-\)?\(/.test(l)),
|
||||
|
||||
36
tests/helpers/shell-doc-scan.cjs
Normal file
36
tests/helpers/shell-doc-scan.cjs
Normal file
@@ -0,0 +1,36 @@
|
||||
'use strict';
|
||||
|
||||
const { scanFencedBlocks } = require('../../gsd-core/bin/lib/markdown-sectionizer.cjs');
|
||||
const { splitLines } = require('../../gsd-core/bin/lib/text-lines.cjs');
|
||||
|
||||
/**
|
||||
* Fold unquoted shell backslash-newline continuations into logical lines.
|
||||
*
|
||||
* A newline is continued only when the immediately preceding backslash run is
|
||||
* odd: the final backslash is consumed and every earlier pair stays literal.
|
||||
* This deliberately remains a text heuristic, not a shell parser; in
|
||||
* particular, a backslash-newline inside single quotes is a known blind spot.
|
||||
*/
|
||||
function foldShellContinuations(src) {
|
||||
return src.replace(
|
||||
/(^|[^\\])((?:\\\\)*)\\\n[ \t]*/gm,
|
||||
(_match, prefix, literalPairs) => `${prefix}${literalPairs} `,
|
||||
);
|
||||
}
|
||||
|
||||
/** Return regex matches from logical lines inside closed bash/sh fences. */
|
||||
function findShellFencedMatches(src, pattern) {
|
||||
const lines = splitLines(src);
|
||||
return scanFencedBlocks(lines).flatMap((block) => {
|
||||
if (block.closeLineIdx === -1) return [];
|
||||
const language = block.infoString.trim().toLowerCase();
|
||||
if (language !== 'bash' && language !== 'sh') return [];
|
||||
const body = lines.slice(block.openLineIdx + 1, block.closeLineIdx).join('\n');
|
||||
return Array.from(
|
||||
foldShellContinuations(body).matchAll(pattern),
|
||||
(match) => match[0],
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
module.exports = { foldShellContinuations, findShellFencedMatches };
|
||||
Reference in New Issue
Block a user