Supersession (steps 1–4) — corrections demote, state lives on Systems #101

Merged

bvandeusen merged 8 commits from dev into main 2026-08-08 18:25:30 -04:00

Conversation 0 Commits 8 Files Changed 32

+1369 -83

8 Commits

Author	SHA1	Message	Date
bvandeusen	3f1523b19f	feat(systems): read-side teeth — the vocabulary at session start, a search filter, and the state/chronicle instructions ... Step 4 of #278, product half. The audit that motivated it: one System in project 2, thirty records tagged, nothing since July 28 — three days after the feature landed. Not a discipline failure; retrieval was completely blind to the association (zero references in embeddings, knowledge, search, auto-inject, or enter_project), so tagging was a write-side label with no read-side payoff, and labels nobody reads don't get maintained. Three changes, ordered by what makes the others workable: 1. enter_project returns the project's Systems (id, name, first line of the charter). Load-bearing for the tagging instruction: you cannot ask an agent to check a record against a vocabulary it never sees. Trimmed because it rides on every session start; the full charter stays get_system's job. Present-and-empty rather than absent when a project has none — "no named areas yet" is information the create-the-System instruction acts on. 2. search accepts system_id, MCP and REST (#33). Implemented once in semantic_search_notes as an EXISTS against record_systems — an association filter deciding candidate-set membership before scoring, like project_id, not a ranking signal. The REST route's missing project filter stays #2463's: it carries a default-scope UI decision this change must not preempt. 3. The instructions (#119, _INSTRUCTIONS + using-scribe skill; plugin 0.1.25 for the cache): - Tag as you write, with an executable test — "would someone investigating that subsystem want this in the pile list_system_records returns?" — rather than "tag appropriately", which is what died. - Create the System when the area has no record: the two-or-more test snippets use, plus "don't wait to be asked to name an area that plainly exists", because the agent's default was leaving un-modelled areas un-modelled forever. - State vs chronicle: dev-logs are written once and never rewritten; durable findings live in the System's reference note, updated in place — safe because note versions are the changelog, which has existed since the feature shipped and was never named as one. list_system_records' docstring now sells it as the way to READ a subsystem, reference note first. No auto-inject boost by System — vocabulary and filter first, measure before adding ranking behaviour (the #2486 lesson). Refs #278, #2546	2026-08-08 18:19:58 -04:00
bvandeusen	6c4c1bccfc	test: stub the auto-inject supersession lookup where there is no database ... CI on f20c019: nine failures across three files, all on the auto-inject path. Step 3's own tests passed; these are the same shape as the step 2 breakage — a real database call added to a path whose unit tests run without one. Stubbed per file, each saying why, rather than once in conftest. A global stub would hide the dependency from every future test on these paths too, which is the same "make the code lie" trade refused in 984407f, one level up. The auto-inject query is kept separate from the ranker's rather than threaded through, and that is deliberate: `_reserve_slot_for_reuse` runs a SECOND search and can add a hit to the menu, so the final `kept` set is not a subset of what the ranker scored. Labelling whatever actually reached the menu needs its own lookup over that final set — one indexed query on a handful of ids. Refs #278	2026-08-08 02:07:14 -04:00
bvandeusen	f20c019f2a	feat(supersession): demote what a later note overtook, and label it ... Step 3 of #278. First step with visible effect. ## Where the demotion happens, and why not in SQL Applied AFTER the pgvector fetch, over an over-fetched candidate set, not as part of the ORDER BY. Ordering by `distance + penalty` would be exact and would turn an indexed top-k into a scan-and-sort of every embedded note — the HNSW index from migration 0067 can only serve a raw-distance ordering. So the query fetches 3x the requested rows by raw distance and the re-rank happens in Python. Demoting after a LIMIT k with no over-fetch would have been theatre: the cut already happened, so a superseded record pushed down still sits in the results and the live record that should have replaced it was never fetched. The cost is stated in the code: a live record outside the over-fetch window cannot be promoted in. With a 0.05 penalty against neighbours ~0.014 apart, that needs the true answer more than three ranks down, which no observed query approaches. ## Demote, never hide — enforced in three places The penalty applies to the RANKING score, not to the relevance threshold. The floor decides whether a record is relevant at all; the penalty decides which relevant record comes first. Applying it to the floor would drop a superseded record out of the results entirely, which is the one thing this must not do. It is small on purpose. Supersession is a claim about SOME of a record's content, so one that strongly answers a question nothing else answers still surfaces — just behind anything comparable that is current. Its test asserts both bounds, the upper one citing the operator's constraint rather than an optimisation. And every test here that could be satisfied by dropping a record instead asserts the record is still present. ## The dedup gate opts out A superseded record is still a duplicate of what you are about to write — the claim is that it is no longer current, not that it is gone. Demoting it there would let the same note be recorded a second time, and the second copy would be the one nothing warns about. ## The label Auto-inject marks a superseded line SUPERSEDED with a pointer to check the later record. One query for the whole menu. An agent handed stale material with nothing marking it acts on it with full confidence, which is worse than never having surfaced it — the ranking is only half the fix. Fails open: a supersession lookup error returns unpenalised results rather than none, because unpenalised ranking is the behaviour that shipped for months and a broken search is not. Refs #278	2026-08-08 02:00:06 -04:00
bvandeusen	984407f931	fix(supersession): one query for both directions, not two per note read ... CI failed on 8d9e96c — eight tests in test_mcp_tool_notes.py, all "Connect call failed (127.0.0.1, 5432)". The proximate cause is that `_attach_supersession` runs on every note read/write and those are unit tests of the tool layer with no database. But the test failure exposed a worse decision underneath it. I had written the two directions as two service calls, so every `get_note` made TWO extra round trips plus TWO ACL checks — on the hottest path in the product — to save a two-line partition in Python. That is the wrong trade whether or not a test noticed. `get_relations` replaces both: one OR query, one ACL check, partitioned by which column holds the note's id. Its test asserts `execute.await_count == 1`, so the collapse can't quietly come apart later. The tests then get an autouse stub rather than the code getting a swallow. The tool genuinely has a new dependency; hiding that behind a try/except to keep unit tests green would be arranging for the code to lie about what it does. This file already records the same hazard for note 2109, so the stub sits next to that precedent. Added the test that matters, which the first pass missed: a superseded record still surfaces, so an agent WILL read stale material — and it must arrive with a plain-language warning, not just a numeric field to notice. Also pinned that both keys are ABSENT rather than present-and-empty when there are no relations. Refs #278	2026-08-07 22:45:56 -04:00
bvandeusen	8d9e96cc6d	feat(supersession): declare it — supersedes on both write paths ... Step 2 of #278. Records and reads the claim; the demotion that makes it matter is step 3. `services/supersession.py` with set/get on both directions, following the set_record_systems shape since this is the same kind of mutable M2M at the tool/route layer rather than inside notes_svc. ## Both directions are exposed, and only one is obvious `supersedes` is what the author claimed. `superseded_by` is what a READER needs and what the note itself cannot know — a stale record handed over with no marker gets acted on confidently, which is worse than never surfacing it. So get_note carries it, says so in its docstring, and adds a plain-language line telling the reader to open the newer note first. Both are OMITTED when empty rather than serialised as empty lists. A field that always says nothing trains readers to skip fields — the lesson consolidated_at cost, removed in the previous commit. ## Refuse vs drop, which is the one real judgement here Dropped silently: a target that doesn't exist, is trashed, is the note itself, or would close a cycle. Each is a claim with no subject or no meaning; none is something the caller can act on. REFUSED with PermissionError: a target the caller can read but not write. That is the single case where the caller could believe they succeeded and be wrong in a way that matters — demoting someone else's record out of their retrieval is damage invisible from the outside, with no symptom for the owner to trace. Rule #47, and PermissionError because services/snippets.py already uses it for read-but-not-write with both surfaces catching it. The PATCH/PUT routes scope by the CALLER, not owner_uid: an editor-share holder may edit the note and must not thereby inherit the owner's write access to whatever they name as superseded. ## Cycles A ring claims every member is obsolete. Under flat demotion that demotes them all equally, so the set drops out of ranked retrieval together with nothing in the data saying why. Refused by walking the existing graph from the proposed target — iteratively with a visited set, because the graph is user-supplied and a deep chain must not become a stack overflow on a write path. The visited set also makes the walk terminate on a ring that already exists, which is pinned by its own test rather than trusted. Both surfaces (#33), the instruction surface per #119 — framed as the third answer beside update-instead and force=true: not everything resembling an existing record should be folded into it, and not everything distinct should compete with it forever. Refs #278	2026-08-07 22:38:09 -04:00
bvandeusen	5dcb738ce8	fix(backup): carry note_supersessions — the coverage guard caught the omission ... CI failed on 45c6b1c. `tests/test_services_backup.py` asserts every table is either in `_BACKED_UP` or explicitly in `_NOT_INCLUDED`, and the new table was in neither. That guard exists for exactly this: a new table gets a model and a migration — both of which fail loudly if wrong — and then silently never gets a backup section. No error, no warning, and a restore that reports success. Its comment says it was written to "stop the seventh" table slipping through. This was the seventh. Supersession claims are backed up rather than excluded because they are a JUDGEMENT. Someone decided this note replaced that one, and nothing in either note's text records the decision. Lose them and a restored corpus silently returns to ranking stale material alongside current material — with no symptom that says why. Scoping differs by backup kind, deliberately. A whole-instance export takes every row. A single-user export takes only claims where BOTH ends are that user's notes: a claim spanning out to someone else's record cannot be restored into a single-user import, since the far id is not in the map, so exporting it would write a row that silently vanishes on the way back in. Restore maps both ids and skips the row unless both resolve. A claim is about a PAIR — half of one is not a weaker claim, it is a dangling row pointing at whatever note holds that id next. Guarded by `data.get` like every post-v2 section, so v5 and older payloads restore cleanly without it. BACKUP_VERSION 5 -> 6, and the version test moved with it. A payload section added without moving the version produces backups that are structurally different and indistinguishable by inspection. Refs #278	2026-08-07 21:49:08 -04:00
bvandeusen	45c6b1c88a	feat(supersession): the relation, and the dead column that stood where it should ... Step 1 of #278. Structure only — nothing reads or writes the new table yet. Old records outrank newer ones on the same subject because a similarity score cannot tell time. A note that accurately described how something worked in June is still accurate ABOUT June; it is just no longer the answer. Nothing recorded that, so nothing could act on it. `note_supersessions(superseder_id, superseded_id)`. The claim points FORWARD — the newer record names what it overtakes — because the older one cannot know it has been overtaken; asking it to record its own obsolescence is asking it to predict the future. A table rather than a column because the relation is genuinely many-to-many and partial, and both directions are hot: superseded_id answers "has this been overtaken?" at ranking time, superseder_id answers "what does this replace?" in a record view. An array column serves one and not the other. CASCADE is safe because trashing is not a delete — trash_svc stamps deleted_at, so a trashed note keeps its claims and restore brings them back. It fires only on purge_trash, where a claim about the row would be unactionable anyway. A CHECK rejects self-supersession, which under flat demotion would let a record demote itself. ## consolidated_at, and what it actually was Dropped. Written by nothing while serialised into every note and task payload as null — and worse, it implied a capability. The survey (#2483) read it as note consolidation modelled and abandoned. That was wrong, and the frontend is what says so: `TaskViewerView` rendered "✦ Auto-summarized from work logs" gated on this column. It is a survivor of the pre-pivot auto-summary subsystem (migration 0030), whose own column #599 removed. Not an unbuilt feature — an outlived one. So four more remnants went with it: the banner, its CSS, a `consolidatedAt` ref in TaskEditorView assigned and never read, and `.auto-summary-banner-editor` styling with zero template usage. That last one is presence-without-reference in the same family as the column itself. Dropped rather than repurposed for supersession, and the distinction is the point: consolidation folds records into one survivor and destroys the originals. Supersession is the opposite — both survive, the older ranks behind. Smuggling one in under a column named for the other would bury that in schema. ## The hard delete_note Removed, with a comment where it stood. Zero callers, and the danger was never that it ran — it is that it was findable by name. Someone wanting to delete a note greps `delete_note`, finds a function in the notes service with exactly the right signature, and permanently destroys a record every path downstream expects to be recoverable. The MCP tool of the same name already went through trash_svc; only the service function was the trap. Refs #278, #2483	2026-08-07 21:40:51 -04:00
bvandeusen	bbba0b3ae3	refactor(embeddings): one definition of the document a record is embedded as ... `f"{title}\n{body}"` was written out four times. #2486 found three — the write path, the recurring-task spawn, the startup backfill. The guard added here found the fourth immediately, and it was the one that mattered most. `dedup.find_duplicate_note` built the same string as a QUERY, compared against embedded documents. Shaped differently from the corpus it searches, the gate degrades silently: it still returns neighbours, just less apt ones, and nothing says the query and the index stopped agreeing. The spawn path has the same shape of risk — a recurring task embedded differently from everything else is ranked against documents it doesn't match. None of the four had diverged. That is what makes this worth doing now rather than after: they are identical today, so collapsing them is a no-op, and the whole point is that the next change to the shape can't hit three of four. Which is imminent. #2486 measured a dev-log separating from five unrelated dev-logs by 0.023 where a snippet separates by 0.153 — the difference being that a snippet states its purpose twice in a short document. Whether that shape is right is the open question; testing an alternative against four copies would mean testing a shape that isn't the one in production. This is the precondition the issue named. The guard is source inspection, matching the f-string pattern rather than a variable name, so a copy that renames its locals is still caught. A behavioural test cannot see this: an inlined copy produces the same string today and diverges the day the shape changes. Refs #2486	2026-08-07 13:01:45 -04:00