Skip to content

fix(native-chat): a chat's finished-turn alerts come from the host recorded on its tab, not its workspace id - #25081

Merged
brennanb2025 merged 5 commits into
mainfrom
brennanb2025/fix-attention-bridge-tab-host
Oct 5, 2026
Merged

brennanb2025 merged 5 commits into
mainfrom
brennanb2025/fix-attention-bridge-tab-host

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​160 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​156
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​23 $\color{#cf222e}{\Huge{\mathbf{−}}}$​10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​13

ELI5

A chat running on a paired Orca server can share its workspace id with a workspace on this computer. When that happens and you're looking at a different workspace, the desktop stops listening for that chat's "turn finished" events. You get no notification and no unread dot when the chat finishes. This PR makes the desktop listen to the host the chat actually runs on, which the chat tab already records.

What Changed

The problem (reproduced). Take a structured chat mirrored from a paired Orca server, in workspace repo1::/tmp/wt. A workspace with the same id then appears on this computer. Worktree ids are repoId::path, so two hosts can produce the same one. Now select any other workspace. The desktop drops its subscription to the paired server's turn-completion stream and subscribes to this computer instead. When the paired chat finishes a turn, nothing lights up and no notification is sent. The same mistake happens the other way round: a chat on this computer gets moved to the paired server's stream while that server's same-id workspace is selected.

Why it happened. The attention bridge decides which host to listen to by looking up the tab's workspace id (getRuntimeEnvironmentIdForWorktree). When two hosts have that id, the lookup can only answer through the selected workspace. With another workspace selected it returns nothing, and the bridge falls back to this computer. #24205 gave every structured chat tab a record of the host it was launched on or mirrored from (executionHostId). The chat pane, the status bridge and tab close all read that owner through one function, structuredAgentSessionOwnerForTab. It returns the recorded host, and falls back to the workspace only for a tab without a recorded host, refusing an ambiguous id. The attention bridge was the one reader with its own rule.

The fix. The attention bridge now asks structuredAgentSessionOwnerForTab and maps the answer with structuredAgentSessionTargetForHost, exactly as the status bridge and the chat pane do. Its own lookup is deleted, so every reader of a chat agrees on which host owns it. When no host can be named, the bridge holds no subscription, as the chat pane and status bridge already do.

Because the bridge now uses the shared rule, it also changes how it treats a tab with no recorded host whose workspace id two hosts share while that workspace isn't selected. Before, the bridge guessed this computer. Now it waits until the tab's host is recorded. The test for that case, added in #24518, changes accordingly. The wait is short: every structured chat tab gets its host recorded when it is launched (structured-agent-session-provisional-tab.ts:63) or the first time a host publishes it (terminal-surfaces.ts:126). That publish path is used by paired servers and by this computer's own chat sync (local-structured-session-tabs-sync/snapshot-apply.ts:149). A new test checks that a tab restored from before hosts were recorded gets its host on that first publish.

A tab with no recorded host whose workspace is the one selected keeps working exactly as before. Main has a test for this case: a paired chat whose tab was restored from before hosts were recorded, with a same-id workspace appearing on this computer while the paired workspace is selected. It must stay subscribed to the paired server and deliver the completion. That test is kept unchanged and passes with this PR, because the shared rule names the selected workspace's host when the id is ambiguous. Only the case where another workspace is selected changes, as described above.

What users see. A chat's finished-turn notification, unread dot and tab marker now arrive whichever workspace you have selected, including when a paired server and this computer share a workspace id. Nothing changes when workspace ids don't collide.

Why

One rule for "who owns this chat" means the chat pane, its status and its alerts can't follow different hosts. Before this change, during a collision the chat pane could read one host while the alerts listened to another. Reusing the shared function removes this bug class from the bridge rather than adding another guard for the colliding case.

Alternatives considered:

  • Read the recorded host in the bridge but keep the bridge's own fallback (guess this computer) for a tab without one. That was this PR's first version. It leaves two owner rules that disagree during a collision. The guess it keeps is the same guess this PR removes for recorded tabs, so it was dropped.
  • Pick the paired server whenever the lookup is ambiguous. That is still a guess, and wrong for a local chat whose id collides.

Linked Issue

None. Found while fixing a red test on main (that fix has since landed on main separately).

Visual Proof

Checked in the real app on Windows, before (main 752871bd678) and after (this PR, d1169d8ba06). The chat tab in this check has its host recorded, the normal case. That check is still valid for the current head: the only change since is a merge of main, which leaves the bridge code from d1169d8ba06 unchanged. The desktop app is paired to a second Orca running as a headless server on the same machine. A structured Codex chat runs on that server, using a stand-in Codex so no real agent or login is involved. The desktop is given a workspace of its own with the same id as the server's workspace, the way the repo's end-to-end tests set up a collision. The chat is sent a turn that takes about a minute, and another workspace is selected while it runs. Desktop notifications were switched off, so the unread marker is the signal.

Before (main): the server finished the turn, but the desktop never heard about it. The paired workspace shows no unread bell, and the desktop recorded no unread completion.

Before: other workspace selected, no unread bell on the paired workspace

The chat really did finish on the server (full reply, "Worked for 1m 1s"):

Before: the chat's reply finished

After (this PR): with another workspace selected, the paired workspace shows the amber unread bell ("Mark as read"), and the desktop recorded the chat's completion as unread.

After: other workspace selected, unread bell on the paired workspace

After: the chat's reply finished

Selecting the empty workspace opens a Claude chat there on its own, in both builds. That is existing behaviour, unrelated to this change. On this test machine it fails to start, because no Claude is signed in.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • StructuredAgentSessionAttentionBridge.test.tsx:

    • A new collision test for a chat whose tab records its paired server runs twice: with the paired workspace selected, and with another workspace selected.
    • Main's collision test for a tab restored without a recorded host (paired workspace selected) is kept unchanged and passes.
    • A new test covers a chat on this computer while a paired workspace with the same id is selected.
    • Turn desktop notifications on or off per machine #24518's test for an unrecorded tab now checks the shared rule: no subscription while the id is ambiguous, then a local subscription and delivery once the tab's host is recorded.
  • mirrored-agent-tab-label.test.ts: a tab restored without a recorded host gets one on its first publish.

  • Ablation:

    • With the bridge's own fallback put back (this PR's first version), the updated unrecorded-tab test fails.
    • With main's bridge, the collision case with another workspace selected, the local-chat case and the unrecorded-tab test fail (3 failed, 15 passed). Main's test for a tab without a recorded host passes with main's bridge, this PR's first version, and this PR.
  • Related files: the bridge test, structured-attention-dispatch.test.ts, notification-sources.test.ts, StructuredAgentSessionStatusBridge.test.tsx, ai-vault-session-resume-in-chat-workspace.test.ts and mirrored-agent-tab-label.test.ts give 91 passed.

  • Manual: the before/after check in the real app on Windows, described under Visual Proof.

  • Web typecheck (tc:web), oxlint and oxfmt on the changed files, the changed-code quality gate and the React Doctor changed-files check pass.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Remote / mixed versions: the recorded host is set by this desktop when it mirrors a paired server's tabs, from the connection the tab list arrived on. It works with paired servers of any version, and nothing sent between machines changes.
  • A removed paired server: a tab that still records a paired server that has since been removed keeps trying to reach it until the tab closes, the same as the status bridge does today.
  • SSH: structured chats don't run over SSH, so no owner names an SSH host for one. If one ever did, the bridge would hold no subscription, matching the chat pane and status bridge.
  • Folder workspaces: the recorded host doesn't depend on the workspace kind, and the shared fallback already handles folder workspaces.
  • Performance: one store selector per chat tab, as before.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…ntion-bridge-tab-host

