Skip to content

Commit 25b3d8f

Browse files
jhelferty-nvjhelferty-nv
committed
Address review: gate on token, real-time fork reviews, clearer docs
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.
1 parent 956e9aa commit 25b3d8f

6 files changed

Lines changed: 171 additions & 107 deletions

File tree

‎.github/workflows/pr-board-sync.yml‎

Lines changed: 72 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,11 @@ name: PR Board Sync (reusable)
3636
# inlined into the Reconcile step (so this reusable workflow needs no checkout
3737
# when called from another repo). That inlined block is the single source of
3838
# truth; .github/scripts/{pr-signal,pr-assign}.test.js extract it from this YAML
39-
# (via extract-workflow-js.js) and unit-test it. scripts/pr_sweep.py (run on a
40-
# cadence) remains the idempotent safety net for events this workflow cannot see
41-
# -- e.g. fork-PR CI, missed webhooks, a manual issue link, or repos not yet
42-
# onboarded -- and for board drift reconciliation.
39+
# (via extract-workflow-js.js) and unit-test it. A periodic sweep (run on a
40+
# cadence; see pr-sweep-nightly.yml / mode: sweep) remains the idempotent safety
41+
# net for what events cannot deliver -- missed webhooks or failed runs, a
42+
# manually linked issue or post-hoc issue assignment, board drift, repos not yet
43+
# onboarded -- and reconciles the whole board.
4344
#
4445
# Cross-repo reuse: any shader-slang repo adds a thin caller workflow that
4546
# declares the PR event triggers and calls this one, mapping the single required
@@ -66,7 +67,17 @@ name: PR Board Sync (reusable)
6667
#
6768
# Safety: every job runs only on PR metadata / API calls and never checks out or
6869
# executes PR code, so `pull_request_target` (secrets available for fork PRs) is
69-
# safe here.
70+
# safe here. This is also why the privileged `workflow_run` path (below) is safe.
71+
#
72+
# Triggers and fork secrets (PUBLIC repo): callers drive this via privileged
73+
# events that DO receive the secret for fork PRs -- `pull_request_target` (PR
74+
# lifecycle/commits), `check_suite` (CI), `workflow_run` (the fork-review relay),
75+
# and `schedule` (the sweep). `pull_request_review` only receives the secret for
76+
# ORIGIN PRs; a fork review runs the fork head with no secret, so the caller
77+
# routes fork reviews through an unprivileged bridge whose completion fires a
78+
# privileged `workflow_run` here (see the dispatch's `workflow_run` branch). When
79+
# no token is delivered, every privileged step is gated off (HAS_TOKEN) and the
80+
# run skips cleanly; the sweep is the backstop.
7081

