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

fix(#4055): verify a phase branch is genuinely new before create-and-switch (#4694)

Browse Source 
* test(#4055): full-lifecycle regression for merged-branch resurrection

* fix(#4055): verify a phase branch is genuinely new before create-and-switch

* test(#4055): assert branch absence via the gitOrThrow throw contract

* test(#4055): observe the refusal disclosure via the process seam

* chore(#4055): add changeset fragment

* test(#4055): drop an unused fixture variable

* fix(#4055): name the milestone-arm residual and the guard degradations

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-09-13 10:48:34 -04:00
committed by GitHub
parent 4f487e4e75
commit a155ff4fb6
3 changed files with 205 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/gallant-wolves-romp.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4694
---
**A merged-and-deleted phase branch is no longer resurrected by a post-merge phase-scoped commit** — `query commit` re-created the deleted branch and moved HEAD onto it (the #3079 hijack reopened by #3363); the create arm now requires a genuinely new phase (no committed history touching the phase directory, caller on the resolved base branch) and otherwise commits in place with a disclosed warning. and refusing to recreate an absent phase branch when the caller is off the resolved base branch. The milestone arm keeps its existence-only guard in this fix (its state-3 exposure is unchanged and named at the guard site) but now also requires the base branch before creating. (#4055)

85
src/commands.cts
View File

@@ -1985,6 +1985,10 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
const branchingStrategy = config['branching_strategy'] as string | undefined;
if (branchingStrategy && branchingStrategy !== 'none') {
let branchName: string | null = null;
// #4055: the phase directory (cwd-relative POSIX path from
// findPhaseInternal) captured while resolving the phase identity — the
// state-3 guard below needs it for the committed-history check.
let phaseDirRelative: string | null = null;
if (branchingStrategy === 'phase') {
// Determine which phase we're committing for from the file paths.
// #2539: the extraction is anchored to the directory SEGMENT immediately
@@ -2015,6 +2019,10 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
phaseInfo['phase_number'],
phaseInfo['phase_slug'],
);
// #4055: findPhaseInternal already returns the directory as a
// cwd-relative POSIX path.
const dir = phaseInfo['directory'];
if (typeof dir === 'string' && dir !== '') phaseDirRelative = dir;
}
}
} else if (branchingStrategy === 'milestone') {
@@ -2038,6 +2046,14 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
}
}
if (branchName) {
// #4055: state-3 discriminator for the create arm. `rev-parse --verify`
// alone cannot distinguish "branch never existed" (create is the #1278
// intent) from "branch existed, was merged, then deleted" (the phase is
// over — recreating it hijacks the close-out commit onto a resurrected
// ref, the #3079 bug #3363 reopened). Both extra conditions come from
// the confirmed issue: the create arm may fire only for a phase whose
// directory has NO committed history on the current line (a genuinely
// new phase) while the caller sits on the resolved base branch.
const currentBranch = execGit(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd });
if (currentBranch.exitCode === 0 && currentBranch.stdout.trim() !== branchName) {
// #2539/#3079/#3207: two cases the prior (#3079) code collapsed into one.
@@ -2052,20 +2068,71 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
// EXISTING branch is never switched to (the else arm logs + commits in
// place). The fresh create is logged so the first phase-scoped commit is
// not silent about where the work is landing (#3207 AC3).
// #4055: "brand-new" is now VERIFIED, not assumed — see the state-3
// guard between the verify and the create below.
const verify = execGit(['rev-parse', '--verify', `refs/heads/${branchName}`], { cwd });
if (verify.exitCode !== 0) {
// Branch does not exist — CREATE AND SWITCH (the #1278 first-commit
// case). checkout -b cannot resurrect anything: the branch was just
// verified absent, so it is created fresh at HEAD.
const create = execGit(['checkout', '-b', branchName], { cwd });
if (create.exitCode === 0) {
process.stderr.write(
`${branchingStrategy} branch "${branchName}" created; switched to it for this commit.\n`
// Branch does not exist — but absence alone cannot distinguish a
// genuinely new phase from a merged-and-deleted one (#4055).
let createBlockReason: string | null = null;
if (branchingStrategy === 'phase' && phaseDirRelative) {
// #4055 residual: searchPhaseInDir's #2237 fail-safe can return an
// empty `directory` for ambiguous phase names (leaving
// phaseDirRelative null) — there the history half is skipped and
// only the base check below guards; shallow clones can also show
// an empty probe for old merged phases (depth-sensitive).
const history = execGit(
['log', 'HEAD', '--oneline', '--', phaseDirRelative],
{ cwd },
);
if (history.exitCode === 0 && history.stdout.trim() !== '') {
createBlockReason =
'its phase directory already has committed history (the phase is resolved)';
}
}
if (!createBlockReason) {
// The base half of the guard applies to BOTH strategies (it does
// not need a directory): a phase/milestone branch is created only
// from the resolved base branch. NOTE the milestone arm keeps its
// existence-only guard for the HISTORY half — a merged-and-deleted
// milestone branch remains resurrectable by an on-base caller
// until a milestone-directory derivation exists here (#4055
// follow-up candidate).
/* eslint-disable @typescript-eslint/no-require-imports */
const gitBaseBranch = require('./git-base-branch.cjs') as {
resolveBaseBranch: (cwd: string) => string;
};
/* eslint-enable @typescript-eslint/no-require-imports */
const resolvedBase = gitBaseBranch.resolveBaseBranch(cwd);
if (resolvedBase && resolvedBase !== currentBranch.stdout.trim()) {
createBlockReason =
`the current branch "${currentBranch.stdout.trim()}" is not the ` +
`resolved base branch "${resolvedBase}"`;
}
}
if (createBlockReason === null) {
// State 1 confirmed: brand-new phase, first phase-scoped commit
// from the base branch. CREATE AND SWITCH (the #1278 first-commit
// case). checkout -b cannot resurrect anything: the branch was
// just verified absent, so it is created fresh at HEAD.
const create = execGit(['checkout', '-b', branchName], { cwd });
if (create.exitCode === 0) {
process.stderr.write(
`${branchingStrategy} branch "${branchName}" created; switched to it for this commit.\n`
);
} else {
process.stderr.write(
`Warning: could not create ${branchingStrategy} branch "${branchName}" ` +
`(${create.stderr.trim()}); committing on the current branch "${currentBranch.stdout.trim()}".\n`
);
}
} else {
// State 3 (or a non-base caller): the phase is resolved — commit
// in place, disclosed (#2539 AC2), never recreate the branch.
process.stderr.write(
`Warning: could not create ${branchingStrategy} branch "${branchName}" ` +
`(${create.stderr.trim()}); committing on the current branch "${currentBranch.stdout.trim()}".\n`
`Warning: resolved ${branchingStrategy} branch "${branchName}" is absent and ` +
`will not be recreated (${createBlockReason}); committing on the current ` +
`branch "${currentBranch.stdout.trim()}" instead of recreating it.\n`
);
}
} else {

124
tests/commands.test.cjs
View File

@@ -6545,3 +6545,127 @@ describe('gsd-tools.cjs resolveMainWorktreeCwd (#3050)', () => {
assert.equal(resolved, '/repo/wt');
});
});
// ─── #4055 — a merged-and-deleted phase branch must not be resurrected ──────
describe('#4055: merged-and-deleted phase branch must not be resurrected', () => {
const { createTempGitProject } = require('./helpers.cjs');
test('post-merge phase-scoped commit lands on the current branch', () => {
const tmpDir = createTempGitProject('gsd-4055-lifecycle-');
const base = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim();
// Configure phase branching (the issue's config shape).
fs.writeFileSync(
path.join(tmpDir, '.planning', 'config.json'),
JSON.stringify({
commit_docs: true,
branching_strategy: 'phase',
phase_branch_template: 'gsd/phase-{phase}-{slug}',
})
);
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '07-example-phase'), { recursive: true });
fs.writeFileSync(
path.join(tmpDir, '.planning', 'phases', '07-example-phase', '07-PLAN.md'),
'---\nphase: 07-example-phase\nplan: 01\n---\n# Plan\n'
);
gitOrThrow(['add', '-A'], { cwd: tmpDir });
gitOrThrow(['commit', '-m', 'chore: seed phase 07'], { cwd: tmpDir });
// Normal phase lifecycle: branch, work, merge, delete the branch.
gitOrThrow(['checkout', '-qb', 'gsd/phase-07-example-phase'], { cwd: tmpDir });
fs.writeFileSync(
path.join(tmpDir, '.planning', 'phases', '07-example-phase', '07-01-SUMMARY.md'),
'summary\n'
);
gitOrThrow(['add', '-A'], { cwd: tmpDir });
gitOrThrow(['commit', '-m', 'docs(07-01): summary'], { cwd: tmpDir });
gitOrThrow(['checkout', '-q', base], { cwd: tmpDir });
gitOrThrow(['merge', '-q', '--no-ff', '-m', 'Phase 07 (#1)', 'gsd/phase-07-example-phase'], { cwd: tmpDir });
gitOrThrow(['branch', '-qD', 'gsd/phase-07-example-phase'], { cwd: tmpDir });
assert.strictEqual(
gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(),
base,
'lifecycle setup: must be back on the base branch post-merge'
);
// The ordinary post-merge close-out commit.
fs.writeFileSync(
path.join(tmpDir, '.planning', 'phases', '07-example-phase', '07-VERIFICATION.md'),
'verification\n'
);
// Invoke via the process seam so stderr is observable on the success
// path — the refusal disclosure (#2539 AC2) is written to stderr, which
// execFileSync discards on success (same idiom as the #2539 no-switch
// test above).
const { TOOLS_PATH } = require('./helpers.cjs');
const proc = runNode([
TOOLS_PATH, 'commit', 'docs(phase-07): verification report',
'--files', '.planning/phases/07-example-phase/07-VERIFICATION.md',
], { cwd: tmpDir });
throwIfFailed(proc, 'gsd-tools commit (post-merge close-out)');
const output = JSON.parse((proc.stdout || '').trim());
assert.strictEqual(output.committed, true, 'must commit');
// The fix: no resurrection, no switch — the commit lands in place.
assert.strictEqual(
gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(),
base,
'HEAD must stay on the base branch (no create-and-switch)'
);
// gitOrThrow throws on the expected absence (rev-parse --quiet exits 1) —
// the throw itself is the proof the branch was not recreated.
let resurrected = true;
try {
gitOrThrow(['rev-parse', '--verify', '--quiet', 'refs/heads/gsd/phase-07-example-phase'], { cwd: tmpDir });
} catch {
resurrected = false;
}
assert.strictEqual(resurrected, false, 'the deleted phase branch must not be recreated');
const landed = gitOrThrow(
['show', 'HEAD:.planning/phases/07-example-phase/07-VERIFICATION.md'], { cwd: tmpDir }
);
assert.ok(landed.includes('verification'), 'the commit must land on the base branch');
assert.match(
proc.stderr || '',
/instead of recreating/,
'the refusal must be disclosed on stderr (#2539 AC2)'
);
});
test('create arm requires the current branch to be the resolved base', () => {
const tmpDir = createTempGitProject('gsd-4055-base-');
fs.writeFileSync(
path.join(tmpDir, '.planning', 'config.json'),
JSON.stringify({
commit_docs: true,
branching_strategy: 'phase',
phase_branch_template: 'gsd/phase-{phase}-{slug}',
})
);
// A genuinely new phase (no committed history touches its directory) but
// the caller is NOT on the base branch — the create arm must not fire.
gitOrThrow(['checkout', '-qb', 'side-work'], { cwd: tmpDir });
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-next'), { recursive: true });
fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '02-next', '02-CONTEXT.md'), '# Context\n');
const result = runGsdTools(
'commit "docs(02): context" --files .planning/phases/02-next/02-CONTEXT.md',
tmpDir
);
assert.ok(result.success, `commit failed: ${result.error || result.output}`);
assert.strictEqual(
gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(),
'side-work',
'HEAD must stay on the non-base branch (no create-and-switch)'
);
let createdBranch = true;
try {
gitOrThrow(['rev-parse', '--verify', '--quiet', 'refs/heads/gsd/phase-02-next'], { cwd: tmpDir });
} catch {
createdBranch = false;
}
assert.strictEqual(createdBranch, false, 'no phase branch may be created off a non-base branch');
});
});