golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 8 Packages Projects Releases Wiki Activity

fix(#2539): anchor commit phase-token detection; drop silent wrong-branch switch (#2669)

Browse Source 
* 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 <existing-branch>` 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/<v>-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
This commit is contained in:
Tom Boucher
2026-07-26 14:14:28 -04:00
committed by GitHub
parent 46ba02acde
commit 115433bba6
3 changed files with 239 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/2539-commit-phase-detection-anchored.md Normal file
View File

@@ -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)

92
src/commands.cts
View File

@@ -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/<version>-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/<phase-dir>/…
// .planning/milestones/v1.0-phases/<phase-dir>/…
// 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/<v>-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<string, unknown> | 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 <branch>` 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`
);
}
}
}

150
tests/commands.test.cjs
View File

@@ -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 <existing-branch>` 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/<v>-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}`
);
});
});
// ─────────────────────────────────────────────────────────────────────────────