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..c7595d2f 100644 --- a/tests_py/core/test_gist_extraction.py +++ b/tests_py/core/test_gist_extraction.py @@ -61,3 +61,228 @@ 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" + + +# ── 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 daf6f104..117e8091 100644 --- a/tests_py/handlers/test_forget.py +++ b/tests_py/handlers/test_forget.py @@ -274,3 +274,210 @@ 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 + + +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 + )