diff --git a/.github/scripts/activity-labeler.js b/.github/scripts/activity-labeler.js index bf645b67d..56f7dbb3c 100644 --- a/.github/scripts/activity-labeler.js +++ b/.github/scripts/activity-labeler.js @@ -90,13 +90,13 @@ module.exports = async ({ github, context, core }) => { } core.info(`Scanning ${prs.length} open PRs`); - for (const pr of prs) { + async function processOnePr(pr) { const pr_number = pr.number; const labelNames = pr.labels.map((l) => l.name); if (pr.draft) { core.info(`#${pr_number}: draft, skipping`); - continue; + return; } if (labelNames.some((n) => EXEMPT_LABELS.includes(n))) { // A manual outcome label means a human has already made the call — @@ -113,7 +113,7 @@ module.exports = async ({ github, context, core }) => { } else { core.info(`#${pr_number}: has manual outcome label, skipping`); } - continue; + return; } // Gather every timestamped human event on the PR. @@ -224,4 +224,32 @@ module.exports = async ({ github, context, core }) => { } } } + + let scanFailures = 0; + + for (const pr of prs) { + try { + await processOnePr(pr); + } catch (e) { + // One PR's failure (a deleted PR, a transient API error, anything) + // must never abort the scan for every other open PR still queued + // behind it. This matters most here — more than in the backfill + // workflow's own loop — because the nightly cron and a full manual + // run both scan every open PR in a single pass; without this, one + // bad PR would silently kill that night's entire backstop scan. + core.warning(`#${pr.number}: failed during activity scan, skipping (${e.message})`); + scanFailures += 1; + } + } + + // Per-PR isolation (above) stops one bad PR from killing the whole scan, + // but it also means a SHARED failure (rate limit, API outage) would hit + // every remaining PR the same way — each one caught, logged, and skipped + // individually — and the job would still exit green with nothing actually + // done. Failing the job here (after the loop, not per-PR) surfaces that + // case as a run someone will notice and rerun, without giving up the + // per-PR isolation itself. + if (scanFailures > 0) { + core.setFailed(`Activity scan failed for ${scanFailures} of ${prs.length} PR(s) — see warnings above for details.`); + } }; diff --git a/.github/scripts/pr-metadata-labeler.js b/.github/scripts/pr-metadata-labeler.js index dbb2de7ad..d1f519e93 100644 --- a/.github/scripts/pr-metadata-labeler.js +++ b/.github/scripts/pr-metadata-labeler.js @@ -21,6 +21,57 @@ function sizeTier(totalLines, filesChanged) { return `${SIZE_PREFIX}XS`; } +// --- first-contribution --- +// Deliberately NOT using GitHub's Search API here — it has a much +// stricter *secondary* rate limit (30/min) than everything else this +// codebase calls, and hitting it once per PR is what caused a real +// backfill to fail partway the first time this ran at scale. +// +// Also deliberately NOT fetching the repo's full PR history and caching +// it in memory (an earlier version of this file did exactly that): that +// works fine for a single backfill process looping over many PRs, but +// pr.area-labeler.yml runs this in a FRESH process for every single +// real-time PR event — so that cache was empty every time it mattered, +// and "fixing" the backfill case reintroduced the same scaling problem +// on the far more frequent real-time path (every open/push/reopen event, +// on every PR, forever, would refetch the entire repo's PR history just +// to check one author). +// +// Instead: `creator` filters server-side to just this one author's items +// — cheap in both contexts regardless of total repo size — and this +// stops paginating the instant it's seen enough to know the answer. +async function isFirstContribution(github, owner, repo, author) { + try { + let prCount = 0; + let page = 1; + // eslint-disable-next-line no-constant-condition + while (true) { + const { data } = await github.rest.issues.listForRepo({ + owner, + repo, + creator: author, + state: "all", + per_page: 100, + page, + }); + for (const item of data) { + // listForRepo returns issues AND PRs by this author — `pull_request` + // is only present on the PR ones, which is all we're counting. + if (item.pull_request) prCount++; + if (prCount > 1) return false; // already confirmed not their first — stop here, no need to see the rest + } + if (data.length < 100) break; // last page + page++; + } + return prCount <= 1; + } catch (e) { + // A missing "nice to have" label is a much smaller problem than + // letting this crash the caller's loop — log and move on. + console.warn(`first-contribution check failed for ${author}: ${e.message}`); + return false; + } +} + async function labelOne({ github, owner, repo, pr }) { const pr_number = pr.number; const { data: current } = await github.rest.issues.get({ owner, repo, issue_number: pr_number }); @@ -48,11 +99,7 @@ async function labelOne({ github, owner, repo, pr }) { // --- first-contribution (sticky once set, cheap to skip re-checking) --- if (!labelNames.includes("first-contribution")) { - const author = pr.user.login; - const { data: pastPRs } = await github.rest.search.issuesAndPullRequests({ - q: `repo:${owner}/${repo} type:pr author:${author}`, - }); - if (pastPRs.total_count <= 1) { + if (await isFirstContribution(github, owner, repo, pr.user.login)) { await github.rest.issues.addLabels({ owner, repo, issue_number: pr_number, labels: ["first-contribution"] }); } } diff --git a/.github/workflows/ops.label-sync.yml b/.github/workflows/ops.label-sync.yml index c5acca9c9..56ea44864 100644 --- a/.github/workflows/ops.label-sync.yml +++ b/.github/workflows/ops.label-sync.yml @@ -6,16 +6,17 @@ on: paths: [".github/labels.yml"] workflow_dispatch: {} # run manually any time to re-sync -permissions: - contents: read # REQUIRED for actions/checkout to read the repo — this was missing and caused the "repository not found" failure - issues: write # labels API lives under Issues - jobs: sync: runs-on: ubuntu-latest + permissions: + contents: read # REQUIRED for actions/checkout to read the repo — this was missing and caused the "repository not found" failure + issues: write # labels API lives under Issues steps: - - uses: actions/checkout@v7 - - uses: crazy-max/ghaction-github-labeler@v6 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: crazy-max/ghaction-github-labeler@548a7c3603594ec17c819e1239f281a3b801ab4d # v6 with: github-token: ${{ secrets.GITHUB_TOKEN }} yaml-file: .github/labels.yml diff --git a/.github/workflows/ops.pr-activity-labeler.yml b/.github/workflows/ops.pr-activity-labeler.yml index 823ccdab3..57c80987a 100644 --- a/.github/workflows/ops.pr-activity-labeler.yml +++ b/.github/workflows/ops.pr-activity-labeler.yml @@ -5,7 +5,7 @@ on: - cron: "0 3 * * *" # daily at 03:00 UTC — adjust to your timezone/preference; still the backstop for anything the triggers below can't reach for a forked PR (see note on pull_request_review below) issue_comment: types: [created] # near-real-time: a comment on the Conversation tab - pull_request_target: + pull_request_target: # zizmor: ignore[dangerous-triggers] — no ref: on checkout below, so it always uses the base branch; the fork's code is never checked out or executed. See pr.area-labeler.yml for the same reasoning in more detail. types: [synchronize] # near-real-time: author pushes new commits. Must be pull_request_target (not pull_request) for full write permissions on forked PRs — same reasoning as pr.area-labeler.yml. Checkout below has no `ref:`, so it uses the base branch, never the fork's code. pull_request_review: types: [submitted] # near-real-time: a maintainer's review (approve/request changes/comment) @@ -38,11 +38,6 @@ on: # 24h regardless, so nothing is silently wrong, just delayed for that one # combination of circumstances. -permissions: - pull-requests: write - issues: write # labels/labels-list live under the Issues API even for PRs - contents: read - jobs: activity-scan: # issue_comment fires for comments on plain issues too, not just PRs — @@ -52,6 +47,10 @@ jobs: # property access) for both of those. if: github.event_name != 'issue_comment' || github.event.issue.pull_request runs-on: ubuntu-latest + permissions: + pull-requests: write + issues: write # labels/labels-list live under the Issues API even for PRs + contents: read env: STALE_AFTER_DAYS: ${{ inputs.stale_after_days }} NEEDS_DECISION_AFTER_DAYS: ${{ inputs.needs_decision_after_days }} @@ -64,8 +63,10 @@ jobs: # (blank by default, meaning "scan everything"). ONLY_PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number || inputs.pr_number }} steps: - - uses: actions/checkout@v7 - - uses: actions/github-script@v9 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: script: | const run = require('${{ github.workspace }}/.github/scripts/activity-labeler.js'); diff --git a/.github/workflows/ops.pr-labels-reset.yml b/.github/workflows/ops.pr-labels-reset.yml index a9dbfd3d6..572effac3 100644 --- a/.github/workflows/ops.pr-labels-reset.yml +++ b/.github/workflows/ops.pr-labels-reset.yml @@ -30,14 +30,13 @@ on: description: 'Required when dry_run is unchecked: type RESET exactly to allow real changes.' required: false -permissions: - pull-requests: write - issues: write # labels live under the Issues API even for PRs - contents: read - jobs: reset: runs-on: ubuntu-latest + permissions: + pull-requests: write + issues: write # labels live under the Issues API even for PRs + contents: read env: PR_NUMBERS: ${{ inputs.pr_numbers }} ALL_OPEN_PRS: ${{ inputs.all_open_prs }} @@ -46,8 +45,10 @@ jobs: DRY_RUN: ${{ inputs.dry_run }} CONFIRM: ${{ inputs.confirm }} steps: - - uses: actions/checkout@v7 - - uses: actions/github-script@v9 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: script: | const run = require('${{ github.workspace }}/.github/scripts/pr-labels-reset.js'); diff --git a/.github/workflows/pr.area-labeler.yml b/.github/workflows/pr.area-labeler.yml index 681412945..0ce191e2a 100644 --- a/.github/workflows/pr.area-labeler.yml +++ b/.github/workflows/pr.area-labeler.yml @@ -7,19 +7,17 @@ name: PR Area & Size Labeler # defaults to the base branch's ref on this event, not the fork's head, # so it never pulls untrusted code either. on: - pull_request_target: + pull_request_target: # zizmor: ignore[dangerous-triggers] — we only read file paths/metadata here, never check out or execute fork code; see the note above. types: [opened, synchronize, reopened] -permissions: - pull-requests: write - issues: write # labels/labels-list live under the Issues API even for PRs - contents: read - jobs: area-labels: runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read # REQUIRED: this job has no checkout step, so actions/labeler fetches .github/labeler.yml via the API — that needs contents: read (see actions/labeler's own docs) steps: - - uses: actions/labeler@v7 + - uses: actions/labeler@bf12e9b00b37c5c0ca2b87b79b2daf7891dbda13 # v7 with: configuration-path: .github/labeler.yml sync-labels: true # remove area labels that no longer match after new commits @@ -27,9 +25,14 @@ jobs: metadata-labels: needs: area-labels runs-on: ubuntu-latest + permissions: + contents: read + issues: write # labels/labels-list live under the Issues API even for PRs steps: - - uses: actions/checkout@v7 - - uses: actions/github-script@v9 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: script: | const { labelOne } = require('${{ github.workspace }}/.github/scripts/pr-metadata-labeler.js'); diff --git a/.github/workflows/pr.labels-backfill.yml b/.github/workflows/pr.labels-backfill.yml index 97b291d15..f8f3e4982 100644 --- a/.github/workflows/pr.labels-backfill.yml +++ b/.github/workflows/pr.labels-backfill.yml @@ -7,20 +7,17 @@ name: Backfill All PR Labels (existing open PRs) on: workflow_dispatch: {} -permissions: - pull-requests: write - issues: write # labels/labels-list live under the Issues API even for PRs - contents: read - jobs: list-open-prs: runs-on: ubuntu-latest + permissions: + pull-requests: read outputs: json: ${{ steps.list.outputs.json }} # e.g. [3,2,1] — for embedding into JS array literals lines: ${{ steps.list.outputs.lines }} # e.g. "3\n2\n1" with REAL newlines — for actions/labeler's pr-number input steps: - id: list - uses: actions/github-script@v9 + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: script: | const prs = await github.paginate(github.rest.pulls.list, { @@ -41,8 +38,14 @@ jobs: needs: list-open-prs if: needs.list-open-prs.outputs.json != '[]' runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write steps: - - uses: actions/labeler@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 — without this, actions/labeler can't find .github/labeler.yml locally and re-fetches it via the API once per PR in the list — harmless but noisy, and this avoids it + with: + persist-credentials: false + - uses: actions/labeler@bf12e9b00b37c5c0ca2b87b79b2daf7891dbda13 # v7 with: configuration-path: .github/labeler.yml pr-number: ${{ needs.list-open-prs.outputs.lines }} @@ -51,26 +54,51 @@ jobs: metadata-labels-backfill: needs: [list-open-prs, area-labels-backfill] runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read # REQUIRED: this job's own script calls pulls.get directly (to fetch each PR before calling labelOne) + issues: write # labels/labels-list live under the Issues API even for PRs steps: - - uses: actions/checkout@v7 - - uses: actions/github-script@v9 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 + env: + PR_NUMBERS_JSON: ${{ needs.list-open-prs.outputs.json }} with: script: | const { labelOne } = require('${{ github.workspace }}/.github/scripts/pr-metadata-labeler.js'); const { owner, repo } = context.repo; - const prNumbers = ${{ needs.list-open-prs.outputs.json }}; + const prNumbers = JSON.parse(process.env.PR_NUMBERS_JSON); for (const pr_number of prNumbers) { - const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }); - await labelOne({ github, owner, repo, pr }); + try { + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }); + await labelOne({ github, owner, repo, pr }); + } catch (e) { + // One PR's failure (rate limit, deleted PR, anything) must + // never abort processing for every PR still queued behind + // it — this is exactly what happened before this fix: a + // single Search API rate-limit hit crashed the whole loop + // partway through a real backfill, silently leaving the + // rest of the PRs untouched. + core.warning(`#${pr_number}: failed during metadata backfill, skipping (${e.message})`); + } } activity-labels-backfill: needs: metadata-labels-backfill + if: always() # runs even if metadata-labels-backfill reported a failure — the two are independent (this script does its own full open-PR scan and doesn't depend on metadata-labels-backfill's output), so one weak spot shouldn't also block the other job that would otherwise work fine on its own runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read # REQUIRED: activity-labeler.js calls pulls.list/listCommits/listReviewComments/listReviews — the identical script gets pull-requests: write in ops.pr-activity-labeler.yml for the same reason + issues: write # labels/labels-list live under the Issues API even for PRs steps: - - uses: actions/checkout@v7 - - uses: actions/github-script@v9 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: script: | const run = require('${{ github.workspace }}/.github/scripts/activity-labeler.js'); diff --git a/tools/pr-labeler/README.md b/tools/pr-labeler/README.md index 2645ab484..600cd4825 100644 --- a/tools/pr-labeler/README.md +++ b/tools/pr-labeler/README.md @@ -42,6 +42,8 @@ tools/pr-labeler/ 2. Merge this to your default branch. Merging alone triggers `ops.label-sync.yml` (it watches `.github/labels.yml`) — check the Actions tab and confirm the full label catalog now exists under Issues → Labels. If it doesn't fire automatically, run it manually: **Actions → Sync Labels → Run workflow**. 3. Run **Actions → Backfill All PR Labels → Run workflow** once. This applies every label type — area, size, multi, first-contribution, and activity status — to every PR that was already open before this system existed. +**Note on scale**: the first-contribution check doesn't call GitHub's Search API at all — that endpoint has a much stricter *secondary* rate limit (30 requests/minute) than everything else this system uses, and calling it once per PR is exactly what caused a real backfill to fail partway the first time this ran at scale. It also doesn't fetch the repo's entire PR history to work this out (an earlier version did exactly that, which fixed the backfill case but reintroduced the same scaling problem on the far more frequent real-time path — see the comment above `isFirstContribution` in `pr-metadata-labeler.js` for why that didn't hold up). Instead, it uses GitHub's server-side `creator` filter to scope the query to just the one author being checked, and stops paginating the instant it's confirmed the answer — cheap in both the real-time (one PR per run) and backfill (many PRs per run) contexts, regardless of how large the repo's overall PR history is. On top of that, both the backfill loop and the activity scan's own loop isolate failures per PR: if any single PR errors for any reason, that failure is logged and skipped rather than aborting every other PR still queued behind it. If a run does still fail outright, it's always safe to just re-run it: every operation here checks current label state before changing anything, so re-running only picks up what's still missing. + From there it's automatic: new/updated PRs get area and size labels within seconds, and activity status updates instantly on new comments, new commits, and reviews, with a nightly scan as a backstop for anything time-based (a tier aging from day 6 to day 7 with no new activity, for instance) or anything the real-time triggers can't reach — see the caveat on forked PRs below. **Caveat on forked PRs**: `pull_request_review` and `pull_request_review_comment` have no fork-safe "_target" variant, so GitHub gives them a read-only token when the PR is from a fork (a platform limitation, not something fixable here). For PRs from branches within this repo — the normal case — this doesn't apply. If this repo ever accepts outside-fork contributions, a review left on a fork's PR won't update labels instantly through this specific trigger, but the nightly scan still catches it correctly within 24h.