Skip to content

Commit 19e744b

Browse files
kyli87kyli87
authored
Merge branch 'main' into kenli/plan-ffi-iter
2 parents 43fd26d + ae7a2b8 commit 19e744b

107 files changed

Lines changed: 5390 additions & 1771 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/SECURITY_MODEL.md‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# Benchmark workflow security model
2+
3+
Scope: [`benchmark.yml`](workflows/benchmark.yml) (runs bench on PR head) and
4+
[`benchmark-post-comment.yml`](workflows/benchmark-post-comment.yml) (posts the
5+
result comment in base-branch context). Other workflows in this repo may follow
6+
different patterns -- this document is specific to the benchmark pair.
7+
8+
## Running untrusted PR code
9+
10+
The `run-benchmark` job in `benchmark.yml` checks out the PR head and runs
11+
its build + bench harness. To bound what that code can do:
12+
13+
- Workflow-level `permissions: {}` is the default; each job opts in to the
14+
minimum scope it needs. `run-benchmark` declares only `contents: read`,
15+
so any leaked GITHUB_TOKEN has no write power. Fork PRs additionally get
16+
a read-only token from GitHub by default under the `pull_request` event.
17+
- `actions/checkout` uses `persist-credentials: false` so the token isn't
18+
left in the local git config after checkout.
19+
- `cargo install critcmp` runs *before* the PR-head checkout, so the PR's
20+
`.cargo/config.toml` cannot redirect the registry for that install.
21+
22+
Do not add other secrets or grant write permissions to `run-benchmark`.
23+
24+
## Rust-cache poisoning
25+
26+
`Swatinem/rust-cache` is configured with
27+
`save-if: github.event_name == 'push' && github.ref == 'refs/heads/main'`.
28+
PR runs read-restore from main's cache; they never save. The
29+
`github.event_name == 'push'` clause is the load-bearing half -- the
30+
benchmark workflow has no `push` trigger outside of `push: main` for the
31+
cache-warming job, so PR runs cannot match this condition and therefore
32+
cannot write the cache.
33+
34+
Do not relax the `github.event_name == 'push'` clause. If you do, a PR-head
35+
run can land compile artifacts in main's cache scope (poisoning subsequent
36+
runs) or evict main's entries through the shared cache budget.
37+
38+
## Cross-PR comment forgery
39+
40+
`benchmark-post-comment.yml` runs in base-branch context with
41+
`pull-requests: write`. It must derive the target PR number from a source
42+
the fork cannot forge. The choice depends on which event triggered the
43+
upstream bench workflow:
44+
45+
- **`pull_request` upstream:** use `gh pr view --repo "$REPO" "owner:branch"`,
46+
where `owner:branch` is built from `workflow_run.head_repository.owner.login`
47+
and `workflow_run.head_branch`. Both fields are populated by GitHub from the
48+
trusted workflow_run event payload, not from anything the fork wrote.
49+
- **`issue_comment` upstream:** trust the `bench-pr-number` artifact. For
50+
`issue_comment` events GitHub serves `benchmark.yml` from the default
51+
branch (not the PR head), so the "Stash PR number" + "Upload PR number"
52+
steps run trusted code. The upload happens *before* `actions/checkout`
53+
brings PR code onto the runner, so once the artifact is on GitHub's
54+
storage it is immutable for the rest of the run -- the bench script
55+
cannot alter it, and `contents: read` denies any API-level mutation.
56+
57+
`workflow_run` itself runs the post-comment workflow from the default
58+
branch (a GitHub guarantee), so a fork PR cannot modify the resolver to
59+
trust the wrong source.

‎.github/workflows/benchmark-post-comment.yml‎