7182
on:
7283
workflow_call:
@@ -156,11 +167,30 @@ jobs:
156167
concurrency:
157168
group: pr-board-sync-${{ github.event.pull_request.number || github.event.check_suite.head_sha || github.run_id }}
158169
cancel-in-progress: false
170+
# HAS_TOKEN is false when SLANG_PR_BOT_TOKEN is not delivered to the run. On a
171+
# PUBLIC repo, GitHub withholds secrets from fork-PR head-context events
172+
# (pull_request_review / pull_request); it also fails to resolve for a repo
173+
# that has not configured the secret. github-script hard-errors on an empty
174+
# github-token, so every privileged step is gated on this and skips cleanly --
175+
# the nightly sweep, a later pull_request_target event, or (for fork reviews)
176+
# the workflow_run path reconciles instead. pull_request_target, check_suite,
177+
# workflow_run, and schedule are privileged and DO receive the secret for forks.
178+
env:
179+
HAS_TOKEN: ${{ secrets.SLANG_PR_BOT_TOKEN != '' }}
159180
steps:
181+
# --- No-token breadcrumb: explain a skipped run -------------------------
182+
- name: Note skipped (no token)
183+
if: ${{ env.HAS_TOKEN != 'true' }}
184+
run: |
185+
echo "Skipping board sync: SLANG_PR_BOT_TOKEN is not available to this run."
186+
echo "(Fork-PR head-context event on a public repo, or unconfigured repo.)"
187+
echo "The nightly sweep / a later pull_request_target / the workflow_run path will reconcile."
188+
160189
# --- Classify Source (repo write-access read; opened/reopened only) ------
161190
- name: Classify PR Source
162191
id: classify
163192
if: >-
193+
env.HAS_TOKEN == 'true' &&
164194
github.event_name == 'pull_request_target' &&
165195
(github.event.action == 'opened' || github.event.action == 'reopened')
166196
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
@@ -209,6 +239,7 @@ jobs:
209239
210240
# --- Board writes: add + Source + Status (ProjectsV2 PAT) ---------------
211241
- name: Reconcile board (add, Source, Status)
242+
if: ${{ env.HAS_TOKEN == 'true' }}
212243
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
213244
env:
214245
MODE: ${{ inputs.mode }}
@@ -1137,6 +1168,29 @@ jobs:
11371168
}
11381169
}
11391170
1171+
// Resolve a commit SHA (and any pre-listed PRs) to its open PR(s) and
1172+
// reconcile each. check_suite and workflow_run are both commit-scoped
1173+
// and their pull_requests array is empty for fork PRs, so resolve via
1174+
// the API too. (One commit can head several PRs, hence a set.)
1175+
async function reconcileByCommit(headSha, prNodes) {
1176+
const numbers = new Set();
1177+
for (const p of (prNodes || [])) {
1178+
if (!p.state || p.state === "open") numbers.add(p.number);
1179+
}
1180+
if (headSha) {
1181+
try {
1182+
const assoc = await github.rest.repos.listPullRequestsAssociatedWithCommit({
1183+
owner: context.repo.owner, repo: context.repo.repo, commit_sha: headSha });
1184+
for (const p of (assoc.data || [])) {
1185+
if (p.state === "open") numbers.add(p.number);
1186+
}
1187+
} catch (error) {
1188+
core.warning(`could not resolve PRs for ${headSha} (${error.status})`);
1189+
}
1190+
}
1191+
for (const number of numbers) await reconcileStatus(number, null);
1192+
}
1193+
11401194
// --- Dispatch on the triggering event ----------------------------
11411195
const ev = context.eventName;
11421196
const action = context.payload.action;
@@ -1190,35 +1244,19 @@ jobs:
11901244
const itemId = await ensureItem(pr.node_id);
11911245
if (itemId) await reconcileStatus(pr.number, pr.node_id, itemId);
11921246
} else if (ev === "check_suite") {
1193-
// check_suite is commit-scoped, not PR-scoped. GitHub only lists
1194-
// *same-repo* PRs in check_suite.pull_requests (it is empty for fork
1195-
// PRs), so also resolve the suite's head SHA to its open PR(s) via
1196-
// the API - that covers fork PRs - and union the two. A CI
1197-
// completion recomputes the target, so a green run also recovers a
1198-
// Snagged/Revising PR. (One commit can head several PRs, hence a set.)
1247+
// check_suite is commit-scoped: a CI completion recomputes the
1248+
// target, so a green run also recovers a Snagged/Revising PR. Its
1249+
// pull_requests array is empty for fork PRs, so resolve by head SHA.
11991250
const suite = context.payload.check_suite || {};
1200-
const numbers = new Set();
1201-
for (const p of (suite.pull_requests || [])) {
1202-
if (!p.state || p.state === "open") numbers.add(p.number);
1203-
}
1204-
if (suite.head_sha) {
1205-
try {
1206-
const assoc = await github.rest.repos.listPullRequestsAssociatedWithCommit({
1207-
owner: context.repo.owner,
1208-
repo: context.repo.repo,
1209-
commit_sha: suite.head_sha,
1210-
});
1211-
for (const p of (assoc.data || [])) {
1212-
if (p.state === "open") numbers.add(p.number);
1213-
}
1214-
} catch (error) {
1215-
core.warning(
1216-
`could not resolve PRs for ${suite.head_sha} (${error.status})`);
1217-
}
1218-
}
1219-
for (const number of numbers) {
1220-
await reconcileStatus(number, null);
1221-
}
1251+
await reconcileByCommit(suite.head_sha, suite.pull_requests);
1252+
} else if (ev === "workflow_run") {
1253+
// Stage 2 of the fork-review relay (see header): a fork PR's review
1254+
// can't get secrets, so an unprivileged caller bridge fires on the
1255+
// review and completes; this privileged run resolves the PR from the
1256+
// upstream run's head SHA and reconciles. Metadata only -- NEVER add a
1257+
// checkout here (the upstream ran fork-controlled context).
1258+
const wr = context.payload.workflow_run || {};
1259+
await reconcileByCommit(wr.head_sha, wr.pull_requests);
12221260
}
12231261
12241262
# --- Unrequest auto-assigned non-approver reviewers --------------------
@@ -1232,6 +1270,7 @@ jobs:
12321270
# cadence sweep (scripts/pr_sweep.py) remains the backstop for both.
12331271
- name: Unrequest ignored reviewers
12341272
if: >-
1273+
env.HAS_TOKEN == 'true' &&
12351274
github.event_name == 'pull_request_target' &&
12361275
(github.event.action == 'opened' || github.event.action == 'reopened')
12371276
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8

