Skip to content

fix(desktop): notify every session through the Host catalog - #5742

Merged
jackwener merged 5 commits into
apache:mainfrom
Astro-Han:fix/desktop-all-session-notifications
Sep 26, 2026
Merged

jackwener merged 5 commits into
apache:mainfrom
Astro-Han:fix/desktop-all-session-notifications

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Completion and waiting-for-input notifications depended on the renderer's selected conversation, so a running background Session could finish or ask a question silently. Desktop now receives attention events for every Session available through its connected Host, independently of conversation selection.

Runtime Host includes attention in its existing Session catalog changes: completion/failure follows the canonical terminal fence, and waiting follows durable interaction admission. The existing connection feed owns delivery, reconnect, owner/Guest scope, and grant revocation. Attention is live best-effort; disconnected events are not replayed.

Desktop keeps session names, question/reply previews, the notification toggle, foreground suppression, and Dock bounce. Successful context compaction and cancellation stay silent. One desktop notifier deduplicates overlapping owner/Guest deliveries by Host epoch, Session, and event ID using a bounded in-memory set.

The source Host now applies its authoritative privacy policy; private or unreadable policy omits attention while ordinary catalog changes continue. This replaces the notification gate's stale client-local privacy read. The renderer notification IPC and its duplicate input model/validation are removed, and desktop notification wiring is required.

Adjacent cleanup removes the renderer refresh's unused row-return contract and local-only interfaces, consolidates Host event delivery and failed-subscription cleanup into one loop, and removes a pass-through copy helper. Existing routing tests now also cover attention payloads; Session retirement tests exercise the production refresh drain instead of mirroring it in a fixture.

Refs #5682.

Compatibility

Runtime Host compatibility epoch advances from 190 to 191 for the optional catalog frame payload. Mixed epochs are rejected during handshake. There is no new protocol operation, persisted subscription state, or storage migration.

Verification

  • Runtime Host build and 129 targeted tests pass: 10 execution tests through the real Host and 119 interaction, continuity, feed, protocol, and reconnect tests. Coverage includes unopened Session completion, question/sandbox requests, live privacy changes, scoped delivery/revocation, silent successful Turns/cancellation, and failure attention.
  • Desktop build, typecheck (including stories), and 49 targeted tests pass, including UDS candidate wiring, background Sessions, overlapping owner/Guest delivery, disposal, content fallback, notification gates, and Session retirement through the production catalog drain.
  • Ablations of Host privacy suppression, foreground suppression, the notification toggle, preview fallback, disposal, deduplication, catalog commit ordering, failed-read row preservation, connection scope, grant revocation, failed-subscription cleanup, silent successful Turns, and silent cancellation each produce assertion failures. Temporary mutations were restored.
  • Format, lint, and the strict renderer architecture check pass.
  • Removed the superseded renderer notification test and IPC-kind validation tests; coverage now exercises Host publication and the main-process consumer.
  • Native macOS banner delivery was not revalidated. Local E2E and the full repository test suite were not run.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex — diagnosis, implementation, tests, and review.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han Astro-Han added the effort/L Under 1000 readable lines label Sep 26, 2026
@Astro-Han Astro-Han changed the title fix(desktop): notify for every running session fix(desktop): notify every session through the Host catalog Sep 26, 2026
@Astro-Han Astro-Han added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 26, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[kabi-grok-reviewer]

I reviewed 77eedf6186e455bdf34912c4005f162dafbf73a3.

Design. #5682 made waiting notifications work for the selected conversation via renderer IPC. Background Sessions still finished or asked silently. Putting attention on the existing Host catalog feed is the right cut: one path for owner/Guest scope, reconnect, and grant revocation. Removing the renderer IPC and the stale client-local privacy read is Occam.

Privacy. Host omits attention when privacy.incognitoActive is true, and when reading that policy throws (execution-composition.ts 993–1005), then still publishes the catalog change (1006). That is fail-closed for notifications, fail-open for ordinary catalog. Desktop no longer consults a local settings store for this. docs/workspace-privacy-context.md matches.

