Repository navigation
fix(agents): shared-workspace panes, and a second agent pane that deleted itself - #2394
Conversation
…ownership listing
A conversation of yours can live in a workspace ANOTHER drive member owns —
`decideAgentSessionAccess` grants by drive membership, not workspace ownership,
which is what makes workspaces shared working contexts. Opening one of those
from the past-conversations list rendered a working chat surrounded by dead
controls: the agent switcher and New Conversation disabled, and closing a
non-last chat pane a silent no-op.
THE ASYMMETRY. `AgentPanes` reaches a workspace through `checkSessionAccess`
(drive MEMBERSHIP — the nodes route's gate) and then read its conversation
directory from `GET /api/agent-workspaces`, where `ownerId` rides every filter.
A workspace owned by someone else is never in that listing, so
`sessionKnownToConversationsCache` never armed. Since such a workspace also
never appears in your sidebar, the history list is the ONLY route back to that
conversation — the broken path was the only path.
Two access models for one workspace, and the grid was reading the wrong one.
THE FIX is a deletion. `useWorkspaceLayoutSync` already reads the tree from the
membership-gated nodes route at mount and keeps it live on `session:<id>`, the
per-workspace room every member joins. `WorkspaceNodeTarget` already carries
`{agentPageId, lastMessageAt}` — a superset of what the deciders need — and the
redaction rule keeps both for a foreign private thread, masking only the title.
Both sets derive from the same rows. So the grid reads the tree, and the second
source goes, along with `recordMintedConversation`, `recordClosedConversation`
and their cache patches: the tree write IS the update.
MEMBERSHIP FROM `nodes`, FACTS FROM `targets`, and it has to be that way. A
structural `session:<id>` broadcast updates `nodes` and carries `targets`
forward unchanged (it cannot redact per viewer), so a thread another member just
placed is a member IMMEDIATELY with no agent known. Deriving membership from
`targets` would hide it from the switch decision, which would then read "no
thread for this agent" and mint a duplicate. An unresolved member is ABSENT from
the agent-keyed list, never padded with `agentPageId: null` — null is the Global
Assistant, a real agent, and a placeholder would focus the wrong thread.
READINESS IS TWO FACTS: the tree was read from the server AND no member is
missing its facts. `workspaces[id] !== undefined` cannot answer the first —
`runCommand` seeds a root locally at rev 0 — so `hasServerSnapshot` is exposed
on the tree view rather than letting components reach into `sync`.
MRU is preserved where it can decide anything: `findOpenForAgent` picks the most
recently active match, and tree `lastMessageAt` is only as fresh as the last
snapshot (the bump that keeps the listing live rides the OWNER's room, which a
member working in someone else's workspace never joins). With one match recency
picks nothing, so the switch stays instant; with two or more it is the whole
decision, so it re-reads first.
The sidebar still reads the ownership listing, correctly — it lists only
workspaces you own. Nothing changes about which sessions appear there, or about
whose sandbox a conversation uses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JLHPJBFNbCCcrL3F83PoBX
Master cut 1.7.1 while this branch was in flight, so `## [Unreleased]` became `## [1.7.1]` under the merge and this entry — which has not shipped — landed inside a released section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JLHPJBFNbCCcrL3F83PoBX
Opening a SECOND chat pane in a workspace — global assistant or page agent —
made the pane appear and then vanish, silently. Only the session's first
conversation survived. Pages were unaffected. Confirmed in production by the
`DELETE /api/agent-workspaces/{id}/conversations/{id}` that fires 200 moments
after the pane renders.
WHAT HAPPENS. A mint's own server write ADMITS what it created, and admitting
places: `admit` -> `place` fills exactly the pane the user picked into, because
"only an unbound pane may be filled ... is what makes the picker path land in
the very pane the user is looking at". The `session:<id>` broadcast then binds
that pane here, before the mint's POST has even resolved.
`stillMinting` asked whether the pane was still UNBOUND. So the SUCCESS case —
the server placed what we asked for, where we asked for it — became
indistinguishable from the abandonment case it was written to catch. Both
callers answer abandonment by destroying what they just made: the chat mint
DELETEs its conversation, the shell mint kills its shell. Both cleanups are
deliberately silent, which is why there was no toast and nothing in the console.
WHY NOW. `eceeb956f` ("one live client plane") wrote that check while the client
was the only thing that could bind a pane, and it was right then. `3c12c9e75`
("membership moves to the tree") made the server bind it too and never revisited
the check. The stale comment above it gives the game away — it justifies itself
by a PARKED state that the same epic later deleted.
THE FIX. The question is still the right one; `target === null` stopped being
the way to answer it. A pane carrying THIS MINT'S OWN target is still this
mint's pane, so both call sites now name what they produced (`conversationId`,
`shell.shellId`). Bound to anything else is still a genuine loss and still
cleans up — pinned by its own test, so the guard is corrected rather than
disabled.
Regression tests cover both directions and were mutation-checked against the
pre-fix rule: it fails the new "admission bound this pane first" test and only
that one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JLHPJBFNbCCcrL3F83PoBX
Review follow-up (codex) on the previous commit, verified before acting: the symptom is real, the mechanism reported for it was not. THE SYMPTOM. With two empty panes open, picking an agent filled the WRONG one and left the pane the user clicked blank. `open()`'s policy prefers `input.activeNodeId` and otherwise falls to `panes.find(canReplace)` — the first pane in grid order that qualifies (`workspace-node-commands.ts:627-628`). The mint carried no preference, so the server placed blind. NOT what the review said, and it matters because it changes the fix. `stillMinting` was reported to "search the entire tree" and so read a different-pane placement as success. It does not: `findNode(nodes, nodeId)` is a lookup BY ID, and the pane-scoped target match added in the previous commit is already exactly the "verify the node's id matches" the review asked for. What actually happens is the other branch — the user's pane is still UNBOUND, so the guard correctly says "still mine", and the misplacement is entirely upstream in the blind server placement. NOT a regression from the previous commit either. Pre-fix the guard read `node.target === null`, which is true in this same scenario, so it returned true then too. Identical before and after; this is a pre-existing gap the previous fix merely made visible by keeping the conversation alive long enough to see where it landed. THE FIX is the one the model already implies rather than a client-side compensating move: the caller says WHERE. `admit` has always forwarded `activeNodeId` to the placement policy — it was simply never given one — so this threads the picked pane from the client through both mint routes and `createConversationInSession` into `admitConversationNode`. A preference, not an instruction: `open()` still refuses a pane it may not give up, and an id naming nothing in the tree loses to the default. Compensating client-side instead would have reinstated the second writer this epic exists to delete. Pure tests pin both directions (fills the pointed-at pane; ignores a preference naming a pane it may not take) and were mutation-checked against dropping the forward in `place`. The client test asserts the mint carries the node id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JLHPJBFNbCCcrL3F83PoBX
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds server-backed workspace conversation membership, selected-pane placement during admission, and tree-based pane lifecycle coordination. It updates tests and changelog entries for pane persistence, replacement, switching, and cross-session conversation behavior. ChangesWorkspace conversation lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentPanes
participant ConversationRoute
participant Admission
participant WorkspaceTree
AgentPanes->>ConversationRoute: Create conversation with activeNodeId
ConversationRoute->>Admission: Forward selected-pane preference
Admission->>WorkspaceTree: Bind conversation to eligible pane
WorkspaceTree-->>AgentPanes: Update conversation membership
AgentPanes->>WorkspaceTree: Read readiness and placement state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca8da22da1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/lib/src/agent-workspaces/__tests__/workspace-membership.test.ts (1)
129-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider one more case:
activeNodeIdnaming no node in the tree.These two tests pin the honored preference and the bound-pane fall-through. The route comment in
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts(Line 217) also claims that an id naming nothing in this tree loses to the default. A stale client produces exactly that shape after its pane closes mid-mint. No test pins it.♻️ Proposed additional test
+ it('ignores a preference naming no node in this tree', () => { + const before = [root(), unbound('p1', 'root-1', 0)]; + + const next = applied( + before, + admit(before, { ...ids, target: { kind: 'chat', id: 'conv-a' }, activeNodeId: 'gone' }), + ); + + expect(memberNode(next, { kind: 'chat', id: 'conv-a' })?.id).toBe('p1'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/agent-workspaces/__tests__/workspace-membership.test.ts` around lines 129 - 158, Add a test in workspace-membership.test.ts covering admit with activeNodeId set to an ID absent from the pane tree, using otherwise eligible panes. Assert the admission falls back to the default eligible-pane selection rather than failing or honoring the stale ID, while preserving existing membership behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx`:
- Around line 685-705: Update the comments in the test case around the picker
click and request assertion to describe the current behavior: the selected pane
is propagated as activeNodeId, so the conversation lands in the user-picked
pane. Remove the outdated explanation about missing placement preference and
server fallback to the first unbound pane; leave the test logic and assertion
unchanged.
In `@apps/web/src/components/agents/panes/AgentPanes.tsx`:
- Around line 1120-1150: Protect the async switch callback around the refresh
await by using the existing per-node assignment token: start or capture the
invocation token before awaiting, then discard the invocation if a newer
assignment superseded it before selecting or closing conversations. Apply the
guard to the focus-existing path while preserving the mint delegation behavior,
and include beginPaneAssign in the callback dependencies.
- Around line 473-479: Update the readiness and refresh flow around
conversationDirectoryOf and the useEffect in AgentPanes so deleted or unreadable
targets are handled as terminally absent rather than remaining unresolved
indefinitely. Mark those targets resolved or otherwise exclude them from pending
hydration while preserving valid unresolved targets for refresh, and do not
represent absence with agentPageId: null because that means Global Assistant.
---
Nitpick comments:
In `@packages/lib/src/agent-workspaces/__tests__/workspace-membership.test.ts`:
- Around line 129-158: Add a test in workspace-membership.test.ts covering admit
with activeNodeId set to an ID absent from the pane tree, using otherwise
eligible panes. Assert the admission falls back to the default eligible-pane
selection rather than failing or honoring the stale ID, while preserving
existing membership behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2932cf77-8158-42e7-8c62-126759ece517
📒 Files selected for processing (13)
CHANGELOG.mdapps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsxapps/web/src/lib/agent-workspaces/agent-workspaces-runtime.tsapps/web/src/lib/agent-workspaces/claim-conversation-in-workspace.tsapps/web/src/lib/agent-workspaces/create-conversation-in-workspace.tsapps/web/src/stores/agent-workspace/__tests__/useAgentWorkspaceStore.test.tsapps/web/src/stores/agent-workspace/__tests__/workspace-tree-view.test.tsapps/web/src/stores/agent-workspace/useAgentWorkspaceStore.tsapps/web/src/stores/agent-workspace/workspace-tree-view.tspackages/lib/src/agent-workspaces/__tests__/workspace-membership.test.ts
…MRU re-read Four findings from Codex and CodeRabbit on #2394. Each verified against the code first; all four were real, and two are defects this PR introduced. 1. THE AWAIT WINDOW (P1, both reviewers). Adding the MRU re-read turned `handleSwitchAgent` from synchronous into suspending, and nothing invalidated an earlier invocation across it — so a pick the user had already replaced could resume and act on a decision computed two selections ago, taking the focus off what they actually chose. It now claims the node with the same `beginPaneAssign` token `handlePickAgent` next to it already uses, and bails if a newer pick landed. Regression-tested, and the test earns it: an earlier version of that test PASSED with the guard removed (the stale handler's cleanup no-ops because the node it captured is gone by then), so it was retargeted at the real harm — the focus jumping to the superseded agent's thread. 2 & 3. READINESS COULD NEVER COME BACK (P2 + Major). A missing target entry means "gone, or not readable by this viewer" exactly as often as "not fetched yet" — `readWorkspaceNodes` omits both, deliberately. Blocking on `hasUnresolved` alone therefore left the switcher and New Conversation dead for the rest of the session on a thread that was merely deleted, and the refresh dedupe (recorded BEFORE the read returned) did the same after one transient failure. The probe now records that it was ANSWERED, failures included, and readiness arms on that: once we have asked and been told, whatever is still missing is missing for good. Recovery does not lean on this effect retrying — the socket's reconnect re-reads, and a changed member set starts a fresh probe. The residual risk is bounded and preferred: a thread nobody here can identify fails to match and mints one extra conversation, which beats controls that never return. 4. A stale test comment that contradicted the assertion under it (minor). The old "disabled while unresolved" test is SPLIT rather than deleted — one pinning the pending window (the re-read held open), one pinning that readiness arms once it is answered. Both mutation-checked, as is the guard in 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JLHPJBFNbCCcrL3F83PoBX
|
All five threads addressed in 1. The await window (P1, flagged by both). Correct, and it was mine: adding the MRU re-read turned 2 + 3. Readiness could never come back (P2 + Major). Both correct and they share one cause: a missing target entry means "gone, or not readable by this viewer" exactly as often as "not fetched yet" — On the retry-vs-arm choice — I went with arm rather than retry-on-failure, because an unconditional retry loops on a persistent failure. The residual risk is bounded and I think preferable: a thread nobody here can identify fails to match and mints one extra conversation, versus controls that never return. 4. Stale test comment. Fixed. The old "disabled while unresolved" test is split rather than dropped — one pins the pending window (re-read held open), one pins that readiness arms once answered. Both mutation-checked, as is the guard in (1). Gates: typecheck 17/17 · lint 15/15 · knip within baseline · web 16686 passing · lib 0 test failures (remaining failures in both are the DB-dependent suites, which pass in CI). |
…ng it Self-review of the previous commit's P1 fix, mutation-verified in both directions: it closed the await window by CLAIMING the pane's assignment token at the top of `handleSwitchAgent`, and that claim is a write with a victim. WHAT THE CLAIM COSTS. `paneAssignTokens` is what an in-flight mint reads to decide it was superseded, and `handlePickAgent` answers supersession by DELETING the conversation it just created — silently. The rule the codebase already states at `decideClosePane` is therefore "only once a decision ACTUALLY commits to altering this node", and `handleSwitchAgent` cannot honour it with a claim taken on entry: it does not yet know whether it will act. `selectPaneAgent` can still answer `noop`, which is exactly what re-picking the agent a pane already shows returns. So: pane bound to Researcher, user switches to Writer (a mint, in flight — the pane is bound, so it keeps showing Researcher for the whole round trip), then re-picks Researcher. That second pick decides `noop` and changes nothing, but it has already invalidated the mint's token, so the mint resolves, reads itself superseded, and DELETEs Writer's brand-new conversation. The pane appears and vanishes with no toast and nothing in the console — defect 2 of this very PR, arriving through a narrower door. THE FIX. The guard needs to OBSERVE ownership across the await, not take it. `peekPaneAssign` reads the token without bumping it; the guard compares it before and after and bails if anyone claimed the pane meanwhile — a newer switch that committed, a History pick, a mint. A newer switch that decided `noop` is correctly invisible to it, having changed the pane no more than this one has. The claim stays where it belongs, in `handlePickAgent`'s mint branch. MUTATION-CHECKED BOTH WAYS, which is what makes this a correction rather than a loosening: - Restoring the claim fails the new "re-picking the agent a pane already shows" test, and ONLY that one. - Removing the peek guard fails the previous commit's "a superseding pick cancels the one waiting on the MRU re-read" test, and only that one — so the P1 window is still closed. Also moves `markAnswered` below `isSessionListingBody`'s doc block, which the previous commit split from the function it documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
Self-review finding: the P1 fix in
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/agents/panes/AgentPanes.tsx (1)
1215-1226: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle a failed MRU refresh before selecting a conversation.
refreshWorkspaceSnapshot(sessionId)can reject. The readiness probe handles this at Lines 537-543. At Line 1216, a rejection exitshandleSwitchAgentbefore it uses the stale candidate order or checks the pane token. Lines 1345-1347 discard the promise, so a transient failure prevents the switch and produces an unhandled rejection.Catch the refresh failure, retain
candidates, and then run the existing token check. Add a test for two matching conversations with a rejected refresh.Proposed fix
if (candidates.filter((candidate) => candidate.agentPageId === nextAgentPageId).length > 1) { - await useAgentWorkspaceStore.getState().refreshWorkspaceSnapshot(sessionId); + try { + await useAgentWorkspaceStore.getState().refreshWorkspaceSnapshot(sessionId); + } catch { + // Keep the current directory order when the refresh fails. + } const refreshed = conversationDirectoryOf(useAgentWorkspaceStore.getState().workspaces[sessionId]);Also applies to: 1345-1347
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/agents/panes/AgentPanes.tsx` around lines 1215 - 1226, Update handleSwitchAgent around refreshWorkspaceSnapshot so a rejected refresh is caught, candidates remains unchanged, and the existing peekPaneAssign token check still runs. Apply the same rejection handling to the promise-discarding refresh path around the related refresh call, preventing unhandled rejections. Add a test covering two matching conversations with a rejected refresh and verifying the switch proceeds using stale candidate ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/src/components/agents/panes/AgentPanes.tsx`:
- Around line 1215-1226: Update handleSwitchAgent around
refreshWorkspaceSnapshot so a rejected refresh is caught, candidates remains
unchanged, and the existing peekPaneAssign token check still runs. Apply the
same rejection handling to the promise-discarding refresh path around the
related refresh call, preventing unhandled rejections. Add a test covering two
matching conversations with a rejected refresh and verifying the switch proceeds
using stale candidate ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f32179a2-0f9c-4349-8ad3-1e0c61f57b54
📒 Files selected for processing (2)
apps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/agents/panes/tests/AgentPanes.test.tsx
Threading `activeNodeId` through the page-agents route wedged its declaration — and its own comment — between the session-binding comment block and the `workspaceIdFromBody`/`sessionId` parsing that block describes. The reader met "The contract PR drops the legacy key." followed immediately by "WHERE the caller wants it", then a declaration belonging to neither. Moved to where it is used, inside the `sessionId !== null` branch, which also puts it after the access check its "cannot address anything outside the workspace" claim depends on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
Review follow-up (CodeRabbit) on #2394, verified against the code first. The finding's premise does not hold, but the invariant it is worried about was unguarded, so this pins it rather than dismissing it. THE FINDING: `refreshWorkspaceSnapshot(sessionId)` can reject, so a transient failure would exit `handleSwitchAgent` before it uses the stale candidate order or checks the pane token — refusing the user's switch and producing an unhandled rejection out of the `void`-ed call. WHY IT DOES NOT HAPPEN TODAY: that function's entire body is inside one try/catch whose handler returns `false`, and it is typed `Promise<boolean>`. It reports failure as a value; it cannot reject. So the handler resumes normally, keeps the candidates it had, runs the token check, and decides — which is what the comment above it already promised ("a failed re-read is not a reason to refuse the switch") and what nothing was checking. Adding a try/catch around a call that cannot throw would be dead code. A test is the honest form of the same guarantee, and it makes the contract load-bearing: if `refreshWorkspaceSnapshot` is ever changed to throw, this goes red. Mutation-checked by making that catch rethrow — i.e. by making the finding's premise true. The new test fails and vitest reports an Unhandled Rejection, reproducing both harms exactly as described. Restored, it passes. The assertions are the two halves that matter: reaching the outgoing thread's DELETE proves the handler resumed past the failed re-read, and never POSTing proves it found the agent's thread in the stale list rather than concluding there was none and minting a duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
…ms it
Two findings from a review pass over the branch, both verified against the code
and both the same shape as defects this PR already fixes — one surface short.
1. THE SHELL MINT STILL PLACED BLIND. Defect 3 was fixed for chat by threading
the picked pane through both conversation routes, but `spawnShell` admits
exactly the way a conversation does and was never given one: `handlePickShell`
POSTed `{}`, the route parsed only `name`, and `admit` fell to "the first pane
that qualifies". With two empty panes the terminal opened in the one the user
did not click, and their own pane stayed empty — the identical symptom, on the
surface this PR already recognised as symmetric when it fixed the mint-cleanup
half for terminals. Threaded client → route → runtime.
2. THE `focus-existing` BRANCH NEVER CLAIMED THE PANE. The guard added in
`3ccc65b07` says a newer switch that COMMITTED is visible to it, and that was
aspirational: only the mint branch took the token, so a newer switch that landed
an EXISTING thread was invisible. Two switches that both take the MRU await, the
later one commits — and the earlier then resumes against an unchanged token and
acts on top of it, DELETEing the outgoing thread a second time and dragging the
console selection back to the agent the user had already replaced. It now claims
on the same rule `decideClosePane` states: once a decision actually commits to
altering this node. The `noop` path still takes nothing, so the correction in
`3ccc65b07` stands.
Mutation-checked: dropping the `focus-existing` claim fails the new "superseding
pick that lands an EXISTING thread" test and only that one; dropping the route's
forward fails the new shells-route test and only that one.
Coverage boundary, stated plainly: the route's forward and `admit`'s handling of
`activeNodeId` are each pinned by a test (the latter by the existing lib test);
the one-line `spawnShell` → `admit` glue between them is covered by typecheck
alone, there being no unit harness for that runtime.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
The third and last placement path a human drives, and the one the previous
commits left behind. Review finding, verified against the code first.
`3ccc65b07`'s predecessor added `activeNodeId` to `AdmitConversationInput` and
documented it as "the pane a HUMAN pointed at" — but the CLAIM path never
populated it, so the doc described behaviour no caller could produce. Reopening
a never-bound thread from a pane's History ADMITS it, and admitting places, so
it landed in whichever pane qualified first rather than the one whose History
the user opened.
The client cannot correct it afterwards, which is what makes this a real defect
rather than a cosmetic one: `openConversation(..., { activeNodeId })` runs after
the claim, and by then the thread is already showing somewhere — which that call
reads as "focus it there", not "move it here". The preference has to travel with
the write or it does not exist.
Threaded client → route → `claimConversationInSession` →
`claimConversationInSessionWith` → `admitConversation`, matching the two mint
routes and the shell spawn. The claim route now reads a body it never had; an
absent or unusable one is still a valid claim, since a preference must not
become the first way to fail one.
Mutation-checked: dropping the pure module's forward fails the new
`claimConversationInSessionWith` test and only that one; dropping the route's
fails the new route test and only that one.
Local `tsc` note for reviewers: this worktree's `.next/types/app/**` is missing,
so `bun run typecheck` reports TS6053 for generated route types and turbo's
green replays were cache hits. Verified instead by running `tsc --noEmit`
directly and confirming no non-TS6053 diagnostics — and CI's own Lint &
TypeScript Check is green on every pushed commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
…al did Review finding, verified against the code. A regression this PR introduced by deleting the grid's own subscription to the session listing. THE ASYMMETRY. `forgetWorkspaceInCache` strips this workspace's row from EVERY listing entry — it takes an `isWorkspaceListingKey` PREDICATE. The rollback snapshot beside it read ONE built key, `agentWorkspacesKey(driveId)`. While the grid subscribed to that key itself the two always agreed; once the subscription went, the read depends on some other surface having populated that exact key. This grid is handed the WORKSPACE's drive, and the listing may well have been fetched under another scope — the reused-global case `agentWorkspacesKey`'s own doc comment warns about — or under none. The snapshot then comes back null for a row that was nonetheless removed everywhere, and a failed DELETE leaves a workspace that is still very much alive missing from the sidebar, with only `mutate(isWorkspaceListingKey)` to bring it back — the revalidate `restoreWorkspaceInCache` exists precisely because it cannot be trusted to, since the network that failed the DELETE fails it too. The read is now the same scan the writers do. AND THE CACHE THEY EACH TALK TO. `mutate` was the bare top-level import, which targets SWR's DEFAULT cache whatever provider this tree is mounted under, while `cache` came from `useSWRConfig()`. Snapshot read from one, removal written to another. `forgetWorkspaceInCache`'s own doc asks callers for the mutate bound to their cache for exactly this reason; both now come from the same config. Identical in production, where there is one cache — but it is what makes the rollback coherent, and it is what lets a test observe it at all. Mutation-checked: restoring the single-key read fails the new test with the row dropped everywhere and restored nowhere. Two earlier versions of that test were DISCARDED for passing vacuously, which is why the fixture looks the way it does: SWR's predicate `mutate` only reaches keys SWR itself tracks, so a hand-seeded cache entry is never removed; and the rollback's own revalidate re-fetched the row back, hiding the difference. The listing is therefore subscribed by a real `useSWR` standing in for the sidebar, and goes offline after its first read — the same dead network that failed the DELETE. Gates: `tsc --noEmit` clean, `eslint` clean on the touched file (the five `useCallback`s that close over `mutate` now name it), full web unit suite 16696 passing with the same 10 failures across the same 16 DB-dependent files as the pre-change baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx (1)
1141-1148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the superseded first half of this comment.
Two comment blocks now state the same claim. The first block also says "the focus lands on its pane", which this test does not assert; it asserts the outgoing DELETE and the absence of a POST. Keep the second block only.
📝 Proposed comment fix
- // It still decides, on the ordering it already had: conv-3 is the most - // recently active of Researcher's two, so the focus lands on its pane. // IT STILL DECIDED, on the ordering it already had. Both halves matter: // reaching the outgoing thread's DELETE means the handler resumed past the // failed re-read and ran its token check rather than rejecting out of a // `void`-ed call; and never POSTing means it found Researcher's thread in // the stale candidate list instead of concluding there was none and minting // a duplicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx` around lines 1141 - 1148, In the test comment near the ordering assertion, remove the superseded first comment block describing focus landing on conv-3’s pane. Keep only the following block that explains the outgoing DELETE and absence of a POST, matching the behaviors actually asserted by the test.apps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/route.ts (1)
89-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
activeNodeIdnarrowing rule is written twice. Both routes repeat the same "object body, string value, non-empty, otherwise no preference" logic. Extract it once so a future third route cannot drift from the rule.
apps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/route.ts#L89-L91: replace the two-step extraction with a call to a sharedreadActiveNodeId(body)helper.apps/web/src/app/api/agent-workspaces/[workspaceId]/shells/route.ts#L152-L154: replace the identical extraction with the same helper call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/agent-workspaces/`[workspaceId]/conversations/[conversationId]/claim/route.ts around lines 89 - 91, Extract the duplicated activeNodeId narrowing logic into a shared readActiveNodeId(body) helper, preserving the object check, non-empty string validation, and null fallback. In apps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/route.ts:89-91 and apps/web/src/app/api/agent-workspaces/[workspaceId]/shells/route.ts:152-154, replace the two-step extraction with calls to that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/web/src/app/api/agent-workspaces/`[workspaceId]/conversations/[conversationId]/claim/route.ts:
- Around line 89-91: Extract the duplicated activeNodeId narrowing logic into a
shared readActiveNodeId(body) helper, preserving the object check, non-empty
string validation, and null fallback. In
apps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/route.ts:89-91
and apps/web/src/app/api/agent-workspaces/[workspaceId]/shells/route.ts:152-154,
replace the two-step extraction with calls to that helper.
In `@apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx`:
- Around line 1141-1148: In the test comment near the ordering assertion, remove
the superseded first comment block describing focus landing on conv-3’s pane.
Keep only the following block that explains the outgoing DELETE and absence of a
POST, matching the behaviors actually asserted by the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1ed0cea-9a95-407b-91df-7619959270af
📒 Files selected for processing (10)
apps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/__tests__/route.test.tsapps/web/src/app/api/agent-workspaces/[workspaceId]/conversations/[conversationId]/claim/route.tsapps/web/src/app/api/agent-workspaces/[workspaceId]/shells/__tests__/route.test.tsapps/web/src/app/api/agent-workspaces/[workspaceId]/shells/route.tsapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsxapps/web/src/lib/agent-workspaces/__tests__/claim-conversation-in-workspace.test.tsapps/web/src/lib/agent-workspaces/agent-workspaces-runtime.tsapps/web/src/lib/agent-workspaces/claim-conversation-in-workspace.tsapps/web/src/lib/agent-workspaces/workspace-shells-runtime.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/agents/panes/AgentPanes.tsx
The last of the review findings, and the one the previous commit's own doc
comment argued the other side of — correctly, for the case it was reasoning
about, and not for this one.
`refreshWorkspaceSnapshot` reports failure as `false` rather than throwing, and
that boolean is the only thing here that can tell "we asked, and these facts are
not ours to have" from "we never got to ask". Arming readiness on the first
settled read conflated them. The permanently-unreadable case is why arming has
to happen at all — a deleted thread never gets an entry however many times you
ask, and blocking on it left the switcher and New Conversation dead for the rest
of the session. But a DROPPED request answers nothing, and treating it as an
answer has a specific cost: one failed read at the moment another member's
thread lands arms readiness against a list missing that thread, and the switch
then reads "no thread for this agent" and mints a DUPLICATE of a conversation
sitting on the grid in front of the user.
One more ask, then arm either way. The point is to stop conflating the two
states, not to keep asking until the network agrees — the retry is bounded at
one, and everything the previous commit said about recovery still holds (the
socket's reconnect re-reads, a changed member set starts a fresh probe).
Mutation-checked: dropping the retry fails the new test with "expected spy to
not be called at all, but actually been called 1 times" — the duplicate mint,
exactly as described. An earlier version of the test passed under that mutation
and was rewritten: the MOUNT's own `/nodes` read was absorbing the failure, so
the probe's read always succeeded and the retry was never exercised. The fixture
now answers the mount read without the facts (which is what starts the probe)
and drops the probe's read specifically. Its `nodeShowingChat('conv-2')`
assertion went too — conv-2 is a node from the start, so it could never fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
Review pass over the branch: five findings, all verified, all closedI ran an independent review over the full branch diff and worked each finding against the code before acting. All five were real. Two were regressions this PR introduced; three were surfaces it left one step short of its own reasoning.
1 — terminals placed blind. The PR threaded 2 — 3 — the History claim. A claim admits, and admitting places. The client can't correct it afterwards: 4 — the rollback's reach. 5 — the dropped read. On the testsEvery fix is mutation-checked in both directions — break the source, watch the named test go red, restore. Three test versions were discarded for passing vacuously, and I'd rather record that than present a clean story:
I also dropped one fix I'd written for finding 2's neighbourhood: claiming the token on Gates
One environment note for anyone reproducing locally: this worktree's |
…tory too The blind-placement entry was written when only the agent picker carried a pane preference. Terminals and History reopens had the identical defect and now carry one as well, so this is its own note rather than a clause inside the vanishing- pane entry it never really belonged to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016XuyYoTEPucZHnBiZZNYCa
1 participant
Three fixes on the Agents pane surface. They are separate defects, but they share one root: the node-model cutover (
3c12c9e75, "membership moves to the tree") gave the server a job the client was still doing, and three places were never revisited.Two of them made a second agent pane unusable; the third made a shared workspace's pane half-dead.
1. The pane read its conversations from an ownership-filtered listing
A conversation of yours can live in a workspace another drive member owns —
decideAgentSessionAccessgrants by drive membership, not workspace ownership, which is what makes workspaces shared working contexts.AgentPanesreached such a workspace throughcheckSessionAccess(drive membership — the nodes route's gate) and then read its conversation directory fromGET /api/agent-workspaces, whereownerIdrides every filter. That workspace is never in that listing, so readiness never armed: the agent switcher and New Conversation stayed disabled and closing a non-last chat pane was a silent no-op.Two access models for one workspace, and the grid read the wrong one. Since such a workspace also never appears in your sidebar, the history list is the only route back to that conversation — so the broken path was the only path.
The fix is a deletion.
useWorkspaceLayoutSyncalready reads the tree from the membership-gated nodes route and keeps it live onsession:<id>, the per-workspace room every member joins.WorkspaceNodeTargetalready carries{agentPageId, lastMessageAt}, and the redaction rule preserves both for a foreign private thread, masking only the title. So the grid reads the tree, and the second source goes — withrecordMintedConversation,recordClosedConversationand their cache patches. The tree write is the update.Two constraints that shape it:
nodes, facts fromtargets. A structural broadcast updatesnodesand carriestargetsforward unchanged (it cannot redact per viewer), so a thread another member just placed is a member immediately with no agent known. Deriving membership fromtargetswould hide it from the switch decision, which would then mint a duplicate. An unresolved member is absent from the agent-keyed list, never padded withagentPageId: null— null is the Global Assistant, a real agent.workspaces[id] !== undefinedcannot answer the first (runCommandseeds a root locally at rev 0), sohasServerSnapshotis exposed on the tree view rather than letting components reach intosync.The sidebar still reads the ownership listing, correctly — it lists only workspaces you own. Nothing changes about which sessions appear there, or whose sandbox a conversation uses.
2. A mint's own admission looked like somebody else stealing its pane
Opening a second chat pane made it appear and then vanish, silently. Only the session's first conversation survived. Confirmed in production by a
DELETE /api/agent-workspaces/{id}/conversations/{id}returning 200 moments after the pane rendered.A mint's own server write admits what it created, and admitting places:
admit→placefills exactly the pane the user picked into. Thesession:<id>broadcast binds that pane before the mint's POST resolves.stillMintingasked whether the pane was still unbound — so the success case became indistinguishable from the abandonment case it was written to catch. Both callers answer abandonment by destroying what they just made: the chat mint DELETEs its conversation, the shell mint kills its shell. Both cleanups are deliberately silent, which is why there was no toast and nothing in the console.eceeb956fwrote that check while the client was the only thing that could bind a pane, and it was right then.3c12c9e75made the server bind too and never revisited it. The stale comment above it gives it away — it justifies itself by aparkedstate the same epic later deleted.A pane carrying this mint's own target is still this mint's pane, so both call sites now name what they produced. Bound to anything else is still a genuine loss and still cleans up — pinned by its own test, so the guard is corrected rather than disabled.
Terminals had the identical defect (
spawnShelladmits the same way); pages never did, having no server-side admit — which is exactly the asymmetry the report described.3. A mint placed blind when more than one pane was empty
With two empty panes, picking an agent filled the wrong one and left the pane you clicked blank.
open()prefersinput.activeNodeIdand otherwise falls topanes.find(canReplace)— the first qualifying pane in grid order. The mint carried no preference.admithas always forwardedactiveNodeIdto the placement policy; it was simply never given one. This threads the picked pane from the client through both mint routes intoadmitConversationNode. A preference, not an instruction:open()still refuses a pane it may not give up, and an id naming nothing in the tree loses to the default.Deliberately not solved by a client-side compensating move — that would reinstate the second writer this epic exists to delete.
4. The review round: five more, same family
Two later review passes (Codex, CodeRabbit, and a full-branch pass) turned up five findings. Each was verified against the code before acting; all five were real. Two were regressions this PR introduced, three were surfaces it left one step short of its own reasoning.
The blind placement was only fixed for chat.
spawnShelladmits exactly the way a conversation does and was never given a pane either, so a terminal opened in whichever pane qualified first — defect 3, unfixed on the surface this PR already treats as symmetric. The History claim had it too, and there the client cannot compensate:openConversationruns after the claim, by which time the thread is showing somewhere, which that call reads as "focus it there". The preference has to travel with the write. Both now thread it, client → route → runtime, like the two mint routes.The await guard had a victim, and then a blind spot. Closing the MRU window by claiming the pane's assignment token was wrong: the token is what an in-flight mint reads to decide it was superseded, and that mint answers supersession by DELETING what it just created. Since
handleSwitchAgentcan still decidenoop— re-picking the agent a pane already shows — a claim taken to find out destroys a conversation. It now peeks across the await and leaves the claiming to the branches that commit. Converselyfocus-existingwas committing without claiming, so a newer switch that landed an existing thread was invisible to the guard, and the earlier suspended one would act on top of it.The end-session rollback stopped reaching as far as its own removal.
forgetWorkspaceInCachestrips the row from every entry a predicate matches; the snapshot beside it read one built key. Those agreed only while the grid subscribed to that key itself — which §1 removed. A failed DELETE could drop a still-live workspace from the sidebar everywhere and restore it nowhere. (mutatewas also the bare top-level import, targeting SWR's default cache whilecachecame fromuseSWRConfig(); both now come from the same config.)A dropped probe read was being treated as an answer.
refreshWorkspaceSnapshotreports failure asfalse, and arming readiness on the first settled read conflated "asked, and these facts are not ours to have" with "never got to ask" — one dropped request as another member's thread lands would mint a duplicate of a conversation sitting on the grid. One more ask, then arm either way.Testing
Every new test mutation-checked (break the source, watch the named test go red, restore). Notably:
stillMintingrule fails the new "admission bound this pane first" test and only that one — so the guard is corrected, not disabled.place'sactiveNodeIdforward fails the "fills the pane the caller pointed at" test.Every fix above is mutation-checked in both directions. Worth naming, because the alternative is a clean story that is not true: three test versions were discarded for passing vacuously — one hand-seeded an SWR cache entry that SWR's predicate
mutatenever reaches (so nothing was removed and nothing needed restoring); its replacement was rescued by the rollback's own revalidate re-fetching the row; and the probe-retry test let the mount's/nodesread absorb the failure, so the retry was never exercised. One fix was also dropped rather than kept unpinned — claiming the token onfocus-existinglooked right, but went back in only once I had an observable that could fail without it.Gates:
eslintclean on every touched file ·tsc --noEmitclean ·@pagespace/lib0 test failures (13 DB-only files) ·web16696 passing, 10 failures across the same 16 DB-dependent files as the pre-change baseline (ECONNREFUSED …:5432, no test Postgres). CI's own Lint & TypeScript Check and Unit Tests are green on every pushed commit.Reviewer notes
DELETE; the fixes themselves are covered by tests only. Worth a manual pass: open a second and third agent pane; pick an agent with two empty panes on screen; open a terminal the same way; and reopen a past conversation from a pane's History with more than one pane empty.workspace-tree-view.ts): the listing filteredconversations.isActive; target resolution does not. A pane pointing at a history-deleted conversation therefore becomes evictable where it was previously protected. I judged that an improvement — a dead pane should be reusable — but it is a deliberate change, not an oversight.decideClosePane's listing guard could go away entirely if "the node IS membership" is trusted; and an ended workspace whose tree destroy failed keeps aworkspaceIdthe listing excludes and the sidebar cannot show, leaving it permanently un-endable.🤖 Generated with Claude Code
Summary by CodeRabbit