CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 44s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m7s
CI & Build / Build & push image (push) Skipped
CI 7124 Python tests: 14 failed. One root cause behind ten of them, and the failure was the exact one `tests/helpers.writepath_cfg`'s docstring already warns about in prose — while being unable to prevent this instance of it. Three arms read their numbers out of that config dict inside a fail-open `except`. A missing key raises where nobody sees it, so the arm becomes a silent no-op, indistinguishable from the arm working and finding nothing. The helper derives its keys from `retrieval_surfaces.SURFACES` precisely to stop that — and `checkpoint_threshold` is deliberately NOT a surface, because everything in that table is a floor/budget pair belonging to one query and the checkpoint runs none. The derivation therefore could not see it, the write-path rule arm died before `record_retrieval`, and ten tests went red at once. Fixed at the helper, from the module constant, so there is still exactly one literal and it lives in the product. And the guard the docstring claimed now exists: `test_the_config_stand_in_carries_every_key_the_real_one_does` compares the stand-in's key set against the real `get_writepath_config`, so the next key that is not a surface fails loudly here instead of quietly disabling an arm under test. `test_retrieval_surfaces`'s hand-written key list gains it for the same reason, spelled out in place. The other four were the contract widening itself: `checkpoint` is present on every return of the tool arm, including its early ones, so four assertions comparing the whole dict needed it. That key is deliberately always present — the two arms feed one shell reader where an absent key and an empty one are read the same, so the difference is invisible exactly where it would bite. Verified statically: every cfg in the suite now routes through `writepath_cfg` (test_write_path_trigger's local `_cfg` delegates to it), no hand-written config dict survives, and the real config's eight keys match the stand-in's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
192 lines
8.4 KiB
Python
192 lines
8.4 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
|
|
|
|
missing = [
|
|
s.name for s in rs.SURFACES.values()
|
|
if f'source="{s.name}"' not in blob and s.name != SOURCE
|
|
]
|
|
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"
|