mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-07 06:16:06 -10:00
fix(ci): stop the PR merge review notifier from flooding the Actions queue (#19630)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Its GitHub Actions CI shares the `ubuntu-latest` runner pool across all workflows > - #19480 added the `PR merge review notification` workflow. It triggers on `workflow_run`, on every completed `check_run`, and on every commit `status` > - One PR push completes about 60 checks, so it queues about 60 notifier jobs. Each job takes a runner > - The Actions queue now holds about 340 queued runs. About 260 of them are this notifier. Other `ubuntu-latest` jobs wait more than one hour (for example, the `authorize` job of Runner Full-Stack E2E image builds) > - This pull request skips events that cannot complete a green PR state before a runner is allocated, and it collapses the remaining events to the newest one per PR head > - The benefit is that this unjams the Actions queue and keeps the advisory review notification ## Linked Issues or Issue Description Refs #19480 **What happened?** The `PR merge review notification` workflow from #19480 schedules one runner job for each `workflow_run`, `check_run`, and `status` event. One PR push makes about 60+ jobs. The repository Actions queue has about 340 queued runs (about 260 from this notifier at the time of writing). Other `ubuntu-latest` jobs wait more than one hour. **What did you expect?** The notifier requests one advisory merge review when PR verification is complete. It must not use a runner for each intermediate check. **Steps to reproduce** 1. Push to any PR. 2. Run `gh run list --workflow pr-merge-review.yml -L 200 --json event,status`. 3. See tens of queued `check_run`, `status`, and `workflow_run` runs for the same push. ## What Changed - `.github/workflows/pr-merge-review.yml`: the job `if:` now schedules the job only for events that can complete a green PR state: - `workflow_run` from `pull_request` or `pull_request_target` with conclusion `success`, `skipped`, or `neutral`. - `check_run` from a non-Actions app (app id is not `15368`) with conclusion `success`, `skipped`, or `neutral`. GitHub Actions check runs are already covered by the `workflow_run` list. - `status` with state `success`. - GitHub cannot filter `check_run` or `status` at the trigger level. A skipped job makes a run record but does not take a runner. - The job has a concurrency group `pr-merge-review-<PR head SHA>` with `cancel-in-progress: true`. The notifier reads the current PR state, so the newest event subsumes older ones. The group is on the job, not the workflow, so a skipped event cannot cancel a scheduled notifier. - The environment, permissions, trusted default-branch checkout, and secret handling do not change. - `.github/scripts/tests/notify-pr-merge-review.test.mjs`: a new test pins the trigger kinds, the job condition, and the job-level concurrency group. - `doc/PR-MERGE-REVIEW-WEBHOOK.md`: describes the event filter and the concurrency group. **Expected runner jobs per PR push** (typical PR: `PR`, `Storybook Visual`, `commitperclip PR Review` workflows; Greptile, Paperclip Review, Socket (2), Superagent (2) external checks; one Snyk status): | | Before | After | | --- | --- | --- | | Eligible events that take a runner | about 60–70 (every check run + 2 statuses + 3 workflow runs) | at most about 10 (3 workflow runs + 6 external checks + 1 status, only when green) | | Jobs that run to completion | about 60–70 | usually 1–4: events that arrive while a job is pending or running cancel it, so a burst collapses to the newest event. In a backed-up queue, all events for one head collapse to one job | ## Verification - `node --test .github/scripts/tests/notify-pr-merge-review.test.mjs`: 22 tests pass. - The new workflow test fails on the old workflow (red) and passes on the new workflow (green). - `actionlint .github/workflows/pr-merge-review.yml`: no findings. - I confirmed with `gh run list` that `workflow_run.head_sha` for the `pull_request_target` workflow (`commitperclip PR Review`) is the PR head SHA. So one SHA key groups all event kinds for one PR head. - I confirmed that all workflows that make PR checks are in the `workflow_run` list (`CI routing` and `Trusted PR CI` are reusable workflows called by `PR`). - After merge: check that `gh run list --workflow pr-merge-review.yml` shows mostly `skipped` runs for each push, and a small number of runs that execute. ## Risks - A check provider that reports a final green state through a GitHub Actions check run from a workflow that is not in the `workflow_run` list does not trigger the notifier. All current PR workflows are in the list. The doc already tells authors to add new PR workflows to the list. - A failed or cancelled check that later succeeds on re-run still triggers the notifier, because the re-run completes with `success`. - `cancel-in-progress` can cancel a notifier during delivery. The newer job evaluates the same or a more complete state, and the Paperclip idempotency key deduplicates deliveries for the same PR, head, and base. - Each event still makes a workflow run record. Skipped runs do not take a runner. - Low risk otherwise: the notifier is advisory and `continue-on-error`. It does not affect PR verification results. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use (shell, file edits, GitHub CLI). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
This commit is contained in:
1 parent
b5b972ceb1
commit
2e7c66aac9
3 files changed
+83
-3
No files matched your search
@@ -253,6 +253,58 @@ test("workflow uses read permissions, a protected environment, trusted checkout,
|
||||
}
|
||||
});
|
||||
|
||||
// Evaluates the small GitHub expression subset used by the notifier job condition.
|
||||
// Unknown functions or syntax fail the test instead of being silently approximated.
|
||||
function jobCondition(expression) {
|
||||
const source = expression.trim().replace(/\s+/g, " ");
|
||||
const allowed = /^(?:\s|\(|\)|&&|\|\||==|!=|,|'[^']*'|\d+|github(?:\.[a-z_]+)+|contains|fromJSON)+$/;
|
||||
assert.match(source, allowed, "Unsupported syntax in the notifier job condition");
|
||||
const javascript = source.replace(/github((?:\.[a-z_]+)+)/g, (_, path) => `github${path.replaceAll(".", "?.")}`)
|
||||
.replace(/==/g, "===").replace(/!===/g, "!==");
|
||||
const evaluate = new Function("github", "contains", "fromJSON", `return (${javascript});`);
|
||||
return (github) => evaluate(github, (list, value) => list.includes(value), (text) => JSON.parse(text));
|
||||
}
|
||||
|
||||
test("workflow skips non-final events before runner allocation and collapses bursts per PR head", () => {
|
||||
const workflow = readFileSync(".github/workflows/pr-merge-review.yml", "utf8");
|
||||
const triggers = workflow.slice(workflow.indexOf("\non:"), workflow.indexOf("\npermissions:"));
|
||||
// Exactly these trigger kinds; check_run and status cannot be filtered at the trigger level.
|
||||
assert.deepEqual([...triggers.matchAll(/^ {2}([a-z_]+):/gm)].map((match) => match[1]),
|
||||
["workflow_run", "check_run", "status"]);
|
||||
assert.match(triggers, /check_run:\n {4}types: \[completed\]/);
|
||||
const job = workflow.slice(workflow.indexOf("\n notify:"));
|
||||
const condition = job.slice(job.indexOf(" if: >-"), job.indexOf("\n # ", job.indexOf(" if: >-")));
|
||||
// GitHub Actions check runs arrive through workflow_run; only other providers schedule a job.
|
||||
assert.match(condition, /github\.event_name == 'check_run' &&\s+github\.event\.check_run\.app\.id != 15368/);
|
||||
assert.match(condition, /fromJSON\('\["success","skipped","neutral"\]'\), github\.event\.check_run\.conclusion/);
|
||||
assert.match(condition, /fromJSON\('\["success","skipped","neutral"\]'\), github\.event\.workflow_run\.conclusion/);
|
||||
assert.match(condition, /github\.event_name == 'status' && github\.event\.state == 'success'/);
|
||||
assert.match(condition, /github\.event\.workflow_run\.event == 'pull_request'/);
|
||||
assert.match(condition, /github\.repository == 'paperclipai\/paperclip'/);
|
||||
// Evaluate the condition for real events so an operator change cannot reopen the flood.
|
||||
const schedules = jobCondition(condition.replace(/^ {4}if: >-/, ""));
|
||||
const repository = { full_name: "paperclipai/paperclip" };
|
||||
const run = (event, conclusion) => ({ event_name: "workflow_run", repository: "paperclipai/paperclip",
|
||||
event: { repository, workflow_run: { event, conclusion, head_sha: head } } });
|
||||
const checkRun = (appId, conclusion) => ({ event_name: "check_run", repository: "paperclipai/paperclip",
|
||||
event: { repository, check_run: { app: { id: appId }, conclusion, head_sha: head } } });
|
||||
const status = (state) => ({ event_name: "status", repository: "paperclipai/paperclip",
|
||||
event: { repository, state, sha: head } });
|
||||
for (const accepted of [run("pull_request", "success"), run("pull_request_target", "neutral"),
|
||||
run("pull_request", "skipped"), checkRun(867647, "success"), checkRun(156372, "neutral"), status("success")]) {
|
||||
assert.equal(schedules(accepted), true, JSON.stringify(accepted));
|
||||
}
|
||||
for (const rejected of [run("pull_request", "failure"), run("pull_request", "cancelled"), run("push", "success"),
|
||||
run("schedule", "success"), checkRun(15368, "success"), checkRun(867647, "failure"),
|
||||
checkRun(867647, "action_required"), status("pending"), status("failure"), status("error"),
|
||||
{ ...status("success"), repository: "someone/fork" }]) {
|
||||
assert.equal(schedules(rejected), false, JSON.stringify(rejected));
|
||||
}
|
||||
// Job-level concurrency: a skipped event must not cancel a scheduled notifier.
|
||||
assert.doesNotMatch(workflow.slice(0, workflow.indexOf("\njobs:")), /concurrency:/);
|
||||
assert.match(job, /\n {4}concurrency:\n {6}group: pr-merge-review-\$\{\{ github\.event\.workflow_run\.head_sha \|\| github\.event\.check_run\.head_sha \|\| github\.event\.sha \}\}\n {6}cancel-in-progress: true\n/);
|
||||
});
|
||||
|
||||
test("API probe uses the same read permissions without private webhook access", () => {
|
||||
const probe = readFileSync(".github/workflows/pr-review-api-access.yml", "utf8");
|
||||
for (const permission of ["actions", "checks", "contents", "pull-requests", "statuses"]) {
|
||||
|
||||
@@ -16,6 +16,9 @@ on:
|
||||
- PR review API access
|
||||
types: [completed]
|
||||
# External check providers and legacy commit statuses can finish after CI.
|
||||
# GitHub has no trigger-level filter for these events, so the job condition
|
||||
# below skips them before a runner is allocated. GitHub Actions check runs
|
||||
# are covered by the workflow_run list above.
|
||||
check_run:
|
||||
types: [completed]
|
||||
status:
|
||||
@@ -29,11 +32,27 @@ permissions:
|
||||
|
||||
jobs:
|
||||
notify:
|
||||
# A skipped job does not take a runner. Only events that can complete a
|
||||
# green PR state schedule the job: successful, skipped, or neutral PR
|
||||
# workflow runs, the same results from non-Actions check providers (GitHub
|
||||
# Actions app 15368 checks arrive through workflow_run), and successful
|
||||
# commit statuses. One PR push previously queued about 60 runner jobs.
|
||||
if: >-
|
||||
github.repository == 'paperclipai/paperclip' &&
|
||||
(github.event_name != 'workflow_run' ||
|
||||
github.event.workflow_run.event == 'pull_request' ||
|
||||
github.event.workflow_run.event == 'pull_request_target')
|
||||
((github.event_name == 'workflow_run' &&
|
||||
(github.event.workflow_run.event == 'pull_request' ||
|
||||
github.event.workflow_run.event == 'pull_request_target') &&
|
||||
contains(fromJSON('["success","skipped","neutral"]'), github.event.workflow_run.conclusion)) ||
|
||||
(github.event_name == 'check_run' &&
|
||||
github.event.check_run.app.id != 15368 &&
|
||||
contains(fromJSON('["success","skipped","neutral"]'), github.event.check_run.conclusion)) ||
|
||||
(github.event_name == 'status' && github.event.state == 'success'))
|
||||
# Collapse bursts for one PR head to the latest event. The notifier reads
|
||||
# the current PR state, so the newest event subsumes older ones. Job-level
|
||||
# concurrency keeps skipped events from cancelling a scheduled notifier.
|
||||
concurrency:
|
||||
group: pr-merge-review-${{ github.event.workflow_run.head_sha || github.event.check_run.head_sha || github.event.sha }}
|
||||
cancel-in-progress: true
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 5
|
||||
# Restrict this environment to master. Repository secrets would also be
|
||||
|
||||
@@ -34,6 +34,15 @@ PR code or downloads PR artifacts or caches. It reacts to the repository's PR
|
||||
workflow completions, external check completions, and legacy commit statuses.
|
||||
When adding another PR verification workflow, add its name to `workflow_run`.
|
||||
|
||||
GitHub cannot filter `check_run` or `status` triggers, so the job condition
|
||||
skips events that cannot complete a green state before a runner is allocated.
|
||||
Only successful, skipped, or neutral PR workflow runs and non-Actions check runs,
|
||||
and successful commit statuses, schedule the job. GitHub Actions check runs are
|
||||
skipped because their workflow completions already arrive through `workflow_run`.
|
||||
A job-level concurrency group keyed by the PR head SHA cancels older pending or
|
||||
running notifiers, because the newest event sees the most complete PR state.
|
||||
The group is on the job so skipped events cannot cancel a scheduled notifier.
|
||||
|
||||
The separate `PR review API access` workflow checks notifier changes using
|
||||
the real read-only Actions token. It has no private environment or webhook
|
||||
credentials and sends no webhook. Both workflows explicitly grant
|
||||
|
||||
Reference in new issue
Block a user