fix: the filed bugs, plus MCP 2026-07-28 dual-era support - #3
Merged
Conversation
Every length check used str::len() (UTF-8 bytes) while the error message and the advertised MCP JSON schema both said "characters". A summary of 1900 characters written with em dashes is 5696 bytes and was rejected as "Summary exceeds maximum length of 2000 characters", while 1900 ASCII characters was accepted. Verified against a running server. Byte semantics are intentional and kept: the on-disk record encodes the summary length as a u16. What was wrong was the reporting. - Add src/model/validation.rs as the single source of truth. The HTTP create path, the HTTP batch path and both MCP store handlers each carried a hand-rolled copy of these checks, and the copies had drifted. - Errors now name the field, the measured byte count, the character count and the limit, and explain the discrepancy only when one exists: "summary is 5700 bytes (1900 characters), which exceeds the 2000-byte limit. Limits are measured in UTF-8 bytes, not characters: ..." - Memory::validate() already produced correct messages but had zero call sites. It now delegates here instead of being a second source of truth. - Validate the HTTP batch endpoint, which previously checked only that the summary was non-empty. A summary of 65536 bytes or more reached the wrapping `as u16` cast in record.rs and corrupted the stored record. - Fix a latent panic in the CLI: &text[..200] aborts when byte 200 is not a char boundary. Replaced with truncate_on_char_boundary(). - Replace magic literals with named constants and advertise the real limits in the tool schemas (fullText maxLength, array maxItems). - Correct docs/mcp.md and docs/guide.md, which stated the wrong unit. Behavioral changes: entities/topics/emotions counts are now enforced on the HTTP path; an empty-string summary is now rejected over MCP; namespace names are ASCII-only on both paths (previously HTTP accepted Unicode). Removing three public ValidationError variants is semver-breaking, which is acceptable at 0.1.x. Error message text changed on every path; the machine-readable error code and field are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
fullText's 1 MiB limit was exactly the daemon's 1 MiB frame limit, so a request that passed validation could exceed the frame once wrapped in its JSON envelope. Reproduced against a running server: four sequential store_memory calls on one connection gave OK, then a 1,048,570-byte write failed with "Broken pipe", and then every later call failed the same way forever. Only restarting the MCP server recovered. The response direction was worse and silent. A single get_memory on a large memory, or a recall_memories returning several, made the daemon write an over-limit frame. The client rejected it after consuming the 4-byte length prefix but before draining the payload, leaving unread JSON in the socket -- no broken pipe, just every subsequent read_u32 parsing from the middle of a message. - Raise the frame limit to 8 MiB and keep fullText at 1 MiB. What overflows is JSON escaping of the text (up to 6x for control bytes), which is a transport artifact, so the headroom belongs in the transport. The worst-case arithmetic is documented on the constant. - Check the size before any byte reaches the socket, so an oversized request fails cleanly and leaves the connection usable. - Drain an oversized but well-formed incoming frame into a sink under a timeout. Length-prefixed framing makes the boundary known, so this resynchronizes exactly rather than best-effort. A length beyond the drain limit is treated as unrecoverable desync and closes instead. - Stop dropping the whole connection for one bad frame; recoverable errors now reply and keep serving. - Add lazy reconnect to DaemonClient with bounded attempts. Retry only read-only methods: store_memory mints a server-side id, so a transparent retry could double-write, and neither a failed write nor a failed read proves the operation did not execute. Mutating calls return an explicit "may or may not have been applied" error instead. - Correlate response ids. Stale replies are skipped up to a bound, and an impossible id resets the connection rather than looping forever. - Negotiate the frame limit via ping, defaulting to 1 MiB for an unannounced peer, so a v0.1.10 daemon or client on either side of the socket is never sent a frame it will reject. - Dispatch "list_memories", which the client called but the daemon had no arm for, leaving that tool broken in daemon mode. The primary fix does not depend on retry: poisoning plus lazy reconnect alone ends the permanent-failure behavior. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
A bug report described store_memory silently corrupting records: trailing parameters absorbed into the preceding field as literal text, tags empty, namespace wrong, supersedes ignored -- all returned as success. The corruption itself originates client-side, in tool-call serialization; the server does structured serde_json parsing over newline-delimited stdio with no text splicing, no shared buffers and no cross-request state, and the report's own reproduction stores cleanly here. That part is not ours to fix. What is ours is that the server accepted it cheerfully. Arguments were read with .get().and_then(..).ok().unwrap_or_default(), so a tags array sent as a string became an empty vec, a wrong-typed namespace fell back to the default partition, and an unparseable supersedes became None -- each silently, each reported as success. That is why roughly ten bad writes accumulated before anyone noticed. - Add src/mcp/args.rs: typed extractors that fail loudly and name the field, shared by store_memory and store_memories so the two cannot drift again. Length and count limits delegate to model::validation, so there is still one source of truth for them. - Detect literal tool-call markup in summary and fullText and reject the write. The report suggested keying on any parameter-named tag; that would be unusable, since <summary> is standard HTML5, the backbone of C# doc comments, and an Atom element. The detector instead anchors on tokens that are not English words in tag position -- the client's reserved vendor prefix, function-call literals, the exact camelCase parameter tags -- plus closing-tag adjacency, which needs two independent signals before a generic name counts. Errors carry the matched token and byte offset so a false positive is diagnosable in seconds. - Reject rather than sanitise. Stripping the markup would leave a record whose tags, namespace and supersedes are still silently wrong, and whose embedding and graph edges are computed from garbage-adjacent text. A rejected write costs one round trip and lands in the agent's transcript, where it self-corrects. - Log rejections at warn with the field and rule, never the text body. Explicit JSON null keeps meaning "absent" for every optional field: many SDKs serialize optionals as null, and treating that as a type error would break them on every call. Behavioral change: a wrong-typed tags/namespace, or an unparseable parentId/supersedes, now fails the whole write where it previously stored a quietly incomplete memory. Batch items that used to store minus their tags now report an error instead, so stored counts drop and errors rise for affected clients. The per-item error object gains an additive "field". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
Closes #2. The issue proposed bumping the protocol version as a backward-compatible first step. It is not. PROTOCOL_VERSION is echoed verbatim into the `initialize` response, so bumping it alone tells a legacy client it is talking to a revision where `initialize` does not exist, `ping` is removed, and resultType/ttlMs/cacheScope are mandatory -- none of which this server emitted. Under 2025-06-18 lifecycle rules such a client should disconnect. The bump is therefore landed together with the stateless path, never before it. The spec explicitly sanctions serving both eras on one endpoint, so the era is decided per message rather than per connection: a `params._meta` carrying io.modelcontextprotocol/protocolVersion is modern, an `initialize` is legacy. The `_meta` key wins over the method name, since a legacy client can send `_meta` for progressToken but never that reverse-DNS key. - Split McpServer into a stateless McpDispatcher plus a thin wrapper holding only legacy lifecycle state. Modern HTTP requests take no lock at all. Merely bypassing the session map would have funnelled every concurrent modern request through one mutex, which is worse than today's per-session locks and would not have fixed the scaling complaint the issue actually raises. - Add server/discover, which the revision requires and the issue does not mention. It is also the era probe, so it and `ping` are answered without `_meta` -- demanding a protocol version in order to discover which versions are supported is circular, and any error there would push a modern client into the legacy fallback. - Add the required resultType to every result, and SEP-2549 caching hints. The catalogs are compile-time constants, so they advertise public/1h; resources/read is live instance state, so it advertises private/0. Emitted on both eras: the 2025-06-18 Result type is an open index signature, so the extra keys are schema-legal there. - Validate the SEP-2243 headers against the body, including the base64 sentinel, and answer GET/DELETE with 405 on the modern path. - Fix resources/templates/list, which emitted resource_templates instead of resourceTemplates. That was broken against every MCP revision, so no compliant client could ever read the result. - Validate Origin (a spec MUST this server never honoured) and apply a body limit to /mcp, which sits outside the tower stack and had none. Loopback is allowed on any scheme or port, since DNS rebinding needs an attacker-controlled hostname. - Fix three session-map defects: entries never expired, a repeat initialize orphaned the previous entry, and a failed initialize still inserted an unusable one. Unknown sessions now answer 404 with a JSON-RPC body saying to re-initialize, so expiry is recoverable. - Stop discarding the real request id on parse and session errors. Uncertainties are preserved as code comments rather than silently resolved: 2025-11-25 is deliberately absent from the legacy list pending a wire diff, the base64 alphabet needs confirming against SEP-2243, and notification header requirements are undefined by the revision. Breaking: mcp_router takes a config argument; /mcp enforces a 10 MiB body limit where it had none; non-loopback browser origins get 403; legacy HTTP sessions expire after 30 minutes idle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
A bug report found that supersedes never appeared to do anything. The parameter was in fact wired up, but every way it could fail was swallowed: a non-existent target produced GraphError::MemoryNotFound which was logged as "non-fatal" and discarded, a duplicate did the same, and a persistence failure did the same again. The store returned success in all three cases, and nothing in the response ever indicated whether a link had been made. parentId was worse -- it was parsed into StoreInput and then read by nothing at all, on both the MCP and HTTP batch paths. It has never created an edge. The report asked that the recall side be verified independently rather than assumed broken or fine. It was, and it had two real defects. - Validate the target before anything is written: it must exist, live in the same namespace, and not be the memory itself. The record used to be committed before the edge was attempted, so failing afterwards would have meant unwinding six subsystems. Checking first makes failing the whole store free, and costs nothing but a lookup. - Treat a duplicate edge as success, not failure. Supersedes is a desired end state, so asking for it twice is idempotent, which also makes client retries safe. - Report the outcome. The store response now carries the target and one of applied / alreadyApplied / appliedNotDurable / failed, so a caller can tell the difference between a link that was made, one already present, one that will not survive restart, and one that was not made. A persistence failure is not an error: the memory really was stored and the edge really is live this session, and saying otherwise would be a lie about a store that succeeded. - Make parentId actually link, via the same helper, so the two kinds of caller-specified edge cannot drift apart again. - Recall: stop removing a superseded memory before its replacement has been validated. Previously the original was dropped first, and if the replacement had decayed to ghost or tombstone, or failed to load, the result set simply lost it with nothing in its place -- permanently unrecallable. - Recall: run injected replacements through the same filters as every other candidate. They bypassed all of them, so a replacement in a different namespace leaked into results. That is a data-isolation defect, not a ranking nicety. - Guard the supersedes chain walk with a visited set, so a cycle returns None instead of an arbitrary node after ten hops. - HTTP batch: every silent `continue` now records a failure entry. Previously items vanished from the response with no explanation. - Correct the docs, which claimed the old memory is "deprioritized" in search. It is removed and replaced. Breaking: a non-existent or cross-namespace supersedes/parentId now fails the store (MCP isError, HTTP 422) where it previously returned success. Callers passing stale ids will see errors immediately. New response fields are all optional and skipped when empty, so happy-path payloads are unchanged, and StoredMemory.supersedes carries serde(default) so a new client can still decode an old daemon's response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
Entities, topics and emotions had a count limit but no per-string length limit anywhere. Two consequences, both silent. First, the derived tag. Those labels are merged into tags as "entity/<name>" after validation, so a 128-byte entity became a 135-byte tag, which Tag::new rejected, which filter_map(..ok()) discarded without an error or a log. The caller sent 20 tags, 17 were stored, and nothing said so. The limits now reserve the prefix -- 121 bytes for entities, 122 for topics, 120 for emotions -- so a request that validates cannot produce a tag that gets thrown away, and a test asserts the derived tag survives Tag::new at exactly the limit. Second, the frame budget. The 8 MiB daemon frame limit was justified by arithmetic that assumed 128-byte labels; without a per-string limit the envelope was unbounded and that justification did not hold. There is now a test that builds the worst-case valid request out of control bytes, which JSON escapes at 6x, and asserts it serializes within MAX_MESSAGE_SIZE. It fails the moment anyone raises a content limit or lowers the frame limit. - Validate every element of tags, entities, topics and emotions in the shared validator, naming the field, the index, the measured bytes and the limit. Counts are still reported first when both fail. - Replace the silent tag drops with parse_tags_lossy, which logs each drop with the field, the value and the reason. It stays a drop rather than an error because the reachable case is the tag alphabet, not length -- an entity named Jose with an accent is legitimate input that should not fail a store. - Close the daemon RPC store_memory path, which deserialized StoreInput straight off the socket with no limit checks at all. It was the last door through which an over-long label reached storage and was dropped while the call reported success. - Advertise maxLength on the label arrays in both tool schemas, and document the limits and the prefix reservation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
Two independent reviews of 740cdd1..HEAD. They cleared the areas most at risk -- resolve_supersedes' index bookkeeping and many-to-one handling, the shared validator being reachable from all five store doors, no multi-byte panic in the markup detector, and the daemon's mixed-version clamp in both directions -- and found the following. - is_retry_safe was wrong about its own criterion. It listed search and find_similar as read-only, but both record accesses, which advance last_accessed_at and append FSRS events; replaying one after a mid-call socket death would double-count and skew the decay schedule. Removed them, and stated the real bar in the doc: no observable state change of any kind, not merely "not a write". get_memory stays, because the daemon dispatches it to the storage adapter, which does no access recording -- the review cited QueryEngine::get_memory, which the socket never reaches. Their non-retry error also claimed the operation "may or may not have been applied", which is false for a read; those two now say plainly that nothing was written. - parentId still drifted from supersedes on the HTTP door. explicit_links exists precisely so the two cannot diverge, but only the MCP path was converted; both HTTP sites still called the generic add_edge, which persists an edge without bumping edge_count -- so a parent edge counted toward ranking or not depending on which door created the memory. Both now use the shared helper, and the generic add_edge is removed rather than left as the trap that caused this. - The daemon's drain timeout was bypassable. It guarded the oversized path, but a peer declaring exactly the limit and then sending nothing hit an untimed read_exact, pinning a connection task and allocating the full declared length up front. The payload read is now bounded in time, and the buffer grows in chunks, so a declared-but-unsent 8 MiB frame costs one chunk instead of 8 MiB. - Nothing bounded a *sequence* of recoverable frame errors, so a peer could stream malformed frames indefinitely, each costing a slot. Eight consecutive failures now close the connection; any good frame resets. - Malformed responses were reported as TooLarge, telling the caller to shorten fullText in response to a serialization bug. Now distinguished. - The markup detector rejected legitimate XML. Bare `<invoke name=` and `<parameter name=` were single-signal matches added beyond the design; TestNG suite files use exactly that spelling. Real corruption always carries the client's vendor prefix, which another rule already catches alone, so the bare forms are gone and TestNG and Maven fragments store normally. - sweep_sessions computed its count as a subtraction across a DashMap that concurrent initialize handlers mutate, which underflows and panics in debug builds. Counted inside retain instead, which is also exact. - ping's frame-limit negotiation released the lock between the call and the write, so a concurrent reconnect to a v1 daemon could have its correct 1 MiB clamp overwritten with 8 MiB -- the exact permanent stream corruption the design prevents. Not reachable today, since ping runs once at startup, but it would become reachable the moment anyone adds a periodic health check. The exchange now holds the lock throughout. - is_era_neutral_method promised ping was "answered identically in both eras" while the dispatcher correctly returned method-not-found for a modern ping. Renamed to is_handshake_exempt_method and documented: only server/discover is era-neutral; ping is legacy-only and merely tolerated without _meta. - store_memories emitted "supersedes": null on every batch item, because json! ignores skip_serializing_if. The single-memory tool, which serializes the struct, was already correct. Several of the new tests were confirmed to fail against the code they fix, including the sweep underflow panic, the stale clamp overwrite, and the connection-pinning timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
Cargo.toml has declared `rust-version = "1.94"` since fdf69f7, but every workflow still installed 1.87 and clippy.toml still claimed 1.87 as the MSRV. Cargo refuses to build a package whose rust-version exceeds the active toolchain, so every CI job has been failing on that alone since the bump landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the bug report filed on 2026-08-10/11, and closes #2.
The report's diagnosis was wrong in ways that mattered
Bug 1's corruption is client-side, not ours. Running the report's own reproduction on a fresh connection — including the ~2800-char write it named as the trigger, and the small writes after it — stored perfectly clean every time. The server does structured
serde_jsonparsing over newline-delimited stdio with no text splicing, no shared buffers and no cross-request state. The</invoke>literal is the client's own tool-call syntax.That mattered because the report treated Bugs 2 and 3 as downstream effects of Bug 1, which hid two independent defects:
str::len()(UTF-8 bytes) while the message and the advertised JSON schema said characters. A 1900-character summary of em dashes is 5696 bytes and was rejected as "exceeds maximum length of 2000 characters". The reporter's 1794-character summary was em-dash-heavy prose — that alone explains the rejection.parentIdhad never worked at all: it was parsed intoStoreInputand then read by nothing, on any path, in any released version.The "sticky state" intuition was right, but not about the described mechanism.
fullText's 1 MiB limit exactly equalled the daemon's frame limit, so a write that passed validation could exceed the frame once enveloped, killing the socket — andDaemonClientnever reconnected. Reproduced live, and the fix verified end-to-end against a real server.What else this fixes
list_memorieswas never dispatched by the daemon, so the tool was broken in daemon mode.Issue #2 assumed bumping the protocol version was backward-compatible. It isn't — the constant is echoed verbatim into the
initializeresponse. It ships here as a dual-era server, which the spec explicitly sanctions: modern requests are served statelessly with no lock, legacyinitializeclients keep working unchanged.Verification
68 → 478 tests across the branch. Each commit was verified to build and pass independently. The daemon fix and the validation fix were additionally replayed end-to-end against a real server with real embeddings.
CI has been failing on every job since
fdf69f7bumpedrust-versionto 1.94 while the workflows still installed 1.87; that's fixed here so this opens green.Deliberately not done
No backfill of already-corrupted records, and no retrieval-precision tuning — both your call. Note the retrieval complaint (Bug 4) may have a real cause: see the vector-index finding in the follow-up PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JNhUehmChiQKJUjv3QPnkH