review: diff-base ladder mis-resolves in constrained clones (single-branch / shallow) — deepen-vs-report design fork

[kyle-sexton]

Plugin: review · Category: baked repo assumption / silent wrong-or-empty diff base
Source: bot review on #591 (chatgpt-codex-connector, two P1 findings) + the maintainer's own thread-1 scope note; confirmed VALID, deferred out of #591's scope as design-shaped.

Summary

#591 fixed the origin/main-baked terminal fallback and the full-depth --single-branch default-base case (fetch resolved default branch, diff against FETCH_HEAD). Two constrained-clone facets of the diff-base ladder remain, both sharing one root — the ladder resolves the base from local refs / a single shared FETCH_HEAD that constrained clones don't populate as assumed — and one fix design fork. They belong together in a single reworked base-resolution path, not re-patched across the five dense one-liners (four reviewer agents + the fanout diff-size snippet), where each inline escalation has demonstrably invited the next bot finding.

Facet A — --depth=1 shallow clone (default actions/checkout shape)

After fetching the resolved default branch at the same shallow depth, git merge-base FETCH_HEAD HEAD finds no common ancestor, so the ladder degrades to echo HEAD and git diff HEAD is empty — the silent no-op #591 set out to eliminate. Reproduced (3-commit main, feature off its tip, --depth=1 --single-branch): after git fetch origin main, merge-base FETCH_HEAD HEAD fails; git fetch --unshallow origin flips is-shallow-repository false and it then resolves.

Facet B — full-depth --single-branch clone with a PR against a NON-default base

The PR_BASE rung (git merge-base "origin/${PR_BASE:-HEAD}" HEAD) — pre-existing, untouched by #591 — relies on a local origin/<base> ref. In a --single-branch clone, git fetch origin "$PR_BASE" populates only FETCH_HEAD, never refs/remotes/origin/<base>, so this rung fails. The ladder then falls to #591's new default-branch rung, which fetches the default branch into FETCH_HEAD (overwriting the PR base) and diffs against the default merge-base rather than the PR's real base (e.g. release) — silently folding the base branch's pre-existing changes into the review as if they were the PR's. git fetch needs --append to add to FETCH_HEAD instead of overwriting; the base must be captured (an explicit rev/ref) before the default-branch fallback runs. #591 changed this scenario's symptom from empty-diff to a wrong, misleading diff base; it was already broken (empty) pre-#591. Maintainer flagged the same root in #591 thread 1: "the pre-existing PR_BASE rung shares the same local-resolution limitation for a non-default PR base in single-branch clones."

Design fork (needs a human decision — hence status: needs-decision)

1. Fetch-more / deepen — capture each fetched base as an explicit rev before the next fetch overwrites FETCH_HEAD (facet B), and lazily git fetch --unshallow when the shallow merge-base fails (facet A). Correct, but --unshallow on a large repo is a heavy, unbounded download on every affected agent invocation — a cost that should be chosen deliberately, not baked into an unattended pass.
2. Convention-aligned report-and-stop — fanout/SKILL.md already prescribes "report which ref failed and STOP" for an unresolvable base; the agent files currently violate that by silently reaching echo HEAD (facet A) or a wrong base (facet B). Failing loudly is cheap and honest but changes agent behavior on constrained clones.

Picking between "fetch/unshallow at any cost" and "fail loudly", and factoring the shared resolution into one helper, is a real design decision — the maintainer's stated intent is for this logic to live once in a shared default-branch/base helper rather than across five one-liners.

Relationships

