Skip to content

Commit c009a85

Browse files
committed
ci(pr-review-poster): add line-anchored review poster and migrate clang-tidy
Adds a generic PR review-comment poster as a sibling of the issue-comment poster from #27021. Replaces platisd/clang-tidy-pr-comments@v1 in the Static Analysis workflow with an in-tree, fork-friendly producer + poster pair so fork PRs get inline clang-tidy annotations on the Files changed tab without trusting a third-party action with a write token. Architecture mirrors pr-comment-poster: a producer (clang-tidy.yml) runs inside the px4-dev container and writes a `pr-review` artifact containing manifest.json and a baked comments.json. A separate workflow_run-triggered poster runs on ubuntu-latest with the base-repo write token, validates the artifact, dismisses any stale matching review, and posts a fresh review on the target PR. The poster never checks out PR code and only ever reads two opaque JSON files from the artifact. Stale-review dismissal is restricted to reviews authored by github-actions[bot] AND whose body contains the producer's marker. A fork cannot impersonate the bot login or inject the marker into a human reviewer's body, so the poster can never dismiss a human review. APPROVE events are explicitly forbidden so a bot cannot approve a pull request. To avoid duplicating ~120 lines of HTTP plumbing between the two posters, the GitHub REST helpers (single-request, pagination, error handling) are extracted into Tools/ci/_github_helpers.py with a small GitHubClient class. The existing pr-comment-poster.py is refactored to use it; net change is roughly -80 lines on that script. The shared module is sparse-checked-out alongside each poster script and is stdlib only. The clang-tidy producer reuses MIT-licensed translation logic from platisd/clang-tidy-pr-comments (generate_review_comments, reorder_diagnostics, get_diff_line_ranges_per_file and helpers) under a preserved attribution header. The HTTP layer is rewritten on top of _github_helpers so the producer does not pull in `requests`. Conversation resolution (the GraphQL path) is intentionally dropped for v1. clang-tidy.yml now produces the pr-review artifact in the same job as the build, so the cross-runner compile_commands.json hand-off and workspace-path rewriting are no longer needed and the post_clang_tidy_comments job is removed. Signed-off-by: Ramon Roche <mrpollo@gmail.com>
1 parent 8c4b703 commit c009a85

File tree

7 files changed

+1405
-215
lines changed

7 files changed

+1405
-215
lines changed

.github/workflows/clang-tidy.yml

Lines changed: 38 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,9 @@ jobs:
2121
runs-on: [runs-on, runner=16cpu-linux-x64, "run-id=${{ github.run_id }}", "extras=s3-cache"]
2222
container:
2323
image: ghcr.io/px4/px4-dev:v1.17.0-rc2
24-
outputs:
25-
has_findings: ${{ steps.clang_tidy.outputs.has_findings }}
24+
permissions:
25+
contents: read
26+
pull-requests: read
2627
steps:
2728
- uses: runs-on/action@v2
2829
- uses: actions/checkout@v4
@@ -47,96 +48,50 @@ jobs:
4748
run: |
4849
if [ "${{ github.event_name }}" != "pull_request" ]; then
4950
make -j$(nproc) clang-tidy
50-
echo "has_findings=false" >> $GITHUB_OUTPUT
5151
else
52-
output=$(python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }} 2>&1)
53-
exit_code=$?
54-
echo "$output"
55-
# Helper prints this message on both early-exit paths
56-
# (no changed C++ files, or no matching TUs in the compile DB)
57-
if echo "$output" | grep -q "skipping clang-tidy"; then
58-
echo "has_findings=false" >> $GITHUB_OUTPUT
59-
else
60-
echo "has_findings=true" >> $GITHUB_OUTPUT
61-
fi
62-
exit $exit_code
52+
python3 Tools/ci/run-clang-tidy-pr.py origin/${{ github.base_ref }}
6353
fi
6454
65-
- name: Upload compile_commands.json
55+
# On PRs, also produce a `pr-review` artifact for the PR Review Poster
56+
# workflow to consume. clang-tidy-diff-18 emits a unified fixes.yml that
57+
# the producer script translates into line-anchored review comments.
58+
# Running this inside the same container as the build means there is no
59+
# workspace-path rewriting and no cross-runner artifact handoff.
60+
- name: Export clang-tidy fixes for PR review
61+
if: always() && github.event_name == 'pull_request'
62+
run: |
63+
mkdir -p pr-review
64+
git diff -U0 origin/${{ github.base_ref }}...HEAD \
65+
| clang-tidy-diff-18.py -p1 \
66+
-path build/px4_sitl_default-clang \
67+
-export-fixes pr-review/fixes.yml \
68+
-j0 || true
69+
70+
- name: Build pr-review artifact
71+
if: always() && github.event_name == 'pull_request'
72+
env:
73+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
74+
run: |
75+
python3 Tools/ci/clang-tidy-fixes-to-review.py \
76+
--fixes pr-review/fixes.yml \
77+
--repo-root "$GITHUB_WORKSPACE" \
78+
--repo "$GITHUB_REPOSITORY" \
79+
--pr-number "${{ github.event.pull_request.number }}" \
80+
--commit-sha "${{ github.event.pull_request.head.sha }}" \
81+
--out-dir pr-review \
82+
--event REQUEST_CHANGES
83+
84+
- name: Upload pr-review artifact
6685
if: always() && github.event_name == 'pull_request'
6786
uses: actions/upload-artifact@v4
6887
with:
69-
name: compile-commands
70-
path: build/px4_sitl_default-clang/compile_commands.json
88+
name: pr-review
89+
path: |
90+
pr-review/manifest.json
91+
pr-review/comments.json
7192
retention-days: 1
7293

