fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic (#4644)
* fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic $((10#${PHASE_NUMBER})) is a hard bash/zsh syntax error when PHASE_NUMBER is decimal (01.1, from an inserted phase) or N-segment (23.1.2) — neither is valid shell-arithmetic syntax at all, and the failed expansion aborts the rest of the snippet in a non-interactive shell. safe_resume_gate runs unconditionally before trusting STATE.md or dispatching any executor, so execute-phase failed at its own gate before the first executor on any decimal phase, regardless of workflow.tdd_mode. Regression from #4194. Fixes all 4 sites: safe_resume_gate and the TDD gate in workflows/execute-phase.md, the completion-signal spot-check fallback in workflows/execute-phase/steps/completion-reconciliation.md, and the executor gate validation example in references/tdd.md. Each now zero-strips only the leading integer segment into a *_INT variable (via %%.* / # parameter expansion — always valid shell syntax regardless of what follows) and keeps the remainder as an escaped-dot string for the anchored commit- scope regex, exactly as issue #4619 verified in both bash and zsh. A plain integer phase (12, 01) computes byte-identically to before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): pin the decimal/N-segment fix and characterize the pre-fix bug Behavioral coverage via real bash execution: the old $((10#01.1)) form throws (characterizes the bug, matching the issue's own reproduction); the new form resolves 01.1 -> 1\.1 and 23.1.2 -> 23\.1\.2, unchanged for plain integers (12 -> 12, 01 -> 1); the resulting anchored ERE matches feat(01.1-03):/test(1.1-3): and correctly rejects feat(01-03):, feat(01.2-03):, feat(011-03):, feat(12-03): for a decimal phase — mirroring issue #4619's own verified table exactly. Updates safe-resume-gate-anchoring.test.cjs's 4 existing source-text assertions (one per site) to the new fixed text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): refine the shell-arith drift detector to distinguish safe from unsafe arithmetic With #4619's fix in place, the guard's original "ban $((10#... outright, match any occurrence" was too blunt: it flagged a comment merely mentioning the pattern in prose, the now-safe $((10#$PHASE_INT)) arithmetic on an already-%%.*-stripped integer, and the always-safe plan-id arithmetic (plan ids are plain integers, never decimal). Refines the detector to skip full-line comments and to only flag a captured variable/placeholder name that contains "phase" and does NOT end in _INT/_int — the naming convention the #4619 fix establishes at all four sites for "already reduced to a safe integer." A plan-id variable was never phase-number arithmetic in the first place and is excluded on the same basis. This closes epic #4634's D6 ("lint-phase-id-drift... passes with no new exemptions") and D7 ("a decimal and N-segment phase id survive an end-to-end execute-phase selection without error") for real — the guard now reports zero violations across all five .cts/.md rules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: regenerate conformance-tier manifests for the new test file Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): cover the plain-padded-integer near-miss matrix too Review found the anchored-ERE near-miss coverage only exercised the decimal case (PHASE_NUMBER=01.1); issue #4619's own worked table also verifies the plain padded-integer case (01 -> PHASE_N=1) against its own near-miss set (matches 01-03, rejects 01.1-03/011-03/12-03). Adds the missing assertion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): add Fixed changeset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): correct JS backslash-escaping in safe-resume-gate anchoring test The test's string-literal assertions for the PHASE_FRAC//./\\.} pattern wrote only 2 backslash characters in JS source, which single-quoted-string parsing collapses to 1 real backslash at runtime -- but the workflow/reference files actually contain 2 raw backslash bytes at that position (needed so bash's ${var//pattern/replacement} produces the correct single-backslash output). Write 4 backslash characters in the JS source at all 4 occurrences so the runtime string matches the files' real bytes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): refresh the committed compact-content benchmark baseline The new PHASE_INT/PHASE_FRAC arithmetic lines added to gsd-core/workflows/execute-phase.md shifted its committed compaction-ratio baseline. Regenerate via `node scripts/benchmark-compact-content.cjs --write`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): note the safe_resume_gate arithmetic growth in the test header The emitted-attribution gate flags execute-phase.md growing 91253 -> 91846 bytes (593 bytes). The growth is the fix: the safe_resume_gate and TDD RED block now derive PHASE_INT/PHASE_FRAC before computing PHASE_N, so a decimal/N-segment phase number (e.g. 01.1, 2.3.1) zero-strips its leading integer segment via base-10 arithmetic instead of forcing the whole value through $((10#...)) and hitting a hard shell syntax error on the first dot. A blank line previously separated the Emitted-Drift-Ack-Growth trailer from the Co-Authored-By trailer below it, which splits git's trailer-block detection: only the last contiguous non-blank run of Key: Value lines at the end of a commit message is recognized as trailers, so the growth ack was silently read as ordinary body text and the differential-attribution gate failed with the growth unacknowledged. Joining the two trailers into one contiguous block fixes it. Emitted-Drift-Ack-Growth: execute-phase.md — adds PHASE_INT/PHASE_FRAC derivation to the safe_resume_gate and TDD RED commit-scope grep so a decimal/N-segment phase number zero-strips its leading integer segment via base-10 arithmetic instead of failing on a non-numeric value (#4619) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4208): replace chmod-based restore-failure injection with a root-proof git shim `tests/commit-files-deletion.test.cjs`'s two restore-failure tests simulated an unwritable index via a `post-index-change` hook running `chmod a-w` on the git dir. That relies on the OS enforcing the *owner's own* permission bits against itself, which uid 0 (a routine identity inside this repo's Docker-based gsd-test benches) does not: every DAC check short-circuits true for root, so the write the chmod meant to block silently succeeds, the restore comes back clean, and the disclosure/rollback behavior under test never actually gets exercised. This is CLAUDE.md's own named anti-pattern for I/O-failure injection ("Cross-platform test IO-failure injection" — chmod tricks fail under root Docker/CI). It is confirmed as the actual root cause here, not a production defect: `src/commands.cts`'s `restoreRemovedEntries`/rollback-disclosure logic (added by #4253, merged just before this run) was hand-traced and manually reproduced end to end on an unprivileged workstation against a freshly built `gsd-core/bin/lib/commands.cjs`, and it already produces exactly the `staging_failed` + "could not be restored" / "could NOT be restored during rollback" results both tests assert. The other `post-index-change`-based tests in this file (a `sleep` to force a timeout; a real `update-index` to flip a restored entry's mode) are unaffected because neither depends on a permission check — consistent with only the two chmod-based tests failing on the real remote run. Replaces the chmod fixture with a fake `git` placed ahead of the real one on PATH that fails only `update-index --add --cacheinfo` — the one call the restore makes — unconditionally, regardless of privilege level. Every other git invocation execs straight through to the real binary, so the rest of each scenario (`rm --cached`, the restore's own `ls-files` verification, etc.) is exercised exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): backfill changeset pr number to 4644 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): feed the bash fixture script via stdin, not argv, to fix Windows CI Passing the script as a `-c "<script>"` argv element made it subject to Windows' CreateProcess command-line argument encoding, which silently dropped the escaped-dot backslashes before bash ever saw them (observed on PR #4644's windows-latest CI shard: `1\.1` came back as `1.1`). Feeding the same script via stdin instead removes argv entirely from the transport, so there is nothing for Windows to re-encode. POSIX behavior is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user