Fix hook wiring so permission-approval hooks actually run - #105
Draft
dirkraft wants to merge 1 commit into
Draft
Conversation
Three of the four permission-approval hooks were registered incorrectly, so `asta` and `bd` commands were never auto-approved and users were prompted for them every time. Two independent regressions: - #80 (plugin reorganization) created plugins/asta-flows/hooks/hooks.json pointing at approve-asta-bash.sh, but only copied approve-bd-bash.sh into that plugin. The referenced script does not exist there, so the bd auto-approval never fired. - #81 (Fix pdf-download location) dropped the entire `Bash` matcher block from plugins/asta-tools/hooks/hooks.json as unrelated collateral, orphaning approve-asta-bash.sh. Fixes: - asta-flows/hooks.json now references approve-bd-bash.sh. - asta-tools/hooks.json re-registers approve-asta-bash.sh under a `Bash` matcher. - Remove plugins/asta-tools/hooks/approve-bd-bash.sh, a byte-identical leftover of the #80 split. `bd` is only used by asta-flows skills, and tests already treat the asta-flows copy as canonical. Prevention: - New test_hooks_json_commands_resolve walks every plugins/*/hooks/hooks.json, asserts each ${CLAUDE_PLUGIN_ROOT}-relative command resolves to an existing executable script, and asserts no *.sh in a hooks dir is left unregistered. Verified to fail on each of the two bugs above independently. - CI shellcheck scandir now includes ./plugins/asta-flows/hooks, which was unlinted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Noticed this while trying to figure out what the "hooks" mentioned in the README's install snippet actually are — the term appears exactly once and isn't explained anywhere. Tracing it into
plugins/*/hooks/turned up wiring bugs, so opening as a draft in case it's useful.The problem
Three of the four permission-approval hooks were registered incorrectly, so
astaandbdcommands were never auto-approved — users got prompted for them every session, which is exactly what these hooks exist to prevent.Two independent regressions:
plugins/asta-flows/hooks/hooks.jsonreferencingapprove-asta-bash.sh, but only copiedapprove-bd-bash.shinto that plugin. The referenced script doesn't exist there, so bd auto-approval never fired.Bashmatcher block fromplugins/asta-tools/hooks/hooks.jsonas unrelated collateral (13 -------------in a PDF-path commit), orphaningapprove-asta-bash.sh.Before #80,
asta-preview/hooks/hooks.jsoncorrectly wired all three scripts, so this is a regression rather than something that never worked.Fixes
asta-flows/hooks.jsonnow referencesapprove-bd-bash.sh.asta-tools/hooks.jsonre-registersapprove-asta-bash.shunder aBashmatcher.plugins/asta-tools/hooks/approve-bd-bash.sh— a byte-identical leftover of the Plugin reorganization #80 split.bdis only used by asta-flows skills, andtests/test_hooks.pyalready treats the asta-flows copy as canonical. Flagging this one since it's a deletion — happy to keep it if you'd rather asta-tools also approvebd.Prevention
tests/test_hooks.pyonly checked that scripts exist and behave correctly — it never checked thathooks.jsoncommands resolve to real files, which is how both bugs shipped green.New
test_hooks_json_commands_resolvewalks everyplugins/*/hooks/hooks.jsonand asserts:${CLAUDE_PLUGIN_ROOT}-relative command resolves to an existing, executable script;*.shin a hooks dir is left unregistered (catches the orphan case).Verified it fails on each of the two bugs independently:
Also: CI's shellcheck
scandircovered./plugins/asta-tools/hooksbut not./plugins/asta-flows/hooks, so the latter was unlinted. Added it.Testing
191 passed(full suite),make lintandmake format-checkclean. shellcheck isn't installed locally, but the newly-scannedasta-flows/approve-bd-bash.shis byte-identical to the asta-tools copy that was already passing CI.Not addressed
Two doc gaps I noticed but left alone, since fixing them means writing docs rather than code — happy to follow up:
README.md:37is the only mention of "hooks" in the README, with no explanation of what they do.DEVELOPER.md:114links todocs/plugins.mdfor "Authoring skills and hooks", but there's nodocs/directory in the repo.🤖 Generated with Claude Code