Skip to content

ci: split clang-tidy-bazel into pull_request + workflow_run stages#10521

Merged
maliberty merged 2 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:ci/clang-tidy-bazel-fork-pr-fix
May 27, 2026
Merged

ci: split clang-tidy-bazel into pull_request + workflow_run stages#10521
maliberty merged 2 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:ci/clang-tidy-bazel-fork-pr-fix

Conversation

@openroad-ci
Copy link
Copy Markdown
Collaborator

Summary

  • Split clang-tidy-bazel into two stages so reviewdog can post review comments on fork PRs on the public repo, where the pull_request token is read-only.
  • Stage A (clang-tidy-bazel) keeps the bazel --config=lint build and uploads clang-tidy.txt + pr-meta.txt as an artifact. No write permissions.
  • Stage B (clang-tidy-bazel-post) runs on workflow_run in base-repo context with a writable token. Downloads the artifact, validates pr-meta.txt fields against a strict regex allowlist, synthesizes a pull_request event JSON (because workflow_run.pull_requests[] is empty for fork PRs), then runs reviewdog with -reporter=github-pr-review.

Why

Single-stage pull_request workflow can't post reviewdog reviews on fork PRs — GitHub gives those runs a read-only GITHUB_TOKEN regardless of permissions:, so POST /pulls/N/reviews returned 403 and reviewdog exited 1 (see failed run on public tier). This mirrors the legacy two-stage clang-tidy-review / clang-tidy-review-post setup that solved the same problem before the bazel-based rewrite landed.

Security

  • Stage B never checks out fork code and never runs any fork scripts. It only consumes the text artifact + validated metadata.
  • pr-meta.txt is parsed with a strict allowlist (pr_number numeric, SHAs hex-40, repo slugs ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$).
  • Bazel build of fork code stays on Stage A's read-only token; BAZEL_CACHE_PASSWORD is not available there (consistent with prior behaviour).

Test plan

  • Sync to staging and public; verify Stage A succeeds and uploads artifact on a normal in-org PR.
  • Verify Stage B triggers on workflow_run completed and posts a review on the in-org PR.
  • Re-trigger public PR mpl: Fixes MPL-45 can't split cluster error #10520 (or equivalent fork PR) and confirm reviewdog posts review comments instead of 403'ing.
  • Confirm Stage B is skipped when Stage A's conclusion != 'success'.

Fork PRs on the public repo (where external contributors land) get a
read-only GITHUB_TOKEN, so the single-stage pull_request workflow
couldn't post reviewdog review comments — POST /pulls/N/reviews returned
403 and reviewdog exited 1.

Stage A (clang-tidy-bazel) keeps the bazel --config=lint build but now
only uploads clang-tidy.txt and pr-meta.txt as an artifact. It needs no
write permissions, so it stays safe on fork code.

Stage B (clang-tidy-bazel-post) runs on workflow_run in the base repo's
context with a writable token. It downloads the artifact, validates
pr-meta fields against a strict allowlist, synthesizes a pull_request
event payload (workflow_run.pull_requests[] is empty for fork PRs), and
runs reviewdog with -reporter=github-pr-review. It never checks out the
fork's code or runs any fork scripts.

Signed-off-by: Joao Luis Sombrio <sombrio@sombrasoft.dev>
@openroad-ci openroad-ci requested a review from a team as a code owner May 27, 2026 03:51
@openroad-ci openroad-ci requested a review from precisionmoon May 27, 2026 03:51
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@maliberty
Copy link
Copy Markdown
Member

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d79066ddb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/github-actions-clang-tidy-bazel-post.yml
Reviewdog's github-pr-review reporter resolves the local git root before
flushing comments. On a fresh runner containing only the downloaded
artifact, reviewdog logs "reviewdog: .git not found" and silently no-ops
(exit 0, no review posted). Confirmed locally by running reviewdog from a
scratch dir against the same synthesized event + artifact: zero comments
posted; running the same command from inside a checkout works.

Add a shallow actions/checkout step before the artifact download. The
default ref in workflow_run context is the base repo's default branch,
which is safe (no fork code) and sufficient — reviewdog fetches the
actual PR diff via the GitHub API, so the local SHA need not match.

Signed-off-by: Joao Luis Sombrio <sombrio@sombrasoft.dev>
@maliberty
Copy link
Copy Markdown
Member

@codex review

@maliberty maliberty requested review from maliberty and removed request for precisionmoon May 27, 2026 16:55
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24d544ace0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

echo "base_repo=${{ github.event.pull_request.base.repo.full_name }}"
} > pr-meta.txt

- name: Upload clang-tidy artifact
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore failing the PR check on lint findings

When a PR produces clang-tidy diagnostics, this pull_request workflow now finishes successfully after uploading clang-tidy.txt; the only remaining -fail-level=any is in the workflow_run companion, and GitHub documents workflow_run as running at the default-branch GITHUB_SHA, not the PR head. That means branch protection on the original clang-tidy-bazel PR check can pass even though the previous inline reviewdog step would have failed the PR check for the same diagnostics.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@sombraSoft sombraSoft May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in fcb0180.

Stage A now re-runs reviewdog with -reporter=local -filter-mode=added -fail-level=any after the artifact upload. Local mode only needs the filesystem and an explicit -diff command, so it works fine under the fork-PR read-only token. Step is positioned after upload-artifact so the post workflow still has the findings to comment on even when this check exits non-zero.

Stage B's guard relaxes from conclusion == 'success' to conclusion != 'cancelled' so it still runs and posts review comments when Stage A's check step failed. Net behaviour: same as the original single-stage workflow — clang-tidy findings in the PR diff fail the clang-tidy-bazel check (branch protection blocks merge) AND the post workflow posts inline review comments. Fork PRs still benefit from the split because the posting half runs in base-repo context with a writable token.

@maliberty maliberty merged commit c183cc4 into The-OpenROAD-Project:master May 27, 2026
16 checks passed
@maliberty maliberty deleted the ci/clang-tidy-bazel-fork-pr-fix branch May 27, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants