From 115433bba67ea4c523cb605cb6e6a22e59bb8efd Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 26 Jul 2026 14:14:28 -0400 Subject: [PATCH] fix(#2539): anchor commit phase-token detection; drop silent wrong-branch switch (#2669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2539): anchor commit phase-token extraction to the phases/ segment; drop silent switch-to-existing cmdCommit auto-detected the commit's phase from --files with an unanchored `match(/(\d+(?:\.\d+)*)-/)`, which returns the leftmost digit-run-then-hyphen anywhere in the joined path. A project_code ending in a digit (PROJECT_V2) made `.planning/phases/PROJECT_V2-07-name/…` match the `2-` inside `V2-` before the real `07-` token, resolving phase 2. findPhaseInternal also searches archived milestones, so an existing archived phase 2 produced a real branch name and the silent `git checkout ` fallback switched the whole working tree onto the wrong branch in the same call that then committed. The extraction now anchors to the directory segment immediately under `.planning/phases/` (or `.planning/milestones/-phases/`) and runs it through the existing project-code-aware extractPhaseToken helper — the single owner shared by the other 6 call sites — rather than introducing a fourth independent copy of phase-token-matching logic. The auto-switch keeps create-if-absent only (the #1278 intent: ensure the branch exists before the first commit on it); it no longer force-switches an already-checked-out working branch onto a different existing branch. Adds two regression fixtures: a digit-suffixed project_code + an archived phase whose number collides with the trailing digit (the silent-wrong-branch case), and a pre-existing phase branch that must not be silently switched onto. * test(#2539): assert non-silent warning; hoist execFileSync; normalizePhaseName guard Address orthogonal-review findings on the #2539 fix: - Spec AC2 ('an auto-checkout mid-commit must never happen silently'): the no-switch path now writes a 'Warning: resolved phase branch X already exists; committing on Y instead' line to stderr when checkout -b fails because the branch already exists. The regression test captures stderr via spawnSync and asserts the warning, so neither direction of the branching resolution is silent. - Spec AC3 ('reuse normalizePhaseName/extractPhaseToken/stripProjectCodePrefix'): the token-acceptance guard now runs the candidate token through normalizePhaseName and accepts it only when it normalizes to a numeric phase form, rather than the brittle 'token !== phaseDir && /\d/.test(token)' check that leaned on extractPhaseToken's undocumented dirName fallback. - Standards (Duplicated Code): hoist execFileSync/spawnSync requires to the top of the 'commit command' describe block instead of inlining them per test. * fix(#2539): build phase-token shape from PHASE_NUMBER_TOKEN_SOURCE (#2128 guard) The acceptance guard regex in detectPhaseNumberFromFiles was a hardcoded `/^\d+[A-Z]?(?:\.\d+)*$/i` — a literal re-derivation of the canonical phase-number grammar, which the #2128 phase-id drift guard (tests/phase-id-drift-guard.test.cjs) rejects unless sanctioned with a `// phase-id-owner:` marker. Build it from the single-owner PHASE_NUMBER_TOKEN_SOURCE export instead, so this read-side acceptance check cannot drift from every other phase-token reader. gsd-test reported this as 2 failures (linux-node22 + linux-node24) on the prior commit. * docs(#2539): backfill changeset pr: 2669 --- .../2539-commit-phase-detection-anchored.md | 5 + src/commands.cts | 92 ++++++++++- tests/commands.test.cjs | 150 +++++++++++++++++- 3 files changed, 239 insertions(+), 8 deletions(-) create mode 100644 .changeset/2539-commit-phase-detection-anchored.md diff --git a/.changeset/2539-commit-phase-detection-anchored.md b/.changeset/2539-commit-phase-detection-anchored.md new file mode 100644 index 000000000..35cb149db --- /dev/null +++ b/.changeset/2539-commit-phase-detection-anchored.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2669 +--- +**`query commit --files` no longer silently checks out the wrong phase branch mid-commit** — the phase-token extraction is now anchored to the directory segment under `.planning/phases/` and reuses the project-code-aware `extractPhaseToken` helper instead of an unanchored regex, so a `project_code` ending in a digit (e.g. `PROJECT_V2`) no longer makes `…/PROJECT_V2-07-name/…` match the `2-` inside `V2-` and resolve to the wrong phase. The commit-path branch auto-switch also no longer silently force-switches an already-checked-out working branch onto a different existing phase branch (it creates-if-absent only, per the original `#1278` intent); the only prior trace of the silent switch was a `git reflog` entry. (#2539) diff --git a/src/commands.cts b/src/commands.cts index 72e5522d7..68efa923f 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -21,7 +21,7 @@ import coreUtilsMod = require('./core-utils.cjs'); const { toPosixPath, generateSlugInternal, extractOneLinerFromBody } = coreUtilsMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { normalizePhaseName, comparePhaseNum, extractPhaseToken } = phaseIdMod; +const { normalizePhaseName, comparePhaseNum, extractPhaseToken, PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); const { getArchivedPhaseDirs, findPhaseInternal } = phaseLocatorMod; @@ -736,6 +736,58 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con output({ synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir }, raw, synced > 0 ? 'changed' : 'ok'); } +/** + * Detect the phase number for a commit from its `--files` path list. + * + * #2539: the extraction is anchored to the directory segment immediately under + * `.planning/phases/` or `.planning/milestones/-phases/`, then run + * through the project-code-aware `extractPhaseToken` helper. The prior + * unanchored `match(/(\d+(?:\.\d+)*)-/)` returned the leftmost digit-run-then- + * hyphen anywhere in the joined path, so a project_code ending in a digit + * (e.g. PROJECT_V2) made `…/PROJECT_V2-07-name/…` match the `2-` inside `V2-` + * before the real `07-` phase token — resolving phase "2" instead of "7". + * + * Returns the phase number string (e.g. '07', '45.14'), or null when no phase + * directory segment is present in any of the file paths (e.g. a commit of + * `.planning/ROADMAP.md` has no phase segment, so no branch is resolved — + * matching the prior regex-no-match behaviour). + */ +function detectPhaseNumberFromFiles(files: string[] | undefined): string | null { + if (!files || files.length === 0) return null; + // A phase directory lives one segment below a `phases` parent segment: + // .planning/phases//… + // .planning/milestones/v1.0-phases//… + // The segment immediately after the `…phases` segment is the phase directory + // name. extractPhaseToken owns the project-code-aware token read. + for (const file of files) { + const norm = String(file).replace(/\\/g, '/').replace(/^\.\//, ''); + const segments = norm.split('/'); + for (let i = 0; i < segments.length - 1; i++) { + if (segments[i] === 'phases' || segments[i].endsWith('-phases')) { + const phaseDir = segments[i + 1]; + if (!phaseDir) continue; + const token = extractPhaseToken(phaseDir); + // extractPhaseToken falls back to returning dirName unchanged when no + // numeric token is found. normalizePhaseName is the canonical arbiter + // of "is this a real phase token": it strips the project-code prefix + // and returns a zero-padded numeric form for a genuine phase token, or + // the input unchanged otherwise. Accept the token only when it + // normalizes to a numeric phase form (the single-owner rule shared by + // every other phase-token reader — see #2528). + const normalized = normalizePhaseName(token); + // Built from the single-owner PHASE_NUMBER_TOKEN_SOURCE (the canonical + // phase-number grammar — #2128 anti-divergence guard) so this read-side + // acceptance check cannot drift from every other phase-token reader. + const phaseTokenShape = new RegExp(`^${PHASE_NUMBER_TOKEN_SOURCE}$`, 'i'); + if (token !== phaseDir && phaseTokenShape.test(normalized)) { + return token; + } + } + } + } + return null; +} + function cmdCommit(cwd: string, message: string | undefined, files: string[] | undefined, raw: boolean, amend: boolean, noVerify: boolean): void { if (!message && !amend) { error('commit message required'); @@ -776,10 +828,21 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u if (branchingStrategy && branchingStrategy !== 'none') { let branchName: string | null = null; if (branchingStrategy === 'phase') { - // Determine which phase we're committing for from the file paths - const phaseMatch = (files || []).join(' ').match(/(\d+(?:\.\d+)*)-/); - if (phaseMatch) { - const phaseNum = phaseMatch[1]; + // Determine which phase we're committing for from the file paths. + // #2539: the extraction is anchored to the directory SEGMENT immediately + // under `.planning/phases/` (or `.planning/milestones/-phases/`) and + // runs through the project-code-aware extractPhaseToken helper, NOT a + // free unanchored regex. The prior `match(/(\d+(?:\.\d+)*)-/)` returned + // the leftmost digit-run-then-hyphen anywhere in the joined path, so a + // project_code ending in a digit (PROJECT_V2) made `.../PROJECT_V2-07-…` + // match the `2-` inside `V2-` before the real `07-` phase token — + // resolving phase "2" instead of phase "7" and silently checking out the + // wrong branch. extractPhaseToken already owns project-code-aware phase- + // token parsing (it is the single owner shared by the other 6 call sites + // — see #2528 for the parallel drift problem in phase-locator/phase), + // so this is the canonical path-segment-bound read, not a fourth copy. + const phaseNum = detectPhaseNumberFromFiles(files); + if (phaseNum) { const phaseInfo = findPhaseInternal(cwd, phaseNum) as Record | null; if (phaseInfo) { branchName = (config['phase_branch_template'] as string) @@ -798,10 +861,25 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u if (branchName) { const currentBranch = execGit(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd }); if (currentBranch.exitCode === 0 && currentBranch.stdout.trim() !== branchName) { - // Create branch if it doesn't exist, or switch to it if it does + // #2539: the #1278 intent is to CREATE the phase/milestone branch + // before the FIRST commit on it — not to force-switch an already- + // checked-out working branch onto a DIFFERENT existing branch. The + // prior fallback to a bare `git checkout ` silently switched + // the whole working tree onto an existing unrelated branch in the same + // call that then committed (the only trace was a reflog entry). So: + // create-if-absent only. If the resolved branch already exists and the + // tree is on some other branch, do NOT switch — but never silently: log + // the resolution so the operator sees that the phase branch was + // resolved and deliberately not switched to (#2539 AC2: an auto- + // checkout mid-commit must never happen silently). const create = execGit(['checkout', '-b', branchName], { cwd }); if (create.exitCode !== 0) { - execGit(['checkout', branchName], { cwd }); + // `git checkout -b` fails (non-zero) when the branch already exists. + // The operator is on the branch they intend to be on; commit there. + process.stderr.write( + `Warning: resolved ${branchingStrategy} branch "${branchName}" already exists; ` + + `committing on the current branch "${currentBranch.stdout.trim()}" instead of switching.\n` + ); } } } diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index ed2cc7764..d367f6a96 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -1303,7 +1303,7 @@ describe('resolve-model command', () => { describe('commit command', () => { const { createTempGitProject } = require('./helpers.cjs'); - const { execSync } = require('child_process'); + const { execSync, execFileSync } = require('child_process'); let tmpDir; beforeEach(() => { @@ -1490,6 +1490,154 @@ describe('commit command', () => { const branch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(); assert.strictEqual(branch, 'gsd/phase-45.14-golden-capture', 'should be on decimal phase branch, not integer-only'); }); + + // #2539: the phase-token extraction must be anchored to the path segment under + // .planning/phases/ and reuse the project-code-aware extractPhaseToken helper. + // The prior unanchored `match(/(\d+(?:\.\d+)*)-/)` matched the leftmost + // digit-run-then-hyphen anywhere in the joined file path, so a project_code + // ending in a digit (e.g. PROJECT_V2) made `.../PROJECT_V2-07-name/...` match + // the `2-` inside `V2-` BEFORE reaching the real `07-` phase token — + // resolving phase "2" instead of phase "7". findPhaseInternal also searches + // archived milestones, so an existing archived phase 2 produced a real branch + // name, and the silent `git checkout ` fallback switched the + // whole working tree onto the wrong branch in the same call that then + // committed. This fixture reproduces both preconditions. + test('#2539: digit-suffixed project_code does not collide with the phase number', () => { + // Configure phase branching strategy with a project_code ending in a digit. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ + commit_docs: true, + project_code: 'PROJECT_V2', + branching_strategy: 'phase', + phase_branch_template: 'gsd/phase-{phase}-{slug}', + }) + ); + + // Archived phase 02 under a shipped milestone — the collision target that + // findPhaseInternal reaches via the .planning/milestones/-phases/ search. + fs.mkdirSync( + path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', 'PROJECT_V2-02-archived-phase'), + { recursive: true } + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', 'PROJECT_V2-02-archived-phase', '02-CONTEXT.md'), + '# Archived\n' + ); + + // Active phase 07 — the phase actually being committed. + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', 'PROJECT_V2-07-active-phase'), { recursive: true }); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## Phase 7: Active Phase\nGoal: ship it\n' + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'phases', 'PROJECT_V2-07-active-phase', '07-CONTEXT.md'), + '# Context\n' + ); + + const result = runGsdTools( + 'commit "docs(07): add context" --files .planning/phases/PROJECT_V2-07-active-phase/07-CONTEXT.md', + tmpDir + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.committed, true, 'should have committed'); + + // The commit must land on the phase-07 branch. Pre-fix this resolved the + // `2-` in `PROJECT_V2-` and silently switched onto the archived phase-02 + // branch instead. + const branch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(); + assert.strictEqual( + branch, + 'gsd/phase-07-active-phase', + `should be on the active phase-07 branch, not the archived phase-02 branch (got ${branch})` + ); + + // The committed file must exist on the phase-07 branch's HEAD, proving the + // commit did not silently land on the wrong branch. + const committedFile = execFileSync( + 'git', + ['show', 'HEAD:.planning/phases/PROJECT_V2-07-active-phase/07-CONTEXT.md'], + { cwd: tmpDir, encoding: 'utf-8' } + ); + assert.ok(committedFile.includes('# Context'), 'phase-07 file must be in the commit'); + }); + + // #2539 second defect: an auto-checkout mid-commit must never be silent. The + // #1278 intent was to CREATE the phase branch before the FIRST commit on it — + // not to force-switch an already-checked-out working branch onto a different + // existing branch. If the resolved phase branch already exists and the working + // tree is on some other branch, switching to it silently is the dangerous + // drift; the fix keeps create-if-absent but drops the silent switch-to-existing. + test('#2539: does not silently switch onto an existing unrelated phase branch', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ + commit_docs: true, + branching_strategy: 'phase', + phase_branch_template: 'gsd/phase-{phase}-{slug}', + }) + ); + // Active phase 01. + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-first-phase'), { recursive: true }); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## Phase 1: First Phase\nGoal: start\n' + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'phases', '01-first-phase', '01-CONTEXT.md'), + '# Context\n' + ); + + // Pre-create the phase-01 branch and check it out, then return to the + // default branch so the working tree is NOT on the phase branch when commit + // runs. The resolved branch already exists; the pre-fix code silently + // switched onto it. + execFileSync('git', ['branch', 'gsd/phase-01-first-phase'], { cwd: tmpDir, stdio: 'pipe' }); + // Ensure the file is staged only by the commit command itself (it must run + // from the current/default branch and must not be force-switched). + const beforeBranch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: tmpDir, encoding: 'utf-8', + }).trim(); + + // Invoke gsd-tools via spawnSync so stderr is observable on the success + // path — the warning that proves the no-switch path is not silent (#2539 + // AC2) is written to stderr, which execFileSync discards on success. + const { TOOLS_PATH } = require('./helpers.cjs'); + const { spawnSync } = require('child_process'); + const proc = spawnSync(process.execPath, [ + TOOLS_PATH, 'commit', 'docs(01): add context', + '--files', '.planning/phases/01-first-phase/01-CONTEXT.md', + ], { cwd: tmpDir, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }); + const stdout = proc.stdout || ''; + const stderr = proc.stderr || ''; + if (proc.status !== 0) { + throw new Error(`gsd-tools commit exited ${proc.status}: stdout=${stdout} stderr=${stderr}`); + } + + const output = JSON.parse(stdout.trim()); + assert.strictEqual(output.committed, true, 'should have committed'); + + // The command must NOT have silently switched the working tree onto the + // pre-existing phase branch. The commit lands on the branch we were on. + const afterBranch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: tmpDir, encoding: 'utf-8', + }).trim(); + assert.strictEqual( + afterBranch, + beforeBranch, + `must not silently switch onto an existing phase branch mid-commit (was ${beforeBranch}, now ${afterBranch})` + ); + + // #2539 AC2: the no-switch path must not be silent either. The warning + // surfaces the resolved branch and the branch the commit actually lands on. + assert.ok( + /Warning: resolved phase branch .* already exists/.test(stderr), + `expected a non-silent warning on stderr when the resolved branch already exists; got stderr=${stderr}` + ); + }); }); // ─────────────────────────────────────────────────────────────────────────────