fix(source-control): gate push -u on absent-upstream + resolved-remote match (#442)

[kyle-sexton]

No linked issue — fix-forward of merged #763; the original issue #442 is already closed, and this residual was caught by post-merge review rather than filed.

Summary

Fix-forward on just-merged #763 (source-control /pull-request create flow, issue #442). An independent post-merge review empirically reproduced a residual silent clobber that #763 left behind.

The residual finding. create.md §2.4.1's conditional -u gate keyed on the literal branch.<name>.remote config being set. In the remote.pushDefault-only triangular shape — pushDefault=fork globally, branch.<name>.remote unset, fetch falling back to origin — the gate read "unset", took the -u bootstrap path, and git push -u <fork> rewrote branch.<name>.remote to the fork. The next fetch/rebase then silently targeted the fork instead of origin.

Reproduced before and after (two bare remotes, remote.pushDefault=fork, branch.<name>.remote unset):

Gate	branch.<name>.remote after push	next fetch resolves
old (merged in #763)	fork (clobbered)	fork — wrong base
this fix	unset (untouched)	origin — correct

Root-cause fix. The -u conditional moved out of the markdown prose into a new co-located scripts/push-branch.sh (§2.4.1 now delegates to it), so the resolve-fetch → resolve-push → conditional-push sequence is executable and testable rather than living only in a reference doc. The gate fires -u only when the branch has no existing upstream and its fetch and push remotes resolve to the same name; otherwise it pushes plain and writes no branch config. The normalized .-as-unset / \r-strip handling stays solely in resolve-remote.sh — the literal-unset probe reads the key raw (. is non-empty → "has an upstream").

Divergence from the review's literal prescription (disclosed). The review prescribed gating purely on resolved-remote equality (-u iff fetch == push). An independent Codex review of this change found that git push -u rewrites the branch's whole upstream — both branch.<name>.remote and branch.<name>.merge — so the pure-resolved gate still clobbered the merge ref of an already-tracked branch, and of a deliberate local-only . upstream (git branch --track . <ref>), whenever the resolved names happened to match. Requiring the upstream to be absent before bootstrapping (unset AND resolved-equal) closes that second corruption while still fixing the reported pushDefault-only case. This also improves #763's . handling: publishing a branch for a PR no longer mutates a deliberate local-only upstream. (Per the DEFER-FORBIDDEN carve-out, both Codex findings were folded in inline, not deferred.)

Test plan

• New push-branch.test.sh drives real bare-remote pushes across: pushRemote-triangular, pushDefault-only triangular, non-triangular (asserting the merge ref survives), fresh-branch bootstrap (asserting -u fired), local-only ., and fetch-ambiguous/push-determinate. 18/18 pass.
• Non-vacuity proven both directions: the suite fails 5 cases against the old literal-probe gate and 3 cases against the pure-resolved gate it replaces — so it pins the refined invariant, not merely the original bug. (This integration layer is exactly what resolve-remote.test.sh's resolver-only cases could not catch, which is why the clobber escaped fix(source-control): resolve remote dynamically in pull-request create flow (#442) #763.)
• resolve-remote.test.sh 15/15 (unchanged resolver), changelog parity (--check + --check-bump), markdownlint, shellcheck, shfmt — all green.

Merge posture

Fix-forward proceeding under operator momentum: the change is verified and ready, but the agent will not merge it. The merge decision is delegated to the operator. A veto window is open — object here before merge if the unset AND resolved-equal refinement (vs the review's pure-resolved prescription) warrants discussion; otherwise it may be merged.

Related

• Refs source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 — original low-severity finding (closed by fix(source-control): resolve remote dynamically in pull-request create flow (#442) #763); this fix-forward addresses the residual clobber that survived it.
• fix(source-control): resolve remote dynamically in pull-request create flow (#442) #763 — the PR this follows forward; introduced the conditional -u gate whose literal-probe form this replaces.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 4m 9s —— View job

---

Security Review

• Gather PR diff and changed files
• Review push-branch.sh for shell injection / quoting issues
• Review push-branch.test.sh for unsafe test patterns
• Review create.md and plugin.json for security-relevant changes
• Synthesize and report findings

---

Scope: push-branch.sh (new), push-branch.test.sh (new), create.md (modified delegation), plugin.json (version bump), CHANGELOG.md (prose only). The pre-existing resolve-remote.sh was read for context on what PUSH_REMOTE/FETCH_REMOTE can contain.

---

Findings

No exploitable security vulnerabilities found. Detailed notes per surface below.

push-branch.sh — shell quoting and injection

All external data flows into shell commands through properly double-quoted variables:

PUSH_REMOTE=$(bash "$RESOLVER" --push "$BRANCH") || exit 1   # L55
FETCH_REMOTE=$(bash "$RESOLVER" "$BRANCH" 2>/dev/null)       # L56
EXISTING_UPSTREAM=$(git config "branch.${BRANCH}.remote" 2>/dev/null)  # L57
git push -u "$PUSH_REMOTE" "$BRANCH"   # L60
git push "$PUSH_REMOTE" "$BRANCH"      # L62

No unquoted expansions, no eval, no $() inside unquoted contexts.

Flag injection via PUSH_REMOTE: resolve-remote.sh only emits a remote name from git config or the string "origin". Git enforces that remote names satisfy ref-name constraints — they cannot start with -, contain whitespace, or embed shell metacharacters. The [[ "$FETCH_REMOTE" == "$PUSH_REMOTE" ]] comparison is in a [[...]] context (no glob expansion on the right side). Confirmed safe.

Flag injection via BRANCH: Branch names are either received from git branch --show-current (already constrained by git's ref rules — no spaces, no leading -) or from the caller argument ${1:-}. The value is always passed inside "$BRANCH" so even a pathological value like -f or --force would be passed as a literal argument string, not parsed as a flag. Confirmed safe.

git config "branch.${BRANCH}.remote" key interpolation: BRANCH is embedded in a double-quoted string that becomes a single argument to git config. Git config parses the key branch.<subsection>.remote where the subsection is everything between the first and last dot. Branch names with embedded dots (e.g. feat/1.0) produce a valid subsection. Characters that git's ref rules forbid (newlines, null bytes, colons, etc.) cannot appear in any branch name git itself issues. An adversarially-crafted value cannot escape into a second argument because bash quoting prevents word-splitting. If the key is malformed, git config exits non-zero, the 2>/dev/null suppresses the error, and EXISTING_UPSTREAM is empty — which is the safe/conservative default (plain push).

Empty-BRANCH edge case — safe failure, not a security issue

If the script is invoked with no argument while in detached HEAD state, git branch --show-current returns empty, and BRANCH stays empty throughout. This causes a divergence:

• The resolver is called with "" and resolves the branch internally from git branch --show-current — likely also empty, likely exiting non-zero → || exit 1 aborts. If somehow the resolver returns a remote anyway, the script continues.
• git config "branch..remote" is an invalid key → returns empty → EXISTING_UPSTREAM="".
• git push "$PUSH_REMOTE" "" — git rejects the empty refspec (error: src refspec does not match any).

The failure path is a non-zero exit, not a corrupt push or an inadvertent -u write. Not a security issue; a safe error. Not in scope of the PR's stated test matrix (all six test cases use an explicit branch).

push-branch.test.sh — test environment handling

• Temp dir via mktemp -d with trap 'rm -rf "$WORKDIR"' EXIT — correct cleanup pattern.
• All path variables ($wc, $base, $WORKDIR) are double-quoted at point of use.
• Git operations are scoped with git -C "$dir" not cd+unguarded paths.
• test@example.com / Test are local-repo-only config writes; no credential leakage.
• The has_branch, fetch_config, merge_config helpers pass controlled, script-internal paths to git. No user-controlled data flows through these.

create.md — delegation to push-branch.sh

Old inline snippet removed; replaced with:

bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1

CLAUDE_PLUGIN_ROOT is set by the Claude Code plugin runtime to the plugin cache directory — same trust level as any other ${CLAUDE_PLUGIN_ROOT}/… reference throughout the plugin. The || exit 1 correctly propagates failures. Net security change: the gate logic moved from untestable markdown prose into an executable script with an integration test suite. This is strictly a security improvement — the behavior is now verifiable rather than advisory.

plugin.json / CHANGELOG.md

Version bump and prose only. No security-relevant changes.

---

Summary

The PR introduces no new injection surfaces, no credential handling, no elevated permissions, and no external network egress beyond the git push that was already present. The logic change (the refined unset AND resolved-equal upstream gate) is the security-relevant improvement this PR was written to deliver, and the analysis confirms it correctly closes the described upstream-clobber vector without introducing new ones.

[claude]

Claude finished @kyle-sexton's task in 3m 12s —— View job

---

PR #783 Review

• Gather context and understand the request
• Read changed files
• Review push-branch.sh (new script)
• Review push-branch.test.sh (new test)
• Review create.md (updated reference)
• Review plugin.json and CHANGELOG.md
• Post findings

---

Summary

This is a well-constructed fix-forward on a residual silent-clobber bug that survived PR #763. The logic, tests, and documentation are all sound. I have a few minor observations but no blocking concerns.

---

push-branch.sh

Logic is correct. The two-condition gate — branch.<name>.remote literally unset AND FETCH_REMOTE == PUSH_REMOTE — correctly handles all four problematic shapes described in the PR:

Shape	Old result	New result
pushDefault=fork, no branch.remote	-u clobbers → fork	plain push, upstream stays unset
Already-tracked branch	-u rewrites merge ref	plain push, merge ref preserved
Deliberate . upstream	-u clobbers .	plain push, . preserved
Ambiguous fetch (empty FETCH_REMOTE)	-u (wrong)	plain push (empty ≠ any push remote)

Intentional set -uo pipefail without -e is correct. The script relies on explicit exit handling (|| exit 1 for push resolution, silent degradation for fetch resolution). An empty FETCH_REMOTE from a failed resolver call is a valid outcome that triggers plain push — the right behavior for the ambiguous-fetch shape. This is consistent with resolve-remote.sh.

Minor: BRANCH empty-string path under detached HEAD. If git branch --show-current returns empty (detached HEAD), BRANCH stays empty, git config "branch..remote" is a no-op (correctly returns empty), and the resolver is called with an empty branch arg. The resolver then re-calls git branch --show-current itself and either resolves origin or errors. No regression from prior behavior — this edge case is out of scope — but worth noting for future hardening.

push-branch.sh L50–53

Minor: bash "$RESOLVER" rather than direct invocation. Both the script and test invoke the resolver via bash rather than directly (even though both have #!/usr/bin/env bash and are marked 100755). The test checks for executability; the script doesn't. This is harmless — bash invocation bypasses the shebang and works regardless of execute bit — but slightly inconsistent.

---

push-branch.test.sh

Test coverage is excellent. 18 assertions, 6 real-bare-remote scenarios. The suite pins the refined invariant in both directions — it would fail against an always-plain-push implementation (case 1 expects -u to fire) AND against an always--u or resolved-name-only implementation (cases 2 and 6 assert the merge ref survives).

has_branch path construction is correct. $1.git path convention is consistent throughout: callers pass bare paths (e.g. $WORKDIR/bootstrap/origin) and the function appends .git. ✓

Case 5 exit-code check is well-structured. Capturing push_exit=$? separately after the subshell (line 141) correctly validates that a failed fetch resolution produces exit 0 from the script (degraded to plain push, not abort). ✓

One cosmetic note: The fetch_config / merge_config helpers use || echo UNSET (capital) while the check assertions compare against "UNSET" — consistent, but the sentinel string is easy to misread as a shell keyword. No functional issue.

push-branch.test.sh L84–90

---

create.md

The 18-line inline bash block is cleanly replaced by a single delegation call:

bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1

This is the right call — the gate logic belonged in a testable script, not a reference doc. The inline comment concisely names the two corruptions being closed without duplicating the full rationale (which lives correctly in the script header). ✓

---

plugin.json / CHANGELOG.md

• Version bump 0.15.4 → 0.15.5 is correct (patch-level fix). ✓
• Changelog entry is detailed and accurate. ✓
• File permissions 100755 for both new scripts. ✓

---

Verdict

Approve. The unset AND resolved-equal refinement over the pure-resolved gate is the right call for the reasons the PR articulates — a pure resolved-equal gate would still clobber the merge ref on an already-tracked branch or a . upstream. No blocking issues. The minor observations above are cosmetic or future-hardening material.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 873c7aec05

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 53s —— View job

---

Security Review — PR #783

• Gather context and understand the request
• Fetch PR diff and changed files
• Review push-branch.sh (new script) for injection, quoting, flag-injection
• Review push-branch.test.sh (new test) for unsafe patterns
• Review create.md delegation change
• Review plugin.json and CHANGELOG.md
• Synthesize findings and post report

---

Scope: push-branch.sh (new), push-branch.test.sh (new), create.md (delegation update), plugin.json (version bump), CHANGELOG.md (prose only). resolve-remote.sh read for context on what PUSH_REMOTE/FETCH_REMOTE can contain.

---

Findings

No exploitable security vulnerabilities found. Detail per surface below.

---

push-branch.sh — shell quoting and injection

All external data flows through properly double-quoted variables:

PUSH_REMOTE=$(bash "$RESOLVER" --push "$BRANCH") || exit 1   # L55
FETCH_REMOTE=$(bash "$RESOLVER" "$BRANCH" 2>/dev/null)       # L56
EXISTING_UPSTREAM=$(git config "branch.${BRANCH}.remote" 2>/dev/null)  # L57
git push -u "$PUSH_REMOTE" "$BRANCH"   # L60
git push "$PUSH_REMOTE" "$BRANCH"      # L62

No unquoted expansions, no eval, no $() inside unquoted context.

Flag injection via PUSH_REMOTE: resolve-remote.sh only emits a remote name resolved from git config or the hardcoded string "origin". Git enforces that remote names satisfy ref-name constraints — they cannot start with -, contain whitespace, or embed shell metacharacters. The [[ "$FETCH_REMOTE" == "$PUSH_REMOTE" ]] comparison is in a [[ ]] context (no glob expansion on the right-hand side). Safe.

Flag injection via BRANCH: Branch names either come from git branch --show-current (constrained by git's ref rules — no spaces, no leading -, no +) or from the caller's $1. In the production call path (create.md delegates with no arguments), BRANCH always comes from git. Even if called with $1, the value passes through "$BRANCH" so a value like --force becomes a literal string argument to git push, not a parsed flag — git push origin --force vs git push origin "--force" differs only if word-splitting occurs, which double-quoting prevents. Safe.

git config "branch.${BRANCH}.remote" key interpolation: BRANCH is embedded in a double-quoted string that becomes a single argument. Git parses the key branch.<subsection>.remote where the subsection is delimited by the first and last dot. Characters that git's ref rules forbid (null bytes, ^, :, [, ?, *, .., etc.) cannot appear in any branch name git itself produces. The git config parser handles embedded dots in branch names (e.g. feat/1.0) correctly as subsection content. If the key is malformed, git config exits non-zero, stderr is suppressed, and EXISTING_UPSTREAM stays empty — the safe/conservative default that triggers a plain push. Safe.

SCRIPT_DIR / RESOLVER path construction: L47–48 use the standard dirname "${BASH_SOURCE[0]}" + pwd idiom. RESOLVER is derived entirely from the script's own path — no user-controlled data flows into it. Safe.

set -uo pipefail without -e: Intentional. The script relies on explicit || exit 1 for the mandatory push-resolution path and silent degradation for fetch resolution. -u catches unset-variable bugs at development time. Correct pattern.

---

push-branch.test.sh — test environment safety

L37 mktemp -d with L38 trap 'rm -rf "$WORKDIR"' EXIT — correct cleanup. All path variables double-quoted at point of use. Git operations scoped with git -C "$dir". test@example.com / Test are local-to-repo config writes; no credential leakage. No user-controlled data flows through any test helper. Safe.

---

create.md — delegation to push-branch.sh

Old 18-line inline bash block replaced with:

bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1

${CLAUDE_PLUGIN_ROOT} is set by the Claude Code plugin runtime to the plugin cache directory — same trust level as every other ${CLAUDE_PLUGIN_ROOT}/… reference in this plugin. The || exit 1 correctly propagates failure. Net security change: gate logic moved from untestable markdown prose into an executable script with an integration test suite. Strictly a security improvement — behavior is now verifiable rather than advisory.

---

plugin.json / CHANGELOG.md

Version bump and prose only. No security-relevant changes.

---

Codex P2 finding (pre-existing comment)

The Codex review flagged a case where branch.<name>.merge is set but branch.<name>.remote is unset — the gate reads the upstream as absent and fires -u, which overwrites the merge ref. This is a correctness concern (a form of the same upstream-clobber the PR sets out to fix), not an injection or authorization issue. It does not affect confidentiality, integrity of push destinations, or access control. It is out of scope for this security review but would be a legitimate bug-level follow-up.

---

Summary

The PR introduces no new injection surfaces, no credential handling, no elevated permissions, and no external network egress beyond the git push already present in the original code. The refined unset AND resolved-equal upstream gate is the security-relevant improvement this PR was written to deliver, and analysis confirms it correctly closes the upstream-clobber vector without introducing new ones.

[claude]

Claude finished @kyle-sexton's task in 2m 25s —— View job

---

PR #783 Review

• Gather context
• Read push-branch.sh (new script)
• Read push-branch.test.sh (new tests)
• Read create.md (updated delegation)
• Evaluate Codex P2 finding on branch.<name>.merge
• Synthesize findings and post review

---

Summary

The core fix and its test harness are well-constructed. The unset AND resolved-equal gate is the right refinement over the pure-resolved gate. One confirmed correctness defect remains (the Codex P2 finding) and one nit; details below.

---

🔴 Important — Merge-only tracking config not guarded: -u clobbers branch.<name>.merge

File: push-branch.sh L57–60

The gate probes only branch.<name>.remote to decide whether to bootstrap with -u:

EXISTING_UPSTREAM=$(git config "branch.${BRANCH}.remote" 2>/dev/null)

if [[ -z "$EXISTING_UPSTREAM" && "$FETCH_REMOTE" == "$PUSH_REMOTE" ]]; then
  git push -u "$PUSH_REMOTE" "$BRANCH"

branch.<name>.merge can be set without branch.<name>.remote — for example, a branch configured to track refs/heads/main as its integration target via git config branch.feat/x.merge refs/heads/main without setting a remote. In that state EXISTING_UPSTREAM is empty, the gate sees "no upstream", and git push -u fires, rewriting branch.<name>.merge from refs/heads/main to refs/heads/feat/x. This is exactly the merge-ref clobber the PR's stated invariant promises to prevent ("the gate fires -u only when BOTH hold: branch..remote is literally unset…").

The fix is one additional probe:

EXISTING_UPSTREAM=$(git config "branch.${BRANCH}.remote" 2>/dev/null)
EXISTING_MERGE=$(git config "branch.${BRANCH}.merge" 2>/dev/null)

if [[ -z "$EXISTING_UPSTREAM" && -z "$EXISTING_MERGE" && "$FETCH_REMOTE" == "$PUSH_REMOTE" ]]; then
  git push -u "$PUSH_REMOTE" "$BRANCH"

A matching test case would be:

# merge-only partial tracking: branch.merge set, branch.remote unset.
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 (plain push)" "$(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"

Why not purely academic: the Codex review on this PR already flagged this; the scenario arises with direct git config manipulation and some plumbing tools. The gate's own header comment says a "deliberate local-only . upstream" is the motivation for not relying solely on resolved-name equality — this is the analogous case where a partial merge-ref config exists without an explicit remote.

Fix this →

---

🟡 Nit — Script header comment doesn't mention the branch.<name>.merge-only gap

The header's coverage of the gate's two corruption shapes (lines 17–25) describes corruption 2 as "If branch..remote is already set… -u replaces both keys." That framing covers the . case and the already-tracked case but would need a third bullet to cover the merge-only partial config. Minor — but since the header is the canonical rationale document for this gate, it should be updated alongside the fix above.

---

Everything else — no concerns

Surface	Verdict
Quoting / injection in push-branch.sh	Clean — all external data passes through double-quoted variables, [[…]] comparison context, no eval
set -uo pipefail without -e	Intentional and correct — failed fetch resolution degrades to empty, not abort
PUSH_REMOTE resolution with || exit 1, FETCH_REMOTE without	Correct — push must be determinate; ambiguous fetch is a valid degraded case
Test cases 1–6, 18 assertions	Cover the 4 stated shapes plus bootstrap and fetch-ambiguous; non-vacuity proven in both directions as claimed
create.md delegation to push-branch.sh	Clean replacement; inline comment accurately names the two corruptions being closed
plugin.json version bump 0.15.4 → 0.15.5	Correct patch increment
Shell quoting in push-branch.test.sh	All path vars double-quoted at use; git -C "$dir" scoping throughout; mktemp + trap cleanup correct

---

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @kyle-sexton's task in 1m 56s —— View job

---

Security Review — PR #783

• Gather context and understand the request
• Fetch PR diff and read changed files
• Review push-branch.sh for injection, quoting, flag-injection
• Review push-branch.test.sh for unsafe test patterns
• Review create.md delegation change
• Synthesize findings and post report

---

Scope: push-branch.sh (new), push-branch.test.sh (new), create.md (delegation update), plugin.json (version bump), CHANGELOG.md (prose only). resolve-remote.sh read for context on what PUSH_REMOTE/FETCH_REMOTE can contain.

---

Finding: None — no exploitable security vulnerabilities found

The HEAD commit (1a134d2) already incorporates the EXISTING_MERGE guard that prior review rounds flagged as missing. Per-surface detail below.

---

push-branch.sh — quoting and injection surfaces

All external data flows through properly double-quoted variables:

PUSH_REMOTE=$(bash "$RESOLVER" --push "$BRANCH") || exit 1   # L64
FETCH_REMOTE=$(bash "$RESOLVER" "$BRANCH" 2>/dev/null)       # L65
EXISTING_REMOTE=$(git config "branch.${BRANCH}.remote" 2>/dev/null)  # L66
EXISTING_MERGE=$(git config "branch.${BRANCH}.merge" 2>/dev/null)    # L67
git push -u "$PUSH_REMOTE" "$BRANCH"   # L70
git push "$PUSH_REMOTE" "$BRANCH"      # L72

No unquoted expansions, no eval, no $() inside unquoted context.

Flag injection via PUSH_REMOTE: resolve-remote.sh only emits a remote name resolved from git config or the hardcoded string "origin". Git enforces that remote names satisfy ref-name constraints — they cannot start with -, contain whitespace, or embed shell metacharacters. The [[ "$FETCH_REMOTE" == "$PUSH_REMOTE" ]] comparison uses [[...]] (no right-hand glob expansion). Safe.

Flag injection via BRANCH: In the production call path (create.md invokes the script with no arguments), BRANCH comes entirely from git branch --show-current, which is bounded by git's own ref rules — no leading -, no spaces. Even in the direct-invocation path ($1), the value is always passed through "$BRANCH" — word-splitting cannot occur, so an adversarial value like --force would be a literal positional argument, not parsed as a git flag by bash. Git itself would then reject it as an invalid refspec. Safe.

git config "branch.${BRANCH}.remote" and "branch.${BRANCH}.merge" key interpolation: BRANCH is embedded in a double-quoted string passed as a single argument. Git's key parser reads the subsection between the first and last dot; embedded dots in branch names (e.g. feat/1.0) are valid subsection content. Characters that git's ref rules forbid cannot appear in any branch git itself produces. If the key is malformed, git config exits non-zero, stderr is suppressed, and the relevant variable stays empty — a safe/conservative outcome that triggers plain push. Safe.

BRANCH empty under detached HEAD: If the script is called with no $1 in detached HEAD state, git branch --show-current returns empty. git config "branch..remote" and "branch..merge" are invalid keys — both exit non-zero silently, so both upstream variables stay empty. The resolver is called with "" and either resolves a remote or exits 1 (which propagates via || exit 1). If a remote is resolved, git push "$PUSH_REMOTE" "" fails with "src refspec does not match any" — a non-zero exit, not a silent clobber or unauthorized push. Not a security issue; a safe error.

SCRIPT_DIR / RESOLVER construction (L56–57): Uses the standard dirname "${BASH_SOURCE[0]}" + pwd idiom. RESOLVER is derived entirely from the script's own install path — no user-controlled data flows into it. Safe.

set -uo pipefail without -e: Intentional and correct. Failed fetch resolution degrades to an empty FETCH_REMOTE (not abort); failed push resolution aborts via explicit || exit 1. The gate then fires plain push (empty ≠ any real push remote), which is the correct behavior for the fetch-ambiguous shape. Safe.

---

push-branch.test.sh — test environment safety

mktemp -d with trap 'rm -rf "$WORKDIR"' EXIT — correct cleanup. All path variables double-quoted at point of use. Git operations scoped with git -C "$dir". test@example.com / Test are local-to-test-repo config writes; no credential leakage. The $BRANCH used in has_branch (L79) is the script-internal constant "feat/x", not user-controlled. No external data flows through any test helper. Safe.

---

create.md — delegation to push-branch.sh

The 18-line inline bash block is replaced with:

bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1

${CLAUDE_PLUGIN_ROOT} is set by the Claude Code plugin runtime to the plugin cache directory — same trust level as every other ${CLAUDE_PLUGIN_ROOT}/… reference in this plugin. The || exit 1 correctly propagates failure. Net security effect: gate logic moved from untestable markdown prose into an executable script with an integration test suite. Strictly a security improvement.

---

Prior-round P2 finding — now closed

The Codex P2 finding and subsequent review comment both flagged that the gate probed only branch.<name>.remote, leaving a branch.<name>.merge-only config shape able to trigger -u and clobber the merge ref. The current HEAD (L66–69) checks both keys and the test suite (case 7, merge-only tracking) pins the fix in both directions. Closed.

plugin.json / CHANGELOG.md

Version bump and prose only. No security-relevant changes.

---

Summary

The PR introduces no new injection surfaces, no credential handling, no elevated permissions, and no external network egress beyond the git push already present. The unset AND resolved-equal upstream gate is the security-relevant improvement this PR was written to deliver, and both upstream keys (branch.<name>.remote and branch.<name>.merge) are now correctly guarded. No security issues found.

[claude]

Claude finished @kyle-sexton's task in 2m 25s —— View job

---

PR #783 Review

• Gather context and understand the request
• Read changed files at HEAD (1a134d2)
• Verify the branch.<name>.merge fix (Codex P2 / prior Claude finding)
• Review push-branch.sh for correctness and security
• Review push-branch.test.sh for coverage and correctness
• Review create.md delegation change
• Synthesize and post findings

---

Status of prior findings

Codex P2 / prior Claude review (branch.<name>.merge partial tracking) — RESOLVED. The HEAD commit 1a134d2 adds the EXISTING_MERGE probe and test case 7, closing the gap both reviewers identified. The gate now reads:

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

This is the correct fix. A branch with branch.<name>.merge=refs/heads/main and no branch.<name>.remote now produces a non-empty EXISTING_MERGE, so the gate skips -u and the merge ref is preserved.

---

push-branch.sh — correctness and security

Gate logic is sound across all seven shapes:

Shape	EXISTING_REMOTE	EXISTING_MERGE	Fetch == Push?	Gate fires -u?
Fresh branch bootstrap	∅	∅	yes (both origin)	✓ correct
Already-tracked branch	origin	refs/heads/main	yes	no — preserves merge ref ✓
pushRemote-triangular	upstream	(set)	no	no ✓
pushDefault-only triangular (#442)	∅	∅	no (origin≠fork)	no ✓
Fetch-ambiguous	∅	∅	no (∅ ≠ fork)	no ✓
Local-only .	.	refs/heads/main	(moot)	no — "." is non-empty ✓
Merge-only tracking (new)	∅	refs/heads/main	yes	no — EXISTING_MERGE non-empty ✓

Raw key reads are correct. The header explains the design clearly: upstream-absent probes read both keys raw (no .-normalization, no \r-strip). Any non-empty value means "has an upstream" → plain push. That normalization stays once, in resolve-remote.sh. This is the right architecture — no duplication.

Quoting / injection. No unquoted expansions, no eval. PUSH_REMOTE and BRANCH come from git (ref-name-constrained) or from the caller but always pass through "$VAR". git config "branch.${BRANCH}.remote" embeds BRANCH in a double-quoted string (single git argument), so no word-splitting occurs. [[ "$FETCH_REMOTE" == "$PUSH_REMOTE" ]] uses [[...]] context — no glob expansion on the right side. The prior security reviews confirmed this; the HEAD commit doesn't add any new surfaces.

set -uo pipefail without -e is intentional and correct. Explicit || exit 1 on push resolution, silent degradation for fetch resolution. -u catches unset-variable bugs at development time.

Detached HEAD / empty BRANCH. If git branch --show-current returns empty and no argument was passed, BRANCH stays empty. git config "branch..remote" is an invalid key → non-zero exit, suppressed, EXISTING_REMOTE="". Same for EXISTING_MERGE. Then bash "$RESOLVER" --push "" will almost certainly fail (the resolver also calls git branch --show-current) → || exit 1 aborts safely. Not a corruption risk; a safe error. Out of scope for the PR's stated test matrix.

Minor nit: Both push-branch.sh and push-branch.test.sh invoke the resolver via bash "$RESOLVER" rather than directly (both scripts are 100755 with a shebang). This is harmless — bash invocation bypasses the shebang and is equivalent — but slightly inconsistent with direct invocation. Noted by the prior reviewer as well; truly cosmetic.

---

push-branch.test.sh — coverage and correctness

Test case 7 (merge-only tracking) is well-constructed. It sets branch.merge=refs/heads/main without branch.remote, runs push-branch.sh, and asserts both that branch.remote stays UNSET and the merge ref survives. This is exactly the Codex P2 scenario, now pinned in the test matrix. Three assertions for the new case, consistent with the other cases.

Non-vacuity is proven in three directions:

• Case 1 (bootstrap) fails against an always-plain-push implementation.
• Cases 2, 6 fail against an always--u or resolved-name-only gate.
• Case 7 (new) fails against a remote-only "no upstream" probe that misses merge-only tracking.

Test infrastructure is correct throughout. mktemp -d + trap 'rm -rf "$WORKDIR"' EXIT, git -C "$dir" scoping, double-quoted paths at point of use, test@example.com/Test local-only config writes. has_branch() consistently appends .git and callers pass bare-repo base paths without .git — the convention is uniform.

Case 5 exit-code capture (push_exit=$? after the subshell, then check "..." "$push_exit" "0") correctly validates that a failed fetch resolution exits 0 (degraded to plain push) rather than aborting. ✓

Count: 21 assertions across 7 real-bare-remote scenarios (up from 18/6 in the prior reviews, adding 3 assertions for case 7). The PR description says 18/18 — the count is now 21/21 with the merge-only case.

---

create.md

Old 18-line inline bash block cleanly replaced by a one-line delegation:

bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/push-branch.sh" || exit 1

The inline comment concisely names both corruptions being closed and points to the script header for full rationale. ${CLAUDE_PLUGIN_ROOT} is the standard plugin path — correct trust level, no ../ reach-out. || exit 1 propagates failures. ✓

---

plugin.json / CHANGELOG.md

Version bump 0.15.4 → 0.15.5 is the correct patch increment for a bug fix. The [0.15.5] changelog entry is comprehensive and accurate, covering the pushDefault-only clobber, the merge-ref overwrite family (already-tracked, ., merge-only), and the integration-test addition. ✓

---

Verdict

Approve. The HEAD commit resolves the one confirmed defect flagged by two independent prior reviewers. The unset AND resolved-equal gate — now requiring both branch.<name>.remote and branch.<name>.merge to be absent before bootstrapping — is the correct implementation. All seven shapes are exercised with real bare-remote pushes; non-vacuity is proven in three directions. No remaining correctness, security, or alignment issues.