Lines changed: 266 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,266 @@
1+
# IMPORTANT: this workflow posts a github-actions[bot] comment on PRs based
2+
# on data produced by benchmark.yml (which runs untrusted PR code). The
3+
# trust boundaries are non-obvious; see ../SECURITY_MODEL.md. Anyone
4+
# editing this file -- or reviewing changes to it -- MUST read that
5+
# document first. In particular, the `case "$EVENT"` resolver in each job
6+
# is load-bearing: trusting the wrong source per upstream event would let
7+
# a fork forge cross-PR comments.
8+
#
9+
# Companion to .github/workflows/benchmark.yml. The bench workflow runs under
10+
# `pull_request` / `push` / `merge_group` / `issue_comment` with `contents: read`
11+
# only -- it has no write scope on PR-triggered runs from forks (read-only
12+
# GITHUB_TOKEN per GitHub's default), so it cannot post a comment itself.
13+
# Instead, it uploads the comment body as an artifact, and this workflow runs
14+
# in base-branch context (which does have write scope), downloads the artifact,
15+
# and posts/updates the PR comment.
16+
#
17+
# Pattern is identical to pr-validator.yml + comment-on-title-failure.yml.
18+
#
19+
# CAVEAT: workflow_run only fires for workflow definitions that live on the
20+
# default branch. A PR that modifies either this workflow or benchmark.yml
21+
# won't trigger this post workflow until the changes are merged to main. Test
22+
# end-to-end on a fork/staging repo, or accept a one-PR window of no comments
23+
# right after merge.
24+
25+
name: Post benchmark results
26+
27+
on:
28+
workflow_run:
29+
workflows: ["Benchmarking PR performance"]
30+
types: [completed]
31+
32+
# Empty default; each job opts into the minimum scopes it needs.
33+
permissions: {}
34+
35+
# Two rapid runs of workflow A (e.g., a quick push followed by a /bench
36+
# comment) can both complete near-simultaneously; without concurrency, both
37+
# instances of this workflow race on find-comment / create-or-update-comment
38+
# and may double-post. Key on the PR identity (head repo + head branch),
39+
# not workflow_run.id, since the latter is unique per upstream run and would
40+
# put every instance in its own group. For issue_comment upstream events
41+
# head_branch is the default branch, which over-serializes /bench comments
42+
# across different PRs through one group -- acceptable since posting is
43+
# fast and we don't cancel-in-progress here.
44+
concurrency:
45+
group: bench-comment-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }}
46+
cancel-in-progress: false
47+
48+
jobs:
49+
post-comment:
50+
name: Post benchmark results
51+
# Only post when the upstream succeeded AND the upstream event was a PR
52+
# event with a conversation to post to. `merge_group` and `push: main`
53+
# runs of workflow A have no PR to comment on.
54+
if: >
55+
github.event.workflow_run.conclusion == 'success'
56+
&& (github.event.workflow_run.event == 'pull_request'
57+
|| github.event.workflow_run.event == 'issue_comment')
58+
runs-on: ubuntu-latest
59+
permissions:
60+
pull-requests: write
61+
# download-artifact across workflows needs actions:read.
62+
actions: read
63+
steps:
64+
- name: Download bench-pr-number (issue_comment upstream only)
65+
# See SECURITY MODEL at the top of this file -- this artifact is
66+
# trusted only on the issue_comment path. continue-on-error so an
67+
# absent artifact (e.g., a non-/bench issue_comment that skipped
68+
# run-benchmark) flows through to the resolve step rather than
69+
# failing the whole workflow.
70+
id: download-num
71+
if: github.event.workflow_run.event == 'issue_comment'
72+
continue-on-error: true
73+
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
74+
with:
75+
name: bench-pr-number
76+
path: /tmp/
77+
run-id: ${{ github.event.workflow_run.id }}
78+
github-token: ${{ github.token }}
79+
- name: Resolve PR number
80+
id: pr
81+
env:
82+
EVENT: ${{ github.event.workflow_run.event }}
83+
GH_TOKEN: ${{ github.token }}
84+
REPO: ${{ github.repository }}
85+
# Conditional `owner:branch` for fork PRs vs same-repo (copied from
86+
# comment-on-title-failure.yml's resolver).
87+
PR_BRANCH: |-
88+
${{
89+
(github.event.workflow_run.head_repository.owner.login != github.event.workflow_run.repository.owner.login)
90+
&& format('{0}:{1}', github.event.workflow_run.head_repository.owner.login, github.event.workflow_run.head_branch)
91+
|| github.event.workflow_run.head_branch
92+
}}
93+
run: |
94+
set -u
95+
# Step 1: pick the trusted source for the PR number based on which
96+
# upstream event fired. See ../SECURITY_MODEL.md for why each
97+
# branch is safe -- they trust different things for different reasons.
98+
case "$EVENT" in
99+
pull_request)
100+
# Look up the PR by its head branch. head_branch and
101+
# head_repository come from the workflow_run payload (set by
102+
# GitHub, not the fork), so this lookup is trusted.
103+
NUM=$(gh pr view --repo "$REPO" "$PR_BRANCH" --json number --jq .number || true)
104+
;;
105+
issue_comment)
106+
# Read the PR number from the bench-pr-number artifact. The
107+
# upstream YAML uploaded this before PR-controlled code ran on
108+
# the runner (for issue_comment events GitHub serves
109+
# benchmark.yml from the default branch -- trusted).
110+
if [[ ! -f /tmp/pr-number.txt ]]; then
111+
echo "bench-pr-number artifact missing (likely a non-/bench comment that skipped run-benchmark); nothing to post." >&2
112+
echo "skip=true" >> "$GITHUB_OUTPUT"
113+
exit 0
114+
fi
115+
NUM=$(tr -d '[:space:]' < /tmp/pr-number.txt)
116+
;;
117+
*)
118+
echo "Unexpected upstream event: $EVENT" >&2
119+
echo "skip=true" >> "$GITHUB_OUTPUT"
120+
exit 0
121+
;;
122+
esac
123+
# Step 2: validate before emitting -- catches gh-lookup failures
124+
# (empty NUM) and any malformed artifact contents.
125+
if [[ -z "$NUM" || ! "$NUM" =~ ^[1-9][0-9]*$ ]]; then
126+
echo "Could not resolve PR number ('$NUM'); skipping." >&2
127+
echo "skip=true" >> "$GITHUB_OUTPUT"
128+
else
129+
printf 'number=%s\n' "$NUM" >> "$GITHUB_OUTPUT"
130+
fi
131+
- name: Download bench comment
132+
# continue-on-error: a successful upstream run may still have skipped
133+
# run-benchmark (e.g., a draft PR push satisfies the `pull_request`
134+
# trigger but the job's `if:` excludes drafts). In that case there is
135+
# no body artifact, and we should exit cleanly without touching the
136+
# existing PR comment.
137+
id: download-body
138+
if: steps.pr.outputs.skip != 'true'
139+
continue-on-error: true
140+
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
141+
with:
142+
name: bench-comment
143+
path: /tmp/
144+
run-id: ${{ github.event.workflow_run.id }}
145+
github-token: ${{ github.token }}
146+
- name: Find existing bench comment
147+
id: find
148+
if: steps.pr.outputs.skip != 'true' && steps.download-body.outcome == 'success'
149+
uses: peter-evans/find-comment@3eae4d37986fb5a8592848f6a574fdf654e61f9e # v3.1.0
150+
with:
151+
issue-number: ${{ steps.pr.outputs.number }}
152+
comment-author: 'github-actions[bot]'
153+
body-includes: '<!-- delta-kernel-bench-comment -->'
154+
- name: Post or update PR comment
155+
# When `comment-id` is empty (no prior comment) this creates one;
156+
# otherwise it replaces the existing body. Either way only one bench
157+
# comment exists on the PR.
158+
if: steps.pr.outputs.skip != 'true' && steps.download-body.outcome == 'success'
159+
uses: peter-evans/create-or-update-comment@71345be0265236311c031f5c7866368bd1eff043 # v4.0.0
160+
with:
161+
comment-id: ${{ steps.find.outputs.comment-id }}
162+
issue-number: ${{ steps.pr.outputs.number }}
163+
edit-mode: replace
164+
body-path: /tmp/bench-comment.md
165+
166+
post-failure-comment:
167+
name: Post benchmark failure note
168+
# Replace any existing success comment with a failure note so reviewers
169+
# don't see stale-green after a regression-causing push that breaks the
170+
# bench itself. `timed_out` counts as failure for our purposes; we skip
171+
# `cancelled` (a superseding run will post its own update) and `skipped`
172+
# (nothing meaningful happened, e.g., draft).
173+
if: >
174+
contains(fromJSON('["failure","timed_out"]'), github.event.workflow_run.conclusion)
175+
&& (github.event.workflow_run.event == 'pull_request'
176+
|| github.event.workflow_run.event == 'issue_comment')
177+
runs-on: ubuntu-latest
178+
permissions:
179+
pull-requests: write
180+
actions: read
181+
steps:
182+
- name: Download bench-pr-number (issue_comment upstream only)
183+
# Same security rationale as the post-comment job. Trusted only on
184+
# the issue_comment path. On the pull_request path we use gh pr view
185+
# against the trusted workflow_run.head_branch instead.
186+
id: download-num
187+
if: github.event.workflow_run.event == 'issue_comment'
188+
continue-on-error: true
189+
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
190+
with:
191+
name: bench-pr-number
192+
path: /tmp/
193+
run-id: ${{ github.event.workflow_run.id }}
194+
github-token: ${{ github.token }}
195+
- name: Resolve PR number
196+
id: pr
197+
env:
198+
EVENT: ${{ github.event.workflow_run.event }}
199+
GH_TOKEN: ${{ github.token }}
200+
REPO: ${{ github.repository }}
201+
PR_BRANCH: |-
202+
${{
203+
(github.event.workflow_run.head_repository.owner.login != github.event.workflow_run.repository.owner.login)
204+
&& format('{0}:{1}', github.event.workflow_run.head_repository.owner.login, github.event.workflow_run.head_branch)
205+
|| github.event.workflow_run.head_branch
206+
}}
207+
run: |
208+
set -u
209+
# Step 1: pick the trusted source for the PR number based on which
210+
# upstream event fired. See ../SECURITY_MODEL.md for why each
211+
# branch is safe -- they trust different things for different reasons.
212+
case "$EVENT" in
213+
pull_request)
214+
# Look up the PR by its head branch. head_branch and
215+
# head_repository come from the workflow_run payload (set by
216+
# GitHub, not the fork), so this lookup is trusted.
217+
NUM=$(gh pr view --repo "$REPO" "$PR_BRANCH" --json number --jq .number || true)
218+
;;
219+
issue_comment)
220+
# Read the PR number from the bench-pr-number artifact. The
221+
# upstream YAML uploaded this before PR-controlled code ran on
222+
# the runner (for issue_comment events GitHub serves
223+
# benchmark.yml from the default branch -- trusted).
224+
if [[ ! -f /tmp/pr-number.txt ]]; then
225+
echo "bench-pr-number artifact missing; cannot post failure note." >&2
226+
echo "skip=true" >> "$GITHUB_OUTPUT"
227+
exit 0
228+
fi
229+
NUM=$(tr -d '[:space:]' < /tmp/pr-number.txt)
230+
;;
231+
*)
232+
echo "Unexpected upstream event: $EVENT" >&2
233+
echo "skip=true" >> "$GITHUB_OUTPUT"
234+
exit 0
235+
;;
236+
esac
237+
# Step 2: validate before emitting -- catches gh-lookup failures
238+
# (empty NUM) and any malformed artifact contents.
239+
if [[ -z "$NUM" || ! "$NUM" =~ ^[1-9][0-9]*$ ]]; then
240+
echo "Could not resolve PR number ('$NUM'); skipping failure comment." >&2
241+
echo "skip=true" >> "$GITHUB_OUTPUT"
242+
else
243+
printf 'number=%s\n' "$NUM" >> "$GITHUB_OUTPUT"
244+
fi
245+
- name: Find existing bench comment
246+
id: find
247+
if: steps.pr.outputs.skip != 'true'
248+
uses: peter-evans/find-comment@3eae4d37986fb5a8592848f6a574fdf654e61f9e # v3.1.0
249+
with:
250+
issue-number: ${{ steps.pr.outputs.number }}
251+
comment-author: 'github-actions[bot]'
252+
body-includes: '<!-- delta-kernel-bench-comment -->'
253+
- name: Post or update failure note
254+
if: steps.pr.outputs.skip != 'true'
255+
uses: peter-evans/create-or-update-comment@71345be0265236311c031f5c7866368bd1eff043 # v4.0.0
256+
env:
257+
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}
258+
with:
259+
comment-id: ${{ steps.find.outputs.comment-id }}
260+
issue-number: ${{ steps.pr.outputs.number }}
261+
edit-mode: replace
262+
body: |
263+
<!-- delta-kernel-bench-comment -->
264+
## Benchmark results
265+
266+
<sub>Benchmark run failed. [See workflow run](${{ env.RUN_URL }}) for details.</sub>

0 commit comments

Comments
 (0)