diff --git a/electron/__tests__/pull-request-review.test.ts b/electron/__tests__/pull-request-review.test.ts index 26c6a25..17d4f3f 100644 --- a/electron/__tests__/pull-request-review.test.ts +++ b/electron/__tests__/pull-request-review.test.ts @@ -155,3 +155,97 @@ process.stdin.on('end', () => { await removeGitTestDirectory(directory); } }); + +test('matches pull requests to remotes with custom SSH users or through gh', async () => { + const directory = await mkdtemp(join(tmpdir(), 'codiff-pull-request-review-')); + const sshUserRepo = join(directory, 'ssh-user-repo'); + const aliasedRepo = join(directory, 'aliased-repo'); + const fakeBin = join(directory, 'bin'); + const fakeGh = join(fakeBin, 'gh'); + const callsPath = join(directory, 'calls.jsonl'); + const previousPath = process.env.PATH; + const previousCallsPath = process.env.CODIFF_GITHUB_REVIEW_TEST_CALLS; + + try { + await Promise.all([mkdir(sshUserRepo), mkdir(aliasedRepo), mkdir(fakeBin)]); + await execFileAsync('git', ['-C', sshUserRepo, 'init']); + await execFileAsync('git', [ + '-C', + sshUserRepo, + 'remote', + 'add', + 'origin', + 'org-12345@github.com:acme/widgets.git', + ]); + await execFileAsync('git', ['-C', aliasedRepo, 'init']); + await execFileAsync('git', [ + '-C', + aliasedRepo, + 'remote', + 'add', + 'origin', + 'git@github-work:acme/widgets.git', + ]); + await writeFile( + fakeGh, + `#!/usr/bin/env node +const { appendFileSync } = require('node:fs'); +const args = process.argv.slice(2); +let input = ''; +process.stdin.setEncoding('utf8'); +process.stdin.on('data', (chunk) => { input += chunk; }); +process.stdin.on('end', () => { + appendFileSync( + process.env.CODIFF_GITHUB_REVIEW_TEST_CALLS, + JSON.stringify({ args, input }) + '\\n', + ); + process.stdout.write( + args[0] === 'repo' && args[1] === 'view' + ? '{"owner":{"login":"acme"},"name":"widgets"}' + : '{}', + ); +}); +`, + ); + await chmod(fakeGh, 0o755); + process.env.PATH = `${fakeBin}:${previousPath ?? ''}`; + process.env.CODIFF_GITHUB_REVIEW_TEST_CALLS = callsPath; + + const review = { + body: 'Looks good.', + comments: [], + event: 'APPROVE' as const, + source: { + provider: 'github' as const, + type: 'pull-request' as const, + url: 'https://github.com/acme/widgets/pull/42', + }, + }; + + await submitPullRequestReview(sshUserRepo, review); + await submitPullRequestReview(aliasedRepo, review); + + const calls = (await readFile(callsPath, 'utf8')) + .trim() + .split('\n') + .map((line) => JSON.parse(line) as { args: ReadonlyArray; input: string }); + expect(calls).toHaveLength(3); + // The custom SSH user matches through remote parsing alone — no gh lookup. + expect(calls[0].args).toContain('repos/acme/widgets/pulls/42/reviews'); + // The aliased host hides github.com, so the repository resolves through gh. + expect(calls[1].args.join(' ')).toBe('repo view --json owner,name'); + expect(calls[2].args).toContain('repos/acme/widgets/pulls/42/reviews'); + } finally { + if (previousPath == null) { + delete process.env.PATH; + } else { + process.env.PATH = previousPath; + } + if (previousCallsPath == null) { + delete process.env.CODIFF_GITHUB_REVIEW_TEST_CALLS; + } else { + process.env.CODIFF_GITHUB_REVIEW_TEST_CALLS = previousCallsPath; + } + await removeGitTestDirectory(directory); + } +}); diff --git a/electron/git-state/pull-request.cjs b/electron/git-state/pull-request.cjs index 8040ad0..c52eb96 100644 --- a/electron/git-state/pull-request.cjs +++ b/electron/git-state/pull-request.cjs @@ -64,7 +64,9 @@ const parseGitHubPullRequestUrl = (value) => { /** @param {string} value @returns {GitHubRemote | null} */ const parseGitHubRemoteUrl = (value) => { const trimmed = value.trim(); - const sshMatch = trimmed.match(/^git@github\.com:([^/]+)\/(.+?)(?:\.git)?$/i); + // GitHub organizations with SSO issue SSH remotes as `org-@github.com` + // instead of `git@github.com`, so both users are recognized. + const sshMatch = trimmed.match(/^(?:git|org-\d+)@github\.com:([^/]+)\/(.+?)(?:\.git)?$/i); if (sshMatch) { return { owner: sshMatch[1], @@ -118,6 +120,62 @@ const getRemotePriority = (remote) => ? 2 : 3; +/** @param {string} repoRoot @returns {Promise} */ +const readGitHubRepositoryReference = (repoRoot) => + new Promise((resolve, reject) => { + const child = spawn('gh', ['repo', 'view', '--json', 'owner,name'], { + cwd: repoRoot, + stdio: ['ignore', 'pipe', 'pipe'], + }); + /** @type {Array} */ + const stdout = []; + /** @type {Array} */ + const stderr = []; + + child.stdout.on('data', (chunk) => stdout.push(chunk)); + child.stderr.on('data', (chunk) => stderr.push(chunk)); + child.on('error', reject); + child.on('close', (code) => { + if (code !== 0) { + const errorOutput = Buffer.concat(stderr).toString('utf8').trim(); + reject(new Error(errorOutput || `gh repo view exited with code ${code}.`)); + return; + } + + try { + const data = JSON.parse(Buffer.concat(stdout).toString('utf8')); + const owner = data?.owner?.login; + const repo = data?.name; + if (owner && repo) { + resolve({ owner, repo }); + return; + } + reject(new Error('gh repo view did not return the repository owner and name.')); + } catch (error) { + reject(error instanceof Error ? error : new Error('gh repo view returned invalid JSON.')); + } + }); + }); + +/** @type {Map>} */ +const repositoryReferenceCache = new Map(); + +/** @param {string} repoRoot @param {PullRequestReference} pullRequest @returns {Promise} */ +const gitHubRepositoryMatchesPullRequest = async (repoRoot, pullRequest) => { + let reference = repositoryReferenceCache.get(repoRoot); + if (!reference) { + reference = readGitHubRepositoryReference(repoRoot).catch(() => null); + repositoryReferenceCache.set(repoRoot, reference); + } + + const repository = await reference; + return ( + repository != null && + repository.owner.toLowerCase() === pullRequest.owner.toLowerCase() && + repository.repo.toLowerCase() === pullRequest.repo.toLowerCase() + ); +}; + /** @param {string} repoRoot @param {PullRequestReference} pullRequest @returns {Promise} */ const selectPullRequestRemote = async (repoRoot, pullRequest) => { const remotes = await readLocalGitHubRemotes(repoRoot); @@ -129,13 +187,25 @@ const selectPullRequestRemote = async (repoRoot, pullRequest) => { ) .sort((left, right) => getRemotePriority(left) - getRemotePriority(right))[0]; - if (!remote) { - throw new Error( - `Pull request ${pullRequest.owner}/${pullRequest.repo} does not match a GitHub remote in this repository.`, - ); + if (remote) { + return remote; } - return remote; + // Remotes using SSH host aliases or `insteadOf` rewrites hide the github.com + // host from URL parsing. Only when such a remote exists, ask gh to resolve + // the repository and fall back to that remote's name so git can fetch. + const names = (await gitOrEmpty(repoRoot, ['remote'])) + .split('\n') + .map((name) => name.trim()) + .filter((name) => name && !remotes.some((remote) => remote.name === name)); + const name = names.includes('origin') ? 'origin' : names[0]; + if (name && (await gitHubRepositoryMatchesPullRequest(repoRoot, pullRequest))) { + return { direction: 'fetch', name, owner: pullRequest.owner, repo: pullRequest.repo }; + } + + throw new Error( + `Pull request ${pullRequest.owner}/${pullRequest.repo} does not match a GitHub remote in this repository.`, + ); }; /** @param {string} repoRoot @param {PullRequestReference} pullRequest */