feat(rules): the history is readable — service, REST and MCP (#3242, milestone 323 step 3)
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 14s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / integration (push) Successful in 38s
CI & Build / Python tests (push) Successful in 1m7s
CI & Build / Build & push image (push) Successful in 30s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 14s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / integration (push) Successful in 38s
CI & Build / Python tests (push) Successful in 1m7s
CI & Build / Build & push image (push) Successful in 30s
`list_rule_versions` / `get_rule_version` in the rulebooks service, a pair of REST routes beside the note-version ones, and an MCP `rule_history` tool. The ACL-scoped reads live in services/rulebooks.py rather than in services/rule_versions.py because rulebooks already imports rule_versions for the write path and the reverse would be a cycle. It is also the honest split: rule_versions owns what a version IS, rulebooks owns who may read one. Scoping is through the RULE, never the version's user_id, and both directions of that mistake are now pinned by tests. That column is the ACTOR — scoping by it would hand someone the snapshots they personally wrote on a rule that has since moved out of their reach, and would hide from the rule's owner every edit anyone else made. `get_rule_version` takes the rule id as well as the version id so the ownership check and the fetch agree about which rule is in play; the test for that uses a second rule the caller genuinely owns, because a nonexistent id would pass on the ownership check alone and prove nothing. An unreadable rule returns None, not an empty list. The two mean different things — "not your rule" versus "never reworded" — and the MCP tool keeps them apart: None raises, empty says so in band. THE DIFF QUESTION, ANSWERED — and the task's premise was half wrong. It says "notes have DiffView.vue and a diff endpoint already". The component exists and is reusable as-is: it takes `DiffLine[]` and nothing note-shaped, so step 4 can render a rule diff with it unchanged. The ENDPOINT does not exist — diffs are computed client-side by `computeDiff` in useAssist.ts. So no diff route is needed here, and none was written. For the MCP door the answer is different again: an agent has no client to compute a diff, but it also does not need one. Each entry holds the text the edit REPLACED, so "what did this say before the most recent change?" is the first entry, and the text that change produced is the rule as it stands. The docstring says so, and a test pins that sentence — read the other way round, every diff comes out backwards. No restore, per the task. Putting an old wording back goes through update_rule, which snapshots what it replaces, so the undo stays visible like any other edit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -229,8 +229,9 @@ def test_register_attaches_every_tool():
|
||||
mcp = FakeMCP()
|
||||
|
||||
register(mcp)
|
||||
# 26 through milestone 307, +2 for the staleness sweep (milestone 312).
|
||||
assert len(mcp.names) == 28
|
||||
# 26 through milestone 307, +2 for the staleness sweep (milestone 312),
|
||||
# +1 for a rule's edit history (milestone 323).
|
||||
assert len(mcp.names) == 29
|
||||
# spot-check a few names
|
||||
assert "list_rulebooks" in mcp.names
|
||||
assert "create_rule" in mcp.names
|
||||
@@ -244,6 +245,8 @@ def test_register_attaches_every_tool():
|
||||
# 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
|
||||
# milestone 323: what a rule used to say
|
||||
assert "rule_history" 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
|
||||
@@ -433,3 +436,115 @@ async def test_unrelate_rules_raises_when_the_edge_is_gone():
|
||||
from scribe.mcp.tools.rulebooks import unrelate_rules
|
||||
with pytest.raises(ValueError, match="not found"):
|
||||
await unrelate_rules(relation_id=99)
|
||||
|
||||
|
||||
# ── rule_history (milestone 323 step 3) ────────────────────────────────
|
||||
|
||||
def _fake_version(**over):
|
||||
"""A RuleVersion-shaped stand-in. A real model instance rather than a
|
||||
MagicMock, because the tool calls `to_dict` and a mock would hand back
|
||||
another mock instead of failing."""
|
||||
from scribe.models.rule_version import RuleVersion
|
||||
from datetime import datetime, timezone
|
||||
|
||||
defaults = {
|
||||
"id": 5, "rule_id": 100, "user_id": 1,
|
||||
"title": "The runner has no bash", "statement": "Use sh.",
|
||||
"why": "the image ships no bash", "how_to_apply": None,
|
||||
"when_to_apply": None, "tier": "always_on",
|
||||
"verify_with": "read the workflow's shell setting",
|
||||
"expires_when": None,
|
||||
"created_at": datetime(2026, 8, 29, tzinfo=timezone.utc),
|
||||
}
|
||||
return RuleVersion(**{**defaults, **over})
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rule_history_lists_without_the_heavy_text():
|
||||
"""The listing answers "when, and by whom". A rule's statement runs to
|
||||
thousands of characters, so a history carrying every field would cost
|
||||
more to read than the answer is worth."""
|
||||
with patch(
|
||||
"scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions",
|
||||
AsyncMock(return_value=[_fake_version(), _fake_version(id=4)]),
|
||||
):
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
out = await rule_history(rule_id=100)
|
||||
|
||||
assert out["total"] == 2
|
||||
assert out["versions"][0]["title"] == "The runner has no bash"
|
||||
assert "statement" not in out["versions"][0]
|
||||
assert "why" not in out["versions"][0]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rule_history_opens_one_version_in_full():
|
||||
"""Passing a version id switches from the index to the text — which is
|
||||
the whole reason the listing can afford to omit it."""
|
||||
with patch(
|
||||
"scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule_version",
|
||||
AsyncMock(return_value=_fake_version()),
|
||||
):
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
out = await rule_history(rule_id=100, version_id=5)
|
||||
|
||||
assert out["statement"] == "Use sh."
|
||||
assert out["verify_with"] == "read the workflow's shell setting"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rule_history_says_an_empty_history_is_ordinary():
|
||||
"""Most rules have never been reworded, and nothing was written before
|
||||
milestone 323. Without this line an empty list reads as a lost history or
|
||||
a broken tool."""
|
||||
with patch(
|
||||
"scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions",
|
||||
AsyncMock(return_value=[]),
|
||||
):
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
out = await rule_history(rule_id=100)
|
||||
|
||||
assert out["total"] == 0
|
||||
assert "never been reworded" in out["note"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rule_history_raises_when_the_rule_is_not_yours():
|
||||
"""None from the service means "not readable", and the tool must not
|
||||
turn that into an empty history — which would read as "this rule has no
|
||||
past" rather than "this is not your rule"."""
|
||||
with patch(
|
||||
"scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions",
|
||||
AsyncMock(return_value=None),
|
||||
):
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
with pytest.raises(ValueError, match="rule 100 not found"):
|
||||
await rule_history(rule_id=100)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rule_history_raises_for_a_version_on_another_rule():
|
||||
with patch(
|
||||
"scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule_version",
|
||||
AsyncMock(return_value=None),
|
||||
):
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
with pytest.raises(ValueError, match="version 5 not found"):
|
||||
await rule_history(rule_id=100, version_id=5)
|
||||
|
||||
|
||||
def test_rule_history_docstring_says_what_a_version_HOLDS():
|
||||
"""The one thing a reader gets wrong unaided: an entry is the text the
|
||||
edit REPLACED, not the text it introduced. Read the other way, every
|
||||
diff comes out backwards — so the docstring has to say it, and this is
|
||||
the guard against a later tidy-up dropping the line."""
|
||||
from scribe.mcp.tools.rulebooks import rule_history
|
||||
|
||||
doc = rule_history.__doc__ or ""
|
||||
assert "REPLACED" in doc
|
||||
assert "no restore" in doc.lower(), (
|
||||
"the docstring no longer explains that a rule version cannot be "
|
||||
"restored. A caller who assumes a revert exists will look for one "
|
||||
"and, not finding it, is likely to hand-copy the old text back with "
|
||||
"no record of why."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user