fix(mcp): ensure call logs are reliably written to MongoDB#60
Conversation
The fire-and-forget `logCall` pattern silently dropped writes when the MongoDB connection was not ready (`bufferCommands: false` rejects immediately). Additionally, if a tool handler threw an unhandled error the SDK caught it but `logCall` was never reached, leaving zero trace in the monitoring page. - Replace the fire-and-forget `logCall` with an async `writeCallLog` that calls `connectDB()` before every `McpCallLog.create()`. - Wrap every tool handler in a `loggedTool` helper that catches exceptions, writes the error log entry, then re-throws so the SDK still returns its error payload to the client. - Extract `summariseQueryResult` for query-tool log abbreviation. - Add unit tests covering field persistence, call ordering, and resilience to connectDB / create failures. Co-authored-by: Cursor <cursoragent@cursor.com>
railway-app
Bot
temporarily deployed
to
archmax SemLayer / archmax-pr-60
|
🚅 Deployed to the archmax-pr-60 environment in archmax SemLayer
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
No concrete security issues found in the changed files for the requested threat surfaces.
Checked apps/api/src/mcp/archmax-server.ts and the new test, plus the surrounding MCP auth/query/admin-auth paths. MCP bearer-token lookup and project binding still happen in archmax-route before tool registration/dispatch, token scopes are still passed into every semantic-model tool, and invalid tokens return JSON-RPC errors. execute_query still flows through executeScopedQuery, AST validation, scoped model VIEW materialization, query timeout/concurrency controls, and the 1000-row cap. Admin Better Auth startup secret validation, CSRF middleware, and production cookie attributes are unchanged. No new dependencies or committed secrets were introduced.
Verification note: I attempted npx vitest run apps/api/src/mcp/archmax-server.test.ts packages/core/src/services/mcp-tools.test.ts packages/core/src/services/sql-ast-validation.test.ts, but this checkout has no local node_modules/vitest, so the command could not load vitest/config.
Sent by Cursor Automation: archmax Security Review
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default mode and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 623c582. Configure here.
Docker image readydocker pull ghcr.io/archmaxai/archmax:pr-60 |
writeCallLog was called fire-and-forget inside loggedTool — the same pattern this PR set out to fix. Now properly awaited so the MongoDB write completes before the tool response returns (or the error re-throws). Also refactors loggedTool to remove unnecessary generics and extracts queryToolResult helper to deduplicate query tools. Adds 7 tests exercising loggedTool via registerArchmaxTools: success logging, error logging + rethrow, DB-failure resilience, and query-result summarisation. Co-authored-by: Cursor <cursoragent@cursor.com>
railway-app
Bot
temporarily deployed
to
archmax SemLayer / archmax-pr-60
1 participant
Summary
Root cause: The
logCallfunction was fire-and-forget (McpCallLog.create().catch(...)) without callingconnectDB()first. Since the app usesbufferCommands: false, any MongoDB write attempted before the connection was established (or after a transient disconnect) would silently fail — the.catch()only logged to console, invisible in the monitoring UI. Additionally, if a tool handler threw an unhandled error, the MCP SDK caught it for the client butlogCallwas never reached at all.Fix: Replace the fire-and-forget
logCallwith an asyncwriteCallLogthat always callsconnectDB()beforeMcpCallLog.create(), ensuring the connection is ready. A newloggedToolwrapper wraps every tool handler in try/catch so that even when a handler throws, the error is still logged before re-throwing.Adds unit tests covering field persistence, call ordering, and resilience to
connectDB/createfailures.Test plan
writeCallLogcallsconnectDBbeforeMcpCallLog.create(verified by call ordering test)writeCallLogdoes not throw whencreateorconnectDBfailstsc --noEmit)Made with Cursor
Note
Medium Risk
Touches MCP tool execution and logging flow by making logging awaited and wrapping all tool handlers in centralized try/catch, which could subtly affect timing and error propagation. Main risk is operational (logging/DB connection overhead) rather than user-facing behavior.
Overview
Makes MCP call logging reliable and consistent. Replaces fire-and-forget
logCallwith asyncwriteCallLogthat alwaysconnectDB()s beforeMcpCallLog.create, capturesclientIp, and never throws on logging failures.Wraps every MCP tool in a shared
loggedToolwrapper so successes and thrown errors both write a call log; query tools now log a summarised output ("N rows, M columns") vialogResultwhile still returning the full query payload.Adds a new
archmax-server.test.tssuite covering DB connect-before-write ordering, persisted fields for success/error, resilience to DB failures, tool registration count, and logging behavior for success/throwing/query handlers.Reviewed by Cursor Bugbot for commit 5fe2871. Bugbot is set up for automated code reviews on this repo. Configure here.