Files
FabledScribe/tests/test_hook_duplicate_confirmation.py
T
bvandeusenandClaude Opus 5 029692945e
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / integration (push) Successful in 51s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / Python tests (push) Successful in 1m48s
CI & Build / Build & push image (push) Successful in 15s
fix(hooks): the by-name duplicate arm confirms its grep hits against the real extractor (#4227)
#4222 fixed one end of this defect — the extractors that decide what a payload
DEFINES now blank comment and string spans before any line matcher runs. This
is the other end: `scribe_local_dups`, which decides which OTHER files already
define that name, and which was still a plain `git grep`.

A grep sees lines, not spans, so the sentence

    class with only modifier rules is a deletion that went half-way.

— real prose from a module docstring in this repo — matched the arm's pattern
for `name=with`. Writing a genuine `change`, `beside` or `wrapped` would be
told it already existed, and pointed at a docstring.

EVERY HIT IS NOW CONFIRMED by running `scribe_defs` over the candidate file and
keeping only names it actually reports. That is the only check that cannot
disagree with the other end of the pipe, which is the whole point.

MEASURED, NOT ASSUMED — both numbers the task reasoned from turned out wrong.

  - WHAT IT REMOVES, across 141 payload files of this repo: 15 of 210 report
    lines. Every one a string literal, a comment, a TypeScript `import { type
    Foo }`, or Vue's `const emit = defineEmits()` boilerplate. No real
    definition was lost. Where a name had both — `create_note` — the phantom
    in a test's `shape_form("async def create_note(...)")` argument dropped out
    and the definition in services/notes.py stayed. `with` went from four files
    to none, which is the correct answer: nothing here defines it, and it is a
    keyword in several of these languages.

  - WHAT IT COSTS: mean 193ms -> 222ms, worst 569ms -> 572ms. The task feared
    "over a second added to a PreToolUse hook" from 48 confirmations. It is
    about 15%, because the arm was already dominated by its twelve `git grep`
    calls, and because confirmation runs once per DISTINCT candidate file
    rather than once per (name, file) pair. A deliberately pathological payload
    — nine names that are ordinary English words — reaches 28 distinct files
    and 947KB; `scribe_defs` runs at ~33ms per 250KB.

THE CANDIDATE CAP IS RAISED FROM FOUR TO TWELVE, and that is load-bearing.
Confirmation REMOVES hits, so capping before it runs lets phantom matches crowd
a real definition out of the window — hits dropped before anyone looked at
them, which is #4042's bug in a new place. The display cap stays at four and
now applies to CONFIRMED hits, which is where a cap belongs. #4042's own `||
true` inside the substitution is untouched, and its regression case still
passes.

The task's own advice not to fix this by tightening the grep pattern is
followed and written down: requiring `(` or `{` or `:` after the name rejects
`class with only…` and also `class Foo extends Bar {`, `class Foo : Base()` and
`type Foo struct {`. This arm exists because it works with no server, no index
and no binding (#2280, #2682), which makes a miss here invisible — a visible
false positive is the better failure.

Guards: tests/test_hook_duplicate_confirmation.py, against real git repos
because what is pinned is the interaction between `git grep`, `head` and the
extractor. Seven of the eight fail against the previous implementation; the
eighth is #4042's regression case, whose job is to keep passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
2026-09-21 02:49:07 -04:00

225 lines
9.7 KiB
Python

"""The by-name duplicate arm confirms its grep hits against the real extractor (#4227).
#4222 fixed one end of this defect: the extractors that decide what a payload
DEFINES now blank comment and string spans before any line matcher runs. This
is the other end — `scribe_local_dups`, which decides which OTHER files already
define that name — and it was a plain `git grep`.
A grep sees lines, not spans. The line
class with only modifier rules is a deletion that went half-way.
is prose from a module docstring in this repo, and it matches the arm's pattern
for `name=with`. So the arm would tell a writer their genuine symbol was
already defined, and point at a docstring.
WHAT THE FIX COSTS, measured rather than guessed, over 141 payload files of
this repo: the arm removed 15 of 210 report lines, and every one was a string
literal, a comment, an import statement, or Vue's `const emit = defineEmits()`
boilerplate. No real definition was lost — where a name had both a real
definition and a phantom, the phantom was dropped and the definition kept.
Timing: mean 193ms → 222ms, worst 569ms → 572ms. The arm was already dominated
by its twelve `git grep` calls, so confirming costs about 15% rather than the
second a per-(name, file) shape would have cost.
These cases are written against real git repositories rather than mocks,
because what is being pinned is the interaction between `git grep`, `head` and
the extractor — which is where #4042 lived too.
"""
from __future__ import annotations
import os
import shutil
import subprocess
from pathlib import Path
import pytest
ROOT = Path(__file__).resolve().parents[1]
DEFS = ROOT / "plugin" / "hooks" / "scribe_defs.sh"
@pytest.fixture
def repo(tmp_path):
"""A committed git repo, plus a runner for the arm against it."""
for tool in ("git", "bash", "awk"):
if shutil.which(tool) is None:
pytest.skip(f"{tool!r} not installed")
env = {"PATH": os.environ["PATH"], "HOME": str(tmp_path),
"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@x",
"GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@x"}
root = tmp_path / "repo"
root.mkdir()
subprocess.run(["git", "init", "-q"], cwd=root, check=True, env=env)
def build(files: dict[str, str]):
for rel, text in files.items():
path = root / rel
path.parent.mkdir(parents=True, exist_ok=True)
path.write_text(text)
subprocess.run(["git", "add", "-A"], cwd=root, check=True, env=env,
capture_output=True)
subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=root,
check=True, env=env, capture_output=True)
def run(names: str, writing: str = "new.py") -> str:
script = (f'set -uo pipefail\n. "{DEFS}"\n'
f'printf %s "$NAMES" | scribe_local_dups "{root}" {writing}\n')
out = subprocess.run(["bash", "-c", script], capture_output=True,
text=True, env={**env, "NAMES": names}, timeout=120)
assert out.returncode == 0, out.stderr
return out.stdout
return build, run
# ── the case the task names ──────────────────────────────────────────────────
def test_a_sentence_about_a_definition_is_not_a_definition(repo):
"""The verify case from #4227: one real `def change`, one docstring that
wraps onto a line beginning `function change`. The writer is told about
the first file and not the second."""
build, run = repo
build({
"real.py": "def change(a, b):\n return a\n",
"prose.py": (
'"""Some module.\n\n'
'When a caller moves, the enclosing\n'
'function change may have moved with it, which is the case\n'
'this paragraph exists to describe.\n'
'"""\n\n'
'X = 1\n'
),
})
out = run("sym\tchange\n")
assert "real.py" in out
assert "prose.py" not in out, (
"a wrapped docstring line still reads as a definition — the "
"confirmation pass did not run, or did not see the span"
)
assert "in 1 other file(s)" in out
def test_the_repos_own_prose_no_longer_defines_with():
"""Not a fixture — this repo's actual corpus, which is where the defect
was found.
Before the confirmation pass the arm named four files for `with`:
`…record type with no vector…`, `A class with no rules anywhere…` and two
more, all of them prose in docstrings and comments. It now names none,
and none is the correct answer: nothing here defines a symbol called
`with`, and in several of the languages in this tree it could not, because
it is a keyword.
Asserting on emptiness is only honest if the assertion could see a hit —
so it checks the arm ran, via a name that IS defined here.
"""
if shutil.which("bash") is None:
pytest.skip("bash not installed")
def arm(name: str) -> str:
script = (f'set -uo pipefail\n. "{DEFS}"\n'
f'printf "sym\\t{name}\\n" | scribe_local_dups "{ROOT}" nothing.py\n')
out = subprocess.run(["bash", "-c", script], capture_output=True,
text=True, timeout=180)
assert out.returncode == 0, out.stderr
return out.stdout
assert arm("extract_definitions").strip(), (
"the arm found nothing for a name this repo certainly defines, so a "
"silent result below would prove nothing"
)
assert arm("with").strip() == "", (
"`with` is prose in every file that matches the grep — a hit here is "
"a sentence being read as a definition"
)
def test_a_name_that_is_both_real_and_phantom_keeps_the_real_file(repo):
"""The narrowing case, and the one that would betray an over-eager filter:
the phantom file drops out and the genuine definition stays."""
build, run = repo
build({
"lib.py": "def create_note(user_id):\n return user_id\n",
"test_form.py": 'canon = shape_form("async def create_note(user_id: int, ...):", "sym")\n',
})
out = run("sym\tcreate_note\n")
assert "lib.py" in out
assert "test_form.py" not in out
assert "in 1 other file(s)" in out
def test_an_import_is_not_a_definition(repo):
"""TypeScript's `import { type Foo }` matched the `type\\s+NAME` arm, so a
new type was reported as a duplicate of the files importing it."""
build, run = repo
build({
"card.vue": 'import {\n type Choices, type Decision,\n} from "@/api/inception";\n',
"real.ts": "export type Choices = { a: number };\n",
})
out = run("sym\tChoices\n")
assert "card.vue" not in out, "an import site is not a definition site"
# ── the caps, which is where #4042 lived ─────────────────────────────────────
def test_the_candidate_cap_is_raised_above_the_display_cap():
"""Confirmation REMOVES hits, so a cap applied before it runs lets phantom
matches crowd real definitions out of the window — hits dropped before
anyone looked at them, which is #4042's bug in a new place. Asserted on
structure (rule 167) because the ordering is what matters, not the
numbers."""
src = DEFS.read_text()
for const in ("_SCRIBE_DUP_CANDIDATES=", "_SCRIBE_DUP_SHOWN="):
assert const in src, f"{const.rstrip(chr(61))} is gone — the two caps were folded back together"
cands = int(src.split("_SCRIBE_DUP_CANDIDATES=")[1].split("\n")[0])
shown = int(src.split("_SCRIBE_DUP_SHOWN=")[1].split("\n")[0])
assert cands > shown, (
f"candidates ({cands}) must exceed the display cap ({shown}), or "
"confirmation can only ever shrink an already-truncated window"
)
def test_phantoms_ahead_of_the_display_cap_do_not_hide_a_real_definition(repo):
"""Four phantom files sort before the one real definition. With the old
`head -4` on the grep, the real file never entered the window at all."""
build, run = repo
files = {f"a_phantom_{i}.py": f'BLURB = """\nclass Widget is described here.\n"""\n'
for i in range(4)}
files["z_real.py"] = "class Widget:\n pass\n"
build(files)
out = run("sym\tWidget\n")
assert "z_real.py" in out, "the real definition sorted behind four phantoms"
assert "a_phantom_0.py" not in out
def test_the_early_exiting_head_still_keeps_its_own_output(repo):
"""#4042, re-run through the new shape: under `pipefail` a `head` that
exits while git grep is still writing must not void the hits it printed.
Every file here holds a REAL definition, so confirmation keeps them all
and only the display cap applies."""
build, run = repo
build({f"module_with_a_fairly_long_descriptive_name_{i}.py": "def slug(t):\n return t\n"
for i in range(3000)})
out = run("sym\tslug\n")
assert "`slug` is already defined in 4 other file(s)" in out
# ── css ──────────────────────────────────────────────────────────────────────
def test_a_selector_named_inside_a_css_comment_is_not_a_rule(repo):
"""#2990's defect, on the lookup side rather than the extraction side."""
build, run = repo
build({
"real.css": ".badge { color: red; }\n",
# The selector must START the line, or the old anchored pattern
# never matched it and this case would pass without the fix.
"notes.css": "/*\n.badge, .chip and friends were retired here\n*/\na { color: blue; }\n",
})
out = run("css\tbadge\n")
assert "real.css" in out
assert "notes.css" not in out