From d2fa7233732e59aaa1c94d7d67b0d4b568e7d3e5 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 5 Oct 2026 20:25:47 -0400 Subject: [PATCH] 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 --- src/scribe/services/plugin_context.py | 199 +++++++++----------------- tests/test_rule_via_lesson.py | 64 +++++++-- tests/test_system_rulings.py | 8 +- 3 files changed, 124 insertions(+), 147 deletions(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 9167cf4b..5b9641ec 100644 --- a/src/scribe/services/plugin_context.py +++ b/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 diff --git a/tests/test_rule_via_lesson.py b/tests/test_rule_via_lesson.py index 860e9aee..688c2107 100644 --- a/tests/test_rule_via_lesson.py +++ b/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(): diff --git a/tests/test_system_rulings.py b/tests/test_system_rulings.py index fe94da9f..a3b14841 100644 --- a/tests/test_system_rulings.py +++ b/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