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 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5df12cd1e
ℹ️ 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 targetRepository = | ||
| upstreamRepository.repositoryNameWithOwner !== null ? upstreamRepository : originRepository; |
There was a problem hiding this comment.
When origin is an organization-owned fork and a GitHub upstream exists, selecting upstreamRepository here makes isCrossRepository true; later preferredHeadSelector becomes org:branch and runPrStep sends it to gh pr create --head with the new --repo upstream context. I checked the GitHub CLI manual for gh pr create, which says --head supports <user>:<branch> but using an organization as that owner is unsupported, so Create PR now fails for org-owned forks even though user-owned forks work.
Useful? React with 👍 / 👎.
| ? remoteRepository.repositoryNameWithOwner.toLowerCase() !== | ||
| originRepository.repositoryNameWithOwner.toLowerCase() | ||
| targetRepository.repositoryNameWithOwner.toLowerCase() |
There was a problem hiding this comment.
When this comparison reclassifies a fork branch relative to upstream, PR creation is now aimed at the upstream repo, but runPrStep still computes the generated PR title/body via resolveBaseRangeRef(cwd, baseBranch), whose fallback remote is always the primary remote (origin whenever present). In a normal fork where origin/main is stale or missing while upstream/main is the actual base, the generated PR content is based on the wrong commit range and can include unrelated upstream commits; thread the selected target remote into the base range resolution as well.
Useful? React with 👍 / 👎.
| const isCrossRepository = | ||
| remoteRepository.repositoryNameWithOwner !== null && | ||
| originRepository.repositoryNameWithOwner !== null | ||
| targetRepository.repositoryNameWithOwner !== null | ||
| ? remoteRepository.repositoryNameWithOwner.toLowerCase() !== | ||
| originRepository.repositoryNameWithOwner.toLowerCase() | ||
| targetRepository.repositoryNameWithOwner.toLowerCase() |
There was a problem hiding this comment.
This new comparison makes conventional fork checkouts probe owner:branch first, but I checked the gh pr list manual and its --head option explicitly says <owner>:<branch> syntax is not supported (https://cli.github.com/manual/gh_pr_list). The code then falls back to a plain branch query after filtering the CLI's limited results locally, so when another fork has the same branch name and appears first (especially in findOpenPr with limit: 1), the existing upstream PR is missed and create/status can behave as if no PR exists.
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 discovery and GitHub CLI targeting for fork layouts; behavior is well-tested but affects all Git status and change-request flows when
upstreamis present.Overview
Fixes fork workflows where
ghwas scoped toorigin(the contributor fork) so open PRs on the canonical repo were missed.Provider selection prefers a GitHub
upstreamremote overoriginwhen both are GitHub (SourceControlProviderRegistry).GitHubCliandGitHubSourceControlProvideraccept an optional repository and pass--repo owner/nameon list/view/create/checkout/default-branch calls when the bound context isupstream.GitManager.resolveBranchHeadContexttreats the upstream remote as the comparison target for cross-repo detection (not onlyorigin), keepingowner:branchhead selectors while lookups run against the upstream repo.Regression tests cover fork status, CLI args, provider listing, and registry default-branch resolution; unrelated non-GitHub
upstreamremotes still do not override GitHuborigin.Reviewed by Cursor Bugbot for commit 94b4746. 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 94b4746. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.