feat(agents): fully resource-addressed session orchestration — permission-gated, any surface, any workspace (#2335) - #2336
Merged
Merged
Conversation
…n has ended
From the global assistant, spawn_session in a conversation whose session had
ENDED failed every time with the generic conversation_unavailable ('That
conversation id is not available') and left no trace in any log (issue #2335):
- resolveCallerSessionForWorker handed the dead session to the create path
(findByConversation does not filter endedAt), whose ended-session gate threw
ConversationUnavailableError;
- that error was excluded from createWorkerSession's logging, and the create
module's own catch dropped the underlying error entirely.
Fixes:
- resolveCallerSessionForWorker refuses a bound-but-ended session with a new
truthful session_ended reason; createWorkerSession maps it to an actionable
message (bindings are write-once, so 'start a new conversation' is the only
remedy).
- ConversationUnavailableError now carries the refusing gate as cause, and
createWorkerSession logs EVERY failure, cause included — the caller-facing
message stays generic.
- resolveOrCreateConversation sets updatedAt explicitly, matching the page
path, instead of leaning on drizzle's fill-$onUpdate-on-insert subtlety.
The issue's suspected root cause (updatedAt NOT NULL without default breaking
the global insert) is disproven by a new integration test against a
migration-built Postgres: drizzle 0.45 fills a default-less $onUpdate column
on INSERT, and the whole global spawn flow succeeds. A second integration
test locks in the session_ended refusal.
Fixes #2335
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughGlobal worker session handling now supports lazy global and drive-scoped provisioning, claims into ended sessions, workspace-aware access, and session-listing reopening. Conversation failures preserve causes and are logged. Global conversation inserts set ChangesGlobal worker session flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GlobalAssistant
participant SessionTools
participant AgentSessionsRuntime
participant ConversationCreator
participant Database
GlobalAssistant->>SessionTools: spawn_session with workspace target
SessionTools->>AgentSessionsRuntime: resolve or provision session
AgentSessionsRuntime->>ConversationCreator: create worker conversation
ConversationCreator->>Database: insert conversation with updatedAt
Database-->>ConversationCreator: conversation result or error
ConversationCreator-->>AgentSessionsRuntime: created conversation or unavailable error
AgentSessionsRuntime->>Database: claim conversation and reopen ended listing
AgentSessionsRuntime-->>GlobalAssistant: worker session result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/agent-sessions/__tests__/spawn-worker-global-session.integration.test.ts`:
- Around line 8-11: Correct the premise in the integration test documentation
around resolveOrCreateConversation: state that the insert succeeds because the
function explicitly sets conversations.updatedAt, rather than claiming Drizzle
fills the column automatically. Do not infer default-less $onUpdate insert
behavior unless adding a separate test that omits updatedAt.
🪄 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: b082f599-12ff-45d7-9416-1fe1614485b8
📒 Files selected for processing (6)
apps/web/src/app/api/ai/global/[id]/messages/resolve-or-create-conversation.tsapps/web/src/lib/agent-sessions/__tests__/create-conversation-in-session.test.tsapps/web/src/lib/agent-sessions/__tests__/spawn-worker-global-session.integration.test.tsapps/web/src/lib/agent-sessions/create-conversation-in-session.tsapps/web/src/lib/ai/tools/__tests__/session-tools-runtime.test.tsapps/web/src/lib/ai/tools/session-tools-runtime.ts
…ing, revivable sessions Product decision (issue #2335 follow-up): orchestration works from ANY surface, gated ONLY by the permission primitives — the global assistant acts with the user's own authority; a page AI orchestrates iff its drive RBAC allows. Workspace binding state and session lifecycle state never refuse a permitted operation. Three coordinated changes: 1. Worker verbs are RESOURCE-addressed, like read_page. send_session/ read_session/kill_session take a worker id and authorize against the resource (ownership; not closed; actually a worker) — the calling conversation no longer plays any authorization role and is not required at all. Supersedes issue #2262 finding 1's workspace confinement, per the product owner. The runtime dep now also refuses history-deleted workers explicitly (the old workspace comparison masked them). 2. Ended sessions are usable, and REOPEN on use. The lifecycle already treats ended rows as resumable (ensure fresh-provisions under the same identity) — only the claim/create gates disagreed, which is exactly how a thread bound to a stale session dead-ended every spawn (the original bug). The session_ended outcome is gone; a claim landing in an ended session withdraws the end-intent via the new pure planSessionReopen() stamp (endedAt only — the confirmed-kill stamp survives, so provisioning still fresh-creates and attach still refuses), applied CAS-guarded in the ONE claim dep every path shares. A reopened row is re-endable (end intent stamps the fresh intent without re-killing). 3. Unbound threads mint permission-gated, both kinds. Global conversations already minted; page conversations now mint a session in their agent's drive behind the exact primitives the manual New-session route enforces (canUserViewPage + checkAccessForSubject), via the generalized ensureConversationSession body (one body: global + drive variants). Integration tests prove the recovered story end-to-end on a migration-built Postgres: a global conversation bound to an ENDED session spawns a worker into that same workspace and the listing reopens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
2witstudios
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/agent-sessions/agent-sessions-runtime.ts`:
- Around line 316-325: Update claimConversation so failures from the post-claim
reopenEndedSessionListing operation are caught and logged without propagating.
Preserve the claimed outcome when conversationRepository.claimConversation
returns 'claimed', even if reopening the session listing fails.
🪄 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: d3c38baf-1da6-483a-bc9a-b5dbac458e8c
📒 Files selected for processing (16)
apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/claim/__tests__/route.test.tsapps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/claim/route.tsapps/web/src/app/api/agent-sessions/__tests__/route.test.tsapps/web/src/app/api/agent-sessions/route.tsapps/web/src/lib/agent-sessions/__tests__/claim-conversation-in-session.test.tsapps/web/src/lib/agent-sessions/__tests__/create-conversation-in-session.test.tsapps/web/src/lib/agent-sessions/__tests__/spawn-worker-global-session.integration.test.tsapps/web/src/lib/agent-sessions/agent-sessions-runtime.tsapps/web/src/lib/agent-sessions/claim-conversation-in-session.tsapps/web/src/lib/agent-sessions/create-conversation-in-session.tsapps/web/src/lib/ai/tools/__tests__/session-tools-runtime.test.tsapps/web/src/lib/ai/tools/__tests__/session-tools.test.tsapps/web/src/lib/ai/tools/session-tools-runtime.tsapps/web/src/lib/ai/tools/session-tools.tspackages/lib/src/agent-sessions/__tests__/plan-session-lifecycle.test.tspackages/lib/src/agent-sessions/plan-session-lifecycle.ts
💤 Files with no reviewable changes (2)
- apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/claim/route.ts
- apps/web/src/app/api/agent-sessions/[sessionId]/conversations/[conversationId]/claim/tests/route.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/lib/agent-sessions/create-conversation-in-session.ts
Completes the resource-addressed orchestration system (issue #2335): - spawn_session gains `workspace`: omitted = the caller's own workspace (minted if needed, as before); 'new' = a fresh ISOLATED workspace (own sandbox and filesystem — the fan-out case where subagents must not share the caller's working context), gated by the same checkAccessForSubject primitive as any mint and unwound if the worker never lands; any other value = an existing workspaceId, gated by checkSessionAccess (owner or drive member; foreign ids read as nonexistent). The advisory cap pre-count only applies to own-workspace spawns — a full caller workspace must not refuse a spawn aimed somewhere with room; the enforced cap at the claim covers every target. The result reports the workspaceId the worker landed in. - list_sessions now lists ALL the caller's active workspaces: the current one in full detail (workers, shells, sandbox) plus every other workspace with its workspaceId (a spawn target) and workers (addressable by the verbs from anywhere). Composed from the same primitives the sidebar uses (listSessions + listSessionConversationsBulk), agent labels batched. A session-less conversation still lists the caller's other workspaces. Location never gates anything: it supplies defaults and pane placement only. Integration test covers isolated mint, explicit target, foreign-id refusal, and cross-workspace discovery end-to-end on a real Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
2witstudios
changed the title
…mise Two CodeRabbit findings on PR #2336: 1. claimConversation's post-claim reopenEndedSessionListing call was unguarded — a thrown error (DB fault) after the binding UPDATE already committed would propagate out of claimConversation, through claimConversationInSessionWith (no try/catch there), and land in createWorkerSession's catch as a generic conversation_unavailable — reporting failure for a spawn whose conversation was, in fact, already successfully bound. The reopen is now caught and logged; a reopen failure never undoes or misreports a successful claim (matches the reviewer's proposed diff). 2. The integration test's doc comment claimed to prove drizzle's default-less--fills-on-insert behavior, but Fix 2 in this same PR made that moot: resolveOrCreateConversation sets updatedAt explicitly, so the happy-path test no longer exercises that fallback. Corrected the comment to describe what the test actually demonstrates, and to attribute the drizzle-behavior finding to where it was actually verified (direct inspection of pg-core/dialect.js), not to this test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/lib/ai/tools/session-tools.ts (1)
532-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the
as stringassertion by narrowingconversationIddirectly.Line 556 asserts
conversationId as string. The assertion is sound today becauseworkspaceis non-null only whenconversationIdwas truthy at line 534. That reasoning is implicit, so a later edit to line 534 can make the assertion unsound without a type error. Narrow once and let the compiler carry the fact.♻️ Proposed refactor
- const workspace = conversationId ? await deps.findOwnWorkspace(conversationId) : null; + const own = conversationId + ? { conversationId, workspace: await deps.findOwnWorkspace(conversationId) } + : null; + const workspace = own?.workspace ?? null; const otherWorkspaces = await deps.listOwnWorkspaces({ userId: actor.userId, excludeWorkspaceSessionId: workspace?.sessionId, }); @@ - if (!workspace) { + if (!own || !own.workspace) { @@ const listing = await deps.listSessionWorkers({ - workspaceSessionId: workspace.sessionId, - callerConversationId: conversationId as string, + workspaceSessionId: own.workspace.sessionId, + callerConversationId: own.conversationId, }); - return { success: true, workspaceId: workspace.sessionId, ...listing, otherWorkspaces }; + return { success: true, workspaceId: own.workspace.sessionId, ...listing, otherWorkspaces };As per coding guidelines: "Enable TypeScript strict mode for all files".
🤖 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/lib/ai/tools/session-tools.ts` around lines 532 - 558, Remove the conversationId as string assertion in the listSessionWorkers call by narrowing conversationId explicitly before the workspace-dependent path. Preserve the existing plain-conversation return and ensure the narrowed value is passed as a string to deps.listSessionWorkers.Source: Coding guidelines
apps/web/src/lib/ai/tools/session-tools-runtime.ts (1)
355-358: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider excluding the current workspace before the list limit applies.
listSessionsreturns at mostSESSION_LIST_LIMITrows. The filter then removes the caller's current workspace from that already-truncated slice. The result can contain one fewer other workspace than the limit allows, and the oldest workspace inside the limit is not backfilled. Since this listing is the discovery surface forspawn_session'sworkspacetarget, a workspace can drop off the list without any signal. IflistSessionsaccepts an exclusion or a limit argument, pass the exclusion down instead.🤖 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/lib/ai/tools/session-tools-runtime.ts` around lines 355 - 358, Update the session listing flow around listSessions so excludeWorkspaceSessionId is applied before SESSION_LIST_LIMIT truncates results. Pass the exclusion through listSessions using its existing exclusion or limit parameter if supported, and remove the post-fetch filter while preserving the empty-sessions return 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.
Nitpick comments:
In `@apps/web/src/lib/ai/tools/session-tools-runtime.ts`:
- Around line 355-358: Update the session listing flow around listSessions so
excludeWorkspaceSessionId is applied before SESSION_LIST_LIMIT truncates
results. Pass the exclusion through listSessions using its existing exclusion or
limit parameter if supported, and remove the post-fetch filter while preserving
the empty-sessions return behavior.
In `@apps/web/src/lib/ai/tools/session-tools.ts`:
- Around line 532-558: Remove the conversationId as string assertion in the
listSessionWorkers call by narrowing conversationId explicitly before the
workspace-dependent path. Preserve the existing plain-conversation return and
ensure the narrowed value is passed as a string to deps.listSessionWorkers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10f6b1b3-e21e-43eb-9378-d6edf4c20975
📒 Files selected for processing (5)
apps/web/src/lib/agent-sessions/__tests__/spawn-worker-global-session.integration.test.tsapps/web/src/lib/agent-sessions/agent-sessions-runtime.tsapps/web/src/lib/ai/tools/__tests__/session-tools.test.tsapps/web/src/lib/ai/tools/session-tools-runtime.tsapps/web/src/lib/ai/tools/session-tools.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/lib/agent-sessions/agent-sessions-runtime.ts
…hrow during isolated-workspace spawn Adversarial review pass on PR #2336 (independent agent, verified by hand) found a real gap: spawn_session's workspace: 'new' path minted a fresh workspace, then on ANY thrown error from createConversationInSession unconditionally called endSession() to unwind it — without the ambiguous-throw re-verification ensureGlobalSandboxSession already uses elsewhere in this same file for the structurally identical hazard (a connection dropping between a durable autocommit UPDATE committing and its acknowledgment reaching this process). If the worker's claim had actually committed before the throw, this would end a session the worker is genuinely bound to — and because nothing will ever claim into or list a freshly-minted-then-ended session again, that worker becomes permanently unreachable while spawn_session reports failure with no sessionId for the caller to investigate. Mirrors the existing pattern: re-resolve the worker's own binding before unwinding. Bound to the just-minted workspace -> report the success the throw obscured, never unwind. Confirmed unbound -> unwind as before. Re-verification itself fails -> leave the workspace alone (a stray, empty, endable-by-hand session is cheap; wrongly killing a bound one is not) and still report the original failure. Also added unit coverage for createWorkerSession's full placement logic (zero unit tests existed before this commit — only resolveCallerSessionFor- Worker was covered), including a dedicated regression test for the ambiguous-throw fix itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
…logic Pure simplification pass, no behavior change (verified: full affected test suite still 1,742/1,743 passing, same pre-existing unrelated failure; tsc/eslint clean). createWorkerSession had grown to ~150 lines interleaving three concerns: resolving WHERE a worker lands (3 branches), creating it, and recovering from the ambiguous-throw hazard on the isolated-workspace path. Extracted into resolveWorkerPlacement() and recoverMintedWorkspaceAfterThrow(), so the dep itself now reads as a straight-line pipeline: resolve placement, create, recover-or-map-error on failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
A fresh CodeRabbit review pass (submitted after the earlier two threads were already resolved) surfaced two Trivial/Low-value nitpicks, both verified against current code and addressed: 1. list_sessions carried an unnecessary `conversationId as string` assertion — sound only because `workspace` is non-null exclusively when `conversationId` was truthy, a fact a reader had to trust rather than one the compiler enforced. Folded into the existing null-guard (`if (!conversationId || !workspace)`) so TypeScript's own control-flow narrowing carries it instead — simpler than CodeRabbit's suggested wrapper-object diff, same effect. 2. listOwnWorkspaces excludes the caller's current workspace AFTER listSessions' own SESSION_LIST_LIMIT cap, which COULD drop a workspace a pre-cap exclusion would have backfilled — but verified unreachable today: SESSION_LIST_LIMIT and MAX_ACTIVE_SESSIONS_PER_OWNER are both 100, and the latter is a STRUCTURAL cap (createIfUnderLimit's atomic count-and-insert), so no owner can ever exceed the page size the listing already shows. Documented the coupling explicitly (a pre-cap exclusion would need a new AgentSessionListFilter shape for zero practical benefit) and added a standalone invariant test that fails loudly if the two constants — defined in different packages — ever drift apart, rather than silently reintroducing the gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
Owner
Author
|
Addressed both nitpicks from the 2026-08-05T20:16:31Z review pass in 4c0c96d:
|
This was referenced Aug 6, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2335 — and builds out the full orchestration model decided while diagnosing it: session tools are resource-addressed and permission-gated; binding state, lifecycle state, and calling surface never refuse a permitted operation. Global assistant = the user's own authority, from any surface. Page AI = its drive's RBAC. Location's only job is defaults and pane placement.
Commit 1 — diagnosis + stop swallowing errors
The issue's suspected root cause (
updatedAtwithout a DB default) is disproven: drizzle-orm 0.45.2's insert compiler (pg-core/dialect.js'sbuildInsertQuery) fills a default-less$onUpdatecolumn on INSERT too, verified by direct source inspection. The reproduced cause: a conversation bound to an ended session dead-ended every spawn, invisibly.ConversationUnavailableErrorcarries the refusing gate ascause; every throw site names its gate;createWorkerSessionlogs every failure (exclusion removed). Caller-facing messages stay generic.resolveOrCreateConversationsetsupdatedAtexplicitly (consistency hardening, matching the page path) — which is also why the happy-path integration test below does NOT exercise drizzle's default-fill fallback: the explicit set makes it moot for this code path (CodeRabbit caught the test's doc comment overclaiming this — fixed).Commit 2 — the authorization re-model
read_page.send_session/read_session/kill_sessionauthorize against the resource: ownership, not human-closed, actually a worker, not history-deleted. The calling conversation plays no authorization role and isn't required. Supersedes fix(session-tools): scope worker verbs to the workspace (H2 parity with shells), per-session caps, bounded listings [MED/security] #2262 finding 1's workspace confinement (product decision). Page-worker dispatch still re-enforces agent RBAC in the standard chat pipeline.session_endedis gone everywhere. A claim landing in an ended session withdraws the end-intent via the pureplanSessionReopen()stamp (endedAtonly — the confirmed-kill stamp survives, so provisioning still fresh-creates andattachstill refuses), applied CAS-guarded in the one claim dep all paths share. A reopened row is re-endable.canUserViewPage+checkAccessForSubject), through one generalized mint-and-claim body.Commit 3 — placement + discovery (the fan-out surface)
spawn_sessiongainsworkspace: omitted = the caller's workspace (minted if needed);'new'= a fresh isolated workspace (own sandbox/filesystem — for subagent fleets that must not share the caller's working context; unwound if the worker never lands); an existingworkspaceId= spawn straight into it (checkSessionAccess; foreign ids read as nonexistent). The advisory cap pre-count applies only to own-workspace spawns — a full caller workspace can't refuse a spawn aimed somewhere with room. Returns theworkspaceIdthe worker landed in.list_sessionslists ALL the caller's workspaces: the current one in full detail (workers, shells, sandbox), every other with itsworkspaceId(a spawn target) and workers — every worker everywhere addressable by the verbs. Same primitives as the sidebar (listSessions+listSessionConversationsBulk). A session-less conversation still sees its other workspaces.Unchanged invariants: the conversation→session binding stays write-once and owner-only (H1 hijack surface closed); shells stay workspace-scoped; the closed-listing gate holds.
Commit 4 — review response
Two CodeRabbit findings, both fixed:
claimConversation's post-claimreopenEndedSessionListingcall was unguarded — a thrown error after the binding UPDATE already committed would propagate throughclaimConversationInSessionWith(no try/catch there) and surface as a falseconversation_unavailablefor a spawn that had, in fact, already succeeded. Now caught and logged; a reopen failure never misreports a successful claim.Commit 5 — independent adversarial review (self-requested)
Dispatched a fresh review agent against the full diff to check for what an author reviewing their own work misses. It confirmed the two areas noted above were sound (resource-addressing keeps ownership as the sole-and-sufficient gate; the placement/CAS/lifecycle logic behaves as documented) and surfaced one real gap:
spawn_session'sworkspace: 'new'path minted a fresh workspace, then on ANY thrown error from the worker's creation unconditionally ended that workspace — without the ambiguous-throw re-verificationensureGlobalSandboxSessionalready uses elsewhere in the same file for the identical hazard (a connection dropping between a durable UPDATE committing and its acknowledgment). If the claim had actually committed, this would end a session the worker is genuinely bound to, permanently orphaning it (nothing will ever claim or list a freshly-minted-then-ended session again). Fixed by reusing the same re-verify-before-cleanup pattern: bound to the just-minted workspace → report the success the throw obscured; confirmed unbound → unwind as before; verification itself fails → leave the workspace alone (cheap and recoverable) and still report the original failure.createWorkerSession's placement logic had zero unit tests (onlyresolveCallerSessionForWorkerwas covered). Added a full suite, including a dedicated regression test for the ambiguous-throw fix.list_sessions's cross-workspace listing is scoped to workspaces the caller OWNS, whilespawn_session's explicit-workspaceIdpath (correctly) also permits drive members into a shared workspace — so a member can target a shared workspace by id but won't discover it vialist_sessions. This is a pre-existing shared-workspace visibility question (not a security issue — the spawn gate's permission logic is unaffected), and expanding discovery to every drive a member belongs to is a separate, larger feature with its own privacy/scale tradeoffs. Flagging here rather than silently dropping it.Commit 6 — a fresh CodeRabbit pass, post-refactor
A later review submission (after the two earlier threads had already closed) surfaced two Trivial/Low-value nitpicks — both verified against current code and fixed:
list_sessionscarried an unnecessaryconversationId as stringcast; folded into the existing null-guard so TypeScript's own control-flow narrowing carries the fact instead.listOwnWorkspacesexcludes the caller's current workspace afterlistSessions' own page-size cap, which could in principle drop a workspace a pre-cap exclusion would have backfilled — verified unreachable today (SESSION_LIST_LIMITandMAX_ACTIVE_SESSIONS_PER_OWNERare both 100, and the latter is a structural cap enforced at every spawn path), documented the coupling explicitly, and added a standalone test that fails loudly if the two constants — defined in separate packages — ever drift apart.Tests
planSessionReopen+ reopened-row re-end, spawn placement passthrough + advisory-count bypass, listing composition, fullcreateWorkerSessionplacement coverage including the ambiguous-throw regression test, and theSESSION_LIST_LIMIT/MAX_ACTIVE_SESSIONS_PER_OWNERinvariant guard.activity-tools.test.ts(untouched file; PR feat(security): GDPR PII encryption live cutover (dual-lookup edge) #1728'susers.emaildual-lookup vs a migrated local test DB) — not a regression from this branch. Full@pagespace/libsuite: 394 passed.tsc --noEmit+ eslint clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01YJwgtR1MR6Dgo3hkjTV4Nw
Summary by CodeRabbit
New Features
Bug Fixes