diff --git a/alembic/versions/0104_tuning_events_carry_their_calibration.py b/alembic/versions/0104_tuning_events_carry_their_calibration.py new file mode 100644 index 0000000..2211714 --- /dev/null +++ b/alembic/versions/0104_tuning_events_carry_their_calibration.py @@ -0,0 +1,51 @@ +"""retrieval_tuning_events carries the space each number was measured in (#4104) + +Revision ID: 0104 +Revises: 0103 +Create Date: 2026-09-17 + +Milestone 416 step 6. A retrieval floor is a cosine distance in ONE embedding +model's geometry, computed over documents cut one particular way. Change the +model and every score moves at once; change the chunker and the same record +embeds different text. Either way a number chosen before the change is a +measurement of something that no longer exists — and today nothing records +which world it was chosen in, so the staleness is unknowable rather than +merely unknown. + +`CHUNKER_VERSION` already solved exactly this for documents: stored per row, so +the startup backfill re-embeds precisely what is stale instead of wiping the +table. These two columns are that idea applied to the tuned numbers. + +TWO COLUMNS, NOT ONE (rule 149). A reader has to be able to say WHICH half +moved: a new embedding model and a re-cut document shape invalidate the same +numbers for different reasons, and a fused `"@"` could only report +that something changed. + +NULLABLE, and not backfilled. The rows already in this table were written +under something, but naming it would be inventing a fact — the honest value is +"unstamped", which is a different answer from a model name that might be wrong. +`current_settings` reports an unstamped dial as exactly that. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0104" +down_revision = "0103" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "retrieval_tuning_events", + sa.Column("embedding_model", sa.Text(), nullable=True), + ) + op.add_column( + "retrieval_tuning_events", + sa.Column("shape_version", sa.Integer(), nullable=True), + ) + + +def downgrade() -> None: + op.drop_column("retrieval_tuning_events", "shape_version") + op.drop_column("retrieval_tuning_events", "embedding_model") diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 8b1b452..587aba5 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -178,6 +178,64 @@ interface TuningEvent { reason: string; } +// What a dial's number was measured against, and whether that space has moved +// (#4104). Mirrors the `calibration` block `current_settings` returns per dial. +// `stale` is nullable on purpose: a dial moved before Scribe recorded stamps is +// UNKNOWN rather than fine, and rendering the two the same would hide the case +// most likely to be wrong. +interface DialCalibration { + source: string; + embedding_model: string | null; + shape_version: number | null; + model_changed: boolean | null; + shape_changed: boolean | null; + stale: boolean | null; +} + +interface SurfaceRow { + surface: string; + calibration: Record; +} + +const surfaceRows = ref([]); + +// Flattened to one line per dial that needs looking at, because the operator's +// question is "which numbers do I no longer trust", not "tell me about each of +// six surfaces". Anything calibrated against the live model contributes +// nothing — the panel below renders only when this is non-empty, so a healthy +// install never sees it. A warning that is always on the screen is one nobody +// reads when it finally means something. +const staleDials = computed(() => + surfaceRows.value.flatMap((row) => + Object.entries(row.calibration ?? {}) + .filter(([, cal]) => cal.stale !== false) + .map(([dial, cal]) => ({ + key: `${row.surface}.${dial}`, + surface: row.surface, + dial, + why: + cal.source === "unstamped" + ? "changed before Scribe recorded what it was measured against" + : cal.model_changed && cal.shape_changed + ? `measured under ${cal.embedding_model} at document shape ${cal.shape_version} — both have changed since` + : cal.model_changed + ? `measured under ${cal.embedding_model}, which is no longer the embedding model` + : `measured at document shape ${cal.shape_version}, and documents are cut differently now`, + })), + ), +); + +async function loadSurfaces() { + try { + const res = await apiGet<{ surfaces: SurfaceRow[] }>("/api/retrieval/surfaces"); + surfaceRows.value = res.surfaces ?? []; + } catch { + // Same reasoning as the history below: unreadable calibration is not worth + // a toast on page load, and the dials above still work. + surfaceRows.value = []; + } +} + async function loadTuningHistory() { loadingTuning.value = true; try { @@ -292,6 +350,7 @@ async function saveKbInject() { // otherwise the panel shows a trail that is stale by exactly the change // the operator is looking at it to confirm. await loadTuningHistory(); + await loadSurfaces(); } catch { toastStore.show('Failed to save auto-inject settings', 'error'); } finally { @@ -767,6 +826,7 @@ onMounted(async () => { kbReportPrefTopK.value = allSettings.kb_reportpref_top_k; } await loadTuningHistory(); + await loadSurfaces(); if (allSettings.kb_duplicate_threshold_snippet !== undefined) { kbDupThresholdSnippet.value = allSettings.kb_duplicate_threshold_snippet; } @@ -1772,6 +1832,32 @@ async function deleteUser(userId: number) { moves the dial with the argument attached. This panel is the other half of that bargain — a change made on your behalf is one you can read, disagree with, and set back by hand above. --> + +
+

+ {{ staleDials.length }} + {{ staleDials.length === 1 ? 'value was' : 'values were' }} + measured against something that has changed +

+

+ These bars are similarity scores, and a similarity score only means + something inside the model that produced it. They still apply — they + just no longer measure what they were set to measure. Nothing is + adjusted automatically; ask Claude to migrate them and it will carry + each one across at the same selectivity, then check the result. +

+
    +
  • + {{ d.surface }} + {{ d.dial }} + {{ d.why }} +
    • +
+

What has been tuned

@@ -4115,4 +4201,36 @@ async function deleteUser(userId: number) { font-size: 0.85rem; color: var(--fs-text-secondary); } +/* Stale calibration (#4104). Tinted rather than loud: the numbers still work, + they have merely stopped being measurements — that is a "come back to this", + not an outage. It only renders when something is actually stale, which is + what earns it the tint at all. */ +.calibration-warning { + margin-top: var(--fs-space-5); + padding: var(--fs-space-3); + border: 1px solid var(--fs-warning); + border-radius: var(--fs-radius-md); + background: color-mix(in srgb, var(--fs-warning) 12%, var(--fs-surface-raised)); +} +.calibration-warning-title { + margin: 0 0 var(--fs-space-2); + font-size: 0.95rem; + color: var(--fs-warning-fg); +} +.calibration-list { + list-style: none; + margin: var(--fs-space-2) 0 0; + padding: 0; + display: flex; + flex-direction: column; + gap: var(--fs-space-1); + font-size: 0.85rem; +} +.calibration-list li { + display: flex; + align-items: baseline; + flex-wrap: wrap; + gap: var(--fs-space-2); +} +.calibration-why { color: var(--fs-text-secondary); } diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 5600365..a266b4f 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.17.0111", + "version": "2026.09.17.1618", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 972ee1a..ea83b4b 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -1,6 +1,6 @@ --- name: using-scribe -description: Use at the START of every session, and before answering anything about the operator's work or starting any task — establishes the Scribe-first reflex. You hold none of the operator's rules: they arrive by retrieval when your work matches one, and search(content_type="rule") is how you ask before a consequential act. Call enter_project when a repo/project is in scope. Then recall before acting, update over duplicate, plan in Scribe not in files. +description: Use at the START of every session, and before answering anything about the operator's work or starting any task — establishes the Scribe-first reflex. You hold none of the operator's rules: they arrive by retrieval when your work matches one, and what_might_apply is how you ask before a consequential act — it returns the wide net of candidates with no bar. Call enter_project when a repo/project is in scope. Then recall before acting, update over duplicate, plan in Scribe not in files. --- # Using Scribe @@ -73,11 +73,21 @@ Two constraints on *how* that's achieved: So "no rule arrived" means "nothing matched", never "no rule exists" — an empty session is not evidence of an empty rulebook. Retrieval fires when - something asks: before a consequential act, `search(content_type="rule")` on - what you are about to do, and pull a record's full statement with - `get_rule(id)` when it is about to bite. When a project is in scope, pass - its `project_id`: the answer is then the global rules plus that project's - own, never another project's. `enter_project(id)` lists the project's own + something asks, so ask — and reach for the tool that fits the moment: + + - **Before a consequential act**, and before handing work back because you + are unsure you may finish it: `what_might_apply("what you are about to + do")`. It returns up to fifty ranked candidates with NO bar. The arms that + push rules at you spend a budget of three and say nothing about what sat + just underneath — right for something firing before every command, wrong + for the one moment you actually want to be sure. Expect the tail to be + noise; you are reading for the one record you would have missed. + - **When you already suspect a particular rule**: reach for + `search(content_type="rule")` to read full statements, or `get_rule(id)` + when one is about to bite. + + Either way, when a project is in scope pass its `project_id`: the answer is + then the global rules plus that project's own, never another project's. `enter_project(id)` lists the project's own rules by title. **`kind` says how much force a record carries, and it is never something to diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 4d8bd73..7462c89 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -44,8 +44,11 @@ client reads Agent Skills) and in each tool's description. The index: system. An `inception` key: ask what it inherits, then decide_project_inception. - RULES: nothing preloads; a rule arrives when your work matches it. Before a - consequential act, search(content_type="rule"). Silence means - nothing matched, not none. Rules bind; preferences guide. + consequential act — or before handing work back unsure you may finish it — + what_might_apply("what you are about to do"): the wide net, fifty ranked + candidates, no bar. search(content_type="rule") reads one you already + suspect. Silence means nothing matched, not none. Rules bind; preferences + guide. - RECALL: search before acting, scoped with the active project_id. - RECORD: create_task; a fix is kind="issue". add_task_log as you go; status in_progress on start, done on finish. Tag system_ids as you write. @@ -137,6 +140,11 @@ _READ_ONLY_TOOLS = frozenset({ # usual for these two: a session that cannot see the bar in force, or the # reason it was last moved, is a session that will move it again blind. "retrieval_surfaces", "retrieval_tuning_history", + # The wide net (#4103) — ranked rule candidates with no bar, for the + # moment before a consequential act. A pure read, and one a read key needs + # most: it is the surface the "ask before acting" reflex calls, and a key + # that could not reach it would be denied exactly the check it should run. + "what_might_apply", }) # Every tool that WRITES, by name. Nothing reads this set at runtime — a tool @@ -175,6 +183,7 @@ _WRITE_TOOLS = frozenset({ # retrieval tuning — a write in both senses: it moves the number the arm # reads, and it appends the reason to the audit trail (#4102). "tune_retrieval", + "migrate_retrieval_floor", # trash "restore", "purge_trash", }) diff --git a/src/scribe/mcp/tools/__init__.py b/src/scribe/mcp/tools/__init__.py index f292d41..cdc6944 100644 --- a/src/scribe/mcp/tools/__init__.py +++ b/src/scribe/mcp/tools/__init__.py @@ -6,6 +6,7 @@ from `mcp.server.build_mcp_server`. """ from scribe.mcp.tools import ( design_systems, milestones, notes, processes, projects, recent, repos, retrieval_tuning, + wide_net, rulebooks, search, shapes, snippets, systems, tags, tasks, trash, ) @@ -14,6 +15,7 @@ def register_all(mcp) -> None: """Register every tool module's tools on the given FastMCP instance.""" search.register(mcp) retrieval_tuning.register(mcp) + wide_net.register(mcp) notes.register(mcp) tasks.register(mcp) projects.register(mcp) diff --git a/src/scribe/mcp/tools/retrieval_tuning.py b/src/scribe/mcp/tools/retrieval_tuning.py index f4594e0..062ce36 100644 --- a/src/scribe/mcp/tools/retrieval_tuning.py +++ b/src/scribe/mcp/tools/retrieval_tuning.py @@ -5,6 +5,7 @@ bar did; these say what the bar IS, and let it be changed with the argument attached. """ from scribe.mcp._context import current_user_id +from scribe.services import retrieval_migration as migration_svc from scribe.services import retrieval_tuning as tuning_svc @@ -37,6 +38,33 @@ async def retrieval_surfaces() -> dict: are starting points: they were measured against one corpus with one embedding model and cannot be right for another install by construction. That is why this tool exists rather than a better set of defaults. + + `calibration` says what SPACE each number was chosen in, per dial, and + whether that space has moved. A floor is a cosine distance in one embedding + model's geometry over documents cut one particular way; swap the model and + every score shifts at once, re-cut the documents and the same record embeds + different text. Either way the number is a measurement of something that no + longer exists, and no amount of telemetry will say so — the scores simply + come out different and the bar keeps applying. + + Read the three fields apart: + + - `stale: true` with `model_changed` — the geometry is new. Every floor on + this install needs re-measuring, not adjusting; a number that meant "quite + similar" in the old space means nothing particular in this one. + - `stale: true` with `shape_changed` — the documents are cut differently. + The scale still holds, but what a record embeds has changed, so which + records clear a bar has. + - `stale: null`, `source: "unstamped"` — the dial was moved before Scribe + recorded this. It was measured under SOMETHING and there is no way to say + what, which is a different answer from "it is fine". Treat it as worth + re-measuring, and the next tuning call stamps it. + + NOTHING IS RETUNED AUTOMATICALLY on the strength of this, here or anywhere. + A stale stamp says a number is no longer a measurement; it does not say what + the number should be. That judgement wants the same procedure as any other + tuning change — `retrieval_telemetry(near_miss_samples=5)`, open the records + it names, then `tune_retrieval` with what you read in the reason. """ return {"surfaces": await tuning_svc.current_settings(current_user_id())} @@ -109,7 +137,53 @@ async def retrieval_tuning_history(surface: str = "", limit: int = 20) -> dict: } +async def migrate_retrieval_floor( + surface: str, sample: int = 200, apply: bool = False, +) -> dict: + """Carry one surface's floor across an embedding-model or chunker change. + + Reach for this when `retrieval_surfaces` reports a dial `stale` — and only + then. A floor is a cosine similarity, so a new embedding model moves every + score on the install at once and the stored number silently stops describing + anything. Re-deriving six floors by reading telemetry is the work this + avoids. + + WHAT IT TRANSFERS. Not the number — the SELECTIVITY. A floor's real content + is a decision about how much of what an arm sees is worth spending attention + on, and that decision survives a change of units. This measures what fraction + of the surface's recent calls the old floor admitted, re-scores those same + queries under the current model, and proposes the value that admits the same + fraction. + + DRY RUN BY DEFAULT. With `apply=False` it returns the arithmetic and writes + nothing. Read it before applying: the sample sizes, both score ranges, and + whether the shift looks like a change of scale or like a corpus that has not + finished re-embedding. A model change is the moment every number is + uncertain at once, which is the worst moment to let a statistic move dials + unattended. + + AND IT IS STILL A STARTING POINT. Percentile-preserving means the new floor + is as good as the old one was, not better — if the old floor was wrong, this + faithfully carries the wrongness onto the new scale. It is the number to + begin from while the surface collects enough calls to judge properly, which + is `retrieval_telemetry(near_miss_samples=5)` and `tune_retrieval` as usual. + + Args: + surface: the surface name from `retrieval_surfaces`. + sample: how many recent logged calls to re-score, default 200. Each one + is an embedding plus a scan, so this is real work; a surface with + only a handful of logged calls gives a percentile made of noise. + apply: False (default) returns the proposal. True writes it, as an + ordinary tuning event with the arithmetic in its reason — so a + migrated floor is reviewable and revertible like any other. + """ + return await migration_svc.migrate_floor( + current_user_id(), surface, sample=sample, apply=apply, + ) + + def register(mcp) -> None: mcp.tool(name="retrieval_surfaces")(retrieval_surfaces) + mcp.tool(name="migrate_retrieval_floor")(migrate_retrieval_floor) mcp.tool(name="tune_retrieval")(tune_retrieval) mcp.tool(name="retrieval_tuning_history")(retrieval_tuning_history) diff --git a/src/scribe/mcp/tools/wide_net.py b/src/scribe/mcp/tools/wide_net.py new file mode 100644 index 0000000..981db0c --- /dev/null +++ b/src/scribe/mcp/tools/wide_net.py @@ -0,0 +1,218 @@ +"""The wide net — every candidate that might govern what you are about to do (#4103). + +WHY THIS EXISTS + +Milestone 416 step 5, from the operator's compromise: + + "if you're worried about excluding potentially important data let limit it + to 50 entries or something like that" + +Fifty in the PUSH would be milestone 394 with extra steps — a wall the reader +skims, with the governing record indistinguishable from forty-nine others. +Fifty in a PULL is a different object: it arrives only when a session asks, it +crowds nothing out, and it is the honest home for "do not exclude potentially +important data". + +WHY IT IS NOT `search(content_type="rule")` + +That tool is the DEEP pull and should stay that way — `_search_rules` returns +`statement`, `why` and `how_to_apply` in full, on the reasoning that a caller +who went looking deserves the whole record rather than a summary to re-fetch. +Six results already run to thousands of tokens. + +This is the SHALLOW pull: many candidates, each just enough to decide whether +to open it. Opposite trade-off, so it is a second tool rather than a bigger +`limit` on the first. + +WHY THE TAIL IS TRUNCATED RATHER THAN FULL + +Because the step's premise needed correcting. The task said fifty "costs +nothing", and `_rule_hint_line` had already measured otherwise: a line runs +~143 tokens once its trigger is rendered, and #3855 tripled trigger lengths +across the corpus. Fifty of those is ~7,000 tokens — cheap next to an arm that +fires before every Bash call, but not free, and a tool that promises a free +wide net gets reached for casually and then regretted. + +So this reuses the graduated shape #3851 measured for the push: the top few +carry their trigger in full, the rest carry a cut of it. TRUNCATED, never +dropped — the trigger is what lets a reader judge relevance without opening +the record, and a teaser without one is just an id. +""" +from __future__ import annotations + +import textwrap +import time + +from scribe.mcp._context import current_user_id +from scribe.services.embeddings import semantic_search_rules +from scribe.services.retrieval_telemetry import record_retrieval + +# The telemetry `source`, and a PULL — never added to AMBIENT_SOURCES (those +# are deliveries nobody chose) and never confused with a push arm. Keeping it +# separate is load-bearing right now: the push arms' near-miss distributions +# are the evidence #4121 rests on, and a pull mixed into them would move the +# very numbers that step is arguing from. +SOURCE = "wide_net" + +# No bar. That is the point of the tool rather than an oversight — the caller +# asked for the wide net precisely because they do not trust a bar to decide +# for them here. Every row carries its score, so the reader sees where the +# ranking falls off and judges it themselves. +THRESHOLD = 0.0 + +MAX_LIMIT = 50 +DEFAULT_LIMIT = 25 + +# How many candidates carry their trigger in full before the rest are cut. +DEFAULT_DETAIL = 5 + +# The cut length for the tail. Long enough to carry the first clause of a +# trigger — which is where these state the act they are about — short enough +# that forty-five of them stay affordable. +_TEASER_CHARS = 140 + + +def _teaser(text: str) -> tuple[str, bool]: + """Shorten at a word break with a visible cut. Returns (text, was_cut). + + The technique is `plugin_context._goal_line`'s, and the fallback is the + part worth copying: `textwrap.shorten` returns a bare "…" when the string + is one unbroken word longer than the cap, and a raw slice ends mid-word + claiming to be the whole thing (#4036). Kept local rather than shared + because the two callers wrap it in different sentences; if a third appears, + that is the moment to extract it rather than now. + """ + flat = " ".join((text or "").split()) + if len(flat) <= _TEASER_CHARS: + return flat, False + short = textwrap.shorten(flat, width=_TEASER_CHARS, placeholder="…") + if short == "…": + short = flat[: _TEASER_CHARS - 1] + "…" + return short, True + + +async def what_might_apply( + query: str, + limit: int = DEFAULT_LIMIT, + detail: int = DEFAULT_DETAIL, + kind: str = "", + project_id: int = 0, +) -> dict: + """Every rule that might bear on what you are about to do, ranked, with no bar. + + REACH FOR THIS BEFORE A CONSEQUENTIAL OR IRREVERSIBLE ACT — a push, a + merge, a delete, a deploy, anything outward-facing — and before handing + work back because you are unsure whether you are allowed to finish it. + + It is the tool the "ask before a consequential act" reflex should call. + A rule reaches a session by retrieval, and the arms that push rules at you + have a small budget: they deliver the few highest-scoring candidates and + say nothing about what sat just underneath. That is right for an arm that + fires before every command and wrong for the one moment you actually want + to be sure. This is that moment's tool. + + WHAT IT RETURNS, AND WHY THE TAIL LOOKS LIKE NOISE + + There is no threshold. You get the `limit` nearest candidates whatever they + score, ordered, each with its score — so the tail IS expected to be + irrelevant, and that is the design. You are not reading the list for its + average quality; you are reading it for the one record you would otherwise + have missed. Scan the triggers, open what looks live with `get_rule(id)`, + and ignore the rest. + + The first few carry their trigger in full; the rest carry a cut of it, + marked `truncated`. A cut trigger is still enough to decide whether to + open the record, which is the whole job of a teaser. + + `kind` is on every row and is never something to infer: a **rule** must be + followed, a **preference** records how the operator wants work done. + Missing a rule is a mistake; missing a preference costs consistency. + + A NOTE ON WHAT THIS CANNOT DO. It is a pull, so it only helps if you ask. + The failure it was built for — a session withholding a routine action + because the rule permitting it never arrived — produces no tool call of its + own, so nothing will prompt you. Asking is the habit; this is where to put + it. + + Args: + query: what you are about to do, in the words you would use to + describe it — "push to dev after committing", "delete the staging + database", "merge dev to main". A command string works; a sentence + usually works better, because triggers are written as prose. + limit: how many candidates, default 25, capped at 50. + detail: how many carry their trigger in FULL before the rest are cut, + default 5. + kind: "rule" or "preference" to restrict; omit for both. + project_id: scope to one project — its own rules plus every global + one. Omit to ask the whole rulebook. + """ + uid = current_user_id() + limit = max(1, min(int(limit), MAX_LIMIT)) + detail = max(0, min(int(detail), limit)) + + report: dict = {} + started = time.perf_counter() + if project_id: + raw = await semantic_search_rules( + uid, query, limit=limit, threshold=THRESHOLD, kind=kind or None, + report=report, project_id=project_id, + ) + else: + raw = await semantic_search_rules( + uid, query, limit=limit, threshold=THRESHOLD, kind=kind or None, + report=report, everywhere=True, + ) + duration_ms = (time.perf_counter() - started) * 1000 + + candidates = [] + for rank, (score, rule) in enumerate(raw): + full = rank < detail + trigger, was_cut = ( + (" ".join((rule.when_to_apply or "").split()), False) + if full else _teaser(rule.when_to_apply or "") + ) + candidates.append({ + "id": rule.id, + "title": rule.title, + # Force, not topic. See the docstring — this is never inferred. + "kind": rule.kind, + # A rule in a rulebook topic is global; one on a project binds + # there alone (milestone 414). Which it is changes how far a + # reader should generalise from it. + "scope": "project" if rule.project_id else "global", + "when_to_apply": trigger, + "truncated": was_cut, + "score": round(float(score), 4), + }) + + # Logged as a pull, with `searched` honoured: a search that never ran must + # not be recorded as a ranker declining (#3765). + record_retrieval( + user_id=uid, + source=SOURCE, + query=query, + threshold=THRESHOLD, + limit=limit, + project_id=project_id or None, + is_task=None, + results=raw, + duration_ms=duration_ms, + best_available=report.get("best_available_score"), + best_available_id=report.get("best_available_id"), + searched=report.get("searched", True), + ) + + return { + "candidates": candidates, + "returned": len(candidates), + "detailed": min(detail, len(candidates)), + # Present so a caller can tell "the corpus offered nothing" from "the + # search never ran" — an empty query, an unavailable embedder and a + # failed query all return zero rows and mean different things (#3670). + "searched": bool(report.get("searched", True)), + "open_with": "get_rule(id)", + } + + +def register(mcp) -> None: + mcp.tool(name="what_might_apply")(what_might_apply) diff --git a/src/scribe/models/retrieval_tuning.py b/src/scribe/models/retrieval_tuning.py index b08d2c7..dff3fe8 100644 --- a/src/scribe/models/retrieval_tuning.py +++ b/src/scribe/models/retrieval_tuning.py @@ -75,6 +75,23 @@ class RetrievalTuningEvent(Base): # becomes a formality; the service refuses a blank one. reason: Mapped[str] = mapped_column(Text, nullable=False) + # WHAT SPACE THIS NUMBER WAS MEASURED IN (#4104). A floor is a cosine + # distance in one embedding model's geometry, over documents cut one + # particular way — change either and the number describes something that + # no longer exists. + # + # On the EVENT rather than beside the setting, because this table already + # holds one row per change and `current_settings` already reads the latest + # per dial. A parallel stamp row next to the value would be a second place + # to keep in sync, and the two disagreeing is worse than neither. + # + # Both NULLABLE for the rows written before this step: those were measured + # under something, but claiming to know which would be inventing a fact. + # Null here means "unstamped", which is a different and honest answer from + # naming a model that may be wrong. + embedding_model: Mapped[str | None] = mapped_column(Text, nullable=True) + shape_version: Mapped[int | None] = mapped_column(Integer, nullable=True) + __table_args__ = ( # The only read this table has: one surface's history, newest first. Index( @@ -94,4 +111,8 @@ class RetrievalTuningEvent(Base): "new_value": self.new_value, "actor": self.actor, "reason": self.reason, + # Null for every row written before #4104, and rendered as + # "unstamped" rather than guessed at — see the column comments. + "embedding_model": self.embedding_model, + "shape_version": self.shape_version, } diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 5a9d135..cf7573e 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -74,8 +74,14 @@ logger = logging.getLogger(__name__) # argument for them silently dropped — and from this step on those dials are # moved by the model, which is exactly the case where the operator needs the # argument to review. +# v17 (2026-09) added retrieval_tuning_events.embedding_model / shape_version +# (milestone 416 step 6): a floor is a distance in ONE embedding model's +# geometry over documents cut one particular way, so the number alone cannot +# say whether it still measures anything. Both travel NULLABLE and unfilled — +# a row written before the stamp existed restores unstamped, because inventing +# the model it was measured under would turn "unknown" into a stated fact. # Bump when the serialized schema changes. -BACKUP_VERSION = 16 +BACKUP_VERSION = 17 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -358,6 +364,12 @@ def _retrieval_tuning_event_rows(rows) -> list[dict]: "user_id": r.user_id, "surface": r.surface, "dial": r.dial, "old_value": r.old_value, "new_value": r.new_value, "actor": r.actor, "reason": r.reason, + # Carried, and NOT defaulted to the current model on the way out + # (#4104): a row that was unstamped when it was written is still + # unstamped after a round trip, and a backup that quietly filled + # the gap would turn "we don't know" into a stated fact. + "embedding_model": r.embedding_model, + "shape_version": r.shape_version, "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows @@ -1247,6 +1259,12 @@ async def _restore_v2(data: dict) -> dict: new_value=t_data.get("new_value"), actor=t_data.get("actor") or "model", reason=t_data.get("reason", ""), + # .get with no default, deliberately (v17): an archive written + # before the stamp existed has no key here, and None is the + # right answer for it — the same "unstamped" a pre-#4104 row + # carries in place. + embedding_model=t_data.get("embedding_model"), + shape_version=t_data.get("shape_version"), created_at=_dt(t_data.get("created_at")), )) stats["retrieval_tuning_events"] += 1 diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 000dfed..2dcbbe7 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -217,6 +217,38 @@ def embedding_text(title: str | None, body: str | None) -> str: # wipe migrations 0067/0077 had to do. CHUNKER_VERSION = 1 + +# The public name of the space every score lives in, and the two facts that +# can invalidate a tuned number (#4104). +# +# EMBEDDING_MODEL is `_MODEL_NAME` under a name other modules may read. It was +# private until this step, which is precisely why nothing outside this file +# could state what space a threshold was measured in — a floor is a distance in +# THIS model's geometry and means nothing in another's. +# +# The pair is what a stamp is made of, and the pairing is the point: a score +# changes when the model changes (different geometry) OR when the document +# shape changes (different text embedded for the same record). Either one +# invalidates a number that was measured before it. +EMBEDDING_MODEL = _MODEL_NAME + + +def calibration_stamp() -> dict: + """What a tuned retrieval number was measured against. + + ONE definition, because the alternative is each reader assembling the pair + and one of them forgetting a half. Returned as a dict rather than a string + so a mismatch can say WHICH half moved — "the model changed" and "the + chunker changed" call for different responses, and a fused string can only + report that something did. + + Deliberately says nothing about the CHAT model. A Claude upgrade changes no + score here and must never raise a recalibration prompt: a false alarm on + this surface teaches an operator to ignore the true one. `tests/ + test_calibration_stamp.py` asserts that absence rather than trusting it. + """ + return {"embedding_model": EMBEDDING_MODEL, "shape_version": CHUNKER_VERSION} + # Character budget approximating the model window. Tokens-per-char varies by # content — ~4 chars/token for prose, closer to 3 for code and tables — so 1400 # chars sits at roughly 350-470 tokens, leaving headroom for the title prefixed diff --git a/src/scribe/services/retrieval_migration.py b/src/scribe/services/retrieval_migration.py new file mode 100644 index 0000000..ba20473 --- /dev/null +++ b/src/scribe/services/retrieval_migration.py @@ -0,0 +1,248 @@ +"""Carrying a tuned floor across an embedding-model change (#4104). + +THE PROBLEM + +A floor is a cosine similarity, and a cosine similarity is a distance in one +model's geometry. `bge-small-en-v1.5` → `bge-base-en-v1.5` moves every score on +the install at once, in no direction anyone can predict per record. The numbers +in `settings` survive the swap unchanged and silently stop describing anything: +the bar keeps applying, the telemetry keeps filling, and nothing anywhere says +the arm is now cutting in a different place. + +Re-deriving six floors by hand is the alternative, and it is the thing the +operator asked for a way out of — *"a path for thresholds to be inherited by +the next version or different model so that they don't have to recalibrate a +lot."* + +WHAT TRANSFERS, AND WHY IT IS NOT THE NUMBER + +The raw cosine does not transfer. **The percentile it represented does.** A +floor's real content is a decision about SELECTIVITY — "admit roughly the top +fifth of what this arm sees" — and that decision is about the operator's +tolerance for noise on that surface, not about the embedder. It was true before +the model changed and is still true after. + +So: measure what fraction of this surface's calls the old floor admitted, using +the scores the old model actually produced; re-score the same queries under the +new one; and take the value that admits the same fraction. Same decision, new +units. + +WHY `best_available_score` IS THE FIGURE ON BOTH SIDES + +Because it is the one number that exists whether or not a call returned +anything — the highest score the corpus offered, before the bar was applied +(#3670). It is what `retrieval_logs` recorded under the old model, and it is +what a re-score reproduces under the new one, so the two sides are the same +measurement rather than two things that resemble each other. + +It also makes the re-score cheap to get right: `best_available_score` is +computed over the whole candidate set BEFORE `limit` and before any +`exclude_ids` the arm passed, so neither has to be reproduced here. Only the +corpus FILTERS matter, which is why `_RESCORERS` below carries those and +nothing else. + +WHAT THIS DOES NOT DO + +It does not fire by itself, and `migrate_floor` will not write anything unless +asked twice — `apply=True` on top of having read the dry run. A model change is +exactly the moment when every number is uncertain at once, which is the worst +possible moment to let a statistic move six dials unattended. The percentile is +a starting point on the new scale, in the same sense the shipped defaults are a +starting point: better than a stale number, not a substitute for reading what +the surface actually turned away. +""" +from __future__ import annotations + +import logging + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.retrieval_log import RetrievalLog +from scribe.services.embeddings import ( + calibration_stamp, + semantic_search_notes, + semantic_search_rules, +) +from scribe.services.retrieval_surfaces import floor_for, get_surface +from scribe.services.retrieval_tuning import set_dial + +logger = logging.getLogger(__name__) + +# How many of a surface's recent calls to re-score. Every one is an embedding +# plus a full scan, so this is a real cost — but a percentile off twenty calls +# is noise, and the arms that matter here log hundreds a week. +DEFAULT_SAMPLE = 200 + + +async def _rescore_rules(user_id: int, query: str, project_id: int | None, + kind: str | None) -> float | None: + rep: dict = {} + await semantic_search_rules( + user_id, query, limit=1, threshold=0.0, kind=kind, + report=rep, project_id=project_id or None, + ) + return rep.get("best_available_score") + + +async def _rescore_notes(user_id: int, query: str, project_id: int | None, + note_type, task_kind) -> float | None: + rep: dict = {} + await semantic_search_notes( + user_id, query, limit=1, threshold=0.0, + project_id=project_id or None, note_type=note_type, + task_kind=task_kind, scope="browse", report=rep, + ) + return rep.get("best_available_score") + + +# ONE re-scorer per surface, carrying that arm's corpus filters and nothing +# else. These filters are stated a second time here — the arms in +# `plugin_context` are where they are first declared — and that duplication is +# deliberate rather than overlooked: the alternative is calling the arms +# themselves, which build a menu, write telemetry and record records as +# surfaced. A migration that logged two hundred fake retrievals would corrupt +# the very table the next tuning decision reads. +# +# `tests/test_retrieval_migration.py` asserts every registry surface has an +# entry, so a seventh arm cannot quietly become un-migratable. +_RESCORERS = { + "auto_inject": lambda u, q, p: _rescore_notes(u, q, p, None, None), + "write_path": lambda u, q, p: _rescore_notes( + u, q, p, ("snippet", "note"), "issue" + ), + "write_path_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "pre_tool_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "prompt_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "report_preference": lambda u, q, p: _rescore_rules(u, q, p, "preference"), +} + + +def _floor_admitting(scores: list[float], fraction: float) -> float: + """The floor that admits `fraction` of `scores`, on this scale. + + Deliberately exact rather than interpolated: with the scores sorted + highest-first, the k-th one IS the bar that admits exactly k. An + interpolated quantile would return a number no observed call sits on, which + is harder to sanity-check against the sample it came from. + + A fraction rounding to zero returns a floor just above the best score seen — + an arm that admitted nothing keeps admitting nothing, rather than being + quietly reopened by a migration. + """ + ranked = sorted(scores, reverse=True) + k = int(round(fraction * len(ranked))) + if k <= 0: + return min(1.0, ranked[0] + 1e-6) + return ranked[min(k, len(ranked)) - 1] + + +async def migrate_floor( + user_id: int, + surface: str, + *, + sample: int = DEFAULT_SAMPLE, + apply: bool = False, +) -> dict: + """Recompute one surface's floor on the current model, preserving selectivity. + + Returns the working: how many calls were sampled, what fraction the old + floor admitted, and what value admits the same fraction now. Writes nothing + unless `apply=True`, and when it does it writes an ordinary tuning event + with the arithmetic in its reason — a migrated floor is reviewable and + revertible on exactly the same terms as one a reader chose. + """ + get_surface(surface) # refuses an unknown name + rescore = _RESCORERS.get(surface) + if rescore is None: + raise ValueError( + f"no re-scorer for surface {surface!r}. A surface that cannot be " + "re-scored cannot be migrated — add it to _RESCORERS beside the " + "arm's own corpus filters." + ) + + old_floor = await floor_for(user_id, surface) + async with async_session() as session: + rows = (await session.execute( + select( + RetrievalLog.query, + RetrievalLog.project_id, + RetrievalLog.best_available_score, + ) + .where( + RetrievalLog.source == surface, + RetrievalLog.user_id == user_id, + RetrievalLog.best_available_score.is_not(None), + RetrievalLog.query.is_not(None), + RetrievalLog.query != "", + ) + .order_by(RetrievalLog.id.desc()) + .limit(max(1, int(sample))) + )).all() + + if not rows: + # Not an error. A surface with no logged calls has no evidence of what + # its floor was doing, and inventing a migration for it would be the + # exact failure this module's docstring warns about. + return { + "surface": surface, "migrated": False, + "why": "no logged calls carry a best_available_score for this " + "surface, so there is no old distribution to preserve", + "sampled": 0, "old_floor": old_floor, + } + + old_scores = [float(r.best_available_score) for r in rows] + admitted = sum(1 for s in old_scores if s >= old_floor) + fraction = admitted / len(old_scores) + + new_scores: list[float] = [] + for r in rows: + score = await rescore(user_id, r.query, r.project_id) + if score is not None: + new_scores.append(float(score)) + + if not new_scores: + # The corpus answered nothing for any sampled query. Almost always an + # embedder that has not finished backfilling under the new model — + # migrating from it would set every floor off an empty distribution. + return { + "surface": surface, "migrated": False, + "why": "re-scoring returned nothing for any sampled query — the " + "corpus is probably not embedded under the current model yet", + "sampled": len(rows), "old_floor": old_floor, + "old_admit_rate": round(fraction, 4), + } + + proposed = round(min(1.0, max(0.0, _floor_admitting(new_scores, fraction))), 4) + stamp = calibration_stamp() + reason = ( + f"Migrated across a calibration change, preserving selectivity: the old " + f"floor {old_floor} admitted {admitted} of {len(old_scores)} sampled " + f"calls ({fraction:.1%}); {proposed} admits the same share of " + f"{len(new_scores)} queries re-scored under " + f"{stamp['embedding_model']}/shape {stamp['shape_version']}. The " + f"percentile is what carried across, not the number — spot-check the " + f"arm before trusting it." + ) + + result = { + "surface": surface, + "migrated": False, + "sampled": len(rows), + "rescored": len(new_scores), + "old_floor": old_floor, + "old_admit_rate": round(fraction, 4), + "old_score_range": [round(min(old_scores), 4), round(max(old_scores), 4)], + "new_score_range": [round(min(new_scores), 4), round(max(new_scores), 4)], + "proposed_floor": proposed, + "calibration": stamp, + "reason": reason, + } + if not apply: + return result + + result["migrated"] = True + result["applied"] = await set_dial( + user_id, surface, "floor", proposed, reason=reason, actor="model", + ) + return result diff --git a/src/scribe/services/retrieval_surfaces.py b/src/scribe/services/retrieval_surfaces.py index bd6063e..88ae47b 100644 --- a/src/scribe/services/retrieval_surfaces.py +++ b/src/scribe/services/retrieval_surfaces.py @@ -103,6 +103,31 @@ class Surface: asks: str over: str fires: str + measured_model: str = "BAAI/bge-small-en-v1.5" + measured_shape: int = 1 + """What the SHIPPED defaults above were measured against (#4104). + + A floor is a distance in one embedding model's geometry, over documents cut + one particular way. Either can change, and when one does every number in + this table describes something that no longer exists. + + TWO FIELDS, NEVER ONE FUSED STRING (rule 149). A mismatch has to be able to + say WHICH half moved: a new embedding model and a re-cut document shape + invalidate the same numbers for different reasons and call for different + responses. `"@"` could only report that something changed, which + is the answer nobody can act on. Same reason `calibration_stamp()` returns + a dict and the event table gives each half its own column. + + Recorded per surface rather than once for the module because they need not + move together: a surface retuned after a model change carries the new stamp + while its untouched siblings still carry the old one, and telling those + apart is the whole job. + + LITERALS, deliberately, rather than an import of the live values — a stamp + says what was true when the number was chosen, so one that tracked the + current model would always agree with it and could never report staleness. + """ + budget_falls_back_to: str = "" """A budget key to inherit when this surface has none of its own set. diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py index 471df2f..f6a8682 100644 --- a/src/scribe/services/retrieval_tuning.py +++ b/src/scribe/services/retrieval_tuning.py @@ -47,6 +47,7 @@ from sqlalchemy import select from scribe.models import async_session from scribe.models.retrieval_tuning import RetrievalTuningEvent +from scribe.services.embeddings import calibration_stamp from scribe.services.retrieval_surfaces import ( MAX_BUDGET, budget_for, @@ -86,6 +87,50 @@ def _clean_reason(reason: str) -> str: return text +def _calibration(row, s, live: dict) -> dict: + """What space one dial's number was chosen in, and whether that space moved. + + Three sources, and they are not interchangeable: + + - `tuned` — the dial was moved after #4104 and the event carries its + stamp. The only case where the answer is known. + - `shipped` — the dial has never been moved, so the number in force is + the registry default, and the registry records what THAT + was measured against (`Surface.measured_model`). + - `unstamped` — the dial was moved before this step existed. It was + measured under something; naming it would be inventing a + fact, so `stale` is None rather than True or False. + "Unknown" and "fine" must not render the same. + + `model_changed` and `shape_changed` are reported apart (rule 149) because + they call for different responses: a new embedding model means every number + is a distance in a geometry that no longer exists, while a re-cut document + shape means the same records now embed different text. A caller that only + ever sees `stale: true` cannot tell those apart. + """ + if row is None: + model, shape, source = s.measured_model, s.measured_shape, "shipped" + elif row.embedding_model is None and row.shape_version is None: + return { + "source": "unstamped", "embedding_model": None, + "shape_version": None, "model_changed": None, + "shape_changed": None, "stale": None, + } + else: + model, shape, source = row.embedding_model, row.shape_version, "tuned" + + model_changed = model != live["embedding_model"] + shape_changed = shape != live["shape_version"] + return { + "source": source, + "embedding_model": model, + "shape_version": shape, + "model_changed": model_changed, + "shape_changed": shape_changed, + "stale": model_changed or shape_changed, + } + + async def current_settings(user_id: int) -> list[dict]: """Every tunable surface with its live pair and its last stated reason. @@ -94,23 +139,44 @@ async def current_settings(user_id: int) -> list[dict]: often — because a floor cannot be moved sensibly without those three — and the reason last given, so the next change argues with the last one instead of overwriting it blind. + + From #4104 it also carries `calibration` per dial: the embedding model and + document shape the number in force was measured in, and whether either has + moved since. NOTHING AUTO-RETUNES on the strength of it. A stale stamp says + a number is a measurement of a space that no longer exists — it does not + say what the number should be now, and the one time a statistic was allowed + to answer that question it was wrong (see the module docstring). The stamp + is here so a reader knows which floors to go and re-measure. """ + live = calibration_stamp() out: list[dict] = [] async with async_session() as session: for name in surface_names(): s = get_surface(name) - rows = ( - await session.execute( - select(RetrievalTuningEvent) - .where( - RetrievalTuningEvent.surface == name, - RetrievalTuningEvent.user_id == user_id, + # ONE QUERY PER DIAL, not one `limit(len(DIALS))` over both. + # "The newest two rows" is not "the newest row of each kind": a + # surface whose floor was moved three times and whose budget was + # moved once returns two floor rows, and the budget change + # disappears. That was a missing reason when this only fed + # `last_change`; since #4104 it is also a WRONG calibration answer — + # a tuned dial reporting as "still on the shipped default", which is + # the one state a reader would not think to check. + last = {} + for dial in DIALS: + row = ( + await session.execute( + select(RetrievalTuningEvent) + .where( + RetrievalTuningEvent.surface == name, + RetrievalTuningEvent.user_id == user_id, + RetrievalTuningEvent.dial == dial, + ) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(1) ) - .order_by(RetrievalTuningEvent.created_at.desc()) - .limit(len(DIALS)) - ) - ).scalars().all() - last = {r.dial: r for r in rows} + ).scalars().first() + if row is not None: + last[dial] = row out.append({ "surface": name, "floor": await floor_for(user_id, name), @@ -126,6 +192,13 @@ async def current_settings(user_id: int) -> list[dict]: "last_change": { dial: last[dial].to_dict() for dial in DIALS if dial in last }, + # Always present for BOTH dials, unlike `last_change`: an + # untouched dial still has a number in force, and that number + # was still measured in some space. Reported, never acted on — + # see the note below on why nothing auto-retunes. + "calibration": { + dial: _calibration(last.get(dial), s, live) for dial in DIALS + }, }) return out @@ -179,9 +252,16 @@ async def set_dial( await set_setting(user_id, key, stored) async with async_session() as session: + # Stamped with the space this number was chosen in (#4104). Read at + # write time rather than passed in: the caller measuring a floor and + # the caller recording it are the same call, so there is no window in + # which they could disagree. + stamp = calibration_stamp() session.add(RetrievalTuningEvent( user_id=user_id, surface=surface, dial=dial, old_value=old, new_value=applied, actor=actor, reason=text, + embedding_model=stamp["embedding_model"], + shape_version=stamp["shape_version"], )) await session.commit() diff --git a/tests/test_calibration_stamp.py b/tests/test_calibration_stamp.py new file mode 100644 index 0000000..6e2d530 --- /dev/null +++ b/tests/test_calibration_stamp.py @@ -0,0 +1,238 @@ +"""A tuned number carries the space it was measured in — and only that (#4104). + +WHY THIS EXISTS + +Milestone 416 step 6 answers *"a path for thresholds to be inherited by the next +version or different model so that they don't have to recalibrate a lot."* The +mechanism is a stamp: every tuning event records the embedding model and +document shape the number was chosen under, and a mismatch is reported rather +than left to be noticed. + +THE ONE THAT MATTERS MOST IS THE ABSENCE + +`test_no_chat_model_identifier_reaches_the_calibration_path` is the load-bearing +guard here, and it is a guard against a plausible mistake rather than a +hypothetical one. These floors live in BAAI/bge-small-en-v1.5's vector space. +The CHAT model — Claude — produces none of these scores and changes none of +them, so a Claude upgrade must trigger nothing at all. If it ever did, the +operator would be asked to recalibrate six dials for no reason, learn that this +surface cries wolf, and ignore it on the day `bge-small` → `bge-base` actually +moves every score on the install. A false alarm here does not cost a +notification; it costs the real alarm. + +The rest pins the honest-unknown behaviour: a row written before stamps existed +reports as `unstamped` with `stale: None`, never as fine and never as a model +name somebody guessed at. +""" +import re +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services import retrieval_tuning as rt +from scribe.services.retrieval_surfaces import SURFACES, get_surface +from tests.helpers import make_mock_session, tool_doc + +# Names for the thing that talks, as opposed to the thing that embeds. Any of +# these appearing in the calibration path means a chat-model change could move a +# stamp — see the module docstring for why that is the expensive failure. +_CHAT_MODEL_WORDS = ( + "claude", "opus", "sonnet", "haiku", "gpt", "llama", "mistral", "gemini", + "anthropic", "openai", "chat_model", "chatmodel", "model_version", +) + + +def test_calibration_stamp_names_the_embedder_and_the_shape(): + stamp = emb.calibration_stamp() + assert stamp == { + "embedding_model": emb.EMBEDDING_MODEL, + "shape_version": emb.CHUNKER_VERSION, + } + # Two keys, never one fused string (rule 149): a mismatch has to be able to + # say WHICH half moved, because a new embedder and a re-cut document + # invalidate the same numbers for different reasons. + assert len(stamp) == 2 + + +def test_the_stamp_tracks_the_embedding_model_constant_not_a_copy_of_it(): + """A second literal would drift, and drift here reads as a model change.""" + with patch.object(emb, "EMBEDDING_MODEL", "some/other-model"): + assert emb.calibration_stamp()["embedding_model"] == "some/other-model" + with patch.object(emb, "CHUNKER_VERSION", 99): + assert emb.calibration_stamp()["shape_version"] == 99 + + +def test_no_chat_model_identifier_reaches_the_calibration_path(): + """THE GUARD. A Claude upgrade must move nothing in this path. + + Structural rather than behavioural on purpose: the failure is someone + *adding* a chat-model field in good faith ("surely the model matters"), and + no behavioural test catches a field that has not been written yet. Asserted + over the source of every module that produces or consumes a stamp. + """ + import inspect + + from scribe.services import retrieval_migration as rm + + for label, obj in ( + ("calibration_stamp", emb.calibration_stamp), + ("_calibration", rt._calibration), + ("migrate_floor", rm.migrate_floor), + ): + src = inspect.getsource(obj).lower() + # Comments and docstrings here legitimately discuss the chat model in + # order to rule it out, so strip prose and assert on code only. + code = "\n".join( + line.split("#", 1)[0] + for line in re.sub(r'""".*?"""', "", src, flags=re.S).splitlines() + ) + for word in _CHAT_MODEL_WORDS: + assert word not in code, f"{label} names the chat model: {word!r}" + + +@pytest.mark.asyncio +async def test_a_moved_dial_is_written_with_the_live_stamp(): + session = make_mock_session() + with patch.object(rt, "async_session", MagicMock(return_value=session)), \ + patch.object(rt, "set_setting", AsyncMock()), \ + patch.object(rt, "floor_for", AsyncMock(return_value=0.72)), \ + patch.object(rt, "budget_for", AsyncMock(return_value=3)), \ + patch.object(rt, "calibration_stamp", + MagicMock(return_value={"embedding_model": "m/x", + "shape_version": 7})): + await rt.set_dial( + 1, "prompt_rule", "floor", 0.66, + reason="read the five refused records; four were genuine matches", + ) + event = session.add.call_args[0][0] + assert event.embedding_model == "m/x" + assert event.shape_version == 7 + + +def _row(model="BAAI/bge-small-en-v1.5", shape=1): + return MagicMock(embedding_model=model, shape_version=shape) + + +def test_an_untouched_dial_reports_the_shipped_default_it_is_still_on(): + s = get_surface("prompt_rule") + live = {"embedding_model": s.measured_model, "shape_version": s.measured_shape} + cal = rt._calibration(None, s, live) + assert cal["source"] == "shipped" + assert cal["stale"] is False + + +def test_a_dial_moved_before_stamps_existed_is_unknown_not_fine(): + """`stale: None`, because "we don't know" and "it's fine" are not the same. + + Collapsing them would hide the dials MOST likely to be wrong — the ones + somebody tuned longest ago. + """ + s = get_surface("prompt_rule") + cal = rt._calibration(_row(model=None, shape=None), s, emb.calibration_stamp()) + assert cal["source"] == "unstamped" + assert cal["stale"] is None + assert cal["model_changed"] is None and cal["shape_changed"] is None + + +def test_a_model_change_and_a_shape_change_are_reported_apart(): + """Rule 149. Two conditions, two answers — they call for different work.""" + s = get_surface("prompt_rule") + live = {"embedding_model": "new/model", "shape_version": 1} + cal = rt._calibration(_row(model="old/model", shape=1), s, live) + assert (cal["model_changed"], cal["shape_changed"], cal["stale"]) == ( + True, False, True, + ) + + live = {"embedding_model": "old/model", "shape_version": 2} + cal = rt._calibration(_row(model="old/model", shape=1), s, live) + assert (cal["model_changed"], cal["shape_changed"], cal["stale"]) == ( + False, True, True, + ) + + +def test_a_dial_tuned_under_the_live_stamp_is_not_stale(): + s = get_surface("prompt_rule") + live = emb.calibration_stamp() + cal = rt._calibration( + _row(model=live["embedding_model"], shape=live["shape_version"]), s, live, + ) + assert cal["source"] == "tuned" + assert cal["stale"] is False + + +def test_every_surface_ships_a_stamp_for_its_defaults(): + """A default with no stamp cannot be told from one measured yesterday.""" + for name, s in SURFACES.items(): + assert s.measured_model, f"{name} default has no measured model" + assert isinstance(s.measured_shape, int), f"{name} shape is not a version" + + +def test_the_registry_stamp_is_a_literal_not_the_live_value(): + """It says what WAS true, so it must not follow the current constants. + + A field that tracked `EMBEDDING_MODEL` would agree with it forever and could + never report the one thing it exists to report. + """ + with patch.object(emb, "EMBEDDING_MODEL", "some/other-model"): + assert get_surface("prompt_rule").measured_model != "some/other-model" + + +def test_the_tool_says_nothing_auto_retunes(): + """The contract an agent reads. A stale stamp is a prompt to MEASURE. + + Without this, the obvious next move on seeing `stale: true` is to move the + dial — which is tuning from a statistic, the exact failure #4102 measured + pointing the wrong way. + """ + doc = tool_doc("scribe.mcp.tools.retrieval_tuning", "retrieval_surfaces") + assert "calibration" in doc + assert "NOTHING IS RETUNED AUTOMATICALLY" in doc + assert "unstamped" in doc + + +@pytest.mark.asyncio +async def test_a_dial_is_read_per_dial_not_from_the_newest_two_rows(): + """The floor's history must not be able to bury the budget's. + + "The newest two rows" and "the newest row of each dial" differ the moment + one dial moves more often than the other — which is the normal case, since + floors get walked and budgets rarely do. Before this was one query per dial, + a surface with three floor changes and one budget change reported the budget + as untouched: a WRONG calibration answer rather than a missing one, and + wrong in the direction a reader would not think to check. + """ + session = make_mock_session() + per_dial = { + "floor": MagicMock(dial="floor", embedding_model="old/model", + shape_version=1, + to_dict=MagicMock(return_value={"dial": "floor"})), + "budget": MagicMock(dial="budget", embedding_model="old/model", + shape_version=1, + to_dict=MagicMock(return_value={"dial": "budget"})), + } + calls = [] + + def execute(stmt): + # The dial is whichever the WHERE clause names; a query that did not + # scope by dial would render this stand-in unable to answer, which is + # the point. + sql = str(stmt.compile(compile_kwargs={"literal_binds": True})) + dial = "budget" if "'budget'" in sql else "floor" + calls.append(dial) + result = MagicMock() + result.scalars.return_value.first.return_value = per_dial[dial] + return result + + session.execute = AsyncMock(side_effect=execute) + with patch.object(rt, "async_session", MagicMock(return_value=session)), \ + patch.object(rt, "floor_for", AsyncMock(return_value=0.7)), \ + patch.object(rt, "budget_for", AsyncMock(return_value=3)): + out = await rt.current_settings(1) + + assert calls.count("floor") == len(SURFACES) + assert calls.count("budget") == len(SURFACES) + for row in out: + # Both dials tuned under a model that is not the live one. + assert row["calibration"]["budget"]["source"] == "tuned" + assert row["calibration"]["budget"]["stale"] is True diff --git a/tests/test_retrieval_migration.py b/tests/test_retrieval_migration.py new file mode 100644 index 0000000..e46820e --- /dev/null +++ b/tests/test_retrieval_migration.py @@ -0,0 +1,159 @@ +"""Carrying a floor across a calibration change (#4104). + +WHAT THIS PINS + + 1. **The percentile is what transfers, not the number.** A floor's content is + a decision about selectivity; the cosine expressing it is units. So a + migration that admitted 30% before admits 30% after, whatever the new + scores look like. + 2. **Nothing is written unless asked twice.** `apply` defaults to False. A + model change makes every number uncertain at once, which is the worst + moment to let a statistic move six dials unattended. + 3. **Missing evidence is a refusal, not a guess.** No logged calls, or a + corpus that re-scores to nothing, returns `migrated: False` with a reason — + never a floor computed off an empty distribution, which is how an install + mid-backfill would end up with every bar at zero. + 4. **Every registry surface can be migrated.** A seventh arm that nobody adds + a re-scorer for is one whose floor silently cannot survive a model change. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import retrieval_migration as rm +from scribe.services.retrieval_surfaces import SURFACES +from tests.helpers import make_mock_session, tool_doc + + +def _logs(pairs): + """Rows as `migrate_floor` reads them: (query, project_id, old score).""" + return [MagicMock(query=q, project_id=p, best_available_score=s) + for q, p, s in pairs] + + +def _session_with(rows): + """`.all()` is SYNCHRONOUS on a Result, so it needs a MagicMock. + + `make_mock_session` is an AsyncMock, and every child of an AsyncMock is one + too — leaving `.all` as it comes hands the service a coroutine where it + expects a list, the same trap the helper's docstring flags for `add`. + """ + session = make_mock_session() + session.execute.return_value = MagicMock(all=MagicMock(return_value=rows)) + return session + + +def test_every_surface_has_a_rescorer(): + """Otherwise a surface's floor cannot cross a model change at all.""" + assert set(rm._RESCORERS) == set(SURFACES) + + +def test_the_floor_that_admits_a_fraction_is_an_observed_score(): + scores = [0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1, 0.05] + # 30% of ten is three; the third-best score is the bar that admits exactly + # those three. Exact rather than interpolated, so the answer can be checked + # against the sample it came from. + assert rm._floor_admitting(scores, 0.3) == 0.7 + assert sum(1 for s in scores if s >= 0.7) == 3 + + +def test_a_surface_that_admitted_nothing_keeps_admitting_nothing(): + """A migration must not quietly reopen an arm the operator had shut.""" + scores = [0.5, 0.4, 0.3] + assert rm._floor_admitting(scores, 0.0) > max(scores) + + +@pytest.mark.asyncio +async def test_selectivity_is_preserved_across_a_scale_change(): + """The whole idea, on numbers that move a long way. + + Old scores cluster near 0.7 with a bar at 0.72 admitting two of five. New + scores sit far lower — a different geometry — and the proposal is the value + that admits two of five there, not anything resembling 0.72. + """ + rows = _logs([ + ("q1", 2, 0.80), ("q2", 2, 0.75), ("q3", 2, 0.70), + ("q4", 2, 0.60), ("q5", 2, 0.50), + ]) + new = {"q1": 0.42, "q2": 0.38, "q3": 0.31, "q4": 0.22, "q5": 0.10} + rescore = AsyncMock(side_effect=lambda u, q, p: new[q]) + + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.72)), \ + patch.dict(rm._RESCORERS, {"prompt_rule": rescore}): + out = await rm.migrate_floor(1, "prompt_rule") + + assert out["old_admit_rate"] == 0.4 # 0.80 and 0.75 cleared 0.72 + assert out["proposed_floor"] == 0.38 # admits 0.42 and 0.38 — also two + assert out["migrated"] is False # dry run by default + + +@pytest.mark.asyncio +async def test_a_dry_run_writes_nothing(): + rows = _logs([("q1", None, 0.9), ("q2", None, 0.1)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=0.4)}): + out = await rm.migrate_floor(1, "prompt_rule") + set_dial.assert_not_called() + assert "proposed_floor" in out + + +@pytest.mark.asyncio +async def test_applying_writes_an_ordinary_tuning_event_with_the_arithmetic(): + """A migrated floor is reviewable and revertible like any other change.""" + rows = _logs([("q1", None, 0.9), ("q2", None, 0.1)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock(return_value={})) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=0.4)}): + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + + assert out["migrated"] is True + kwargs = set_dial.call_args.kwargs + reason = kwargs["reason"] + # The reason has to carry the working, not just the verdict — it is what the + # operator reads to decide whether to keep the number. + assert "0.5" in reason and "sampled" in reason + assert kwargs["actor"] == "model" + + +@pytest.mark.asyncio +async def test_no_logged_calls_refuses_rather_than_inventing_a_distribution(): + with patch.object(rm, "async_session", MagicMock(return_value=_session_with([]))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial: + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + assert out["migrated"] is False + assert "no logged calls" in out["why"] + set_dial.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_corpus_that_rescores_to_nothing_refuses(): + """The mid-backfill case: every bar would otherwise be set off no data.""" + rows = _logs([("q1", None, 0.9), ("q2", None, 0.8)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=None)}): + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + assert out["migrated"] is False + assert "not embedded" in out["why"] + set_dial.assert_not_called() + + +@pytest.mark.asyncio +async def test_an_unknown_surface_is_refused(): + with pytest.raises(ValueError): + await rm.migrate_floor(1, "promptrule") + + +def test_the_tool_says_it_is_a_starting_point_and_defaults_to_a_dry_run(): + doc = tool_doc("scribe.mcp.tools.retrieval_tuning", "migrate_retrieval_floor") + assert "DRY RUN BY DEFAULT" in doc + # Percentile-preserving carries the old floor's wrongness forward faithfully. + # A reader who misses that will treat a migrated number as a measured one. + assert "STARTING POINT" in doc.upper() + assert "stale" in doc diff --git a/tests/test_retrieval_tuning.py b/tests/test_retrieval_tuning.py index 733315d..d55169f 100644 --- a/tests/test_retrieval_tuning.py +++ b/tests/test_retrieval_tuning.py @@ -183,17 +183,16 @@ def test_the_tool_teaches_reading_the_records_not_the_percentile(): assert "69" in doc -def test_all_three_tools_are_registered(): +def test_every_tool_in_the_module_is_registered(): from scribe.mcp.tools import retrieval_tuning as tool - names = [] + from tests.helpers import FakeMCP - class _MCP: - def tool(self, name): - names.append(name) - return lambda fn: fn - - tool.register(_MCP()) - assert names == [ - "retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history", + mcp = FakeMCP() + tool.register(mcp) + # Order is the module's, and asserted rather than sorted: an unregistered + # tool is invisible to every caller, so the list is worth reading literally. + assert mcp.names == [ + "retrieval_surfaces", "migrate_retrieval_floor", + "tune_retrieval", "retrieval_tuning_history", ] diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index a74ac45..40ed6f2 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -23,7 +23,7 @@ def test_backup_version_is_current(): (Named for the number it asserted until v10, which is exactly the drift a name-carrying-a-value invites; it now says what it checks.)""" - assert backup.BACKUP_VERSION == 16 + assert backup.BACKUP_VERSION == 17 def _exportable_note(**over): diff --git a/tests/test_wide_net.py b/tests/test_wide_net.py new file mode 100644 index 0000000..cce7ee9 --- /dev/null +++ b/tests/test_wide_net.py @@ -0,0 +1,243 @@ +"""The wide net: many candidates, no bar, and a cost that stays bounded (#4103). + +WHY THIS EXISTS + +Milestone 416 step 5 moves "do not exclude potentially important data" off the +push, where fifty candidates would be a wall nobody reads, and onto a pull, +where they arrive only when a session asks for them. + +Three things have to hold, and each has a way of quietly failing: + + 1. **No bar.** The moment this tool serves is the one where the caller does + not trust a threshold to decide for them. A default floor creeping in + here would turn the wide net into the narrow one and nothing would look + wrong — the results would simply be fewer. + 2. **A bounded cost.** The step was filed claiming fifty "costs nothing". + `_rule_hint_line` had already measured ~143 tokens for a line with its + trigger rendered, so fifty is ~7,000. The graduated shape (#3851) is what + keeps this affordable, and a change that renders every trigger in full + would pass every other test here. + 3. **A pull, logged as one.** The push arms' near-miss distributions are the + evidence #4121 argues from. A pull mixed into them moves those numbers. +""" +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp.tools import wide_net +from tests.helpers import FakeMCP, fake_rule, tool_doc + + +def _hits(n=3, trigger="Running git push", **over): + """n (score, rule) pairs, descending, the shape the service returns.""" + return [ + (0.9 - i * 0.01, + fake_rule(id=i + 1, title=f"rule {i + 1}", when_to_apply=trigger, **over)) + for i in range(n) + ] + + +def _patched(hits, report=None): + """Patch the search and the telemetry sink; hand back the search mock.""" + async def _search(*a, **kw): + if report is not None and "report" in kw and kw["report"] is not None: + kw["report"].update(report) + return hits + return patch.object(wide_net, "semantic_search_rules", AsyncMock(side_effect=_search)) + + +@pytest.mark.asyncio +async def test_no_bar_reaches_the_search(): + """THE POINT OF THE TOOL. A floor here would narrow the net silently.""" + with _patched(_hits()) as search, \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("push to dev") + + assert search.await_args.kwargs["threshold"] == 0.0 + assert wide_net.THRESHOLD == 0.0 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("asked, expected", [(999, 50), (0, 1), (-5, 1), (25, 25)]) +async def test_the_limit_is_clamped_to_the_cap(asked, expected): + with _patched(_hits()) as search, \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("q", limit=asked) + + assert search.await_args.kwargs["limit"] == expected + + +@pytest.mark.asyncio +async def test_the_head_carries_its_trigger_whole_and_the_tail_is_cut(): + """The graduated shape (#3851), which is what makes fifty affordable. + + A regression that rendered every trigger in full would satisfy every other + assertion in this file, so the cut is pinned on both sides: the head is + NOT marked truncated and the tail IS. + """ + long_trigger = " ".join(["running a git command before pushing anything"] * 12) + with _patched(_hits(6, trigger=long_trigger)), \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q", detail=2) + + head, tail = out["candidates"][:2], out["candidates"][2:] + assert all(c["truncated"] is False for c in head) + assert all(c["when_to_apply"] == " ".join(long_trigger.split()) for c in head) + assert all(c["truncated"] is True for c in tail) + assert all(len(c["when_to_apply"]) <= wide_net._TEASER_CHARS + 1 for c in tail) + assert out["detailed"] == 2 + + +@pytest.mark.asyncio +async def test_a_short_trigger_is_never_marked_truncated(): + """`truncated` is a claim about this row, not about its rank. A tail row + whose trigger already fits must not claim a cut that did not happen.""" + with _patched(_hits(4, trigger="Running git push")), \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q", detail=1) + + assert all(c["truncated"] is False for c in out["candidates"]) + + +def test_the_cut_breaks_on_a_word_and_says_it_was_cut(): + """#4036's lesson, borrowed: a raw slice ends mid-word and reads as the + whole thing.""" + text, cut = wide_net._teaser("alpha beta gamma delta " * 40) + assert cut is True + assert text.endswith("…") + # Broke on a word, so no partial token sits before the marker. + assert not text.removesuffix("…").rstrip().endswith(("alph", "bet", "gam")) + + +def test_one_unbroken_word_still_yields_text_rather_than_a_bare_marker(): + """`textwrap.shorten` returns just "…" here, which would render a teaser + carrying no information at all.""" + text, cut = wide_net._teaser("x" * 500) + assert cut is True and text != "…" + assert len(text) == wide_net._TEASER_CHARS + + +@pytest.mark.asyncio +async def test_every_row_says_its_force_and_its_scope(): + """`kind` is never inferred — a rule must be followed, a preference guides + — and `scope` says how far a reader should generalise from it.""" + hits = [ + (0.8, fake_rule(id=1, kind="rule", project_id=None)), + (0.7, fake_rule(id=2, kind="preference", project_id=44)), + ] + with _patched(hits), patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q") + + assert [c["kind"] for c in out["candidates"]] == ["rule", "preference"] + assert [c["scope"] for c in out["candidates"]] == ["global", "project"] + + +# ── telemetry: a pull, and never mistaken for a push ──────────────────────── + +@pytest.mark.asyncio +async def test_the_call_is_logged_under_its_own_pull_source(): + with _patched(_hits()), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("push to dev") + + kw = rec.call_args.kwargs + assert kw["source"] == wide_net.SOURCE == "wide_net" + assert kw["threshold"] == 0.0 + + +def test_the_wide_net_is_not_one_of_the_tunable_push_surfaces(): + """THE GUARD that keeps #4121's evidence clean (rule 167). + + The registry holds the PUSH arms — the ones with a floor and a budget the + model tunes. This source must not appear there: a pull folded into those + rows would move the near-miss distributions that step is arguing from, and + would offer a floor to tune on a tool whose whole point is not having one. + """ + from scribe.services.retrieval_surfaces import SURFACES + + assert wide_net.SOURCE not in SURFACES + + +def test_the_wide_net_is_not_ambient_either(): + """Ambient means a delivery nobody chose. This one is chosen by definition + — somebody called the tool — so counting it as ambient would make a + deliberate ask read as a bulk hand-over.""" + from scribe.services.note_usage import AMBIENT_SOURCES + + assert wide_net.SOURCE not in AMBIENT_SOURCES + + +@pytest.mark.asyncio +async def test_a_search_that_never_ran_is_not_reported_as_a_decline(): + """#3765: an empty query, a dead embedder and a failed query all return + nothing, and none of them is a ranker declining.""" + with _patched([], report={"searched": False, "best_available_score": None}), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("") + + assert rec.call_args.kwargs["searched"] is False + assert out["searched"] is False + + +@pytest.mark.asyncio +async def test_what_the_bar_turned_away_is_carried_through(): + """There is no bar here, but `best_available` still answers "did the corpus + have anything at all" for a call that came back empty (#3670).""" + with _patched([], report={"searched": True, "best_available_score": 0.31, + "best_available_id": 7}), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("q") + + assert rec.call_args.kwargs["best_available"] == 0.31 + assert rec.call_args.kwargs["best_available_id"] == 7 + + +# ── the contract a session actually reads ─────────────────────────────────── + +def test_the_docstring_says_when_to_reach_for_it(): + """The step's done-when, and the load-bearing half of this tool. + + A wide net nobody knows to call is worth nothing, so the docstring has to + name the MOMENT, not just the parameters — including the one that prompted + it, where a session hands work back rather than finishing it. + """ + doc = tool_doc("scribe.mcp.tools.wide_net", "what_might_apply").lower() + + assert "consequential" in doc + assert "handing work back" in doc + # Says the tail is expected to be noise — otherwise the first caller reads + # a low-scoring list as the tool being broken. + assert "noise" in doc + # And names its own limit: a pull only helps if somebody asks. + assert "only helps if you ask" in doc + + +def test_it_is_registered_and_readable_with_a_read_key(): + from scribe.mcp.server import _READ_ONLY_TOOLS + + mcp = FakeMCP() + wide_net.register(mcp) + assert mcp.names == ["what_might_apply"] + assert "what_might_apply" in _READ_ONLY_TOOLS + + +def test_the_instruction_surfaces_point_at_it(): + """Rule 119: the instruction surfaces ARE the specification for product + behaviour, so a tool the reflex never learns about is not shipped.""" + import pathlib + + from scribe.mcp import server + + assert "what_might_apply" in server._INSTRUCTIONS + + skill = (pathlib.Path(__file__).resolve().parents[1] + / "plugin" / "skills" / "using-scribe" / "SKILL.md").read_text() + assert "what_might_apply" in skill