fix(source-control): resolve worktree-create fresh base from any default remote

[kyle-sexton]

Closes #904

Summary

worktree-create.sh --base-ref fresh resolved the base ref by probing refs/remotes/origin/HEAD and nothing else. A repository cloned with git clone -o upstream has no origin at all, so fresh took the remoteless local-HEAD fallback even though upstream/HEAD was correctly cached — and a worktree created from a feature branch carried unpushed local commits into a base that fresh promises is the remote default branch.

Fix

fresh now resolves the effective default remote before probing any symref, through the three-rung chain the issue prescribes:

1. the current branch's configured remote (branch.<name>.remote)
2. origin, when it exists
3. the sole remote, when the repository has exactly one

The resolved remote's HEAD symref supplies the base. Nothing hardcodes a default branch name — resolution stays symbolic, as the portability lint requires.

Details worth a reviewer's attention:

• Rung 1 rejects two values. A configured remote is accepted only when it names a remote that still exists, so stale config cannot shadow a healthy origin; and git's . sentinel (meaning "tracks a local branch", not a remote — refs/remotes/./HEAD is nonsense) is refused. A detached HEAD has no branch, so the rung is skipped rather than erroring.
• The HEAD probe deliberately does not cascade back down the rungs. fresh means the effective remote's default branch; quietly substituting a different remote's default branch is a worse failure than the fallback, because the caller cannot see it happen.
• The HEAD fallback and its loud warning remain for the genuinely unresolvable cases, and the warning now names the cause: the resolved remote whose HEAD is uncached (with the git remote set-head <remote> --auto fix), or the absence of any default remote.
• Every git read in the resolver is tr -d '\r'-trimmed. Under git.exe on an MSYS/Cygwin shell the output carries CRLF, and an untrimmed upstream\r would make each downstream lookup miss while still reading correctly in an error message.
• Behavior change beyond the non-origin-clone case: rung 1 also applies in repos that do have origin. When branch.<name>.remote names a different existing remote, fresh now bases on that remote's default branch. That is the prescribed precedence; it is called out in the CHANGELOG so it is not a surprise.

This closes a gap the helper shared with Claude Code's own native fresh, which keeps origin/HEAD current and falls back to local HEAD when origin/HEAD is absent (fetched and verified while preparing this change). The helper is now deliberately more general than the native behavior in remote resolution — though not a strict superset, since it reads the cached ref rather than fetching; create.md says so explicitly.

Test plan

plugins/source-control/scripts/worktree-create.test.sh — 128 assertions, exit 0, no failures or skips. The fixture gained --remote-name plus helpers that seed a second remote at a distinguishable tip.

New coverage: a sole non-origin remote; the uncached-<remote>/HEAD warning naming the resolved remote; branch-config precedence over a coexisting origin; a stale branch-configured remote; the . sentinel; a detached HEAD; several remotes with no resolvable default; the remoteless case.

The new tests were checked against the pre-change helper to prove they are not vacuous. Running the updated suite against the old script (same fixtures, unmodified) produces five discriminating failures:

• fresh bases on upstream/HEAD, not local HEAD — the exact bug in the issue
• warning names the resolved remote's symref
• warning offers set-head for that remote
• branch-configured remote wins over origin
• ambiguous remotes name the no-default-remote cause

The precedence test asserts against three distinct commits (origin tip, upstream tip, local HEAD) so it cannot pass whichever remote is picked. Guard-only assertions (detached HEAD, remoteless, stale/. fallthrough) pass under both scripts by design — they are regression guards, not discriminators.

Other gates, all green: shellcheck -x clean on both scripts; scripts/check-shell-portability.sh introduces no new hits (the two reported are pre-existing lines, unchanged and only shifted); scripts/check-skill-portability.sh clean; scripts/check-changelog-parity.sh --check and --check-bump origin/main pass; scripts/validate-plugins.sh passes; markdownlint-cli2 reports 0 errors.

Independent fresh-context review ran with the implementation rationale withheld; every finding it raised was fixed before this PR. Two of its catches are worth naming: two new tests could have passed on a failing helper (an empty $out degrades the probed path to /UNPUSHED.md, which is absent) and now carry exit + positive-existence guards, and the new warning templating had no discriminating assertion at all — the case that covers it is the upstream/HEAD not set / set-head upstream --auto pair above. Its shellcheck pass also caught a defect this change introduced: the added --help text used backticks inside the unquoted usage() heredoc, which would have executed git clone -o upstream on every --help. Fixed; --help now renders the text literally.

Related

