diff --git a/src/github/data/pr-fetcher.ts b/src/github/data/pr-fetcher.ts index 44c9364c..facbfaa7 100644 --- a/src/github/data/pr-fetcher.ts +++ b/src/github/data/pr-fetcher.ts @@ -1,6 +1,7 @@ import type { Octokits } from "../api/client"; import { PR_QUERY, REPO_DEFAULT_BRANCH_QUERY } from "../api/queries/github"; import type { GitHubPullRequest } from "../types"; +import { retryWithBackoff } from "../../utils/retry"; /** * Represents the PR data needed by fill and review commands @@ -16,9 +17,125 @@ export type PRBranchData = { type PullRequestQueryResponse = { repository: { pullRequest: GitHubPullRequest | null; - }; + } | null; }; +type RepoDefaultBranchQueryResponse = { + repository: { + defaultBranchRef: { + name: string; + } | null; + } | null; +}; + +const GRAPHQL_RETRY_OPTIONS = { + maxAttempts: 3, + initialDelayMs: 1000, + maxDelayMs: 4000, +}; + +type OctokitHttpResponse = { + status: number; + headers: Record; + data: unknown; +}; + +/** + * Structural type for an `@octokit/request` hook. Octokit types + * `request.hook` as `any`, so it is spelled out here to keep the capture below + * type-checked. + */ +type OctokitRequestHook = ( + request: (options: unknown) => Promise, + options: unknown, +) => Promise; + +/** + * Summarizes a value for an error message without dumping a whole response. + */ +function describePayload(payload: unknown): string { + if (payload === undefined) { + return "undefined"; + } + if (payload === null) { + return "null"; + } + if (typeof payload === "string") { + const preview = payload.slice(0, 200); + return `string(${JSON.stringify(preview)}${payload.length > 200 ? "…" : ""})`; + } + if (typeof payload !== "object") { + return `${typeof payload}(${String(payload)})`; + } + return `object with keys [${Object.keys(payload).join(", ")}]`; +} + +/** + * Runs a GraphQL query and returns one top-level field of the response. + * + * `@octokit/graphql` resolves `response.data.data` and only throws when the + * body carries an `errors` array, so an HTTP 200 whose body is not a GraphQL + * envelope (an HTML error page, or JSON without a `data` key) resolves + * `undefined`. Dereferencing that directly turned a transient GitHub answer + * into a `TypeError` that killed the whole action step, so the shape is checked + * here and the request is retried: the same query has been observed to succeed + * minutes earlier in the same job. + * + * A present-but-null field is returned as-is; GraphQL nulls a field the token + * may not read, which is a stable "not found" answer rather than a broken + * response, and the caller reports it without burning retries. + */ +async function fetchGraphQLField({ + octokits, + query, + variables, + field, + queryDescription, +}: { + octokits: Octokits; + query: string; + variables: Record; + field: string; + queryDescription: string; +}): Promise { + return retryWithBackoff(async () => { + // The HTTP response is captured through a request hook because + // `@octokit/graphql` hands back only the `data` member, which leaves an + // unexpected body impossible to diagnose from the action log. + let httpStatus: number | undefined; + let httpContentType: string | undefined; + let httpBody: unknown; + + const captureResponse: OctokitRequestHook = async (request, options) => { + const response = await request(options); + httpStatus = response.status; + httpContentType = String(response.headers["content-type"] ?? ""); + httpBody = response.data; + return response; + }; + + const payload = await octokits.graphql>(query, { + ...variables, + request: { hook: captureResponse }, + }); + + if ( + payload === null || + typeof payload !== "object" || + !(field in payload) + ) { + throw new Error( + `${queryDescription} returned no "${field}" field ` + + `(GraphQL data: ${describePayload(payload)}; ` + + `HTTP ${httpStatus ?? "unknown"} ${httpContentType || "unknown content-type"}, ` + + `body: ${describePayload(httpBody)})`, + ); + } + + return payload[field] as T; + }, GRAPHQL_RETRY_OPTIONS); +} + /** * Fetches PR branch information needed for fill/review commands. * This is a focused function that only retrieves the branch names and SHA @@ -35,21 +152,26 @@ export async function fetchPRBranchData({ prNumber: number; }): Promise { try { - const prResult = await octokits.graphql( - PR_QUERY, - { + const repositoryResult = await fetchGraphQLField< + PullRequestQueryResponse["repository"] + >({ + octokits, + query: PR_QUERY, + variables: { owner: repository.owner, repo: repository.repo, number: prNumber, }, - ); + field: "repository", + queryDescription: `PR query for ${repository.owner}/${repository.repo}#${prNumber}`, + }); + + const pullRequest = repositoryResult?.pullRequest; - if (!prResult.repository.pullRequest) { + if (!pullRequest) { throw new Error(`PR #${prNumber} not found`); } - const pullRequest = prResult.repository.pullRequest; - return { baseRefName: pullRequest.baseRefName, headRefName: pullRequest.headRefName, @@ -59,18 +181,14 @@ export async function fetchPRBranchData({ }; } catch (error) { console.error(`Failed to fetch PR branch data:`, error); - throw new Error(`Failed to fetch PR branch data for PR #${prNumber}`); + throw new Error( + `Failed to fetch PR branch data for PR #${prNumber}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); } } -type RepoDefaultBranchQueryResponse = { - repository: { - defaultBranchRef: { - name: string; - } | null; - }; -}; - /** * Fetches the repository's default branch name. * Used by security-scan which operates without a PR context. @@ -83,25 +201,34 @@ export async function fetchRepoDefaultBranch({ repository: { owner: string; repo: string }; }): Promise { try { - const result = await octokits.graphql( - REPO_DEFAULT_BRANCH_QUERY, - { + const repositoryResult = await fetchGraphQLField< + RepoDefaultBranchQueryResponse["repository"] + >({ + octokits, + query: REPO_DEFAULT_BRANCH_QUERY, + variables: { owner: repository.owner, repo: repository.repo, }, - ); + field: "repository", + queryDescription: `Default branch query for ${repository.owner}/${repository.repo}`, + }); + + const defaultBranchRef = repositoryResult?.defaultBranchRef; - if (!result.repository.defaultBranchRef) { + if (!defaultBranchRef) { throw new Error( `Default branch not found for ${repository.owner}/${repository.repo}`, ); } - return result.repository.defaultBranchRef.name; + return defaultBranchRef.name; } catch (error) { console.error(`Failed to fetch default branch:`, error); throw new Error( - `Failed to fetch default branch for ${repository.owner}/${repository.repo}`, + `Failed to fetch default branch for ${repository.owner}/${repository.repo}: ${ + error instanceof Error ? error.message : String(error) + }`, ); } } diff --git a/test/github/data/pr-fetcher.test.ts b/test/github/data/pr-fetcher.test.ts new file mode 100644 index 00000000..354eb047 --- /dev/null +++ b/test/github/data/pr-fetcher.test.ts @@ -0,0 +1,296 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import { graphql } from "@octokit/graphql"; +import { Octokit } from "@octokit/rest"; +import type { Octokits } from "../../../src/github/api/client"; +import { + fetchPRBranchData, + fetchRepoDefaultBranch, +} from "../../../src/github/data/pr-fetcher"; + +type StubResponse = { + status?: number; + contentType?: string; + body: string; +}; + +/** + * Builds an Octokits whose GraphQL client talks to a stubbed fetch, so the real + * @octokit/graphql envelope handling stays under test. + */ +function createStubOctokits(responses: StubResponse[]): { + octokits: Octokits; + requestCount: () => number; +} { + let calls = 0; + + const stubFetch = async () => { + const response = responses[Math.min(calls, responses.length - 1)]!; + calls += 1; + return new Response(response.body, { + status: response.status ?? 200, + headers: { + "content-type": response.contentType ?? "application/json", + }, + }); + }; + + return { + octokits: { + rest: new Octokit({ auth: "stub-token" }), + graphql: graphql.defaults({ + headers: { authorization: "token stub-token" }, + request: { fetch: stubFetch }, + }), + }, + requestCount: () => calls, + }; +} + +function prEnvelope(pullRequest: Record | null): StubResponse { + return { body: JSON.stringify({ data: { repository: { pullRequest } } }) }; +} + +const VALID_PR = { + title: "fix(frontend): stabilize session navigation", + body: "description", + baseRefName: "dev", + headRefName: "ross/app-1930-session-nav", + headRefOid: "abc123", +}; + +const REPOSITORY = { owner: "Factory-AI", repo: "factory-mono" }; + +describe("pr-fetcher", () => { + let consoleErrorSpy: ReturnType; + let consoleLogSpy: ReturnType; + + beforeEach(() => { + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(() => { + consoleErrorSpy.mockRestore(); + consoleLogSpy.mockRestore(); + }); + + describe("fetchPRBranchData", () => { + it("returns the branch data from a well formed response", async () => { + const { octokits, requestCount } = createStubOctokits([ + prEnvelope(VALID_PR), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result).toEqual({ + baseRefName: "dev", + headRefName: "ross/app-1930-session-nav", + headRefOid: "abc123", + title: "fix(frontend): stabilize session navigation", + body: "description", + }); + expect(requestCount()).toBe(1); + }); + + it("defaults a null PR body to an empty string", async () => { + const { octokits } = createStubOctokits([ + prEnvelope({ ...VALID_PR, body: null }), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result.body).toBe(""); + }); + + it("reports the response instead of crashing when a 200 carries no GraphQL envelope", async () => { + const { octokits } = createStubOctokits([ + { + contentType: "text/html", + body: "unavailable", + }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + // The crash signature this guards against: a TypeError from + // dereferencing `prResult.repository` on an undefined payload. + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain( + 'PR query for Factory-AI/factory-mono#19051 returned no "repository" field', + ); + expect(error!.message).toContain("HTTP 200 text/html"); + expect(error!.message).toContain("unavailable"); + expect(error!.message).toContain( + "Failed to fetch PR branch data for PR #19051", + ); + }, 15000); + + it("reports the response when a 200 body is JSON without a data member", async () => { + const { octokits } = createStubOctokits([{ body: "{}" }]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain('returned no "repository" field'); + }, 15000); + + it("recovers when only the first attempt returns a malformed response", async () => { + const { octokits, requestCount } = createStubOctokits([ + { contentType: "text/html", body: "unavailable" }, + prEnvelope(VALID_PR), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result.headRefName).toBe("ross/app-1930-session-nav"); + expect(requestCount()).toBe(2); + }, 15000); + + it("does not retry a null repository, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([ + { body: JSON.stringify({ data: { repository: null } }) }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain("PR #19051 not found"); + expect(requestCount()).toBe(1); + }); + + it("does not retry a null pull request, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([prEnvelope(null)]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain("PR #19051 not found"); + expect(requestCount()).toBe(1); + }); + + it("surfaces GraphQL error messages returned alongside a null data member", async () => { + const { octokits } = createStubOctokits([ + { + body: JSON.stringify({ + data: null, + errors: [{ message: "Bad credentials" }], + }), + }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain("Bad credentials"); + }, 15000); + }); + + describe("fetchRepoDefaultBranch", () => { + it("returns the default branch name", async () => { + const { octokits } = createStubOctokits([ + { + body: JSON.stringify({ + data: { repository: { defaultBranchRef: { name: "dev" } } }, + }), + }, + ]); + + await expect( + fetchRepoDefaultBranch({ octokits, repository: REPOSITORY }), + ).resolves.toBe("dev"); + }); + + it("reports the response instead of crashing when a 200 carries no GraphQL envelope", async () => { + const { octokits } = createStubOctokits([ + { contentType: "text/html", body: "unavailable" }, + ]); + + const error = await fetchRepoDefaultBranch({ + octokits, + repository: REPOSITORY, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain( + 'Default branch query for Factory-AI/factory-mono returned no "repository" field', + ); + expect(error!.message).toContain( + "Failed to fetch default branch for Factory-AI/factory-mono", + ); + }, 15000); + + it("does not retry a null repository, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([ + { body: JSON.stringify({ data: { repository: null } }) }, + ]); + + const error = await fetchRepoDefaultBranch({ + octokits, + repository: REPOSITORY, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain( + "Default branch not found for Factory-AI/factory-mono", + ); + expect(requestCount()).toBe(1); + }); + }); +});