fix(tests): the config stand-in fell behind the real one, and ten arms silently no-opped (#4214)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 44s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m7s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 44s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m7s
CI & Build / Build & push image (push) Skipped
CI 7124 Python tests: 14 failed. One root cause behind ten of them, and the failure was the exact one `tests/helpers.writepath_cfg`'s docstring already warns about in prose — while being unable to prevent this instance of it. Three arms read their numbers out of that config dict inside a fail-open `except`. A missing key raises where nobody sees it, so the arm becomes a silent no-op, indistinguishable from the arm working and finding nothing. The helper derives its keys from `retrieval_surfaces.SURFACES` precisely to stop that — and `checkpoint_threshold` is deliberately NOT a surface, because everything in that table is a floor/budget pair belonging to one query and the checkpoint runs none. The derivation therefore could not see it, the write-path rule arm died before `record_retrieval`, and ten tests went red at once. Fixed at the helper, from the module constant, so there is still exactly one literal and it lives in the product. And the guard the docstring claimed now exists: `test_the_config_stand_in_carries_every_key_the_real_one_does` compares the stand-in's key set against the real `get_writepath_config`, so the next key that is not a surface fails loudly here instead of quietly disabling an arm under test. `test_retrieval_surfaces`'s hand-written key list gains it for the same reason, spelled out in place. The other four were the contract widening itself: `checkpoint` is present on every return of the tool arm, including its early ones, so four assertions comparing the whole dict needed it. That key is deliberately always present — the two arms feed one shell reader where an absent key and an empty one are read the same, so the difference is invisible exactly where it would bite. Verified statically: every cfg in the suite now routes through `writepath_cfg` (test_write_path_trigger's local `_cfg` delegates to it), no hand-written config dict survives, and the real config's eight keys match the stand-in's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -374,7 +374,18 @@ def writepath_cfg(**over):
|
||||
So the keys come from `retrieval_surfaces.SURFACES`. A seventh surface, or a
|
||||
rename, changes this helper for free and cannot quietly disable an arm in
|
||||
ten hand-written dicts that each looked complete on the day they were typed.
|
||||
|
||||
NOT EVERY KEY IS A SURFACE, and that gap already bit once (#4214). The
|
||||
checkpoint bar has no entry in SURFACES on purpose — everything in that
|
||||
table is a floor/budget pair belonging to one QUERY, and the checkpoint
|
||||
runs none, it re-reads hits the rule arms already produced. Adding it there
|
||||
would give it a phantom budget. So it is taken from the module constant
|
||||
instead, which keeps the "one literal, in the product" property even though
|
||||
the derivation differs. `test_the_config_stand_in_carries_every_key_the_
|
||||
real_one_does` is what makes the next addition fail loudly here rather than
|
||||
silently no-op an arm, which is the whole claim this docstring makes.
|
||||
"""
|
||||
from scribe.services.plugin_context import _CHECKPOINT_DEFAULT
|
||||
from scribe.services.retrieval_surfaces import SURFACES
|
||||
|
||||
cfg = {
|
||||
@@ -385,6 +396,7 @@ def writepath_cfg(**over):
|
||||
"rule_top_k": SURFACES["write_path_rule"].budget_default,
|
||||
"tool_rule_threshold": SURFACES["pre_tool_rule"].floor_default,
|
||||
"tool_rule_top_k": SURFACES["pre_tool_rule"].budget_default,
|
||||
"checkpoint_threshold": _CHECKPOINT_DEFAULT,
|
||||
}
|
||||
cfg.update(over)
|
||||
return cfg
|
||||
|
||||
@@ -183,5 +183,9 @@ async def test_the_write_path_config_carries_every_arm_it_drives():
|
||||
cfg = await pc.get_writepath_config(1)
|
||||
|
||||
for key in ("threshold", "top_k", "rule_threshold", "rule_top_k",
|
||||
"tool_rule_threshold", "tool_rule_top_k"):
|
||||
"tool_rule_threshold", "tool_rule_top_k",
|
||||
# Not a surface, and that is why it is spelled out here: it
|
||||
# has no floor/budget pair to derive from, so nothing else in
|
||||
# this file would notice it going missing (#4214).
|
||||
"checkpoint_threshold"):
|
||||
assert key in cfg, f"{key} missing — its arm will silently no-op"
|
||||
|
||||
@@ -508,7 +508,7 @@ async def test_the_prompt_arm_says_nothing_when_asked_nothing():
|
||||
stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock()))
|
||||
out = await pc.build_prompt_rule_hint(1, " ")
|
||||
|
||||
assert out == {"context": "", "rule_ids": []}
|
||||
assert out == {"context": "", "rule_ids": [], "checkpoint": {}}
|
||||
search.assert_not_called()
|
||||
log.assert_not_called()
|
||||
|
||||
@@ -653,7 +653,7 @@ async def test_an_empty_command_asks_the_ranker_nothing():
|
||||
stack.enter_context(patch.object(pc, "record_rule_surfaced", rec))
|
||||
out = await pc.build_tool_rule_hint(1, "Bash", " ")
|
||||
|
||||
assert out == {"context": "", "rule_ids": []}
|
||||
assert out == {"context": "", "rule_ids": [], "checkpoint": {}}
|
||||
search.assert_not_called()
|
||||
rec.assert_not_called()
|
||||
|
||||
@@ -669,7 +669,7 @@ async def test_the_tool_arm_fails_open():
|
||||
AsyncMock(side_effect=RuntimeError("boom"))))
|
||||
out = await pc.build_tool_rule_hint(1, "Bash", "docker compose up -d")
|
||||
|
||||
assert out == {"context": "", "rule_ids": []}
|
||||
assert out == {"context": "", "rule_ids": [], "checkpoint": {}}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -833,7 +833,7 @@ async def test_the_tool_arm_logs_the_call_that_found_nothing():
|
||||
log, rec = MagicMock(), MagicMock()
|
||||
out = await _run_tool_arm([], rec, retrieval_log=log)
|
||||
|
||||
assert out == {"context": "", "rule_ids": []}
|
||||
assert out == {"context": "", "rule_ids": [], "checkpoint": {}}
|
||||
assert log.call_count == 1
|
||||
assert log.call_args.kwargs["source"] == "pre_tool_rule"
|
||||
assert log.call_args.kwargs["results"] == []
|
||||
|
||||
@@ -518,3 +518,37 @@ async def test_write_path_labels_a_non_snippet_hit_with_its_kind():
|
||||
assert '[similar 0.72] "debounce helper"' in ctx # the snippet does not
|
||||
# The header now names the right opener for each kind.
|
||||
assert "get_task(id)" in ctx and "get_snippet(id)" in ctx
|
||||
|
||||
|
||||
# ── The config stand-in cannot fall behind the real one (#4214) ───────────
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_config_stand_in_carries_every_key_the_real_one_does():
|
||||
"""THE GUARD `writepath_cfg`'s DOCSTRING ALREADY CLAIMED AND DID NOT HAVE.
|
||||
|
||||
Three arms read their numbers out of that dict inside a fail-open
|
||||
`except`, so a missing key does not raise where anyone can see it — the
|
||||
arm silently becomes a no-op, which is indistinguishable from the arm
|
||||
working and finding nothing. The helper derives its SURFACE keys from the
|
||||
registry to prevent exactly that, and then #4214 added a key that is
|
||||
deliberately not a surface: the whole derivation missed it, ten tests went
|
||||
red at once, and the diagnosis cost a CI round.
|
||||
|
||||
Asserting the KEY SETS match, not the values: the stand-in exists to let a
|
||||
test set different numbers.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
from scribe.services import plugin_context as pc
|
||||
from tests.helpers import writepath_cfg
|
||||
|
||||
with patch.object(pc, "get_setting", AsyncMock(return_value="0.6")), \
|
||||
patch.object(pc, "floor_for", AsyncMock(return_value=0.6)), \
|
||||
patch.object(pc, "budget_for", AsyncMock(return_value=3)):
|
||||
real = await pc.get_writepath_config(1)
|
||||
|
||||
assert set(writepath_cfg()) == set(real), (
|
||||
"tests/helpers.writepath_cfg has fallen behind get_writepath_config; "
|
||||
"a key the real config has and the stand-in does not turns an arm "
|
||||
"into a silent no-op under test"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user