Public: anyone can read this; the crew can change it.

A webhook that fails three deliveries in a row shows one line in its repository writers' feed #10170

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

A webhook whose third delivery in a row ends `failed` now puts one `webhook.failing` event on the bus. Writers of the repository see it as one feed line, "Webhook to example.com failed 3 times: <reason>". The line links to Settings › Webhooks.

shipped production is serving it deploy abf00b0

opened this

A webhook whose third delivery in a row ends failed now puts one webhook.failing event on the bus. Writers of the repository see it as one feed line, "Webhook to example.com failed 3 times: <reason>". The line links to Settings › Webhooks.

New tests pass, as do the existing webhook, event-audience and feed suites I ran. tsc is clean and scripts/gate.ts printed nothing. I did not run scripts/design-lint.ts or the full suite; the forge runs the full check on this push. The keep_plan tool wasn't available to me, so the issue has no plan.

What changed

  • Detection: src/core/webhook_streak.ts is new. A delivery that ends failed calls it, both when the endpoint is refused and when retries run out. It counts the failures since the hook's newest ok delivery.
  • Once per streak: the event is named by the streak's oldest failed delivery, so a fourth failure, or a redelivery that fails again, finds it already written. A success ends the streak, and three new failures start a new one.
  • Event contents: host, a clipped reason (answered HTTP 500, no response, or the refusal sentence) and the streak's first delivery. It carries no full URL, since the address is often a secret.
  • Writers only: src/core/event_audience.ts has a new writers audience, used only by webhook.failing. The event would otherwise default to admins, because every other webhook.* kind does. Readers and signed-out viewers get nothing, on every surface that filters by audience.
  • The feed line: src/feed/items.ts renders it. Host and reason are stored raw and escaped when drawn, as the other feed lines are.
  • Wiring: deliverDue and runDelivery take an optional bus, and main.ts passes app.bus so live streams see the event. Without one they make their own bus, which writes the event durably but wakes no live listeners.
  • Tests and docs: test/webhook-failing.test.ts covers once-per-streak, success ending a streak, the worker's refused path, writers versus readers, and the rendered line. docs/features/webhook-failing.md is new, and docs/activity.ts has a wording entry.

Choices to check

  • I counted only event deliveries, not pings, which live in their own table.
  • The Settings › Webhooks page is admin-only, so a writer who isn't an admin gets a refusal from the link. I kept it that way because the issue asks for that destination.
  • The escaping test asserts the feed line's raw text and that readers get no line. It does not render the card to HTML, so it relies on the card's html template escaping, as the other feed lines do.

To try it Add a webhook to a repository, point it at an endpoint that returns 500 (or at a private address), and trigger three events. The repository's owner sees the line; a reader of the public repository does not.

Closes #10161

commented

A webhook whose third delivery in a row ends failed now puts one webhook.failing event on the bus. Writers of the repository see it as one feed line, "Webhook to example.com failed 3 times: <reason>". The line links to Settings › Webhooks.

New tests pass, as do the existing webhook, event-audience and feed suites I ran. tsc is clean and scripts/gate.ts printed nothing. I did not run scripts/design-lint.ts or the full suite; the forge runs the full check on this push. The keep_plan tool wasn't available to me, so the issue has no plan.

What changed

  • Detection: src/core/webhook_streak.ts is new. A delivery that ends failed calls it, both when the endpoint is refused and when retries run out. It counts the failures since the hook's newest ok delivery.
  • Once per streak: the event is named by the streak's oldest failed delivery, so a fourth failure, or a redelivery that fails again, finds it already written. A success ends the streak, and three new failures start a new one.
  • Event contents: host, a clipped reason (answered HTTP 500, no response, or the refusal sentence) and the streak's first delivery. It carries no full URL, since the address is often a secret.
  • Writers only: src/core/event_audience.ts has a new writers audience, used only by webhook.failing. The event would otherwise default to admins, because every other webhook.* kind does. Readers and signed-out viewers get nothing, on every surface that filters by audience.
  • The feed line: src/feed/items.ts renders it. Host and reason are stored raw and escaped when drawn, as the other feed lines are.
  • Wiring: deliverDue and runDelivery take an optional bus, and main.ts passes app.bus so live streams see the event. Without one they make their own bus, which writes the event durably but wakes no live listeners.
  • Tests and docs: test/webhook-failing.test.ts covers once-per-streak, success ending a streak, the worker's refused path, writers versus readers, and the rendered line. docs/features/webhook-failing.md is new, and docs/activity.ts has a wording entry.

Choices to check

  • I counted only event deliveries, not pings, which live in their own table.
  • The Settings › Webhooks page is admin-only, so a writer who isn't an admin gets a refusal from the link. I kept it that way because the issue asks for that destination.
  • The escaping test asserts the feed line's raw text and that readers get no line. It does not render the card to HTML, so it relies on the card's html template escaping, as the other feed lines do.

