Skip to content

fix(mcp): validate anchor in MemoryHistory - #241

Merged
radimsem merged 4 commits into
devfrom
fix/history-validate-node-id
Jun 3, 2026
Merged

fix(mcp): validate anchor in MemoryHistory#241
radimsem merged 4 commits into
devfrom
fix/history-validate-node-id

Conversation

@radimsem

@radimsem radimsem commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Closes #201.

Summary

HandleHistory was the only anchor-taking tool in pkg/mcp/tools/ without input validation: a mistyped ID fell through to GetDiffsForNode and rendered as "no history for <anchor>" — a confidently-wrong answer indistinguishable from a real-but-empty trail.

This adds the empty-anchor guard + node-existence check that sibling handlers (forget, pin) use, adapted to the tool's nature:

  • Empty anchor → anchor is required (matches related.go's field-named message).
  • GetDiffsForNode runs first; GetNode is probed only when the trail is empty, to distinguish a live-but-unchanged node (no history for X) from an unknown ID (node_id not found: X).

Why trail-first (the important bit)

A naive copy of the sibling preflight — GetNode before everything — is a functional regression. qSelectDiffsForNode reads FROM diffs WHERE node_id = ?, keyed independently of the nodes table, so a forgotten node (removed from nodes, rem/prior diffs retained) still has a trail that history must surface — that's how you roll back a delete by re-writing the old payload. An unconditional preflight would return node_id not found for exactly those nodes. Caught in review (Codex), fixed, and locked down with a regression test.

Input Behavior
empty anchor anchor is required
unknown ID (no diffs, not in nodes) node_id not found: X — the bug, fixed
live node, no changes no history for X — legit empty preserved
forgotten node (diffs present, not in nodes) renders trail — regression avoided

Tests

  • TestHandleHistory — happy path now seeds a real node.
  • TestHandleHistory_EmptyAnchor, TestHandleHistory_MissingNode — the two new error paths.
  • TestHandleHistory_DeletedNodeKeepsTrail — seeds a rem diff for a node absent from nodes, asserts the trail still renders.

Verification

make build · gofmt -l clean · make test (full suite, 212 MCP tests) · make lint 0 issues. Read-tool discipline preserved (no OpMu, no snapshot, no boost change). No public-skill update required (tool name/input/return shape unchanged).

🤖 Generated with Claude Code

radimsem and others added 3 commits June 3, 2026 15:01
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: radimsem <me@radimsemerak.cz>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: radimsem <me@radimsemerak.cz>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: radimsem <me@radimsemerak.cz>
@radimsem radimsem self-assigned this Jun 3, 2026
@radimsem radimsem added the bug Something isn't working label Jun 3, 2026
@radimsem
radimsem merged commit 08d7c57 into dev Jun 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[enhancement] MemoryHistory should validate node ID like other tool handlers

1 participant