7394
- uses: ./.github/actions/save-ccache
7495
if: always()
7596
with:
7697
cache-primary-key: ${{ steps.ccache.outputs.cache-primary-key }}
77-
78-
post_clang_tidy_comments:
79-
name: Clang-Tidy PR Annotations
80-
needs: [clang_tidy]
81-
runs-on: ubuntu-latest
82-
permissions:
83-
pull-requests: write
84-
contents: read
85-
if: >-
86-
always()
87-
&& github.event.pull_request
88-
&& github.event.pull_request.head.repo.full_name == github.repository
89-
&& (needs.clang_tidy.result == 'success' || needs.clang_tidy.result == 'failure')
90-
&& needs.clang_tidy.outputs.has_findings == 'true'
91-
steps:
92-
- uses: actions/checkout@v4
93-
with:
94-
fetch-depth: 0
95-
96-
- name: Install clang-tools (for clang-tidy-diff)
97-
run: sudo apt-get install -y clang-tools
98-
99-
- name: Download compile_commands.json
100-
uses: actions/download-artifact@v4
101-
with:
102-
name: compile-commands
103-
path: build/px4_sitl_default-clang
104-
105-
- name: Run clang-tidy-diff and export fixes
106-
run: |
107-
# WHY WE REWRITE compile_commands.json PATHS
108-
#
109-
# The clang_tidy job runs on a RunsOn/AWS runner where the workspace
110-
# root is "/__w/PX4-Autopilot/PX4-Autopilot". All absolute paths baked
111-
# into compile_commands.json (the "file" and "directory" fields, and
112-
# every -I include path in "command") use that prefix.
113-
#
114-
# This annotation job runs on a GitHub-hosted runner where the
115-
# workspace root is "/home/runner/work/PX4-Autopilot/PX4-Autopilot".
116-
# When clang-tidy-diff invokes clang-tidy, it reads the "directory"
117-
# field from compile_commands.json and calls chdir() on it. Since
118-
# the AWS-style path does not exist here, clang-tidy crashes with:
119-
# LLVM ERROR: Cannot chdir into "/__w/.../build/px4_sitl_default-clang"
120-
# and silently produces an empty fixes.yml, so no annotations are posted.
121-
#
122-
# Fix: rewrite all occurrences of the AWS workspace prefix to the
123-
# current runner workspace ($GITHUB_WORKSPACE) before invoking
124-
# clang-tidy-diff. Safe because compile_commands.json is a local
125-
# scratch file pulled from the artifact; no source file is modified.
126-
sed -i "s|/__w/PX4-Autopilot/PX4-Autopilot|${GITHUB_WORKSPACE}|g" \
127-
build/px4_sitl_default-clang/compile_commands.json
128-
129-
mkdir -p clang-tidy-result
130-
git diff -U0 origin/${{ github.base_ref }}...HEAD \
131-
| clang-tidy-diff-18.py -p1 \
132-
-path build/px4_sitl_default-clang \
133-
-export-fixes clang-tidy-result/fixes.yml \
134-
-j0 || true
135-
136-
- name: Annotate PR with clang-tidy findings
137-
uses: platisd/clang-tidy-pr-comments@v1
138-
with:
139-
github_token: ${{ secrets.GITHUB_TOKEN }}
140-
clang_tidy_fixes: clang-tidy-result/fixes.yml
141-
request_changes: true
142-
suggestions_per_comment: 10

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,11 @@ name: PR Comment Poster
4242
#
4343
# 6. `actions/checkout@v4` below uses NO ref (so it pulls the base branch,
4444
# 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.
45+
# sparse-checkout to materialize ONLY Tools/ci/pr-comment-poster.py and
46+
# its stdlib-only helper module Tools/ci/_github_helpers.py. The rest of
47+
# the repo never touches the workspace. This is safe: the only files the
48+
# job executes are base-repo Python scripts that were reviewed through
49+
# normal code review, never anything from the PR.
4950
#
5051
# ==============================================================================
5152
# ARTIFACT CONTRACT
@@ -102,6 +103,7 @@ jobs:
102103
with:
103104
sparse-checkout: |
104105
Tools/ci/pr-comment-poster.py
106+
Tools/ci/_github_helpers.py
105107
sparse-checkout-cone-mode: false
106108

