diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 77b2ab3..1274c21 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -55,6 +55,53 @@ here=$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) || exit 0 event=$(cat 2>/dev/null || true) source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source="" +# --- The rule ledger outlives the context it describes (#3749) --- +# +# scribe_prior_art.sh and scribe_tool_rules.sh record every rule id they have +# named in /.rules.ids and hand it back as exclude_rule_ids, so a +# rule is named once per session and then goes quiet. That is right while the +# session still HOLDS what it was told, and wrong the moment it does not. +# +# A compaction summarizes the earlier injections away and does not touch the +# filesystem, so the rule ends up absent from context AND still excluded — +# unreachable for the rest of the session. The banner below tells the model to +# re-pull its ALWAYS-ON rules, but a rule an arm surfaced is conditional and is +# not in that set, so it has no other way back. The rules most likely to be in +# 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. +# +# CLEARED ON THE SOURCES THAT DESTROY CONTEXT, AND ONLY THOSE: +# +# compact CLEAR — summarized away; the file survived. +# clear CLEAR — context wiped. +# startup nothing to do: a new session id means a new, empty file. +# resume KEEP. The context was genuinely restored, so the ledger still +# describes what the session holds. Clearing here would re-surface +# every rule after a restore that lost nothing — the mirror error. +# fork KEEP, and the answer is the same whichever way forks are keyed: a +# 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. +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 + fi + ;; +esac + out="" # Append $1 to $out, separated by a horizontal rule when $out already has content. append() { if [ -n "$out" ]; then out="${out}"$'\n\n---\n\n'"$1"; else out="$1"; fi; } diff --git a/tests/test_session_context_ledger.py b/tests/test_session_context_ledger.py new file mode 100644 index 0000000..5546d51 --- /dev/null +++ b/tests/test_session_context_ledger.py @@ -0,0 +1,158 @@ +"""The SessionStart hook clears the rule ledger exactly when context dies (#3749). + +WHY THIS EXISTS + +The prior-art and tool-rule hooks record every rule id they have named in +`/.rules.ids` and hand it back as `exclude_rule_ids`, so a rule is +surfaced once per session and then goes quiet. That is correct while the +session still holds what it was told. + +A compaction breaks that assumption in the worst available way: it summarizes +the earlier injections out of context and does not touch the filesystem. The +rule ends up absent from context AND still excluded — unreachable for the rest +of the session. The compaction banner tells the model to re-pull its +*always-on* rules, but a rule an arm surfaced is conditional and is not in that +set, so it has no other way back. The rules most likely to be in that state are +the ones that fire most often. + +WHAT THIS PINS + +Not "the ledger is cleared" — that would pass against a hook which deletes it +on every source, and deleting on `resume` is its own defect: the context was +genuinely restored there, so re-surfacing every rule is the mirror error. + +What is pinned is the DISCRIMINATION. The whole source table is asserted in one +statement, so a blanket delete (all False) and a no-op (all True) both fail, +and neither can be made to pass by editing one case. + +Runs the real shell against a temp TMPDIR, like the after-write hook's tests. +Deliberately with no SCRIBE_URL/SCRIBE_TOKEN in the environment: clearing the +ledger is local, keyless and networkless, and must still happen on an instance +that is unreachable or unconfigured. +""" +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +PLUGIN = Path(__file__).resolve().parents[1] / "plugin" +HOOK = PLUGIN / "hooks" / "scribe_session_context.sh" + + +def _env(tmp_path): + """Near-namesake of test_after_write_hook's `_env`, and deliberately not it: + that one needs git and curl and SUPPLIES credentials, because the behaviour + it tests is a network round-trip. This one must prove the opposite — that + the clear happens with no credentials and no network at all — so sharing a + helper would mean testing this case in an environment that cannot show it. + """ + for tool in ("jq", "bash"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + # No SCRIBE_URL / SCRIBE_TOKEN on purpose — see the module docstring. + return {"PATH": os.environ["PATH"], "TMPDIR": str(tmp_path), + "HOME": str(tmp_path)} + + +def _ledger(tmp_path, sid: str, name: str = "rules.ids") -> Path: + d = tmp_path / "scribe-priorart" + d.mkdir(exist_ok=True) + f = d / f"{sid}.{name}" + f.write_text("156\n168\n") + return f + + +def _fire(source: str, sid: str, env) -> None: + subprocess.run( + ["bash", str(HOOK)], + input=json.dumps({"source": source, "session_id": sid}), + capture_output=True, text=True, env=env, timeout=30, + ) + + +def test_the_rules_ledger_survives_exactly_when_the_context_does(tmp_path): + """The whole source table, in one assertion, so it cannot be half-satisfied. + + `startup` is listed even though it is a no-op against a session id that has + never been seen: it is asserted here so that a future change which starts + clearing indiscriminately fails on a case somebody would otherwise call + harmless. + """ + env = _env(tmp_path) + survived = {} + for source in ("compact", "clear", "resume", "startup"): + sid = f"sess-{source}" + ledger = _ledger(tmp_path, sid) + _fire(source, sid, env) + survived[source] = ledger.exists() + + assert survived == { + "compact": False, + "clear": False, + "resume": True, + "startup": True, + }, ( + f"got {survived}. A rule surfaced before a compaction is summarized " + f"out of context while its id stays on the exclusion ledger, so it " + f"becomes unreachable for the rest of the session — that is what the " + f"compact/clear cases prevent. The resume case is the other half: the " + f"context came back intact there, and re-surfacing every rule after a " + f"restore that lost nothing is the same defect from the other side. " + f"All-False means something is deleting unconditionally; all-True " + f"means the clear never runs." + ) + + +def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path): + """Scope, asserted rather than described. + + 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. + """ + env = _env(tmp_path) + sid = "sess-scope" + rules = _ledger(tmp_path, sid, "rules.ids") + notes = _ledger(tmp_path, sid, "ids") + sync = _ledger(tmp_path, sid, "sync.ids") + derive = _ledger(tmp_path, sid, "derive.ids") + + _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." + ) + + +def test_a_compact_without_a_session_id_is_survivable(tmp_path): + """Defensive, because this hook's contract is fail-open. + + An event with no `session_id` names no ledger. The hook must not error, and + must not fall back to a wildcard — clearing every session's ledger on the + machine because this one event was malformed is the worst available + reading of "best effort". + """ + env = _env(tmp_path) + other = _ledger(tmp_path, "someone-elses-session") + + proc = subprocess.run( + ["bash", str(HOOK)], + input=json.dumps({"source": "compact"}), + capture_output=True, text=True, env=env, timeout=30, + ) + + assert proc.returncode == 0, proc.stderr + assert other.exists(), ( + "an event with no session id cleared a ledger belonging to a different " + "session" + )