CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / integration (push) Successful in 50s
CI & Build / TypeScript typecheck (push) Successful in 57s
CI & Build / Python tests (push) Failing after 1m3s
CI & Build / Build & push image (push) Skipped
Groundwork for the step's real change. The operator's decision is that the floor is chosen and adjusted by the model using it, not shipped as a value somebody has to defend: "we need a model consistent surface for the adjustment of these floor values. 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 consistent across six arms that each spell their configuration differently, so the arms stop owning their numbers. `retrieval_surfaces.SURFACES` names each one, its floor key and default, its budget key and default, and — because they are rendered by the tuning tool and the Settings UI — what it asks, over what corpus, and how often it fires. A floor cannot be moved responsibly by anyone who does not know those three. Three things fall out: - **`k` becomes a real budget everywhere.** Only auto-inject had a configurable one; `RULEHINT_LIMIT`, `PROMPTRULE_LIMIT` and `reply_preferences.LIMIT` were constants. `k` is what binds under a low floor, so it has to be settable per surface — and per surface is the point, since `pre_tool_rule` fires before every Bash call while `prompt_rule` fires once a turn. - **`write_path` gets its own budget, inherited not reset.** It shared auto-inject's outright on the argument that "how many titles at once" means the same thing on both. It does not, for the same reason. Unset, it still reads auto-inject's key, so an install that tuned the shared knob does not silently drop to a new default. - **The duplicated read-and-clamp goes.** That shape is canon #2860 across 295 of 372 judged siblings. Survivable while the numbers were constants; not once they are meant to move. The long measurement comments stay exactly where they are — #2223's noise-floor probe, #3853's command-vs-code split, #3851's band measurement. The constants they annotate now alias the registry, so there is one value and the reasoning still sits beside it. Tests build the write-path config from the registry (`helpers.writepath_cfg`) instead of from hand-written dicts. That is not tidiness: the rule arms read their numbers inside a fail-open `except`, so a dict missing one key does not raise where a reader would see it — the arm silently becomes a no-op that reads exactly like "fired and found nothing". Ten hand-written dicts each looked complete on the day they were typed. tests/test_retrieval_surfaces.py pins the identity everything rests on: a surface's name IS its telemetry source. Nothing in the type system says so — `record_retrieval(source="pre_tool_rule")` is a literal in another file — and renaming one without the other yields an arm that can be tuned and not measured, or measured and not tuned, with no symptom either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
188 lines
8.1 KiB
Python
188 lines
8.1 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"):
|
|
assert key in cfg, f"{key} missing — its arm will silently no-op"
|