diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index b7774a264..37029463b 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -217,9 +217,34 @@ Check `branching_strategy` from init: **"none":** Skip, continue on current branch. -**"phase" or "milestone":** Use pre-computed `branch_name` from init: +**"phase" or "milestone":** Use pre-computed `branch_name` from init. + +The new phase branch must fork off the project's default branch (`origin/HEAD`), +not off whatever HEAD happens to be checked out — otherwise consecutive phases +compound on top of each other and stay unpushed (#2916). If `$BRANCH_NAME` +already exists locally, reuse it as-is so resumed work is not rebased. + ```bash -git checkout -b "$BRANCH_NAME" 2>/dev/null || git checkout "$BRANCH_NAME" +DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') +DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} + +if git show-ref --verify --quiet "refs/heads/$BRANCH_NAME"; then + git switch "$BRANCH_NAME" +else + if [ -n "$(git status --porcelain)" ]; then + echo "WARNING: Uncommitted changes present. Commit or stash before starting a new phase so it branches off $DEFAULT_BRANCH cleanly. Falling back to current HEAD as base." + git checkout -b "$BRANCH_NAME" + else + git fetch --quiet origin "$DEFAULT_BRANCH" 2>/dev/null || true + git switch "$DEFAULT_BRANCH" 2>/dev/null && git merge --ff-only "origin/$DEFAULT_BRANCH" 2>/dev/null + git checkout -b "$BRANCH_NAME" + fi +fi + +INHERITED=$(git rev-list --count "${DEFAULT_BRANCH}..HEAD" 2>/dev/null || echo "?") +if [ "$INHERITED" != "0" ] && [ "$INHERITED" != "?" ]; then + echo "WARNING: Phase branch '$BRANCH_NAME' contains $INHERITED commit(s) inherited from a non-default base. Verify this is intentional before continuing." +fi ``` All subsequent commits go to this branch. User handles merging. diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index f8a93c00b..6ea3bb780 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -180,10 +180,34 @@ Quick tasks can run mid-phase - validation only checks ROADMAP.md exists, not ph **If `branch_name` is empty/null:** Skip and continue on the current branch. -**If `branch_name` is set:** Check out the quick-task branch before any planning commits: +**If `branch_name` is set:** Check out the quick-task branch before any planning commits. + +The new branch must fork off the project's default branch (`origin/HEAD`), not +off whatever HEAD happens to be checked out — otherwise consecutive quick tasks +compound on top of each other and stay unpushed (#2916). If `$branch_name` +already exists locally, reuse it as-is so resumed work is not rebased. ```bash -git checkout -b "$branch_name" 2>/dev/null || git checkout "$branch_name" +DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') +DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} + +if git show-ref --verify --quiet "refs/heads/$branch_name"; then + git switch "$branch_name" +else + if [ -n "$(git status --porcelain)" ]; then + echo "WARNING: Uncommitted changes present. Commit or stash before starting a new quick task so it branches off $DEFAULT_BRANCH cleanly. Falling back to current HEAD as base." + git checkout -b "$branch_name" + else + git fetch --quiet origin "$DEFAULT_BRANCH" 2>/dev/null || true + git switch "$DEFAULT_BRANCH" 2>/dev/null && git merge --ff-only "origin/$DEFAULT_BRANCH" 2>/dev/null + git checkout -b "$branch_name" + fi +fi + +INHERITED=$(git rev-list --count "${DEFAULT_BRANCH}..HEAD" 2>/dev/null || echo "?") +if [ "$INHERITED" != "0" ] && [ "$INHERITED" != "?" ]; then + echo "WARNING: Quick-task branch '$branch_name' contains $INHERITED commit(s) inherited from a non-default base. Verify this is intentional before continuing." +fi ``` All quick-task commits for this run stay on that branch. User handles merge/rebase afterward. diff --git a/tests/bug-2916-handle-branching-default-base.test.cjs b/tests/bug-2916-handle-branching-default-base.test.cjs new file mode 100644 index 000000000..bfaca64f6 --- /dev/null +++ b/tests/bug-2916-handle-branching-default-base.test.cjs @@ -0,0 +1,235 @@ +/** + * Regression test for #2916: execute-phase `handle_branching` step creates the + * per-phase branch off whatever HEAD is currently checked out (typically the + * previous phase's unmerged branch) instead of off `origin/HEAD`. + * + * The bug compounded phases on top of each other and stranded them unpushed + * for weeks. The fix: + * 1. Detect the default branch via `git symbolic-ref refs/remotes/origin/HEAD`. + * 2. If $BRANCH_NAME exists, switch to it (preserve existing behavior). + * 3. Otherwise, ff-update the default branch from origin and create the new + * phase branch off the default-branch tip. + * 4. Refuse-or-warn on dirty working tree. + * 5. Post-creation, assert `git rev-list --count $DEFAULT_BRANCH..HEAD == 0`. + * + * This test extracts the bash payload from the + * block in execute-phase.md (parsed structurally — no regex on prose), executes + * it inside a fixture git repo where HEAD sits on a previous-phase branch with + * extra commits, and asserts that the new phase branch's tip equals + * `origin/main` (no commits inherited from the previous phase). + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const EXECUTE_PHASE_PATH = path.join( + __dirname, + '..', + 'get-shit-done', + 'workflows', + 'execute-phase.md' +); + +const GIT_ENV = Object.freeze({ + ...process.env, + GIT_AUTHOR_NAME: 'Test', + GIT_AUTHOR_EMAIL: 'test@test.com', + GIT_COMMITTER_NAME: 'Test', + GIT_COMMITTER_EMAIL: 'test@test.com', +}); + +function git(cwd, ...args) { + return execFileSync('git', args, { + cwd, + env: GIT_ENV, + stdio: ['pipe', 'pipe', 'pipe'], + }) + .toString() + .trim(); +} + +/** + * Structurally extract the bash code that the handle_branching step instructs + * the agent to run. We: + * 1. Locate the ... block. + * 2. Walk its body looking for fenced ```bash blocks. + * 3. Concatenate every bash block in the step (the fix may use more than one). + * + * No `.includes()` content checks — we parse fence-delimited code blocks the + * same way a markdown parser would. + */ +function extractHandleBranchingBash() { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + const lines = content.split(/\r?\n/); + + let start = -1; + let end = -1; + for (let i = 0; i < lines.length; i += 1) { + if (start === -1 && /^\s*$/.test(lines[i])) { + start = i + 1; + } else if (start !== -1 && /^<\/step>\s*$/.test(lines[i])) { + end = i; + break; + } + } + if (start === -1 || end === -1) { + throw new Error( + 'execute-phase.md does not contain a ... block' + ); + } + + const bashBlocks = []; + let inBash = false; + let buffer = []; + for (let i = start; i < end; i += 1) { + const line = lines[i]; + if (!inBash && /^```bash\s*$/.test(line)) { + inBash = true; + buffer = []; + continue; + } + if (inBash && /^```\s*$/.test(line)) { + bashBlocks.push(buffer.join('\n')); + inBash = false; + continue; + } + if (inBash) buffer.push(line); + } + if (bashBlocks.length === 0) { + throw new Error( + 'handle_branching step contains no ```bash code blocks to execute' + ); + } + return bashBlocks.join('\n'); +} + +/** + * Build a fixture: a bare "origin" repo with `main` (one commit), a clone with + * `origin/HEAD` pointed at `main`, and a checked-out previous-phase branch + * carrying its own unmerged commit. Returns the clone path. + */ +function setupFixture() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2916-')); + const seedPath = path.join(root, 'seed'); + const originPath = path.join(root, 'origin.git'); + const clonePath = path.join(root, 'clone'); + + fs.mkdirSync(seedPath); + git(seedPath, 'init', '-b', 'main'); + git(seedPath, 'config', 'commit.gpgsign', 'false'); + fs.writeFileSync(path.join(seedPath, 'README.md'), '# seed\n'); + git(seedPath, 'add', 'README.md'); + git(seedPath, 'commit', '-m', 'initial'); + + git(root, 'clone', '--bare', seedPath, originPath); + git(originPath, 'symbolic-ref', 'HEAD', 'refs/heads/main'); + + git(root, 'clone', originPath, clonePath); + git(clonePath, 'config', 'commit.gpgsign', 'false'); + git(clonePath, 'config', 'user.email', 'test@test.com'); + git(clonePath, 'config', 'user.name', 'Test'); + + // Simulate finishing a previous phase: branch off main, add a commit, and + // *stay* on it (this is the failure scenario described in the bug). + git(clonePath, 'checkout', '-b', 'feature/phase-01-foundation'); + fs.writeFileSync(path.join(clonePath, 'phase01.txt'), 'phase 1 work\n'); + git(clonePath, 'add', 'phase01.txt'); + git(clonePath, 'commit', '-m', 'phase 01 work'); + + return { root, clonePath }; +} + +function runHandleBranchingStep(bash, cwd, branchName) { + // Write the script to a sibling tempdir, not inside the repo — putting it in + // `cwd` would create an untracked file that trips `git status --porcelain` + // and steers the step into its dirty-tree fallback path. + const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2916-step-')); + const scriptPath = path.join(scriptDir, 'handle-branching.sh'); + const script = `#!/usr/bin/env bash\nset -uo pipefail\nBRANCH_NAME="${branchName}"\n${bash}\n`; + fs.writeFileSync(scriptPath, script, { mode: 0o755 }); + try { + return execFileSync('bash', [scriptPath], { + cwd, + env: GIT_ENV, + stdio: ['pipe', 'pipe', 'pipe'], + }).toString(); + } finally { + fs.rmSync(scriptDir, { recursive: true, force: true }); + } +} + +describe('handle_branching branches off origin/HEAD, not current HEAD (#2916)', () => { + test('new phase branch contains 0 commits inherited from previous-phase HEAD', () => { + const bash = extractHandleBranchingBash(); + const { root, clonePath } = setupFixture(); + + try { + // Sanity: we begin sitting on the previous phase branch, 1 ahead of origin/main. + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-01-foundation' + ); + assert.equal( + git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'), + '1', + 'fixture should be 1 commit ahead of origin/main' + ); + + runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); + + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-02-content-sync', + 'handle_branching should switch to the new phase branch' + ); + + const inherited = git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'); + assert.equal( + inherited, + '0', + `new phase branch must branch off origin/main, but inherited ${inherited} commit(s) from previous-phase HEAD` + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + git(clonePath, 'rev-parse', 'origin/main'), + 'new phase branch tip must equal origin/main tip' + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + test('handle_branching reuses an existing branch instead of forking again', () => { + const bash = extractHandleBranchingBash(); + const { root, clonePath } = setupFixture(); + + try { + // Pre-create the target branch off origin/main with its own commit, then + // walk away to a different branch — the step must switch back to it. + git(clonePath, 'checkout', '-B', 'feature/phase-02-content-sync', 'origin/main'); + fs.writeFileSync(path.join(clonePath, 'phase02.txt'), 'phase 2 work\n'); + git(clonePath, 'add', 'phase02.txt'); + git(clonePath, 'commit', '-m', 'phase 02 wip'); + const phase02Sha = git(clonePath, 'rev-parse', 'HEAD'); + git(clonePath, 'checkout', 'feature/phase-01-foundation'); + + runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); + + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-02-content-sync' + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + phase02Sha, + 'existing-branch tip must be preserved (no rebase/reset)' + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/tests/quick-branching.test.cjs b/tests/quick-branching.test.cjs index ff15c55c2..5697e4f13 100644 --- a/tests/quick-branching.test.cjs +++ b/tests/quick-branching.test.cjs @@ -26,7 +26,20 @@ describe('quick workflow: branching support', () => { test('workflow includes quick-task branching step', () => { content = fs.readFileSync(workflowPath, 'utf-8'); assert.ok(content.includes('Step 2.5: Handle quick-task branching')); - assert.ok(content.includes('git checkout -b "$branch_name" 2>/dev/null || git checkout "$branch_name"')); + // Branching block must (a) honour the existing branch if present and + // (b) create new branches off origin/HEAD, not current HEAD (#2916). + assert.ok( + content.includes('git switch "$branch_name"'), + 'should reuse existing branch via git switch' + ); + assert.ok( + content.includes('refs/remotes/origin/HEAD'), + 'should detect default branch from origin/HEAD instead of branching off current HEAD' + ); + assert.ok( + content.includes('git checkout -b "$branch_name"'), + 'should create new branch via git checkout -b after switching to default' + ); }); test('branching step runs before task directory creation', () => {