diff --git a/.changeset/gallant-wolves-romp.md b/.changeset/gallant-wolves-romp.md new file mode 100644 index 000000000..b92b0314a --- /dev/null +++ b/.changeset/gallant-wolves-romp.md @@ -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) diff --git a/src/commands.cts b/src/commands.cts index 90a00eb12..c9e27d1b3 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -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 { diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index a03efb6b9..685a65f32 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -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'); + }); +});