From c63172272d3a82bb1dfa020d0494a3a638ee19a4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 21:14:29 -0400 Subject: [PATCH 1/5] feat(rules): a preference is a rule that does not bind (#3849 step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `kind` to rules — `rule` binds, `preference` is how the operator wants work done. One column, because the two differ in exactly one dimension and everything else a preference needs already lives on `rules`: a trigger column, a trigger-dominated embedding document, ownership-scoped search, three retrieval arms with telemetry, typed relations, and versioning. Defaults to `rule`, so nothing changes force on upgrade — 0088's argument for `tier`, unchanged. `rule_versions` gets the column too, and that half is not bookkeeping. `record_if_changed` decides whether an edit deserves a snapshot by comparing the fields a version carries, so a field absent from SNAPSHOT_FIELDS is a field whose change records no history at all. Without it, turning a rule into a preference — the moment something stops binding, and the single most consequential edit either kind can undergo — would leave the history silent. Backup carries it through all four seams. A missed one would have restored every preference as a rule, quietly. Guarded on real Postgres in three halves: a preference writes, a typo is refused (without which every other assertion would pass against a table whose CHECK had been dropped), and a row written with no kind reads back as `rule` — the migration's whole safety claim, asserted rather than assumed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- alembic/versions/0098_rule_kind.py | 90 +++++++++++++++ src/scribe/models/rule_version.py | 7 ++ src/scribe/models/rulebook.py | 25 ++++ src/scribe/services/backup.py | 13 ++- src/scribe/services/rule_versions.py | 2 +- src/scribe/services/rulebooks.py | 36 +++++- tests/test_integration_rule_kind.py | 165 +++++++++++++++++++++++++++ 7 files changed, 332 insertions(+), 6 deletions(-) create mode 100644 alembic/versions/0098_rule_kind.py create mode 100644 tests/test_integration_rule_kind.py diff --git a/alembic/versions/0098_rule_kind.py b/alembic/versions/0098_rule_kind.py new file mode 100644 index 0000000..35bb0aa --- /dev/null +++ b/alembic/versions/0098_rule_kind.py @@ -0,0 +1,90 @@ +"""rules gain a `kind` — a preference is a rule that does not bind (#3849) + +Revision ID: 0098 +Revises: 0097 +Create Date: 2026-09-10 + +A rule says what must be followed. There was no way to record the other +thing the operator kept writing rules for: **how they want work done**. +Several rules in a mature rulebook are not really rules — pace this kind of +debugging, hand off an action with its reason, end a finding with an offer. +Ignoring one of those does not break anything or cross a boundary; it costs +consistency. They were written as rules because a rule was the only record +that is global, keyed to a situation, and delivered when that situation +arrives. + +So: `kind`. `rule` binds. `preference` describes how this person wants it +done, and — the part that makes it its own kind rather than a softer label — +**it is expected to change as the work teaches it.** The agent updates a +preference in the ordinary course of working, where a rule waits for its +author. + +WHY A COLUMN AND NOT A TABLE. Everything a preference needs already exists on +`rules` and nowhere else: `when_to_apply` as a real column, a +trigger-dominated embedding document, ownership-scoped search that is +deliberately not filtered to one project, three retrieval arms with per-arm +telemetry, typed relations, and versioning. The two differ in exactly one +dimension — force — and one dimension is a field. + +The drift machinery is the decisive part. `rule_versions` already snapshots +every write, and `rule_relations.overrides` already models "this supersedes +that for its scope". A separate table would have rebuilt both, and moving +existing rows into it would have changed their ids — silently invalidating +every record in the corpus that cites a rule by number. + +DEFAULTS TO `rule`, so this migration changes NOTHING about what binds. An +install upgrades and every existing row keeps the force it had. That is the +same reasoning 0088 used for `tier`, and it is the reason both are safe to +apply without reading the data first. + +The CHECK is created with the column (rule 36: there is no prior constraint, +so the pair is created together — a value added to it LATER does DROP + ADD +in one migration). + +`rule_versions` GETS THE COLUMN TOO, and that half is not bookkeeping. +`record_if_changed` decides whether an edit is worth a snapshot by comparing +the fields a version carries; a field a version does not carry is a field +whose change records NO HISTORY AT ALL. Without this, turning a rule into a +preference — the single most consequential edit either kind can undergo, +because it is the moment something stops binding — would leave the history +silent. Nullable and no CHECK there, matching `tier`: a version is a record +of what was, and a constraint on it would refuse to store a kind later +dropped from the live whitelist. + +Downgrade drops all three. Nothing reads `kind` for correctness; a rule that +was a preference simply becomes a rule again, which is the safe direction. +""" +import sqlalchemy as sa +from alembic import op + + +revision = "0098" +down_revision = "0097" +branch_labels = None +depends_on = None + +# Kept in one place so upgrade and the CHECK agree by construction — 0088's +# idiom, for the same reason. +_KINDS = ("rule", "preference") + + +def _in_list(column: str, values: tuple[str, ...]) -> str: + return f"{column} IN (" + ", ".join(f"'{v}'" for v in values) + ")" + + +def upgrade() -> None: + op.add_column( + "rules", + sa.Column("kind", sa.Text(), nullable=False, server_default="rule"), + ) + op.create_check_constraint("ck_rules_kind", "rules", _in_list("kind", _KINDS)) + # Nullable, no CHECK — see the module docstring. A version written before + # this migration genuinely does not know, and NULL there means "not + # recorded", never "was a rule". + op.add_column("rule_versions", sa.Column("kind", sa.Text(), nullable=True)) + + +def downgrade() -> None: + op.drop_column("rule_versions", "kind") + op.drop_constraint("ck_rules_kind", "rules", type_="check") + op.drop_column("rules", "kind") diff --git a/src/scribe/models/rule_version.py b/src/scribe/models/rule_version.py index a8f8fac..477a186 100644 --- a/src/scribe/models/rule_version.py +++ b/src/scribe/models/rule_version.py @@ -59,6 +59,12 @@ class RuleVersion(Base, CreatedAtMixin): how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) when_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) tier: Mapped[str | None] = mapped_column(Text, nullable=True) + # Carried so that a change of FORCE leaves a trace. `record_if_changed` + # snapshots only the fields a version holds, so a kind omitted here would + # make "this stopped binding" the one edit with no history behind it. + # NULL means a version older than migration 0098 — not recorded, never + # "was a rule". + kind: Mapped[str | None] = mapped_column(Text, nullable=True) verify_with: Mapped[str | None] = mapped_column(Text, nullable=True) expires_when: Mapped[str | None] = mapped_column(Text, nullable=True) @@ -85,6 +91,7 @@ class RuleVersion(Base, CreatedAtMixin): "how_to_apply": self.how_to_apply or "", "when_to_apply": self.when_to_apply or "", "tier": self.tier or "", + "kind": self.kind or "", "verify_with": self.verify_with or "", "expires_when": self.expires_when or "", }) diff --git a/src/scribe/models/rulebook.py b/src/scribe/models/rulebook.py index ac581cb..74dfc8e 100644 --- a/src/scribe/models/rulebook.py +++ b/src/scribe/models/rulebook.py @@ -105,6 +105,24 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): # default preserves existing behaviour exactly: nothing stops binding # because of an upgrade. CHECK ck_rules_tier (migration 0088, rule 36). tier: Mapped[str] = mapped_column(Text, default="always_on", server_default="always_on") + # WHAT KIND of instruction this is — force, where `tier` is delivery. + # `rule` must be FOLLOWED: ignoring it breaks something or crosses a + # boundary. `preference` is how this person wants work DONE: ignoring it + # costs consistency, not correctness. + # + # The second half is what makes it a kind rather than a softer label — a + # preference is expected to CHANGE as the work teaches it, and the agent + # updates it in the ordinary course of working, where a rule waits for its + # author. So one column decides two behaviours: whether create's approval + # gate fires, and which voice the injected line speaks in. + # + # Lives here and not in its own table because a preference needs exactly + # what a rule has and a note does not — a trigger column, a + # trigger-dominated document, ownership-scoped search, the retrieval arms, + # relations, and `rule_versions`, which is where its drift is recorded. + # Defaults to `rule` so nothing changes force on upgrade. + # CHECK ck_rules_kind (migration 0098, rule 36). + kind: Mapped[str] = mapped_column(Text, default="rule", server_default="rule") why: Mapped[str | None] = mapped_column(Text, nullable=True) how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) # The three fields that tell a CONSTRAINT apart from a NORM (milestone @@ -144,6 +162,13 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): "statement": self.statement, "when_to_apply": self.when_to_apply or "", "tier": self.tier, + # Unconditional, unlike the `if present` keys below. A reader + # deciding how much force a record carries must never infer it + # from an ABSENT key: "no kind field" and "kind is rule" would be + # the same payload, and that equivalence is the defect shape this + # codebase keeps re-encountering. Twenty bytes buys an answer that + # cannot be misread. + "kind": self.kind or "rule", "why": self.why or "", "how_to_apply": self.how_to_apply or "", "verify_with": self.verify_with or "", diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 50e9f82..1a8f8d8 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -513,7 +513,7 @@ def _rule_version_rows(rows) -> list[dict]: "id": rv.id, "rule_id": rv.rule_id, "user_id": rv.user_id, "title": rv.title, "statement": rv.statement, "why": rv.why, "how_to_apply": rv.how_to_apply, "when_to_apply": rv.when_to_apply, - "tier": rv.tier, "verify_with": rv.verify_with, + "tier": rv.tier, "kind": rv.kind, "verify_with": rv.verify_with, "expires_when": rv.expires_when, "created_at": rv.created_at.isoformat(), } @@ -575,7 +575,7 @@ def _rule_rows(rows) -> list[dict]: "id": r.id, "topic_id": r.topic_id, "project_id": r.project_id, "title": r.title, "statement": r.statement, "why": r.why, "how_to_apply": r.how_to_apply, "order_index": r.order_index, - "when_to_apply": r.when_to_apply, "tier": r.tier, + "when_to_apply": r.when_to_apply, "tier": r.tier, "kind": r.kind, "verify_with": r.verify_with, "expires_when": r.expires_when, "verified_at": r.verified_at.isoformat() if r.verified_at else None, "arose_from_id": r.arose_from_id, @@ -1283,6 +1283,11 @@ async def _restore_v2(data: dict) -> dict: # is the pre-0088 behaviour, so an old backup restores rules # that bind exactly as they did when it was taken. tier=r_data.get("tier") or "always_on", + # Same shape, same reason: a file written before 0098 has no + # kind, and every rule in it was a rule. Defaulting the other + # way would restore an old backup with things that had always + # bound quietly no longer binding. + kind=r_data.get("kind") or "rule", verify_with=r_data.get("verify_with") or None, expires_when=r_data.get("expires_when") or None, # Restored as-is, NOT reset to null. `verified_at` records @@ -1423,6 +1428,10 @@ async def _restore_v2(data: dict) -> dict: how_to_apply=rv.get("how_to_apply"), when_to_apply=rv.get("when_to_apply"), tier=rv.get("tier"), + # NOT defaulted, unlike the rule above. A version records what + # was; absent means nobody wrote it down, and inventing "rule" + # here would put an artifact where a measurement belongs. + kind=rv.get("kind"), verify_with=rv.get("verify_with"), expires_when=rv.get("expires_when"), created_at=_dt(rv.get("created_at")), diff --git a/src/scribe/services/rule_versions.py b/src/scribe/services/rule_versions.py index 2d905a4..004d481 100644 --- a/src/scribe/services/rule_versions.py +++ b/src/scribe/services/rule_versions.py @@ -36,7 +36,7 @@ from scribe.models.rule_version import RuleVersion # snapshots would bury the edits somebody is actually looking for. SNAPSHOT_FIELDS = ( "title", "statement", "why", "how_to_apply", "when_to_apply", - "tier", "verify_with", "expires_when", + "tier", "kind", "verify_with", "expires_when", ) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index c8e380e..8b1a40e 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -295,6 +295,9 @@ async def _assert_rulebook_rule_owned(session, rule_id: int, user_id: int) -> No # two in step; this keeps the error readable). TIERS = ("always_on", "conditional") RELATION_KINDS = ("co_surfaces", "overrides", "elaborates") +# Migration 0098's CHECK. `rule` binds; `preference` is how the operator +# wants work done — see the model comment for why both live on one table. +KINDS = ("rule", "preference") # The rule columns that are nullable, and therefore the ones where EMPTY has @@ -319,6 +322,22 @@ def _valid_tier(tier: str) -> str: return tier if tier in TIERS else "always_on" +def _valid_kind(kind: str) -> str: + """An unrecognised kind falls back to `rule` — the SAFE direction. + + Same shape as _valid_tier and the same argument, pointed at force instead + of delivery. A preference wrongly treated as binding costs a little + friction: the reader is told something is required that was only + preferred. A rule wrongly treated as a preference costs the thing the rule + was written to prevent, and costs it silently, because nothing downstream + can tell a softened rule from a preference that was always one. + + Between a reader who is too careful and a reader who is not careful + enough, the typo should produce the first. + """ + return kind if kind in KINDS else "rule" + + # Re-exported, not redefined. Notes gained the same trio in milestone 317 and # this reading of it is genuinely common, so it moved to services/verification # — the DRY win note 3163 names, as against sharing the QUERY, which the two @@ -351,6 +370,13 @@ def rule_brief(rule: Rule, **extra) -> dict: "statement": rule.statement, "topic_id": rule.topic_id, "tier": rule.tier, + # Unconditional, and the payload cost is accepted deliberately. Every + # other optional key below is attached only when present, because an + # absent key should never read as a capability the record lacks. Force + # is the opposite case: a reader seeing no `kind` would have to assume + # one, and the assumption it would reach for — "this binds" — is the + # expensive one to get wrong in the other direction. Say it outright. + "kind": rule.kind or "rule", "updated_at": rule.updated_at.date().isoformat() if rule.updated_at else None, } # Attached only when present (#2483: never a null key that reads as a @@ -478,7 +504,7 @@ 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 = "", tier: str = "always_on", arose_from_id: int = 0, - verify_with: str = "", expires_when: str = "", + verify_with: str = "", expires_when: str = "", kind: str = "rule", ) -> Rule: async with async_session() as session: await _assert_topic_owned(session, topic_id, user_id) @@ -488,6 +514,7 @@ async def create_rule( statement=statement, when_to_apply=when_to_apply or None, tier=_valid_tier(tier), + kind=_valid_kind(kind), why=why or None, how_to_apply=how_to_apply or None, verify_with=verify_with or None, @@ -506,7 +533,7 @@ async def create_project_rule( project_id: int, user_id: int, title: str, statement: str, why: str = "", how_to_apply: str = "", order_index: int = 0, when_to_apply: str = "", tier: str = "always_on", arose_from_id: int = 0, - verify_with: str = "", expires_when: str = "", + verify_with: str = "", expires_when: str = "", kind: str = "rule", ) -> Rule: """Create a rule scoped to a single project (no rulebook ceremony). @@ -522,6 +549,7 @@ async def create_project_rule( statement=statement, when_to_apply=when_to_apply or None, tier=_valid_tier(tier), + kind=_valid_kind(kind), why=why or None, how_to_apply=how_to_apply or None, verify_with=verify_with or None, @@ -752,7 +780,7 @@ async def update_rule( return None allowed = { "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", } check_before = rule.verify_with @@ -770,6 +798,8 @@ async def update_rule( continue if key == "tier": value = _valid_tier(value) + elif key == "kind": + value = _valid_kind(value) elif key in NULLABLE_RULE_TEXT: value = value or None elif key == "arose_from_id": diff --git a/tests/test_integration_rule_kind.py b/tests/test_integration_rule_kind.py new file mode 100644 index 0000000..613df6f --- /dev/null +++ b/tests/test_integration_rule_kind.py @@ -0,0 +1,165 @@ +"""Real-Postgres tests for `rules.kind` and its CHECK (0098, #3849 step 1). + +Rule 36 exists because a value and its constraint drift apart: the code +starts writing a new kind while the database still refuses it, and nothing +catches it until a write fails in front of someone. A mock cannot show that — +it has no CHECK — so the constraint gets a real-DB test, exactly as 0091's +`spike` did. + +THREE HALVES, not one. The positive (a preference writes), the negative (a +typo is refused — without it every other assertion here would pass just as +happily against a table whose CHECK was dropped and never re-added), and the +DEFAULT. + +The default is the one worth spelling out, because it is the half that has no +obvious failure. `kind` is NOT NULL with a server default, and the entire +safety argument for this migration is that every existing row keeps the force +it had. A row written without a kind must read back as `rule` — if the server +default did not apply, an upgraded install gets a NOT NULL violation on its +next rule write, or worse, a column that reads as something other than what +every rule in it has always been. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +from scribe.models import async_session +from scribe.models.rulebook import Rule, Rulebook +from scribe.services import rulebooks as rulebooks_svc +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +OWNER_USERNAME = "rule_kind_owner" + + +@pytest_asyncio.fixture +async def topic_id(): + """A topic to hang rules on. + + CLEANED UP AT SETUP, NOT TEARDOWN, for the reason spelled out in + test_integration_rule_versions: the rule write path fires a detached + embedding task that opens its own connection and UPDATEs the row, and a + teardown deleting the rulebook deadlocks against it. Purging at setup runs + on a fresh loop, after the previous test's loop cancelled whatever it left + in flight. + """ + async with async_session() as s: + owner = await ensure_user(s, OWNER_USERNAME) + uid = owner.id + await s.commit() + for book in (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == uid) + )).scalars().all(): + await s.delete(book) + await s.commit() + + book = await rulebooks_svc.create_rulebook(uid, "Kind fixtures") + topic = await rulebooks_svc.create_topic(book.id, uid, "conventions") + return uid, topic.id + + +async def _write_raw(topic: int, **kw) -> int: + """Straight to the model, bypassing `_valid_kind`. + + The service coerces an unrecognised kind to `rule`, which is right for + callers and useless for testing the CHECK — it means no service call can + ever reach the database with a bad value. These tests are about the + constraint, so they go around the coercion. + """ + async with async_session() as s: + rule = Rule(topic_id=topic, title="t", statement="s", **kw) + s.add(rule) + await s.commit() + return rule.id + + +async def test_a_preference_can_be_written(topic_id): + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "preference" + + +async def test_a_rule_still_writes(topic_id): + """0098 widens nothing — it adds a column — but it must not narrow either.""" + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="rule") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_an_unknown_kind_is_refused(topic_id): + """The half that proves the constraint is there at all. + + Without this, every other assertion in this file would pass against a + table whose CHECK had been dropped — the exact failure rule 36 is written + against. + """ + _uid, topic = topic_id + with pytest.raises(IntegrityError): + await _write_raw(topic, kind="suggestion") + + +async def test_a_rule_written_without_a_kind_defaults_to_rule(topic_id): + """The migration's whole safety claim, asserted rather than assumed. + + Every row that existed before 0098 was a rule and must stay one. This is + the closest a test can get to that: write the way code predating the + column would, and read back the force it should have. + """ + _uid, topic = topic_id + rule_id = await _write_raw(topic) + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_a_rule_can_become_a_preference_after_the_fact(topic_id): + """The read-back is the whole test. + + A version asserting only that the update call succeeded would pass against + code that accepted `kind` and dropped it — which is how the same bug + survived on `task_kind` long enough to be found by hand (#3129). + + This is also the migration path the always-on triage needs: the rules that + turn out to be preferences change one column and keep their id, history + and relations. + """ + uid, topic = topic_id + rule_id = await _write_raw(topic) + await rulebooks_svc.update_rule(rule_id, uid, kind="preference") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "preference" + + +async def test_an_unrecognised_kind_falls_back_rather_than_raising(topic_id): + """`_valid_kind` coerces, matching `_valid_tier`, and the direction matters. + + Deliberately NOT the `task_kind` behaviour, which raises a readable + ValueError. A rule's kind falls back to the binding value, because between + a reader who is too careful and one who is not careful enough, a typo + should produce the first. Asserted here so a later "make it consistent + with task_kind" change has to argue with a test rather than a comment. + """ + uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + await rulebooks_svc.update_rule(rule_id, uid, kind="prefrence") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_kind_reaches_both_read_shapes(topic_id): + """`to_dict` and `rule_brief` both carry it, unconditionally. + + Force must never be inferred from an ABSENT key: "no kind field" and + "kind is rule" would be the same payload, and a reader would have to + assume one. Both shapes say it outright, so pin both — the two dicts are + built in different modules and have diverged before. + """ + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + async with async_session() as s: + rule = await s.get(Rule, rule_id) + assert rule.to_dict()["kind"] == "preference" + assert rulebooks_svc.rule_brief(rule)["kind"] == "preference" -- 2.54.0 From 4aae4973f76695c59577312441a46891332d69a2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 21:31:16 -0400 Subject: [PATCH 2/5] fix(tests): the backup stand-ins predate `kind` (#3849 step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unit tests build a rule with SimpleNamespace rather than the model, so adding a column broke them — the fixture has no `kind` for `_rule_rows` to read. Fixture-only; the export itself was already right, which the existing column-coverage guard confirmed by passing. Adds the guard that coverage check cannot make. `_stand_in` walks `__table__.columns` and proves the KEY is emitted; it cannot prove the VALUE survives. A preference exported as a rule is a silent failure — the restored rule reads fine and simply binds when it was only ever a preference — and a fixture carrying the default would pass against a `_rule_rows` that dropped the field and let the importer's `or "rule"` refill it. So the new test asserts on `preference`, the one value that cannot be reconstructed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- tests/test_services_backup.py | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index d85681d..294c7d4 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -388,7 +388,7 @@ def test_rule_rows_carry_the_verification_fields(): row = SimpleNamespace( id=1, topic_id=2, project_id=None, title="t", statement="s", why="w", how_to_apply="h", order_index=0, - when_to_apply="when", tier="conditional", + when_to_apply="when", tier="conditional", kind="rule", verify_with="cat some/file", expires_when="the file grows a shell", verified_at=checked, arose_from_id=99, created_at=checked, updated_at=checked, @@ -415,7 +415,7 @@ def test_rule_rows_keep_an_unverified_rule_unverified(): row = SimpleNamespace( id=1, topic_id=2, project_id=None, title="t", statement="s", why=None, how_to_apply=None, order_index=0, - when_to_apply=None, tier="always_on", + when_to_apply=None, tier="always_on", kind="rule", verify_with=None, expires_when=None, verified_at=None, arose_from_id=None, created_at=datetime(2026, 8, 27, tzinfo=timezone.utc), @@ -426,3 +426,27 @@ def test_rule_rows_keep_an_unverified_rule_unverified(): assert backup._dt_or_none("2026-08-27T12:00:00+00:00") == datetime( 2026, 8, 27, 12, 0, tzinfo=timezone.utc ) + + +def test_rule_rows_carry_the_kind_so_a_preference_does_not_restore_as_a_rule(): + """FORCE has to survive a backup, and the failure would be silent. + + A preference that comes back as a rule is not a missing field anyone would + notice — the rule reads fine, it simply binds when it was only ever meant + to be how the operator prefers things done. Nothing in the restored + rulebook says it used to be softer. + + Asserted with `preference` rather than `rule` on purpose: a fixture + carrying the DEFAULT would pass just as happily against a `_rule_rows` + that dropped the field entirely and let the importer's `or "rule"` fill + the hole back in, which is precisely the bug this guards. + """ + stamp = datetime(2026, 9, 10, tzinfo=timezone.utc) + row = SimpleNamespace( + id=1, topic_id=2, project_id=None, title="t", statement="s", + why=None, how_to_apply=None, order_index=0, + when_to_apply="when", tier="conditional", kind="preference", + verify_with=None, expires_when=None, verified_at=None, + arose_from_id=None, created_at=stamp, updated_at=stamp, + ) + assert backup._rule_rows([row])[0]["kind"] == "preference" -- 2.54.0 From 89d16d89a943f52ca1b64d6d793ffad82a73ec17 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 21:59:23 -0400 Subject: [PATCH 3/5] feat(rules): a preference updates without asking, and says what taught it (#3849 step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write path, and the step where a preference stops being a relabelled rule. `create_preference` / `update_preference` on the MCP surface, plus `kind` on update_rule and both HTTP doors. SEPARATE TOOLS, NOT A `kind=` ARGUMENT. create_rule's docstring IS the approval gate (#3557): propose, offer three answers, wait. That is right for a rule — the person it binds should have agreed. A preference inverts it, and one reached through create_rule would be read through that prose, so the caller would hesitate over exactly the act this kind exists to make routine. Two doors, two contracts, one table. Reads stay shared: a preference IS a rule row, and "what governs this" wants both. Two required fields, each buying something: - `when_to_apply`, because the trigger is two-thirds of the embedded document. Without one the record is written, stored, and silently never delivered — indistinguishable from one nobody wrote. - `arose_from_id`, the price of the ungated write. A corpus that drifts with no record of what taught each change cannot be audited, and the operator's veto over drift is worth exactly as much as their ability to read why it happened. The near-duplicate gate is what lets this corpus be written freely and stay small: the second preference about a thing updates the first. It is title-scoped and kind-blind, so it also catches a preference restating a rule that already binds. The asymmetry is guarded as two PRESENCE facts — the rule door still asks, the preference door still says write it — never as an absence. An absence check passes against a docstring that was deleted or rewritten into something else, which is snippet #3352's warning and would read as coverage here while proving nothing. `_plain_detail` moved to tests/helpers on its second copy, per that module's own reason for existing (#2825). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- src/scribe/mcp/tools/rulebooks.py | 180 +++++++++++++++++++++- src/scribe/routes/rulebooks.py | 12 +- tests/helpers.py | 25 ++++ tests/test_mcp_tool_rulebooks.py | 28 ++-- tests/test_preference_write_path.py | 225 ++++++++++++++++++++++++++++ 5 files changed, 454 insertions(+), 16 deletions(-) create mode 100644 tests/test_preference_write_path.py 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." + ) -- 2.54.0 From 26e0dff706255ef33aec5d6922116665db255d90 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 22:04:14 -0400 Subject: [PATCH 4/5] feat(rules): a preference does not speak in a rule's voice (#3849 step 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent axes on one hint line. `kind` decides the head, `seen` decides the tail, and neither reads the other — which is what let a second kind arrive without reopening #3750's repeat question. Whether a record is already on the exclusion ledger has nothing to do with how much force it carries, so the seen branch is shared verbatim. The noun carries the whole visual difference, deliberately. A reader skimming an injected block gets one word to place the register, so the word that moves is the one naming force: "Standing rule" / "Preference". Everything structural after it is identical, so the kinds read as one set rather than two formats. Force is asserted in exactly one other place, and that moves too. A rule's line says to read it BEFORE DECIDING IT DOES NOT APPLY, because dismissing a rule unread is how the thing it prevents happens. A preference makes no such claim: it says where to find HOW THIS HAS BEEN DONE BEFORE, and following it buys consistency rather than correctness. Guarded on both places at once. Pinning the noun alone would pass a line reading "Preference … before deciding it does not apply" — label swapped, instruction kept — which is worse than not distinguishing them, because it looks handled. And a guard on the independence claim itself, exercising all four combinations: the way this breaks silently is a seen branch that grows a kind test, leaving one combination rendered by nobody's intention. Noted, not fixed: plugin/skills/using-scribe/SKILL.md still says "Standing rules are binding" with no room for a kind that does not. That surface is step 6's. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- src/scribe/services/plugin_context.py | 37 ++++++++++-- tests/test_rule_usage_wiring.py | 87 +++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 1b9d6d2..0a27476 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -859,7 +859,13 @@ async def get_writepath_config(user_id: int) -> dict: } def _rule_hint_line(rule, *, where: str, seen: bool) -> str: - """One rule hint line — both arms, both tails (#3750). + """One rule hint line — both arms, both tails, both kinds (#3750, #3849). + + TWO INDEPENDENT AXES. `kind` decides the head, `seen` decides the tail, + and neither reads the other. A preference and a rule differ in force; a + repeat and a first surfacing differ in whether the session already holds + the line. Those are unrelated facts, and keeping them unrelated in the + code is what stopped the second kind from reopening the repeat question. ONE FUNCTION BECAUSE THE TAILS MUST NOT DRIFT. The two arms phrase their heads differently ("may apply here" vs "may apply to this Bash call") and @@ -891,15 +897,38 @@ def _rule_hint_line(rule, *, where: str, seen: bool) -> str: managing it. """ trigger = (rule.when_to_apply or "").strip() + preference = rule.kind == "preference" + # KIND CHANGES THE HEAD; `seen` CHANGES THE TAIL. The two axes are + # independent and stay that way, which is what lets the repeat logic above + # survive a second kind without being reasoned about again: whether a + # record is already on the ledger has nothing to do with how much force it + # carries, so the seen-branch is shared verbatim. + # + # The noun is the whole of the visual difference, and that is deliberate. + # A reader skimming an injected block gets one word to place the register \u2014 + # so it is the SECOND word that moves, and it is the word naming force. + # Everything structural after it is identical, so the three kinds read as + # one set rather than three formats (milestone 385's step 5 writes the + # lesson voice against these two; they are one paragraph, not three). + noun = "Preference" if preference else "Standing rule" + # The only other place force is asserted. A rule's line tells the reader + # not to dismiss it unread, because dismissing a rule unread is how the + # thing it prevents happens. A preference makes no such claim: it says + # where to find how this has been done, and following it is what keeps + # things consistent rather than what keeps them correct. + reason = ( + "for how this has been done before" if preference + else "before deciding it does not apply" + ) tail = ( f"You saw it earlier this session; pull it with get_rule({rule.id}) " "if you no longer hold it." if seen else - f"Read it with get_rule({rule.id}) before deciding it does not " - "apply; it is not in this session's loaded set." + f"Read it with get_rule({rule.id}) {reason}; it is not in this " + "session's loaded set." ) return ( - f"Standing rule that may apply {where} \u2014 \u201c{rule.title}\u201d" + f"{noun} that may apply {where} \u2014 \u201c{rule.title}\u201d" + (f" ({trigger})" if trigger else "") + f". {tail}" ) diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 462c113..6eaa975 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -1006,6 +1006,93 @@ async def test_the_two_tails_are_distinguishable_and_say_the_true_one(source, ru assert fresh_ctx != seen_ctx, "the two tails collapsed into one" +# ── the third axis: KIND (milestone 399) ──────────────────────────────── +# +# A preference is a rule row that does not bind, so it rides the same arms and +# the same line. What must differ is the REGISTER: a rule's line tells the +# reader not to dismiss it unread, because dismissing a rule unread is how the +# thing it prevents happens. A preference makes no such claim — it says where +# to find how this has been done, and following it buys consistency rather +# than correctness. +# +# Rendered in the rule's voice, a preference becomes the thing milestone 399 +# exists to avoid: a rule with a different column. + +_PREF = fake_rule( + id=177, + kind="preference", + title="Pace hard debugging one step at a time", + statement="Advance one investigative step per turn.", + when_to_apply="during hard debugging", +) +_RULE_FORCE = "before deciding it does not apply" +_PREF_FORCE = "for how this has been done before" + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_a_preference_does_not_speak_in_the_rules_voice(source, run): + """The register, pinned on the two places force is actually asserted. + + On BOTH the noun and the clause, because either alone is weak. A line + reading "Preference … before deciding it does not apply" has swapped the + label and kept the instruction, which is worse than not distinguishing + them at all: it looks handled. + """ + ctx = (await run([(0.81, _PREF)], MagicMock()))["context"] + + assert "Preference that may apply" in ctx, ( + f"{source} announced a preference as something else. The noun is the " + f"one word a skimming reader gets to place the register, so it is the " + f"word that has to move. Context was: {ctx!r}" + ) + assert _PREF_FORCE in ctx and _RULE_FORCE not in ctx, ( + f"{source} told the session to read a PREFERENCE before deciding it " + f"does not apply. That is a rule's claim: it is the sentence that " + f"makes a line bind, and a preference does not." + ) + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_kind_and_seen_do_not_read_each_other(source, run): + """The structural claim the design rests on: two INDEPENDENT axes. + + Whether a record is already on the exclusion ledger has nothing to do with + how much force it carries. Keeping the two unrelated in the code is what + let a second kind arrive without reopening #3750's repeat question — and + the way that silently breaks is a `seen` branch that grows a kind test, + or a kind branch that grows a `seen` test, leaving one of the four + combinations rendered by nobody's intention. + + So: all four are exercised, and the seen tail must come out identical for + both kinds. + """ + pref_fresh = (await run([(0.81, _PREF)], MagicMock()))["context"] + pref_seen = (await run([(0.81, _PREF)], MagicMock(), + exclude_rule_ids=[177]))["context"] + rule_seen = (await run([(0.81, _HELD)], MagicMock(), + exclude_rule_ids=[156]))["context"] + + assert _SEEN_TAIL in pref_seen and _FRESH_TAIL in pref_fresh, ( + f"{source}: the seen/fresh split stopped working once kind was added — " + f"the tail axis is now reading the head axis" + ) + # The tail is about the LEDGER, so it is shared verbatim. Compared as the + # tail alone rather than the whole line, since the heads differ by design. + assert _SEEN_TAIL in rule_seen, "the rule's seen tail changed" + assert pref_seen.count(_SEEN_TAIL) == rule_seen.count(_SEEN_TAIL) == 1, ( + f"{source} rendered the repeat clause a different number of times for " + f"the two kinds; the tail is about the ledger and does not vary by force" + ) + # And the head still differs in the seen case — a repeat of a preference + # is still a preference. + assert "Preference that may apply" in pref_seen, ( + f"{source} lost the preference register on a repeat, so a preference " + f"seen twice reads as a rule the second time" + ) + + @pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) @pytest.mark.asyncio async def test_neither_tail_injects_the_rule_statement(source, run): -- 2.54.0 From 8406871085ffc5b6a4656f0fe7ac3632f9732101 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 23:20:04 -0400 Subject: [PATCH 5/5] fix(plugin): the instruction surfaces still said every rule binds (#3849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 3 shipped a line a session can receive — "Preference that may apply here …" — into surfaces that told it, in the most authoritative voice it has, that anything arriving in that shape is binding. That is the confusion milestone 399 exists to prevent, arriving through the one channel a session has least reason to doubt. Silent in both directions, which is why it could not wait for step 6. A session treating a preference as a rule refuses to proceed over something the operator merely preferred; and it loses the whole reason preferences exist, which is that they are brought up to date rather than obeyed. Three surfaces, each to its own budget: - SKILL.md gets the full account: kind decides force, the injected line names which in its opening words, and a preference is the one record a session keeps current itself (update_preference, with what taught the change). - scribe_static_context.md gets six lines — enough to tell the kinds apart and to say a preference is yours to update. - _INSTRUCTIONS gets four words. It is a MAP at 1978 of its 2000-char budget (#2562), and the detail belongs in the surfaces above and in the tool docstrings, which is what that budget exists to force. Guarded so it cannot drift back: a surface that claims rules bind must name the kind that does not. Pinned on the CLAIM rather than the word "bind", because a bare substring also matches bind_repo, list_repo_bindings and server.py's DNS-rebinding comment — a guard that would one day fail a skill about repo binding is rule 167's named failure, raising a false alarm about the very thing it protects. Falsified against all three surfaces losing the mention. Plugin version minted: the cache refreshes only on a version bump (#2209), so a skill edit without one reaches no installed plugin. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_static_context.md | 8 +++- plugin/skills/using-scribe/SKILL.md | 24 ++++++++++-- src/scribe/mcp/server.py | 5 ++- tests/test_instruction_surfaces_agree.py | 47 ++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 7 deletions(-) diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 59a9fdd..8e55c7c 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "2026.09.10.0221", + "version": "2026.09.11.0319", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_static_context.md b/plugin/hooks/scribe_static_context.md index 5a6c5fc..258a670 100644 --- a/plugin/hooks/scribe_static_context.md +++ b/plugin/hooks/scribe_static_context.md @@ -6,7 +6,7 @@ of record (notes, tasks, projects, milestones, rules) reachable through the for the operator's work, and as your own working memory across sessions. **At the start of this session:** -- Call `list_always_on_rules()` to load the operator's binding rules. +- Call `list_always_on_rules()` to load the operator's standing rules. - If the working repo maps to a Scribe project (check `list_repo_bindings`), call `enter_project()` to load that project's rules, open tasks, and recent notes in one shot. @@ -21,6 +21,12 @@ for the operator's work, and as your own working memory across sessions. compaction — call `list_always_on_rules()` (and `enter_project()` when a project is in scope) BEFORE acting. When a loaded rule and a default habit disagree, the rule wins; if no rule speaks to it, ask rather than assume. +- **Rules bind; preferences do not.** A record's `kind` says which. A **rule** + must be followed — ignoring it breaks something or crosses a boundary. A + **preference** is how the operator wants work done: worth following for + consistency, not a defect to miss. Injected lines name the kind in their + opening words. A preference is also yours to keep current when they correct + you (`update_preference`); a rule waits for them. - **What you loaded is not all of the rules.** Only the always-on tier arrives that way; conditional rules are RETRIEVED, and one you were never handed binds exactly as hard. So before a consequential act, `search` for a rule diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index fffa5ba..93d0611 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -58,9 +58,27 @@ Two constraints on *how* that's achieved: 2. **Standing rules are binding — and the ones you were handed are not all of them.** Load the resident set via `list_always_on_rules()` at session start - (see "Do this first"); treat every one as binding. Pull a rule's full - statement with `get_rule(id)` when it's about to bite. When a project is in - scope, `enter_project(id)` also returns its applicable rules. + (see "Do this first"). Pull a record's full statement with `get_rule(id)` + when it's about to bite. When a project is in scope, `enter_project(id)` + also returns its applicable rules. + + **`kind` says how much force a record carries, and it is never something to + infer.** A **rule** must be followed: ignoring it breaks something or + crosses a boundary. A **preference** records how the operator wants work + done, and ignoring it costs consistency rather than correctness. Both are + worth following and both arrive the same way; only one is a mistake to + miss. An injected line names which in its opening words — *"Standing rule + that may apply…"* against *"Preference that may apply…"* — and every + payload carries `kind` outright. + + **A preference is the one record you keep current yourself.** When the + operator corrects you, or the preference on file no longer matches how they + actually want something done, `update_preference` — that is expected, not a + liberty, and it wants the task or note that taught the change. Say in the + same turn that you did it, so they can disagree while it is in front of + them. A rule waits for the operator instead: `create_rule` proposes and + asks. If what you learned is that something MUST be done a certain way, + that is a rule to propose, not a preference to harden in place. Rules come in two tiers. **Always-on** rules are delivered — they arrive whether or not you ask. **Conditional** rules are RETRIEVED, and one binds diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index b37b5d4..0f9ee1b 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -88,8 +88,9 @@ Hierarchy: Project -> Milestone -> Task/Note. The map, by purpose: active project_id to stay in scope. - WHERE work happens: Systems. Tag records with system_ids as you write; create_system when the area is unmodelled. -- HOW: rules bind. list_always_on_rules() at start; before a consequential - act, search(content_type="rule") — the resident set is not all of them. +- HOW: rules bind; preferences guide. list_always_on_rules() at start; + before a consequential act, search(content_type="rule") — the resident + set is not all of them. - UI: the project's design system is binding — resolve_design_system / get_design_system_stylesheet before hand-writing a value. - REUSE: search snippets before writing a helper; record what you build with diff --git a/tests/test_instruction_surfaces_agree.py b/tests/test_instruction_surfaces_agree.py index 4495650..6bd4ad6 100644 --- a/tests/test_instruction_surfaces_agree.py +++ b/tests/test_instruction_surfaces_agree.py @@ -252,3 +252,50 @@ def test_no_surface_names_the_push_without_stating_the_pull(): f"the bridge — it can be absent without saying so. Name it if it helps, " f"but say to call {PULL}() regardless." ) + + +# ── force: a surface that says rules bind must say what does not ──────── +# +# Added with the preference kind (milestone 399). Before it, "rules bind" was +# the whole truth and every surface said so flatly. It is now half of one, and +# the half that is missing is the dangerous half to omit: a session reading +# only "rules bind" and then receiving a preference has been told, by the most +# authoritative surface it has, to treat it as binding. +# +# That failure is silent in both directions. Treating a preference as a rule +# produces a session that refuses to proceed over something the operator only +# preferred; and it removes the reason preferences exist, which is that they +# can be brought up to date rather than obeyed. +# +# Same bargain as every test in this file: STRUCTURE, not wording. A surface +# passes by mentioning the other kind at all, so the prose stays free. +# +# PINNED ON THE CLAIM, NOT THE WORD "bind". A bare substring also matches +# `bind_repo`, `list_repo_bindings` and the DNS-rebinding comment in +# server.py — so it would one day fail a skill that mentions repo binding and +# has nothing to do with force, which is rule 167's named failure: a guard +# raising a false alarm about the very thing it protects. These phrases are +# the ones that actually assert bindingness to a reader. +BINDING_CLAIMS = ( + "rules bind", + "binding rules", + "rules are binding", + "treat every one as binding", +) +OTHER_KIND = "preference" + + +def test_a_surface_claiming_rules_bind_also_names_what_does_not(): + offenders = [ + label for label, text in _all_surfaces() + if any(c in text.lower() for c in BINDING_CLAIMS) + and OTHER_KIND not in text.lower() + ] + assert not offenders, ( + f"these surfaces tell a session that rules bind and never mention " + f"preferences: {offenders}. A preference arrives through the same arms " + f"and renders in the same line shape, so a surface that describes only " + f"the binding kind is read as covering both — and the session treats " + f"'how the operator likes this done' as something it may not proceed " + f"past. Name the other kind, however briefly." + ) -- 2.54.0