Repository navigation
fix(github): fail closed instead of running client git against a remote repoPath when the SSH provider is unregistered - #14945
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change updates PR branch resolution to resolve SSH Git providers before executing Git operations. Missing SSH providers now return no head or an empty upstream result without using local Git. WSL repositories continue to use the local Git runtime with configured options. New tests cover provider routing, command arguments, fallback suppression, GitHub lookup counts, and current-branch or upstream PR results. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
getCurrentHeadOid and probeTrackedUpstreamBranches only routed through the SSH provider when one was registered. With connectionId set but the provider unregistered (dropped connection, not yet reattached) they fell through to client-side git with cwd pointing at the remote repoPath — on a machine with a same-named local path that silently answers for the wrong repository. getCurrentHeadOid feeds shouldHideMergedImplicitPR, so a wrong OID changes which PR the UI attributes to a worktree. Both now take their existing unknown path (null / probeFailed) instead, matching repo-default-branch.ts. Local and WSL routing is unchanged.
8d715ab to
817ae27
Compare
|
Review verdict: code-approved after fixes at The review found and fixed several execution-boundary issues: missing SSH providers could fall back to client Git, transient SSH probe/auth failures could collapse into Observed validation on the exact pushed head:
Electron QA used the exact worktree build through Playwright CDP. The child dispatch path was consumer-fenced, so I captured the evidence directly. The exact build rendered successfully, but initiating the real SSH connection made the CDP endpoint stop responding; therefore the post-connect/disconnect UI refresh leg is unverifiable, not passed. No Windows/WSL UI leg was observed. |
…te repoPath when the SSH provider is unregistered (stablyai#14945) * fix(github): fail closed when the SSH git provider is gone getCurrentHeadOid and probeTrackedUpstreamBranches only routed through the SSH provider when one was registered. With connectionId set but the provider unregistered (dropped connection, not yet reattached) they fell through to client-side git with cwd pointing at the remote repoPath — on a machine with a same-named local path that silently answers for the wrong repository. getCurrentHeadOid feeds shouldHideMergedImplicitPR, so a wrong OID changes which PR the UI attributes to a worktree. Both now take their existing unknown path (null / probeFailed) instead, matching repo-default-branch.ts. Local and WSL routing is unchanged. * fix(github): preserve PR state when SSH probes fail * fix(github): keep failed SSH discovery unverifiable * fix(github): propagate SSH identity failures * fix(github): scope verified SSH identity probes * test(github): preserve tolerant resolver calls * fix(github): preserve indeterminate auth discovery * fix(github): isolate SSH repository probe generations * test(github): expose SSH probe generation in mocks
…te repoPath when the SSH provider is unregistered (stablyai#14945) * fix(github): fail closed when the SSH git provider is gone getCurrentHeadOid and probeTrackedUpstreamBranches only routed through the SSH provider when one was registered. With connectionId set but the provider unregistered (dropped connection, not yet reattached) they fell through to client-side git with cwd pointing at the remote repoPath — on a machine with a same-named local path that silently answers for the wrong repository. getCurrentHeadOid feeds shouldHideMergedImplicitPR, so a wrong OID changes which PR the UI attributes to a worktree. Both now take their existing unknown path (null / probeFailed) instead, matching repo-default-branch.ts. Local and WSL routing is unchanged. * fix(github): preserve PR state when SSH probes fail * fix(github): keep failed SSH discovery unverifiable * fix(github): propagate SSH identity failures * fix(github): scope verified SSH identity probes * test(github): preserve tolerant resolver calls * fix(github): preserve indeterminate auth discovery * fix(github): isolate SSH repository probe generations * test(github): expose SSH probe generation in mocks

The rule
For an SSH-hosted repo, an operation on a remote
repoPathmust never fall back to running on the client machine. "We could not ask" has to fail closed, not answer. The codebase already states and enforces this:src/main/source-control/repo-default-branch.ts:76-78— "a dropped SSH provider must not fall back to local git — the repoPath is remote, so a local run could answer for the wrong repo"src/main/repo-worktrees.ts:45-48src/main/runtime/orca-runtime.ts— "fail closed to unknown instead of probing a server path on the desktop filesystem"src/renderer/src/lib/connection-context.ts:22-24src/main/runtime/orca-runtime-git.tsenforces it ~33 times by throwing whenconnectionIdis set but the provider is missinggetSshGitProviderreturnsundefined(it does not throw) when a connection is unregistered, so the boundary has to be checked explicitly.The two sites
Both in
src/main/github/client.ts, same shape:getCurrentHeadOid(rev-parse HEAD)probeTrackedUpstreamBranches(for-each-ref refs/heads)When
connectionIdis set but the provider is unregistered — connection dropped, or not yet reattached — the ternary silently selected the local branch and ran client git withcwdset to a remote path. On a machine where a same-named path exists locally, it returns the wrong repository's answer instead of failing.getCurrentHeadOidfeedsshouldHideMergedImplicitPR, so a wrong OID silently changes which PR the UI attributes to a worktree.The fix
Both functions already have a natural "unknown" return, so no new signalling was needed:
getCurrentHeadOid→nullprobeTrackedUpstreamBranches→{ probeFailed: true, upstreamsByBranchName: new Map() }The guard is lifted above the
tryand mirrorsrepo-default-branch.tsexactly.probeFailed: truealso keeps the empty snapshot out of the tracked-upstream cache, so the probe re-runs once the provider reattaches rather than pinning an empty result for the TTL.Local and WSL behavior is unchanged. The new branch is reached only when
connectionIdis truthy;wslDistrostill routes throughgitExecFileAsyncwith{ cwd, wslDistro }exactly as before, and both WSL paths are pinned by new tests.Red-first evidence
New suite:
src/main/github/client-ssh-provider-execution-boundary.test.ts(6 tests), built on the existingclient-test-mocks/client-test-harnessrig used byclient-merged-pr-visibility.test.tsandclient-tracked-upstream-snapshot.test.ts. It follows the precedent set byclient-pr-local-runtime.test.ts("never falls through to the default gh host for an unresolved SSH repository"), asserting that the local exec mock is not called.Verified red by reverting
client.tstoHEADand running the new suite unchanged:The failure output is the bug: local git invoked with
cwd: /remote/repo. With the fix restored, 6/6 pass.The 4 tests that pass in both runs are deliberate non-regression controls — they pin that a registered provider still gets
provider.exec, and that a WSL repo still getsgitExecFileAsync(..., { cwd, wslDistro })— so the guard cannot be over-broad.Gates
vitest run --config config/vitest.config.ts src/main/github/— 61 files, 655 tests, all passoxlinton both changed files — clean (verified the gate is live by linting a probe file with an injectedno-unused-vars+no-debugger, which reported both)tsc --noEmit -p config/tsconfig.node.json— exit 0 (src/main/**/*covers the new test file)oxfmt(no prettier); commit passed lint-staged incl. the react-doctor passScope
Two call sites only. No behavior change for local or WSL repos, and no change to what the SSH path does when a provider is registered.