‎.github/workflows/pr-maintenance.yml‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ name: PR Maintenance
77
#
88
# uses: shader-slang/slang/.github/workflows/pr-board-sync.yml@master
99
#
10-
# See .github/workflows/pr-board-sync.yml for the board IDs and required secrets.
10+
# pull_request_target and check_suite are privileged events that receive the
11+
# secret even for fork PRs, so they run here directly. pull_request_review only
12+
# receives the secret for ORIGIN PRs, so the job below skips it for forks; fork
13+
# reviews are handled by the pr-review-fork-bridge.yml -> pr-review-fork-apply.yml
14+
# (workflow_run) relay. See pr-board-sync.yml for the board IDs and the secret.
1115

1216
on:
1317
pull_request_target:
@@ -30,6 +34,11 @@ on:
3034

3135
jobs:
3236
board-sync:
37+
# Run directly for everything except a fork PR's review (no secret there);
38+
# fork reviews go through the workflow_run relay instead.
39+
if: >-
40+
github.event_name != 'pull_request_review' ||
41+
github.event.pull_request.head.repo.fork == false
3342
uses: ./.github/workflows/pr-board-sync.yml
3443
secrets:
3544
SLANG_PR_BOT_TOKEN: ${{ secrets.SLANG_PR_BOT_TOKEN }}

‎.github/workflows/pr-review-fork-apply.yml‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
name: PR Review Fork Apply
2+
3+
# Stage 2 of the fork-review relay for the PR board sync. Triggered by the
4+
# completion of "PR Review Fork Bridge" (pr-review-fork-bridge.yml).
5+
#
6+
# A workflow_run workflow runs in the base repo's PRIVILEGED context with secrets
7+
# even when the upstream run was a fork PR's review -- which is the only way to
8+
# update the board for fork-PR reviews on a public repo. The reusable workflow's
9+
# workflow_run dispatch branch resolves the PR from the upstream run's head SHA
10+
# and reconciles. It does NO checkout (metadata only), so executing it in this
11+
# privileged context is not a pwn-request vector.
12+
#
13+
# (workflow_run only fires for workflow files on the default branch, so this
14+
# activates once the workflow is merged to the default branch.)
15+
16+
on:
17+
workflow_run:
18+
workflows: ["PR Review Fork Bridge"]
19+
types: [completed]
20+
21+
jobs:
22+
board-sync:
23+
# Only when the bridge completed successfully (it always should; this guards
24+
# against a cancelled/failed relay run).
25+
if: ${{ github.event.workflow_run.conclusion == 'success' }}
26+
uses: ./.github/workflows/pr-board-sync.yml
27+
secrets:
28+
SLANG_PR_BOT_TOKEN: ${{ secrets.SLANG_PR_BOT_TOKEN }}

