diff --git a/src/scribe/models/note_usage.py b/src/scribe/models/note_usage.py index cb3671c..85eb5d4 100644 --- a/src/scribe/models/note_usage.py +++ b/src/scribe/models/note_usage.py @@ -51,10 +51,23 @@ class NoteUsageEvent(Base): note_id: Mapped[int] = mapped_column(Integer, nullable=False) # 'surfaced' | 'pulled' event: Mapped[str] = mapped_column(Text, nullable=False) - # Which surface produced it: 'auto_inject' | 'write_path_place' | - # 'write_path_semantic' | 'mcp_get_snippet' | 'mcp_get_note' | 'rest_note'. - # Kept granular so the place arm and the semantic arm can be compared — - # that comparison is the whole reason the place arm needed logging at all. + # Which surface produced it. Kept granular so the place arm and the + # semantic arm can be compared — that comparison is the whole reason the + # place arm needed logging at all. + # + # A CONVENTION, not a fixed vocabulary: `mcp_` for an agent call, + # `rest_` for a human opening a detail view, and a bare name for a + # hook or background surface ('auto_inject', 'write_path_place', + # 'write_path_semantic'). This comment deliberately no longer lists the + # members — the previous list had gone stale, naming 'rest_note' that + # nothing wrote while omitting sources that existed, and a half-true + # enumeration reads as authoritative in exactly the way that misleads + # (#2476). `grep -rn record_pulled\\\|record_surfaced src/` is the + # authoritative list, and unlike a comment it cannot drift. + # + # The mcp_/rest_ split is load-bearing. "Is this dead weight?" is served by + # any pull; "was that injected line useful?" is served by AGENT pulls only, + # so never aggregate across the prefix without saying why (#1038, #2085). source: Mapped[str] = mapped_column(Text, nullable=False) __table_args__ = ( diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index 3dcca5a..9e48ded 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -22,6 +22,7 @@ from scribe.services.notes import ( update_note, ) from scribe.services.note_drafts import upsert_draft, get_draft, delete_draft +from scribe.services.note_usage import record_pulled from scribe.services.note_versions import list_versions, get_version logger = logging.getLogger(__name__) @@ -178,6 +179,13 @@ async def get_note_route(note_id: int): note, permission = result data = note.to_dict() data["permission"] = permission + # Opening the detail view IS a pull — the operator chose to look. Tagged by + # SURFACE, not by the record's kind, matching rest_snippet: the kind is a + # join away, but which surface asked is not recoverable after the fact. + # Keeping rest_* apart from mcp_* is load-bearing, not tidiness — "was that + # injected line useful?" is answered by agent pulls alone, and a human + # clicking a link would inflate exactly the number #1038 and #2085 gate on. + record_pulled(user_id=uid, note_id=note_id, source="rest_note") return jsonify(data) diff --git a/src/scribe/routes/tasks.py b/src/scribe/routes/tasks.py index d564b55..52662f9 100644 --- a/src/scribe/routes/tasks.py +++ b/src/scribe/routes/tasks.py @@ -13,6 +13,7 @@ from scribe.services.notes import ( list_notes, update_note, ) +from scribe.services.note_usage import record_pulled from scribe.services.planning import start_planning as svc_start_planning from scribe.services.recurrence import calculate_next_due, validate_recurrence_rule @@ -186,6 +187,9 @@ async def get_task_route(task_id: int): parent = await get_note_for_user(uid, task.parent_id) data["parent_title"] = parent[0].title if parent else None data["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)] + # Opening the detail view IS a pull — see the note beside rest_note in + # routes/notes.py for why the rest_* and mcp_* prefixes stay separable. + record_pulled(user_id=uid, note_id=task_id, source="rest_task") return jsonify(data) diff --git a/tests/test_mcp_pull_telemetry.py b/tests/test_pull_telemetry.py similarity index 51% rename from tests/test_mcp_pull_telemetry.py rename to tests/test_pull_telemetry.py index 760bfe1..cffddd9 100644 --- a/tests/test_mcp_pull_telemetry.py +++ b/tests/test_pull_telemetry.py @@ -1,4 +1,8 @@ -"""Every getter that opens ONE note-backed record must record the pull. +"""Every surface that opens ONE note-backed record must record the pull. + +Covers both halves: the MCP getters an agent calls, and the REST detail views a +human opens. They are one ledger with two prefixes (`mcp_*` / `rest_*`), and a +gap on either side makes the same records look untouched. WHY THIS EXISTS @@ -14,7 +18,9 @@ That has now happened twice: #2476 `get_process` recorded nothing — and the auto-inject menu header names `get_process` as the way to open that kind. Processes were embedded when #2245 was fixed; the fix enumerated the kinds someone thought of - rather than the kinds that exist. + rather than the kinds that exist. The same issue found the REST half: + `rest_snippet` was recorded, note and task detail were not, and the + model's own comment named a `'rest_note'` source nothing wrote. A missing call is the shape no behavioural test catches: it changes no return value (#2278, shape 4). Source inspection is the only thing that sees it. @@ -55,7 +61,9 @@ SINGLE_NOTE_LOADERS = ( "get_snippet", ) -TOOLS_DIR = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" / "mcp" / "tools" +_SRC = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" +TOOLS_DIR = _SRC / "mcp" / "tools" +ROUTES_DIR = _SRC / "routes" def _getters(): @@ -88,6 +96,88 @@ def test_every_single_record_getter_records_a_pull(): ) +def _detail_routes(): + """(module, handler, expanded source) for every bare-id GET route. + + A route registered at exactly `/` for GET is the DETAIL view of one + record — that shape is what distinguishes it from a list, a sub-resource + (`//versions`) or a write. Nothing else about the handler has to be + guessed. + + The source is expanded one level through module-private helpers, because + `get_snippet_route` loads via `_load_snippet` rather than calling the loader + itself. Without the expansion the snippet route — the one that already got + this right — would drop out of the check. + """ + import re + bare_id = re.compile(r"^/$") + for path in sorted(ROUTES_DIR.glob("*.py")): + source = path.read_text() + tree = ast.parse(source) + helpers = { + node.name: ast.get_source_segment(source, node) or "" + for node in tree.body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith("_") + } + for node in tree.body: + if not isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)): + continue + for dec in node.decorator_list: + if not isinstance(dec, ast.Call): + continue + route = next((a.value for a in dec.args + if isinstance(a, ast.Constant)), None) + if not isinstance(route, str) or not bare_id.match(route): + continue + methods = [ + e.value for kw in dec.keywords if kw.arg == "methods" + and isinstance(kw.value, ast.List) + for e in kw.value.elts if isinstance(e, ast.Constant) + ] + if "GET" not in methods: + continue + body = ast.get_source_segment(source, node) or "" + expanded = body + "".join( + src for name, src in helpers.items() if name in body + ) + yield path.name, node.name, expanded + + +def test_every_rest_detail_view_records_a_pull(): + """The human half of the same ledger. + + `rest_snippet` was recorded; note and task detail recorded nothing, so the + UI's most direct evidence of interest — someone opened the record — existed + for one kind out of three. The model's own comment listed `'rest_note'` as + a source, which means the design intended it and the implementation stopped + at snippets. + + Same derivation as the MCP test above, over the other surface: the routes + are discovered, and the ones that must record are identified by the loader + they reach. A `/` GET added for a fourth note-backed kind is covered + the day it is written. + """ + missing = [] + checked = [] + for module, handler, body in _detail_routes(): + if not any(loader in body for loader in SINGLE_NOTE_LOADERS): + continue # not note-backed — groups and projects land here + checked.append(f"{module}:{handler}") + if "record_pulled" not in body: + missing.append(f"{module}:{handler}") + + assert checked, ( + "found no note-backed detail routes; the route shape or the loader " + "names must have moved" + ) + assert not missing, ( + f"these detail views open a record without recording the pull: " + f"{missing}. Add record_pulled(user_id=…, note_id=…, source='rest_') " + f"before returning — see routes/snippets.py:get_snippet_route." + ) + + def test_every_named_loader_still_exists(): """Pins the hand-written list against a rename.