feat(500): retire the fixed-question preference arm - completion-report preferences ride the moments
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 1m10s
CI & Build / Python tests (push) Successful in 1m54s
CI & Build / Build & push image (push) Successful in 31s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 1m10s
CI & Build / Python tests (push) Successful in 1m54s
CI & Build / Build & push image (push) Successful in 31s
report_preference searched one constant string when a task closed and handed matches back as update_task's reply_preferences. Milestone 500 step 3 delivers the reply shapes and the preferences mounted beside them on the moments, so the arm goes, whole (rule 22): - services/reply_preferences.py, the REPORT_PREFERENCE RuleArm, its re-scorer and corpus entries, and the REPORTPREF constants - update_task's reply_preferences block and its cue; the docstring now points at moment_rules - the fixed_query field and the fixed_query_never_clears warning: this arm was the only one that set it, so the concept and the cannot_decline exemption go with it - the two Settings fields for its floor and budget - migration 0121 deletes its rows (logs, judgments, rule-usage events, tuning history, settings keys), operator-approved 2026-10-09. Without it the readout would call the source unregistered and its surfacings would count as ambient. The unindexed rule_usage delete is bounded by created_at. #5496 (step 4 of milestone 500). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -4,8 +4,10 @@ The in-band half of milestone 409 step 3: the skill and static context only
|
||||
exist in the Claude Code plugin, and a tool response reaches every MCP client
|
||||
at the moment a piece of work closes. Pinned on the response, not the wording.
|
||||
|
||||
Step 4 adds the operator's own completion-report preferences beside the cue,
|
||||
retrieved at that same moment — and only then, and only when there are any.
|
||||
The operator's own completion-report preferences used to ride here too, from
|
||||
a fixed-question search (409 step 4). Milestone 500 retired that: they are
|
||||
mounted on finishing work and arrive in `moment_rules` beside the delivered
|
||||
reply shape, so this response carries no key of its own for them.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
@@ -13,49 +15,37 @@ import pytest
|
||||
|
||||
pytestmark = pytest.mark.usefixtures("_bind_user")
|
||||
|
||||
_PREF = {"id": 41, "title": "Say how it was checked", "statement": "…", "kind": "preference"}
|
||||
|
||||
|
||||
async def _update(prefs=None, owed=None, **kwargs):
|
||||
async def _update(owed=None, **kwargs):
|
||||
from scribe.mcp.tools.tasks import update_task
|
||||
|
||||
note = MagicMock(id=5, user_id=7, project_id=3, started_at="2026-10-06T09:00:00+00:00")
|
||||
note.to_dict.return_value = {"id": 5}
|
||||
lookup = AsyncMock(return_value=list(prefs or []))
|
||||
with patch("scribe.mcp.tools.tasks.notes_svc.update_note", AsyncMock(return_value=note)), \
|
||||
patch("scribe.mcp.tools.tasks.systems_tools.attach_systems", AsyncMock()), \
|
||||
patch("scribe.mcp.tools.tasks.placement_svc.attach_placement", AsyncMock()), \
|
||||
patch("scribe.mcp.tools.tasks.family_adoption_svc.owed_since",
|
||||
AsyncMock(return_value=list(owed or []))), \
|
||||
patch("scribe.mcp.tools.tasks.reply_prefs_svc.completion_preferences", lookup):
|
||||
return await update_task(task_id=5, **kwargs), lookup
|
||||
AsyncMock(return_value=list(owed or []))):
|
||||
return await update_task(task_id=5, **kwargs)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("status", ["done", "cancelled"])
|
||||
async def test_closing_a_task_carries_the_cue(status):
|
||||
out, _ = await _update(status=status)
|
||||
out = await _update(status=status)
|
||||
assert "placement" in out["report_back"] and "needs them" in out["report_back"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("kwargs", [{"status": "in_progress"}, {"status": "todo"}, {"body": "more notes"}])
|
||||
async def test_other_updates_do_not(kwargs):
|
||||
out, lookup = await _update(prefs=[_PREF], **kwargs)
|
||||
assert "report_back" not in out and "reply_preferences" not in out
|
||||
lookup.assert_not_awaited()
|
||||
out = await _update(**kwargs)
|
||||
assert "report_back" not in out
|
||||
|
||||
|
||||
async def test_closing_hands_back_the_operators_report_preferences():
|
||||
out, lookup = await _update(prefs=[_PREF], status="done")
|
||||
assert out["reply_preferences"] == [_PREF]
|
||||
# The cue says the key is there, so it never arrives unexplained.
|
||||
assert "reply_preferences" in out["report_back"]
|
||||
lookup.assert_awaited_once_with(7, project_id=3)
|
||||
|
||||
|
||||
async def test_no_preferences_means_no_key_and_the_plain_cue():
|
||||
async def test_closing_carries_the_plain_cue_and_no_preference_key_of_its_own():
|
||||
"""The retired key does not come back beside the moment delivery."""
|
||||
from scribe.mcp.tools.tasks import REPORT_BACK_CUE
|
||||
|
||||
out, _ = await _update(prefs=[], status="done")
|
||||
out = await _update(status="done")
|
||||
assert "reply_preferences" not in out
|
||||
assert out["report_back"] == REPORT_BACK_CUE
|
||||
|
||||
@@ -69,11 +59,11 @@ async def test_closing_names_the_owed_adoptions_filed_while_the_task_was_open():
|
||||
into a project during this task comes back for the report to name."""
|
||||
from scribe.services.family_adoption import OWED_CUE
|
||||
|
||||
out, _ = await _update(owed=[_OWED], status="done")
|
||||
out = await _update(owed=[_OWED], status="done")
|
||||
assert out["family_owed"] == [_OWED]
|
||||
assert out["report_back"].endswith(OWED_CUE) and "family_owed" in out["report_back"]
|
||||
|
||||
|
||||
async def test_no_owed_adoptions_means_no_key():
|
||||
out, _ = await _update(owed=[], status="done")
|
||||
out = await _update(owed=[], status="done")
|
||||
assert "family_owed" not in out
|
||||
|
||||
@@ -103,10 +103,8 @@ def test_the_extractor_finds_something() -> None:
|
||||
def test_the_extractor_resolves_the_constant_sources() -> None:
|
||||
"""The specific capability a grep would lose. See the module docstring.
|
||||
|
||||
`report_preference` was the second example until milestone 456 moved it
|
||||
into the retrieval pipeline, where its source is a spec field; the
|
||||
pipeline's reserved slot still records through a module constant, so it
|
||||
carries the case instead.
|
||||
The pipeline's reserved slot records through a module constant, so it
|
||||
carries the case.
|
||||
"""
|
||||
found, _ = call_sites()
|
||||
for via_constant in ("wide_net", "preference_slot"):
|
||||
|
||||
@@ -34,7 +34,6 @@ def test_every_ranked_source_is_measured_as_its_spec_declares():
|
||||
point = POINTS[spec.source]
|
||||
assert point.kind == UNBIDDEN
|
||||
assert point.what == d.what
|
||||
assert point.fixed_query == d.fixed_query
|
||||
# A quiet source must say why, and only a quiet source may (#2475).
|
||||
assert point.expects_traffic == (not d.quiet_because)
|
||||
assert point.quiet_because == d.quiet_because
|
||||
|
||||
@@ -24,9 +24,9 @@ WHAT THIS PINS
|
||||
different file. Renaming one without the other produces a surface that can
|
||||
be tuned and cannot be measured, or measured and not tuned, and both fail
|
||||
silently.
|
||||
2. **Keys are unique.** Two surfaces sharing a settings key is how
|
||||
`report_preference` spent its first release moving whenever the prompt arm
|
||||
was tuned (#3860) — one dial wearing two labels.
|
||||
2. **Keys are unique.** Two surfaces sharing a settings key is how the
|
||||
completion-report arm (since retired) spent its first release moving
|
||||
whenever the prompt arm was tuned (#3860) — one dial wearing two labels.
|
||||
3. **An unknown surface is refused.** Settings keys are free-form strings in
|
||||
a generic table, so a typo'd name would write a key nothing reads: a
|
||||
change that appears to succeed, reports a new value, and alters nothing.
|
||||
@@ -44,7 +44,7 @@ import pytest
|
||||
|
||||
from scribe.services import retrieval_surfaces as rs
|
||||
|
||||
SERVICES = ("plugin_context", "reply_preferences")
|
||||
SERVICES = ("plugin_context",)
|
||||
|
||||
|
||||
def _service_source(name: str) -> str:
|
||||
@@ -63,10 +63,6 @@ def test_every_surface_name_is_a_real_telemetry_source():
|
||||
to justify a change describes a different arm from the one the change hits.
|
||||
"""
|
||||
blob = "\n".join(_service_source(n) for n in SERVICES)
|
||||
# `report_preference` passes its name through a module constant rather than
|
||||
# a literal, so that one name is satisfied by the constant holding it.
|
||||
from scribe.services.reply_preferences import SOURCE
|
||||
|
||||
# The rule arms record through the one pipeline (milestone 456), where
|
||||
# `source` is the spec's own field — so for them the spec IS the string
|
||||
# `record_retrieval` receives, and the join key is checked against it.
|
||||
@@ -76,7 +72,7 @@ def test_every_surface_name_is_a_real_telemetry_source():
|
||||
missing = [
|
||||
s.name for s in rs.SURFACES.values()
|
||||
if f'source="{s.name}"' not in blob
|
||||
and s.name != SOURCE and s.name not in via_pipeline
|
||||
and s.name not in via_pipeline
|
||||
]
|
||||
assert not missing, (
|
||||
f"these surfaces can be tuned but never measured: {missing}. The "
|
||||
|
||||
@@ -471,83 +471,6 @@ def test_a_quiet_arm_is_not_suspended_either_way() -> None:
|
||||
assert "floor_moved_mid_window" not in codes(ws)
|
||||
|
||||
|
||||
# ── a fixed-query arm: decline rate is arithmetic, not evidence (#4232) ─────
|
||||
#
|
||||
# `report_preference` searches one constant string (COMPLETION_QUERY), so it
|
||||
# scores against one number on every call. Its decline rate is therefore 0% or
|
||||
# 100% and never in between, and which one depends only on where the bar sits
|
||||
# relative to that constant.
|
||||
#
|
||||
# So the two warnings swap roles for these arms. "Never declined" stops being
|
||||
# evidence about the floor — `cannot_decline`'s own remedy, "check that it
|
||||
# applies its floor", is unanswerable from it. "Always declined" starts being
|
||||
# evidence, because for a constant score it means the bar is above it and no
|
||||
# further traffic will ever say otherwise.
|
||||
|
||||
|
||||
def test_cannot_decline_is_silent_on_a_fixed_query_arm():
|
||||
"""The live readout fired this on `report_preference` at 45 calls, 0
|
||||
empty, with p10 = p50 = p90 = min = max = 0.791 — five identical
|
||||
percentiles, which is one record at one score rather than a ranking."""
|
||||
ws = warn({"report_preference": src(calls=45, zero_result_calls=0, p10=0.791)})
|
||||
assert "cannot_decline" not in codes(ws, "report_preference")
|
||||
|
||||
|
||||
def test_cannot_decline_still_fires_where_the_rate_means_something():
|
||||
"""The falsifier for the case above (rule 167). If this passes only
|
||||
because the check was disabled rather than narrowed, this fails."""
|
||||
ws = warn({"auto_inject": src(calls=N, zero_result_calls=0)})
|
||||
assert "cannot_decline" in codes(ws, "auto_inject")
|
||||
|
||||
|
||||
def test_a_fixed_query_arm_that_never_clears_its_bar_is_named():
|
||||
"""69 consecutive declines at 0.0006 under the bar is a state this arm has
|
||||
actually been in. Nothing else in the readout would have said so: it looks
|
||||
exactly like an arm with nothing to report."""
|
||||
ws = warn({"report_preference": src(calls=45, zero_result_calls=45)})
|
||||
assert "fixed_query_never_clears" in codes(ws, "report_preference")
|
||||
|
||||
detail = next(w["detail"] for w in ws if w["code"] == "fixed_query_never_clears")
|
||||
assert "near_miss_samples" in detail, (
|
||||
"the last time this fired, every percentile said lower the floor and "
|
||||
"the refused record showed the refusal was right — so the warning has "
|
||||
"to send the reader to the record, not to the dial"
|
||||
)
|
||||
|
||||
|
||||
def test_an_ordinary_arm_returning_nothing_all_window_is_not_dead():
|
||||
"""For an arm whose score can vary, an empty window means nothing matched,
|
||||
which is an answer rather than a fault."""
|
||||
ws = warn({"auto_inject": src(calls=N, zero_result_calls=N)})
|
||||
assert "fixed_query_never_clears" not in codes(ws, "auto_inject")
|
||||
|
||||
|
||||
def test_a_fixed_query_arm_that_sometimes_clears_is_not_dead():
|
||||
"""Only ALL-empty says the bar is above the constant. Anything in between
|
||||
means the score is not actually constant, and the premise is wrong."""
|
||||
ws = warn({"report_preference": src(calls=45, zero_result_calls=44)})
|
||||
assert "fixed_query_never_clears" not in codes(ws, "report_preference")
|
||||
|
||||
|
||||
def test_the_dead_arm_warning_still_needs_volume():
|
||||
ws = warn({"report_preference": src(calls=N - 1, zero_result_calls=N - 1)})
|
||||
assert "fixed_query_never_clears" not in codes(ws)
|
||||
|
||||
|
||||
def test_the_registry_declares_which_arms_ask_a_fixed_question():
|
||||
"""Asserted on structure (rule 167), and able to fail: if `fixed_query`
|
||||
is dropped or defaults to True, one of these two halves breaks."""
|
||||
from scribe.services.retrieval_registry import POINTS
|
||||
|
||||
assert POINTS["report_preference"].fixed_query is True, (
|
||||
"services/reply_preferences.py::COMPLETION_QUERY is a module constant"
|
||||
)
|
||||
# An arm whose query is built from the prompt, the file or the command is
|
||||
# not fixed, and marking one would silence a warning that works there.
|
||||
for varying in ("auto_inject", "write_path", "pre_tool_rule", "prompt_rule"):
|
||||
assert POINTS[varying].fixed_query is False, varying
|
||||
|
||||
|
||||
# ── floor_history_unknown: the window reaches back past the ledger ─────────
|
||||
#
|
||||
# THE SAME SUSPENSION, FOR THE CASE THE ONE ABOVE CANNOT SEE.
|
||||
|
||||
@@ -1702,7 +1702,6 @@ def test_every_hook_rule_search_says_which_project_it_is_for():
|
||||
# ranked-search stage, checked below — so a direct search appearing
|
||||
# here again is a copy of the arm coming back, and fails the count.
|
||||
"src/scribe/services/plugin_context.py": 0,
|
||||
"src/scribe/services/reply_preferences.py": 0,
|
||||
}
|
||||
for path, expected in sources.items():
|
||||
calls = [
|
||||
|
||||
@@ -1,134 +0,0 @@
|
||||
"""The completion-report preference lookup (milestone 409 step 4).
|
||||
|
||||
What it pins: the lookup asks for PREFERENCES only, logs every call under its
|
||||
own source (the empty ones too), counts only what it showed as surfaced, and
|
||||
fails open. The query stays domain-neutral, because every kind of project
|
||||
closes tasks.
|
||||
"""
|
||||
import re
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
M = "scribe.services.reply_preferences"
|
||||
# Its two numbers are resolved through the registry now (#4102), so the
|
||||
# settings reader to patch lives there rather than in the arm's own module.
|
||||
RS = "scribe.services.retrieval_surfaces"
|
||||
|
||||
|
||||
def _rule(rid, kind="preference"):
|
||||
return MagicMock(id=rid, kind=kind, title=f"t{rid}", statement=f"s{rid}")
|
||||
|
||||
|
||||
async def _run(hits, *, searched=True, raises=None):
|
||||
from scribe.services.reply_preferences import completion_preferences
|
||||
|
||||
async def search(user_id, query, **kw):
|
||||
if raises:
|
||||
raise raises
|
||||
kw["report"].update({"searched": searched, "best_available_score": 0.7})
|
||||
return hits
|
||||
|
||||
search_mock = AsyncMock(side_effect=search)
|
||||
with patch(f"{M}.semantic_search_rules", search_mock), \
|
||||
patch(f"{RS}.get_setting", AsyncMock(return_value="0.72")), \
|
||||
patch(f"{M}.record_retrieval") as logged, \
|
||||
patch(f"{M}.record_rule_surfaced") as surfaced:
|
||||
out = await completion_preferences(7, project_id=3)
|
||||
return out, search_mock, logged, surfaced
|
||||
|
||||
|
||||
async def test_asks_for_preferences_only_and_returns_them_best_first():
|
||||
out, search, logged, surfaced = await _run([(0.9, _rule(1)), (0.8, _rule(2))])
|
||||
assert search.await_args.kwargs["kind"] == "preference"
|
||||
assert [p["id"] for p in out] == [1, 2]
|
||||
assert all(p["kind"] == "preference" for p in out)
|
||||
assert logged.call_args.kwargs["source"] == "report_preference"
|
||||
assert surfaced.call_args.kwargs == {"user_id": 7, "rule_ids": [1, 2], "source": "report_preference"}
|
||||
|
||||
|
||||
async def test_a_rule_that_slips_through_is_not_handed_back_as_a_preference():
|
||||
out, _, _, surfaced = await _run([(0.9, _rule(1, kind="rule")), (0.8, _rule(2))])
|
||||
assert [p["id"] for p in out] == [2]
|
||||
assert surfaced.call_args.kwargs["rule_ids"] == [2]
|
||||
|
||||
|
||||
async def test_an_empty_call_is_still_logged_and_surfaces_nothing():
|
||||
out, _, logged, surfaced = await _run([])
|
||||
assert out == []
|
||||
logged.assert_called_once()
|
||||
assert logged.call_args.kwargs["results"] == []
|
||||
surfaced.assert_not_called()
|
||||
|
||||
|
||||
async def test_a_search_that_never_ran_says_so_to_the_log():
|
||||
_, _, logged, _ = await _run([], searched=False)
|
||||
assert logged.call_args.kwargs["searched"] is False
|
||||
|
||||
|
||||
async def test_fails_open():
|
||||
out, _, _, surfaced = await _run([], raises=RuntimeError("embedder down"))
|
||||
assert out == []
|
||||
surfaced.assert_not_called()
|
||||
|
||||
|
||||
def test_it_is_a_ranked_source():
|
||||
from scribe.services.rule_usage import is_ambient
|
||||
|
||||
assert not is_ambient("report_preference")
|
||||
|
||||
|
||||
def test_its_bar_is_its_own_key_not_the_prompt_arm_s(monkeypatch):
|
||||
"""Regression on the coupling #3860 found (and on the fix being real).
|
||||
|
||||
This arm shipped reading PROMPTRULE_THRESHOLD_KEY, so an operator tuning
|
||||
the bar for their own prose moved this one with it and was never told.
|
||||
That is worse here than anywhere else: every other arm scores a query that
|
||||
varies per call, while COMPLETION_QUERY is fixed — so this arm's score for
|
||||
a given corpus is a CONSTANT, and a constant that lands under the bar is a
|
||||
dead arm rather than a quiet one. No amount of traffic reveals it.
|
||||
|
||||
Asserted on the key the lookup actually asks for, which is the thing that
|
||||
broke, rather than on the constant being defined somewhere.
|
||||
"""
|
||||
import asyncio
|
||||
|
||||
from scribe.services import plugin_context
|
||||
from scribe.services.reply_preferences import completion_preferences
|
||||
|
||||
asked: list[str] = []
|
||||
|
||||
async def get_setting(user_id, key, default=""):
|
||||
asked.append(key)
|
||||
return default
|
||||
|
||||
async def search(user_id, query, **kw):
|
||||
kw["report"].update({"searched": True})
|
||||
return []
|
||||
|
||||
with patch(f"{RS}.get_setting", AsyncMock(side_effect=get_setting)), \
|
||||
patch(f"{M}.semantic_search_rules", AsyncMock(side_effect=search)), \
|
||||
patch(f"{M}.record_retrieval"):
|
||||
asyncio.run(completion_preferences(7))
|
||||
|
||||
# Both numbers are its own since #4102 — a floor AND a budget — so the arm
|
||||
# asks for two keys, and neither may be the prompt arm's.
|
||||
from scribe.services.retrieval_surfaces import get_surface
|
||||
|
||||
mine = get_surface("report_preference")
|
||||
assert asked == [mine.floor_key, mine.budget_key]
|
||||
theirs = get_surface("prompt_rule")
|
||||
assert mine.floor_key != theirs.floor_key, (
|
||||
"the two keys are the same string again, so the settings form has one "
|
||||
"dial driving two arms — which is the defect, whatever the value is"
|
||||
)
|
||||
assert mine.floor_key == plugin_context.REPORTPREF_THRESHOLD_KEY, (
|
||||
"the registry and the module constant disagree about this arm's key, "
|
||||
"so the Settings form would write one and the arm would read the other"
|
||||
)
|
||||
|
||||
|
||||
def test_the_query_assumes_no_particular_domain():
|
||||
from scribe.services.reply_preferences import COMPLETION_QUERY
|
||||
|
||||
dev_only = [w for w in (r"\bCI\b", r"\bcommit", r"\bpull request", r"\bcode\b", r"\btest")
|
||||
if re.search(w, COMPLETION_QUERY, re.IGNORECASE)]
|
||||
assert not dev_only, f"software-only vocabulary in a query every project runs: {dev_only}"
|
||||
@@ -53,7 +53,6 @@ _SURFACE_PAIRS = (
|
||||
("write_path_rule", "kbRuleHintThreshold"),
|
||||
("pre_tool_rule", "kbToolRuleThreshold"),
|
||||
("prompt_rule", "kbPromptRuleThreshold"),
|
||||
("report_preference", "kbReportPrefThreshold"),
|
||||
("reply_rule", "kbReplyRuleThreshold"),
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user