From cca40affe415f16d3bf31000ac8a47f9903e7a10 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 26 Jul 2026 18:25:55 -0400 Subject: [PATCH 1/8] =?UTF-8?q?feat(snippets):=20add=20notes.data=20JSONB?= =?UTF-8?q?=20=E2=80=94=20the=20indexed=20mirror=20of=20snippet=20fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #232 step 1 (task #2081). Takes the enabler first rather than the write-path trigger: reverse lookup, drift checks and the duplicate finder all need to QUERY structured fields, and building them on body-regex first means writing them twice. #227 deferred this bag "unless body-convention ergonomics prove insufficient" — answering "which snippets live in this file?" by scanning every snippet and regexing its body is that condition being met. Migration 0070 adds `notes.data` (nullable JSONB) + a GIN index. The body is UNCHANGED and still what gets embedded and read by humans; `data` mirrors the same facts in a shape Postgres can index. Code is deliberately not copied into it — the body holds it, and duplicating a blob into the column we index around would be waste. - compose_data() builds the mirror, omitting empties so the column stays sparse - snippet_fields() prefers `data`, falling back to parsing the body. Rows written before 0070 have no `data` and are never backfilled, so a hand-edited body stays authoritative for them with no conversion deadline - create / update / merge all write body and mirror from the same merged field set, so the two can't drift; merge in particular has to grow the mirror with the survivor's location set or a merged snippet would be unfindable at the very call sites the merge just recorded Named `data`, not `metadata`, because that collides with SQLAlchemy's declarative Base.metadata — which is why the pre-0069 model had to map an awkward `entity_metadata` attribute. Not a revival of the column 0069 dropped: different name, different purpose, nothing reads the old shape. Two test fakes needed an explicit `data = None`: snippet_fields prefers `data` when truthy and an auto-MagicMock attribute is truthy, so every parsed field would have come back a MagicMock. Checked every fake reaching snippet code this time rather than waiting for CI (note 2109). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- alembic/versions/0070_notes_data_jsonb.py | 60 +++++++++++++ src/scribe/models/note.py | 10 +++ src/scribe/services/notes.py | 2 + src/scribe/services/snippets.py | 105 ++++++++++++++++++++-- tests/test_mcp_tool_snippets.py | 4 + tests/test_services_snippets.py | 89 ++++++++++++++++++ tests/test_shared_write_access.py | 3 + 7 files changed, 264 insertions(+), 9 deletions(-) create mode 100644 alembic/versions/0070_notes_data_jsonb.py diff --git a/alembic/versions/0070_notes_data_jsonb.py b/alembic/versions/0070_notes_data_jsonb.py new file mode 100644 index 0000000..5518c36 --- /dev/null +++ b/alembic/versions/0070_notes_data_jsonb.py @@ -0,0 +1,60 @@ +"""add notes.data JSONB — queryable structured fields for typed records + +Revision ID: 0070 +Revises: 0069 +Create Date: 2026-07-26 + +Snippets (note_type='snippet') carry structured fields — name, language, +signature, and a list of canonical locations (repo · path · symbol). Those were +stored as a markdown body-convention, which reads well and feeds the embedding +but cannot be QUERIED: answering "which snippets live in this file?" meant +scanning every snippet and regexing its body. + +This adds a general `data` JSONB column plus a GIN index, so those fields become +indexable. The body stays exactly as it was — it is still the human-readable +form and still what gets embedded. `data` is a queryable mirror of the same +facts, not a replacement, and the code itself is deliberately NOT copied into it +(the body already holds it; duplicating a blob to index fields around it would +be waste). + +Relationship to 0069: that migration DROPPED `notes.metadata`, a JSONB column +which only ever held person/place/list entity fields, when those surfaces were +removed. This is not a revival of it — different name, different purpose, and +nothing reads the old shape. The column is named `data` rather than `metadata` +because `metadata` collides with SQLAlchemy's declarative `Base.metadata`, which +is why the old model had to map an awkward `entity_metadata` attribute onto it. + +Nullable with no backfill, deliberately: rows written before this migration keep +working because the service falls back to parsing the body when `data` is +absent. That means no migration deadline and no risk of a backfill mangling a +hand-edited body. + +Downgrade drops the index and the column. Any structured fields it held remain +recoverable from the body convention, which is the same source they mirror. +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects.postgresql import JSONB + + +revision = "0070" +down_revision = "0069" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("notes", sa.Column("data", JSONB, nullable=True)) + # GIN supports containment (`data @> '{"locations":[{"repo":"x"}]}'`), which + # covers exact repo/path/symbol and language lookups. Path PREFIX matching + # ("everything under frontend/src/") is not an index-served operation here + # and still filters after the fact — acceptable while snippet counts are + # small, and a generated column is the escape hatch if that changes. + op.create_index( + "ix_notes_data_gin", "notes", ["data"], postgresql_using="gin", + ) + + +def downgrade() -> None: + op.drop_index("ix_notes_data_gin", table_name="notes") + op.drop_column("notes", "data") diff --git a/src/scribe/models/note.py b/src/scribe/models/note.py index 6682c6d..1b85701 100644 --- a/src/scribe/models/note.py +++ b/src/scribe/models/note.py @@ -69,6 +69,13 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): # notes keep the 'work' default and ignore it. Orthogonal to note_type # (which is the note/entity axis). task_kind: Mapped[str] = mapped_column(Text, default="work", server_default="work") + # Queryable structured fields for typed records — currently snippets, whose + # name/language/signature/locations live here so they can be INDEXED. The + # body keeps the same facts in readable markdown and remains what gets + # embedded; this is a mirror for querying, not the source of truth for + # display. NULL on every row written before migration 0070, so readers fall + # back to parsing the body (see services/snippets.snippet_fields). + data: Mapped[dict | None] = mapped_column(JSONB, nullable=True) __table_args__ = ( Index("ix_notes_tags", "tags", postgresql_using="gin"), @@ -79,6 +86,9 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): Index("ix_notes_milestone_id", "milestone_id"), Index("ix_notes_note_type", "note_type"), Index("ix_notes_arose_from_id", "arose_from_id"), + # Containment queries into `data` — e.g. which snippets name a given + # repo/path in their locations. See migration 0070. + Index("ix_notes_data_gin", "data", postgresql_using="gin"), ) @property diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 3df0486..f36d08b 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -65,6 +65,7 @@ async def create_note( note_type: str = "note", task_kind: str = "work", arose_from_id: int | None = None, + data: dict | None = None, ) -> Note: # Validate status/priority here so the MCP create_task path (which passes # them straight through) can't persist an out-of-enum value that the REST @@ -108,6 +109,7 @@ async def create_note( note_type=note_type, task_kind=task_kind, arose_from_id=arose_from_id, + data=data, ) session.add(note) await session.commit() diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 0c4636e..94a37b5 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -250,11 +250,78 @@ def parse_snippet_fields( return fields +# --- the queryable mirror (notes.data, migration 0070) ----------------------- + +# 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") + + +def compose_data( + *, + name: str = "", + when_to_use: str = "", + signature: str = "", + language: str = "", + locations: list[dict] | None = None, +) -> dict: + """Build the `notes.data` mirror of a snippet's structured fields. + + Same facts as the body convention, in a shape Postgres can index — so + "which snippets live in this path?" is a containment query rather than a + regex over every body. Empty values are omitted so the column stays sparse + and containment matches don't trip over blanks. + """ + out: dict = {} + for key, value in ( + ("name", (name or "").strip()), + ("when_to_use", (when_to_use or "").strip()), + ("signature", (signature or "").strip()), + ("language", (language or "").strip().lower()), + ): + if value: + out[key] = value + locs = _normalize_locations(locations) + if locs: + out["locations"] = locs + return out + + +def snippet_fields(note) -> dict: + """Structured fields for a snippet, preferring the indexed `data` column and + falling back to parsing the body. + + Both paths must agree, because rows written before migration 0070 have no + `data` and are never backfilled — a hand-edited body is the authority for + those, and there is no deadline by which they must be converted. `code` only + ever comes from the body, since `data` doesn't carry it. + """ + parsed = parse_snippet_fields(note.title, note.body, note.tags) + stored = getattr(note, "data", None) + if not stored: + return parsed + merged = dict(parsed) + for key in _DATA_FIELDS: + if stored.get(key): + merged[key] = stored[key] + # Keep the single-location back-compat mirror consistent with whichever + # location list won. + locs = merged.get("locations") or [] + merged["repo"] = locs[0]["repo"] if locs else "" + merged["path"] = locs[0]["path"] if locs else "" + merged["symbol"] = locs[0]["symbol"] if locs else "" + return merged + + def snippet_to_dict(note) -> dict: """Note serialization plus a parsed ``snippet`` sub-object of structured - fields, so callers get both the raw record and the typed view.""" + fields, so callers get both the raw record and the typed view. + + The raw `data` column is intentionally NOT exposed: it mirrors what + ``snippet`` already reports, and shipping both would give API consumers two + sources of truth for the same facts.""" data = note.to_dict() - data["snippet"] = parse_snippet_fields(note.title, note.body, note.tags) + data["snippet"] = snippet_fields(note) return data @@ -278,17 +345,24 @@ async def create_snippet( """Create a snippet note (embedded on create for immediate recall). Returns the created Note. Pass ``locations`` for the multi-location case; the single ``repo``/``path``/``symbol`` are the one-location shorthand.""" + if locations is None: + locations = [{"repo": repo, "path": path, "symbol": symbol}] note = await notes_svc.create_note( user_id, title=compose_title(name, when_to_use), body=compose_body( code=code, language=language, signature=signature, - when_to_use=when_to_use, repo=repo, path=path, symbol=symbol, - locations=locations, + when_to_use=when_to_use, locations=locations, ), note_type=SNIPPET_NOTE_TYPE, tags=compose_tags(language, tags), project_id=project_id, + # The indexed mirror of the same fields (0070). Written together with the + # body so the two can never describe different things. + data=compose_data( + name=name, when_to_use=when_to_use, signature=signature, + language=language, locations=locations, + ), ) _embed_snippet(note) return note @@ -379,7 +453,7 @@ async def update_snippet( f"for edit access, or record your own version" ) - cur = parse_snippet_fields(note.title, note.body, note.tags) + cur = snippet_fields(note) overlay = { "name": name, "code": code, "language": language, "signature": signature, "when_to_use": when_to_use, @@ -405,6 +479,13 @@ async def update_snippet( signature=merged["signature"], when_to_use=merged["when_to_use"], locations=merged_locations, ), + # Re-derived from the same merged field set as the body, so an edit can't + # leave the indexed mirror describing the previous version. + "data": compose_data( + name=merged["name"], when_to_use=merged["when_to_use"], + signature=merged["signature"], language=merged["language"], + locations=merged_locations, + ), } # Recompute tags: keep any non-language, non-marker tags the note already had # (or the caller's replacement set), then re-derive language + marker. @@ -511,10 +592,8 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): continue sources.append(s) - tgt_fields = parse_snippet_fields(target.title, target.body, target.tags) - parsed_sources = [ - (parse_snippet_fields(s.title, s.body, s.tags), s.tags) for s in sources - ] + tgt_fields = snippet_fields(target) + parsed_sources = [(snippet_fields(s), s.tags) for s in sources] locations, extra_tags = merge_snippet_fields(tgt_fields, target.tags, parsed_sources) # Owner-scoped write, authorised above — same reason as update_snippet. @@ -526,6 +605,14 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): locations=locations, ), tags=compose_tags(tgt_fields["language"], extra_tags), + # The survivor's location set grew, so its mirror has to grow with it — + # otherwise a merged snippet would be unfindable at the very call sites + # the merge just recorded. + 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, + ), ) if updated is None: return None diff --git a/tests/test_mcp_tool_snippets.py b/tests/test_mcp_tool_snippets.py index adadc80..4c10f95 100644 --- a/tests/test_mcp_tool_snippets.py +++ b/tests/test_mcp_tool_snippets.py @@ -24,6 +24,10 @@ def _fake_snippet(user_id: int = 7): # decide whether to attach a shared/owner marker; an auto-MagicMock would read # as another user's record and send them off to look up a username. n.user_id = user_id + # Explicitly None, not an auto-attribute: snippet_fields prefers `data` when + # truthy, and a MagicMock is truthy — every parsed field would come back as a + # MagicMock instead of a string. + n.data = None n.to_dict.return_value = { "id": 1, "title": n.title, "note_type": "snippet", "tags": n.tags, } diff --git a/tests/test_services_snippets.py b/tests/test_services_snippets.py index 55487a9..31ad56f 100644 --- a/tests/test_services_snippets.py +++ b/tests/test_services_snippets.py @@ -158,6 +158,95 @@ def test_merge_snippet_fields_unions_locations_and_tags(): assert extra == ["core", "helper"] +# --- the queryable mirror (notes.data, migration 0070) ----------------------- + +def test_compose_data_keeps_only_populated_fields_and_no_code(): + got = s.compose_data( + name="formatDuration", when_to_use="humanize a ms count", + signature="f(ms) -> string", language="TS", + locations=[{"repo": "web", "path": "a.ts", "symbol": "f"}, + {"repo": "", "path": "", "symbol": ""}], + ) + assert got == { + "name": "formatDuration", + "when_to_use": "humanize a ms count", + "signature": "f(ms) -> string", + "language": "ts", + "locations": [{"repo": "web", "path": "a.ts", "symbol": "f"}], + } + # Code stays in the body — duplicating a blob into the column we index + # around would be pure weight. + assert "code" not in got + + +def test_compose_data_omits_blanks_entirely(): + """A sparse column keeps containment matches from tripping over empties.""" + assert s.compose_data(name="x") == {"name": "x"} + assert s.compose_data() == {} + + +class _Note: + """Minimal stand-in — snippet_fields only reads title/body/tags/data.""" + + def __init__(self, title="", body="", tags=None, data=None): + self.title, self.body, self.tags, self.data = title, body, tags or [], data + + +def test_snippet_fields_prefers_the_data_column(): + body = s.compose_body(code="x = 1", language="py", signature="old()", + locations=[{"repo": "old", "path": "o.py", "symbol": "o"}]) + note = _Note( + title="thing — old blurb", body=body, tags=["py", "snippet"], + data=s.compose_data(name="thing", when_to_use="new blurb", + signature="new()", language="py", + locations=[{"repo": "new", "path": "n.py", "symbol": "n"}]), + ) + got = s.snippet_fields(note) + assert got["when_to_use"] == "new blurb" + assert got["signature"] == "new()" + assert got["locations"] == [{"repo": "new", "path": "n.py", "symbol": "n"}] + # The back-compat single-location mirror follows whichever list won. + assert (got["repo"], got["path"], got["symbol"]) == ("new", "n.py", "n") + # Code has no home in `data`, so it still comes from the body. + assert got["code"] == "x = 1" + + +def test_snippet_fields_falls_back_to_the_body_when_data_is_absent(): + """Rows written before 0070 are never backfilled, so the body stays + authoritative for them — with no deadline to convert.""" + body = s.compose_body(code="y = 2", language="rb", signature="g()", + when_to_use="do a thing", + locations=[{"repo": "r", "path": "p.rb", "symbol": "g"}]) + got = s.snippet_fields(_Note(title="g — do a thing", body=body, + tags=["rb", "snippet"], data=None)) + assert got["signature"] == "g()" + assert got["language"] == "rb" + assert got["locations"] == [{"repo": "r", "path": "p.rb", "symbol": "g"}] + assert got["code"] == "y = 2" + + +def test_data_and_body_round_trip_to_the_same_fields(): + """The two representations must agree — they're written together, and a + disagreement would make a snippet read one way and query another.""" + fields = dict(name="debounce", when_to_use="rate-limit a callback", + signature="debounce(fn, ms)", language="ts") + locs = [{"repo": "web", "path": "src/util.ts", "symbol": "debounce"}] + from_body = s.snippet_fields(_Note( + title=s.compose_title(fields["name"], fields["when_to_use"]), + body=s.compose_body(code="const x = 1", locations=locs, **fields), + tags=s.compose_tags(fields["language"]), + )) + from_data = s.snippet_fields(_Note( + title=s.compose_title(fields["name"], fields["when_to_use"]), + body=s.compose_body(code="const x = 1", locations=locs, **fields), + tags=s.compose_tags(fields["language"]), + data=s.compose_data(locations=locs, **fields), + )) + for key in ("name", "when_to_use", "signature", "language", "locations", + "repo", "path", "symbol", "code"): + assert from_body[key] == from_data[key], key + + def test_snippet_to_dict_includes_parsed_fields(): class FakeNote: title = "debounce — rate-limit" diff --git a/tests/test_shared_write_access.py b/tests/test_shared_write_access.py index c68abb7..99e498a 100644 --- a/tests/test_shared_write_access.py +++ b/tests/test_shared_write_access.py @@ -24,6 +24,9 @@ def _snippet(id=1, owner=9): n.tags = ["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 (see note 2109). + n.data = None return n From fc9c8119a224cf7667710113fdddd00be65ebcee Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 26 Jul 2026 18:28:21 -0400 Subject: [PATCH 2/8] test(snippets): fix the round-trip test's shared kwargs spread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 2923: 1 failed, 408 passed. My test bug, not a code one — I spread one `fields` dict into both compose_body and compose_data, but compose_body takes no `name` (the name lives in the title). Each serializer now gets its own argument list. The migration itself was fine: the integration lane ran 0001→0070 on pgvector/pg17 green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- tests/test_services_snippets.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tests/test_services_snippets.py b/tests/test_services_snippets.py index 31ad56f..e6f364b 100644 --- a/tests/test_services_snippets.py +++ b/tests/test_services_snippets.py @@ -228,19 +228,21 @@ def test_snippet_fields_falls_back_to_the_body_when_data_is_absent(): def test_data_and_body_round_trip_to_the_same_fields(): """The two representations must agree — they're written together, and a disagreement would make a snippet read one way and query another.""" - fields = dict(name="debounce", when_to_use="rate-limit a callback", - signature="debounce(fn, ms)", language="ts") + name, when, sig, lang = ("debounce", "rate-limit a callback", + "debounce(fn, ms)", "ts") locs = [{"repo": "web", "path": "src/util.ts", "symbol": "debounce"}] - from_body = s.snippet_fields(_Note( - title=s.compose_title(fields["name"], fields["when_to_use"]), - body=s.compose_body(code="const x = 1", locations=locs, **fields), - tags=s.compose_tags(fields["language"]), - )) + # compose_body takes no `name` — the name lives in the title — so the two + # 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) + tags = s.compose_tags(lang) + + from_body = s.snippet_fields(_Note(title=title, body=body, tags=tags)) from_data = s.snippet_fields(_Note( - title=s.compose_title(fields["name"], fields["when_to_use"]), - body=s.compose_body(code="const x = 1", locations=locs, **fields), - tags=s.compose_tags(fields["language"]), - data=s.compose_data(locations=locs, **fields), + title=title, body=body, tags=tags, + data=s.compose_data(name=name, when_to_use=when, signature=sig, + language=lang, locations=locs), )) for key in ("name", "when_to_use", "signature", "language", "locations", "repo", "path", "symbol", "code"): From 8977bed28d483148255395504da56fe5db71fa86 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 27 Jul 2026 15:18:03 -0400 Subject: [PATCH 3/8] feat(inject): label the record kind on each auto-inject menu line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every injected hit rendered identically, so a recorded snippet was indistinguishable from a stray dev-log in the one place prior art most needs to stand out. Each line now carries its kind — [snippet], [process], [task], [issue], [note] — and the header says "records" rather than "notes", which it can no longer claim. Task-ness wins over note_type in the marker: "there's an open issue about this" is the more useful thing to know at a glance. Still title-first: the marker is metadata already on the ORM object, so no extra query and no bodies (#2084). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- src/scribe/services/plugin_context.py | 28 +++++++++++++--- tests/test_retrieval_scopes.py | 46 +++++++++++++++++++++++++-- tests/test_services_plugin_context.py | 12 ++++--- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 7241838..4f1b4f3 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -163,6 +163,22 @@ async def get_autoinject_config(user_id: int) -> dict: return {"enabled": enabled, "threshold": threshold, "top_k": top_k} +def _record_kind(note) -> str: + """The one-word kind marker for an injected menu line. + + The menu is drawn from every record that carries an embedding, so a snippet, + a stored process, an issue and a stray dev-log all arrive looking identical. + Recorded prior art only stands out if the line says what it is — and the kind + is also what tells the reader which tool opens it. + + Task-ness wins over `note_type` because it's the more useful distinction at a + glance: "there's an open issue about this" beats "there's a note about this". + """ + if note.is_task: + return "issue" if note.task_kind == "issue" else "task" + return note.note_type or "note" + + async def build_autoinject_hint( user_id: int, query: str, @@ -175,7 +191,7 @@ async def build_autoinject_hint( 1. high-confidence threshold (stricter than pull) — set per-user; 2. margin gate — keep only hits within _AUTOINJECT_BAND of the top score; 3. session dedup — caller passes already-injected ids as `exclude_ids`; - 4. title-first payload — id + title + score only, never bodies. + 4. title-first payload — id + kind + title + score only, never bodies. Disabled, blank-query, or nothing-clears-the-gates all return empty context, so most turns inject nothing. @@ -222,15 +238,19 @@ async def build_autoinject_hint( int(n.user_id) for _s, n in kept if n.user_id != user_id }) + # "records", not "notes" — the menu can hold snippets, processes and tasks + # too, and the kind marker on each line is only legible if the header doesn't + # already claim they're all one thing. lines = [ - "> Possibly relevant from your Scribe notes — call `get_note(id)` to " - "open any in full (titles only; injected once per session):", + "> Possibly relevant from your Scribe records — open any in full with " + "`get_note(id)`, or `get_snippet` / `get_process` for those kinds " + "(titles only; injected once per session):", ] note_ids: list[int] = [] for score, note in kept: note_ids.append(int(note.id)) title = (note.title or "(untitled)").replace("\n", " ").strip() - line = f"> - #{note.id} \"{title}\" ({score:.2f})" + line = f"> - #{note.id} [{_record_kind(note)}] \"{title}\" ({score:.2f})" if note.user_id != user_id: who = owners.get(int(note.user_id)) or "another user" line += f" — shared by {who}, treat as a suggestion" diff --git a/tests/test_retrieval_scopes.py b/tests/test_retrieval_scopes.py index 5eeaf29..478edce 100644 --- a/tests/test_retrieval_scopes.py +++ b/tests/test_retrieval_scopes.py @@ -17,15 +17,19 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -def _note(id=1, user_id=7, title="A note"): +def _note(id=1, user_id=7, title="A note", note_type="note", + is_task=False, task_kind="work"): n = MagicMock() n.id = id n.user_id = user_id n.title = title n.body = "body" n.tags = [] - n.is_task = False - n.note_type = "note" + # Real values, not auto-attributes: the menu reads these to label each line, + # and a MagicMock is truthy — every record would render as a task (note 2109). + n.is_task = is_task + n.task_kind = task_kind + n.note_type = note_type return n @@ -146,3 +150,39 @@ async def test_injected_menu_attributes_another_users_note(): assert "suggestion" in theirs_line # The operator's own note stays unadorned — absence of a marker is the signal. assert "shared by" not in mine_line + + +@pytest.mark.asyncio +async def test_injected_menu_labels_the_record_kind(): + """Every injected line renders the same shape, so without a kind marker a + recorded snippet is indistinguishable from a stray dev-log — exactly where + prior art most needs to stand out.""" + from scribe.services import plugin_context + + hits = [ + (0.92, _note(1, title="debounce — rate-limit a callback", note_type="snippet")), + (0.91, _note(2, title="Release checklist", note_type="process")), + (0.90, _note(3, title="Auth token expiry", is_task=True, task_kind="issue")), + (0.89, _note(4, title="Ship the drafter", is_task=True)), + (0.88, _note(5, title="Why we dropped CalDAV")), + ] + with patch.object(plugin_context, "semantic_search_notes", + AsyncMock(return_value=hits)), \ + patch.object(plugin_context, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.5, + "top_k": 5})), \ + patch.object(plugin_context, "record_retrieval", MagicMock()): + out = await plugin_context.build_autoinject_hint(7, "anything") + + lines = out["context"].splitlines() + by_id = {n: next(ln for ln in lines if f"#{n} " in ln) for n in (1, 2, 3, 4, 5)} + assert "[snippet]" in by_id[1] + assert "[process]" in by_id[2] + # Task-ness wins over note_type, and an issue says so rather than "task". + assert "[issue]" in by_id[3] + assert "[task]" in by_id[4] + assert "[note]" in by_id[5] + # Still title-first: the marker is metadata, not an excuse to carry bodies. + assert "body" not in out["context"] + # The header can't claim they're all notes when the markers say otherwise. + assert "Scribe records" in lines[0] diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 6281a47..42c19b6 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -10,13 +10,15 @@ def _rule(rid, title, topic_id): return r -def _note(nid, title, user_id=1): +def _note(nid, title, user_id=1, note_type="note", is_task=False, task_kind="work"): n = MagicMock() n.id, n.title = nid, title - # Real int, defaulting to the caller used in these tests: the injected menu - # compares it to decide whether the line needs a "shared by …" attribution, - # and an auto-MagicMock would read as another user's note. + # Real values, defaulting to the caller used in these tests: the injected menu + # compares user_id to decide whether the line needs a "shared by …" + # attribution, and reads is_task/task_kind/note_type for the kind marker. An + # auto-MagicMock is truthy, so every line would read as another user's task. n.user_id = user_id + n.note_type, n.is_task, n.task_kind = note_type, is_task, task_kind return n @@ -79,7 +81,7 @@ async def test_build_autoinject_hint_titles_only_with_margin_gate(): exclude_ids=[99]) # Margin gate kept the top two, dropped the straggler. assert out["note_ids"] == [11, 22] - assert '#11 "Pool sizing decision" (0.80)' in out["context"] + assert '#11 [note] "Pool sizing decision" (0.80)' in out["context"] assert "#33" not in out["context"] # Title-first: no body text, ever. assert "get_note(id)" in out["context"] From 9fa474b3c4882dbba9e4e0fbfa187eeb9a2ab6e3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 27 Jul 2026 15:18:03 -0400 Subject: [PATCH 4/8] fix(mcp): make get_note / get_task share-aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The injected menu tells the agent to open any hit with get_note(id), and that menu can list a collaborator's record reached through a shared project. The fetch was still owner-only, so those lines answered "not found" — for a record the same user opens fine in the browser. The agent path was strictly narrower than the web path for the same id. Same boundary miss as #2093: the list side was widened for sharing, the fetch side wasn't. Both tools now resolve through get_note_for_user, apply the trash filter themselves (permission resolution says nothing about liveness), and attach describe_provenance so a shared record arrives marked as someone else's rather than passing as the caller's. routes/tasks.py's parent-title lookup had the same narrowness: a shared subtask rendered as an orphan when its parent was equally shared. Four fetch tests across three files were patching notes_svc.get_note and had to be retargeted — note 2109's third sub-case, caught by grepping tests/ for the old name before pushing rather than by CI (#2159). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- src/scribe/mcp/tools/notes.py | 12 ++++++-- src/scribe/mcp/tools/tasks.py | 16 ++++++---- src/scribe/routes/tasks.py | 7 +++-- tests/test_mcp_tool_notes.py | 52 ++++++++++++++++++++++++++++++--- tests/test_mcp_tool_planning.py | 27 ++++++++++------- tests/test_mcp_tool_tasks.py | 44 ++++++++++++++++++++++------ 6 files changed, 123 insertions(+), 35 deletions(-) diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index d8a5b8a..58f696a 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -14,6 +14,7 @@ Sentinel conventions (inherited from existing fable-mcp tools): from __future__ import annotations from scribe.mcp._context import current_user_id +from scribe.services import access as access_svc from scribe.services import dedup as dedup_svc from scribe.services import notes as notes_svc from scribe.services import systems as systems_svc @@ -57,12 +58,17 @@ async def get_note(note_id: int) -> dict: """Fetch the full content of a single Scribe note by its ID. Returns id, title, body (markdown), tags, project_id, created_at, updated_at. + A note another user shared with you also carries `shared`, `owner` and + `permission` — read it as their suggestion, not as settled practice you set. """ uid = current_user_id() - note = await notes_svc.get_note(uid, note_id) - if note is None: + loaded = await notes_svc.get_note_for_user(uid, note_id) + note = loaded[0] if loaded else None + if note is None or note.deleted_at is not None: raise ValueError(f"note {note_id} not found") - return note.to_dict() + out = note.to_dict() + out.update(await access_svc.describe_provenance(uid, note)) + return out async def create_note( diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 1cb6eef..143f670 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -19,6 +19,7 @@ Sentinels (preserved from existing fable-mcp): from __future__ import annotations from scribe.mcp._context import current_user_id +from scribe.services import access as access_svc from scribe.services import dedup as dedup_svc from scribe.services import notes as notes_svc from scribe.services import planning as planning_svc @@ -68,17 +69,21 @@ async def get_task(task_id: int) -> dict: kind=plan tasks, the response also includes applicable_rules + subscribed_rulebooks from the task's project's rulebook subscriptions (new plans are milestones — use get_milestone for those). + + A task another user shared with you also carries `shared`, `owner` and + `permission` — it's their work item, not one you took on. """ uid = current_user_id() - note = await notes_svc.get_note(uid, task_id) - if note is None: + loaded = await notes_svc.get_note_for_user(uid, task_id) + note = loaded[0] if loaded else None + if note is None or note.deleted_at is not None: raise ValueError(f"task {task_id} not found") data = note.to_dict() parent_title = None if note.parent_id: - parent = await notes_svc.get_note(uid, note.parent_id) - if parent is not None: - parent_title = parent.title + parent_loaded = await notes_svc.get_note_for_user(uid, note.parent_id) + if parent_loaded is not None: + parent_title = parent_loaded[0].title data["parent_title"] = parent_title # Legacy kind=plan tasks predate milestone-as-plan; still surface their @@ -93,6 +98,7 @@ async def get_task(task_id: int) -> dict: data["project_rules"] = applicable.get("project_rules", []) data["suppressed_rules"] = applicable.get("suppressed_rules", []) data["suppressed_topics"] = applicable.get("suppressed_topics", []) + data.update(await access_svc.describe_provenance(uid, note)) return data diff --git a/src/scribe/routes/tasks.py b/src/scribe/routes/tasks.py index 9f0ee5f..c76f4f5 100644 --- a/src/scribe/routes/tasks.py +++ b/src/scribe/routes/tasks.py @@ -11,7 +11,6 @@ from scribe.services import systems as systems_svc from scribe.services.embeddings import upsert_note_embedding from scribe.services.notes import ( create_note, - get_note, get_note_for_user, list_notes, update_note, @@ -187,8 +186,10 @@ async def get_task_route(task_id: int): data = task.to_dict() data["permission"] = permission if task.parent_id: - parent = await get_note(uid, task.parent_id) - data["parent_title"] = parent.title if parent else None + # Share-aware like the task itself: a shared subtask whose parent is also + # shared would otherwise render as an orphan with no parent title. + parent = await get_note_for_user(uid, task.parent_id) + data["parent_title"] = parent[0].title if parent else None data["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)] return jsonify(data) diff --git a/tests/test_mcp_tool_notes.py b/tests/test_mcp_tool_notes.py index ccb5144..6836f20 100644 --- a/tests/test_mcp_tool_notes.py +++ b/tests/test_mcp_tool_notes.py @@ -17,11 +17,18 @@ def _bind_user(): _user_id_ctx.reset(token) -def _fake_note(**overrides) -> MagicMock: +def _fake_note(*, user_id: int = 7, **overrides) -> MagicMock: note = MagicMock() base = {"id": 1, "title": "t", "body": "b", "tags": [], "is_task": False} base.update(overrides) note.to_dict.return_value = base + # Real values, not auto-attributes: get_note reads deleted_at, and compares + # user_id against the bound caller to decide whether to attach a shared/owner + # marker. A MagicMock is truthy on both, so it would read as another user's + # trashed note and reach for the DB (note 2109). + note.id = base["id"] + note.user_id = user_id + note.deleted_at = None return note @@ -108,24 +115,61 @@ async def test_list_notes_limit_clamped(): async def test_get_note_returns_dict(): fake = _fake_note(id=5, title="found") with patch( - "scribe.mcp.tools.notes.notes_svc.get_note", - AsyncMock(return_value=fake), + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner")), ): out = await get_note(note_id=5) assert out["id"] == 5 assert out["title"] == "found" + # Own record: no provenance noise. + assert "shared" not in out + + +@pytest.mark.asyncio +async def test_get_note_opens_a_shared_record_and_says_whose_it_is(): + """The injected menu tells the agent to open any hit with get_note(id), and + that menu can list a collaborator's note reached through a shared project. + An owner-only fetch would answer "not found" for a record the agent was just + handed — and the web UI opens the same note fine.""" + theirs = _fake_note(id=5, title="Their note", user_id=9) + with patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(theirs, "viewer")), + ), patch( + "scribe.services.access.describe_provenance", + AsyncMock(return_value={"shared": True, "owner": "alex", + "permission": "viewer"}), + ): + out = await get_note(note_id=5) + assert out["shared"] is True + assert out["owner"] == "alex" + assert out["permission"] == "viewer" @pytest.mark.asyncio async def test_get_note_raises_when_not_found(): with patch( - "scribe.mcp.tools.notes.notes_svc.get_note", + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", AsyncMock(return_value=None), ): with pytest.raises(ValueError, match="note 999 not found"): await get_note(note_id=999) +@pytest.mark.asyncio +async def test_get_note_treats_a_trashed_note_as_missing(): + """get_note_for_user resolves permission, not liveness — the trash filter is + the caller's to apply.""" + trashed = _fake_note(id=5) + trashed.deleted_at = "2026-07-01T00:00:00Z" + with patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(trashed, "owner")), + ): + with pytest.raises(ValueError, match="note 5 not found"): + await get_note(note_id=5) + + @pytest.mark.asyncio async def test_create_note_passes_through(): fake = _fake_note(id=10, title="new") diff --git a/tests/test_mcp_tool_planning.py b/tests/test_mcp_tool_planning.py index d079e5d..e4d3ba3 100644 --- a/tests/test_mcp_tool_planning.py +++ b/tests/test_mcp_tool_planning.py @@ -24,16 +24,25 @@ async def test_start_planning_tool_delegates_to_service(): assert mock.call_args.kwargs == {"user_id": 7, "project_id": 3, "title": "Plan it"} -@pytest.mark.asyncio -async def test_get_task_augments_plan_with_rules(): +def _plan_note(task_kind: str): note = MagicMock() note.parent_id = None note.project_id = 3 - note.to_dict.return_value = {"id": 9, "task_kind": "plan", "project_id": 3} + note.id = 9 + # Real values — get_task reads deleted_at and compares user_id to the bound + # caller, and a MagicMock is truthy on both (note 2109). + note.user_id = 7 + note.deleted_at = None + note.to_dict.return_value = {"id": 9, "task_kind": task_kind, "project_id": 3} + return note + + +@pytest.mark.asyncio +async def test_get_task_augments_plan_with_rules(): applicable = {"rules": [{"id": 1, "title": "r"}], "truncated": False, "subscribed_rulebooks": [{"id": 2, "title": "rb"}]} - with patch("scribe.mcp.tools.tasks.notes_svc.get_note", - AsyncMock(return_value=note)), \ + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(_plan_note("plan"), "owner"))), \ patch("scribe.mcp.tools.tasks.rulebooks_svc.get_applicable_rules", AsyncMock(return_value=applicable)): from scribe.mcp.tools.tasks import get_task @@ -45,12 +54,8 @@ async def test_get_task_augments_plan_with_rules(): @pytest.mark.asyncio async def test_get_task_work_kind_has_no_rules(): - note = MagicMock() - note.parent_id = None - note.project_id = 3 - note.to_dict.return_value = {"id": 9, "task_kind": "work", "project_id": 3} - with patch("scribe.mcp.tools.tasks.notes_svc.get_note", - AsyncMock(return_value=note)), \ + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(_plan_note("work"), "owner"))), \ patch("scribe.mcp.tools.tasks.rulebooks_svc.get_applicable_rules", AsyncMock()) as mock_rules: from scribe.mcp.tools.tasks import get_task diff --git a/tests/test_mcp_tool_tasks.py b/tests/test_mcp_tool_tasks.py index 3841eb7..30d62d6 100644 --- a/tests/test_mcp_tool_tasks.py +++ b/tests/test_mcp_tool_tasks.py @@ -18,7 +18,8 @@ def _bind_user(): _user_id_ctx.reset(token) -def _fake_task(*, parent_id: int | None = None, **overrides) -> MagicMock: +def _fake_task(*, parent_id: int | None = None, user_id: int = 7, + **overrides) -> MagicMock: n = MagicMock() n.parent_id = parent_id base = { @@ -29,6 +30,12 @@ def _fake_task(*, parent_id: int | None = None, **overrides) -> MagicMock: base.update(overrides) n.to_dict.return_value = base n.title = base["title"] + n.id = base["id"] + # Real values, not auto-attributes: get_task reads deleted_at and compares + # user_id against the bound caller for the shared/owner marker — a MagicMock + # is truthy on both (note 2109). + n.user_id = user_id + n.deleted_at = None return n @@ -63,12 +70,13 @@ async def test_list_tasks_empty_status_means_no_filter(): async def test_get_task_with_no_parent_returns_null_parent_title(): fake = _fake_task(id=5, title="solo", parent_id=None) with patch( - "scribe.mcp.tools.tasks.notes_svc.get_note", - AsyncMock(return_value=fake), + "scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner")), ): out = await get_task(task_id=5) assert out["parent_title"] is None assert out["title"] == "solo" + assert "shared" not in out @pytest.mark.asyncio @@ -76,9 +84,9 @@ async def test_get_task_enriches_with_parent_title(): """When parent_id is set, get_task fetches the parent and adds parent_title.""" child = _fake_task(id=10, title="child", parent_id=5) parent = _fake_task(id=5, title="parent of 10", parent_id=None) - # get_note is called twice: once for child, once for parent - mock_get = AsyncMock(side_effect=[child, parent]) - with patch("scribe.mcp.tools.tasks.notes_svc.get_note", mock_get): + # fetched twice: once for the child, once for the parent + mock_get = AsyncMock(side_effect=[(child, "owner"), (parent, "owner")]) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", mock_get): out = await get_task(task_id=10) assert out["parent_title"] == "parent of 10" assert mock_get.await_count == 2 @@ -88,16 +96,34 @@ async def test_get_task_enriches_with_parent_title(): async def test_get_task_parent_missing_returns_null(): """If parent_id is set but the parent is gone (orphaned), parent_title is None.""" child = _fake_task(id=10, parent_id=5) - mock_get = AsyncMock(side_effect=[child, None]) - with patch("scribe.mcp.tools.tasks.notes_svc.get_note", mock_get): + mock_get = AsyncMock(side_effect=[(child, "owner"), None]) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", mock_get): out = await get_task(task_id=10) assert out["parent_title"] is None +@pytest.mark.asyncio +async def test_get_task_opens_a_shared_task_and_says_whose_it_is(): + """A task in a shared project opens in the web UI, so the agent path must not + answer "not found" for the same id — and must say it isn't the caller's.""" + theirs = _fake_task(id=5, title="Their task", user_id=9) + with patch( + "scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(theirs, "viewer")), + ), patch( + "scribe.services.access.describe_provenance", + AsyncMock(return_value={"shared": True, "owner": "alex", + "permission": "viewer"}), + ): + out = await get_task(task_id=5) + assert out["shared"] is True + assert out["owner"] == "alex" + + @pytest.mark.asyncio async def test_get_task_raises_when_not_found(): with patch( - "scribe.mcp.tools.tasks.notes_svc.get_note", + "scribe.mcp.tools.tasks.notes_svc.get_note_for_user", AsyncMock(return_value=None), ): with pytest.raises(ValueError, match="task 999 not found"): From 0d396de215e32291b6c49ceb6da2834ef71dba4f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 27 Jul 2026 15:23:41 -0400 Subject: [PATCH 5/8] feat(snippets): record merge provenance on the survivor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- frontend/src/api/snippets.ts | 3 + frontend/src/views/SnippetDetailView.vue | 23 +++++ src/scribe/mcp/tools/snippets.py | 4 + src/scribe/services/snippets.py | 70 +++++++++++++- tests/test_services_snippets.py | 43 ++++++++- tests/test_snippet_merge_provenance.py | 113 +++++++++++++++++++++++ 6 files changed, 249 insertions(+), 7 deletions(-) create mode 100644 tests/test_snippet_merge_provenance.py 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"] From dd1b5e5ddb72f16776b7a656dcf36a3f88e849ce Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 27 Jul 2026 23:09:37 -0400 Subject: [PATCH 6/8] =?UTF-8?q?feat(snippets):=20reverse=20lookup=20?= =?UTF-8?q?=E2=80=94=20find=20snippets=20by=20repo/path/symbol?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "What canonical helpers already live in this file?" was unanswerable: location lived only in the body markdown. It is now a jsonpath containment query over the `notes.data` mirror added by migration 0070. - One predicate in two dialects in services/knowledge.py: SQL (`data @?`, applied in the browse arm and the keyword arm before count/pagination, so totals stay honest) and Python (`location_matches`, for the semantic arm which post-filters candidates it already holds). Both must change together. - Parts are ANDed within a SINGLE locations entry — repo A in one entry and path B in another is not "recorded at A/B". `path` also matches as a directory prefix, via jsonpath `starts with` rather than `@>`, which the same GIN index serves. - `repo`/`path`/`symbol` reach the service, the REST list and the MCP tool under one name with one default (rule #33); the MCP docstring teaches the place form, and so does the reusing-code skill (plugin.json bumped). - UI: a Location disclosure beside the snippet search, with its own empty state — "nothing kept there, so what you're about to write is new." Settles #2083's open question (pre-0070 NULL `data`) by backfilling after all: `backfill_snippet_data` runs at startup, deriving the mirror from the body with the same parser the read path trusts. 0070's caution was about mangling a hand-edited body; this never touches the body. The alternative was a permanent second body-regex arm, or a query that silently answers "nothing here" for an old snippet and gets the helper written twice. Refs #2083, milestone #232. --- frontend/src/api/snippets.ts | 15 +- frontend/src/views/SnippetListView.vue | 148 +++++++++- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/reusing-code/SKILL.md | 8 + src/scribe/app.py | 9 + src/scribe/mcp/tools/snippets.py | 18 ++ src/scribe/routes/snippets.py | 6 + src/scribe/services/knowledge.py | 99 ++++++- src/scribe/services/snippets.py | 93 +++++- tests/test_integration_snippet_locations.py | 299 ++++++++++++++++++++ tests/test_routes_snippets.py | 13 + tests/test_snippet_location_filter.py | 205 ++++++++++++++ 12 files changed, 899 insertions(+), 16 deletions(-) create mode 100644 tests/test_integration_snippet_locations.py create mode 100644 tests/test_snippet_location_filter.py diff --git a/frontend/src/api/snippets.ts b/frontend/src/api/snippets.ts index 6b400dd..fa7ef03 100644 --- a/frontend/src/api/snippets.ts +++ b/frontend/src/api/snippets.ts @@ -80,13 +80,26 @@ export interface SnippetInput { force?: boolean; } +/** `repo`/`path`/`symbol` are the reverse lookup — "what already lives here?" + * They must all match the same recorded location; `path` also matches anything + * beneath it. Combinable with `q`. */ export async function listSnippets( - params: { q?: string; tag?: string; projectId?: number | null } = {}, + params: { + q?: string; + tag?: string; + projectId?: number | null; + repo?: string; + path?: string; + symbol?: string; + } = {}, ): Promise<{ snippets: SnippetListItem[]; total: number }> { const qs = new URLSearchParams(); if (params.q) qs.set("q", params.q); if (params.tag) qs.set("tag", params.tag); if (params.projectId) qs.set("project_id", String(params.projectId)); + if (params.repo) qs.set("repo", params.repo); + if (params.path) qs.set("path", params.path); + if (params.symbol) qs.set("symbol", params.symbol); const query = qs.toString(); return apiGet(`/api/snippets${query ? `?${query}` : ""}`); } diff --git a/frontend/src/views/SnippetListView.vue b/frontend/src/views/SnippetListView.vue index c5849cb..8fbed78 100644 --- a/frontend/src/views/SnippetListView.vue +++ b/frontend/src/views/SnippetListView.vue @@ -13,6 +13,27 @@ const error = ref(null); const search = ref(""); let searchTimer: ReturnType | undefined; +// Reverse lookup — "what already lives here?" All three must match the same +// recorded location; path also matches anything beneath it. +const showLocationFilter = ref(false); +const locRepo = ref(""); +const locPath = ref(""); +const locSymbol = ref(""); +const locationActive = computed( + () => !!(locRepo.value.trim() || locPath.value.trim() || locSymbol.value.trim()), +); +function clearLocation() { + locRepo.value = ""; + locPath.value = ""; + locSymbol.value = ""; + loadSnippets(); +} +function toggleLocationFilter() { + showLocationFilter.value = !showLocationFilter.value; + // Collapsing while filtered would hide why the list is short. + if (!showLocationFilter.value && locationActive.value) clearLocation(); +} + // Multi-select → merge const selectMode = ref(false); const selectedIds = ref>(new Set()); @@ -70,7 +91,12 @@ async function loadSnippets() { loading.value = true; error.value = null; try { - const data = await listSnippets({ q: search.value.trim() || undefined }); + const data = await listSnippets({ + q: search.value.trim() || undefined, + repo: locRepo.value.trim() || undefined, + path: locPath.value.trim() || undefined, + symbol: locSymbol.value.trim() || undefined, + }); snippets.value = data.snippets; } catch { error.value = "Couldn't load your snippets."; @@ -84,6 +110,9 @@ function onSearchInput() { clearTimeout(searchTimer); searchTimer = setTimeout(loadSnippets, 300); } +// Same debounce for the location fields — typing a path shouldn't fire a query +// per keystroke. +const onLocationInput = onSearchInput; onMounted(loadSnippets); @@ -127,6 +156,48 @@ function languageOf(tags: string[]): string { @input="onSearchInput" @keydown.enter="loadSnippets" /> + +
+ + +
+ + + +
@@ -138,15 +209,24 @@ function languageOf(tags: string[]): string {
❭_

- {{ search.trim() ? "No snippets match your search" : "No snippets kept yet" }} + {{ locationActive + ? "Nothing kept at that location yet" + : search.trim() + ? "No snippets match your search" + : "No snippets kept yet" }}

- {{ search.trim() - ? "Try a different term, or clear the search." - : "Record a reusable function or component and it will be offered back to you later." }} + {{ locationActive + ? "No recorded snippet lives there — so whatever you're about to write is new. Widen the path, or clear the filter." + : search.trim() + ? "Try a different term, or clear the search." + : "Record a reusable function or component and it will be offered back to you later." }}

+
+
+ +

+ Checks the file Claude is about to write or edit against your recorded + snippets — what's already kept at that path, and what resembles the code + being written — so a helper you already have is offered before it's + rewritten. Uses the same threshold and ceiling above, and never blocks the + edit. Off = prior art surfaces only on your own prompts. +

+