diff --git a/.changeset/proud-goats-romp.md b/.changeset/proud-goats-romp.md new file mode 100644 index 000000000..356d772df --- /dev/null +++ b/.changeset/proud-goats-romp.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4644 +--- +**`execute-phase` no longer fails on a decimal or multi-segment phase** — an inserted phase (`01.1`) or an N-segment phase (`23.1.2`) hit a hard shell arithmetic syntax error at the very first gate (`safe_resume_gate`, which runs unconditionally before any executor dispatches), aborting the workflow before it could do anything. The phase number's leading integer segment is now zero-stripped for the commit-scope regex while the rest is kept as an escaped-dot string, instead of forcing the whole value through base-10 arithmetic. A plain integer phase is unaffected. (#4619) diff --git a/gsd-core/references/tdd.md b/gsd-core/references/tdd.md index 0411aec4d..5915d99bc 100644 --- a/gsd-core/references/tdd.md +++ b/gsd-core/references/tdd.md @@ -273,8 +273,11 @@ When `workflow.tdd_mode` is enabled in config, the RED/GREEN/REFACTOR gate seque After completing a `type: tdd` plan, the executor validates the git log: ```bash # The commit protocol promises no zero-padding for ${PHASE}/${PLAN} — strip both and -# match the commit-scope position anchored (#4003). -PHASE_N=$((10#${PHASE})); PLAN_N=$((10#${PLAN})) +# match the commit-scope position anchored (#4003). #4619: PHASE may be decimal/ +# N-segment; zero-strip only the leading integer segment, escape the rest. +PHASE_INT=${PHASE%%.*}; PHASE_FRAC=${PHASE#"$PHASE_INT"} +PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" +PLAN_N=$((10#${PLAN})) # Check for RED gate commit git log --oneline -E --grep="^test\((0*${PHASE_N})-(0*${PLAN_N})\):" | head -1 # Check for GREEN gate commit diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 14f8fb236..5cd28d9fa 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -190,7 +190,12 @@ from the active incomplete plan in `INIT`, then search recent history: SUMMARY_PATH="{phase_dir}/{plan_padded}-SUMMARY.md" # #4003: no padding rule in the commit protocol, so zero-strip both components and # match ANCHORED at the commit scope; bound to the latest reachable tag (milestone marker). -PHASE_N=$((10#{phase_number})) +PHASE_NUMBER="{phase_number}" +# #4619: {phase_number} may be decimal (01.1) or N-segment (23.1.2) — $((10#...)) +# is a hard shell syntax error on a non-integer, so zero-strip only the LEADING +# integer segment and keep the rest as an escaped-dot string for the ERE below. +PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"} +PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" PLAN_N=$((10#{plan_padded})) PLAN_SCOPE_RE="^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):" MILESTONE_BASE=$(git describe --tags --abbrev=0 2>/dev/null || echo "") @@ -211,7 +216,10 @@ if [ "$TDD_MODE" = "true" ]; then if [ "$IS_BEHAVIOR_ADDING" = "true" ]; then # #4003: same anchored scope and milestone bound as safe_resume_gate — a padded # literal grep hard-halts on a correct unpadded RED commit. - PHASE_N=$((10#${PHASE_NUMBER})) + # #4619: PHASE_NUMBER may be decimal/N-segment; zero-strip only the leading + # integer segment, escape the rest for the ERE below. + PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"} + PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" PLAN_N=$((10#${PLAN_ID})) PLAN_SCOPE_RE="^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):" # TDD gate's own scope check TDD_MILESTONE_BASE=$(git describe --tags --abbrev=0 2>/dev/null || echo "") diff --git a/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md b/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md index 42bfbd0fa..fb55f574d 100644 --- a/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md +++ b/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md @@ -27,7 +27,10 @@ block indefinitely waiting for a signal; verify via filesystem and git state. # For each plan in this wave, check if the executor finished: SUMMARY_EXISTS=$(test -f "{phase_dir}/{plan_number}-{plan_padded}-SUMMARY.md" && echo "true" || echo "false") # #4003: anchored, zero-pad-tolerant scope (see safe_resume_gate); --since stays. -SPOT_PHASE_N=$((10#{phase_number})) +SPOT_PHASE_NUMBER="{phase_number}" +# #4619: same decimal/N-segment handling as safe_resume_gate. +SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%.*}; SPOT_PHASE_FRAC=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"} +SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_FRAC//./\\.}" SPOT_PLAN_N=$((10#{plan_padded})) COMMITS_FOUND=$(git log --oneline --all -E --grep="^[a-z]+\((0*${SPOT_PHASE_N})-(0*${SPOT_PLAN_N})\):" --since="1 hour ago" | head -1) COMMITS_SINCE_DISPATCH=$(git log "${EXPECTED_BRANCH}" --since="${DISPATCH_TS}" --oneline | head -1) diff --git a/scripts/lib/macos-conformance-tier.generated.cjs b/scripts/lib/macos-conformance-tier.generated.cjs index 01ce918d5..94545e8a6 100644 --- a/scripts/lib/macos-conformance-tier.generated.cjs +++ b/scripts/lib/macos-conformance-tier.generated.cjs @@ -61,6 +61,7 @@ module.exports = { "tests/effort-sync-installed-runtime.test.cjs", "tests/emitted-attribution.test.cjs", "tests/ensure-runtime-build.test.cjs", + "tests/execute-phase-decimal-arithmetic.test.cjs", "tests/executed-plan.test.cjs", "tests/executor-mvp-tdd-section.test.cjs", "tests/external-job.test.cjs", diff --git a/scripts/lib/platform-conformance-tier.generated.cjs b/scripts/lib/platform-conformance-tier.generated.cjs index 29c23413a..d727f6b32 100644 --- a/scripts/lib/platform-conformance-tier.generated.cjs +++ b/scripts/lib/platform-conformance-tier.generated.cjs @@ -158,6 +158,7 @@ module.exports = { "tests/estimate-calibrate.test.cjs", "tests/estimate-loop-convergence.test.cjs", "tests/execute-mvp-tdd-gate.test.cjs", + "tests/execute-phase-decimal-arithmetic.test.cjs", "tests/execute-phase-wave.test.cjs", "tests/execute-phase-worktree-guard.test.cjs", "tests/execute-plan-update-codebase-map-diff-base.test.cjs", diff --git a/scripts/lint-phase-id-drift.cjs b/scripts/lint-phase-id-drift.cjs index a46157e1c..c709bbc40 100644 --- a/scripts/lint-phase-id-drift.cjs +++ b/scripts/lint-phase-id-drift.cjs @@ -234,10 +234,30 @@ function findBranchSlugFallbackDrift(text) { return out; } -// #4634: ban base-10-forced shell arithmetic (`$((10#...))`) on any variable — -// this construct is exactly the pattern that breaks on a decimal or -// multi-segment phase id, so any occurrence is banned outright, full stop. -const SHELL_PHASE_ARITH_DRIFT_RE = /\$\(\(\s*10#/; +// #4634: ban base-10-forced shell arithmetic (`$((10#...))`) on a variable +// that still carries a possibly-decimal/multi-segment phase id — this +// construct is exactly the pattern that breaks on a value like `08.5`. The +// capture group grabs the token immediately inside the parens (after an +// optional `$` and/or `{`, stripping a trailing `}`) so callers can inspect +// *which* variable is being coerced, not merely that the substring occurred. +// +// Refined post-#4619: the original blunt "ban `$((10#` outright" version +// over-fired on three false-positive classes once #4619's fix landed: +// 1. Prose mentioning the literal pattern in a full-line `#`-comment +// (filtered by the caller, not this regex — see below). +// 2. `$((10#$PHASE_INT))` / `$((10#$SPOT_PHASE_INT))` — arithmetic on the +// NOW-safe variable the #4619 fix produces via `PHASE_INT=${PHASE_NUMBER%%.*}`; +// a `%%.*`-stripped value can never contain a dot, so base-10 arithmetic +// on it can never hit the #4619 syntax-error class. Any name ending in +// `_INT` (case-insensitive) is that established "already reduced to a +// safe integer" convention. +// 3. `$((10#{plan_padded}))` / `$((10#${PLAN_ID}))` — plan ids are plain +// integers and were never in scope; this rule only polices variables +// that carry a *phase* id. +// So a match is only a violation when the captured name contains `phase` +// case-insensitively (it is phase-carrying) AND does not end in `_int` +// case-insensitively (it has not already been reduced to a safe integer). +const SHELL_PHASE_ARITH_DRIFT_RE = /\$\(\(\s*10#\$?\{?([A-Za-z0-9_]+)\}?/; // A markdown comment can't easily carry a `//` line, so the sanction for the // shell-arithmetic rule is an HTML comment on the nearest preceding non-blank @@ -246,15 +266,25 @@ const MD_OWNER_RE = /^\s*` - * on the nearest preceding non-blank line. Returns [{ line, found }]. + * site in `text` that still coerces an un-reduced phase-carrying variable. + * Skips full-line `#` comments outright (pure prose mentioning the pattern, + * not executable code), and skips any captured variable name that either + * doesn't contain `phase` (never in scope — e.g. plan ids) or already ends + * in `_int` (the #4619-fix convention for "safely stripped to an integer"). + * Sanctioned by an HTML comment `` on the + * nearest preceding non-blank line. Returns [{ line, found }]. */ function findShellPhaseArithDrift(text) { const out = []; const lines = text.split('\n'); for (let i = 0; i < lines.length; i++) { - const m = SHELL_PHASE_ARITH_DRIFT_RE.exec(lines[i]); + const line = lines[i]; + if (/^\s*#/.test(line)) continue; + const m = SHELL_PHASE_ARITH_DRIFT_RE.exec(line); if (!m) continue; + const name = m[1]; + if (!/phase/i.test(name)) continue; + if (/_int$/i.test(name)) continue; if (isSanctionedByPrecedingComment(lines, i, MD_OWNER_RE)) continue; out.push({ line: i + 1, found: m[0] }); } diff --git a/tests/commit-files-deletion.test.cjs b/tests/commit-files-deletion.test.cjs index 6228424ca..e9d70b13d 100644 --- a/tests/commit-files-deletion.test.cjs +++ b/tests/commit-files-deletion.test.cjs @@ -12,7 +12,9 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); +const os = require('os'); const path = require('path'); +const { execFileSync } = require('child_process'); const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); const fc = require('fast-check'); const { collectListFlagValues, COMMIT_LIST_FLAGS } = require('../gsd-core/bin/gsd-tools.cjs'); @@ -487,6 +489,49 @@ describe('commit --files-removed: index states absent by design are never remova beforeEach(() => { tmpDir = createTempGitProject(); stray = null; }); afterEach(() => { cleanup(tmpDir); if (stray) cleanup(stray); }); + // Deterministic, privilege-independent restore-failure injection. + // `chmod a-w` on the git dir (as this file's other restore-failure tests + // used to) relies on the OS enforcing the *owner's own* permission bits + // against itself -- which root, a routine identity inside a Docker-based + // CI bench, does not: every DAC check short-circuits true for uid 0, so the + // write the chmod meant to block SUCCEEDS, the restore silently comes back + // clean, and the disclosure this test exists to pin never fires. That is + // this repo's own named anti-pattern for I/O-failure injection (see + // CLAUDE.md "Cross-platform test IO-failure injection") -- and it was the + // actual root cause here: the two tests below failed under a real remote + // `gsd-test` run against unmodified `next` (root inside the bench + // container) while passing on an unprivileged workstation, and every OTHER + // fault-injection test in this file that does NOT depend on a permission + // check (the timeout hook two tests down that just sleeps; the mode-flip + // hook after it that runs a real `update-index`) passed in that same run. + // The fix here targets the CALL, not a permission bit: a fake `git` ahead + // of the real one on PATH turns `update-index --add --cacheinfo` — the one + // and only call the restore path makes — into a hard failure unconditionally, + // in any process regardless of uid. Every other invocation execs straight + // through to the real binary, so the rest of the commit (the `rm --cached`, + // the verification `ls-files`, etc.) behaves exactly as it does today. + function findRealGit() { + return execFileSync('command', ['-v', 'git'], { shell: '/bin/sh', timeout: GIT_TIMEOUT_MS }).toString().trim(); + } + function installCacheinfoRestoreFailureShim() { + const realGit = findRealGit(); + const shimDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-fake-git-')); + fs.writeFileSync(path.join(shimDir, 'git'), [ + '#!/bin/sh', + 'has_cacheinfo=0', + 'for arg in "$@"; do', + ' if [ "$arg" = "--cacheinfo" ]; then has_cacheinfo=1; fi', + 'done', + 'if [ "$1" = "update-index" ] && [ "$has_cacheinfo" = "1" ]; then', + ' echo "fake-git: forced update-index --cacheinfo failure for test" >&2', + ' exit 1', + 'fi', + `exec "${realGit}" "$@"`, + '', + ].join('\n'), { mode: 0o755 }); + return { shimDir, path: `${shimDir}${path.delimiter}${process.env.PATH}` }; + } + test('a directory entry leaves a hand-deleted submodule gitlink in the index and records only the file move', () => { seedMove(); addSubmoduleThenDeleteDir(); @@ -705,14 +750,15 @@ describe('commit --files-removed: index states absent by design are never remova }); test('a removal the call cannot put back is reported, never as nothing_to_commit', - { skip: process.platform === 'win32' ? 'chmod cannot make a directory unwritable on Windows (driven: a write into a ReadOnly directory succeeds), so the fixture cannot drive a failed restore' : false }, + { skip: process.platform === 'win32' ? 'the fault-injection shim is a #!/bin/sh script resolved via PATH; Windows git resolution needs a .exe/.cmd shim, a separate fixture' : false }, (t) => { // The restore is best-effort, so it can FAIL -- and reporting // nothing_to_commit over a removal we tried and could not undo is the same // false "no state changed" the restore exists to prevent, one level down. - // Driven with a post-index-change hook that makes the git dir unwritable - // the moment `rm --cached` lands, so the `update-index --cacheinfo` restore - // cannot take its lock. + // Driven with a fake `git` ahead of the real one on PATH that fails the + // one call the restore makes (`update-index --add --cacheinfo`) — see + // installCacheinfoRestoreFailureShim's header for why this replaced a + // chmod-based hook. fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); fs.writeFileSync(path.join(tmpDir, PENDING, 'seed.md'), 'seed\n'); git(['add', '.planning/']); @@ -720,29 +766,14 @@ describe('commit --files-removed: index states absent by design are never remova fs.writeFileSync(path.join(tmpDir, PENDING, 'gone.md'), 'gone\n'); git(['add', path.join(PENDING, 'gone.md')]); fs.unlinkSync(path.join(tmpDir, PENDING, 'gone.md')); - const gitDir = path.join(tmpDir, '.git'); - const hooksDir = path.join(gitDir, 'hooks'); - fs.mkdirSync(hooksDir, { recursive: true }); - fs.writeFileSync(path.join(hooksDir, 'post-index-change'), - '#!/bin/sh\nchmod a-w "$(git rev-parse --git-dir)"\n', { mode: 0o755 }); - // Give the dir back in a FINALLY below, not only in `t.after`: t.after runs - // AFTER the parent afterEach, so a throw between the hook and the explicit - // chmod leaves afterEach unable to delete the fixture. t.after stays as a - // belt for the case where the finally itself is skipped. - t.after(() => { try { fs.chmodSync(gitDir, 0o755); } catch { /* already writable */ } }); - const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); - fs.writeFileSync(emptyConfig, ''); + const shim = installCacheinfoRestoreFailureShim(); + t.after(() => cleanup(shim.shimDir)); - let result; - try { - result = runGsdTools( - ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/gone.md'], - tmpDir, - { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, - ); - } finally { - fs.chmodSync(gitDir, 0o755); - } + const result = runGsdTools( + ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/gone.md'], + tmpDir, + { PATH: shim.path }, + ); const parsed = JSON.parse(result.output); assert.strictEqual(parsed.committed, false, result.output); assert.notStrictEqual(parsed.reason, 'nothing_to_commit', 'a removal left staged must never be reported as no state change'); @@ -752,38 +783,29 @@ describe('commit --files-removed: index states absent by design are never remova }); test('a rollback that cannot restore a removal discloses it, even when the reported failure is another entry', - { skip: process.platform === 'win32' ? 'chmod cannot make a directory unwritable on Windows (driven: a write into a ReadOnly directory succeeds), so the fixture cannot drive a failed restore' : false }, + { skip: process.platform === 'win32' ? 'the fault-injection shim is a #!/bin/sh script resolved via PATH; Windows git resolution needs a .exe/.cmd shim, a separate fixture' : false }, (t) => { // The rollback exit reports the failure that CAUSED it -- here a // contradictory declaration about a path still on disk -- so a caller // reading `failures` would learn nothing about the removal this call had // already staged and then could not put back. Both must be disclosed. + // Driven with the same fake-`git` restore-failure shim as the test above + // (see installCacheinfoRestoreFailureShim's header). seedMove(); fs.writeFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'stays\n'); git(['add', path.join(PENDING, 'stays.md')]); git(['commit', '-q', '-m', 'seed a present todo']); - const gitDir = path.join(tmpDir, '.git'); - const hooksDir = path.join(gitDir, 'hooks'); - fs.mkdirSync(hooksDir, { recursive: true }); - fs.writeFileSync(path.join(hooksDir, 'post-index-change'), - '#!/bin/sh\nchmod a-w "$(git rev-parse --git-dir)"\n', { mode: 0o755 }); - t.after(() => { try { fs.chmodSync(gitDir, 0o755); } catch { /* already writable */ } }); - const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); - fs.writeFileSync(emptyConfig, ''); + const shim = installCacheinfoRestoreFailureShim(); + t.after(() => cleanup(shim.shimDir)); - let result; - try { - // mine.md was moved away (a real removal); stays.md is still on disk, so - // declaring it removed contradicts the declaration and fails the call. - result = runGsdTools( - ['commit', 'docs: bad declaration', - '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/stays.md'], - tmpDir, - { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, - ); - } finally { - fs.chmodSync(gitDir, 0o755); - } + // mine.md was moved away (a real removal); stays.md is still on disk, so + // declaring it removed contradicts the declaration and fails the call. + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/stays.md'], + tmpDir, + { PATH: shim.path }, + ); const parsed = JSON.parse(result.output); assert.strictEqual(parsed.reason, 'staging_failed', result.output); assert.strictEqual(parsed.file, '.planning/todos/pending/stays.md', 'the REPORTED failure is still the contradictory declaration'); diff --git a/tests/execute-phase-decimal-arithmetic.test.cjs b/tests/execute-phase-decimal-arithmetic.test.cjs new file mode 100644 index 000000000..8f2932017 --- /dev/null +++ b/tests/execute-phase-decimal-arithmetic.test.cjs @@ -0,0 +1,162 @@ +'use strict'; + +/** + * #4619 — execute-phase's `$((10#{phase_number}))` shell arithmetic is a hard + * syntax error when `{phase_number}` is decimal (inserted phase, e.g. `01.1`) or + * N-segment (e.g. `23.1.2`): `$((10#01.1))` aborts the whole script in a + * non-interactive shell, both in bash and zsh. The fix zero-strips only the + * LEADING integer segment via parameter expansion and keeps the remainder as an + * escaped-dot string for the downstream anchored ERE — never touching real + * shell arithmetic on a non-integer value. + * + * These tests are BEHAVIORAL: they execute the actual fixed snippet (and, for + * the regression control, the actual OLD broken snippet) via a real bash + * subprocess, asserting on literal stdout/exit-code — not just string-matching + * the source. A companion sourcetext check (mirroring the established pattern + * in tests/safe-resume-gate-anchoring.test.cjs) proves each of the 4 real + * production sites still carries a byte-identical copy of the fixed logic + * (under each site's own variable-name spelling), so a future accidental + * revert of any ONE site is caught. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); + +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 TIMEOUT = 5000; + +// The fixed transformation, parameterized by (source variable, target prefix) — this +// is a byte-for-byte copy of what ships at all 4 sites: +// site 1/2 (execute-phase.md): source PHASE_NUMBER, prefix PHASE +// site 3 (completion-reconciliation.md): source SPOT_PHASE_NUMBER, prefix SPOT_PHASE +// site 4 (tdd.md): source PHASE, prefix PHASE +function fixedSnippet(sourceVar, prefix, indent = '') { + return `${indent}${prefix}_INT=\${${sourceVar}%%.*}; ${prefix}_FRAC=\${${sourceVar}#"$${prefix}_INT"}\n` + + `${indent}${prefix}_N="$((10#$${prefix}_INT))\${${prefix}_FRAC//./\\\\.}"`; +} + +function runFixed(phaseNumberValue) { + const script = `PHASE_NUMBER="${phaseNumberValue}"\n${fixedSnippet('PHASE_NUMBER', 'PHASE')}\necho "$PHASE_N"`; + return execFileSync('bash', [], { input: script, encoding: 'utf8', timeout: TIMEOUT }).trim(); +} + +describe('#4619 — execute-phase decimal/N-segment phase-number arithmetic', () => { + test('fixed snippet zero-strips the leading integer segment for a decimal phase (01.1 -> 1\\.1)', () => { + assert.equal(runFixed('01.1'), '1\\.1'); + }); + + test('fixed snippet zero-strips the leading integer segment for an N-segment phase (23.1.2 -> 23\\.1\\.2)', () => { + assert.equal(runFixed('23.1.2'), '23\\.1\\.2'); + }); + + test('regression control: a plain unpadded phase number is unchanged (12 -> 12)', () => { + assert.equal(runFixed('12'), '12'); + }); + + test('regression control: a padded plain phase number is still zero-stripped (01 -> 1)', () => { + assert.equal(runFixed('01'), '1'); + }); + + test('failing-first: the OLD $((10#...)) form is a hard shell syntax error on a decimal phase number', () => { + assert.throws(() => { + execFileSync('bash', ['-c', 'echo $((10#01.1))'], { encoding: 'utf8', timeout: TIMEOUT }); + }, /syntax error|status/); + }); + + test('the NEW form succeeds on the exact same input that hard-errors the OLD form', () => { + assert.doesNotThrow(() => runFixed('01.1')); + }); + + test('the resulting anchored ERE matches decimal commit scopes and rejects near-miss scopes', () => { + const phaseN = runFixed('01.1'); // '1\.1' + const planN = '3'; + const re = `^[a-z]+\\((0*${phaseN})-(0*${planN})\\):`; + const cases = [ + ['feat(01.1-03):', true], + ['test(1.1-3):', true], + ['feat(01-03):', false], + ['feat(01.2-03):', false], + ['feat(011-03):', false], + ['feat(12-03):', false], + ]; + for (const [subject, expected] of cases) { + const script = `echo ${JSON.stringify(subject)} | grep -qE ${JSON.stringify(re)}`; + let matched; + try { + execFileSync('bash', [], { input: script, encoding: 'utf8', timeout: TIMEOUT }); + matched = true; + } catch { + matched = false; + } + assert.equal(matched, expected, `expected ${subject} match=${expected} against ${re}`); + } + }); + + test('the resulting anchored ERE matches a plain padded-integer phase and rejects near-miss scopes', () => { + const phaseN = runFixed('01'); // '1' + const planN = '3'; + const re = `^[a-z]+\\((0*${phaseN})-(0*${planN})\\):`; + const cases = [ + ['feat(01-03):', true], + ['feat(01.1-03):', false], + ['feat(011-03):', false], + ['feat(12-03):', false], + ]; + for (const [subject, expected] of cases) { + const script = `echo ${JSON.stringify(subject)} | grep -qE ${JSON.stringify(re)}`; + let matched; + try { + execFileSync('bash', [], { input: script, encoding: 'utf8', timeout: TIMEOUT }); + matched = true; + } catch { + matched = false; + } + assert.equal(matched, expected, `expected ${subject} match=${expected} against ${re}`); + } + }); + + describe('source parity — each of the 4 production sites carries the fixed logic', () => { + test('execute-phase.md safe_resume_gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_FRAC/PHASE_N logic', () => { + const w = fs.readFileSync(EXECUTE_PHASE, 'utf8'); + assert.ok(w.includes(fixedSnippet('PHASE_NUMBER', 'PHASE')), + 'safe_resume_gate must carry the byte-identical fixed decimal-tolerant snippet'); + }); + + test('execute-phase.md TDD gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_FRAC/PHASE_N logic', () => { + const w = fs.readFileSync(EXECUTE_PHASE, 'utf8'); + // The TDD gate block is nested one level deeper (4-space indent) than + // safe_resume_gate's top-level snippet. + assert.ok(w.includes(fixedSnippet('PHASE_NUMBER', 'PHASE', ' ')), + 'the TDD gate must carry the byte-identical fixed decimal-tolerant snippet (indented)'); + }); + + test('completion-reconciliation.md carries the fixed SPOT_-prefixed logic', () => { + const frag = fs.readFileSync(COMPLETION_RECONCILIATION, 'utf8'); + assert.ok(frag.includes(fixedSnippet('SPOT_PHASE_NUMBER', 'SPOT_PHASE')), + 'completion-reconciliation spot-check must carry the byte-identical fixed SPOT_-prefixed snippet'); + }); + + test('tdd.md carries the fixed bare PHASE/PLAN logic', () => { + const ref = fs.readFileSync(TDD_REF, 'utf8'); + assert.ok(ref.includes(fixedSnippet('PHASE', 'PHASE')), + 'tdd.md gate-enforcement example must carry the byte-identical fixed bare-PHASE snippet'); + }); + + test('none of the 4 sites still contains the old unconditional $((10#...)) form on a template/variable phase number', () => { + const w = fs.readFileSync(EXECUTE_PHASE, 'utf8'); + const frag = fs.readFileSync(COMPLETION_RECONCILIATION, 'utf8'); + const ref = fs.readFileSync(TDD_REF, 'utf8'); + assert.ok(!w.includes('PHASE_N=$((10#{phase_number}))'), 'old broken form must not remain in execute-phase.md (site 1)'); + assert.ok(!w.includes('PHASE_N=$((10#${PHASE_NUMBER}))'), 'old broken form must not remain in execute-phase.md (site 2)'); + assert.ok(!frag.includes('SPOT_PHASE_N=$((10#{phase_number}))'), 'old broken form must not remain in completion-reconciliation.md (site 3)'); + assert.ok(!ref.includes('PHASE_N=$((10#${PHASE}))'), 'old broken form must not remain in tdd.md (site 4)'); + }); + }); +}); diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index af0ef488e..15e6db86a 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -18,9 +18,9 @@ "reductionPct": 16.51 }, "execute-phase": { - "offTokens": 25643, - "onTokens": 23392, - "reductionPct": 8.78 + "offTokens": 25827, + "onTokens": 23576, + "reductionPct": 8.72 }, "new-project": { "offTokens": 14279, @@ -39,8 +39,8 @@ } }, "aggregate": { - "offTokens": 107102, - "onTokens": 90454, - "reductionPct": 15.54 + "offTokens": 107286, + "onTokens": 90638, + "reductionPct": 15.52 } } diff --git a/tests/lint-phase-id-drift.test.cjs b/tests/lint-phase-id-drift.test.cjs index 39130d3f0..d4a18c92f 100644 --- a/tests/lint-phase-id-drift.test.cjs +++ b/tests/lint-phase-id-drift.test.cjs @@ -86,3 +86,28 @@ test('findShellPhaseArithDrift does NOT flag a site sanctioned with an HTML comm ].join('\n'); assert.deepEqual(findShellPhaseArithDrift(text), []); }); + +test('findShellPhaseArithDrift still flags a raw un-reduced phase variable (#4619 regression)', () => { + const text = 'PHASE_N=$((10#$PHASE_NUMBER))'; + const found = findShellPhaseArithDrift(text); + assert.equal(found.length, 1); + assert.equal(found[0].line, 1); +}); + +test('findShellPhaseArithDrift does NOT flag arithmetic on an already-`_INT`-reduced phase variable', () => { + const text = [ + 'PHASE_INT=${PHASE_NUMBER%%.*}', + 'PHASE_N=$((10#$PHASE_INT))', + ].join('\n'); + assert.deepEqual(findShellPhaseArithDrift(text), []); +}); + +test('findShellPhaseArithDrift does NOT flag arithmetic on a plan-id variable (never phase-carrying)', () => { + const text = 'PLAN_N=$((10#${PLAN_ID}))'; + assert.deepEqual(findShellPhaseArithDrift(text), []); +}); + +test('findShellPhaseArithDrift skips a full-line comment merely mentioning the pattern as prose', () => { + const text = '# Note: $((10#$PHASE_NUMBER)) is a hard shell syntax error on a decimal id.'; + assert.deepEqual(findShellPhaseArithDrift(text), []); +}); diff --git a/tests/phase-id-drift-guard.test.cjs b/tests/phase-id-drift-guard.test.cjs index a493a7562..76001adc2 100644 --- a/tests/phase-id-drift-guard.test.cjs +++ b/tests/phase-id-drift-guard.test.cjs @@ -366,20 +366,18 @@ describe('#2128 phase-id drift scanner: the live repo is clean', () => { } }); - test( - "#4619 shell-arith violations are known and tracked separately (characterization, not this PR's scope)", - () => { - const violations = scanMarkdownShellArith(ROOT); - assert.equal( - violations.length, - 7, - 'expected exactly the 7 known #4619 sites (workflows/execute-phase.md x4, ' + - 'workflows/execute-phase/steps/completion-reconciliation.md x2, references/tdd.md x1) — ' + - 'if this count changed, either #4619 was fixed (great — update/remove this pin) or a NEW ' + - 'unrelated shell-arith site was introduced (investigate before adjusting the number)', - ); - }, - ); + test('scanMarkdownShellArith finds zero unsanctioned shell phase-arithmetic (#4619 fixed)', () => { + // Was a characterization test pinning 7 known #4619 sites + // (workflows/execute-phase.md x4, workflows/execute-phase/steps/ + // completion-reconciliation.md x2, references/tdd.md x1) while #4619 was + // still unfixed. #4619 is now fixed — every site zero-strips the leading + // integer segment into a `*_INT`-suffixed variable before doing + // `$((10#...))` arithmetic on it, which the refined detector recognizes + // as safe — so this retires back to the same "must be zero" assertion + // the branch-slug-fallback pin used once ITS underlying bug was fixed. + const violations = scanMarkdownShellArith(ROOT); + assert.equal(violations.length, 0); + }); }); describe('#2128 phase-id single-owner identity guard', () => { diff --git a/tests/safe-resume-gate-anchoring.test.cjs b/tests/safe-resume-gate-anchoring.test.cjs index 3ca931342..87f4acfde 100644 --- a/tests/safe-resume-gate-anchoring.test.cjs +++ b/tests/safe-resume-gate-anchoring.test.cjs @@ -10,6 +10,11 @@ * live in this repository's history). Workflow text IS the deployed product here, so * the shape assertions are the faithful check; the behavioral fixture row runs the * actual pipeline against a crafted history. + * + * #4619 — the gate's PHASE_N derivation grew to zero-strip only the leading + * integer segment of a decimal/N-segment phase number (`01.1`, `23.1.2`) via + * base-10 arithmetic, instead of forcing the whole value through + * `$((10#...))` and hitting a hard shell syntax error on the first dot. */ const { test, describe } = require('node:test'); @@ -26,8 +31,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { const w = fs.readFileSync(WORKFLOW, 'utf8'); // Anchored, ERE, zero-pad-tolerant on BOTH components — matches feat(2-02): and // feat(02-02): alike, never a substring elsewhere in the message. - assert.ok(w.includes('PHASE_N=$((10#{phase_number}))'), - 'phase component must be zero-stripped via arithmetic base-10'); + assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}') && + w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"'), + 'phase component must be zero-stripped via arithmetic base-10 (#4619: leading integer segment only, decimal/N-segment tolerant)'); assert.ok(w.includes('PLAN_N=$((10#{plan_padded}))'), 'plan component must be zero-stripped via arithmetic base-10'); assert.ok(w.includes('PLAN_SCOPE_RE="^[a-z]+\\((0*${PHASE_N})-(0*${PLAN_N})\\):"'), @@ -54,8 +60,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { test('tdd red gate tolerates both commit-scope spellings (#4011 keying untouched)', () => { const w = fs.readFileSync(WORKFLOW, 'utf8'); - assert.ok(w.includes('PHASE_N=$((10#${PHASE_NUMBER}))') && w.includes('PLAN_N=$((10#${PLAN_ID}))'), - 'the TDD block derives zero-stripped components'); + assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}') && + w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"') && w.includes('PLAN_N=$((10#${PLAN_ID}))'), + 'the TDD block derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); assert.ok(w.includes('RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "**/*.test.*"'), 'the RED grep must use the same anchored padding-tolerant scope, milestone-bounded'); assert.ok(!w.includes('--grep="^test(${PHASE_NUMBER}-${PLAN_ID})"'), @@ -73,8 +80,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { 'the padded-literal example grep must not remain'); assert.ok(ref.includes('--grep="^test\\((0*${PHASE_N})-(0*${PLAN_N})\\):"'), 'the RED example is anchored and zero-pad-tolerant'); - assert.ok(ref.includes('PHASE_N=$((10#${PHASE})); PLAN_N=$((10#${PLAN}))'), - 'the examples derive zero-stripped components'); + assert.ok(ref.includes('PHASE_INT=${PHASE%%.*}; PHASE_FRAC=${PHASE#"$PHASE_INT"}') && + ref.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"') && ref.includes('PLAN_N=$((10#${PLAN}))'), + 'the examples derive zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); }); test('completion spot-check uses the anchored scope and keeps its time bound', () => { @@ -88,8 +96,10 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { 'execute-phase', 'steps', 'completion-reconciliation.md'), 'utf8'); assert.ok(!w.includes('--grep="{phase_number}-{plan_padded}"') && !frag.includes('--grep="{phase_number}-{plan_padded}"'), 'the raw padded placeholder substring grep must not remain'); - assert.ok(frag.includes('SPOT_PHASE_N=$((10#{phase_number}))') && frag.includes('SPOT_PLAN_N=$((10#{plan_padded}))'), - 'the spot-check derives zero-stripped components'); + assert.ok(frag.includes('SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%.*}; SPOT_PHASE_FRAC=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"}') && + frag.includes('SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_FRAC//./\\\\.}"') && + frag.includes('SPOT_PLAN_N=$((10#{plan_padded}))'), + 'the spot-check derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); assert.ok(frag.includes('--since="1 hour ago"'), 'the spot-check keeps its temporal bound'); });