fix(knowledge): the browse vocabulary catches up three kinds, and a snippet's mirror survives the generic door (#3128 recs 2-6)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / integration (push) Successful in 27s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 33s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / integration (push) Successful in 27s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 33s
Spike #3128 found the storage sound and the retrieval vocabulary frozen before `issue` shipped (0065). Five things, in the order they had to land. **The mirror (rec 5, the data-integrity one).** `notes.data` is DERIVED from a snippet's body, but only `update_snippet` knew that. `update_note` is a hasattr loop with no snippet awareness, and both doors reach it — so PATCH /api/notes/<snippet_id> {body} rewrote the body and left the mirror behind. `snippet_fields` PREFERS the mirror, so the row went on reporting its old repo/path/symbol to the location reverse lookup and to prior-art recall while displaying its new body: surfaced with full authority, and wrong. `snippets.recompose_data` rebuilds it from the body, carrying `verification` and `provenance` (neither is in the body to parse). An explicit `data` still wins, so every snippet-service write is untouched. **One facet table (rec 3), before adding any facet.** The type predicate was written three times — SQL, Python over semantic candidates, and a ternary computing the `is_task` pre-filter — and agreed only by luck. Adding `issue` to the SQL arm alone would have set the pre-filter to is_task=False, handed the Python arm a candidate set with no tasks in it, and returned an empty semantic half for the Issues facet forever with nothing red. `_FACETS` now generates all three. The Python arm also regains the `status IS NULL` half its SQL twin always had. **Issue and spike become facets (rec 2).** 435 issues — 17% of every task — were filterable nowhere on the human surface, while retired `plan` (90 rows) had a chip of its own. `_VALID_TYPES` was a hand-kept copy and is now derived. `plan` stays a valid facet for its legacy rows; it loses its chip. **Snippets stop being half-present in the feed (rec 4).** All 90 were in the All list, in no count, wearing an empty badge, and opening in the note editor. Counts now group by task_kind — every kind for the same two round-trips, which is why `issue` had no number — and total includes snippets, so the All chip matches the list it labels. Snippet cards route to /snippets/:id. **The prose that excused it (rec 6).** `snippet_fields` and the `data` column both still said pre-0070 rows were "never backfilled". True when 0070 landed, false since `backfill_snippet_data` shipped, and it read as licence for a stale mirror. Tests: the pre-filter can never exclude a row its own facet accepts (the regression, parameterised over every facet); both dialects select exactly their own rows; an unknown facet matches nothing; the mirror follows a body or title write, carries the verdict, and yields to an explicit `data`. `compiled_sql` moves to tests/helpers rather than becoming a third copy. Write-up: note #3161.
This commit is contained in:
@@ -0,0 +1,111 @@
|
||||
"""A snippet's `data` mirror survives the GENERIC note door.
|
||||
|
||||
`notes.data` is derived from the body. The snippet service always composed it
|
||||
from the field set it had just merged, so `update_snippet` was never the
|
||||
problem — the problem was every other way a snippet's body could be written.
|
||||
`update_note` is a `hasattr` loop with no snippet awareness, and both doors
|
||||
reach it: PATCH /api/notes/<id> and the MCP update_note tool. The Knowledge
|
||||
feed handed you that path, because a snippet card there routed to /notes/:id.
|
||||
|
||||
The failure was silent and the wrong way round: `snippet_fields` PREFERS the
|
||||
mirror, so the row went on reporting its old repo/path/symbol to the location
|
||||
reverse lookup and to prior-art recall while displaying its new body — a record
|
||||
surfaced with full authority and wrong, which the drift-check docstring calls
|
||||
worse than having no record at all (#3128).
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from tests.helpers import fake_note, fake_snippet, make_mock_session
|
||||
|
||||
OLD_MIRROR = {
|
||||
"name": "debounce",
|
||||
"language": "javascript",
|
||||
"locations": [{"repo": "Scribe", "path": "old/place.js", "symbol": "debounce"}],
|
||||
"verification": {"status": "ok", "code_sha": "abc", "checked_at": "2026-01-01"},
|
||||
"provenance": {"commit_sha": "deadbeef"},
|
||||
}
|
||||
|
||||
MOVED_BODY = (
|
||||
"**Locations:**\n"
|
||||
"- `Scribe` · `new/place.ts` · `debounce`\n\n"
|
||||
"```typescript\nexport const debounce = 1;\n```\n"
|
||||
)
|
||||
|
||||
|
||||
async def _update(note, **fields):
|
||||
session = make_mock_session()
|
||||
result = MagicMock()
|
||||
result.scalars.return_value.first.return_value = note
|
||||
session.execute = AsyncMock(return_value=result)
|
||||
with patch("scribe.services.notes.async_session") as cls, \
|
||||
patch("scribe.services.notes.embed_note", MagicMock()), \
|
||||
patch("scribe.services.notes._maybe_reactivate_project", AsyncMock()), \
|
||||
patch("scribe.services.note_versions.create_version", AsyncMock()):
|
||||
cls.return_value = session
|
||||
from scribe.services.notes import update_note
|
||||
await update_note(user_id=7, note_id=note.id, **fields)
|
||||
return note
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_body_write_moves_the_mirror_with_it():
|
||||
note = fake_snippet(data=dict(OLD_MIRROR), project_id=None)
|
||||
await _update(note, body=MOVED_BODY)
|
||||
assert note.data["locations"] == [
|
||||
{"repo": "Scribe", "path": "new/place.ts", "symbol": "debounce"}
|
||||
], "the mirror still describes where the snippet used to live"
|
||||
assert note.data["language"] == "typescript"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_verdict_and_provenance_are_carried_not_dropped():
|
||||
"""Neither is in the body to parse, so recomposing must carry them. An
|
||||
ordinary edit must not erase the last drift check — and it needs no
|
||||
invalidation branch either: `code_sha` is recomputed from the new code, so
|
||||
a verdict stamped against the old code expires itself on read."""
|
||||
note = fake_snippet(data=dict(OLD_MIRROR), project_id=None)
|
||||
await _update(note, body=MOVED_BODY)
|
||||
assert note.data["verification"] == OLD_MIRROR["verification"]
|
||||
assert note.data["provenance"] == OLD_MIRROR["provenance"]
|
||||
assert note.data["code_sha"] != OLD_MIRROR["verification"]["code_sha"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_explicit_data_wins_over_recomposition():
|
||||
"""`update_snippet` composes the mirror from the merged field set it holds
|
||||
and passes it here. That caller knows things the body cannot be re-read for
|
||||
— which locations were replaced, whether provenance survives the edit — so
|
||||
an explicit mirror must not be recomputed out from under it."""
|
||||
note = fake_snippet(data=dict(OLD_MIRROR), project_id=None)
|
||||
authoritative = {"name": "from the service", "locations": []}
|
||||
await _update(note, body=MOVED_BODY, data=authoritative)
|
||||
assert note.data == authoritative
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_plain_note_is_left_alone():
|
||||
"""Only snippets carry a mirror; a note's `data` must not be invented."""
|
||||
note = fake_note(note_type="note", data=None, project_id=None)
|
||||
await _update(note, body="just some prose")
|
||||
assert note.data is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_write_that_cannot_change_the_parse_does_not_touch_the_mirror():
|
||||
"""Status, priority, project — none of them is an input to the body parser,
|
||||
so recomposing on them would be work for nothing and would rebuild a mirror
|
||||
from a body nobody claimed to have changed."""
|
||||
note = fake_snippet(data=dict(OLD_MIRROR), project_id=None)
|
||||
await _update(note, project_id=4)
|
||||
assert note.data == OLD_MIRROR
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_title_change_reaches_the_mirror_too():
|
||||
"""A snippet's NAME lives in its title, not its body — `parse_snippet_fields`
|
||||
reads both, so both are triggers."""
|
||||
note = fake_snippet(data=dict(OLD_MIRROR), project_id=None)
|
||||
await _update(note, title="throttle — cap a callback's rate")
|
||||
assert note.data["name"] == "throttle"
|
||||
assert note.data["when_to_use"] == "cap a callback's rate"
|
||||
Reference in New Issue
Block a user