ci: add Tier-2 LLM skill security review and address initial audit findings

[yarikoptic]

Summary

Adds a Tier-2 LLM-based security review pipeline for all skills, then applies
the fixes surfaced by running it.

Infrastructure: skill-security-review/SKILL.md is a new skill that audits
any other skill for prompt injection, capability mismatch, exfiltration risk,
unsafe subprocess use, hardcoded secrets/paths, and supply-chain issues.
ci/security_review.py drives it headlessly against every skill directory and
collects structured VERDICT reports. Two new tox environments expose it:

Command	What it does	Needs
tox -e security-static	cisco-ai-skill-scanner YARA/AST/dataflow scan; fails on HIGH+	nothing
tox -e security-llm	LLM review via claude -p; fails on CRITICAL/HIGH	Claude Code
SECURITY_CMD="yolo --worktree=skip --" tox -e security-llm	Same, via Podman wrapper	yolo + Claude

The prompt is embedded inline in security_review.py rather than loaded via
--plugin-dir, so it works identically with both claude and yolo (the
Podman wrapper does not mount arbitrary host paths inside the container).

Findings fixed: the first tox -e security-llm run found 5 HIGH, 7 MEDIUM,
and 3 LOW findings across 13 skills. All HIGH and most MEDIUM findings are
addressed in commit f3e92ea.

Finding summary (first security-review.out)

Skill	Verdict	Key findings addressed
analyze-duplicates	HIGH	Untrusted-output note for scanned source files (Step 5); credential strip in remote URL; remove unused Agent from allowed-tools
introduce-git-bug	HIGH	Untrusted-output note on git bug ls output (issue titles/bodies are externally authored)
pr-review-update	HIGH	Add missing allowed-tools; reinforce <untrusted-output> at Step 3 and output template
scan-projects	HIGH	Add missing allowed-tools; untrusted-output note before Phase 2 README/source reads
tinuous-analyzer	HIGH	Add missing allowed-tools; untrusted-output reminder at Step 4
credit-contributions	MEDIUM	Untrusted-output note for git commit subjects used in CRediT role inference
introduce-codespell	MEDIUM	Untrusted-output note for codespell output and git diffs (Steps 5, 7, 10)
introduce-reuse-compliance	MEDIUM	Untrusted-output note before CONTRIBUTING.md/README ingestion at Step 1
make-scriv-changelog	MEDIUM	Add missing allowed-tools, name, user-invocable; untrusted note on git log/diff
pr-feedback-review	MEDIUM	Replace -f body="…" shell injection with jq/Python --input -; externalize GITHUB_USER
issue-triage	MEDIUM	Remove unused WebFetch/WebSearch from allowed-tools
introduce-intuit-auto	LOW	Remove unused WebFetch from allowed-tools
bisect-and-patch-git-annex	LOW	No change (unrestricted Bash intentional; container image hint is advisory)

Test plan

• tox -e validate — zero errors after all changes
• tox -e security-static — passes on this branch
• SECURITY_CMD="yolo --worktree=skip --" tox -e security-llm — re-run LLM review; former HIGH findings expected to drop to ≤MEDIUM
• CI validate job passes on this PR

[yarikoptic]

here I wonder what to do about this -- likely should explicitly state having the config?

[yarikoptic]

Reviewed this with two independent domain reviews (LLM/agent-security focus and Python/CI code-quality-and-reuse focus), then cross-checked their findings against each other before posting. Both converged on the same bottom line: this would warrant REQUEST_CHANGES from an independent reviewer (GitHub won't let me self-request-changes on my own PR, so posting as a comment review instead).

Why REQUEST_CHANGES-equivalent

The core work here — a new skill-security-review skill, the ci/security_review.py driver, two new tox environments, and ~13 skill files gaining missing allowed-tools/untrusted-output framing — is a genuine, welcome improvement, and several of the fixes (the pr-feedback-review shell-injection fix, the credential-stripping regex in generate-report.py, the missing-frontmatter additions) are correct and valuable as-is.

But there's one structural issue in the new tooling itself that both reviews agreed is worth blocking on (inline comment on ci/security_review.py:51): skill-security-review/SKILL.md declares a narrow, read-only allowed-tools posture appropriate for auditing untrusted content, but ci/security_review.py actually invokes it via claude --dangerously-skip-permissions with cwd set to the skill directory being audited — which disables that permission gating entirely. Since the explicit purpose of this tool is to vet unvetted/adversarial skill submissions (that's not yet wired into CI, but it's exactly the manual workflow a maintainer would run against an incoming PR), this is a live foot-gun: a malicious skill under review could hijack the fully-privileged reviewing process, including exfiltrating the ANTHROPIC_API_KEY that tox.ini's new passenv makes available to it, or simply spoofing a clean verdict (the VERDICT: extraction is an unvalidated regex match).

Alongside that, both reviews independently flagged the same two reuse issues (see inline comments):

• ci/security_review.py's find_skill_dirs() duplicates ci/validate_skills.py's version almost verbatim.
• ci/security_review.py's _REVIEW_PROMPT hand-duplicates skill-security-review/SKILL.md's ~90-line body, and the two have already drifted within this PR (SKILL.md's check 6 has a sentence the script's copy lacks).

I'd suggest addressing these three before merge — the first is a real safety-boundary defect in a security tool, the other two are just reuse cleanup with an easy fix (see inline suggestions).

Non-blocking, worth a look

• introduce-intuit-auto/SKILL.md's workflow_dispatch rewrite is ~90 of that file's 102 changed lines and reads as unrelated feature/docs work bundled into a PR titled/scoped as a security audit. Not a defect, but worth asking whether it belongs in its own PR for cleaner history.
• Three placement/consistency nits posted inline (issue-triage's unauthenticated-server warning appearing after the launch/expose instructions instead of before; credit-contributions's note appearing after the risky commands instead of before, unlike sibling files in the same PR; pr-feedback-review's new heredoc fix uses a fixed delimiter that adversary-influenced reply text could still collide with).
• tox.ini's new passenv lists HOME/PATH, which tox already passes through by default — harmless, just noise.
• The 13 independently-worded "Security note"/"Untrusted data reminder" blocks are reasonable for a docs-only repo, but since this PR is what establishes the pattern 13 times over, it'd be a cheap win to add one canonical wording to CLAUDE.md/AGENTS.md's "Other repo-wide conventions" placeholder so future skills copy one phrasing instead of a 14th variant.

Happy to take another pass once the permission-model and dedup items are addressed.

---

Generated by Claude Code

[yarikoptic]

Blocking: the auditor's own read-only posture isn't actually enforced here.

skill-security-review/SKILL.md's frontmatter declares a narrow, read-only allowed-tools: Read, Glob, Grep, Bash(find:*), Bash(head:*), Bash(wc:*) — the right posture for auditing untrusted content. But _DEFAULT_CMD bypasses that entirely: review_skill() (line 196) runs this via subprocess.run(..., cwd=skill_dir, ...), i.e. a fully-privileged claude process (--dangerously-skip-permissions disables all permission gating) with its cwd set to the very skill directory under audit.

Since this tool's whole purpose is to review unvetted/adversarial skill submissions, a malicious SKILL.md or bundled script can embed a prompt-injection payload that this unrestricted process will simply execute — including reading ANTHROPIC_API_KEY (passed through via tox.ini's new passenv) and exfiltrating it, or spoofing a clean verdict outright (the VERDICT: extraction below is a bare re.search, line 207-ish, with no provenance check). This isn't reachable from CI yet (confirmed — no workflow wires security-llm), but it's exactly the local workflow a maintainer would use to vet an incoming PR's skill, so the exposure is real today, not hypothetical.

Suggested fix: don't use --dangerously-skip-permissions. Use an explicit read-only tool allowlist matching the declared frontmatter instead, e.g.:

_DEFAULT_CMD = (
    "claude --allowedTools "
    "Read,Glob,Grep,Bash(find:*),Bash(head:*),Bash(wc:*)"
)

(or the equivalent for the yolo launcher). If some environments genuinely can't express a scoped allowlist, at minimum add a loud, explicit warning in both this file and skill-security-review/SKILL.md that it must never be pointed at untrusted content without process isolation (no secrets, no network) until this is fixed.

---

Generated by Claude Code

[yarikoptic]

Should-fix: duplicates ci/validate_skills.py's find_skill_dirs() almost verbatim (only the exclusion set differs: {"ci"} vs {"ci", "skill-security-review"}). Two copies of "what counts as a skill directory" will silently drift if one is ever updated without the other. Since ci/ is already sys.path[0] when this script is run directly, it can just import and filter:

Suggested change

	def find_skill_dirs(repo_root: Path) -> list[Path]:
	return sorted(
	p.parent
	for p in repo_root.glob("*/SKILL.md")
	if p.parent.name not in {"ci", "skill-security-review"}
	)
	def find_skill_dirs(repo_root: Path) -> list[Path]:
	from validate_skills import find_skill_dirs as _all_skill_dirs
	return [d for d in _all_skill_dirs(repo_root) if d.name != "skill-security-review"]

---

Generated by Claude Code

[yarikoptic]

Should-fix: hand-duplicates skill-security-review/SKILL.md's body (~90 lines) instead of loading it. The two have already drifted within this same PR — SKILL.md's check 6 has an extra sentence ("Already caught by tox -e validate (E003)...") that this copy is missing. The stated reason for embedding a prompt here ("works identically with both claude and yolo... no --plugin-dir needed") only requires not depending on named-skill resolution — it doesn't require duplicating the text. Read it from the one canonical file instead, e.g.:

def _review_prompt() -> str:
    text = (REPO_ROOT / "skill-security-review" / "SKILL.md").read_text(encoding="utf-8")
    _, _, body = text.partition("---\n")[2].partition("\n---\n")
    return body.strip() + "\n"

then full_cmd = cmd_base + ["-p", _review_prompt()]. That gives one source of truth for security-critical review criteria instead of two that can silently diverge.

---

Generated by Claude Code

[yarikoptic]

Nit: good fix overall (this closes a real shell-injection hole), but the fixed heredoc delimiter REPLY_BODY can still collide with reply content — if a drafted reply body (which can quote untrusted PR-comment text per earlier steps) contains a line that's exactly REPLY_BODY, the heredoc terminates early and the rest of the generated script is parsed as shell commands instead of data. Use a delimiter that can't appear in ordinary text (e.g. a per-run random token), or sidestep heredocs entirely by writing the reply to a temp file with the Write tool and reading it back in the python3 -c line — no shell quoting involved at all.

---

Generated by Claude Code

[yarikoptic]

Nit: this "no authentication" warning lands after the launch command (a few lines above this hunk) and the "binds to 0.0.0.0" sentence, and immediately before the "Container access" section that walks through publishing the port further. Move the > **Warning** block above the python3 ~/.claude/skills/issue-triage/server.py ... command so the risk is known before the server is started/exposed, not after.

---

Generated by Claude Code

[yarikoptic]

Nit: this new security note is placed after the git log/shortlog commands it's warning about (a few lines above this hunk), whereas the equivalent notes in introduce-git-bug/SKILL.md and analyze-duplicates/SKILL.md in this same PR are placed before the risky read. Move it above the bullet list for consistency and so the framing is in effect before the output is read.

---

Generated by Claude Code