Conversation
…s and billing processes - Improved user ID handling for token billing in multiple workflows. - Added non-fatal error handling for billing failures in token usage tracking. - Enhanced customer resolution logic for Stripe disputes. - Updated user ID types for Bedrock access control to ensure type safety.
…ng across multiple components
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances token usage tracking and error handling across the workflow execution system. It makes billing operations non-fatal throughout the codebase, improves type safety by removing unsafe type casts, and enhances user identity resolution.
Changes:
- Wrapped all billing/metering operations in try-catch blocks to treat them as non-fatal, preventing billing failures from breaking primary functionality
- Removed unsafe
as anytype casts and replaced with properId<"users">types from the generated dataModel - Improved user identity resolution by removing email as a fallback (emails can collide across providers) and enhanced error messages
- Enhanced Stripe dispute webhook handling to properly resolve customer IDs from unexpanded charge objects
- Added validation for missing user IDs in bedrockGate and improved token estimation accuracy in multiple execution paths
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| convex/workflowExecutor.ts | Updated user identity resolution logic (removed email fallback), added non-fatal billing error handling, fixed type safety for gateResult userId |
| convex/unifiedAgentExecution.ts | Added user record validation before Bedrock gate check, wrapped billing in try-catch, removed as any cast |
| convex/tools.ts | Added non-fatal billing error handling for all tool actions (selfConsistency, treeOfThoughts, reflexion, mapReduce, parallelPrompts), fixed userId types |
| convex/testExecution.ts | Removed unsafe as any casts for userId parameters |
| convex/strandsAgentExecutionDynamic.ts | Added non-fatal billing error handling, improved token estimation to use user-facing text instead of full JSON payload |
| convex/strandsAgentExecution.ts | Added non-fatal billing error handling with try-catch wrapper |
| convex/promptChainExecutor.ts | Added non-fatal billing error handling, fixed userId types, added error field to executeParallelPrompts return type |
| convex/mcpConfig.ts | Added BuiltInMcpServer interface to properly type built-in servers with synthetic string IDs instead of database IDs |
| convex/mcpClient.ts | Added non-fatal billing error handling, added logic to skip database operations for built-in servers, improved token estimation |
| convex/http.ts | Enhanced dispute webhook handling to fetch charge details when charge is an unexpanded ID, added comprehensive error logging |
| convex/deployments.ts | Removed unsafe as any casts for userId |
| convex/deploymentRouter.ts | Added non-fatal billing error handling, fixed userId types, removed unsafe casts |
| convex/awsDiagramGenerator.ts | Removed unsafe as any cast for userId |
| convex/awsDeployment.ts | Added non-fatal billing error handling, cleaned up userId type handling in createDeploymentInternal |
| convex/automatedAgentBuilder.ts | Added non-fatal billing error handling with try-catch |
| convex/agentBuilderWorkflow.ts | Added non-fatal billing error handling with try-catch |
| convex/lib/bedrockGate.ts | Added validation for missing user _id, fixed return type to use proper Id<"users"> |
| convex/interleavedReasoning.ts | Added non-fatal billing error handling with try-catch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughConverted many loose Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/mcpConfig.ts (2)
238-248:⚠️ Potential issue | 🟡 MinorUser can create a server whose name collides with a built-in, making it unreachable.
addMCPServeronly checks the DB for duplicate names (line 239-244). A user could insert a server named"bedrock-agentcore-mcp-server", butgetMCPServerByName(line 157) always returns the built-in first, permanently shadowing the user's record.Add a check against
BUILT_IN_MCP_SERVERSnames before inserting:Proposed fix
+ // Reject names that collide with built-in servers + const isBuiltInName = BUILT_IN_MCP_SERVERS.some(s => s.name === args.name); + if (isBuiltInName) { + throw new Error(`"${args.name}" is a reserved built-in server name`); + } + // Check if server with this name already exists for this user const existingServer = await ctx.db
378-407:⚠️ Potential issue | 🟠 Major
updateMCPServerStatusmutation lacks authentication and authorization checks.Unlike
addMCPServer,updateMCPServer, anddeleteMCPServerin the same file, this mutation has nogetAuthUserId()check or ownership verification. Any unauthenticated or unauthorized user can updatestatus,lastError,lastConnected, andavailableToolsfor any server by providing a validserverId. This is called from bothtestMCPConnectionandinvokeMCPToolactions, which are also public.Add authentication and ownership verification consistent with the other mutations:
- Call
getAuthUserId(ctx)to verify the user is authenticated- Verify that
server.userId === userIdbefore patchingconvex/promptChainExecutor.ts (1)
246-253:⚠️ Potential issue | 🟠 MajorMissing non-fatal try-catch around billing in
executeParallelPrompts.Unlike
executePromptChain(line 124) andtestPrompt(line 470), this billing call is not wrapped in a try-catch. A billing failure here will throw an unhandled exception and crash the entire parallel prompts execution, since there's no outer try-catch in this handler either. This is inconsistent with the non-fatal billing pattern applied everywhere else in this PR.🐛 Proposed fix: wrap in try-catch like other billing calls
// Meter: token-based billing for all parallel prompts if ( gateUserId && ( totalInputTokens > 0 || totalOutputTokens > 0 ) ) { - await ctx.runMutation( internal.stripeMutations.incrementUsageAndReportOverage, { - userId: gateUserId, - modelId: gateModelId, - inputTokens: totalInputTokens, - outputTokens: totalOutputTokens, - } ); + try { + await ctx.runMutation( internal.stripeMutations.incrementUsageAndReportOverage, { + userId: gateUserId, + modelId: gateModelId, + inputTokens: totalInputTokens, + outputTokens: totalOutputTokens, + } ); + } catch ( billingErr ) { + console.error( "promptChainExecutor: billing failed (non-fatal)", { + userId: gateUserId, modelId: gateModelId, + inputTokens: totalInputTokens, outputTokens: totalOutputTokens, + error: billingErr instanceof Error ? billingErr.message : billingErr, + } ); + } }
🤖 Fix all issues with AI agents
In `@convex/awsDeployment.ts`:
- Line 707: The deployTier2 → createDeploymentInternal type mismatch: change
deployTier2's userId parameter (and any internal actions like
executeWebIdentityDeploymentInternal and executeCrossAccountDeploymentInternal)
from plain string/v.string() to the user-id type expected by
createDeploymentInternal (use v.id("users") / Id<"users"> as used in
createDeploymentInternal) so the passed userId is strongly typed; update the
function signatures and any internal action/mutation validators and callers to
accept and propagate v.id("users") instead of string to keep types consistent
end-to-end (search for deployTier2, createDeploymentInternal,
executeWebIdentityDeploymentInternal, executeCrossAccountDeploymentInternal to
locate all sites to change).
In `@convex/mcpConfig.ts`:
- Around line 22-36: BuiltInMcpServer currently defines _id as string while DB
records use Id<"mcpServers">, causing incompatible union return types from
listMCPServers and lookup helpers (getMCPServerByName,
getMCPServerByNameInternal); add a discriminant like isBuiltIn: true to
BuiltInMcpServer and update its type to reflect _id: string, then adjust
listMCPServers/getMCPServerByName/getMCPServerByNameInternal return types to a
discriminated union (BuiltInMcpServer | DbMcpServer) so callers can narrow on
isBuiltIn and avoid type assertions when handling differing _id types.
🧹 Nitpick comments (5)
convex/mcpConfig.ts (1)
41-41:Date.now()evaluated at module-load time, not per-request.All four built-in server entries share the same
_creationTime,createdAt, andupdatedAtvalues, frozen at the moment the Convex module was first loaded. If these timestamps are displayed in the UI or used for sorting/ordering, they'll be misleading. Consider using fixed epoch values (e.g.,0) to make it explicit that these are synthetic, or compute them lazily inside each query handler.Also applies to: 53-54
convex/deploymentRouter.ts (1)
68-68:deployTier1correctly typed asId<"users">, butdeployTier2/deployTier3are stillstring.Lines 164 and 192 still declare
_userId: stringwhile the caller on lines 42/45 passes the sameId<"users">value. This compiles becauseId<"users">extendsstring, but it's inconsistent with the type-tightening intent of this PR.Suggested fix for consistency
-async function deployTier2(ctx: any, args: any, _userId: string): Promise<any> { +async function deployTier2(ctx: any, args: any, _userId: Id<"users">): Promise<any> {-async function deployTier3(_ctx: any, _args: any, _userId: string): Promise<any> { +async function deployTier3(_ctx: any, _args: any, _userId: Id<"users">): Promise<any> {convex/workflowExecutor.ts (1)
166-166: Consider using the exportedBedrockAccessGrantedtype.The inline type
{ allowed: true; userId: import("./_generated/dataModel").Id<"users">; tier: string }duplicates the shape ofBedrockAccessGrantedfrombedrockGate.ts. Using the exported type would reduce duplication and stay in sync if the gate result shape changes.♻️ Suggested refactor
+import type { BedrockAccessGranted } from "./lib/bedrockGate"; // ... - let gateResult: { allowed: true; userId: import("./_generated/dataModel").Id<"users">; tier: string } | undefined; + let gateResult: BedrockAccessGranted | undefined;convex/tools.ts (1)
364-364: Repeated inline type annotation — consider importingBedrockAccessGrantedonce.The type
{ allowed: true; userId: import("./_generated/dataModel").Id<"users">; tier: string }is repeated 5 times in this file and additionally inworkflowExecutor.tsandpromptChainExecutor.ts.BedrockAccessGrantedis already exported from./lib/bedrockGateand carries the same shape.♻️ Suggested refactor
Add a single import at the top of the file:
+import type { BedrockAccessGranted } from "./lib/bedrockGate";Then replace each occurrence:
- let gateResult: { allowed: true; userId: import("./_generated/dataModel").Id<"users">; tier: string } | undefined; + let gateResult: BedrockAccessGranted | undefined;Also applies to: 453-453, 552-552, 658-658, 761-761
convex/promptChainExecutor.ts (1)
46-46: Same inline type duplication as other files.
import("./_generated/dataModel").Id<"users">is used 3 times here. SameBedrockAccessGrantedimport refactor applies (see comment onconvex/tools.ts).Also applies to: 188-188, 444-444
| args: { | ||
| agentId: v.id( "agents" ), | ||
| userId: v.union( v.id( "users" ), v.string() ), | ||
| userId: v.id( "users" ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its existence
find . -name "awsDeployment.ts" -type fRepository: MikePfunk28/agent_builder_application
Length of output: 106
🏁 Script executed:
# Read the area around line 707 to see createDeploymentInternal
sed -n '700,730p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 1256
🏁 Script executed:
# Read the area around line 989 to see deployTier2
sed -n '980,1010p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 1377
🏁 Script executed:
# Check line 10 for the import
sed -n '1,15p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 783
🏁 Script executed:
# Search for all calls to createDeploymentInternal
rg -n "createDeploymentInternal" convex/awsDeployment.ts -A 2 -B 2Repository: MikePfunk28/agent_builder_application
Length of output: 804
🏁 Script executed:
# Check the executeCrossAccountDeploymentInternal and executeWebIdentityDeploymentInternal mentioned in scratchpad
rg -n "executeCrossAccountDeploymentInternal\|executeWebIdentityDeploymentInternal" convex/awsDeployment.ts | head -20Repository: MikePfunk28/agent_builder_application
Length of output: 63
🏁 Script executed:
# Find where deployTier2 is called
rg -n "deployTier2" convex/awsDeployment.ts -B 2 -A 2Repository: MikePfunk28/agent_builder_application
Length of output: 588
🏁 Script executed:
# Check the context where deployTier2 is called (around line 88)
sed -n '75,100p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 1411
🏁 Script executed:
# Check the function signature that contains this code (around line 75)
sed -n '40,80p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 1914
🏁 Script executed:
# Check what getAuthUserId returns
rg -n "function getAuthUserId\|const getAuthUserId\|export.*getAuthUserId" convex/awsDeployment.ts -A 5Repository: MikePfunk28/agent_builder_application
Length of output: 63
🏁 Script executed:
# Search for getAuthUserId definition more broadly
rg -n "getAuthUserId" convex/awsDeployment.tsRepository: MikePfunk28/agent_builder_application
Length of output: 445
🏁 Script executed:
# Check imports at the top of the file more carefully
sed -n '1,50p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 2237
🏁 Script executed:
# Verify the return type of getAuthUserId by checking how it's used throughout
# and looking for null checks
sed -n '44,90p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 2189
🏁 Script executed:
# Check for all calls to deployTier2
rg -n "deployTier2\(" convex/awsDeployment.tsRepository: MikePfunk28/agent_builder_application
Length of output: 222
🏁 Script executed:
# Verify how getAuthUserId is actually typed in the Convex auth library
# by checking if there are any .d.ts files or checking the node_modules
find . -path "*/node_modules/@convex-dev/auth*" -name "*.d.ts" 2>/dev/null | head -5Repository: MikePfunk28/agent_builder_application
Length of output: 63
🏁 Script executed:
# Check how userId is used within deployTier2 - particularly when passed to createDeploymentInternal
sed -n '989,1010p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 1064
🏁 Script executed:
# Also check if there are any TypeScript errors or types in comments
rg -n "@convex-dev/auth" . --type=ts -A 2 -B 2 | head -30Repository: MikePfunk28/agent_builder_application
Length of output: 1652
🏁 Script executed:
# Check all the internal action/mutation signatures that accept userId
rg -n "userId: (string|any)" convex/awsDeployment.ts | head -20Repository: MikePfunk28/agent_builder_application
Length of output: 166
🏁 Script executed:
# Check all internal action/mutation function signatures to see which ones accept userId parameter
rg -n "async function" convex/awsDeployment.ts | grep -E "deployTier|executeWebIdentity|executeCrossAccount"Repository: MikePfunk28/agent_builder_application
Length of output: 257
🏁 Script executed:
# Get the full signatures of these functions
sed -n '950,955p' convex/awsDeployment.ts | cat -nRepository: MikePfunk28/agent_builder_application
Length of output: 417
🏁 Script executed:
# Check executeWebIdentityDeploymentInternal
rg -n "executeWebIdentityDeploymentInternal" convex/awsDeployment.ts -A 5 | head -20Repository: MikePfunk28/agent_builder_application
Length of output: 571
🏁 Script executed:
# Check executeCrossAccountDeploymentInternal as mentioned in the review scratchpad
rg -n "executeCrossAccountDeploymentInternal" convex/awsDeployment.ts -A 5 | head -30Repository: MikePfunk28/agent_builder_application
Length of output: 312
deployTier2 still accepts userId: string, creating a type mismatch with createDeploymentInternal.
createDeploymentInternal requires userId: v.id("users") (line 708), but deployTier2 (line 989) declares userId: string and passes it to this mutation (line 1000). While this works at runtime because userId originates from getAuthUserId() which returns Id<"users">, it defeats the purpose of the tightened typing.
Additionally, executeWebIdentityDeploymentInternal (line 1138) and executeCrossAccountDeploymentInternal (line 1063) also declare userId: v.string(), creating further inconsistency in the internal deployment flow.
🔧 Proposed fixes
-async function deployTier2( ctx: any, args: any, userId: string ): Promise<any> {
+async function deployTier2( ctx: any, args: any, userId: Id<"users"> ): Promise<any> {Also update the internal actions:
- userId: v.string(),
+ userId: v.id( "users" ),🤖 Prompt for AI Agents
In `@convex/awsDeployment.ts` at line 707, The deployTier2 →
createDeploymentInternal type mismatch: change deployTier2's userId parameter
(and any internal actions like executeWebIdentityDeploymentInternal and
executeCrossAccountDeploymentInternal) from plain string/v.string() to the
user-id type expected by createDeploymentInternal (use v.id("users") /
Id<"users"> as used in createDeploymentInternal) so the passed userId is
strongly typed; update the function signatures and any internal action/mutation
validators and callers to accept and propagate v.id("users") instead of string
to keep types consistent end-to-end (search for deployTier2,
createDeploymentInternal, executeWebIdentityDeploymentInternal,
executeCrossAccountDeploymentInternal to locate all sites to change).
- Introduced a `source` property to distinguish between built-in and user-defined MCP servers. - Updated interfaces and types across the MCP server management codebase to accommodate the new `source` property. - Modified the `listMCPServers` query to include the `source` attribute for user-defined servers. - Adjusted the MCP management panel to conditionally render action buttons based on the server's source. - Ensured compatibility with both string synthetic IDs for built-in servers and Convex IDs for user-defined servers. - Updated forms and selectors to handle the new `source` property and maintain type safety.
Deploying agent-builder-application with
|
| Latest commit: |
9a8e7cd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d7f02845.agent-builder-application.pages.dev |
| Branch Preview URL: | https://012-visual-builder-update.agent-builder-application.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/mcpConfig.ts (1)
266-318:⚠️ Potential issue | 🔴 CriticalFix
v.object({})to accept environment variable objects.
v.object({})in Convex validates that an object has exactly zero properties. This causes validation to fail when the UI form sends environment variables (e.g.,{ AWS_REGION: "us-east-1" }), preventing users from saving MCP servers with env vars. The same issue exists inupdateMCPServer.The database schema already expects arbitrary env maps (with comment: "Accepts arbitrary env maps (e.g. { "PATH": "/usr/bin" })"), and the codebase uses
v.record(v.string(), v.string())elsewhere for this exact pattern.🐛 Proposed fix
args: { name: v.string(), command: v.string(), args: v.array( v.string() ), - env: v.optional( v.object( {} ) ), + env: v.optional( v.record( v.string(), v.string() ) ), disabled: v.optional( v.boolean() ), timeout: v.optional( v.number() ), },Apply the same fix to
updateMCPServer:updates: v.object( { name: v.optional( v.string() ), command: v.optional( v.string() ), args: v.optional( v.array( v.string() ) ), - env: v.optional( v.object( {} ) ), + env: v.optional( v.record( v.string(), v.string() ) ), disabled: v.optional( v.boolean() ), timeout: v.optional( v.number() ), } ),
🤖 Fix all issues with AI agents
In `@convex/mcpClient.ts`:
- Around line 522-523: Remove the unused dynamic import that shadows the
module-level api inside invokeBedrockAgentCore: delete the line "const { api } =
await import(\"./_generated/api.js\")" and any leftover references to that local
binding so the function uses the module-level api import instead; after removal,
run build/tests to ensure there are no remaining references or linter errors.
In `@convex/mcpConfig.ts`:
- Around line 505-530: The docstring/comment for getMCPServerById says "internal
query" but the export uses query(), exposing it publicly; either change the
comment to reflect that getMCPServerById is a public query or convert the
exported symbol to an internal query by replacing query(...) with
internalQuery(...) (or the project's internal-query helper) so the handler
remains server-only; update any import/usage accordingly and keep the function
name getMCPServerById and its auth/authorization logic unchanged.
- Around line 228-261: The fallback branch in getMCPServerByNameInternal (inside
the handler when args.userId is falsy) currently queries the mcpServers table
and returns the first record matching name, which can leak another user's
server; change the logic so that when args.userId is not provided the handler
returns null (or otherwise restricts results to non-user/scoped entries) instead
of querying by name across all users — update the code paths that reference
args.userId and the mcpServers query to short-circuit to null (or add an
explicit public/source filter) and adjust any type/return expectations
accordingly.
🧹 Nitpick comments (3)
convex/mcpClient.ts (1)
254-263:as anycast weakens the type guard — consider narrowing the query return type instead.
isDbMcpServer(server as any)works at runtime but defeats compile-time safety. Theservervariable from the query is untyped becauserunQueryreturnsany-ish. A cleaner approach is to cast toMCPServerEntry(the union) rather thanany, so TypeScript still checks the guard's input.♻️ Suggested change
- if ( isDbMcpServer( server as any ) ) { + if ( isDbMcpServer( server as MCPServerEntry ) ) {(Apply in both locations — lines 257 and 300. You'll also need to import
MCPServerEntryfrom./mcpConfig.)Also applies to: 299-306
src/components/MCPManagementPanel.tsx (1)
202-233: Good: source-based UI gating for action buttons.System servers correctly show only a "System" badge, while user servers expose Test/Edit/Delete actions. The
as Id<"mcpServers">casts on lines 207 and 224 are safe here because they're only reachable whenserver.source === 'user'.One nit: line 217 uses
handleEdit(server as any)— consider casting to theMCPServerinterface instead ofanyto preserve some type safety.src/components/MCPServerForm.tsx (1)
8-20: Type widening accepted but_idis passed unchecked to a mutation that requiresId<"mcpServers">.On line 124,
server._idis passed asserverIdtoupdateMCPServer, which validates withv.id("mcpServers"). If a system server (with a plain string_idlike"system_ollama") were ever passed to this form, the mutation would throw at runtime.Currently this is safe because
MCPManagementPanelgates the edit button behindsource === 'user', but nothing in the form itself prevents misuse. Consider adding a guard or narrowing the type for edit mode.🛡️ Optional: add a runtime guard
if (isEditMode) { + if (server.source === 'system') { + toast.error('System servers cannot be edited'); + return; + } // Update existing server await updateServer({ - serverId: server._id, + serverId: server._id as Id<"mcpServers">,
| const { api } = await import( "./_generated/api.js" ); | ||
|
|
There was a problem hiding this comment.
Unused dynamic import of api — dead code.
const { api } = await import("./_generated/api.js") is imported but never used within invokeBedrockAgentCore. It also shadows the module-level api import (line 5). Remove it.
🧹 Proposed fix
- // Get Cognito JWT token
- const { api } = await import( "./_generated/api.js" );
-
- // Import action runner - this is a workaround since we're in a non-Convex context
- // In production, you'd inject the ctx or use a proper service
+ // Get Cognito JWT token🤖 Prompt for AI Agents
In `@convex/mcpClient.ts` around lines 522 - 523, Remove the unused dynamic import
that shadows the module-level api inside invokeBedrockAgentCore: delete the line
"const { api } = await import(\"./_generated/api.js\")" and any leftover
references to that local binding so the function uses the module-level api
import instead; after removal, run build/tests to ensure there are no remaining
references or linter errors.
| export const getMCPServerByNameInternal = internalQuery( { | ||
| args: { | ||
| serverName: v.string(), | ||
| userId: v.optional(v.id("users")), | ||
| userId: v.optional( v.id( "users" ) ), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| handler: async ( ctx, args ) => { | ||
| // Check if it's a built-in server first | ||
| const builtInServer = BUILT_IN_MCP_SERVERS.find(s => s.name === args.serverName); | ||
| if (builtInServer) { | ||
| const builtInServer = BUILT_IN_MCP_SERVERS.find( s => s.name === args.serverName ); | ||
| if ( builtInServer ) { | ||
| return builtInServer; | ||
| } | ||
|
|
||
| if (args.userId) { | ||
| if ( args.userId ) { | ||
| // Get server by name for specific user | ||
| const userId = args.userId; // Type narrowing | ||
| const server = await ctx.db | ||
| .query("mcpServers") | ||
| .withIndex("by_user_and_name", (q) => | ||
| q.eq("userId", userId).eq("name", args.serverName) | ||
| const serverRaw = await ctx.db | ||
| .query( "mcpServers" ) | ||
| .withIndex( "by_user_and_name", ( q ) => | ||
| q.eq( "userId", userId ).eq( "name", args.serverName ) | ||
| ) | ||
| .first(); | ||
| return server; | ||
| if ( !serverRaw ) return null; | ||
| return { ...serverRaw, source: "user" } as DbMcpServer; | ||
| } else { | ||
| // Get first server by name (for other MCP servers) | ||
| const server = await ctx.db | ||
| .query("mcpServers") | ||
| .filter((q) => q.eq(q.field("name"), args.serverName)) | ||
| const serverRaw = await ctx.db | ||
| .query( "mcpServers" ) | ||
| .filter( ( q ) => q.eq( q.field( "name" ), args.serverName ) ) | ||
| .first(); | ||
| return server; | ||
| if ( !serverRaw ) return null; | ||
| return { ...serverRaw, source: "user" } as DbMcpServer; | ||
| } | ||
| }, | ||
| }); | ||
| } ); |
There was a problem hiding this comment.
Fallback path (no userId) returns the first matching server from any user.
Lines 253-258: when args.userId is not provided, the query fetches the first mcpServers record matching the name across all users. This could return another user's server configuration, which is a potential data leak for an internal query.
Consider whether this fallback is intentional. If so, document it clearly. If not, return null instead.
🤖 Prompt for AI Agents
In `@convex/mcpConfig.ts` around lines 228 - 261, The fallback branch in
getMCPServerByNameInternal (inside the handler when args.userId is falsy)
currently queries the mcpServers table and returns the first record matching
name, which can leak another user's server; change the logic so that when
args.userId is not provided the handler returns null (or otherwise restricts
results to non-user/scoped entries) instead of querying by name across all users
— update the code paths that reference args.userId and the mcpServers query to
short-circuit to null (or add an explicit public/source filter) and adjust any
type/return expectations accordingly.
| export const getMCPServerById = query( { | ||
| args: { | ||
| serverId: v.id("mcpServers"), | ||
| serverId: v.id( "mcpServers" ), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| handler: async ( ctx, args ) => { | ||
| // Get Convex user document ID | ||
| const userId = await getAuthUserId(ctx); | ||
| const userId = await getAuthUserId( ctx ); | ||
|
|
||
| if (!userId) { | ||
| throw new Error("Not authenticated"); | ||
| if ( !userId ) { | ||
| throw new Error( "Not authenticated" ); | ||
| } | ||
|
|
||
| const server = await ctx.db.get(args.serverId); | ||
| const server = await ctx.db.get( args.serverId ); | ||
|
|
||
| if (!server) { | ||
| if ( !server ) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify ownership | ||
| if (server.userId !== userId) { | ||
| throw new Error("Not authorized to access this MCP server"); | ||
| if ( server.userId !== userId ) { | ||
| throw new Error( "Not authorized to access this MCP server" ); | ||
| } | ||
|
|
||
| return server; | ||
| }, | ||
| }); | ||
| } ); |
There was a problem hiding this comment.
Docstring says "internal query" but this is a public query.
Line 503: the comment says "Get MCP server by ID (internal query)" but line 505 uses query() (public). Either update the comment or convert to internalQuery if it's not meant to be client-accessible.
🤖 Prompt for AI Agents
In `@convex/mcpConfig.ts` around lines 505 - 530, The docstring/comment for
getMCPServerById says "internal query" but the export uses query(), exposing it
publicly; either change the comment to reflect that getMCPServerById is a public
query or convert the exported symbol to an internal query by replacing
query(...) with internalQuery(...) (or the project's internal-query helper) so
the handler remains server-only; update any import/usage accordingly and keep
the function name getMCPServerById and its auth/authorization logic unchanged.
1 participant
feat: Enhance token usage tracking and error handling across workflow…
@MikePfunk28
MikePfunk28 committed 2 hours ago
feat: Enhance non-fatal billing error handling and token usage meteri…
@MikePfunk28
MikePfunk28 committed 38 minutes ago
Summary by CodeRabbit
Bug Fixes
Improvements
UI