Fix correctness gaps and make the evaluation honest (v0.3.0) - #1
Conversation
The MCP server — the headline feature — did not start on a fresh install, and the published results overstated what the benchmark showed. Both are fixed, along with the durability and data-loss issues underneath them. Correctness - MCP server: `mcp` 2.0 renamed FastMCP to mcp.server.mcpserver.MCPServer, so `pip install -e ".[mcp]"` produced a server that exited claiming the package was missing. Load either class; both majors are now tested in CI. - memory_boot ignored its own budget: `remaining or None` turned a fully consumed budget into "no cap", returning 6.6x the requested tokens. The budget is now shared across handoff and recall by MemoryStore.boot. - Concurrent agents silently lost memories: the store rewrote the whole file from a snapshot taken at startup. Writes now take a cross-process lock, merge in anything another agent appended, and land atomically via os.replace. Reads reload when the file changes, so a running server sees another agent's writes. - Fixed a TOCTOU bug in that reload: load() stamped the file after reading it, so a store replaced mid-read was recorded as current and never reloaded again. Two agents then chose the same id and a write vanished. Stamp is now taken before the read; regression test included. - Switching embedders bricked a store (512-d query against 384-d vectors). Mismatched dim or backend now re-embeds from the stored text. - Ids are derived from the ids present, surviving explicit ids and deletions. Memory you can correct - Added forget/update plus memory_list, memory_update, memory_forget and the matching CLI commands. An append-only store keeps recalling stale state. - write_with_status reports when a near-duplicate was dropped; the tools no longer claim a save that did not happen. - Added a relevance floor so an off-topic task returns nothing instead of k unrelated memories the agent cannot tell are noise. Calibrated per embedder from a published sweep. Evaluation - Added a random-k control. It shows the same 78% token saving at 0.07 recall, which is the point: the saving is arithmetic (k of n), and only the 0.86 recall gap is evidence that retrieval works. - Added a paraphrase arm. The default embedder is lexical, and recall falls 0.93 -> 0.43 when queries stop sharing vocabulary with the memories. This is now published rather than implied by the word "semantic". - Added a scaling experiment with distractor memories, so "cost stays fixed as the store grows" is measured on a store that actually grows. - Dropped the precision-vs-baseline comparison; the baseline's 0.10 is just |relevant|/|store|. Also: base64 float16 embeddings (~5x smaller on disk), ingest_markdown chunks oversized sections and counts only real writes, tiktoken in dev so the numbers are exact, CI fails when committed results go stale, and tests went 17 -> 55. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d062da113a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| picked = rng.sample(all_ids, min(k, len(all_ids))) | ||
| record("random_k", picked, sum(count_tokens(by_id[i]["text"]) for i in picked)) |
There was a problem hiding this comment.
On this seven-task benchmark, recording only one random sample per task makes the control extremely seed-dependent: with the committed seed it reports 0.071 recall, while the expected recall for choosing 3 of 14 memories is 0.214 (and individual seeds range far more widely). Because the README uses this unusually low draw to claim a 0.86 retrieval gap, the headline comparison overstates the evidence; average many trials or calculate the expectation analytically before publishing the control.
Useful? React with 👍 / 👎.
| reembed = ( | ||
| stored_dim != self.embedder.dim | ||
| or stored_embedder != type(self.embedder).__name__ |
There was a problem hiding this comment.
When a persisted store is reopened with two different SentenceTransformerEmbedder models that have the same dimension, this check considers them compatible because it compares only the class name and dimension. Their vector spaces are not interchangeable, so old memories retain vectors from the previous model while queries and new writes use the new model, silently producing meaningless rankings; persist and compare model_name or another stable embedder fingerprint and re-embed on mismatch.
Useful? React with 👍 / 👎.
| candidate = "\n\n".join(buffer + [para]) | ||
| if buffer and count_tokens(candidate) > max_tokens: | ||
| out.append("\n\n".join(buffer)) | ||
| buffer = [para] | ||
| else: | ||
| buffer.append(para) |
There was a problem hiding this comment.
When a Markdown section contains one paragraph or an unbroken list longer than --max-tokens, buffer is empty here, so the oversized paragraph is accepted and later emitted unchanged. This defeats the new option's stated guarantee and can create a memory that never fits into the normal recall budget; oversized paragraphs need a secondary sentence- or token-level split.
Useful? React with 👍 / 👎.
1 participant
Addresses the findings from a code review of
main. Two headline problems, plus the durability issues underneath them.The two that mattered most
The MCP server did not start on a fresh install.
mcp>=1.0resolves to 2.x today, which renamedmcp.server.fastmcp.FastMCPtomcp.server.mcpserver.MCPServer.build_server()hit theexcept ImportErrorand exited saying the package was missing — when it was installed. CI never installed themcpextra and no test imported the module, which is why nobody noticed. Now: both classes are supported, and a CI matrix testsmcp<2andmcp>=2separately.The published numbers overstated the benchmark. The 78% token saving is arithmetic — you loaded 3 of 14 memories — and a new random-k control proves it by reporting the same 78% at 0.07 recall. The claim worth making is the 0.86 recall gap at identical token cost, and that is what the README now says.
Correctness
memory_bootbudgetremaining or None→ 0 became "no cap"; returned 264 tokens for a 40-token budgetos.replace; 8 processes × 5 writes = 40/40ValueErroron the first matmul (512-d vs 384-d)mem_{len+1}collided with explicit ids and after deletionsWhile stress-testing the lock I found a TOCTOU bug in the fix itself:
load()stamped the file after reading it, so a store replaced mid-read was recorded as current and never reloaded again — two agents then picked the same id and a write disappeared (~1 run in 7). The stamp is now taken before the read, with a deterministic regression test that fails if the ordering is restored.Memory you can correct
The store was append-only, so a stale
statememory was recalled forever. Addedforget/update, thememory_list/memory_update/memory_forgettools and CLI equivalents.write_with_statusalso reports when a near-duplicate was dropped instead of replying "Saved" for a write that never happened.Added a relevance floor: an off-topic query used to return k memories anyway, and since tool output hides scores, the agent could not tell. Calibrated per embedder from a sweep that ships in the results.
Evaluation
|relevant|/|store|.Also
base64 float16 embeddings (131 KB → 25 KB for 15 memories),
ingest_markdownchunks oversized sections that could never fit a budget,tiktokenindevso numbers are exact, CI fails when committed results go stale, and a trust-boundary note since memories are replayed verbatim into another agent's context.Tests: 17 → 55. Verified on
mcp1.29.0 and 2.0.0, and from a cleanpip install -e ".[dev]".🤖 Generated with Claude Code