# Conflicts:
#	src/renderer/src/components/native-chat/StructuredAgentSessionAttentionBridge.test.tsx
@brennanb2025
brennanb2025 marked this pull request as ready for review October 5, 2026 09:50
@coderabbitai

coderabbitai Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6475e4f7-bcbe-4486-a5e2-f188d066789f
📥 Commits

Reviewing files that changed from the base of the PR and between e46fa0d and 201f197.

📒 Files selected for processing (3)
  • src/renderer/src/components/native-chat/StructuredAgentSessionAttentionBridge.test.tsx
  • src/renderer/src/components/native-chat/StructuredAgentSessionAttentionBridge.tsx
  • src/renderer/src/runtime/web-session-tabs-sync/mirrored-agent-tab-label.test.ts

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


📝 Walkthrough

Walkthrough

The attention bridge now resolves a session owner from the tab and derives a runtime target from that owner. It renders the attention subscriber only when a target exists. Tests cover paired and local chats with colliding workspace IDs, an unrecorded chat that later records its host, and restoration of a mirrored tab’s execution host ID.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 201f1

No actionable merge-blocking risk is identified for this change; it is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 201f1

The change ties completion alerts to the host recorded on the chat tab rather than a potentially colliding workspace ID. No new permission or trust-boundary bypass was identified. Remote authorization and duplicate-event delivery guarantees were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is desktop attention state and notification delivery for open structured-chat tabs on local or paired hosts. Correcting the target restores the intended host’s event reachability without adding a command-execution or administrative operation to this path.

Trust Boundaries and Controls

  • observed — Target conversion rejects invalid, unresolved, and SSH-only owners. Downstream dispatch rejects unknown outcomes and mismatched sessions, while surface admission rejects closed tabs or superseded session keys. These are routing and attribution controls, not proof of remote authentication or authorization.

Resilience and Maintainability Implications

  • observed — The existing feed uses activation tokens and generation checks to reject stale frames and late subscription handles. Last-activation cleanup cancels reconnect work and unsubscribes; unsupported remote capabilities terminate without repeated probing. The feed is live-only and does not replay missed completions. Identical live frames are not deduplicated here, and the upstream guarantee was not established. These mechanisms are unchanged by the PR.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: finished-turn alerts use the host recorded on the chat tab instead of the workspace ID. It is specific and relevant.
Description check ✅ Passed The description is detailed and covers the template’s main sections, including the user-facing change, rationale, visual proof, testing, compatibility notes, and checklist. The Linked Issue section sa…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · 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.

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

✅ No new issues found.

Reviewed changes

  • Attention bridge follows the tab's recorded host. StructuredAgentSessionAttention now resolves its runtime target via structuredAgentSessionOwnerForTab + structuredAgentSessionTargetForHost instead of getRuntimeEnvironmentIdForWorktree + getActiveRuntimeTarget, matching the chat pane and StructuredAgentSessionStatusBridge.tsx:204-212. The subscribed body moves into StructuredAgentSessionOwnedAttention.
  • No owner means no subscription. A tab whose workspace id is ambiguous on an unselected workspace (or whose host no runtime serves) now holds no subscription, where it previously fell back to local. This is the same posture the status bridge and chat pane already take.
  • Tests. New it.each collision test (recorded paired tab stays on environment:env-1 and delivers with either workspace selected), a local-chat-under-paired-collision test, a rewritten unrecorded-tab test, and a mirrored-tab test pinning the host stamp on first publish.

Verified: both changed test files pass (26 tests), and oxlint is clean on the three changed files. The production change is a strict reuse of the shared owner rule, so the three readers now agree by construction.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 merged commit ad5aa01 into main Oct 5, 2026
55 checks passed
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