FabledScribe/tests/test_pull_telemetry.py

"""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

`note_usage_events` answers "did anyone ever actually open this?" — the
surfaced:pulled ratio is what makes dead weight visible and prunable. A getter
that opens a record without recording it leaves that kind permanently at zero
pulls, so it looks like dead weight beside kinds that merely had a counter.

A fix that enumerates the kinds someone thought of, rather than the kinds that
exist, leaves the next kind's getter silent — and the REST detail views are a
second half that is easy to forget.

A missing call is the shape no behavioural test catches: it changes no return
value. Source inspection is the only thing that sees it.

WHAT MAKES THIS DERIVED RATHER THAN A LIST

The getters are not enumerated here. They are discovered from the tool modules
by AST, and the ones that must record are identified by the loader they call —
so a `get_<newkind>` added tomorrow is covered the moment it loads a note the
way every other getter does.

The loader names ARE a list, and that is the residual weakness. The second test
pins them against a RENAME — the failure mode that would silently empty the
candidate set and let this pass while checking nothing.

It does not discover NEW loaders, and an earlier draft that tried to failed for
the wrong reason: `create_note` and `update_note` also return a `Note`, so an
annotation scan finds writers, not readers. Distinguishing them needs more than
a type, so the honest position is a pinned list plus a non-empty assertion,
and this paragraph saying so.
"""
from __future__ import annotations

import ast
import inspect
import pathlib
import pkgutil

# Loaders that return ONE note-backed record in full. A getter calling any of
# these is opening a record, which is the act `pulled` describes.
#
# `list_notes` is deliberately absent: `get_milestone` calls it to list a
# milestone's steps, and that is a LIST — the milestone itself is not a note,
# and its steps are surfaced rather than opened.
SINGLE_NOTE_LOADERS = (
    "get_note_for_user",
    "resolve_process",
    "get_snippet",
)

_SRC = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe"
TOOLS_DIR = _SRC / "mcp" / "tools"
ROUTES_DIR = _SRC / "routes"


def _getters():
    """(module name, function name, source) for every `get_*` MCP tool."""
    for mod in pkgutil.iter_modules([str(TOOLS_DIR)]):
        path = TOOLS_DIR / f"{mod.name}.py"
        source = path.read_text()
        for node in ast.parse(source).body:
            if isinstance(node, ast.AsyncFunctionDef) and node.name.startswith("get_"):
                yield mod.name, node.name, ast.get_source_segment(source, node) or ""


def test_every_single_record_getter_records_a_pull():
    missing = []
    checked = []
    for module, name, body in _getters():
        if not any(loader in body for loader in SINGLE_NOTE_LOADERS):
            continue
        checked.append(f"{module}.{name}")
        if "record_pulled" not in body:
            missing.append(f"{module}.{name}")

    # If this ever drops to zero the test has stopped testing anything — a
    # renamed loader would silently empty the candidate set and pass.
    assert checked, "found no note-backed getters; the loader names must have moved"
    assert not missing, (
        f"these getters open a record without recording the pull: {missing}. "
        f"Add record_pulled(user_id=…, note_id=…, source='mcp_<tool>') before "
        f"returning — see mcp/tools/notes.py:get_note."
    )


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():
    """Pins the hand-written list against a rename.

    A renamed loader is the failure that matters: the candidate set above would
    quietly empty and the first test would pass while checking nothing. The
    `assert checked` there catches it too; this says WHICH name moved, which is
    the difference between a five-minute fix and a puzzle.
    """
    from scribe.services import notes as notes_svc
    from scribe.services import snippets as snippets_svc

    available = {
        name
        for svc in (notes_svc, snippets_svc)
        for name, obj in vars(svc).items()
        if inspect.iscoroutinefunction(obj)
    }
    gone = [name for name in SINGLE_NOTE_LOADERS if name not in available]
    assert not gone, (
        f"SINGLE_NOTE_LOADERS names {gone} that no longer exist — they were "
        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 ───────────────────────────────
#
# The same sibling-drift shape as above, one dimension over. `RetrievalLog`
# carries `project_id`, so "surfaced on project B" is answerable; without it on
# the usage row, "opened on project B" is not, and the two cannot be joined (no
# session identity server-side — NoteUsageEvent's own docstring). That is
# exactly a lesson's claim: 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 _takes_reading_project(body: str) -> bool:
    """Does this function declare `project_id: int = 0`?

    Parsed, not string-matched against the first line. The first version read
    `body.split("\n")[0]`, which sees only as far as the first newline — so
    adding a parameter to `get_task` wrapped its signature over four lines and
    the guard reported a function that DOES take the project as one that does
    not. A guard that fails on formatting is a guard that gets appeased by
    reflowing the code it was meant to check.
    """
    node = ast.parse(body).body[0]
    args = node.args
    params = list(args.posonlyargs) + list(args.args) + list(args.kwonlyargs)
    for arg in params:
        if arg.arg != "project_id":
            continue
        ann = getattr(arg, "annotation", None)
        return isinstance(ann, ast.Name) and ann.id == "int"
    return False


def test_every_getter_that_pulls_takes_the_reading_project():
    """Asserted on structure: a behavioural test cannot see a
    parameter that was never threaded through."""
    missing = [
        f"{module}.{name}"
        for module, name, body in _pulling_getters()
        if not _takes_reading_project(body)
    ]
    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 a lesson's transfer 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 "<computed>")
        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():
    """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