From 0fab08276cd9980f433e10c5af1682be961b4205 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 18:54:49 -0400 Subject: [PATCH] fix(plugin): an early-exiting head no longer voids its own output under pipefail (#4042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a repo where 3,000 files define `slug`, scribe_local_dups printed nothing; with 3 files it printed the duplicate line. Every hook runs under `set -uo pipefail`, and `hits=$(git grep -l … | head -4) || hits=""` lost head's four lines whenever git grep was still writing when head exited. That is a SIGPIPE, the substitution fails, and the outer fallback wipes the result. So the by-name duplicate arm went silent for exactly the most-duplicated names. Found while fixing the same trap in the new Stop hook (#4041). - The fallback moves inside the substitution, `$(… | head -N || true)`, at all five sites: scribe_defs.sh (local dups), scribe_prior_art.sh (names, old_first, shapes) and scribe_after_write.sh (names). A real upstream failure still yields empty output. - check_plugin gains a known-bad pattern for the shape, so no hook can bring it back. - Tests: the real function against a 3,000-file repo (past the pipe buffer), and the pattern shown to flag the old shape and pass the fix. Plugin version minted. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_after_write.sh | 3 +- plugin/hooks/scribe_defs.sh | 7 +++- plugin/hooks/scribe_prior_art.sh | 7 ++-- scripts/check_plugin.py | 13 +++++++ tests/test_hook_pipefail_head.py | 59 ++++++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 tests/test_hook_pipefail_head.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 7cd103f..323ec5f 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.14.2237", + "version": "2026.09.14.2254", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index b474098..977c7fb 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -138,7 +138,8 @@ while IFS= read -r rel_path; do code=$(cat "$file_path" 2>/dev/null) || code="" fi [ -n "$code" ] || continue - names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12) || names="" + # `|| true` inside: an early-exiting `head` must not void its own output (#4042). + names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12 || true) # Nothing DEFINED in what was written (prose, data, a call-site edit) → # nothing to say; the arms are about shapes. [ -n "$names" ] || continue diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 9da0088..86ea6bb 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -114,7 +114,12 @@ scribe_local_dups() { *) pat="(function|def|class|func|fun|fn|sub|struct|trait|interface|enum|object|protocol|type)[[:space:]]+${name}[^A-Za-z0-9_]|func[[:space:]]*\([^)]*\)[[:space:]]*${name}[[:space:]]*\(|(const|let)[[:space:]]+${name}[[:space:]]*=" ;; esac # -I skips binaries; :(exclude) drops the file being written. - hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null | head -4) || hits="" + # `|| 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) [ -n "$hits" ] || continue count=$(printf '%s\n' "$hits" | grep -c . 2>/dev/null || echo 0) label=$([ "$kind" = css ] && printf '.%s' "$name" || printf '%s' "$name") diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index 04f2cd7..d0dce6f 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -75,7 +75,8 @@ fi # in the repo? (scribe_local_dups in scribe_defs.sh carries the why.) names="" if [ -n "$code" ]; then - names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12) || names="" + # `|| true` inside: an early-exiting `head` must not void its own output (#4042). + names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12 || true) fi local_lines="" @@ -105,11 +106,11 @@ shapes="$names" if [ -z "$shapes" ] && [ -f "$file_path" ] && command -v tac >/dev/null 2>&1; then old_first=$(printf '%s' "$event" \ | jq -r '.tool_input.old_string // .tool_input.old_str // empty' 2>/dev/null \ - | grep -m1 -v '^[[:space:]]*$') || old_first="" + | grep -m1 -v '^[[:space:]]*$' || true) if [ -n "$old_first" ]; then ln=$(grep -nF -m1 -- "$old_first" "$file_path" 2>/dev/null | cut -d: -f1) || ln="" if [ -n "$ln" ]; then - shapes=$(head -n "$ln" "$file_path" | tac | scribe_defs | head -1) || shapes="" + shapes=$(head -n "$ln" "$file_path" | tac | scribe_defs | head -1 || true) fi fi fi diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index 8271ff4..9e82042 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -208,6 +208,19 @@ PATTERNS: list[tuple[re.Pattern, str, str]] = [ "produces separate encoded lines joined by raw newlines — an invalid " "URL. Use -s (slurp) as well, e.g. `jq -sRr '@uri'`.", ), + ( + # `$(producer | head -N) || var=""` — the fallback OUTSIDE the + # substitution (#4042). Every hook runs under `pipefail`; once `head` + # (or `grep -m`/`-q`) has what it needs it exits, a producer still + # writing dies of SIGPIPE, the substitution fails, and the fallback + # wipes the output that was already captured. Silent, and only on big + # inputs — which for the duplicate arm meant the most-duplicated names. + re.compile(r"\|\s*(?:head\b|grep\s+-[A-Za-z]*[mq])[^)]*\)\s*\|\|\s*\w+=\"\""), + "early-exit consumer with the fallback outside the substitution", + "Under pipefail, `$(x | head -N) || v=\"\"` discards head's output whenever x " + "is still writing when head exits (SIGPIPE). Put the fallback inside: " + "`$(x | head -N || true)`.", + ), ( re.compile(r"\|\s*cut\s+-c"), "line-oriented cut for a payload cap", diff --git a/tests/test_hook_pipefail_head.py b/tests/test_hook_pipefail_head.py new file mode 100644 index 0000000..272778c --- /dev/null +++ b/tests/test_hook_pipefail_head.py @@ -0,0 +1,59 @@ +"""Under `pipefail`, an early-exiting `head` must not void the output it kept (#4042). + +The hooks run with `set -uo pipefail`. `hits=$(git grep -l … | head -4) || +hits=""` lost its four hits whenever git grep was still writing when head +exited: SIGPIPE, a failed substitution, and the fallback wiped the result. It +only showed on big outputs, so the by-name duplicate arm went silent for +exactly the most-duplicated names. + +Two halves: the real function on a repo big enough to overflow the pipe, and +the check_plugin pattern that keeps the shape out of every hook (shown able to +fail, rule #167). +""" +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" + + +def test_the_duplicate_arm_still_names_a_name_defined_in_thousands_of_files(tmp_path): + for tool in ("git", "bash"): + 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"} + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True, env=env) + # ~3000 × ~60 bytes of `git grep -l` output: well past a 64 KiB pipe buffer, + # so git grep is still writing when head has its four lines. + for i in range(3000): + (repo / f"module_with_a_fairly_long_descriptive_name_{i}.py").write_text("def slug(t):\n return t\n") + subprocess.run(["git", "add", "."], cwd=repo, check=True, env=env) + subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True, env=env) + + script = f'set -uo pipefail\n. "{DEFS}"\nprintf "sym\\tslug\\n" | scribe_local_dups "{repo}" new.py\n' + out = subprocess.run(["bash", "-c", script], capture_output=True, text=True, env=env, timeout=60) + assert out.returncode == 0, out.stderr + assert "`slug` is already defined in 4 other file(s)" in out.stdout + + +def test_the_lint_pattern_catches_the_shape_and_passes_the_fix(): + from scripts.check_plugin import PATTERNS + + pattern = next(p for p, label, _why in PATTERNS if label.startswith("early-exit consumer")) + for bad in ('hits=$(git grep -l x | head -4) || hits=""', + " | grep -m1 -v '^[[:space:]]*$') || old_first=\"\"", + 'x=$(a | grep -q b) || x=""'): + assert pattern.search(bad), bad + for good in ('hits=$(git grep -l x | head -4 || true)', + 'ln=$(grep -nF -m1 -- "$a" "$f" | cut -d: -f1) || ln=""'): + assert not pattern.search(good), good