Conversation
…rvice integration - Updated package dependencies in package.json and package-lock.json to latest versions. - Refactored the server implementation in src/index.ts to utilize the Model Context Protocol SDK for handling graph operations. - Introduced a new Redis service in src/services/redis.service.ts for managing key-value storage. - Enhanced configuration to include Redis connection details in src/config/index.ts. - Changed TypeScript target and module settings in tsconfig.json to ES2022 for improved compatibility.
- Remove legacy HTTP API components (controllers, routes, middleware) - Clean up Express dependencies from package.json - Simplify graph_list resource to return markdown format - Add comprehensive CLAUDE.md for development guidance - Modernize README with enhanced setup instructions and examples - Focus entirely on MCP server functionality via stdio transport 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace @/* path aliases with relative imports in src/index.ts and src/mcp/resources.ts - Update logger service to use platformdirs for cross-platform log directory - Add platformdirs dependency and mock in Jest configuration - Fix transformIgnorePatterns in jest.config.js for ES module compatibility - Update lint configuration to use flat config format (eslint.config.js) - Add error handling for log directory creation with fallback - All tests now pass and build succeeds without module resolution errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Updated .env.example to reflect new environment and Redis configurations. - Added CONTRIBUTING.md to provide guidelines for community contributions. - Modified .gitignore to exclude log files and MCP-specific artifacts. - Updated package.json to require Node.js 18+ and added prepublishOnly script for CI checks. - Improved README.md with clearer setup instructions and additional commands. - Refined GitHub Actions workflow for CI/CD, including security audits and coverage reporting. - Refactored prompts in src/mcp/prompts.ts to include a new memory query prompt for enhanced functionality.
- Added environment name and URL for the release job in the node.yml workflow to link to the published package on npm.
- Bump package version to 1.0.1 in package.json. - Add instructions for running the server from npm in README.md. - Include a shebang in src/index.ts for direct execution. - Introduce a new graph reorganization prompt in src/mcp/prompts.ts for improved graph management.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Katie Mulliken <katie@mulliken.net>
chore: update version and enhance README with npm usage instructions
Add comprehensive read-only query support to enable safe querying of FalkorDB read replicas and prevent accidental write operations. Features: - Add FALKORDB_DEFAULT_READONLY config option for global read-only mode - Enhance query_graph tool with optional readOnly parameter - Add dedicated query_graph_readonly tool for explicit read-only queries - Use graph.roQuery() for read-only operations (GRAPH.RO_QUERY) - Maintain backward compatibility (default: read-write mode) Implementation: - Update FalkorDBService.executeQuery() with readOnly parameter - Add FalkorDBService.executeReadOnlyQuery() convenience method - Add config.falkorDB.defaultReadOnly setting - Update MCP tools with read-only support - Add comprehensive test coverage (24 new tests) Tests: - All 100 tests passing - Test coverage for read-only query execution - Test both roQuery() and query() code paths - Test error handling for read-only operations - Test configuration defaults Documentation: - Update README.md with read-only mode usage - Update CLAUDE.md with API documentation - Add .env.example configuration - Document use cases for replica instances Use Cases: - Connect to FalkorDB read replicas in replication setups - Prevent accidental writes in production environments - Run analytics/reporting queries safely - Provide read-only access in multi-tenant setups Breaking Changes: None Backward Compatibility: Fully maintained
feat: add read-only query support for replica instances
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the Express HTTP MCP server with a stdio-based ModelContextProtocol server; adds Redis support, centralized error and logging subsystems, robust FalkorDB and Redis initialization with retries, extensive MCP tools/resources/prompts, ESM/tooling migration, and large additions to tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Starter as Startup
participant CFG as Config
participant LOG as Logger
participant FDB as FalkorDB Service
participant RED as Redis Service
participant MCP as McpServer
Starter->>CFG: load env & defaults
Starter->>LOG: instantiate logger
Starter->>FDB: initialize()
FDB-->>Starter: success / retries
Starter->>RED: initialize()
RED-->>Starter: success / retries
Starter->>MCP: create McpServer (stdio)
Starter->>LOG: setMcpServer(MCP)
Starter->>MCP: register tools/resources/prompts
Starter->>MCP: start listening (stdio)
MCP-->>Starter: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Rebases the project onto an MCP stdio-based server architecture (moving away from the prior Express HTTP API), adding service-layer resiliency, logging, and expanded MCP capabilities.
Changes:
- Replaced the Express server/routes/auth/controller stack with an MCP server over stdio, plus MCP tools/resources/prompts registration.
- Added Redis + enhanced FalkorDB services with retries, structured logging, and centralized error handling.
- Updated project config/tooling (ESM, ESLint flat config, CI workflow, docs) and added/expanded Jest test coverage.
Reviewed changes
Copilot reviewed 30 out of 34 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/connection-parser.test.ts | Adds unit tests for connection string parsing behavior and edge cases |
| src/services/redis.service.ts | Introduces Redis client service with initialization, retries, and basic operations |
| src/services/redis.service.test.ts | Adds Jest coverage for Redis service initialize/get/set/close behavior |
| src/services/logger.service.ts | Adds MCP-aware logger with optional file logging fallback |
| src/services/logger.service.test.ts | Adds basic logger interface/resilience tests |
| src/services/falkordb.service.ts | Refactors FalkorDB service to explicit initialization + retries + typed replies + logging |
| src/services/falkordb.service.test.ts | Expands FalkorDB service tests for init, read-only queries, deleteGraph, errors |
| src/routes/mcp.routes.ts | Removes old Express MCP routes |
| src/middleware/auth.middleware.ts | Removes old Express API key auth middleware |
| src/controllers/mcp.controller.ts | Removes old Express MCP controller |
| src/controllers/mcp.controller.test.ts | Removes tests tied to removed Express controller |
| src/models/mcp-client-config.test.ts | Adds tests validating MCP client/server config model shapes |
| src/mcp/tools.ts | Adds MCP tools for graph/redis operations (query/list/delete/set/get) |
| src/mcp/resources.ts | Adds MCP resource for graph listing |
| src/mcp/prompts.ts | Adds MCP prompts (user setup, memory query, graph reorg) |
| src/index.ts | Replaces Express startup with MCP server startup + shutdown + global error handlers |
| src/errors/ErrorHandler.ts | Adds centralized error handler and trusted/untrusted crash policy |
| src/errors/ErrorHandler.test.ts | Adds tests for ErrorHandler behavior |
| src/errors/AppError.ts | Adds AppError + common error names for consistent error handling |
| src/config/index.ts | Extends config (read-only default + redis config) and tweaks dotenv loading |
| src/config/index.test.ts | Updates config tests for new falkorDB defaultReadOnly |
| package.json | Migrates to ESM + updates deps/scripts/metadata; adds redis/platformdirs/zod |
| eslint.config.js | Adds ESLint flat config for TS |
| README.md | Updates docs for MCP stdio usage, new tools, and configuration |
| CONTRIBUTING.md | Adds contribution guidelines |
| CLAUDE.md | Adds repo guidance for Claude Code |
| .github/workflows/node.yml | Updates CI to modern Node versions, coverage, and NPM publish on release |
| .eslintrc | Removes legacy ESLint config |
| Dockerfile | Removes Docker image build definition |
| .env.example | Updates environment variable examples for new server/services |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
@claude[agent] open a new pull request to apply changes based on the relevant comments in this thread |
…ng, add tests Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Address PR review comments: fix concurrent init, improve error handling, add tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tsconfig.json (1)
33-35:⚠️ Potential issue | 🟡 MinorRemove unused path alias configuration.
The
@/*path alias is not used anywhere in the codebase. Remove thepathsconfiguration along withbaseUrlto eliminate unnecessary configuration.
🤖 Fix all issues with AI agents
In @.env.example:
- Line 18: The file is missing a POSIX-style trailing newline; open the
.env.example file and ensure the final line ("ENABLE_FILE_LOGGING=false") is
terminated with a newline character so the file ends with a blank line; simply
add a single trailing newline at EOF and save the file.
In @.github/workflows/node.yml:
- Around line 58-61: The environment URL currently interpolates
github.event.release.tag_name which may include a leading "v" and produce a
broken npm URL; add a preceding workflow step that normalizes the release tag
into a plain version string (remove any leading "v"/"V" from
github.event.release.tag_name using a small shell or actions step to export
RELEASE_VERSION) and then set environment.url to use that normalized value
(replace usage of github.event.release.tag_name with the computed
RELEASE_VERSION) so the npm link becomes .../v/1.0.1 instead of .../v/v1.0.1.
- Around line 45-52: The Codecov upload step using codecov/codecov-action@v4 is
missing the required upload token; add the token parameter under the action
inputs (e.g., add token: ${{ secrets.CODECOV_TOKEN }} to the with: block for the
Upload coverage to Codecov step) so v4 can perform uploads, and ensure the
secret CODECOV_TOKEN is configured in the repository secrets; keep the existing
file, flags, name, and fail_ci_if_error settings intact.
In `@CLAUDE.md`:
- Line 12: Replace the incorrect npm command string `npm test:coverage` with the
correct `npm run test:coverage` in the CLAUDE.md entry for "Test with coverage"
so npm invokes the script via `run` instead of treating it as a built-in
command; update the markdown line that currently reads `**Test with coverage**:
`npm test:coverage` to use `npm run test:coverage` instead.
In `@CONTRIBUTING.md`:
- Around line 9-10: Update CONTRIBUTING.md so the Node requirement line "Node.js
16+" is changed to "Node.js 18+" to match package.json and CI, leave the "npm
8+" line unchanged, and add language specifiers (e.g., bash or text) to the two
fenced code blocks that currently lack a language tag (the blocks near the
examples referenced in the doc) so they render correctly.
In `@package.json`:
- Around line 95-98: The package.json funding entry currently points to the fork
author's sponsor URL; update the "funding" object (the funding field and its
"url" value) to reference the correct org sponsor or funding destination for
FalkorDB (e.g., change the URL from "https://github.com/sponsors/SecKatie" to
the FalkorDB sponsors URL or remove the field if no org sponsorship exists) and
ensure the "type" remains appropriate ("github") if using a GitHub sponsors
link.
- Around line 11-18: Update the package metadata fields in package.json so they
point to the canonical upstream repo instead of the fork: replace the
"repository" object url, the "bugs" url, and the "homepage" string (the
"repository", "bugs", and "homepage" fields) to reference
https://github.com/FalkorDB/FalkorDB-MCPServer (use git+https for the
repository.url and the correct /issues and README anchor for bugs and homepage
respectively) so npm metadata, issue links and docs point to the official
repository.
- Line 87: The dependency declaration for "platformdirs" in package.json is
using a release-candidate version ("platformdirs": "^4.3.8-rc3"); update it to a
stable release by replacing that version with "4.3.7" (or, if you truly need the
RC, pin it exactly without a caret) so production installs use a supported
stable release; ensure you run package manager install (npm/yarn/pnpm) and
update lockfile after changing the "platformdirs" version string.
In `@README.md`:
- Line 56: The README's git clone URL currently points to the fork URL
"https://github.com/SecKatie/falkordb-mcpserver.git"; update that line to the
upstream repository "https://github.com/FalkorDB/FalkorDB-MCPServer.git"
(matching the PR target) so contributors clone the canonical repo, and verify
the README contains the correct repository name/capitalization for
FalkorDB/FalkorDB-MCPServer.
In `@src/index.ts`:
- Around line 14-18: The uncaughtException handler calls the async
errorHandler.handleError(error) without awaiting it, creating unhandled promise
risk and racing with errorHandler.crashIfUntrustedError(error); change this to
not call the async function directly — either remove the handleError call from
the process.on('uncaughtException', ...) handler and rely on synchronous logging
(logger.errorSync) and crashIfUntrustedError, or, if you must invoke it, call
errorHandler.handleError(error).catch(err => logger.errorSync('handleError
rejected', err)) to swallow rejections and avoid unhandledRejection/recursion
while ensuring crashIfUntrustedError still runs synchronously.
In `@src/mcp/prompts.ts`:
- Around line 107-114: The prompt injects user values directly into the Cypher
example (the interpolations of query and relationship_depth in the Example
Cypher patterns), risking injection; update the prompt generation in
src/mcp/prompts.ts to stop splicing raw values into the code block—either remove
the inline Cypher example and ask the LLM to construct the query from textual
instructions, or sanitize/escape user input before interpolation: escape
double-quotes and Cypher-special characters in query and strictly coerce
relationship_depth to an integer with safe bounds (e.g., clamp to a small max)
before use; locate the template where query and relationship_depth are
interpolated and apply one of these mitigations.
- Line 5: The comment "Register graph_list resource" is stale; update or replace
it to accurately describe the code that registers the user_setup prompt (e.g.,
change to "Register user_setup prompt" or remove the comment entirely). Locate
the comment near the code that calls/defines the user_setup registration (look
for the user_setup identifier and the function that registers prompts) and
ensure the comment text matches that function's purpose.
- Around line 63-66: The argsSchema entry relationship_depth currently uses
z.string() but is later interpolated into a Cypher range
"*1..${relationship_depth}", allowing non-numeric or out-of-range values; change
the schema for relationship_depth in argsSchema (in src/mcp/prompts.ts) to
validate/coerce a numeric integer within 1–3 (e.g.,
z.coerce.number().int().min(1).max(3)) or alternatively restrict to
z.enum(["1","2","3"]) so only safe, expected values reach the Cypher
interpolation and prompt generation.
In `@src/mcp/resources.ts`:
- Line 13: The resource currently declares mimeType: "text/plain" while the
content generated in the same resource (see the line that formats graph names as
`- ${name}`) is Markdown; change the mimeType value to "text/markdown" so MCP
clients render the response as Markdown. Locate the mimeType property in the
resource definition in src/mcp/resources.ts and replace the string "text/plain"
with "text/markdown", leaving the rest of the object and the Markdown list
formatting unchanged.
In `@src/services/falkordb.service.ts`:
- Around line 20-32: The initialize() entrypoint currently leaves the
instance-level retryCount unchanged after a failed _initialize(), causing
subsequent initialize() calls to immediately throw; fix by resetting retryCount
to 0 at the start of initialize() (before setting this.initializingPromise or
calling _initialize()) so every top-level initialize() call gets a fresh retry
budget; ensure this change addresses both occurrences related to
initialize()/_initialize() and that close() no longer needs to be called to
restore retries.
- Around line 106-113: The error log currently passes the full query string to
logger.error (logger.error('Query execution failed', appError, { graphName,
query, readOnly })), which can leak sensitive/user-supplied data; change this to
log a truncated/sanitized version of query (reuse the same truncation logic used
for debug logs) before calling logger.error, e.g., compute a safeQuery variable
from query via the existing truncation/sanitization helper and pass safeQuery
instead of query along with graphName, readOnly, and the AppError instance.
In `@src/services/logger.service.ts`:
- Around line 88-97: The MCP notification currently uses level.toLowerCase(),
which turns 'WARN' into 'warn' but the MCP spec expects 'warning'; update the
logic around the notification call (reference: the level variable and the
this.mcpServer.server.notification(...) invocation) to map 'WARN' (and any
'Warn' variants) to 'warning' before sending — e.g., normalize level then apply
a small mapping or switch (create a helper or inline conditional) so all other
levels continue to use toLowerCase() while 'WARN' becomes 'warning'.
In `@src/services/redis.service.ts`:
- Around line 30-71: The _initialize method leaks a connected Redis client when
connect() succeeds but ping() fails because this.client is overwritten on retry;
before creating or reassigning a new client in _initialize (and before recursing
to retry), check if this.client is set and connected and gracefully
close/disconnect it (await this.client.disconnect() or equivalent, catching any
errors), then null out this.client before creating a fresh client via
createClient; also ensure the same cleanup runs in the final failure path
(before throwing AppError) so no connected client is left open.
In `@src/utils/connection-parser.test.ts`:
- Around line 264-275: The test reveals parseFalkorDBConnectionString currently
truncates passwords containing ':' because it does auth.split(':') and only uses
parts[0] and parts[1]; fix parseFalkorDBConnectionString so when parsing the
auth segment you treat the first ':' as the user/password separator and
reconstruct the password by joining any remaining parts (e.g.,
parts.slice(1).join(':')) instead of dropping extras, ensuring username =
parts[0] and password = joined remainder.
- Around line 251-262: The test exposes that parseFalkorDBConnectionString
incorrectly splits on the first '@' causing parts after additional '@' to be
dropped; update the parser to locate the auth/host delimiter with
lastIndexOf('@') instead of naive split so the segment before the last '@' is
treated as userinfo (username:password) and the segment after as host:port;
modify parseFalkorDBConnectionString to extract auth = str.slice(0, lastAt) and
hostPort = str.slice(lastAt + 1), then parse username/password from auth
(handling missing username or password) and parse host/port from hostPort, and
add a follow-up to update this test expectation once the parser is fixed.
🧹 Nitpick comments (24)
src/models/mcp-client-config.test.ts (3)
85-92: "Immutable reference" test is misleading — it only checks referential identity, not immutability.Assigning a module-level
constto two variables and comparing them withtoBewill always pass — it proves nothing about immutability. If the intent is to ensure the exported config isn't accidentally mutated between tests, consider usingObject.freezeon the export and testing that mutations throw, or remove this test.
77-83: Type-compliance tests provide no runtime value.These tests assign a sample to a typed variable and then assert reference equality. The type check is a compile-time concern already handled by TypeScript — the runtime assertion (
toBe) adds nothing. Consider removing these or replacing them with meaningful structural assertions if the intent is to guard against breaking changes.Also applies to: 121-127
105-142: Redundant coverage: exact args snapshot overlaps with individualtoContainchecks.Lines 105-119 duplicate the full args array from the source (brittle if the sample changes), and lines 129-142 re-check the same values with
toContain. Pick one approach — thetoContainstyle (lines 129-142) is more resilient to ordering/addition changes, while thetoEqualstyle (lines 105-119) serves as a snapshot. Having both is redundant..gitignore (1)
139-140: Optional: Consider whether explicit patterns add clarity or create redundancy.The patterns
logs/and*.mcp.logare technically redundant with existing entries:
- Line 2 already contains
logswhich matches both files and directories named "logs"- Line 3 already contains
*.logwhich matches all.logfiles including.mcp.logThat said, explicit project-specific patterns can serve as useful documentation of what artifacts the project generates, and the redundancy is harmless.
src/utils/connection-parser.test.ts (1)
227-249: Mockingglobal.parseIntis brittle — consider a less invasive approach.Overriding
global.parseIntrisks leaking if thefinallyblock doesn't restore it (e.g., a test runner crash). It also won't break if the implementation is refactored to useNumber()or a different parsing path. A more targeted approach would be to pass in a string that actually exercises thecatchblock in the implementation, though admittedly the current try/catch is very broad and hard to trigger naturally. This is acceptable for now but worth noting.tsconfig.json (1)
29-31: Consider using"moduleResolution": "node16"for ES module compatibility.With
"module": "es2022"and"type": "module"in package.json,"moduleResolution": "node"uses the legacy CommonJS resolution algorithm, which doesn't enforce file extensions in imports and doesn't properly resolveexportsfields in package.json. Using"node16"or"nodenext"aligns resolution with Node.js's actual ESM behavior and catches missing.jsextensions at compile time.♻️ Suggested change
- "moduleResolution": "node", /* Specify how TypeScript looks up a file from a given module specifier. */ + "moduleResolution": "node16", /* Specify how TypeScript looks up a file from a given module specifier. */CONTRIBUTING.md (1)
71-78: Add language specifiers to fenced code blocks.Per markdownlint MD040, the code block at Line 71 (commit message examples) and Line 183 (project structure) should have language specifiers.
📝 Suggested fix
-``` -type(scope): description +```text +type(scope): description-``` -src/ +```text +src/Also applies to: 183-198
CLAUDE.md (1)
44-58: Add language specifier to the project structure code block.Per markdownlint MD040, the fenced code block at Line 45 should have a language specified (e.g.,
text).Also, the project structure here doesn't mention
logger.service.tsunderservices/, though CONTRIBUTING.md and README.md both list it. Consider keeping these in sync.README.md (1)
116-121: Add language specifiers to fenced code blocks.Several code blocks showing natural language prompts and the project structure tree lack language specifiers (markdownlint MD040). Use
textfor these blocks.Also applies to: 131-135, 138-142, 188-209
eslint.config.js (1)
1-27: Update ESLint flat config to follow@typescript-eslintv8 documented pattern.The current configuration works but uses a legacy spreading pattern. With
@typescript-eslintv8.38.0 and ESLint 9.31.0, the recommended approach is to importtypescript-eslintdirectly and use the flat config arrays:Suggested refactor
import eslint from '@eslint/js'; import tseslint from 'typescript-eslint'; export default [ eslint.configs.recommended, ...tseslint.configs.recommended, { ignores: ['node_modules/**', 'dist/**', 'coverage/**'], }, ];If type-aware rules are needed, use
tseslint.configs.recommendedTypeCheckedand addparserOptions.projectService: true.package.json (1)
68-69:prepublishandprepublishOnlyoverlap.
prepublishruns on bothnpm publishandnpm install(in some npm versions), andprepublishOnlyruns only onnpm publish. SinceprepublishOnlyalready doesclean && test:ci && lint && build, the separateprepublishscript is redundant and may cause double execution of tests/lint/build duringnpm publish. Consider removing theprepublishscript.Proposed fix
- "prepublish": "npm run test:ci && npm run lint && npm run build", "prepublishOnly": "npm run clean && npm run test:ci && npm run lint && npm run build"src/mcp/resources.ts (1)
7-31: Inconsistent indentation throughout the resource registration block.The arguments to
server.registerResourcemix 1-space, 3-space, and 4-space indentation (e.g., lines 8–14 vs. 15–29). Consider normalizing to a consistent style..env.example (1)
5-15: Consider adding a comment clarifying the relationship between FalkorDB and Redis configs.
FALKORDB_HOST/FALKORDB_PORTdefault tolocalhost:6379, andREDIS_URLalso defaults toredis://localhost:6379. Since FalkorDB is backed by Redis, newcomers may wonder if these are the same instance or need separate servers. A brief comment would help.src/services/logger.service.test.ts (2)
125-138: The failing MCP server mock set viasetMcpServeris never cleaned up.Line 133 sets a failing MCP server on the singleton
logger, but there's no corresponding teardown to reset it. Sinceloggeris a module-level singleton, this state could leak into other test files if Jest happens to share the module instance (depending on--isolateModulesconfig). Consider resetting the MCP server inafterEachor at the end of this test.Proposed fix
// Logger should not throw even if MCP notifications fail await expect(logger.info('Test with failing MCP')).resolves.not.toThrow(); + + // Reset MCP server to avoid leaking state + logger.setMcpServer(null as any); });
57-103: Tests only verify no-throw behavior, not actual logging output.The async and sync tests are useful as smoke tests, but they don't assert that messages are actually written (to console, file, or MCP). If the logger silently swallows all input due to a bug, these tests would still pass. Consider adding at least one assertion that verifies the log output reaches the expected sink (e.g.,
console.errorwas called for error-level logs, orfs.appendFileSyncwas called when file logging is enabled).src/mcp/tools.ts (2)
9-57: Consider exposing query parameters in thequery_graphtool input schema.The underlying
executeQuerymethod supports aparamsargument, but the tool always passesundefined(line 42). For parameterized OpenCypher queries, this means users must inline values into the query string rather than using parameter binding, which is less safe and less convenient.💡 Proposed schema addition
inputSchema: { graphName: z.string().describe("The name of the graph to query"), query: z.string().describe("The OpenCypher query to run"), + params: z.record(z.unknown()).optional().describe("Optional query parameters for parameterized queries"), readOnly: z.boolean().optional().describe("If true, executes as a read-only query (GRAPH.RO_QUERY). Useful for replica instances or to prevent accidental writes. Defaults to FALKORDB_DEFAULT_READONLY environment variable."), }, }, - async ({graphName, query, readOnly}) => { + async ({graphName, query, params, readOnly}) => {Then pass
paramsinstead ofundefinedon line 42:- const result = await falkorDBService.executeQuery(graphName, query, undefined, isReadOnly); + const result = await falkorDBService.executeQuery(graphName, query, params, isReadOnly);
220-226: Thevaluenull/undefined check is unreachable when zod validates input.Since
valueis declared asz.string()in the input schema (line 207), the MCP SDK will validate the input before the handler runs. A string value will never beundefinedornullhere. If the intention is defense-in-depth, the check is harmless but misleading.src/services/redis.service.ts (1)
32-35: Logger calls are not awaited throughout the service.All
logger.*()calls in this file are fire-and-forget (e.g., line 32logger.info(...)withoutawait). Since logger methods are async, unhandled rejections could occur if the logger fails. This is inconsistent withsrc/mcp/tools.tswhere all logger calls are awaited.♻️ Example fix for a few locations
- logger.info('Attempting to connect to Redis', { + await logger.info('Attempting to connect to Redis', { url: config.redis.url, attempt: this.retryCount + 1 });- logger.info('Successfully connected to Redis'); + await logger.info('Successfully connected to Redis');Apply similarly to all other
logger.*()calls in the file.Also applies to: 46-47, 52-56
src/index.ts (1)
104-105: Unhandled promise fromstartServer()call.The returned promise from
startServer()is not handled. WhilestartServerhas its own try/catch that callsgracefulShutdown, ifgracefulShutdownitself throws (unlikely but possible), the rejection propagates to theunhandledRejectionhandler, which re-throws, then hitsuncaughtException. This chain works but is fragile.💡 Add a catch handler for clarity
-startServer(); +startServer().catch((error) => { + logger.errorSync('Fatal startup error', error instanceof Error ? error : new Error(String(error))); + process.exit(1); +});src/errors/ErrorHandler.test.ts (1)
226-232: Singleton test is a tautology—expect(errorHandler).toBe(errorHandler)is always true.Line 230 compares a reference to itself, which doesn't actually verify singleton behavior. To meaningfully test the singleton, import
errorHandlerfrom two different calls or compare the exported instance against a newErrorHandler()to confirm they differ.💡 A more meaningful singleton check
describe('singleton instance', () => { it('should export a singleton instance', () => { // Assert expect(errorHandler).toBeInstanceOf(ErrorHandler); - expect(errorHandler).toBe(errorHandler); // Same instance + // Verify it's a pre-constructed singleton, not a new instance each access + const anotherNew = new ErrorHandler(); + expect(errorHandler).not.toBe(anotherNew); }); });src/services/logger.service.ts (2)
70-78: Synchronous file I/O on every log call — no rotation or size cap.
appendFileSyncblocks the event loop on each write. For a low-throughput stdio server this is tolerable, but the log file can grow without bound. Consider adding a simple size check or rotation strategy (e.g., rotate when the file exceeds a threshold) to prevent disk exhaustion in long-running instances.
121-128: Conditional spread with&&works but is non-obvious — consider explicit conditional.
...(error as any).isOperational !== undefined && { isOperational: ... }relies on...falsebeing a no-op. It works, but a more readable pattern would be an explicit conditional or helper.Also, this same logic is duplicated in
errorSync(lines 153-160). Consider extracting a small private helper to build the error context.src/services/falkordb.service.ts (2)
67-68: Fixed 5-second retry delay — consider exponential backoff.All 5 retry attempts use a static 5 000 ms delay. Exponential backoff (e.g.,
Math.min(1000 * 2 ** this.retryCount, 30000)) would be more resilient to transient outages and avoid thundering-herd issues if multiple instances retry simultaneously.
82-89: Optionalparamsbefore required-with-defaultreadOnly— API ergonomics concern.The signature
executeQuery(graphName, query, params?, readOnly = false)means callers who want read-only without params must passundefinedexplicitly:executeQuery('g', 'q', undefined, true). The dedicatedexecuteReadOnlyQueryhelper mitigates this, but consider using an options object for the trailing parameters if this API is expected to grow.
| if: github.event_name == 'release' && github.event.action == 'published' | ||
| environment: | ||
| name: Default | ||
| url: https://www.npmjs.com/package/falkordb-mcpserver/v/${{ github.event.release.tag_name }} |
There was a problem hiding this comment.
Environment URL may be incorrect if release tags use a v prefix.
github.event.release.tag_name typically includes a v prefix (e.g., v1.0.1), but npm package URLs expect the bare version (1.0.1). This would produce a broken link like .../v/v1.0.1.
Consider stripping the prefix:
url: https://www.npmjs.com/package/falkordb-mcpserver/v/${{ github.event.release.tag_name }}→
url: https://www.npmjs.com/package/falkordb-mcpserver/v/${{ github.event.release.tag_name }}Unfortunately, GitHub Actions expressions don't have a built-in replace, so you may need to accept a potentially imperfect URL or handle tag naming conventions (e.g., always tag without the v prefix, or use a step to compute the version).
🤖 Prompt for AI Agents
In @.github/workflows/node.yml around lines 58 - 61, The environment URL
currently interpolates github.event.release.tag_name which may include a leading
"v" and produce a broken npm URL; add a preceding workflow step that normalizes
the release tag into a plain version string (remove any leading "v"/"V" from
github.event.release.tag_name using a small shell or actions step to export
RELEASE_VERSION) and then set environment.url to use that normalized value
(replace usage of github.event.release.tag_name with the computed
RELEASE_VERSION) so the npm link becomes .../v/1.0.1 instead of .../v/v1.0.1.
| it('should handle edge case with multiple : in auth', () => { | ||
| // Act | ||
| const result = parseFalkorDBConnectionString('falkordb://user:pass:extra@host:1234'); | ||
|
|
||
| // Assert - implementation takes first : as separator between user and pass | ||
| expect(result).toEqual({ | ||
| host: 'host', | ||
| port: 1234, | ||
| username: 'user', | ||
| password: 'pass' | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test documents password truncation when it contains :.
auth.split(':') on user:pass:extra yields ['user', 'pass', 'extra'], and only parts[0] and parts[1] are used — extra is silently dropped. Passwords containing : are valid, so the parser should rejoin from index 1 onward (e.g., parts.slice(1).join(':')). Same follow-up as the @ issue above.
🤖 Prompt for AI Agents
In `@src/utils/connection-parser.test.ts` around lines 264 - 275, The test reveals
parseFalkorDBConnectionString currently truncates passwords containing ':'
because it does auth.split(':') and only uses parts[0] and parts[1]; fix
parseFalkorDBConnectionString so when parsing the auth segment you treat the
first ':' as the user/password separator and reconstruct the password by joining
any remaining parts (e.g., parts.slice(1).join(':')) instead of dropping extras,
ensuring username = parts[0] and password = joined remainder.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@claude[agent] In In In In In |
…ping, clean up Redis client on retry, fix connection parser edge cases Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Address PR review comments: sanitize query logging, fix MCP level mapping, clean up Redis client on retry, fix connection parser edge cases
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/node.yml:
- Line 21: The CI matrix includes an unsupported Node.js version: update the
node-version array (the node-version key in the workflow) to remove "18.x" and
test only against actively supported releases—replace node-version: [18.x, 20.x,
22.x] with node-version: [22.x, 24.x] (or [22.x, 24.x] as suggested) so the
workflow runs on current LTS/maintenance versions.
In `@src/services/falkordb.service.ts`:
- Around line 57-79: When ping() fails after a successful FalkorDB.connect()
(this.client is set), ensure you close/cleanup the existing client before
retrying: inside the catch of _initialize(), before incrementing this.retryCount
and calling _initialize() again, check if this.client exists and call its
close/disconnect method (the same cleanup used in redis.service.ts) and null out
this.client to avoid leaking connections; then proceed with the existing retry
logging (logger.warn), backoff, and recursive call, keeping the AppError/logging
path unchanged when maxRetries is exceeded.
In `@src/services/redis.service.ts`:
- Around line 16-28: The bug is that retryCount is never reset so once it
reaches maxRetries future calls immediately fail; to fix, reset retryCount to 0
at the start of the initialization flow (e.g., set this.retryCount = 0 either at
the top of initialize() before calling this._initialize() or at the start of
_initialize()), then preserve existing retry loop logic that increments
retryCount and compares against maxRetries; update the RedisService methods
initialize() and/or _initialize() where retryCount and maxRetries are referenced
to ensure retryCount is reinitialized on each new initialization attempt.
🧹 Nitpick comments (6)
src/utils/connection-parser.ts (1)
52-58: IPv6 host addresses would break the host:port split.An IPv6 literal like
[::1]:6379contains multiple colons, sohostPort.split(':')would mis-parse. This is a pre-existing limitation and likely out of scope for this PR, but worth noting for future work.src/utils/connection-parser.test.ts (1)
227-249: Mockingglobal.parseIntis fragile — any internal use ofparseIntbefore the target line will also throw.This works today because
parseInthappens to be the first throwable call in the code path after the early returns, but it's a brittle coupling. Consider testing the error/catch path by passing an input that naturally triggers an exception, or by using a more targeted mock. Low priority since it does exercise the catch branch.src/services/logger.service.ts (2)
123-135: Consider extracting the duplicated error-context construction.The
error()anderrorSync()methods share identical logic for buildingerrorContextfrom anErrorobject (lines 124–132 and 156–164). A small private helper would reduce duplication.♻️ Suggested refactor
+ private buildErrorContext(error?: Error, context?: LogContext): LogContext | undefined { + if (!error) return context; + return { + name: error.name, + message: error.message, + stack: error.stack, + ...(error as any).isOperational !== undefined && { + isOperational: (error as any).isOperational + }, + ...context + }; + } + async error(message: string, error?: Error, context?: LogContext): Promise<void> { - const errorContext = error ? { - name: error.name, - message: error.message, - stack: error.stack, - ...(error as any).isOperational !== undefined && { - isOperational: (error as any).isOperational - }, - ...context - } : context; - - await this.log('ERROR', message, errorContext); + await this.log('ERROR', message, this.buildErrorContext(error, context)); }Apply the same to
errorSync.Also applies to: 155-168
64-78: Synchronous file I/O on every log call could become a bottleneck.
appendFileSyncblocks the event loop. For an MCP stdio server with moderate log volume this is likely acceptable, but worth noting if throughput becomes a concern. An async write buffer orappendFile(async) would be the improvement path.src/services/redis.service.ts (1)
155-192:listKeysusing SCAN is a good pattern, but be aware of production risks.Using
SCANwithCOUNT: 1000is preferable toKEYS *, but on a Redis instance with millions of keys, the unbounded accumulation intoallKeyscould cause high memory usage. For the current use case this is likely fine, but worth noting if the key space grows significantly..github/workflows/node.yml (1)
78-84: Consider adding--provenancetonpm publishfor supply chain transparency.npm provenance links published packages to their source commit and build, improving supply chain security. This is a low-effort addition:
- name: Publish to NPM - run: npm publish + run: npm publish --provenance --access public env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}This requires
id-token: writepermission on the job:publish: name: Publish to NPM needs: [test] runs-on: ubuntu-latest + permissions: + contents: read + id-token: write
The MCP SDK's registerTool method has deeply nested generic type parameters (OutputArgs and InputArgs) that exceed TypeScript's type instantiation recursion limit. Add @ts-expect-error directive to suppress this known SDK issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 35 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { redisService } from './redis.service'; | ||
| import { AppError, CommonErrors } from '../errors/AppError.js'; | ||
|
|
||
| // Mock the logger service | ||
| jest.mock('./logger.service.js', () => ({ | ||
| logger: { | ||
| info: jest.fn().mockResolvedValue(undefined), | ||
| warn: jest.fn().mockResolvedValue(undefined), | ||
| error: jest.fn().mockResolvedValue(undefined), | ||
| debug: jest.fn().mockResolvedValue(undefined), | ||
| } | ||
| })); | ||
|
|
||
| // Mock the config | ||
| jest.mock('../config/index.js', () => ({ | ||
| config: { |
There was a problem hiding this comment.
This test file uses ESM Jest (ts-jest/presets/default-esm), but it imports redisService before declaring jest.mock(...) for redis, config, and logger. In ESM, static imports are evaluated before the module body runs, so these mocks won’t affect the already-imported service (and createClient will remain the real implementation). Convert these mocks to jest.unstable_mockModule(...) + dynamic await import(...), or move to dependency injection so the service can be tested without module-level mocking.
| import { ErrorHandler, errorHandler } from './ErrorHandler'; | ||
| import { AppError, CommonErrors } from './AppError'; | ||
|
|
||
| // Mock the logger service | ||
| jest.mock('../services/logger.service.js', () => ({ | ||
| logger: { | ||
| error: jest.fn().mockResolvedValue(undefined), | ||
| info: jest.fn().mockResolvedValue(undefined), | ||
| errorSync: jest.fn(), | ||
| } | ||
| })); | ||
|
|
||
| // Get mock logger from the mocked module | ||
| let mockLogger: any; | ||
|
|
||
| describe('ErrorHandler', () => { | ||
| let handler: ErrorHandler; | ||
|
|
||
| beforeEach(async () => { | ||
| handler = new ErrorHandler(); | ||
| jest.clearAllMocks(); | ||
| // Import the mock logger | ||
| const loggerModule = await import('../services/logger.service.js'); | ||
| mockLogger = loggerModule.logger; | ||
| }); | ||
|
|
There was a problem hiding this comment.
ErrorHandler is imported before mocking ../services/logger.service.js. With Jest ESM, the logger import inside ErrorHandler will already be resolved to the real module, so assertions against mockLogger can be unreliable and the test may perform real logging. Use jest.unstable_mockModule('../services/logger.service.js', ...) and await import('./ErrorHandler') after setting up mocks.
| import { ErrorHandler, errorHandler } from './ErrorHandler'; | |
| import { AppError, CommonErrors } from './AppError'; | |
| // Mock the logger service | |
| jest.mock('../services/logger.service.js', () => ({ | |
| logger: { | |
| error: jest.fn().mockResolvedValue(undefined), | |
| info: jest.fn().mockResolvedValue(undefined), | |
| errorSync: jest.fn(), | |
| } | |
| })); | |
| // Get mock logger from the mocked module | |
| let mockLogger: any; | |
| describe('ErrorHandler', () => { | |
| let handler: ErrorHandler; | |
| beforeEach(async () => { | |
| handler = new ErrorHandler(); | |
| jest.clearAllMocks(); | |
| // Import the mock logger | |
| const loggerModule = await import('../services/logger.service.js'); | |
| mockLogger = loggerModule.logger; | |
| }); | |
| import type { ErrorHandler as ErrorHandlerType } from './ErrorHandler'; | |
| import { AppError, CommonErrors } from './AppError'; | |
| // References to the ErrorHandler exports, populated after mocking logger.service | |
| let ErrorHandler: { new (): ErrorHandlerType }; | |
| let errorHandler: ErrorHandlerType; | |
| // Get mock logger from the mocked module | |
| let mockLogger: any; | |
| describe('ErrorHandler', () => { | |
| let handler: ErrorHandlerType; | |
| beforeAll(async () => { | |
| // Mock the logger service before importing ErrorHandler | |
| await jest.unstable_mockModule('../services/logger.service.js', () => ({ | |
| logger: { | |
| error: jest.fn().mockResolvedValue(undefined), | |
| info: jest.fn().mockResolvedValue(undefined), | |
| errorSync: jest.fn(), | |
| }, | |
| })); | |
| // Import the mock logger and the ErrorHandler module after setting up the mock | |
| const loggerModule = await import('../services/logger.service.js'); | |
| mockLogger = loggerModule.logger; | |
| const errorHandlerModule = await import('./ErrorHandler'); | |
| ErrorHandler = errorHandlerModule.ErrorHandler; | |
| errorHandler = errorHandlerModule.errorHandler; | |
| }); | |
| beforeEach(() => { | |
| handler = new ErrorHandler(); | |
| jest.clearAllMocks(); | |
| }); |
| it('should return default options for undefined input', () => { | ||
| // Act | ||
| const result = parseFalkorDBConnectionString(undefined as any); | ||
|
|
||
| // Assert | ||
| expect(result).toEqual({ | ||
| host: 'localhost', | ||
| port: 6379 | ||
| }); | ||
| }); | ||
|
|
||
| it('should return default options for null input', () => { | ||
| // Act | ||
| const result = parseFalkorDBConnectionString(null as any); | ||
|
|
There was a problem hiding this comment.
The implementation explicitly handles undefined/null by returning defaults, but the public signature is parseFalkorDBConnectionString(connectionString: string). The tests currently work around this by casting to any. Consider widening the function signature to string | undefined | null so callers don’t need unsafe casts and the behavior is encoded in the type system.
| import { falkorDBService } from './falkordb.service'; | ||
| import { AppError, CommonErrors } from '../errors/AppError.js'; | ||
|
|
||
| // Mock the logger service | ||
| jest.mock('./logger.service.js', () => ({ | ||
| logger: { | ||
| info: jest.fn().mockResolvedValue(undefined), | ||
| warn: jest.fn().mockResolvedValue(undefined), | ||
| error: jest.fn().mockResolvedValue(undefined), | ||
| debug: jest.fn().mockResolvedValue(undefined), | ||
| } | ||
| })); | ||
|
|
||
| // Mock the config | ||
| jest.mock('../config/index.js', () => ({ | ||
| config: { |
There was a problem hiding this comment.
Same ESM mocking issue as above: falkorDBService is imported before the jest.mock('falkordb'|config|logger) declarations. Under Jest ESM, these mocks won’t apply to the already-evaluated imports, so FalkorDB.connect will be the real implementation during tests. Use jest.unstable_mockModule(...) with dynamic import (or dependency injection) so the mocks are effective.
| describe('sampleMCPClientConfig', () => { | ||
| it('should have correct structure and values', () => { | ||
| // Assert | ||
| expect(sampleMCPClientConfig).toBeDefined(); | ||
| expect(sampleMCPClientConfig.defaultServer).toBe('falkordb'); | ||
| expect(sampleMCPClientConfig.servers).toBeDefined(); | ||
| expect(sampleMCPClientConfig.servers.falkordb).toBeDefined(); | ||
| expect(sampleMCPClientConfig.servers.falkordb.url).toBe('http://localhost:3000/api/mcp'); | ||
| expect(sampleMCPClientConfig.servers.falkordb.apiKey).toBe('your_api_key_here'); | ||
| }); |
There was a problem hiding this comment.
The sampleMCPClientConfig assertions expect an HTTP endpoint (http://localhost:3000/api/mcp) and an apiKey, but the server entrypoint in this PR is an MCP stdio server (no Express/HTTP routes anymore). These sample configs (and therefore this test) appear out of date with the new architecture; consider updating the model/sample to the stdio-based mcpServers { command, args, env } format (as shown in the README) or removing the legacy HTTP client config entirely.
| describe('sampleMCPServerConfig', () => { | ||
| it('should have correct structure and values', () => { | ||
| // Assert | ||
| expect(sampleMCPServerConfig).toBeDefined(); | ||
| expect(sampleMCPServerConfig.mcpServers).toBeDefined(); | ||
| expect(sampleMCPServerConfig.mcpServers.falkordb).toBeDefined(); | ||
| expect(sampleMCPServerConfig.mcpServers.falkordb.command).toBe('docker'); | ||
| expect(sampleMCPServerConfig.mcpServers.falkordb.args).toBeInstanceOf(Array); | ||
| }); | ||
|
|
||
| it('should have correct docker arguments', () => { | ||
| // Arrange | ||
| const expectedArgs = [ | ||
| 'run', | ||
| '-i', | ||
| '--rm', | ||
| '-p', '3000:3000', | ||
| '--env-file', '.env', | ||
| 'falkordb-mcpserver', | ||
| 'falkordb://host.docker.internal:6379' | ||
| ]; | ||
|
|
||
| // Assert | ||
| expect(sampleMCPServerConfig.mcpServers.falkordb.args).toEqual(expectedArgs); | ||
| }); |
There was a problem hiding this comment.
This test validates a Docker-based sampleMCPServerConfig, but the PR removes the Dockerfile and states Docker-based build/publish flow is removed. Keeping a Docker sample config here will mislead users/tests into thinking Docker is still supported. Update the sample to a node/npx stdio-based config or reintroduce documented Docker support.
| async initialize(): Promise<void> { | ||
| if (this.initializingPromise) { | ||
| return this.initializingPromise; | ||
| } | ||
|
|
||
| this.initializingPromise = this._initialize(); | ||
|
|
There was a problem hiding this comment.
RedisService.initialize() does not reset retryCount before starting a new initialization sequence. If a previous initialization attempt exhausts retries and throws, subsequent calls to initialize() will start with retryCount already at maxRetries and will fail immediately without retrying. Reset retryCount (and any other state) at the start of initialize() similar to FalkorDBService.initialize().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/falkordb.service.ts (1)
77-78: Flat 5 s retry delay with no backoff or jitter; recursion could be replaced with a loop.The hardcoded 5 000 ms constant gives 25 s of total wait time across 5 retries and makes all retrying clients hit the server simultaneously after a shared outage. Exponential backoff with jitter (e.g.
min(5000 * 2^retryCount, 30000) + Math.random() * 1000) would improve resilience. Separately, rewriting the retry as awhile/forloop avoids five nested async stack frames, is easier to follow, and sidesteps the (benign here) tail-call non-optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/falkordb.service.ts` around lines 77 - 78, Replace the recursive retry in _initialize with an iterative loop that caps retries (e.g., 5 attempts) and uses exponential backoff with jitter instead of a flat 5000ms sleep; compute delay as Math.min(5000 * 2 ** retryCount, 30000) + Math.random() * 1000 between attempts, await the delay, and retry within the loop rather than calling return this._initialize() to avoid nested async stack frames and synchronized client retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/falkordb.service.ts`:
- Line 37: Several logger.* calls in this module are invoked fire-and-forget
even though logger methods return Promise<void>, which can lose log delivery
when an error is thrown (e.g., logger.error(...) is called then throw appError).
Update the failure paths to await the logger.error promise before throwing or
returning (ensure the enclosing function is async or properly awaits), and for
other critical log sites (tests or shutdown paths) either await logger.* or add
a short flush/await helper; if you intentionally want fire-and-forget, add a
clear comment at each logger call (e.g., above logger.info/warn/debug)
explaining that behavior. Reference the module-level logger instance (logger)
and the error path that throws appError to locate and fix the calls.
- Around line 20-33: Add an idempotency guard to initialize() so it does not
overwrite an already-connected client: before resetting retryCount or assigning
initializingPromise, check if this.client exists and is connected (or otherwise
healthy) and simply return; keep the existing concurrent-call guard
(initializingPromise) intact. This prevents _initialize() from calling
FalkorDB.connect() and replacing a live this.client; do not reset
this.retryCount when skipping work. Ensure the logic references initialize(),
_initialize(), this.client, initializingPromise and retryCount so the
early-return condition is applied in initialize() prior to starting a new
initialization attempt.
---
Nitpick comments:
In `@src/services/falkordb.service.ts`:
- Around line 77-78: Replace the recursive retry in _initialize with an
iterative loop that caps retries (e.g., 5 attempts) and uses exponential backoff
with jitter instead of a flat 5000ms sleep; compute delay as Math.min(5000 * 2
** retryCount, 30000) + Math.random() * 1000 between attempts, await the delay,
and retry within the loop rather than calling return this._initialize() to avoid
nested async stack frames and synchronized client retries.
- Prevent Cypher injection in prompt templates by using parameterized query examples ($query) instead of string interpolation - Validate relationship_depth as a bounded number (1-3) instead of string - Strip credentials from Redis URL before logging to prevent secret exposure - Reset retryCount in RedisService.initialize() for consistency with FalkorDBService - Read version from package.json instead of hardcoding in MCP server config - Add confirmDelete safety parameter to delete_graph and delete_key tools Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add idempotency guard to initialize() to prevent overwriting a connected client and leaking connections - Replace recursive retry in _initialize() with iterative loop using exponential backoff with jitter (5s * 2^attempt, capped at 30s) - Await logger.error() before throw on error paths to ensure log delivery is not lost - Add fire-and-forget comments to non-critical logger calls (info, debug, warn) documenting intentional behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Handle async errorHandler.handleError() rejection in uncaughtException handler with void + catch to prevent secondary unhandled rejection - Truncate query strings in tools.ts error logs to first 100 chars, matching the pattern already used in FalkorDBService - Update README Node.js prerequisite from 16+ to 18+ to match package.json engines.node >=18.0.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (1)
116-121: Add language specifiers to fenced code blocks.Several fenced code blocks at lines 116, 131, 138, and 188 lack a language specifier, which markdownlint flags as
MD040. Since these blocks contain natural-language example prompts and a directory tree respectively, consider using```textto silence the lint warning and improve rendering.Also applies to: 131-135, 138-142, 188-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 116 - 121, The README has multiple fenced code blocks containing natural-language prompts and a directory tree (e.g. the blocks with "Show me all people who know each other", "Find the shortest path between two nodes", "What relationships does John have?", "Run a read-only query on the replica instance" and the directory tree block) that lack a language specifier and trigger MD040; update each offending triple-backtick fence to use ```text (or another appropriate non-code language) so the blocks are annotated (e.g., change ``` to ```text for the prompt blocks and the directory tree) to silence the lint warning and improve rendering.src/index.ts (1)
97-114: Top-levelstartServer()promise is unhandled.
startServer()on line 114 is not awaited or.catch()-ed. While the internaltry/catchcallsgracefulShutdown(which exits the process), a theoretical rejection fromgracefulShutdownitself could surface as an unhandled rejection. Adding a.catchmakes this explicit and silences Node's--unhandled-rejections=strictmode.Proposed fix
-startServer(); +startServer().catch(() => process.exit(1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 97 - 114, The call to startServer() is left unhandled and could surface an unhandled rejection (e.g., if gracefulShutdown rejects); update the invocation of startServer to handle promise rejections—either wrap in a top-level async IIFE and await it with try/catch or append .catch(...) to startServer() that logs the error via logger.error and invokes gracefulShutdown or process.exit; reference the startServer function and gracefulShutdown in your fix so any rejection is explicitly caught and logged.src/mcp/tools.ts (1)
22-38: Consider extracting repeated validation into a shared helper.The empty-string check pattern (
if (!value?.trim()) { throw new AppError(...) }) is repeated across 6+ tool handlers forgraphName,query, andkey. A small helper likeassertNonEmpty(value, fieldName)would reduce duplication and ensure consistent error messages.Also applies to: 71-87, 148-154, 214-220, 260-266, 298-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/tools.ts` around lines 22 - 38, Extract the repeated empty-string validation into a helper named assertNonEmpty(value, fieldName) that throws new AppError(CommonErrors.INVALID_INPUT, `${fieldName} is required and cannot be empty`, true); add this helper to src/mcp/tools.ts (module scope or exported) and replace the repeated checks for graphName, query, and key inside the tool handlers (the async handlers that currently do if (!graphName?.trim()) / if (!query?.trim()) / if (!key?.trim())) with calls to assertNonEmpty(graphName, 'Graph name'), assertNonEmpty(query, 'Query'), and assertNonEmpty(key, 'Key') respectively to remove duplication and ensure consistent error messages.src/services/redis.service.ts (1)
44-94: Inconsistent retry strategy compared tofalkordb.service.ts.
falkordb.service.tsuses an iterativeforloop with exponential backoff + jitter (Math.min(5000 * 2 ** retryCount, 30000) + Math.random() * 1000), while this file uses recursion with a flat 5-second delay. Consider aligning the two services for consistent retry behavior and to avoid unbounded delays diverging between services.♻️ Sketch: iterative retry with exponential backoff
private async _initialize(): Promise<void> { - try { - logger.info('Attempting to connect to Redis', { - url: this.sanitizeUrl(config.redis.url), - attempt: this.retryCount + 1 - }); - - this.client = createClient({ - url: config.redis.url, - username: config.redis.username, - password: config.redis.password, - }); - - await this.client.connect(); - await this.client.ping(); - - logger.info('Successfully connected to Redis'); - this.retryCount = 0; - } catch (error) { - // Clean up failed client before retrying or throwing - if (this.client) { - try { - await this.client.disconnect(); - } catch { - // Ignore disconnect errors + for (;; this.retryCount++) { + try { + logger.info('Attempting to connect to Redis', { + url: this.sanitizeUrl(config.redis.url), + attempt: this.retryCount + 1 + }); + + this.client = createClient({ + url: config.redis.url, + username: config.redis.username, + password: config.redis.password, + }); + + await this.client.connect(); + await this.client.ping(); + + logger.info('Successfully connected to Redis'); + this.retryCount = 0; + return; + } catch (error) { + if (this.client) { + try { + await this.client.disconnect(); + } catch { + // Ignore disconnect errors + } + this.client = null; } - this.client = null; - } - if (this.retryCount < this.maxRetries) { - this.retryCount++; - logger.warn('Failed to connect to Redis, retrying...', { - attempt: this.retryCount, - maxRetries: this.maxRetries, - error: error instanceof Error ? error.message : String(error) - }); - - await new Promise(resolve => setTimeout(resolve, 5000)); - return this._initialize(); - } else { - const appError = new AppError( - CommonErrors.CONNECTION_FAILED, - `Failed to connect to Redis after ${this.maxRetries} attempts: ${error instanceof Error ? error.message : String(error)}`, - true - ); - - logger.error('Redis connection failed permanently', appError); - throw appError; + if (this.retryCount < this.maxRetries) { + logger.warn('Failed to connect to Redis, retrying...', { + attempt: this.retryCount + 1, + maxRetries: this.maxRetries, + error: error instanceof Error ? error.message : String(error) + }); + + const delay = Math.min(5000 * 2 ** this.retryCount, 30000) + Math.random() * 1000; + await new Promise(resolve => setTimeout(resolve, delay)); + } else { + const appError = new AppError( + CommonErrors.CONNECTION_FAILED, + `Failed to connect to Redis after ${this.maxRetries} attempts: ${error instanceof Error ? error.message : String(error)}`, + true + ); + + await logger.error('Redis connection failed permanently', appError); + throw appError; + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/redis.service.ts` around lines 44 - 94, The _initialize method currently uses recursion and a flat 5s delay; change it to an iterative retry loop (use for or while) that attempts connection up to maxRetries, uses the same exponential backoff + jitter as falkordb.service.ts (delay = Math.min(5000 * 2 ** retryCount, 30000) + Math.random() * 1000), and preserves existing behavior: log attempts with sanitizeUrl, construct createClient and await connect/ping, cleanly disconnect and nullify client on failure, increment retryCount per attempt, reset retryCount on success, and throw the AppError after exhausting retries; update references to this._initialize recursion to instead loop inside _initialize and ensure logging and error payloads remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/redis.service.ts`:
- Around line 62-93: The catch block in _initialize handles permanent Redis
failure but calls logger.error(...) without awaiting it, risking lost async logs
before the subsequent throw; change the call to await logger.error('Redis
connection failed permanently', appError) so the log completes before throwing
the AppError (keep the rest of the cleanup/retry logic intact and ensure this
await remains inside the same async catch scope that references this.client,
this.retryCount, this.maxRetries, AppError and CommonErrors.CONNECTION_FAILED).
- Around line 29-42: The initialize() method can overwrite a live Redis client
when called a second time, leaking the existing connection; add an idempotency
guard that returns immediately if this.client is already set (e.g., at the start
of initialize()), and keep the existing initializingPromise logic to handle
concurrent inits; ensure _initialize() does not replace this.client if a client
was created in the meantime (re-check this.client before assigning) so the
existing client is never closed/overwritten.
---
Duplicate comments:
In `@README.md`:
- Line 56: The README's git clone instruction still points to the fork URL ("git
clone https://github.com/SecKatie/falkordb-mcpserver.git"); update that clone
command to the upstream repository URL (replace the SecKatie URL with the
upstream repo URL) so the README shows the canonical clone source; locate the
git clone line in README.md and swap the URL accordingly.
In `@src/mcp/prompts.ts`:
- Around line 4-5: Update the stale inline comment inside function
registerUserSetupPrompt to accurately describe what the function does: replace
the incorrect "// Register graph_list resource" comment with a brief, correct
comment such as "// Register user_setup prompt" (or similar) so it reflects that
registerUserSetupPrompt registers the user_setup prompt for the McpServer.
---
Nitpick comments:
In `@README.md`:
- Around line 116-121: The README has multiple fenced code blocks containing
natural-language prompts and a directory tree (e.g. the blocks with "Show me all
people who know each other", "Find the shortest path between two nodes", "What
relationships does John have?", "Run a read-only query on the replica instance"
and the directory tree block) that lack a language specifier and trigger MD040;
update each offending triple-backtick fence to use ```text (or another
appropriate non-code language) so the blocks are annotated (e.g., change ``` to
```text for the prompt blocks and the directory tree) to silence the lint
warning and improve rendering.
In `@src/index.ts`:
- Around line 97-114: The call to startServer() is left unhandled and could
surface an unhandled rejection (e.g., if gracefulShutdown rejects); update the
invocation of startServer to handle promise rejections—either wrap in a
top-level async IIFE and await it with try/catch or append .catch(...) to
startServer() that logs the error via logger.error and invokes gracefulShutdown
or process.exit; reference the startServer function and gracefulShutdown in your
fix so any rejection is explicitly caught and logged.
In `@src/mcp/tools.ts`:
- Around line 22-38: Extract the repeated empty-string validation into a helper
named assertNonEmpty(value, fieldName) that throws new
AppError(CommonErrors.INVALID_INPUT, `${fieldName} is required and cannot be
empty`, true); add this helper to src/mcp/tools.ts (module scope or exported)
and replace the repeated checks for graphName, query, and key inside the tool
handlers (the async handlers that currently do if (!graphName?.trim()) / if
(!query?.trim()) / if (!key?.trim())) with calls to assertNonEmpty(graphName,
'Graph name'), assertNonEmpty(query, 'Query'), and assertNonEmpty(key, 'Key')
respectively to remove duplication and ensure consistent error messages.
In `@src/services/redis.service.ts`:
- Around line 44-94: The _initialize method currently uses recursion and a flat
5s delay; change it to an iterative retry loop (use for or while) that attempts
connection up to maxRetries, uses the same exponential backoff + jitter as
falkordb.service.ts (delay = Math.min(5000 * 2 ** retryCount, 30000) +
Math.random() * 1000), and preserves existing behavior: log attempts with
sanitizeUrl, construct createClient and await connect/ping, cleanly disconnect
and nullify client on failure, increment retryCount per attempt, reset
retryCount on success, and throw the AppError after exhausting retries; update
references to this._initialize recursion to instead loop inside _initialize and
ensure logging and error payloads remain the same.
| async initialize(): Promise<void> { | ||
| if (this.initializingPromise) { | ||
| return this.initializingPromise; | ||
| } | ||
|
|
||
| this.retryCount = 0; | ||
| this.initializingPromise = this._initialize(); | ||
|
|
||
| try { | ||
| await this.initializingPromise; | ||
| } finally { | ||
| this.initializingPromise = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing idempotency guard — calling initialize() twice leaks the connected client.
falkordb.service.ts has an if (this.client) { return; } guard (lines 22-24) to prevent overwriting a live client, but redis.service.ts does not. A second initialize() call after a successful connection will overwrite this.client on line 51 without closing the existing one.
🐛 Proposed fix
async initialize(): Promise<void> {
+ // Idempotency guard: skip if already connected
+ if (this.client) {
+ return;
+ }
+
if (this.initializingPromise) {
return this.initializingPromise;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/redis.service.ts` around lines 29 - 42, The initialize() method
can overwrite a live Redis client when called a second time, leaking the
existing connection; add an idempotency guard that returns immediately if
this.client is already set (e.g., at the start of initialize()), and keep the
existing initializingPromise logic to handle concurrent inits; ensure
_initialize() does not replace this.client if a client was created in the
meantime (re-check this.client before assigning) so the existing client is never
closed/overwritten.
| } catch (error) { | ||
| // Clean up failed client before retrying or throwing | ||
| if (this.client) { | ||
| try { | ||
| await this.client.disconnect(); | ||
| } catch { | ||
| // Ignore disconnect errors | ||
| } | ||
| this.client = null; | ||
| } | ||
|
|
||
| if (this.retryCount < this.maxRetries) { | ||
| this.retryCount++; | ||
| logger.warn('Failed to connect to Redis, retrying...', { | ||
| attempt: this.retryCount, | ||
| maxRetries: this.maxRetries, | ||
| error: error instanceof Error ? error.message : String(error) | ||
| }); | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 5000)); | ||
| return this._initialize(); | ||
| } else { | ||
| const appError = new AppError( | ||
| CommonErrors.CONNECTION_FAILED, | ||
| `Failed to connect to Redis after ${this.maxRetries} attempts: ${error instanceof Error ? error.message : String(error)}`, | ||
| true | ||
| ); | ||
|
|
||
| logger.error('Redis connection failed permanently', appError); | ||
| throw appError; | ||
| } | ||
| } |
There was a problem hiding this comment.
logger.error not awaited before throw — log may be lost.
On line 90, logger.error(...) is fire-and-forget (no await), but the very next line throws. Since logger.error is async (it sends MCP notifications), the log may never complete. The equivalent path in falkordb.service.ts (line 95) correctly uses await logger.error(...).
Proposed fix
- logger.error('Redis connection failed permanently', appError);
+ await logger.error('Redis connection failed permanently', appError);
throw appError;📝 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.
| } catch (error) { | |
| // Clean up failed client before retrying or throwing | |
| if (this.client) { | |
| try { | |
| await this.client.disconnect(); | |
| } catch { | |
| // Ignore disconnect errors | |
| } | |
| this.client = null; | |
| } | |
| if (this.retryCount < this.maxRetries) { | |
| this.retryCount++; | |
| logger.warn('Failed to connect to Redis, retrying...', { | |
| attempt: this.retryCount, | |
| maxRetries: this.maxRetries, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| await new Promise(resolve => setTimeout(resolve, 5000)); | |
| return this._initialize(); | |
| } else { | |
| const appError = new AppError( | |
| CommonErrors.CONNECTION_FAILED, | |
| `Failed to connect to Redis after ${this.maxRetries} attempts: ${error instanceof Error ? error.message : String(error)}`, | |
| true | |
| ); | |
| logger.error('Redis connection failed permanently', appError); | |
| throw appError; | |
| } | |
| } | |
| } catch (error) { | |
| // Clean up failed client before retrying or throwing | |
| if (this.client) { | |
| try { | |
| await this.client.disconnect(); | |
| } catch { | |
| // Ignore disconnect errors | |
| } | |
| this.client = null; | |
| } | |
| if (this.retryCount < this.maxRetries) { | |
| this.retryCount++; | |
| logger.warn('Failed to connect to Redis, retrying...', { | |
| attempt: this.retryCount, | |
| maxRetries: this.maxRetries, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| await new Promise(resolve => setTimeout(resolve, 5000)); | |
| return this._initialize(); | |
| } else { | |
| const appError = new AppError( | |
| CommonErrors.CONNECTION_FAILED, | |
| `Failed to connect to Redis after ${this.maxRetries} attempts: ${error instanceof Error ? error.message : String(error)}`, | |
| true | |
| ); | |
| await logger.error('Redis connection failed permanently', appError); | |
| throw appError; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/redis.service.ts` around lines 62 - 93, The catch block in
_initialize handles permanent Redis failure but calls logger.error(...) without
awaiting it, risking lost async logs before the subsequent throw; change the
call to await logger.error('Redis connection failed permanently', appError) so
the log completes before throwing the AppError (keep the rest of the
cleanup/retry logic intact and ensure this await remains inside the same async
catch scope that references this.client, this.retryCount, this.maxRetries,
AppError and CommonErrors.CONNECTION_FAILED).
Add configurable transport mode via MCP_TRANSPORT env var: - 'stdio' (default): existing stdio transport for AI client integration - 'http': Streamable HTTP transport for remote/networked access HTTP mode features: - Session management with per-session McpServer instances - API key authentication via Authorization Bearer header - Configurable port via MCP_PORT env var - Graceful HTTP server shutdown Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add idempotency guard in RedisService.initialize() to prevent overwriting a live client connection - Replace recursive retry in _initialize() with iterative loop using exponential backoff with jitter (consistent with FalkorDBService) - Await logger.error() before throw in all error paths to prevent lost log delivery - Fix stale comment in prompts.ts (was 'graph_list resource', now 'user_setup prompt') - Fix git clone URL in README to point to upstream FalkorDB repo - Add language specifiers to fenced code blocks (MD040) - Add .catch() to startServer() to handle unhandled rejections Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 35 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function readRequestBody(req: IncomingMessage): Promise<string> { | ||
| return new Promise((resolve, reject) => { | ||
| let body = ''; | ||
| req.on('data', (chunk: Buffer) => { body += chunk.toString(); }); | ||
| req.on('end', () => resolve(body)); | ||
| req.on('error', reject); |
There was a problem hiding this comment.
readRequestBody buffers the entire request body with no size limit. For the HTTP transport this makes the server vulnerable to memory exhaustion via large POST bodies. Add a maximum body size (and abort/return 413 when exceeded) while reading the stream.
| function readRequestBody(req: IncomingMessage): Promise<string> { | |
| return new Promise((resolve, reject) => { | |
| let body = ''; | |
| req.on('data', (chunk: Buffer) => { body += chunk.toString(); }); | |
| req.on('end', () => resolve(body)); | |
| req.on('error', reject); | |
| const MAX_REQUEST_BODY_SIZE_BYTES = 1024 * 1024; // 1 MiB | |
| function readRequestBody(req: IncomingMessage): Promise<string> { | |
| return new Promise((resolve, reject) => { | |
| let body = ''; | |
| let totalBytes = 0; | |
| let finished = false; | |
| const cleanup = (): void => { | |
| req.off('data', onData); | |
| req.off('end', onEnd); | |
| req.off('error', onError); | |
| }; | |
| const onError = (err: Error): void => { | |
| if (finished) { | |
| return; | |
| } | |
| finished = true; | |
| cleanup(); | |
| reject(err); | |
| }; | |
| const onData = (chunk: Buffer): void => { | |
| if (finished) { | |
| return; | |
| } | |
| totalBytes += chunk.length; | |
| if (totalBytes > MAX_REQUEST_BODY_SIZE_BYTES) { | |
| finished = true; | |
| cleanup(); | |
| const error = new Error('Request body too large'); | |
| (error as any).statusCode = 413; | |
| // Stop reading further data from the client. | |
| req.destroy(error); | |
| reject(error); | |
| return; | |
| } | |
| body += chunk.toString(); | |
| }; | |
| const onEnd = (): void => { | |
| if (finished) { | |
| return; | |
| } | |
| finished = true; | |
| cleanup(); | |
| resolve(body); | |
| }; | |
| req.on('data', onData); | |
| req.on('end', onEnd); | |
| req.on('error', onError); |
| async initialize(): Promise<void> { | ||
| // Idempotency guard: don't overwrite an already-connected client | ||
| if (this.client) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.initializingPromise) { | ||
| return this.initializingPromise; |
There was a problem hiding this comment.
RedisService.initialize() is not idempotent: if it’s called again after a successful connection, initializingPromise is null so it will create a new client and overwrite this.client without closing the existing connection. Add a guard similar to FalkorDBService (e.g., return early when this.client is already connected / isOpen), or close the existing client before recreating it.
| // Read the request body | ||
| const body = await readRequestBody(req); | ||
| const parsedBody = JSON.parse(body); |
There was a problem hiding this comment.
JSON.parse(body) is executed without guarding for invalid/empty JSON. A malformed request will throw and can crash the HTTP transport handler (potentially taking down the process). Wrap parsing in try/catch and return a 400 response on parse failures.
| // Read the request body | |
| const body = await readRequestBody(req); | |
| const parsedBody = JSON.parse(body); | |
| // Read and parse the request body safely | |
| let parsedBody: unknown; | |
| try { | |
| const body = await readRequestBody(req); | |
| if (!body || body.toString().trim().length === 0) { | |
| res.writeHead(400, { 'Content-Type': 'application/json' }); | |
| res.end(JSON.stringify({ error: 'Bad Request: Empty request body' })); | |
| return; | |
| } | |
| parsedBody = JSON.parse(body); | |
| } catch (err) { | |
| logger.warn('Failed to parse JSON request body', { error: err instanceof Error ? err.message : String(err) }); | |
| res.writeHead(400, { 'Content-Type': 'application/json' }); | |
| res.end(JSON.stringify({ error: 'Bad Request: Invalid JSON payload' })); | |
| return; | |
| } |
| logging: {}, | ||
| }, | ||
| }); | ||
| logger.setMcpServer(sessionServer); |
There was a problem hiding this comment.
The singleton logger is re-pointed to each newly-created sessionServer via logger.setMcpServer(sessionServer). With concurrent HTTP sessions this will route MCP log notifications to whichever session connected most recently (and potentially leak logs across sessions). Consider making the logger session-scoped (new Logger per session) or having Logger support multiple MCP servers keyed by session/transport.
| logger.setMcpServer(sessionServer); |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add multi-stage Dockerfile (node:18-alpine, defaults to HTTP transport) - Add .dockerignore - Add Docker and docker-compose usage instructions to README Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary by CodeRabbit
New Features
Infrastructure
Documentation
Chores
Tests