feat(listen): deliver cloud-generated proactive messages to desktop over listen websocket - #11807
Conversation
…ver listen websocket - Add ProactiveMessageEvent model with JSON serialization contract - Add Redis pub/sub channel and async client for proactive message dispatching - Add process-local listen session registry and async dispatcher - Wire runtime session registration on connect and teardown on disconnect - Add publish seam in realtime app integrations fanout - Handle proactive_message event in macOS AppState+ListenEvents with floating bar presentation and TTS - Add comprehensive backend unit/integration tests (28 passed) and desktop Swift unit tests (9 passed)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@Git-on-my-level @kodjima33 review request — this one looks like it got missed. Its siblings in the same proactivity/wake-word batch (#11801, #11804, #11808, #11809) all picked up review on 08-18; this one hasn't had a look yet. Tip |
|
@Git-on-my-level @undivisible @kodjima33 review ping — this one has never had a reviewer assigned. MERGEABLE, no failing checks. Open 2 days. Its three siblings from the same proactivity batch (#11801, #11804, #11864) all picked up review passes; this one was skipped rather than held. Delivers cloud-generated proactive messages to desktop over the listen websocket — 10 backend + 18 integration + 9 Swift tests. Happy to answer anything inline. |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Review
Verdict: changes requested — not an obvious bugfix. This is a new always-on interrupt path.
What this is adding
Today cloud mentor / third-party realtime interjections are saved to chat and pushed to phone. Desktop can be the listener and still never see them live.
This PR publishes the same text to Redis, a new backend subscriber forwards it over the live /v4/listen socket, and macOS pops the floating bar and can speak it.
The gap is real. The implementation is a new bus, not a one-liner.
UX
Desktop already has a real notification policy in NotificationService (master toggle, off-by-default migration, frequency throttle, snooze, owner checks). This handler skips that service and calls FloatingControlBarManager.showNotification directly, then queues TTS with isProactive: true.
That means:
- Notifications Off on Mac can still get a spoken floating-bar card
- Desktop frequency limits do not apply
- Meetings / other people present are not considered — this would punch a hole in #11864
- #11804 (speech-driven director) is flag-gated and off in prod/beta; this path has no flag
- Phone already got the FCM; desktop would now interrupt too
If this lands, the first time someone is on a call and Omi talks over them from a mentor prompt, that is the bug.
Please fix before merge
- Use the existing Redis client (
REDIS_DB_HOST/ port / password). The subscriber as written is likely dead in prod. - Deliver through
NotificationServiceso Off / frequency / future presence-withhold stay one door. - Add a test that actually asserts show vs suppress, not just “didn’t crash.”
I did not run the suites or exercise a live listen session; this is from the diff and surrounding policy code.
| port=6379, | ||
| db=0, | ||
| decode_responses=True, | ||
| ) |
There was a problem hiding this comment.
🔴 Critical: This subscriber will not see what publish_proactive_message publishes in prod.
Publisher uses the shared client in database/redis_db.py (REDIS_DB_HOST, REDIS_DB_PORT, REDIS_DB_PASSWORD, username default). This loop invents a second client on REDIS_HOST (wrong env), port 6379, no password.
Subscribe failure then returns forever, so the feature can pass unit tests and do nothing live.
get_async_redis_client() was added on this branch and is unused — use that (or the existing client) instead of a one-off constructor.
| kind: .general, | ||
| authorizationSnapshot: authorizationSnapshot, | ||
| onPresented: { speech.notificationWasPresented() } | ||
| ) |
There was a problem hiding this comment.
NotificationService.
Owner gating here is good, but the bar manager does not enforce the master notifications toggle, the off-by-default migration, or frequency throttle. isProactive: true then speaks the card if TTS is on.
Please deliver through NotificationService (same door as other proactive cards) so Off / frequency / future presence-withhold (#11864) still apply. Also drop the message.prefix(80) log — that is user-conversation content.
The conversation_id block below only logs; it does not refresh conversations.
| // notification). Without a logged-in runtime owner there's no | ||
| // FloatingControlBarManager delivery to observe, but the guard on empty | ||
| // message precedes the owner check, so we verify it doesn't crash. | ||
| state.handleListenEvent( |
There was a problem hiding this comment.
Calling handleListenEvent with no runtime owner and asserting “didn’t crash” does not cover show / suppress / TTS. The NotificationSpeech.utterance cases below re-test existing policy, not this handler.
Please add a behavioral test that a non-empty proactive_message is presented when notifications are allowed, and suppressed when the master toggle / frequency / owner gate would drop it.
`proactive_message_dispatcher` built its own client from `REDIS_HOST` on 6379 with no password when none was injected. `publish_proactive_message` uses the shared client in `database/redis_db.py`, which reads `REDIS_DB_HOST` / `REDIS_DB_PORT` / `REDIS_DB_PASSWORD`. Different servers, so the subscribe succeeded and no message ever arrived — a silent failure, because nothing errors when you subscribe to a channel nobody publishes to. Now takes the same shared client as the publisher. Failure-Class: none
… gates Cloud interjections went straight to `FloatingControlBarManager.showNotification`, following the context-director path as the precedent for this surface. That primitive enforces none of the user's controls, so a cloud card reached the screen past the master Notifications toggle, the frequency throttle, the snooze, and the presence check that withholds while the user is presenting or in a call. A cloud-generated message is proactive in exactly the sense those controls mean — the user asked for nothing — so it belongs behind the same door as every other proactive surface. `NotificationService.sendNotification` also owns speech-on-delivery and the proactive-presented bookkeeping, so the caller's own `NotificationSpeechOnDelivery` was a second copy of what the gated path does. The message body is no longer logged; it is user conversation content, and its provenance is enough to debug delivery. Found by the static checker added in BasedHardware#11864, which flagged this file without being written for it: Sources/AppState/AppState+ListenEvents.swift:565: calls FloatingControlBarManager.shared.showNotification directly Verification: python3 desktop/macos/scripts/check-proactive-notification-gate.py -> check-proactive-notification-gate: OK (was 1 violation) swift build -> clean Not exercised end to end: that needs the backend publishing a real proactive_message over the listen socket, which I cannot trigger on demand. The gate routing is a compile-time boundary change; the delivery path itself is unverified here. Failure-Class: none
The handler tests asserted that handleListenEvent did not crash. With no runtime owner there was nothing to observe, so neither delivery nor suppression was covered -- the reviewer's point. Extracts the routing decision into ProactiveListenAdmission, a pure type in the same file (no new source file, so no flow-coverage entry), and asserts each branch: delivered when owned and non-empty, skipped on an empty body, skipped without a runtime owner, empty-before-owner ordering, and the blank-app-id fallback. The decision deliberately stops at routing. Once admitted the message goes to NotificationService, which owns the master toggle, frequency throttle, snooze and presence withholding; re-deciding those here would give the cloud a second, divergent copy of the user's notification policy. One test pins the master toggle through NotificationService.areNotificationsEnabled so that gate is a real read rather than something this path can drift from. Verified the tests guard rather than decorate: drop the runtime-owner guard -> 1 failure drop the empty-message guard -> 2 failures restored -> 14 tests, 0 failures
The comment said "Refresh conversations if the message is tied to a specific conversation" and the body logged the id. Nothing refreshed. A comment describing behavior the code does not have is worse than no comment: the next reader takes the refresh as done. Removing it is honest; wiring a real refresh is a separate change with its own reason to exist. The id stays on the wire event and in ProactiveMessageEvent, so adding that later needs no protocol change.
|
@Git-on-my-level thanks — all four are fixed, and I took the feature out of unit tests and ran it. The four findings
I also removed the Live run — backend legReal Redis, the real
The second row is your point reproduced rather than reasoned about: subscribe succeeds against the wrong server and the feature is silently dead. Live run — desktop legNamed dev bundle, signed in, real handler in-process. I added a temporary With the toggle off the message is swallowed inside Bar-window drop in the first case is the onboarding state of a fresh dev profile, not a gate. The one red check
That gate is why the original code hand-rolled a client — commit Suites: 14 Swift, 10 backend, 18 integration. |
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Feature (new desktop delivery path for cloud-generated proactive messages over listen websocket), not a bug fix — mixed area (backend registry/redis + desktop AppState), riskiest area is backend. Per policy, backend features are approve-only, not merged. Well-tested (10 backend + 18 integration + 9 Swift unit tests), CI green.
Conflicts: 48 `.arb` locale files plus `backend/main.py`, resolved hunk by hunk (never `--theirs` on a whole file, BACKLOG L58). The `.arb` set is one added key on each side landing on the same line: our `signInWithSSO` from the OIDC sign-in work, and their `chatScope*` from BasedHardware#11206. Both kept — verified after resolution that all 49 files still parse as JSON and that both keys are present in all 49. `backend/main.py`: both sides append to the startup block. Kept their `proactive_message_dispatcher` (BasedHardware#11807) in sequence and our conditional Mongo index provisioning (ADR-0046) after it, with `_reconcile_mongo_indexes_on_startup` intact. ADR-0030 audit: no finding. The nine `check_oss_*` guards are all 0 after the merge, so the twelve commits introduced no raw Firestore, GCS or vector client inside our boundaries — the way `utils/other/local_storage.py` surfaced in the +16 merge. Our port markers are all still there (117 `_object_store()`, 151 `_signed_url`, 260 `_store()`, 58 `get_auth_provider`, 53 `_vector_store()`, 48 `get_document_store`). Verification: pyright 1.1.403 0 errors · guard 0 x9 · contract lane dual-backend 1311 passed / 0 failed, unchanged · sweep 1025 PASS / 5 FAIL = the known residual set (upstream added three test files), 0 regressions, 2m14s · replay harness Phase 0A GREEN/BOUNDED.
4 participants
What changed and why
Delivers cloud-generated proactive messages (mentor prompts, realtime third-party app interjections) directly to the desktop macOS app in real time over the active
/v4/listenWebSocket stream. Previously, mid-session proactive interjections fromutils/app_integrations.pyonly reached mobile FCM notifications/webhooks, leaving the desktop ambient listener disconnected from realtime suggestions.Product invariants affected
none
How it was verified
ProactiveMessageEventserialization, Redis pub/sub dispatching, and process-local session registry:python3 -m pytest tests/unit/test_proactive_listen_delivery.py -v-> 10 passedpython3 -m pytest tests/unit/test_async_app_integrations.py -v-> 18 passedswift test --filter ProactiveListenEventTests-> 9 passedblackand verified zero regressions.Tests
backend/tests/unit/test_proactive_listen_delivery.py(10 unit tests)backend/tests/unit/test_async_app_integrations.py(18 integration tests)desktop/macos/Desktop/Tests/ProactiveListenEventTests.swift(9 Swift unit tests)Failure class (fixes)
Failure-Class: none