Skip to content

Make notifications a first-class system service - #650

Closed
deregtd wants to merge 6 commits into
mainfrom
ddr/notification-system-worker
Closed

deregtd wants to merge 6 commits into
mainfrom
ddr/notification-system-worker

Conversation

@deregtd

@deregtd deregtd commented Oct 2, 2026 •

Copy link
Copy Markdown

Summary

Make notifications a first-class Cloudflare OS platform feature instead of a gatekeeper.

  • adds live browser notification delivery with a three-second acknowledgement window and push fallback
  • adds a preinstalled, non-public notification-proxy system Worker per CFOS installation
  • keeps install signing material out of Workshop core and makes the proxy unavailable to agents, connector discovery, and the public router
  • adds typed task-completed and permission-requested notifications with bounded titles and same-origin deep links
  • prevents the browser notification storm by keeping the subscription stable across toast-manager rerenders
  • extends the release manifest and local dev orchestration with an explicit system Worker kind

Production push delivery depends on the companion gadgets-internal MR !416. Self-hosted installs remain functional: browser presentation works and push registration reports that central delivery is unavailable.

Registration flow and boundaries

flowchart LR
  subgraph Phone[Native app and Apple boundary]
    APNS[APNs device token]
    App[Cloudflare OS app]
  end

  subgraph Central[Cloudflare-operated notification service]
    Device[Device registration API]
    Registry[(Device and subscription directory)]
  end

  subgraph Install[One customer CFOS installation]
    Browser[Authenticated Workshop session]
    User[User Durable Object]
    Proxy[notification-proxy]
    ProxyState[(Opaque subscription id)]
    Key[Install signing private key]
  end

  APNS -->|device token| App
  App -->|Dashboard OAuth plus device token| Device
  Device -->|store token; return one-time id| Registry
  Device -->|one-time registration id| App
  App -->|inject opaque id| Browser
  Browser -->|registerNotificationDevice| User
  User -->|account id plus one-time id| Proxy
  Key -->|sign request locally| Proxy
  Proxy -->|signed POST /v1/subscriptions| Device
  Device -->|validate install; consume one-time id| Registry
  Device -->|opaque subscription id| Proxy
  Proxy --> ProxyState
Loading
  • APNs device tokens and Dashboard OAuth bearers never enter a customer installation.
  • The install signing private key never leaves notification-proxy.
  • Workshop core stores only a random proxy account id; the proxy stores only an opaque central subscription id.
  • The short-lived registration id cannot send a notification.

Delivery flow and boundaries

flowchart LR
  subgraph Install[One customer CFOS installation]
    Agent[Agent turn]
    User[User Durable Object]
    Browser[Visible browser subscriber]
    Proxy[notification-proxy]
    Key[Install signing private key]
  end

  subgraph Central[Cloudflare-operated notification service]
    Delivery[Typed delivery API]
    Registry[(Device and subscription directory)]
    Audit[(Dedupe, rate limit, audit state)]
  end

  subgraph Apple[Apple and phone boundary]
    APNS[APNs]
    App[Cloudflare OS app]
  end

  Agent -->|completed or needs permission| User
  User -->|visible client first| Browser
  Browser -->|presentation acknowledged| User
  User -->|fallback if no acknowledgement| Proxy
  Key -->|sign request locally| Proxy
  Proxy -->|typed title and same-origin path| Delivery
  Delivery -->|validate install and subscription| Registry
  Delivery --> Audit
  Delivery -->|fixed APNs template| APNS
  APNS --> App
Loading

Only the event id/type, stable task id, bounded chat title, opaque subscription id, and same-origin deep-link path cross the central boundary. Permission details, chat content, gatekeeper grants, and provider credentials do not.

Validation

  • pnpm configs:check
  • pnpm lint:check
  • notification proxy: 14 tests
  • browser bridge regression: 1 test
  • release manifest: 11 tests
  • backend notification boundary: 1 test
  • backend, frontend, and notification-proxy builds

The architecture and deployment contract are also captured in docs/notifications.md.

@github-actions github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel delivery Changes to CI or release delivery workshop/shared Changes to shared Workshop APIs labels Oct 2, 2026
Comment thread packages/workshop-backend/src/agent.ts
@ask-bonk

ask-bonk Bot commented Oct 2, 2026

Copy link
Copy Markdown

Review: 2 findings.

Posted two actionable inline comments in a single review.

github run

