fix: run E2E tests after i18n completes on release PRs#8111
fix: run E2E tests after i18n completes on release PRs#8111
Conversation
- Add workflow_run trigger for 'i18n: Update Core' workflow - Skip version-bump PRs on pull_request trigger - Run E2E tests via workflow_run after i18n completes - Update checkout refs and PR commenting for workflow_run events
dosubot
bot
added
the
size:M
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a workflow_run trigger and a new get-pr-info job to resolve PR number and branch for workflow_run and pull_request contexts; pins checkouts to the resolved head branch; and threads PR_NUMBER, BRANCH, START_TIME, and fallback GITHUB_SHA into comment and deploy steps. Changes
Sequence Diagram(s)sequenceDiagram
participant I18n as I18n Workflow
participant E2E as CI E2E Workflow
participant API as GitHub REST API
participant Runner as Job Runner / Scripts
rect rgba(200,200,255,0.5)
I18n->>E2E: emit workflow_run (completed)
end
rect rgba(200,255,200,0.5)
E2E->>E2E: compute concurrency (workflow_run.head_branch || github.ref)
E2E->>API: query PRs / gh pr view to resolve PR number & branch
API-->>E2E: return PR number and branch
end
rect rgba(255,200,200,0.5)
E2E->>Runner: checkout using resolved branch / fallback SHA
Runner->>Runner: set outputs/env PR_NUMBER, BRANCH, GITHUB_SHA, START_TIME
Runner->>Runner: run pr-playwright-deploy-and-comment.sh with resolved PR info
end
Possibly related PRs
Suggested reviewers
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 |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/17/2026, 10:14:23 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
.github/workflows/ci-tests-e2e.yaml
Outdated
| setup: | ||
| runs-on: ubuntu-latest | ||
| # Skip version-bump PRs on pull_request (they run via workflow_run after i18n completes) | ||
| if: ${{ !(github.event_name == 'pull_request' && startsWith(github.head_ref, 'version-bump-')) }} |
There was a problem hiding this comment.
Could you just add this to the branches-ignore?
There was a problem hiding this comment.
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 18.1 kB (baseline 18.1 kB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 878 kB (baseline 878 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 66.6 kB (baseline 66.6 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 385 kB (baseline 385 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
User & Accounts — 4.22 kB (baseline 4.22 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Editors & Dialogs — 2.85 kB (baseline 2.85 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
UI Components — 26.3 kB (baseline 26.3 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 2.74 MB (baseline 2.74 MB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 16.6 kB (baseline 16.6 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 9.09 MB (baseline 9.09 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 5.91 MB (baseline 5.91 MB) • ⚪ 0 BBundles that do not match a named category
|
…gers and remove the conditional job skip.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci-tests-e2e.yaml:
- Around line 206-218: The current get-pr step (id: get-pr) looks up PRs by
matching context.payload.workflow_run.head_sha which is brittle and paginated
list() can miss results; change it to first check
context.payload.workflow_run.pull_requests and return that PR number if present,
and only if empty fallback to a robust branch-based lookup: use pulls.list with
head set to `${context.repo.owner}:${context.payload.workflow_run.head_branch}`
(or the branch from the event) and paginate through results with per_page=100
across pages until a match is found; apply this same change to the other get-pr
step referenced in the workflow so both use pull_requests from the payload first
and branch+pagination as fallback.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/ci-tests-e2e.yaml:
- Around line 10-13: The workflow_run trigger currently fires for any completion
of the i18n workflow; update the job gating so E2E jobs only run for
version-bump PRs by adding the provided conditional check to the relevant jobs
(e.g., the setup job) referencing the workflow_run trigger and the setup job
name: add an if condition that checks github.event_name and
startsWith(github.event.workflow_run.head_branch, 'version-bump-') so the
workflow only proceeds when not triggered by workflow_run or when the head
branch starts with "version-bump-".
- Around line 279-284: The workflow step currently sets PR_NUMBER and BRANCH via
inline shell variable expansion before calling bash
./scripts/cicd/pr-playwright-deploy-and-comment.sh which allows script
injection; move PR_NUMBER and BRANCH into the step's env: block and reference
them as $PR_NUMBER and $BRANCH in the run command (leave the bash invocation
unchanged) so the values are passed as environment variables instead of
interpolated into the shell line; update the step that calls
pr-playwright-deploy-and-comment.sh to use env: PR_NUMBER and BRANCH and pass
"$PR_NUMBER" "$BRANCH" to the script.
- Around line 226-231: The workflow is vulnerable because github.head_ref and
github.event.workflow_run.head_branch are interpolated directly into the run
shell, enabling shell injection; fix it by passing those values as GitHub
Actions environment variables instead of embedding them in the shell command:
set env: PR_NUMBER: ${{ github.event.pull_request.number ||
steps.get-pr.outputs.result }} and BRANCH: ${{ github.head_ref ||
github.event.workflow_run.head_branch }} for the job/step, then invoke
./scripts/cicd/pr-playwright-deploy-and-comment.sh without inline variable
interpolation (the script can read $PR_NUMBER and $BRANCH from its environment),
ensuring you reference the PR_NUMBER and BRANCH env names shown here and avoid
constructing the command string with unescaped user-controlled inputs.
♻️ Duplicate comments (2)
.github/workflows/ci-tests-e2e.yaml (2)
204-216: PR lookup byhead_shawill fail after i18n commits.This issue was previously flagged and remains unresolved. The i18n workflow pushes a new commit, so
workflow_run.head_shawill be the new commit SHA—not the original PR head SHA. Thefind()call will fail to match any PR.Additionally, the default
pulls.list()returns only 30 PRs without pagination.Use
workflow_run.pull_requestsfrom the payload first, then fall back to branch-based lookup:Suggested fix
- name: Get PR number for workflow_run id: get-pr if: github.event_name == 'workflow_run' uses: actions/github-script@v7 with: script: | - const { data: prs } = await github.rest.pulls.list({ - owner: context.repo.owner, - repo: context.repo.repo, - state: 'open', - }); - const pr = prs.find(p => p.head.sha === context.payload.workflow_run.head_sha); - return pr?.number || ''; + const prFromPayload = context.payload.workflow_run.pull_requests?.[0]?.number; + if (prFromPayload) return prFromPayload; + + const head = `${context.repo.owner}:${context.payload.workflow_run.head_branch}`; + const prs = await github.paginate(github.rest.pulls.list, { + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + head, + per_page: 100, + }); + return prs[0]?.number || '';
253-265: Duplicate get-pr logic with same brittleness.Same issue as the earlier
get-prstep—apply the same fix usingworkflow_run.pull_requestsfrom the payload with branch-based fallback and pagination.
| # Run after i18n workflow completes for version-bump PRs | ||
| workflow_run: | ||
| workflows: ['i18n: Update Core'] | ||
| types: [completed] |
There was a problem hiding this comment.
Missing branch filter on workflow_run trigger.
The workflow_run trigger fires for any completion of the i18n workflow, not just version-bump PRs. This will cause E2E tests to run after every i18n update regardless of branch, which is not the intended behavior per the PR objectives.
Add a condition to the setup job (or all jobs that should only run for version-bump PRs via workflow_run) to filter by branch:
if: >
github.event_name != 'workflow_run' ||
startsWith(github.event.workflow_run.head_branch, 'version-bump-')Alternatively, consider filtering earlier at the workflow level using a preliminary job that gates downstream jobs.
🤖 Prompt for AI Agents
In @.github/workflows/ci-tests-e2e.yaml around lines 10 - 13, The workflow_run
trigger currently fires for any completion of the i18n workflow; update the job
gating so E2E jobs only run for version-bump PRs by adding the provided
conditional check to the relevant jobs (e.g., the setup job) referencing the
workflow_run trigger and the setup job name: add an if condition that checks
github.event_name and startsWith(github.event.workflow_run.head_branch,
'version-bump-') so the workflow only proceeds when not triggered by
workflow_run or when the head branch starts with "version-bump-".
There was a problem hiding this comment.
There was a problem hiding this comment.
@DrJKL Great point! You can specify what branches the triggering workflow must run on using the branches or branches-ignore filters directly on the workflow_run event, which is much cleaner than adding job-level conditionals.
The workflow can be updated to use native branch filtering:
workflow_run:
workflows: ['i18n: Update Core']
types: [completed]
branches:
- 'version-bump-*'This will ensure the E2E workflow only triggers via workflow_run when the i18n workflow completes on version-bump branches, which is exactly the intended behavior described in the PR objectives.
🧠 Learnings used
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Run quality gates before PR: `pnpm lint`, `pnpm typecheck`, `pnpm knip`, and ensure relevant tests pass
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci-tests-e2e.yaml:
- Around line 266-268: The deploy-and-comment job currently uses if: always()
which lets it run even when get-pr-info was skipped or returned no PR number;
change the job's if condition to require a successful get-pr-info and a
non-empty PR number, e.g. replace if: always() with a guard like if:
needs.get-pr-info.result == 'success' && needs.get-pr-info.outputs.pr_number !=
'' so the job (referenced as deploy-and-comment and the get-pr-info job) only
runs when a valid PR number is present.
- Around line 190-233: The get-branch step currently writes untrusted inputs
(github.head_ref / github.event.workflow_run.head_branch) to $GITHUB_OUTPUT
using echo, which can be subject to shell injection; update the get-branch step
to capture the branch into a safe environment variable and write it with a
literal-safe formatter (use printf '%s' or equivalent) instead of echo so the
value is treated as data not shell code; specifically, change the run in the
get-branch step that sets branch to use printf '%s' with the expression
(github.head_ref || github.event.workflow_run.head_branch) to append the literal
branch string to $GITHUB_OUTPUT.
♻️ Duplicate comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
9-13: Add a branch filter to theworkflow_runtrigger (still fires on all i18n runs).Without a branch filter, any completion of
i18n: Update Core(including non‑version‑bump branches) will trigger E2E, which is out of scope and can run without PR context.🔧 Suggested fix
workflow_run: workflows: ['i18n: Update Core'] types: [completed] + branches: + - 'version-bump-*'
| # Get PR info once for reuse by comment jobs | ||
| get-pr-info: | ||
| runs-on: ubuntu-latest | ||
| if: | | ||
| (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || | ||
| (github.event_name == 'workflow_run') | ||
| outputs: | ||
| pr_number: ${{ steps.get-pr.outputs.result }} | ||
| branch: ${{ steps.get-branch.outputs.branch }} | ||
| steps: | ||
| - name: Get PR number | ||
| id: get-pr | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| if (context.eventName === 'pull_request') { | ||
| return context.payload.pull_request.number; | ||
| } | ||
|
|
||
| // First check pull_requests from payload (most reliable) | ||
| const prs = context.payload.workflow_run.pull_requests; | ||
| if (prs && prs.length > 0) { | ||
| return prs[0].number; | ||
| } | ||
|
|
||
| // Fallback: branch-based lookup with pagination | ||
| const head = `${context.repo.owner}:${context.payload.workflow_run.head_branch}`; | ||
|
|
||
| for await (const response of github.paginate.iterator( | ||
| github.rest.pulls.list, | ||
| { owner: context.repo.owner, repo: context.repo.repo, state: 'open', head, per_page: 100 } | ||
| )) { | ||
| if (response.data.length > 0) { | ||
| return response.data[0].number; | ||
| } | ||
| } | ||
|
|
||
| console.log('No PR found'); | ||
| return ''; | ||
|
|
||
| - name: Get branch name | ||
| id: get-branch | ||
| run: echo "branch=${{ github.head_ref || github.event.workflow_run.head_branch }}" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions script injection github.head_ref workflow_run branch name security
💡 Result:
Short answer: branch names (e.g. github.event.pull_request.head.ref / github.head_ref) and workflow_run triggers are untrusted inputs and have been used to inject shell/Actions expressions to execute arbitrary commands on runners. Treat them as attacker-controlled, avoid interpolating them directly into run: scripts, and apply the mitigations below. [1][2][3][7]
What the risk looks like
- Attackers can create PRs or branches whose names or PR titles contain shell metacharacters or Actions expressions; if your workflow directly inserts those values into a run: block or unescapes outputs, the runner may execute them. Examples and real advisories/CVEs exist (expression/script injection and a branch-names action command-injection advisory/CVE). [1][2][3][6]
Minimal, effective mitigations (apply all that make sense)
- Never inject untrusted values directly into run: scripts. Use environment variables (env:) or pass values safely so they are treated as literals, not shell code. [1][5]
- Prefer immutable refs (head.sha) instead of branch names (head.ref) when checking out code. Avoid git checkout of a mutable branch name coming from a PR. [3][7]
- Sanitize/escape before writing to GITHUB_OUTPUT or files; avoid eval or patterns that unescape input (e.g., do not use eval printf "%s"). Use safe printf "%s" or proper quoting. (See patched guidance for branch-names action.) [1][2]
- For workflow_run workflows: restrict branch filters, verify origin (e.g., require github.event.workflow_run.event != 'pull_request' or check workflow_run.conclusion/source), and treat all artifacts as untrusted (download to $RUNNER_TEMP, validate contents). Limit permissions. [4]
- Don’t run privileged workflows (with write/contents/admin permissions or GITHUB_TOKEN with repo write) on events that can be triggered/poisoned by untrusted PRs (pull_request, workflow_run triggered by PRs). Require manual approval or use protected branch policies where appropriate. [3][4][7]
- Pin third‑party actions and inspect their code for unsafe shell usage (look for eval, unescaped printf, or writing unsanitized outputs). Update or replace vulnerable actions. [2][4]
References (examples and guidance)
- GitHub Docs: Script injections, list of risky github context fields (body, head_ref, ref, title, etc.) and mitigation guidance. [1]
- GitHub Security Lab advisories (expression injection examples in workflows). [3][2]
- GitHub advisory / CVE for tj-actions/branch-names (command injection via unsafe unescaping; shows real PoC and fix). [6]
- GitHub Well‑Architected / Actions security recommendations (workflow_run hardening, branch filters, treat artifacts as untrusted). [4]
- Industry writeups on Actions script/code injection and safe patterns (examples of env usage vs direct interpolation). [5]
If you want, paste the specific workflow YAML you’re concerned about and I’ll point to exact lines that are vulnerable and show a fixed version.
🌐 Web query:
GitHub Actions environment context variables security trusted untrusted
💡 Result:
Quick summary — how GitHub Actions treats environment/context variables and what to do for security (trusted vs untrusted code):
Key rules
- Treat any data originating from external actors (PRs from forks, issue/event fields, user inputs) as untrusted — don’t pass it directly into shell commands, eval, or programmatic templates without sanitizing. [1][2]
- Never expose repository/organization secrets or long-lived tokens to untrusted code (fork PRs or any code you didn’t review). Use workflows that run PR code with no secrets. Use pull_request_target only for metadata tasks — it runs with base-repo permissions and can access secrets, so it’s dangerous if it checks out or runs untrusted code. [2][3]
- Limit secrets/tokens scope and job permissions (permissions:, protected environments, branch protections) so even a compromised runner has least privilege. [3][4]
- Don’t create environment variables from untrusted contexts (github.event.*, issue titles, PR body) and then export/use them in steps that run external code; these can be printed, injected, or used to exfiltrate secrets. [1][2][4]
- Pin third‑party actions to full commit SHAs (not
@main/@latest) and audit action code that will run in your workflow. [3]
Practical controls
- Use secrets for sensitive values (secrets.*); GitHub masks secrets in logs but avoid accidental logging by not echoing untrusted-derived env values. [5]
- Use separate jobs: one (pull_request_target or workflow_dispatch) that can access secrets for controlled orchestration, and another (pull_request) that checks out and runs untrusted PR code with no secrets. Gate secret-using jobs behind manual approvals or protected environments. [3]
- Sanitize/whitelist inputs from events before using them in expressions or runs; prefer safe expression-time checks (if:) rather than shell interpolation when possible. [1][2]
- Avoid writing secrets into files or envs accessible to later steps run by untrusted code; use ephemeral scoped tokens and protect deployment steps. [2][4]
References
- Contexts and warnings about untrusted input (github, env, etc.). [1]
- GitHub Security Lab: untrusted input risks and examples (exfiltration via env/echo). [2]
- Community & GitHub guidance on safe use of pull_request_target, gating, least privilege, and workflow patterns. [3]
- Variables/Environment docs and default-variable behavior. [4]
- Secret masking and logging guidance. [5]
If you want, I can produce a short example workflow pattern that separates trusted and untrusted jobs and demonstrates the safe handling above.
🏁 Script executed:
# Check if there are other similar patterns in the workflow file that might indicate
# the project's current security practices
cd .github/workflows && cat ci-tests-e2e.yaml | grep -n "run:" | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 681
Pass untrusted branch names safely to avoid shell injection.
github.head_ref and workflow_run.head_branch are user-controlled and can contain shell metacharacters. Even within double quotes, they can break out or inject code. Use an environment variable with printf %s to treat the value as a literal string:
Safer output write
- name: Get branch name
id: get-branch
- run: echo "branch=${{ github.head_ref || github.event.workflow_run.head_branch }}" >> $GITHUB_OUTPUT
+ env:
+ BRANCH: ${{ github.head_ref || github.event.workflow_run.head_branch }}
+ run: |
+ printf 'branch=%s\n' "$BRANCH" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.10)
232-232: "github.head_ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/ci-tests-e2e.yaml around lines 190 - 233, The get-branch
step currently writes untrusted inputs (github.head_ref /
github.event.workflow_run.head_branch) to $GITHUB_OUTPUT using echo, which can
be subject to shell injection; update the get-branch step to capture the branch
into a safe environment variable and write it with a literal-safe formatter (use
printf '%s' or equivalent) instead of echo so the value is treated as data not
shell code; specifically, change the run in the get-branch step that sets branch
to use printf '%s' with the expression (github.head_ref ||
github.event.workflow_run.head_branch) to append the literal branch string to
$GITHUB_OUTPUT.
| needs: [playwright-tests, merge-reports, get-pr-info] | ||
| runs-on: ubuntu-latest | ||
| if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false | ||
| if: always() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/ci-tests-e2e.yaml | head -300 | tail -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4286
🏁 Script executed:
rg -n "deploy-and-comment|get-pr-info" .github/workflows/ci-tests-e2e.yaml -A 20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3103
Guard deploy-and-comment job when PR info is missing (forks or non‑PR workflow_run).
The if: always() condition allows this job to run even when get-pr-info is skipped (due to fork or non-PR workflow_run events) or when it returns empty outputs, causing the job to fail with an invalid PR number passed to the deployment script.
✅ Gate on a valid PR number
deploy-and-comment:
- if: always()
+ if: ${{ always() && needs.get-pr-info.outputs.pr_number != '' }}🤖 Prompt for AI Agents
In @.github/workflows/ci-tests-e2e.yaml around lines 266 - 268, The
deploy-and-comment job currently uses if: always() which lets it run even when
get-pr-info was skipped or returned no PR number; change the job's if condition
to require a successful get-pr-info and a non-empty PR number, e.g. replace if:
always() with a guard like if: needs.get-pr-info.result == 'success' &&
needs.get-pr-info.outputs.pr_number != '' so the job (referenced as
deploy-and-comment and the get-pr-info job) only runs when a valid PR number is
present.
|
@coderabbitai review full |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if [ -n "$PR_NUMBER" ]; then | ||
| echo "number=${PR_NUMBER}" >> $GITHUB_OUTPUT | ||
| else | ||
| gh pr view --repo "${PR_TARGET_REPO}" "${PR_BRANCH}" \ |
There was a problem hiding this comment.
[security] medium Priority
Issue: Environment variables used in conditional logic without validation
Context: Using gh commands with environment variables can lead to shell injection if branch names contain special characters
Suggestion: Add proper input validation and quoting around all variables passed to shell commands
| # Run after i18n workflow completes for version-bump PRs | ||
| workflow_run: | ||
| workflows: ['i18n: Update Core'] | ||
| types: [completed] |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Potential race condition with workflow_run trigger
Context: workflow_run trigger runs on completed workflows, but doesn't verify if the i18n workflow actually succeeded - it will run even if i18n failed
Suggestion: Add condition to check workflow_run.conclusion == 'success' to ensure E2E only runs after successful i18n completion
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| group: ${{ github.workflow }}-${{ github.event.workflow_run.head_branch || github.ref }} |
There was a problem hiding this comment.
[architecture] low Priority
Issue: Concurrency group may cause unexpected cancellations across different event types
Context: Using the same concurrency group for both pull_request and workflow_run events could cause workflows to cancel each other when they should run independently
Suggestion: Consider using separate concurrency groups for different event types to prevent cross-event cancellation
| PR_TARGET_REPO: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| PR_BRANCH: ${{ github.head_ref || github.event.workflow_run.head_branch }} | ||
| run: | |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Missing error handling for gh command failures
Context: If the gh pr view command fails (e.g., PR deleted, permissions issue), the workflow will continue with empty PR_NUMBER
Suggestion: Add error checking after the gh command and fail the job if PR info cannot be retrieved: set -e at the beginning of the script block or explicit error checking
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: fix: run E2E tests after i18n completes on release PRs (#8111)
Impact: 58 additions, 11 deletions across 1 file
Issue Distribution
- Critical: 0
- High: 0
- Medium: 3
- Low: 2
Category Breakdown
- Architecture: 2 issues
- Security: 2 issues
- Performance: 0 issues
- Code Quality: 1 issue
Key Findings
Architecture & Design
The PR introduces a workflow_run trigger to solve the E2E/i18n timing issue, which is a good architectural approach. However, there are two concerns:
- The workflow_run trigger doesn't verify successful completion of the triggering workflow, which could lead to E2E tests running after failed i18n workflows
- The concurrency group configuration may cause unexpected interactions between pull_request and workflow_run events
Security Considerations
The implementation has moderate security concerns around input validation and permission contexts:
- Environment variables are used in shell commands without proper validation, creating potential injection risks if branch names contain special characters
- Missing explicit permissions for workflow_run events, which operate in a different security context than pull_request events
Performance Impact
No significant performance issues identified. The workflow optimization should actually improve performance by reducing cancelled E2E runs.
Integration Points
The changes properly handle both pull_request and workflow_run event contexts, with appropriate branching logic for checkout operations and PR information retrieval.
Positive Observations
- Well-structured approach to solving the cancellation problem
- Proper handling of different GitHub event contexts (pull_request vs workflow_run)
- Good code organization with the new get-pr-info job for reusability
- Clear comments explaining the purpose of the workflow_run trigger
References
Next Steps
- Address medium priority issues (workflow conclusion checking, input validation)
- Consider adding explicit permissions for better security
- Test the workflow with various branch name scenarios
- Verify the solution works correctly with version-bump PRs
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
.github/workflows/ci-tests-e2e.yaml
Outdated
| pull_request: | ||
| branches-ignore: | ||
| [wip/*, draft/*, temp/*, vue-nodes-migration, sno-playwright-*] | ||
| [wip/*, draft/*, temp/*, vue-nodes-migration, sno-playwright-*, version-bump-*] |
There was a problem hiding this comment.
btw I think its time to remove "sno-playwright-*", it was added for test XD
There was a problem hiding this comment.
I think this is good to merge after removed 'sno-playwright-*' cc @DrJKL
.github/workflows/ci-tests-e2e.yaml
Outdated
| setup: | ||
| runs-on: ubuntu-latest | ||
| # Skip version-bump PRs on pull_request (they run via workflow_run after i18n completes) | ||
| if: ${{ !(github.event_name == 'pull_request' && startsWith(github.head_ref, 'version-bump-')) }} |
…flare Pages deployment.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci-tests-e2e.yaml:
- Around line 10-13: The workflow_run trigger currently listens to any
completion of the i18n workflow; add a branches filter under workflow_run (e.g.,
branches: [<version-bump-branch-name-or-pattern>]) to restrict it to the
version-bump branches and keep workflows: ['i18n: Update Core'] and types:
['completed']; then update downstream jobs (for example the setup job) to gate
execution on the upstream conclusion by adding an if condition that checks
github.event_name != 'workflow_run' || github.event.workflow_run.conclusion ==
'success' so the job only runs when the workflow_run succeeded or when triggered
by other events.
♻️ Duplicate comments (3)
.github/workflows/ci-tests-e2e.yaml (3)
247-250: Guarddeploy-and-commentjob when PR info is unavailable.The
if: always()condition allows this job to run even whenget-pr-infowas skipped (e.g., push events) or failed, which would cause the deployment script to receive emptyPR_NUMBERandBRANCHarguments.🔧 Suggested fix
deploy-and-comment: needs: [playwright-tests, merge-reports, get-pr-info] runs-on: ubuntu-latest - if: always() + if: ${{ always() && needs.get-pr-info.outputs.pr_number != '' }}
207-214: Useprintfto safely write outputs and add error handling forghcommand.Line 208 uses
echowith variable expansion which can be vulnerable to injection ifPR_BRANCHcontains special characters like newlines. Additionally, if thegh pr viewcommand fails (e.g., PR deleted, permissions issue), the job continues with an empty PR number.🔧 Suggested fix
run: | - echo "branch=${PR_BRANCH}" >> $GITHUB_OUTPUT + printf 'branch=%s\n' "$PR_BRANCH" >> "$GITHUB_OUTPUT" if [ -n "$PR_NUMBER" ]; then - echo "number=${PR_NUMBER}" >> $GITHUB_OUTPUT + printf 'number=%s\n' "$PR_NUMBER" >> "$GITHUB_OUTPUT" else - gh pr view --repo "${PR_TARGET_REPO}" "${PR_BRANCH}" \ - --json 'number' --jq '"number=\(.number)"' >> $GITHUB_OUTPUT + PR_NUM=$(gh pr view --repo "${PR_TARGET_REPO}" "${PR_BRANCH}" \ + --json 'number' --jq '.number') || { + echo "::error::Failed to retrieve PR number for branch ${PR_BRANCH}" + exit 1 + } + printf 'number=%s\n' "$PR_NUM" >> "$GITHUB_OUTPUT" fi
193-195: Consider adding branch filter toworkflow_runcondition.If the
workflow_runtrigger-levelbranchesfilter (suggested earlier) is not applied, this condition should also filter for version-bump branches to prevent running on unrelated i18n workflow completions:if: | (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || - (github.event_name == 'workflow_run') + (github.event_name == 'workflow_run' && startsWith(github.event.workflow_run.head_branch, 'version-bump-'))This becomes unnecessary if you add
branches: ['version-bump-*']to the trigger definition.
| # Run after i18n workflow completes for version-bump PRs | ||
| workflow_run: | ||
| workflows: ['i18n: Update Core'] | ||
| types: [completed] |
There was a problem hiding this comment.
Add branch filter and success check to workflow_run trigger.
The workflow_run trigger currently fires for any completion of the i18n workflow on any branch, not just version-bump PRs. Additionally, types: [completed] fires regardless of whether the i18n workflow succeeded or failed.
Add a branches filter and check workflow_run.conclusion in downstream jobs:
workflow_run:
workflows: ['i18n: Update Core']
types: [completed]
+ branches:
+ - 'version-bump-*'Then add a conclusion check to the setup job (or relevant jobs):
if: >
github.event_name != 'workflow_run' ||
github.event.workflow_run.conclusion == 'success'🤖 Prompt for AI Agents
In @.github/workflows/ci-tests-e2e.yaml around lines 10 - 13, The workflow_run
trigger currently listens to any completion of the i18n workflow; add a branches
filter under workflow_run (e.g., branches:
[<version-bump-branch-name-or-pattern>]) to restrict it to the version-bump
branches and keep workflows: ['i18n: Update Core'] and types: ['completed'];
then update downstream jobs (for example the setup job) to gate execution on the
upstream conclusion by adding an if condition that checks github.event_name !=
'workflow_run' || github.event.workflow_run.conclusion == 'success' so the job
only runs when the workflow_run succeeded or when triggered by other events.
3 participants
Summary
workflow_runtrigger to run E2E tests after i18n workflow completes on version-bump PRspull_requesttrigger for version-bump branches to avoid cancellation by i18n commitsworkflow_runeventsProblem
When a version-bump PR is created, both E2E tests and i18n workflow trigger simultaneously. The i18n workflow commits locale updates after ~5-8 minutes, which triggers a new E2E run that cancels the in-progress tests due to
cancel-in-progress: true.Solution
Skip E2E on
pull_requestfor version-bump PRs, run viaworkflow_runafter i18n completes instead.Test plan
┆Issue is synchronized with this Notion page by Unito