feat(ci): gate exec-form hooks against bare command names (#2569)

[kyle-sexton]

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 fix(repo-hygiene): destructive-guard fail-open on Windows — shell-form hook launch + jq fail-closed degraded mode #1006 → fix(disk-hygiene): guard-launch detector shares the guard's interpreter, so it is blind to the interpreter fail-open #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 disk-hygiene: the destructive-operation guard has never launched — 73 recorded runs, all hook_non_blocking_error #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:

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 3a51996c:plugins/disk-hygiene/hooks/hooks.json > "$t/plugins/disk-hygiene/hooks/hooks.json"
git show 3a51996c: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=$?")

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

$ 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-fix(disk-hygiene): launch wired hooks in shell form, reviving the dead destructive guard #2570 shape fails and names both wired entries; the fix(disk-hygiene): launch wired hooks in shell form, reviving the dead destructive guard #2570 shell-form fix passes on both surfaces; the fix(disk-hygiene): launch the skill-scoped guard in shell form (#2568) #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: skill-quality: plugin.json ships two version members (0.11.0 and 0.12.0) — advertised release is parser-dependent #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 CI: add a repo-wide gate rejecting exec-form hooks whose command is a bare non-portable executable name #2569
• disk-hygiene: skill-scoped guard hook still launches via bare python3 in exec form (latent Windows fail-open) #2568 (PR fix(disk-hygiene): launch the skill-scoped guard in shell form (#2568) #2572) — was the one remaining violation this gate reported; landed as be72131e, and this branch is rebased on top of it
• disk-hygiene: the destructive-operation guard has never launched — 73 recorded runs, all hook_non_blocking_error #1416 — the wired-hook instance the gate would have caught, closed COMPLETED while the guard was dead
• fix(repo-hygiene): destructive-guard fail-open on Windows — shell-form hook launch + jq fail-closed degraded mode #1006 — the original fix for this class
• fix(disk-hygiene): guard-launch detector shares the guard's interpreter, so it is blind to the interpreter fail-open #1504 — the reintroduction
• fix(disk-hygiene): launch wired hooks in shell form, reviving the dead destructive guard #2570 — the fix this gate pins

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[cursor]

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 encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c12d4367b

ℹ️ 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".

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[kyle-sexton]

@codex review

Both threads are addressed and resolved. The quoted-YAML-key finding was a genuine bypass and is fixed in 6b6a146 (prefilter + walk now accept every spelling of the key, three cases pinning it, suite 39/39 green in CI). The ci-status.needs finding is answered in-thread: red is by construction until #2568 / PR #2572 lands, this PR carries do-not-merge, and deferring the wiring would reproduce the #1416 shape the gate exists to end.

Please re-review the parser changes in particular — is_hooks_key / is_hooks_key_inline and the grep prefilter they must stay in sync with.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b6a146a5d

ℹ️ 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".

[kyle-sexton]

Ordering cross-check — the blocking PR is now open as #2572, and its shape clears this gate.

Ran this PR's gate against #2572's head version of plugins/disk-hygiene/skills/clean/SKILL.md (plus repo-hygiene's, in the same fixture tree):

$ bash scripts/check-hook-exec-form.sh
No exec-form hooks with a bare command name.
exit=0

So the sequence is settled and verified rather than assumed: #2572 merges, then this rebases onto main, hook-exec-form-gate and ci-status go green, and do-not-merge comes off. Posted the same evidence on #2572 so both sides hold the same ordering.

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[kyle-sexton]

@codex review

Round 2 addressed in 7e85ee5, both threads resolved. Both findings were symptoms of one mistake — enumerating YAML spellings — so the fix inverts the posture rather than adding patterns:

• No prefilter. All 997 plugins/**/*.md go to the walk in one batched awk pass; double-quoted keys and scalars are escape-decoded (\uXXXX, \xXX, \, \") before comparison.
• Fail closed inside a hooks block on anything the walk cannot model: alias, anchor, merge key, flow mapping, block scalar, or any unclassifiable line — each with a named reason.
• Narrowed to a top-level hooks key, the only position Claude Code reads one from, so a nested hooks: mapping is not mistaken for a declaration now that every file is walked.

48/48. Please look hardest at the inverse risk this introduces: false positives. Outside a hooks block nothing is judged, and args: as a block sequence of scalars must stay walkable — both are pinned, and the full 997-file corpus reports exactly one finding (the #2568 instance).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e85ee5b2c

ℹ️ 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".

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[kyle-sexton]

@codex review

Round 4 addressed in c84c3ba. The duplicate-key finding was real and reachable — hooks: {} followed by a second hooks: carrying an exec-form bare bash cleared the gate — and is now reported rather than resolved by preference, since PyYAML keeps the last value while js-yaml rejects the document and Claude Code reads this frontmatter with a JS parser. Scoped to the hooks subtree; merge-key precedence unaffected. 56/56, green on the real tree.

This should be the last round unless something reachable remains. If what turns up is a YAML shape no plugin author in this repo would write and that the fail-closed and duplicate-key rules already cover generically, I would rather answer it in-thread than keep enlarging the surface — but a bypass or a false positive I will fix.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: c84c3ba854

ℹ️ 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".

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[kyle-sexton]

Self-caught follow-up to the duplicate-key fix, landed in c05b669 before it could become a round-5 finding.

The check covered keys written at the hook object and at the document root, which left << as the way in: an anchored mapping carrying two command keys contributed both to a hook object unexamined. A merged mapping's keys are the hook object's keys, so they carry the same ambiguity and now get the same refusal — followed transitively, with the same cycle guard the entry walk uses. One case pins it; 57/57, green on the real tree.

[claude]

Claude finished @kyle-sexton's task in 37s —— View job

---

Security review skipped

• Gather context (PR body, comments, changed files)
• Run /review:security-review per custom instructions
• Post findings

/review:security-review's skip gate stops early when "PR is closed or not open." PR #2571 is merged (confirmed via the provided context and the local checkout, whose main history is already several commits past this PR's head c05b669d), so per the skill's instructions I'm stopping here and posting nothing else.

[github-actions]

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

[claude]

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

---

/review:code-review — skipped

• Gather context (PR state, diff, CLAUDE.md)
• Apply skip gate checks
• Determine outcome

Skip gate hit: PR is closed / not open. Per this repo's plugins/review/skills/code-review/SKILL.md skip gate, condition 1 ("PR is closed or not open") applies — PR #2571 is already merged. Per the lane's instructions, deep review is skipped and nothing else is posted.

If a fresh review of a later change is needed, please reopen the PR or open a new one against main.

[github-actions]

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