feat(mcp): add request log writer (PoC for #1351)#1487
Closed
nhangen wants to merge 3 commits into
Closed
Conversation
Wraps the CallToolRequestSchema handler with a JSONL writer that appends one entry per call to ~/.gitnexus/mcp-requests.log with ts, tool, durationMs, resultBytes, error fields. A simpler counterpart to the OpenTelemetry/Prometheus direction discussed in abhigyanpatwari#1351 — kept small so the design conversation can happen on top of working code. Either approach plugs into the same instrumented() wrapper. Configuration via GITNEXUS_MCP_REQUEST_LOG env var (off / on / explicit path). Failures to write are swallowed; server availability matters more than logging fidelity. Tests: 11 new in test/unit/mcp/request-log.test.ts (pass via npx vitest run; typecheck clean on the changed files).
|
@nhangen is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
Pre-review the resolveLogPath was opt-out (unset → default path) which contradicted the docstring and PR body claim of opt-in. Both pr-review-toolkit and a meta-auditor flagged it; meta-auditor also caught that 'GITNEXUS_MCP_REQUEST_LOG=on' was treated as a literal file path named 'on' in cwd because the on/true/1 branch never existed. Fix: unset/empty/off/false/0 → null (disabled); on/true/1 → default ~/.gitnexus/mcp-requests.log; explicit path → that path. Tests grow from 11 → 12 covering the new affirmative-boolean branch and pinning the opt-in default.
Three independent expert reviewers (security/privacy, async-correctness, project-fit) reviewed the PoC. Two real code issues:
1. Async-correctness reviewer found that the MCP CallToolRequestSchema handler converts thrown tool errors into a returned { isError: true } envelope BEFORE the instrumented wrapper sees them — so the wrapper's catch branch never fires for tool errors and the log was recording error: null for every failure. Fix: instrumented() now accepts an optional errorOf(result) hook; the wrap site at server.ts reads result.isError and passes the error text through.
2. Security/privacy reviewer noted that while tool inputs (params/rawArgs) are not logged, the error field captures error messages verbatim, and tool errors routinely echo user input (cypher parse errors include the offending clause, etc.). Documented in the module header so operators treat the log as input-adjacent.
Project-fit reviewer predicted the upstream maintainer will ask why we don't use the project's existing pino logger (core/logger.ts) — recorded in the open design questions on the PR body rather than rewriting the PoC.
Tests: 12 → 14 (one for envelope-error logging via errorOf, one pinning the no-errorOf back-compat default of error: null on success).
1 task
Author
|
Apologies for the noise here. I misread the assignment on the first pass — @magyargergo asked for OpenTelemetry/Prometheus with quantized, anonymous metrics, and I shipped a JSONL request log instead, with OTel framed as a follow-up. That inverts the ask. Closing this and starting fresh with an OTel meter + Prometheus exporter as the primary path. Will open a new PR shortly. |
Collaborator
|
@nhangen this could be interesting 🧐 I'm open for something new if they are well tested in practice and there are working solutions. :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1351.
Summary
Wraps the
CallToolRequestSchemahandler with aninstrumented()helper that appends one JSONL line pertools/callto a configurable log file:{"ts":"2026-05-10T18:00:00Z","tool":"impact","durationMs":214,"resultBytes":1834,"error":null}The same wrapper can host an OTLP span emitter alongside or instead of the JSONL writer — the single seam is
src/mcp/server.ts:166.Changes
gitnexus/src/mcp/request-log.ts—resolveLogPath(),appendRequestLog(),instrumented(toolName, fn, resultBytesOf?, errorOf?). Async append,mkdir -pon the parent, errors swallowed.gitnexus/src/mcp/server.ts— wraps the existing handler. Behaviour unchanged when logging is disabled.errorOfreads the{ isError: true, ... }envelope so tool errors land in the log with their message (the handler converts thrown errors into a returned envelope, so the wrapper's catch branch never sees them).gitnexus/test/unit/mcp/request-log.test.ts— 14 unit tests.Configuration
Opt-in default:
GITNEXUS_MCP_REQUEST_LOGoff/false/0on/true/1~/.gitnexus/mcp-requests.log/abs/path/logPrivacy
Tool inputs (
params/rawArgs) are never logged. Theerrorfield captures error messages verbatim; tool errors routinely echo input (cypher parse errors include the offending clause, etc.). Treat the log as input-adjacent.Verify
End-to-end after
npm run build:Risk
JSON.stringify+ onefs.appendFileper tool call. Fire-and-forget; the request handler returns without awaiting.shutdown()— no drain in this PoC. See decision table below.Recommended decisions for v1
OTEL_EXPORTER_OTLP_ENDPOINT. Same wrapper hosts both.erroralready leaks input under failure; broadening would move privacy backwards.shutdown()await Promise.allSettled(pending)beforeserver.close(). Cheap and bounded.appendFilevscore/logger.ts(pino)createLogger('mcp-requests', { ... })gives NDJSON, log-injection hardening, and the existing test seam.One open question
Opt-in or opt-out default? Opt-in maximises privacy posture; opt-out gives server-side ground truth without setup. Both are one line in
resolveLogPath(). Maintainer's call.Draft until that decision lands and we scope whether #2/#4/#5/#6 ship in this PR or as follow-ups.
Local CI:
npx vitest run test/unit/mcp/request-log.test.ts— 14 pass.npx tsc --noEmit— clean on the changed files (baseline has unrelated tree-sitter errors from--ignore-scriptsinstall)..husky/pre-commitnot run locally; please verify in CI.