fix: Split PR deployment workflow for forked vs non-forked repos #5425

snomiao · 2025-09-08T03:02:15Z

Summary

Separates deployment workflows to handle forked and non-forked PRs differently
Extracts common deployment logic to a reusable shell script
Fixes permissions issues for forked repository PRs

Changes

Created scripts/cicd/pr-playwright-deploy-and-comment.sh: Reusable script for deploying Playwright reports and commenting on PRs
Updated test-ui.yaml: Added deployment for non-forked PRs using direct pull_request events (faster)
Updated pr-playwright-deploy.yaml: Now only handles forked PRs via workflow_run events (solves permission issues)

Benefits

Faster deployment for non-forked PRs: Direct pull_request events run immediately
Proper permissions for forked PRs: workflow_run events have necessary permissions to comment
DRY principle: Shared script reduces code duplication and maintenance burden
Starting comments: Both workflows now post "Tests are starting..." comments

Test plan

Test with a non-forked PR (should use test-ui.yaml workflow)
Test with a forked PR (should use pr-playwright-deploy.yaml workflow)
Verify starting comments appear for both cases
Verify deployment links work correctly
Verify comments update properly

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

…links The issue in PR #5298 was caused by missing deployment-info artifact creation. The deploy-reports job was deploying to Cloudflare but wasn't creating the deployment-info-* artifacts that comment-tests-completed job expected to download. This change consolidates the deployment and commenting into a single job, eliminating the artifact dependency and ensuring links are always available. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Extract deployment logic to reusable script (scripts/cicd/pr-playwright-deploy-and-comment.sh) - Non-forked PRs: Use direct pull_request event in test-ui.yaml (faster) - Forked PRs: Use workflow_run in pr-playwright-deploy.yaml (handles permissions) - Add starting comment for both forked and non-forked PRs - Make Cloudflare tokens optional for starting status comments

github-actions · 2025-09-08T03:03:16Z

🎭 Playwright Test Results

✅ All tests passed across all browsers!

⏰ Completed at: 09/09/2025, 05:42:38 AM UTC

📊 Test Reports by Browser

🎉 Your tests are passing across all browsers!

- Consolidate workflow into single job with clearer structure - Reduce script from 200+ to ~140 lines - Simplify deployment retry logic and comment generation - Remove redundant checks and unnecessary complexity

- Add debug output to identify missing reports - Install wrangler if not available - Show deployment attempts and failures - Log available reports before deployment

- Use dot notation (0.5x) for artifact names as Playwright creates them - Convert to dash notation (0-5x) for Cloudflare project names - Properly handle browser name display in comments

- Replace bash arrays with space-separated strings - Use while loops instead of bash-specific for syntax - Remove bash-specific string manipulation features - Replace local variables (not required in functions) - Ensure compatibility with standard /bin/sh

- Redirect debug messages to stderr - Only output URL to stdout for proper capture - This fixes the missing deployment links in PR comments

snomiao · 2025-09-08T04:30:36Z

While this pr running comment correctly

However the comment is overwritten by origin/main later, pr-playwright-deploy.yaml

So its showing no links above now, but it should work fine after merge to origin/main

@DrJKL cc @christian-byrne

snomiao · 2025-09-08T14:39:35Z

Here's how to confirm it works at least for non-forked (aka members/collaborators PR) PR

You can also trigger a empty commit to verify this

scripts/cicd/pr-playwright-deploy-and-comment.sh

.github/workflows/pr-playwright-deploy.yaml

scripts/cicd/pr-playwright-deploy-and-comment.sh

christian-byrne

Review Summary

I've added inline comments for specific issues found during security, performance, and quality analysis. The most critical concerns are around input validation and command injection vulnerabilities in the shell script.

Key Findings:

High Priority: Input validation needed to prevent command injection
Medium Priority: Late branch sanitization and unsafe command execution
Low Priority: Performance optimizations and minor reliability improvements

Overall Assessment:

This is a solid architectural improvement that properly separates forked vs non-forked PR workflows. The DRY principle implementation with the shared script significantly reduces maintenance burden. The solution addresses real permission issues while improving deployment speed for non-forked PRs.

Recommendation: Address the high-priority security issues before merging, but the overall approach is sound and ready for production with those fixes.

- Validate PR number is numeric only - Sanitize branch name at script start - Validate status parameter values - Use pre-sanitized branch throughout script - Addresses high-severity security issue from PR review

