fix(gateway): route lobu chat to org's default agent end-to-end#1136
Merged
Conversation
Closes #1129, #1133. The 'isEphemeral' code path in createAgent — which generated a random UUID and tried to auto-install system-key providers for any no-agentId chat — was always broken: saveSettings was UPDATE-only and silently no-op'd because the agents row had never been INSERTed. Even after that was patched it would have polluted the DB with one phantom agent per chat invocation, with no continuity across calls. This PR removes the concept entirely and wires the existing per-org default agent (`owletto-default`, provisioned at signup via `ensureDefaultAgent`) into the chat path. Changes: 1. `createAgent` (gateway/routes/public/agent.ts): when the caller omits agentId, look up `owletto-default` for the caller's org instead of minting a UUID. Two flows now: caller-pinned, or default-agent — no third. 2. `ensureDefaultAgent` (auth/default-provisioning.ts): also write `installed_providers` from the set of providers with system keys (env-var API keys, Claude OAuth-discovery) at boot, so the default agent is chat-ready out of the box. Resolve the personal-org owner from `organization.metadata.personal_org_for_user_id` and write the agent as user-owned + mirror into `agent_users` so the per-user ownership check in `verifyOwnedAgentAccess` recognizes the PAT session's user as the owner. 3. `createApiAuthMiddleware` (gateway/auth/api-auth-middleware.ts): plumb the caller's resolved org through (worker-token payload, /oauth/userinfo, settings session) by attaching `authContext` to the Hono context AND running the request inside `orgContext.run()`. This was the #1068 follow-up pi flagged: the PAT path's `createLobuAuthBridge` already wraps in orgContext, but the OAuth and worker-token paths didn't — so any handler that reached for AsyncLocalStorage-scoped org id would miss for those auth methods. 4. `ExternalAuthClient.fetchUserInfo` (gateway/auth/external/client.ts): surface the organization id resolved from /oauth/userinfo's organization_slug + organizations[] list (previously typed away). 5. `ChatResponseBridge` (gateway/connections/chat-response-bridge.ts): render the NO_MODEL_CONFIGURED error with a real link to the agent's settings page (`${publicWebUrl}/${orgSlug}/agents/${agentId}`) instead of vague 'ask an admin' text. 6. CLI dry-run help text (cli/src/index.ts): tighten from 'Process without persisting history' (misleading — history IS persisted) to 'Skip side-effecting tool calls (sandbox writes, sdk_run mutations).' 7. `ThreadSession.isEphemeral` field deleted along with the wasEphemeral cleanup branch in DELETE /api/v1/agents — there are no ephemeral sessions anymore.
📝 WalkthroughWalkthroughThis PR implements tenant-scoped agent sessions and eliminates ephemeral agents. It propagates authentication identity through middleware, provisions default agents with per-user ownership, updates the session model, enforces cross-org access controls in agent routes, and adds org-contextual admin links for error handling. ChangesTenant-Scoped Agent Sessions with Default Provisioning
Sequence DiagramsequenceDiagram
participant Client
participant AuthMiddleware as Auth Middleware
participant HonoContext as Hono Context
participant AgentRoute as Agent Route
participant SessionManager as Session Mgr
Client->>AuthMiddleware: POST /api/v1/agents (with auth)
AuthMiddleware->>AuthMiddleware: Validate token/session
AuthMiddleware->>AuthMiddleware: Extract userId, organizationId
AuthMiddleware->>HonoContext: set authContext {userId, organizationId}
AuthMiddleware->>HonoContext: orgContext.run() if organizationId
AuthMiddleware->>AgentRoute: next()
AgentRoute->>HonoContext: c.get("authContext")
AgentRoute->>AgentRoute: Resolve pinned or default agent
AgentRoute->>AgentRoute: requireAgentOwnership with sessionForTenantCheck
AgentRoute->>AgentRoute: Tenant guard: compare org contexts
AgentRoute->>SessionManager: Create org-scoped session
SessionManager->>SessionManager: Append org suffix to conversationId
AgentRoute->>Client: 200 with session & token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
|
bug_free 38, simplicity 72, slop 0, bugs 1, 1 blockers typecheck/unit/integration logs all exit 0; reran bun test packages/server/src/gateway/tests/agent-session-create.test.ts -> 5 pass. Exploratory Hono harness showed orgB can GET orgA session when agentId matches because session.organizationId is ignored. Blockers
Suggested fixes
Full verdict JSON{
"bug_free_confidence": 38,
"bugs": 1,
"slop": 0,
"simplicity": 72,
"blockers": [
"Cross-tenant session routes ignore session.organizationId; reproduced GET /api/v1/agents/:sessionKey returning 200 for orgB on an orgA session when both orgs own the same agentId."
],
"change_type": "fix",
"behavior_change_risk": "high",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/server/src/gateway/routes/public/agent.ts",
"line": 836,
"change": "After resolving ownership, compare the authorized/caller organizationId with session.organizationId and return 403 on mismatch; apply the same guard before DELETE, SSE backlog replay, and direct message enqueue."
}
],
"notes": "typecheck/unit/integration logs all exit 0; reran bun test packages/server/src/gateway/__tests__/agent-session-create.test.ts -> 5 pass. Exploratory Hono harness showed orgB can GET orgA session when agentId matches because session.organizationId is ignored.",
"categories": {
"src": 474,
"tests": 242,
"docs": 0,
"config": 0,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
Pi review caught this: orgs where ensureDefaultAgent ran before this PR have the agents row but with owner_user_id = NULL and installed_providers = []. The sentinel-fast-path would skip them, so legacy installs would still hit 403 / 'No model configured' on lobu chat -c local. Add an idempotent backfillDefaultAgent helper that runs unconditionally on every ensureDefaultAgent call. It: - Populates owner_platform = 'external' and owner_user_id from the personal-org marker when the row's owner is unset (legacy 'lobu', NULL). - Adds the agent_users row for that owner so ownsAgent returns true. - Appends missing system-key providers to installed_providers when the list is currently empty. Never removes existing entries; never overwrites a non-empty list (admins may have curated it). No-op on freshly-inserted rows — those already have the correct shape.
…review)
Pi caught a real one: when the caller omits agentId, this PR resolves to
the global constant DEFAULT_AGENT_ID = 'owletto-default'. The userId
default downstream was `requestedUserId || agentId` — so for no-userId
requests userId also became 'owletto-default', and the session-store key
`${agentId}_${userId}` collapsed to the literal string
'owletto-default_owletto-default' identically across every tenant. Org B's
PAT session could then resume org A's session and inherit its worker
token.
Two-layer fix:
1. Derive userId from the authenticated caller via the new authContext
(`requestedUserId || authContext.userId || agentId`). For the
default-agent path, this makes conversationId per-user-per-org
unique. For pinned agents the agentId fallback stays safe because
pinned IDs are per-org scoped.
2. Defensive resume guard: even if a future caller bypasses the auth
bridge or sends a colliding requestedUserId, refuse to resume
when the existing session's organizationId doesn't match the
current tokenOrganizationId — return 403 instead of leaking the
worker token.
Also addressed pi's second finding (default-provisioning.ts:164,274):
replace the `metadata::jsonb` selects with `readOrgMetadata` so a row
with invalid legacy JSON doesn't abort provisioning/backfill.
… (pi review)
Pi found another cross-tenant path: even with userId now derived from the
authenticated caller, two different orgs whose external OAuth tokens share
the same `sub` would still produce the same conversationId — and a
forceNew request from org B would overwrite org A's session at
setSession time (the resume guard only catches the resume path).
Append the tokenOrganizationId to the conversationId so the key is
tenant-unique end-to-end. Covers:
- default-agent path (agentId = DEFAULT_AGENT_ID is a global constant)
- pinned-agent path (per-org rows can share the same id string across
tenants)
Watcher sessions retain their existing deterministic key (their userId
is already unique by virtue of `watcher_${watcherId}` and they bypass
the user-derived auth context).
Pi flagged tests_adequate=false — no committed tests for the new
default-agent ownership stamping or the backfill path. Add two
integration tests in default-provisioning.test.ts:
1. `stamps owner_user_id + installed_providers + agent_users on insert`
exercises the create path: marks the org as personal_org_for_user_id=<X>,
then asserts the inserted agent row has owner_platform='external',
owner_user_id=X, a populated installed_providers array, and a
corresponding agent_users row.
2. `backfills owner + agent_users on a legacy row past the sentinel`
simulates a pre-this-PR row (owner_platform='lobu', owner_user_id=NULL,
installed_providers='[]', sentinel already set) and asserts that
calling ensureDefaultAgent now heals it: owner fields updated to
('external', <ownerUserId>) and the agent_users row is added.
Also move the orphaned JSDoc back above ensureDefaultAgent (pi noted it
ended up over backfillDefaultAgent after the helper landed).
… review)
manager.publicGatewayUrl is the gateway base — in embedded mode the
gateway is mounted at /lobu under the web app (see server-lifecycle.ts:253:
`wrapper.route('/lobu', lobuApp)`), so it ends with /lobu. Admin UI
routes live at the web origin (`/<orgSlug>/agents/...`), not under
/lobu, so the link built by buildAgentSettingsUrl currently points to a
404 like http://localhost:8821/lobu/local-install/agents/owletto-default
instead of http://localhost:8821/local-install/agents/owletto-default.
Strip a trailing /lobu before composing the link.
Pi flagged tests_adequate=false on the route — store-level coverage landed but POST /api/v1/agents itself wasn't tested for the new default-agent path. Add three tests to agent-session-create.test.ts: 1. empty body resolves to owletto-default, returns 201 with a session id of shape <agentId>_<userId>_<orgId> (the tenant suffix is what prevents cross-org session collisions when DEFAULT_AGENT_ID is a global constant). 2. empty body returns 404 when the caller's org has no default agent. 3. empty body returns 404 when the default agent exists but belongs to another org — must NOT leak. All three use a real createApiAuthMiddleware path: the worker token carries organizationId, the middleware stamps authContext, the handler resolves through that. No DB needed; in-memory stubs match the production interfaces.
Pi caught a real cross-tenant exploit: GET/DELETE/SSE/messages routes checked agent ownership (platform, userId, agentId) but NOT session.organizationId. With agentIds repeating across tenants (the global DEFAULT_AGENT_ID constant, or two orgs sharing an id string), org B's caller could hit org A's sessionKey URL and read or delete A's session because both pass ownership against their own agent-X. I almost shipped four `denyOnCrossTenantSession` calls scattered next to each ownership check. That's the workaround — the next route added forgets the second call and the exploit is back. The structural fix is to fold the tenant check INTO requireAgentOwnership: pass the session (when available) and the helper denies on org mismatch before running the ownership query. One function, one place, can't be missed. createAgent has no pre-existing session and passes null; the other four routes (GET, DELETE, SSE, /messages) pass the session they already loaded for ownership resolution. Added a route-level test (cross-tenant GET → 403) that asserts the guard fires inside the auth helper itself, not at the call site, so a future route handler that forgets to plumb the session still gets the agent-level ownership check while route-level coverage flags the plumbing gap.
…enant gap Two follow-ups to the org-scoped conversationId / tenant-guard work in this PR: 1. Exempt watcher sessions from the orgScope suffix. It was spliced between userId and thread, turning ..._watcher_<id>_run_<id> into ..._watcher_<id>_<org>_run_<id> — which breaks watcher->worker dispatch: the worker session key AND the API/SSE owner-routing key (unified-thread-consumer) both derive from this conversationId and rely on the watcher_<id>_run_<id> shape. That reddened the sdk-e2e watcher gate (green on main). Watcher sessions are already globally unique via the DB-serial watcherId + runId, and tenant isolation rides session.organizationId (still set) + the route guard, not the string. 2. Enforce the tenant guard on the settings-session cookie path. The guard only fired when callerOrgId was set (PAT bridge / worker token / external OAuth), but the cookie path sets userId with no org, so the guard was a no-op there — and verifyOwnedAgentAccess authorizes on (platform, userId, agentId) and returns the caller's org, not the session's. A cookie session for org B could therefore read org A's session via the shared global DEFAULT_AGENT_ID. Now we compare the org ownership actually resolves to against the session's org and deny on a definite mismatch (undefined on either side falls through, so a legitimate same-org caller is never denied).
Two regression tests for the fixes in 6c040c3 (both verified red→green — they fail on the pre-fix source and pass after): - Cross-tenant GET via a settings-session COOKIE is denied with 403. The existing cross-tenant test only drives the worker-token path (which sets an org on authContext, so the up-front guard catches it). This drives the cookie path, where authContext has a userId but no org, so the up-front guard is a no-op — and exercises the resolved-org comparison that closes the leak. Pre-fix this GET returned 200 and served org A's session to org B. - Watcher conversationId keeps the exact `<agentId>_watcher_<watcherId>_run_<runId>` shape and omits the org suffix, even though the agent's metadata carries an org (so a non-watcher path would add it). Pre-fix the id was `watcher-agent_watcher_5_org-watcher_run_27`, which breaks watcher->worker dispatch correlation (the sdk-e2e gate).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/gateway/__tests__/agent-session-create.test.ts (1)
106-107: 💤 Low valueRemove unused
USER_IDconstant.
USER_IDis declared but only suppressed withvoidrather than used in assertions. Since the session ID shape is implicitly tested via the conversationId containingowletto-defaultandORG_ID, this constant can be removed.As per coding guidelines, "Fix unused parameters by deleting them, not by prefixing with underscore" — the same principle applies to unused constants.
♻️ Suggested fix
describe("POST /api/v1/agents — default-agent resolution", () => { const ORG_ID = "org-test"; - const USER_ID = "user-test"; // A worker token bound to the caller's org...And remove lines 409-413:
- // Silence the unused-symbol warning for USER_ID — it's part of the - // documented session-id shape (`<agentId>_<userId>_<orgId>`) the test - // asserts on but we don't pin the exact userId since it's derived from - // authContext under the hood. - void USER_ID; });Also applies to: 409-413
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/gateway/__tests__/agent-session-create.test.ts` around lines 106 - 107, Remove the unused USER_ID constant and any residual no-op usage (the `void USER_ID` suppression) from the test; the session ID shape is already asserted via conversationId containing "owletto-default" and the ORG_ID constant, so delete the USER_ID declaration and the corresponding void/suppression lines (the no-op block referencing USER_ID) to satisfy the unused-constant guideline.
🤖 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 `@packages/cli/src/index.ts`:
- Around line 254-257: The CLI help for the .option("--dry-run", ...) is
inconsistent with the server's documented dryRun contract in session.ts; update
the CLI help text to match the server wording (e.g., "Process without persisting
history.") so the .option("--dry-run", ...) description and the dryRun
documentation in session.ts use the same phrasing.
---
Nitpick comments:
In `@packages/server/src/gateway/__tests__/agent-session-create.test.ts`:
- Around line 106-107: Remove the unused USER_ID constant and any residual no-op
usage (the `void USER_ID` suppression) from the test; the session ID shape is
already asserted via conversationId containing "owletto-default" and the ORG_ID
constant, so delete the USER_ID declaration and the corresponding
void/suppression lines (the no-op block referencing USER_ID) to satisfy the
unused-constant guideline.
🪄 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: 56c09898-67e6-4196-9619-6783ceba3a45
📒 Files selected for processing (11)
packages/cli/src/index.tspackages/server/src/__tests__/integration/auth/default-provisioning.test.tspackages/server/src/auth/default-provisioning.tspackages/server/src/gateway/__tests__/agent-session-create.test.tspackages/server/src/gateway/auth/api-auth-middleware.tspackages/server/src/gateway/auth/external/client.tspackages/server/src/gateway/connections/chat-instance-manager.tspackages/server/src/gateway/connections/chat-response-bridge.tspackages/server/src/gateway/routes/public/agent.tspackages/server/src/gateway/services/agent-threads.tspackages/server/src/gateway/session.ts
💤 Files with no reviewable changes (2)
- packages/server/src/gateway/services/agent-threads.ts
- packages/server/src/gateway/session.ts
Comment on lines
+254
to
+257
| .option( | ||
| "--dry-run", | ||
| "Skip side-effecting tool calls (sandbox writes, sdk_run mutations). The turn still runs and history is still persisted." | ||
| ) |
There was a problem hiding this comment.
Align dry-run wording with server contract docs
This help text now says history is persisted, but packages/server/src/gateway/session.ts (Line 49 in snippet context) still documents dryRun as “Process without persisting history.” Please update one side so the contract is unambiguous across CLI and server docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/index.ts` around lines 254 - 257, The CLI help for the
.option("--dry-run", ...) is inconsistent with the server's documented dryRun
contract in session.ts; update the CLI help text to match the server wording
(e.g., "Process without persisting history.") so the .option("--dry-run", ...)
description and the dryRun documentation in session.ts use the same phrasing.
Member
Author
|
Two follow-up commits on top of the earlier review fixes, and a note on the review status. The org-scope suffix was being spliced into watcher conversationIds, so a watcher session id became The second commit closes a gap in the tenant guard. It only fired when the auth context carried an org, which holds for PAT, worker token, and OAuth, but not for the settings-session cookie path. On that path a cookie session for one org could read another org's session through the shared owletto-default agent id. The guard now also compares the org that ownership resolves to against the session's org, so the cookie path is covered. Added two regression tests, both confirmed failing on the pre-fix source and passing after. On the review status: the pi verdict still shown above (bug_free 38) predates these commits, and the cross-tenant blocker it flagged is fixed by them plus the new test. I could not re-run pi to refresh it because the Codex quota is maxed out for a few hours, so this is merging on green CI (21 checks, including sdk-e2e and integration) plus a manual review. |
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
2 participants
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.
Replaces #1129 and #1133.
Summary
lobu chat -c local "ping"in a non-project directory has been broken since the cross-pod conversation-lock guard landed in #1068. Several attempts patched the symptom — this PR removes the broken shape and uses what was already there: every signup auto-provisions a default agent (owletto-default); chat should resolve to it. It just never did.What was wrong
createAgentPOST/api/v1/agentshad an "ephemeral" branch that ran whenever the caller omittedagentId. It did:randomUUID()→ a fresh agent identity per chat callsaveSettings— butsaveSettingsis UPDATE-only, the row had never been INSERTed, the save silently no-op'dThe result: every
lobu chatcall generated a new throwaway UUID, never used the user's actual default agent, and the worker eventually hit "No model configured" becauseinstalled_providers = '[]'for an agent row that didn't exist.Fix
Two paths, no third.
createAgentnow either uses a caller-pinnedagentIdor resolves to the org'sowletto-default. TheisEphemeralfield onThreadSessionand the matching delete-time cleanup branch are gone.ensureDefaultAgentdoes the right thing at provisioning time:installed_providersfrom the set of providers with system keys (env-var API keys, Claude OAuth-discovery) so the default agent is chat-ready out of the box.organization.metadata.personal_org_for_user_idand writes the agent as user-owned with that user (owner_platform = 'external', matching the EmbeddedSettingsSession the PAT path produces).agent_userssoverifyOwnedAgentAccess(PAT-session path) recognizes the user as the agent's owner without a manual UI step.Auth-context plumbing (this is the #1068 follow-up pi flagged on #1133):
createApiAuthMiddlewarenow sets a typedauthContexton the Hono context AND wraps the request inorgContext.run()for the worker-token and OAuth paths.createLobuAuthBridgealready does this for PATs; this closes the gap for the other two methods so any handler that needs the AsyncLocalStorage org id works regardless of auth method.ExternalAuthClient.fetchUserInfonow surfaces the org id resolved from/oauth/userinfo'sorganization_slug+organizations[]list (previously typed away).User-facing error link.
NO_MODEL_CONFIGUREDnow renders asConnect a provider at <publicWebUrl>/<orgSlug>/agents/<agentId>when the org slug can be resolved, instead of vague "ask an admin" text.CLI dry-run help text fix. Was "Process without persisting history" (misleading — history IS persisted). Now "Skip side-effecting tool calls (sandbox writes, sdk_run mutations). The turn still runs and history is still persisted."
Test plan
Reproducer on
origin/main:With this PR:
(The 403 is the unrelated
claude.hasSystemKey()heuristic — Claude returns true for hasSystemKey via OAuth-discovery, so it gets picked first even whenANTHROPIC_API_KEYisn't set. Separate ticket below.)Gateway log confirms the worker actually spawned and picked a provider:
bun run typecheckclean (root + per-package)make build-packagescleanlobu runboot →lobu chat -c local --dry-run "ping"reaches the provider call (provider resolution end-to-end)make review BASE=origin/main— runningConfidence
~90 on this surface. Reproduced both bugs (cross-pod lock throw, then no-providers) on
origin/main, applied the fix, confirmed both errors are gone and the chat path reaches a real model API call. The remaining 403 is documented and out of scope.Follow-ups
claude.hasSystemKey()heuristic: should return false when noANTHROPIC_API_KEYand no OAuth state is present, so the provider-priority loop picks a provider with credentials (Gemini / Z.ai / OpenRouter) when Anthropic isn't usable. Currently the OAuth-discovery-always-true heuristic short-circuits that.lobu chat -c localend-to-end (install_operator → default agent → worker → provider call). This bug existed for ~weeks because no test exercises this path.ensureDefaultAgentalready ran before this change, the row exists butinstalled_providersis'[]'. A small one-shot refresh that adds missing system-key providers (never removes existing ones) would close the gap for those installs — separate PR.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
--dry-runflag help text to clarify that history is persisted.Refactor