refactor(hooks): read stdin via hook::buffer_stdin in advisory hooks (8 plugins) - #443
Conversation
…(8 plugins) Replace the bare `INPUT=$(cat)` fd0 read in the 8 advisory entry hooks (actionlint, bash-format, biome-format, ruff-format, markdown-format, powershell-format, eol-normalizer, desktop-notification) with the shared `hook::buffer_stdin` helper. The bare cat blocks indefinitely on the Windows Win32-pipe late-EOF stall; buffer_stdin bounds the read (default 2s) and returns the payload, so a stalled pipe degrades to a visible skip instead of hanging the hook. Empty or timed-out stdin exits 0, matching the existing empty-payload skip behavior. markdown-format's telemetry-gate test now expects the additional `jq -e .` payload-completeness probe buffer_stdin spawns (a piped read always ends `read -d ''` with a non-zero status at EOF, so the probe runs every invocation). Epic #313 deferred-backlog item 3 (advisory half; the guardrails half ships separately with fail-closed timeout semantics). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 49s —— View job PR Review
SummaryThe core change — replacing One finding and one observation below. Finding: Stale "bare cat" header comment in 7 of 8 hooks7 of the 8 changed hook scripts still carry a comment block that explicitly references the now-removed Affected files:
The "stdin is read ONCE" / "twice would drain the pipe" logic is still accurate, but the "bare cat" description and the "NEVER # hook::buffer_stdin encapsulates the Win32-pipe-safe bounded fd0 read. stdin is
# read ONCE here; reading fd0 twice would drain the pipe.
# shellcheck source=hook-utils.sh
source "$(dirname "${BASH_SOURCE[0]}")/hook-utils.sh"Observation:
|
…closed on timeout (#444) ## Summary Second half of the #313 item-3 stdin migration (sibling: #443, the 8 advisory plugins). All seven guardrails entry hooks now read stdin via the shared `hook::buffer_stdin` helper (bounded `read -t`, default 2s) instead of a bare `cat`, so the Windows Win32-pipe late-EOF stall can no longer hang a hook — and with it every tool call — indefinitely. **The load-bearing decision — blocking guards fail closed on timeout.** `hook::buffer_stdin` distinguishes rc 1 (empty/incomplete stdin) from rc 2 (read timed out before a complete JSON payload). The audit-hook idiom collapses both to a skip; for a security guard that would fail OPEN — a timed-out read means the guard could not evaluate the command, and skipping would pass exactly the traffic it exists to stop (dangerous git, hook bypass, `--no-verify`, secrets, hardcoded paths). Instead: - **5 blocking guards** (`block-dangerous-git`, `block-hook-bypass`, `block-no-verify`, `secret-pattern-detection`, `hardcoded-path-check`): rc 2 → `exit 2` (block; `buffer_stdin` already printed the `BLOCKED:` reason to stderr); rc 1 → `exit 0` (skip, matching the previous empty-payload behavior — these guards already skipped on empty `COMMAND`/fields after the bare `cat`). - **2 advisory hooks** (`flag-commit-pr-skill-bypass`, `workflow-resilience-check`): any read failure → skip, as before. guardrails **0.8.0** (behavior change on the timeout path; the previous behavior was an indefinite hang, not a skip). ## Verification - shellcheck (`--rcfile=.shellcheckrc`) + `shfmt -d` clean on all 7 hooks - All 7 guardrails test suites green (194/37/75/44/28/19/10 passes) - markdownlint clean on the CHANGELOG ## Related Epic #313 (deferred-backlog item 3, guardrails half). Sibling PR #443. Also relates to #317, #323. No linked issue: incremental epic work; closes nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…r-facing text (#503) ## Summary Internal fleet-audit jargon and internal/sibling names leaked into consumer-facing shipped files for the `markdown-format` plugin. The CHANGELOG is definitively consumer-facing (read on version bumps), so external consumers saw maintainer-only vocabulary that means nothing to them, plus a cross-reference to a sibling plugin. This is a prose-hygiene pass — not a config seam. ## Fix - **CHANGELOG.md (rewritten in place, factual record preserved):** - `0.5.1`: dropped the trailing "the same class fix applied across the fleet's other hook-plugin setups". - `0.5.0`: removed "(fleet conformance wave, dim 8 — the fleet's first conforming exemplar)". - `0.4.1`: sibling-plugin reference ("the guardrails plugin's git guards") described generically as "git-guard hooks" — also satisfies the never-cross-reference-plugin-names rule. - `0.4.0`: removed the same-class "(prerequisite-visibility wave)" rollout jargon. This instance was **not** in the issue's enumerated list but is the identical jargon class in the same consumer-facing file; removed deliberately so the hygiene pass is complete rather than leaving matching jargon behind. - **hooks/markdown-format.sh:** stripped "(dim-9 doctrine)" from two comments (lines 51, 154). - **hooks/markdown-format.test.sh:** renamed "medley-policy"/"medley policy tail" to "repo-specific policy" in the header comment and `ok`/`fail` message strings. - Added `0.5.3` CHANGELOG entry and bumped `plugin.json` `0.5.2 → 0.5.3` (docs/comment hygiene = patch), mirroring PR #443's two-file convention. `marketplace.json` carries no version field, so `plugin.json` is the sole version source. ### Zero behavior change (hook files) Hook `.sh`/`.test.sh` edits are **comments and human-readable `ok`/`fail` message strings only**. No executable logic, control flow, or matched pattern changed. Critically, the test's residual-prose assertion greps the literal `'commit/CI will block'` (unchanged) — "medley" was never load-bearing in any assertion. ## Verification All gates run against the changed files; all pass, none suppressed: - **markdownlint-cli2** v0.18.1 (`--config .markdownlint-cli2.jsonc`) on `CHANGELOG.md` → `0 error(s)`. - **shellcheck** (repo `.shellcheckrc`) on both changed hook `.sh`/`.test.sh` → clean (exit 0). - **editorconfig-checker** v3.8.0 on all 4 changed files → clean (exit 0). - **typos** 1.44.0 (repo `_typos.toml`) on `plugins/markdown-format/` → clean (exit 0). Removing "medley"/"dim" altered no allowlist entries (none existed for these terms). - **Hook contract test** (`markdown-format.test.sh`) → `PASS=65 FAIL=0`, empirically confirming no behavior change. ## Related - Closes #425 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…757) Closes #446 `hook::read_file_path` ran jq directly against the inherited fd0 with no read bound in `cli-flag-verify.sh` — the last remaining fd0-direct reader after the fleet-wide `hook::buffer_stdin` migration, same Windows Win32-pipe late-EOF stall class. Fix per the issue's decided shape: buffer first, pipe the payload into the parser; empty/timed-out stdin skips this advisory hook (plain `|| exit 0` collapse matching its class and its siblings). Guardrails 0.9.0 → 0.9.1 + CHANGELOG. Verification: `cli-flag-verify.test.sh` PASS=37 FAIL=0 (35 baseline + 2 new empty-stdin skip-contract assertions); shellcheck clean. The new test asserts the skip contract, not the stall itself — a here-string/`/dev/null` harness cannot reproduce the Win32 late-EOF stall; behavior-preservation evidence is the suite green before/after with every case now routed through the buffered read. ## Related - #443 / #444 (the `hook::buffer_stdin` migration this completes) - #547 (hook-precision umbrella; this member carries its skip-contract guard) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…lly (#751) ## Summary The `desktop-notification.test.sh` C1 fd1-leak detector false-failed on Windows Git Bash. It asserted the hook returns in a fixed `< 2000ms` against a sink that sleeps 3s — but the hook's own process-spawn overhead (jq/tr/awk subshells) is ~1.6s solo and 4-10s under parallel-suite load, so the bound had a thin-to-negative margin even though no fd1 leak exists. On this dev machine it already failed outright: `2023ms` elapsed, pure overhead, no leak. ## Fix Measure the invariant differentially instead of against a fixed wall-clock bound. The real invariant is "the hook did not wait for the backgrounded telemetry sink." A fast-sink baseline run captures the machine's current spawn overhead under the same command-substitution capture; the slow-sink run's excess over that baseline isolates the leak signal. Ambient overhead cancels in the subtraction, so the check holds under sustained load. Under a leak the sink's whole sleep lands in the delta; with no leak the delta is ~0 (± scheduling jitter). Threshold is half the sink sleep — comfortably above observed jitter, comfortably below the leak signal. **Why differential over the issue's literal Option A/B** (`< sink_sleep`, or a proportional bump): both are still *fixed* thresholds measured against *variable* overhead, and the actual trigger is sustained load (parallel suites), under which overhead rises on both the baseline and the measured run together. A differential cancels that; a fixed bound eventually loses. Fixed `< sink_sleep` would additionally need `sink_sleep > ~10s` to clear the observed 4-10s overhead — a sink that then lingers past the suite's EXIT cleanup and locks its stub file on Windows. The differential keeps the sleep small (6s), so it self-expires during the post-C1 cases before cleanup. ## Verification Windows Git Bash (Ubuntu CI spawns ~10x cheaper — always had a huge margin and is unaffected). Before (origin/main, fixed `< 2000ms`) — false-fail with no leak: ``` (C1 slow-sink elapsed: 2023ms) FAIL: telemetry/slow-sink: 2023ms — fd1 leak blocks PASS=53 FAIL=1 ``` After — 5 back-to-back runs, all green (worst observed base/slow jitter ~0.85s vs 3000ms threshold): ``` (C1 fd1-leak: base=1450ms slow=1624ms delta=174ms, threshold <3000ms, sink sleeps 6s) ... PASS=54 FAIL=0 (C1 fd1-leak: base=2008ms slow=1162ms delta=-846ms, threshold <3000ms, sink sleeps 6s) ... PASS=54 FAIL=0 (C1 fd1-leak: base=1056ms slow=1139ms delta=83ms, threshold <3000ms, sink sleeps 6s) ... PASS=54 FAIL=0 (C1 fd1-leak: base=1104ms slow=1360ms delta=256ms, threshold <3000ms, sink sleeps 6s) ... PASS=54 FAIL=0 (C1 fd1-leak: base=757ms slow=970ms delta=213ms, threshold <3000ms, sink sleeps 6s) ... PASS=54 FAIL=0 ``` Detector still catches a real leak — temporarily dropped the sink-spawn `>/dev/null` redirect in `hook::emit_telemetry` (fd1 inherited by the backgrounded sink), C1 went RED, then reverted: ``` (C1 fd1-leak: base=1876ms slow=8139ms delta=6263ms, threshold <3000ms, sink sleeps 6s) FAIL: telemetry/slow-sink: delta 6263ms ≈ sink's 6s sleep — fd1 leak blocks $() until the sink exits PASS=53 FAIL=1 ``` Full suite green after the fix: `PASS=54 FAIL=0`. No EXIT-cleanup errors across runs (the 6s sink self-expires during the later cases). Only the test file changed behaviorally; `hook-utils.sh` is unmodified (leak simulation was reverted). Closes #448 ## Related #443 (the `hook::buffer_stdin` migration that thinned the margin). Epic #313 (closed). --------- Co-authored-by: Claude Opus <noreply@anthropic.com>
…nk invariant, not half the sleep (#1362) Closes #448 ## Summary - The `#751` fix that closed `#448` could not have worked: `THRESHOLD_MS` was derived as `SINK_SLEEP * 1000 / 2`, so widening `SINK_SLEEP` widened the threshold by the same ratio and left the margin unchanged by construction. `#448` was reopened after this reproduced on clean `main` (`delta=3697ms` false-fail with no leak present), and the same construction was found ported verbatim into `markdown-format`'s C1 copy (from `#1209`). - `THRESHOLD_MS` in both plugins' C1 fd1-leak detector now asserts the actual invariant directly — sink-sleep-minus-a-safety-margin (`SAFETY_MARGIN_MS`), not half the sleep — and `SINK_SLEEP` widens from 6s to 8s (still under the 10s ceiling documented against EXIT-cleanup file-locking on Windows) for more absolute separation between ambient noise and the leak signal. - Both constants were sized against measurements taken on this machine: the reported 3697ms false-fail, and up to ~2150ms of noise generated by 30 concurrent full-suite runs under heavy load — the fixed threshold clears both with comfortable margin while staying meaningfully below the ~8000ms leak signal. - Per-plugin version bump + CHANGELOG entry in both `desktop-notification` and `markdown-format`. ## Test plan Windows Git Bash (the platform this flake is specific to — Ubuntu CI spawns ~10x cheaper and was always unaffected). - [x] 10 consecutive clean sequential runs of `desktop-notification.test.sh` — all green (`PASS=54 FAIL=0`), C1 deltas -175ms to 644ms against the new `<6000ms` threshold. - [x] 40 runs under heavy concurrent load (30x `desktop-notification.test.sh` + 10x `markdown-format.test.sh` launched simultaneously) — all green, worst observed no-leak delta ~1590ms (desktop-notification) / ~1555ms (markdown-format), no `FAIL`. - [x] Deliberately reintroduced the fd1 leak (dropped the `>/dev/null` redirect on the sink spawn in `hook::emit_telemetry`, `plugins/desktop-notification/hooks/hook-utils.sh:455`) — C1 correctly went red: `delta 8065ms ≈ sink's 8s sleep`. Reverted (`git diff` against the reverted file is empty — confirmed clean). - [x] `shellcheck` clean on both modified test files. - [x] Full suites green after the fix: `desktop-notification` `PASS=54 FAIL=0`, `markdown-format` `PASS=65 FAIL=0`. ## Related #443 (the `hook::buffer_stdin` migration that thinned the original margin). #751 (the fix that attempted to close #448 but could not — see Summary). Epic #313 (closed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Replaces the bare
INPUT=$(cat)fd0 read in the 8 advisory entry hooks (actionlint, bash-format, biome-format, ruff-format, markdown-format, powershell-format, eol-normalizer, desktop-notification) with the sharedhook::buffer_stdinhelper. The barecatblocks indefinitely on the Windows Win32-pipe late-EOF stall;buffer_stdinbounds the read (default 2s) and returns the payload, so a stalled pipe degrades to a visible skip instead of hanging the hook. Empty or timed-out stdin exits 0, matching the existing empty-payload skip behavior.Scope decisions (from the #313 item-3 scout):
stdin_read_timeoutmanifest options are re-added. The option was deliberately removed from these plugins in the feat: migrate hook kill switches to native userConfig (10 plugins) #323 review ("least churn" option); the 2s default bound fixes the actual defect. Onlyclaude-opsstill declares the knob.claude-ops/hook-telemetry-sink.shis intentionally untouched: it reads a fire-and-forget producer telemetry envelope (not a hook payload) whose producer discards stdout+stderr, its barecatissilent-skip-ok-annotated, and it must never block.Test change: markdown-format's telemetry-gate assertion now expects the additional
jq -e .payload-completeness probebuffer_stdinspawns (a piped read always endsread -d ''with a non-zero status at EOF, so the probe runs every invocation).Version bumps: actionlint 0.4.1, bash-format 0.5.1, biome-format 0.4.1, ruff-format 0.4.1, markdown-format 0.5.2, powershell-format 0.4.2, eol-normalizer 0.4.1, desktop-notification 0.4.1.
Verification
--rcfile=.shellcheckrc) +shfmt -dclean on all 8 hooksvalidate-plugins.sh,validate-plugin-contracts.mjs,check-silent-skips.shcleanRelated
Epic #313 (deferred-backlog item 3, advisory half). Also relates to #317, #323.
No linked issue: incremental epic work; closes nothing.
🤖 Generated with Claude Code