Skip to content

Conversation

@MatLBS
Copy link
Contributor

@MatLBS MatLBS commented Feb 9, 2026

  • Implemented Mcp using the library from the AI SDK : https://ai-sdk.dev/docs/ai-sdk-core/mcp-tools
  • Mcps are initialized when we click on 'connect' in project page or when a first message is sent to the agent
  • Servers runs during 5 minutes. If no tools is called during this laps of time, it is closed
Enregistrement.de.l.ecran.2026-02-09.a.16.26.45.mov

@claude
Copy link

claude bot commented Feb 9, 2026

Code Review

I've found several issues in this PR that need attention:

1. 🐛 Critical Bug: Debounce function doesn't actually debounce

File: apps/shared/src/utils.ts (lines 1-8)

The debounce function is missing the core mechanism of debouncing - it never stores and clears previous timers. Every call schedules a new setTimeout without canceling previous ones, so all calls will execute after the delay.

This is used in mcp.service.ts to debounce file watcher changes, but because it's broken, multiple rapid file change events will trigger multiple concurrent calls to handleCacheMcpServerState() instead of being collapsed into a single call, causing race conditions.

Fix: The function needs to track and clear the timer:

export function debounce<T extends (...args: any[]) => any>(func: T, delay: number): (...args: Parameters<T>) => void {
	let timer: ReturnType<typeof setTimeout>;
	return (...args: Parameters<T>) => {
		clearTimeout(timer);
		timer = setTimeout(() => {
			func(...args);
		}, delay);
	};
}

Reference:

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function debounce<T extends (...args: any[]) => any>(func: T, delay: number): (...args: Parameters<T>) => void {
return (...args: Parameters<T>) => {
setTimeout(() => {
func(...args);
}, delay);
};
}


2. 🔒 Security: Missing admin authorization on reconnect endpoint

File: apps/backend/src/trpc/mcp.routes.ts (lines 9-12)

The reconnect endpoint uses protectedProcedure (any authenticated user) instead of adminProtectedProcedure. This endpoint triggers handleCacheMcpServerState() which spawns child processes based on MCP config file commands.

While the UI only shows the reconnect button to admins, any authenticated user can call mcp.reconnect directly via the tRPC API, bypassing the frontend guard.

Fix: Change to adminProtectedProcedure:

reconnect: adminProtectedProcedure.mutation(async () => {
    await mcpService.handleCacheMcpServerState();
    return mcpService.cachedMcpState;
}),

Reference:

reconnect: protectedProcedure.mutation(async () => {
await mcpService.handleCacheMcpServerState();
return mcpService.cachedMcpState;
}),
});


3. 📝 CLAUDE.md: Use full names instead of abbreviations

Per CLAUDE.md: "Use descriptive names — code should read like prose; avoid abbreviations"

a) apps/backend/src/utils/utils.ts:

  • replaceEnvVars should be replaceEnvironmentVariables
  • varName parameter should be variableName

Reference:

export const replaceEnvVars = (fileContent: string) => {
const replaced = fileContent.replace(/\$\{(\w+)\}/g, (match, varName) => {
return process.env[varName] || match;
});
return JSON.parse(replaced);
};

b) apps/backend/src/mcp/mcp.client.ts:

  • _ttlMs should be _timeToLiveMilliseconds or _idleTimeoutMilliseconds

Reference:

private _runningServers: Map<string, RunningMcpClient> = new Map();
private _ttlMs: number = 5 * 60 * 1000; // 5 minutes
private _cleanupInterval: NodeJS.Timeout | null = null;


4. 📝 CLAUDE.md: Remove unnecessary JSDoc comment

File: apps/backend/src/utils/tools.ts (lines 232-239)

Per CLAUDE.md: "Minimize comments — only comment complex or ambiguous logic"

The 5-line JSDoc on sanitizeTools just restates what the function name and code already make clear. The function's logic is straightforward and self-explanatory.

Recommendation: Remove the JSDoc comment.

Reference:

/**
* Sanitizes MCP tool schemas by:
* - Ensures all array types have an items property
* - Removes null from required arrays
* - Recursively processes nested objects
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function sanitizeTools(schema: any): any {

@Standlc
Copy link
Collaborator

Standlc commented Feb 10, 2026

Remove the changes in the package-lock.json, rebase with main, make sure to use the correct node version and npm i again because your package-lock changes are suspicious

@MatLBS MatLBS force-pushed the feat/implement_mcp branch from 760a942 to 9eb6773 Compare February 10, 2026 11:04
Copy link
Collaborator

@Standlc Standlc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the MCP service and client classes not well integrated and architected. They both share always the same properties (cached tools for example) and seem to be at the same level of abstraction (the client doesn't really do more "low" level things than the service that parses the mcp config (which is pretty low level) for example).

The "client" really handles all the MCP servers and tools, which is not what a client typically does (a client should be given the configuration for what server to talk to how to talk to it).

Some method names could be clearer: handleCacheMcpServerState gives a lot of details (caching, state, server) about the internals of the service. Instead, loadTools is clear, accurate, and straight to the point.

@MatLBS MatLBS force-pushed the feat/implement_mcp branch from 69be439 to a3c0fb2 Compare February 10, 2026 17:17
@MatLBS MatLBS force-pushed the feat/implement_mcp branch from a3c0fb2 to 96c2b13 Compare February 10, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants