CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m45s
CI & Build / Build & push image (push) Successful in 30s
CI run 8137 had two failures, both source-scanning guards whose property moved to the pipeline. The property itself still held: - test_every_surface_name_is_a_real_telemetry_source looked for source="<name>" literals. The rule arms now record under their spec field, so the spec is the join key, read from RULE_ARMS. - test_every_hook_rule_search_says_which_project_it_is_for counted 4 direct searches in plugin_context. It now expects 0 there, so a copied arm fails the count. It also asserts that the pipeline has exactly one search and that its keyword literal carries project_id and never everywhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
199 lines
8.8 KiB
Python
199 lines
8.8 KiB
Python
"""The surface registry, and the identity the whole tuning story rests on (#4102).
|
|
|
|
WHY THIS EXISTS
|
|
|
|
Six arms used to own their own numbers: a settings key, a default, and a limit
|
|
that was usually a module constant nobody could change. That survived while the
|
|
values were shipped constants. It stops surviving once the operator's decision
|
|
is that the numbers MOVE:
|
|
|
|
"the floor should be chosen and adjusted by the model using it… the user
|
|
should be able to touch it but the model should be the thing handling it 9
|
|
times out of 10."
|
|
|
|
A tuning surface cannot be model-consistent across arms that each spell their
|
|
configuration differently, so the arms now read `{floor, budget}` from one
|
|
registry.
|
|
|
|
WHAT THIS PINS
|
|
|
|
1. **A surface's name IS its telemetry source.** This is the load-bearing
|
|
one. The tool that moves a floor and the table that reports what the floor
|
|
did have to be naming the same arm, and nothing in the type system says so
|
|
— `record_retrieval(source="pre_tool_rule")` is a string literal in a
|
|
different file. Renaming one without the other produces a surface that can
|
|
be tuned and cannot be measured, or measured and not tuned, and both fail
|
|
silently.
|
|
2. **Keys are unique.** Two surfaces sharing a settings key is how
|
|
`report_preference` spent its first release moving whenever the prompt arm
|
|
was tuned (#3860) — one dial wearing two labels.
|
|
3. **An unknown surface is refused.** Settings keys are free-form strings in
|
|
a generic table, so a typo'd name would write a key nothing reads: a
|
|
change that appears to succeed, reports a new value, and alters nothing.
|
|
4. **The budget clamps at 1, never 0.** A zero-budget arm searches, logs a
|
|
retrieval and renders nothing — indistinguishable in the telemetry from a
|
|
bar nothing cleared, which is the exact confusion this milestone exists to
|
|
remove. "Off" is what the `enabled` switch is for.
|
|
5. **`write_path` inherits auto-inject's budget when it has none.** It used
|
|
to share that key outright; giving it its own without a fallback would
|
|
silently reset the budget on every install that had tuned the shared one.
|
|
"""
|
|
from unittest.mock import AsyncMock, patch
|
|
|
|
import pytest
|
|
|
|
from scribe.services import retrieval_surfaces as rs
|
|
|
|
SERVICES = ("plugin_context", "reply_preferences")
|
|
|
|
|
|
def _service_source(name: str) -> str:
|
|
from pathlib import Path
|
|
root = Path(__file__).resolve().parents[1] / "src" / "scribe" / "services"
|
|
return (root / f"{name}.py").read_text()
|
|
|
|
|
|
def test_every_surface_name_is_a_real_telemetry_source():
|
|
"""The join key, checked against the arms that emit it.
|
|
|
|
Asserted on the source text rather than by calling the arms, because what
|
|
can rot here is the literal: a surface renamed in the registry and not in
|
|
the `record_retrieval(source=…)` call still runs, still logs, and still
|
|
tunes — just against two different names, so the readout an operator uses
|
|
to justify a change describes a different arm from the one the change hits.
|
|
"""
|
|
blob = "\n".join(_service_source(n) for n in SERVICES)
|
|
# `report_preference` passes its name through a module constant rather than
|
|
# a literal, so that one name is satisfied by the constant holding it.
|
|
from scribe.services.reply_preferences import SOURCE
|
|
|
|
# The rule arms record through the one pipeline (milestone 456), where
|
|
# `source` is the spec's own field — so for them the spec IS the string
|
|
# `record_retrieval` receives, and the join key is checked against it.
|
|
from scribe.services.retrieval_pipeline import RULE_ARMS
|
|
|
|
via_pipeline = {arm.source for arm in RULE_ARMS}
|
|
missing = [
|
|
s.name for s in rs.SURFACES.values()
|
|
if f'source="{s.name}"' not in blob
|
|
and s.name != SOURCE and s.name not in via_pipeline
|
|
]
|
|
assert not missing, (
|
|
f"these surfaces can be tuned but never measured: {missing}. The "
|
|
"registry name must match the string the arm passes to record_retrieval."
|
|
)
|
|
|
|
|
|
def test_no_two_surfaces_share_a_settings_key():
|
|
"""One dial, one label. #3860 is what the other way costs."""
|
|
floors = [s.floor_key for s in rs.SURFACES.values()]
|
|
budgets = [s.budget_key for s in rs.SURFACES.values()]
|
|
assert len(set(floors)) == len(floors), f"duplicate floor key in {floors}"
|
|
assert len(set(budgets)) == len(budgets), f"duplicate budget key in {budgets}"
|
|
assert not (set(floors) & set(budgets)), "a floor key doubles as a budget key"
|
|
|
|
|
|
def test_every_surface_says_what_it_asks_over_what_and_how_often():
|
|
"""The prose is rendered, not decorative.
|
|
|
|
A floor cannot be moved responsibly by anyone — model or human — who does
|
|
not know the query shape, the corpus, or how often the arm costs something.
|
|
An empty string here reaches the tuning tool and the Settings UI as a blank.
|
|
"""
|
|
for s in rs.SURFACES.values():
|
|
for field in ("asks", "over", "fires"):
|
|
assert getattr(s, field).strip(), f"{s.name}.{field} is empty"
|
|
|
|
|
|
def test_an_unknown_surface_is_refused_rather_than_written():
|
|
with pytest.raises(ValueError) as e:
|
|
rs.get_surface("pretool_rule") # a real typo for pre_tool_rule
|
|
# The message has to name the alternatives, or the caller's next move is a
|
|
# second guess.
|
|
assert "pre_tool_rule" in str(e.value)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.parametrize("stored, expected", [
|
|
("0.8", 0.8),
|
|
("5", 1.0), # clamped, not believed
|
|
("-3", 0.0),
|
|
("banana", 0.55), # unparseable falls back to the surface's default
|
|
("", 0.55),
|
|
])
|
|
async def test_a_floor_is_clamped_to_the_unit_interval(stored, expected):
|
|
with patch.object(rs, "get_setting", AsyncMock(return_value=stored)):
|
|
assert await rs.floor_for(1, "auto_inject") == expected
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.parametrize("stored, expected", [
|
|
("4", 4),
|
|
("999", rs.MAX_BUDGET),
|
|
("0", 1),
|
|
("-2", 1),
|
|
("banana", 3),
|
|
])
|
|
async def test_a_budget_is_clamped_with_a_floor_of_one(stored, expected):
|
|
"""Zero is the value that must not get through.
|
|
|
|
An arm with a budget of 0 runs its search, writes a `retrieval_logs` row
|
|
with `result_count == 0`, and renders nothing — which reads in the telemetry
|
|
exactly like a bar nothing cleared. Turning a surface off is the `enabled`
|
|
switch's job, and that one says so.
|
|
"""
|
|
with patch.object(rs, "get_setting", AsyncMock(return_value=stored)):
|
|
assert await rs.budget_for(1, "auto_inject") == expected
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_write_path_budget_falls_back_to_auto_injects():
|
|
"""The migration this fallback exists for.
|
|
|
|
`write_path` had no budget key of its own — it read auto-inject's. An
|
|
install that had tuned that shared knob to 6 must not silently drop to the
|
|
new key's default the day this ships; nothing would look broken and the
|
|
operator would never know to look.
|
|
"""
|
|
async def _setting(_uid, key, default=""):
|
|
return {"kb_writepath_top_k": "", "kb_autoinject_top_k": "6"}.get(key, default)
|
|
|
|
with patch.object(rs, "get_setting", AsyncMock(side_effect=_setting)):
|
|
assert await rs.budget_for(1, "write_path") == 6
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_its_own_budget_wins_once_it_is_set():
|
|
"""And the fallback must not become a permanent override."""
|
|
async def _setting(_uid, key, default=""):
|
|
return {"kb_writepath_top_k": "2", "kb_autoinject_top_k": "6"}.get(key, default)
|
|
|
|
with patch.object(rs, "get_setting", AsyncMock(side_effect=_setting)):
|
|
assert await rs.budget_for(1, "write_path") == 2
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_write_path_config_carries_every_arm_it_drives():
|
|
"""One hook request drives three arms, and each reads its pair out of this.
|
|
|
|
A missing key does not raise where a reader would see it — the rule arms
|
|
fail open, so a KeyError becomes an empty hint, and the arm reads as "fired
|
|
and found nothing". That is the failure this asserts against, and it is the
|
|
reason tests build the dict from the registry rather than by hand.
|
|
"""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
# Both modules read settings: `pc` for the enabled switch, `rs` for the
|
|
# pairs. Patching one and not the other reaches a real database.
|
|
with patch.object(pc, "get_setting", AsyncMock(return_value="")), \
|
|
patch.object(rs, "get_setting", AsyncMock(return_value="")):
|
|
cfg = await pc.get_writepath_config(1)
|
|
|
|
for key in ("threshold", "top_k", "rule_threshold", "rule_top_k",
|
|
"tool_rule_threshold", "tool_rule_top_k",
|
|
# Not a surface, and that is why it is spelled out here: it
|
|
# has no floor/budget pair to derive from, so nothing else in
|
|
# this file would notice it going missing (#4214).
|
|
"checkpoint_threshold"):
|
|
assert key in cfg, f"{key} missing — its arm will silently no-op"
|