From 4684617fed412fac2eb35012085d49da11c944e0 Mon Sep 17 00:00:00 2001 From: Gabriel Brown Date: Sat, 11 Jul 2026 13:42:02 -0400 Subject: [PATCH] fix(sync): paginate compare and resolve real head SHA --- packages/backend/convex/githubClient.ts | 43 ++++++++-- packages/backend/convex/githubSync.ts | 11 ++- .../backend/tests/unit/github-head.test.ts | 86 ++++++++++++++++++- 3 files changed, 128 insertions(+), 12 deletions(-) diff --git a/packages/backend/convex/githubClient.ts b/packages/backend/convex/githubClient.ts index 4fd06c8..e82f0d2 100644 --- a/packages/backend/convex/githubClient.ts +++ b/packages/backend/convex/githubClient.ts @@ -117,6 +117,9 @@ const normalizeCompareCommit = ( htmlUrl: commit.html_url, }); +const COMPARE_PER_PAGE = 100; +const COMPARE_MAX_PAGES = 30; // bound: up to 3000 commits collected + export const compareAcrossForkNetwork = async ( octokit: Octokit, args: { @@ -125,24 +128,46 @@ export const compareAcrossForkNetwork = async ( baseOwner: string; baseBranch: string; headOwner: string; + headRepo: string; headBranch: string; }, ): Promise => { const basehead = `${args.baseOwner}:${args.baseBranch}...${args.headOwner}:${args.headBranch}`; - const result = await octokit.rest.repos.compareCommitsWithBasehead({ + const first = await octokit.rest.repos.compareCommitsWithBasehead({ owner: args.owner, repo: args.repo, basehead, - per_page: 100, + per_page: COMPARE_PER_PAGE, + page: 1, }); - const commits = result.data.commits.map(normalizeCompareCommit); + const rawCommits = [...first.data.commits]; + const totalCommits = first.data.total_commits; + let page = 2; + while (rawCommits.length < totalCommits && page <= COMPARE_MAX_PAGES) { + const next = await octokit.rest.repos.compareCommitsWithBasehead({ + owner: args.owner, + repo: args.repo, + basehead, + per_page: COMPARE_PER_PAGE, + page, + }); + if (next.data.commits.length === 0) break; + rawCommits.push(...next.data.commits); + page += 1; + } + const head = await resolveBranchHead( + octokit, + args.headOwner, + args.headRepo, + args.headBranch, + ); return { - aheadBy: result.data.ahead_by, - mergeBaseSha: result.data.merge_base_commit.sha, - headSha: commits[commits.length - 1]?.sha, - baseSha: result.data.base_commit.sha, - htmlUrl: result.data.html_url, - commits, + aheadBy: first.data.ahead_by, + mergeBaseSha: first.data.merge_base_commit.sha, + headSha: head.sha, + baseSha: first.data.base_commit.sha, + htmlUrl: first.data.html_url, + commits: rawCommits.map(normalizeCompareCommit), }; }; diff --git a/packages/backend/convex/githubSync.ts b/packages/backend/convex/githubSync.ts index 9f07e28..ac79d7c 100644 --- a/packages/backend/convex/githubSync.ts +++ b/packages/backend/convex/githubSync.ts @@ -32,8 +32,13 @@ const toStatus = (upstreamAheadBy: number, forkAheadBy: number) => { return 'unknown' as const; }; -const getLastCommitAt = (compare: GitHubCompareSummary) => - compare.commits[compare.commits.length - 1]?.committedAt; +const getLastCommitAt = (compare: GitHubCompareSummary) => { + const head = compare.commits.find((c) => c.sha === compare.headSha); + return ( + head?.committedAt ?? + compare.commits[compare.commits.length - 1]?.committedAt + ); +}; const ensureForkMetadata = (spoon: Doc<'spoons'>) => { if (!spoon.forkOwner || !spoon.forkRepo) { @@ -121,6 +126,7 @@ const refreshOwnedSpoon = async ( baseOwner: forkOwner, baseBranch: resolvedForkBranch, headOwner: spoon.upstreamOwner, + headRepo: spoon.upstreamRepo, headBranch: upstreamBranch, }), compareAcrossForkNetwork(octokit, { @@ -129,6 +135,7 @@ const refreshOwnedSpoon = async ( baseOwner: spoon.upstreamOwner, baseBranch: upstreamBranch, headOwner: forkOwner, + headRepo: forkRepo, headBranch: resolvedForkBranch, }), listPullRequests(octokit, { owner: forkOwner, repo: forkRepo }), diff --git a/packages/backend/tests/unit/github-head.test.ts b/packages/backend/tests/unit/github-head.test.ts index 9e6fd60..4f7b8c4 100644 --- a/packages/backend/tests/unit/github-head.test.ts +++ b/packages/backend/tests/unit/github-head.test.ts @@ -1,7 +1,10 @@ import type { Octokit } from '@octokit/rest'; import { describe, expect, it, vi } from 'vitest'; -import { resolveBranchHead } from '../../convex/githubClient'; +import { + compareAcrossForkNetwork, + resolveBranchHead, +} from '../../convex/githubClient'; describe('resolveBranchHead', () => { it('returns the branch tip sha from the branch ref', async () => { @@ -22,3 +25,84 @@ describe('resolveBranchHead', () => { }); }); }); + +const makeCommit = (sha: string) => ({ + sha, + commit: { + message: `commit ${sha}`, + author: { name: 'Dev', email: 'dev@example.com', date: '2026-01-01T00:00:00Z' }, + committer: { name: 'Dev', email: 'dev@example.com', date: '2026-01-01T00:00:00Z' }, + }, + author: { login: 'dev' }, + html_url: `https://github.com/o/r/commit/${sha}`, +}); + +describe('compareAcrossForkNetwork', () => { + it('paginates the compare and takes headSha from the branch tip, not the last commit', async () => { + const page1 = Array.from({ length: 100 }, (_, i) => makeCommit(`c${i}`)); + // Last commit in the collected list is a DIFFERENT sha than the branch tip. + const page2 = Array.from({ length: 50 }, (_, i) => + makeCommit(i === 49 ? 'LAST_COMMIT_NOT_TIP' : `c${100 + i}`), + ); + + const compareCommitsWithBasehead = vi.fn( + async ({ page }: { page?: number }) => { + const commits = page === 2 ? page2 : page1; + return { + data: { + total_commits: 150, + ahead_by: 150, + merge_base_commit: { sha: 'MERGE_BASE' }, + base_commit: { sha: 'BASE' }, + html_url: 'https://github.com/o/r/compare/base...head', + commits, + }, + }; + }, + ); + const getBranch = vi.fn(async () => ({ + data: { commit: { sha: 'TIP' } }, + })); + const octokit = { + rest: { repos: { compareCommitsWithBasehead, getBranch } }, + } as unknown as Octokit; + + const result = await compareAcrossForkNetwork(octokit, { + owner: 'up', + repo: 'upstream-repo', + baseOwner: 'fork', + baseBranch: 'main', + headOwner: 'up', + headRepo: 'upstream-repo', + headBranch: 'main', + }); + + // Core bug: head is the branch tip, NOT the last commit in the compare list. + expect(result.headSha).toBe('TIP'); + expect(result.commits[result.commits.length - 1]?.sha).toBe( + 'LAST_COMMIT_NOT_TIP', + ); + expect(result.headSha).not.toBe('LAST_COMMIT_NOT_TIP'); + + // ahead_by / merge base / base come from the first compare page. + expect(result.aheadBy).toBe(150); + expect(result.mergeBaseSha).toBe('MERGE_BASE'); + expect(result.baseSha).toBe('BASE'); + + // All commits collected across both pages. + expect(result.commits).toHaveLength(150); + + // Pagination is bounded and the tip is resolved via a single branch ref call. + expect(getBranch).toHaveBeenCalledTimes(1); + expect(getBranch).toHaveBeenCalledWith({ + owner: 'up', + repo: 'upstream-repo', + branch: 'main', + }); + expect(compareCommitsWithBasehead).toHaveBeenCalledTimes(2); + expect(compareCommitsWithBasehead.mock.calls[0]?.[0]).toMatchObject({ + per_page: 100, + page: 1, + }); + }); +});