fix(csp): use Content-Length framing for MCP stdio transport - #89
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom ContentLengthTransport to support LSP-style Content-Length framing for the MCP stdio transport, replacing the default newline-delimited transport. The feedback suggests several improvements to the new transport implementation: simplifying empty line detection by checking line.is_empty() after stripping suffixes, avoiding unnecessary UTF-8 validation by comparing the header name directly as a byte slice, and simplifying the skip_exact helper using tokio::io::copy and tokio::io::sink instead of manual buffer management.
| if line == b"\r\n" || line == b"\n" || line.is_empty() { | ||
| // Empty line -> end of headers. | ||
| return content_length; | ||
| } | ||
|
|
||
| // Strip trailing newline and optional carriage return. | ||
| let line = line.strip_suffix(b"\n").unwrap_or(&line); | ||
| let line = line.strip_suffix(b"\r").unwrap_or(line); |
There was a problem hiding this comment.
[MEDIUM] Simplify empty line detection by checking after stripping suffixes
Symptom: The code checks for an empty line using a boolean expression with three conditions (line == b"\r\n" || line == b"\n" || line.is_empty()) before stripping the trailing newline and carriage return characters.
Source: Ousterhout — A Philosophy of Software Design (Ch. 4: Cognitive Overload / Complexity).
Consequence: The redundant checks increase cognitive load and make the header parsing loop slightly harder to read and maintain.
Remedy: Strip the trailing \r and \n first, then perform a single, unified check for line.is_empty().
| if line == b"\r\n" || line == b"\n" || line.is_empty() { | |
| // Empty line -> end of headers. | |
| return content_length; | |
| } | |
| // Strip trailing newline and optional carriage return. | |
| let line = line.strip_suffix(b"\n").unwrap_or(&line); | |
| let line = line.strip_suffix(b"\r").unwrap_or(line); | |
| // Strip trailing newline and optional carriage return. | |
| let line = line.strip_suffix(b"\n").unwrap_or(&line); | |
| let line = line.strip_suffix(b"\r").unwrap_or(line); | |
| if line.is_empty() { | |
| // Empty line -> end of headers. | |
| return content_length; | |
| } |
| fn parse_content_length(line: &[u8]) -> Option<usize> { | ||
| let mut parts = line.splitn(2, |&b| b == b':'); | ||
| let name = parts.next()?; | ||
| let value = parts.next()?; | ||
|
|
||
| let name = std::str::from_utf8(name).ok()?; | ||
| if !name.eq_ignore_ascii_case("Content-Length") { | ||
| return None; | ||
| } | ||
|
|
||
| let value = std::str::from_utf8(value).ok()?; | ||
| value.trim().parse::<usize>().ok() | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid unnecessary UTF-8 validation for header name
Symptom: The parse_content_length function converts the header name byte slice to a UTF-8 string before performing a case-insensitive comparison.
Source: Fowler — Refactoring (Primitive Obsession / unnecessary type conversion).
Consequence: Unnecessary UTF-8 validation is performed on the header name, which adds minor overhead and extra code.
Remedy: Use eq_ignore_ascii_case directly on the byte slice name with b"Content-Length".
fn parse_content_length(line: &[u8]) -> Option<usize> {
let mut parts = line.splitn(2, |&b| b == b':');
let name = parts.next()?;
let value = parts.next()?;
if !name.eq_ignore_ascii_case(b"Content-Length") {
return None;
}
let value = std::str::from_utf8(value).ok()?;
value.trim().parse::<usize>().ok()
}| async fn skip_exact<R: AsyncReadExt + Unpin>( | ||
| read: &mut R, | ||
| mut n: usize, | ||
| ) -> Result<(), std::io::Error> { | ||
| const CHUNK: usize = 4096; | ||
| let mut buf = vec![0u8; CHUNK]; | ||
| while n > 0 { | ||
| let to_read = n.min(CHUNK); | ||
| read.read_exact(&mut buf[..to_read]).await?; | ||
| n -= to_read; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Simplify skip_exact using tokio::io::copy and tokio::io::sink
Symptom: The skip_exact function manually manages a 4KB buffer and a loop to discard bytes from the reader.
Source: Fowler — Refactoring (Alternative Classes with Different Interfaces / standard library reuse).
Consequence: Manual buffer management and loop logic increase code complexity and the potential for bugs compared to using standard Tokio utilities.
Remedy: Use tokio::io::copy with read.take(n) and tokio::io::sink() to discard the bytes efficiently and cleanly.
async fn skip_exact<R: AsyncReadExt + Unpin>(
read: &mut R,
n: usize,
) -> Result<(), std::io::Error> {
let mut discard = read.take(n as u64);
tokio::io::copy(&mut discard, &mut tokio::io::sink()).await?;
Ok(())
}
Greptile SummaryThe PR replaces rmcp's newline-delimited stdio transport with a custom
Confidence Score: 3/5The PR should not merge until header parsing is bounded, because an MCP client can otherwise exhaust the server's memory through stdin. The body-size guard runs only after unbounded header-line reads, leaving the long-running MCP process vulnerable to memory exhaustion from a client-controlled unterminated header. Files Needing Attention: crates/csp/src/bin/csp/mcp_transport.rs
|
| Filename | Overview |
|---|---|
| crates/csp/src/bin/csp/mcp_transport.rs | Adds the framing implementation, but header parsing allocates without a bound and exposes a client-triggered memory-exhaustion path. |
| crates/csp/src/bin/csp/mcp_server.rs | Replaces rmcp's built-in stdio transport with the new Content-Length transport. |
| crates/csp/Cargo.toml | Enables Tokio's io-util feature for buffered asynchronous reads and writes. |
| crates/csp/src/bin/csp/main.rs | Registers the new transport module in the binary. |
Sequence Diagram
sequenceDiagram
participant Host as MCP host
participant Transport as ContentLengthTransport
participant Server as rmcp service
Host->>Transport: Content-Length header + JSON body
Transport->>Transport: Parse headers and bounded body
Transport->>Server: JSON-RPC message
Server-->>Transport: JSON-RPC response
Transport-->>Host: Content-Length header + JSON body
Prompt To Fix All With AI
### Issue 1
crates/csp/src/bin/csp/mcp_transport.rs:119-120
**Unbounded MCP header allocation**
When an MCP client writes a long header line without a newline, `read_until` grows `line` without limit before the 8 MiB body check runs, causing memory exhaustion and making the MCP session unavailable. Bound both individual header lines and the complete header block while parsing. **How this was verified:** The client-controlled stdin reaches this unbounded allocation before any message-size guard is applied.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(csp): use Content-Length framing for..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let mut line = Vec::new(); | ||
| match read.read_until(b'\n', &mut line).await { |
There was a problem hiding this comment.
When an MCP client writes a long header line without a newline, read_until grows line without limit before the 8 MiB body check runs, causing memory exhaustion and making the MCP session unavailable. Bound both individual header lines and the complete header block while parsing. How this was verified: The client-controlled stdin reaches this unbounded allocation before any message-size guard is applied.
Knowledge Base Used: MCP server
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/csp/src/bin/csp/mcp_transport.rs
Line: 119-120
Comment:
**Unbounded MCP header allocation**
When an MCP client writes a long header line without a newline, `read_until` grows `line` without limit before the 8 MiB body check runs, causing memory exhaustion and making the MCP session unavailable. Bound both individual header lines and the complete header block while parsing. **How this was verified:** The client-controlled stdin reaches this unbounded allocation before any message-size guard is applied.
**Knowledge Base Used:** [MCP server](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/code-search/-/docs/mcp-server.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Architecture diagram
sequenceDiagram
participant Client as MCP Client
participant Stdio as csp MCP Process
participant Transport as ContentLengthTransport
participant Server as MCP Server
participant Search as Code Search Engine
Note over Client,Stdio: Process boundary: client communicates with csp over stdin/stdout
Note over Transport: LSP-style framing: Content-Length header, blank line, JSON body
Note over Stdio,Transport: Tokio BufReader and BufWriter provide asynchronous buffered I/O
Client->>Stdio: Write request to stdin
Stdio->>Transport: Read buffered bytes
Transport->>Transport: Parse Content-Length header
Transport->>Transport: Read exactly N body bytes
Transport-->>Server: Decode framed JSON-RPC request
alt initialize
Server->>Server: Negotiate MCP protocol and capabilities
Server-->>Transport: JSON-RPC initialize response
else tools/list
Server->>Server: Enumerate available tools
Server-->>Transport: JSON-RPC tool list response
else tools/call search
Server->>Search: Execute code search with tool arguments
Search-->>Server: Search results
Server-->>Transport: JSON-RPC tool result response
end
Transport->>Transport: Serialize response JSON
Transport->>Transport: Write Content-Length header and blank line
Transport->>Stdio: Write framed response to stdout
Stdio-->>Client: Content-Length framed JSON-RPC response
alt Invalid or incomplete frame
Transport-->>Server: Framing or parse error
Server-->>Transport: JSON-RPC error response
Transport->>Stdio: Write framed error to stdout
Stdio-->>Client: Error response without newline-delimited parsing
end
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
rmcp's stdio transport is newline-delimited, which does not conform to the
MCP stdio transport spec. Replace it with a custom transport that reads and
writes Content-Length framed messages, matching what standard MCP clients
(including Oh My Pi) expect.
- Add crates/csp/src/bin/csp/mcp_transport.rs implementing the framing
- Enable tokio io-util feature for BufReader/BufWriter/Async{Read,Write,BufRead}Ext
- Switch mcp_server::run_mcp to use ContentLengthTransport
8507884 to
0f74ec1
Compare
1 participant
Problem
csp mcpcurrently uses rmcp's built-instdio()transport, which is newline-delimited JSON. The MCP stdio transport spec expects LSP-styleContent-Lengthframing, so spec-compliant clients (Oh My Pi, Claude Code, etc.) receive aParse errorand cannot use the server.Fix
ContentLengthTransportincrates/csp/src/bin/csp/mcp_transport.rsthat reads/writesContent-Lengthframed messages.io-utilfeature forBufReader/BufWriterand theAsync{BufRead,Read,Write}Exttraits.mcp_server::run_mcpto use the new transport.Verification
cargo check -p code-search-pleasepassescargo build -p code-search-please --releaseproduces acspbinaryinitialize,tools/list, andtools/call(search) all working without parse errors.Summary by cubic
Switches the MCP stdio transport to LSP-style
Content-Lengthframing so spec-compliant clients (Oh My Pi, Claude Code) no longer get parse errors.ContentLengthTransportincrates/csp/src/bin/csp/mcp_transport.rsthat reads and writes framed messages with size and header limits.io-utilfeature for buffered I/O traits.run_mcpinstead ofrmcp's newline-delimitedstdio().Written for commit 0f74ec1. Summary will update on new commits.