@deregtd
deregtd force-pushed the ddr/notification-system-worker branch from d07f8ce to a7bd1de Compare October 3, 2026 01:12
@ask-bonk

ask-bonk Bot commented Oct 3, 2026

Copy link
Copy Markdown

LGTM!

github run

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

Preview: pr650-ddr-notificat-a0d1b68a

https://pr650-ddr-notificat-a0d1b68a-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@ask-bonk

ask-bonk Bot commented Oct 3, 2026

Copy link
Copy Markdown

LGTM!

github run

@ask-bonk

ask-bonk Bot commented Oct 3, 2026

Copy link
Copy Markdown

LGTM!

github run

@github-actions github-actions Bot deleted a comment from ask-bonk Bot Oct 3, 2026
@github-actions github-actions Bot deleted a comment from ask-bonk Bot Oct 3, 2026
@ask-bonk

ask-bonk Bot commented Oct 5, 2026

Copy link
Copy Markdown

LGTM!

github run

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

Eval results

Verdict: ⚪ Unchanged. No task moved beyond what 10 runs can tell apart from noise.

Task Score Δ score Fisher test Cache hits Avg min Avg steps
change-calendar 100% 0 pp p = 1.00 86% → 88%
+2 pp
3.4 → 3.7 25.0 → 25.7
chess 90% → 80% −10 pp p = 1.00 97%
0 pp
9.2 → 8.7 61.9 → 54.4
incident-desk 100% → 90% −10 pp p = 1.00 95%
0 pp
4.3 → 4.2 38.3 → 36.1
worker-logs 100% → 70% −30 pp p = 0.21 92% → 93%
+1 pp
4.0 → 3.8 24.2 → 24.9
Failed checks
Task Check Failed
chess t1 agrees-with-the-oracle-on-the-hard-positions 1/10
chess t1 agrees-with-the-oracle-on-perft-positions 1/10
chess t1 starts-from-the-standard-position 0/10 → 1/10
chess t1 agrees-with-the-oracle-through-random-games 0/10 → 1/10
chess t1 the-game-is-shared-across-connections 0/10 → 1/10
chess t3 detects-insufficient-material-after-a-capture 0/9 → 1/9
chess t3 judges-material-and-terminal-positions-like-the-oracle 0/9 → 1/9
incident-desk t1 opens-acknowledges-and-resolves-in-order 0/10 → 1/10
incident-desk t1 simultaneous-acknowledges-yield-exactly-one-owner 0/10 → 1/10
incident-desk t1 simultaneous-opens-of-one-id-admit-exactly-one 0/10 → 1/10
worker-logs t1 ingests-resets-and-summarises-per-worker 0/10 → 3/10
worker-logs t1 hourly-buckets-cover-every-hour-including-empty-ones 0/10 → 3/10
worker-logs t1 ranges-are-half-open-and-filter-by-worker 0/10 → 3/10

Run · trajectories and raw results

@github-actions github-actions Bot deleted a comment from ask-bonk Bot Oct 5, 2026
@deregtd
deregtd marked this pull request as ready for review October 5, 2026 17:22
@ask-bonk

ask-bonk Bot commented Oct 5, 2026

Copy link
Copy Markdown

🔬 Eval runs review

Performance

Pass rates were Change Calendar 100% → 100%, Chess 90% → 80%, Incident Desk 100% → 90%, and Worker Logs 100% → 70%; none moved beyond noise with ten runs per task. Cache-hit and cache-break changes were not significant, and cost differences were smaller than within-task variation: Chess averaged approximately $0.0595 → $0.0535 and 61.9 → 54.4 steps, but remained the most expensive task on both sides. Worker Logs lost the most runs, with three distinct turn-1 implementation mistakes; these early failures also make its lower average time and cost misleading as efficiency gains.

⚪ VERDICT: NO REGRESSION FROM THIS PR

The comparison is unchanged, and the trajectories trace failures to generated gadget logic rather than the diff’s notification delivery and turn-disposition bookkeeping, which leave the relevant prompts, tools, and checks unchanged.

Triage

