Detect pull requests against upstream repositories#3907
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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:M
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
|
|
||
| const origin = candidates.find((candidate) => candidate.remoteName === "origin"); | ||
| const conventionalGitHubUpstream = candidates.find( |
There was a problem hiding this comment.
🟡 Medium sourceControl/SourceControlProviderRegistry.ts:145
conventionalGitHubUpstream overrides origin whenever an upstream remote and origin are both GitHub repositories, even when upstream is an unrelated GitHub repo (e.g. origin=acme/app, upstream=acme/shared-template). The returned context points at upstream, so list/create/checkout/resolve operations target acme/shared-template instead of the checked-out origin repository. This heuristic only verifies the remote name and provider kind, not that upstream is actually the fork parent of origin. If this convention is intentional, consider documenting the rationale; otherwise the upstream heuristic should only apply when upstream is demonstrably the parent of origin (or at minimum not preempt origin).
Also found in 1 other location(s)
apps/server/src/git/GitManager.ts:874
targetRepositoryalways prefers any parseable remote namedupstream, without checking that it is related toorigin. If a checkout tracksoriginand also has an unrelated GitHubupstreamremote,remoteRepository(the origin) is compared against that unrelated repository, soisCrossRepositorybecomes true. This changes PR lookup to fork-qualified/cross-repository matching (and, with the accompanying provider targeting, queries the unrelated repository), causing valid same-repository PR metadata fromoriginto be missed. This directly contradicts the PR's stated requirement to keep unrelatedupstreamremotes from overriding a valid GitHuborigin.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/sourceControl/SourceControlProviderRegistry.ts around line 145:
`conventionalGitHubUpstream` overrides `origin` whenever an `upstream` remote and `origin` are both GitHub repositories, even when `upstream` is an unrelated GitHub repo (e.g. `origin=acme/app`, `upstream=acme/shared-template`). The returned context points at `upstream`, so list/create/checkout/resolve operations target `acme/shared-template` instead of the checked-out `origin` repository. This heuristic only verifies the remote name and provider kind, not that `upstream` is actually the fork parent of `origin`. If this convention is intentional, consider documenting the rationale; otherwise the `upstream` heuristic should only apply when `upstream` is demonstrably the parent of `origin` (or at minimum not preempt `origin`).
Also found in 1 other location(s):
- apps/server/src/git/GitManager.ts:874 -- `targetRepository` always prefers any parseable remote named `upstream`, without checking that it is related to `origin`. If a checkout tracks `origin` and also has an unrelated GitHub `upstream` remote, `remoteRepository` (the origin) is compared against that unrelated repository, so `isCrossRepository` becomes true. This changes PR lookup to fork-qualified/cross-repository matching (and, with the accompanying provider targeting, queries the unrelated repository), causing valid same-repository PR metadata from `origin` to be missed. This directly contradicts the PR's stated requirement to keep unrelated `upstream` remotes from overriding a valid GitHub `origin`.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR changes runtime behavior by preferring 'upstream' over 'origin' remotes for PR operations. An unresolved review comment identifies that the heuristic doesn't verify upstream is actually the fork parent, which could cause operations to target unrelated repositories. 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: 5e512ba2d4
ℹ️ 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".
| const conventionalGitHubUpstream = candidates.find( | ||
| (candidate) => | ||
| candidate.remoteName === "upstream" && | ||
| candidate.provider.kind === "github" && | ||
| origin?.provider.kind === "github", |
There was a problem hiding this comment.
When a checkout has a valid GitHub origin plus a stale/reference upstream that is also GitHub but not the fork base, this condition still selects upstream solely by provider kind. bindProviderContext then sends that context into GitHub operations, and GitHubSourceControlProvider.repositoryFromContext turns it into --repo <upstream>, so status/default-branch/PR create/checkout are run against the wrong repo even though origin was valid. The guard needs to prove the upstream is the conventional base, or fall back to origin, rather than just matching kind === "github".
Useful? React with 👍 / 👎.
| context?.remoteName === "upstream" | ||
| ? (parseGitHubRepositoryNameWithOwnerFromRemoteUrl(context.remoteUrl) ?? undefined) |
There was a problem hiding this comment.
For a GitHub Enterprise fork where the remotes are on a host like github.example.test, the registry still binds the provider to the upstream context, but this parser returns undefined because the shared helper only recognizes github.com URL forms. The new withRepositoryFromContext path then omits repository, so the gh pr ... calls fall back to the local checkout’s origin and continue querying/creating PRs in the fork instead of the upstream repo; build a host-qualified [HOST/]OWNER/REPO value from the context for self-hosted GitHub (the gh manual documents that --repo form: https://cli.github.com/manual/gh_pr).
Useful? React with 👍 / 👎.
1 participant
Summary
upstreamGitHub remoteowner:branchand target GitHub CLI operations at the upstream repositoryupstreamfrom overriding a valid GitHuboriginRoot cause
PR discovery derived the fork-qualified head selector, but GitHub CLI still resolved the repository from the local checkout's
origin. In a fork checkout, that queried the fork rather than the upstream repository where the pull request exists.Impact
Branches pushed to a fork now show their upstream pull request metadata in T3 Code. Related PR operations consistently use the upstream repository context.
Validation
vp checkvp run typecheckNote
Medium Risk
Changes core PR status and GitHub CLI routing for fork workflows; behavior is scoped to upstream context and covered by new tests, but mistakes could mis-associate PRs or hit the wrong repo.
Overview
Fork checkouts can now surface pull requests opened against the upstream repo instead of only querying the fork via
origin.GitHub CLI gains an optional
repositoryargument on PR/repo commands; when set,ghgets--repo owner/nameso lookups run against the correct repository.Provider registry prefers a conventional GitHub
upstreamremote (with GitHuborigin) overoriginalone, and the GitHub provider threads that repo into list/get/create/checkout/default-branch calls when the active context isupstream.Git manager treats cross-repo PR detection relative to the
upstreamremote’s repo when it exists (not onlyorigin), keepingowner:branchhead selectors aligned with fork workflows.Regression tests cover fork/upstream status, CLI args, provider listing, and registry resolution; unrelated
upstreamremotes (e.g. Azure DevOps) still do not override GitHuborigin.Reviewed by Cursor Bugbot for commit 5e512ba. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Detect pull requests against upstream repositories in conventional GitHub fork setups
GitHubSourceControlProvidernow detects when the Git context references anupstreamremote with a GitHub URL, and passes--repo <owner/name>to allghcommands (list, view, create, checkout, default branch).selectProviderContextinSourceControlProviderRegistry.tsprefers theupstreamremote overoriginwhen both are GitHub remotes.GitManagerresolves the upstream repository context and uses it for cross-repository (fork) detection when available.GitHubClimethod signatures are extended with an optionalrepositoryparameter, inserted as--repoor a positional arg where appropriate.origin, canonicalupstream) will now be listed and created against the upstream repository rather than origin.📊 Macroscope summarized 5e512ba. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.