Skip to content

Instantly share code, notes, and snippets.

@n-rodriguez

n-rodriguez/AUDIT.md

Last active June 24, 2026 01:41
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save n-rodriguez/cf07fce0b0144d1bf8c9252cebb74f7f to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save n-rodriguez/cf07fce0b0144d1bf8c9252cebb74f7f to your computer and use it in GitHub Desktop.
Download ZIP
Road to 1.0 — comprehensive audit of spider-gazelle/mcp.cr
Raw
AUDIT.md

Road to 1.0 — Comprehensive audit of mcp.cr

Audited against main. The core architecture is genuinely good — capability-derived handler wiring, a clean transport abstraction, a real typed MCPError, and an ergonomic @[MCP::Tool] annotator. This document maps what stands between today and a 1.0 tag, organized by severity tier with a suggested phased plan. Everything is cited file:line.

A short umbrella issue links here; per-tier child issues track the work.

Tier 0 — Blockers (compile/break when the path is exercised)

Crystal only typechecks methods on instantiation, so these pass a bare crystal build but detonate when a user touches them — the most dangerous class of 1.0 hazard.

# file:line Bug
1 server.cr:124 @tools[rtool.tool.name] = rt — rt undefined (→ rtool). add_tools can't compile when called. Same scope bug: Log lines referencing name in add_tools/add_resources (:117,:211).
2 annotator.cr:275 &.asi64 — no such method; any tool with an Int64 parameter fails to compile (should be as_i64?).
3 jsonrpc_response.cr:28 @[JSON::Field(key: "NextCursor")] — PascalCase; wire field must be nextCursor. Breaks all pagination.
4 jsonrpc_response.cr:366 ListToolsResult#initialize calls super(@meta) instead of super(@next_cursor, @meta) (siblings at :251/278/306 are correct).
5 sse.cr:91 broadcast calls conn.send(event, data, id, retry) but the signature is send(data, id, event, retry) — malformed SSE frames.
6 client.cr:158,162 subscribe_resource/unsubscribe_resource declare return type EmptyRequestResult — a type defined nowhere (→ EmptyResult).
7 client.cr:165 + server.cr:165,168 add_prompt overloads pass a captured block positionally into block-param overloads — won't compile; add_prompts inherits it.
8 request_params.cr:13-20 progress_token/= call meta["…"]?/delete/[]= on meta : Hash? with no nil-guard → runtime undefined method for Nil whenever _meta is absent (the default).

Tier 1 — Protocol completeness vs MCP 2025-06-18

Declared in methods.cr but no handler (server replies -32601) or stubbed:

  • logging/setLevel — MISSING server handler; client can send, level never stored/honored.
  • completion/complete — MISSING server handler; completions capability advertisable but unimplemented.
  • resources/subscribe / unsubscribe — MISSING server-side; client can send, no subscription registry; resources/updated never triggers.
  • resources/templates/list — STUBBED: always returns empty (server.cr:404), no registration API.
  • sampling/createMessage — client receive handler MISSING (server can send, client can't answer).
  • elicitation/create — MISSING both directions (type scaffolding only).
  • notifications/cancelled — not consumed either side; in-flight work is not aborted.
  • Pagination — STUBBED end-to-end: cursor never read, next_cursor hardcoded nil (server.cr:350).
  • notifications/progress send — no helper, no progressToken read from inbound _meta; a tool can't emit progress. Key mismatch: @progress_handlers keyed by request id, on_progress looks up by token.

Solid and done well: initialize + version negotiation, tools list/call, prompts list/get, resource list/read, ping (both directions), progress receive, roots (client).

Tier 2 — Correctness & concurrency

  • Tool error model (server.cr:317 + shared/protocol.cr:139): a raised tool handler → -32603, and unknown tool name → -32603 (should be -32602). Per spec, tool execution failures should be a normal result with isError:true. (Macro handlers wrap; manual ones don't.)
  • No body size bound (streamable_server_transport.cr:277 gets_to_end) — memory-exhaustion vector.
  • Unsynchronized cross-fiber Hash mutation: streamable per-session maps (:13-16), @response_handlers/@roots mutated from the read fiber without a lock — corruption under MT.
  • Single global session masquerading as multi-session (@session_id/@initialized transport-global); 2nd client rejected.
  • Mid-stream disconnect leaks all four streamable maps (no close hook).
  • Runners connect a singleton server per POST/session (streamable_runner.cr, sse_runner.cr) → connect overwrites @transport, stomping concurrent sessions; also re-chains on_* handlers per POST → unbounded growth.
  • increment_and_get (ext/atomic.cr) is non-atomic (add(1); get) → duplicate ids.
  • _meta round-trip broken (resource_contents.cr): JSON::Any.new(read_raw).as_h? always nil.
  • Double _on_close on stdio client (fires from both ensure and the start fiber).

Tier 3 — Performance / memory / CPU

  • read_buffer.cr:17-27 — O(n²) + unbounded memory (HOT, every stdio message): consumed bytes never reclaimed; newline scan restarts at offset 0 each call.
  • 3 JSON parse passes + 1 re-serialize per inbound message (jsonrpc_message.cr + protocol.cr discriminators) where 1 suffices.
  • Streamable send scans the whole request map → O(R²) per batch (streamable_server_transport.cr:191-223); fix with a stream_id → Set(request_id) reverse index.
  • Busy-poll in stdio transports (select … timeout(1s) + sleep(100ms)) — constant idle CPU on every connection; receive? already blocks.
  • Per-POST server.connect grows the handler chain unboundedly on long-lived sessions.
  • Startup-only (low): annotator double JSON round-trips & regex type-mapping (not hot — fine).

Perf strengths: bounded stdio channels (backpressure), O(1) hash dispatch, atomic CAS lifecycle, no global lock across tool IO.

Tier 4 — Public API / SemVer surface

  • Public typo in a base class: abstract class PaginateRequestdParams (request_params.cr:23) — superclass of every List*Params; renaming post-1.0 is breaking.
  • Incoherent error contract: bare raise "string" across the client/server surface despite a real MCPError; three vocabularies (Exception/MCPError/ArgumentError) — callers can only string-match.
  • Misleading nilable returns: list_tools : ListToolsResult? etc. — request never returns nil (it raises).
  • JSON::Any leaking where typed structs exist (call_tool(Hash(String,JSON::Any)), list_roots(...)), inconsistent with typed create_message.
  • Mutable public state that desyncs internals (ServerOptions#capabilities, Protocol#transport, StreamableServerTransport#session_id).
  • Process-global runners (@@sessions/@@transports class vars) — can't run two servers in one process.
  • puts to STDOUT in runners — corrupts a stdio MCP stream.
  • Two competing definition paths (macro annotator vs manual add_*); bless the annotator, the manual add_* is where most Tier-0 bugs live.
  • Name stutter MCP::Server::Server; SseServerRunner naming asymmetry.

Tier 5 — Tests, CI, packaging

  • No CI anywhere (no .github/, no workflow) — nothing guards the Tier-0 blockers.
  • shard.yml declares no dev-deps; specs require "wait_group" etc. — non-deterministic contributor/CI builds. Missing repository/documentation.
  • The two largest, most logic-dense files have zero tests: annotator.cr (575 LOC) and shared/protocol.cr (341 LOC, the correlation/timeout/cancel core).
  • Streamable transport spec is smoke-only (asserts handlers not called); no POST/GET/batch/session/correlation.
  • A few no-op / tautological assertions: sse_spec "heartbeats" asserts their absence; client_spec ListToolsResult is_a? line lacks .should.
  • None of the Tier-0 bugs would be caught by the current suite (Int64 tool, cursor round-trip, broadcast arg order all uncovered).

Suggested path to 1.0

  1. Stop the bleeding — fix the 8 Tier-0 blockers + add CI (crystal build + crystal spec + ameba) so they can't recur. (Small, mechanical, high-trust — PR-able now.)
  2. Honor the contract — tool errors → isError, unknown tool → -32602, bounded body, nextCursor/pagination, logging/setLevel.
  3. Finish or fence the protocol — either implement (completion, resource subscribe, sampling-receive, elicitation, cancellation) or stop advertising those capabilities until they exist.
  4. Concurrency hardening — lock/own the cross-fiber maps, per-session server, connect-once, disconnect cleanup.
  5. Perf — read_buffer compaction, single-pass decode, reverse-index streamable sends, drop busy-polls.
  6. API freeze — rename PaginateRequestdParams, unify on MCPError, tighten JSON::Any/nilable returns, instance-scope the runners — before the tag, since these are breaking.
  7. Tests — cover annotator schema-gen (per arg type), the protocol correlation/timeout core, and a real streamable integration spec.

Audit methodology: multi-pass static review of the main source tree across correctness, concurrency, performance, MCP 2025-06-18 protocol completeness, public-API/SemVer surface, and test/CI coverage. Findings are cited by file and line; the headline compile/wire-format bugs were verified by direct reading.

Produced with Claude Opus 4.8 [1M], with the headline findings hand-verified against the source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment