Skip to content

feat: add RPC request handling endpoint#11

Open
castlenthesky wants to merge 3 commits into
FalkorDB:mainfrom
castlenthesky:main
Open

feat: add RPC request handling endpoint#11
castlenthesky wants to merge 3 commits into
FalkorDB:mainfrom
castlenthesky:main

Conversation

@castlenthesky
Copy link
Copy Markdown

@castlenthesky castlenthesky commented Feb 7, 2026

  • Enables Antigravity MCP connections
  • Adds three tools:
    • get_graph_schema
    • get_node_properties
    • get_relationship_properties

Summary by CodeRabbit

  • New Features

    • Added MCP protocol support with session-based initialization and unified RPC endpoint for graph workflows
    • Graph toolset: query graphs, list graphs, retrieve schemas, and fetch node/relationship properties
    • Metadata/provider capability responses for MCP compatibility
  • Tests

    • Expanded coverage for MCP flows, session handling, tool calls, and error scenarios
  • Chores

    • Added UUID support for session ID generation

- Enables Antigravity MCP connections
- Adds three tools:
    - get_graph_schema
    - get_node_properties
    - get_relationship_properties
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 7, 2026

📝 Walkthrough

Walkthrough

Adds a new MCPController with JSON-RPC routing, session initialization (uuidv4 usage), MCP tool handling (graph queries, schema/properties, graph listing), metadata/context endpoints, comprehensive tests, and a POST route that delegates RPC requests to the controller. Integrates FalkorDB service calls and sets Mcp-Session-Id header.

Changes

Cohort / File(s) Summary
Dependencies
package.json
Added devDependency @types/uuid.
MCP Controller Implementation
src/controllers/mcp.controller.ts
New MCPController exported as mcpController. Implements initialize, handleRpcRequest, processContextRequest, processMetadataRequest, and listGraphs. Implements JSON-RPC routing, MCP tool calls (graph_query, list_graphs, get_graph_schema, get_node_properties, get_relationship_properties), input validation, FalkorDB interactions, MCP-formatted responses and Mcp-Session-Id header.
MCP Controller Tests
src/controllers/mcp.controller.test.ts
Expanded test suite: mocks uuid.v4, augments mockResponse, and covers init validation/success (header + capabilities), metadata, notifications, ping, tools/list, resources/prompts, multiple tools/call flows (errors, graph_query, list_graphs, schema/properties) and FalkorDB service interactions.
MCP Routes
src/routes/mcp.routes.ts
Added POST / route bound to mcpController.handleRpcRequest.

Sequence Diagram

sequenceDiagram
    participant Client as Client
    participant Controller as MCPController
    participant DB as FalkorDBService
    participant HTTP as Response

    Client->>Controller: POST /mcp (JSON-RPC request)
    Controller->>Controller: Inspect request.method

    alt initialize
        Controller->>Controller: Validate JSON-RPC 2.0
        Controller->>Controller: Generate sessionId (uuidv4)
        Controller->>HTTP: Set "Mcp-Session-Id" header
        Controller->>HTTP: Respond with capabilities (200)
    else tools.call (graph_query)
        Controller->>Controller: Validate tool args (graphName, query)
        Controller->>DB: executeQuery(graphName, query)
        DB-->>Controller: Query results
        Controller->>HTTP: Return JSON-RPC result with content (200)
    else tools.call (list_graphs)
        Controller->>DB: listGraphs()
        DB-->>Controller: Graph list
        Controller->>HTTP: Return formatted graph list (200)
    else metadata/context/ping/notifications
        Controller->>HTTP: Handle accordingly and return 200
    else unknown method
        Controller->>HTTP: Return JSON-RPC error (-32601) with HTTP 200
    end

    HTTP-->>Client: HTTP response with MCP payload
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I thumped a key and spun up a thread,
A uuid crown upon my head,
Graphs and queries leapt in tune,
Falkor hummed beneath the moon,
MCP hops forward — joy, not dread!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add RPC request handling endpoint' accurately summarizes the main change: adding an RPC request handling endpoint (POST route) to support MCP connections, which is the primary objective of this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🤖 Fix all issues with AI agents
In `@package.json`:
- Line 28: package.json currently lists "@types/uuid" as a devDependency which
is redundant because the project uses "uuid@^13.0.0" that includes its own
TypeScript types; remove the "@types/uuid" entry from package.json and run
npm/yarn install to update lockfiles (and optionally run a typecheck) to ensure
no breakages, referencing the "@types/uuid" key to locate the dependency and
"uuid@^13.0.0" to confirm built-in types.
- Line 44: The package.json dependency entry for "uuid" is invalid (currently
"uuid": "^13.0.0"); change it to a valid published version such as "uuid":
"^12.0.0" and run npm install to verify; additionally, because uuid@12 is
ESM-only, audit your imports/usages (places referencing uuid, e.g., import sites
or require calls) and update them to ESM-style imports (or add a CJS-compatible
wrapper) so consumption works after switching to "uuid": "^12.0.0".

