Repository navigation
fix(github): scope GHES host-auth cache to the executing connection - #14948
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: 📥 CommitsReviewing files that changed from the base of the PR and between b6d5972 and b0e557d192edcb78538f4b0a5d34c65acc45db69. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change scopes GitHub authentication cache keys by SSH connection ID and provider generation. Local and WSL repositories retain their existing runtime-based keys. Origin repository caches re-probe after provider-generation changes. Tests cover separate connections, reconnects, stale in-flight probes, and local versus connection-backed repositories at the same path. 🚥 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 |
3a3ba51 to
a901bd9
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. |
a901bd9 to
7c258fa
Compare
|
SSH QA evidence at |
7c258fa to
b0e557d
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: DO NOT LAND yetThe code review is clean and all requested fixes are pushed at Meaningful fixes pushed
Review and verification
SSH/Electron QAI launched the exact PR head in an isolated Electron profile and verified The QA leg is explicitly blocked, not passed: on both targets Setup evidence only (the target is visibly Disconnected, matching the blocked result): Remote repos remained clean with only the main worktree, and both containers, relay processes, profiles, keys, and temporary files were removed. Once a controlled GHES-over-SSH reconnect run observes the cache refresh, this can move to a land verdict. |
b0e557d to
fe99ca4
Compare
The gh auth answer cached for a connection-backed repo is probed without the repository cwd, so it describes that connection's runtime — not the local host. Keying only on repoPath+wslDistro let a local repo and an SSH-hosted repo at the same path share one entry, and whichever resolved first decided "is this GHES host authenticated" for both. Include the connection identity in the runtime cache key. Local and WSL keys are unchanged.
fe99ca4 to
445be9a
Compare
Verdict update: LAND — supersedes the DO NOT LAND aboveThe earlier verdict was correct when written, but it described head Why DO NOT LAND no longer appliesThat verdict withheld approval for exactly one reason: the SSH/GHES manual QA leg was unobserved, because That was not a product defect. The Docker daemon on the review machine was wedged — it accepted socket connections and never answered — which starved every container-backed QA. Three separate PRs in this batch hit the identical symptom at the identical stage. Docker has since been restarted and verified healthy (server With a working daemon, the leg was re-run and passed: the Docker Ubuntu SSH E2E reached Connected state and completed a real reconnect, leaving zero stray containers. What was verified at
|
…tablyai#14948) * fix(github): scope GHES host-auth cache to the executing connection The gh auth answer cached for a connection-backed repo is probed without the repository cwd, so it describes that connection's runtime — not the local host. Keying only on repoPath+wslDistro let a local repo and an SSH-hosted repo at the same path share one entry, and whichever resolved first decided "is this GHES host authenticated" for both. Include the connection identity in the runtime cache key. Local and WSL keys are unchanged. * fix(github): fence GHES auth cache across SSH reconnects * fix(github): fence origin cache across SSH reconnects * fix(github): fence repository identity cache on reconnect
…tablyai#14948) * fix(github): scope GHES host-auth cache to the executing connection The gh auth answer cached for a connection-backed repo is probed without the repository cwd, so it describes that connection's runtime — not the local host. Keying only on repoPath+wslDistro let a local repo and an SSH-hosted repo at the same path share one entry, and whichever resolved first decided "is this GHES host authenticated" for both. Include the connection identity in the runtime cache key. Local and WSL keys are unchanged. * fix(github): fence GHES auth cache across SSH reconnects * fix(github): fence origin cache across SSH reconnects * fix(github): fence repository identity cache on reconnect
…tablyai#14948) * fix(github): scope GHES host-auth cache to the executing connection The gh auth answer cached for a connection-backed repo is probed without the repository cwd, so it describes that connection's runtime — not the local host. Keying only on repoPath+wslDistro let a local repo and an SSH-hosted repo at the same path share one entry, and whichever resolved first decided "is this GHES host authenticated" for both. Include the connection identity in the runtime cache key. Local and WSL keys are unchanged. * fix(github): fence GHES auth cache across SSH reconnects * fix(github): fence origin cache across SSH reconnects * fix(github): fence repository identity cache on reconnect


The rule
AGENTS.md(Git Binary Compatibility): capability state must be scoped to the host that executes — native, WSL distro, SSH provider, or relay connection. State derived on the client must not be attributed to a remote host, and vice versa.The site
src/main/github/github-enterprise-repository.ts:39-44built the GHES host-auth cache key fromruntimeCacheKey(repoPath, wslDistro)and omittedconnectionId:The answer it caches is
gh auth status, resolved from the client's gh (resolveAuthenticatedGitHubHost, ~L143-160). Two lookups at the samerepoPath— one local, one connection-backed, or two different SSH connections — collapse onto one entry. Whichever resolves first decides "is this GHES host authenticated" for all of them.That is not merely a wasted-spawn issue: the two probes genuinely differ.
ghRepoExecOptionsreturns{ cwd: repoPath }for a local repo and{}for a connection-backed one, so the cached string describes a different gh invocation than the one the second caller would have made. The laptop's answer gets attributed to the remote host.The fix
Include the connection identity in the runtime cache key, and scope the entry to the host it actually describes:
Not in scope:
ghstill executes on the native host on purpose (the design intent recorded atgithub-enterprise-repository.ts:39-41). This PR does not move gh execution — only the cache key changes.Red-first evidence
The three new tests were committed first and run against unmodified source. All three fail:
With the fix applied:
The assertions are on the answer, not just the spawn count — each test makes the two probes return different auth inventories (distinguished by whether gh got a
cwd), so a shared entry produces a demonstrably wrongtrue/false, not merely a saved subprocess. The(local resolves first)/(connection resolves first)pair proves the contamination runs in both directions.One existing test was inverted
shares the native gh auth probe across SSH connectionsasserted exactly the defective behavior (toHaveBeenCalledTimes(1)forssh-1thenssh-2). It is replaced bydoes not share a cached answer between SSH connections at the same path, which asserts the connections get independent answers. Flagging it explicitly since it is a deliberate behavior change, not incidental churn.Reconnect fencing
The connection key also includes the SSH provider generation. A reconnect cannot reuse a cached answer or join an in-flight probe started by the replaced provider; two focused tests cover both cases. Repeated calls within one generation still reuse the bounded cache, so reconnect adds at most one fresh auth probe per host.
Local / WSL behavior is unchanged
When
connectionIdis falsy the function returns the identicallocal:${distro ?? 'host'}key as before — same implicit-WSL-UNC derivation viaparseWslPath, sametoLowerCase(). The pre-existing coverage for that path is untouched and still green:caches per runtime+host so detection polling does not re-spawn ghdoes not share cache state across WSL distrosderives cache scope from an implicit WSL UNC repository pathprobes gh in the repository WSL runtime, not the host/default distroOne intentional consequence worth naming:
isGitHubHostAuthenticatedForGlobalClipasses the sentinelconnectionId: 'project-host-validation'(to suppresscwd), so it now gets its own bucket instead of sharinglocal:host. That is the same correctness property — it is a genuinely different gh invocation — at the cost of at most one extragh auth statusper 60s TTL.Gates
vitest src/main/github src/main/source-controltsc --noEmit -p config/tsconfig.node.jsonoxlint(changed files)check-changed-code-quality.mjscheck-max-lines-ratchet.mjscheck-reliability-gates.mjsoxfmtRebased onto
origin/mainat96b03bab4fand re-verified green there.🤖 Generated with Claude Code