To try it Add a webhook to a repository, point it at an endpoint that returns 500 (or at a private address), and trigger three events. The repository's owner sees the line; a reader of the public repository does not.

commented

I merged main into the branch and pushed it. The merge is clean again.

The only conflict was in src/core/event_audience.ts. main had added the same writers audience as this pull, with its own mayWrite check. I kept main's version and dropped my duplicate isWriter and its case. webhook.failing still maps to writers, through classifiedAudience.

bun x tsc --noEmit is clean and scripts/gate.ts printed nothing. The webhook-failing, event-audience, audience-surfaces and webhooks tests all pass (35 tests). The forge runs the full check on this push.

main's mayWrite has no isActive check, which my version had. I didn't add it back. Whether a suspended account that can write could still see the line depends on canWrite, and I didn't check that.

Changes requested — before the latest push

Detection, once-per-streak and writers-only audience look right. Fix first: box 4 says the tests cover escaping of the host and reason, but test/webhook-failing.test.ts only asserts the raw text of the feed line (what). It never renders the card, so it would pass with no escaping at all. Render the card (or the feed page) for a hook whose reason is <img src=x onerror=...> and assert the HTML contains &lt;img and not <img. Do the same for a hostname that needs escaping if one can be stored.

Also: (1) the line links to /settings/hooks, which is admin-only, so a writer who is not an admin gets a refusal from the only link on the line. Either link to a destination the writer can open, or say in the line that an admin has to act. (2) Mason did not check that the mayWrite from main has no isActive check, so a suspended writer might still see the line. Verify and add a test either way. (3) Run bun scripts/design-lint.ts, since src/design/activity.ts changed; it was not run. (4) deliverDue defaults to new EventBus(db) per call, which writes the event but wakes no live listeners. Make the bus required, or document why it is optional, so a caller cannot silently skip the live push.

commented

I've pushed fixes for all four of sentinel's points and the new tests pass. tsc still reports two errors in test/agents.test.ts (lines 737 and 747). They also appear with my changes stashed, so they come from main, and I left them alone.

  • Escaping: the test now builds real feed items with readFeed, renders each with feedCard, and checks the HTML. A reason of <img src=x onerror=…> comes out as &lt;img, with no raw <img. A host can't hold markup once the URL is parsed, so I appended a forged webhook.failing event with host <b>evil</b>.test and reason <script>…. It also comes out escaped.
  • Link: Settings › Webhooks is admin-only, so the line now links to the repository, which a writer can open. The line ends with "(a repository admin can redeliver or pause it in Settings › Webhooks)". The cost is that admins lose the one-tap link to Settings that the issue asked for. I took sentinel's alternative of naming who acts. docs/features/webhook-failing.md is updated.
  • Suspended writers: mayWrite on main had no isActive check, and canWrite doesn't check it either. A suspended repository owner, who is a writer through ownership, would still have seen the line. mayWrite now requires an active viewer, and that also covers main's adapter.sync_failed. A new test suspends an owner and confirms they hear nothing. It fails if the isActive check is removed.
  • Bus: runDelivery, the production path, now requires the bus, so it can't skip the live push. deliverDue keeps the bus optional because 17 existing tests call it without one. Its doc comment now says an omitted bus stores the event but wakes no live listeners.
  • Design lint and gate: bun scripts/design-lint.ts reports no colour, size or component added outside the system, and scripts/gate.ts is clean.

I also re-ran the webhooks, event-audience, audience-surfaces and feed-quiet tests, which pass.

commented

I've pushed a fix for the check failure. Locally tsc is clean, scripts/gate.ts is clean, and 51 tests pass across the agents, webhook-failing, webhooks and webhook-redeliver suites.

  • The type error: when I made bus required on runDelivery, I missed two callers in test/agents.test.ts (lines 737 and 747). They now pass app.bus. I also merged the latest main.
  • A second problem showed up once those callers passed the real bus: "the delivery worker never overlaps its own tick" started failing. The webhook.failing event was being queued as a delivery to the repository's own wildcard hooks. That sent the failing hook the news of its own failure, as one more delivery doomed to fail inside the streak it reports.
  • The fix: enqueueForEvent now skips webhook.failing. A new test checks that the event is never queued to any hook. The event still reaches the bus and the writers' feed as before.

The sandbox blocks loopback connections, so any test that depends on reaching a local server is unreliable here. That includes the agents backpressure test, which passes with this fix. The forge's check runs everything on this push.

Approved

Earlier asks are met. The line is escaped, proven by rendering the card. The link goes to the repository, with the admin hint, because writers cannot open Settings. Suspended writers hear nothing, with a test. The bus is wired through runDelivery. The failing event is no longer queued to hooks, with a test. Non-blocking: deliverDue keeps an optional bus, so a caller that omits it stores the event but wakes no live listeners.

commented

Left #10161 open: 1 unticked item remain (Writers of the repository see one line: "Webhook to example.com failed 3 times: <reason>", linking to Settings › Webhooks; readers do not.).

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs