From 5d48e23285b1e7ca82f25c28610046f9031f3610 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 21 Oct 2026 20:18:30 -0400 Subject: [PATCH] docs(6205): notes comments state the reason, not the instance that taught it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue, decision, milestone and operator-decision citations are removed; "previously each route…" and "list_tasks once returned…" stories become the present-tense reasons they taught. The note about the removed delete_note now says plainly that there is deliberately none, and why. Co-Authored-By: Claude Opus 5.5 --- src/scribe/services/notes.py | 118 ++++++++++++++++------------------- 1 file changed, 55 insertions(+), 63 deletions(-) diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 4c460a56..440090df 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -21,11 +21,9 @@ _PARSED_FROM_BODY = frozenset({"title", "body", "tags"}) def _mirror_recomposers() -> dict[str, Callable[[Note], dict]]: """note_type -> the function that rebuilds that kind's derived `data`. - A TABLE rather than a chain of `if note_type == ...`, because the previous - shape tested one constant and the next kind with a derived mirror was - silently not covered — which is exactly what happened: the snippet guard - (#3128) was hard-coded, and lessons arrived in milestone 385 with the same - body-is-authority/`data`-is-mirror design and none of the protection. + A TABLE rather than a chain of `if note_type == ...`, because a chain + tests the constants someone remembered, and the next kind with a derived + mirror (body is authority, `data` is mirror) is silently not covered. The failure is invisible from here. Nothing raises, nothing logs; the row simply keeps answering queries from a mirror that no longer matches its @@ -49,7 +47,7 @@ def _mirror_recomposers() -> dict[str, Callable[[Note], dict]]: } -# Text fields where EMPTY MEANS NULL (milestone 317). The sweep's whole signal +# Text fields where EMPTY MEANS NULL. The sweep's whole signal # is `verify_with IS NULL` = "this is a decision, there is nothing to go and # check". An empty string that is not NULL makes a norm look like a constraint # nobody has verified, forever — and it would sit at the top of the sweep, @@ -64,7 +62,7 @@ def guard_check_fields(status: str | None, note_type: str | None) -> None: which fields a caller passed, so it also catches the sideways route: a checked note being turned into a task, which no per-field gate would see. - Raises rather than dropping silently, for minted_kind's reason (#3129) — a + Raises rather than dropping silently, for minted_kind's reason — a silently-corrected write is the defect that reasoning exists to end, and a caller putting a check on the wrong record has an idea an error corrects and a default hides. Lives at the service, not either door, so REST and MCP @@ -92,12 +90,11 @@ def embed_note(note) -> None: """Refresh a note's embedding, fire-and-forget. Lives HERE — at the service, not the route — so every caller gets it by - construction. Previously each REST route made this call itself and the MCP - tools did not, so a record created through MCP stayed out of semantic search - and auto-inject until the next restart's backfill ran (#2056). That is - invisible on an instance that redeploys constantly and permanent on one that - doesn't, which is the worst shape a bug can have: it only appears where - nobody is looking. + construction. A door that forgot the call would leave its records out of + semantic search and auto-inject until the next restart's backfill ran — + invisible on an instance that redeploys constantly and permanent on one + that doesn't, which is the worst shape a bug can have: it only appears + where nobody is looking. Uses `note.user_id` — the OWNER — rather than the caller. Embeddings belong to the record, and a collaborator editing a shared note must refresh the @@ -110,16 +107,16 @@ def embed_note(note) -> None: Detaching also means this task races anything that deletes the note out from under it. That is not handled here: `upsert_note_embedding` claims - the note's row before touching its vectors, and loses if it can't (#3262). + the note's row before touching its vectors, and loses if it can't. """ try: import asyncio from scribe.services.embeddings import document_title, upsert_note_embedding # Chunking and the empty-record gate live inside upsert_note_embedding — - # one path for every writer (#280). The title is the EMBEDDED one: a + # one path for every writer. The title is the EMBEDDED one: a # snippet's or lesson's trigger joins its name here, not in the stored - # title (milestone 427). + # title. asyncio.create_task( upsert_note_embedding( note.id, note.user_id, @@ -305,7 +302,7 @@ def build_note( expires_when=expires_when, ) # A create that names a status is the same transition an update to it is - # (#3683) — otherwise `create_task(status="in_progress")` writes a row no + # — otherwise `create_task(status="in_progress")` writes a row no # update could produce: started, with no `started_at`. if status is not None: apply_status_transition(note, user_id) @@ -318,11 +315,11 @@ def apply_status_transition(note: Note, user_id: int | None = None) -> None: Called by the update path whenever `status` is written and by `build_note` whenever a create names one, so a task created at a status is indistinguishable from one that reached it by update. Two copies of "what - a status implies" are where the two drift, and #3683 was that drift. + a status implies" are where the two drift. `user_id` is whoever is making the change: reaching `in_progress` is the moment a session takes the work on, so it stamps that user's claim, and a - status that ends or un-starts the work releases it (milestone 381). + status that ends or un-starts the work releases it. """ from scribe.services.task_claims import release_claim, stamp_claim @@ -382,15 +379,14 @@ async def list_notes( ) -> tuple[list[Note], int]: """Lifecycle-shaped listing. Two contracts worth knowing: - VISIBILITY is the shared browse clause (#47, #2462): the caller's own - records plus anything in a project they can reach — the same reach - query_knowledge has always had. Before this, list_notes was silently - owner-only, so a task in a shared project was invisible in list_tasks, + VISIBILITY is the shared browse clause: the caller's own records plus + anything in a project they can reach — the same reach query_knowledge has. + Owner-only here would hide a task in a shared project from list_tasks, enter_project's open-task list and the web UI while the same project's - notes appeared. Browse, not read, per decision #2094: an ambient list must - never surface a record someone shared one-to-one — those stay search-only. + notes appeared. Browse, not read: an ambient list must never surface a + record someone shared one-to-one — those stay search-only. - `q` is SEMANTIC (operator decision, 2026-08-06: "make it match") — the same + `q` is SEMANTIC — the same meaning-based match as Browse search, at the interactive floor. When `q` is present, relevance ordering wins and `sort` is ignored; a query is a relevance claim and sorting its results by date would shuffle the answer. @@ -430,9 +426,9 @@ async def list_notes( if query_vec is not None: from scribe.models.embedding import NoteEmbedding from scribe.services.embeddings import INTERACTIVE_SEARCH_THRESHOLD - # Best-chunk-per-note as a correlated MIN, not a join (#280): - # a note stores one embedding row PER CHUNK, so the plain join - # this used to be would repeat a long note once per matching + # Best-chunk-per-note as a correlated MIN, not a join: + # a note stores one embedding row PER CHUNK, so a plain join + # would repeat a long note once per matching # chunk — duplicated list rows and a total that counts chunks. # This query is filter-heavy and paginated, never HNSW-bound, # so the scalar subquery costs what the join did. @@ -528,7 +524,7 @@ async def list_notes( elif sort == "touched": # Last touched: the later of the task's own edit and its newest # work-log. Logging doesn't bump updated_at, so a task worked - # through its logs would otherwise sort as untouched (#4045). + # through its logs would otherwise sort as untouched. # GREATEST skips a NULL, so a task with no logs sorts by its edit. from scribe.models.task_log import TaskLog last_log = ( @@ -589,8 +585,8 @@ def minted_kind(kind: str) -> str: list is how the two doors would come to disagree. Raises rather than falling back to 'work'. A silently-corrected kind is - the defect this exists to end (#3129: the editor's Kind select reported - success and changed nothing), and a caller naming a kind we do not know + the defect this exists to end (an editor that reports success and changes + nothing), and a caller naming a kind we do not know has a wrong idea that an error corrects and a default hides. """ if kind in MINTABLE_KINDS: @@ -616,7 +612,7 @@ async def update_note( the one form that cannot happen by accident. The REST door, where a cleared form input arrives as "", reaches the same place through the NULLABLE_NOTE_TEXT normalisation below — two idioms, one outcome. - (Same shape as rulebooks.update_rule, milestone 312 step 2.) + (Same shape as rulebooks.update_rule.) """ async with async_session() as session: result = await session.execute( @@ -650,9 +646,7 @@ async def update_note( # Same shape as status/priority above, and for the same # reason: a kind the column will refuse should fail here with # a readable message, not as a CheckViolationError from the - # database. Before this, `task_kind` reached setattr through - # the hasattr guard with no validation at all — but no door - # ever offered it, so a task's kind was write-once (#3129). + # database. try: value = TaskKind(value).value except ValueError: @@ -684,12 +678,12 @@ async def update_note( if note.verify_with != check_before: note.verified_at = None # Some kinds derive `data` from their body — so a write that moves the - # body through this generic door must move the mirror with it (#3128). - # Without this, PATCH /api/notes/ {body} left the mirror behind, - # and every read PREFERS the mirror: a snippet went on reporting its + # body through this generic door must move the mirror with it. + # Without this, PATCH /api/notes/ {body} would leave the mirror + # behind, and every read PREFERS the mirror: a snippet would report its # old repo/path/symbol to prior-art recall while showing its new body, - # and a lesson would go on being retrieved for the situation it used to - # name. The kind's own updater composes the mirror itself and passes it + # and a lesson would be retrieved for the situation it used to name. + # The kind's own updater composes the mirror itself and passes it # explicitly, so an explicit `data` always wins — the caller that knows # the field set beats the one that can only re-read the body. if "data" not in fields and not _PARSED_FROM_BODY.isdisjoint(fields): @@ -715,22 +709,21 @@ async def update_note( return note -# A hard `delete_note(user_id, note_id)` lived here with ZERO callers, and was -# removed with #278 step 1. It is recorded rather than silently dropped because -# 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. +# There is deliberately no hard `delete_note(user_id, note_id)` here. The +# danger of one is not that it runs — it is that it is 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 delete path is `trash_svc.delete`, which soft-deletes an entity AND its # descendants under one batch_id so `restore(batch)` works. `purge_trash` owns # permanent deletion. Both are reachable; neither is spelled `delete_note`. -# ── The sweep (milestone 317 step 3) ───────────────────────────────────────── +# ── The sweep ──────────────────────────────────────────────────────────────── # # A SIBLING of rulebooks.rules_due_for_verification, not a shared -# implementation, and deliberately so (note 3163). The row could have been +# implementation, and deliberately so. The row could have been # shared; the QUERY cannot. That sweep scopes by rulebook ownership XOR project # ownership because rules have no sharing ACL at all — no rule_shares, no # can_read_rule. A note scopes by the note ACL, which is a different question @@ -760,12 +753,12 @@ async def notes_due_for_verification( decisions, there is nothing to go and check, and listing them would dilute the result until nobody reads it. - Scoped with `browsable_notes_clause`, NOT the read scope (decision note - 2094): a sweep is a passive surface, and a record shared one-to-one must + Scoped with `browsable_notes_clause`, NOT the read scope: a sweep is a + passive surface, and a record shared one-to-one must not arrive in one unasked. Deliberately NOT filtered to non-task, non-snippet records even though the - write path (step 2) permits a check on nothing else. A row in that state + write path permits a check on nothing else. A row in that state would be a row in an ILLEGAL state, and this is the one surface that could tell somebody about it. Hiding it here to match the invariant would make the sweep agree with a database it had stopped describing. @@ -942,10 +935,10 @@ async def resolve_process(user_id: int, name_or_id) -> tuple[Note | None, list[d """Resolve a stored process by id or name. note_type='process', non-trashed, scoped to what this user may READ — owned - plus shared (rule #78). Naming one is an explicit act, so a Process shared - with the caller resolves here even though it is deliberately absent from the - passive process list and the skill manifest (decision note 2094); without - this, a Process a search surfaced could not then be run — see #2093. + plus shared (services/access.py). Naming one is an explicit act, so a + Process shared with the caller resolves here even though it is + deliberately absent from the passive process list and the skill manifest; + without this, a Process a search surfaced could not then be run. Precedence: numeric id → exact case-insensitive title → substring. Returns (note, other_candidates); on a substring tie with no exact hit, `note` is the @@ -1118,16 +1111,15 @@ async def get_note_for_user( return (note, perm) if note else None -# What a LISTING row carries (#4061): what the record is and where it sits, -# never what it says. list_tasks once returned every row's to_dict(), body -# included, and a project's todo list came to 93-165k characters, past what an -# MCP client accepts inline, so the list arrived as a file to page through. The -# same failure #4045 fixed for enter_project, one call over. get_task / get_note -# read a record in full; a list is for choosing which one to open. +# What a LISTING row carries: what the record is and where it sits, never what +# it says. Every row's to_dict(), body included, can bring a project's todo +# list past what an MCP client accepts inline, so the list arrives as a file to +# page through. get_task / get_note read a record in full; a list is for +# choosing which one to open. # # A field most rows leave empty (a one-line description, a parent, a due date) # is attached only when set: a hundred rows of `null` are a hundred chances to -# learn to skip the key (#2483), and the bytes are the thing being cut. +# learn to skip the key, and the bytes are the thing being cut. def brief_row( note: Note, milestone_titles: dict[int, str] | None = None,