From e58a53f6935466a4659f16fd4a73b50e03077404 Mon Sep 17 00:00:00 2001 From: Pedro Lobato <69770518+Lob26@users.noreply.github.com> Date: Mon, 7 Sep 2026 10:46:20 -0500 Subject: [PATCH 1/2] fix(github): name the kickstart failure instead of returning a bare 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kickstart answered "Internal server error" for every refusal GitHub could give it, which is the least useful answer possible for the first flow a new installation runs: the operator cannot tell whether the repository, the installation, or Facility is at fault. Two causes, one symptom. Octokit reports HTTP failures on `status`, while the API error handler reads `statusCode`, so a 404, 403 or 409 from GitHub fell through to the generic 500 branch. And kickstart's own refusals were bare `Error`s, including "already contains the Facility 0.12 kickstart files" — a conflict the caller can act on, delivered as a server fault. Map both. An unreadable base ref, an empty repository, a refused or throttled installation, a missing or suspended connection, and an already-kickstarted repository now answer 4xx with a stable code and a message naming the repository, the ref and the remedy. Statuses with no advice attached stay unmapped, so genuinely unknown failures are still logged and masked. --- services/api/src/github/kickstart.ts | 93 ++++++++- services/api/test/kickstart-failures.test.ts | 204 +++++++++++++++++++ 2 files changed, 288 insertions(+), 9 deletions(-) create mode 100644 services/api/test/kickstart-failures.test.ts diff --git a/services/api/src/github/kickstart.ts b/services/api/src/github/kickstart.ts index 50721f57..f0053242 100644 --- a/services/api/src/github/kickstart.ts +++ b/services/api/src/github/kickstart.ts @@ -1,6 +1,7 @@ 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 { readRepoFiles } from "./repo-files.js"; @@ -35,7 +36,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 +51,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 +70,12 @@ export async function kickstartPreview( repository: GithubRepositoryRow, answers: KickstartAnswers, ) { + const ref = answers.defaultBranch ?? repository.defaultBranch; const client = await createGithubClientForRepo(db, factory, repository); - const existing = await readRepoFiles(client, repository.defaultBranch); - const detection = detectWorkspace(existing, answers.defaultBranch ?? repository.defaultBranch); + const existing = await readRepoFiles(client, repository.defaultBranch).catch((error) => { + throw kickstartFailure(error, repository, ref); + }); + const detection = detectWorkspace(existing, ref); const workspaceAnswers = workspaceKickstartAnswers( repository, answers, @@ -97,18 +111,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); + } +} + +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"; @@ -148,6 +176,53 @@ export async function kickstartRepo(args: { }; } +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): unknown { + if (error instanceof ApiError) return error; + const status = (error as { status?: unknown }).status; + if (typeof status !== "number") return error; + 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: + return new ApiError( + 409, + "kickstart_repository_empty", + `${slug(repository)} has no commits on ${ref}. Push an initial commit before running kickstart.`, + ); + 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.`, + ); + case 429: + return new ApiError( + 429, + "kickstart_github_rate_limited", + `GitHub is rate limiting Facility's installation for ${slug(repository)}. Retry once the installation's rate limit resets.`, + ); + default: + return error; + } +} + function workspaceKickstartAnswers( repository: GithubRepositoryRow, answers: KickstartAnswers, diff --git a/services/api/test/kickstart-failures.test.ts b/services/api/test/kickstart-failures.test.ts new file mode 100644 index 00000000..56c068fc --- /dev/null +++ b/services/api/test/kickstart-failures.test.ts @@ -0,0 +1,204 @@ +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 }); +} + +/** + * `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 } } }>; + 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: async () => ({ data: {} }), + 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("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 }); + }); +}); From eee45b143e4d5550e76c5aaf9b3524515c51dfc7 Mon Sep 17 00:00:00 2001 From: Pedro Lobato <69770518+Lob26@users.noreply.github.com> Date: Tue, 15 Sep 2026 08:34:38 -0500 Subject: [PATCH 2/2] fix(github): tell throttling apart from refusal, and a moving ref from an empty repo Two statuses were carrying two conditions each, so the message named the wrong remedy half the time. GitHub answers 403 both when an installation may not do something and when it has spent its budget. Calling every 403 a permission fault sends an operator to the App settings while GitHub is only asking them to wait. `githubRateLimitRetryAt` already makes that decision from the headers for the mirror and trigger paths, so kickstart now asks it first and reports the reset instant instead of advice about permissions. It answers for every 429 as well, so that branch is gone. GitHub also answers 409 for a repository with no commits and for a ref that stopped being a fast-forward. Only the first is fixed by pushing. `applyKickstart` now separates resolving the base commit from writing the refs, and the segment that failed picks the message: a 409 while resolving the base is an empty repository, a 409 once the branch is being written is another writer having moved it. The failure mapper is no longer wrapped around `readRepoFiles`, which swallows every per-path failure and cannot surface one. It is wrapped around installation-token minting instead, which can. Three regressions, all red without the change: a 403 carrying `x-ratelimit-remaining: 0` reports the reset rather than a permission fault, a plain 403 still reports the permission fault, and a non fast-forward ref update reports a moving branch rather than a repository without commits. --- services/api/src/github/kickstart.ts | 107 ++++++++++++------- services/api/test/kickstart-failures.test.ts | 62 ++++++++++- 2 files changed, 131 insertions(+), 38 deletions(-) diff --git a/services/api/src/github/kickstart.ts b/services/api/src/github/kickstart.ts index f0053242..137f564a 100644 --- a/services/api/src/github/kickstart.ts +++ b/services/api/src/github/kickstart.ts @@ -4,6 +4,7 @@ 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 = { @@ -71,10 +72,10 @@ export async function kickstartPreview( answers: KickstartAnswers, ) { const ref = answers.defaultBranch ?? repository.defaultBranch; - const client = await createGithubClientForRepo(db, factory, repository); - const existing = await readRepoFiles(client, repository.defaultBranch).catch((error) => { - throw kickstartFailure(error, repository, ref); + 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, ref); const workspaceAnswers = workspaceKickstartAnswers( repository, @@ -116,7 +117,7 @@ export async function kickstartRepo(args: { try { return await applyKickstart(args, client, ref); } catch (error) { - throw kickstartFailure(error, args.repo, ref); + throw kickstartFailure(error, args.repo, ref, "base"); } } @@ -151,29 +152,39 @@ async function applyKickstart( 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"); } - 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 slug(repository: GithubRepositoryRow) { @@ -189,10 +200,31 @@ function slug(repository: GithubRepositoryRow) { * 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): unknown { +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( @@ -201,23 +233,26 @@ function kickstartFailure(error: unknown, repository: GithubRepositoryRow, ref: `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: - return new ApiError( - 409, - "kickstart_repository_empty", - `${slug(repository)} has no commits on ${ref}. Push an initial commit before running kickstart.`, - ); + // 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.`, ); - case 429: - return new ApiError( - 429, - "kickstart_github_rate_limited", - `GitHub is rate limiting Facility's installation for ${slug(repository)}. Retry once the installation's rate limit resets.`, - ); default: return error; } diff --git a/services/api/test/kickstart-failures.test.ts b/services/api/test/kickstart-failures.test.ts index 56c068fc..7962a82b 100644 --- a/services/api/test/kickstart-failures.test.ts +++ b/services/api/test/kickstart-failures.test.ts @@ -39,6 +39,23 @@ 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 @@ -46,6 +63,8 @@ function githubError(status: number, message: string) { */ 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(); @@ -56,8 +75,8 @@ function fakeOctokit(options: { createBlob: async () => ({ data: { sha: "c".repeat(40) } }), createTree: async () => ({ data: { sha: "d".repeat(40) } }), createCommit: async () => ({ data: { sha: "e".repeat(40) } }), - createRef: async () => ({ data: {} }), - updateRef: async () => ({ data: {} }), + createRef: options.createRef ?? (async () => ({ data: {} })), + updateRef: options.updateRef ?? (async () => ({ data: {} })), }, repos: { getContent: async (args: Record) => { @@ -163,6 +182,45 @@ describe("kickstart failures name the condition", () => { }); }); + 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() });