diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 2b8533f..3aa5c75 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.1232", + "version": "2026.09.16.2102", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/hooks.json b/plugin/hooks/hooks.json index ff1d27b..6907f98 100644 --- a/plugin/hooks/hooks.json +++ b/plugin/hooks/hooks.json @@ -53,6 +53,15 @@ "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_after_write.sh\"" } ] + }, + { + "matcher": "mcp__.*__get_rule", + "hooks": [ + { + "type": "command", + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_record_opened.sh\"" + } + ] } ], "PreCompact": [ diff --git a/plugin/hooks/scribe_autoinject.sh b/plugin/hooks/scribe_autoinject.sh index 956d8a2..b61aac6 100755 --- a/plugin/hooks/scribe_autoinject.sh +++ b/plugin/hooks/scribe_autoinject.sh @@ -100,6 +100,8 @@ if [ -n "$session_id" ]; then # stops holding what it was told. scribe_rules_live carries the reasoning. rule_seen=$(scribe_rules_live "$rulefile") [ -n "$rule_seen" ] && exclude_q="${exclude_q}&exclude_rule_ids=${rule_seen}" + # What the session actually OPENED, as against what it was shown (#4100). + exclude_q="${exclude_q}$(scribe_held_query "$rule_state_dir/${safe_sid}.opened.ids")" fi body=$(curl -fsS --max-time 5 \ diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 4d75438..0331abe 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -289,6 +289,25 @@ scribe_rules_append() { awk -v ts="$now" 'NF { print $1 "\t" ts }' >> "$f" 2>/dev/null || true } +# The OPENED ledger's contribution to a rule arm's query string (#4100). +# +# TWO LEDGERS, BECAUSE THEY RECORD TWO DIFFERENT FACTS. `.rules.ids` holds +# every id an arm has NAMED; `.opened.ids` holds the ids the session actually +# read, written by scribe_record_opened.sh from the `get_rule` call itself. +# Named is not read: the injected line is a teaser, and one skimmed past +# leaves nothing behind — least of all across a compaction. Sending both lets +# the server tell a reader who opened a rule from one who was only shown it, +# instead of telling them both the same untrue thing. +# +# Same reader as the naming ledger on purpose, so ageing, the last-entry-wins +# rule and the bare-id format are defined once and cannot drift apart. +scribe_held_query() { + local ids + ids=$(scribe_rules_live "$1") + [ -n "$ids" ] && printf '&held_rule_ids=%s' "$ids" + return 0 +} + # --------------------------------------------------------------------------- # WHICH PROJECT IS THIS DIRECTORY'S? (#4085) # diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index db2081f..af4eda6 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -207,6 +207,8 @@ if [ -n "$session_id" ]; then # this arm's sibling hook reads the same rule file through the same helper. rule_seen=$(scribe_rules_live "$rulefile") [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" + # What the session actually OPENED, as against what it was shown (#4100). + rule_exclude_q="${rule_exclude_q}$(scribe_held_query "$state_dir/${safe_sid}.opened.ids")" fi # Not `|| exit 0`: an unreachable instance must not discard a local finding diff --git a/plugin/hooks/scribe_record_opened.sh b/plugin/hooks/scribe_record_opened.sh new file mode 100644 index 0000000..f41c7b2 --- /dev/null +++ b/plugin/hooks/scribe_record_opened.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# Scribe — record that this session OPENED a rule, not merely saw it named (#4100). +# +# WHAT THIS CLOSES +# +# The rule arms keep a ledger of every id they have NAMED, and the injected +# line used to tell the reader "You saw it earlier this session". That claim +# was never checked. The line those arms emit is a TEASER — title, trigger, +# `get_rule(N)` — so a session can be named a rule twenty times and never read +# one word of it, and after a compaction the teaser is summarised away leaving +# nothing at all. The server was asserting something about the reader's +# context that it had no way to know. +# +# This is the observable half. PostToolUse fires for MCP tools (the event's own +# output schema carries `updatedMCPToolOutput`, which would be meaningless +# otherwise), so the `get_rule` CALL can be watched directly. +# +# WHY THIS IS NOT THE SELF-REPORT MILESTONE 386 REJECTED +# +# 386 ruled out asking the session whether it holds a rule, because a model +# asked "do you still hold rule 156?" will say yes and the answer is +# unverifiable self-report. That objection is about ASKING. This asks nobody: +# a tool call happened or it did not, and the harness reports it either way. +# Recording what a session DID is a different kind of evidence from believing +# what it says about itself. +# +# WHAT IT DELIBERATELY DOES NOT DO +# +# It does not prove the rule is still in context — nothing can, and a +# compaction can drop it moments later. That is why `.opened.ids` ages exactly +# like `.rules.ids` and is cleared on the same events (#3749): both ledgers +# describe a context that no longer exists once the context is destroyed. The +# claim it supports is only ever "you opened this, pull it again if you no +# longer hold it", which stays true in every case and carries its own remedy. +# +# EXIT 0, ALWAYS. This decorates a ledger; a bookkeeping failure must never +# turn a successful tool call into a hook error. Worst case the id is missed +# and the reader is offered a rule it already read — the cost of a wrong guess +# here is one extra line, in the direction that shows more rather than less. +set -uo pipefail + +event=$(cat 2>/dev/null || true) +[ -n "$event" ] || exit 0 + +# No jq, no ledger — and no complaint. Every other hook degrades the same way +# rather than printing a tooling error in front of the operator's work (#4107 +# tracks making that dependency honest; this is not the place to diverge). +command -v jq >/dev/null 2>&1 || exit 0 + +session_id=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || session_id="" +[ -n "$session_id" ] || exit 0 + +# The matcher in hooks.json already narrows to the get_rule tools, but the +# server segment of an MCP tool name varies with how the plugin was installed, +# so the id is read from whichever field is actually present rather than from +# an assumed tool name. An event that carries none simply records nothing. +rule_id=$(printf '%s' "$event" \ + | jq -r '(.tool_input.rule_id // empty) | tostring' 2>/dev/null) || rule_id="" +rule_id=$(printf '%s' "$rule_id" | tr -cd '0-9') +[ -n "$rule_id" ] || exit 0 + +# The same directory the naming ledger uses. One session keeps its state in one +# place, and the prior-art name is kept for the reason scribe_tool_rules.sh +# gives: renaming it would orphan every live session's state for a cosmetic +# gain. +state_dir="${TMPDIR:-/tmp}/scribe-priorart" +mkdir -p "$state_dir" 2>/dev/null || true +safe_sid=$(printf '%s' "$session_id" | tr -c 'A-Za-z0-9._-' '_') + +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + +# Stamped and append-only, exactly like the naming ledger — so the same reader +# (`scribe_rules_live`) ages both, and the last entry for an id wins. +printf '%s\n' "$rule_id" | scribe_rules_append "$state_dir/${safe_sid}.opened.ids" +exit 0 diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 85aa216..5d0da25 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -105,6 +105,13 @@ case "$source" in # 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 fi ;; esac diff --git a/plugin/hooks/scribe_tool_rules.sh b/plugin/hooks/scribe_tool_rules.sh index 3262460..1995aaa 100644 --- a/plugin/hooks/scribe_tool_rules.sh +++ b/plugin/hooks/scribe_tool_rules.sh @@ -87,6 +87,8 @@ if [ -n "$session_id" ]; then # session is still holding. scribe_rules_live carries the reasoning. rule_seen=$(scribe_rules_live "$rulefile") [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" + # What the session actually OPENED, as against what it was shown (#4100). + rule_exclude_q="${rule_exclude_q}$(scribe_held_query "$state_dir/${safe_sid}.opened.ids")" fi # `|| exit 0` here, unlike the prior-art hook: there is no local arm whose diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index 9e82042..b1df3cf 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -345,13 +345,43 @@ SMOKE_EVENTS: dict[str, str] = { {"session_id": "smoke", "transcript_path": "/nonexistent/smoke.jsonl", "cwd": ".", "hook_event_name": "Stop", "stop_hook_active": False} ), + # The PreCompact preserver (#3680). Its whole contract is the inverse of + # every other hook's: it must exit 0 AND print, because stdout is what + # becomes the summarizer's custom instructions. A silent success here is + # the failure mode, and it would look like every other hook's success. + "scribe_precompact_preserve.sh": json.dumps( + {"session_id": "smoke", "cwd": ".", "hook_event_name": "PreCompact", + "trigger": "manual", "custom_instructions": None} + ), + # The opened-ledger recorder (#4100). It writes to TMPDIR and prints + # nothing — a PostToolUse hook that emitted output would put a line in + # front of every get_rule call, which is the opposite of its purpose. The + # smoke case is a well-formed open: it must exit 0 and stay silent. + "scribe_record_opened.sh": json.dumps( + {"session_id": "smoke", "cwd": ".", + "tool_name": "mcp__scribe__get_rule", "tool_input": {"rule_id": 1}, + "tool_response": {}} + ), # The shared library is sourced, never run; executed bare it defines # functions and exits — silent by construction. "scribe_defs.sh": "", } -# The one hook that legitimately produces output with no credentials. -STATIC_FLOOR = "scribe_session_context.sh" +# The hooks that legitimately produce output with NO credentials, each for its +# own reason — named per hook rather than shared, because "this one is allowed +# to speak" is exactly the kind of exemption that quietly grows to cover a hook +# that is merely leaking. +STATIC_EMITTERS = { + # The two-tier SessionStart design: the bundled static tier ships whatever + # the instance does, and that floor is the whole point of the split. + "scribe_session_context.sh": "static floor present", + # PreCompact's contract is INVERTED (#3680). Its stdout becomes the + # summarizer's custom instructions, and what must survive a summary is + # known without asking anything — so it needs no instance, and silence is + # the failure mode rather than the success one. Read as a generic hook it + # would look like a leak; it is the opposite. + "scribe_precompact_preserve.sh": "preservation instructions present", +} # The hooks that say so when a configured instance does not answer (#2932). OUTAGE_SPEAKERS = {"scribe_prior_art.sh", "scribe_after_write.sh"} OUTAGE_LINE = "> Scribe did not answer the prior-art check" @@ -398,14 +428,13 @@ def check_fail_open() -> None: f"a recall aid may never fail the operator's action") continue out = proc.stdout.strip() - if script.name == STATIC_FLOOR: - # Emits its bundled static tier regardless; that floor is the - # whole point of the two-tier design. + if script.name in STATIC_EMITTERS: + what = STATIC_EMITTERS[script.name] if not out: - fail(f"{rel} [{label}]: emitted nothing — the static " - f"behavioural floor must survive having no credentials") + fail(f"{rel} [{label}]: emitted nothing — this hook's " + f"output must survive having no credentials ({what})") else: - ok(f"{rel} [{label}]: exit 0, static floor present") + ok(f"{rel} [{label}]: exit 0, {what}") elif out and label == "unreachable" and script.name in OUTAGE_SPEAKERS: # The only thing allowed here is the outage line itself. try: diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index ee9e6e6..d5d58c0 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -100,6 +100,13 @@ async def autoinject_retrieve(): purpose: one session keeps ONE rule ledger, so a rule named by any arm is not re-announced by another. Ages out (#3751), so salience decays. + held_rule_ids — comma-separated rule ids the session actually + (opt) OPENED, observed from the `get_rule` call itself + rather than claimed. A rule on exclude_rule_ids + was NAMED; a rule here was READ, and the two say + different things about what the reader holds — + so they get different lines (#4100). Shares the + ledger directory and the same clear-on-compact. TWO ARMS, TWO SETS OF GATES. Rules ride the same hook and the same query but nothing else: the notes menu can be disabled, thresholded and top-k'd @@ -116,9 +123,10 @@ async def autoinject_retrieve(): project_id, _repo, _unbound = await _project_scope() exclude_ids = _int_list(request.args.get("exclude_ids")) exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids")) + held_rule_ids = _int_list(request.args.get("held_rule_ids")) rules = await plugin_ctx_svc.build_prompt_rule_hint( - g.user.id, q, project_id=project_id, exclude_rule_ids=exclude_rule_ids + g.user.id, q, project_id=project_id, exclude_rule_ids=exclude_rule_ids, held_rule_ids=held_rule_ids ) result = await plugin_ctx_svc.build_autoinject_hint( g.user.id, q, project_id=project_id, exclude_ids=exclude_ids @@ -157,14 +165,20 @@ async def pre_tool_rules(): ledger on purpose: one session keeps one list, so a rule named by either arm is not re-offered by the other. + held_rule_ids (opt) — rule ids the session actually OPENED, as + against merely named. Named and read are + different claims about the reader's + context, so they get different lines + (#4100). """ tool = (request.args.get("tool") or "tool").strip() command = request.args.get("command") or "" project_id, _repo, _unbound = await _project_scope() exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids")) + held_rule_ids = _int_list(request.args.get("held_rule_ids")) result = await plugin_ctx_svc.build_tool_rule_hint( g.user.id, tool, command, - project_id=project_id, exclude_rule_ids=exclude_rule_ids, + project_id=project_id, exclude_rule_ids=exclude_rule_ids, held_rule_ids=held_rule_ids, ) return jsonify(result) @@ -202,6 +216,9 @@ async def write_path_prior_art(): above, and for the same reason: a rule named twenty turns ago should not be re-offered on every subsequent write. + held_rule_ids (opt) — RULE ids the session actually OPENED, as + against merely named; drives the third + reference wording (#4100). exclude_derive (opt) — comma-separated derive keys (a derive group id or `canon:`) already named this session by the ledger arm (#2900); its own @@ -225,6 +242,7 @@ async def write_path_prior_art(): p.strip() for p in (request.args.get("exclude_derive") or "").split(",") if p.strip() ] exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids")) + held_rule_ids = _int_list(request.args.get("held_rule_ids")) shapes = _parse_shapes(request.args.get("shapes") or "") api_key = getattr(g, "api_key", None) may_stamp = api_key is None or getattr(api_key, "scope", "") == "write" @@ -235,7 +253,7 @@ async def write_path_prior_art(): stamp_shapes=shapes if may_stamp else None, repo_key=repo_bindings_svc.normalize_repo_key(repo) if repo else "", exclude_derive=exclude_derive, - exclude_rule_ids=exclude_rule_ids, + exclude_rule_ids=exclude_rule_ids, held_rule_ids=held_rule_ids, ) return jsonify(result) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index d4a29ed..b992354 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -927,6 +927,7 @@ async def build_prompt_rule_hint( *, project_id: int = 0, exclude_rule_ids: list[int] | None = None, + held_rule_ids: list[int] | None = None, ) -> dict: """Rules and preferences that may apply to what the operator just asked. @@ -985,6 +986,7 @@ async def build_prompt_rule_hint( duration_ms = (time.perf_counter() - t0) * 1000.0 already = set(exclude_rule_ids or []) + held = set(held_rule_ids or []) fresh = [(score, rule) for score, rule in hits if rule.id not in already] # BEFORE the early return, for the reason both sibling arms spell out @@ -1020,7 +1022,10 @@ async def build_prompt_rule_hint( return out lines = [ - _rule_hint_line(rule, where="to this request", seen=rule.id in already) + _rule_hint_line( + rule, where="to this request", + seen=rule.id in already, held=rule.id in held, + ) for _score, rule in hits ] # FRESH-ONLY (#3752). A reference is a rendering decision, not a @@ -1297,7 +1302,9 @@ def _rule_band(hits: list) -> list: return [(s, r) for s, r in hits if s >= top - _RULEHINT_BAND] -def _rule_hint_line(rule, *, where: str, seen: bool, compact: bool = False) -> str: +def _rule_hint_line( + rule, *, where: str, seen: bool, held: bool = False, compact: bool = False, +) -> str: """One rule hint line — both arms, both tails, both kinds (#3750, #3849). THREE INDEPENDENT AXES SINCE #3851. `compact` joins `kind` and `seen`, and @@ -1380,13 +1387,37 @@ def _rule_hint_line(rule, *, where: str, seen: bool, compact: bool = False) -> s "for how this has been done before" if preference else "before deciding it does not apply" ) - tail = ( - f"You saw it earlier this session; pull it with get_rule({rule.id}) " - "if you no longer hold it." - if seen else - f"Read it with get_rule({rule.id}) {reason}; it is not in this " - "session's loaded set." - ) + # THREE STATES, BECAUSE TWO OF THEM WERE BEING TOLD THE SAME LIE (#4100). + # + # `seen` means an arm NAMED this rule earlier. It does not mean the session + # read it — the line is a teaser, and a teaser skimmed past leaves nothing + # behind, least of all after a compaction summarises the turn it arrived + # in. "You saw it earlier this session" asserted something about the + # reader's context that the server had no way to know. + # + # `held` is the observable half: a PostToolUse hook watches for the + # `get_rule` call itself, so this is a recorded EVENT rather than a claim. + # That distinction is what keeps the non-goal above intact — the objection + # was to asking a model about its own context, not to noticing what it did. + # + # The middle state is the honest one and the one that was missing: named, + # not opened. It gets the full invitation, because a session that skipped + # the teaser is in almost the same position as one that never saw it. + if held: + tail = ( + f"You opened it earlier this session; pull it with " + f"get_rule({rule.id}) again if you no longer hold it." + ) + elif seen: + tail = ( + f"Mentioned earlier this session but not opened — read it with " + f"get_rule({rule.id}) {reason}." + ) + else: + tail = ( + f"Read it with get_rule({rule.id}) {reason}; it is not in this " + "session's loaded set." + ) if compact: # THE TRIGGER GOES; THE TAIL STAYS. Only one of the two is expensive \u2014 # a trigger runs 300-400 characters after #3855, the tail about 100 \u2014 @@ -1421,6 +1452,7 @@ async def build_write_path_hint( repo_key: str = "", exclude_derive: list[str] | None = None, exclude_rule_ids: list[int] | None = None, + held_rule_ids: list[int] | None = None, ) -> dict: """Prior-art hint for the plugin's PreToolUse hook on Write/Edit. @@ -1842,6 +1874,7 @@ async def build_write_path_hint( rule_ids: list[int] = [] try: already = set(exclude_rule_ids or []) + held = set(held_rule_ids or []) # Timed like the notes arm above. Without this the rule row was the one # source in the whole readout reporting a null p90_duration_ms (#3311) # — a gap that reads as "this surface is somehow not measurable" rather @@ -1867,6 +1900,7 @@ async def build_write_path_hint( lines.append( _rule_hint_line( rule, where="here", seen=rule.id in already, + held=rule.id in held, compact=idx > 0, ) ) @@ -1957,6 +1991,7 @@ async def build_tool_rule_hint( *, project_id: int = 0, exclude_rule_ids: list[int] | None = None, + held_rule_ids: list[int] | None = None, ) -> dict: """Standing rules that may apply to the ACTION about to be taken (#3476). @@ -2011,6 +2046,7 @@ async def build_tool_rule_hint( duration_ms = (time.perf_counter() - t0) * 1000.0 already = set(exclude_rule_ids or []) + held = set(held_rule_ids or []) # Band first, dedup second — see the sibling arm for why that order is # load-bearing rather than incidental. kept = _rule_band(hits) @@ -2048,6 +2084,7 @@ async def build_tool_rule_hint( _rule_hint_line( rule, where=f"to this {tool_name} call", seen=rule.id in already, + held=rule.id in held, # Rank decides volume (#3851): the ranker's best guess gets the # trigger, the rest get cited. compact=idx > 0, diff --git a/tests/test_rule_opened_ledger.py b/tests/test_rule_opened_ledger.py new file mode 100644 index 0000000..3c7c765 --- /dev/null +++ b/tests/test_rule_opened_ledger.py @@ -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 diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index d589587..0da18d4 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -747,7 +747,13 @@ def test_the_hook_and_the_route_agree_on_every_parameter_name(): # repo and `project_id=` where a `.scribe` marker names the project. Both # halves are one contract with the route, so both are pinned. assert "scribe_scope_query" in hook, "hook no longer asks for a project scope" - assert set(re.findall(r"printf '([a-z_]+)=", defs)) == {"repo", "project_id"} + # `held_rule_ids` joins them for the same reason (#4100): the OPENED ledger + # is read by a shared helper so its ageing and format cannot drift from the + # naming ledger's, which means the key is spelled in defs rather than here. + assert "scribe_held_query" in hook, "hook no longer sends the opened ledger" + assert set(re.findall(r"printf '([a-z_]+)=", defs)) == { + "repo", "project_id", "held_rule_ids", + } # Both scope keys are read by the shared _project_scope() helper, not inline. assert "_project_scope()" in handler @@ -756,6 +762,11 @@ def test_the_hook_and_the_route_agree_on_every_parameter_name(): assert f'request.args.get("{arg}"' in scope, ( f"the hooks can send {arg!r} and the route never reads it" ) + # Read by the handler itself, not the scope helper — it is about what the + # reader holds, not about which project the work belongs to. + assert 'request.args.get("held_rule_ids")' in handler, ( + "the hook sends held_rule_ids and the route never reads it" + ) for arg in ("tool", "command", "exclude_rule_ids"): assert f'request.args.get("{arg}")' in handler, ( f"the hook sends {arg!r} and the route never reads it" diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 84af7cd..f5d9dc5 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -938,8 +938,13 @@ def test_route_reads_every_arg_the_hook_sends(): # whichever of `repo=` / `project_id=` scribe_scope_query picks, so the hook # splices in its output and the helper is the other half of the contract. assert "scribe_scope_query" in hook, "hook no longer asks for a project scope" + # Same arrangement for the OPENED ledger (#4100): spelled once in the + # helper so its ageing and format cannot drift from the naming ledger's. + assert "scribe_held_query" in hook, "hook no longer sends the opened ledger" defs = (HOOK.parent / "scribe_defs.sh").read_text() - assert set(re.findall(r"printf '([a-z_]+)=", defs)) == {"repo", "project_id"} + assert set(re.findall(r"printf '([a-z_]+)=", defs)) == { + "repo", "project_id", "held_rule_ids", + } def test_route_resolves_repo_to_a_project_not_to_a_location_filter():