Repository navigation
fix(agent-status): converge the structured pane key and stateStartedAt (PR 2a) - #19786
brennanb2025 wants to merge 4 commits into
Conversation
a267639 to
56e5046
Compare
d65010e to
3bbd8d9
Compare
08f5918 to
9d90a71
Compare
3bbd8d9 to
0f2bc5d
Compare
9d90a71 to
9c9451e
Compare
Removing the IPC filter is only safe once main and the renderer agree on the structured pane key and on stateStartedAt. Both divergences are named with their live consumers, and PR 2 is renamed 2b to make the order explicit.
… row derivation
Both writers project the same host summary into a row, and their two
derivations disagreed in ways that only the IPC filter was hiding.
The pane key now comes from the session id alone. `tab.id` was the wrong
input: terminal-surfaces re-hosts a mirrored session at
`${baseId}:history-N` when its derived id is occupied, and the host never
sees that suffix, so the two writers keyed one session differently. That
key also carried a second `:`, which parsePaneKey rejects, so such a
session had no sidebar or dashboard row at all. Two surfaces can mirror
one session and now share one key, so the bridge's unmount cleanup only
clears the row when no other surface still mirrors it.
`stateStartedAt` moves to one shared rule, the host's: it marks the start
of the state the row is in, and republished evidence never moves it.
`done` is not an exception — agentEntryCompletionAt reads a settled row's
stateStartedAt as its completion time, so the renderer's reset re-dated a
finished turn on every republish. `hookWorkingEpoch` and the chat's
elapsed clock are unaffected (both read only `working` rows); Class 2
ordering for a settled session now follows when it finished.
No filter is removed and no writer is retired here.
9c9451e to
d95f3f1
Compare
📝 WalkthroughWalkthroughThe change makes structured agent session pane keys depend only on session IDs. The renderer no longer supplies Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The runtime changes are mergeable with a small documentation correction needed to accurately describe replacement-path identity behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 18 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description provides detailed technical rationale and testing results, but it omits most required template sections, including ELI5, What Changed, Linked Issue with a Fixes # entry, Visual Proof or an explicit N/A reason, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Resolution Update the description to include all required template headings. Add the linked issue using a Fixes # entry, provide visual proof or state N/A with a reason, complete the testing checkboxes with platform coverage, and complete the remaining checklist and review sections.
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 19d25a27-cbc1-4f41-aa18-497a68c4c37a
📒 Files selected for processing (13)
docs/reference/agent-status-store.mdsrc/main/agent-hooks/server-ingest-structured-status.test.tssrc/main/agent-hooks/server/server-ingest-structured.tssrc/main/agent-hooks/server/server-status-application.tssrc/main/runtime/runtime-worktree-agent-rows-structured.test.tssrc/main/runtime/structured-worker-identity.test.tssrc/renderer/src/components/native-chat/NativeChatStructuredSession.tsxsrc/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.test.tsxsrc/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsxsrc/shared/agent-status-state-start.test.tssrc/shared/agent-status-state-start.tssrc/shared/structured-agent-session-projection.test.tssrc/shared/structured-agent-session-projection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
|
|
||
| The PR that carries this document is PR 1a. Sections below are grouped under | ||
| the step that delivers them; PR 1a and PR 1b have landed. | ||
| the step that delivers them; PR 1a, PR 1b and PR 2a have landed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the document ownership statement.
Line 15 says PR 2a has landed. The preceding sentence still says that PR 1a carries this document. Make that sentence historical, or identify the current PR, so rollout documentation has one clear owner.
The PR objective identifies this change as PR #19786, PR 2a.
| observation: 'live' | 'unverifiable' | ||
| ): void { | ||
| const paneKey = structuredAgentSessionPaneKey(tab.id, tab.entityId) | ||
| const paneKey = structuredAgentSessionPaneKey(tab.entityId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx --items all
rg -n -C 5 '\bsetAgentStatus\s*\(|agentStatusByPaneKey|\.tabId\b|tabId:' \
src/main src/renderer/src src/sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bridge ---'
sed -n '110,215p' src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx
printf '%s\n' '--- status writers ---'
rg -n -C 8 'setAgentStatus|function getEntryTabId|const getEntryTabId' \
src/renderer/src --glob '!**/*.test.*'
printf '%s\n' '--- status entry declarations and production consumers ---'
rg -n -C 5 'AgentStatusEntry|agentStatusByPaneKey|entry\.tabId|getEntryTabId' \
src/renderer/src --glob '!**/*.test.*'Repository: stablyai/orca
Length of output: 50370
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 25970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bridge ---'
cat -n src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx | sed -n '120,215p'
printf '%s\n' '--- status writers ---'
rg -n -C 6 '\bsetAgentStatus\b' src/renderer/src --glob '!**/*.test.*' | head -n 240
printf '%s\n' '--- entry contract ---'
rg -n -C 8 'export (type|interface) AgentStatusEntry|function getEntryTabId|const getEntryTabId' \
src/shared src/renderer/src --glob '!**/*.test.*' | head -n 240
printf '%s\n' '--- production tabId consumers ---'
rg -n -C 5 'entry\.tabId|getEntryTabId\(' \
src/renderer/src --glob '!**/*.test.*' | head -n 320Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact bridge imports and update call ---'
rg -n -C 12 'setAgentStatus|structuredAgentSessionTabId|structuredAgentSessionPaneKey' \
src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx
printf '%s\n' '--- exact tab-id helper and callers ---'
rg -n -C 10 'getEntryTabId' src/renderer/src src/shared --glob '!**/*.test.*' | head -n 200
printf '%s\n' '--- shared entry fields ---'
rg -n -C 12 'AgentStatusEntry' src/shared/agent-status-types.tsRepository: stablyai/orca
Length of output: 20280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- structured-session identity ---'
rg -n -C 8 'structuredAgentSession(TabId|PaneKey)|getStructuredAgentSessionTabs' \
src/shared src/renderer/src --glob '!**/*.test.*' | head -n 260
printf '%s\n' '--- status entry identity fields ---'
sed -n '84,164p' src/shared/agent-status-types.ts
printf '%s\n' '--- setAgentStatus contract and implementation ---'
sed -n '70,100p' src/renderer/src/store/slices/agent-status-slice-contract.ts
rg -n -C 12 'metadata\.tabId|tabId:.*metadata|const entry|AgentStatusEntry =' \
src/renderer/src/store/slices --glob '!**/*.test.*' | head -n 260
printf '%s\n' '--- hibernation use of entry tab identity ---'
sed -n '74,115p' src/renderer/src/lib/agent-hibernation-planner.ts
sed -n '145,175p' src/renderer/src/lib/agent-hibernation-coordinator.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- structured-session identity definitions ---'
rg -l 'structuredAgentSessionTabId|structuredAgentSessionPaneKey' src/shared src/renderer/src --glob '!**/*.test.*' |
while read -r f; do
printf '\n### %s\n' "$f"
rg -n -C 10 'structuredAgentSessionTabId|structuredAgentSessionPaneKey|getStructuredAgentSessionTabs' "$f"
done
printf '%s\n' '--- tab binding consumers ---'
sed -n '105,145p' src/renderer/src/store/slices/agent-status-pane-key-tab-binding.ts
rg -n -C 6 'findTabForAgentEntry|getEligiblePane\(' \
src/renderer/src --glob '!**/*.test.*' | head -n 220Repository: stablyai/orca
Length of output: 20843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bridge test coverage and identity expectations ---'
rg -n -C 8 'StructuredAgentSessionStatusBridge|history-|tabId|setAgentStatus' \
src/renderer/src/components/native-chat --glob '*StructuredAgentSessionStatusBridge*' --glob '*test*'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- structured-tab construction ---'
sed -n '38,115p' src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx
printf '%s\n' '--- mirrored-surface tests and tab-id assertions ---'
rg -n -C 18 'history-|mirrored|structuredTab\.id|tabId:' \
src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.test.tsxRepository: stablyai/orca
Length of output: 7483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- metadata-to-entry binding ---'
rg -n -C 8 'tabId: metadata|metadata\?\.tabId|metadata\.tabId' \
src/renderer/src/store/slices/agent-status-live-entry-builder.ts \
src/renderer/src/store/slices/agent-status-live-facts.ts
printf '%s\n' '--- mirrored tests only ---'
rg -n -C 12 'history-|mirrored|structuredTab\.id|secondStructuredTab' \
src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.test.tsxRepository: stablyai/orca
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- setAgentStatus implementation binding ---'
rg -n -C 10 'setAgentStatus\s*:' src/renderer/src/store/slices --glob '*.ts' --glob '!**/*.test.*'
printf '%s\n' '--- tabId assignment in the bound live-entry builder ---'
rg -n -C 8 'tabId' src/renderer/src/store/slices/agent-status-live-entry-builder.tsRepository: stablyai/orca
Length of output: 6142
Use the canonical tabId for mirrored status rows. setAgentStatus derives entry.tabId from routing.tabId, and this bridge passes the surface-local tab.id. For a :history-N surface, tab-binding and hibernation consumers can therefore resolve the row to a non-existent tab. Use structuredAgentSessionTabId(tab.entityId) in the equality check and routing.tabId.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one rough edge in the mirrored-surface path, plus a forward-looking note on what PR 2b currently still inherits.
Reviewed changes
- Pane key now derived from the session id alone —
structuredAgentSessionPaneKey(sessionId)builds the tab id itself; main's key is unchanged, and the renderer's three call sites (projection write, unmount cleanup,NativeChatStructuredSession) stop passing the surfacetab.id. This fixes the:history-Nmirrored surface, whose old key disagreed with main's and was unparseable. - Unmount cleanup tolerates a mirrored pair — the bridge no longer removes the shared row while another surface still mirrors the same session.
- One shared
stateStartedAtrule — newresolveAgentStatusStateStartedAtis used by both the hook writer and the renderer; the renderer'sdesired.state !== 'done'restamp clause is removed so a settled row keeps its completion time. - Tests and docs — coverage for the pure rule, the disambiguated key, the mirrored-pair cleanup, and held completions, plus the
agent-status-store.mdupdate.
ℹ️ Main's structured row still omits the subagent projection
ingestStructuredStatus builds its payload from state, prompt, agent type, model and tool fields only (server-ingest-structured.ts:28-38), whereas the renderer's projectStatus also emits subagents/subagentObservation from summary.backgroundTasks (StructuredAgentSessionStatusBridge.tsx:144-158). So once the filter comes off and the renderer writer is retired in 2b, sidebar child rows would disappear unless main projects them too. The PR body scopes 2a to the pane key and the clock, so this is likely already tracked — flagging it only because 2b is described as a switch flip that these two changes make safe.
Technical details
# Main structured payload lacks subagent data
## Affected sites
- `src/main/agent-hooks/server/server-ingest-structured.ts:28-38` — payload has no `subagents`.
- `src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx:144-158` — renderer adds `subagents`/`subagentObservation` from `summary.backgroundTasks`.
## Required outcome
- Either main's structured ingest projects `summary.backgroundTasks` into the payload, or PR 2b keeps a renderer-side source for child rows. Confirm which before retiring the writer.
## Open questions for the human
- Is the subagent projection intentionally deferred to 2b, or did "the two derivations now agree" assume only the two named divergences?openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| observation: 'live' | 'unverifiable' | ||
| ): void { | ||
| const paneKey = structuredAgentSessionPaneKey(tab.id, tab.entityId) | ||
| const paneKey = structuredAgentSessionPaneKey(tab.entityId) |
There was a problem hiding this comment.
Now that the pane key is derived from the session id alone, the primary and :history-N surfaces share a single agentStatusByPaneKey entry — but the equality guard just below still requires current.tabId === tab.id (StructuredAgentSessionStatusBridge.tsx:174), which can never hold for the suffixed surface. So both surfaces re-run setAgentStatus on every summary update, and the stored row's tabId ends up owned by whichever wrote last (main writes structuredAgentSessionTabId(sessionId) for the same row at server-ingest-structured.ts:43). This is the one path PR 2a is converging, so it is worth settling now.
Technical details
# Residual `tabId` divergence defeats the equality short-circuit for mirrored surfaces
## Affected sites
- `src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx:174` — guard term `current.tabId === tab.id` cannot hold for a `:history-N` surface sharing the row.
- `src/renderer/src/components/native-chat/StructuredAgentSessionStatusBridge.tsx:202` — routing stores surface `tab.id`.
- `src/main/agent-hooks/server/server-ingest-structured.ts:43` — main stores `structuredAgentSessionTabId(sessionId)`.
## Required outcome
- The guard must be satisfiable by either surface sharing a session-derived key, and the row's `tabId` must be stable across both writers. Otherwise every streaming update emits two writes (freshness/telemetry churn) and the field oscillates.
## Suggested approach (optional)
- Persist the host-derived id in `desired`/routing and compare against it, or drop the surface-specific term from the guard if the row's `tabId` is genuinely surface-scoped (then document why it intentionally differs from main's).
## Open questions for the human
- Should a shared structured row's `tabId` follow the host-derived id (main's choice) or the visible surface? Generated-title targeting depends on the answer.… by its key The pane key's tab-id half is now derived from the session id, so it stops naming the surface whenever `terminal-surfaces.ts` reuses a tab: a conversation that replaces another keeps the superseded tab, whose local id still spells the old session while `entityId` becomes the new one. Two readers were using the key as surface routing and broke on that. `WorktreeCardAgents`'s row click matched the tab by exact id, found nothing, and returned silently — the row stayed and did nothing. It now falls back to the session the key encodes, via a new inverse of `structuredAgentSessionTabId`. The live-entry worktree index maps tab id to worktree, and a `done` row is bucketed only through it (a live row still has `entry.worktreeId`). A replaced conversation's settled row therefore left the sidebar entirely. The index now registers each agent-session tab under its derived session tab id too. `stateStartedAt` needed no shared module. The renderer store's own default already holds the state clock across republished evidence, and carries the Command Code new-turn clause besides; the divergence was that the bridge passed a `timing.stateStartedAt` that overrode it. The bridge now passes none, so the store applies its own rule, and `resolveAgentStatusStateStartedAt` is gone — it would have had one caller, and `server-reaping.ts` keeps its own inline copy regardless.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (d95f3f1), this run reviewed the new commit cf2ff66, which resolves a structured row's surface by session rather than by the pane key's tab-id half, and simplifies the stateStartedAt convergence.
- Reader-side surface resolution —
structuredAgentSessionIdFromTabIdinvertsstructuredAgentSessionTabId, andactivateStructuredAgentSessionForRowresolves a status row's surface by exact tab id first, then by the session id decoded from the pane key against a tab'sentityId.WorktreeCardAgents's row click now uses it, so a conversation that replaced another activates instead of silently doing nothing. - Live-entry worktree index —
worktree-agent-row-selectors.tsregisters each agent-session tab under its derived session tab id as well as its local id, so a settled (done) row for a replaced or mirrored conversation is bucketed instead of vanishing from the sidebar. The newreplaced-structured-session-agent-row.test.tsxcovers both this and the activation path against the real resolver. stateStartedAtconvergence simplified — the bridge stops passingtiming.stateStartedAt, letting the renderer store's own rule (agent-status-live-entry-builder.ts, which already carries Command Code's same-state new-turn clause) apply, and main keeps its inline rule. The earlier shared helper module is deleted; the two writers now agree without a shared call.- Tests — coverage for
structuredAgentSessionIdFromTabId,activateStructuredAgentSessionForRow(including a non-session tab id), the replaced-conversation activation anddone-row bucketing, plus updated bridge completion-clock and mirrored-pair expectations.
The prior pullfrog inline thread on the bridge's residual tabId routing (StructuredAgentSessionStatusBridge.tsx:173/:197) is unchanged by this commit and remains open; the PR body now explicitly scopes it to PR 2b, so it is not re-raised here.
openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…-key The key is derived from the session id; what the key is derived FROM is not. On the replacement path the renderer's local tab id still trails the old session, so "the tab id was the wrong input" was only half of it — the tab id is wrong because the renderer lets it go stale, and the host does not have the problem because `replaceConversationInSnapshot` re-keys and renames every reference. Records what the re-key probe measured so the split PR does not re-derive it: re-keying alone drops focus through `resolveWebSessionVisibleTabId` (which matches by id, then by entityId, and a replacement changes both), the existing `rekeyedTabIds` channel repairs that, `layoutByWorktree` is group-keyed so persisted layouts are untouched, and the re-key would remove the main source of `:history-N`.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f6da8e68-a46b-4049-912a-f3a5e8e2223d
📒 Files selected for processing (1)
docs/reference/agent-status-store.md
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| session, so the session id in the key and the session the surface actually hosts | ||
| disagree. So "the tab id was the wrong input" is only half the story — the tab id | ||
| is wrong because the renderer lets it go stale, and 2a converged the key without | ||
| converging that. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the replacement-path identity description.
These lines state that the session ID in the pane key disagrees with the session hosted by the surface. The renderer uses tab.entityId for the session lookup and pane-key cleanup; only the local tab.id remains stale after replacement. State that the local surface ID diverges from the session-derived key instead.
Suggested wording
- so the session id in the key and the session the surface actually hosts
- disagree.
+ so the surface's local tab id and the session-derived key can disagree;
+ the key still names the session identified by tab.entityId.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| session, so the session id in the key and the session the surface actually hosts | |
| disagree. So "the tab id was the wrong input" is only half the story — the tab id | |
| is wrong because the renderer lets it go stale, and 2a converged the key without | |
| converging that. | |
| session, so the surface's local tab id and the session-derived key can disagree; | |
| the key still names the session identified by tab.entityId. So "the tab id was | |
| the wrong input" is only half the story — the tab id is wrong because the | |
| renderer lets it go stale, and 2a converged the key without converging that. |
|
Closing as superseded. The agent-status unification lane was rebuilt as a new stack and this approach did not survive the redesign. What replaced it. Rather than converging the structured pane key, the current design demotes the pane key to routing and location only — it stops being identity at all. That lineage has since landed: #20530, #20531, #20664, #20716, #20717 and #20718, the last of which makes the canonical store live for structured sessions. Salvage. The behaviour worth keeping from this PR is recorded in the plan and lands with the reader cutover, not here. Its type definitions are superseded by the identity contract in #20531. The branch |
Why this exists
PR 1a left main and the renderer each deriving a structured status row from the same host summary, kept apart by an IPC filter. This PR is scaffolding for PR 2b, not an end state: it makes the two derivations agree so 2b can delete one of them. The goal is one writer per row — convergence is how we get there safely, not the destination. If 2b does not follow, this PR leaves the codebase with two writers that merely agree, which is worse than either endpoint.
Both divergences below were found while reviewing #19683.
Divergence 1 — the pane key
structuredAgentSessionPaneKey(structuredAgentSessionTabId(sessionId), sessionId)structuredAgentSessionPaneKey(tab.id, tab.entityId)tab.idis normally that same derived id — butweb-session-tabs-sync/terminal-surfaces.tsassigns${baseId}:history-${++suffix}when a mirrored session collides with an occupied id.parsePaneKeyrejects a key with two colons, so such a key was unparseable and dropped from every sidebar bucket. Removing the filter with the keys still divergent would instead yield two rows for one chat.An earlier revision said a
:history-Nsession "gets a row now". That overstates it: the suffix is only assigned when the base id is already occupied, and the occupant is normally the same session's other surface, which was already publishing the base key. The suffixed surface now shares that row rather than adding one. The bridge test for a disambiguated surface pins the key derivation, not a reachable end state — its fixture has nothing at the base id, whichbuildMirroredAgentTabsdoes not produce.Fix:
structuredAgentSessionPaneKeynow takes only the session id and derives the tab id internally, so surface identity cannot enter the key. All call sites are single-argument; divergence is structurally impossible rather than merely tested.But
tab.idis not simply "the wrong input", and this PR does not close that. The key is now derived from the session id; what the key is derived from is not. On the replacement path the renderer's local tab id still trails the old session —terminal-surfaces.ts:99reusesexisting?.id— so the session named by the key and the session the surface hosts disagree. The host has no such problem:replaceConversationInSnapshot(structured-conversation-tab-replacement.ts:18) re-keys its tab toagent-session:${sessionId}and renames every reference that held the old id. The renderer is the outlier, and/clearand/compactare what mint a replacement session id, so this is routine. 2a converges the key without converging what the key is derived from; the reader section below is the containment, and the re-key is tracked as its own prerequisite PR.Consequence, handled: two surfaces mirroring one session now share a key, so the bridge's unmount cleanup only clears the row when no other surface still mirrors that session.
Consequence found in review: two readers were using the key as surface routing
Because the key's tab-id half is now derived from the session, it stops naming the surface whenever
terminal-surfaces.tsreuses a tab — most sharply for a conversation that replaces another, where the reused tab's local id still spells the old session whileentityIdbecomes the new one. Both readers are fixed here:WorktreeCardAgents's row click matched the unified tab by exact id, found none, and returned silently: the row stayed and clicking it did nothing. It now falls back toactivateStructuredAgentSessionForRow, which reads the session id back out of the tab id (structuredAgentSessionIdFromTabId, the inverse ofstructuredAgentSessionTabId) and resolves the surface byentityId.worktree-agent-row-selectors.tsmaps tab id to worktree, and adonerow is bucketed only through that index (a live row still falls back toentry.worktreeId). A replaced conversation's settled row therefore left the sidebar entirely. The index now registers each agent-session tab under its derived session tab id as well as its local id.This is the invariant guard, not a stand-in for the re-key. It resolves from the pane key, never from
entry.tabId, so 2b making main the sole writer of that field does not decay it — and it covers the:history-Nsurface, which a re-key cannot, because the suffix exists precisely when the derived id is already taken.The re-key belongs in its own PR ahead of this one — measured, not guessed
Making the renderer's tab id follow the session, as the host's already does, is the durable fix. It was probed against
structured-conversation-tab-replacement.test.tsand theweb-session-tabs-syncsuites (28 files / 249 tests green at baseline) and then reverted; nothing in this PR touchesterminal-surfaces.ts,apply-preparation-groups.tsorapply-preparation-unified.ts. Findings are recorded indocs/reference/agent-status-store.mdso they are not re-derived:resolveWebSessionVisibleTabIdmatches the sticky visible tab by exact id, then falls back toentityId. A replacement changes both, so focus resolves to null and falls through to the first agent tab in snapshot order — in thehistory: 'before'variants that is the reopened archive, so/clearwould leave the user looking at the old conversation.apply-preparation-groups.ts'srekeyedTabIdsis documented for exactly this rename and is already fed by provisional-terminal and local-editor renames. Adding agent tabs restored correct focus in all six variants; 5 of 7 cases then pass, with anactiveTabIdByWorktreeresidue in the two agent-session × history-present variants.layoutByWorktreeis not at risk —TabGroupLayoutNodeholdsgroupId, never tab ids.:history-N, since the suffix exists because the replaced tab squats on the base id.It is a separate PR because it rewrites the
clear pane identitycontract — seven pinned cases spanning agent-session and the terminal→chat conversion, where the reused surface is a terminal holdingptyIdsByTabId/terminalLayoutsByTabId/runtimePaneTitlesByTabId.worktree-agent-row-fallback-tab.tsstill synthesizes the row's tab fromparsePaneKey, deliberately: preferringentry.tabIdwould break theparsed.tabId === tab.idinvariant that the click handler checks, turning a dead row into a dismissed one, and a host-originated entry has notabIdto prefer. The session id in the key is the identity that survives both collision paths, so the resolution happens at the activation seam.Divergence 2 —
stateStartedAtMain preserved
previous.stateStartedAtwhile the state was unchanged (includingdone); the renderer reset it tosummary.updatedAtondone.Mechanism revised after review — the outcome is unchanged. An earlier revision routed both writers through a new shared module,
src/shared/agent-status-state-start.ts. That module is gone. The renderer store's own default inagent-status-live-entry-builder.tsalready implements main's rule, and carries the Command Code new-turn clause besides, which it computes internally; the bridge was displacing it by passing atiming.stateStartedAt. The bridge now passes none, so the store applies its own rule and main keeps its inline expression. That is strictly more correct than the helper call, which could not supplynewTurnfrom the bridge's call site.Do not read this as "one shared rule for every writer":
server-reaping.tsholds another inline copy of the same shape that this PR does not touch.Blast radius, corrected. An earlier draft of this description claimed the chat's "Working for Ns" and its Stop-suppression epoch would change. They do not: both are read only while the row is
working, where the two rules already agreed. The real and only user-visible delta is sidebar/dashboard ordering — a settled structured session whose journal keeps appending now sorts by when it finished rather than by its latest journal write.Why main's rule:
agent-completion-time.tsreads a settled row'sstateStartedAtas its completion time, andcomponents/sidebar/smart-attention.tsdocuments that same-statedonewrites must not move the completion. Every hook agent already behaves that way; the structured bridge was the only writer re-dating a finished turn.Known gap this PR inherits and does not close
AgentSessionStatusSummarycarries no turn id, so neither writer can pass the shared rule'snewTurnflag for a structured session. On adone → doneacross two turns — a fast turn whereworkingis never observed — the row keeps turn 1's completion time for turn 2. The host always had this hole; the renderer's reset masked it by accident. Converging on the host's rule inherits the blind spot rather than fixing it.The durable fix is to carry the turn boundary and state clock on the wire summary, so the renderer copies instead of deriving. That is tracked separately — it would also make 2b a deletion rather than a migration.
Also noted, not fixed here: the row payload still carries surface fields (
tabId,worktreeId,label) on a key that is now deliberately surface-independent, so with two surfaces it is last-writer-wins for those fields. PR 2b removes the local-lane writer and resolves those fields from the tab list byentityId, which closes it for local sessions; a remote-mirror residue remains.Scope
No filter removed, no writer retired — that is 2b. This step only makes the two derivations agree.
Testing
tscfor node, cli and web. Focused suites plus the runtime aggregator green (524 files / 6042 tests across the sidebar, native-chat, agent-hooks and runtime suites; 1672 files / 17060 tests across shared, store, lib and dashboard).check:code-quality:changedclean.Every new test ablated: reverted its production change, confirmed failure, restored, re-verified green — including the both-changes-reverted arm for the two reader fixes. The one exception, stated as such: the
structuredAgentSessionIdFromTabIdunit test is a contract test for a new function and cannot fail without it; the regression pins for that path arereplaced-structured-session-agent-row.test.tsxand the activation-lib test.