fix(cli,server): apply takes effect without lobu run restart#993
Conversation
A freshly-applied provider/model now takes effect for the next chat session against a running embedded server, with no restart required. Root cause: BaseProviderModule.hasCredentials only checked per-user auth_profiles, ignoring the org-shared API key that `lobu apply` writes to agent_secrets (provider:<id>:apiKey). buildEnvVars already falls back to that org-shared key, but hasCredentials did not — so the session- context primary-provider detection (resolveProviderConfig) found no credentialed provider and returned defaultProvider: none. With a bare model ref like "glm-4.6" (no provider/ prefix, the format the CLI and UI both store) the worker's resolveModelRef then threw "No provider specified for model", and the model never updated until lobu run was restarted. Fix: - hasCredentials now also recognizes an org-shared provider key (same source buildEnvVars already uses), so apply-provisioned providers report credentials. - Thread the worker token's organizationId into resolveProviderConfig and its hasCredentials calls so the org-shared lookup resolves reliably on the worker session-context path (which runs outside the org-scoped HTTP middleware's AsyncLocalStorage). Settings are read fresh from Postgres per message (no caching), so once the provider resolves, both the first session and any re-applied model take effect on the next session without a restart.
📝 WalkthroughWalkthroughThis PR extends provider credential resolution with organization-scoped fallback: when a user has no per-profile API key configured, ChangesOrganization-Aware Provider Credentials
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/server/src/gateway/auth/__tests__/base-provider-module-credentials.test.ts (2)
25-27: ⚡ Quick winMake the DB mock validate the org-scoped lookup inputs.
This stub returns
orgSharedSecretRowsfor every tagged-template call, so the suite still goes green ifreadOrgSharedProviderKey()uses the wrongorganization_id/secret name or accidentally ignorescontext.organizationIdand falls back to ALS. Making the mock inspect the interpolated values would turn these into real contract tests for the new fallback path.🤖 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/auth/__tests__/base-provider-module-credentials.test.ts` around lines 25 - 27, The DB mock currently always returns orgSharedSecretRows regardless of the tagged-template inputs; update the mock.module replacement for "../../../db/client.js" so getDb returns a function that inspects the tagged-template call arguments (the template strings and interpolated values) used by readOrgSharedProviderKey and validates the organization_id and secret name (and that context.organizationId is being used rather than ALS) against expected values, and throw or return an empty result when they don't match so tests fail when the wrong org/secret or fallback is used.
29-30: ⚡ Quick winMove the dynamic import into an allow-listed test hook.
Top-level
await import(...)is outside the repo's permitted test pattern. Loading this module inbeforeAll/beforeEachaftermock.module(...)keeps the mock ordering and matches the guideline.As per coding guidelines,
Use static import by default; restrict await import(...) (dynamic imports) to the allow-listed locations onlyandIn test files, await import(...) inside beforeAll, beforeEach, or test() blocks is allowed (load after vi.mock(...)) following the vitest pattern.🤖 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/auth/__tests__/base-provider-module-credentials.test.ts` around lines 29 - 30, The dynamic top-level await import of ApiKeyProviderModule should be moved into an allow-listed test hook (e.g., beforeAll or beforeEach) so mocks are registered first; update the test to call await import("../api-key-provider-module.js") inside beforeAll/ beforeEach after any vi.mock(...) or mock.module(...) calls and assign the imported ApiKeyProviderModule to the same variable used by the tests so the module graph picks up mocks correctly.
🤖 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
`@packages/server/src/gateway/auth/__tests__/base-provider-module-credentials.test.ts`:
- Around line 25-27: The DB mock currently always returns orgSharedSecretRows
regardless of the tagged-template inputs; update the mock.module replacement for
"../../../db/client.js" so getDb returns a function that inspects the
tagged-template call arguments (the template strings and interpolated values)
used by readOrgSharedProviderKey and validates the organization_id and secret
name (and that context.organizationId is being used rather than ALS) against
expected values, and throw or return an empty result when they don't match so
tests fail when the wrong org/secret or fallback is used.
- Around line 29-30: The dynamic top-level await import of ApiKeyProviderModule
should be moved into an allow-listed test hook (e.g., beforeAll or beforeEach)
so mocks are registered first; update the test to call await
import("../api-key-provider-module.js") inside beforeAll/ beforeEach after any
vi.mock(...) or mock.module(...) calls and assign the imported
ApiKeyProviderModule to the same variable used by the tests so the module graph
picks up mocks correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed1350b8-008d-4f29-a5b9-cede6671c0a3
📒 Files selected for processing (3)
packages/server/src/gateway/auth/__tests__/base-provider-module-credentials.test.tspackages/server/src/gateway/auth/base-provider-module.tspackages/server/src/gateway/gateway/index.ts
|
bug_free 84, simplicity 88, slop 0, bugs 0, 0 blockers Typecheck and unit suites passed. [env] Integration suite failed before exercising diff because DATABASE_URL pointed at database "postgres" and the harness refused destructive setup. Ran new targeted test packages/server/src/gateway/auth/tests/base-provider-module-credentials.test.ts: 4 pass. Exploratory server boot failed because process env lacked DATABASE_URL. Full verdict JSON{
"bug_free_confidence": 84,
"bugs": 0,
"slop": 0,
"simplicity": 88,
"blockers": [],
"change_type": "fix",
"behavior_change_risk": "medium",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "Typecheck and unit suites passed. [env] Integration suite failed before exercising diff because DATABASE_URL pointed at database \"postgres\" and the harness refused destructive setup. Ran new targeted test packages/server/src/gateway/auth/__tests__/base-provider-module-credentials.test.ts: 4 pass. Exploratory server boot failed because process env lacked DATABASE_URL.",
"categories": {
"src": 23,
"tests": 101,
"docs": 0,
"config": 0,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
2 participants
Problem
After
lobu applyagainst a running embeddedlobu runserver, a newlobu chatsession kept using the previously-selected provider/model untillobu runwas restarted. In fact, anlobu apply-provisioned provider often never resolved at all — the worker errored withNo provider specified for model "glm-4.6".Root cause
BaseProviderModule.hasCredentialsonly checked per-userauth_profiles. It ignored the org-shared API key thatlobu applywrites toagent_secrets(provider:<id>:apiKey).buildEnvVarsalready falls back to that org-shared key when injecting credentials, buthasCredentialsdid not.As a result, the session-context primary-provider detection in
resolveProviderConfig(gateway/index.ts) found no credentialed provider and returneddefaultProvider: none. The CLI and the web UI both store model preferences as a bare model ref (e.g.glm-4.6, noprovider/prefix), so the worker'sresolveModelRef("glm-4.6", { defaultProvider: "" })threwNo provider specified for model "glm-4.6". The model was effectively pinned to whatever the first successful resolution produced until the gateway restarted.Agent settings are read fresh from Postgres per message (
resolveAgentOptions→getSettings, no caching), so once the provider resolves correctly, both the first session and any re-applied model take effect on the next session with no restart.Fix
hasCredentialsnow also recognizes an org-shared provider key (the same sourcebuildEnvVarsalready uses), solobu apply-provisioned providers report credentials.organizationIdintoresolveProviderConfigand itshasCredentialscalls. The worker session-context endpoint runs outside the org-scoped HTTP middleware's AsyncLocalStorage, so passing the org explicitly makes theagent_secretslookup reliable.Reproducer (red → green)
Embedded
lobu runon a clean DB, agenthotreload-testwith one z-ai provider,model = "glm-4.6". Single server process throughout (workerlobu-worker-api-4c5447d606e), no restart.RED (before fix) — first chat after apply:
GREEN (after fix) — chat 1 (
glm-4.6), then editlobu.toml→glm-4.5,lobu applyagainst the running server (incremental~ settings hotreload-test (providerModelPreferences)), then a new chat — nolobu runrestart:Model switched
glm-4.6→glm-4.5for the next session with no restart.Validation
bunx tsc --noEmit(packages/server) — clean.make build-packages+packages/clibuild — green.base-provider-module-credentials.test.ts(4 pass) covers: profile present, org-shared key only (the apply path, via explicit org + ALS fallback), neither present, and no-org short-circuit.lobu run(Node 22, z-ai / glm-4.6 → glm-4.5).Note: the 2 pre-existing failures in
api-auth-middleware.test.ts(ENCRYPTION_KEY rotation / agent-scoped token isolation) fail identically at HEAD and don't reference any code in this PR.Summary by CodeRabbit