From 3d4f5be7110fe18efcd44bb923d3bcf8d39b8df2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:44:13 -0400 Subject: [PATCH 1/8] =?UTF-8?q?feat(rules):=20a=20rule=20can=20carry=20its?= =?UTF-8?q?=20own=20check=20=E2=80=94=20verify=5Fwith,=20expires=5Fwhen,?= =?UTF-8?q?=20verified=5Fat=20(#3095,=20milestone=20312=20step=201)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rulebook holds two kinds of row in one table. A NORM is a decision: no truth value, changes only when its author changes it, and they know they did. A CONSTRAINT asserts a fact about someone else's software, and goes false with nobody present. Milestone 307's audit found nine stale sites; every one was a constraint, and not one norm had rotted. Three nullable columns so a rule can say how to check itself. expires_when is a STATE, not a date — constraints expire when the ground moves, not on a schedule. verified_at NULL means never checked and sorts FIRST in the sweep to come: unexamined outranks examined-long-ago. Most rules set none of the three; a null verify_with is the marker for "this is a decision, there is nothing to go and check," and it only reads that way while it stays honest. Nothing is backfilled and nothing is indexed. A migration cannot invent a check any more than 0088 could invent a trigger, and the sweep reads a whole rulebook — hundreds of rows, on operator demand, never on a request path. Also, in the backup service the fields had to pass through: - Restore now remaps arose_from_id through note_id_map. It has been exported since 0088 and silently dropped on the way back in ever since, so every restore lost every rule's provenance link. - _dt_or_none, because _dt substitutes now() for an absent value. That is right for created_at/updated_at and wrong here: a rule nobody ever checked would restore looking freshly checked and fall to the bottom of the sweep it should top. Column additions do not move BACKUP_VERSION; only new sections do, as when 0088 added when_to_apply/tier/arose_from_id to the same helper. Co-Authored-By: Claude Opus 5 (1M context) --- alembic/versions/0090_rule_verification.py | 64 ++++++++++++++++++++ src/scribe/models/rulebook.py | 23 ++++++++ src/scribe/services/backup.py | 27 +++++++++ tests/test_services_backup.py | 68 ++++++++++++++++++++-- 4 files changed, 177 insertions(+), 5 deletions(-) create mode 100644 alembic/versions/0090_rule_verification.py diff --git a/alembic/versions/0090_rule_verification.py b/alembic/versions/0090_rule_verification.py new file mode 100644 index 0000000..0ba64a9 --- /dev/null +++ b/alembic/versions/0090_rule_verification.py @@ -0,0 +1,64 @@ +"""a rule can carry its own check — verify_with, expires_when, verified_at +(milestone 312 step 1) + +Revision ID: 0090 +Revises: 0089 +Create Date: 2026-08-27 + +A rulebook holds two kinds of row in one table. A NORM is a decision: it has +no truth value, and it changes only when its author changes it — which they +know they did. A CONSTRAINT is a fact about someone else's software: a +runner's shell, a bot's config, a tool that exists. Nobody is present when +that goes false. + +Milestone 307's rulebook audit found nine stale sites. Every one was a +constraint; not one norm had rotted. One of them had been telling every +session to skip database-backed tests for weeks while the integration lane +sat green in the workflow. + +Three nullable columns, so a rule can say how to check itself: + +- `verify_with` — how to tell whether this is still true. A command, a path, + a URL, a query. Prose is allowed; something runnable is better. +- `expires_when` — the STATE under which it stops being true. Deliberately + not a date: constraints do not expire on a schedule, they expire when the + world underneath them moves. +- `verified_at` — when the check last passed. NULL means never checked, and + sorts FIRST in the sweep: unexamined outranks examined-long-ago. + +All three nullable and all three optional, because most rules should set +none of them. A null `verify_with` is not an omission — it is the honest +marker of "this one is a decision, and there is nothing to go and check." +That signal only works if the field stays empty wherever it belongs empty. + +No CHECK constraint is involved, so rule 36 does not apply here. Nothing is +backfilled: a migration cannot invent a check any more than 0088 could +invent a trigger. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0090" +down_revision = "0089" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("rules", sa.Column("verify_with", sa.Text(), nullable=True)) + op.add_column("rules", sa.Column("expires_when", sa.Text(), nullable=True)) + op.add_column( + "rules", + sa.Column("verified_at", sa.DateTime(timezone=True), nullable=True), + ) + # No index on (verify_with, verified_at). The sweep this exists for reads + # an operator's whole rulebook — hundreds of rows, not millions — and runs + # when a human asks for it, never on a request path. An index here would + # be maintained on every rule write to serve a query that a sequential + # scan answers instantly. + + +def downgrade() -> None: + op.drop_column("rules", "verified_at") + op.drop_column("rules", "expires_when") + op.drop_column("rules", "verify_with") diff --git a/src/scribe/models/rulebook.py b/src/scribe/models/rulebook.py index d05da94..ac581cb 100644 --- a/src/scribe/models/rulebook.py +++ b/src/scribe/models/rulebook.py @@ -107,6 +107,26 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): tier: Mapped[str] = mapped_column(Text, default="always_on", server_default="always_on") 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 + # 312). A norm is a decision — no truth value, changes only when its + # author changes it. A constraint asserts a fact about someone else's + # software, and goes false with nobody watching: every stale rule the + # 307 audit found was one, and no norm had rotted. + # + # `verify_with` is how to check the rule is still true; `expires_when` is + # the STATE that ends it, deliberately not a date — constraints expire + # when the ground moves, not on a schedule. `verified_at` NULL means + # never checked, and sorts FIRST in the sweep: unexamined outranks + # examined-long-ago. + # + # Most rules should leave all three empty. A null `verify_with` is not a + # gap — it is the marker for "this is a decision, there is nothing to go + # and check," and the signal is only worth reading while that stays true. + verify_with: Mapped[str | None] = mapped_column(Text, nullable=True) + expires_when: Mapped[str | None] = mapped_column(Text, nullable=True) + verified_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) # The record that caused this rule — the edge notes and tasks already # have. Rule 46's `why` names note 2813 in prose; this is that link as a # field, so it survives a rewording of the paragraph. @@ -126,6 +146,9 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): "tier": self.tier, "why": self.why or "", "how_to_apply": self.how_to_apply or "", + "verify_with": self.verify_with or "", + "expires_when": self.expires_when or "", + "verified_at": iso(self.verified_at), "arose_from_id": self.arose_from_id, "order_index": self.order_index, "created_at": iso(self.created_at), diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 7140da1..dd23826 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -112,6 +112,18 @@ def _dt(val: str | None) -> datetime: return datetime.fromisoformat(val) if val else datetime.now(timezone.utc) +def _dt_or_none(val: str | None) -> datetime | None: + """Like _dt, but keeps an absent timestamp absent. + + _dt substitutes now() because created_at/updated_at must not be null. + For a nullable column that MEANS something by being empty, that default + is a lie: a rule nobody ever verified would restore looking verified at + the moment of the restore, and drop straight to the bottom of the sweep + it should have topped. + """ + return datetime.fromisoformat(val) if val else None + + def _d(val: str | None) -> date | None: return date.fromisoformat(val) if val else None @@ -385,6 +397,8 @@ def _rule_rows(rows) -> list[dict]: "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, + "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, "created_at": r.created_at.isoformat(), "updated_at": r.updated_at.isoformat(), @@ -1007,6 +1021,19 @@ 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", + 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 + # when someone last ran the check; a restore does not make + # that untrue, and clearing it would put every constraint at + # the top of the sweep with nothing having actually changed. + verified_at=_dt_or_none(r_data.get("verified_at")), + # Remapped through note_id_map like every other note edge. + # Exported since 0088 but dropped on the way back in until + # milestone 312 — a restore silently lost every rule's + # provenance link. SET NULL semantics apply here too: a + # source note that didn't restore leaves the rule intact. + arose_from_id=note_id_map.get(r_data.get("arose_from_id") or 0), order_index=r_data.get("order_index", 0), created_at=_dt(r_data.get("created_at")), updated_at=_dt(r_data.get("updated_at")), diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 3eaac1c..a5d78bc 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -1,10 +1,13 @@ -"""Unit tests for the v4 backup export contract. +"""Unit tests for the backup export contract. -CI runs pytest with no database, so these cover the parts that don't need one: -the version/coverage constants, the pure join-table row helpers, and the export -dict shape (via a mocked session). Full FK-remapping round-trip is exercised -manually against a real DB (export a backup, confirm rulebooks appear). +This is the no-database lane, so these cover the parts that need none: the +version/coverage constants, the pure row helpers, and the export dict shape +(via a mocked session). The full FK-remapping round-trip needs real Postgres +and belongs in a `@pytest.mark.integration` module — it is not written yet, +which is why every row helper here is a plain function that can be tested +without a session. """ +from datetime import datetime, timezone from types import SimpleNamespace from unittest.mock import patch @@ -132,3 +135,58 @@ def test_supersession_rows_serialise_the_pair(): {"superseder_id": 9, "superseded_id": 4}, {"superseder_id": 9, "superseded_id": 5}, ] + + +def test_rule_rows_carry_the_verification_fields(): + """A rule's check must survive a backup. + + `verify_with`/`expires_when`/`verified_at` (milestone 312) say whether a + rule is a fact that can go false and when it was last confirmed. A backup + that drops them restores a rulebook that has forgotten which of its rules + can rot — the exact blindness the fields were added to end. + + Column additions do not bump BACKUP_VERSION; only new SECTIONS do. Same + call made for when_to_apply/tier/arose_from_id in 0088 (commit 6ddb8bf). + """ + checked = datetime(2026, 8, 27, 12, 0, tzinfo=timezone.utc) + 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", + 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, + ) + out = backup._rule_rows([row])[0] + + assert out["verify_with"] == "cat some/file" + assert out["expires_when"] == "the file grows a shell" + assert out["verified_at"] == checked.isoformat() + # Provenance was exported from 0088 onward but silently dropped on the way + # back IN until milestone 312. Export side asserted here; the restore side + # remaps it through note_id_map. + assert out["arose_from_id"] == 99 + + +def test_rule_rows_keep_an_unverified_rule_unverified(): + """NULL verified_at means never checked, and it must round-trip as null. + + _dt substitutes now() so created_at/updated_at are never null. Reusing it + here would restore a rule nobody ever checked as though it had just been + checked — dropping it to the BOTTOM of the sweep it should top. That is + why _dt_or_none exists. + """ + 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", + verify_with=None, expires_when=None, verified_at=None, + arose_from_id=None, + created_at=datetime(2026, 8, 27, tzinfo=timezone.utc), + updated_at=datetime(2026, 8, 27, tzinfo=timezone.utc), + ) + assert backup._rule_rows([row])[0]["verified_at"] is None + assert backup._dt_or_none(None) is None + assert backup._dt_or_none("2026-08-27T12:00:00+00:00") == datetime( + 2026, 8, 27, 12, 0, tzinfo=timezone.utc + ) From 91b34619f94e3c553bfc2bde37c78675e0f5dbec Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 09:28:03 -0400 Subject: [PATCH 2/8] feat(rules): the write path carries a rule's check, and empty finally means empty (#3096, milestone 312 step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verify_with / expires_when now reach a rule through both doors and come back on every read. The open question this step existed to settle was how to UNSET a nullable field, and the answer is one convention per door: - MCP: "" still means "leave unchanged" — an agent filling three fields must not wipe the other five — so clearing is explicit, clear_fields=["..."]. Naming the field is the one form that cannot happen by accident. - REST: a cleared form input arrives as "", and the service normalises "" to NULL for every nullable rule column, so an emptied input does what it looks like it does. Two idioms, one outcome, and the normalisation is what makes the step-3 sweep correct: `verify_with IS NOT NULL` would otherwise be true for every rule ever touched through the UI, and the sweep would list the whole rulebook and mean nothing. to_dict renders "" and NULL identically, so this is only visible against a real column — hence the integration module rather than a mock. Editing verify_with drops verified_at. A stamp certifies A CHECK, not a rule; reword the check and the old stamp vouches for something that no longer exists. Safe direction, same asymmetry as _valid_tier: a rule wrongly listed as due costs one look, a rule wrongly vouched for costs the thing the sweep exists to catch. Editing anything else leaves the stamp alone, or a rulebook tidy-up would reset every constraint and the ordering would carry nothing. Reads: rule_brief attaches `last_verified` ONLY to a rule that carries a check — its presence is the signal, and it says both "this asserts a fact that can go false" and "here is how long ago anyone confirmed it". "never" rather than null, per #2483. The check text itself stays in get_rule; a listing needs to know which rules can rot, not how to test them. Search hits carry the full trio, since a hit is exactly the moment someone is about to act on a rule. Also folds in the #3078 finding, which had been sitting as a note: create_rule now teaches that when_to_apply is the retrieval surface and must carry the SYMPTOM — the words you would type while stuck — not just the situation. fake_rule gains the three fields as None for the reason the helper already documents one line up: unnamed, verify_with is a truthy MagicMock and every stand-in rule would claim a check it does not have. Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/api/rulebooks.ts | 27 +++- src/scribe/mcp/tools/rulebooks.py | 78 ++++++++++- src/scribe/mcp/tools/search.py | 14 +- src/scribe/routes/rulebooks.py | 11 +- src/scribe/services/rulebooks.py | 87 ++++++++++++- tests/helpers.py | 5 + tests/test_integration_rule_verification.py | 136 ++++++++++++++++++++ tests/test_services_rulebooks.py | 54 ++++++++ 8 files changed, 403 insertions(+), 9 deletions(-) create mode 100644 tests/test_integration_rule_verification.py diff --git a/frontend/src/api/rulebooks.ts b/frontend/src/api/rulebooks.ts index 2c3eb8c..01215e0 100644 --- a/frontend/src/api/rulebooks.ts +++ b/frontend/src/api/rulebooks.ts @@ -55,6 +55,16 @@ export interface Rule { tier: RuleTier; why: string; how_to_apply: string; + /** + * How to check the rule is still true, and the state that ends it. Set + * only on a rule that asserts a fact about something outside the + * operator's control; empty on a rule that is a decision, which is most + * of them. Empty is meaningful, not missing. + */ + verify_with: string; + expires_when: string; + /** When the check last passed. Null means never checked. */ + verified_at: string | null; /** The note or task that caused this rule, if one was recorded. */ arose_from_id: number | null; order_index: number; @@ -80,6 +90,12 @@ export interface RuleHeader { updated_at: string | null; when_to_apply?: string; arose_from_id?: number; + /** + * Present ONLY on a rule that carries a check — the presence of the key + * is itself the signal that this rule asserts a fact that can go false. + * A date (YYYY-MM-DD), or the literal "never". + */ + last_verified?: string; } export interface ApplicableRules { @@ -170,7 +186,14 @@ export async function getRule(id: number): Promise { return apiGet(`/api/rules/${id}`); } -/** The fields both write paths accept. `system_ids` REPLACES a rule's areas. */ +/** + * The fields both write paths accept. `system_ids` REPLACES a rule's areas. + * + * Sending "" for a nullable text field CLEARS it here — the server maps an + * empty string to NULL, so an emptied form input does what it looks like it + * does. (The MCP door reads "" as "leave unchanged" and needs an explicit + * clear_fields list instead; the two idioms reach the same state.) + */ export interface RuleWrite { title: string; statement: string; @@ -181,6 +204,8 @@ export interface RuleWrite { order_index: number; system_ids: number[]; arose_from_id: number | null; + verify_with: string; + expires_when: string; } export async function createRule(topicId: number, data: Partial & { title: string; statement: string }): Promise { diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 6d3c094..0e9fae8 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -246,6 +246,14 @@ async def list_always_on_rules(project_id: int = 0) -> dict: Pair with get_project(id).applicable_rules when working on a specific project to also load that project's subscription-derived rules. + A rule carrying `last_verified` asserts a FACT about something outside the + operator's control — a runner's shell, a tool's existence, a setting + somewhere. It is still binding; the field says how long ago anyone + confirmed it, and "never" means nobody has. Follow the rule, and if you + are already standing where the check could be made, make it: get_rule + gives you its `verify_with`. Most rules have no such field, which means + they are decisions and there is nothing to check. + Args: project_id: 0 (default) = the user-wide set. Inside a project, pass its id: an always-on rulebook the project EXCLUDED at inception @@ -276,7 +284,8 @@ async def create_rule( topic_id: int, title: str, statement: str, when_to_apply: str = "", why: str = "", how_to_apply: str = "", order_index: int = 0, tier: str = "always_on", system_ids: list[int] | None = None, - arose_from_id: int = 0, force: bool = False, + arose_from_id: int = 0, verify_with: str = "", expires_when: str = "", + force: bool = False, ) -> dict: """Create a new rule in a rulebook (a SHARED rule — keep it general). @@ -314,6 +323,15 @@ async def create_rule( optional: it decides the tier below, it is how the rule is found when it matters, and a rule nobody can place is a rule nobody applies. + This field is also the rule's RETRIEVAL SURFACE — it and the + statement are what a search is matched against, so it should + carry the SYMPTOM, not just the situation: the words someone + would actually type while stuck. Measured (note 3078): a rule + whose trigger named only its situation did not surface at all + for the problem it solves; adding the symptom to the same field + brought it back as the top hit. Where a rule prevents a specific + failure, put that failure's vocabulary here — the error text, + the wrong behaviour, the dead end. tier: "always_on" (default) or "conditional". The test: can you name the trigger WITHOUT naming a system, an artifact type or a moment? If the honest answer is "whenever you @@ -328,6 +346,23 @@ async def create_rule( cannot be followed and does not survive a rewording. why: Optional rationale — the reason the rule exists. how_to_apply: Optional operationalization — when / where it kicks in. + verify_with: How to CHECK this rule is still true. Set it only when + the rule asserts a fact about something outside your control — a + runner's shell, a bot's config, whether a tool exists. Those go + false silently, with nobody present. Give a command, a path, a + URL or a query; something runnable beats prose, because prose + has to be re-interpreted by whoever finds it. + LEAVE IT EMPTY for a rule that is a DECISION — a preference, a + standard, a way of working. A decision has no truth value: it + changes when you change it, and you know that you did. An empty + verify_with is not a gap, it is the marker for "there is nothing + to go and check," and the whole signal is worthless the moment + it is filled in out of tidiness. + expires_when: The STATE under which this rule stops being true — + "when the runner can be given a bash shell", "when the dashboard + approval setting is turned off". Deliberately not a date: a + constraint expires when the ground under it moves, not on a + schedule. Pairs with verify_with; both empty is the normal case. order_index: Display order within the topic (default 0). force: Bypass the near-duplicate gate. By default, a title-identical rule already in this topic BLOCKS creation and returns its id so you update @@ -343,6 +378,7 @@ async def create_rule( title=title, statement=statement, when_to_apply=when_to_apply, tier=tier, arose_from_id=arose_from_id, why=why, how_to_apply=how_to_apply, order_index=order_index, + verify_with=verify_with, expires_when=expires_when, ) return await rulebooks_svc.rule_detail(uid, rule, system_ids) @@ -351,7 +387,8 @@ async def create_project_rule( project_id: int, statement: str, title: str = "", when_to_apply: str = "", why: str = "", how_to_apply: str = "", order_index: int = 0, tier: str = "always_on", system_ids: list[int] | None = None, - arose_from_id: int = 0, force: bool = False, + arose_from_id: int = 0, verify_with: str = "", expires_when: str = "", + force: bool = False, ) -> dict: """Create a rule scoped to a single project (no rulebook needed). @@ -383,6 +420,13 @@ async def create_project_rule( arose_from_id: The note or task that CAUSED this rule. why: Optional rationale — the reason the rule exists. how_to_apply: Optional operationalization — when / where it kicks in. + verify_with: How to check this rule is still true — see create_rule. + Set it when the rule asserts a fact about someone else's software; + leave it empty when the rule is a decision. Project rules are the + likelier home for a real check: they name this project's files, + paths and quirks, which is exactly the kind of claim that rots. + expires_when: The state under which the rule stops being true — see + create_rule. A state, not a date. order_index: Display order within the project's rule list (default 0). force: Bypass the near-duplicate gate. By default, a title-identical rule already on this project BLOCKS creation and returns its id so you @@ -399,6 +443,7 @@ async def create_project_rule( title=derived_title, statement=statement, when_to_apply=when_to_apply, tier=tier, arose_from_id=arose_from_id, why=why, how_to_apply=how_to_apply, order_index=order_index, + verify_with=verify_with, expires_when=expires_when, ) return await rulebooks_svc.rule_detail(uid, rule, system_ids) @@ -407,12 +452,33 @@ 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 = "", + clear_fields: list[str] | None = None, ) -> dict: """Update a rule. Empty strings / order_index=-1 leave fields unchanged. 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). + + TO EMPTY A FIELD, NAME IT: clear_fields=["verify_with"]. Passing "" cannot + do it — "" means "leave this alone" here, which is what lets you update + two fields without wiping the other six. Clearable: why, how_to_apply, + when_to_apply, verify_with, expires_when, arose_from_id. Clearing and + setting the same field in one call clears it first, so the new value wins. + + Editing `verify_with` DROPS the rule's verification stamp. The stamp + certifies a check, not a rule; once the check is reworded the old stamp + vouches for something that no longer exists, so the rule re-enters the + staleness sweep as never-verified. + + Args: + verify_with: How to check the rule is still true — set it when the + rule asserts a fact about someone else's software, leave it empty + when the rule is a decision. See create_rule. + expires_when: The state under which the rule stops being true. A + state, not a date. See create_rule. + clear_fields: Names of fields to empty, as above. """ uid = current_user_id() fields: dict = {} @@ -430,9 +496,15 @@ async def update_rule( fields["why"] = why if how_to_apply: fields["how_to_apply"] = how_to_apply + if verify_with: + fields["verify_with"] = verify_with + if expires_when: + fields["expires_when"] = expires_when if order_index >= 0: fields["order_index"] = order_index - rule = await rulebooks_svc.update_rule(rule_id, uid, **fields) + 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) diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index e7eb1c4..c6db75d 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -14,6 +14,7 @@ from scribe.services.access import owner_names_for from scribe.services.embeddings import ( DEFAULT_SIMILARITY_THRESHOLD, semantic_search_notes, semantic_search_rules, ) +from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary @@ -23,7 +24,10 @@ async def _search_rules(uid: int, q: str, limit: int) -> dict: A rule hit carries `why` and `how_to_apply`: they are the operational half of a rule and the session-start payload never includes them, so a caller who went looking should get the whole thing rather than a summary they then - have to re-fetch. + have to re-fetch. It also carries the rule's check (`verify_with`, + `expires_when`, `last_verified`) when it has one — a search hit is exactly + the moment someone is about to act on a rule, and "this asserts a fact + nobody has confirmed" is part of what the rule says. Rules are not project-scoped the way notes are (a family rule belongs to no project), so `project_id` and `system_id` do not apply here. @@ -39,6 +43,14 @@ async def _search_rules(uid: int, q: str, limit: int) -> dict: "tier": rule.tier, "why": rule.why or "", "how_to_apply": rule.how_to_apply or "", + "verify_with": rule.verify_with or "", + "expires_when": rule.expires_when or "", + # Only on a rule that carries a check; its absence means the + # rule is a decision, not that nobody has looked. + **( + {"last_verified": rulebooks_svc.last_verified_label(rule)} + if rule.verify_with else {} + ), "topic_id": rule.topic_id, "project_id": rule.project_id, "similarity": float(score), diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index 08d0d9e..e402584 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -165,6 +165,8 @@ async def create_rule(topic_id: int): when_to_apply=data.get("when_to_apply", ""), tier=data.get("tier", "always_on"), arose_from_id=data.get("arose_from_id", 0) or 0, + verify_with=data.get("verify_with", ""), + expires_when=data.get("expires_when", ""), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -191,8 +193,13 @@ 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", "arose_from_id", + "verify_with", "expires_when") } + # No clear_fields here: a form sends "" for an emptied input, and the + # service normalises "" to NULL for every nullable text column. The MCP + # door needs the explicit list only because "" already means "unchanged" + # there — two idioms, one outcome. rule = await rulebooks_svc.update_rule(rule_id, uid, **fields) if rule is None: return jsonify({"error": "rule not found"}), 404 @@ -375,6 +382,8 @@ async def create_project_rule(project_id: int): when_to_apply=data.get("when_to_apply", ""), tier=data.get("tier", "always_on"), arose_from_id=data.get("arose_from_id", 0) or 0, + verify_with=data.get("verify_with", ""), + expires_when=data.get("expires_when", ""), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index aece902..a6c1c8b 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -8,6 +8,7 @@ depending on the caller's needs (mirroring services/events.py pattern). from __future__ import annotations import logging +from collections.abc import Iterable from typing import Optional from sqlalchemy import delete as sql_delete, insert, or_, select @@ -288,6 +289,17 @@ TIERS = ("always_on", "conditional") RELATION_KINDS = ("co_surfaces", "overrides", "elaborates") +# The rule columns that are nullable, and therefore the ones where EMPTY has +# to mean empty. A write that stores "" leaves a column that is not NULL and +# not content — `verify_with IS NOT NULL` would then be true for a rule with +# no check, and the staleness sweep would list rules it should never see. +# Normalising here, at the one service seam, is what makes "unset" a single +# state instead of two that read alike through to_dict's `or ""`. +NULLABLE_RULE_TEXT = ( + "why", "how_to_apply", "when_to_apply", "verify_with", "expires_when", +) + + def _valid_tier(tier: str) -> str: """An unrecognised tier falls back to always_on — the SAFE direction. @@ -299,6 +311,21 @@ def _valid_tier(tier: str) -> str: return tier if tier in TIERS else "always_on" +def last_verified_label(rule: Rule) -> str | None: + """How long ago the rule's check passed — None when it carries no check. + + One helper because two surfaces need the same answer and the brief-dict + lesson in rule_brief's docstring is what happens otherwise: three copies + that had already drifted. `None` means "this rule is a decision, the + question does not apply"; "never" means "it is a fact and nobody has + confirmed it" — a distinction worth keeping, because the second is the + one worth acting on. + """ + if not rule.verify_with: + return None + return rule.verified_at.date().isoformat() if rule.verified_at else "never" + + def rule_brief(rule: Rule, **extra) -> dict: """The shape a rule takes when it is SURFACED rather than opened. @@ -331,6 +358,16 @@ def rule_brief(rule: Rule, **extra) -> dict: out["when_to_apply"] = rule.when_to_apply if rule.arose_from_id: out["arose_from_id"] = rule.arose_from_id + # Present ONLY on a rule that carries a check — its presence is the + # signal, and it says two things at once: this rule asserts a fact that + # can go false, and here is how long ago anyone confirmed it. The check + # text itself stays in get_rule; a listing needs to know WHICH rules can + # rot, not how to test them. "never" rather than null, per #2483: a key + # that reads as an unused capability is a different claim from a rule + # nobody has ever verified. + stamp = last_verified_label(rule) + if stamp: + out["last_verified"] = stamp out.update({k: v for k, v in extra.items() if v is not None}) return out @@ -436,6 +473,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 = "", ) -> Rule: async with async_session() as session: await _assert_topic_owned(session, topic_id, user_id) @@ -447,6 +485,8 @@ async def create_rule( tier=_valid_tier(tier), why=why or None, how_to_apply=how_to_apply or None, + verify_with=verify_with or None, + expires_when=expires_when or None, arose_from_id=arose_from_id or None, order_index=order_index, ) @@ -461,6 +501,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 = "", ) -> Rule: """Create a rule scoped to a single project (no rulebook ceremony). @@ -478,6 +519,8 @@ async def create_project_rule( tier=_valid_tier(tier), why=why or None, how_to_apply=how_to_apply or None, + verify_with=verify_with or None, + expires_when=expires_when or None, arose_from_id=arose_from_id or None, order_index=order_index, ) @@ -681,7 +724,23 @@ async def get_rule(rule_id: int, user_id: int) -> Optional[Rule]: return await _fetch_owned_rule(session, rule_id, user_id) -async def update_rule(rule_id: int, user_id: int, **fields) -> Optional[Rule]: +async def update_rule( + rule_id: int, user_id: int, clear: Iterable[str] = (), **fields, +) -> Optional[Rule]: + """Patch a rule. `clear` names fields to unset; **fields carries new values. + + Clearing is EXPLICIT and separate because a nullable field cannot be + emptied by passing it. The MCP door reads "" as "leave this alone" — an + agent filling three fields must not wipe the other five — so a caller + there has no value that means "remove it", and a rule that stops being a + constraint genuinely needs its check removed. Naming the field is the one + form that cannot happen by accident. + + Callers that DO have a meaningful empty value (the REST door, where a + cleared form input arrives as "") get the same outcome through + NULLABLE_RULE_TEXT normalisation below, so the two doors keep their own + idiom and agree about the result. + """ async with async_session() as session: rule = await _fetch_owned_rule(session, rule_id, user_id) if rule is None: @@ -689,10 +748,32 @@ async def update_rule(rule_id: int, user_id: int, **fields) -> Optional[Rule]: allowed = { "title", "statement", "why", "how_to_apply", "order_index", "when_to_apply", "tier", "arose_from_id", + "verify_with", "expires_when", } + check_before = rule.verify_with + for key in clear: + if key in allowed and key in NULLABLE_RULE_TEXT: + setattr(rule, key, None) + elif key == "arose_from_id": + setattr(rule, key, None) for key, value in fields.items(): - if key in allowed and value is not None: - setattr(rule, key, _valid_tier(value) if key == "tier" else value) + if key not in allowed or value is None: + continue + if key == "tier": + value = _valid_tier(value) + elif key in NULLABLE_RULE_TEXT: + value = value or None + elif key == "arose_from_id": + value = value or None + setattr(rule, key, value) + # A verification stamp certifies A CHECK, not a rule. Rewrite or + # remove the check and the old stamp certifies something that no + # longer exists — so it is dropped, and the rule re-enters the sweep. + # The safe direction, for the same reason _valid_tier falls back to + # always_on: a rule wrongly listed as due costs one look, a rule + # wrongly vouched for costs the thing the sweep exists to catch. + if rule.verify_with != check_before: + rule.verified_at = None await session.commit() await session.refresh(rule) _refresh_rule_embedding(rule) diff --git a/tests/helpers.py b/tests/helpers.py index e263e61..df77475 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -159,6 +159,11 @@ 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, + # 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 + # onto all of them. Most rules have none — that is the default here. + "verify_with": None, "expires_when": None, "verified_at": None, "order_index": 0, "created_at": _now(), "updated_at": _now(), }, attrs) diff --git a/tests/test_integration_rule_verification.py b/tests/test_integration_rule_verification.py new file mode 100644 index 0000000..381a245 --- /dev/null +++ b/tests/test_integration_rule_verification.py @@ -0,0 +1,136 @@ +"""Real-Postgres tests for a rule's CHECK — the write half (milestone 312). + +What mocks cannot prove, and what the staleness sweep depends on: + +1. **Empty means NULL.** The sweep asks for rules where `verify_with` is set. + A write that stored "" would leave a column that is neither null nor + content, and every rule ever touched through the REST door would answer + "yes, I have a check" — the sweep would list the whole rulebook and mean + nothing. Only a real column can show the difference; `to_dict`'s `or ""` + renders both the same. + +2. **Clearing is possible at all.** "" means "leave unchanged" at the MCP + door, so without an explicit clear there is no way to retire a check. + +3. **A stamp does not outlive the check it certifies.** Reword the check and + the old `verified_at` vouches for something that no longer exists. +""" +from datetime import datetime, timezone + +import pytest +import pytest_asyncio + +from scribe.models import async_session +from scribe.models.rulebook import Rule +from scribe.services import rulebooks as rulebooks_svc +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + + +@pytest_asyncio.fixture +async def constraint(): + """One rule carrying a check, already verified. + + Verified at creation time rather than left null, because every assertion + here is about what happens to an EXISTING stamp — a fixture that started + null could pass all of them by doing nothing. + """ + async with async_session() as s: + owner = await ensure_user(s, "verification_owner") + uid = owner.id + await s.commit() + + book = await rulebooks_svc.create_rulebook(uid, "Environment facts") + topic = await rulebooks_svc.create_topic(book.id, uid, "ci") + rule = await rulebooks_svc.create_rule( + topic.id, uid, "The runner has no bash", + "Write every `run:` step in POSIX sh.", + verify_with="read the workflow's shell setting", + expires_when="the runner can be given a bash shell", + ) + async with async_session() as s: + row = await s.get(Rule, rule.id) + row.verified_at = datetime(2026, 8, 1, tzinfo=timezone.utc) + await s.commit() + return {"uid": uid, "rule": rule.id} + + +async def _row(rule_id: int) -> Rule: + async with async_session() as s: + return await s.get(Rule, rule_id) + + +async def test_the_check_and_its_expiry_persist(constraint): + row = await _row(constraint["rule"]) + assert row.verify_with == "read the workflow's shell setting" + assert row.expires_when == "the runner can be given a bash shell" + assert row.verified_at is not None + + +async def test_an_empty_string_becomes_null_not_an_empty_column(constraint): + """The REST door's idiom: a cleared form input arrives as "". + + NULL is asserted directly against the column rather than through to_dict, + which renders `None` and `""` identically — the difference this test + exists for would be invisible one layer up. + """ + await rulebooks_svc.update_rule( + constraint["rule"], constraint["uid"], verify_with="", expires_when="", + ) + row = await _row(constraint["rule"]) + assert row.verify_with is None + assert row.expires_when is None + + +async def test_naming_a_field_in_clear_empties_it(constraint): + """The MCP door's idiom, where "" already means "leave this alone".""" + await rulebooks_svc.update_rule( + constraint["rule"], constraint["uid"], clear=["verify_with"], + ) + row = await _row(constraint["rule"]) + assert row.verify_with is None + # expires_when was NOT named, so it survives — clearing is per-field, and + # a caller retiring one field must not lose the others. + assert row.expires_when == "the runner can be given a bash shell" + + +async def test_rewording_the_check_drops_the_stamp(constraint): + """A stamp certifies a check, not a rule. + + The safe direction, for the same reason _valid_tier falls back to + always_on: a rule wrongly listed as due costs one look, a rule wrongly + vouched for costs exactly what the sweep exists to catch. + """ + await rulebooks_svc.update_rule( + constraint["rule"], constraint["uid"], + verify_with="read the runner's container shell, not the image's", + ) + row = await _row(constraint["rule"]) + assert row.verified_at is None + + +async def test_clearing_the_check_drops_the_stamp(constraint): + await rulebooks_svc.update_rule( + constraint["rule"], constraint["uid"], clear=["verify_with"], + ) + row = await _row(constraint["rule"]) + assert row.verified_at is None + + +async def test_editing_anything_else_leaves_the_stamp_alone(constraint): + """The other half of the rule above, and the one that keeps it useful. + + If any edit reset the stamp, a rulebook tidy-up would put every constraint + back at the top of the sweep and the ordering would carry no information. + Only the check's own text invalidates its verification. + """ + await rulebooks_svc.update_rule( + constraint["rule"], constraint["uid"], + why="act_runner picks the shell, and the image's SHELL directive " + "applies to the build, not to `run:`.", + expires_when="the runner grows a shell setting", + ) + row = await _row(constraint["rule"]) + assert row.verified_at is not None + assert row.why.startswith("act_runner picks the shell") diff --git a/tests/test_services_rulebooks.py b/tests/test_services_rulebooks.py index b7de1ad..69aba3b 100644 --- a/tests/test_services_rulebooks.py +++ b/tests/test_services_rulebooks.py @@ -392,3 +392,57 @@ def test_an_unknown_tier_falls_back_to_binding(): assert _valid_tier("Conditional") == "always_on" assert _valid_tier("") == "always_on" assert _valid_tier("occasionally") == "always_on" + + +# ── verify_with / expires_when (milestone 312) ────────────────────────── + +def test_a_rule_with_no_check_says_nothing_about_verification(): + """The empty case is the COMMON case, and it must stay silent. + + Most rules are decisions: they have no truth value and there is nothing to + go and check. If a brief carried `last_verified` for those too, the signal + would be worthless — every rule would look like something someone ought to + be verifying, and the handful that genuinely rot would stop standing out. + """ + from scribe.services.rulebooks import last_verified_label, rule_brief + + rule = fake_rule() + assert last_verified_label(rule) is None + assert "last_verified" not in rule_brief(rule) + + +def test_an_unverified_constraint_reads_never_rather_than_null(): + """#2483 again: a null key reads as a capability going unused. "never" is + a different and much stronger claim — this rule asserts a fact about + someone else's software and nobody has ever confirmed it.""" + from scribe.services.rulebooks import last_verified_label, rule_brief + + rule = fake_rule(verify_with="cat CI-runner/renovate/config.js") + assert last_verified_label(rule) == "never" + assert rule_brief(rule)["last_verified"] == "never" + + +def test_a_verified_constraint_reports_the_date_it_was_checked(): + """A date, not a stamp — the question is "how old is this", the same call + rule_brief makes for updated_at.""" + from scribe.services.rulebooks import last_verified_label + + rule = fake_rule( + verify_with="cat CI-runner/renovate/config.js", + verified_at=datetime(2026, 8, 27, 11, 46, tzinfo=timezone.utc), + ) + assert last_verified_label(rule) == "2026-08-27" + + +def test_the_check_text_itself_never_enters_a_listing(): + """A listing says WHICH rules can rot, not how to test them. The check can + be a long command; multiplied across an always-on set it is the same bloat + `why` and `how_to_apply` are kept out of a brief to avoid.""" + from scribe.services.rulebooks import rule_brief + + out = rule_brief(fake_rule( + verify_with="a very long command " * 20, + expires_when="the runner learns a new shell", + )) + assert "verify_with" not in out + assert "expires_when" not in out From 874f7cacdb8adedeb0e3fcc224d117f4e414f2af Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 09:31:30 -0400 Subject: [PATCH 3/8] test(rules): the kwargs assertion learns about `clear` (#3096, milestone 312 step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_update_rule_only_sends_non_default_fields pins that the MCP door forwards only what the caller actually gave. `clear` is now always forwarded — an empty tuple is "clear nothing", a value rather than an absent argument — so the expected kwargs gained it. The property under test is unchanged: everything left at its default still stays out. Two tests added beside it while the shape is in view: naming a field for clearing reaches the service as `clear`, and the check fields are forwarded when given. CI 4630 otherwise green — the integration lane ran all six of the new real-Postgres cases (72 selected, was 66) and applied 0089 -> 0090. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_mcp_tool_rulebooks.py | 41 +++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 33fbd58..1c68ffb 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -115,7 +115,46 @@ async def test_update_rule_only_sends_non_default_fields(): await update_rule(rule_id=1, statement="new statement") args, kwargs = mock.call_args assert args == (1, 7) - assert kwargs == {"statement": "new statement"} + # `clear` is always forwarded — an empty tuple is "clear nothing", which is + # a value, not an absent argument. Everything the caller left at its + # default stays out: that is the property this test pins. + assert kwargs == {"statement": "new statement", "clear": ()} + + +@pytest.mark.asyncio +async def test_update_rule_forwards_the_fields_named_for_clearing(): + """Naming a field is the only way to empty it through this door. + + "" means "leave unchanged" here, so a caller has no value that means + "remove it" — which is what makes an explicit list necessary and what + stops a partial update from wiping the fields it did not mention. + """ + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) + mock = AsyncMock(return_value=rule) + with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule", mock), _plain_detail(): + from scribe.mcp.tools.rulebooks import update_rule + await update_rule(rule_id=1, clear_fields=["verify_with"]) + _args, kwargs = mock.call_args + assert kwargs == {"clear": ["verify_with"]} + + +@pytest.mark.asyncio +async def test_update_rule_sends_the_check_fields_when_given(): + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) + mock = AsyncMock(return_value=rule) + with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule", mock), _plain_detail(): + from scribe.mcp.tools.rulebooks import update_rule + await update_rule( + rule_id=1, + verify_with="cat CI-runner/renovate/config.js", + expires_when="approval is turned off", + ) + _args, kwargs = mock.call_args + assert kwargs == { + "verify_with": "cat CI-runner/renovate/config.js", + "expires_when": "approval is turned off", + "clear": (), + } @pytest.mark.asyncio From b97f57ee7f33b79853b55073c0ed0280bb2a98ac Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 10:49:47 -0400 Subject: [PATCH 4/8] =?UTF-8?q?feat(rules):=20the=20staleness=20sweep=20?= =?UTF-8?q?=E2=80=94=20which=20standing=20rules=20assert=20a=20fact=20nobo?= =?UTF-8?q?dy=20has=20confirmed=20(#3097,=20milestone=20312=20step=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The query the last two steps were storage for. `rules_due_for_verification` returns every rule carrying a `verify_with`, ordered by `verified_at` ASC NULLS FIRST, each row carrying the check IN FULL — the opposite call from rule_brief, because the reader is about to go and run it. NULLS FIRST is the ordering this turns on. Postgres sorts NULLs last on an ASC ordering, which would put the rules nobody has ever confirmed BEHIND every rule someone once looked at. Exactly backwards: a claim with no evidence at all outranks an old one. Rules with no check never appear, and that is the property that keeps the list worth reading. Most rules are decisions — no truth value, nothing to go and check. If they appeared here the sweep would be the rulebook. `mark_rule_verified(rule_id, still_true)` closes the loop, asymmetrically: passing writes a stamp, FAILING WRITES NOTHING. There is no "verified false" state because a rule whose check failed is not in a special condition, it is wrong — and recording the failure as a flag would let it sit there being false with the sweep satisfied that someone had looked. So it stays at the top until someone corrects or retires it, and the response says so. An unrecognised `tier` filter raises rather than falling back. _valid_tier's silent always_on default is right for a WRITE — a typo should leave a rule binding — and wrong for a FILTER, where the same fallback quietly answers a different question and returns a short list that reads as good news. Deliberately NOT filterable by project: a project reaches rules through project scope, subscriptions, always-on rulebooks and exclusions, and a filter missing one of those paths would UNDER-report — the exact failure this surface exists to prevent. Said so in the docstring rather than shipping a half-correct filter. Ownership-scoped like every other rule read (owned rulebook, or owned project), in ONE statement with an OR across the XOR rather than two queries merged in Python, so the ordering is the database's and cannot disagree with itself. Note that rules have no sharing ACL in this schema — no rule_shares, no rulebook_shares — so there is no wider set for access.py to consult here. Also fixes a test title that had been lying for ten tools: "all sixteen tools" asserted 26. The number now lives only in the assertion. Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/api/rulebooks.ts | 56 ++++++++ src/scribe/mcp/tools/rulebooks.py | 92 ++++++++++++ src/scribe/routes/rulebooks.py | 53 +++++++ src/scribe/services/rulebooks.py | 149 +++++++++++++++++++- tests/test_integration_rule_verification.py | 139 ++++++++++++++++++ tests/test_mcp_tool_rulebooks.py | 16 ++- tests/test_services_rulebooks.py | 59 ++++++++ 7 files changed, 560 insertions(+), 4 deletions(-) diff --git a/frontend/src/api/rulebooks.ts b/frontend/src/api/rulebooks.ts index 01215e0..f884999 100644 --- a/frontend/src/api/rulebooks.ts +++ b/frontend/src/api/rulebooks.ts @@ -281,3 +281,59 @@ export async function includeAlwaysOnRulebook(projectId: number, rulebookId: num await apiDelete(`/api/projects/${projectId}/exclusions/rulebooks/${rulebookId}`); } + +/** + * One row of the staleness sweep. Unlike RuleHeader this carries the CHECK + * in full — the reader is about to go and run it, so the text is the point + * of the payload rather than the bloat a listing avoids. + */ +export interface RuleVerificationRow { + id: number; + title: string; + statement: string; + tier: RuleTier; + topic_id: number | null; + project_id: number | null; + when_to_apply: string; + verify_with: string; + expires_when: string; + /** A date (YYYY-MM-DD), or the literal "never". */ + last_verified: string | null; + /** Null when never verified — "never" is not zero days ago. */ + days_since_verified: number | null; +} + +/** + * Rules asserting a fact that may have gone false, oldest verification + * first, never-checked at the top. Rules without a check never appear: + * they are decisions, and there is nothing to go and check. + * + * Not filterable by project — a project reaches rules through project + * scope, subscriptions, always-on rulebooks and exclusions, and a filter + * missing one of those paths would under-report. + */ +export async function listRulesDueForVerification(opts: { + olderThanDays?: number; + tier?: RuleTier; + neverOnly?: boolean; +} = {}): Promise<{ rules: RuleVerificationRow[]; total: number }> { + const q = new URLSearchParams(); + if (opts.olderThanDays) q.set("older_than_days", String(opts.olderThanDays)); + if (opts.tier) q.set("tier", opts.tier); + if (opts.neverOnly) q.set("never_only", "true"); + const qs = q.toString(); + return apiGet(`/api/rules-due-for-verification${qs ? `?${qs}` : ""}`); +} + +/** + * Record that a rule's check was RUN, and what it said. + * + * `stillTrue: false` writes nothing on purpose — a rule whose check failed + * is not in a recordable state, it is wrong — so it stays at the top of the + * sweep until someone corrects or retires it. + */ +export async function markRuleVerified( + id: number, stillTrue = true, +): Promise { + return apiPost(`/api/rules/${id}/verify`, { still_true: stillTrue }); +} diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 0e9fae8..43bd216 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -691,6 +691,97 @@ async def unrelate_rules(relation_id: int) -> dict: raise ValueError(f"relation {relation_id} not found") return {"deleted": relation_id} +# ── The staleness sweep (milestone 312) ──────────────────────────────── + +async def rules_due_for_verification( + older_than_days: int = 0, tier: str = "", never_only: bool = False, +) -> dict: + """Which standing rules assert a FACT that nobody has confirmed lately. + + A rulebook holds two kinds of thing. Most rules are DECISIONS — how the + operator wants to work. They have no truth value and cannot rot. A few + assert a fact about someone else's software: what a CI runner does, which + tools exist, what a setting is currently set to. Those go false silently, + with nobody present, and they keep being handed to every session as + binding instructions long after they stopped being true. + + This lists the second kind, oldest verification first, never-checked at + the top. Each row carries the rule's `verify_with` in full — you are + about to go and run it — plus `expires_when`, and `days_since_verified`. + + Reach for it when you are curating the rulebook, when a rule's advice + just contradicted what you observed, or periodically. Then, for each row: + run the check, and call mark_rule_verified with what you found. + + Rules with no `verify_with` never appear here. That is correct: they are + decisions, and there is nothing to go and check. Do not "fix" their + absence by giving them checks — the list is only worth reading while + everything on it genuinely can go false. + + Args: + older_than_days: only rules last verified longer ago than this. + Never-checked rules always qualify. 0 = no age filter. + tier: "always_on" or "conditional" to narrow. An always-on constraint + that has gone false is the expensive kind — it is preloaded into + every session, so a wrong one is wrong everywhere at once. + never_only: only rules nobody has ever verified. + + NOT filterable by project, deliberately: a project reaches rules through + project scope, subscriptions, always-on rulebooks and exclusions, and a + filter that missed one of those paths would UNDER-report — which is the + exact failure this whole surface exists to prevent. Read the whole list. + """ + uid = current_user_id() + rules = await rulebooks_svc.rules_due_for_verification( + uid, older_than_days=older_than_days, tier=tier, never_only=never_only, + ) + return { + "rules": [rulebooks_svc.verification_row(r) for r in rules], + "total": len(rules), + } + + +async def mark_rule_verified(rule_id: int, still_true: bool = True) -> dict: + """Record that you ran a rule's check — and what it said. + + Call this AFTER actually running the rule's `verify_with`, never on the + strength of the rule sounding plausible. A stamp nobody earned is worse + than no stamp: it moves the rule to the bottom of the sweep and buys it + another long silence. + + `still_true=False` writes NOTHING. A rule whose check failed is not in a + special state to be recorded — it is WRONG, and the only honest next + moves are to correct it, retire it, or find out why. So it stays at the + top of the sweep until someone deals with it, and the response tells you + what the rule said would end it. + + Args: + rule_id: the rule whose check you ran. + still_true: True if the check passed. False if the fact it asserts is + no longer true — say so, that is the outcome worth having. + """ + uid = current_user_id() + rule = await rulebooks_svc.mark_rule_verified(rule_id, uid, still_true) + if rule is None: + raise ValueError( + f"rule {rule_id} not found, or carries no verify_with " + f"(nothing to verify is not the same as verified)" + ) + data = await rulebooks_svc.rule_detail(uid, rule) + if still_true: + data["verified"] = True + return data + data["verified"] = False + data["next"] = ( + "This rule is no longer true and is still binding on every session " + "that loads it. Correct it with update_rule, retire it with " + "delete_rule, or open a task to work out what replaced it. Its " + "verified_at is deliberately untouched, so it stays at the top of " + "rules_due_for_verification until one of those happens." + ) + return data + + def register(mcp) -> None: for fn in ( list_rulebooks, get_rulebook, create_rulebook, update_rulebook, delete_rulebook, @@ -702,5 +793,6 @@ def register(mcp) -> None: suppress_rule_for_project, unsuppress_rule_for_project, suppress_topic_for_project, unsuppress_topic_for_project, exclude_always_on_rulebook, include_always_on_rulebook, + rules_due_for_verification, mark_rule_verified, ): mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index e402584..2acc4bb 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -390,3 +390,56 @@ async def create_project_rule(project_id: int): return jsonify(await rulebooks_svc.rule_detail( get_current_user_id(), rule, data.get("system_ids"), )), 201 + + +# ── The staleness sweep (milestone 312) ──────────────────────────────── + +@rulebooks_bp.get("/rules-due-for-verification") +@login_required +async def rules_due_for_verification(): + """Rules that carry a check, oldest verification first, never-checked top. + + Query params: older_than_days, tier, never_only. A rule with no + `verify_with` never appears — it is a decision, not a fact. + """ + uid = get_current_user_id() + args = request.args + try: + older = int(args.get("older_than_days", 0) or 0) + except ValueError: + return jsonify({"error": "older_than_days must be an integer"}), 400 + try: + rules = await rulebooks_svc.rules_due_for_verification( + uid, + older_than_days=older, + tier=args.get("tier", ""), + never_only=args.get("never_only", "").lower() in ("1", "true", "yes"), + ) + except ValueError as exc: + # An unrecognised tier is a 400, not a silently narrowed result set: + # a filter that quietly answers a different question is the failure + # this whole surface exists to catch. + return jsonify({"error": str(exc)}), 400 + return jsonify({ + "rules": [rulebooks_svc.verification_row(r) for r in rules], + "total": len(rules), + }) + + +@rulebooks_bp.post("/rules//verify") +@login_required +async def mark_rule_verified(rule_id: int): + """Record that the rule's check was run. Body: {"still_true": bool}. + + `still_true: false` writes nothing — a rule whose check failed is wrong, + not in a recordable state — so it stays at the top of the sweep. + """ + data = await request.get_json() or {} + uid = get_current_user_id() + still_true = data.get("still_true", True) + rule = await rulebooks_svc.mark_rule_verified(rule_id, uid, bool(still_true)) + if rule is None: + return jsonify({"error": "rule not found, or carries no verify_with"}), 404 + payload = await rulebooks_svc.rule_detail(uid, rule) + payload["verified"] = bool(still_true) + return jsonify(payload) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index a6c1c8b..536754e 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -11,7 +11,7 @@ import logging from collections.abc import Iterable from typing import Optional -from sqlalchemy import delete as sql_delete, insert, or_, select +from sqlalchemy import and_, delete as sql_delete, insert, or_, select from scribe.models import async_session from scribe.models.system import System @@ -1361,3 +1361,150 @@ def rules_payload(applicable: dict) -> dict: "suppressed_topics": applicable.get("suppressed_topics", []), "excluded_always_on": applicable.get("excluded_always_on", []), } + + +# ── The staleness sweep (milestone 312) ──────────────────────────────── + +async def rules_due_for_verification( + user_id: int, + older_than_days: int = 0, + tier: str = "", + never_only: bool = False, +) -> list[Rule]: + """Rules that carry a check, oldest verification first, never-checked top. + + THE QUERY THIS MILESTONE EXISTS FOR. `verify_with` and `expires_when` are + storage; this is what turns them into something that gets acted on. The + 307 audit cost a session and found four broken rules by luck — this makes + the same question a list, and staleness measurable by age instead of + discoverable by accident. + + Ordered `verified_at` ASC NULLS FIRST: never-checked outranks + checked-long-ago, because a rule nobody has ever confirmed is a claim + with no evidence behind it at all. + + Rules with no `verify_with` never appear. That is not an omission — they + are decisions, there is nothing to go and check, and listing them would + dilute the result until nobody reads it. + + Ownership-scoped exactly like list_rules: a rule reached through an owned + rulebook, or scoped to an owned project. Rules have no sharing ACL in this + schema — no rule_shares, no rulebook_shares — so there is no wider set to + consult here, unlike notes and projects. + + Args: + user_id: whose rules. + older_than_days: only rules last verified longer ago than this. + Never-checked rules always qualify — they are the most overdue + thing there is. 0 = no age filter. + tier: "always_on" or "conditional" to narrow. Raises on anything else + rather than falling back: _valid_tier's silent always_on default + is right for a WRITE (the safe direction is to keep binding), and + wrong for a FILTER, where it would quietly answer a different + question than the one asked. + never_only: only rules that have never been verified. + """ + from datetime import datetime, timedelta, timezone + + from scribe.models.project import Project + + if tier and tier not in TIERS: + raise ValueError(f"tier must be one of {TIERS}, got {tier!r}") + + async with async_session() as session: + stmt = ( + select(Rule) + .outerjoin(RulebookTopic, Rule.topic_id == RulebookTopic.id) + .outerjoin(Rulebook, RulebookTopic.rulebook_id == Rulebook.id) + .outerjoin(Project, Rule.project_id == Project.id) + .where( + Rule.deleted_at.is_(None), + Rule.verify_with.is_not(None), + # One statement rather than two queries merged in Python, so + # the ordering below is the database's and cannot disagree + # with itself across the two halves of the XOR. + or_( + and_( + Rulebook.owner_user_id == user_id, + Rulebook.deleted_at.is_(None), + RulebookTopic.deleted_at.is_(None), + ), + Project.user_id == user_id, + ), + ) + ) + if tier: + stmt = stmt.where(Rule.tier == tier) + if never_only: + stmt = stmt.where(Rule.verified_at.is_(None)) + elif older_than_days > 0: + cutoff = datetime.now(timezone.utc) - timedelta(days=older_than_days) + stmt = stmt.where( + or_(Rule.verified_at.is_(None), Rule.verified_at < cutoff) + ) + stmt = stmt.order_by(Rule.verified_at.asc().nullsfirst(), Rule.id) + return list((await session.execute(stmt)).scalars().all()) + + +def verification_row(rule: Rule) -> dict: + """One row of the sweep — the CHECK in full, unlike rule_brief. + + The opposite call from a listing: here the caller is about to go and run + the check, so the text they need is the point of the payload rather than + the bloat. `days_since` is computed rather than left to the reader, + because "2026-06-14" and "74 days" prompt different reactions and only + one of them is the question being asked. + """ + from datetime import datetime, timezone + + days = None + if rule.verified_at is not None: + stamp = rule.verified_at + if stamp.tzinfo is None: + stamp = stamp.replace(tzinfo=timezone.utc) + days = (datetime.now(timezone.utc) - stamp).days + return { + "id": rule.id, + "title": rule.title, + "statement": rule.statement, + "tier": rule.tier, + "topic_id": rule.topic_id, + "project_id": rule.project_id, + "when_to_apply": rule.when_to_apply or "", + "verify_with": rule.verify_with or "", + "expires_when": rule.expires_when or "", + "last_verified": last_verified_label(rule), + "days_since_verified": days, + } + + +async def mark_rule_verified( + rule_id: int, user_id: int, still_true: bool = True, +) -> Optional[Rule]: + """Stamp a rule as verified — or, when the check FAILED, refuse to. + + A failing check is the outcome worth having, and the asymmetry is + deliberate: passing writes a stamp, failing writes nothing. There is no + "verified false" state to record, because a rule whose check failed is + not a rule in a special condition — it is a rule that is WRONG, and the + only honest resolutions are to correct it, retire it, or find out why. + Recording the failure as a flag would let it sit there being false with + the sweep quietly satisfied that someone had looked. + + So a failed check leaves `verified_at` untouched, and the rule stays at + the top of the sweep until someone actually deals with it. + + Returns None when the rule is not found, not owned, or carries no + `verify_with` — nothing to verify is a different answer from verified. + """ + from datetime import datetime, timezone + + async with async_session() as session: + rule = await _fetch_owned_rule(session, rule_id, user_id) + if rule is None or not rule.verify_with: + return None + if still_true: + rule.verified_at = datetime.now(timezone.utc) + await session.commit() + await session.refresh(rule) + return rule diff --git a/tests/test_integration_rule_verification.py b/tests/test_integration_rule_verification.py index 381a245..8982153 100644 --- a/tests/test_integration_rule_verification.py +++ b/tests/test_integration_rule_verification.py @@ -134,3 +134,142 @@ async def test_editing_anything_else_leaves_the_stamp_alone(constraint): row = await _row(constraint["rule"]) assert row.verified_at is not None assert row.why.startswith("act_runner picks the shell") + + +# ── the sweep itself (step 3) ────────────────────────────────────────── + +@pytest_asyncio.fixture +async def rulebook_of_three(): + """A decision, a never-checked constraint, and a long-ago-checked one. + + Three rows because the sweep's whole value is an ORDER, and an order + cannot be asserted with fewer. + """ + async with async_session() as s: + owner = await ensure_user(s, "sweep_owner") + uid = owner.id + await s.commit() + + book = await rulebooks_svc.create_rulebook(uid, "Sweep fixture") + topic = await rulebooks_svc.create_topic(book.id, uid, "mixed") + decision = await rulebooks_svc.create_rule( + topic.id, uid, "dev is home", "Work directly on dev.", + ) + never = await rulebooks_svc.create_rule( + topic.id, uid, "The runner has no bash", "Use POSIX sh.", + verify_with="read the workflow's shell setting", + ) + stale = await rulebooks_svc.create_rule( + topic.id, uid, "Bumps need a dashboard tick", "Tick it first.", + verify_with="cat CI-runner/renovate/config.js", + tier="conditional", + ) + async with async_session() as s: + row = await s.get(Rule, stale.id) + row.verified_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + await s.commit() + return { + "uid": uid, "decision": decision.id, + "never": never.id, "stale": stale.id, + } + + +async def test_a_rule_with_no_check_is_never_in_the_sweep(rulebook_of_three): + """The common case, and the one that keeps the list worth reading. + + Most rules are decisions. If they appeared here the sweep would be the + rulebook, and nobody would read it twice. + """ + rules = await rulebooks_svc.rules_due_for_verification(rulebook_of_three["uid"]) + assert rulebook_of_three["decision"] not in [r.id for r in rules] + + +async def test_never_checked_outranks_checked_long_ago(rulebook_of_three): + """NULLS FIRST is the ordering decision this surface turns on. + + Postgres sorts NULLs LAST by default on an ASC ordering, which would put + the rules nobody has ever confirmed at the BOTTOM — behind every rule + that at least once had someone look at it. That is exactly backwards: a + claim with no evidence at all outranks an old one. + """ + ids = [r.id for r in await rulebooks_svc.rules_due_for_verification( + rulebook_of_three["uid"] + )] + assert ids.index(rulebook_of_three["never"]) < ids.index(rulebook_of_three["stale"]) + + +async def test_verifying_a_rule_moves_it_off_the_top(rulebook_of_three): + """The loop closing: check it, stamp it, and it stops being the question.""" + await rulebooks_svc.mark_rule_verified( + rulebook_of_three["never"], rulebook_of_three["uid"], still_true=True, + ) + ids = [r.id for r in await rulebooks_svc.rules_due_for_verification( + rulebook_of_three["uid"] + )] + # Still present — verified is not retired, and it will come due again. + assert rulebook_of_three["never"] in ids + assert ids.index(rulebook_of_three["stale"]) < ids.index(rulebook_of_three["never"]) + + +async def test_a_failed_check_writes_nothing(rulebook_of_three): + """The asymmetry that keeps the sweep honest. + + There is no "verified false" state, because a rule whose check failed is + not in a special condition — it is WRONG. Recording the failure would let + it sit there being false with the sweep satisfied that someone looked. + """ + before = await _row(rulebook_of_three["stale"]) + await rulebooks_svc.mark_rule_verified( + rulebook_of_three["stale"], rulebook_of_three["uid"], still_true=False, + ) + after = await _row(rulebook_of_three["stale"]) + assert after.verified_at == before.verified_at + + +async def test_a_rule_with_no_check_cannot_be_verified(rulebook_of_three): + """Nothing to verify is a different answer from verified — and stamping + one would put a decision into a sweep it has no business being in.""" + assert await rulebooks_svc.mark_rule_verified( + rulebook_of_three["decision"], rulebook_of_three["uid"], + ) is None + + +async def test_never_only_and_the_age_filter_narrow_to_what_they_say(rulebook_of_three): + uid = rulebook_of_three["uid"] + # Membership, not equality: the integration lane shares one database for + # the whole run and this fixture is function-scoped, so this owner has + # accumulated rules from earlier tests. Asserting the exact list would + # pass alone and fail in the suite. + never_ids = [r.id for r in await rulebooks_svc.rules_due_for_verification( + uid, never_only=True, + )] + assert rulebook_of_three["never"] in never_ids + assert rulebook_of_three["stale"] not in never_ids + assert rulebook_of_three["decision"] not in never_ids + + # A rule checked in January is well past any sane window; one never + # checked always qualifies, because it is the most overdue thing there is. + aged = [r.id for r in await rulebooks_svc.rules_due_for_verification( + uid, older_than_days=30, + )] + assert rulebook_of_three["stale"] in aged + assert rulebook_of_three["never"] in aged + + +async def test_the_tier_filter_narrows_to_one_tier(rulebook_of_three): + ids = [r.id for r in await rulebooks_svc.rules_due_for_verification( + rulebook_of_three["uid"], tier="conditional", + )] + assert rulebook_of_three["stale"] in ids + assert rulebook_of_three["never"] not in ids + + +async def test_another_users_rules_are_not_in_your_sweep(rulebook_of_three): + """Rules are ownership-scoped: there is no rule-sharing ACL in this + schema, so the only correct answer is your own rules.""" + async with async_session() as s: + stranger = await ensure_user(s, "sweep_stranger") + sid = stranger.id + await s.commit() + + assert await rulebooks_svc.rules_due_for_verification(sid) == [] diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 1c68ffb..c922a06 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -217,13 +217,20 @@ async def test_unsubscribe_project_from_rulebook_calls_service(): assert mock.called -def test_register_attaches_all_sixteen_tools(): - """register(mcp) should call mcp.tool(name=...) for all 16 tools.""" +def test_register_attaches_every_tool(): + """Every tool in the module reaches the server. + + The count is the guard: a function added to the module but left out of + register()'s tuple is invisible to callers and raises nothing. The name + said "sixteen" for ten tools' worth of growth — the number lives in the + assertion, not the title, so it cannot drift again. + """ from scribe.mcp.tools.rulebooks import register mcp = FakeMCP() register(mcp) - assert len(mcp.names) == 26 # +relate_rules/unrelate_rules (milestone 307) + # 26 through milestone 307, +2 for the staleness sweep (milestone 312). + assert len(mcp.names) == 28 # spot-check a few names assert "list_rulebooks" in mcp.names assert "create_rule" in mcp.names @@ -234,6 +241,9 @@ def test_register_attaches_all_sixteen_tools(): assert "include_always_on_rulebook" in mcp.names assert "create_project_rule" in mcp.names assert "suppress_rule_for_project" in mcp.names + # milestone 312: the sweep, and the stamp that answers it + assert "rules_due_for_verification" in mcp.names + assert "mark_rule_verified" in mcp.names assert "unsuppress_rule_for_project" in mcp.names assert "suppress_topic_for_project" in mcp.names assert "unsuppress_topic_for_project" in mcp.names diff --git a/tests/test_services_rulebooks.py b/tests/test_services_rulebooks.py index 69aba3b..eeaeb57 100644 --- a/tests/test_services_rulebooks.py +++ b/tests/test_services_rulebooks.py @@ -446,3 +446,62 @@ def test_the_check_text_itself_never_enters_a_listing(): )) assert "verify_with" not in out assert "expires_when" not in out + + +# ── the sweep's row shape (milestone 312 step 3) ──────────────────────── + +def test_a_sweep_row_carries_the_check_in_full(): + """The OPPOSITE call from rule_brief, and deliberately so. + + A listing omits the depth because nobody reading it wants to act on one + rule. A sweep row exists to be acted on — the reader is about to go and + run the check — so the text is the payload's point, not its bloat. + """ + from scribe.services.rulebooks import verification_row + + row = verification_row(fake_rule( + verify_with="cat CI-runner/renovate/config.js", + expires_when="dependencyDashboardApproval is turned off", + when_to_apply="when a dependency bump is in play", + )) + assert row["verify_with"] == "cat CI-runner/renovate/config.js" + assert row["expires_when"] == "dependencyDashboardApproval is turned off" + assert row["when_to_apply"] == "when a dependency bump is in play" + assert row["tier"] == "always_on" + + +def test_never_verified_reports_no_day_count_rather_than_zero(): + """"Never" is not "0 days ago" — the second reads as freshly checked. + + Getting this wrong would invert the row's meaning for exactly the rules + that most need attention. + """ + from scribe.services.rulebooks import verification_row + + row = verification_row(fake_rule(verify_with="read the workflow")) + assert row["last_verified"] == "never" + assert row["days_since_verified"] is None + + +def test_a_verified_row_counts_the_days(): + from datetime import timedelta + + from scribe.services.rulebooks import verification_row + + row = verification_row(fake_rule( + verify_with="read the workflow", + verified_at=datetime.now(timezone.utc) - timedelta(days=74, hours=1), + )) + assert row["days_since_verified"] == 74 + + +@pytest.mark.asyncio +async def test_an_unrecognised_tier_filter_raises_rather_than_narrowing(): + """_valid_tier's silent always_on fallback is right for a WRITE — a typo + should leave a rule binding. It is wrong for a FILTER, where the same + fallback would quietly answer a different question than the one asked and + return a short list that looks like good news.""" + from scribe.services.rulebooks import rules_due_for_verification + + with pytest.raises(ValueError, match="tier must be one of"): + await rules_due_for_verification(7, tier="occasionally") From 35c632f8343d6fd6981d8e561ae0d6a6e4d6979c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 11:29:10 -0400 Subject: [PATCH 5/8] docs(rules): a project rule is shaped differently, not just scoped differently (milestone 312) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three surfaces already agree on WHERE a rule goes — the using-scribe skill's "Where a new rule goes" section and both tool docstrings frame it as one question, who should this bind. What they did not say is that the two homes want differently SHAPED rules, and one deferral was actively misleading. `create_project_rule` said `tier: "always_on" or "conditional" — see create_rule`. That imports a bar calibrated for a different blast radius. On a rulebook rule always_on means every session in every project, so the test is severe: the trigger must be nameless. A project rule is already scoped by construction, so always_on costs only that project's sessions — and being specific, which the family test treats as the signal for conditional, is what project rules are FOR. The instance's own data says so: rules 78, 115 and 119 are all project rules and all always_on. Not zero bar, a different one: conditional is right when the rule is about one AREA of a large project, because forty always-on rules on one project reproduces locally the preload bloat milestone 307 fixed globally. Also: - create_rule now says to write the general form WITHOUT hedging for exceptions — a project needing to narrow it writes its own and links with overrides/elaborates. A rulebook rule padded with "unless…" for two projects is two project rules that were never written. Only the project side mentioned that relationship; the side that benefits from it did not. - arose_from_id: reach for it harder on a project rule, which usually comes from one traceable incident in the repo, where a family rule is more often a standing preference with no single origin. - system_ids is worth setting on a project rule too — it is what lets a conditional one arrive with its area. - when_to_apply no longer claims to "decide" the tier here, which stopped being true one entry down. Co-Authored-By: Claude Opus 5 (1M context) --- src/scribe/mcp/tools/rulebooks.py | 41 +++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 43bd216..ee5da8e 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -297,6 +297,13 @@ async def create_rule( rulebook+topic ceremony). If it's a standard a CATEGORY of projects shares, put it in a themed subscribed rulebook, not the always-on one. + Write it general WITHOUT hedging for the exceptions. A project that needs + to strengthen, narrow or replace this rule writes its own and links it + with relate_rules(kind="overrides"), and one that adds local specifics + uses "elaborates" — so the general form does not have to anticipate every + project it will ever reach. A rulebook rule padded with "unless…" clauses + for two projects is two project rules that were never written. + Before writing a rule at all, check whether another entity already models the thing. A rule is prose an agent must remember and apply; the others are structure a tool can resolve, render and check. Visual standards are a @@ -412,12 +419,36 @@ async def create_project_rule( title: Short imperative title. If empty, derived from the first ~50 characters of statement. when_to_apply: WHEN this rule fires — the trigger, not the - instruction. See create_rule; it decides the tier and it is how - the rule is found at the moment it matters. - tier: "always_on" (default) or "conditional" — see create_rule. + instruction, and the rule's retrieval surface: name the SYMPTOM, + the words someone would type while stuck. See create_rule for the + full argument. It informs the tier below rather than deciding it, + since a project rule's tier turns on area-scope, not on whether + the trigger can be named. + tier: "always_on" (default) or "conditional". The SAME two values as + create_rule, judged against a different cost — do not import that + tool's test wholesale. There, always_on means every session in + every project, so the bar is high: the trigger must be nameless + ("whenever you are working"). Here the rule is already scoped to + one project by construction, so always_on costs only that + project's sessions and the bar is correspondingly lower. A + project rule that names something specific is still ordinarily + always_on — being specific is what project rules are FOR. + Reach for conditional when the rule is about one AREA of a large + project — a CI quirk, a migration gotcha, one subsystem's + convention — so it arrives with that area instead of resident in + every session. The failure to avoid is local: forty always-on + rules on one project reproduces, inside that project, exactly the + preload bloat that made every rule compete for the same budget. system_ids: Ids from list_canonical_systems — the global AREAS this - rule is about. - arose_from_id: The note or task that CAUSED this rule. + rule is about. Worth setting even on a project rule: it is what + lets a conditional one surface when the project is working in + that area. + arose_from_id: The note or task that CAUSED this rule. Reach for it + harder here than on a rulebook rule — a project rule usually + comes from one traceable incident in this repo, where a family + rule is more often a standing preference with no single origin. + The link is what lets a later reader judge whether the incident + still describes the project. why: Optional rationale — the reason the rule exists. how_to_apply: Optional operationalization — when / where it kicks in. verify_with: How to check this rule is still true — see create_rule. From 3345be84d1c51d5b713146ceb1e2c0ac5b82b5d5 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 11:53:35 -0400 Subject: [PATCH 6/8] feat(rules): the check is editable, visible, and sweepable in the UI (#3098, milestone 312 step 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 27 — the milestone was backend-only until this. Four surfaces: RULE EDITOR — verify_with and expires_when under a legend that asks the actual question ("Can this rule go stale?") and says empty is the normal answer, because most rules are decisions and a form that implies a missing field would get them filled in out of tidiness. When the SAVED rule carries a check, the stamp shows with Still true / No longer true beside it. The stamp reads the stored value, not the draft: an unsaved edit to the textarea has not been run against anything. SWEEP PANE — its own surface, not a filter on the rule list. That list can only ever show one topic of one rulebook, and a rule that has gone false belongs to no one rulebook; filtering it would under-report, which is the failure this whole surface exists to catch. Reached from the rulebook list, below the rulebooks, because that is where you go to look at rules. RULE ROWS — a chip only on rules carrying a check, so its presence is the signal. PROJECT RULES TAB — the check shows beside `why` when a rule has one, read-only: that tab is the project's view of what binds it. NO AGE-GRADED COLOUR anywhere, deliberately. The sweep is already ordered by urgency, so a red/amber ramp would restate the ordering AND require an invented "stale after N days" threshold — a magic number nobody could defend and the first thing to go out of date. --fs-overdue is error red and reserved for a broken promise like a missed due date; a verification age is not one, and colouring it that way makes a rule someone just wrote look broken. Only "never" is marked, because it is categorically different from a date rather than a worse one — and it is marked by weight, not hue. An empty sweep says "Nothing to check", not nothing: good news must not read as a broken page. Two chips (tier, then verification) turned out byte-identical, so .rule-chip moves to rules-shared.css and snippet #2906 is updated to match rather than left describing a file that has moved on. Its header comment counted the panes it served; that count went stale the moment a fourth arrived, so it no longer counts. Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/assets/rules-shared.css | 22 ++- .../src/components/rules/ProjectRulesTab.vue | 35 +++- .../components/rules/RuleEditorSlideOver.vue | 100 ++++++++++ .../src/components/rules/RuleListPane.vue | 31 +-- .../src/components/rules/RuleSweepPane.vue | 180 ++++++++++++++++++ .../src/components/rules/RulebookListPane.vue | 25 ++- frontend/src/stores/rulebooks.ts | 50 ++++- frontend/src/views/RulesView.vue | 25 ++- 8 files changed, 447 insertions(+), 21 deletions(-) create mode 100644 frontend/src/components/rules/RuleSweepPane.vue diff --git a/frontend/src/assets/rules-shared.css b/frontend/src/assets/rules-shared.css index 9efc62b..72c1448 100644 --- a/frontend/src/assets/rules-shared.css +++ b/frontend/src/assets/rules-shared.css @@ -1,5 +1,7 @@ -/* Shared by the three rules panes (RulebookListPane, RuleListPane, - RulebookDetailPane): the pane surface and its heading. Load with +/* Shared by the rules panes (RulebookListPane, RuleListPane, + RulebookDetailPane, RuleSweepPane): the pane surface, its heading, and the + title chip. Counting them in this comment went stale the first time a + fourth was added, so it no longer does. Load with diff --git a/frontend/src/components/rules/RuleListPane.vue b/frontend/src/components/rules/RuleListPane.vue index 78ba14a..6c2a662 100644 --- a/frontend/src/components/rules/RuleListPane.vue +++ b/frontend/src/components/rules/RuleListPane.vue @@ -17,7 +17,17 @@ const emit = defineEmits<{ {{ r.title }} - conditional + conditional + + {{ r.last_verified === "never" ? "unverified" : `checked ${r.last_verified}` }}
{{ r.statement }}
@@ -47,16 +57,13 @@ li:hover { background: var(--fs-surface-hover); } .meta { display: flex; align-items: baseline; gap: 0.5rem; margin-top: 0.35rem; font-size: 0.75em; } .trigger { flex: 1; min-width: 0; color: var(--fs-text-secondary); overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } .age { color: var(--fs-text-tertiary); font-variant-numeric: tabular-nums; flex-shrink: 0; } -.tier-chip { - margin-left: 0.4rem; - font-family: var(--fs-font-body); - font-style: normal; - font-size: 0.62rem; - color: var(--fs-text-secondary); - background: var(--fs-surface-raised); - border-radius: var(--fs-radius-pill); - padding: 0.05rem 0.4rem; - vertical-align: middle; -} +/* Only the departures from .rule-chip (rules-shared.css) live here. */ +.check-chip { font-variant-numeric: tabular-nums; } +/* No age-graded colour on purpose. The sweep is already ordered by urgency, so + a red/amber ramp would restate the ordering AND require an invented "stale + after N days" threshold — a magic number nobody could defend and the first + thing to go out of date. Only "never" is marked, because it is categorically + different from a date rather than a worse one. */ +.check-chip.unchecked { font-style: italic; color: var(--fs-text-tertiary); } .new-rule { cursor: pointer; } diff --git a/frontend/src/components/rules/RuleSweepPane.vue b/frontend/src/components/rules/RuleSweepPane.vue new file mode 100644 index 0000000..f072500 --- /dev/null +++ b/frontend/src/components/rules/RuleSweepPane.vue @@ -0,0 +1,180 @@ + + + + + diff --git a/frontend/src/components/rules/RulebookListPane.vue b/frontend/src/components/rules/RulebookListPane.vue index b808c3d..f76effe 100644 --- a/frontend/src/components/rules/RulebookListPane.vue +++ b/frontend/src/components/rules/RulebookListPane.vue @@ -3,8 +3,8 @@ import { ref } from "vue"; import { useRulebooksStore } from "@/stores/rulebooks"; import type { Rulebook } from "@/api/rulebooks"; -defineProps<{ rulebooks: Rulebook[]; selectedId: number | null }>(); -const emit = defineEmits<{ select: [id: number] }>(); +defineProps<{ rulebooks: Rulebook[]; selectedId: number | null; sweepActive: boolean }>(); +const emit = defineEmits<{ select: [id: number]; "select-sweep": [] }>(); const store = useRulebooksStore(); const isCreating = ref(false); @@ -34,6 +34,18 @@ async function submitNew() { always on + + +
@@ -63,6 +75,15 @@ li:hover { background: var(--fs-surface-hover); } color: var(--fs-text-on-action); margin-left: auto; } +.sweep-entry { + display: block; width: 100%; text-align: left; + margin-top: var(--fs-space-3); + padding: 0.5rem; border-radius: 6px; + background: none; border: 1px dashed var(--fs-border-color); + color: var(--fs-text-secondary); font: inherit; cursor: pointer; +} +.sweep-entry:hover { background: var(--fs-surface-hover); } +.sweep-entry.active { background: var(--fs-accent-soft); color: var(--fs-text-primary); } .new-rulebook { margin-top: 1rem; } .new-rulebook input { width: 100%; margin-bottom: 0.5rem; diff --git a/frontend/src/stores/rulebooks.ts b/frontend/src/stores/rulebooks.ts index cfaa8ea..a02be56 100644 --- a/frontend/src/stores/rulebooks.ts +++ b/frontend/src/stores/rulebooks.ts @@ -9,6 +9,11 @@ export const useRulebooksStore = defineStore("rulebooks", () => { const topicsByRulebook = ref>({}); const rulesByTopic = ref>({}); const currentRule = ref(null); + const rulesDue = ref([]); + // Kept so a verify re-reads the sweep with the SAME filters the operator is + // looking at — re-fetching unfiltered would silently widen the list under + // them at the moment they acted on it. + const lastSweepOpts = ref<{ olderThanDays?: number; tier?: api.RuleTier; neverOnly?: boolean }>({}); const loading = ref(false); async function fetchRulebooks() { @@ -111,6 +116,13 @@ export const useRulebooksStore = defineStore("rulebooks", () => { updated_at: rule.updated_at, when_to_apply: rule.when_to_apply || undefined, arose_from_id: rule.arose_from_id ?? undefined, + // Mirrors services.rulebooks.last_verified_label: present ONLY when the + // rule carries a check, and "never" rather than absent when it has one + // nobody has run. Computed here so a row just written looks identical to + // the same row re-fetched, instead of losing its chip until a reload. + last_verified: rule.verify_with + ? (rule.verified_at ? rule.verified_at.slice(0, 10) : "never") + : undefined, }; } @@ -148,6 +160,41 @@ export const useRulebooksStore = defineStore("rulebooks", () => { await fetchRule(refreshRuleId); } + /** The staleness sweep: rules asserting a fact, oldest verification first. */ + async function fetchRulesDue(opts: { + olderThanDays?: number; tier?: api.RuleTier; neverOnly?: boolean; + } = {}) { + loading.value = true; + lastSweepOpts.value = opts; + try { + const data = await api.listRulesDueForVerification(opts); + rulesDue.value = data.rules; + } finally { + loading.value = false; + } + } + + /** + * Record that a rule's check was RUN, and what it said. + * + * A pass re-sorts the row to the back of the sweep, so the list is re-read + * rather than patched: the whole point of this surface is an ORDER, and a + * locally-mutated row would sit in its old position claiming a new date. + * A failure writes nothing server-side and the row keeps its place — also + * correct, and also what a re-read shows. + */ + async function verifyRule(id: number, stillTrue: boolean) { + const rule = await api.markRuleVerified(id, stillTrue); + if (currentRule.value?.id === id) currentRule.value = rule; + for (const tid of Object.keys(rulesByTopic.value)) { + const list = rulesByTopic.value[Number(tid)]; + const idx = list.findIndex((r) => r.id === id); + if (idx >= 0) list[idx] = toHeader(rule); + } + if (rulesDue.value.length) await fetchRulesDue(lastSweepOpts.value); + return rule; + } + async function deleteRule(id: number) { await api.deleteRule(id); if (currentRule.value?.id === id) currentRule.value = null; @@ -157,10 +204,11 @@ export const useRulebooksStore = defineStore("rulebooks", () => { } return { - rulebooks, topicsByRulebook, rulesByTopic, currentRule, loading, + rulebooks, topicsByRulebook, rulesByTopic, currentRule, rulesDue, lastSweepOpts, loading, fetchRulebooks, fetchTopics, fetchRules, fetchRule, createRulebook, updateRulebook, toggleAlwaysOn, deleteRulebook, createTopic, updateTopic, deleteTopic, createRule, updateRule, deleteRule, relateRules, unrelateRules, + fetchRulesDue, verifyRule, }; }); diff --git a/frontend/src/views/RulesView.vue b/frontend/src/views/RulesView.vue index 1236267..c201e2e 100644 --- a/frontend/src/views/RulesView.vue +++ b/frontend/src/views/RulesView.vue @@ -6,6 +6,7 @@ import RulebookListPane from "@/components/rules/RulebookListPane.vue"; import RulebookDetailPane from "@/components/rules/RulebookDetailPane.vue"; import RuleListPane from "@/components/rules/RuleListPane.vue"; import RuleEditorSlideOver from "@/components/rules/RuleEditorSlideOver.vue"; +import RuleSweepPane from "@/components/rules/RuleSweepPane.vue"; const store = useRulebooksStore(); const route = useRoute(); @@ -15,6 +16,7 @@ const selectedRulebookId = ref(null); const selectedTopicId = ref(null); const editingRuleId = ref(null); const creatingRuleForTopic = ref(null); +const sweepActive = ref(false); function syncFromRoute() { const rb = route.query.rb ? Number(route.query.rb) : null; @@ -23,9 +25,20 @@ function syncFromRoute() { selectedRulebookId.value = rb; selectedTopicId.value = topic; editingRuleId.value = rule; + sweepActive.value = route.query.view === "due"; +} + +function selectSweep() { + sweepActive.value = true; + // Keeps ?rule=… so the editor survives the mode switch, and drops the + // rulebook/topic selection the sweep does not use. + const { rb, topic, ...rest } = route.query; + void rb; void topic; + router.replace({ query: { ...rest, view: "due" } }); } function selectRulebook(id: number) { + sweepActive.value = false; selectedRulebookId.value = id; selectedTopicId.value = null; router.replace({ query: { rb: String(id) } }); @@ -70,10 +83,13 @@ watch(() => route.query, syncFromRoute); + route.query, syncFromRoute);

Select a rulebook to view its topics.

-
+

Select a topic to view its rules.

route.query, syncFromRoute); gap: 1px; background: var(--fs-border-color); } +/* The sweep is cross-cutting, so it takes the width the rulebook + topic + panes would have used rather than being squeezed into one column. */ +.sweep-span { grid-column: 2 / -1; } .pane.empty { background: var(--fs-surface-hover); padding: 1rem; From e2e64b94c07943cc8877d7a878916560619d4ab9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 12:02:41 -0400 Subject: [PATCH 7/8] =?UTF-8?q?feat(tasks):=20task=5Fkind=20gains=20'spike?= =?UTF-8?q?'=20=E2=80=94=20the=20investigation,=20not=20the=20change=20(#3?= =?UTF-8?q?099,=20milestone=20312=20step=205)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A spike is a shape the other kinds cannot hold. `work` ships a change; `issue` fixes something broken. A spike is time-boxed and its output is KNOWLEDGE — it succeeds by producing an answer, and nothing ships at the end of it. Filing one as `work` makes a finished investigation look like an abandoned change, which is why the distinction earns a value rather than a convention. It is also the record a failed check asks for. This milestone gave rules a verify_with; when one fails the rule is wrong, and the next move is often to go and find out what replaced it. notes.arose_from_id already exists (0065), so constraint -> spike provenance needed no schema at all — only a docstring saying it is there. Rule 36: the value and the widened CHECK land in the same migration, DROP then ADD, exactly as 0065 did for 'issue'. The two whitelists live in one tuple each so upgrade and downgrade cannot disagree about what the list was on either side. The downgrade demotes existing spikes to 'work' first — lossy, deliberately, because the alternative is a downgrade that fails on real data, and one that says what it did beats one that cannot run. 'plan' stays whitelisted though retired: historical plan-tasks carry it, and a row that cannot be rewritten cannot be edited, restored or migrated. The integration test asserts both halves. A test that only proved 'spike' is accepted would pass just as happily against a table whose CHECK had been dropped and never re-added — which is the other way rule 36's failure happens — so an unknown kind is asserted to still raise. Not in scope, deliberately: any special lifecycle, time-box enforcement, or gating relationship. It is a kind, not a workflow. Co-Authored-By: Claude Opus 5 (1M context) --- alembic/versions/0091_task_kind_spike.py | 66 +++++++++++++++++++++ frontend/src/types/note.ts | 11 +++- frontend/src/views/TaskEditorView.vue | 1 + src/scribe/mcp/tools/systems.py | 3 +- src/scribe/mcp/tools/tasks.py | 25 +++++--- src/scribe/models/note.py | 10 +++- tests/test_integration_task_kind_spike.py | 71 +++++++++++++++++++++++ tests/test_mcp_tool_tasks_kind.py | 14 +++++ 8 files changed, 190 insertions(+), 11 deletions(-) create mode 100644 alembic/versions/0091_task_kind_spike.py create mode 100644 tests/test_integration_task_kind_spike.py diff --git a/alembic/versions/0091_task_kind_spike.py b/alembic/versions/0091_task_kind_spike.py new file mode 100644 index 0000000..5b03374 --- /dev/null +++ b/alembic/versions/0091_task_kind_spike.py @@ -0,0 +1,66 @@ +"""task_kind gains 'spike' — the investigation, not the change +(milestone 312 step 5) + +Revision ID: 0091 +Revises: 0090 +Create Date: 2026-08-27 + +A spike is a task shape the others cannot hold. `work` ships a change; +`issue` fixes something broken. A spike is time-boxed and its output is +KNOWLEDGE — it succeeds by producing an answer, and nothing ships at the +end of it. "Find out whether the runner can be given a bash shell" is not +work, and filing it as work makes a finished investigation look like an +abandoned change. + +It is the record a failed check asks for. Milestone 312 gave rules a +`verify_with`; when one of those fails, the rule is wrong and the next move +is often to go and find out what replaced it. `notes.arose_from_id` already +exists (0065), so that constraint -> spike link needs no further schema. + +Rule 36: `task_kind` is gated by a CHECK whitelist, so the value and the +widened constraint land in the SAME migration — DROP then ADD, exactly as +0065 did when it introduced 'issue'. Adding the value and constraining it +later leaves a window where the database accepts anything. + +'plan' stays in the list though it is retired (plans are milestones since +0066): historical plan-tasks still carry it, and dropping it from the +whitelist would make old rows unwritable. +""" +from alembic import op + +revision = "0091" +down_revision = "0090" +branch_labels = None +depends_on = None + +# One tuple so the upgrade and the downgrade cannot disagree about what the +# list was on either side of this migration. +_KINDS_AFTER = ("work", "plan", "issue", "spike") +_KINDS_BEFORE = ("work", "plan", "issue") + + +# Restated rather than imported from 0088, which has the same helper. A +# migration is a snapshot: it must keep working when the code around it has +# moved on, so it never imports from live modules or from its siblings. Six +# duplicated lines are the price of that, and the cheap half of the bargain. +def _in_list(values: tuple[str, ...]) -> str: + return "task_kind IN (" + ", ".join(f"'{v}'" for v in values) + ")" + + +def upgrade() -> None: + op.drop_constraint("notes_task_kind_check", "notes", type_="check") + op.create_check_constraint( + "notes_task_kind_check", "notes", _in_list(_KINDS_AFTER), + ) + + +def downgrade() -> None: + # Any row already filed as a spike would violate the narrowed constraint, + # so they are demoted to 'work' first. Lossy and deliberately so: the + # alternative is a downgrade that fails on real data, which is worse than + # a downgrade that says what it did. + op.execute("UPDATE notes SET task_kind = 'work' WHERE task_kind = 'spike'") + op.drop_constraint("notes_task_kind_check", "notes", type_="check") + op.create_check_constraint( + "notes_task_kind_check", "notes", _in_list(_KINDS_BEFORE), + ) diff --git a/frontend/src/types/note.ts b/frontend/src/types/note.ts index 64105e4..c11a9d3 100644 --- a/frontend/src/types/note.ts +++ b/frontend/src/types/note.ts @@ -2,7 +2,16 @@ import type { System } from "@/api/systems"; export type TaskStatus = "todo" | "in_progress" | "done" | "cancelled"; export type TaskPriority = "none" | "low" | "medium" | "high"; -export type TaskKind = "work" | "plan" | "issue"; +/** + * What KIND of work a task is, not how it is going. + * work — ships a change (default) + * issue — corrective; something was broken + * spike — time-boxed, output is knowledge; it succeeds by producing an + * answer and nothing ships at the end of it + * plan — retired (plans are milestones); kept so historical plan-tasks + * still render their kind + */ +export type TaskKind = "work" | "plan" | "issue" | "spike"; export type NoteType = "note" | "process" | "snippet"; export interface Note { diff --git a/frontend/src/views/TaskEditorView.vue b/frontend/src/views/TaskEditorView.vue index f19115f..4212ca5 100644 --- a/frontend/src/views/TaskEditorView.vue +++ b/frontend/src/views/TaskEditorView.vue @@ -578,6 +578,7 @@ useEditorGuards(dirty, save);