fix(repo-hygiene): destructive-guard fail-open on Windows — shell-form hook launch + jq fail-closed degraded mode - #1006
Conversation
…m hook launch + jq fail-closed degraded mode Exec-form hook resolved bash via PATH, hitting the WSL relay on Windows; launch failure is non-blocking so the guard enforced nothing. Shell form with shell: bash lets Claude Code resolve Git Bash itself. Missing jq now blocks destructive patterns against the raw payload instead of going inert. Closes #992 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
|
Claude finished @kyle-sexton's task in 2m 20s —— View job PR Review —
|
|
Claude finished @kyle-sexton's task in 3m 16s —— View job Security Review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d70bf35af
ℹ️ 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".
…fore matching In the raw payload a command starts right after a double quote, so the rm pattern's (^|[[:space:];&|(]) anchor never matched. Quotes become spaces before the destructive patterns run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
|
Claude finished @kyle-sexton's task in 1m 59s —— View job PR Review —
|
| Input | After //\"/ |
rm anchor |
-r check |
-f check |
Result |
|---|---|---|---|---|---|
rm -rf build/ |
: rm -rf build/ |
: +rm ✓ |
-rf✓ |
-rf✓ |
block ✓ |
git clean -fdx |
: git clean -fdx |
— | — | — | via git branch ✓ |
git status |
: git status |
no match | — | — | pass ✓ |
CLEAN_GUARD_ACK=1 git clean -fdx |
: CLEAN_GUARD_ACK=1 git clean -fdx |
— | — | — | via git branch, block ✓ |
The CLEAN_GUARD_ACK=1 prefix correctly does not bypass the block in degraded mode (test L99): since git clean -fdx still satisfies the git clean predicate, the guard still exits 2 regardless of the prefix — which is the intended behavior when jq is unavailable and the ACK can't be verified.
The inline comment at L63–64 is accurate and explains exactly why the substitution is needed.
Carry-over from prior review (not introduced or fixed in 486353b)
nojq_exit printf footgun (destructive-guard.test.sh L91) — constructs JSON with printf '{"tool_input":{"command":"%s"}}' "$1". Unsafe for test inputs containing " or %s-lookalikes. All current inputs are safe (no special characters), so this is a maintainability note only, not a correctness issue for this PR. Fix this →
Verdict: The HIGH-confidence finding from the previous security review is fully addressed. The fix is correct, minimal, and well-documented. All five changed files across the two commits are now clean. Ready to merge.
|
Claude finished @kyle-sexton's task in 2m 42s —— View job Security Review
FindingsRESOLVED — Prior IMPORTANT:
|
| Input JSON | After substitution | rm anchor match |
|---|---|---|
{"tool_input":{"command":"rm -rf build/"}} |
{ tool_input :{ command : rm -rf build/ }} |
rm ✓ |
{"tool_input":{"command":"CLEAN_GUARD_ACK=1 git clean -fdx"}} |
{ tool_input :{ command : CLEAN_GUARD_ACK=1 git clean -fdx}} |
git clean ✓ → BLOCKED |
All three original degraded-mode test cases (blocks rm -rf, benign passes, ack prefix does NOT bypass) are now correct.
The substitution is implemented as a bash parameter expansion — no subprocess, no injection vector. Safe.
SUGGESTION — CLAUDE_PLUGIN_ROOT path interpolated into an unescaped shell string (low risk, carry-over from prior review)
Confidence: LOW
command: "bash \"${CLAUDE_PLUGIN_ROOT}/skills/clean/scripts/destructive-guard.sh\""
shell: bashThe harness interpolates ${CLAUDE_PLUGIN_ROOT} before passing the string to bash -c. The resulting path is double-quoted inside the shell string, which handles spaces. If the plugin cache directory path contained a literal " character, the quoting would break and arbitrary shell words could follow. In practice, plugin cache paths are set by the Claude Code harness and are not user-supplied, so the exploitable precondition does not exist. Low-priority if the hooks reference confirms harness-managed paths never contain ".
Summary
No security issues with the core logic in its current (post-486353b) state. The rm anchor gap identified in the prior review is closed. The degraded mode now correctly: blocks rm -rf payloads, blocks ACK-prefixed destructive payloads, and passes benign commands — consistent with the PR's stated fail-closed objective. The only carry-over item is the low-confidence SUGGESTION about CLAUDE_PLUGIN_ROOT path handling, which requires a harness-managed precondition to be exploitable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 486353b26c
ℹ️ 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".
…mary (#1023) Reworks the `clean` skill's build/caches enumeration and apply flow as one coherent change, closing four coupled issues. ## What changed, per issue ### #993 — single pruned walk (perf) The selective `caches`/`build` tiers ran ~10 unpruned full-tree `find` walks per repo (7 build dir-names + 1 build glob + 1 cache dir-name + 1 cache glob); the `! -path` exclusions filtered output but did **not** `-prune`, so every walk re-descended `.git/`, `node_modules/`, and `.venv/`. A new shared `clean_enumerate` runs **one pruned walk per tier** that prunes those three trees once and `-print`s all dir-name and file-glob matches, then classifies protections on the result list. Field measurement (large .NET + node repo, Windows/NTFS): one pruned walk incl. `du` sizing **~16.7 s** vs a 10-walk unpruned dry-run that **exceeded 10 min** (killed, never finished). Verified locally that the prune excludes a `bin/` nested in `node_modules/` and an `obj/` nested in `.venv/`. ### #995 — dry-run manifest, apply consumes it, resume - `--dry-run` writes a session-scoped manifest (`<class>\t<bytes>\t<relpath>` per eligible target), prints `Manifest: <path>` and `Summary: planned=N bytes=K` so the confirmation gate can state reclaimable space. - `--apply --manifest <path>` consumes the manifest with a **re-stat + re-classify staleness guard** (a path that became protected since the dry-run is not removed) instead of re-walking. - **Resume** = re-run the identical `--apply --manifest <path>`; already-removed entries are idempotent no-ops. - `--apply` without a manifest builds one then applies it, preserving the standalone CLI contract. `--include-caches` folds the caches tier into the **one** build manifest (no `clean-caches.sh` subprocess). ### #1002 — machine-parseable apply summary, fail-closed Each `--apply` ends with `Summary: removed=N failed=M bytes=K` (bytes actually reclaimed) and exits non-zero when `failed>0`, so a fleet sweep no longer needs per-log grepping. Reuses the existing `Summary: k=v` + `[[ failed -eq 0 ]] || exit 1` convention (`git-branch-audit.sh`, `git-tree-reset-batch.sh`). ### #999 — drop the `dotnet clean` driver `clean-build.sh --apply` ran `dotnet clean <solution>` (full MSBuild evaluation, minutes on a large solution) before removing `bin/`/`obj/` wholesale anyway; the driver added **no** removal coverage and re-created `obj/` evaluation artifacts. Removed entirely — one walk + `rm` is strictly faster and equally complete — along with its `Planned: dotnet clean …` (dry-run) and `DRIVER_FAILED:` (apply) output markers and the now-stale driver references in the config/ecosystem/README docs. ## CLI contract changes (both `clean-build.sh` and `clean-caches.sh`) - **New flag `--manifest <path>`.** On `--dry-run` it writes the manifest to `<path>` instead of a `mktemp` default; on `--apply` it names the manifest to consume (no re-walk). - **New stdout lines on `--dry-run`:** `Manifest: <path>` and `Summary: planned=N bytes=K`. - **New stdout line on `--apply`:** `Summary: removed=N failed=M bytes=K`. - **New exit semantics:** `--apply` now exits `1` when any removal fails (was always `0`); `2` usage error and `1` not-a-git-repo are unchanged. - **Removed markers (`clean-build.sh` only):** `Planned: dotnet clean …` and `DRIVER_FAILED:` are gone. - **Unchanged:** `--dry-run` default, `--apply`, `--include-caches`, the protection classes (secrets / runtime deps / skill data preserved by default), and the `Planned remove:` / `Removed:` / `Skip (…)` line prefixes (the dry-run `Planned remove:` line now also carries a human size suffix). ## Design notes - **Cross-tier nested-target dedup**: any eligible path under an eligible ancestor is dropped before sizing/manifesting, so byte totals never double-count and apply never chases an already-removed path (e.g. a `*.tsbuildinfo` inside a `dist/`, or a nested `obj/` inside `bin/`). Anchored on `/` so `build` never swallows `buildstuff`. - **Portability**: `du -sk` (POSIX) for sizing; GNU-only `find -printf` intentionally avoided for BSD/macOS. Scripts stay `shell=bash`. - **Bytes** are block-approximate (du granularity), matching `scan.sh`'s `Total reclaimable`. ## Version `0.4.6` → `0.5.0` (minor: new `--manifest`/resume surface). Per the official plugin-manifest reference, `version` is an optional semver string and bumping it is how consumers receive the update: https://code.claude.com/docs/en/plugins-reference (§ plugin.json fields — `version`: "Semantic version. Setting this pins the plugin to that version string, so users only receive updates when you bump it."). ## Tests Extended `clean-caches.test.sh` and `clean-build.test.sh` TDD-style: manifest path + planned summary, exact `class<TAB>bytes<TAB>path` line format (the manifest is a consumed contract), apply summary + exit 0, resume (`removed=0 failed=0`, exit 0), nested-dedup single-count, `--include-caches` folding both classes into one manifest, and a chmod-guarded rm-failure case (`failed=1`, exit 1) that runs on Linux CI and skips on Cygwin where the FS ignores a write-denied parent. All existing repo-hygiene tests remain green; shellcheck, shfmt, markdownlint, editorconfig, changelog-parity, validate-plugins, and check-skill-portability all pass locally. ## Related - #994 — consumes the dry-run manifest this PR produces (built on top of this batch; not closed here). - #1011 — `scan.sh` shares the same unpruned-walk pattern; migrating it onto `clean_enumerate` is deferred to this follow-up (not closed here). `scan.sh` is outside #993's named targets. - #1006 — Batch A (destructive-guard fail-open fix), already merged; this branch was rebased onto `main` after it landed. Closes #993 Closes #995 Closes #1002 Closes #999 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…1465) Closes #1416 ## Summary - **Closes the observability half of #1416.** A repo-operator investigation (issue comments, 2026-07-25T23:46-23:47Z) found both originally-reported launch-refusal root causes already fixed and merged (disk-hygiene: #1242/0.9.0; repo-hygiene: #1006), and split the one remaining live defect (a silent post-launch death) to #1423, fixed separately by #1449. What #1416 kept as its own scope, per the operator's brief and its amendment: make a guard-launch/runtime failure loud, because "the guard denied nothing because it approved" and "the guard denied nothing because it never ran, or ran and died" were indistinguishable from outside the harness. - **New detector, `plugins/disk-hygiene/skills/clean/scripts/guard_launch_monitor.py`.** A second, independent hook — stdlib-only, imports nothing from `destructive_guard.py` or `lib/` — registered on `Stop` (not `PreToolUse`/`PostToolUse`) in `hooks/hooks.json`. Deliberately not per-tool-call: this repo already paid for that mistake once (`docs/adr/0004-rightsize-instruction-surfaces-by-incumbent-first-arbitration.md`'s D-12, a guardrails `PreToolUse` hook costing 12-19s p50 on every Bash call). Per the [hooks reference](https://code.claude.com/docs/en/hooks) (fetched 2026-07-25), `Stop` fires once per turn — the guard, if it ran, ran synchronously before the guarded command, so its failure record is already in the transcript well before the turn ends. The read itself is a bounded byte-seek tail (2MB cap) so per-turn cost never scales with session length, and a once-per-session marker (keyed by the hook's own `session_id` input, never a field found inside transcript records — those can differ from the file's own session, confirmed empirically against real local transcripts) short-circuits the read entirely after the first warning. - **Satisfies the amended criteria 2/3 exactly.** The emitted `systemMessage` states the most recent failure's `exitCode` and `durationMs` explicitly (labelled, not just embedded) alongside truncated stderr and the total failure count — verified both by the rendered-string test in `test_guard_launch_monitor.py` (fixture shaped like the real #1423 record: `exitCode: 1`, `durationMs: 17054`) and by a manual smoke test against a genuine local transcript record (see Test plan) that reproduces `exitCode: 1`, `durationMs: 11`, and the real config-refusal stderr text. - Never blocks, never emits `permissionDecision` or `decision: block`; on any transcript read/parse failure it exits 0 with no output. The once-per-session marker degrades toward *re-warning*, never toward silence, if its own bookkeeping write fails — over-warning is the safe direction for a module whose entire purpose is killing a silent-suppression defect class. - `plugin.json` 0.9.4 → 0.9.5, `CHANGELOG.md` entry, `README.md` and `skills/clean/reference/safety-model.md` both state what's covered (only `destructive_guard.py`'s own command string, current-session only) and what isn't (repo-hygiene's own guard — verified working separately; no retroactive scan of past sessions). ## Test plan - [x] `plugins/disk-hygiene/skills/clean/scripts/guard_launch_monitor.test.sh` — 17/17 pass: the #1423 shape (states `exitCode: 1`/`durationMs: 17054` in the rendered string), the launch-refusal shape, empty-stderr placeholder rendering, a clean session with a *different* hook's failure present (proves the command-substring filter discriminates), a fully clean session, malformed/unreadable transcript, missing `transcript_path`, malformed stdin, once-per-session suppression (same session id) vs independent warnings (different session ids), tail-bounded read still finds a failure near the end of an oversized transcript, marker-write failure still emits the warning this run, and a direct assertion that no `permissionDecision`/`decision: block` is ever emitted. - [x] Manual smoke test against a genuine local transcript (copied outside the repo, not committed): piped a real `hook_non_blocking_error` record for `destructive_guard.py` (`exitCode: 1`, `durationMs: 11`, the real "Plugin option \"disk_hygiene_enabled\" isn't set" stderr) through the finished detector — emitted `systemMessage` names all three correctly. - [x] `bash scripts/check-hook-userconfig-argv.sh` — pass (new hook's args carry no `${user_config.*}` token). - [x] `bash scripts/check-changelog-parity.sh --check-bump origin/main` — pass. - [x] `node scripts/validate-plugin-contracts.mjs` — pass (43 setup skills, 2101 plugin files). - [x] `claude plugin validate plugins/disk-hygiene/` — pass. - [x] `bash scripts/run-plugin-tests.sh` (full repo, 149 `*.test.sh` files) — run locally; time-boxed partway through (29/149 files, 0 failures) given this change's isolation to new disk-hygiene-only files plus the repo-wide structural gates above already passing across all 2101 plugin files. CI runs the same script to completion as the authoritative full-repo gate. ## Related Refs #1423 — the live launch/runtime-death fail-open this issue was found alongside, fixed separately by #1449 (open, unmerged as of this PR). Refs #1449 — open PR, unmerged, also touches `plugins/disk-hygiene/.claude-plugin/plugin.json`, `CHANGELOG.md`, and `hooks/hooks.json` for the #1423 fix. Both PRs edit the same three files; whoever merges second should expect a straightforward rebase (this PR adds a new `Stop` hooks.json key and a new CHANGELOG/version entry — no overlapping lines with #1449's `PreToolUse`-side edit, but git may still want a manual pass). 🤖 Generated with [Claude Code](https://claude.com/claude-code) *This was generated by AI during work-loop execution.* --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…e docs actually assert
Self-review of the previous commit found both new rows overreaching — each would have produced the
mirror image of the false positive being removed.
1. The path-placeholder row was titled "use exec form (`args`)" and rated a warning. Fixing a check
that wrongly warned on exec form by warning on shell form instead is not a fix. The hooks page
states a preference ("Prefer exec form for any hook that references a path placeholder") and in
the same breath gives the correct shell-form spelling ("In shell form, wrap each placeholder in
double quotes"), and endorses omitting `args` for pipes, `&&`, redirects, and `.cmd`/`.bat`
shims. Quoted shell form is therefore a documented, correct spelling — and the one this
repository's own `.claude/settings.json` hooks use, so the row as written would have warned on
this repo. The check now flags only the unquoted placeholder and reports exec form as a
preference rather than a finding.
2. The executable-resolution row was unscoped and rated an error, so it would have flagged
`"command": "bash"` on macOS and Linux, where `bash` is an ordinary executable and resolves fine.
The page scopes the constraint explicitly — "On Windows, exec form requires `command` to resolve
to a real executable such as a `.exe`" — so the row now applies to Windows-targeting repos only.
It also names `"shell": "bash"` as a fix alongside the `node`-plus-`args` pattern, since shell
form is what this repo actually shipped for #1006: the page states shell form runs via "Git Bash
on Windows", resolved by Claude Code rather than by a PATH lookup.
Same fetch as the previous commit: https://code.claude.com/docs/en/hooks. No version change — this
corrects rows added in the unreleased 0.22.0 entry, whose text is amended in place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and add the timeout-unit row (#2004) No linked issue Consumer report drained from the handoff inbox: `20260729-170500-claude-config-plugin-audit-skill-drift`. F1 and F2 were already fixed at HEAD; this closes the two residual findings. F3 was flagged CONTESTED by prior triage and needed adjudication against current docs, not implementation. ## F3 — UPHELD, but the reporter's reason was wrong The checklist's own rationale was that exec form "backslash-mangles `${CLAUDE_PROJECT_DIR}` on native Windows." Per the hooks page fetched this session (<https://code.claude.com/docs/en/hooks>), *Exec form and shell form*: > There is no shell, so each `args` element is one argument exactly as written, and path placeholders like `${CLAUDE_PLUGIN_ROOT}` are substituted into `command` and into each `args` element as plain strings. Special characters such as apostrophes, `$`, and backticks pass through verbatim because there is no shell to interpret them. **No shell tokenization happens on any platform.** Mangling is a shell artifact and exec form has no shell, so that mechanism is falsified. The page also contradicts the prescription directly: > Prefer exec form for any hook that references a path placeholder. In shell form, wrap each placeholder in double quotes. **But the observation behind the finding was misdiagnosed, not invented.** There is a real, documented Windows constraint, and it is narrower: "On Windows, exec form requires `command` to resolve to a real executable such as a `.exe`." This repo hit exactly that in #1006 — `"command": "bash"` resolved to the WSL relay `System32\bash.exe`, and because a failed hook launch is non-blocking, a destructive guard silently enforced nothing across an 82-repo session. So the fix is to replace the wrong mechanism with the right one rather than to delete the row: - a new row scoped to Windows-targeting repos (`bash`/`sh` are ordinary executables elsewhere), naming both documented remedies: a real binary with the script path in `args`, or shell form with `"shell": "bash"`; - a replacement quoting row that flags **only the unquoted placeholder**, never shell form itself. That second scoping is deliberate. Quoted shell form is a spelling the page endorses for pipes, `&&`, redirects, and `.cmd` shims, and it is what this repo's own `.claude/settings.json` hooks use — a check that condemned shell form outright would have replaced a false negative with the mirror-image false positive. The old mangling claim is **not** preserved as a caveat. Keeping the false mechanism next to the correct one would re-seed the error. ## F4 — added, unit confirmed from the same page > `timeout` | no | **Seconds before canceling.** Defaults: 600 for `command`, `http`, and `mcp_tool`; 30 for `prompt`; 60 for `agent`. New Category D row flags `timeout > 600` as near-certainly milliseconds, and names where the confusion documentably comes from: on that same page, the Bash/PowerShell `tool_input.timeout` is "Optional timeout in milliseconds" with example `120000` — which read as seconds is about 33 hours. A second row covers the bare `$CLAUDE_PROJECT_DIR` spelling in PowerShell shell-form hooks, which the page says PowerShell "resolves to `$null`". ## Method note carried from the report The doc was verified twice — WebFetch **and** the raw `.md` via curl with grep — because the inbox item's own method note records the WebFetch summarizer fabricating on long docs pages. Worth knowing for the next contract-surface change: the summarizer is a lossy read of a page whose exact wording is the thing being adjudicated. ## Verification | Check | Result | |---|---| | `markdownlint-cli2` (22 files) | 0 issues | | `skill-quality` `check-skill.sh audit` | PASS — 0 errors, 2 pre-existing warnings | | `check-changelog-parity.sh --check-bump origin/main` | pass | `check-orphaned-fixtures.sh` was not run locally; it exceeds a 300s timeout on this machine. CI covers it. `claude-config` 0.21.9 → **0.22.0** — minor, not patch: consumer-visible check behavior moves in both directions (one warning retired, four added). Three surfaces move together: `reference/audit-checklist.md`, `context/validation-categories.md`, and `SKILL.md`'s Category D summary. ## Out of scope, surfaced not fixed - No evals added for the new rows — `audit/evals/evals.json` has no hook-form coverage at all today, which is a larger gap than these two findings. - `skills/audit/SKILL.md` is 251 lines against skill-quality's 200-line soft target (pre-existing warning). - The `audit` skill has no Gotchas surface despite a documented failure history — this finding plus 0.21.9's inoperable `:*` check would both belong there. - The inbox item carries no YAML front matter, so it is invisible to the inbox's own status contract. Its triage note already flags this; still true. - The report's transferable method note (prefer raw `.md` + grep over the WebFetch summarizer for long docs pages) is not reflected in the audit skill's Phase 3 instructions. - `docs/PLUGIN-PHILOSOPHY.md` and `docs/conventions/hook-config-delivery/README.md` already state the exec-form rules correctly and were left alone. ## Related - #1006 — the destructive-guard incident that is the real, documented Windows failure this row now describes correctly. - Inbox item `20260729-170500-claude-config-plugin-audit-skill-drift` — the consumer report. F1 and F2 were already fixed at HEAD. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d destructive guard (#2570) ## Problem `disk-hygiene`'s `PreToolUse` destructive-operation guard **has not been running at all** on Windows hosts whose `PATH` resolves `bash` to the WSL relay. This is a dead safety guard, not log noise: `destructive_guard.py` never executes, so destructive `Bash`/`PowerShell` commands proceed ungated while the operator sees only `PreToolUse:Bash hook error` lines. Both registrations in `hooks/hooks.json` used **exec form**: ```json { "type": "command", "command": "bash", "args": ["${CLAUDE_PLUGIN_ROOT}/hooks/run-python-hook.sh", "..."], "timeout": 60 } ``` ### Reproduction ```console $ where.exe bash # from PowerShell, i.e. the real Windows PATH C:\Windows\System32\bash.exe C:\Users\<user>\AppData\Local\Microsoft\WindowsApps\bash.exe ``` Git Bash's directory is not on the Windows `PATH` at all, so the WSL relay wins. Invoking the launcher through it: ```console $ printf '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"rm -rf /"}}' \ | /c/Windows/System32/bash.exe plugins/disk-hygiene/hooks/run-python-hook.sh \ plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py --mode engine-gate <3>WSL (31 - Relay) ERROR: CreateProcessCommon:818: execvpe(/bin/bash) failed: No such file or directory EXIT=1 ``` The only installed WSL distro is `docker-desktop`, which has no `/bin/bash`. ### Why it is silent Per the [hooks reference](https://code.claude.com/docs/en/hooks): > **Exec form** runs when `args` is present. Claude Code resolves `command` as an executable on `PATH` and spawns it directly with `args` as the argument vector, with no shell involved. … No shell tokenization happens on any platform. > On Windows, exec form requires `command` to resolve to a real executable such as a `.exe`. And a hook that cannot start is **non-blocking**: > A hook that can't start lands in the same non-blocking bucket. … For most hook events, the action proceeds. When you set up a policy hook, watch for this notice on its first run: a mistyped path in `settings.json` leaves the gate silently disabled. So "guard never ran" and "guard approved" are indistinguishable from outside. ### This is a regression, and the repo already knew the rule `0.17.6` moved both registrations onto `"command": "bash"` + `args` to fix Python resolution (#1504) — reintroducing the exact launch failure #1006 had already fixed on the skill-frontmatter surface. #1416 was closed `COMPLETED` while the guard stayed dead; it is **reopened** by this PR. `plugins/claude-config/skills/audit/reference/audit-checklist.md` Category D already carries this as an `error` row naming this precise failure — and `disk-hygiene` shipped it anyway. A checklist a human reads is not a gate; see #2569. ## Fix Shell form, which Claude Code routes through **its own Git Bash** on Windows rather than a `PATH` lookup: > **Shell form** runs when `args` is absent. The `command` string is passed to a shell: `sh -c` on macOS and Linux, Git Bash on Windows, or PowerShell when Git Bash isn't installed. Set the `shell` field to choose explicitly. ```json { "type": "command", "command": "\"${CLAUDE_PLUGIN_ROOT}\"/hooks/run-python-hook.sh \"${CLAUDE_PLUGIN_ROOT}\"/skills/clean/scripts/destructive_guard.py --mode engine-gate --plugin-root \"${CLAUDE_PLUGIN_ROOT}\" --authorized-data-root \"${CLAUDE_PLUGIN_DATA}\"", "shell": "bash", "timeout": 60 } ``` The #1504 Python-resolution behaviour is untouched — only the launch mechanism moves. **Design notes** - **Every placeholder is double-quoted**, per the page's "In shell form, wrap each placeholder in double quotes." Verified for **both** hooks that the resulting argv is byte-identical to the exec-form vector, against a `CLAUDE_PLUGIN_ROOT` *and* a `CLAUDE_PLUGIN_DATA` containing spaces and backslashes. The Stop hook was checked explicitly: `guard_launch_monitor.py` is what emits the `systemMessage` when no interpreter resolves, so a mangled `--data-root` there would have broken the detector that makes a dead guard visible. - **`"shell": "bash"` is declared explicitly**, unlike sibling `guardrails` which omits it. Shell form otherwise falls back to PowerShell on a Windows host with no Git Bash detected, which cannot run a `.sh`. This is the spelling `claude-config`'s Category D row names as the fix, and the one #1006 used. It is ignored when `args` is set, so it is meaningful only in shell form. - **Exec form with a real binary was not viable here.** The launcher is a `.sh`, so it needs bash — and there is no portable spelling of Git Bash's `bash.exe` to put in `command`. That is circular, which is why the page's general "prefer exec form for path placeholders" preference (a quoting-safety point) yields to the Windows platform constraint. - No environment workaround. `CLAUDE_CODE_GIT_BASH_PATH` and `PATH` reordering are explicitly **not** the fix; shell form is resolved by Claude Code, so `PATH` order neither causes nor fixes this. ## Tests fixed — both encoded the bug as the contract This is why the defect survived two fix attempts: - `hooks/run-python-hook.test.sh` asserted `.command == "bash"` and read `.args[0]`. It now asserts the **portability property**: launcher named in `command`, `args` absent, `shell: bash`, and every `${CLAUDE_PLUGIN_*}` placeholder double-quoted. Verified it **fails** against the old exec-form JSON. - `skills/clean/scripts/test_hygiene.py` selected guard hooks with `if hook.get("args") and …`, which matches nothing once a hook moves to shell form — every assertion built on it would have gone **vacuously green**. Replaced with a form-agnostic `_hook_argv()` (`shlex.split` for shell form, `[command, *args]` for exec form) plus `_guard_argv_from_hook()`. ## Audit — full list of instances Audited **every tracked JSON file** repo-wide via a recursive walk for `type: command` + `args` — which covers the 18 `hooks/hooks.json` files, any manifest-pointed hook config, and **inline `hooks` objects declared in a `plugin.json`** — plus **every** SKILL.md and agent-definition YAML frontmatter `hooks:` block across all 77 plugins. | # | Location | Shape | Status | | --- | --- | --- | --- | | 1 | `plugins/disk-hygiene/hooks/hooks.json` — `PreToolUse` | exec form, `"command": "bash"` | **Fixed here** (proven dead) | | 2 | `plugins/disk-hygiene/hooks/hooks.json` — `Stop` | exec form, `"command": "bash"` | **Fixed here** (proven dead) | | 3 | `plugins/disk-hygiene/skills/clean/SKILL.md` frontmatter | exec form, `"command": "python3"` | **Deferred → #2568** (latent) | No other plugin in the repo has this defect — `disk-hygiene` is the only one using exec form at all. Every other hook, including all of `guardrails`, is already quoted shell form. **Why #3 is deferred rather than fixed here:** it is *latent*, not live — on the reporting host `python3` resolves to a real binary, so that guard currently works. It would fail only where `python3` is the zero-length `WindowsApps` alias stub. Converting a **currently-working** safety guard on a premise not verifiable in CI risks killing a live guard, which is the exact harm this PR fixes. #2568 carries the full analysis and the #1014 constraint (skill hooks receive only `${CLAUDE_PLUGIN_ROOT}`; `--authorized-data-root` must not be reintroduced). ## Verification - `run-python-hook.test.sh` — **13/13 PASS**; confirmed it fails against the pre-fix `hooks.json`. - Full `disk-hygiene` suite (5 files, 250 + 23 tests) — **no new failures**. Three telemetry-sink timing failures reproduce locally on Windows but are confirmed **pre-existing on unmodified `origin/main`** via a detached baseline worktree, and the `hygiene` lane is green on Linux CI. - `setup` shipped no `evals/evals.json`, which the skill-quality gate requires once its SKILL.md changes; six cases were added and pass both the schema and `check-evals-quality.sh`. - `shellcheck -x` (repo `.shellcheckrc`) clean; `typos`, `markdownlint-cli2`, `check-hook-userconfig-argv.sh`, `check-shell-portability.sh`, and `check-changelog-parity.sh` (`--check`, `--check-bump`, `--check-order`) all green. - Live corroboration: `guardrails`' quoted shell-form hooks fired and blocked a command during this session on the same machine — the pattern is demonstrably working here. - Version bumped `0.17.7` → `0.17.8` with a CHANGELOG entry framing it honestly as a regression. ## Docs corrected Both stated the now-disproved premise, and one was actively harmful: - `README.md` claimed the wired hooks "register as `bash` … so `bash` must resolve on `PATH`". - `skills/setup/SKILL.md` preflight item 1 told operators to **reorder their Windows `PATH`** so Git Bash precedes stub paths — a check that passes while the guard is dead, prescribing the workaround rather than the fix. It now verifies the registration shape and explicitly says `PATH` ordering is not the cause. - `hooks/run-python-hook.sh`'s header comment asserted the launcher "is registered as `bash` (available in Git Bash on Windows)" — the exact false assumption. Rewritten to document shell form and warn against reverting to exec form. Closes #1416 ## Related - #2568 — the remaining skill-scoped `python3` instance, split out of this audit - #2569 — proposed repo-wide CI gate for this defect class, modelled on `check-hook-userconfig-argv.sh` - #1504 — the 0.17.6 Python-resolution fix that introduced this regression - #1006 — the original shell-form fix for this defect class - #1014 — established the skill-hook substitution allowlist - #1346 — earlier report of the same silently-unenforced guard 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2572) ## Problem `plugins/disk-hygiene/skills/clean/SKILL.md`'s frontmatter belt was the **third and last** exec-form hook registration, deliberately left unconverted by #2570 (row 3 of that PR's audit table): ```yaml - type: command command: "python3" args: ["${CLAUDE_PLUGIN_ROOT}/skills/clean/scripts/destructive_guard.py", "--plugin-root", "${CLAUDE_PLUGIN_ROOT}"] timeout: 60 ``` Per the [hooks reference](https://code.claude.com/docs/en/hooks), exec form (`args` present) resolves `command` as an executable on `PATH` with no shell, and "On Windows, exec form requires `command` to resolve to a real executable such as a `.exe`." On stock Windows `python3` resolves to `%LOCALAPPDATA%\Microsoft\WindowsApps\python3.exe` — a **zero-length App Execution Alias stub**, not a real executable. The spawn fails, a failed hook launch is non-blocking, and the belt silently enforces nothing. This registration has named a bare interpreter since #215, predating `hooks/run-python-hook.sh` (#1504) entirely — the launcher that exists precisely to reject that stub. **This one is latent, not dead.** It works wherever `python3` is a real interpreter, which is the reporting host. So the risk here is the inverse of #2570's: breaking a *working* safety guard. Everything below is scoped to proving that cannot happen. ## Fix Shell form through the shared launcher — the shape #2570 established, no third variant: ```yaml - type: command command: '"${CLAUDE_PLUGIN_ROOT}"/hooks/run-python-hook.sh "${CLAUDE_PLUGIN_ROOT}"/skills/clean/scripts/destructive_guard.py --plugin-root "${CLAUDE_PLUGIN_ROOT}"' shell: bash timeout: 60 ``` **On the YAML spelling.** This is the *same value* `hooks.json` carries; only the file-level escape differs. JSON needs `\"` escapes, YAML expresses it as a single-quoted scalar wrapping literal double quotes. It is not a third shape — `_hook_argv()` tokenizes both identically. **Shell form is honored on the skill-frontmatter surface — verified, not assumed.** #1014 found this surface more restricted than documented, so this was checked rather than inferred: `repo-hygiene`'s `skills/clean/SKILL.md` has shipped a shell-form frontmatter hook with `shell: bash` since #1006 (`a83d1748`, titled "shell-form hook launch"), and still carries it on `main` today. A live, working, same-repo precedent on the exact surface. **The #1014 constraint is respected.** Verified in code, not from a summary: `destructive_guard.py:686-687` resolves `--plugin-root`, rejecting the literal unexpanded placeholder, and derives the data root from it via `_plugin_data_root_from_root`; `resolve_authorized_data_root`'s own docstring records that "a plugin `hooks.json` hook can [supply `${CLAUDE_PLUGIN_DATA}`]; a skill hook cannot." The new command string therefore substitutes **only** `${CLAUDE_PLUGIN_ROOT}` — no `${CLAUDE_PLUGIN_DATA}`, no `${user_config.*}`, and `--authorized-data-root` is **not** reintroduced. This is asserted by an existing test that was made form-agnostic rather than left form-specific. ## Argv equivalence — the load-bearing evidence **Claim, scoped precisely:** the argv **`destructive_guard.py` itself receives** is byte-identical before and after. `argv[0]` necessarily changes — from the interpreter name `python3` to the launcher path — because replacing interpreter resolution with the launcher *is* the fix. Verified two ways. First with Python's `shlex` (the tokenizer `test_hygiene.py` uses), then against **a real bash**, because a claim about how a shell tokenizes should be proven by a shell. The launcher path is swapped for an argv dumper so the vector it would receive is directly observable: ```console root = C:\Program Files\Claude Code\plug in root shell sees: "C:\Program Files\Claude Code\plug in root"/hooks/run-python-hook.sh "C:\Program Files\Claude Code\plug in root"/skills/clean/scripts/destructive_guard.py --plugin-root "C:\Program Files\Claude Code\plug in root" argv built by bash (launcher swapped for an argv dumper): C:\Program Files\Claude Code\plug in root/skills/clean/scripts/destructive_guard.py --plugin-root C:\Program Files\Claude Code\plug in root root = D:\a b\c\d argv built by bash (launcher swapped for an argv dumper): D:\a b\c\d/skills/clean/scripts/destructive_guard.py --plugin-root D:\a b\c\d ``` Exactly three tokens after the launcher, matching the exec-form `args` array element for element, for a POSIX root and for two Windows roots containing **both spaces and backslashes**. These are the same three roots the added test asserts against. **Why it holds, and why the quoting is required rather than stylistic:** inside POSIX double quotes a backslash is literal (it escapes only `$`, `` ` ``, `"`, `\`, newline), and whitespace does not split. Remove the double quotes and `C:\Program Files\Claude Code\plug in root` splits into four argv entries and the backslashes get eaten — the guard would receive a truncated `--plugin-root` and lose its data-root authority. That is exactly why #2570's "double-quote every placeholder" rule is a correctness requirement. ## Closest reproducible proxy for "the guard still blocks" The belt is skill-scoped, so it fires only inside `/disk-hygiene:clean` and cannot be exercised from an ordinary session — I am **not** claiming a live in-session verification of the belt itself. Instead, the exact new command string was substituted the way Claude Code substitutes it and driven end to end: ```console $ printf '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"rm -rf /tmp/example"}}' \ | bash -c '"<root>"/hooks/run-python-hook.sh "<root>"/skills/clean/scripts/destructive_guard.py --plugin-root "<root>"' {"hookSpecificOutput": {"hookEventName": "PreToolUse", "permissionDecision": "deny", "permissionDecisionReason": "Disk-hygiene fails closed: Bash is restricted to exact bundled scan, preview, handoff-verify, and apply invocations of ... hygiene.py ..."}} ``` The belt launches through the new string and emits a `deny`. Note for anyone re-running this: the belt is **deny-by-default** during active cleanup (it allows only the exact bundled engine invocations), so a benign payload such as `echo hi` denies too. Both denying is the documented behavior of this surface, not over-blocking introduced here. Composed with the argv equivalence above, the argument is stronger than a single live observation: the guard receives a byte-identical vector, the existing 252-test suite proves the guard's decisions *given* that vector, and `test_skill_hook_launcher_resolves_a_supported_interpreter` proves the launcher resolves and execs end to end. No behavior change is reachable — only the launch mechanism moved. ## What this does NOT fix — stated so it is not overclaimed `run-python-hook.sh` **exits 0 silently in guard mode** when no interpreter resolves anywhere on its ladder (its own test asserts this). So this closes *"the belt cannot start against the alias stub"*. It does **not** make the belt fail-closed on a host with no Python at all — that residual is unchanged, and is documented as such in `safety-model.md` and the README. ## Tests — the third bug-as-contract site, and its fix #2568 named one form-specific helper. There were **three** sites, all of which encoded the launch form they were supposed to be checking: | Site | Old shape | Post-conversion behavior | | --- | --- | --- | | `_skill_hook_command_and_args()` | `next(… "args:" in line)` | **raises `StopIteration`** | | same helper's `command` read | `.split(":",1)[1].strip().strip('"')` | silently mis-parses a single-quoted YAML scalar into a string `shlex` then tokenizes wrongly | | `test_skill_hook_interpreter_is_python3_and_resolves` | `assertEqual("python3", interpreter)` | asserts the defect | The helper is replaced by `_skill_hook()`, which parses the frontmatter into a **hook mapping** and feeds it to the *existing* form-agnostic `_hook_argv()` / `_guard_argv_from_hook()` added by #2570 — so both surfaces are now asserted through one reading path, in either form. A `_yaml_flow_scalar()` helper decodes the three one-line YAML scalar styles rather than hand-stripping quotes (the suite is stdlib-only; PyYAML is not a dependency). The interpreter test now **exercises the launcher's real resolution ladder** instead of restating it, so the two cannot drift. **Added — and proven discriminating (requirement 5):** - `test_skill_hook_registers_in_portable_shell_form` — the same four portability properties `run-python-hook.test.sh` asserts for `hooks.json` (launcher named in `command`, `args` absent, `shell: bash`, every `${CLAUDE_PLUGIN_*}` double-quoted). That suite is jq-based and cannot read YAML frontmatter, which is why this surface is asserted in `test_hygiene.py`. **Verified to FAIL against the pre-change frontmatter** (`git stash` of SKILL.md only): ``` FAIL: test_skill_hook_registers_in_portable_shell_form AssertionError: 'hooks/run-python-hook.sh' not found in 'python3' : skill hook must launch through the shared launcher: 'python3' ``` Note the substitution-allowlist test passes in **both** forms, so it is explicitly *not* the discriminator — its docstring now says so. - `test_skill_hook_argv_matches_the_exec_form_vector_it_replaced` — encodes the equivalence proof above over three roots (POSIX, and two Windows roots with spaces + backslashes). This one passes in both forms *by design*: it is the equivalence evidence, not the regression gate. ## Docs corrected (requirement 6) Every surface that described this hook's form: - `README.md` — "Claude Code launches the skill-scoped guard in shell-free **exec form**"; "the **wired** hooks resolve Python through `run-python-hook.sh`" (x2, now all three). - `skills/clean/reference/safety-model.md` — "(the clean skill's frontmatter hook, **still exec form**)"; "The skill-scoped belt is a separate surface and **still launches in exec form via `python3`** (#2568)" (sentence's purpose is gone); the "Hook launch form" section extended from two hooks to three; "since #1504 **both wired hooks** launch through the shared launcher". - `skills/setup/SKILL.md` — "the skill-scoped belt through the **literal name `python3`**"; preflight item 1's scope; item 2's "The `clean` guard hook runs the literal command `python3`". - `hooks/run-python-hook.sh` header — "Launch disk-hygiene **wired** hooks"; "**hooks.json** invokes this file in SHELL FORM". - `skills/setup/scripts/python3_alias_probe.py` docstring + operator message, and `test_python3_alias_probe.py` docstring — both grounded the check on "the guard is launched as `python3`". **Also changed — fallout of the conversion, caught in review (thread from `chatgpt-codex-connector`, P2):** `/disk-hygiene:setup check` step 2 verdicts the interpreter **ladder**, not `python3` alone. My first pass kept `store-alias-stub` → FAIL and merely re-grounded the rationale. That was wrong, and the review found the concrete counter-case: a host with real Python installed **without "Add to PATH" but with the `py` launcher** has a stubbed `python3`, a working `py -3`, and a guard that now launches on every call — and would have been reported broken and told to reinstall Python. Before this PR that FAIL was correct (the belt really did launch as bare `python3`); the conversion is what made it a false positive, so fixing it belongs here. Step 2 now treats the alias probe as **diagnostic input** — it still exists to classify the first rung *without executing it*, since a bare `python3 --version` pops the Store — resolves the ladder in the launcher's own order skipping stubs, and checks the selected interpreter against the parsed `MIN_PYTHON`. This is strictly more accurate in both directions, not a safety downgrade: - **FAIL** on an **exhausted** ladder or a below-floor interpreter — the real fail-open. Both still FAIL under a disabled toggle, for the reason already in the file. - **WARN** when the ladder resolves a supported interpreter but `python3` is the stub. Guards launch; the residual is only that a bare `python3` typed by hand still opens the Store. The probe's own return values are unchanged (27/27 setup tests still green) — only the verdict mapping and the message wording moved. **Not changed, deliberately:** - **`safety-model.md`'s "`${CLAUDE_PLUGIN_ROOT}` — the only substitution a skill-frontmatter hook receives"** — still true, still the #1014 constraint, and now load-bearing for this shape. - **`scripts/check-hook-userconfig-argv.sh`** — audited for the vacuous-green risk #2570 found. It scopes to hook *config JSON* only (`hooks/hooks.json`, manifest-pointed configs, inline manifest `hooks`), so SKILL.md frontmatter was never in scope and this change cannot make it silently pass. A repo-wide gate covering the frontmatter surface is what **#2569** proposes; not built here. ## Verification - **`test_hygiene.py` — 252 tests**, 1 failure. `check-changed-skills.sh` reports 2 script-test failures for `clean`. All three are the **pre-existing Windows telemetry-sink timing failures** #2570 baselined — confirmed identical on **unmodified `origin/main`** via a detached baseline worktree (`250 tests, same single failure` in `test_hygiene.py`; `23 tests, same 2 failures` in `guard_launch_monitor.test.sh`). Count moves 250 → 252 (two added, one renamed). The `hygiene` lane is green on Linux CI. - `hooks/run-python-hook.test.sh` — **13/13 PASS**. - `skills/setup/scripts` — 27/27 PASS. - `markdownlint-cli2` (5 files) — 0 errors. `typos` — clean. `shellcheck -x` — clean. `ruff check` — all checks passed (`ruff format` drift in these files is pre-existing on `main` and is not a CI gate). - `check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-order` — all green. - `check-hook-userconfig-argv.sh`, `check-shell-portability.sh`, `check-skill-portability.sh`, `check-silent-skips.sh`, `check-discriminating-test-skips.sh`, `check-manifest-duplicate-keys.py` — all green. - All **5** `disk-hygiene` test entry scripts run individually: `run-python-hook.test.sh`, `kill_switch_probe.test.sh`, `python3_alias_probe.test.sh` PASS; `guard_launch_monitor.test.sh` and `hygiene.test.sh` carry only the three baselined failures above. (I started `scripts/run-plugin-tests.sh` for the whole repo but it exceeded its window and was stopped, so I am **not** claiming a full-repo local run — CI covers that lane.) - `machine-specific-paths` initially failed on the first push: the argv fixtures used `C:\Users\<name>\…` and `/home/user/…` roots. A correct catch — the properties under test are spaces, backslashes, and a drive-letter shape, none of which need a home-directory spelling. Fixtures moved to `/opt/claude/plugins/disk-hygiene`, `C:\Program Files\Claude Code\plug in root`, and `D:\a b\c\d`, and the evidence above was re-run against those exact roots. - Version bumped `0.17.8` → `0.17.9` with a CHANGELOG entry. - No `lefthook` config exists in this repo (no `lefthook.yml` / `.lefthook.yml`), so there is no such gate to run here. Closes #2568 ## Related - #2570 — converted the two wired hooks; deliberately deferred this instance as row 3 of its audit - #1416 — the wired-hook instance of this defect class - #1014 — established the skill-hook substitution allowlist this change stays inside - #1006 — the original shell-form fix, and the live precedent for shell form on this surface - #1504 — introduced `run-python-hook.sh`, which this belt now finally routes through - #2569 — proposed repo-wide CI gate for this defect class 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
An exec-form hook (a hook object carrying `args`) resolves `command` as an executable through PATH, so a bare name is machine-dependent. On Windows `bash` resolves to the WSL relay bash.exe under System32 and `python3` to a zero-length WindowsApps App Execution Alias stub; the launch fails, and a failed hook launch is a non-blocking error, so a PreToolUse guard wired this way silently enforces nothing. The class has shipped three times in disk-hygiene alone: #1006 fixed it, #1504 reintroduced it while fixing Python resolution, #2570 fixed it again. The claude-config audit checklist carried it as an `error` row throughout — a checklist a human reads is not a gate. Add scripts/check-hook-exec-form.sh, modelled on the sibling check-hook-userconfig-argv.sh gate: same scope rules for hook config JSON (default hooks/hooks.json, manifest-pointed paths with the out-of-tree trust boundary, inline manifest hooks object), extended to the skill/agent YAML frontmatter `hooks:` blocks that gate does not cover — the surface where the remaining instance lives. `args` presence is the sole exec-form discriminator, so shell form with a leading bare `bash` (the #2570 fix) is never flagged. The rule is the broad one: no path separator in `command` fails, rather than a denylist of names already known to burn us — a {bash, sh} denylist would have waved `python3` straight through. `node` is the one allowlisted bare name, because no Windows shim or alias stub shadows it and both docs/PLUGIN-PHILOSOPHY.md and the audit checklist name it as THE Windows-correct exec-form spelling; the array is pinned by a test so growing it is a visible diff. Wire it as its own self-test-first CI job and into the ci-status needs graph, and point the philosophy doc's Hooks row at the mechanical check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
) ## Summary An exec-form hook — a hook object carrying `args` — resolves `command` as an **executable through PATH**, not as a shell command line. A bare name is therefore machine-dependent, and on Windows two spellings resolve to something that is not the interpreter the author meant: `bash`/`sh` hit the WSL relay `bash.exe` under `System32` (dying with `execvpe(/bin/bash) failed` when no distro provides `/bin/bash`), and `python`/`python3`/`py` hit the zero-length WindowsApps App Execution Alias stub. A failed hook launch is a **non-blocking** error, so a `PreToolUse` guard wired this way silently enforces nothing. This class has now shipped **three times in `disk-hygiene` alone**: #1006 fixed it, #1504 reintroduced it while fixing a different (Python resolution) bug, #2570 fixed it again — and #1416 was closed `COMPLETED` while the guard stayed dead through 73 recorded runs, every one a `hook_non_blocking_error`. `plugins/claude-config/skills/audit/reference/audit-checklist.md` Category D has carried this as an `error` row the whole time. A checklist a human reads is not a gate; nothing structural prevented reintroduction. This PR is that structure. ### What lands `scripts/check-hook-exec-form.sh` + `scripts/check-hook-exec-form.test.sh`, modelled directly on the sibling `check-hook-userconfig-argv.sh` gate — same `cd`-to-repo-root shape, same scope rules for hook config JSON, same out-of-tree manifest-path trust boundary with a visible skip, same self-test-first CI wiring, same failure-output style. **Coverage — both declaration surfaces**, because the defect has appeared in each: | Surface | Covered | |---|---| | `plugins/*/hooks/hooks.json` (default location) | yes | | Manifest-pointed hook config (`hooks` as a string, or an array of paths) | yes | | Inline manifest `hooks` object | yes | | Skill / agent YAML frontmatter `hooks:` blocks | **yes — new**; the userconfig-argv gate does not cover this, and it is where the third instance lived (#2568) | `args` presence is the **sole** exec-form discriminator. The command *string* is never searched for interpreter names, so the #2570 fix — shell form with a leading bare `bash` and `shell: bash`, exactly as `plugins/repo-hygiene/skills/clean/SKILL.md` now carries it — is not flagged. That is a named test case, not an accident. ### Two readers, one rule — and why the YAML one is a real parser The JSON surface is read with `jq`, which parses that format completely. The frontmatter surface is read by `scripts/check-hook-exec-form-frontmatter.py`, which hands the document to **PyYAML**. Both readers only *report* exec-form hooks; the shell gate owns the rule, so one implementation governs both surfaces. That split was not the starting point. The frontmatter surface began as a hand-rolled `awk` walk over block-style YAML, and review found **four ways past it in three rounds** — a quoted key `"hooks":`, two escape encodings of the same key (`"hooks":`, then `"\U00000068ooks":`), an alias under the key, and a brace inside an ordinary scalar misread as flow style. Two were fail-open, two were false positives. Each fix enlarged the parser and invited the next case, because *"which spellings does YAML permit"* has no natural end — and a gate that misparses is worse than no gate, since it either blocks valid frontmatter or waves through the very defect it exists to catch. This one had done both. PyYAML settles the class by construction: quoted and escaped keys arrive already decoded by the scanner, anchors and aliases and merge keys resolve, flow and block style are the same document. The fail-closed refusals those rounds forced went away with it — the reader now refuses only frontmatter that genuinely does not parse. The dependency is handled the way this repo already handles a pinned tool: `pyyaml` is hash-locked in `.github/requirements-ci.txt` beside every other Python pin, and resolved exactly as `scripts/run-ruff.sh` resolves ruff — a python that can already import it (the CI path), else `uv run --with pyyaml==<pin>` reading that same pin (the cross-platform local path, no global install and no virtualenv ceremony), else exit 1. A missing module never becomes a silent pass over 997 files. Scope narrowing that came with it: `hooks` is read only as a **top-level** frontmatter key, the one position Claude Code loads a skill or agent hook from. A `hooks:` mapping nested under another key is data, not a registration. Relationship to the sibling gate: complementary, non-overlapping. `check-hook-userconfig-argv.sh` constrains **whether** a `${user_config.*}` token may appear in a hook config; this gate constrains **what shape `command` takes** once a hook is in exec form. Neither subsumes the other. ### The rule, and why this one **Broad rule, not a denylist:** an exec-form `command` containing no path separator (`/` or `\`) fails. A denylist of known-problematic names can only ever hold the spellings someone was already burned by — and that is *precisely* this defect's history. A `{bash, sh}` denylist written after #1006 would have waved `python3` straight through, which is exactly what #2568 is. The failure is not "these particular names are bad"; it is "a bare `command` is a PATH lookup whose resolution is a property of the machine, and CI cannot see the machine". The broad rule names the actual mechanism. Nothing legitimate is rejected. The tree contains **zero** exec-form hooks in any `hooks.json` or manifest today, and the alternatives cost nothing: `${CLAUDE_PLUGIN_ROOT}`-rooted or absolute paths carry a separator and pass untouched, and shell form has no `args` at all and is never inspected. **One allowlisted bare name: `node`.** Not a judgement call about "real executables on every platform" — that unverifiable judgement is what failed three times. The admission criterion is mechanical: *a name qualifies only when no Windows shim, relay, or App Execution Alias stub shadows it on PATH ahead of the real interpreter.* `bash`/`sh` fail it (WSL relay). `python`/`python3`/`py` fail it (WindowsApps stubs). `node` passes — `node.exe` is the only resolution — which is why both `docs/PLUGIN-PHILOSOPHY.md` (Hooks row) and the claude-config audit checklist already name `"command": "node", "args": [...]` as **the** Windows-correct exec-form spelling. Allowlisting it keeps the gate agreeing with the repo's own documented guidance instead of contradicting it; a gate that forbids what the philosophy doc recommends does not get obeyed, it gets exempted. The array is a bash literal in the script (not a data file) and is **pinned by a test**, so growing it is a visible code+test diff, never a quiet one-word edit. ### Deviation from the issue's proposal — stated, not silent The issue proposed *"a documented escape-hatch allowlist file that can only shrink"* for **file paths**, mirroring `scripts/hook-userconfig-argv-allowlist.txt`. **This PR does not ship that file.** Reasoning: - The sibling gate has a path allowlist because a *sanctioned* use is foreseeable there — a ratified channel D (`required:true` + argv, no unset case) adoption. Here no legitimate case exists: shell form and a rooted path are always available at zero cost, so a path exemption could only ever grandfather debt. - A file-path hatch for *this* class is the artifact that would have kept `bash` alive across #1006 → #1504. For a defect whose signature is "the guard is silently off", the escape hatch and the bug are the same object. - The name allowlist above already carries the one real exception, with a mechanical admission criterion CI can restate. If a genuine case ever appears, adding the file is a small, reviewed change modelled on the sibling (including its stale-entry guard). I would rather add it against evidence than ship it empty. ### Also - New CI job `hook-exec-form-gate` (self-test first, then the gate), added to the `ci-status` `needs` graph — a gate that is not a required check is the #1416 failure mode again. - `docs/PLUGIN-PHILOSOPHY.md` Hooks row now names the failing spellings and points at the mechanical check, in the repo's own "prose states it but cannot self-verify" idiom. No behavioural doc change — `"command": "node"` remains correct, which is the payoff of allowlisting it. ## Test plan ### Red — the gate fails against the shapes that actually shipped A gate never demonstrated red is not a gate. `3a51996c` is the commit immediately before #2570 landed, so its tree carries **all three** historical instances of this defect: the two wired hooks in `hooks/hooks.json` (both `"command": "bash"` + `args`) and the skill-frontmatter one (`"command": "python3"` + `args`, which #2568 owned). Reconstruct them into a fixture tree and run the gate: ```bash t=$(mktemp -d) mkdir -p "$t/scripts" "$t/.github" "$t/plugins/disk-hygiene/hooks" "$t/plugins/disk-hygiene/skills/clean" cp scripts/check-hook-exec-form.sh scripts/check-hook-exec-form-frontmatter.py "$t/scripts/" cp .github/requirements-ci.txt "$t/.github/" git show 3a51996:plugins/disk-hygiene/hooks/hooks.json > "$t/plugins/disk-hygiene/hooks/hooks.json" git show 3a51996:plugins/disk-hygiene/skills/clean/SKILL.md > "$t/plugins/disk-hygiene/skills/clean/SKILL.md" (cd "$t" && bash scripts/check-hook-exec-form.sh; echo "exit=$?") ``` ```text EXEC-FORM HOOK: plugins/disk-hygiene/hooks/hooks.json:.hooks.PreToolUse[0].hooks[0]: exec-form hook (`args` present) with bare command "bash" EXEC-FORM HOOK: plugins/disk-hygiene/hooks/hooks.json:.hooks.Stop[0].hooks[0]: exec-form hook (`args` present) with bare command "bash" EXEC-FORM HOOK: plugins/disk-hygiene/skills/clean/SKILL.md:11: exec-form hook (`args` present) with bare command "python3" exit=1 ``` Every instance is named, on both surfaces, with a resolvable location. Note the JSON paths: the pre-#2570 `hooks` value is an **event-keyed object**, not an array, so a shape-specific walk would have found one entry and missed the other. ### Green — the current tree ```console $ bash scripts/check-hook-exec-form.sh No exec-form hooks with a bare command name. exit=0 ``` Clean across every JSON surface and all 997 markdown files under `plugins/`. This PR was **red by construction** until #2568 landed, on exactly one file — `plugins/disk-hygiene/skills/clean/SKILL.md:11` — and it deliberately never touched that file, grandfathered it in a baseline, or added a path allowlist to clear itself. #2568's fix (PR #2572, `be72131e`) removed the last instance; this branch is rebased on top of it. That ordering is the point rather than an inconvenience: this class's entire history is a guard being switched off with a plausible-looking justification attached, and a gate that exempts its own last violation to go green is that same move. The shape #2572 landed is pinned as a case here too, so the gate can never start rejecting the fix that unblocked it. So is the must-stay-green case from #2570: `plugins/repo-hygiene/skills/clean/SKILL.md` carries shell form with a leading bare `bash`, and the gate would be **wrong** to flag it — `args` presence is the sole exec-form discriminator, and the command *string* is never searched for interpreter names. ### Self-test `scripts/check-hook-exec-form.test.sh` — 57 cases, all passing locally and in CI. Fixture-tree pattern copied from the sibling gate's suite. Coverage: - the pre-#2570 shape fails and names **both** wired entries; the #2570 shell-form fix passes on both surfaces; the #2572 shape that unblocked this PR passes - rooted `${CLAUDE_PLUGIN_ROOT}` command passes; absolute Windows path passes - `node` passes; `bash`, `sh`, `python`, `python3`, `py`, `pwsh`, `deno` each fail; allowlist contents pinned so growing it is a visible code+test diff - `args: []` (present but empty) is still exec form - a `matcher` entry is not itself a hook object; one clean + one dirty sibling flags only the dirty one - manifest string-path, manifest array, inline manifest object; out-of-tree manifest path skipped visibly; unreferenced `hooks/*.json` not scanned; unparsable manifest does not crash the gate; an MCP `command`/`args` pair outside the `hooks` key is out of scope - **every YAML spelling of the key** is one declaration: `hooks:`, `"hooks":`, `'hooks':`, `hooks :`, `"hooks" :`, `"hooks":`, `"\U00000068ooks":`, `"\x68ooks":` — each was a live bypass of the walk this gate used to carry - **YAML the old walk refused or misread is now simply read**: flow-style mappings, a whole declaration on the key line, an alias under the key resolved to its anchor, an anchor in value position, a merge key expanded (with the explicit-key-wins precedence pinned separately), a block-scalar `command`, a trailing comment on the key, and braces inside an ordinary `args` scalar (that last one had been red-lining valid frontmatter) - **reported, never resolved by preference**: a duplicate top-level `hooks` key, a duplicate key inside the hooks declaration, or one arriving through a `<<` merge. PyYAML keeps the last value and js-yaml rejects the document, so what would actually run is ambiguous — and a gate premised on "do not assume how this resolves on the target machine" must not resolve it toward the reading that clears the file. Not theoretical: #1492 shipped a duplicate manifest key through a fully green suite. - **fail-closed** on the one thing a parser still cannot clear: unparsable YAML frontmatter, and unparsable hook config JSON - frontmatter hygiene: line-accurate reporting, block-sequence `args`, unquoted value with a trailing comment, CRLF, agent frontmatter, a `hooks:` block in the prose body or a fenced example is not a declaration, a `hooks:` mapping nested under another key is not a declaration, and nothing outside the `hooks` key is ever judged (a folded `description:`, an `&anchor`, and a `<<:` merge in ordinary frontmatter stay inert) - a clean tree passes with an explicit positive statement ### Repo gates run locally `shellcheck` (clean), `shfmt -d` (clean), `actionlint` (clean), `typos` (clean), `scripts/run-ruff.sh check` on the new Python (clean), `scripts/check-shell-portability.sh --paths` on both shell files (clean), `check-silent-skips.sh`, `check-discriminating-test-skips.sh`, `check-changelog-parity.sh --check`, `check-contract-slice-prune.sh --check`, `check-orphaned-fixtures.sh --check`, `check-cross-plugin-source-drift.sh --check`, `check-contract-clause-coverage.py`, `check-manifest-duplicate-keys.py`. All three new scripts committed mode `100755`. No `--no-verify`. ### Review Four rounds from `chatgpt-codex-connector`, seven findings, all addressed and resolved. Six were code changes; one — "fix the existing violation or defer requiring the gate" — was answered rather than applied, because deferring the `ci-status.needs` wiring reproduces the #1416 shape this PR exists to end. Rounds 2 and 3 are what motivated replacing the hand-rolled YAML walk with a real parser (three of those findings were bypasses of a parser that was chasing YAML's spelling rules); round 4 found the duplicate-key ambiguity above. That reasoning is in the resolved threads and in the reader's own doc-block. ## Related - Closes #2569 - #2568 (PR #2572) — was the one remaining violation this gate reported; landed as `be72131e`, and this branch is rebased on top of it - #1416 — the wired-hook instance the gate would have caught, closed `COMPLETED` while the guard was dead - #1006 — the original fix for this class - #1504 — the reintroduction - #2570 — the fix this gate pins --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Problem
The clean skill's session-scoped destructive guard (SKILL.md frontmatter PreToolUse hook) never launched on Windows. The hook used exec form (
command: "bash"+args), and per the hooks reference, exec form resolvescommandas an executable on PATH — on Windows that finds the WSL relay (System32\bash.exe), which fails withexecvpe(/bin/bash) failed. A failed hook launch is a non-blocking error, so the guard silently enforced nothing: 48 hook errors in one 82-repo fleet session (fee18cc2), with bulkrm -rfapplies running ungated.Fix
shell: bash. Per the hooks reference, shell form runs the command string via Git Bash on Windows, resolved by Claude Code itself instead of PATH lookup. The guard now launches wherever the skill itself can run (the skill already requires Git Bash viaallowed-tools: Bash(bash …)andshell: bashprecompute).CLEAN_GUARD_ACKacknowledgement is unverifiable, so the guard now matches its destructive patterns against the raw hook payload and blocks outright, telling the agent to install jq. Benign commands still pass. Same fail-open class as disk-hygiene: guard hook fails to launch on skill-frontmatter hooks referencing ${CLAUDE_PLUGIN_DATA} — live fail-open on 0.4.6 #983 / the convention: false-green checks — surfaces report success while the capability is dead or findings invisible (liveness-assertion umbrella) #532 umbrella.Platform note: a hook that fails to launch can never block (documented, non-blocking by design), so true fail-closed at the harness layer is impossible — the achievable posture is making launch succeed in every environment the skill runs in, plus fail-closed behavior for every failure mode inside the script's control. Cross-linked in #983, which hits the same class via a different vector.
Tests
destructive-guard.test.sh: PATH sandbox without jq — destructive payloads block (exit 2), benign pass, ack prefix does NOT bypass, block reason names degraded mode. Sandbox self-verifies and skips on platforms where symlinked bash can't run (Windows/MSYS); executes on Linux CI.skill-quality:checkPASS (0 errors).Closes #992
🤖 Generated with Claude Code
https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
Related