In `@src/controllers/mcp.controller.test.ts`:
- Around line 296-316: The test's mock for falkorDBService.executeQuery returns
raw arrays but the controller (mcpController.handleRpcRequest) expects results
with a .data property (e.g., labelsResult.data.map(...)); update the test mocks
to return objects with a data array (e.g., Promise.resolve({ data: [{ label:
'Person' }] }) and Promise.resolve({ data: [{ relationshipType: 'KNOWS' }] })
and for the MATCH branch Promise.resolve({ data: [] })) so the mocked shape
matches executeQuery's real return and the controller's use of .data.

In `@src/controllers/mcp.controller.ts`:
- Line 357: The Cypher query builds a relationship type from unsanitized
args.relationshipType (const query = `MATCH ()-[r:${args.relationshipType}]->()
RETURN r LIMIT 1`), which allows injection; fix it by validating/whitelisting
args.relationshipType before using it (e.g. enforce a strict regex like
/^[A-Za-z0-9_]+$/ or compare against an allowed relationship name list) and
throw an error if invalid, then only interpolate the validated value into the
query; keep using query (and the surrounding method in mcp.controller.ts) but do
not accept arbitrary strings and continue using parameterized neo4j params for
other dynamic values.
- Around line 183-184: The handler branch checking method === 'tools/call'
destructures const { name, arguments: args } = req.body.params which will throw
if req.body.params is missing; update the branch in mcp.controller.ts to first
validate that req.body.params is present and shaped correctly (e.g., ensure
req.body && req.body.params and that params.name exists) and if not, return a
proper error response (400/invalid params) or throw a handled error instead of
destructuring; alternatively, use a safe access/destructuring pattern with
defaults (e.g., const params = req.body?.params || {}) and explicitly check
params.name before proceeding so the code in the tools/call branch does not
crash on missing params.
- Line 319: The Cypher query builds a label via unsanitized interpolation (const
query = `MATCH (n:${args.label}) RETURN n LIMIT 1`), which allows Cypher
injection; fix by validating args.label against a safe regex (e.g., only
letters, digits and underscores) before using it, throw/reject if it fails, and
only then construct the query using the validated label; add or reuse a helper
like validateLabel(label: string) and call it where args.label is used (in the
code that constructs query) to ensure only safe labels are permitted.
- Around line 266-282: The graph schema code wrongly assumes executeQuery
returns an object with a .data array; update the code in get_graph_schema to use
the returned arrays directly: replace labelsResult.data.map(...) with
labelsResult.map(...), typesResult.data.map(...) with typesResult.map(...), and
set connections to schemaResult instead of schemaResult.data; keep the existing
property accessors (e.g., r['label'], r['relationshipType']) so the mapping
logic in the labelsResult, typesResult, and schemaResult handling (const
labelsResult, const typesResult, const schemaResult) still produces nodeLabels,
relationshipTypes, and connections correctly.
🧹 Nitpick comments (1)
src/controllers/mcp.controller.ts (1)

72-429: Consider extracting tool handlers to reduce complexity.

handleRpcRequest is ~360 lines with deeply nested if chains for method routing and tool dispatch. Each tool handler follows the same try/catch + JSON-RPC response pattern. Extracting each tool into its own method (or a tool registry map) would significantly improve readability and testability.

Comment thread package.json
Comment thread package.json Outdated
Comment thread src/controllers/mcp.controller.test.ts
Comment thread src/controllers/mcp.controller.ts
Comment thread src/controllers/mcp.controller.ts Outdated
Comment thread src/controllers/mcp.controller.ts
throw new Error('Missing required arguments: graphName and label');
}

const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Cypher injection via unsanitized label interpolation.

