Uh oh!
There was an error while loading. Please reload this page.
fix(ci): harden bundle-size PR workflow trust boundaries - #6773
Merged
Conversation
Contributor
View your CI Pipeline Execution ↗ for commit f88be98
☁️ Nx Cloud last updated this comment at |
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTrigger changed from Changes
Sequence Diagram(s)sequenceDiagram
participant PR as PR/Event
participant Runner as Actions Runner
participant Benchmark as benchmark-pr
participant Capture as capture
participant Comment as comment-pr
participant GH as GitHub API
PR->>Runner: trigger (pull_request_target)
Runner->>Benchmark: run benchmark-pr (checkout PR merge ref, run benchmarks)
Benchmark->>Runner: upload artifact (current.json) and set outputs (current_json_b64, base_sha, pr_number)
Runner->>Capture: run capture (download artifact, emit outputs)
Capture->>Comment: pass outputs (current_json_b64, base_sha, pr_number)
Comment->>Runner: restore current.json from base64
Comment->>GH: upsert/update PR comment using pr_number
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/bundle-size.yml (1)
20-23: Consider adding a timeout to the untrusted benchmark job.A job-level timeout helps cap resource abuse and runaway builds when executing PR code.
⏱️ Suggested tweak
benchmark-pr: name: Benchmark PR if: github.event_name == 'pull_request_target' runs-on: ubuntu-latest + timeout-minutes: 20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bundle-size.yml around lines 20 - 23, Add a job-level timeout to the benchmark-pr GitHub Actions job to prevent runaway or abusive PR builds: update the benchmark-pr job definition (identifier "benchmark-pr") to include a timeout-minutes field with an appropriate value (e.g., 30 or 60) so the workflow runner will cancel the job after that duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bundle-size.yml:
- Around line 47-49: The delimiter echo can be concatenated to the JSON if the
file lacks a trailing newline; change the three-line block that uses DELIM so
the delimiter is always on its own line by printing it with an explicit newline
(e.g. use printf '%s\n' "$DELIM" or echo -e "\n$DELIM" where appropriate) and
ensure you emit a newline after cat e2e/bundle-size/results/current.json before
printing the delimiter; update the statements referencing DELIM (the echo
"current_json<<$DELIM", the cat of current.json, and the final echo "$DELIM")
accordingly.
---
Nitpick comments:
In @.github/workflows/bundle-size.yml:
- Around line 20-23: Add a job-level timeout to the benchmark-pr GitHub Actions
job to prevent runaway or abusive PR builds: update the benchmark-pr job
definition (identifier "benchmark-pr") to include a timeout-minutes field with
an appropriate value (e.g., 30 or 60) so the workflow runner will cancel the job
after that duration.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bundle-size.yml:
- Around line 24-27: The workflow is exposing PR metadata (outputs
current_json_b64, base_sha, pr_number) from an untrusted path (benchmark-pr)
which creates a confused-deputy risk; ensure these values are produced and
consumed only by a trusted job (e.g., capture) and not passed through
benchmark-pr. Modify the job wiring so that capture declares and sets outputs
current_json_b64, base_sha, pr_number, and have comment-pr consume
capture.outputs.* directly (and depend on capture), removing any assignments or
passthrough of those keys from benchmark-pr; also audit other occurrences (lines
referenced around 47-49, 91-92, 99-100) to replace untrusted job outputs with
the trusted capture job outputs.
- Around line 66-72: The restore step currently writes an empty
e2e/bundle-size/results/current.json when process.env.CURRENT_JSON_B64 is
missing; change the inline Node script that uses process.env.CURRENT_JSON_B64 to
check for a non-empty value (CURRENT_JSON_B64) and, if absent, print a clear
error and exit non‑zero (e.g., throw or call process.exit(1)) so the workflow
fails fast instead of creating an empty file; update the node -e invocation that
writes current.json to perform this guard and error out when the payload is
missing.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+66
to
+72
| - name: Restore Benchmark Outputs | ||
| env: | ||
| CURRENT_JSON_B64: ${{ needs.benchmark-pr.outputs.current_json_b64 }} | ||
| run: | | ||
| mkdir -p e2e/bundle-size/results | ||
| node -e "const fs=require('node:fs'); fs.writeFileSync('e2e/bundle-size/results/current.json', Buffer.from(process.env.CURRENT_JSON_B64 || '', 'base64'))" | ||
Contributor
There was a problem hiding this comment.
Fail fast when benchmark payload is missing.
Current restore logic silently writes an empty current.json when output propagation fails, which makes later failures harder to diagnose.
🧪 Suggested guardrail
- name: Restore Benchmark Outputs
env:
CURRENT_JSON_B64: ${{ needs.benchmark-pr.outputs.current_json_b64 }}
run: |
mkdir -p e2e/bundle-size/results
- node -e "const fs=require('node:fs'); fs.writeFileSync('e2e/bundle-size/results/current.json', Buffer.from(process.env.CURRENT_JSON_B64 || '', 'base64'))"+ if [ -z "${CURRENT_JSON_B64:-}" ]; then+ echo "Missing benchmark payload from benchmark-pr outputs." >&2+ exit 1+ fi+ printf '%s' "$CURRENT_JSON_B64" | base64 -d > e2e/bundle-size/results/current.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Restore Benchmark Outputs | |
| env: | |
| CURRENT_JSON_B64: ${{ needs.benchmark-pr.outputs.current_json_b64 }} | |
| run: | | |
| mkdir -p e2e/bundle-size/results | |
| node -e "const fs=require('node:fs'); fs.writeFileSync('e2e/bundle-size/results/current.json', Buffer.from(process.env.CURRENT_JSON_B64 || '', 'base64'))" | |
| - name: Restore Benchmark Outputs | |
| env: | |
| CURRENT_JSON_B64: ${{ needs.benchmark-pr.outputs.current_json_b64 }} | |
| run: | | |
| mkdir -p e2e/bundle-size/results | |
| if [ -z "${CURRENT_JSON_B64:-}" ]; then | |
| echo "Missing benchmark payload from benchmark-pr outputs." >&2 | |
| exit 1 | |
| fi | |
| printf '%s' "$CURRENT_JSON_B64" | base64 -d > e2e/bundle-size/results/current.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/bundle-size.yml around lines 66 - 72, The restore step
currently writes an empty e2e/bundle-size/results/current.json when
process.env.CURRENT_JSON_B64 is missing; change the inline Node script that uses
process.env.CURRENT_JSON_B64 to check for a non-empty value (CURRENT_JSON_B64)
and, if absent, print a clear error and exit non‑zero (e.g., throw or call
process.exit(1)) so the workflow fails fast instead of creating an empty file;
update the node -e invocation that writes current.json to perform this guard and
error out when the payload is missing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/bundle-size.yml (1)
87-93:⚠️ Potential issue | 🟡 MinorAdd a guard to fail fast when benchmark payload is missing.
The current implementation silently writes an empty
current.jsonwhenCURRENT_JSON_B64is missing or empty, making downstream failures harder to diagnose.,
🛡️ Suggested guardrail
- name: Restore Benchmark Outputs env: CURRENT_JSON_B64: ${{ needs.capture.outputs.current_json_b64 }} run: | mkdir -p e2e/bundle-size/results - node -e "const fs=require('node:fs'); fs.writeFileSync('e2e/bundle-size/results/current.json', Buffer.from(process.env.CURRENT_JSON_B64 || '', 'base64'))"+ if [ -z "${CURRENT_JSON_B64:-}" ]; then+ echo "::error::Missing benchmark payload from capture outputs."+ exit 1+ fi+ printf '%s' "$CURRENT_JSON_B64" | base64 -d > e2e/bundle-size/results/current.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bundle-size.yml around lines 87 - 93, The workflow currently decodes process.env.CURRENT_JSON_B64 and writes an empty e2e/bundle-size/results/current.json when the env var is missing; update the run step's node -e script to check process.env.CURRENT_JSON_B64 first and if it's undefined or empty, print a clear error (including the variable name CURRENT_JSON_B64) to stderr and exit with a non-zero status so the job fails fast instead of writing an empty file; otherwise decode and write the file as before.
🧹 Nitpick comments (1)
.github/workflows/bundle-size.yml (1)
96-96: Redundant directory creation.Line 91 already creates
e2e/bundle-size/results, so thismkdir -pis unnecessary.♻️ Suggested removal
- name: Read Historical Data (if available) run: | - mkdir -p e2e/bundle-size/results if git fetch --depth=1 origin gh-pages; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bundle-size.yml at line 96, Remove the redundant directory creation command "mkdir -p e2e/bundle-size/results" from the workflow; since the same directory is already created earlier in the bundle-size job (the earlier "mkdir -p e2e/bundle-size/results" invocation), delete the duplicate line in .github/workflows/bundle-size.yml to avoid unnecessary repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bundle-size.yml:
- Line 27: The workflow uses an outdated checkout action version string
"actions/checkout@v6.0.1"; update that reference to the latest patch by
replacing it with "actions/checkout@v6.0.2" (or change to the major pin
"actions/checkout@v6" if you prefer major-version pinning) so the bundle-size
GitHub Action uses the recommended latest patch release.
---
Duplicate comments:
In @.github/workflows/bundle-size.yml:
- Around line 87-93: The workflow currently decodes process.env.CURRENT_JSON_B64
and writes an empty e2e/bundle-size/results/current.json when the env var is
missing; update the run step's node -e script to check
process.env.CURRENT_JSON_B64 first and if it's undefined or empty, print a clear
error (including the variable name CURRENT_JSON_B64) to stderr and exit with a
non-zero status so the job fails fast instead of writing an empty file;
otherwise decode and write the file as before.
---
Nitpick comments:
In @.github/workflows/bundle-size.yml:
- Line 96: Remove the redundant directory creation command "mkdir -p
e2e/bundle-size/results" from the workflow; since the same directory is already
created earlier in the bundle-size job (the earlier "mkdir -p
e2e/bundle-size/results" invocation), delete the duplicate line in
.github/workflows/bundle-size.yml to avoid unnecessary repetition.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
pull_requesttopull_request_targetand split execution into untrusted benchmark + trusted comment jobspull-requests: writeonly to the comment jobSummary by CodeRabbit