Repository navigation
Conversation
📝 WalkthroughWalkthroughThe change adds shared Git branch-reference conflict utilities. Repository and SSH worktree flows now distinguish blocking prefixes from suffixable directory/file collisions. Worktree and runtime flows stop suffix retries for unsuffixable conflicts and use shared conflict messages. Workspace error classification recognizes existing branch-name collisions as Merge Risk: 🔵 Low · up to The change is mergeable with owner awareness: conflict handling may omit worktree-name guidance in some error messages, and the SSH conflict regression test may not fully validate ancestor-ref probing because of a broad mock. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…se gap Review follow-up from three independent gpt-5.6-luna reviewers on PR #15383. Suffixes are appended to the last segment of a branch name, so every proper prefix survives them: if `release` blocks `release/1.0`, it blocks `release/1.0-2` through `release/1.0-100` too. The create loops treated that conflict like any other and ground through all 100 candidates before failing. Locally that is ~2.7s of wasted work; over SSH each attempt is several sequential remote round trips, so the same dead end costs roughly 400 RPCs. Split the detected collision by direction: - `local-directory` (refs nest under the requested name) stays suffixable — `feature` still resolves to `feature-2` with no user-visible error. - `local-ref-prefix` (an existing ref is a path prefix) is unsuffixable, so all three create loops now break instead of continuing. The message for the unsuffixable case says so, and points at the real fix — rename or delete the blocking branch — rather than "pick a different worktree name", which cannot help. Also documents a limitation the probe does not close: `for-each-ref` matches ref names byte-for-byte, while the ref store on macOS/Windows is usually case-insensitive and may normalize unicode, so `Feature/x` blocking `feature` (or an NFC ref blocking an NFD name) still reaches git. `--ignore-case` is deliberately not used — on a case-sensitive volume git genuinely allows both `feature` and `Feature/x`, so it would block names git accepts. A false positive is worse than the raw error it would avoid; those cases fail exactly as they do today. Reviewers independently confirmed the reachability verdict (the bug is reachable on main today, without PR #15104) and verified multi-pattern `for-each-ref` on real Git 2.25.1 in an Ubuntu 20.04 container. Regression test asserts the SSH create path issues fewer than 5 ref probes instead of 100; reverting the break makes it fail with "expected 100".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a59dd03f-c15b-4ec0-b69e-67bdf7e10ef7
📒 Files selected for processing (7)
src/main/git/branch-ref-conflict.test.tssrc/main/git/branch-ref-conflict.tssrc/main/git/repo.test.tssrc/main/git/repo.tssrc/main/ipc/worktree-remote.tssrc/main/ipc/worktrees-ssh-branch-conflict-suffixing.test.tssrc/main/runtime/orca-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| if (args[0] === 'for-each-ref') { | ||
| return args.some((arg) => arg.startsWith('refs/heads/')) | ||
| ? { stdout: 'refs/heads/release\n', stderr: '' } | ||
| : { stdout: '', stderr: '' } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the blocking ref probe exactly.
This mock returns refs/heads/release for every local-ref query. The test can pass if the implementation probes an unrelated ref or the requested branch instead of its blocking prefix.
Return the blocking ref only when the command requests the release ancestor pattern. Assert that the expected ancestor probe occurred.
|
CI red is not from this PR — The 5 failures are 4 test shards plus the
Cause: #15381 ( Evidence:
No action needed on this branch. This needs a fix on |
Git stores refs as files, so `feature` and `feature/x` can never both exist. `getBranchConflictKind` only probed the exact ref, so neither direction was detected and `git worktree add -b` failed with raw text surfaced through IPC: fatal: cannot lock ref 'refs/heads/feature': 'refs/heads/feature/tti_fix_1440' exists; cannot create 'refs/heads/feature' This is reachable on main today, independent of PR #15104 (still open). The new name never needs a slash: any repo with a nested branch (`feature/x`, `renovate/...`, `dependabot/...`) blocks the flat name typed by the user. The reverse direction needs a slash, which main already produces via the default `git-username` branch prefix, a custom prefix, or a create-from-review `branchNameOverride` such as `release/1.0` over an existing flat `release`. One `for-each-ref` covers both directions: a pattern matches a ref completely or up to a `/`, so the requested name's own pattern finds refs nested under it while each proper-prefix pattern finds a shorter ref blocking it. Prefix patterns over-match, so a prefix only counts on an exact match. A detected conflict returns the new `local-directory` kind, which the existing suffix loops already treat as "try the next candidate" — `feature` resolves to `feature-2` with no user-visible error. The fallback message no longer claims the branch "already exists", which would be false: no ref has that exact name. Wired into the local, WSL, SSH, and runtime create paths. Probe failures are swallowed so they can never block a create git would accept. - `for-each-ref` multi-pattern predates the Git 2.25 baseline; no capability gate needed. Every argv element is `refs/heads/`-prefixed, so a branch name can never be read as an option. - Provider-neutral: classification is local-ref-only, so GitHub/GitLab and the remote-conflict path are untouched. - Telemetry buckets the new message as `path_collision`, not `git_failed`.
…se gap Review follow-up from three independent gpt-5.6-luna reviewers on PR #15383. Suffixes are appended to the last segment of a branch name, so every proper prefix survives them: if `release` blocks `release/1.0`, it blocks `release/1.0-2` through `release/1.0-100` too. The create loops treated that conflict like any other and ground through all 100 candidates before failing. Locally that is ~2.7s of wasted work; over SSH each attempt is several sequential remote round trips, so the same dead end costs roughly 400 RPCs. Split the detected collision by direction: - `local-directory` (refs nest under the requested name) stays suffixable — `feature` still resolves to `feature-2` with no user-visible error. - `local-ref-prefix` (an existing ref is a path prefix) is unsuffixable, so all three create loops now break instead of continuing. The message for the unsuffixable case says so, and points at the real fix — rename or delete the blocking branch — rather than "pick a different worktree name", which cannot help. Also documents a limitation the probe does not close: `for-each-ref` matches ref names byte-for-byte, while the ref store on macOS/Windows is usually case-insensitive and may normalize unicode, so `Feature/x` blocking `feature` (or an NFC ref blocking an NFD name) still reaches git. `--ignore-case` is deliberately not used — on a case-sensitive volume git genuinely allows both `feature` and `Feature/x`, so it would block names git accepts. A false positive is worse than the raw error it would avoid; those cases fail exactly as they do today. Reviewers independently confirmed the reachability verdict (the bug is reachable on main today, without PR #15104) and verified multi-pattern `for-each-ref` on real Git 2.25.1 in an Ubuntu 20.04 container. Regression test asserts the SSH create path issues fewer than 5 ref probes instead of 100; reverting the break makes it fail with "expected 100".
… message The runtime path passes no subject and the IPC paths pass 'worktree name' or 'branch name'; the new message hardcoded 'branch name' for all three.
05da25a to
90cac3a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/orca-runtime.ts (1)
24447-24481: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
'worktree name'toformatBranchConflictMessage.Without a subject,
local,remote, andlocal-directoryconflicts omit the guidance to choose a different worktree name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7443eb42-4e33-451e-a858-881201bd13c6
📒 Files selected for processing (1)
src/main/runtime/orca-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
Rebased onto latest The earlier red was the |
Summary
STA-4746 is invalid — on current
mainand on the reported v1.4.184. This PR contains no production change, only the regression coverage that proves it and guards the contract.The report claimed that in a paired/remote setup a folder workspace (
ORCA_WORKTREE_ID=folder:<uuid>) makes the relay spawn its PTY in a fallback cwd, because "relay cwd resolution only consultsORCA_WORKTREE_ID, parses it asrepoId::path, and never falls back toORCA_WORKSPACE_ROOT."That is not how the relay resolves cwd.
PtyHandler.spawnAfterAdmissiontakes cwd from the spawn request (params.cwd);splitWorktreeId(worktreeId)feeds only thebeginPtyCreationadmission key. The client side already resolvesfolder:<uuid>throughresolveTerminalStartupCwdForWorkspace→store.getFolderWorkspace(id).folderPath.What actually produces the reported symptom
On every POSIX relay branch the host's login startup chain runs after the PTY is placed, so a remote startup file that
cds wins after the shell already started in the workspace root. Two mechanisms get there:POSIX_LOGIN_ARGS = ['-l'](src/relay/pty-shell-launch.ts:12), so/etc/profile,/etc/profile.d/*and~/.zprofileare sourced.--rcfile, not-l), but the relay's own wrapper explicitly sources/etc/profile, then~/.bash_profile/~/.bash_login/~/.profile(src/relay/pty-shell-overlay-wrappers.ts:63-70).That is exactly the reporter's own evidence:
OLDPWDis only ever set bycd. Phase B of the SSH spec reproduces that byte-shape by adding one/etc/profile.dentry to the container. Phase C shows the samecdmoves a plainrepoId::/pathgit worktree identically, so this mechanism is not folder-specific. That establishes a non-folder-specific cause for the reported signature; combined with the topology table below finding no folder-specific defect, it is why this closes as invalid.Evidence (all on clean
main@ 1b5cc00)folderPath; filesystem marker written intofolderPath; git-worktree + host-local controls greenorca servepaired hostterminal.create(folder:<uuid>)→pwd == folderPath~/.orca-remote/relay-0.1.0+…)$PWDand remote/proc/<pid>/cwdboth == folder pathVersion oracle
The cwd-resolution logic is behaviourally identical at
v1.4.184:src/shared/terminal-startup-cwd.tsandsrc/shared/workspace-scope.tsdiffer only by import-path moves (shared/worktree-id→shared/worktree/id,shared/types→shared/folder-workspace-types), andsrc/relay/pty-handler.tsstill takes cwd fromparams.cwd. Those files carry unrelated changes since the tag — the equivalence claim is about the cwd path, not the whole diff. v1.4.184 also did not sendworktreeIdas a spawn param at all (added later inssh-pty-spawn-request.tsfor shell-history isolation), so the relay could not have derived cwd from it there either. Nothing was "already fixed".Separately, on node-pty's POSIX implementation a nonexistent cwd makes the child exit 1 rather than silently inheriting the parent's cwd (
node_modules/node-pty/src/unix/pty.cc), ruling out the "fell back to the container's CMD directory" mechanism as well. Windows uses a different spawn path and was not measured.Why the proposed fix is not implemented
The proposed
ORCA_WORKSPACE_ROOTfallback would change nothing for the reporter — cwd is already correct at spawn — and it would make the relay adopt an unvalidated client-supplied path for a request that named no working directory.cwdis optional on the wire (PtySpawnOptions.cwd,src/main/providers/pty-provider-contract.ts:42), and the relay's default-cwd branch exists for requests that genuinely name none. But every relay-bound caller for a folder workspace resolves a path first viaresolveTerminalStartupCwdForWorkspace; the one in-repo caller that can omitcwd(codex-detached-pane-restart.ts:210) is local-only and never reaches the relay. So the fallback has no relay caller to serve, and promoting an env var into it would only widen what the relay trusts.Coverage added
src/relay/pty-handler-folder-workspace-cwd.test.ts— 5 deterministic cases: folder id honours request cwd, folder id with noORCA_WORKSPACE_ROOT, worktree-id precedence unchanged, legacy/mixed-version client (env-onlyORCA_WORKTREE_ID), and the default-cwd fallback pinned to not adoptORCA_WORKSPACE_ROOT.tests/e2e/sta4746-folder-remote-cwd.spec.ts— headless pairedorca servehost.tests/e2e/sta4746-paired-desktop-folder-cwd.spec.ts— headed paired desktop server + client (@headful), with git-worktree and local-only controls.tests/e2e/sta4746-ssh-folder-cwd.spec.ts— real Docker SSH relay, 3 phases (clean / login-profile cd / git-worktree control), gated onORCA_E2E_SSH_DOCKER=1like the sibling docker specs.Guard is non-vacuous: injecting the alleged id-derived cwd into
pty-handler.tsturns 4 of the 5 relay cases red; reverting the injection returns 5/5 green.Commands
Gaps
CI runs all three specs on Linux runners (the changed-e2e job picked them up and all 3 passed), and the SSH phases were additionally exercised against a Linux container from a macOS host. Windows and WSL hosts were not exercised.
Closes STA-4746 as invalid.