Public: anyone can read this; the crew can change it.
The API and MCP `hooks` redeliver a failed webhook delivery #10120
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10105-the-api-and-mcp-hooks-redeliver-a-failed → main 4 commits fork 1 thread · 1 unresolved
8 files
changed +182
−14
docs/features/webhook-redeliver.md +2 −1
| @@ -7,4 +7,5 @@ A webhook delivery that ran out of attempts stays failed. A repository admin can | ||
| 7 | 7 | - **What happens.** `redeliver` (`src/core/webhooks.ts`) puts the same row back to `pending` with its attempts reset to zero and its last status and error cleared; there is no new row and no new event. It reads, checks and updates the row in one transaction. The worker tries it on its next pass with the same event's payload; on a paused hook (`p`) it waits, pending, until the hook is resumed. |
| 8 | 8 | - **Sealed reports.** The delivery of a sealed report stays refused, as it is when the event first fires: `r` leaves the row failed. |
| 9 | 9 | - **Page.** Settings > Webhooks lists each hook's recent deliveries, and a `failed` row has a Redeliver button, a POST form to `/:owner/:name/settings/hooks/:id/deliveries/:delivery/redeliver` (`webhookRedeliver`, `src/web/pages/hooks.ts`). Only a repository admin sees it, the POST is same-origin only, and anyone else gets a 404. It redirects back to the page, where the row reads `pending`; a refused redelivery redirects with a notice and changes nothing. |
| 10 | - **Not yet.** There is no MCP action; the read-only deliveries list (`GET …/hooks/:id/deliveries`, MCP `hooks` with `action: deliveries`) has no control. | |
| 10 | - **API.** `POST /api/v1/repos/:owner/:name/hooks/:id/deliveries/:delivery/redeliver` (admin-scope token, a repository you administer; 404 otherwise) goes through the same action as the form (`redeliverWebhook`, `src/actions/repos.ts`) and answers `{ id, state: "pending" }`. A row that is not `failed`, or a sealed report's, answers 409 with a sentence and changes nothing. | |
| 11 | - **MCP.** `hooks` with `action: redeliver`, `id` and `delivery`: the same checks, a `conflict` error in the 409 cases. |
src/actions/repos.ts +22
| @@ -49,8 +49,11 @@ import type { User } from "../core/users.ts"; | ||
| 49 | 49 | import { |
| 50 | 50 | createWebhook as insertWebhook, |
| 51 | 51 | deleteWebhook as removeWebhook, |
| 52 | getWebhook, | |
| 53 | redeliver, | |
| 52 | 54 | setWebhookActive, |
| 53 | 55 | type CreateWebhookError, |
| 56 | type RedeliverError, | |
| 54 | 57 | type Webhook, |
| 55 | 58 | } from "../core/webhooks.ts"; |
| 56 | 59 | import { resolveOid } from "../git/merge.ts"; |
| @@ -718,6 +721,25 @@ export function pauseWebhook( | ||
| 718 | 721 | return ok(); |
| 719 | 722 | } |
| 720 | 723 | |
| 724 | /** | |
| 725 | * Send a failed delivery of this repository's hook again (admins). The one | |
| 726 | * action behind the Settings form, the API and MCP; `redeliver` does the | |
| 727 | * checks (only `failed` moves, a sealed report stays refused). | |
| 728 | */ | |
| 729 | export function redeliverWebhook( | |
| 730 | app: App, | |
| 731 | actor: User | undefined, | |
| 732 | repo: Repo, | |
| 733 | webhookId: string, | |
| 734 | deliveryId: string, | |
| 735 | ): Result<void, RedeliverError | "no_admin"> { | |
| 736 | const admin = requireRepoAdmin(app, actor, repo); | |
| 737 | if (!admin.ok) return err("no_admin"); | |
| 738 | if (!getWebhook(app.db, repo.id, webhookId)) return err("not_found"); | |
| 739 | const done = redeliver(app.db, deliveryId, webhookId); | |
| 740 | return done.ok ? ok() : done; | |
| 741 | } | |
| 742 | ||
| 721 | 743 | export type CommitToBranchError = |
| 722 | 744 | | AccessError |
| 723 | 745 | | CommitFilesError |
src/contract/openapi/routes/index.ts +1
| @@ -288,6 +288,7 @@ export { route as postApiV1ReposOwnerNameFollowingSever } from "./postApiV1Repos | ||
| 288 | 288 | export { route as postApiV1ReposOwnerNameFollowingTake } from "./postApiV1ReposOwnerNameFollowingTake.ts"; |
| 289 | 289 | export { route as postApiV1ReposOwnerNameForks } from "./postApiV1ReposOwnerNameForks.ts"; |
| 290 | 290 | export { route as postApiV1ReposOwnerNameHooks } from "./postApiV1ReposOwnerNameHooks.ts"; |
| 291 | export { route as postApiV1ReposOwnerNameHooksIdDeliveriesDeliveryRedeliver } from "./postApiV1ReposOwnerNameHooksIdDeliveriesDeliveryRedeliver.ts"; | |
| 291 | 292 | export { route as postApiV1ReposOwnerNameHooksIdPing } from "./postApiV1ReposOwnerNameHooksIdPing.ts"; |
| 292 | 293 | export { route as postApiV1ReposOwnerNameIssues } from "./postApiV1ReposOwnerNameIssues.ts"; |
| 293 | 294 | export { route as postApiV1ReposOwnerNameIssuesNumberComments } from "./postApiV1ReposOwnerNameIssuesNumberComments.ts"; |
src/contract/openapi/routes/postApiV1ReposOwnerNameHooksIdDeliveriesDeliveryRedeliver.ts added +9
| @@ -0,0 +1,9 @@ | ||
| 1 | import { REPO, type Route } from "../shared.ts"; | |
| 2 | ||
| 3 | export const route: Route = { | |
| 4 | method: "post", | |
| 5 | path: `${REPO}/hooks/{id}/deliveries/{delivery}/redeliver`, | |
| 6 | summary: | |
| 7 | "Send a `failed` delivery again (admin scope, a repository you administer): the same row goes back to `pending` with attempts reset; answers { id, state }, 409 if the row is not failed or belongs to a sealed report, 404 otherwise", | |
| 8 | auth: true, | |
| 9 | }; |
src/http/api/hooks.ts +15 −1
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | * since a webhook exfiltrates repo events. |
| 7 | 7 | */ |
| 8 | 8 | |
| 9 | import { createWebhook, deleteWebhook } from "../../actions/repos.ts"; | |
| 9 | import { createWebhook, deleteWebhook, redeliverWebhook } from "../../actions/repos.ts"; | |
| 10 | 10 | import { canAdmin } from "../../core/repos.ts"; |
| 11 | 11 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; |
| 12 | 12 | import { getWebhook, listWebhooks, pingWebhook, recentDeliveries } from "../../core/webhooks.ts"; |
| @@ -71,6 +71,20 @@ export function hookRoutes(router: Router<Handler>): void { | ||
| 71 | 71 | return json({ id: pinged.value.id }, 202); |
| 72 | 72 | }); |
| 73 | 73 | |
| 74 | router.add("POST", "/api/v1/repos/:owner/:name/hooks/:id/deliveries/:delivery/redeliver", (ctx) => { | |
| 75 | const denied = requireScope(ctx, "admin"); | |
| 76 | if (denied) return denied.status === 403 ? notFound() : denied; | |
| 77 | const repo = loadRepo(ctx); | |
| 78 | if (!repo) return notFound(); | |
| 79 | const done = redeliverWebhook(ctx.app, ctx.auth.user, repo, ctx.params.id!, ctx.params.delivery!); | |
| 80 | if (done.ok) return json({ id: ctx.params.delivery!, state: "pending" }); | |
| 81 | if (done.error === "not_failed") | |
| 82 | return json({ error: "conflict", message: "only a failed delivery can be sent again" }, 409); | |
| 83 | if (done.error === "sealed") | |
| 84 | return json({ error: "conflict", message: "this delivery belongs to a sealed report and stays refused" }, 409); | |
| 85 | return notFound(); | |
| 86 | }); | |
| 87 | ||
| 74 | 88 | router.add("POST", "/api/v1/repos/:owner/:name/hooks", async (ctx) => { |
| 75 | 89 | const denied = requireScope(ctx, "admin"); |
| 76 | 90 | if (denied) return denied; |
src/http/mcp/tools/hooks.ts +15 −5
| @@ -1,10 +1,10 @@ | ||
| 1 | 1 | import { canAdmin } from "../../../core/repos.ts"; |
| 2 | 2 | import { getWebhook, listWebhooks, pingWebhook, recentDeliveries } from "../../../core/webhooks.ts"; |
| 3 | import { createWebhook, deleteWebhook } from "../../../actions/repos.ts"; | |
| 3 | import { createWebhook, deleteWebhook, redeliverWebhook } from "../../../actions/repos.ts"; | |
| 4 | 4 | import { pageRows } from "../page.ts"; |
| 5 | 5 | import { good, bad, type Answer, type Args, type Tool, REPO_ARG, repoArg, isAnswer, str } from "../shared.ts"; |
| 6 | 6 | |
| 7 | const HOOK_LIMITS = { url: 2048, kinds: 200, secret: 200, id: 64 } as const; | |
| 7 | const HOOK_LIMITS = { url: 2048, kinds: 200, secret: 200, id: 64, delivery: 64 } as const; | |
| 8 | 8 | |
| 9 | 9 | /** The refusal for a `hooks` argument that is not text or is over its declared length. */ |
| 10 | 10 | function hookTooLong(args: Args): Answer | undefined { |
| @@ -20,17 +20,18 @@ const notAdmin = (): Answer => bad("not_found", "no such app or repository, or y | ||
| 20 | 20 | |
| 21 | 21 | export const tool: Tool = { |
| 22 | 22 | name: "hooks", |
| 23 | doc: "`{ repo, action, url, secret, kinds, id }`: `list` (pages: `limit` 1–200, `cursor`; `{ items, next_cursor }`), `add` (`secret`, 16–200 characters, is required) or `remove` the repository's webhooks, as `…/hooks`; `deliveries` with `id` answers that hook's last 20, newest first (`{ items: [{ id, state, attempts, lastStatus, lastError, kind }] }`); `ping` with `id` queues one `ping` delivery (`{ id }`), at most 3 a minute per hook; an admin-scope token on a repository you administer, and a secret is never shown", | |
| 23 | doc: "`{ repo, action, url, secret, kinds, id }`: `list` (pages: `limit` 1–200, `cursor`; `{ items, next_cursor }`), `add` (`secret`, 16–200 characters, is required) or `remove` the repository's webhooks, as `…/hooks`; `deliveries` with `id` answers that hook's last 20, newest first (`{ items: [{ id, state, attempts, lastStatus, lastError, kind }] }`); `ping` with `id` queues one `ping` delivery (`{ id }`), at most 3 a minute per hook; `redeliver` with `id` and `delivery` queues a failed delivery again (`{ id, state }`; 409-like `conflict` if not failed or sealed); an admin-scope token on a repository you administer, and a secret is never shown", | |
| 24 | 24 | description: "List, add (`url`, `secret`, `kinds`) or remove (`id`) webhooks; admins.", |
| 25 | 25 | inputSchema: { |
| 26 | 26 | type: "object", |
| 27 | 27 | properties: { |
| 28 | 28 | repo: REPO_ARG, |
| 29 | action: { type: "string", enum: ["list", "add", "remove", "deliveries", "ping"] }, | |
| 29 | action: { type: "string", enum: ["list", "add", "remove", "deliveries", "ping", "redeliver"] }, | |
| 30 | 30 | url: { type: "string", maxLength: 2048 }, |
| 31 | 31 | kinds: { type: "string", maxLength: 200 }, |
| 32 | 32 | secret: { type: "string", maxLength: 200 }, |
| 33 | 33 | id: { type: "string", maxLength: 64 }, |
| 34 | delivery: { type: "string", maxLength: 64 }, | |
| 34 | 35 | limit: { type: "number" }, |
| 35 | 36 | cursor: { type: "string" }, |
| 36 | 37 | }, |
| @@ -63,6 +64,15 @@ export const tool: Tool = { | ||
| 63 | 64 | const pinged = pingWebhook(ctx.app.db, `${repo.ownerName}/${repo.name}`, hook); |
| 64 | 65 | return pinged.ok ? good({ id: pinged.value.id }) : bad("rate_limited", `too many pings: try again in ${pinged.error.retryAfter} s`); |
| 65 | 66 | } |
| 67 | if (action === "redeliver") { | |
| 68 | const long = hookTooLong(args); | |
| 69 | if (long !== undefined) return long; | |
| 70 | const done = redeliverWebhook(ctx.app, user, repo, str(args["id"]), str(args["delivery"])); | |
| 71 | if (done.ok) return good({ id: str(args["delivery"]), state: "pending" }); | |
| 72 | if (done.error === "not_failed") return bad("conflict", "only a failed delivery can be sent again"); | |
| 73 | if (done.error === "sealed") return bad("conflict", "this delivery belongs to a sealed report and stays refused"); | |
| 74 | return done.error === "no_admin" ? notAdmin() : bad("not_found", "no such webhook or delivery here"); | |
| 75 | } | |
| 66 | 76 | if (action === "add") { |
| 67 | 77 | const long = hookTooLong(args); |
| 68 | 78 | if (long !== undefined) return long; |
| @@ -87,6 +97,6 @@ export const tool: Tool = { | ||
| 87 | 97 | const removed = deleteWebhook(ctx.app, user, repo, str(args["id"])); |
| 88 | 98 | return removed.ok ? good({}) : bad("not_found", "no such webhook here"); |
| 89 | 99 | } |
| 90 | return bad("invalid", "action is list, add, remove, deliveries or ping"); | |
| 100 | return bad("invalid", "action is list, add, remove, deliveries, ping or redeliver"); | |
| 91 | 101 | }, |
| 92 | 102 | }; |
src/web/pages/hooks.ts +4 −7
| @@ -7,9 +7,9 @@ | ||
| 7 | 7 | * place of its last delivery. |
| 8 | 8 | */ |
| 9 | 9 | |
| 10 | import { createWebhook, deleteWebhook, pauseWebhook } from "../../actions/repos.ts"; | |
| 10 | import { createWebhook, deleteWebhook, pauseWebhook, redeliverWebhook } from "../../actions/repos.ts"; | |
| 11 | 11 | import { notePhase, timePhase } from "../../core/route_timing.ts"; |
| 12 | import { getWebhook, listWebhooks, pingWebhook, recentDeliveries, redeliver } from "../../core/webhooks.ts"; | |
| 12 | import { getWebhook, listWebhooks, pingWebhook, recentDeliveries } from "../../core/webhooks.ts"; | |
| 13 | 13 | import type { Ctx } from "../../http/context.ts"; |
| 14 | 14 | import { repoBase, repoNotFound } from "../chrome.ts"; |
| 15 | 15 | import { field } from "../form.ts"; |
| @@ -168,14 +168,11 @@ export async function webhookRedeliver(ctx: Ctx): Promise<Response> { | ||
| 168 | 168 | const gate = await adminForm(ctx); |
| 169 | 169 | if (gate instanceof Response) return gate; |
| 170 | 170 | const { repo } = gate; |
| 171 | const hook = getWebhook(ctx.app.db, repo.id, ctx.params.id ?? ""); | |
| 172 | const done = hook | |
| 173 | ? redeliver(ctx.app.db, ctx.params.delivery ?? "", hook.id) | |
| 174 | : undefined; | |
| 171 | const done = redeliverWebhook(ctx.app, ctx.auth.user, repo, ctx.params.id ?? "", ctx.params.delivery ?? ""); | |
| 175 | 172 | return flashTo( |
| 176 | 173 | ctx, |
| 177 | 174 | `${repoBase(repo)}/settings/hooks`, |
| 178 | done?.ok | |
| 175 | done.ok | |
| 179 | 176 | ? { message: "Delivery queued again." } |
| 180 | 177 | : { tone: "danger", message: "That delivery cannot be sent again." }, |
| 181 | 178 | ); |
test/hook-redeliver-doors.test.ts added +114
| @@ -0,0 +1,114 @@ | ||
| 1 | /** Redeliver over the API and MCP `hooks` (#10105): a failed row goes back to pending; others are refused. */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createIssue } from "../src/core/issues.ts"; | |
| 9 | import { setSealed } from "../src/core/sealed.ts"; | |
| 10 | import { createToken } from "../src/core/tokens.ts"; | |
| 11 | import { createUser } from "../src/core/users.ts"; | |
| 12 | import { createWebhook, recentDeliveries } from "../src/core/webhooks.ts"; | |
| 13 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 14 | ||
| 15 | let dataDir = ""; | |
| 16 | let app: App, servers: Servers; | |
| 17 | let repoId = "", aliceId = ""; | |
| 18 | let tok = "", other = "", readTok = "", hook = ""; | |
| 19 | ||
| 20 | beforeAll(async () => { | |
| 21 | dataDir = await mkdtemp(join(tmpdir(), "forge-hook-redeliver-doors-")); | |
| 22 | app = createApp({ dataDir, port: 0 }); | |
| 23 | servers = await serve(app); | |
| 24 | const mk = async (username: string, first: boolean) => { | |
| 25 | const u = await createUser(app.db, { origin: "seed", trusted: true, username, email: `${username}@f.dev`, password: "a-good-passphrase", adminIfFirst: first }); | |
| 26 | if (!u.ok) throw new Error(String(u.error)); | |
| 27 | return u.value; | |
| 28 | }; | |
| 29 | const alice = await mk("alice", true); | |
| 30 | const bob = await mk("bob", false); | |
| 31 | const repo = await createRepo(app, alice, { name: "a", visibility: "public" }); | |
| 32 | if (!repo.ok) throw new Error("repo"); | |
| 33 | const h = createWebhook(app.db, { repoId: repo.value.id, url: "https://a.example/h" }); | |
| 34 | if (!h.ok) throw new Error("hook"); | |
| 35 | hook = h.value.webhook.id; | |
| 36 | repoId = repo.value.id; | |
| 37 | aliceId = alice.id; | |
| 38 | app.bus.append({ kind: "push", repoId: repo.value.id }); | |
| 39 | tok = createToken(app.db, alice.id, "t", "admin").token; | |
| 40 | readTok = createToken(app.db, alice.id, "r", "rw").token; | |
| 41 | other = createToken(app.db, bob.id, "o", "admin").token; | |
| 42 | }); | |
| 43 | afterAll(async () => { | |
| 44 | await servers.stop(); | |
| 45 | app.close(); | |
| 46 | await rm(dataDir, { recursive: true, force: true }); | |
| 47 | }); | |
| 48 | ||
| 49 | const fail = () => { | |
| 50 | app.db.run("UPDATE webhook_delivery SET state = 'failed', attempts = 3, last_status = 500, last_error = 'boom'"); | |
| 51 | return recentDeliveries(app.db, hook, 1)[0]!.id; | |
| 52 | }; | |
| 53 | const state = () => recentDeliveries(app.db, hook, 1)[0]!; | |
| 54 | const api = (id: string, t: string) => | |
| 55 | fetch(`${servers.url}/api/v1/repos/alice/a/hooks/${hook}/deliveries/${id}/redeliver`, { method: "POST", headers: { authorization: `Bearer ${t}` } }); | |
| 56 | const mcp = async (args: unknown, t = tok) => { | |
| 57 | const res = await fetch(`${servers.url}/mcp`, { | |
| 58 | method: "POST", | |
| 59 | headers: { "content-type": "application/json", accept: "application/json, text/event-stream", authorization: `Bearer ${t}` }, | |
| 60 | body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/call", params: { name: "hooks", arguments: args } }), | |
| 61 | }); | |
| 62 | const body = (await res.json()) as any; | |
| 63 | if (body.error) return { error: String(body.error.message) }; | |
| 64 | const text: string = body.result.content[0].text; | |
| 65 | return body.result.isError === true ? { error: text } : { value: JSON.parse(text) }; | |
| 66 | }; | |
| 67 | ||
| 68 | test("the API redelivers a failed row to pending, then refuses it with 409", async () => { | |
| 69 | const id = fail(); | |
| 70 | const res = await api(id, tok); | |
| 71 | expect(res.status).toBe(200); | |
| 72 | expect(state()).toMatchObject({ id, state: "pending", attempts: 0 }); | |
| 73 | const again = await api(id, tok); | |
| 74 | expect(again.status).toBe(409); | |
| 75 | expect(((await again.json()) as any).message).toContain("failed"); | |
| 76 | }); | |
| 77 | ||
| 78 | test("MCP redelivers a failed row to pending, then refuses it", async () => { | |
| 79 | const id = fail(); | |
| 80 | const ok = await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: id }); | |
| 81 | expect(ok.value).toEqual({ id, state: "pending" }); | |
| 82 | expect(state().state).toBe("pending"); | |
| 83 | expect((await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: id })).error).toContain("failed"); | |
| 84 | }); | |
| 85 | ||
| 86 | test("strangers, short scopes and unknown rows get 404; none of them moves the row", async () => { | |
| 87 | const id = fail(); | |
| 88 | expect((await api(id, other)).status).toBe(404); | |
| 89 | expect((await api(id, readTok)).status).toBe(404); | |
| 90 | expect((await api("nope", tok)).status).toBe(404); | |
| 91 | expect(state().state).toBe("failed"); | |
| 92 | expect((await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: "nope" })).error).toContain("no such"); | |
| 93 | expect((await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: id }, other)).error).toBeTruthy(); | |
| 94 | expect(state().state).toBe("failed"); | |
| 95 | }); | |
| 96 | ||
| 97 | test("a failed delivery of a sealed report stays failed: 409 on the API, conflict on MCP", async () => { | |
| 98 | const issue = createIssue(app.db, { repoId, authorId: aliceId, title: "hole" }); | |
| 99 | if (!issue.ok) throw new Error("issue"); | |
| 100 | // delivered while open (a sealed event is never queued), sealed afterwards | |
| 101 | app.bus.append({ kind: "issue.opened", repoId, payload: { number: issue.value.number } }); | |
| 102 | const id = fail(); | |
| 103 | expect(recentDeliveries(app.db, hook, 10).length).toBeGreaterThan(1); | |
| 104 | app.db.run("UPDATE webhook_delivery SET state = 'failed' WHERE id = ?1", [id]); | |
| 105 | setSealed(app.db, issue.value.id, true); | |
| 106 | const res = await api(id, tok); | |
| 107 | expect(res.status).toBe(409); | |
| 108 | expect(((await res.json()) as any).message).toContain("sealed"); | |
| 109 | expect(state().state).toBe("failed"); | |
| 110 | expect((await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: id })).error).toContain("sealed"); | |
| 111 | expect(state().state).toBe("failed"); | |
| 112 | expect((await mcp({ repo: "alice/a", action: "redeliver", id: hook, delivery: id }, readTok)).error).toBeTruthy(); | |
| 113 | expect(state().state).toBe("failed"); | |
| 114 | }); |
Missing test: a failed delivery of a sealed report, redelivered through the API (expect 409, message mentions sealed) and through MCP (expect conflict error). Both branches are new in this pull and nothing here would fail if they were deleted. Assert the row stays failed.