Feat: Sweep approved fork PRs on a schedule - #455
Merged
keanjapesan merged 2 commits intoSep 24, 2026
Merged
keanjapesan merged 2 commits into
keanjapesan merged 2 commits into
Conversation
The fan-out tests (lfreleng-actions#422) carry a small evaluator for GitHub expressions and a runner for the enumerate job's script against a stubbed GraphQL endpoint. The scheduled sweep (lfreleng-actions#421) needs both, and a second copy would drift from the first, so they move to tests/workflow_harness.py. It is not a test module; pytest puts tests/ on sys.path, so test files import it by name. Two small changes come with the move. A page handed to the stub may now hold full GraphQL nodes as well as bare numbers, which the scheduled listing's filter needs, and the page types are Sequences so the type checkers accept lists of either. test_precommit_mypy_pins treated the new import as an unpinned third-party package, since its first-party set was a fixed list. It now derives the local modules from the tracked files in tests/, so the next shared helper will not need an edit there either. No test changes what it asserts. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
Closes lfreleng-actions#421. The comment re-check transfers an approved fork pull request once somebody posts '@github2gerrit check'. A caller of the reusable workflow that also subscribes to schedule now gets the zero-touch design: a maintainer approves in the ordinary way and the next sweep transfers the change. lfreleng-actions#421 recorded why the version first tried in lfreleng-actions#418 was withdrawn; each of its requirements holds here. Serialisation. A schedule fans out through the enumerate job exactly as a bulk dispatch does (lfreleng-actions#422): a leg per pull request, each in the group every event run for that pull request uses. A sweep leg and a comment re-check for the same commit, the collision lfreleng-actions#421 thought likeliest, queue behind each other rather than both creating a first Gerrit change. Scope. A scheduled listing keeps only what the gate could be holding back: a head not known to be in this repository, by head_repo_is_trusted's own rule and ignoring case, so a head whose repository no longer exists is still visited; and an approving review of the current head, which the gate cannot authorise without. The approval test only drops pull requests the gate would block anyway, and a review list GitHub truncated keeps the pull request. Who approved and whether changes are requested are still the leg's gate to decide. The listing query gains the fields this reads; a bulk dispatch still visits everything and ignores them. A schedule has no cleanup leg, so all 256 matrix jobs are pull requests, and a run with nothing to visit starts no legs. Idempotency. A pull request already transferred at its head still gets a leg, which reads the transfer record and stops (lfreleng-actions#419). The record is not read during enumeration: that would duplicate _sweep_can_skip's reading of the comment thread in jq. schedule joins RECHECK_EVENTS, which gives a scheduled leg what a comment re-check has. It runs as UPDATE, so an existing change gains a patchset rather than a sibling. The create-missing fallback lets the first transfer through. A same-repository head is left alone, behind the listing's own filter. The existing parametrised tests over that set now cover schedule too. The composite action accepts PR_NUMBER on schedule, which is how a leg names its pull request. Without one, or with 0, it fails and says why, rather than sweeping every pull request in one job: a composite action cannot start jobs, and one job over every pull request is the race lfreleng-actions#421 describes. tests/test_scheduled_sweep.py covers the routing, the lock a scheduled leg shares, the listing filter case by case against the stubbed API, the matrix limit, the action steps a leg passes through, and the recheck membership. Each was checked to fail against the behaviour it guards. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
ModeSevenIndustrialSolutions
requested review from
a team
and
a balanced review from Copilot
September 23, 2026 21:16
Copilot started reviewing on behalf of
ModeSevenIndustrialSolutions
September 23, 2026 21:16
View session
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces unattended Gerrit transfers and intricate workflow concurrency that warrant validation on real runners.
Review effort: Balanced
Findings: None
What changed in this PR
Adds scheduled, zero-touch processing of approved fork pull requests while preserving per-PR serialization and transfer idempotency.
Changes:
- Adds scheduled candidate enumeration and per-PR fan-out.
- Treats scheduled legs as privileged rechecks with validated PR numbers.
- Adds documentation and comprehensive workflow tests.
| File | Description |
|---|---|
.github/workflows/github2gerrit.yaml |
Implements scheduled enumeration and fan-out. |
action.yaml |
Accepts validated scheduled sweep legs. |
src/github2gerrit/models.py |
Classifies schedules as recheck events. |
README.md |
Documents scheduled reusable-workflow usage. |
docs/features.md |
Explains scheduling behavior and caveats. |
tests/workflow_harness.py |
Provides shared workflow-test utilities. |
tests/test_scheduled_sweep.py |
Covers filtering, locking, routing, and limits. |
tests/test_bulk_fan_out.py |
Reuses the shared test harness. |
tests/test_precommit_mypy_pins.py |
Recognizes test helper modules as first-party. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
keanjapesan
approved these changes
Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #421
A caller of the reusable workflow that subscribes to
schedulenow gets the zero-touch fork flow: a maintainer approves in the ordinary way, and the next sweep transfers the change. Nobody has to comment@github2gerrit check. That comment still works, and is still the faster route.This builds on #454. The per-PR fan-out and the transfer record were the two pieces #421 said a correct sweep needed.
Commits
Test: Share the reusable workflow test harness: a refactor that moves the expression evaluator and enumeration runner out oftest_bulk_fan_out.pyintotests/workflow_harness.py, so the new tests can reuse them. No assertion changes.test_precommit_mypy_pinsnow derives local test modules from the tracked files instead of a fixed list, so the next shared helper won't trip it either.Feat: Sweep approved fork PRs on a schedule: the feature.#421's requirements, one by one
Each transfer is serialised against per-PR runs. A schedule fans out through the
enumeratejob exactly as a bulk dispatch does: one leg per PR, each in the concurrency group every event run for that PR uses. A sweep leg and a comment re-check for the same commit — the collision #421 thought likeliest — now queue instead of both creating a first Gerrit change.The sweep visits only PRs the gate can block. A scheduled listing keeps a PR only when both of these hold:
head_repo_is_trusted's own rule: name for name, ignoring case. A head whose repository no longer exists (a deleted fork) is visited, because the gate applies to unresolved provenance.Same-repository PRs, including every automation PR, never enter a scheduled sweep. Bulk dispatch still visits everything. A schedule has no cleanup leg, and a run with nothing to visit starts no jobs.
An already-transferred PR is not submitted again. Its leg reads the #419 record and stops. The record isn't read during enumeration, since that would mean reimplementing
_sweep_can_skip's reading of the comment thread injq.Covered by tests, not just documented. See Validation below.
Tool side
schedulejoinsRECHECK_EVENTS, which gives a scheduled leg everything a comment re-check already has:UPDATEmode, so an existing change gains a patchset rather than a sibling.The existing parametrised tests over
RECHECK_EVENTSnow coverscheduleautomatically.The composite action accepts
PR_NUMBERonschedule, which is how a leg names its PR. Without one, or with0, the action fails with an explanation rather than sweeping every PR in one job. A composite action can't start jobs, and one job over every PR is exactly the race #421 describes.Trade-offs worth a reviewer's eye
headRefOid,headRepositoryand up to 100 approving reviews per PR. That is still about 1 GraphQL point per page of 100 PRs.Validation
uv run pytest tests/: full suite passes.ruff,mypyandbasedpyrightare clean.prekpasses on both commits (actionlint with shellcheck, yamllint, check-yaml, write-good, markdownlint, reuse, pytest), andgitlintpasses.aislop ci --changes --base upstream/main: 100/100, nothing new.zizmor --persona auditor: no findings ingithub2gerrit.yaml; the 3 pre-existing ones intesting.yamlare untouched.tests/test_scheduled_sweep.pycovers:pull_request_targetand comment runs, evaluated from the workflow's own expressions;RECHECK_EVENTSmembership.As with #454, this hasn't run on real runners. After merge, adding a
scheduletrigger totesting.yaml's reusable-workflow job, which runs withG2G_NO_GERRIT, would exercise it safely.