‎.github/workflows/pr-review-fork-bridge.yml‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
name: PR Review Fork Bridge
2+
3+
# Stage 1 of the fork-review relay for the PR board sync.
4+
#
5+
# On a PUBLIC repo, a pull_request_review on a PR from a FORK runs the fork's
6+
# head with no secrets, so it cannot update the board directly. This workflow is
7+
# that unprivileged stage: for fork-PR reviews only, it does nothing but complete
8+
# successfully, which lets a privileged `workflow_run` workflow
9+
# (pr-review-fork-apply.yml) fire and do the board update with secrets.
10+
#
11+
# It deliberately runs NO board logic and checks out NO code -- it exists solely
12+
# to relay the event. Origin-PR reviews are handled directly by pr-maintenance.yml,
13+
# so this is gated to forks to avoid a redundant relay run.
14+
15+
on:
16+
pull_request_review:
17+
types: [submitted]
18+
19+
permissions: {}
20+
21+
jobs:
22+
bridge:
23+
if: ${{ github.event.pull_request.head.repo.fork == true }}
24+
runs-on: ubuntu-latest
25+
steps:
26+
- name: Relay fork PR review to the privileged apply workflow
27+
run: |
28+
echo "Fork PR #${{ github.event.pull_request.number }} review submitted."
29+
echo "Completing so pr-review-fork-apply.yml (workflow_run) can reconcile with secrets."

‎CONTRIBUTING.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ Once a PR is created against `shader-slang/slang:master`, the PR will be merged
286286

287287
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.
288288

289-
> **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).
289+
> **Committers:** PRs are tracked on a shared project board whose status is updated automatically as a PR progresses. The board is an org project visible only to `shader-slang` org members, so outside contributors won't see it — that's expected. If you are a committer assigned to shepherd or review PRs, see [The Slang PR Tracking board](docs/maintainers/pr-review-board.md).
290290
291291
There are two cases where the workflow may fail for reasons that are not directly related to the change:
292292

‎docs/maintainers/pr-review-board.md‎

