2026-09-21

A stranger's code review caught two real gaps we'd missed

Posted Spomory publicly and got real technical review back, not just reactions. Two comments each pointed at a specific line of code and asked a question we didn't have a clean answer for. Both turned into real fixes, shipped in v0.2.0 — this is what was actually wrong and what changed.

"Is this relying on the host's approval dialog, or could a client just ignore the hint?"

That was the question, more or less. forget_all_memory is annotated destructive_hint=true, and in Claude Desktop that annotation is what makes the per-tool approval dialog show up before it runs. The question underneath: what happens on a host that doesn't bother showing that dialog, or an agent configured to auto-approve destructive tools?

Checked the actual tool implementation to answer honestly instead of guessing. It called delete_all() on the very first invocation — no second step, no dry run. destructive_hint is documented in the MCP spec as an advisory hint; nothing requires a host to gate on it. So the honest answer was: yes, this depended entirely on the host choosing to enforce something the protocol doesn't require it to enforce.

@mcp.tool(annotations=_TOOL_ANNOTATIONS["forget_all_memory"])
def forget_all_memory(confirm: bool = False) -> str:
    """Permanently delete the entire memory graph -- every entity and relation.

    destructive_hint=True on this tool's annotations is only a hint -- the
    MCP spec doesn't require a host to gate on it. confirm is the actual
    enforcement: the first call (confirm left at its default, False) never
    deletes anything -- it only reports what a real call would remove.
    """
    ...

A bare call now reports the entity/relation counts a real call would remove and deletes nothing. Wiping the memory graph for real needs an explicit second call with confirm=true — regardless of whether the host's UI shows anything at all. It's a small change with an outsized effect: the safeguard no longer depends on something outside this codebase's control.

"Capture is the easy half. Nothing in a memory graph makes the newer write right."

The sharper comment, verbatim: two clients writing facts that contradict each other is the failure worth designing for first, because nothing decides which one is true. Went back to the actual merge logic to check, rather than assert it wasn't a problem.

It wasn't just "recency wins" — it was worse than that. Same subject+predicate, different object, and the code picked existing[0] (not even guaranteed ordered by time) and overwrote it unconditionally. No timestamp comparison. There's a confidence field on every Relation that's exactly shaped for this — except it's never set to anything but its 1.0 default anywhere in the extraction path, and the merge logic never reads it. A field that looks like a mechanism and isn't one.

candidate_client = _client_name(candidate)
outdated_client = _client_name(outdated)
if candidate_client and outdated_client and candidate_client != outdated_client:
    # Two different MCP clients disagreeing, not the same session
    # correcting itself -- keep both instead of guessing.
    return MemoryAction(ActionType.ADD, relation=candidate, conflict_with=outdated.id)

The actual fix doesn't try to guess who's right — a genuinely hard, possibly unsolvable problem in general. It captures which MCP client wrote each fact automatically, read straight off the client's declared identity during its own initialize handshake, not a value a caller has to remember to pass. When an existing and an incoming relation name two different clients, that's not one session correcting itself — it's two clients disagreeing. Both relations are kept, and add_memory's response says so explicitly, instead of one silently overwriting the other. A same-client correction, or either side missing client identity (still the common case today), falls through to the original update-in-place behavior unchanged.

What's live now

Both fixes are in v0.2.0 — a minor version bump on purpose, because forget_all_memory's confirm requirement is a real breaking change for anything that called it the old way. Published to PyPI, the official MCP Registry, and deployed to production; 9 new or updated tests cover the cross-client-keeps-both case, the same-client-still-updates case, the unknown-client fallback, and both paths of the confirm parameter.

The pattern so far

Every fix on this list — the Glama deploy chain, the embedding startup block, these two — started with someone actually using or reading Spomory and hitting something real, not a feature planned in the abstract. That's the point of posting the actual source and inviting people to poke at it: the gaps that matter are the ones nobody thought to ask about internally.

v0.2.0 on GitHub, with the full changelog →
spomory 0.2.0 on PyPI →