Skip to content

Commit 8c4b703

Browse files
mrpollomrpollo
committed
ci(pr-comment-poster): add generic PR comment poster and migrate producers
Adds a stand-alone workflow that posts or updates sticky PR comments on behalf of any analysis workflow, including those triggered by fork PRs. The poster runs on `workflow_run` in the base repo context, which is the standard GitHub-sanctioned way to get a write token on events that originate from untrusted forks without ever checking out fork code. All validation, GitHub API interaction, and upsert logic lives in Tools/ci/pr-comment-poster.py (Python 3 stdlib only, two subcommands: `validate` and `post`). The workflow file itself is a thin orchestrator: sparse-checkout the script, download the pr-comment artifact via github-script, unzip, then invoke the script twice. No inline jq, no inline bash validation, no shell-interpolated marker strings. The sparse-checkout ensures only Tools/ci/pr-comment-poster.py lands in the workspace, never the rest of the repo. Artifact contract: a producer uploads an artifact named exactly `pr-comment` containing `manifest.json` (with `pr_number`, `marker`, and optional `mode`) and `body.md`. The script validates the manifest (positive integer pr_number, printable-ASCII marker bounded 1..200 chars, UTF-8 body under 60000 bytes, mode in an allowlist), finds any existing comment containing the marker via the comments REST API, and either edits it in place or creates a new one. The workflow file header documents six security invariants that any future change MUST preserve, most importantly: NEVER check out PR code, NEVER execute anything from the artifact, and treat all artifact contents as opaque data. Why a generic poster and not `pull_request_target`: `pull_request_target` is the tool people reach for first and the one that most often turns into a supply-chain vulnerability, because it hands a write token to a workflow that is then tempted to check out the PR head. `workflow_run` gives the same write token without any check-out temptation, because the only input is a pre-produced artifact treated as opaque data. Producer migrations =================== flash_analysis.yml: - Drop the fork gate on the `post_pr_comment` job. - Drop the obsolete TODO pointing at issue #24408 (the fork-comment workflow does not error anymore; it just no-ops). - Keep the existing "comment only if threshold crossed or previous comment exists" behaviour verbatim. peter-evans/find-comment@v3 stays as a read-only probe (forks can read issue comments just fine); its body-includes is updated to search for the new marker `<!-- pr-comment-poster:flash-analysis -->` instead of the old "FLASH Analysis" heading substring. - Replace the peter-evans/create-or-update-comment@v4 step with two new steps that write pr-comment/manifest.json and pr-comment/body.md and then upload them as artifact pr-comment. The body markdown is byte-for-byte identical to the previous heredoc, with the marker prepended as the first line so subsequent runs can find it. - The threshold-or-existing-comment gate is preserved on both new steps. When the gate does not fire no artifact is uploaded and the poster no-ops. docs-orchestrator.yml (link-check job): - Drop the fork gate on the sticky-comment step. - Replace marocchino/sticky-pull-request-comment@v2 with two new steps that copy logs/filtered-link-check-results.md into pr-comment/body.md, write a pr-comment/manifest.json with the marker `<!-- pr-comment-poster:docs-link-check -->`, and upload the directory as artifact pr-comment. - The prepare step checks `test -s` on the results file and emits a prepared step output; the upload step is gated on that output. In practice the existing link-check step always writes a placeholder ("No broken links found in changed files.") into the file when empty, so the guard is defensive but not load-bearing today. - Tighten the link-check job's permissions from `pull-requests: write` down to `contents: read`; writing PR comments now happens in the poster workflow. The poster's workflows allowlist is seeded with the two active producers: "FLASH usage analysis" and "Docs - Orchestrator". clang-tidy (workflow name "Static Analysis") is not in the list because platisd/clang-tidy-pr-comments posts line-level review comments, a different REST API from issue comments that the poster script does not handle. Extending the poster to cover review comments is a follow-up. Signed-off-by: Ramon Roche <mrpollo@gmail.com>
1 parent c9f1d2a commit 8c4b703

4 files changed

Lines changed: 622 additions & 35 deletions

File tree

