docs: add integrity filtering, idempotency, trigger guide to gh-aw skill

[PureWeen]

Comprehensive update to the gh-aw-guide skill incorporating validated security patterns from field research.

New content (293→410 lines, +135/-17):

Security-Critical Patterns (expanded from 4 to 5 + 3 new subsections)

• Integrity filtering: full hierarchy (merged > approved > unapproved > none > blocked), interaction matrix with roles:, configuration with trusted-users/blocked-users/approval-labels
• Idempotency requirement: slash commands MUST be idempotent; lock-for-agent: true guidance
• Edited-comment time-bomb: issue_comment.edited fires TODAY with today's secrets on old comments
• Read-only contributor write surface: what read-only users can fire, defense priority list
• Triage role footgun: label_command: needs triage but default roles: excludes it
• Pre-cancellation race: already-running steps complete after cancel (concurrency ≠ idempotency)

Trigger Selection Guide (new section)

• ✅ Recommended / ⚠️ Use with Caution / ⛔ Avoid tiers
• Design principles: deterministic-by-default, limitations-are-security, least-privilege, signal-to-noise

All claims validated against official GitHub docs and the gh-aw integrity/triggers/concurrency references.

[github-actions]

Security Documentation Review — .claude/skills/gh-aw-guide/SKILL.md

I reviewed the full 412-line file for internal consistency, factual accuracy, and security implications. 6 findings below.

---

🔴 CRITICAL: min-integrity example directly contradicts its own warning (lines 254–273)

The YAML example shows min-integrity: approved hardcoded as a working config, but the warning 19 lines below says this crashes the MCP Gateway due to a compiler bug (v0.62.2). The old version was unambiguous: "Do NOT set min-integrity explicitly" with the broken example commented out. The new version presents the crashing config as the primary copy-paste example.

Failing scenario: Reader copies the YAML block → MCP Gateway crashes at startup.

Fix: Either comment out min-integrity: in the example (matching old approach), or confirm the bug is fixed in a specific version and remove the warning. Don't show a crashing config as a working example.

---

🟡 MODERATE: Cancellation signal sequence is factually incorrect (line 129)

The PR states: "GitHub sends SIGTERM, waits up to 7500ms, then SIGKILL."

Per GitHub's official cancellation reference, the actual sequence is: SIGINT → 7500ms → SIGTERM → 2500ms → kill process tree. The PR omits SIGINT and incorrectly claims SIGTERM is the first signal. Someone writing a graceful-shutdown handler based on this would trap SIGTERM and miss the initial SIGINT for 7500ms.

---

🟡 MODERATE: Mixed signals on roles: all safety (lines 228 vs 338)

Pattern #1 says "NEVER use 'all'..." but the ✅ Recommended trigger table says issues with "roles: all acceptable with tight safe-outputs." These should use consistent language — either soften the "NEVER" or add a cross-reference from the Recommended table.

---

🟡 MODERATE: roles: YAML placement may be incorrect (lines 223–228)

on:
  slash_command:
    name: review
    events: [pull_request_comment]
  roles: [admin, maintainer, write]  # ← sibling of slash_command under on:

If roles: is a top-level frontmatter key (as its description as a workflow-level ACL suggests), this indentation is wrong and will be silently ignored — leaving the workflow without role filtering (security regression). Verify correct placement in the gh-aw schema.

---

🟢 MINOR: Fork guard example could use ${{ }} wrapper (lines 209–215)

The guard example uses bare ! in an if: expression. While it doesn't start with !, showing the ${{ }} wrapped form as the primary example would be safer for copy-paste and consistent with the YAML gotcha warning that follows.

---

🟢 MINOR: Duplicate Fork PR checkout content (lines 188–199 vs 285–294)

Pattern #5 is nearly identical to the earlier "Fork PR Checkout (workflow_dispatch)" section — same YAML snippet, same script. Pre-existing duplication not introduced by this PR, but the renumbering is an opportunity to consolidate.

Generated by Expert Code Review (auto) for issue #754 · ● 9.4M · ◷

[github-actions]

Expert Code Review — PR #754

docs: add integrity filtering, idempotency, trigger guide to gh-aw skill

Methodology

3 independent reviewers conducted parallel deep reviews, followed by adversarial consensus with targeted follow-up verification on disputed findings. Findings required ≥2/3 agreement to be included.

Summary of Findings

#	Severity	Finding	Agreement
1	🔴 CRITICAL	min-integrity: approved shown as active config directly contradicts the compiler-bug crash warning that follows it — copy-paste hazard	3/3
2	🟡 MODERATE	roles: YAML indentation as sibling of slash_command: may silently disable access control	2/3
3	🟡 MODERATE	Fork guard YAML gotcha suggests non-standard parentheses fix; should use canonical ${{ }}	2/3
4	🟡 MODERATE	roles: all "acceptable" in Trigger Guide contradicts "NEVER" in Security Pattern #1	2/3
5	🟡 MODERATE	pull_request in ⛔ Avoid (unqualified) contradicts existing Noise Reduction section showing valid pull_request + paths: patterns	3/3
6	🟡 MODERATE	Cancellation signal sequence omits SIGINT and misstates timing — actual is SIGINT → 7500ms → SIGTERM → 2500ms → kill	3/3

Discarded Findings (single-reviewer only, no consensus)

• Edited-comment time-bomb mitigation missing for slash_command: (1/3)
• roles: auto-injection claim unverified (1/3)
• Fork guard only gates jobs.guard, not other jobs (1/3)

Overall Assessment

This is a high-quality, security-focused documentation update that adds significant value — the integrity filtering hierarchy, trigger selection guide, and edited-comment time-bomb warning are all important additions. The critical finding (Finding 1) should be addressed before merge: the min-integrity example currently inverts the safety guidance from the previous version, creating a copy-paste hazard that crashes the MCP Gateway. The remaining moderate findings are internal consistency issues that could mislead developers relying on this as a security reference.

CI / Review Status

• CI: Commit status pending (no status checks reported); activation and pre_activation checks passed
• Prior reviews: None
• Changed files: 1 file (.claude/skills/gh-aw-guide/SKILL.md, +135/-17)

Generated by Expert Code Review (auto) for issue #754 · ● 9.4M

[github-actions]

🔴 CRITICAL — min-integrity example contradicts its own crash warning (Flagged by: 3/3 reviewers)

This line shows min-integrity: approved as an uncommented, active configuration value with an inline comment presenting it as the default. However, the warning at line ~274 states that hardcoded min-integrity in source "emits an incomplete guard policy (missing repos field) that crashes the MCP Gateway."

The previous version of this file unambiguously marked this as ❌ BROKEN with the value commented out. The new version inverts the guidance — the example looks like the recommended configuration, with a softened footnote caveat appearing after the code block. Anyone copy-pasting this YAML gets a workflow that crashes the MCP Gateway at startup.

Suggested fix: Either comment out min-integrity: approved (as was done previously), or move the compiler-bug warning above the YAML block with a clear ❌ marker so the danger is visible before the copyable code.

[github-actions]

🟡 MODERATE — roles: YAML nesting level may silently disable access control (Flagged by: 2/3 reviewers)

roles: is indented as a sibling of slash_command: (a key under on:), not as a sub-field of slash_command:. If the gh-aw schema expects roles: under slash_command:, this placement would make it silently ignored — meaning the workflow has no access control while appearing to have it.

Concrete failure: A developer copies this pattern verbatim. If roles: is ignored at this nesting level, any user (including the PR author) can trigger /review on a malicious PR — the exact scenario this pattern is supposed to prevent.

Suggested fix: Verify the correct schema level for roles: and adjust indentation accordingly. If it's valid at the on: level, add an explicit note confirming this, since events: being under slash_command: creates the natural expectation that roles: also belongs there.

[github-actions]

🟡 MODERATE — Parentheses workaround is non-standard; prefer $\{\{ }} (Flagged by: 2/3 reviewers)

The suggested alternative if: (!github.event.repository.fork) using bare parentheses is not a documented GitHub Actions workaround. The canonical, supported fix for the YAML ! tag-indicator gotcha is if: $\{\{ !github.event.repository.fork }}. Using bare parentheses may work today via implicit expression evaluation, but it's not in GitHub's official docs and could break in future runner versions.

Suggested fix: Replace the parentheses suggestion with the canonical form, e.g.:

Either wrap in $\{\{ }}: if: $\{\{ !github.event.repository.fork }}

[github-actions]

🟡 MODERATE — Unqualified ⛔ Avoid for pull_request contradicts Noise Reduction section (Flagged by: 3/3 reviewers after follow-up)

This table row places pull_request in ⛔ Avoid with no qualification. However, lines 170–184 of this same file actively demonstrate a pull_request + paths: pattern as a recommended noise-reduction technique, and line ~81 scopes the anti-pattern to "agentic workflows" specifically.

A developer scanning this quick-reference table could incorrectly remove a legitimate path-filtered pull_request trigger from a non-agentic CI/pre-check workflow.

Suggested fix: Add the "for agentic workflows" qualifier back to this row, e.g.:

pull_request | For agentic workflows: causes "Approve and run" gate for ALL workflows... Path-filtered pull_request remains appropriate for deterministic CI/pre-check workflows.

[github-actions]

🟡 MODERATE — Cancellation signal sequence is incomplete (Flagged by: 3/3 reviewers after follow-up)

This states GitHub sends SIGTERM, waits 7500ms, then SIGKILL. According to GitHub's cancellation docs, the actual sequence is SIGINT → 7500ms → SIGTERM → 2500ms → kill process tree. SIGINT is omitted, and the timing attribution is imprecise (7500ms is the SIGINT→SIGTERM gap, not SIGTERM→SIGKILL).

Impact: Process authors who register only a SIGTERM handler would believe they have 7500ms, but would actually receive SIGINT first — potentially triggering default behavior (immediate exit) before their handler ever runs.

Suggested fix: Update to: "GitHub sends SIGINT, waits up to 7500ms, then SIGTERM, waits 2500ms, then kills the process tree."

[github-actions]

🟡 MODERATE — roles: all "acceptable" here contradicts "NEVER" in Pattern #1 (Flagged by: 2/3 reviewers)

Pattern #1 (line ~222) says: "NEVER use all unless you've audited every safe-output". This row says roles: all is "acceptable with tight safe-outputs" — presenting it as a recommended default for issues triggers without the audit caveat.

While the intent may be consistent (both require restricted safe-outputs), the phrasing creates opposing impressions: one emphatic prohibition with conditions, one casual acceptance. A developer scanning only the quick-reference table won't see the audit requirement from Pattern #1.

Suggested fix: Align the language, e.g.: roles: all permissible after auditing all safe-outputs (see Pattern #1).