fix(mcp): never respond to JSON-RPC notifications (#129)

[rohitg00]

Fixes #129 reported by @GuillaumeOuint.

Symptom

```
MCP client for agentmemory failed to start: MCP startup failed: Transport closed
```

Codex CLI v0.120.0 fails to start the standalone MCP server. Other MCP clients (Claude Desktop, Cursor, OpenClaw) work fine with the same binary. Running `npx agentmemory-mcp` directly in a terminal also "works" — it prints the v0.8.1 banner and waits for input. Only Codex's stdio handshake breaks.

Root cause

`src/mcp/transport.ts` unconditionally writes a JSON-RPC response for every incoming message, including notifications. Per JSON-RPC 2.0 §4.1:

A Notification is a Request object without an "id" member. [...] The Server MUST NOT reply to a Notification, including those that are within a batch request.

Lenient clients (Claude Desktop, Cursor) silently ignore unsolicited responses. Strict clients (Codex CLI) treat them as protocol violations and close the transport. That's exactly what Guillaume sees.

The exact sequence on Codex CLI:

1. Codex sends initialize (request, has id) → server responds ✓
2. Codex sends notifications/initialized (notification, no id)
3. Our handler in `standalone.ts:138-139` returns `{}`
4. `transport.ts:62-67` writes `{"jsonrpc":"2.0","result":{}}` to stdout (no `id` field because `request.id` is undefined)
5. Codex parses an unsolicited response → closes transport
6. Guillaume sees `Transport closed`

`memory_save` / `memory_recall` etc never had a chance to fire because the handshake itself was broken.

Fix

• `src/mcp/transport.ts`: detect notifications via missing/null `id`, run the handler for side effects, then drop the response. Errors inside notification handlers are logged to stderr (not silently swallowed) so production debugging still works
• `src/mcp/transport.ts`: malformed messages with no `id` are also dropped silently (consistent with notification semantics) instead of being responded to with `id: null`
• `src/mcp/transport.ts`: extracted `processLine()` as an exported function so the line-handling logic is testable independently of `process.stdin` / `process.stdout`. The `createStdioTransport` factory still works the same way externally
• `test/mcp-transport.test.ts` (new): 9 tests covering all paths
  • Request responses (success + error)
  • Notification suppression (no `id` + `id: null` + handler-throws)
  • Malformed input (parse error, invalid request with `id`, invalid request without `id`, empty/whitespace lines)

The existing `test/mcp-standalone.test.ts` mocks the transport entirely so it never exercised the notification path. The new file imports the real `processLine` and verifies the actual behavior.

Why the bug went undetected

• All existing MCP clients we test against (Claude Desktop, Cursor, OpenClaw, Hermes, the gateway plugin) tolerate unsolicited responses — they just drop them
• Codex CLI's MCP support is newer and stricter
• The standalone server passes a manual smoke test in a terminal because no client is doing the handshake — only the stderr banner is visible

Test plan

• `npm test` → 693 / 693 passing (684 baseline + 9 new)
• `npm run build` → clean
• After merge + release: ask @GuillaumeOuint to upgrade to the new version and re-test with the same Codex CLI config
• Should also fix any other strict MCP clients that hit the same issue (probably more to come as MCP adoption broadens)

Related

• Closes MCP doesn't work on codex-cli #129 (pending verification from reporter)
• Reporter installed `@agentmemory/agentmemory` globally so they're seeing v0.8.1 — this fix needs a release on top of main to actually reach them

Summary by CodeRabbit

• Bug Fixes

  • More robust JSON-RPC handling: malformed JSON yields a Parse error (id null); invalid request shapes return Invalid Request when an id is present; non-primitive ids are rejected.
  • Notifications (no id or id null) no longer generate responses; notification handler failures are logged without replying.
  • Handler exceptions for regular requests produce an internal error response echoing the request id.

• Tests

  • Added comprehensive tests covering parsing, request/notification behavior, id validation, and error responses.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c4375cf-3cc6-4053-82b7-6568b08bd92e

📥 Commits

Reviewing files that changed from the base of the PR and between 745ad87 and 461ef82.

📒 Files selected for processing (2)

• src/mcp/transport.ts
• test/mcp-transport.test.ts

---

📝 Walkthrough

Walkthrough

Extracted per-line JSON-RPC handling into processLine(), made request id optional and response id nullable, added strict validation and notification suppression (errors from notification handlers go to stderr), and refactored stdio transport to use processLine().

Changes

Cohort / File(s)	Summary
JSON-RPC Transport Implementation src/mcp/transport.ts	Made JsonRpcRequest.id optional and JsonRpcResponse.id nullable. Added exported processLine(line, handler, writeOut, writeErr) to parse/validate JSON-RPC per-line, emit -32700 parse errors (id: null), emit -32600 invalid request errors for malformed requests or invalid id types, detect notifications (missing/null id) and suppress responses while logging handler errors to writeErr. Refactored createStdioTransport() to delegate to processLine() and write responses to stdout.
Tests test/mcp-transport.test.ts	Added Vitest suite for processLine() covering: successful request responses, handler-thrown request errors (-32603), notification suppression (no stdout for missing/null id), stderr logging for notification handler exceptions, parse error handling for invalid JSON (-32700 with id: null), ignoring empty/whitespace lines, invalid-request cases (missing jsonrpc), and JSON-RPC §4 id-type validation (reject non-primitive ids).

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Transport as processLine
participant Handler
participant Stdout
participant Stderr

Client->>Transport: send line (JSON-RPC request)
Transport->>Transport: parse JSON
alt parse error
    Transport->>Stdout: write {"jsonrpc":"2.0","id":null,"error":{-32700,...}}
else valid request
    Transport->>Transport: validate shape and id
    alt invalid request (id primitive)
        Transport->>Stdout: write {"jsonrpc":"2.0","id":<id>,"error":{-32600,...}}
    else notification (id missing or null)
        Transport->>Handler: call handler (no response expected)
        alt handler throws
            Transport->>Stderr: write diagnostic about notification handler error
        end
    else regular request
        Transport->>Handler: call handler
        alt handler returns result
            Transport->>Stdout: write {"jsonrpc":"2.0","id":<id>,"result":...}
        else handler throws
            Transport->>Stdout: write {"jsonrpc":"2.0","id":<id>,"error":{-32603,...}}
        end
    end
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰✨ I nibble bytes and parse each line,
Notifications hop, no answer this time,
When handlers tumble, stderr gets a peep,
Responses return only promises we keep. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(mcp): never respond to JSON-RPC notifications' accurately summarizes the main change: preventing server responses to JSON-RPC notifications, which is the core fix for issue #129.
Linked Issues check	✅ Passed	All coding requirements from issue #129 are met: notifications (missing or null id) no longer produce responses, errors in notification handlers are logged to stderr, malformed messages are handled correctly, and the processLine function enables proper testing.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with fixing the JSON-RPC protocol violation. The interface updates, processLine extraction, and tests all serve the primary objective of suppressing responses to notifications.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mcp-notification-no-response-129

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 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/mcp/transport.ts`:
- Around line 54-95: The code currently treats any non-null id as valid and may
call handler or echo invalid ids; before invoking handler (around where parsed
is cast to JsonRpcRequest and before calling isNotification/handler), explicitly
validate request.id: if request.id is null/undefined treat as a notification; if
request.id is present but not a string or number, immediately writeOut a
JSON-RPC error with code -32600 and id: null and return; only call handler when
id is absent (notification) or id is a string/number, and when writing responses
use the validated id (casted to string | number) in writeOut; ensure writeErr
path for notifications remains unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c17ff76a-e1a3-4b1e-a25e-c8862e3df140

📥 Commits

Reviewing files that changed from the base of the PR and between 9bee067 and 745ad87.

📒 Files selected for processing (2)

• src/mcp/transport.ts
• test/mcp-transport.test.ts

[rohitg00]

Addressed the CodeRabbit id-validation finding in 461ef82.

Before: processLine only checked id != null in the invalid-shape path, so (a) malformed messages carrying id: {} could be echoed back with that object as the response id, and (b) valid-shape requests with id: {} / [] / true fell through to the handler and got a response using the bogus id. Both are JSON-RPC 2.0 §4 violations.

Now:

• New isValidId() helper: string | number | null | undefined.
• rawId extracted once as unknown.
• Invalid-shape path only echoes when rawId is string or number; malformed-with-bad-id drops silently (can't safely synthesize an id).
• After shape validation, bad-id requests get -32600 with id: null before the handler is invoked.

5 new tests in test/mcp-transport.test.ts cover object/array/boolean ids, the malformed+non-primitive drop, and a positive string-id path. Full suite: 698 passing.