From 4e2f1105d95b14a34490ad3ac92cb024c9ba00cb Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 21:22:44 -0400 Subject: [PATCH] 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