- Add explicit null checks for head_repository and repository - Add debug logging to help diagnose workflow trigger issues - Prevents potential failures from undefined repository objects - Addresses medium-severity issue from PR review

- Pin wrangler to major version 4 (^4.0.0) for stability - Add error handling if wrangler installation fails - Return 'failed' status if installation fails - Addresses dependency management issue from PR review

- Deploy all browser reports in parallel using background processes - Use temporary directory to collect deployment results - Wait for all deployments to complete before generating comment - Maintains result order for consistent output - Significantly reduces deployment time from sequential to parallel execution

- Use GitHub API to find exact comment ID - Update specific comment by ID to avoid editing wrong comment - Prevents race conditions if user posts between finding and editing - More reliable comment updates

…b for pull requests to ensure deployment consistency

…gler command for consistency and simplicity

Copilot

Pull Request Overview

This PR separates deployment workflows to handle forked and non-forked PRs differently, addressing permissions issues while improving deployment speed. The changes extract common deployment logic into a reusable shell script and implement conditional workflow execution based on repository fork status.

Extracts deployment and commenting logic into a centralized shell script
Implements separate workflow paths for forked vs non-forked PRs to handle different permission models
Adds immediate "starting" comments and faster deployment for non-forked PRs

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
scripts/cicd/pr-playwright-deploy-and-comment.sh	New reusable script that handles PR deployment and commenting logic
.github/workflows/test-ui.yaml	Added deployment steps for non-forked PRs using direct pull_request events
.github/workflows/pr-playwright-deploy.yaml	Simplified to handle only forked PRs via workflow_run events

scripts/cicd/pr-playwright-deploy-and-comment.sh

.github/workflows/pr-playwright-deploy.yaml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ction to include valid characters and ensure correct URL format

… bash for better compatibility with script features docs(pr-playwright-deploy-and-comment.sh): add warning comment to deploy_report function about input sanitization

…n to the beginning of the script to avoid redundancy and improve efficiency

snomiao · 2025-09-09T05:33:39Z

@christian-byrne Thank you for reviews! Just fixed all suggestions and its ready for review again :D

snomiao · 2025-09-09T05:35:17Z

flaky link visibility test on shard2 will be solved here:

fix: Stabilize flaky link visibility toggle test by snomiao · Pull Request #5447 · Comfy-Org/ComfyUI_frontend

* [fix] Consolidate Playwright workflow jobs to fix missing deployment links The issue in PR #5298 was caused by missing deployment-info artifact creation. The deploy-reports job was deploying to Cloudflare but wasn't creating the deployment-info-* artifacts that comment-tests-completed job expected to download. This change consolidates the deployment and commenting into a single job, eliminating the artifact dependency and ensuring links are always available. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: Split PR deployment workflow for forked vs non-forked repos - Extract deployment logic to reusable script (scripts/cicd/pr-playwright-deploy-and-comment.sh) - Non-forked PRs: Use direct pull_request event in test-ui.yaml (faster) - Forked PRs: Use workflow_run in pr-playwright-deploy.yaml (handles permissions) - Add starting comment for both forked and non-forked PRs - Make Cloudflare tokens optional for starting status comments * refactor: Simplify PR deployment workflow and script - Consolidate workflow into single job with clearer structure - Reduce script from 200+ to ~140 lines - Simplify deployment retry logic and comment generation - Remove redundant checks and unnecessary complexity * fix: Add debugging and wrangler installation to deployment script - Add debug output to identify missing reports - Install wrangler if not available - Show deployment attempts and failures - Log available reports before deployment * chore: Trigger CI to test deployment workflow * fix: Fix browser artifact name mismatch in deployment script - Use dot notation (0.5x) for artifact names as Playwright creates them - Convert to dash notation (0-5x) for Cloudflare project names - Properly handle browser name display in comments * refactor: Convert deployment script to POSIX sh for better compatibility - Replace bash arrays with space-separated strings - Use while loops instead of bash-specific for syntax - Remove bash-specific string manipulation features - Replace local variables (not required in functions) - Ensure compatibility with standard /bin/sh * fix: Fix deployment script output to properly capture URLs - Redirect debug messages to stderr - Only output URL to stdout for proper capture - This fixes the missing deployment links in PR comments * fix: Add input validation to prevent command injection - Validate PR number is numeric only - Sanitize branch name at script start - Validate status parameter values - Use pre-sanitized branch throughout script - Addresses high-severity security issue from PR review * fix: Add null checks and logging to workflow condition - Add explicit null checks for head_repository and repository - Add debug logging to help diagnose workflow trigger issues - Prevents potential failures from undefined repository objects - Addresses medium-severity issue from PR review * fix: Pin wrangler to major version 4 with error handling - Pin wrangler to major version 4 (^4.0.0) for stability - Add error handling if wrangler installation fails - Return 'failed' status if installation fails - Addresses dependency management issue from PR review * perf: Implement parallel deployments to reduce CI time - Deploy all browser reports in parallel using background processes - Use temporary directory to collect deployment results - Wait for all deployments to complete before generating comment - Maintains result order for consistent output - Significantly reduces deployment time from sequential to parallel execution * fix: Use specific comment ID for updates instead of edit-last - Use GitHub API to find exact comment ID - Update specific comment by ID to avoid editing wrong comment - Prevents race conditions if user posts between finding and editing - More reliable comment updates * fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency * fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency * fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity * fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity * Update scripts/cicd/pr-playwright-deploy-and-comment.sh Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(pr-playwright-deploy-and-comment.sh): improve regex for URL extraction to include valid characters and ensure correct URL format * chore(pr-playwright-deploy-and-comment.sh): move wrangler installation to the beginning of the script to avoid redundancy and improve efficiency --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

