fix(source-control): D6 resolves the push remote instead of hardcoding origin

[kyle-sexton]

Closes #2310

Successor to #2262, which squash-merged (9daeecb, 0.51.12) before Codex's P1-2 fix could land on it.

What

• Both D6 verify-commit-pushed gates (reference/review-discipline.md, skills/pull-request/SKILL.md) now resolve the branch's push remote through the existing skills/pull-request/scripts/resolve-remote.sh --push — the same resolver push-branch.sh pushes through — and compare the fix SHA against FETCH_HEAD:

  REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push <branch>) \
    && git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD
  

  The hardcoded origin was new in 0.51.12 (base 78dbb10e greps clean for git fetch origin in both files): on a non-origin/triangular-fork checkout it false-fails a successful push, and an origin base repo with a same-named branch can verify the wrong ref.

• CHANGELOG: new 0.51.15 entry; plugin.json 0.51.14 → 0.51.15. The shipped 0.51.12 entry keeps describing its own release (hardcoded-origin form, with a forward note) and takes exactly two in-place factual corrections, both declared in the 0.51.15 entry: verify_fix_commit matches the reachability guarantee (it uses the fork-aware compare API, not git merge-base), and babysit_gh.py's per_page=100 sits at fetch_paginated_api's call sites, not adjacent to the --paginate flag.

Live verification (both directions)

The #2244 motivating branch (fix/babysit-merge-ruleset-context-union) no longer exists on the remote — where the new gate fails loudly at the fetch rather than passing, itself the fail-closed behavior we want. Equivalent live case on feat/2257-worktree-liveness-lock (fix commit d683d535 is not the tip; 0f7be746 is):

check	input	result
new gate (resolve → fetch → --is-ancestor … FETCH_HEAD)	d683d535 (on branch, not tip)	exit 0 ✔
old tip read commits?sha=<branch>&per_page=1	same branch	returns 0f7be746 ≠ fix → false "missing"
new gate	a223aee3 (sibling branch only)	exit 1 ✔
repo-presence commits/a223aee3	same input	HTTP 200 → false pass (the P1-1 class)

Draft until an independent verifier verdict is posted here, per the batch rule.

Related

• fix(source-control): D6 verifies commit presence, not the branch tip; per_page=100 on every paginated read #2262 — predecessor PR whose squash-merge (9daeecb) shipped the hardcoded-origin form; Codex P1-2 thread: fix(source-control): D6 verifies commit presence, not the branch tip; per_page=100 on every paginated read #2262 (comment)
• source-control: D6's verify-commit-pushed gate reads only the tip — a positional index on a list, violating two of the three rules the same skill publishes as absolute #2244 — the original D6 tip-read issue that 0.51.12 fixed
• test harnesses: git_init writes a test identity and disables signing into whatever dir it is handed, with no guard against the real repo #2162 — the misattributed-commit harness bug in the same gate family

🤖 Generated with Claude Code

[claude]

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

---

Security Review

• Gather context (diff, changed files)
• Understand the request (security review)
• Review changed files for security issues
• Post findings

Scope: git diff origin/main...HEAD — 4 files: plugins/source-control/.claude-plugin/plugin.json (version bump), plugins/source-control/CHANGELOG.md (new entry + two in-place corrections to the 0.51.12 entry), plugins/source-control/reference/review-discipline.md, plugins/source-control/skills/pull-request/SKILL.md (both: D6 verify-commit-pushed gate text).

Findings

No security issues found.

This PR is documentation-only: it changes the instructional text of the D6 "verify commit pushed" gate (a checklist an agent follows), a changelog entry, and a semver bump. No executable scripts, workflow files, or config/permission settings are touched — resolve-remote.sh and push-branch.sh, which the new instructions reference, are pre-existing and unmodified by this PR (out of scope per the review instructions, but I read them for context).

Specifically considered and ruled out:

