🧠 refactor: Memoize MCP Permission Checks Per Request - #13419
Conversation
There was a problem hiding this comment.
Pull request overview
Adds request-scoped memoization for MCP server USE permission checks so multi-agent flows and repeated MCP tool invocations within a single request do not re-fetch the user's role. A new checkAccessWithRequestCache helper stores a Promise<boolean> keyed by permission type/permissions/user id/role on a non-enumerable property of the Express request, and userCanUseMCPServers is routed through it. The req object is now threaded through MCP load-time filtering, agent CRUD MCP filtering, and the MCP tool construction/invocation path.
Changes:
- New
checkAccessWithRequestCachehelper inpackages/api/src/middleware/access.tswith unit tests covering memoization, in-flight sharing, request isolation, and permission-key separation. userCanUseMCPServersaccepts and forwardsreq; threaded throughcreateMCPTools/createMCPTool/createToolInstanceand through agent CRUD (filterAuthorizedTools, update/duplicate/revert handlers) andloadTools/loadToolDefinitionsWrapper/loadAgentTools.- New MCP service test asserts a single
getRoleByNamecall across two MCP tool invocations sharing the same request.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/api/src/middleware/access.ts | Adds CheckAccessParams type and checkAccessWithRequestCache with a per-request promise cache. |
| packages/api/src/middleware/access.spec.ts | New tests for memoization, in-flight sharing, request isolation, and per-permission cache keys. |
| api/server/services/MCP.js | Switches userCanUseMCPServers to the cached helper and plumbs req through MCP tool construction/invocation. |
| api/server/services/MCP.spec.js | Adds test verifying cached role lookup across two MCP tool invocations on one request. |
| api/server/services/ToolService.js | Passes req into userCanUseMCPServers in both tool definition and tool loading paths. |
| api/server/controllers/agents/v1.js | Threads req into filterAuthorizedTools and MCP permission checks across create/update/duplicate/revert. |
| api/app/clients/tools/util/handleTools.js | Passes req into the MCP permission check and into MCP tool construction params. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
GitNexus: 🚀 deployedThe |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9b28b87 to
333837f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
GitNexus: 🚀 deployedThe |
Summary
I added request-scoped memoization for MCP server use permission checks so multi-agent requests and repeated MCP tool execution reuse the same role permission lookup.
checkAccessWithRequestCacheto cache simple role permission checks by permission type, permissions, user ID, and role on the Express request.canUseServers(user)instead of raw Express request state.Change Type
Testing
cd packages/api && npx jest src/middleware/access.spec.ts --runInBand --coverage=falsenpm run build:apicd api && npx jest server/services/MCP.spec.js --runInBand --coverage=falsecd api && npx jest server/services/__tests__/ToolService.spec.js server/controllers/agents/filterAuthorizedTools.spec.js --runInBand --coverage=falsecd api && npx jest server/services/MCP.spec.js server/services/__tests__/ToolService.spec.js server/controllers/agents/filterAuthorizedTools.spec.js --runInBand --coverage=falsenpx eslint packages/api/src/middleware/access.ts packages/api/src/middleware/access.spec.ts api/server/services/MCP.js api/server/services/MCP.spec.js api/app/clients/tools/util/handleTools.js api/server/services/ToolService.js api/server/controllers/agents/v1.jsnpx eslint api/server/services/MCP.js api/server/services/MCP.spec.js api/app/clients/tools/util/handleTools.js api/server/services/ToolService.js api/server/services/__tests__/ToolService.spec.js api/server/controllers/agents/v1.js api/server/controllers/agents/filterAuthorizedTools.spec.jsTest Configuration:
dev/Users/danny/.codex/worktrees/9dfc/LibreChatChecklist