Two milestones: a note can carry its own check (317), and a rule keeps what it used to say (323) #135

Merged
bvandeusen merged 23 commits from dev into main 2026-08-31 00:01:15 -04:00
11 changed files with 296 additions and 77 deletions
Showing only changes of commit efabba58dd - Show all commits
+20 -2
View File
@@ -96,7 +96,25 @@ Two constraints on *how* that's achieved:
not restraint. Only a record genuinely about no particular area goes
untagged.
8. **State updates in place; chronicles don't.** A dev-log records what
8. **Name the record, never just its number.** Whenever you refer to a Scribe
record — in a message to the operator, a commit message, a task body, a
work-log — write the id *and* its title: `#3244 "the staleness signal"`,
`milestone 323 "rule versioning"`. Not `#3244`.
You have the record open; the operator does not. A bare id reads as
complete to you and as homework to them — they have to look it up to know
what their own conversation is about, or guess. Scribe's own duplicate gate
already writes `id 412: "debounce helper"` for exactly this reason; match
it everywhere else.
The first mention in a message carries the title; later mentions of the
same record can use the bare id. If you don't know the title, look it up
before citing the number — an id you can't name is one you haven't checked.
This matters most in the places read later by someone with even less
context than the operator has now: commit messages, task bodies, and any
record that cites another.
9. **State updates in place; chronicles don't.** A dev-log records what
*happened* — write it once, never rewrite it. A durable finding (how a
subsystem works, a measured number) lives in that System's **reference
note** ("«System» — reference"), which you UPDATE as facts change — safe,
@@ -106,7 +124,7 @@ Two constraints on *how* that's achieved:
re-measurement, a reversed decision), pass the old id in `supersedes` so the
stale record is demoted and labelled rather than left competing.
9. **A few notes assert a FACT, and those can carry their own check.**
10. **A few notes assert a FACT, and those can carry their own check.**
Supersession only fires once somebody has read a note and disagreed — which
is the case where it was already believed. A note asserting something about
*someone else's* software — what a service does on a duplicate upload, how a
+8 -2
View File
@@ -137,11 +137,17 @@ async def delete_milestone(milestone_id: int) -> dict:
"""Move a milestone to the trash (recoverable). Its tasks go with it as one batch.
Restore via restore(batch_id)."""
uid = current_user_id()
# Read the title BEFORE the delete: afterwards the row is trashed and the
# confirmation could only echo the number back. A deletion the operator
# cannot recognise is one they cannot tell was the wrong one.
doomed = await milestones_svc.get_milestone(uid, milestone_id)
title = getattr(doomed, "title", "") if doomed else ""
batch = await trash_svc.delete(uid, "milestone", milestone_id)
if batch is None:
raise ValueError(f"milestone {milestone_id} not found")
return {"deleted_batch_id": batch,
"message": f"Milestone {milestone_id} + its tasks moved to trash. Restore with restore('{batch}')."}
return {"deleted": milestone_id, "title": title, "deleted_batch_id": batch,
"message": f'Milestone {milestone_id} ("{title}") and its tasks '
f"moved to trash. Restore with restore('{batch}')."}
def register(mcp) -> None:
+8 -2
View File
@@ -324,11 +324,17 @@ async def find_duplicate_records(kind: str = "note", threshold: float = 0.0) ->
async def delete_note(note_id: int) -> dict:
"""Move a Scribe note to the trash (recoverable). Restore via restore(batch_id)."""
uid = current_user_id()
# Read the title BEFORE the delete: afterwards the row is trashed and the
# confirmation could only echo the number back. A deletion the operator
# cannot recognise is one they cannot tell was the wrong one.
loaded = await notes_svc.get_note_for_user(uid, note_id)
title = getattr(loaded[0], "title", "") if loaded else ""
batch = await trash_svc.delete(uid, "note", note_id)
if batch is None:
raise ValueError(f"note {note_id} not found")
return {"deleted_batch_id": batch,
"message": f"Note {note_id} moved to trash. Restore with restore('{batch}')."}
return {"deleted": note_id, "title": title, "deleted_batch_id": batch,
"message": f'Note {note_id} ("{title}") moved to trash. '
f"Restore with restore('{batch}')."}
async def notes_due_for_verification(
+11 -6
View File
@@ -122,8 +122,9 @@ async def delete_rulebook(rulebook_id: int, confirmed: bool = False) -> dict:
"confirmed_required": True,
}
batch = await trash_svc.delete(uid, "rulebook", rulebook_id)
return {"deleted": rulebook_id, "deleted_batch_id": batch,
"message": f"Moved to trash. Restore with restore('{batch}')."}
return {"deleted": rulebook_id, "title": rb.title, "deleted_batch_id": batch,
"message": f'Rulebook {rulebook_id} ("{rb.title}") moved to trash. '
f"Restore with restore('{batch}')."}
# ── Topic CRUD ─────────────────────────────────────────────────────────
@@ -192,8 +193,9 @@ async def delete_topic(topic_id: int, confirmed: bool = False) -> dict:
"confirmed_required": True,
}
batch = await trash_svc.delete(uid, "topic", topic_id)
return {"deleted": topic_id, "deleted_batch_id": batch,
"message": f"Moved to trash. Restore with restore('{batch}')."}
return {"deleted": topic_id, "title": topic.title, "deleted_batch_id": batch,
"message": f'Topic {topic_id} ("{topic.title}") moved to trash. '
f"Restore with restore('{batch}')."}
# ── Rule CRUD ──────────────────────────────────────────────────────────
@@ -602,8 +604,10 @@ async def rule_history(rule_id: int, version_id: int = 0) -> dict:
versions = await rulebooks_svc.list_rule_versions(rule_id, uid)
if versions is None:
raise ValueError(f"rule {rule_id} not found")
rule = await rulebooks_svc.get_rule(rule_id, uid)
return {
"rule_id": rule_id,
"title": rule.title if rule else "",
"versions": [v.to_dict(include_text=False) for v in versions],
"total": len(versions),
# Said in-band because an empty list is the ordinary case and reads
@@ -632,8 +636,9 @@ async def delete_rule(rule_id: int, confirmed: bool = False) -> dict:
"confirmed_required": True,
}
batch = await trash_svc.delete(uid, "rule", rule_id)
return {"deleted": rule_id, "deleted_batch_id": batch,
"message": f"Moved to trash. Restore with restore('{batch}')."}
return {"deleted": rule_id, "title": rule.title, "deleted_batch_id": batch,
"message": f'Rule {rule_id} ("{rule.title}") moved to trash. '
f"Restore with restore('{batch}')."}
# ── Subscriptions ──────────────────────────────────────────────────────
+5 -1
View File
@@ -494,9 +494,13 @@ async def delete_snippet(snippet_id: int) -> dict:
that should survive, prefer merge_snippets — that keeps the call sites.
"""
uid = current_user_id()
# Read before deleting so the confirmation can NAME what went — an id
# alone leaves the operator unable to tell which snippet this was.
doomed = await snippets_svc.get_snippet(uid, snippet_id)
title = getattr(doomed, "title", "") if doomed else ""
if not await snippets_svc.delete_snippet(uid, snippet_id):
raise ValueError(f"snippet {snippet_id} not found")
return {"deleted": True, "id": snippet_id}
return {"deleted": True, "id": snippet_id, "title": title}
async def merge_snippets(target_id: int, source_ids: list[int]) -> dict:
+8 -2
View File
@@ -364,11 +364,17 @@ async def delete_task(task_id: int) -> dict:
"""Move a Scribe task (or plan) to the trash (recoverable). Sub-tasks go with it.
Restore via restore(batch_id)."""
uid = current_user_id()
# Read the title BEFORE the delete: afterwards the row is trashed and the
# confirmation could only echo the number back. A deletion the operator
# cannot recognise is one they cannot tell was the wrong one.
loaded = await notes_svc.get_note_for_user(uid, task_id)
title = getattr(loaded[0], "title", "") if loaded else ""
batch = await trash_svc.delete(uid, "task", task_id)
if batch is None:
raise ValueError(f"task {task_id} not found")
return {"deleted_batch_id": batch,
"message": f"Task {task_id} moved to trash. Restore with restore('{batch}')."}
return {"deleted": task_id, "title": title, "deleted_batch_id": batch,
"message": f'Task {task_id} ("{title}") moved to trash. '
f"Restore with restore('{batch}')."}
def register(mcp) -> None:
+71 -59
View File
@@ -952,7 +952,74 @@ async def build_write_path_hint(
derive = [d for d in found if d.get("key") not in skip]
except Exception:
logger.warning("write-time derive check failed", exc_info=True)
if not synced and not menu and not stamped and not divergence and not derive:
staleness: list[str] = []
# ── Have the rules moved under this session? (milestone 323) ───────
#
# THE CARRIER IS THE POINT. This hook already fires before a write — the
# moment acting on a stale rule actually costs something — and the check
# is one comparison against a marker the session already holds. No
# payload, no extra round trip, and nothing said when nothing moved.
#
# WHAT THIS CANNOT SEE, and a reader who finds an etag here will assume
# otherwise:
#
# what goes wrong | caught?
# ---------------------------------------------------|--------
# another session edits a rule mid-flight | yes
# the session is misremembering a rule read hours ago | yes
# compaction summarised the rules out of context | NO
#
# The third is the most common and this is blind to it: the etag was in
# context too and went with the rules. The SessionStart nudge is that
# case's only mechanism and must not be softened because this shipped.
#
# Fails open, like every other arm here: a staleness hint must never
# break a write.
if rules_etag:
try:
current = await rulebooks_svc.list_always_on_rules(
user_id, project_id=project_id or 0,
)
if rulebooks_svc.rules_etag(current) != rules_etag:
moved = rulebooks_svc.rules_moved_since(current, rules_etag)
held = rulebooks_svc.etag_count(rules_etag)
bits = []
if moved:
named = ", ".join(
f"#{r.id} \u201c{r.title}\u201d" for r in moved[:3]
)
more = len(moved) - 3
bits.append(
f"{named}" + (f", and {more} more" if more > 0 else "")
)
# A DELETED rule moves no timestamp and leaves no row to name,
# so the count is the only thing that can report the one change
# that takes an instruction OUT of force.
if held is not None and held != len(current):
delta = len(current) - held
bits.append(
f"{abs(delta)} rule(s) {'added' if delta > 0 else 'no longer in force'}"
)
if bits:
staleness.append(
"Your loaded rules have changed since this session "
"started — " + "; ".join(bits) + ". Re-read them with "
"list_always_on_rules() before relying on the set you "
"are holding."
)
except Exception:
logger.debug("write-path rules-etag arm failed", exc_info=True)
# The guard sits BELOW the staleness arm on purpose. A rules change is
# unconditional news — it does not become less true because this
# particular write happened to match no prior art — and this arm is one
# indexed query, only when the session actually sent a marker.
#
# The standing-rule arm further down is deliberately left on the far side
# of this guard: that one runs a SEMANTIC search, and moving it here would
# run an embedding query on every write in the session. Its gating is a
# separate question from this one (see the note on #3244).
if not staleness and not synced and not menu and not stamped and not divergence and not derive:
return empty
owners = await owner_names_for({
@@ -971,7 +1038,9 @@ async def build_write_path_hint(
for marker, item in menu:
rendered.append((item, marker, _owner_of(item), _foreign_language(item, target_lang)))
lines: list[str] = []
# Seeded with the staleness line, which is decided above the early
# return and so cannot wait for this list to exist.
lines: list[str] = list(staleness)
sync_note_ids: list[int] = []
if synced:
# The sync framing (#2708). Deliberately imperative about the record —
@@ -1039,63 +1108,6 @@ async def build_write_path_hint(
for arm, ids in by_arm.items():
record_surfaced(user_id=user_id, note_ids=ids, source=arm)
# ── Have the rules moved under this session? (milestone 323) ───────
#
# THE CARRIER IS THE POINT. This hook already fires before a write — the
# moment acting on a stale rule actually costs something — and the check
# is one comparison against a marker the session already holds. No
# payload, no extra round trip, and nothing said when nothing moved.
#
# WHAT THIS CANNOT SEE, and a reader who finds an etag here will assume
# otherwise:
#
# what goes wrong | caught?
# ---------------------------------------------------|--------
# another session edits a rule mid-flight | yes
# the session is misremembering a rule read hours ago | yes
# compaction summarised the rules out of context | NO
#
# The third is the most common and this is blind to it: the etag was in
# context too and went with the rules. The SessionStart nudge is that
# case's only mechanism and must not be softened because this shipped.
#
# Fails open, like every other arm here: a staleness hint must never
# break a write.
if rules_etag:
try:
current = await rulebooks_svc.list_always_on_rules(
user_id, project_id=project_id or 0,
)
if rulebooks_svc.rules_etag(current) != rules_etag:
moved = rulebooks_svc.rules_moved_since(current, rules_etag)
held = rulebooks_svc.etag_count(rules_etag)
bits = []
if moved:
named = ", ".join(
f"#{r.id} \u201c{r.title}\u201d" for r in moved[:3]
)
more = len(moved) - 3
bits.append(
f"{named}" + (f", and {more} more" if more > 0 else "")
)
# A DELETED rule moves no timestamp and leaves no row to name,
# so the count is the only thing that can report the one change
# that takes an instruction OUT of force.
if held is not None and held != len(current):
delta = len(current) - held
bits.append(
f"{abs(delta)} rule(s) {'added' if delta > 0 else 'no longer in force'}"
)
if bits:
lines.append(
"Your loaded rules have changed since this session "
"started — " + "; ".join(bits) + ". Re-read them with "
"list_always_on_rules() before relying on the set you "
"are holding."
)
except Exception:
logger.debug("write-path rules-etag arm failed", exc_info=True)
# ── Standing rules that may apply here (milestone 307) ──────────────
#
# A SUGGESTION, not a binding surface, and the distinction is the design
+14 -2
View File
@@ -1449,8 +1449,20 @@ def rules_etag(rules: list) -> str:
"""
if not rules:
return _ETAG_EMPTY
newest = max(r.updated_at for r in rules)
return f"{newest.isoformat()}|{len(rules)}"
# A decoration must not be able to break what it decorates. This is
# computed on the SessionStart path, where raising would cost the whole
# context payload to save a hint — so a row with no usable timestamp is
# skipped rather than compared, and a set with none degrades to a
# count-only marker instead of failing. Count-only still catches a rule
# added or deleted; it just cannot see an edit, which is the right way
# round to lose information.
stamps = [
r.updated_at for r in rules
if isinstance(getattr(r, "updated_at", None), datetime)
]
if not stamps:
return f"unknown|{len(rules)}"
return f"{max(stamps).isoformat()}|{len(rules)}"
async def rules_etag_for(user_id: int, project_id: int = 0) -> str:
+4 -1
View File
@@ -260,7 +260,10 @@ async def test_list_always_on_rules_returns_empty_when_no_always_on_rulebooks():
):
from scribe.mcp.tools.rulebooks import list_always_on_rules
out = await list_always_on_rules()
assert out == {"rules": [], "total": 0}
# An install with no always-on rulebooks still gets a marker (milestone
# 323): "no rules" is a STATE, and a payload that omitted the key would
# make the write path read every session on a fresh install as a change.
assert out == {"rules": [], "total": 0, "rules_etag": "empty|0"}
@pytest.mark.asyncio
@@ -0,0 +1,106 @@
"""A record is cited by id AND title — never by number alone.
THE PROBLEM THIS IS ABOUT. The agent has the record open; the operator does
not. `#3244` reads as complete to the writer and as homework to the reader,
who has to look it up to know what their own conversation is about. The
operator's words: *"I don't know what a note, task, or milestone is by its ID
number."*
PRODUCT, NOT A RULE (rule 119). Every Scribe user hits this, so the fix is in
the surfaces the product ships — the skill that shapes how an agent writes,
and the tool responses that hand a record back. A per-instance rule would fix
it for one operator and leave the behaviour wrong for everyone else.
Scribe's duplicate gate already had the right shape — `id 412: "debounce
helper"` — which is why these assert the CONVENTION reaches the other
surfaces rather than inventing a new one.
"""
import pathlib
import pytest
from unittest.mock import AsyncMock, MagicMock, patch
SKILL = (
pathlib.Path(__file__).resolve().parents[1]
/ "plugin/skills/using-scribe/SKILL.md"
)
def test_the_skill_carries_the_convention():
"""The skill is read while deciding HOW to write, which is the only moment
this can be applied. A tool response can name one record; only the skill
can govern the prose around it."""
text = " ".join(SKILL.read_text().split())
assert "Name the record, never just its number" in text, (
"the using-scribe skill no longer tells an agent to write the title "
"alongside the id. Nothing else governs how records are cited in "
"prose, commit messages, or task bodies."
)
def test_the_skill_says_where_it_matters_most():
"""A convention stated only for chat messages gets applied only there —
and the places read LATER, by someone with even less context, are where a
bare id costs most."""
text = " ".join(SKILL.read_text().split()).lower()
assert "commit message" in text and "task bod" in text
def test_the_skill_says_to_look_up_a_title_it_does_not_know():
"""The escape hatch that would otherwise swallow the convention whole: an
agent that does not know the title will emit the number and move on."""
text = " ".join(SKILL.read_text().split()).lower()
assert "an id you can't name is one you haven't checked" in text
# ── The tool responses (the other half) ────────────────────────────────
#
# A deletion is the sharpest case: afterwards the row is trashed, so if the
# confirmation did not name it, nothing can. An operator who cannot recognise
# what was deleted cannot tell it was the wrong thing.
@pytest.mark.parametrize("module,fn,kind,rid", [
("tasks", "delete_task", "task", 3244),
("notes", "delete_note", "note", 2109),
("milestones", "delete_milestone", "milestone", 323),
])
@pytest.mark.asyncio
async def test_a_delete_confirmation_names_what_it_deleted(
module, fn, kind, rid, monkeypatch,
):
import importlib
mod = importlib.import_module(f"scribe.mcp.tools.{module}")
title = "the staleness signal"
row = MagicMock()
row.title = title
patches = [
patch.object(mod, "current_user_id", MagicMock(return_value=1)),
patch.object(mod.trash_svc, "delete", AsyncMock(return_value="batch-1")),
]
if module == "milestones":
patches.append(
patch.object(mod.milestones_svc, "get_milestone",
AsyncMock(return_value=row)))
else:
patches.append(
patch.object(mod.notes_svc, "get_note_for_user",
AsyncMock(return_value=(row, "owner"))))
for p in patches:
p.start()
try:
out = await getattr(mod, fn)(rid)
finally:
for p in patches:
p.stop()
assert out["title"] == title, f"{fn} returns no title for the record"
assert title in out["message"], (
f"{fn}'s message names only the id. After the delete the row is "
f"trashed, so this line is the last chance to say WHAT went."
)
assert str(rid) in out["message"], (
f"{fn} dropped the id — the title alone is not addressable, and the "
f"convention is id AND title, not one or the other."
)
+41
View File
@@ -224,3 +224,44 @@ async def test_the_arm_fails_open():
for p in patches:
p.stop()
assert "changed since this session started" not in out["context"]
def test_the_marker_cannot_break_the_payload_it_decorates():
"""It is computed on the SessionStart path. Raising there would cost the
whole context payload — every rule title, the project, the lot — to save
a hint, which is the wrong trade in every case.
A row with no usable timestamp is skipped; a set with none degrades to a
count-only marker. Count-only still catches a rule ADDED or DELETED and
only loses edits, which is the right way round to lose information.
Found by CI: `build_session_context` tests hand it MagicMock rules, and
`max()` over those raises TypeError rather than returning anything.
"""
from unittest.mock import MagicMock
assert svc.rules_etag([MagicMock(), MagicMock()]) == "unknown|2"
assert svc.rules_etag([SimpleNamespace()]) == "unknown|1"
# A count-only marker still moves when the set does.
assert svc.rules_etag([MagicMock()]) != svc.rules_etag([MagicMock(), MagicMock()])
# One usable stamp is enough to keep the real thing.
assert svc.rules_etag([_rule(), MagicMock()]).startswith("2026-")
@pytest.mark.asyncio
async def test_the_signal_arrives_even_when_nothing_else_matched():
"""THE BUG CI CAUGHT, and the one the task's acceptance criterion was
written to catch.
`build_write_path_hint` returns early when no prior art, stamp, divergence
or derive matched — which sat ABOVE this arm, so a session whose rules had
changed was told only if the file it happened to be editing also matched
something else. A staleness signal that fires on that coincidence is not a
staleness signal.
"""
held = svc.rules_etag([_rule(rid=1), _rule(rid=2, title="dev is home")])
current = [_rule(rid=1), _rule(rid=2, title="dev is home", updated_at=LATER)]
# Every other arm silent — which is exactly the case that used to return "".
ctx = await _hint(current, held)
assert "changed since this session started" in ctx