feat(retrieval): the ledger records what was OPENED, not merely what was shown (#4100)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 50s
CI & Build / TypeScript typecheck (push) Successful in 55s
CI & Build / Python tests (push) Failing after 1m3s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 50s
CI & Build / TypeScript typecheck (push) Successful in 55s
CI & Build / Python tests (push) Failing after 1m3s
CI & Build / Build & push image (push) Skipped
Milestone 386 made a repeat REFERENCED rather than withheld, and the line it chose says "You saw it earlier this session". Nothing ever checked that. The arms emit a TEASER — title, trigger, get_rule(N) — so a session can be shown a rule twenty times and never read a word of it, and a compaction summarises the teaser away leaving nothing behind. The server was asserting something about the reader's context it had no way to know. Three states now, where there were two: never surfaced "it is not in this session's loaded set" named, unopened "Mentioned earlier this session but not opened — read it…" opened "You opened it earlier this session; pull it… again" The middle one is the honest one and the one that was missing. It keeps the full invitation, because a session that skipped a teaser is in nearly the position of one never shown it. HOW "OPENED" BECOMES OBSERVABLE. A new PostToolUse hook watches the get_rule call itself and appends to `<sid>.opened.ids`. PostToolUse does fire for MCP tools — the event's own output schema carries `updatedMCPToolOutput`, which would be meaningless otherwise — and the matcher is `mcp__.*__get_rule` so the server segment, which varies by install, is not pinned. This is NOT the self-report 386 rejected. That objection was to ASKING a model whether it holds a rule, which is unverifiable. A tool call is an event the harness reports whether anyone asks. Recording what a session DID and believing what it SAYS about itself are different kinds of evidence. Both ledgers clear together on compact/clear. Keeping `.opened.ids` across a compaction would have the arms telling a freshly-summarised session "you opened it earlier" about a rule now nowhere in its context — a more confident version of the bug being removed. Same reader (scribe_rules_live) for both, so ageing, last-entry-wins and the bare-id format are defined once. Also closes two smoke-coverage holes the checker was reporting as SKIP: the new recorder, and scribe_precompact_preserve.sh from #3680. The latter needed STATIC_FLOOR to become a set — PreCompact's contract is inverted, its stdout BECOMES the summarizer's instructions, so silence is its failure mode and a generic read of it looks like a leak. Step 2 of milestone 416, and a hard prerequisite for step 4: while suppression keys on shown, widening k marks records "seen" faster than they are read, and the ledger would degrade in proportion to the improvement. Plugin minted 2026.09.16.1232 -> 2026.09.16.2102. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -0,0 +1,166 @@
|
||||
"""Named is not read: the ledger's third state (#4100).
|
||||
|
||||
WHY THIS EXISTS
|
||||
|
||||
Milestone 386 made a repeat REFERENCED rather than withheld, and the line it
|
||||
chose says "You saw it earlier this session". That claim was never checked.
|
||||
The arms emit a TEASER — title, trigger, `get_rule(N)` — so a session can be
|
||||
shown a rule twenty times and never read a word of it, and a compaction
|
||||
summarises the teaser away leaving nothing at all. The server was asserting
|
||||
something about the reader's context it had no way to know.
|
||||
|
||||
`.opened.ids` is the observable half, written by a PostToolUse hook from the
|
||||
`get_rule` call itself. That is why this is not the self-report 386 rejected:
|
||||
the objection was to ASKING a model about its own context, and a tool call is
|
||||
an event the harness reports whether anyone asks or not.
|
||||
|
||||
WHAT THIS PINS
|
||||
|
||||
The three states and their three lines, the hook that records an open, and the
|
||||
fact that BOTH ledgers die together on a compaction — a session told "you
|
||||
opened it earlier" about a rule that was just summarised out of its context
|
||||
would be a more confident version of the bug this removes.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from scribe.services.plugin_context import _rule_hint_line
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[1]
|
||||
HOOKS = ROOT / "plugin" / "hooks"
|
||||
RECORDER = HOOKS / "scribe_record_opened.sh"
|
||||
|
||||
|
||||
def _rule(rid=156, kind="rule"):
|
||||
return MagicMock(id=rid, kind=kind, title="A wait with no deadline is a bug",
|
||||
when_to_apply="crossing a process boundary")
|
||||
|
||||
|
||||
# ── the three lines ────────────────────────────────────────────────────────
|
||||
|
||||
def test_a_rule_never_surfaced_is_offered_as_new():
|
||||
line = _rule_hint_line(_rule(), where="here", seen=False, held=False)
|
||||
assert "not in this session's loaded set" in line
|
||||
|
||||
|
||||
def test_a_rule_named_but_not_opened_says_so_and_still_invites():
|
||||
"""The state that did not exist. It must NOT claim the reader saw it, and
|
||||
must still carry the pull pointer — a session that skipped the teaser is
|
||||
in nearly the position of one that was never shown it."""
|
||||
line = _rule_hint_line(_rule(), where="here", seen=True, held=False)
|
||||
assert "not opened" in line
|
||||
assert "get_rule(156)" in line
|
||||
assert "You saw it earlier" not in line, (
|
||||
"the middle state is claiming the reader read something they did not"
|
||||
)
|
||||
|
||||
|
||||
def test_a_rule_the_session_opened_is_described_as_opened():
|
||||
line = _rule_hint_line(_rule(), where="here", seen=True, held=True)
|
||||
assert "opened it earlier" in line
|
||||
assert "get_rule(156)" in line
|
||||
|
||||
|
||||
def test_the_three_states_produce_three_different_lines():
|
||||
"""Guard against a refactor collapsing two branches: each state has to be
|
||||
distinguishable, or the distinction this milestone bought is gone while
|
||||
every individual assertion above still passes."""
|
||||
lines = {
|
||||
_rule_hint_line(_rule(), where="here", seen=s, held=h)
|
||||
for s, h in ((False, False), (True, False), (True, True))
|
||||
}
|
||||
assert len(lines) == 3
|
||||
|
||||
|
||||
def test_held_outranks_seen_regardless_of_kind():
|
||||
"""`kind` moves the head and the ledger moves the tail; #3497's history is
|
||||
the two being reasoned about together and one of them being forgotten."""
|
||||
for kind in ("rule", "preference"):
|
||||
line = _rule_hint_line(_rule(kind=kind), where="here", seen=True, held=True)
|
||||
assert "opened it earlier" in line
|
||||
|
||||
|
||||
# ── the recorder ───────────────────────────────────────────────────────────
|
||||
|
||||
def _run_recorder(event: dict, tmp: Path) -> Path:
|
||||
for tool in ("bash", "jq"):
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"hook runtime tool {tool!r} not installed")
|
||||
env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)}
|
||||
out = subprocess.run(["bash", str(RECORDER)], input=json.dumps(event),
|
||||
capture_output=True, text=True, env=env, timeout=30)
|
||||
assert out.returncode == 0, out.stderr
|
||||
return tmp / "scribe-priorart" / "s1.opened.ids"
|
||||
|
||||
|
||||
def test_opening_a_rule_is_recorded(tmp_path):
|
||||
led = _run_recorder(
|
||||
{"session_id": "s1", "tool_name": "mcp__scribe__get_rule",
|
||||
"tool_input": {"rule_id": 156}}, tmp_path)
|
||||
assert led.exists()
|
||||
assert led.read_text().split("\t")[0] == "156"
|
||||
|
||||
|
||||
def test_the_entry_is_stamped_so_it_ages_like_the_naming_ledger(tmp_path):
|
||||
"""Both ledgers are read by `scribe_rules_live`, which ages on that stamp.
|
||||
An unstamped entry never expires — bounded, but it would mean an opened
|
||||
rule stays 'opened' for a session's whole life."""
|
||||
led = _run_recorder(
|
||||
{"session_id": "s1", "tool_name": "mcp__scribe__get_rule",
|
||||
"tool_input": {"rule_id": 9}}, tmp_path)
|
||||
parts = led.read_text().strip().split("\t")
|
||||
assert len(parts) == 2 and parts[1].isdigit()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("event", [
|
||||
{"session_id": "s1", "tool_name": "mcp__scribe__get_rule", "tool_input": {}},
|
||||
{"session_id": "s1", "tool_name": "mcp__scribe__get_rule",
|
||||
"tool_input": {"rule_id": "../../etc"}},
|
||||
{"tool_name": "mcp__scribe__get_rule", "tool_input": {"rule_id": 5}},
|
||||
])
|
||||
def test_an_event_with_nothing_usable_records_nothing_and_still_exits_zero(event, tmp_path):
|
||||
"""A hook that fails a tool call over bookkeeping is worse than one that
|
||||
misses an id: the cost of a miss is one extra line, in the direction that
|
||||
shows more rather than less."""
|
||||
led = _run_recorder(event, tmp_path)
|
||||
assert not led.exists()
|
||||
|
||||
|
||||
# ── the two ledgers stay in step ───────────────────────────────────────────
|
||||
|
||||
def test_a_compaction_clears_both_ledgers():
|
||||
"""The one that would be worst to get wrong. `.opened.ids` describes a
|
||||
context the compaction just destroyed, so keeping it while clearing the
|
||||
naming ledger would have the arms telling a freshly-summarised session
|
||||
"you opened it earlier" about a rule now nowhere in its context.
|
||||
"""
|
||||
sh = (HOOKS / "scribe_session_context.sh").read_text()
|
||||
block = sh.split("case \"$source\" in")[1].split("esac")[0]
|
||||
assert "compact|clear)" in block
|
||||
for led in (".rules.ids", ".opened.ids"):
|
||||
assert re.search(rf"rm -f .*{re.escape(led)}", block), (
|
||||
f"{led} survives a compaction that destroyed what it describes"
|
||||
)
|
||||
|
||||
|
||||
def test_the_recorder_is_registered_on_the_get_rule_tool():
|
||||
hooks = json.loads((HOOKS / "hooks.json").read_text())["hooks"]
|
||||
posts = hooks["PostToolUse"]
|
||||
mine = [b for b in posts
|
||||
if any("scribe_record_opened.sh" in h["command"] for h in b["hooks"])]
|
||||
assert len(mine) == 1, "the opened-recorder is not registered exactly once"
|
||||
# An MCP tool's server segment varies with how the plugin was installed, so
|
||||
# the matcher must not pin one spelling of it.
|
||||
matcher = mine[0]["matcher"]
|
||||
assert re.fullmatch(matcher, "mcp__plugin_scribe_scribe__get_rule"), matcher
|
||||
assert re.fullmatch(matcher, "mcp__scribe__get_rule"), matcher
|
||||
assert not re.fullmatch(matcher, "Bash"), matcher
|
||||
Reference in New Issue
Block a user