Repository navigation
fix(pr): show a branch's own open PR, not a newer closed or same-named one - #29330
AmoabaKelvin wants to merge 1 commit into
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 36.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 38 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The PR meets the coding requirements for #28324and#27811.readReviewHeadandresolveBranchReviewTargetresolve the review head from the configured push destination. Provider lookups filter by so…Out of Scope Changes check The changes stay within the linked issue scope. Git configuration resolution, provider-specific source-repository filters, review selection, cache handling, and related tests support correct branch-to… Title check The title clearly and concisely summarizes the main change: selecting the branch’s own open pull request instead of a newer closed or same-named pull request. Description check The description is detailed and mostly complete. It includes the user impact, implementation, rationale, linked issues, visual proof, testing coverage, platform considerations, follow-ups, and checkli… Full details: Linked Issues check
-
Explanation
The PR meets the coding requirements for
#28324and#27811.readReviewHeadandresolveBranchReviewTargetresolve the review head from the configured push destination. Provider lookups filter by source repository and branch.selectReviewForHeadprefers open reviews and performs one open-state follow-up when the first page contains only closed reviews. The GitHub, GitLab, Bitbucket, Azure DevOps, and Gitea tests cover newer closed reviews, cross-fork or cross-project branches, linked-review precedence, and remembered head validation. These changes address wrong open/closed selection after branch changes and cross-fork attachment.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the linked issue scope. Git configuration resolution, provider-specific source-repository filters, review selection, cache handling, and related tests support correct branch-to-review association for
#28324and#27811. Changes to Create PR eligibility are part of the same review lifecycle: closed and merged reviews no longer block creation, while open and draft reviews still block it. No unrelated product area or unrelated behavior is demonstrated.
Full details: Description check
-
Explanation
The description is detailed and mostly complete. It includes the user impact, implementation, rationale, linked issues, visual proof, testing coverage, platform considerations, follow-ups, and checklist status. Some optional checklist items remain unchecked, but the required information is present.
-
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
56256e08-dfee-460a-a2fd-22daf6fc6e99
📥 Commits
Reviewing files that changed from the base of the PR and between 66631d0 and 21ce3ec5f64644d8a767d34aa8a919a757560905.
📒 Files selected for processing: 45
src/main/azure-devops/client.test.tssrc/main/azure-devops/client.tssrc/main/azure-devops/pull-request-mappers.tssrc/main/bitbucket/client.test.tssrc/main/bitbucket/client.tssrc/main/bitbucket/pull-request-creation.test.tssrc/main/bitbucket/pull-request-mappers.tssrc/main/gitea/client-scan-invalidation.test.tssrc/main/gitea/client.test.tssrc/main/gitea/client.tssrc/main/gitea/pull-request-mappers.tssrc/main/github/client-pr-branch-discovery.test.tssrc/main/github/client-pr-conflict-summary.test.tssrc/main/github/client-pr-fallback-number.test.tssrc/main/github/client-pr-review-head.test.tssrc/main/github/client-ssh-provider-execution-boundary.test.tssrc/main/github/client-test-harness.tssrc/main/github/client-test-mocks.tssrc/main/github/client-tracked-upstream-default-branch.test.tssrc/main/github/client-tracked-upstream-fork-owner.test.tssrc/main/github/client-tracked-upstream-snapshot.test.tssrc/main/github/client.tssrc/main/github/client/lookup/branch-lookup-resolution.tssrc/main/github/client/lookup/implicit-default-branch-pr.tssrc/main/github/client/lookup/pr-branch-lookup.tssrc/main/github/client/lookup/review-head-pr-policy.tssrc/main/github/client/lookup/tracked-upstream-branch.tssrc/main/github/client/lookup/tracked-upstream-cache.tssrc/main/github/default-branch-stale-pr.test.tssrc/main/gitlab/client-mr-branch-lookup.test.tssrc/main/gitlab/client-mr-test-harness.tssrc/main/gitlab/gitlab-project-id.tssrc/main/gitlab/mappers.tssrc/main/gitlab/merge-request-lookup.tssrc/main/source-control/branch-review-selection.tssrc/main/source-control/hosted-review-bitbucket.integration.test.tssrc/main/source-control/hosted-review-creation-eligibility.test.tssrc/main/source-control/hosted-review-creation.tssrc/main/source-control/hosted-review-gitea.integration.test.tssrc/main/source-control/repo-git-config-snapshot.test.tssrc/main/source-control/repo-git-config-snapshot.tssrc/main/source-control/review-head.tssrc/shared/git-config-snapshot-runner.tssrc/shared/git-review-head.test.tssrc/shared/git-review-head.ts
💤 Files with no reviewable changes: 7
- src/main/github/client-tracked-upstream-default-branch.test.ts
- src/main/github/client-tracked-upstream-snapshot.test.ts
- src/main/github/client-tracked-upstream-fork-owner.test.ts
- src/main/github/client.ts
- src/main/github/client/lookup/implicit-default-branch-pr.ts
- src/main/github/client/lookup/tracked-upstream-branch.ts
- src/main/github/client/lookup/tracked-upstream-cache.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| for (const candidate of args.candidates) { | ||
| if (githubRepositoryWebHost(candidate).trim().toLowerCase() !== headHost) { | ||
| continue | ||
| } | ||
| const branchData = await getRestPRForHead( | ||
| candidate, | ||
| args.headRepo.owner, | ||
| args.branchName, | ||
| args.ghOptions | ||
| ) | ||
| // Why: REST/list branch lookup identifies the PR cheaply; exact `gh pr view` carries review, merge-queue, and auto-merge state. | ||
| const data = await hydrateBranchLookupWithExactPR( | ||
| candidate, | ||
| branchData, | ||
| args.ghOptions, | ||
| args.executionScope | ||
| ) | ||
| if (data && pullRequestMatchesHeadRepository(data, candidate, args.headRepo)) { | ||
| return { data, dataRepo: candidate } | ||
| } | ||
| // Why: branch-list failures are ambiguous for fork discovery; give exact fallback-number recovery a chance before surfacing the error. | ||
| return hasPendingError | ||
| ? { data: null, dataRepo: null, pendingError } | ||
| : { data: null, dataRepo: null } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let one failing candidate abort the whole head lookup.
lookupPRByHead loops over candidates and awaits getRestPRForHead without a try block. In a fork clone the candidates are [upstream, origin]. If the upstream query throws (for example a 404 or 403 because the upstream repository is private or no longer accessible), the error propagates. getPRForBranchOutcome then reports upstream-error, and the origin candidate, which holds the PR, is never queried.
The removed lookupPRByBranchName path caught per-candidate errors and kept the first error pending. The tests for that behavior were deleted from client-pr-fallback-number.test.ts, and nothing replaces it. A persistent error on one candidate now hides the branch's PR on every poll.
Fix: catch errors per candidate and continue to the next one. Throw the first captured error only when no candidate returns a PR. Rethrow rate-limit errors immediately, so the loop does not keep spending quota.
Proposed fix
const headHost = githubRepositoryWebHost(args.headRepo).trim().toLowerCase()
+ let pendingError: unknown = null
for (const candidate of args.candidates) {
if (githubRepositoryWebHost(candidate).trim().toLowerCase() !== headHost) {
continue
}
- const branchData = await getRestPRForHead(
- candidate,
- args.headRepo.owner,
- args.branchName,
- args.ghOptions
- )
+ let branchData: PullRequestLookupData | null
+ try {
+ branchData = await getRestPRForHead(
+ candidate,
+ args.headRepo.owner,
+ args.branchName,
+ args.ghOptions
+ )
+ } catch (err) {
+ pendingError ??= err
+ continue
+ }
@@
}
+ if (pendingError) {
+ throw pendingError
+ }
return { data: null, dataRepo: null }| const config = await getRepoGitConfigSnapshot(repoPath, connectionId, options) | ||
| // Why: an unreadable config defaults to `origin/<branch>` rather than hiding the review. | ||
| const head = readReviewHead(config ?? new Map(), branchName) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP -C10 'export function (resolveGitRouteForHost|requireLocalRoute)\b' src/main/providersRepository: stablyai/orca
Length of output: 2260
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- review-head and config snapshot references ---'
rg -n -F --glob '*.ts' -e 'resolveBranchReviewTarget' -e 'getRepoGitConfigSnapshot' -e 'requireReachableGitRoute' -e 'readReviewHead' src/main
printf '%s\n' '--- review-head.ts ---'
nl -ba src/main/source-control/review-head.ts
printf '%s\n' '--- config snapshot declarations ---'
rg -n -P -C18 'function (getRepoGitConfigSnapshot|requireReachableGitRoute)|export (async )?function (getRepoGitConfigSnapshot|requireReachableGitRoute)' src/main
printf '%s\n' '--- provider lookup blocks ---'
for f in src/main/azure-devops/client.ts src/main/bitbucket/client.ts src/main/gitea/client.ts src/main/gitlab/merge-request-lookup.ts; do
echo "--- $f"
nl -ba "$f" | sed -n '185,275p'
doneRepository: stablyai/orca
Length of output: 31361
🏁 Script executed:
#!/bin/bash
set -o pipefail
rg -n -F --glob '*.ts' -e 'resolveBranchReviewTarget' -e 'getRepoGitConfigSnapshot' -e 'requireReachableGitRoute' -e 'readReviewHead' src/main
nl -ba src/main/source-control/review-head.ts
rg -n -P -C18 'function (getRepoGitConfigSnapshot|requireReachableGitRoute)|export (async )?function (getRepoGitConfigSnapshot|requireReachableGitRoute)' src/main
for f in src/main/azure-devops/client.ts src/main/bitbucket/client.ts src/main/gitea/client.ts src/main/gitlab/merge-request-lookup.ts; do
echo "--- $f"
nl -ba "$f" | sed -n '185,275p'
doneRepository: stablyai/orca
Length of output: 31219
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- config snapshot implementation ---'
nl -ba src/main/source-control/repo-git-config-snapshot.ts | sed -n '50,120p'
printf '%s\n' '--- GitLab lookup implementation ---'
nl -ba src/main/gitlab/merge-request-lookup.ts | sed -n '80,215p'
printf '%s\n' '--- GitHub branch lookup resolution ---'
nl -ba src/main/github/client/lookup/branch-lookup-resolution.ts | sed -n '1,180p'
printf '%s\n' '--- provider catch/throwOnFailure context ---'
rg -n -P -C6 'throwOnFailure|try \{|catch \(' src/main/azure-devops/client.ts src/main/bitbucket/client.ts src/main/gitea/client.tsRepository: stablyai/orca
Length of output: 31497
Preserve null-on-failure for branch lookups.
resolveBranchReviewTarget can reject with SshHostNotServedError before the provider's request error handling runs. The non-throwing Azure DevOps, Bitbucket, Gitea, and GitLab branch lookups can therefore reject instead of returning their documented null result.
Catch this resolver failure at each provider boundary. Rethrow when throwOnFailure is true; otherwise return null. Keep the existing unreachable-host behavior in getRepoGitConfigSnapshot.
Branches matched reviews by name only, so a newer closed PR, another fork's same-named branch, or the base branch's PR could stand in for the branch's own. Fixes stablyai#28324
21ce3ec to
e71709b
Compare
ELI5
Orca found a branch's pull request by asking for the newest PR with the same branch name. That let a newer closed PR hide the open one, let another fork's branch with the same name lend its PR, and let a branch that tracks
developshow develop's PR. Orca now asks only for PRs from the exact place the branch is pushed to, and shows the open one first.What Changed
Before:
mainshowed an outside contributor's PR from their fork'smain, with its checks and comments, and the next refresh brought it back after the cache entry was deleted ([Bug]: PR panel attaches a cross-fork PR to a worktree when only the head ref name matches (no head repo validation) #27811).After:
Mechanism: one rule in
src/shared/git-review-head.tsworks out where a plaingit pushof the branch lands. The remote isbranch.<b>.pushRemote, thenremote.pushDefault, then the tracked remote, thenorigin. The remote branch name is the local name, unlesspush.defaultisupstream/trackingor the branch tracks a differently named branch on a fork remote. A branch whose upstream isrefs/pull/N/headorrefs/merge-requests/N/head(whatgh pr checkoutandglab mr checkoutset up) counts as linked to that review. Orca reads all of this from onegit config --list -zper repo, cached for 30 seconds and re-read as soon as the config files change.Each provider then asks only for reviews from that repository and branch, and takes the open one first:
pulls?head=<owner>:<branch>source_project_idandsource_branchsource.repository.full_namein the BBQL querysearchCriteria.sourceRepositoryId, plus each PR's fork sourcehead.repoA head rarely has more than one review. If the first page of 10 is full and none is open, Orca asks once more for open reviews only. An explicitly linked review now wins over any branch match on every provider.
The GitHub fallback that retried with the tracked upstream branch, and the default-branch policy built around it, are removed. The push-head rule covers both.
Behaviour changes to look at:
originorupstream(saygit checkout -b fix origin/develop) no longer shows that branch's PR, unlesspush.defaultisupstream.Why
Asking for more pages or more states still guesses from a branch name. A long enough history or a same-named fork branch beats it, and every extra page costs API quota on every poll. Falling back to the tracked branch is what attached
develop's PR in #26948. Matching by commit breaks on the first rebase.gh pr viewfinds a branch's PR from its push target too, and every provider can filter on it (Gitea filters the scan it already shares).The Refresh bug behind "Pull request already exists" is a separate fix in #29231. The two PRs touch no common files.
Linked Issue
Fixes #28324
Fixes #27811
Visual Proof
Scratch repo, branch
feature/stack-a, with open PR #1 and a newer closed PR #2.Testing
gh pr checkout(shows its PR))git configoutput (own branch, trackingdevelop, triangular push remote, a contributor fork under another local name,refs/pullandrefs/merge-requestslinks, URL-valued remotes,push.default,remote.pushDefault); per provider, open over newer closed, another fork's branch ignored, a full page of closed reviews, and link precedence; Create PR eligibility for open, draft, merged and closed reviews; the config cache.Review
git config --list -zis far older than the 2.25 baseline.Agent skill upstream boundary
Notes
Follow-ups, not in this PR:
git checkout -b x upstream/developtoupstream:develop. It should push wheregit pushwould.originis the fork still looks for reviews in the fork's project.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)