• Raised by a Codex review thread on feat(source-control): route /worktree create through a shared worktree-creation helper (#399 Phase A) #898 (the PR for feat(source-control): unify native-hook + /worktree-skill worktree creation via one shared helper #399, Phase A), which added this helper.
• Deliberately not addressed: parse_owner_repo's remote lookup in the same file still probes origin only, so in a -o upstream clone the owner/repo directory name degrades to the repo-directory name. It is outside this issue's acceptance criteria, and changing it would move existing worktree directory names — a separate decision from the base-ref fix.
• Pre-existing, not introduced here: a dangling <remote>/HEAD symref (present, but its target ref absent — e.g. after an upstream default-branch rename) makes symbolic-ref succeed, so git receives a non-existent ref and the helper exits 4 with a message about the branch already existing. The old code had the identical hole.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e059da050c

ℹ️ 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 encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

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

---

PR Review — fix(source-control): resolve worktree-create fresh base from any default remote

• Gather context and understand the request
• Read changed files
• Review worktree-create.sh implementation
• Review worktree-create.test.sh test coverage
• Review create.md skill doc and CHANGELOG.md
• Check Codex review finding (option-shaped remote names)
• Post comprehensive review

---

This is a high-quality fix with thorough tests, solid rationale, and careful handling of every edge case it introduces. The design decisions (no HEAD-probe cascading, explicit fallback warning, rung ordering, CRLF trimming) are well-reasoned and clearly documented. One actionable correctness bug was found — the Codex P2 finding — and the rest of the review is notes on things that are fine.

Finding: git remote get-url should use -- before "$cfg" (correctness)

worktree-create.sh:477:

if [[ -n "$cfg" && "$cfg" != "." ]] && git -C "$repo_top" remote get-url "$cfg" >/dev/null 2>&1; then

The Codex finding here is confirmed. Git allows remote names beginning with - (via git remote add -- -foo <url>), and git remote get-url -foo mis-parses -foo as option flags rather than a remote name, returning a non-zero exit for the wrong reason. The error is eaten by >/dev/null 2>&1, so rung 1 silently falls through to rung 2 — the helper picks origin instead of the branch-configured remote with no diagnostic to the caller. The fix is a single character:

if [[ -n "$cfg" && "$cfg" != "." ]] && git -C "$repo_top" remote get-url -- "$cfg" >/dev/null 2>&1; then

Rung 2's remote get-url origin is safe (hardcoded literal, never starts with -). Fix this →

---

Notes (not blocking, all fine as-is)

Implementation logic is correct end-to-end. The three-rung chain handles every edge case: detached HEAD skips rung 1 (symbolic-ref yields nothing, [[ -n "$branch" ]] is false); the . sentinel is excluded by "$cfg" != "."; stale config is excluded by requiring remote get-url to succeed; the ambiguous multi-remote case (neither branch-config, origin, nor sole-remote) correctly declines to guess and falls back to HEAD with a named cause. The decision not to cascade the HEAD probe back down the rungs is sound — a silent wrong-remote substitution would be a worse failure than an honest fallback warning.

CRLF trimming is consistently applied. tr -d '\r' on symbolic-ref, config --get, and the ${r%$'\r'} strip in the array loop. The symbolic-ref in the fresh case block (line 528) also gets tr -d '\r'. This is complete.

The HEAD-ref value flows safely. head_ref on line 528 is either empty or the output of symbolic-ref -q — a well-formed git ref like refs/remotes/upstream/main. It's passed quoted to git worktree add on line 549, so no injection risk.

Test suite is thorough and correctly discriminating. The branch-config precedence test (lines 327–335) uses three distinct commits — origin/HEAD at the initial commit, upstream/HEAD at commit 2 (with UPSTREAM_ONLY.md), and local HEAD at commit 3 (with UNPUSHED.md) — so it provably cannot pass whichever remote the helper happens to pick. The positive-existence guards on the stale-remote and sentinel tests (e.g., assert_file_exists "stale branch remote materializes the worktree") prevent an empty $out from causing absent-file assertions to pass vacuously. The newly added assert_file_exists "base-ref fresh materializes the worktree" guard on the pre-existing case (line 258) follows the same pattern.

create.md accurately describes the updated behavior, including the explicit note that this is "more general than Claude Code's native fresh in remote resolution" but not a strict superset since the helper reads the cached ref rather than fetching.

CHANGELOG is accurate and calls out the non-obvious behavior change (rung 1 also affects repos that have origin — when branch.<name>.remote names a different remote, fresh now bases on that remote's default branch rather than origin's). This is the kind of change that could surprise a caller who wasn't aware of it, so the explicit call-out is appropriate.

---

Summary: One confirmed correctness bug (Codex P2 — add -- before "$cfg" in remote get-url). Everything else is in good shape. The fix is a one-character change; the rest can merge as-is if that's addressed.

[claude]

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

---

Security Review

• Fetch PR diff
• Read changed shell scripts
• Analyze for injection surfaces, command injection, path traversal, trust boundary issues
• Post findings

---

Reviewed commit a1f87a1. Security scope: the new resolve_default_remote function and the updated fresh base-ref resolution block.

SUGGESTION — option-shaped remote name bypasses rung 1 validation silently (confidence: high)

File: plugins/source-control/scripts/worktree-create.sh#L477

if [[ -n "$cfg" && "$cfg" != "." ]] && git -C "$repo_top" remote get-url "$cfg" >/dev/null 2>&1; then

Git permits remote names that begin with - via git remote add -- -upstream …. If branch.<name>.remote is set to such a value, git remote get-url -upstream parses -upstream as an option flag, exits non-zero with unknown switch 'u', and the rung-1 validation silently falls through to rung 2 (origin) — even when the named remote exists and is healthy. The branch-configured remote is then ignored in favor of origin, contradicting the documented precedence.

The practical impact is limited: the worst outcome is the wrong base commit being used for the new worktree. No code execution path exists here — $cfg is never passed to a shell interpreter, and even if default_remote ends up being an option-shaped name (via rung 3), line 528 constructs refs/remotes/$default_remote/HEAD, which starts with refs/ and is not parsed as a flag by symbolic-ref. This is a correctness-as-security concern rather than a traditional vulnerability, and exploiting it requires prior control over git config. The Codex review flagged the same issue (P2).

Fix: add -- before "$cfg" to terminate option parsing:

git -C "$repo_top" remote get-url -- "$cfg" >/dev/null 2>&1

Fix this →

---

No other security findings

The remainder of the changed surface is clean:

• All printf calls that include user-controlled values ($default_remote, $PROG) use %s positional arguments — no format-string injection.
• $branch (from symbolic-ref --short) and $cfg are used only inside double-quoted strings passed to git subcommands — git enforces that branch and remote names cannot contain the shell metacharacters that would escape these quotes.
• $default_remote is interpolated into refs/remotes/$default_remote/HEAD within double quotes; git remote names cannot contain /, .., or null bytes, so no ref-path traversal is possible, and the leading refs/ prefix prevents it from being parsed as a flag by symbolic-ref.
• No credentials, tokens, or secrets are read, logged, or passed through the new code paths.
• The $toplevel variable is unchanged and originates from git rev-parse --show-toplevel, which is safe.

[claude]

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

---

PR Review — fix(source-control): resolve worktree-create fresh base from any default remote

• Gather context and understand the request
• Read current diff (origin/main...HEAD)
• Review worktree-create.sh at current HEAD — verify -- fix and check for new issues
• Review worktree-create.test.sh — verify new regression test
• Post comprehensive review

---

This is a follow-up review at current HEAD (f877dc3), which includes commit 17c3f9d (fix(source-control): terminate options before the branch-configured remote name) addressing the P2 finding from the previous review. The fix has been applied correctly, and no new issues were found.

Previously-found P2: confirmed resolved

worktree-create.sh#L484:

if [[ -n "$cfg" && "$cfg" != "." ]] && git -C "$repo_top" remote get-url -- "$cfg" >/dev/null 2>&1; then

The -- option terminator is present. The surrounding comment (lines ~476–483) explains the why thoroughly: git clone -o -foo is a legal invocation, writes -foo into branch.<name>.remote, and without -- that value is parsed as switches. This is both fixed and documented in the CHANGELOG under [0.35.1].

A dedicated regression test (--- Case: an option-shaped branch-configured remote still outranks origin ---) verifies it with three distinct commits (origin tip, -foo tip with DASHED_ONLY.md, local HEAD with UNPUSHED.md), so the assertion discriminates all three possible outcomes.

---

Overall assessment

Implementation is correct end-to-end. The three-rung chain in resolve_default_remote handles every documented edge case:

• Detached HEAD: symbolic-ref --quiet yields nothing → [[ -n "$branch" ]] is false → rung 1 skipped, not an error.
• . sentinel: excluded by "$cfg" != "." before the get-url call.
• Stale config: excluded by requiring remote get-url to exit 0.
• Option-shaped remote: handled by --.
• Ambiguous multi-remote (no branch config, no origin, more than one): ${#remotes[@]} == 1 is false → returns non-zero → default_remote="" → HEAD fallback with the "no default remote" cause message.

The HEAD probe non-cascading decision is correct. Setting head_ref="" before the conditional and not re-trying other remotes when the resolved remote's HEAD is uncached is the right call: a silent wrong-remote substitution is worse than an explicit fallback with a named cause.

CRLF trimming is complete. tr -d '\r' is applied to all three git reads in resolve_default_remote (symbolic-ref, config --get, and ${r%$'\r'} in the array loop), and the symbolic-ref in the fresh block (which constructs head_ref) also pipes through tr -d '\r'.

$default_remote in refs/remotes/$default_remote/HEAD is safe. Git remote names cannot contain /, so no path traversal; the leading refs/remotes/ prefix prevents the value from being parsed as a flag by symbolic-ref; even an option-shaped name like -foo produces refs/remotes/-foo/HEAD, which is a well-formed ref path.

The --help backtick fix is in. The usage() heredoc now uses 'git clone -o upstream' (single-quoted) rather than a backtick-delimited span that would have executed the command on every --help invocation.

Test suite is discriminating. The branch-config-wins-over-origin case uses three distinct commits so it cannot pass regardless of which remote is picked. Each assert_file_absent pair is preceded by an assert_file_exists guard on a file the helper would only produce for a specific remote, preventing vacuous passes when $out is empty. The assert_exit guard at the top of each new case would also catch a silent helper failure before the file assertions run.

CHANGELOG and documentation are accurate. The 0.35.1 entry calls out the non-obvious behavior change (rung 1 also affects repos with origin when branch.<name>.remote names a different remote) — exactly the right thing to flag explicitly. create.md correctly notes that this is more general than Claude Code's native fresh in remote resolution but not a strict superset (reads cached ref vs. fetching).

---

Notes (not blocking)

• printf 'origin' at rung 2 (no format-string argument) is fine — the literal has no % characters — but the style is inconsistent with the printf '%s' "$cfg" calls on either side of it. This is cosmetic.
• The double space in "Run: git remote set-head %s --auto to cache it.\n" (two spaces before "to") appears intentional for visual separation between the command and the explanation.

---

Summary: The P2 finding from the previous review is fixed, the regression test is correctly discriminating, and no new issues were found. This PR is in good shape to merge.

[claude]

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

---

Security Review — fix(source-control): resolve worktree-create fresh base from any default remote

• Fetch PR diff and read changed files
• Audit worktree-create.sh for injection surfaces, trust boundary issues, unsafe variable handling
• Audit worktree-create.test.sh for test security concerns
• Check CHANGELOG.md, plugin.json, create.md for secrets or unsafe content
• Post findings

---

Reviewed at f877dc3. Security scope: the resolve_default_remote function, the updated fresh base-ref resolution block, and the new test fixtures.

No new security findings

The prior security review (14:11 UTC) found one confirmed issue — the missing option terminator before "$cfg" in git remote get-url. That finding is fixed in the current HEAD: worktree-create.sh:484 now reads git -C "$repo_top" remote get-url -- "$cfg", with the load-bearing -- present and explained in the comment block immediately above it. No re-report.

The remainder of the new surface is clean:

Command injection / option injection. $branch (from symbolic-ref --short) and $cfg (from git config --get) are passed only inside double-quoted arguments to git subcommands — no word splitting or glob expansion can escape. $default_remote is interpolated into "refs/remotes/$default_remote/HEAD", which starts with refs/ and is then passed in double quotes to symbolic-ref -q; git remote names are validated by git remote add against git's ref-component rules and cannot contain the characters (.., @{, control bytes, spaces) that would corrupt a ref path or be parsed as flags at this position. Rung 3's sole-remote value (${remotes[0]}) follows the same path.

Format string injection. Every printf call that incorporates user-controlled data ($default_remote, $cfg, $PROG) uses a %s positional specifier — none interpolates the value directly as a format string.

CRLF injection. tr -d '\r' is applied to symbolic-ref, config --get, and the rung-3 array loop (${r%$'\r'}). The symbolic-ref in the fresh case block (line 535) is also trimmed. Coverage is complete.

Credential / secret handling. No credentials, tokens, or secrets are read, logged, or transmitted. The new code reads only remote names and cached ref symrefs.

Test file. Temporary directories are created under mktemp -d and cleaned up via trap 'rm -rf "$TEST_TMPDIR"' EXIT. The option-shaped remote test (lines 346–355) uses git -C "$repo" remote add -- "$name" with the option terminator correctly in place. No unsafe temp file patterns, no world-writable paths, no hardcoded credentials.

create.md. The updated base-ref description correctly documents the --root-file / Write-tool approach to prevent ${user_config.worktree_root} substitution from reaching a shell parser. No new trust boundaries introduced.

The changed surface is clear.