• Surfaced on fix(review): resolve default branch dynamically before origin/main fallback (#436) #591 (fix for review: origin/main baked as default-branch fallback across ~7 surfaces #436). fix(review): resolve default branch dynamically before origin/main fallback (#436) #591 documents facet A as a known limitation of review 0.14.3.
• Sibling to source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 (source-control origin remote-name hardcode) — same "baked repo assumption" ladder family, distinct mechanism; kept separate to avoid muddying source-control: remote name origin hardcoded in pull-request create flow (low severity) #442's ready, source-control-scoped spec.

[kyle-sexton]

2-minute decision brief — depth-1 shallow clone still yields empty diff base

Decision needed: on a --depth=1 shallow clone (the default actions/checkout shape), the diff-base ladder degrades to echo HEAD → empty git diff HEAD → silent no-op review. Pick how the affected surfaces respond: deepen (pay the fetch cost) vs report-and-stop (fail loud).

RECOMMENDED — Option 2: convention-aligned report-and-stop, with is-shallow-repository discrimination and an actionable message. Rationale below. Optional opt-in unshallow escalation layers on top without changing the default.

---

Problem (2 sentences)

When the base ref resolves but the repo is a --depth=1 shallow clone, git merge-base <base> HEAD finds no common ancestor, so the ladder falls to its echo HEAD terminal and git diff HEAD is empty on a clean committed branch — a silent no-op review. This is the exact silent-empty-diff failure #591 set out to kill, surfacing in the single most common CI checkout shape.

Today (verified, current main)

Note: #591 is still OPEN, not merged — #625 is a bot-review finding on #591, deferred out of its scope. The current ladder in the four reviewer agents is (code-reviewer.md:20, security-reviewer.md:20, architecture-guardian.md:20; ecosystem-specialist.md:14 is the same expression with --stat):

git diff "$(git merge-base "origin/${PR_BASE:-HEAD}" HEAD 2>/dev/null \
  || git merge-base origin/main HEAD 2>/dev/null \
  || echo HEAD)"

The 2>/dev/null || … || echo HEAD chain cannot tell "no such ref" from "ref exists but shares no ancestor with HEAD (shallow)" — both fail the same way and slide to echo HEAD. When #591 merges it inserts a git ls-remote --symref rung to resolve the default branch, but that fixes which ref, not the shallow no-common-ancestor failure — so the ladder still terminates in echo HEAD. That terminal is already condemned by convention: fanout/SKILL.md:27 says "Never a hardcoded git diff HEAD, which is empty on a clean committed branch" — yet echo HEAD produces exactly that. And fanout/SKILL.md:46 already prescribes the intended behavior for an unresolvable base: "report which ref failed and STOP." The agents violate both.

Severity is not uniform across the five surfaces. The four agents end in echo HEAD → silently empty diff. The fanout precompute (fanout/SKILL.md:14) ends in echo "unavailable" → a visible marker. So the silent-danger concentrates in the four agents; fanout already fails semi-loudly.

External facts (verified today)

• actions/checkout default fetch-depth is 1 (shallow clone) — actions/checkout README, fetch-depth input: "Number of commits to fetch. 0 indicates all history… Default: 1." So the affected shape is the default, not an edge case.
• git fetch --unshallow fetches all remaining history for the ref — download size proportional to full repo history, unbounded.
• The cost concern is real because the target repo is unknown. docs/PLUGIN-PHILOSOPHY.md binds these agents to run in any consuming repo ("outside the repository and organization that produced it"). Unshallowing is cheap for this small plugin repo but can be arbitrarily heavy in the large monorepo a consumer runs it against — so the cost must be an operator choice, not baked into an unattended pass (the issue's own framing).
• Blast radius includes the home lane. The home review CI (ci-workflows/.../claude-review.yml, main checkout step) uses filter: blob:none with no explicit fetch-depth → still a shallow depth-1 (blobless) checkout per the default above. So report-and-stop is not automatically "safe at home." Confirm whether that reusable workflow computes the diff itself vs relying on the agents' one-liner before shipping — that determines whether the home lane's behavior actually changes.

Options

➤ Option 2 — Report-and-stop (RECOMMENDED). Replace the echo HEAD terminal with the convention's prescribed behavior. Add git rev-parse --is-shallow-repository to disambiguate the two causes of an empty diff (genuinely-clean branch vs shallow-broke-the-base) and emit an actionable message, e.g. "shallow clone — cannot compute a diff base; re-checkout with fetch-depth: 0."

• Why first: cheapest, honest, and the only option that restores an existing, already-documented convention the agents currently violate (fanout/SKILL.md:27 + :46). The is-shallow-repository check is what elevates "fail loud" to "fail loud with the fix."
• Tradeoff: changes agent behavior on shallow clones — a shallow run that today silently returns nothing will instead stop and report (including, likely, the home lane — see blast radius).
• Reversibility: high — pure guard/message change, no fetch, trivially revertable.

Option 1 — Lazy git fetch --unshallow escalation. Deepen only when merge-base fails, then retry. Verified in the #591 thread to make merge-base resolve.

• Tradeoff: unbounded, heavy download on every affected invocation in an unknown consumer repo; invites follow-on concerns (fetch timeout, auth, rate limits) — exactly why it was deferred from fix(review): resolve default branch dynamically before origin/main fallback (#436) #591.
• Reversibility: medium — behavioral + network side-effect on every run; harder to walk back once consumers depend on it.

Option 3 — Bounded deepen (git fetch --deepen=<N> / capped --depth). Middle ground: cap the extra history instead of full unshallow.

• Tradeoff: caps cost but reintroduces silent-wrong-diff risk when the branch diverged more than N commits back — trades an honest failure for a possibly-incomplete diff. Adds a magic constant with no principled value for an arbitrary repo.
• Reversibility: medium.

Compatible layer (not a competing option) — opt-in escalation. Ship Option 2 as the default; gate Option 1's unshallow behind an explicit opt-in (env var / arg, e.g. REVIEW_UNSHALLOW=1). This satisfies the issue's constraint that "that cost tradeoff should be chosen deliberately, not baked into an unattended pass" — the default fails loud, the operator who wants deepening asks for it.

Recommendation rationale (one line)

Report-and-stop is the only choice that restores a convention already written into fanout/SKILL.md rather than adding new, unbounded runtime cost to every consumer — and with is-shallow-repository it turns today's silent no-op into a message that names the one-line fix (fetch-depth: 0). Deepening stays available, but as a deliberate opt-in.

Cross-links

• fix(review): resolve default branch dynamically before origin/main fallback (#436) #591 — predecessor fix (default-branch resolution), OPEN; review: diff-base ladder mis-resolves in constrained clones (single-branch / shallow) — deepen-vs-report design fork #625 is the deferred residual on it. This shares its 5 surfaces (4 agents + fanout precompute).
• review: origin/main baked as default-branch fallback across ~7 surfaces #436 — root issue fix(review): resolve default branch dynamically before origin/main fallback (#436) #591 fixes (baked main default-branch fallback).
• source-control: remote name origin hardcoded in pull-request create flow (low severity) #442 — sibling "baked repo assumption" (source-control origin remote-name hardcode); related family, distinct mechanism (remote-name resolution vs shallow-depth deepen/report) — kept separate per this issue's body.

[kyle-sexton]

This was generated by AI during triage.

Triage — T3, needs-human.

Genuine open design fork, not a single maintainer-vetoable default: (1) fetch-more/deepen (correct, but an unbounded heavy download cost baked into every unattended agent invocation on a large repo) vs (2) convention-aligned report-and-stop (cheap and honest, but changes agent behavior on constrained clones and diverges from silently continuing). Both carry real, differently-shaped costs — this is a product/operational tradeoff call, not a technical correctness question with one right answer. The reporter's own suggested status: needs-decision maps to this lane's needs-human (this lane reserves status: needs-decision for recommend-close cases per its own convention).

Not blocking any currently-open PR.

[kyle-sexton]

Research-resolution pass (Class B dispatch) — facts settled, one product default remains reserved

Outcome: research + empirical repro settle every technical question and hand the work lane an executable Facet-B correctness fix and a fully-specced Facet-A path. But the Facet-A default (deepen vs report-and-stop) is genuinely a product/operational call, not a research-forced one — consistent with this issue's own ratified triage ("not a single maintainer-vetoable default"). So this comment is a maintainer-ratification-ready recommendation, not an auto-proceed: labels stay needs-human / status: needs-decision. Tagged: operator veto window (Class B research-resolution, control tower dispatch) — here exercised as "leave to human," which the dispatch explicitly reserves for the operator-personal case.

Verified against post-#591 main (#591 merged 2026-07-20 62f5d19; the decision brief above was written pre-merge, so its ladder listing is one revision stale — the current ladder is quoted below).

---

The one fact that most de-risks the decision (resolves the maintainer's open blast-radius question)

The brief flagged as unconfirmed: "Confirm whether that reusable workflow computes the diff itself vs relying on the agents' one-liner." Resolved: the home lane does not use the agents' ladder at all.

ci-workflows/.github/workflows/claude-review.yml invokes anthropics/claude-code-action with an inline prompt that says "Use gh pr diff to identify what changed" (line ~70). gh pr diff is GitHub-API-based and independent of local clone depth/filter. The workflow contains no reference to the review plugin, fanout, or the agents — grep confirms. So report-and-stop in the agents changes nothing on the home CI lane. Blast radius is limited to local / consuming-repo invocations of the plugin's agents and fanout skill — exactly the surfaces that should fail honestly.

(Drop the brief's "filter: blob:none → still shallow depth-1" line — a blobless partial clone is not a depth-1 shallow clone. It's the wrong reason for the right conclusion; the gh pr diff fact is the clean one.)

---

Current ladder (post-#591, all four agents; ecosystem-specialist is the same expr with --stat)

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   # shallow/single-branch clones may lack the base ref
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)"

Both facets reproduced empirically (scratch bare-remote, file:// clones)

Facet A — --depth=1 --single-branch (default actions/checkout shape). Clone has only refs/remotes/origin/feature; git rev-parse --is-shallow-repository → true; rung 1 git merge-base origin/HEAD HEAD → fatal: Not a valid object name; ls-remote resolves D=main; git fetch origin main --depth=1 then git merge-base FETCH_HEAD HEAD → empty (no common ancestor); ladder falls to echo HEAD → git diff HEAD empty → silent no-op. Then git fetch --unshallow origin flips is-shallow-repository→false and merge-base FETCH_HEAD HEAD resolves. Matches the brief.

Facet B — full-depth --single-branch, PR base = non-default (release), feature branched off release. git fetch origin release populates FETCH_HEAD but never creates refs/remotes/origin/release (git rev-parse origin/release → fatal: Needed a single revision); rung 1 fails; rung 2 fetches the default branch, overwriting FETCH_HEAD, and diffs against the main merge-base. Concrete wrong result:

CURRENT LADDER  base=main-merge-base → diff = FEAT-1, REL-only-2, REL-only-1   (3 commits — folds base branch in)
CORRECT         base=merge-base(release,feature) → diff = FEAT-1               (1 commit)

The review sees 3 commits when it should see 1 — the base branch's pre-existing changes silently folded in as if they were the PR's.

External facts, corroborated against official docs (not just training data)

• actions/checkout default fetch-depth: 1 — README: "Number of commits to fetch. 0 indicates all history for all branches and tags. Default: 1" → the affected shape is the default, not an edge case.
• A normal git fetch overwrites FETCH_HEAD — git-fetch(1) --append: "Append ref names and object names of fetched refs to the existing contents of .git/FETCH_HEAD. Without this option old data in .git/FETCH_HEAD will be overwritten." → this is the mechanical root cause of Facet B rung 2 clobbering the captured base.
• --unshallow fetches as much as possible to complete a shallow repo (unbounded download); --deepen=<n> adds n commits from the current shallow boundary — git-fetch(1).

---

Recommendation

Facet B — pure correctness, no fork; ship independent of the Facet-A decision

A wrong/misleading diff base is never acceptable, and fanout/SKILL.md already forbids it (pre-flight gate: "do NOT silently substitute a different base — that reviews the wrong diff"). Fix: capture the PR base as an explicit rev before any fallback runs, and merge-base against the captured rev — never re-reference origin/$PR_BASE (absent in single-branch clones) and never let the default-branch rung overwrite it. Empirically verified: capture BASE="$(git rev-parse FETCH_HEAD)" immediately after git fetch origin "$PR_BASE", then git merge-base "$BASE" HEAD → correct 1-commit diff.

Acceptance (executable, no contract guessing): in a --single-branch clone whose branch is based off a non-default base, the resolved diff base equals merge-base(<PR base tip>, HEAD) and the diff contains only the PR's own commits — not the base branch's pre-existing commits. Reproduction harness: bare remote with feature→release→main, --single-branch --branch feature; assert diff = 1 commit, not 3.

Facet A — recommended Option 2 (report-and-stop default) + opt-in deepen layer, but this default is the reserved product call

Recommended shape (ratifies the maintainer's own brief):

• Default: replace the echo HEAD terminal with the convention's prescribed report-and-STOP (fanout/SKILL.md pre-flight gate already mandates "report which ref failed and STOP" and "Never a hardcoded git diff HEAD, which is empty on a clean committed branch" — the agents currently violate both). Add git rev-parse --is-shallow-repository to discriminate genuinely-clean branch from shallow-broke-the-base and emit an actionable message, e.g. "shallow clone — cannot compute a diff base; re-checkout with fetch-depth: 0."
• Opt-in escalation (compatible layer, not a competing option): gate git fetch --unshallow + retry behind an explicit opt-in (env var, e.g. REVIEW_UNSHALLOW=1), default off. This satisfies the issue's sole stated constraint that the deepen cost "be chosen deliberately, not baked into an unattended pass."

Why this stays needs-human (honest scope boundary): research settled every fact, but the facts do not force the default. Deepen-as-default is also buildable and convention-citable — "always produce a real review, pay the fetch cost" is a coherent stance, and fanout's report-when-unresolvable rule only governs the terminal, not whether --unshallow is a resolution rung tried first. Choosing between "fail honest and cheap, changing behavior on the most common CI shape" and "always deepen, at unbounded cost in an unknown consumer repo" is a product/operational judgment — precisely this issue's ratified triage ("not a single maintainer-vetoable default"). So the recommendation is offered for maintainer ratification, and the labels are left as-is.

Acceptance once the default is chosen:

• If Option 2: depth-1 clone on a clean-but-shallow branch → the pre-flight gate STOPs, names the fetch-depth: 0 fix, and spawns zero reviewers (matching fanout's existing gate); with REVIEW_UNSHALLOW=1 set, the path unshallows and resolves a real base. Repro: the Facet-A harness above.
• If deepen-by-default: the shallow path unshallows unconditionally and resolves; document the cost in the plugin's CHANGELOG/known-limitations.

Structural (maintainer's stated intent — left to the lane's design)

The maintainer wants the base-resolution logic to "live once in a shared default-branch/base helper rather than across five one-liners." Endorsed as direction, but the mechanism is a genuine implementation-design choice, not a research fact: the four agents run as isolated subagents and fanout's is a SKILL !-precompute, so a runtime-shared helper needs a real home (e.g. a ${CLAUDE_PLUGIN_ROOT}/… script each surface invokes) rather than an inlined snippet copied five times. Leave the factoring to the implementing lane's planning phase; don't over-constrain it here.

---

Label disposition

Leaving needs-human + status: needs-decision in place. The technical research is complete and the Facet-B correctness fix is ready to execute now; the Facet-A default awaits a one-read maintainer ratification of Option 2 (the blast-radius unknown that made it feel heavier is now resolved: home CI is unaffected). Not relabeling to status: ready, because that would exercise a maintainer-vetoable-default mechanism on the one point the maintainer deliberately reserved.

Class B research-resolution dispatch (control tower). Operator veto window applies to this recommendation.