From 446d6da0d7aac3bd3395b3ffa961c1efbbf4436b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 23 Aug 2026 22:36:04 -0400 Subject: [PATCH 1/3] =?UTF-8?q?fix(snippets):=20an=20annotated=20record=20?= =?UTF-8?q?is=20not=20a=20diverged=20one=20=E2=80=94=20a=20standing=20verd?= =?UTF-8?q?ict=20vouches=20at=20its=20commit=20(#2782)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull-time freshness confirms a cached body by containment: normalised cached code must appear inside the fetched file. That is right for a record kept verbatim and permanently wrong for a deliberately annotated one. A record whose job is to say WHY the shape is what it is carries commentary the source does not, so containment fails on every pull, forever — #2508 was reading `diverged` although its declarations match the source exactly, and always would. Annotation is a sanctioned record style, so this was two deliberate designs colliding, and it was quietly poisoning the one honest drift signal: decision #2707's scoreboard watches body_freshness for `diverged` and was accruing false positives it could never age out. The escape hatch is the verdict itself. verify_snippet is exactly where a human or agent already judged this body a faithful rendering of that source, and `verification.commit_sha` records the repo commit they judged it at — a field whose own docstring (#2688) anticipated this: "makes 'the REPO moved on since the check' computable, once the forge integration can compare it against the current head." This is that comparison. When containment fails, a standing verdict can still vouch, on four conditions and no fewer: - the verdict says `ok`; - it has not EXPIRED — verification_view recomputes code_sha against the record's current body, so editing the record retires the verdict; - it was not INVALIDATED by a push touching the location (#2691); - the file just fetched is at the very commit the verdict was stamped at. That last one is what keeps it honest: the hatch vouches for a body against ONE known commit, never against whatever the source became since. The moment the file moves, containment resumes as the authority and the record reads `diverged` until someone re-verifies — correct, because at that point nobody has looked. The first three are checked by reusing verification_view rather than restating its rule, so "expired" keeps meaning one thing. Nothing is rewritten and no new freshness value is minted; `verification` already travels in the same payload, so a reader can see the basis rather than take "current" on faith. Verdicts predating commit stamping carry no commit to compare and therefore do not vouch — they fall through to containment rather than passing on age alone. Tests pin the fix and, more usefully, every condition that switches it back off: moved commit, expired verdict, push-invalidated verdict, non-ok verdict, no verdict, and a pre-#2688 verdict with no commit_sha. Plus a regression that a verbatim record still takes the containment path untouched. Co-Authored-By: Claude Fable 5 --- src/scribe/mcp/tools/snippets.py | 7 +++ src/scribe/services/snippets.py | 48 ++++++++++++++++++ tests/test_snippet_live_body.py | 86 ++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index 78425ce..f591eeb 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -207,6 +207,13 @@ async def get_snippet(snippet_id: int) -> dict: the source moved on — trust the location over the cached body and consider verify_snippet after you look. + A record kept VERBATIM is confirmed by containment. A deliberately + ANNOTATED one — commentary the source does not carry — cannot be, so it + reads "current" on the authority of a standing `ok` verdict stamped at + the very commit just fetched (#2782); `verification` in the same payload + shows that basis. Edit the record, or let the file move past that commit, + and it reads "diverged" again until someone re-runs verify_snippet. + When the shape ledger has judgments against this snippet, the response carries `instances` (shapes classified as conforming to it — the structured consumer map) and/or `variants` (named departures, each with diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 5365b68..064b685 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -987,6 +987,46 @@ async def _refresh_provenance(note, commit_sha: str) -> None: await notes_svc.update_note(note.user_id, note.id, data=data) +def _verdict_still_vouches(note, fields: dict, fetched_commit_sha: str) -> bool: + """Does a standing `ok` verdict still speak for this body, at this commit? + + Containment (cached code ∈ fetched file) is the fast path, and it is right + for a record kept verbatim. It is WRONG for a deliberately annotated one + (#2782): a record whose job is to say why the shape is what it is carries + commentary the source does not, so containment fails forever and the record + reads `diverged` on every pull. That turns the one honest drift signal into + a permanent false positive — and annotation is a sanctioned record style, + so this is two deliberate designs colliding, not a malformed record. + + The escape hatch is the verdict itself. `verify_snippet` is precisely where + a human or agent already judged this body a faithful rendering of that + source, and `verification.commit_sha` records the repo commit they judged + it at — a field whose own docstring (#2688) anticipated this use: "makes + 'the REPO moved on since the check' computable, once the forge integration + can compare it against the current head." This is that comparison. + + All four conditions, and none is optional: + - the verdict says `ok`; + - it has not EXPIRED — `verification_view` recomputes `code_sha` against + the record's current body, so editing the record retires the verdict; + - it was not INVALIDATED by a push touching the location (#2691); + - the file we just fetched is at the very commit the verdict was stamped + at. Any later commit means nobody has judged what is there now. + + The last one is what keeps this honest: it vouches for a body against ONE + known commit, never against whatever the source has become since. The + moment the file moves, containment resumes as the authority and the record + reads `diverged` until someone re-verifies — which is the correct outcome, + because at that point nobody has looked. + """ + if not fetched_commit_sha: + return False + view = verification_view(note, fields) + if view.get("status") != VERIFY_OK or view.get("needs_attention"): + return False + return view.get("commit_sha") == fetched_commit_sha + + async def attach_live_body(note, data: dict) -> None: """Decorate a PULL response with forge-checked freshness (#2690). @@ -1115,6 +1155,14 @@ async def attach_live_body(note, data: dict) -> None: _refresh_provenance(note, fetched.commit_sha), site="pull provenance-refresh", ) + elif _verdict_still_vouches(note, fields, fetched.commit_sha or ""): + # Containment failed, but an unexpired `ok` verdict stamped at exactly + # this commit already judged this body a faithful rendering of it — + # the annotated-record case (#2782). Trust the judgment over the + # substring test; `data["verification"]` travels in the same payload, + # so a reader can see the basis rather than take "current" on faith. + data["body_source"] = "forge" + data["body_freshness"] = "current" else: data["body_source"] = "cache" data["body_freshness"] = "diverged" diff --git a/tests/test_snippet_live_body.py b/tests/test_snippet_live_body.py index 47eea42..004e95b 100644 --- a/tests/test_snippet_live_body.py +++ b/tests/test_snippet_live_body.py @@ -272,3 +272,89 @@ async def test_forge_failure_inside_lookup_never_breaks_the_pull(): data = _data() await svc.attach_live_body(_note(), data) assert "body_source" not in data + + +# --- #2782: an annotated record is not a diverged one ------------------------ +# Containment is right for a verbatim record and wrong for a deliberately +# annotated one: the commentary that makes the record worth reading is exactly +# what makes `cached in fetched` false, forever. These pin the escape hatch — +# a standing `ok` verdict stamped at the commit we just fetched — and, just as +# importantly, every condition that must switch it back off. + +ANNOTATED = "# Membership is the contract — this record says WHY, the source can't.\n" + CODE + + +def _ok_verdict(code=ANNOTATED, commit=SHA, **extra): + verdict = svc.compose_verification( + status=svc.VERIFY_OK, checked_code_sha=svc.code_sha(code), commit_sha=commit + ) + verdict.update(extra) + return verdict + + +async def _freshness(data, *, file_commit=SHA, content=CODE): + forge = _forge_with(lambda r: _file_response(content, commit_sha=file_commit)) + with _patched(forge), patch.object(svc.notes_svc, "update_note", AsyncMock()): + await svc.attach_live_body(_note(), data) + await background.drain() + return data["body_source"], data["body_freshness"] + + +async def test_annotated_record_with_a_standing_verdict_reads_current(): + """The bug: the record's commentary is absent from the source, so + containment fails and every pull said `diverged`. A verdict that already + judged this body faithful, at this very commit, outranks the substring.""" + data = _data(code=ANNOTATED, verification=_ok_verdict()) + assert await _freshness(data) == ("forge", "current") + assert data["snippet"]["code"] == ANNOTATED # still never rewritten + + +async def test_the_verdict_vouches_for_one_commit_only(): + """The guard that keeps the hatch honest. The file has moved past the + commit the verdict was stamped at, so nobody has judged what is there + now — containment resumes as the authority and the record reads diverged + until someone re-verifies.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(commit="a" * 40)) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_an_expired_verdict_does_not_vouch(): + """The record was edited after the check, so `code_sha` no longer matches + and the verdict describes a body that is not this one.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(code="def other(): pass")) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_a_push_invalidated_verdict_does_not_vouch(): + """A push touched the recorded location since the check (#2691) — the repo + moved under the verdict even though the record didn't.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(invalidated_by="c" * 40)) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_only_an_ok_verdict_vouches(): + """A drifted verdict is evidence AGAINST the body, not for it.""" + verdict = svc.compose_verification( + status=svc.VERIFY_CHANGED, checked_code_sha=svc.code_sha(ANNOTATED), commit_sha=SHA + ) + data = _data(code=ANNOTATED, verification=verdict) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_a_verbatim_record_still_takes_the_containment_path(): + """No regression: the happy path does not route through the hatch, and an + unverified verbatim record is still confirmed by containment alone.""" + data = _data(code=CODE) + assert await _freshness(data) == ("forge", "current") + + +async def test_a_verdict_predating_commit_stamping_does_not_vouch(): + """Verdicts recorded before `commit_sha` existed (#2688) carry no commit to + compare, so they cannot tie the body to a known state of the source. They + fall through to containment rather than vouching on age alone.""" + verdict = svc.compose_verification( + status=svc.VERIFY_OK, checked_code_sha=svc.code_sha(ANNOTATED) + ) + assert "commit_sha" not in verdict + data = _data(code=ANNOTATED, verification=verdict) + assert await _freshness(data) == ("cache", "diverged") -- 2.54.0 From 64bfa5725fa577b3c7da781e7bd13603dd63f9d2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 23 Aug 2026 22:48:54 -0400 Subject: [PATCH 2/3] =?UTF-8?q?feat(telemetry):=20a=20read=20surface=20ove?= =?UTF-8?q?r=20retrieval=5Flogs=20=E2=80=94=20the=20tuning=20loop=20had=20?= =?UTF-8?q?no=20read=20half=20(#2975)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retrieval_logs` was write-only. `record_retrieval` inserted rows and nothing in the tree ever selected from them: the only `select()` over RetrievalLog lived in a test. So #1038's gate — "build the reranker once telemetry shows precision is the bottleneck" — was unsatisfiable by construction, and the one real tuning decision on record (the 0.68 write-path threshold, #2223) had to be reached by hand-probing the live instance with eight payloads. This adds the half that was missing. `retrieval_summary(user_id, days=30)` returns two aggregates side by side, each read from the table built for it — NOT a join. NoteUsageEvent's docstring is explicit that the two are complements ("RetrievalLog tunes the threshold, this tunes the corpus") and that RetrievalLog's JSONB `result_ids` cannot be indexed at the per-note grain, so correlating through it would be both slower and less honest than reading each source directly. That corrects the approach sketched on the task. - `sources`, per surface: calls, zero_result_calls, cleared_threshold (how often the best hit beat the threshold in force for THAT call), the top_score spread as p10/p50/p90/min/max, avg_result_count, p90 duration. Zero-result calls are counted apart from low-scoring ones — they are a different failure and averaging them together would hide both. - `usage`, from note_usage_events: ranked surfacings, ambient surfacings, and pulls split into `pulled_by_agent` / `pulled_by_human`. That split is not decoration. NoteUsageEvent's own comment says the mcp_/rest_ prefix is load-bearing and names #1038 while saying so: "is this dead weight?" is answered by any pull, "was that injected line useful?" only by an agent pull. `pull_through` exists to answer the second, so it counts agent pulls over ranked surfacings; both halves ship so the first stays answerable. Two things the code made me get right rather than guess: - Distinct-note counts get their own queries. `count(distinct note_id)` per (event, source) group cannot be summed across groups — a note surfaced by two sources is one distinct note and would be counted twice. A wrong number labelled "distinct" is worse than no number. - No CASE in the GROUP BY. #2663 is the bug where a second case() rendered its own expanding bind names, Postgres rejected the query, a broad except swallowed it, and every counter read zero in production while mocked tests passed. Grouping on raw `source` and classifying in Python cannot fail that way. For the same reason the readout distinguishes `read_failed` from an empty window, and its tests are integration against real Postgres — percentile_cont ... WITHIN GROUP only proves it parses against a database. Exposed as the `retrieval_telemetry` MCP tool, added to `_READ_ONLY_TOOLS`: it mutates nothing, but its name carries no read prefix, so the completeness test cannot derive it and it would otherwise have failed closed for read-only keys in silence — the same reason `enter_project` is spelled out there. Docs updated to name both exceptions rather than leave the rule looking derivable. Scoped to the caller's own telemetry: a retrieval log records what one user's agent asked for, query text included, and is not a shared record kind — the owner filter is the whole access rule, not a shortcut past access.py. Co-Authored-By: Claude Fable 5 --- docs/api-keys-and-mcp.md | 8 +- src/scribe/mcp/server.py | 5 + src/scribe/mcp/tools/search.py | 49 ++++- src/scribe/services/retrieval_telemetry.py | 208 +++++++++++++++++++++ tests/test_services_retrieval_telemetry.py | 120 ++++++++++++ 5 files changed, 386 insertions(+), 4 deletions(-) diff --git a/docs/api-keys-and-mcp.md b/docs/api-keys-and-mcp.md index f6ebe4c..d950d4b 100644 --- a/docs/api-keys-and-mcp.md +++ b/docs/api-keys-and-mcp.md @@ -43,8 +43,10 @@ client straight to the URL with a Bearer token. Authenticate with an API key generated from **Settings → API Keys** (see above), sent as `Authorization: Bearer fmcp_`. A `read`-scoped key may call only the -read tools (`get_*`, `list_*`, `search`, `enter_project`); any write/delete tool -is rejected with `403`. A `write`-scoped key may call everything. +read tools (`get_*`, `list_*`, `search`, `enter_project`, `retrieval_telemetry`); +any write/delete tool is rejected with `403`. The allow-list is explicit rather +than derived from the name — see `_READ_ONLY_TOOLS`, which is why the two reads +without a read-shaped name are spelled out here. A `write`-scoped key may call everything. ### Claude Code (Project-scoped) @@ -85,7 +87,7 @@ table here. The tools are grouped by family: | Notes | `create_note`, `get_note`, `update_note`, `delete_note`, `list_notes` | Free-form knowledge | | Tasks | `create_task`, `update_task`, `add_task_log`, `start_planning` | Actionable work + plans | | Projects / Milestones | `enter_project`, `get_project`, `create_milestone`, … | Containers and outcomes | -| Search / Recall | `search`, `get_recent`, `list_tags` | Semantic + structured recall | +| Search / Recall | `search`, `get_recent`, `list_tags`, `retrieval_telemetry` | Semantic + structured recall, and the readout its thresholds are tuned from | | Systems | `create_system`, `list_systems`, `list_system_records` | Reusable per-project subsystems/areas | | Rulebooks | `list_always_on_rules`, `list_rules`, `create_rule`, `create_project_rule`, `subscribe_project_to_rulebook`, … | Engineering/workflow rules | | Processes | `list_processes`, `get_process`, `create_process` | Saved prompts/workflows | diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 4ccc7a0..5f0a8d5 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -112,6 +112,11 @@ _READ_ONLY_TOOLS = frozenset({ # The shape ledger's todo query (#2789). Reads only — classify_shapes is # the write, and it is deliberately NOT here. "list_shapes", "shape_history", + # The retrieval telemetry readout (#2975). Aggregates two log tables and + # writes nothing. Listed explicitly because its name carries no read + # prefix, so the completeness test below cannot derive it — the same + # reason `enter_project` is spelled out above. + "retrieval_telemetry", }) # Read-SHAPED tools that must NOT be reachable with a read key — a getter that diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index dbe2f28..b1e5775 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -12,7 +12,7 @@ import time from scribe.mcp._context import current_user_id from scribe.services.access import owner_names_for from scribe.services.embeddings import DEFAULT_SIMILARITY_THRESHOLD, semantic_search_notes -from scribe.services.retrieval_telemetry import record_retrieval +from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary async def search( @@ -95,5 +95,52 @@ async def search( } +async def retrieval_telemetry(days: int = 30) -> dict: + """What the retrieval telemetry says about YOUR surfaces, over a window. + + The read half of the loop the ranker's thresholds are meant to be tuned + from (#2975). Reach for it before changing a similarity threshold, a top-k, + or deciding whether a reranker is worth building — the alternative is + hand-probing the live instance, which is how the last such decision had to + be made. + + Two readouts, from the two tables built for them: + + `sources` — per retrieval surface (`auto_inject`, `write_path`, + `mcp_search`, …), from `retrieval_logs`: `calls`, `zero_result_calls`, + `cleared_threshold` (how often the best hit beat the threshold in force for + that call), the `top_score` spread (p10/p50/p90/min/max), `avg_result_count` + and `p90_duration_ms`. THE number to read first is `cleared_threshold` + against `calls`, with the spread beside it: a surface that clears its bar + on nearly every call is either well-tuned or too loose, and p10 says which. + + `usage` — from `note_usage_events`, at the per-note grain + `retrieval_logs` cannot be indexed at: `surfaced` (ranked surfacings — a + scored surface CHOSE the record), `ambient` (the rest), `pulled` split into + `pulled_by_agent` / `pulled_by_human`, the distinct-note counts, and + `pull_through`. That ratio is the corpus-side precision signal: records + surfaced often and opened never are dead weight competing for the injection + budget every turn. + + `pull_through` is AGENT pulls over RANKED surfacings, and both halves of + that matter. "Is this record dead weight?" is answered by any pull; "was + that injected line useful?" — the question a threshold or a reranker is + tuned against — only by a pull the agent made. Aggregating across the + mcp_/rest_ prefix would silently answer the wrong one. + + Scoped to your own telemetry — a retrieval log records what your agent + asked for, query text included, and is not a shared record kind. + + `read_failed: true` means the query itself failed — deliberately distinct + from an empty window, because those two looked identical for weeks once + (#2663) and every counter silently read zero. + + Args: + days: window size, default 30. Clamped to at least 1. + """ + return await retrieval_summary(current_user_id(), days=days) + + def register(mcp) -> None: mcp.tool(name="search")(search) + mcp.tool(name="retrieval_telemetry")(retrieval_telemetry) diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index b8fb21f..65b6cbc 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -18,8 +18,14 @@ from __future__ import annotations import asyncio import logging +from datetime import datetime, timedelta, timezone + +from sqlalchemy import case, func, select + from scribe.models import async_session +from scribe.models.base import iso from scribe.models.note import Note +from scribe.models.note_usage import PULLED, SURFACED, NoteUsageEvent from scribe.models.retrieval_log import RetrievalLog logger = logging.getLogger(__name__) @@ -135,3 +141,205 @@ def record_retrieval( return _pending.add(task) task.add_done_callback(_pending.discard) + + +# --- The read half (#2975) --------------------------------------------------- +# Until this existed, `retrieval_logs` was WRITE-ONLY: rows accrued and the only +# `select()` over them in the whole tree lived in a test. That made #1038's gate +# — "build the reranker once telemetry shows precision is the bottleneck" — +# unsatisfiable by construction, and it is why the one real tuning decision on +# record (the 0.68 write-path threshold, #2223) was reached by hand-probing the +# live instance with eight payloads instead of by reading what was collected. + +def _bucket(rows: list) -> dict: + """A score readout a human can act on, from one aggregate row.""" + calls, zero, cleared, p10, p50, p90, lo, hi, avg_n, dur = rows + return { + "calls": int(calls or 0), + # A call that returned nothing is not a low-scoring call — it is a + # different failure (nothing indexed, filter too narrow), and averaging + # it into the score distribution would hide both. + "zero_result_calls": int(zero or 0), + # How often the best hit actually cleared the threshold in force for + # that call. THE precision-adjacent number: a surface that clears its + # bar on almost every call is either well-tuned or too loose, and the + # score spread below says which. + "cleared_threshold": int(cleared or 0), + "top_score": { + "p10": _round(p10), "p50": _round(p50), "p90": _round(p90), + "min": _round(lo), "max": _round(hi), + }, + "avg_result_count": _round(avg_n), + "p90_duration_ms": _round(dur, 1), + } + + +def _round(v, places: int = 4): + return None if v is None else round(float(v), places) + + +async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: + """What the retrieval telemetry says, per surface, over a window. + + Two aggregates side by side, each read from the table built for it — NOT a + join. `NoteUsageEvent`'s own docstring is explicit that the two are + complements ("RetrievalLog tunes the threshold, this tunes the corpus") and + that RetrievalLog's JSONB `result_ids` "can't be indexed at" the per-note + grain. So the score distribution comes from `retrieval_logs` on its indexed + columns, and surfaced-vs-pulled comes from `note_usage_events` at the grain + it was built for. Reading each from its own table is both cheaper and more + honest than correlating them through JSONB. + + Scoped to one user's own telemetry. There is no sharing model for a + retrieval log — it records what THIS user's agent asked for, including the + query text — so an owner filter is the whole access rule here rather than a + shortcut around `services/access.py` (P#78 governs shared record kinds). + + Never raises: a telemetry readout that can break its caller is worse than + no readout. It does distinguish "no rows" from "the read failed", because + #2663 is exactly the bug where those two looked identical for weeks. + """ + since = datetime.now(timezone.utc) - timedelta(days=max(1, int(days))) + out: dict = { + "window_days": int(days), + "since": iso(since), + "sources": {}, + "usage": {}, + "read_failed": False, + } + + cleared = case( + ( + (RetrievalLog.threshold.isnot(None)) + & (RetrievalLog.top_score.isnot(None)) + & (RetrievalLog.top_score >= RetrievalLog.threshold), + 1, + ), + else_=0, + ) + zero = case((RetrievalLog.result_count == 0, 1), else_=0) + + def pct(p: float): + return func.percentile_cont(p).within_group(RetrievalLog.top_score.asc()) + + try: + async with async_session() as session: + rows = ( + await session.execute( + select( + RetrievalLog.source, + func.count().label("calls"), + func.sum(zero).label("zero"), + func.sum(cleared).label("cleared"), + pct(0.1), pct(0.5), pct(0.9), + func.min(RetrievalLog.top_score), + func.max(RetrievalLog.top_score), + func.avg(RetrievalLog.result_count), + func.percentile_cont(0.9).within_group( + RetrievalLog.duration_ms.asc() + ), + ) + .where( + RetrievalLog.created_at >= since, + RetrievalLog.user_id == user_id, + ) + .group_by(RetrievalLog.source) + ) + ).all() + for row in rows: + out["sources"][row[0]] = _bucket(list(row[1:])) + + # The corpus side, at its own grain. `ambient` mirrors + # note_usage.usage_for_notes: an ambient surfacing was not a scored + # CHOICE, so folding it into pull-through would understate it. + # Grouped by RAW source, then classified in Python. The + # alternative — CASE expressions in the GROUP BY — is the shape + # that produced #2663: a second case() renders its own expanding + # bind names, the database sees two different expressions and + # rejects the query, and the broad except swallows it. One CASE is + # provably fine (usage_for_notes does it); two is where it broke. + # `source` has a handful of distinct values, so grouping on it + # directly is cheap and cannot fail that way at all. + urows = ( + await session.execute( + select( + NoteUsageEvent.event, + NoteUsageEvent.source, + func.count().label("n"), + ) + .where( + NoteUsageEvent.created_at >= since, + NoteUsageEvent.user_id == user_id, + ) + .group_by(NoteUsageEvent.event, NoteUsageEvent.source) + ) + ).all() + + # Distinct-note counts need their OWN queries, and this is not + # fussiness: count(distinct note_id) per (event, source) group + # cannot be summed across groups — a note surfaced by two sources + # is one distinct note and would be counted twice. A wrong number + # labelled "distinct" is worse than no number. + from scribe.services.note_usage import AMBIENT_SOURCES as _AMB + + distinct_surfaced = ( + await session.execute( + select(func.count(func.distinct(NoteUsageEvent.note_id))).where( + NoteUsageEvent.created_at >= since, + NoteUsageEvent.user_id == user_id, + NoteUsageEvent.event == SURFACED, + NoteUsageEvent.source.notin_(_AMB), + ) + ) + ).scalar_one() + distinct_pulled = ( + await session.execute( + select(func.count(func.distinct(NoteUsageEvent.note_id))).where( + NoteUsageEvent.created_at >= since, + NoteUsageEvent.user_id == user_id, + NoteUsageEvent.event == PULLED, + ) + ) + ).scalar_one() + except Exception: + logger.warning("retrieval summary read failed", exc_info=True) + out["read_failed"] = True + return out + + from scribe.services.note_usage import AMBIENT_SOURCES + + usage = { + "surfaced": 0, "ambient": 0, + "pulled": 0, "pulled_by_agent": 0, "pulled_by_human": 0, + "distinct_notes_surfaced": int(distinct_surfaced or 0), + "distinct_notes_pulled": int(distinct_pulled or 0), + } + for event, source, n in urows: + n = int(n) + if event == SURFACED: + if source in AMBIENT_SOURCES: + usage["ambient"] += n + else: + usage["surfaced"] += n + elif event == PULLED: + usage["pulled"] += n + # The mcp_/rest_ split is load-bearing (see NoteUsageEvent's own + # comment, which names #1038 — this readout's whole purpose). "Is + # this record dead weight?" is answered by ANY pull; "was that + # injected line useful to the agent?" only by an AGENT pull. So + # pull-through, which exists to answer the second, counts mcp_* + # only. Both halves are reported so the first question is still + # answerable from the same payload. + if source.startswith("mcp_"): + usage["pulled_by_agent"] += n + else: + usage["pulled_by_human"] += n + # Ranked surfacings in the denominator, agent pulls in the numerator: the + # "surfaced often, opened never" reading is only valid where a scored + # surface CHOSE the record and an agent was the one who declined it. + usage["pull_through"] = ( + round(usage["pulled_by_agent"] / usage["surfaced"], 4) + if usage["surfaced"] else None + ) + out["usage"] = usage + return out diff --git a/tests/test_services_retrieval_telemetry.py b/tests/test_services_retrieval_telemetry.py index 67a5d01..bd69dd7 100644 --- a/tests/test_services_retrieval_telemetry.py +++ b/tests/test_services_retrieval_telemetry.py @@ -102,3 +102,123 @@ async def test_insert_retrieval_log_roundtrip(_dispose_engine): assert row.created_at is not None # server_default now() await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == 990001)) await s.commit() + + +# ─── the read half: retrieval_summary (integration) ────────────────────────── +# Integration, not mocked, and deliberately so. #2663 is the bug where a +# GROUP BY the database rejected was swallowed by a broad except, so every +# counter read zero in production while the writes landed fine and the mocked +# tests passed. `retrieval_summary` runs a grouped aggregate with +# percentile_cont ... WITHIN GROUP and a two-label CASE — precisely the shape +# that failed then. Only a real Postgres can say it parses. + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_retrieval_summary_reads_what_the_writer_wrote(_dispose_engine): + from sqlalchemy import delete + + from scribe.models import async_session + from scribe.models.note_usage import NoteUsageEvent + from scribe.models.retrieval_log import RetrievalLog + from scribe.services.retrieval_telemetry import ( + _insert_retrieval_log, retrieval_summary, + ) + + UID = 990002 + # Three auto_inject calls at a 0.55 bar: two clear it, one does not. + # Plus one call that returned nothing at all — a different failure from a + # low-scoring one, and the readout must not blend them. + for score in (0.91, 0.72, 0.40): + await _insert_retrieval_log(_build_payload( + user_id=UID, source="auto_inject", query="q", threshold=0.55, + limit=3, project_id=None, is_task=None, + results=[(score, _note(1))], duration_ms=5.0, + )) + await _insert_retrieval_log(_build_payload( + user_id=UID, source="auto_inject", query="q", threshold=0.55, + limit=3, project_id=None, is_task=None, results=[], duration_ms=5.0, + )) + # A second surface, so the GROUP BY has something to separate. + await _insert_retrieval_log(_build_payload( + user_id=UID, source="mcp_search", query="q", threshold=0.45, + limit=10, project_id=None, is_task=None, + results=[(0.80, _note(2))], duration_ms=11.0, + )) + # Corpus side: two ranked surfacings, one ambient, one pull. + async with async_session() as s: + s.add_all([ + NoteUsageEvent(user_id=UID, note_id=1, event="surfaced", source="auto_inject"), + NoteUsageEvent(user_id=UID, note_id=2, event="surfaced", source="auto_inject"), + NoteUsageEvent(user_id=UID, note_id=3, event="surfaced", source="enter_project"), + NoteUsageEvent(user_id=UID, note_id=1, event="pulled", source="mcp_get_note"), + ]) + await s.commit() + + try: + out = await retrieval_summary(UID, days=30) + + assert out["read_failed"] is False, "the aggregate did not execute" + ai = out["sources"]["auto_inject"] + assert ai["calls"] == 4 + assert ai["zero_result_calls"] == 1 + assert ai["cleared_threshold"] == 2 # 0.91 and 0.72, not 0.40 + # p50 over the three scored calls; the empty one contributes no score. + assert ai["top_score"]["p50"] == pytest.approx(0.72, abs=1e-4) + assert ai["top_score"]["min"] == pytest.approx(0.40, abs=1e-4) + assert ai["top_score"]["max"] == pytest.approx(0.91, abs=1e-4) + assert out["sources"]["mcp_search"]["calls"] == 1 + + u = out["usage"] + assert u["surfaced"] == 2 and u["ambient"] == 1 and u["pulled"] == 1 + assert u["distinct_notes_surfaced"] == 2 + # The pull came from `mcp_get_note`, so it counts as an AGENT pull + # and drives pull_through; a human `rest_*` pull would not. + assert u["pulled_by_agent"] == 1 and u["pulled_by_human"] == 0 + assert u["pull_through"] == pytest.approx(0.5) + finally: + async with async_session() as s: + await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID)) + await s.execute(delete(NoteUsageEvent).where(NoteUsageEvent.user_id == UID)) + await s.commit() + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_retrieval_summary_is_empty_not_broken_for_a_fresh_install(_dispose_engine): + """Rule #115: an install with no telemetry gets a coherent zero readout, + and `read_failed` stays False — the distinction #2663 says must exist.""" + from scribe.services.retrieval_telemetry import retrieval_summary + + out = await retrieval_summary(990003, days=30) + assert out["read_failed"] is False + assert out["sources"] == {} + assert out["usage"]["pull_through"] is None # no division by zero + assert out["usage"]["surfaced"] == 0 + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_retrieval_summary_sees_only_its_own_users_telemetry(_dispose_engine): + """A retrieval log records what one user's agent asked for, query text + included. The owner filter is the access rule, so it gets a test.""" + from sqlalchemy import delete + + from scribe.models import async_session + from scribe.models.retrieval_log import RetrievalLog + from scribe.services.retrieval_telemetry import ( + _insert_retrieval_log, retrieval_summary, + ) + + await _insert_retrieval_log(_build_payload( + user_id=990004, source="auto_inject", query="theirs", threshold=0.55, + limit=3, project_id=None, is_task=None, results=[(0.9, _note(1))], + duration_ms=1.0, + )) + try: + assert (await retrieval_summary(990005, days=30))["sources"] == {} + assert (await retrieval_summary(990004, days=30))["sources"]["auto_inject"]["calls"] == 1 + finally: + async with async_session() as s: + await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == 990004)) + await s.commit() -- 2.54.0 From a8f35e465e5ef0f68935bd7ec49289517051cbb6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 24 Aug 2026 01:28:07 -0400 Subject: [PATCH 3/3] =?UTF-8?q?refactor(plugin+tests):=20the=20last=20two?= =?UTF-8?q?=20parallel-family=20gaps=20=E2=80=94=20pageable=20list=20tools?= =?UTF-8?q?,=20one=20config=20preamble=20(#2278)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DRY pass 3's remainder. Both halves start from enumeration, because the task's candidate list was hypotheses and the process requires counting before proposing — and counting changed the answer twice. ## The list_* family: a limit with no offset Enumerated all 19 `list_*` MCP tools first. They are genuinely heterogeneous — 8 take `project_id`, 6 take `limit`, six take no arguments at all — so a common-parameter guard would invent a convention the API does not have, which is the over-DRY trap (§5). One contract IS real: a `limit` without an `offset` is a truncation with no continuation. The caller is told there are 250 results, handed 50, and given no way to ask for the rest. Two tools had it, and both were capped over a service that already accepted an offset: `snippets_svc.list_snippets(offset=0)` was simply not exposed, and `list_processes` passed a hardcoded `offset=0` into `query_knowledge`. The capability existed one layer down in both; only the door was missing — the missing-sibling shape exactly. Both now expose it. `tests/test_mcp_list_family.py` guards it, with `list_tags` exempted for a stated reason (a ranked top-N over a bounded vocabulary has no "rest" to page into). Candidates derived, decision explicit, same design as test_mcp_auth — plus the reverse checks: a stale exemption, and an offset with no limit, which would page through an unbounded result set. Verified non-vacuous by running the sweep against the pre-fix tree, where it fails naming both tools. ## The verb pairs: no finding, which is the finding `preview`/`apply` and `dry_run`/`commit` do not exist anywhere in the 102 tools — those were guesses about a shape Scribe never adopted. `count_*` does not exist either. Of the create/delete stems only `project_rule` lacks a `delete_X`, and deliberately: a project rule IS a rule, `delete_rule` removes it, and the docstring says so. `force` sits on 6 of 7 duplicate-gated creates; the exception is `create_system`, whose gate is an exact normalized-NAME match rather than a semantic near-match — forcing it would split one area's records across two piles, which its own message explains. No guard added: it would need a seven-entry exemption list to defend against a hypothetical. Recorded on the leave-alone list instead, which the process asks for by name. ## The hook config preamble Not 3 of 6 hooks as recorded — all FIVE carried their own copy, and of four lines rather than two. The extra two are a guard treating an unexpanded `${...}` placeholder as unset, so it is never sent as a garbage Bearer token: precisely the correctness detail a sixth hook would omit with nothing failing loudly. Now `scribe_config` in scribe_defs.sh, which also declares the two names it owns. It sets globals rather than echoing, so a token never passes through a subshell's output where xtrace or a log could catch it, and returns a status so a caller can bail (`|| exit 0`) or continue degraded — the session-context hook still owes its static floor when Scribe is unconfigured. `check_plugin.py` now runs shellcheck with `-x`. Without it the shared helpers were invisible: every variable they set read as unassigned and every bug inside them went unlinted at the call site, which is the opposite of what sharing them was for. All twelve fail-open scenarios still pass, and all five hooks were probed live against the instance — prior_art and after_write both still name canon, autoinject returns context, session_context serves 11k chars of rules, sync_processes stays silent. Plugin 0.1.46 (#2209). Co-Authored-By: Claude Fable 5 --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_after_write.sh | 5 +- plugin/hooks/scribe_autoinject.sh | 10 +-- plugin/hooks/scribe_defs.sh | 32 ++++++- plugin/hooks/scribe_prior_art.sh | 6 +- plugin/hooks/scribe_session_context.sh | 13 ++- plugin/hooks/scribe_sync_processes.sh | 10 +-- scripts/check_plugin.py | 7 +- src/scribe/mcp/tools/processes.py | 9 +- src/scribe/mcp/tools/snippets.py | 7 +- tests/test_mcp_list_family.py | 114 +++++++++++++++++++++++++ 11 files changed, 181 insertions(+), 34 deletions(-) create mode 100644 tests/test_mcp_list_family.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 794ecf9..2034969 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "0.1.45", + "version": "0.1.46", "author": { "name": "Bryan Van Deusen" }, "mcpServers": { "scribe": { diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index cf5959f..52dcfd4 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -104,10 +104,7 @@ while IFS=$'\t' read -r path sha; do done <<< "$current" [ -n "$changed" ] || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +scribe_config || : # sets url/token; the call below is guarded on them repo=$(git -C "$repo_root" remote get-url origin 2>/dev/null || true) repo_q="" if [ -n "$repo" ]; then diff --git a/plugin/hooks/scribe_autoinject.sh b/plugin/hooks/scribe_autoinject.sh index 599f30e..3197b06 100755 --- a/plugin/hooks/scribe_autoinject.sh +++ b/plugin/hooks/scribe_autoinject.sh @@ -23,6 +23,9 @@ # note is injected at most once per session. Passed back as exclude_ids. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 command -v curl >/dev/null 2>&1 || exit 0 @@ -35,13 +38,8 @@ event_cwd=$(printf '%s' "$event" | jq -r '.cwd // empty' 2>/dev/null) || event_c # Nothing to retrieve against. [ -n "$prompt" ] || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded ${...} placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac # Unconfigured install → silent (auto-inject is pure enrichment). -[ -n "$url" ] && [ -n "$token" ] || exit 0 +scribe_config || exit 0 # Cap the query length — a giant prompt makes a giant URL for no extra signal. # `head -c`, not `cut -c1-2000`: cut is line-oriented and caps EACH LINE, so a diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 67b4a32..93a2b39 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # shellcheck shell=bash -# Scribe plugin — the pieces the two write-path hooks share (#2901). +# Scribe plugin — the pieces the hooks share (#2901, #2278). # # scribe_prior_art.sh fires BEFORE a Write/Edit tool call; scribe_after_write.sh # fires AFTER a Bash tool call and diffs the working tree, so code written by @@ -14,6 +14,8 @@ # scribe_unreached STATE SID SECS REL the "Scribe didn't answer" line, once # per outage (#2932) — or nothing, if said lately # scribe_reached STATE SID the server answered: the next outage speaks again +# scribe_config sets `url` + `token` from the env, returns 0 +# only if BOTH are usable (#2278) # # Sourced, not executed: `. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh"`. @@ -128,6 +130,34 @@ scribe_local_dups() { # the time it last spoke; within ten minutes of that it stays quiet, and a # successful call clears it so the next outage announces itself afresh. # Unconfigured installs never reach this: no URL/token means no call was owed. +# Where every hook gets its endpoint and credential. Four lines, and each of +# the five hooks carried its own copy until #2278 — which is exactly the +# missing-sibling shape: the `${...}` guard below is a correctness detail a +# sixth hook would have forgotten, and nothing would have failed loudly. +# +# Sets `url` and `token` as globals rather than echoing them: a token must not +# pass through a subshell's output, where it could land in a log or an `xtrace` +# line. Returns 0 only when both are usable, so a caller can either bail +# (`scribe_config || exit 0`) or carry on degraded — the session-context hook +# still owes its static floor when Scribe is unconfigured. +# Declared here, not just assigned inside the function: `scribe_defs.sh` owns +# these two names, and a sourcing hook should have them defined the moment it +# sources — before any code path that might reference them. It also lets +# the linter see the assignment, which it cannot follow into a function in +# another file without -x (SC2154). +url="" +token="" + +scribe_config() { + url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} + token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} + # An unexpanded `${...}` placeholder arriving as a literal would be sent as a + # garbage Bearer token and 401. Treat it as unset. + case "$url" in *'${'*) url="" ;; esac + case "$token" in *'${'*) token="" ;; esac + [ -n "$url" ] && [ -n "$token" ] +} + _SCRIBE_UNREACHED_QUIET=600 scribe_unreached() { diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index a19d1b0..2a19aa6 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -121,11 +121,7 @@ if [ -n "$shapes" ]; then [ -n "$enc" ] && shapes_q="&shapes=${enc}" fi -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded ${...} placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +scribe_config || : # sets url/token; unconfigured is handled just below # Unconfigured install → the recorded-prior-art arms are skipped, but the local # arm above already ran and may have something to say. if [ -z "$url" ] || [ -z "$token" ]; then diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 77b5a17..04aad2a 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -39,6 +39,9 @@ # allowed to fail quietly; see the #2198 comment at the status block below. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 # needed to emit the JSON envelope safely # `CDPATH= cd` is deliberate, not a typo'd assignment: it runs this one `cd` @@ -87,13 +90,9 @@ if [ -f "$manifest" ]; then fi # --- Tier 2: dynamic rules + active-project context (best-effort) --- -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} - -# Guard against an unexpanded `${...}` placeholder reaching us as a literal — it -# would otherwise be sent as a garbage Bearer token and 401. Treat as unset. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +# Unconfigured is NOT a failure here: tier 1's static floor is still owed, +# so this records the answer rather than acting on it. +scribe_config || : dyn="" status="" diff --git a/plugin/hooks/scribe_sync_processes.sh b/plugin/hooks/scribe_sync_processes.sh index 6422e45..8e0d0dc 100755 --- a/plugin/hooks/scribe_sync_processes.sh +++ b/plugin/hooks/scribe_sync_processes.sh @@ -23,15 +23,13 @@ # #2198), with SCRIBE_URL / SCRIBE_TOKEN as the override. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 command -v curl >/dev/null 2>&1 || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded `${...}` placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac -[ -n "$url" ] && [ -n "$token" ] || exit 0 +scribe_config || exit 0 body=$(curl -fsS --max-time 8 \ -H "Authorization: Bearer ${token}" \ diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index c4bdcfd..159eaa2 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -159,7 +159,12 @@ def check_shellcheck() -> None: return for script in hook_scripts(): proc = subprocess.run( - [exe, "--severity=warning", "--shell=bash", str(script)], + # -x FOLLOWS `# shellcheck source=` directives into the sourced + # file. Without it the shared helpers in scribe_defs.sh are + # invisible, so every variable they set reads as unassigned + # (SC2154) and every bug inside them goes unlinted at the call + # site — which is the opposite of what sharing them was for. + [exe, "--severity=warning", "--shell=bash", "-x", str(script)], capture_output=True, text=True, ) rel = script.relative_to(ROOT) diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index 2ad837e..e3994b6 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -15,13 +15,17 @@ from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled -async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: +async def list_processes( + q: str = "", tag: str = "", limit: int = 50, offset: int = 0, +) -> dict: """List stored processes (reusable saved prompts). Args: q: Free-text search across title + body (optional). tag: Filter to a single tag (optional). limit: Max results (1-100). + offset: Skip this many before returning — page past the cap. + `total` is the unpaged count, so it says whether more remains. Returns {"processes": [{id, title, tags, preview}], "total": int}. An entry marked `shared: true` with an `owner` is another person's procedure — treat @@ -34,7 +38,8 @@ async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: uid = current_user_id() items, total = await knowledge_svc.query_knowledge( user_id=uid, note_type="process", tags=[tag] if tag else [], - sort="modified", q=q or None, limit=max(1, min(limit, 100)), offset=0, + sort="modified", q=q or None, limit=max(1, min(limit, 100)), + offset=max(0, offset), ) labelled = await access_svc.label_shared_items(uid, items) procs = [{"id": it["id"], "title": it["title"], "tags": it.get("tags", []), diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index f591eeb..ef61aec 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -20,7 +20,8 @@ from scribe.services import systems as systems_svc async def list_snippets( - q: str = "", tag: str = "", limit: int = 50, project_id: int = 0, + q: str = "", tag: str = "", limit: int = 50, offset: int = 0, + project_id: int = 0, repo: str = "", path: str = "", symbol: str = "", verification: str = "", ) -> dict: """List recorded snippets — the project's pattern library. @@ -41,6 +42,9 @@ async def list_snippets( well as wording, so describe what you need the code to DO. tag: Filter to a single tag, e.g. a language like "python" (optional). limit: Max results (1-100). + offset: Skip this many before returning — page through a corpus + larger than one call. `total` is the unpaged count, so + offset+limit against it says whether more remains. project_id: Narrow to one project. 0 (default) searches every project — usually what you want, since a helper you need here may well have been written somewhere else. @@ -81,6 +85,7 @@ async def list_snippets( uid = current_user_id() items, total = await snippets_svc.list_snippets( uid, q=q or None, tag=tag, limit=max(1, min(limit, 100)), + offset=max(0, offset), project_id=project_id or None, repo=repo, path=path, symbol=symbol, verification=verification, ) diff --git a/tests/test_mcp_list_family.py b/tests/test_mcp_list_family.py new file mode 100644 index 0000000..352843b --- /dev/null +++ b/tests/test_mcp_list_family.py @@ -0,0 +1,114 @@ +"""Cross-family contracts for the `list_*` MCP tools (#2278, shape 4). + +Per-tool tests cover what each list tool does. Nothing covered what the FAMILY +owes its callers, which is where the missing-sibling shape hides: a capability +added to one member and not its neighbour changes no return value, so no +behavioural test can see it. Source inspection can. + +WHAT THIS DELIBERATELY DOES NOT ASSERT. The 19 `list_*` tools are genuinely +heterogeneous — 8 take `project_id`, 6 take `limit`, and six take no arguments +at all (`list_projects`, `list_trash`, `list_rulebooks`, `list_design_systems`, +`list_repo_bindings`, `list_starter_role_groups`). Requiring a common parameter +across them would be inventing a convention the API does not have, which the +DRY process's over-DRY guard (§5) warns against by name: a wrong abstraction is +worse than the duplication. So this file asserts ONE contract, the one that is +a real promise rather than a shape coincidence. + +THE CONTRACT: a `limit` without an `offset` is a truncation with no +continuation. The caller is told there are 250 results and handed 50, with no +way to ask for the rest. Both tools that had this were capped over a service +that already accepted an offset — `snippets_svc.list_snippets(offset=0)` was +simply not exposed, and `list_processes` passed a hardcoded `offset=0` into +`query_knowledge`. The capability existed one layer down in both cases; only +the door was missing. + +As in `test_mcp_auth`, the CANDIDATES are derived and the DECISION is explicit. +Deriving the exemption too would make the contract follow a naming convention, +so any future `list_*` could opt itself out by accident. +""" +import ast +import pathlib + +TOOLS_DIR = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" / "mcp" / "tools" + +# `limit` here caps a RANKED top-N, not a page into a corpus, so there is no +# "rest" to ask for — the 51st most-used tag is not what the caller wanted and +# an offset into that ordering answers no question. Anything added here needs a +# reason of that kind, not "it isn't paged yet". +_DELIBERATELY_UNPAGED = { + "list_tags", # most-used tags by count, over a bounded vocabulary +} + + +def _list_tools() -> dict[str, set[str]]: + """{tool name: parameter names} for every `list_*` in the tools package.""" + out: dict[str, set[str]] = {} + for path in sorted(TOOLS_DIR.glob("*.py")): + if path.name == "__init__.py": + continue + for node in ast.parse(path.read_text()).body: + if ( + isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith("list_") + ): + out[node.name] = { + a.arg for a in node.args.args + } | {a.arg for a in node.args.kwonlyargs} + return out + + +def test_the_tools_package_is_where_we_think_it_is(): + """If this fails the sweep below is silently checking nothing.""" + tools = _list_tools() + assert len(tools) >= 15, f"found only {len(tools)} list tools — did the package move?" + + +def test_every_capped_list_tool_can_be_paged(): + """A `limit` promises a cap; without an `offset` it also imposes a ceiling.""" + tools = _list_tools() + capped = {name for name, args in tools.items() if "limit" in args} + assert capped, "no list tool takes a limit — the sweep is not finding signatures" + + unpageable = sorted( + name for name in capped + if "offset" not in tools[name] and name not in _DELIBERATELY_UNPAGED + ) + assert not unpageable, ( + f"these list tools cap their results with no way to page past the cap: " + f"{unpageable}. Each hands the caller a `total` it cannot reach. Add an " + f"`offset` (check the service first — it usually already takes one), or " + f"add the tool to _DELIBERATELY_UNPAGED with a reason saying why there " + f"is no 'rest' to ask for." + ) + + +def test_the_unpaged_exemptions_still_exist(): + """A stale exemption is an exemption for nothing, and it hides the next + tool that inherits the name. Same reverse check `test_mcp_auth` runs on + its allow-lists.""" + tools = _list_tools() + missing = sorted(_DELIBERATELY_UNPAGED - set(tools)) + assert not missing, ( + f"_DELIBERATELY_UNPAGED names tools that no longer exist: {missing}. " + f"Renamed or deleted — drop them from the set." + ) + still_capped = sorted( + name for name in _DELIBERATELY_UNPAGED + if name in tools and "limit" not in tools[name] + ) + assert not still_capped, ( + f"these are exempted from paging but no longer take a `limit` at all, " + f"so the exemption is moot: {still_capped}." + ) + + +def test_offset_never_appears_without_limit(): + """The inverse, and it is a real bug rather than a style point: an offset + with no cap pages through an unbounded result set, so page 2 of an + ever-growing list silently returns everything after the skip.""" + tools = _list_tools() + bad = sorted( + name for name, args in tools.items() + if "offset" in args and "limit" not in args + ) + assert not bad, f"these take an offset but no limit: {bad}" -- 2.54.0