[Bot] Fail loudly instead of silently black-holing every event on a misconfigured notification channel

claude-bot commented

2026-08-25 20:42:09 +00:00

Collaborator

Impact: CRITICAL

Found in the August 2026 session lifecycle review (#319).

What the user experiences

A GM types a Discord channel ID into a plain text field with no validation and no instructions. If it's wrong — non-numeric, deleted, or the bot lacks permission to post there — every single notification the product would ever send to that campaign (reminders, vote posts, summaries, everything) silently vanishes, forever, with no error in Discord and no error in the web app. This is the single most likely first-run misconfiguration for a new hosted customer, and today it is completely undiagnosable from either side of the product.

Evidence

bot/questboard_bot/cogs/notifications.py:291-314 — _resolve_channel returns None for a non-integer, deleted, forbidden, or non-text channel with no caller-visible signal.
bot/questboard_bot/services/redis_bus.py (_process_entry, lines ~85-113, docstring at :93-95: "The entry is XACKed even when the handler could not post ... acking avoids a poison entry blocking the group forever") — the stream entry is acknowledged as delivered regardless of whether _resolve_channel actually succeeded, so there is no durable record that delivery failed either.
webapp/frontend/src/pages/CampaignDetail.jsx:1830-1836 — "Bot notification channel ID (optional)" is a bare free-text field with no instructions and no validation, inside a collapsed Edit Campaign form.

Why it matters for a hosted product

A typo in one text field produces total, permanent, silent loss of every downstream notification with no signal on either side. This is the textbook "the bot just stopped working" ticket, and today there is no way for a self-hoster or a support agent to diagnose it without reading logs.

Proposed fix

redis_bus._process_entry must not XACK when the handler could not post (or must XACK but record the failure separately so it isn't silently discarded). notifications.py's channel-resolution failure should be surfaced back to the web app — e.g. via a lightweight status callback or a periodic health check — so CampaignDetail can show "Discord notifications are failing — check the channel ID" (the audit's P16). The existing "Send test message" button (CampaignDetail.jsx:1839-1846) is a genuinely good pattern already in the codebase for this kind of feedback — extend that pattern rather than inventing a new one, and make it available for each ID field independently rather than only once both webhook and channel ID are filled (:1837).

Acceptance criteria

A channel-resolution failure is recorded (not just logged) and does not get silently XACKed as "delivered".
CampaignDetail.jsx shows a persistent, visible warning when the configured notification channel is failing to deliver.
The "Send test message" affordance (or an equivalent) can validate the channel ID independently of the webhook URL.
A regression test covers the case where _resolve_channel returns None and asserts the entry is not silently marked delivered.

**Impact: CRITICAL** Found in the August 2026 session lifecycle review (#319). ## What the user experiences A GM types a Discord channel ID into a plain text field with no validation and no instructions. If it's wrong — non-numeric, deleted, or the bot lacks permission to post there — every single notification the product would ever send to that campaign (reminders, vote posts, summaries, everything) silently vanishes, forever, with no error in Discord and no error in the web app. This is the single most likely first-run misconfiguration for a new hosted customer, and today it is completely undiagnosable from either side of the product. ## Evidence - `bot/questboard_bot/cogs/notifications.py:291-314` — `_resolve_channel` returns `None` for a non-integer, deleted, forbidden, or non-text channel with no caller-visible signal. - `bot/questboard_bot/services/redis_bus.py` (`_process_entry`, lines ~85-113, docstring at :93-95: *"The entry is XACKed even when the handler could not post ... acking avoids a poison entry blocking the group forever"*) — the stream entry is acknowledged as delivered regardless of whether `_resolve_channel` actually succeeded, so there is no durable record that delivery failed either. - `webapp/frontend/src/pages/CampaignDetail.jsx:1830-1836` — "Bot notification channel ID (optional)" is a bare free-text field with no instructions and no validation, inside a collapsed Edit Campaign form. ## Why it matters for a hosted product A typo in one text field produces total, permanent, silent loss of every downstream notification with no signal on either side. This is the textbook "the bot just stopped working" ticket, and today there is no way for a self-hoster or a support agent to diagnose it without reading logs. ## Proposed fix `redis_bus._process_entry` must not XACK when the handler could not post (or must XACK but record the failure separately so it isn't silently discarded). `notifications.py`'s channel-resolution failure should be surfaced back to the web app — e.g. via a lightweight status callback or a periodic health check — so `CampaignDetail` can show "Discord notifications are failing — check the channel ID" (the audit's P16). The existing "Send test message" button (`CampaignDetail.jsx:1839-1846`) is a genuinely good pattern already in the codebase for this kind of feedback — extend that pattern rather than inventing a new one, and make it available for each ID field independently rather than only once both webhook and channel ID are filled (`:1837`). ## Acceptance criteria - [ ] A channel-resolution failure is recorded (not just logged) and does not get silently XACKed as "delivered". - [ ] `CampaignDetail.jsx` shows a persistent, visible warning when the configured notification channel is failing to deliver. - [ ] The "Send test message" affordance (or an equivalent) can validate the channel ID independently of the webhook URL. - [ ] A regression test covers the case where `_resolve_channel` returns `None` and asserts the entry is not silently marked delivered.

claude-bot added this to the v4.3.0 - Product Surface: Navigation, Attention & Feedback milestone

2026-08-25 20:42:09 +00:00

claude-bot added the

bug

bot

labels

2026-08-25 20:42:09 +00:00

claude-bot referenced this issue

2026-09-06 00:34:01 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot commented

2026-09-06 02:03:14 +00:00

Author

Collaborator

Picking this up as v4.3.0 phase 5, lane A (#514), followed by #371 on the same branch. Decisions: the failure is recorded durably on the campaign (discord_delivery_status JSONB, one migration) through a new additive bot→backend endpoint, cleared by the next successful post; the stream entry is still acked, but only after the failure is recorded, so nothing is both unrecorded and acked; CampaignDetail shows a persistent banner while delivery is failing; the test-message affordance can test the channel ID on its own and the input validates the ID shape.

Picking this up as v4.3.0 phase 5, lane A (#514), followed by #371 on the same branch. Decisions: the failure is recorded durably on the campaign (`discord_delivery_status` JSONB, one migration) through a new additive bot→backend endpoint, cleared by the next successful post; the stream entry is still acked, but only after the failure is recorded, so nothing is both unrecorded and acked; `CampaignDetail` shows a persistent banner while delivery is failing; the test-message affordance can test the channel ID on its own and the input validates the ID shape.

claude-bot referenced this issue

2026-09-06 02:03:16 +00:00

[Bot] Acknowledge Discord votes and RSVPs, and fix the Discord/web vote asymmetry #371

claude-bot referenced this issue

2026-09-06 02:23:52 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot referenced this issue

2026-09-06 03:12:47 +00:00

v4.3.0 build order: Product Surface in eight phases #514

rbrooks referenced this issue from a commit

2026-09-06 04:02:51 +00:00

fix(bot/webapp): stop black-holing every notification on a bad channel ID (#369)

claude-bot referenced this issue from a pull request that will close it,

2026-09-06 04:03:13 +00:00

feat(bot/webapp): a misconfigured channel fails loudly, and Discord votes answer back (phase 5 lane A) #523

claude-bot commented

2026-09-06 04:03:37 +00:00

Author

Collaborator

Done in PR #523 (auto-merging on green); ships with v4.3.0.

A channel-resolution failure is now recorded rather than only logged. _resolve_channel returns why it failed (invalid_id / not_found / forbidden / not_text / unknown) and on_bot_notify posts that to a new additive POST /api/bot/campaigns/{id}/delivery-status, which writes a nullable JSONB campaigns.discord_delivery_status ({state, reason, channel_id, event_type, at, message}; migration f0ab1c2d3e4f on e9fa0b1c2d3e, one head; at is stamped by the backend so a bot on another machine cannot produce a future timestamp). NULL is deliberately not "ok" and nothing is backfilled: an untried channel has demonstrated nothing.

A fifth reason the issue did not list, cannot_send, comes from a discord.Forbidden on the post rather than the resolve. A channel the bot can see but not speak in was indistinguishable from success, and it is the commonest misconfiguration after a wrong ID.

The entry is still acked (the poison-entry argument stands), but the record is written before the ack, and if the record also fails that goes to the log at ERROR with campaign, reason and event type, so nothing is ever both unrecorded and acked without a trace. Both paths are regression-tested.

CampaignResponse exposes the status to every member (a channel ID, a reason code and a sentence; no secrets, and a player wondering why the reminders stopped is owed the same answer). CampaignDetail renders it as a banner at the top of the page, outside the collapsed Edit form, since putting it behind the same disclosure as the broken field would inherit the invisibility. It clears itself on the first successful post, including the first after a bot restart.

The "Send test message" button had three further problems and is replaced: it was admin-only on a page any GM can open (a 403 for most self-hosters), it reported success on having published to Redis rather than on anything arriving (the same lie as the black hole), and it tested channel and webhook as one thing. There are now two buttons, one per field, hitting a GM-authorised POST /api/campaigns/{id}/discord-test that is synchronous end to end: the channel test calls a new POST /test-channel on the bot, which resolves and posts before answering, and the verdict is written to the delivery status, so a passing test clears the banner; the webhook test is guarded by the existing Discord-webhook URL check. The channel field rejects anything that is not 17–20 digits, with a line saying where to find the ID. An older bot returns 404 on /test-channel, which the backend reports as "upgrade the bot image" rather than blaming the channel ID.

No BOT_CONTRACT_VERSION bump: the endpoint is purely additive and an older bot never calls it. Bot 432 tests pass, backend 2405 / 13 skipped, frontend 539, ruff and version-sync clean, migration round-trips up/down/up on Postgres 16.

Done in PR #523 (auto-merging on green); ships with v4.3.0. A channel-resolution failure is now recorded rather than only logged. `_resolve_channel` returns *why* it failed (`invalid_id` / `not_found` / `forbidden` / `not_text` / `unknown`) and `on_bot_notify` posts that to a new additive `POST /api/bot/campaigns/{id}/delivery-status`, which writes a nullable JSONB `campaigns.discord_delivery_status` (`{state, reason, channel_id, event_type, at, message}`; migration `f0ab1c2d3e4f` on `e9fa0b1c2d3e`, one head; `at` is stamped by the backend so a bot on another machine cannot produce a future timestamp). NULL is deliberately not "ok" and nothing is backfilled: an untried channel has demonstrated nothing. A fifth reason the issue did not list, **`cannot_send`**, comes from a `discord.Forbidden` on the *post* rather than the resolve. A channel the bot can see but not speak in was indistinguishable from success, and it is the commonest misconfiguration after a wrong ID. The entry is still acked (the poison-entry argument stands), but the record is written before the ack, and if the record *also* fails that goes to the log at ERROR with campaign, reason and event type, so nothing is ever both unrecorded and acked without a trace. Both paths are regression-tested. `CampaignResponse` exposes the status to every member (a channel ID, a reason code and a sentence; no secrets, and a player wondering why the reminders stopped is owed the same answer). `CampaignDetail` renders it as a banner at the top of the page, outside the collapsed Edit form, since putting it behind the same disclosure as the broken field would inherit the invisibility. It clears itself on the first successful post, including the first after a bot restart. The "Send test message" button had three further problems and is replaced: it was admin-only on a page any GM can open (a 403 for most self-hosters), it reported success on having *published to Redis* rather than on anything arriving (the same lie as the black hole), and it tested channel and webhook as one thing. There are now two buttons, one per field, hitting a GM-authorised `POST /api/campaigns/{id}/discord-test` that is synchronous end to end: the channel test calls a new `POST /test-channel` on the bot, which resolves and posts before answering, and the verdict is written to the delivery status, so a passing test clears the banner; the webhook test is guarded by the existing Discord-webhook URL check. The channel field rejects anything that is not 17–20 digits, with a line saying where to find the ID. An older bot returns 404 on `/test-channel`, which the backend reports as "upgrade the bot image" rather than blaming the channel ID. No `BOT_CONTRACT_VERSION` bump: the endpoint is purely additive and an older bot never calls it. Bot 432 tests pass, backend 2405 / 13 skipped, frontend 539, ruff and version-sync clean, migration round-trips up/down/up on Postgres 16.

claude-bot closed this issue

2026-09-06 04:20:51 +00:00

rbrooks referenced this issue from a commit

2026-09-06 04:22:08 +00:00

chore(alembic): chain the RSVP migration after the delivery-status one (#370, #369)

claude-bot referenced this issue

2026-09-06 04:22:13 +00:00

v4.3.0 build order: Product Surface in eight phases #514

claude-bot referenced this issue

2026-09-06 04:22:52 +00:00

[Frontend] Add a web RSVP control — RSVP has no web surface at all #370

claude-bot referenced this issue