refactor(retrieval): one registry for every surface's floor and budget (#4102)
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
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
This commit is contained in:
@@ -6,6 +6,7 @@ source, and the two ways this must stay silent. Plus the plugin hook contract
|
||||
a PreToolUse hook that returns a permission decision would be able to block the
|
||||
operator's edit, which this feature must never do.
|
||||
"""
|
||||
import contextlib
|
||||
import json
|
||||
import re
|
||||
import subprocess
|
||||
@@ -13,7 +14,30 @@ from pathlib import Path
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from tests.helpers import fake_note, http_sink
|
||||
from tests.helpers import fake_note, http_sink, writepath_cfg
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def _stored_settings(stored):
|
||||
"""Patch BOTH readers, because one call now uses two (#4102).
|
||||
|
||||
`get_writepath_config` reads its `enabled` switch through
|
||||
`plugin_context.get_setting` and all six tunable numbers through
|
||||
`retrieval_surfaces.get_setting`. Patching only the first leaves the numbers
|
||||
talking to a real database — which in a unit job is a connection error, and
|
||||
in an integration job is worse: the test would pass or fail on whatever the
|
||||
instance happened to have stored.
|
||||
"""
|
||||
from scribe.services import plugin_context as pc
|
||||
from scribe.services import retrieval_surfaces as rs
|
||||
|
||||
def _side(uid, k, d=""):
|
||||
return stored.get(k, d)
|
||||
|
||||
with patch.object(pc, "get_setting", AsyncMock(side_effect=_side)), \
|
||||
patch.object(rs, "get_setting", AsyncMock(side_effect=_side)):
|
||||
yield
|
||||
|
||||
|
||||
PLUGIN = Path(__file__).resolve().parents[1] / "plugin"
|
||||
HOOK = PLUGIN / "hooks" / "scribe_prior_art.sh"
|
||||
@@ -31,9 +55,7 @@ def _cfg(**over):
|
||||
# missing key raises inside its fail-open except and turns the arm into a
|
||||
# silent no-op — which is indistinguishable from it working and finding
|
||||
# nothing.
|
||||
base = {"enabled": True, "threshold": 0.68, "top_k": 3, "rule_threshold": 0.72}
|
||||
base.update(over)
|
||||
return base
|
||||
return writepath_cfg(**over)
|
||||
|
||||
|
||||
# The semantic arm ignores payloads carrying less than WRITEPATH_MIN_CODE_CHARS
|
||||
@@ -423,11 +445,10 @@ async def test_sync_surfacing_is_measured_under_its_own_usage_source():
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_config_has_its_own_switch_and_threshold_but_shares_top_k():
|
||||
async def test_config_has_its_own_switch_threshold_and_inherited_budget():
|
||||
from scribe.services import plugin_context as pc
|
||||
stored = {pc.WRITEPATH_ENABLED_KEY: "false"}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
cfg = await pc.get_writepath_config(1)
|
||||
# Its own switch is off while auto-inject stays on...
|
||||
assert cfg["enabled"] is False
|
||||
@@ -436,8 +457,12 @@ async def test_config_has_its_own_switch_and_threshold_but_shares_top_k():
|
||||
# made unrelated code — including `x = 1` at 0.58 — clear the bar.
|
||||
assert cfg["threshold"] == pc.WRITEPATH_DEFAULT_THRESHOLD
|
||||
assert cfg["threshold"] > pc.AUTOINJECT_DEFAULT_THRESHOLD
|
||||
# ...and top_k is still shared: "how many titles at once" means the same
|
||||
# thing on both surfaces.
|
||||
# ...and the budget is INHERITED rather than shared (#4102). This arm has
|
||||
# its own key now, because it fires before every Write and Edit while
|
||||
# auto-inject fires once a turn, so the same number buys very different
|
||||
# amounts of attention. Unset, it still reads auto-inject's — which is what
|
||||
# stops the split from silently resetting an install that had tuned the
|
||||
# knob when it was shared.
|
||||
assert cfg["top_k"] == pc.AUTOINJECT_DEFAULT_TOP_K
|
||||
|
||||
|
||||
@@ -449,8 +474,7 @@ async def test_writepath_threshold_is_operator_tunable_and_clamped():
|
||||
|
||||
async def _cfg_with(raw):
|
||||
stored = {pc.WRITEPATH_THRESHOLD_KEY: raw}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
return await pc.get_writepath_config(1)
|
||||
|
||||
assert (await _cfg_with("0.9"))["threshold"] == 0.9
|
||||
@@ -474,8 +498,7 @@ async def test_the_rule_arm_has_its_own_tunable_bar():
|
||||
|
||||
async def _cfg_with(raw):
|
||||
stored = {pc.RULEHINT_THRESHOLD_KEY: raw}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
return await pc.get_writepath_config(1)
|
||||
|
||||
assert (await _cfg_with("0.8"))["rule_threshold"] == 0.8
|
||||
@@ -494,8 +517,7 @@ async def test_the_two_write_path_bars_are_independent():
|
||||
from scribe.services import plugin_context as pc
|
||||
|
||||
stored = {pc.WRITEPATH_THRESHOLD_KEY: "0.90", pc.RULEHINT_THRESHOLD_KEY: "0.61"}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
cfg = await pc.get_writepath_config(1)
|
||||
|
||||
assert cfg["threshold"] == 0.90
|
||||
@@ -516,8 +538,7 @@ async def test_the_two_act_arms_read_independent_rule_bars():
|
||||
from scribe.services import plugin_context as pc
|
||||
|
||||
stored = {pc.RULEHINT_THRESHOLD_KEY: "0.75", pc.TOOLRULE_THRESHOLD_KEY: "0.61"}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
cfg = await pc.get_writepath_config(1)
|
||||
|
||||
assert cfg["rule_threshold"] == 0.75
|
||||
@@ -536,8 +557,7 @@ async def test_a_garbage_command_bar_falls_back_to_its_own_default():
|
||||
from scribe.services import plugin_context as pc
|
||||
|
||||
stored = {pc.TOOLRULE_THRESHOLD_KEY: "banana"}
|
||||
with patch.object(pc, "get_setting",
|
||||
AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))):
|
||||
with _stored_settings(stored):
|
||||
cfg = await pc.get_writepath_config(1)
|
||||
|
||||
assert cfg["tool_rule_threshold"] == pc.TOOLRULE_DEFAULT_THRESHOLD
|
||||
|
||||
Reference in New Issue
Block a user