fix(telemetry): the human half of the pull ledger recorded one kind in three
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 11s
CI & Build / integration (push) Successful in 20s
CI & Build / Python tests (push) Successful in 47s
CI & Build / Build & push image (push) Successful in 27s
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 11s
CI & Build / integration (push) Successful in 20s
CI & Build / Python tests (push) Successful in 47s
CI & Build / Build & push image (push) Successful in 27s
Opening a snippet in the UI recorded rest_snippet. Opening a note or a task recorded nothing — so the most direct evidence the product has that anyone cares about a record existed for one kind out of three, and the other two sat at zero pulls looking like dead weight beside a kind that merely had a counter. Not an open question about intent: models/note_usage.py already documented 'rest_note' as a source value. Nothing wrote it. The design named it and the implementation stopped at snippets. Adds rest_note and rest_task. Tagged by SURFACE rather than by the record's kind, matching rest_snippet — the kind is a join away, but which surface asked is not recoverable after the fact. The mcp_/rest_ split stays load-bearing: "is this dead weight" is served by any pull, "was that injected line useful" by agent pulls alone, and a human clicking a link would inflate exactly the number #1038 and #2085 gate on. The vocabulary comment in the model was itself the stale-enumeration shape this survey keeps finding — it named a source nothing wrote while omitting sources that existed. Replaced with the naming CONVENTION plus a pointer to grep, which cannot drift, rather than a longer list that would go stale the same way. Guard extended to the REST surface, same derivation as the MCP half: a route registered at exactly /<int:x> for GET is a detail view, and one reaching a note-backed loader must record. Handler source is expanded one level through module-private helpers, without which get_snippet_route — the route that already got this right — would drop out of the check by loading via _load_snippet. Verified the guard fires when a call is removed. Renamed test_mcp_pull_telemetry.py -> test_pull_telemetry.py; it is no longer only about MCP. Closes #2476
This commit is contained in:
@@ -51,10 +51,23 @@ class NoteUsageEvent(Base):
|
|||||||
note_id: Mapped[int] = mapped_column(Integer, nullable=False)
|
note_id: Mapped[int] = mapped_column(Integer, nullable=False)
|
||||||
# 'surfaced' | 'pulled'
|
# 'surfaced' | 'pulled'
|
||||||
event: Mapped[str] = mapped_column(Text, nullable=False)
|
event: Mapped[str] = mapped_column(Text, nullable=False)
|
||||||
# Which surface produced it: 'auto_inject' | 'write_path_place' |
|
# Which surface produced it. Kept granular so the place arm and the
|
||||||
# 'write_path_semantic' | 'mcp_get_snippet' | 'mcp_get_note' | 'rest_note'.
|
# semantic arm can be compared — that comparison is the whole reason the
|
||||||
# Kept granular so the place arm and the semantic arm can be compared —
|
# place arm needed logging at all.
|
||||||
# that comparison is the whole reason the place arm needed logging at all.
|
#
|
||||||
|
# A CONVENTION, not a fixed vocabulary: `mcp_<tool>` for an agent call,
|
||||||
|
# `rest_<kind>` 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)
|
source: Mapped[str] = mapped_column(Text, nullable=False)
|
||||||
|
|
||||||
__table_args__ = (
|
__table_args__ = (
|
||||||
|
|||||||
@@ -22,6 +22,7 @@ from scribe.services.notes import (
|
|||||||
update_note,
|
update_note,
|
||||||
)
|
)
|
||||||
from scribe.services.note_drafts import upsert_draft, get_draft, delete_draft
|
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
|
from scribe.services.note_versions import list_versions, get_version
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
@@ -178,6 +179,13 @@ async def get_note_route(note_id: int):
|
|||||||
note, permission = result
|
note, permission = result
|
||||||
data = note.to_dict()
|
data = note.to_dict()
|
||||||
data["permission"] = permission
|
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)
|
return jsonify(data)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ from scribe.services.notes import (
|
|||||||
list_notes,
|
list_notes,
|
||||||
update_note,
|
update_note,
|
||||||
)
|
)
|
||||||
|
from scribe.services.note_usage import record_pulled
|
||||||
from scribe.services.planning import start_planning as svc_start_planning
|
from scribe.services.planning import start_planning as svc_start_planning
|
||||||
from scribe.services.recurrence import calculate_next_due, validate_recurrence_rule
|
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)
|
parent = await get_note_for_user(uid, task.parent_id)
|
||||||
data["parent_title"] = parent[0].title if parent else None
|
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)]
|
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)
|
return jsonify(data)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
WHY THIS EXISTS
|
||||||
|
|
||||||
@@ -14,7 +18,9 @@ That has now happened twice:
|
|||||||
#2476 `get_process` recorded nothing — and the auto-inject menu header names
|
#2476 `get_process` recorded nothing — and the auto-inject menu header names
|
||||||
`get_process` as the way to open that kind. Processes were embedded
|
`get_process` as the way to open that kind. Processes were embedded
|
||||||
when #2245 was fixed; the fix enumerated the kinds someone thought of
|
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
|
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.
|
value (#2278, shape 4). Source inspection is the only thing that sees it.
|
||||||
@@ -55,7 +61,9 @@ SINGLE_NOTE_LOADERS = (
|
|||||||
"get_snippet",
|
"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():
|
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 `/<int:x>` for GET is the DETAIL view of one
|
||||||
|
record — that shape is what distinguishes it from a list, a sub-resource
|
||||||
|
(`/<int:x>/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"^/<int:\w+>$")
|
||||||
|
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 `/<int:x>` 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_<kind>') "
|
||||||
|
f"before returning — see routes/snippets.py:get_snippet_route."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_every_named_loader_still_exists():
|
def test_every_named_loader_still_exists():
|
||||||
"""Pins the hand-written list against a rename.
|
"""Pins the hand-written list against a rename.
|
||||||
|
|
||||||
Reference in New Issue
Block a user