[fsdwen]

Summary

Fixes #7331 — PR details fail to load for fork repositories.

Root Cause

getOwnerRepo() only checked the origin remote (the fork), but in fork repos all PR operations — viewing, commenting, merging, checks — operate on the upstream canonical repo where the PR actually lives. This caused getWorkItem() and all PR detail functions to return null, showing "Unable to load details for this GitHub item."

Meanwhile getIssueOwnerRepo() already had the correct upstream-first-then-origin fallback, which is why issue lookups worked fine in fork repos.

Fix

Root fix in github-repository-identity.ts:

• getOwnerRepo() now resolves upstream-first (checks upstream remote, falls back to origin)
• New getOriginOwnerRepo() for the two callers that explicitly need the fork origin
• getIssueOwnerRepo() kept as a backwards-compatible alias

Caller updates in client.ts:

• createGitHubPullRequest → getOriginOwnerRepo (head repo is the fork)
• getRepoUpstream → getOriginOwnerRepo (fork detection)

Zero changes to work-item-details.ts and other PR detail functions — all ~10 functions that fetch PR body, comments, files, checks, etc. automatically benefit from the root fix because they all use getOwnerRepo().

Changes

File	Change
github-repository-identity.ts	getOwnerRepo upstream-first + new getOriginOwnerRepo
gh-utils.ts	Export getOriginOwnerRepo
client.ts	2 origin-specific callers use getOriginOwnerRepo
client-create-pr.test.ts	Update mocks for createGitHubPullRequest
client.test.ts	Add getOriginOwnerRepo mock, update 4 getRepoUpstream tests
gh-utils.test.ts	Update 5 tests to expect upstream-first behavior

Verification

Test results: 2291 passed, only 1 pre-existing unrelated failure (pty-subprocess WSL test).

Locally verified on a fork repo (fsdwen/openclaw with upstream openclaw/openclaw):

• Before: PR #94139 → "Unable to load details for this GitHub item."
• After: PR loads correctly with body, comments, files, and checks.

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

src/main/github/github-repository-identity.ts now resolves upstream before origin in getOwnerRepo, adds getOriginOwnerRepo for origin-only resolution, and routes getIssueOwnerRepo through getOwnerRepo. src/main/github/client.ts uses getOriginOwnerRepo for upstream and pull-request creation flows, and src/main/github/gh-utils.ts re-exports the new helper. The test suites were updated to mock getOriginOwnerRepo and to assert upstream-first call ordering where repository identity is resolved.

Sequence Diagram(s)

See hidden artifact diagram above.

Compact metadata

• Files changed: 5
• Lines changed: +64/-21

Related issues: None provided
Related PRs: None provided
Suggested labels: github, tests, refactor
Suggested reviewers: None

Poem
Upstream comes first, then origin’s trace,
A new helper marks the place.
Client calls and tests align,
One small shift in repo line.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title concisely describes the main change: switching PR owner/repo resolution to an upstream-first lookup.
Description check	✅ Passed	It covers summary, root cause, fix, change list, and verification, though several template sections are omitted.
Linked Issues check	✅ Passed	The code changes match #7331 by making PR resolution upstream-first and preserving origin-only lookup where needed.
Out of Scope Changes check	✅ Passed	The extra test and helper updates are directly supporting the upstream-first repo lookup fix and not unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/main/github/client.ts (2)

1872-1913: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

getIssueOwnerRepo name no longer reflects its usage.

The function is now used to resolve owner/repo for both issue and PR lookups, but its name still implies issue-only scope. This can mislead future maintainers reading getWorkItem or the function's definition site.

Consider renaming it to something scope-neutral (e.g. getUpstreamFirstOwnerRepo or getWorkItemOwnerRepo) in github-repository-identity.ts, updating the few call sites in client.ts and the mock in client.test.ts.

---

1888-1917: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant getIssueOwnerRepo resolution in the fallback path.

When type is unspecified, getIssueOwnerRepo(repoPath, connectionId, localGitOptions) is invoked twice with identical arguments (line 1891 and line 1913) — once for the issue attempt, once for the PR fallback. Each call shells out to git to resolve remotes; resolving it once and reusing the result would avoid a duplicate subprocess invocation on this path.

