fix(toolchain): resolve a present remote for default-branch detection instead of assuming origin - #528
Conversation
… instead of assuming origin The clean-working-tree branch-diff fallback in /toolchain:check and /toolchain:lint forced REMOTE=origin whenever the current branch had no tracking remote (an unpushed feature branch). In a clone made with a differently named remote (git clone -o vendor) that has no origin, every default-branch probe then failed and the branch diff was skipped. Both call sites now keep origin when present and otherwise select the first present remote, so detection resolves against a remote that actually exists; it still degrades gracefully when no remote is present. Stale prose asserting the old origin fallback "still resolves" a non-origin clone is corrected in both files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 1m 28s —— View job PR Review: fix(toolchain) — present-remote selection for default-branch detection
SummaryClean, minimal, correctly-scoped bug fix. The shell logic is sound, all edge cases degrade gracefully, and the common case is preserved exactly. No issues that would block merging. Shell logic — correctness and edge casesThe new block in both SKILL.md files: if [[ -z "$REMOTE" || "$REMOTE" == "." ]]; then
REMOTE=origin
git remote | grep -qx origin || REMOTE=$(git remote | head -n1)
fiStepping through each scenario:
The "set-origin-first, then check" pattern reads slightly backwards (assign then maybe overwrite), but it makes the Duplication across check/lint SKILL.mdThe same block is duplicated in two files. The PR body calls this out explicitly and references #436/#442 as the tracked cross-plugin opportunity for a shared helper. That's the right call — pulling in a shared helper is a cross-version, cross-plugin change that would widen the scope of this patch substantially. The duplication is contained (two identical blocks) and appropriate for a targeted bug fix. DocumentationThe prose update is accurate and more precise than what it replaces. The old text claimed CHANGELOG and versioning
Alignment with CLAUDE.md guidelines
The Minor observation (no action needed)
Verdict: LGTM. The fix is correct, well-scoped, and the documentation and changelog accurately describe the change. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 442145cb2f
ℹ️ 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".
…miter
The default-branch resolution stripped the `$REMOTE/` prefix with
`sed "s#^$REMOTE/##"`. When the selected remote name contains `#`
(a Git-legal character, e.g. `foo#bar`), that `#` collides with the
sed delimiter and corrupts DEFAULT_BRANCH (`foo#bar/main` -> `bar/#bar/main`),
so the rev-parse guard silently skips the branch diff. Both `/toolchain:check`
and `/toolchain:lint` now strip the prefix with `${DEFAULT_BRANCH#"$REMOTE/"}`,
which treats the remote name literally regardless of its characters.
Addresses codex P2 on #528.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 47s —— View job PR Review: fix(toolchain) — present-remote selection + sed delimiter fix
Shell logic — correctness and edge casesBoth SKILL.md files now contain the same corrected block: REMOTE=$(git config "branch.$(git branch --show-current | tr -d '\r').remote" 2>/dev/null | tr -d '\r')
if [[ -z "$REMOTE" || "$REMOTE" == "." ]]; then
REMOTE=origin
git remote | grep -qx origin || REMOTE=$(git remote | head -n1)
fi
DEFAULT_BRANCH=$(git symbolic-ref --short "refs/remotes/$REMOTE/HEAD" 2>/dev/null)
DEFAULT_BRANCH=${DEFAULT_BRANCH#"$REMOTE/"}Fix 1 — Present-remote selection (check/SKILL.md:61–64, lint/SKILL.md:63–66):
Fix 2 — Parameter expansion for prefix strip (check/SKILL.md:66, lint/SKILL.md:68): The old Codex finding (P2) — already addressedThe Codex review flagged the sed delimiter collision against commit Documentation and CHANGELOG
CLAUDE.md alignment
Verdict: LGTM. Both fixes are correct and well-scoped. The Codex finding was valid against the first commit and is resolved in the second. No blocking issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23fcdad051
ℹ️ 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".
…t the first present one The clean-working-tree branch-diff fallback in /toolchain:check and /toolchain:lint picked `git remote | head -n1` when the branch had no tracking remote and no `origin`. A remote added but never fetched (e.g. `backup` alongside a `vendor` clone) has no local `refs/remotes/<remote>/*`, so its `git ls-remote --symref` default-branch query succeeds over the network but `git rev-parse <remote>/<branch>` fails — the branch diff was skipped even though a later remote (`vendor/main`) was usable locally. Both call sites now probe candidate remotes in priority order (tracking remote, then `origin` if present, then the rest) and select the first whose default branch resolves to a locally available tracking ref. The common tracking-remote case still short-circuits on the first candidate with no extra network calls, and detection still degrades gracefully when no candidate yields a local default branch. Verified empirically: the never-fetched-remote case now selects the usable remote; a pushed feature branch tracking `origin` still selects `origin/<default>` (not the feature ref, so merge-base is non-empty); no-remote degrades gracefully. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 16s —— View job PR Review: fix(toolchain) — probe remotes for available default-branch ref (all 3 commits)
What changed in
|
| Scenario | CANDIDATES order | Loop selects | Correct? |
|---|---|---|---|
Tracking remote = origin, only remote |
origin |
origin (first try) |
✅ common case, no extra calls |
Tracking remote = upstream, origin also present |
upstream, origin |
upstream (first, if fetchable), else origin |
✅ tracking remote priority preserved |
Unpushed branch, vendor clone + unfetched backup |
backup, vendor |
backup tried, fails rev-parse → vendor selected |
✅ Codex P2 fixed |
| No remotes at all | "" (empty) |
loop body: both probes fail, REMOTE stays "" |
✅ graceful degradation |
| All remotes unfetched | each fails rev-parse | loop exhausted, REMOTE stays "" |
✅ graceful degradation |
The guard that makes it correct: git rev-parse --verify --quiet "$CANDIDATE/$BRANCH" — a candidate is only accepted when its default-branch tracking ref exists locally. ls-remote network success without a local ref is explicitly skipped. This is the precise fix the Codex finding required.
Parameter expansion (${BRANCH#"$CANDIDATE/"}) carries forward correctly from 23fcdad — quoting "$CANDIDATE/" inside the expansion suppresses glob interpretation, so remote names with #, *, ?, or other metacharacters are treated literally.
awk 'NF && !seen[$0]++': NF (field count) is zero for empty lines, so they are filtered. The !seen[$0]++ idiom prints first-occurrence lines only. Correct deduplication.
while IFS= read -r ... done <<< "$CANDIDATES": Standard bash idiom. IFS= preserves leading/trailing whitespace. -r prevents backslash interpretation. When CANDIDATES is empty, the herestring feeds one empty line: the loop runs once with CANDIDATE="", both probes fail (guarded by [[ -n "$BRANCH" ]]), and REMOTE stays "". Not a bug — just an extra no-op iteration.
Security: $CANDIDATE originates from git remote (reads .git/config) and is always quoted when passed to git subcommands. No user-controlled external input is interpolated into shell expressions. Clean.
Network-call behaviour
The ls-remote fallback fires when git symbolic-ref refs/remotes/$CANDIDATE/HEAD is absent. For the common pushed-branch case, this symref is set and no network call is made — the commit message claim ("short-circuits on the first candidate with no extra network calls") holds. For a never-fetched candidate, ls-remote does fire, returns a branch name, and then rev-parse rejects it. That network call is unavoidable to determine the default branch name; the current design accepts this as the right trade-off.
Documentation and CHANGELOG
- Prose in both SKILL.md files accurately describes the three-tier priority and the "accepted only when
git rev-parseconfirms the tracking ref exists locally" criterion. Clear improvement over the previous text. - CHANGELOG entry at
d334b5c..5aafd7ecovers all three commits (present-remote selection,seddelimiter fix, and the probing loop) in a single[0.4.2]block. Format and content match Keep a Changelog. ✅ - Version at
0.4.2(bumped in the first PR commit, not re-bumped ind64b22c— correct). ✅
CLAUDE.md alignment
| Criterion | Status |
|---|---|
| Repo-agnostic — no hardcoded remote names | ✅ substantially improved across all three commits |
| Configurable without editing the plugin | ✅ unchanged |
Plugin-form-safe — no ../ reach-outs |
✅ |
| Explicit semver bump | ✅ 0.4.1 → 0.4.2 |
| No injection surface | ✅ all variables quoted; no user-controlled shell interpolation |
Minor observation (no action needed)
The [[ -n "$TRACKED" ]] && echo "$TRACKED" construct inside the compound group short-circuits without output when TRACKED is empty — relying on exit code 1 from [[. Inside { }, that exit code is swallowed by the pipeline to awk, so this is correct, just unconventional. Functionally identical to if [[ -n "$TRACKED" ]]; then echo "$TRACKED"; fi.
Verdict: LGTM. All three Codex findings are addressed (P2 sed delimiter collision in 23fcdad, P2 first-remote-not-fetched in d64b22c). The shell logic is correct across every reachable scenario, the common case is unaffected, and degradation is still graceful. No blocking issues.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d64b22c725
ℹ️ 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".
…emote names The candidate-remote loop in /toolchain:check and /toolchain:lint passed the remote name and the `<remote>/<branch>` tracking ref as bare positional arguments to `git ls-remote`, `git rev-parse`, and `git merge-base`. A Git-legal remote whose name begins with a dash (e.g. `git clone --origin=-x`) is then parsed as a command option, so with no `origin` and an unpushed branch that remote is skipped and the branch diff reported unavailable even though its `refs/remotes/-x/main` tracking ref exists locally. Both call sites now verify and diff against the fully-qualified `refs/remotes/<remote>/<branch>` form (no leading dash exposed, and no shadowing by a local branch literally named `<remote>/<branch>`) and pass `--end-of-options` to the `git ls-remote` probe. Verified empirically against a `git clone --origin=-x` clone: the dash-named remote is now selected and diffed correctly, and the origin/vendor/no-remote scenarios are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 46s —— View job PR Review: fix(toolchain) — dash-leading remote name safety (commit
|
| Surface | Old form | New form | Technique | Safe? |
|---|---|---|---|---|
git symbolic-ref |
"refs/remotes/$CANDIDATE/HEAD" |
unchanged | refs/ prefix, never parsed as option |
✅ was already safe |
git ls-remote |
--symref "$CANDIDATE" HEAD |
--symref --end-of-options "$CANDIDATE" HEAD |
--end-of-options terminates option parsing |
✅ fixed |
git rev-parse |
"$CANDIDATE/$BRANCH" |
"refs/remotes/$CANDIDATE/$BRANCH" |
refs/ prefix, parsed as refspec not option |
✅ fixed |
git merge-base |
"$REMOTE/$DEFAULT_BRANCH" |
"refs/remotes/$REMOTE/$DEFAULT_BRANCH" |
refs/ prefix, parsed as refspec not option |
✅ fixed |
${BRANCH#"$CANDIDATE/"} |
unchanged | unchanged | bash parameter expansion, no subprocess | ✅ never at risk |
Two distinct techniques, both correct:
-
For
git ls-remote:--end-of-optionsis the right mechanism. It's part of git'sparse_options()infrastructure since git 2.24 (Nov 2019). Any command using that infrastructure, includingls-remote, picks it up. The flag order--symref --end-of-options "$CANDIDATE" HEADis correct —--symrefis parsed as an option first, then--end-of-optionsterminates option processing, then"$CANDIDATE"is taken as the repository positional. -
For
git rev-parseandgit merge-base: using the fully-qualifiedrefs/remotes/$CANDIDATE/$BRANCHpath is strictly better than--, becauserefs/...strings are never parsed as option flags regardless of their content. This also removes the need for a local branch named after the default branch to exist — only the remote-tracking ref need be present.
The approach the Codex finding suggested ("add --") would have worked too, but the chosen alternatives are cleaner: the refs/-prefix approach requires no separator at all, and --end-of-options is the git-standard way to do what Codex asked for.
No remaining unsafe surfaces
Tracing the full data flow for a dash-leading $CANDIDATE = "-x", $BRANCH = "main", $REMOTE = "-x", $DEFAULT_BRANCH = "main":
git symbolic-ref --short "refs/remotes/-x/HEAD"→-xis inside the string, not a positional ✅git ls-remote --symref --end-of-options "-x" HEAD→--end-of-optionsguards the positional ✅git rev-parse --verify --quiet "refs/remotes/-x/main"→refs/prefix guards ✅git merge-base "refs/remotes/-x/main" HEAD→refs/prefix guards ✅${BRANCH#"-x/"}→ parameter expansion only ✅
All four git-calling surfaces are addressed. No remaining unsafe path.
Edge cases — unchanged behavior
| State | Result | Correct? |
|---|---|---|
| No remotes | loop body: both probes fail, REMOTE="", branch-diff skipped |
✅ graceful |
Common case: tracking remote = origin |
first candidate succeeds on local symref, no ls-remote call | ✅ common-case unchanged |
Pushed branch, non-origin clone |
tracking remote tried first, succeeds | ✅ unchanged |
| Only dash-leading remote, never fetched | ls-remote succeeds (network), rev-parse fails (no local ref) → loop exhausted, REMOTE="" |
✅ graceful |
| Only dash-leading remote, fetched | ls-remote + rev-parse both succeed, REMOTE="-x" |
✅ now works — Codex finding fixed |
Minor observations (no action needed)
git ls-remote --end-of-options minimum git version. The --end-of-options flag is part of git's parse_options() infrastructure since 2.24.0 (Nov 2019). Any git ≥ 2.24 supports it in all commands using that infrastructure. This covers all currently-maintained Linux, macOS, and Windows environments. Not a practical concern in 2026, but worth knowing the floor.
Prose accuracy. The explanatory paragraph in both files (check/SKILL.md:80, lint/SKILL.md:82) explicitly calls out both techniques and names the git clone --origin=-x scenario. Clear and correct.
Duplication across check/lint. Both blocks are byte-for-byte identical. Still appropriate for a targeted bug fix; the cross-plugin shared helper remains the tracked follow-up (#436/#442).
Verdict: LGTM. All three Codex findings across the PR's four commits are now fully addressed. The dash-leading-name fix in d726a8b correctly defends every git-calling surface using the right technique for each. No blocking issues.
Summary
/toolchain:checkand/toolchain:lint, when the working tree is clean, fall back to a branch diff against the default branch. Resolving that default branch depends on picking the right remote. Both call sites read the current branch's tracking remote (branch.<name>.remote) and, when it was unset, forcedREMOTE=origin. On an unpushed feature branch in a clone made with a differently named remote (git clone -o vendor), there is no tracking remote and noorigin, so every default-branch probe failed and the branch diff was silently skipped ("branch diff unavailable"). The common case (a pushed branch, or a plainoriginclone) was unaffected, and the failure degraded gracefully rather than producing a wrong diff.Fix
When the branch has no tracking remote, select a remote that is actually present instead of blindly assuming
origin.Before:
After:
This preserves the common-case behavior exactly —
originis still chosen when it is present — and only diverges whenoriginis absent, in which case the first present remote is used. When no remote exists at all,$REMOTEis empty and the existingrev-parse --verifyguard skips the branch-diff path, so detection still degrades gracefully. The stale prose in both skills that claimed the oldoriginfallback "still resolves" a non-origin clone is corrected to describe present-remote selection. The change is applied identically inskills/check/SKILL.mdandskills/lint/SKILL.md.Verification
Repo-pinned gates on the changed files — all clean:
shellcheck(repo.shellcheckrc) on the extracted detection block: cleanmarkdownlint-cli2on bothSKILL.mdfiles andCHANGELOG.md:0 error(s)editorconfig-checkeron all four changed files: cleantyposon all four changed files: cleanEmpirical demonstration on the exact reported scenario — a
git clone -o vendor(noorigin) with an unpushedfeaturebranch (branch.feature.remoteunset) and one committed changef.txt:Common-case regression check — with
originpresent alongside another remote,originis still selected (clone -o origin (+extra) -> REMOTE=origin); only whenoriginis absent is another present remote chosen (clone -o vendor (+extra) -> REMOTE=other).Version bumped
0.4.1->0.4.2(patch, bug fix) with a matchingCHANGELOG.mdentry.Related
<default-branch>placeholder in check/lint has no repo-agnostic resolution guidance #411 (source issue), review: origin/main baked as default-branch fallback across ~7 surfaces #436 (review:origin/mainbaked across ~7 surfaces), source-control: remote nameoriginhardcoded in pull-request create flow (low severity) #442 (source-controloriginhardcoded)This fix is deliberately toolchain-local. The broader, correct root-cause fix that #436 and #442 also need is a single shared default-branch/remote helper (the
gh repo view --json defaultBranchRefauthoritative fallback thatrepo-hygiene'sgit-tree-reset.shalready uses). That spans multiple plugins and version files, so it is left as the tracked cross-plugin opportunity rather than pulled into this patch.Closes #483
🤖 Generated with Claude Code