Skip to content

feat(claude-ops): surface silent hook launch failures via a decoupled Stop audit - #2580

Merged
kyle-sexton merged 4 commits into
mainfrom
claude-ops-2577-hook-failure-audit
Aug 14, 2026
Merged

feat(claude-ops): surface silent hook launch failures via a decoupled Stop audit#2580
kyle-sexton merged 4 commits into
mainfrom
claude-ops-2577-hook-failure-audit

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Problem

Claude Code records a hook that fails to launch only as a hook_non_blocking_error transcript attachment — the guarded tool call proceeds as if approved, and nobody is told. The #1416#2570#2572#2571 chain fixed the disk-hygiene instances and gated the source shape, but the fleet's only silent-failure detector (disk-hygiene's guard_launch_monitor.py) lives inside the plugin it watches and launches through the same registration form it watches. Full-fleet transcript mining on the incident host (97 transcript files, all projects) shows what that coupling costs:

hook failures stderr
PreToolUse:Bashdestructive_guard.py 95 execvpe(/bin/bash) failed (WSL relay)
PreToolUse:PowerShelldestructive_guard.py 45 same
Stopguard_launch_monitor.py (the detector itself) 23 same

And the stale-session window no source-side gate can reach: hook config loads at session start, so a session running when the #2570 fix landed on disk (2026-08-13T21:19:56Z) kept executing the dead exec-form config — 22 further failures after the fix shipped, latest 2026-08-14T03:53Z, guard and detector both dead, zero operator-visible signal.

Fix