‎.github/workflows/docs-orchestrator.yml‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ jobs:
213213
if: always() && (github.event_name == 'pull_request')
214214
permissions:
215215
contents: read
216-
pull-requests: write
217216
runs-on: ubuntu-latest
218217
steps:
219218
- name: Checkout
@@ -281,12 +280,32 @@ jobs:
281280
> ./logs/link-check-results.md || true
282281
cat ./logs/link-check-results.md
283282
284-
- name: Post PR comment with link check results
285-
if: github.event.pull_request.head.repo.full_name == github.repository
286-
uses: marocchino/sticky-pull-request-comment@v2
283+
- name: Prepare pr-comment artifact
284+
id: prepare-pr-comment
285+
run: |
286+
if [ ! -s ./logs/filtered-link-check-results.md ]; then
287+
echo "No link-check results file; skipping pr-comment artifact."
288+
echo "prepared=false" >> "$GITHUB_OUTPUT"
289+
exit 0
290+
fi
291+
mkdir -p pr-comment
292+
cp ./logs/filtered-link-check-results.md pr-comment/body.md
293+
cat > pr-comment/manifest.json <<EOF
294+
{
295+
"pr_number": ${{ github.event.pull_request.number }},
296+
"marker": "<!-- pr-comment-poster:docs-link-check -->",
297+
"mode": "upsert"
298+
}
299+
EOF
300+
echo "prepared=true" >> "$GITHUB_OUTPUT"
301+
302+
- name: Upload pr-comment artifact
303+
if: steps.prepare-pr-comment.outputs.prepared == 'true'
304+
uses: actions/upload-artifact@v4
287305
with:
288-
header: flaws
289-
path: ./logs/filtered-link-check-results.md
306+
name: pr-comment
307+
path: pr-comment/
308+
retention-days: 1
290309

291310
- name: Upload link check results
292311
uses: actions/upload-artifact@v4

‎.github/workflows/flash_analysis.yml‎

Lines changed: 45 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,6 @@ jobs:
9393
echo '${{ steps.bloaty-step.outputs.bloaty-summary-map }}' >> $GITHUB_OUTPUT
9494
echo "$EOF" >> $GITHUB_OUTPUT
9595
96-
# TODO:
97-
# This part of the workflow is causing errors for forks. We should find a way to fix this and enable it again for forks.
98-
# Track this issue https://github.com/PX4/PX4-Autopilot/issues/24408
9996
post_pr_comment:
10097
name: Publish Results
10198
runs-on: [runs-on,runner=1cpu-linux-x64,image=ubuntu24-full-x64,"run-id=${{ github.run_id }}"]
@@ -105,50 +102,69 @@ jobs:
105102
V5X-SUMMARY-MAP-PERC: ${{ fromJSON(fromJSON(needs.analyze_flash.outputs.px4_fmu-v5x-bloaty-summary-map).vm-percentage) }}
106103
V6X-SUMMARY-MAP-ABS: ${{ fromJSON(fromJSON(needs.analyze_flash.outputs.px4_fmu-v6x-bloaty-summary-map).vm-absolute) }}
107104
V6X-SUMMARY-MAP-PERC: ${{ fromJSON(fromJSON(needs.analyze_flash.outputs.px4_fmu-v6x-bloaty-summary-map).vm-percentage) }}
108-
if: github.event.pull_request && github.event.pull_request.head.repo.full_name == github.repository
105+
if: github.event.pull_request
109106
steps:
110107
- name: Find Comment
111108
uses: peter-evans/find-comment@v3
112109
id: fc
113110
with:
114111
issue-number: ${{ github.event.pull_request.number }}
115112
comment-author: 'github-actions[bot]'
116-
body-includes: FLASH Analysis
113+
body-includes: '<!-- pr-comment-poster:flash-analysis -->'
117114

118115
- name: Set Build Time
119116
id: bt
120117
run: |
121118
echo "timestamp=$(date +'%Y-%m-%dT%H:%M:%S')" >> $GITHUB_OUTPUT
122119
123-
- name: Create or update comment
120+
- name: Write pr-comment artifact
124121
# This can't be moved to the job-level conditions, as GH actions don't allow a job-level if condition to access the env.
125122
if: |
126123
steps.fc.outputs.comment-id != '' ||
127124
env.V5X-SUMMARY-MAP-ABS >= fromJSON(env.MIN_FLASH_POS_DIFF_FOR_COMMENT) ||
128125
env.V5X-SUMMARY-MAP-ABS <= fromJSON(env.MIN_FLASH_NEG_DIFF_FOR_COMMENT) ||
129126
env.V6X-SUMMARY-MAP-ABS >= fromJSON(env.MIN_FLASH_POS_DIFF_FOR_COMMENT) ||
130127
env.V6X-SUMMARY-MAP-ABS <= fromJSON(env.MIN_FLASH_NEG_DIFF_FOR_COMMENT)
131-
uses: peter-evans/create-or-update-comment@v4
128+
run: |
129+
mkdir -p pr-comment
130+
cat > pr-comment/manifest.json <<EOF
131+
{
132+
"pr_number": ${{ github.event.pull_request.number }},
133+
"marker": "<!-- pr-comment-poster:flash-analysis -->",
134+
"mode": "upsert"
135+
}
136+
EOF
137+
cat > pr-comment/body.md <<'PR_COMMENT_BODY_EOF'
138+
<!-- pr-comment-poster:flash-analysis -->
139+
## 🔎 FLASH Analysis
140+
<details>
141+
<summary>px4_fmu-v5x [Total VM Diff: ${{ env.V5X-SUMMARY-MAP-ABS }} byte (${{ env.V5X-SUMMARY-MAP-PERC}} %)]</summary>
142+
143+
```
144+
${{ needs.analyze_flash.outputs.px4_fmu-v5x-bloaty-output }}
145+
```
146+
</details>
147+
148+
<details>
149+
<summary>px4_fmu-v6x [Total VM Diff: ${{ env.V6X-SUMMARY-MAP-ABS }} byte (${{ env.V6X-SUMMARY-MAP-PERC }} %)]</summary>
150+
151+
```
152+
${{ needs.analyze_flash.outputs.px4_fmu-v6x-bloaty-output }}
153+
```
154+
</details>
155+
156+
**Updated: _${{ steps.bt.outputs.timestamp }}_**
157+
PR_COMMENT_BODY_EOF
158+
159+
- name: Upload pr-comment artifact
160+
if: |
161+
steps.fc.outputs.comment-id != '' ||
162+
env.V5X-SUMMARY-MAP-ABS >= fromJSON(env.MIN_FLASH_POS_DIFF_FOR_COMMENT) ||
163+
env.V5X-SUMMARY-MAP-ABS <= fromJSON(env.MIN_FLASH_NEG_DIFF_FOR_COMMENT) ||
164+
env.V6X-SUMMARY-MAP-ABS >= fromJSON(env.MIN_FLASH_POS_DIFF_FOR_COMMENT) ||
165+
env.V6X-SUMMARY-MAP-ABS <= fromJSON(env.MIN_FLASH_NEG_DIFF_FOR_COMMENT)
166+
uses: actions/upload-artifact@v4
132167
with:
133-
comment-id: ${{ steps.fc.outputs.comment-id }}
134-
issue-number: ${{ github.event.pull_request.number }}
135-
body: |
136-
## 🔎 FLASH Analysis
137-
<details>
138-
<summary>px4_fmu-v5x [Total VM Diff: ${{ env.V5X-SUMMARY-MAP-ABS }} byte (${{ env.V5X-SUMMARY-MAP-PERC}} %)]</summary>
139-
140-
```
141-
${{ needs.analyze_flash.outputs.px4_fmu-v5x-bloaty-output }}
142-
```
143-
</details>
144-
145-
<details>
146-
<summary>px4_fmu-v6x [Total VM Diff: ${{ env.V6X-SUMMARY-MAP-ABS }} byte (${{ env.V6X-SUMMARY-MAP-PERC }} %)]</summary>
147-
148-
```
149-
${{ needs.analyze_flash.outputs.px4_fmu-v6x-bloaty-output }}
150-
```
151-
</details>
152-
153-
**Updated: _${{ steps.bt.outputs.timestamp }}_**
154-
edit-mode: replace
168+
name: pr-comment
169+
path: pr-comment/
170+
retention-days: 1

‎.github/workflows/pr-comment-poster.yml‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
name: PR Comment Poster
2+
3+
# Generic PR comment poster. Any analysis workflow (clang-tidy, flash_analysis,
4+
# fuzz coverage, SITL perf, etc.) can produce a `pr-comment` artifact and this
5+
# workflow will post or update a sticky PR comment with its contents. Designed
6+
# so that analysis jobs running on untrusted fork PRs can still get their
7+
# results posted back to the PR.
8+
#
9+
# ==============================================================================
10+
# SECURITY INVARIANTS
11+
# ==============================================================================
12+
# This workflow runs on `workflow_run` which means it runs in the BASE REPO
13+
# context with a WRITE token, even when the triggering PR comes from a fork.
14+
# That is the entire reason it exists, and also the reason it is a loaded
15+
# footgun. Anyone modifying this file MUST preserve the following invariants:
16+
#
17+
# 1. NEVER check out PR code. No `actions/checkout` with a ref. No git clone
18+
# of a fork branch. No execution of scripts from the downloaded artifact.
19+
# The ONLY things read from the artifact are `manifest.json` and `body.md`,
20+
# and both are treated as opaque data (JSON parsed by the poster script
21+
# and markdown posted verbatim via the GitHub API).
22+
#
23+
# 2. `pr_number` is validated to be a positive integer before use.
24+
# `marker` is validated to be printable ASCII only before use. Validation
25+
# happens inside Tools/ci/pr-comment-poster.py which is checked out from
26+
# the base branch, not from the artifact.
27+
#
28+
# 3. The comment body is passed to the GitHub API as a JSON field, never
29+
# interpolated into a shell command string.
30+
#
31+
# 4. This workflow file lives on the default branch. `workflow_run` only
32+
# loads workflow files from the default branch, so a fork cannot modify
33+
# THIS file as part of a PR. The fork CAN cause this workflow to fire
34+
# by triggering a producer workflow that uploads a `pr-comment` artifact.
35+
# That is intended.
36+
#
37+
# 5. The artifact-name filter (`pr-comment`) is the only gate on which
38+
# workflow runs get processed. Any workflow in this repo that uploads
39+
# an artifact named `pr-comment` is trusted to have written the
40+
# manifest and body itself, NOT copied fork-controlled content into
41+
# them. Producer workflows are responsible for that.
42+
#
43+
# 6. `actions/checkout@v4` below uses NO ref (so it pulls the base branch,
44+
# the default-branch commit this workflow file was loaded from) AND uses
45+
# sparse-checkout to materialize ONLY Tools/ci/pr-comment-poster.py. The
46+
# rest of the repo never touches the workspace. This is safe: the only
47+
# file the job executes is a base-repo Python script that was reviewed
48+
# through normal code review, never anything from the PR.
49+
#
50+
# ==============================================================================
51+
# ARTIFACT CONTRACT
52+
# ==============================================================================
53+
# Producers upload an artifact named exactly `pr-comment` containing:
54+
#
55+
# manifest.json:
56+
# {
57+
# "pr_number": 12345, // required, int > 0
58+
# "marker": "<!-- pr-comment-poster:flash-analysis -->", // required, printable ASCII
59+
# "mode": "upsert" // optional, default "upsert"
60+
# }
61+
#
62+
# body.md: the markdown content of the comment. Posted verbatim.
63+
#
64+
# The `marker` string is used to find an existing comment to update. It MUST
65+
# be unique per producer (e.g. include the producer name). If no existing
66+
# comment contains the marker, a new one is created. If the marker is found
67+
# in an existing comment, that comment is edited in place.
68+
#
69+
# Producers MUST write `pr_number` from their own workflow context
70+
# (`github.event.pull_request.number`) and MUST NOT read it from any
71+
# fork-controlled source.
72+
73+
on:
74+
workflow_run:
75+
# Producers that may upload a `pr-comment` artifact. When a new producer
76+
# is wired up, add its workflow name here. Runs of workflows not in this
77+
# list will never trigger the poster. Every run of a listed workflow will
78+
# trigger the poster, which will no-op if no `pr-comment` artifact exists.
79+
workflows:
80+
- "FLASH usage analysis"
81+
- "Docs - Orchestrator"
82+
types:
83+
- completed
84+
85+
permissions:
86+
pull-requests: write
87+
actions: read
88+
contents: read
89+
90+
jobs:
91+
post:
92+
name: Post PR Comment
93+
runs-on: ubuntu-latest
94+
if: github.event.workflow_run.conclusion != 'cancelled'
95+
steps:
96+
# Checkout runs first so the poster script is available AND so that
97+
# actions/checkout@v4's default clean step does not delete the artifact
98+
# zip that the next step writes into the workspace. Sparse-checkout
99+
# restricts the materialized tree to just the poster script.
100+
- name: Checkout poster script only
101+
uses: actions/checkout@v4
102+
with:
103+
sparse-checkout: |
104+
Tools/ci/pr-comment-poster.py
105+
sparse-checkout-cone-mode: false
106+
107+
- name: Download pr-comment artifact
108+
id: download
109+
uses: actions/github-script@v7
110+
with:
111+
script: |
112+
const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
113+
owner: context.repo.owner,
114+
repo: context.repo.repo,
115+
run_id: context.payload.workflow_run.id,
116+
});
117+
const match = artifacts.data.artifacts.find(a => a.name === 'pr-comment');
118+
if (!match) {
119+
core.info('No pr-comment artifact on this run; nothing to post.');
120+
core.setOutput('found', 'false');
121+
return;
122+
}
123+
const download = await github.rest.actions.downloadArtifact({
124+
owner: context.repo.owner,
125+
repo: context.repo.repo,
126+
artifact_id: match.id,
127+
archive_format: 'zip',
128+
});
129+
const fs = require('fs');
130+
fs.writeFileSync('pr-comment.zip', Buffer.from(download.data));
131+
core.setOutput('found', 'true');
132+
133+
- name: Unpack artifact
134+
if: steps.download.outputs.found == 'true'
135+
run: |
136+
mkdir -p pr-comment
137+
unzip -q pr-comment.zip -d pr-comment
138+
139+
- name: Validate artifact
140+
if: steps.download.outputs.found == 'true'
141+
run: python3 Tools/ci/pr-comment-poster.py validate pr-comment
142+
143+
- name: Upsert sticky comment
144+
if: steps.download.outputs.found == 'true'
145+
env:
146+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
147+
run: python3 Tools/ci/pr-comment-poster.py post pr-comment

0 commit comments

Comments
 (0)