From 26a757ecfee58063244b3a4ea4fa8d9a45e45f00 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 19 Sep 2026 23:17:20 -0400 Subject: [PATCH 1/3] feat(lessons): a lesson is yours to keep current too (#4195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lesson kind shipped with every mechanism for growing and nothing telling a session to use them. `learned_from` is a list on purpose, the dedup gate hands back an existing id rather than minting a twin, and `update_lesson` already names re-keying a bad trigger as the edit that pays most. None of that was reachable as a habit. The exclusivity claim was the bug. The skill said "a preference is the one record you keep current yourself", and by naming only preferences it put lessons outside the habit. That sentence is now "a preference is yours to keep current", which says the same thing about preferences without saying anything false about lessons. Beside it, a paragraph on what growing a lesson means: another incident added to what taught it, a claim stated more exactly, or a trigger re-keyed to the situation that really fired. Written as a practice rather than a prohibition (rule 165) — the reader is named as the one person placed to judge the trigger, because they are standing in the situation it claims to name. `get_lesson` carries the same prompt at the moment it bites: a session reading a lesson inside the situation it names is the only reader who can tell whether the trigger is keyed to what actually fired. The guidance-ownership registry gains the topic and re-points the preference topic's statement, since the phrase it pinned is the sentence this change rewrites — the module asks for exactly that, in the same commit. No index marker: the index names session-start reflexes and this one fires mid-work, so `_INSTRUCTIONS` stays at 1998/2000. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/using-scribe/SKILL.md | 30 ++++++++++++++++++++--------- src/scribe/mcp/tools/lessons.py | 12 +++++++++++- tests/test_guidance_ownership.py | 11 ++++++++++- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 6d5993e..62a9f31 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.19.1614", + "version": "2026.09.20.0317", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 96025e5..da98344 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -110,15 +110,27 @@ Two constraints on *how* that's achieved: silently, because it sometimes answers that what you are holding was never a rule — and routing an observation is not losing it. - **A preference is the one record you keep current yourself.** When the - operator corrects you, or the preference on file no longer matches how they - actually want something done, `update_preference` — that is expected, not a - liberty, and updating one mid-work is the normal case rather than an - interruption of it. It wants the task or note that taught the change. Say in the - same turn that you did it, so they can disagree while it is in front of - them. A rule waits for the operator instead: `create_rule` proposes and - asks. If what you learned is that something MUST be done a certain way, - that is a rule to propose, not a preference to harden in place. + **A preference is yours to keep current.** When the operator corrects you, + or the preference on file no longer matches how they actually want + something done, `update_preference` — that is expected, not a liberty, and + updating one mid-work is the normal case rather than an interruption of it. + It wants the task or note that taught the change. Say in the same turn that + you did it, so they can disagree while it is in front of them. A rule waits + for the operator instead: `create_rule` proposes and asks. If what you + learned is that something MUST be done a certain way, that is a rule to + propose, not a preference to harden in place. + + **A lesson grows each time it proves itself.** When one arrives and the + situation it names is the one you are actually in, you are the single + reader placed to tell whether its trigger is keyed right and whether its + claim covers what you are seeing. `update_lesson` takes what you now know: + another incident added to what taught it, the claim stated more exactly, + or — the edit worth most — a trigger re-keyed to the situation that really + fired. A lesson nobody reaches is seldom wrong; far more often it is + waiting in a situation nobody is in. One claim that has met the same + failure four times is worth more than four claims that each met it once, + so when a near-duplicate create hands back an existing id, that is the + record to grow. **Preferences shape how work is done, never what gets recorded.** They govern your conduct — how you report, how carefully you pace, which form diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 05bd134..1d981ff 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -178,7 +178,17 @@ async def create_lesson( async def get_lesson(lesson_id: int) -> dict: - """Fetch one lesson by id, with its trigger and sources read back out.""" + """Fetch one lesson by id, with its trigger and sources read back out. + + IF THIS LESSON JUST PROVED ITSELF, IT IS WORTH MORE THAN IT SAYS. You are + reading it inside the situation it names, which makes you the one reader + who can tell whether its trigger is keyed to what actually fired and + whether its claim covers what you are seeing. `update_lesson` takes + another incident into `learned_from`, a claim stated more exactly, or a + re-keyed trigger — and the trigger is the edit that pays most, because a + lesson keyed to a situation nobody is in looks exactly like one nobody + needed. + """ uid = current_user_id() note = await lessons_svc.get_lesson(uid, lesson_id) if note is None: diff --git a/tests/test_guidance_ownership.py b/tests/test_guidance_ownership.py index bbf1f38..57f256e 100644 --- a/tests/test_guidance_ownership.py +++ b/tests/test_guidance_ownership.py @@ -118,7 +118,8 @@ TOPICS: tuple[Topic, ...] = ( "an empty session is not evidence of an empty rulebook", index=('content_type="rule"', "nothing matched")), Topic("rules bind, preferences guide and are kept current", U, ("preference", "update_preference"), - "a preference is the one record you keep current yourself", index=("preferences guide",)), + "updating one mid-work is the normal case rather than an interruption of it", + index=("preferences guide",)), Topic("kind is force: a rule binds, a preference guides, a lesson informs", U, ("lesson", "costs time", "costs consistency"), "what happens if someone doesn't do this", @@ -129,6 +130,14 @@ TOPICS: tuple[Topic, ...] = ( # mis-routing #3733 recorded happens, and the caller there has not # necessarily read the skill. shared_with=("docstrings",)), + Topic("a lesson grows each time it proves itself", U, + ("update_lesson", "re-keyed", "grows each time"), + "one claim that has met the same failure four times is worth more " + "than four claims that each met it once", + # No index marker. The index names session-START reflexes, and this + # one fires mid-work, the moment a lesson arrives in a situation it + # actually names. `lessons inform` already points at the kind. + shared_with=("docstrings",)), Topic("preferences shape how work is done, never what is recorded", U, ("never what gets recorded",), "a preference never makes a task into a note"), From a4aae974a230d33abc455ef020056c160f840eb1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 19 Sep 2026 23:26:55 -0400 Subject: [PATCH 2/3] feat(telemetry): a usage event records which project the reader was in (#4196, #3735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RetrievalLog` has carried `project_id` since it existed, so "this record was SURFACED on project B" was always answerable. `note_usage_events` had none, so "this record was OPENED on project B" was not — and the two cannot be joined to recover it, because there is deliberately no session identity server-side. NoteUsageEvent's own docstring rules that out. That gap sat exactly on the question milestone 385 exists to answer. A lesson's whole claim is that it reaches a session on a project it was not written on, and step 8's acceptance is "retrieved on a different project AND opened". Each half was answerable; the conjunction was not. WHICH project, because the name is ambiguous and the wrong reading makes the column useless: it is the project the READER was in, never the one the record belongs to. The record's own project is already on the note; copying it here would answer a question nobody asked while looking like it answered this one. The surfacing half is free — every arm already holds the scope it just searched, so auto_inject, lesson_slot, the write-path arms and enter_project now record it. process_skill_sync does not and should not: it installs every Process the operator can reach, which is not a project-scoped question, so a project there would be a fiction. The pull half needs the caller, since a getter knows only what it was handed. The five single-record getters take `project_id: int = 0` and pass it through, following the convention `search` and `create_*` already set. Null stays an ordinary answer meaning "not reported" — a pull with no project is still a pull and still counts toward dead weight; it simply cannot speak to transfer. The four REST detail views report none for now: a human opening a record in a browser is a different event from an agent recalling one, and #2245 left that asymmetry deliberately undecided. Guarded the way #2245 and #2476 taught: by source inspection, because a parameter that was never threaded through changes no return value and shows up only as a column that is mysteriously always null. Three guards — the signature, the pass-through, and the arms — plus the can-fail test rule 167 asks for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- ..._usage_events_carry_the_reading_project.py | 62 ++++++++++ src/scribe/mcp/tools/lessons.py | 11 +- src/scribe/mcp/tools/notes.py | 11 +- src/scribe/mcp/tools/processes.py | 11 +- src/scribe/mcp/tools/projects.py | 1 + src/scribe/mcp/tools/snippets.py | 11 +- src/scribe/mcp/tools/tasks.py | 11 +- src/scribe/models/note_usage.py | 17 +++ src/scribe/services/note_usage.py | 52 ++++++++- src/scribe/services/plugin_context.py | 6 +- tests/test_pull_telemetry.py | 107 ++++++++++++++++++ 11 files changed, 286 insertions(+), 14 deletions(-) create mode 100644 alembic/versions/0105_note_usage_events_carry_the_reading_project.py diff --git a/alembic/versions/0105_note_usage_events_carry_the_reading_project.py b/alembic/versions/0105_note_usage_events_carry_the_reading_project.py new file mode 100644 index 0000000..721fef3 --- /dev/null +++ b/alembic/versions/0105_note_usage_events_carry_the_reading_project.py @@ -0,0 +1,62 @@ +"""note_usage_events records WHICH project the reader was in (#4196, #3735) + +Revision ID: 0105 +Revises: 0104 +Create Date: 2026-09-20 + +Milestone 385. `RetrievalLog` has carried `project_id` from the start, so +"this record was SURFACED on project B" has always been answerable. The usage +table never had one, so "this record was OPENED on project B" never was — and +the two cannot be joined to recover it, because there is no session identity +server-side (see the NoteUsageEvent docstring, which rules that out on +purpose). + +That gap sits exactly on the question milestone 385 exists to answer. A +lesson's whole claim is that it reaches a session on a project it was not +written on; step 8's acceptance is "retrieved on a different project, at the +moment it applies, AND opened". Surfacing cross-project: answerable. Opening: +answerable. Both at once: not, until this column. + +WHICH PROJECT THIS IS, because the name is ambiguous and the wrong reading +makes the column useless. It is the project the READER was working in at the +moment of the event — NOT the project the record belongs to. The record's own +project is already on `notes.project_id`; copying it here would answer a +question nobody asked and quietly look like it answered this one. + +NULLABLE, and not backfilled. Every existing row was written by a reader who +was somewhere, but naming it would be inventing a fact. Null means "not +reported", which is a different answer from a project id that might be wrong — +the same reasoning 0104 applied to its calibration stamps. + +NULL IS ALSO THE ONGOING DEGRADED CASE, not only a historical one. A surfacing +arm always knows the project it searched; a getter only knows what its caller +passed. A pull with no project is still a pull and still counts toward "is this +dead weight" — it simply cannot answer "was it opened away from home". + +THE INDEX carries `note_id` first, matching `ix_note_usage_note_event`: every +readout starts from a set of note ids and narrows, never from a project. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0105" +down_revision = "0104" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "note_usage_events", + sa.Column("project_id", sa.Integer(), nullable=True), + ) + op.create_index( + "ix_note_usage_note_project", + "note_usage_events", + ["note_id", "project_id"], + ) + + +def downgrade() -> None: + op.drop_index("ix_note_usage_note_project", table_name="note_usage_events") + op.drop_column("note_usage_events", "project_id") diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 1d981ff..0324919 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -177,7 +177,7 @@ async def create_lesson( return data -async def get_lesson(lesson_id: int) -> dict: +async def get_lesson(lesson_id: int, project_id: int = 0) -> dict: """Fetch one lesson by id, with its trigger and sources read back out. IF THIS LESSON JUST PROVED ITSELF, IT IS WORTH MORE THAN IT SAYS. You are @@ -188,6 +188,10 @@ async def get_lesson(lesson_id: int) -> dict: re-keyed trigger — and the trigger is the edit that pays most, because a lesson keyed to a situation nobody is in looks exactly like one nobody needed. + + `project_id` is the project you are WORKING IN, not this record's own. + Passing the active project is what makes "opened away from where it was + written" answerable; 0 leaves it unreported and the pull still counts. """ uid = current_user_id() note = await lessons_svc.get_lesson(uid, lesson_id) @@ -199,7 +203,10 @@ async def get_lesson(lesson_id: int) -> dict: # retrieval as any other note, so a getter that records nothing would leave # the kind permanently at zero pulls — reading as dead weight beside kinds # that merely had a counter (#2476, the repeat of #2245). - record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_lesson") + record_pulled( + user_id=uid, note_id=int(note.id), + source="mcp_get_lesson", project_id=project_id, + ) return out diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index 2c649bb..158cbb0 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -67,7 +67,7 @@ async def list_notes( -async def get_note(note_id: int) -> dict: +async def get_note(note_id: int, project_id: int = 0) -> dict: """Fetch the full content of a single Scribe note by its ID. Returns id, title, body (markdown), tags, project_id, created_at, @@ -80,6 +80,10 @@ async def get_note(note_id: int) -> dict: this one up to date. It is still here and still readable — supersession demotes, it never hides — but read it as what was true when written, and open the newer note before acting on it. + + `project_id` is the project you are WORKING IN, not this record's own. + Passing the active project is what makes "opened away from where it was + written" answerable; 0 leaves it unreported and the pull still counts. """ uid = current_user_id() loaded = await notes_svc.get_note_for_user(uid, note_id) @@ -92,7 +96,10 @@ async def get_note(note_id: int) -> dict: # menu surfaces notes, tasks and processes too, so restricting this to # snippets would leave those permanently at zero pulls and make them look # like dead weight next to snippets that merely had a counter (#2085). - record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_note") + record_pulled( + user_id=uid, note_id=int(note.id), + source="mcp_get_note", project_id=project_id, + ) await supersession_svc.attach_relations(uid, note_id, out, hint=True) await systems_tools.attach_systems( uid, getattr(note, "user_id", uid) or uid, out, note.id, note.project_id diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index df755be..23f12fc 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -102,7 +102,7 @@ async def create_process( return note.to_dict() -async def get_process(name_or_id: str) -> dict: +async def get_process(name_or_id: str, project_id: int = 0) -> dict: """Fetch a stored process by name or id and return its full prompt — the fire mechanism. The operator says "run the process"; call this and follow the returned body (including any 'clarify first' steps it contains). @@ -127,6 +127,10 @@ async def get_process(name_or_id: str) -> dict: follow-it-as-written contract above applies only to the operator's own processes — a shared one is a proposal, and running it unasked would put someone else's judgement in charge of this session. + + `project_id` is the project you are WORKING IN, not this record's own. + Passing the active project is what makes "opened away from where it was + written" answerable; 0 leaves it unreported and the pull still counts. """ uid = current_user_id() note, candidates = await notes_svc.resolve_process(uid, name_or_id) @@ -141,7 +145,10 @@ async def get_process(name_or_id: str) -> dict: # this, the getter the product points at is the one getter that records # nothing, and every process sits permanently at zero pulls looking like dead # weight beside kinds that merely had a counter (#2476, the repeat of #2245). - record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_process") + record_pulled( + user_id=uid, note_id=int(note.id), + source="mcp_get_process", project_id=project_id, + ) return out diff --git a/src/scribe/mcp/tools/projects.py b/src/scribe/mcp/tools/projects.py index d5df64c..56491c2 100644 --- a/src/scribe/mcp/tools/projects.py +++ b/src/scribe/mcp/tools/projects.py @@ -214,6 +214,7 @@ async def enter_project(project_id: int) -> dict: user_id=uid, note_ids=[int(t.id) for t in open_tasks], source="enter_project", + project_id=project_id, ) # A project need not have one, and most installs won't — null is ordinary # here, not a missing prerequisite. Summary only: the guidance is ~9k of diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index 5b5c563..e4ad0ad 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -209,7 +209,7 @@ async def create_snippet( return data -async def get_snippet(snippet_id: int) -> dict: +async def get_snippet(snippet_id: int, project_id: int = 0) -> dict: """Fetch a snippet by id — the full record: code, signature, location, and a parsed `snippet` field of its structured parts. @@ -236,6 +236,10 @@ async def get_snippet(snippet_id: int) -> dict: `owner` and your `permission`. Read that as ONE PERSON'S SUGGESTION, not as established practice here: judge it on its merits, say whose it is when you reference it, and don't adopt it as the house pattern without checking. + + `project_id` is the project you are WORKING IN, not this record's own. + Passing the active project is what makes "opened away from where it was + written" answerable; 0 leaves it unreported and the pull still counts. """ uid = current_user_id() note = await snippets_svc.get_snippet(uid, snippet_id) @@ -250,7 +254,10 @@ async def get_snippet(snippet_id: int) -> dict: # snippets_svc.get_snippet — the service is also reached by update/merge # paths, and counting those would inflate exactly the number that is # supposed to mean "someone chose to look at this" (#2085). - record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_snippet") + record_pulled( + user_id=uid, note_id=int(note.id), + source="mcp_get_snippet", project_id=project_id, + ) await systems_tools.attach_systems( uid, note.user_id, data, note.id, note.project_id ) diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 093dfce..24a740f 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -78,7 +78,7 @@ async def list_tasks( return {"tasks": [notes_svc.brief_row(n, titles) for n in rows], "total": total} -async def get_task(task_id: int) -> dict: +async def get_task(task_id: int, project_id: int = 0) -> dict: """Fetch a single Scribe task by ID. Returns id, title, body, status, priority, tags, project_id, milestone_id, @@ -91,6 +91,10 @@ async def get_task(task_id: int) -> dict: A task another user shared with you also carries `shared`, `owner` and `permission` — it's their work item, not one you took on. + + `project_id` is the project you are WORKING IN, not this record's own. + Passing the active project is what makes "opened away from where it was + written" answerable; 0 leaves it unreported and the pull still counts. """ uid = current_user_id() loaded = await notes_svc.get_note_for_user(uid, task_id) @@ -123,7 +127,10 @@ async def get_task(task_id: int) -> dict: await systems_tools.attach_systems( uid, getattr(note, "user_id", uid) or uid, data, note.id, note.project_id ) - record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_task") + record_pulled( + user_id=uid, note_id=int(note.id), + source="mcp_get_task", project_id=project_id, + ) return data diff --git a/src/scribe/models/note_usage.py b/src/scribe/models/note_usage.py index 433cccb..1685469 100644 --- a/src/scribe/models/note_usage.py +++ b/src/scribe/models/note_usage.py @@ -66,12 +66,29 @@ class NoteUsageEvent(Base, CreatedAtMixin): # so never aggregate across the prefix without saying why (#1038, #2085). source: Mapped[str] = mapped_column(Text, nullable=False) + # The project the READER was in when this happened — not the project the + # record belongs to, which is already on `notes.project_id`. The whole + # point is the comparison between the two: a record opened somewhere other + # than where it was written is the evidence that it TRANSFERRED, which is + # the claim the lesson kind rests on (milestone 385) and the one thing + # RetrievalLog could half-answer and this table could not answer at all. + # + # Nullable, and null is ordinary rather than historical. A surfacing arm + # always knows the project it searched; a getter knows only what its caller + # passed, and a caller that passed nothing is reporting "unknown", not + # "none". Treat a null as unreported: it still counts toward "is this dead + # weight", and it cannot speak to "was it opened away from home". + project_id: Mapped[int | None] = mapped_column(Integer, nullable=True) + __table_args__ = ( # The readout is always "these note ids, split by event" — a covering # composite beats separate single-column indexes for it. Index("ix_note_usage_note_event", "note_id", "event"), Index("ix_note_usage_created_at", "created_at"), Index("ix_note_usage_user_id", "user_id"), + # note_id first, like ix_note_usage_note_event: every readout starts + # from a set of note ids and narrows, never from a project. + Index("ix_note_usage_note_project", "note_id", "project_id"), ) def to_dict(self) -> dict: diff --git a/src/scribe/services/note_usage.py b/src/scribe/services/note_usage.py index 21c3596..5b0400e 100644 --- a/src/scribe/services/note_usage.py +++ b/src/scribe/services/note_usage.py @@ -85,13 +85,40 @@ def _schedule(rows: list[dict]) -> None: task.add_done_callback(_pending.discard) +def _project_or_none(project_id: int | None) -> int | None: + """0 and None both mean "no project reported" — store one of them. + + Callers reach this from two conventions at once: the MCP tools spell "no + project" as `0` (it is an int parameter with an int default), while the + column is nullable. Folding them here keeps every call site from having to + remember which one this function wants, and stops a row claiming it was + read on project #0. + """ + try: + pid = int(project_id or 0) + except (TypeError, ValueError): + return None + return pid or None + + def record_surfaced( - *, user_id: int | None, note_ids: list[int] | set[int], source: str + *, + user_id: int | None, + note_ids: list[int] | set[int], + source: str, + project_id: int | None = None, ) -> None: """Fire-and-forget: record that these notes were shown to the agent. Takes the whole menu at once — one insert per surfacing event, not per note — because a menu is a single decision and its rows should land together. + + `project_id` is where the READER was, not where the record lives. Every + surfacing arm knows it — it is the scope it just searched — so pass it; + it is what makes "surfaced away from home" answerable without joining + RetrievalLog. 0 is normalised to None: a project id of zero means "no + project" everywhere else in this codebase, and storing it would read as + project #0. """ try: rows = [ @@ -100,6 +127,7 @@ def record_surfaced( "note_id": int(nid), "event": SURFACED, "source": source, + "project_id": _project_or_none(project_id), } for nid in note_ids ] @@ -109,8 +137,25 @@ def record_surfaced( _schedule(rows) -def record_pulled(*, user_id: int | None, note_id: int, source: str) -> None: - """Fire-and-forget: record that a note was opened in full.""" +def record_pulled( + *, + user_id: int | None, + note_id: int, + source: str, + project_id: int | None = None, +) -> None: + """Fire-and-forget: record that a note was opened in full. + + `project_id` is where the READER was — the caller's active project, never + the record's own. Compared against the record's `project_id`, it answers + whether this was opened somewhere other than where it was written, which + is the evidence that a record TRANSFERRED (milestone 385, #3735). + + Unlike a surfacing arm, a getter only knows what it was handed, so this + stays optional and null is an ordinary answer meaning "not reported". A + pull with no project is still a pull: it counts toward dead-weight + detection and simply cannot speak to transfer. + """ try: rows = [ { @@ -118,6 +163,7 @@ def record_pulled(*, user_id: int | None, note_id: int, source: str) -> None: "note_id": int(note_id), "event": PULLED, "source": source, + "project_id": _project_or_none(project_id), } ] except Exception: diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 399bea4..150cbb5 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -872,6 +872,7 @@ async def _reserve_slot_for_lesson( if slot_id not in already: record_surfaced( user_id=user_id, note_ids=[slot_id], source="lesson_slot", + project_id=project_id, ) return kept + slot, slot_id @@ -1076,6 +1077,7 @@ async def build_autoinject_hint( if i not in already and i != lesson_slot_id ], source="auto_inject", + project_id=project_id, ) return {"context": "\n".join(lines), "note_ids": note_ids, "config": cfg} @@ -2178,7 +2180,9 @@ async def build_write_path_hint( else "write_path_semantic") by_arm.setdefault(arm, []).append(int(item["id"])) for arm, ids in by_arm.items(): - record_surfaced(user_id=user_id, note_ids=ids, source=arm) + record_surfaced( + user_id=user_id, note_ids=ids, source=arm, project_id=project_id, + ) # ── Standing rules that may apply here (milestone 307) ────────────── # diff --git a/tests/test_pull_telemetry.py b/tests/test_pull_telemetry.py index cffddd9..dfbf399 100644 --- a/tests/test_pull_telemetry.py +++ b/tests/test_pull_telemetry.py @@ -201,3 +201,110 @@ def test_every_named_loader_still_exists(): f"renamed or moved. Update the list, or the pull check silently stops " f"covering whatever used them." ) + + +# ── the reading project travels with the event (#4196, #3735) ──────────────── +# +# The same sibling-drift shape as above, one dimension over. `RetrievalLog` has +# always carried `project_id`, so "surfaced on project B" was answerable; the +# usage table had none, so "opened on project B" was not, and the two cannot be +# joined (no session identity server-side — NoteUsageEvent's own docstring). +# That gap sat exactly on milestone 385's acceptance: a lesson's claim is that +# it reaches a session on a project it was NOT written on. +# +# A getter that takes the project and forgets to pass it fails the same way a +# missing record_pulled did: silently, changing no return value, and showing up +# only as a column that is mysteriously always null. + + +def _pulling_getters(): + """(module, name, source) for every getter that records a pull.""" + for module, name, body in _getters(): + if "record_pulled" in body: + yield module, name, body + + +def test_every_getter_that_pulls_takes_the_reading_project(): + """Asserted on structure (rule 167): a behavioural test cannot see a + parameter that was never threaded through.""" + missing = [ + f"{module}.{name}" + for module, name, body in _pulling_getters() + if "project_id: int = 0" not in body.split("\n")[0] + ] + assert not missing, ( + f"these getters record a pull but cannot say where the reader was: " + f"{missing}. Add `project_id: int = 0` to the signature — see " + f"mcp/tools/notes.py:get_note." + ) + + +def test_every_getter_that_pulls_passes_the_project_through(): + """Taking the argument and dropping it is worse than not taking it: the + signature advertises a dimension the column never receives.""" + checked, missing = [], [] + for module, name, body in _pulling_getters(): + checked.append(f"{module}.{name}") + if "project_id=project_id" not in body: + missing.append(f"{module}.{name}") + assert checked, "found no pulling getters; the detection must have moved" + assert not missing, ( + f"these getters accept a project and do not hand it to record_pulled: " + f"{missing}." + ) + + +# The arms are the other half: unlike a getter, a surfacing arm ALWAYS knows +# the project — it is the scope it just searched — so there is no excuse for a +# null there, and `lesson_slot` is the one milestone 385 is measured through. +# +# `process_skill_sync` is exempt and named here rather than pattern-matched: it +# installs every Process the operator can reach, which is not a project-scoped +# question, so a project on that row would be a fiction. +SURFACING_ARMS_WITHOUT_A_PROJECT = {"process_skill_sync"} + + +def test_every_project_scoped_surfacing_arm_records_its_project(): + import ast as _ast + + src = (_SRC / "services" / "plugin_context.py").read_text() + missing = [] + for node in _ast.walk(_ast.parse(src)): + if not (isinstance(node, _ast.Call) + and getattr(node.func, "id", "") == "record_surfaced"): + continue + kwargs = {k.arg: k.value for k in node.keywords} + source = kwargs.get("source") + named = (source.value + if isinstance(source, _ast.Constant) else "") + if named in SURFACING_ARMS_WITHOUT_A_PROJECT: + continue + if "project_id" not in kwargs: + missing.append(f"{named} (line {node.lineno})") + assert not missing, ( + f"these surfacing arms know the project they searched and do not " + f"record it: {missing}. Pass project_id=project_id, or add the arm to " + f"SURFACING_ARMS_WITHOUT_A_PROJECT with the reason it has none." + ) + + +def test_the_reading_project_guards_can_fail(): + """Rule 167: shown turning red once, so a guard that has quietly stopped + matching anything is distinguishable from one that passes.""" + assert list(_pulling_getters()), "the pulling-getter detection matches nothing" + fake = "async def get_thing(thing_id: int) -> dict:\n record_pulled(x)" + assert "project_id: int = 0" not in fake.split("\n")[0] + assert "project_id=project_id" not in fake + + +def test_zero_and_none_both_mean_no_project_reported(): + """0 is how the MCP tools spell "no project" (int parameter, int default); + the column is nullable. A row claiming project #0 would be a fiction.""" + from scribe.services.note_usage import _project_or_none + + assert _project_or_none(0) is None + assert _project_or_none(None) is None + assert _project_or_none("") is None + assert _project_or_none("nonsense") is None + assert _project_or_none(2) == 2 + assert _project_or_none("2") == 2 From 97867d47ff585ece6e1c09e3f79a83927e652784 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 19 Sep 2026 23:32:54 -0400 Subject: [PATCH 3/3] fix(telemetry): the reading project survives a backup (#4196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 7068 failed two tests on a4aae97, and the second was a real bug the guard caught before it shipped. THE COLUMN GUARD (#3182) was right. `note_usage_events` excludes only `id` from backup, so telemetry IS exported — and a new column that the serialiser never learned about would have restored as null on every row. That is the exact failure #3182 was built from, where nine columns vanished from `notes` the same way: added to the model and the migration, which fail loudly, and never to the serialiser, which fails silently. So `project_id` is exported, not excluded, via the one `_usage_event_rows` builder both exporters share. The importer re-maps it through `project_id_map` and DEGRADES rather than skipping. `code_shape_events` drops a row whose project will not map, because a shape event without its project says nothing; a usage event is not like that. The project is optional by design and null already means "not reported", so an unmappable one restores as unreported. Dropping it would lose a real pull and deflate the pull-through this table exists to report. BACKUP_VERSION deliberately unchanged: the suite states that column additions do not bump it, only new sections do, and a third test asserts the current number. The second failure was mine and smaller — test_record_pulled_writes_a_ single_row compares exact dicts, so it now expects the null project. I had claimed these assertions were key-based; they are not, and I had read a summary line rather than the assertion. Added an import-side guard while here. The column family checks the EXPORT side only, so a column can be exported faithfully and dropped on the way back in — a restore that reports success and has quietly lost a dimension. No general guard for that direction exists yet; this covers the column added here and names the gap for whoever builds the general one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/backup.py | 8 +++++++- tests/test_note_usage.py | 3 ++- tests/test_services_backup.py | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index cf7573e..735b559 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -333,7 +333,7 @@ def _usage_event_rows(rows) -> list[dict]: return [ { "user_id": r.user_id, "note_id": r.note_id, "event": r.event, - "source": r.source, + "source": r.source, "project_id": r.project_id, "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows @@ -1548,6 +1548,12 @@ async def _restore_v2(data: dict) -> dict: note_id=mapped_nid, event=ev.get("event", ""), source=ev.get("source", ""), + # Degrades to None rather than dropping the row: unlike a shape + # event, a usage event without a project is still a real pull, + # and discarding it would deflate the pull-through this table + # exists to report. Null already means "not reported". + project_id=(project_id_map.get(ev["project_id"]) + if ev.get("project_id") else None), created_at=_dt(ev.get("created_at")), )) stats["note_usage_events"] += 1 diff --git a/tests/test_note_usage.py b/tests/test_note_usage.py index 5ab69cf..26ba708 100644 --- a/tests/test_note_usage.py +++ b/tests/test_note_usage.py @@ -41,7 +41,8 @@ async def test_record_pulled_writes_a_single_row(): record_pulled(user_id=7, note_id=11, source="mcp_get_snippet") rows = sched.call_args[0][0] assert rows == [ - {"user_id": 7, "note_id": 11, "event": "pulled", "source": "mcp_get_snippet"} + {"user_id": 7, "note_id": 11, "event": "pulled", + "source": "mcp_get_snippet", "project_id": None} ] diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 40ed6f2..33e63e9 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -245,6 +245,42 @@ def test_every_column_is_exported_or_declared_excluded(table): ) +def test_the_usage_importer_restores_the_reading_project(): + """The column guard above checks the EXPORT side only. + + A column can be exported faithfully and then dropped on the way back in, + which restores a backup that reports success and has quietly lost a + dimension — #3182's failure mode, one direction over. There is no general + import-side guard yet; this covers the column #4196 added, by source + inspection, because the behavioural path needs Postgres. + + It also pins the DEGRADE. `code_shape_events` skips a row whose project + will not map, because a shape event without its project says nothing. A + usage event is not like that: the project is optional by design and null + already means "not reported", so an unmappable one must restore as + unreported rather than vanish — dropping it would lose a real pull and + deflate the very pull-through this table exists to report. + """ + import inspect + + src = inspect.getsource(backup._restore_v2) + marker = 'for ev in data.get("note_usage_events", []):' + assert marker in src, "the usage import loop moved; this guard is blind" + block = src[src.index(marker):][:1200] + + assert 'ev.get("project_id")' in block, ( + "the usage importer drops project_id — a restore would report success " + "and come back without the reading project" + ) + assert "project_id_map" in block, ( + "project_id must be re-mapped; a raw id points at whatever project " + "happens to hold that number in the destination install" + ) + assert "continue" not in block.split('project_id=')[1][:200], ( + "an unmappable project must degrade to None, not skip the row" + ) + + def test_the_column_guard_covers_every_table_with_a_row_helper(): """The guard is only as good as its registry — a table added to _BACKED_UP with a new helper, and not to the registry, would be unguarded and look