fix(brainbar): stabilize json-rpc envelope order#274
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@coderabbitai review |
|
@codex review |
📝 WalkthroughWalkthroughThe PR refactors JSON-RPC response serialization in the MCP framing layer to produce deterministic JSON-RPC envelopes with fixed key ordering ( ChangesDeterministic JSON-RPC Envelope Encoding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
✅ Actions performedReview triggered.
|
|
Swift is not available in this environment, but that's fine. The PR description indicates all tests have passed. Let me now provide a comprehensive review: Bug Bot Review: PR #274 - fix(brainbar): stabilize json-rpc envelope order✅ SummaryThis PR addresses a critical production bug where Claude Desktop rejected BrainBar on boot due to non-deterministic JSON-RPC envelope key ordering. The fix is correct, minimal, and well-tested. 🎯 Core Changes ReviewMCPFraming.swift - The Fix ✅The new
Key insight from the code: The guard clause correctly identifies JSON-RPC envelopes and avoids breaking non-envelope JSON objects. guard let jsonrpc = response["jsonrpc"],
let id = response["id"],
response["result"] != nil || response["error"] != nil else {
return try JSONSerialization.data(withJSONObject: response)
}BrainBarServer.swift - Unified Path ✅The change at line 405 routes the newline-delimited fallback through the same encoder: -guard let jsonData = try? JSONSerialization.data(withJSONObject: response) else { return false }
+guard let jsonData = try? MCPFraming.encodeJSONResponse(response) else { return false }This is critical — before this change, Content-Length framing used the fixed encoder but newline-delimited didn't, creating two different serialization paths. Now both paths produce identical envelope shapes. 🧪 Test Coverage ReviewMCPFramingTests ✅func testEncodesJSONRPCEnvelopeWithJSONRPCFirst()
MCPRouterTests ✅func testEncodedToolsListEnvelopeStartsWithJSONRPC()
Both tests follow TDD pattern described in the PR: they would have failed before the fix, pass after. 🔍 Correctness Analysis✅ No Breaking Changes
✅ Edge Cases Handled
|
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brain-bar/Sources/BrainBar/MCPFraming.swift`:
- Around line 127-131: The encodeJSONResponse helper currently falls back to
JSONSerialization when "result"/"error" are missing, which lets notification
envelopes use non-deterministic Foundation ordering; update encodeJSONResponse
to always use the deterministic JSON path (remove the JSONSerialization
fallback) so notifications as well as responses are encoded via the
deterministic encoder used elsewhere (referencing encodeJSONResponse and the
deterministicEncoder/encoding helper in MCPFraming) and return that Data for all
messages.
In `@brain-bar/Tests/BrainBarTests/MCPRouterTests.swift`:
- Around line 155-157: The assertion currently expects the framed response to
contain "result":" (a string) but tools/list returns an object, so update the
XCTAssertTrue that uses body.hasPrefix(...) in MCPRouterTests.swift to expect
...,"result":{ instead of ...,"result":"; locate the XCTAssertTrue call (the one
checking body.hasPrefix and the accompanying failure message) and change the
prefix string accordingly so the test matches an object-valued result.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae40e121-7838-42d6-90bb-2aaa3166149a
📒 Files selected for processing (4)
brain-bar/Sources/BrainBar/BrainBarServer.swiftbrain-bar/Sources/BrainBar/MCPFraming.swiftbrain-bar/Tests/BrainBarTests/MCPFramingTests.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Cursor Bugbot
- GitHub Check: Macroscope - Correctness Check
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/BrainBarServer.swiftbrain-bar/Sources/BrainBar/MCPFraming.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/BrainBarServer.swiftbrain-bar/Sources/BrainBar/MCPFraming.swift
🔇 Additional comments (2)
brain-bar/Tests/BrainBarTests/MCPFramingTests.swift (1)
131-133: Same prefix bug as the router test.This fixture also encodes
resultas an object, so the expected prefix needs to end with...,"result":{instead of...,"result":".brain-bar/Sources/BrainBar/BrainBarServer.swift (1)
403-409: Good: reuse the shared encoder here.This keeps the newline-delimited branch aligned with the Content-Length path for JSON-RPC response envelopes.
| static func encodeJSONResponse(_ response: [String: Any]) throws -> Data { | ||
| guard let jsonrpc = response["jsonrpc"], | ||
| let id = response["id"], | ||
| response["result"] != nil || response["error"] != nil else { | ||
| return try JSONSerialization.data(withJSONObject: response) |
There was a problem hiding this comment.
Keep notification envelopes on the deterministic path too.
This helper still falls back to JSONSerialization when result/error is absent, so newline-delimited notifications can still come out with Foundation-dependent top-level ordering. If those messages hit the same client/parser, the original ordering bug is not fully closed here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Sources/BrainBar/MCPFraming.swift` around lines 127 - 131, The
encodeJSONResponse helper currently falls back to JSONSerialization when
"result"/"error" are missing, which lets notification envelopes use
non-deterministic Foundation ordering; update encodeJSONResponse to always use
the deterministic JSON path (remove the JSONSerialization fallback) so
notifications as well as responses are encoded via the deterministic encoder
used elsewhere (referencing encodeJSONResponse and the
deterministicEncoder/encoding helper in MCPFraming) and return that Data for all
messages.
| XCTAssertTrue( | ||
| body.hasPrefix(#"{"jsonrpc":"2.0","id":2,"result":"#), | ||
| "Claude Desktop expects jsonrpc to be the first envelope key for tools/list; got: \(body.prefix(80))" |
There was a problem hiding this comment.
Fix the expected prefix.
tools/list returns an object in result, so the framed body starts with ...,"result":{..., not ...,"result":".... As written, this assertion will fail even when the encoder is correct.
🔧 Proposed fix
- XCTAssertTrue(
- body.hasPrefix(#"{"jsonrpc":"2.0","id":2,"result":"#),
- "Claude Desktop expects jsonrpc to be the first envelope key for tools/list; got: \(body.prefix(80))"
- )
+ XCTAssertTrue(
+ body.hasPrefix(#"{"jsonrpc":"2.0","id":2,"result":{"#),
+ "Claude Desktop expects jsonrpc to be the first envelope key for tools/list; got: \(body.prefix(80))"
+ )📝 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.
| XCTAssertTrue( | |
| body.hasPrefix(#"{"jsonrpc":"2.0","id":2,"result":"#), | |
| "Claude Desktop expects jsonrpc to be the first envelope key for tools/list; got: \(body.prefix(80))" | |
| XCTAssertTrue( | |
| body.hasPrefix(#"{"jsonrpc":"2.0","id":2,"result":{"#), | |
| "Claude Desktop expects jsonrpc to be the first envelope key for tools/list; got: \(body.prefix(80))" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Tests/BrainBarTests/MCPRouterTests.swift` around lines 155 - 157,
The assertion currently expects the framed response to contain "result":" (a
string) but tools/list returns an object, so update the XCTAssertTrue that uses
body.hasPrefix(...) in MCPRouterTests.swift to expect ...,"result":{ instead of
...,"result":"; locate the XCTAssertTrue call (the one checking body.hasPrefix
and the accompanying failure message) and change the prefix string accordingly
so the test matches an object-valued result.
1 participant
Summary
jsonrpcfirst, thenid, thenresult/error.tools/listrouter response.Diagnosis
Claude Desktop rejected BrainBar on boot with
Invalid JSON-RPC messageeven though the response was structurally valid JSON-RPC. Live framed socket probing against the installed BrainBar showed the Content-Length path emitted{"id":2,"jsonrpc":"2.0","result":...}. A release-mode TDD test reproduced Foundation dictionary serialization emitting non-MCP-like top-level ordering ({"result":...,"id":2,"jsonrpc":"2.0"}). Python/Node MCP servers emitjsonrpcfirst; this PR makes BrainBar match that stable envelope shape instead of relying on Swift dictionary serialization.Tests
swift test --package-path brain-bar -c release --filter MCPFramingTests/testEncodesJSONRPCEnvelopeWithJSONRPCFirstfailed before the fix with body{"result":{"tools":[{"name":"brain_search"}]},"id":2,"jsonrpc":"2.0"}.swift test --package-path brain-bar -c release --filter MCPFramingTests/testEncodesJSONRPCEnvelopeWithJSONRPCFirstpassed after fix.swift test --package-path brain-bar --filter MCPFramingTestspassed: 8 tests, 0 failures.swift test --package-path brain-bar --filter MCPRouterTests/testEncodedToolsListEnvelopeStartsWithJSONRPCpassed.swift test --package-path brain-barpassed: 339 tests, 0 failures../scripts/run_tests.shpassed: pytest unit suite 1823 passed, 9 skipped, 75 deselected, 1 xfailed; MCP registration 3 passed; isolated eval/hook routing 32 passed; bun suite 1 pass; FTS5 regression PASS../scripts/run_tests.shand passed with the same suites.Deployment Plan
After merge, rebuild and reinstall BrainBar, restart it with enrichment paused, then smoke-test
/tmp/brainbar.sockusingtools/listto confirm the deployed response starts with{"jsonrpc":"2.0","id":2,"result":...}.Note
Medium Risk
Changes JSON-RPC response serialization at the transport boundary (both Content-Length and newline-delimited modes), which could affect compatibility with existing clients if they relied on Foundation’s previous output shape.
Overview
Ensures BrainBar’s MCP JSON-RPC responses serialize with a deterministic envelope key order (
jsonrpc, thenid, thenresult/error) to avoid client-side rejection caused by Foundation dictionary ordering.Routes the newline-delimited JSON-RPC fallback through the same encoder as the Content-Length framing path, and adds regression tests covering both the framing encoder and a real
tools/listrouter response.Reviewed by Cursor Bugbot for commit 45ac3a7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix JSON-RPC envelope key order in
MCPFramingto be deterministicJSONSerialization-based encoding with a newMCPFraming.encodeJSONResponsehelper that explicitly orders top-level keys asjsonrpc,id, thenresult/error.jsonrpcfirst in both framing modes.Macroscope summarized 45ac3a7.
Summary by CodeRabbit
Release Notes
Improvements
Tests