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

fix(#2916): branch new phases off origin/HEAD instead of current HEAD

Browse Source 
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 <step name="handle_branching">
block structurally and runs it against a fixture repo where HEAD sits on
a previous-phase branch with extra commits.
This commit is contained in:
Tom Boucher
2026-04-30 17:30:52 -04:00
parent 006cdafe8f
commit 294564b951
4 changed files with 302 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
get-shit-done/workflows/execute-phase.md
View File

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

28
get-shit-done/workflows/quick.md
View File

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

235
tests/bug-2916-handle-branching-default-base.test.cjs Normal file
View File

@@ -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 <step name="handle_branching">
* 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 <step name="handle_branching"> ... </step> 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 && /^<step\s+name="handle_branching">\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 <step name="handle_branching"> ... </step> 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 });
}
});
});

15
tests/quick-branching.test.cjs
View File

@@ -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', () => {