feat(snippets): un-merge — reverse one source out of a merged survivor

Closes the last of milestone #232. The task said to settle the design
before coding; here is what was settled and why.

THE HAZARD. Restoring a merged-in source from the trash brought the record
back but never stripped its locations off the survivor, so both claimed
the same call sites and the reverse lookup read the duplicate claims as
real. Subtracting blindly is not a fix: a location can arrive from a
source AND genuinely be the survivor's own, and _normalize_locations dedups
them into one, so blind subtraction would strip a call site the survivor
owns. Same problem defeated partial un-merge — `merged_from` recorded ids,
not which locations came from which source.

THE ANSWER. Record per-source attribution AT MERGE TIME, where it is known
exactly: each entry keeps only what that source ADDED, computed
incrementally as sources fold in. Anything the survivor already had, or an
earlier source already brought, is attributed to nobody. Both open
questions fall out of that one change — partial un-merge is exact, and a
survivor-owned location can never be stripped, because it was never
attributed in the first place.

The shape moved from [id] to [{id, locations, tags}]. Free to do: the
corpus holds one snippet and zero merges, so there is no legacy data (rule
#22). A bare int still normalizes to {"id": n} — not legacy tolerance, but
because snippet_fields falls back to PARSING THE BODY when a row has no
`data`, and the body's provenance line can only carry ids. Such an entry
shows history and refuses un-merge with a reason rather than guessing.

WHICH SURFACE. Neither option in the task, quite. Making trash-restore
notice the merge would teach the generic trash path snippet semantics for
one record type. Instead un-merge OWNS the restore: one operation, one
authorization check, trash stays ignorant. Restoring by hand is still
allowed and still leaves both records claiming the same places — so
un-merge treats an already-alive source as the normal case and goes
straight to the subtraction that repairs it. That is the state that
motivated the feature, not an error.

Adds trash.restore_entity(user_id, type, id) — the missing inverse of
delete(), which returns a batch id callers don't keep. Restores the whole
batch, since the batch is the entity plus its cascade.

Refs #2165

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs

tests/test_mcp_tool_snippets.py

@@ -229,5 +229,5 @@ def test_register_attaches_all_tools():
     assert set(names) == {
         "list_snippets", "create_snippet", "get_snippet", "update_snippet",
         "delete_snippet", "merge_snippets", "verify_snippet",
-        "find_duplicate_snippets",
+        "find_duplicate_snippets", "unmerge_snippet",
     }

tests/test_routes_snippets.py

@@ -21,6 +21,7 @@ def test_snippet_handlers_callable():
         "list_snippets_route", "create_snippet_route", "get_snippet_route",
         "update_snippet_route", "delete_snippet_route", "merge_snippet_route",
         "verify_snippet_route", "duplicate_snippets_route",
+        "unmerge_snippet_route",
     ):
         assert callable(getattr(routes, name))
 
@@ -31,6 +32,7 @@ def test_service_functions_take_user_id():
     for fn_name in (
         "create_snippet", "list_snippets", "get_snippet", "update_snippet",
         "delete_snippet", "merge_snippets", "record_verification",
+        "unmerge_snippet",
     ):
         fn = getattr(svc, fn_name)
         assert callable(fn)
@@ -46,7 +48,8 @@ def test_agent_and_web_surfaces_stay_at_parity():
     from scribe.routes import snippets as routes
 
     # Every write verb the web surface offers, the agent surface offers too.
-    for verb in ("create", "get", "list", "update", "delete", "merge", "verify"):
+    for verb in ("create", "get", "list", "update", "delete", "merge", "verify",
+                 "unmerge"):
         assert callable(getattr(tools, f"{verb}_snippets", None) or
                         getattr(tools, f"{verb}_snippet", None)), f"MCP lacks {verb}"
 

tests/test_services_snippets.py

@@ -148,7 +148,9 @@ def test_merge_snippet_fields_unions_locations_and_tags():
             ["py", "snippet", "helper"])
     src2 = ({"language": "py", "locations": [{"repo": "a", "path": "a.py", "symbol": "f"}]},  # dup loc
             ["snippet"])
