Repository navigation
feat(aggregator): alert throat — sentinel intake, Supabase ledger, Web Push delivery + ack (#3676) - #3680
Conversation
…n the #2445 merge 4d7f643 landed without the function's closing brace, leaving src/feed.ts unparseable: the aggregator cannot boot from current main at all (the running service survives only because it started before the merge), and every test importing feed.ts errors. One character, verified by the nav/curation/charges-feed-gate suites going green again.
…b Push delivery + ack (#3676) Implements #3676 (Drew approved 2026-11-01), building the #1254 design's slices (a) installability + (b) push plumbing with an incident class, ahead of the dashboard classes in #3317: - PWA: /manifest.webmanifest (start_url = the attention view), generated icons incl. maskable + apple-touch, install meta in the shared layout, PM_AGG_PUBLIC_ORIGIN defaulting to the Tailscale Serve HTTPS origin (verified: serve 443 -> 127.0.0.1:4870). - /sw.js is the real worker (push + notificationclick) with the design §6 fallback eviction carried forward (activate cache purge + page-side foreign-registration unregister via the probe script); /api/sw-probe counts registration reports. - Subscription API over Supabase alerts.push_subscriptions (per-class map + quiet prefs round-trip; 404/410 gone-count pruning at 2). VAPID private key fetched by name from Bitwarden at runtime, memory only; sending via the web-push package. - POST /api/alerts intake (bearer token = the bws secret pm-aggregator.alerts-intake-token): dedupe rides the partial unique index; every transition writes alerts.incident_events. - Engine tick (~60s): new open critical -> immediate Web Push to incident-class subscriptions, delivered_at + notification_sends rows; unacked critical past ALERT_ACK_WINDOW_MIN (default 15) -> exactly one escalation push. Warnings: page + ledger only. - /alerts page: open incidents, exactly-once ack, close, ledger history, subscribe/preference controls; push payloads deep-link to /alerts#incident-<id>. scripts/post-alert.ts is the sender CLI. - Supabase access reuses the collector's exact path (Management API SQL, bws supabase.access-token-muse-cli) in src/supabase.ts; secrets resolve by name in src/secrets.ts. Tests: test/alerts.test.ts (13 pure-helper tests) green; boot test's kill-switch assertion updated to the real worker per §6 + a manifest test added. Full suite on this branch is additionally blocked by a pre-existing main breakage (feed.ts brace dropped in #2445) — fixed separately on fix/feed-ts-missing-brace.
…feat/alert-throat-3676
…s page (#3676) - curation-1497: record the Alerts page in the decision table at dropdown tier (post-#1497 addition, no primary slot without a new curation decision). - nav #1135: /manifest.webmanifest joins NON_PAGE — PWA plumbing, not a navigable page. - a11y #1215: the /alerts ledger table opens with a <caption> and scope="col" header cells like every other aggregator table. Full suite: 870 pass / 0 fail.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 58 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7b849b7c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const summary = await deliverToAll(store, targets, incident, cfg.publicOrigin, false); | ||
| await store.markDelivered(incident.id, { ...summary }); |
There was a problem hiding this comment.
When every push attempt fails, such as during a push-provider outage, deliverToAll() returns a summary with zero successful sends, but the engine still sets delivered_at. This removes the incident from subsequent delivery ticks despite nobody receiving it. The escalation path likewise sets escalated_at after failed sends, permanently exhausting both notification opportunities if the outage spans the acknowledgement window. Only finalize a delivery or escalation after successful delivery, and retain failed attempts for retry.
Useful? React with 👍 / 👎.
| configurePromise = (async () => { | ||
| const priv = await resolveBwsSecret(VAPID_PRIVATE_KEY_SECRET); | ||
| if (!priv) return false; |
There was a problem hiding this comment.
If Bitwarden is temporarily unavailable on the first push attempt, this caches a promise resolving to false indefinitely. Subsequent calls reuse that promise instead of retrying secret resolution, even after the secret cache's five-minute expiry and Bitwarden recovery. The HTTP 401/403 reset cannot recover this state because sendPush() returns before contacting the provider. Clear unsuccessful initialization promises so a transient lookup failure does not disable all push delivery until the server restarts.
Useful? React with 👍 / 👎.
| if (!clientPromise) { | ||
| clientPromise = (async () => { | ||
| const token = await resolveBwsSecret("supabase.access-token-muse-cli"); | ||
| if (!token) return null; |
There was a problem hiding this comment.
If the first management-token lookup fails temporarily, clientPromise permanently retains the resulting null, and getAlertsStore() additionally caches that unavailable result in storePromise. Consequently intake, acknowledgement, and the delivery engine remain unavailable after Bitwarden recovers; the periodic engine ticks never attempt initialization again. Retry unsuccessful initialization at both cache layers rather than requiring a process restart to restore the entire alert pipeline.
Useful? React with 👍 / 👎.
| <label><input type="checkbox" id="push-class-incident" checked> incident class</label> | ||
| <label>quiet <input type="time" id="push-quiet-start" value="23:00">–<input type="time" id="push-quiet-end" value="07:00"></label> |
There was a problem hiding this comment.
For an existing subscription, these controls always render the defaults rather than its saved preferences, and neither the inline script nor push-client.js subsequently loads those preferences. After disabling the incident class and reloading, changing only the quiet-hour start and clicking Save preferences submits incident: true and the default quiet-hour end, silently undoing the notification opt-out and other saved settings. Initialize the controls from the subscription matching this browser's endpoint before allowing preference updates.
Useful? React with 👍 / 👎.
| const sub = await window.pmPush.current().catch(() => null); | ||
| status.textContent = sub ? "This browser is subscribed — pushes will land here." : "This browser is not subscribed yet."; | ||
| } | ||
| refreshStatus(); |
There was a problem hiding this comment.
On every normal Alerts page load, this inline script calls refreshStatus() during HTML parsing, before the deferred /assets/push-client.js script emitted by layout() executes. Consequently window.pmPush is still undefined and the page reports that Web Push is unsupported even in supported, already-subscribed browsers. Nothing refreshes the status after the client initializes. Run this initialization after the deferred script, such as on DOMContentLoaded, so the displayed delivery status reflects the browser's actual subscription.
Useful? React with 👍 / 👎.
| ); | ||
| if (rows[0]) { | ||
| await this.addEvent(id, "acked", {}); | ||
| await this.db.query( | ||
| `update alerts.notification_sends set acted_on_at = now() where incident_id = ${sqlStr(id)} and acted_on_at is null`, |
There was a problem hiding this comment.
If the acknowledgement event insertion fails after the preceding status update succeeds, the request fails but the incident remains acked. A retry cannot repair the missing event or stamp notification_sends.acted_on_at: the update's status = 'open' predicate no longer matches, so both follow-up writes are skipped permanently. Execute the status transition, event insertion, and send updates in one database transaction so a transient query failure cannot leave an acknowledged incident without its durable acknowledgement record.
Useful? React with 👍 / 👎.
| objectRef: incidentObjectRef(incident.id), | ||
| url: incidentUrl(origin, incident.id), | ||
| title: escalation ? `Escalation: ${incident.title}` : incident.title, | ||
| body: escalation ? `Still unacknowledged: ${base} (source: ${incident.source})` : `${base} (source: ${incident.source})`, |
There was a problem hiding this comment.
For incidents containing long diagnostic output, intake accepts the complete body and this function embeds it unbounded into the push payload. A valid 4,000-byte diagnostic body already makes the plaintext JSON exceed the 3,993-byte budget supported by services enforcing the standard 4,096-byte encrypted-payload limit, so those providers reject the notification regardless of retries. RFC 8291 §4 specifies these limits. Apply a UTF-8 byte budget to the complete serialized notification, truncating its display text while retaining the full diagnostics in the ledger and linked incident page.
Useful? React with 👍 / 👎.
Implements #3676 (Drew approved 2026-11-01): every sentinel posts incidents to ONE intake; one engine owns dedupe, delivery, escalation, and ack; every transition lands in the Supabase
alertsledger. Delivery is the #1254 PWA Web Push design (slices a+b of #3315/#3316 with anincidentclass, ahead of #3317's dashboard classes).Highlights:
PM_AGG_PUBLIC_ORIGINdefaults to the verified Tailscale Serve origin (443 → 127.0.0.1:4870).Tests: full aggregator suite 870 pass / 0 fail on this branch (incl. 13 new alerts helper tests, the updated §6 worker test, a manifest test, and the curation/a11y/nav updates for the new page).
Need help on this PR? Tag
@codesmith-botwith what you need.