diff --git a/.claude/skills/sweep-open-prs/SKILL.md b/.claude/skills/sweep-open-prs/SKILL.md new file mode 100644 index 000000000..26d557985 --- /dev/null +++ b/.claude/skills/sweep-open-prs/SKILL.md @@ -0,0 +1,189 @@ + + +--- +name: sweep-open-prs +description: Use when the maintainer asks what can be landed across the open PR queue - "check if I can merge any PRs", "which PRs can be approved and merged", "sweep the PRs", "what's ready to land", "anything new on the PRs". Also for a routine pass over the queue after an overnight dependabot batch. Not for a deep review of one named PR. +--- + +# Sweep the open PR queue + +Answer "what can I land right now?" for every open PR in one pass, then act +only on what the maintainer confirms. + +The queue is mostly dependabot: allowlist bumps under +`.github/actions/for-dependabot-triggered-reviews/`, plus workflow and +`uv.lock` bumps. A sweep is cheap because two `gh` calls cover the whole +queue; the expensive part is reviewing whatever the sweep surfaces as red or +as needing a human read. + +## Environment + +`gh` reads its config and auth keyring from `~/.config/gh` and needs +api.github.com, so it does not work inside the sandbox. The usual setup +excludes it with `sandbox.excludedCommands: ["gh *"]`, and then every `gh` +call runs unsandboxed with no bypass needed. Without that exclusion, each call +needs `dangerouslyDisableSandbox: true`, and the first failure looks like this +(it is not an auth problem): + +``` +failed to load config: open /Users//.config/gh/config.yml: operation not permitted +``` + +**The exclusion only matches a bare `gh` command.** A pipe, redirect, `;` or +`&&` puts the whole command back in the sandbox, where it fails with +`deny network-outbound api.github.com:443`, or with +`tls: failed to verify certificate: x509: OSStatus -26276` once the host is +allowed. Filter with `--jq` / `--template` instead of piping, and read output +from the tool result instead of redirecting it to a file. Keep the command on +one line too: a multi-line argument (a `--body` with newlines, a `\` +continuation) does not match the exclusion either, so pass long text with +`--body-file`. + +Two more traps, both of which produce a bogus `HTTP 401`: + +| Trap | Fix | +|---|---| +| Batching `gh` calls in a shell `for` loop | One `gh` call per Bash invocation | +| `gh` invoked from a python/sh child process | Call `gh` directly from Bash | + +## Step 1: gather + +Two calls cover the queue. Run them as written, each on a single line (see +Environment) - the per-PR alternative burns a call per PR and hits the loop +trap above. + +State: + +``` +gh pr list --repo apache/infrastructure-actions --limit 50 --json number,title,author,isDraft,mergeable,mergeStateStatus,reviewDecision --jq '.[] | "\(.number)\t@\(.author.login)\tmergeable=\(.mergeable)\tstate=\(.mergeStateStatus)\treview=\(.reviewDecision)\t\(.title)"' +``` + +Checks, collapsed to only what is NOT green: + +``` +gh pr list --repo apache/infrastructure-actions --limit 50 --json number,statusCheckRollup --jq '.[] | "\(.number)\t" + ([.statusCheckRollup[] | . as $c | (if ($c.status // "COMPLETED") != "COMPLETED" then $c.status elif ($c.conclusion // "") != "" then $c.conclusion elif ($c.state // "") != "" then $c.state else "PENDING" end) as $s | select($s != "SUCCESS" and $s != "NEUTRAL" and $s != "SKIPPED") | "\($c.name // $c.context)=\($s)"] | if length==0 then "ALL GREEN" else join(" ") end)' +``` + +**Read `status` before `conclusion`.** A `CheckRun` carries `conclusion: null` +until its `status` reaches `COMPLETED`, and its `state` is always null - `state` +belongs to the `StatusContext` shape instead. A query that reaches straight for +`(.conclusion // .state)` therefore prints a bare `name=` for every check still +running, which reads like a finding rather than "ask again in a minute". The +query above reports `QUEUED` or `IN_PROGRESS` instead. + +Two states that are *not* verdicts, and must be re-queried rather than +classified: + +- `mergeStateStatus: UNKNOWN` - GitHub is recomputing mergeability, usually + right after another PR merged. It also appears mid-merge-storm when several + PRs touch one file; the state settles back to `CLEAN` on its own. +- A check reported `QUEUED` or `IN_PROGRESS` - not yet a result. Checks on a + fresh PR routinely take minutes, so a sweep run the moment a PR opens will + see them unfinished. + +## Step 2: classify + +Bucket each PR on the observable fields alone: + +| Bucket | Signature | Action | +|---|---|---| +| **Merge now** | `state=CLEAN`, `review=APPROVED`, all green | Merge on confirmation | +| **Approve + merge** | all green, `review=REVIEW_REQUIRED`, author is dependabot or another person | Review, then approve + merge | +| **Needs triage** | any check not green | Read the failing job before judging | +| **Blocked on reviewer** | all green, `review=REVIEW_REQUIRED`, **maintainer is the author** | Cannot self-approve - nudge, do not attempt | +| **Blocked on author** | `review=CHANGES_REQUESTED` | Waiting on the bump or an upstream fix | +| **Conflicting** | `mergeable=CONFLICTING` | Needs a rebase first | + +Two rules that decide most of the queue: + +- **You cannot approve your own PR.** A maintainer-authored PR with + `review=REVIEW_REQUIRED` is never in an actionable bucket no matter how + green it is. Say so plainly rather than offering to approve it. +- **`state=BLOCKED` with `review=REVIEW_REQUIRED` just means "needs an + approval"** - it is not a failure. `BLOCKED` alongside a red check is the + failure. + +## Step 3: act, per bucket + +### Allowlist bumps (`action-allowlist-review:` titles) + +The `verify` check is the gate. Green `verify` means the tool reconciled the +published action against a rebuild from its own lock file. + +**REQUIRED SUB-SKILL:** for anything red, or for any bump you are about to +approve on substance rather than on a green tick, use `analyze-action-pr`. It +owns the failure taxonomy (pipe-to-shell, unverified download, in-tree +binaries, verify-script gaps) and the upstream-issue workflow. + +Before treating a warning as a finding, **diff it against the approved +version**. A warning the already-approved version also carries is a standing +property of the action, not a regression the bump introduced. Carried-over +warnings do not block. + +### Lock-file and workflow bumps + +`uv.lock` / `package-lock.json` only, or a workflow action SHA bump: read the +diff, confirm it touches no action surface, approve. + +### Red checks + +Read the failing job before forming a view: + +``` +gh pr checks --repo apache/infrastructure-actions --json name,state,link --jq '.[] | select(.state=="FAILURE") | .link' +gh run view --repo apache/infrastructure-actions --job --log-failed +``` + +Run each as its own Bash call, with no pipe (see Environment). The log keeps +its ANSI colour escapes; read past them to the summary table. + +**A red check on a stale branch may be a fixed bug.** Before reporting a +finding, check whether the failure predates something already merged to main - +rebase or re-run rather than filing against it. + +### Superseded PRs + +A bump held for an upstream defect is often replaced rather than fixed: +dependabot opens a new PR at the newer version and closes the old one. Before +digging back into a held PR, check whether a higher-numbered PR for the same +action already exists. + +## Reporting + +Lead with what the maintainer can act on, not with the full queue. A table +keyed by bucket beats a PR-by-PR narrative once the queue is more than a +handful. + +**Draft every outbound message and wait for explicit confirmation** - reviews, +PR comments, upstream issues. Approving is a message in the maintainer's name. +Merging a PR they just approved in the same turn is the mid-flow exception. + +## Common mistakes + +| Mistake | Consequence | +|---|---| +| Reading `state=BLOCKED` as "broken" | Most of a healthy queue is `BLOCKED` pending one approval | +| Classifying on a `mergeStateStatus: UNKNOWN` | Misses a PR that is actually ready; re-query first | +| Reading an unfinished check as a failure | A running check has no conclusion yet; re-query rather than triaging it | +| Offering to approve the maintainer's own PR | Impossible on GitHub; wastes a round-trip | +| Treating a carried-over warning as a regression | Blocks a bump that is no worse than the approved version | +| Filing upstream before searching the target repo | Duplicate issues; search existing issues, including the maintainer's own | +| Looping `gh` over PR numbers | Spurious `HTTP 401` on every call after the first | +| Piping or redirecting `gh` output | The command runs sandboxed and fails with a network deny or `x509` TLS error | diff --git a/README.md b/README.md index 8e50da541..576e8292a 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,7 @@ This repository hosts GitHub Actions developed by the ASF community and approved - [Checking the Action Usage in an ASF Project](#checking-the-action-usage-in-an-asf-project) - [Submitting an Action](#submitting-an-action) +- [Agent Skills](#agent-skills) - [Available GitHub Actions](#available-github-actions) - [Versioning and Pinning Actions](#versioning-and-pinning-actions) - [Organization-wide GitHub Actions Allow List](#management-of-organization-wide-github-actions-allow-list) @@ -93,6 +94,28 @@ action, even if they're not committers on _this_ project: you're especially qualified to judge and vouch for the safety and correctness of the action. +## Agent Skills + +Skills under `.claude/skills/` package the review workflows a maintainer repeats, so an +agent runs them the same way each time. They are agent-facing and need no setup beyond +an authenticated `gh` CLI; a human can follow the same steps by reading the `SKILL.md`. + + - [`analyze-action-pr`](/.claude/skills/analyze-action-pr/SKILL.md): Triage a single PR + that adds or bumps an action. Runs `verify-action-build`, classifies each failing + action (pipe-to-shell, unverified download, unverified in-tree binaries, gaps in the + verify script itself) and proposes the next step -- recommend approval, open an + upstream issue, or fix `verify-action-build` with a regression test. Its "Recent + precedents" table records how earlier PRs in each shape were resolved. + - [`sweep-open-prs`](/.claude/skills/sweep-open-prs/SKILL.md): Triage the whole open PR + queue in one pass. Buckets every open PR into merge-now, approve-and-merge, + needs-triage or blocked, and routes anything red to `analyze-action-pr`. Useful after + an overnight dependabot batch. + +Use `sweep-open-prs` to find what needs attention and `analyze-action-pr` to go deep on +one PR. For the non-agent equivalent of the sweep, see +[Batch-Reviewing Dependabot PRs](#batch-reviewing-dependabot-prs), which drives the same +verification from `verify-action-build` directly. + ## Available GitHub Actions - [ASF Infrastructure Pelican Action](/pelican/README.md): Generate and publish project websites with GitHub Actions