-    locs, extra = s.merge_snippet_fields(target_fields, ["py", "snippet", "core"], [src1, src2])
+    locs, extra, contributions = s.merge_snippet_fields(
+        target_fields, ["py", "snippet", "core"], [src1, src2]
+    )
     # target location first, then unique source locations; dup dropped.
     assert locs == [
         {"repo": "a", "path": "a.py", "symbol": "f"},
@@ -157,6 +159,14 @@ def test_merge_snippet_fields_unions_locations_and_tags():
     # extra tags unioned, language + "snippet" markers excluded.
     assert extra == ["core", "helper"]
 
+    # Attribution (#2165): only what each source ACTUALLY added. src2's location
+    # is one the target already had, so it contributed nothing — un-merging it
+    # must not strip a call site the survivor owns in its own right.
+    assert contributions[0]["locations"] == [{"repo": "b", "path": "b.py", "symbol": "g"}]
+    assert contributions[0]["tags"] == ["helper"]
+    assert contributions[1]["locations"] == []
+    assert contributions[1]["tags"] == []
+
 
 # --- the queryable mirror (notes.data, migration 0070) -----------------------
 
@@ -253,19 +263,44 @@ def test_data_and_body_round_trip_to_the_same_fields():
 
 def test_normalize_merged_from_keeps_history_order_and_drops_junk():
     """Order is history, not sorting — earlier merges stay first."""
-    assert s._normalize_merged_from([9, 3, 9, "4", None, 0, -2, "x"]) == [9, 3, 4]
+    assert s.merged_from_ids([9, 3, 9, "4", None, 0, -2, "x"]) == [9, 3, 4]
     assert s._normalize_merged_from(None) == []
 
 
+def test_normalize_merged_from_carries_per_source_attribution():
+    """The shape that makes un-merge exact (#2165): each entry holds what THAT
+    source contributed, so reversing one can't strip what the survivor owns."""
+    loc = {"repo": "a", "path": "a.py", "symbol": "f"}
+    out = s._normalize_merged_from([{"id": 7, "locations": [loc], "tags": ["x"]}])
+    assert out == [{"id": 7, "locations": [loc], "tags": ["x"]}]
+
+
+def test_a_bare_id_normalizes_to_an_entry_with_no_attribution():
+    """Not legacy tolerance: snippet_fields falls back to PARSING THE BODY when a
+    row has no `data`, and the body's provenance line can only carry ids. Such an
+    entry still shows history; un-merge refuses it rather than guessing."""
+    assert s._normalize_merged_from([5]) == [{"id": 5}]
+
+
 def test_compose_body_renders_merged_from_and_parse_reads_it_back():
     body = s.compose_body(code="x = 1", merged_from=[12, 13])
     assert "**Merged from:** #12, #13" in body
     got = s.parse_snippet_fields("n — u", body, None)
-    assert got["merged_from"] == [12, 13]
+    # Ids survive the body round-trip; attribution cannot, since the body only
+    # ever renders ids — which is exactly why `data` is the authority for it.
+    assert s.merged_from_ids(got["merged_from"]) == [12, 13]
     # The code fence still comes last — provenance is header metadata.
     assert body.rstrip().endswith("```")
 
 
+def test_compose_body_renders_ids_from_rich_entries():
+    body = s.compose_body(
+        code="x = 1",
+        merged_from=[{"id": 12, "locations": [{"repo": "", "path": "a.py", "symbol": ""}]}],
+    )
+    assert "**Merged from:** #12" in body
+
+
 def test_compose_body_omits_merged_from_when_there_is_none():
     """A snippet that was never merged says nothing about merging."""
     assert "Merged from" not in s.compose_body(code="x = 1")
@@ -273,7 +308,8 @@ def test_compose_body_omits_merged_from_when_there_is_none():
 
 
 def test_compose_data_mirrors_merged_from():
-    assert s.compose_data(name="f", merged_from=[3, 3, 2])["merged_from"] == [3, 2]
+    got = s.compose_data(name="f", merged_from=[3, 3, 2])["merged_from"]
+    assert s.merged_from_ids(got) == [3, 2]
     assert "merged_from" not in s.compose_data(name="f")
 
 
@@ -283,7 +319,7 @@ def test_snippet_fields_prefers_the_data_columns_merged_from():
     body = s.compose_body(code="x = 1", merged_from=[12])
     note = _Note(title="n — u", body=body, tags=["snippet"],
                  data=s.compose_data(name="n", merged_from=[12, 13]))
-    assert s.snippet_fields(note)["merged_from"] == [12, 13]
+    assert s.merged_from_ids(s.snippet_fields(note)["merged_from"]) == [12, 13]
 
 
 def test_snippet_to_dict_includes_parsed_fields():

tests/test_snippet_merge_provenance.py

@@ -51,7 +51,7 @@ async def test_merge_records_what_it_absorbed_in_body_and_mirror():
         _snippet(id=1), [_snippet(id=2), _snippet(id=3)], [2, 3],
     )
     assert "**Merged from:** #2, #3" in kwargs["body"]
