feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

[kyle-sexton]

What

Two coordinated changes for the ! dynamic-context-injection concern (one concern, one PR).

No linked issue — this implements ledger "Skill M" items 2–3 (authoring guidance + repeatable check), which have no standalone GitHub issue.

skill-quality — check 18 (precompute opportunity)

Adds an advisory WARN (never a FAIL) to check-skill.sh and its skill. It flags a SKILL.md that gathers deterministic, read-only context by telling Claude to run shell commands at invocation, when that output could instead be inlined at load time via !command`` / ```! injection (one preprocessing pass, no per-invocation tool round-trip).

• Heuristic: scans fenced shell blocks whose command lines are all read-only context-gatherers — a pure-reader allowlist plus a read-only-subcommand allowlist for git/gh (so an unlisted mutation like git stash or gh pr merge is never read-only), redirections excluded; stays silent when the skill already uses any ! injection.
• Honest limits (documented in the check skill Gotchas): a static scan cannot tell an instruction-to-run block from an illustrative example, so a WARN is a candidate to hand-verify, not a defect; it reads fenced blocks only (not prose) and under-reports by design.
• Repo-agnostic: the WARN points at the official #inject-dynamic-context docs, not another plugin, so it stays valid in any consumer repo.
• Four fixture cases added; seventeen→eighteen rename applied across the script header, plugin.json, README, and the check SKILL.md; one eval case added.

playbooks — precompute authoring guidance in skill-authoring

Codified as a locally-owned reference/precompute-context.md spoke (kept out of the upstream-distilled body; vendor/SKILL.md untouched) plus a hub pointer.

• When to precompute vs. not; single-pass semantics, every-invocation rendering, disableSkillShellExecution kill switch, shell: awareness, substitution variables — all pointing at the official docs rather than restating syntax.
• Pinned-by-us conventions: a mandatory || echo "<fallback>" defensive form and shell:/Windows-host awareness. Rationale: the skills docs do not document ! failure/timeout/stderr semantics (verified absent 2026-07-20). Recheck trigger: upstream documents those semantics.

Verification

• check-skill.test.sh: 22 assertions pass (4 new).
• shellcheck clean (repo .shellcheckrc); markdownlint-cli2 clean on all changed markdown.
• Both touched skills pass the full /skill-quality:check gate.
• Changelog parity (incl. bump-vs-main) and catalog-sync checks pass; root catalog regenerated.
• Both plugins bumped a minor version (skill-quality 0.6.0→0.7.0, playbooks 0.2.0→0.3.0).

Related

• Ledger "Skill M" item 1 — the one-time ! precompute sweep across all plugins — is a separate follow-up lane (M2), not part of this PR. This PR delivers the repeatable check and the authoring guidance that sweep will lean on.
• No ADR or decision-log entry is closed by this PR.

Notes for the merge orchestrator

• Branched off origin/main@4b4b67d; main has since advanced. Expect a possible conflict on the seventeen/eighteen lines and the generated catalog block — regenerate the catalog (node scripts/generate-catalog.mjs) after any concurrent description change lands.
• Security surface: read-only static scan + documentation only; no new execution, egress, secrets, or MCP servers. The guidance net-improves posture (defensive fallbacks + kill-switch awareness).

🤖 Generated with Claude Code

[claude]

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

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context and understand the request
• Read the changed files (check-skill.sh, check-skill.test.sh, SKILL.md, precompute-context.md)
• Review heuristic logic and correctness
• Review test coverage and fixture cases
• Review documentation and alignment with CLAUDE.md guidelines
• Post review findings

---

Summary

This PR is well-implemented and follows the repo's design principles. The new check 18 is correctly advisory-only (WARN never FAIL), the write-token denylist/allowlist architecture is defensively conservative, all existing tests should continue to pass, and the playbooks spoke is clean. Four minor issues below, all non-blocking.

---

Findings

1. ${CLAUDE_SKILL_DIR} may be a non-existent substitution variable

precompute-context.md, line 70:

Reference bundled scripts with `${CLAUDE_SKILL_DIR}` (or `${CLAUDE_PLUGIN_ROOT}` for a plugin's own tree)

${CLAUDE_PLUGIN_ROOT} is the established variable used throughout this repo (e.g., check's SKILL.md line 31). ${CLAUDE_SKILL_DIR} doesn't appear anywhere else in the codebase and may not be a valid substitution variable. If this is a hypothetical "per-skill directory" variable, it's worth verifying against the substitution table at the linked docs before a reader relies on it. The pointer to the docs mitigates the risk, but the variable name should be confirmed or replaced with ${CLAUDE_PLUGIN_ROOT}. Fix this →

---

2. run in the write-token denylist disqualifies read-only gh run view/list

check-skill.sh, line 418:

if grep -qE '(^|[[:space:]])(commit|push|...|run)([[:space:]]|$)' <<<"$line"; then

run is intended to catch npm run build, yarn run, etc. But it also disqualifies gh run view and gh run list — both read-only GitHub Actions status commands that would be valid precompute candidates. A skill documenting "check CI status" would have its block incorrectly disqualified by the denylist before the allowlist check even runs.

This is under-reporting (missed opportunity), which is the stated conservative design. Low impact. Worth noting in the check's documented limitations if not already there, or narrowing to npm run / yarn run / pnpm run anchored patterns if the false-negative rate matters.

---

3. Test gap: non-shell fenced block should not trigger the check

The four new test cases (18a–18d) cover: read-only candidate, already-injecting, side-effecting, and git-write. There's no test verifying that a python, json, or text fenced block is NOT flagged, since the scanner only targets shell-typed fences. Given that many SKILL.md files include Python or JSON examples, a regression here would produce false WARNs that the test suite currently wouldn't catch.

Suggested test case:

# 18e. A non-shell fenced block is not flagged.
make_skill precompute-notshellfence '---
name: precompute-notshellfence
description: "Returns JSON. Use when: '"'"'needing json'"'"'."
---
## Steps
```json
{"key": "value"}

Gotchas

None known.
'
out="$(run precompute-notshellfence 2>&1)"
rc=$?
if [[ $rc -eq 0 ]] && ! grep -q 'precompute opportunity' <<<"$out"; then
pass "non-shell fenced block (json) is not flagged as precompute opportunity"
else
fail "json block should not warn precompute (rc=$rc): $out"
fi

---

**4. Whole-file `!` injection gate can be triggered by illustrative prose examples**

