From 07bdbf1647771dfb6b1842e8c8dee3ae2bfee246 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 14:45:32 -0400 Subject: [PATCH] feat(rules)!: a rule cannot be created, or edited into, having no trigger (#4099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `when_to_apply` is not metadata. `rule_document` embeds a rule as `{title} — {trigger}` / `When to apply: {trigger}\n\n{statement}`, the trigger appearing twice so purpose dominates a short vector — the shape note 2485 measured on snippets (a 0.153 top-to-second gap against 0.010–0.023 for everything else). Without one the document silently becomes title + statement: a DIFFERENT shape, ranked against a corpus it does not match, with nothing to report it. Every bar and every rank in the system assumes one shape. `create_preference` has refused an empty trigger since it shipped. The two rule creators defaulted it to "" — so the shape was enforced for the record kind that guides and optional for the kind that binds. The guard lives in the SERVICE, because both doors reach it: the MCP tools and the frontend's fast path in routes/rulebooks.py. Written in either alone, the other could still create a rule that never fires. The route keeps a matching check for the STATUS CODE only (400, not the 404 it maps ValueError to). update_rule refuses to EMPTY an existing trigger, checked after the mutation so it covers `clear=[...]`, an emptied form input, and any route added later. Deliberately asked as "did this edit remove one" rather than "does one exist": a rule predating the guard has none, and refusing to save it would freeze precisely the unreachable records that most need fixing. Deliberately not following arose_from_id, which the human door exempts itself from because provenance is about auditing what the AGENT changed. That reasoning does not reach this field — a missing trigger is not a missing explanation, it is a rule that does not work, and it fails an operator as badly as a session. 15 test fixtures across 6 files were creating rules with no trigger. They now pass one; that they did not is the point — curation is not a guarantee. Step 1 of milestone 416 "Retrieval stops guessing a bar". First because every later step assumes one document shape, and it is much cheaper to guarantee before a corpus grows than to backfill after. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/rulebooks.py | 16 ++- src/scribe/routes/rulebooks.py | 9 ++ src/scribe/services/rulebooks.py | 51 ++++++++ ..._integration_embedding_yields_to_delete.py | 1 + tests/test_integration_rule_verification.py | 4 + tests/test_integration_rule_versions.py | 2 + tests/test_mcp_tool_rulebooks.py | 6 + tests/test_preference_write_path.py | 1 + tests/test_rule_trigger_required.py | 119 ++++++++++++++++++ tests/test_services_rulebooks.py | 1 + 10 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 tests/test_rule_trigger_required.py diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 630201f..e193ce8 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -246,7 +246,7 @@ async def get_rule(rule_id: int) -> dict: async def create_rule( - topic_id: int, title: str, statement: str, when_to_apply: str = "", + topic_id: int, title: str, statement: str, when_to_apply: str, why: str = "", how_to_apply: str = "", order_index: int = 0, arose_from_id: int = 0, verify_with: str = "", expires_when: str = "", system_ids: list[int] | None = None, force: bool = False, @@ -344,10 +344,14 @@ async def create_rule( when_to_apply: WHEN this rule fires — the trigger, not the instruction. State the moment or the material: "before any git push", "when adding a value to a CHECK-gated column", "when a - release is being cut". Write it even though the parameter is - optional: it is how the rule is found - when it matters, and a rule nobody can place is a rule nobody - applies. + release is being cut". REQUIRED, and not as ceremony: nothing is + preloaded, so this is the whole of how the rule is found when it + matters — and it is half of what the rule is EMBEDDED as, so a + rule without one is not merely hard to find, it is stored in a + different shape from every rule it competes with. Name the SYMPTOM + — the words someone would type while stuck — rather than the + category: "the CI job passed locally and fails on the runner with + a permission error" retrieves; "when touching CI config" does not. This field is also the rule's RETRIEVAL SURFACE — it and the statement are what a search is matched against, so it should carry the SYMPTOM, not just the situation: the words someone @@ -411,7 +415,7 @@ async def create_rule( async def create_project_rule( - project_id: int, statement: str, title: str = "", when_to_apply: str = "", + project_id: int, statement: str, when_to_apply: str, title: str = "", why: str = "", how_to_apply: str = "", order_index: int = 0, arose_from_id: int = 0, verify_with: str = "", expires_when: str = "", system_ids: list[int] | None = None, force: bool = False, diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index b94ef57..e9e2bc4 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -345,6 +345,15 @@ async def create_project_rule(project_id: int): statement = (data.get("statement") or "").strip() if not statement: return jsonify({"error": "statement is required"}), 400 + # Checked here as well as in the service, only for the STATUS CODE: the + # service raises ValueError, which this route maps to 404 for "project not + # found", and a missing trigger is a 400. The service stays the guard — + # this is the door telling the truth about whose mistake it was. + if not (data.get("when_to_apply") or "").strip(): + return jsonify({ + "error": "when_to_apply is required: a rule with no trigger never " + "surfaces at the moment it applies." + }), 400 title = (data.get("title") or "").strip() or statement.split(".")[0][:50] try: rule = await rulebooks_svc.create_project_rule( diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 57b0849..9c60f47 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -457,12 +457,44 @@ async def rule_detail(user_id: int, rule: Rule, system_ids: list[int] | None = N return data +def _require_trigger(when_to_apply: str | None) -> None: + """A rule without a trigger is not a quiet rule — it is an unreachable one. + + Nothing is preloaded, so `when_to_apply` is the whole of how a rule + arrives. It is also what the record is EMBEDDED as: `rule_document` builds + `{title} — {trigger}` / `When to apply: {trigger}\\n\\n{statement}`, with + the trigger appearing twice so that purpose dominates a short vector. Drop + it and the document silently changes shape to title + statement, so the + same score means something different for that rule than for its + neighbours — and every bar and every rank in the system assumes one shape. + + ENFORCED IN THE SERVICE, so both doors are covered: the MCP tools and the + frontend's fast path (`routes/rulebooks.py`) both land here, and a guard + written in one of them would leave the other able to create a rule that + never fires. + + Deliberately NOT following `arose_from_id`, which the human door exempts + itself from on the stated grounds that provenance is about auditing what + the AGENT changed. That reasoning does not reach this field: a missing + trigger is not a missing explanation, it is a rule that does not work, and + it fails an operator exactly as badly as it fails a session. + """ + if not (when_to_apply or "").strip(): + raise ValueError( + "when_to_apply is required: a rule with no trigger never surfaces " + "at the moment it applies. Name that moment in the words a session " + "would actually be producing then — the command, the error, the " + "half-formed ask — not the category it belongs to." + ) + + async def create_rule( topic_id: int, user_id: int, title: str, statement: str, why: str = "", how_to_apply: str = "", order_index: int = 0, when_to_apply: str = "", arose_from_id: int = 0, verify_with: str = "", expires_when: str = "", kind: str = "rule", ) -> Rule: + _require_trigger(when_to_apply) async with async_session() as session: await _assert_topic_owned(session, topic_id, user_id) rule = Rule( @@ -498,6 +530,7 @@ async def create_project_rule( rule in a rulebook topic is global. Topic_id is left NULL — the CHECK constraint enforces exactly-one of (topic_id, project_id). """ + _require_trigger(when_to_apply) async with async_session() as session: await _assert_project_owned(session, project_id, user_id) rule = Rule( @@ -651,6 +684,17 @@ async def update_rule( "verify_with", "expires_when", } check_before = rule.verify_with + # A create-time guard is worth nothing if an edit can undo it, and + # both doors can: `clear=["when_to_apply"]` from the MCP side, and a + # emptied form input normalised to None from the REST side. Checked + # AFTER the mutation instead, so it covers every route to an empty + # trigger including ones added later. + # + # Asked as "did this edit REMOVE a trigger", not "does one exist": + # a rule predating the guard has none, and refusing to save it would + # make the record permanently unfixable — freezing the exact rules + # that most need the edit. + trigger_before = (rule.when_to_apply or "").strip() # Captured BEFORE anything is written, and as plain values — this has # to survive the mutation below. A rule's history is the only record # of what it used to say; the edit itself destroys that. @@ -677,6 +721,13 @@ async def update_rule( # rule wrongly vouched for costs the thing the sweep exists to catch. if rule.verify_with != check_before: rule.verified_at = None + if trigger_before and not (rule.when_to_apply or "").strip(): + raise ValueError( + "when_to_apply cannot be cleared: it is how this rule arrives, " + "and it is half of what the rule is embedded as. Replace the " + "trigger with a better one rather than removing it — a rule " + "with none is not a quieter rule, it is an unreachable one." + ) # Same session as the edit, so the two commit together. The snapshot # holds the OLD verify_with — the check that was in force when that # wording was written — which is why it is taken before the loop and diff --git a/tests/test_integration_embedding_yields_to_delete.py b/tests/test_integration_embedding_yields_to_delete.py index be17d73..356562b 100644 --- a/tests/test_integration_embedding_yields_to_delete.py +++ b/tests/test_integration_embedding_yields_to_delete.py @@ -75,6 +75,7 @@ async def seeded(): book = await rulebooks_svc.create_rulebook(uid, "Lock fixtures") topic = await rulebooks_svc.create_topic(book.id, uid, "locks") rule = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "A rule with vectors", "Something for the embedder to index.", ) diff --git a/tests/test_integration_rule_verification.py b/tests/test_integration_rule_verification.py index 3d5ee78..4523442 100644 --- a/tests/test_integration_rule_verification.py +++ b/tests/test_integration_rule_verification.py @@ -44,6 +44,7 @@ async def constraint(): book = await rulebooks_svc.create_rulebook(uid, "Environment facts") topic = await rulebooks_svc.create_topic(book.id, uid, "ci") rule = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "The runner has no bash", "Write every `run:` step in POSIX sh.", verify_with="read the workflow's shell setting", @@ -153,13 +154,16 @@ async def rulebook_of_three(): book = await rulebooks_svc.create_rulebook(uid, "Sweep fixture") topic = await rulebooks_svc.create_topic(book.id, uid, "mixed") decision = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "dev is home", "Work directly on dev.", ) never = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "The runner has no bash", "Use POSIX sh.", verify_with="read the workflow's shell setting", ) stale = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "Bumps need a dashboard tick", "Tick it first.", verify_with="cat CI-runner/renovate/config.js", ) diff --git a/tests/test_integration_rule_versions.py b/tests/test_integration_rule_versions.py index 83cc28e..9ebeed4 100644 --- a/tests/test_integration_rule_versions.py +++ b/tests/test_integration_rule_versions.py @@ -57,6 +57,7 @@ async def constraint(): book = await rulebooks_svc.create_rulebook(uid, "History fixtures") topic = await rulebooks_svc.create_topic(book.id, uid, "ci") rule = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic.id, uid, "The runner has no bash", "Write every `run:` step in POSIX sh.", why="the image ships no bash", @@ -258,6 +259,7 @@ async def test_a_version_cannot_be_read_through_a_DIFFERENT_rule(constraint): rule = await s.get(Rule, constraint["rule_id"]) topic_id = rule.topic_id sibling = await rulebooks_svc.create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic_id, constraint["uid"], "A different rule", "Unrelated.", ) diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 6899261..458d826 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -62,6 +62,7 @@ async def test_create_rule_passes_required_fields(): with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", mock), _plain_detail(): from scribe.mcp.tools.rulebooks import create_rule await create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic_id=10, title="dev is home", statement="Work directly on dev", ) kwargs = mock.call_args.kwargs @@ -80,6 +81,7 @@ async def test_create_rule_blocked_by_duplicate_gate(): patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", create_mock): from scribe.mcp.tools.rulebooks import create_rule out = await create_rule(topic_id=10, title="dev is home", statement="x") + when_to_apply="when the moment this fixture stands in for arises", assert out["duplicate"] is True assert out["existing_id"] == 47 assert "update_rule" in out["message"] @@ -95,6 +97,7 @@ async def test_create_rule_force_bypasses_duplicate_gate(): _plain_detail(): from scribe.mcp.tools.rulebooks import create_rule out = await create_rule(topic_id=10, title="dev is home", statement="x", force=True) + when_to_apply="when the moment this fixture stands in for arises", assert out["id"] == 5 find_mock.assert_not_called() @@ -245,6 +248,7 @@ async def test_create_project_rule_passes_required_fields(): with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock), _plain_detail(): from scribe.mcp.tools.rulebooks import create_project_rule await create_project_rule( + when_to_apply="when the moment this fixture stands in for arises", project_id=42, statement="Always run migrations through alembic, not raw SQL.", why="audit trail", @@ -263,6 +267,7 @@ async def test_create_project_rule_derives_title_from_statement(): with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock), _plain_detail(): from scribe.mcp.tools.rulebooks import create_project_rule await create_project_rule( + when_to_apply="when the moment this fixture stands in for arises", project_id=42, statement="Avoid auto-generated docstrings. Reviewers find them noise.", ) @@ -278,6 +283,7 @@ async def test_create_project_rule_uses_explicit_title_when_given(): with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock), _plain_detail(): from scribe.mcp.tools.rulebooks import create_project_rule await create_project_rule( + when_to_apply="when the moment this fixture stands in for arises", project_id=42, statement="anything", title="no auto-docstrings", diff --git a/tests/test_preference_write_path.py b/tests/test_preference_write_path.py index 8ed4b27..6d56e0c 100644 --- a/tests/test_preference_write_path.py +++ b/tests/test_preference_write_path.py @@ -10,6 +10,7 @@ A preference inverts it. The operator's framing: *"preferences are rules that scribe can and should update during use."* A preference that asks every time never drifts, and drifting is the whole feature. Reaching one through `create_rule(kind=...)` would mean reading it through the gate's prose, and +when_to_apply="when the moment this fixture stands in for arises", the caller would hesitate over exactly the act this kind exists to make routine. diff --git a/tests/test_rule_trigger_required.py b/tests/test_rule_trigger_required.py new file mode 100644 index 0000000..be48654 --- /dev/null +++ b/tests/test_rule_trigger_required.py @@ -0,0 +1,119 @@ +"""A rule cannot be created, or edited into, having no trigger (#4099). + +WHY THIS EXISTS + +`when_to_apply` is not metadata. `rule_document` embeds a rule as +`{title} — {trigger}` / `When to apply: {trigger}\n\n{statement}`, with the +trigger appearing TWICE so purpose dominates a short vector — the shape note +2485 measured. Drop the trigger and the document silently becomes title + +statement: a DIFFERENT shape, ranked against a corpus it does not match, with +nothing to report it. Every bar and every rank in the system assumes one shape. + +`create_preference` has always refused an empty trigger. `create_rule` and +`create_project_rule` defaulted it to `""` — so the shape was enforced for the +record kind that merely guides and optional for the kind that binds. + +WHAT THIS PINS + +The guard lives in the SERVICE, because both doors reach it: the MCP tools and +the frontend's fast path in `routes/rulebooks.py`. A guard written in either +one alone would leave the other able to create a rule that never fires, which +is the failure mode this whole change exists to remove. + +These call the service directly and assert on the refusal, so an empty guard +cannot make them pass by accident (rule 167). +""" +from __future__ import annotations + +import re + +import pytest + +from scribe.services import rulebooks as rulebooks_svc + +TRIGGER_RE = re.compile(r"when_to_apply", re.I) + + +@pytest.mark.parametrize("blank", ["", " ", "\n\t "]) +def test_a_rulebook_rule_needs_a_trigger(blank): + """Whitespace is not a trigger — it embeds exactly like an empty one.""" + with pytest.raises(ValueError, match=TRIGGER_RE): + rulebooks_svc._require_trigger(blank) + + +def test_a_real_trigger_passes(): + """The guard has to let the normal case through, or the parametrised + refusals above would pass for a guard that rejects everything.""" + assert rulebooks_svc._require_trigger("before any git push") is None + + +@pytest.mark.asyncio +async def test_create_rule_refuses_before_it_touches_the_database(): + """The guard runs ahead of the ownership check, so a missing trigger is + reported as such rather than as whatever the session lookup says. A bogus + topic_id proves no database was reached: were the guard absent, this would + fail on the topic instead, with a different message.""" + with pytest.raises(ValueError, match=TRIGGER_RE): + await rulebooks_svc.create_rule( + topic_id=999_999, user_id=1, title="t", statement="s", + ) + + +@pytest.mark.asyncio +async def test_create_project_rule_refuses_too(): + """The second door into the same defect. Named separately because the two + functions are separate code paths that have drifted apart before.""" + with pytest.raises(ValueError, match=TRIGGER_RE): + await rulebooks_svc.create_project_rule( + project_id=999_999, user_id=1, title="t", statement="s", + ) + + +def test_both_creators_actually_call_the_guard(): + """Structural, and deliberately not satisfied by the behavioural tests + above: those would still pass if a future edit inlined a second copy of + the check. One definition is the point — a rule shape enforced in two + places is a rule shape that will be enforced differently in two places.""" + import inspect + + for fn in (rulebooks_svc.create_rule, rulebooks_svc.create_project_rule): + src = inspect.getsource(fn) + assert "_require_trigger(when_to_apply)" in src, ( + f"{fn.__name__} no longer delegates to the shared guard" + ) + + +def test_update_cannot_empty_a_trigger_that_exists(): + """The create guard is worth nothing if an edit can undo it, and both + doors can try: `clear=['when_to_apply']` from the MCP side and an emptied + form input from the REST side. + + Asserted on the source because the check sits mid-transaction, after the + mutation and before the commit — reaching it needs a database. What must + not silently vanish is the pairing: the before-value captured, and the + raise that reads it. + """ + import inspect + + src = inspect.getsource(rulebooks_svc.update_rule) + assert "trigger_before" in src, "the pre-edit trigger is no longer captured" + assert re.search(r"if trigger_before and not", src), ( + "update_rule no longer refuses to empty an existing trigger" + ) + + +def test_a_rule_that_never_had_one_is_still_editable(): + """The deliberate asymmetry, and the reason the check asks 'did this edit + REMOVE a trigger' rather than 'does one exist'. + + A rule predating the guard has no trigger. Refusing to save it would + freeze exactly the records that most need fixing — the unreachable ones — + so the guard must not fire when there was nothing to remove. + """ + import inspect + + src = inspect.getsource(rulebooks_svc.update_rule) + assert "trigger_before and not" in src, ( + "the guard no longer keys on the BEFORE value, so a legacy rule with " + "no trigger can no longer be edited at all" + ) diff --git a/tests/test_services_rulebooks.py b/tests/test_services_rulebooks.py index f701f28..52e6b8d 100644 --- a/tests/test_services_rulebooks.py +++ b/tests/test_services_rulebooks.py @@ -140,6 +140,7 @@ async def test_create_rule_requires_owned_topic(): from scribe.services.rulebooks import create_rule with pytest.raises(ValueError, match="topic .* not found"): await create_rule( + when_to_apply="when the moment this fixture stands in for arises", topic_id=999, user_id=7, title="x", statement="y", )