Skip to content

fix(notifications): retire phone alerts on the next agent turn - #30900

Closed
brennanb2025 wants to merge 44 commits into
mainfrom
brennanb2025/sta-10535-duplicate-phone-alerts
Closed

brennanb2025 wants to merge 44 commits into
mainfrom
brennanb2025/sta-10535-duplicate-phone-alerts

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2603 $\color{#cf222e}{\Huge{\mathbf{−}}}$​261 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2342
Prod 44 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2030 $\color{#cf222e}{\Huge{\mathbf{−}}}$​336 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1694

Problem and change

A phone alert for an earlier agent turn can remain after the next turn starts. Each sending machine now withdraws its own earlier phone alert when it positively observes a genuine later turn of the same live source. Queued or refused input, maintenance, replay and unrelated work do not count. When that live proof is unavailable, the alert remains until read or dismissed. Notification bookkeeping does not block an agent action.

Phone pairing remains independent for each machine. A phone paired with desktop A and remote host B can receive one copy from each machine by design. A phone paired only with A receives A's alerts, including remote work A observes; the existing terminal observer duplicate described below remains possible. The remote host continues its normal phone send path; the capability, subscription declaration and origin-based suppression from the earlier version of this PR have been removed.

The retained terminal and native completion paths share the existing turn-end, cancellation and turn-identity mechanism. New-host phone registration keeps its register-first ordering. No new setting, saved field, RPC operation or push-gateway change is introduced.

Remaining terminal case

This PR does not fix the existing case where a desktop observes one terminal turn through both its title watcher and a later, unlinked hook completion. That completion-authority work is tracked in STA-12618 and follows the shared terminal observer work in #31182. Legacy hosts retain their existing path. There is no time-window, content or agent-name matching added here.

Tracked in STA-10535. Missing terminal start signals remain tracked in STA-11617. The existing workspace Mark as read dismissal gap remains tracked in STA-11936.

Verification

The approved suppression removal is pushed at dd008787ac7e901a5abf0b0ab3b230d7b4562ed5. Its eight named owning suites passed all 96 cases through the local test gate, and the ordinary commit hook passed. The production blobs match the approved removal exactly; the hook formatted three test/support files.

Fresh correctness and final whole-mechanism reference reviews found no new proven finding; the removal is a notification/mobile tests-row change with no new real run or screenshot required. The exact-head CI watch passed. The dedicated performance/readiness review is complete; final evidence and refs=done are recorded in the review status comment. The uncommitted terminal regressions are preserved for STA-12618 and are outside this PR; their failing cases describe an unchanged limitation, not a fix shipped here.

Historical real runs covered Claude and Codex terminals, native Codex chat, a terminal without status hooks, terminal exit/relaunch/close, old and updated paired hosts, previous-build state through graceful upgrade, and withdrawal on a later genuine turn. They cover unchanged flows only. The phone boundary used real encrypted sockets with simulated devices; physical-phone receipt, banner/tray presentation, push-gateway presentation and navigation were not exercised. No real-run or screenshot claim is made for the follow-up completion-authority work.

Historical visual proof

These screenshots show earlier real replies and restored host state. They cover unchanged flows; they do not verify the follow-up terminal completion-authority work.

Two genuine forwarded remote turns Socket event records establish A's exact sender-local withdrawal. The earlier suppressed-B count is superseded; these screenshots do not prove current B routing.

Old paired host Updated paired host
Old host native completion Updated host native completion
Baseline terminal Branch terminal
Baseline Claude third turn Branch Claude third turn

…ca machine

The machine that runs a terminal agent now sends that agent's finished and
needs-input alerts to its own phones from main, from the hook rows, instead of
only when its own window happens to hold the tab. A phone takes those alerts
only when every host it registered with does this, and tells each host which
other hosts already reach it, so a desktop driving that machine stops
forwarding the same alert to that phone. Any older desktop, server or phone in
the set keeps today's path.
@brennanb2025
brennanb2025 marked this pull request as ready for review October 21, 2026 20:43
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
Walkthrough

The changes add turn-based retirement for terminal-agent and structured-session mobile notifications. Terminal notifications now use hook-status observations and turn timestamps, while structured-session notifications use caller attribution and root-turn start events. Notification APIs and runtime wiring carry retirement data and desktop-connection proof. Desktop away-state detection also considers whether a visible window exists. Tests cover notification delivery, replay handling, caller attribution, and retirement behavior.

Priority: Normal

Merge Risk: Moderate · up to f3418

Some phone alerts may remain visible after a later turn or fail to dismiss with their desktop alert. Resolve the open notification-retirement concerns before merging.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description explains the problem, intended behavior, known review hold, linked issues, testing history, and visual evidence. However, it omits or incompletely fills several required template secti… Complete every required template section. Add an explicit issue reference under Fixes #, state the AI Disclosure or omit it only if the contributor is an internal maintainer, complete Review, Agent skill upstream boundary, Notes, and all Ch…
Passed Passed checks: 3 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the main change: retiring earlier phone alerts when a later agent turn starts.
Full details: Description check

Explanation

The description explains the problem, intended behavior, known review hold, linked issues, testing history, and visual evidence. However, it omits or incompletely fills several required template sections, including ELI5, explicit What Changed and Why sections, Fixes #, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the Checklist. It also states that the PR is not ready and that current CI and review are outstanding.

Resolution

Complete every required template section. Add an explicit issue reference under Fixes #, state the AI Disclosure or omit it only if the contributor is an internal maintainer, complete Review, Agent skill upstream boundary, Notes, and all Checklist items, and provide current-head CI and review results. Resolve or clearly document the STA-10535 duplicate-alert review hold before marking the PR ready.

✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c537648b-5fd2-4a73-82ed-f2726f8eb802
📥 Commits

Reviewing files that changed from the base of the PR and between 209e468 and 6063428.

⛔ Files ignored due to path filters: 1
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing: 36
  • src/main/agent-hooks/agent-transition-detector.ts
  • src/main/agent-hooks/server/server-status-identity.ts
  • src/main/host/electron-runtime-desktop-surface.ts
  • src/main/ipc/notifications-subject-retirement.test.ts
  • src/main/ipc/notifications.ts
  • src/main/native-chat/agent-session-journal/command-receipt-table.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-phone-origin.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turn-completion-feed.ts
  • src/main/notifications/desktop-away-state.test.ts
  • src/main/notifications/desktop-away-state.ts
  • src/main/runtime/agent-session-record-store.ts
  • src/main/runtime/agent-session-store-transactions.ts
  • src/main/runtime/orca-runtime-agent-transitions.ts
  • src/main/runtime/rpc/mobile-socket-wiring.test.ts
  • src/main/runtime/rpc/mobile-socket-wiring.ts
  • src/main/runtime/runtime-mobile-notification-controller.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-state.ts
  • src/main/runtime/runtime-service-command-surface.ts
  • src/main/runtime/structured-agent-session-mobile-attention.ts
  • src/main/runtime/structured-agent-session-phone-mirror.test.ts
  • src/main/runtime/structured-agent-session-runtime.ts
  • src/main/runtime/terminal-agent-mobile-attention.test.ts
  • src/main/runtime/terminal-agent-mobile-attention.ts
  • src/main/runtime/workspaces/workspace-process-listing.ts
  • src/preload/api/notifications-bridge.ts
  • src/preload/api/os-permission-api.ts
  • src/renderer/src/components/native-chat/StructuredAgentSessionAttentionBridge.tsx
  • src/renderer/src/components/terminal-pane/agent-completion-coordinator-types.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.ts
  • src/renderer/src/hooks/agent-hook-completion-notifications.test.ts
  • src/renderer/src/hooks/agent-hook-completion-notifications.ts
  • src/renderer/src/hooks/agent-hook-notification-retirement.ts
  • src/renderer/src/hooks/ipc-events/agent-status-event-applicator.ts
  • src/renderer/src/hooks/ipc-events/normalize-agent-status-event.ts
  • src/shared/notification-settings-types.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: acbaf2d6-18f9-43b8-9fec-35f752993923
📥 Commits

Reviewing files that changed from the base of the PR and between 6063428 and 7714dac.

📒 Files selected for processing: 18
  • src/main/agent-hooks/agent-transition-detector.ts
  • src/main/agent-hooks/server/server-status-identity.ts
  • src/main/runtime/agent-session-record-store.ts
  • src/main/runtime/agent-session-store-transactions.ts
  • src/main/runtime/orca-runtime-agent-transitions.ts
  • src/main/runtime/runtime-service-command-surface.ts
  • src/main/runtime/structured-agent-session-mobile-attention.ts
  • src/main/runtime/structured-agent-session-runtime.ts
  • src/main/runtime/workspaces/workspace-process-listing.ts
  • src/renderer/src/components/terminal-pane/agent-completion-coordinator-types.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.test.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.ts
  • src/renderer/src/hooks/agent-hook-completion-notifications.test.ts
  • src/renderer/src/hooks/agent-hook-completion-notifications.ts
  • src/renderer/src/hooks/agent-hook-notification-retirement.ts
  • src/renderer/src/hooks/ipc-events/agent-status-event-applicator.ts
  • src/renderer/src/hooks/ipc-events/normalize-agent-status-event.ts
  • src/renderer/src/hooks/useIpcEvents-agent-status-turn-completion.test.ts
🚧 Files skipped from review as they are similar to previous changes: 1
  • src/main/agent-hooks/agent-transition-detector.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔇 Additional comments: 23
src/main/runtime/structured-agent-session-phone-mirror.test.ts: 1

66-68: LGTM!

src/renderer/src/hooks/agent-hook-notification-retirement.ts: 1

13-13: Duplicate of an earlier review comment: remove the live PTY id from the source identity.

sourceFor includes getPtyId(). accepts calls sourceFor again when the notification dispatches. A hook done completion dispatches after the quiet window. If the PTY or leaf is gone by then, getPtyId() returns a different value, accepts returns false, and the completion is dropped. This affects run-aware payloads (runId or executionId).

The payload already carries terminalHandle, runId, executionId, and connectionId. Remove the PTY lookup from the identity. Alternatively, capture the PTY id once in observe and do not re-read it in accepts.

src/main/runtime/runtime-mobile-notification-controller.ts: 1

1-1: LGTM!

src/main/runtime/runtime-service-command-surface.ts: 1

14-14: LGTM!

Also applies to: 124-124, 151-154

src/main/ipc/notifications-subject-retirement.test.ts: 1

100-218: LGTM!

config/tsconfig.node.json: 1

16-20: LGTM!

src/main/runtime/orca-runtime-agent-transitions.ts: 1

39-41: LGTM!

Also applies to: 52-57

src/main/runtime/orca-runtime-terminal-agent-phone-push.test.ts: 1

13-13: LGTM!

Also applies to: 35-35, 55-65, 69-100

src/main/runtime/terminal-agent-phone-push.test.ts: 1

1-1: LGTM!

Also applies to: 31-39, 50-55, 79-85, 107-116, 138-149, 198-201, 234-237, 291-294, 317-317

src/main/runtime/terminal-agent-phone-push.ts: 1

53-131: LGTM!

Also applies to: 183-208

src/main/startup/main-process-runtime-service.ts: 1

151-152: LGTM!

src/renderer/src/components/terminal-pane/agent-completion-coordinator-stamped-turn-boundary.test.ts: 1

11-51: LGTM!

Also applies to: 175-175

src/renderer/src/components/terminal-pane/agent-completion-coordinator-types.ts: 1

7-7: LGTM!

Also applies to: 52-52

src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts: 1

150-157: LGTM!

Also applies to: 272-272

src/renderer/src/components/terminal-pane/agent-completion-hook-observer.ts: 1

159-159: LGTM!

Also applies to: 220-304

src/renderer/src/components/terminal-pane/agent-completion-identity-store.ts: 1

112-124: LGTM!

src/renderer/src/components/terminal-pane/agent-completion-notification-controller.ts: 1

46-60: LGTM!

Also applies to: 87-87, 122-123, 144-145, 186-191, 288-290

src/renderer/src/components/terminal-pane/use-notification-dispatch.test.ts: 1

95-95: LGTM!

Also applies to: 115-118, 276-277, 287-295

src/renderer/src/components/terminal-pane/use-notification-dispatch.ts: 1

20-20: LGTM!

Also applies to: 69-69, 134-136, 183-192

src/renderer/src/hooks/agent-hook-completion-background-turn-notifications.test.ts: 1

95-100: LGTM!

Also applies to: 112-120, 183-184, 219-228

src/renderer/src/hooks/agent-hook-completion-notifications.ts: 1

210-219: LGTM!

Also applies to: 228-234, 243-258

src/renderer/src/hooks/agent-hook-notification-dismissal.ts: 1

1-11: LGTM!

src/main/ipc/notifications.ts: 1

103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Use the shared LiveNotificationRetirement type for turnStarts.

The handler declares the turnStarts element shape inline as { notificationId: string; executionHost?: string }. src/shared/notification-settings-types.ts already exports LiveNotificationRetirement with the same shape. The renderer, preload, and controller use the shared type. A separate inline declaration can drift from it without any compile error across the process boundary.

♻️ Proposed change
-      turnStarts?: { notificationId: string; executionHost?: string }[]
+      turnStarts?: LiveNotificationRetirement[]

Add LiveNotificationRetirement to the existing type import from ../../shared/notification-settings-types in this file.

Based on learnings: "define IPC message and payload types in a shared module imported by both main and renderer processes, rather than duplicating type declarations."

Source: Learnings


ℹ️ Review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 72b42ff5-5830-4223-a7d3-ac2bb908d67d
📥 Commits

Reviewing files that changed from the base of the PR and between 7714dac and dbdd21c.

📒 Files selected for processing: 23
  • config/tsconfig.node.json
  • src/main/ipc/notifications-subject-retirement.test.ts
  • src/main/ipc/notifications.ts
  • src/main/runtime/orca-runtime-agent-transitions.ts
  • src/main/runtime/orca-runtime-terminal-agent-phone-push.test.ts
  • src/main/runtime/runtime-mobile-notification-controller.ts
  • src/main/runtime/runtime-service-command-surface.ts
  • src/main/runtime/structured-agent-session-phone-mirror.test.ts
  • src/main/runtime/terminal-agent-phone-push.test.ts
  • src/main/runtime/terminal-agent-phone-push.ts
  • src/main/startup/main-process-runtime-service.ts
  • src/renderer/src/components/terminal-pane/agent-completion-coordinator-stamped-turn-boundary.test.ts
  • src/renderer/src/components/terminal-pane/agent-completion-coordinator-types.ts
  • src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts
  • src/renderer/src/components/terminal-pane/agent-completion-hook-observer.ts
  • src/renderer/src/components/terminal-pane/agent-completion-identity-store.ts
  • src/renderer/src/components/terminal-pane/agent-completion-notification-controller.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.test.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.ts
  • src/renderer/src/hooks/agent-hook-completion-background-turn-notifications.test.ts
  • src/renderer/src/hooks/agent-hook-completion-notifications.ts
  • src/renderer/src/hooks/agent-hook-notification-dismissal.ts
  • src/renderer/src/hooks/agent-hook-notification-retirement.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1998d708-bb00-4707-87c3-3a0d213b7b3f
📥 Commits

Reviewing files that changed from the base of the PR and between 5268ae0 and f3418c5.

📒 Files selected for processing: 6
  • config/tsconfig.node.json
  • src/main/host/electron-runtime-desktop-surface.ts
  • src/main/runtime/orca-runtime-agent-transitions.ts
  • src/main/runtime/runtime-mobile-notification-controller.ts
  • src/main/runtime/runtime-service-command-surface.ts
  • src/main/runtime/workspaces/workspace-process-listing.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@brennanb2025

brennanb2025 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Held for unified alert retirement review (round 22). Desktop and phone alerts will share the same genuine-next-turn retirement decision. The audit, implementation and targeted verification are in progress; prior evidence below covers only unchanged flows.

A phone alert for an earlier agent turn can remain after later work starts. This PR withdraws only the sending machine's own earlier alert when held live status positively proves a genuine later turn of the same source. Unknown, replayed, refused, queued or unrelated work leaves that alert available for read or dismissal.

Each paired machine sends independently. A phone paired with desktop A and execution host B may get one copy from each machine by design. A phone paired only with A receives A's observed work. The former remote-host suppression, forwarding capability, subscription responsibility and native-origin bookkeeping are removed. Registration keeps main's register-new-host-first ordering. No saved field, phone operation or push-gateway change is added.

The existing terminal title-then-unlinked-hook case can still announce twice within one desktop. This PR does not change that edge or claim terminal exactly-once delivery. Execution-host completion authority is tracked in STA-12618 after #31182. Missing terminal starts remain STA-11617; the existing workspace Mark as read dismissal gap remains STA-11936.

Verification: the removal's eight named owning suites passed 96 cases through the test gate, and the ordinary commit hook passed. The exact-head CI watch passed at dd008787ac7e901a5abf0b0ab3b230d7b4562ed5. Fresh correctness and final whole-mechanism reference reviews found no new proven finding. The dedicated performance and readiness checklist passes are complete (refs=done); their operation bounds do not claim runtime latency, CPU or heap measurements. No production or test source change was made during this closeout.

Retained real-run evidence is on afd524e: Claude and Codex terminal turns, native Codex chat, a terminal without hooks, terminal exit/relaunch/close, old/updated hosts and previous-build state through graceful upgrade. It covers unchanged flows only. The phone boundary used real encrypted sockets with simulated devices; physical-phone receipt, banner/tray/push-gateway presentation and navigation were not exercised. No live SSH app result is claimed. The removal changes only notification/mobile subscription and delivery tests-row flows, verified by registered-RPC, retirement, mixed-version and scaling contracts. The earlier suppressed-B count is superseded and is not evidence for current independent routing.

Review carry: the preceding tests-row review closed at 27d4a6a1f58f840edf3c16b21f799d7e9158369b; the retained development-watch attribution correction was b8ad0dc, with test-observer-only adaptations at 27d4a6a and 15cfdb66a19530a451ee89a184ab827f6dfe93aa. Main merges 6ef407f and 8748d1c6904e1f8053ddc0685f390694f69e9f01 did not stale real-run evidence. Current review covers the shipped notification/mobile tests-row removal dd008787 against 15cfdb66, with the eight named contracts above. Historical qa_head=afd524e carries only the unchanged flows just listed; the removed suppression policy is not carried as a pass.

For Brennan's review: these retained screenshots show real replies and host state on the unchanged flows. Socket records establish A's sender-local withdrawal; the earlier suppressed-B count is superseded, and the screenshots do not verify STA-12618's follow-up design.

Two genuine forwarded remote turns

Old paired host Updated paired host
Old host native completion Updated host native completion
Baseline terminal Branch terminal
Baseline Claude third turn Branch Claude third turn

Review points:

@brennanb2025 brennanb2025 changed the title fix(notifications): one phone alert when a desktop drives a second Orca machine fix(notifications): avoid duplicate phone alerts for remote native chats Oct 26, 2026
@brennanb2025 brennanb2025 changed the title fix(notifications): avoid duplicate phone alerts for remote native chats fix(notifications): retire phone alerts on the next agent turn Oct 28, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing: the next-turn alert clearing this PR ended up shipping no longer fixes the reported problem, so we're not merging it. The duplicate-alert problem stays tracked in STA-10535, and the desktop-only terminal double alert in STA-12618.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant