feat(api): add reindexAppTools function to refresh app tools from MCP #1680
Conversation
- Introduced the `reindexAppTools` function in the registry API to fetch and update tool definitions for a specified app. - Implemented input and output schemas for the function, ensuring proper validation and response structure. - Enhanced error handling to manage cases where the app ID is not found and to log errors during tool fetching. - Streamlined the process of deleting old tools and inserting fresh definitions, improving the overall efficiency of tool management.
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as reindexAppTools
participant DB as Database
participant MCP as MCP Connection
Client->>API: reindexAppTools(appId)
API->>API: Validate workspace/resource access
API->>DB: Verify app exists
rect rgb(200, 220, 240)
note over API,MCP: Fetch fresh tools
API->>MCP: Get tools for app
MCP-->>API: Tool definitions
end
rect rgb(255, 240, 200)
note over API,DB: Delete & reinsert
API->>DB: Delete existing tools for app
API->>DB: Insert fetched tools with updated_at
DB-->>API: Operation results
end
API-->>Client: {appId, toolsCount, deletedCount, upsertedCount}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Deploying decocms-admin-frontend with
|
| Latest commit: |
21893b3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2c4f596c.admin-decocms-frontend.pages.dev |
| Branch Preview URL: | https://tavano-reindex-tool.admin-decocms-frontend.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/sdk/src/mcp/registry/api.ts (1)
624-629: Consider renamingupsertedCountfor accuracy.Since the current implementation uses
insert(notupsert) andupsertedCountequalstoolsCount, the name is misleading. If you adopt the upsert approach, this naming would be accurate; otherwise, considerinsertedCount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk/src/mcp/index.ts(1 hunks)packages/sdk/src/mcp/registry/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
packages/sdk/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/structure.mdc)
Place shared SDK code for data access and MCP integrations under packages/sdk/src
Files:
packages/sdk/src/mcp/index.tspackages/sdk/src/mcp/registry/api.ts
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep shared logic (UI kit, SDK, runtime, CLI tooling) under packages/
Files:
packages/sdk/src/mcp/index.tspackages/sdk/src/mcp/registry/api.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Biome formatting: use two-space indentation and double quotes
Keep imports sorted
Name hooks and utility functions using camelCase
Files:
packages/sdk/src/mcp/index.tspackages/sdk/src/mcp/registry/api.ts
packages/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enforce kebab-case filenames in shared packages
Files:
packages/sdk/src/mcp/index.tspackages/sdk/src/mcp/registry/api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer specific TypeScript types over any
Files:
packages/sdk/src/mcp/index.tspackages/sdk/src/mcp/registry/api.ts
🧬 Code graph analysis (1)
packages/sdk/src/mcp/registry/api.ts (3)
packages/sdk/src/mcp/context.ts (1)
createTool(360-362)packages/sdk/src/mcp/assertions.ts (1)
assertWorkspaceResourceAccess(125-314)packages/ai/src/mcp.ts (1)
listToolsByConnectionType(261-320)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
packages/sdk/src/mcp/index.ts (1)
155-155: LGTM!The new
reindexAppToolstool is correctly added to thePROJECT_TOOLSarray, following the same pattern as other registry tools.packages/sdk/src/mcp/registry/api.ts (3)
543-559: LGTM!The function definition and schemas are well-structured, following the established patterns in this file. The input/output schemas provide clear validation and documentation.
560-575: LGTM!Authorization and app validation are properly implemented. The error message clearly indicates when an app is not found.
588-595: LGTM!Current tool counting logic is correct and includes proper error handling.
| // Fetch fresh tools from the MCP connection | ||
| const { tools = [] } = await listToolsByConnectionType( | ||
| app.connection, | ||
| c, | ||
| true, | ||
| ).catch((e) => { | ||
| console.error("Error fetching tools for reindex:", e); | ||
| return { tools: [] }; | ||
| }); |
There was a problem hiding this comment.
Critical: Potential data loss if tool fetch fails.
If listToolsByConnectionType fails and returns an empty tools array, the function proceeds to delete all existing tools (lines 598-605) and insert nothing, leaving the app with zero tools. This silent failure could cause significant data loss for apps that previously had tools.
Consider failing fast when tool fetch returns empty:
- const { tools = [] } = await listToolsByConnectionType(
- app.connection,
- c,
- true,
- ).catch((e) => {
- console.error("Error fetching tools for reindex:", e);
- return { tools: [] };
- });
+ const result = await listToolsByConnectionType(
+ app.connection,
+ c,
+ true,
+ ).catch((e) => {
+ console.error("Error fetching tools for reindex:", e);
+ throw new Error(`Failed to fetch tools for app "${appId}": ${e.message}`);
+ });
+
+ const { tools = [] } = result;
+
+ // Optionally: warn if legitimately no tools
+ if (tools.length === 0) {
+ console.warn(`App "${appId}" has no tools after reindex`);
+ }📝 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.
| // Fetch fresh tools from the MCP connection | |
| const { tools = [] } = await listToolsByConnectionType( | |
| app.connection, | |
| c, | |
| true, | |
| ).catch((e) => { | |
| console.error("Error fetching tools for reindex:", e); | |
| return { tools: [] }; | |
| }); | |
| // Fetch fresh tools from the MCP connection | |
| const result = await listToolsByConnectionType( | |
| app.connection, | |
| c, | |
| true, | |
| ).catch((e) => { | |
| console.error("Error fetching tools for reindex:", e); | |
| throw new Error(`Failed to fetch tools for app "${appId}": ${e.message}`); | |
| }); | |
| const { tools = [] } = result; | |
| // Optionally: warn if legitimately no tools | |
| if (tools.length === 0) { | |
| console.warn(`App "${appId}" has no tools after reindex`); | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/mcp/registry/api.ts around lines 577 to 585, the current
catch for listToolsByConnectionType swallows errors and returns an empty tools
array which can lead to deleting all existing tools later; change the flow to
fail fast by rethrowing the caught error (or return a rejected promise) instead
of returning { tools: [] }, and add an explicit check after the fetch: if the
fetched tools array is empty but the app currently has existing tools, abort
(throw) and log a clear error so we do not proceed to delete existing tools;
alternatively, only proceed with deletion when the fetch succeeded and returned
a non-empty array (or when the app had no tools to begin with).
| // Delete all existing tools for this app | ||
| if (currentToolCount > 0) { | ||
| const { error: deleteError } = await c.db | ||
| .from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE) | ||
| .delete() | ||
| .eq("app_id", appId); | ||
|
|
||
| if (deleteError) throw deleteError; | ||
| } |
There was a problem hiding this comment.
Critical: Missing transaction boundary risks data loss.
The delete and insert operations (lines 598-605 and 607-622) are not wrapped in a transaction. If the insert operation fails after deletion succeeds, all tools will be lost with no rollback mechanism.
Additionally, this "delete-all then insert-all" approach differs from publishApp (lines 503-516), which calculates a diff and only deletes tools that are no longer present. The diff approach is safer and more efficient.
Consider using a transaction and the diff approach:
+ const currentToolNames = new Set(currentTools?.map((t) => t.name) || []);
+ const newToolNames = new Set(tools.map((t) => t.name));
+
+ // Find tools to delete (exist in DB but not in new tools)
+ const toolsToDelete = Array.from(currentToolNames).filter(
+ (name) => !newToolNames.has(name),
+ );
+
- // Delete all existing tools for this app
- if (currentToolCount > 0) {
+ // Delete old tools that are no longer present
+ if (toolsToDelete.length > 0) {
const { error: deleteError } = await c.db
.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE)
.delete()
- .eq("app_id", appId);
+ .eq("app_id", appId)
+ .in("name", toolsToDelete);
if (deleteError) throw deleteError;
}Then use upsert instead of insert (see next comment).
📝 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.
| // Delete all existing tools for this app | |
| if (currentToolCount > 0) { | |
| const { error: deleteError } = await c.db | |
| .from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE) | |
| .delete() | |
| .eq("app_id", appId); | |
| if (deleteError) throw deleteError; | |
| } | |
| const currentToolNames = new Set(currentTools?.map((t) => t.name) || []); | |
| const newToolNames = new Set(tools.map((t) => t.name)); | |
| // Find tools to delete (exist in DB but not in new tools) | |
| const toolsToDelete = Array.from(currentToolNames).filter( | |
| (name) => !newToolNames.has(name), | |
| ); | |
| // Delete old tools that are no longer present | |
| if (toolsToDelete.length > 0) { | |
| const { error: deleteError } = await c.db | |
| .from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE) | |
| .delete() | |
| .eq("app_id", appId) | |
| .in("name", toolsToDelete); | |
| if (deleteError) throw deleteError; | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/mcp/registry/api.ts around lines 597 to 605 (and following
insert block 607-622), the code deletes all existing tools then inserts new ones
without a transaction and without computing a diff; wrap the delete+insert
sequence in a single database transaction so deletes roll back on insert
failure, and replace the blind delete-all approach with the same diff logic used
in publishApp: compute which tools to delete, update, or add (delete only those
absent in the new list), then perform deletes/inserts/updates inside the
transaction and finally use upsert instead of raw insert for inserts/updates to
avoid conflicts and partial failures.
| // Insert all fresh tools | ||
| const now = new Date().toISOString(); | ||
| await Promise.all( | ||
| tools.map((tool) => | ||
| c.db.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE).insert({ | ||
| // @ts-expect-error - this is a valid field | ||
| app_id: appId, | ||
| name: tool.name, | ||
| description: tool.description || null, | ||
| input_schema: tool.inputSchema || null, | ||
| output_schema: tool.outputSchema || null, | ||
| metadata: null, | ||
| updated_at: now, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer upsert for consistency with publishApp pattern.
The function uses insert (line 611) whereas publishApp uses upsert (line 521). If you adopt the diff approach (per previous comment), using upsert would be more appropriate and consistent with the established pattern.
Apply this diff (assumes diff approach from previous comment):
- // Insert all fresh tools
+ // Upsert new/updated tools
const now = new Date().toISOString();
await Promise.all(
tools.map((tool) =>
- c.db.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE).insert({
+ c.db.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE).upsert(
+ {
// @ts-expect-error - this is a valid field
app_id: appId,
name: tool.name,
description: tool.description || null,
input_schema: tool.inputSchema || null,
output_schema: tool.outputSchema || null,
metadata: null,
updated_at: now,
- }),
+ },
+ {
+ onConflict: "app_id,name",
+ },
+ ),
),
);📝 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.
| // Insert all fresh tools | |
| const now = new Date().toISOString(); | |
| await Promise.all( | |
| tools.map((tool) => | |
| c.db.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE).insert({ | |
| // @ts-expect-error - this is a valid field | |
| app_id: appId, | |
| name: tool.name, | |
| description: tool.description || null, | |
| input_schema: tool.inputSchema || null, | |
| output_schema: tool.outputSchema || null, | |
| metadata: null, | |
| updated_at: now, | |
| }), | |
| ), | |
| ); | |
| // Upsert new/updated tools | |
| const now = new Date().toISOString(); | |
| await Promise.all( | |
| tools.map((tool) => | |
| c.db.from(DECO_CHAT_REGISTRY_APPS_TOOLS_TABLE).upsert( | |
| { | |
| // @ts-expect-error - this is a valid field | |
| app_id: appId, | |
| name: tool.name, | |
| description: tool.description || null, | |
| input_schema: tool.inputSchema || null, | |
| output_schema: tool.outputSchema || null, | |
| metadata: null, | |
| updated_at: now, | |
| }, | |
| { | |
| onConflict: "app_id,name", | |
| }, | |
| ), | |
| ), | |
| ); |
| }); | ||
|
|
||
| export const reindexAppTools = createTool({ | ||
| name: "REGISTRY_REINDEX_APP_TOOLS", |
There was a problem hiding this comment.
use REGISTRY_INDEX_APP_TOOLS
| }), | ||
| ), | ||
| handler: async ({ appId }, c) => { | ||
| await assertWorkspaceResourceAccess(c); |
There was a problem hiding this comment.
you're duplicating the most part of the PUBLISH code. so you just need to query app and invoke PUBLISH tool. also you may want to check access by PUBLISH instead of creating a new policy.
assertWorkspaceResourceAccess("REGISTRY_PUBLISH_APP")
reindex tools tool
Summary by CodeRabbit