Skip to content

detect Pattern 4 injection-guard callout; fix four pr-management skills#220

Merged
potiuk merged 3 commits into
apache:mainfrom
justinmclean:injection-guard
May 19, 2026
Merged

detect Pattern 4 injection-guard callout; fix four pr-management skills#220
potiuk merged 3 commits into
apache:mainfrom
justinmclean:injection-guard

Conversation

@justinmclean

Copy link
Copy Markdown
Member

What

Adds a validate_injection_guard check to tools/skill-validator
(Pattern 4 from write-skill/security-checklist.md) and retrofits the
four pr-management-* skills that were missing the required callout.

Why

security-checklist.md states the callout is required on every skill
that reads external content, and init_skill.py already scaffolds a
placeholder for it — but nothing in the automated toolchain caught skills
that dropped the placeholder without adding the real block. The four
PR-management skills each read attacker-controlled content (PR titles,
bodies, diffs, review comments, issue threads) and relied on an inline
"golden rule" paraphrase instead of the standard callout. That paraphrase
was invisible to any tooling and easy to miss in review.

Changes

tools/skill-validator/src/skill_validator/__init__.py

  • New constants: INJECTION_GUARD_CATEGORY (hard),
    INJECTION_GUARD_TODO_CATEGORY (soft, added to SOFT_CATEGORIES),
    INJECTION_GUARD_CALLOUT_SENTINEL, INJECTION_GUARD_TODO_SENTINEL,
    EXTERNAL_SURFACE_SIGNALS.
  • New helpers _strip_html_comments and _skill_body — strip the
    scaffolded <!-- TODO … --> comment before scanning for signals, so
    the placeholder's own example list ("Gmail, public PRs, scanner
    findings") doesn't self-trigger.
  • New function validate_injection_guard wired into run_validation
    for every SKILL.md. Two violation classes:
    • HARD (injection_guard) — skill body signals external-surface
      reads but the callout phrase is absent and the scaffold TODO has
      been deleted.
    • SOFT (injection_guard_todo) — the init_skill.py
      <!-- TODO — INJECTION-GUARD CALLOUT placeholder is still present;
      suppresses the hard check (skill is mid-development).
  • Updated module docstring and _SOFT_RULE_PREFIXES.

Signal design — seven patterns, kept specific to avoid false positives
on skills that document what to do with external content rather than
reading it themselves (write-skill mentions Gmail in prose;
setup-shared-config-sync drafts commit messages — neither fires):

Signal Rationale
gh pr view/diff/list Unambiguous workflow fetch
gh issue view Unambiguous workflow fetch
ponymail, mbox Specific enough; no doc-only context
gmail.googleapis | Gmail MCP | Gmail API Requires specificity; bare gmail rejected
scanner-finding Tight string
golden/hard rule … external content … data/never an instruction Self-declaration about this skill's own behavior — strongest signal

tools/skill-validator/tests/test_validator.py

Fourteen new tests in TestValidateInjectionGuard covering clean skills,
each individual signal, callout-inside-HTML-comment not counted,
TODO-suppresses-HARD, signal-inside-HTML-comment ignored, and category
exposure. TestSoftCategories updated to assert the new soft category.

Four pr-management-* SKILL.md files

Pattern 4 callout inserted just before the --- / ## Adopter overrides
preamble in each, with a surface list specific to what that skill reads:

Skill Surfaces named in callout
pr-management-triage public PR titles, bodies, commit messages, author profiles
pr-management-code-review public PR titles, bodies, diff lines, commit messages, code comments, inline review comments
pr-management-mentor GitHub issue and PR thread titles, bodies, comments
pr-management-stats public PR titles, labels, GitHub-provided metadata

Verification

  • run_validation() returns zero hard violations and zero soft warnings
    against the full repo.
  • All 384 skill-eval cases load and template-render cleanly (--quiet
    sweep of the entire tools/skill-evals/evals/ tree).
  • The four patched SKILL.md files confirmed to contain the callout
    sentinel.
  • write-skill and setup-shared-config-sync confirmed to produce zero
    false-positive hard violations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants