diff --git a/packages/code-review/src/aggregate.test.ts b/packages/code-review/src/aggregate.test.ts index e4ab18b31..d1c328cc6 100644 --- a/packages/code-review/src/aggregate.test.ts +++ b/packages/code-review/src/aggregate.test.ts @@ -23,6 +23,7 @@ const DIFF: PullRequestDiff = { changedLines: [1, 2, 3], }, ], + truncated: false, }; function pass(id: string, report: unknown): ReviewerPass { @@ -279,3 +280,32 @@ test("a fingerprint already posted is skipped on a re-run", () => { expect(review.body).not.toContain("already flagged"); expect(review.comments).toEqual([]); }); + +test("an untruncated diff and comment page post no incompleteness note", () => { + const review = aggregateReview( + [pass("correctness", { summary: "read it", findings: [] })], + DIFF, + ); + expect(review.body).not.toContain("may be incomplete"); +}); + +test("a truncated diff surfaces an incompleteness note in the review body", () => { + const review = aggregateReview( + [pass("correctness", { summary: "read it", findings: [] })], + { ...DIFF, truncated: true }, + ); + expect(review.body).toContain( + "This review may be incomplete: the pull request has more changed " + + "files or already-posted comments than one review pass reads", + ); +}); + +test("a truncated already-posted-comments page surfaces the same note", () => { + const review = aggregateReview( + [pass("correctness", { summary: "read it", findings: [] })], + DIFF, + new Set(), + true, + ); + expect(review.body).toContain("This review may be incomplete"); +}); diff --git a/packages/code-review/src/aggregate.ts b/packages/code-review/src/aggregate.ts index cd169189f..fe36a10d8 100644 --- a/packages/code-review/src/aggregate.ts +++ b/packages/code-review/src/aggregate.ts @@ -195,6 +195,16 @@ function commentBody(entry: AggregatedFinding, diff: PullRequestDiff): string { ); } +// GitHub caps a paginated fetch at `fetchAllPages`' page bound; a pull +// request past it reads back partial. Silence would let this review +// read as complete when it saw only part of the change, or re-flag a +// finding whose earlier post fell outside the read page — both worse +// than saying so plainly. +const TRUNCATED_NOTE = + "_This review may be incomplete: the pull request has more changed " + + "files or already-posted comments than one review pass reads, so " + + "some results may be partial or repeated._"; + function countLine(findings: readonly AggregatedFinding[]): string { if (findings.length === 0) { return "No findings — the reviewers read the change and had nothing to raise."; @@ -219,6 +229,7 @@ export function aggregateReview( passes: readonly ReviewerPass[], diff: PullRequestDiff, alreadyPosted: ReadonlySet = new Set(), + commentsTruncated = false, ): PullRequestReviewDraft { const collected = collect(passes, alreadyPosted); const sections: string[] = [ @@ -227,6 +238,10 @@ export function aggregateReview( countLine(collected.findings), ]; + if (diff.truncated || commentsTruncated) { + sections.push("", TRUNCATED_NOTE); + } + for (const severity of SEVERITY_ORDER) { const forSeverity = collected.findings.filter( (entry) => entry.finding.severity === severity, diff --git a/packages/code-review/src/prompt.test.ts b/packages/code-review/src/prompt.test.ts index 96bd19522..f9827fbd6 100644 --- a/packages/code-review/src/prompt.test.ts +++ b/packages/code-review/src/prompt.test.ts @@ -27,6 +27,7 @@ function diffOf(files: readonly PullRequestFileDiff[]): PullRequestDiff { headSha: "headsha", baseSha: "basesha", files, + truncated: false, }; } diff --git a/packages/code-review/src/review-run.test.ts b/packages/code-review/src/review-run.test.ts index 88cca4404..0c626507e 100644 --- a/packages/code-review/src/review-run.test.ts +++ b/packages/code-review/src/review-run.test.ts @@ -31,6 +31,7 @@ const DIFF: PullRequestDiff = { changedLines: [1, 2], }, ], + truncated: false, }; interface FakeGitHub { @@ -47,6 +48,7 @@ interface FakeGitHub { function fakeGitHub( diff: PullRequestDiff = DIFF, postedComments: readonly string[] = [], + commentsTruncated = false, ): FakeGitHub { const posted: FakeGitHub["posted"][number][] = []; const diffReads: PullRequestRef[] = []; @@ -69,7 +71,10 @@ function fakeGitHub( }, listPostedComments: (ref) => { listedComments.push(ref); - return Promise.resolve(postedComments); + return Promise.resolve({ + comments: postedComments, + truncated: commentsTruncated, + }); }, }, }; @@ -221,3 +226,35 @@ test("a finding whose fingerprint was already posted is not raised again", async expect(result.review.body).toContain("release-risk finding"); expect(result.review.body).not.toContain("architecture finding"); }); + +test("a truncated diff surfaces an incompleteness note in the posted review", async () => { + const github = fakeGitHub({ ...DIFF, truncated: true }); + + const result = await runPullRequestReview( + { + github: github.client, + runReviewerTurn: ({ reviewer }) => + Promise.resolve(reportFor(reviewer.id)), + }, + REF, + ); + + if (result.skipped) throw new Error("expected the review to run"); + expect(result.review.body).toContain("This review may be incomplete"); +}); + +test("a truncated already-posted-comments page surfaces the same note", async () => { + const github = fakeGitHub(DIFF, [], true); + + const result = await runPullRequestReview( + { + github: github.client, + runReviewerTurn: ({ reviewer }) => + Promise.resolve(reportFor(reviewer.id)), + }, + REF, + ); + + if (result.skipped) throw new Error("expected the review to run"); + expect(result.review.body).toContain("This review may be incomplete"); +}); diff --git a/packages/code-review/src/review-run.ts b/packages/code-review/src/review-run.ts index b90ce070e..c995b427a 100644 --- a/packages/code-review/src/review-run.ts +++ b/packages/code-review/src/review-run.ts @@ -21,6 +21,7 @@ import type { PostedPullRequestReview, PullRequestDiff, PullRequestRef, + PullRequestReviewCommentsPage, PullRequestReviewDraft, } from "@corbits/github-tools"; @@ -39,7 +40,9 @@ export interface CodeReviewGitHub { review: PullRequestReviewDraft, ): Promise; /** Bodies of every review comment already posted, for the fingerprint scan. */ - listPostedComments(ref: PullRequestRef): Promise; + listPostedComments( + ref: PullRequestRef, + ): Promise; } /** Runs one reviewer's turn and returns its raw reply. */ @@ -111,13 +114,17 @@ export async function runPullRequestReview( }; } const prompt = renderReviewPrompt(diff); - const alreadyPosted = fingerprintsIn( - await deps.github.listPostedComments(ref), - ); + const postedComments = await deps.github.listPostedComments(ref); + const alreadyPosted = fingerprintsIn(postedComments.comments); const passes = await Promise.all( reviewers.map((reviewer) => runOne(deps, reviewer, prompt)), ); - const review = aggregateReview(passes, diff, alreadyPosted); + const review = aggregateReview( + passes, + diff, + alreadyPosted, + postedComments.truncated, + ); const posted = await deps.github.postReview(ref, diff.headSha, review); return { skipped: false, diff, passes, review, posted }; } diff --git a/packages/github-tools/package.json b/packages/github-tools/package.json index b91dba576..9d270daea 100644 --- a/packages/github-tools/package.json +++ b/packages/github-tools/package.json @@ -2,7 +2,7 @@ "name": "@corbits/github-tools", "private": true, "description": "GitHub search integration: a minimal REST client and an @intx/agent tool bundle exposing github_activity", - "version": "0.0.6", + "version": "0.0.8", "license": "LGPL-2.1-or-later", "type": "module", "exports": { diff --git a/packages/github-tools/src/index.ts b/packages/github-tools/src/index.ts index 793ee5ecd..609d5e10c 100644 --- a/packages/github-tools/src/index.ts +++ b/packages/github-tools/src/index.ts @@ -15,6 +15,7 @@ export type { PullRequestFileDiff, PullRequestRef, PullRequestReviewComment, + PullRequestReviewCommentsPage, PullRequestReviewDraft, } from "./pull-requests"; export { diff --git a/packages/github-tools/src/pull-requests.test.ts b/packages/github-tools/src/pull-requests.test.ts index 6463e0a02..995b5341e 100644 --- a/packages/github-tools/src/pull-requests.test.ts +++ b/packages/github-tools/src/pull-requests.test.ts @@ -176,7 +176,7 @@ test("postPullRequestReview posts one comment-only review at the head sha", asyn }); test("fetchPullRequestReviewComments returns each comment's body", async () => { - const bodies = await fetchPullRequestReviewComments( + const page = await fetchPullRequestReviewComments( { apiKey: "token", baseUrl: BASE, @@ -186,7 +186,173 @@ test("fetchPullRequestReviewComments returns each comment's body", async () => { }, { owner: "acme", repo: "widgets", number: 7 }, ); - expect(bodies).toEqual(["first", "second"]); + expect(page).toEqual({ comments: ["first", "second"], truncated: false }); +}); + +/** A page short enough to end pagination whichever way the caller detects it. */ +function shortPage(bodies: string[], link?: string): Response { + return new Response(JSON.stringify(bodies.map((body) => ({ body }))), { + status: 200, + headers: link === undefined ? {} : { link }, + }); +} + +/** A full (100-item) page, to drive the page-number fallback when no `Link` header is sent. */ +function fullPage(prefix: string): Response { + const bodies = Array.from({ length: 100 }, (_, i) => ({ + body: `${prefix}-${String(i)}`, + })); + return new Response(JSON.stringify(bodies), { status: 200 }); +} + +test('fetchPullRequestReviewComments follows a Link: rel="next" header across pages', async () => { + const requestedUrls: string[] = []; + const page = await fetchPullRequestReviewComments( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch((input) => { + const url = String(input); + requestedUrls.push(url); + if (url.includes("page=2")) { + return Promise.resolve(shortPage(["third", "fourth"])); + } + return Promise.resolve( + shortPage(["first", "second"], `<${BASE}/next?page=2>; rel="next"`), + ); + }), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(page).toEqual({ + comments: ["first", "second", "third", "fourth"], + truncated: false, + }); + expect(requestedUrls).toHaveLength(2); +}); + +test("fetchPullRequestReviewComments falls back to page= when no Link header is sent", async () => { + let calls = 0; + const page = await fetchPullRequestReviewComments( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch(() => { + calls += 1; + return Promise.resolve( + calls === 1 ? fullPage("p1") : shortPage(["last"]), + ); + }), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(page.comments).toHaveLength(101); + expect(page.comments[100]).toBe("last"); + expect(page.truncated).toBe(false); + expect(calls).toBe(2); +}); + +test("fetchPullRequestReviewComments reports truncated when the page bound is hit", async () => { + const page = await fetchPullRequestReviewComments( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch(() => Promise.resolve(fullPage("p"))), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(page.truncated).toBe(true); + expect(page.comments).toHaveLength(3000); +}); + +test("fetchPullRequestDiff follows pagination for files and reports it unset for one page", async () => { + const diff = await fetchPullRequestDiff( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch((input) => { + const url = String(input); + if (url.includes("/files")) { + return Promise.resolve( + jsonResponse([ + { + filename: "src/loop.ts", + status: "modified", + additions: 2, + deletions: 1, + patch: PATCH, + }, + ]), + ); + } + return Promise.resolve(jsonResponse(PULL_BODY)); + }), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(diff.truncated).toBe(false); + expect(diff.files).toHaveLength(1); +}); + +function fileAt(index: number): Record { + return { + filename: `src/file-${String(index)}.ts`, + status: "modified", + additions: 1, + deletions: 0, + }; +} + +test("fetchPullRequestDiff merges every page of changed files", async () => { + const diff = await fetchPullRequestDiff( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch((input) => { + const url = String(input); + if (url.includes("page=2")) { + return Promise.resolve( + new Response(JSON.stringify([fileAt(100)]), { status: 200 }), + ); + } + if (url.includes("/files")) { + const files = Array.from({ length: 100 }, (_, i) => fileAt(i)); + return Promise.resolve( + new Response(JSON.stringify(files), { + status: 200, + headers: { + link: `<${BASE}/repos/acme/widgets/pulls/7/files?page=2>; rel="next"`, + }, + }), + ); + } + return Promise.resolve(jsonResponse(PULL_BODY)); + }), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(diff.files).toHaveLength(101); + expect(diff.truncated).toBe(false); +}); + +test("fetchPullRequestDiff reports truncated when the file-page bound is hit", async () => { + const diff = await fetchPullRequestDiff( + { + apiKey: "token", + baseUrl: BASE, + fetchImpl: fakeFetch((input) => { + const url = String(input); + if (url.includes("/files")) { + const files = Array.from({ length: 100 }, (_, i) => fileAt(i)); + return Promise.resolve(jsonResponse(files)); + } + return Promise.resolve(jsonResponse(PULL_BODY)); + }), + }, + { owner: "acme", repo: "widgets", number: 7 }, + ); + expect(diff.truncated).toBe(true); + expect(diff.files).toHaveLength(3000); }); test("postPullRequestReview refuses an empty body", async () => { diff --git a/packages/github-tools/src/pull-requests.ts b/packages/github-tools/src/pull-requests.ts index 450d12468..d47ec4240 100644 --- a/packages/github-tools/src/pull-requests.ts +++ b/packages/github-tools/src/pull-requests.ts @@ -17,7 +17,13 @@ import { type } from "arktype"; import type { GitHubClientConfig } from "./client"; const DEFAULT_BASE_URL = "https://api.github.com"; -const MAX_FILES_PER_PAGE = 100; +const MAX_ITEMS_PER_PAGE = 100; +/** + * Upper bound on how many pages a paginated fetch follows. A pull + * request with more than 3,000 changed files or review comments is + * pathological; past that we stop and say so rather than loop forever. + */ +const MAX_PAGES = 30; const PullRequestResponse = type({ title: "string", @@ -72,6 +78,8 @@ export interface PullRequestDiff { readonly headSha: string; readonly baseSha: string; readonly files: readonly PullRequestFileDiff[]; + /** True when the file list hit the page bound and may be incomplete. */ + readonly truncated: boolean; } /** One inline comment on a posted review. */ @@ -92,6 +100,12 @@ export interface PostedPullRequestReview { readonly url: string; } +export interface PullRequestReviewCommentsPage { + readonly comments: readonly string[]; + /** True when the comment list hit the page bound and may be incomplete. */ + readonly truncated: boolean; +} + const PULL_REQUEST_URL = /^https?:\/\/github\.com\/([^/\s]+)\/([^/\s]+)\/pull\/(\d+)(?:[/?#].*)?$/; @@ -153,13 +167,19 @@ function headers(apiKey: string | undefined): Record { return base; } -async function requestJSON( +/** + * Fetches a URL and parses its body as JSON, throwing one consistently + * worded error for a non-2xx response. The one seam every GitHub call in + * this module goes through, so `requestJSON` and `fetchAllPages` report + * a transport failure identically instead of each spelling it out. + */ +async function fetchJSON( config: GitHubClientConfig, url: URL, init: { readonly method: string; readonly body?: string }, -): Promise { +): Promise<{ readonly response: Response; readonly body: unknown }> { const doFetch = config.fetchImpl ?? fetch; - const response = await doFetch(url, { + const response: Response = await doFetch(url, { method: init.method, headers: headers(config.apiKey), ...(init.body === undefined ? {} : { body: init.body }), @@ -170,7 +190,73 @@ async function requestJSON( `${String(response.status)} ${response.statusText}`, ); } - return response.json(); + return { response, body: await response.json() }; +} + +async function requestJSON( + config: GitHubClientConfig, + url: URL, + init: { readonly method: string; readonly body?: string }, +): Promise { + return (await fetchJSON(config, url, init)).body; +} + +const NEXT_LINK = /<([^>]+)>\s*;\s*rel="next"/; + +/** Reads the `rel="next"` URL out of a GitHub `Link` response header. */ +function nextPageUrl(linkHeader: string | null): URL | null { + if (linkHeader === null) return null; + const match = NEXT_LINK.exec(linkHeader); + return match?.[1] === undefined ? null : new URL(match[1]); +} + +/** The next page to request when a paginated endpoint sent no `Link`. */ +function fallbackNextPage(url: URL, pageItemCount: number): URL | null { + if (pageItemCount < MAX_ITEMS_PER_PAGE) return null; + const next = new URL(url); + const currentPage = Number(next.searchParams.get("page") ?? "1"); + next.searchParams.set("page", String(currentPage + 1)); + return next; +} + +/** + * Follows a paginated GitHub list endpoint to completion: the `Link` + * header's `rel="next"` when GitHub sends one, otherwise successive + * `page=` requests until a page comes back short. Stops at `MAX_PAGES` + * and reports `truncated: true` rather than looping forever against a + * pathological pull request. + */ +async function fetchAllPages( + config: GitHubClientConfig, + initialUrl: URL, +): Promise<{ + readonly items: readonly unknown[]; + readonly truncated: boolean; +}> { + const items: unknown[] = []; + let url: URL | null = initialUrl; + let pageCount = 0; + let truncated = false; + + while (url !== null) { + if (pageCount >= MAX_PAGES) { + truncated = true; + break; + } + pageCount += 1; + const { response, body: page } = await fetchJSON(config, url, { + method: "GET", + }); + if (!Array.isArray(page)) { + throw new Error(`GitHub GET ${url.pathname} returned a non-array page`); + } + items.push(...page); + url = + nextPageUrl(response.headers.get("link")) ?? + fallbackNextPage(url, page.length); + } + + return { items, truncated }; } function baseOf(config: GitHubClientConfig): string { @@ -208,11 +294,11 @@ export async function fetchPullRequestDiff( const pullUrl = new URL(`${base}${pullPath(ref)}`); const filesUrl = new URL(`${base}${pullPath(ref)}/files`); - filesUrl.searchParams.set("per_page", String(MAX_FILES_PER_PAGE)); + filesUrl.searchParams.set("per_page", String(MAX_ITEMS_PER_PAGE)); - const [pullRaw, filesRaw] = await Promise.all([ + const [pullRaw, filesPage] = await Promise.all([ requestJSON(config, pullUrl, { method: "GET" }), - requestJSON(config, filesUrl, { method: "GET" }), + fetchAllPages(config, filesUrl), ]); const pull = PullRequestResponse(pullRaw); @@ -221,7 +307,7 @@ export async function fetchPullRequestDiff( `GitHub pull-request response did not match the expected shape: ${pull.summary}`, ); } - const files = PullRequestFilesResponse(filesRaw); + const files = PullRequestFilesResponse(filesPage.items); if (files instanceof type.errors) { throw new Error( `GitHub pull-request files response did not match the expected shape: ${files.summary}`, @@ -237,6 +323,7 @@ export async function fetchPullRequestDiff( headSha: pull.head.sha, baseSha: pull.base.sha, files: files.map(toFileDiff), + truncated: filesPage.truncated, }; } @@ -251,17 +338,20 @@ const ReviewCommentsResponse = ReviewCommentResponse.array(); export async function fetchPullRequestReviewComments( config: GitHubClientConfig, ref: PullRequestRef, -): Promise { +): Promise { const url = new URL(`${baseOf(config)}${pullPath(ref)}/comments`); - url.searchParams.set("per_page", String(MAX_FILES_PER_PAGE)); - const raw = await requestJSON(config, url, { method: "GET" }); - const comments = ReviewCommentsResponse(raw); + url.searchParams.set("per_page", String(MAX_ITEMS_PER_PAGE)); + const page = await fetchAllPages(config, url); + const comments = ReviewCommentsResponse(page.items); if (comments instanceof type.errors) { throw new Error( `GitHub pull-request comments response did not match the expected shape: ${comments.summary}`, ); } - return comments.map((comment) => comment.body); + return { + comments: comments.map((comment) => comment.body), + truncated: page.truncated, + }; } /** diff --git a/workflows/code-review/src/index.ts b/workflows/code-review/src/index.ts index 62acb6afe..b0a6b4e92 100644 --- a/workflows/code-review/src/index.ts +++ b/workflows/code-review/src/index.ts @@ -43,7 +43,7 @@ export const CODE_REVIEW_STEP_ID = "code-review"; /** The tool packages this definition pins: GitHub reach, nothing else. */ export const CODE_REVIEW_TOOL_PACKAGE_PINS: readonly ToolPackagePin[] = [ - { name: "@corbits/github-tools", version: "0.0.6" }, + { name: "@corbits/github-tools", version: "0.0.8" }, ]; /** Binds the pinned package's "github" handle to the tenant's connection. */ diff --git a/workflows/last-30-days-research/src/index.ts b/workflows/last-30-days-research/src/index.ts index b7180e719..20df4ca38 100644 --- a/workflows/last-30-days-research/src/index.ts +++ b/workflows/last-30-days-research/src/index.ts @@ -145,7 +145,7 @@ export const LAST_30_DAYS_RESEARCH_PENDING_SOURCES = [ export const LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS: readonly ToolPackagePin[] = [ { name: "@corbits/web-search-tools", version: "0.0.3" }, - { name: "@corbits/github-tools", version: "0.0.6" }, + { name: "@corbits/github-tools", version: "0.0.8" }, ]; const SYSTEM_PROMPT = [ diff --git a/workflows/last-30-days-research/test/definition.test.ts b/workflows/last-30-days-research/test/definition.test.ts index 1263a7a6c..424c44ce6 100644 --- a/workflows/last-30-days-research/test/definition.test.ts +++ b/workflows/last-30-days-research/test/definition.test.ts @@ -58,7 +58,7 @@ test("the step carries an explicit per-turn timeout, no inline tools, and the tw expect(only.agent.capabilities).toEqual([]); expect(LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS).toEqual([ { name: "@corbits/web-search-tools", version: "0.0.3" }, - { name: "@corbits/github-tools", version: "0.0.6" }, + { name: "@corbits/github-tools", version: "0.0.8" }, ]); expect(only.agent.toolPackagePins).toEqual( LAST_30_DAYS_RESEARCH_TOOL_PACKAGE_PINS,