feat(pull-requests): resolve ssh config host aliases on git remotes - #6196
feat(pull-requests): resolve ssh config host aliases on git remotes#6196nyedle wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
size:L
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new sshRemoteUrl helper and its consumers (GitManager, GitVcsDriver, RepositoryIdentityResolver) against the Effect service conventions. Layer/dependency wiring follows the repo pattern (yield* ProcessRunner.ProcessRunner in make, Layer.provide(ProcessRunner.layer) on the exported layer), and the SshConfigProbe callback is a legitimate strategy since GitVcsDriver supplies a cwd-scoped VcsProcess variant. One finding: process-wide mutable cache state kept outside the Effect environment.
Posted via Macroscope — Effect Service Conventions
| const HOSTNAME_TTL_MS = 5 * 60_000; | ||
| const effectiveHostnames = new Map<string, { readonly at: number; readonly hostname: string }>(); | ||
|
|
||
| export const canonicalizeSshRemoteUrl = Effect.fnUntraced(function* ( | ||
| remoteUrl: string, | ||
| probe: SshConfigProbe, | ||
| ) { | ||
| const host = hostPattern(remoteUrl).exec(remoteUrl)?.[2]; | ||
| if (host === undefined) return remoteUrl; | ||
|
|
||
| const now = Date.now(); | ||
| const cached = effectiveHostnames.get(host); | ||
| const hostname = | ||
| (cached !== undefined && now - cached.at < HOSTNAME_TTL_MS ? cached.hostname : undefined) ?? | ||
| /^hostname[ \t]+(\S+)/imu.exec(yield* probe(host))?.[1] ?? | ||
| host; | ||
| effectiveHostnames.set(host, { at: now, hostname }); |
There was a problem hiding this comment.
This TTL cache is process-global mutable state living outside the Effect environment, and it reads time with Date.now() instead of the Effect clock. That has a few consequences worth avoiding: entries survive layer/runtime lifetimes and leak between tests (the "probes each host once" case depends on that global), TTL expiry can't be driven with TestClock, and because the key is only the host, a hostname resolved by GitVcsDriver's cwd-scoped ssh -G probe is reused for other cwds and for the ProcessRunner-backed probe, which may resolve differently.
Consider owning the memoization in the Effect environment instead — e.g. build it inside each make with Effect.cachedFunction/Cache.makeWith over the probe (which gives you clock-based TTL for free) and pass the memoized probe in, or expose a small ProcessRunner-backed service that owns the cache and keys it by probe context.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Reviewed the new sshRemoteUrl helper and its consumers (GitManager, GitVcsDriver, RepositoryIdentityResolver) against the Effect service conventions. Layer/dependency wiring follows the repo pattern (yield* ProcessRunner.ProcessRunner in make, Layer.provide(ProcessRunner.layer) on the exported layer), and the SshConfigProbe callback is a legitimate strategy since GitVcsDriver supplies a cwd-scoped VcsProcess variant. One finding: process-wide mutable cache state kept outside the Effect environment.
Posted via Macroscope — Effect Service Conventions
| const HOSTNAME_TTL_MS = 5 * 60_000; | ||
| const effectiveHostnames = new Map<string, { readonly at: number; readonly hostname: string }>(); | ||
|
|
||
| export const canonicalizeSshRemoteUrl = Effect.fnUntraced(function* ( | ||
| remoteUrl: string, | ||
| probe: SshConfigProbe, | ||
| ) { | ||
| const host = hostPattern(remoteUrl).exec(remoteUrl)?.[2]; | ||
| if (host === undefined) return remoteUrl; | ||
|
|
||
| const now = Date.now(); | ||
| const cached = effectiveHostnames.get(host); | ||
| const hostname = | ||
| (cached !== undefined && now - cached.at < HOSTNAME_TTL_MS ? cached.hostname : undefined) ?? | ||
| /^hostname[ \t]+(\S+)/imu.exec(yield* probe(host))?.[1] ?? | ||
| host; | ||
| effectiveHostnames.set(host, { at: now, hostname }); |
There was a problem hiding this comment.
This TTL cache is process-global mutable state living outside the Effect environment, and it reads time with Date.now() instead of the Effect clock. That has a few consequences worth avoiding: entries survive layer/runtime lifetimes and leak between tests (the "probes each host once" case depends on that global), TTL expiry can't be driven with TestClock, and because the key is only the host, a hostname resolved by GitVcsDriver's cwd-scoped ssh -G probe is reused for other cwds and for the ProcessRunner-backed probe, which may resolve differently.
Consider owning the memoization in the Effect environment instead — e.g. build it inside each make with Effect.cachedFunction/Cache.makeWith over the probe (which gives you clock-based TTL for free) and pass the memoized probe in, or expose a small ProcessRunner-backed service that owns the cache and keys it by probe context.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Needs human review This PR introduces new SSH config alias resolution functionality affecting multiple remote URL parsing paths. The architectural concerns about global mutable cache state and unresolved issues with certain SSH config patterns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32708d8c4f
ℹ️ 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".
| return hostname === host | ||
| ? remoteUrl | ||
| : remoteUrl.replace(hostPattern(remoteUrl), (_, prefix: string) => prefix + hostname); |
There was a problem hiding this comment.
When the remote relies on ~/.ssh/config for the user, e.g. Host main with HostName github.com and User git, Git remotes are commonly stored as main:owner/repo.git. This replacement preserves the empty prefix and returns github.com:owner/repo.git; the downstream source-control and repository parsers only recognize URL forms or git@... scp-style remotes, so these aliases still resolve as unknown/local-looking remotes and PR discovery remains broken for exactly that SSH-config workflow.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
| export function environmentQueryError<A, E>(result: AsyncResult.AsyncResult<A, E>): string | null { | ||
| if (result._tag !== "Failure" || Cause.hasInterruptsOnly(result.cause)) return null; |
There was a problem hiding this comment.
This fixes interrupted AsyncResult reads only in the web copy of useEnvironmentQuery, while apps/mobile/src/state/query.ts still formats every Failure as an error. Any mobile screen that refreshes overlapping environment atoms can therefore continue showing the same spurious “environment request failed” state that this patch removes from web/desktop; move this logic to shared client code or update the mobile hook as well.
AGENTS.md reference: AGENTS.md:L65-L75
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 585cf49. Configure here.
|
Note 🤖 GPT-6 Astra (preview) responding on behalf of Theo This was closed as part of an automated cleanup pass. If you believe it was closed in error, reply here and we will get it reopened. Closing in favor of #7186. Its approach keeps the raw Git remote for SSH authentication and resolves a separate URL for provider detection, instead of rewriting stored remote locators. Review continues there. The direct GitManager lookup paths and SSH alias cases from this branch are recorded on that PR and still need coverage. |
2 participants
What Changed
Git remote URLs now go through
ssh -G <host>wherever the server reads one,so an
~/.ssh/configalias is understood as the host it points at.useEnvironmentQueryno longer treats an interrupted read as a failed one.Why
If you use a per account SSH key, your remote reads
git@main:owner/repo. Thehost parses as
main, the provider resolves tounknown, and the repo neverlists its pull requests.
ssh -Gdoes the resolving, so no config parser. Non ssh remotes, local pathsand a missing
sshare left alone.Second fix, same page: refreshing re-arms five atoms at once and interrupts
whatever read is in flight. An interrupt arrives as a Failure, so the page
showed "the latest request failed" over rows that were fine. Retry always
worked because it refreshes one atom and interrupts nothing.
Note
Alias users will see the host switcher say GitHub instead of the alias name,
and two clones of one repo under different aliases now group as one.
UI Changes
None.
Checklist
Note
Medium Risk
Changes how git remotes are normalized for provider detection and repository identity, which can regroup clones and alter cached remote keys for SSH-alias users. Also introduces
ssh -Gprocess probes on remote reads.Overview
Resolves
~/.ssh/confighost aliases before provider detection and repository identity, so remotes likegit@main:owner/repoare treated as their real host (e.g. GitHub) and PRs can load.Adds
canonicalizeSshRemoteUrl, which probesssh -G(with a TTL hostname cache) and rewrites SCP/ssh://remotes. Wired intoGitManagerremote reads,GitVcsDriver.listRemotes, andRepositoryIdentityResolver. Non-SSH URLs, local paths, and failed probes are left unchanged.Also fixes
useEnvironmentQueryso interrupt-only failures during refresh no longer surface as request errors.Reviewed by Cursor Bugbot for commit 0fed439. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Resolve SSH config host aliases on git remote URLs before provider and repository identity resolution
canonicalizeSshRemoteUrlwhich probesssh -Gto resolve SSH config aliases, bracket IPv6 hosts, and return a canonical hostname for any SSH or SCP remote URL.GitVcsDriver.listRemotes,GitManager, andRepositoryIdentityResolvernow canonicalize remote URLs before hosting provider detection, repository context resolution, and identity construction.SshHostnameCachewith TTL is used to avoid redundantssh -Ginvocations per host.Macroscope summarized 0fed439.