diff --git a/docs/configuration.md b/docs/configuration.md index 5e41d51..c4140b0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -225,7 +225,17 @@ the factory adds it back so a human reply can start the continuation. Use ## Check a deployment `doctor` is read-only, does not print secret values, and exits nonzero when a -required check fails. +required check fails or cannot be verified. This includes unreachable task APIs, +missing task registration, and installation permissions or events that have not +been accepted. The gateway intentionally skips GitHub checks because it has no +App credentials. + +Webhook delivery is verified only when the latest attempt went to the configured +gateway and received a 2xx response. If no such evidence is available, confirm +Webhook > Active in the App's settings, save, and redeliver a ping from Advanced +before rerunning doctor. GitHub's API does not report the Active setting, so a +successful delivery proves delivery only at the reported time; it cannot guarantee +that the hook has not since been disabled. From a local checkout, this checks the services and credentials represented by local `.env`: diff --git a/docs/runbooks/setup.md b/docs/runbooks/setup.md index dfcd9fa..7f806f0 100644 --- a/docs/runbooks/setup.md +++ b/docs/runbooks/setup.md @@ -308,9 +308,14 @@ Run: npm run doctor -- --service workflows ``` -The warning `no webhook deliveries yet, so the hook may be inactive` is -expected before the smoke test. Do not continue while either command has a -failed check. +A required check that cannot be verified exits nonzero. Before continuing, +resolve failed checks on both services. Workflows doctor checks that the latest +webhook delivery reached the configured gateway successfully. If it reports +delivery as unverified, open the App's settings, confirm Webhook > Active, save, +and redeliver a ping from Advanced. Then rerun doctor. A historical attempt to a +placeholder URL does not verify this setup. A successful delivery is historical +evidence; the smoke test below verifies the current end-to-end path. +The gateway's skipped GitHub checks are expected: it holds no App credentials. ## Phase 11: Smoke-test Triage diff --git a/scripts/lib/doctor.ts b/scripts/lib/doctor.ts index 3c52ccb..62a1135 100644 --- a/scripts/lib/doctor.ts +++ b/scripts/lib/doctor.ts @@ -1,4 +1,5 @@ /** doctor: read-only checks of one or both services' environment, the GitHub App, and Workflows tasks. */ +import { z } from "zod"; import type { Agent } from "../../app/agents/define.js"; import { AGENTS } from "../../app/agents/registry.js"; import { TASKS } from "../../app/config/index.js"; @@ -109,6 +110,7 @@ async function checkGitHub(opts: DoctorOptions, out: Reporter): Promise { async function checkWebhook(app: GitHubApp, info: Awaited>, env: Env, out: Reporter) { const settings = appSettingsUrl(info, app.apiUrl); + let configuredUrl: string | undefined; try { const hook = await app.getHookConfig(); const url = hook.url ? safeUrl(hook.url) : null; @@ -117,36 +119,45 @@ async function checkWebhook(app: GitHubApp, info: Awaited Active and save; redeliver a ping in Advanced, then rerun doctor`; try { - const [latest] = await app.recentDeliveries(10); + const [latest] = await app.recentDeliveries(1); if (!latest) { - out.warn("no webhook deliveries yet, so the hook may be inactive", `Check Webhook > Active in ${settings}`); - } else if (latest.status_code >= 200 && latest.status_code < 300) { - out.pass(`webhook is active: latest delivery (${latest.event}, ${latest.delivered_at}) got ${latest.status_code}`); - } else if (latest.status_code === 401) { + out.fail("webhook activation and delivery are unverified: no deliveries yet", fix); + return; + } + const delivery = await app.hookDelivery(latest.id); + if (!configuredUrl || delivery.url !== configuredUrl) { + out.fail("latest webhook delivery was not sent to the configured gateway", fix); + return; + } + if (delivery.status_code >= 200 && delivery.status_code < 300) { + out.pass(`latest delivery to the configured gateway (${delivery.event}, ${delivery.delivered_at}) got ${delivery.status_code}`); + } else if (delivery.status_code === 401) { out.fail( "the gateway rejected the latest delivery's signature (401)", - "The gateway's GITHUB_WEBHOOK_SECRET differs from the App's: rerun setup:github with the gateway's value", + "The gateway's GITHUB_WEBHOOK_SECRET differs from the App's: rerun setup:github with the gateway's value, then redeliver a ping", ); } else { - out.warn(`latest webhook delivery (${latest.event}, ${latest.delivered_at}) got ${latest.status_code || "no response"}`); + out.fail(`latest webhook delivery got ${delivery.status_code || "no response"}`, fix); } } catch (error) { - out.warn(`could not list webhook deliveries: ${errorMessage(error)}`); + out.fail(`webhook delivery is unverified: ${errorMessage(error)}`, fix); } } @@ -162,10 +173,10 @@ async function checkRepo(app: GitHubApp, info: { slug: string; html_url: string out.pass(`installed (installation ${installation.id})`); const perms = missingPermissions(installation.permissions); if (perms.length) { - out.warn(`installation hasn't granted: ${perms.join(", ")}`, "An owner must accept the App's updated permissions in the installation settings"); + out.fail(`installation hasn't granted: ${perms.join(", ")}`, "An owner must accept the App's updated permissions in the installation settings"); } const events = missingEvents(installation.events); - if (events.length) out.warn(`installation isn't receiving: ${events.join(", ")}`, "Accept the App's updated events"); + if (events.length) out.fail(`installation isn't receiving: ${events.join(", ")}`, "Accept the App's updated events"); const token = await app.installationToken(installation.id, name, { issues: "read", metadata: "read" }); out.addSecrets(token); @@ -183,15 +194,17 @@ async function checkRepo(app: GitHubApp, info: { slug: string; html_url: string } } +const taskRows = z.array(z.object({ task: z.object({ name: z.string().min(1).max(255) }) })).max(100); + /** * The Render API's GET /v1/tasks, filtered by slug, for each task the gateway starts. The SDK's workflows client - * has no list-tasks method, so this is a plain request. A non-auth API error skips rather than fails. + * has no list-tasks method, so this is a plain request. Unverified registration must not pass readiness. */ async function checkTasks(opts: DoctorOptions, out: Reporter): Promise { out.heading("Workflows tasks"); const apiKey = opts.env.RENDER_API_KEY?.trim(); if (!apiKey) { - out.skip("task registration: RENDER_API_KEY is not set"); + out.fail("task registration is unverified: RENDER_API_KEY is not set", "Set RENDER_API_KEY and rerun doctor"); return; } const slug = opts.env.RENDER_WORKFLOW_SLUG?.trim() || "factory-workflows"; @@ -206,7 +219,7 @@ async function checkTasks(opts: DoctorOptions, out: Reporter): Promise { signal: AbortSignal.timeout(15_000), }); } catch (error) { - out.skip(`task registration: Render API unreachable (${errorMessage(error)})`); + out.fail(`task registration is unverified: Render API unreachable (${errorMessage(error)})`, "Check connectivity and rerun doctor"); return; } if (response.status === 401 || response.status === 403) { @@ -214,14 +227,18 @@ async function checkTasks(opts: DoctorOptions, out: Reporter): Promise { return; } if (!response.ok) { - out.skip(`task registration: the Render API returned ${response.status} for GET /v1/tasks`); + out.fail(`task registration is unverified: the Render API returned ${response.status} for GET /v1/tasks`, "Retry doctor after the API recovers"); + return; + } + const result = taskRows.safeParse(await response.json().catch(() => undefined)); + if (!result.success) { + out.fail("task registration is unverified: Render returned an invalid task list", "Retry doctor; if this persists, check the Render API response schema"); return; } - const rows = (await response.json().catch(() => [])) as { task?: { name?: string } }[]; - const found = new Set(rows.map((row) => row.task?.name).filter((name): name is string => !!name)); + const found = new Set(result.data.map((row) => row.task.name)); const missing = expected.filter((task) => !found.has(task)); if (found.size === 0) { - out.warn(`no tasks found for workflow ${slug}`, "Deploy factory-workflows; on the gateway, RENDER_WORKFLOW_SLUG must be its slug"); + out.fail(`no tasks found for workflow ${slug}`, "Deploy factory-workflows; on the gateway, RENDER_WORKFLOW_SLUG must be its slug"); } else if (missing.length) { out.fail(`tasks not registered on ${slug}: ${missing.join(", ")}`, "Redeploy factory-workflows"); } else { diff --git a/scripts/lib/github.ts b/scripts/lib/github.ts index ea95aba..7d5beac 100644 --- a/scripts/lib/github.ts +++ b/scripts/lib/github.ts @@ -1,5 +1,6 @@ /** GitHub App access for the setup scripts and doctor, from injected env and fetch rather than process globals. */ import { createPrivateKey } from "node:crypto"; +import { z } from "zod"; import { appJwt, githubApiUrl, normalizePrivateKey } from "../../app/github/auth.js"; import { GitHubHttp } from "../../app/github/request.js"; import { GitHubError } from "../../app/github/types.js"; @@ -30,12 +31,14 @@ export interface HookConfig { secret?: string; } -export interface HookDelivery { - id: number; - delivered_at: string; - status_code: number; - event: string; -} +const hookDelivery = z.object({ + id: z.number().int().positive(), + delivered_at: z.iso.datetime(), + status_code: z.number().int().min(0).max(599), + event: z.string().min(1).max(100), +}); + +export type HookDelivery = z.infer; export interface Installation { id: number; @@ -97,8 +100,19 @@ export class GitHubApp { } /** The most recent deliveries, newest first. */ - recentDeliveries(perPage = 10): Promise { - return this.http.json("GET", "/app/hook/deliveries", { query: { per_page: perPage } }); + async recentDeliveries(perPage = 10): Promise { + const response = await this.http.json("GET", "/app/hook/deliveries", { query: { per_page: perPage } }); + const result = z.array(hookDelivery).max(100).safeParse(response); + if (!result.success) throw new Error("GitHub returned an invalid webhook delivery list"); + return result.data; + } + + /** Only retain delivery metadata: request/response payloads can contain private repository content. */ + async hookDelivery(id: number): Promise { + const response = await this.http.json("GET", `/app/hook/deliveries/${id}`); + const result = hookDelivery.extend({ url: z.url() }).safeParse(response); + if (!result.success) throw new Error("GitHub returned invalid webhook delivery metadata"); + return result.data; } /** The App's installation on a repository, or null when it isn't installed there. */ diff --git a/scripts/lib/setup-github.ts b/scripts/lib/setup-github.ts index 3c24871..2f93ed3 100644 --- a/scripts/lib/setup-github.ts +++ b/scripts/lib/setup-github.ts @@ -57,22 +57,11 @@ async function configureWebhook(app: GitHubApp, info: AppInfo, url: string, secr else out.pass(`webhook content type set to json${current.content_type ? ` (was ${current.content_type})` : ""}`); out.pass("webhook secret set (the gateway's GITHUB_WEBHOOK_SECRET must be the same value)"); - // The REST API can't activate an App webhook (PATCH /app/hook/config has no `active`). A delivery proves it's on. - const settings = appSettingsUrl(info, app.apiUrl); - let deliveries: Awaited> = []; - try { - deliveries = await app.recentDeliveries(1); - } catch (error) { - out.warn(`could not list webhook deliveries: ${errorMessage(error)}`); - } - if (deliveries.length > 0) { - out.pass(`webhook is delivering (latest ${deliveries[0]?.event} at ${deliveries[0]?.delivered_at})`); - } else { - out.warn( - "webhook activation can't be set through the API: turn it on by hand", - `Open ${settings}, check Webhook > Active, and click Save changes. Leave the URL and secret as they are.`, - ); - } + // The API cannot read or set Active. Past attempts cannot verify the URL and secret just written. + out.warn( + "webhook configured; activation and delivery are unverified", + `Open ${appSettingsUrl(info, app.apiUrl)}, check Webhook > Active and save. Redeliver a ping in Advanced, then run doctor. Leave the URL and secret as they are.`, + ); } async function ensureRepoLabels(app: GitHubApp, info: AppInfo, fullName: string, out: Reporter): Promise { diff --git a/tests/scripts-doctor.test.ts b/tests/scripts-doctor.test.ts index 1766e30..873b27d 100644 --- a/tests/scripts-doctor.test.ts +++ b/tests/scripts-doctor.test.ts @@ -35,6 +35,7 @@ const APP = { events: ["issues", "issue_comment", "pull_request", "push"], }; const LABEL_NAMES = ["factory:triage", "factory:spec-approved", "factory:pr-review", "factory:autopilot", "factory:working"]; +const DELIVERY = { id: 1, delivered_at: "2026-10-28T11:59:00Z", status_code: 202, event: "issues" }; const TASK_NAMES = ["triage", "spec-to-pr", "pr-review", "autopilot", "cancel", "refresh-snapshot"]; interface Call { @@ -48,10 +49,17 @@ interface World { hook?: Record; deliveries?: { id: number; delivered_at: string; status_code: number; event: string }[]; installed?: boolean; + installation?: { permissions: Record; events: string[] }; + delivery?: unknown; + deliveryStatus?: number; + deliveriesStatus?: number; labels?: string[]; labelsResponse?: { status: number; body: unknown }; tasks?: string[]; tasksStatus?: number; + tasksBody?: unknown; + tasksJson?: string; + tasksUnreachable?: boolean; } function fakeFetch(world: World = {}) { @@ -75,16 +83,21 @@ function fakeFetch(world: World = {}) { }, ); case "GET api.github.com/app/hook/deliveries": - return json(world.deliveries ?? [{ id: 1, delivered_at: "2026-10-18T00:00:00Z", status_code: 202, event: "issues" }]); + return json(world.deliveries ?? [DELIVERY], world.deliveriesStatus ?? 200); + case "GET api.github.com/app/hook/deliveries/1": + return json(world.delivery ?? { ...DELIVERY, url: "https://gw.onrender.com/webhooks/github" }, world.deliveryStatus ?? 200); case "GET api.github.com/repos/acme/web/installation": - return world.installed === false ? json({ message: "Not Found" }, 404) : json({ id: 7, permissions: APP.permissions, events: APP.events }); + return world.installed === false ? json({ message: "Not Found" }, 404) : json({ id: 7, ...(world.installation ?? { permissions: APP.permissions, events: APP.events }) }); case "POST api.github.com/app/installations/7/access_tokens": return json({ token: TOKEN, expires_at: new Date(Date.now() + 3_600_000).toISOString() }, 201); case "GET api.github.com/repos/acme/web/labels": if (world.labelsResponse) return json(world.labelsResponse.body, world.labelsResponse.status); return json((world.labels ?? LABEL_NAMES).map((name) => ({ name, color: "000000", description: null }))); case "GET api.render.com/v1/tasks": + if (world.tasksUnreachable) throw new Error(`connection failed ${SECRETS.RENDER_API_KEY}`); if (world.tasksStatus) return json({ message: "nope" }, world.tasksStatus); + if (world.tasksJson !== undefined) return new Response(world.tasksJson); + if (Object.hasOwn(world, "tasksBody")) return json(world.tasksBody); return json((world.tasks ?? TASK_NAMES).map((name, i) => ({ task: { id: `tsk-${i}`, name }, cursor: `c${i}` }))); default: return json({ message: "Not Found" }, 404); @@ -138,6 +151,74 @@ describe("doctor", () => { expect(find("fail", "issue_comment, push")).toBeDefined(); }); + it("keeps the gateway's intentional lack of App credentials as a skip", async () => { + const { code, find } = await doctor({ + services: ["gateway"], + env: { RENDER_API_KEY: SECRETS.RENDER_API_KEY, GITHUB_WEBHOOK_SECRET: SECRETS.GITHUB_WEBHOOK_SECRET, RENDER_WORKFLOW_SLUG: "factory-workflows" }, + }); + expect(code).toBe(0); + expect(find("skip", "no App credentials")).toBeDefined(); + }); + + it.each([ + ["null", { tasksBody: null }], + ["object", { tasksBody: {} }], + ["invalid JSON", { tasksJson: "not json" }], + ["missing task", { tasksBody: [{}] }], + ["missing name", { tasksBody: [{ task: {} }] }], + ["non-string name", { tasksBody: [{ task: { name: 1 } }] }], + ["null row", { tasksBody: [null] }], + ] satisfies [string, World][])("fails without crashing on a %s task response", async (_, world) => { + const { code, find, output } = await doctor({ world }); + expect(code).toBe(1); + expect(find("fail", "invalid task list")).toBeDefined(); + expectNoSecrets(output); + }); + + it.each([ + ["no tasks", { tasks: [] }, "no tasks found"], + ["partial registration", { tasks: ["triage"] }, "tasks not registered"], + ["unreachable API", { tasksUnreachable: true }, "Render API unreachable"], + ["server error", { tasksStatus: 503 }, "returned 503"], + ["rejected key", { tasksStatus: 401 }, "rejected"], + ["forbidden key", { tasksStatus: 403 }, "rejected"], + ] satisfies [string, World, string][])("blocks readiness for %s", async (_, world, message) => { + const { code, find, output } = await doctor({ world }); + expect(code).toBe(1); + expect(find("fail", message)).toBeDefined(); + expectNoSecrets(output); + }); + + it("fails when installed permissions or events have not been accepted", async () => { + const { code, find } = await doctor({ world: { installation: { permissions: { metadata: "read" }, events: ["push"] } } }); + expect(code).toBe(1); + expect(find("fail", "installation hasn't granted")).toBeDefined(); + expect(find("fail", "installation isn't receiving")).toBeDefined(); + }); + + it.each([ + ["no history", { deliveries: [] }, "no deliveries yet"], + ["different target", { delivery: { ...DELIVERY, url: "https://placeholder.example.com/webhooks/github" } }, "not sent to the configured gateway"], + ["failed attempt", { delivery: { ...DELIVERY, url: "https://gw.onrender.com/webhooks/github", status_code: 502 } }, "got 502"], + ["signature rejection", { delivery: { ...DELIVERY, url: "https://gw.onrender.com/webhooks/github", status_code: 401 } }, "signature"], + ["unavailable history", { deliveriesStatus: 503 }, "unverified"], + ["invalid metadata", { delivery: { secret: SECRETS.GITHUB_WEBHOOK_SECRET } }, "invalid webhook delivery metadata"], + ] satisfies [string, World, string][])("does not mistake %s for verified webhook delivery", async (_, world, message) => { + const { code, find, output } = await doctor({ world }); + expect(code).toBe(1); + expect(find("fail", message)).toBeDefined(); + expectNoSecrets(output); + }); + + it("reports a successful delivery as historical evidence without claiming the hook is currently active", async () => { + const { code, output } = await doctor({ + world: { delivery: { ...DELIVERY, url: "https://gw.onrender.com/webhooks/github", delivered_at: "2026-10-26T00:00:00Z" } }, + }); + expect(code).toBe(0); + expect(output).toContain("latest delivery to the configured gateway (issues, 2026-10-26T00:00:00Z) got 202"); + expect(output).not.toContain("webhook is active"); + }); + it("never prints a secret, even when everything fails", async () => { const { output } = await doctor({ world: { diff --git a/tests/scripts-setup.test.ts b/tests/scripts-setup.test.ts index 8569c19..8bd3c1e 100644 --- a/tests/scripts-setup.test.ts +++ b/tests/scripts-setup.test.ts @@ -123,12 +123,29 @@ describe("setup:github", () => { expect(created.every((c) => c.auth === `Bearer ${TOKEN}`)).toBe(true); const output = lines.join("\n"); - expect(output).toContain("webhook activation can't be set through the API"); + expect(output).toContain("webhook configured; activation and delivery are unverified"); expect(output).toContain("https://github.com/organizations/acme/settings/apps/sf"); expect(output).not.toContain(SECRET); expect(output).not.toContain(TOKEN); }); + it.each([200, 502])("does not present a historical %s attempt as proof after updating the webhook", async (status_code) => { + const { fetch } = githubWorld({ deliveries: [{ id: 1, event: "ping", status_code, delivered_at: "2020-01-01T00:00:00Z" }] }); + const lines: string[] = []; + const code = await setupGithub( + new GitHubApp({ env: env(), fetch }), + { gatewayUrl: "https://gw.onrender.com", webhookSecret: SECRET, repos: ["acme/web"] }, + new Reporter((line) => lines.push(line)), + ); + const output = lines.join("\n"); + expect(code).toBe(0); + expect(output).toContain("activation and delivery are unverified"); + expect(output).toContain("Webhook > Active"); + expect(output).toContain("Redeliver a ping"); + expect(output).not.toContain("webhook is delivering"); + expect(output).not.toContain(SECRET); + }); + it("is idempotent: a second run creates no labels", async () => { const world = githubWorld(); const run = () =>