args.label is directly interpolated into the Cypher query string. A malicious input like Person}) RETURN n UNION MATCH (n could alter query semantics or exfiltrate data.

Sanitize by validating that label matches a safe pattern (e.g., alphanumeric/underscores only):

🔒 Proposed fix
             if (!args.graphName || !args.label) {
                 throw new Error('Missing required arguments: graphName and label');
             }

+            if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(args.label)) {
+                throw new Error('Invalid label: must be alphanumeric');
+            }
+
             const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
📝 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.

Suggested change
const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
if (!args.graphName || !args.label) {
throw new Error('Missing required arguments: graphName and label');
}
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(args.label)) {
throw new Error('Invalid label: must be alphanumeric');
}
const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
🤖 Prompt for AI Agents
In `@src/controllers/mcp.controller.ts` at line 319, The Cypher query builds a
label via unsanitized interpolation (const query = `MATCH (n:${args.label})
RETURN n LIMIT 1`), which allows Cypher injection; fix by validating args.label
against a safe regex (e.g., only letters, digits and underscores) before using
it, throw/reject if it fails, and only then construct the query using the
validated label; add or reuse a helper like validateLabel(label: string) and
call it where args.label is used (in the code that constructs query) to ensure
only safe labels are permitted.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not updating pre-existing tools. This PR is focused on enabling RPC request handling for Antigravity editor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@castlenthesky, I appreciate the context, but I wanted to clarify: get_node_properties is actually one of the three new tools being introduced in this PR (along with get_graph_schema and get_relationship_properties), not a pre-existing tool.

Since this is new code being added, the Cypher injection vulnerability would be introduced in this PR. The same issue also affects get_relationship_properties at line 354, where args.relationshipType is interpolated directly into the query string.

Given that this is a critical security vulnerability that could allow attackers to:

  • Manipulate query logic
  • Access unauthorized data
  • Potentially cause denial of service

I'd recommend either:

  1. Fixing it now (should take just a few minutes to add the regex validation to both tools), or
  2. Creating a security issue to track this before the code goes to production

Would you like me to open a security issue to track this if you prefer to address it separately?

Comment thread src/controllers/mcp.controller.ts
This is appropriate error handling.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Fix all issues with AI agents
In `@src/controllers/mcp.controller.ts`:
- Around line 197-236: The graph_query handler currently forwards user-supplied
Cypher to falkorDBService.executeQuery (in the block that checks name ===
'graph_query'), allowing destructive statements; change this to use a read-only
execution path (e.g., call falkorDBService.roQuery or another read-only method)
or validate/reject queries containing write keywords (CREATE, MERGE, DELETE,
SET, REMOVE, DROP, LOAD, CALL) before calling executeQuery so only safe read
queries run; update the error responses accordingly if a write keyword is
detected or if roQuery is unavailable.
- Around line 55-65: The catch block in the MCP session init handler currently
returns the raw error message in the JSON-RPC error data (see the
res.status(500).json(...) call), which leaks internals; remove or sanitize the
error exposure by omitting the error.message from the response (drop the
error.data field or replace it with a generic token like "internal_error") and
keep the full error logged internally via console.error (or processLogger) so
debugging info stays out of the client-facing payload.
- Around line 79-82: The handler for the "notifications/initialized" branch
currently always returns a JSON-RPC response; change it to check the incoming
request's id (e.g., inspect req.body.id) and only send a JSON-RPC response
echoing that id when one is provided (return { jsonrpc: '2.0', result: true, id:
<incoming id> } via res.status(200).json), otherwise treat it as a true
notification and send no body (res.status(204).end() or simply return without
sending a response). Update the logic around the method ===
'notifications/initialized' branch in mcp.controller.ts to implement this
conditional behavior using the existing req and res symbols.
- Around line 72-73: The handleRpcRequest method lacks a guard for missing
request body and doesn't validate the JSON-RPC version like initialize does;
update handleRpcRequest to first check that req.body is defined before
destructuring (return a 400/JSON-RPC parse error if absent), then verify
req.body.jsonrpc === '2.0' and return a proper JSON-RPC error response when it
isn't. Locate the handleRpcRequest function and add these early-return checks
(mirror the validation logic used in initialize) so you avoid destructuring
undefined and enforce the jsonrpc version contract.
- Around line 195-200: The code extracts params.arguments into args and then
accesses args.graphName and args.query which will throw if args is undefined;
change the extraction to safely default args (e.g., const args =
params.arguments ?? {}) or add an explicit guard (if (!args || !args.graphName
|| !args.query) throw ...) inside the graph_query branch (reference: params,
args, 'graph_query') and apply the same defensive pattern to every other tool
branch that reads args to prevent TypeError when params.arguments is omitted.
🧹 Nitpick comments (1)
src/controllers/mcp.controller.ts (1)

