feat(skill-quality): enforce shell declaration for injection-carrying skills

[kyle-sexton]

Closes #865

What

Two new checks in plugins/skill-quality/scripts/check-skill.sh, guarding the regression the 2026-07-21 fleet census surfaced (64 skills across 26 plugins using ! injections with bash-only constructs and no shell: declaration — silently broken on a Windows host without Git Bash, where injections fall through to the PowerShell tool). PR #860 swept the fleet; nothing prevented regression until now.

• Check 19 — injection shell-declaration. When a skill carries ! dynamic-context injections (inline !`cmd` or ```! blocks) and declares no shell: frontmatter, it FAILs on detectable bash-only syntax and WARNs on portable-looking commands.
• Check 20 — defensive fallback. WARNs on any injected command with no || <fallback> continuation, per the pinned precompute convention.

Eight self-tests cover both tiers, fenced-block extraction, the ||-not-|| echo match, and two over-reach guards.

FAIL/WARN tiering rationale

Portability is not statically decidable, so the tiers split on evidence strength:

• FAIL only on detectable bash-only syntax with no shell: — the exact census failure. The bash-only token set is deliberately narrow: /dev/null (PowerShell is $null), command -v (a bash builtin; PowerShell is Get-Command), and a pipe into a Unix text tool with no same-named PowerShell cmdlet (head, sed, awk, …; sort/tee are excluded because PowerShell aliases them). Tight avoids a false FAIL that blocks a PR; anything the set misses degrades to the WARN path — never a false negative that FAILs a portable skill.
• WARN when injections exist with no shell: but the commands only look portable — static analysis can't prove it, so nudge rather than block.
• Silent when shell: is declared: the author has taken explicit responsibility for the shell (see scope boundary below).

Check 20 is WARN-only (not FAIL) — the fallback convention is a defensive nicety, not a correctness invariant; injection failure/stderr semantics are undocumented, so this is a nudge.

Why in the plugin's check-skill.sh, not the repo-level check-skill-portability.sh

The repo already has scripts/check-skill-portability.sh, but that is a flat token-present-in-file → violation grep (its concern is ecosystem/forge/branch agnosticism, e.g. a bare origin/main). #865 needs a three-way conditional that a flat scanner cannot express: an injection exists ∧ no shell: is declared ∧ a bash-only token sits inside the injected command text (not anywhere in the file — a bash-only token in a plain ```bash example must not flag). That per-skill, structure-aware logic belongs with the other skill-contract checks in check-skill.sh, which already parses frontmatter and code fences.

Scope boundary

A shell: declaration is trusted wholesale — the check does not validate that the injected commands actually match the declared shell, so shell: pwsh with bash-only commands is intentionally out of scope (per-shell syntax validation is a separate, much larger concern). Both checks scan the injected command text only; inline injections are recognized only at line start or after whitespace (per the injection docs), so a mid-token !` such as an inline `#!` code span in prose is not captured.

Version

