fix(source-control): resolve remote dynamically in pull-request create flow (#442)

[kyle-sexton]

Summary

/pull-request's create.md reference hardcoded the remote name origin in the §2.2 rebase
(git fetch origin, origin/$DEFAULT_BRANCH) and the §2.4.1 push (git push -u origin <branch>),
so any consumer whose remote is not named origin — a repo cloned with git clone -o <name>, or a
fork-based multi-remote setup — would break. That is a baked repo assumption the convention-resolution
ladder forbids (issue #442, work-readiness sweep vs docs/PLUGIN-PHILOSOPHY.md).

Fix

Both sites now resolve the remote with the same candidate-priority idiom the toolchain linters use:
the current branch's configured remote (branch.<name>.remote, a local-only . upstream treated as
unset), else origin, else the sole configured remote. In §2.2 every origin/$DEFAULT_BRANCH
occurrence (fetch, merge-base, rev-parse, rev-list, rebase, the progress echo, and the
merge-vs-rebase / skip-condition prose) now reads $REMOTE/$DEFAULT_BRANCH, and ORIGIN_DEFAULT is
renamed REMOTE_DEFAULT. On the common path — a single-remote repo, or a fresh feature branch with no
branch.<name>.remote yet — both sites still resolve to origin, so current behavior is preserved
exactly. Doc-only change to create.md (no scripts touched); plugin.json bumped to 0.15.4 with a
matching CHANGELOG entry.

Verification

• Rebased onto current origin/main (0.15.3); resolved the plugin.json + CHANGELOG version
  conflicts, re-bumping to 0.15.4 from main's head rather than the stale branch-point value.
• create.md auto-merged cleanly against main and correctly integrates the source-control: PR-body "Generated with Claude Code" attribution is unconditional — no trailer_policy-equivalent seam #439 attribution line.
• scripts/check-changelog-parity.sh --check and --check-bump origin/main: green.
• plugin.json parses and reports version 0.15.4.
• No #NNN issue-number references in the added create.md code comments (comment-hygiene gate).
• Confirmed no other open PR touches plugins/source-control/ before opening.
• Remaining hygiene/lint gates (skill-portability, cross-plugin-drift, silent-skips) run in CI on
  this PR.

Closes #442

Related

• The same origin hardcoding still lives in the pull-request merge.md and the babysit-prs
  references; deferred to a follow-up (noted in the CHANGELOG entry). Triangular fork flows that fetch
  a separate upstream while pushing to a fork remain out of scope — branch.<name>.remote tracks
  the push remote, not upstream.

[claude]

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

---

Security Review

• Gather context and PR diff
• Read changed files
• Audit remote-resolution logic for injection / trust issues
• Check plugin.json and CHANGELOG for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/reference/create.md (doc-only), plugin.json (version bump), CHANGELOG.md. No scripts or workflows changed.

Shell injection — §2.2 and §2.4.1 remote-resolution blocks

Both new blocks use $(git branch --show-current | tr -d '\r') embedded inside a double-quoted string argument passed to git config:

git config "branch.$(git branch --show-current | tr -d '\r').remote"

This is safe. Bash $(...) inside double quotes substitutes the result as literal text; the result is not re-evaluated for shell metacharacters. A branch name containing ;, $(), or other metacharacters produces a string that becomes a single argument to git config — it never reaches a shell parser again. All downstream uses of $REMOTE, $PUSH_REMOTE, and $(git branch --show-current) are consistently double-quoted.

Remote trust model — head -1 fallback

else REMOTE=$(git remote | head -1); fi

The fallback when origin is absent picks the first remote git remote returns. git remote output order is insertion-ordered (.git/config parse order), so an actor who can write .git/config could insert a remote named aaa-evil pointing to a malicious URL and have it selected here. Confidence: LOW — exploiting this requires prior write access to .git/config, at which point the attacker could just replace origin's URL, making this no worse than the hardcoded-origin baseline it replaces. Not a new vulnerability.

Empty-$REMOTE edge case

If the repo has no remotes at all, git remote | head -1 returns nothing and $REMOTE stays empty. Subsequent git fetch "" "$DEFAULT_BRANCH" and git rev-parse "/$DEFAULT_BRANCH" will fail with an error, not silently misbehave. Not a security issue — a UX robustness gap only.

plugin.json / CHANGELOG

Version bump (0.15.3 → 0.15.4) and prose changelog entry. No new permissions, dependencies, egress, or execution surfaces.

---

No security issues found. The remote-resolution logic uses correct quoting throughout, does not expand new attack surfaces beyond the existing .git/config trust model, and introduces no new injection, credential, or trust-delegation concerns.

[claude]

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

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Review changed files
• Analyze logic correctness of remote-resolution idiom
• Check alignment with PLUGIN-PHILOSOPHY.md / MIGRATION-PLAYBOOK.md
• Post findings

---

This is a clean, focused doc-only fix that correctly addresses issue #442. The version bump (0.15.3 → 0.15.4) is appropriate for a patch fix, and the CHANGELOG entry is thorough. Two things worth noting:

---

Finding 1 — §2.2 and §2.4.1 use inconsistent fallback logic (narrow edge case)

The two resolution sites use different strategies when branch.<name>.remote is unset and origin doesn't exist:

Site	Primary	Fallback if no branch.remote	Fallback if no origin
§2.2 (fetch)	branch.<name>.remote	origin	git remote | head -1
§2.4.1 (push)	branch.<name>.remote	origin	none — stays origin

In a repo cloned with git clone -o vendor where no branch.<name>.remote is yet set (an unpushed feature branch):

§2.2  → REMOTE=vendor  → git fetch vendor main  ✓
§2.4.1 → PUSH_REMOTE=origin → git push -u origin <branch>  ✗ (remote not found)

The LLM executing these in sequence would successfully rebase in §2.2 and then fail at the push in §2.4.1. The PR description states "both sites now resolve the remote with the same candidate-priority idiom" — that's accurate for the happy path and for repos with an origin remote, but they diverge when origin is absent and no tracking remote is configured.

The head-1 fallback on pushes is arguably intentional (pushing to an unknown first remote is riskier than fetching from it), but the asymmetry isn't documented. Either:

• Document the intentional difference in the §2.4.1 comment (the current comment only explains the "." case and the fresh-branch case), or
• Apply the head-1 fallback to §2.4.1 as well (consistent with §2.2, and git push -u sets upstream so later pushes resolve correctly)

See §2.2 resolution · See §2.4.1 resolution

---

Finding 2 — CHANGELOG overstate: "same candidate-priority idiom the toolchain linters already use"

The priority order (branch-remote → origin → sole remote) matches the toolchain, but the toolchain idiom is meaningfully different:

# Toolchain (check/lint SKILL.md lines 60-72):
# Builds a deduplicated candidate list, then loops and validates each
# with git rev-parse --verify --quiet "refs/remotes/$CANDIDATE/$BRANCH"
# before accepting — skips remotes never fetched locally.
CANDIDATES=$( { [[ -n "$TRACKED" ]] && echo "$TRACKED"; git remote | grep -qx origin && echo origin; git remote; } | awk 'NF && !seen[$0]++' )
while IFS= read -r CANDIDATE; do
  ...
  if [[ -n "$BRANCH" ]] && git rev-parse --verify --quiet "refs/remotes/$CANDIDATE/$BRANCH" >/dev/null; then
    REMOTE=$CANDIDATE DEFAULT_BRANCH=$BRANCH; break
  fi
done <<< "$CANDIDATES"

The PR's §2.2 doesn't validate that the resolved remote has a locally cached tracking ref — it just picks the first viable name and calls git fetch. That's fine here (fetch updates the ref), but calling it "the same idiom" is an overstatement. The word "idiom" implies structural parity that doesn't exist. Consider softening to "the same candidate-priority ordering" or "the same resolution ladder."

See CHANGELOG entry

---

Everything else looks good

• The "." upstream guard is correct and consistent with the toolchain's own [[ "$TRACKED" == "." ]] && TRACKED="" — both sites handle it.
• tr -d '\r' on both sites is consistent and correct for Windows compat.
• All origin/$DEFAULT_BRANCH occurrences in §2.2 prose and code are updated to $REMOTE/$DEFAULT_BRANCH, including the merge-vs-rebase advisory and skip-condition paragraph.
• The §2.4.1 scope is correctly narrower than §2.2 (push-remote vs fetch-remote semantics), and the comment clearly explains the fresh-branch common-path.
• No #NNN references in code comments (comment-hygiene gate passes on inspection).
• Version bump and CHANGELOG format are correct.

Finding 1 is the only functional concern; Finding 2 is a documentation accuracy note. Neither blocks merge if the team is comfortable with the documented limitation on triangular setups (which the PR explicitly calls out of scope).

[claude]

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

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Read changed files
• Audit remote-resolution logic and edge cases
• Check alignment with PLUGIN-PHILOSOPHY.md
• Post findings

---

Scope: create.md (doc-only), plugin.json (version bump 0.15.3 → 0.15.4), CHANGELOG.md. No scripts, hooks, or workflows changed.

The fix correctly addresses the hardcoded origin assumption that PLUGIN-PHILOSOPHY.md §"Two-lane convention posture" classifies as a defect. The candidate-priority chain (branch.<name>.remote → origin → sole remote) is the right shape. A few things to flag:

---

Finding 1 — §2.2 and §2.4.1 use asymmetric fallback chains (functional gap)

The two resolution blocks diverge in the last fallback:

Site	Primary	Fallback 1	Fallback 2
§2.2 (fetch)	branch.<name>.remote	origin	git remote | head -1
§2.4.1 (push)	branch.<name>.remote	origin	none — stays origin

In a repo cloned with git clone -o vendor (no origin remote, no upstream configured yet on a new branch):

• §2.2 resolves → vendor → git fetch vendor main succeeds
• §2.4.1 resolves → origin → git push -u origin <branch> → remote not found

The rebase in §2.2 would silently succeed, then the push would fail with a confusing fatal: 'origin' does not appear to be a git repository. This is the core failure mode the PR is supposed to eliminate.

The PR description states "both sites now resolve the remote with the same candidate-priority idiom" — that claim is only true on the happy path (repos that have origin or have a tracking remote set). The asymmetry should be resolved in one direction or the other, and documented if intentional. A safe fix for §2.4.1:

PUSH_REMOTE=$(git config "branch.$(git branch --show-current | tr -d '\r').remote" 2>/dev/null | tr -d '\r')
[[ "$PUSH_REMOTE" == "." ]] && PUSH_REMOTE=""
if [[ -z "$PUSH_REMOTE" ]]; then
  if git remote | grep -qx origin; then PUSH_REMOTE=origin; else PUSH_REMOTE=$(git remote | head -1); fi
fi

See §2.4.1

Fix this →

---

Finding 2 — git branch --show-current called twice in §2.4.1 (robustness nit)

PUSH_REMOTE=$(git config "branch.$(git branch --show-current | tr -d '\r').remote" ...)
...
git push -u "$PUSH_REMOTE" "$(git branch --show-current)"

The branch name is read twice from git in the same logical block. While unlikely in practice, if a hook or concurrent operation switched the branch between the two calls, the push would go to the wrong branch name. Caching it in a variable is the standard pattern and matches how §2.2 should be written too:

CURRENT_BRANCH=$(git branch --show-current | tr -d '\r')
PUSH_REMOTE=$(git config "branch.$CURRENT_BRANCH.remote" 2>/dev/null | tr -d '\r')
[[ -z "$PUSH_REMOTE" || "$PUSH_REMOTE" == "." ]] && PUSH_REMOTE=origin
git push -u "$PUSH_REMOTE" "$CURRENT_BRANCH"

See §2.4.1 line 178–180

---

Finding 3 — CHANGELOG wording overstates structural parity with toolchain idiom (doc accuracy)

The CHANGELOG at line 15–16 says both sites "resolve the remote with the same candidate-priority idiom the toolchain linters already use." The priority order matches, but the toolchain idiom validates each candidate against a locally cached tracking ref (git rev-parse --verify --quiet "refs/remotes/$CANDIDATE/$BRANCH") before accepting. The PR's blocks just pick the first viable name. That's fine here (a fetch will update the ref anyway), but calling it "the same idiom" implies structural parity that doesn't exist.

Suggested softening: "the same candidate-priority ordering" or "the same resolution ladder."

See CHANGELOG L15

---

Everything else is solid

• The "." upstream guard ([[ "$REMOTE" == "." ]] && REMOTE="") is correct and consistent with the toolchain's own treatment of local-only upstreams.
• tr -d '\r' on both sites is consistent and appropriate for Windows CRLF safety.
• All origin/$DEFAULT_BRANCH occurrences in §2.2 prose and code are updated to $REMOTE/$DEFAULT_BRANCH — no instances missed (the merge-vs-rebase advisory, skip-conditions paragraph, and progress echo are all covered).
• The ORIGIN_DEFAULT → REMOTE_DEFAULT rename is correct and complete within the block.
• Shell injection: $(git branch --show-current | tr -d '\r') inside double quotes is safe — the result is literal text passed as a single argument to git config, never re-evaluated.
• Version bump 0.15.3 → 0.15.4 and CHANGELOG format are correct for a patch.
• No #NNN references in added code comments (comment-hygiene gate passes).

Finding 1 is the only functional concern and directly contradicts the PR description's guarantee; the other two are documentation accuracy and a minor robustness improvement. Finding 1 should be addressed before merge for the fix to actually work on all non-origin remote names in the push path.

[claude]

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

---

Security Review — PR #763

• Gather context and PR diff
• Read changed files
• Audit remote-resolution logic for injection / trust issues
• Check plugin.json and CHANGELOG for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/reference/create.md (doc-only), plugin.json (version bump 0.15.3 → 0.15.4), CHANGELOG.md. No scripts, workflows, or MCP surfaces changed.

---

Shell injection — git config "branch.$(git branch --show-current | tr -d '\r').remote" (§2.2 and §2.4.1)

Both new resolution blocks embed $(git branch --show-current | tr -d '\r') inside a double-quoted string argument:

REMOTE=$(git config "branch.$(git branch --show-current | tr -d '\r').remote" 2>/dev/null | tr -d '\r')

Safe. Bash $(...) inside "..." executes the substitution and inserts the result as literal text — the result is not re-parsed for shell metacharacters. A branch name containing ;, $(), backticks, or glob characters becomes part of the single quoted argument to git config with no further evaluation. All downstream uses of $REMOTE, $PUSH_REMOTE, and $(git branch --show-current) in the git fetch, git rebase, git push, and git rev-parse calls are consistently double-quoted at every call site.

Permalink to §2.2: create.md#L79-L93
Permalink to §2.4.1: create.md#L178-L180

---

git remote | head -1 fallback — .git/config write-access prerequisite

else REMOTE=$(git remote | head -1); fi

SUGGESTION — Confidence: LOW. An actor who can write .git/config before the flow runs could prepend a remote name (aaa-attacker) that lexicographically beats origin in insertion order and points to a malicious URL. $REMOTE would then resolve to that name and git fetch would contact the attacker-controlled server.

This is not a new attack surface: the attacker who can write .git/config can also replace the origin URL directly, achieving the same outcome with the hardcoded baseline this PR replaces. Not a regression. Flagging for completeness only — no action required.

---

DEFAULT_BRANCH trust boundary

DEFAULT_BRANCH=$(gh repo view --json defaultBranchRef -q .defaultBranchRef.name) queries the authenticated GitHub API. The returned value is used in double-quoted positions ("$REMOTE/$DEFAULT_BRANCH") throughout §2.2 — no shell injection path exists even if a repo's default branch name contained metacharacters. Repository owners who control the default branch name already have full repository write access, so this is within the existing trust boundary.

---

plugin.json / CHANGELOG

Version bump and prose changelog only. No new permissions, egress endpoints, execution surfaces, or dependency pins changed.

---

No security issues found. The remote-resolution logic uses correct quoting at every call site, does not open new injection surfaces, and introduces no new credential, trust-delegation, or supply-chain concerns relative to the hardcoded-origin baseline it replaces.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 466bdf3468

ℹ️ 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 3m 17s —— View job

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Read changed files
• Review remote-resolution logic and test coverage
• Check alignment with PLUGIN-PHILOSOPHY.md
• Post findings

---

Scope: create.md (doc update), resolve-remote.sh (new shared script), resolve-remote.test.sh (new tests), plugin.json (version bump 0.15.3 → 0.15.4), CHANGELOG.md. No hooks or workflows changed.

The a78661d follow-up commit correctly addresses Finding 1 from earlier reviews — the asymmetric fallback between §2.2 and §2.4.1 — by extracting a shared resolve-remote.sh script and changing the sole-remote fallback to fail loudly on ambiguity. One documentation gap remains.

---

Finding 1 — CHANGELOG entry doesn't reflect a78661d's behavioral changes (doc accuracy)

The CHANGELOG entry was written to describe commit 466bdf3's behavior. Commit a78661d made two meaningful changes that the CHANGELOG doesn't capture:

1. Resolution is now delegated to a shared script. The CHANGELOG says "Both sites now resolve the remote with the same candidate-priority idiom" without mentioning that the logic was extracted to scripts/resolve-remote.sh. This is a structural change worth documenting — it's the mechanism that guarantees both sites stay in sync going forward.

2. The last-resort fallback behavior changed. The CHANGELOG says:

else the sole configured remote

The actual implementation in resolve-remote.sh is:

elif [[ ${#REMOTES[@]} -eq 1 ]]; then
  REMOTE="${REMOTES[0]}"
elif [[ ${#REMOTES[@]} -eq 0 ]]; then
  # ... exit 1
else
  echo "error: cannot resolve a remote for branch '${BRANCH}': no branch.${BRANCH}.remote, no 'origin', and ${#REMOTES[@]} other remotes exist ..." >&2
  exit 1
fi

When 2+ non-origin remotes exist with neither branch.<name>.remote nor origin set (e.g. a fork clone with fork + upstream remotes and no origin), the resolver exits 1 with a diagnostic rather than silently picking the first entry. This is the key behavioral change from 466bdf3 that addresses the Codex P2 finding — "fail loudly, not head -1" — and the CHANGELOG doesn't mention it.

Suggested addition to the existing CHANGELOG entry (or an additional bullet under ### Fixed):

Resolution logic is extracted to scripts/resolve-remote.sh (shared by §2.2 and §2.4.1). The last-resort fallback — when no branch.<name>.remote and no origin are configured — applies only when exactly one other remote exists; two or more candidates without a branch.<name>.remote or origin now exit 1 with a diagnostic listing the candidates, rather than silently resolving to git remote | head -1.

See CHANGELOG L15 · See resolve-remote.sh L36-L44

Fix this →

---

Finding 2 — CHANGELOG wording overstates parity with toolchain idiom (minor, previously flagged)

Still present: "the same candidate-priority idiom the toolchain linters already use." The toolchain validates each candidate against a locally-cached tracking ref (git rev-parse --verify --quiet "refs/remotes/$CANDIDATE/$BRANCH") before accepting; resolve-remote.sh does not (which is fine — a fetch updates the ref anyway). "Idiom" implies structural parity that doesn't exist. Consider softening to "the same candidate-priority ordering."

See CHANGELOG L15

---

Everything else is solid

resolve-remote.sh — correct and well-structured.

• Priority chain (branch.<name>.remote → origin → sole remote → fail) is exactly right.
• . upstream guard ([[ "$REMOTE" == "." ]] && REMOTE="") is consistent with the toolchain's own treatment.
• tr -d '\r' on the git config result handles Windows CRLF correctly.
• mapfile -t REMOTES < <(git remote) correctly handles the zero-remotes case (REMOTES is empty, hits the ${#REMOTES[@]} -eq 0 branch, exits 1).
• Error messages name the branch and list all candidates — clear diagnostics for the ambiguous case.
• Both new scripts are committed as 100755 (executable).

resolve-remote.test.sh — comprehensive coverage.
All 7 cases are present: sole origin, origin + second remote, explicit branch.<name>.remote, . upstream fallthrough, sole non-origin remote, multiple non-origin remotes (fail loudly), no remotes (fail loudly). The fork+upstream repro that was specifically requested is covered at line 78–79. Test infrastructure (make_repo, run_test) is clean.

create.md — §2.2 and §2.4.1 now symmetric.
Both sites call the shared resolver and propagate its exit code (|| exit 1). The ORIGIN_DEFAULT → REMOTE_DEFAULT rename is complete. All §2.2 prose occurrences (git log --merges, skip-conditions, merge-vs-rebase advisory) use $REMOTE/$DEFAULT_BRANCH. No #NNN references in added code comments.

Shell injection: $(git branch --show-current | tr -d '\r') inside double-quoted git config argument is safe (result is literal text, not re-evaluated). "$PUSH_REMOTE" and "$(git branch --show-current)" in the push call are consistently double-quoted. No new injection surface.

plugin.json: Version bump 0.15.3 → 0.15.4 is appropriate for a patch (doc-only + shared script). No new permissions or egress.

Finding 1 (CHANGELOG stale on a78661d's changes) is a straightforward documentation fix — the resolve-remote.sh script header comment at lines 6–14 already has the right wording and could be adapted directly. Everything else is ready to merge.

[claude]

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

---

Security Review — PR #763

• Gather context and PR diff
• Read changed files
• Audit resolve-remote.sh for injection / trust issues
• Audit create.md shell blocks for injection surfaces
• Check resolve-remote.test.sh for security-relevant patterns
• Check plugin.json and CHANGELOG.md for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/scripts/resolve-remote.sh (new), resolve-remote.test.sh (new), create.md (doc only), plugin.json (version bump), CHANGELOG.md. No scripts other than the two new files, no Actions workflows, no permissions changes.

---

Shell injection — git config "branch.${BRANCH}.remote" in resolve-remote.sh

Line 29:

REMOTE=$(git config "branch.${BRANCH}.remote" 2>/dev/null | tr -d '\r')

Safe. BRANCH is expanded inside a double-quoted string argument to git config — the result of the substitution is never re-parsed by the shell. A branch name containing ;, $(), backticks, or glob characters becomes literal text in the single argument git config receives. Even a worst-case external $1 carrying metacharacters only produces a config-key lookup for an odd key; it does not reach a shell parser again.

$1 → git config key subsection — unvalidated passthrough

Lines 24–29: BRANCH="${1:-}" flows directly into "branch.${BRANCH}.remote".

SUGGESTION — Confidence: LOW. If $1 is provided externally with a branch name that contains dots (e.g. feat.v2), git config parses the key as section=branch, subsection=feat.v2, key=remote, which is correct per the git-config spec. However, any text $1 supplies becomes part of the config key with no prior validation against git's branch-name character restrictions. The impact is limited to reading an unintended config value (not code execution or credential exposure), and in the create.md usage $1 is never passed — the default git branch --show-current path always runs. The concern only materialises if the script is later called programmatically with untrusted input. Adding a guard such as [[ "$BRANCH" =~ ^[a-zA-Z0-9_./-]+$ ]] before the git config call would close the surface cleanly.

Remote name as output — stdout capture trust

Line 47: echo "$REMOTE" is captured by the caller via $(). The value is then used in "$REMOTE" positions in git commands (fetch, merge-base, rev-parse, push).

Safe. Git enforces that remote names contain only [A-Za-z0-9._/-] and cannot begin with -, so no leading-dash injection into git flag parsing is possible. Trailing newlines are stripped by command substitution; non-trailing newlines cannot appear in a git remote name. All downstream uses in create.md are consistently double-quoted.

mapfile and ${REMOTES[@]} — remote list handling

Lines 33–44:

mapfile -t REMOTES < <(git remote)
if printf '%s\n' "${REMOTES[@]}" | grep -qx origin; then

Safe. grep -qx matches the whole line exactly, so no partial-match or regex-injection path exists. ${REMOTES[@]} is used only for exact comparison and in the stderr diagnostic — not in any eval or command construction. The diagnostic at line 42 includes ${REMOTES[*]} (remote names joined by IFS), which is fine because git remote names are already restricted to safe characters.

Detached HEAD produces branch..remote config lookup

Lines 25–29: In detached HEAD mode git branch --show-current returns an empty string; BRANCH stays empty and git config "branch..remote" is queried (malformed subsection). git config exits 1, the 2>/dev/null suppresses it, REMOTE is empty, and the script falls through to the remote-list logic. Not a security issue — the detached-HEAD path reaches the same safe failure modes as a fresh branch with no configured remote.

resolve-remote.test.sh — test harness

Safe. mktemp -d creates a unique directory; the trap cleanup double-quotes $WORKDIR. All $branch values in run_test calls are hardcoded literals controlled by the test script itself. Remote URLs are /dev/null (never fetched). No injection path in the harness.

bash "${CLAUDE_PLUGIN_ROOT}/..." in create.md

Create.md, §2.2 and §2.4.1: both sites call bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh".

Within existing trust model. CLAUDE_PLUGIN_ROOT is set by the Claude Code plugin runtime from the installed plugin's cache path — it is not derived from repository content or user-supplied input. An attacker who can influence CLAUDE_PLUGIN_ROOT has already compromised the plugin runtime itself.

plugin.json / CHANGELOG.md

Version bump 0.15.3 → 0.15.4 and prose changelog entry only. No new permissions, egress endpoints, MCP surfaces, dependency pins, or execution entrypoints added.

---

No CRITICAL or IMPORTANT security findings. The one SUGGESTION (unvalidated $1 passthrough in resolve-remote.sh) is low-confidence and zero-impact for all current callers. The create.md usage never passes $1; the concern is forward-looking if the script is later called with untrusted input. Everything else — quoting discipline, remote-name trust, set -uo pipefail, cleanup — is correct throughout.

[claude]

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

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Read changed files (resolve-remote.sh, resolve-remote.test.sh, create.md, plugin.json, CHANGELOG.md)
• Review resolve-remote.sh logic and correctness
• Review resolve-remote.test.sh coverage
• Review create.md shell blocks and doc accuracy
• Review plugin.json and CHANGELOG.md
• Post findings

---

Scope: create.md (doc update), scripts/resolve-remote.sh (new shared script), scripts/resolve-remote.test.sh (new tests), plugin.json (version bump 0.15.3 → 0.15.4), CHANGELOG.md. No hooks, workflows, or executable paths changed beyond the two new scripts.

The a78661d follow-up addressed both Codex P2 findings — the asymmetric §2.2/§2.4.1 fallback chains and the silent head -1 pick on ambiguous remotes — by extracting a shared resolve-remote.sh with comprehensive test coverage. Two issues remain.

---

Finding 1 — CHANGELOG entry is stale after a78661d (doc accuracy)

The [0.15.4] entry was authored against commit 466bdf3 and was never updated to reflect a78661d's structural changes. Two concrete gaps:

1a. Resolution is now delegated to a shared script, which isn't mentioned.
The entry says "Both sites now resolve the remote with the same candidate-priority idiom" but doesn't mention scripts/resolve-remote.sh. That's the mechanism that guarantees both sites stay in sync — it's the material part of what a78661d changed.

1b. The last-resort fallback behavior changed in a way the entry contradicts.
The entry says:

else the sole configured remote

The actual implementation in resolve-remote.sh L36–44 is:

elif [[ ${#REMOTES[@]} -eq 1 ]]; then
  REMOTE="${REMOTES[0]}"
elif [[ ${#REMOTES[@]} -eq 0 ]]; then
  echo "error: cannot resolve a remote ... no remotes are configured." >&2; exit 1
else
  echo "error: cannot resolve a remote ... ${#REMOTES[@]} other remotes exist ..." >&2; exit 1
fi

With 2+ non-origin remotes and no branch.<name>.remote (the fork+upstream case), the resolver exits 1 with a diagnostic — it does NOT pick the sole remote. The CHANGELOG as written implies that path is always taken, which will confuse any reader who hits the fail-loud case.

The script header comment at lines 8–14 already has the right wording and can be adapted directly:

Resolution logic is extracted to `scripts/resolve-remote.sh` (shared by §2.2 and §2.4.1).
The last-resort fallback — when no `branch.<name>.remote` and no `origin` are configured —
applies only when exactly one other remote exists; two or more candidates without a
`branch.<name>.remote` or `origin` now exit 1 with a diagnostic listing the candidates,
rather than silently resolving to `git remote | head -1`.

Fix this →

---

Finding 2 — CHANGELOG wording overstates parity with toolchain idiom (minor)

CHANGELOG L15:

resolve the remote with the same candidate-priority idiom the toolchain linters already use

The priority order matches, but the toolchain idiom validates each candidate against a locally-cached tracking ref before accepting:

git rev-parse --verify --quiet "refs/remotes/$CANDIDATE/$BRANCH"

resolve-remote.sh picks the first viable name without that check (appropriate here since a git fetch updates the ref). "Idiom" implies structural parity that doesn't exist. Consider "the same candidate-priority ordering" or "the same resolution ladder."

---

Finding 3 — git branch --show-current read twice in §2.4.1 (robustness nit)

create.md L183–184:

PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh") || exit 1
git push -u "$PUSH_REMOTE" "$(git branch --show-current)"

resolve-remote.sh calls git branch --show-current internally (L26) to determine branch.<name>.remote. The push call reads it again. A hook or concurrent operation that changes HEAD between the two calls would resolve the remote config for branch A and push branch B. Unlikely in practice; the fix is one line:

CURRENT_BRANCH=$(git branch --show-current)
PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" "$CURRENT_BRANCH") || exit 1
git push -u "$PUSH_REMOTE" "$CURRENT_BRANCH"

The resolver already accepts an optional $1 branch arg (and the test suite uses it that way), so no script changes are needed — just the caller.

---

Everything else is solid

resolve-remote.sh

• Priority chain (branch.<name>.remote → origin → sole remote → fail loudly on ambiguity) is exactly right and matches the documented design.
• "." upstream guard ([[ "$REMOTE" == "." ]] && REMOTE="") correct and consistent with the toolchain's own treatment.
• tr -d '\r' on the git config result handles Windows CRLF correctly.
• mapfile -t REMOTES < <(git remote) + zero-check correctly handles the no-remotes case.
• grep -qx origin uses whole-line matching — no partial-match or injection path.
• ${REMOTES[*]} in the diagnostic is safe — git enforces that remote names contain only [A-Za-z0-9._/-].
• set -uo pipefail is appropriate; omitting -e is a defensible choice for a script that explicitly checks exit codes.
• Both scripts committed 100755 (executable). ✓

resolve-remote.test.sh
All 7 test cases are present and meaningful: sole origin, origin + second remote, explicit branch.<name>.remote, "." upstream fallthrough, sole non-origin remote, 2+ non-origin remotes (fail loudly — the fork+upstream repro), no remotes at all. Test harness (make_repo, run_test) uses mktemp -d + trapped cleanup, remotes pointed at /dev/null (never fetched). Clean and hermetic. ✓

create.md — §2.2 and §2.4.1 now symmetric
Both sites call the shared resolver and propagate its exit code (|| exit 1). ORIGIN_DEFAULT → REMOTE_DEFAULT rename is complete. All §2.2 prose occurrences (git log --merges, skip-conditions, merge-vs-rebase advisory) use $REMOTE/$DEFAULT_BRANCH. No #NNN references in added code comments. ✓

Shell injection
${CLAUDE_PLUGIN_ROOT} is set by the plugin runtime (not user/repo input). "$REMOTE", "$PUSH_REMOTE", "$(git branch --show-current)" are consistently double-quoted. $(git config "branch.${BRANCH}.remote" ...) inside double-quotes is safe — the substitution result is literal text passed as a single argument. ✓

plugin.json
Version bump 0.15.3 → 0.15.4 is appropriate for a patch. No new userConfig keys, permissions, or egress surfaces. ✓

---

Summary: Finding 1 is the only substantive concern — the CHANGELOG misrepresents the fail-loud behavior introduced in a78661d and omits the resolve-remote.sh extraction. Findings 2 and 3 are minor doc-accuracy and robustness nits. The core logic in resolve-remote.sh is correct and the test coverage is comprehensive. Ready to merge after Finding 1 is addressed.

[kyle-sexton]

Post-green review classification (rule 8)

Finding	Classification	Resolution
Sole-remote fallback silently picks `head -1` when 2+ non-origin remotes exist, no origin	CONFIRMED — bordered DEFER-FORBIDDEN (silent-wrong-behavior class)	Fixed in a78661d: fallback now applies only for exactly one non-origin remote; 2+ candidates fail loudly with a diagnostic instead of silently rebasing against the wrong base.
Push path (§2.4.1) never got the non-origin fallback the rebase path (§2.2) got	CONFIRMED — incomplete fix	Fixed in a78661d: both paths now call the same shared `scripts/resolve-remote.sh`, so a non-origin single-remote clone works end-to-end (rebase AND push).

Both repro cases verified directly (fork+upstream-no-origin fails loudly; vendor-only clone resolves `vendor` for both steps). New test file `resolve-remote.test.sh` (7/7) plus all other source-control tests pass. Both threads resolved. Flipping to ready and handing off to babysit — no further re-polling from this lane.

[claude]

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

---

Security Review — PR #763

• Gather context and understand the request
• Read PR diff and changed files
• Audit resolve-remote.sh for injection / trust issues
• Audit create.md shell blocks for injection surfaces
• Audit resolve-remote.test.sh for security-relevant patterns
• Check plugin.json and CHANGELOG.md for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/scripts/resolve-remote.sh (new), resolve-remote.test.sh (new), create.md (modified), plugin.json (version bump), CHANGELOG.md (prose). No Actions workflows, hooks, or MCP surfaces changed.

---

Shell injection — git config "branch.${BRANCH}.remote" in resolve-remote.sh

Line 29:

REMOTE=$(git config "branch.${BRANCH}.remote" 2>/dev/null | tr -d '\r')

Safe. ${BRANCH} undergoes variable expansion inside a double-quoted string — the result is substituted literally and is NOT re-parsed by the shell. A branch name containing ;, $(), backticks, or glob characters becomes part of a single opaque argument to git config with no further evaluation. This holds regardless of whether $BRANCH comes from $1 or from git branch --show-current.

$1 → git config subsection — unvalidated passthrough

Lines 24–29: BRANCH="${1:-}" flows into "branch.${BRANCH}.remote".

SUGGESTION — Confidence: LOW. An externally supplied $1 containing a crafted branch name reaches git config as the key's subsection without validation. The impact is limited to reading an unintended or empty config key — no code execution or credential exposure. For all current callers this is unexploitable: create.md never passes $1 (the default git branch --show-current path always runs). The concern only materialises if the script is later invoked programmatically with untrusted input. A guard such as [[ "$BRANCH" =~ ^[a-zA-Z0-9_./-]+$ ]] || { echo "error: invalid branch name" >&2; exit 1; } before line 29 would close the surface with no behavior change for valid branch names.

echo "$REMOTE" captured by callers — leading-dash injection

Line 47: echo "$REMOTE" is captured by the caller and substituted into git fetch "$REMOTE", git push -u "$PUSH_REMOTE", etc.

Safe. Git enforces that remote names cannot begin with - and constrains their character set at creation time (via git remote add). An attacker would need write access to .git/config to insert a remote named -mirror or similar — at which point they already control the repository. All downstream uses in create.md are consistently double-quoted.

mapfile + grep -qx origin — remote list handling

Lines 33–35:

mapfile -t REMOTES < <(git remote)
if printf '%s\n' "${REMOTES[@]}" | grep -qx origin; then

Safe. grep -qx does an exact full-line match of the literal string origin — no partial-match or regex-injection path. ${REMOTES[@]} is used only for exact comparison and in the stderr diagnostic, never in eval or unquoted command construction.

Error diagnostic including ${REMOTES[*]}

Line 42: Remote names are included in a stderr diagnostic. Git constrains remote names (no control characters, validated on creation); the output goes only to stderr and is never piped to further shell evaluation. Safe.

bash "${CLAUDE_PLUGIN_ROOT}/..." in create.md

create.md §2.2 and §2.4.1: Both sites invoke the resolver as bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh".

Within existing trust model. CLAUDE_PLUGIN_ROOT is set by the Claude Code plugin runtime from the installed plugin's cache path — it is not derived from repository content, git state, or user-supplied input. An actor who can influence CLAUDE_PLUGIN_ROOT has already compromised the plugin runtime itself.

git push -u "$PUSH_REMOTE" "$(git branch --show-current)" — argument injection

create.md §2.4.1:

Safe. $PUSH_REMOTE is the output of resolve-remote.sh — a git remote name constrained at creation time (cannot start with -, no path separators). $(git branch --show-current) produces a branch name subject to the same git constraints. Both are double-quoted.

Test harness — mktemp -d, rm -rf, cd "$repo" patterns

Safe. "$WORKDIR" is double-quoted in the trap cleanup. $repo paths are derived from the mktemp output and hardcoded subdirectory names. $RESOLVER is a canonical path computed from SCRIPT_DIR. All branch name arguments to run_test are hardcoded literals ("main") — no injection path from test data. Remote URLs in make_repo calls are the literal /dev/null (never fetched).

plugin.json / CHANGELOG.md

Version bump (0.15.3 → 0.15.4) and prose changelog entry only. No new permissions, egress endpoints, MCP surfaces, dependency pins, or execution entrypoints.

---

No CRITICAL or IMPORTANT security findings. The one SUGGESTION (unvalidated $1 passthrough in resolve-remote.sh) is low-confidence and zero-impact for all current callers. The create.md invocation never passes $1; the concern is forward-looking if the script is later called with untrusted branch-name input. Shell injection, remote-name leading-dash injection, .git/config trust, and $CLAUDE_PLUGIN_ROOT are all within the existing trust model and correctly handled throughout.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a78661d15c

ℹ️ 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 3m 0s —— View job

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Read changed files at HEAD (9a9f3bb)
• Audit CHANGELOG accuracy for 9a9f3bb fixes
• Check Codex P2: pushRemote/pushDefault override gap
• Review resolve-remote.sh, create.md, and test coverage
• Post findings

---

Scope: create.md (doc update), scripts/resolve-remote.sh (new shared script), scripts/resolve-remote.test.sh (new tests), plugin.json (version bump 0.15.3 → 0.15.4), CHANGELOG.md. No hooks, workflows, or executable paths changed beyond the two new scripts (both 100755).

Reviewing at HEAD 9a9f3bb — the third commit on the branch.

---

Prior-round findings — resolution status

Finding	Round	Status
§2.2/§2.4.1 asymmetric fallback (no head-1 on push)	a78661d	✅ Fixed — both paths share resolve-remote.sh
CHANGELOG omits resolve-remote.sh extraction and misrepresents fail-loud behavior	9a9f3bb	✅ Fixed — entry now names the shared script, describes sole-remote vs. fail-loud distinction
CHANGELOG overstates parity with toolchain idiom ("same idiom" → "same ordering")	9a9f3bb	✅ Fixed — now reads "same candidate-priority ordering"
git branch --show-current read twice in §2.4.1	—	⚠️ Still present — minor robustness nit (see Finding 1 below)
Codex P2: branch.<name>.pushRemote / remote.pushDefault not consulted for push	—	Explicitly out of scope — documentation adequate? (see Finding 2 below)

---

Finding 1 — git branch --show-current still called twice in §2.4.1 (robustness nit, previously flagged)

create.md L183–184:

PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh") || exit 1
git push -u "$PUSH_REMOTE" "$(git branch --show-current)"

resolve-remote.sh reads git branch --show-current internally when no $1 is supplied (line 26); the git push call reads it again. A branch switch between the two calls (by a hook, for instance) resolves the push remote for branch A and pushes branch B. Unlikely in practice; fix is one line since the resolver already accepts $1:

CURRENT_BRANCH=$(git branch --show-current | tr -d '\r')
PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" "$CURRENT_BRANCH") || exit 1
git push -u "$PUSH_REMOTE" "$CURRENT_BRANCH"

Fix this →

---

Finding 2 — §2.4.1 comment doesn't mention the pushRemote/pushDefault out-of-scope caveat (doc gap)

The Codex P2 finding is valid: in a triangular fork setup where branch.<name>.remote=upstream (set when checking out off upstream/main) and remote.pushDefault=origin (or branch.<name>.pushRemote=origin), resolve-remote.sh returns upstream for both §2.2 (correct — that's the rebase target) and §2.4.1 (wrong — the push should go to origin/fork).

The PR description, CHANGELOG, and §2.2 comment all explicitly call this out of scope. However, the §2.4.1 code comment does not mention this limitation. A user reading only the push section has no hint that the resolver doesn't consult pushRemote or pushDefault:

# Push to the same remote §2.2 resolved (branch.<name>.remote, else `origin`,
# else the sole other configured remote), via the shared resolver ...

Adding one line to the existing §2.4.1 comment — e.g. # Note: branch.<name>.pushRemote / remote.pushDefault are not consulted (out of scope; see §2.2 comment). — would make the limitation discoverable without reading the PR description.

Fix this →

---

Everything else is solid

resolve-remote.sh — logic is correct and complete:

• Priority chain (branch.<name>.remote → origin → sole remote → fail loudly on 2+) is exact.
• "." upstream guard consistent with toolchain's own treatment. tr -d '\r' on both reads is correct for Windows CRLF.
• mapfile -t REMOTES < <(git remote) handles zero-remotes correctly (empty array → ${#REMOTES[@]} -eq 0 → exit 1 with diagnostic).
• grep -qx origin does whole-line exact match — no partial-match or injection path.
• set -uo pipefail without -e is defensible; the script checks exit codes explicitly throughout.
• Error messages name the branch and enumerate candidates — clear and actionable diagnostics.

resolve-remote.test.sh — 7/7 cases are present and hermetic:

• sole origin, origin+second, explicit branch.<name>.remote, "." fallthrough, sole non-origin, 2+ non-origin (fail loudly — the fork+upstream repro), no remotes.
• mktemp -d + trap cleanup with double-quoted $WORKDIR is correct. Remote URLs are /dev/null (never fetched). All branch arguments are hardcoded literals.

create.md — §2.2 and §2.4.1 are now symmetric and both propagate the resolver's exit code (|| exit 1). ORIGIN_DEFAULT → REMOTE_DEFAULT rename is complete. All §2.2 prose occurrences updated (git log --merges, skip-conditions, merge-vs-rebase advisory). No #NNN refs in added code comments.

Shell injection — bash "${CLAUDE_PLUGIN_ROOT}/..." uses a runtime-set path not influenced by repo content. "$REMOTE" and "$PUSH_REMOTE" are consistently double-quoted at every git call site.

plugin.json — version bump 0.15.3 → 0.15.4 is appropriate for a patch. No new permissions or egress.

CHANGELOG — now accurately describes: (1) delegation to scripts/resolve-remote.sh; (2) sole-remote fallback applies only when exactly one non-origin remote exists; (3) 2+ candidates without branch.<name>.remote or origin exit 1 with a diagnostic. Wording uses "ordering" not "idiom." ✅

---

Summary: Finding 1 is a carried-forward minor robustness nit (easy one-liner). Finding 2 is a small documentation gap — the out-of-scope caveat for pushRemote/pushDefault is present in §2.2 and the CHANGELOG but absent from §2.4.1's own comment. Neither blocks merge if the team is comfortable with the documented triangular-fork limitation. Core logic in resolve-remote.sh is correct and test coverage is comprehensive.

[claude]

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

---

Security Review — PR #763

Tasks

• Gather context and understand the request
• Read PR diff and changed files
• Audit resolve-remote.sh for injection / trust issues
• Audit create.md shell blocks for injection surfaces
• Audit resolve-remote.test.sh for security-relevant patterns
• Check plugin.json and CHANGELOG.md for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/scripts/resolve-remote.sh (new), resolve-remote.test.sh (new), create.md (modified), plugin.json (version bump), CHANGELOG.md (prose). The HEAD commit (9a9f3bb) is a CHANGELOG-only update; all functional changes are in a78661d and 466bdf3. No Actions workflows, hooks, or MCP surfaces changed.

---

Shell injection — git config "branch.${BRANCH}.remote" in resolve-remote.sh

Line 29:

REMOTE=$(git config "branch.${BRANCH}.remote" 2>/dev/null | tr -d '\r')

Safe. ${BRANCH} undergoes variable expansion inside a double-quoted string — the substituted value is inserted as literal text and is NOT re-parsed by the shell. A branch name containing ;, $(), backticks, or glob characters becomes part of a single opaque argument passed to git config with no further evaluation. This holds whether $BRANCH originates from $1 or from git branch --show-current.

$1 → git config subsection — unvalidated passthrough

Lines 24–29:

SUGGESTION — Confidence: LOW. BRANCH="${1:-}" flows into "branch.${BRANCH}.remote" without validation. The impact is limited to reading an unintended or empty config key — no code execution, no credential exposure. For all current callers this is unexploitable: create.md never passes $1 (the default git branch --show-current path always runs). The concern is forward-looking only — if the script is later invoked programmatically with untrusted branch-name input, the unvalidated value reaches git config as a subsection name. A guard before line 29 would close the surface at zero cost to current callers:

[[ -n "$BRANCH" && ! "$BRANCH" =~ ^[a-zA-Z0-9_./-]+$ ]] && {
  echo "error: invalid branch name: '${BRANCH}'" >&2; exit 1
}

echo "$REMOTE" captured by callers — leading-dash argument injection

Line 47: echo "$REMOTE" is captured by callers and substituted into git fetch "$REMOTE", git push -u "$PUSH_REMOTE", etc.

Safe. Git enforces at git remote add time that remote names cannot begin with - and are subject to git check-ref-format constraints. An attacker would need write access to .git/config to insert a problematic name — at which point they already control the repository. All downstream uses in create.md are consistently double-quoted.

mapfile + grep -qx origin — remote list handling

Lines 33–35:

Safe. grep -qx performs an exact full-line match of the literal string origin — no partial-match or regex-injection path. ${REMOTES[@]} is used only for array-length comparison and in the stderr diagnostic, never in eval or unquoted command construction. Remote names included in the error diagnostic at line 42 (${REMOTES[*]}) go only to stderr and are never piped to further shell evaluation.

bash "${CLAUDE_PLUGIN_ROOT}/..." in create.md

create.md §2.2 and §2.4.1:

Within existing trust model. CLAUDE_PLUGIN_ROOT is set by the Claude Code plugin runtime from the installed plugin's cache path — it is not derived from repository content, git state, or user-supplied input. An actor who can influence CLAUDE_PLUGIN_ROOT has already compromised the plugin runtime.

.git/config write-access prerequisite for sole-remote fallback

The resolver selects the sole non-origin remote when exactly one exists and no branch.<name>.remote/origin is set. An actor who can write .git/config before the flow runs could prepend a remote before the one they want the resolver to skip over. Not a new attack surface: write access to .git/config already allows replacing the origin URL directly. Not a regression vs the hardcoded-origin baseline this PR replaces.

Test harness — resolve-remote.test.sh

Safe. mktemp -d + trap 'rm -rf "$WORKDIR"' EXIT uses double-quoting that expands $WORKDIR at trap-fire time (value is fixed, never reassigned after mktemp). All branch-name arguments in run_test calls are hardcoded literals. Remote URLs are the literal /dev/null (never fetched). bash "$RESOLVER" "$branch" — $RESOLVER is a canonical path computed from SCRIPT_DIR; $branch is a caller-controlled literal. No injection path.

plugin.json / CHANGELOG.md

Version bump (0.15.3 → 0.15.4) and prose changelog entry only. No new permissions, egress endpoints, MCP surfaces, dependency pins, or execution entrypoints.

---

No CRITICAL or IMPORTANT security findings. The one SUGGESTION (unvalidated $1 passthrough in resolve-remote.sh) is low-confidence and zero-impact for all current callers — create.md never passes $1. Shell injection, remote-name leading-dash injection, .git/config trust, and $CLAUDE_PLUGIN_ROOT trust are all correctly handled and within the existing trust model throughout.

[kyle-sexton]

Babysit classification — PR Review findings (commit a78661d → 9a9f3bb)

#	Finding	Classification	Evidence
1	CHANGELOG stale — omits resolve-remote.sh extraction and the fail-loud-on-ambiguity behavior	VALID — fixed	9a9f3bbf rewrites the [0.15.4] entry to name scripts/resolve-remote.sh and describe that 2+ non-origin candidates with neither branch.<name>.remote nor origin now exit 1 with a diagnostic rather than resolving via git remote | head -1
2	"idiom" overstates structural parity with the toolchain	VALID — fixed	Same commit softens "same candidate-priority idiom" → "same candidate-priority ordering" (the resolver shares the priority order, not the per-candidate tracking-ref validation)
3	git branch --show-current read twice in §2.4.1 (TOCTOU nit)	VALID (defer)	The suggested cache-and-pass-$1 fix would regress Windows CRLF handling: resolve-remote.sh strips \r only in the no-arg branch (L26); passing an unstripped $1 makes git config "branch.<name>\r.remote" miss a legitimately-set branch.<name>.remote and silently fall through to origin. A CRLF-safe caller change is a valid follow-up; the TOCTOU window is near-zero (as noted, "unlikely").

Verified independently: resolve-remote.test.sh passes 7/7 (single origin, origin+second, explicit branch.<name>.remote, .-upstream fallthrough, sole non-origin, 2+ non-origin fail-loud, no-remotes fail-loud), and both §2.2 (L83) and §2.4.1 (L183) call the shared resolver with || exit 1.

Separately, a new codex P2 (push-precedence in triangular fork setups) is reproduced and left as an open blocker — see the inline thread on the §2.4.1 push line. Not marking ready this round.

[claude]

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

---

Security Review — PR #763 (HEAD: f890d5b)

• Gather context and PR diff
• Read changed files at HEAD (f890d5b)
• Audit resolve-remote.sh --push mode for injection / trust issues
• Audit create.md §2.4.1 shell blocks for injection surfaces
• Audit resolve-remote.test.sh new push tests for security-relevant patterns
• Check CHANGELOG.md for security-relevant changes
• Report findings

---

Scope: plugins/source-control/skills/pull-request/scripts/resolve-remote.sh (modified — new --push mode), resolve-remote.test.sh (modified — new push-precedence tests), create.md (modified — §2.4.1 push block rewritten), CHANGELOG.md (prose). No Actions workflows, hooks, or MCP surfaces changed.

Reviewing changes introduced in the HEAD commit f890d5b, which adds --push mode to the shared resolver, rewrites §2.4.1 to use it, and makes git push -u conditional.

---

Shell injection — git config "branch.${BRANCH}.pushRemote" and remote.pushDefault (new in f890d5b)

Lines 57–62:

REMOTE=$(git config "branch.${BRANCH}.pushRemote" 2>/dev/null | tr -d '\r')
[[ "$REMOTE" == "." ]] && REMOTE=""
if [[ -z "$REMOTE" ]]; then
  REMOTE=$(git config "remote.pushDefault" 2>/dev/null | tr -d '\r')
  [[ "$REMOTE" == "." ]] && REMOTE=""
fi

Safe. ${BRANCH} undergoes variable expansion inside a double-quoted string — the result is literal text passed as a single argument to git config, not re-parsed by the shell. remote.pushDefault has no variable substitution at all. The "." guard is consistently applied to both new keys, matching the existing branch.<name>.remote treatment. Analysis is identical to the branch.${BRANCH}.remote call reviewed in prior rounds.

---

pushRemote/pushDefault values flowing into git push without -- separator — SUGGESTION (Confidence: LOW)

create.md lines 197–203:

PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push) || exit 1
...
git push "$PUSH_REMOTE" "$BRANCH_NAME"

$PUSH_REMOTE can come from branch.<name>.pushRemote or remote.pushDefault — both set via git config, not via git remote add. The git remote add validation that rejects names beginning with - (enforced by check-ref-format) does not apply to pushRemote/pushDefault config values set with git config. An actor who can write .git/config could set branch.<name>.pushRemote = --force or remote.pushDefault = --mirror; git config would return that value, and git push "--force" "$BRANCH_NAME" or git push "--mirror" "$BRANCH_NAME" would be executed rather than a normal push.

Confidence is LOW for the same reason as the .git/config findings in prior rounds: exploiting this requires write access to .git/config, at which point an attacker can already replace any remote's URL, making this no worse than the hardcoded-origin baseline this PR replaces. Not a regression. Adding -- before "$PUSH_REMOTE" (git push -- "$PUSH_REMOTE" "$BRANCH_NAME") would close the surface cleanly with no behavior change for any valid remote name, but a valid remote name cannot start with - anyway (git enforces this at git remote add time for all well-formed repos).

The pre-existing fetch path has the same exposure (git fetch "$REMOTE" "$DEFAULT_BRANCH", git rebase "$REMOTE/$DEFAULT_BRANCH") and was flagged in prior rounds with the same low-confidence assessment — this commit doesn't widen that surface.

---

--push flag parsing — no injection path

Lines 40–44:

PUSH=0
if [[ "${1:-}" == "--push" ]]; then
  PUSH=1
  shift
fi

PUSH is an integer flag used only in [[ $PUSH -eq 1 ]] arithmetic comparisons — no eval or command construction uses it. The --push comparison is a literal string match inside [[ ]]. Safe.

BRANCH_NAME caching in §2.4.1 — TOCTOU nit addressed

create.md lines 197–203: BRANCH_NAME=$(git branch --show-current) is cached once and used in both the git config lookup and the git push call. The prior git branch --show-current-called-twice robustness nit is resolved.

Conditional -u logic in §2.4.1

Lines 199–203:

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

${BRANCH_NAME} is from git branch --show-current — inside a double-quoted string, safe. The comparison "$EXISTING_FETCH_REMOTE" != "." is consistently quoted. Both git push call sites double-quote "$PUSH_REMOTE" and "$BRANCH_NAME". No new injection surface.

resolve-remote.test.sh — new push-precedence tests (lines 54–147)

run_push_test calls bash "$RESOLVER" --push "$branch" where every $branch argument is a hardcoded literal ("main"). Remote URLs are /dev/null (never fetched). All git config calls in the test setup use hardcoded values (upstream, origin, fork, .). No injection path from test data.

CHANGELOG.md

Prose-only update. No new permissions, egress endpoints, MCP surfaces, dependency pins, or execution entrypoints.

---

No CRITICAL or IMPORTANT security findings. The one SUGGESTION (pushRemote/pushDefault values reaching git push without --) is low-confidence and shares the .git/config write-access prerequisite already documented in prior rounds — it is not a regression relative to the fetch path or the hardcoded-origin baseline this PR replaces. Shell injection, remote-name injection, $CLAUDE_PLUGIN_ROOT trust, and test harness safety are all correct throughout.

[claude]

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

---

PR Review: fix(source-control): resolve remote dynamically in pull-request create flow (#442)

Tasks

• Gather context and understand the request
• Read changed files at HEAD (f890d5b)
• Audit push-precedence fix (P2 blocker resolution)
• Check TOCTOU nit (git branch --show-current twice in §2.4.1)
• Check §2.4.1 out-of-scope caveat in code comment
• Review resolve-remote.sh, create.md, and test coverage
• Post findings

---

Scope: create.md (doc update), scripts/resolve-remote.sh (extended with --push flag), scripts/resolve-remote.test.sh (7 new push-path tests), plugin.json (version bump), CHANGELOG.md. No hooks, workflows, or other execution surfaces changed.

Reviewing at HEAD f890d5b.

---

Prior-round findings — resolution status

Finding	Status
P2 blocker: push path used fetch resolver, ignoring pushRemote/pushDefault (triangular fork pushed to fetch remote)	✅ Resolved — resolve-remote.sh --push prepends Git's documented push precedence
§2.4.1 unconditional git push -u would overwrite branch.<name>.remote on triangular forks	✅ Resolved — -u is now conditional on whether a real fetch remote is already configured
TOCTOU: git branch --show-current called twice in §2.4.1 (once in resolver, once in push call)	⚠️ Partially resolved — BRANCH_NAME is cached and reused for the push call and conditional, but the resolver is still invoked without $BRANCH_NAME, so the resolver makes its own git branch --show-current call internally. See Finding 1 below.
§2.4.1 comment didn't mention the triangular-fork out-of-scope caveat	✅ Resolved — the §2.4.1 comment now explains the push/fetch split design in detail
CHANGELOG inaccurate — stale after a78661d	✅ Resolved — CHANGELOG now accurately describes --push mode, conditional -u, and what remains deferred
CHANGELOG "same idiom" overstatement	✅ Resolved — now reads "same candidate-priority ordering"

---

Finding 1 — BRANCH_NAME not passed to resolver; still two git branch --show-current calls (nit)

create.md L197–198:

PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push) || exit 1
BRANCH_NAME=$(git branch --show-current)

The resolver is called without a branch argument, so it invokes git branch --show-current internally (resolve-remote.sh L47–49). The caller then reads it again. The resolver already accepts [branch-name] as an optional positional arg after --push, so passing $BRANCH_NAME would eliminate the second call — and also adds CRLF-safety to BRANCH_NAME itself (the resolver strips \r; the caller does not, which is what the comment "Match resolve-remote.sh's \r strip" refers to for the config read but not for the branch name used in git push):

BRANCH_NAME=$(git branch --show-current | tr -d '\r')
PUSH_REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push "$BRANCH_NAME") || exit 1
EXISTING_FETCH_REMOTE=$(git config "branch.${BRANCH_NAME}.remote" 2>/dev/null | tr -d '\r')

This is an extremely minor nit — the TOCTOU window between the two git branch --show-current calls is vanishingly small in practice (no hook in a PR flow legitimately switches branches between a resolver call and the next line). Not a blocker.

Fix this →

---

P2 blocker — definitively resolved

The --push flag implementation in resolve-remote.sh is correct and complete:

Priority chain for --push mode (L52–63):

1. branch.<name>.pushRemote — overrides everything else for push destination
2. remote.pushDefault — workspace-level override for push destination
3. Falls through to fetch order: branch.<name>.remote → origin → sole other remote → fail loudly

Both .-as-local-repo guards are applied to the two new push-specific keys (lines 58 and 61). The error hint for the ambiguous push case (line 81) correctly names pushRemote/pushDefault in addition to the fetch-path hints — exactly right for diagnosing fork+upstream-no-origin setups.

Triangular fork repro verified by test suite (test L107–118):

• branch.main.remote=upstream + branch.main.pushRemote=origin → fetch returns upstream, push returns origin ✓
• branch.main.remote=upstream + remote.pushDefault=origin → push returns origin ✓
• pushRemote overrides pushDefault when both set ✓
• No push config → push falls through to branch.<name>.remote (same as fetch) ✓
• .-valued pushRemote/pushDefault both fall through to branch.<name>.remote ✓
• pushDefault only, no branch.<name>.remote → resolves pushDefault ✓
• Ambiguous push (2+ non-origin, no push config) → fails loudly ✓

Conditional -u logic (create.md L199–204):

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

The logic is correct across all relevant scenarios:

• Fresh branch (no branch.<name>.remote): -u applied, sets tracking to push target — correct
• Local-only . upstream: treated as unset, -u applied — correct
• Branch tracking upstream (triangular fork): EXISTING_FETCH_REMOTE=upstream → NO -u → push to fork without repointing fetch remote — correct; a git push -u origin feature here would silently overwrite branch.feature.remote=upstream with origin, breaking the next rebase
• Normal tracking branch (branch.<name>.remote=origin): no -u, tracking already set — correct

---

Everything else is solid

resolve-remote.sh structure — The --push flag handling is clean: shift after detecting the flag keeps $1 available for the optional branch arg, so the argument parsing is correct for all combinations (--push, --push branch, branch, no args).

resolve-remote.test.sh — 14 tests total (7 fetch + 7 push), up from 7. The run_push_test helper correctly passes --push before the branch arg. The test at line 111 (run_test "fetch path still resolves branch.<name>.remote") with a push config set is the key isolation check — confirms --push mode doesn't bleed into the default fetch path.

Shell injection — "branch.${BRANCH}.pushRemote" and "remote.pushDefault" follow the same safe double-quoting pattern as the existing "branch.${BRANCH}.remote" line. The config output flows through | tr -d '\r' and is never re-evaluated.

CHANGELOG — Now accurately describes: (1) --push mode and its precedence chain; (2) conditional -u and the rationale for it; (3) what remains deferred (merge.md, babysit-prs). Wording uses "ordering" not "idiom." ✅

plugin.json — version 0.15.4 correct for a patch. No new permissions or egress. ✅

---

Summary: The P2 blocker is definitively resolved. The push-precedence implementation is correct, test coverage is comprehensive (7 new push-path cases including the triangular fork repro), and the CHANGELOG/comments accurately describe the change. Finding 1 is a carry-forward micro-nit with no functional consequence. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f890d5ba8f

ℹ️ 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".

[chatgpt-codex-connector]

Avoid Bash 4-only mapfile in remote resolver

In environments that run the plugin with macOS's stock Bash 3.2 (or any Bash 3), this mapfile builtin is unavailable, while source-control's documented runtime only requires “Bash”/Git Bash and does not declare Bash 4+. This path is hit for the common fresh-branch case where branch.<name>.remote is unset, so /pull-request create fails during remote resolution before the fetch/push that previously worked with hardcoded origin. Use a Bash-3-compatible while read loop or explicitly declare and enforce Bash 4+ for this plugin.

Useful? React with 👍 / 👎.