Add reusable PR board-sync workflow (status + assignment) for org-wide reuse#11740
Add reusable PR board-sync workflow (status + assignment) for org-wide reuse#11740jhelferty-nv wants to merge 15 commits into
Conversation
Move the event-driven half of the slang PR board state machine into a reusable GitHub Actions workflow (pr-board-sync.yml) so every shader-slang repo can reconcile PRs onto the shared "Slang PR Tracking" board per-event: - add PR to board + classify/set Source (Internal/Community/Bot) - advance Status on the happy path (opened->Revising, closed->Done, converted_to_draft->Revising, changes-requested->Revising, CI failed->Revising, CI passed + not draft->Todo) - unrequest auto-assigned non-approver reviewers (e.g. bmillsNV) slang dogfoods it via a thin caller (pr-maintenance.yml) using the local workflow path; other repos call shader-slang/slang/.github/workflows/ pr-board-sync.yml@master with secrets: inherit. This supersedes and replaces add-pr-to-project.yml. Owner/reviewer assignment (committer-signal ranking) and full drift reconciliation remain in the cadence-run pr_sweep.py, which is idempotent and acts as the safety net for events Actions cannot see (fork-PR CI, missed webhooks, un-onboarded repos).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR automation is split into a reusable board-sync workflow and a maintenance wrapper. The reusable workflow now handles source classification, status reconciliation, event-driven updates, and ignored reviewer cleanup. ChangesPR Board Sync Workflow
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b98f30a-0e43-451c-8701-a36fa6b4549f
📒 Files selected for processing (3)
.github/workflows/add-pr-to-project.yml.github/workflows/pr-board-sync.yml.github/workflows/pr-maintenance.yml
💤 Files with no reviewable changes (1)
- .github/workflows/add-pr-to-project.yml
Replace the Revising/Todo/In Progress/Done event logic with the In Review / Revising / Snagged / Approved / Done machine, keeping computeTarget identical to target_status in the slang-pr-maintenance cadence sweep so the two never fight. - computeTarget is the single source of truth: changes-request -> Revising; draft -> In Review (Bot) / Revising (human); CI failed -> Revising (Bot) / Snagged (human); approved -> Snagged if green & un-queued else Approved; else In Review. Stale review feedback (a commit after the last review) falls back to In Review. - Every state-changing event routes through reconcileStatus(computeTarget); closed -> Done and dequeued -> Snagged stay explicit. Subscribe to enqueued so entering the queue clears a transient Snagged (closes the CI-pass/enqueue race). - check_suite resolves PRs by the suite head SHA via the API (covers fork PRs, whose check_suite.pull_requests is empty) unioned with the payload list. - Classify Internal vs Community by repo write access (not org membership), and resolve is_bot from the board Source when set (else author); only backfill Source when the board has none, preserving manual overrides. - Drop the ready-for-review PR comment and the coverage gate. - TODO noted: align the bmillsNV unrequest with the sweep (assignment-tied).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/pr-board-sync.yml (2)
465-479: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid undoing intentional reviewer requests.
This fires on
review_requestedand removes any matching ignored reviewer from the PR, so a maintainer manually requesting that reviewer is immediately undone. The comment says the desired behavior is to remove auto-assigned non-approvers only; narrow this trigger or move the cleanup to the sweep where assignment intent is known.🐛 Minimal trigger narrowing
- github.event.action == 'ready_for_review' || - github.event.action == 'review_requested') + github.event.action == 'ready_for_review')I can help wire a safer trigger or open a follow-up issue for the sweep-based cleanup.
Also applies to: 490-498
296-337: 🗄️ Data Integrity & Integration | 🟠 MajorUse the review’s commit to determine freshness.
reviews(last: 1)+committedDatecan mark stale approvals/change-requests as current when a newer comment-only review exists, or when an older commit was pushed later. FilterlatestOpinionatedReviewsby the currentheadRefOid/headCommit.oidand derivereviewedfrom that set instead of review timestamps.
♻️ Duplicate comments (1)
.github/workflows/pr-maintenance.yml (1)
14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the
typeslist in the repo’s multiline YAML style.This is the same formatting issue already flagged on this line, so I’m not repeating the full analysis here. Reformat to avoid formatter/CI failures.
Source: Pipeline failures
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dee8a271-76a5-4bcf-83aa-f96848f7995c
📒 Files selected for processing (2)
.github/workflows/pr-board-sync.yml.github/workflows/pr-maintenance.yml
- Fix the prettier formatting check (multi-line types list in pr-maintenance.yml).
- Tighten token scope: top-level permissions: {} (deny-by-default) and grant the
job only pull-requests: write; add an explicit permissions block to the caller.
- Route CI action_required (a fork PR awaiting maintainer approval to run CI) to
Snagged in computeTarget, matching target_status in slang-pr-maintenance.
- Replace secrets: inherit with explicit workflow_call.secrets declaration and
caller-side mapping of ADD_TO_PROJECT_PAT and SLANGBOT_MEMBERS_READONLY.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-board-sync.yml (1)
481-495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid unrequesting explicitly requested reviewers.
This runs on
review_requested, so a maintainer who intentionally requests an ignored reviewer can have that request immediately removed. That conflicts with the stated “auto-assigned non-approver reviewers” behavior and the TODO’s preferred sweep semantics.At minimum, drop
review_requestedfrom this trigger or move this cleanup back to the cadence sweep path that knows it performed the assignment.Proposed narrowing
- github.event.action == 'ready_for_review' || - github.event.action == 'review_requested') + github.event.action == 'ready_for_review')Do you want me to draft the sweep-aligned implementation?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09bee1f9-e90a-43a9-803d-64dc1bf686db
📒 Files selected for processing (2)
.github/workflows/pr-board-sync.yml.github/workflows/pr-maintenance.yml
CodeRabbit flagged that the unrequest step ran on review_requested, so a maintainer manually requesting an ignored reviewer (e.g. bmillsNV) was undone immediately. Restrict it to opened/reopened (the onboarding moment, when repo automation auto-adds the reviewer); the cadence sweep remains the backstop and removes it at assignment. Drop review_requested from the caller triggers (it no longer drives anything) and the now-moot review_requested guard on the board step.
|
Addressed the two outside-diff review comments:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-board-sync.yml (1)
306-347: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTie opinionated review freshness to the reviewed commit.
latestOpinionatedReviewssupplies the current opinion states, butreviews(last: 1)can be a later comment-only review. That lets an oldCHANGES_REQUESTED/APPROVEDon a previous head appear current, moving the board toRevising/Approvedincorrectly. Query each opinionated review’s commit and compare it with the PR head SHA instead.🐛 Proposed direction
- id isDraft state + id isDraft state headRefOid author { login __typename } mergeQueueEntry { id } - latestOpinionatedReviews(first: 100) { nodes { state } } - reviews(last: 1) { nodes { submittedAt } } + latestOpinionatedReviews(first: 100) { + nodes { state commit { oid } } + } commits(last: 1) { nodes { commit { committedDate statusCheckRollup {- const opinions = (pr.latestOpinionatedReviews?.nodes || []).map(r => r.state); + const opinions = (pr.latestOpinionatedReviews?.nodes || []) + .filter(r => r.commit?.oid === pr.headRefOid) + .map(r => r.state); const changeRequested = opinions.includes("CHANGES_REQUESTED"); const approved = opinions.includes("APPROVED") && !changeRequested; - // A review counts only if no commit landed after it (a newer commit - // makes the feedback stale -> the PR is back to In Review). - const headAt = headCommit.committedDate; - const lastReviewAt = pr.reviews?.nodes?.[0]?.submittedAt || null; - const reviewed = !(headAt && lastReviewAt && new Date(headAt) > new Date(lastReviewAt)); + const reviewed = opinions.length > 0;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60f0b9c0-acc3-4f2e-af4a-64f2ef7e4e2d
📒 Files selected for processing (2)
.github/workflows/pr-board-sync.yml.github/workflows/pr-maintenance.yml
💤 Files with no reviewable changes (1)
- .github/workflows/pr-maintenance.yml
Per CodeRabbit: a review opinion now counts only when it was made on the current head commit (latestOpinionatedReviews[].commit.oid == head commit oid), so a comment-only review after a fix-commit, or a late-pushed older commit, can no longer revive stale feedback. Drops the committedDate / last-review timestamp comparison; changeRequested/approved carry the currency, so computeTarget no longer needs a separate reviewed gate.
|
Follow-up on the review-freshness comment: implemented in both halves (this workflow's |
Collapse the two PATs (ADD_TO_PROJECT_PAT, SLANGBOT_MEMBERS_READONLY) and the implicit GITHUB_TOKEN into one dedicated bot PAT used for every GitHub call. Callers now map one secret and need no permissions block, so growing the workflow's capabilities (e.g. adding committer-signal assignment) widens the PAT's scopes centrally instead of editing every consuming repo's caller. Drop the job permissions block since the GITHUB_TOKEN is now unused.
Document the shared Slang PR Tracking board for assignees: the Source field, the five Status states framed around when a human must act, the state-precedence rules, and the human/Bot state diagrams. Point to the computeTarget function in pr-board-sync.yml as the source of truth, and link the guide from CONTRIBUTING.md.
Add a shared reconcileAssignment step that backfills an unassigned PR's assignee (and requests them as reviewer), mirroring the signal-free half of pr_sweep.py's selection: Internal -> author; Community/Bot -> the first linked-issue assignee who is a member of the owners team. The committer- signal ranking, the maintainer fallback, and the extra dev-reviewer remain the cadence sweep's job. Subscribe to the edited event so a Fixes #N added after open is caught, and fetch assignees/reviewRequests/closingIssues in prInfo to drive the pick.
Replicate the cadence sweep's full assignee/reviewer selection (committer- signal ranking + selection, ported from pr_signal.py / pr_common.py) in the board-sync workflow, resolving the fallback assignee from the slang-maintainer team. Inline the JS into the reusable workflow so it needs no checkout when called cross-repo, and make that inlined block the single source of truth: extract-workflow-js.js pulls it back out so the unit tests run against exactly what the workflow executes. Add a Check Workflow Scripts CI job that runs every .github/scripts/*.test.js on workflow/script changes.
Parameterize extract-workflow-js.js on {workflow, block} (repo-relative or
absolute path; a validated extract-js:<block>:begin/end marker pair) so it is
not tied to pr-board-sync.yml or the assignment block. Rename the markers to
the generic extract-js:assignment scheme and have the tests pass the workflow
path + block name.
Add a fallback_assignee input (default bmillsNV) used as the terminal pick when maintainer_team yields nobody, so a non-Internal PR is never left unassigned. Resolve it via resolveMaintainer (team sorted-first, else fallback), and never request an ignored non-approver as a reviewer, which can leave the reviewer set empty.
Add a per-PR concurrency group so near-simultaneous events for one PR cannot double-write (cancel-in-progress: false queues the next). Include the edited action in the recompute-branch comment, which the edited trigger now routes there.
jhelferty-nv
changed the title
Add a mode input (event default / sweep) to the reusable workflow: in sweep mode it enumerates every open PR in the repo and reconciles each through the same per-PR engine the event path uses (Source, Status, assignment), so a scheduled cadence run shares all logic with no duplication or checkout. Make reconcileStatus sweep-safe: classify + backfill Source when the board has none, write Status only when it changes (no nightly re-timelining), and share a run-scoped committer-signal cache across PRs. Add pr-sweep-nightly.yml, a scheduled (and manually dispatchable) caller that invokes mode: sweep.
| When the conditions above are all met, you will have a chance to rewrite the commit message. Since the Slang repo uses the "squash" strategy for merging, multiple commits in your PR will become one commit. By default, GitHub will concatenate all of the commit messages sequentially, but often it is not readable. Please rewrite the final commit message in a way that people can easily understand what the purpose of the commit is. | ||
|
|
||
| > **Maintainers:** PRs are tracked on a shared project board whose status is updated automatically as your PR progresses. If you are assigned to shepherd or review PRs, see [The Slang PR Tracking board](docs/maintainers/pr-review-board.md). | ||
|
|
There was a problem hiding this comment.
This project board isn't visible to non-org members. I'm not sure whether the contributing.md is the right place to document it-- or perhaps we should note that the PR tracking boards are only accessible to committers.
There was a problem hiding this comment.
Good point — reworded this so it's framed for committers and notes the board is a shader-slang org project visible only to org members (outside contributors won't see it, which is expected). Added the same note at the top of the maintainer guide. (25b3d8f)
|
|
||
| ### Community / Internal (human) PRs | ||
|
|
||
| ```mermaid |
There was a problem hiding this comment.
The state diagrams are rendering pretty illegibly, at least in the GitHub PR preview. It's not possible to really tell where arrows go from/to, and some of the blocks in the diagram overlap.
There was a problem hiding this comment.
Agreed they were illegible. Root cause: the status is recomputed from the PR's current signals on every event (it's a pure function, not a transition system), so a state diagram is near-complete-graph. Replaced both diagrams with a single decision flowchart of the first-match-wins ladder, which renders as a clean tree. (25b3d8f)
| // owner -> maintainer; reviewers = assignee + top dev-not-owner unless a | ||
| // real reviewer is already requested). Internal PRs go to the author with | ||
| // no requested reviewer. Idempotent: only ever touches an UNASSIGNED PR, | ||
| // so re-running on any event is a no-op once an owner is set. |
There was a problem hiding this comment.
Is there any expected case where we might want a PR to be explicitly unassigned? I can't think of one, but if there is, having some explicit way to tell the bot to stop reassigning if assignees are un-set. (I think this is probably moot, though.)
There was a problem hiding this comment.
Agreed it's probably moot — the design intentionally keeps an assignee (always-have-an-assignee). If a real need for a deliberately-unassigned PR comes up, we can add a label-gated opt-out (skip re-assignment when e.g. a no-auto-assign label is present) in both the workflow and the sweep. Leaving it out for now.
| shepherd it to ready-for-review and merge. | ||
|
|
||
| Being the assignee on a **Community** or **Bot** PR means you're responsible for | ||
| moving it forward (or finding the right reviewer). |
There was a problem hiding this comment.
We may want to be explicit that if you are an internal author / have write access, you're expected to identify the reviewer yourself (if I'm understanding the state logic correctly). This may be something new committers from outside NV might need some additional help or suggestions with, I'm not sure.
There was a problem hiding this comment.
Good call — added a note in the guide under the Internal source: Internal (write-access) PRs get no auto-requested reviewer; the author is expected to identify and request their own reviewer, with a hint for newer committers (pick someone who recently touched the same files, or ask in the team channel). (25b3d8f)
Make board-sync robust and fully real-time across origin and fork PRs, and address Shannon's review comments: - Gate the privileged github-script steps on HAS_TOKEN so a run without the bot token (a fork PR's head-context event on a public repo, or an unconfigured repo) skips cleanly instead of hard-failing github-script. This fixes the failing board-sync check. - Add a workflow_run dispatch branch and a fork-review relay: pull_request_target and check_suite stay privileged for forks; origin reviews run directly; fork reviews route through pr-review-fork-bridge.yml (unprivileged) -> pr-review-fork-apply.yml (workflow_run, privileged) which resolves the PR by head SHA. No checkout anywhere, so the privileged path is not a pwn-request vector. - Replace the two crowded state diagrams with one decision flowchart (status is recomputed each event, not transitioned, so a decision tree reads far better). - Note the board is visible to org members (committers) only, in CONTRIBUTING.md and the maintainer guide. - Clarify that Internal (write-access) authors identify and request their own reviewer.
2 participants
Motivation
PRs across the shader-slang org are triaged on the shared Slang PR Tracking
project board, but the only automation was
add-pr-to-project.yml, which firedon
opened, added the PR to the board, and set aSourcefield — nothing keptStatuscurrent or routed the PR to an owner. Everything else was manual orleft to a periodic sweep, run out-of-band. So a PR that got CI failures, a
changes-requested review, or fell out of the merge queue did not reflect that on
the board until the next sweep, and community/bot PRs sat unassigned.
Concretely: a community contributor opens a PR that
Fixes #123(an issuealready owned by a maintainer). Previously the board showed only "Community" with
no status and no assignee until a human (or the periodic sweep) intervened. We
want the board to show, in real time, that the PR is In Review, and to assign
the issue's owner — the same decision the sweep would eventually make.
Proposed solution
Move the event-driven half of the slang-pr-maintenance state machine into a
reusable workflow,
pr-board-sync.yml, that every shader-slang repo can call sothe board reconciles per PR event. It is the real-time counterpart to the cadence
sweep and is kept behaviorally identical to it, so the two never fight:
Done) recomputed from observed PR state on every relevant event, via a
computeTargetthat is a line-for-line mirror of the sweep's status logic.inlined into the workflow (see below) so a non-Internal PR is routed to a
linked-issue owner → top committer-signal owner → the maintainer, and the
maintainer is resolved from the
slang-maintainerteam (elsebmillsNV) so aPR is never left unassigned.
Inlining the assignment JS (rather than
require-ing a checked-out file) is theprincipled choice for a reusable workflow: a checkout inside a reusable
workflow lands the caller's repo, not this one, so a required file would be
absent for every other repo. To keep the inlined logic testable, it is delimited
by
extract-js:assignmentsentinels and the unit tests extract that exact blockfrom the YAML at run time — making the workflow the single source of truth.
Full board reconciliation is provided as a
mode: sweepof the sameworkflow, driven by a nightly scheduled caller (
pr-sweep-nightly.yml): itenumerates every open PR in the repo and runs the identical per-PR engine — the
idempotent backstop for what the event stream cannot deliver (missed webhooks or
failed runs, an issue assigned or linked after the fact, board drift,
not-yet-onboarded PRs). Sharing the one engine means no second copy of the logic.
(Its broader, org-wide form is still being decided.)
Change summary
.github/workflows/pr-board-sync.ymlmode: sweepthat reconciles every open PR in the repo..github/workflows/pr-maintenance.yml.github/workflows/pr-sweep-nightly.ymlmode: sweepnightly..github/workflows/add-pr-to-project.yml.github/scripts/pr-signal.test.js,pr-assign.test.js.github/scripts/extract-workflow-js.jsextract-js:<block>region from any workflow YAML and load it as a module..github/workflows/check-workflow-scripts.yml.github/scripts/*.test.json workflow/script changes.docs/maintainers/pr-review-board.mdCONTRIBUTING.mdConcepts and vocabulary
(author has repo write access), Community (outside contributor), or Bot.
(human draft, or changes requested, or — for a Bot PR — a CI failure), Snagged
(needs a human: a human PR's CI failure, CI awaiting approval, or approved+green
but not enqueued), Approved (approved, waiting only on CI/merge queue), Done
(closed). Bot and human flows differ only on a CI failure.
recent LOC they have on the PR's changed files (the same ranking the sweep uses).
commit.oidequals thePR head; a new push supersedes earlier feedback (commit-identity, not timestamps).
extract-js:<block>markers — sentinel comments around the inlined JS soextract-workflow-js.jscan pull it back out for unit testing.Process report
pr-board-sync.ymlis a reusable workflow onpull_request_targetsosecrets are available for fork PRs; it never checks out or runs PR code, and
reads PR-controlled data only as GraphQL variables /
JSON.stringify-escapedquery fragments.
permissions: {}denies the ambientGITHUB_TOKEN— everycall uses the PAT.
change vs.
add-pr-to-project.yml). Internal means the author can approveworkflow runs and approve/merge here; org members without write to this repo are
Community. This matches the sweep's Source classification and is the property
the routing actually depends on. On a read error it fails safe to Community.
reconcileStatus→computeTarget(observed state)rather than hard-codingper-event edges, so the event machine and the sweep cannot disagree;
closed(→ Done) and
dequeued(→ Snagged) are the only hard-coded edges.reconcileAssignmentonly evertouches an unassigned PR (gated on unassigned / not-merge-queued / not a human
draft), so re-running on any event is a no-op once an owner is set, and the pick
is a pure function of observed state (so concurrent runs converge — see the
per-PR
concurrencygroup). The fallback assignee resolvesslang-maintainer(sorted-first) →bmillsNV; an ignored non-approver isassigned but never requested as a reviewer (the reviewer set may be empty).
extract-workflow-js.jsand exercised bypr-signal.test.js/pr-assign.test.js;check-workflow-scripts.ymlruns them in CI so a change tothe inlined block is what gets tested. Indent-on-inline / dedent-on-extract are
exact inverses, so the tests run byte-identical code.
editedis subscribed so aFixes #Nadded after open is caught;check_suiteresolves its head SHA to associated PRs so fork-PR CI is covered(its
pull_requestsarray is empty for forks).mode: sweepbranch of the same workflow, not a secondimplementation: it enumerates open PRs and calls the same
reconcileStatus.Two guards keep a nightly pass quiet — Status is written only when it changes
(no re-timelining of unchanged cards), and a run-scoped committer-signal cache
fetches each file's history once across all PRs. In sweep mode
reconcileStatusalso classifies + backfills
Sourcewhen the board has none (the classify steponly runs on
opened/reopened), matching the periodic sweep.Migration notes (for adopters / branch protection)
add-pr-to-project.ymlused(
SLANGBOT_MEMBERS_READONLY+ADD_TO_PROJECT_PAT) with a singleSLANG_PR_BOT_TOKEN(org Projects RW + Members read; repo Contents read,Pull requests write, Issues write, Metadata read). Callers map it explicitly
(no
secrets: inherit). Other repos adopting the reusable workflow must set it.Project Board" to the board-sync job; update branch-protection required checks
accordingly.
Full board reconciliation is intentionally left to a separate periodic sweep,
kept behaviorally identical to this workflow (its final form is still being
decided).
Review follow-up (latest commit)
Addressing review feedback and making fork-PR handling fully real-time:
github-scriptsteps are gated on the bottoken being present, so a run that doesn't receive it (a fork PR's
head-context event on a public repo, or a repo that hasn't configured the
secret) skips cleanly instead of hard-failing. This fixes the failing
board-synccheck.pull_request_targetandcheck_suiteare privilegedfor forks and run directly.
pull_request_reviewonly gets secrets for originPRs, so fork reviews route through a 2-stage relay: an unprivileged
pr-review-fork-bridge.ymlfires on the review, and a privilegedpr-review-fork-apply.yml(workflow_run) resolves the PR by head SHA andreconciles (a new
workflow_rundispatch branch in the reusable workflow). Nocheckout anywhere, so the privileged path is not a pwn-request vector.
decision flowchart (status is recomputed per event, not transitioned); noted
the board is visible to org members/committers only; clarified that Internal
(write-access) authors identify and request their own reviewer.
Note: the
pull_request_target/check_suite/workflow_runpaths read theworkflow from the default branch, so the real-time behavior activates once this
merges to
master.