Repository navigation
fix(agents): scope worker verbs, cap conversations, bound listings [session-tools] - #2269
Conversation
…ons, bound listings Post-merge audit follow-up on PR #2258's session-tools slice (six-slice adversarial review of the agent-session un-conflation). - send_session/read_session/kill_session now resolve the caller's own workspace (findOwnWorkspace) and require the target conversation's session binding to match, exactly like openOwnShell already does. Ownership of a conversation row was not enough: a caller could reach any conversation they owned, including ones in a different session, a different drive, or with no session at all. - planSpawnWorkerSession takes a required sessionConversationCount and enforces MAX_SESSION_CONVERSATIONS (100) per session; the same cap is a backstop inside createConversationInSessionWith (the one write path both HTTP conversation-create routes and the tool's spawn dep funnel through), so worker minting is bounded on both the tool and HTTP paths. New SessionFullError / sessionConversationLimitExceeded (429). - Raw error.message no longer reaches model context from the worker dispatch or worker-create paths; fixed messages are returned and the real error is logged server-side. - listSessionWorkers and listSessionConversationsBulk are now bounded in SQL (a LIMIT and a ROW_NUMBER() OVER (PARTITION BY sessionId ...) window filter, respectively) instead of relying on a JS-side cap after pulling the full row set into memory. - list_sessions' no-session guidance no longer claims spawn_session/ spawn_shell will start a session — both refuse there too. - Documented the shared-session metadata-exposure semantics (listings show titles/agents of every conversation in a session to any member; transcripts stay owner-gated) as deliberate, where the queries live. Closes #2262 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ANkaVygvTgwLNwe6y9UGtT
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb1b0db6e
ℹ️ 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".
| const activeCount = await deps.countActiveConversations(sessionId); | ||
| if (activeCount >= MAX_SESSION_CONVERSATIONS) { |
There was a problem hiding this comment.
When multiple conversation-create requests reach a session near the ceiling concurrently, each request can observe the same count below 100 and then insert independently, so the new backstop does not actually guarantee the advertised maximum; a client can intentionally issue a burst through either HTTP route or spawn_session, producing conversations that the capped listings then hide. Serialize the count-and-insert operation for the session, or enforce the limit transactionally in the database.
Useful? React with 👍 / 👎.
| .from(rankedConversations) | ||
| .where(sql`${rankedConversations.rowNumber} <= ${MAX_SESSION_CONVERSATIONS}`); |
There was a problem hiding this comment.
The outer query has no ORDER BY, and the ordering inside ROW_NUMBER() only determines rankings; it does not guarantee the returned row order. Consequently each grouped array can be arbitrary even though this function promises newest activity first, and AgentsSidebar.tsx treats session.conversations[0] as the most recent conversation both when opening a session and when choosing the agent for a new conversation. Add an outer ordering by session and rank/lastMessageAt.
Useful? React with 👍 / 👎.
1 participant
Summary
Full fix for issue #2262 — the six findings from the post-merge audit of PR #2258's session-tools slice (
apps/web/src/lib/ai/tools/session-tools*.ts,apps/web/src/lib/agent-sessions/agent-sessions-runtime.ts):send_session/read_session/kill_sessionnow resolve the caller's own workspace (findOwnWorkspace) and require the target conversation'ssessionIdbinding to match it — mirroringopenOwnShell's existing check. Previously ownership of the conversation row alone was sufficient, so a prompt-injected agent could reach any conversation its user owned: a different session, a different drive, or a session-less thread — and exfiltrate a transcript or dispatch turns into a foreign sandbox.planSpawnWorkerSessiontakes a requiredsessionConversationCountinput and enforcesMAX_SESSION_CONVERSATIONS(100). The same cap is enforced as a backstop insidecreateConversationInSessionWith— the one write path both HTTP conversation-create routes and the tool's spawn dep funnel through — so worker minting is bounded on the tool path AND the HTTP path. NewSessionFullError/sessionConversationLimitExceeded(429), with truthful denial copy (the oldconcurrency_exceededmessage told callers tokill_session, which frees no counted slot).listSessionWorkersgets aLIMIT;listSessionConversationsBulkis bounded via aROW_NUMBER() OVER (PARTITION BY sessionId ...)window filter (preserving per-session fairness, which a flatLIMITwould lose) instead of pulling the full row set into JS before capping it.list_sessions' no-session note no longer claimsspawn_session/spawn_shellwill start a session — both refuse there too, post-unconflation.Test plan
session-tools.test.tspin: cross-session-same-owner refusal, session-less-target refusal, session-less-caller refusal — identical failure shape to a nonexistent session.plan-spawn-session.test.tspin the conversation-cap ordering (account cap before session cap) and the ceiling value.create-conversation-in-session.test.tspin the HTTP-path cap, including that an idempotent retry at the ceiling is still allowed through.bun run typecheck && bun run lint && bun run test:unit && bun run knip:checkall pass.bun run test:security— 44/50 suites pass; the 6 failures (Session Service, Device Auth Utilities, Permissions, Login/Signup/Mobile-Login Routes) are pre-existingtest-security.shpath/config drift unrelated to this change (confirmed unchanged vsorigin/master, and touching none of the files in this diff).cd packages/lib && bun run typecheckpasses.Closes #2262
🤖 Generated with Claude Code
https://claude.ai/code/session_01ANkaVygvTgwLNwe6y9UGtT