107109
- name: Download pr-comment artifact
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
name: PR Review Poster
2+
3+
# Generic PR review-comment poster. Sibling of "PR Comment Poster": that
4+
# workflow posts sticky issue-style comments, this one posts line-anchored
5+
# review comments on the "Files changed" tab. Any analysis workflow that
6+
# wants to flag specific lines can produce a `pr-review` artifact and this
7+
# workflow will dismiss any stale matching review and post a fresh one.
8+
# Designed so analysis jobs running on untrusted fork PRs can still get
9+
# their inline annotations posted back to the PR.
10+
#
11+
# ==============================================================================
12+
# SECURITY INVARIANTS
13+
# ==============================================================================
14+
# This workflow runs on `workflow_run` which means it runs in the BASE REPO
15+
# context with a WRITE token, even when the triggering PR comes from a fork.
16+
# That is the entire reason it exists, and also the reason it is a loaded
17+
# footgun. Anyone modifying this file MUST preserve the following invariants:
18+
#
19+
# 1. NEVER check out PR code. No `actions/checkout` with a ref. No git clone
20+
# of a fork branch. No execution of scripts from the downloaded artifact.
21+
# The ONLY things read from the artifact are `manifest.json` and
22+
# `comments.json`, and both are treated as opaque data (JSON parsed by
23+
# the poster script and the comment fields posted via the GitHub API).
24+
#
25+
# 2. `pr_number` is validated to be a positive integer before use.
26+
# `marker` is validated to be printable ASCII only before use.
27+
# `commit_sha` is validated to be 40 lowercase hex characters.
28+
# `event` is validated against an allowlist of `COMMENT` and
29+
# `REQUEST_CHANGES`. `APPROVE` is intentionally forbidden so a bot
30+
# cannot approve a pull request. Validation happens inside
31+
# Tools/ci/pr-review-poster.py which is checked out from the base
32+
# branch, not from the artifact.
33+
#
34+
# 3. Comment bodies and the optional summary are passed to the GitHub API
35+
# as JSON fields, never interpolated into a shell command string.
36+
#
37+
# 4. This workflow file lives on the default branch. `workflow_run` only
38+
# loads workflow files from the default branch, so a fork cannot modify
39+
# THIS file as part of a PR. The fork CAN cause this workflow to fire
40+
# by triggering a producer workflow that uploads a `pr-review`
41+
# artifact. That is intended.
42+
#
43+
# 5. The artifact-name filter (`pr-review`) is the only gate on which
44+
# workflow runs get processed. Any workflow in this repo that uploads
45+
# an artifact named `pr-review` is trusted to have written the
46+
# manifest and comments itself, NOT copied fork-controlled content
47+
# into them. Producer workflows are responsible for that.
48+
#
49+
# 6. `actions/checkout@v4` below uses NO ref (so it pulls the base branch,
50+
# the default-branch commit this workflow file was loaded from) AND
51+
# uses sparse-checkout to materialize ONLY
52+
# Tools/ci/pr-review-poster.py and its stdlib-only helper module
53+
# Tools/ci/_github_helpers.py. The rest of the repo never touches the
54+
# workspace. This is safe: the only files the job executes are
55+
# base-repo Python scripts that were reviewed through normal code
56+
# review, never anything from the PR.
57+
#
58+
# 7. Stale-review dismissal is restricted to reviews whose AUTHOR is
59+
# `github-actions[bot]` AND whose body contains the producer's
60+
# marker. A fork PR cannot impersonate the bot login, and cannot
61+
# inject the marker into a human reviewer's body without API
62+
# access. Both filters together prevent the poster from ever
63+
# dismissing a human review.
64+
#
65+
# ==============================================================================
66+
# ARTIFACT CONTRACT
67+
# ==============================================================================
68+
# Producers upload an artifact named exactly `pr-review` containing:
69+
#
70+
# manifest.json:
71+
# {
72+
# "pr_number": 12345, // required, int > 0
73+
# "marker": "<!-- pr-review-poster:clang-tidy -->", // required, printable ASCII
74+
# "event": "REQUEST_CHANGES", // required, "COMMENT" | "REQUEST_CHANGES"
75+
# "commit_sha": "0123456789abcdef0123456789abcdef01234567", // required, 40 hex chars
76+
# "summary": "Optional review summary text" // optional
77+
# }
78+
#
79+
# comments.json: JSON array of line-anchored review comment objects:
80+
# [
81+
# {"path": "src/foo.cpp", "line": 42, "side": "RIGHT", "body": "..."},
82+
# {"path": "src/bar.hpp", "start_line": 10, "line": 15,
83+
# "side": "RIGHT", "start_side": "RIGHT", "body": "..."}
84+
# ]
85+
#
86+
# The `marker` string is used to find an existing matching review to
87+
# dismiss before posting a new one. It MUST be unique per producer (e.g.
88+
# include the producer name).
89+
#
90+
# Producers MUST write `pr_number` and `commit_sha` from their own
91+
# workflow context (`github.event.pull_request.number` and
92+
# `github.event.pull_request.head.sha`) and MUST NOT read either from any
93+
# fork-controlled source.
94+
95+
on:
96+
workflow_run:
97+
# Producers that may upload a `pr-review` artifact. When a new
98+
# producer is wired up, add its workflow name here. Runs of workflows
99+
# not in this list will never trigger the poster. Every run of a
100+
# listed workflow will trigger the poster, which will no-op if no
101+
# `pr-review` artifact exists.
102+
workflows:
103+
- "Static Analysis"
104+
types:
105+
- completed
106+
107+
permissions:
108+
pull-requests: write
109+
actions: read
110+
contents: read
111+
112+
jobs:
113+
post:
114+
name: Post PR Review
115+
runs-on: ubuntu-latest
116+
if: github.event.workflow_run.conclusion != 'cancelled'
117+
steps:
118+
# Checkout runs first so the poster scripts are available AND so
119+
# that actions/checkout@v4's default clean step does not delete the
120+
# artifact zip that the next step writes into the workspace.
121+
# Sparse-checkout restricts the materialized tree to just the
122+
# poster script and its stdlib helper module.
123+
- name: Checkout poster script only
124+
uses: actions/checkout@v4
125+
with:
126+
sparse-checkout: |
127+
Tools/ci/pr-review-poster.py
128+
Tools/ci/_github_helpers.py
129+
sparse-checkout-cone-mode: false
130+
131+
- name: Download pr-review artifact
132+
id: download
133+
uses: actions/github-script@v7
134+
with:
135+
script: |
136+
const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
137+
owner: context.repo.owner,
138+
repo: context.repo.repo,
139+
run_id: context.payload.workflow_run.id,
140+
});
141+
const match = artifacts.data.artifacts.find(a => a.name === 'pr-review');
142+
if (!match) {
143+
core.info('No pr-review artifact on this run; nothing to post.');
144+
core.setOutput('found', 'false');
145+
return;
146+
}
147+
const download = await github.rest.actions.downloadArtifact({
148+
owner: context.repo.owner,
149+
repo: context.repo.repo,
150+
artifact_id: match.id,
151+
archive_format: 'zip',
152+
});
153+
const fs = require('fs');
154+
fs.writeFileSync('pr-review.zip', Buffer.from(download.data));
155+
core.setOutput('found', 'true');
156+
157+
- name: Unpack artifact
158+
if: steps.download.outputs.found == 'true'
159+
run: |
160+
mkdir -p pr-review
161+
unzip -q pr-review.zip -d pr-review
162+
163+
- name: Validate artifact
164+
if: steps.download.outputs.found == 'true'
165+
run: python3 Tools/ci/pr-review-poster.py validate pr-review
166+
167+
- name: Post PR review
168+
if: steps.download.outputs.found == 'true'
169+
env:
170+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
171+
run: python3 Tools/ci/pr-review-poster.py post pr-review

0 commit comments

Comments
 (0)