From 294564b951bc1f93f0d96487b2c65b09fa15d939 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 17:30:52 -0400 Subject: [PATCH 1/8] fix(#2916): branch new phases off origin/HEAD instead of current HEAD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handle_branching in execute-phase.md (and the equivalent step in quick.md) created the per-phase branch from whatever branch happened to be checked out — typically the previous phase's still-unmerged feature branch — so consecutive phases compounded on top of each other and stayed unpushed. Detect the default branch via git symbolic-ref refs/remotes/origin/HEAD, fast-forward it from origin, and fork the new phase branch off that tip. Existing branches are still reused as-is. Dirty working trees fall back to current HEAD with a loud warning, and a post-creation guard reports any inherited commits. Regression test extracts the bash from the block structurally and runs it against a fixture repo where HEAD sits on a previous-phase branch with extra commits. --- get-shit-done/workflows/execute-phase.md | 29 ++- get-shit-done/workflows/quick.md | 28 ++- ...916-handle-branching-default-base.test.cjs | 235 ++++++++++++++++++ tests/quick-branching.test.cjs | 15 +- 4 files changed, 302 insertions(+), 5 deletions(-) create mode 100644 tests/bug-2916-handle-branching-default-base.test.cjs 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', () => { From 4e2f1105d95b14a34490ad3ac92cb024c9ba00cb Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:22:44 -0400 Subject: [PATCH 2/8] fix(#2916): pin new-branch base to origin/$DEFAULT_BRANCH explicitly Address CodeRabbit HIGH findings on PR #2921. The previous fix had three unconditional code paths where `git checkout -b "$BRANCH_NAME"` would run from the *current* HEAD when the upstream sync failed silently: - the dirty-tree warn-and-continue path, - the clean path where `git switch` / `git merge --ff-only` errors were swallowed by `2>/dev/null` (still falling through to checkout -b), - any case where `git fetch` failed but the script continued. This rewrites both `execute-phase.md` (handle_branching) and `quick.md` (Step 2.5) to: 1. Fetch origin/$DEFAULT_BRANCH; if fetch fails AND no local copy of origin/$DEFAULT_BRANCH exists, abort with a clear ERROR (exit 1) rather than create the branch off arbitrary HEAD. 2. Always create the new branch with an explicit start point: `git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH"`. The base is now deterministic regardless of which branch is currently checked out, regardless of whether the optional local fast-forward succeeded, and regardless of dirty-tree state. 3. Carry uncommitted changes onto the new (origin-pinned) branch instead of inheriting the previous-phase HEAD as a fallback base. The post-creation INHERITED check now references origin/$DEFAULT_BRANCH rather than the (possibly-stale) local default branch, so the warning fires accurately even when the local fast-forward was skipped. --- get-shit-done/workflows/execute-phase.md | 26 +++++++++--------- get-shit-done/workflows/quick.md | 34 ++++++++++++++++++------ 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 37029463b..1e2bd3617 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -219,10 +219,7 @@ Check `branching_strategy` 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. +Fork the new phase branch off `origin/HEAD` (the project's default branch), not the current HEAD — otherwise consecutive phases compound and stay unpushed (#2916). If `$BRANCH_NAME` already exists locally, reuse it as-is. ```bash DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||') @@ -231,19 +228,22 @@ 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" + if ! git fetch --quiet origin "$DEFAULT_BRANCH"; then # #2916 + git show-ref --verify --quiet "refs/remotes/origin/$DEFAULT_BRANCH" \ + || { echo "ERROR: fetch origin/$DEFAULT_BRANCH failed and no local copy exists. Refusing to create '$BRANCH_NAME' off current HEAD (#2916)." >&2; exit 1; } + echo "WARNING: fetch origin/$DEFAULT_BRANCH failed; using local copy as base." >&2 fi + if [ -n "$(git status --porcelain)" ]; then + echo "WARNING: Uncommitted changes will be carried onto '$BRANCH_NAME' (branched off origin/$DEFAULT_BRANCH, not previous HEAD)." + else + git switch --quiet "$DEFAULT_BRANCH" 2>/dev/null && git merge --ff-only --quiet "origin/$DEFAULT_BRANCH" 2>/dev/null || true + fi + git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH" # pinned base (#2916) fi -INHERITED=$(git rev-list --count "${DEFAULT_BRANCH}..HEAD" 2>/dev/null || echo "?") +INHERITED=$(git rev-list --count "origin/${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." + echo "WARNING: Phase branch '$BRANCH_NAME' contains $INHERITED commit(s) inherited from a non-default base. Verify this is intentional." fi ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 6ea3bb780..6ced91b82 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -194,17 +194,35 @@ 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" + # Fetch the default branch so origin/$DEFAULT_BRANCH is current. If the fetch + # fails (offline, no remote, auth failure) AND we have no local copy of + # origin/$DEFAULT_BRANCH to fall back on, abort — creating the branch off + # arbitrary HEAD is exactly the bug #2916 fixed. + if ! git fetch --quiet origin "$DEFAULT_BRANCH"; then + if ! git show-ref --verify --quiet "refs/remotes/origin/$DEFAULT_BRANCH"; then + echo "ERROR: Could not fetch origin/$DEFAULT_BRANCH and no local copy exists. Refusing to create '$branch_name' off the current HEAD (#2916). Resolve the remote/network issue and retry." >&2 + exit 1 + fi + echo "WARNING: git fetch origin $DEFAULT_BRANCH failed; using the local copy of origin/$DEFAULT_BRANCH as base." >&2 fi + + if [ -n "$(git status --porcelain)" ]; then + echo "WARNING: Uncommitted changes present. Carrying them onto the new quick-task branch — they will be branched off origin/$DEFAULT_BRANCH (not the previous-task HEAD)." + else + # Best-effort: fast-forward the local default branch so subsequent local + # work sees the latest tip. Failure here is non-fatal because we always + # create the new branch directly from origin/$DEFAULT_BRANCH below. + git switch --quiet "$DEFAULT_BRANCH" 2>/dev/null \ + && git merge --ff-only --quiet "origin/$DEFAULT_BRANCH" 2>/dev/null \ + || true + fi + + # Always pin the new branch to origin/$DEFAULT_BRANCH so the start point is + # deterministic regardless of which branch we are currently on (#2916). + git checkout -b "$branch_name" "origin/$DEFAULT_BRANCH" fi -INHERITED=$(git rev-list --count "${DEFAULT_BRANCH}..HEAD" 2>/dev/null || echo "?") +INHERITED=$(git rev-list --count "origin/${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 From 140d334dab15a3aea2dc91a523deb8b008c92818 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:22:56 -0400 Subject: [PATCH 3/8] test(#2916): replace string-grep assertions with behavioral fixture test CodeRabbit nitpick (per project policy `feedback_no_source_grep_tests`): the prior `tests/quick-branching.test.cjs` asserted branching correctness by `.includes()`-grepping the raw markdown content for literal command substrings. Those assertions stayed green even when the underlying behavior regressed (e.g. when `git checkout -b` was unconditionally run from the wrong HEAD). Replace with the same pattern as `bug-2916-handle-branching-default-base .test.cjs`: - Structurally extract the Step 2.5 bash block from quick.md by walking the markdown for fenced ```bash blocks under the heading (no regex on prose). - Spin up a fixture git repo with a bare origin, a clone whose `origin/HEAD` points at `main`, and a checked-out previous-task branch carrying its own unmerged commit. - Execute the extracted bash block via `bash -c` and assert that the new branch's tip equals `origin/main` (0 commits inherited from the previous-task HEAD). - Add a reuse test that pre-creates the target branch with its own commit and verifies the script switches back to it without a rebase or reset. The two informational tests (workflow file exists, branching runs before task-directory creation) are retained, plus the `branch_name` parsing assertion is rewritten to walk fenced bash blocks rather than substring-grep arbitrary content. --- tests/quick-branching.test.cjs | 272 +++++++++++++++++++++++++++++---- 1 file changed, 241 insertions(+), 31 deletions(-) diff --git a/tests/quick-branching.test.cjs b/tests/quick-branching.test.cjs index 5697e4f13..4068029dd 100644 --- a/tests/quick-branching.test.cjs +++ b/tests/quick-branching.test.cjs @@ -1,52 +1,262 @@ /** * Quick task branching tests * - * Validates that /gsd-quick exposes branch_name from init and that the - * workflow checks out a dedicated quick-task branch when configured. + * Validates that /gsd-quick exposes branch_name from init and that the Step 2.5 + * "Handle quick-task branching" block: + * 1. Reuses an existing branch as-is (no rebase / no reset). + * 2. When the branch does not exist, creates it from origin/HEAD's default + * branch — never off the previous task's HEAD (#2916). + * + * Assertions are behavioral (run the bash block in a fixture git repo and + * inspect git state) and structural (parse the markdown for the step's bash + * block). No `.includes()` / regex grepping of raw markdown content — see + * CONTRIBUTING.md "no-source-grep" testing standard. */ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); -const fs = require('fs'); -const path = require('path'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const QUICK_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.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 under the "Step 2.5: Handle quick-task + * branching" heading. We: + * 1. Locate the Step 2.5 heading. + * 2. Find the next horizontal rule (`---`) that ends the section. + * 3. Concatenate every fenced ```bash block in between. + * + * No `.includes()` content checks — fenced code blocks are parsed the same way + * a markdown parser would. + */ +function extractStep25Bash() { + const content = fs.readFileSync(QUICK_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 && /^\*\*Step 2\.5:\s*Handle quick-task branching\*\*\s*$/.test(lines[i])) { + start = i + 1; + } else if (start !== -1 && /^---\s*$/.test(lines[i])) { + end = i; + break; + } + } + if (start === -1) { + throw new Error('quick.md does not contain a "Step 2.5: Handle quick-task branching" section'); + } + if (end === -1) end = lines.length; + + 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('Step 2.5 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-task branch + * carrying its own unmerged commit. + */ +function setupFixture() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-quick-branching-')); + 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 quick task: branch off main, add a commit, + // and stay on it (this is the failure scenario from #2916). + git(clonePath, 'checkout', '-b', 'quick/01-prev-task'); + fs.writeFileSync(path.join(clonePath, 'prev.txt'), 'prev work\n'); + git(clonePath, 'add', 'prev.txt'); + git(clonePath, 'commit', '-m', 'prev quick task work'); + + return { root, clonePath }; +} + +function runStep(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 the dirty-tree path. + const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-quick-step-')); + const scriptPath = path.join(scriptDir, 'step25.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('quick workflow: branching support', () => { - const workflowPath = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.md'); - let content; - test('workflow file exists', () => { - assert.ok(fs.existsSync(workflowPath), 'workflows/quick.md should exist'); + assert.ok(fs.existsSync(QUICK_PATH), 'workflows/quick.md should exist'); }); test('init parse list includes branch_name', () => { - content = fs.readFileSync(workflowPath, 'utf-8'); - assert.ok(content.includes('branch_name'), 'quick workflow should parse branch_name from init JSON'); + // Structural: the workflow's init JSON parsing must mention branch_name as + // a top-level field. We assert this by parsing the init step's bash blocks + // for the `branch_name` JSON path / variable rather than substring-grep. + const content = fs.readFileSync(QUICK_PATH, 'utf-8'); + const lines = content.split(/\r?\n/); + // Walk every fenced bash block in the file; look for an assignment that + // reads a `branch_name` field (jq, awk, or shell parameter). + let found = false; + let inBash = false; + for (const line of lines) { + if (!inBash && /^```bash\s*$/.test(line)) { inBash = true; continue; } + if (inBash && /^```\s*$/.test(line)) { inBash = false; continue; } + if (!inBash) continue; + if (/\bbranch_name\b/.test(line)) { found = true; break; } + } + assert.ok(found, 'quick workflow should expose branch_name inside a bash block'); }); - test('workflow includes quick-task branching step', () => { - content = fs.readFileSync(workflowPath, 'utf-8'); - assert.ok(content.includes('Step 2.5: Handle quick-task branching')); - // 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('Step 2.5 section is present and contains executable bash', () => { + const bash = extractStep25Bash(); + assert.ok(bash.length > 0, 'Step 2.5 should contain at least one bash block'); }); - test('branching step runs before task directory creation', () => { - content = fs.readFileSync(workflowPath, 'utf-8'); + test('Step 2.5 runs before Step 3 (task directory creation)', () => { + const content = fs.readFileSync(QUICK_PATH, 'utf-8'); const branchingIndex = content.indexOf('Step 2.5: Handle quick-task branching'); const createDirIndex = content.indexOf('Step 3: Create task directory'); - assert.ok(branchingIndex !== -1 && createDirIndex !== -1, 'workflow should contain both branching and directory steps'); - assert.ok(branchingIndex < createDirIndex, 'branching should happen before quick task directories and commits'); + assert.ok( + branchingIndex !== -1 && createDirIndex !== -1, + 'workflow should contain both branching and directory steps' + ); + assert.ok( + branchingIndex < createDirIndex, + 'branching should happen before quick task directories and commits' + ); + }); + + test('new quick-task branch contains 0 commits inherited from previous-task HEAD (#2916)', () => { + const bash = extractStep25Bash(); + const { root, clonePath } = setupFixture(); + + try { + // Sanity: we begin sitting on the previous quick-task branch, 1 ahead of origin/main. + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'quick/01-prev-task' + ); + assert.equal( + git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'), + '1', + 'fixture should be 1 commit ahead of origin/main' + ); + + runStep(bash, clonePath, 'quick/02-new-task'); + + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'quick/02-new-task', + 'Step 2.5 should switch to the new quick-task branch' + ); + + const inherited = git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'); + assert.equal( + inherited, + '0', + `new quick-task branch must branch off origin/main, but inherited ${inherited} commit(s) from previous-task HEAD` + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + git(clonePath, 'rev-parse', 'origin/main'), + 'new quick-task branch tip must equal origin/main tip' + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + test('Step 2.5 reuses an existing quick-task branch instead of forking again', () => { + const bash = extractStep25Bash(); + 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', 'quick/02-new-task', 'origin/main'); + fs.writeFileSync(path.join(clonePath, 'task02.txt'), 'task 2 work\n'); + git(clonePath, 'add', 'task02.txt'); + git(clonePath, 'commit', '-m', 'task 02 wip'); + const task02Sha = git(clonePath, 'rev-parse', 'HEAD'); + git(clonePath, 'checkout', 'quick/01-prev-task'); + + runStep(bash, clonePath, 'quick/02-new-task'); + + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'quick/02-new-task' + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + task02Sha, + 'existing-branch tip must be preserved (no rebase/reset)' + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } }); }); From 2256e4c9a34c2da44e084af6a2b0644eb6e832ee Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:48:36 -0400 Subject: [PATCH 4/8] fix(#2916): use fork-point detection for non-default-base warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the "ahead-of" heuristic with a structural check that compares the HEAD↔origin/$DEFAULT_BRANCH merge-base to origin/$DEFAULT_BRANCH itself. The previous count-based warning fired on legitimate WIP that was simply ahead of the default branch — the correct signal is that the branch did not fork from the default branch in the first place. Addresses CodeRabbit review on PR #2921. Co-Authored-By: Claude Opus 4.7 --- get-shit-done/workflows/execute-phase.md | 6 +++--- get-shit-done/workflows/quick.md | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 1e2bd3617..f7e88cbc4 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -241,9 +241,9 @@ else git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH" # pinned base (#2916) fi -INHERITED=$(git rev-list --count "origin/${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." +# Warn only when HEAD did NOT fork from origin/$DEFAULT_BRANCH (merge-base ≠ tip) — #2916. +if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then + echo "WARNING: Phase branch '$BRANCH_NAME' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional." fi ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 6ced91b82..d3d110c31 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -222,9 +222,9 @@ else git checkout -b "$branch_name" "origin/$DEFAULT_BRANCH" fi -INHERITED=$(git rev-list --count "origin/${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." +# Warn only when HEAD did NOT fork from origin/$DEFAULT_BRANCH (merge-base ≠ tip) — #2916. +if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then + echo "WARNING: Quick-task branch '$branch_name' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional before continuing." fi ``` From 80f14cac1fb1d0525a53f3b905dd91c49cb9264a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:48:43 -0400 Subject: [PATCH 5/8] test(#2916): scope branch_name scan to init step and harden fixture - Restrict the "init parse list includes branch_name" assertion to the bash blocks inside Step 2 (Initialize) so an unrelated step that mentions branch_name cannot mask the contract. - Switch the fixture's default branch from main to trunk so the symbolic-ref code path is locked in: a regression that silently defaults to "main" instead of consulting origin/HEAD now fails. Addresses CodeRabbit review on PR #2921. Co-Authored-By: Claude Opus 4.7 --- tests/quick-branching.test.cjs | 80 ++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/tests/quick-branching.test.cjs b/tests/quick-branching.test.cjs index 4068029dd..9c4fed373 100644 --- a/tests/quick-branching.test.cjs +++ b/tests/quick-branching.test.cjs @@ -93,9 +93,15 @@ function extractStep25Bash() { } /** - * Build a fixture: a bare "origin" repo with `main` (one commit), a clone with - * `origin/HEAD` pointed at `main`, and a checked-out previous-task branch - * carrying its own unmerged commit. + * Build a fixture: a bare "origin" repo with a non-`main` default branch + * (`trunk`) so the test fails if the workflow silently falls back to "main" + * instead of consulting `origin/HEAD`. The clone has `origin/HEAD` pointed at + * `trunk` and a checked-out previous-task branch carrying its own unmerged + * commit. + * + * Using `trunk` here locks in the symbolic-ref code path: if the + * implementation skips `git symbolic-ref refs/remotes/origin/HEAD` and just + * defaults to `main`, every assertion below collapses (#2921 CR nitpick). */ function setupFixture() { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-quick-branching-')); @@ -104,21 +110,21 @@ function setupFixture() { const clonePath = path.join(root, 'clone'); fs.mkdirSync(seedPath); - git(seedPath, 'init', '-b', 'main'); + git(seedPath, 'init', '-b', 'trunk'); 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(originPath, 'symbolic-ref', 'HEAD', 'refs/heads/trunk'); 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 quick task: branch off main, add a commit, + // Simulate finishing a previous quick task: branch off trunk, add a commit, // and stay on it (this is the failure scenario from #2916). git(clonePath, 'checkout', '-b', 'quick/01-prev-task'); fs.writeFileSync(path.join(clonePath, 'prev.txt'), 'prev work\n'); @@ -153,22 +159,40 @@ describe('quick workflow: branching support', () => { }); test('init parse list includes branch_name', () => { - // Structural: the workflow's init JSON parsing must mention branch_name as - // a top-level field. We assert this by parsing the init step's bash blocks - // for the `branch_name` JSON path / variable rather than substring-grep. + // Structural: the workflow's init step (Step 2) must declare branch_name as + // a parseable field of the init JSON. Restrict the scan to the init step's + // section only — a global walk over every bash fence could be fooled by an + // unrelated step that happens to mention branch_name (#2921 CR). const content = fs.readFileSync(QUICK_PATH, 'utf-8'); const lines = content.split(/\r?\n/); - // Walk every fenced bash block in the file; look for an assignment that - // reads a `branch_name` field (jq, awk, or shell parameter). - let found = false; - let inBash = false; - for (const line of lines) { - if (!inBash && /^```bash\s*$/.test(line)) { inBash = true; continue; } - if (inBash && /^```\s*$/.test(line)) { inBash = false; continue; } - if (!inBash) continue; - if (/\bbranch_name\b/.test(line)) { found = true; break; } + + // Locate the "Step 2: Initialize" heading and the next "Step N" heading + // that ends the section. We match the markdown bold-step convention used + // throughout quick.md: `**Step N[.M]: Title**`. + let start = -1; + let end = -1; + for (let i = 0; i < lines.length; i += 1) { + if (start === -1 && /^\*\*Step 2:\s*Initialize\*\*\s*$/.test(lines[i])) { + start = i + 1; + } else if (start !== -1 && /^\*\*Step \d+(?:\.\d+)?:\s/.test(lines[i])) { + end = i; + break; + } } - assert.ok(found, 'quick workflow should expose branch_name inside a bash block'); + assert.notEqual(start, -1, 'quick.md should contain a "Step 2: Initialize" section'); + if (end === -1) end = lines.length; + + // Within that section, look for the branch_name token inside fenced bash + // blocks AND in the surrounding markdown prose that documents the JSON + // fields. Both are part of the init contract. + let found = false; + for (let i = start; i < end; i += 1) { + if (/\bbranch_name\b/.test(lines[i])) { found = true; break; } + } + assert.ok( + found, + 'Step 2 (Initialize) of quick workflow should expose branch_name as part of the init contract' + ); }); test('Step 2.5 section is present and contains executable bash', () => { @@ -195,15 +219,15 @@ describe('quick workflow: branching support', () => { const { root, clonePath } = setupFixture(); try { - // Sanity: we begin sitting on the previous quick-task branch, 1 ahead of origin/main. + // Sanity: we begin sitting on the previous quick-task branch, 1 ahead of origin/trunk. assert.equal( git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), 'quick/01-prev-task' ); assert.equal( - git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'), + git(clonePath, 'rev-list', '--count', 'origin/trunk..HEAD'), '1', - 'fixture should be 1 commit ahead of origin/main' + 'fixture should be 1 commit ahead of origin/trunk' ); runStep(bash, clonePath, 'quick/02-new-task'); @@ -214,16 +238,16 @@ describe('quick workflow: branching support', () => { 'Step 2.5 should switch to the new quick-task branch' ); - const inherited = git(clonePath, 'rev-list', '--count', 'origin/main..HEAD'); + const inherited = git(clonePath, 'rev-list', '--count', 'origin/trunk..HEAD'); assert.equal( inherited, '0', - `new quick-task branch must branch off origin/main, but inherited ${inherited} commit(s) from previous-task HEAD` + `new quick-task branch must branch off origin/trunk, but inherited ${inherited} commit(s) from previous-task HEAD` ); assert.equal( git(clonePath, 'rev-parse', 'HEAD'), - git(clonePath, 'rev-parse', 'origin/main'), - 'new quick-task branch tip must equal origin/main tip' + git(clonePath, 'rev-parse', 'origin/trunk'), + 'new quick-task branch tip must equal origin/trunk tip' ); } finally { fs.rmSync(root, { recursive: true, force: true }); @@ -235,9 +259,9 @@ describe('quick workflow: branching support', () => { const { root, clonePath } = setupFixture(); try { - // Pre-create the target branch off origin/main with its own commit, then + // Pre-create the target branch off origin/trunk with its own commit, then // walk away to a different branch — the step must switch back to it. - git(clonePath, 'checkout', '-B', 'quick/02-new-task', 'origin/main'); + git(clonePath, 'checkout', '-B', 'quick/02-new-task', 'origin/trunk'); fs.writeFileSync(path.join(clonePath, 'task02.txt'), 'task 2 work\n'); git(clonePath, 'add', 'task02.txt'); git(clonePath, 'commit', '-m', 'task 02 wip'); From ace61869d0688b1fa833e1239d0b2a67d247d824 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:57:27 -0400 Subject: [PATCH 6/8] test(#2916): parameterize fixtures so both main and trunk are exercised Two follow-ups on commit 80f14cac (which hardened quick-branching with a trunk fixture): 1. quick-branching.test.cjs: add a `defaultBranch` parameter to setupFixture and run the "branches off origin/HEAD" assertion against both `main` and `trunk`. The wholesale switch to trunk in 80f14cac removed coverage of the conventional `main` path; parameterizing restores it without giving up the symbolic-ref guarantee. 2. bug-2916-handle-branching-default-base.test.cjs: apply the same parameterization here. handle_branching has the same default-branch detection logic as Step 2.5, so it deserves the same trunk regression guard. Previously this file only exercised `main`. A regression that silently defaults to `main` instead of consulting `git symbolic-ref refs/remotes/origin/HEAD` now fails the `trunk` variant in both files. Tests: 10/10 in the touched suites. --- ...916-handle-branching-default-base.test.cjs | 99 ++++++++++--------- tests/quick-branching.test.cjs | 89 +++++++++-------- 2 files changed, 103 insertions(+), 85 deletions(-) diff --git a/tests/bug-2916-handle-branching-default-base.test.cjs b/tests/bug-2916-handle-branching-default-base.test.cjs index bfaca64f6..a51cbf2c0 100644 --- a/tests/bug-2916-handle-branching-default-base.test.cjs +++ b/tests/bug-2916-handle-branching-default-base.test.cjs @@ -108,39 +108,44 @@ function extractHandleBranchingBash() { } /** - * 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. + * Build a fixture: a bare "origin" repo with the named default branch (one + * commit), a clone with `origin/HEAD` pointed at it, and a checked-out + * previous-phase branch carrying its own unmerged commit. + * + * `defaultBranch` is parameterized so callers can lock in that the workflow + * honors `git symbolic-ref refs/remotes/origin/HEAD` rather than silently + * defaulting to `main` (#2921 CR feedback — quick-branching.test.cjs got the + * same treatment in 80f14cac; this test deserves the same coverage). */ -function setupFixture() { +function setupFixture(defaultBranch = 'main') { 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, 'init', '-b', defaultBranch); 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(originPath, 'symbolic-ref', 'HEAD', `refs/heads/${defaultBranch}`); 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). + // Simulate finishing a previous phase: branch off the default branch, add + // a commit, and *stay* on it (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 }; + return { root, clonePath, defaultBranch }; } function runHandleBranchingStep(bash, cwd, branchName) { @@ -163,45 +168,51 @@ function runHandleBranchingStep(bash, cwd, branchName) { } 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(); + // Run against `main` (conventional default) and `trunk` (non-main default + // exercising the symbolic-ref code path) so a regression that hard-codes + // `main` instead of consulting origin/HEAD will fail the trunk variant. + for (const defaultBranch of ['main', 'trunk']) { + test(`new phase branch branches off origin/${defaultBranch} with 0 inherited commits`, () => { + const bash = extractHandleBranchingBash(); + const { root, clonePath } = setupFixture(defaultBranch); - 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' - ); + try { + const upstream = `origin/${defaultBranch}`; - runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-01-foundation' + ); + assert.equal( + git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`), + '1', + `fixture should be 1 commit ahead of ${upstream}` + ); - assert.equal( - git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'feature/phase-02-content-sync', - 'handle_branching should switch to the new phase branch' - ); + runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); - 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 }); - } - }); + 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', `${upstream}..HEAD`); + assert.equal( + inherited, + '0', + `new phase branch must branch off ${upstream}, but inherited ${inherited} commit(s) from previous-phase HEAD` + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + git(clonePath, 'rev-parse', upstream), + `new phase branch tip must equal ${upstream} tip` + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + } test('handle_branching reuses an existing branch instead of forking again', () => { const bash = extractHandleBranchingBash(); diff --git a/tests/quick-branching.test.cjs b/tests/quick-branching.test.cjs index 9c4fed373..d0ac0dacb 100644 --- a/tests/quick-branching.test.cjs +++ b/tests/quick-branching.test.cjs @@ -103,35 +103,35 @@ function extractStep25Bash() { * implementation skips `git symbolic-ref refs/remotes/origin/HEAD` and just * defaults to `main`, every assertion below collapses (#2921 CR nitpick). */ -function setupFixture() { +function setupFixture(defaultBranch = 'trunk') { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-quick-branching-')); 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', 'trunk'); + git(seedPath, 'init', '-b', defaultBranch); 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/trunk'); + git(originPath, 'symbolic-ref', 'HEAD', `refs/heads/${defaultBranch}`); 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 quick task: branch off trunk, add a commit, - // and stay on it (this is the failure scenario from #2916). + // Simulate finishing a previous quick task: branch off the default branch, + // add a commit, and stay on it (this is the failure scenario from #2916). git(clonePath, 'checkout', '-b', 'quick/01-prev-task'); fs.writeFileSync(path.join(clonePath, 'prev.txt'), 'prev work\n'); git(clonePath, 'add', 'prev.txt'); git(clonePath, 'commit', '-m', 'prev quick task work'); - return { root, clonePath }; + return { root, clonePath, defaultBranch }; } function runStep(bash, cwd, branchName) { @@ -214,45 +214,52 @@ describe('quick workflow: branching support', () => { ); }); - test('new quick-task branch contains 0 commits inherited from previous-task HEAD (#2916)', () => { - const bash = extractStep25Bash(); - const { root, clonePath } = setupFixture(); + // Run against both `main` (the conventional default) and `trunk` (a non- + // main default that exercises the symbolic-ref code path). Keeping both + // restores main coverage that was removed when the fixture switched + // wholesale to trunk in 80f14cac. + for (const defaultBranch of ['main', 'trunk']) { + test(`new quick-task branch branches off origin/${defaultBranch} (#2916)`, () => { + const bash = extractStep25Bash(); + const { root, clonePath } = setupFixture(defaultBranch); - try { - // Sanity: we begin sitting on the previous quick-task branch, 1 ahead of origin/trunk. - assert.equal( - git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'quick/01-prev-task' - ); - assert.equal( - git(clonePath, 'rev-list', '--count', 'origin/trunk..HEAD'), - '1', - 'fixture should be 1 commit ahead of origin/trunk' - ); + try { + const upstream = `origin/${defaultBranch}`; - runStep(bash, clonePath, 'quick/02-new-task'); + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'quick/01-prev-task' + ); + assert.equal( + git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`), + '1', + `fixture should be 1 commit ahead of ${upstream}` + ); - assert.equal( - git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'quick/02-new-task', - 'Step 2.5 should switch to the new quick-task branch' - ); + runStep(bash, clonePath, 'quick/02-new-task'); - const inherited = git(clonePath, 'rev-list', '--count', 'origin/trunk..HEAD'); - assert.equal( - inherited, - '0', - `new quick-task branch must branch off origin/trunk, but inherited ${inherited} commit(s) from previous-task HEAD` - ); - assert.equal( - git(clonePath, 'rev-parse', 'HEAD'), - git(clonePath, 'rev-parse', 'origin/trunk'), - 'new quick-task branch tip must equal origin/trunk tip' - ); - } finally { - fs.rmSync(root, { recursive: true, force: true }); - } - }); + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'quick/02-new-task', + 'Step 2.5 should switch to the new quick-task branch' + ); + + const inherited = git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`); + assert.equal( + inherited, + '0', + `new quick-task branch must branch off ${upstream}, but inherited ${inherited} commit(s) from previous-task HEAD` + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + git(clonePath, 'rev-parse', upstream), + `new quick-task branch tip must equal ${upstream} tip` + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + } test('Step 2.5 reuses an existing quick-task branch instead of forking again', () => { const bash = extractStep25Bash(); From c3aef27aa692a105ae8ca7a468a94cc82433b439 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 22:07:46 -0400 Subject: [PATCH 7/8] fix(#2916): fail-fast on switch/checkout, gate fork-point warning to fresh branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit findings on PR #2921 (review 4209533909 + comment 3171721073, both still unresolved): A. Branch switch and create steps now abort on non-zero exit. Previously `git switch "$BRANCH_NAME"` and `git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH"` could fail (locked worktree, dirty tree refusing the checkout, etc.) and the workflow would silently continue on the wrong branch — sending the phase's later commits to the wrong place. Both calls now `|| { echo "ERROR: …" >&2; exit 1; }`. B. The fork-point base-warning is now scoped to the creation arm of the if/else. Previously it ran for the resume path too, so a legitimate resumed branch where origin/$DEFAULT_BRANCH had advanced since first creation would falsely warn ("does not fork from origin/"). Moving the check inside the else arm means it only runs immediately after a fresh `git checkout -b`, when the merge-base check is meaningful. Same fix mirrored in get-shit-done/workflows/quick.md. execute-phase.md stays at the 1700-line XL budget. Full suite: 6102/6102. --- get-shit-done/workflows/execute-phase.md | 15 ++++++++------- get-shit-done/workflows/quick.md | 15 +++++++++------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index f7e88cbc4..86d243c6a 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -226,7 +226,7 @@ DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/de DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} if git show-ref --verify --quiet "refs/heads/$BRANCH_NAME"; then - git switch "$BRANCH_NAME" + git switch "$BRANCH_NAME" || { echo "ERROR: Could not switch to existing branch '$BRANCH_NAME'." >&2; exit 1; } else if ! git fetch --quiet origin "$DEFAULT_BRANCH"; then # #2916 git show-ref --verify --quiet "refs/remotes/origin/$DEFAULT_BRANCH" \ @@ -238,12 +238,13 @@ else else git switch --quiet "$DEFAULT_BRANCH" 2>/dev/null && git merge --ff-only --quiet "origin/$DEFAULT_BRANCH" 2>/dev/null || true fi - git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH" # pinned base (#2916) -fi - -# Warn only when HEAD did NOT fork from origin/$DEFAULT_BRANCH (merge-base ≠ tip) — #2916. -if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then - echo "WARNING: Phase branch '$BRANCH_NAME' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional." + git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH" \ + || { echo "ERROR: Could not create '$BRANCH_NAME' from origin/$DEFAULT_BRANCH (#2916)." >&2; exit 1; } + # Warn only on fresh creation when HEAD did NOT fork from origin/$DEFAULT_BRANCH — #2916. + # Skipped on resume because origin may have advanced legitimately since first creation. + if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then + echo "WARNING: Phase branch '$BRANCH_NAME' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional." + fi fi ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index d3d110c31..c7cf6f265 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -192,7 +192,8 @@ DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/de DEFAULT_BRANCH=${DEFAULT_BRANCH:-main} if git show-ref --verify --quiet "refs/heads/$branch_name"; then - git switch "$branch_name" + git switch "$branch_name" \ + || { echo "ERROR: Could not switch to existing quick-task branch '$branch_name'." >&2; exit 1; } else # Fetch the default branch so origin/$DEFAULT_BRANCH is current. If the fetch # fails (offline, no remote, auth failure) AND we have no local copy of @@ -219,12 +220,14 @@ else # Always pin the new branch to origin/$DEFAULT_BRANCH so the start point is # deterministic regardless of which branch we are currently on (#2916). - git checkout -b "$branch_name" "origin/$DEFAULT_BRANCH" -fi + git checkout -b "$branch_name" "origin/$DEFAULT_BRANCH" \ + || { echo "ERROR: Could not create '$branch_name' from origin/$DEFAULT_BRANCH (#2916)." >&2; exit 1; } -# Warn only when HEAD did NOT fork from origin/$DEFAULT_BRANCH (merge-base ≠ tip) — #2916. -if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then - echo "WARNING: Quick-task branch '$branch_name' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional before continuing." + # Warn only on fresh creation when HEAD did NOT fork from origin/$DEFAULT_BRANCH — #2916. + # Skipped on resume because origin may have advanced legitimately since first creation. + if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then + echo "WARNING: Quick-task branch '$branch_name' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional before continuing." + fi fi ``` From 3ed898051945966167bf33bce586b3d59f7e8f50 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 22:18:34 -0400 Subject: [PATCH 8/8] fix(#2916): drop unreachable post-creation merge-base guard CodeRabbit pointed out the post-creation guard is structurally unreachable: immediately after `git checkout -b X origin/$DEFAULT_BRANCH`, HEAD == origin/$DEFAULT_BRANCH, so both the merge-base form (`MB == DT`) and the alternative "ahead-of" count form (`AHEAD == 0`) are sentinels that always pass on a successful fresh checkout. With the explicit base arg + fail-fast on the checkout, the guard cannot catch anything new. Removing it (rather than swapping in another no-op that satisfies the linter but adds no actual coverage) is the honest fix. Comment retained to explain why no post-creation guard is needed: the explicit base argument to `git checkout -b` is the single source of correctness for #2916. Same simplification mirrored in get-shit-done/workflows/quick.md. Full suite: 6102/6102. --- get-shit-done/workflows/execute-phase.md | 8 +++----- get-shit-done/workflows/quick.md | 11 ++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 86d243c6a..b50ea11f2 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -238,13 +238,11 @@ else else git switch --quiet "$DEFAULT_BRANCH" 2>/dev/null && git merge --ff-only --quiet "origin/$DEFAULT_BRANCH" 2>/dev/null || true fi + # Pinned base + fail-fast: on success HEAD is exactly at origin/$DEFAULT_BRANCH, + # so a post-creation merge-base or "ahead-of" guard would be unreachable. The + # explicit base argument here is the single source of correctness for #2916. git checkout -b "$BRANCH_NAME" "origin/$DEFAULT_BRANCH" \ || { echo "ERROR: Could not create '$BRANCH_NAME' from origin/$DEFAULT_BRANCH (#2916)." >&2; exit 1; } - # Warn only on fresh creation when HEAD did NOT fork from origin/$DEFAULT_BRANCH — #2916. - # Skipped on resume because origin may have advanced legitimately since first creation. - if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then - echo "WARNING: Phase branch '$BRANCH_NAME' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional." - fi fi ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index c7cf6f265..936578346 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -218,16 +218,13 @@ else || true fi - # Always pin the new branch to origin/$DEFAULT_BRANCH so the start point is + # Pin the new branch to origin/$DEFAULT_BRANCH so the start point is # deterministic regardless of which branch we are currently on (#2916). + # On success HEAD is exactly at origin/$DEFAULT_BRANCH, so a post-creation + # merge-base / "ahead-of" guard would be unreachable — the explicit base + # argument here is the single source of correctness for #2916. git checkout -b "$branch_name" "origin/$DEFAULT_BRANCH" \ || { echo "ERROR: Could not create '$branch_name' from origin/$DEFAULT_BRANCH (#2916)." >&2; exit 1; } - - # Warn only on fresh creation when HEAD did NOT fork from origin/$DEFAULT_BRANCH — #2916. - # Skipped on resume because origin may have advanced legitimately since first creation. - if MB=$(git merge-base HEAD "origin/${DEFAULT_BRANCH}" 2>/dev/null) && DT=$(git rev-parse --verify --quiet "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null) && [ "$MB" != "$DT" ]; then - echo "WARNING: Quick-task branch '$branch_name' does not fork from origin/${DEFAULT_BRANCH}; verify the base is intentional before continuing." - fi fi ```