-    assert kwargs["data"]["merged_from"] == [2, 3]
+    assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
 
 
 @pytest.mark.asyncio
@@ -60,7 +60,7 @@ async def test_merge_accumulates_across_merges():
     replace the record of the first."""
     target = _snippet(id=1, data=s.compose_data(name="f", merged_from=[2]))
     kwargs = await _run_merge(target, [_snippet(id=3)], [3])
-    assert kwargs["data"]["merged_from"] == [2, 3]
+    assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
     assert "**Merged from:** #2, #3" in kwargs["body"]
 
 
@@ -74,7 +74,7 @@ async def test_merge_does_not_record_a_skipped_cross_owner_source():
         [_snippet(id=2, owner=9), _snippet(id=3, owner=42)],
         [2, 3],
     )
-    assert kwargs["data"]["merged_from"] == [2]
+    assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2]
     assert "#3" not in kwargs["body"]
 
 
@@ -93,7 +93,7 @@ async def test_an_ordinary_edit_carries_provenance_forward():
         await s.update_snippet(7, 1, signature="f(ms) -> string")
 
     kwargs = mock_update.await_args.kwargs
-    assert kwargs["data"]["merged_from"] == [2, 3]
+    assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
     assert "**Merged from:** #2, #3" in kwargs["body"]
 
 

tests/test_snippet_unmerge.py

@@ -0,0 +1,183 @@
+"""Tests for un-merge (#2165) — reversing one source out of a merged survivor.
+
+The design question this task said to settle first was how to subtract without
+stripping call sites the survivor legitimately owns. The answer is per-source
+attribution recorded AT MERGE TIME: each `merged_from` entry holds only what
+that source actually added. These tests pin that rule, and the refusal that
+guards the case where the attribution isn't there.
+"""
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from scribe.services import snippets as s
+
+
+def _loc(path, repo="r", symbol=""):
+    return {"repo": repo, "path": path, "symbol": symbol}
+
+
+def _survivor(locations, merged_from, tags=None, owner=7):
+    """A survivor note whose `data` carries locations + merge provenance."""
+    return SimpleNamespace(
+        id=1, user_id=owner, note_type="snippet", deleted_at=None,
+        title="f — does a thing", tags=tags or ["python", "snippet"],
+        body=s.compose_body(code="x = 1", language="python", locations=locations,
+                            merged_from=merged_from),
+        data=s.compose_data(name="f", when_to_use="does a thing", language="python",
+                            code="x = 1", locations=locations,
+                            merged_from=merged_from),
+    )
+
+
+async def _run_unmerge(survivor, *, source_alive=None, restore=1):
+    """Drive unmerge_snippet with the DB stubbed; return update_note's kwargs."""
+    async def fake_get(_uid, sid):
+        if sid == survivor.id:
+            return survivor
+        return source_alive
+
+    with (
+        patch.object(s, "get_snippet", AsyncMock(side_effect=fake_get)),
+        patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)),
+        patch("scribe.services.trash.restore_entity", AsyncMock(return_value=restore)),
+        patch.object(s.notes_svc, "update_note",
+                     AsyncMock(return_value=survivor)) as upd,
+        patch.object(s, "_embed_snippet", MagicMock()),
+    ):
+        await s.unmerge_snippet(7, 1, 2)
+    return upd.await_args.kwargs
+
+
+# --- the subtraction ------------------------------------------------------
+
+
+async def test_unmerge_strips_only_what_the_source_contributed():
+    """The survivor's own location survives; the source's is removed."""
+    mine, theirs = _loc("mine.py"), _loc("theirs.py")
+    survivor = _survivor(
+        [mine, theirs],
+        [{"id": 2, "locations": [theirs], "tags": ["helper"]}],
+        tags=["python", "snippet", "helper", "core"],
+    )
+    kwargs = await _run_unmerge(survivor)
+    assert kwargs["data"]["locations"] == [mine]
+    assert "helper" not in kwargs["tags"]
+    assert "core" in kwargs["tags"]
+
+
+async def test_a_location_the_survivor_also_owned_is_never_stripped():
+    """The central hazard the task named. If a source brought a location the
+    survivor ALREADY had, merge attributes nothing to it — so reversing must
+    leave that call site in place."""
+    shared = _loc("shared.py")
+    survivor = _survivor([shared], [{"id": 2, "locations": [], "tags": ["t"]}])
+    kwargs = await _run_unmerge(survivor)
+    assert kwargs["data"]["locations"] == [shared]
+
+
+async def test_the_reversed_entry_leaves_the_provenance_list():
+    survivor = _survivor(
+        [_loc("a.py"), _loc("b.py")],
+        [{"id": 2, "locations": [_loc("b.py")]}, {"id": 3, "locations": []}],
+    )
+    kwargs = await _run_unmerge(survivor)
+    assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [3]
+    assert "#2" not in kwargs["body"]
+    # The other source's history is untouched — un-merge reverses one thing.
+    assert "**Merged from:** #3" in kwargs["body"]
+
+
+async def test_unmerging_the_last_source_clears_the_provenance_line():
+    survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": [], "tags": ["t"]}])
+    kwargs = await _run_unmerge(survivor)
+    assert "Merged from" not in kwargs["body"]
+
+
+# --- restoring the source -------------------------------------------------
+
+
+async def test_an_already_restored_source_is_repaired_not_refused():
+    """The scenario that motivated the feature: the operator restored the source
+    from the trash by hand, so both records claim its call sites and nothing ever
+    stripped the survivor's copy. Un-merge must fix that, not reject it."""
+    theirs = _loc("theirs.py")
+    survivor = _survivor([_loc("mine.py"), theirs],
+                         [{"id": 2, "locations": [theirs]}])
+    alive = SimpleNamespace(id=2, user_id=7, note_type="snippet", deleted_at=None,
+                            title="g — x", tags=["snippet"], body="", data=None)
+    with patch("scribe.services.trash.restore_entity", AsyncMock()) as revive:
+        kwargs = await _run_unmerge(survivor, source_alive=alive)
+    # Nothing to revive — it's already alive — but the subtraction still happens.
+    revive.assert_not_called()
+    assert kwargs["data"]["locations"] == [_loc("mine.py")]
+
+
+async def test_a_purged_source_is_refused_and_the_survivor_is_untouched():
+    """If the source can't come back, stripping the survivor would lose the
+    locations entirely — no record would claim them."""
+    survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": [_loc("a.py")]}])
+
+    async def fake_get(_uid, sid):
+        return survivor if sid == 1 else None
+
+    with (
+        patch.object(s, "get_snippet", AsyncMock(side_effect=fake_get)),
+        patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)),
+        patch("scribe.services.trash.restore_entity", AsyncMock(return_value=None)),
+        patch.object(s.notes_svc, "update_note", AsyncMock()) as upd,
+        patch.object(s, "_embed_snippet", MagicMock()),
+    ):
+        with pytest.raises(s.UnmergeError, match="purged"):
+            await s.unmerge_snippet(7, 1, 2)
+    upd.assert_not_called()
+
+
+# --- refusals -------------------------------------------------------------
+
+
+async def test_an_entry_without_attribution_is_refused_not_guessed():
+    """Bare-id provenance comes from parsing the body, which can only hold ids.
+    Subtracting a guess could strip call sites the survivor owns — so refuse and
+    say what to do instead."""
+    survivor = _survivor([_loc("a.py")], [2])
+
+    async def fake_get(_uid, sid):
+        return survivor if sid == 1 else None
+
+    with (
+        patch.object(s, "get_snippet", AsyncMock(side_effect=fake_get)),
+        patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)),
+        patch.object(s.notes_svc, "update_note", AsyncMock()) as upd,
+        patch.object(s, "_embed_snippet", MagicMock()),
+    ):
+        with pytest.raises(s.UnmergeError, match="provenance"):
+            await s.unmerge_snippet(7, 1, 2)
+    upd.assert_not_called()
+
+
+async def test_unmerging_something_never_absorbed_is_refused():
+    survivor = _survivor([_loc("a.py")], [{"id": 99, "locations": []}])
+    with (
+        patch.object(s, "get_snippet", AsyncMock(return_value=survivor)),
+        patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)),
+    ):
+        with pytest.raises(s.UnmergeError, match="no record of absorbing"):
+            await s.unmerge_snippet(7, 1, 2)
+
+
+async def test_unmerge_requires_write_access():
+    """Same rule as merge: a read-only share can see the record, not rearrange it."""
+    survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": []}])
+    with (
+        patch.object(s, "get_snippet", AsyncMock(return_value=survivor)),
+        patch("scribe.services.access.can_write_note", AsyncMock(return_value=False)),
+    ):
+        with pytest.raises(PermissionError):
+            await s.unmerge_snippet(7, 1, 2)
+
+
+async def test_unmerge_on_a_missing_survivor_returns_none():
+    with patch.object(s, "get_snippet", AsyncMock(return_value=None)):
+        assert await s.unmerge_snippet(7, 1, 2) is None