Failure modes

  • Oversized SQL inserts · worker-logs 1/10 · model error · this PR: no — Trial 2, turn 1: editFile replaced working per-row ingestion with 100-row inserts containing 600 parameters; verification hit too many SQL variables, leaving subsequent reports empty. Main’s passing trial 2 retained per-row inserts.
  • Rejecting valid timestamps · worker-logs 1/10 · model error · this PR: no — Trial 3, turn 1: writeFile compared millisecond-form timestamps against a normalization that removes .000, rejecting valid timestamps without fractions. Its executeCode smoke test used only toISOString() values and missed the defect.
  • Including the excluded end hour · worker-logs 1/10 · model error · this PR: no — Trial 4, turn 1: editFile recognized exact hour boundaries only when the time was 00:00:00, adding an extra bucket at ordinary hour endpoints. Summary values matched the reference; the extra buckets caused all three checks to fail.
  • Mixing square representations · chess 1/10 · model error · this PR: no — Trial 3, turn 1: writeFile generated algebraic move coordinates but passed them directly to applyMove, which indexes the board numerically. This produced the toUpperCase exception; the final reply claimed completion without executing a smoke test.
  • Missing knight-only draws · chess 1/10 · model error · this PR: no — Trial 5, turn 3: editFile implemented insufficient material only when there were no knights. Later executeCode tests covered bare kings and bishops, not king-and-knight versus king; the requested draw therefore remained undetected.
  • Reporting successful inserts as duplicates · incident-desk 1/10 · model error · this PR: no — Trial 3, turn 1: writeFile used inserted.rowsWritten === 1 to decide whether open succeeded. Checks found newly inserted incidents despite DUPLICATE_ID replies; no test incidents had been created by the agent. Main’s passing trial 3 checked existence and inserted within a transaction instead.

