diff --git a/frontend/src/api/snippets.ts b/frontend/src/api/snippets.ts index 6b4a025..6b400dd 100644 --- a/frontend/src/api/snippets.ts +++ b/frontend/src/api/snippets.ts @@ -20,6 +20,9 @@ export interface SnippetFields { path: string; symbol: string; locations: SnippetLocation[]; + /** Ids of the snippets folded into this one by merge, oldest first. Read-only: + * merge is the only thing that adds to it, and an edit carries it forward. */ + merged_from: number[]; code: string; } diff --git a/frontend/src/views/SnippetDetailView.vue b/frontend/src/views/SnippetDetailView.vue index 6718ac5..3d72f92 100644 --- a/frontend/src/views/SnippetDetailView.vue +++ b/frontend/src/views/SnippetDetailView.vue @@ -23,6 +23,10 @@ const canWrite = computed(() => { const locations = computed(() => snippet.value?.snippet.locations ?? []); +// What this record absorbed. Merge keeps the target's fields and trashes the +// sources, so this line is the only visible trace that the variants existed. +const mergedFrom = computed(() => snippet.value?.snippet.merged_from ?? []); + function locParts(loc: { repo: string; path: string; symbol: string }): string[] { return [loc.repo, loc.path, loc.symbol].filter((p) => p && p.trim()); } @@ -110,6 +114,15 @@ async function confirmDelete() {
Language
{{ snippet.snippet.language }}
+
@@ -274,6 +287,16 @@ async function confirmDelete() { gap: 0.35rem; align-items: center; } +.merged-from { + display: flex; + flex-wrap: wrap; + gap: 0.35rem; + align-items: baseline; +} +.merged-hint { + color: var(--color-text-muted); + font-size: 0.8rem; +} .code-block { border: 1px solid var(--color-border); diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index f2a93fe..6d2e319 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -243,6 +243,10 @@ async def merge_snippets(target_id: int, source_ids: list[int]) -> dict: trash (recoverable). The survivor is re-embedded so recall stops surfacing the now-merged duplicates. + The survivor records what it absorbed as `merged_from` (in its `snippet` + fields and as a "Merged from: #ids" line in the body), so a variant that got + folded in leaves a trace outside the trash. It accumulates across merges. + Args: target_id: The snippet to keep (the canonical record). source_ids: Snippet ids to fold into the target and retire. Ids that diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 94a37b5..94c92d5 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -120,6 +120,23 @@ def _render_location_block(locations: list[dict]) -> str | None: return f"**Locations:**\n{lines}" +def _normalize_merged_from(ids: list[int] | None) -> list[int]: + """Merge provenance as a clean id list: ints only, no dups, order kept. + + Order is history, not sorting — earlier merges stay first, so the list reads + as the sequence of things folded in. + """ + out: list[int] = [] + for raw in ids or []: + try: + i = int(raw) + except (TypeError, ValueError): + continue + if i > 0 and i not in out: + out.append(i) + return out + + def compose_body( *, code: str, @@ -130,6 +147,7 @@ def compose_body( path: str = "", symbol: str = "", locations: list[dict] | None = None, + merged_from: list[int] | None = None, ) -> str: """Render structured fields into the snippet body markdown. Empty fields are omitted so the body stays clean. @@ -151,6 +169,14 @@ def compose_body( loc_block = _render_location_block(locs) if loc_block: header.append(loc_block) + merged = _normalize_merged_from(merged_from) + if merged: + # Human-readable mirror of data["merged_from"]. Without it, a merge folds + # variants in and the record of what was absorbed lives only in the trash + # — recoverable only by someone who already knows to go looking. + header.append( + "**Merged from:** " + ", ".join(f"#{i}" for i in merged) + ) fence_lang = (language or "").strip().lower() code_block = f"```{fence_lang}\n{(code or '').rstrip()}\n```" if header: @@ -166,7 +192,9 @@ _LOC_RE = re.compile(r"^\*\*Location:\*\*\s*(.+?)\s*$", re.MULTILINE) _LOCS_RE = re.compile( r"^\*\*Locations:\*\*[ \t]*\n((?:[ \t]*-[ \t]*.+\n?)+)", re.MULTILINE ) +_MERGED_RE = re.compile(r"^\*\*Merged from:\*\*\s*(.+?)\s*$", re.MULTILINE) _CODE_RE = re.compile(r"```([\w+.#-]*)\n(.*?)\n```", re.DOTALL) +_ID_RE = re.compile(r"#(\d+)") def _parse_location_str(s: str) -> dict | None: @@ -202,6 +230,7 @@ def parse_snippet_fields( "path": "", "symbol": "", "locations": [], + "merged_from": [], "code": "", } @@ -235,6 +264,12 @@ def parse_snippet_fields( fields["path"] = locations[0]["path"] fields["symbol"] = locations[0]["symbol"] + m = _MERGED_RE.search(body) + if m: + fields["merged_from"] = _normalize_merged_from( + [int(i) for i in _ID_RE.findall(m.group(1))] + ) + m = _CODE_RE.search(body) if m: fields["language"] = m.group(1).strip() @@ -254,7 +289,9 @@ def parse_snippet_fields( # Fields kept in `data`. Code is deliberately absent: the body already holds it, # and copying a blob into the column we index *around* would be pure weight. -_DATA_FIELDS = ("name", "when_to_use", "signature", "language", "locations") +_DATA_FIELDS = ( + "name", "when_to_use", "signature", "language", "locations", "merged_from", +) def compose_data( @@ -264,6 +301,7 @@ def compose_data( signature: str = "", language: str = "", locations: list[dict] | None = None, + merged_from: list[int] | None = None, ) -> dict: """Build the `notes.data` mirror of a snippet's structured fields. @@ -284,6 +322,9 @@ def compose_data( locs = _normalize_locations(locations) if locs: out["locations"] = locs + merged = _normalize_merged_from(merged_from) + if merged: + out["merged_from"] = merged return out @@ -478,6 +519,9 @@ async def update_snippet( code=merged["code"], language=merged["language"], signature=merged["signature"], when_to_use=merged["when_to_use"], locations=merged_locations, + # Carried, never set here: an ordinary edit must not erase the record + # of what was folded in, and only a merge may add to it. + merged_from=merged.get("merged_from"), ), # Re-derived from the same merged field set as the body, so an edit can't # leave the indexed mirror describing the previous version. @@ -485,6 +529,7 @@ async def update_snippet( name=merged["name"], when_to_use=merged["when_to_use"], signature=merged["signature"], language=merged["language"], locations=merged_locations, + merged_from=merged.get("merged_from"), ), } # Recompute tags: keep any non-language, non-marker tags the note already had @@ -566,8 +611,16 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): cross-owner merge stays out of scope (#231) — and must itself be writable; sources failing either test are skipped rather than silently half-merged. + The survivor records what it absorbed as `merged_from` — in the `data` mirror + and as a `**Merged from:** #ids` line in the body, written from one value like + every other field. It accumulates across merges and ordinary edits carry it + forward; only a merge adds to it. + Returns (merged_target_note, merged_source_ids), or None if the target isn't - a snippet the caller can see.""" + a snippet the caller can see. Note the returned ids are the sources actually + TRASHED, while `merged_from` is everything folded in — they differ only if a + source vanished between the field union and the trash call, which leaves the + source visible rather than losing anything.""" from scribe.services.access import can_write_note target = await get_snippet(user_id, target_id) if target is None: @@ -596,13 +649,22 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): parsed_sources = [(snippet_fields(s), s.tags) for s in sources] locations, extra_tags = merge_snippet_fields(tgt_fields, target.tags, parsed_sources) + # Provenance: what this record absorbed, and when it absorbed it, in order. + # Merge keeps the target's scalar fields and trashes the sources, so without + # this the fact that a variant ever existed survives only in the trash — and + # only for someone who already knew to go looking. Accumulated, not replaced: + # a target merged twice keeps both histories. + merged_from = _normalize_merged_from( + list(tgt_fields.get("merged_from") or []) + [s.id for s in sources] + ) + # Owner-scoped write, authorised above — same reason as update_snippet. updated = await notes_svc.update_note( owner_id, target_id, body=compose_body( code=tgt_fields["code"], language=tgt_fields["language"], signature=tgt_fields["signature"], when_to_use=tgt_fields["when_to_use"], - locations=locations, + locations=locations, merged_from=merged_from, ), tags=compose_tags(tgt_fields["language"], extra_tags), # The survivor's location set grew, so its mirror has to grow with it — @@ -611,7 +673,7 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): data=compose_data( name=tgt_fields["name"], when_to_use=tgt_fields["when_to_use"], signature=tgt_fields["signature"], language=tgt_fields["language"], - locations=locations, + locations=locations, merged_from=merged_from, ), ) if updated is None: diff --git a/tests/test_services_snippets.py b/tests/test_services_snippets.py index e6f364b..a6246e2 100644 --- a/tests/test_services_snippets.py +++ b/tests/test_services_snippets.py @@ -235,20 +235,57 @@ def test_data_and_body_round_trip_to_the_same_fields(): # serializers get their own argument lists rather than a shared spread. title = s.compose_title(name, when) body = s.compose_body(code="const x = 1", language=lang, signature=sig, - when_to_use=when, locations=locs) + when_to_use=when, locations=locs, merged_from=[41, 42]) tags = s.compose_tags(lang) from_body = s.snippet_fields(_Note(title=title, body=body, tags=tags)) from_data = s.snippet_fields(_Note( title=title, body=body, tags=tags, data=s.compose_data(name=name, when_to_use=when, signature=sig, - language=lang, locations=locs), + language=lang, locations=locs, merged_from=[41, 42]), )) for key in ("name", "when_to_use", "signature", "language", "locations", - "repo", "path", "symbol", "code"): + "merged_from", "repo", "path", "symbol", "code"): assert from_body[key] == from_data[key], key +# --- merge provenance (#2087) ------------------------------------------------ + +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._normalize_merged_from(None) == [] + + +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] + # The code fence still comes last — provenance is header metadata. + assert body.rstrip().endswith("```") + + +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") + assert s.parse_snippet_fields("n — u", "```\nx = 1\n```", None)["merged_from"] == [] + + +def test_compose_data_mirrors_merged_from(): + assert s.compose_data(name="f", merged_from=[3, 3, 2])["merged_from"] == [3, 2] + assert "merged_from" not in s.compose_data(name="f") + + +def test_snippet_fields_prefers_the_data_columns_merged_from(): + """Same rule as every other mirrored field: `data` wins when it's populated, + so a hand-edited body can't silently rewrite the merge history.""" + 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] + + def test_snippet_to_dict_includes_parsed_fields(): class FakeNote: title = "debounce — rate-limit" diff --git a/tests/test_snippet_merge_provenance.py b/tests/test_snippet_merge_provenance.py new file mode 100644 index 0000000..9fc6bb4 --- /dev/null +++ b/tests/test_snippet_merge_provenance.py @@ -0,0 +1,113 @@ +"""Merge records what it absorbed (#2087). + +Merging keeps the target's scalar fields, unions locations + tags, and trashes +the sources. Without provenance the fact that a variant ever existed survives +only in the trash — and only for someone who already knew to go looking. So the +survivor carries `merged_from`, written to the body and the indexed mirror from +one value, and ordinary edits must carry it forward rather than erase it. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import snippets as s + + +def _snippet(id=1, owner=9, body=None, data=None, tags=None): + n = MagicMock() + n.id = id + n.user_id = owner + n.title = "formatDuration — humanize a millisecond count" + n.body = body if body is not None else s.compose_body(code="x = 1", language="ts") + n.tags = tags if tags is not None else ["ts", "snippet"] + n.note_type = "snippet" + n.deleted_at = None + # Explicitly None — snippet_fields prefers `data` when truthy, and an + # auto-MagicMock attribute is truthy (note 2109). + n.data = data + return n + + +async def _run_merge(target, sources, source_ids): + """Drive merge_snippets with the DB stubbed out; return update_note's kwargs.""" + by_id = {target.id: target, **{x.id: x for x in sources}} + + async def fake_get(_uid, sid): + return by_id.get(sid) + + 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(return_value=target)) as mock_update, \ + patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ + patch.object(s, "_embed_snippet", MagicMock()): + await s.merge_snippets(7, target.id, source_ids) + return mock_update.await_args.kwargs + + +@pytest.mark.asyncio +async def test_merge_records_what_it_absorbed_in_body_and_mirror(): + kwargs = await _run_merge( + _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] + + +@pytest.mark.asyncio +async def test_merge_accumulates_across_merges(): + """A target merged twice keeps both histories — the second merge must not + 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 "**Merged from:** #2, #3" in kwargs["body"] + + +@pytest.mark.asyncio +async def test_merge_does_not_record_a_skipped_cross_owner_source(): + """A source owned by someone else is skipped, not folded in (#231) — so it + must not appear in the provenance either, or the record would claim to + contain something it never absorbed.""" + kwargs = await _run_merge( + _snippet(id=1, owner=9), + [_snippet(id=2, owner=9), _snippet(id=3, owner=42)], + [2, 3], + ) + assert kwargs["data"]["merged_from"] == [2] + assert "#3" not in kwargs["body"] + + +@pytest.mark.asyncio +async def test_an_ordinary_edit_carries_provenance_forward(): + """Only a merge may add to it, but nothing else may drop it — update + recomposes body and mirror from scratch, so an omission here would silently + erase the history on the next unrelated edit.""" + note = _snippet(id=1, data=s.compose_data(name="f", language="ts", + merged_from=[2, 3])) + with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ + patch.object(s.notes_svc, "update_note", + AsyncMock(return_value=note)) as mock_update, \ + patch.object(s, "_embed_snippet", MagicMock()): + await s.update_snippet(7, 1, signature="f(ms) -> string") + + kwargs = mock_update.await_args.kwargs + assert kwargs["data"]["merged_from"] == [2, 3] + assert "**Merged from:** #2, #3" in kwargs["body"] + + +@pytest.mark.asyncio +async def test_a_pre_0070_row_keeps_provenance_through_the_body(): + """Rows written before the `data` column are never backfilled, so the body + line is their only copy — and it has to survive an edit too.""" + body = s.compose_body(code="x = 1", language="ts", merged_from=[5]) + note = _snippet(id=1, body=body, data=None) + with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ + patch.object(s.notes_svc, "update_note", + AsyncMock(return_value=note)) as mock_update, \ + patch.object(s, "_embed_snippet", MagicMock()): + await s.update_snippet(7, 1, when_to_use="humanize a ms count") + + assert "**Merged from:** #5" in mock_update.await_args.kwargs["body"]