diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 67457d7..e37bf57 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -126,9 +126,75 @@ WRITEPATH_DEFAULT_THRESHOLD = 0.68 # arrive unread; lower it if rules you needed never arrived. What would RETIRE # it: a cross-encoder rerank (#1038), which would make a similarity bar the # wrong control entirely. +# SCOPED TO THE WRITE-PATH ARM SINCE #3853. The command arm has its own bar +# below, and the measurement that separated them is recorded there. Everything +# above still holds for THIS arm: a code payload is long and rich, which is the +# case 0.72 was calibrated on, and the telemetry says it is working — the +# write-path rule arm speaks on 37% of its calls and its refused mass sits at +# p50 0.6989, comfortably under the bar rather than piled against it. RULEHINT_THRESHOLD_KEY = "kb_rulehint_threshold" RULEHINT_DEFAULT_THRESHOLD = 0.72 +# THE COMMAND ARM'S OWN BAR, AND WHY IT IS NOT THE WRITE PATH'S (#3853). +# +# One bar served both act arms until this. They are not the same problem: a +# write-path query is a code payload, long and rich, while a pre-tool query is +# a shell command — often under a dozen words. Less text, less signal, lower +# scores for the same relevance. At a shared 0.72 the two arms measured like +# different subsystems: +# +# write_path_rule 2,325 calls, speaks on 37%, near-miss p50 0.6989 +# pre_tool_rule 11,768 calls, speaks on 2%, near-miss p50 0.6794 +# +# The second is not a quiet surface, it is a mute one: 11,530 of 11,768 calls +# said nothing, with near-miss p90 at 0.7097 — refused mass piled one +# hundredth under the line, which is the shape a bar set too high leaves. The +# note arms are the control and look nothing like it (auto_inject refuses at +# p90 0.5463, write_path at 0.6738, both far below their bars). +# +# WHAT 0.68 IS MEASURED AGAINST. Eight replayed queries, consequential acts +# against innocuous ones, scored on the post-#3855 corpus: +# +# 0.7571 git push origin dev consequential +# 0.7245 cd ...; git fetch; git add -A consequential +# 0.7193 git pull --rebase origin dev consequential +# 0.6850 docker compose up -d consequential +# ---------------------------------------- 0.68 +# 0.6735 wc -l src/*.py && date innocuous +# 0.6544 grep -rn useState src/ innocuous +# 0.6099 sed -n '120,160p' package.json innocuous +# 0.6056 ls -la && cat README.md innocuous +# +# At 0.72 three of the four consequential acts retrieved NOTHING, including +# `git pull --rebase origin dev`, where rules 153, 1 and 2 all ranked +# correctly and all sat between 0.7126 and 0.7193. +# +# THE SEPARATION IS 0.0115 WIDE, and that is a caveat, not a result. Eight +# probes set a direction; they do not settle a number. `near_miss_samples` on +# a few days of post-#3855 traffic is what settles it, and this is the bar to +# re-read first. +# +# This also CORRECTS an assumption stated above. That comment argued 0.68 was +# "below where this corpus's noise sits", inferring a higher floor from the +# corpus being homogeneous. Measured, the command arm's noise ceiling is +# 0.6735 — so 0.68 clears it, barely, rather than sitting under it. The +# inference was reasonable and the measurement disagrees. +# +# WHY LOWERING IS SAFER NOW THAN IT WOULD HAVE BEEN. Until #3851 this arm had +# a single slot, so its one line had to be right and a high bar was the only +# control. The band now does noise control downstream: a marginal hit that +# clears the bar still has to score within `_RULEHINT_BAND` of the top to be +# rendered. The bar's job shrank, so the bar can. +# +# The noise floor above is set by CROSS-PROJECT BLEED rather than bad ranking +# — 0.6735 is another project's shell-command rule matching a shell command in +# this one, which is a correct match to a rule that should never have been +# eligible. Retrieval is ownership-scoped, not project-scoped. Scoping it +# would drop that ceiling and widen the 0.0115, which is the larger fix and +# the reason to settle project scoping before tuning this number twice. +TOOLRULE_THRESHOLD_KEY = "kb_toolrule_threshold" +TOOLRULE_DEFAULT_THRESHOLD = 0.68 + # A SET OF RULES PER ACT, NOT THE SINGLE BEST ONE (#3851). # # This was 1, and the reasoning for that is kept below rather than deleted @@ -1169,12 +1235,24 @@ async def get_writepath_config(user_id: int) -> dict: rule_threshold = RULEHINT_DEFAULT_THRESHOLD rule_threshold = min(1.0, max(0.0, rule_threshold)) + try: + tool_rule_threshold = float(await get_setting( + user_id, TOOLRULE_THRESHOLD_KEY, str(TOOLRULE_DEFAULT_THRESHOLD))) + except (TypeError, ValueError): + tool_rule_threshold = TOOLRULE_DEFAULT_THRESHOLD + tool_rule_threshold = min(1.0, max(0.0, tool_rule_threshold)) + return { **cfg, "enabled": enabled_raw.strip().lower() in ("true", "1", "yes", "on"), "threshold": threshold, # Its own bar, for a third corpus — see RULEHINT_DEFAULT_THRESHOLD. "rule_threshold": rule_threshold, + # And the COMMAND arm's own bar again, for the same reason one level + # down: a shell command is a different query shape from a code payload + # and scores lower for the same relevance (#3853). Separate keys, so an + # install can move one without the other — which is the whole finding. + "tool_rule_threshold": tool_rule_threshold, } def _rule_band(hits: list) -> list: @@ -1945,7 +2023,7 @@ async def build_tool_rule_hint( _rep_ptr: dict = {} hits = await semantic_search_rules( user_id, query, limit=RULEHINT_LIMIT, - threshold=cfg["rule_threshold"], + threshold=cfg["tool_rule_threshold"], report=_rep_ptr, ) duration_ms = (time.perf_counter() - t0) * 1000.0 @@ -1966,7 +2044,7 @@ async def build_tool_rule_hint( # failure the arm was built to stop. record_retrieval( user_id=user_id, source="pre_tool_rule", query=query, - threshold=cfg["rule_threshold"], limit=RULEHINT_LIMIT, + threshold=cfg["tool_rule_threshold"], limit=RULEHINT_LIMIT, project_id=project_id, is_task=None, results=fresh, duration_ms=duration_ms, best_available=_rep_ptr.get("best_available_score"), diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 981c480..c8b06b4 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -62,6 +62,12 @@ def _arm_patches(pc, hits, recorder, prior_art=None, cfg=None, rule_search=None, AsyncMock(return_value=cfg or { "enabled": True, "threshold": 0.6, "top_k": 3, "rule_threshold": 0.6, + # The command arm reads its OWN bar since #3853, and + # a stub missing this key does not fail where a + # reader would see it: the arm fails open, so the + # KeyError becomes an empty hint and every case in + # _ARMS reports the arm went silent instead. + "tool_rule_threshold": 0.6, })), patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), patch.object(pc, "semantic_search_notes", @@ -428,6 +434,12 @@ def _tool_patches(pc, hits, recorder, cfg=None, retrieval_log=None): AsyncMock(return_value=cfg or { "enabled": True, "threshold": 0.6, "top_k": 3, "rule_threshold": 0.6, + # The command arm reads its OWN bar since #3853, and + # a stub missing this key does not fail where a + # reader would see it: the arm fails open, so the + # KeyError becomes an empty hint and every case in + # _ARMS reports the arm went silent instead. + "tool_rule_threshold": 0.6, })), patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)), patch.object(pc, "record_retrieval", retrieval_log or MagicMock()), @@ -1581,3 +1593,70 @@ async def test_a_preference_on_the_ledger_keeps_the_slot_and_is_not_recounted(): "a repeat was written back to the hook's ledger, which would keep " "pushing its stamp forward so it never aged out (#3751)" ) + + +# ── each act arm uses its OWN bar, end to end (#3853) ─────────────────── +# +# The two act arms shared one threshold until the telemetry showed them +# behaving like different subsystems at the same number: write_path_rule +# speaking on 37% of 2,325 calls, pre_tool_rule on 2% of 11,768, because a +# code payload is long and rich where a shell command is short and carries +# less signal for the same relevance. +# +# Splitting the bar creates a failure the old single-bar code could not have: +# an arm can now search at one threshold and REPORT another. That row is what +# near-miss analysis is read against, so a mismatch does not look like a bug — +# it looks like a corpus whose scores sit somewhere they do not, and it would +# be acted on by moving the very bar it is misreporting. + +@pytest.mark.asyncio +async def test_each_act_arm_searches_at_its_own_bar(): + """The split, where it actually takes effect.""" + from scribe.services import plugin_context as pc + + cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, + "rule_threshold": 0.77, "tool_rule_threshold": 0.61} + + search = AsyncMock(return_value=list(_THREE_HITS)) + with ExitStack() as stack: + stack.enter_context(patch.object( + pc, "get_writepath_config", AsyncMock(return_value=cfg))) + stack.enter_context(patch.object(pc, "semantic_search_rules", search)) + stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) + stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) + await pc.build_tool_rule_hint(1, "Bash", "git push origin dev") + + assert search.await_args.kwargs["threshold"] == 0.61, ( + "the command arm searched at the write-path arm's bar; the two were " + "split at #3853 precisely because one number cannot serve both" + ) + + +@pytest.mark.asyncio +async def test_an_act_arm_reports_the_bar_it_actually_searched_at(): + """Search and log must agree, or the telemetry lies about the refusal. + + `retrieval_logs.threshold` is what `near_miss_samples` is read against. + An arm searching at 0.61 and logging 0.72 reports every hit between them + as having cleared a bar it never faced — and the reader's conclusion would + be to move the bar that was already right. + """ + from scribe.services import plugin_context as pc + + cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, + "rule_threshold": 0.77, "tool_rule_threshold": 0.61} + + search = AsyncMock(return_value=list(_THREE_HITS)) + log = MagicMock() + with ExitStack() as stack: + stack.enter_context(patch.object( + pc, "get_writepath_config", AsyncMock(return_value=cfg))) + stack.enter_context(patch.object(pc, "semantic_search_rules", search)) + stack.enter_context(patch.object(pc, "record_retrieval", log)) + stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) + await pc.build_tool_rule_hint(1, "Bash", "git push origin dev") + + rows = [c for c in log.call_args_list + if c.kwargs.get("source") == "pre_tool_rule"] + assert len(rows) == 1 + assert rows[0].kwargs["threshold"] == search.await_args.kwargs["threshold"] diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 264980e..d6dd323 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -481,6 +481,63 @@ async def test_the_two_write_path_bars_are_independent(): assert cfg["rule_threshold"] == 0.61 +@pytest.mark.asyncio +async def test_the_two_act_arms_read_independent_rule_bars(): + """#3853's split: the command arm's bar moves without the write path's. + + The two act arms shared one key until the telemetry showed them behaving + like different subsystems at the same number — the write-path arm speaking + on 37% of calls against the command arm's 2%, because a code payload is + long and rich where a shell command is short. A config assembler that + reads one key into both fields would silently undo that, and the symptom + would be invisible: both arms would simply agree again. + """ + 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))): + cfg = await pc.get_writepath_config(1) + + assert cfg["rule_threshold"] == 0.75 + assert cfg["tool_rule_threshold"] == 0.61 + + +@pytest.mark.asyncio +async def test_a_garbage_command_bar_falls_back_to_its_own_default(): + """Not to 0.0, and not to the write path's default. + + Falling back to 0.0 would attach a rule to every Bash call in the session; + falling back to the sibling's default would quietly re-merge the two bars + that #3853 separated, which is the harder failure to see because the arm + keeps working. + """ + 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))): + cfg = await pc.get_writepath_config(1) + + assert cfg["tool_rule_threshold"] == pc.TOOLRULE_DEFAULT_THRESHOLD + + +def test_the_command_bar_defaults_below_the_write_path_bar(): + """A DIRECTION check, like its sibling above, and for the same rule-115 + reason: the value is measured against one corpus, the relationship is not. + + A shell command carries less text than a code payload and therefore scores + lower for the same relevance — measured at #3853, where three of four + consequential commands retrieved nothing at the shared bar while the + write-path arm was healthy at it. Tuning either value stays free; inverting + the relationship would reinstate the mute arm that spoke on 2% of 11,768 + calls. + """ + from scribe.services import plugin_context as pc + + assert pc.TOOLRULE_DEFAULT_THRESHOLD < pc.RULEHINT_DEFAULT_THRESHOLD + + def test_the_rule_bar_defaults_above_the_code_bar(): """Not a number check — a DIRECTION check, and the only part of the default that is defensible without one instance's histogram (rule 115).