Lines changed: 31 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ board. It explains what the board's fields mean, what each `Status` tells you,
77
and when a PR actually needs your attention. (If you're just opening a PR, you
88
don't need this — see [CONTRIBUTING.md](../../CONTRIBUTING.md).)
99

10+
> The board is a `shader-slang` org project, so it is only visible to **org
11+
> members (committers)**. Outside contributors can't see it — that's expected;
12+
> their PRs are still tracked on it by the committers who shepherd them.
13+
1014
You never set the board fields by hand: they are maintained automatically from
1115
PR activity (open/close, pushes, reviews, CI results, merge-queue changes). Your
1216
job is to **read the state and act when it asks you to**.
@@ -16,7 +20,10 @@ job is to **read the state and act when it asks you to**.
1620
Every PR carries a **`Source`** — one of:
1721

1822
- **Internal** — opened by someone with write access to the repo. The **author
19-
drives it**; they're the assignee. No oversight is added.
23+
drives it**; they're the assignee, and **they are expected to identify and
24+
request their own reviewer** (no reviewer is auto-requested for Internal PRs).
25+
If you're a newer committer and unsure who to ask, pick someone who has
26+
recently touched the same files, or ask in the team channel.
2027
- **Community** — opened by an outside contributor. A maintainer (you) is
2128
assigned to shepherd it and arrange review.
2229
- **Bot** — opened by an automated coworker. A maintainer is assigned to
@@ -58,84 +65,36 @@ Two things worth knowing:
5865
- The **only** difference between the Bot and human flows is what a **CI failure**
5966
does (Bot → `Revising`, human → `Snagged`); everything else is identical.
6067

61-
## State diagrams
62-
63-
### Community / Internal (human) PRs
64-
65-
```mermaid
66-
stateDiagram-v2
67-
[*] --> InReview: opened (ready for review)
68-
[*] --> Revising: opened as draft
69-
70-
InReview: In Review
71-
Revising: Revising
72-
Snagged: Snagged
73-
Approved: Approved
74-
Done: Done
75-
76-
InReview --> Revising: changes requested
77-
InReview --> Snagged: CI failed / CI needs approval
78-
InReview --> Approved: approved (CI pending or in queue)
79-
InReview --> Snagged: approved + CI green + not queued
80-
81-
Revising --> InReview: new commit / marked ready
82-
Revising --> Snagged: CI failed / CI needs approval
83-
Revising --> Approved: approved (CI pending or in queue)
84-
85-
Snagged --> InReview: new commit / CI back to green
86-
Snagged --> Revising: changes requested
87-
Snagged --> Approved: approved (CI pending or in queue)
88-
89-
Approved --> Snagged: CI failed / dequeued / green + not queued
90-
Approved --> Revising: changes requested
91-
Approved --> InReview: new commit
92-
93-
InReview --> Done: closed
94-
Revising --> Done: closed
95-
Snagged --> Done: closed
96-
Approved --> Done: closed
97-
Done --> [*]
98-
```
68+
## The decision, as a flowchart
9969

100-
### Bot PRs
70+
The board does not move a PR along edges between states — it **recomputes** the
71+
status from the PR's current signals on every event. So the priority rules above
72+
are best read as a decision tree (first match wins):
10173

10274
```mermaid
103-
stateDiagram-v2
104-
[*] --> InReview: opened (incl. draft - kept visible)
105-
106-
InReview: In Review
107-
Revising: Revising
108-
Snagged: Snagged
109-
Approved: Approved
110-
Done: Done
111-
112-
InReview --> Revising: changes requested / CI failed
113-
InReview --> Snagged: CI needs approval
114-
InReview --> Approved: approved (CI pending or in queue)
115-
InReview --> Snagged: approved + CI green + not queued
116-
117-
Revising --> InReview: new commit / CI back to green
118-
Revising --> Approved: approved (CI pending or in queue)
119-
120-
Snagged --> InReview: new commit
121-
Snagged --> Revising: changes requested / CI failed
122-
Snagged --> Approved: approved (CI pending or in queue)
123-
124-
Approved --> Revising: changes requested / CI failed
125-
Approved --> Snagged: dequeued / green + not queued
126-
Approved --> InReview: new commit
127-
128-
InReview --> Done: closed
129-
Revising --> Done: closed
130-
Snagged --> Done: closed
131-
Approved --> Done: closed
132-
Done --> [*]
75+
flowchart TD
76+
ev([PR event - recompute]) --> closed{"Closed?"}
77+
closed -->|yes| done["Done"]
78+
closed -->|no| cr{"Changes requested<br/>on current commit?"}
79+
cr -->|yes| rev["Revising"]
80+
cr -->|no| draft{"Draft?"}
81+
draft -->|"yes (human)"| rev
82+
draft -->|"yes (bot)"| inrev["In Review"]
83+
draft -->|no| ciappr{"CI awaiting approval?"}
84+
ciappr -->|yes| snag["Snagged"]
85+
ciappr -->|no| cifail{"CI failed?"}
86+
cifail -->|"yes (bot)"| rev
87+
cifail -->|"yes (human)"| snag
88+
cifail -->|no| appr{"Approved?"}
89+
appr -->|"yes, green and not queued"| snag
90+
appr -->|"yes, pending or queued"| approved["Approved"]
91+
appr -->|no| inrev
13392
```
13493

13594
## Source of truth
13695

137-
These diagrams are an illustration. The authoritative rules live in the
96+
This flowchart is an illustration. The authoritative rules live in the
13897
`computeTarget` function in
13998
[`.github/workflows/pr-board-sync.yml`](../../.github/workflows/pr-board-sync.yml),
140-
which sets the board `Status` from PR events. If a diagram here ever disagrees
99+
which sets the board `Status` from PR events. If the diagram here ever disagrees
141100
with that function, the function is correct — please fix the diagram.

0 commit comments

Comments
 (0)