CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 1m1s
CI & Build / Python tests (push) Successful in 1m35s
CI & Build / Build & push image (push) Successful in 38s
The third rule arm, and the one the other two cannot reach. `write_path_rule` is keyed on code, `pre_tool_rule` on a command — both things the session is about to DO. A rule that governs what to SAY has no such trigger: extract intent from loose phrasing, raise a conflict before acting, hand off an action with its reason, end a finding with an offer all bind on a RESPONSE, and no tool call precedes one. The operator's message is the only query that exists before a response is composed. That hook searched notes alone, so no rule had ever been retrieved against a thing the operator actually said — and residency was the only surface those rules had, which is what milestone 394 removes. A SEPARATE FUNCTION, not a branch in build_autoinject_hint, because of its early returns. That arm bails when auto-inject is disabled, when the query is blank, when nothing clears the note bar — every one a statement about NOTES. Folded in, an operator who turned the awareness menu off would silently lose their rules, a coupling with no symptom since both look like a quiet hook. Two functions, two sets of gates, composed in the route. Guarded as "the rule arm never asks the notes arm's config", which is the structural fact. Joins _ARMS rather than getting its own test file. #3497's history is that the pre-tool arm inherited a defect from its sibling by being MODELLED on it instead of sharing with it, and a third arm modelled on two is two chances to repeat that. Repeat rendering, fresh-only counting, log-before-bailout, the kind register and the two-recorders identity are properties of every arm or of none. The bar is INHERITED and says so. 0.72 was tuned against code and commands; prose is a different query shape against the same documents, and triggers are written in the vocabulary of the moment — which for most rules is act vocabulary. Starting at the only number with evidence behind it and logging every call from the first deploy is what makes it settleable; guessing lower would put an unmeasured bar in front of a corpus that binds. k=3, anchored on this hook's own budget rather than the act arms'. RULEHINT_LIMIT is 1 because that arm fires before every Bash call; this one fires once per turn, beside a notes menu already spending three slots. And a prompt genuinely contains more than one act — "merge to main and then start on X" is two — where a command is one thing. `prompt_rule` added to RANKED_SOURCES: a ranker picked it, and a ranked source missing from that tuple is silently counted as bulk delivery and drops out of the pull-through denominator. The hook reads and writes the SHARED rule ledger under scribe-priorart, not a private one — one session keeps one list, aged (#3751), so a rule named here is not re-announced before the next Bash call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
1338 lines
60 KiB
Python
1338 lines
60 KiB
Python
"""Both ends of the rule-usage loop are actually wired (milestone 333 step 2).
|
|
|
|
Step 1 built the table and the service. A counter nobody calls reads zero and
|
|
looks exactly like a surface nobody uses — which is #2663's shape and the whole
|
|
reason this milestone exists. So this file is about the CALL SITES, not the
|
|
storage.
|
|
|
|
Cross-cutting on purpose: the surfaced end lives in `plugin_context`, the pull
|
|
end in two different doors, and the property under test is that they meet. Split
|
|
across three module-shaped files, "both ends are wired" is a thing no single
|
|
test asserts.
|
|
"""
|
|
import ast
|
|
from contextlib import ExitStack
|
|
from pathlib import Path
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from tests.helpers import fake_note, fake_rule
|
|
|
|
# The MCP tool layer reads its caller from a ContextVar the HTTP transport sets
|
|
# per request; a unit test has no request, so it binds the caller itself. The
|
|
# arm tests do not need it — build_write_path_hint takes user_id directly — but
|
|
# the module-level mark is how every tool-layer test file in this repo opts in.
|
|
pytestmark = pytest.mark.usefixtures("_bind_user")
|
|
|
|
|
|
# ── The surfaced end ───────────────────────────────────────────────────
|
|
#
|
|
# conftest's autouse `_no_rule_arm` stubs `semantic_search_rules` so unrelated
|
|
# plugin-context tests don't pull a real embedding model through this arm. Its
|
|
# docstring says a test that wants the arm live can re-patch it — that is what
|
|
# each of these does.
|
|
|
|
|
|
# The write-path hint returns early when a write matched nothing at all — no
|
|
# staleness, no synced record, no prior-art menu, no shape signal. The rule arm
|
|
# sits deliberately on the FAR side of that guard, because it runs a semantic
|
|
# search and moving it above would mean an embedding query on every write in
|
|
# the session (#3311's closing note, and the reason its gating is a separate
|
|
# question from precision).
|
|
#
|
|
# So a fixture that stubs every other arm to empty never reaches the rule arm
|
|
# at all — which is what the first run of this file did. The note hit below is
|
|
# not decoration: it is the condition the arm requires in order to fire.
|
|
_PRIOR_ART = [(0.72, fake_note(id=9, title="debounce helper", user_id=1,
|
|
note_type="snippet"))]
|
|
|
|
|
|
def _arm_patches(pc, hits, recorder, prior_art=None, cfg=None, rule_search=None,
|
|
retrieval_log=None):
|
|
"""The minimum stubbing that lets the rule arm run and nothing else.
|
|
|
|
`cfg`, `rule_search` and `retrieval_log` are overridable so a caller can
|
|
inspect what the arm ASKED for, and what it told the CALL log, rather than
|
|
only what it did with the answer — patching them a second time on top would
|
|
work, but reads as an accident.
|
|
"""
|
|
return (
|
|
patch.object(pc, "get_writepath_config",
|
|
AsyncMock(return_value=cfg or {
|
|
"enabled": True, "threshold": 0.6,
|
|
"top_k": 3, "rule_threshold": 0.6,
|
|
})),
|
|
patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))),
|
|
patch.object(pc, "semantic_search_notes",
|
|
AsyncMock(return_value=_PRIOR_ART if prior_art is None
|
|
else prior_art)),
|
|
patch.object(pc, "semantic_search_rules",
|
|
rule_search or AsyncMock(return_value=hits)),
|
|
patch.object(pc, "record_retrieval", retrieval_log or MagicMock()),
|
|
patch.object(pc, "record_surfaced", MagicMock()),
|
|
patch.object(pc, "record_rule_surfaced", recorder),
|
|
patch.object(pc, "owner_names_for", AsyncMock(return_value={})),
|
|
patch.object(pc, "concept_query", MagicMock(return_value="a deadline on a fetch")),
|
|
)
|
|
|
|
|
|
async def _run_arm(hits, recorder, prior_art=None, retrieval_log=None, **kwargs):
|
|
from scribe.services import plugin_context as pc
|
|
with ExitStack() as stack:
|
|
for ctx in _arm_patches(pc, hits, recorder, prior_art,
|
|
retrieval_log=retrieval_log):
|
|
stack.enter_context(ctx)
|
|
return await pc.build_write_path_hint(
|
|
1, "frontend/src/api/client.ts", code="x" * 400, **kwargs
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_arm_records_what_it_showed():
|
|
"""The claim being measured. Without this call the arm keeps producing
|
|
scores in retrieval_logs and no evidence that any hint was ever read."""
|
|
rec = MagicMock()
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug"))]
|
|
await _run_arm(hits, rec)
|
|
|
|
assert rec.call_count == 1
|
|
kw = rec.call_args.kwargs
|
|
assert kw["rule_ids"] == [156]
|
|
assert kw["source"] == "write_path_rule"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_rule_the_session_already_holds_is_not_counted_as_surfaced():
|
|
"""`exclude_rule_ids` drops what the session already has, and the recorded
|
|
set must be what was SHOWN, not what was considered.
|
|
|
|
Counting the excluded ones would inflate the denominator with claims the
|
|
agent never saw — the ratio would fall for a reason that has nothing to do
|
|
with whether the hints landed, which is precisely the misreading this
|
|
milestone exists to prevent.
|
|
"""
|
|
rec = MagicMock()
|
|
hits = [
|
|
(0.71, fake_rule(id=156, title="A wait with no deadline is a bug")),
|
|
(0.70, fake_rule(id=157, title="A loop re-arms in a finally")),
|
|
]
|
|
await _run_arm(hits, rec, exclude_rule_ids=[157])
|
|
|
|
assert rec.call_args.kwargs["rule_ids"] == [156]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_nothing_is_recorded_when_every_hit_was_already_held():
|
|
"""No surfacing happened, so no surfacing is recorded. A zero-row batch
|
|
would still be a call, and a call that says "we showed nothing" pollutes
|
|
the count of times the arm spoke."""
|
|
rec = MagicMock()
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug"))]
|
|
await _run_arm(hits, rec, exclude_rule_ids=[156])
|
|
|
|
assert rec.call_count == 0
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_arm_searches_on_its_OWN_bar_not_the_code_one():
|
|
"""The consuming half of step 4. `get_writepath_config` assembling a
|
|
separate `rule_threshold` means nothing if the arm still passes
|
|
`cfg["threshold"]` to its search — the split would exist in the config and
|
|
not in the behaviour, and #3311 would be exactly where it was.
|
|
|
|
The two values are deliberately different here so the assertion can tell
|
|
them apart.
|
|
"""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
search = AsyncMock(return_value=[])
|
|
with ExitStack() as stack:
|
|
for ctx in _arm_patches(
|
|
pc, [], MagicMock(), rule_search=search,
|
|
cfg={"enabled": True, "threshold": 0.60,
|
|
"top_k": 3, "rule_threshold": 0.81},
|
|
):
|
|
stack.enter_context(ctx)
|
|
await pc.build_write_path_hint(
|
|
1, "frontend/src/api/client.ts", code="x" * 400,
|
|
)
|
|
|
|
kw = search.await_args.kwargs
|
|
assert kw["threshold"] == 0.81, "the arm is still using the code threshold"
|
|
assert kw["limit"] == pc.RULEHINT_LIMIT
|
|
# NO tier filter (#3702). The arms search every rule the caller owns,
|
|
# because "already in the session" is not the same as "in front of the
|
|
# reader at the moment it applies" — and relevance is the threshold's
|
|
# job, not a category's. If this assertion is failing because a tier
|
|
# argument came back, read the block above RULEHINT_LIMIT first: the
|
|
# filter may legitimately return, but only carrying a measured reason.
|
|
assert "tier" not in kw or kw["tier"] is None, (
|
|
"the arm is filtering the rule corpus by tier again"
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_arm_does_not_fire_on_a_write_that_matched_nothing():
|
|
"""The gate, pinned — because the fixture above now depends on it and a
|
|
silent change would make every other test here pass vacuously.
|
|
|
|
A write matching no prior art returns before the rule arm runs. That is
|
|
deliberate: the arm is a semantic search, and ungating it means an
|
|
embedding query on every write in the session. #3311 is explicit that the
|
|
gate stays until the arm's precision is fixed, so this failing is a signal
|
|
to go read that issue rather than to update the assertion.
|
|
"""
|
|
rec = MagicMock()
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug"))]
|
|
await _run_arm(hits, rec, prior_art=[])
|
|
|
|
assert rec.call_count == 0
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_failing_recorder_does_not_break_the_write():
|
|
"""Telemetry must never take down the surface it observes. The arm is
|
|
already wrapped in a fail-open try/except; this pins that the new call is
|
|
INSIDE it rather than after."""
|
|
rec = MagicMock(side_effect=RuntimeError("telemetry is down"))
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug"))]
|
|
out = await _run_arm(hits, rec)
|
|
|
|
assert "context" in out
|
|
|
|
|
|
# ── The pull end ───────────────────────────────────────────────────────
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_mcp_get_rule_records_an_agent_pull():
|
|
"""THE pull that matters: the arm's own message ends "Read it with
|
|
get_rule(N)", so this is the exact action a landed hint produces."""
|
|
rec = MagicMock()
|
|
rule = fake_rule(id=156, title="A wait with no deadline is a bug")
|
|
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule",
|
|
AsyncMock(return_value=rule)), \
|
|
patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail",
|
|
AsyncMock(return_value={"id": 156})), \
|
|
patch("scribe.mcp.tools.rulebooks.record_rule_pulled", rec):
|
|
from scribe.mcp.tools.rulebooks import get_rule
|
|
await get_rule(rule_id=156)
|
|
|
|
assert rec.call_args.kwargs["rule_id"] == 156
|
|
assert rec.call_args.kwargs["source"] == "mcp_get_rule"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_rule_that_cannot_be_read_is_not_a_pull():
|
|
"""Recorded after the access check. A refused read is not a pull, and
|
|
counting it would credit the arm for a hint nobody could open."""
|
|
rec = MagicMock()
|
|
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule",
|
|
AsyncMock(return_value=None)), \
|
|
patch("scribe.mcp.tools.rulebooks.record_rule_pulled", rec):
|
|
from scribe.mcp.tools.rulebooks import get_rule
|
|
with pytest.raises(ValueError):
|
|
await get_rule(rule_id=156)
|
|
|
|
assert rec.call_count == 0
|
|
|
|
|
|
# ── Completeness: every door, and only the doors ───────────────────────
|
|
|
|
|
|
def _source_of(module_path: str) -> str:
|
|
return (Path(__file__).resolve().parents[1] / module_path).read_text()
|
|
|
|
|
|
def test_every_rule_detail_door_records_a_pull():
|
|
"""The task's own warning, made mechanical: miss a door and the ratio
|
|
reads low for a reason that is not about the rules.
|
|
|
|
Source inspection rather than behaviour, because the REST door has no
|
|
live-HTTP harness in the unit lane (see test_routes_rulebooks.py's own
|
|
note). What it can still prove is that the handler names the recorder —
|
|
which is the thing that gets forgotten when a door is added.
|
|
"""
|
|
rest = _source_of("src/scribe/routes/rulebooks.py")
|
|
mcp = _source_of("src/scribe/mcp/tools/rulebooks.py")
|
|
assert 'source="rest_rule"' in rest, (
|
|
"the REST rule-detail route does not record a pull"
|
|
)
|
|
assert 'source="mcp_get_rule"' in mcp, (
|
|
"the MCP get_rule tool does not record a pull"
|
|
)
|
|
|
|
|
|
def test_the_bulk_loaders_are_not_counted_as_pulls():
|
|
"""`list_always_on_rules` and `enter_project` hand over every applicable
|
|
rule at once. That is delivery, not somebody choosing to open one record,
|
|
and counting it would swamp the signal with exactly the ambient surfacing
|
|
the ratio exists to distinguish from.
|
|
|
|
Stated as a test because it is the tempting addition: both put rules in
|
|
front of an agent, so "surely those are pulls too" is the reading someone
|
|
arrives at without the argument.
|
|
"""
|
|
for path in ("src/scribe/mcp/tools/rulebooks.py",
|
|
"src/scribe/mcp/tools/projects.py"):
|
|
src = _source_of(path)
|
|
for door in ("list_always_on_rules", "enter_project"):
|
|
if f"async def {door}" not in src:
|
|
continue
|
|
body = src.split(f"async def {door}", 1)[1].split("\nasync def ", 1)[0]
|
|
assert "record_rule_pulled" not in body, (
|
|
f"{door} records a pull. It is a bulk resident load — every "
|
|
"applicable rule at once — so counting it would drown the "
|
|
"surfaced:pulled ratio in ambient delivery."
|
|
)
|
|
|
|
|
|
# ── The AMBIENT end: bulk deliveries (#3473) ───────────────────────────
|
|
#
|
|
# The preload was the largest rule surface in the product and emitted nothing,
|
|
# so its cost was certain and its usefulness unfalsifiable. These assert the
|
|
# three delivery shapes now emit — and, just as importantly, that the two
|
|
# lookalike call sites which show nobody anything do NOT.
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_session_start_preload_records_what_it_delivered():
|
|
"""The block every session opens with. Chosen by nobody, paid for every
|
|
turn — and until it emitted, invisible to the scoreboard that judges every
|
|
other surface."""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
rec = MagicMock()
|
|
rules = [fake_rule(id=1, title="`dev` is home"),
|
|
fake_rule(id=2, title="`main` — never without explicit request")]
|
|
with ExitStack() as stack:
|
|
stack.enter_context(
|
|
patch.object(pc.rulebooks_svc, "list_always_on_rules",
|
|
AsyncMock(return_value=rules))
|
|
)
|
|
stack.enter_context(
|
|
patch.object(pc.rulebooks_svc, "excluded_always_on_rulebooks",
|
|
AsyncMock(return_value=[]))
|
|
)
|
|
stack.enter_context(patch.object(pc, "record_rule_surfaced", rec))
|
|
stack.enter_context(
|
|
patch.object(pc, "_topic_titles", AsyncMock(return_value={}))
|
|
)
|
|
await pc.build_session_context(1, project_id=0)
|
|
|
|
assert rec.call_count == 1, "the preload recorded nothing"
|
|
kw = rec.call_args.kwargs
|
|
assert kw["rule_ids"] == [1, 2]
|
|
assert kw["source"] == "session_start"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_always_on_tool_records_what_it_handed_over():
|
|
from scribe.mcp.tools import rulebooks as tools
|
|
|
|
rec = MagicMock()
|
|
rules = [fake_rule(id=3, title="No GitHub — Fabled-Git only")]
|
|
with ExitStack() as stack:
|
|
stack.enter_context(
|
|
patch.object(tools.rulebooks_svc, "list_always_on_rules",
|
|
AsyncMock(return_value=rules))
|
|
)
|
|
stack.enter_context(
|
|
patch.object(tools.rulebooks_svc, "rules_etag",
|
|
MagicMock(return_value="etag"))
|
|
)
|
|
stack.enter_context(patch.object(tools, "record_rule_surfaced", rec))
|
|
await tools.list_always_on_rules()
|
|
|
|
assert rec.call_args.kwargs["rule_ids"] == [3]
|
|
assert rec.call_args.kwargs["source"] == "list_always_on_rules"
|
|
|
|
|
|
def test_rules_payload_records_both_the_family_and_project_halves():
|
|
"""One emit site for all five `rules_payload` surfaces.
|
|
|
|
Per-caller emission would be five sites to remember, and #3430 gap 2 is
|
|
what that costs: the process→skill sync went un-emitted through an entire
|
|
dedicated telemetry survey because nothing forced its surface to be
|
|
accounted for.
|
|
"""
|
|
from scribe.services import rulebooks as svc
|
|
|
|
rec = MagicMock()
|
|
with patch.object(svc, "record_rule_surfaced", rec):
|
|
svc.rules_payload(
|
|
{
|
|
"rules": [{"id": 10}, {"id": 11}],
|
|
"project_rules": [{"id": 12}],
|
|
"truncated": False,
|
|
"subscribed_rulebooks": [],
|
|
},
|
|
user_id=1,
|
|
source="enter_project",
|
|
)
|
|
|
|
kw = rec.call_args.kwargs
|
|
assert kw["rule_ids"] == [10, 11, 12], "project-scoped rules were delivered too"
|
|
assert kw["source"] == "enter_project"
|
|
|
|
|
|
def test_every_rules_payload_caller_names_itself():
|
|
"""`source` is the CALLER's name, so the readout can still separate the
|
|
session handshake from a mid-session milestone read. A shared constant here
|
|
would collapse five distinguishable surfaces into one."""
|
|
import re
|
|
|
|
seen = set()
|
|
for path in Path("src/scribe").rglob("*.py"):
|
|
for m in re.finditer(r"rules_payload\([^)]*source=\"([a-z_]+)\"", path.read_text()):
|
|
seen.add(m.group(1))
|
|
assert seen == {
|
|
"enter_project", "get_project", "get_milestone",
|
|
"start_planning", "get_task",
|
|
}, f"a rules_payload caller is missing or misnamed: {sorted(seen)}"
|
|
|
|
|
|
def test_the_marker_paths_stay_silent():
|
|
"""The two call sites that read the rules and show NOBODY anything.
|
|
|
|
`rules_etag_for` and the write-path staleness arm both call
|
|
`list_always_on_rules` to build or compare a marker. Emitting there would
|
|
put rules in the denominator that no agent ever saw — the exact inflation
|
|
`record_rule_surfaced`'s docstring forbids, arriving from the one direction
|
|
nothing else guards.
|
|
"""
|
|
svc_src = Path("src/scribe/services/rulebooks.py").read_text()
|
|
etag_fn = svc_src.split("async def rules_etag_for")[1].split("\ndef ")[0]
|
|
assert "record_rule_surfaced" not in etag_fn, (
|
|
"rules_etag_for emits a surfacing — it builds a marker, it shows nothing"
|
|
)
|
|
|
|
pc_src = Path("src/scribe/services/plugin_context.py").read_text()
|
|
staleness = pc_src.split("if rules_etag:")[1].split("# The guard sits BELOW")[0]
|
|
assert "record_rule_surfaced" not in staleness, (
|
|
"the staleness arm emits a surfacing — it compares a marker, it shows nothing"
|
|
)
|
|
|
|
|
|
# ── The PRE-TOOL arm: rules keyed on the action (#3476) ────────────────
|
|
#
|
|
# The write-path arm can only be reached by a code write, so every rule about
|
|
# which tool to reach for was unretrievable at the moment it mattered — which
|
|
# is why they all had to be resident. These cover the surface that changes it.
|
|
|
|
|
|
def _tool_patches(pc, hits, recorder, cfg=None, retrieval_log=None):
|
|
return (
|
|
patch.object(pc, "get_writepath_config",
|
|
AsyncMock(return_value=cfg or {
|
|
"enabled": True, "threshold": 0.6,
|
|
"top_k": 3, "rule_threshold": 0.6,
|
|
})),
|
|
patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)),
|
|
patch.object(pc, "record_retrieval", retrieval_log or MagicMock()),
|
|
patch.object(pc, "record_rule_surfaced", recorder),
|
|
)
|
|
|
|
|
|
async def _run_tool_arm(hits, recorder, command="curl -s https://git.example/api/v1/runs",
|
|
tool="Bash", retrieval_log=None, **kwargs):
|
|
from scribe.services import plugin_context as pc
|
|
with ExitStack() as stack:
|
|
for ctx in _tool_patches(pc, hits, recorder, retrieval_log=retrieval_log):
|
|
stack.enter_context(ctx)
|
|
return await pc.build_tool_rule_hint(1, tool, command, **kwargs)
|
|
|
|
|
|
# ── the prompt-boundary arm (#3852) ─────────────────────────────────────
|
|
#
|
|
# The third arm, and it joins _ARMS rather than getting a test file of its
|
|
# own. That is the point of the shared parametrisation: #3497's history is
|
|
# that the pre-tool arm inherited a defect from its sibling by being MODELLED
|
|
# on it instead of sharing with it, and a third arm modelled on two is two
|
|
# chances to repeat that. Everything in the family — repeat rendering,
|
|
# fresh-only counting, the log-before-bailout order, the kind register, the
|
|
# two recorders reading one list — is a property of every arm or of none.
|
|
#
|
|
# Fewer patches than its siblings because it does less: no prior-art menu, no
|
|
# config object, no concept query. Just a bar, a search, and two recorders.
|
|
|
|
|
|
def _prompt_patches(pc, hits, recorder, retrieval_log=None):
|
|
return (
|
|
# The arm reads its own threshold key rather than a shared config
|
|
# object — a third corpus with a bar nothing has yet tuned for it.
|
|
patch.object(pc, "get_setting", AsyncMock(return_value="0.6")),
|
|
patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)),
|
|
patch.object(pc, "record_retrieval", retrieval_log or MagicMock()),
|
|
patch.object(pc, "record_rule_surfaced", recorder),
|
|
)
|
|
|
|
|
|
async def _run_prompt_arm(hits, recorder, prompt="please merge to main",
|
|
retrieval_log=None, **kwargs):
|
|
from scribe.services import plugin_context as pc
|
|
with ExitStack() as stack:
|
|
for ctx in _prompt_patches(pc, hits, recorder, retrieval_log=retrieval_log):
|
|
stack.enter_context(ctx)
|
|
return await pc.build_prompt_rule_hint(1, prompt, **kwargs)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_prompt_arm_retrieves_against_what_the_operator_SAID():
|
|
"""The gap this arm closes, stated as the thing that had no trigger.
|
|
|
|
Both other arms are keyed on an act — a file write, a command. A rule that
|
|
governs what to SAY has no act in front of it: extract intent from loose
|
|
phrasing, raise a conflict before acting, end a finding with an offer all
|
|
bind on a response. Before this, the operator's message reached only
|
|
`semantic_search_notes`, so no rule had ever been retrieved against a
|
|
thing the operator actually said.
|
|
"""
|
|
rec = MagicMock()
|
|
search = AsyncMock(return_value=[(0.79, fake_rule(
|
|
id=2, title="`main` — never without explicit request",
|
|
when_to_apply="opening or merging a dev→main pull request",
|
|
))])
|
|
from scribe.services import plugin_context as pc
|
|
with ExitStack() as stack:
|
|
stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6")))
|
|
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", rec))
|
|
out = await pc.build_prompt_rule_hint(1, "please merge to main")
|
|
|
|
# The PROMPT is the query — not a path, not a command.
|
|
assert search.call_args.args[1] == "please merge to main"
|
|
assert "get_rule(2)" in out["context"]
|
|
assert rec.call_args.kwargs["source"] == "prompt_rule"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_prompt_arm_addresses_the_request_not_a_tool_call():
|
|
"""`where` has to name the moment, and this arm's moment is the asking.
|
|
|
|
"may apply to this Bash call" would be a lie here — there is no Bash call,
|
|
which is the entire reason the arm exists.
|
|
"""
|
|
out = await _run_prompt_arm(
|
|
[(0.79, fake_rule(id=77, title="Extract intent from loose phrasing"))],
|
|
MagicMock(),
|
|
)
|
|
assert "may apply to this request" in out["context"], out["context"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_prompt_arm_says_nothing_when_asked_nothing():
|
|
"""A blank prompt is not a query, and searching on one would put a row in
|
|
retrieval_logs that no operator action produced."""
|
|
search = AsyncMock(return_value=[])
|
|
log = MagicMock()
|
|
from scribe.services import plugin_context as pc
|
|
with ExitStack() as stack:
|
|
stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6")))
|
|
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()))
|
|
out = await pc.build_prompt_rule_hint(1, " ")
|
|
|
|
assert out == {"context": "", "rule_ids": []}
|
|
search.assert_not_called()
|
|
log.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_prompt_arm_logs_the_calls_that_found_nothing():
|
|
"""#3497's defect, pinned on the arm that most needs it.
|
|
|
|
This bar is INHERITED from the act arms and unverified against prose. The
|
|
zero rows are therefore the whole evidence base for whether 0.72 belongs
|
|
here at all — an arm that logged only the calls it liked would report a
|
|
flawless clear-rate however wrong the number is.
|
|
"""
|
|
log = MagicMock()
|
|
out = await _run_prompt_arm([], MagicMock(), retrieval_log=log)
|
|
|
|
assert out["context"] == ""
|
|
assert log.call_count == 1, (
|
|
"the prompt arm returned early without logging a call that found "
|
|
"nothing — the only evidence its inherited threshold is too high"
|
|
)
|
|
assert log.call_args.kwargs["source"] == "prompt_rule"
|
|
assert log.call_args.kwargs["results"] == []
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_turning_off_the_notes_menu_does_not_turn_off_rules():
|
|
"""Why this is a separate function and not a branch in the notes arm.
|
|
|
|
`build_autoinject_hint` returns early when auto-inject is disabled, when
|
|
the query is blank, and when nothing clears the note bar. Every one of
|
|
those is a statement about NOTES. Folded together, an operator who turned
|
|
the awareness menu off would silently stop receiving RULES — a coupling
|
|
with no symptom, since both failure modes look like a quiet hook.
|
|
|
|
Pinned as "the rule arm never asks the notes arm's config", which is the
|
|
structural fact rather than a simulation of the setting. A future refactor
|
|
that reaches for that config here fails, whatever it then does with it.
|
|
"""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
notes_cfg = AsyncMock(return_value={
|
|
"enabled": False, "threshold": 0.55, "top_k": 3,
|
|
})
|
|
with ExitStack() as stack:
|
|
stack.enter_context(patch.object(pc, "get_autoinject_config", notes_cfg))
|
|
for ctx in _prompt_patches(
|
|
pc, [(0.79, fake_rule(id=2, title="`main` — never without explicit request"))],
|
|
MagicMock(),
|
|
):
|
|
stack.enter_context(ctx)
|
|
out = await pc.build_prompt_rule_hint(1, "please merge to main")
|
|
|
|
assert "get_rule(2)" in out["context"], (
|
|
"the rule arm went quiet while the notes menu was disabled — the two "
|
|
"are different claims with different costs of being missed, and one "
|
|
"operator setting should not silence both"
|
|
)
|
|
notes_cfg.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_names_a_rule_for_the_command_about_to_run():
|
|
"""The 2026-09-03 incident in one test: reaching for curl against the forge
|
|
API is a Bash call, and nothing watched Bash."""
|
|
rec = MagicMock()
|
|
hits = [(0.71, fake_rule(id=161,
|
|
title="Reach the forge through its MCP tools, never curl",
|
|
when_to_apply="whenever you need CI status"))]
|
|
out = await _run_tool_arm(hits, rec)
|
|
|
|
assert out["rule_ids"] == [161]
|
|
assert "Reach the forge through its MCP tools" in out["context"]
|
|
assert "get_rule(161)" in out["context"], "the hint must hand over the way to read it"
|
|
assert "Bash" in out["context"], "the hint names the tool it is about"
|
|
assert rec.call_args.kwargs["source"] == "pre_tool_rule"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_is_a_ranked_source():
|
|
"""It CHOSE what it showed, so a pull can settle whether the choice was any
|
|
good — unlike a preload, which chose nothing. If this drifts into the
|
|
ambient class the arm becomes unjudgeable, which is the state #3311
|
|
described and M333 existed to end."""
|
|
from scribe.services.rule_usage import is_ambient
|
|
|
|
assert not is_ambient("pre_tool_rule")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_rule_the_session_already_holds_is_referenced_not_re_offered():
|
|
"""WAS `..._is_not_re_offered`, asserting `"161" not in out["context"]`.
|
|
|
|
That assertion was the old contract and #3750 deliberately reverses half of
|
|
it: a rule already on the ledger now gets a line with a different tail
|
|
instead of being dropped in silence. Withholding was only ever right while
|
|
the session still HELD what it was told, and a compaction breaks exactly
|
|
that while leaving the id excluded.
|
|
|
|
The half that survives is the half that was always about telemetry rather
|
|
than rendering: an already-held rule stays out of `rule_ids`, so it reaches
|
|
neither the exclusion ledger (where it already is) nor the surfacing count
|
|
(which a reference must not inflate).
|
|
"""
|
|
rec = MagicMock()
|
|
hits = [(0.71, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
|
(0.70, fake_rule(id=12, title="Don't run a local stack unless asked"))]
|
|
out = await _run_tool_arm(hits, rec, exclude_rule_ids=[161])
|
|
|
|
assert out["rule_ids"] == [12], (
|
|
"a referenced rule was counted as surfaced; only the fresh one was "
|
|
"actually chosen by this arm"
|
|
)
|
|
assert "get_rule(161)" in out["context"], (
|
|
"the held rule vanished instead of being referenced (#3750)"
|
|
)
|
|
assert "get_rule(12)" in out["context"], (
|
|
"the fresh rule was lost while adding the reference — both belong in "
|
|
"the hint, and the reference must not displace the surfacing"
|
|
)
|
|
assert _SEEN_TAIL in out["context"] and _FRESH_TAIL in out["context"], (
|
|
"one call rendered two hits in different states and gave them the same "
|
|
"tail; the tails are the entire difference a reader can act on"
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_empty_command_asks_the_ranker_nothing():
|
|
"""Every Bash call reaches this. A blank payload must cost no embedding
|
|
query at all, not merely return nothing after paying for one."""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
search = AsyncMock(return_value=[])
|
|
rec = MagicMock()
|
|
with ExitStack() as stack:
|
|
stack.enter_context(patch.object(pc, "semantic_search_rules", search))
|
|
stack.enter_context(patch.object(pc, "record_rule_surfaced", rec))
|
|
out = await pc.build_tool_rule_hint(1, "Bash", " ")
|
|
|
|
assert out == {"context": "", "rule_ids": []}
|
|
search.assert_not_called()
|
|
rec.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_fails_open():
|
|
"""A recall aid may never break the operator's action. A ranker that raises
|
|
must cost the hint, not the command."""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
with ExitStack() as stack:
|
|
stack.enter_context(patch.object(pc, "get_writepath_config",
|
|
AsyncMock(side_effect=RuntimeError("boom"))))
|
|
out = await pc.build_tool_rule_hint(1, "Bash", "docker compose up -d")
|
|
|
|
assert out == {"context": "", "rule_ids": []}
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_long_command_is_bounded_before_it_reaches_the_ranker():
|
|
"""A heredoc or a pasted script would push the verb and its target — the
|
|
part a rule is about — out of the embedding window."""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
search = AsyncMock(return_value=[])
|
|
with ExitStack() as stack:
|
|
for ctx in _tool_patches(pc, [], MagicMock()):
|
|
stack.enter_context(ctx)
|
|
stack.enter_context(patch.object(pc, "semantic_search_rules", search))
|
|
await pc.build_tool_rule_hint(1, "Bash", "git tag v1 && " + "x" * 5000)
|
|
|
|
sent = search.call_args.args[1]
|
|
assert len(sent) <= pc._TOOL_QUERY_CHARS
|
|
assert sent.startswith("git tag v1"), "the head of the command is the signal"
|
|
|
|
|
|
def test_the_two_pre_tool_arms_share_one_session_rule_ledger():
|
|
"""The integration point most worth guarding.
|
|
|
|
Two ledgers would mean a rule named by the write arm gets re-offered by the
|
|
tool arm — and the hint that fires most often is exactly the one that must
|
|
not repeat itself. Asserted on the FILENAME both scripts build, because
|
|
that is the shared thing; a copy of the path in each is how they drift.
|
|
"""
|
|
prior = Path("plugin/hooks/scribe_prior_art.sh").read_text()
|
|
tool = Path("plugin/hooks/scribe_tool_rules.sh").read_text()
|
|
|
|
for src, name in ((prior, "scribe_prior_art.sh"), (tool, "scribe_tool_rules.sh")):
|
|
assert '"${TMPDIR:-/tmp}/scribe-priorart"' in src, f"{name}: state dir moved"
|
|
assert '.rules.ids' in src, f"{name}: rules ledger filename moved"
|
|
assert "exclude_rule_ids" in src, f"{name}: does not send the exclusion"
|
|
|
|
|
|
def test_the_tool_arm_is_registered_on_bash():
|
|
"""A hook that exists and is not registered runs never — and reads exactly
|
|
like a surface nobody needed."""
|
|
import json
|
|
|
|
manifest = json.loads(Path("plugin/hooks/hooks.json").read_text())
|
|
pre = manifest["hooks"]["PreToolUse"]
|
|
entries = {
|
|
m.get("matcher"): [h["command"] for h in m["hooks"]] for m in pre
|
|
}
|
|
assert "Bash" in entries, "nothing watches Bash — the reflex surface is unguarded"
|
|
assert any("scribe_tool_rules.sh" in c for c in entries["Bash"])
|
|
# The write arm keeps its own matcher; this is an addition, not a move.
|
|
assert any("scribe_prior_art.sh" in c for c in entries["Write|Edit"])
|
|
|
|
|
|
def test_the_hook_and_the_route_agree_on_every_parameter_name():
|
|
"""Rule 33, on a brand-new integration between layers.
|
|
|
|
The hook is shell and the route is Python; nothing but this test connects
|
|
them. A renamed query arg fails SILENTLY — the route reads an absent value,
|
|
the arm quietly searches nothing, and the surface looks like one that never
|
|
finds anything rather than one that is broken.
|
|
"""
|
|
import re
|
|
|
|
hook = Path("plugin/hooks/scribe_tool_rules.sh").read_text()
|
|
route = Path("src/scribe/routes/plugin.py").read_text()
|
|
handler = route.split("async def pre_tool_rules")[1].split("\n@plugin_bp")[0]
|
|
|
|
sent = set(re.findall(r"[?&]([a-z_]+)=", hook))
|
|
assert sent == {"tool", "command", "repo", "exclude_rule_ids"}, sent
|
|
|
|
# `repo` is read by the shared _project_scope() helper, not inline.
|
|
assert "_project_scope()" in handler
|
|
for arg in ("tool", "command", "exclude_rule_ids"):
|
|
assert f'request.args.get("{arg}")' in handler, (
|
|
f"the hook sends {arg!r} and the route never reads it"
|
|
)
|
|
|
|
|
|
# ── The CALL log is unconditional; the SURFACING log is not (#3497) ────
|
|
#
|
|
# Both arms used to write their retrieval_logs row inside a guard on having
|
|
# results, so `zero_result_calls` was pinned at 0 and `cleared_threshold` at
|
|
# `calls` by the shape of the code — at any threshold whatsoever. #3311 read
|
|
# that as a measurement of the corpus and a milestone was scoped on it.
|
|
#
|
|
# The distinction these tests hold: a CALL happened whether or not it found
|
|
# anything, and the calls that found nothing are the only evidence a threshold
|
|
# is set too high. A SURFACING did not happen when nothing was shown.
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_write_path_arm_logs_the_call_that_found_nothing():
|
|
log, rec = MagicMock(), MagicMock()
|
|
await _run_arm([], rec, retrieval_log=log)
|
|
|
|
rule_calls = [c for c in log.call_args_list
|
|
if c.kwargs.get("source") == "write_path_rule"]
|
|
assert len(rule_calls) == 1, (
|
|
"a rule call that found nothing wrote no row — `zero_result_calls` can "
|
|
"then only ever read 0, however badly the threshold is tuned"
|
|
)
|
|
assert rule_calls[0].kwargs["results"] == []
|
|
rec.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_write_path_arm_logs_a_call_whose_only_hit_was_already_shown():
|
|
"""The subtler half. The ranker DID find something; the session had already
|
|
been told. That is a decline from the reader's side and must be logged as
|
|
one — the note arms get this for free by passing exclusions into the search,
|
|
so their zero-result rows already include this case."""
|
|
log, rec = MagicMock(), MagicMock()
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug"))]
|
|
await _run_arm(hits, rec, retrieval_log=log, exclude_rule_ids=[156])
|
|
|
|
rule_calls = [c for c in log.call_args_list
|
|
if c.kwargs.get("source") == "write_path_rule"]
|
|
assert len(rule_calls) == 1
|
|
assert rule_calls[0].kwargs["results"] == [], (
|
|
"the row must record what the arm could SHOW, so this row is comparable "
|
|
"with an auto_inject row, whose exclusions are applied by the search"
|
|
)
|
|
rec.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_logs_the_call_that_found_nothing():
|
|
"""It matters more here than on the sibling. This arm fires on every Bash
|
|
call, so an empty `sources` row is the normal outcome — and with no row at
|
|
all, "the ranker declined" is indistinguishable from "the hook never fired",
|
|
which is exactly the silent failure the arm was built to stop (#3476)."""
|
|
log, rec = MagicMock(), MagicMock()
|
|
out = await _run_tool_arm([], rec, retrieval_log=log)
|
|
|
|
assert out == {"context": "", "rule_ids": []}
|
|
assert log.call_count == 1
|
|
assert log.call_args.kwargs["source"] == "pre_tool_rule"
|
|
assert log.call_args.kwargs["results"] == []
|
|
assert log.call_args.kwargs["query"] == "curl -s https://git.example/api/v1/runs", (
|
|
"the query is the point of the row: it is what a threshold is tuned against"
|
|
)
|
|
rec.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_logs_a_call_whose_only_hit_was_already_shown():
|
|
log, rec = MagicMock(), MagicMock()
|
|
hits = [(0.71, fake_rule(id=161, title="Reach the forge through its MCP tools"))]
|
|
out = await _run_tool_arm(hits, rec, retrieval_log=log, exclude_rule_ids=[161])
|
|
|
|
assert out["rule_ids"] == []
|
|
assert log.call_count == 1
|
|
assert log.call_args.kwargs["results"] == []
|
|
rec.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_command_the_arm_never_searched_writes_no_row_at_all():
|
|
"""The one case that must stay silent, and the boundary of the rule above.
|
|
|
|
A blank command costs no embedding query, so there was no retrieval to log.
|
|
A row here would report a call that never happened and drag the clear-rate
|
|
down with phantom declines — the mirror of the defect, from the other side.
|
|
"""
|
|
from scribe.services import plugin_context as pc
|
|
|
|
log = MagicMock()
|
|
with ExitStack() as stack:
|
|
for ctx in _tool_patches(pc, [], MagicMock(), retrieval_log=log):
|
|
stack.enter_context(ctx)
|
|
await pc.build_tool_rule_hint(1, "Bash", " ")
|
|
|
|
log.assert_not_called()
|
|
|
|
|
|
def test_neither_rule_arm_logs_its_call_behind_a_results_guard():
|
|
"""Structural, on top of the behavioural pair above, because the defect was
|
|
one level of indentation and it appeared INDEPENDENTLY in two places — the
|
|
pre-tool arm inherited it by being modelled on its sibling. The third arm
|
|
modelled on either of them is the one this catches.
|
|
"""
|
|
pc_src = Path("src/scribe/services/plugin_context.py").read_text()
|
|
|
|
# Write-path arm: what remains inside `if fresh:` is the SURFACING log only.
|
|
guarded = pc_src.split('source="write_path_rule", query=code or path')[1]
|
|
guarded = guarded.split("if fresh:")[1].split("except Exception:")[0]
|
|
assert "record_rule_surfaced" in guarded, "the surfacing log must stay guarded"
|
|
assert "record_retrieval" not in guarded, (
|
|
"the call log is back inside the results guard — a call that found "
|
|
"nothing is the only evidence a threshold is set too high"
|
|
)
|
|
|
|
# Pre-tool arm: the call log comes BEFORE the early return.
|
|
#
|
|
# WALKED, NOT SUBSTRING-MATCHED (rule 167). This assertion used to read
|
|
# `body.index("if not fresh:")`, which pinned the name of a local variable
|
|
# rather than the property. #3750 changed that guard to `if not hits:` —
|
|
# the arm still logs before returning, so the property held perfectly, and
|
|
# a name-matching assertion would have raised ValueError and reported the
|
|
# #3497 defect as back. A guard that cries regression when the thing it
|
|
# protects is intact is the failure mode rule 167 names.
|
|
#
|
|
# The property is positional: between the search and the first guard that
|
|
# can return early, the call row has already been written.
|
|
fn = next(
|
|
n for n in ast.walk(ast.parse(pc_src))
|
|
if isinstance(n, ast.AsyncFunctionDef) and n.name == "build_tool_rule_hint"
|
|
)
|
|
search_at = min(
|
|
n.lineno for n in ast.walk(fn)
|
|
if isinstance(n, ast.Call)
|
|
and getattr(n.func, "id", None) == "semantic_search_rules"
|
|
)
|
|
logged_at = min(
|
|
n.lineno for n in ast.walk(fn)
|
|
if isinstance(n, ast.Call)
|
|
and getattr(n.func, "id", None) == "record_retrieval"
|
|
)
|
|
# Every `if <cond>: return ...` after the search — whatever it tests.
|
|
bailouts = [
|
|
n.lineno for n in ast.walk(fn)
|
|
if isinstance(n, ast.If) and n.lineno > search_at
|
|
and any(isinstance(b, ast.Return) for b in n.body)
|
|
]
|
|
assert bailouts, (
|
|
"no early return found after the search in build_tool_rule_hint — the "
|
|
"guard has nothing left to protect, which means this test is now "
|
|
"passing vacuously rather than the arm being correct"
|
|
)
|
|
assert logged_at < min(bailouts), (
|
|
f"the pre-tool arm returns at line {min(bailouts)} before logging its "
|
|
f"call at line {logged_at} — a surface with no rows at all cannot be "
|
|
f"told apart from a hook that never fired (#3497)"
|
|
)
|
|
|
|
|
|
# ── Suppression: which zeros were the ranker, which were repeats (#3497) ──
|
|
#
|
|
# Making the call log unconditional exposed a second ambiguity in the same row.
|
|
# A zero-result rule call is two unrelated events: the ranker found nothing
|
|
# above the bar, or it found only what this session already held. Only the
|
|
# first says anything about the threshold, and a long session excludes its way
|
|
# into the second — so without the split, the arm looks worse the longer it
|
|
# runs correctly.
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_write_path_arm_reports_what_the_session_already_held():
|
|
log, rec = MagicMock(), MagicMock()
|
|
hits = [(0.71, fake_rule(id=156, title="A wait with no deadline is a bug")),
|
|
(0.70, fake_rule(id=157, title="A loop re-arms in a finally"))]
|
|
await _run_arm(hits, rec, retrieval_log=log, exclude_rule_ids=[156, 157])
|
|
|
|
row = next(c for c in log.call_args_list
|
|
if c.kwargs.get("source") == "write_path_rule")
|
|
assert row.kwargs["results"] == []
|
|
assert row.kwargs["suppressed"] == 2, (
|
|
"both hits were repeats, so this zero is not evidence about the bar"
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_genuine_ranker_decline_reports_zero_suppression():
|
|
"""Zero, not None. The arm filters in Python, so it always knows — and
|
|
'measured none' has to stay distinguishable from 'cannot measure'."""
|
|
log, rec = MagicMock(), MagicMock()
|
|
await _run_arm([], rec, retrieval_log=log)
|
|
|
|
row = next(c for c in log.call_args_list
|
|
if c.kwargs.get("source") == "write_path_rule")
|
|
assert row.kwargs["suppressed"] == 0
|
|
assert row.kwargs["suppressed"] is not None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_the_tool_arm_reports_suppression_too():
|
|
log, rec = MagicMock(), MagicMock()
|
|
hits = [(0.75, fake_rule(id=161, title="Reach the forge through its MCP tools"))]
|
|
await _run_tool_arm(hits, rec, retrieval_log=log, exclude_rule_ids=[161])
|
|
|
|
assert log.call_args.kwargs["results"] == []
|
|
assert log.call_args.kwargs["suppressed"] == 1
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_shown_hit_is_not_counted_as_suppressed():
|
|
"""The obvious inverse, worth pinning: `suppressed` counts what was DROPPED,
|
|
not what came back. Off by one here and every zero row reads as a repeat."""
|
|
log, rec = MagicMock(), MagicMock()
|
|
hits = [(0.75, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
|
(0.70, fake_rule(id=12, title="Don't run a local stack unless asked"))]
|
|
out = await _run_tool_arm(hits, rec, retrieval_log=log, exclude_rule_ids=[12])
|
|
|
|
assert out["rule_ids"] == [161]
|
|
assert log.call_args.kwargs["suppressed"] == 1
|
|
assert len(log.call_args.kwargs["results"]) == 1
|
|
|
|
|
|
# ── The identity that falsified this milestone (#3668) ─────────────────
|
|
#
|
|
# `rule_usage.surfaced` == `pre_tool_rule.cleared` + `write_path_rule.cleared`.
|
|
# Milestone #379 was scoped on a reconstruction that put ~64% of ranked rule
|
|
# surfacings as never reaching `rule_usage_events`. Five steps were planned
|
|
# against it. One read of this identity — 17 = 17, then 39 = 39 on a second
|
|
# window — falsified the whole thing: the gap was two counters that started
|
|
# recording on different days, not a write path dropping rows.
|
|
#
|
|
# So the identity is not a nice-to-have. It is the cheapest true statement
|
|
# available about this pair of tables, and its absence is what let a magnitude
|
|
# that merely LOOKED wrong survive a code review and a five-step plan. An
|
|
# identity that must hold exactly beats a magnitude that looks wrong.
|
|
#
|
|
# WHY THE ARM IS THE RIGHT PLACE TO PIN IT, and the readout is not. Inside an
|
|
# arm, one `fresh` list feeds both recorders in one function, so the counts
|
|
# cannot legitimately differ — at any limit. The readout-level form is weaker
|
|
# than it looks: `cleared_threshold` counts CALLS that beat the bar while
|
|
# `surfaced` counts RULES, and those coincide only while `RULEHINT_LIMIT` is 1.
|
|
# Raise the limit and the readout identity breaks while nothing is wrong.
|
|
# `RULEHINT_LIMIT` has already moved once (2 → 1, `2385100`), and that move is
|
|
# half of why the original reconstruction misread its own numbers.
|
|
#
|
|
# Hence three hits below, where production currently returns at most one. The
|
|
# test is deliberately in a state the limit does not permit today, because what
|
|
# is being pinned is that the two recorders read the same list — not that the
|
|
# list happens to be short.
|
|
|
|
_THREE_HITS = [
|
|
(0.81, fake_rule(id=156, title="A wait with no deadline is a bug")),
|
|
(0.77, fake_rule(id=157, title="A loop re-arms in a finally")),
|
|
(0.74, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
|
]
|
|
|
|
_ARMS = [
|
|
("write_path_rule", _run_arm),
|
|
("pre_tool_rule", _run_tool_arm),
|
|
# The prompt arm joins the family rather than being modelled on it — see
|
|
# the block above _prompt_patches for why that distinction is the whole
|
|
# lesson of #3497.
|
|
("prompt_rule", _run_prompt_arm),
|
|
]
|
|
|
|
|
|
def _both_ends(log, rec, source):
|
|
"""What the two recorders said about one call, at the same grain.
|
|
|
|
Ids rather than counts. Equal counts drawn from different lists is a real
|
|
way for this to break — an off-by-one slice, or one recorder reading `hits`
|
|
where the other reads `fresh` in a window where the exclusion happened to
|
|
remove as many as it added — and a count comparison would call that agreement.
|
|
"""
|
|
rows = [c for c in log.call_args_list if c.kwargs.get("source") == source]
|
|
assert len(rows) == 1, (
|
|
f"expected exactly one {source} call row, got {len(rows)} — the "
|
|
f"identity is per call and cannot be read across several"
|
|
)
|
|
logged = [rule.id for _score, rule in rows[0].kwargs["results"]]
|
|
surfaced = [
|
|
rid
|
|
for c in rec.call_args_list if c.kwargs.get("source") == source
|
|
for rid in c.kwargs["rule_ids"]
|
|
]
|
|
return logged, surfaced
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.parametrize(
|
|
("excluded", "expected"),
|
|
[([], 3), ([157], 2), ([156, 157, 161], 0)],
|
|
ids=["nothing-held", "one-already-held", "all-already-held"],
|
|
)
|
|
@pytest.mark.asyncio
|
|
async def test_both_recorders_report_the_same_rules_for_one_call(
|
|
source, run, excluded, expected
|
|
):
|
|
"""One list, two tables, no room to disagree.
|
|
|
|
The middle case is the one that discriminates. With nothing excluded both
|
|
recorders see the same three rules however wrongly they are wired, so an
|
|
arm logging `hits` to the call log and `fresh` to the surfacing log passes
|
|
that case and fails this one — and logging `hits` is exactly the divergence
|
|
that would manufacture an apparent write loss out of a correct system.
|
|
"""
|
|
log, rec = MagicMock(), MagicMock()
|
|
await run(list(_THREE_HITS), rec, retrieval_log=log, exclude_rule_ids=excluded)
|
|
|
|
logged, surfaced = _both_ends(log, rec, source)
|
|
assert surfaced == logged, (
|
|
f"{source} told its two tables different stories about one call: the "
|
|
f"call log recorded {logged} and the surfacing log recorded {surfaced}. "
|
|
f"Both come from `fresh`, in one function, so any difference is a bug "
|
|
f"in the wiring — and it is the shape that reads as a lost write when "
|
|
f"the two tables are later compared in aggregate (#3668)."
|
|
)
|
|
assert len(logged) == expected, (
|
|
"the fixture stopped exercising what it claims to; check the exclusion "
|
|
"filter still runs before both recorders"
|
|
)
|
|
|
|
|
|
# ── A repeat is REFERENCED, not withheld (#3750) ──────────────────────────
|
|
#
|
|
# Both arms used to drop a hit already on the session's exclusion ledger and
|
|
# emit nothing at all. That is right only while the session still HOLDS what it
|
|
# was told — and it stops being right the moment a compaction summarizes the
|
|
# earlier injection away while the id stays on the ledger, which leaves the
|
|
# rule absent from context AND unreachable for the rest of the session.
|
|
#
|
|
# The tests below pin the emitted LINE, not the prose describing it, and both
|
|
# arms are parametrized through one body so the two tails cannot drift apart —
|
|
# #3497's history is that the pre-tool arm inherited a defect from its sibling
|
|
# by being modelled on it rather than sharing with it.
|
|
|
|
_HELD = fake_rule(
|
|
id=156,
|
|
title="A wait with no deadline is a bug",
|
|
statement="Every wait on something that can fail to answer carries a deadline.",
|
|
when_to_apply="writing any call that crosses a process boundary",
|
|
)
|
|
_FRESH_TAIL = "not in this session's loaded set"
|
|
_SEEN_TAIL = "You saw it earlier this session"
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_a_rule_already_on_the_ledger_still_produces_a_line(source, run):
|
|
"""THE REGRESSION, stated as the thing that used to be absent.
|
|
|
|
Falsified against the old behaviour: before #3750 both arms filtered to
|
|
`fresh` before rendering, so an all-excluded call returned an empty
|
|
context and this assertion fails on `context == ""`.
|
|
"""
|
|
out = await run([(0.81, _HELD)], MagicMock(), exclude_rule_ids=[156])
|
|
|
|
# ON `get_rule(156)` RATHER THAN A TRUTHY CONTEXT. The write-path arm's
|
|
# context also carries the prior-art menu, the staleness line and the shape
|
|
# signals, so `assert out["context"]` is TRUE under the old behaviour and
|
|
# would pin nothing on that arm while looking identical to a real check on
|
|
# the other. The rule line is the only part of the string this changes.
|
|
assert "get_rule(156)" in out["context"], (
|
|
f"{source} emitted no rule line for a rule the session had already "
|
|
f"been shown. Silence is only correct while the session still holds "
|
|
f"the line — after a compaction it does not, and the id is still on "
|
|
f"the ledger, so the rule is unreachable for the rest of the session. "
|
|
f"Context was: {out['context']!r}"
|
|
)
|
|
assert _HELD.title in out["context"], (
|
|
"the reference names no rule; a pull pointer with nothing attached "
|
|
"gives a reader no way to judge whether it is worth pulling"
|
|
)
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_the_two_tails_are_distinguishable_and_say_the_true_one(source, run):
|
|
"""One clause differs, and it is the clause that would otherwise be false.
|
|
|
|
A repeat rendered with the FRESH tail would assert "it is not in this
|
|
session's loaded set" about a rule this session was handed twenty minutes
|
|
ago — a line that is wrong in the one way a reader cannot check.
|
|
"""
|
|
fresh_ctx = (await run([(0.81, _HELD)], MagicMock()))["context"]
|
|
seen_ctx = (await run([(0.81, _HELD)], MagicMock(),
|
|
exclude_rule_ids=[156]))["context"]
|
|
|
|
assert _FRESH_TAIL in fresh_ctx and _SEEN_TAIL not in fresh_ctx, (
|
|
f"{source} rendered a first surfacing with the repeat tail"
|
|
)
|
|
assert _SEEN_TAIL in seen_ctx and _FRESH_TAIL not in seen_ctx, (
|
|
f"{source} told the session a rule it has already been shown is not "
|
|
f"in its loaded set"
|
|
)
|
|
assert fresh_ctx != seen_ctx, "the two tails collapsed into one"
|
|
|
|
|
|
# ── the third axis: KIND (milestone 399) ────────────────────────────────
|
|
#
|
|
# A preference is a rule row that does not bind, so it rides the same arms and
|
|
# the same line. What must differ is the REGISTER: a rule's line tells the
|
|
# reader not to dismiss it unread, because dismissing a rule unread is how the
|
|
# thing it prevents happens. A preference makes no such claim — it says where
|
|
# to find how this has been done, and following it buys consistency rather
|
|
# than correctness.
|
|
#
|
|
# Rendered in the rule's voice, a preference becomes the thing milestone 399
|
|
# exists to avoid: a rule with a different column.
|
|
|
|
_PREF = fake_rule(
|
|
id=177,
|
|
kind="preference",
|
|
title="Pace hard debugging one step at a time",
|
|
statement="Advance one investigative step per turn.",
|
|
when_to_apply="during hard debugging",
|
|
)
|
|
_RULE_FORCE = "before deciding it does not apply"
|
|
_PREF_FORCE = "for how this has been done before"
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_a_preference_does_not_speak_in_the_rules_voice(source, run):
|
|
"""The register, pinned on the two places force is actually asserted.
|
|
|
|
On BOTH the noun and the clause, because either alone is weak. A line
|
|
reading "Preference … before deciding it does not apply" has swapped the
|
|
label and kept the instruction, which is worse than not distinguishing
|
|
them at all: it looks handled.
|
|
"""
|
|
ctx = (await run([(0.81, _PREF)], MagicMock()))["context"]
|
|
|
|
assert "Preference that may apply" in ctx, (
|
|
f"{source} announced a preference as something else. The noun is the "
|
|
f"one word a skimming reader gets to place the register, so it is the "
|
|
f"word that has to move. Context was: {ctx!r}"
|
|
)
|
|
assert _PREF_FORCE in ctx and _RULE_FORCE not in ctx, (
|
|
f"{source} told the session to read a PREFERENCE before deciding it "
|
|
f"does not apply. That is a rule's claim: it is the sentence that "
|
|
f"makes a line bind, and a preference does not."
|
|
)
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_kind_and_seen_do_not_read_each_other(source, run):
|
|
"""The structural claim the design rests on: two INDEPENDENT axes.
|
|
|
|
Whether a record is already on the exclusion ledger has nothing to do with
|
|
how much force it carries. Keeping the two unrelated in the code is what
|
|
let a second kind arrive without reopening #3750's repeat question — and
|
|
the way that silently breaks is a `seen` branch that grows a kind test,
|
|
or a kind branch that grows a `seen` test, leaving one of the four
|
|
combinations rendered by nobody's intention.
|
|
|
|
So: all four are exercised, and the seen tail must come out identical for
|
|
both kinds.
|
|
"""
|
|
pref_fresh = (await run([(0.81, _PREF)], MagicMock()))["context"]
|
|
pref_seen = (await run([(0.81, _PREF)], MagicMock(),
|
|
exclude_rule_ids=[177]))["context"]
|
|
rule_seen = (await run([(0.81, _HELD)], MagicMock(),
|
|
exclude_rule_ids=[156]))["context"]
|
|
|
|
assert _SEEN_TAIL in pref_seen and _FRESH_TAIL in pref_fresh, (
|
|
f"{source}: the seen/fresh split stopped working once kind was added — "
|
|
f"the tail axis is now reading the head axis"
|
|
)
|
|
# The tail is about the LEDGER, so it is shared verbatim. Compared as the
|
|
# tail alone rather than the whole line, since the heads differ by design.
|
|
assert _SEEN_TAIL in rule_seen, "the rule's seen tail changed"
|
|
assert pref_seen.count(_SEEN_TAIL) == rule_seen.count(_SEEN_TAIL) == 1, (
|
|
f"{source} rendered the repeat clause a different number of times for "
|
|
f"the two kinds; the tail is about the ledger and does not vary by force"
|
|
)
|
|
# And the head still differs in the seen case — a repeat of a preference
|
|
# is still a preference.
|
|
assert "Preference that may apply" in pref_seen, (
|
|
f"{source} lost the preference register on a repeat, so a preference "
|
|
f"seen twice reads as a rule the second time"
|
|
)
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_neither_tail_injects_the_rule_statement(source, run):
|
|
"""The budget, pinned on both branches.
|
|
|
|
A reference costs the same ~40 tokens as a first surfacing precisely
|
|
because neither carries the statement. If a repeat ever starts inlining the
|
|
body "since we are re-showing it anyway", this arm stops being cheap enough
|
|
to always emit — and always emitting is the whole mechanism.
|
|
"""
|
|
for excluded in ([], [156]):
|
|
ctx = (await run([(0.81, _HELD)], MagicMock(),
|
|
exclude_rule_ids=excluded))["context"]
|
|
assert _HELD.statement not in ctx, (
|
|
f"{source} inlined the rule statement (excluded={excluded!r}); the "
|
|
f"line carries title, trigger and a pull pointer and nothing more"
|
|
)
|
|
assert _HELD.title in ctx and "process boundary" in ctx, (
|
|
"the line dropped the title or the trigger — those are what let a "
|
|
"reader decide whether to pull without pulling"
|
|
)
|
|
|
|
|
|
# ── What a reference IS in the telemetry: nothing new (#3752) ─────────────
|
|
#
|
|
# THE RELATION, STATED BEFORE IT SHIPS. #3750 changes what is RENDERED and
|
|
# nothing about what is COUNTED:
|
|
#
|
|
# result_count counts fresh surfacings — unchanged
|
|
# suppressed_count counts repeats — unchanged
|
|
# rule_usage counts fresh surfacings — unchanged
|
|
#
|
|
# A reference is a rendering decision, not a retrieval outcome. That answer is
|
|
# not free: the naive implementation renders repeats by dropping the `fresh`
|
|
# filter, which takes `suppressed_count` to zero everywhere — and #3739's
|
|
# near-miss fix identifies repeat-caused zeros by `suppressed_count > 0`, so
|
|
# the contamination corrected on 2026-09-08 would return by a different route,
|
|
# in the same field, with the fix still sitting in the code not working.
|
|
#
|
|
# #3712 gave a reader `complete_from` for a counter that started late. Nothing
|
|
# tells a reader a counter's DEFINITION moved. This test is the cheap version
|
|
# of that guarantee: the claim "nothing moved" is only worth anything if it is
|
|
# checkable, so it is asserted rather than described.
|
|
|
|
|
|
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool", "prompt"])
|
|
@pytest.mark.asyncio
|
|
async def test_a_reference_is_rendered_but_not_counted(source, run):
|
|
"""The counters must read exactly as they did before #3750."""
|
|
log, rec = MagicMock(), MagicMock()
|
|
out = await run([(0.81, _HELD)], rec, retrieval_log=log,
|
|
exclude_rule_ids=[156])
|
|
|
|
row = next(c for c in log.call_args_list
|
|
if c.kwargs.get("source") == source).kwargs
|
|
assert row["results"] == [], (
|
|
f"{source} counted a referenced rule as a result. `result_count` "
|
|
f"drives zero_result_calls and the whole threshold picture; a repeat "
|
|
f"is not evidence the bar is set correctly."
|
|
)
|
|
assert row["suppressed"] == 1, (
|
|
f"{source} stopped reporting the repeat as suppressed. #3739's "
|
|
f"near_misses predicate excludes declines with suppressed_count > 0 — "
|
|
f"if this reads 0, every repeat-caused zero is re-counted as a genuine "
|
|
f"ranker rejection and the near-miss contamination returns."
|
|
)
|
|
assert rec.call_count == 0, (
|
|
f"{source} recorded a surfacing for a rule it only referenced, which "
|
|
f"inflates pull_through's denominator with a choice the arm never made"
|
|
)
|
|
assert out["rule_ids"] == [], (
|
|
"a referenced id went back to the hook for the exclusion ledger; it is "
|
|
"already there by definition, and returning it conflates 'shown fresh' "
|
|
"with 'mentioned again'"
|
|
)
|
|
assert "get_rule(156)" in out["context"], (
|
|
"guard is passing vacuously — no rule line was rendered, so 'rendered "
|
|
"but not counted' is not what this run demonstrated. Truthiness of the "
|
|
"whole context will not do: the write-path arm fills it from four "
|
|
"other sources."
|
|
)
|