CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 54s
CI & Build / Python tests (push) Failing after 1m8s
CI & Build / Build & push image (push) Skipped
Milestone 416 step 5. The operator's compromise — "if you're worried about excluding potentially important data let limit it to 50 entries" — moved to the surface where it is safe. Fifty in the push would be milestone 394 with extra steps; fifty in a pull crowds nothing out. `what_might_apply(query)` returns ranked rule candidates with NO threshold. The moment it serves is the one where the caller does not trust a bar to decide for them, so it does not have one — every row carries its score and the reader judges. WHY IT IS NOT A BIGGER `limit` ON `search` `_search_rules` returns statement, why and how_to_apply in full, on the stated reasoning that a caller who went looking deserves the whole record. That is the DEEP pull and should stay that way. This is the SHALLOW one — many candidates, each just enough to decide whether to open it. Opposite trade-offs, so it is a second tool. THE PREMISE THE STEP GOT WRONG The task said fifty "costs nothing". `_rule_hint_line` had already measured otherwise: ~143 tokens per line once the trigger is rendered, and #3855 tripled trigger lengths across the corpus. Fifty is ~7,000 tokens — cheap next to an arm firing before every Bash call, but not free, and a tool promising a free wide net gets reached for casually and then regretted. So it reuses the graduated shape #3851 measured for the push: the top few carry their trigger whole, the rest carry a cut of it. TRUNCATED, never dropped — the trigger is what lets a reader judge without opening, and a teaser without one is just an id. The cut borrows `_goal_line`'s technique including the fallback that matters (#4036): `textwrap.shorten` returns a bare "…" for one unbroken word. TELEMETRY Logged under its own pull source, asserted absent from both the tunable push registry and AMBIENT_SOURCES. That guard is load-bearing right now: the push arms' near-miss distributions are the evidence #4121 argues from, and a pull folded into them would move those numbers. INSTRUCTION SURFACES The using-scribe reflex and the MCP instructions both pointed at `search(content_type="rule")` for the consequential moment — the deep tool, at the moment you want breadth. They now point here, and keep `search` for reading a rule you already suspect. Written as a practice rather than a prohibition (rule 165). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
196 lines
8.4 KiB
Python
196 lines
8.4 KiB
Python
"""Moving a floor, and the reason that has to come with it (#4102).
|
|
|
|
WHY THIS EXISTS
|
|
|
|
The operator handed the dial to the model: *"the user should be able to touch it
|
|
but the model should be the thing handling it 9 times out of 10."* Everything
|
|
here guards the half of that sentence people skip — the operator still has to be
|
|
able to see what was done on their behalf and disagree with it.
|
|
|
|
WHAT THIS PINS
|
|
|
|
1. **A reason is required, and "" does not count.** The column is NOT NULL,
|
|
which an empty string satisfies; the service is where the requirement is
|
|
real. This is the whole guardrail: a caller who must write down why has to
|
|
have looked, and a caller who writes down something wrong leaves the
|
|
operator a sentence to argue with. A number that moved silently leaves
|
|
nothing.
|
|
2. **The change is recorded, with what it was before.** Without `old_value` a
|
|
history cannot answer "was this always like that", which is the first
|
|
question anyone asks of a surface behaving oddly.
|
|
3. **A clamp is reported, never swallowed.** A caller that believes it set 1.4
|
|
will read the next telemetry as evidence about a bar that was never in
|
|
force — the same class of error as #3739, one layer up.
|
|
4. **An unknown surface is refused before anything is written.** Settings keys
|
|
are free-form strings, so a typo'd surface would write a key nothing reads:
|
|
a change that reports success and alters nothing.
|
|
5. **The tool teaches the procedure that works**, not the one the numbers
|
|
suggest. Asserted on the docstring because the docstring IS the contract an
|
|
agent reads, and the failure it prevents is a model tuning from a
|
|
percentile — which has been measured pointing the wrong way.
|
|
"""
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from scribe.services import retrieval_tuning as rt
|
|
from tests.helpers import make_mock_session
|
|
|
|
|
|
def _patches(floor=0.72, budget=3):
|
|
"""Patch everything `set_dial` touches except the thing under test."""
|
|
session = make_mock_session()
|
|
return session, (
|
|
patch.object(rt, "async_session", MagicMock(return_value=session)),
|
|
patch.object(rt, "set_setting", AsyncMock()),
|
|
patch.object(rt, "floor_for", AsyncMock(return_value=floor)),
|
|
patch.object(rt, "budget_for", AsyncMock(return_value=budget)),
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.parametrize("reason", ["", " ", "noisy", "too high"])
|
|
async def test_a_change_without_a_real_reason_is_refused(reason):
|
|
"""The guardrail. "too high" is a restatement of the change, not a basis."""
|
|
session, ctx = _patches()
|
|
with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError) as e:
|
|
await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason)
|
|
# The message has to name the tool that produces a real basis, or the
|
|
# caller's next move is a longer sentence rather than a look at the records.
|
|
assert "near_miss_samples" in str(e.value)
|
|
session.add.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_nothing_is_written_when_the_reason_is_refused():
|
|
"""Refused BEFORE the setting is touched, not after.
|
|
|
|
Writing the value and then raising would leave the number moved and the
|
|
history empty — the exact state this table exists to make impossible.
|
|
"""
|
|
session, ctx = _patches()
|
|
with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \
|
|
ctx[2], ctx[3]:
|
|
with pytest.raises(ValueError):
|
|
await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason="x")
|
|
setter.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_good_change_writes_the_setting_and_the_event():
|
|
session, ctx = _patches(floor=0.72)
|
|
reason = ("read prompt_rule's 5 highest declines: 3 were project rules for "
|
|
"another repo, so the bar is doing its job here")
|
|
with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \
|
|
ctx[2], ctx[3]:
|
|
out = await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason)
|
|
|
|
setter.assert_awaited_once()
|
|
_uid, key, stored = setter.await_args.args
|
|
assert key == "kb_promptrule_threshold" and stored == "0.66"
|
|
|
|
event = session.add.call_args.args[0]
|
|
assert event.surface == "prompt_rule" and event.dial == "floor"
|
|
# The before-value is what makes the history answerable.
|
|
assert event.old_value == 0.72 and event.new_value == 0.66
|
|
assert event.reason == reason and event.actor == "model"
|
|
assert out["previous"] == 0.72 and out["applied"] == 0.66
|
|
assert out["clamped"] is False
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.parametrize("dial, sent, applied", [
|
|
("floor", 1.4, 1.0),
|
|
("floor", -0.2, 0.0),
|
|
("budget", 99, 10),
|
|
("budget", 0, 1),
|
|
])
|
|
async def test_a_clamp_is_reported_rather_than_swallowed(dial, sent, applied):
|
|
"""Said out loud, because silence here poisons the next reading.
|
|
|
|
A caller that believes it set 1.4 treats the following week's telemetry as
|
|
evidence about a bar that never existed, and then moves the dial again to
|
|
fix a problem it invented.
|
|
"""
|
|
session, ctx = _patches()
|
|
reason = "checked the refused records for this surface and they were fine"
|
|
with ctx[0], ctx[1], ctx[2], ctx[3]:
|
|
out = await rt.set_dial(1, "auto_inject", dial, sent, reason=reason)
|
|
assert out["applied"] == applied
|
|
assert out["clamped"] is True
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_unknown_surface_is_refused_before_anything_is_written():
|
|
session, ctx = _patches()
|
|
with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \
|
|
ctx[2], ctx[3]:
|
|
with pytest.raises(ValueError):
|
|
await rt.set_dial(1, "pretool_rule", "floor", 0.6,
|
|
reason="a perfectly good reason that is long enough")
|
|
setter.assert_not_called()
|
|
session.add.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.parametrize("dial", ["threshold", "limit", "", "FLOOR"])
|
|
async def test_an_unknown_dial_is_refused(dial):
|
|
"""The near-misses are the old vocabulary — `threshold` and `limit` are what
|
|
these were called before this step, so they are exactly what a stale caller
|
|
will send, and a silent no-op there would be indistinguishable from a
|
|
change that did not take."""
|
|
session, ctx = _patches()
|
|
with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError):
|
|
await rt.set_dial(1, "auto_inject", dial, 0.6,
|
|
reason="a perfectly good reason that is long enough")
|
|
session.add.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_human_change_is_distinguishable_from_the_models():
|
|
"""Both act as the same user, so the id cannot tell them apart — and "did I
|
|
do this, or did the session?" is the first question the history is asked."""
|
|
session, ctx = _patches()
|
|
with ctx[0], ctx[1], ctx[2], ctx[3]:
|
|
await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="human",
|
|
reason="operator set this themselves in Settings")
|
|
assert session.add.call_args.args[0].actor == "human"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_unknown_actor_is_refused():
|
|
"""Free text here would make the column unreadable within a month."""
|
|
session, ctx = _patches()
|
|
with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError):
|
|
await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="agent",
|
|
reason="a perfectly good reason that is long enough")
|
|
|
|
|
|
def test_the_tool_teaches_reading_the_records_not_the_percentile():
|
|
"""The contract an agent actually reads (rule 167).
|
|
|
|
The failure this prevents is a model moving a bar because `near_misses.p90`
|
|
sat close to it. That has been measured pointing the wrong way — 69 declines
|
|
where every percentile said "lower it" and the refused record was a false
|
|
positive — so the docstring has to carry the method, not just the warning.
|
|
"""
|
|
from scribe.mcp.tools import retrieval_tuning as tool
|
|
|
|
doc = tool.tune_retrieval.__doc__
|
|
assert "near_miss_samples" in doc, "the tool does not name how to get records"
|
|
assert "PERCENTILE ALONE" in doc.upper()
|
|
# And the worked example, because an abstract warning loses to a number.
|
|
assert "69" in doc
|
|
|
|
|
|
def test_all_three_tools_are_registered():
|
|
from scribe.mcp.tools import retrieval_tuning as tool
|
|
|
|
from tests.helpers import FakeMCP
|
|
|
|
mcp = FakeMCP()
|
|
tool.register(mcp)
|
|
assert mcp.names == [
|
|
"retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history",
|
|
]
|