Repository navigation
fix(repos): never probe the client filesystem for a remote repo icon - #14947
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 4 remain after this review. 📝 WalkthroughWalkthroughRepository icon detection now distinguishes connected repositories from local repositories. Connected repositories use a filesystem provider for file and package icon probing. Connected repositories without a provider return no icon instead of probing the local filesystem. Repositories without a connection continue local icon detection. Tests cover filesystem boundaries, provider-based detection, concurrency behavior, and local favicon detection. 🚥 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 |
detectRepoFileIcon fell back to a LOCAL read whenever the SSH filesystem provider was absent, so a disconnected/not-yet-reattached remote repo whose path also exists on the client picked up the wrong repository's icon. Thread the connection identity through and fail closed, matching the rule already stated in connection-context.ts and repo-default-branch.ts.
6845b02 to
64128ad
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Review verdict: LANDNo P0, P1, or P2 findings. The four-file patch is focused and matches the SSH execution-boundary rule: a remote Observed verification:
The connected/disconnected Playwright-CDP screenshots already attached to the PR were downloaded and visually inspected and are valid evidence for commit Residual risk is low. Previously persisted incorrect file icons are not migrated, which is appropriate scope because they cannot be safely distinguished from legitimate remote file icons. |
…tablyai#14947) detectRepoFileIcon fell back to a LOCAL read whenever the SSH filesystem provider was absent, so a disconnected/not-yet-reattached remote repo whose path also exists on the client picked up the wrong repository's icon. Thread the connection identity through and fail closed, matching the rule already stated in connection-context.ts and repo-default-branch.ts.
…tablyai#14947) detectRepoFileIcon fell back to a LOCAL read whenever the SSH filesystem provider was absent, so a disconnected/not-yet-reattached remote repo whose path also exists on the client picked up the wrong repository's icon. Thread the connection identity through and fail closed, matching the rule already stated in connection-context.ts and repo-default-branch.ts.
The rule
An operation on a remote
repoPathmust never fall back to the client machine's filesystem. "We could not ask the host" has to fail closed. Already stated in:src/renderer/src/lib/connection-context.ts:22-24— "Callers must not fall back to a LOCAL read of a remote path"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/runtime/orca-runtime.ts:24242-24246The site
src/main/repo-icon-file-detection.ts:265(pre-change):reached from
src/main/repo-icon-autodetect.ts:105-113. The ternary keys off provider presence, not repo location. When the repo is SSH-hosted butgetSshFilesystemProvider(connectionId)returnsundefined— connection dropped, or not yet reattached after a restore — this stats and reads the client filesystem at a remote absolute path. On a machine where a same-named path exists locally (very common:~/code/<repo>on both sides), the sidebar shows the wrong repository's icon, silently and persistently, since the detected icon is stored on the repo record at add time.The identical fallback sat five lines below in the package-homepage probe (
detectLocalPackageHomepageIcon), so it is closed by the same gate.The fix
connectionIdwas already resolved indetectRepoIcon— it just was not passed down. It is now threaded intodetectRepoFileIcon, which distinguishes the three cases explicitly:connectionIdnull/undefined)detectRepoIcongates both filesystem probes on the same condition. The GitHub-avatar fallback is deliberately untouched: it routes through the connection-aware git transport (getRepoSlug(repoPath, connectionId)), not the client filesystem, so a missing filesystem provider must not suppress it.Remoteness is derived from the threaded connection identity only — never inferred from the path string.
Red-first evidence
Both new assertions were verified failing before the fix, by reverting source while keeping the tests.
RED-A — end-to-end, source reverted to
HEAD(git checkout HEAD -- src/main/repo-icon-autodetect.ts). A real temp repo with afavicon.png,connectionId: 'ssh-target-not-connected', no provider registered:That received value is the defect: the local repo's icon returned for a remote repo.
RED-B — guard-only revert (new signature kept, only the
if (connectionId) return nullblock deleted), so the unit assertion cannot pass for the wrong reason:37 local
statcalls against the remote path. The unit test spies the realnode:fs/promisesentrypoints, so it asserts the absence of the read itself rather than only the return value.GREEN (fix restored):
Test Files 3 passed (3) · Tests 28 passed (28)acrossrepo-icon-file-detection.test.ts,repo-icon-autodetect.test.ts,repos-add-linked-worktree.test.ts.Local-repo behavior is unchanged
connectionIdis null/undefined for every local repo (the local add/clone/create call sites do not pass one), so the guard is never reached and the local probe runs exactly as before. Two tests pin this rather than leaving it to inspection:still detects local icons when the repo has no connection—connectionId: nullstill returns thefavicon.pngicon.still probes the local filesystem for a repo with no connection— assertsstatis called, so the guard cannot silently widen into local repos.All 14 pre-existing icon-detection tests pass untouched (only the
detectRepoFileIconcall sites were updated for the options-object argument).Gates
npx oxlinton the four changed files — clean (verified live: it flagged aconsistent-type-importsviolation on the first pass, which is fixed).oxfmtvia lint-staged; react-doctor config also ran on commit. No prettier.tsc --noEmit -p config/tsconfig.node.json— clean. Notsc -b.max-linesdisable added.Scope
Four files, one behavior change. No new files, no renames, no product surface touched.
Review QA evidence
Validated commit
64128add6ein the exact PR worktree with isolated Electron user data and Playwright CDP.pnpm build:relaypassed before connecting; app identity reportedOrca: brennanb2025/gate-14947.A disposable Ubuntu 22.04 Linux SSH target (
linux-arm64, Node 20.20.2) hosted a realdemo-projectcloned from a transferred Git bundle after direct HTTPS clone was blocked by authentication. Connectedrepos.addRemotediscovered the remotefavicon.png; the rendered sidebar showed the project on the SSH host, and SSH backing-state checks confirmed/work/demo-project, branchmain, its worktree, and only the deliberate icon fixture.After disconnecting, the rendered app showed
Connectand0 hosts; a realrepos.addRemotecall failed with the not-connected error and left repo state unchanged despite an identically pathed local repo/icon fixture.The product API rejects a disconnected remote add before icon detection, so the exact missing-filesystem-provider race cannot be driven through the user surface without bypassing the changed code. That branch is covered by the deterministic no-local-
stat/readFiletests and the end-to-end icon test. Renderer console had zero errors; relay logs showed successful handshake/streaming and clean disconnect. The app session, remote repo/target, container, relay, keys, profile, and temporary bundles were removed after verification.