• Command injection via the new gate text — the new command (REMOTE=$(bash ".../resolve-remote.sh" --push <branch>) && git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD) quotes $REMOTE on use, and resolve-remote.sh derives the remote name only from local git config/git remote values, not from any PR-supplied or network-supplied content. No new externally-controlled input reaches this string versus the prior hardcoded-origin version.
• Verification-gate weakening — the change is a strict improvement: it replaces a hardcoded origin fetch/compare with a resolved push remote and compares against FETCH_HEAD (what was just fetched) rather than a stale/wrong-repo ref, closing a false-pass path where an origin-based repo with a same-named branch could verify the wrong SHA.
• CI/workflow risk (pull_request_target, permissions:, script injection via github context) — not applicable; no .github/workflows files are touched.
• Supply chain / pinning — not applicable; no action or dependency pins are touched.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31560181502
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a20f1667c

ℹ️ 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]

Fetch the push URL rather than the remote's fetch URL

When the resolved remote has remote.<name>.pushurl pointing somewhere different from remote.<name>.url, push-branch.sh writes to the push URL but git fetch "$REMOTE" reads from the fetch URL, so a successful fix push can be checked against another repository and falsely block D7/thread resolution. This distinction is already documented in plugins/source-control/skills/babysit-prs/reference/loop.md:188-195 and :220-229, which uses git remote get-url --push --all because git push honors separate push URLs; the verification needs to fetch the actual PR-head push destination rather than assuming FETCH_HEAD came from where the push went.

Useful? React with 👍 / 👎.

[kyle-sexton]

Post-merge verification — posted because this PR merged with no verdict on it

This PR merged at 2026-08-12T03:31:36Z (4e8dd9cc) with zero verification evidence on the artifact. That is the third merge in this batch to do so. The dispatched verifier may well have produced a verdict, but a verdict that never reaches the PR is not evidence and dies with the session, so I verified the merged result myself and am recording it here.

This is a main-thread check by the same context that requested the work — it is NOT the independent verdict the process calls for. It is a floor, not a substitute.

The regression is gone from main

review-discipline.md   grep -c 'git fetch origin'  ->  0
SKILL.md               grep -c 'git fetch origin'  ->  0
review-discipline.md   grep -c 'resolve-remote.sh" --push'  ->  1

The shipped form:

REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push <branch>) &&
  git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD

It reuses the seam rather than reimplementing it

resolve-remote.sh on main implements Git's documented push precedence — branch.<name>.pushRemote, then remote.pushDefault, each ignored when set to . — and its documented signature resolve-remote.sh [--push] [branch-name] matches the call above. So the gate now resolves the same remote push-branch.sh publishes to, which is the whole point of #2310.

Comparing against FETCH_HEAD rather than <remote>/<branch> is the stronger choice: it is exactly what the resolved remote just served, so it cannot drift to a stale remote-tracking ref or collide with a same-named branch in an unrelated origin.

The motivating case still passes — the part that actually matters

A fix that only works when the fix commit is the branch tip would silently reintroduce #2244. Tested against the live case that motivated it, where fix commit 1c17be55 is deliberately not the tip (af771ff0 is):

compare 1c17be55...af771ff0  ->  status=ahead  ahead_by=3  behind_by=0   => ancestor: YES

Three commits behind the tip and still correctly reachable. The gate answers "is my fix on this branch", not "is my fix the tip" — which is the distinction #2244 was filed over.

What this check does NOT cover

• I did not execute the gate end-to-end against a real triangular/fork checkout. The remote-resolution behaviour is verified from resolve-remote.sh's documented precedence and its signature, not from a live fork run.
• I did not review the rest of the diff — CHANGELOG integrity, version bump, or any non-D6 hunk.
• No independent context reviewed this. If the dispatched verifier produced a verdict, it should still be posted here; this comment does not close that gap.

Closes the loop on #2310, which is already closed.

[claude]

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

---

Code Review

• Gather context (diff, changed files)
• Understand the request (correctness/maintainability review)
• Review changed files
• Post findings