[`check-skill.sh`, line 436](https://github.com/melodic-software/claude-code-plugins/blob/a23efaf1ee7d6de830b71160ddfdc45609118a9b/plugins/skill-quality/scripts/check-skill.sh#L436):

```bash
if grep -qE '(^|[[:space:]])!`' "$SKILL_MD" || grep -qE '^[[:space:]]*```+!' "$SKILL_MD"; then
  PRECOMPUTE_INJECTS=1
fi

This is a whole-file byte scan — it would match !`command` appearing inside a markdown example block that explains the syntax, or inside a comment. A skill authoring guide that documents ! injection as an illustrative example (not actual injection) would trigger PRECOMPUTE_INJECTS=1 and silence check 18 for all its shell blocks, even if those blocks are genuine precompute candidates.

This is an edge case (skills describing ! injection are rare, and the playbooks spoke just added is a reference doc not a procedural skill). Documenting it in the Gotchas section of the check SKILL.md would cover the practical surface. The existing gotcha at line 98 already mentions the under-reporting limit, but not this specific cause.

---

Positive notes

• Fence scanner architecture: The single-level CommonMark fence scan (track opener fence-length, require same-or-more-length bare closer) is correct. Inner ``` inside a wider ```` opener is correctly treated as literal content, not scanned as a shell block.
• Denylist + allowlist combination: Conservative and appropriate for an advisory check. Defaulting to "not a read-only context-gatherer" prevents false positives (spurious WARNs) at the cost of false negatives (missed opportunities). The right tradeoff for an advisory check.
• Test coverage: The four fixture cases cover all four primary branches of the logic correctly. Assertion messages clearly state expected vs. actual.
• Repo-agnostic WARN text: Pointing at https://code.claude.com/docs/en/skills#inject-dynamic-context rather than another plugin is exactly right.
• Playbooks spoke: Locally-owned, clearly separated from vendor/ baseline, "recheck trigger" convention is good practice, and the hub SKILL.md pointer to reference/precompute-context.md prevents check 15 from warning about an orphan spoke.
• Version bumps, changelog parity, and catalog regeneration are all handled correctly per the PR description.

---

[claude]

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

---

Security Review — PR #773

• Gather PR diff and changed files
• Review check-skill.sh for shell injection and unsafe patterns
• Review check-skill.test.sh for test safety
• Review new precompute-context.md and SKILL.md changes
• Review plugin.json and evals.json changes
• Summarize security findings

---

No security vulnerabilities found. Findings below, most-severe first.

---

Findings

SUGGESTION — confidence: HIGH

Write-token denylist is per-line, not per-subcommand: git log --grep='commit message' is safe but a SKILL.md containing git log commit would correctly be disqualified, while git log --grep='add changes' would be incorrectly disqualified (false positive advisory WARN, not a security issue)

This is a heuristic correctness note, not a security defect. The grep -qE '(^|[[:space:]])add([[:space:]]|$)' <<<"$line" call matches the word add in a bare token position (preceded by start-of-string or whitespace, followed by whitespace or end-of-string). An argument value like --grep='add changes' is safe because the ' before add is not a space character, so the leading group (^|[[:space:]]) does not match — the denylist behaves correctly here. A bare git log add would correctly disqualify. The only real exposure is a false-positive WARN (an advisory suppressed for a safe read-only block) — no execution, no data exfiltration, no privilege change.

Permalink

---

What I checked

check-skill.sh — Check 18 block (L398–L491)

• Shell injection via SKILL.md content: SKILL.md is read with IFS= read -r bl (the safe pattern); each line is passed to precompute_readonly_line "$bl" as an argument and then consumed only by grep -qE ... <<<"$line" (a here-string — stdin to grep, never eval'd) and case "$line" in patterns. No content from the analyzed file is ever executed as shell code.
• Regex construction from user input: fence_open_re is a hard-coded string in the script, not derived from file content. Its use as an unquoted operand in [[ "$trimmed" =~ $fence_open_re ]] is intentional bash idiom (quoting would make it a literal string). No injection surface.
• Write-token denylist logic (L418): the two-gate design (denylist first, then allowlist for command words) is conservative. git and gh are allowed wholesale and constrained by the denylist to avoid maintaining a subcommand allowlist that would need continuous updating. No novel execution surface.
• $SKILL_MD path handling: all uses are double-quoted; no word-splitting or glob expansion. The path-traversal risk (e.g. ../ in a skill name) pre-dates this PR and is not worsened — this PR adds only additional read operations using the same already-validated path.
• warn message output (L490): the WARN string is a single-quoted literal — no interpolation of file content into the output.

check-skill.test.sh (L465–L469): fixture SKILL.md bodies are single-quoted string literals written to temp files — no expansion, no execution of fixture content.

precompute-context.md and SKILL.md: pure documentation. The recommended || echo "<fallback>" defensive fallback is a sound and conservative security posture; the guidance explicitly acknowledges unknown ! failure semantics and documents the disableSkillShellExecution kill switch.

plugin.json, evals.json, CHANGELOG.md: version bumps and eval prompts; no executable content, no secret handling, no new permissions or MCP servers.

No new egress, secrets handling, code execution, or permission changes in this PR. The PR description's self-assessment ("read-only static scan + documentation only; no new execution, egress, secrets, or MCP servers") is accurate.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a23efaf1ee

ℹ️ 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 finished @kyle-sexton's task in 3m 18s —— View job

---

Security Review — PR #773

• Gather context and understand the request
• Read PR diff and changed files
• Audit check-skill.sh check 18 block for injection and logic flaws
• Audit test fixtures for unsafe patterns
• Audit documentation and plugin manifests
• Post security findings

---

No exploitable security vulnerabilities found. One SUGGESTION below.

---

Findings

SUGGESTION — confidence: HIGH

git-branch of precompute_readonly_line does not screen for pipe-embedded mutations (git log | tee output.txt is classified read-only)

check-skill.sh, lines ~440–450

The pure-reader commands (ls, cat, find, …) run through a secondary grep gate that rejects embedded mutating tokens (tee, rm, mv, etc.). The git and gh branches skip that gate entirely: a line like git log | tee build-log.txt extracts subcommand log, finds it in the read-only allowlist, and returns 0 (read-only candidate). The advisory WARN would then suggest precomputing it — advice that, if followed, would run tee build-log.txt on every skill load.

Impact is bounded: this is an advisory WARN only; no code is executed and no mutation happens unless the skill author manually acts on the advice. This is a false-positive quality issue rather than a direct security exploit. Applying the embedded-mutation grep screen to the git/gh branches — or documenting the pipe-through gap as a known limit in the Gotchas section — closes the surface.

---

What I checked

check-skill.sh — Check 18 block (new lines, both commits)

• Shell injection via SKILL.md content. Every line from $SKILL_MD enters the loop via IFS= read -r bl and is consumed only by:

  • case "$line" in … — shell case matching (no execution)
  • ${line#…} / ${line%%…} parameter expansions — no execution
  • grep -qE '…' <<<"$line" — here-string to grep; file content is never on the command line and never eval'd
  • [[ "$trimmed" =~ $fence_open_re ]] — $fence_open_re is a hardcoded literal; $trimmed is the string operand (left side), not the regex; no injection surface

  No SKILL.md content is ever executed as shell code. Safe.

• warn message interpolation. The emitted WARN string is a hard-coded single-quoted literal; no file content is interpolated into the output. Safe.

• $SKILL_MD path handling. Both new grep calls (grep -qE … "$SKILL_MD") and the done <"$SKILL_MD" redirect double-quote the path; no word-splitting or glob expansion. The path is pre-validated by earlier checks (pre-existing). Safe.

• fence_open_re regex. Hardcoded as '^(```+)([^]*)$'; not derived from file content. Simple linear pattern — no catastrophic backtracking risk. BASH_REMATCHcaptures are used only for${#ticks}(length arithmetic) and acasematch on$info_first`. Safe.

• Subcommand allowlist correctness (second commit 4330015). The refactor correctly addressed the Codex P2 finding: git stash, git clean, git branch -D, and similar mutations are now rejected at the subcommand level (not allowlisted). gh run list / gh pr diff are correctly admitted. The remaining gap (pipe-embedded tokens) is noted above.

check-skill.test.sh — Fixture SKILL.md bodies are single-quoted string literals; backticks and ! characters inside are not expanded. The make_skill helper writes them to a mktemp directory. No execution of fixture content. Safe.

precompute-context.md — Pure documentation. The || echo "<fallback>" defensive convention is a sound security posture acknowledging unknown !-failure semantics. The disableSkillShellExecution kill-switch mention improves posture. No executable content. Safe.

plugin.json (both plugins) — Version bumps only. No new permissions, MCP servers, egress declarations, or execution surfaces. Safe.

---

[claude]

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

---

PR Review — refactor(skill-quality): git/gh read-only subcommand allowlist in check 18

• Gather context and understand the request
• Diff the latest commit (refactor of git/gh subcommand allowlist)
• Review the allowlist implementation for correctness and coverage
• Check for regressions and edge cases
• Verify ${CLAUDE_SKILL_DIR} finding from prior review
• Post findings

---

Summary

The latest commit (4330015) is a clean, correct fix for two converging review findings: the Codex P2 on unlisted mutating git subcommands, and the prior Claude review's finding #2 on gh run view/list being blocked by the run denylist token. The approach — a narrow read-only subcommand allowlist for git/gh instead of wholesale allowance + a broad write-token denylist — is the right design, and the miss direction is always the safe one (missed precompute opportunity, never advice to auto-run a mutation).

One prior review finding is resolved (confirmed false alarm). Three new fixtures cover the three gaps originally noted. Two minor edge cases remain, both non-blocking.

---

Findings

1. ${CLAUDE_SKILL_DIR} (prior finding #1) — confirmed valid; no action needed

The previous Claude review flagged ${CLAUDE_SKILL_DIR} in precompute-context.md:70 as possibly unverified. A repo-wide search shows it's an established substitution variable used in at least four plugins (docs-hygiene, code-tidying, firecrawl — with ! injection in some). Finding #1 was a false positive. ✓

---

2. git flags before subcommand — safe miss (non-blocking)

The subcommand extraction:

local sub="${line#git}"
sub="${sub#"${sub%%[![:space:]]*}"}"
sub="${sub%%[[:space:]]*}"

For git -C /repo/path log, sub extracts as -C, which doesn't match any allowlist entry → return 1. The line is silently skipped as a precompute candidate. This is the safe miss direction (no WARN on a potentially-mutating block), but it means git -C prefixed commands are never recognized. Given that SKILL.md files rarely use git -C, the false-negative rate is very low in practice. A comment in the git case noting this limit would help future maintainers, but it's not required.

---

3. gh flag values containing "comment" as a bare word — safe miss (non-blocking)

For a line like gh issue list --search "fix comment styling", the padded-string pattern *" comment "* matches the word comment in the search term → return 1. This is a false-negative (valid precompute candidate disqualified), landing on the safe side. Low practical impact — search arguments with bare comment in a SKILL.md are rare. The existing Gotchas text in the check SKILL.md already covers the general under-reporting caveat.

---

4. Three new fixtures — all correct

Case	Input	Expected	Rationale
18e	git stash / git clean -fd	No WARN	Both unlisted in git allowlist → return 1 ✓
18f	```json fence	No WARN	is_shell=0 for json type → block never scanned ✓
18g	gh pr diff / gh run list	WARN fires	diff and list in read-verb allowlist ✓

Assertion message for 18d also correctly updated from "write-token denylist" to "subcommand allowlist" to reflect the new mechanism.

---

Positive notes

• Safe miss direction preserved: an unlisted git subcommand or an unknown gh verb now defaults to return 1, never to return 0. The refactor makes the conservative bias explicit rather than accidental (previously any git subcommand not in the write-token denylist was classified read-only).
• gh two-gate design: checking write verbs first, then read verbs, with a *) return 1 fallback means gh api, gh auth, gh search (none of them in the allowlist) are silently skipped. gh api in particular can POST — the fallback is the right behavior.
• The run denylist token is now gone: gh run view and gh run list are correctly recognized as read-only candidates (confirmed by new fixture 18g).
• Pipe safety in pure-reader path: find -delete is caught by the embedded-mutation screen on the pure-reader branch (ls | pwd | cat | ...). The git branch does not apply the embedded-mutation screen, which is fine — git log | tee /tmp/f is an advisory miss (WARN is still issued), not a false safe.
• 25 assertions, shellcheck + markdownlint clean: per the commit message. Assertion count increased from 22 (prior commit) to 25 (three new fixtures), matching the three added cases.

---

Verdict

The refactor is sound and complete for its stated scope. The two remaining edge cases (#2, #3 above) are both safe-miss direction and well within the documented "advisory heuristic, not a defect" contract for check 18. No blocking issues.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4330015c7e

ℹ️ 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 finished @kyle-sexton's task in 2m 30s —— View job

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context (PR body, prior reviews, commit history)
• Read latest commit changes (b012f7e — pipeline mutation fix)
• Review check-skill.sh check 18 block (all three commits)
• Review check-skill.test.sh fixture coverage
• Review precompute-context.md and SKILL.md documentation
• Verify prior review findings resolved/still-open
• Post final review

---

Summary

This is a clean, well-structured three-commit PR. The check 18 implementation is correct, its heuristic bias is in the right direction (safe miss, never false-safe), and all prior reviewer P2/blocking issues have been resolved. No blocking issues remain. Two minor non-blocking observations below.

---

Latest commit (b012f7e) — pipe disqualification

The fix is correct. The ||-strip-before-pipe-test approach is the right implementation:

local no_or="${line//||/}"
case "$no_or" in
  *'|'*) return 1 ;;

This correctly:

• Passes git status --short || echo "(status unavailable)" — || is stripped, no | remains → still a candidate ✓
• Rejects git status --short | tee snapshot.txt — | (not ||) survives the strip → disqualified ✓
• Handles git status ||| echo fallback safely — || consumed → | remains → disqualified ✓
• Handles git status || echo "a || b" correctly — both || stripped, no | remains → candidate ✓

The two new test fixtures (18h pipe-sink, 18i or-fallback) directly exercise the critical path and are properly asserting opposite outcomes.

---

Prior review findings status

Finding	Source	Status
Mutating git subcommands not on a denylist	Codex P2 (commit a23efaf)	✅ Resolved by 4330015 (allowlist replaces denylist)
run blocks gh run view/list	Claude review #2	✅ Resolved by 4330015 (allowlist admits list)
Test gap: non-shell fenced block	Claude review #3	✅ Resolved by 4330015 (fixture 18f)
Mutating pipeline after read-only head	Codex P2 (commit 4330015)	✅ Resolved by b012f7e
Whole-file ! gate triggered by prose	Claude review #4	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review #1	✅ Confirmed valid — used in docs-hygiene, code-tidying, firecrawl

---

Observation 1 — git -C /path log safely misses (non-blocking)

check-skill.sh, line ~441

The subcommand extractor strips git and leading whitespace, then takes the first token:

local sub="${line#git}"
sub="${sub#"${sub%%[![:space:]]*}"}"
sub="${sub%%[[:space:]]*}"

For git -C /repo log, sub resolves to -C, which is not in the allowlist → return 1. This is a safe-miss (missed precompute opportunity, never advice to run a mutation). git -C prefixed invocations are uncommon in SKILL.md bodies. Not blocking.

---

Observation 2 — gh command containing "comment" as a bare word in a value safely misses (non-blocking)

check-skill.sh, line ~453

For gh issue list --search "fix comment styling", the pattern *" comment "* matches comment in the search value → return 1. This disqualifies a valid candidate. Impact is low — search arguments with bare comment in a SKILL.md are rare and the existing Gotchas already covers under-reporting. Safe-miss direction. Not blocking.

---

Positive notes

• ||-strip logic: Correctly differentiates || (logical OR fallback) from | (pipe sink). The strip is applied to the whole line before the pipe check, so the fallback form the guidance mandates is never accidentally disqualified.
• Allowlist architecture (commits 4330015): The conservative default (unlisted subcommand → return 1) ensures new git or gh subcommands introduced after this PR is merged land on the safe side without requiring a code update.
• 9 test fixtures (18a–18i): Cover all primary branches: read-only candidate, already-injecting gate, side-effecting command, git write subcommand, unlisted git subcommand, non-shell fence, gh read verbs, pipe-sink disqualification, and || fallback preservation. Assertion messages are specific and diagnostic.
• Gotchas section in check SKILL.md: Accurately characterizes check 18 as heuristic-advisory, explains the under-reporting-by-design behavior, and flags the ! whole-file gate edge case.
• precompute-context.md: Clean, locally-owned spoke. "Recheck trigger" convention is good practice. Mandatory defensive fallback + shell: awareness conventions are correctly scoped as Melodic pinning on top of undocumented upstream semantics.
• Repo-agnostic WARN text: Points at https://code.claude.com/docs/en/skills#inject-dynamic-context rather than another plugin. Valid in any consumer repo.

---

Verdict

Ready to merge. The three commits form a coherent, well-reviewed progression. All P2/blocking findings from prior reviews are resolved. The two remaining observations both land in the safe-miss direction, are consistent with the documented "advisory heuristic, not a defect" contract, and are non-blocking.

[claude]

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

---

Security Review — PR #773 (commit b012f7e)

• Gather context and understand the request
• Fetch PR diff (git diff 4b4b67d...HEAD)
• Read changed files for security analysis
• Audit check-skill.sh check 18 block for injection and logic flaws
• Audit test fixtures and documentation for unsafe patterns
• Post security findings

---

No exploitable security vulnerabilities found. One SUGGESTION below.

---

Findings

SUGGESTION — confidence: HIGH

|| stripping is too broad for git/gh branches: git status || rm -f file is classified read-only

check-skill.sh, line 427–448

The b012f7e fix correctly rejects single-pipe mutations (git status | tee f), but the ||-stripping step is too broad for the git/gh classification branches. Walking through git status || rm -f file:

1. local no_or="${line//||/}" → git status rm -f file (no || remaining)
2. case "$no_or" in *'|'*) → no | in no_or → passes the pipe gate
3. cmd = git, sub = status → status is in the allowlist → return 0

A WARN is issued advising the author to convert the block to ! injection. If followed, the rendered skill runs rm -f file on every invocation where git status exits non-zero (e.g., outside a git repo).

The pure-reader branch (e.g., cat, ls) is not affected: it applies an embedded mutating-token grep against the original $line (not $no_or), so cat file || rm -f other is correctly rejected by that screen. The gap is specific to git/gh, which return based only on the allowlisted subcommand and skip the embedded mutation check entirely.

Practical impact is bounded: the check is advisory only (WARN, never FAIL), the author must manually convert, and the documented recommended form (|| echo "<fallback>") is safe. A SKILL.md that contains git status || rm -f file is unusual; the risk is that a confused author following the advisory would escalate a latent local mutation into a per-invocation !-injection side-effect.

Narrowest fix: after resolving the git/gh subcommand and before returning 0, run the same embedded-mutation grep against the original $line:

git)
  ...
  case "$sub" in
    status | log | ...) 
      grep -qE '(^|[[:space:]])(rm|mv|cp|tee|sed|mkdir|touch|delete|-delete)([[:space:]]|$)' <<<"$line" && return 1
      return 0 ;;
    *) return 1 ;;
  esac

Or document the gap explicitly in the check 18 Gotchas: "|| <mutation> after an allowlisted git/gh subcommand is not screened; only || echo/|| printf are safe forms."

---

What I checked

Shell injection via SKILL.md content — SKILL.md lines enter the loop via IFS= read -r bl and are consumed only by bash parameter expansions (${bl#...}), case "$line" in pattern matching, [[ "$trimmed" =~ $fence_open_re ]] (content is the left operand, not the regex), and grep -qE ... <<<"$line" (here-string stdin to grep, never on the command line). File content is never eval'd. Safe.

fence_open_re regex — '^(```+)([^]*)$'is a hardcoded literal, not derived from file content. Used as the right operand of=~` (unquoted, bash idiom). No catastrophic backtracking; no injection surface. Safe.

$SKILL_MD path handling — all uses are double-quoted in shell expansions and <"$SKILL_MD" redirections; no word-splitting or glob expansion. Safe.

PRECOMPUTE_INJECTS gate — grep -qE '(^|[[:space:]])!' "$SKILL_MD"is syntactically correct (backtick is literal inside single quotes). A false positive (prose example of!` syntax silences the check) is acknowledged in prior reviews and is a quality miss, not a security issue. Safe.

Fence scanner CommonMark correctness — inner ``` inside a wider ```` opener correctly goes to ((n >= fence_len)) && [[ -z "$info" ]], which fails for the inner triple since `3 < 4`. Inner backtick sequences are treated as literal content, never opened as nested shell blocks. Safe.

check-skill.test.sh — fixture SKILL.md bodies are single-quoted string literals; backticks and ! characters are content, not shell expansions. Written to mktemp directories. No execution of fixture content. Safe.

precompute-context.md and SKILL.md changes — pure documentation. The || echo "<fallback>" recommendation is a sound posture. The disableSkillShellExecution mention improves posture. No executable content. Safe.

plugin.json files (both plugins) — version bumps only. No new permissions, MCP servers, egress declarations, or execution surfaces. Safe.

No GitHub Actions workflow changes in this PR. No new pull_request_target, workflow_run, script injection surfaces via github context, or permission widening.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b012f7ecf2

ℹ️ 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 finished @kyle-sexton's task in 4m 41s —— View job

---

Security Review — PR #773 (commit 9e23619)

• Gather PR diff and identify changed files
• Audit check-skill.sh check 18 block for injection/logic flaws
• Audit check-skill.test.sh for unsafe test patterns
• Audit documentation files and plugin manifests
• Post security findings

---

No GitHub Actions workflow changes. No new permissions, MCP servers, egress declarations, or execution surfaces. SKILL.md content is never executed — the scanner consumes lines only via IFS= read -r, case pattern matching, parameter expansion, and grep … <<<"$line" here-strings. Two findings follow.

---

Findings

IMPORTANT — confidence: HIGH

&& and ; compound operators are not screened: git status && rm -f file is classified read-only

check-skill.sh line 441–448

After the pipe guard (line 428–431) passes, the git and gh branches inspect only the first token after the command word and return based solely on the subcommand allowlist. Neither && nor ; is stripped or rejected before subcommand extraction, so everything after the first space-delimited token (status, log, etc.) is silently dropped.

Tracing git status && rm -f file through precompute_readonly_line:

1. No > redirection — passes.
2. no_or = "git status && rm -f file" (no || to strip). No | in no_or — passes.
3. cmd = git.
4. sub: ${line#git} → status && rm -f file; ltrim → status && rm -f file; ${sub%%[[:space:]]*} → status.
5. status is allowlisted → return 0. The && rm -f file tail is never examined.

PRECOMPUTE_CANDIDATE=1 is set. The WARN advises converting the block to ! injection. An author who follows that advice without reading the block converts git status && rm -f file into a load-time ! injection that runs rm -f file on every skill invocation where git status exits non-zero.

The same gap exists for the gh branch and for ; separators. It is partially mitigated in the pure-reader branch: the embedded mutation grep (line 438) checks the full $line including tokens after &&, so ls && rm -f file is correctly rejected (because rm is in the denylist). But the git/gh branches skip that grep entirely and rely only on the subcommand extraction, leaving git status && <any mutation> undetected.

Narrowest fix: before the case "$cmd" in git) branch returns 0, screen the original $line for && or ; operators:

git)
  ...
  case "$sub" in
    status | log | ...)
      case "$line" in *'&&'* | *';'*) return 1 ;; esac
      return 0 ;;
    *) return 1 ;;
  esac

Or apply the same embedded-mutation grep used in the pure-reader branch against the full $line before returning 0.

---

SUGGESTION — confidence: HIGH

|| stripping is too broad: git status || rm -f file is classified read-only (prior finding, unresolved)

check-skill.sh line 427

no_or = "${line//||/}" strips every occurrence of || before the pipe guard. For git status || rm -f file, stripping || produces git status rm -f file — no | character remains, the pipe guard passes, cmd=git, sub=status → classified read-only.

The intent is to allow the recommended defensive form git status || echo "<fallback>" without false-negatives. But the strip is unconditional: any || including || rm -f file or || bash ./mutate.sh is silently removed. An author who follows the WARN and converts git status || rm -f file to ! injection introduces a mutation that runs on every skill load when git status exits non-zero.

This was raised as a SUGGESTION in the security review of commit b012f7e and remains unresolved in the current head.

Narrowest fix: scope the strip to only || echo and || printf — the only safe fallback forms:

local no_or="${line}"
no_or="${no_or//||\ echo*/}"
no_or="${no_or//||\ printf*/}"

Or screen for a restricted set of post-|| tokens (whitespace then echo/printf) rather than stripping all ||.

---

What I checked

Shell injection — SKILL.md content enters only via IFS= read -r bl and is consumed by case, ${…} parameter expansions, [[ … =~ … ]] (content is the left operand), and grep … <<<"$line" here-strings. Nothing is eval'd. No injection surface. Safe.

fence_open_re regex — hardcoded literal '^(```+)([^]*)$'`, not derived from file content. No catastrophic backtracking. Safe.

$SKILL_MD path handling — all uses double-quoted; no word-splitting or glob expansion. Safe.

PRECOMPUTE_INJECTS gate — the whole-file ! scan can be triggered by prose that documents ! syntax (silencing check 18 for the whole file). Documented in SKILL.md Gotchas; quality issue, not security. Safe.

check-skill.test.sh — fixture bodies are single-quoted string literals; backticks and ! characters are content, never expanded. Written to a mktemp directory. No execution of fixture content. Safe.

precompute-context.md, SKILL.md changes, plugin.json, evals.json — documentation and version bumps only. No executable content, no new permissions or MCP servers. Safe.

GitHub Actions — no workflow files changed. Safe.

[claude]

Claude finished @kyle-sexton's task in 5m 36s —— View job

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context (PR body, prior reviews, commit history)
• Read current state of check-skill.sh check 18 block
• Verify two open Codex P2 findings on b012f7e — both confirmed genuine
• Fix: add && / ; continuation gate to disqualify multi-command lines
• Fix: remove env from pure-reader allowlist (launches arbitrary commands)
• Fix: add -exec / -ok to embedded mutation check for find
• Add 3 regression test cases (18j–18l)
• Update CHANGELOG
• Commit (push blocked — see note below)

---

Summary

The two remaining open Codex P2 findings on b012f7e are both genuine false-safe gaps in precompute_readonly_line. I've applied fixes in a local commit (5d987e1) — the remote push was denied by this bot's token. Changes are ready to land; the diff is small and fully tested (30 assertions).

---

Codex P2 findings — verified and fixed

Finding 1: && / ; shell continuations not screened

Root cause: Only | (pipe) was gated; && and ; pass through. So git status --short && touch marker extracts subcommand status, finds it in the allowlist, and returns 0 → WARN issued.

Trace for git status --short && touch marker:

1. No > → passes redirection gate
2. || strip: no change; case "$no_or" only checked *'|'* — && is not | → passes (bug)
3. cmd = git, sub = status → in allowlist → return 0 → WARN issued advising precompute

Fix — check-skill.sh line 429:

# Before
case "$no_or" in
  *'|'*) return 1 ;;

# After
case "$no_or" in
  *'|'* | *'&&'* | *';'*) return 1 ;;

Finding 2a: env CMD classified as read-only

Root cause: env was in the pure-reader allowlist. The embedded mutation token check (rm|mv|cp|tee|...) doesn't include bash, sh, etc., so env bash ./deploy.sh passes the mutation screen and returns 0 → WARN issued.

Fix: Remove env from the pure-reader allowlist. env CMD executes CMD; env alone (display all vars) or via pipe (already caught by pipe gate) is too rare a precompute target to keep the allowance. Unlisted commands fall to *) return 1.

Finding 2b: find -exec / find -ok classified as read-only

Root cause: -exec and -ok (find's command-launcher options) were not in the embedded mutation token pattern. find . -exec rm {} + passes the screen and returns 0 → WARN issued.

Fix — add -exec and -ok to the pattern:

# Before
grep -qE '(^|[[:space:]])(rm|mv|cp|tee|sed|mkdir|touch|delete|-delete)([[:space:]]|$)' <<<"$line"

# After
grep -qE '(^|[[:space:]])(rm|mv|cp|tee|sed|mkdir|touch|delete|-delete|-exec|-ok)([[:space:]]|$)' <<<"$line"

---

New test cases (18j–18l)

Case	Input	Expected	Rationale
18j	git status --short && touch marker	No WARN	&& after `
18k	find . -exec rm {} +	No WARN	-exec in mutation token list → return 1
18l	env bash ./scripts/deploy.sh	No WARN	env removed from allowlist → *) return 1

