diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 4f3a5a9..c3fe04c 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "2026.09.04.0140", + "version": "2026.09.09.0408", "author": { "name": "Bryan Van Deusen" }, 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/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 20cd999..bccd208 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -169,10 +169,12 @@ async def retrieval_telemetry(days: int = 30) -> dict: `avg_result_count` and `p90_duration_ms`. THE NUMBER TO READ FIRST IS `near_misses.p90`, AGAINST THE THRESHOLD IN - FORCE FOR THAT SURFACE. It is measured only on the calls that returned - NOTHING, on the best score the ranker reached before the bar rejected it — - so it is the one figure here that says something the bar cannot make true - by construction. A bar at 0.72 turning away a stream of 0.71s is set too + FORCE FOR THAT SURFACE. It is measured on the calls the BAR turned away — + zero-result calls, minus the ones whose zero was a repeat the reader had + already been shown — using the best score the ranker reached before the bar + rejected it. So it is the one figure here that says something the bar + cannot make true by construction, and `max` is always below the threshold: + an above-bar candidate nobody excluded would have been returned. A bar at 0.72 turning away a stream of 0.71s is set too high by a hair and the surface is losing hits it should have had. The same bar turning away 0.30s is working, and the corpus simply had nothing. Both render as a zero-result call, and nothing else in this readout tells them diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index 18e94df..577aac8 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -360,15 +360,45 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: } zero = case((RetrievalLog.result_count == 0, 1), else_=0) - # THE NEAR-MISS POPULATION: calls that returned nothing AND recorded what - # the bar turned away. Both conditions matter. Restricting to zero-result - # calls is what makes the number say something the bar cannot fix by - # construction — on a call that returned something, `best_available_score` - # equals `top_score` and adds nothing. Requiring the column to be non-null - # keeps rows written before #3670 out of the sample rather than letting - # them read as scoreless declines. - declined = (RetrievalLog.result_count == 0) & ( - RetrievalLog.best_available_score.isnot(None) + # THE NEAR-MISS POPULATION: calls that returned nothing BECAUSE THE BAR + # TURNED SOMETHING AWAY, and recorded what it was. Three conditions, and + # the third was missing for one deploy (#3739). + # + # Zero-result only: on a call that returned something, + # `best_available_score` equals `top_score` and adds nothing. + # + # Non-null only: rows written before #3670 genuinely do not know, and must + # not read as scoreless declines. + # + # AND NOT A REPEAT. A zero-result call is two unrelated events — the ranker + # found nothing above the bar, or it found only what this session had + # already been shown — and just the first says anything about the bar. That + # is the whole of #3497, and #3670 reintroduced the conflation one level up: + # the rule arms filter exclusions in PYTHON, after the search, so a rule + # that cleared the bar and was dropped as a repeat still reported a high + # `best_available_score` on a zero-result row. Live proof, first read after + # deploy: pre_tool_rule's near-miss max was 0.7457 while the lowest score it + # ever RETURNED was 0.7204 — a "rejection" that outscored acceptances. + # + # The NULL arm is principled, not permissive: `suppressed_count IS NULL` + # means the caller passed its exclusions INTO the search, which is exactly + # the case where the reported score is already post-exclusion and cannot be + # contaminated. Note arms stay measured; rule arms get cleaned. + # + # Deliberately conservative: a call carrying both a repeat and a lower + # genuine miss is dropped whole, losing that point. It undercounts; it + # cannot corrupt — the right way round for a number read against a bar. + # + # This also makes `near_misses.max < threshold` true BY CONSTRUCTION. An + # above-bar candidate that was not excluded would have been returned, so + # its call is not in this population at all. + declined = ( + (RetrievalLog.result_count == 0) + & (RetrievalLog.best_available_score.isnot(None)) + & ( + RetrievalLog.suppressed_count.is_(None) + | (RetrievalLog.suppressed_count == 0) + ) ) miss = case((declined, 1), else_=0) # `best_available_score` only for those rows; NULL elsewhere, and diff --git a/tests/test_services_retrieval_telemetry.py b/tests/test_services_retrieval_telemetry.py index ff5b8fd..243744c 100644 --- a/tests/test_services_retrieval_telemetry.py +++ b/tests/test_services_retrieval_telemetry.py @@ -994,6 +994,17 @@ async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_e user_id=UID, source="pre_tool_rule", query="ls", threshold=0.72, limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0, )) + # THE CASE WHOSE ABSENCE LET THIS GUARD PASS OVER BROKEN CODE (#3739). + # A zero-result call whose zero was a REPEAT, not a rejection: the ranker + # cleared the bar at 0.9 and the session had already been shown that rule, + # so the arm dropped it in Python after the search. Without the suppression + # arm of the predicate this row lands in the near-miss population and drags + # `max` to 0.9 — above the very threshold the field is read against. + await _insert_retrieval_log(_build_payload( + user_id=UID, source="pre_tool_rule", query="git commit", threshold=0.72, + limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0, + best_available=0.9, suppressed=1, + )) try: out = await retrieval_summary(UID, days=30) @@ -1002,22 +1013,28 @@ async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_e "zeros everywhere, which is #2663 exactly" ) src = out["sources"]["pre_tool_rule"] - assert src["calls"] == 5 - assert src["zero_result_calls"] == 4 + assert src["calls"] == 6 + assert src["zero_result_calls"] == 5 nm = src["near_misses"] assert nm is not None, "the near-miss block did not survive the query" assert nm["measured_calls"] == 3, ( - "the population is declines that RECORDED a score: three measured, " - "one unmeasured (excluded, not counted as a scoreless decline), and " - "one call that showed something (excluded — its best-available is " - "just its top score and says nothing about the bar)" + "the population is declines the BAR caused, that recorded a score. " + "Three qualify. Excluded: the unmeasured row (predates the column, " + "not a scoreless decline), the call that showed something (its " + "best-available is just its top score), and the REPEAT — a zero " + "the reader caused, not the bar (#3739)" ) assert nm["max"] == pytest.approx(0.7189, abs=1e-4), ( "the closest thing the bar turned away — 0.7189 against a 0.72 " "threshold, which is the reading the whole field exists to give" ) - assert nm["max"] < 0.72, "a near miss that cleared the bar is not a miss" + assert nm["max"] < 0.72, ( + "a rejection that outscores the bar is not a rejection. This is " + "structural once the suppression arm is in the predicate: an " + "above-bar candidate that was not excluded would have been " + "RETURNED, so its call cannot be in this population (#3739)" + ) assert 0.70 <= nm["p50"] <= 0.7189 finally: async with async_session() as s: 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" + )