Files
FabledScribe/tests/test_usage_attach_seam.py
T
bvandeusenandClaude Opus 5 62f3a485ad
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 52s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
fix(usage): one seam attaches the surfaced-vs-opened chip, and the Knowledge browse uses it (#4230)
`usage_for_notes` is named for notes and works on every note row, yet the chip
reached snippets and rules only. Notes had it nowhere. Lessons had it collected
and shown nowhere a person could reach, because #4196 taught `/api/lessons` to
attach it and `KnowledgeView` — the only lesson list in the UI — browses through
`/api/knowledge`, so `listLessons` still has no consumer.

The cause was not a missing line. SEVEN call sites carried their own copy of the
same few lines: two REST lists, two REST details, two MCP lists, one MCP detail.
Each read perfectly well alone, so "which doors attach usage?" had no answer
anywhere in the code — the same asymmetry test_system_tagging_door_parity.py
records for System tagging (#4249), where whichever door nobody exercised for a
kind is the one that never grew the feature.

`attach_usage(rows, key="id")` is now that answer, and all seven go through it.
A detail payload is a one-row list, so the single-record doors share the seam
rather than keeping a second shape beside it. Deliberately NO try/except: the
fail-open already lives in `usage_for_notes`, which reports through
`_report_failure("readout")` and returns the zero-filled map. Wrapping it again
would swallow the REPORT as well as the error, and a silently-swallowed readout
failure is exactly #2663 — every counter reading zero in production for weeks
while the writes landed fine.

`/api/knowledge` now attaches usage, which closes both holes at once: it is how
notes, lessons and processes are all browsed. `KnowledgeView` renders the badge
on the card footer, looking the advice up per row because the feed is mixed.

The advice moves to utils/deadWeight.ts. Canon #3460 says each caller owns its
own const, and that held while each caller showed ONE kind; a mixed feed would
need five of its own and the next surface another five. The canon's actual
invariant — advice is kind-specific and never baked into the badge — is kept:
it is still a prop. The three existing callers now read the same table, so the
sentence has one home rather than four. Recorded against #3460 so the next
reader is not left re-litigating it.

`_row_id` rejects bools explicitly: `int(True)` is 1, so a row carrying a flag
under the key would be credited with note #1's counts, and a wrong chip is worse
than no chip because it reads as a measurement. A row with no usable id is
skipped rather than failing the page.

Tests pin the PROPERTY, not one route: no door calls the aggregate directly
(AST, so a comment naming it is not a false positive), and every door that shows
usage reaches the seam. Plus the N+1 guard — one aggregate per page, asserted on
await_count, because the per-row version reads more naturally and is invisible
in review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
2026-09-21 20:47:16 -04:00

206 lines
8.3 KiB
Python

"""One seam attaches `usage`, and every door that shows it uses that seam (#4230).
WHAT WENT WRONG. `usage_for_notes` is named for notes and works on every note
row. Yet the surfaced-vs-opened chip reached snippets and rules only: notes had
it nowhere, and lessons had it collected but shown nowhere a person could
reach, because the only lesson LIST in the UI is the Knowledge browse and that
route never attached it.
The cause was not any one missing line. Seven call sites carried their own copy
of the same few lines — two REST lists, two REST details, two MCP lists, one
MCP detail — and each read perfectly well on its own. Nobody was comparing
them, so "which doors attach usage?" had no answer anywhere in the code. That
is the same failure `test_system_tagging_door_parity.py` records for System
tagging (#4249): whichever door nobody exercised for a kind is the one that
never grew the feature, and a human reviewer does not reliably catch it because
each door is only ever read alone.
So this file asserts the PROPERTY, not the behaviour of one route: the attach
logic exists once, and no door re-implements it. A kind added next month either
goes through the seam or fails here.
"""
from __future__ import annotations
import ast
import pathlib
from unittest.mock import AsyncMock, patch
import pytest
from scribe.services.note_usage import attach_usage, empty_usage
ROOT = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe"
# The aggregate the seam is built around. Calling it from a door is the shape
# this file exists to prevent — not because the call is wrong, but because
# seven of them drift.
AGGREGATE = "usage_for_notes"
def _counts(surfaced: int = 5, pulled: int = 0) -> dict:
u = empty_usage()
u["surfaced_count"] = surfaced
u["pull_count"] = pulled
return u
def _aggregate_returns(mapping: dict[int, dict]) -> AsyncMock:
return patch(
"scribe.services.note_usage.usage_for_notes",
AsyncMock(return_value=mapping),
)
# ── the seam itself ───────────────────────────────────────────────────────
async def test_every_row_gets_the_key_even_with_no_events() -> None:
"""Zero-filled, never absent. The UI must not have to tell "no events"
from "no field" — and `UsageBadge` renders nothing below one surfacing, so
an un-surfaced record is quiet without the caller doing anything."""
rows = [{"id": 1}, {"id": 2}]
with _aggregate_returns({1: _counts(surfaced=3)}):
await attach_usage(rows)
assert rows[0]["usage"]["surfaced_count"] == 3
assert rows[1]["usage"] == empty_usage()
async def test_one_aggregate_for_the_whole_page() -> None:
"""The N+1 guard. A per-row lookup here would be N+1 by construction, which
is the one shape a list route must not have — and it is invisible in
review, because the per-row version reads more naturally."""
rows = [{"id": n} for n in range(25)]
mock = AsyncMock(return_value={})
with patch("scribe.services.note_usage.usage_for_notes", mock):
await attach_usage(rows)
assert mock.await_count == 1, "usage must be read once per page, not per row"
assert sorted(mock.await_args.args[0]) == list(range(25))
async def test_a_detail_payload_is_just_a_one_row_list() -> None:
"""The single-record doors share the seam rather than keeping a second
shape beside it. Two shapes for one job is how the seven copies started."""
data = {"id": 7, "title": "x"}
with _aggregate_returns({7: _counts(surfaced=9, pulled=2)}):
await attach_usage([data])
assert data["usage"]["pull_count"] == 2
async def test_a_row_with_no_id_is_skipped_rather_than_failing_the_list() -> None:
"""An unusable id is not a reason to 500 a page of otherwise fine rows."""
rows = [{"id": 1}, {"title": "no id here"}]
with _aggregate_returns({1: _counts()}):
await attach_usage(rows)
assert "usage" in rows[0]
assert "usage" not in rows[1]
async def test_a_boolean_is_not_an_id() -> None:
"""`int(True)` is 1, so a row carrying a flag under the key would silently
be credited with note #1's counts. A wrong chip is worse than no chip: it
reads as a measurement."""
rows = [{"id": True}]
with _aggregate_returns({1: _counts(surfaced=40)}):
await attach_usage(rows)
assert "usage" not in rows[0]
async def test_a_string_id_still_resolves() -> None:
"""Payload rows come from several serialisers; one of them handing back a
stringified id should not silently drop the chip."""
rows = [{"id": "12"}]
with _aggregate_returns({12: _counts(surfaced=4)}):
await attach_usage(rows)
assert rows[0]["usage"]["surfaced_count"] == 4
@pytest.mark.parametrize("key", ["note_id", "record_id"])
async def test_the_key_can_be_named(key: str) -> None:
rows = [{key: 3}]
with _aggregate_returns({3: _counts()}):
await attach_usage(rows, key=key)
assert "usage" in rows[0]
async def test_an_empty_page_asks_nothing_and_breaks_nothing() -> None:
mock = AsyncMock(return_value={})
with patch("scribe.services.note_usage.usage_for_notes", mock):
await attach_usage([])
assert mock.await_args.args[0] == []
# ── the property: one seam, and every door uses it ────────────────────────
def _calls(tree: ast.Module) -> set[str]:
out = set()
for node in ast.walk(tree):
if isinstance(node, ast.Call):
fn = node.func
name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None)
if name:
out.add(name)
return out
def _door_modules() -> list[pathlib.Path]:
return sorted(
[*(ROOT / "routes").glob("*.py"), *(ROOT / "mcp" / "tools").glob("*.py")]
)
def test_no_door_calls_the_aggregate_directly() -> None:
"""THE GUARD. Seven doors each called `usage_for_notes` and zero-filled by
hand; the eighth would have been `/api/knowledge`, and the chip would have
kept reaching some kinds and not others.
Keyed on the CALL, not on the text, so a module that merely names the
function in a comment explaining the seam is not a false positive — and a
hand-kept skip list, which would itself go stale, is not needed (rule 167).
"""
offenders = []
for path in _door_modules():
if AGGREGATE in _calls(ast.parse(path.read_text())):
offenders.append(str(path.relative_to(ROOT.parent.parent)))
assert not offenders, (
f"these doors call {AGGREGATE}() themselves instead of attach_usage(); "
f"that is how the chip came to reach two record kinds out of four: "
f"{offenders}"
)
# (module, the functions that return note-bearing payloads)
#
# Not a list of everything that COULD attach usage — a list of the doors that
# demonstrably show it today. A door dropping its call silently is the exact
# regression this pins.
DOORS = [
("routes/lessons.py", "list_lessons_route or get_lesson_route"),
("routes/snippets.py", "list/get snippet routes"),
("routes/knowledge.py", "list_knowledge — the only note & lesson list in the UI"),
("mcp/tools/lessons.py", "list_lessons / get_lesson"),
("mcp/tools/snippets.py", "list_snippets"),
]
@pytest.mark.parametrize(("module", "why"), DOORS)
def test_every_door_that_shows_usage_goes_through_the_seam(module: str, why: str) -> None:
assert "attach_usage" in _calls(ast.parse((ROOT / module).read_text())), (
f"{module} no longer attaches usage ({why}). If that is deliberate, "
f"remove it from DOORS and say why; a door that silently stops "
f"attaching looks exactly like a corpus nobody uses."
)
def test_the_knowledge_browse_is_covered_because_it_is_the_only_note_list() -> None:
"""Pinned on its own, with the reason, because it is the non-obvious one.
`/api/lessons` already attached usage and it did not help: no view calls
it. `KnowledgeView` is the only list in the UI that renders notes and
lessons, so `/api/knowledge` is the only route through which those two
kinds can show the counter at all. Deleting this line would restore the
original bug while every other test here still passed.
"""
assert any(m == "routes/knowledge.py" for m, _ in DOORS)
assert "attach_usage" in _calls(ast.parse((ROOT / "routes" / "knowledge.py").read_text()))