From 53146030131390b2b524a1ffb7ef94495af4d21b Mon Sep 17 00:00:00 2001 From: Clement Deust Date: Thu, 6 Aug 2026 14:47:23 +0200 Subject: [PATCH 1/2] fix(forget): delete across every substrate holding the content (#366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PRIVACY.md tells users "The `forget` tool deletes individual memories", but a hard delete issued a single DELETE FROM memories. Two substrates kept the content: - the raw full text of an oversized auto-capture, in its content-addressed artifact under ~/.claude/methodology/artifacts/ — artifact_store.py had no removal path at all, so the row's gist went and the complete original stayed readable on disk; - wiki claim events derived from the memory, which survive by design: pg_schema.py declares wiki.claim_events.memory_id ON DELETE SET NULL, so after the row goes the claim persists and can no longer be found by id. Root cause of the first was not the missing unlink but the missing contract: the artifact pointer format was duplicated as an f-string in hooks/post_tool_capture and handlers/backfill_helpers, so nothing could parse it back safely. format_artifact_pointer / parse_artifact_pointer now own that format in core, and both writers call it. Removal is reference-counted. store_artifact is content-addressed with dedup, so byte-identical captures map to ONE file; unlinking it for the first of two referrers would strip the survivor's content. artifact_gc counts referrers first and fails CLOSED — an unknown count keeps the file. Two orderings are load-bearing and each has its own test: claims must be deleted BEFORE the row (the FK nulls the link), and the artifact reference count must be taken AFTER it (or the memory counts itself and nothing is ever collected). Neither is discoverable from the happy path. soft=true keeps the artifact: a soft delete is recoverable by design, so its content must survive with it. Reported as artifact_deleted=false rather than omitted, so a caller can tell "kept on purpose" from "we forgot to look". PRIVACY.md now states the exact scope including both exceptions, and adds the artifacts directory to the user's controls. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 26 +++++ PRIVACY.md | 27 ++++- docs/mcp-tools.md | 2 +- mcp_server/core/gist_extraction.py | 50 +++++++++ mcp_server/handlers/backfill_helpers.py | 8 +- mcp_server/handlers/forget.py | 52 +++++++++- mcp_server/hooks/post_tool_capture.py | 3 +- mcp_server/infrastructure/artifact_gc.py | 110 ++++++++++++++++++++ tests_py/core/test_gist_extraction.py | 84 +++++++++++++++ tests_py/handlers/test_forget.py | 125 +++++++++++++++++++++++ 10 files changed, 480 insertions(+), 7 deletions(-) create mode 100644 mcp_server/infrastructure/artifact_gc.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c5005f8..a5e0ab88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,32 @@ adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Fixed + +- **`forget` now deletes across every substrate that holds the content + (issue #366).** PRIVACY.md told users "The `forget` tool deletes individual + memories", but a hard delete issued a single `DELETE FROM memories`: the raw + full text of an oversized auto-capture stayed on disk in its content-addressed + artifact (`artifact_store` had no removal path at all), and wiki claim events + derived from the memory survived with `memory_id` nulled by + `ON DELETE SET NULL`. A hard delete now removes the row, the derived claims, + and the artifact, reporting `artifact_deleted` / `claims_deleted` in its + result. Two behaviours are deliberate and asserted: an artifact shared by a + still-live memory is kept (content addressing dedups identical output to one + file, so unconditional removal would strip the survivor's content), and a + `soft=true` delete retains the artifact because it is recoverable by design. + Deletion ordering is load-bearing in both directions — claims must go before + the row (the FK nulls the link) and the artifact reference count must be taken + after it (or the memory counts itself) — and each ordering has its own test. + PRIVACY.md now states the exact scope, including both exceptions. +- **The artifact pointer format has one definition (issue #366).** It was + duplicated as an f-string in `hooks/post_tool_capture` and + `handlers/backfill_helpers`, so no reader could parse it safely. Both writers + now call `core.gist_extraction.format_artifact_pointer`, with + `parse_artifact_pointer` as its inverse; the round trip is tested for paths + containing spaces and for malformed pointers, which resolve to "no artifact" + rather than a guessed path. + ### Added - **Native Codex local plugin packaging.** A dedicated diff --git a/PRIVACY.md b/PRIVACY.md index 0207c3d2..7780a488 100644 --- a/PRIVACY.md +++ b/PRIVACY.md @@ -40,7 +40,10 @@ like any other memory (locally) — avoid doing so. connects to any database you did not configure. You own this data. Deleting the database file (or the relevant rows) permanently -removes it. The `forget` tool deletes individual memories. +removes it. The `forget` tool deletes individual memories: a hard delete +(the default) removes the memory row, any wiki claims derived from it, and the +raw-output artifact file its body pointed at — see *Your controls* below for the +exact scope, including the one case where an artifact is deliberately kept. ## What leaves your machine @@ -87,8 +90,28 @@ memories over time; this is a local maintenance operation, not a transfer. ## Your controls -- `forget` — delete a specific memory. +- `forget` — delete a specific memory. A **hard** delete (the default) removes, + across every substrate that holds the content: + 1. the memory row itself; + 2. the wiki claim events derived from that memory; + 3. the raw-output **artifact** file the memory body pointed at, under + `~/.claude/methodology/artifacts/` — an oversized auto-capture keeps only a + short gist in the row and the full raw text in that file, so deleting the + row alone would leave your content on disk. + + Two deliberate exceptions, both reported in the tool's result so you can see + which applied: + - **Artifacts are content-addressed and shared.** If a second memory still + points at the same artifact (identical captured output dedups to one file), + the file is kept until the last memory referring to it is forgotten — + otherwise the surviving memory would lose its content. `artifact_deleted` + is `false` in that case. + - **`soft=true` keeps everything but the row's visibility.** A soft delete is + recoverable by design (it sets `is_stale` and `heat=0`), so the artifact is + deliberately retained. Use a hard delete when you want the content gone. - Delete `~/.claude/methodology/memory.db` — remove all SQLite-stored data. +- Delete `~/.claude/methodology/artifacts/` — remove all raw-output artifacts, + including any left by soft deletes or still shared between memories. - For PostgreSQL, manage retention directly in your database. ## Contact diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index abe9f528..c9d532e1 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -32,7 +32,7 @@ upstream MCP server is configured (55 total with both present). | `narrative` | Generate project narrative from stored memories | <500ms | | `memory_stats` | Memory system diagnostics | <50ms | | `import_sessions` | Import conversation history into memory store | varies | -| `forget` | Hard/soft delete with is_protected guard | <50ms | +| `forget` | Hard/soft delete with is_protected guard; hard delete is cross-substrate (row + derived wiki claims + unreferenced raw artifact) | <50ms | | `validate_memory` | Validate memories against filesystem state | <500ms | | `rate_memory` | Useful/not-useful feedback → metamemory confidence | <50ms | | `seed_project` | 5-stage codebase bootstrap | varies | diff --git a/mcp_server/core/gist_extraction.py b/mcp_server/core/gist_extraction.py index ebf55ef3..4e3c9249 100644 --- a/mcp_server/core/gist_extraction.py +++ b/mcp_server/core/gist_extraction.py @@ -22,6 +22,8 @@ from __future__ import annotations +import re + # source: measured p90 curated memory length, production DB 2026-06-10 # (3,041 chars, n=68) — see docs/provenance/bounded-io-phase2-design.md. Makes # auto-captures size-comparable to curated content, removing the @@ -67,6 +69,54 @@ ] +# ── Artifact pointer: ONE definition, used by every writer and the reader ── +# +# The pointer line is the only link from a memory body to its full raw content +# on disk. It was previously built by two duplicated f-strings: one in +# hooks/post_tool_capture._gist_or_full, one in handlers/backfill_helpers. That +# meant no reader could parse it safely — a drift in either writer would break +# the parse silently. The forget handler must find and remove that artifact, +# see issue #366, so the format is defined once here, in the layer both +# writers already import. +_ARTIFACT_LABEL = "**Artifact:**" + +# Matches the line format emitted by format_artifact_pointer. The path group is +# non-greedy and backtick-delimited, so a path containing spaces is preserved +# and a trailing "(N chars ...)" suffix is never swallowed into it. +_ARTIFACT_POINTER_RE = re.compile( + r"\*\*Artifact:\*\*\s+`(?P[^`]+)`", +) + + +def format_artifact_pointer(path: str, char_count: int) -> str: + """Render the pointer line linking a memory body to its raw artifact. + + Pre: ``path`` is the artifact path as a string; ``char_count`` is the length + of the full raw output the artifact holds. + Post: returns a single line that ``parse_artifact_pointer`` recovers + ``path`` from. This is the ONLY place the format is defined. + """ + return f"{_ARTIFACT_LABEL} `{path}` ({char_count} chars full output)" + + +def parse_artifact_pointer(content: str) -> str | None: + """Recover the artifact path from a memory body, or None when absent. + + Pre: ``content`` is a memory body (may be empty, may contain no pointer). + Post: returns the path string from the FIRST pointer line produced by + ``format_artifact_pointer``, else None. Never raises — a body with no + pointer, or a malformed one, yields None so callers treat "no artifact" and + "unparseable" identically (both mean: nothing safe to delete). + """ + if not content: + return None + match = _ARTIFACT_POINTER_RE.search(content) + if match is None: + return None + path = match.group("path").strip() + return path or None + + def needs_gist(output: str) -> bool: """True when output exceeds the gist budget and should be artifact-backed. diff --git a/mcp_server/handlers/backfill_helpers.py b/mcp_server/handlers/backfill_helpers.py index 5f21cc30..f488152f 100644 --- a/mcp_server/handlers/backfill_helpers.py +++ b/mcp_server/handlers/backfill_helpers.py @@ -12,7 +12,11 @@ from mcp_server.infrastructure.memory_store import MemoryStore from mcp_server.observability import silent_failure from mcp_server.shared.domain_mapping import resolve_domain -from mcp_server.core.gist_extraction import extract_gist, needs_gist +from mcp_server.core.gist_extraction import ( + extract_gist, + format_artifact_pointer, + needs_gist, +) from mcp_server.infrastructure.artifact_store import store_artifact # Core concept keywords for entity linking @@ -195,7 +199,7 @@ def gist_oversized_content(content: str) -> str: except Exception as exc: # noqa: BLE001 — mechanism boundary; failure is observable via silent_failure silent_failure.note("backfill.artifact_store", exc) return content - pointer = f"**Artifact:** `{path}` ({len(content)} chars full output)" + pointer = format_artifact_pointer(str(path), len(content)) return f"{extract_gist(content)}\n\n{pointer}" diff --git a/mcp_server/handlers/forget.py b/mcp_server/handlers/forget.py index 0f584ce3..81da41cd 100644 --- a/mcp_server/handlers/forget.py +++ b/mcp_server/handlers/forget.py @@ -6,13 +6,19 @@ from __future__ import annotations +import logging from typing import Any +from mcp_server.core.gist_extraction import parse_artifact_pointer +from mcp_server.infrastructure.artifact_gc import delete_artifact_if_unreferenced from mcp_server.infrastructure.memory_config import get_memory_settings from mcp_server.infrastructure.memory_store import MemoryStore, get_shared_store +from mcp_server.infrastructure.pg_store_wiki import delete_claims_for_memory from mcp_server.handlers._tool_meta import DESTRUCTIVE from mcp_server.handlers._telemetry_wrap import instrument +logger = logging.getLogger(__name__) + # ── Schema ──────────────────────────────────────────────────────────────── schema = { @@ -109,18 +115,62 @@ async def _handler_impl(args: dict[str, Any] | None = None) -> dict[str, Any]: "method": "soft", "memory_id": memory_id, "content_preview": mem["content"][:80], + # Soft delete is recoverable, so the raw content must survive with + # it. Reported explicitly rather than omitted so a caller can tell + # "kept on purpose" from "we forgot to look" (issue #366). + "artifact_deleted": False, } - # Hard delete + # ── Hard delete: cross-substrate, and the two orderings below are + # load-bearing (issue #366). + # + # The artifact path must be read BEFORE the row goes — it lives in the + # memory body, which is about to be unreachable. + artifact_path = parse_artifact_pointer(mem.get("content") or "") + + # Wiki claims must go BEFORE the row: wiki.claim_events.memory_id is + # ON DELETE SET NULL (infrastructure/pg_schema.py), so once the memory row + # is deleted the claim survives with a NULL link and can no longer be + # found by memory_id at all. + claims_deleted = _delete_derived_claims(store, memory_id) + deleted = store.delete_memory(memory_id) + + # The reference check must run AFTER the row is gone, or the memory being + # forgotten counts itself as a live referrer and nothing is ever collected. + artifact_deleted = ( + delete_artifact_if_unreferenced(store._conn, artifact_path) + if deleted + else False + ) + return { "deleted": deleted, "method": "hard", "memory_id": memory_id, "content_preview": mem["content"][:80], + "artifact_deleted": artifact_deleted, + "claims_deleted": claims_deleted, } +def _delete_derived_claims(store: MemoryStore, memory_id: int) -> int: + """Remove wiki claim_events derived from ``memory_id``; return the count. + + Never raises: a wiki table that is absent or unreachable (SQLite installs + without the wiki schema, a degraded connection) must not block the deletion + the user asked for. Returns 0 when nothing was removed, and logs the reason + so a silent miss is still observable. + """ + try: + return int(delete_claims_for_memory(store._conn, memory_id)) + except Exception as exc: # noqa: BLE001 — mechanism boundary, see docstring + logger.warning( + "wiki claims for memory %s could not be removed: %s", memory_id, exc + ) + return 0 + + # Telemetry-instrumented public entry. Records latency / byte volume # / result count per call (Popper C6 read/write ratio audit). handler = instrument("forget", _handler_impl, result_count_key=None) diff --git a/mcp_server/hooks/post_tool_capture.py b/mcp_server/hooks/post_tool_capture.py index 545e6556..101d85c7 100644 --- a/mcp_server/hooks/post_tool_capture.py +++ b/mcp_server/hooks/post_tool_capture.py @@ -22,6 +22,7 @@ from mcp_server.core.gist_extraction import ( HIGH_VALUE_PATTERNS, extract_gist, + format_artifact_pointer, needs_gist, ) from mcp_server.shared.redaction import scrub_secrets @@ -195,7 +196,7 @@ def _gist_or_full(output: str) -> tuple[str, str | None]: except Exception as exc: # noqa: BLE001 — hook boundary — failure is logged to the hook log; the hook stays non-fatal _log(f"artifact write failed (non-fatal, full output kept): {exc}") return output, None - pointer = f"**Artifact:** `{path}` ({len(output)} chars full output)" + pointer = format_artifact_pointer(str(path), len(output)) return extract_gist(output), pointer diff --git a/mcp_server/infrastructure/artifact_gc.py b/mcp_server/infrastructure/artifact_gc.py new file mode 100644 index 00000000..7b52f948 --- /dev/null +++ b/mcp_server/infrastructure/artifact_gc.py @@ -0,0 +1,110 @@ +"""Reference-counted removal of raw-output artifacts. + +`artifact_store` writes content-addressed artifacts and never removes them. +That is safe while nothing deletes memories, but `forget` claims — in +PRIVACY.md, to users — that it deletes a memory. A gist+pointer memory keeps +its FULL raw text in the artifact, so deleting only the row leaves the content +readable on disk (issue #366). + +Removal cannot be unconditional. `store_artifact` is content-addressed with +dedup: two captures of byte-identical output map to ONE file. Deleting that +file when the first of two referring memories is forgotten would leave the +second memory's pointer dangling at a path that no longer exists. So this +module answers "is anyone still referring to this artifact?" before unlinking. + +Separate module from `artifact_store` on purpose: that one owns filesystem +writes and has no database dependency. Garbage collection needs both the store +and the filesystem, and mixing them into the writer would give it two reasons +to change (SRP). +""" + +from __future__ import annotations + +import logging +from pathlib import Path +from typing import TYPE_CHECKING + +from mcp_server.infrastructure.row_factory import DICT_ROW + +if TYPE_CHECKING: + from mcp_server.infrastructure.db_types import StoreConnection + +logger = logging.getLogger(__name__) + + +def count_artifact_references(conn: StoreConnection, artifact_path: str) -> int: + """Number of memories whose body still points at ``artifact_path``. + + Pre: ``conn`` is a live store connection (PgMemoryStore's psycopg + connection or the SQLite `PsycopgCompatConnection`, which translates the + ``%s`` placeholder); ``artifact_path`` is a non-empty path string. + Post: returns the count of rows in ``memories`` whose ``content`` contains + the path. Substring match is the correct test here because the pointer line + embeds the path verbatim and is the only place a memory body carries it. + + Uses ``COUNT(*) AS c`` + ``row["c"]`` with ``row_factory=DICT_ROW``, the + shared cross-backend scalar convention (cf. count_relationships / + count_entities): the SQLite compat cursor yields mapping rows, so reading + by position raises KeyError there. + + Raises whatever the driver raises — a failure to COUNT must NOT be read as + "zero references", which would delete a still-referenced artifact. The + caller decides how to degrade; see ``delete_artifact_if_unreferenced``. + """ + with conn.cursor(row_factory=DICT_ROW) as cur: + cur.execute( + "SELECT COUNT(*) AS c FROM memories WHERE content LIKE %s", + (f"%{artifact_path}%",), + ) + row = cur.fetchone() + return int(row["c"]) if row else 0 + + +def delete_artifact_if_unreferenced( + conn: StoreConnection, artifact_path: str | None +) -> bool: + """Unlink ``artifact_path`` when no memory refers to it any more. + + Pre: the referring memory row has ALREADY been deleted — otherwise it + counts itself and the artifact is never collected. ``artifact_path`` may be + None (memory had no artifact), in which case this is a no-op. + Post: returns True only when the file was actually unlinked. Returns False + when there was no artifact, when another memory still refers to it, when + the file is already gone, or when the reference count could not be + established. Never raises: a failure to collect an artifact must not fail + the deletion of the memory the user asked to forget, and every non-removal + path logs why. + """ + if not artifact_path: + return False + + try: + references = count_artifact_references(conn, artifact_path) + except Exception as exc: # noqa: BLE001 — mechanism boundary, see docstring + # Fail CLOSED: an unknown reference count must not authorize a delete. + logger.warning( + "artifact reference count failed for %s — keeping the file: %s", + artifact_path, + exc, + ) + return False + + if references > 0: + logger.info( + "artifact %s kept — still referenced by %d memory/memories", + artifact_path, + references, + ) + return False + + path = Path(artifact_path) + try: + path.unlink() + except FileNotFoundError: + logger.info("artifact %s already absent — nothing to remove", artifact_path) + return False + except OSError as exc: + logger.warning("artifact %s could not be removed: %s", artifact_path, exc) + return False + logger.info("artifact %s removed (last referrer forgotten)", artifact_path) + return True diff --git a/tests_py/core/test_gist_extraction.py b/tests_py/core/test_gist_extraction.py index 7aa56bca..a164538f 100644 --- a/tests_py/core/test_gist_extraction.py +++ b/tests_py/core/test_gist_extraction.py @@ -61,3 +61,87 @@ def test_custom_budget(): big = "\n".join(f"line {i}" for i in range(500)) gist = extract_gist(big, budget=300) assert len(gist) <= 300 + _OVERHEAD + + +# ── Artifact pointer round-trip (issue #366) ────────────────────────────── +# +# The pointer is the only link from a memory body to its raw content on disk, +# and `forget` deletes that artifact by parsing it back out. Format and parse +# are therefore one contract: anything format_artifact_pointer emits must be +# recoverable by parse_artifact_pointer, or forget silently leaves content +# behind. These pin the round-trip, including the paths that break naive +# regexes. + + +class TestArtifactPointerRoundTrip: + def test_round_trip_recovers_the_path(self): + from mcp_server.core.gist_extraction import ( + format_artifact_pointer, + parse_artifact_pointer, + ) + + path = "/home/u/.claude/methodology/artifacts/2026-08/deadbeefcafe0001.md" + line = format_artifact_pointer(path, 61234) + assert parse_artifact_pointer(line) == path + + def test_round_trip_inside_a_full_memory_body(self): + """Production shape: gist text, blank line, then the pointer.""" + from mcp_server.core.gist_extraction import ( + format_artifact_pointer, + parse_artifact_pointer, + ) + + path = "/tmp/artifacts/2026-08/abc123.md" + body = "some gist text\n… [gist: 10 of 99 chars] …\n\n" + ( + format_artifact_pointer(path, 99) + ) + assert parse_artifact_pointer(body) == path + + def test_path_containing_spaces_survives(self): + """A macOS home or temp dir can contain spaces; the path must not clip.""" + from mcp_server.core.gist_extraction import ( + format_artifact_pointer, + parse_artifact_pointer, + ) + + path = "/Users/some user/Library/Application Support/artifacts/2026-08/a1.md" + assert parse_artifact_pointer(format_artifact_pointer(path, 5)) == path + + def test_char_count_suffix_is_not_absorbed_into_the_path(self): + from mcp_server.core.gist_extraction import ( + format_artifact_pointer, + parse_artifact_pointer, + ) + + path = "/a/b/c.md" + line = format_artifact_pointer(path, 4242) + assert "4242 chars" in line, "precondition: the suffix must be present" + assert parse_artifact_pointer(line) == path + + def test_absent_pointer_returns_none(self): + from mcp_server.core.gist_extraction import parse_artifact_pointer + + assert parse_artifact_pointer("a plain memory with no artifact") is None + + def test_empty_content_returns_none(self): + from mcp_server.core.gist_extraction import parse_artifact_pointer + + assert parse_artifact_pointer("") is None + + def test_malformed_pointer_returns_none_rather_than_a_bad_path(self): + """Unparseable must mean "nothing safe to delete", never a guess.""" + from mcp_server.core.gist_extraction import parse_artifact_pointer + + assert parse_artifact_pointer("**Artifact:** no backticks here") is None + assert parse_artifact_pointer("**Artifact:** ``") is None + assert parse_artifact_pointer("**Artifact:** ` `") is None + + def test_first_pointer_wins_when_a_body_carries_two(self): + from mcp_server.core.gist_extraction import ( + format_artifact_pointer, + parse_artifact_pointer, + ) + + first = format_artifact_pointer("/first.md", 1) + second = format_artifact_pointer("/second.md", 2) + assert parse_artifact_pointer(f"{first}\n{second}") == "/first.md" diff --git a/tests_py/handlers/test_forget.py b/tests_py/handlers/test_forget.py index daf6f104..37e72f99 100644 --- a/tests_py/handlers/test_forget.py +++ b/tests_py/handlers/test_forget.py @@ -274,3 +274,128 @@ async def test_protected_soft_delete_refused_without_force(self): result = await forget_handler({"memory_id": mid, "soft": True}) assert result["deleted"] is False assert "protected" in result["reason"].lower() + + +# ── Cross-substrate deletion (issue #366) ───────────────────────────────── +# +# PRIVACY.md tells users "The `forget` tool deletes individual memories." +# A gist+pointer memory keeps its FULL raw text in a content-addressed +# filesystem artifact (core/gist_extraction.py -> infrastructure/ +# artifact_store.py). Deleting only the row leaves that content readable on +# disk, so the published control does not do what it says. These tests pin the +# contract the policy already claims. +# +# Two orderings are load-bearing and asserted separately below: +# - wiki claims must go BEFORE the row (the FK is ON DELETE SET NULL, so +# after the row is gone the claim no longer matches memory_id) +# - the artifact reference check must run AFTER the row is gone (otherwise +# the memory being forgotten counts itself as a live reference) + + +def _seed_artifact_backed_memory(raw: str) -> tuple[int, "object"]: + """Store ``raw`` as an artifact and seed a memory whose body points at it. + + Returns (memory_id, artifact_path). Mirrors the production shape built by + hooks/post_tool_capture._gist_or_full and handlers/backfill_helpers. + """ + from mcp_server.core.gist_extraction import extract_gist, format_artifact_pointer + from mcp_server.infrastructure.artifact_store import store_artifact + + path = store_artifact(raw) + body = f"{extract_gist(raw)}\n\n{format_artifact_pointer(str(path), len(raw))}" + return _seed_memory(body), path + + +class TestForgetDeletesArtifact: + def test_hard_delete_removes_the_artifact_file(self): + raw = "SECRET-CANARY-A " + ("x" * 60_000) + mid, path = _seed_artifact_backed_memory(raw) + assert path.exists(), "precondition: artifact must exist before forget" + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result["deleted"] is True + assert not path.exists(), ( + "artifact still on disk after hard delete — PRIVACY.md claims the " + "memory is deleted, but the full raw content survives" + ) + + def test_hard_delete_reports_artifact_removal(self): + """The signal itself is asserted, not just the side effect (§13 F1).""" + raw = "SECRET-CANARY-B " + ("y" * 60_000) + mid, _path = _seed_artifact_backed_memory(raw) + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result.get("artifact_deleted") is True, ( + f"forget must report artifact removal in its result; got {result!r}" + ) + + def test_soft_delete_keeps_the_artifact(self): + """Soft delete is recoverable, so its content must survive.""" + raw = "SECRET-CANARY-C " + ("z" * 60_000) + mid, path = _seed_artifact_backed_memory(raw) + + result = pytest.importorskip("asyncio").run( + forget_handler({"memory_id": mid, "soft": True}) + ) + + assert result["method"] == "soft" + assert path.exists(), "soft delete must NOT remove the artifact" + assert result.get("artifact_deleted") is False + + def test_refused_delete_keeps_the_artifact(self): + """A protected memory is not deleted, so nothing may be removed.""" + from mcp_server.core.gist_extraction import ( + extract_gist, + format_artifact_pointer, + ) + from mcp_server.infrastructure.artifact_store import store_artifact + + raw = "SECRET-CANARY-D " + ("w" * 60_000) + path = store_artifact(raw) + store = _get_forget_store() + body = f"{extract_gist(raw)}\n\n{format_artifact_pointer(str(path), len(raw))}" + mid = store.insert_memory({"content": body, "is_protected": True}) + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result["deleted"] is False + assert path.exists(), "refused delete must not touch the artifact" + + def test_shared_artifact_survives_while_another_memory_references_it(self): + """Content addressing dedups: two identical outputs share ONE file. + + Deleting the artifact for one memory would corrupt the other, so the + file must survive until the last referrer is gone. + """ + raw = "SECRET-CANARY-E " + ("q" * 60_000) + mid_a, path_a = _seed_artifact_backed_memory(raw) + mid_b, path_b = _seed_artifact_backed_memory(raw) + assert path_a == path_b, ( + "precondition: identical content must dedup to one path" + ) + + first = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid_a})) + assert first["deleted"] is True + assert path_a.exists(), ( + "artifact removed while a second memory still references it — " + "the surviving memory's pointer now dangles" + ) + assert first.get("artifact_deleted") is False + + second = pytest.importorskip("asyncio").run( + forget_handler({"memory_id": mid_b}) + ) + assert second["deleted"] is True + assert not path_a.exists(), "last referrer gone — artifact must be removed" + assert second.get("artifact_deleted") is True + + def test_memory_without_artifact_reports_false_not_none(self): + """Negative case: a plain memory has no artifact to remove.""" + mid = _seed_memory("plain memory, no artifact pointer") + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result["deleted"] is True + assert result.get("artifact_deleted") is False From 39f476ec59c4618ea496bd1394068d80568d1f9c Mon Sep 17 00:00:00 2001 From: Clement Deust Date: Thu, 6 Aug 2026 15:43:22 +0200 Subject: [PATCH 2/2] =?UTF-8?q?test(forget):=20close=20the=20mutation=20ga?= =?UTF-8?q?ps=20the=20scoped=20=C2=A712=20run=20surfaced=20(#366)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scoping mutmut to the three changed files reported 8 genuine survivors, 6 of them in the code this series added. Each was a real gap, not an equivalent: forget/_delete_derived_claims: no test proved claims are actually deleted — passing the wrong arity still returned 0 through the mechanism boundary, so half of #366's purpose was unasserted. Now seeds a wiki claim_event, forgets the memory, and asserts both the reported count and that the claim is gone. forget/_handler_impl: the claims_deleted key was never read by a test, so renaming it survived. Asserted, alongside the zero case. artifact_gc/delete_artifact_if_unreferenced: the already-absent branch logged a path nothing checked. §13 F1 wants the signal itself asserted, so the test now pins that the message names the path it expected. Two pre-existing gaps in the same file, in scope under §14 because the run that saw them was this change's own verification: _fill_signal's budget check and char charge. Both inflate the gist past its stated bound, which defeats the reason the budget exists (an oversized capture must stop outranking curated memories). Bounded by a measured assertion plus a check that the elision marker's kept count agrees with the budget actually consumed. extract_gist's elision total and _fill_tail's direction/skip, from the first round: a reversed step makes the tail range EMPTY so the end of every dump vanishes, and break-instead-of-continue truncates the tail whenever a signal line sits inside the tail window. Scoped mutation run now reports 0 surviving mutants, 347/347. Co-Authored-By: Claude Opus 5 (1M context) --- tests_py/core/test_gist_extraction.py | 141 ++++++++++++++++++++++++++ tests_py/handlers/test_forget.py | 82 +++++++++++++++ 2 files changed, 223 insertions(+) diff --git a/tests_py/core/test_gist_extraction.py b/tests_py/core/test_gist_extraction.py index a164538f..c7595d2f 100644 --- a/tests_py/core/test_gist_extraction.py +++ b/tests_py/core/test_gist_extraction.py @@ -145,3 +145,144 @@ def test_first_pointer_wins_when_a_body_carries_two(self): first = format_artifact_pointer("/first.md", 1) second = format_artifact_pointer("/second.md", 2) assert parse_artifact_pointer(f"{first}\n{second}") == "/first.md" + + +# ── Gist composition invariants (mutation-surfaced, issue #366) ─────────── +# +# Scoping the §12 mutation run to this file surfaced three surviving mutants in +# extract_gist/_fill_tail — pre-existing gaps, each a behaviour a reader of a +# gist would notice: +# - _elision(used, len(output)) -> _elision(used, None): the marker reports +# "of None chars", losing how much was dropped +# - the tail loop's step -1 -> +1: the range becomes EMPTY, so the tail +# silently disappears from every gist +# - the tail loop's `continue` -> `break` on an already-taken index: the tail +# stops early whenever a signal line sits inside the tail window +# The gist is the only thing a reader sees for an oversized capture, so each of +# these is a silent information loss. Lines below are 9 chars so the budget +# arithmetic is exact and the boundaries land where the comments say. + + +def _nine(tag: str) -> str: + """A 9-char line tagged for identification, free of HIGH_VALUE_PATTERNS.""" + return f"{tag}aaaaaa"[:9].ljust(9, "a") + + +class TestGistCompositionInvariants: + def test_elision_marker_reports_the_true_total_length(self): + """The marker is the reader's only record of how much was dropped.""" + from mcp_server.core.gist_extraction import GIST_BUDGET, extract_gist + + output = "\n".join(f"line {i} " + "z" * 40 for i in range(300)) + assert len(output) > GIST_BUDGET, "precondition: must exceed the budget" + + gist = extract_gist(output) + + assert f"of {len(output)} chars" in gist, ( + "elision marker must state the true total char count; got: " + + next((ln for ln in gist.splitlines() if "gist:" in ln), "") + ) + + def test_the_tail_of_the_output_survives_into_the_gist(self): + """A tail loop that collects nothing would drop the end of every dump.""" + from mcp_server.core.gist_extraction import GIST_BUDGET, extract_gist + + body = "\n".join(f"middle {i} " + "m" * 40 for i in range(300)) + output = f"FIRSTLINEMARKER\n{body}\nLASTLINEMARKER" + assert len(output) > GIST_BUDGET, "precondition: must exceed the budget" + + gist = extract_gist(output) + + assert "LASTLINEMARKER" in gist, ( + "the final line must reach the gist — an empty tail fill drops the " + "end of the output silently" + ) + + def test_a_taken_line_inside_the_tail_window_is_skipped_not_terminal(self): + """An already-taken signal line must not stop the tail fill. + + Layout (9-char lines, budget 70 so the boundaries are exact): + idx 0-1 taken by the head fill (limit 0.4*70 = 28 -> 2 lines, used 20) + idx 6 taken by the signal fill (limit 0.6*70 = 42 -> used 30) + idx 8,7 taken by the tail fill (used 50) + idx 6 ALREADY TAKEN -> must `continue`, not `break` + idx 5,4 taken by the tail fill (used 70, exactly at budget) + With `break`, PL5 and PL4 never appear. + """ + from mcp_server.core.gist_extraction import extract_gist + + lines = [ + _nine("HD0"), + _nine("HD1"), + _nine("PL2"), + _nine("PL3"), + _nine("PL4"), + _nine("PL5"), + "err6error", # the only signal line — 9 chars, contains "error" + _nine("PL7"), + _nine("PL8"), + ] + output = "\n".join(lines) + budget = 70 + assert len(output) > budget, "precondition: must exceed the budget" + + gist = extract_gist(output, budget=budget) + + assert "err6error" in gist, "precondition: the signal line must be taken" + assert _nine("PL8") in gist and _nine("PL7") in gist, ( + "precondition: the tail fill must reach the lines after the signal line" + ) + assert _nine("PL5") in gist, ( + "the tail fill must SKIP the already-taken signal line and keep " + "walking backwards; breaking there truncates the tail" + ) + assert _nine("PL4") in gist, ( + "the tail fill must continue past the taken line to the budget" + ) + + def test_signal_fill_budget_accounting_bounds_the_gist(self): + """The signal pass must charge each line against the budget correctly. + + Two mutation-surfaced gaps live here: an inverted budget check (which + stops tripping, so the signal pass takes every remaining line) and an + off-by-two char charge (which under-counts, so the pass takes more lines + than the budget allows and the tail then over-fills too). Both inflate + the gist past its stated bound, which is the whole point of the budget: + an oversized capture must stop outranking curated memories. + + # source: measured on this input 2026-08-06 — correct behaviour yields + # 3056 chars for a 3599-char all-signal input at GIST_BUDGET=3000 + # (budget + one elision marker). The bound below allows 150 chars of + # headroom over the budget; broken accounting exceeds it by 250+. + """ + from mcp_server.core.gist_extraction import GIST_BUDGET, extract_gist + + # Every line is a signal line, so the signal pass does the real work. + output = "\n".join(["error"] * 600) + assert len(output) > GIST_BUDGET, "precondition: must exceed the budget" + + gist = extract_gist(output) + + assert len(gist) <= GIST_BUDGET + 150, ( + f"gist is {len(gist)} chars for a {GIST_BUDGET}-char budget — the " + "signal pass is not charging lines against the budget correctly" + ) + + def test_elision_marker_kept_count_matches_the_budget_actually_used(self): + """`kept` is the reader's record of how much was retained. + + Under-counting in the fill passes makes this number disagree with the + content the gist actually carries. + """ + from mcp_server.core.gist_extraction import GIST_BUDGET, extract_gist + + output = "\n".join(["error"] * 600) + gist = extract_gist(output) + + marker = next(ln for ln in gist.splitlines() if "gist:" in ln) + kept = int(marker.split("gist:")[1].split("of")[0].strip()) + assert kept <= GIST_BUDGET, f"kept={kept} exceeds the budget" + assert kept >= GIST_BUDGET - 60, ( + f"kept={kept} is far below the budget — the fill passes are " + "under-charging, so more content was taken than reported" + ) diff --git a/tests_py/handlers/test_forget.py b/tests_py/handlers/test_forget.py index 37e72f99..117e8091 100644 --- a/tests_py/handlers/test_forget.py +++ b/tests_py/handlers/test_forget.py @@ -399,3 +399,85 @@ def test_memory_without_artifact_reports_false_not_none(self): assert result["deleted"] is True assert result.get("artifact_deleted") is False + + +class TestForgetDeletesDerivedClaims: + """The wiki half of issue #366. + + wiki.claim_events.memory_id is ON DELETE SET NULL, so deleting the row + leaves the claim behind with its link nulled — the claim text, which can + carry the same content, stays searchable and unattributable. forget must + remove the claims BEFORE the row, while they can still be found by id. + """ + + @staticmethod + def _seed_claim(memory_id: int, text: str) -> int: + from mcp_server.infrastructure.pg_store_wiki import insert_claim_events + + store = _get_forget_store() + ids = insert_claim_events( + store._conn, + [{"text": text, "claim_type": "observation", "memory_id": memory_id}], + ) + assert ids, "precondition: the claim must be inserted" + return ids[0] + + @staticmethod + def _claims_for(memory_id: int) -> list: + from mcp_server.infrastructure.pg_store_wiki import get_claims_for_memory + + return get_claims_for_memory(_get_forget_store()._conn, memory_id) + + def test_hard_delete_removes_derived_claims_and_reports_the_count(self): + mid = _seed_memory("memory with a derived wiki claim") + self._seed_claim(mid, "CLAIM-CANARY derived from the memory") + assert len(self._claims_for(mid)) == 1, "precondition: claim must exist" + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result["deleted"] is True + assert result.get("claims_deleted") == 1, ( + "forget must report how many derived claims it removed; got " + f"{result.get('claims_deleted')!r}" + ) + assert self._claims_for(mid) == [], ( + "derived claims survived the delete — ON DELETE SET NULL only nulls " + "the link, it does not remove the claim" + ) + + def test_memory_with_no_claims_reports_zero(self): + mid = _seed_memory("memory with no derived claims") + + result = pytest.importorskip("asyncio").run(forget_handler({"memory_id": mid})) + + assert result["deleted"] is True + assert result.get("claims_deleted") == 0 + + +class TestForgetArtifactSignals: + def test_already_absent_artifact_logs_the_path_it_expected(self, caplog): + """§13 F1: the degraded path must emit an actionable signal. + + A pointer whose file is already gone must say WHICH path was missing — + a message without the path cannot be acted on. + """ + import logging + + raw = "SECRET-CANARY-F " + ("v" * 60_000) + mid, path = _seed_artifact_backed_memory(raw) + path.unlink() + assert not path.exists(), "precondition: artifact removed out of band" + + with caplog.at_level( + logging.INFO, logger="mcp_server.infrastructure.artifact_gc" + ): + result = pytest.importorskip("asyncio").run( + forget_handler({"memory_id": mid}) + ) + + assert result["deleted"] is True + assert result.get("artifact_deleted") is False + assert str(path) in caplog.text, ( + "the already-absent signal must name the path it expected; got: " + + caplog.text + )