refactor(retrieval): the rule builders compose one moment - _rule_moment runs the arm and its via-lesson step for all three (milestone 456 step 6, #4908)

build_prompt_rule_hint, build_tool_rule_hint and build_write_path_hint
each ran the rule arm and then the via-lesson step by hand, the first two
passing the query between them through a _via_query key on the payload.
They now call one composer, _rule_moment, and the split helpers
(_prompt_rule_hint, _tool_rule_hint, _add_rules_via_lessons) and the side
channel are deleted. Output shapes are unchanged: the prompt builder
returns no checkpoint key, the tool builder always does, and
shown_rule_ids stays the direct band.

A structural test pins that only _rule_moment runs a rule arm or the
via-lesson step, and that the three builders are its only callers.

plugin_context.py: 2,418 -> 2,361 lines this step; 3,558 when the
milestone began.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/scribe/services/plugin_context.py

@@ -998,31 +998,33 @@ async def _rules_via_lessons(
     return result.lines, result.rule_ids
 
 
-async def _add_rules_via_lessons(
-    user_id: int, out: dict, *, project_id: int, exclude_rule_ids, held_rule_ids,
-    where: str,
-) -> dict:
-    """Run the via-lesson step after a direct rule arm, on the query that arm
-    actually searched with.
+async def _rule_moment(
+    arm: rp.RuleArm, moment: rp.RuleMoment, *, floor: float, budget: int,
+    checkpoint_floor: float = 0.0,
+) -> rp.RuleResult:
+    """One rule moment, composed: the direct arm, then the rules its matching
+    lessons bring in (#4633) — after the band and never in place of it.
 
-    The arm leaves `_via_query` in its payload only when it RAN — enabled, and
-    with something to search — so an arm the operator switched off, or a
-    blank prompt, brings no rule in through a side door either. The key is
-    popped here; it never leaves the server.
+    Every rule builder below calls this ONE function, where each used to run
+    the arm and then the via-lesson step by hand, passing the query between
+    them through a key on the payload. The via-lesson step searches with the
+    query the arm searched with, and skips what the arm named plus what the
+    session's ledger holds — suppression is by RULE, whichever lesson reached
+    it. Its lines and fresh ids join the arm's; `shown_rule_ids` stays the
+    direct arm's band, which is what the soft-link recorder pairs (#4637).
     """
-    query = out.pop("_via_query", "")
-    if not query:
-        return out
-    skip = (set(exclude_rule_ids or []) | set(out.get("rule_ids") or [])
-            | set(out.get("shown_rule_ids") or []))
-    lines, ids = await _rules_via_lessons(
-        user_id, query, project_id=project_id or None, skip=skip,
-        held=set(held_rule_ids or []), where=where,
+    result = await rp.run_rule_arm(
+        arm, moment, floor=floor, budget=budget, io=_rule_io(),
+        checkpoint_floor=checkpoint_floor,
     )
-    if lines:
-        out["context"] = "\n".join(c for c in (out.get("context") or "", *lines) if c)
-        out["rule_ids"] = list(out.get("rule_ids") or []) + ids
-    return out
+    lines, ids = await _rules_via_lessons(
+        moment.user_id, moment.query, project_id=moment.project_id or None,
+        skip=set(moment.exclude) | set(result.shown_rule_ids),
+        held=set(moment.held), where=moment.where,
+    )
+    result.lines.extend(lines)
+    result.rule_ids.extend(ids)
+    return result
 
 
 async def build_prompt_rule_hint(
@@ -1033,28 +1035,6 @@ async def build_prompt_rule_hint(
     exclude_rule_ids: list[int] | None = None,
     held_rule_ids: list[int] | None = None,
     context: str = "",
-) -> dict:
-    """Rules and preferences that may apply to what the operator just asked —
-    matched directly (`_prompt_rule_hint`, where the design is written), then
-    reached through a linked lesson (`_rules_via_lessons`)."""
-    out = await _prompt_rule_hint(
-        user_id, query, project_id=project_id, exclude_rule_ids=exclude_rule_ids,
-        held_rule_ids=held_rule_ids, context=context,
-    )
-    return await _add_rules_via_lessons(
-        user_id, out, project_id=project_id, exclude_rule_ids=exclude_rule_ids,
-        held_rule_ids=held_rule_ids, where="to this request",
-    )
-
-
-async def _prompt_rule_hint(
-    user_id: int,
-    query: str,
-    *,
-    project_id: int = 0,
-    exclude_rule_ids: list[int] | None = None,
-    held_rule_ids: list[int] | None = None,
-    context: str = "",
 ) -> dict:
     """Rules and preferences that may apply to what the operator just asked.
 
@@ -1081,6 +1061,10 @@ async def _prompt_rule_hint(
     rules unquoted separates the two claims visually with no extra prose, and
     matches how a rule line already renders on both act arms.
 
+    NO `checkpoint` KEY (#4214): the act arms can hold a call because there
+    is a composed act to hold, while this one fires before anything has been
+    decided.
+
     Fails open and returns empty context on any error, like its siblings: a
     recall aid may never break the operator's prompt.
     """
@@ -1093,16 +1077,12 @@ async def _prompt_rule_hint(
     # names it — and "yes, go ahead" names nothing a trigger can match, while
     # the reply it answers ("commit this to dev and push") does.
     q = _autoinject_query(q, context)
-    # The query this arm searches with, for the via-lesson step that follows
-    # it (#4633) — set only once the arm is going to run.
-    out["_via_query"] = q
-
     try:
         # SCOPED TO THIS SESSION'S PROJECT (milestone 414): global rules plus
         # the bound project's own. An unbound session (project_id 0) gets
         # global rules only — this surface speaks unasked, and a whole-rulebook
         # answer is only right for someone who asked the whole rulebook.
-        result = await rp.run_rule_arm(
+        result = await _rule_moment(
             rp.PROMPT_RULE,
             rp.RuleMoment(
                 user_id=user_id, query=q, project_id=project_id,
@@ -1112,14 +1092,13 @@ async def _prompt_rule_hint(
             ),
             floor=await floor_for(user_id, "prompt_rule"),
             budget=await budget_for(user_id, "prompt_rule"),
-            io=_rule_io(),
         )
         if result.lines:
             out["context"] = "\n".join(result.lines)
             out["rule_ids"] = result.rule_ids
-            # Every rule LINE, repeats and the reserved slot included — for
-            # the soft-link recorder (#4637). Distinct from `rule_ids`, which
-            # is telemetry's fresh-only cut.
+            # Every rule LINE the direct arm showed, repeats and the reserved
+            # slot included — for the soft-link recorder (#4637). Distinct
+            # from `rule_ids`, which is telemetry's fresh-only cut.
             out["shown_rule_ids"] = result.shown_rule_ids
     except Exception:
         logger.debug("prompt rule arm failed", exc_info=True)
@@ -1877,11 +1856,13 @@ async def build_write_path_hint(
     # with no body), and a line says the rule may apply "here".
     #
     # Fails open like every other arm: a rule hint must never break a write.
+    # The direct band, then a rule reached through a linked lesson (#4633),
+    # composed by `_rule_moment` exactly as the other two rule builders are.
     rule_ids: list[int] = []
     shown_rule_ids: list[int] = []
     checkpoint: dict = {}
     try:
-        result = await rp.run_rule_arm(
+        result = await _rule_moment(
             rp.WRITE_PATH_RULE,
             rp.RuleMoment(
                 user_id=user_id, query=code or path, project_id=project_id,
@@ -1890,28 +1871,17 @@ async def build_write_path_hint(
                 held=frozenset(held_rule_ids or []),
             ),
             floor=cfg["rule_threshold"], budget=cfg["rule_top_k"],
-            io=_rule_io(), checkpoint_floor=cfg["checkpoint_threshold"],
+            checkpoint_floor=cfg["checkpoint_threshold"],
         )
         lines.extend(result.lines)
         rule_ids.extend(result.rule_ids)
-        # Every rule line, repeats included — what the soft-link recorder
-        # pairs with this response's lessons (#4637).
+        # Every rule line the band showed, repeats included — what the
+        # soft-link recorder pairs with this response's lessons (#4637).
         shown_rule_ids = result.shown_rule_ids
         checkpoint = result.checkpoint
     except Exception:
         logger.debug("write-path rule arm failed", exc_info=True)
 
-    # A rule reached through a linked lesson (#4633), after the direct band
-    # and never in place of it. Skips what the band already named and what
-    # the session's ledger holds — suppression is by rule.
-    via_lines, via_ids = await _rules_via_lessons(
-        user_id, code or path, project_id=project_id or None,
-        skip=set(exclude_rule_ids or []) | set(shown_rule_ids),
-        held=set(held_rule_ids or []), where="here",
-    )
-    lines.extend(via_lines)
-    rule_ids.extend(via_ids)
-
     # A lesson and a rule on this one response (#4637): the soft-link
     # recorder counts the pair, keyed on the FILE — every edit to one file is
     # one situation — and asks about it once the evidence holds. Fails open.
@@ -1954,54 +1924,9 @@ async def build_tool_rule_hint(
     cwd: str = "",
     seen_ruling_systems: list[int] | None = None,
 ) -> dict:
-    """Standing rules that may apply to the action about to be taken — matched
-    directly (`_tool_rule_hint`, where the design is written), then reached
-    through a linked lesson (`_rules_via_lessons`, #4633) — and the rulings of
-    any area whose files the command names (milestone 444).
-
-    `root` and `cwd` are the repo's absolute root and the command's working
-    directory, from the hook; they are what turn the paths a command names
-    into the repo-relative paths a System's patterns are written in."""
-    out = await _tool_rule_hint(
-        user_id, tool_name, command, project_id=project_id,
-        exclude_rule_ids=exclude_rule_ids, held_rule_ids=held_rule_ids,
-    )
-    out = await _add_rules_via_lessons(
-        user_id, out, project_id=project_id, exclude_rule_ids=exclude_rule_ids,
-        held_rule_ids=held_rule_ids, where=f"to this {tool_name} call",
-    )
-    # Its own arm, not part of the rule search: a lookup by path, with no
-    # floor and no budget, so it adds to the rule lines rather than competing
-    # with them. First, because a ruling is the operator's own decision.
-    # `ruling_system_ids` is set only when a ruling was shown: this arm fires
-    # on every command, and the hook reads an absent list as an empty one.
-    try:
-        paths = system_rulings_svc.command_paths(command, root=root, cwd=cwd) if project_id else []
-        if paths and (await get_writepath_config(user_id)).get("enabled"):
-            rulings = await system_rulings_svc.rulings_for_paths(
-                user_id, project_id, paths,
-                seen=seen_ruling_systems, source="rulings_pre_tool",
-            )
-            if rulings["lines"]:
-                out["context"] = "\n".join(
-                    rulings["lines"] + ([out["context"]] if out.get("context") else [])
-                )
-                out["ruling_system_ids"] = rulings["system_ids"]
-    except Exception:
-        logger.debug("pre-tool rulings arm failed", exc_info=True)
-    return out
-
-
-async def _tool_rule_hint(
-    user_id: int,
-    tool_name: str,
-    command: str,
-    *,
-    project_id: int = 0,
-    exclude_rule_ids: list[int] | None = None,
-    held_rule_ids: list[int] | None = None,
-) -> dict:
-    """Standing rules that may apply to the ACTION about to be taken (#3476).
+    """Standing rules that may apply to the ACTION about to be taken (#3476) —
+    matched directly, then reached through a linked lesson (`_rule_moment`) —
+    and the rulings of any area whose files the command names (milestone 444).
 
     The sibling of the write-path rule arm, and the surface that was missing.
     That arm is keyed on `code or path`, so a rule can only be retrieved at the
@@ -2020,10 +1945,12 @@ async def _tool_rule_hint(
     which tools it watches, so widening the matcher is a `hooks.json` edit with
     no change here.
 
-    EVERY TIER, since #3702 — the tier filter this docstring used to describe
-    is gone from both arms, for the reason recorded at RULEHINT_LIMIT: present
-    in context and salient at the moment are different properties, and only the
-    second is what this arm is for.
+    EVERY TIER, since #3702 — present in context and salient at the moment are
+    different properties, and only the second is what this arm is for.
+
+    `root` and `cwd` are the repo's absolute root and the command's working
+    directory, from the hook; they are what turn the paths a command names
+    into the repo-relative paths a System's patterns are written in.
 
     Fails open and returns an empty context on any error: a recall aid may
     never break the operator's action.
@@ -2044,26 +1971,22 @@ async def _tool_rule_hint(
         cfg = await get_writepath_config(user_id)
         if not cfg.get("enabled"):
             return out
-
         # The command text is the query. A long heredoc or a pasted script
         # would otherwise push the meaningful head of the command out of the
         # embedding window, so it is bounded — the verb and its target sit at
         # the front, which is the part a rule is about.
-        query = command[:_TOOL_QUERY_CHARS]
-        # For the via-lesson step (#4633) — set only once the arm is enabled.
-        out["_via_query"] = query
-
-        result = await rp.run_rule_arm(
+        result = await _rule_moment(
             rp.PRE_TOOL_RULE,
             rp.RuleMoment(
-                user_id=user_id, query=query, project_id=project_id,
+                user_id=user_id, query=command[:_TOOL_QUERY_CHARS],
+                project_id=project_id,
                 where=f"to this {tool_name} call",
                 checkpoint_where=f"this {tool_name} call",
                 exclude=frozenset(exclude_rule_ids or []),
                 held=frozenset(held_rule_ids or []),
             ),
             floor=cfg["tool_rule_threshold"], budget=cfg["tool_rule_top_k"],
-            io=_rule_io(), checkpoint_floor=cfg["checkpoint_threshold"],
+            checkpoint_floor=cfg["checkpoint_threshold"],
         )
         if result.lines:
             out["context"] = "\n".join(result.lines)
@@ -2071,6 +1994,26 @@ async def _tool_rule_hint(
             out["checkpoint"] = result.checkpoint
     except Exception:
         logger.debug("pre-tool rule arm failed", exc_info=True)
+
+    # Its own arm, not part of the rule search: a lookup by path, with no
+    # floor and no budget, so it adds to the rule lines rather than competing
+    # with them. First, because a ruling is the operator's own decision.
+    # `ruling_system_ids` is set only when a ruling was shown: this arm fires
+    # on every command, and the hook reads an absent list as an empty one.
+    try:
+        paths = system_rulings_svc.command_paths(command, root=root, cwd=cwd) if project_id else []
+        if paths and (await get_writepath_config(user_id)).get("enabled"):
+            rulings = await system_rulings_svc.rulings_for_paths(
+                user_id, project_id, paths,
+                seen=seen_ruling_systems, source="rulings_pre_tool",
+            )
+            if rulings["lines"]:
+                out["context"] = "\n".join(
+                    rulings["lines"] + ([out["context"]] if out.get("context") else [])
+                )
+                out["ruling_system_ids"] = rulings["system_ids"]
+    except Exception:
+        logger.debug("pre-tool rulings arm failed", exc_info=True)
     return out
 
 

tests/test_rule_via_lesson.py

@@ -18,6 +18,7 @@ import pytest
 from scribe.services import plugin_context as pc
 from scribe.services import rule_usage
 from scribe.services.retrieval_registry import POINTS
+from scribe.services.retrieval_pipeline import RuleMoment, RuleResult
 
 # Bound before conftest's autouse stub replaces the module attribute.
 _REAL = pc._rules_via_lessons
@@ -123,23 +124,56 @@ async def test_a_failure_brings_no_rule_and_raises_nothing():
 
 
 @pytest.mark.asyncio
-async def test_the_step_runs_only_when_the_arm_ran_and_never_leaks_its_key():
+async def test_the_step_follows_the_arm_on_its_query_and_skips_what_it_named():
+    """One composer for all three rule builders (milestone 456 step 6): the
+    via-lesson step searches with the query the arm searched with, skips the
+    session's ledger and every rule the arm's band named, and its lines and
+    fresh ids join the arm's — while `shown_rule_ids` stays the direct band."""
     step = AsyncMock(return_value=(["Standing rule … Reached through lesson #41"], [7]))
+    direct = RuleResult(lines=["direct line"], rule_ids=[5], shown_rule_ids=[5, 6])
+    moment = RuleMoment(user_id=1, query="q", project_id=2, where="here",
+                        exclude=frozenset({3, 6}))
+    with patch.object(pc, "_rules_via_lessons", step), \
+         patch.object(pc.rp, "run_rule_arm", AsyncMock(return_value=direct)):
+        out = await pc._rule_moment(pc.rp.PROMPT_RULE, moment, floor=0.5, budget=3)
+    assert step.await_args.args[1] == "q"
+    assert step.await_args.kwargs["skip"] == {3, 5, 6}
+    assert out.rule_ids == [5, 7]
+    assert out.lines == ["direct line", "Standing rule … Reached through lesson #41"]
+    assert out.shown_rule_ids == [5, 6]
+
+
+@pytest.mark.asyncio
+async def test_a_blank_prompt_runs_neither_the_arm_nor_the_step():
+    step = AsyncMock(return_value=([], []))
     with patch.object(pc, "_rules_via_lessons", step):
-        idle = await pc._add_rules_via_lessons(
-            1, {"context": "", "rule_ids": []}, project_id=2,
-            exclude_rule_ids=[3], held_rule_ids=[], where="here",
-        )
-        ran = await pc._add_rules_via_lessons(
-            1, {"context": "direct line", "rule_ids": [5], "_via_query": "q"},
-            project_id=2, exclude_rule_ids=[3], held_rule_ids=[], where="here",
-        )
-    assert idle == {"context": "", "rule_ids": []}
-    assert step.await_count == 1
-    assert step.await_args.kwargs["skip"] == {3, 5}
-    assert "_via_query" not in ran
-    assert ran["rule_ids"] == [5, 7]
-    assert ran["context"].startswith("direct line\n")
+        out = await pc.build_prompt_rule_hint(1, "   ")
+    assert out == {"context": "", "rule_ids": []}
+    step.assert_not_awaited()
+
+
+def test_only_the_composer_runs_a_rule_arm_or_the_via_lesson_step():
+    """Structural (rule 167): every rule builder goes through `_rule_moment`,
+    so a builder that runs the arm or the via-lesson step by hand again is the
+    copy this step removed coming back — and a count is what lets it fail."""
+    import ast
+    from pathlib import Path
+
+    tree = ast.parse(Path("src/scribe/services/plugin_context.py").read_text())
+    callers: dict[str, set[str]] = {}
+    for fn in ast.walk(tree):
+        if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)):
+            continue
+        for call in ast.walk(fn):
+            if isinstance(call, ast.Call):
+                name = getattr(call.func, "attr", None) or getattr(call.func, "id", None)
+                if name in ("run_rule_arm", "_rules_via_lessons", "_rule_moment"):
+                    callers.setdefault(name, set()).add(fn.name)
+    assert callers["run_rule_arm"] == {"_rule_moment"}
+    assert callers["_rules_via_lessons"] == {"_rule_moment"}
+    assert callers["_rule_moment"] == {
+        "build_prompt_rule_hint", "build_tool_rule_hint", "build_write_path_hint",
+    }
 
 
 def test_the_source_is_ranked_and_registered():

tests/test_system_rulings.py

@@ -22,6 +22,7 @@ import pytest
 
 from scribe.services import plugin_context as pc
 from scribe.services import system_rulings as sr
+from scribe.services.retrieval_pipeline import RuleResult
 # Bound at import, before conftest's autouse stub replaces the module attribute.
 from scribe.services.system_rulings import rulings_for_paths as real_rulings_for_paths
 from tests.helpers import writepath_cfg
@@ -198,8 +199,8 @@ RULING_LINE = "> Rulings for `src/a.py` — the operator's decisions about A (Sy
 @pytest.mark.asyncio
 async def test_the_tool_arm_leads_with_rulings_and_hands_back_what_it_showed():
     rulings = AsyncMock(return_value={"lines": [RULING_LINE], "system_ids": [3]})
-    with patch.object(pc, "_tool_rule_hint", AsyncMock(return_value={
-            "context": "> a rule line", "rule_ids": [8], "checkpoint": {}})), \
+    with patch.object(pc, "_rule_moment", AsyncMock(return_value=RuleResult(
+            lines=["> a rule line"], rule_ids=[8]))), \
          patch.object(pc, "get_writepath_config", AsyncMock(return_value=writepath_cfg())), \
          patch.object(pc.system_rulings_svc, "rulings_for_paths", rulings):
         out = await pc.build_tool_rule_hint(
@@ -215,8 +216,7 @@ async def test_the_tool_arm_leads_with_rulings_and_hands_back_what_it_showed():
 
 @pytest.mark.asyncio
 async def test_the_tool_arm_adds_no_key_when_no_ruling_was_shown():
-    with patch.object(pc, "_tool_rule_hint", AsyncMock(return_value={
-            "context": "", "rule_ids": [], "checkpoint": {}})), \
+    with patch.object(pc, "_rule_moment", AsyncMock(return_value=RuleResult())), \
          patch.object(pc, "get_writepath_config", AsyncMock(return_value=writepath_cfg())):
         out = await pc.build_tool_rule_hint(1, "Bash", "ls", project_id=2)
     assert "ruling_system_ids" not in out