From c61925be76354f1f254d92d94ac6b7fb46853467 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 09:28:03 -0400 Subject: [PATCH] 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