diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index a2060da..87b1569 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -581,11 +581,18 @@ async def update_rule( rule_id: int, title: str = "", statement: str = "", when_to_apply: str = "", why: str = "", how_to_apply: str = "", order_index: int = -1, tier: str = "", system_ids: list[int] | None = None, arose_from_id: int = 0, - verify_with: str = "", expires_when: str = "", + verify_with: str = "", expires_when: str = "", kind: str = "", clear_fields: list[str] | None = None, ) -> dict: """Update a rule. Empty strings / order_index=-1 leave fields unchanged. + `kind` here is how a rule BECOMES a preference, and it is a real change of + force rather than a relabelling — so make it deliberately and say so. The + rule keeps its id, its history and its typed edges, which is why this is a + field rather than a new record: everything that cites it by number stays + correct. Ordinary edits to an existing preference belong in + update_preference, which asks for what taught the change. + Adding `when_to_apply` and a `tier` to an existing rule is the ordinary way a rule stops being preloaded into every session and starts arriving when it is relevant. `system_ids` REPLACES the rule's areas (pass [] to clear). @@ -619,6 +626,8 @@ async def update_rule( fields["when_to_apply"] = when_to_apply if tier: fields["tier"] = tier + if kind: + fields["kind"] = kind if arose_from_id: fields["arose_from_id"] = arose_from_id if why: @@ -639,6 +648,174 @@ async def update_rule( return await rulebooks_svc.rule_detail(uid, rule, system_ids) +# ── Preferences ───────────────────────────────────────────────────────── +# +# Separate tools rather than a `kind=` argument on create_rule, and the reason +# is the docstring rather than the data. create_rule's docstring IS the +# approval gate: it tells its caller to propose, offer three answers, and +# wait. A preference reached through that door would be read through that +# prose, and the caller would hesitate over exactly the act this kind exists +# to make routine. Two doors, two contracts, one table. +# +# Reads stay shared — get_rule and list_rules return preferences as they are, +# because a preference IS a rule row and a reader asking "what governs this" +# wants both. Only the WRITE contracts differ. + + +async def create_preference( + topic_id: int, title: str, statement: str, when_to_apply: str, + arose_from_id: int, why: str = "", how_to_apply: str = "", + order_index: int = 0, force: bool = False, +) -> dict: + """Record how the operator wants work done. No approval loop — write it. + + A PREFERENCE IS NOT A RULE, and the axis is force rather than importance: + + * a RULE is what must be FOLLOWED — ignoring it breaks something or + crosses a boundary. It is the operator's decision, so create_rule + proposes and waits for them. + * a PREFERENCE is how they want it DONE — ignoring it costs consistency, + not correctness. Noticing one and recording it is ordinary work. + + If the answer to "what happens if someone doesn't do this" is "something + breaks", you are holding a rule: propose it with create_rule instead. + + WHY IT IS WORTH RECORDING AT ALL. A preference stated in one session dies + with that session, and the next one re-derives it or asks again. The point + is consistency: the tenth time you do something it goes the way the ninth + did, without the operator having to say so a tenth time. + + `when_to_apply` IS REQUIRED, and not as ceremony. A rule's trigger is + two-thirds of its embedded document, so a preference without one is a + record that will never surface at the moment it applies — written, + findable by nobody, and silently useless. Name the moment in the words a + session would actually be producing then: the command it is about to run, + the code it is writing, the thing the operator just asked for. + + `arose_from_id` IS REQUIRED for the same kind of reason. A preference is + expected to change as the work teaches it, and a corpus that drifts with + no record of what taught each change is one nobody can audit. Point it at + the task or note where this became clear. + + WHAT A PREFERENCE NEVER DOES: change what gets RECORDED. It shapes how + work is done — pacing, phrasing, which tool to reach for, how much to + check first. A dev-log, an issue and a snippet read the same whoever + produced them, because the record has to outlive the person and their + preferences. + + A near-duplicate BLOCKS and returns the existing id. That is the whole + reason this corpus can stay small while being written freely: the second + preference about a thing UPDATES the first rather than sitting beside it, + and two preferences that quietly disagree are worse than none — retrieval + surfaces whichever scores higher and nobody learns the other exists. The + gate is title-based within the topic and does not care about kind, so it + also catches a preference restating a rule that already binds. + + Args: + topic_id: The rulebook topic to file it under. A preference is + user-scoped: it follows the operator across every project, which + is what separates it from a project rule. + title: What the preference is about. Half the embedded document — + worth as much care as the statement. + statement: How the operator wants it done, in their terms. + when_to_apply: The moment it applies. Required; see above. + arose_from_id: The task or note that taught this. Required; see above. + force: Bypass the near-duplicate gate. For a genuinely distinct + preference, not for one that is "mostly" different — a mostly + different preference is an update. + """ + uid = current_user_id() + if not when_to_apply.strip(): + raise ValueError( + "when_to_apply is required: a preference with no trigger never " + "surfaces at the moment it applies. Name that moment in the words " + "a session would be producing then." + ) + if not arose_from_id: + raise ValueError( + "arose_from_id is required: preferences change as the work teaches " + "them, and a change with no record of what taught it cannot be " + "audited. Pass the task or note where this became clear." + ) + if not force: + dup = await dedup_svc.find_duplicate_rule(title, topic_id=topic_id) + if dup is not None: + return dedup_svc.duplicate_response(dup, "rule") + rule = await rulebooks_svc.create_rule( + topic_id=topic_id, user_id=uid, + title=title, statement=statement, when_to_apply=when_to_apply, + kind="preference", arose_from_id=arose_from_id, + why=why, how_to_apply=how_to_apply, order_index=order_index, + ) + return await rulebooks_svc.rule_detail(uid, rule, None) + + +async def update_preference( + rule_id: int, arose_from_id: int, statement: str = "", + when_to_apply: str = "", title: str = "", why: str = "", + how_to_apply: str = "", order_index: int = -1, + system_ids: list[int] | None = None, clear_fields: list[str] | None = None, +) -> dict: + """Bring a preference up to date. Doing this mid-work is expected. + + THIS IS THE TOOL THAT MAKES A PREFERENCE DIFFERENT FROM A RULE. A rule + waits for its author; a preference is kept current by whoever is working. + When the operator corrects you, or you notice the preference on file no + longer matches how they actually want this done, edit it — that is the + feature, not a liberty being taken. A preference nothing ever updates has + become a rule nobody enforces. + + So: no proposal, no three answers, no waiting. Update it and say in the + conversation that you did, so the operator can disagree while it is still + in front of them. + + `arose_from_id` IS REQUIRED, and it is the price of the ungated write. + Every edit here is versioned, and the operator can read what changed and + put it back — but a diff with no reason attached leaves them deciding + whether to trust a change they cannot account for. Point at the task or + note that taught it. + + WHEN NOT TO EDIT. If what you learned is that something MUST be done a + certain way — that skipping it breaks something or crosses a boundary — + that is a rule, and rules are the operator's call: propose it with + create_rule rather than hardening a preference in place. Softening in the + other direction is equally an edit worth flagging out loud. + + Empty strings leave fields unchanged; clear_fields empties them by name, + exactly as update_rule does. + + Args: + rule_id: The preference to update. + arose_from_id: What taught this change. Required; see above. + """ + uid = current_user_id() + if not arose_from_id: + raise ValueError( + "arose_from_id is required: this edit is the record of how the " + "operator's preference changed, and a change with no reason " + "attached cannot be judged. Pass the task or note that taught it." + ) + fields: dict = {"arose_from_id": arose_from_id} + if title: + fields["title"] = title + if statement: + fields["statement"] = statement + if when_to_apply: + fields["when_to_apply"] = when_to_apply + if why: + fields["why"] = why + if how_to_apply: + fields["how_to_apply"] = how_to_apply + if order_index >= 0: + fields["order_index"] = order_index + rule = await rulebooks_svc.update_rule( + rule_id, uid, clear=clear_fields or (), **fields, + ) + if rule is None: + raise ValueError(f"rule {rule_id} not found") + return await rulebooks_svc.rule_detail(uid, rule, system_ids) + + async def rule_history(rule_id: int, version_id: int = 0) -> dict: """What a rule USED TO SAY, newest change first. @@ -985,6 +1162,7 @@ def register(mcp) -> None: list_topics, create_topic, update_topic, delete_topic, list_rules, list_always_on_rules, get_rule, create_rule, create_project_rule, update_rule, delete_rule, + create_preference, update_preference, relate_rules, unrelate_rules, subscribe_project_to_rulebook, unsubscribe_project_from_rulebook, suppress_rule_for_project, unsuppress_rule_for_project, diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index 5f6b77e..7574f5d 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -178,6 +178,11 @@ async def create_rule(topic_id: int): order_index=data.get("order_index", 0), when_to_apply=data.get("when_to_apply", ""), tier=data.get("tier", "always_on"), + # The human door carries `kind` too, and without the MCP door's + # required provenance: an operator editing their own preference + # owes nobody an explanation. That requirement is about auditing + # what the AGENT changed, not what they did themselves. + kind=data.get("kind", "rule"), arose_from_id=data.get("arose_from_id", 0) or 0, verify_with=data.get("verify_with", ""), expires_when=data.get("expires_when", ""), @@ -212,7 +217,7 @@ async def update_rule(rule_id: int): fields = { k: v for k, v in data.items() if k in ("title", "statement", "why", "how_to_apply", "order_index", - "when_to_apply", "tier", "arose_from_id", + "when_to_apply", "tier", "kind", "arose_from_id", "verify_with", "expires_when") } # No clear_fields here: a form sends "" for an emptied input, and the @@ -436,6 +441,11 @@ async def create_project_rule(project_id: int): order_index=data.get("order_index", 0), when_to_apply=data.get("when_to_apply", ""), tier=data.get("tier", "always_on"), + # The human door carries `kind` too, and without the MCP door's + # required provenance: an operator editing their own preference + # owes nobody an explanation. That requirement is about auditing + # what the AGENT changed, not what they did themselves. + kind=data.get("kind", "rule"), arose_from_id=data.get("arose_from_id", 0) or 0, verify_with=data.get("verify_with", ""), expires_when=data.get("expires_when", ""), diff --git a/tests/helpers.py b/tests/helpers.py index c23516c..4510914 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -226,6 +226,12 @@ def fake_rule(**attrs) -> MagicMock: # `when_to_apply` and `arose_from_id` would be truthy MagicMocks and # rule_brief would attach both keys on every stand-in. "when_to_apply": None, "tier": "always_on", "arose_from_id": None, + # Named for the same reason one line up, and it bites harder here. + # `rule_brief` and `to_dict` both emit `kind or "rule"`, and a + # MagicMock is truthy — so an unnamed `kind` would put a MagicMock + # where every payload promises a force, and every stand-in rule would + # read as neither a rule nor a preference. + "kind": "rule", # Same reason, and the same trap one field further on: an unnamed # `verify_with` is a truthy MagicMock, so every stand-in rule would # claim to carry a check and rule_brief would stamp a MagicMock date @@ -235,6 +241,25 @@ def fake_rule(**attrs) -> MagicMock: }, attrs) +def plain_rule_detail(): + """Stub `rulebooks_svc.rule_detail` down to the record's own dict. + + Every rule-tool unit test needs it and none of them wants it: the real + `rule_detail` reads the rule's Systems and its typed edges from the + database, which a unit test has none of. What these tests assert is that + the TOOL forwarded the right arguments, so the seam is stubbed the same + way the create/update calls themselves already are. + + Consolidated here on its second copy, per this module's own reason for + existing (#2825) — two stubs for one seam drift apart quietly, and a test + stubbing the seam slightly differently is a test asserting something + slightly different than it appears to. + """ + async def _detail(_uid, rule, _system_ids=None): + return rule.to_dict() + return patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail", _detail) + + class FakeMCP: """Stand-in for the FastMCP server a tool module's ``register(mcp)`` is handed: records the ``name=`` of every ``@mcp.tool(...)`` registration in diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 8b96917..b5be914 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -3,6 +3,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from tests.helpers import FakeMCP, fake_rule, fake_rulebook, fake_topic +from tests.helpers import plain_rule_detail as _plain_detail pytestmark = pytest.mark.usefixtures("_bind_user") @@ -48,18 +49,10 @@ 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) +# _plain_detail moved to tests/helpers on its second copy (#2825's own +# reason for existing): two stubs for one seam drift apart quietly, and a +# test stubbing it slightly differently asserts something slightly different +# than it appears to. @pytest.mark.asyncio @@ -230,11 +223,18 @@ def test_register_attaches_every_tool(): register(mcp) # 26 through milestone 307, +2 for the staleness sweep (milestone 312), - # +1 for a rule's edit history (milestone 323). - assert len(mcp.names) == 29 + # +1 for a rule's edit history (milestone 323), +2 for preferences + # (milestone 399). + assert len(mcp.names) == 31 # spot-check a few names assert "list_rulebooks" in mcp.names assert "create_rule" in mcp.names + # Preferences get their own WRITE door — create_rule's docstring is the + # approval gate, and a preference reached through it would be read + # through that prose. Reads stay shared deliberately, so there is no + # get_preference to look for here. + assert "create_preference" in mcp.names + assert "update_preference" in mcp.names assert "subscribe_project_to_rulebook" in mcp.names assert "list_always_on_rules" in mcp.names # milestone 297: a project's opt-out of a whole always-on rulebook diff --git a/tests/test_preference_write_path.py b/tests/test_preference_write_path.py new file mode 100644 index 0000000..8ed4b27 --- /dev/null +++ b/tests/test_preference_write_path.py @@ -0,0 +1,225 @@ +"""The preference write path, and how it differs from a rule's (milestone 399). + +WHY TWO DOORS AT ALL + +`create_rule`'s docstring IS the approval gate (#3557): it tells its caller to +propose, offer three answers, and wait. That is right for a rule — the person +a rule binds should have agreed to be bound. + +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 +the caller would hesitate over exactly the act this kind exists to make +routine. + +So the asymmetry is the product, and these tests pin it. + +TWO PRESENCE CHECKS, NEVER AN ABSENCE + +The tempting guard is "create_preference's docstring does NOT run the approval +loop". That is the shape snippet #3352 warns against: an absence check passes +against a docstring that has been deleted, emptied, or rewritten into +something else entirely, and it reads as coverage while proving nothing. + +So the asymmetry is asserted as two PRESENCE facts — the rule door still asks, +the preference door still says write it — and each fails if its own side is +tidied away. Synonym families, structure not wording, the same bargain +test_rule_creation_asks_first strikes. +""" +from unittest.mock import AsyncMock, patch + +import pytest + +from tests.helpers import fake_rule, plain_rule_detail as _plain_detail +from tests.helpers import tool_doc as _doc + +# The tool layer reads its caller from a ContextVar the HTTP transport sets. +# With no request in flight, the module binds it itself (snippet #2836). +pytestmark = pytest.mark.usefixtures("_bind_user") + +MODULE = "scribe.mcp.tools.rulebooks" + + +# ── the required fields, and why each is required ─────────────────────── + + +@pytest.mark.asyncio +async def test_a_preference_without_a_trigger_is_refused(): + """A preference with no `when_to_apply` is inert, not merely incomplete. + + The trigger is two-thirds of the embedded document, so a record without + one never surfaces at the moment it applies. Refusing at the tool is the + difference between an error the writer can fix and a preference that is + written, stored, and silently never delivered — which looks identical to + one nobody wrote. + """ + create_mock = AsyncMock() + with patch(f"{MODULE}.rulebooks_svc.create_rule", create_mock): + from scribe.mcp.tools.rulebooks import create_preference + with pytest.raises(ValueError, match="when_to_apply is required"): + await create_preference( + topic_id=10, title="t", statement="s", + when_to_apply=" ", arose_from_id=42, + ) + create_mock.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_preference_without_provenance_is_refused(): + """Provenance is the price of the ungated write. + + A preference is expected to change as the work teaches it. A corpus that + drifts with no record of what taught each change is one nobody can audit — + and the operator's veto over drift depends entirely on being able to read + why it happened. + """ + create_mock = AsyncMock() + with patch(f"{MODULE}.rulebooks_svc.create_rule", create_mock): + from scribe.mcp.tools.rulebooks import create_preference + with pytest.raises(ValueError, match="arose_from_id is required"): + await create_preference( + topic_id=10, title="t", statement="s", + when_to_apply="when x", arose_from_id=0, + ) + create_mock.assert_not_called() + + +@pytest.mark.asyncio +async def test_create_preference_stores_the_preference_kind(): + """The tool's one irreducible job. + + Asserted on the kwarg reaching the service rather than on the returned + payload: a tool that accepted the call and wrote a plain rule would + return something that reads correctly, and the force would be wrong. + """ + rule = fake_rule(id=100, kind="preference") + create_mock = AsyncMock(return_value=rule) + with patch(f"{MODULE}.rulebooks_svc.create_rule", create_mock), _plain_detail(): + from scribe.mcp.tools.rulebooks import create_preference + await create_preference( + topic_id=10, title="Pace hard debugging", + statement="One step per turn.", + when_to_apply="during hard debugging", + arose_from_id=42, + ) + kwargs = create_mock.call_args.kwargs + assert kwargs["kind"] == "preference" + assert kwargs["arose_from_id"] == 42 + assert kwargs["when_to_apply"] == "during hard debugging" + + +@pytest.mark.asyncio +async def test_a_near_duplicate_preference_blocks(): + """The gate is what lets this corpus be written freely and stay small. + + The second preference about a thing must UPDATE the first. Two that + quietly disagree are worse than none: retrieval surfaces whichever scores + higher, and nobody learns the other exists. + """ + from scribe.services.dedup import DuplicateMatch + dup = DuplicateMatch(id=47, title="Pace hard debugging", similarity=1.0, reason="title") + create_mock = AsyncMock() + with patch(f"{MODULE}.dedup_svc.find_duplicate_rule", AsyncMock(return_value=dup)), \ + patch(f"{MODULE}.rulebooks_svc.create_rule", create_mock): + from scribe.mcp.tools.rulebooks import create_preference + out = await create_preference( + topic_id=10, title="Pace hard debugging", statement="s", + when_to_apply="when", arose_from_id=42, + ) + assert out["duplicate"] is True + assert out["existing_id"] == 47 + create_mock.assert_not_called() + + +@pytest.mark.asyncio +async def test_updating_a_preference_without_provenance_is_refused(): + update_mock = AsyncMock() + with patch(f"{MODULE}.rulebooks_svc.update_rule", update_mock): + from scribe.mcp.tools.rulebooks import update_preference + with pytest.raises(ValueError, match="arose_from_id is required"): + await update_preference(rule_id=5, arose_from_id=0, statement="new") + update_mock.assert_not_called() + + +@pytest.mark.asyncio +async def test_update_preference_forwards_what_taught_the_change(): + rule = fake_rule(id=5, kind="preference") + update_mock = AsyncMock(return_value=rule) + with patch(f"{MODULE}.rulebooks_svc.update_rule", update_mock), _plain_detail(): + from scribe.mcp.tools.rulebooks import update_preference + await update_preference( + rule_id=5, arose_from_id=99, statement="the new way", + ) + kwargs = update_mock.call_args.kwargs + assert kwargs["arose_from_id"] == 99 + assert kwargs["statement"] == "the new way" + + +# ── the asymmetry, as two presence facts ──────────────────────────────── + + +def test_the_rule_door_still_asks_before_writing(): + """Half one of the asymmetry. If this fails, the gate was tidied away and + preferences are no longer the exception — they are just the same thing. + """ + doc = _doc(MODULE, "create_rule").lower() + asks = ("approve", "propose", "ask", "question") + assert any(w in doc for w in asks), ( + "create_rule's docstring no longer runs the propose-then-approve loop. " + "The preference path's whole justification is that it is the exception " + "to this; with the gate gone there is no asymmetry left to justify." + ) + + +def test_the_preference_door_says_to_write_it(): + """Half two. The inverting instruction has to be PRESENT, not merely + unaccompanied by a gate. + + An agent that has internalised #3557 will hesitate to write or rewrite a + preference unless told plainly that this door is different. Silence here + does not read as permission — it reads as an omission, and the caller + falls back on the behaviour it already knows. + """ + for tool in ("create_preference", "update_preference"): + doc = _doc(MODULE, tool).lower() + permits = ("expected", "no approval", "without asking", "ordinary work", + "write it", "not a liberty", "no proposal") + assert any(w in doc for w in permits), ( + f"{tool}'s docstring no longer tells its caller that writing " + "without an approval loop is expected. A caller carrying " + "create_rule's gate will default to asking, and a preference " + "nothing ever updates is a rule nobody enforces." + ) + + +def test_the_preference_door_names_the_force_distinction(): + """The routing test, stated positively (rule 165). + + The confusion this milestone exists to fix is that a session cannot tell + which kind it is holding. If the docstring stops drawing the line, the + tool becomes a second way to write rules. + """ + doc = _doc(MODULE, "create_preference").lower() + assert "rule" in doc and any( + w in doc for w in ("followed", "binds", "breaks", "consistency") + ), ( + "create_preference's docstring no longer distinguishes a preference " + "from a rule by force. Without that line the tool is a second door " + "onto the rulebook with a weaker gate." + ) + + +def test_the_preference_door_keeps_the_record_out_of_scope(): + """Preferences shape HOW work is done, never WHAT is recorded. + + Worth pinning because "record it the way I like it" is the natural next + reach, and it would make dev-logs and issues idiosyncratic per author — + while the record is the one thing that has to outlive the person. + """ + doc = _doc(MODULE, "create_preference").lower() + assert "record" in doc, ( + "create_preference's docstring no longer says that a preference does " + "not change what gets recorded. That boundary is the one a reader " + "would cross without noticing." + )