fix(#4830): re-land #4768's letter-suffix phase-id fix and its lint-phase-id-drift ratchets on current next (#4873)
* fix(#4830): re-land #4768's letter-suffix phase-id fix and its lint-phase-id-drift ratchets on current next Commit740ba0d8a(#4781) removed every change #4768 had merged for #4748: the first-non-digit split at execute-phase.md's two arithmetic sites, the init-emitted `padded_phase` the REVIEW.md lookup binds instead of `printf "%02d"`, the canonical-grammar extractions in autonomous.md and plan-review-convergence.md, the `.changeset/zesty-wolves-tumble.md` fragment, and the three lint-phase-id-drift ratchets with their tests. The guard and the code it guarded left together, so nothing went red. This is a cherry-pick of092d9256bonto current `next`, resolved against the #4683 threat-id fields on execute-phase.md's Parse-JSON line, with the changeset `pr:` reset to the placeholder and the compact-content benchmark baseline regenerated against the current base. (cherry picked from commit092d9256b8) Emitted-Drift-Ack-Growth: autonomous.md — restores #4768's canonical-grammar extraction and its explanatory comment for --from/--to/--only Emitted-Drift-Ack-Growth: execute-phase.md — restores #4768's first-non-digit split at two arithmetic sites and the padded_phase binding for the REVIEW.md lookup Emitted-Drift-Ack-Growth: plan-review-convergence.md — restores #4768's canonical-grammar phase extraction and its comment Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AX7LXxc3uAkGki6iaYiAMP * chore(#4830): set changeset fragment pr to 4873 --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
0xdhx
@@ -191,6 +191,7 @@ describe('#4568 — plan-phase.md captures the full N-segment --research-phase v
|
||||
// The canonical grammar is read from the committed bin/lib mirror the other
|
||||
// grammar tests use (shell cannot import it; the test can).
|
||||
const { PHASE_NUMBER_TOKEN_SOURCE } = require('../gsd-core/bin/lib/phase-id.cjs');
|
||||
const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs');
|
||||
const CANONICAL_ANCHORED = new RegExp('^(?:' + PHASE_NUMBER_TOKEN_SOURCE + ')$');
|
||||
|
||||
// Inputs the canonical grammar ACCEPTS. `03A` is what `normalizePhaseName('3A')`
|
||||
@@ -318,3 +319,290 @@ describe('#4660 — plan-phase.md captures the full letter-suffixed --research-p
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// #4748 — the letter axis at the seven shell sites OUTSIDE #4660's six. These
|
||||
// are not grammar mirrors but consumers of the id: the post-#4619
|
||||
// `PHASE_INT=${PHASE_NUMBER%%.*}; $((10#$PHASE_INT))` split (four sites),
|
||||
// the `printf "%02d"` re-pad before the REVIEW.md lookup (one site), the
|
||||
// `[0-9]+\.?[0-9]*` argument extraction (two files, four lines) and the
|
||||
// legacy manual normalizer. On a letter-suffixed id the first aborts bash,
|
||||
// the second prints the wrong file, the last two silently truncate. Same
|
||||
// discipline as above: each site's live lines are read off disk by anchor
|
||||
// and executed in a real bash subprocess.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const EXECUTE_PHASE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md');
|
||||
const COMPLETION_RECONCILIATION = path.join(
|
||||
__dirname, '..', 'gsd-core', 'workflows', 'execute-phase', 'steps', 'completion-reconciliation.md',
|
||||
);
|
||||
const TDD_REF = path.join(__dirname, '..', 'gsd-core', 'references', 'tdd.md');
|
||||
const AUTONOMOUS = path.join(__dirname, '..', 'gsd-core', 'workflows', 'autonomous.md');
|
||||
const PLAN_REVIEW_CONVERGENCE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md');
|
||||
const PHASE_ARGUMENT_PARSING = path.join(__dirname, '..', 'gsd-core', 'references', 'phase-argument-parsing.md');
|
||||
|
||||
/**
|
||||
* Pure: the indexes of every line containing `anchor`. Asserts the count so a
|
||||
* site that is added, removed or renamed breaks this test loudly instead of
|
||||
* silently narrowing what it covers (execute-phase.md carries the split TWICE
|
||||
* — plan selection and the TDD gate — and both must stay under test).
|
||||
*/
|
||||
function findAnchoredLineIndexes(lines, anchor, expectedCount) {
|
||||
const idx = [];
|
||||
lines.forEach((l, i) => {
|
||||
if (l.includes(anchor)) idx.push(i);
|
||||
});
|
||||
assert.equal(
|
||||
idx.length,
|
||||
expectedCount,
|
||||
`expected ${expectedCount} line(s) containing ${JSON.stringify(anchor)}, found ${idx.length}`,
|
||||
);
|
||||
return idx;
|
||||
}
|
||||
|
||||
/** Run `script` in bash with `env` merged in; never throws — returns { status, stdout, stderr }. */
|
||||
function runBash(script, env) {
|
||||
try {
|
||||
const stdout = execFileSync('bash', [], {
|
||||
input: script,
|
||||
encoding: 'utf8',
|
||||
timeout: TIMEOUT,
|
||||
env: { ...process.env, ...env },
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
return { status: 0, stdout: stdout.trim(), stderr: '' };
|
||||
} catch (e) {
|
||||
return { status: e.status, stdout: String(e.stdout || '').trim(), stderr: String(e.stderr || '').trim() };
|
||||
}
|
||||
}
|
||||
|
||||
// What each Class 1 site must compute from the id it is handed: the integer
|
||||
// half zero-stripped for the anchored `0*` commit-scope ERE, everything after
|
||||
// it carried through with dots escaped. `03A` is the padded form `init` emits
|
||||
// for a `03A-slug/` directory; `12A` / `3A` are the bare forms; `03A.1.2` is
|
||||
// the letter-and-N-segment combination the canonical grammar admits.
|
||||
const CLASS1_CASES = [
|
||||
// [PHASE_NUMBER, expected PHASE_N]
|
||||
['03A', '3A'],
|
||||
['12A', '12A'],
|
||||
['3A', '3A'],
|
||||
['03A.1.2', '3A\\.1\\.2'],
|
||||
];
|
||||
const CLASS1_CONTROLS = [
|
||||
['06', '6'],
|
||||
['7', '7'],
|
||||
['08.5', '8\\.5'],
|
||||
['23.1.2', '23\\.1\\.2'],
|
||||
];
|
||||
|
||||
describe('#4748 — the $((10#$PHASE_INT)) split sites carry a letter suffix into PHASE_N instead of aborting', () => {
|
||||
const sites = [
|
||||
// execute-phase.md: plan selection (safe_resume_gate) and the TDD gate a
|
||||
// few lines below are the same two lines twice; both must be under test.
|
||||
{ name: 'execute-phase.md', file: EXECUTE_PHASE, anchor: 'PHASE_INT=${PHASE_NUMBER%%', count: 2, input: 'PHASE_NUMBER', output: 'PHASE_N' },
|
||||
{ name: 'completion-reconciliation.md', file: COMPLETION_RECONCILIATION, anchor: 'SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%', count: 1, input: 'SPOT_PHASE_NUMBER', output: 'SPOT_PHASE_N' },
|
||||
{ name: 'tdd.md', file: TDD_REF, anchor: 'PHASE_INT=${PHASE%%', count: 1, input: 'PHASE', output: 'PHASE_N' },
|
||||
];
|
||||
|
||||
for (const site of sites) {
|
||||
describe(site.name, () => {
|
||||
const lines = splitLines(fs.readFileSync(site.file, 'utf8'));
|
||||
const indexes = findAnchoredLineIndexes(lines, site.anchor, site.count);
|
||||
|
||||
indexes.forEach((i, n) => {
|
||||
// The split line and the PHASE_N line directly below it, verbatim.
|
||||
const splitLine = lines[i].trim();
|
||||
const nLine = lines[i + 1].trim();
|
||||
assert.ok(nLine.startsWith(`${site.output}=`), `line after the split must assign ${site.output}: ${nLine}`);
|
||||
const snippet = ['set -e', splitLine, nLine, `printf '%s' "$${site.output}"`].join('\n');
|
||||
const label = site.count > 1 ? ` (occurrence ${n + 1})` : '';
|
||||
|
||||
for (const [id, expected] of CLASS1_CASES) {
|
||||
test(`${id} → ${site.output}=${expected} without a shell error${label} (fails before the fix)`, () => {
|
||||
const r = runBash(snippet, { [site.input]: id });
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, expected);
|
||||
});
|
||||
}
|
||||
|
||||
for (const [id, expected] of CLASS1_CONTROLS) {
|
||||
test(`regression control: ${id} → ${site.output}=${expected}${label}`, () => {
|
||||
const r = runBash(snippet, { [site.input]: id });
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, expected);
|
||||
});
|
||||
}
|
||||
|
||||
test(`the commit-scope ERE built from PHASE_N matches both the padded and the unpadded scope of a letter phase${label}`, () => {
|
||||
// Each site feeds PHASE_N into `^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):`
|
||||
// — the #4003 zero-pad-tolerant scope. Prove the value it now yields
|
||||
// for `03A` matches the two subjects an executor could have written,
|
||||
// and does NOT match the letter-less phase 3.
|
||||
const script = [
|
||||
'set -e',
|
||||
splitLine,
|
||||
nLine,
|
||||
`SCOPE_RE="^[a-z]+\\((0*\${${site.output}})-(0*1)\\):"`,
|
||||
'for s in "feat(3A-01): x" "feat(03A-1): x"; do printf \'%s\\n\' "$s" | grep -qE "$SCOPE_RE" || { echo "MISS $s"; exit 3; }; done',
|
||||
'printf \'%s\\n\' "feat(3-01): x" | grep -qE "$SCOPE_RE" && { echo "FALSE-MATCH"; exit 4; }',
|
||||
'echo OK',
|
||||
].join('\n');
|
||||
const r = runBash(script, { [site.input]: '03A' });
|
||||
assert.equal(r.status, 0, `${r.stdout} ${r.stderr}`);
|
||||
assert.equal(r.stdout, 'OK');
|
||||
});
|
||||
});
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
describe('#4748 — execute-phase.md resolves the REVIEW.md path from init\'s padded_phase, not a shell re-pad', () => {
|
||||
const lines = splitLines(fs.readFileSync(EXECUTE_PHASE, 'utf8'));
|
||||
const [i] = findAnchoredLineIndexes(lines, 'REVIEW_FILE="${PHASE_DIR}/${PADDED}-REVIEW.md"', 1);
|
||||
const paddedLine = lines[i - 1].trim();
|
||||
|
||||
test('the PADDED binding directly above the lookup reads {padded_phase} (fails before the fix)', () => {
|
||||
// `printf "%02d"` cannot pad `03A` (prints `03`, exits 1) — and cannot
|
||||
// even re-pad an already-padded `08` (bash reads it as octal, prints
|
||||
// `00`). `init execute-phase` now emits `padded_phase` through the
|
||||
// canonical normalizer, so the workflow binds it instead of re-deriving.
|
||||
assert.ok(paddedLine.startsWith('PADDED='), `line above the lookup must bind PADDED: ${paddedLine}`);
|
||||
assert.equal(paddedLine, 'PADDED="{padded_phase}"');
|
||||
});
|
||||
|
||||
test('regression control: the lookup line itself is unchanged', () => {
|
||||
assert.equal(lines[i].trim(), 'REVIEW_FILE="${PHASE_DIR}/${PADDED}-REVIEW.md"');
|
||||
});
|
||||
|
||||
test('composition: the value init emits, substituted into the live lookup lines, resolves the letter phase\'s own REVIEW.md', (t) => {
|
||||
// The model substitutes `{padded_phase}` from the init JSON, which is
|
||||
// `normalizePhaseName(phase_number)` (src/init.cts). Do that substitution
|
||||
// here and run the three live lines against a fixture, so the emitted
|
||||
// value, the binding, the path construction and the status extraction are
|
||||
// exercised together — the executable half of a `{template}` site.
|
||||
const { normalizePhaseName } = require('../gsd-core/bin/lib/phase-id.cjs');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
const dir = createTempDir();
|
||||
t.after(() => cleanup(dir));
|
||||
for (const [id, status] of [['3A', 'clean'], ['8', 'issues'], ['9', 'skipped']]) {
|
||||
const emitted = normalizePhaseName(id);
|
||||
fs.writeFileSync(path.join(dir, `${emitted}-REVIEW.md`), `---\nstatus: ${status}\n---\n# review\n`);
|
||||
const script = [
|
||||
'set -e',
|
||||
paddedLine.replace('{padded_phase}', emitted),
|
||||
lines[i].trim(),
|
||||
lines[i + 1].trim(),
|
||||
'printf \'%s %s\' "$PADDED" "$REVIEW_STATUS"',
|
||||
].join('\n');
|
||||
assert.ok(lines[i + 1].includes('REVIEW_STATUS='), `line after the lookup must extract REVIEW_STATUS: ${lines[i + 1]}`);
|
||||
const r = runBash(script, { PHASE_DIR: dir });
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, `${emitted} ${status}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('the workflow\'s init parse list names padded_phase, so the binding is not a literal (fails before the fix)', () => {
|
||||
// A `{field}` token is substituted from the init JSON only for fields the
|
||||
// workflow tells the model to parse; `phase_number` is on that list and
|
||||
// `padded_phase` was not (adversarial review, claim 2).
|
||||
const [p] = findAnchoredLineIndexes(lines, 'Parse JSON for: `executor_model`', 1);
|
||||
assert.match(lines[p], /`phase_number`, `padded_phase`,/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#4748 — autonomous.md --from/--to/--only and plan-review-convergence.md extract the full letter-suffixed phase', () => {
|
||||
const autonomousText = fs.readFileSync(AUTONOMOUS, 'utf8');
|
||||
const prcText = fs.readFileSync(PLAN_REVIEW_CONVERGENCE, 'utf8');
|
||||
|
||||
const sites = [
|
||||
{ name: 'autonomous.md --from', pattern: extractGrepPattern(autonomousText, 'FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--from ${v}`, tail: "| awk '{print $2}'" },
|
||||
{ name: 'autonomous.md --to', pattern: extractGrepPattern(autonomousText, 'TO_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--from 1 --to ${v}`, tail: "| awk '{print $2}'" },
|
||||
{ name: 'autonomous.md --only', pattern: extractGrepPattern(autonomousText, 'ONLY_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--only ${v} --interactive`, tail: "| awk '{print $2}'" },
|
||||
{ name: 'plan-review-convergence.md', pattern: extractGrepPattern(prcText, 'PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `${v} --codex --max-cycles 3`, tail: '| head -1' },
|
||||
];
|
||||
|
||||
function extract(site, v) {
|
||||
const script = `echo "$ARGUMENTS" | grep -oE '${site.pattern}' ${site.tail}`;
|
||||
return runBash(script, { ARGUMENTS: site.args(v) }).stdout;
|
||||
}
|
||||
|
||||
for (const site of sites) {
|
||||
describe(site.name, () => {
|
||||
// Before the fix `[0-9]+\.?[0-9]*` stops at the letter: `12A` → `12`,
|
||||
// silently targeting a different phase. `23.1.2` → `23.1` is the same
|
||||
// truncation one axis over (#4568's class in a spelling neither lint saw).
|
||||
for (const v of ['12A', '3A', '23A.1.2', '23.1.2']) {
|
||||
test(`${v} extracts ${v}, not a truncated prefix (fails before the fix)`, () => {
|
||||
const got = extract(site, v);
|
||||
assert.equal(got, v);
|
||||
assert.equal(CANONICAL_ANCHORED.test(got), true);
|
||||
});
|
||||
}
|
||||
|
||||
for (const v of ['6', '36.14']) {
|
||||
test(`regression control: ${v} extracts ${v}`, () => {
|
||||
assert.equal(extract(site, v), v);
|
||||
});
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
test('autonomous.md: the three flags extract independently from one argument string', () => {
|
||||
const script = [
|
||||
`FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE '${sites[0].pattern}' | awk '{print $2}')`,
|
||||
`TO_PHASE=$(echo "$ARGUMENTS" | grep -oE '${sites[1].pattern}' | awk '{print $2}')`,
|
||||
'printf \'%s %s\' "$FROM_PHASE" "$TO_PHASE"',
|
||||
].join('\n');
|
||||
assert.equal(runBash(script, { ARGUMENTS: '--from 3A --to 5B --max-cycles 2' }).stdout, '3A 5B');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#4748 — phase-argument-parsing.md\'s legacy normalizer pads a letter-suffixed id instead of leaving it alone', () => {
|
||||
const lines = splitLines(fs.readFileSync(PHASE_ARGUMENT_PARSING, 'utf8'));
|
||||
const [start] = findAnchoredLineIndexes(lines, '# Normalize phase number', 1);
|
||||
let end = start;
|
||||
while (end < lines.length && lines[end].trim() !== 'fi') end++;
|
||||
assert.ok(end < lines.length, 'normalizer block must close with `fi`');
|
||||
const block = lines.slice(start, end + 1).join('\n');
|
||||
|
||||
function normalize(v) {
|
||||
return runBash(`set -e\n${block}\nprintf '%s' "$PHASE"`, { PHASE: v });
|
||||
}
|
||||
|
||||
// Before the fix neither branch matches a letter id, so `12A` passes through
|
||||
// unpadded and `3A` is never zero-padded to the `03A` a directory carries.
|
||||
for (const [input, expected] of [['3A', '03A'], ['12A', '12A'], ['3A.1', '03A.1'], ['23A.1.2', '23A.1.2']]) {
|
||||
test(`${input} → ${expected} (fails before the fix)`, () => {
|
||||
const r = normalize(input);
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, expected);
|
||||
assert.equal(CANONICAL_ANCHORED.test(r.stdout), true);
|
||||
});
|
||||
}
|
||||
|
||||
// `08` is the octal trap: `printf "%02d" 08` is an invalid octal number in
|
||||
// bash (exit 1, prints `00`), so the old integer branch mangled any
|
||||
// already-padded id it was handed. `23.1.2` matched neither old branch and
|
||||
// passed through unchanged — the N-segment axis was silently unpadded.
|
||||
for (const [input, expected] of [['08', '08'], ['23.1.2', '23.1.2']]) {
|
||||
test(`${input} → ${expected} without a shell error (fails before the fix)`, () => {
|
||||
const r = normalize(input);
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, expected);
|
||||
});
|
||||
}
|
||||
|
||||
for (const [input, expected] of [['8', '08'], ['2.1', '02.1'], ['36.14', '36.14']]) {
|
||||
test(`regression control: ${input} → ${expected}`, () => {
|
||||
const r = normalize(input);
|
||||
assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`);
|
||||
assert.equal(r.stdout, expected);
|
||||
});
|
||||
}
|
||||
|
||||
test('a non-canonical value passes through untouched (the normalizer is not a validator)', () => {
|
||||
const r = normalize('AUTH-101');
|
||||
assert.equal(r.status, 0);
|
||||
assert.equal(r.stdout, 'AUTH-101');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user