If a Guest receives the frame, they already had sessionCatalog mask for that sessionId (host-change-feed.ts 177–178). Desktop then fills title/body from getSession / getSharedSession (runtime-host-notifications.ts 34–45); fetch failure falls back to generic copy, so a failed lookup does not invent a name. I did not prove Guest mask filtering for revoked grants (sol's lane).

Protocol. Epoch 191 (main is 190) for optional attention on session.catalog.changed. Mixed epochs reject at handshake. No new operation, no storage migration.

P0–P2: none.

Checked this round: session-catalog-change.ts 23–76; protocol index.ts 107–108 vs origin/main 190; execution-composition.ts 993–1006; host-change-feed.ts 106–178; runtime-host-notifications.ts 23–51; notifications-policy.ts 30–101; privacy doc 42–52; #5682 as prior selected-conversation path; git merge-tree --write-tree origin/main 77eedf618 exit 0, tree 3fd865b71.

Not checked: once-per-fence delivery, bounded-set eviction, disconnect loss, grant-revocation leak, native banner, tests this round.

简体中文

我审查了 77eedf6186e455bdf34912c4005f162dafbf73a3。通知走 Host catalog 站得住。隐身或读不到策略时 Host 去掉 attention,catalog 照发。epoch 191。没有 P0–P2。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[kabi-sol] Reviewed fe24748b64c66e07a596f0bb090c3830c5ddc620. No new P0–P2 findings in the state/delivery paths I checked.

After inspecting the updated feed publication and catalog-refresh code, I rebuilt the workspace dependencies and Desktop main, then ran 146 targeted tests successfully: terminal fencing, interaction admission, Host feed/protocol, real Host message execution, Desktop notification observation/policy/candidate lifecycle, session retirement, and the local-IPC/authenticated-WebSocket owner/Guest scenario.

The delivery guarantees are bounded:

  • Terminal fencing and admitted waiting events are covered; cancellation and successful compaction are silent, while failed compaction still emits an error notification.
  • Owner/Guest overlap is deduplicated by Host epoch, Session and event ID. A separate production-module probe confirmed suppression within the 512-entry window and delivery again after eviction; this is not indefinite exactly-once delivery.
  • A disconnect/reconnect probe confirmed missed attention is not replayed.
  • The real transport test checks Guest grant revocation closes access; the scoped-feed test checks no subsequent attention is delivered to that revoked subscription. This does not retract notifications already delivered or queued before revocation.

The live test CI check is failing at publication; these local results do not establish merge readiness. I have not attributed that CI failure.

Not verified: native OS banners, packaged Electron, Windows, every in-flight notification/revocation ordering, or the full repository suite. This is a scoped COMMENT, not an approval.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

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

Independent review (blind — no existing comments read). Conclusions bind to the live head 010f2994ccff1209c85e7bddac2e502c9ea50fc6; everything executable below was verified on fe24748b64c66e07a596f0bb090c3830c5ddc620, and the coordinator has confirmed the only delta is the main merge plus the refreshed docs/astryx-surface-file-inventory.md (no PR-diff change).

Verified locally (real Windows 11, Node 24.18.1 — the version CI pins):

  • Suites: execution-host-message 10/10 (incl. completion attention reaching the catalog feed from an unopened Session, waiting question/sandbox attention, privacy suppression), host-change-feed 2/2, session-catalog-protocol 22/22 (attention frame decode, body cap), session-continuity-coordinator 49/49, desktop runtime-host-notifications 4/4 (owner/Guest dedup, disposal, fallback copy, scoped guest feed), notifications-policy 2/2, runtime-host-desktop-candidate 26/26. One attribution note: interaction-coordinator.test.js reports 0/21 — all 21 "failures" are EBUSY unlinking the temp SQLite fixture at teardown with zero assertion failures (my machine's documented environment class; the same teardown fails on the base), not this PR.
  • Ablation 1 (dedup set): disabling the dedup check in deduplicateRunNotifications (scratch, reverted) → exactly overlapping owner and Guest feeds notify once while distinct events, sessions and Hosts still notify fails. The bounded set (512, oldest-evict) is load-bearing.
  • Ablation 2 (terminal fence): disabling the completion publish at the canonical fence (session-continuity-coordinator.ts:698) → exactly an unopened Session completion reaches the Host catalog feed once fails; the errored/waiting paths are untouched.
  • Ablation 3 (privacy gate): disabling the Host-side incognito check (execution-composition.ts:997) → exactly the two privacy tests fail (Host privacy changes suppress completion attention…, a pending private question…), everything else green. The gate reads the Host's authoritative policy and fails closed on read error.
  • After every revert-restore I rebuilt and re-ran — all green again.

Windows notification notes: toast delivery and foreground suppression are covered by the desktop suites above (the gates are main-process predicates — windowFocused, enabled, supported, e2e — platform-neutral); Dock bounce is macOS-only by design and the PR keeps it (the PR does not change taskbar flashing or keep-system-awake.ts beyond a 1-line dependency rename, which I read — behavior unchanged).

Read and agree with: attention rides the existing catalog frame as an optional payload (epoch 191 rejects mixed peers at handshake — no silent drop on old clients); waiting attention bodies are built per request kind with permission/client_capability deliberately body-less; the renderer notification IPC and its duplicate input model are genuinely gone (preload/bridge-contract deletions check out); delivery is live best-effort with no replay of missed events.

Not verified: native banner delivery on any OS (no visible-window automation on this user-active machine; the macOS banner was not revalidated by the author either); E2E; the full repo suite.

No P0–P3 findings.


Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han marked this pull request as ready for review September 26, 2026 17:34

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at 010f2994ccff1209c85e7bddac2e502c9ea50fc6. Three reviews from different model families found no P0–P2, and CI test is green on this head.

This head merges main and refreshes docs/astryx-surface-file-inventory.md. Apart from that file, this PR's own diff is identical to fe24748b, which is where the delivery, privacy and Windows verification ran.

  • Triggers: completion and failure follow the canonical terminal fence, and waiting follows durable interaction admission. Cancellation and successful compaction stay silent; failed compaction still notifies.
  • Delivery:
    • Owner and Guest overlaps are deduplicated by Host epoch, Session and event ID, in a bounded set of the most recent 512 events. An evicted event can notify again.
    • Events missed while disconnected are not replayed, which is the documented live best-effort contract.
    • Revocation stops further attention through the scoped feed. It cannot recall a system notification that was already shown or queued.
  • Privacy: the source Host omits attention when the session is incognito or the policy read fails (execution-composition.ts 993–1005), and the ordinary catalog change still publishes.
  • Protocol: epoch 190 → 191 for the optional catalog frame payload. The renderer notification IPC is removed.
  • Tests constrain the change: disabling the dedup set, the terminal-fence trigger, or the privacy gate fails exactly the corresponding new test, whether overlap, unopened-completion, or the two privacy cases. The affected suites pass on Windows; interaction-coordinator teardown hits EBUSY there, with no assertion failures, and so does main.
  • Not verified: native OS banners, packaged Electron, and E2E.

Automated review by an AI agent, approving at a maintainer's request. This is not an independent human review.

@jackwener
jackwener merged commit 538c37c into apache:main Sep 26, 2026
2 checks passed
@Astro-Han
Astro-Han deleted the fix/desktop-all-session-notifications branch September 26, 2026 17:45
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
Desktop raised a notification only for the Session on screen, from the
renderer's own event stream; a Session waiting on the user never notified;
and the incognito gate read a local settings copy that privacy changes never
reach, so incognito did not suppress banners carrying the session name and
reply.

The Runtime Host now attaches `attention` (completed / errored / waiting) to
its Session catalog change: completion and failure after the canonical
terminal cut, waiting after durable interaction admission. Cancellation and a
successful compaction stay silent. The Host drops attention while its privacy
policy is incognito or unreadable; the catalog change itself still goes out.
Desktop main observes every connected Host's catalog feed, names the banner
after the Session, deduplicates owner and Guest deliveries by Host epoch,
Session and event, and bounces the Dock. The renderer's
`notifications.runEnded` IPC and its inputs are gone.

Compatibility epoch 162 -> 163 for the optional frame payload.

Deviations from upstream: Desktop ignores attention from the WorkHub
coordination Session, which has no conversation to open here. A canonical
refresh that throws still publishes its catalog change, without attention;
upstream moved the change after the refresh and lost it. The real-Host tests
wait for attention instead of asserting it the moment the Turn reads
terminal, and prove its absence with the next Turn, whose admission queues
behind the terminal publication. We never took apache#5682, whose waiting copy and
Dock bounce this brings.

Lead: apache#5742 (538c37c).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
Watermark bfb315a. Done: apache#5573/apache#5600/apache#5601, apache#5521, apache#4875, apache#5723, apache#5738,
apache#5742. Not applicable: apache#5737, apache#5593. Deferred: apache#5730. Consider: apache#5599,
apache#5120, apache#5693. Diverged: apache#5740. Skipped: ACP, WorkHub, upstream renderer and
packages/ui, one refactor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants