From 8d84037abcf0a6703b2696330d20125b21552ffd Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 5 May 2026 20:25:39 +0200 Subject: [PATCH 1/2] feat(checkpoints): add GH-34/35/36 + reusable-workflow-pitfalls reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GH-34 (mechanical, error): Composite-action references inside .github/workflows/*.yml must be SHA-pinned. Reusable-workflow refs (uses: org/repo/.github/workflows/foo.yml@main) remain exempt per the existing reusable-workflow-security.md doc, but composite-action refs (uses: org/repo/.github/actions/foo@main) break consumers that enforce SHA-pinning when this workflow is called as a reusable workflow. Implemented via PCRE negative lookahead in regex_not. GH-35 (mechanical, warning): Every workflow job with `steps:` should start with step-security/harden-runner. YAML-AST check via embedded python yaml.safe_load — a regex-only check cannot reliably reason about "first step of each job" across multi-line YAML. Pure reusable-workflow callers (job-level `uses:` instead of `steps:`) are exempt. No-ops if python3 or PyYAML are unavailable. GH-36 (llm_review, info): Before declaring PR review feedback addressed, verify zero unresolved review threads via GraphQL. Process check that catches the failure mode where only top-of-discussion threads are addressed and inline-code or earlier-reviewer threads stay open. New reference: references/reusable-workflow-pitfalls.md covers four operational traps distinct from the supply-chain focus of reusable-workflow-security.md: - ./ does NOT resolve to the reusable workflow's repo - composite-action SHA-pinning rule (paired with GH-34) - gh run rerun caches @ref resolution at original trigger time - permissions ceiling: caller's job-level permissions clamp the token SKILL.md description broadened to activate on reusable-workflow, composite-action, and harden-runner authoring tasks (329 chars total). References table gains rows for both reusable-workflow-* docs. Self-tested: skill repo's own workflows pass GH-34 and GH-35. Signed-off-by: Sebastian Mendel --- skills/github-project/SKILL.md | 4 +- skills/github-project/checkpoints.yaml | 107 ++++++++++++++++++ .../references/reusable-workflow-pitfalls.md | 64 +++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 skills/github-project/references/reusable-workflow-pitfalls.md diff --git a/skills/github-project/SKILL.md b/skills/github-project/SKILL.md index 4f1cbd6..8a5d04f 100644 --- a/skills/github-project/SKILL.md +++ b/skills/github-project/SKILL.md @@ -1,6 +1,6 @@ --- name: github-project -description: "Use when PRs won't merge or show BLOCKED status (including green-CI Copilot-review race conditions), auto-approve / auto-merge fails for Dependabot/Renovate, branch protection or rulesets need configuring, GitHub Actions workflows have issues or CI is failing, or setting up CODEOWNERS / PR templates." +description: "Use when PRs won't merge or show BLOCKED (including Copilot-review race), auto-approve/auto-merge fails for Dependabot/Renovate, branch protection or rulesets need configuring, GitHub Actions/CI fails, authoring reusable workflows or composite actions, harden-runner setup, or setting up CODEOWNERS / PR templates." license: "(MIT AND CC-BY-SA-4.0). See LICENSE-MIT and LICENSE-CC-BY-SA-4.0" compatibility: "Requires gh CLI, git." metadata: @@ -107,6 +107,8 @@ scripts/verify-github-project.sh /path/to/repository | Bash pitfalls in workflow `run:` steps | `references/workflow-bash-patterns.md` | | PR shows too many commits (fork merge base) | `references/pr-commit-cleanup.md` | | Multi-repo batch ops | `references/multi-repo-operations.md` | +| Reusable workflow supply-chain trust + SHA pinning | `references/reusable-workflow-security.md` | +| Reusable workflow pitfalls (composite actions, ref caching, permissions) | `references/reusable-workflow-pitfalls.md` | --- diff --git a/skills/github-project/checkpoints.yaml b/skills/github-project/checkpoints.yaml index 9705cab..ed6621b 100644 --- a/skills/github-project/checkpoints.yaml +++ b/skills/github-project/checkpoints.yaml @@ -227,6 +227,79 @@ mechanical: severity: info desc: "Auto-merge should delegate to reusable workflow OR dynamically detect merge strategy from repo settings" + # === COMPOSITE-ACTION SHA PINNING INSIDE WORKFLOWS === + # Reusable-workflow refs like `uses: org/repo/.github/workflows/foo.yml@main` + # are exempt (see references/reusable-workflow-security.md — internal `@main` + # is allowed for full reusable workflows). Composite-action refs like + # `uses: org/repo/.github/actions/foo@main` are NOT exempt: they are resolved + # at the *consumer's* runner under the consumer's allow-list policy. A + # consumer that enforces "all actions must be SHA-pinned" will reject a + # reusable workflow that internally references a composite action by branch + # or tag, even though the workflow file itself is unchanged. SHA-pin + # composite-action refs to avoid breaking SHA-pinned consumers. + # PCRE negative lookahead matches refs that are NOT a 40-char hex SHA. + - id: GH-34 + type: regex_not + target: ".github/workflows/*.{yml,yaml}" + pattern: 'uses:\s+["'']?[A-Za-z0-9._-]+/[A-Za-z0-9._-]+/\.github/actions/[^@]+@(?![0-9a-f]{40}\b)' + severity: error + desc: >- + Composite action references inside .github/workflows/*.yml must be + SHA-pinned (40-char hex). Branch/tag refs (@main, @v1) break consumers + that enforce SHA pinning when this workflow is called as a reusable + workflow. See references/reusable-workflow-pitfalls.md. + + # === HARDEN-RUNNER FIRST-STEP CHECK === + # Mechanical YAML-AST check via python yaml.safe_load (a regex-only check + # cannot reliably reason about "first step of each job" across multi-line + # YAML). Skipped if PyYAML is unavailable. Jobs that are pure + # reusable-workflow callers (job-level `uses:` instead of `steps:`) are + # exempt — they have no runner steps to harden. + - id: GH-35 + type: command + target: | + command -v python3 >/dev/null 2>&1 || exit 0 + python3 - <<'PY' 2>/dev/null || exit 0 + try: + import yaml + except ImportError: + import sys; sys.exit(0) + import glob, sys + bad = [] + for f in sorted(glob.glob('.github/workflows/*.yml') + + glob.glob('.github/workflows/*.yaml')): + try: + with open(f) as fh: + wf = yaml.safe_load(fh) + except Exception: + continue + if not isinstance(wf, dict): + continue + for jname, job in (wf.get('jobs') or {}).items(): + if not isinstance(job, dict): + continue + # Skip reusable-workflow callers (no steps array). + if 'uses' in job and 'steps' not in job: + continue + steps = job.get('steps') + if not isinstance(steps, list) or not steps: + continue + first = steps[0] if isinstance(steps[0], dict) else {} + uses = first.get('uses', '') or '' + if not uses.startswith('step-security/harden-runner'): + bad.append(f"{f}::{jname}") + if bad: + sys.stderr.write("Jobs missing harden-runner as first step: " + + ", ".join(bad) + "\n") + sys.exit(1) + sys.exit(0) + PY + severity: warning + desc: >- + Every workflow job with `steps:` should start with + `step-security/harden-runner` (audit egress). Jobs that are pure + reusable-workflow callers (job-level `uses:`) are exempt. + # === AUTO-APPROVE (pr-quality.yml) COPILOT RACE CONDITION === - id: GH-33 type: command @@ -352,6 +425,40 @@ llm_reviews: severity: error desc: "Verify required_conversation_resolution is enabled (error); enforce_admins is advisory only (see GH-30)." + # === PR REVIEW COMPLETENESS PROCESS CHECK === + - id: GH-36 + domain: pr-process + prompt: | + When a PR's review feedback is being addressed, before declaring it + "ready to merge" or "all comments addressed": + + 1. Query unresolved review threads via GraphQL: + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){ + repository(owner:$owner,name:$repo){pullRequest(number:$pr){ + reviewThreads(first:50){nodes{ + id isResolved comments(first:1){nodes{body path line}} + }} + }} + }' -f owner=OWNER -f repo=REPO -F pr=NUMBER \ + --jq '.data.repository.pullRequest.reviewThreads.nodes + | map(select(.isResolved == false))' + + 2. The count of unresolved threads must be exactly zero. + 3. If non-zero, list each thread with the first comment's body + (truncated to ~100 chars) plus path/line so the user can see + exactly what's outstanding. + + Common failure mode: addressing only the threads visible at the top + of the PR discussion, missing inline-code threads or threads from + earlier reviewers. The GraphQL query is the single source of truth — + do not rely on the PR conversation tab alone. + + Report: "All threads resolved" OR "N unresolved thread(s): ". + severity: info + desc: >- + Before declaring PR review feedback addressed, verify zero unresolved + review threads via GraphQL (not just the conversation tab). + # === SUBJECTIVE CHECKS (require LLM judgment) === - id: GH-15 domain: repo-health diff --git a/skills/github-project/references/reusable-workflow-pitfalls.md b/skills/github-project/references/reusable-workflow-pitfalls.md new file mode 100644 index 0000000..4e3a2b7 --- /dev/null +++ b/skills/github-project/references/reusable-workflow-pitfalls.md @@ -0,0 +1,64 @@ +# Reusable Workflow Pitfalls + +Operational and structural pitfalls when authoring reusable workflows and the composite actions they use. Distinct from `reusable-workflow-security.md` — that doc covers supply-chain trust and SHA pinning of *external* actions; this doc covers structural traps that have bitten in practice when building *internal* reusable workflows. + +## 1. `./` does NOT resolve to the reusable workflow's repo + +When a workflow is invoked via `workflow_call`, references like `uses: ./.github/actions/foo` are resolved against the **consumer's** workspace, not the reusable workflow's repo. The action will fail to load unless the consumer happens to have an identically-named local action. + +GitHub documents this directly: "Local actions in workflows that are called as a reusable workflow are not supported." (See [Reusing workflows — limitations](https://docs.github.com/en/actions/sharing-automations/reusing-workflows#limitations).) + +To call a composite action from a reusable workflow, reference it absolutely: + +```yaml +# In the reusable workflow: +uses: org/repo/.github/actions/foo@ # works (absolute reference) +# uses: ./.github/actions/foo # FAILS at the consumer +``` + +## 2. Composite-action references must be SHA-pinned + +Internal `@main` references are accepted for **full reusable workflows** (`uses: org/repo/.github/workflows/foo.yml@main`) — see `reusable-workflow-security.md`. But **composite actions** (`uses: org/repo/.github/actions/foo@main`) are different: the consumer's runner resolves them under the consumer's allow-list policy. A consumer enforcing "all actions must be SHA-pinned" will reject the reusable workflow's job entirely, even though the reusable workflow file itself is unchanged. + +This is enforced mechanically by checkpoint **GH-34**. + +```yaml +# Inside a reusable workflow: +- uses: org/repo/.github/actions/preflight-gate@<40-char-sha> # OK +# - uses: org/repo/.github/actions/preflight-gate@main # breaks SHA-pinned consumers +``` + +When you self-reference a composite action inside the same repo that hosts the reusable workflow, you create a chicken-and-egg: the SHA you pin to must already exist in the same PR you're authoring. Two options: (a) inline the action's body as bash steps directly in the workflow (avoiding the cross-action `uses:`), or (b) land the composite action first, then SHA-pin to it in a follow-up PR. + +## 3. `gh run rerun` caches `@ref` resolution + +When a workflow run is created, GitHub records the resolved SHAs of every `uses: org/repo/...@ref` at that moment. **Re-running the same run replays those exact SHAs** — it does not re-evaluate the refs. This means: if you fix a bug in an upstream reusable workflow and merge the fix, then re-run a failed workflow that consumed `@main`, you will get the **old, broken** body. + +To pick up upstream changes after merging the fix: + +- Push a new commit to the PR (`git commit --allow-empty -m "trigger ci"` works), or +- Close + reopen the PR, or +- Trigger a fresh `workflow_dispatch` run. + +`gh run rerun` is fine for genuinely transient failures (network blips, rate limits) — not for "I fixed the upstream reusable workflow." + +## 4. Permissions ceiling — caller cannot grant what it lacks + +A reusable workflow's `permissions:` block sets the **maximum** scopes the called job is allowed to use. The actual token issued is the **intersection** of the caller's job-level `permissions:` and the reusable workflow's declared `permissions:`. If the calling job sets `permissions: {}` (empty), the reusable job receives a token with **zero** scopes regardless of what the reusable workflow declares. + +When a reusable workflow gains a new requirement (e.g., a step that calls `gh api` and needs `actions: read`), every consumer's calling job must also be updated to grant that scope. Otherwise the new step fails with a 403 in production but works in your test repo where you happen to have broader permissions. + +```yaml +# Consumer side — must include EVERY scope the reusable workflow needs: +jobs: + call-shared: + permissions: + contents: read + actions: read # required by the reusable workflow's gh api step + pull-requests: write + uses: org/ci-workflows/.github/workflows/shared.yml@ +``` + +See [GitHub docs — access and permissions](https://docs.github.com/en/actions/sharing-automations/reusing-workflows#access-and-permissions) for the full intersection rules. + +> **See also:** [`reusable-workflow-security.md`](./reusable-workflow-security.md) for SHA-pinning and supply-chain trust of *external* actions. From 535c97fdcbd558bf1f9d000396050ac26da83514 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 5 May 2026 23:08:25 +0200 Subject: [PATCH 2/2] fix(checkpoints): address PR review feedback + skill word limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SKILL.md: trim to 495 words (under 500 limit) — tightened description, removed redundant "See ..." pointers already covered by the references table. - SKILL.md / checkpoints.yaml GH-36: bump reviewThreads(first:50) to first:100 (GraphQL max) and document pageInfo.hasNextPage pagination so PRs with >100 threads aren't silently mis-reported as "all resolved". - checkpoints.yaml GH-34: tighten the 40-hex-SHA negative lookahead so refs like @<40hex>-tag are correctly flagged as non-SHA-pinned; the previous \b boundary let any 40-hex prefix pass. - checkpoints.yaml GH-35: drop the trailing "|| exit 0" and the "2>/dev/null" wrapper on the Python harden-runner check so real failures (and the "Jobs missing harden-runner..." message) actually surface; missing python3/PyYAML cases are still handled explicitly upstream. Signed-off-by: Sebastian Mendel --- skills/github-project/SKILL.md | 12 ++++++------ skills/github-project/checkpoints.yaml | 21 ++++++++++++++------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/skills/github-project/SKILL.md b/skills/github-project/SKILL.md index 8a5d04f..b769a9c 100644 --- a/skills/github-project/SKILL.md +++ b/skills/github-project/SKILL.md @@ -1,6 +1,6 @@ --- name: github-project -description: "Use when PRs won't merge or show BLOCKED (including Copilot-review race), auto-approve/auto-merge fails for Dependabot/Renovate, branch protection or rulesets need configuring, GitHub Actions/CI fails, authoring reusable workflows or composite actions, harden-runner setup, or setting up CODEOWNERS / PR templates." +description: "Use when PRs won't merge or show BLOCKED (including Copilot-review race), auto-approve/auto-merge fails for Dependabot/Renovate, branch protection/rulesets need configuring, CI fails, authoring reusable workflows or composite actions, harden-runner setup, or CODEOWNERS / PR templates." license: "(MIT AND CC-BY-SA-4.0). See LICENSE-MIT and LICENSE-CC-BY-SA-4.0" compatibility: "Requires gh CLI, git." metadata: @@ -35,18 +35,18 @@ GitHub repository configuration, troubleshooting, and collaboration workflow bes gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){ repository(owner:$owner,name:$repo){pullRequest(number:$pr){ mergeStateStatus reviewDecision mergeable - reviewThreads(first:50){nodes{isResolved comments(first:1){nodes{body}}}} + reviewThreads(first:100){nodes{isResolved comments(first:1){nodes{body}}}} }} }' -f owner=OWNER -f repo=REPO -F pr=NUMBER --jq '.data.repository.pullRequest' ``` ### Solo Maintainer: PRs Stuck on REVIEW_REQUIRED -Use `assets/pr-quality.yml.template` for auto-approve with `required_approving_review_count >= 1`. See `references/auto-merge-guide.md`. +Use `assets/pr-quality.yml.template` for auto-approve with `required_approving_review_count >= 1`. ### Auto-merge Setup -Requirements: `allow_auto_merge` on repo, `pull_request_target` trigger (not `pull_request`), check `user.login` (not `github.actor`), `gh pr merge --auto` with dynamic strategy. See `references/auto-merge-guide.md`. +Requirements: `allow_auto_merge` on repo, `pull_request_target` trigger (not `pull_request`), check `user.login` (not `github.actor`), `gh pr merge --auto` with dynamic strategy. ### Auto-merge Not Working @@ -74,14 +74,14 @@ gh api repos/OWNER/REPO/branches/main/protection --jq '.enforce_admins.enabled' gh api repos/OWNER/REPO/code-scanning/default-setup --jq '.state' gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){ repository(owner:$owner,name:$repo){pullRequest(number:$pr){ - reviewThreads(first:50){nodes{id isResolved}} + reviewThreads(first:100){nodes{id isResolved}} }} }' -f owner=OWNER -f repo=REPO -F pr=NUMBER ``` ### Merge Strategy Issues -See `references/auto-merge-guide.md` for: rebase-merge-with-signed-commits fixes, workflow-file PR manual merges, and the Copilot-review auto-approve race (PR `BLOCKED` despite green CI + empty `reviewDecision`). +See `references/auto-merge-guide.md` for: rebase-merge-with-signed-commits fixes, workflow-file PR manual merges, and the Copilot-review auto-approve race. ## Running Scripts diff --git a/skills/github-project/checkpoints.yaml b/skills/github-project/checkpoints.yaml index ed6621b..e73ef30 100644 --- a/skills/github-project/checkpoints.yaml +++ b/skills/github-project/checkpoints.yaml @@ -238,10 +238,13 @@ mechanical: # or tag, even though the workflow file itself is unchanged. SHA-pin # composite-action refs to avoid breaking SHA-pinned consumers. # PCRE negative lookahead matches refs that are NOT a 40-char hex SHA. + # The lookahead requires the 40 hex chars to be followed by quote/comment/ + # whitespace/EOL — without that terminator, refs like `@<40hex>-tag` would + # spuriously pass because `\b` matches between hex and `-`. - id: GH-34 type: regex_not target: ".github/workflows/*.{yml,yaml}" - pattern: 'uses:\s+["'']?[A-Za-z0-9._-]+/[A-Za-z0-9._-]+/\.github/actions/[^@]+@(?![0-9a-f]{40}\b)' + pattern: 'uses:\s+["'']?[A-Za-z0-9._-]+/[A-Za-z0-9._-]+/\.github/actions/[^@]+@(?![0-9a-f]{40}(?:["''#\s]|$))' severity: error desc: >- Composite action references inside .github/workflows/*.yml must be @@ -259,7 +262,7 @@ mechanical: type: command target: | command -v python3 >/dev/null 2>&1 || exit 0 - python3 - <<'PY' 2>/dev/null || exit 0 + python3 - <<'PY' try: import yaml except ImportError: @@ -432,18 +435,22 @@ llm_reviews: When a PR's review feedback is being addressed, before declaring it "ready to merge" or "all comments addressed": - 1. Query unresolved review threads via GraphQL: + 1. Query unresolved review threads via GraphQL (first:100 is the + GraphQL maximum; for PRs at the cap, paginate with + pageInfo.hasNextPage / endCursor + after:): gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){ repository(owner:$owner,name:$repo){pullRequest(number:$pr){ - reviewThreads(first:50){nodes{ - id isResolved comments(first:1){nodes{body path line}} - }} + reviewThreads(first:100){ + pageInfo{hasNextPage endCursor} + nodes{id isResolved comments(first:1){nodes{body path line}}} + } }} }' -f owner=OWNER -f repo=REPO -F pr=NUMBER \ --jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false))' - 2. The count of unresolved threads must be exactly zero. + 2. The count of unresolved threads must be exactly zero. If + pageInfo.hasNextPage is true, paginate before concluding. 3. If non-zero, list each thread with the first comment's body (truncated to ~100 chars) plus path/line so the user can see exactly what's outstanding.