Scope: git diff between the PR's merge-base (aca6ab4) and head (3a20f16) — the same 4 files listed in the PR: plugins/source-control/.claude-plugin/plugin.json, plugins/source-control/CHANGELOG.md, plugins/source-control/reference/review-discipline.md, plugins/source-control/skills/pull-request/SKILL.md. Security review is out of scope for this lane (already covered by the security lane's comment above).

Verification performed

• Read skills/pull-request/scripts/resolve-remote.sh — confirmed it supports --push (pushRemote → pushDefault → fetch order: branch.<name>.remote → origin → sole other remote), fails loudly (exit 1) on ambiguity rather than silently guessing, and is the identical resolver push-branch.sh already pushes through (grepped its RESOLVER=".../resolve-remote.sh" reference).
• Traced REMOTE=$(resolve-remote.sh --push <branch>) && git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD: if the resolver exits non-zero, the VAR=$(...) assignment itself carries that exit status, so && short-circuits before the fetch — the gate fails closed on an unresolvable remote rather than falling back to a guess.
• Confirmed both edited files (review-discipline.md:230-243, SKILL.md:181) now use identical resolve→fetch→compare logic, and no other hardcoded git fetch origin <branch>/origin/<branch> D6 pattern remains in either file (grepped for it).
• Verified the two in-place CHANGELOG corrections to the 0.51.12 entry against the actual code: verify_fix_commit (skills/babysit-prs/scripts/babysit_resolve_thread.py:546-604) does use the fork-aware compare/{sha}...{head_oid} API, not git merge-base — matches the corrected "guarantee, not mechanism" wording. babysit_gh.py's per_page=100 does sit at fetch_paginated_api's 4 call sites (lines 458-459, 477-478, 522-523, 690-691), not adjacent to a --paginate flag — matches the correction.
• Checked for stale version references: no other file in the repo mentions 0.51.14 that should have moved to 0.51.15.

Findings

No correctness issues found. The change is a well-scoped, internally consistent doc fix:

• Both D6 gate instances are updated identically and correctly delegate remote resolution to the existing, unmodified resolve-remote.sh --push rather than re-deriving remote logic inline — good reuse, avoids a second place to keep in sync with push-branch.sh.
• Comparing against FETCH_HEAD instead of origin/<branch> is the right fix for the bug described: it ties the compare target to whatever was just fetched from the resolved remote, closing the same-named-branch-on-wrong-repo false-pass path.
• The CHANGELOG's in-place corrections to the shipped 0.51.12 entry are both factually accurate against current code (verified above), and the 0.51.15 entry itself is well-substantiated with the live verification table from the PR description.
• No test coverage is applicable here — this is instructional/checklist text for an agent to execute, not executable code with a test harness in this plugin.