## Summary - Improves Storybook deployment and PR comment workflow similar to the Playwright improvements in #5425 - Creates unified deployment and commenting system for better maintainability - Adds Cloudflare Pages deployment for Storybook previews ## Deployment Cases Matrix | Case | PR Type | Branch | Deployment | Features | |------|---------|--------|------------|----------| | **1** | Non-forked PR | `version-bump-*` | ✅ Chromatic | • Visual diff testing • Chromatic build URL • Chromatic Storybook URL • Shows visual changes | | **2** | Non-forked PR | All branches | ✅ Cloudflare Pages | • Live Storybook preview • pages.dev URL • No visual diff | | **3** | Forked PR | Any branch | ✅ Cloudflare Pages | • Live Storybook preview • pages.dev URL • No visual diff • Runs via separate workflow to avoid permission problems | ### Key Points: - **Chromatic** (paid service): Only for `version-bump-*` branches to track visual changes between releases - **Cloudflare Pages** (free): For all other PRs to provide Storybook preview without visual diff - **Security**: Forked PRs use a separate workflow with limited permissions ## Changes ### New Features - 🚀 **Cloudflare Pages Deployment**: Storybook builds are now deployed to Cloudflare Pages for easy preview - 🔄 **Unified Script**: Single reusable shell script handles both deployment and PR comments - 🔒 **Better Security**: Separate workflows for fork vs non-fork PRs ### Improvements - ♻️ **Retry Logic**: Automatic retry (3 attempts) for failed deployments - 📝 **Better Comments**: Clearer PR comments with deployment links and status - 🎯 **Simplified Logic**: Workflow logic moved to reusable script for easier maintenance - ⚡ **Better Error Handling**: Proper handling of different workflow conclusions - 🐛 **Fixed Comment Output**: Deployment logs now properly redirected to stderr ### Files Changed - `scripts/cicd/pr-storybook-deploy-and-comment.sh` - New unified deployment script - `.github/workflows/chromatic.yaml` - Updated to use new script and add deployment - `.github/workflows/pr-storybook-deploy.yaml` - New workflow for forked PRs - `.github/workflows/pr-storybook-comment.yaml` - Removed (replaced by new system) ## ⚠️ Required Setup The Cloudflare Pages project `comfyui-storybook` needs to be created under the organization's Cloudflare account: ```bash # Using the account ID from GitHub secrets export CLOUDFLARE_ACCOUNT_ID=5ae914d9b87bcf6bbe1ada5798f92a5f export CLOUDFLARE_API_TOKEN=<org-token> wrangler pages project create comfyui-storybook --production-branch main ``` **Note**: The project must be created under the same Cloudflare account that's configured in the GitHub secrets. ## Test Plan - [x] Create Cloudflare Pages project `comfyui-storybook` - [x] Workflow runs successfully on all PRs - [x] PR comments are posted correctly at start and completion - [x] Storybook deploys to Cloudflare Pages with correct URL - [ ] Fork PRs are handled by separate workflow - [ ] Non-fork PRs get inline deployment - [ ] version-bump-* branches show Chromatic info ## References - Similar improvements for Playwright: #5459 - Based on pattern from sno-fix-playwright-comment-2 branch 🤖 Generated with [Claude Code](https://claude.ai/code) --------- Co-authored-by: Claude <noreply@anthropic.com>

