feat(snippets): record merge provenance on the survivor
CI & Build / Python lint (push) Successful in 4s
CI & Build / integration (push) Successful in 38s
CI & Build / TypeScript typecheck (push) Successful in 40s
CI & Build / Python tests (push) Successful in 59s
CI & Build / Build & push image (push) Successful in 1m7s
CI & Build / Python lint (push) Successful in 4s
CI & Build / integration (push) Successful in 38s
CI & Build / TypeScript typecheck (push) Successful in 40s
CI & Build / Python tests (push) Successful in 59s
CI & Build / Build & push image (push) Successful in 1m7s
Merge kept the target's fields, unioned locations and tags, and trashed the sources — recording nothing about what it absorbed. If a variant handled an edge case the survivor doesn't, that difference left the visible record entirely; recovering it meant knowing to go digging in the trash. The survivor now carries `merged_from`: a "**Merged from:** #2, #3" line in the body for humans, and the same list in the `data` mirror for queries, written from one value like every other field (#2087). It accumulates rather than replaces — a target merged twice keeps both histories — and skipped sources (cross-owner, per #231) are excluded, so the record never claims to contain something it never absorbed. Ordinary edits carry it forward. update_snippet recomposes body and mirror from scratch, so an omission there would silently erase the history on the next unrelated edit; that path is pinned by its own test, including the pre-0070 case where the body line is the only copy. Surfaced in the snippet detail view as a "Merged from" row — the view renders parsed fields, not the raw body, so the body line alone would have been invisible to the operator (rule #27). Un-merge, the other half of #2087, stays open: restoring a source from trash still doesn't strip its locations off the survivor, and what partial un-merge should mean is a design question, not a coding one. `merged_from` is the record that makes it tractable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
This commit is contained in:
@@ -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"
|
||||
|
||||
@@ -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"]
|
||||
Reference in New Issue
Block a user