feat(rules): the write path carries a rule's check, and empty finally means empty (#3096, milestone 312 step 2)
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 45s
CI & Build / Build & push image (push) Skipped
CI & Build / integration (push) Successful in 29s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 45s
CI & Build / Build & push image (push) Skipped
CI & Build / integration (push) Successful in 29s
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user