Repository navigation
Conversation
|
Review: 2 findings. Posted two actionable inline comments in a single review. |
d07f8ce to
a7bd1de
Compare
|
LGTM! |
Preview:
|
|
LGTM! |
|
LGTM! |
|
LGTM! |
Eval resultsVerdict: ⚪ Unchanged. No task moved beyond what 10 runs can tell apart from noise.
Failed checks
|
🔬 Eval runs reviewPerformancePass 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 PRThe 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. TriageFailure modes
Tool errors
What to do
|
There was a problem hiding this comment.
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) → subscriptionIdanddeliver(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
NotificationAccountStateclass - 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
- this is an easy fix if want: send send "completed" only when
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.
- 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.
- 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().
- 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.
- "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
- 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.
- 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.
|
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, But even if you are still worried about that, there's a better way to prevent it: use a CryptoKey binding with 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. |
|
Here's an old PR that implemented the last bit of CryptoKey bindings but was never merged. 😭 cloudflare/workers-sdk#3492 |
|
Random thougths:
|
|
Nathan taking this over, closing in lieu of #676 |
Summary
Make notifications a first-class Cloudflare OS platform feature instead of a gatekeeper.
notification-proxysystem Worker per CFOS installationsystemWorker kindProduction 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 --> ProxyStatenotification-proxy.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 --> AppOnly 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:checkpnpm lint:checkThe architecture and deployment contract are also captured in
docs/notifications.md.