72-440: handleRpcRequest is ~370 lines — consider extracting tool handlers.

This single method handles all RPC routing and every tool implementation inline. Extracting each tool handler into its own private method (or a strategy map) would significantly improve readability, testability, and maintainability.

For example:

private toolHandlers: Record<string, (args: any, id: any, res: Response) => Promise<Response>> = {
  'graph_query': this.handleGraphQuery.bind(this),
  'list_graphs': this.handleListGraphs.bind(this),
  // ...
};

Comment thread src/controllers/mcp.controller.ts
Comment thread src/controllers/mcp.controller.ts
Comment thread src/controllers/mcp.controller.ts
Comment on lines +195 to +200
const { name, arguments: args } = params;

if (name === 'graph_query') {
try {
if (!args.graphName || !args.query) {
throw new Error('Missing required arguments: graphName and query are required');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

args may be undefined when params.arguments is omitted, causing a TypeError on line 199.

The validation on lines 184-194 only checks for params.name. If a client sends { name: "graph_query" } without an arguments field, args will be undefined, and args.graphName on line 199 will throw.

🛡️ Suggested fix
         const { name, arguments: args } = params;
+        const safeArgs = args || {};

         if (name === 'graph_query') {
             try {
-                if (!args.graphName || !args.query) {
+                if (!safeArgs.graphName || !safeArgs.query) {

Apply the same pattern to all tool branches that access args.

📝 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.

Suggested change
const { name, arguments: args } = params;
if (name === 'graph_query') {
try {
if (!args.graphName || !args.query) {
throw new Error('Missing required arguments: graphName and query are required');
const { name, arguments: args } = params;
const safeArgs = args || {};
if (name === 'graph_query') {
try {
if (!safeArgs.graphName || !safeArgs.query) {
throw new Error('Missing required arguments: graphName and query are required');
🤖 Prompt for AI Agents
In `@src/controllers/mcp.controller.ts` around lines 195 - 200, The code extracts
params.arguments into args and then accesses args.graphName and args.query which
will throw if args is undefined; change the extraction to safely default args
(e.g., const args = params.arguments ?? {}) or add an explicit guard (if (!args
|| !args.graphName || !args.query) throw ...) inside the graph_query branch
(reference: params, args, 'graph_query') and apply the same defensive pattern to
every other tool branch that reads args to prevent TypeError when
params.arguments is omitted.

Comment on lines +197 to +236
if (name === 'graph_query') {
try {
if (!args.graphName || !args.query) {
throw new Error('Missing required arguments: graphName and query are required');
}

const result = await falkorDBService.executeQuery(
args.graphName,
args.query,
args.params || {}
);

return res.status(200).json({
jsonrpc: '2.0',
id,
result: {
content: [
{
type: "text",
text: JSON.stringify(result, null, 2)
}
]
}
});
} catch (error: any) {
return res.status(200).json({
jsonrpc: '2.0',
id,
result: {
content: [
{
type: "text",
text: `Error executing query: ${error.message}`
}
],
isError: true
}
});
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

graph_query tool accepts arbitrary Cypher — consider a read-only safeguard.

The graph_query tool passes user-supplied Cypher directly to executeQuery. If this is intended for MCP tool consumers (LLM agents, etc.), a write operation like CREATE or DELETE could mutate the database unintentionally. Consider using graph.roQuery() or at minimum documenting this risk.

🤖 Prompt for AI Agents
In `@src/controllers/mcp.controller.ts` around lines 197 - 236, The graph_query
handler currently forwards user-supplied Cypher to falkorDBService.executeQuery
(in the block that checks name === 'graph_query'), allowing destructive
statements; change this to use a read-only execution path (e.g., call
falkorDBService.roQuery or another read-only method) or validate/reject queries
containing write keywords (CREATE, MERGE, DELETE, SET, REMOVE, DROP, LOAD, CALL)
before calling executeQuery so only safe read queries run; update the error
responses accordingly if a write keyword is detected or if roQuery is
unavailable.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/controllers/mcp.controller.test.ts`:
- Around line 182-192: The test passes an uninitialized mockRequest to
mcpController.processMetadataRequest which can cause a TypeError; initialize
mockRequest in the test setup (beforeEach) to a minimal Request-like object
(e.g., an empty object cast to Request or an object with any properties the
controller reads) and use that initialized mockRequest when calling
processMetadataRequest; update the beforeEach or the specific test to assign
mockRequest so processMetadataRequest receives a defined Request and the mocks
(mockResponse, mockStatus, mockJson) continue to be asserted.
🧹 Nitpick comments (2)
src/controllers/mcp.controller.test.ts (2)

225-235: resources/list and prompts/list tests only assert the HTTP status — consider verifying response shape.

These tests pass as long as the handler doesn't throw, regardless of what the response body contains. Adding a mockJson expectation (even a shallow one) would catch regressions in the response structure.


246-349: No tests for tools/call error paths (e.g., executeQuery rejection).

All tool call tests cover the happy path. Consider adding at least one test where executeQuery rejects to verify that the controller returns a well-formed error response rather than crashing or leaking stack traces.

Comment on lines +182 to +192
describe('processMetadataRequest', () => {
test('should return metadata', async () => {
await mcpController.processMetadataRequest(mockRequest as Request, mockResponse as Response);

expect(mockStatus).toHaveBeenCalledWith(200);
expect(mockJson).toHaveBeenCalledWith(expect.objectContaining({
provider: 'FalkorDB MCP Server',
capabilities: expect.any(Array)
}));
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

mockRequest is uninitialized — passing undefined to the controller.

mockRequest is declared on Line 19 but never assigned in beforeEach, and this test doesn't set it either. If processMetadataRequest accesses any property on req, this will throw a TypeError at runtime. Even if the current implementation ignores the request, this is fragile.

Proposed fix
   describe('processMetadataRequest', () => {
     test('should return metadata', async () => {
+      mockRequest = {};
       await mcpController.processMetadataRequest(mockRequest as Request, mockResponse as Response);
📝 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.

Suggested change
describe('processMetadataRequest', () => {
test('should return metadata', async () => {
await mcpController.processMetadataRequest(mockRequest as Request, mockResponse as Response);
expect(mockStatus).toHaveBeenCalledWith(200);
expect(mockJson).toHaveBeenCalledWith(expect.objectContaining({
provider: 'FalkorDB MCP Server',
capabilities: expect.any(Array)
}));
});
});
describe('processMetadataRequest', () => {
test('should return metadata', async () => {
mockRequest = {};
await mcpController.processMetadataRequest(mockRequest as Request, mockResponse as Response);
expect(mockStatus).toHaveBeenCalledWith(200);
expect(mockJson).toHaveBeenCalledWith(expect.objectContaining({
provider: 'FalkorDB MCP Server',
capabilities: expect.any(Array)
}));
});
});
🤖 Prompt for AI Agents
In `@src/controllers/mcp.controller.test.ts` around lines 182 - 192, The test
passes an uninitialized mockRequest to mcpController.processMetadataRequest
which can cause a TypeError; initialize mockRequest in the test setup
(beforeEach) to a minimal Request-like object (e.g., an empty object cast to
Request or an object with any properties the controller reads) and use that
initialized mockRequest when calling processMetadataRequest; update the
beforeEach or the specific test to assign mockRequest so processMetadataRequest
receives a defined Request and the mocks (mockResponse, mockStatus, mockJson)
continue to be asserted.

@gkorland gkorland requested a review from Copilot February 7, 2026 20:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a unified MCP JSON-RPC endpoint to support Antigravity MCP connections and expose graph-focused tools via tools/list and tools/call.

Changes:

  • Added POST /mcp RPC endpoint routing to a new handleRpcRequest controller method
  • Implemented MCP session initialization (initialize) and tool listing/calling flows
  • Added Jest coverage for initialization and RPC method handling + UUID mocking

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
src/routes/mcp.routes.ts Adds the unified MCP RPC POST endpoint wiring to the controller
src/controllers/mcp.controller.ts Implements JSON-RPC dispatch, session initialization, and graph tool handlers
src/controllers/mcp.controller.test.ts Expands tests for initialize + RPC methods and tool calls
package.json Adds TypeScript type dependency for uuid

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


const response = {
jsonrpc: '2.0',
id,
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

In JSON-RPC, id should be null when absent; using id directly can serialize as missing/undefined, producing a non-compliant response. Prefer id: id ?? null (and similarly in other success responses) to keep output consistent with the error responses that already use id || null.

Suggested change
id,
id: id ?? null,

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +74
const { method, id } = req.body;

Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

handleRpcRequest dispatches on method without validating jsonrpc (or even that req.body is an object) while initialize performs validation. This leads to inconsistent behavior where invalid JSON-RPC requests can still execute tools. Add the same basic JSON-RPC validation at the start of handleRpcRequest (e.g., reject when jsonrpc !== '2.0' or method is missing/invalid) and return a JSON-RPC error object.

Suggested change
const { method, id } = req.body;
// Basic JSON-RPC validation similar to initialize()
if (!req.body || typeof req.body !== 'object') {
return res.status(400).json({
jsonrpc: '2.0',
id: null,
error: {
code: -32600,
message: 'Invalid Request: body must be a JSON object'
}
});
}
const { jsonrpc, method, id } = req.body as { jsonrpc?: unknown; method?: unknown; id?: unknown };
if (jsonrpc !== '2.0') {
return res.status(400).json({
jsonrpc: '2.0',
id: id ?? null,
error: {
code: -32600,
message: 'Invalid Request: jsonrpc must be 2.0'
}
});
}
if (typeof method !== 'string' || method.trim() === '') {
return res.status(400).json({
jsonrpc: '2.0',
id: id ?? null,
error: {
code: -32600,
message: 'Invalid Request: method must be a non-empty string'
}
});
}

Copilot uses AI. Check for mistakes.
throw new Error('Missing required arguments: graphName and label');
}

const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

This interpolates a user-provided label directly into a Cypher query, enabling Cypher injection (e.g., crafted label values that alter the query). Since labels/relationship types can’t be parameterized like values, validate args.label against a strict identifier allowlist (e.g., /^[A-Za-z_][A-Za-z0-9_]*$/) and/or safely quote it with backticks after escaping backticks.

Suggested change
const query = `MATCH (n:${args.label}) RETURN n LIMIT 1`;
const label = String(args.label);
const labelPattern = /^[A-Za-z_][A-Za-z0-9_]*$/;
if (!labelPattern.test(label)) {
throw new Error('Invalid label format');
}
const query = `MATCH (n:\`${label}\`) RETURN n LIMIT 1`;

Copilot uses AI. Check for mistakes.
throw new Error('Missing required arguments: graphName and relationshipType');
}

const query = `MATCH ()-[r:${args.relationshipType}]->() RETURN r LIMIT 1`;
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

Same Cypher injection risk as labels: relationshipType is user-controlled and interpolated as an identifier. Apply strict identifier validation and safe quoting/escaping before constructing the query.

Copilot uses AI. Check for mistakes.
Comment on lines +285 to +286
// Get detailed schema connections
const schemaQuery = "MATCH (a)-[r]->(b) RETURN DISTINCT labels(a) as source, type(r) as relationship, labels(b) as target";
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

This schema query will scan relationships across the whole graph, which can be expensive on large datasets. Consider using a schema/introspection procedure if FalkorDB provides one, or limit sampling (e.g., with LIMIT) / add guardrails (timeouts, max result size) to avoid heavy full-graph scans on a tool call.

Suggested change
// Get detailed schema connections
const schemaQuery = "MATCH (a)-[r]->(b) RETURN DISTINCT labels(a) as source, type(r) as relationship, labels(b) as target";
// Get detailed schema connections (bounded to avoid full-graph scans on large datasets)
const schemaQuery = "MATCH (a)-[r]->(b) RETURN DISTINCT labels(a) as source, type(r) as relationship, labels(b) as target LIMIT 1000";

Copilot uses AI. Check for mistakes.
Comment on lines +185 to +194
if (!params || !params.name) {
return res.status(200).json({
jsonrpc: '2.0',
id,
error: {
code: -32602,
message: 'Invalid params: name is required'
}
});
}
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

There’s no test covering the tools/call path where params or params.name is missing and the controller returns -32602. Add a Jest test that calls handleRpcRequest with method: 'tools/call' and missing/invalid params to lock in this error behavior.

Copilot uses AI. Check for mistakes.
Comment thread src/controllers/mcp.controller.ts
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