From 215d42e597d2529ea2d67aa799cae662f9e8f291 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:54:36 -0700 Subject: [PATCH] Thread GitHub host through issue detail requests Incorporates ghes-issue-host-support (ed6bb96ef): one hosted issue repository identity is resolved before the details fan-out so comments, timeline, participants, and mention lookups cannot drift across hosts, with SSH guards so unresolved issue/PR repositories never fall through to gh's default host. Co-authored-by: Orca --- src/main/github/work-item-details.test.ts | 110 +++++++++++++++++++--- src/main/github/work-item-details.ts | 92 ++++++++++-------- 2 files changed, 149 insertions(+), 53 deletions(-) diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 3c39a252df5..87ca15355f7 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -7,8 +7,9 @@ type RateLimitGuardResult = const { ghExecFileAsyncMock, getOwnerRepoMock, - getIssueOwnerRepoMock, + getOwnerRepoForRemoteMock, getEnterpriseGitHubRepoSlugMock, + getEnterpriseGitHubRepoSlugForRemoteMock, getWorkItemMock, getWorkItemByOwnerRepoMock, getPRChecksMock, @@ -24,8 +25,9 @@ const { } = vi.hoisted(() => ({ ghExecFileAsyncMock: vi.fn(), getOwnerRepoMock: vi.fn(), - getIssueOwnerRepoMock: vi.fn(), + getOwnerRepoForRemoteMock: vi.fn(), getEnterpriseGitHubRepoSlugMock: vi.fn(), + getEnterpriseGitHubRepoSlugForRemoteMock: vi.fn(), getWorkItemMock: vi.fn(), getWorkItemByOwnerRepoMock: vi.fn(), getPRChecksMock: vi.fn(), @@ -51,7 +53,7 @@ const { vi.mock('./gh-utils', () => ({ ghExecFileAsync: ghExecFileAsyncMock, getOwnerRepo: getOwnerRepoMock, - getIssueOwnerRepo: getIssueOwnerRepoMock, + getOwnerRepoForRemote: getOwnerRepoForRemoteMock, ghRepoExecOptions: ghRepoExecOptionsMock, githubRepoContext: githubRepoContextMock, acquire: acquireMock, @@ -66,7 +68,8 @@ vi.mock('./client', () => ({ })) vi.mock('./github-enterprise-repository', () => ({ - getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock, + getEnterpriseGitHubRepoSlugForRemote: getEnterpriseGitHubRepoSlugForRemoteMock })) vi.mock('./rate-limit', () => ({ @@ -90,9 +93,11 @@ describe('getWorkItemDetails', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() getOwnerRepoMock.mockReset() - getIssueOwnerRepoMock.mockReset() + getOwnerRepoForRemoteMock.mockReset() getEnterpriseGitHubRepoSlugMock.mockReset() getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugForRemoteMock.mockReset() + getEnterpriseGitHubRepoSlugForRemoteMock.mockResolvedValue(null) getWorkItemMock.mockReset() getWorkItemByOwnerRepoMock.mockReset() getPRChecksMock.mockReset() @@ -122,7 +127,7 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'issue-author' }) - getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) const timelineEvents = [ { id: 101, @@ -307,7 +312,7 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'issue-author' }) - getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) const makeTimelineEvent = (page: number, index: number, event: string): string => JSON.stringify({ id: `${page}:${index}`, @@ -383,7 +388,7 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'issue-author' }) - getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) // Collapsed GraphQL throws → fallback path picks up. ghExecFileAsyncMock .mockRejectedValueOnce(new Error('GraphQL error')) @@ -404,12 +409,12 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', '--cache', '60s', 'repos/acme/widgets/issues/923'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['api', '--cache', '60s', 'repos/acme/widgets/issues/923/comments?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 4, @@ -421,7 +426,7 @@ describe('getWorkItemDetails', () => { '--jq', '.[] | @json' ], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(details?.body).toBe('Issue body') }) @@ -452,7 +457,7 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'issue-author' }) - getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockResolvedValueOnce({ stdout: JSON.stringify({ body: 'Issue body', assignees: [] }) }) .mockResolvedValueOnce({ stdout: '[]' }) @@ -480,7 +485,7 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'issue-author' }) - getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockResolvedValueOnce({ stdout: JSON.stringify({ @@ -501,11 +506,86 @@ describe('getWorkItemDetails', () => { const details = await getWorkItemDetails('/home/tester/widgets', 923, 'issue', 'ssh-test-1') expect(getWorkItemMock).toHaveBeenCalledWith('/home/tester/widgets', 923, 'issue', 'ssh-test-1') - expect(getIssueOwnerRepoMock).toHaveBeenCalledWith('/home/tester/widgets', 'ssh-test-1') - expect(ghExecFileAsyncMock.mock.calls[0][1]).toEqual({}) + expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith( + '/home/tester/widgets', + 'upstream', + 'ssh-test-1', + {} + ) + expect(ghExecFileAsyncMock.mock.calls[0][1]).toEqual({ host: 'github.com' }) expect(details?.body).toBe('Remote issue body') }) + it('uses the GitHub Enterprise host for SSH-backed issue work item details', async () => { + const enterpriseRepository = { + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + } + getWorkItemMock.mockResolvedValueOnce({ + id: 'issue:7', + type: 'issue', + number: 7, + title: 'Enterprise issue', + state: 'open', + url: 'https://github.acme-corp.com/team/orca/issues/7', + labels: [], + updatedAt: '2026-07-16T00:00:00Z', + author: 'issue-author' + }) + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(enterpriseRepository) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + if (query.includes('comments(first: 100)')) { + return { + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: 'Enterprise issue body', + assignees: { nodes: [] }, + participants: { nodes: [] }, + comments: { nodes: [] } + } + } + } + }) + } + } + const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' + if (endpoint === 'repos/team/orca/issues/7/timeline?per_page=100&page=1') { + return { stdout: '' } + } + throw new Error(`unexpected gh call: ${args.join(' ')}`) + }) + + const details = await getWorkItemDetails('/remote/repo', 7, 'issue', 'ssh-1') + + expect(details?.body).toBe('Enterprise issue body') + expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'issue', 'ssh-1') + expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith(enterpriseRepository, 'graphql', {}) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalled() + expect( + ghExecFileAsyncMock.mock.calls.every( + ([, options]) => options?.host === 'github.acme-corp.com' + ) + ).toBe(true) + }) + + it('does not query the default host when an SSH issue repository is unresolved', async () => { + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + + await expect(getWorkItemDetails('/remote/repo', 7, 'issue', 'ssh-1')).resolves.toBeNull() + + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + it('routes local WSL PR detail fan-out through the selected distro', async () => { const localGitOptions = { wslDistro: 'Ubuntu' } rateLimitGuardMock.mockReturnValue({ diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 1af2c90ad4b..7ed75c8772c 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -17,24 +17,19 @@ import { ghExecFileAsync, acquire, release, - getIssueOwnerRepo, ghRepoExecOptions, githubRepoContext, type LocalGitExecOptions } from './gh-utils' import { getWorkItem, getWorkItemByOwnerRepo, getPRChecks, getPRComments } from './client' import { + getIssueGitHubApiRepository, getOriginGitHubApiRepository, githubHostExecOptions, isGitHubDotComRepository, type GitHubApiRepository } from './github-api-repository' -import { - noteRateLimitSpend, - noteRepositoryRateLimitSpend, - rateLimitGuard, - repositoryRateLimitGuard -} from './rate-limit' +import { noteRepositoryRateLimitSpend, repositoryRateLimitGuard } from './rate-limit' import { getPRReviewCommentLineNumbersFromPatch } from './pr-review-comment-lines' import { isMaxBufferOverflowError } from '../git/max-buffer-overflow' @@ -344,6 +339,7 @@ async function getIssueTimelineItems( async function getIssueDetailsViaGraphQL( repoPath: string, issueNumber: number, + ownerRepo: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ @@ -357,20 +353,18 @@ async function getIssueDetailsViaGraphQL( participants: GitHubAssignableUser[] timelineItems: GitHubIssueTimelineItem[] } | null> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getIssueOwnerRepo( - repoPath, - connectionId, - ...localGitOptionArgs(localGitOptions) - ) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (!ownerRepo) { return null } - if (rateLimitGuard('graphql').blocked) { + if (repositoryRateLimitGuard(ownerRepo, 'graphql', localGitOptions).blocked) { return null } try { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, localGitOptions) const { stdout } = await ghExecFileAsync( [ 'api', @@ -711,6 +705,7 @@ function mergePRFileViewedStates( async function getIssueBodyAndComments( repoPath: string, issueNumber: number, + ownerRepo: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ @@ -719,12 +714,10 @@ async function getIssueBodyAndComments( assignees: string[] timelineItems: GitHubIssueTimelineItem[] }> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getIssueOwnerRepo( - repoPath, - connectionId, - ...localGitOptionArgs(localGitOptions) - ) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } try { if (ownerRepo) { const [issueResult, commentsResult, timelineItems] = await Promise.all([ @@ -773,6 +766,11 @@ async function getIssueBodyAndComments( const assignees = (issue.assignees ?? []).map((a) => a.login) return { body: issue.body ?? '', comments, assignees, timelineItems } } + if (connectionId) { + // Why: connection-backed gh has no cwd. A bare issue lookup could honor + // process GH_REPO/GH_HOST and return an unrelated repository's issue. + return { body: '', comments: [], assignees: [], timelineItems: [] } + } // Fallback: non-GitHub remote const { stdout } = await ghExecFileAsync( ['issue', 'view', String(issueNumber), '--json', 'body,comments,assignees'], @@ -812,13 +810,9 @@ async function getWorkItemParticipants( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - // Why: issues in a fork live on the upstream remote, so participants must be - // resolved via getIssueOwnerRepo to stay consistent with getIssueBodyAndComments. - // PRs remain tied to origin via getOwnerRepo. - const ownerRepo = - item.type === 'issue' - ? await getIssueOwnerRepo(repoPath, connectionId, ...localGitOptionArgs(localGitOptions)) - : resolvedRepository + // Why: one repository identity is resolved before the details fan-out so + // comments, participants, mentions, and PR files cannot drift across hosts. + const ownerRepo = resolvedRepository if (!ownerRepo) { return [] } @@ -1090,6 +1084,15 @@ export async function getWorkItemDetails( if (connectionId && type !== 'issue' && !originApiRepository) { return null } + const issueApiRepository = + type === 'issue' + ? await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) + : null + // Why: the same cwd-less fallthrough exists for issues — an unresolved issue + // repository over SSH must not surface a same-numbered github.com issue. + if (connectionId && type === 'issue' && !issueApiRepository) { + return null + } // Why: connection-backed repos have no local cwd for gh to inspect, so GHES // must use its host-qualified repository identity during the initial lookup. const useExplicitEnterpriseRepository = @@ -1125,6 +1128,12 @@ export async function getWorkItemDetails( return null } + const resolvedRepository = + item.type === 'issue' + ? (issueApiRepository ?? + (await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions))) + : (item.prRepo ?? originApiRepository) + await acquire() try { if (item.type === 'issue') { @@ -1138,6 +1147,7 @@ export async function getWorkItemDetails( const collapsed = await getIssueDetailsViaGraphQL( repoPath, item.number, + resolvedRepository, connectionId, localGitOptions ) @@ -1160,15 +1170,21 @@ export async function getWorkItemDetails( // Why: fall back to body/comments and GraphQL participants in parallel; // the mention-participant merge is a cheap local operation afterward. const [{ body, comments, assignees, timelineItems }, participants] = await Promise.all([ - getIssueBodyAndComments(repoPath, item.number, connectionId, localGitOptions), - getWorkItemParticipants(repoPath, item, null, connectionId, localGitOptions) + getIssueBodyAndComments( + repoPath, + item.number, + resolvedRepository, + connectionId, + localGitOptions + ), + getWorkItemParticipants(repoPath, item, resolvedRepository, connectionId, localGitOptions) ]) const mentionParticipants = await getMentionParticipants( repoPath, item, comments, participants, - null, + resolvedRepository, connectionId, localGitOptions ) @@ -1184,23 +1200,23 @@ export async function getWorkItemDetails( // PR: fetch metadata + comments + files + viewed states + participants in parallel. const [metadata, comments, files, viewedStates, participants] = await Promise.all([ - getPRMetadata(repoPath, item.number, originApiRepository, connectionId, localGitOptions), + getPRMetadata(repoPath, item.number, resolvedRepository, connectionId, localGitOptions), getPRComments( repoPath, item.number, - { prRepo: originApiRepository }, + { prRepo: resolvedRepository }, connectionId, ...localGitOptionArgs(localGitOptions) ), - getPRFiles(repoPath, item.number, originApiRepository, connectionId, localGitOptions), + getPRFiles(repoPath, item.number, resolvedRepository, connectionId, localGitOptions), getPRFileViewedStates( repoPath, item.number, - originApiRepository, + resolvedRepository, connectionId, localGitOptions ), - getWorkItemParticipants(repoPath, item, originApiRepository, connectionId, localGitOptions) + getWorkItemParticipants(repoPath, item, resolvedRepository, connectionId, localGitOptions) ]) // Why: run the mention-author GraphQL lookup in parallel with the final @@ -1212,7 +1228,7 @@ export async function getWorkItemDetails( item, comments, participants, - originApiRepository, + resolvedRepository, connectionId, localGitOptions ), @@ -1220,7 +1236,7 @@ export async function getWorkItemDetails( repoPath, item.number, metadata.headSha, - originApiRepository, + resolvedRepository, connectionId, localGitOptions )