One minor, non-blocking style note (not filed inline since it's not a defect): the 0.51.12 CHANGELOG entry's forward-reference clause — "0.51.15 replaces the hardcoded origin with the resolved push remote" (CHANGELOG.md:59) — is a slightly unusual pattern (an older entry referencing a newer one), but it's clearly intentional per the PR description's stated policy of keeping shipped entries historically accurate while cross-linking corrections, so I'm not raising it as an issue.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[kyle-sexton]

Independent verifier verdict — post-hoc, PR already merged as 4e8dd9cc

All file reads pinned to 4e8dd9cc (merge parent 92bab2f4) via the contents API, never a branch name.

ITEM 1: CONFIRMED — Both D6 gates resolve the remote; no hardcoded `origin` survives in either command, only in the prose explaining what NOT to do.
ITEM 2: CONFIRMED — The gate shells out to the existing resolver at the documented skill-private path; resolve-remote.sh's header documents `[--push] [branch-name]` and the code implements pushRemote -> pushDefault -> fetch order, treating "." as unset.
ITEM 3: CONFIRMED — compare says 1c17be55 is a strict ancestor of the tip, so the is-ancestor predicate finds a non-tip fix commit.
ITEM 4: CONFIRMED — the shipped form compares against `FETCH_HEAD` from the just-issued fetch, not `<remote>/<branch>`; that is the correct choice.
ITEM 5: REFUTED — heading counts pass (+1, -0) but the tail is NOT byte-identical: two in-place edits inside the already-released `## [0.51.12]` entry.
VERDICT: DEFECT FOUND — The D6 gate itself is correct as merged (items 1-4); the failure is item 5's CHANGELOG integrity criterion: the released 0.51.12 entry was edited in place.
SCOPE: see below.

---

ITEM 1 — is origin actually gone? CONFIRMED

plugins/source-control/reference/review-discipline.md L233-234:

  - [ ] **verify commit pushed:** `REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push <branch>) &&
    git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD` — exit 0

plugins/source-control/skills/pull-request/SKILL.md L181:

    - [ ] **Verify commit pushed:** `REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push <branch>) && git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD` — exit 0 means the fix commit is on the PR branch as just fetched from the resolved push remote; non-zero means it is not.

Identical predicate in both files. Every remaining origin token in the two D6 blocks is prose stating why a hardcoded origin is wrong ("never a hardcoded origin: a triangular/fork checkout pushes elsewhere…"); none is executable. For contrast, the merge parent 92bab2f4 had git fetch origin <branch> && git merge-base --is-ancestor <fix-sha> origin/<branch> — both hardcodes are gone.

ITEM 2 — does it reuse the existing seam? CONFIRMED

The gate invokes plugins/source-control/skills/pull-request/scripts/resolve-remote.sh --push rather than reimplementing resolution; no inline git config branch.*.pushRemote logic appears in either file.

Path-prefix form checked (a wrong prefix would make the gate unrunnable in every case): ${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/ matches the convention SKILL.md L37 documents for skill-private scripts, and the file resolves at that path at 4e8dd9cc.

resolve-remote.sh header at 4e8dd9cc:

#   resolve-remote.sh [--push] [branch-name]

Push precedence as documented and as implemented (L52-68):

1. branch.<name>.pushRemote, ignored when .
2. remote.pushDefault, ignored when .
3. …then the fetch order (branch.<name>.remote -> origin -> sole other remote)

Both . guards are real code ([[ "$REMOTE" == "." ]] && REMOTE=""), not just comments. Ambiguity (2+ remotes, no configured remote, no origin) exits 1 with a diagnostic rather than guessing.

ITEM 3 — does it answer the right question? CONFIRMED

gh api "repos/melodic-software/claude-code-plugins/compare/1c17be55...af771ff0", verbatim:

{"ahead_by":3,"behind_by":0,"status":"ahead","total_commits":3}

behind_by: 0 with status: ahead means 1c17be55 is a strict ancestor of af771ff0 — on the branch, three commits back from the tip. git merge-base --is-ancestor 1c17be55 FETCH_HEAD therefore exits 0: the gate finds a non-tip fix commit, which the old tip read (commits?sha=<branch>&per_page=1) would have reported missing. It is also stronger than mere object presence — a commits/<sha> lookup returns 200 for any commit in the repo, including one force-pushed off the branch or living only on a sibling; --is-ancestor against the just-fetched head does not.

Caveat, stated plainly: branch fix/babysit-merge-ruleset-context-union is now 404 on the remote (both /branches/… and /git/refs/heads/…), so the fetch step of the gate is not runnable against it today. The ancestry relation is established by the compare API, the verification method this item named. A deleted branch makes the gate fail at git fetch — fail-closed, the right direction.

ITEM 4 — FETCH_HEAD vs <remote>/<branch>? CONFIRMED

The shipped form uses FETCH_HEAD. That is the correct choice and I see nothing wrong with it. git fetch "$REMOTE" <branch> with an explicit refspec-less branch argument writes exactly what that remote just served into FETCH_HEAD; reading it cannot observe a stale value, because the read is downstream of the fetch that wrote it in the same && chain. <remote>/<branch> is a remote-tracking ref whose freshness depends on the remote's configured fetch refspec covering that branch and on the opportunistic-update path having fired — on a remote added without a standard +refs/heads/*:refs/remotes/<name>/* refspec (plausible for exactly the ad-hoc fork/triangular remotes this fix exists to serve), <remote>/<branch> may not exist at all or may hold a stale value, silently. The merge parent's form (origin/<branch>) carried that exposure; the shipped form does not.

ITEM 5 — CHANGELOG integrity at 4e8dd9cc: REFUTED

Counts, plugins/source-control/CHANGELOG.md, 92bab2f4 -> 4e8dd9cc:

• ## [<version>] headings: 131 -> 132
• headings added: 1 (## [0.51.15]) — pass
• headings deleted: 0 — pass
• diff of heading lines only: 0a1 > ## [0.51.15]
• file diff stat: +33 / -4

Tail from the parent's first heading (## [0.51.14], parent L6) vs the new file from its ## [0.51.14] (L30):

• bytes: 279283 (parent) vs 279698 (new)
• sha256: 8dee13e244c97058a18f446748bdcd80f560356629e31b60fb484a7d4bf9d726 vs 9d29d09bfc7f52741ad25f28c3f5be16a4a529d5da73b2f4a76eac7d5f0b3133

Not byte-identical. Two hunks, both inside the already-released ## [0.51.12] entry (parent L23-49):

31,32c31,35
<   remote PR branch), matching the reachability primitive `babysit-prs` already uses in
<   `verify_fix_commit`.
---
>   remote PR branch; 0.51.15 replaces the hardcoded `origin` with the resolved push remote),
>   matching the reachability *guarantee* of `babysit-prs`'s `verify_fix_commit` — the same
>   is-ancestor-of-the-live-head property — not its mechanism, which is the clone-free, fork-aware
>   compare API (`repos/{owner}/{repo}/compare/{sha}...{head_oid}`) against the PR's own head
>   repository.
42,43c45,48
<   #2246 sweep but were already conformant: their `per_page=100` sits in the endpoint URL on the
<   line adjacent to the `--paginate` flag the line-based sweep matched.
---
>   #2246 sweep but were already conformant: each passes `per_page=100` inside the endpoint URL —
>   adjacent to the `--paginate` flag in `request_review.py`, and at `fetch_paginated_api`'s four
>   call sites in `babysit_gh.py` — so the line-based sweep matched the flag without seeing the
>   parameter.

Both edits are declared — in the PR body and in the 0.51.15 entry's second bullet — and both are factual corrections rather than rewrites of what the release did. That is context, not compliance. Under Keep a Changelog (which this file's own header adopts) a released entry is an immutable record of what shipped; correcting it in place means a reader of 0.51.12 at any later SHA no longer sees what 0.51.12 actually claimed, and any consumer diffing changelogs across versions sees churn in a frozen section. The declared-in-the-successor-entry pattern mitigates but does not satisfy the byte-identical-tail criterion.

Severity: low, and it does not touch the gate. It is a real deviation and warrants an issue against main — either to establish that released entries are append-only and corrections go in the new entry only, or to record the in-place-correction-with-declaration pattern as a sanctioned exception. I am read-only and have not filed it.

SCOPE — what I did NOT examine

• No live execution of the shipped gate end to end; branch fix/babysit-merge-ruleset-context-union is deleted, so the fetch step cannot run. Item 3 rests on the compare API, as the item specified.
• No repo-wide sweep for lingering git fetch origin / origin/<branch> in other D6 call sites, other skills, other plugins, or scripts. Item 1 was fenced to the two named files.
• push-branch.sh not read; the claim that it pushes through this same resolver is taken from the two files' prose and not independently verified.
• resolve-remote.sh's fetch-mode (non---push) callers not examined for regression; only the --push path was in scope.
• No execution of resolve-remote.sh against real git configs — precedence verified by reading the script, not by running it.
• plugin.json beyond the +1/-1 line count, marketplace version consistency, and CI status not checked.
• No review of the 0.51.15 entry's own factual claims (e.g. that verify_fix_commit uses the compare API, or the per_page=100 call-site count in babysit_gh.py) — those corrections were checked for presence, not for truth.