feat(rules): both doors carry the trigger, the tier, the areas and the edges (#3029, milestone 307 step 3, surfaces)
CI & Build / Python lint (push) Successful in 5s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / integration (push) Successful in 21s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m7s
CI & Build / Build & push image (push) Successful in 24s
CI & Build / Python lint (push) Successful in 5s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / integration (push) Successful in 21s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m7s
CI & Build / Build & push image (push) Successful in 24s
MCP and REST both gain when_to_apply / tier / system_ids / arose_from_id on create and update, plus relate_rules / unrelate_rules for the typed edges, and get_rule now returns a rule's areas and relations alongside it. rule_detail() is a SERVICE function, not one per door. It started as a copy in each — identical, and the prior-art hook flagged it immediately, which is the same lesson rules_payload (#2858) already recorded: a second copy drifts. Both doors call the one seam, so create, update and get cannot disagree about what a rule looks like coming back. The authoring guidance lands in create_rule's docstring rather than in a rule, per rule 119 as the operator described it: this is behaviour every instance should inherit, not one operator's preference. It states the test — ONE RULE = ONE THING YOU COULD VIOLATE. Rules that FAIL TOGETHER get linked with relate_rules(kind="co_surfaces"), never merged into one row. — and names why merging loses: a merged rule cannot be cited, surfaced or suppressed a clause at a time, and it grows without limit because adding to it is always cheaper than adding a rule. create_project_rule says the same about "overrides", which is what FabledCurator's 85/86 should have been instead of near-copies that drift from their parent. The tier arg carries the test itself: can you name the trigger WITHOUT naming a system, an artifact type or a moment? If the honest answer is "whenever you are working", it is always_on. Tests: the applicable-rules cases fabricated raw tuples matching the old column lists, so they move to the entity shape via fake_rule; new cases pin rule_brief (a DATE not a stamp, the depth left to get_rule, no null keys) and that an unknown tier falls back to BINDING. fake_rule gains when_to_apply / tier / arose_from_id for the note-2109 reason the helper exists: unnamed, they would be truthy MagicMocks. The tool tests stub the new rule_detail seam — they are about argument forwarding and have no database. The module header's "Sixteen tools" had been wrong for two milestones; the registration count test is what actually catches that, so the header now says so instead of carrying a number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
"""Tests for MCP rulebook tools — patches the service layer."""
|
||||
from unittest.mock import AsyncMock, patch
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from tests.helpers import FakeMCP, fake_rule, fake_rulebook, fake_topic
|
||||
@@ -48,11 +48,25 @@ async def test_get_rulebook_raises_when_not_found():
|
||||
await get_rulebook(rulebook_id=999)
|
||||
|
||||
|
||||
def _plain_detail():
|
||||
"""Stub the rule_detail seam these tool tests are not about.
|
||||
|
||||
create/update/get_rule now return through services.rulebooks.rule_detail,
|
||||
which reads the rule's areas and edges from the database. These are unit
|
||||
tests with no database, and what they assert is that the TOOL forwards the
|
||||
right arguments — so the seam is stubbed to the plain record, the same way
|
||||
they already stub the create/update calls themselves.
|
||||
"""
|
||||
async def _detail(_uid, rule, _system_ids=None):
|
||||
return rule.to_dict()
|
||||
return patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail", _detail)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_rule_passes_required_fields():
|
||||
rule = fake_rule(id=100, title="r", statement="s", topic_id=10)
|
||||
mock = AsyncMock(return_value=rule)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", mock):
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", mock), _plain_detail():
|
||||
from scribe.mcp.tools.rulebooks import create_rule
|
||||
await create_rule(
|
||||
topic_id=10, title="dev is home", statement="Work directly on dev",
|
||||
@@ -84,7 +98,8 @@ async def test_create_rule_force_bypasses_duplicate_gate():
|
||||
find_mock = AsyncMock()
|
||||
with patch("scribe.mcp.tools.rulebooks.dedup_svc.find_duplicate_rule", find_mock), \
|
||||
patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule",
|
||||
AsyncMock(return_value=fake_rule(id=5, title="r", statement="s", topic_id=10))):
|
||||
AsyncMock(return_value=fake_rule(id=5, title="r", statement="s", topic_id=10))), \
|
||||
_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)
|
||||
assert out["id"] == 5
|
||||
@@ -95,7 +110,7 @@ async def test_create_rule_force_bypasses_duplicate_gate():
|
||||
async def test_update_rule_only_sends_non_default_fields():
|
||||
rule = fake_rule(id=100, title="r", statement="s", topic_id=10)
|
||||
mock = AsyncMock(return_value=rule)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule", mock):
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule", mock), _plain_detail():
|
||||
from scribe.mcp.tools.rulebooks import update_rule
|
||||
await update_rule(rule_id=1, statement="new statement")
|
||||
args, kwargs = mock.call_args
|
||||
@@ -169,7 +184,7 @@ def test_register_attaches_all_sixteen_tools():
|
||||
mcp = FakeMCP()
|
||||
|
||||
register(mcp)
|
||||
assert len(mcp.names) == 24 # +exclude/include_always_on_rulebook (milestone 297)
|
||||
assert len(mcp.names) == 26 # +relate_rules/unrelate_rules (milestone 307)
|
||||
# spot-check a few names
|
||||
assert "list_rulebooks" in mcp.names
|
||||
assert "create_rule" in mcp.names
|
||||
@@ -239,7 +254,7 @@ async def test_update_rulebook_omits_always_on_when_none():
|
||||
async def test_create_project_rule_passes_required_fields():
|
||||
rule = fake_rule(id=100, title="r", statement="s", topic_id=10)
|
||||
mock = AsyncMock(return_value=rule)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock):
|
||||
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(
|
||||
project_id=42,
|
||||
@@ -257,7 +272,7 @@ async def test_create_project_rule_passes_required_fields():
|
||||
async def test_create_project_rule_derives_title_from_statement():
|
||||
rule = fake_rule(id=100, title="r", statement="s", topic_id=10)
|
||||
mock = AsyncMock(return_value=rule)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock):
|
||||
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(
|
||||
project_id=42,
|
||||
@@ -272,7 +287,7 @@ async def test_create_project_rule_derives_title_from_statement():
|
||||
async def test_create_project_rule_uses_explicit_title_when_given():
|
||||
rule = fake_rule(id=100, title="r", statement="s", topic_id=10)
|
||||
mock = AsyncMock(return_value=rule)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock):
|
||||
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(
|
||||
project_id=42,
|
||||
@@ -325,3 +340,47 @@ async def test_unsuppress_topic_for_project_passes_through():
|
||||
kwargs = mock.call_args.kwargs
|
||||
assert kwargs == {"project_id": 3, "topic_id": 22, "user_id": 7}
|
||||
assert out == {"project_id": 3, "topic_id": 22, "suppressed": False}
|
||||
|
||||
|
||||
# ── Typed edges between rules (milestone 307) ───────────────────────────
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_relate_rules_forwards_the_kind_and_the_why():
|
||||
"""The edge exists so a shape stops being merged into one row. The `note`
|
||||
travels with it for the same reason a rule carries `why`: whoever later
|
||||
decides whether the edge still holds needs the reasoning."""
|
||||
relation = MagicMock()
|
||||
relation.to_dict.return_value = {"id": 9, "kind": "co_surfaces"}
|
||||
mock = AsyncMock(return_value=relation)
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.add_rule_relation", mock):
|
||||
from scribe.mcp.tools.rulebooks import relate_rules
|
||||
out = await relate_rules(
|
||||
from_rule_id=46, to_rule_id=144, kind="co_surfaces",
|
||||
note="a stale channel tag and an unparseable version both read as "
|
||||
"no update available",
|
||||
)
|
||||
assert out["id"] == 9
|
||||
args = mock.call_args.args
|
||||
assert args[0] == 7 and args[1] == 46 and args[2] == 144
|
||||
assert args[3] == "co_surfaces"
|
||||
assert "no update available" in args[4]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_relate_rules_raises_when_either_end_is_not_yours():
|
||||
"""The service returns None when it cannot see both rules — a one-sided
|
||||
edge would surface a rule the caller has no business reading."""
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.add_rule_relation",
|
||||
AsyncMock(return_value=None)):
|
||||
from scribe.mcp.tools.rulebooks import relate_rules
|
||||
with pytest.raises(ValueError, match="not found"):
|
||||
await relate_rules(from_rule_id=1, to_rule_id=2, kind="overrides")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unrelate_rules_raises_when_the_edge_is_gone():
|
||||
with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.remove_rule_relation",
|
||||
AsyncMock(return_value=False)):
|
||||
from scribe.mcp.tools.rulebooks import unrelate_rules
|
||||
with pytest.raises(ValueError, match="not found"):
|
||||
await unrelate_rules(relation_id=99)
|
||||
|
||||
Reference in New Issue
Block a user