diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 3bb7caaf6..1f82e5f67 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.15.4", + "version": "0.15.5", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 9858288c4..2675f3539 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,41 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.15.5] + +### Fixed + +- **`/pull-request` create flow no longer silently corrupts a branch's fetch/rebase upstream when + publishing it for a PR (#442, residual finding from PR #763).** A post-merge review empirically + reproduced a residual silent clobber: §2.4.1's conditional `-u` gate keyed on the LITERAL + `branch..remote` config being set. In the triangular shape where `remote.pushDefault` names a + fork globally but `branch..remote` is unset (so fetch/rebase falls back to `origin`), the gate + read "unset", took the `-u` bootstrap path, and `git push -u ` rewrote `branch..remote` + to the fork — so the next fetch/rebase silently targeted the fork instead of `origin`. The gate now + fires `-u` only when the branch has NO existing upstream (`branch..remote` AND + `branch..merge` both literally unset) AND its fetch and push remotes resolve to the same name + (`resolve-remote.sh` fetch-mode vs `--push`); otherwise it pushes plain and writes no branch config. + This closes the reported `pushDefault`-only clobber (fetch resolves `origin`, push resolves the fork → + they differ → plain push, upstream untouched) and a broader corruption family the fix surfaced: + `git push -u` rewrites the branch's WHOLE upstream — both `branch..remote` and + `branch..merge` — so a branch with any configured tracking kept its merge ref overwritten under + a resolved-name-only comparison. Three such shapes: an already-tracked branch; a deliberate local-only + `.` upstream (`git branch --track . `); and merge-only tracking (`branch..merge` set with + `branch..remote` unset — valid, since Git defaults the remote to `origin`, so the branch tracks + `origin/`). Requiring BOTH upstream keys to be absent before bootstrapping preserves any + existing tracking via plain push. This also changes #763's behavior for the `.` case (it took the + `-u` path); publishing a branch for a PR no longer mutates a deliberate local-only or merge-only + upstream — a strict improvement. An ambiguous fetch resolution (empty) is unequal to any push remote → + plain push, never an abort. The conditional moved out of the `create.md` prose into a new co-located + `scripts/push-branch.sh` (§2.4.1 now delegates to it), so the gate sequence is executable and testable + rather than living only in markdown; the normalized `.`-as-unset / `\r`-strip handling stays solely in + `resolve-remote.sh` and is not duplicated (the upstream-absent probe reads both keys raw — any + non-empty value means "has an upstream"). New `push-branch.test.sh` drives the full resolve-fetch → + resolve-push → conditional-push → re-resolve-fetch sequence against real bare remotes across the + pushRemote-triangular, `pushDefault`-only triangular, non-triangular (asserting the merge ref is + preserved), fresh-branch bootstrap, local-only `.`, merge-only tracking, and fetch-ambiguous shapes — + the integration coverage whose absence let this escape `resolve-remote.test.sh`'s resolver-only cases. + ## [0.15.4] ### Fixed diff --git a/plugins/source-control/skills/pull-request/reference/create.md b/plugins/source-control/skills/pull-request/reference/create.md index a8f286310..9c951f518 100644 --- a/plugins/source-control/skills/pull-request/reference/create.md +++ b/plugins/source-control/skills/pull-request/reference/create.md @@ -172,36 +172,19 @@ Persist chosen line(s) into `${CLOSES_LINE}`. NEVER wrap a closing keyword in an ### 2.4.1 Push and assemble PR body ```bash -# Push via the shared resolver in --push mode, which applies Git's documented -# push precedence: branch..pushRemote, else remote.pushDefault, else the -# §2.2 fetch order (branch..remote, else `origin`, else the sole other -# configured remote). This is why the push resolves separately from §2.2's -# fetch remote — Git lets the push destination differ, so a triangular fork -# flow that fetches from `upstream` but sets pushRemote/pushDefault to the fork -# pushes to the fork, not `upstream`. A fork may be named `origin`, `fork`, or -# anything else, and a repo cloned with a non-origin sole remote (`git clone -o -# vendor`) must push there too, not `origin`. Two or more non-origin candidates -# with no `origin` set fails loudly (see §2.2) rather than pushing to an -# arbitrary remote. -# -# `-u` is CONDITIONAL: `git push -u` rewrites `branch..remote` to the -# push target, so on a triangular fork (fetch `upstream`, push a fork via -# pushRemote/pushDefault) an unconditional `-u` would silently repoint the -# FETCH remote §2.2 reads to the fork — breaking the next rebase. So bootstrap -# tracking with `-u` only when the branch has no real `branch..remote` -# yet (a fresh feature branch, or a local-only `.` upstream): the common first -# push still sets upstream to `origin` exactly as before. When a real fetch -# remote is already configured, push WITHOUT `-u` to preserve it. Either way -# later pushes need no remote argument (tracking was already set, or `-u` just -# set it). Match resolve-remote.sh's `\r` strip and `.`-as-unset convention. -PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push) || exit 1 -BRANCH_NAME=$(git branch --show-current) -EXISTING_FETCH_REMOTE=$(git config "branch.${BRANCH_NAME}.remote" 2>/dev/null | tr -d '\r') -if [[ -n "$EXISTING_FETCH_REMOTE" && "$EXISTING_FETCH_REMOTE" != "." ]]; then - git push "$PUSH_REMOTE" "$BRANCH_NAME" -else - git push -u "$PUSH_REMOTE" "$BRANCH_NAME" -fi +# Push via push-branch.sh, which resolves the push and fetch/rebase remotes +# independently (resolve-remote.sh --push vs plain) and sets upstream (`-u`) +# ONLY for a branch with NO existing upstream — branch..remote AND +# branch..merge both unset — whose fetch and push resolve to the same +# remote (a fresh feature branch's first push). `git push -u` rewrites the +# branch's whole upstream — both keys — so any existing upstream (a real remote, +# a deliberate local-only `.`, or a merge ref set with the remote defaulting to +# `origin`) is preserved by a plain push instead. This closes two silent +# corruptions: a triangular fork (push a fork via pushRemote/pushDefault, fetch +# `origin`/`upstream`) no longer repoints the fetch remote to the fork, and a +# branch with any configured tracking no longer has its merge ref overwritten. +# See the script header for the full rationale. +bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1 ``` Derive PR title from the commit subject, shaped to satisfy the resolved subject/title convention (the ladder in [SKILL.md](../SKILL.md): layered `source-control.md` config → project convention → Conventional Commits default). Build body with `${CLOSES_LINE}` at top, followed by Summary + Test plan + a `## Related` section + a config-gated attribution line: diff --git a/plugins/source-control/skills/pull-request/scripts/push-branch.sh b/plugins/source-control/skills/pull-request/scripts/push-branch.sh new file mode 100755 index 000000000..6f159d977 --- /dev/null +++ b/plugins/source-control/skills/pull-request/scripts/push-branch.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# Push the current (or named) branch to its resolved push remote, setting +# upstream tracking (`-u`) ONLY for a branch that has no existing upstream and +# whose fetch and push remotes resolve to the same name. +# +# Push destination and fetch/rebase remote are resolved independently by the +# sibling resolve-remote.sh (Git lets them differ in a triangular fork flow): +# PUSH_REMOTE = resolve-remote.sh --push (pushRemote -> pushDefault -> fetch order) +# FETCH_REMOTE = resolve-remote.sh (branch..remote -> origin -> sole other) +# +# `-u` is CONDITIONAL because `git push -u` rewrites the branch's ENTIRE upstream +# — both branch..remote AND branch..merge (to refs/heads/). Two +# distinct ways that silently corrupts where the branch fetches/rebases from: +# +# 1. Retarget the remote. On a triangular fork (push to a fork via +# pushRemote/pushDefault, fetch from origin/upstream) `-u` would repoint +# branch..remote to the fork, so the next rebase targets the wrong +# base. The trap is the shape where the fetch remote is NOT literally +# configured but resolves via fallback: remote.pushDefault=fork with +# branch..remote unset fetches from `origin` (fallback) yet pushes to +# the fork. +# 2. Overwrite an existing upstream. The upstream is TWO keys — +# branch..remote AND branch..merge — and `-u` rewrites both. +# Any of these partial or full configs is an upstream the user chose that a +# bootstrap must not clobber: +# - both set to a real remote/ref (an ordinary tracked branch); +# - branch..remote="." (a deliberate local-only upstream, e.g. +# `git branch --track . `), merge naming the tracked local ref; +# - branch..merge set with branch..remote UNSET — valid: Git +# defaults the missing remote to `origin`, so the branch tracks +# `origin/`. `-u` here replaces the chosen merge ref with +# refs/heads/, the same clobber family as (1). +# +# The gate therefore fires `-u` only when BOTH hold: +# * the branch has NO existing upstream — branch..remote AND +# branch..merge are BOTH literally unset (a genuinely fresh feature +# branch, nothing to preserve), AND +# * FETCH_REMOTE == PUSH_REMOTE (bootstrapping tracking cannot misdirect the +# fetch remote, because it can only be set to what fetch already resolves to). +# Any existing upstream — full, ".", or merge-only — is preserved by a plain +# push, which never writes branch config. An ambiguous fetch (FETCH_REMOTE empty: +# 2+ non-origin remotes, no origin) is unequal to any push remote -> plain push +# -> no write. +# +# The upstream-absent check reads both keys raw (no "." or CRLF normalization): +# any non-empty value -> "has an upstream" -> plain push preserves it; only the +# truly-unset (empty) keys are a bootstrap candidate. That normalized "." +# handling lives once, in resolve-remote.sh, and is not duplicated here. +# +# Push resolution must be determinate (it names the destination), so a failed +# --push resolution aborts; a failed fetch resolution does not. +# +# Usage: push-branch.sh [branch-name] (defaults to the current branch) +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +RESOLVER="${SCRIPT_DIR}/resolve-remote.sh" + +BRANCH="${1:-}" +if [[ -z "$BRANCH" ]]; then + BRANCH="$(git branch --show-current 2>/dev/null | tr -d '\r')" +fi + +PUSH_REMOTE=$(bash "$RESOLVER" --push "$BRANCH") || exit 1 +FETCH_REMOTE=$(bash "$RESOLVER" "$BRANCH" 2>/dev/null) +EXISTING_REMOTE=$(git config "branch.${BRANCH}.remote" 2>/dev/null) +EXISTING_MERGE=$(git config "branch.${BRANCH}.merge" 2>/dev/null) + +if [[ -z "$EXISTING_REMOTE" && -z "$EXISTING_MERGE" && "$FETCH_REMOTE" == "$PUSH_REMOTE" ]]; then + git push -u "$PUSH_REMOTE" "$BRANCH" +else + git push "$PUSH_REMOTE" "$BRANCH" +fi diff --git a/plugins/source-control/skills/pull-request/scripts/push-branch.test.sh b/plugins/source-control/skills/pull-request/scripts/push-branch.test.sh new file mode 100755 index 000000000..44cf244e2 --- /dev/null +++ b/plugins/source-control/skills/pull-request/scripts/push-branch.test.sh @@ -0,0 +1,175 @@ +#!/usr/bin/env bash +# Integration tests for push-branch.sh — the §2.4.1 push gate sequence +# (resolve-fetch -> resolve-push -> conditional `-u` push -> re-resolve-fetch). +# +# resolve-remote.test.sh exercises the resolver in ISOLATION; it cannot catch a +# gate that resolves correctly yet still writes the branch's upstream to the +# wrong place — the pushDefault-only triangular clobber lived in exactly that +# gap. These cases drive REAL pushes into REAL bare remotes +# and assert the destination that received the branch AND whether the conditional +# `-u` rewrote the branch's upstream (branch..remote AND branch..merge +# — `git push -u` writes both), the observables a clobber would corrupt. +# +# The suite pins the refined invariant in BOTH directions, so neither strawman +# passes: +# * bootstrap POSITIVELY asserts `-u` fired (branch.remote set from unset) — +# rejects an always-plain-push implementation. +# * non-triangular, local-only `.`, and merge-only tracking assert an EXISTING +# upstream's merge ref survives the push — rejects an always-`-u` +# implementation, the resolved-name-only gate that shipped `-u` whenever +# fetch==push, and a remote-only "no upstream" probe that misses a branch +# tracking origin/ via branch..merge with the remote unset. +# * the fetch-ambiguous case proves a failed fetch resolution degrades to a +# plain push (no clobber), never an abort. + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +PUSH_BRANCH="${SCRIPT_DIR}/push-branch.sh" +RESOLVER="${SCRIPT_DIR}/resolve-remote.sh" + +for s in "$PUSH_BRANCH" "$RESOLVER"; do + if [[ ! -x "$s" ]]; then + echo "FAIL: missing or not executable: $s" >&2 + exit 1 + fi +done + +PASS=0 +FAIL=0 +WORKDIR="$(mktemp -d)" +trap 'rm -rf "$WORKDIR"' EXIT + +BRANCH="feat/x" + +# Build a working clone under $WORKDIR/$1 with `feat/x` checked out (one commit), +# plus a bare remote per remaining `name` arg. The clone's `origin` is a bare +# repo of its own so a real push has somewhere to land. Echoes the clone path. +make_clone() { + local name="$1" + shift + local base="$WORKDIR/$name" + mkdir -p "$base" + git init -q --bare "$base/origin.git" + git clone -q "$base/origin.git" "$base/wc" 2>/dev/null + git -C "$base/wc" config user.email test@example.com + git -C "$base/wc" config user.name Test + git -C "$base/wc" commit -q --allow-empty -m init + git -C "$base/wc" checkout -q -b "$BRANCH" + for extra in "$@"; do + git init -q --bare "$base/$extra.git" + git -C "$base/wc" remote add "$extra" "$base/$extra.git" + done + echo "$base/wc" +} + +check() { + local desc="$1" got="$2" want="$3" + if [[ "$got" == "$want" ]]; then + echo "PASS: $desc" + PASS=$((PASS + 1)) + else + echo "FAIL: $desc" + echo " got '$got'" + echo " wanted '$want'" + FAIL=$((FAIL + 1)) + fi +} + +# 'yes' if $2.git has the branch ref, else 'no'. +has_branch() { + if git -C "$1.git" rev-parse --verify --quiet "refs/heads/$BRANCH" >/dev/null 2>&1; then + echo yes + else + echo no + fi +} + +fetch_config() { + git -C "$1" config "branch.${BRANCH}.remote" 2>/dev/null || echo UNSET +} + +merge_config() { + git -C "$1" config "branch.${BRANCH}.merge" 2>/dev/null || echo UNSET +} + +# 1. Fresh-branch bootstrap: no branch.remote, no push config. Fetch and push +# both fall back to origin -> equal -> `-u` fires, tracking is bootstrapped. +wc=$(make_clone bootstrap) +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "bootstrap: -u fired, branch.remote set to origin" "$(fetch_config "$wc")" "origin" +check "bootstrap: origin received the branch" "$(has_branch "$WORKDIR/bootstrap/origin")" "yes" + +# 2. Non-triangular, established upstream: branch.remote=origin already set, with +# a NON-default merge ref (tracks origin/main). Push resolves origin too, but +# the upstream already exists -> plain push -> the merge ref must survive. An +# always-`-u` (or resolved-name-only) gate would rewrite it to refs/heads/feat/x. +wc=$(make_clone nontri) +git -C "$wc" config "branch.${BRANCH}.remote" origin +git -C "$wc" config "branch.${BRANCH}.merge" refs/heads/main +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "non-triangular: branch.remote stays origin" "$(fetch_config "$wc")" "origin" +check "non-triangular: merge ref preserved (not rewritten by -u)" "$(merge_config "$wc")" "refs/heads/main" +check "non-triangular: origin received the branch" "$(has_branch "$WORKDIR/nontri/origin")" "yes" + +# 3. Triangular via branch..pushRemote: fetch upstream, push fork. +# Resolved remotes differ -> plain push -> branch.remote preserved as upstream. +wc=$(make_clone tri-pushremote upstream fork) +git -C "$wc" config "branch.${BRANCH}.remote" upstream +git -C "$wc" config "branch.${BRANCH}.pushRemote" fork +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "pushRemote-tri: fetch remote preserved (upstream)" "$(fetch_config "$wc")" "upstream" +check "pushRemote-tri: fork received the branch" "$(has_branch "$WORKDIR/tri-pushremote/fork")" "yes" +check "pushRemote-tri: origin did NOT receive the branch" "$(has_branch "$WORKDIR/tri-pushremote/origin")" "no" + +# 4. THE #442 REGRESSION: pushDefault-only triangular. branch.remote UNSET, +# remote.pushDefault=fork, origin present. Fetch falls back to origin, push +# resolves fork. Resolved remotes differ -> plain push -> branch.remote stays +# UNSET, so the next fetch/rebase still targets origin (no silent clobber). +wc=$(make_clone tri-pushdefault fork) +git -C "$wc" config remote.pushDefault fork +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "pushDefault-tri: branch.remote NOT clobbered (stays unset)" "$(fetch_config "$wc")" "UNSET" +check "pushDefault-tri: next fetch still resolves origin" "$(cd "$wc" && bash "$RESOLVER")" "origin" +check "pushDefault-tri: fork received the branch" "$(has_branch "$WORKDIR/tri-pushdefault/fork")" "yes" +check "pushDefault-tri: origin did NOT receive the branch" "$(has_branch "$WORKDIR/tri-pushdefault/origin")" "no" + +# 5. Fetch ambiguous, push determinate: 2+ non-origin remotes, no origin, +# remote.pushDefault=fork. Fetch resolution fails (empty) -> treated as +# != push -> plain push to fork, never an abort, never a clobber. +wc=$(make_clone ambiguous fork upstream) +git -C "$wc" remote remove origin +git -C "$wc" config remote.pushDefault fork +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +push_exit=$? +check "fetch-ambiguous: push-branch.sh did not abort" "$push_exit" "0" +check "fetch-ambiguous: branch.remote not written" "$(fetch_config "$wc")" "UNSET" +check "fetch-ambiguous: fork received the branch" "$(has_branch "$WORKDIR/ambiguous/fork")" "yes" + +# 6. Local-only `.` upstream: branch.remote=. (tracks the local repo), merge +# tracks refs/heads/main, only origin configured. The resolver normalizes `.` +# to origin for BOTH modes, so a resolved-name-only gate would see fetch==push +# and fire `-u`, clobbering the deliberate local-only upstream. Because the +# upstream literally exists, the refined gate pushes plain: both keys survive. +wc=$(make_clone dot-upstream) +git -C "$wc" config "branch.${BRANCH}.remote" . +git -C "$wc" config "branch.${BRANCH}.merge" refs/heads/main +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "dot-upstream: branch.remote stays '.' (local-only preserved)" "$(fetch_config "$wc")" "." +check "dot-upstream: merge ref preserved (not rewritten by -u)" "$(merge_config "$wc")" "refs/heads/main" +check "dot-upstream: origin received the branch" "$(has_branch "$WORKDIR/dot-upstream/origin")" "yes" + +# 7. Merge-only tracking: branch.merge set, branch.remote UNSET (valid — Git +# defaults the remote to origin, so the branch tracks origin/main). Both +# resolvers fall back to origin, so a remote-only "no existing upstream" probe +# reads the branch as fresh and fires `-u`, replacing the merge ref. The gate +# must treat a set merge ref as an existing upstream too -> plain push -> the +# merge ref survives. +wc=$(make_clone merge-only) +git -C "$wc" config "branch.${BRANCH}.merge" refs/heads/main +(cd "$wc" && bash "$PUSH_BRANCH") >/dev/null 2>&1 +check "merge-only: branch.remote not written (stays unset)" "$(fetch_config "$wc")" "UNSET" +check "merge-only: merge ref preserved (not rewritten by -u)" "$(merge_config "$wc")" "refs/heads/main" +check "merge-only: origin received the branch" "$(has_branch "$WORKDIR/merge-only/origin")" "yes" + +echo +echo "Results: ${PASS} passed, ${FAIL} failed" +[[ $FAIL -eq 0 ]]