CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 57s
CI & Build / Python tests (push) Successful in 1m36s
CI & Build / Build & push image (push) Successful in 16s
Milestone 419 step 4. Five of the milestone's seven misses were the same move: acting on the thing in hand without reading the contract around it. Lesson #4207 says so in words, 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, 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 — the defined symbol (a rename or removal), its parameter names (arity), and the quoted keys of its dict literals (the shape of what it returns). THE THIRD IS THE ONE A SIGNATURE-WATCHER MISSES, and it is in because of the miss that produced this step. Two commits ago `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 with nothing pointing at the cause. No signature changed. Run against that exact edit, the check now names tests/helpers.py — the actual root cause — among six files, before the write. TWO GATES, AND THE SECOND IS WHAT MAKES IT USABLE. 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. Body-only edits say nothing, a subject is named once per session, and the ledger lives in the swept directory under the `.ids` convention, so the existing compaction-clear guards cover it — checked against test_session_ledger_clear's own parsers rather than assumed. LOCAL AND SERVERLESS, like the duplicate-name arm beside it. It needs the working tree and nothing else; the server has no checkout, so this is the only place the question can be asked. It is a NUDGE: scribe_prior_art.sh still returns no permissionDecision, which is the operator's recorded decision that a recall aid may not stand in the way of a write. A test asserts that here as well as in test_write_path_trigger.py, because this is the arm most likely to tempt someone into making it a gate — it reports something that may already be broken. The `sym` half delegates to `scribe_defs` rather than repeating its patterns: those cover nine languages and have been corrected several times, and a second copy would inherit today's version and quietly stop agreeing with it (#3497). Verified by lifting the test file's own helpers and driving all 19 cases against the real shell over a fixture git repo. Two of my own errors were caught that way and are fixed: the fixtures were arriving as single lines because Python `repr` inside bash single quotes leaves `\n` as two characters (the extractor is line-oriented, so the tests would have gone green against input no editor can produce), and the no-readers case put its subject in a file that was not the excluded one, so it had a reader and tested the opposite of its name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
254 lines
10 KiB
Python
254 lines
10 KiB
Python
"""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)
|