Total assertions: 30 (up from 27).

---

Changes made locally (commit 5d987e1, push blocked)

plugins/skill-quality/CHANGELOG.md           |  6 +-
plugins/skill-quality/scripts/check-skill.sh | 15 ++---
plugins/skill-quality/scripts/check-skill.test.sh | 75 ++++++++++++

The push to feat/precompute-check-and-guidance was denied (github-actions[bot] lacks write access to this repo). Please cherry-pick or apply the three changes above — they're minimal and each corresponds directly to one P2 finding. The rest of the PR is ready to merge once these three gaps are closed.

---

Prior findings status (all resolved)

Finding	Source	Status
Mutating git subcommands (clean, stash, etc.)	Codex P2 (a23efaf)	✅ 4330015
run blocks gh run view/list	Claude review	✅ 4330015
Non-shell fenced block test gap	Claude review	✅ 4330015
Mutating pipeline after read-only head	Codex P2 (4330015)	✅ b012f7e
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 5d987e1 (local)
env CMD classified read-only	Codex P2 (b012f7e)	✅ 5d987e1 (local)
find -exec classified read-only	Codex P2 (b012f7e)	✅ 5d987e1 (local)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e23619d5e

ℹ️ 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 finished @kyle-sexton's task in 3m 24s —— View job

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context and understand the request
• Read the latest commit (0da65d5 — "fail closed on shell metacharacters in check 18")
• Verify whether remaining Codex P2 findings are resolved
• Review the full check 18 block in check-skill.sh
• Review test coverage in check-skill.test.sh
• Post review findings

---

Summary

The latest commit (0da65d5) comprehensively addresses all four remaining Codex P2 findings from the review of 9e23619. The approach — failing closed on any shell metacharacter that can introduce a second command — is architecturally sound and avoids the prior pattern of case-by-case enumeration. All previously open findings are now resolved. No blocking issues. Two minor non-blocking observations below.

---

Prior findings status — all clear

Finding	Source	Status
Mutating git subcommands not on a denylist	Codex P2 (a23efaf)	✅ 4330015 — allowlist replaces denylist
run blocks gh run view/list	Claude review	✅ 4330015 — allowlist admits list
Non-shell fenced block test gap	Claude review	✅ 4330015 — fixture 18f
Mutating pipeline after read-only head	Codex P2 (4330015)	✅ b012f7e — | pipe gate
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 0da65d5 — *'&'* | *';'* in metachar case
Command substitution $(...) not screened	Codex P2 (9e23619)	✅ 0da65d5 — *'$('* check
env CMD classified as read-only	Codex P2 (b012f7e)	✅ 0da65d5 — env dropped from allowlist
find -exec classified as read-only	Codex P2 (b012f7e)	✅ 0da65d5 — -exec|-execdir|-ok|-okdir in grep
git diff --output=file classified as read-only	Codex P2 (9e23619)	✅ 0da65d5 — --output in grep pattern
Whole-file ! gate triggered by prose	Claude review	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review	✅ Confirmed — used in docs-hygiene, code-tidying, firecrawl, playbooks/update
|| stripping too broad (git status || rm -f file)	Security SUGGESTION	✅ Mitigated — grep checks original $line (not $no_or), so rm, tee, mv etc. in the tail are caught by the existing token pattern

---

Observation 1 — git status || bash ./deploy.sh safely misses (non-blocking)

check-skill.sh line 437

The || strip removes || before the pipe test, and the follow-up grep runs against the original $line. For || rm -f file this works correctly because rm is in the grep token list. For || bash ./deploy.sh, bash is not in the grep list, so the tail mutation survives and the line is classified read-only.

This is the residual limit of the || concern raised in earlier security reviews. In practice: (a) bash <script> as a || fallback in a SKILL.md is unusual; (b) the guidance mandates || echo "<fallback>" as the only sanctioned form; (c) the check is advisory-only; (d) an author following the WARN would need to manually craft the ! injection and would see bash ./deploy.sh in their own file. Documenting this remaining gap in the Gotchas section alongside the existing under-reporting caveat would be the minimal closure.

---

Observation 2 — *'<'* gate safely rejects cat < file.txt (non-blocking)

check-skill.sh line 429

Input redirection (cat < file.txt) is read-only but is rejected by the *'<'* case gate. Since cat file.txt without redirection is the common form in SKILL.md, the false-negative rate is very low. Safe-miss direction, consistent with the documented under-reporting contract.

---

Positive notes

• Fail-closed metacharacter approach: Switching from case-by-case mutation enumeration to a "disqualify any character that can introduce a second command" gate is the right architectural move. It is provably exhaustive for the listed metacharacters rather than relying on a maintainer-updated denylist.
• <( / >( process substitution correctly caught by the existing *'<'* / *'>'* gates (noted in the comment).
• || semantic: The no_or strip is scoped only to the pipe-vs-logical-or disambiguation; the grep for mutation tokens runs against the original $line, so || rm / || tee / || mv are still rejected even after the strip.
• --output pattern: ([[:space:]=]|$) delimiter group correctly handles both --output=file (equals-sign delimiter) and --output file (space delimiter) and --output alone at end of line.
• Fixture coverage (18a–18m, 31 assertions): Each new case (18j–18m) directly targets one of the four P2 fixes and asserts the opposite outcome from 18i (|| fallback). Assertion messages are specific and diagnostic.
• ${CLAUDE_SKILL_DIR} confirmed: Used in 7+ plugin files across the repo; the earlier review flag was a false positive.
• precompute-context.md: Clean locally-owned spoke. "Recheck trigger" convention is good practice. The document correctly identifies undocumented ! failure/timeout/stderr semantics and pins a conservative mandatory fallback without overstating what's upstream-documented.

---

Verdict

Ready to merge. The five-commit progression closes every P2/IMPORTANT finding raised across four prior review passes. The two remaining observations both land in the safe-miss direction and are well within the documented "advisory heuristic, not a defect" contract for check 18.

[claude]

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

---

Security Review — PR #773 (commit 0da65d5)

• Gather context and understand the request
• Fetch PR diff and identify changed files
• Audit check-skill.sh check 18 block for injection/logic flaws
• Audit check-skill.test.sh for unsafe test patterns
• Audit documentation files and plugin manifests
• Post security findings

---

One finding below. All prior P2 findings from earlier review passes are resolved. No injection, path-traversal, secrets, or GitHub Actions issues in the new code.

---

Findings

IMPORTANT — confidence: HIGH

||-stripping does not validate what follows ||; shell interpreters and network tools in a || fallback are classified read-only

check-skill.sh, line 437–438

The || strip at line 437 removes every occurrence of || before the pipe gate. The mutation grep at line 443 catches specific tokens (rm, mv, cp, tee, sed, mkdir, touch, -exec, --output) but does not include shell interpreters (bash, sh, python3, node, ruby, perl) or network tools (curl, wget, nc, ssh).

Tracing git status || bash ./deploy.sh through precompute_readonly_line:

1. Line 429: no >, <, ;, & (note: || contains |, not &) → passes
2. Line 433–434: no $(, no backtick → passes
3. Line 437: no_or = "git status bash ./deploy.sh" (both | stripped)
4. Line 438: no_or has no | → passes
5. Line 443: mutation grep against $line — bash is absent from the token list → grep returns 1, && return 1 does not fire → passes
6. cmd=git, sub=status → allowlisted → return 0

WARN is issued; the skill is flagged as a precompute candidate. If an author follows the advisory and converts to:

!`git status || bash ./deploy.sh`

…bash ./deploy.sh executes on every skill load when git status exits non-zero (e.g., outside a git repo). Likewise, git log --oneline || curl https://hook.example.com/collect would be flagged as a precompute candidate, and converting it introduces a per-load egress call that violates the "no egress" requirement.

The rm form from prior security reviews (git status || rm -f file) is caught — rm appears in the mutation grep — so that specific case is safe. The remaining hole is interpreters and network commands not in the grep list.

Impact: Advisory WARN only; the author must manually act. But a careful author trusting check 18 on a line like git log --oneline || curl https://metrics.internal/... would introduce silent load-time egress. The SKILL.md advisory does say to verify before converting, but the WARN implies the block is read-only, which is incorrect for these patterns.

Narrowest fix — extend the mutation grep at line 443 to include shell interpreters and common network/execution tools:

grep -qE '(^|[[:space:]])(-exec|-execdir|-ok|-okdir|-delete|-fprintf|-fprint|-fls|--output|rm|mv|cp|tee|sed|mkdir|touch|bash|sh|python[23]?|node|ruby|perl|curl|wget|nc|ssh|xargs)([[:space:]=]|$)' <<<"$line" && return 1

Or, stricter — validate what follows || rather than stripping blindly, rejecting any continuation that is not echo or printf:

# reject any || <other-than-echo/printf> form
case "$line" in
  *'||'*)
    local after_or="${line#*||}"
    after_or="${after_or#"${after_or%%[![:space:]]*}"}"  # ltrim
    case "$after_or" in echo\ * | printf\ * | echo | printf) ;; *) return 1 ;; esac
    ;;
esac

---

What is confirmed fixed since prior reviews

Prior finding	Fixed in	Verification
Mutating git subcommands (git stash, git clean -fd)	4330015	Allowlist only; unlisted → return 1
run blocked gh run view/list	4330015	list in read-verb allowlist
Non-shell fence type test gap	4330015	is_shell=0 for json etc.
Mutating pipeline after read-only head (| tee)	b012f7e	`
&& / ; continuations	0da65d5	Line 429: *'&'* catches &&; *';'* catches ;
$(...) command substitution	0da65d5	Line 433: *'$('* catches $(...)
Backtick substitution	0da65d5	Line 434: *'\'*` catches backticks
find -exec / -delete / -fprint	0da65d5	Line 443 mutation grep
git diff --output=file	0da65d5	Line 443: --output in grep, [[:space:]=] suffix
env CMD classified read-only	0da65d5	env dropped from pure-reader allowlist at line 447

What I checked

Shell injection via SKILL.md content — Lines enter the loop via IFS= read -r bl and are consumed only by case, ${…} parameter expansions, [[ … =~ … ]] (content is left operand), and grep … <<<"$line" here-strings. File content is never eval'd. Safe.