skill-quality 0.7.2 → 0.8.0 (minor — matches check-18's minor-bump precedent for a new check), with the parity-gated CHANGELOG entry and regenerated README catalog in the same diff.

Gates run locally (all green)

• check-skill.test.sh — 47 assertions pass (39 existing + 8 new)
• shellcheck --rcfile=.shellcheckrc on both scripts — clean
• scripts/validate-plugins.sh — passes (catalog regenerated)
• scripts/check-changelog-parity.sh --check-bump origin/main — passes
• markdownlint-cli2 on changed markdown — clean
• Self-check of the modified check skill — PASS, all 7 triggers preserved, SKILL.md 107/500

Related

Related but NOT closed by this PR:

• chore: declare shell: bash on every skill with dynamic-context injections #860 — the fleet sweep that added shell: bash to the 64 affected skills; this PR is the regression guard that keeps the census failure from returning.
• ci: portability lint lane — enforce declared agnosticism mechanically (19-issue coupling class + top review-churn source) #531 — the repo-level check-skill-portability.sh agnosticism gate (the sibling "portability" gate this PR deliberately does not extend; see the rationale section above).
• plugins/playbooks/skills/skill-authoring/reference/precompute-context.md — the pinned precompute / defensive-fallback convention that checks 19 and 20 enforce.

[claude]

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

---

PR Review — feat(skill-quality): Check 19 + 20

• Gather context and understand the request
• Read changed files (check-skill.sh, test file, SKILL.md, CHANGELOG, plugin.json, README)
• Review Check 19 logic (injection shell-declaration)
• Review Check 20 logic (defensive fallback)
• Review test coverage
• Check for correctness, security, and CLAUDE.md alignment
• Post findings

---

Summary

Solid implementation of a well-reasoned check. The three-way conditional (injection present ∧ no shell: ∧ bash-only syntax) is correctly expressed, the bash-only token set is appropriately tight to avoid false FAILs, and the fence parsing logic is sound. Eight tests provide good coverage of the principal scenarios. A few things worth looking at before merge:

---

Findings

1. || head (or any listed tool) after || triggers a false FAIL [correctness, minor]

The bash-only pipe regex matches \|[[:space:]]*(head|tail|grep|...) anywhere in the injection string. That catches || head (two | characters: first \| matches the first |, zero spaces from [[:space:]]*, then the second | doesn't match; but then at the second | position: \| + + head). So:

git log --oneline || head -20 /dev/stdin

would FAIL Check 19 because || head matches the pipe-to-head arm of bash_only_re. Using head as a fallback is unusual in practice, but so is an || head continuation. The impact is limited to that specific pattern and the token set is narrow enough that real-world false FAILs are unlikely. Worth noting in a code comment alongside the existing sort/tee exclusion note.

See: check-skill.sh L611–614

Fix this →

---

2. INFO note fires on every passing skill-with-injections run [minor — verbosity]

When shell: is declared and injections are present:

note "shell: declared — dynamic-context injection portability is the author's explicit choice"

This outputs INFO: shell: declared — ... to stdout on every invocation of any skill that has injections + shell:. For skills that PASS cleanly, the INFO line is extra noise. Most other notes in the script are gated by something actionable (char counts, specific counts). This one always fires. Consider dropping it or guarding it behind a verbose flag, since the PASS outcome is already the expected signal.

See: check-skill.sh L622

---

3. Check 20 test coverage gap — multi-injection count [minor — tests]

Test 26 exercises a single injection without fallback and checks that 'carry no' appears. The WARN message includes the count ($missing_fallback \!`-injected command(s) carry no...), but no test verifies the count for 2+ injections without fallback. If the loop increments incorrectly (e.g., exits early), a multi-injection skill might report 0 command(s) carry no ...` and silently skip the WARN. A test fixture with two injection lines would close this gap.

See: check-skill.sh L657–660, check-skill.test.sh L1062–1085

---

What's correct and well-done

• Fence-exit condition (((ilen >= inj_fence_len)) && [[ -z "$iinfo" ]]): handles indented fences, longer-backtick inner "fences", and trailing-space-only info strings (the ltrim expression ${iinfo#"${iinfo%%[![:space:]]*}"} correctly reduces " " to "").
• Inline injection anchor (only at ^ or [[:space:]]): correctly excludes mid-token !`` like `` #!` `` code spans in prose.
• Trust boundary on shell:: trusting the declaration wholesale is the right call — per-shell syntax validation is a separate and much larger concern.
• bash_only_re is correctly narrow: sort/tee exclusion (PowerShell aliases) is well-reasoned and documented in comments. The WARN fallback for portable-looking commands means nothing escalates to a false FAIL.
• shellcheck disable=SC2016 annotations: all single-quoted strings that are intentionally not expanded are annotated with the disable comment and a reason.
• Version bump 0.7.2 → 0.8.0: appropriate MINOR bump for new checks; consistent with check-18 precedent.
• Test 25 (over-reach guard) and Test 28 (prose anchor guard): these two are particularly valuable — they guard the scope boundary that the PR description explicitly calls out.
• Check 20 matches || not || echo: correct; || printf / || true are valid fallbacks, and the test (Test 27 || printf) confirms this.

---

CLAUDE.md alignment

No issues. This is a new check added to an existing plugin's internal script — no new hardcoded paths, no PII, no secrets, plugin-form-safe. Semver bump is present. The CHANGELOG entry meets the parity requirement. The README catalog and plugin.json description are updated in the same diff.

[claude]

Claude finished @kyle-sexton's task in 3m 17s —— View job

---

Security Review — PR #883

• Read PR diff and changed files
• Analyze check-skill.sh for command/shell injection surfaces
• Analyze check-skill.test.sh for security concerns
• Check other changed files (plugin.json, SKILL.md, CHANGELOG, README)
• Compile and post findings

---

Scope

Reviewed the six files changed in this PR against commit b3507be:

• plugins/skill-quality/scripts/check-skill.sh — new checks 19-20 and the INJECTIONS extraction loop (lines 556–656)
• plugins/skill-quality/scripts/check-skill.test.sh — eight new test fixtures (tests 21-28)
• plugins/skill-quality/skills/check/SKILL.md, plugin.json, CHANGELOG.md, README.md — metadata/documentation

---

Findings

No security issues found in the code added by this PR.

Detailed analysis of each candidate surface follows.

INJECTIONS extraction loop (L564–L604)

Skill file content is read line-by-line with IFS= read -r, applied to grep -oE via a here-string (<<< "$il"), and stored in the INJECTIONS array. None of this executes file content. Backtick metacharacters inside $il are inert when passed as here-string stdin — they are never evaluated as shell constructs.

Check 19 — bash-only token scan (L622–L641)

The bash_only_re pattern is a fixed script literal; no user-supplied content is interpolated into it. Each element of INJECTIONS is piped to grep -oE "$bash_only_re" via here-string — the array element is stdin, not a command argument that would undergo word-splitting or glob expansion. The matched hit is then embedded in an error message via err, which calls:

printf 'FAIL: %s\n' "$*"

The %s argument position prevents format-string injection regardless of what $bash_only_hit contains. Because grep -o returns only the matched substring of the fixed pattern, the possible values of bash_only_hit are limited to strings like /dev/null, command -v, or a pipe followed by a named Unix tool — all benign in a printf %s context.

Check 20 — fallback scan (L649–L655)

Pure [[ "$inj" == *'||'* ]] string comparison over INJECTIONS. No execution surface.

Test fixtures (check-skill.test.sh L956–L1141)

Fixtures are written with printf '%s' "$body" into a mktemp -d tree isolated from the real repo. All fixture SKILL.md bodies are single-quoted shell literals, so no expansion occurs at write time. The test suite's git -C "$TMP" calls are properly scoped to the temporary directory. No attacker-controlled content can reach an eval, bash -c, or similar construct.

Other changed files

plugin.json (version bump), SKILL.md (documentation), CHANGELOG.md, README.md — no executable content; no security-relevant changes.

---

Note on a pre-existing pattern (not in this PR's diff)

Check 7 (line 304, unchanged by this PR) runs bash "$test_sh" for each *.test.sh file found under a skill's scripts/ directory. Checking a skill from an untrusted source therefore executes its test scripts — this is a design choice that predates this PR and is documented behaviour. It is not introduced or widened by these changes, so it's out of scope here, but worth keeping in mind if the checker is ever run in an automated pipeline over externally-sourced skills.