Add organization-level model provider configuration#1732
Open
chelojimenez wants to merge 2 commits intomainfrom
Open
Add organization-level model provider configuration#1732chelojimenez wants to merge 2 commits intomainfrom
chelojimenez wants to merge 2 commits intomainfrom
Conversation
…d UI Wire org-scoped model provider configuration throughout the inspector: Server changes: - Add org-model-config.ts utility that calls the backend HTTP resolver to fetch provider configs with decrypted secrets from Vault - Local chat (mcp/chat-v2): resolve API keys from org config when workspaceId is present, falling back to client-sent keys - Hosted chat (web/chat-v2): unblock BYOK by resolving provider config from org when model is not MCPJam-provided - Eval routes: resolve modelApiKeys from org config when not provided by client (runEvals, runTestCase, streamTestCase) - Replay and trace repair: resolve from org config automatically Frontend changes: - Add "Models" tab to organization settings (OrganizationsTab) - Add OrganizationModelsSection for admin provider CRUD via Convex - Add useOrgModelConfig hook backed by Convex query/actions - Add buildAvailableModelsFromOrgConfig to model-helpers for dropdowns - Update SettingsTab to show org-managed notice when org-backed - Update use-chat-session to send workspaceId instead of API keys - Update eval handlers to omit modelApiKeys when workspaceId present - Add workspaceId to ChatV2Request type https://claude.ai/code/session_013YhDfdj193M62kR2oAhHD2
dosubot
Bot
added
the
size:XXL
|
🚅 Deployed to the inspector-pr-1732 environment in triumphant-alignment
|
railway-app
Bot
temporarily deployed
to
triumphant-alignment / inspector-pr-1732
Contributor
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
railway-app
Bot
temporarily deployed
to
triumphant-alignment / inspector-pr-1732
Contributor
WalkthroughThis pull request introduces organization-level model provider management capabilities. The changes add a new "Models" section to organization settings, enabling admins to configure providers (API keys, base URLs, custom providers) at the organization level. A new hook and server utility resolve these provider configurations during chat and evaluation runs when a workspace context is present. Client-side API key collection is conditionally skipped for workspace-backed scenarios, delegating credential resolution to the server. Chat requests and evaluation flows now optionally include workspace identifiers to enable server-side provider resolution. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/web/chat-v2.ts (1)
381-402:⚠️ Potential issue | 🟠 MajorPersisting
sessionMessages: []drops the chat transcript.Line 401 always writes an empty array, and
mcpjam-inspector/server/utils/chat-ingestion.ts:64-106forwardssessionMessagesverbatim. Successful BYOK chats will therefore be saved without any history, which is silent data loss for replay/debug flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/chat-v2.ts` around lines 381 - 402, The persist call in onFinish is currently writing sessionMessages: [] which drops the chat transcript; change persistChatSessionToConvex (in the onFinish handler where hostedChatSessionId is used) to pass the actual messages collection used during the session (e.g., the sessionMessages / messages variable maintained by the chat flow) instead of a literal empty array, making sure that variable is in scope and defaults to [] only if truly undefined so the real transcript is saved for BYOK chats and replay/debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/chat-v2/shared/model-helpers.ts`:
- Around line 95-106: The org-backed availability checks (isOrgProviderAvailable
and the other org predicate around lines 121-131) incorrectly treat Azure like
generic providers because the per-provider readiness rule is only applied for
"ollama"; centralize the per-provider readiness logic into a single helper
(e.g., create or use a function like isProviderReady(provider, getAzureBaseUrl))
that handles special cases such as Azure (using getAzureBaseUrl()) and Ollama
(using baseUrl) and then call that helper from isOrgProviderAvailable and the
other org-backed predicate so Azure readiness is determined identically in both
places.
In
`@mcpjam-inspector/client/src/components/organization/OrganizationModelsSection.tsx`:
- Line 242: The readiness check is inconsistent: the line computing configured
uses !!provider?.hasSecret || !!provider?.baseUrl but canSave/handleSave apply
different validation per provider kind (e.g., azure and openrouter), so partial
entries can appear "configured" when they are not usable. Add a
provider-kind-specific readiness helper (e.g., isProviderReadyForSave(provider)
or computeReadinessForKind(kind, provider)) that performs the exact same
validation used in canSave/handleSave (filtering openrouter inputs, requiring
both secret+url for azure, etc.), call that helper once when preparing the
provider list, and reuse its result for both the save guard (canSave) and the
configured/badge computation (replace configured = !!provider?.hasSecret ||
!!provider?.baseUrl with the helper result); update any other spots (lines
~524-548) where readiness is derived to use this same helper.
- Around line 743-754: The providerKey derived from trimmedName can collide with
existing normalized keys (e.g., "Acme AI" -> "custom-acme-ai") and may cause
upsertProvider to overwrite another provider; add an existingProviderKeys prop
to OrganizationModelsSection and, before calling onSave/upsertProvider in the
save handler, compute the normalized key exactly as you do now (const
providerKey = editProvider ? editProvider.providerKey :
`custom-${trimmedName.toLowerCase().replace(/\s+/g, "-")}`) and check it against
existingProviderKeys; if not editing (or editing but the computed providerKey
!== editProvider.providerKey) and the key exists, reject the save by setting an
error state/validation message and do not call onSave/upsertProvider so the
duplicate is prevented.
- Around line 451-469: The icon-only edit/delete buttons in
OrganizationModelsSection (the Button with onClick={onConfigure} rendering
<Pencil /> and the Button with onClick={onRemove} rendering <Trash2 />) lack
accessible names; update those Buttons to include explicit accessible labels
(e.g., add aria-label="Edit provider" to the onConfigure icon-only Button and
aria-label="Remove provider" to the onRemove Button), or alternatively render
visually hidden text inside the Buttons when configured is true to provide a
readable label for assistive tech.
In `@mcpjam-inspector/server/routes/mcp/chat-v2.ts`:
- Around line 200-204: The handler currently accepts caller-supplied workspaceId
and immediately calls resolveOrgModelConfig and resolveProviderForModel,
allowing unauthorized access to org provider credentials; before calling
resolveOrgModelConfig/resolveProviderForModel, verify the requester is
authorized for body.workspaceId (e.g., resolve the requester’s identity from the
request/token and call your workspace authorization check such as
authorizeWorkspaceAccess(request.user or tokenSubject, body.workspaceId) or
fetch the workspace from the authenticated session), reject the request if not
authorized, and only then proceed to call resolveOrgModelConfig and
resolveProviderForModel.
- Around line 200-214: The org-resolution branch currently lets exceptions from
resolveOrgModelConfig/resolveProviderForModel bubble out and cause a 500;
instead wrap the org resolution in a try/catch around resolveOrgModelConfig(...)
and resolveProviderForModel(...), and on any error assign the documented
fallback values (set resolvedApiKey = apiKey ?? "", resolvedBaseUrls = { ollama:
body.ollamaBaseUrl, azure: body.azureBaseUrl }, and clear or leave
resolvedCustomProviders undefined) so the handler degrades to client-provided
model config; include a brief log of the error before falling back.
In `@mcpjam-inspector/server/routes/shared/evals.ts`:
- Around line 503-515: When resolving model API keys, do not swallow resolver
failures: in the block using resolvedModelApiKeys/modelApiKeys call
resolveOrgModelConfig and buildModelApiKeysFromOrgConfig but if
resolveOrgModelConfig throws, rethrow (or throw a new Error wrapping the
resolver error) instead of only logger.warn so callers see the real outage;
update all identical patterns (the other blocks that call
resolveOrgModelConfig/buildModelApiKeysFromOrgConfig and currently logger.warn)
to propagate the error rather than continuing with undefined keys, ensuring the
runner receives the resolver error instead of a generic "Missing API key".
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Around line 356-370: The BYOK branch currently assumes resolveOrgModelConfig
always succeeds; wrap the org config resolution and subsequent
resolveProviderForModel/createLlmModel calls in a try/catch (or check for a
falsy orgConfig) and on failure fall back to using the caller-provided
credentials (hostedBody.apiKey, hostedBody.baseUrls, hostedBody.customProviders)
before calling createLlmModel; use the same fallback semantics as in
resolveProviderForModel/shared evals (see resolveOrgModelConfig,
resolveProviderForModel, createLlmModel, and hostedBody) so transient or missing
org config does not block BYOK chats.
- Around line 405-408: Replace the console.error call in the BYOK persistence
catch block with the centralized server logger: import logger from
'@/utils/logger' if not already imported, then call logger.error(...) instead of
console.error(...) and pass the same error and context message ("Failed to
persist BYOK chat session") so the failure emits structured logs via
logger.error in the catch block that currently contains console.error("Failed to
persist BYOK chat session", e).
In `@mcpjam-inspector/server/utils/org-model-config.ts`:
- Around line 147-151: The branch in buildModelApiKeysFromOrgConfig that is
supposed to strip the "custom:" prefix is currently a no-op; update the logic
that computes key from p.providerKey (used in buildModelApiKeysFromOrgConfig and
referencing p and providerKey) so that when p.providerKey.startsWith("custom:")
you remove the "custom:" prefix (e.g., by slicing off the prefix) and otherwise
keep p.providerKey unchanged, ensuring custom provider names are normalized
before lookup.
---
Outside diff comments:
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Around line 381-402: The persist call in onFinish is currently writing
sessionMessages: [] which drops the chat transcript; change
persistChatSessionToConvex (in the onFinish handler where hostedChatSessionId is
used) to pass the actual messages collection used during the session (e.g., the
sessionMessages / messages variable maintained by the chat flow) instead of a
literal empty array, making sure that variable is in scope and defaults to []
only if truly undefined so the real transcript is saved for BYOK chats and
replay/debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bc63d62-61cc-4894-877d-b30f3d4bf045
📒 Files selected for processing (17)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/OrganizationsTab.tsxmcpjam-inspector/client/src/components/SettingsTab.tsxmcpjam-inspector/client/src/components/chat-v2/shared/model-helpers.tsmcpjam-inspector/client/src/components/evals/single-test-case-runner.tsmcpjam-inspector/client/src/components/evals/use-eval-handlers.tsmcpjam-inspector/client/src/components/organization/OrganizationModelsSection.tsxmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/use-org-model-config.tsmcpjam-inspector/client/src/lib/hosted-navigation.tsmcpjam-inspector/server/routes/mcp/chat-v2.tsmcpjam-inspector/server/routes/shared/evals.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/services/evals/replay-suite-run.tsmcpjam-inspector/server/services/evals/trace-repair-runner.tsmcpjam-inspector/server/utils/org-model-config.tsmcpjam-inspector/shared/chat-v2.ts
Comment on lines
+95
to
+106
| export function isOrgProviderAvailable( | ||
| orgConfig: OrgVisibleConfig | undefined, | ||
| providerKey: string, | ||
| ): boolean { | ||
| if (!orgConfig?.providers) return false; | ||
| return orgConfig.providers.some((p) => { | ||
| if (p.providerKey !== providerKey) return false; | ||
| if (!p.enabled) return false; | ||
| // Ollama only needs baseUrl, not a secret | ||
| if (p.providerKey === "ollama") return Boolean(p.baseUrl); | ||
| return p.hasSecret; | ||
| }); |
Contributor
There was a problem hiding this comment.
Org-backed availability checks don't mirror the existing Azure readiness rule.
The local builder on Line 47 exposes Azure models when getAzureBaseUrl() is set, but both org-backed predicates switch Azure to the generic hasSecret path because Line 103 only special-cases ollama. That divergence means org-backed workspaces can hide a valid Azure setup or advertise Azure models that still have no runnable base URL. Please centralize the per-provider readiness rule and reuse it in both helpers.
Also applies to: 121-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/chat-v2/shared/model-helpers.ts`
around lines 95 - 106, The org-backed availability checks
(isOrgProviderAvailable and the other org predicate around lines 121-131)
incorrectly treat Azure like generic providers because the per-provider
readiness rule is only applied for "ollama"; centralize the per-provider
readiness logic into a single helper (e.g., create or use a function like
isProviderReady(provider, getAzureBaseUrl)) that handles special cases such as
Azure (using getAzureBaseUrl()) and Ollama (using baseUrl) and then call that
helper from isOrgProviderAvailable and the other org-backed predicate so Azure
readiness is determined identically in both places.
| <> | ||
| {PROVIDER_CATALOG.map((entry) => { | ||
| const provider = providerMap.get(entry.key); | ||
| const configured = !!provider?.hasSecret || !!provider?.baseUrl; |
Contributor
There was a problem hiding this comment.
Don't treat partial Azure/OpenRouter entries as configured.
azure can be saved with only a base URL, and openrouter will accept comma-only input because canSave checks the raw string before handleSave filters it down. The list badge then reduces readiness to hasSecret || baseUrl, so the org can appear configured while requests still fail or fall back to user credentials. Derive readiness once per provider kind and reuse it for both the save guard and the badge.
🩹 Tighten the readiness checks
+ const parsedSelectedModels = selectedModels
+ .split(",")
+ .map((s) => s.trim())
+ .filter(Boolean);
+
const handleSave = () => {
const args: Parameters<typeof onSave>[0] = { providerKey };
if (secret.trim()) args.secret = secret.trim();
if (baseUrl.trim()) args.baseUrl = baseUrl.trim();
- if (kind === "openrouter" && selectedModels.trim()) {
- args.selectedModels = selectedModels
- .split(",")
- .map((s) => s.trim())
- .filter(Boolean);
+ if (kind === "openrouter" && parsedSelectedModels.length > 0) {
+ args.selectedModels = parsedSelectedModels;
}
void onSave(args);
};
const canSave = (() => {
switch (kind) {
case "api-key-only":
return !!secret.trim() || existing?.hasSecret;
case "azure":
- return !!baseUrl.trim();
+ return !!baseUrl.trim() && (!!secret.trim() || existing?.hasSecret);
case "ollama":
return !!baseUrl.trim();
case "openrouter":
- return (
- (!!secret.trim() || existing?.hasSecret) && !!selectedModels.trim()
- );
+ return (!!secret.trim() || existing?.hasSecret) && parsedSelectedModels.length > 0;
default:
return false;
}
})();Then reuse the same provider-kind-specific helper when computing configured in the main list.
Also applies to: 524-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/organization/OrganizationModelsSection.tsx`
at line 242, The readiness check is inconsistent: the line computing configured
uses !!provider?.hasSecret || !!provider?.baseUrl but canSave/handleSave apply
different validation per provider kind (e.g., azure and openrouter), so partial
entries can appear "configured" when they are not usable. Add a
provider-kind-specific readiness helper (e.g., isProviderReadyForSave(provider)
or computeReadinessForKind(kind, provider)) that performs the exact same
validation used in canSave/handleSave (filtering openrouter inputs, requiring
both secret+url for azure, etc.), call that helper once when preparing the
provider list, and reuse its result for both the save guard (canSave) and the
configured/badge computation (replace configured = !!provider?.hasSecret ||
!!provider?.baseUrl with the helper result); update any other spots (lines
~524-548) where readiness is derived to use this same helper.
Comment on lines
+451
to
+469
| <Button variant="ghost" size="sm" onClick={onConfigure}> | ||
| {configured ? ( | ||
| <Pencil className="size-3.5" /> | ||
| ) : ( | ||
| <> | ||
| <Settings2 className="mr-1.5 size-3.5" /> | ||
| Configure | ||
| </> | ||
| )} | ||
| </Button> | ||
| {configured ? ( | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={onRemove} | ||
| className="text-destructive hover:text-destructive hover:bg-destructive/10" | ||
| > | ||
| <Trash2 className="size-3.5" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Give the icon-only actions an accessible name.
Once a provider is configured, the edit and delete controls collapse to bare icons, so assistive tech gets unlabeled buttons. Add an aria-label or visually hidden text to both actions.
♿ Add explicit labels
- <Button variant="ghost" size="sm" onClick={onConfigure}>
+ <Button
+ variant="ghost"
+ size="sm"
+ onClick={onConfigure}
+ aria-label={
+ configured
+ ? `Edit ${name} provider`
+ : `Configure ${name} provider`
+ }
+ >
{configured ? (
<Pencil className="size-3.5" />
) : (
<>
<Settings2 className="mr-1.5 size-3.5" />
@@
<Button
variant="ghost"
size="sm"
onClick={onRemove}
+ aria-label={`Remove ${name} provider`}
className="text-destructive hover:text-destructive hover:bg-destructive/10"
>
<Trash2 className="size-3.5" />
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button variant="ghost" size="sm" onClick={onConfigure}> | |
| {configured ? ( | |
| <Pencil className="size-3.5" /> | |
| ) : ( | |
| <> | |
| <Settings2 className="mr-1.5 size-3.5" /> | |
| Configure | |
| </> | |
| )} | |
| </Button> | |
| {configured ? ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| onClick={onRemove} | |
| className="text-destructive hover:text-destructive hover:bg-destructive/10" | |
| > | |
| <Trash2 className="size-3.5" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| onClick={onConfigure} | |
| aria-label={ | |
| configured | |
| ? `Edit ${name} provider` | |
| : `Configure ${name} provider` | |
| } | |
| > | |
| {configured ? ( | |
| <Pencil className="size-3.5" /> | |
| ) : ( | |
| <> | |
| <Settings2 className="mr-1.5 size-3.5" /> | |
| Configure | |
| </> | |
| )} | |
| </Button> | |
| {configured ? ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| onClick={onRemove} | |
| aria-label={`Remove ${name} provider`} | |
| className="text-destructive hover:text-destructive hover:bg-destructive/10" | |
| > | |
| <Trash2 className="size-3.5" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/organization/OrganizationModelsSection.tsx`
around lines 451 - 469, The icon-only edit/delete buttons in
OrganizationModelsSection (the Button with onClick={onConfigure} rendering
<Pencil /> and the Button with onClick={onRemove} rendering <Trash2 />) lack
accessible names; update those Buttons to include explicit accessible labels
(e.g., add aria-label="Edit provider" to the onConfigure icon-only Button and
aria-label="Remove provider" to the onRemove Button), or alternatively render
visually hidden text inside the Buttons when configured is true to provide a
readable label for assistive tech.
Comment on lines
+743
to
+754
| // Use lowercase name as providerKey for new providers, keep existing key for edits | ||
| const providerKey = editProvider | ||
| ? editProvider.providerKey | ||
| : `custom-${trimmedName.toLowerCase().replace(/\s+/g, "-")}`; | ||
|
|
||
| const args: Parameters<typeof onSave>[0] = { | ||
| providerKey, | ||
| displayName: trimmedName, | ||
| protocol, | ||
| baseUrl: trimmedBaseUrl, | ||
| modelIds: parsedModelIds, | ||
| }; |
Contributor
There was a problem hiding this comment.
Reject normalized-name collisions before calling upsertProvider.
The add path derives providerKey from a normalized display name, so values like Acme AI and acme-ai both become custom-acme-ai. Because this flows into upsertProvider, a new provider can silently overwrite an existing custom config instead of creating a second one.
🧭 Fail fast on duplicate normalized keys
const providerKey = editProvider
? editProvider.providerKey
: `custom-${trimmedName.toLowerCase().replace(/\s+/g, "-")}`;
+
+ if (!editProvider && existingProviderKeys.has(providerKey)) {
+ setValidationError("A provider with this name already exists");
+ return;
+ }
const args: Parameters<typeof onSave>[0] = {This needs an existingProviderKeys prop from the parent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/organization/OrganizationModelsSection.tsx`
around lines 743 - 754, The providerKey derived from trimmedName can collide
with existing normalized keys (e.g., "Acme AI" -> "custom-acme-ai") and may
cause upsertProvider to overwrite another provider; add an existingProviderKeys
prop to OrganizationModelsSection and, before calling onSave/upsertProvider in
the save handler, compute the normalized key exactly as you do now (const
providerKey = editProvider ? editProvider.providerKey :
`custom-${trimmedName.toLowerCase().replace(/\s+/g, "-")}`) and check it against
existingProviderKeys; if not editing (or editing but the computed providerKey
!== editProvider.providerKey) and the key exists, reject the save by setting an
error state/validation message and do not call onSave/upsertProvider so the
duplicate is prevented.
Comment on lines
+200
to
+204
| if (body.workspaceId) { | ||
| const orgConfig = await resolveOrgModelConfig({ | ||
| workspaceId: body.workspaceId, | ||
| }); | ||
| const resolved = resolveProviderForModel(orgConfig, modelDefinition); |
Contributor
There was a problem hiding this comment.
Authorize workspaceId before resolving org credentials.
Line 200 trusts a caller-supplied workspaceId and immediately resolves org-scoped provider config with the service token. This handler never proves the requester belongs to that workspace, so a crafted request that knows another workspace ID can make chat run against that org’s provider configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/chat-v2.ts` around lines 200 - 204, The
handler currently accepts caller-supplied workspaceId and immediately calls
resolveOrgModelConfig and resolveProviderForModel, allowing unauthorized access
to org provider credentials; before calling
resolveOrgModelConfig/resolveProviderForModel, verify the requester is
authorized for body.workspaceId (e.g., resolve the requester’s identity from the
request/token and call your workspace authorization check such as
authorizeWorkspaceAccess(request.user or tokenSubject, body.workspaceId) or
fetch the workspace from the authenticated session), reject the request if not
authorized, and only then proceed to call resolveOrgModelConfig and
resolveProviderForModel.
Comment on lines
+200
to
+214
| if (body.workspaceId) { | ||
| const orgConfig = await resolveOrgModelConfig({ | ||
| workspaceId: body.workspaceId, | ||
| }); | ||
| const resolved = resolveProviderForModel(orgConfig, modelDefinition); | ||
| resolvedApiKey = resolved.apiKey; | ||
| resolvedBaseUrls = resolved.baseUrls; | ||
| resolvedCustomProviders = resolved.customProviders; | ||
| } else { | ||
| resolvedApiKey = apiKey ?? ""; | ||
| resolvedBaseUrls = { | ||
| ollama: body.ollamaBaseUrl, | ||
| azure: body.azureBaseUrl, | ||
| }, | ||
| body.customProviders, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Restore the documented fallback path on resolver errors.
Any exception in this branch bubbles to the outer catch and returns 500, so org-backed BYOK chat goes hard-down on transient resolver failures instead of degrading gracefully to client-provided model config. That contradicts the PR’s stated fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/chat-v2.ts` around lines 200 - 214, The
org-resolution branch currently lets exceptions from
resolveOrgModelConfig/resolveProviderForModel bubble out and cause a 500;
instead wrap the org resolution in a try/catch around resolveOrgModelConfig(...)
and resolveProviderForModel(...), and on any error assign the documented
fallback values (set resolvedApiKey = apiKey ?? "", resolvedBaseUrls = { ollama:
body.ollamaBaseUrl, azure: body.azureBaseUrl }, and clear or leave
resolvedCustomProviders undefined) so the handler degrades to client-provided
model config; include a brief log of the error before falling back.
Comment on lines
+503
to
+515
| // Resolve model API keys: prefer client-sent keys, fall back to org config | ||
| let resolvedModelApiKeys = modelApiKeys; | ||
| if (!resolvedModelApiKeys && workspaceId) { | ||
| try { | ||
| const orgConfig = await resolveOrgModelConfig({ workspaceId }); | ||
| resolvedModelApiKeys = buildModelApiKeysFromOrgConfig(orgConfig); | ||
| } catch (error) { | ||
| logger.warn("[evals] Failed to resolve org model config", { | ||
| workspaceId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate org-resolution failures instead of deferring them.
Lines 503, 590, and 795 only log and continue. When modelApiKeys is absent, the downstream runner fails fast on the first missing provider key anyway, so users get a generic “Missing API key” error instead of the real org-config outage. Please fail here with the resolver error, or keep a genuine fallback credential source alive.
Also applies to: 590-604, 795-813
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/shared/evals.ts` around lines 503 - 515, When
resolving model API keys, do not swallow resolver failures: in the block using
resolvedModelApiKeys/modelApiKeys call resolveOrgModelConfig and
buildModelApiKeysFromOrgConfig but if resolveOrgModelConfig throws, rethrow (or
throw a new Error wrapping the resolver error) instead of only logger.warn so
callers see the real outage; update all identical patterns (the other blocks
that call resolveOrgModelConfig/buildModelApiKeysFromOrgConfig and currently
logger.warn) to propagate the error rather than continuing with undefined keys,
ensuring the runner receives the resolver error instead of a generic "Missing
API key".
Comment on lines
+356
to
+370
| // BYOK model — resolve provider config from org | ||
| const orgConfig = await resolveOrgModelConfig({ | ||
| workspaceId: hostedBody.workspaceId, | ||
| }); | ||
| const { | ||
| apiKey: resolvedKey, | ||
| baseUrls, | ||
| customProviders: resolvedCustomProviders, | ||
| } = resolveProviderForModel(orgConfig, modelDefinition); | ||
| const llmModel = createLlmModel( | ||
| modelDefinition, | ||
| resolvedKey, | ||
| baseUrls, | ||
| resolvedCustomProviders, | ||
| ); |
Contributor
There was a problem hiding this comment.
This BYOK branch never performs the advertised fallback when org resolution fails.
Line 357 turns org config lookup into a hard dependency for every non-MCPJam chat. Unlike mcpjam-inspector/server/routes/shared/evals.ts:505-515, there is no guarded fallback here, so any transient lookup failure or missing org config now fails BYOK chats outright instead of degrading to caller-supplied credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/chat-v2.ts` around lines 356 - 370, The
BYOK branch currently assumes resolveOrgModelConfig always succeeds; wrap the
org config resolution and subsequent resolveProviderForModel/createLlmModel
calls in a try/catch (or check for a falsy orgConfig) and on failure fall back
to using the caller-provided credentials (hostedBody.apiKey,
hostedBody.baseUrls, hostedBody.customProviders) before calling createLlmModel;
use the same fallback semantics as in resolveProviderForModel/shared evals (see
resolveOrgModelConfig, resolveProviderForModel, createLlmModel, and hostedBody)
so transient or missing org config does not block BYOK chats.
Comment on lines
+405
to
408
| } catch (e) { | ||
| // Non-fatal — log but don't break the stream | ||
| console.error("Failed to persist BYOK chat session", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Use the server logger here.
This failure path should emit structured logs through logger.error(...), not console.error(...).
Suggested change
+import { logger } from "../../utils/logger.js";
@@
- } catch (e) {
- // Non-fatal — log but don't break the stream
- console.error("Failed to persist BYOK chat session", e);
+ } catch (e) {
+ // Non-fatal — log but don't break the stream
+ logger.error("[chat-v2] Failed to persist BYOK chat session", {
+ chatSessionId: hostedChatSessionId,
+ workspaceId: hostedBody.workspaceId,
+ error: e instanceof Error ? e.message : String(e),
+ });
}As per coding guidelines, mcpjam-inspector/server/**/*.{ts,tsx,js}: Do not use console.log, console.warn, or console.error directly in server code. Use the centralized logger utility from @/utils/logger instead with methods like logger.error(), logger.warn(), logger.info(), and logger.debug()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (e) { | |
| // Non-fatal — log but don't break the stream | |
| console.error("Failed to persist BYOK chat session", e); | |
| } | |
| import { logger } from "../../utils/logger.js"; | |
| } catch (e) { | |
| // Non-fatal — log but don't break the stream | |
| logger.error("[chat-v2] Failed to persist BYOK chat session", { | |
| chatSessionId: hostedChatSessionId, | |
| workspaceId: hostedBody.workspaceId, | |
| error: e instanceof Error ? e.message : String(e), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/chat-v2.ts` around lines 405 - 408,
Replace the console.error call in the BYOK persistence catch block with the
centralized server logger: import logger from '@/utils/logger' if not already
imported, then call logger.error(...) instead of console.error(...) and pass the
same error and context message ("Failed to persist BYOK chat session") so the
failure emits structured logs via logger.error in the catch block that currently
contains console.error("Failed to persist BYOK chat session", e).
Comment on lines
+147
to
+151
| // Strip "custom:" prefix for custom providers — the eval runner | ||
| // looks up by provider name (e.g. "anthropic", "openai") | ||
| const key = p.providerKey.startsWith("custom:") | ||
| ? p.providerKey | ||
| : p.providerKey; |
Contributor
There was a problem hiding this comment.
Normalize custom-provider keys correctly.
Line 149 is a no-op today: both branches return p.providerKey. That means buildModelApiKeysFromOrgConfig() never performs the custom: normalization its comment promises, so org-backed evals for custom providers can miss their API key entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/utils/org-model-config.ts` around lines 147 - 151,
The branch in buildModelApiKeysFromOrgConfig that is supposed to strip the
"custom:" prefix is currently a no-op; update the logic that computes key from
p.providerKey (used in buildModelApiKeysFromOrgConfig and referencing p and
providerKey) so that when p.providerKey.startsWith("custom:") you remove the
"custom:" prefix (e.g., by slicing off the prefix) and otherwise keep
p.providerKey unchanged, ensuring custom provider names are normalized before
lookup.
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.
Summary
This PR adds organization-level configuration for AI model providers, allowing admins to manage API keys and provider settings centrally that are shared across all organization members. This enables org-backed workspaces to resolve model credentials from the organization config instead of requiring individual user setup.
Key Changes
Client-Side Components
OrganizationModelsSection: New component for managing provider configurations with support for:
Model selection helpers: Added
buildAvailableModelsFromOrgConfig()andisOrgProviderAvailable()to determine available models based on org provider configurationNavigation: Extended organization routes to include
/modelssection alongside existing overview and billing sectionsSettings integration: Updated SettingsTab to accept organization context and conditionally hide local provider configuration when in org-backed mode
Server-Side Utilities
resolveOrgModelConfig(): Fetches org provider configuration from Convex via HTTP endpointresolveProviderForModel(): Maps model definitions to resolved provider credentialsbuildModelApiKeysFromOrgConfig(): Extracts API keys for eval/test case executionIntegration Points
workspaceIdis present, falling back to client-sent credentialsType Updates
ChatV2Requestto include optionalworkspaceIdparameterOrgModelProviderinterface for provider configuration stateOrganizationRouteSectiontype to support "models" routeImplementation Details
selectedModelsfieldhttps://claude.ai/code/session_013YhDfdj193M62kR2oAhHD2
Note
High Risk
Touches credential resolution for chat and eval execution by introducing org-backed provider config lookup (via service token/Convex HTTP), which is security- and reliability-sensitive and could break BYOK model access if misconfigured.
Overview
Adds an organization-level “Models” settings area where admins can configure shared model providers (including OpenRouter selections and custom providers) and routes it under
organizations/:id/models.Updates the client to treat org-backed contexts differently:
SettingsTabhides per-user/local provider keys and links to org models, model availability can be derived from org provider config, and chat/evals requests can omit client API keys.Updates server chat (
mcp+web) and eval/repair/replay flows to acceptworkspaceIdinChatV2Requestand, when present, resolve provider API keys/base URLs from org config (via newutils/org-model-config.ts) with fallback to client-sent credentials.Reviewed by Cursor Bugbot for commit b7691b0. Bugbot is set up for automated code reviews on this repo. Configure here.