diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 3aa5c75..d2b1158 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.16.2102", + "version": "2026.09.17.0100", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 0331abe..542824c 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -308,6 +308,36 @@ scribe_held_query() { return 0 } +# Drop EVERY per-session ledger, matched by convention rather than listed (#4101). +# +# A LIST IS THE BUG. Until now the compact/clear branch named its files one at +# a time, and it named two of the five: `.rules.ids` and `.opened.ids` were +# cleared while `.ids` (notes), `.sync.ids` (shape signals) and `.derive.ids` +# survived. So milestone 386's defect — "a compaction destroys the context but +# not the ledger, so the most applicable records become permanently +# unreachable mid-session" — was fixed for rules and left standing on the note +# and snippet surfaces, which are the ones that fire most often. +# +# Nobody decided that. The list was written when rules were the only ledger +# that mattered and was never revisited when the others arrived, which is what +# a hand-maintained list of "things to remember to clean up" does. Adding three +# more `rm` lines would rebuild the same trap for the sixth ledger. +# +# So the rule is the NAME: a per-session ledger is `[.].ids`, and +# everything matching that goes. A new ledger following the convention is +# covered the day it is written, by nobody. One that does not follow it is a +# deliberate exception and has to say so. +# +# Scoped to `.ids` rather than `.*` so a marker that is REWRITTEN on +# compact rather than discarded can still live in this directory without +# being swept away by a glob that was never told about it. +scribe_clear_session_ledgers() { + local dir="$1" sid="$2" + [ -n "$dir" ] && [ -n "$sid" ] || return 0 + rm -f "$dir/$sid"*.ids 2>/dev/null || true + return 0 +} + # --------------------------------------------------------------------------- # WHICH PROJECT IS THIS DIRECTORY'S? (#4085) # diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 5d0da25..2fa739d 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -77,9 +77,11 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # this state are the ones that fire most often, which is to say the ones that # apply most. # -# The session id survives a compaction — the etag marker further down is -# rewritten on `compact` and keyed by session_id, which is only meaningful if -# the id is stable — so the stale ledger is genuinely found again, not orphaned. +# The session id survives a compaction — the ledgers are keyed by it and are +# still found under the same name afterwards, so a stale ledger is genuinely +# reached again rather than orphaned. (This used to point at an etag marker as +# the evidence for that; milestone 394 removed the preload the etag described, +# and `plugin_context.py` records its retirement.) # # CLEARED ON THE SOURCES THAT DESTROY CONTEXT, AND ONLY THOSE: # @@ -93,25 +95,40 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # fork carries the conversation, so if it inherits the id the ledger # is accurate, and if it gets a new one the file is empty anyway. # -# ONLY the rules ledger. The same directory holds .ids / .sync.ids / -# .derive.ids for the note arms. Whether a surfaced NOTE should return after a -# compaction is a different question with a different answer, and leaving those -# alone is a decision rather than an oversight. +# EVERY LEDGER, AND THE NOTE ARMS NEEDED IT MOST (#4101). +# +# This used to clear the two rule ledgers by name and say, in a comment, that +# leaving `.ids` / `.sync.ids` / `.derive.ids` alone was "a decision rather than +# an oversight". Reading the note arms says otherwise, on two counts: +# +# - They are HARD exclusions. `exclude_ids` goes into `semantic_search_notes` +# itself, so a surfaced note is removed from the result set — it is not +# rendered as a reference the way #3750 made a repeated rule. There is no +# weaker form for it to fall back to. +# - They never AGE. #3751 gave the rules ledger a TTL precisely because +# salience decays without a context event; the note channels were left on a +# flat read. +# +# Hard plus permanent plus never cleared means a note surfaced in the first +# minute of a session is unreachable for the rest of it, through any number of +# compactions. That is milestone 386's original defect, alive on the arms that +# fire most often, and nothing about it was decided. +# +# THE LIST WAS THE BUG, so the fix is not a longer list. `scribe_clear_session_ +# ledgers` matches on the naming convention — a per-session ledger is +# `[.].ids` — which covers all five and covers the sixth on the day +# it is written. Best-effort, like every other filesystem touch in these hooks: +# a ledger that cannot be removed costs a repeated exclusion, never a session. +# +# NOT swept: `.unreached`, which records that the instance was unreachable +# rather than what the session holds, and survives on purpose. case "$source" in compact|clear) sid=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || sid="" if [ -n "$sid" ]; then safe_sid=$(printf '%s' "$sid" | tr -c 'A-Za-z0-9._-' '_') - # Best-effort, like every other filesystem touch in these hooks: a ledger - # that cannot be removed costs a repeated exclusion, never a session. - rm -f "${TMPDIR:-/tmp}/scribe-priorart/${safe_sid}.rules.ids" 2>/dev/null || true - # BOTH ledgers, for one reason (#4100). `.opened.ids` records what the - # session read; a compaction is exactly the event that takes it away - # again. Clearing the naming ledger while keeping this one would leave - # the surfacing arms telling a freshly-summarised session "you opened - # it earlier" about a rule that is no longer anywhere in its context — - # a more confident version of the claim this milestone removed. - rm -f "${TMPDIR:-/tmp}/scribe-priorart/${safe_sid}.opened.ids" 2>/dev/null || true + scribe_clear_session_ledgers \ + "${TMPDIR:-/tmp}/scribe-priorart" "$safe_sid" fi ;; esac diff --git a/tests/test_rule_opened_ledger.py b/tests/test_rule_opened_ledger.py index 3c7c765..72e23c8 100644 --- a/tests/test_rule_opened_ledger.py +++ b/tests/test_rule_opened_ledger.py @@ -135,21 +135,9 @@ def test_an_event_with_nothing_usable_records_nothing_and_still_exits_zero(event 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" - ) +# What a compaction clears — including `.opened.ids`, whose claim is the one +# that would be worst to get wrong — is asserted against the running hook in +# tests/test_session_ledger_clear.py, so there is one home for it. def test_the_recorder_is_registered_on_the_get_rule_tool(): diff --git a/tests/test_session_ledger_clear.py b/tests/test_session_ledger_clear.py new file mode 100644 index 0000000..a1d1bf8 --- /dev/null +++ b/tests/test_session_ledger_clear.py @@ -0,0 +1,161 @@ +"""A compaction clears every session ledger, by convention not by list (#4101). + +WHY THIS EXISTS + +Milestone 386 established the claim: a ledger describes what a session HOLDS, +so the events that destroy context must destroy it too, or the records it names +become permanently unreachable mid-session. `scribe_session_context.sh` +implemented that — for the rules ledger, by name. + +Five ledgers live in that directory and two were on the list. The note arms' +three (`.ids`, `.sync.ids`, `.derive.ids`) were left, under a comment asserting +this was "a decision rather than an oversight". It was not a decision, and the +arms left out are the ones where it costs most: + + - their exclusions are HARD — `exclude_ids` is passed into + `semantic_search_notes` itself, so a surfaced note leaves the result set + entirely, with no weaker rendering to fall back to the way #3750 gave a + repeated rule one; + - and they never AGE — #3751's TTL was added to the rules ledger only. + +Hard, permanent and never cleared: a note surfaced in a session's first minute +is unreachable for the rest of it, through any number of compactions. + +WHAT THIS PINS + +The list was the bug, so the fix cannot be a longer list and neither can the +test. Both sides are asserted: + + 1. BEHAVIOUR — the hook is run on a real `compact` event with all five + ledgers on disk, and all five are gone afterwards. Run rather than + grepped, because grepping for the names is the hand-maintained pattern + this step removes. + 2. THE CONVENTION THE BEHAVIOUR RESTS ON — every per-session ledger any hook + builds is named `[.].ids`. That is what makes a sixth ledger + covered on the day it is written, and it is the assumption that would + rot silently, because a ledger named outside it simply never clears and + nothing says so. + +And the negative: `.unreached` is not a ledger of held context but a +record that the instance could not be reached, and a glob that swept it away +would make a hook forget an outage it is meant to report (#2932). +""" +import json +import os +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +HOOKS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" +SESSION_START = HOOKS / "scribe_session_context.sh" + +# Every ledger the hooks write today. Named here so a failure READS as "this +# one survived", but never used to build the hook's own delete list — the +# convention test below is what keeps this roster honest as it grows. +LEDGERS = (".ids", ".rules.ids", ".opened.ids", ".sync.ids", ".derive.ids") + + +def _run_session_start(source: str, tmp: Path) -> Path: + """Run the SessionStart hook for real, with a populated ledger directory.""" + for tool in ("bash", "jq"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + + state = tmp / "scribe-priorart" + state.mkdir(parents=True, exist_ok=True) + for suffix in LEDGERS: + (state / f"s1{suffix}").write_text("42\t1789600000\n") + # Not a ledger: an outage marker that must outlive the clear. + (state / "s1.unreached").write_text("1\n") + + env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)} + out = subprocess.run( + ["bash", str(SESSION_START)], + input=json.dumps({"session_id": "s1", "source": source}), + capture_output=True, text=True, env=env, timeout=60, + ) + assert out.returncode == 0, out.stderr + return state + + +@pytest.mark.parametrize("source", ["compact", "clear"]) +def test_a_context_destroying_source_clears_every_ledger(source, tmp_path): + """The step, stated as behaviour: all five, not the two that were listed.""" + state = _run_session_start(source, tmp_path) + survived = [s for s in LEDGERS if (state / f"s1{s}").exists()] + assert not survived, ( + f"{survived} survived a {source!r} that destroyed what they describe" + ) + + +@pytest.mark.parametrize("source", ["startup", "resume"]) +def test_a_source_that_kept_the_context_keeps_the_ledgers(source, tmp_path): + """The mirror error, and the more expensive one. + + `resume` genuinely restores the context, so the ledger still describes what + the session holds; clearing there would re-surface every record after a + restore that lost nothing. A blanket glob makes over-clearing cheap to + write, which is exactly why this direction needs a test of its own. + """ + state = _run_session_start(source, tmp_path) + for suffix in LEDGERS: + assert (state / f"s1{suffix}").exists(), ( + f"s1{suffix} was cleared on {source!r}, which lost no context" + ) + + +def test_the_outage_marker_is_not_swept_with_them(tmp_path): + """`.unreached` records that the instance was down, not what was surfaced. + + Different lifetime, different question. #2932's whole point is that "we + checked and found nothing" and "we never managed to check" must stay + distinguishable, and a clear that took this file out would quietly answer + the second with the first. + """ + state = _run_session_start("compact", tmp_path) + assert (state / "s1.unreached").exists() + + +def test_every_ledger_any_hook_builds_follows_the_naming_convention(): + """The assumption the glob rests on, asserted where it can actually fail. + + A ledger named outside `[.].ids` does not break loudly — it just + never clears, on the arm whose author had no reason to know a convention + existed. So the convention is checked against the hooks themselves rather + than trusted: every path any hook composes from the session-id stem has to + end in `.ids`, or be named below as a deliberate non-ledger. + """ + # Files built from the safe session id that are NOT per-session ledgers. + NOT_LEDGERS = {".unreached"} + + offenders = [] + for script in sorted(HOOKS.glob("*.sh")): + for suffix in re.findall(r'\$\{safe_sid\}([A-Za-z0-9_.]*)', + script.read_text()): + if suffix in NOT_LEDGERS or suffix.endswith(".ids"): + continue + offenders.append(f"{script.name}: ${{safe_sid}}{suffix}") + + assert not offenders, ( + "these session files clear on no compaction, because the clear matches " + f"on the `.ids` convention and they do not follow it: {offenders}" + ) + + +def test_the_clear_is_derived_and_not_a_list_of_names(): + """The regression that would look like a fix. + + Appending an `rm -f` per ledger passes every behavioural test above while + rebuilding the trap for the sixth one. The property worth keeping is that + the hook names no ledger at all. + """ + sh = SESSION_START.read_text() + block = sh.split('case "$source" in')[1].split("esac")[0] + assert "scribe_clear_session_ledgers" in block + for suffix in LEDGERS: + assert suffix not in block, ( + f"the clear names {suffix} again — a list, not a convention" + )