diff --git a/skills/github-project/SKILL.md b/skills/github-project/SKILL.md index 4f1cbd6..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 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/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 @@ -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..e73ef30 100644 --- a/skills/github-project/checkpoints.yaml +++ b/skills/github-project/checkpoints.yaml @@ -227,6 +227,82 @@ 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. + # 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}(?:["''#\s]|$))' + 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' + 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 +428,44 @@ 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 (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: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. 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. + + 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.