fix(skill-required): address all audit findings#400
fix(skill-required): address all audit findings#400jack-nsheaps[bot] wants to merge 3 commits intomainfrom
Conversation
- Extract shared lib: add lib/utils.sh with get_project_slug() and sanitize_skill_name() helpers; both hook scripts now source it - Fix colon sanitization: skill names like scm-utils:commit now map to scm-utils--commit on the filesystem, preventing invalid path chars - Batch yq calls: check-skill-required.sh reads all skill rules in one yq call (yq -o=json) then uses jq for per-rule field extraction, reducing subprocesses from 4x per rule to 1 per loop iteration - Add mise.toml declaring yq as a required dependency - Add skill-required.settings.yaml template with documented defaults and example rules using YAML arrays - Convert required_before and command_pattern to YAML arrays in docs and settings template; legacy pipe-separated strings still supported for backwards compatibility - Update README to reflect new config format and mise.toml dependency
Plugin Version StatusVersions are auto-bumped in PRs. Manual bumps to higher versions are preserved.
|
nsheaps
added
the
request-review
henry-nsheaps
Bot
removed
the
request-review
![henry-nsheaps[bot]](https://avatars.githubusercontent.com/in/2371206?s=60&v=4){kind=small}
There was a problem hiding this comment.
💬 Solid audit cleanup — a few maintainability nits worth tightening before merge
⚠️ Array-vs-legacy-string parsing is duplicated ~40 lines across required_before and command_pattern — extract a helper
⚠️ jq is used everywhere but not declared in the new mise.toml (only yq is)
⚠️ PR body claims the settings template "matches" common-sense / agentic-behavior but the override file path diverges from their standard
ℹ️ Dead null/empty fallbacks after jq '// …' substitutions
ℹ️ sanitize_skill_name only covers : — either narrow the name or broaden the implementation
✅ Good: batched yq + jq reads, atomic cache writes, CLAUDE_PLUGIN_ROOT fallback, backwards-compat with legacy pipe strings, helper extraction into lib/utils.sh
🖱️ Click to expand for full details
required_before and command_pattern — extract a helperjq is used everywhere but not declared in the new mise.toml (only yq is)common-sense / agentic-behavior but the override file path diverges from their standardℹ️ Dead
null/empty fallbacks after jq '// …' substitutionsℹ️
sanitize_skill_name only covers : — either narrow the name or broaden the implementation✅ Good: batched
yq + jq reads, atomic cache writes, CLAUDE_PLUGIN_ROOT fallback, backwards-compat with legacy pipe strings, helper extraction into lib/utils.shCode Quality (78%)
The refactor meaningfully improves the plugin — get_project_slug() / sanitize_skill_name() are now a single source of truth across both hooks, the yq -o=json → jq batch reduces subprocess count per rule from ~4 to roughly 1, and the config template matches the shape of other plugins in the marketplace.
Two things dock the score:
- DRY violation in
check-skill-required.sh: the "is it a JSON array, or a legacy pipe-separated string?" branching is written twice, once forrequired_beforeand once forcommand_pattern. That's the largest block of duplicated logic in the file and the most likely place for future drift. Aread_list_field()helper inlib/utils.shcollapses both call sites. Suggested implementation in the inline thread. - Dead fallback checks after
jq '// 10'/jq '// empty'. Harmless, but they're remnants of the pre-jqpath — worth cleaning up while you're here.
Simplicity (72%)
check-skill-required.sh grew from ~140 to ~200 lines. Most of the growth is the dual-path handling; pulling the helper out would bring it back closer to the original footprint while keeping the new YAML-array support.
Dependencies
The new mise.toml declares yq but not jq. Both are hard deps (scripts will crash without jq — there's no command -v jq guard, unlike yq). README lists both under Dependencies. For consistency and to make mise install actually set up the environment, jq should be declared too.
Config Pattern Alignment
The PR body advertises alignment with common-sense and agentic-behavior, but those plugins use the standard plugins.settings.yaml + shared plugin-config-read.sh resolution, whereas skill-required ships its own settings.skill-required.yaml path and parses inline. The template file accurately reflects what the scripts actually read — but the "matches the pattern" claim is only half true. Either adjust the PR description or treat the full migration as a follow-up (see inline thread).
Security (N/A)
No new attack surface. Regex patterns are applied via grep -qE (not eval), cache writes are atomic, and skill/tool names come from Claude Code's hook payload. sanitize_skill_name only handles : today but skill names are plugin-controlled.
Versioning
0.1.4 → 0.1.5 (auto-bumped). All changes are backwards compatible (legacy pipe-strings still parse), so patch is appropriate.
Test Plan
The PR body's test plan checklist is entirely unchecked. No visible evidence of manual verification for colon sanitization, YAML-array round-trip, or legacy-format backwards compat. Worth confirming those were run locally at least once before merge, given there aren't automated tests for the hook scripts.
How the scores were derived
- Quality 78%: strong refactor core, offset by duplication + dead code + dep gap.
- Security N/A: no security-relevant changes; config is trusted user input.
- Simplicity 72%: dual-path logic roughly doubles the size of the rule-matching loop vs. what a shared helper would achieve.
- Confidence 90%: shell code is straightforward, no framework indirection, I read both scripts end-to-end and cross-referenced with the reference plugins.
Recommended follow-ups (non-blocking):
- Migrate
skill-requiredontoshared/lib/plugin-config-read.shand the standardplugins.settings.yamloverride path, to genuinely matchcommon-sense/agentic-behavior. - Add hook-script unit tests (even a simple
batssuite) so the dual YAML-array/legacy-string paths don't silently regress. - Decide whether
sanitize_skill_nameshould become a general path-safe sanitizer or be renamed to its actual (colon-only) scope.
Footnotes
-
Workflow Run: https://github.com/nsheaps/ai-mktpl/actions/runs/24538394948/attempts/1 ↩
-
Reference plugin config pattern: common-sense.settings.yaml, agentic-behavior.settings.yaml ↩
-
Shared libs rule: .claude/rules/shared-libs.md ↩
| # Check required_before — supports both YAML array and legacy pipe-separated string | ||
| # Build a newline-separated list of tool names | ||
| required_before_json="$(echo "$rule_json" | jq -c '.required_before // empty')" | ||
| if [ -z "$required_before_json" ] || [ "$required_before_json" = "null" ]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Determine if value is a JSON array or a plain string | ||
| tool_matches=false | ||
| IFS='|' read -ra tool_list <<< "$required_before" | ||
| for t in "${tool_list[@]}"; do | ||
| t="$(echo "$t" | xargs)" # trim whitespace | ||
| if [ "$t" = "$tool_name" ]; then | ||
| tool_matches=true | ||
| break | ||
| fi | ||
| done | ||
| if echo "$required_before_json" | jq -e 'type == "array"' &>/dev/null; then | ||
| # YAML array: ["Bash", "Edit"] | ||
| while IFS= read -r t; do | ||
| if [ "$t" = "$tool_name" ]; then | ||
| tool_matches=true | ||
| break | ||
| fi | ||
| done < <(echo "$required_before_json" | jq -r '.[]') | ||
| else | ||
| # Legacy pipe-separated string: "Bash|Edit" | ||
| legacy_str="$(echo "$required_before_json" | jq -r '.')" | ||
| IFS='|' read -ra tool_list <<< "$legacy_str" | ||
| for t in "${tool_list[@]}"; do | ||
| t="$(echo "$t" | xargs)" # trim whitespace | ||
| if [ "$t" = "$tool_name" ]; then | ||
| tool_matches=true | ||
| break | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| if [ "$tool_matches" != "true" ]; then | ||
| continue | ||
| fi | ||
|
|
||
| # If there's a command_pattern, check it (only for Bash tool) | ||
| if [ -n "$command_pattern" ] && [ "$command_pattern" != "null" ] && [ "$tool_name" = "Bash" ]; then | ||
| command_pattern_json="$(echo "$rule_json" | jq -c '.command_pattern // empty')" | ||
| if [ -n "$command_pattern_json" ] && [ "$command_pattern_json" != "null" ] && [ "$tool_name" = "Bash" ]; then | ||
| command="$(echo "$input" | jq -r '.tool_input.command // empty' 2>/dev/null || true)" | ||
| if [ -n "$command" ]; then | ||
| pattern_matches=false | ||
| IFS='|' read -ra patterns <<< "$command_pattern" | ||
| for p in "${patterns[@]}"; do | ||
| p="$(echo "$p" | xargs)" | ||
| if echo "$command" | grep -qE "$p"; then | ||
| pattern_matches=true | ||
| break | ||
| fi | ||
| done | ||
| if echo "$command_pattern_json" | jq -e 'type == "array"' &>/dev/null; then | ||
| # YAML array: ["git commit", "git push"] | ||
| while IFS= read -r p; do | ||
| if echo "$command" | grep -qE "$p"; then | ||
| pattern_matches=true | ||
| break | ||
| fi | ||
| done < <(echo "$command_pattern_json" | jq -r '.[]') | ||
| else | ||
| # Legacy pipe-separated string: "git commit|git push" | ||
| legacy_pat="$(echo "$command_pattern_json" | jq -r '.')" | ||
| IFS='|' read -ra patterns <<< "$legacy_pat" | ||
| for p in "${patterns[@]}"; do | ||
| p="$(echo "$p" | xargs)" | ||
| if echo "$command" | grep -qE "$p"; then | ||
| pattern_matches=true | ||
| break | ||
| fi | ||
| done | ||
| fi | ||
| if [ "$pattern_matches" != "true" ]; then | ||
| continue | ||
| fi | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The required_before block (lines 121-149) and the command_pattern block (lines 156-184) implement essentially the same logic: check if the JSON is an array → iterate items; else treat as pipe-separated string → split + trim + iterate. About 40 lines of near-duplicate shell.
Consider extracting a helper into lib/utils.sh, e.g.:
# Emits one item per line from either a JSON array or a legacy pipe-separated string.
# Args: $1=json_value (compact JSON, e.g. '["a","b"]' or '"a|b"')
read_list_field() {
local val="$1"
if echo "$val" | jq -e 'type == "array"' &>/dev/null; then
echo "$val" | jq -r '.[]'
else
# shellcheck disable=SC2046
IFS='|' read -ra items <<< "$(echo "$val" | jq -r '.')"
for item in "${items[@]}"; do
echo "$item" | xargs
done
fi
}Then each call site collapses to a single loop. Halves the size of this script and keeps the two code paths in lockstep — right now a future change to one branch can silently drift from the other.
Non-blocking, but worth doing since the audit was explicitly about maintainability findings.
| [tools] | ||
| # yq (Go version: mikefarah/yq) is required for YAML config parsing in hook scripts. | ||
| # https://github.com/mikefarah/yq | ||
| yq = "latest" |
There was a problem hiding this comment.
jq is a hard dependency too but isn't declared here
Both hook scripts call jq extensively (JSON parsing, atomic cache writes, etc.), and the README lists jq alongside yq under Dependencies. If the point of mise.toml is "declare tool dependencies so mise install sets them up", jq belongs here as well:
| yq = "latest" | |
| [tools] | |
| # jq is required for JSON processing in hook scripts. | |
| # https://github.com/jqlang/jq | |
| jq = "latest" | |
| # yq (Go version: mikefarah/yq) is required for YAML config parsing in hook scripts. | |
| # https://github.com/mikefarah/yq | |
| yq = "latest" |
Otherwise a fresh environment that goes through mise install from this plugin's mise.toml still won't have jq guaranteed, and the scripts will crash with jq: command not found (there's no command -v jq guard like there is for yq).
| skill_name="$(echo "$rule_json" | jq -r '.name // empty')" | ||
| max_uses="$(echo "$rule_json" | jq -r '.max_tool_uses_before_reset // 10')" | ||
|
|
||
| # Validate max_uses is a number | ||
| if [ -z "$max_uses" ] || [ "$max_uses" = "null" ]; then | ||
| max_uses=10 | ||
| fi | ||
|
|
||
| # Check if this rule applies to the current tool | ||
| if [ -z "$required_before" ] || [ "$required_before" = "null" ]; then | ||
| if [ -z "$skill_name" ] || [ "$skill_name" = "null" ]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Check if tool name matches (pipe-separated list) | ||
| # Check required_before — supports both YAML array and legacy pipe-separated string | ||
| # Build a newline-separated list of tool names | ||
| required_before_json="$(echo "$rule_json" | jq -c '.required_before // empty')" | ||
| if [ -z "$required_before_json" ] || [ "$required_before_json" = "null" ]; then | ||
| continue | ||
| fi |
There was a problem hiding this comment.
ℹ️ Dead fallback checks after jq '// …'
jq -r '.max_tool_uses_before_reset // 10' (line 110) already substitutes 10 for missing/null, so the if [ -z "$max_uses" ] || [ "$max_uses" = "null" ] (lines 113-115) can only fire if max_tool_uses_before_reset is a literal empty string — not a realistic YAML input.
Similarly, jq -c '.required_before // empty' (line 123) outputs an empty string (not the literal null) when the field is missing/null, so [ "$required_before_json" = "null" ] on line 124 is dead. Same for command_pattern_json on line 157.
Not a bug, just leftover from the pre-jq path. A one-line simplification on each:
max_uses="$(echo "$rule_json" | jq -r '.max_tool_uses_before_reset // 10')"
if [ -z "$skill_name" ]; then
continue
fi
…
if [ -z "$required_before_json" ]; then
continue
fiNon-blocking cleanup.
| # skill-required plugin configuration | ||
| # | ||
| # Override per-project via ${CLAUDE_PROJECT_DIR}/.claude/settings.skill-required.yaml | ||
| # or per-user via ~/.claude/settings.skill-required.yaml under the "skill-required" key. |
There was a problem hiding this comment.
The PR description says this template "match[es] the pattern used by common-sense and agentic-behavior". It matches on having a <plugin>.settings.yaml default in the plugin root, but it diverges on where the override lives:
common-sense.settings.yaml:Override per-project via
${CLAUDE_PROJECT_DIR}/.claude/plugins.settings.yaml- This file:
Override per-project via
${CLAUDE_PROJECT_DIR}/.claude/settings.skill-required.yaml
The reference plugins use the standard 3-tier plugins.settings.yaml resolution via the shared shared/lib/plugin-config-read.sh helper. This plugin ships its own bespoke file and its own YAML parsing inline.
Not a blocker for this PR (the scripts currently read settings.skill-required.yaml, and the template must match what the scripts read), but the audit's goal of "align with reference plugins" isn't fully met. Worth a follow-up to:
- Symlink
shared/lib/plugin-config-read.shintoplugins/skill-required/lib/. - Set
PLUGIN_NAME="skill-required"in the hooks. - Use
plugin_is_enabled/plugin_get_config_json "skills"etc. - Remove the bespoke
settings.skill-required.yamlpath in favor ofplugins.settings.yaml+ plugin key.
Or update the PR body to clarify that this PR keeps the plugin's existing settings-file location by design.
| # Sanitize a skill name for use as a filesystem path component. | ||
| # Replaces colons (invalid on some filesystems) with double-hyphens. | ||
| # Example: "scm-utils:commit" → "scm-utils--commit" | ||
| # Args: $1=skill_name | ||
| # Returns: sanitized name via stdout | ||
| sanitize_skill_name() { | ||
| local name="$1" | ||
| echo "$name" | sed 's|:|--|g' | ||
| } |
There was a problem hiding this comment.
ℹ️ sanitize_skill_name only handles :
The function name promises general filesystem safety, but only : is replaced. /, \, null bytes, leading . (dotfile), path traversal (..) etc. are passed through. Since skill names in practice are plugin-controlled (plugin:skill-name), this is low risk today, but the name of the function suggests broader guarantees than it provides.
Two options:
-
Narrow the name to what it actually does, e.g.
replace_skill_name_colons()— truthful, avoids false sense of safety. -
Broaden the implementation to also escape
/,\, and other path-unsafe characters:sanitize_skill_name() { local name="$1" # Replace any character that isn't [A-Za-z0-9._-] with a single hyphen echo "$name" | sed 's|[^A-Za-z0-9._-]|-|g' }
This would also cover
:via the default class, so no behavior change for known inputs.
Non-blocking — just worth being explicit about scope in a helper that ends up in lib/.
| # Resolve plugin root (injected by claude-code as CLAUDE_PLUGIN_ROOT) | ||
| _SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| _PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-$(cd "${_SCRIPT_DIR}/../.." && pwd)}" | ||
|
|
||
| # shellcheck source=../../lib/utils.sh | ||
| source "${_PLUGIN_ROOT}/lib/utils.sh" |
There was a problem hiding this comment.
✅ Nice defensive fallback here (and in cache-skill-read.sh) — computing _PLUGIN_ROOT from BASH_SOURCE when CLAUDE_PLUGIN_ROOT is unset means the scripts are still runnable outside a Claude Code session (unit tests, ad-hoc invocation). Good hygiene.
1 participant
Summary
Addresses all findings from the skill-required plugin audit (2026-04-08).
lib/utils.shwithget_project_slug()andsanitize_skill_name()helpers, sourced from both hook scripts — eliminates duplicated project-slug generationscm-utils:commitnow map toscm-utils--commitas cache filenames, preventing invalid path chars on some filesystemscheck-skill-required.shnow reads all skill rules in oneyq -o=jsoncall, then usesjqfor per-rule field extraction — reduces subprocess overhead from 4 yq calls per rule to 1 per loop iterationmise.toml: Declaresyq(mikefarah/yq) as an explicit plugin dependencyskill-required.settings.yaml: Shipped settings template with commented defaults and example rules, matching the pattern used bycommon-senseandagentic-behaviorpluginsrequired_beforeandcommand_patternfields are now documented and templated as proper YAML arrays; legacy pipe-separated strings remain supported for backwards compatibilitySource
.claude/tmp/skill-required-audit.mdin the ai-agent-jack repoTest plan
cache-skill-read.shcorrectly sanitizes colon-containing skill names (scm-utils:commit→ cache filescm-utils--commit.json)check-skill-required.shreads the sanitized cache filename and correctly denies/allows based on skill-load staterequired_beforeandcommand_patternworks the same as the old pipe-separated formatyqdependency is recognized bymise installfrommise.tomllib/utils.shis sourced from both hooks without errorsCo-Authored-By: Jack Oat <jack-nsheaps[bot]@users.noreply.github.com>