fix(review): resolve default branch dynamically before origin/main fallback (#436)

[kyle-sexton]

Summary

The review plugin's diff-base resolution baked main as its terminal default-branch fallback. Every base-resolution surface resolved the default branch as origin/HEAD, then fell straight to a literal origin/main. origin/HEAD is frequently unset in CI, shallow, single-branch, and fresh clones, so a repository whose default branch is master/develop fell past a non-existent origin/main all the way to the echo HEAD / echo "unavailable" terminal — producing an empty diff on a clean committed branch: a silent no-op review with no error. This violated the convention-resolution ladder's "No baked repo assumptions, ever" (docs/MIGRATION-PLAYBOOK.md).

Fix

A dynamic resolution rung now sits before the literal origin/main in every surface:

git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}'

This queries the remote's own default branch over the same transport the clone already used — host-agnostic, needing neither a locally-set origin/HEAD symref nor gh — and origin/main remains only as the terminal last resort (the existing echo HEAD / echo "unavailable" finals are unchanged).

Mechanism choice. Two candidates resolve the default branch independently of the local origin/HEAD symref: gh repo view --json defaultBranchRef and git ls-remote --symref. git symbolic-ref refs/remotes/origin/HEAD was rejected — it reads the same ref the existing first rung (origin/HEAD) already tries, so it would be a no-op that merely satisfies a grep check. Between the survivors, git ls-remote wins on the plugin design boundary ("must work outside the repository and organization that produced it"; "never assume a browser is present") — gh is GitHub-only and often unauthenticated in exactly the CI/shallow-clone environment this bug targets. It also matches the resolution mechanism the sibling toolchain gap (#411, resolved separately) converged on.

The remote name stays origin; de-hardcoding the remote is the cross-plugin shared default-branch helper tracked by #442, out of scope here.

Applied identically across nine surfaces — the resolution sub-expression is held constant, adapted to each line's surrounding syntax:

• 4 reviewer agents (code-reviewer, security-reviewer, architecture-guardian, ecosystem-specialist) — the merge-base diff-base one-liner.
• fanout/SKILL.md pre-computed diff-size shortstat snippet (three-dot range).
• fanout/SKILL.md, quality-gate/SKILL.md, quality-gate/context/per-slice.md, quality-gate/context/self.md — the ladder-describing prose, updated so the docs don't drift from the changed runtime behavior.

Verification

Post-fix census — every remaining origin/main in plugins/review/ is the intentional terminal fallback preceded by the new rung, or updated prose (CHANGELOG excluded):

$ grep -rn "origin/main" plugins/review/ | grep -v CHANGELOG
plugins/review/agents/architecture-guardian.md:20:   git diff "$(git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null || { D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; } || git merge-base origin/main HEAD 2>/dev/null || echo HEAD)"
plugins/review/agents/code-reviewer.md:20:   git diff "$(git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null || { D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; } || git merge-base origin/main HEAD 2>/dev/null || echo HEAD)"
plugins/review/agents/ecosystem-specialist.md:14:1. **Identify the change set** — `git status --porcelain` plus `PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)"; [ -n "$PR_BASE" ] && git fetch origin "$PR_BASE" 2>/dev/null; git diff --stat "$(git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null || { D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; } || git merge-base origin/main HEAD 2>/dev/null || echo HEAD)"` — the PR's real base wins when one exists (fetched first; shallow clones may lack it).
plugins/review/agents/security-reviewer.md:20:   git diff "$(git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null || { D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; } || git merge-base origin/main HEAD 2>/dev/null || echo HEAD)"
plugins/review/skills/fanout/SKILL.md:14:Committed diff size vs default-base merge base (recompute against the PR's baseRefName when it differs): !`git diff --shortstat origin/HEAD...HEAD 2>/dev/null || { D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git diff --shortstat FETCH_HEAD...HEAD 2>/dev/null; } || git diff --shortstat origin/main...HEAD 2>/dev/null || echo "unavailable"`
plugins/review/skills/fanout/SKILL.md:27:- **Review diff base** — ... Otherwise `git merge-base origin/HEAD HEAD` (falling back to the remote's resolved default branch via `git ls-remote --symref`, then `origin/main`, then `HEAD`). ...
plugins/review/skills/quality-gate/context/per-slice.md:20:   when one exists, else the origin/HEAD -> remote default branch -> origin/main -> HEAD fallback),
plugins/review/skills/quality-gate/context/self.md:29:   when one exists, else the origin/HEAD -> remote default branch -> origin/main -> HEAD fallback)
plugins/review/skills/quality-gate/SKILL.md:23:- **Review diff base** — ... Otherwise `git merge-base origin/HEAD HEAD` (falling back to the remote's resolved default branch via `git ls-remote --symref`, then `origin/main`, then `HEAD`) so committed-clean branches still show their changes; ...

Behavioral proof — with the local origin/HEAD symref deleted (the CI/shallow-clone scenario), the first rung fails yet the base still resolves via the new git ls-remote rung before reaching origin/main:

$ git symbolic-ref --delete refs/remotes/origin/HEAD
$ git rev-parse origin/HEAD 2>&1 | head -1
fatal: ambiguous argument 'origin/HEAD': unknown revision or path not in the working tree.
$ PR_BASE=""; git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null \
    || git merge-base "origin/$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')" HEAD 2>/dev/null \
    || git merge-base origin/main HEAD 2>/dev/null || echo HEAD
789ec6c183aeac5d18d8bf277db48af7a8804722    # resolved via ls-remote rung, not the origin/main literal

Repo lint/validation, post-fix:

$ npx markdownlint-cli2 <all changed review .md files>
Summary: 0 error(s)

$ node scripts/validate-plugin-contracts.mjs
Plugin contracts validated: 33 setup skills and 1587 plugin files checked.

Closes #436

Related

• review: origin/main baked as default-branch fallback across ~7 surfaces #436 — this issue.
• toolchain: <default-branch> placeholder in check/lint has no repo-agnostic resolution guidance #411 — the same baked-<default-branch> bug shape in the toolchain plugin, resolved separately (informational only; not touched here — it has its own open work). This PR reuses that fix's git ls-remote --symref resolution mechanism for consistency.
• source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 — the cross-plugin shared default-branch helper (de-hardcoding the remote name), which this fix defers to and stays scoped away from.

🤖 Generated with a Claude Code implementation subagent (issue #436)

[claude]

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

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR diff and changed files
• Review correctness of the git ls-remote resolution rung
• Review security implications
• Check alignment with CLAUDE.md guidelines
• Check shell expression correctness across all 4 agent files
• Check fanout and quality-gate skill updates
• Verify CHANGELOG and version bump
• Post findings

---

Summary

This is a well-scoped, correctly implemented fix. The git ls-remote --symref rung fills the genuine gap between origin/HEAD (unset in CI/shallow/single-branch clones) and the literal origin/main, addresses the "no baked repo assumptions" rule from the MIGRATION-PLAYBOOK, and is applied consistently across all nine affected surfaces. The semver bump (patch) and CHANGELOG entry are appropriate.

Verdict: LGTM — one minor observation and one design note worth tracking, neither blocking.

---

Findings

Shell expression correctness

The awk pattern is correct across all four agent files and the fanout shortstat:

• /^ref:/ anchors to the symref line in git ls-remote --symref output ([ref: refs/heads/<name>\tHEAD]).
• sub(/refs\/heads\//,"",$2) strips the prefix from $2 — correct field given default whitespace IFS.
• print $2; exit emits the branch name and stops — avoids re-matching against the SHA line below. ✓
• Slash-containing branch names (e.g. feature/my-branch) survive: sub replaces only the first occurrence of refs/heads/, leaving feature/my-branch intact, and the outer "origin/feature/my-branch" is a valid ref. ✓

Empty-output edge case (remote unreachable, remote in detached HEAD, awk matches nothing): the outer git merge-base "origin/" HEAD 2>/dev/null fails silently and the origin/main fallback fires. ✓

code-reviewer.md:20

---

Security — shell injection surface

The extracted branch name is embedded inside double-quoted "origin/$(...)" and passed as a single argument to git merge-base. Inside double quotes, $() output is not subject to word splitting or globbing, and git itself rejects branch names containing shell metacharacters (", ;, |, etc.) at creation time. The practical injection surface is negligible given that the remote is already trusted (same trust level as the git transport). --end-of-options is a correct defensive touch even though origin is literal. ✓

---

Minor observation: network round-trip with no explicit timeout

fanout/SKILL.md:14, code-reviewer.md:20

git ls-remote makes a live network call on every agent invocation. The existing fallback chain handles failure gracefully (2>/dev/null || ...), but there is no explicit timeout (GIT_TERMINAL_PROMPT=0 is not set, and no --timeout equivalent is available for git ls-remote). In slow-network or air-gapped CI environments this rung could stall for the OS-level TCP timeout (~30s+) before falling through.

The PR body notes "same transport the clone already used" as justification — a valid point that a freshly-cloned checkout should have fast connectivity to the same remote. This is a design tradeoff, not a defect. Tracking it via #442 (the cross-plugin shared default-branch helper) would be the right place to address a GIT_TERMINAL_PROMPT=0 or timeout guard if it becomes an observed problem.

---

Consistency across nine surfaces ✓

All nine surfaces named in the PR body carry the identical sub-expression:

Surface	File
code-reviewer merge-base	agents/code-reviewer.md:20
security-reviewer merge-base	agents/security-reviewer.md:20
architecture-guardian merge-base	agents/architecture-guardian.md:20
ecosystem-specialist inline stat	agents/ecosystem-specialist.md:14
fanout shortstat	skills/fanout/SKILL.md:14
fanout prose	skills/fanout/SKILL.md:27
quality-gate prose	skills/quality-gate/SKILL.md:23
per-slice subagent prompt	skills/quality-gate/context/per-slice.md:20
self subagent prompt	skills/quality-gate/context/self.md:29

The ecosystem-specialist adaptation (inline --stat form rather than multi-line merge-base form) correctly matches its original calling convention. ✓

---

Prose accuracy ✓

The updated prose in quality-gate/SKILL.md, per-slice.md, and self.md now reads "origin/HEAD → remote default branch → origin/main → HEAD fallback" — accurately reflecting the new runtime ladder. No prose–runtime drift. ✓

---

Plugin design rules ✓

• No new hardcoded paths or repo-specific assumptions introduced.
• The hardcoded remote name origin was pre-existing; de-hardcoding it is correctly deferred to source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 per PR scope.
• No PII or secrets introduced.
• Version bump from 0.14.2 → 0.14.3 is correctly patch-level for a bug fix.
• CHANGELOG entry is accurate and complete.

[kyle-sexton]

Post-green review classification

Finding	Severity	Classification	Action
No explicit network timeout on the new git ls-remote rung (slow/air-gapped CI could stall ~30s+ before falling through)	minor	VALID, deferred	Not fixed here — reviewer's own recommendation is to track via #442 (cross-plugin shared default-branch helper) if it becomes an observed problem, since that issue already owns remote-handling de-hardcoding for this plugin. Citing #442 in ## Related.

No blocking findings. CI green (14/14), consistency verified across all 9 surfaces. Flipping to ready for review.

[claude]

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

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR diff and changed files
• Review correctness of the git ls-remote resolution rung
• Review security implications
• Check alignment with CLAUDE.md guidelines
• Check shell expression correctness across all agent files
• Check fanout and quality-gate skill updates
• Verify CHANGELOG and version bump
• Post findings

---

Summary

Well-scoped, correctly implemented fix. The git ls-remote --symref rung fills the genuine gap between origin/HEAD (frequently unset in CI/shallow/fresh clones) and the baked origin/main, directly addresses the "No baked repo assumptions, ever" rule from the MIGRATION-PLAYBOOK, and is applied consistently across all nine surfaces. The semver bump and CHANGELOG entry are appropriate.

Verdict: LGTM — one minor observation (pre-existing from the earlier automated review), not blocking.

---

Shell expression analysis

The inserted rung across the four agent files:

git merge-base "origin/$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')" HEAD 2>/dev/null

code-reviewer.md:20, security-reviewer.md:20, architecture-guardian.md:20, ecosystem-specialist.md:14

• /^ref:/ anchors to the symref line (ref: refs/heads/<name>\tHEAD) — correct. The SHA line has no ref: prefix and is ignored. ✓
• sub(/refs\/heads\//,"",$2) uses sub (not gsub) — replaces only the first occurrence of refs/heads/, leaving slash-containing branch names like feature/foo intact as feature/foo. The outer "origin/feature/foo" is a valid ref. ✓
• print $2; exit emits the branch name and stops before re-matching. ✓
• Empty-output edge case (unreachable remote, detached HEAD remote, awk matches nothing): the substitution produces "origin/", an invalid ref. git merge-base "origin/" HEAD 2>/dev/null fails silently and origin/main fires. ✓
• --end-of-options is a correct defensive guard even though origin is a literal, not a variable. ✓

The three-dot form in fanout/SKILL.md:

git diff --shortstat "origin/$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')...HEAD" 2>/dev/null

fanout/SKILL.md:14

• Three-dot range semantics (A...B in git diff) show changes from the common ancestor to HEAD — equivalent semantics to the merge-base form used in agents. ✓
• Empty awk output → "origin/...HEAD" → invalid ref, fails silently with 2>/dev/null. ✓

---

Security

$() inside double quotes is not subject to word splitting or globbing. Git rejects branch names containing shell metacharacters at creation time. The remote is already trusted at the transport level. Injection surface is negligible. ✓

---

Consistency across nine surfaces ✓

Surface	File
code-reviewer merge-base	agents/code-reviewer.md:20
security-reviewer merge-base	agents/security-reviewer.md:20
architecture-guardian merge-base	agents/architecture-guardian.md:20
ecosystem-specialist inline stat	agents/ecosystem-specialist.md:14
fanout shortstat	skills/fanout/SKILL.md:14
fanout ladder prose	skills/fanout/SKILL.md:27
quality-gate ladder prose	skills/quality-gate/SKILL.md:23
per-slice subagent prompt	skills/quality-gate/context/per-slice.md:20
self subagent prompt	skills/quality-gate/context/self.md:29

The ecosystem-specialist adaptation (inline --stat form rather than a code-block merge-base form) correctly matches its original calling convention. ✓ Prose in all four docs now accurately reads "origin/HEAD → remote default branch → origin/main → HEAD fallback". ✓

---

Plugin design rules ✓

• No new hardcoded paths or repo-specific assumptions introduced.
• Hardcoded remote name origin was pre-existing; de-hardcoding it is correctly deferred to source-control: remote name origin hardcoded in pull-request create flow (low severity) #442.
• No PII or secrets introduced.
• Version bump 0.14.2 → 0.14.3 is patch-level — correct for a bug fix.
• CHANGELOG entry is accurate and complete.

---

Minor observation: network round-trip with no explicit timeout

fanout/SKILL.md:14

git ls-remote makes a live network call on each invocation. There is no explicit timeout guard (no GIT_TERMINAL_PROMPT=0, no --timeout equivalent). In slow-network or air-gapped CI environments this rung could stall for the OS-level TCP timeout (~30s+) before the 2>/dev/null fallback fires. The PR body's rationale ("same transport the clone already used") is valid — fresh clones should have fast connectivity to the same remote — making this a design tradeoff, not a defect. The right venue to address a GIT_TERMINAL_PROMPT=0 or timeout guard is #442 (the cross-plugin shared default-branch helper), if it becomes an observed problem in practice.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15631b0e65

ℹ️ 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 2m 55s —— View job

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR context and prior review comments
• Read PR diff (both commits)
• Analyze second commit's response to Codex P1 finding
• Verify correctness of fetch-before-merge-base approach
• Check consistency across all surfaces
• Post findings

---

Summary

This PR now has two commits. The first (15631b0) received LGTM in prior automated reviews. The second (e0ae735) was pushed to address a Codex P1: in shallow/--single-branch clones, git ls-remote --symref reports the remote's default branch name but does not populate the local refs/remotes/origin/* ref, so git merge-base "origin/<default>" HEAD fails — the very silent-no-op path this fix targeted.

The second commit is correct and closes the gap. Verdict: LGTM.

---

Analysis of commit 2: fetch-before-merge-base

The old rung (commit 1):

git merge-base "origin/$(git ls-remote ... | awk '...')" HEAD 2>/dev/null

The new rung (commit 2):

{ D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; }

Shell correctness (code-reviewer.md:20):

• { ...; } group requires a trailing semicolon before } — present ✓
• Empty-output guard: [ -n "$D" ] — if awk matches nothing, group exits 1, next || fires (origin/main fallback). ✓
• git fetch origin "$D" — sets FETCH_HEAD to the fetched commit and makes the ref locally available. ✓
• git merge-base FETCH_HEAD HEAD 2>/dev/null — FETCH_HEAD is the only locally-resolvable handle after fetching a non-tracked branch; mirrors the existing PR_BASE fetch pattern. ✓
• Any failure in the && chain exits non-zero, triggering the origin/main fallback. ✓
• $D used inside double quotes, no word splitting. ✓

Consistency across 5 executable surfaces — all apply the identical pattern:

Surface	File
code-reviewer merge-base	agents/code-reviewer.md:20
security-reviewer merge-base	agents/security-reviewer.md:20
architecture-guardian merge-base	agents/architecture-guardian.md:20
ecosystem-specialist inline stat	agents/ecosystem-specialist.md:14
fanout shortstat	skills/fanout/SKILL.md:14

The fanout shortstat correctly uses git diff --shortstat FETCH_HEAD...HEAD (three-dot range, consistent with its original origin/HEAD...HEAD form). ✓

The 4 prose-only surfaces (fanout/SKILL.md:27, quality-gate/SKILL.md:23, per-slice.md:20, self.md:29) were not changed by commit 2 — they describe the ladder as "origin/HEAD → remote default branch → origin/main → HEAD fallback", which remains accurate. The fetch detail is an implementation mechanism, not a ladder-order change. ✓

CHANGELOG (CHANGELOG.md) — updated under [0.14.3] to describe both changes: the ls-remote rung and the subsequent fetch step, with explanation of why FETCH_HEAD is used. Accurate and complete. ✓

Version — 0.14.3 patch bump unchanged; both commits together form one logical bug fix. ✓

Security — $D is inside double quotes, not subject to word splitting or globbing. Git rejects branch names with shell metacharacters at creation time. Same trust level as the existing PR_BASE fetch. ✓

---

Carry-forward observation (non-blocking)

The minor observation from prior reviews — git ls-remote + git fetch make live network calls with no explicit timeout guard — remains a design tradeoff, not a defect. The rung now fires only when origin/HEAD is unset (first rung fails), keeping it off the hot path for well-connected clones. Correctly deferred to #442.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0ae7355ac

ℹ️ 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 2m 13s —— View job

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR context and prior review comments
• Read third commit diff (bc546d1 — "docs(review): scope 0.14.3 changelog honestly; note depth-1 limitation")
• Assess whether the second Codex P1 (depth-1 shallow clone merge-base failure) is fixed, documented, or deferred
• Verify CHANGELOG accuracy against runtime behavior
• Post findings

---

Summary of commit 3 (bc546d1)

This commit is CHANGELOG-only — no code changes. It corrects an overclaim in the commit 2 (e0ae735) changelog entry and documents the remaining --depth=1 limitation as deferred.

What was overclaimed: The commit 2 changelog said origin/<default> is unresolvable "in --single-branch/shallow clones (the exact CI scenario)". This implied the FETCH_HEAD fix covered the default actions/checkout shape (--depth=1 --single-branch), which it does not.

What commit 3 corrects it to: The fix covers full-depth --single-branch clones and full clones with unset origin/HEAD. A --depth=1 shallow clone still degrades to the empty-diff terminal because fetching the resolved branch at the same shallow depth leaves merge-base FETCH_HEAD HEAD without a common ancestor.

Is the CHANGELOG now accurate?

Yes. Walking through the ladder for a --depth=1 --single-branch clone (the default actions/checkout):

1. git merge-base "origin/HEAD" HEAD — fails (origin/HEAD unset in single-branch) ✓
2. { D="$(git ls-remote ...)"; [ -n "$D" ] && git fetch origin "$D" 2>/dev/null && git merge-base FETCH_HEAD HEAD 2>/dev/null; } — ls-remote succeeds, git fetch origin main at depth 1 downloads the tip of main, but no common ancestor exists between this tip and the depth-1 feature HEAD → fails ✓
3. git merge-base origin/main HEAD — origin/main is not a local ref in single-branch → fails ✓
4. echo HEAD → empty diff terminal ✓

The CHANGELOG's "Known limitation" section now correctly states this scenario and defers it to #442.

---

Assessment of the Codex P1 handling

The second Codex P1 was:

In a --depth=1 --single-branch clone, fetching the remote default branch without --deepen downloads its tip into FETCH_HEAD but leaves the feature tip marked shallow, so git merge-base FETCH_HEAD HEAD still has no common ancestor.

This finding is valid and unaddressed in code. The response was to document rather than fix. That is a reasonable project-level decision: deepening/unshallowing is a heavier operation with its own tradeoffs (speed, permissions, retry logic) that merits a dedicated design decision rather than being bolted onto every reviewer-agent invocation. Deferring to #442 is the right venue.

The practical effect: the default actions/checkout configuration still produces a silent empty diff via this plugin when origin/HEAD is unset. The fix as merged improves the situation for full-depth single-branch clones and full clones with unset origin/HEAD, but not for the majority of CI runs that use --depth=1. Consumers who need correct diffs in --depth=1 CI should set fetch-depth: 0 in their actions/checkout step as a workaround until #442 delivers.

---

CHANGELOG prose accuracy

CHANGELOG.md:21–47

All three scenarios described in the changelog are correct:

Scenario	Fix covers?	Changelog says?	Match
Full clone, origin/HEAD unset	✓ Yes	"full clone whose origin/HEAD is unset"	✓
Full-depth --single-branch	✓ Yes	"full-depth --single-branch clone"	✓
--depth=1 (default actions/checkout)	✗ No	"Known limitation" section, deferred to #442	✓

The "Known limitation" language is accurate and honest. ✓

---

Residual observation (non-blocking, unchanged from prior reviews)

The git ls-remote + git fetch network round-trip has no explicit timeout. This remains a design tradeoff — not a defect — and the rung only fires when origin/HEAD is absent, keeping it off the hot path. Deferred to #442.

---

Verdict

LGTM. The three commits together form a coherent, correctly scoped fix:

• Commit 1 (15631b0): adds the ls-remote name-resolution rung
• Commit 2 (e0ae735): adds the fetch-before-merge-base step so the rung actually resolves in full-depth single-branch/unset-symref clones
• Commit 3 (bc546d1): corrects the changelog overclaim and honestly documents the --depth=1 limitation

The --depth=1 gap is real and unaddressed by the code, but correctly documented as a known limitation deferred to #442. No new defects introduced. Plugin design rules and semver bump are correct.

[claude]

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

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR context and prior review comments
• Read commit 4 diff (99e52af — "docs(review): retarget depth-1 known-limitation to review: diff-base ladder mis-resolves in constrained clones (single-branch / shallow) — deepen-vs-report design fork #625, not source-control: remote name origin hardcoded in pull-request create flow (low severity) #442")
• Verify CHANGELOG accuracy: is review: diff-base ladder mis-resolves in constrained clones (single-branch / shallow) — deepen-vs-report design fork #625 the correct tracker for the depth-1 gap?
• Check for any non-docs changes in commit 4
• Post findings

---

Summary

Commits 1–3 were already LGTM'd by prior automated passes. Commit 4 (99e52af) is the only new work to assess.

Verdict: LGTM. The retargeting is correct, the prose is more accurate than before, and no code changes are introduced.

---

Commit 4 analysis

The commit is CHANGELOG-only — exactly 2 lines changed in plugins/review/CHANGELOG.md:40-43:

Before:

report-and-stop), a heavier tradeoff deferred to the #442 shared default-branch
helper rather than bolted onto every reviewer-agent invocation here.

After:

report-and-stop) — a real design fork, tracked and deferred to #625 rather than
bolted onto every reviewer-agent invocation here.

Retargeting rationale is sound. The commit message explains it directly: #442 is scoped to the source-control plugin's origin remote-name hardcode — a distinct concern from the review plugin's depth-1 shallow-clone deepen/report problem. Pointing the known-limitation at #442 would have created scope drift on that issue and left the depth-1 gap with no accurate home. #625 as a dedicated issue for the depth-1 residual is the correct place.

Prose improvement. The new wording ("a real design fork, tracked and deferred to #625") is more precise than the old ("a heavier tradeoff deferred to the #442 shared default-branch helper") — it names a concrete design choice (deepening vs. report-and-stop) rather than implying it's just a weight consideration, and it drops the inaccurate "shared default-branch helper" label.

No code changes. All five executable surfaces (code-reviewer, security-reviewer, architecture-guardian, ecosystem-specialist, fanout) are unchanged. ✓

No new prose drift. The four prose-only surfaces (fanout/SKILL.md:27, quality-gate/SKILL.md:23, per-slice.md, self.md) describe the ladder as "origin/HEAD → remote default branch → origin/main → HEAD fallback", which remains accurate and does not reference #442 or #625. ✓

---

Carry-forward observation (non-blocking, unchanged from prior reviews)

The git ls-remote + git fetch network round-trip has no explicit timeout guard. Design tradeoff noted in prior reviews — the rung fires only when origin/HEAD is absent, keeping it off the hot path. Correctly deferred (now via #625 for the depth-1 piece, and #442 for the remote-name de-hardcoding).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99e52af577

ℹ️ 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 2m 18s —— View job

---

PR Review: fix(review): resolve default branch dynamically before origin/main fallback

• Read PR context and prior review comments
• Check what's new in HEAD commit (5186cf8) vs last reviewed commit
• Read the PR diff end-to-end
• Verify correctness and alignment with CLAUDE.md guidelines
• Post findings

---

Summary

This PR has four commits that have been reviewed incrementally across prior automated passes. This pass covers the full PR at HEAD (5186cf8). Verdict: LGTM.

---

Commit 4 (5186cf8) — new in this pass

CHANGELOG-only, 2-line change in plugins/review/CHANGELOG.md:43:

-  report-and-stop), a heavier tradeoff deferred to the #442 shared default-branch
-  helper rather than bolted onto every reviewer-agent invocation here.
+  report-and-stop) — a real design fork, tracked and deferred to #625 rather than
+  bolted onto every reviewer-agent invocation here.

The retargeting is correct. #442 is scoped to the source-control plugin's origin remote-name hardcode — a different plugin, different mechanism. #625 is the dedicated issue for the review plugin's depth-1 shallow-clone empty-diff residual (deepen-vs-report-and-stop design fork). The old pointer would have muddied #442's scope; the new one gives the depth-1 gap an accurate, non-drifting home. ✓

---

Full PR code-correctness summary

All five executable surfaces carry the identical, correct rung (verified in prior passes; spot-checked here against code-reviewer.md:20 and fanout/SKILL.md:14):

{ D="$(git ls-remote --symref --end-of-options origin HEAD 2>/dev/null \
    | awk '/^ref:/{sub(/refs\/heads\//,"",$2); print $2; exit}')"; \
  [ -n "$D" ] && git fetch origin "$D" 2>/dev/null \
    && git merge-base FETCH_HEAD HEAD 2>/dev/null; }

• [ -n "$D" ] guards the empty-awk-output case (unreachable remote → exit 1 → origin/main fallback fires). ✓
• git fetch origin "$D" populates FETCH_HEAD without requiring a local refs/remotes/origin/* ref — correct for full-depth --single-branch clones. ✓
• $D inside double quotes: no word-splitting or globbing. ✓
• --end-of-options is a correct defensive guard. ✓
• The fanout shortstat uses FETCH_HEAD...HEAD (three-dot, consistent with its original origin/HEAD...HEAD form). ✓
• The four prose-only surfaces correctly read "origin/HEAD → remote default branch → origin/main → HEAD fallback". ✓

Plugin design rules ✓

• No new hardcoded paths or repo-specific assumptions.
• Hardcoded remote name origin pre-existed; de-hardcoding deferred to source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 (out of scope here).
• No PII or secrets.
• Version 0.14.2 → 0.14.3 is patch-level — correct for a bug fix.
• CHANGELOG entry is accurate and complete including the known-limitation section.

Carry-forward observation (non-blocking, unchanged)

git ls-remote + git fetch make live network calls with no explicit timeout guard. The rung only fires when origin/HEAD is absent, keeping it off the hot path for well-connected clones. Correctly deferred — timeout/unshallow handling belongs in the shared helper, not bolted onto five dense one-liners.

---