feat(rules): a session is told when its rules move under it (#3244, milestone 323 step 5)
CI & Build / Python lint (push) Successful in 6s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / integration (push) Successful in 32s
CI & Build / TypeScript typecheck (push) Successful in 35s
CI & Build / Python tests (push) Failing after 1m0s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 6s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / integration (push) Successful in 32s
CI & Build / TypeScript typecheck (push) Successful in 35s
CI & Build / Python tests (push) Failing after 1m0s
CI & Build / Build & push image (push) Skipped
The rules payload carries a marker; the write-path hook hands it back; the server says which rules moved. Nothing is said when nothing moved. THE COUNT IS NOT DECORATION. max(updated_at) alone cannot see a DELETED rule — it moves no timestamp — and that is the single change that takes an instruction OUT of force, which is the one a session most needs to hear about. The marker is `<max updated_at>|<count>`, and a deletion is reported through the count because there is no row left to name. THE HOOK IS THE CARRIER because it already fires before a write, which is the moment acting on a stale rule costs something. One comparison, no payload, no extra round trip. WHERE THE MARKER IS CAPTURED, and it could not be anywhere else: the SessionStart hook, from /api/plugin/context. The model also receives one from list_always_on_rules, but a hook cannot see an MCP tool's result — so the value the write path compares has to be stored where a shell script can reach it. Keyed by session id in the state dir the prior-art hook already uses, so "changed since" means since THIS session loaded its rules. NOT ON rules_payload, against the task's letter. Those are applicable_rules — a different, subscription-derived set. One key name over two sets is how a comparison starts reporting phantom changes, and the write path compares against the always-on set. WHAT IT CANNOT SEE is stated in both the service and the write-path arm as a table, because a reader who finds an etag will assume it covers staleness generally: another session edits a rule mid-flight | caught the session is misremembering a rule read hours ago | caught compaction summarised the rules out of context | NOT caught The third is the most common, and the marker 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. A test asserts both modules still explain that. Instance-agnostic (rule 115): an install with no rules produces a stable marker rather than an error, and "no rules" reads as a state rather than as a change. An unreadable or absent marker reports nothing — a signal that cries wolf is worse than none, because it trains a reader to skip the line that will one day be true. The arm fails open like every other arm on this hook. The delivery is tested through the real build_write_path_hint rather than the helper alone: the feature IS a line arriving in a session, and the arithmetic being right proves nothing about that. Live acceptance is deploy-gated and not yet recorded on the task. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -262,7 +262,16 @@ async def list_always_on_rules(project_id: int = 0) -> dict:
|
||||
"""
|
||||
uid = current_user_id()
|
||||
rules = await rulebooks_svc.list_always_on_rules(uid, project_id=project_id)
|
||||
return {"rules": [_rule_summary(r) for r in rules], "total": len(rules)}
|
||||
return {
|
||||
"rules": [_rule_summary(r) for r in rules],
|
||||
"total": len(rules),
|
||||
# A marker for the set you are now holding. It is not for you to read:
|
||||
# the write-path hook carries it back and is told if these rules have
|
||||
# moved since. Deliberately NOT on rules_payload's applicable_rules —
|
||||
# that is a DIFFERENT set (subscription-derived), and one key name
|
||||
# over two sets is how a comparison starts reporting phantom changes.
|
||||
"rules_etag": rulebooks_svc.rules_etag(rules),
|
||||
}
|
||||
|
||||
|
||||
async def get_rule(rule_id: int) -> dict:
|
||||
|
||||
@@ -138,6 +138,11 @@ async def write_path_prior_art():
|
||||
or `canon:<snippet_id>`) already named this
|
||||
session by the ledger arm (#2900); its own
|
||||
channel, like the two above.
|
||||
rules_etag (opt) — the marker the session was given when it loaded
|
||||
its always-on rules (milestone 323). Sent back
|
||||
so the server can say whether those rules have
|
||||
MOVED since. Absent means the hook has nothing
|
||||
stored, which is silence, not a mismatch.
|
||||
shapes (opt) — comma-separated `kind:name` definitions the hook
|
||||
found in (or enclosing) the payload, kind being
|
||||
css|sym. The shape ledger's write-path feed
|
||||
@@ -157,6 +162,7 @@ async def write_path_prior_art():
|
||||
p.strip() for p in (request.args.get("exclude_derive") or "").split(",") if p.strip()
|
||||
]
|
||||
exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids"))
|
||||
rules_etag = (request.args.get("rules_etag") or "").strip()
|
||||
shapes = _parse_shapes(request.args.get("shapes") or "")
|
||||
api_key = getattr(g, "api_key", None)
|
||||
may_stamp = api_key is None or getattr(api_key, "scope", "") == "write"
|
||||
@@ -168,6 +174,7 @@ async def write_path_prior_art():
|
||||
repo_key=repo_bindings_svc.normalize_repo_key(repo) if repo else "",
|
||||
exclude_derive=exclude_derive,
|
||||
exclude_rule_ids=exclude_rule_ids,
|
||||
rules_etag=rules_etag,
|
||||
)
|
||||
return jsonify(result)
|
||||
|
||||
|
||||
@@ -708,6 +708,7 @@ async def build_write_path_hint(
|
||||
repo_key: str = "",
|
||||
exclude_derive: list[str] | None = None,
|
||||
exclude_rule_ids: list[int] | None = None,
|
||||
rules_etag: str = "",
|
||||
) -> dict:
|
||||
"""Prior-art hint for the plugin's PreToolUse hook on Write/Edit.
|
||||
|
||||
@@ -1038,6 +1039,63 @@ 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
|
||||
@@ -1208,7 +1266,10 @@ async def build_session_context(
|
||||
its normalized key — triggers a one-line "bind this repo" hint so
|
||||
the binding is self-healing.
|
||||
|
||||
Returns {"context": str, "rule_count": int, "project": dict | None}.
|
||||
Returns {"context": str, "rule_count": int, "project": dict | None,
|
||||
"rules_etag": str}. The etag is for the HOOK, not for the model — the
|
||||
hook stores it and hands it back on each write so the server can say
|
||||
whether these rules have moved since the session loaded them.
|
||||
`context` is markdown ready to drop into `additionalContext`; it is capped
|
||||
at _MAX_CHARS with an explicit truncation note so the hook can pass it
|
||||
through verbatim.
|
||||
@@ -1317,4 +1378,12 @@ async def build_session_context(
|
||||
if len(context) > _MAX_CHARS:
|
||||
context = context[:_MAX_CHARS].rstrip() + "\n\n…(truncated — call list_always_on_rules())"
|
||||
|
||||
return {"context": context, "rule_count": len(rules), "project": project_dict}
|
||||
return {
|
||||
"context": context,
|
||||
"rule_count": len(rules),
|
||||
"project": project_dict,
|
||||
# Computed from the rules THIS payload was built from, not re-queried:
|
||||
# the marker has to describe the set the session is actually holding,
|
||||
# and a second query could disagree with the first.
|
||||
"rules_etag": rulebooks_svc.rules_etag(rules),
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from collections.abc import Iterable
|
||||
from datetime import datetime
|
||||
from typing import Optional
|
||||
|
||||
from sqlalchemy import and_, delete as sql_delete, insert, or_, select
|
||||
@@ -1413,6 +1414,89 @@ def rules_payload(applicable: dict) -> dict:
|
||||
}
|
||||
|
||||
|
||||
# ── The staleness marker (milestone 323 step 5) ────────────────────────
|
||||
#
|
||||
# WHAT THIS CAN AND CANNOT SEE. An etag catches a rule that MOVED after a
|
||||
# session loaded it. It is not a general staleness check, and a reader who
|
||||
# finds one here will assume it is:
|
||||
#
|
||||
# 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 the marker is blind to it, because 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 exists —
|
||||
# retiring something that covers the common case in favour of something that
|
||||
# does not is the plausible mistake here.
|
||||
|
||||
_ETAG_EMPTY = "empty|0"
|
||||
|
||||
|
||||
def rules_etag(rules: list) -> str:
|
||||
"""A marker for "is the set you are holding still the current one?".
|
||||
|
||||
`max(updated_at)` alone is not enough: DELETING a rule moves no timestamp,
|
||||
and that is the single change that takes an instruction OUT of force —
|
||||
the one a session most needs to hear about. The count catches it.
|
||||
|
||||
Instance-agnostic (rule 115): it knows nothing about any particular
|
||||
rulebook, and an install with one rule or none produces a stable marker
|
||||
rather than an error. "No rules" must read as a state, not as a change,
|
||||
or every session on a fresh install would be told its rules had moved.
|
||||
"""
|
||||
if not rules:
|
||||
return _ETAG_EMPTY
|
||||
newest = max(r.updated_at for r in rules)
|
||||
return f"{newest.isoformat()}|{len(rules)}"
|
||||
|
||||
|
||||
async def rules_etag_for(user_id: int, project_id: int = 0) -> str:
|
||||
"""The current marker for the set a session at this scope would hold.
|
||||
|
||||
Deliberately built from `list_always_on_rules` rather than from a
|
||||
`max()/count()` aggregate. An aggregate would be cheaper, and would have
|
||||
to restate that function's definition of the set — the always_on flag,
|
||||
the project's inception exclusions, the tier filter. Two definitions of
|
||||
"the session's rules" is how the marker starts disagreeing with the
|
||||
rules, which is worse than materialising a few dozen rows.
|
||||
"""
|
||||
rules = await list_always_on_rules(user_id, project_id=project_id)
|
||||
return rules_etag(rules)
|
||||
|
||||
|
||||
def rules_moved_since(rules: list, held_etag: str) -> list:
|
||||
"""The rules whose text changed after `held_etag` was issued.
|
||||
|
||||
Returns [] when the marker matches, is unparseable, or is absent — a
|
||||
caller cannot act on "something is different but I cannot say what", and
|
||||
a garbled marker must not be reported as a change.
|
||||
|
||||
A count difference is real news that this list cannot show: a rule
|
||||
DELETED since the marker was issued has no row left to return. Callers
|
||||
compare counts separately.
|
||||
"""
|
||||
if not held_etag or held_etag == _ETAG_EMPTY:
|
||||
return []
|
||||
stamp, _, _count = held_etag.partition("|")
|
||||
try:
|
||||
held_at = datetime.fromisoformat(stamp)
|
||||
except ValueError:
|
||||
return []
|
||||
return [r for r in rules if r.updated_at and r.updated_at > held_at]
|
||||
|
||||
|
||||
def etag_count(held_etag: str) -> int | None:
|
||||
"""How many rules the holder had. None when the marker cannot be read."""
|
||||
_stamp, _, count = (held_etag or "").partition("|")
|
||||
try:
|
||||
return int(count)
|
||||
except ValueError:
|
||||
return None
|
||||
|
||||
|
||||
# ── The staleness sweep (milestone 312) ────────────────────────────────
|
||||
|
||||
async def rules_due_for_verification(
|
||||
|
||||
Reference in New Issue
Block a user