From a4aae974a230d33abc455ef020056c160f840eb1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 19 Sep 2026 23:26:55 -0400 Subject: [PATCH] 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