From 2e84de050eba75f5acdc1c7bb3dcc7b37786a8f1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:12:02 -0700 Subject: [PATCH] Support GitHub Enterprise work-item diffs with host-scoped execution Preserve host identity on PR/issue work items throughout the mutation and diff pipeline so Enterprise instances (including ported endpoints like github.acme.test:8443) can execute mutations without ambiguity. Rate-limit gh commands by the pre-qualified --repo host, cache auth state per ported host, and surface Enterprise hosts in project metadata and error messages. --- .../src/session/github-pr-mutations.test.ts | 51 +++++- mobile/src/session/github-pr-mutations.ts | 23 +-- mobile/src/session/github-pr-rpc.test.ts | 24 ++- mobile/src/session/github-pr-rpc.ts | 5 +- .../session/github-pr-value-readers.test.ts | 8 + mobile/src/session/pr-comment-actions.test.ts | 10 +- .../git/runner-gh-rate-limit-breaker.test.ts | 17 ++ src/main/git/runner.ts | 17 ++ src/main/github/auth-diagnose.test.ts | 12 ++ src/main/github/auth-diagnose.ts | 2 +- src/main/github/client-file-viewed.test.ts | 17 ++ .../github/client-pr-local-runtime.test.ts | 25 +++ src/main/github/client.test.ts | 56 +++++- src/main/github/client.ts | 17 +- src/main/github/github-api-repository.test.ts | 30 +++ src/main/github/github-api-repository.ts | 19 +- .../github-enterprise-repository.test.ts | 56 ++++++ .../github/github-enterprise-repository.ts | 173 ++++++++++++------ src/main/github/project-view.test.ts | 20 ++ src/main/github/project-view.ts | 14 +- src/main/github/project-view/internals.ts | 6 +- src/main/github/project-view/mutations.ts | 6 +- .../project-error-classification.ts | 13 +- .../github/work-item-details-pr-files.test.ts | 63 ++++++- src/main/github/work-item-details.ts | 65 ++++--- .../src/components/GitHubItemDialog.tsx | 89 +++++---- .../src/components/PullRequestPage.tsx | 87 +++++---- src/renderer/src/components/TaskPage.tsx | 43 +++-- ...b-enterprise-slug-routing-boundary.test.ts | 19 +- ...github-item-dialog-source-boundary.test.ts | 16 +- .../github-project/ProjectViewWrapper.tsx | 78 ++++---- ...ew-wrapper-source-context-boundary.test.ts | 35 ++++ .../github/IssueSourceIndicator.tsx | 19 +- .../pull-request-page-host-boundary.test.ts | 11 +- .../components/right-sidebar/ChecksPanel.tsx | 5 +- .../src/hooks/useGitHubSlugMetadata.test.tsx | 27 ++- .../src/hooks/useGitHubSlugMetadata.ts | 7 +- src/renderer/src/store/slices/github.test.ts | 6 + src/renderer/src/web/web-preload-api.test.ts | 3 +- src/renderer/src/web/web-preload-api.ts | 5 +- .../project-host-setup-projection.test.ts | 55 ++++++ src/shared/project-host-setup-projection.ts | 95 ++++++++-- 42 files changed, 1037 insertions(+), 312 deletions(-) diff --git a/mobile/src/session/github-pr-mutations.test.ts b/mobile/src/session/github-pr-mutations.test.ts index d7d722ea966..52a6ef824ff 100644 --- a/mobile/src/session/github-pr-mutations.test.ts +++ b/mobile/src/session/github-pr-mutations.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it, vi } from 'vitest' import type { RpcResponse } from '../transport/types' import { + fetchAddIssueComment, + fetchAddPRReviewCommentReply, fetchDeleteIssueComment, fetchMergePR, fetchResolveReviewThread, @@ -29,6 +31,7 @@ function clientRejecting(message: string) { } const WORKTREE_ID = 'repo-42::/path/to/wt' +const ENTERPRISE_PR_REPO = { owner: 'o', repo: 'r', host: 'github.acme.test' } describe('fetchResolveReviewThread / fetchUpdatePRTitle — bare-boolean host result', () => { it('treats an explicit true as success', async () => { @@ -102,12 +105,51 @@ describe('mutation transport rejection normalization', () => { }) }) +describe('Enterprise PR repo forwarding', () => { + it('forwards the host through title, root-comment, and review-reply RPCs', async () => { + const titleClient = clientReturning(okResponse(true)) + await fetchUpdatePRTitle(titleClient, WORKTREE_ID, { + prNumber: 7, + title: 'New title', + prRepo: ENTERPRISE_PR_REPO + }) + expect(titleClient.sendRequest).toHaveBeenCalledWith( + 'github.updatePRTitle', + expect.objectContaining({ prRepo: ENTERPRISE_PR_REPO }) + ) + + const rootClient = clientReturning(okResponse({ ok: true })) + await fetchAddIssueComment(rootClient, WORKTREE_ID, { + prNumber: 7, + body: 'Root comment', + prRepo: ENTERPRISE_PR_REPO + }) + expect(rootClient.sendRequest).toHaveBeenCalledWith( + 'github.addIssueComment', + expect.objectContaining({ prRepo: ENTERPRISE_PR_REPO }) + ) + + const replyClient = clientReturning(okResponse({ ok: true })) + await fetchAddPRReviewCommentReply(replyClient, WORKTREE_ID, { + prNumber: 7, + commentId: 42, + body: 'Reply', + prRepo: ENTERPRISE_PR_REPO + }) + expect(replyClient.sendRequest).toHaveBeenCalledWith( + 'github.addPRReviewCommentReply', + expect.objectContaining({ prRepo: ENTERPRISE_PR_REPO }) + ) + }) +}) + describe('fetchUpdateIssueComment / fetchDeleteIssueComment — slug-addressed envelope', () => { it('sends owner/repo/commentId(+body) and reads the { ok } envelope', async () => { const editClient = clientReturning(okResponse({ ok: true })) const edit = await fetchUpdateIssueComment(editClient, { owner: 'o', repo: 'r', + host: 'github.acme.test', commentId: 5, body: 'edited' }) @@ -115,16 +157,23 @@ describe('fetchUpdateIssueComment / fetchDeleteIssueComment — slug-addressed e expect(editClient.sendRequest).toHaveBeenCalledWith('github.project.updateIssueCommentBySlug', { owner: 'o', repo: 'r', + host: 'github.acme.test', commentId: 5, body: 'edited' }) const delClient = clientReturning(okResponse({ ok: true })) - const del = await fetchDeleteIssueComment(delClient, { owner: 'o', repo: 'r', commentId: 5 }) + const del = await fetchDeleteIssueComment(delClient, { + owner: 'o', + repo: 'r', + host: 'github.acme.test', + commentId: 5 + }) expect(del).toEqual({ ok: true }) expect(delClient.sendRequest).toHaveBeenCalledWith('github.project.deleteIssueCommentBySlug', { owner: 'o', repo: 'r', + host: 'github.acme.test', commentId: 5 }) }) diff --git a/mobile/src/session/github-pr-mutations.ts b/mobile/src/session/github-pr-mutations.ts index 4c52f5a8fbc..d9fa04ce06a 100644 --- a/mobile/src/session/github-pr-mutations.ts +++ b/mobile/src/session/github-pr-mutations.ts @@ -101,16 +101,10 @@ export async function fetchUpdatePRTitle( args: { prNumber: number; title: string; prRepo?: GitHubPrRepoSlug | null } ): Promise { const params: Record = { prNumber: args.prNumber, title: args.title } - // updatePRTitle accepts prRepo for fork PRs, but it is not in the centralized - // METHODS_ACCEPTING_PR_REPO read allow-list — pass it explicitly so it reaches the - // host schema (which declares it optional/nullable). - if (args.prRepo) { - params.prRepo = githubPrRepoSlugParam(args.prRepo) - } const response = await sendRaw( client, 'github.updatePRTitle', - buildGithubPrParams('github.updatePRTitle', worktreeId, params) + buildGithubPrParams('github.updatePRTitle', worktreeId, params, { prRepo: args.prRepo }) ) if (!response.ok) { return { ok: false, error: response.error || 'Request failed: github.updatePRTitle' } @@ -225,16 +219,12 @@ export async function fetchAddPRReviewCommentReply( if (typeof args.line === 'number') { params.line = args.line } - // addPRReviewCommentReply accepts prRepo for fork PRs, but it is not in the - // centralized METHODS_ACCEPTING_PR_REPO allow-list (read-focused) — pass it - // explicitly so it reaches the host schema, which declares it optional. - if (args.prRepo) { - params.prRepo = githubPrRepoSlugParam(args.prRepo) - } return sendGithubPrMutation( client, 'github.addPRReviewCommentReply', - buildGithubPrParams('github.addPRReviewCommentReply', worktreeId, params) + buildGithubPrParams('github.addPRReviewCommentReply', worktreeId, params, { + prRepo: args.prRepo + }) ) } @@ -249,13 +239,10 @@ export async function fetchAddIssueComment( body: args.body, type: 'pr' } - if (args.prRepo) { - params.prRepo = githubPrRepoSlugParam(args.prRepo) - } return sendGithubPrMutation( client, 'github.addIssueComment', - buildGithubPrParams('github.addIssueComment', worktreeId, params) + buildGithubPrParams('github.addIssueComment', worktreeId, params, { prRepo: args.prRepo }) ) } diff --git a/mobile/src/session/github-pr-rpc.test.ts b/mobile/src/session/github-pr-rpc.test.ts index 767fcbfeaeb..a287aa4f616 100644 --- a/mobile/src/session/github-pr-rpc.test.ts +++ b/mobile/src/session/github-pr-rpc.test.ts @@ -103,13 +103,17 @@ describe('readPRForBranch', () => { const parsed = readPRForBranch({ number: 3, state: 'open', - prRepo: { owner: 'forkOwner', repo: 'forkRepo' }, + prRepo: { owner: 'forkOwner', repo: 'forkRepo', host: 'github.acme.test' }, mergeMethodSettings: { defaultMethod: 'squash', allowedMethods: { merge: false, squash: true, rebase: true } } }) - expect(parsed?.prRepo).toEqual({ owner: 'forkOwner', repo: 'forkRepo' }) + expect(parsed?.prRepo).toEqual({ + owner: 'forkOwner', + repo: 'forkRepo', + host: 'github.acme.test' + }) expect(parsed?.mergeMethodSettings).toEqual({ defaultMethod: 'squash', allowedMethods: { merge: false, squash: true, rebase: true } @@ -266,7 +270,7 @@ describe('readAssignableUsers', () => { }) describe('buildGithubPrParams — method-aware prRepo / headSha', () => { - const fork = { owner: 'forkOwner', repo: 'forkRepo' } + const fork = { owner: 'forkOwner', repo: 'forkRepo', host: 'github.acme.test' } it('reuses mobileRepoSelectorFromWorktreeId for the repo selector', () => { const params = buildGithubPrParams('github.prChecks', WORKTREE_ID, { prNumber: 1 }) @@ -286,9 +290,12 @@ describe('buildGithubPrParams — method-aware prRepo / headSha', () => { 'github.removePRReviewers', 'github.mergePR', 'github.setPRAutoMerge', + 'github.updatePRTitle', 'github.prComments', 'github.prFileContents', - 'github.addPRReviewComment' + 'github.addPRReviewComment', + 'github.addIssueComment', + 'github.addPRReviewCommentReply' ]) { const params = buildGithubPrParams(method, WORKTREE_ID, { prNumber: 1 }, { prRepo: fork }) expect(params.prRepo).toEqual(fork) @@ -382,12 +389,15 @@ describe('fetch wrappers', () => { expect('headSha' in (params as object)).toBe(false) }) - it('fetchGithubRepoSlug returns the slug for a github repo, null otherwise', async () => { + it('fetchGithubRepoSlug preserves an Enterprise host, and returns null otherwise', async () => { const found = await fetchGithubRepoSlug( - mockClient(okResponse({ owner: 'o', repo: 'r' })).client, + mockClient(okResponse({ owner: 'o', repo: 'r', host: 'github.acme.test' })).client, WORKTREE_ID ) - expect(found).toEqual({ ok: true, result: { owner: 'o', repo: 'r' } }) + expect(found).toEqual({ + ok: true, + result: { owner: 'o', repo: 'r', host: 'github.acme.test' } + }) const none = await fetchGithubRepoSlug(mockClient(okResponse(null)).client, WORKTREE_ID) expect(none).toEqual({ ok: true, result: null }) }) diff --git a/mobile/src/session/github-pr-rpc.ts b/mobile/src/session/github-pr-rpc.ts index c83b155cbe9..0ae009d926a 100644 --- a/mobile/src/session/github-pr-rpc.ts +++ b/mobile/src/session/github-pr-rpc.ts @@ -54,9 +54,12 @@ const METHODS_ACCEPTING_PR_REPO = new Set([ 'github.removePRReviewers', 'github.mergePR', 'github.setPRAutoMerge', + 'github.updatePRTitle', 'github.prComments', 'github.prFileContents', - 'github.addPRReviewComment' + 'github.addPRReviewComment', + 'github.addIssueComment', + 'github.addPRReviewCommentReply' ]) // Why: only github.prChecks declares a `headSha` param (PullRequestCheckDetails diff --git a/mobile/src/session/github-pr-value-readers.test.ts b/mobile/src/session/github-pr-value-readers.test.ts index cfd3142cff7..0b78d11dbb1 100644 --- a/mobile/src/session/github-pr-value-readers.test.ts +++ b/mobile/src/session/github-pr-value-readers.test.ts @@ -9,6 +9,14 @@ describe('readRepoIdentity', () => { }) }) + it('preserves an Enterprise host', () => { + expect(readRepoIdentity({ owner: 'octo', repo: 'orca', host: 'github.acme.test' })).toEqual({ + owner: 'octo', + repo: 'orca', + host: 'github.acme.test' + }) + }) + it('drops a non-record value', () => { expect(readRepoIdentity(null)).toBeUndefined() expect(readRepoIdentity('octo/orca')).toBeUndefined() diff --git a/mobile/src/session/pr-comment-actions.test.ts b/mobile/src/session/pr-comment-actions.test.ts index 4d70428845d..bb6f1758b24 100644 --- a/mobile/src/session/pr-comment-actions.test.ts +++ b/mobile/src/session/pr-comment-actions.test.ts @@ -134,13 +134,19 @@ describe('canEditComment / canDeleteComment', () => { describe('buildEditCommentParams / buildDeleteCommentParams', () => { it('build slug-addressed params', () => { - const slug = { owner: 'o', repo: 'r' } + const slug = { owner: 'o', repo: 'r', host: 'github.acme.test' } expect(buildEditCommentParams(slug, 42, 'new body')).toEqual({ owner: 'o', repo: 'r', + host: 'github.acme.test', commentId: 42, body: 'new body' }) - expect(buildDeleteCommentParams(slug, 42)).toEqual({ owner: 'o', repo: 'r', commentId: 42 }) + expect(buildDeleteCommentParams(slug, 42)).toEqual({ + owner: 'o', + repo: 'r', + host: 'github.acme.test', + commentId: 42 + }) }) }) diff --git a/src/main/git/runner-gh-rate-limit-breaker.test.ts b/src/main/git/runner-gh-rate-limit-breaker.test.ts index 24fd2d5d0e6..17e14b8e7d6 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -138,6 +138,23 @@ describe('ghExecFileAsync rate-limit breaker', () => { } }) + it.each([ + ['--repo', ['pr', 'list', '--repo', 'github.acme-corp.com/acme/widgets']], + ['-R', ['pr', 'list', '-R', 'github.acme-corp.com/acme/widgets']], + ['--repo=', ['pr', 'list', '--repo=github.acme-corp.com/acme/widgets']] + ])('scopes a pre-qualified %s value to its repository host', async (_name, args) => { + mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) + + await expect( + ghExecFileAsync(args, { env: { ...process.env, GH_HOST: 'github.com' } }) + ).rejects.toThrow('rate limit') + + expect( + getGhRateLimitBlockedUntilMs('core', Date.now(), 'native:github.acme-corp.com') + ).not.toBeNull() + expect(getGhRateLimitBlockedUntilMs('core', Date.now(), 'native:github.com')).toBeNull() + }) + it('scopes a WSL UNC-cwd 403 to the distro runtime and probes that scope', async () => { // Why: a \\wsl.localhost\... cwd routes gh through wsl.exe, so the 403 // belongs to the distro's account — the native scope must stay usable and diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 5efa1aca015..d4111a3210b 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1339,6 +1339,22 @@ function explicitGhHostname(args: readonly string[]): string | undefined { return undefined } +function explicitGhRepoHostname(args: readonly string[]): string | undefined { + for (let i = 0; i < args.length; i += 1) { + let value: string | undefined + if (args[i] === '--repo' || args[i] === '-R') { + value = args[i + 1] + } else if (args[i].startsWith('--repo=')) { + value = args[i].slice('--repo='.length) + } + const parts = value?.trim().split('/') + if (parts?.length === 3 && parts.every(Boolean)) { + return parts[0] + } + } + return undefined +} + function ghRateLimitScope( args: readonly string[], options: GhExecOptions, @@ -1350,6 +1366,7 @@ function ghRateLimitScope( const host = explicitGhHostname(args) ?? options.host ?? + explicitGhRepoHostname(args) ?? options.env?.GH_HOST ?? process.env.GH_HOST ?? 'github.com' diff --git a/src/main/github/auth-diagnose.test.ts b/src/main/github/auth-diagnose.test.ts index 1218824f88b..7735910dc83 100644 --- a/src/main/github/auth-diagnose.test.ts +++ b/src/main/github/auth-diagnose.test.ts @@ -88,6 +88,18 @@ ghe.acme.io expect(accounts[1].scopes).toContain('project') }) + it('parses a ported GHES auth-status section', () => { + const text = `ghe.acme.io:8443 + ✓ Logged in to ghe.acme.io:8443 account bob (keyring) + - Active account: true + - Token scopes: 'project', 'repo' +` + + expect(parseAuthStatus(text)).toEqual([ + expect.objectContaining({ host: 'ghe.acme.io:8443', user: 'bob', active: true }) + ]) + }) + it('recovers host from the Logged-in line when the section header is missing', () => { // gh prints a colon after the host on some versions; we tolerate it, // but if the regex ever fails to match the header we still want diff --git a/src/main/github/auth-diagnose.ts b/src/main/github/auth-diagnose.ts index 3c06785595c..8051af7f8b4 100644 --- a/src/main/github/auth-diagnose.ts +++ b/src/main/github/auth-diagnose.ts @@ -39,7 +39,7 @@ export function parseAuthStatus(text: string): GhAuthAccount[] { // (internal GHES like `github` or `ghe-internal`); we also recover the // host from the `Logged in to ` line below if this header was // missed, so a parser miss never silently drops every account. - const hostMatch = line.match(/^([a-z0-9][a-z0-9.-]*)\s*:?\s*$/i) + const hostMatch = line.match(/^([a-z0-9][a-z0-9.-]*(?::\d+)?)\s*:?\s*$/i) if (hostMatch && !/^logged\b/i.test(line)) { currentHost = hostMatch[1] continue diff --git a/src/main/github/client-file-viewed.test.ts b/src/main/github/client-file-viewed.test.ts index b9a0bc6fcd4..b3f066daa7a 100644 --- a/src/main/github/client-file-viewed.test.ts +++ b/src/main/github/client-file-viewed.test.ts @@ -156,4 +156,21 @@ describe('setPRFileViewed', () => { const [args] = ghExecFileAsyncMock.mock.calls[0] expect(args).not.toContain('--hostname') }) + + it('refuses an unresolved local file-viewed mutation', async () => { + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + + await expect( + setPRFileViewed({ + repoPath: '/repo-root', + prRepo: { owner: 'team', repo: 'orca' }, + pullRequestId: 'PR_unresolved', + path: 'src/app.ts', + viewed: true + }) + ).resolves.toBe(false) + + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index d54db52aaa1..bbe8c02127e 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -333,6 +333,31 @@ describe('GitHub PR local runtime routing', () => { expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) + it('refuses unresolved local PR mutations instead of using ambient gh defaults', async () => { + const legacyRepo = { owner: 'team', repo: 'orca' } + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + + await expect( + resolveReviewThread('/repo-root', 'thread-1', true, null, legacyRepo) + ).resolves.toBe(false) + await expect(updatePRTitle('/repo-root', 7, 'New title', null, legacyRepo)).resolves.toBe(false) + await expect( + requestPRReviewers('/repo-root', 7, ['octo'], null, legacyRepo) + ).resolves.toMatchObject({ ok: false }) + await expect( + removePRReviewers('/repo-root', 7, ['octo'], null, legacyRepo) + ).resolves.toMatchObject({ ok: false }) + await expect(mergePR('/repo-root', 7, 'squash', null, legacyRepo)).resolves.toMatchObject({ + ok: false + }) + await expect( + setPRAutoMerge('/repo-root', 7, true, 'squash', null, legacyRepo) + ).resolves.toMatchObject({ ok: false }) + + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + it('host-qualifies SSH-backed GitHub Enterprise review reads and mutations', async () => { const enterpriseRepo = { owner: 'team', diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 81f30a472dc..cdbcfb9f5a6 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -76,6 +76,9 @@ vi.mock('./gh-utils', () => ({ if (lower.includes('rate limit')) { return { type: 'rate_limited', message: stderr } } + if (lower.includes('resource not accessible')) { + return { type: 'permission_denied', message: stderr } + } return { type: 'unknown', message: stderr } }, parseGitHubOwnerRepo: (remoteUrl: string) => { @@ -3776,6 +3779,45 @@ describe('getPRForBranch', () => { ) }) + it('probes the next PR work-item candidate after a permission denial', async () => { + const upstream = { owner: 'upstream', repo: 'orca', host: 'github.com' } + const origin = { owner: 'fork', repo: 'orca', host: 'github.com' } + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [upstream, origin], + headRepo: origin + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('GraphQL: Resource not accessible by integration')) + .mockRejectedValueOnce(new Error('GraphQL: Resource not accessible by integration')) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 42, + title: 'Origin PR', + state: 'OPEN', + url: 'https://github.com/fork/orca/pull/42', + labels: [], + updatedAt: '2026-07-16T00:00:00Z', + author: { login: 'octo' }, + isDraft: false + }) + }) + .mockRejectedValueOnce(new Error('metadata unavailable')) + + await expect(getWorkItem('/repo-root', 42, 'pr')).resolves.toMatchObject({ + number: 42, + title: 'Origin PR', + prRepo: origin + }) + + expect(ghExecFileAsyncMock.mock.calls[0][0]).toEqual( + expect.arrayContaining(['pr', 'view', '--repo', 'upstream/orca']) + ) + expect(ghExecFileAsyncMock.mock.calls[1][0]).toEqual(['api', 'repos/upstream/orca/pulls/42']) + expect(ghExecFileAsyncMock.mock.calls[2][0]).toEqual( + expect.arrayContaining(['pr', 'view', '--repo', 'fork/orca']) + ) + }) + it('normalizes reviewer avatars from REST pull request payloads', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock.mockResolvedValueOnce({ @@ -4089,7 +4131,8 @@ describe('GitHub GraphQL rate-limit guard', () => { await expect( setPRAutoMerge('/repo-root', 7, true, 'squash', undefined, { owner: 'stablyai', - repo: 'orca' + repo: 'orca', + host: 'github.com' }) ).resolves.toEqual({ ok: true }) @@ -4116,7 +4159,8 @@ describe('GitHub GraphQL rate-limit guard', () => { await expect( setPRAutoMerge('/repo-root', 7, true, 'squash', undefined, { owner: 'stablyai', - repo: 'orca' + repo: 'orca', + host: 'github.com' }) ).resolves.toEqual({ ok: false, @@ -4137,21 +4181,23 @@ describe('GitHub GraphQL rate-limit guard', () => { await expect( setPRAutoMerge('/repo-root', 7, true, 'squash', undefined, { owner: 'stablyai', - repo: 'orca' + repo: 'orca', + host: 'github.com' }) ).resolves.toEqual({ ok: true }) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, expect.arrayContaining(['api', 'graphql', '-f', 'branch=main']), - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['pr', 'merge', '7', '--auto', '--squash', '--repo', 'stablyai/orca'], expect.objectContaining({ cwd: '/repo-root', - env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) + env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }), + host: 'github.com' }) ) expect( diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 8d74b6b496f..6c05d286c2f 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -981,7 +981,8 @@ async function fetchPullRequestWorkItemFromCandidates( ) } catch (err) { const message = err instanceof Error ? err.message : String(err) - if (classifyGhError(message).type !== 'not_found') { + const classification = classifyGhError(message).type + if (classification !== 'not_found' && classification !== 'permission_denied') { throw err } } @@ -4380,7 +4381,7 @@ export async function setPRFileViewed(args: { args.connectionId, args.localGitOptions ) - if (args.connectionId && !ownerRepo) { + if (!ownerRepo) { return false } const mutation = args.viewed ? 'markFileAsViewed' : 'unmarkFileAsViewed' @@ -4432,7 +4433,7 @@ export async function resolveReviewThread( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return false } const guard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) @@ -4617,7 +4618,7 @@ export async function mergePR( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } await acquire() @@ -4668,7 +4669,7 @@ export async function setPRAutoMerge( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } await acquire() @@ -4919,7 +4920,7 @@ export async function requestPRReviewers( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } await acquire() @@ -4960,7 +4961,7 @@ export async function removePRReviewers( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } await acquire() @@ -5000,7 +5001,7 @@ export async function updatePRTitle( connectionId, localGitOptions ) - if (connectionId && !ownerRepo) { + if (!ownerRepo) { return false } await acquire() diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index e3fd4666c7d..0d052653061 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -22,6 +22,7 @@ vi.mock('./github-enterprise-repository', async (importOriginal) => ({ import { _resetOriginGitHubApiRepositoryCache, + getOriginGitHubApiRepository, githubHostExecOptions, resolveGitHubApiRepository, resolveGitHubRepoExecution @@ -154,6 +155,12 @@ describe('resolveGitHubRepoExecution', () => { ).resolves.toEqual({ ownerRepo: null, ghOptions: {} }) }) + it('rejects a host-less caller-specific resolver for an unresolved local repository', async () => { + await expect( + resolveGitHubRepoExecution('/repo', async () => ({ owner: 'upstream', repo: 'widgets' })) + ).resolves.toEqual({ ownerRepo: null, ghOptions: { cwd: '/repo' } }) + }) + it('preserves an authoritative null from a caller-specific resolver', async () => { getOwnerRepoMock.mockResolvedValue({ owner: 'origin', repo: 'widgets' }) @@ -165,3 +172,26 @@ describe('resolveGitHubRepoExecution', () => { expect(getOwnerRepoMock).not.toHaveBeenCalled() }) }) + +describe('origin repository cache', () => { + it('does not cache an indeterminate Enterprise auth probe', async () => { + const enterprise = { + owner: 'acme', + repo: 'widgets', + host: 'github.acme-corp.com' + } + getEnterpriseGitHubRepoSlugMock + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(enterprise) + + await expect(getOriginGitHubApiRepository('/repo')).resolves.toBeNull() + await expect(getOriginGitHubApiRepository('/repo')).resolves.toEqual(enterprise) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) + + it('still caches a definitive negative Enterprise probe', async () => { + await expect(getOriginGitHubApiRepository('/repo')).resolves.toBeNull() + await expect(getOriginGitHubApiRepository('/repo')).resolves.toBeNull() + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 6e372592313..7ef11c9f8fb 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -130,9 +130,16 @@ export async function getGitHubApiRepositoryForRemote( connectionId, enterpriseOptions ) - originRepoCache.set(cacheKey, { value: slug, expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS }) - pruneOriginRepoCache(Date.now()) - return slug + // Why: undefined means the gh auth inventory could not be read. Caching it + // as a negative would turn a transient spawn failure into a 30-second miss. + if (slug !== undefined) { + originRepoCache.set(cacheKey, { + value: slug, + expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS + }) + pruneOriginRepoCache(Date.now()) + } + return slug ?? null })() originRepoInFlight.set(cacheKey, probe) try { @@ -289,9 +296,9 @@ export async function resolveGitHubApiRepository( if (originRepository?.host) { return { ...repository, host: originRepository.host } } - // Why: connection-backed gh has no cwd to infer a host from. An unhosted - // legacy identity is unsafe there because gh would target its default account. - return connectionId ? null : repository + // Why: a host-less identity can honor ambient GH_HOST even with a local cwd. + // Only a resolved origin may supply the execution host for legacy clients. + return null } export function isGitHubDotComRepository(repository: GitHubApiRepository): boolean { diff --git a/src/main/github/github-enterprise-repository.test.ts b/src/main/github/github-enterprise-repository.test.ts index 2cf8914f987..0a361ea1323 100644 --- a/src/main/github/github-enterprise-repository.test.ts +++ b/src/main/github/github-enterprise-repository.test.ts @@ -78,6 +78,28 @@ describe('getEnterpriseGitHubRepoSlug', () => { }) }) + it('uses the unique ported auth host for a hostname-only SSH remote', async () => { + mockOriginRemote('git@ghe.acme.com:team/orca.git') + mockHostAuthenticated('ghe.acme.com:8443') + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com:8443' + }) + }) + + it('uses the auth inventory host when an HTTPS remote includes a non-default port', async () => { + mockOriginRemote('https://ghe.acme.com:8443/team/orca.git') + mockHostAuthenticated('ghe.acme.com') + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com' + }) + }) + it('probes gh in the repository WSL runtime, not the host/default distro', async () => { mockOriginRemote('https://github.acme-corp.com/team/orca.git') mockHostAuthenticated() @@ -119,6 +141,22 @@ describe('getEnterpriseGitHubRepoSlug', () => { await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toBeNull() }) + + it('preserves an indeterminate auth inventory failure so a later probe can recover', async () => { + mockOriginRemote('git@ghe.acme.com:team/orca.git') + ghExecFileAsyncMock.mockRejectedValueOnce( + Object.assign(new Error('not installed'), { stdout: '', stderr: '' }) + ) + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toBeUndefined() + + mockHostAuthenticated('ghe.acme.com:8443') + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com:8443' + }) + }) }) describe('isGitHubHostAuthenticated', () => { @@ -226,6 +264,24 @@ describe('isGitHubHostAuthenticated', () => { expect(ghExecFileAsyncMock.mock.calls.flat()).not.toContain('evil.example.test') }) + it('does not guess between multiple ported auth hosts for one SSH hostname', async () => { + ghExecFileAsyncMock.mockResolvedValue({ + stdout: `ghe.acme.com:8443 + ✓ Logged in to ghe.acme.com:8443 account kelora (keyring) +ghe.acme.com:9443 + ✓ Logged in to ghe.acme.com:9443 account other (keyring)`, + stderr: '' + }) + + await expect(isGitHubHostAuthenticated('ghe.acme.com', '/repo')).resolves.toBe(false) + }) + + it('treats default web ports as the same auth host', async () => { + mockHostAuthenticated('ghe.acme.com:443') + + await expect(isGitHubHostAuthenticated('ghe.acme.com', '/repo')).resolves.toBe(true) + }) + it('validates a global project host by inventory without targeting it', async () => { mockHostAuthenticated('github.acme-corp.com') diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index 35e3cc2533f..b6b28e62f4e 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -24,12 +24,12 @@ const HOST_AUTH_TTL_MS = 60_000 const HOST_AUTH_CACHE_MAX_ENTRIES = 512 type HostAuthCacheEntry = { - authenticated: boolean + authenticatedHost: string | null expiresAt: number } const hostAuthCache = new Map() -const hostAuthInFlight = new Map>() +const hostAuthInFlight = new Map>() // Why: connection-backed Git operations execute remotely, but gh intentionally // executes on the native host. Only WSL selects a distinct gh config/runtime. @@ -75,6 +75,110 @@ function ghCommandOutput(error: unknown): string { .join('\n') } +type NormalizedGitHubHost = { + hostname: string + port: string | null + authority: string +} + +function normalizeGitHubHost(host: string): NormalizedGitHubHost | null { + const match = host + .trim() + .toLowerCase() + .match(/^([a-z0-9][a-z0-9.-]*)(?::(\d+))?$/i) + if (!match) { + return null + } + const hostname = match[1] + const rawPort = match[2] ?? null + // Why: default web ports do not distinguish a gh auth host from the same + // hostname without an explicit port. + const port = rawPort === '80' || rawPort === '443' ? null : rawPort + return { hostname, port, authority: port ? `${hostname}:${port}` : hostname } +} + +function authenticatedHostFromInventory(host: string, output: string): string | null { + const requested = normalizeGitHubHost(host) + if (!requested) { + return null + } + const inventory = Array.from( + new Map( + parseAuthStatus(output) + .map((account) => normalizeGitHubHost(account.host)) + .filter((candidate): candidate is NormalizedGitHubHost => candidate !== null) + .map((candidate) => [candidate.authority, candidate]) + ).values() + ) + const exact = inventory.find((candidate) => candidate.authority === requested.authority) + if (exact) { + return exact.authority + } + const compatible = inventory.filter( + (candidate) => + candidate.hostname === requested.hostname && (!requested.port || candidate.port === null) + ) + // Why: an SSH remote has no API port. Only a unique auth-inventory host can + // safely supply it; multiple ported endpoints on one hostname are ambiguous. + return compatible.length === 1 ? compatible[0].authority : null +} + +async function resolveAuthenticatedGitHubHost( + host: string, + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + const normalizedHost = normalizeGitHubHost(host)?.authority ?? host.trim().toLowerCase() + const cacheKey = `${runtimeCacheKey(repoPath, localGitOptions.wslDistro)}\0${normalizedHost}` + const now = Date.now() + pruneHostAuthCache(now) + const cached = hostAuthCache.get(cacheKey) + if (cached && cached.expiresAt > now) { + return cached.authenticatedHost + } + const inFlight = hostAuthInFlight.get(cacheKey) + if (inFlight) { + return inFlight + } + // Why: provider detection and review loading can probe the same runtime at + // once; coalesce them so one host never spawns duplicate auth subprocesses. + const probe = (async () => { + const execOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + } + let authenticatedHost: string | null + try { + const { stdout, stderr } = await ghExecFileAsync(['auth', 'status'], execOptions) + authenticatedHost = authenticatedHostFromInventory(host, `${stdout}\n${stderr}`) + } catch (error) { + const output = ghCommandOutput(error) + if (!output) { + // Indeterminate (gh missing / spawn failure) — do not cache so a later + // probe (gh installed, tunnel ready, token added) can recover. + return undefined + } + // gh exits non-zero when a host has a token problem but still prints the + // per-host status; trust only hosts that are actually listed. + authenticatedHost = authenticatedHostFromInventory(host, output) + } + hostAuthCache.set(cacheKey, { + authenticatedHost, + expiresAt: Date.now() + HOST_AUTH_TTL_MS + }) + pruneHostAuthCache(Date.now()) + return authenticatedHost + })() + hostAuthInFlight.set(cacheKey, probe) + try { + return await probe + } finally { + if (hostAuthInFlight.get(cacheKey) === probe) { + hostAuthInFlight.delete(cacheKey) + } + } +} + /** * Whether `gh` is authenticated to `host` from the repository's own runtime. * @@ -89,55 +193,9 @@ export async function isGitHubHostAuthenticated( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const normalizedHost = host.toLowerCase() - const cacheKey = `${runtimeCacheKey(repoPath, localGitOptions.wslDistro)}\0${normalizedHost}` - const now = Date.now() - pruneHostAuthCache(now) - const cached = hostAuthCache.get(cacheKey) - if (cached && cached.expiresAt > now) { - return cached.authenticated - } - const inFlight = hostAuthInFlight.get(cacheKey) - if (inFlight) { - return inFlight - } - // Why: provider detection and review loading can probe the same runtime at - // once; coalesce them so one host never spawns duplicate auth subprocesses. - const probe = (async () => { - const execOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - } - let authenticated: boolean - try { - const { stdout, stderr } = await ghExecFileAsync(['auth', 'status'], execOptions) - authenticated = parseAuthStatus(`${stdout}\n${stderr}`).some( - (account) => account.host.toLowerCase() === normalizedHost - ) - } catch (error) { - const output = ghCommandOutput(error) - if (!output) { - // Indeterminate (gh missing / spawn failure) — do not cache so a later - // probe (gh installed, tunnel ready, token added) can recover. - return false - } - // gh exits non-zero when a host has a token problem but still prints the - // per-host status; treat the host as GitHub only when it is actually listed. - authenticated = parseAuthStatus(output).some( - (account) => account.host.toLowerCase() === normalizedHost - ) - } - hostAuthCache.set(cacheKey, { authenticated, expiresAt: Date.now() + HOST_AUTH_TTL_MS }) - pruneHostAuthCache(Date.now()) - return authenticated - })() - hostAuthInFlight.set(cacheKey, probe) - try { - return await probe - } finally { - if (hostAuthInFlight.get(cacheKey) === probe) { - hostAuthInFlight.delete(cacheKey) - } - } + return Boolean( + await resolveAuthenticatedGitHubHost(host, repoPath, connectionId, localGitOptions) + ) } /** Safely validate a project-selected host without giving the untrusted host @@ -158,7 +216,7 @@ export async function getEnterpriseGitHubRepoSlugForRemote( remoteName: string, connectionId?: string | null, options: HostedReviewExecutionOptions = {} -): Promise { +): Promise { const localGitOptions = getHostedReviewLocalGitOptions(options) const context = githubRepoContext(repoPath, connectionId, localGitOptions) let remoteUrl: string | null @@ -171,19 +229,24 @@ export async function getEnterpriseGitHubRepoSlugForRemote( if (!identity || identity.host === 'github.com') { return null } - const authenticated = await isGitHubHostAuthenticated( + const authenticatedHost = await resolveAuthenticatedGitHubHost( identity.host, repoPath, connectionId, localGitOptions ) - return authenticated ? { owner: identity.owner, repo: identity.repo, host: identity.host } : null + if (authenticatedHost === undefined) { + return undefined + } + return authenticatedHost + ? { owner: identity.owner, repo: identity.repo, host: authenticatedHost } + : null } export async function getEnterpriseGitHubRepoSlug( repoPath: string, connectionId?: string | null, options: HostedReviewExecutionOptions = {} -): Promise { +): Promise { return getEnterpriseGitHubRepoSlugForRemote(repoPath, 'origin', connectionId, options) } diff --git a/src/main/github/project-view.test.ts b/src/main/github/project-view.test.ts index f5f468b7d9b..1339c5c0fdc 100644 --- a/src/main/github/project-view.test.ts +++ b/src/main/github/project-view.test.ts @@ -68,6 +68,19 @@ describe('classifyProjectError', () => { it('classifies auth-required when gh is not signed in', () => { expect(classifyProjectError('gh auth login required', '').type).toBe('auth_required') }) + + it('pins Enterprise auth and scope remediation to the selected host', () => { + expect( + classifyProjectError('gh auth login required', '', 'github.acme.test').message + ).toContain('gh auth login --hostname github.acme.test') + expect( + classifyProjectError( + 'your token has not been granted the required scopes', + '', + 'github.acme.test' + ).message + ).toContain('gh auth refresh --hostname github.acme.test') + }) }) describe('isValidOwnerSlug', () => { @@ -247,6 +260,13 @@ describe('project view owner caches', () => { expect(_getProjectViewOwnerTypeForTests('owner-1')).toBe('user') }) + it('shares owner type probes between implicit and explicit github.com hosts', () => { + _rememberProjectViewOwnerTypeForTests('acme', 'organization') + + expect(_getProjectViewOwnerTypeForTests('acme', 'github.com')).toBe('organization') + expect(_getProjectViewCacheSizesForTests().ownerTypes).toBe(1) + }) + it('LRU-evicts old parent-field retry and warning probes', () => { for (let i = 0; i <= PROJECT_VIEW_OWNER_CACHE_MAX_ENTRIES; i++) { const scopeKey = `owner-${i}\u0000organization` diff --git a/src/main/github/project-view.ts b/src/main/github/project-view.ts index 00371d21dd0..2e53eff357c 100644 --- a/src/main/github/project-view.ts +++ b/src/main/github/project-view.ts @@ -126,15 +126,15 @@ const parentFieldProbeInFlight = new Map>() // Why: GHES owners are a separate namespace and capability surface from // github.com owners with the same login — scope cache keys by host so one -// host's probe result can't leak into another. Host-less keys keep the -// legacy shape so existing entries/tests are unaffected. +// host's probe result can't leak into another. Normalize github.com so +// host-less callers share the same probe state as explicitly pinned calls. function ownerScopeKey(owner: string, ownerType: GitHubProjectOwnerType, host?: string): string { const base = `${owner}\u0000${ownerType}` - return host ? `${base}\u0000${host}` : base + return `${base}\u0000${githubProjectHost(host)}` } function ownerTypeCacheKey(owner: string, host?: string): string { - return host ? `${owner}\u0000${host}` : owner + return `${owner}\u0000${githubProjectHost(host)}` } function rememberOwnerType( @@ -930,7 +930,7 @@ async function fetchItemsPageWithRaw(args: { if (execFailed) { return { ok: false, - error: classifyProjectError(stderr, stdout), + error: classifyProjectError(stderr, stdout, args.host), rawErrors: [], stderr } @@ -946,7 +946,7 @@ async function fetchItemsPageWithRaw(args: { if (execFailed && (!parsed.errors || parsed.errors.length === 0) && !parsed.data) { return { ok: false, - error: classifyProjectError(stderr, stdout), + error: classifyProjectError(stderr, stdout, args.host), rawErrors: [], stderr } @@ -954,7 +954,7 @@ async function fetchItemsPageWithRaw(args: { if (parsed.errors && parsed.errors.length > 0) { return { ok: false, - error: classifyProjectError(stderr, stdout), + error: classifyProjectError(stderr, stdout, args.host), rawErrors: parsed.errors, stderr } diff --git a/src/main/github/project-view/internals.ts b/src/main/github/project-view/internals.ts index 10e2c033dcf..982458cf58b 100644 --- a/src/main/github/project-view/internals.ts +++ b/src/main/github/project-view/internals.ts @@ -186,7 +186,7 @@ export async function runGraphql( if (parsed.errors && parsed.errors.length > 0) { return { ok: false, - error: classifyProjectError(stderr, stdout), + error: classifyProjectError(stderr, stdout, exec?.host), raw: { stderr, stdout } } } @@ -213,7 +213,7 @@ export async function runGraphql( const { stderr, stdout: maybeStdout } = extractExecError(err) return { ok: false, - error: classifyProjectError(stderr, maybeStdout), + error: classifyProjectError(stderr, maybeStdout, exec?.host), raw: { stderr, stdout: maybeStdout } } } finally { @@ -263,7 +263,7 @@ export async function runRest( } } catch (err) { const { stderr, stdout: maybeStdout } = extractExecError(err) - return { ok: false, error: classifyProjectError(stderr, maybeStdout) } + return { ok: false, error: classifyProjectError(stderr, maybeStdout, options?.host) } } finally { release() } diff --git a/src/main/github/project-view/mutations.ts b/src/main/github/project-view/mutations.ts index ac4b20ed580..453a695baec 100644 --- a/src/main/github/project-view/mutations.ts +++ b/src/main/github/project-view/mutations.ts @@ -251,7 +251,7 @@ export async function updateIssueBySlug( await ghExecFileAsync(stateArgs, { encoding: 'utf-8', ...githubHostExecOptions(args) }) } catch (err) { const { stderr, stdout } = extractExecError(err) - return { ok: false, error: classifyProjectError(stderr, stdout) } + return { ok: false, error: classifyProjectError(stderr, stdout, args.host) } } finally { release() } @@ -558,7 +558,7 @@ export async function listLabelsBySlug( } } catch (err) { const { stderr, stdout: maybeStdout } = extractExecError(err) - return { ok: false, error: classifyProjectError(stderr, maybeStdout) } + return { ok: false, error: classifyProjectError(stderr, maybeStdout, args.host) } } finally { release() } @@ -610,7 +610,7 @@ export async function listAssignableUsersBySlug( } } catch (err) { const { stderr } = extractExecError(err) - return { ok: false, error: classifyProjectError(stderr, '') } + return { ok: false, error: classifyProjectError(stderr, '', args.host) } } finally { release() } diff --git a/src/main/github/project-view/project-error-classification.ts b/src/main/github/project-view/project-error-classification.ts index f170abec515..1468552b345 100644 --- a/src/main/github/project-view/project-error-classification.ts +++ b/src/main/github/project-view/project-error-classification.ts @@ -2,6 +2,7 @@ // renderer can act on (auth vs scope vs rate limit vs drift), shared by the // project-view read and mutation paths. import type { GitHubProjectViewError } from '../../../shared/github-project-types' +import { githubProjectHost } from '../../../shared/github-project-identity' export type GhGraphqlErrorShape = { type?: string @@ -53,9 +54,14 @@ export function errorsIndicateParentField(errors: GhGraphqlErrorShape[], stderr: }) } -export function classifyProjectError(stderr: string, stdout: string): GitHubProjectViewError { +export function classifyProjectError( + stderr: string, + stdout: string, + host?: string +): GitHubProjectViewError { const errors = extractGraphqlErrors(stderr, stdout) const s = stderr.toLowerCase() + const selectedHost = githubProjectHost(host) // Auth if ( @@ -65,7 +71,7 @@ export function classifyProjectError(stderr: string, stdout: string): GitHubProj ) { return { type: 'auth_required', - message: 'Sign in to GitHub to load project tasks. Run `gh auth login`.' + message: `Sign in to GitHub to load project tasks. Run \`gh auth login --hostname ${selectedHost}\`.` } } // Scope @@ -76,8 +82,7 @@ export function classifyProjectError(stderr: string, stdout: string): GitHubProj ) { return { type: 'scope_missing', - message: - 'GitHub project access needs additional scopes. Run `gh auth refresh -s project -s read:org -s repo`.' + message: `GitHub project access needs additional scopes. Run \`gh auth refresh --hostname ${selectedHost} -s project -s read:org -s repo\`.` } } // Rate limit diff --git a/src/main/github/work-item-details-pr-files.test.ts b/src/main/github/work-item-details-pr-files.test.ts index 93c524ae888..fd596584ef2 100644 --- a/src/main/github/work-item-details-pr-files.test.ts +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -6,6 +6,7 @@ type RateLimitGuardResult = const { ghExecFileAsyncMock, + getEnterpriseGitHubRepoSlugMock, getOwnerRepoMock, getWorkItemMock, getPRChecksMock, @@ -14,6 +15,7 @@ const { repositoryRateLimitGuardMock } = vi.hoisted(() => ({ ghExecFileAsyncMock: vi.fn(), + getEnterpriseGitHubRepoSlugMock: vi.fn(), getOwnerRepoMock: vi.fn(), getWorkItemMock: vi.fn(), getPRChecksMock: vi.fn(), @@ -58,7 +60,7 @@ vi.mock('./client', () => ({ })) vi.mock('./github-enterprise-repository', () => ({ - getEnterpriseGitHubRepoSlug: vi.fn().mockResolvedValue(null), + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock, isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true) })) @@ -124,6 +126,8 @@ describe('getWorkItemDetails PR file listing', () => { ghExecFileAsyncMock.mockReset() getOwnerRepoMock.mockReset() getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getEnterpriseGitHubRepoSlugMock.mockReset() + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) getWorkItemMock.mockReset() getPRChecksMock.mockReset() getPRChecksMock.mockResolvedValue([]) @@ -223,6 +227,63 @@ describe('getWorkItemDetails PR file listing', () => { expect(details?.files).toEqual([]) }) + it('backfills the Enterprise origin host before a host-less PR detail fan-out', async () => { + const enterprise = { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' } + getWorkItemMock.mockResolvedValueOnce({ + ...pullRequestItem(8, 'Host-less Enterprise PR'), + author: '', + prRepo: { owner: 'team', repo: 'orca' } + }) + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(enterprise) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' + if (endpoint === 'repos/team/orca/pulls/8') { + return { stdout: JSON.stringify({ body: 'Enterprise body' }) } + } + if (endpoint === 'repos/team/orca/pulls/8/files?per_page=100') { + return { stdout: '[]' } + } + return auxiliaryPRResponse(args) + }) + + const details = await getWorkItemDetails('/remote/repo', 8, 'pr', 'ssh-1') + + expect(details?.item.prRepo).toEqual(enterprise) + expect(getPRCommentsMock).toHaveBeenCalledWith( + '/remote/repo', + 8, + { prRepo: enterprise }, + 'ssh-1' + ) + expect(getPRChecksMock).toHaveBeenCalledWith( + '/remote/repo', + 8, + undefined, + enterprise, + undefined, + 'ssh-1' + ) + expect( + ghExecFileAsyncMock.mock.calls.every(([, options]) => options?.host === enterprise.host) + ).toBe(true) + }) + + it('does not run bare PR detail commands when local host resolution fails', async () => { + getWorkItemMock.mockResolvedValueOnce({ + ...pullRequestItem(9, 'Unresolved PR'), + prRepo: { owner: 'team', repo: 'orca' } + }) + getOwnerRepoMock.mockResolvedValue(null) + + const details = await getWorkItemDetails('/repo-root', 9, 'pr') + + expect(details).toMatchObject({ body: '', comments: [], checks: [], filesUnavailable: true }) + expect(getPRCommentsMock).not.toHaveBeenCalled() + expect(getPRChecksMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + it('uses the selected upstream GitHub Enterprise repo for both PR file sides', async () => { const prRepo = { owner: 'team', diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 5a88d49d1c7..5e389be8581 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -22,7 +22,6 @@ import { import { getWorkItem, getPRChecks, getPRComments } from './client' import { getIssueGitHubApiRepository, - getOriginGitHubApiRepository, githubHostExecOptions, resolveGitHubRepoExecution, type GitHubApiRepository @@ -493,6 +492,11 @@ async function getPRMetadata( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ body: string; headSha?: string; baseSha?: string }> { + if (!ownerRepo) { + // Why: a bare `gh pr view` can honor ambient GH_HOST/GH_REPO after hosted + // repository resolution fails, returning metadata for the wrong PR. + return { body: '' } + } const ghOptions = { ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), ...githubHostExecOptions(ownerRepo) @@ -502,35 +506,19 @@ async function getPRMetadata( } try { noteRepositoryRateLimitSpend(ownerRepo, 'core', 1, ghOptions) - if (ownerRepo) { - const { stdout } = await ghExecFileAsync( - ['api', '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}`], - ghOptions - ) - const data = JSON.parse(stdout) as { - body?: string | null - head?: { sha?: string } - base?: { sha?: string } - } - return { - body: data.body ?? '', - ...(data.head?.sha ? { headSha: data.head.sha } : {}), - ...(data.base?.sha ? { baseSha: data.base.sha } : {}) - } - } const { stdout } = await ghExecFileAsync( - ['pr', 'view', String(prNumber), '--json', 'body,headRefOid,baseRefOid'], + ['api', '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}`], ghOptions ) const data = JSON.parse(stdout) as { - body?: string - headRefOid?: string - baseRefOid?: string + body?: string | null + head?: { sha?: string } + base?: { sha?: string } } return { body: data.body ?? '', - ...(data.headRefOid ? { headSha: data.headRefOid } : {}), - ...(data.baseRefOid ? { baseSha: data.baseRefOid } : {}) + ...(data.head?.sha ? { headSha: data.head.sha } : {}), + ...(data.base?.sha ? { baseSha: data.base.sha } : {}) } } catch { return { body: '' } @@ -869,6 +857,9 @@ async function getGitHubUsersByLogin( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + if (!repository) { + return [] + } const uniqueLogins = Array.from( new Set(logins.filter((login) => login && login !== 'ghost').map((login) => login.trim())) ).slice(0, 40) @@ -1022,6 +1013,9 @@ async function getPRChecksForDetails( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + if (!repository) { + return [] + } try { return await getPRChecks( repoPath, @@ -1069,8 +1063,8 @@ export async function getWorkItemDetails( const resolvedRepository = item.type === 'issue' ? await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) - : (item.prRepo ?? - (await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions))) + : (await resolveGitHubRepoExecution(repoPath, item.prRepo, connectionId, localGitOptions)) + .ownerRepo if (item.type === 'issue') { return withWorkItemDetailsPermit(async () => { @@ -1150,13 +1144,15 @@ export async function getWorkItemDetails( getWorkItemParticipants(repoPath, item, resolvedRepository, connectionId, localGitOptions) ) ]), - getPRComments( - repoPath, - item.number, - { prRepo: resolvedRepository }, - connectionId, - ...localGitOptionArgs(localGitOptions) - ) + resolvedRepository + ? getPRComments( + repoPath, + item.number, + { prRepo: resolvedRepository }, + connectionId, + ...localGitOptionArgs(localGitOptions) + ) + : Promise.resolve([]) ]) // Why: mention hydration spawns gh directly while checks owns a permit; bound them without nesting. @@ -1183,7 +1179,10 @@ export async function getWorkItemDetails( ]) return { - item: enrichItemDisplayAvatars(item, mentionParticipants), + item: enrichItemDisplayAvatars( + resolvedRepository ? { ...item, prRepo: resolvedRepository } : item, + mentionParticipants + ), body: metadata.body, comments, headSha: metadata.headSha, diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index 2af308acdf1..815c8dac7e4 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -187,6 +187,7 @@ import { GITHUB_PR_MERGE_METHOD_LABELS, resolveGitHubPRMergeMethods } from '../../../shared/github-pr-merge-methods' +import { githubProjectHost } from '../../../shared/github-project-identity' import { githubRepoIdentityKey } from '../../../shared/github-repository-identity-key' import { findGithubIssueWorkspaceAttachment, @@ -307,6 +308,25 @@ export type GitHubItemDialogProjectOrigin = { cacheKey: string } +// Why: every PR mutation needs the same host-pinned identity so process GH_HOST +// cannot redirect a github.com item or a Project row to the wrong server. +function resolvePullRequestRepo( + item: Pick, + projectOrigin?: Pick +): GitHubOwnerRepo | null { + const repo = + item.prRepo ?? + (projectOrigin + ? { + owner: projectOrigin.owner, + repo: projectOrigin.repo, + host: projectOrigin.host + } + : null) ?? + parseOwnerRepoFromItemUrl(item.url) + return repo ? { ...repo, host: githubProjectHost(repo.host) } : null +} + type GitHubItemDialogProps = { workItem: GitHubWorkItem | null repoPath: string | null @@ -687,12 +707,14 @@ function PRReviewersPanel({ loading, repoPath, sourceContext, + projectOrigin, onReviewersRequested }: { item: GitHubWorkItem loading: boolean repoPath: string | null sourceContext?: TaskSourceContext | null + projectOrigin?: GitHubItemDialogProjectOrigin onReviewersRequested: (reviewRequests: GitHubAssignableUser[]) => void }): React.JSX.Element { const [open, setOpen] = useState(false) @@ -792,20 +814,23 @@ function PRReviewersPanel({ return Array.from(byLogin.values()) }, [item.author, item.latestReviews, localReviewRequests]) - const reviewSlug = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const reviewRepo = useMemo( + () => resolvePullRequestRepo(item, projectOrigin), + [item, projectOrigin] + ) const reviewerMetadataBySlug = useRepoAssigneesBySlug( - open && reviewSlug ? reviewSlug.owner : null, - open && reviewSlug ? reviewSlug.repo : null, + open && reviewRepo ? reviewRepo.owner : null, + open && reviewRepo ? reviewRepo.repo : null, reviewerSeedUsers.map((user) => user.login), sourceSettings, - reviewSlug?.host + reviewRepo?.host ) const reviewerMetadataByPath = useRepoAssignees( - open && !reviewSlug ? repoPath : null, - open && !reviewSlug ? item.repoId : null, + open && !reviewRepo ? repoPath : null, + open && !reviewRepo ? item.repoId : null, sourceSettings ) - const reviewerMetadata = reviewSlug ? reviewerMetadataBySlug : reviewerMetadataByPath + const reviewerMetadata = reviewRepo ? reviewerMetadataBySlug : reviewerMetadataByPath const displayItem = { ...item, reviewRequests: localReviewRequests } const reviewers = getGitHubPRReviewerRows(displayItem) const authorLogin = item.author?.toLowerCase() ?? null @@ -939,7 +964,7 @@ function PRReviewersPanel({ repo: runtimeRepo, prNumber: item.number, reviewers: logins, - prRepo: item.prRepo ?? reviewSlug + prRepo: reviewRepo }, { timeoutMs: 30_000 } ) @@ -949,7 +974,7 @@ function PRReviewersPanel({ sourceContext, prNumber: item.number, reviewers: logins, - prRepo: item.prRepo ?? reviewSlug + prRepo: reviewRepo }) if (!reviewerPanelMountedRef.current) { return @@ -1036,7 +1061,7 @@ function PRReviewersPanel({ repo: runtimeRepo, prNumber: item.number, reviewers: logins, - prRepo: item.prRepo ?? reviewSlug + prRepo: reviewRepo }, { timeoutMs: 30_000 } ) @@ -1046,7 +1071,7 @@ function PRReviewersPanel({ sourceContext, prNumber: item.number, reviewers: logins, - prRepo: item.prRepo ?? reviewSlug + prRepo: reviewRepo }) if (!reviewerPanelMountedRef.current) { return @@ -3148,6 +3173,7 @@ function ConversationTab({ } const bodySlug = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const prRepo = useMemo(() => resolvePullRequestRepo(item, projectOrigin), [item, projectOrigin]) const markdownGitHubRepo = useMemo( () => (projectOrigin ? { owner: projectOrigin.owner, repo: projectOrigin.repo } : bodySlug), [bodySlug, projectOrigin] @@ -3221,7 +3247,7 @@ function ConversationTab({ repoId: item.repoId, sourceContext, prNumber: item.number, - prRepo: item.prRepo ?? parseOwnerRepoFromItemUrl(item.url), + prRepo, commentId: comment.id, body: replyBody, threadId: comment.threadId, @@ -3235,7 +3261,7 @@ function ConversationTab({ number: item.number, body: `@${comment.author} ${replyBody}`, type: item.type, - prRepo: item.prRepo ?? parseOwnerRepoFromItemUrl(item.url) + prRepo }) if (!result.ok) { @@ -3253,11 +3279,10 @@ function ConversationTab({ [ canUseRepoMutationContext, item.number, - item.prRepo, item.repoId, item.type, - item.url, onCommentAdded, + prRepo, repoPath, sourceContext ] @@ -3288,6 +3313,7 @@ function ConversationTab({ loading={loading} repoPath={repoPath} sourceContext={sourceContext} + projectOrigin={projectOrigin} onReviewersRequested={onReviewersRequested} />