From 3587da94b377e580e42d3871e2913fe54106d775 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:22:02 -0700 Subject: [PATCH] Make repository identity host-aware across all GitHub surfaces Generalize the auth-gated enterprise resolver to any remote and build a cached hosted-identity family (origin/issue/candidates/source) on top of it, then migrate every github.com-only consumer: Tasks listing/counting, branch-to-PR discovery, push targets, fork upstream, issue operations, Projects, web links, avatars, and PR-link facts. Scope the rate-limit breaker probe per runtime:host and classify WSL UNC cwds correctly. Co-authored-by: Orca --- src/main/git/gh-rate-limit-breaker.test.ts | 42 ++- src/main/git/gh-rate-limit-breaker.ts | 38 ++- .../git/runner-gh-rate-limit-breaker.test.ts | 37 ++- src/main/github/client-issue-source.test.ts | 32 +++ .../client-work-items-query-paging.test.ts | 267 ++++++++++++++++++ src/main/github/client-work-items.test.ts | 161 +++-------- src/main/github/client.test.ts | 72 ++++- src/main/github/client.ts | 110 +++++--- src/main/github/github-api-repository.ts | 161 ++++++++++- .../github/github-enterprise-repository.ts | 19 +- src/main/github/issues.test.ts | 21 ++ src/main/github/issues.ts | 35 ++- src/main/github/project-view.test.ts | 15 + src/main/github/project-view.ts | 159 +++++++---- src/main/github/project-view/internals.ts | 227 +++------------ .../github/project-view/mutations.test.ts | 83 +++++- src/main/github/project-view/mutations.ts | 112 +++++--- .../project-error-classification.ts | 172 +++++++++++ src/main/github/rate-limit.test.ts | 137 +++++++++ src/main/github/rate-limit.ts | 82 +++++- src/main/ipc/github.ts | 5 +- src/main/repo-icon-autodetect.test.ts | 3 +- src/main/runtime/orca-runtime.test.ts | 4 +- src/main/runtime/orca-runtime.ts | 7 +- src/main/runtime/rpc/methods/github.ts | 39 ++- .../source-control/forge-provider.test.ts | 13 +- src/main/source-control/forge-provider.ts | 25 +- ...hosted-review-creation-eligibility.test.ts | 17 +- .../hosted-review-creation.test.ts | 17 +- src/preload/api-types.ts | 5 +- src/preload/index.ts | 7 +- src/renderer/src/components/TaskPage.tsx | 15 +- .../parked-terminal-byte-watcher.test.ts | 2 +- .../terminal-pane/pty-connection.test.ts | 2 +- src/renderer/src/lib/github-links.test.ts | 28 +- src/shared/github-links.ts | 10 +- src/shared/github-project-types.ts | 21 ++ src/shared/repo-icon.ts | 5 +- .../terminal-github-pr-link-detector.test.ts | 21 +- 39 files changed, 1670 insertions(+), 558 deletions(-) create mode 100644 src/main/github/client-work-items-query-paging.test.ts create mode 100644 src/main/github/project-view/project-error-classification.ts diff --git a/src/main/git/gh-rate-limit-breaker.test.ts b/src/main/git/gh-rate-limit-breaker.test.ts index 8cd972ac776..3e136954522 100644 --- a/src/main/git/gh-rate-limit-breaker.test.ts +++ b/src/main/git/gh-rate-limit-breaker.test.ts @@ -7,6 +7,7 @@ import { isGhPrimaryRateLimitStderr, isGhRateLimitProbe, notifyGhPrimaryRateLimit, + parseGhRateLimitScopeKey, recordGhPrimaryRateLimit, registerGhRateLimitResetProbe } from './gh-rate-limit-breaker' @@ -59,6 +60,36 @@ describe('isGhPrimaryRateLimitStderr', () => { }) }) +describe('parseGhRateLimitScopeKey', () => { + it('parses native and wsl scopes, keeping ported hosts intact', () => { + expect(parseGhRateLimitScopeKey('native:github.com')).toEqual({ + runtime: 'native', + host: 'github.com' + }) + expect(parseGhRateLimitScopeKey('native:ghe.corp:8443')).toEqual({ + runtime: 'native', + host: 'ghe.corp:8443' + }) + expect(parseGhRateLimitScopeKey('wsl:ubuntu:github.com')).toEqual({ + runtime: 'wsl', + wslDistro: 'ubuntu', + host: 'github.com' + }) + expect(parseGhRateLimitScopeKey('wsl:ubuntu:ghe.corp:8443')).toEqual({ + runtime: 'wsl', + wslDistro: 'ubuntu', + host: 'ghe.corp:8443' + }) + }) + + it('rejects malformed scopes', () => { + expect(parseGhRateLimitScopeKey('native:')).toBeNull() + expect(parseGhRateLimitScopeKey('wsl:ubuntu')).toBeNull() + expect(parseGhRateLimitScopeKey('wsl:ubuntu:')).toBeNull() + expect(parseGhRateLimitScopeKey('ssh:github.com')).toBeNull() + }) +}) + describe('breaker state', () => { it('blocks until the recorded time, then expires', () => { const now = 1_000_000 @@ -90,17 +121,22 @@ describe('breaker state', () => { const probe = vi.fn() registerGhRateLimitResetProbe(probe) notifyGhPrimaryRateLimit('search') - expect(probe).toHaveBeenCalledWith('search') + expect(probe).toHaveBeenCalledWith('search', 'native:github.com') expect(getGhRateLimitBlockedUntilMs('search')).toBeGreaterThan(Date.now()) }) - it('does not refine a non-default scope with the native github.com probe', () => { + it('fires the reset probe with the tripping scope for non-default scopes', () => { + // Why: a WSL-only Windows install (or GHES) would otherwise stay on the + // blunt 5-minute fallback forever — the probe must learn which scope to + // query. const probe = vi.fn() registerGhRateLimitResetProbe(probe) notifyGhPrimaryRateLimit('core', 'native:github.acme-corp.com') + notifyGhPrimaryRateLimit('core', 'wsl:ubuntu:github.com') - expect(probe).not.toHaveBeenCalled() + expect(probe).toHaveBeenNthCalledWith(1, 'core', 'native:github.acme-corp.com') + expect(probe).toHaveBeenNthCalledWith(2, 'core', 'wsl:ubuntu:github.com') expect( getGhRateLimitBlockedUntilMs('core', Date.now(), 'native:github.acme-corp.com') ).not.toBeNull() diff --git a/src/main/git/gh-rate-limit-breaker.ts b/src/main/git/gh-rate-limit-breaker.ts index 85394f0d540..623555d8133 100644 --- a/src/main/git/gh-rate-limit-breaker.ts +++ b/src/main/git/gh-rate-limit-breaker.ts @@ -31,9 +31,34 @@ export function ghRateLimitScopeKey(runtime: string, host: string): string { return `${runtime}:${host.toLowerCase()}` } +export type GhRateLimitScopeParts = + | { runtime: 'native'; host: string } + | { runtime: 'wsl'; wslDistro: string; host: string } + +// Why: the reset probe must spawn gh against the tripping scope's runtime and +// host. Parsing lives next to the builder so the key format stays private to +// this module instead of being string-split at call sites. +export function parseGhRateLimitScopeKey(scope: string): GhRateLimitScopeParts | null { + if (scope.startsWith('native:')) { + const host = scope.slice('native:'.length) + return host ? { runtime: 'native', host } : null + } + if (scope.startsWith('wsl:')) { + // Hosts may carry a port (host:port), so only the first colon after the + // distro is a separator. + const rest = scope.slice('wsl:'.length) + const sep = rest.indexOf(':') + if (sep <= 0 || sep === rest.length - 1) { + return null + } + return { runtime: 'wsl', wslDistro: rest.slice(0, sep), host: rest.slice(sep + 1) } + } + return null +} + const DEFAULT_SCOPE = ghRateLimitScopeKey('native', 'github.com') const blockedUntilMsByScopeAndBucket = new Map() -let resetProbe: ((bucket: GhRateLimitBucket) => void) | null = null +let resetProbe: ((bucket: GhRateLimitBucket, scope: string) => void) | null = null function breakerKey(bucket: GhRateLimitBucket, scope = DEFAULT_SCOPE): string { return `${scope}\0${bucket}` @@ -148,7 +173,7 @@ export function getGhRateLimitBlockedUntilMs( /** Register the (single) callback that refreshes precise reset times after a * breaker trip. Registered by the github rate-limit prober at module load. */ export function registerGhRateLimitResetProbe( - probe: ((bucket: GhRateLimitBucket) => void) | null + probe: ((bucket: GhRateLimitBucket, scope: string) => void) | null ): void { resetProbe = probe } @@ -156,13 +181,10 @@ export function registerGhRateLimitResetProbe( /** Called by the runner when a gh spawn came back with a primary 403. */ export function notifyGhPrimaryRateLimit(bucket: GhRateLimitBucket, scope = DEFAULT_SCOPE): void { recordGhPrimaryRateLimit(bucket, Date.now() + FALLBACK_BLOCK_MS[bucket], scope) - // The registered probe uses native github.com credentials; it cannot refine - // a GHES or WSL bucket without querying the wrong account/runtime. - if (scope !== DEFAULT_SCOPE) { - return - } + // The probe receives the tripping scope so it can run gh against that + // runtime/host and refine the fallback block into the real reset time. try { - resetProbe?.(bucket) + resetProbe?.(bucket, scope) } catch { // best-effort refinement; the fallback block stands } 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 6d089971b3c..a887b70ac61 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -13,7 +13,11 @@ vi.mock('child_process', () => ({ })) import { ghExecFileAsync } from './runner' -import { _resetGhRateLimitBreaker } from './gh-rate-limit-breaker' +import { + _resetGhRateLimitBreaker, + getGhRateLimitBlockedUntilMs, + registerGhRateLimitResetProbe +} from './gh-rate-limit-breaker' const PRIMARY_RATE_LIMIT_STDERR = 'gh: API rate limit exceeded for user ID 1775218. Please wait. (HTTP 403)' @@ -104,6 +108,37 @@ describe('ghExecFileAsync rate-limit breaker', () => { expect(execFileMock).toHaveBeenCalledTimes(5) }) + 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 + // the reset probe must be told which scope tripped. + const probe = vi.fn() + registerGhRateLimitResetProbe(probe) + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) + await expect( + ghExecFileAsync(['api', 'repos/a/b/pulls'], { + cwd: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo' + }) + ).rejects.toThrow() + + expect(probe).toHaveBeenCalledWith('core', 'wsl:ubuntu:github.com') + expect( + getGhRateLimitBlockedUntilMs('core', Date.now(), 'wsl:ubuntu:github.com') + ).not.toBeNull() + expect(getGhRateLimitBlockedUntilMs('core', Date.now())).toBeNull() + + mockGhSuccess('[]') + await expect(ghExecFileAsync(['api', 'repos/a/b/pulls'])).resolves.toMatchObject({ + stdout: '[]' + }) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + it('never blocks the exempt rate_limit probe', async () => { mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) await expect(ghExecFileAsync(['api', 'repos/a/b/pulls'])).rejects.toThrow() diff --git a/src/main/github/client-issue-source.test.ts b/src/main/github/client-issue-source.test.ts index 904f579cfd2..fd0b11d827a 100644 --- a/src/main/github/client-issue-source.test.ts +++ b/src/main/github/client-issue-source.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' import type * as GhUtils from './gh-utils' const { @@ -50,6 +51,37 @@ vi.mock('./rate-limit', () => ({ spendsSharedGitHubComQuota: () => true })) +vi.mock('./github-api-repository', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Why: these suites drive source resolution through the legacy gh-utils + // mocks; bridge the hosted seams onto the same mocks. + resolveIssueGitHubApiRepositorySource: ( + repoPath: string, + preference: unknown, + connectionId?: string | null, + localGitOptions?: unknown + ) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions), + getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) => + getIssueOwnerRepoMock(repoPath, connectionId), + getOriginGitHubApiRepository: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => getOwnerRepoMock(repoPath, connectionId, localGitOptions), + getGitHubApiRepositoryForRemote: ( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => + remoteName === 'origin' + ? getOwnerRepoMock(repoPath, connectionId, localGitOptions) + : getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId, localGitOptions) + } +}) + import { countWorkItems, getWorkItem, listWorkItems, _resetOwnerRepoCache } from './client' const PR_LIST_FIELDS = diff --git a/src/main/github/client-work-items-query-paging.test.ts b/src/main/github/client-work-items-query-paging.test.ts new file mode 100644 index 00000000000..df95e0cbebb --- /dev/null +++ b/src/main/github/client-work-items-query-paging.test.ts @@ -0,0 +1,267 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' + +const { + execFileAsyncMock, + ghExecFileAsyncMock, + getOwnerRepoMock, + getIssueOwnerRepoMock, + getOwnerRepoForRemoteMock, + resolveIssueSourceMock, + gitExecFileAsyncMock, + rateLimitGuardMock, + noteRateLimitSpendMock, + acquireMock, + releaseMock +} = vi.hoisted(() => ({ + execFileAsyncMock: vi.fn(), + ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + getIssueOwnerRepoMock: vi.fn(), + getOwnerRepoForRemoteMock: vi.fn(), + resolveIssueSourceMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), + rateLimitGuardMock: vi.fn((_bucket?: unknown) => ({ blocked: false })), + noteRateLimitSpendMock: vi.fn(), + acquireMock: vi.fn(), + releaseMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + execFileAsync: execFileAsyncMock, + ghExecFileAsync: ghExecFileAsyncMock, + githubRepoContext: ( + repoPath: string, + connectionId?: string | null, + localGitOptions: { wslDistro?: string } = {} + ) => ({ + repoPath, + connectionId: connectionId ?? null, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + }), + ghRepoExecOptions: (context: { + repoPath: string + connectionId?: string | null + wslDistro?: string + }) => + context.connectionId + ? {} + : { cwd: context.repoPath, ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) }, + getOwnerRepo: getOwnerRepoMock, + getIssueOwnerRepo: getIssueOwnerRepoMock, + getOwnerRepoForRemote: getOwnerRepoForRemoteMock, + resolveIssueSource: resolveIssueSourceMock, + acquire: acquireMock, + release: releaseMock, + _resetOwnerRepoCache: vi.fn(), + classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }), + classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr }) +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('./rate-limit', () => ({ + rateLimitGuard: rateLimitGuardMock, + noteRateLimitSpend: noteRateLimitSpendMock, + getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })), + // Mirror production: shared-scope calls delegate to the global guard/spend. + repositoryRateLimitGuard: vi.fn((_repo: unknown, bucket: string) => rateLimitGuardMock(bucket)), + noteRepositoryRateLimitSpend: vi.fn((_repo: unknown, bucket: string, cost?: number) => + noteRateLimitSpendMock(bucket, cost) + ), + spendsSharedGitHubComQuota: () => true +})) + +vi.mock('./github-api-repository', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Why: these suites drive source resolution through the legacy gh-utils + // mocks; bridge the hosted seams onto the same mocks. + resolveIssueGitHubApiRepositorySource: ( + repoPath: string, + preference: unknown, + connectionId?: string | null, + localGitOptions?: unknown + ) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions), + getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) => + getIssueOwnerRepoMock(repoPath, connectionId), + getOriginGitHubApiRepository: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => getOwnerRepoMock(repoPath, connectionId, localGitOptions), + getGitHubApiRepositoryForRemote: ( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => + remoteName === 'origin' + ? getOwnerRepoMock(repoPath, connectionId, localGitOptions) + : getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId, localGitOptions) + } +}) + +import { + countWorkItems, + listWorkItems, + _resetMergeQueueCacheForTests, + _resetOwnerRepoCache +} from './client' +import { _resetGhCwdRepoNegativeCache } from './gh-cwd-repo-negative-cache' +import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from '../../shared/github-work-items-query-bounds' + +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) +// Why: split from client-work-items.test.ts to keep both suites under the +// max-lines cap; this file owns query/paging request-shaping cases. +describe('listWorkItems query paging', () => { + beforeEach(() => { + execFileAsyncMock.mockReset() + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getIssueOwnerRepoMock.mockReset() + getOwnerRepoForRemoteMock.mockReset() + resolveIssueSourceMock.mockReset() + gitExecFileAsyncMock.mockReset() + rateLimitGuardMock.mockReset() + rateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRateLimitSpendMock.mockReset() + acquireMock.mockReset() + releaseMock.mockReset() + acquireMock.mockResolvedValue(undefined) + // Why: preference-aware `listWorkItems` calls `resolveIssueSource`. + // Route through the same `getIssueOwnerRepoMock` so existing tests that + // only set up `getIssueOwnerRepoMock` continue to work. + resolveIssueSourceMock.mockImplementation(async () => ({ + source: await getIssueOwnerRepoMock(), + fellBack: false + })) + getOwnerRepoForRemoteMock.mockResolvedValue(null) + _resetOwnerRepoCache() + _resetMergeQueueCacheForTests() + _resetGhCwdRepoNegativeCache() + }) + + it('returns zero for oversized count queries before resolving repo sources', async () => { + const secret = 'main-github-work-items-secret' + const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES) + + await expect(countWorkItems('/repo-root', oversizedQuery)).resolves.toBe(0) + + expect(resolveIssueSourceMock).not.toHaveBeenCalled() + expect(getIssueOwnerRepoMock).not.toHaveBeenCalled() + expect(getOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(acquireMock).not.toHaveBeenCalled() + expect(releaseMock).not.toHaveBeenCalled() + }) + + it('passes review-requested as a --search qualifier (gh CLI has no dedicated flag)', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) + + await listWorkItems('/repo-root', 10, 'review-requested:@me is:open') + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--search', 'is:pr is:open review-requested:@me sort:created-desc']), + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).not.toHaveBeenCalledWith( + expect.arrayContaining(['--review-requested']), + expect.anything() + ) + }) + + it('uses the requested numbered Search API page for issues', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) + + await listWorkItems('/repo-root', 10, 'is:issue is:open', 2) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'api', + '--cache', + '120s', + `search/issues?q=${encodeURIComponent('repo:acme/widgets is:issue is:open')}&sort=created&order=desc&per_page=10&page=2`, + '--jq', + '.items' + ], + { cwd: '/repo-root' } + ) + }) + + it('fetches and slices stable PR results for the requested numbered page', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify( + [1, 2, 3, 4].map((number) => ({ + number, + title: `PR ${number}`, + state: 'OPEN', + url: `https://github.com/acme/widgets/pull/${number}`, + labels: [], + updatedAt: `2026-07-0${number}T00:00:00Z`, + author: { login: 'octocat' }, + isDraft: false, + headRefName: `feature/${number}`, + headRefOid: `head-${number}`, + baseRefName: 'main' + })) + ) + }) + + const { items } = await listWorkItems('/repo-root', 2, 'is:pr is:open', 2) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--limit', '4', '--search', 'is:pr is:open sort:created-desc']), + { cwd: '/repo-root' } + ) + expect(items.map((item) => item.number)).toEqual([4, 3]) + }) + + it('filters pull request rows out of issue Search API results', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 2, + title: 'PR-shaped search row', + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/2', + labels: [], + updated_at: '2026-07-02T00:00:00Z', + user: { login: 'octocat' }, + pull_request: { url: 'https://api.github.com/repos/acme/widgets/pulls/2' } + }, + { + number: 1, + title: 'Issue row', + state: 'open', + html_url: 'https://github.com/acme/widgets/issues/1', + labels: [], + updated_at: '2026-07-01T00:00:00Z', + user: { login: 'octocat' } + } + ]) + }) + + const { items } = await listWorkItems('/repo-root', 10, 'is:issue is:open') + + expect(items.map((item) => item.id)).toEqual(['issue:1']) + }) +}) diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index 35191c3be20..a32b2e0eda8 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' const { execFileAsyncMock, @@ -73,6 +74,37 @@ vi.mock('./rate-limit', () => ({ spendsSharedGitHubComQuota: () => true })) +vi.mock('./github-api-repository', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Why: these suites drive source resolution through the legacy gh-utils + // mocks; bridge the hosted seams onto the same mocks. + resolveIssueGitHubApiRepositorySource: ( + repoPath: string, + preference: unknown, + connectionId?: string | null, + localGitOptions?: unknown + ) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions), + getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) => + getIssueOwnerRepoMock(repoPath, connectionId), + getOriginGitHubApiRepository: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => getOwnerRepoMock(repoPath, connectionId, localGitOptions), + getGitHubApiRepositoryForRemote: ( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => + remoteName === 'origin' + ? getOwnerRepoMock(repoPath, connectionId, localGitOptions) + : getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId, localGitOptions) + } +}) + import { countWorkItems, listWorkItems, @@ -118,6 +150,21 @@ describe('listWorkItems', () => { _resetGhCwdRepoNegativeCache() }) + it('routes GHES work-item listing through the Enterprise host', async () => { + const ghes = { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' } + getIssueOwnerRepoMock.mockResolvedValue(ghes) + getOwnerRepoMock.mockResolvedValue(ghes) + ghExecFileAsyncMock.mockResolvedValue({ stdout: '[]' }) + + await listWorkItems('/repo-root', 10) + + const calls = ghExecFileAsyncMock.mock.calls + expect(calls.length).toBeGreaterThan(0) + // Why: every list spawn must pin options.host so the runner targets the + // Enterprise server instead of gh's default host. + expect(calls.every(([, options]) => options?.host === 'github.acme-corp.com')).toBe(true) + }) + it('stops re-spawning gh for a repo whose cwd resolution already failed with no remotes', async () => { getIssueOwnerRepoMock.mockResolvedValue(null) getOwnerRepoMock.mockResolvedValue(null) @@ -594,120 +641,6 @@ describe('listWorkItems', () => { expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) - it('returns zero for oversized count queries before resolving repo sources', async () => { - const secret = 'main-github-work-items-secret' - const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES) - - await expect(countWorkItems('/repo-root', oversizedQuery)).resolves.toBe(0) - - expect(resolveIssueSourceMock).not.toHaveBeenCalled() - expect(getIssueOwnerRepoMock).not.toHaveBeenCalled() - expect(getOwnerRepoMock).not.toHaveBeenCalled() - expect(ghExecFileAsyncMock).not.toHaveBeenCalled() - expect(acquireMock).not.toHaveBeenCalled() - expect(releaseMock).not.toHaveBeenCalled() - }) - - it('passes review-requested as a --search qualifier (gh CLI has no dedicated flag)', async () => { - getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) - - await listWorkItems('/repo-root', 10, 'review-requested:@me is:open') - - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - expect.arrayContaining(['--search', 'is:pr is:open review-requested:@me sort:created-desc']), - { cwd: '/repo-root' } - ) - expect(ghExecFileAsyncMock).not.toHaveBeenCalledWith( - expect.arrayContaining(['--review-requested']), - expect.anything() - ) - }) - - it('uses the requested numbered Search API page for issues', async () => { - getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) - - await listWorkItems('/repo-root', 10, 'is:issue is:open', 2) - - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - [ - 'api', - '--cache', - '120s', - `search/issues?q=${encodeURIComponent('repo:acme/widgets is:issue is:open')}&sort=created&order=desc&per_page=10&page=2`, - '--jq', - '.items' - ], - { cwd: '/repo-root' } - ) - }) - - it('fetches and slices stable PR results for the requested numbered page', async () => { - getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify( - [1, 2, 3, 4].map((number) => ({ - number, - title: `PR ${number}`, - state: 'OPEN', - url: `https://github.com/acme/widgets/pull/${number}`, - labels: [], - updatedAt: `2026-07-0${number}T00:00:00Z`, - author: { login: 'octocat' }, - isDraft: false, - headRefName: `feature/${number}`, - headRefOid: `head-${number}`, - baseRefName: 'main' - })) - ) - }) - - const { items } = await listWorkItems('/repo-root', 2, 'is:pr is:open', 2) - - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - expect.arrayContaining(['--limit', '4', '--search', 'is:pr is:open sort:created-desc']), - { cwd: '/repo-root' } - ) - expect(items.map((item) => item.number)).toEqual([4, 3]) - }) - - it('filters pull request rows out of issue Search API results', async () => { - getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 2, - title: 'PR-shaped search row', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/2', - labels: [], - updated_at: '2026-07-02T00:00:00Z', - user: { login: 'octocat' }, - pull_request: { url: 'https://api.github.com/repos/acme/widgets/pulls/2' } - }, - { - number: 1, - title: 'Issue row', - state: 'open', - html_url: 'https://github.com/acme/widgets/issues/1', - labels: [], - updated_at: '2026-07-01T00:00:00Z', - user: { login: 'octocat' } - } - ]) - }) - - const { items } = await listWorkItems('/repo-root', 10, 'is:issue is:open') - - expect(items.map((item) => item.id)).toEqual(['issue:1']) - }) - it('returns open issues and PRs for the all-open preset query', async () => { getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 86d12c075ee..0246b45e60e 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -1,5 +1,6 @@ /* oxlint-disable max-lines -- Why: GitHub client fixtures cover local and SSH repo identity paths in one suite so mocked CLI behavior stays consistent. */ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' type RateLimitGuardResult = | { blocked: false } @@ -111,6 +112,38 @@ vi.mock('./rate-limit', () => ({ spendsSharedGitHubComQuota: () => true })) +vi.mock('./github-api-repository', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Why: these suites inject repo identities through the legacy gh-utils + // mocks; bridge the hosted seams onto the same mocks so per-test setups + // keep driving resolution without real enterprise probes. + resolveGitHubApiRepositoryCandidates: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => resolvePRRepositoryCandidatesMock(repoPath, connectionId, localGitOptions), + getGitHubApiRepositoryForRemote: ( + repoPath: string, + remoteName: string, + connectionId?: string | null + ) => getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId), + getOriginGitHubApiRepository: async (repoPath: string, connectionId?: string | null) => { + const slug = await getOwnerRepoMock(repoPath, connectionId) + // Mirror production: dotcom origin slugs come back pinned to github.com. + return slug ? { host: 'github.com', ...slug } : slug + }, + getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) => + getIssueOwnerRepoMock(repoPath, connectionId), + resolveIssueGitHubApiRepositorySource: async ( + repoPath: string, + _preference: unknown, + connectionId?: string | null + ) => ({ source: await getIssueOwnerRepoMock(repoPath, connectionId), fellBack: false }) + } +}) + import { checkOrcaStarred, getPRComments, @@ -3520,7 +3553,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['repo', 'view', 'StablyAI/Orca', '--json', 'isFork,parent'], - { cwd: '/repo-root', timeout: 10_000 } + { cwd: '/repo-root', host: 'github.com', timeout: 10_000 } ) }) @@ -3545,10 +3578,45 @@ describe('getPRForBranch', () => { await expect(getRepoUpstream('/repo-root')).resolves.toEqual({ owner: 'stablyai', - repo: 'orca' + repo: 'orca', + // Why: fork parents live on the same server as the fork's origin. + host: 'github.com' }) }) + it('routes GHES push-target probes through the Enterprise host', async () => { + const ghes = { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' } + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [ghes], + headRepo: ghes + }) + getOwnerRepoForRemoteMock.mockResolvedValueOnce(ghes) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + head: { + ref: 'feature', + repo: { + full_name: 'team/orca', + name: 'orca', + clone_url: 'https://github.acme-corp.com/team/orca.git', + ssh_url: 'git@github.acme-corp.com:team/orca.git', + owner: { login: 'team' } + } + } + }) + }) + + await expect(getPullRequestPushTarget('/repo-root', 7)).resolves.toEqual({ + pushTarget: { remoteName: 'origin', branchName: 'feature' } + }) + // Why: the candidate probe must pin options.host so the runner targets the + // Enterprise server instead of gh's default host. + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['api', 'repos/team/orca/pulls/7'], + expect.objectContaining({ host: 'github.acme-corp.com' }) + ) + }) + it('probes additional PR repo candidates when the first lookup is not found', async () => { resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ candidates: [ diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 5e31b44dfb9..657c69e5e9b 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -49,11 +49,6 @@ import { extractExecError, acquire, release, - getOwnerRepo, - getIssueOwnerRepo, - getOwnerRepoForRemote, - resolvePRRepositoryCandidates, - resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, @@ -80,10 +75,15 @@ import { } from './gh-cwd-repo-negative-cache' import type { GitHubRepoContext } from './github-repository-identity' import { + getGitHubApiRepositoryForRemote, + getIssueGitHubApiRepository, getOriginGitHubApiRepository, githubHostExecOptions, + githubRepositorySlugArg, githubRepositoryWebHost, resolveGitHubApiRepository, + resolveGitHubApiRepositoryCandidates, + resolveIssueGitHubApiRepositorySource, type GitHubApiRepository } from './github-api-repository' import { githubRepoIdentityKey } from '../../shared/github-repository-identity-key' @@ -329,7 +329,7 @@ export async function getPullRequestPushTarget( ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) const ghOptions = ghRepoExecOptions(context) - const { candidates } = await resolvePRRepositoryCandidates( + const { candidates } = await resolveGitHubApiRepositoryCandidates( repoPath, connectionId, localGitOptions @@ -345,9 +345,7 @@ export async function getPullRequestPushTarget( try { const { stdout } = await ghExecFileAsync( ['api', `repos/${candidate.owner}/${candidate.repo}/pulls/${prNumber}`], - { - ...ghOptions - } + { ...ghOptions, ...githubHostExecOptions(candidate) } ) prStdout = stdout break @@ -364,7 +362,12 @@ export async function getPullRequestPushTarget( if (!prStdout) { return null } - const origin = await getOwnerRepoForRemote(repoPath, 'origin', connectionId, localGitOptions) + const origin = await getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ) const pr = JSON.parse(prStdout) as { maintainer_can_modify?: boolean head?: { @@ -1123,8 +1126,8 @@ async function resolvePrWorkItemSource( localGitOptions: LocalGitExecOptions = {} ): Promise { const [originCandidate, upstreamCandidate] = await Promise.all([ - getOwnerRepo(repoPath, connectionId, localGitOptions), - getOwnerRepoForRemote(repoPath, 'upstream', connectionId, localGitOptions) + getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions), + getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) ]) const source = preference === 'upstream' ? (upstreamCandidate ?? originCandidate) : originCandidate @@ -1195,12 +1198,15 @@ async function listRecentWorkItems( // side, matching the parent design doc's partial-failure rule (§2). const [issuesSettled, prsSettled] = await Promise.allSettled([ issueOwnerRepo - ? ghExecFileAsync(issueRequest.args, ghOptions) + ? ghExecFileAsync(issueRequest.args, { + ...ghOptions, + ...githubHostExecOptions(issueOwnerRepo) + }) : requiresExplicitRepo ? Promise.resolve({ stdout: '[]' }) : ghCwdResolvedExec(repoContext, issueRequest.args, ghOptions), prOwnerRepo - ? ghExecFileAsync(prRequest.args, ghOptions) + ? ghExecFileAsync(prRequest.args, { ...ghOptions, ...githubHostExecOptions(prOwnerRepo) }) : requiresExplicitRepo ? Promise.resolve({ stdout: '[]' }) : ghCwdResolvedExec(repoContext, prRequest.args, ghOptions) @@ -1228,7 +1234,10 @@ async function listRecentWorkItems( prs = (JSON.parse(prsSettled.value.stdout) as Record[]) .slice(prRequest.offset, prRequest.offset + limit) .map((item) => mapPullRequestWorkItem(item, prOwnerRepo)) - prs = await hydrateWorkItemRepositoryMergeMetadata(prs, prOwnerRepo, ghOptions) + prs = await hydrateWorkItemRepositoryMergeMetadata(prs, prOwnerRepo, { + ...ghOptions, + ...githubHostExecOptions(prOwnerRepo) + }) } else { // Why: PR-side failures must preserve the pre-diff behavior of // Promise.all by re-throwing so the rejection propagates up through @@ -1320,7 +1329,10 @@ async function listQueriedWorkItems( }) try { const { stdout } = issueOwnerRepo - ? await ghExecFileAsync(request.args, ghOptions) + ? await ghExecFileAsync(request.args, { + ...ghOptions, + ...githubHostExecOptions(issueOwnerRepo) + }) : await ghCwdResolvedExec(repoContext, request.args, ghOptions) return { items: (JSON.parse(stdout) as Record[]) @@ -1349,12 +1361,18 @@ async function listQueriedWorkItems( }) try { const { stdout } = prOwnerRepo - ? await ghExecFileAsync(request.args, ghOptions) + ? await ghExecFileAsync(request.args, { + ...ghOptions, + ...githubHostExecOptions(prOwnerRepo) + }) : await ghCwdResolvedExec(repoContext, request.args, ghOptions) const mapped = (JSON.parse(stdout) as Record[]) .slice(request.offset, request.offset + limit) .map((item) => mapPullRequestWorkItem(item, prOwnerRepo)) - const hydrated = await hydrateWorkItemRepositoryMergeMetadata(mapped, prOwnerRepo, ghOptions) + const hydrated = await hydrateWorkItemRepositoryMergeMetadata(mapped, prOwnerRepo, { + ...ghOptions, + ...githubHostExecOptions(prOwnerRepo) + }) if (query.state === 'closed') { return hydrated.filter((item) => item.state !== 'merged') } @@ -1396,7 +1414,7 @@ export async function listWorkItems( } } const [issueResolved, prResolved] = await Promise.all([ - resolveIssueSource(repoPath, preference, connectionId, localGitOptions), + resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) ]) const issueOwnerRepo = issueResolved.source @@ -1504,7 +1522,10 @@ async function countWorkItemsForQuery( localGitOptions: LocalGitExecOptions = {} ): Promise { const searchQ = buildSearchQueryString(ownerRepo, query) - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } const { stdout } = await ghExecFileAsync( [ 'api', @@ -1561,7 +1582,7 @@ export async function countWorkItems( return 0 } const [issueResolved, prResolved] = await Promise.all([ - resolveIssueSource(repoPath, preference, connectionId, localGitOptions), + resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) ]) const issueOwnerRepo = issueResolved.source @@ -1658,8 +1679,12 @@ export async function getRepoSlug( repoPath: string, connectionId?: string | null, options: HostedReviewExecutionOptions = {} -): Promise<{ owner: string; repo: string } | null> { - return getOwnerRepo(repoPath, connectionId, ...hostedReviewLocalGitOptionArgs(options)) +): Promise { + return getOriginGitHubApiRepository( + repoPath, + connectionId, + getHostedReviewLocalGitOptions(options) + ) } /** @@ -1677,27 +1702,30 @@ export async function getRepoUpstream( ): Promise { const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} - const origin = await getOwnerRepo(repoPath, connectionId, ...localGitArgs) + const origin = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (!origin) { return null } - const upstreamRemote = await getOwnerRepoForRemote( + const upstreamRemote = await getGitHubApiRepositoryForRemote( repoPath, 'upstream', connectionId, - ...localGitArgs + localGitOptions ) if (upstreamRemote && !sameOwnerRepo(upstreamRemote, origin)) { return upstreamRemote } await acquire() try { + // Why: positional slugs bypass the runner's --repo qualifier, so the slug + // itself must carry the Enterprise host. const { stdout } = await ghExecFileAsync( - ['repo', 'view', `${origin.owner}/${origin.repo}`, '--json', 'isFork,parent'], + ['repo', 'view', githubRepositorySlugArg(origin), '--json', 'isFork,parent'], // Why: best-effort fork lookup runs at add-time; cap latency so a stalled // gh process can't hold up repo creation. { ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(origin), timeout: 10_000 } ) @@ -1707,7 +1735,10 @@ export async function getRepoUpstream( } const owner = data.parent?.owner?.login const repo = data.parent?.name - return data.isFork && owner && repo ? { owner, repo } : null + // Why: a fork parent lives on the same server as the fork. + return data.isFork && owner && repo + ? { owner, repo, ...(origin.host ? { host: origin.host } : {}) } + : null } catch { return null } finally { @@ -2015,7 +2046,7 @@ export async function getWorkItem( if (type === 'issue') { return await fetchIssueWorkItem( repoPath, - await getIssueOwnerRepo(repoPath, connectionId, localGitOptions), + await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), number, connectionId, localGitOptions @@ -2024,7 +2055,7 @@ export async function getWorkItem( if (type === 'pr') { return await fetchPullRequestWorkItem( repoPath, - await getOwnerRepo(repoPath, connectionId, localGitOptions), + await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions), number, connectionId, localGitOptions @@ -2034,7 +2065,7 @@ export async function getWorkItem( try { const issue = await fetchIssueWorkItem( repoPath, - await getIssueOwnerRepo(repoPath, connectionId, localGitOptions), + await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), number, connectionId, localGitOptions @@ -2056,7 +2087,7 @@ export async function getWorkItem( } return await fetchPullRequestWorkItem( repoPath, - await getOwnerRepo(repoPath, connectionId, localGitOptions), + await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions), number, connectionId, localGitOptions @@ -2960,10 +2991,11 @@ export async function getPRForBranch( // Why: the exact-linked fallback (`gh pr view` with no resolved repo candidates) // returns dataRepo=null, which would leave the merged-PR membership probe unable // to run. Derive the PR's own repo from its web URL so a diverged merged linked -// PR can still be confirmed and cleared. Host-agnostic to cover GitHub Enterprise. +// PR can still be confirmed and cleared. The URL's hostname carries the identity +// for GitHub Enterprise, so the probe targets the PR's actual server. function ownerRepoFromPullRequestUrl(url: string): OwnerRepo | null { - const match = url.match(/^https?:\/\/[^/\s]+\/([^/\s]+)\/([^/\s]+)\/pull\/\d+/) - return match ? { owner: match[1], repo: match[2] } : null + const match = url.match(/^https?:\/\/([^/\s]+)\/([^/\s]+)\/([^/\s]+)\/pull\/\d+/) + return match ? { owner: match[2], repo: match[3], host: match[1] } : null } export async function getPRForBranchOutcome( @@ -2988,10 +3020,10 @@ export async function getPRForBranchOutcome( await acquire() try { - const { candidates, headRepo } = await resolvePRRepositoryCandidates( + const { candidates, headRepo } = await resolveGitHubApiRepositoryCandidates( repoPath, connectionId, - ...localGitArgs + localGitOptions ) let data: PullRequestLookupData | null = null let dataRepo: OwnerRepo | null = null @@ -3110,11 +3142,11 @@ export async function getPRForBranchOutcome( ) if (upstreamBranch) { const upstreamHeadRepo = - (await getOwnerRepoForRemote( + (await getGitHubApiRepositoryForRemote( repoPath, upstreamBranch.remoteName, connectionId, - ...localGitArgs + localGitOptions )) ?? headRepo if ( upstreamHeadRepo && diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 571ec5a5b2d..e84e455b08f 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -1,7 +1,13 @@ -import type { GitHubOwnerRepo } from '../../shared/types' -import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' -import { getOwnerRepo, type LocalGitExecOptions } from './gh-utils' -import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' +import type { GitHubOwnerRepo, IssueSourcePreference } from '../../shared/types' +import { + githubRepoIdentityKey, + isDefaultGitHubHost +} from '../../shared/github-repository-identity-key' +import { getOwnerRepo, getOwnerRepoForRemote, type LocalGitExecOptions } from './gh-utils' +import { + getEnterpriseGitHubRepoSlug, + getEnterpriseGitHubRepoSlugForRemote +} from './github-enterprise-repository' export type GitHubApiRepository = GitHubOwnerRepo @@ -14,10 +20,11 @@ const originRepoInFlight = new Map>( function originRepoCacheKey( repoPath: string, + remoteName: string, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): string { - return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}` + return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}\0${remoteName}` } /** @internal - exposed for tests only */ @@ -26,16 +33,27 @@ export function _resetOriginGitHubApiRepositoryCache(): void { originRepoInFlight.clear() } -export async function getOriginGitHubApiRepository( +/** + * Host-qualified repository identity for one remote: github.com remotes come + * from the cached slug parser; any other GitHub-shaped host is auth-gated so a + * non-GitHub forge never routes to the GitHub provider. + */ +export async function getGitHubApiRepositoryForRemote( repoPath: string, + remoteName: string, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await getOwnerRepo(repoPath, connectionId, localGitOptions) + // Why: origin routes through the origin-named entry points so their + // call-signature (and the tests mocking them) stay the single seam. + const ownerRepo = + remoteName === 'origin' + ? await getOwnerRepo(repoPath, connectionId, localGitOptions) + : await getOwnerRepoForRemote(repoPath, remoteName, connectionId, localGitOptions) if (ownerRepo) { return { ...ownerRepo, host: 'github.com' } } - const cacheKey = originRepoCacheKey(repoPath, connectionId, localGitOptions) + const cacheKey = originRepoCacheKey(repoPath, remoteName, connectionId, localGitOptions) const cached = originRepoCache.get(cacheKey) if (cached && cached.expiresAt > Date.now()) { return cached.value @@ -47,7 +65,15 @@ export async function getOriginGitHubApiRepository( const probe = (async () => { const enterpriseOptions = Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} - const slug = await getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + const slug = + remoteName === 'origin' + ? await getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + : await getEnterpriseGitHubRepoSlugForRemote( + repoPath, + remoteName, + connectionId, + enterpriseOptions + ) originRepoCache.set(cacheKey, { value: slug, expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS }) return slug })() @@ -61,6 +87,111 @@ export async function getOriginGitHubApiRepository( } } +export async function getOriginGitHubApiRepository( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + return getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) +} + +/** Hosted mirror of getIssueOwnerRepo: issues prefer `upstream` over `origin`. */ +export async function getIssueGitHubApiRepository( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + const upstream = await getGitHubApiRepositoryForRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + if (upstream) { + return upstream + } + return getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) +} + +export type GitHubApiRepositoryCandidates = { + candidates: GitHubApiRepository[] + headRepo: GitHubApiRepository | null +} + +/** Hosted mirror of resolvePRRepositoryCandidates: upstream first, then origin. */ +export async function resolveGitHubApiRepositoryCandidates( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + const [upstream, origin] = await Promise.all([ + getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions), + getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) + ]) + const seen = new Set() + const candidates: GitHubApiRepository[] = [] + for (const candidate of [upstream, origin]) { + if (!candidate) { + continue + } + const key = githubRepoIdentityKey(candidate) + if (seen.has(key)) { + continue + } + seen.add(key) + candidates.push(candidate) + } + return { candidates, headRepo: origin } +} + +export type ResolvedGitHubApiRepositorySource = { + source: GitHubApiRepository | null + /** True when explicit upstream is gone and resolver fell back to origin. */ + fellBack: boolean +} + +/** Hosted mirror of resolveIssueSource — same preference semantics. */ +export async function resolveIssueGitHubApiRepositorySource( + repoPath: string, + preference: IssueSourcePreference | undefined, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + if (preference === 'upstream') { + const upstream = await getGitHubApiRepositoryForRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + if (upstream) { + return { source: upstream, fellBack: false } + } + const origin = await getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ) + return { source: origin, fellBack: origin !== null } + } + if (preference === 'origin') { + return { + source: await getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ), + fellBack: false + } + } + return { + source: await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), + fellBack: false + } +} + export async function resolveGitHubApiRepository( repoPath: string, repository?: GitHubApiRepository | null, @@ -104,3 +235,15 @@ export function githubHostExecOptions(repository: GitHubApiRepository | null | u export function githubRepositoryWebHost(repository: GitHubApiRepository): string { return repository.host ?? 'github.com' } + +/** + * Positional `HOST/OWNER/REPO` argv value (e.g. `gh repo view `). + * Positional slugs bypass the runner's `--repo` qualifier, so they must be + * qualified here; github.com stays bare for compatibility with gh's default. + */ +export function githubRepositorySlugArg(repository: GitHubApiRepository): string { + const slug = `${repository.owner}/${repository.repo}` + return repository.host && !isDefaultGitHubHost(repository.host) + ? `${repository.host}/${slug}` + : slug +} diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index e16ab673415..1986ce11afb 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -120,14 +120,15 @@ export async function isGitHubHostAuthenticated( } /** - * Resolve owner/repo for a GitHub Enterprise Server `origin` remote — a custom - * host the user is gh-authenticated to. Returns null for github.com (already - * handled by {@link getOwnerRepo}) and for hosts gh is not logged in to + * Resolve owner/repo for a GitHub Enterprise Server remote — a custom host the + * user is gh-authenticated to. Returns null for github.com (already handled by + * {@link getOwnerRepo}) and for hosts gh is not logged in to * (Gitea/Forgejo/self-hosted GitLab/etc.), so GHES routes to the GitHub provider * without a GitHub provider stealing another forge's remote. */ -export async function getEnterpriseGitHubRepoSlug( +export async function getEnterpriseGitHubRepoSlugForRemote( repoPath: string, + remoteName: string, connectionId?: string | null, options: HostedReviewExecutionOptions = {} ): Promise { @@ -135,7 +136,7 @@ export async function getEnterpriseGitHubRepoSlug( const context = githubRepoContext(repoPath, connectionId, localGitOptions) let remoteUrl: string | null try { - remoteUrl = await getRemoteUrlForRepo(context, 'origin') + remoteUrl = await getRemoteUrlForRepo(context, remoteName) } catch { return null } @@ -151,3 +152,11 @@ export async function getEnterpriseGitHubRepoSlug( ) return authenticated ? { owner: identity.owner, repo: identity.repo, host: identity.host } : null } + +export async function getEnterpriseGitHubRepoSlug( + repoPath: string, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + return getEnterpriseGitHubRepoSlugForRemote(repoPath, 'origin', connectionId, options) +} diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index 7ff695ba12d..e510a363cb1 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' import type * as GhUtils from './gh-utils' const { @@ -27,6 +28,26 @@ vi.mock('./gh-utils', async () => { } }) +vi.mock('./github-api-repository', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Why: these suites drive source resolution through the legacy gh-utils + // mocks; bridge the hosted seams onto the same mocks. + getIssueGitHubApiRepository: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => getIssueOwnerRepoMock(repoPath, connectionId, localGitOptions), + resolveIssueGitHubApiRepositorySource: ( + repoPath: string, + preference: unknown, + connectionId?: string | null, + localGitOptions?: unknown + ) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions) + } +}) + import { addIssueComment, createIssue, diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index dcfb91d8013..15219994859 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -15,9 +15,14 @@ import type { } from '../../shared/types' import { mapIssueInfo } from './mappers' import type { LocalGitExecOptions, OwnerRepo } from './gh-utils' -import { githubHostExecOptions, resolveGitHubApiRepository } from './github-api-repository' +import { + getIssueGitHubApiRepository, + githubHostExecOptions, + resolveGitHubApiRepository, + resolveIssueGitHubApiRepositorySource +} from './github-api-repository' // prettier-ignore -import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' +import { ghExecFileAsync, acquire, release, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' // Why: distinguishes a successful-empty listing from a failed fetch. The // previous `catch { return [] }` conflated a 403 on a private upstream with an @@ -56,8 +61,8 @@ export async function getIssue( localGitOptions: LocalGitExecOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const ownerRepo = await getIssueOwnerRepo(repoPath, connectionId, localGitOptions) + const ownerRepo = await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { if (ownerRepo) { @@ -106,13 +111,13 @@ export async function listIssues( localGitOptions: LocalGitExecOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const { source: ownerRepo } = await resolveIssueSource( + const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( repoPath, preference, connectionId, localGitOptions ) + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { if (ownerRepo) { @@ -175,13 +180,13 @@ export async function createIssue( return { ok: false, error: 'Title is required' } } const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const { source: ownerRepo } = await resolveIssueSource( + const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( repoPath, preference, connectionId, localGitOptions ) + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -288,11 +293,11 @@ export async function updateIssue( localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const ownerRepo = await getIssueOwnerRepo(repoPath, connectionId, localGitOptions) + const ownerRepo = await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } const repo = `${ownerRepo.owner}/${ownerRepo.repo}` const errors: string[] = [] @@ -413,7 +418,7 @@ export async function addIssueComment( const context = githubRepoContext(repoPath, connectionId, localGitOptions) const ownerRepo = ownerRepoOverride ? await resolveGitHubApiRepository(repoPath, ownerRepoOverride, connectionId, localGitOptions) - : await getIssueOwnerRepo(repoPath, connectionId, localGitOptions) + : await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -466,13 +471,13 @@ export async function listLabels( localGitOptions: LocalGitExecOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const { source: ownerRepo } = await resolveIssueSource( + const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( repoPath, preference, connectionId, localGitOptions ) + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return [] } @@ -506,13 +511,13 @@ export async function listAssignableUsers( localGitOptions: LocalGitExecOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const { source: ownerRepo } = await resolveIssueSource( + const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( repoPath, preference, connectionId, localGitOptions ) + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return [] } diff --git a/src/main/github/project-view.test.ts b/src/main/github/project-view.test.ts index 00a8844bb34..bde450d8108 100644 --- a/src/main/github/project-view.test.ts +++ b/src/main/github/project-view.test.ts @@ -152,6 +152,21 @@ describe('parseProjectPaste', () => { expect(parseProjectPaste('https://github.com/orgs/co_op/projects/1')).toBeNull() }) + it('accepts enterprise-host URLs only when that host is provided (GHES)', () => { + const url = 'https://github.corp.example/orgs/acme/projects/7/views/2' + expect(parseProjectPaste(url, 'github.corp.example')).toEqual({ + kind: 'org', + owner: 'acme', + number: 7, + viewNumber: 2 + }) + expect(parseProjectPaste(url)).toBeNull() + // github.com URLs still parse when a GHES host is supplied. + expect( + parseProjectPaste('https://github.com/orgs/acme/projects/7', 'github.corp.example') + ).toEqual({ kind: 'org', owner: 'acme', number: 7 }) + }) + it('returns null for empty input', () => { expect(parseProjectPaste('')).toBeNull() expect(parseProjectPaste(' ')).toBeNull() diff --git a/src/main/github/project-view.ts b/src/main/github/project-view.ts index 3a040d37520..0fe26a5c3ee 100644 --- a/src/main/github/project-view.ts +++ b/src/main/github/project-view.ts @@ -9,19 +9,22 @@ import { release, extractExecError, ghExecFileAsync, - rateLimitGuard, - noteRateLimitSpend, - classifyProjectError, - driftError, - errorsIndicateParentField, - rateLimitedError, + repositoryRateLimitGuard, + noteRepositoryRateLimitSpend, runGraphql, isValidOwnerSlug, assertSlug, assertPositiveInt, - type GhGraphqlErrorShape, + projectGhExecOptions, type GraphqlVars } from './project-view/internals' +import { + classifyProjectError, + driftError, + errorsIndicateParentField, + rateLimitedError, + type GhGraphqlErrorShape +} from './project-view/project-error-classification' import type { GetProjectViewTableArgs, GetProjectViewTableResult, @@ -41,6 +44,7 @@ import type { GitHubProjectViewError, GitHubProjectViewLayout, GitHubProjectViewSummary, + ListAccessibleProjectsArgs, ListAccessibleProjectsResult, ListProjectViewsArgs, ListProjectViewsResult, @@ -54,12 +58,8 @@ import { // Re-export the public API so existing call sites (`./project-view`) keep // working unchanged. The split is internal-only. -export { - isValidOwnerSlug, - isValidRepoSlug, - isValidSlug, - classifyProjectError -} from './project-view/internals' +export { isValidOwnerSlug, isValidRepoSlug, isValidSlug } from './project-view/internals' +export { classifyProjectError } from './project-view/project-error-classification' export { updateProjectItemFieldValue, clearProjectItemFieldValue, @@ -145,16 +145,32 @@ const parentFieldWarningLoggedByOwner = new Map() // one caller drives the probe; siblings await the same result. const parentFieldProbeInFlight = new Map>() -function ownerScopeKey(owner: string, ownerType: GitHubProjectOwnerType): string { - return `${owner}\u0000${ownerType}` +// 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. +function ownerScopeKey(owner: string, ownerType: GitHubProjectOwnerType, host?: string): string { + const base = `${owner}\u0000${ownerType}` + return host ? `${base}\u0000${host}` : base } -function rememberOwnerType(owner: string, ownerType: GitHubProjectOwnerType | null): void { - rememberProjectViewCacheEntry(ownerTypeCache, owner, ownerType) +function ownerTypeCacheKey(owner: string, host?: string): string { + return host ? `${owner}\u0000${host}` : owner } -function getCachedOwnerType(owner: string): GitHubProjectOwnerType | null | undefined { - return getProjectViewCacheEntry(ownerTypeCache, owner) +function rememberOwnerType( + owner: string, + ownerType: GitHubProjectOwnerType | null, + host?: string +): void { + rememberProjectViewCacheEntry(ownerTypeCache, ownerTypeCacheKey(owner, host), ownerType) +} + +function getCachedOwnerType( + owner: string, + host?: string +): GitHubProjectOwnerType | null | undefined { + return getProjectViewCacheEntry(ownerTypeCache, ownerTypeCacheKey(owner, host)) } function markParentFieldRetried(scopeKey: string): void { @@ -197,16 +213,18 @@ export function _getProjectViewCacheSizesForTests(): { /** @internal - exposed for cache-bound tests only. */ export function _rememberProjectViewOwnerTypeForTests( owner: string, - ownerType: GitHubProjectOwnerType | null + ownerType: GitHubProjectOwnerType | null, + host?: string ): void { - rememberOwnerType(owner, ownerType) + rememberOwnerType(owner, ownerType, host) } /** @internal - exposed for cache-bound tests only. */ export function _getProjectViewOwnerTypeForTests( - owner: string + owner: string, + host?: string ): GitHubProjectOwnerType | null | undefined { - return getCachedOwnerType(owner) + return getCachedOwnerType(owner, host) } /** @internal - exposed for cache-bound tests only. */ @@ -642,6 +660,7 @@ async function fetchProjectViewsPage(args: { owner: string ownerType: GitHubProjectOwnerType projectNumber: number + host?: string after: string | null }): Promise< | { @@ -686,7 +705,8 @@ async function fetchProjectViewsPage(args: { } const res = await runGraphql>( query, - vars + vars, + projectGhExecOptions(args.host) ) if (!res.ok) { return res @@ -709,7 +729,8 @@ async function fetchProjectViewsPage(args: { async function fetchViewFieldsContinuation( viewId: string, - after: string + after: string, + host?: string ): Promise< { ok: true; fields: RawProjectV2Field[] } | { ok: false; error: GitHubProjectViewError } > { @@ -745,7 +766,7 @@ async function fetchViewFieldsContinuation( nodes?: (RawProjectV2Field | null)[] } } | null - }>(query, { viewId, after: cursor }) + }>(query, { viewId, after: cursor }, projectGhExecOptions(host)) if (!res.ok) { return res } @@ -855,6 +876,7 @@ async function fetchItemsPageWithRaw(args: { first: number after: string | null includeParent: boolean + host?: string }): Promise< | { ok: true; page: RawItemsPage } | { @@ -896,7 +918,9 @@ async function fetchItemsPageWithRaw(args: { argsArr.push('-f', `after=${args.after}`) } - const guard = rateLimitGuard('graphql') + // Why: GHES traffic runs against its own quota — only github.com requests + // consult/debit the shared snapshot. + const guard = repositoryRateLimitGuard(args, 'graphql') if (guard.blocked) { return { ok: false, @@ -906,13 +930,16 @@ async function fetchItemsPageWithRaw(args: { } } await acquire() - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(args, 'graphql') try { let stdout = '' let stderr = '' let execFailed = false try { - const r = await ghExecFileAsync(argsArr, { encoding: 'utf-8' }) + const r = await ghExecFileAsync(argsArr, { + encoding: 'utf-8', + ...projectGhExecOptions(args.host) + }) stdout = r.stdout stderr = r.stderr } catch (err) { @@ -983,6 +1010,7 @@ async function fetchAllItems(args: { ownerType: GitHubProjectOwnerType projectNumber: number query: string + host?: string }): Promise< | { ok: true; rows: GitHubProjectRow[]; totalCount: number; parentFieldDropped: boolean } | { ok: false; error: GitHubProjectViewError; totalCount?: number } @@ -990,7 +1018,7 @@ async function fetchAllItems(args: { // Why: caches keyed by (owner, ownerType) so a single owner's lack of // Issue.parent capability (e.g. token without scope on org A) doesn't // poison fetches for unrelated owners. See bug-scan finding 2. - const scopeKey = ownerScopeKey(args.owner, args.ownerType) + const scopeKey = ownerScopeKey(args.owner, args.ownerType, args.host) // Why: if another caller for the SAME owner is currently probing whether // Issue.parent is supported, await its decision so we don't fire a // duplicate with-parent probe. We must re-read the retried flag AFTER @@ -1024,7 +1052,8 @@ async function fetchAllItems(args: { query: args.query, first: ITEM_PAGE_SIZE, after: null, - includeParent: true + includeParent: true, + host: args.host }) // Why: flip parentFieldRetriedByOwner BEFORE resolveProbe()/clearing // parentFieldProbeInFlight so siblings that awoke on `inFlight.catch()` @@ -1050,7 +1079,8 @@ async function fetchAllItems(args: { query: args.query, first: ITEM_PAGE_SIZE, after: null, - includeParent + includeParent, + host: args.host }) } if (!first.ok && includeParent && errorsIndicateParentField(first.rawErrors, first.stderr)) { @@ -1073,7 +1103,8 @@ async function fetchAllItems(args: { query: args.query, first: ITEM_PAGE_SIZE, after: null, - includeParent: false + includeParent: false, + host: args.host }) } if (!first.ok) { @@ -1140,7 +1171,8 @@ async function fetchAllItems(args: { query: args.query, first: ITEM_PAGE_SIZE, after: cursor as string, - includeParent + includeParent, + host: args.host }) if (!next.ok) { return { ok: false, error: next.error, totalCount } @@ -1179,6 +1211,7 @@ async function fetchItemsCountOnly(args: { ownerType: GitHubProjectOwnerType projectNumber: number query: string + host?: string }): Promise { const root = ownerQueryRoot(args.ownerType) const query = ` @@ -1192,7 +1225,11 @@ async function fetchItemsCountOnly(args: { ` const res = await runGraphql< Record - >(query, { owner: args.owner, num: args.projectNumber, q: args.query }) + >( + query, + { owner: args.owner, num: args.projectNumber, q: args.query }, + projectGhExecOptions(args.host) + ) if (!res.ok) { return null } @@ -1231,6 +1268,7 @@ export async function getProjectViewTable( owner: args.owner, ownerType: args.ownerType, projectNumber: args.projectNumber, + host: args.host, after: cursor }) if (!page.ok) { @@ -1287,7 +1325,7 @@ export async function getProjectViewTable( let extraFields: RawProjectV2Field[] = [] const fieldsPi = selectedRaw.fields?.pageInfo if (fieldsPi?.hasNextPage === true && typeof fieldsPi.endCursor === 'string' && selectedRaw.id) { - const cont = await fetchViewFieldsContinuation(selectedRaw.id, fieldsPi.endCursor) + const cont = await fetchViewFieldsContinuation(selectedRaw.id, fieldsPi.endCursor, args.host) if (!cont.ok) { return { ok: false, error: cont.error } } @@ -1314,7 +1352,8 @@ export async function getProjectViewTable( owner: args.owner, ownerType: args.ownerType, projectNumber: args.projectNumber, - query: effectiveQuery + query: effectiveQuery, + host: args.host }) return { ok: false, @@ -1331,7 +1370,8 @@ export async function getProjectViewTable( owner: args.owner, ownerType: args.ownerType, projectNumber: args.projectNumber, - query: effectiveQuery + query: effectiveQuery, + host: args.host }) if (!items.ok) { return { @@ -1386,7 +1426,10 @@ type RawViewerDiscovery = { } } -export async function listAccessibleProjects(): Promise { +export async function listAccessibleProjects( + args?: ListAccessibleProjectsArgs +): Promise { + const host = args?.host const viewerProjects: GitHubProjectSummary[] = [] const orgProjects: GitHubProjectSummary[] = [] // Why: per-org failures are collected so the picker can render a "some orgs @@ -1421,7 +1464,7 @@ export async function listAccessibleProjects(): Promise(query, vars) + const res = await runGraphql(query, vars, projectGhExecOptions(host)) if (!res.ok) { // Why: a viewer-level failure is structural — if we can't list the // user's own projects, we have nothing to build on. Propagate as a @@ -1496,7 +1539,7 @@ export async function listAccessibleProjects(): Promise(query, vars) + const res = await runGraphql(query, vars, projectGhExecOptions(host)) if (!res.ok) { // Why: the org-listing query itself failed (not a nested projectsV2). // Record it as a partial failure against a synthetic `*` owner so the @@ -1519,7 +1562,7 @@ export async function listAccessibleProjects(): Promise - >(query, vars) + >(query, vars, projectGhExecOptions(host)) if (!res.ok) { return { ok: false, error: res.error } } @@ -1656,7 +1704,7 @@ async function resolveOwnerType( return { ok: true, title: '' } } - const cached = getCachedOwnerType(owner) + const cached = getCachedOwnerType(owner, host) const candidates: GitHubProjectOwnerType[] = preferred ? [preferred] : cached @@ -1674,7 +1722,7 @@ async function resolveOwnerType( for (const ot of ordered) { const r = await tryOne(ot, null) if (r.ok) { - rememberOwnerType(owner, ot) + rememberOwnerType(owner, ot, host) return { ok: true, ownerType: ot, title: r.title } } lastError = r.error @@ -1683,7 +1731,7 @@ async function resolveOwnerType( return { ok: false, error: r.error } } } - rememberOwnerType(owner, null) + rememberOwnerType(owner, null, host) return { ok: false, error: lastError ?? { type: 'not_found', message: 'Owner not found.' } @@ -1706,7 +1754,7 @@ export async function resolveProjectRef( error: { type: 'validation_error', message: GITHUB_PROJECT_REF_INPUT_TOO_LARGE_ERROR } } } - const parsed = parseProjectPaste(input) + const parsed = parseProjectPaste(input, args.host) if (!parsed) { return { ok: false, @@ -1719,7 +1767,7 @@ export async function resolveProjectRef( const preferred: GitHubProjectOwnerType | null = parsed.kind === 'org' ? 'organization' : parsed.kind === 'user' ? 'user' : null // Verify by fetching project title. - const ownerRes = await resolveOwnerType(parsed.owner, preferred) + const ownerRes = await resolveOwnerType(parsed.owner, preferred, args.host) if (!ownerRes.ok) { return { ok: false, error: ownerRes.error } } @@ -1732,7 +1780,7 @@ export async function resolveProjectRef( ` const res = await runGraphql< Record - >(query, { owner: parsed.owner, num: parsed.number }) + >(query, { owner: parsed.owner, num: parsed.number }, projectGhExecOptions(args.host)) if (!res.ok) { return { ok: false, error: res.error } } @@ -1778,6 +1826,7 @@ export async function listProjectViews( owner: args.owner, ownerType: args.ownerType, projectNumber: args.projectNumber, + host: args.host, after: cursor }) if (!page.ok) { diff --git a/src/main/github/project-view/internals.ts b/src/main/github/project-view/internals.ts index 586bbaa6e95..07a98827ad6 100644 --- a/src/main/github/project-view/internals.ts +++ b/src/main/github/project-view/internals.ts @@ -4,12 +4,43 @@ // translation, and a single hook point for future quota tracking. import { acquire, release } from '../gh-utils' import { extractExecError, ghExecFileAsync } from '../../git/runner' -import { rateLimitGuard, noteRateLimitSpend, type RateLimitBucketKind } from '../rate-limit' +import { + rateLimitGuard, + noteRateLimitSpend, + repositoryRateLimitGuard, + noteRepositoryRateLimitSpend, + type RateLimitBucketKind +} from '../rate-limit' import type { GitHubProjectViewError } from '../../../shared/github-project-types' +import { + classifyProjectError, + driftError, + rateLimitedError, + type GhGraphqlErrorShape +} from './project-error-classification' -export { acquire, release, extractExecError, ghExecFileAsync, rateLimitGuard, noteRateLimitSpend } +export { + acquire, + release, + extractExecError, + ghExecFileAsync, + rateLimitGuard, + noteRateLimitSpend, + repositoryRateLimitGuard, + noteRepositoryRateLimitSpend +} export type { RateLimitBucketKind } +/** gh exec routing shared by the project-view read/write paths. `host` pins + * GHES requests to the enterprise server via the runner's host qualifier. */ +export type ProjectGhExecOptions = { cwd?: string; host?: string } + +// Why: owner/project-addressed calls carry only a host (no repo slug), so they +// can't reuse githubHostExecOptions — mirror its host pinning for that shape. +export function projectGhExecOptions(host: string | undefined): ProjectGhExecOptions { + return host ? { host } : {} +} + // ─── Slug validation ────────────────────────────────────────────────── // Why: GitHub usernames/org logins disallow `_`, `.`, leading `-`. Repo names @@ -88,176 +119,6 @@ export function validateSlugArgs( return { ok: true } } -// ─── Error classification ────────────────────────────────────────────── - -export type GhGraphqlErrorShape = { - type?: string - message?: string - path?: (string | number)[] - extensions?: { code?: string } -} - -export function extractGraphqlErrors(stderr: string, stdout: string): GhGraphqlErrorShape[] { - // `gh api graphql` prints the response JSON to stdout even on GraphQL - // errors, and the stderr carries a summary. Try stdout first; if parsing - // fails, fall back to stderr. - const sources = [stdout, stderr] - for (const src of sources) { - if (!src) { - continue - } - try { - const parsed = JSON.parse(src) as { errors?: GhGraphqlErrorShape[] } - if (parsed.errors && parsed.errors.length > 0) { - return parsed.errors - } - } catch { - // not JSON — continue - } - } - return [] -} - -export function errorsIndicateParentField(errors: GhGraphqlErrorShape[], stderr: string): boolean { - const lower = stderr.toLowerCase() - // Preview-header shape: gh returns a 4xx with "preview" in the message. - if (lower.includes('preview') && lower.includes('parent')) { - return true - } - return errors.some((e) => { - const type = (e.type ?? '').toUpperCase() - if (type === 'FIELD_NOT_FOUND' || type === 'UNDEFINED_FIELD' || type === 'FIELD_ERRORS') { - const tail = e.path?.at(-1) - if (tail === 'parent') { - return true - } - // FIELD_ERRORS often omits `path`; match on message for the parent field. - if ((e.message ?? '').toLowerCase().includes('parent')) { - return true - } - } - return false - }) -} - -export function classifyProjectError(stderr: string, stdout: string): GitHubProjectViewError { - const errors = extractGraphqlErrors(stderr, stdout) - const s = stderr.toLowerCase() - - // Auth - if ( - s.includes('authentication required') || - s.includes('not logged in') || - s.includes('gh auth login') - ) { - return { - type: 'auth_required', - message: 'Sign in to GitHub to load project tasks. Run `gh auth login`.' - } - } - // Scope - if ( - s.includes('missing required scope') || - s.includes('your token has not been granted') || - (s.includes('resource not accessible') && (s.includes('project') || s.includes('scope'))) - ) { - return { - type: 'scope_missing', - message: - 'GitHub project access needs additional scopes. Run `gh auth refresh -s project -s read:org -s repo`.' - } - } - // Rate limit - if (s.includes('rate limit') || s.includes('api rate limit exceeded')) { - return { type: 'rate_limited', message: 'GitHub rate limit hit. Try again in a few minutes.' } - } - // Network — checked BEFORE not_found because DNS failures surface as - // "could not resolve host", which would otherwise be partially matched by - // the not_found branch's "could not resolve" check. Substring matching here - // is a one-way trapdoor: a real GraphQL "Could not resolve to a User…" - // error always contains "to a", so we tighten the not_found check below to - // require that token. - if ( - s.includes('timeout') || - s.includes('no such host') || - s.includes('network') || - s.includes('could not resolve host') || - s.includes('dial tcp') - ) { - return { type: 'network_error', message: 'Network error — check your connection.' } - } - // Not found - if ( - s.includes('http 404') || - errors.some((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') || - // Why: GitHub uses "to an" for vowel-leading types ("to an Issue", "to - // an Organization") and "to a" otherwise. The previous singular-only - // check missed the "an" variants when gh emits only the stderr summary - // without a structured GraphQL error array. See bug-scan finding 3. - /could not resolve to an? /.test(s) - ) { - const firstNotFound = errors.find((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') - return { - type: 'not_found', - message: 'Project or view not found.', - details: firstNotFound - ? { path: firstNotFound.path, code: firstNotFound.extensions?.code } - : undefined - } - } - // Validation - if (s.includes('http 422') || s.includes('validation failed')) { - return { type: 'validation_error', message: `Invalid request — ${stderr.trim()}` } - } - // GraphQL error with structured info - if (errors.length > 0) { - const first = errors[0] - return { - type: 'unknown', - message: first.message ?? 'Unknown GraphQL error.', - details: { path: first.path, code: first.extensions?.code } - } - } - // Why: don't leak full stderr to the UI — it can include verbose request - // dumps with header diagnostics. Truncate to the first non-empty line and - // cap length so unexpected diagnostics stay readable but bounded. - const firstLine = - stderr - .split('\n') - .map((l) => l.trim()) - .find((l) => l.length > 0) ?? '' - const safe = firstLine.length > 200 ? `${firstLine.slice(0, 200)}…` : firstLine - return { - type: 'unknown', - message: safe ? `GitHub request failed: ${safe}` : 'GitHub request failed.' - } -} - -export function driftError( - reason: string, - details?: { path?: (string | number)[]; code?: string } -): GitHubProjectViewError { - return { type: 'schema_drift', message: `Could not read this project view: ${reason}.`, details } -} - -// Why: the rate-limit circuit breaker short-circuits before we spawn `gh` -// when the cached snapshot says we're below the safety floor. Synthesize the -// same `rate_limited` error shape as the post-hoc classifier so the UI path -// is unchanged. We DO NOT fail open here when there's no cached snapshot — -// rateLimitGuard already handles that case (returns `blocked:false`). -export function rateLimitedError(blocked: { - remaining: number - limit: number - resetAt: number -}): GitHubProjectViewError { - const resetIn = Math.max(0, blocked.resetAt - Math.floor(Date.now() / 1000)) - const mins = Math.ceil(resetIn / 60) - return { - type: 'rate_limited', - message: `GitHub rate limit nearly exhausted (${blocked.remaining}/${blocked.limit} left). Resets in ~${mins}m.` - } -} - // ─── Low-level gh api graphql invocation ─────────────────────────────── export type GraphqlVars = Record @@ -265,12 +126,14 @@ export type GraphqlVars = Record export async function runGraphql( query: string, vars: GraphqlVars, - cwd?: string + exec?: ProjectGhExecOptions ): Promise< | { ok: true; data: T } | { ok: false; error: GitHubProjectViewError; raw: { stderr: string; stdout: string } } > { - const guard = rateLimitGuard('graphql') + // Why: GHES traffic runs against its own quota — only github.com requests + // consult/debit the shared snapshot. + const guard = repositoryRateLimitGuard({ host: exec?.host }, 'graphql') if (guard.blocked) { return { ok: false, error: rateLimitedError(guard), raw: { stderr: '', stdout: '' } } } @@ -286,11 +149,12 @@ export async function runGraphql( } } await acquire() - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend({ host: exec?.host }, 'graphql') try { const { stdout, stderr } = await ghExecFileAsync(args, { encoding: 'utf-8', - ...(cwd ? { cwd } : {}) + ...(exec?.cwd ? { cwd: exec.cwd } : {}), + ...(exec?.host ? { host: exec.host } : {}) }) try { const parsed = JSON.parse(stdout) as { data?: T; errors?: GhGraphqlErrorShape[] } @@ -336,18 +200,21 @@ export async function runRest( args: string[], cwd?: string, bucket: RateLimitBucketKind = 'core', - options?: { expectEmpty?: boolean } + options?: { expectEmpty?: boolean; host?: string } ): Promise<{ ok: true; data: T } | { ok: false; error: GitHubProjectViewError }> { - const guard = rateLimitGuard(bucket) + // Why: GHES traffic runs against its own quota — only github.com requests + // consult/debit the shared snapshot. + const guard = repositoryRateLimitGuard({ host: options?.host }, bucket) if (guard.blocked) { return { ok: false, error: rateLimitedError(guard) } } await acquire() - noteRateLimitSpend(bucket) + noteRepositoryRateLimitSpend({ host: options?.host }, bucket) try { const { stdout, stderr } = await ghExecFileAsync(['api', ...args], { encoding: 'utf-8', - ...(cwd ? { cwd } : {}) + ...(cwd ? { cwd } : {}), + ...(options?.host ? { host: options.host } : {}) }) // Why: 204/empty-body endpoints (DELETE label, DELETE comment) return no // body. Treat empty stdout as success rather than misclassifying the diff --git a/src/main/github/project-view/mutations.test.ts b/src/main/github/project-view/mutations.test.ts index 9d8cabca7f8..5272f03acb3 100644 --- a/src/main/github/project-view/mutations.test.ts +++ b/src/main/github/project-view/mutations.test.ts @@ -1,11 +1,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { acquireMock, ghExecFileAsyncMock, releaseMock, runRestMock } = vi.hoisted(() => ({ - acquireMock: vi.fn(), - ghExecFileAsyncMock: vi.fn(), - releaseMock: vi.fn(), - runRestMock: vi.fn() -})) +const { acquireMock, ghExecFileAsyncMock, releaseMock, runGraphqlMock, runRestMock } = vi.hoisted( + () => ({ + acquireMock: vi.fn(), + ghExecFileAsyncMock: vi.fn(), + releaseMock: vi.fn(), + runGraphqlMock: vi.fn(), + runRestMock: vi.fn() + }) +) vi.mock('./internals', () => ({ acquire: acquireMock, @@ -17,9 +20,12 @@ vi.mock('./internals', () => ({ ghExecFileAsync: ghExecFileAsyncMock, rateLimitGuard: () => ({ blocked: false }), noteRateLimitSpend: vi.fn(), + repositoryRateLimitGuard: () => ({ blocked: false }), + noteRepositoryRateLimitSpend: vi.fn(), + projectGhExecOptions: (host?: string) => (host ? { host } : {}), classifyProjectError: (stderr: string) => ({ type: 'unknown', message: stderr }), rateLimitedError: () => ({ type: 'rate_limited', message: 'rate limited' }), - runGraphql: vi.fn(), + runGraphql: runGraphqlMock, runRest: runRestMock, validateSlugArgs: (owner: string, repo: string) => owner && repo ? { ok: true } : { ok: false, error: { type: 'validation_error' } }, @@ -29,7 +35,7 @@ vi.mock('./internals', () => ({ : { ok: false, error: { type: 'validation_error', message: `${name} invalid` } } })) -import { updateIssueBySlug } from './mutations' +import { getWorkItemDetailsBySlug, updateIssueBySlug } from './mutations' describe('updateIssueBySlug', () => { beforeEach(() => { @@ -118,4 +124,65 @@ describe('updateIssueBySlug', () => { { encoding: 'utf-8' } ) }) + + it('threads the GHES host into gh exec and REST options', async () => { + await expect( + updateIssueBySlug({ + owner: 'acme', + repo: 'widgets', + number: 12, + host: 'github.corp.example', + updates: { state: 'closed', stateReason: 'completed', title: 'New title' } + }) + ).resolves.toEqual({ ok: true }) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['issue', 'close', '12', '--repo', 'acme/widgets', '--reason', 'completed'], + { encoding: 'utf-8', host: 'github.corp.example' } + ) + expect(runRestMock).toHaveBeenCalledWith( + ['-X', 'PATCH', 'repos/acme/widgets/issues/12', '--raw-field', 'title=New title'], + undefined, + 'core', + { host: 'github.corp.example' } + ) + }) +}) + +describe('getWorkItemDetailsBySlug', () => { + beforeEach(() => { + runGraphqlMock.mockReset() + }) + + it('threads the GHES host into the GraphQL exec options', async () => { + runGraphqlMock.mockResolvedValue({ + ok: true, + data: { + repository: { + issue: { + id: 'I_1', + number: 7, + title: 'Enterprise issue', + url: 'https://github.corp.example/acme/widgets/issues/7', + state: 'OPEN' + } + } + } + }) + + const result = await getWorkItemDetailsBySlug({ + owner: 'acme', + repo: 'widgets', + number: 7, + type: 'issue', + host: 'github.corp.example' + }) + + expect(result.ok).toBe(true) + expect(runGraphqlMock).toHaveBeenCalledWith( + expect.any(String), + { owner: 'acme', repo: 'widgets', num: 7 }, + { host: 'github.corp.example' } + ) + }) }) diff --git a/src/main/github/project-view/mutations.ts b/src/main/github/project-view/mutations.ts index e2da3dae068..b42833cd63b 100644 --- a/src/main/github/project-view/mutations.ts +++ b/src/main/github/project-view/mutations.ts @@ -6,16 +6,17 @@ import { release, extractExecError, ghExecFileAsync, - rateLimitGuard, - noteRateLimitSpend, - classifyProjectError, - rateLimitedError, + repositoryRateLimitGuard, + noteRepositoryRateLimitSpend, runGraphql, runRest, validateSlugArgs, assertPositiveInt, + projectGhExecOptions, type GraphqlVars } from './internals' +import { classifyProjectError, rateLimitedError } from './project-error-classification' +import { githubHostExecOptions } from '../github-api-repository' import type { GitHubAssignableUser, GitHubWorkItemDetails, PRComment } from '../../../shared/types' import type { AddIssueCommentBySlugArgs, @@ -122,7 +123,7 @@ export async function updateProjectItemFieldValue( fieldId: args.fieldId, value: valVar.val } - const res = await runGraphql(query, vars) + const res = await runGraphql(query, vars, projectGhExecOptions(args.host)) if (!res.ok) { return { ok: false, error: res.error } } @@ -144,11 +145,15 @@ export async function clearProjectItemFieldValue( }) { projectV2Item { id } } } ` - const res = await runGraphql(query, { - projectId: args.projectId, - itemId: args.itemId, - fieldId: args.fieldId - }) + const res = await runGraphql( + query, + { + projectId: args.projectId, + itemId: args.itemId, + fieldId: args.fieldId + }, + projectGhExecOptions(args.host) + ) if (!res.ok) { return { ok: false, error: res.error } } @@ -229,7 +234,7 @@ export async function updateIssueBySlug( } await acquire() try { - await ghExecFileAsync(stateArgs, { encoding: 'utf-8' }) + await ghExecFileAsync(stateArgs, { encoding: 'utf-8', ...githubHostExecOptions(args) }) } catch (err) { const { stderr, stdout } = extractExecError(err) return { ok: false, error: classifyProjectError(stderr, stdout) } @@ -247,7 +252,7 @@ export async function updateIssueBySlug( if (body !== undefined) { patchArgs.push('--raw-field', `body=${body}`) } - const r = await runRest(patchArgs) + const r = await runRest(patchArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -263,7 +268,12 @@ export async function updateIssueBySlug( const addCount = addLabels?.length ?? 0 if (removeCount > 1) { type RawLabelResp = { name?: string }[] - const fetched = await runRest(['-X', 'GET', `${base}/labels`]) + const fetched = await runRest( + ['-X', 'GET', `${base}/labels`], + undefined, + 'core', + githubHostExecOptions(args) + ) if (!fetched.ok) { return { ok: false, error: fetched.error } } @@ -282,7 +292,8 @@ export async function updateIssueBySlug( // dedicated DELETE endpoint is the documented way to remove all // labels in a single call. const r = await runRest(['-X', 'DELETE', `${base}/labels`], undefined, 'core', { - expectEmpty: true + expectEmpty: true, + ...githubHostExecOptions(args) }) if (!r.ok && r.error.type !== 'not_found') { return { ok: false, error: r.error } @@ -292,7 +303,7 @@ export async function updateIssueBySlug( for (const name of currentNames) { putArgs.push('--raw-field', `labels[]=${name}`) } - const r = await runRest(putArgs) + const r = await runRest(putArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -303,7 +314,7 @@ export async function updateIssueBySlug( for (const l of addLabels ?? []) { restArgs.push('--raw-field', `labels[]=${l}`) } - const r = await runRest(restArgs) + const r = await runRest(restArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -313,7 +324,7 @@ export async function updateIssueBySlug( ['-X', 'DELETE', `${base}/labels/${encodeURIComponent(removeLabels![0])}`], undefined, 'core', - { expectEmpty: true } + { expectEmpty: true, ...githubHostExecOptions(args) } ) if (!r.ok && r.error.type !== 'not_found') { return { ok: false, error: r.error } @@ -328,7 +339,7 @@ export async function updateIssueBySlug( for (const u of addAssignees) { restArgs.push('--raw-field', `assignees[]=${u}`) } - const r = await runRest(restArgs) + const r = await runRest(restArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -338,7 +349,7 @@ export async function updateIssueBySlug( for (const u of removeAssignees) { restArgs.push('--raw-field', `assignees[]=${u}`) } - const r = await runRest(restArgs) + const r = await runRest(restArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -384,7 +395,7 @@ export async function updatePullRequestBySlug( // No fields to update — nothing to do. return { ok: true } } - const r = await runRest(patchArgs) + const r = await runRest(patchArgs, undefined, 'core', githubHostExecOptions(args)) if (!r.ok) { return { ok: false, error: r.error } } @@ -425,13 +436,18 @@ export async function addIssueCommentBySlug( if (typeof args.body !== 'string' || !args.body.trim()) { return { ok: false, error: { type: 'validation_error', message: 'Comment body required.' } } } - const r = await runRest([ - '-X', - 'POST', - `repos/${args.owner}/${args.repo}/issues/${args.number}/comments`, - '--raw-field', - `body=${args.body}` - ]) + const r = await runRest( + [ + '-X', + 'POST', + `repos/${args.owner}/${args.repo}/issues/${args.number}/comments`, + '--raw-field', + `body=${args.body}` + ], + undefined, + 'core', + githubHostExecOptions(args) + ) if (!r.ok) { return { ok: false, error: r.error } } @@ -452,13 +468,18 @@ export async function updateIssueCommentBySlug( if (typeof args.body !== 'string' || !args.body.trim()) { return { ok: false, error: { type: 'validation_error', message: 'Comment body required.' } } } - const r = await runRest([ - '-X', - 'PATCH', - `repos/${args.owner}/${args.repo}/issues/comments/${args.commentId}`, - '--raw-field', - `body=${args.body}` - ]) + const r = await runRest( + [ + '-X', + 'PATCH', + `repos/${args.owner}/${args.repo}/issues/comments/${args.commentId}`, + '--raw-field', + `body=${args.body}` + ], + undefined, + 'core', + githubHostExecOptions(args) + ) if (!r.ok) { return { ok: false, error: r.error } } @@ -480,7 +501,7 @@ export async function deleteIssueCommentBySlug( ['-X', 'DELETE', `repos/${args.owner}/${args.repo}/issues/comments/${args.commentId}`], undefined, 'core', - { expectEmpty: true } + { expectEmpty: true, ...githubHostExecOptions(args) } ) if (!r.ok) { return { ok: false, error: r.error } @@ -497,18 +518,18 @@ export async function listLabelsBySlug( if (!v.ok) { return v } - const guard = rateLimitGuard('core') + const guard = repositoryRateLimitGuard(args, 'core') if (guard.blocked) { return { ok: false, error: rateLimitedError(guard) } } await acquire() // Why: `--paginate` may fan out to multiple pages; we can only reasonably // estimate a 1-call spend up front. The next probe will reconcile. - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(args, 'core') try { const { stdout } = await ghExecFileAsync( ['api', '--paginate', `repos/${args.owner}/${args.repo}/labels`, '--jq', '.[].name'], - { encoding: 'utf-8' } + { encoding: 'utf-8', ...githubHostExecOptions(args) } ) return { ok: true, @@ -535,12 +556,12 @@ export async function listAssignableUsersBySlug( // Seed logins merge after the fetch so callers can include currently-visible // assignees even if the repo participant search is sparse. const result: GitHubAssignableUser[] = [] - const guard = rateLimitGuard('core') + const guard = repositoryRateLimitGuard(args, 'core') if (guard.blocked) { return { ok: false, error: rateLimitedError(guard) } } await acquire() - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(args, 'core') try { const { stdout } = await ghExecFileAsync( [ @@ -550,7 +571,7 @@ export async function listAssignableUsersBySlug( '--jq', '.[] | {login: .login, name: null, avatarUrl: .avatar_url}' ], - { encoding: 'utf-8' } + { encoding: 'utf-8', ...githubHostExecOptions(args) } ) for (const line of stdout .trim() @@ -614,7 +635,7 @@ export async function listIssueTypesBySlug( } | null)[] } | null } | null - }>(query, { owner: args.owner, repo: args.repo }) + }>(query, { owner: args.owner, repo: args.repo }, githubHostExecOptions(args)) if (!res.ok) { // Why: repos without issue types respond with a GraphQL error claiming the // `issueTypes` field is unknown. Map that to an empty list so the UI shows @@ -659,7 +680,8 @@ export async function updateIssueTypeBySlug( `query($owner:String!, $repo:String!, $num:Int!) { repository(owner:$owner, name:$repo) { issue(number:$num) { id } } }`, - { owner: args.owner, repo: args.repo, num: args.number } + { owner: args.owner, repo: args.repo, num: args.number }, + githubHostExecOptions(args) ) if (!lookup.ok) { return { ok: false, error: lookup.error } @@ -689,7 +711,7 @@ export async function updateIssueTypeBySlug( const vars: GraphqlVars = args.issueTypeId ? { issueId, issueTypeId: args.issueTypeId } : { issueId } - const res = await runGraphql(query, vars) + const res = await runGraphql(query, vars, githubHostExecOptions(args)) if (!res.ok) { return { ok: false, error: res.error } } @@ -810,7 +832,7 @@ export async function getWorkItemDetailsBySlug( }) | null } | null - }>(query, { owner: args.owner, repo: args.repo, num: args.number }) + }>(query, { owner: args.owner, repo: args.repo, num: args.number }, githubHostExecOptions(args)) if (!res.ok) { return { ok: false, error: res.error } } diff --git a/src/main/github/project-view/project-error-classification.ts b/src/main/github/project-view/project-error-classification.ts new file mode 100644 index 00000000000..f170abec515 --- /dev/null +++ b/src/main/github/project-view/project-error-classification.ts @@ -0,0 +1,172 @@ +// Why: turns raw gh stderr/stdout into the typed GitHubProjectViewError the +// 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' + +export type GhGraphqlErrorShape = { + type?: string + message?: string + path?: (string | number)[] + extensions?: { code?: string } +} + +export function extractGraphqlErrors(stderr: string, stdout: string): GhGraphqlErrorShape[] { + // `gh api graphql` prints the response JSON to stdout even on GraphQL + // errors, and the stderr carries a summary. Try stdout first; if parsing + // fails, fall back to stderr. + const sources = [stdout, stderr] + for (const src of sources) { + if (!src) { + continue + } + try { + const parsed = JSON.parse(src) as { errors?: GhGraphqlErrorShape[] } + if (parsed.errors && parsed.errors.length > 0) { + return parsed.errors + } + } catch { + // not JSON — continue + } + } + return [] +} + +export function errorsIndicateParentField(errors: GhGraphqlErrorShape[], stderr: string): boolean { + const lower = stderr.toLowerCase() + // Preview-header shape: gh returns a 4xx with "preview" in the message. + if (lower.includes('preview') && lower.includes('parent')) { + return true + } + return errors.some((e) => { + const type = (e.type ?? '').toUpperCase() + if (type === 'FIELD_NOT_FOUND' || type === 'UNDEFINED_FIELD' || type === 'FIELD_ERRORS') { + const tail = e.path?.at(-1) + if (tail === 'parent') { + return true + } + // FIELD_ERRORS often omits `path`; match on message for the parent field. + if ((e.message ?? '').toLowerCase().includes('parent')) { + return true + } + } + return false + }) +} + +export function classifyProjectError(stderr: string, stdout: string): GitHubProjectViewError { + const errors = extractGraphqlErrors(stderr, stdout) + const s = stderr.toLowerCase() + + // Auth + if ( + s.includes('authentication required') || + s.includes('not logged in') || + s.includes('gh auth login') + ) { + return { + type: 'auth_required', + message: 'Sign in to GitHub to load project tasks. Run `gh auth login`.' + } + } + // Scope + if ( + s.includes('missing required scope') || + s.includes('your token has not been granted') || + (s.includes('resource not accessible') && (s.includes('project') || s.includes('scope'))) + ) { + return { + type: 'scope_missing', + message: + 'GitHub project access needs additional scopes. Run `gh auth refresh -s project -s read:org -s repo`.' + } + } + // Rate limit + if (s.includes('rate limit') || s.includes('api rate limit exceeded')) { + return { type: 'rate_limited', message: 'GitHub rate limit hit. Try again in a few minutes.' } + } + // Network — checked BEFORE not_found because DNS failures surface as + // "could not resolve host", which would otherwise be partially matched by + // the not_found branch's "could not resolve" check. Substring matching here + // is a one-way trapdoor: a real GraphQL "Could not resolve to a User…" + // error always contains "to a", so we tighten the not_found check below to + // require that token. + if ( + s.includes('timeout') || + s.includes('no such host') || + s.includes('network') || + s.includes('could not resolve host') || + s.includes('dial tcp') + ) { + return { type: 'network_error', message: 'Network error — check your connection.' } + } + // Not found + if ( + s.includes('http 404') || + errors.some((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') || + // Why: GitHub uses "to an" for vowel-leading types ("to an Issue", "to + // an Organization") and "to a" otherwise. The previous singular-only + // check missed the "an" variants when gh emits only the stderr summary + // without a structured GraphQL error array. See bug-scan finding 3. + /could not resolve to an? /.test(s) + ) { + const firstNotFound = errors.find((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') + return { + type: 'not_found', + message: 'Project or view not found.', + details: firstNotFound + ? { path: firstNotFound.path, code: firstNotFound.extensions?.code } + : undefined + } + } + // Validation + if (s.includes('http 422') || s.includes('validation failed')) { + return { type: 'validation_error', message: `Invalid request — ${stderr.trim()}` } + } + // GraphQL error with structured info + if (errors.length > 0) { + const first = errors[0] + return { + type: 'unknown', + message: first.message ?? 'Unknown GraphQL error.', + details: { path: first.path, code: first.extensions?.code } + } + } + // Why: don't leak full stderr to the UI — it can include verbose request + // dumps with header diagnostics. Truncate to the first non-empty line and + // cap length so unexpected diagnostics stay readable but bounded. + const firstLine = + stderr + .split('\n') + .map((l) => l.trim()) + .find((l) => l.length > 0) ?? '' + const safe = firstLine.length > 200 ? `${firstLine.slice(0, 200)}…` : firstLine + return { + type: 'unknown', + message: safe ? `GitHub request failed: ${safe}` : 'GitHub request failed.' + } +} + +export function driftError( + reason: string, + details?: { path?: (string | number)[]; code?: string } +): GitHubProjectViewError { + return { type: 'schema_drift', message: `Could not read this project view: ${reason}.`, details } +} + +// Why: the rate-limit circuit breaker short-circuits before we spawn `gh` +// when the cached snapshot says we're below the safety floor. Synthesize the +// same `rate_limited` error shape as the post-hoc classifier so the UI path +// is unchanged. We DO NOT fail open here when there's no cached snapshot — +// rateLimitGuard already handles that case (returns `blocked:false`). +export function rateLimitedError(blocked: { + remaining: number + limit: number + resetAt: number +}): GitHubProjectViewError { + const resetIn = Math.max(0, blocked.resetAt - Math.floor(Date.now() / 1000)) + const mins = Math.ceil(resetIn / 60) + return { + type: 'rate_limited', + message: `GitHub rate limit nearly exhausted (${blocked.remaining}/${blocked.limit} left). Resets in ~${mins}m.` + } +} diff --git a/src/main/github/rate-limit.test.ts b/src/main/github/rate-limit.test.ts index cddcadab361..323f729ea26 100644 --- a/src/main/github/rate-limit.test.ts +++ b/src/main/github/rate-limit.test.ts @@ -22,6 +22,12 @@ import { noteRepositoryRateLimitSpend, _resetRateLimitCache } from './rate-limit' +import { + clearGhRateLimitBlock, + getGhRateLimitBlockedUntilMs, + notifyGhPrimaryRateLimit, + type GhRateLimitBucket +} from '../git/gh-rate-limit-breaker' const PAYLOAD = JSON.stringify({ resources: { @@ -191,6 +197,39 @@ describe('shared-quota host scoping', () => { expect(spendsSharedGitHubComQuota(undefined, { wslDistro: 'Ubuntu' })).toBe(false) }) + it('treats a WSL UNC cwd as non-shared even without an explicit wslDistro', () => { + // Why: the runner derives the WSL distro from a \\wsl.localhost\... cwd, so + // gh runs inside WSL and spends that distro's quota — not the native one. + expect( + spendsSharedGitHubComQuota( + { host: 'github.com' }, + { cwd: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo' } + ) + ).toBe(false) + expect( + spendsSharedGitHubComQuota( + { host: 'github.com' }, + { cwd: '\\\\wsl$\\Ubuntu\\home\\me\\repo' } + ) + ).toBe(false) + // Regular Windows/POSIX cwds keep sharing the native github.com budget. + expect(spendsSharedGitHubComQuota({ host: 'github.com' }, { cwd: 'C:\\repos\\a' })).toBe(true) + expect(spendsSharedGitHubComQuota({ host: 'github.com' }, { cwd: '/Users/me/repo' })).toBe(true) + }) + + it('bypasses the guard and spend for a WSL UNC cwd', async () => { + await cacheExhaustedSnapshot(61) + + const cwd = '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo' + expect(repositoryRateLimitGuard({ host: 'github.com' }, 'core', { cwd })).toEqual({ + blocked: false + }) + + noteRepositoryRateLimitSpend({ host: 'github.com' }, 'core', 1, { cwd }) + const guard = rateLimitGuard('core') + expect(guard.blocked && guard.remaining).toBe(3) + }) + it('applies the shared guard for github.com but bypasses non-shared scopes', async () => { await cacheExhaustedSnapshot(61) @@ -227,3 +266,101 @@ describe('shared-quota host scoping', () => { expect(guard.blocked && guard.remaining).toBe(2) }) }) + +// Why: the breaker's reset probe must query the tripping scope's runtime/host +// (WSL distro, GHES host), record into that scope only, and never touch the +// shared native-github.com snapshot. +describe('scope-aware breaker reset probe', () => { + const WSL_SCOPE = 'wsl:ubuntu:github.com' + const GHES_SCOPE = 'native:github.acme-corp.com' + const ALL_BUCKETS: GhRateLimitBucket[] = ['core', 'search', 'graphql'] + + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + ghExecFileAsyncMock.mockReset() + _resetRateLimitCache() + }) + + afterEach(() => { + // Clear per-scope breaker state without unregistering the module-load + // probe (which _resetGhRateLimitBreaker would drop for later tests). + for (const scope of [WSL_SCOPE, GHES_SCOPE]) { + for (const bucket of ALL_BUCKETS) { + clearGhRateLimitBlock(bucket, scope) + } + } + vi.useRealTimers() + }) + + async function flushProbe(): Promise { + for (let i = 0; i < 20; i++) { + await Promise.resolve() + } + } + + it('refines a WSL-scope trip via a probe matching that scope', async () => { + // resetAt 601s > the 5-minute fallback so the refinement is observable. + ghExecFileAsyncMock.mockResolvedValue({ + stdout: JSON.stringify({ + resources: { + core: { limit: 5000, remaining: 0, reset: 601 }, + search: { limit: 30, remaining: 20, reset: 601 }, + graphql: { limit: 5000, remaining: 100, reset: 601 } + } + }) + }) + + notifyGhPrimaryRateLimit('core', WSL_SCOPE) + // Single-flight: a concurrent second trip must not spawn a second probe. + notifyGhPrimaryRateLimit('core', WSL_SCOPE) + await flushProbe() + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith(['api', 'rate_limit'], { + encoding: 'utf-8', + host: 'github.com', + wslDistro: 'ubuntu' + }) + // The exhausted core bucket is refined to the real reset for THAT scope. + expect(getGhRateLimitBlockedUntilMs('core', 1_000, WSL_SCOPE)).toBe(601_000) + // Non-exhausted buckets in the same scope stay clear. + expect(getGhRateLimitBlockedUntilMs('search', 1_000, WSL_SCOPE)).toBeNull() + // The shared native-github.com snapshot was not written: the guard has no + // cached data and fails open. + expect(rateLimitGuard('core')).toEqual({ blocked: false }) + expect(getGhRateLimitBlockedUntilMs('core', 1_000)).toBeNull() + }) + + it('fails open on a GHES probe failure and negative-caches the scope', async () => { + ghExecFileAsyncMock.mockRejectedValue(new Error('HTTP 404: Rate limiting is not enabled.')) + + notifyGhPrimaryRateLimit('core', GHES_SCOPE) + await flushProbe() + // A second trip inside the failure TTL must not spawn another probe. + notifyGhPrimaryRateLimit('core', GHES_SCOPE) + await flushProbe() + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + // Fail open: the blunt fallback block stands (5 minutes from trip time). + expect(getGhRateLimitBlockedUntilMs('core', 1_000, GHES_SCOPE)).toBe(301_000) + + // Past the TTL the scope is probed again (and can recover on success). + vi.setSystemTime(1_000 + 30_000) + ghExecFileAsyncMock.mockResolvedValue({ + stdout: JSON.stringify({ + resources: { core: { limit: 5000, remaining: 4999, reset: 700 } } + }) + }) + notifyGhPrimaryRateLimit('core', GHES_SCOPE) + await flushProbe() + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(ghExecFileAsyncMock).toHaveBeenLastCalledWith(['api', 'rate_limit'], { + encoding: 'utf-8', + host: 'github.acme-corp.com' + }) + // Budget is actually available in that scope — the block is cleared. + expect(getGhRateLimitBlockedUntilMs('core', 31_000, GHES_SCOPE)).toBeNull() + }) +}) diff --git a/src/main/github/rate-limit.ts b/src/main/github/rate-limit.ts index d3a4a4a056f..524d302c5ef 100644 --- a/src/main/github/rate-limit.ts +++ b/src/main/github/rate-limit.ts @@ -19,11 +19,14 @@ import type { GitHubRateLimitSnapshot } from '../../shared/types' import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' +import { isWslUncPath } from '../../shared/wsl-paths' import { acquire, release } from './gh-utils' import { ghExecFileAsync } from '../git/runner' import { clearGhRateLimitBlock, getGhRateLimitBlockedUntilMs, + ghRateLimitScopeKey, + parseGhRateLimitScopeKey, recordGhPrimaryRateLimit, registerGhRateLimitResetProbe, type GhRateLimitBucket @@ -73,6 +76,8 @@ function parseBucket( export function _resetRateLimitCache(): void { cached = null probeFailure = null + scopeProbeFailureAtMs.clear() + scopeRefinementInFlight.clear() } // Why: hard-stop thresholds for the circuit breaker. We refuse to issue a new @@ -167,15 +172,21 @@ export function noteRateLimitSpend(bucket: RateLimitBucketKind, cost = 1): void // breaker still protects those hosts after a real 403). export function spendsSharedGitHubComQuota( repository: { host?: string } | null | undefined, - executionOptions?: { wslDistro?: string } + executionOptions?: { wslDistro?: string; cwd?: string } ): boolean { - return isDefaultGitHubHost(repository?.host) && !executionOptions?.wslDistro + if (!isDefaultGitHubHost(repository?.host) || executionOptions?.wslDistro) { + return false + } + // Why: a \\wsl.localhost\... (or \\wsl$\...) cwd makes the runner spawn gh + // inside WSL, so it spends that distro's quota. The UNC shape is + // platform-unambiguous, so no win32 gate is needed. + return !(executionOptions?.cwd && isWslUncPath(executionOptions.cwd)) } export function repositoryRateLimitGuard( repository: { host?: string } | null | undefined, bucket: RateLimitBucketKind, - executionOptions?: { wslDistro?: string } + executionOptions?: { wslDistro?: string; cwd?: string } ): ReturnType { return spendsSharedGitHubComQuota(repository, executionOptions) ? rateLimitGuard(bucket) @@ -186,7 +197,7 @@ export function noteRepositoryRateLimitSpend( repository: { host?: string } | null | undefined, bucket: RateLimitBucketKind, cost = 1, - executionOptions?: { wslDistro?: string } + executionOptions?: { wslDistro?: string; cwd?: string } ): void { if (spendsSharedGitHubComQuota(repository, executionOptions)) { noteRateLimitSpend(bucket, cost) @@ -224,7 +235,68 @@ function refineBreakerFromSnapshot(): void { })() } -registerGhRateLimitResetProbe(() => refineBreakerFromSnapshot()) +const DEFAULT_BREAKER_SCOPE = ghRateLimitScopeKey('native', 'github.com') +// Why: non-default scopes (GHES hosts, WSL runtimes) have their own quota and +// their own probe lifecycle — single-flight and failure negative-cache are +// keyed per scope so a GHES 403 storm can't starve a WSL refinement (or vice +// versa). +const scopeRefinementInFlight = new Map>() +const scopeProbeFailureAtMs = new Map() + +function refineBreakerForScope(scope: string): void { + // Default scope keeps the existing shared-snapshot refinement path. + if (scope === DEFAULT_BREAKER_SCOPE) { + refineBreakerFromSnapshot() + return + } + const parts = parseGhRateLimitScopeKey(scope) + if (!parts || scopeRefinementInFlight.has(scope)) { + return + } + // Why: GHES with rate limiting disabled 404s every probe. Fail open (the + // fallback block stands) and don't re-probe in a tight loop while the + // breaker keeps tripping. + const failedAt = scopeProbeFailureAtMs.get(scope) + if (failedAt !== undefined && Date.now() - failedAt < RATE_LIMIT_CACHE_TTL_MS) { + return + } + const probe = (async () => { + try { + await acquire() + try { + // Run the probe on the tripping scope's runtime/host so it describes + // that account's buckets, not the native github.com ones. + const { stdout } = await ghExecFileAsync(['api', 'rate_limit'], { + encoding: 'utf-8', + host: parts.host, + ...(parts.runtime === 'wsl' ? { wslDistro: parts.wslDistro } : {}) + }) + const parsed = JSON.parse(stdout) as GhRateLimitPayload + scopeProbeFailureAtMs.delete(scope) + // Why: mirrors the default-scope refinement, but records into the + // per-scope breaker only — the shared snapshot must keep describing + // native github.com exclusively. + for (const bucket of ['core', 'search', 'graphql'] as GhRateLimitBucket[]) { + const b = parseBucket(parsed.resources?.[bucket]) + if (b.limit > 0 && b.remaining <= 0) { + recordGhPrimaryRateLimit(bucket, b.resetAt * 1000, scope) + } else if (b.limit > 0) { + clearGhRateLimitBlock(bucket, scope) + } + } + } finally { + release() + } + } catch { + scopeProbeFailureAtMs.set(scope, Date.now()) + } finally { + scopeRefinementInFlight.delete(scope) + } + })() + scopeRefinementInFlight.set(scope, probe) +} + +registerGhRateLimitResetProbe((_bucket, scope) => refineBreakerForScope(scope)) // Why: a 90-repo fan-out that primes the guard concurrently must resolve to // one `gh api rate_limit` spawn, not one per repo — the TTL cache alone can't diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index f693a750ed2..1aa3fbb43a2 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -93,6 +93,7 @@ import type { ClearProjectItemFieldArgs, DeleteIssueCommentBySlugArgs, GetProjectViewTableArgs, + ListAccessibleProjectsArgs, ListAssignableUsersBySlugArgs, ListIssueTypesBySlugArgs, ListLabelsBySlugArgs, @@ -1168,7 +1169,9 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi // Handlers never throw across IPC — every failure mode resolves through the // GitHubProjectViewError envelope. - ipcMain.handle('gh:listAccessibleProjects', () => listAccessibleProjects()) + ipcMain.handle('gh:listAccessibleProjects', (_event, args?: ListAccessibleProjectsArgs) => + listAccessibleProjects(args) + ) ipcMain.handle('gh:resolveProjectRef', (_event, args: ResolveProjectRefArgs) => resolveProjectRef(args) diff --git a/src/main/repo-icon-autodetect.test.ts b/src/main/repo-icon-autodetect.test.ts index e6abbe278f8..c491f344422 100644 --- a/src/main/repo-icon-autodetect.test.ts +++ b/src/main/repo-icon-autodetect.test.ts @@ -179,7 +179,8 @@ describe('detectRepoIcon', () => { source: 'github', label: 'stablyai/orca' }, - upstream: { owner: 'stablyai', repo: 'orca' } + // Why: fork parents resolve host-qualified so avatars/links stay on the fork's server. + upstream: { owner: 'stablyai', repo: 'orca', host: 'github.com' } }) }) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 055d1c2ac48..43c3913cd94 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -7482,7 +7482,7 @@ describe('OrcaRuntimeService', () => { kind: 'pr-link', link: { url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 } }, @@ -7490,7 +7490,7 @@ describe('OrcaRuntimeService', () => { kind: 'pr-link', link: { url: 'https://github.com/acme/orca/pull/43', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 43 } } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 8a8e8916f93..72da7f5bbef 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -600,6 +600,7 @@ import { import type { ClearProjectItemFieldArgs, GetProjectViewTableArgs, + ListAccessibleProjectsArgs, ListAssignableUsersBySlugArgs, ListIssueTypesBySlugArgs, ListLabelsBySlugArgs, @@ -14397,8 +14398,10 @@ export class OrcaRuntimeService { ) } - async listGitHubProjects(): Promise>> { - return listAccessibleProjects() + async listGitHubProjects( + args?: ListAccessibleProjectsArgs + ): Promise>> { + return listAccessibleProjects(args) } async listGitHubLabelsBySlug( diff --git a/src/main/runtime/rpc/methods/github.ts b/src/main/runtime/rpc/methods/github.ts index e10afbd82af..d48096c5c8c 100644 --- a/src/main/runtime/rpc/methods/github.ts +++ b/src/main/runtime/rpc/methods/github.ts @@ -207,6 +207,9 @@ const ProjectOwnerType = z.enum(['organization', 'user']) const ProjectViewTable = z.object({ owner: requiredString('Missing owner'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, ownerType: ProjectOwnerType, projectNumber: z.number().int().positive(), viewId: OptionalString, @@ -221,17 +224,26 @@ const ProjectWorkItemDetailsBySlug = SlugRepo.extend({ }) const ProjectRef = z.object({ - input: requiredString('Missing project reference') + input: requiredString('Missing project reference'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString }) const ProjectViews = z.object({ owner: requiredString('Missing owner'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, ownerType: ProjectOwnerType, projectNumber: z.number().int().positive() }) const ProjectItemField = z.object({ projectId: requiredString('Missing project ID'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, itemId: requiredString('Missing item ID'), fieldId: requiredString('Missing field ID'), value: z.any() @@ -239,6 +251,9 @@ const ProjectItemField = z.object({ const ClearProjectItemField = z.object({ projectId: requiredString('Missing project ID'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, itemId: requiredString('Missing item ID'), fieldId: requiredString('Missing field ID') }) @@ -246,6 +261,9 @@ const ClearProjectItemField = z.object({ const SlugIssueUpdate = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, number: z.number().int().positive(), updates: IssueUpdate }) @@ -253,6 +271,9 @@ const SlugIssueUpdate = z.object({ const SlugPullRequestUpdate = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, number: z.number().int().positive(), updates: z.object({ state: z.enum(['open', 'closed']).optional(), @@ -264,6 +285,9 @@ const SlugPullRequestUpdate = z.object({ const SlugIssueTypeUpdate = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, number: z.number().int().positive(), issueTypeId: z.string().nullable() }) @@ -271,6 +295,9 @@ const SlugIssueTypeUpdate = z.object({ const SlugIssueComment = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, number: z.number().int().positive(), body: requiredString('Comment body required') }) @@ -278,6 +305,9 @@ const SlugIssueComment = z.object({ const SlugIssueCommentEdit = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, commentId: z.number().int().positive(), body: requiredString('Comment body required') }) @@ -285,6 +315,9 @@ const SlugIssueCommentEdit = z.object({ const SlugIssueCommentDelete = z.object({ owner: requiredString('Missing owner'), repo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, commentId: z.number().int().positive() }) @@ -554,8 +587,8 @@ export const GITHUB_METHODS: RpcMethod[] = [ }), defineMethod({ name: 'github.project.listAccessible', - params: z.object({}), - handler: async (_params, { runtime }) => runtime.listGitHubProjects() + params: z.object({ host: OptionalString }), + handler: async (params, { runtime }) => runtime.listGitHubProjects(params) }), defineMethod({ name: 'github.project.listLabelsBySlug', diff --git a/src/main/source-control/forge-provider.test.ts b/src/main/source-control/forge-provider.test.ts index 95c5c9d2676..884b72f90de 100644 --- a/src/main/source-control/forge-provider.test.ts +++ b/src/main/source-control/forge-provider.test.ts @@ -81,6 +81,14 @@ import { getForgeProviderForRepository } from './forge-provider' +import { _resetOriginGitHubApiRepositoryCache } from '../github/github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('forge provider interface', () => { beforeEach(() => { createGitHubPullRequestMock.mockReset() @@ -113,8 +121,9 @@ describe('forge provider interface', () => { // slug parsing returns null. Detection must claim it via the enterprise // resolver instead of falling through to Gitea's demand for ORCA_GITEA_TOKEN. getProjectSlugMock.mockResolvedValue(null) - getRepoSlugMock.mockResolvedValue(null) - getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ + // Why: getRepoSlug resolves hosted identities itself now — a GHES remote + // comes back host-qualified instead of null + separate enterprise fallback. + getRepoSlugMock.mockResolvedValue({ owner: 'team', repo: 'orca', host: 'github.acme-corp.com' diff --git a/src/main/source-control/forge-provider.ts b/src/main/source-control/forge-provider.ts index a7d22ad48ae..2d05c9c804d 100644 --- a/src/main/source-control/forge-provider.ts +++ b/src/main/source-control/forge-provider.ts @@ -22,7 +22,6 @@ import { } from '../gitea/client' import { createGiteaPullRequest } from '../gitea/pull-request-creation' import { createGitHubPullRequest, getPRForBranchOutcome, getRepoSlug } from '../github/client' -import { getEnterpriseGitHubRepoSlug } from '../github/github-enterprise-repository' import { getMergeRequest, getMergeRequestForBranch, getProjectSlug } from '../gitlab/client' import { createGitLabMergeRequest } from '../gitlab/merge-request-creation' import { @@ -123,25 +122,11 @@ function unwrapGitHubPRForBranchOutcome( const gitHubForgeProvider = { id: 'github', supportsReviewCreation: true, - resolveRepository: async (context) => { - const slug = await getRepoSlug( - context.repoPath, - context.connectionId, - ...hostedReviewExecutionArgs(context) - ) - if (slug) { - return slug - } - // Why: GHES remotes live on a custom host, so github.com-only slug parsing - // misses them and detection would otherwise fall through to Gitea (#8312). - // Claim the repo when gh is authenticated to its host — the same signal - // GitLab uses for self-hosted instances. - return getEnterpriseGitHubRepoSlug( - context.repoPath, - context.connectionId, - ...hostedReviewExecutionArgs(context) - ) - }, + // Why: getRepoSlug resolves hosted identities — GHES remotes are claimed when + // gh is authenticated to their host (the same signal GitLab uses for + // self-hosted instances), so detection never falls through to Gitea (#8312). + resolveRepository: async (context) => + getRepoSlug(context.repoPath, context.connectionId, ...hostedReviewExecutionArgs(context)), async getReviewForBranch(input) { const fallbackReviewNumber = input.linkedReviewNumber == null ? (input.fallbackReviewNumber ?? null) : null diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts index 010d950d50f..595fa820132 100644 --- a/src/main/source-control/hosted-review-creation-eligibility.test.ts +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -117,6 +117,14 @@ vi.mock('./hosted-review', () => ({ import { getHostedReviewCreationEligibility } from './hosted-review-creation' +import { _resetOriginGitHubApiRepositoryCache } from '../github/github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + function resetMocks(): void { for (const mock of [ createGitHubPullRequestMock, @@ -155,10 +163,17 @@ function mockGitHubProvider(): void { // resolver claims the repo and reports the host for the gh auth probe (#8312). function mockGitHubEnterpriseProvider(): void { getProjectSlugMock.mockResolvedValue(null) - getRepoSlugMock.mockResolvedValue(null) + // Why: getRepoSlug resolves hosted identities itself now — a GHES remote + // comes back host-qualified instead of null + separate enterprise fallback. + getRepoSlugMock.mockResolvedValue({ + owner: 'acme', + repo: 'orca', + host: 'github.acme-corp.com' + }) getBitbucketRepoSlugMock.mockResolvedValue(null) getAzureDevOpsRepoSlugMock.mockResolvedValue(null) getGiteaRepoSlugMock.mockResolvedValue(null) + // The auth gate still keys off the enterprise resolver (authed-GHES signal). getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ owner: 'acme', repo: 'orca', diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index abfcd11935e..cb593ae6a00 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -117,6 +117,14 @@ vi.mock('./hosted-review', () => ({ import { createHostedReview } from './hosted-review-creation' +import { _resetOriginGitHubApiRepositoryCache } from '../github/github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + function resetMocks(): void { for (const mock of [ createGitHubPullRequestMock, @@ -155,10 +163,17 @@ function mockGitHubProvider(): void { // resolver claims the repo and reports the host for the gh auth probe (#8312). function mockGitHubEnterpriseProvider(): void { getProjectSlugMock.mockResolvedValue(null) - getRepoSlugMock.mockResolvedValue(null) + // Why: getRepoSlug resolves hosted identities itself now — a GHES remote + // comes back host-qualified instead of null + separate enterprise fallback. + getRepoSlugMock.mockResolvedValue({ + owner: 'acme', + repo: 'orca', + host: 'github.acme-corp.com' + }) getBitbucketRepoSlugMock.mockResolvedValue(null) getAzureDevOpsRepoSlugMock.mockResolvedValue(null) getGiteaRepoSlugMock.mockResolvedValue(null) + // The auth gate still keys off the enterprise resolver (authed-GHES signal). getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ owner: 'acme', repo: 'orca', diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 7c03b6f8879..35cf0cb2dbe 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -254,6 +254,7 @@ import type { GetProjectViewTableResult, GitHubProjectCommentMutationResult, GitHubProjectMutationResult, + ListAccessibleProjectsArgs, ListAccessibleProjectsResult, ListAssignableUsersBySlugArgs, ListAssignableUsersBySlugResult, @@ -1728,7 +1729,9 @@ export type PreloadApi = { */ diagnoseAuth: () => Promise // ── ProjectV2 (GitHub Projects) ───────────────────────────────── - listAccessibleProjects: () => Promise + listAccessibleProjects: ( + args?: ListAccessibleProjectsArgs + ) => Promise resolveProjectRef: (args: ResolveProjectRefArgs) => Promise listProjectViews: (args: ListProjectViewsArgs) => Promise getProjectViewTable: (args: GetProjectViewTableArgs) => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 1395d808906..ebfa0c65853 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -110,6 +110,7 @@ import type { GetProjectViewTableResult, GitHubProjectCommentMutationResult, GitHubProjectMutationResult, + ListAccessibleProjectsArgs, ListAccessibleProjectsResult, ListAssignableUsersBySlugArgs, ListAssignableUsersBySlugResult, @@ -1538,8 +1539,10 @@ const api = { diagnoseAuth: (): Promise => ipcRenderer.invoke('gh:diagnoseAuth'), // ── ProjectV2 (GitHub Projects) ─────────────────────────────────── - listAccessibleProjects: (): Promise => - ipcRenderer.invoke('gh:listAccessibleProjects'), + listAccessibleProjects: ( + args?: ListAccessibleProjectsArgs + ): Promise => + ipcRenderer.invoke('gh:listAccessibleProjects', args), resolveProjectRef: (args: ResolveProjectRefArgs): Promise => ipcRenderer.invoke('gh:resolveProjectRef', args), listProjectViews: (args: ListProjectViewsArgs): Promise => diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index e43cc6a8089..2695d989cee 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -1516,19 +1516,22 @@ function formatPRDelta(item: GitHubWorkItem): string | null { } function ReviewChipAvatar({ - reviewer + reviewer, + avatarHost }: { reviewer: GitHubPRPrimaryReviewer | null + avatarHost?: string }): React.JSX.Element { if (reviewer?.login) { - // Why: `gh pr list --json reviewRequests` can return only logins. Prefer the - // API avatar_url so GHE renders; GitHubUserAvatar falls back to the login URL - // and then an initials placeholder when no avatar loads. See #8784. + // Why: review requests can contain only logins; use the PR host for the + // fallback while GitHubUserAvatar handles failed images with initials. + const avatarUrl = + reviewer.avatarUrl || `https://${avatarHost ?? 'github.com'}/${reviewer.login}.png?size=40` return ( @@ -2523,7 +2526,7 @@ function PRReviewCell({ > {primaryReviewer ? ( <> - + {extraReviewerCount > 0 ? ( +{extraReviewerCount} diff --git a/src/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.ts b/src/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.ts index e01aa2029ff..f806b994e02 100644 --- a/src/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.ts +++ b/src/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.ts @@ -366,7 +366,7 @@ describe('startParkedTerminalByteWatcher', () => { expect.objectContaining({ url: 'https://github.com/orca-dev/orca/pull/421', number: 421, - slug: { owner: 'orca-dev', repo: 'orca' } + slug: { owner: 'orca-dev', repo: 'orca', host: 'github.com' } }) ) dispose() diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 18e99dac5a7..5e13075d90a 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -1060,7 +1060,7 @@ describe('connectPanePty', () => { 'wt-1', expect.objectContaining({ url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 }) ) diff --git a/src/renderer/src/lib/github-links.test.ts b/src/renderer/src/lib/github-links.test.ts index 64a9bd59df3..f4492b1cc76 100644 --- a/src/renderer/src/lib/github-links.test.ts +++ b/src/renderer/src/lib/github-links.test.ts @@ -19,6 +19,12 @@ describe('buildGitHubRepoUrl', () => { 'https://github.com/stably%20ai/orca%2Ftools' ) }) + + it('links hosted slugs to their GitHub Enterprise server', () => { + expect(buildGitHubRepoUrl({ owner: 'team', repo: 'orca', host: 'github.acme-corp.com' })).toBe( + 'https://github.acme-corp.com/team/orca' + ) + }) }) describe('parseGitHubIssueOrPRNumber', () => { @@ -66,7 +72,7 @@ describe('parseGitHubIssueOrPRNumber', () => { describe('parseGitHubIssueOrPRLink', () => { it('parses slug, number, and type for direct item URLs', () => { expect(parseGitHubIssueOrPRLink('https://github.com/stablyai/orca/pull/123')).toEqual({ - slug: { owner: 'stablyai', repo: 'orca' }, + slug: { owner: 'stablyai', repo: 'orca', host: 'github.com' }, number: 123, type: 'pr' }) @@ -74,18 +80,18 @@ describe('parseGitHubIssueOrPRLink', () => { expect( parseGitHubIssueOrPRLink('https://github.my-company.net/MyOrg/my_repo/pull/395') ).toEqual({ - slug: { owner: 'MyOrg', repo: 'my_repo' }, + slug: { owner: 'MyOrg', repo: 'my_repo', host: 'github.my-company.net' }, number: 395, type: 'pr' }) expect(parseGitHubIssueOrPRLink('https://git.corp.com/MyOrg/my_repo/pull/395')).toEqual({ - slug: { owner: 'MyOrg', repo: 'my_repo' }, + slug: { owner: 'MyOrg', repo: 'my_repo', host: 'git.corp.com' }, number: 395, type: 'pr' }) expect(parseGitHubIssueOrPRLink('https://github.com/stablyai/orca/issues/923')).toEqual({ - slug: { owner: 'stablyai', repo: 'orca' }, + slug: { owner: 'stablyai', repo: 'orca', host: 'github.com' }, number: 923, type: 'issue' }) @@ -93,12 +99,12 @@ describe('parseGitHubIssueOrPRLink', () => { it('derives item type from the route segment when trailing segments are present', () => { expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/1965/changes')).toEqual({ - slug: { owner: 'o', repo: 'r' }, + slug: { owner: 'o', repo: 'r', host: 'github.com' }, number: 1965, type: 'pr' }) expect(parseGitHubIssueOrPRLink('https://github.com/o/r/issues/923/comments')).toEqual({ - slug: { owner: 'o', repo: 'r' }, + slug: { owner: 'o', repo: 'r', host: 'github.com' }, number: 923, type: 'issue' }) @@ -107,13 +113,13 @@ describe('parseGitHubIssueOrPRLink', () => { it('accepts query, fragment, and repeated trailing slashes', () => { expect(parseGitHubIssueOrPRLink('https://github.com/o/r/pull/1965/files?plain=1#diff')).toEqual( { - slug: { owner: 'o', repo: 'r' }, + slug: { owner: 'o', repo: 'r', host: 'github.com' }, number: 1965, type: 'pr' } ) expect(parseGitHubIssueOrPRLink('https://github.com/o/r/issues/923/comments///')).toEqual({ - slug: { owner: 'o', repo: 'r' }, + slug: { owner: 'o', repo: 'r', host: 'github.com' }, number: 923, type: 'issue' }) @@ -135,7 +141,7 @@ describe('normalizeGitHubLinkQuery', () => { query: 'https://github.com/stablyai/orca/issues/923', directNumber: 923, directLink: { - slug: { owner: 'stablyai', repo: 'orca' }, + slug: { owner: 'stablyai', repo: 'orca', host: 'github.com' }, number: 923, type: 'issue' } @@ -147,7 +153,7 @@ describe('normalizeGitHubLinkQuery', () => { query: 'https://github.com/stablyai/orca/pull/6934', directNumber: 6934, directLink: { - slug: { owner: 'stablyai', repo: 'orca' }, + slug: { owner: 'stablyai', repo: 'orca', host: 'github.com' }, number: 6934, type: 'pr' } @@ -159,7 +165,7 @@ describe('normalizeGitHubLinkQuery', () => { query: 'HTTPS://github.com/stablyai/orca/pull/6934', directNumber: 6934, directLink: { - slug: { owner: 'stablyai', repo: 'orca' }, + slug: { owner: 'stablyai', repo: 'orca', host: 'github.com' }, number: 6934, type: 'pr' } diff --git a/src/shared/github-links.ts b/src/shared/github-links.ts index a53562f5d5d..8f122ba4ac8 100644 --- a/src/shared/github-links.ts +++ b/src/shared/github-links.ts @@ -6,6 +6,7 @@ const GH_ITEM_PATH_RE = /^\/([^/]+)\/([^/]+)\/(issues|pull)\/(\d+)(?:\/.*)?$/i export type RepoSlug = { owner: string repo: string + host?: string } export type GitHubIssueOrPRLink = { @@ -18,7 +19,10 @@ export function buildGitHubRepoUrl(slug: RepoSlug | null | undefined): string | if (!slug?.owner || !slug.repo) { return null } - return `https://github.com/${encodeURIComponent(slug.owner)}/${encodeURIComponent(slug.repo)}` + // Why: hosted identities carry the GHES host; links must point at that + // server, not github.com. + const host = slug.host ?? 'github.com' + return `https://${host}/${encodeURIComponent(slug.owner)}/${encodeURIComponent(slug.repo)}` } function matchGitHubItemPath(url: URL): RegExpExecArray | null { @@ -95,7 +99,9 @@ export function parseGitHubIssueOrPRLink(input: string): GitHubIssueOrPRLink | n } return { - slug: { owner: match[1], repo: match[2] }, + // Why: the URL proves the host — GHES links must keep their server identity + // instead of being treated as github.com slugs. + slug: { owner: match[1], repo: match[2], host: url.hostname }, type: match[3].toLowerCase() === 'pull' ? 'pr' : 'issue', number } diff --git a/src/shared/github-project-types.ts b/src/shared/github-project-types.ts index 3067aa95260..960a061d8ed 100644 --- a/src/shared/github-project-types.ts +++ b/src/shared/github-project-types.ts @@ -331,6 +331,8 @@ export type GetProjectViewTableArgs = { owner: string ownerType: GitHubProjectOwnerType projectNumber: number + /** GitHub host (e.g. GHES); absent means github.com. */ + host?: string /** View selection precedence: viewId > viewNumber > viewName > first * TABLE_LAYOUT view. */ viewId?: string @@ -346,12 +348,14 @@ export type GetProjectViewTableArgs = { export type ProjectWorkItemDetailsBySlugArgs = { owner: string repo: string + host?: string number: number type: 'issue' | 'pr' } export type UpdateProjectItemFieldArgs = { projectId: string + host?: string itemId: string fieldId: string value: GitHubProjectFieldMutationValue @@ -359,6 +363,7 @@ export type UpdateProjectItemFieldArgs = { export type ClearProjectItemFieldArgs = { projectId: string + host?: string itemId: string fieldId: string } @@ -366,6 +371,7 @@ export type ClearProjectItemFieldArgs = { export type UpdateIssueBySlugArgs = { owner: string repo: string + host?: string number: number updates: GitHubIssueUpdate & { body?: string } } @@ -373,6 +379,7 @@ export type UpdateIssueBySlugArgs = { export type UpdatePullRequestBySlugArgs = { owner: string repo: string + host?: string number: number updates: { title?: string; body?: string; state?: 'open' | 'closed' } } @@ -380,6 +387,7 @@ export type UpdatePullRequestBySlugArgs = { export type AddIssueCommentBySlugArgs = { owner: string repo: string + host?: string number: number body: string } @@ -387,6 +395,7 @@ export type AddIssueCommentBySlugArgs = { export type UpdateIssueCommentBySlugArgs = { owner: string repo: string + host?: string commentId: number body: string } @@ -394,28 +403,33 @@ export type UpdateIssueCommentBySlugArgs = { export type DeleteIssueCommentBySlugArgs = { owner: string repo: string + host?: string commentId: number } export type ListLabelsBySlugArgs = { owner: string repo: string + host?: string } export type ListAssignableUsersBySlugArgs = { owner: string repo: string + host?: string seedLogins?: string[] } export type ListIssueTypesBySlugArgs = { owner: string repo: string + host?: string } export type UpdateIssueTypeBySlugArgs = { owner: string repo: string + host?: string number: number /** null clears the issue type. */ issueTypeId: string | null @@ -423,10 +437,17 @@ export type UpdateIssueTypeBySlugArgs = { export type ResolveProjectRefArgs = { input: string + host?: string } export type ListProjectViewsArgs = { owner: string ownerType: GitHubProjectOwnerType projectNumber: number + host?: string +} + +export type ListAccessibleProjectsArgs = { + /** GitHub host (e.g. GHES); absent means github.com. */ + host?: string } diff --git a/src/shared/repo-icon.ts b/src/shared/repo-icon.ts index 01ac83c1392..7c8edb709dd 100644 --- a/src/shared/repo-icon.ts +++ b/src/shared/repo-icon.ts @@ -31,10 +31,11 @@ export function faviconUrlFromWebsite(rawUrl: string): string | null { // Why: the GitHub owner avatar is the default repo icon, built the same way in // main (auto-detect) and renderer (picker); keep the URL and label in one place. -export function githubAvatarIcon(slug: { owner: string; repo: string }): RepoIcon { +export function githubAvatarIcon(slug: { owner: string; repo: string; host?: string }): RepoIcon { + // Why: GHES serves the same /.png avatar convention as github.com. return { type: 'image', - src: `https://github.com/${encodeURIComponent(slug.owner)}.png?size=64`, + src: `https://${slug.host ?? 'github.com'}/${encodeURIComponent(slug.owner)}.png?size=64`, source: 'github', label: `${slug.owner}/${slug.repo}` } diff --git a/src/shared/terminal-github-pr-link-detector.test.ts b/src/shared/terminal-github-pr-link-detector.test.ts index 360fd16d63e..23a00fe493d 100644 --- a/src/shared/terminal-github-pr-link-detector.test.ts +++ b/src/shared/terminal-github-pr-link-detector.test.ts @@ -14,7 +14,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('Created https://github.com/acme/orca/pull/42\r\n')).toEqual([ { url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 } ]) @@ -26,7 +26,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe(`${issue8126Url}\x1b[22m\n`)).toEqual([ { url: issue8126Url, - slug: { owner: 'owner', repo: 'repo' }, + slug: { owner: 'owner', repo: 'repo', host: 'github.com' }, number: 10 } ]) @@ -39,7 +39,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('[22m\n')).toEqual([ { url: issue8126Url, - slug: { owner: 'owner', repo: 'repo' }, + slug: { owner: 'owner', repo: 'repo', host: 'github.com' }, number: 10 } ]) @@ -73,7 +73,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe(`${issue8126Url}\x1b[22m\n${issue8126Url}\n`)).toEqual([ { url: issue8126Url, - slug: { owner: 'owner', repo: 'repo' }, + slug: { owner: 'owner', repo: 'repo', host: 'github.com' }, number: 10 } ]) @@ -86,7 +86,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('2\r\n')).toEqual([ { url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 } ]) @@ -99,7 +99,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('ub.com/acme/orca/pull/42\n')).toEqual([ { url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 } ]) @@ -132,7 +132,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('Created https://github.my-company.net/MyOrg/my_repo/pull/395\r\n')).toEqual([ { url: 'https://github.my-company.net/MyOrg/my_repo/pull/395', - slug: { owner: 'MyOrg', repo: 'my_repo' }, + slug: { owner: 'MyOrg', repo: 'my_repo', host: 'github.my-company.net' }, number: 395 } ]) @@ -144,7 +144,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('Created http://github.internal/MyOrg/my_repo/pull/395\r\n')).toEqual([ { url: 'http://github.internal/MyOrg/my_repo/pull/395', - slug: { owner: 'MyOrg', repo: 'my_repo' }, + slug: { owner: 'MyOrg', repo: 'my_repo', host: 'github.internal' }, number: 395 } ]) @@ -156,7 +156,8 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe('Created https://github.internal:8443/MyOrg/my_repo/pull/397\r\n')).toEqual([ { url: 'https://github.internal:8443/MyOrg/my_repo/pull/397', - slug: { owner: 'MyOrg', repo: 'my_repo' }, + // Ports are dropped to match parseGitHubRemoteIdentity's hostname identity. + slug: { owner: 'MyOrg', repo: 'my_repo', host: 'github.internal' }, number: 397 } ]) @@ -170,7 +171,7 @@ describe('createTerminalGitHubPRLinkDetector', () => { expect(observe(`${noise}Created https://github.com/acme/orca/pull/42\r\n`)).toEqual([ { url: 'https://github.com/acme/orca/pull/42', - slug: { owner: 'acme', repo: 'orca' }, + slug: { owner: 'acme', repo: 'orca', host: 'github.com' }, number: 42 } ])