fix(host-service): stop misattributing cross-fork PRs to local workspaces#3625
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds upstream owner/repo/branch tracking to workspaces, updates host-service PR query/types to include cross-repo and head repo owner/name, changes host-service sync and PR matching to use owner/repo#branch keys, and adjusts desktop PR-resolution signature/logic to account for cross-repository PRs. Changes
Sequence DiagramsequenceDiagram
participant Desktop as Desktop
participant HostService as HostService
participant Git as Git
participant GitHub as GitHub
participant DB as DB
Desktop->>HostService: syncWorkspaceBranches(localBranch)
HostService->>Git: resolveBranchUpstream(localBranch)\n(use @{push} + fallbacks -> remote/merge)
Git-->>HostService: upstreamOwner/upstreamRepo/upstreamBranch
HostService->>DB: UPDATE workspaces SET upstreamOwner, upstreamRepo, upstreamBranch, headSha, branch
HostService->>HostService: build wantedKeys = Set{owner/repo#branch}
HostService->>GitHub: PULL_REQUESTS_QUERY(wantedKeys)\n(include isCrossRepository, headRepositoryOwner, headRepository.name)
GitHub-->>HostService: PR nodes with head metadata
HostService->>HostService: fetchRepoPullRequests(wantedKeys)\nfilter & dedupe → map by owner/repo#branch
HostService->>DB: match workspaces by upstream ref and persist PR links
HostService-->>Desktop: return matched PRs keyed by owner/repo#branch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
164-176:⚠️ Potential issue | 🟡 MinorStartup race clears PR associations for pre-existing workspaces until next sync.
start()firessyncWorkspaceBranches()andrefreshEligibleProjects(true)concurrently without awaiting. After this migration, existing rows haveupstreamOwner/upstreamRepo = null, so ifrefreshEligibleProjectsruns first,performProjectRefreshproduces an emptywantedKeysand unconditionally setspullRequestId = nullfor every workspace at Line 391‑395. PRs then re-attach on the next 30s sync cycle, but users will see PR badges briefly disappear on every host-service restart until the backfill is complete.Consider awaiting the first
syncWorkspaceBranches()before kicking off the initial refresh, or inperformProjectRefreshskip theset({ pullRequestId: null })write when the workspace has no upstream key yet and already has a non-nullpullRequestId(preserve last-known until we have data to dispute it).🛠️ Suggested startup ordering
- void this.syncWorkspaceBranches(); - void this.refreshEligibleProjects(true); + void (async () => { + await this.syncWorkspaceBranches(); + await this.refreshEligibleProjects(true); + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around lines 164 - 176, start() races initial sync and refresh causing performProjectRefresh to clear pullRequestId for workspaces that haven't yet been backfilled; fix by awaiting the initial syncWorkspaceBranches() before calling refreshEligibleProjects(true) (i.e., call await this.syncWorkspaceBranches() then kick off this.refreshEligibleProjects(true)), or alternatively modify performProjectRefresh so that when a workspace has no upstream key (wantedKeys empty) it does not call set({ pullRequestId: null }) if the existing workspace.pullRequestId is non-null (preserve last-known PR until upstreamOwner/upstreamRepo are available).apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.ts (1)
107-121:⚠️ Potential issue | 🔴 CriticalMissing
isCrossRepositoryinshouldAcceptPRMatch'sprPick — reintroduces cross-repository misattribution bug.
prMatchesLocalBranchrequiresisCrossRepositoryin itsprparameter (line 85), butshouldAcceptPRMatchtypes itsprargument asPick<GHPRResponse, "headRefName" | "headRefOid" | "headRepositoryOwner" | "state">without it. At line 119, this narrower object is passed toprMatchesLocalBranch. SinceisCrossRepositoryis optional in the schema, TypeScript won't error, but at runtime when the field is missing, it becomesundefined. The guard at line 96 (if (pr.isCrossRepository) return false;) then fails silently, and the exact misattribution defect warned about in lines 94–95 leaks back in for any cross-repository PR.🔧 Proposed fix
pr: Pick< GHPRResponse, - "headRefName" | "headRefOid" | "headRepositoryOwner" | "state" + "headRefName" | "headRefOid" | "headRepositoryOwner" | "state" | "isCrossRepository" >;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.ts` around lines 107 - 121, The PR object passed into shouldAcceptPRMatch is missing the isCrossRepository field, causing prMatchesLocalBranch's guard (the pr.isCrossRepository check inside prMatchesLocalBranch) to receive undefined and allow cross-repo PRs to be misattributed; update the pr parameter type in shouldAcceptPRMatch (the Pick<GHPRResponse, ...>) to include "isCrossRepository" so the runtime value is present, and ensure any callers that construct the pr argument populate that field before calling shouldAcceptPRMatch/prMatchesLocalBranch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.ts`:
- Around line 107-121: The PR object passed into shouldAcceptPRMatch is missing
the isCrossRepository field, causing prMatchesLocalBranch's guard (the
pr.isCrossRepository check inside prMatchesLocalBranch) to receive undefined and
allow cross-repo PRs to be misattributed; update the pr parameter type in
shouldAcceptPRMatch (the Pick<GHPRResponse, ...>) to include "isCrossRepository"
so the runtime value is present, and ensure any callers that construct the pr
argument populate that field before calling
shouldAcceptPRMatch/prMatchesLocalBranch.
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 164-176: start() races initial sync and refresh causing
performProjectRefresh to clear pullRequestId for workspaces that haven't yet
been backfilled; fix by awaiting the initial syncWorkspaceBranches() before
calling refreshEligibleProjects(true) (i.e., call await
this.syncWorkspaceBranches() then kick off this.refreshEligibleProjects(true)),
or alternatively modify performProjectRefresh so that when a workspace has no
upstream key (wantedKeys empty) it does not call set({ pullRequestId: null }) if
the existing workspace.pullRequestId is non-null (preserve last-known PR until
upstreamOwner/upstreamRepo are available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e3abb71-3fc4-4836-8ff6-d846f9e3c5e8
📒 Files selected for processing (10)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.tspackages/host-service/drizzle/0003_workspace_upstream_ref.sqlpackages/host-service/drizzle/meta/0003_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/runtime/pull-requests/utils/github-query/query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/types.tspackages/host-service/test/pull-requests.test.tsplans/20260421-pr-detection-matcher-fix.md
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/runtime/pull-requests/pull-requests.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:476">
P2: PR matching uses case-sensitive owner/repo key comparison, which can miss valid matches when remote URL casing differs from GitHub API casing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…aces Two match sites keyed on branch name alone, so any cross-fork PR whose `headRefName` collided with a local branch got attached — e.g. PR #3261 (`quueli/superset-windows:main` → `superset-sh/superset:main`) latched onto every local `main` workspace in the sidebar. - `pr-resolution.ts` (v1 `usePRStatus` via `workspaces.getGitHubStatus`): reject `isCrossRepository` when the local branch has no fork-owner prefix. Added a v1-sunset marker — don't evolve this module. - `pull-requests.ts` (`_dashboard` sidebar + v2 review tab via `pullRequests.getByWorkspaces` / `git.getPullRequest`): key the match on a `(upstreamOwner, upstreamRepo, upstreamBranch)` tuple. Adds those three columns to the host-service `workspaces` table, populated in `syncWorkspaceBranches` by resolving `@{push}` first and falling back to the same config chain `gh` uses. Tuple key lowercases owner/repo (GitHub is case-insensitive there) but preserves branch casing. `upstreamBranch` is separate from local `branch` so `gh pr checkout`-style renames (`main` → `quueli-main`) still match the PR's `headRefName`. Query extended with `isCrossRepository`, `headRepositoryOwner`, `headRepository`. Local SQLite migration `0003_workspace_upstream_ref` auto-applies on next host-service start; workspaces self-heal within one 30s sync cycle.
Kitenite
force-pushed
the
pr-3261-detection-in-v1-sidebar
branch
from
79021c0 to
aff1763
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 84-126: In resolveWorkspaceUpstream, ensure a tracked remote
branch exists before returning an upstream: after computing mergeRef via
tryConfig(`branch.${localBranch}.merge`), if mergeRef is falsy then return null
(instead of falling back to localBranch); only compute branch =
mergeRef.replace(/^refs\/heads\//, "") and return the { owner, name, branch }
when mergeRef exists so fresh local branches with no upstream won't be
associated with an upstream PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e14bc0c-ea59-4e07-80f9-5cf7a4a05b2d
📒 Files selected for processing (10)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.tspackages/host-service/drizzle/0003_workspace_upstream_ref.sqlpackages/host-service/drizzle/meta/0003_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/runtime/pull-requests/utils/github-query/query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/types.tspackages/host-service/test/pull-requests.test.tsplans/20260421-pr-detection-matcher-fix.md
✅ Files skipped from review due to trivial changes (5)
- packages/host-service/drizzle/0003_workspace_upstream_ref.sql
- packages/host-service/drizzle/meta/_journal.json
- packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts
- packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts
- plans/20260421-pr-detection-matcher-fix.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/host-service/test/pull-requests.test.ts
- packages/host-service/src/db/schema.ts
- packages/host-service/drizzle/meta/0003_snapshot.json
- apps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.ts
Without an explicit merge ref, a repo-wide `remote.pushDefault` would let an untracked local branch attach to any PR on that remote whose headRefName happens to match. That's the same branch-name-collision bug this PR was introduced to prevent — just via a different route. Reported by coderabbitai on #3625.
Without an explicit merge ref, a repo-wide `remote.pushDefault` would let an untracked local branch attach to any PR on that remote whose headRefName happens to match. That's the same branch-name-collision bug this PR was introduced to prevent — just via a different route. Reported by coderabbitai on #3625.
Kitenite
force-pushed
the
pr-3261-detection-in-v1-sidebar
branch
from
8c82d26 to
6811f74
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
415-437: Extract the upstream key builder; drop the dead?? workspace.branchfallback.The
(upstreamOwner, upstreamRepo, upstreamBranch ?? branch)construction is duplicated between thewantedKeysbuild (Lines 417-421) and the per-workspace match (Lines 432-436). Additionally,resolveWorkspaceUpstreamalways returns all three fields ornull, so by constructionupstreamBranchis non-null wheneverupstreamOwner/upstreamRepoare set — meaning the?? workspace.branchfallback is dead today and, if a future code path ever persisted a partial upstream tuple, it would silently misattribute PRs to the local branch name (re-opening the very collision this PR fixes). Safer to key strictly on the persistedupstreamBranch.♻️ Proposed refactor
+ private workspaceUpstreamKey(workspace: { + upstreamOwner: string | null; + upstreamRepo: string | null; + upstreamBranch: string | null; + }): string | null { + if (!workspace.upstreamBranch) return null; + return upstreamKey( + workspace.upstreamOwner, + workspace.upstreamRepo, + workspace.upstreamBranch, + ); + } + private async performProjectRefresh(projectId: string): Promise<void> { @@ const wantedKeys = new Set<string>(); for (const workspace of projectWorkspaces) { - const key = upstreamKey( - workspace.upstreamOwner, - workspace.upstreamRepo, - workspace.upstreamBranch ?? workspace.branch, - ); + const key = this.workspaceUpstreamKey(workspace); if (key) wantedKeys.add(key); } @@ for (const workspace of projectWorkspaces) { - const key = upstreamKey( - workspace.upstreamOwner, - workspace.upstreamRepo, - workspace.upstreamBranch ?? workspace.branch, - ); + const key = this.workspaceUpstreamKey(workspace); const match = key ? keyToPullRequest.get(key) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around lines 415 - 437, Extract and centralize the upstream key construction into a single helper call (use the existing upstreamKey function) and remove the dead fallback "?? workspace.branch" so the key is built strictly from workspace.upstreamOwner, workspace.upstreamRepo, and workspace.upstreamBranch; update both the wantedKeys population and the per-workspace matching to call the same helper (referencing projectWorkspaces and upstreamKey) so the logic is not duplicated and no longer falls back to workspace.branch before calling fetchRepoPullRequests and when resolving the match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 415-437: Extract and centralize the upstream key construction into
a single helper call (use the existing upstreamKey function) and remove the dead
fallback "?? workspace.branch" so the key is built strictly from
workspace.upstreamOwner, workspace.upstreamRepo, and workspace.upstreamBranch;
update both the wantedKeys population and the per-workspace matching to call the
same helper (referencing projectWorkspaces and upstreamKey) so the logic is not
duplicated and no longer falls back to workspace.branch before calling
fetchRepoPullRequests and when resolving the match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd59753e-2bfd-4ff5-88fb-7716b66d9b8f
📒 Files selected for processing (1)
packages/host-service/src/runtime/pull-requests/pull-requests.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plans/20260421-pr-detection-matcher-fix.md (1)
34-34: Replace internal “memory” reference with a durable source.Line 34’s
project_v1_sunsetmemory reference is hard to validate later. Prefer a concrete doc/issue/ADR link for traceability.✏️ Proposed wording
-- **Consolidate v1 and v2 into one path** — deferred. v1 dies with the v1 UI sunset (see `project_v1_sunset` memory). No port needed. +- **Consolidate v1 and v2 into one path** — deferred. v1 is planned to be removed with the v1 UI sunset (link the tracking issue/ADR here). No port needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/20260421-pr-detection-matcher-fix.md` at line 34, The note referencing the ephemeral internal memory key project_v1_sunset should be replaced with a durable, verifiable reference: update the sentence that reads "see `project_v1_sunset` memory" to point to a concrete artifact (a design doc, ADR, or issue/PR URL or ID) that records the v1 UI sunset decision and timeline; ensure the replacement uses the full link or canonical identifier and, if appropriate, add a brief parenthetical summary (one line) so readers know what the linked artifact contains while keeping traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plans/20260421-pr-detection-matcher-fix.md`:
- Line 3: Replace the inaccurate phrase "Shipped in PR `#3625`." with wording that
reflects the PR is still open (e.g., "Proposed/implemented in PR `#3625`") so the
document does not imply the change is released; update the exact sentence in the
plans file that currently reads "Shipped in PR `#3625`." to "Proposed/implemented
in PR `#3625`" (or similar) to accurately convey status.
---
Nitpick comments:
In `@plans/20260421-pr-detection-matcher-fix.md`:
- Line 34: The note referencing the ephemeral internal memory key
project_v1_sunset should be replaced with a durable, verifiable reference:
update the sentence that reads "see `project_v1_sunset` memory" to point to a
concrete artifact (a design doc, ADR, or issue/PR URL or ID) that records the v1
UI sunset decision and timeline; ensure the replacement uses the full link or
canonical identifier and, if appropriate, add a brief parenthetical summary (one
line) so readers know what the linked artifact contains while keeping
traceability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8866855c-5524-4c65-ad6e-2f5411f1028a
📒 Files selected for processing (1)
plans/20260421-pr-detection-matcher-fix.md
| @@ -0,0 +1,52 @@ | |||
| # Fix PR → workspace matcher (cross-fork collision) | |||
|
|
|||
| Shipped in PR #3625. | |||
There was a problem hiding this comment.
Use “proposed/implemented in PR #3625” instead of “shipped.”
Line 3 is currently inaccurate while PR #3625 is still open, and this can confuse future readers about release state.
✏️ Proposed wording
-Shipped in PR `#3625`.
+Proposed in PR `#3625` (implemented, pending merge).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Shipped in PR #3625. | |
| Proposed in PR `#3625` (implemented, pending merge). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plans/20260421-pr-detection-matcher-fix.md` at line 3, Replace the inaccurate
phrase "Shipped in PR `#3625`." with wording that reflects the PR is still open
(e.g., "Proposed/implemented in PR `#3625`") so the document does not imply the
change is released; update the exact sentence in the plans file that currently
reads "Shipped in PR `#3625`." to "Proposed/implemented in PR `#3625`" (or similar)
to accurately convey status.
1 participant
Summary
Two workspace → PR match sites keyed on branch name alone, so any cross-fork PR whose
headRefNamecollided with a local branch got attached. Example in the wild: PR #3261 (quueli/superset-windows:main→superset-sh/superset:main) showing up on every localmainworkspace.Fixes:
apps/desktop/.../pr-resolution.ts(v1 sidebarusePRStatus/workspaces.getGitHubStatus):prMatchesLocalBranchnow rejects PRs withisCrossRepository === truewhen the local branch has no fork-owner prefix. One-line gate;isCrossRepositorywas already fetched and typed, just never consulted.packages/host-service/.../pull-requests.ts(_dashboardsidebarpullRequests.getByWorkspacesand v2 workspace review tabgit.getPullRequest): matcher now keys on(headRepositoryOwner, headRepository, headRefName)tuples instead of branch strings. AddsupstreamOwner/upstreamRepocolumns to the host-serviceworkspacestable, populated duringsyncWorkspaceBranchesby resolving the tracking remote using the same config fallback order asgh(branch.<n>.pushRemote→remote.pushDefault→branch.<n>.remote). GraphQL query extended withisCrossRepository,headRepositoryOwner { login },headRepository { name }.Migration
0003_workspace_upstream_reflives inpackages/host-service/drizzle/— local SQLite only, no Neon impact. Replaces the now-unusedworkspaces_branch_idxwith compositeworkspaces_upstream_ref_idxon(upstream_owner, upstream_repo, branch). Auto-applies on next host-service start; existing workspaces getnullupstream and self-heal within one 30s sync cycle.Full design in
plans/20260421-pr-detection-matcher-fix.md.Test plan
main.Summary by cubic
Stops cross‑fork PRs from attaching to unrelated workspaces. We now match on upstream owner/repo + tracked branch, and the v1 sidebar ignores cross‑repo PRs when the local branch has no owner prefix.
Bug Fixes
apps/desktopv1 sidebar:prMatchesLocalBranchrejectsisCrossRepositorywhen the local branch lacks a fork‑owner prefix.packages/host-service: match on${owner}/${repo}#${branch}from the tracked upstream; resolve via@{push}first, then requirebranch.<name>.mergeand fall back tobranch.<name>.pushRemote→remote.pushDefault→branch.<name>.remote. StoreupstreamOwner,upstreamRepo,upstreamBranch; lowercase owner/repo; GitHub query addsisCrossRepository,headRepositoryOwner.login,headRepository.name.Migration
upstream_owner,upstream_repo,upstream_branchand replaces the branch index with composite(upstream_owner, upstream_repo, upstream_branch)via0003_workspace_upstream_ref(auto‑applies on host‑service start). Existing workspaces backfill on next sync; restart desktop in dev to reload the host‑service.Written for commit 14587af. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores
Related
originhardcoded instead of tracking remote); this PR fixes the PR-matching half. Not a duplicate; both should land.