Tool errors

  • editFile: Multiple matches were found · change-calendar 0 → 3, chess 10 → 3, incident-desk 1 → 6, worker-logs 0 → 4 · model error — Agents supplied repeated fragments despite the unique-match requirement; candidate Chess trial 2 tried replacing this.exclusive( globally, then recovered with full-method matches.
  • editFile: Validation failed · change-calendar 5 → 4, chess 1 → 2, incident-desk 5 → 5, worker-logs 5 → 2 · model error — Calls omitted required fields or used incorrect argument names. Candidate Worker Logs trial 4 omitted filename, then supplied it on retry.
  • editFile: No matching text was found · chess 8 → 7, incident-desk 3 → 1, worker-logs 1 → 2 · model error — Replacement anchors differed from current file contents. Candidate Chess trial 6 repeatedly guessed PGN-parser text; main Chess trial 2 similarly used an incorrect CSS anchor before rereading.
  • readFile: File does not exist · change-calendar 0 → 4, incident-desk 2 → 0, worker-logs 2 → 2 · model error — Agents read server.js and client.js immediately after creating an empty gadget, despite the prompt explicitly saying new gadgets have no files.
  • executeCode: Failed to start Worker · chess 0 → 1 · model error — Candidate trial 6, turn 2 supplied code: "24" without a default export; the next call supplied a complete function and succeeded.

What to do

  • No notification change is indicated by these evals. Optional follow-up in packages/workshop-backend/src/agent.ts: add a short instruction to smoke-test requested RPCs before claiming completion, covering realistic batch sizes, timestamps with and without fractional seconds, non-midnight range boundaries, fresh insert outcomes, and representative chess positions.

github run

@ndisidore ndisidore left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good and going to be a massive value add 🙌

Design wise, a few comments:

  • Instead of the per-account Durable Object in the proxy, what if we had a stateless proxy with register(regId) → subscriptionId and deliver(subscriptionId, event), and the user Durable Object storing a set of subscription ids.
    • This would actually be a stronger security boundary: with the key in the proxy and subscription ids in the kernel, compromising the proxy alone doesn't give an attacker anyone to send to
    • We could kill NotificationAccountState class
    • multi-device would work for free
  • Do we want automated events also notifying the initiator? Currently if e.g. I have a schedule setup to run every hour, my phone will buzz every single hour
    • this is an easy fix if want: send send "completed" only when !callbackInitiated

Robo findings 🤖 (take with a grain of 🧂 )

The shared contract is about twice what it needs to be (kernel API, reviewed line by line).

  • workspaceTitle is computed in the overseer and carried through two shared types, but neither the proxy nor the frontend reads it.
  • completedAt, requestedAt and createdAt aren't read either. They mostly exist so the two types differ (Omit<…, "completedAt"> & { requestedAt }).
  • There are two publish methods, two deliver methods, two request builders and two wire types, differing only by a discriminator.
  • The deep-link path is built in both user.ts and delegate.ts, so the browser and push links can drift apart.

One { id, kind, workspaceId, chatId, chatTitle } type with one publishNotification and one deliver replaces all of that.

  1. Open tabs stop getting toasts after a user Durable Object reset (packages/workshop-backend/src/user.ts:369, medium). #notificationSubscribers is kept only in the object's memory, and NotificationBridge only re-subscribes on visibilitychange; nothing watches for a lost connection. So after a storage-timeout or code-update reset, a visible tab gets no more toasts and every notification goes to push instead. The smallest fix is probably for the bridge to re-subscribe when the subscription stub's onRpcBroken fires. I haven't confirmed that the stub actually breaks on a reset.
  2. as unknown as cast in the kernel (user.ts:429, low). AGENTS.md bans this in workshop-backend, and it does nothing here: env.d.ts:40 already types NOTIFICATION_DELIVERY as Service, and no generated config types it as Fetcher. The comment justifying it is also wrong. Read this.env.NOTIFICATION_DELIVERY directly and delete #notificationDelivery().
  3. The device-registration-id check is written three times (user.ts:353, account-state.ts:11, delegate.ts:140, low). The same 64-hex test appears in all three, and worker.ts also checks the UUID format of an id that only the backend creates. Only the proxy, which talks to the central service, needs the check. Removing the backend copy takes kernel lines out of the diff.
  4. "Open task" reloads the whole app (NotificationBridge.tsx:100, low). window.location.assign drops the WebSocket session and any unsent drafts. A TanStack Router navigate fixes it with the same amount of code.

Product decisions to make or document, not code to add

  1. Only the last-registered device gets push (notification-proxy/src/account-state.ts:20, medium). Each registration overwrites the single deliverySubscription key, so someone with an iPhone and an iPad only gets pushes on one of them. Because the bridge registers again on every page load, which device that is keeps changing. If one device per user is intended for v1, say so in the docs; otherwise this needs a keyed set.
  2. A toast fires for the chat the user is already watching (NotificationBridge.tsx:92, low). Every finished turn produces a toast in any visible tab, including the one showing that chat. Hiding the toast when the current route matches targetPath is a one-line change, but whether to do it is a UX call.

@kentonv

kentonv commented Oct 6, 2026

Copy link
Copy Markdown
Member

It seems like the notification-proxy exists purely to isolate the signing key? This doesn't seem worthwhile to me.

Workers already prohibits running dynamically-loaded code within your own isolate -- you can only run it in a separate dynamic isolate. Specifically, eval(), new Function(), etc. are prohibited. Given this, it's hard to imagine a vulnerability that would leak the signing key.

But even if you are still worried about that, there's a better way to prevent it: use a CryptoKey binding with extractable set to false. This gives the Worker an API to use the key to sign things, but not the ability to read the actual key material. So even arbitrary code couldn't leak it.

Sadly, CryptoKey bindings still aren't documented, and I'm not sure if wrangler supports them, despite the runtime and deploy API having supported them for a very long time... maybe we can get a bot to make the wrangler change.

@kentonv

kentonv commented Oct 6, 2026

Copy link
Copy Markdown
Member

Here's an old PR that implemented the last bit of CryptoKey bindings but was never merged. 😭 cloudflare/workers-sdk#3492

@kentonv

kentonv commented Oct 6, 2026

Copy link
Copy Markdown
Member

Random thougths:

  • Spawned agents should probably be treated differently. They probably shouldn't notify when done by default.
  • However, I do think spawned agents should have a way to call for help when they're unable to complete their task. Maybe an askForHelp tool would be a good idea in general, and would raise a more urgent notification?
  • We should provide an API for gadgets to issue notifications. This probably should be a binding? But I suppose since notifications aren't a gatekeeper it should be a built-in binding (like the agent spawner binding and AI model binding).
  • We've talked about the idea of feeding notifications into another gadget. Probably not for this PR but I wonder how that should eventually work? I can imagine wanting to handle notifications from different workspaces differently, maybe some workspaces still notify me directly while others are configured to feed into another gadget... is that just a workspace config setting to set notification destination, or something?

@deregtd

deregtd commented Oct 6, 2026

Copy link
Copy Markdown
Author

Nathan taking this over, closing in lieu of #676

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delivery Changes to CI or release delivery kernel Changes to the Workshop kernel workshop/frontend Changes to the Workshop frontend workshop/shared Changes to shared Workshop APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants