feat(platform): add MCP server management UI and backend#1408
Merged
Conversation
Contributor
Author
|
@coderabbitai review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request adds comprehensive MCP (Model Context Protocol) server support to the platform. It introduces a new settings page for managing MCP servers with React components for displaying, creating, editing, and deleting servers. The implementation supports multiple transport types (stdio, SSE, HTTP) and authentication methods (API key, OAuth2). Backend Convex queries and mutations handle server management, data persistence, and authorization checks. MCP-discovered tools are integrated into agent generation as bound tools. Localization support is provided for English and German UI text, and comprehensive test coverage is included for components and backend logic. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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)
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 |
Add settings page for managing MCP (Model Context Protocol) server connections with full CRUD operations, connection testing, and agent integration. Includes shared Zod validation schemas, encrypted credential storage, and i18n support for EN/DE locales. Closes #1347
larryro
force-pushed
the
feat/issue-1347-mcp-ui
branch
from
ea657f1 to
c1c1679
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/convex/lib/agent_chat/internal_actions.ts (1)
332-342: 🧹 Nitpick | 🔵 TrivialRun MCP discovery in the existing parallel batch.
listActiveByOrgis currently awaited after the firstPromise.all, which adds one serial DB read to every generation. This can be merged into the same parallel batch to reduce per-request latency.♻️ Suggested refactor
+ const buildMcpTools = async (): Promise<Record<string, unknown> | undefined> => { + interface ActiveMcpServer { + _id: string; + name: string; + displayName: string; + discoveredTools?: Array<{ + name: string; + description?: string; + inputSchema?: Record<string, unknown>; + requiresApproval?: boolean; + }>; + } + + const activeServers: ActiveMcpServer[] = await ctx.runQuery( + internal.mcp_servers.internal_queries.listActiveByOrg, + { organizationId }, + ); + if (!activeServers.length) return undefined; + + const tools: Record<string, unknown> = {}; + for (const server of activeServers) { + for (const tool of server.discoveredTools ?? []) { + tools[`mcp_${server.name}_${tool.name}`] = createBoundMcpTool( + server._id, + server.displayName, + tool, + ); + } + } + return Object.keys(tools).length ? tools : undefined; + }; + const [ integrationExtraTools, delegationResult, workflowExtraTools, governanceResult, + mcpExtraTools, ] = await Promise.all([ buildIntegrationTools(), buildDelegationTools(), buildWorkflowTools(), fetchGovernancePolicy(), + buildMcpTools(), ]); - - // Build bound MCP server tools dynamically - let mcpExtraTools: Record<string, unknown> | undefined; - { - ... - }Also applies to: 358-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/lib/agent_chat/internal_actions.ts` around lines 332 - 342, The DB call listActiveByOrg is being awaited after the initial Promise.all, adding an unnecessary serial DB read; include listActiveByOrg in the same Promise.all that calls buildIntegrationTools, buildDelegationTools, buildWorkflowTools, and fetchGovernancePolicy so all four builds plus listActiveByOrg run in parallel, then destructure its result along with integrationExtraTools, delegationResult, workflowExtraTools, and governanceResult; remove the separate await for listActiveByOrg later (and repeat the same change for the other occurrence that similarly awaits listActiveByOrg after a Promise.all).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-card.tsx`:
- Around line 49-53: The button in mcp-server-card.tsx currently strips the
browser focus indicator with "outline-none" and provides no replacement; remove
"outline-none" from the button's className and add a visible keyboard focus
style (e.g., add Tailwind focus-visible utilities such as "focus-visible:ring-2
focus-visible:ring-offset-2 focus-visible:ring-[color]" or your project's
equivalent) so the interactive element (the button with onClick) has a clear
visible focus state for keyboard users.
- Around line 89-90: The UI currently hardcodes English pluralization in
mcp-server-card.tsx where the JSX renders "{toolCount} {toolCount === 1 ? 'tool'
: 'tools'}"; replace this with the project's translation/pluralization API
(e.g., useTranslation hook or i18n.t) to fetch a localized string and handle
plural forms using toolCount (for example, t('tools', { count: toolCount }) or
the equivalent in our i18n library). Update the component (mcp-server-card) to
import and use the translation hook/function and pass toolCount to the
translation call so the label is localized and correctly pluralized.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-form.tsx`:
- Around line 113-120: The OAuth2 form state (tokenUrl, authorizationUrl,
clientId, clientSecret, scopesStr, grantType) is always initialized empty so
editing an existing server forces re-entry; update the component to hydrate
these useState values from the incoming server prop when in edit mode (e.g., in
a useEffect that runs when server changes) by reading server.auth?.oauth2 (or
server.auth) and setting tokenUrl, authorizationUrl, clientId, clientSecret,
scopesStr, and grantType accordingly, falling back to existing defaults if any
field is absent; do the same for the other OAuth2-related fields referenced
around the 151-164 area so validation won’t block non-auth edits.
- Around line 55-70: The option labels, placeholders and validation messages in
MCPServerForm are hardcoded; update TRANSPORT_OPTIONS, AUTH_OPTIONS and
GRANT_TYPE_OPTIONS and any hardcoded placeholders/validation strings (e.g.,
"Streamable HTTP", "API Key", "my-mcp-server", "is required", "read, write") to
use the app i18n hook (e.g., const { t } = useTranslation() or the project’s t
helper). Move those constant arrays into the component (or export functions) so
you can call t when building each { value, label } entry, and replace hardcoded
placeholder and Yup/react-hook-form error messages with t('...') keys; add
matching translation keys for EN/DE. Ensure validation messages referenced in
the form schema (e.g., required/errorText) also use t instead of literal
strings.
- Around line 129-133: The name validator currently skips the regex for
single-character inputs allowing invalid values like "-"; in the mcp-server-form
component update the validation around newErrors.name (the block that checks
name.trim() and the regex) to always run a stricter check — either remove the
"&& name.length > 1" condition or replace the test with a pattern that permits
only a single lowercase alphanumeric character OR a multi-character value that
starts/ends with alphanumeric and may contain hyphens (e.g. change the regex
logic used when assigning newErrors.name so single-character inputs are
validated correctly).
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-panel.tsx`:
- Around line 203-225: Replace hardcoded UI strings in mcp-server-panel.tsx with
translation lookups: use the page/component translation hook (e.g., t or
useTranslations) and replace the Badge labels for transport types ('stdio',
'SSE', 'HTTP'), the auth Badge labels ('API Key', 'OAuth 2.0'), the Text label
'URL', and the test-result suffix that appends "tools" with keys from the
translations file; update the Badge and Text usages and the test result
rendering (the JSX that composes the suffix) to call the translator function
instead of literal strings so all user-facing text is localized.
- Around line 356-363: The delete button lacks a visible focus style for
keyboard users; update the raw <button> (the one using setConfirmDelete,
disabled={isTesting || isDeleting}, and t('deleteServer')) to include the same
focus-visible classes used by the shared Button component (e.g.,
focus-visible:ring, ring color, and ring-offset) so it shows a clear focus ring
when tabbed to; alternatively replace this raw button with the shared Button
component so it inherits the standard accessible focus styles. Ensure the
classes target focus-visible (not only :focus) to avoid styling on mouse click.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-servers.tsx`:
- Around line 35-36: The component currently stores the selected server as an
object snapshot (selectedServer via setSelectedServer), which becomes stale
after refetch(); change the state to store only the selectedServerId (string |
null), update setSelectedServer calls to setSelectedServerId, and derive the
current selected server each render by finding serverList.find(s => s.id ===
selectedServerId) (type McpServerListItem | undefined) so UI reflects refreshed
data; update any handlers or props that used selectedServer to use the derived
current record or its id, and ensure clearing selection sets the id to null.
In `@services/platform/convex/mcp_servers/__tests__/public_mutations.test.ts`:
- Around line 34-37: The HandlerFn type currently uses `ctx: never`, causing
widespread `as never` casts; replace it with a proper mock context type or
`unknown` and remove casts: define a minimal interface (e.g., `MockActionCtx`)
that includes only the properties your handlers use, update `type HandlerFn =
(ctx: MockActionCtx | unknown, args: Record<string, unknown>) =>
Promise<unknown>` (or use `unknown` and apply the `satisfies` operator when
constructing test inputs) and update test callers to rely on the typed mock
context instead of casting `ctx as never`; ensure all references to `ctx as
never` in the tests are removed and use proper type guards or the `satisfies`
operator for type-checked test data.
In `@services/platform/convex/mcp_servers/public_mutations.ts`:
- Around line 138-155: The update path currently only includes apiKeyEncrypted
and oauth2Config when new secrets are provided, so switching auth modes leaves
old encrypted credentials on the record; modify the logic around
args/apiKey/rawOAuth2 to explicitly clear the obsolete fields based on the
incoming auth type (e.g., inspect rest.authType or rest.auth), such that if
authType !== 'api_key' you include apiKeyEncrypted: null in the payload and if
authType !== 'oauth2' you include oauth2Config: null; keep using encryptString
and encryptOAuth2Config when new values exist and always pass the explicit nulls
into the internal.mcp_servers.mutations.update call to remove stale secrets.
- Around line 63-77: The action argument validators (used in the create action
and the similar update block) only validate primitives and allow invalid combos
(e.g., transportType 'stdio' without required command, authType 'oauth2' without
oauth2Config); replace these ad-hoc validators with the shared Zod schema from
lib/shared/validators (the domain schema that encodes conditional rules) and use
that schema for the action args instead of
transportTypeValidator/authTypeValidator/jsonRecordValidator/oauth2InputValidator;
ensure the shared schema includes refinements/conditional checks for
transportType -> command/url and authType -> apiKey/oauth2Config so the server
rejects impossible configs.
In `@services/platform/convex/mcp_servers/queries.ts`:
- Around line 7-12: The Convex queries currently use an untyped return (returns:
v.any()), causing downstream casts; define a shared validator (e.g.,
mcpServerValidator or McpServerSerializedValidator) that describes the
serialized MCP server shape (including _id as v.string() or Id style string and
all returned fields) and replace returns: v.any() with returns:
v.array(mcpServerValidator) for the list query and returns: mcpServerValidator
for getById; then update the corresponding usages in internal_queries.ts and
execute_approved.ts to consume the validated shapes (removing the (servers ??
[]) as McpServerListItem[] and server._id as Id<'mcpServers'> casts).
In `@services/platform/messages/de.json`:
- Line 4057: The JSON entry with key "apiKey" currently uses the English value
"API Key"; change its German translation to the established compound term
"API-Schlüssel" by updating the value for the "apiKey" key (replace "API Key"
with "API-Schlüssel") so it matches project terminology and German compounding
conventions.
---
Outside diff comments:
In `@services/platform/convex/lib/agent_chat/internal_actions.ts`:
- Around line 332-342: The DB call listActiveByOrg is being awaited after the
initial Promise.all, adding an unnecessary serial DB read; include
listActiveByOrg in the same Promise.all that calls buildIntegrationTools,
buildDelegationTools, buildWorkflowTools, and fetchGovernancePolicy so all four
builds plus listActiveByOrg run in parallel, then destructure its result along
with integrationExtraTools, delegationResult, workflowExtraTools, and
governanceResult; remove the separate await for listActiveByOrg later (and
repeat the same change for the other occurrence that similarly awaits
listActiveByOrg after a Promise.all).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 32b0b53a-d4f8-49c7-a4e6-0041ec8c5b5c
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (22)
services/platform/app/features/settings/components/settings-navigation.tsxservices/platform/app/features/settings/mcp-servers/components/__tests__/mcp-server-card.test.tsxservices/platform/app/features/settings/mcp-servers/components/__tests__/mcp-server-form.test.tsxservices/platform/app/features/settings/mcp-servers/components/__tests__/mcp-server-panel.test.tsxservices/platform/app/features/settings/mcp-servers/components/__tests__/mcp-servers.test.tsxservices/platform/app/features/settings/mcp-servers/components/mcp-server-card.tsxservices/platform/app/features/settings/mcp-servers/components/mcp-server-form.tsxservices/platform/app/features/settings/mcp-servers/components/mcp-server-panel.tsxservices/platform/app/features/settings/mcp-servers/components/mcp-servers.tsxservices/platform/app/features/settings/mcp-servers/components/types.tsservices/platform/app/features/settings/mcp-servers/hooks/use-mcp-servers.tsservices/platform/app/routeTree.gen.tsservices/platform/app/routes/dashboard/$id/settings/mcp-servers.tsxservices/platform/convex/lib/agent_chat/internal_actions.tsservices/platform/convex/mcp_servers/__tests__/public_mutations.test.tsservices/platform/convex/mcp_servers/__tests__/queries.test.tsservices/platform/convex/mcp_servers/internal_queries.tsservices/platform/convex/mcp_servers/mutations.tsservices/platform/convex/mcp_servers/public_mutations.tsservices/platform/convex/mcp_servers/queries.tsservices/platform/messages/de.jsonservices/platform/messages/en.json
Comment on lines
+49
to
+53
| <button | ||
| type="button" | ||
| onClick={onClick} | ||
| className="w-full p-5 text-left outline-none" | ||
| > |
There was a problem hiding this comment.
Restore visible keyboard focus on the card button.
On Line 52, outline-none removes the default focus indicator, and there is no focus-visible replacement style.
Proposed fix
- <button
+ <button
type="button"
onClick={onClick}
- className="w-full p-5 text-left outline-none"
+ className="w-full p-5 text-left focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2"
>📝 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 | |
| type="button" | |
| onClick={onClick} | |
| className="w-full p-5 text-left outline-none" | |
| > | |
| <button | |
| type="button" | |
| onClick={onClick} | |
| className="w-full p-5 text-left focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-card.tsx`
around lines 49 - 53, The button in mcp-server-card.tsx currently strips the
browser focus indicator with "outline-none" and provides no replacement; remove
"outline-none" from the button's className and add a visible keyboard focus
style (e.g., add Tailwind focus-visible utilities such as "focus-visible:ring-2
focus-visible:ring-offset-2 focus-visible:ring-[color]" or your project's
equivalent) so the interactive element (the button with onClick) has a clear
visible focus state for keyboard users.
Comment on lines
+89
to
+90
| {toolCount} {toolCount === 1 ? 'tool' : 'tools'} | ||
| </Text> |
There was a problem hiding this comment.
Localize the tool count label instead of hardcoding English text.
On Line 89, 'tool'/'tools' is user-facing text and should come from translations.
Proposed fix
- <Text variant="muted" className="text-xs">
- {toolCount} {toolCount === 1 ? 'tool' : 'tools'}
- </Text>
+ <Text variant="muted" className="text-xs">
+ {t('tools.count', { count: toolCount })}
+ </Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-card.tsx`
around lines 89 - 90, The UI currently hardcodes English pluralization in
mcp-server-card.tsx where the JSX renders "{toolCount} {toolCount === 1 ? 'tool'
: 'tools'}"; replace this with the project's translation/pluralization API
(e.g., useTranslation hook or i18n.t) to fetch a localized string and handle
plural forms using toolCount (for example, t('tools', { count: toolCount }) or
the equivalent in our i18n library). Update the component (mcp-server-card) to
import and use the translation hook/function and pass toolCount to the
translation call so the label is localized and correctly pluralized.
Comment on lines
+55
to
+70
| const TRANSPORT_OPTIONS = [ | ||
| { value: 'streamable_http', label: 'Streamable HTTP' }, | ||
| { value: 'sse', label: 'SSE' }, | ||
| { value: 'stdio', label: 'stdio' }, | ||
| ]; | ||
|
|
||
| const AUTH_OPTIONS = [ | ||
| { value: 'none', label: 'None' }, | ||
| { value: 'api_key', label: 'API Key' }, | ||
| { value: 'oauth2', label: 'OAuth 2.0' }, | ||
| ]; | ||
|
|
||
| const GRANT_TYPE_OPTIONS = [ | ||
| { value: 'client_credentials', label: 'Client credentials' }, | ||
| { value: 'authorization_code', label: 'Authorization code' }, | ||
| ]; |
There was a problem hiding this comment.
Move the remaining form copy into translations.
Option labels, placeholders, and validation messages are still hardcoded in English ("Streamable HTTP", "API Key", "is required", "my-mcp-server", "read, write", etc.), so the new EN/DE settings page won't localize end-to-end.
As per coding guidelines, "Do NOT hardcode text, use the translation hooks/functions instead for user-facing UI."
Also applies to: 129-164, 271-306, 319-350, 400-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-form.tsx`
around lines 55 - 70, The option labels, placeholders and validation messages in
MCPServerForm are hardcoded; update TRANSPORT_OPTIONS, AUTH_OPTIONS and
GRANT_TYPE_OPTIONS and any hardcoded placeholders/validation strings (e.g.,
"Streamable HTTP", "API Key", "my-mcp-server", "is required", "read, write") to
use the app i18n hook (e.g., const { t } = useTranslation() or the project’s t
helper). Move those constant arrays into the component (or export functions) so
you can call t when building each { value, label } entry, and replace hardcoded
placeholder and Yup/react-hook-form error messages with t('...') keys; add
matching translation keys for EN/DE. Ensure validation messages referenced in
the form schema (e.g., required/errorText) also use t instead of literal
strings.
Comment on lines
+113
to
+120
| const [tokenUrl, setTokenUrl] = useState(''); | ||
| const [authorizationUrl, setAuthorizationUrl] = useState(''); | ||
| const [clientId, setClientId] = useState(''); | ||
| const [clientSecret, setClientSecret] = useState(''); | ||
| const [scopesStr, setScopesStr] = useState(''); | ||
| const [grantType, setGrantType] = useState< | ||
| 'client_credentials' | 'authorization_code' | ||
| >('client_credentials'); |
There was a problem hiding this comment.
Editing existing OAuth2 servers is currently blocked.
In edit mode these OAuth2 fields start empty, but validation still requires tokenUrl, clientId, and sometimes authorizationUrl. Since server does not carry the existing OAuth2 config, a user cannot make a simple non-auth change without re-entering the whole OAuth2 setup.
Also applies to: 151-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-form.tsx`
around lines 113 - 120, The OAuth2 form state (tokenUrl, authorizationUrl,
clientId, clientSecret, scopesStr, grantType) is always initialized empty so
editing an existing server forces re-entry; update the component to hydrate
these useState values from the incoming server prop when in edit mode (e.g., in
a useEffect that runs when server changes) by reading server.auth?.oauth2 (or
server.auth) and setting tokenUrl, authorizationUrl, clientId, clientSecret,
scopesStr, and grantType accordingly, falling back to existing defaults if any
field is absent; do the same for the other OAuth2-related fields referenced
around the 151-164 area so validation won’t block non-auth edits.
Comment on lines
+129
to
+133
| if (!name.trim()) { | ||
| newErrors.name = t('form.name') + ' is required'; | ||
| } else if (!/^[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(name) && name.length > 1) { | ||
| newErrors.name = 'Must be lowercase alphanumeric with hyphens'; | ||
| } |
There was a problem hiding this comment.
The name validator accepts invalid one-character values.
&& name.length > 1 skips the regex check for single-character inputs, so values like "-" pass client validation.
🐛 Suggested fix
- } else if (!/^[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(name) && name.length > 1) {
+ } else if (!/^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/.test(name)) {
newErrors.name = 'Must be lowercase alphanumeric with hyphens';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/mcp-servers/components/mcp-server-form.tsx`
around lines 129 - 133, The name validator currently skips the regex for
single-character inputs allowing invalid values like "-"; in the mcp-server-form
component update the validation around newErrors.name (the block that checks
name.trim() and the regex) to always run a stricter check — either remove the
"&& name.length > 1" condition or replace the test with a pattern that permits
only a single lowercase alphanumeric character OR a multi-character value that
starts/ends with alphanumeric and may contain hyphens (e.g. change the regex
logic used when assigning newErrors.name so single-character inputs are
validated correctly).
Comment on lines
+34
to
+37
| type HandlerFn = ( | ||
| ctx: never, | ||
| args: Record<string, unknown>, | ||
| ) => Promise<unknown>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its structure
fd -t f "public_mutations.test.ts"Repository: tale-project/tale
Length of output: 132
🏁 Script executed:
# Read the file to see lines 34-37 and surrounding context
cat -n services/platform/convex/mcp_servers/__tests__/public_mutations.test.ts | head -80Repository: tale-project/tale
Length of output: 2671
🏁 Script executed:
# Search for all instances of "ctx as never" or similar casts in the file
rg "ctx\s+(as\s+never|as\s+any)" services/platform/convex/mcp_servers/__tests__/public_mutations.test.ts -A 2Repository: tale-project/tale
Length of output: 852
Replace ctx: never with a proper mock context type or unknown to eliminate type casting.
ctx: never forces repeated ctx as never casts throughout test cases (8+ instances). Instead, define a minimal mock interface like MockActionCtx matching the handlers' actual ctx usage, use unknown, or apply satisfies operator for type-checked test data without blanket casts.
Per coding guidelines: DO NOT use type casting (as). Use type guards, generics, or proper type narrowing instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/mcp_servers/__tests__/public_mutations.test.ts`
around lines 34 - 37, The HandlerFn type currently uses `ctx: never`, causing
widespread `as never` casts; replace it with a proper mock context type or
`unknown` and remove casts: define a minimal interface (e.g., `MockActionCtx`)
that includes only the properties your handlers use, update `type HandlerFn =
(ctx: MockActionCtx | unknown, args: Record<string, unknown>) =>
Promise<unknown>` (or use `unknown` and apply the `satisfies` operator when
constructing test inputs) and update test callers to rely on the typed mock
context instead of casting `ctx as never`; ensure all references to `ctx as
never` in the tests are removed and use proper type guards or the `satisfies`
operator for type-checked test data.
Comment on lines
+63
to
+77
| export const create = action({ | ||
| args: { | ||
| organizationId: v.string(), | ||
| name: v.string(), | ||
| displayName: v.string(), | ||
| description: v.optional(v.string()), | ||
| transportType: transportTypeValidator, | ||
| url: v.optional(v.string()), | ||
| command: v.optional(v.string()), | ||
| args: v.optional(v.array(v.string())), | ||
| env: v.optional(jsonRecordValidator), | ||
| authType: authTypeValidator, | ||
| apiKey: v.optional(v.string()), | ||
| oauth2Config: v.optional(oauth2InputValidator), | ||
| }, |
There was a problem hiding this comment.
The backend still accepts invalid transport/auth combinations.
These validators only check primitive shapes, so the API will happily persist impossible configs like transportType: 'stdio' without command or authType: 'oauth2' without oauth2Config if the form is bypassed. This needs a shared schema, not separate client-only rules.
As per coding guidelines, "ALWAYS share validation schemas between client and server using Zod. Validators are organized per domain in lib/shared/validators/ ... DO NOT duplicate validation logic."
Also applies to: 117-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/mcp_servers/public_mutations.ts` around lines 63 -
77, The action argument validators (used in the create action and the similar
update block) only validate primitives and allow invalid combos (e.g.,
transportType 'stdio' without required command, authType 'oauth2' without
oauth2Config); replace these ad-hoc validators with the shared Zod schema from
lib/shared/validators (the domain schema that encodes conditional rules) and use
that schema for the action args instead of
transportTypeValidator/authTypeValidator/jsonRecordValidator/oauth2InputValidator;
ensure the shared schema includes refinements/conditional checks for
transportType -> command/url and authType -> apiKey/oauth2Config so the server
rejects impossible configs.
Comment on lines
+138
to
+155
| const { id, apiKey, oauth2Config: rawOAuth2, ...rest } = args; | ||
|
|
||
| let apiKeyEncrypted: string | undefined; | ||
| if (apiKey) { | ||
| apiKeyEncrypted = await encryptString(apiKey); | ||
| } | ||
|
|
||
| let oauth2Config: EncryptedOAuth2Config | undefined; | ||
| if (rawOAuth2) { | ||
| oauth2Config = await encryptOAuth2Config(rawOAuth2); | ||
| } | ||
|
|
||
| await ctx.runMutation(internal.mcp_servers.mutations.update, { | ||
| id, | ||
| ...rest, | ||
| ...(apiKeyEncrypted !== undefined ? { apiKeyEncrypted } : {}), | ||
| ...(oauth2Config !== undefined ? { oauth2Config } : {}), | ||
| }); |
There was a problem hiding this comment.
Changing auth mode leaves the old encrypted credentials behind.
apiKeyEncrypted and oauth2Config are only sent when new values are provided, so switching from api_key/oauth2 to another auth type preserves the previous secret material on the record. Please add an explicit clear path when auth changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/mcp_servers/public_mutations.ts` around lines 138 -
155, The update path currently only includes apiKeyEncrypted and oauth2Config
when new secrets are provided, so switching auth modes leaves old encrypted
credentials on the record; modify the logic around args/apiKey/rawOAuth2 to
explicitly clear the obsolete fields based on the incoming auth type (e.g.,
inspect rest.authType or rest.auth), such that if authType !== 'api_key' you
include apiKeyEncrypted: null in the payload and if authType !== 'oauth2' you
include oauth2Config: null; keep using encryptString and encryptOAuth2Config
when new values exist and always pass the explicit nulls into the
internal.mcp_servers.mutations.update call to remove stale secrets.
Comment on lines
+7
to
+12
| export const list = query({ | ||
| args: { | ||
| organizationId: v.string(), | ||
| }, | ||
| returns: v.any(), | ||
| handler: async (ctx, args) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the untyped query boundary is forcing casts in MCP server consumers.
rg -n "returns:\s*v\.any\(\)|as McpServerListItem\[]|as Id<'mcpServers'>" \
services/platform/convex/mcp_servers \
services/platform/app/features/settings/mcp-serversRepository: tale-project/tale
Length of output: 1112
Replace returns: v.any() with explicit validators for MCP server queries.
The untyped return contract at the Convex boundary forces downstream assertions: (servers ?? []) as McpServerListItem[] and server._id as Id<'mcpServers'>. Define a shared validator for the serialized MCP server shape and use it in both list (line 11) and getById (line 56) queries instead of v.any().
Also applies to internal_queries.ts and execute_approved.ts.
Coding guidelines require avoiding type casting; explicit boundary types prevent the need for assertions downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/mcp_servers/queries.ts` around lines 7 - 12, The
Convex queries currently use an untyped return (returns: v.any()), causing
downstream casts; define a shared validator (e.g., mcpServerValidator or
McpServerSerializedValidator) that describes the serialized MCP server shape
(including _id as v.string() or Id style string and all returned fields) and
replace returns: v.any() with returns: v.array(mcpServerValidator) for the list
query and returns: mcpServerValidator for getById; then update the corresponding
usages in internal_queries.ts and execute_approved.ts to consume the validated
shapes (removing the (servers ?? []) as McpServerListItem[] and server._id as
Id<'mcpServers'> casts).
| "args": "Argumente", | ||
| "authType": "Authentifizierung", | ||
| "none": "Keine", | ||
| "apiKey": "API Key", |
There was a problem hiding this comment.
Use the established German compound term for API key.
On Line 4057, use API-Schlüssel instead of API Key to stay consistent with the project’s German terminology.
Based on learnings: German (de) terminology ... API ... and USE standard German compounding for compound nouns (e.g., 'API-Schlüssel').
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/de.json` at line 4057, The JSON entry with key
"apiKey" currently uses the English value "API Key"; change its German
translation to the established compound term "API-Schlüssel" by updating the
value for the "apiKey" key (replace "API Key" with "API-Schlüssel") so it
matches project terminology and German compounding conventions.
- Add 'discovering' status to schema to fix validation error during connection test - Strip '$'-prefixed keys from MCP tool inputSchema before storing in Convex - Add overflow-y-auto to Sheet component to enable scrolling for long forms - Refresh server data after successful connection test to show discovered tools - Remove unsafe type assertion in updateDiscoveredTools mutation
Derive selectedServer from query results instead of storing a stale copy in state, so the panel updates automatically when data is refetched (e.g. after a successful connection test).
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
Closes #1347
Test plan
/dashboard/{id}/settings/mcp-serversSummary by CodeRabbit
Release Notes