diff --git a/services/api/src/github/kickstart.ts b/services/api/src/github/kickstart.ts index 50721f57..137f564a 100644 --- a/services/api/src/github/kickstart.ts +++ b/services/api/src/github/kickstart.ts @@ -1,8 +1,10 @@ import { renderWorkspaceKickstart, sha256Hex } from "@facility/core"; import { type FacilityDb, githubInstallations } from "@facility/db"; import { eq } from "drizzle-orm"; +import { ApiError } from "../errors.js"; import type { AppConfig, Principal } from "../types.js"; import { FacilityGithubClient, type GithubClientFactory, type TreeItem } from "./client.js"; +import { githubRateLimitRetryAt } from "./rate-limit.js"; import { readRepoFiles } from "./repo-files.js"; export type KickstartAnswers = { @@ -35,7 +37,13 @@ export async function createGithubClientForRepo( factory: GithubClientFactory, repository: GithubRepositoryRow, ): Promise { - if (!repository.installationId) throw new Error("Repository has no GitHub installation"); + if (!repository.installationId) { + throw new ApiError( + 409, + "github_installation_missing", + `${slug(repository)} is not connected to a GitHub App installation. Reconnect the repository before running kickstart.`, + ); + } const installation = ( await db .select() @@ -44,7 +52,11 @@ export async function createGithubClientForRepo( .limit(1) )[0]; if (!installation || installation.orgId !== repository.orgId || installation.suspendedAt) { - throw new Error("GitHub installation is unavailable"); + throw new ApiError( + 409, + "github_installation_unavailable", + `The GitHub App installation for ${slug(repository)} is suspended or belongs to another organization.`, + ); } return new FacilityGithubClient(await factory(installation.installationId), { owner: repository.owner, @@ -59,9 +71,12 @@ export async function kickstartPreview( repository: GithubRepositoryRow, answers: KickstartAnswers, ) { - const client = await createGithubClientForRepo(db, factory, repository); + const ref = answers.defaultBranch ?? repository.defaultBranch; + const client = await createGithubClientForRepo(db, factory, repository).catch((error) => { + throw kickstartFailure(error, repository, ref, "base"); + }); const existing = await readRepoFiles(client, repository.defaultBranch); - const detection = detectWorkspace(existing, answers.defaultBranch ?? repository.defaultBranch); + const detection = detectWorkspace(existing, ref); const workspaceAnswers = workspaceKickstartAnswers( repository, answers, @@ -97,18 +112,32 @@ export async function kickstartRepo(args: { repo: GithubRepositoryRow; answers: KickstartAnswers; }) { + const ref = args.answers.defaultBranch ?? args.repo.defaultBranch; const client = await createGithubClientForRepo(args.db, args.factory, args.repo); + try { + return await applyKickstart(args, client, ref); + } catch (error) { + throw kickstartFailure(error, args.repo, ref, "base"); + } +} + +async function applyKickstart( + args: Parameters[0], + client: FacilityGithubClient, + ref: string, +) { const existing = await readRepoFiles(client, args.repo.defaultBranch); - const detection = detectWorkspace( - existing, - args.answers.defaultBranch ?? args.repo.defaultBranch, - ); + const detection = detectWorkspace(existing, ref); const rendered = renderWorkspaceKickstart( workspaceKickstartAnswers(args.repo, args.answers, existing, detection.packageManager), existing, ); if (rendered.files.length === 0) { - throw new Error("Repository already contains the Facility 0.12 kickstart files"); + throw new ApiError( + 409, + "kickstart_already_applied", + `${slug(args.repo)} already contains the Facility 0.12 kickstart files. Remove or update them in a normal pull request instead.`, + ); } const branch = "facility/kickstart-0.12"; @@ -123,29 +152,110 @@ export async function kickstartRepo(args: { treeSha, [baseSha], ); + // Everything above resolves the base commit; everything below writes refs. + // The split is what separates the two conditions GitHub reports with the + // same 409: a repository with no commits is observed while resolving the + // base, whereas a ref that stopped being a fast-forward is another writer + // moving the branch after the commit was built. Only the first is fixed by + // pushing, so the segment that failed picks the remedy. try { - await client.createBranch(branch, commitSha); + try { + await client.createBranch(branch, commitSha); + } catch (error) { + if ((error as { status?: number }).status !== 422) throw error; + await client.updateBranch(branch, commitSha); + } + const existingPullRequest = ( + await client.listOpenPullRequestsForHead(branch, args.repo.defaultBranch) + )[0]; + const pr = + existingPullRequest ?? + (await client.createPullRequest({ + title: "feat!: configure Facility 0.12 story workspaces", + head: branch, + body: kickstartPrBody(rendered.files.map((file) => file.path)), + })); + return { + branch, + commitSha, + pr: { number: pr.number, url: pr.url }, + files: rendered.files, + manifest: rendered.manifest, + }; } catch (error) { - if ((error as { status?: number }).status !== 422) throw error; - await client.updateBranch(branch, commitSha); + throw kickstartFailure(error, args.repo, ref, "branch"); + } +} + +function slug(repository: GithubRepositoryRow) { + return `${repository.owner}/${repository.name}`; +} + +/** + * Octokit reports HTTP failures on `status`, while the API error handler reads + * `statusCode`. Every refusal GitHub returned therefore reached the operator as + * "Internal server error", which is the least useful answer possible for the + * first flow a new installation runs. Name the condition and the remedy. + * + * Statuses that are genuinely ours — or that we have no advice for — are passed + * through untouched so they keep being masked and logged as server errors. + */ +function kickstartFailure( + error: unknown, + repository: GithubRepositoryRow, + ref: string, + phase: "base" | "branch", +): unknown { + if (error instanceof ApiError) return error; + const status = (error as { status?: unknown }).status; + if (typeof status !== "number") return error; + + // GitHub answers 403 for throttling as well as for refusal, so the status on + // its own cannot tell an operator whether to fix an installation permission + // or simply wait. `githubRateLimitRetryAt` is the same decision the mirror + // and trigger paths already make, headers and all; reuse it rather than + // reading `x-ratelimit-remaining` a second time here. It also answers for + // every 429, so throttling never reaches the permission branch below. + const retryAt = githubRateLimitRetryAt(error); + if (retryAt) { + return new ApiError( + 429, + "kickstart_github_rate_limited", + `GitHub is rate limiting Facility's installation for ${slug(repository)}. Retry after ${retryAt.toISOString()}.`, + ); + } + + switch (status) { + case 404: + return new ApiError( + 404, + "kickstart_repository_unreachable", + `Facility cannot read ${slug(repository)} at ${ref}. The repository may have no commits yet, the branch may not exist, or the GitHub App installation may no longer include this repository.`, + ); + case 409: + // Empty repository while the base is resolved; a ref that is no longer a + // fast-forward once the kickstart branch is being written. See the split + // in `applyKickstart`. + return phase === "base" + ? new ApiError( + 409, + "kickstart_repository_empty", + `${slug(repository)} has no commits on ${ref}. Push an initial commit before running kickstart.`, + ) + : new ApiError( + 409, + "kickstart_branch_conflict", + `${slug(repository)} changed while kickstart was preparing its branch. Run kickstart again to rebuild it on the current ${ref}.`, + ); + case 403: + return new ApiError( + 403, + "kickstart_repository_forbidden", + `The GitHub App installation for ${slug(repository)} refused the request. Confirm it still grants contents and pull request write access, and that no organization policy blocks it.`, + ); + default: + return error; } - const existingPullRequest = ( - await client.listOpenPullRequestsForHead(branch, args.repo.defaultBranch) - )[0]; - const pr = - existingPullRequest ?? - (await client.createPullRequest({ - title: "feat!: configure Facility 0.12 story workspaces", - head: branch, - body: kickstartPrBody(rendered.files.map((file) => file.path)), - })); - return { - branch, - commitSha, - pr: { number: pr.number, url: pr.url }, - files: rendered.files, - manifest: rendered.manifest, - }; } function workspaceKickstartAnswers( diff --git a/services/api/test/kickstart-failures.test.ts b/services/api/test/kickstart-failures.test.ts new file mode 100644 index 00000000..7962a82b --- /dev/null +++ b/services/api/test/kickstart-failures.test.ts @@ -0,0 +1,262 @@ +import type { FacilityDb } from "@facility/db"; +import { describe, expect, it } from "vitest"; +import { ApiError } from "../src/errors.js"; +import type { Octokit } from "../src/github/client.js"; +import { + type GithubRepositoryRow, + kickstartPreview, + kickstartRepo, +} from "../src/github/kickstart.js"; +import type { AppConfig, Principal } from "../src/types.js"; + +const repository: GithubRepositoryRow = { + id: "repo_test", + orgId: "org_test", + projectId: "proj_test", + installationId: "ghi_test", + owner: "panteraimperial-hub", + name: "facility-test", + defaultBranch: "main", +}; + +const installation = { + id: "ghi_test", + orgId: "org_test", + installationId: 4242, + suspendedAt: null, +}; + +function fakeDb(row: unknown = installation) { + return { + select: () => ({ + from: () => ({ where: () => ({ limit: async () => (row ? [row] : []) }) }), + }), + } as unknown as FacilityDb; +} + +/** An Octokit failure carries `status`, the way `@octokit/request-error` reports it. */ +function githubError(status: number, message: string) { + return Object.assign(new Error(message), { status }); +} + +/** + * GitHub answers 403 both when an installation may not do something and when it + * has spent its hourly budget. The two are told apart by the rate-limit headers, + * which is the shape `githubRateLimitRetryAt` reads. + */ +function throttledError(status: number, resetAt: number) { + return Object.assign(new Error("API rate limit exceeded"), { + status, + response: { + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(Math.floor(resetAt / 1_000)), + }, + }, + }); +} + +/** + * `getContent` is the only call kickstart makes before it needs a base commit, + * and `readRepoFiles` swallows its failures, so a repository is only observed to + * be unusable when the base ref is resolved. + */ +function fakeOctokit(options: { + getBranch?: () => Promise<{ data: { commit: { sha: string } } }>; + createRef?: () => Promise<{ data: unknown }>; + updateRef?: () => Promise<{ data: unknown }>; + contents?: Map; +}): Octokit { + const contents = options.contents ?? new Map(); + return { + rest: { + git: { + getCommit: async () => ({ data: { sha: "a".repeat(40), tree: { sha: "b".repeat(40) } } }), + createBlob: async () => ({ data: { sha: "c".repeat(40) } }), + createTree: async () => ({ data: { sha: "d".repeat(40) } }), + createCommit: async () => ({ data: { sha: "e".repeat(40) } }), + createRef: options.createRef ?? (async () => ({ data: {} })), + updateRef: options.updateRef ?? (async () => ({ data: {} })), + }, + repos: { + getContent: async (args: Record) => { + const path = String(args.path); + if (!contents.has(path)) throw githubError(404, "Not Found"); + return { data: contents.get(path) }; + }, + getBranch: + options.getBranch ?? (async () => ({ data: { commit: { sha: "a".repeat(40) } } })), + }, + pulls: { + create: async () => ({ data: { number: 1, html_url: "https://github.test/pr/1" } }), + list: async () => ({ data: [] }), + }, + }, + } as unknown as Octokit; +} + +/** Every file the 0.12 kickstart would create, so nothing is left to render. */ +function alreadyKickstarted() { + const file = (path: string) => ({ + type: "file", + path, + encoding: "base64", + content: Buffer.from(`# ${path}\n`).toString("base64"), + }); + const agents = [ + "architect", + "builder", + "pr-reviewer", + "address-review", + "ci-doctor", + "security-audit", + ].map((name) => `.agents/${name}.md`); + const contents = new Map([ + [".facility.yml", file(".facility.yml")], + [".agents", agents.map((path) => ({ path }))], + ...agents.map((path) => [path, file(path)] as const), + ]); + return contents; +} + +function applyArgs(db: FacilityDb, octokit: Octokit) { + return { + db, + factory: async () => octokit, + config: {} as AppConfig, + principal: { type: "user", id: "user_test", orgId: "org_test" } as Principal, + projectId: "proj_test", + repo: repository, + answers: {}, + }; +} + +describe("kickstart failures name the condition", () => { + it("reports a repository whose base ref cannot be read, instead of a 500", async () => { + const octokit = fakeOctokit({ + getBranch: async () => { + throw githubError(404, "Branch not found"); + }, + }); + + await expect(kickstartRepo(applyArgs(fakeDb(), octokit))).rejects.toMatchObject({ + statusCode: 404, + code: "kickstart_repository_unreachable", + message: expect.stringContaining("panteraimperial-hub/facility-test"), + }); + }); + + it("names an empty repository when GitHub reports the conflict", async () => { + const octokit = fakeOctokit({ + getBranch: async () => { + throw githubError(409, "Git Repository is empty."); + }, + }); + + await expect(kickstartRepo(applyArgs(fakeDb(), octokit))).rejects.toMatchObject({ + statusCode: 409, + code: "kickstart_repository_empty", + message: expect.stringContaining("main"), + }); + }); + + it("separates a refused installation from a rate-limited one", async () => { + const forbidden = fakeOctokit({ + getBranch: async () => { + throw githubError(403, "Resource not accessible by integration"); + }, + }); + await expect(kickstartRepo(applyArgs(fakeDb(), forbidden))).rejects.toMatchObject({ + statusCode: 403, + code: "kickstart_repository_forbidden", + }); + + const throttled = fakeOctokit({ + getBranch: async () => { + throw githubError(429, "Too Many Requests"); + }, + }); + await expect(kickstartRepo(applyArgs(fakeDb(), throttled))).rejects.toMatchObject({ + statusCode: 429, + code: "kickstart_github_rate_limited", + }); + }); + + it("reads a 403 as throttling when the rate-limit headers say so, not as a permission fault", async () => { + // Telling an operator to check installation permissions while GitHub is + // only asking them to wait sends them to the wrong place entirely, and the + // repository's own helper already knows the difference. + // `x-ratelimit-reset` is whole seconds, so floor the fixture to one rather + // than asserting against a millisecond the header cannot carry. + const resetAt = Math.floor((Date.now() + 15 * 60 * 1_000) / 1_000) * 1_000; + const octokit = fakeOctokit({ + getBranch: async () => { + throw throttledError(403, resetAt); + }, + }); + + const failure = await kickstartRepo(applyArgs(fakeDb(), octokit)).catch((error) => error); + expect(failure).toMatchObject({ statusCode: 429, code: "kickstart_github_rate_limited" }); + // The reset GitHub supplied is carried through, so the advice is a time. + expect(failure.message).toContain(new Date(resetAt + 1_000).toISOString()); + }); + + it("calls a ref conflict a moving branch, not a repository without commits", async () => { + // GitHub answers 409 for an empty repository and for a ref that stopped + // being a fast-forward. Only the first is fixed by pushing a commit, so the + // two must not share a message. This one fails after the base resolved. + const octokit = fakeOctokit({ + createRef: async () => { + throw githubError(422, "Reference already exists"); + }, + updateRef: async () => { + throw githubError(409, "Update is not a fast forward"); + }, + }); + + await expect(kickstartRepo(applyArgs(fakeDb(), octokit))).rejects.toMatchObject({ + statusCode: 409, + code: "kickstart_branch_conflict", + message: expect.stringContaining("main"), + }); + }); + + it("treats an already-kickstarted repository as a conflict, not a server fault", async () => { + const octokit = fakeOctokit({ contents: alreadyKickstarted() }); + + await expect(kickstartRepo(applyArgs(fakeDb(), octokit))).rejects.toMatchObject({ + statusCode: 409, + code: "kickstart_already_applied", + }); + }); + + it("reports a missing or suspended installation before calling GitHub", async () => { + const octokit = fakeOctokit({}); + + await expect(kickstartRepo(applyArgs(fakeDb(null), octokit))).rejects.toMatchObject({ + statusCode: 409, + code: "github_installation_unavailable", + }); + + await expect( + kickstartPreview(fakeDb(), async () => octokit, { ...repository, installationId: null }, {}), + ).rejects.toMatchObject({ + statusCode: 409, + code: "github_installation_missing", + }); + }); + + it("leaves a failure it has no advice for masked as a server error", async () => { + const octokit = fakeOctokit({ + getBranch: async () => { + throw githubError(502, "Bad gateway"); + }, + }); + + const failure = await kickstartRepo(applyArgs(fakeDb(), octokit)).catch((error) => error); + + // Not an ApiError, so the handler keeps logging it and answering a masked 500. + expect(failure).not.toBeInstanceOf(ApiError); + expect(failure).toMatchObject({ status: 502 }); + }); +});