From 9b073e76aeb9c96ea5ab3f0fb4804a6bc05405b7 Mon Sep 17 00:00:00 2001 From: Dylan Boudro <121908331+starmorph@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:23:03 -0700 Subject: [PATCH] Make required readiness checks fail when unverified Validate Render task responses, fail incomplete registration and installation grants, and distinguish webhook configuration from proven delivery. Require delivery metadata for the configured target without claiming current activation. Fixes #75 --- docs/configuration.md | 12 ++++- docs/runbooks/setup.md | 11 +++-- scripts/lib/doctor.ts | 57 +++++++++++++++--------- scripts/lib/github.ts | 30 +++++++++---- scripts/lib/setup-github.ts | 21 +++------ tests/scripts-doctor.test.ts | 85 +++++++++++++++++++++++++++++++++++- tests/scripts-setup.test.ts | 19 +++++++- 7 files changed, 184 insertions(+), 51 deletions(-) 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 = () =>