diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index c89b7ed..6ee017d 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.21.0426", + "version": "2026.09.21.0442", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index f5a285a..2e8e8df 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -367,6 +367,164 @@ scribe_defs() { # $1 repo root, $2 repo-relative path of the file being written (excluded from # the grep — it would always match itself on an Edit). Definitions on stdin. # Prints one "> - `name` is already defined in N other file(s): …" per hit. +# ── The contract around a change (#4215, milestone 419) ─────────────────── +# +# WHAT THIS ANSWERS. "You altered the arity, name or shape of something — here +# is everything that reads it." Rule 33's interface-contract check, one scope +# down: not between layers but between a definition and its callers. +# +# WHY IT IS A CHECK AND NOT A LESSON. #4207 was written — "widening a tuple is +# an interface change to every unpack site, and the compiler will not tell +# you" — hours before a structurally identical mistake was made by its author, +# and it was surfaced twice in the turns before. Text delivered at the moment +# of acting is too weak a carrier for a reflex that has to change what the act +# IS. This looks it up instead. +# +# `scribe_exposed` — the names a CALLER can depend on, from a blob of code. +# Three kinds, because a contract breaks three ways and they look nothing +# alike in the source: +# +# sym what is defined rename / removal +# arg its parameter names arity and order +# key quoted keys of dict literals the shape of what it RETURNS +# +# The third is here because of the miss that produced this step. A config +# function gained one dict key; three arms read that dict inside a fail-open +# `except`, so every one of them silently became a no-op and ten tests went +# red at once with nothing pointing at the cause. No signature changed. A +# check that only watched signatures would have watched the wrong thing. +scribe_exposed() { + # The `sym` half DELEGATES to scribe_defs rather than repeating its patterns. + # Those patterns cover nine languages and have been corrected several times + # (the Go receiver form, the `type` import-specifier false positive, the + # dunder skip); a second copy here would inherit today's version and then + # quietly stop agreeing with it, which is #3497's history for the two rule + # arms. One reader, called twice. + local blob + blob=$(cat) + { + printf '%s' "$blob" | scribe_defs + printf '%s' "$blob" | awk ' + function emit(kind, name) { + if (name != "" && name !~ /^__.*__$/) print kind "\t" name + } + { + line = $0; sub(/^[[:space:]]+/, "", line) + sub(/^((pub(\([a-z]+\))?|export|default|private|internal|protected|public|static|suspend|async|open|sealed|data|abstract|final|inline|unsafe|extern|override)[[:space:]]+)*/, "", line) + + # Parameter names, from whatever announces a definition. Taken from the + # FIRST parenthesis only: a default value can itself contain parens and + # a greedy match would swallow the body of a one-liner. + if (match(line, /^(function|def|func|fun|fn|sub)[[:space:]]+[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*\(/) \ + || match(line, /^(const|let)[[:space:]]+[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*=[[:space:]]*(async[[:space:]]*)?\(/)) { + args = line + sub(/^[^(]*\(/, "", args) + sub(/\).*$/, "", args) + n = split(args, parts, ",") + for (i = 1; i <= n; i++) { + a = parts[i] + gsub(/^[[:space:]]+|[[:space:]]+$/, "", a) + # Strip a type annotation, a default, and the * / ** / & markers. + sub(/[:=].*$/, "", a) + gsub(/^[*&]+/, "", a) + gsub(/[[:space:]]/, "", a) + # `self` and `cls` are not part of anything a caller passes. + if (a != "" && a != "self" && a != "cls" && a ~ /^[A-Za-z_$][A-Za-z0-9_$]*$/) + emit("arg", a) + } + } + + # Quoted keys of a dict / object literal. Anchored on the quote so a + # dictionary ACCESS (`cfg["k"]`) does not read as a definition of one — + # only `"k":` counts, which is the writing position. + rest = $0 + while (match(rest, /["'\''][A-Za-z_][A-Za-z0-9_]*["'\''][[:space:]]*:/)) { + tok = substr(rest, RSTART, RLENGTH) + rest = substr(rest, RSTART + RLENGTH) + gsub(/["'\'']/, "", tok); sub(/[[:space:]]*:$/, "", tok) + emit("key", tok) + } + } + ' 2>/dev/null + } | sort -u +} + +# Who references `name` anywhere else in the repo. Word-bounded, so `cfg` does +# not match `cfg_path`, and the defining file is excluded — a definition is +# always its own first mention and listing it says nothing. +# +# `|| true` INSIDE the substitution for the reason #4042 records at +# scribe_local_dups: under `pipefail` a `head` that exits early kills the +# still-writing git grep, and an outer fallback then wipes the hits head had +# already printed. +scribe_contract_readers() { + local root="$1" rel="$2" name="$3" + [ -n "$root" ] && [ -n "$name" ] || return 0 + git -C "$root" grep -I -l -w -e "$name" -- . ":(exclude)${rel}" 2>/dev/null \ + | head -6 || true +} + +# The whole check, rendered. Kept here rather than inline in the hook so it +# can be exercised against a pair of blobs with no event, no server and no +# session — which is how every case in test_contract_around_the_change.py is +# written. +# +# Arguments: root, repo-relative path, the subject definition, the old text, +# the new text, and the session ledger (may be empty). +# +# TWO GATES, AND THE SECOND IS WHAT KEEPS THIS QUIET. A change to the exposed +# set is necessary but not sufficient: a definition NOTHING else references +# has no contract to break, so the readers lookup runs second and an empty +# result ends it silently. On a repo of any size most edits touch something +# local, so most edits say nothing here — and a hint that fires on everything +# is one that gets skipped. +scribe_contract_block() { + local root="$1" rel="$2" subject="$3" old_text="$4" new_text="$5" ledger="$6" + local changed gained lost readers count + [ -n "$root" ] && [ -n "$subject" ] || return 0 + [ -n "$old_text" ] && [ -n "$new_text" ] || return 0 + + # Named once per session per subject. A second edit to the same definition + # is the SAME contract question, and answering it again would punish the + # ordinary rhythm of getting a change right over several passes. + if [ -n "$ledger" ] && [ -f "$ledger" ]; then + grep -qxF "$subject" "$ledger" 2>/dev/null && return 0 + fi + + # One pass, no process substitution: `comm` would need /dev/fd, and this + # also keeps the two sides' extraction visibly identical. + changed=$( + { + printf '%s' "$old_text" | scribe_exposed | sed 's/^/O\t/' + printf '%s' "$new_text" | scribe_exposed | sed 's/^/N\t/' + } | awk -F'\t' ' + NF >= 3 { k = $2 "\t" $3; side[k] = side[k] $1 } + END { + for (k in side) + if (side[k] == "O") print "lost\t" k + else if (side[k] == "N") print "gained\t" k + } + ' 2>/dev/null + ) + [ -n "$changed" ] || return 0 + + readers=$(scribe_contract_readers "$root" "$rel" "$subject") + [ -n "$readers" ] || return 0 + count=$(printf '%s\n' "$readers" | grep -c . 2>/dev/null || printf '0') + + gained=$(printf '%s\n' "$changed" | awk -F'\t' '$1=="gained" {printf "%s%s %s", (n++?", ":""), $2, $3}') + lost=$(printf '%s\n' "$changed" | awk -F'\t' '$1=="lost" {printf "%s%s %s", (n++?", ":""), $2, $3}') + + printf '> The contract around `%s` changed, and %s other file(s) reference it (`git grep -w`; a nudge, not a gate):\n' \ + "$subject" "$count" + [ -n "$gained" ] && printf '> gained: %s\n' "$gained" + [ -n "$lost" ] && printf '> lost: %s\n' "$lost" + printf '> read by: %s\n' "$(printf '%s' "$readers" | tr '\n' ' ' | sed 's/ $//')" + printf '> A caller that passes or reads the old shape keeps compiling and fails only when that line runs (lesson #4207). Read them before moving on.\n' + [ -n "$ledger" ] && printf '%s\n' "$subject" >> "$ledger" 2>/dev/null + return 0 +} + scribe_local_dups() { local root="$1" rel="$2" kind name pat hits count label files while IFS=$'\t' read -r kind name; do diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index f012943..3813b7a 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -281,10 +281,49 @@ if [ -n "$local_lines" ] && [ "$reached" = 1 ]; then fi fi -# Local first. It answers "this already EXISTS", which is a stronger claim than -# "this resembles something recorded" — and it is the one the recorded arms are -# structurally unable to make. -combined="$local_context" +# --------------------------------------------------------------------------- +# ARM 0 — THE CONTRACT AROUND THE CHANGE (#4215, milestone 419). +# +# "You altered the arity, name or shape of something — here is everything that +# reads it." Rule 33's interface-contract check one scope down: not between +# layers but between a definition and its callers. +# +# LOCAL AND SERVERLESS, like ARM 1. It needs the working tree and nothing +# else — the server has no checkout, so this is the only place the question +# can be asked at all. +# +# EDITS ONLY. The comparison is between what the definition exposed before and +# what it exposes now, so it needs both texts; a Write that creates a file has +# no "before" and nothing to break. +contract_context="" +if [ -n "$repo_root" ] && [ -n "$code" ]; then + old_code=$(scribe_json_pick "$event_flat" '.tool_input.old_string') + [ -n "$old_code" ] || old_code=$(scribe_json_pick "$event_flat" '.tool_input.old_str') + # The SUBJECT is the definition whose contract may have moved. `shapes` + # already holds either the definitions in the payload or — for an edit + # inside a function body — the one enclosing the edit, which is exactly the + # thing whose callers matter. CSS rows are skipped: a class has readers, but + # they are markup files and `scribe_local_dups` already speaks for those. + subject=$(printf '%s\n' "$shapes" \ + | awk -F'\t' 'NF>=2 && $1!="css" {print $2; exit}') + if [ -n "$old_code" ] && [ -n "$subject" ]; then + contract_file="" + [ -n "${safe_sid:-}" ] && contract_file="$state_dir/${safe_sid}.contract.ids" + contract_context=$(scribe_contract_block \ + "$repo_root" "$rel_path" "$subject" "$old_code" "$code" "$contract_file" \ + 2>/dev/null) || contract_context="" + fi +fi + +# CONTRACT FIRST of the three, and the order is the strength of the claim. +# This one says something may already be BROKEN by the edit in hand. The local +# arm says a copy exists. The recorded arms say something resembles this. A +# reader who reads one line should read that one. +combined="$contract_context" +if [ -n "$local_context" ]; then + [ -n "$combined" ] && combined="${combined}"$'\n' + combined="${combined}${local_context}" +fi if [ -n "$context" ]; then [ -n "$combined" ] && combined="${combined}"$'\n' combined="${combined}${context}" diff --git a/tests/test_contract_around_the_change.py b/tests/test_contract_around_the_change.py new file mode 100644 index 0000000..839ae69 --- /dev/null +++ b/tests/test_contract_around_the_change.py @@ -0,0 +1,253 @@ +"""You altered the shape of something — here is everything that reads it (#4215). + +WHY THIS EXISTS + +Milestone 419's most repeated miss, five of seven: acting on the thing in hand +without reading the contract around it. Lesson #4207 says it in words — +"widening a tuple is an interface change to every unpack site, and the +compiler will not tell you" — and was written by its author HOURS before a +structurally identical mistake, having been surfaced twice in the turns +between. Text delivered at the moment of acting is too weak a carrier for a +reflex that has to change what the act IS. This looks it up instead. + +Rule 33 one scope down: its checks are between layers ("every parameter the +caller sends is read by the handler under the same name"), and the same +question exists between a definition and its callers. + +THREE KINDS OF EXPOSED NAME, because a contract breaks three ways that look +nothing alike in source: + + sym what is defined a rename or a removal + arg its parameter names arity and order + key quoted keys of dict literals the shape of what it RETURNS + +The third is here because of the miss that produced this step, and it is the +one a signature-watcher would have missed: `get_writepath_config` gained one +dict key, three arms read that dict inside a fail-open `except`, every one +silently became a no-op, and ten tests went red at once with nothing pointing +at the cause. No signature changed. + +WHAT IS PINNED: which edits speak and which stay silent, and that both gates +are load-bearing. NOT pinned: the wording, which is prose. + +A FIXTURE REPO, NEVER THIS ONE. Asserting against Scribe's own files would +make the test a description of today's tree, failing the next time someone +renames something (rule 115's reasoning, one floor down). +""" +import shutil +import subprocess +from pathlib import Path + +import pytest + +DEFS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" / "scribe_defs.sh" +HOOK = Path(__file__).resolve().parents[1] / "plugin" / "hooks" / "scribe_prior_art.sh" + + +def _need(*tools): + for t in tools: + if shutil.which(t) is None: + pytest.skip(f"hook runtime tool {t!r} not installed") + + +def run(script: str) -> str: + _need("bash", "awk", "git", "grep", "sed") + r = subprocess.run( + ["bash", "-c", f'set -uo pipefail\n. "{DEFS}"\n{script}'], + capture_output=True, text=True, + ) + assert r.returncode == 0, f"exit {r.returncode}: {r.stderr}" + return r.stdout + + +@pytest.fixture() +def repo(tmp_path): + """A small git repo: a definition with a reader, and one without.""" + _need("git") + # `other` lives in lib.py BESIDE widget, and nothing references it. That + # placement is the point of the no-readers case: putting it in its own + # file would leave that file as its reader, since only the file being + # edited is excluded — which is how the first version of this fixture + # quietly tested the opposite of what it claimed. + (tmp_path / "lib.py").write_text( + "def widget(size, colour):\n" + ' return {"size": size, "colour": colour}\n\n' + "def other():\n return 1\n" + ) + (tmp_path / "caller.py").write_text( + "from lib import widget\n\n" + "def render():\n" + ' return widget(1, "red")["size"]\n' + ) + (tmp_path / "unrelated.py").write_text("def gizmo():\n return 2\n") + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run(["git", "add", "-A"], cwd=tmp_path, check=True) + return tmp_path + + +def q(text: str) -> str: + """A multi-line blob as one bash word, newlines intact. + + ANSI-C quoting (`$'...'`) and NOT Python's `repr`, which is what the first + version of this file used. Inside ordinary single quotes bash leaves `\\n` + as two characters and `printf %s` does not interpret it either, so every + fixture arrived as a SINGLE line. The extractor is line-oriented, so the + cases still passed — the test would have gone green while exercising input + no editor could produce. Escaping is explicit rather than delegated to + `repr`, whose quote character depends on the content. + """ + esc = (text.replace("\\", "\\\\").replace("'", "\\'") + .replace("\n", "\\n").replace("\t", "\\t")) + return "$'" + esc + "'" + + +def block(repo, subject, old, new, ledger="") -> str: + return run( + f'scribe_contract_block "{repo}" "lib.py" "{subject}" ' + f'{q(old)} {q(new)} "{ledger}"' + ) + + +# ── scribe_exposed: the three kinds ─────────────────────────────────────── + +def exposed(text: str) -> set[str]: + out = run(f'printf %s {q(text)} | scribe_exposed') + return {ln for ln in out.split("\n") if ln.strip()} + + +def test_a_definitions_name_is_exposed(): + assert "sym\twidget" in exposed("def widget(size):\n pass\n") + + +def test_parameter_names_are_exposed(): + got = exposed("def widget(size, colour):\n pass\n") + assert "arg\tsize" in got and "arg\tcolour" in got + + +def test_self_and_cls_are_not_part_of_the_contract(): + """Nothing a caller passes, so naming them would report a change on every + method that gains a keyword argument.""" + got = exposed("def method(self, size):\n pass\n") + assert "arg\tself" not in got + assert "arg\tsize" in got + + +def test_a_type_annotation_and_a_default_are_stripped_from_a_parameter(): + got = exposed('def f(user_id: int, days: int = 30, *, flag=False):\n pass\n') + assert {"arg\tuser_id", "arg\tdays", "arg\tflag"} <= got + + +def test_a_quoted_dict_key_is_exposed(): + """THE CASE A SIGNATURE-WATCHER MISSES, and the one that produced this + step — see the module docstring.""" + assert 'key\tcheckpoint_threshold' in exposed(' "checkpoint_threshold": floor,\n') + + +def test_reading_a_dict_key_does_not_count_as_defining_one(): + """Anchored on the writing position (`"k":`), so `cfg["k"]` says nothing. + Without this every consumer of a config would report a contract change + the moment it read one.""" + assert exposed('x = cfg["tool_rule_threshold"]\ny = d.get("other")\n') == set() + + +def test_a_dunder_is_never_exposed(): + """Every class defines __init__, so it would be a guaranteed false + positive on every class edit — and noise is what teaches a reader to skip + the block.""" + assert "sym\t__init__" not in exposed(" def __init__(self, x):\n pass\n") + + +# ── The check: what speaks, and what stays quiet ────────────────────────── + +def test_a_gained_dict_key_names_the_files_that_read_it(repo): + out = block( + repo, "widget", + 'def widget(size, colour):\n return {"size": size}\n', + 'def widget(size, colour):\n return {"size": size, "weight": 1}\n', + ) + assert "widget" in out + assert "gained" in out and "weight" in out + assert "caller.py" in out + assert "unrelated.py" not in out, "only files that reference the subject" + + +def test_a_gained_parameter_names_the_files_that_read_it(repo): + out = block(repo, "widget", "def widget(size):\n", "def widget(size, colour):\n") + assert "colour" in out and "caller.py" in out + + +def test_a_rename_reports_both_halves(repo): + out = block(repo, "widget", "def widget(size):\n", "def gadget(size):\n") + assert "lost" in out and "widget" in out + assert "gained" in out and "gadget" in out + + +def test_a_body_only_edit_says_nothing(repo): + """THE GATE THAT DECIDES WHETHER THIS IS USABLE. Most edits are bodies; a + check that spoke on all of them would be skipped by the third turn.""" + out = block( + repo, "widget", + "def widget(size, colour):\n total = size\n return total\n", + "def widget(size, colour):\n total = size + 1\n return total\n", + ) + assert out.strip() == "" + + +def test_a_definition_nothing_references_says_nothing(repo): + """THE SECOND GATE, and it is not redundant. A shape change is necessary + but not sufficient — a definition with no readers has no contract to + break, and on any real repo this is what keeps the check quiet.""" + out = block(repo, "other", "def other():\n", "def other(x):\n") + assert out.strip() == "" + + +def test_the_same_subject_is_named_once_per_session(repo, tmp_path): + """Getting a change right takes several passes over the same definition, + and re-asking the same contract question at each one punishes exactly the + rhythm that gets it right.""" + ledger = tmp_path / "sid.contract.ids" + first = block(repo, "widget", "def widget(size):\n", + "def widget(size, colour):\n", ledger=str(ledger)) + assert first.strip() + second = block(repo, "widget", "def widget(size):\n", + "def widget(size, colour):\n", ledger=str(ledger)) + assert second.strip() == "" + + +def test_a_missing_old_text_says_nothing(repo): + """A Write that creates a file has no `before`, so there is no contract to + have changed. Silence, never a report against an empty string.""" + assert block(repo, "widget", "", "def widget(size, colour):\n").strip() == "" + + +# ── The hook that carries it ────────────────────────────────────────────── + +def test_the_write_hook_still_cannot_stop_a_write(): + """THE RECORDED DECISION THIS ARM MUST NOT ERODE. The operator's position + is that a recall aid may not stand in the way of a write, and this arm is + a nudge like the others — it names files, it does not gate. Asserted here + as well as in test_write_path_trigger.py because this is the change most + likely to tempt someone into making it a gate: it reports something that + may already be broken. + """ + code = [ln for ln in HOOK.read_text().splitlines() + if not ln.lstrip().startswith("#")] + assert not any("permissionDecision" in ln for ln in code) + assert not any("scribe_json_deny" in ln for ln in code) + + +def test_the_hook_asks_the_contract_question_first(): + """Order is the strength of the claim: this arm says something may already + be BROKEN, the local arm says a copy exists, the recorded arms say + something resembles this. A reader who reads one line should read that + one.""" + code = [ln for ln in HOOK.read_text().splitlines() + if not ln.lstrip().startswith("#")] + combined = next(i for i, ln in enumerate(code) if ln.startswith("combined=")) + assert "contract_context" in code[combined] + + +def test_the_hook_is_still_shell_valid(): + _need("bash") + subprocess.run(["bash", "-n", str(HOOK)], check=True) + subprocess.run(["bash", "-n", str(DEFS)], check=True)