fence_open_re regex — hardcoded literal '^(```+)([^]*)$'`, not derived from file content. No catastrophic backtracking. Safe.

$SKILL_MD path handling — all uses are double-quoted; no word-splitting or glob expansion. Safe.

warn output (line 528) — single-quoted literal; no file content is interpolated. Safe.

check-skill.test.sh fixtures — fixture bodies are single-quoted string literals; backticks and $() inside are never expanded by the shell and are written verbatim to temp files. The scanner's own metacharacter checks correctly classify them when read. Safe.

precompute-context.md, SKILL.md — pure documentation; || echo "<fallback>" convention is sound. Safe.

plugin.json (both plugins) — version bumps only; no new permissions, MCP servers, or egress declarations. Safe.

GitHub Actions — no workflow files changed. Safe.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0da65d5035

ℹ️ 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 finished @kyle-sexton's task in 3m 53s —— View job

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context and understand the request
• Read latest commit (d727bc1 — ext-diff/textconv screening fix)
• Review check-skill.sh check 18 block — full current state
• Verify all prior open findings resolved
• Review check-skill.test.sh fixture coverage
• Post final review findings

---

Summary

d727bc1 is a minimal, correct fix for the Codex P2 on git diff --ext-diff/--textconv. The change is in the right place, the added comment correctly acknowledges the inherent static-scan limit for diff.external/textconv on configured paths, and fixture 18n directly covers the case. 32 total assertions.

One prior IMPORTANT security finding from the 0da65d5 review is still open and unaddressed in d727bc1. All other prior P2/IMPORTANT/blocking findings are fully resolved.

---

Prior findings status

Finding	Source	Status
Mutating git subcommands (git stash, git clean)	Codex P2 (a23efaf)	✅ 4330015
run token blocked gh run view/list	Claude review	✅ 4330015
Non-shell fenced block test gap	Claude review	✅ 4330015 (fixture 18f)
Mutating pipeline after read-only head (| tee)	Codex P2 (4330015)	✅ b012f7e
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 0da65d5
$(...) command substitution not screened	Codex P2 (9e23619)	✅ 0da65d5
env CMD classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
find -exec/-delete classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
git diff --output=file classified read-only	Codex P2 (9e23619)	✅ 0da65d5
git diff --ext-diff/--textconv runs helper	Codex P2 (0da65d5)	✅ d727bc1
Whole-file ! gate triggered by prose	Claude review	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review	✅ Confirmed valid
|| bash ./deploy.sh classified read-only	Security IMPORTANT (0da65d5)	⚠️ Still open

---

Open finding — non-blocking

git status \|\| bash ./deploy.sh classified as a precompute candidate (shell interpreters and network tools absent from mutation grep)

check-skill.sh, line 443

The mutation grep screens for write-side tokens (rm, mv, cp, tee, sed, mkdir, touch, -exec*, --output, --ext-diff, --textconv) but omits shell interpreters (bash, sh, python3, node) and network tools (curl, wget, nc, ssh). Combined with the ||-stripping step, a line like:

git status || bash ./deploy.sh

traces through precompute_readonly_line as follows:

1. No metacharacter (>, <, ;, &) — || contains only |, not & → passes
2. No $( or backtick → passes
3. no_or = "git status bash ./deploy.sh" — || stripped; no bare | remains → passes
4. Mutation grep against original $line: bash absent from the token list → no match → passes
5. cmd = git, sub = status → allowlisted → return 0 — WARN issued

An author who follows the advisory and converts to !git status || bash ./deploy.sh introduces a shell invocation that runs on every skill load when git status exits non-zero (e.g., outside a git repo). The same gap exists for || curl, || wget, || sh, || python3, || node.

Bounding factors (why non-blocking):

• Check 18 is advisory only — a WARN, never a FAIL; the author must manually act
• The CHANGELOG explicitly documents || echo "<fallback>" as "the one preserved continuation"; the WARN text asks the author to "judge" whether to convert
• The precompute-context.md guidance requires || echo "<fallback>" as the mandatory defensive form — a careful author referencing it would see the mismatch
• Practical frequency of || bash / || curl in SKILL.md files is low

Recommended closure (pick one):

1. Fix — extend the mutation grep to include common interpreters and network tools:

   grep -qE '(^|[[:space:]])(-exec|-execdir|-ok|-okdir|-delete|-fprintf|-fprint|-fls|--output|--ext-diff|--textconv|rm|mv|cp|tee|sed|mkdir|touch|bash|sh|python[23]?|node|ruby|perl|curl|wget|nc|ssh|xargs)([[:space:]=]|$)' <<<"$line" && return 1

2. Document — add a Gotchas bullet to the check SKILL.md: "|| <interpreter-or-network-tool> after an allowlisted git/gh subcommand (e.g. || bash, || curl) is not screened by the mutation grep and is classified as a read-only candidate. Only the || echo/|| printf fallback forms are safe; other || continuations should be excluded manually before following the WARN."

Fix this →

---

Positive notes

• d727bc1 is minimal and correct. One-line grep extension + inline comment + one fixture. Exactly the right scope for a targeted fix.
• Code comment on the inherent limit (diff.external/textconv on configured paths is beyond a static line scan) is good self-documenting practice — a future maintainer won't re-open the finding thinking it was missed.
• 32 fixtures (18a–18n) cover all primary code paths: the full metacharacter gate, the command/process substitution gate, the pipe/|| disambiguation, every side-effecting option category, and now the external-diff/textconv option.
• Allowlist architecture is provably safe for unlisted paths. Any git subcommand or gh verb not explicitly on the allowlist lands at *) return 1 — a safe miss, never a false-safe. Adding new git subcommands in the future can only expand the true-positive rate, never introduce new false-safes.
• CHANGELOG entry for 0.7.0 correctly lists --ext-diff/--textconv alongside --output as disqualified options.

---

Verdict

Near-ready to merge. The five-commit progression is a well-documented, defensively-correct implementation of an advisory heuristic. All Codex P2 findings are resolved. The one remaining open finding (|| <interpreter>) is non-blocking given check 18's advisory-only contract and the documentation that pins || echo as the only sanctioned continuation — but closing it via either the grep extension or a Gotchas bullet before merge would make the implementation complete relative to all raised security concerns.

[claude]

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

---

Security Review — PR #773 (commit d727bc1)

• Gather PR diff and identify changed files
• Audit check-skill.sh check 18 block — focus on the latest commit's --ext-diff/--textconv fix and any unresolved prior findings
• Audit check-skill.test.sh for unsafe test patterns
• Audit documentation and plugin manifests
• Post security findings

---

One finding below. The --ext-diff/--textconv fix introduced by d727bc1 is correct. One prior IMPORTANT finding from the security review of 0da65d5 remains unresolved.

---

Findings

IMPORTANT — confidence: HIGH

|| <interpreter-or-network-tool> after an allowlisted git/gh subcommand is classified read-only

check-skill.sh, line 437–447

This finding was raised in the security review of 0da65d5 and is not addressed by d727bc1. The ||-strip at line 437 removes every occurrence of || before the pipe gate. The mutation grep at line 447 screens the original $line for specific token names (rm, mv, cp, tee, sed, mkdir, touch, -exec, --output, --ext-diff, --textconv) but does not include shell interpreters (bash, sh, python3, node, ruby, perl) or network/execution tools (curl, wget, nc, ssh, xargs).

Tracing git log --oneline || curl https://metrics.internal/collect through precompute_readonly_line:

1. L429: no >, <, ;, & (note || contains |, not &) → passes
2. L433–434: no $( or backtick → passes
3. L437: no_or = "git log --oneline curl https://metrics.internal/collect" (|| stripped)
4. L438: no | in no_or → passes
5. L447 grep against original $line: curl is absent from the token list → grep returns non-zero; && return 1 does not fire
6. cmd = git, sub = log → in allowlist → return 0

PRECOMPUTE_CANDIDATE=1 is set and the WARN fires, advising the author to convert the block to ! injection. An author who follows that advice and converts to !`git log --oneline || curl https://metrics.internal/collect` introduces a load-time egress call that fires on every skill invocation where git log exits non-zero — violating the "no egress" requirement. The same gap applies to || bash ./deploy.sh, || sh -c "...", || python3 ./mutate.py, || xargs rm, etc.

The rm form from the original finding (git status || rm -f file) is correctly caught because rm appears in the mutation grep.

Narrowest fix — validate what follows || rather than stripping blindly. Reject any || continuation whose first token is not echo or printf:

# Inside precompute_readonly_line(), before the existing `local no_or` block:
case "$line" in
  *'||'*)
    local after_or="${line#*||}"
    after_or="${after_or#"${after_or%%[![:space:]]*}"}"   # ltrim
    case "$after_or" in echo\ * | printf\ * | echo | printf) ;; *) return 1 ;; esac
    ;;
esac

This directly enforces the documented convention (|| echo "<fallback>") rather than relying on an ever-growing token denylist, and removes the need for the no_or strip and downstream pipe test for the || case entirely.

---

What the latest commit (d727bc1) adds — confirmed correct

--ext-diff and --textconv added to the mutation grep — The two new tokens are correctly placed in the side-effecting-options grep at line 447, which runs against the original $line. A SKILL.md line like git diff --ext-diff is now disqualified before reaching the subcommand allowlist. The code comment accurately records the inherent limit: repo diff.external / textconv config attached to a plain git diff (without the explicit flags) cannot be detected by a static line scan; that gap exists in git's design, not in the checker.

Fixture 18n correctly asserts that git diff --ext-diff inside a shell block does not trigger the WARN.

---

What I checked

Shell injection via SKILL.md content — SKILL.md lines enter the loop via IFS= read -r bl and are consumed only by case pattern matching, ${…} parameter expansions, [[ … =~ … ]] (content is the left operand, fence_open_re is the hardcoded literal right operand), and grep -qE … <<<"$line" here-strings. File content is never eval'd. Safe.

$SKILL_MD path handling — all grep calls and the done <"$SKILL_MD" redirect use double-quoted paths; no word-splitting or glob expansion. Safe.

warn output — the emitted WARN string is a hard-coded single-quoted literal; no file content is interpolated into the output. Safe.

check-skill.test.sh — fixture bodies are single-quoted string literals; backticks, !, $() are content, never expanded. Written to a mktemp directory. No execution of fixture content. Safe.

precompute-context.md — pure documentation. The || echo "<fallback>" convention is a sound defensive posture. The disableSkillShellExecution mention improves posture. No executable content. Safe.

plugin.json (both plugins) — version bumps only; no new permissions, MCP servers, egress declarations, or execution surfaces. Safe.

GitHub Actions — no workflow files changed. Safe.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d727bc1922

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

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

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

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context and understand the request
• Read latest commit (1ffc20c — restrict || to echo, screen find -fprint0)
• Verify remaining open findings from prior reviews are resolved
• Review check 18 full current state in check-skill.sh
• Review test coverage in check-skill.test.sh
• Post final review findings