- Improves Storybook deployment and PR comment workflow similar to the Playwright improvements in #5425 - Creates unified deployment and commenting system for better maintainability - Adds Cloudflare Pages deployment for Storybook previews | Case | PR Type | Branch | Deployment | Features | |------|---------|--------|------------|----------| | **1** | Non-forked PR | `version-bump-*` | ✅ Chromatic | • Visual diff testing • Chromatic build URL • Chromatic Storybook URL • Shows visual changes | | **2** | Non-forked PR | All branches | ✅ Cloudflare Pages | • Live Storybook preview • pages.dev URL • No visual diff | | **3** | Forked PR | Any branch | ✅ Cloudflare Pages | • Live Storybook preview • pages.dev URL • No visual diff • Runs via separate workflow to avoid permission problems | - **Chromatic** (paid service): Only for `version-bump-*` branches to track visual changes between releases - **Cloudflare Pages** (free): For all other PRs to provide Storybook preview without visual diff - **Security**: Forked PRs use a separate workflow with limited permissions - 🚀 **Cloudflare Pages Deployment**: Storybook builds are now deployed to Cloudflare Pages for easy preview - 🔄 **Unified Script**: Single reusable shell script handles both deployment and PR comments - 🔒 **Better Security**: Separate workflows for fork vs non-fork PRs - ♻️ **Retry Logic**: Automatic retry (3 attempts) for failed deployments - 📝 **Better Comments**: Clearer PR comments with deployment links and status - 🎯 **Simplified Logic**: Workflow logic moved to reusable script for easier maintenance - ⚡ **Better Error Handling**: Proper handling of different workflow conclusions - 🐛 **Fixed Comment Output**: Deployment logs now properly redirected to stderr - `scripts/cicd/pr-storybook-deploy-and-comment.sh` - New unified deployment script - `.github/workflows/chromatic.yaml` - Updated to use new script and add deployment - `.github/workflows/pr-storybook-deploy.yaml` - New workflow for forked PRs - `.github/workflows/pr-storybook-comment.yaml` - Removed (replaced by new system) The Cloudflare Pages project `comfyui-storybook` needs to be created under the organization's Cloudflare account: ```bash export CLOUDFLARE_ACCOUNT_ID=5ae914d9b87bcf6bbe1ada5798f92a5f export CLOUDFLARE_API_TOKEN=<org-token> wrangler pages project create comfyui-storybook --production-branch main ``` **Note**: The project must be created under the same Cloudflare account that's configured in the GitHub secrets. - [x] Create Cloudflare Pages project `comfyui-storybook` - [x] Workflow runs successfully on all PRs - [x] PR comments are posted correctly at start and completion - [x] Storybook deploys to Cloudflare Pages with correct URL - [ ] Fork PRs are handled by separate workflow - [ ] Non-fork PRs get inline deployment - [ ] version-bump-* branches show Chromatic info - Similar improvements for Playwright: #5459 - Based on pattern from sno-fix-playwright-comment-2 branch 🤖 Generated with [Claude Code](https://claude.ai/code) --------- Co-authored-by: Claude <noreply@anthropic.com>

snomiao requested a review from a team as a code owner September 8, 2025 03:02

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 8, 2025

snomiao and others added 2 commits September 8, 2025 03:02

snomiao force-pushed the sno-fix-playwright-comment-2 branch from 2d5a3e4 to 809c4cc Compare September 8, 2025 03:03

refactor: Simplify PR deployment workflow and script

777e850

- Consolidate workflow into single job with clearer structure - Reduce script from 200+ to ~140 lines - Simplify deployment retry logic and comment generation - Remove redundant checks and unnecessary complexity

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Sep 8, 2025

fix: Add debugging and wrangler installation to deployment script

4a88c7c

- Add debug output to identify missing reports - Install wrangler if not available - Show deployment attempts and failures - Log available reports before deployment

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 8, 2025

snomiao added 4 commits September 8, 2025 03:35

chore: Trigger CI to test deployment workflow

452072d

fix: Fix browser artifact name mismatch in deployment script

dd00ad9

- Use dot notation (0.5x) for artifact names as Playwright creates them - Convert to dash notation (0-5x) for Cloudflare project names - Properly handle browser name display in comments

fix: Fix deployment script output to properly capture URLs

00b8e5d

- Redirect debug messages to stderr - Only output URL to stdout for proper capture - This fixes the missing deployment links in PR comments

snomiao requested review from DrJKL and christian-byrne September 8, 2025 06:21