♻️ Suggested refactor

     try {
+      const ownerRepo = await getIssueOwnerRepo(repoPath, connectionId, localGitOptions)
       const issue = await fetchIssueWorkItem(
         repoPath,
-        await getIssueOwnerRepo(repoPath, connectionId, localGitOptions),
+        ownerRepo,
         number,
         connectionId,
         localGitOptions
       )
       if (issue) {
         return issue
       }
     } catch (err) {
       ...
     }
     return await fetchPullRequestWorkItem(
       repoPath,
-      await getIssueOwnerRepo(repoPath, connectionId, localGitOptions),
+      ownerRepo,
       number,
       connectionId,
       localGitOptions
     )

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7b6cdb33-335a-4bf6-90a8-5ef9af180c82

📥 Commits

Reviewing files that changed from the base of the PR and between 8070e65 and be618d1a05c5ddd567b8177f6ad13ab235cb0bf1.

📒 Files selected for processing (2)

• src/main/github/client.test.ts
• src/main/github/client.ts

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Comment exceeds the one/two-line guideline.

This "Why" comment spans 6 lines. As per path instructions, **/*.{ts,tsx,js,jsx} files should "Keep comments short — one or two lines, capturing only the non-obvious reason."

✏️ Suggested condensed comment

-      // Why: both issue and PR lookups now target `upstream`-first (via
-      // getIssueOwnerRepo), so a transient upstream failure on issue `#N`
-      // would still fall through to a PR lookup on the same upstream repo.
-      // Only fall through when the issue genuinely doesn't exist (404);
-      // re-throw everything else so the outer catch returns null and the
-      // caller sees a real failure instead of a wrong item.
+      // Why: only fall through to PR lookup on a genuine 404 (issue doesn't
+      // exist); re-throw other errors to avoid masking real failures.

📝 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.

Suggested change

	// Why: both issue and PR lookups now target `upstream`-first (via
	// getIssueOwnerRepo), so a transient upstream failure on issue #N
	// would still fall through to a PR lookup on the same upstream repo.
	// Only fall through when the issue genuinely doesn't exist (404);
	// re-throw everything else so the outer catch returns null and the
	// caller sees a real failure instead of a wrong item.
	// Why: only fall through to PR lookup on a genuine 404 (issue doesn't
	// exist); re-throw other errors to avoid masking real failures.

Source: Path instructions

[fsdwen]

@nwparker @AmethystLiang — this fixes a bug where PR details fail to load in fork repos (e.g. origin→fork, upstream→canonical). The root fix changes getOwnerRepo to resolve upstream-first, matching the existing getIssueOwnerRepo behavior. All tests pass (2291 passed). Would be great to get a review when you have a moment. Thanks!

[coderabbitai]

🧹 Nitpick comments (1)

src/main/github/gh-utils.test.ts (1)

146-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test intent is now stale given the delegation refactor.

Since getIssueOwnerRepo now fully delegates to getOwnerRepo (same cache key, same remote-resolution order), this test no longer exercises two distinct code paths — it's effectively calling the same function twice through the same cache entry. The two mockResolvedValueOnce values remain queued but the second call will likely hit cache and never consume the second mock. Consider renaming/simplifying this test to reflect that getIssueOwnerRepo is a thin alias of getOwnerRepo, or removing it as redundant with the "upstream-first" tests above.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bfd547ee-f849-4687-a7da-b79c2bb20729

📥 Commits

Reviewing files that changed from the base of the PR and between be618d1a05c5ddd567b8177f6ad13ab235cb0bf1 and 62c9b87.

📒 Files selected for processing (6)

• src/main/github/client-create-pr.test.ts
• src/main/github/client.test.ts
• src/main/github/client.ts
• src/main/github/gh-utils.test.ts
• src/main/github/gh-utils.ts
• src/main/github/github-repository-identity.ts

[nwparker]

Thanks @fsdwen — your root-cause analysis in #7331 and this fix were spot-on. The upstream-first change just landed on main in #9384 (commit 4930c87), which carries the same core fix rebased against current main plus an audit of the newer getOwnerRepo call sites that grew in since this was opened (getRepoSlug in particular now needs to stay origin-based for renderer identity). You're credited as a commit co-author on the merge. Closing this as superseded — really appreciate the contribution!