fix(hooks): the by-name duplicate arm confirms its grep hits against the real extractor (#4227)
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
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
#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
This commit is contained in:
+103
-10
@@ -702,8 +702,55 @@ scribe_ids_minus() {
|
||||
' </dev/null 2>/dev/null
|
||||
}
|
||||
|
||||
# ARM 1, BY NAME (#2280) — and the confirmation pass that makes it mean
|
||||
# something (#4227).
|
||||
#
|
||||
# WHY A GREP IS NOT ENOUGH. The pattern below looks for a definition keyword
|
||||
# followed by the name. A grep sees LINES, not spans, so the sentence
|
||||
#
|
||||
# class with only modifier rules is a deletion that went half-way.
|
||||
#
|
||||
# — real prose, from the module docstring of scripts/check_dangling_styles.py —
|
||||
# matches it for `name=with`. #4222 fixed the other end of this same defect, in
|
||||
# the extractors that decide what a payload DEFINES; this is the end that
|
||||
# decides which other files already define it, and it was still a plain grep.
|
||||
#
|
||||
# SO EVERY HIT IS CONFIRMED by running the real extractor over the candidate
|
||||
# file and keeping only names it actually reports. That is the honest check and
|
||||
# the only one that cannot disagree with the other end of the pipe.
|
||||
#
|
||||
# TIGHTENING THE PATTERN WOULD HAVE BEEN CHEAPER AND WRONG. Requiring `(` or
|
||||
# `{` or `:` after the name rejects `class with only…` — and also rejects
|
||||
# `class Foo extends Bar {`, `class Foo : Base()` and `type Foo struct {`. This
|
||||
# arm's whole justification (#2280, #2682) is that it works with no server, no
|
||||
# index and no binding, which makes a miss here invisible. Trading a visible
|
||||
# false positive for an invisible false negative is a bad trade.
|
||||
#
|
||||
# ONCE PER DISTINCT FILE, not once per (name, file) pair: the same file is
|
||||
# usually a candidate for several names at once, and the extractor reads the
|
||||
# whole file either way. Measured on this repo, a deliberately pathological
|
||||
# payload — nine names that are ordinary English words — produced 28 distinct
|
||||
# candidate files totalling 947KB, and `scribe_defs` runs at roughly 33ms per
|
||||
# 250KB, so the confirmation costs about 200ms in the worst case anyone has
|
||||
# been able to construct here. The per-(name, file) shape would have paid that
|
||||
# several times over for the same answer.
|
||||
_SCRIBE_DUP_CANDIDATES=12
|
||||
_SCRIBE_DUP_SHOWN=4
|
||||
_SCRIBE_DUP_BUDGET=48
|
||||
|
||||
scribe_local_dups() {
|
||||
local root="$1" rel="$2" kind name pat hits count label files
|
||||
local root="$1" rel="$2" kind name pat hits
|
||||
local records="" cands="" idx=$'\n' seen=0
|
||||
local f defs line record rest files shown
|
||||
|
||||
# PASS 1 — candidates. One grep per name, unchanged except for the cap.
|
||||
#
|
||||
# THE CAP IS RAISED FROM FOUR, and that is not a detail. Confirmation REMOVES
|
||||
# hits, so capping before it runs lets three phantom matches crowd out a real
|
||||
# definition in the fourth file — hits dropped before anyone looked at them,
|
||||
# which is #4042's bug wearing a different hat. The display cap stays at four
|
||||
# (_SCRIBE_DUP_SHOWN); it now applies to CONFIRMED hits, which is where a cap
|
||||
# belongs.
|
||||
while IFS=$'\t' read -r kind name; do
|
||||
[ -n "${name:-}" ] || continue
|
||||
case "$kind" in
|
||||
@@ -712,17 +759,63 @@ scribe_local_dups() {
|
||||
esac
|
||||
# -I skips binaries; :(exclude) drops the file being written.
|
||||
# `|| true` INSIDE the substitution, not `|| hits=""` outside it (#4042):
|
||||
# under the hooks' `pipefail`, `head` exiting after four lines kills a
|
||||
# git grep that is still writing, the pipeline reports SIGPIPE, and an
|
||||
# outer fallback then wipes the four hits head already printed. The name
|
||||
# most duplicated — the one this arm exists for — was the one it dropped.
|
||||
hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null | head -4 || true)
|
||||
# under the hooks' `pipefail`, `head` exiting early kills a git grep that
|
||||
# is still writing, the pipeline reports SIGPIPE, and an outer fallback
|
||||
# then wipes the hits head had already printed. The name most duplicated —
|
||||
# the one this arm exists for — was the one it dropped.
|
||||
hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null \
|
||||
| head -"$_SCRIBE_DUP_CANDIDATES" || true)
|
||||
[ -n "$hits" ] || continue
|
||||
count=$(printf '%s\n' "$hits" | grep -c . 2>/dev/null || echo 0)
|
||||
label=$([ "$kind" = css ] && printf '.%s' "$name" || printf '%s' "$name")
|
||||
files=$(printf '%s' "$hits" | tr '\n' ' ' | sed 's/ $//')
|
||||
printf '> - `%s` is already defined in %s other file(s): %s\n' "$label" "$count" "$files"
|
||||
records+="${kind}"$'\t'"${name}"$'\t'"${hits//$'\n'/$'\t'}"$'\n'
|
||||
cands+="${hits}"$'\n'
|
||||
done
|
||||
|
||||
[ -n "$records" ] || return 0
|
||||
|
||||
# PASS 2 — what each candidate file actually defines, one extractor run per
|
||||
# file. The budget is a floor under the worst case rather than a tuning knob:
|
||||
# the upstream caps (twelve names, twelve candidates each) bound this at 144
|
||||
# files, and a repo that reached that would be paying a second of hook time
|
||||
# for a nudge. A file past the budget is DROPPED rather than passed through
|
||||
# unconfirmed — this arm is a nudge and not a gate, so an unproven claim is
|
||||
# worth less here than no claim.
|
||||
while IFS= read -r f; do
|
||||
[ -n "$f" ] || continue
|
||||
[ "$seen" -ge "$_SCRIBE_DUP_BUDGET" ] && break
|
||||
seen=$((seen + 1))
|
||||
defs=$(scribe_defs < "$root/$f" 2>/dev/null | sort -u) || defs=""
|
||||
[ -n "$defs" ] || continue
|
||||
while IFS= read -r line; do
|
||||
[ -n "$line" ] || continue
|
||||
idx+="${f}"$'\t'"${line}"$'\n'
|
||||
done <<< "$defs"
|
||||
done <<< "$(printf '%s' "$cands" | sort -u)"
|
||||
|
||||
# PASS 3 — emit the confirmed hits, in the order the names arrived.
|
||||
while IFS= read -r record; do
|
||||
[ -n "$record" ] || continue
|
||||
kind=${record%%$'\t'*}
|
||||
rest=${record#*$'\t'}
|
||||
name=${rest%%$'\t'*}
|
||||
files=""
|
||||
shown=0
|
||||
while IFS= read -r f; do
|
||||
[ -n "$f" ] || continue
|
||||
# Delimited on both sides so `usage.ts` cannot satisfy a lookup for
|
||||
# `.ts`, and `handler` cannot satisfy one for `handle`.
|
||||
case "$idx" in
|
||||
*$'\n'"${f}"$'\t'"${kind}"$'\t'"${name}"$'\n'*) ;;
|
||||
*) continue ;;
|
||||
esac
|
||||
files+="${f} "
|
||||
shown=$((shown + 1))
|
||||
[ "$shown" -ge "$_SCRIBE_DUP_SHOWN" ] && break
|
||||
done <<< "$(printf '%s' "${rest#*$'\t'}" | tr '\t' '\n')"
|
||||
[ -n "$files" ] || continue
|
||||
if [ "$kind" = css ]; then label=".$name"; else label="$name"; fi
|
||||
printf '> - `%s` is already defined in %s other file(s): %s\n' \
|
||||
"$label" "$shown" "${files% }"
|
||||
done <<< "$records"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user