Skip to content

feat(desktop): withhold proactive notifications while other people are present - #11864

Open
aryanorastar wants to merge 28 commits into
BasedHardware:mainfrom
aryanorastar:feat/presence-aware-notifications
Open

aryanorastar wants to merge 28 commits into
BasedHardware:mainfrom
aryanorastar:feat/presence-aware-notifications

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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:

  • sharing a screen makes a private nudge visible to everyone on the call
  • being in a call at all makes it interrupt a conversation

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.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, since that prompt is what tells the user to fix it.

Detection reuses three signals that already exist and are already trusted in production:

Signal Catches
activeScreenSharePresent() outgoing share (already used to pause capture during shares, #10143)
callAppIsUsingMicrophone() an active call
browserCallWindowPresent() a muted browser call, where mic input has dropped

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.

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 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_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 PresenceAwareNotificationSuppressionTests10 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.

Also included

One diagnostic commit: the model-chosen category is now named on suggestion delivery and duplicate log lines. SuggestionPacing.dedupMemory picks 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 was commitment all along, carrying full depth, which pointed at the similarity threshold in SuggestionDeduplication.isDuplicate instead. 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.
  • 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. If the team would rather this be a setting than a default, that is an easy follow-up.
  • The other proactive lane (ContextDeliveryAuthority) has its own gate reasons and is not touched here.

Product invariants affected

  • INV-CHAT-1

Cited because FloatingControlBarView.swift and NotchMomentsCoordinator.swift are
surfaces 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 TasksStore and the canonical action-items path exactly as before,
and routing them through NotificationService changes only the gating decision in front
of the same presentation primitive.

Failure class (fixes)

Failure-Class: none

Line-Count-Exception: desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swift | 1706 -> 2072 | 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, presence split into its two harms (sharing withholds delivery, a call withholds only the voice), the required kind, and re-reading snooze/presence inside the async settings callback.

Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarWindow.swift | 6081 -> 6113 | The notch-only card guard has to sit at the presentation boundary, before currentNotification is set: that field drives the bar's width, its minimum size, and several reveal guards, so a card that is not shown must never be marked current. All of that state is private to this file, and the manager entry point every card already routes through is here too.

Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarView.swift | 3226 -> 3250 | 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.

Review in cubic

`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.
@chatgpt-codex-connector

Copy link
Copy Markdown

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.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

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

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 on the assumption they mean the same thing.

Withheld, not destroyed

Same ordering as the presence guard, for the same reason: SuggestionAssistant consults the snooze before writing recentSuggestions. 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), so the three are separable in telemetry.

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."

probe_suggestion_nudge returned outcome suppressed_snoozed. Note the ordering that matters: the suggestion was generated and had already passed the duplicate filter before the snooze withheld it — so this is a deferral, not a suggestion that was going to be dropped anyway.

  • Settings row verified on screen showing live state ("Silenced until 12:35 AM").

Placement, and why it moved

I 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

  • Durations are fixed at 1/4/8 hours — no "until tomorrow", no custom value.
  • The snooze is global, not per-assistant: it 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.
  • Unchanged from the original description: the macOS 14.4 gap in call detection, the window-scan cost, and the untouched ContextDeliveryAuthority lane.

aryanorastar added a commit to aryanorastar/omi that referenced this pull request Aug 19, 2026
@aryanorastar

Copy link
Copy Markdown
Contributor Author

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:

Silence Notifications setting

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 (Silenced until 12:35 AM) and the menu offers Resume now while a snooze is active, so the control is never one-way.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@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 NotificationService.sendNotification choke point all 9 proactive call sites already share, and withholds without burning the dedup window so a good suggestion is deferred, not lost. 10 unit tests + a live Meet session in the PR description prove the deferral guarantee.

Also fixed the missing Line-Count-Exception Hygiene failure — FloatingControlBarView.swift grew for the snooze-menu control (the "no user-facing opt-out yet" gap this PR's own description flagged, built in the same diff).

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.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

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.

Closed

The context director lane bypassed both gates

This is the one that mattered. 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. The 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 there because every caller of that entry point is proactive; functional notices go through sendNotification with respectFrequency: false and are unaffected.

"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 reasons

Window-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

  • 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" displays "Silenced until 9:00 AM tomorrow" and withholds; silencing now withholds director cards as well as suggestion cards, where previously only the latter were affected

Remaining 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; 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. Unifying them is a larger refactor than this PR should carry.
  • Durations remain 1/4/8 hours plus "until tomorrow"; no arbitrary custom value.

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.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

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

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. Nothing user-initiated sits behind them. It called the primitive directly:

_ = FloatingControlBarManager.shared.showNotification(...)

That skips every gate in sendNotification: the master Notifications toggle (#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.

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 NotificationService.sendNotification rather than adding a third call-site guard. The boundary is the problem, not the call site.

Guard, because two lanes shared the cause

The 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.

desktop/macos/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 gets added there with its reason; the rule does not relax.

Wired into .github/checks-manifest.yaml in both local and ci lanes, with fixture tests — a checker that has never failed is not a guard, so the fixtures are the two lanes that actually bypassed the gates.

Full suite

before this PR's test fix:  5479 tests, 1 skipped, 2 failures
after:                      5479 tests, 1 skipped, 1 failure

One failure was mine. testDeliveryOutcomesAreClosedAndJoinWithoutCardContent pins the exact DeliveryOutcome vocabulary so no outcome can carry card content into telemetry. This PR adds two, so it fired — the guard working, not a test to loosen. Extended by exactly the two values added here; a sixth outcome added without touching that line still fails.

The remaining failure is pre-existing. RewindCaptureExclusionGenerationTests.testOwnerSnapshotStaysCurrentWhenAuthLeadsUnresolvedRewindDatabase:

  • passes in isolation via --filter, failing only in a full run — cross-suite ordering, not the assertion
  • a full run with this PR's NotchMomentsCoordinator change reverted — the only change here touching RuntimeOwnerIdentity snapshots, and so the only plausible link to an owner-snapshot test — still fails identically: 5479 tests, 1 skipped, 1 failure, same test

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

  • Per-assistant snooze — nobody has asked to silence insights while keeping task nudges; four times the state and UI on a guess.
  • Notification when a snooze lapses — a notification announcing notifications are back is itself an interruption at a moment the user did not choose.
  • Custom snooze duration — a picker for an arbitrary interval is a lot of UI for what four presets cover.
  • Unifying ContextDeliveryAuthority.freeGate with these gates — a larger refactor than a notifications PR should carry.
  • macOS 14.4 call detection — API availability; already fails open.

Remaining honest gaps

  • The checker matches 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 it is a behaviour change beyond the snooze/presence scope: these receipts were previously unthrottled.
  • No unit test drives presentContextDirectorNotification end to end; it is @MainActor and builds real presentation state. The gate decision is covered by the shared policy tests, the wiring by the checker and live verification.
  • The 9am resume hour for "until tomorrow" is fixed, not configurable.

@Git-on-my-level Git-on-my-level added security-review Touches auth, provider routing, secrets, or security-sensitive surfaces workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior macOS labels Aug 19, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.py correctly encodes the intended invariant that proactive deliveries should route through NotificationService, and its unit test file passes locally, but the checker fails against the real source tree because NotchMomentsCoordinator.post still calls the floating-bar primitive directly.
  • .github/checks-manifest.yaml wires 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.swift posts the “wrote this down” receipt through FloatingControlBarManager.shared.showNotification at line 183. That path still bypasses the snooze/presence checks added in NotificationService, which is exactly the privacy/workflow class this PR is trying to close.
  • desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swift adds the snooze and presence gates in sendNotification and presentContextDirectorNotification; 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.swift checks snooze/presence before writing to recentSuggestions, which preserves deferred suggestions instead of turning a suppressed card into a future duplicate. The new telemetry outcomes in SuggestionAssistantTelemetry.swift and InsightAssistantTelemetry.swift make those deferrals observable.
  • desktop/macos/Desktop/Sources/MainWindow/Pages/Settings/Sections/SettingsContentView+NotificationsPrivacy.swift, SettingsPage.swift, and FloatingControlBarView.swift add 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
@aryanorastar

Copy link
Copy Markdown
Contributor Author

You are right, and the checker was right. Fixed in bd45ea8c.

What happened. 5e63c3e4 routed NotchMomentsCoordinator.post through NotificationService.sendNotification. The very next commit, 960bd33d — whose message is only about the telemetry test — silently reverted it. That commit should not have touched this file at all.

Cause. While measuring an A/B baseline for the gate-evaluation cost I ran git checkout 96f50bbd -- NotchMomentsCoordinator.swift to get the pre-fix behaviour back. That command stages the revert, not just the worktree. I restored the worktree afterwards with cp and never cleared the index. git status showed MM on that path and I read past it, so the staged old version went into the next commit.

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 960bd33d would have suggested looking at a notifications file.

Fix. Restored byte-identical to 5e63c3e4 via git show 5e63c3e4:<path>, and this time inspected git diff --cached before committing rather than trusting the status letters.

$ python3 desktop/macos/scripts/check-proactive-notification-gate.py
check-proactive-notification-gate: OK
$ python3 -m unittest discover -s desktop/macos/tests -p 'test_check_proactive_notification_gate.py'
Ran 7 tests — OK
$ swift build   # clean

On the other two red checks. Desktop Swift Build & Tests was not a separate failure — that job only asserts VERIFY_RESULT = success from Static & Test Contracts, so it was the same checker firing, one hop downstream. PR Metadata Preflight was mine and unrelated: my Line-Count-Exception declared 2708 -> 2732 for FloatingControlBarView.swift while the synthetic merge measures 2713 -> 2737 — main moved under it. Corrected in the PR body.

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.

@aryanorastar

aryanorastar commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Statistics — measured, not estimated

Numbers. 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 positives

The question is whether "someone else is present" fires when it should and only when it should. I cross-checked every suppression against MeetingDetector, a subsystem that predates this work, runs on an independent signal and shares no code path with the presence gate. Two unrelated detectors agreeing is a stronger check than my gate agreeing with itself.

count
Proactive notifications withheld for presence 9
Confirmed by MeetingDetector reporting an active meeting at that instant 9 (100%)
False positives 0

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:

count
Total deliveries in sample 40
Delivered while MeetingDetector reported an active meeting 4
— in sessions predating the feature (baseline) 2
— within 8s of a call ending, after the feature 2

The last two are not clean passes and I am not scoring them as such. MeetingDetector runs poll=4.0s, offGrace=8.0s, so its meeting ENDED timestamp lags the real hang-up by up to 8s. Both deliveries land inside that window (7s and 12s before ENDED). I cannot separate "leaked during a call" from "call was genuinely over" at that resolution. Honest figure: 0 confirmed mid-call leaks after the feature, 2 unresolved at the boundary.

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 after

Before 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.

Cost

Cooldown 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.

outcome evaluations share cost
duplicate of a recent suggestion 205 42.8% $0.082
withheld (snooze or presence) 139 29.0% $0.056
evaluation failed (network) 87 18.2% $0.035
delivered to the user 40 8.4% $0.016
nothing worth saying 8 1.7% $0.003
total paid 479 $0.19
free gate skips (cooldown 720 / dwell 99) 819 $0

$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. SuggestionDeduplication.isDuplicate uses Jaccard word overlap at threshold 0.6, and the same commitment reworded slips under it: "Meet is fine — but the prototype is overdue" against "You said you'd submit the prototype — it's overdue." I found this while adding the category label in this PR's diagnostic commit, and did not fix it here. On cost grounds it is the highest-value next fix in the funnel.

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 NSURLErrorNetworkConnectionLost. Traced to stale pooled connections on URLSession.shared. An in-process A/B confirmed it: after moving GeminiClient to its own session, all 6 remaining -1005s came from subsystems still on the shared pool and zero from Gemini. Separate fix, not in this PR.

Method and limits

  • The cross-check subsystem is independent of the gate, which is the point; its offGrace=8.0s also bounds the resolution, which is why two boundary deliveries are reported unresolved rather than scored either way.
  • $0.0004/evaluation is a rounded estimate. All calls proxy through the backend, so I have no per-call billing line to confirm against — override with --cost-per-eval.
  • 9 presence suppressions is a small sample. 100% precision against an independent detector is meaningful; it is not enough to bound the false-positive rate tightly.
  • macOS 14.0–14.3 cannot use callAppIsUsingMicrophone() (14.4+ API), so below that only browser calls are caught. Not represented in this sample — I have no 14.0–14.3 machine.
  • Single user, single machine, three days.

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
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level requested changes addressed on tip 45aa51ad, plus a third lane the gate caught after the merge with main.

Your ask — NotchMomentsCoordinator.swift:183 — was already fixed in bd45ea8c before this round: the receipt now routes through NotificationService. That was the last remaining direct caller on the branch as it stood.

A third lane appeared from main, not from this branch. IntegrationNudgeCoordinator landed in #11729 after this branch forked, and presents its "Connect <app>" card straight through FloatingControlBarManager. Merging current main made check-proactive-notification-gate.py fail on it — which is the checker doing exactly its job, on a lane nobody wrote with these gates in mind.

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 sendNotification. It returns Void and keeps the presentation result to itself, and the coordinator needs that result: 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 return value to tell "bar refused, do not retry" from "queued, may still appear". Threading that through sendNotification would mean classifying all twelve of its exit paths into the result enum, on privacy-sensitive code, for one caller.

presentActionableProactiveNotification is instead a sibling of presentContextDirectorNotification: same gate order (owner → master toggle → frequency → snooze → presence), same result type, same callback contract. It differs only in carrying a FloatingBarNotificationAction and in throttling against the caller's own assistantId instead of the context-director budget. Suppression composes with the bounded budget for free — onPresented never fires, so a withheld offer stays unspent and the nudge can 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.pyOK (was the failing check); its own 7 fixtures still pass
  • New testASuppressedPresentationDoesNotSpendTheBudget proven live rather than assumed: a stub calling onPresented on a .suppressed card fails it (shownCount 1 vs 0); restoring the stub passes

Honest scope note: that new test pins that .suppressed — newly reachable now that the gates can withhold a nudge — composes correctly with the budget. It is not a regression test for the routing itself; the static checker is that guard, and it is already wired into the desktop contract lane.

Separate commit 45aa51ad clears four pre-existing force_try/force_unwrapping violations in this PR's own NotificationSnoozeTests.swift. They are not in the SwiftLint baseline and were failing desktop-swiftlint independently of the above; both tests are now throws so the same assertions run through XCTUnwrap. 11 tests still pass.

Product/UX sign-off on the hard-scope behaviour is unchanged and still yours.

@undivisible undivisible left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.
  • ContextDeliveryAuthority remains 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.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Delta review on head d57a905e — since the last pass on 2cec3ee105 this is one feature commit (61bbe135, "let a spoken proactive card stay in the notch") plus two merges from main. The earlier rounds' conclusions stand; this covers what changed since.

The notch-only delivery, verified

  • FloatingBarNotchOnlyCardPolicy (FloatingControlBarState.swift) is a pure three-input rule — spokenAloud && !hasAction && !isPersistent — with all four corners pinned in FloatingBarNotificationPreviewPolicyTests. The two guards that matter are right: a silent card still takes the panel, so the panel-suppression can never deliver a card to nobody; and a card with an action, or a persistent card waiting on a decision, always takes the panel, because a notch glow cannot be clicked or answered.
  • The early-return branch in FloatingControlBarWindow.swift (if notification.staysInNotch) mirrors the panel path's bookkeeping: authorization snapshot cleaned with removeValue, onPresented fired exactly once via removeValue(...)?.onPresented(), suggestion outcome .delivered, advice presentation recorded, and notificationSent tagged with the distinct floating_bar_notch surface. NotchCardVoiceDelivery.cardPresented was moved ahead of the branch so both surfaces register voice context. I also traced the telemetry for double-counting — the branch emits .delivered directly while also firing onPresented back into sendNotification's recordPresentation closure — and the closure records speech and the frequency timestamp only, never suggestion outcomes, so there is no double emission.
  • NotificationSpeech.willSpeak feeding spokenAloud: at both call sites in NotificationService.swift keeps the surfaces coherent: alone with speech on, the card rides the notch; on a call (speech withheld), the card takes the panel — shown and unspoken, exactly the tested split.

Still no ungated lanes after the merges

Re-ran the guard on the head tree: check-proactive-notification-gate: OK, 7/7 fixtures pass. A raw scan for direct FloatingControlBarManager.shared.showNotification callers finds only NotificationService.swift (the three gated entry points, plus presentJITDetailCard, which re-presents a tapped banner as a persistent card and is correctly not gated), Onboarding/OnboardingChatView.swift, and TrialBannerService.swift — exactly the checker's documented allowlist. The Aug 29 merge brought in no new lane.

One product edge worth a maintainer's eye (non-blocking)

willSpeak reflects the speech setting, not audibility. With proactive speech enabled but output inaudible — system muted, headphones disconnected mid-session — a spoken card now rides the notch alone: nothing visible, nothing heard, and the suggestion is marked delivered and consumed by the dedup window. Before this commit the panel was the backstop for exactly that case. That is a real trade this commit makes implicitly; it deserves an explicit product call rather than a drive-by code change from review.

Residuals from the last round are unchanged and still non-blocking: the async system-banner callback re-checks owner but not snooze/presence inside its callback window; macOS 14.0–14.3 native call detection fails open; the checker's masker does not blank string-interpolation bodies.

What stands between this and merge is the same human product call as before — the presence semantics (a shared screen suppresses the private card; a call delivers it unspoken), the snooze UX, sign-off on shipping the repo-wide desktop CI gate with its hand-maintained allowlist — now plus the notch-only audibility trade above.


by AI on behalf of David

@Git-on-my-level Git-on-my-level added the needs-maintainer-review Needs a human maintainer to sign off before merge label Aug 30, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Independent verification pass on head d57a905e, this time executing the new tooling rather than only reading it, plus a few residuals worth having on record.

Executed

  • desktop/macos/tests/test_check_proactive_notification_gate.py (run against the head blob): 7/7 fixtures pass, including both real regression shapes and the comment/string-literal masking cases.
  • check-proactive-notification-gate.py on a synthetic tree: clean tree reports OK; a planted FloatingControlBarManager.shared.showNotification( call exits 1 with the correct file:line. Line numbers survive masking, so reports are actionable.

Verified in the diff

  • NotificationService.swift — gate order is cheap-first (defaults read before the window scans), and the snooze/presence gates honor the respectFrequency split, so the screen-recording repair prompt still reaches a user mid-snooze. presentActionableProactiveNotification mirrors the director path's gate order and result type; on suppression onPresented never fires, so the integration's bounded offer budget stays unspent (pinned by the new budget test in IntegrationNudgeCoordinatorTests.swift).
  • SuggestionAssistant.swift — both suppression checks sit after the owner re-check and before SuggestionDeduplication.remembering(...), so a withheld suggestion defers instead of being retired by the dedup window. That ordering is the whole deferral guarantee, and it is right.
  • FloatingControlBarWindow.swift / FloatingControlBarState.swift — the notch-only early return fires onPresented, records delivery under the distinct floating_bar_notch surface, and cleans up the authorization snapshot; FloatingBarNotification.== is id-only, so the new staysInNotch field cannot perturb equality. Since spokenAloud is derived from speech.willSpeak, a card only stays in the notch when speech will actually happen — the "silent card still takes the panel" corner is structurally safe.
  • NotificationSpeech.swiftothersCanHear short-circuits the utterance while visual delivery continues; pinned by the shown-but-not-spoken tests in PresenceAwareNotificationSuppressionTests.swift.
  • NotchMomentsCoordinator.swift / IntegrationNudgeCoordinator.swift — both lanes now route through the gated path instead of the primitive.

Residuals (non-blocking)

  • The checker matches the primitive call textually, so a call routed through an intermediate local alias would evade it; and multi-line raw strings ("""…""") aren't masked, which can only over-report prose, never miss a real call. Acceptable for a cheap sound rule — worth remembering if a bypass ever reappears.
  • NotificationService.snoozeDurations is defined and tested, but the Settings menu (SettingsContentView+NotificationsPrivacy.swift) and the bar context menu (FloatingControlBarView.swift) hard-code the same three durations; mapping over the constant would prevent drift.
  • The silence-notifications changelog omits the "Until tomorrow" option that Settings offers.
  • The snooze row label uses the cached @State (SettingsPage.swift), so it can read "Silenced" briefly after a snooze lapses until re-render; the subtitle already reads live expiry. Cosmetic.

No new dependency, network, or data-handling surfaces; telemetry additions are bounded outcome enums without notification text. The final product call on the suppression policy (share suppresses; a call silences speech only) and on the new CI gate stays with the maintainer, including the documented fail-open on macOS 14.0–14.3 for native-call detection.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

The Settings menu spelled out "For 1 hour" / "For 4 hours" / "For 8 hours" as three
literals while `NotificationService.snoozeDurations` already defined the same three and
the snooze tests asserted against it, so changing the list in one place would have
silently left the other behind. The menu now maps over the constant.

The changelog fragment listed 1, 4 and 8 hours but omitted "Until tomorrow", which
Settings has always offered.

Note for the reviewer: the second hard-coded site named in the review does not exist —
`FloatingControlBarView.swift` contains no duration literals; the Settings menu was the
only one.

Verification: `swift build` in `desktop/macos/Desktop` — clean.

Failure-Class: none
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Both of the drift residuals are fixed on 5e6d07e5.

The Settings menu now maps over NotificationService.snoozeDurations instead of spelling out three literals, so the menu and the constant the snooze tests assert against cannot separate. The changelog fragment now names "Until tomorrow" alongside the 1/4/8-hour options.

One correction: the second hard-coded site does not exist. FloatingControlBarView.swift has no duration literals — I grepped the whole Sources/ tree and the Settings menu was the only place they appeared, so there was nothing to unify on the bar side.

I left the cached-@State label alone. The subtitle already reads live expiry, and the only visible effect is the word "Silenced" persisting until the next render after a snooze lapses; making the label live would mean adding a timer to the settings page for a one-word cosmetic, which seems worse than the symptom. Happy to do it if you disagree.

The checker residual you noted — an intermediate local alias evading the textual match — is real and I have not addressed it. Closing it properly means resolving aliases, which is a different kind of checker than the cheap sound rule this is; it did not seem worth expanding the PR for.

Verification: swift build in desktop/macos/Desktop, clean.

The previous commit replaced three duration literals in the Settings menu with a
ForEach over `NotificationService.snoozeDurations`. Desktop Swift Release Compile then
hit the 1h job cap and was cancelled, taking the required check down with it.

The job was already at 51m of that 60m cap on the last green head, so the margin was
9 minutes and this file feeds the release-mode whole-module compile. I have no proof
the ForEach is what tipped it — the debug build shows no slow type-checking at a 200ms
threshold, and runner variance alone could cover 18% — but spending a gate's remaining
margin on a duplication cleanup the review called non-blocking is a bad trade either
way, so the literals are back.

What stays is the real defect: the changelog fragment listed 1/4/8 hours and omitted
"Until tomorrow", which Settings has always offered.

The duplication is now documented in place, including the fact that it is genuinely
unguarded — `testOfferedDurationsAreSaneAndAscending` asserts on the constant only, so
the menu can drift from it without failing anything.

Verification: `swift build` in `desktop/macos/Desktop` clean; the diff against the last
green head is comment lines plus one changelog string, so the compile shape is unchanged.

Failure-Class: none
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Correcting my last comment: the ForEach change I described is reverted on 35e43d5d. It turned this PR red and I should not have pushed it.

What happened. Replacing the three duration literals with a ForEach over NotificationService.snoozeDurations was followed by Desktop Swift Release Compile hitting the 1h job cap and being cancelled, which took the required Desktop Swift Build & Tests check down with it. Worth noting the static and test lanes actually passed — VERIFY_RESULT: success; the gate failed purely on RELEASE_RESULT: cancelled.

The margin was already thin. On the last green head (d57a905e) that job ran 15:31:28 → 16:22:45 — 51 minutes of a 60 minute cap. Mine ran 60m46s: compile 30 min, then the UserNotifications release-mode test step 29.5 min before the cap cut it. So this job has been sitting at 85% of budget independently of anything here.

I do not have proof the ForEach is what tipped it. A debug build with -warn-long-expression-type-checking=200 reports nothing on that file, and 18% is within plausible runner variance. But spending a gate's last 9 minutes on a duplication cleanup that the review itself called non-blocking is a bad trade whichever way the causation runs, so the literals are back.

What I kept is the actual defect: the changelog fragment listed 1/4/8 hours and omitted "Until tomorrow", which Settings has always offered.

One thing I got wrong in the code comment and want on the record: I first wrote that NotificationSnoozeTests pins the menu against the constant. It does not. testOfferedDurationsAreSaneAndAscending asserts on the constant alone, so the menu really can drift from it silently. The comment now says that plainly rather than implying coverage that is not there.

Also still standing from my last comment: the second hard-coded site named in the review does not exist — FloatingControlBarView.swift has no duration literals.

The release-compile headroom looks like the more useful thing to fix, but it is a workflow change and CODEOWNERS routes /.github/workflows/** away from me, so flagging rather than touching it.

Net diff against the last green head is now comment lines plus one changelog string.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Verification pass on the current head 35e43d5d, covering the two commits since the last maintainer review round (d57a905e) and re-running the new guard rather than only reading it.

Delta since d57a905e5e6d07e55 + 35e43d5d net out to almost nothing, which is the right outcome: the ForEach over NotificationService.snoozeDurations is fully reverted, the three duration literals in SettingsContentView+NotificationsPrivacy.swift are back, and the replacement comment honestly documents the duplication, the 51-minute release-compile budget that forced the choice, and the fact that testOfferedDurationsAreSaneAndAscending pins the constant alone. The kept piece is real: the 20260819-silence-notifications.json changelog fragment now names "Until tomorrow", which Settings has always offered. No behavior change in the delta.

Guard, executed against the head blobtest_check_proactive_notification_gate.py: 7/7 fixtures pass, including both real bypass shapes and the comment/string-literal masking cases. On a synthetic tree with a planted FloatingControlBarManager.shared.showNotification( call outside the allowlist, the checker exits 1 with the correct file:line. The two new .github/checks-manifest.yaml entries are live rather than dead config: run_checks.py resolves the manifest, selects checks by trigger-path match, and honors the platforms: ["macos"] tag — consistent with "Desktop Swift Static & Test Contracts" passing on this head while the Windows portability lane stays out of scope.

Residuals, for the record — the three known ones are documentation-level and correctly owned in the code comments: the checker's textual rule does not resolve intermediate local aliases (accepted; a sound cheap rule beats a fragile alias resolver), the Settings "Silenced" label is cached @State until the next render after a snooze lapses (cosmetic), and the release-compile lane runs at ~85% of its 60-minute cap independent of this PR — that last one is a repo-level CI-budget observation worth a maintainer eye separately from this change.

Everything green on this head. The remaining step is the product/privacy sign-off already reserved for a human maintainer; nothing in this delta changes the state that review rounds on e1edc114/2cec3ee105/d57a905e established.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

The comment claimed the ForEach tipped the release-compile job over its 60 minute cap.
The next run disproved that: the revert is compile-identical to the last green head and
still took 56m55s, against 51m on that head. The lane is at ~95% of its cap and drifting
on its own; the ForEach was not the cause.

Leaving the literals in place regardless — the dedup is cosmetic and the lane has about
three minutes of headroom — but the stated reason is now the measured one.

Failure-Class: none
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Correcting myself again, this time in your favour: the ForEach was not what tipped the release lane over.

The revert run is compile-identical to the last green head — the only delta is comment lines and one changelog string — and it took 56m55s (17:12:26 → 18:09:21). The head before it took 51m. The run I broke took 60m46s.

51m00s   d57a905e   green
60m46s   5e6d07e5   cancelled at the cap   (ForEach)
56m55s   35e43d5d   green, compile-identical to d57a905e

So the lane is drifting on its own and now sits at ~95% of its 60 minute cap. Your "~85% independent of this PR" read was right, and my stated reason for the revert was wrong. I have corrected the code comment, which asserted the ForEach caused it.

I am still leaving the literals in place — the dedup is cosmetic and three minutes of headroom is not where to spend it — but the duplication is documented as genuinely unguarded rather than blamed on something that turned out not to be true.

Flagging the real number here since it is a repo-level problem heading for everyone, not just this PR: on current drift the next few percent of variance cancels this required check on any desktop-touching branch, and the failure mode is ugly — the static and test lanes pass (VERIFY_RESULT: success) while the gate fails on RELEASE_RESULT: cancelled, so it reads as a test failure when it is a budget overrun. I have not touched it because CODEOWNERS routes /.github/workflows/** away from me.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Delta verified on the new head 445f83a5 — exactly one commit since the 35e43d5d round, and it is docs-only: the code comment in SettingsContentView+NotificationsPrivacy.swift now gives the measured reason the ForEach over NotificationService.snoozeDurations stays out (the compile-identical revert ran 56m55s against 51m green), replacing the earlier claim that the ForEach tipped the lane over. No behavior change in the delta.

Record correction, accepted and extended. The correction in #11864 (comment) is right, and the current head makes it sharper: Desktop Swift Release Compile on 445f83a5 measured 56.8m against the 60m cap — ~95%, not the ~85% the earlier automated round stated. With roughly three minutes of headroom, ordinary variance can cancel this required check on any desktop-touching branch, and the failure shape (static and test lanes green, release lane cancelled) reads like a test failure when it is a budget overrun. Nothing in this PR worsens it — the lane drifts on its own — but it deserves maintainer action independent of this change (cap bump or lane split; CODEOWNERS routes /.github/workflows/** away from the contributor, so it cannot come from this branch).

Guard, re-executed on this head's blob. desktop/macos/tests/test_check_proactive_notification_gate.py: 7/7 fixtures pass, including both real bypass shapes and the comment/string-literal masking cases.

The state established across the rounds on e1edc114/2cec3ee105/d57a905e/35e43d5d stands on this head: the choke-point gating in NotificationService.sendNotification plus presentActionableProactiveNotification/presentContextDirectorNotification, the deferral-not-retirement ordering before SuggestionDeduplication.remembering(...) in SuggestionAssistant.swift, the notch-only spoken-card path in FloatingControlBarWindow.swift, and the check-proactive-notification-gate.py gate registered in .github/checks-manifest.yaml. All required checks green on 445f83a5. The persistence across the reverts-from-main, and the self-corrections that argued against your own earlier claims, are genuinely appreciated.

The one remaining step is unchanged and it is not a code step: the product/privacy sign-off on the suppression policy itself — share suppresses, a call silences speech only, the documented fail-open on macOS 14.0–14.3 for native-call detection — plus maintainer acceptance of the new CI gate's allowlist rule.


by AI on behalf of David — the remaining step for this PR is the maintainer's product/privacy sign-off on the suppression policy and the new CI gate, not further code changes.

Three resolutions, all from one root cause: main made `kind:` a required argument
on `FloatingControlBarManager.showNotification` ("what this card *is*", with no
assistant-id fallback), while this branch reroutes three direct `showNotification`
calls through `NotificationService` so those cards are subject to the master
toggle, frequency throttle, snooze and the new presence gates. Git merged main's
new argument onto call sites whose target this branch had changed.

NotificationService.swift (context-director card) -- union of both sides. Main's
`jitAmbientFeedbackContext:` and its widened
`isPersistent: jitFeedbackContext != nil || jitAmbientFeedbackContext != nil`,
plus this branch's `spokenAloud: speech.willSpeak`. The two additions are
independent; both are kept.

IntegrationNudgeCoordinator.swift -- kept this branch's routing through
`presentActionableProactiveNotification`, which is the point of the change, and
carried main's `kind: .integration` through it. That meant threading a required
`kind` into `presentActionableProactiveNotification`: passing a card through the
gate must not cost it the kind main now insists every card declare.

NotchMomentsCoordinator.swift -- auto-merged, so no conflict was raised, but it
did not compile: main's `kind:` landed on a call this branch had already pointed
at `sendNotification`, which takes no such parameter. Dropped the argument,
because `sendNotification` already derives
`ProactiveNotificationKind.from(assistantId:)` internally -- the identical
expression -- and gates its category toggle on that same value. A docstring line
records why it is absent so the next merge from main does not re-add it.

Dart and web formatting skipped: this merge stages no first-party change to
either, and running the formatters over main's files would only diverge them
from main.

Verified: 140 tests, 0 failures across the IntegrationNudge, NotificationService,
ProactiveNotification, NotchMoments, PresenceAware and NotificationSnooze suites.
check-proactive-notification-gate fixtures still 7/7.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Pushed 61b7536 — merged current main, which clears the conflict. No behavior change; the whole delta is conflict resolution plus one compile fix the merge required.

Why it conflicted. Main made kind: a required argument on FloatingControlBarManager.showNotification — "what this card is", with no assistant-id fallback. This branch reroutes three direct showNotification calls through NotificationService so those cards become subject to the master toggle, throttle, snooze and the presence gates. Git therefore merged main's new argument onto call sites whose target this branch had changed.

Three resolutions:

  1. NotificationService.swift (context-director card) — union of both sides. Main's jitAmbientFeedbackContext: and its widened isPersistent: jitFeedbackContext != nil || jitAmbientFeedbackContext != nil, plus this branch's spokenAloud: speech.willSpeak. The two additions are independent, so both are kept.

  2. IntegrationNudgeCoordinator.swift — kept this branch's routing through presentActionableProactiveNotification, which is the point of the change, and carried main's kind: .integration through it. That meant threading a required kind into presentActionableProactiveNotification: passing a card through the gate must not cost it the kind main now insists every card declare. This is the +5 lines the NotificationService.swift exception now accounts for.

  3. NotchMomentsCoordinator.swiftthis one auto-merged, so git raised no conflict, and it did not compile. Main's kind: landed on a call this branch had already pointed at sendNotification, which has no such parameter. I dropped the argument rather than adding one, because sendNotification already derives ProactiveNotificationKind.from(assistantId:) internally — the identical expression the direct call had spelled out — and gates its category toggle on that same value. Passing it too could only ever drift. A docstring line records why it is absent so the next merge from main doesn't re-add it.

Line-Count-Exceptions refreshed in the body. All three were stale — main had grown every one of those files since they were written (NotificationService 1467 → 1706 at base, FloatingControlBarWindow 5411 → 5818, FloatingControlBarView 3012 → 3158). They now declare the real base and head.

Verified: 140 tests, 0 failures across the IntegrationNudge, NotificationService, ProactiveNotification, NotchMoments, PresenceAware and NotificationSnooze suites. check-proactive-notification-gate fixtures still 7/7. PR preflight passed: 129 checks. git diff origin/main...HEAD is 22 files, +1251/-26 — the same shape as before the merge, with nothing from app/ or web/ pulled in.

Nothing here touches the remaining step, which is unchanged and is not a code step: the product/privacy sign-off on the suppression policy — share suppresses, a call silences speech only, the documented fail-open on macOS 14.0–14.3 — plus maintainer acceptance of the new CI gate's allowlist rule.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Merge-delta verification on head 61b7536 — the first head since the 445f83a5 round, and the only unreviewed change is the merge of current main plus its conflict resolutions. The earlier rounds' conclusions stand; this covers what the merge did to them.

The three kind: resolutions, verified on the head tree

  • NotificationService.swift — main made kind: required on FloatingControlBarManager.showNotification and added JIT-generation fences plus a reportUnauthorizedDrop reporter; the merge keeps both sides. This branch's gates are intact on the head blob: the snooze and presence gates still sit before the owner re-check in sendNotification, shouldWithholdSpeechForPresence still feeds othersCanHear into NotificationSpeechOnDelivery, and presentActionableProactiveNotification now carries the required kind: straight through to the primitive, so passing a card through the gate does not cost it what it is.
  • IntegrationNudgeCoordinator.swift — still routes through presentActionableProactiveNotification rather than the primitive, now with kind: .integration; the suppression-without-spending-budget contract is unchanged and still pinned by testASuppressedPresentationDoesNotSpendTheBudget.
  • NotchMomentsCoordinator.swift — the auto-merge that did not compile was resolved by dropping kind: rather than adding one. Verified equivalent on the head blob: sendNotification derives ProactiveNotificationKind.from(assistantId:) at its category-toggle gate and passes the identical expression to its internal showNotification call, so omitting the argument cannot drift. The doc comment recording why it is absent should protect the next merge from main from re-adding it.

Guard, executed rather than read

  • test_check_proactive_notification_gate.py against the head blob: 7/7, including both real bypass shapes and the comment/string-literal masking cases.
  • The checker itself on a synthetic tree with a planted FloatingControlBarManager.shared.showNotification( call outside the allowlist: exits 1 with the correct file:line. CI confirms the same on this head (Desktop Swift Static & Test Contracts green).

Notch-only delivery survived the merge

FloatingBarNotchOnlyCardPolicy (FloatingControlBarState.swift) still computes staysInNotch at construction from spokenAloud && !hasAction && !isPersistent, and the early-return branch in FloatingControlBarWindow.swift still fires onPresented exactly once, records .delivered with the distinct floating_bar_notch surface, and never sets a spoken card as currentNotification. Main's interject/PTT additions reach it only through the shared dismiss/queue helpers.

Known residuals, unchanged by this delta — the async system-banner callback re-checks owner and (new from main) JIT generation but not snooze/presence inside its callback window; macOS 14.0–14.3 native-call detection fails open as documented at currentPresence(); the checker's masker does not blank string-interpolation bodies (acknowledged, left as a cheap-sound-rule tradeoff); the Settings duration literals remain knowingly duplicated with the measured rationale in the comment.

Nothing in the merge changes the state the prior rounds established, and the persistence through the reverts-from-main and this conflict round is appreciated. What stands between this and merge is the same non-code step as before: the product/privacy sign-off on the suppression semantics themselves — a shared screen suppresses the card, a call without sharing delivers it unspoken, the documented fail-open on macOS 14.0–14.3 — plus maintainer acceptance of the new repo-wide desktop CI gate and its hand-maintained allowlist.


by AI on behalf of David — what's left is the product/privacy decision on the suppression semantics and CI-gate acceptance; the code delta itself is verified.

The async system-banner fallback in `presentContextDirectorNotification`
re-checked owner, JIT generation and authorization inside the
`UserNotificationCallbackBridge.notificationSettings` callback, but not snooze
or presence -- the two gates this change is actually named for.

A screen share can start inside that hop. The synchronous gate says nobody is
watching, the callback then puts a private card on a shared screen, and once
seen that cannot be taken back. It is the exact harm the feature exists to
prevent, surviving inside the feature's own fallback path.

Both are pure reads -- defaults for the snooze expiry, the three existing
signals for presence -- so the recheck costs one more window scan on a
notification that has already passed every cheaper gate.

Raised by review in four consecutive rounds as a known non-blocking residual.
It is cheap to close and nothing depends on it staying open, so closing it
rather than carrying it further.

Honest coverage note: the wiring is not unit-covered, for the same reason the
existing gate in this function is not -- it is @mainactor and constructs real
presentation state, which the PR has documented since the round that added it.
The decision itself is the same two pure policies already pinned by
PresenceAwareNotificationSuppressionTests and NotificationSnoozeTests.

Verified: 140 tests, 0 failures across the PresenceAware, NotificationSnooze,
NotificationService, ProactiveNotification, IntegrationNudge and NotchMoments
suites; check-proactive-notification-gate OK with its 7 fixtures passing.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Pushed e388c63 — closed the async-callback residual you've now raised in four consecutive rounds.

presentContextDirectorNotification's system-banner fallback re-checked owner, JIT generation and authorization inside the UserNotificationCallbackBridge.notificationSettings callback, but not snooze or presence — the two gates this change is named for. A screen share can start inside that hop: the synchronous gate says nobody is watching, the callback then puts a private card on a shared screen, and once seen that cannot be taken back. The harm this feature exists to prevent, surviving inside the feature's own fallback path.

Both re-reads are pure — defaults for the snooze expiry, the three existing signals for presence — so the cost is one more window scan on a notification that has already passed every cheaper gate. It re-uses shouldSuppressForSnooze and shouldSuppressForPresence rather than restating either, so the fallback cannot drift from the synchronous gate.

I closed it now rather than carrying it further because there is no approval on this PR to invalidate — which makes this the cheapest moment it will ever have. Every other open PR of mine is sitting on a head-specific approval where a push costs a re-review; this one is not.

Honest coverage note. The wiring is not unit-covered, for the same reason the existing gate in this function is not — it is @MainActor and constructs real presentation state, which this PR has documented since the round that added it. I did not add a test asserting A || B over two policies that are already individually pinned; that would pass whether or not the callback calls them, and a test that cannot fail for the change it claims to cover is worse than none. The decision itself is covered by PresenceAwareNotificationSuppressionTests and NotificationSnoozeTests; the placement is verified by inspection, and I would rather say that than imply otherwise.

Verified: 140 tests, 0 failures across the PresenceAware, NotificationSnooze, NotificationService, ProactiveNotification, IntegrationNudge and NotchMoments suites. check-proactive-notification-gate: OK with its 7 fixtures passing. PR preflight passed: 129 checks. NotificationService.swift Line-Count-Exception refreshed to 1706 → 2051.

Your other three residuals stand unchanged and I have not touched them: the macOS 14.0–14.3 fail-open is API availability, the checker's masker not blanking string-interpolation bodies is the accepted cheap-sound-rule tradeoff, and the Settings duration literals stay duplicated with the measured release-compile rationale in the comment.

Still no approving review on this PR — undivisible left it reviewed rather than approving, and both of your changes-requested were dismissed as resolved. So the gate remains the product/privacy call: a shared screen suppresses the private card, a call delivers it unspoken, and a new repo-wide desktop CI gate ships with a hand-maintained allowlist.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Verification pass on the tip e388c63 — the only commit since the 61b7536 round, and the one that closes the async-callback residual raised across several earlier reviews.

The settings-hop recheck, verified. presentContextDirectorNotification's system-banner fallback now re-reads both snooze and presence inside the UserNotificationCallbackBridge.notificationSettings callback (NotificationService.swift, the +21 lines this commit adds), so a share that starts during that async hop can no longer put a private card on a shared screen. Owner, JIT generation and authorization were already re-checked there for the same reason; the two gates this PR is named for now are too. Both are cheap reads (defaults plus one more presence scan) on a card that has already passed every cheaper gate — the right shape.

The gate demonstrably runs on this head. desktop-proactive-notification-gate was selected and passed in the Desktop Swift Static & Test Contracts lane on e388c63 (check-proactive-notification-gate: OK, then PASS, plus the fixtures lane passing alongside); the Hygiene lane skips it as platforms: [macos], which matches the manifest entry's intent.

Two small residuals, neither blocking:

  1. check-proactive-notification-gate.py still allowlists Sources/Onboarding/OnboardingChatView.swift, but that file no longer exists — main removed the retired onboarding wizard views in 44d48884cf (Sep 3). Harmless today (the scan never sees the path), but any future file created at that path would inherit the exemption silently. Worth pruning the entry in a follow-up.
  2. presentActionableProactiveNotification doesn't consult categoryToggleAllows(kind:) itself; it relies on IntegrationNudgeCoordinator's own isFeatureEnabled environment gate — the same underlying setting categoryToggleAllows reads for .integration. Equivalent for the only current caller (the coordinator's gate runs before offer() spends anything), but the layering differs from both sendNotification and presentContextDirectorNotification, which check at the service. A one-line doc note, or alignment later, would keep the invariant obvious.

Also re-confirmed on this head: NotchMomentsCoordinator.post omitting kind: is equivalent (ProactiveNotificationKind.from maps its assistant ids exactly as the old direct call spelled out, and the category gate reads that same derived value), the notch-only spoken-card path in FloatingControlBarManager fires onPresented, records advice presentation and reports the floating_bar_notch surface as delivered rather than dropping the card silently, and the suppressed-offer-stays-unspent behavior is pinned by testASuppressedPresentationDoesNotSpendTheBudget.

Left for human maintainer review: the final product call — on-call delivery with withheld speech as the default, and the macOS 14.0–14.3 native-call fail-open residual already on record — before merge.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Two residuals from review, both about a guard depending on someone remembering.

The proactive-notification checker allowlisted Sources/Onboarding/OnboardingChatView.swift,
which main removed in 44d4888 when it retired the onboarding wizard views. Nothing
failed, because the scan never sees a path that does not exist -- but the exemption was
still on the list, so any file later created there would have inherited a bypass nobody
granted it. The list only ever shrank by someone noticing, which is the thing this
checker exists not to rely on. It now fails on any allowlist entry whose file is gone.

Removing the entry broke an existing fixture, which had been using that path as its
example of an allowlisted caller: the dead exemption was load-bearing in the tests even
though the file was not there. Repointed at a live entry.

presentActionableProactiveNotification did not consult categoryToggleAllows, relying on
its only caller to gate itself on IntegrationNudgeCoordinator.isFeatureEnabled -- the
same setting categoryToggleAllows reads for .integration, so this changes nothing today.
It is at the service now so the next caller, with a different kind, cannot arrive
ungated. Same reasoning and same argument list as the context-director path.

Line-Count-Exception: desktop/macos/Desktop/Sources/ProactiveAssistants/Services/NotificationService.swift | 1706 -> 2072 | 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 its category gate, presence split into its two harms (sharing withholds delivery, a call withholds only the voice), the required kind, and re-reading snooze/presence inside the async settings callback.
Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarWindow.swift | 5818 -> 5850 | The notch-only card guard has to sit at the presentation boundary, before currentNotification is set: that field drives the bar's width, its minimum size, and several reveal guards, so a card that is not shown must never be marked current. All of that state is private to this file, and the manager entry point every card already routes through is here too.
Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarView.swift | 3158 -> 3182 | 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.

Failure-Class: none

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Pushed 7f95ceb — both residuals from the last pass. Same reasoning as last time for doing it now: there is still no approving review here, so a push costs nothing, which makes this the cheapest moment either will ever have.

The dead allowlist entry. You were right that it is harmless today and dangerous later. Sources/Onboarding/OnboardingChatView.swift came off main in 44d4888, and the checker never sees a path that does not exist — but the exemption stayed on the list, so anything created at that path later would have inherited a bypass nobody granted it.

I did not just prune it, because the failure mode you described is the class rather than the instance: the list only ever shrinks by someone noticing, which is exactly what this checker exists not to rely on. It now fails on any allowlist entry whose file is gone, mutation-checked in both directions (planting the stale entry back exits 1 naming it; removing it returns to OK).

Removing the entry broke one of the existing fixtures, which is the part worth reporting. test_allows_functional_callers_on_the_allowlist used that very path as its example of an allowlisted caller — so the dead exemption was load-bearing in the tests while the file it named had not existed for days. That is the drift shape in miniature. Repointed at a live entry; the suite is 8/8 including the new staleness fixture.

The category-toggle layering. Moved to the service. For the only current caller this changes nothing — categoryToggleAllows reduces to IntegrationNudgeCoordinator.isFeatureEnabled for .integration, which the coordinator already checks before offer() spends anything — so it is redundant today by construction. It is there so the next caller with a different kind cannot arrive ungated, and it uses the same argument list as the context-director path so the two cannot drift. The argument is the one already written in that path's own comment: a toggle that silences only the producers which remembered to check it would make the five-type promise in Settings a lie.

Verification: swift build clean; 46 tests / 0 failures across PresenceAwareNotificationSuppressionTests, NotificationSnoozeTests and IntegrationNudgeCoordinatorTests; check-proactive-notification-gate: OK with 8/8 fixtures; pre-push gate passed. NotificationService.swift Line-Count-Exception refreshed to 1706 -> 2072.

Your other residuals stand untouched and I am not planning to change them: the macOS 14.0–14.3 fail-open is API availability, the masker not blanking string-interpolation bodies is the accepted cheap-sound-rule tradeoff, and the Settings duration literals stay duplicated with the measured release-compile rationale in the comment.

The gate is unchanged and is not a code step: the product/privacy call on the suppression semantics — a shared screen suppresses the private card, a call delivers it unspoken, the documented fail-open below 14.4 — plus maintainer acceptance of the CI gate and its allowlist rule.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Delta review on head 7f95ceb33a — one commit since the e388c63 round, plus the root cause of the two red checks.

The new commit, verified

  • desktop/macos/scripts/check-proactive-notification-gate.py — the stale-allowlist guard is the right fix for the dead-exemption drift: stale_allowlist_entries() now fails the build when an ALLOWLIST path no longer exists, and test_flags_an_allowlist_entry_whose_file_is_gone mutation-checks it in both directions. Repointing test_allows_functional_callers_on_the_allowlist at a live entry (Sources/TrialBannerService.swift) where it previously depended on the dead Onboarding/OnboardingChatView.swift path closes the loop properly — the suite is 8/8 with the new fixture, and both manifest checks ran green in CI on this head (desktop-proactive-notification-gate PASS, -tests PASS).
  • NotificationService.swift — moving the categoryToggleAllows gate into presentActionableProactiveNotification is behavior-neutral for the only current caller: .integration reads IntegrationNudgeCoordinator.isFeatureEnabled, which the coordinator already checked itself. It closes the ungated-future-caller hole — the next caller with a different kind: can no longer arrive outside the Settings taxonomy. Gate order (owner → master toggle → frequency → category → snooze → presence) is documented and consistent with the director path.

The two failing checks are not caused by this PR — evidence

Desktop Swift Release Compile failed at the "Test UserNotifications callback regression in release mode" step, which runs swift test -c release --filter UserNotificationCallbackBridgeTests/. That compiles the entire test module in release mode, where DEBUG is undefined — and main's #13053 (merged today at 13:11Z, about an hour before this push) added ChatStreamingRenderBudgetTests.swift, which references ChatStreamingRenderProbe at 9+ sites with no #if DEBUG guard, while that type only exists under #if DEBUG. Every compile error in the log is cannot find type 'ChatStreamingRenderProbe' in scope (plus its cascades) inside that one file, which this PR does not touch.

The step only runs when the diff touches Notification-named Swift files (pre_push_ci_prediction.py_is_desktop_notification_input), which is why #13053's own PR never compiled that test in release mode and the break landed on main silently — this PR is simply the first Notification-path PR since. Desktop Swift Build & Tests mirrors the release job's result (RELEASE_RESULT: failure), so both reds share this single root cause.

Everything this PR owns is green in the same run: Desktop Swift Static & Test Contracts passed all 841 debug suites — including NotificationSnoozeTests, PresenceAwareNotificationSuppressionTests, the notch-only policy tests, and the new gate — and the release-mode app build itself completed.

Nothing to change on this branch for CI. The fix belongs on main: guard ChatStreamingRenderBudgetTests.swift (or its probe references) with #if DEBUG, or make the probe type visible in release builds. Until then, every notification-path PR will carry these two reds. CODEOWNERS routes /.github/workflows/** and the main branch away from this contributor, so that fix needs a maintainer — flagging it as a repo-level issue independent of this PR.

The remaining gate is the one the human review already named: product sign-off on the privacy-sensitive presence behavior (screen-share suppression, on-a-call delivery with speech withheld, snooze semantics). The engineering case is well-supported across the review history, including the honest self-corrections along the way.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Pushed ea20ef8 — merged current main, which is the refresh this branch needed to clear the release lane. No behaviour change; the whole delta is the merge.

The red release lane is fixed, and it was never this diff. Desktop Swift Release Compile and Desktop Swift Build & Tests failed on the previous head because main's #13053 landed ChatStreamingRenderBudgetTests.swift referencing a #if DEBUG-only probe without the guard, so any release-mode build of the test module could not compile. That is repaired on main by #13123, and this merge brings the repair in. Worth noting the old red was never re-run — CI does not re-evaluate a PR when main moves, so those two failures sat there for three days after the cause was already fixed.

Merged clean across 341 commits, and compiled rather than assumed. swift build is clean on the merged head — this branch has form here: a clean auto-merge on it once produced a tree that did not compile, so the build is the check that matters, not the absence of conflict markers.

Scope is unchanged at 22 files, +1333/−26, desktop/macos and .github only, with nothing pulled in from app/ or web/.

Line-Count-Exceptions refreshed — two of the three were stale again, main having grown both files since the last merge:

File Was Now
FloatingControlBarWindow.swift 5818 -> 5850 6081 -> 6113
FloatingControlBarView.swift 3158 -> 3182 3226 -> 3250

NotificationService.swift (1706 -> 2072) was already current.

Nothing here touches what is actually outstanding, which remains the product/privacy call rather than code: a shared screen suppresses the private card, a call delivers it unspoken, the documented fail-open below macOS 14.4, plus maintainer acceptance of the CI gate and its allowlist rule.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Verification pass on the refreshed head ea20ef89 — the first head since the 7f95ceb3 round, and the only delta is the merge of current main (6cd3dd65), so the earlier rounds' conclusions carry over. This pass re-checks the merge claim, executes the new guard, and root-causes the two red checks.

Merge-only claim, verified. ea20ef89 has parents 7f95ceb3 + origin/main@6cd3dd65; the PR's 19-file diff is unchanged in structure and scope (macOS Swift + guard registration + fixtures + changelog).

Executed, not just read. desktop/macos/tests/test_check_proactive_notification_gate.py against the head blobs: 8/8 pass, including the false-positive shapes (comment/string masking in mask_comments_and_strings) and the stale-allowlist fixture that fails when Sources/TrialBannerService.swift disappears.

The two red checks are lane capacity, not this diff. Desktop Swift Release Compile (job 103300206629) was cancelled at 1h0m24s while still mid-compile ([24/124] Compiling PLCrashReporterNSError.m); Desktop Swift Build & Tests then hard-fails by design on RELEASE_RESULT: cancelled — its own verify step reports VERIFY_RESULT: success. Debug build, the desktop test suite, and Desktop Swift Static & Test Contracts are green on this head. That matches the measured account in the SettingsContentView+NotificationsPrivacy.swift comment (the compile-identical revert ran 56m55s against 51m on the prior head — the lane is at ~95% of its 60-min cap and drifting up).

Spot-checks that still hold on this head:

  • NotificationService.swift — the snooze gate sits before the presence scan (a defaults read is cheaper than the window/audio enumeration currentPresence() performs); respectFrequency: false keeps functional notices (screen-recording repair, Crisp, onboarding test) deliverable through both gates; presentContextDirectorNotification's system-banner fallback re-reads snooze and presence after the async notificationSettings hop.
  • SuggestionAssistant.swift — both suppressions return before recentSuggestions = SuggestionDeduplication.remembering(...), so a withheld suggestion defers rather than retires; the closed telemetry set gains suppressed_presenting/suppressed_snoozed, pinned by the exhaustive-set assertion in SuggestionAssistantTelemetryTests.
  • FloatingControlBarWindow.swift — the staysInNotch early-return fires onPresented, records delivery telemetry on the floating_bar_notch surface, and NotchCardVoiceDelivery.cardPresented now runs before the branch so a spoken card still reaches a live voice session as context.
  • NotchMomentsCoordinator.swift / IntegrationNudgeCoordinator.swift — both former bypass lanes route through the gated service, and testASuppressedPresentationDoesNotSpendTheBudget pins that a withheld integration offer stays unspent.
  • NotificationSpeech.swiftothersCanHear silences the utterance while the banner still delivers; willSpeak is what ties the card's surface to actual speech.

Nothing new blocking from this pass. What remains is what the labels already say: the suppression semantics (share suppresses, call silences speech only) and the snooze UX are product calls needing a maintainer's formal sign-off, and the release-lane margin question is an infra decision independent of this diff.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

No push here — this is a pointer, since the two reds on ea20ef89 are the only thing on this PR that is not the product call.

I measured the release lane properly and put the numbers on #12735: the job is not drifting against a budget it nearly fits, it runs two sequential release compiles — the app at 31.5 min (Build complete! (1887.11s)), then the test module, cancelled at 27.8 min still mid-compile — on a warm cache. The second one exists to run UserNotificationCallbackBridgeTests, and --filter selects at run time, so it compiles the whole test module in release to run one suite. It fires only on notification-touching diffs, which is why this PR meets it every time and most PRs never see it.

Three fixes, all outside what I can send (/.github/workflows/** is CODEOWNERS-routed, and the third is a Package.swift change I would not make unasked): raise the job timeout, split the two compiles into separate jobs, or move that one suite into its own test target so the release compile is proportional to what it tests. Detail on the issue.

Separately, and not the cause here: the suite step's reds that other PRs are seeing are a different failure — every suite passes and one batch wedges for the full 1500s watchdog cap, overrunning the 1800s step guard. Fix opened as #13551. It does not touch this lane, so it will not turn these two checks green.

Nothing above changes what this PR is waiting on, which is still the product/privacy call on the suppression semantics and acceptance of the CI gate.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Fix for the two reds opened as #13553. Correcting what I said an hour ago, because I had the cause wrong and so did the fix I proposed.

I said the release lane's second compile was expensive because --filter selects at run time, so it compiles all 749 files of the Omi ComputerTests target to run one suite — and that moving that suite into its own target would make the cost proportional. Both wrong. The 27.8 minutes is Omi_Computer and every dependency compiling a second time:

14 Compiling FirebaseCore     1 Compiling Omi_Computer   1 Compiling PostHog
 5 Compiling GTMSessionFetcher 1 Compiling OmiWALTests   1 Compiling PHPLCrashReporter ...

Same .build/arm64-apple-macosx/release directory as the first step, so it is not a path mismatch — it is the flags. A plain swift build -c release has testability off, swift test -c release needs it on for @testable import Omi_Computer, and that difference invalidates the graph. A separate test target would have changed nothing.

Measured instead of inferred:

swift build -c release --build-tests -Xswiftc -enable-testing    Build complete
  then swift test -c release --skip-build --filter UserNotif...  6.9s, 11 tests, 0 failures
  without those flags, the same --skip-build                     error: Omi ComputerPackageTests.xctest doesn't exist in file system

That last line is what proves the two steps were never sharing products. #13553 builds the test targets with testability in the first step and lets the second reuse them, gated on should_notification_release_regression so main pushes and Package.swift PRs do not pay for test targets nothing runs.

It does not turn these two checks green by itself — that needs #13553 merged and this branch merged onto it. And the trade it carries belongs to a maintainer, not to me: on notification-path PRs the release-compile check would then verify a testability-enabled release build rather than a plain one. Type checking and the release-only Sendable diagnostics are unaffected; cross-module optimization differs.

Nothing here changes what this PR is actually waiting on, which remains the product/privacy call on the suppression semantics and acceptance of the CI gate.

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

Labels

macOS needs-maintainer-review Needs a human maintainer to sign off before merge positive-signal Automation verified a genuine fix/quality contribution security-review Touches auth, provider routing, secrets, or security-sensitive surfaces workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants