fix(gateway): run /apps/chat turns one at a time and save them - #263
Merged
Merged
Conversation
Account-token verification runs PBKDF2 (120,000 iterations, about 17 ms of CPU) inside OperatorAccountService's global lock and then rewrites the accounts file. The role checks added in this branch meant that /v1/*, /apps/chat, A2A, /ws, /ws/live, and mutating MCP tools verified the same token twice per request, halving account-token throughput on those surfaces. Cache the verification outcome in HttpContext.Items for the rest of the request. Revocation, disabling, and role changes still apply from the next request. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
/ws/live admitted any authenticated role. It runs no tools, but it opens a live session on the gateway's provider credentials, so a viewer token could spend them. It now uses CanExecuteAgent like /ws and closes below operator with 1008. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Companion opened the chat socket before loading the account's role, so a viewer was admitted and then closed by the gateway. It now loads the role first; when the gateway reports a role below operator it explains that chat needs the operator role instead of connecting, and keeps the read-only status views. A role that is only the no-token placeholder does not block the connection, and the role is applied as soon as the auth session loads rather than after the setup status call. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Session.AuthenticatedUserId scopes per-user capability bindings and is passed to MCP servers as _meta.userId, falling back to SenderId. Only /ws set it, so turns from REST messages, MCP send_message, A2A, /v1/*, and /apps/chat ran as whatever senderId or A2A contextId the caller supplied. - EndpointHelpers.ResolveAuthenticatedAccountId resolves the signed-in account (reusing the per-request token verification). REST and MCP stamp it on queued messages; /v1/*, /apps/chat, and the A2A bridge set it on the session. A2A captures it in middleware through an async-local, so detached SDK work still sees it. - The worker no longer lets an account-less external turn inherit the previous writer's account: after an operator posted into a Telegram session, the Telegram user's turns ran as that operator. System, scheduled, automation, and background-continuation turns still act on the session's behalf and keep its identity. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…admin Sessions are keyed only by id, so any operator who knew a session id could post into another account's conversation. - Session.OwnerAccountId is set once, when a signed-in account creates the session (pipeline, /v1/*, /apps/chat, A2A), and never reassigned. - SessionAccess.CanWrite: owned sessions accept the owner or an admin; unowned sessions (channels, cron, older data) stay open and are never claimed by writing to them; callers without an account (bootstrap, open loopback, channel and system turns) are not restricted. - Enforced on every write surface: REST and MCP send_message refuse before queueing (403 / tool error), the worker refuses pipeline turns including /ws with a reply, /apps/chat returns 403, and the A2A bridge emits an error event. Admin status travels with the message because OIDC admins are not in the account store. - Reads are unchanged. GET /api/integration/sessions?owner=me lists the caller's own sessions, backed by an owner filter in the file and SQLite stores and in the shared summary filter. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
POST /apps/chat was the only turn surface that neither took the session lock nor saved the session afterwards. Two requests for the same sessionId ran concurrently against one history, and a turn was saved only if its session later expired or was evicted, so a restart could lose it. Hold the session lock from the ownership check through the turn, as the A2A bridge does, and save the session when the turn ends, including when the client disconnects mid-turn. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (43)
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 |
…role Companion's preflight refused to open chat for any gateway-reported role below operator. With Security.AllowViewerAgentExecution on, the gateway still admits those viewers, so the documented migration switch did not work for Companion users. GET /auth/session now reports canExecuteAgent, computed by the same rule CanExecuteAgent enforces, including the opt-out. Companion blocks only when the gateway says false; when the field is absent (older gateways) it connects and lets the gateway decide. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…p-authenticated-account # Conflicts: # CHANGELOG.md
…rship # Conflicts: # CHANGELOG.md
- /ws resolved its account with a helper that returned nothing on a loopback-bound gateway, even with AlwaysRequireAuth or OIDC. Those turns carried no account, so the owner check treated them as accountless and let them write anywhere, and new sessions had no owner. /ws now uses the same caller resolution as the other surfaces. - /v1/* stable sessions are keyed by the bearer token's hash or, for browser sessions, the client address. Two signed-in accounts behind one address derived the same session and the second ran the first's conversation. Both endpoints now apply the owner check under the session lock and return 403 session_forbidden. - Session management (delete, metadata, abort, branch restore, guided recovery) stayed operator-wide, so another operator could delete an owned session and recreate the id as its own. Those routes now follow the owner-or-admin rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
Contributor
Author
|
Reopening to run CI against main. |
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
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.
Description
Stacked on #260 (← #259 ← #257). Merge those first, then retarget this PR to
main.POST /apps/chatis the only turn surface that neither takes the session lock nor saves the session afterwards. The worker, A2A, and/v1/*stable sessions do both.sessionIdranRunStreamingAsyncat the same time against oneSession, interleaving history writes and token accounting. An MCP App UI can easily send a follow-up while a turn is still streaming./apps/chathistory. With feat(gateway): record session owners and restrict writes to owner or admin #260 that also includes the session'sownerAccountId.This is based on #260 because it edits the same lines: the lock covers #260's ownership check.
Summary
OpenClawA2AExecutionBridgedoes. It is held through the ownership check and the whole turn, so a second request waits for the first to finish.finallywhen the turn ends, with the lock held. It usesCancellationToken.None, so a client that disconnects mid-turn still gets the history the turn produced.docs/MCPAPP.md(with zh-CN).Type of Change
Validation
dotnet build OpenClaw.Net.slnx --configuration Release: 0 warnings, 0 errorsdotnet test OpenClaw.Net.slnx --configuration Release --no-build: OpenClaw.Tests 3,158 passed / 11 skipped / 0 failed; NacosLiveAcceptance 25 passed; LayaService 67 passeddotnet run --project samples/OpenClaw.HelloAgent -c Release --no-buildBoth new tests failed first for the right reason:
Chat_ConcurrentTurnsOnSameSession_RunOneAtATime: the second turn started while the first was held open.Chat_PersistsSessionAfterTurn: nothing was saved to the store.Review Notes
docs/MCPAPP.mdwith zh-CN,CHANGELOG.md)Commercial or Customer-Driven Contribution Disclosure
Found while reviewing the #256 surfaces. Vendor-neutral gateway fix.
Checklist
dotnet test)🤖 Generated with Claude Code