hook-failure-audit.sh, an eighth claude-ops *-audit hook, registered on Stop:

  • Decoupled by construction: lives in a plugin whose hook registrations have been shell-form "${CLAUDE_PLUGIN_ROOT}"/hooks/*.sh throughout — alive during the entire incident, including the stale-session window. A defect that kills a watched plugin's launch path cannot take this detector with it.
  • Bounded cost: Stop cadence (once per turn, per guard_launch_monitor.py's ADR 0004 / D-12 rationale), transcript-tail read capped at 2 MB with the truncated first line dropped — O(cap), not O(session length).
  • Structural matching, never substring: a record counts only when top-level .type == "attachment" and .attachment.type == "hook_non_blocking_error". A hook_success whose stdout quotes an error, and a message record quoting a failure record as a string — both false-positive shapes hit while mining the incident transcripts — cannot fire it (both pinned in the contract test).
  • Once per session per distinct failing hook, re-warning when a new hook starts failing; marker bookkeeping under ${CLAUDE_PLUGIN_DATA} degrades toward re-warning, never toward silence.
  • systemMessage names the failing hooks, counts, a stderr snippet, the fail-open consequence, and the restart-to-reload remedy for the stale-session case; telemetry envelope carries privacy-safe subjects (hook names only).
  • Advisory: always exit 0; fail-open jq gate with the standard skip notice; kill switch hook_failure_audit_enabled.

Overlap with guard_launch_monitor.py is deliberate: that monitor keeps its guard-specific semantics; a destructive-guard failure may warn twice. Its own doc block names over-warning as the safe direction for this class.

Red-first evidence

hook-failure-audit.test.sh was written and run before the hook existed:

FAIL: failure surfaced -> exit 0: expected exit 0, got 127
FAIL: names the dead hook: 'PreToolUse:Bash' not in: bash: .../hook-failure-audit.sh: No such file or directory
...
PASS=4 FAIL=20

With the hook in place: PASS=28 FAIL=0. The core fixture is a structural copy of a real incident attachment record (WSL-relay stderr, session ac1c95e3).

Gates

  • hook-failure-audit.test.sh — PASS=28 FAIL=0
  • shellcheck both new files — clean
  • check-hook-exec-form.sh — clean (the new registration is shell form)
  • check-changelog-parity.sh --check / --check-bump origin/main / --check-order — clean (0.31.14 → 0.32.0)
  • check-silent-skips.sh, check-hook-userconfig-argv.sh, check-manifest-duplicate-keys.py, check-cross-plugin-source-drift.sh — clean
  • check-shell-portability.sh origin/main — clean
  • typos on all touched files — clean
  • lefthook pre-commit suite — passed on commit

Related

Closes #2577

🤖 Generated with Claude Code

https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W

…pled Stop audit (#2577)

A hook that fails to launch is a non-blocking error: the guarded tool
call proceeds as if approved, and the only durable trace is a
hook_non_blocking_error transcript attachment nobody reads. The #1416
incident class proved an in-plugin detector is no shelter — disk-hygiene's
own Stop monitor shared its guard's registration form and died the same
launch death on all 23 of its runs (163 unsurfaced failures on the
incident host, 22 of them after the fix was on disk, in a session still
running stale pre-fix hook config).

hook-failure-audit is the decoupled detector: registered here on Stop in
shell form, bounded transcript-tail read, structural attachment-type
matching (never substring), once per session per distinct failing hook
with marker loss degrading toward re-warning, systemMessage naming the
failing hooks and the stale-session restart remedy, and the standard
telemetry envelope with hook-name-only subjects.

Red-first: hook-failure-audit.test.sh failed 20/24 assertions against
the pre-fix tree (script absent), passes 28/28 with the hook in place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
@cursor

cursor Bot commented Aug 14, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 11s —— View job


Claude Code Review

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Check skip gate (open/draft/trivial/duplicate review)
  • Review changed files for correctness & maintainability
  • Adversarially validate candidate findings
  • Post inline comments for findings
  • Post final summary

Scope note: This repo carries a claude-security-review workflow, so security findings are out of scope for this lane (per REVIEW.md). This review covers correctness, design, error handling, observability, and tests for the 6 changed files (hook-failure-audit.sh, hook-failure-audit.test.sh, hooks.json, plugin.json, README.md, CHANGELOG.md).

Findings

None met the high-signal bar (would a careful senior reviewer block or flag it). Areas specifically checked and found sound:

  • Structural matching (hook-failure-audit.sh:82-91) — select(.type? == "attachment") | .attachment | select(.type? == "hook_non_blocking_error") genuinely can't be tripped by the two false-positive shapes the test fixtures pin (a hook_success quoting error text in stdout, a message record quoting a failure as a string).
  • set -uo pipefail interaction — the read_window | grep -F ... | jq ... pipeline returns nonzero when grep finds no match (pipefail semantics), but since SUMMARY=$(...) isn't gated by set -e or ||, this doesn't short-circuit the script; the empty-array case is handled correctly by the subsequent [[ -n "$SUMMARY" && "$SUMMARY" != "[]" ]] || exit 0 check.
  • Fail-open/degrade-toward-rewarn discipline — every early-exit path (hook::buffer_stdin failure, missing jq, missing/absent transcript, mkdir -p failure for the marker dir) correctly falls through to exit 0 or leaves WARNED empty rather than silently suppressing a real warning, consistent with the stated doctrine and with guard_launch_monitor.py's.
  • Per-hook dedup vs. the disk-hygiene monitor's per-session dedupguard_launch_monitor.py warns once per session because it watches one hook; this one tracks per distinct hookName because it watches every hook in the fleet. Verified this is a deliberate, documented divergence, not an inconsistency.
  • Path/argument safety$TRANSCRIPT is always passed through -- before tail/cat, guarding against a path that looks like an option.
  • Config wiringhook_failure_audit_enabledCLAUDE_PLUGIN_OPTION_HOOK_FAILURE_AUDIT_ENABLED in plugin.json matches the hook::check_enabled "HOOK_FAILURE_AUDIT" call and the kill-switch var the test exercises.
  • Registration (hooks.json) — new Stop entry uses the same "${CLAUDE_PLUGIN_ROOT}"/hooks/*.sh shell form as every other hook in this file, consistent with the PR's core decoupling claim.

One pre-existing, documented tradeoff worth naming for completeness rather than as a defect: read_window's unconditional sed '1d' on the truncated-tail branch (hook-failure-audit.sh:72-78) will occasionally drop a complete record if the tail cut happens to land exactly on a line boundary, not just a genuinely partial one. This mirrors guard_launch_monitor.py's identical accepted approximation and isn't something this PR introduces or needs to fix.

Not independently verified: the PR's Red-first test run (PASS=28/FAIL=0), shellcheck, and the various check-* gate scripts — Bash execution in this environment required an approval step not available to this review pass, so those results are author-reported, not independently confirmed here. Static reading of hook-failure-audit.test.sh shows its 28 assertions do exercise the claimed behaviors (structural matching, dedup, kill switch, bounded tail, missing/absent transcript, telemetry envelope) in a way consistent with the implementation.

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 0s —— View job


Security Review — /review:security-review

  • Gather context (PR diff, changed files)
  • Read new hook script and test file
  • Analyze for security issues (injection, path traversal, data handling, privilege/trust boundary)
  • Check hooks.json registration form
  • Post final review

Scope: plugins/claude-ops/hooks/hook-failure-audit.sh (new), hook-failure-audit.test.sh (new), hooks.json, plugin.json, README.md, CHANGELOG.md. hook-utils.sh (the shared library the new hook calls into) is unchanged in this PR, so its helpers were read for context but not re-audited.

No security issues found. Details of what I checked:

  • Command/injection surface: hook-failure-audit.sh never evals transcript-derived content. tail/cat/wc on $TRANSCRIPT are invoked with -- before the path (L74-76), preventing option injection if the path is attacker-shaped.
  • Matching is structural, not substring (select(.type? == "attachment") | .attachment | select(.type? == "hook_non_blocking_error"), L85-86), and the test file pins both false-positive shapes the PR description calls out — confirmed these are asserted (assert_absent) in hook-failure-audit.test.sh L62-63.
  • Marker/path handling: SESSION is sanitized to [A-Za-z0-9_-] before being used as a filename component (L65), which rules out path traversal into ${CLAUDE_PLUGIN_DATA}/hook-failure-audit/ via a crafted session_id.
  • Untrusted-content reflection: hook names and a 160-char stderr snippet pulled from the transcript flow into the systemMessage, but that path goes through the shared hook::emit_system_messagehook::json_escape, which both JSON-escapes and strips residual C0 control bytes (including ESC/0x1B) — so this doesn't add a new terminal-escape or JSON-breakout vector beyond what the existing library already guards against.
  • Telemetry privacy: the envelope payload is {subjects: [hookName...], total} only (L128-129) — no stderr, no command, no path — consistent with the "privacy-safe subjects" claim in the PR description and README.
  • Trust boundary: transcript_path and session_id come from the Stop-hook payload the Claude Code harness itself supplies, not from remote/attacker-controlled tool input, so reading $TRANSCRIPT isn't a new arbitrary-file-read primitive.
  • No GitHub Actions/workflow files, permissions, or dependency pins touched in this PR — nothing for the Actions-security or supply-chain checks to flag.
  • Advisory posture confirmed: fails open on missing jq, missing transcript, disabled kill switch, and missing marker home (re-warns rather than silently suppressing) — no path found that turns this into a blocking/denial surface.

Everything above is defense-in-depth reasoning on data that stays local to the user who owns the session; I didn't find a concrete exploitable path in this diff.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c4294b314

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/claude-ops/hooks/hooks.json
Comment thread plugins/claude-ops/hooks/hook-failure-audit.sh Outdated
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

Last security-reviewed head: 0a2a60d18ba2366ed83114549dfea1286829d1d1. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Live end-to-end proof against the real incident transcript

Ran the new hook exactly as Claude Code would launch it, with transcript_path pointed at the incident session's actual transcript (ac1c95e3, the session that accumulated the #2577 failures — including 22 recorded after the disk-hygiene fix was on disk, because the session still runs its stale in-memory hook config):

$ printf '{"session_id":"live-proof","transcript_path":".../ac1c95e3-....jsonl","hook_event_name":"Stop"}' \
    | bash plugins/claude-ops/hooks/hook-failure-audit.sh
{"systemMessage":"claude-ops: 81 hook failure record(s) in this session's transcript were never surfaced:
 PreToolUse:Bash (66x; last stderr: Failed with non-blocking status code: <3>WSL (373226 - Relay) ERROR:
 CreateProcessCommon:818: execvpe(/bin/bash) failed: No such file or directory);
 PreToolUse:PowerShell (1x; ...); Stop (14x; ...).
 A hook that fails to launch enforces nothing — the tool calls it guards proceed as if approved (fail-open).
 If a plugin update changed hook config on disk mid-session, this session still runs the config it loaded
 at startup — restart the session to load the fix."}
$ rc=0

81 rather than the session's full 140+: the 2 MB tail cap bounds the window by design — earlier records aged out of it, exactly the bounded-cost tradeoff guard_launch_monitor.py documents. Every surfaced record is one the operator was never told about.

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 35s —— View job


Code Review — /review:code-review

  • Gather context (PR diff, changed files)
  • Read new hook script and test file
  • Check hooks.json registration + file permissions
  • Analyze for correctness/maintainability issues
  • Post final review

Scope: plugins/claude-ops/hooks/hook-failure-audit.sh (new), hook-failure-audit.test.sh (new), hooks.json, plugin.json, README.md, CHANGELOG.md, docs/CATALOG.md. Security is out of scope for this lane (covered separately). Reviewed against origin/main...HEAD.

Posted 2 inline findings:

  1. Blocking — registered script is not executable (hooks.json:107). hook-failure-audit.sh is committed as mode 100644, but the Stop registration invokes it directly (no bash prefix), matching every other hook entry in this file — all of which are 100755. Confirmed via git ls-files -s: every other *-audit.sh sibling is executable; this one and its test file are not. On a POSIX checkout this is exit 126 on every Stop — the detector this PR exists to add would never run. The contract test can't catch this because it calls bash "$HOOK" directly rather than executing the registered command form.

  2. Dedup granularity gap (hook-failure-audit.sh:90). Grouping/marking is keyed on .hookName (hookEvent:matcher) alone, which isn't a per-registration identity — the PR's own doc comments describe multiple plugins sharing an event+matcher pair. If registration A under a given hookName fails and gets marked warned, a later, distinct registration B failing under that same hookName is silently suppressed for the rest of the session — a real gap against the script's own stated goal ("degrades toward re-warning, never toward silence"). The attachment record already carries .command, which could be folded into the grouping/marker key while keeping the public subject privacy-safe.

Everything else checked out:

  • Kill switch wiring is correct end-to-end (hook_failure_audit_enabledCLAUDE_PLUGIN_OPTION_HOOK_FAILURE_AUDIT_ENABLEDhook::check_enabled "HOOK_FAILURE_AUDIT").
  • Structural (non-substring) matching logic is sound and the two false-positive shapes named in the PR description are genuinely pinned by the test file.
  • hook::require_jq/hook::jq_field/hook::emit_system_message/hook::buffer_stdin usage matches the established conventions used by sibling *-audit hooks.
  • Bounded tail-read with dropped truncated first line is a reasonable, previously-established pattern (mirrors guard_launch_monitor.py's approach per the PR description).
  • README/CHANGELOG/CATALOG/plugin.json prose updates (skill count language, options table, kill-switch table) are internally consistent with the new hook's behavior.

I did not independently execute the test suite, shellcheck, or the other CI gates listed in the PR description (no Bash tool access to run them in this lane) — those are author-claimed, not independently verified here.

Comment thread plugins/claude-ops/hooks/hooks.json
Comment thread plugins/claude-ops/hooks/hook-failure-audit.sh Outdated
…udit (#2577)

Review findings from #2580 (chatgpt-codex-connector P1/P2, both confirmed):

P1 — the registration invokes the script directly, but the blob was
committed 100644: on a POSIX checkout every Stop exec fails with 126,
making the silent-failure detector itself a silent failure. Now 100755,
and the contract test executes the script the way the registration does
(no bash prefix) so the shape is pinned cross-platform; the hygiene
lane's shebang-executable gate caught the same defect in CI.

P2 — dedup keyed on hookName alone collapsed distinct registrations
sharing an event:matcher (several PreToolUse:Bash guards exist in this
marketplace): registration B's first failure hid behind registration A's
earlier warning. Identity is now (hookName, command); the systemMessage
carries the registered command so the operator can tell which guard is
dead; telemetry subjects stay hookName-only.

Red-first: the new shared-hookName test case failed against the previous
commit (silent when it must re-warn), passes now — 30/30. Fixing it also
surfaced that this host's jq emits CRLF, which had salted marker
fingerprints with carriage returns; both sides of the comparison now
normalize (rtrimstr on read, tr -d on write).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 32s —— View job


Claude Code Review

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Check skip gate (open PR, non-trivial, re-review after fix push)
  • Review changed files for correctness & maintainability
  • Adversarially validate candidate findings
  • Post inline comments for findings
  • Post final summary

Scope note: Security is covered by a separate claude-security-review lane per REVIEW.md; this pass covers correctness/maintainability only. This is a re-review triggered by the new commit 0a2a60d (fix(claude-ops): exec bit + per-registration dedup for hook-failure-audit), pushed in response to the two P1/P2 findings raised earlier in this PR by both chatgpt-codex-connector and the prior Claude code-review pass.

Verified: both prior findings are correctly fixed

  1. Exec bit (blocking). git ls-files -s now shows hook-failure-audit.sh and hook-failure-audit.test.sh at 100755 (were 100644). The Stop registration in hooks.json invokes the script directly with no bash prefix, matching every sibling *-audit.sh entry, so this would previously have died with exit 126 on every Stop — the detector this PR exists to add would never have run. Good catch that the test harness itself was updated (hook-failure-audit.test.sh:47-51) to invoke the script the way the registration does (no explicit interpreter) — this closes the exact gap that let the mode bug through the first contract-test run, and CI's shebang-executable gate is now a second line of defense.

  2. Dedup granularity. Grouping/marking is now keyed on (hookName, command) rather than hookName alone (hook-failure-audit.sh:89-100), so two distinct registrations sharing an event+matcher (e.g. two different PreToolUse:Bash guards) are now tracked independently instead of the second silently hiding behind the first's marker. The systemMessage now includes the truncated command so an operator can tell which registration is dead (L128-129), while telemetry subjects stay hookName-only for privacy (L144-145). New test coverage exercises exactly this scenario (hook-failure-audit.test.sh:81-90). A nice bonus catch in the same commit: normalizing CRLF (rtrimstr/tr -d '\r') on both sides of the marker-fingerprint comparison, which the author found was silently salting comparisons on a jq build that emits CRLF.

Checked and sound

  • The new command field flowing into systemMessage still goes through hook::json_escape via hook::emit_system_message (hook-utils.sh:62-91, unchanged by this diff) — no new escaping gap from surfacing more transcript-derived content in the message.
  • Fix commit is tightly scoped: only the two files needed for the code change plus README/CHANGELOG prose updates — no unrelated changes riding along.
  • CHANGELOG.md/README.md prose was updated in the same commit to describe the new (hookName, command) identity, keeping docs and behavior in sync.

No new findings on this diff. Nothing to flag as blocking; no inline comments needed since there's nothing new to anchor.

Not independently verified: I don't have Bash execution approval in this lane, so I read the diff/files directly rather than running hook-failure-audit.test.sh, shellcheck, or the other CI gates — the author's commit message states 30/30 passing, consistent with what the test file's added assertions show statically.

Branch: claude-ops-2577-hook-failure-audit

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 33s —— View job


Security Review — /review:security-review

  • Gather context (PR diff, changed files, comment history)
  • Invoke /review:security-review skill instructions
  • Review commits since the last security-reviewed head (1c4294b)
  • Post final review

Scope: Per the skill's skip gate, the last security review on this PR covered head 1c4294b3 and found no issues. Since then three commits landed — e9ebbbd (docs/CATALOG.md), c851bcd (README.md options table), and 0a2a60d (the fix addressing the two prior code-review findings: exec bit + dedup granularity). I reviewed the full delta (git diff 1c4294b..0a2a60d) rather than blanket-skipping, since 0a2a60d does touch the structural jq matching and dedup logic previously reviewed.

Findings

None. No security issues in the delta.

Detail on what changed and why it doesn't introduce a new surface:

  • hook-failure-audit.sh — dedup key changed from hookName alone to (hookName, command), adding the attachment's .command field (truncated to 120 chars) into the grouping, the marker fingerprint, and the systemMessage detail string. command originates from the hook's own registration config in hooks.json (trusted, repo/plugin-owned content, not remote/attacker-controlled input) — the same trust tier as hookName and stderr, which were already reviewed. It flows into systemMessage through the same hook::emit_system_messagehook::json_escape path already verified to JSON-escape and strip control bytes, so no new injection vector there.
  • Marker file fingerprint (hookName\tcommand) — an embedded tab or newline in command could in principle desync a marker line from the expected two-field format. Since command is trusted local config rather than attacker input, and the failure mode is "re-warn instead of dedup" (fail-open toward the safe direction this hook already commits to), this isn't an exploitable issue — worth noting only because it was checked, not because it's a finding.
  • Telemetry envelope — still {subjects: ([...hookName] | unique), total}. command was deliberately not added to telemetry, preserving the privacy-safe-subjects guarantee from the original review.
  • Mode bit change (100644100755) on hook-failure-audit.sh/.test.sh — a permissions fix (addresses the P1 code-review finding that the hook silently never ran), not a security-relevant permission widening; no setuid/group-writable bits involved.
  • Docs-only commits (e9ebbbd, c851bcd) — no code, no security surface.

No GitHub Actions/workflow files, permission grants, or dependency pins touched in this delta.

@kyle-sexton
kyle-sexton merged commit 9623738 into main Aug 14, 2026
39 checks passed
@kyle-sexton
kyle-sexton deleted the claude-ops-2577-hook-failure-audit branch August 14, 2026 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant