diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index d2b1158..5600365 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.17.0100", + "version": "2026.09.17.0111", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 542824c..76e6370 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -329,12 +329,26 @@ scribe_held_query() { # 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. +# compact rather than discarded can still live in these directories without +# being swept away by a glob that was never told about it. `.unreached` +# is the live example: it records that the instance could not be reached, not +# what the session holds, and #2932 needs it to outlive a compaction. +# +# TWO DIRECTORIES, WHICH IS ITS OWN LESSON. The first cut of this swept only +# `scribe-priorart` — every ledger named in the hooks was there, so the list +# looked complete. `scribe_autoinject.sh` keeps its note ledger in +# `scribe-autoinject`, so the arm that fires most (598 calls in five days) was +# the one the clear could not reach, and the fix read as finished. Hence the +# roster here rather than a path at the call site: one place to add to, and +# the tests read THIS string rather than a copy of it. +SCRIBE_LEDGER_DIRS="scribe-priorart scribe-autoinject" + scribe_clear_session_ledgers() { - local dir="$1" sid="$2" - [ -n "$dir" ] && [ -n "$sid" ] || return 0 - rm -f "$dir/$sid"*.ids 2>/dev/null || true + local sid="$1" dir + [ -n "$sid" ] || return 0 + for dir in $SCRIBE_LEDGER_DIRS; do + rm -f "${TMPDIR:-/tmp}/$dir/$sid"*.ids 2>/dev/null || true + done return 0 } diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 2fa739d..b0dacaa 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -121,14 +121,16 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # 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. +# rather than what the session holds, and survives on purpose. The directories +# swept are `scribe_defs.sh`'s `SCRIBE_LEDGER_DIRS` — plural, because the +# auto-inject arm keeps its ledger somewhere else and a single-directory sweep +# missed exactly the arm that fires most. 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._-' '_') - scribe_clear_session_ledgers \ - "${TMPDIR:-/tmp}/scribe-priorart" "$safe_sid" + scribe_clear_session_ledgers "$safe_sid" fi ;; esac diff --git a/tests/test_session_context_ledger.py b/tests/test_session_context_ledger.py index 5546d51..b0e28b1 100644 --- a/tests/test_session_context_ledger.py +++ b/tests/test_session_context_ledger.py @@ -108,14 +108,29 @@ def test_the_rules_ledger_survives_exactly_when_the_context_does(tmp_path): ) -def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path): - """Scope, asserted rather than described. +def test_the_note_ledgers_are_cleared_too_now_that_a_repeat_is_shown(tmp_path): + """The decision this test used to pin, reversed — and why it could be. - The same directory holds `.ids`, `.sync.ids` and `.derive.ids` for the note - arms. Whether a surfaced NOTE should come back after a compaction is a - different question with a different answer, and it is not being answered. - A `rm` glob over `.*` would pass every assertion in the test above - while silently deciding it. + It read: the note arms were deliberately left out of #3749, whether a + surfaced NOTE should come back after a compaction is a different question, + and a glob over `.*` would decide it by accident. That was right when + it was written, and the reason was the note arms' behaviour rather than the + note arms' nature: a note repeat was WITHHELD, so clearing their ledger + meant re-injecting whole menu lines the session had already been given. + Against that, leaving them was the cheaper mistake. + + #4101 removed the premise. A note repeat is now rendered again with a + `seen` marker instead of dropped from the search, so the ledger no longer + decides whether a record is reachable — only whether its line says the + session has met it before. Across a compaction that marker becomes a false + statement: it tells a freshly-summarised session it has already seen a + record that is nowhere in its context. Keeping the ledger buys nothing and + asserts something untrue. + + Kept as a named test rather than deleted, because the roster it guards + moved to tests/test_session_ledger_clear.py and what is worth keeping HERE + is the record that the answer changed, with the reason attached — a + deleted test takes the old decision's reasoning with it. """ env = _env(tmp_path) sid = "sess-scope" @@ -127,10 +142,9 @@ def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path) _fire("compact", sid, env) assert not rules.exists(), "the rule ledger should have been cleared" - assert notes.exists() and sync.exists() and derive.exists(), ( - "a note ledger was cleared too. The note arms were deliberately left " - "out of #3749 — clearing them is a decision about a different surface, " - "and a glob that takes them along makes it by accident." + assert not (notes.exists() or sync.exists() or derive.exists()), ( + "a note ledger outlived the context it describes, so its records are " + "marked `seen` to a session that can no longer see them" ) diff --git a/tests/test_session_ledger_clear.py b/tests/test_session_ledger_clear.py index a1d1bf8..372c77d 100644 --- a/tests/test_session_ledger_clear.py +++ b/tests/test_session_ledger_clear.py @@ -51,25 +51,43 @@ import pytest HOOKS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" SESSION_START = HOOKS / "scribe_session_context.sh" +DEFS = HOOKS / "scribe_defs.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") +# Every ledger the hooks write today, and where. Named here so a failure READS +# as "this one survived", but never used to build the hook's own delete list — +# the convention tests below are what keep this roster honest as it grows. +LEDGERS = { + "scribe-priorart": (".ids", ".rules.ids", ".opened.ids", ".sync.ids", + ".derive.ids"), + "scribe-autoinject": (".ids",), +} + + +def _swept_dirs() -> set[str]: + """The directories the SHIPPED hook sweeps, read from the hook itself. + + Read rather than restated: a test carrying its own copy of the roster would + agree with itself forever, which is precisely the failure that let the + auto-inject ledger sit in a directory nothing cleared. + """ + line = re.search(r'SCRIBE_LEDGER_DIRS="([^"]*)"', DEFS.read_text()) + assert line, "SCRIBE_LEDGER_DIRS is gone; the clear has no roster" + return set(line.group(1).split()) def _run_session_start(source: str, tmp: Path) -> Path: - """Run the SessionStart hook for real, with a populated ledger directory.""" + """Run the SessionStart hook for real, with the ledger directories filled.""" 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") + for dirname, suffixes in LEDGERS.items(): + state = tmp / dirname + state.mkdir(parents=True, exist_ok=True) + for suffix in suffixes: + (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") + (tmp / "scribe-priorart" / "s1.unreached").write_text("1\n") env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)} out = subprocess.run( @@ -78,14 +96,15 @@ def _run_session_start(source: str, tmp: Path) -> Path: capture_output=True, text=True, env=env, timeout=60, ) assert out.returncode == 0, out.stderr - return state + return tmp @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()] + """The step, stated as behaviour: all of them, in both directories.""" + root = _run_session_start(source, tmp_path) + survived = [f"{d}/s1{s}" for d, suffixes in LEDGERS.items() + for s in suffixes if (root / d / f"s1{s}").exists()] assert not survived, ( f"{survived} survived a {source!r} that destroyed what they describe" ) @@ -100,11 +119,13 @@ def test_a_source_that_kept_the_context_keeps_the_ledgers(source, tmp_path): 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" - ) + root = _run_session_start(source, tmp_path) + for dirname, suffixes in LEDGERS.items(): + for suffix in suffixes: + assert (root / dirname / f"s1{suffix}").exists(), ( + f"{dirname}/s1{suffix} was cleared on {source!r}, which lost " + "no context" + ) def test_the_outage_marker_is_not_swept_with_them(tmp_path): @@ -115,33 +136,79 @@ def test_the_outage_marker_is_not_swept_with_them(tmp_path): 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() + root = _run_session_start("compact", tmp_path) + assert (root / "scribe-priorart" / "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. +# ── the two assumptions the sweep rests on ───────────────────────────────── +# +# Both are checked against the hooks rather than trusted, because neither fails +# loudly: a ledger outside them simply never clears, on the arm whose author had +# no reason to know a convention existed. - 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. +def _dir_vars(text: str) -> dict[str, str]: + """`var="${TMPDIR:-/tmp}/"` → {var: name}, per script.""" + return dict(re.findall( + r'(\w+)="\$\{TMPDIR:-/tmp\}/([A-Za-z0-9_-]+)"', text)) + + +def _session_paths(text: str): + """Every `$/${safe_sid}` a script composes.""" + return re.findall(r'\$(\w+)"?/\$\{safe_sid\}([A-Za-z0-9_.]*)', text) + + +def test_every_directory_holding_a_ledger_is_one_the_clear_visits(): + """The failure that shipped once already, in the same step that fixed it. + + The first cut swept `scribe-priorart` alone. Every ledger NAMED in the + hooks was there, so it read as complete — and `scribe_autoinject.sh` keeps + its note ledger in `scribe-autoinject`, which meant the arm that fires most + was the only one still carrying the bug. Nothing said so: the clear ran, + found nothing to remove, and exited 0. """ - # Files built from the safe session id that are NOT per-session ledgers. - NOT_LEDGERS = {".unreached"} - + swept = _swept_dirs() 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"): + text = script.read_text() + dirs = _dir_vars(text) + for var, suffix in _session_paths(text): + if not suffix.endswith(".ids"): continue - offenders.append(f"{script.name}: ${{safe_sid}}{suffix}") + where = dirs.get(var) + if where not in swept: + offenders.append(f"{script.name}: ${var} -> {where or '?'}") 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}" + "these ledgers live in directories the compaction clear never visits, " + f"so they outlive the context they describe: {offenders}. Add the " + "directory to SCRIBE_LEDGER_DIRS in scribe_defs.sh." + ) + + +def test_every_session_file_in_a_swept_directory_follows_the_convention(): + """The other half: the sweep matches `.ids`, so a ledger must be named it. + + Stated as its own test because the two assumptions fail differently — this + one lets a ledger sit in the right directory and still never clear. + """ + # Session files in a swept directory that are NOT per-session ledgers. + NOT_LEDGERS = {".unreached"} + + swept = _swept_dirs() + offenders = [] + for script in sorted(HOOKS.glob("*.sh")): + text = script.read_text() + dirs = _dir_vars(text) + for var, suffix in _session_paths(text): + if dirs.get(var) not in swept: + continue # a different directory, a different question + if suffix in NOT_LEDGERS or suffix.endswith(".ids"): + continue + offenders.append(f"{script.name}: ${var}/${{safe_sid}}{suffix}") + + assert not offenders, ( + "these session files sit in a swept directory but clear on no " + f"compaction, because the sweep matches `.ids`: {offenders}" ) @@ -149,13 +216,13 @@ 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 + rebuilding the trap for the next 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: + for suffix in {s for suffixes in LEDGERS.values() for s in suffixes}: assert suffix not in block, ( f"the clear names {suffix} again — a list, not a convention" )