Repository navigation
*: harden generated Bazel artifact workflow - #68243
Merged
ti-chi-bot[bot] merged 1 commit intoMay 11, 2026
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
do-not-merge/needs-linked-issue
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a new "Check Bazel Prepare" workflow, replaces ChangesBazel CI and Workflow Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Contributor
participant GenerateWorkflow
participant UpdateWorkflow
participant Bazel
participant GitRepo
Contributor->>GenerateWorkflow: open/update PR triggers generate
GenerateWorkflow->>Bazel: run gazelle / update-repos / tazel / mirror
Bazel->>GenerateWorkflow: outputs DEPS.bzl, *.bazel, *.bzl changes
GenerateWorkflow->>UpdateWorkflow: produce bazel.patch artifact
alt fork PR
UpdateWorkflow->>UpdateWorkflow: download artifact, verify non-empty, write manual instructions, fail
else same-repo PR
UpdateWorkflow->>UpdateWorkflow: validate patch paths
UpdateWorkflow->>GitRepo: git apply --check --index
GitRepo->>UpdateWorkflow: ok
UpdateWorkflow->>GitRepo: git apply --index
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
labels
hawkingrei
force-pushed
the
update-bazel-artifact-safety
branch
from
May 9, 2026 02:54
7d7308b to
ee28c2a
Compare
ti-chi-bot
Bot
added
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/update-bazel-files.yml (2)
195-203: 💤 Low valueMinor: commit-time pathspec is broader than the validated allow-list.
The commit gate uses
DEPS.bzl ':(glob)**/*.bazel' ':(glob)**/*.bzl', which is wider than the validation allow-list (which additionally permits only specificbuild/**/BUILD.bazelshapes). In practice this is fine because the validate step already rejected anything outside the allow-list before we reach this point, so nothing unsafe can be staged here. No change required — noting only because, if the validate step is ever relaxed independently, this gate will silently follow along. A brief comment tying these two patterns together (or factoring the allow-list into a single source of truth) would help future maintainers keep them in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/update-bazel-files.yml around lines 195 - 203, The commit gate's pathspec (DEPS.bzl ':(glob)**/*.bazel' ':(glob)**/*.bzl') is broader than the validation allow-list (which restricts to specific build/**/BUILD.bazel shapes); add a brief comment next to the git commit/push block (around the git -C pr commit / git -C pr push lines) explaining that the validate step enforces a stricter allow-list and that these patterns must be kept in sync, or refactor both the validate step and this pathspec to reference a single source-of-truth allow-list constant to avoid accidental drift.
113-130: 💤 Low valueConsider adding a brief inline comment explaining the case-glob semantics to clarify for future maintainers why
build/*/BUILD.bazel(allow) must precedebuild/*(deny) and howWORKSPACE.bazelis properly caught by the explicit deny before*.bazelcould match it. This low-effort addition improves maintainability of the security-sensitive path validation logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/update-bazel-files.yml around lines 113 - 130, Add a short inline comment in the case statement describing the glob-ordering semantics and why specific patterns must come first: explain that the allow pattern build/*/BUILD.bazel must precede the deny build/* to avoid being shadowed, and note that explicit denies like WORKSPACE and WORKSPACE.bazel are listed before the broader *.bazel rule so workspace files are safely caught; place this comment near the case(...) block around the build/*/BUILD.bazel, build/*, WORKSPACE.bazel, and *.bazel patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 195-203: The commit gate's pathspec (DEPS.bzl ':(glob)**/*.bazel'
':(glob)**/*.bzl') is broader than the validation allow-list (which restricts to
specific build/**/BUILD.bazel shapes); add a brief comment next to the git
commit/push block (around the git -C pr commit / git -C pr push lines)
explaining that the validate step enforces a stricter allow-list and that these
patterns must be kept in sync, or refactor both the validate step and this
pathspec to reference a single source-of-truth allow-list constant to avoid
accidental drift.
- Around line 113-130: Add a short inline comment in the case statement
describing the glob-ordering semantics and why specific patterns must come
first: explain that the allow pattern build/*/BUILD.bazel must precede the deny
build/* to avoid being shadowed, and note that explicit denies like WORKSPACE
and WORKSPACE.bazel are listed before the broader *.bazel rule so workspace
files are safely caught; place this comment near the case(...) block around the
build/*/BUILD.bazel, build/*, WORKSPACE.bazel, and *.bazel patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f69431af-c588-4cd4-9e07-36c58d351953
📒 Files selected for processing (3)
.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.ymlMakefile
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- .github/workflows/generate-bazel-files.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68243 +/- ##
================================================
- Coverage 77.7307% 77.0195% -0.7112%
================================================
Files 1990 1973 -17
Lines 551899 553127 +1228
================================================
- Hits 428995 426016 -2979
- Misses 121984 127060 +5076
+ Partials 920 51 -869
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hawkingrei
force-pushed
the
update-bazel-artifact-safety
branch
from
May 9, 2026 03:17
ee28c2a to
5896721
Compare
ti-chi-bot
Bot
added
size/M
D3Hunter
approved these changes
May 9, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
lance6716
approved these changes
May 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
hawkingrei
force-pushed
the
update-bazel-artifact-safety
branch
from
May 9, 2026 04:02
5896721 to
d71e400
Compare
ti-chi-bot
Bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/check-bazel-prepare.yml (3)
34-40: ⚡ Quick winConsider enabling caches for improved CI performance.
All caches are currently disabled for both Go (
cache: false) and Bazel (all three cache types). While this ensures reproducibility and avoids cache-related issues, it can significantly slow down the workflow, especially for a check that runs on every PR update.Unless there's a specific reason for disabling caches (e.g., reproducibility requirements, cache corruption issues, or disk space constraints), consider enabling them to improve CI performance and developer experience.
💡 Proposed change to enable caches
- name: Set up Go uses: actions/setup-go@v6 with: go-version: 1.25 - cache: false + cache: true - name: Set up Bazelisk uses: bazel-contrib/setup-bazel@0.16.0 with: - bazelisk-cache: false - repository-cache: false - external-cache: false + bazelisk-cache: true + repository-cache: true + external-cache: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-bazel-prepare.yml around lines 34 - 40, The workflow currently disables all caches (the Go step uses "cache: false" and the "Set up Bazelisk" step sets bazelisk-cache, repository-cache, and external-cache to false), which slows CI; update the workflow to enable caching by changing "cache: false" to true for the Go/cache step and set bazelisk-cache, repository-cache, and external-cache to true (or remove those false overrides) in the "Set up Bazelisk" step (bazel-contrib/setup-bazel@0.16.0) so Bazel and Go use the available caches unless there is a documented reason to keep them disabled.
28-28: ⚡ Quick winOptimize checkout performance by using shallow clone.
The
check-bazel-prepare.shscript only usesgit diffto compare working directory changes, which does not require full git history. Changefetch-depth: 0tofetch-depth: 1to speed up the workflow without affecting functionality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-bazel-prepare.yml at line 28, The checkout step currently sets fetch-depth: 0 which fetches full history; change the GitHub Actions checkout configuration from the literal "fetch-depth: 0" to "fetch-depth: 1" so the workflow performs a shallow clone (only the latest commit) — locate the checkout step that contains the "fetch-depth" key in .github/workflows/check-bazel-prepare.yml and update its value to 1.
30-34: Consider updating to Go 1.26.3, the latest stable version (released May 7, 2026).Go 1.25 is valid, but unless intentionally specified for compatibility reasons, updating to Go 1.26.3 is recommended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-bazel-prepare.yml around lines 30 - 34, Update the GitHub Actions step that uses actions/setup-go@v6: change the go-version value from 1.25 to 1.26.3 (the current stable release) so the workflow runs with Go 1.26.3; keep other step keys (uses, cache) unchanged unless there is an intentional compatibility constraint requiring 1.25.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/check-bazel-prepare.yml:
- Around line 34-40: The workflow currently disables all caches (the Go step
uses "cache: false" and the "Set up Bazelisk" step sets bazelisk-cache,
repository-cache, and external-cache to false), which slows CI; update the
workflow to enable caching by changing "cache: false" to true for the Go/cache
step and set bazelisk-cache, repository-cache, and external-cache to true (or
remove those false overrides) in the "Set up Bazelisk" step
(bazel-contrib/setup-bazel@0.16.0) so Bazel and Go use the available caches
unless there is a documented reason to keep them disabled.
- Line 28: The checkout step currently sets fetch-depth: 0 which fetches full
history; change the GitHub Actions checkout configuration from the literal
"fetch-depth: 0" to "fetch-depth: 1" so the workflow performs a shallow clone
(only the latest commit) — locate the checkout step that contains the
"fetch-depth" key in .github/workflows/check-bazel-prepare.yml and update its
value to 1.
- Around line 30-34: Update the GitHub Actions step that uses
actions/setup-go@v6: change the go-version value from 1.25 to 1.26.3 (the
current stable release) so the workflow runs with Go 1.26.3; keep other step
keys (uses, cache) unchanged unless there is an intentional compatibility
constraint requiring 1.25.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e63d7db-e53a-4eba-b218-755321663340
📒 Files selected for processing (3)
.github/workflows/check-bazel-prepare.yml.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/generate-bazel-files.yml
- .github/workflows/update-bazel-files.yml
Contributor
Author
|
/unhold |
hawkingrei
force-pushed
the
update-bazel-artifact-safety
branch
from
May 9, 2026 04:14
d71e400 to
80993ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/update-bazel-files.yml (1)
134-178:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftValidation runs against wrong working tree.
Lines 134–178 validate the patch using
git apply --summary,git apply --numstat -zin the workflow root, but lines 180–181 apply the patch to thepr/checkout. If the two directories have different states, validation could pass even when the apply would fail, or vice versa.Move the validation into the
pr/directory to ensure it checks the same tree that will be modified:-patch_path="${PWD}/bazel-artifact/bazel.patch" -summary="$(git apply --summary "${patch_path}")" +patch_path="../bazel-artifact/bazel.patch" +cd pr +summary="$(git apply --summary "${patch_path}")" while IFS= read -r line; do ... done <<< "${summary}" ... -done < <(git apply --numstat -z "${patch_path}") +done < <(git apply --numstat -z "${patch_path}") -git -C pr apply --check --index "${patch_path}" -git -C pr apply --index "${patch_path}" +git apply --check --index "${patch_path}" +git apply --index "${patch_path}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/update-bazel-files.yml around lines 134 - 178, The validation uses git apply --summary and --numstat -z against the workflow root while the actual patch is applied to the pr/ checkout, so move the validation to the same working tree used for apply: run the git apply --summary and git apply --numstat -z commands from the pr/ checkout (or use git -C pr/ ...) so the summary loop that sets summary="$(git apply --summary "${patch_path}")" and the while loop reading git apply --numstat -z "${patch_path}" operate against the same tree that will be modified; keep the same path checks (seen_paths associative array and case checks) but execute them in the pr/ directory context.
🧹 Nitpick comments (2)
.github/workflows/update-bazel-files.yml (1)
96-100: ⚡ Quick winDistinguish missing patch file from empty patch.
The check
[[ ! -s "${patch_path}" ]]returns true for both non-existent and empty files. If the artifact is malformed andbazel.patchis missing entirely, the job silently succeeds. Consider adding an explicit existence check first:+if [[ ! -f "${patch_path}" ]]; then + echo "Error: bazel.patch not found in artifact for fork PR #${PR_NUMBER}." >&2 + exit 1 +fi if [[ ! -s "${patch_path}" ]]; then echo "No generated Bazel changes for fork PR #${PR_NUMBER}." exit 0 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/update-bazel-files.yml around lines 96 - 100, The current check using patch_path and [[ ! -s "${patch_path}" ]] conflates missing and empty bazel.patch; change the logic to first test existence with [[ ! -f "${patch_path}" ]] (or [[ ! -e "${patch_path}" ]]) and, if missing, emit an error message (e.g., "Missing bazel.patch artifact") and exit non-zero so the job fails; only after confirming the file exists, use [[ ! -s "${patch_path}" ]] to detect an empty patch and handle that case (keep the existing "No generated Bazel changes..." message and exit 0)..github/workflows/check-bazel-prepare.yml (1)
40-40: 💤 Low valueDocument why
CIis unset.Unsetting the
CIenvironment variable before runningmake check-bazel-preparesuggests the make target has different behavior in CI vs local contexts. Add a comment explaining why this is necessary to avoid confusion for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-bazel-prepare.yml at line 40, Add a brief inline comment immediately above the unset CI line explaining why CI is being cleared before running make check-bazel-prepare: state that the make target detects CI and alters behavior (e.g., skips local-only setup, changes caching or credentials), and that unsetting CI forces the local execution path needed for this workflow; reference the make target name make check-bazel-prepare and the unset CI command so future maintainers understand the intentional override.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-bazel-prepare.yml:
- Around line 19-24: The workflow currently checks out PR head commits using
actions/checkout@v6 and runs make check-bazel-prepare on pull_request events,
which risks executing untrusted fork code; update the trigger to
pull_request_target (or require manual approval) and change the checkout step to
explicitly check out the base branch (not the PR head) before running make
check-bazel-prepare—ensure the actions/checkout uses the base ref
(github.event.pull_request.base.sha or repository default) so the job runs
against trusted base code while still validating the PR, or gate the job behind
a manual approval step.
- Line 31: Update the pinned GitHub Action version for bazel-contrib/setup-bazel
from `@0.16.0` to the latest stable release (e.g., `@0.19.0`) wherever the workflow
uses "uses: bazel-contrib/setup-bazel@0.16.0" so the workflow references "uses:
bazel-contrib/setup-bazel@0.19.0" (or another current stable tag); ensure no
other steps assume the older behavior and run CI to verify compatibility.
- Line 28: Update the GitHub Actions runner Go version by changing the
go-version key value from "1.25" to a current stable release (e.g., "1.26.3") so
the workflow uses a modern Go runtime; locate the "go-version: 1.25" entry in
the workflow and replace it with "go-version: 1.26.3" (or at least "1.26") to
apply the upgrade.
---
Outside diff comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 134-178: The validation uses git apply --summary and --numstat -z
against the workflow root while the actual patch is applied to the pr/ checkout,
so move the validation to the same working tree used for apply: run the git
apply --summary and git apply --numstat -z commands from the pr/ checkout (or
use git -C pr/ ...) so the summary loop that sets summary="$(git apply --summary
"${patch_path}")" and the while loop reading git apply --numstat -z
"${patch_path}" operate against the same tree that will be modified; keep the
same path checks (seen_paths associative array and case checks) but execute them
in the pr/ directory context.
---
Nitpick comments:
In @.github/workflows/check-bazel-prepare.yml:
- Line 40: Add a brief inline comment immediately above the unset CI line
explaining why CI is being cleared before running make check-bazel-prepare:
state that the make target detects CI and alters behavior (e.g., skips
local-only setup, changes caching or credentials), and that unsetting CI forces
the local execution path needed for this workflow; reference the make target
name make check-bazel-prepare and the unset CI command so future maintainers
understand the intentional override.
In @.github/workflows/update-bazel-files.yml:
- Around line 96-100: The current check using patch_path and [[ ! -s
"${patch_path}" ]] conflates missing and empty bazel.patch; change the logic to
first test existence with [[ ! -f "${patch_path}" ]] (or [[ ! -e "${patch_path}"
]]) and, if missing, emit an error message (e.g., "Missing bazel.patch
artifact") and exit non-zero so the job fails; only after confirming the file
exists, use [[ ! -s "${patch_path}" ]] to detect an empty patch and handle that
case (keep the existing "No generated Bazel changes..." message and exit 0).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 690794e5-1cf1-4a0e-bc8c-8592b51bbfa0
📒 Files selected for processing (3)
.github/workflows/check-bazel-prepare.yml.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/generate-bazel-files.yml
hawkingrei
force-pushed
the
update-bazel-artifact-safety
branch
from
May 9, 2026 04:31
80993ac to
2848569
Compare
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
yongman
pushed a commit
to yongman/tidb
that referenced
this pull request
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #68199
Problem Summary:
The generated Bazel files workflow can produce and later consume a generated patch artifact. The consumer workflow is privileged enough to commit generated files back to same-repo PR branches, so the generated artifact should have a narrow path boundary and should not carry high-risk workspace or build directory changes. Fork PRs also need an explicit signal when generated Bazel changes exist, because the base repository workflow cannot safely commit back to fork branches.
What changed and how does it work?
This PR tightens the generated Bazel artifact path contract:
make bazel_preparecall inGenerate Bazel Filesinto the concrete Bazel commands used by the workflow.WORKSPACE,WORKSPACE.bazel, and non-BUILD.bazelfiles underbuild/.build/**/BUILD.bazelallowed so Bazel metadata generated underbuild/can still be committed automatically.Update Bazel Filesbefore applying a downloaded artifact.Update Bazel Filesvalidate fork PR artifacts. If a fork PR has a non-empty generated patch, the job fails with commands for applying the artifact manually instead of silently skipping.check-bazel-preparebeforebazel-failpoint-enableinbazel_ci_test*targets.Check List
Tests
Manual test:
ruby -e 'require "yaml"; ARGV.each { |f| YAML.load_file(f); puts "ok #{f}" }' .github/workflows/generate-bazel-files.yml .github/workflows/update-bazel-files.yml git -c core.fsmonitor=false diff --check make -n bazel_ci_test make -n bazel_ci_test_ddlargsv1Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit