feat(desktop): withhold proactive notifications while other people are present - #11864
feat(desktop): withhold proactive notifications while other people are present#11864aryanorastar wants to merge 17 commits into
Conversation
`SuggestionPacing.dedupMemory` chooses a suggestion's dedup depth from its
category, and that category is chosen by the model -- it is a decoded field on
`ExtractedSuggestion`, not something the client derives. At Maximum frequency the
depth is 0 for every category except `commitment`, so a mislabelled suggestion
silently loses dedup entirely and repeats forever.
Neither the delivery nor the duplicate log line carried the label, so a user
reporting "it keeps telling me the same thing" could not be answered without
guessing. Investigating exactly that report, the label turned out to be
`commitment` -- which carries the full depth of 10, disproving the mislabel
theory and pointing instead at the similarity threshold in
`SuggestionDeduplication.isDuplicate`:
delivering [90%] [commitment] "Submit prototype for SBI Hackathon @ GFF 2026."
duplicate [commitment] "Submit prototype for SBI Hackathon @ GFF 2026"
delivering [95%] [commitment] "The Omi Proactivity app is fine, but submit
prototype for SBI Hackathon @ GFF 2026."
The same commitment passes as novel once the model prepends a context-aware
clause, because the added words drop the word overlap below the threshold. The
context-awareness that makes a suggestion feel present is what defeats the repeat
guard. That defect is not addressed here; this change is what made it visible.
Verification
- swift build -> clean
- Live session: category now present on every delivery and duplicate line.
Honest gaps
- Diagnostics only, no behaviour change.
- The `below bar` line is left alone; it already carries the confidence numbers
that explain it.
…e present A proactive notification is addressed to one person. Omi delivered them identically whether the user was alone, in a call, or presenting to a room. A live session produced "Submit prototype for SBI Hackathon before the deadline" on screen -- useful alone, a disclosure on a shared screen and an interruption mid-meeting. Two distinct harms, one rule: - sharing a screen makes a private nudge **visible** to everyone on the call - being in a call at all makes it **interrupt a conversation** Scoping this to screen share alone was tested against a live Google Meet call and let "Meet is fine — but you said you'd submit the SBI Hackathon prototype" through while the user was mid-meeting, so detection covers both. ## Where the guard sits `NotificationService.sendNotification` is the single choke point every proactive surface already routes through (suggestion, memory, insight, goals, meeting action items, plugin -- 9 call sites), so one guard covers all of them rather than a per-assistant exception. `respectFrequency` is the existing proactive/functional split and is honoured: functional notices (screen-recording repair prompt, Crisp replies, onboarding test) pass `false` and still reach the user. Suppressing the capture-repair prompt during a share is precisely how a broken capture would stay broken. Detection reuses three signals that already exist and are already trusted: `activeScreenSharePresent()` (used to pause capture during shares, BasedHardware#10143), `callAppIsUsingMicrophone()`, and `browserCallWindowPresent()` -- the documented fallback for a *muted* browser call, which is what caught the Meet case above. It is placed after the cheap boolean gates so the window scans never run for a notification an earlier gate already refused. ## Withheld, not destroyed `SuggestionAssistant` writes `recentSuggestions` immediately *before* delivering, and that window gates every later evaluation. Withholding only at the choke point would have recorded the suggestion as delivered and retired it permanently: the user never sees the card, and every regeneration after the call is filtered as a duplicate of something never shown. The assistant therefore consults the same shared policy *before* the dedup write and returns early, leaving the suggestion eligible once the call ends. `suppressed_presenting` is a distinct delivery outcome from the `filtered_*` ones: those retire a suggestion on its merits, this defers a good one on audience. ## Verification - `swift test --filter PresenceAwareNotificationSuppressionTests` -> 10 passed, including the two that pin the deferral guarantee (remembered => duplicate forever; unremembered => still deliverable). - Live end-to-end on the dev serving plane, one session, no screen share -- joined a Google Meet call in a browser: 17:23:35 delivering (before the call) 17:24:22 delivering 17:25:26 withheld while others are present [commitment] (call starts) 17:25:47 withheld while others are present 17:26:07 withheld while others are present 17:26:27 withheld while others are present 17:27:17 delivering "Google Meet is fine, but..." (call ends) The 17:27:17 delivery is the deferral proof: had the withheld suggestions been remembered, it would have been filtered as a duplicate. ## Honest gaps - `callAppIsUsingMicrophone()` requires macOS 14.4+. On 14.0-14.3 a native-app call (Zoom desktop, FaceTime) is not detected and only browser calls are, via window title. This fails open -- a missed suppression, never a missed notification. - Detection costs up to two `CGWindowList` scans plus an audio-process enumeration per proactive notification that reaches this gate. - Withheld suggestions are not queued for replay; they are re-evaluated naturally when context recurs. - Screen-share detection relies on window titles and covers Zoom, Teams and browser-based sharing; other conferencing apps are not recognised. - No user-facing control yet to snooze or opt out of this behaviour.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Presence-aware suppression handles the case Omi can detect. It does not help a user
who simply does not want to be nudged right now -- the only controls were the
master toggle (off forever) and the frequency slider (permanently quieter).
Adds a bounded silence: 1, 4 or 8 hours, from Settings > Notifications & Privacy,
beside the frequency slider because it answers the same question -- how often may
Omi interrupt me -- for a window rather than forever. The row shows live state
("Silenced until 12:35 AM") and offers "Resume now" while active.
## Not the same as hiding the bar
`floatingBar_snoozedUntil` already exists and deliberately does *not* do this.
`NotificationService` documents why: "Hiding the floating bar ('Hide for 2 hours')
and disabling it are both statements about the BAR, not about notifications: an
hour of a movie with the bar hidden or off must still nudge."
This is a separate key, `notifications_snoozedUntil`, making the statement that one
is documented not to make. A test asserts the two keys stay distinct so a later
change cannot quietly merge them.
## Withheld, not destroyed
`SuggestionAssistant` consults the snooze *before* writing `recentSuggestions`, the
same ordering the presence guard uses and for the same reason: the dedup window
gates every later evaluation, so recording a suggestion that will never be shown
retires it permanently. Silencing for 8 hours must not annihilate every suggestion
generated in that window. `suppressed_snoozed` is a distinct delivery outcome from
both `filtered_*` (retired on merit) and `suppressed_presenting` (deferred on
audience).
Functional notices are unaffected: `respectFrequency: false` still passes, so a
screen-recording repair prompt reaches a user who silenced suggestions -- otherwise
the snooze swallows the message explaining why capture broke.
## Verification
- swift test --filter 'NotificationSnoozeTests|PresenceAwareNotificationSuppressionTests'
-> 19 passed (9 snooze + 10 presence)
- Live on the dev serving plane:
23:35:51 NotificationService: proactive notifications silenced for 60m
23:39:53 Suggestion: withheld while notifications are silenced [commitment]
— "You said you'd submit the SBI Hackathon prototype, but it's overdue."
and `probe_suggestion_nudge` returned outcome `suppressed_snoozed`. The
suggestion was generated and then withheld, which is the ordering that matters:
it had already passed the duplicate filter on merit.
- Settings row verified on screen showing live state.
## Honest gaps
- Placement went to Settings after the floating-bar context menu proved wrong in
practice: in notch-island mode the bar is covered by the very notification the
user wants to silence, so the control was unreachable exactly when needed. The
bar menu still carries the same actions as a shortcut.
- Durations are fixed at 1/4/8 hours. No "until tomorrow", no custom value.
- The snooze is global, not per-assistant; silencing quiets suggestions, insights,
memories and task nudges together.
- No notification when a snooze lapses; suggestions simply resume.
- Withheld suggestions are not queued for replay, only left eligible.
|
Added the user control this PR listed as an honest gap ("No user-facing control yet to snooze or opt out"). Presence-aware suppression handles what Omi can detect; it does nothing for a user who simply does not want to be nudged right now. Before this, the only options were the master toggle (off forever) or the frequency slider (permanently quieter). Settings → Notifications & Privacy → Silence Notifications — 1, 4 or 8 hours, with live state and a "Resume now" action while active. Not the same as hiding the bar
This is a separate key, Withheld, not destroyedSame ordering as the presence guard, for the same reason:
Functional notices are unaffected: Verification
Placement, and why it movedI first put this in the floating bar's context menu, next to "Hide for 2 hours". That was wrong in practice: in notch-island mode the bar is covered by the very notification the user wants to silence, so the control was unreachable at exactly the moment it is wanted. Settings is always reachable. The bar menu still carries the same actions as a shortcut. Honest gaps
|
|
Screenshot of the control, since my previous comment referenced it but the marker was an HTML comment and rendered invisible. Settings → Notifications & Privacy, showing the row in its active state:
It sits directly beneath the Frequency slider on purpose — that slider answers "how often may Omi interrupt me" permanently, and this answers the same question for a bounded window. The subtitle carries live state ( |
|
@kodjima33 @Git-on-my-level review request — no reviewer has looked at this one yet. Withholds proactive notifications while others are present (screen share or a call, including a muted browser call — the case that disproved my initial screen-share-only scope on a live Meet call). Routes through the single Also fixed the missing |
Two follow-ups to the presence and snooze gates in this PR, both from reviewing
the honest-gaps list rather than from new reports.
## The director lane bypassed both gates
`presentContextDirectorNotification` delivers straight through
`FloatingControlBarManager.showNotification` and never reaches `sendNotification`,
so both gates below it were skipped:
let speech = NotificationSpeechOnDelivery(message: message, isProactive: true)
...
return FloatingControlBarManager.shared.showNotification(...)
A user who silenced notifications for four hours, or who was mid-call, still
received director cards. That path sets `isProactive: true` a few lines later, so
it is exactly the class both gates exist to withhold — the omission was reach, not
intent. Both now run at that entry point, reusing the same pure policies rather
than restating them. `respectFrequency: true` is hard-coded because every caller
of this entry point is proactive; functional notices go through `sendNotification`
with `respectFrequency: false` and are unaffected.
## "Until tomorrow"
The offered durations were fixed offsets, and the one people want at night was
missing. "Until tomorrow" is a wall-clock boundary rather than an offset, so it is
a separate entry point: it resolves to the next 9am, not midnight — silencing at
11:30pm and resuming half an hour later is not what the phrase means to anyone.
Silencing at 2am resolves to 9am the same morning rather than waiting 31 hours,
which is the boundary worth pinning. The Settings subtitle now says "tomorrow"
when the expiry is not today, because "Silenced until 9:00 AM" otherwise reads
identically for both.
Verification
- swift build -> clean
- swift test --filter 'NotificationSnoozeTests|PresenceAwareNotificationSuppressionTests'
-> 21 passed, including boundary cases at 11:30pm, 2:15am and exactly 9:00am,
plus a loop asserting every hour of the day yields a future expiry
- Confirmed live: "Until tomorrow" shows "Silenced until 9:00 AM tomorrow" and
withholds; silencing now withholds director cards as well as suggestion cards.
Honest gaps
- No unit test drives `presentContextDirectorNotification` end to end; it is
@mainactor and constructs real presentation state. The gate decision is covered
by the shared policy tests and the wiring was verified live.
- The 9am resume hour is fixed, not user-configurable.
- `ContextDeliveryAuthority.freeGate` still evaluates master/frequency/paywall
separately from these two gates rather than in one place.
|
Worked through the honest-gaps list. Two are closed, three I am deliberately not doing, and one of those was mis-listed by me as a gap when it is the correct design — correcting that here rather than leaving it standing. ClosedThe context director lane bypassed both gatesThis is the one that mattered. let speech = NotificationSpeechOnDelivery(message: message, isProactive: true)
...
return FloatingControlBarManager.shared.showNotification(...)A user who silenced notifications for four hours, or who was mid-call, still received director cards. The path sets
"Until tomorrow"Added as a wall-clock boundary rather than an offset, so it is a separate entry point. It resolves to the next 9am, not midnight — silencing at 11:30pm and resuming half an hour later is not what the phrase means to anyone. Silencing at 2am resolves to 9am the same morning rather than waiting 31 hours, which is the boundary worth pinning. The Settings subtitle now says "tomorrow" when the expiry is not today, since "Silenced until 9:00 AM" otherwise reads identically for both. Not doing, with reasonsWindow-scan cost — I over-flagged this. Measured on a live session: 194 gate evaluations in 24 minutes, and nearly all are cooldown/dwell skips that never reach the presence check. The scan runs once or twice a minute at a few milliseconds. A TTL cache would add mutable static state and let a notification slip through for up to 2s after a share starts — a worse trade than the microseconds saved. Leaving it. Per-assistant snooze. Nobody has asked to silence insights while keeping task nudges. That is four times the state and UI surface on a guess, and it is easy to add later if a real request arrives. Notification when a snooze lapses. A notification announcing that notifications are back is itself an interruption, arriving at a moment the user did not choose. The subtitle already shows the expiry for anyone who wants to check. Replay queue for withheld suggestions — I mis-listed this. A suggestion withheld four hours ago is stale; replaying it would nudge the user about a screen they left. "Left eligible, re-evaluated when context recurs" is the correct behaviour, not a shortfall. Treat that line in the earlier comment as withdrawn. macOS 14.4 call detection. API availability; cannot be closed. It already fails open — a missed suppression, never a missed notification. Verification
Remaining honest gaps
|
Second lane found bypassing the user's notification controls, and the reason this
PR now carries a guard rather than a third copy of the fix.
`NotchMomentsCoordinator` posts the Second Brain "moments" -- live receipts as Omi
writes things down, and the conversation-end follow-ups card -- off transcription
and task state. No user request sits behind them, so they are proactive by any
reading. It called the presentation primitive directly:
_ = FloatingControlBarManager.shared.showNotification(...)
That skips every gate in `sendNotification`: the master Notifications toggle
(BasedHardware#6778), the frequency throttle, the snooze, and the presence check. A user who
silenced notifications for four hours, or who was presenting, still received
"Omi wrote this down" receipts. The coordinator's own doc comment claimed it was
"routed through the existing hardened notification path", which is what made the
gap invisible in review.
Routed through `NotificationService.sendNotification` instead of adding another
call-site guard: the boundary is the problem, not the call site.
## Guard
Two lanes shared this cause -- the context director earlier in this PR, and this
one -- so a reusable guard lands with the fix rather than a third patch later.
`scripts/check-proactive-notification-gate.py` is a **static checker**, labelled as
such: it fails the build when a file outside a documented allowlist names
`FloatingControlBarManager.shared.showNotification`. The hazard is invisible at
runtime and in review -- a new lane still shows a card, still demos correctly, and
simply ignores every control the user has.
Allowlist entries each justify why they are NOT proactive: the gated service
itself, the manager's own implementation, trial/billing banners, and onboarding
permission help. A functional notice that must reach a silenced user is added
there with its reason; the rule itself does not relax.
Comments and string literals are masked before matching, so prose about the rule
does not trip it.
Wired into `.github/checks-manifest.yaml` in both `local` and `ci` lanes, with its
own fixture tests -- a checker that has never failed is not a guard, so the
fixtures are the two lanes that actually bypassed the gates.
Verification
- python3 desktop/macos/scripts/check-proactive-notification-gate.py -> OK
- python3 desktop/macos/tests/test_check_proactive_notification_gate.py -> 7 passed
- python3 .github/scripts/pr_preflight.py --lane local -> manifest contract PASS,
new check selected
- swift build -> clean
Honest gaps
- The checker matches `FloatingControlBarManager.shared.showNotification` by name.
A future alias or a stored reference to the manager would evade it; it is a
cheap tripwire, not type-level enforcement.
- `NotchMomentsCoordinator` now inherits the frequency throttle and master toggle
as well as the two new gates. That is the correct contract for a proactive card
but is a behaviour change beyond the snooze/presence scope, and worth calling
out: these receipts were previously unthrottled.
…y set
`testDeliveryOutcomesAreClosedAndJoinWithoutCardContent` pins the exact
`DeliveryOutcome` vocabulary so no outcome can be added that carries card content
into telemetry. This PR adds two, so the guard fired -- which is the guard working,
not a test to loosen.
Extended by exactly the two values this PR introduces. A sixth outcome added
without touching this line still fails, so the closed-set property is intact; the
edit is the review the guard exists to force.
`suppressed_presenting` and `suppressed_snoozed` are deferrals rather than filters:
the suggestion was deliverable and was withheld on audience or on the user's
explicit silence, and stays eligible afterwards. Both carry only the existing
opaque UUID correlators, so the privacy property the test also asserts is unchanged.
Verification
- Full desktop suite before this commit: 5479 tests, 1 skipped, 2 failures
- Full desktop suite after: 5479 tests, 1 skipped, 1 failure
The remaining failure is `RewindCaptureExclusionGenerationTests`
`testOwnerSnapshotStaysCurrentWhenAuthLeadsUnresolvedRewindDatabase`. It is
pre-existing and not caused by this PR:
* it passes when run in isolation (`--filter`), failing only in a full run,
which points at cross-suite ordering rather than the assertion itself
* a full run with this PR's `NotchMomentsCoordinator` change reverted -- the only
change here that touches `RuntimeOwnerIdentity` snapshots, and therefore the
only plausible link to an owner-snapshot test -- still fails identically:
5479 tests, 1 skipped, 1 failure, same test
Honest gaps
- I have not diagnosed the Rewind failure, only established it is not ours. It
looks like the hand-listed test-isolation problem the repo already tracks, and
it deserves its own issue rather than a drive-by fix in a notifications PR.
|
Ran the full desktop suite rather than the focused filters, which is what turned up the rest of this. Also worked the remaining honest-gaps list — one of them uncovered a second bug. A second lane was bypassing the gates
_ = FloatingControlBarManager.shared.showNotification(...)That skips every gate in Its own doc comment claims it is "routed through the existing hardened notification path". It wasn't — and that sentence is exactly why the gap survived review. Routed through Guard, because two lanes shared the causeThe context director (earlier in this PR) and this one are the same defect twice, so a reusable guard lands with the fix instead of a third patch later.
Allowlist entries each justify why they are not proactive: the gated service itself, the manager's own implementation, trial/billing banners, and onboarding permission help. A functional notice that must reach a silenced user gets added there with its reason; the rule does not relax. Wired into Full suiteOne failure was mine. The remaining failure is pre-existing.
I have not diagnosed it, only established it is not ours. It resembles the hand-listed test-isolation problem the repo already tracks, and deserves its own issue rather than a drive-by fix in a notifications PR. Gaps I decided not to close, with reasons
Remaining honest gaps
|
Git-on-my-level
added
security-review
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the detailed privacy-focused work here. The direction looks useful: proactive suggestions should not leak onto a shared screen or interrupt an active call, and the snooze control is a reasonable user-facing escape hatch.
I’m requesting changes because the new static gate is currently catching an actual remaining bypass in this branch. Running python3 desktop/macos/scripts/check-proactive-notification-gate.py on the PR tree reports:
Sources/FloatingControlBar/NotchMomentsCoordinator.swift:183: calls FloatingControlBarManager.shared.showNotification directly
Specific review notes:
desktop/macos/scripts/check-proactive-notification-gate.pycorrectly encodes the intended invariant that proactive deliveries should route throughNotificationService, and its unit test file passes locally, but the checker fails against the real source tree becauseNotchMomentsCoordinator.poststill calls the floating-bar primitive directly..github/checks-manifest.yamlwires that checker into the desktop contract lane, so this is not just advisory; the PR leaves the new CI check red until the remaining direct caller is routed/gated or explicitly justified as functional.desktop/macos/Desktop/Sources/FloatingControlBar/NotchMomentsCoordinator.swiftposts the “wrote this down” receipt throughFloatingControlBarManager.shared.showNotificationat line 183. That path still bypasses the snooze/presence checks added inNotificationService, which is exactly the privacy/workflow class this PR is trying to close.desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swiftadds the snooze and presence gates insendNotificationandpresentContextDirectorNotification; that part is the right choke-point shape, but it only protects callers that actually enter this service path.desktop/macos/Desktop/Sources/ProactiveAssistants/Assistants/Suggestions/SuggestionAssistant.swiftchecks snooze/presence before writing torecentSuggestions, which preserves deferred suggestions instead of turning a suppressed card into a future duplicate. The new telemetry outcomes inSuggestionAssistantTelemetry.swiftandInsightAssistantTelemetry.swiftmake those deferrals observable.desktop/macos/Desktop/Sources/MainWindow/Pages/Settings/Sections/SettingsContentView+NotificationsPrivacy.swift,SettingsPage.swift, andFloatingControlBarView.swiftadd the Settings and context-menu controls for snoozing/resuming; the UI state update is local and understandable.- The new Swift tests (
NotificationSnoozeTests.swift,PresenceAwareNotificationSuppressionTests.swift, and the telemetry test update) cover the pure policy pieces and deferral semantics, while the two changelog files describe the user-facing behavior.
Please either route the NotchMomentsCoordinator receipt through NotificationService so it gets the master/frequency/snooze/presence gates, or explicitly classify it as a functional notification with a narrow allowlist rationale if maintainers want it to bypass user silence. Because this is privacy-sensitive desktop notification behavior plus a new CI gate, final product/UX sign-off should stay with a human maintainer once the failing check is green.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
`960bd33d84` silently undid `5e63c3e49e`. The telemetry commit's message describes only the closed-set update, but its diff also reverted `NotchMomentsCoordinator.post` from `NotificationService.sendNotification` back to `FloatingControlBarManager.shared.showNotification`. Cause: during an A/B run to establish whether a pre-existing test failure was ours, I reverted that file with `git checkout <commit> -- <file>`, which *stages* the revert. The working tree was restored afterwards with `cp`, but the staged revert was never cleared. `git status` showed `MM` — staged and unstaged — and I did not read it. The next `git commit` swept the staged revert in under an unrelated message. Caught by this PR's own checker running against the branch, which is the outcome it was written for: a proactive lane silently losing its gates, invisible in the diff being reviewed and invisible at runtime. No behaviour change relative to the intent of `5e63c3e49e`; this restores that commit's version of the file byte for byte. Verification - git show 5e63c3e:...NotchMomentsCoordinator.swift restored verbatim - python3 desktop/macos/scripts/check-proactive-notification-gate.py -> OK - python3 desktop/macos/tests/test_check_proactive_notification_gate.py -> 7 passed - swift build -> clean - `git diff --cached` inspected before committing this time
|
You are right, and the checker was right. Fixed in What happened. Cause. While measuring an A/B baseline for the gate-evaluation cost I ran So the fix was never re-argued or intentionally reverted — it was overwritten by my own measurement tooling, which is worse in one specific way: nothing in the diff review of Fix. Restored byte-identical to On the other two red checks. The uncomfortable part worth stating plainly: the guard this PR adds is what caught the guard being removed, one commit after it was written. That is the argument for the checker existing, made at my own expense. |
Statistics — measured, not estimatedNumbers. Source is 78 real sessions, 32.6 h of logged runtime, Aug 18–20, on the dev serving plane with a real Firebase identity, real screen content and real Google Meet calls. No fixtures, no replay, no synthetic input. What it is not: one user, one machine, three days, and not a controlled A/B — the app changed under me while I used it. The caveats below matter more than the totals. Presence — true vs false positivesThe question is whether "someone else is present" fires when it should and only when it should. I cross-checked every suppression against
Three of the nine fired while the foreground app was LinkedIn or Settings, not the Meet window — "LinkedIn is fine, but you said you'd submit the SBI Hackathon prototype." Those read as false positives at a glance and are not: being in a call while looking at another app is the ordinary case, and it is precisely what my original screen-share-only scope would have missed. Misses, the other direction:
The last two are not clean passes and I am not scoring them as such. The 2 baseline leaks are the reason the feature exists — Aug 19 17:15 and 17:16, both mid-call, both "SBI Hackathon prototype is overdue" on screen during a meeting. Triggers, before vs afterBefore is 0. Proactive notifications had no presence check and no snooze; there was nothing to compare against. After: 9 withheld for presence, 130 withheld for snooze, across 40 deliveries. CostCooldown and dwell run before the model and cost nothing. Everything past them is a paid evaluation — one screenshot plus grounding on gemini-2.5-flash-lite, ≈ $0.0004 each.
$0.19 over 32.6 h ≈ $0.006 per hour of active use, about $0.05 a day at 8 hours. Negligible in absolute terms. The ratio is not: 90% of paid evaluations never reached the user. Three findings, in order of size. Two of them are uncomfortable and I would rather state them than have them found. 1. Deduplication is the single largest cost — 43%. 205 paid evaluations produced a suggestion the model had effectively already made. 2. The gates in this PR save no model cost — by design, and the design is worth questioning. Presence and snooze are delivery gates: they run after the model has already produced a suggestion. 139 paid evaluations were spent on cards the user was never going to see. Silence notifications for 8 hours and you pay full price for those 8 hours. Moving the snooze check ahead of evaluation would recover ~29% of spend. I did not do it here because withholding before evaluation changes which suggestions exist rather than which are shown — the deferral guarantee this PR is built on depends on the suggestion being generated and surviving dedup, and I did not want to trade that for cost inside a PR scoped to delivery. It is a real follow-up with a real number attached, not a hypothetical. 3. 18% of evaluations failed on Method and limits
|
The static gate this PR adds caught a third bypass lane, this one landed on main after the branch forked: IntegrationNudgeCoordinator presents its "Connect <app>" card straight through FloatingControlBarManager, so the master toggle, frequency throttle, snooze and presence gates never see it. An integration pitch is a suggestion, not a functional notice — a user who silenced suggestions, or who is on a call with a shared screen, is exactly who should not be offered one. Allowlisting it would have exempted the precise class of lane the checker exists to catch. sendNotification was the wrong door: it returns Void and keeps the presentation result, which the coordinator needs — it spends one of an integration's three lifetime offers from onPresented rather than from the call returning, so a queued-then-dropped card does not burn an offer, and it reads the result to tell "bar refused, do not retry" from "queued". Threading that through sendNotification would mean classifying all twelve of its exit paths into the result enum, on privacy-sensitive code. presentActionableProactiveNotification is instead a sibling of presentContextDirectorNotification: same gate order, same result type, same callback contract, differing only in carrying a FloatingBarNotificationAction and throttling against the caller's own assistantId rather than the context-director budget. Suppression composes with the budget for free — onPresented never fires, so a withheld offer stays unspent and the nudge is free to be made again once the user is no longer silenced or in company. Verification: - swift build -> clean under swift-version 6, -strict-concurrency=complete, -warnings-as-errors - swift test --filter IntegrationNudgeCoordinatorTests -> 18 passed - swift test --filter PresenceAwareNotificationSuppressionTests -> 10 passed - check-proactive-notification-gate.py -> OK (was the failing check) - pytest desktop/macos/tests/test_check_proactive_notification_gate.py -> 7 passed - new test proven live: a stub calling onPresented on a .suppressed card fails it (shownCount 1 vs 0); restoring the stub passes Failure-Class: none
SwiftLint rejects four force operations in this file with force_try and force_unwrapping, all serious. The file is not in the SwiftLint baseline, so the violations block the desktop-swiftlint gate rather than being grandfathered. Both tests reached for `try!`/`!` only because they were not declared `throws`. Marking them `throws` lets the same assertions run through XCTUnwrap, which reports the nil rather than trapping the whole suite. Verification: - swift test --filter NotificationSnoozeTests -> 11 passed - swift-format with the pinned 602.0.0 wrapper Failure-Class: none
|
@Git-on-my-level requested changes addressed on tip Your ask — A third lane appeared from main, not from this branch. I routed it rather than allowlisting it. An integration pitch is a suggestion, not a functional notice: a user who silenced suggestions, or who is on a call with a shared screen, is precisely who should not be offered one. Allowlisting would have exempted the very class of lane the checker exists to catch. Why not
Verification
Honest scope note: that new test pins that Separate commit Product/UX sign-off on the hard-scope behaviour is unchanged and still yours. |
undivisible
left a comment
undivisible
left a comment
There was a problem hiding this comment.
Review
The current tip passes python3 desktop/macos/scripts/check-proactive-notification-gate.py and its 7 tests; the only production direct primitive callers are the documented functional allowlist plus NotificationService, and Notch/IntegrationNudge now route through the service.
Residual risks before product sign-off:
- macOS 14.0–14.3 still misses native-app calls because the microphone-process signal is only available from 14.4; this is fail-open.
ContextDeliveryAuthorityremains a separate proactive lane; the new checker does not prove its policy is equivalent.- Presence/snooze are evaluated before the eventual presentation boundary. The async system-banner branch rechecks owner/context eligibility but not presence/snooze, and each presence check can cost window scans plus audio-process enumeration.
This is privacy-sensitive feature behavior, so I’m leaving it reviewed rather than approving or merging.
…otifications # Conflicts: # desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swift
|
@Git-on-my-level the blocking item from your review is resolved — flagging it because the request-changes is still on file and predates the fix. Your finding: Resolved at Verified at the current head
All checks are green on this head with none failing or pending. What's left is the product/UX sign-off you reserved for a human maintainer on the privacy-sensitive notification behavior — please dismiss the stale request-changes when you next pass. Updated for the current head The guard has now caught a real one, which I think is the strongest argument for landing this. AGENTS.md asks a new guard to cite the incident it would have caught. It has one, from this week: #11807 (cloud proactive messages to desktop over the listen socket) called
Against the fixed tree it returns So the class this PR closes is not hypothetical — it recurred within days, in a different subsystem, written by me, and was caught by a human review pass rather than by CI. That is the case for the checker: it turns "a reviewer has to notice" into "the build fails". Worth noting the reverse too — had this landed first, #11807 could not have been pushed in that shape. Nothing outstanding on my side; the blocking item ( |
…otifications # Conflicts: # .github/checks-manifest.yaml
… 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
…voice Maintainer request: being on a call is often exactly when a nudge is worth most — mid-call with someone is when "you owe them a task" is useful. The guard suppressed on either presence signal, which treated the interruption as the harm. It is not. The two signals are different harms and now decide different things: - Sharing a screen makes a private nudge visible to everyone on the call, and once seen that cannot be taken back. Still suppresses delivery. - Being on a call means others can hear the room but cannot see the screen. Delivery goes through. Speech is the part a call does change. A banner on a call is seen by the user alone; the same text read aloud is heard by everyone in the room and everyone on the call, with no screen share needed — and BasedHardware#11801 makes spoken output routine. So on a call the nudge is shown and not spoken. `currentPresence()` also reports what it detected whenever it detects anything. Without it a delivered nudge cannot be told apart from a detector that saw nothing, and those mean opposite things — the same silent-gate problem that made the wake word undiagnosable for two days. Verification: - xcrun swift test --package-path Desktop --filter 'PresenceAware| NotificationSpeech|NotificationSnooze|IntegrationNudge| SuggestionAssistantTelemetry' -> 140 tests, 0 failures - check-proactive-notification-gate.py -> OK - Live on a named dev bundle during a real Discord call, screen not shared, through the real grounding/evaluation/delivery path (probe_suggestion_nudge): NotificationService: presence — screenShared=false onCall=true Suggestion: delivering [90%] [commitment] "CleanMyMac is fine — but you said you'd Call David today" The same state on the previous build logged "withheld while others are present". Not live-verified: suppression while sharing. That path's detector and policy inputs are unchanged by this diff, and the sharing cases are covered by unit tests, but the live control could not be run — see below. Known gap, pre-existing and unrelated to this diff: `isShareIndicatorWindow` matches Zoom, Microsoft Teams and browser stop-sharing bubbles. Discord publishes no such window — while sharing, its only on-screen windows are `title="Window"` and the channel title — so a Discord screen share is not detected at all. Matching an untitled window would suppress nudges for anyone with Discord open, so it is filed rather than guessed at. Failure-Class: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Done — Sharing makes a private nudge visible to everyone, and once seen that cannot be taken back. Still suppresses. What a call does change is speech. A banner on a call is seen by you alone; the same text read aloud is heard by the whole call with no screen share at all — and #11801 makes spoken output routine. So on a call the nudge is shown and not spoken. Verified live on a real Discord call, screen not shared, through the real grounding → evaluation → delivery path: That is your example almost word for word, and the same state on the previous build logged 140 tests, Two honest notes. The detector now logs what it saw whenever it sees anything. Without it, a delivered nudge is indistinguishable from a detector that never fired, and those mean opposite things — the same silent-gate problem that made the wake word undiagnosable for two days. And the test turned up a gap that predates this change: Which also means the one path I could not verify live is suppression-while-sharing — Discord was the call I had. That path's detector and policy inputs are unchanged by this diff and the sharing cases are unit-tested, but I would rather say so than imply I watched it. Status on the blocking review. @Git-on-my-level's That is fixed on this head. guard let ownerID = RuntimeOwnerIdentity.currentOwnerId() else { return }
NotificationService.shared.sendNotification(
ownerID: ownerID, title: title, message: message,
assistantId: assistantId, sound: .none)The only remaining mention of the old primitive in that file is a comment explaining why it was wrong. I ran the checker your review cited rather than assuming: All checks are green on The product/UX sign-off you reserved is unchanged and still a human's — I'm not asking you to fold that into a dismissal. Edited to append the review-status section above rather than posting a follow-up comment. Verified the checker passes and the routing changed on this head before writing it. |
…ver listen websocket (#11807) * feat(listen): deliver cloud-generated proactive messages to desktop over 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) * fix(listen): fix pyright host cast, remove unused import, add changelog fragment * fix(listen): use direct async redis client to satisfy async blocker check * style(desktop): resolve force_cast swiftlint violations in ProactiveListenEventTests * fix(listen): subscribe the proactive dispatcher to the publisher's Redis `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 * fix(desktop): route cloud proactive messages through the notification 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 #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 * test(desktop): assert proactive-listen delivery shows and suppresses 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 * fix(desktop): drop the conversation_id block that only logged 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.
Current-main refresh + blocking-review proofRefreshed onto Scope integrity
Reviewer blocker: resolved on the current treeThe outstanding The static guard also covers future lanes: every proactive card must enter the shared master-toggle/frequency/snooze/presence boundary unless it has a narrowly documented functional allowlist reason. Behavior verification on current mainThis proves the current product split:
Static/repository gates
Known base-branch preflight issueThe aggregate preflight reaches and passes this PR's new notification guard, then stops on an unrelated That Windows file is not in this PR. The isolated repair is #12339, whose checks are all complete and green; I intentionally kept it out of this privacy-sensitive feature diff. @Git-on-my-level the technical blocker from the changes-requested review is verified resolved on the current head. What remains is the human product/UX sign-off previously reserved by the reviewer: whether this privacy behavior and its default/snooze UX should ship. |
Git-on-my-level
added
the
positive-signal
|
Follow-up review on head Blocker resolution, verified on this tree
What I verified file-by-file
The red Hygiene check is not from this PR
Two non-blocking observations
What remains is human maintainer sign-off: the product call on the presence semantics themselves (a shared screen suppresses the private card; a call without sharing delivers it unspoken) and on shipping a new repo-wide desktop CI gate. Leaving it for that review rather than escalating anything else. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Dismissed as stale: the blocking finding (NotchMomentsCoordinator.swift:183 calling FloatingControlBarManager.shared.showNotification directly) is resolved on head e1edc11 — the receipt now routes through NotificationService, and check-proactive-notification-gate.py plus its 7 fixtures pass on this tree.
3 participants
Summary
A proactive notification is addressed to one person. Omi delivered them identically whether the user was alone, in a call, or presenting to a room. A live session put "Submit prototype for SBI Hackathon before the deadline" on screen — useful when alone, a disclosure on a shared screen, and an interruption mid-meeting.
Two distinct harms, one rule:
I first scoped this to screen share only, reasoning that being on a call is not the same as your screen being visible. Testing against a live Google Meet call disproved that:
"Meet is fine — but you said you'd submit the SBI Hackathon prototype"was delivered mid-meeting. Detection now covers both.Where the guard sits
NotificationService.sendNotificationis the single choke point every proactive surface already routes through — suggestion, memory, insight, goals, meeting action items, plugin (9 call sites) — so one guard covers all of them rather than a per-assistant exception.respectFrequencyis the existing proactive/functional split and is honoured: functional notices (screen-recording repair prompt, Crisp replies, onboarding test) passfalseand still reach the user. Suppressing the capture-repair prompt during a share is precisely how a broken capture would stay broken, since that prompt is what tells the user to fix it.Detection reuses three signals that already exist and are already trusted in production:
activeScreenSharePresent()callAppIsUsingMicrophone()browserCallWindowPresent()The third is what caught the Meet case above. Detection is placed after the cheap boolean gates so the window scans never run for a notification an earlier gate already refused.
Withheld, not destroyed
This is the part worth reviewing closely.
SuggestionAssistantwritesrecentSuggestionsimmediately before delivering, and that window gates every later evaluation. Withholding only at the choke point would have recorded the suggestion as delivered and retired it permanently: the user never sees the card, and every regeneration after the call is filtered as a duplicate of something that was never shown.The assistant therefore consults the same shared policy before the dedup write and returns early, leaving the suggestion eligible once the call ends.
suppressed_presentingis a distinct delivery outcome from thefiltered_*ones: those retire a suggestion on its merits, this defers a good one on audience.Verification
swift test --filter PresenceAwareNotificationSuppressionTests→ 10 passed, including the two that pin the deferral guarantee (remembered ⇒ duplicate forever; unremembered ⇒ still deliverable).Live end-to-end on the dev serving plane, one session, no screen share — joined a Google Meet call in a browser:
The 17:27:17 delivery is the deferral proof: had the withheld suggestions been remembered, it would have been filtered as a duplicate.
Also included
One diagnostic commit: the model-chosen
categoryis now named on suggestion delivery and duplicate log lines.SuggestionPacing.dedupMemorypicks a suggestion's dedup depth from that category, so a repeat that should have been suppressed was previously unexplainable. It is what let me disprove my own first theory about a repeat report — the label wascommitmentall along, carrying full depth, which pointed at the similarity threshold inSuggestionDeduplication.isDuplicateinstead. That defect is not addressed here.Honest gaps
callAppIsUsingMicrophone()requires macOS 14.4+. On 14.0–14.3 a native-app call (Zoom desktop, FaceTime) is not detected and only browser calls are, via window title. This fails open — a missed suppression, never a missed notification.CGWindowListscans plus an audio-process enumeration per proactive notification that reaches this gate.ContextDeliveryAuthority) has its own gate reasons and is not touched here.Product invariants affected
Cited because
FloatingControlBarView.swiftandNotchMomentsCoordinator.swiftaresurfaces the invariant governs. Nothing here forks the shared transcript: both changes
are about whether a card is presented, not about where conversation state lives. No new
store, no per-surface continuity ring, no session identity moved into Swift — the notch
receipts still read
TasksStoreand the canonical action-items path exactly as before,and routing them through
NotificationServicechanges only the gating decision in frontof the same presentation primitive.
Failure class (fixes)
Failure-Class: none
Line-Count-Exception: desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swift | 1255 -> 1572 | This file is the single choke point where the master toggle, frequency throttle, snooze and presence gates live, so gated entry points belong here rather than in a sibling that could be bypassed. Growth is those gates, the deferral-preserving suggestion path, presentActionableProactiveNotification, and splitting presence into its two harms — sharing withholds delivery, a call withholds only the voice.
Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarView.swift | 3012 -> 3036 | Adds the "Silence suggestions for N hours" snooze menu (Resume / 1h / 4h / 8h) to the bar's context menu, the user-facing control this PR's honest gaps called out as a follow-up.