---

Summary

1ffc20c is a minimal, correct fix for the two remaining Codex P2 / Claude IMPORTANT findings carried from the d727bc1 review. All prior open findings are now resolved. No blocking issues. One minor non-blocking observation below.

---

Prior findings status — all clear

Finding	Source	Status
Mutating git subcommands (git stash, git clean)	Codex P2 (a23efaf)	✅ 4330015
run token blocked gh run view/list	Claude review	✅ 4330015
Non-shell fenced block test gap	Claude review	✅ 4330015 (fixture 18f)
Mutating pipeline after read-only head (| tee)	Codex P2 (4330015)	✅ b012f7e
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 0da65d5
$(...) command substitution not screened	Codex P2 (9e23619)	✅ 0da65d5
env CMD classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
find -exec/-delete classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
git diff --output=file classified read-only	Codex P2 (9e23619)	✅ 0da65d5
git diff --ext-diff/--textconv runs helper	Codex P2 (0da65d5)	✅ d727bc1
|| bash ./deploy.sh classified read-only	IMPORTANT (0da65d5, d727bc1)	✅ 1ffc20c
find -fprint0 file-output primary not screened	Codex P2 (d727bc1)	✅ 1ffc20c
Whole-file ! gate triggered by prose	Claude review	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review	✅ Confirmed valid

---

Latest commit (1ffc20c) — verified correct

Fix 1: || echo-only strip via sed

check-skill.sh L439–441

sanitized="$(sed -E 's/\|\|[[:space:]]*echo([[:space:]]|$)/ /g' <<<"$line")"
case "$sanitized" in *'|'*) return 1 ;; *) ;; esac

Key paths traced:

• git status --short || echo "(status unavailable)" → sed strips || echo → no | in sanitized → passes → git status allowlisted → WARN issued ✓
• git status --short || bash ./recover.sh → sed sees || bash, not || echo → no strip → || leaves | in sanitized → *'|'* → return 1 ✓
• git status --short | tee snap.txt → no || to strip → | survives → return 1 ✓
• git log || echo "fallback" || git push → sed strips first || echo → second || git push leaves | → return 1 ✓

The sed call processes $line via here-string (not a command-line argument), so no shell injection surface.

Fix 2: -fprint0 added to mutation grep

check-skill.sh L450

...-fprintf|-fprint0|-fprint|-fls|...

Previously, -fprint0 slipped through because the token delimiter ([[:space:]=]|$) required space, =, or end-of-string — and 0 is none of those, so -fprint did not match the 0 suffix. Adding -fprint0 explicitly (placed before -fprint for clarity) closes the gap.

Test fixtures 18o and 18p correctly assert opposite outcomes: || bash ./recover.sh → no WARN; find -fprint0 → no WARN. 34 assertion pairs across the full file; all pass per the commit message (34 is consistent with the 69 pass/fail branch lines, ~2 per test).

---

Observation — || printf now fails closed (non-blocking)

The prior wholesale ||-strip allowed any || continuation to pass. The new sed strips only || echo. A SKILL.md line like git log --oneline || printf "%s\n" "fallback" now fails closed (the || before printf survives the sed → pipe gate catches it → return 1).

This is the correct behavior per the documented convention — || echo "<fallback>" is the one sanctioned form per both precompute-context.md and the 0.7.0 CHANGELOG — but it's a behavior narrowing not called out in the commit message or CHANGELOG. A skill author using || printf would get a safe-miss (no WARN) rather than the prior WARN; both are safe outcomes. Non-blocking.

---

Positive notes

• Precise strip vs. wholesale strip: replacing ${line//||/} with a sed that only strips || echo [...] is architecturally cleaner — the sanctioned form is explicitly encoded rather than implied by the absence of a | after stripping.
• || echo at end of line: the $ alternative in ([[:space:]]|$) correctly handles the bare || echo case (no argument), which the prior check also passed through. No regression.
• -fprint0 ordering: placing it before -fprint in the grep alternation makes the precedence explicit; the ERE alternation tries left-to-right, so -fprint0 can't accidentally be consumed by a later pattern. (In this regex the word-boundary delimiters mean order doesn't actually matter, but the explicit listing is clearer.)
• Invariant preserved: the fail-closed default still holds — any unlisted git subcommand, unlisted gh verb, unlisted pure-reader command, or shell operator outside the screened set lands at return 1.
• 34 fixtures (18a–18p): comprehensive coverage of all code paths added across the six fix commits.

---

Verdict

Ready to merge. The six-commit progression addresses every raised P2/IMPORTANT finding across five prior review passes. The implementation is defensively correct, consistently biased toward safe misses, and the playbooks spoke is clean. The one remaining observation (|| printf narrowing) is both intentional and in the safe direction.

[claude]

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

---

Security Review — PR #773 (commit 1ffc20c)

• Gather context and understand the request
• Fetch PR diff and identify changed files
• Audit check-skill.sh check 18 block — || restriction and -fprint0 fix
• Audit check-skill.test.sh for unsafe test patterns
• Audit documentation and plugin manifests
• Post security findings

---

No security vulnerabilities found. Both P2 findings from the d727bc1 security review are fully closed. Detailed trace-throughs follow.

---

Findings

None.

---

What I checked

|| restriction — correct (commit 1ffc20c, check-skill.sh lines 435–441)

The prior IMPORTANT finding: the broad ${line//||/} strip removed any || including || bash ./deploy.sh, so git status || bash ./deploy.sh was classified read-only. The fix replaces it with:

sanitized="$(sed -E 's/\|\|[[:space:]]*echo([[:space:]]|$)/ /g' <<<"$line")"
case "$sanitized" in *'|'*) return 1 ;; *) ;; esac

Traced git status || bash ./recover.sh:

1. Line 429: || is not >, <, ;, & → passes
2. Line 433–434: no $(, no backtick → passes
3. Line 440: sed pattern \|\|[[:space:]]*echo([[:space:]]|$) does not match || bash → sanitized = git status || bash ./recover.sh (unchanged)
4. Line 441: sanitized contains | → return 1 ✓

The prior IMPORTANT finding is resolved. The || in || bash recover.sh is now disqualifying, not silently consumed.

No injection surface in the sed invocation: the pattern is a single-quoted literal; $line is stdin via here-string, never on the command line. No content from the analyzed file reaches the sed command name or pattern argument.

Edge cases verified:

• git status || echo "msg" → sed strips || echo → sanitized has no | → WARN fires correctly ✓
• git status || echo "msg" | tee log → sed strips || echo but | tee log tail survives → | in sanitized → return 1 ✓
• git status || echo "a" || bash x → sed strips first || echo , second || is || bash → | survives → return 1 ✓
• git status || echo "pipe | test" → | inside the echo argument causes return 1 (safe miss, not a false safe) ✓
• Metacharacter checks (line 429: &, line 433: $(, line 434: backtick) run before the sed step; git status || echo "$(id)" is caught at line 433 before sanitization ✓

|| printf forms: now disqualified as candidates (the || is not stripped, leaving | → return 1). This is a safe miss (advisory suppressed for a benign line), not a false safe. The guidance documents || echo as the only sanctioned form, so the implementation and documentation are aligned.

sed subprocess availability: if sed were absent (impossible on any POSIX system), sanitized would be empty; case "" in *'|'*) return 1 would not fire, and the flow would continue to the cmd/sub allowlist checks. Worst case: an advisory WARN on a line that's actually not safe. Not a security issue given the advisory-only contract.

-fprint0 addition — correct (commit 1ffc20c, check-skill.sh line 450)

Prior P2: -fprint0 slipped past the -fprint token because the delimiter ([[:space:]=]|$) doesn't include 0, so -fprint followed by 0file.txt didn't match.

New pattern:

(-exec|-execdir|-ok|-okdir|-delete|-fprintf|-fprint0|-fprint|-fls|--output|--ext-diff|--textconv|rm|mv|cp|tee|sed|mkdir|touch)

Traced find . -name "*.md" -fprint0 files.txt:

• -fprint0 token + (space delimiter) → match → grep -qE ... && return 1 ✓

Ordering: -fprint0 is listed before -fprint. For input -fprint0 file.txt, the ERE engine would try -fprintf (no match), then -fprint0 (match) → correct. Even if tried in reverse order, -fprint against -fprint0 would require the 0 to satisfy ([[:space:]=]|$) which it cannot, so -fprint0 would only be caught by the explicit -fprint0 entry — and that entry is now present. ✓

grep receives SKILL.md content via here-string (<<<"$line"), not on the command line. No injection surface. Pattern is a hardcoded single-quoted string. ✓

check-skill.test.sh — fixture safety

Both new fixtures (18o, 18p) write their SKILL.md bodies as single-quoted string literals to temp files via make_skill. Content inside — backticks, !, $(...) — is literal text in the test fixture, never expanded by the test harness shell. The scanner's own metacharacter checks correctly classify those literals when reading the temp file. No unsafe test patterns. ✓

Complete prior-findings table

Finding	Source	Status
Mutating git subcommands (git stash, git clean)	Codex P2 (a23efaf)	✅ 4330015
run blocked gh run view/list	Claude review	✅ 4330015
Non-shell fenced block test gap	Claude review	✅ 4330015
Mutating pipeline after read-only head (| tee)	Codex P2 (4330015)	✅ b012f7e
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 0da65d5 — *'&'* / *';'*
$() command substitution not screened	Codex P2 (9e23619)	✅ 0da65d5 — *'$('*
env CMD classified read-only	Codex P2 (b012f7e)	✅ 0da65d5 — env dropped from allowlist
find -exec/-delete/-fprint/-fls classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
git diff --output=file classified read-only	Codex P2 (9e23619)	✅ 0da65d5
git diff --ext-diff/--textconv runs helper	Codex P2 (0da65d5)	✅ d727bc1
|| bash ./deploy.sh classified read-only	Security IMPORTANT (d727bc1)	✅ 1ffc20c — only || echo is stripped
find -fprint0 word-boundary miss	Codex P2 (d727bc1)	✅ 1ffc20c — -fprint0 added explicitly
Whole-file ! gate triggered by prose	Claude review	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review	✅ Confirmed — used in docs-hygiene, code-tidying, firecrawl, playbooks/update

No open security findings remain. 34 assertions, shellcheck clean, gate passes.

---

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

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

---

Security Review — PR #773 (HEAD fe9c363)

• Gather PR diff and understand what changed in the latest push
• Audit changed files for security issues
• Verify status of prior security findings
• Post findings

---

No security vulnerabilities found.

The HEAD commit (fe9c363) is a merge from origin/main — it adds the new plugins/github plugin and plugins/source-control updates that are not part of this PR's scope. The last substantive change to this PR's code (check-skill.sh check 18, test fixtures, documentation) was 1ffc20c, which received a clean security review. The merge did not touch any of this PR's files.

---

What I checked

check-skill.sh — precompute_readonly_line and check 18 block (current state)

Shell injection via SKILL.md content. Every line from $SKILL_MD enters the loop via IFS= read -r bl and is consumed only by:

• case "$line" in … pattern matching — no execution
• ${line#…} / ${line%%…} parameter expansions — no execution
• [[ "$trimmed" =~ $fence_open_re ]] — $fence_open_re is a hardcoded literal; $trimmed is the left operand (string), not the regex — no injection surface
• grep -qE '…' <<<"$line" — here-string to grep; file content is never on the command line
• sed -E '...' <<<"$line" — sed pattern is a single-quoted literal; $line is stdin via here-string, never a command-line argument

No SKILL.md content is ever eval'd. Safe.

|| echo-only sed strip (L440). The pattern 's/\|\|[[:space:]]*echo([[:space:]]|$)/ /g' strips only || echo [arg]. Tracing git status || bash ./deploy.sh: || bash does not match || echo → sanitized retains ||, which contains | → case "$sanitized" in *'|'*) return 1. The IMPORTANT finding from the d727bc1 review (broad || stripping) is resolved. Safe.

Metacharacter gate (L428–434). >, <, ;, & (catching &&), $(, and backtick all disqualify the line before any allowlist check. Process substitution <( / >( caught by </>. Safe.

Mutation grep (L450). Covers -exec, -execdir, -ok, -okdir, -delete, -fprintf, -fprint0, -fprint, -fls, --output, --ext-diff, --textconv, rm, mv, cp, tee, sed, mkdir, touch. The delimiter ([[:space:]=]|$) correctly handles both --output=file and --output file forms. -fprint0 word-boundary fix (1ffc20c) is in place. Safe.

git/gh allowlist (L455–472). Conservative: unlisted git subcommand or gh verb → return 1. env was removed from the pure-reader allowlist. Write verbs for gh are checked first, before the read-verb allowlist. Safe.

$SKILL_MD path handling. SKILL_MD="$SKILLS_ROOT/$SKILL_NAME/SKILL.md" — the path always starts with an absolute root and is always double-quoted at every use site (grep … "$SKILL_MD", done <"$SKILL_MD"). No flag-injection surface. Safe.

warn output (L535). Single-quoted literal; no file content is interpolated. Safe.

check-skill.test.sh (fixtures 18a–18p). All fixture SKILL.md bodies are single-quoted string literals passed to make_skill and written to mktemp directories. Backticks, !, $(), and || inside are content, never expanded by the test shell. Safe.

precompute-context.md and SKILL.md changes. Pure documentation. The || echo "<fallback>" convention is the correct defensive posture. The disableSkillShellExecution kill-switch mention improves posture. No executable content. Safe.

plugin.json (both plugins). Version bumps only — no new permissions, MCP servers, egress declarations, or execution surfaces. Safe.

GitHub Actions. No workflow files changed in this PR. Safe.

---

Prior findings — all resolved

Finding	Source	Status
Mutating git subcommands (git stash, git clean)	Codex P2 (a23efaf)	✅ 4330015
run blocked gh run view/list	Claude review	✅ 4330015
Non-shell fenced block test gap	Claude review	✅ 4330015 (fixture 18f)
Mutating pipeline after read-only head (| tee)	Codex P2 (4330015)	✅ b012f7e
&&/; continuations not gated	Codex P2 (b012f7e)	✅ 0da65d5
$() command substitution not screened	Codex P2 (9e23619)	✅ 0da65d5
env CMD classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
find -exec/-delete/-fprint classified read-only	Codex P2 (b012f7e)	✅ 0da65d5
git diff --output=file classified read-only	Codex P2 (9e23619)	✅ 0da65d5
git diff --ext-diff/--textconv runs external helper	Codex P2 (0da65d5)	✅ d727bc1
|| bash ./deploy.sh classified read-only	Security IMPORTANT (d727bc1)	✅ 1ffc20c — only || echo is stripped
find -fprint0 word-boundary miss	Codex P2 (d727bc1)	✅ 1ffc20c — -fprint0 added explicitly
Whole-file ! gate triggered by prose	Claude review	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	Claude review	✅ Confirmed — used in multiple plugins

No open security findings.

[claude]

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

---

PR Review — feat(skill-quality,playbooks): precompute-opportunity check and authoring guidance

• Gather context and understand the request
• Review merge state and any new diff since last review pass
• Review current check-skill.sh check 18 block
• Verify all prior findings resolved
• Post final review

---

Post-merge verification

The trigger is the merge commit fe9c363 integrating origin/main into this branch. None of the PR-owned files changed in the merge. git diff 1ffc20c...HEAD -- check-skill.sh check-skill.test.sh precompute-context.md returns empty — the merge only brought in the new github plugin and unrelated fixes from main. No conflict-resolution artifacts.

The catalog (marketplace.json) was correctly regenerated: the new github entry from main was cleanly appended, and the skill-quality/playbooks entries are intact. Plugin versions are skill-quality@0.7.0 and playbooks@0.3.0, matching the changelogs.

---

Prior findings — all clear

All P2/IMPORTANT findings raised across six prior review passes are resolved. Summary table:

Finding	Status
Mutating git subcommands (git stash, git clean)	✅ 4330015 — allowlist
run token blocked gh run view/list	✅ 4330015 — allowlist admits list
Non-shell fenced block test gap	✅ 4330015 — fixture 18f
Mutating pipeline after read-only head (| tee)	✅ b012f7e — pipe gate
&&/; continuations not gated	✅ 0da65d5 — *'&'*/*';'*
$(...) command substitution	✅ 0da65d5 — *'$('*
env CMD classified read-only	✅ 0da65d5 — env dropped from allowlist
find -exec/-delete classified read-only	✅ 0da65d5 — grep pattern
git diff --output=file classified read-only	✅ 0da65d5 — --output in grep
git diff --ext-diff/--textconv runs helper	✅ d727bc1
|| bash ./deploy.sh classified read-only	✅ 1ffc20c — sed strips only || echo
find -fprint0 word-boundary miss	✅ 1ffc20c — -fprint0 added explicitly
Whole-file ! gate triggered by prose	✅ Documented in SKILL.md Gotchas
${CLAUDE_SKILL_DIR} validity	✅ Confirmed — in-use across 4+ plugins

---

Two minor observations (non-blocking, unchanged from last review)

1. git -C /path log safely misses

The subcommand extractor (${line#git} → ltrim → ${sub%%[[:space:]]*}) resolves sub to -C for git -C /repo log, which is not in the allowlist → return 1. This is the correct safe-miss direction (missed opportunity, never advice to run a mutation). git -C prefixed invocations are uncommon in SKILL.md bodies.

2. gh issue list --search "fix comment styling" safely misses

The write-verb pattern *" comment "* matches the word comment inside the search value → return 1. A false-negative (valid candidate disqualified) on the safe side. The Gotchas section already covers the general under-reporting contract.

Both observations land in the documented safe-miss direction, consistent with the advisory-only contract.

---

Positive notes

• || echo-only sed strip (1ffc20c): replacing the broad ${line//||/} with a targeted sed that strips only the sanctioned || echo [...] form is architecturally correct — the convention is explicitly encoded rather than implied.
• 34 test fixtures (18a–18p): comprehensive coverage of all code paths; assertion messages are specific and diagnostic.
• Fail-closed architecture: unlisted git subcommand, unlisted gh verb, unlisted pure-reader command, or unrecognized shell construct → return 1. New cases introduced after this PR lands on the safe side without code changes.
• precompute-context.md: clean locally-owned spoke, "recheck trigger" convention is good practice, and the mandatory defensive fallback is correctly scoped to undocumented upstream semantics.
• Repo-agnostic WARN text: points at https://code.claude.com/docs/en/skills#inject-dynamic-context rather than another plugin.

---

Verdict

Ready to merge. The six-commit progression addresses every raised P2/IMPORTANT finding across five prior review passes. The merge is clean. The two remaining minor observations are both in the safe-miss direction and within the documented "advisory heuristic, not a defect" contract.