ALT-37: Exponer backend como MCP server con 6 tools y RBAC#6
Conversation
![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.
|
@greptile review |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSe introduce la integración del Protocolo de Contexto Modelo (MCP) en la aplicación backend de NestJS, agregando un nuevo módulo MCP con controlador y servicio que implementa seis herramientas (búsqueda de animales, estado de subsidios, solicitudes de rescate, capacidad de hogares de acogida, reportes de abuso y KPIs municipales) con autenticación y autorización basada en roles. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant McpController
participant AuthService
participant McpService
participant MCP Server
participant UnderlyingServices as UnderlyingServices<br/>(Animals, Subsidies, etc.)
Client->>McpController: POST /mcp con Authorization header
McpController->>AuthService: validateToken(token)
alt Token válido
AuthService-->>McpController: ✓ válido
McpController->>McpController: setCurrentRequestUser(user)
McpController->>McpService: getServer()
McpService-->>McpController: MCP Server instance
McpController->>MCP Server: handleRequest(req, res, body)
MCP Server->>MCP Server: Procesa herramienta MCP
MCP Server->>UnderlyingServices: Ejecuta servicio<br/>(findAll, findOne, etc.)
UnderlyingServices-->>MCP Server: Resultado
MCP Server-->>Client: Respuesta JSON-RPC
else Token inválido/ausente
AuthService-->>McpController: ✗ rechazado
McpController-->>Client: 401 + JSON-RPC error (code: -32001)
end
McpController->>McpController: setCurrentRequestUser(null)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/backend/src/mcp/mcp.service.spec.ts (1)
70-170: Estas specs aún no ejercitan las tools registradas.El bloque llama
animalsService.findAll(),findByStatus(),subsidiesService.findOne(), etc. directamente, así que no cubreregisterTool(), el mapping de argumentos ni el RBAC real deMcpService. Incluso la prueba de inicialización sólo comprueba que existaconnect(), no que haya 6 tools registradas. Conviene disparartools/callcontra elMcpServer/transport o, como mínimo, ejecutar los callbacks registrados.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/mcp/mcp.service.spec.ts` around lines 70 - 170, Las pruebas actualmente llaman los servicios directamente en vez de invocar las herramientas registradas por McpService; cambia los tests para inicializar con service.onModuleInit(), obtener el McpServer con service.getServer() y disparar los handlers registrados (en vez de llamar animalsService.findAll/findByStatus/subsidiesService.findOne/ etc. directamente) ya sea invocando los callbacks que McpServer registra vía registerTool() o enviando una petición de tipo "tools/call" al transport del servidor; usa setCurrentRequestUser(...) para cada escenario antes de invocar el handler y luego verifica que los mocks (animalsService, subsidiesService, captureStorage, casasCunasService, abuseReportsService, jurisdictionsService) hayan sido llamados con los argumentos esperados.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/mcp/mcp.service.ts`:
- Around line 302-340: The KPI aggregation in the method that calls
jurisdictionsService.findOne currently uses casasCunasService.findAll() and
animalsService.findAll() (variables casasCunas, animals, totalCapacity,
totalOccupied, kpis) so results are global and loaded into memory; change those
calls to scoped queries that filter by args.jurisdictionId (e.g., replace
findAll() with a service method like findByJurisdiction(jurisdictionId) or add a
filter argument) and, where possible, replace in-memory reduces with database
aggregate/count queries (sum capacity, sum currentCount, count by status) to
compute totalCapacity, totalOccupied and animals counts without loading all
entities into memory. Ensure the kpis.jurisdiction still uses
jurisdictionsService.findOne and that the fosterHomes and animals totals use the
filtered/aggregated results.
- Around line 212-236: El handler valida un argumento jurisdictionId en
inputSchema pero nunca lo usa: actualmente siempre llama
this.casasCunasService.findAll() y serializa todas las casas cuna
(capacityData), por lo que el filtro por jurisdicción no tiene efecto; fix: leer
args.jurisdictionId y pasar ese filtro al servicio (p. ej. usar
this.casasCunasService.findByJurisdictionId(args.jurisdictionId) si existe, o
this.casasCunasService.findAll({ jurisdictionId: args.jurisdictionId })/filtrar
el array devuelto antes de mapear) y luego construir capacityData solo sobre los
registros filtrados; mantén intactas las propiedades usadas (id, name, province,
canton, district, capacity, currentCount, isVerified).
- Around line 20-33: The module-level variable currentRequestUser causes
cross-request leaks; replace this pattern with an AsyncLocalStorage-based
context: create an AsyncLocalStorage<McpUserContext | null> instance and
implement a runWithCurrentRequestUser(context, fn) helper that calls
als.run(context, fn); refactor setCurrentRequestUser and requireAuth to
read/write the current user via the AsyncLocalStorage (requireAuth should
retrieve the store and throw if null) and remove reliance on the module-scoped
currentRequestUser; finally update the controller to wrap the
transport.handleRequest call inside runWithCurrentRequestUser(...) so each
request has its own isolated context.
- Around line 91-101: The inputSchema currently allows any string for
args.status and then force-casts it to call this.animalsService.findByStatus(),
risking invalid enum values; update validation to enforce AnimalStatus (e.g.,
use z.nativeEnum(AnimalStatus) or validate args.status against the AnimalStatus
enum) and only call this.animalsService.findByStatus(args.status as
AnimalStatus) after the check (otherwise throw a validation error); reference
inputSchema, AnimalStatus, args.status, and animalsService.findByStatus to
locate where to change.
---
Nitpick comments:
In `@apps/backend/src/mcp/mcp.service.spec.ts`:
- Around line 70-170: Las pruebas actualmente llaman los servicios directamente
en vez de invocar las herramientas registradas por McpService; cambia los tests
para inicializar con service.onModuleInit(), obtener el McpServer con
service.getServer() y disparar los handlers registrados (en vez de llamar
animalsService.findAll/findByStatus/subsidiesService.findOne/ etc. directamente)
ya sea invocando los callbacks que McpServer registra vía registerTool() o
enviando una petición de tipo "tools/call" al transport del servidor; usa
setCurrentRequestUser(...) para cada escenario antes de invocar el handler y
luego verifica que los mocks (animalsService, subsidiesService, captureStorage,
casasCunasService, abuseReportsService, jurisdictionsService) hayan sido
llamados con los argumentos esperados.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aab5ed40-91dd-451b-bd0d-e1a914850062
⛔ Files ignored due to path filters (1)
apps/backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
apps/backend/package.jsonapps/backend/src/app.module.tsapps/backend/src/mcp/mcp.controller.spec.tsapps/backend/src/mcp/mcp.controller.tsapps/backend/src/mcp/mcp.module.tsapps/backend/src/mcp/mcp.service.spec.tsapps/backend/src/mcp/mcp.service.tsmcp.json
| // Shared state for passing user context to tool handlers | ||
| // This is set per-request before the transport handles the message | ||
| let currentRequestUser: McpUserContext | null = null; | ||
|
|
||
| export function setCurrentRequestUser(user: McpUserContext | null): void { | ||
| currentRequestUser = user; | ||
| } | ||
|
|
||
| function requireAuth(): McpUserContext { | ||
| if (!currentRequestUser) { | ||
| throw new Error('Authentication required'); | ||
| } | ||
| return currentRequestUser; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "mcp.service.ts" apps/backendRepository: altrupets/monorepo
Length of output: 97
🏁 Script executed:
fd -t f "mcp.controller.ts" apps/backendRepository: altrupets/monorepo
Length of output: 100
🏁 Script executed:
cat -n apps/backend/src/mcp/mcp.service.tsRepository: altrupets/monorepo
Length of output: 12824
🏁 Script executed:
cat -n apps/backend/src/mcp/mcp.controller.tsRepository: altrupets/monorepo
Length of output: 4328
No guardes el usuario actual en una variable global compartida.
Ese estado vive a nivel de módulo (línea 22) y apps/backend/src/mcp/mcp.controller.ts lo sobrescribe en línea 60, antes de que transport.handleRequest() (línea 79) ejecute los handlers de las tools de forma asincrónica. Con dos POST /mcp concurrentes, una tool puede evaluar RBAC o persistir reporterId con la identidad de otra solicitud, abriendo una fuga crítica de autorización entre requests.
Usa AsyncLocalStorage para aislar el contexto por cadena de ejecución asincrónica:
🔒 Enfoque de corrección
+import { AsyncLocalStorage } from 'node:async_hooks';
...
-let currentRequestUser: McpUserContext | null = null;
-
-export function setCurrentRequestUser(user: McpUserContext | null): void {
- currentRequestUser = user;
-}
+const requestUserStorage = new AsyncLocalStorage<McpUserContext>();
+
+export function runWithCurrentRequestUser<T>(
+ user: McpUserContext,
+ callback: () => Promise<T>,
+): Promise<T> {
+ return requestUserStorage.run(user, callback);
+}
function requireAuth(): McpUserContext {
+ const currentRequestUser = requestUserStorage.getStore();
if (!currentRequestUser) {
throw new Error('Authentication required');
}
return currentRequestUser;
}// apps/backend/src/mcp/mcp.controller.ts (línea ~60)
await runWithCurrentRequestUser(
{ id: user.id, username: user.username, roles: user.roles },
async () => {
const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: undefined,
});
await this.mcpService.getServer().connect(transport);
await transport.handleRequest(req as any, res as any, req.body);
},
);📝 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.
| // Shared state for passing user context to tool handlers | |
| // This is set per-request before the transport handles the message | |
| let currentRequestUser: McpUserContext | null = null; | |
| export function setCurrentRequestUser(user: McpUserContext | null): void { | |
| currentRequestUser = user; | |
| } | |
| function requireAuth(): McpUserContext { | |
| if (!currentRequestUser) { | |
| throw new Error('Authentication required'); | |
| } | |
| return currentRequestUser; | |
| } | |
| import { AsyncLocalStorage } from 'node:async_hooks'; | |
| // Shared state for passing user context to tool handlers | |
| // Context is isolated per async execution chain using AsyncLocalStorage | |
| const requestUserStorage = new AsyncLocalStorage<McpUserContext>(); | |
| export function runWithCurrentRequestUser<T>( | |
| user: McpUserContext, | |
| callback: () => Promise<T>, | |
| ): Promise<T> { | |
| return requestUserStorage.run(user, callback); | |
| } | |
| function requireAuth(): McpUserContext { | |
| const currentRequestUser = requestUserStorage.getStore(); | |
| if (!currentRequestUser) { | |
| throw new Error('Authentication required'); | |
| } | |
| return currentRequestUser; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/mcp/mcp.service.ts` around lines 20 - 33, The module-level
variable currentRequestUser causes cross-request leaks; replace this pattern
with an AsyncLocalStorage-based context: create an
AsyncLocalStorage<McpUserContext | null> instance and implement a
runWithCurrentRequestUser(context, fn) helper that calls als.run(context, fn);
refactor setCurrentRequestUser and requireAuth to read/write the current user
via the AsyncLocalStorage (requireAuth should retrieve the store and throw if
null) and remove reliance on the module-scoped currentRequestUser; finally
update the controller to wrap the transport.handleRequest call inside
runWithCurrentRequestUser(...) so each request has its own isolated context.
| inputSchema: z.object({ | ||
| species: z.string().optional(), | ||
| status: z.string().optional(), | ||
| }), | ||
| }, | ||
| async (args) => { | ||
| const user = requireAuth(); | ||
|
|
||
| let animals; | ||
| if (args.status) { | ||
| animals = await this.animalsService.findByStatus(args.status as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the AnimalStatus enum
fd -t f animal.entity.ts apps/backendRepository: altrupets/monorepo
Length of output: 112
🏁 Script executed:
# Check the mcp.service.ts file to see the current code
head -120 apps/backend/src/mcp/mcp.service.ts | tail -40Repository: altrupets/monorepo
Length of output: 1211
🏁 Script executed:
# Find the animals.service.ts and check findByStatus() signature
fd -t f animals.service.ts apps/backendRepository: altrupets/monorepo
Length of output: 105
🏁 Script executed:
cat -n apps/backend/src/animals/entities/animal.entity.tsRepository: altrupets/monorepo
Length of output: 3363
🏁 Script executed:
cat -n apps/backend/src/animals/animals.service.ts | head -100Repository: altrupets/monorepo
Length of output: 2476
Validar status con AnimalStatus antes de llamar a findByStatus().
El schema actual acepta cualquier string y luego lo fuerza con as any, pero findByStatus() espera un valor del enum AnimalStatus. Un valor inválido puede causar errores silenciosos en la consulta de base de datos.
Cambio sugerido
+import { AnimalStatus } from '../animals/entities/animal.entity';
...
- status: z.string().optional(),
+ status: z.enum([AnimalStatus.RESCUED, AnimalStatus.IN_CASA_CUNA, AnimalStatus.ADOPTED, AnimalStatus.RETURNED]).optional(),
...
- animals = await this.animalsService.findByStatus(args.status as any);
+ animals = await this.animalsService.findByStatus(args.status);📝 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.
| inputSchema: z.object({ | |
| species: z.string().optional(), | |
| status: z.string().optional(), | |
| }), | |
| }, | |
| async (args) => { | |
| const user = requireAuth(); | |
| let animals; | |
| if (args.status) { | |
| animals = await this.animalsService.findByStatus(args.status as any); | |
| inputSchema: z.object({ | |
| species: z.string().optional(), | |
| status: z.enum([AnimalStatus.RESCUED, AnimalStatus.IN_CASA_CUNA, AnimalStatus.ADOPTED, AnimalStatus.RETURNED]).optional(), | |
| }), | |
| }, | |
| async (args) => { | |
| const user = requireAuth(); | |
| let animals; | |
| if (args.status) { | |
| animals = await this.animalsService.findByStatus(args.status); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/mcp/mcp.service.ts` around lines 91 - 101, The inputSchema
currently allows any string for args.status and then force-casts it to call
this.animalsService.findByStatus(), risking invalid enum values; update
validation to enforce AnimalStatus (e.g., use z.nativeEnum(AnimalStatus) or
validate args.status against the AnimalStatus enum) and only call
this.animalsService.findByStatus(args.status as AnimalStatus) after the check
(otherwise throw a validation error); reference inputSchema, AnimalStatus,
args.status, and animalsService.findByStatus to locate where to change.
| // Fetch jurisdiction info | ||
| const jurisdiction = await this.jurisdictionsService.findOne( | ||
| args.jurisdictionId, | ||
| ); | ||
|
|
||
| // Aggregate KPI data from multiple sources | ||
| const casasCunas = await this.casasCunasService.findAll(); | ||
| const animals = await this.animalsService.findAll(); | ||
|
|
||
| const totalCapacity = casasCunas.reduce( | ||
| (sum, cc) => sum + (cc.capacity ?? 0), | ||
| 0, | ||
| ); | ||
| const totalOccupied = casasCunas.reduce( | ||
| (sum, cc) => sum + (cc.currentCount ?? 0), | ||
| 0, | ||
| ); | ||
|
|
||
| const kpis = { | ||
| jurisdiction: { | ||
| id: jurisdiction.id, | ||
| name: jurisdiction.name, | ||
| }, | ||
| fosterHomes: { | ||
| total: casasCunas.length, | ||
| totalCapacity, | ||
| totalOccupied, | ||
| occupancyRate: | ||
| totalCapacity > 0 | ||
| ? Math.round((totalOccupied / totalCapacity) * 100) | ||
| : 0, | ||
| }, | ||
| animals: { | ||
| total: animals.length, | ||
| rescued: animals.filter((a) => a.status === 'RESCUED').length, | ||
| inCasaCuna: animals.filter((a) => a.status === 'IN_CASA_CUNA') | ||
| .length, | ||
| adopted: animals.filter((a) => a.status === 'ADOPTED').length, | ||
| }, |
There was a problem hiding this comment.
Los KPIs municipales se calculan con datos globales.
jurisdictionId sólo se usa para cargar el nombre y el ID de la jurisdicción; la ocupación y los totales salen de casasCunasService.findAll() y animalsService.findAll() sin filtro. Dos municipalidades distintas recibirían los mismos KPIs con distinto encabezado, y además esta agregación siempre carga todo en memoria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/mcp/mcp.service.ts` around lines 302 - 340, The KPI
aggregation in the method that calls jurisdictionsService.findOne currently uses
casasCunasService.findAll() and animalsService.findAll() (variables casasCunas,
animals, totalCapacity, totalOccupied, kpis) so results are global and loaded
into memory; change those calls to scoped queries that filter by
args.jurisdictionId (e.g., replace findAll() with a service method like
findByJurisdiction(jurisdictionId) or add a filter argument) and, where
possible, replace in-memory reduces with database aggregate/count queries (sum
capacity, sum currentCount, count by status) to compute totalCapacity,
totalOccupied and animals counts without loading all entities into memory.
Ensure the kpis.jurisdiction still uses jurisdictionsService.findOne and that
the fosterHomes and animals totals use the filtered/aggregated results.
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.
Add Model Context Protocol server to the NestJS backend, exposing 6 tools for AI agent integration: search-animals, get-subsidy-status, list-rescue-requests, get-foster-home-capacity, create-abuse-report, and get-municipal-kpis. Each tool enforces role-based access control via JWT authentication on the POST /mcp endpoint using the StreamableHTTPServerTransport in stateless mode. Includes unit tests for both McpService (tool registration) and McpController (auth rejection), fixes a pre-existing syntax error in app.module.ts (stray semicolon in CacheModule config), and registers the backend MCP server entry in the root mcp.json. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…filter by jurisdiction - get-foster-home-capacity: jurisdictionId parameter now filters casas cunas by matching province/canton/district from the jurisdiction entity - get-municipal-kpis: KPIs now scoped to jurisdiction geography instead of querying all data globally; animals filtered by casaCunaId membership Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lapc506
force-pushed
the
ALT-37-backend-mcp-server
branch
from
8e0dcac to
8f84fdd
Compare
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.
1 participant
Summary
Linear Issue
ALT-37
OpenSpec Change
openspec/changes/backend-mcp-server/(6 fases completas)Files Changed (8)
Test plan
npm run buildcompila sin erroresCreated by Claude Code on behalf of @lapc506
🤖 Generated with Claude Code
Summary by CodeRabbit
Notas de Lanzamiento
Nuevas Características
Chores