Fix MCP stdio framing and avoid UTF-8 slicing in Codex ingestion - #19
Fix MCP stdio framing and avoid UTF-8 slicing in Codex ingestion#19paulolimajr77 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughRefactored the MCP server stdio transport to detect and operate in either line-delimited JSON or Content-Length header-framed mode, with stateful mode selection on first input. Also replaced byte-slice truncation with Unicode-aware Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@src/mcp/mod.rs`:
- Around line 164-168: The code currently uses body_len (from content_length)
directly to allocate vec![0_u8; body_len] and then reads into it, which allows a
hostile client to request huge allocations; add an explicit guard before
allocation: define and use a MAX_MCP_BODY_LEN constant (or configurable limit)
and validate content_length is present, non-negative, and <= MAX_MCP_BODY_LEN,
returning an error (e.g., Error::Other or a more specific PayloadTooLarge) if it
exceeds the limit; only after this check convert content_length to usize,
allocate the buffer (body), and call reader.read_exact(&mut body). Ensure you
reference the existing symbols body_len, content_length, and reader.read_exact
when applying the change.
- Around line 91-97: The code currently treats any non-JSON first_line as
HeaderFramed which causes malformed legacy payloads like "oops" to be routed to
parse_header_line; change the decision in the block that calls
looks_like_json(&first_line) so that TransportMode::HeaderFramed is set only
when the first_line actually looks like an MCP header (e.g., starts with a
header key such as "Content-Length:" or matches header syntax), otherwise keep
TransportMode::LineDelimitedJson and return the first_line for JSON parsing;
update the branch around TransportMode::HeaderFramed/read_framed_request and the
condition used to detect headers (introduce or reuse a helper such as
is_mcp_header(first_line) if needed) so non-header, non-JSON lines still get
routed into the JSON parse path used by run()/parse_header_line error handling.
🪄 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: 290aa672-4202-4895-8c56-7fa147eca33c
📒 Files selected for processing (3)
src/mcp/mod.rssrc/palace/convo_miner.rssrc/palace/room_detect.rs
|
Addressing the two automated review findings in
I could not rerun |
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 `@src/mcp/mod.rs`:
- Around line 187-194: The Content-Length handling currently allows
last-one-wins; change the code in the if
name.eq_ignore_ascii_case("Content-Length") block to reject duplicates by
checking if content_length is already Some before parsing: if
content_length.is_some() return an Error::Other with a clear message about
duplicate Content-Length, otherwise parse value.trim().parse::<usize>() as
before and store into *content_length = Some(parsed); reference the
name.eq_ignore_ascii_case("Content-Length") branch, the content_length variable,
and return Error::Other on duplicate to avoid accepting conflicting headers.
- Around line 228-232: The transport detection helper looks_like_mcp_header
currently only recognizes "Content-Length" and thus misclassifies framed clients
that send "Content-Type" first; update looks_like_mcp_header (and any place that
relies on it) to treat either "Content-Length" or "Content-Type"
(case-insensitive) before the ':' as evidence of framed/MCP mode so the code
does not default to LineDelimitedJson when headers appear in any order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if name.eq_ignore_ascii_case("Content-Length") { | ||
| let parsed = value.trim().parse::<usize>().map_err(|e| { | ||
| Error::Other(format!( | ||
| "invalid Content-Length header value '{}': {e}", | ||
| value.trim() | ||
| )) | ||
| })?; | ||
| *content_length = Some(parsed); |
There was a problem hiding this comment.
Reject duplicate Content-Length headers.
This is currently last-one-wins. Conflicting duplicates can make the reader consume the wrong body length and leave the stream misaligned for the next request.
Suggested fix
if name.eq_ignore_ascii_case("Content-Length") {
let parsed = value.trim().parse::<usize>().map_err(|e| {
Error::Other(format!(
"invalid Content-Length header value '{}': {e}",
value.trim()
))
})?;
- *content_length = Some(parsed);
+ if content_length.replace(parsed).is_some() {
+ return Err(Error::Other(
+ "duplicate Content-Length header in MCP request".to_string(),
+ ));
+ }
}📝 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.
| if name.eq_ignore_ascii_case("Content-Length") { | |
| let parsed = value.trim().parse::<usize>().map_err(|e| { | |
| Error::Other(format!( | |
| "invalid Content-Length header value '{}': {e}", | |
| value.trim() | |
| )) | |
| })?; | |
| *content_length = Some(parsed); | |
| if name.eq_ignore_ascii_case("Content-Length") { | |
| let parsed = value.trim().parse::<usize>().map_err(|e| { | |
| Error::Other(format!( | |
| "invalid Content-Length header value '{}': {e}", | |
| value.trim() | |
| )) | |
| })?; | |
| if content_length.replace(parsed).is_some() { | |
| return Err(Error::Other( | |
| "duplicate Content-Length header in MCP request".to_string(), | |
| )); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mod.rs` around lines 187 - 194, The Content-Length handling currently
allows last-one-wins; change the code in the if
name.eq_ignore_ascii_case("Content-Length") block to reject duplicates by
checking if content_length is already Some before parsing: if
content_length.is_some() return an Error::Other with a clear message about
duplicate Content-Length, otherwise parse value.trim().parse::<usize>() as
before and store into *content_length = Some(parsed); reference the
name.eq_ignore_ascii_case("Content-Length") branch, the content_length variable,
and return Error::Other on duplicate to avoid accepting conflicting headers.
| fn looks_like_mcp_header(line: &str) -> bool { | ||
| match line.split_once(':') { | ||
| Some((name, _)) => name.eq_ignore_ascii_case("Content-Length"), | ||
| None => false, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For MCP/LSP stdio framing, may Content-Typelegally appear beforeContent-Length, or must Content-Length be the first header line?
💡 Result:
-
LSP (stdio / base protocol): The spec says the header part “consists of header fields” and that
Content-Lengthis required andContent-Typeis supported, but it does not require any particular header ordering (it never saysContent-Lengthmust be the first line). SoContent-Typemay legally appear beforeContent-Lengthas long asContent-Lengthis present and the header section is correctly terminated with\r\n\r\n. [1] -
MCP (stdio transport): MCP stdio does not use
Content-Length/Content-Typeframing at all in the current spec; messages are newline-delimited JSON-RPC (must not contain embedded newlines). [2]
Sources: [1] LSP 3.17 Specification – “Header Part” (microsoft.github.io) (microsoft.github.io), [2] MCP Specification (2025-11-25) – “Transports / stdio” (modelcontextprotocol.io)
Citations:
- 1: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/
- 2: https://modelcontextprotocol.io/specification/2025-11-25/basic/transports
🏁 Script executed:
cd /tmp && find . -name "mod.rs" -path "*/mcp/*" 2>/dev/null | head -5Repository: bunkerlab-net/mempalace
Length of output: 49
🏁 Script executed:
rg -A 10 -B 5 "looks_like_mcp_header" --type rustRepository: bunkerlab-net/mempalace
Length of output: 1697
🏁 Script executed:
rg "TransportMode|read_request" src/mcp/mod.rs -A 3 -B 1Repository: bunkerlab-net/mempalace
Length of output: 2766
Detect framed mode from Content-Type too.
The transport mode detection defaults to LineDelimitedJson when the first line fails the looks_like_mcp_header() check. If a framed client sends Content-Type: before Content-Length:, the helper returns false, causing the logic to incorrectly lock into line-delimited mode and fail to parse all subsequent framed messages.
Per the LSP specification, header order is not constrained—Content-Type may legally appear before Content-Length.
Suggested fix
fn looks_like_mcp_header(line: &str) -> bool {
match line.split_once(':') {
- Some((name, _)) => name.eq_ignore_ascii_case("Content-Length"),
+ Some((name, _)) => {
+ let name = name.trim();
+ name.eq_ignore_ascii_case("Content-Length")
+ || name.eq_ignore_ascii_case("Content-Type")
+ }
None => false,
}
}📝 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.
| fn looks_like_mcp_header(line: &str) -> bool { | |
| match line.split_once(':') { | |
| Some((name, _)) => name.eq_ignore_ascii_case("Content-Length"), | |
| None => false, | |
| } | |
| fn looks_like_mcp_header(line: &str) -> bool { | |
| match line.split_once(':') { | |
| Some((name, _)) => { | |
| let name = name.trim(); | |
| name.eq_ignore_ascii_case("Content-Length") | |
| || name.eq_ignore_ascii_case("Content-Type") | |
| } | |
| None => false, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mod.rs` around lines 228 - 232, The transport detection helper
looks_like_mcp_header currently only recognizes "Content-Length" and thus
misclassifies framed clients that send "Content-Type" first; update
looks_like_mcp_header (and any place that relies on it) to treat either
"Content-Length" or "Content-Type" (case-insensitive) before the ':' as evidence
of framed/MCP mode so the code does not default to LineDelimitedJson when
headers appear in any order.
|
Hi @paulolimajr77, thank you so much for the pull request. Once CodeRabbit is happy with everything I'm more than happy to take a look and get this merged. |
2 participants
Summary
This PR contains two focused fixes that were reproduced locally while using MemPalace with modern MCP clients and Codex JSONL conversation imports.
1. Native MCP stdio framing support
mempalace mcpcurrently reads stdin line-by-line and writes raw JSON followed by\n. That works with legacy line-delimited JSON-RPC usage, but it fails with MCP clients that speak stdio usingContent-Lengthframing.The result is that a valid MCP
initializerequest gets parsed as if theContent-Lengthheader were JSON, causing startup/handshake failure before tool discovery.This patch updates the MCP server to:
Content-Lengthheadersinitialize,tools/list, andtools/call2. Avoid UTF-8 slicing panics in Codex conversation ingestion
The conversation/room classification logic sliced
content[..N]before lowercasing. Since Rust string slices are byte-indexed, this can panic when the text contains multibyte UTF-8 characters and the truncation point lands inside a code point.This patch switches those hot paths to
content.chars().take(N).collect::<String>(), which keeps the truncation character-safe.The affected paths are used during conversation import / room detection, including Codex JSONL-style content with emoji and non-ASCII text.
Files changed
src/mcp/mod.rssrc/palace/convo_miner.rssrc/palace/room_detect.rsTesting
Passed
cargo fmt --checkAdded test coverage
initializeresponsetools/listtools/call(mempalace_status)Windows validation note
I also validated the MCP fix locally against a Codex setup using framed stdio messages and confirmed the server could complete
initialize, list tools, and call tools successfully.I was not able to complete
cargo test/cargo build --releaseon this Windows host because the localx86_64-pc-windows-gnullvmtoolchain is missing the configured linker (x86_64-w64-mingw32-clang). That appears to be an environment/toolchain issue rather than a code issue.Out of scope
I used a local bridge script as a temporary environment workaround while validating Windows behavior, but that workaround is intentionally not included here. This PR contains only the native source fixes that make sense upstream.
Summary by CodeRabbit
Bug Fixes
Tests