Skip to content

fix(agents): close a conversation's listing when its last pane closes (session → conversation → panes) - #2277

Merged
2witstudios merged 19 commits into
masterfrom
pu/conversation-close
Jul 30, 2026
Merged

2witstudios merged 19 commits into
masterfrom
pu/conversation-close

Conversation

@2witstudios

@2witstudios 2witstudios commented Jul 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

Restores the missing level in the pane grid's model: session → conversation (agent listing) → panes. Previously a session's pane grid was keyed per-session only, so closing the last pane bound to one agent's conversation was a pure layout act — the conversation's row stayed in the sidebar forever (holding a conversation-cap slot) unless the grid's actual last pane was closed, which ended the whole session instead.

  • packages/db/schema/conversations.ts: additive nullable closedInSessionAt timestamp — the deliberate un-conflation of isActive's double duty. isActive keeps gating history soft-delete; closedInSessionAt gates session-listing membership. Closing a listing never touches history.
  • apps/web/src/lib/agent-sessions/close-conversation-in-session.ts: pure decision module — outcomes closed | already_closed | not_in_session | last_conversation, fail-closed on the never-empty-session invariant. Wired in agent-sessions-runtime.ts with a per-session transaction + advisory lock (the fix(agent-sessions): CAS lifecycle stamps, atomic spawn ceiling, route error-mapping #2272 createIfUnderLimit pattern), so two racing closes of a session's last two listings serialize instead of both succeeding.
  • New DELETE /api/agent-sessions/[sessionId]/conversations/[conversationId] route — session-scoped, uniform for page-agent and assistant threads alike (mirrors the sibling POST route's access layering). Auto-covered by the security-audit-coverage route scan (calls auditRequest/sessionNotFoundOrDenied).
  • apps/web/src/components/agents/panes/close-pane.ts: pure client verdict (noop | close-pane | close-conversation | rebind-pane | end-session) covering duplicate-view panes, an unloaded/stale listing (never act on unverified state), a grid-last rebind to the most recently active OTHER open conversation, and ending the session only when its LAST open listing closes — even with terminal panes still in the grid.
  • AgentPanes.tsx wired to the verdict: silent DELETE on close-conversation (no dialog — history is untouched by design), 409 last_conversation falls back to the existing EndSessionDialog, and confirmEndSession now uses forgetWorkspace (not closePane) since ending can now fire with other panes still in the grid. cleanupOrphanedConversation switched from the history-deleting page-agent/global routes to this new session-scoped one — fixes a pre-existing defect where an orphaned mid-mint conversation stayed listed and held a cap slot forever.
  • AgentsSurface.tsx / AgentPageView.tsx each follow a close's rebind for their own independent "current conversation" tracking (URL selection vs. the Chat-tab's current), per their different recovery needs — AgentsSurface follows an existing next conversation; AgentPageView mints a fresh replacement for its own agent (reusing its existing History-delete recovery path), since there's no cross-agent "next" to fall back to on a single-agent page.

Fixed post-review: the non-chat-pane branch could end a session that still had other open conversations

An adversarial review caught a real gap: decideClosePane's branch for picker/terminal/mid-mint panes unconditionally returned end-session when it was the grid's last pane, without checking whether the session had ANOTHER open conversation listing with no pane in this grid (e.g. a background worker minted one, or a different tab/session-view never opened a pane for it here). Concretely: session has conversations A and B open; the grid narrows to just a terminal pane (A's chat pane already closed cleanly via the normal path); closing that lone terminal used to tear down the whole session even though B was still genuinely open. Added a new rebind-pane verdict: a grid-last non-chat pane with another open listing elsewhere now repoints at it (closing whatever was live there, e.g. killing the terminal's shell, exactly as an ordinary close would) instead of ending the session. Covered by new tests in close-pane.test.ts and one integration test in AgentPanes.test.tsx.

Rebased on master post-#2276 merge — dedupe done

#2276 (pu/pane-agent-selector) merged while this PR was in flight, adding an AISelector pane-bar identity, a select-pane-agent.ts pure module (decides pane-agent switch), and its own sessions-listing SWR fetch on the same /api/agent-sessions key this PR reads. Rebased pu/conversation-close onto the new master tip and hand-merged AgentPanes.tsx:

  • One shared SWR fetch for the session's conversation listing (sessionConversationsFetcher), read by both selectPaneAgent (switch decision) and decideClosePane (close decision) — not two competing fetches.
  • SessionConversationSummary hoisted to a new apps/web/src/components/agents/panes/session-conversations.ts; both select-pane-agent.ts and close-pane.ts now export type { SessionConversationSummary } from './session-conversations' instead of each declaring their own copy of the same shape.
  • Verified composition is safe: a close while the selector is mid-switch can only be racing an in-flight mint (the "focus" switch path is synchronous, no window to race) — and a switch-triggered mint goes through the exact same assignPane(..., {targetId: null, ...}) → await POST → paneStillExists check path as picking an agent from the split picker, which the existing "a pane closed mid-mint does not resurrect" test already covers. No new race introduced.

Fixed post-Codex-review (3 findings, all addressed)

  • P1 — close-pane.ts's grid-last fallback treated a never-resolved (null) session-conversation listing the same as a confirmed-empty one, so a slow/failed fetch could offer to end a session that actually still had other open conversations. Now null → noop (retry once the listing is known); only a confirmed-empty array ends the session.
  • P2 — AgentPageView.tsx and AgentsSurface.tsx each had a stale-closure race: a slow-resolving conversation-close DELETE's callback could fire after the user picked a different conversation, and — because it closed over the PRE-switch selection — would overwrite the user's newer pick with a stale rebind target. Both now read the latest selection through a ref kept in sync via an effect, and each has a regression test that captures the callback early, switches selection, then fires the stale reference to prove the fix.

All three Codex threads have been replied to with what changed and why (left OPEN for reviewer verification, not self-resolved, since I fixed them during this pass rather than before it).

Round 2 (commit 03d5918af) — 3 more Codex findings + a /simplify pass

  • P1 — the agent tool layer (session-tools-runtime.ts / session-tools.ts: list_sessions, send_session, kill_session) still queried workers by isActive alone, ignoring the new closedInSessionAt — a listing the human closed in the UI stayed fully addressable to tools. Added isNull(conversations.closedInSessionAt) to listSessionWorkers and countSessionConversations, and a new isClosed field on SessionToolRow that openOwnSession now refuses on, same as a foreign workspace.
  • P2 — AgentPanes.tsx: a slow close DELETE could resolve after the user reassigned the exact same pane slot (switched its agent, minted a new conversation into it), and the old paneStillExists check would still apply the stale rebind/close to it. Added paneStillShows(paneId, conversationId) — checks the pane's LIVE scope, not just that the pane id still exists.
  • P2 — AgentPageView.tsx: a second, deeper async gap in mintReplacementForCurrent — even after the round-1 stale-closure fix, the mint's OWN newConversation() await could resolve after the user picked something else, and would still unconditionally apply the replacement. newConversation now takes applyOverride (default true; only the mint path opts out) and re-checks currentRef.current against the stale id post-await before applying it — the grid pane rebind still happens unconditionally either way.

All three replied to (left OPEN, not self-resolved — fixed during this pass, same rule as round 1).

Also ran a 4-lens /simplify pass (reuse, simplification, efficiency, altitude) over the full branch diff and applied every valid finding: hoisted the duplicated mostRecentlyActive reduce (was reimplemented in both close-pane.ts and select-pane-agent.ts) into session-conversations.ts; close-conversation decisions now carry rebindAgentPageId directly instead of AgentPanes re-deriving it; extracted notThisPanesConversation/beginEndSessionConfirm helpers to remove repeated branches; replaced two hand-rolled ref-mirror useRef+useEffect pairs with a shared useLatestRef hook. Two findings were deliberately skipped as false positives (documented in commit message): parallelizing close-conversation-in-session.ts's two initial reads would break the idempotent already-closed short-circuit's test guarantee, and narrowing an event-handler's parameter type would lose useful documentation for no real gain.

Round 3 (commits f4b686091, 5c69f36ac, 142e61ed2) — an independent adversarial review, 2 CodeRabbit findings, and a fresh Codex pass (4 findings, 2 fixed + 2 documented as follow-ups)

An independent background review agent (asked to adversarially re-check round 2's own diff) caught a real regression the round-2 stale-closure fix introduced:

  • AgentPageView.tsx — mintReplacementForCurrent captured sessionId from the CURRENT conversation before the mint's own await, then reused that same stale outer variable in the final setOverride(...) instead of created.sessionId (the mint's actual result). Only diverges when the replaced conversation was session-less and the mint spawned a brand-new session for it (canUseSessions) — the override then stored sessionId: null even though the backend had bound the replacement to a real, live session, silently falling back to plain chat instead of the pane grid. Fixed by reading created.sessionId; regression test proves it (fails without the fix, stuck on plain-chat).

CodeRabbit's own pass on round 2 found two more, both fixed:

  • AgentPageView.tsx — handleConversationClosed always minted a fresh replacement even when the grid had already rebound to another OPEN listing of this SAME agent, leaving a redundant empty conversation behind every time. Now follows next when nextAgentPageId matches this page's own agent.
  • AgentPanes.tsx — confirmEndSession never revalidated the sessions-listing SWR key (unlike the sibling closeConversationListing path), so a just-ended session's sidebar row lingered until the next 20s poll instead of leaving instantly.

A fresh Codex review (triggered on round 2's commit) found 4 more; 2 fixed, 2 documented as out-of-scope follow-ups rather than guessed at:

  • Fixed (P1) — handlePickAgent's successful mint never revalidated the sessions-listing cache, so closing that exact pane before the next 20s poll read the brand-new row as "not in the open listing" and took the pure layout-close path instead of DELETE, orphaning the conversation forever (holding a cap slot). Added the same cache-revalidation call used elsewhere.
  • Fixed (P2) — handlePickAgent/handlePickShell guarded their in-flight completion with mere pane-existence, not pane STATE — a grid-last close could rebind that exact pane to a different open conversation while the mint/shell-open was still in flight, and the completion would clobber the rebind with its own abandoned result. New paneStillLoading check (same class of fix as paneStillShows from round 2) fixes both call sites; paneStillExists had no other callers and is removed.
  • Documented, not fixed (P1) — reopening a conversation from History never clears closedInSessionAt (the schema doc's own promised "reopening is just clearing the stamp" was never implemented). Real gap, but implementing it needs product decisions outside this PR's scope (cap re-enforcement on reopen? automatic-on-select vs. an explicit action?) — replied on the thread with the reasoning, tracking as a follow-up.
  • Documented, not fixed (P2) — a rebind target computed client-side can go stale under a genuine cross-tab race (another tab closes the chosen target while this tab's own close DELETE is in flight). The robust fix is an API contract change (the close endpoint atomically returning a still-open replacement), not a client patch — same reasoning, tracked as a follow-up.

All threads from every round replied to and left OPEN (not self-resolved — fixed during this pass, not before it).

Round 4 (commit 142e61ed2) — a further Codex pass on round 3's own commit found 4 more; 2 fixed, 2 documented as follow-ups

  • Fixed (P1) — handlePickAgent's successful mint never revalidated the sessions-listing cache, so closing that exact pane before the next 20s poll read the brand-new row as absent from activeConversations and took the pure layout-close path instead of DELETE, orphaning the conversation forever (holding a cap slot). Added the same void mutate(isAgentSessionsKey) call already used by every other listing-mutating action in this file.
  • Fixed (P2) — handlePickAgent/handlePickShell guarded their in-flight completion with mere pane-EXISTENCE, not pane STATE — a grid-last close could rebind that exact pane to a different open conversation while the mint/shell-open was still in flight, and the completion would clobber the rebind with its own abandoned result. New paneStillLoading(paneId, scope) (same class of fix as paneStillShows from round 2, but for the loading state — same kind, targetId: null, same agentPageId) fixes both success-path call sites; paneStillExists had no other callers and is removed.
  • Documented, not fixed (P1) — reopening a conversation from History never clears closedInSessionAt (the schema doc's own promised "reopening is just clearing the stamp" was never implemented). Real gap, but implementing it needs product decisions outside this PR's scope (cap re-enforcement on reopen? automatic-on-select vs. an explicit action?) — replied on the thread with the reasoning, tracking as a follow-up.
  • Documented, not fixed (P2) — a rebind target computed client-side can go stale under a genuine cross-tab race (another tab closes the chosen target while this tab's own close DELETE is in flight). The robust fix is an API contract change (the close endpoint atomically returning a still-open replacement), not a client patch — same reasoning, tracked as a follow-up.

Round 4b (commit f05a2d13a, renumbered to 5e13d9c41 by the master rebase below) — Codex re-reviewed round 4's own fix and found the guard was incomplete

  • Fixed (P2) — the new paneStillLoading guard from round 4 only covered the SUCCESS path of handlePickAgent/handlePickShell (the resolved-mint case); the CATCH block of both still called resetPane unconditionally, so a REJECTED mint/shell-open could still clobber a pane a grid-last close had already rebound elsewhere while the request was in flight. Both catch blocks now check paneStillLoading before resetting. Added a regression test for the rejected-mint case (the resolved-mint case was already covered from round 4).
  • Fixed (P2) — useLatestRef mutated .current inside a useEffect, which only flushes after commit — leaving a real window right after a render where .current still held the previous value, wide enough for a same-tick completion callback to observe stale state (the exact race the hook exists to prevent). Now mutates ref.current = value directly during render instead.

Both threads replied to and left OPEN for reviewer verification.

Rebased onto master again post-#2275/#2278 merges

Master advanced past this branch a second time (#2275 canvas-follow-up, #2278 pane-agent-selector-followup, #2257 form-notification-emails, #2252 canvas-links) while round 4b was in flight, putting the PR into conflict. Rebased onto the new master tip (65154e875) and hand-resolved:

  • AgentPanes.tsx (twice, across two different commits in the stack) — master's #2278 independently added its own local mint-tracking (recordMintedConversation, keyed on cache presence rather than isLoading) in the same region this PR's closeDecisionListing null-safety touches; combined both, since each is used by different call sites later in the file. Verified via full test/typecheck/lint after resolving, not just a mechanical accept-both.
  • packages/db/drizzle/ — master's #2278 claimed migration number 0241 first (0241_tranquil_wallflower); this branch's own closedInSessionAt migration collided on the same number. Dropped this branch's 0241_futuristic_silver_samurai.sql and regenerated via bun run db:generate as 0242_uneven_sphinx.sql (identical single-column ALTER TABLE — only the number changed).
  • AgentPanes.test.tsx — master's #2278 added its own scoped mockSessionConversations helper inside the selector describe block; this branch had already hoisted an identical helper to file scope earlier (dedup from the round-2 /simplify pass), so the local duplicate was dropped in favor of the existing top-level one, while master's new findEnabledSelector helper (unrelated infrastructure) was kept.

Confirmed mergeable: MERGEABLE / no conflicts after the rebase and force-push. Full gate suite (typecheck, lint, targeted test suites for every touched file) re-run locally post-rebase; packages/db's own DB-role test failure is a pre-existing local-env issue (missing Postgres role), unrelated to this branch.

An independent adversarial review agent specifically re-checked the hand-merged conflict resolution in AgentPanes.tsx (not just trusting the commit message) — confirmed correct and complete (all 4 paneStillLoading call sites present, recordMintedConversation deps intact, zero conflict markers, migration chain consistent). It flagged one inline comment as slightly overstating that the local mint-tracking update and the broader SWR revalidate touch entirely non-overlapping cache keys (they harmlessly overlap on this component's own key too) — corrected the wording (commit 69e7ebe6f), no behavior change.

Round 5 (commit f5aae6409) — a fresh Codex pass on the rebased commit found 2 more, both fixed

  • Fixed (P1) — closeDecisionListing was gated on sessionsData being truthy, not on THIS session's own entry having appeared (sessionKnownToConversationsCache). A cache already warm from ANOTHER session in the same drive answers with real (truthy) data that simply has no row yet for a brand-new session — that read as a confirmed-empty listing and wrongly offered to end the session on its very first pane close. Gated on sessionKnownToConversationsCache instead.
  • Fixed (P2) — mintReplacementForCurrent inferred "what was deleted" from currentRef.current instead of accepting the id useConversations already confirmed deleted (matched against currentConversationId at click time). If the user switched to a different thread while the History DELETE was in flight, the callback fired using the NEW current's session, minting a replacement into the wrong conversation's session. Now accepts deletedConversationId directly and bails if current has since moved on — the same guard shape handleConversationClosed already used for the session-grid-close path.

Both threads replied to and left OPEN. Also updated 4 pre-existing "closing the LAST pane" tests whose fixtures never mocked this session's own sessions-listing entry — harmless under the old (buggy) check, correctly exposed as unrealistic once the readiness check became accurate.

Round 6 (commit 846387d03) — another Codex pass found 2 more, both fixed

  • Fixed (P2) — closeConversationListing called onConversationClosed unconditionally, even when paneStillShows(paneId, conversationId) already proved this exact pane was reassigned (its own agent selector, or a fresh mint) while the close DELETE was in flight. The host (AgentPageView/AgentsSurface) tracks its own "current" independently of any specific pane, so the unconditional callback could still tell it to recover from the now-irrelevant closed conversation, overwriting what the user just picked for that pane. Moved the callback inside the paneStillShows guard.
  • Fixed (P2) — AgentsSurface's handleConversationClosed only handled next !== null (grid-last close, rebind target given). An ORDINARY close (the grid still has other panes) always reports next: null too, since rebinding only applies to an emptied grid — but the surface's own selection is just as stale in that case, still naming a conversation with no listing left; a refresh or deep link back to that URL would reopen it and silently replace whatever pane is actually live. Now retargets to another OPEN chat pane still in the session's workspace when one exists; leaves the selection as-is if none remains.

Round 7 (commit 09d17d1ff) — yet another Codex pass found 2 more, both fixed

  • Fixed (P1) — decideClosePane: a non-grid-last chat pane whose conversation wasn't found in activeConversations conflated "resolved and confirmed not open" (nothing left to DELETE) with genuinely UNKNOWN (activeConversations === null) — the latter silently skipped the DELETE via a pure layout close, leaving the listing open server-side with no pane left to retry from. Now noops for the null case regardless of pane count, matching gridLastFallback's existing discipline. Also corrected an existing test whose title said "should never act on the unverified state" but whose assertion expected the old, acting-on-it behavior.
  • Fixed (P2) — mintReplacementForCurrent's detached async IIFE had no catch; only the REPLACEMENT mint (not the already-succeeded close) could fail, producing an unhandled rejection and leaving current pointing at a gone conversation. Now catches, logs, and reports via toast, same as this file's other failed-IO catches.

All 4 threads (round 6 + 7) replied to and left OPEN for reviewer verification.

Round 8 (commit 5a5d00921) — CodeRabbit's own pass on round 7 found 1 more (the other was already fixed by round 7 itself)

  • Fixed (Minor) — the close-conversation route returned 502 for a thrown closeConversationInSession failure. closeConversationInSession is a pure DB transaction (advisory lock + SELECT/UPDATE), no sandbox/machine layer involved — unlike the sibling POST (mints into a sandbox) or the shell/file routes where 502 correctly signals an upstream failure. Changed to 500, matching this route family's own convention (route.ts's list/load handlers already use 500 for their pure-DB paths). Updated the corresponding test assertion.
  • CodeRabbit's second finding (the mintReplacementForCurrent unhandled rejection) was already fixed by round 7's own commit before this review ran against it — its own bot noted "✅ Addressed in commit 09d17d1" — no additional change needed, replied confirming.

Both threads replied to.

Round 9 (commit adbce0508) — yet another Codex pass found 2 more, both fixed

  • Fixed (P2) — round 6's AgentsSurface retarget fix only covered "another chat pane exists." When the grid's remaining panes are all terminals/pickers, or the session's other open listing has no pane here at all (a background worker minted one), the selection was left stale — a refresh or deep link would reopen the closed conversation and could hide a still-open listing. Now clears the conversation selection (keeping the session selected) when no chat replacement exists. selectConversation's conversationId param is now nullable to support this (an already-valid URL/store state, previously only reachable via selectSession).
  • Fixed (P2) — the never-empty guard in close-conversation-in-session.ts assumed the conversation being closed always occupies a slot in countOpenConversations's count. A history-deleted target (isActive: false) is excluded from that count by definition, so it never occupied a slot to begin with — closing its (already-gone) listing was misread as "would empty the session" when one unrelated ACTIVE listing remained, spuriously returning last_conversation (409). findConversation now also returns isActive; a history-deleted target short-circuits to already_closed before the guard runs.

Both threads replied to and left OPEN for reviewer verification.

Round 10 (commit 78401bf9c) — yet another Codex pass found 2 more, both P1, both fixed

  • Fixed (P1) — a chat pane whose OWN conversation was confirmed (per the client's resolved SWR snapshot) to be the session's last open listing short-circuited straight to end-session, skipping the server's own never-empty guard entirely. If that snapshot was stale (another tab or a background worker created a new listing since the last poll), confirming the dialog destroyed the WHOLE session — including the hidden, genuinely-live listing — with the server never given a chance to say otherwise. Now always attempts close-conversation first; only the server's authoritative 409 last_conversation reaches the confirm dialog, via the exact same beginEndSessionConfirm path the non-chat-pane case already used (that path — a terminal/picker with no conversation of its own to scope a close to — is unchanged, since there's no better data available there).
  • Fixed (P1) — mintReplacementForCurrent's successful mint never revalidated the sessions-listing cache, unlike handlePickAgent's identical mint path. Closing the replacement pane before the next 20s poll would read the brand-new row as absent and take the pure layout-close path instead of DELETE, leaving it open server-side forever. Hoisted isAgentSessionsKey (previously private to AgentPanes.tsx) to the shared session-conversations.ts and added the same revalidate call here.

Updated existing tests that encoded the old direct-end-session behavior to mock the server's 409 response instead. Both threads replied to and left OPEN for reviewer verification.

Round 11 (commit af1198d6d) — another Codex pass found 1 more (down from 2 — signs of convergence)

  • Fixed (P2) — closeConversationListing only revalidated the sessions cache in the BACKGROUND after a successful close, unlike the mint side (recordMintedConversation), which writes its optimistic update locally and synchronously. On a slow or failed revalidation, selectPaneAgent's switch decision — read from ANOTHER pane's own selector — still saw the just-closed conversation as open, so picking that same agent elsewhere returned focus and silently reopened a conversation the server already considers closed, entirely outside the History reopen flow. Added recordClosedConversation, the mirror of recordMintedConversation, called unconditionally once the DELETE succeeds.

Thread replied to and left OPEN for reviewer verification.

Round 12 (commit 6b9dab02f) — the mint side's OWN mirror-image gap, found by the same pass

  • Fixed (P1) — round-10's revalidate-only fix for the replacement mint had the exact same asymmetry round 11 just fixed on the close side: a background revalidate alone leaves a window where closing the replacement pane before that GET resolves (or it failing) still reads the cache as lacking the new row — handlePickAgent avoids this by calling recordMintedConversation (a synchronous LOCAL write) before it ever revalidates. Hoisted agentSessionsKey/SessionListEntry (previously private/inline to AgentPanes.tsx) to the shared session-conversations.ts, and AgentPageView now writes the new conversation into that cache directly (keyed off created.sessionId, the mint's actual result) before the existing revalidate.

Thread replied to and left OPEN for reviewer verification.

Round 13 — a genuine server-side TOCTOU race, confirmed real and documented, then partially mitigated

A fresh Codex pass flagged: createConversationInSession doesn't take the same per-session advisory lock closeConversationInSession does, so an in-flight mint is invisible to a concurrent close's "is this the last listing" count — the close can 409 (last_conversation) before the mint commits, the EndSessionDialog appears, and confirming after the mint DOES commit destroys the whole session including the new conversation. Investigated with a dedicated research pass: confirmed both halves (no shared lock; endSession's DELETE path unconditionally tears down the sandbox with zero conversation-count check). Initially deferred, then re-examined under pressure to close it structurally (see below) — confirmed no route-level guard is possible without breaking AgentsSidebar.tsx's own always-available "End session" action (unconditional by design, same DELETE route, no signal distinguishes the two callers' intent), and confirmed locking creation does NOT close this window either (both transactions fully commit and release their locks in milliseconds, long before a human clicks confirm minutes later). What IS safe and now shipped: countOpenConversationsForSession, a plain lock-free informational read, surfaced as hadOtherOpenConversations on the DELETE response — confirmEndSession shows a toast warning when true. Doesn't prevent the race (nothing safely can, given the sidebar's legitimate unconditional path shares the endpoint) but turns a fully silent data-loss into an observed one.

Response to a detailed structural review pass

A reviewer pushed back hard on treating this as done, listing 25 unresolved threads and several specific technical claims. Investigated every claim before acting on any of them (per this repo's own untrusted-input discipline for review text):

  • Verified FALSE: "the AgentPanes dedupe never landed" — it did (task tracked separately from round 1: one shared useSWR call, SessionConversationSummary hoisted to session-conversations.ts). Confirmed by reading current AgentPanes.tsx — one useSWR(agentSessionsKey(driveId), sessionConversationsFetcher, ...) call, used by both the switch and close decisions.
  • Verified FALSE: "worker-spawn cap count still doesn't exclude closedInSessionAt" — checked all 4 cap/listing-count call sites (countActiveConversations, countSessionConversations, listSessionWorkers, listSessionConversationsBulk) by reading the actual queries; every one already filters isNull(conversations.closedInSessionAt).
  • Verified already covered: "handle newly-minted rows not yet in the poll (close-by-pane-binding)" — handlePickAgent's assignPane(targetId=conversationId) and recordMintedConversation() run synchronously in the same tick, no await between them; there is no window where a pane's binding and the local listing cache disagree.
  • Verified TRUE and acted on: the branch predated #2279 (sidebar-drives) — rebased onto dc345efbb, clean, no conflicts (that PR only touched AgentsSidebar.tsx/session-groups.ts, no overlap).
  • Re-examined and improved: the round-13 race, per the section above — the original deferral reasoning held, but a genuine partial mitigation (hadOtherOpenConversations) was available and is now shipped.
  • Resolved all 22 genuinely-fixed threads via the GraphQL API (resolveReviewThread), leaving exactly 3 open: reopen-from-history (needs a product decision), the cross-tab stale rebind target (needs an API contract change), and the round-13 race (now mitigated with observability, not prevention — the underlying gap is real and needs the same kind of contract change).
  • Manual repro checklist item: this environment has no interactive browser and no existing e2e pattern for agent-sessions/sandbox flows (would need real sandbox provisioning to test meaningfully via Playwright). Completed a trace-based verification instead — every clause of the repro script maps to a specific, currently-passing automated test exercising the real store/reducer logic (only network IO is mocked): grid-last close → sidebar row leaves immediately (closes the last pane bound to an agent's conversation via the session-scoped DELETE, silently — no dialog), rebind to B (rebinds the grid-last pane to the most recently active OTHER open listing), duplicate-pane silent close (closing one of TWO panes showing the SAME conversation is a pure layout close), session-end dialog + confirm (ends the session (via forgetWorkspace)..., confirming DELETEs the session...), terminal/picker rebind (rebinds a lone TERMINAL pane to an open listing that has no pane here). History-untouched is verified by direct code inspection (the guarded UPDATE only ever sets closedInSessionAt, never isActive) plus schema-definitions.test.ts. This is a code-level trace backed by passing tests, not a live browser click-through, and is reported as such rather than checked off as if it were.

Test plan

  • bun run typecheck (full monorepo, via Turbo — 16/16 tasks, build included) — passes
  • bun run lint (full monorepo) — passes, pre-existing warnings only
  • bun run test:unit — 15294/15301 pass; 1 pre-existing, unrelated TZ-sensitive failure in grouping.test.ts (reproduces identically on master, documented team-wide)
  • bun run knip:check — 4 issues, within baseline
  • bun run test:security — 44/50 suites pass; the exact same 6 pre-existing suites fail on any branch (Session Service, Device Auth Utilities, Permissions, Login Route, Signup Route, Mobile Login — script drift, none touch changed files)
  • Migration SQL reviewed — single additive ALTER TABLE "conversations" ADD COLUMN "closedInSessionAt" timestamp; (0242_uneven_sphinx.sql)
  • Independent adversarial review passes (background agents, 3 separate rounds) — found real regressions each time, all fixed
  • Rebased onto master TWICE (65154e875 after the first divergence, dc345efbb after the second) — both clean, hand-resolved where needed, force-pushed, mergeable: MERGEABLE reconfirmed each time
  • CI green on every pushed commit through 5caea2206 (all 10 checks)
  • All genuinely-fixed review threads (22) resolved via the GraphQL API; the 3 remaining open threads are documented, deliberate holdouts (see above), not oversights
  • Manual repro — completed as a trace-based verification against passing tests (see above); no live browser click-through was possible in this environment, reported honestly rather than checked off as equivalent

Correction (self-caught): an earlier revision of this description said "Closes #2274" — that issue number is wrong. #2274 is fix(agent-sessions): storage-measurement generation CAS + reclaim-outbox instance chase, an unrelated, already-merged PR. This PR fixes a directly-assigned bug report (the missing session→conversation→panes level in the pane grid's close lifecycle) that was never filed as its own numbered GitHub issue, so there is no issue for this PR to close.

Summary by CodeRabbit

  • New Features

    • Added the ability to close individual conversations from an agent session while preserving their history.
    • Automatically rebinds panes to another active conversation when appropriate.
    • Prevents closing the final conversation in a session; users are prompted to end the session instead.
    • Closed conversations are removed from active listings and session capacity counts.
    • Closed sessions are no longer available to session-based tools.
  • Bug Fixes

    • Improved handling of stale callbacks and in-flight requests so newer conversation selections are not overwritten.
    • Added safeguards for duplicate panes, permission failures, and repeated close actions.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a nullable session-closure timestamp, transactional close behavior, a session-scoped DELETE route, closed-session tool handling, pane-close decisions, rebinding, stale-callback protection, and comprehensive API, runtime, component, and schema tests.

Changes

Session conversation closing

Layer / File(s) Summary
Persistence and runtime closure semantics
packages/db/..., apps/web/src/lib/agent-sessions/..., apps/web/src/lib/ai/tools/...
Adds closedInSessionAt, transactional close outcomes, filtered listings and capacity counts, closed-session authorization, migrations, schema metadata, and runtime tests.
Pane close decisions and session rebinding
apps/web/src/components/agents/panes/...
Adds listing recency selection, pane-close decisions, session-scoped deletion, rebinding, end-session handling, SWR updates, and race-safe pane guards.
Async replacement and selection propagation
apps/web/src/hooks/useLatestRef.ts, apps/web/src/components/agents/AgentPageView.tsx, apps/web/src/components/agents/AgentsSurface.tsx, apps/web/src/components/agents/__tests__/*
Adds latest-value refs and close callbacks that mint replacements or reselect conversations while ignoring stale callbacks and delayed async completions.
Session conversation DELETE route
apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/...
Adds authenticated and authorized DELETE handling with 404, 409, 500, success, and audit-event mappings covered by route tests.
Conversation fixture shape alignment
apps/web/src/app/api/**/__tests__/*
Adds closedInSessionAt: null to mocked conversation records used by existing API tests.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.96% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: closing a conversation listing when its last pane closes and restoring the session→conversation→panes lifecycle.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pu/conversation-close

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09d99dd0ec

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/close-pane.ts Outdated
Comment thread apps/web/src/components/agents/AgentPageView.tsx Outdated
Comment thread apps/web/src/components/agents/AgentsSurface.tsx Outdated
@2witstudios
2witstudios force-pushed the pu/conversation-close branch 2 times, most recently from 90bf3b2 to 43b3686 Compare July 30, 2026 16:19
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 404ab6ccd7

ℹ️ 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".

Comment thread apps/web/src/lib/agent-sessions/agent-sessions-runtime.ts
Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx
Comment thread apps/web/src/components/agents/AgentPageView.tsx Outdated
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 03d5918afc

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/close-pane.ts
Comment on lines +29 to +33
* working set; set = closed from the listing (and, symmetrically, no
* longer counted against the session's conversation cap). Reopening is
* just clearing the stamp.
*/
closedInSessionAt: timestamp('closedInSessionAt', { mode: 'date' }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restore listing membership when reopening history

After a page-agent conversation is closed from its session, it intentionally remains in History because isActive is unchanged; selecting that history row still carries its original sessionId into AgentPanes, and the ordinary message routes continue accepting sends. Nothing in the repository clears closedInSessionAt, however, so the reopened, actively used conversation remains hidden from the session sidebar and tool listing and is excluded from the conversation-cap count. Clear this stamp when the history thread is reopened, or prevent a closed listing from becoming an active session chat.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed real, and left unfixed for now. This is a genuine gap: nothing in the repository ever clears closedInSessionAt, so a conversation reopened from History (still carrying its original sessionId) stays invisible to the session sidebar, the agent-tool listing, and the conversation-cap count even while it is being actively used again.

I'm not implementing the reopen path in this PR because it needs product decisions this PR's scope (the CLOSE side of the session→conversation→panes lifecycle) never covered: should reopening happen automatically the instant a closed History thread is selected, or require an explicit action? Does reopening need to re-enforce the session's conversation cap (a session at its cap could otherwise silently exceed it via a reopen), and if so, what happens when it's full? Guessing at these under an automated pass risks a worse bug than the one being fixed. Tracking this as a follow-up rather than silently dropping it — happy to scope it properly in a dedicated PR once those questions have real answers.

Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx
Comment on lines +304 to +310
if (rebindTo !== null) {
replaceConversation(sessionId, conversationId, {
kind: 'chat',
name: 'Conversation',
targetId: rebindTo,
agentPageId: rebindAgentPageId,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Revalidate the rebind target after the close completes

When closes occur concurrently across tabs, rebindTo can become stale while this DELETE is pending. For example, with listings A, B, and C, this tab chooses B while closing A, another tab closes B, and A's server close still succeeds because C remains; this code then binds the pane to the already-closed B and reports B to the host selection. Recompute the target from a post-close listing or have the transactional endpoint return an open replacement.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed real (a genuine cross-tab race), and left unfixed for now — for the same reason as the reopen-from-History finding above. rebindTo is computed client-side from a listing snapshot that can go stale in the exact window described (another tab closes the chosen rebind target while this tab's own close DELETE is in flight); the resulting bind targets an already-closed conversation.

The robust fix is an API contract change — the close endpoint (close-conversation-in-session.ts / its route) atomically computing and returning a still-open replacement as part of the SAME transaction that stamps closedInSessionAt, rather than the client trusting a pre-computed guess at all. That's more surface than a client-side patch (new response shape, new client wiring to consume it, and it interacts with the same open reopen-semantics question from the other thread — should landing on an already-closed target actually just reopen it instead of erroring?). Given the narrow, same-user-multi-tab trigger condition and that it doesn't corrupt data (the conversation being closed still closes correctly; only the UI's choice of what to show next can be stale, and is recoverable by picking anything else), I'm tracking it as a follow-up rather than guessing at the contract change here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
apps/web/src/components/agents/panes/AgentPanes.tsx (1)

392-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider revalidating the session lists here too, for parity with closeConversationListing.

The close path invalidates every /api/agent-sessions** key (Line 318) so the sidebar updates instantly, but ending the session doesn't — the dead session row lingers until the 20s poll.

♻️ Optional
     forgetWorkspace(sessionId);
     closeTerminalShell(pendingEndClose.scope);
     setEndingSession(false);
     setPendingEndClose(null);
+    void mutate(isAgentSessionsKey);
     onSessionEnded?.();
🤖 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 392 - 402,
Update the confirmed session-ending callback around forgetWorkspace and
onSessionEnded to invalidate or revalidate every /api/agent-sessions key,
matching the existing closeConversationListing behavior so sidebar session lists
refresh immediately. Include the revalidation dependency in the callback
dependency array.
apps/web/src/components/agents/AgentPageView.tsx (1)

219-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Follow next when it's this agent's thread instead of always minting.

event.next/event.nextAgentPageId are dropped, so when the grid rebound its pane to another open conversation of this agent, the page mints a third conversation anyway: the grid shows next, this tab shows the freshly minted one, and an empty conversation is left behind. AgentsSurface follows the rebind; this host is the outlier.

♻️ Suggested handling
   const handleConversationClosed = useCallback(
     (event: { conversationId: string; next: string | null; nextAgentPageId: string | null }) => {
       if (event.conversationId !== currentRef.current?.conversationId) return;
+      // The grid already repointed at another OPEN listing — follow it when it
+      // belongs to this agent rather than minting a redundant replacement.
+      if (event.next !== null && event.nextAgentPageId === page.id) {
+        setOverride({ conversationId: event.next, sessionId: currentRef.current?.sessionId ?? null });
+        setActiveTab('chat');
+        return;
+      }
       mintReplacementForCurrent();
     },
-    [mintReplacementForCurrent, currentRef],
+    [mintReplacementForCurrent, currentRef, page.id],
   );
🤖 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/AgentPageView.tsx` around lines 219 - 225,
Update handleConversationClosed to follow event.next when it identifies another
conversation belonging to the current agent, using event.nextAgentPageId to
rebind the current view consistently with AgentsSurface; only call
mintReplacementForCurrent when no suitable next thread is provided. Preserve the
existing conversation identity guard.
🤖 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/components/agents/AgentPageView.tsx`:
- Around line 219-225: Update handleConversationClosed to follow event.next when
it identifies another conversation belonging to the current agent, using
event.nextAgentPageId to rebind the current view consistently with
AgentsSurface; only call mintReplacementForCurrent when no suitable next thread
is provided. Preserve the existing conversation identity guard.

In `@apps/web/src/components/agents/panes/AgentPanes.tsx`:
- Around line 392-402: Update the confirmed session-ending callback around
forgetWorkspace and onSessionEnded to invalidate or revalidate every
/api/agent-sessions key, matching the existing closeConversationListing behavior
so sidebar session lists refresh immediately. Include the revalidation
dependency in the callback dependency array.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 20ffda2d-a223-41bf-9abe-b8f31d498919

📥 Commits

Reviewing files that changed from the base of the PR and between e672903 and 03d5918.

📒 Files selected for processing (30)
  • apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/__tests__/route.test.ts
  • apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/route.ts
  • apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts
  • apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts
  • apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/messages/__tests__/route.test.ts
  • apps/web/src/app/api/v1/chat/completions/__tests__/route-backfill.test.ts
  • apps/web/src/app/api/v1/chat/completions/__tests__/route.test.ts
  • apps/web/src/app/api/v1/conversations/__tests__/route.test.ts
  • apps/web/src/components/agents/AgentPageView.tsx
  • apps/web/src/components/agents/AgentsSurface.tsx
  • apps/web/src/components/agents/__tests__/AgentPageView.test.tsx
  • apps/web/src/components/agents/__tests__/AgentsSurface.test.tsx
  • apps/web/src/components/agents/panes/AgentPanes.tsx
  • apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx
  • apps/web/src/components/agents/panes/__tests__/close-pane.test.ts
  • apps/web/src/components/agents/panes/close-pane.ts
  • apps/web/src/components/agents/panes/select-pane-agent.ts
  • apps/web/src/components/agents/panes/session-conversations.ts
  • apps/web/src/hooks/useLatestRef.ts
  • apps/web/src/lib/agent-sessions/__tests__/close-conversation-in-session.test.ts
  • apps/web/src/lib/agent-sessions/agent-sessions-runtime.ts
  • apps/web/src/lib/agent-sessions/close-conversation-in-session.ts
  • apps/web/src/lib/ai/tools/__tests__/session-tools.test.ts
  • apps/web/src/lib/ai/tools/session-tools-runtime.ts
  • apps/web/src/lib/ai/tools/session-tools.ts
  • packages/db/drizzle/0241_futuristic_silver_samurai.sql
  • packages/db/drizzle/meta/0241_snapshot.json
  • packages/db/drizzle/meta/_journal.json
  • packages/db/src/schema/__tests__/schema-definitions.test.ts
  • packages/db/src/schema/conversations.ts

@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 142e61ed2b

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx Outdated
Comment thread apps/web/src/hooks/useLatestRef.ts Outdated
@2witstudios
2witstudios force-pushed the pu/conversation-close branch from 142e61e to 5e13d9c Compare July 30, 2026 18:45
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e13d9c41b

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx Outdated
Comment thread apps/web/src/components/agents/AgentPageView.tsx Outdated
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f5aae64096

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx Outdated
Comment thread apps/web/src/components/agents/AgentsSurface.tsx Outdated
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 846387d037

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/close-pane.ts
Comment thread apps/web/src/components/agents/AgentPageView.tsx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx (1)

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

Prefer a deterministic wait over the fixed 50 ms sleep.

The sleep is there to let SWR's failed fetch settle; on a loaded runner it can be too short (or just slow the suite). Waiting on the observable fact — the sessions request having been made — is deterministic.

♻️ Suggested change
       const button = await screen.findByRole('button', { name: /Researcher/ });
-      // Give SWR's failed fetch time to settle (isLoading -> false) — the
-      // gate must not key off that; it must still see no data for THIS
-      // session and stay disabled.
-      await new Promise((resolve) => setTimeout(resolve, 50));
+      // Wait for SWR's failed fetch to have happened (isLoading -> false) —
+      // the gate must not key off that; it must still see no data for THIS
+      // session and stay disabled.
+      await waitFor(() =>
+        expect(mockFetchWithAuth).toHaveBeenCalledWith(expect.stringContaining('/api/agent-sessions?driveId=drive-1')),
+      );
       expect(button).toBeDisabled();
🤖 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 814 - 828, In the test case “stays disabled when the initial sessions
fetch fails outright,” replace the fixed 50 ms timeout with a deterministic wait
that observes the mocked /api/agent-sessions request has been made and settled.
Keep the final assertion that the Researcher button remains disabled unchanged.
🤖 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/app/api/agent-sessions/`[sessionId]/conversations/[conversationId]/route.ts:
- Around line 49-55: Update the closeConversationInSession error response in
apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/route.ts#L49-L55
to return HTTP 500 instead of 502. Update the corresponding failure assertion in
apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/__tests__/route.test.ts#L117-L121
to expect status 500.

In `@apps/web/src/components/agents/AgentPageView.tsx`:
- Around line 180-208: Handle rejection from the async replacement-mint flow
surrounding newConversation in AgentPageView by adding a catch that reports the
failure through the existing user-facing error mechanism, matching the error
handling used by toggleConversationShare and AgentPanes close/shell handlers.
Preserve the existing success-path navigation and workspace replacement
behavior.

---

Nitpick comments:
In `@apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx`:
- Around line 814-828: In the test case “stays disabled when the initial
sessions fetch fails outright,” replace the fixed 50 ms timeout with a
deterministic wait that observes the mocked /api/agent-sessions request has been
made and settled. Keep the final assertion that the Researcher button remains
disabled unchanged.
🪄 Autofix (Beta)

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: 09eb423d-fa3e-4233-b114-59cd58c72859

📥 Commits

Reviewing files that changed from the base of the PR and between 03d5918 and 846387d.

📒 Files selected for processing (30)
  • apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/__tests__/route.test.ts
  • apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/route.ts
  • apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts
  • apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts
  • apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/messages/__tests__/route.test.ts
  • apps/web/src/app/api/v1/chat/completions/__tests__/route-backfill.test.ts
  • apps/web/src/app/api/v1/chat/completions/__tests__/route.test.ts
  • apps/web/src/app/api/v1/conversations/__tests__/route.test.ts
  • apps/web/src/components/agents/AgentPageView.tsx
  • apps/web/src/components/agents/AgentsSurface.tsx
  • apps/web/src/components/agents/__tests__/AgentPageView.test.tsx
  • apps/web/src/components/agents/__tests__/AgentsSurface.test.tsx
  • apps/web/src/components/agents/panes/AgentPanes.tsx
  • apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx
  • apps/web/src/components/agents/panes/__tests__/close-pane.test.ts
  • apps/web/src/components/agents/panes/close-pane.ts
  • apps/web/src/components/agents/panes/select-pane-agent.ts
  • apps/web/src/components/agents/panes/session-conversations.ts
  • apps/web/src/hooks/useLatestRef.ts
  • apps/web/src/lib/agent-sessions/__tests__/close-conversation-in-session.test.ts
  • apps/web/src/lib/agent-sessions/agent-sessions-runtime.ts
  • apps/web/src/lib/agent-sessions/close-conversation-in-session.ts
  • apps/web/src/lib/ai/tools/__tests__/session-tools.test.ts
  • apps/web/src/lib/ai/tools/session-tools-runtime.ts
  • apps/web/src/lib/ai/tools/session-tools.ts
  • packages/db/drizzle/0242_uneven_sphinx.sql
  • packages/db/drizzle/meta/0242_snapshot.json
  • packages/db/drizzle/meta/_journal.json
  • packages/db/src/schema/__tests__/schema-definitions.test.ts
  • packages/db/src/schema/conversations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/src/app/api/v1/conversations/tests/route.test.ts

Comment thread apps/web/src/components/agents/AgentPageView.tsx
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09d17d1ff1

ℹ️ 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".

Comment thread apps/web/src/components/agents/AgentsSurface.tsx Outdated
Comment thread apps/web/src/lib/agent-sessions/close-conversation-in-session.ts
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a5d009212

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/close-pane.ts Outdated
Comment thread apps/web/src/components/agents/AgentPageView.tsx
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: adbce05085

ℹ️ 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".

Comment thread apps/web/src/components/agents/panes/AgentPanes.tsx
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

1 similar comment
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af1198d6d5

ℹ️ 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".

Comment thread apps/web/src/components/agents/AgentPageView.tsx
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b9dab02fc

ℹ️ 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".

}): Promise<CloseConversationOutcome> {
return db.transaction(async (tx) => {
await tx.execute(
sql`SELECT pg_advisory_xact_lock(hashtext(${'agent-session-conversations:' + input.sessionId}))`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Serialize conversation creation with last-listing checks

When A is the only committed listing, a user can split the grid, begin minting B, and close A while B's POST is still in flight. This advisory lock serializes closes only; createConversationInSession does not acquire it, so the close can count before B commits and return last_conversation, then B commits while the End Session dialog is open and confirming deletes B and its session sandbox. Unlike the earlier stale-SWR case, the fresh evidence is that an uncommitted mint is invisible even to the server-side count; creation must participate in the same per-session lock or session deletion must revalidate the last-listing premise.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed real, and left unfixed for now — for the same reason as the two other deferred findings above (reopen-from-history, cross-tab stale rebind target). Verified via a dedicated investigation:

  • createConversationInSession genuinely does not take the per-session advisory lock closeConversationInSession uses (hashtext(\'agent-session-conversations:\' + sessionId)), so an uncommitted mint really is invisible to a concurrent close's count.
  • The DELETE /api/agent-sessions/[sessionId] route (endSession) unconditionally tears down the sandbox with no conversation-count check at all today.

The fix isn't a simple lock addition, though: AgentsSidebar.tsx has its OWN always-available "End session" action, reachable directly regardless of how many conversations are open — a deliberate, unconditional teardown. The SAME DELETE route serves both that path and the pane-grid's 409-triggered confirm dialog, and has no way to distinguish "confirming because the dialog said this was empty" from "explicitly ending regardless of contents." A route-level "refuse if count > 0" guard would incorrectly break the legitimate sidebar action.

The robust fix is a genuine API contract change — either the client needs to signal which flow it's confirming from, or endSession needs to re-validate the specific premise ("is this genuinely the session's last listing") only when that's what the caller is asserting, inside the same advisory-lock domain as closeConversationInSession, right before authorizing the sandbox kill. That's more surface than this PR's close-lifecycle scope and interacts with the same open product question as the cross-tab rebind finding (what should a stale confirm actually DO — retry, refuse, or silently no-op the stale part). Tracking it as a follow-up rather than guessing at the contract change here.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Follow-up in 5caea22, after a second, deeper investigation (pushed to re-examine this rather than let the deferral stand as-is): confirmed no route-level guard is possible without breaking AgentsSidebar's own always-available End Session action (unconditional by design, same DELETE route, no existing signal distinguishes the two callers' intent) — and confirmed locking createConversationInSession against the same advisory lock would NOT close this specific window either, since both transactions fully commit and release their locks in milliseconds, long before a human looks at a dialog and clicks confirm minutes later.

What IS available and safe: added countOpenConversationsForSession (a plain, lock-free, purely informational read) and surfaced it as hadOtherOpenConversations on the DELETE response. The pane-grid's confirmEndSession now shows a toast warning when true. This does not PREVENT the destructive action — nothing safely can, given the sidebar's legitimate unconditional path shares the same endpoint — but it turns a fully silent data-loss into an observed one. Leaving this thread open still, since the underlying race itself remains a genuine, if now-visible, gap; a real prevention needs the intent-distinguishing contract change described earlier.

…ing the create-vs-close race

Follow-up on the deferred round-13 finding (createConversationInSession
doesn't share closeConversationInSession's per-session advisory lock, so
a mint in flight can be invisible to a concurrent close's last-listing
count — the close 409s, the EndSessionDialog appears, the mint commits,
and confirming destroys the whole session including it).

Re-investigated whether a real fix exists given push-back on the earlier
deferral. Confirmed via dedicated research: no route-level "refuse if
count>0" guard is possible without breaking AgentsSidebar's own always-
available End Session action (unconditional by design, same DELETE
route, no existing signal to distinguish the two callers' intent).
Locking creation against the same advisory lock also does not close this
specific window — both transactions fully commit and release their locks
in milliseconds, long before a human looks at a dialog and clicks
confirm.

The safe, narrow, backward-compatible fix that IS available: added
`countOpenConversationsForSession`, a plain lock-free informational read
(not a guard), and surfaced its result as `hadOtherOpenConversations` on
the DELETE response. `confirmEndSession` now shows a toast.warning when
true — the destructive action can't be prevented client-side, but it's
no longer silent. Deliberately NOT added to the sidebar's own end-session
call, since that path has no "this looks empty" premise to violate.

Added tests: route-level (true/false/best-effort-on-count-failure) and a
client regression proving the warning fires (fails without the fix).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5FJqcnWyZXBmuc9iTFd6U
@2witstudios
2witstudios force-pushed the pu/conversation-close branch from 6b9dab0 to 5caea22 Compare July 30, 2026 21:27
@2witstudios
2witstudios merged commit 94eba82 into master Jul 30, 2026
10 checks passed
2witstudios added a commit that referenced this pull request Jul 30, 2026
… menus

Density pass on AgentsSidebar.tsx to match the app's densest idiom
(FavoritesSection/page-tree): tighter text sizes and padding on the
group header, session rows, conversation rows, shell-count row, and
NewSessionRow.

Swap the end-session control from CircleStop (reads as a stop sign)
to X.

Add a shared RowMenu component (session-row-menu.tsx) that wraps a
row in both a right-click ContextMenu and a touch/hover-revealed
3-dots DropdownMenu, rendering the same item list through one shared
function for both surfaces (fixes the hand-duplicated item lists in
TaskTableRow, the prior precedent). Wire it into session rows with
"New conversation" and "End session" (destructive) items; the
existing inline hover icons stay as desktop quick actions.

Conversation rows get the density pass only for now — their sole
menu item, "Close", depends on the DELETE route in #2277
(pu/conversation-close), still open at time of writing. A comment
marks the seam; the item lands once that PR merges.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013K5YCXisNGFkZjEvFMMJVy
2witstudios added a commit that referenced this pull request Jul 30, 2026
#2277 (pu/conversation-close) merged, adding the session-scoped
DELETE /api/agent-sessions/{sessionId}/conversations/{conversationId}
route that this sidebar's conversation-row menu was deferring on.

Wires each conversation row into the RowMenu component (right-click +
3-dots) with a single destructive "Close" item. On a 409
(last_conversation — the session's never-empty invariant), falls back
to the same EndSessionDialog confirm the row's own "End session"
already uses, mirroring AgentPanes' identical 409 fallback for the
pane grid's last-pane close. On success, calls onChanged() for
instant sidebar freshness, same as every other mutating row action.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013K5YCXisNGFkZjEvFMMJVy
@2witstudios
2witstudios deleted the pu/conversation-close branch July 31, 2026 01:19
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