Make /run-skipped-ci persistent using full-ci label #2007
Conversation
Changes: - Add full-ci label when /run-skipped-ci is triggered - Create /stop-run-skipped-ci command to remove the label - Update all workflows to check for full-ci label in addition to force_run input - Label persists across commits, ensuring full CI runs until explicitly stopped This fixes the issue where /run-skipped-ci would start tests but they'd stop when committing. Now the label keeps full CI enabled across all future commits until /stop-run-skipped-ci is run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
👋 Thanks for opening this PR! 🚀 Running Full CI SuiteBy default, PRs run a subset of CI jobs for faster feedback (latest Ruby/Node versions only). To run the complete CI suite including all dependency combinations and skipped jobs, comment: This will trigger:
The full CI suite takes longer but ensures compatibility across all supported versions before merging. |
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughAdds label-driven "full-ci" control to CI: workflows now check for a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PR as "Pull Request / Comments"
participant GH as "GitHub API"
participant Detect as "detect-changes job"
participant CI as "Downstream CI jobs"
User->>PR: Add `full-ci` label (manual or via /run-skipped-ci)
Detect->>GH: Query PR labels (check-label step)
GH-->>Detect: returns has_full_ci_label = true/false
Detect->>Detect: set output `has_full_ci_label`
alt has_full_ci_label == true OR force_run == true
Detect->>CI: allow full matrix (no minimum exclusions)
else
Detect->>CI: preserve existing change-based exclusions
end
User->>PR: Comment `/stop-run-skipped-ci`
PR->>GH: workflow verifies commenter permissions
alt has write/admin access
GH->>PR: remove `full-ci` label
GH-->>PR: post confirmation comment
else
GH-->>PR: post permission denied comment
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Code ReviewThis PR adds persistence to /run-skipped-ci using a full-ci label. Well-designed solution! Strengths:
Code Quality Issues:
Recommendations: High Priority:
Medium Priority:
Security: Good - proper permission checks, no injection risks Verdict: Low risk, good quality, ready after addressing high-priority items. Great work! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/main.yml (1)
184-186: Duplicate matrix exclusion logic for dummy-app-integration-tests job.This duplicates the exclusion pattern from build-dummy-app-webpack-test-bundles (lines 92-96). Consider extracting this as a reusable workflow or documentation to maintain consistency if the logic needs to change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/examples.yml(2 hunks).github/workflows/main.yml(3 hunks).github/workflows/pro-integration-tests.yml(1 hunks).github/workflows/pro-lint.yml(1 hunks).github/workflows/pro-package-tests.yml(1 hunks).github/workflows/run-skipped-ci.yml(2 hunks).github/workflows/stop-run-skipped-ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/stop-run-skipped-ci.yml
[error] 86-86: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (11)
.github/workflows/run-skipped-ci.yml (1)
231-243: YAML syntax issue in conditional string.Line 231-241 uses template literals with nested conditionals. Line 236 and 243 should be reviewed for proper YAML string escaping, though this appears to be valid YAML (likely false positive from static analysis on similar syntax in another file).
.github/workflows/pro-package-tests.yml (2)
33-51: Label check implementation is consistent with other workflows.The step properly:
- Guards against non-PR contexts
- Fetches labels via GitHub API
- Returns boolean result
- Logs presence for debugging
57-57: Detection logic correctly integrates label check.The condition properly combines
force_runinput withhas_full_ci_labeldetection, enabling full CI via either trigger mechanism..github/workflows/pro-integration-tests.yml (1)
33-51: Label check implementation mirrors pro-package-tests.yml pattern.Implementation is consistent across workflows with proper non-PR guards and output exposure.
.github/workflows/pro-lint.yml (1)
33-51: Label check implementation consistent with other Pro workflows..github/workflows/stop-run-skipped-ci.yml (2)
1-20: Permissions and access control are properly configured.The workflow correctly:
- Limits to PR comments
- Checks user write access
- Declares necessary permissions (pull-requests: write, issues: write)
- Exits early if access denied
64-111: Label removal has robust error handling.The implementation properly:
- Attempts label removal
- Handles 404 (label not present) gracefully
- Handles unexpected errors with detailed feedback
- Posts informative comments for all outcomes
.github/workflows/main.yml (2)
39-57: Label check implementation is consistent with established pattern.The step properly guards non-PR contexts and exposes output for downstream use.
92-96: Matrix exclusion logic will not work as intended — empty string values don't exclude the intended matrix row.According to GitHub Actions documentation, matrix exclusion only removes entries that exactly match the specified keys/values; empty strings do not match non-empty values. When the condition is false (regular PRs), the exclude entry becomes
{ruby-version: '', node-version: '', dependency-level: ''}, which does not match the actual matrix row{ruby-version: '3.2', node-version: '20', dependency-level: 'minimum'}. This means the minimum-dependency variant will still run on regular PRs instead of being skipped.The exclusion logic needs to be rewritten using a different approach (e.g., a conditional
ifstatement at the job level or usingmatrix.include/matrix.excludewith proper matching).Likely an incorrect or invalid review comment.
.github/workflows/examples.yml (2)
39-57: Label check implementation is consistent with established pattern across all workflows.
92-94: Matrix exclusion properly gates minimum dependencies on full-ci label.The exclusion correctly skips the minimum dependency variant on regular PRs unless force_run or full-ci label is present. Pattern is consistent with main.yml.
Code Review: Full CI Label ImplementationOverviewThis PR implements a persistent label-based mechanism for controlling full CI execution. The approach is sound and addresses a real workflow issue where full CI runs would be interrupted by new commits. ✅ Strengths
🔍 Issues & Concerns1. Critical Bug: Output Mismatch
|
Replace template literals with array.join() to avoid YAML multiline key parsing issues. This fixes the knip CI failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The check-label step returns `result` output, but workflows were incorrectly referencing it as `has_label`, causing the full-ci label feature to be completely broken. Fixed in all 5 workflows: - .github/workflows/main.yml - .github/workflows/examples.yml - .github/workflows/pro-integration-tests.yml - .github/workflows/pro-lint.yml - .github/workflows/pro-package-tests.yml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code ReviewThis PR implements a persistent full CI mode using GitHub labels, solving the problem where CI tests would be interrupted when new commits are pushed. Positive Aspects
Issues & Recommendations1. Missing issues: write Permission (Medium Priority)Location: .github/workflows/run-skipped-ci.yml:25 The workflow adds a label but is missing the issues: write permission. While GitHub may sometimes allow label operations with just pull-requests: write, explicitly declaring issues: write is the documented requirement and prevents potential permission errors. 2. Output Variable Mismatch (Medium Priority)Location: All detect-changes jobs in updated workflows The output variable reference is incorrect in all 5 workflow files. Currently uses steps.check-label.outputs.has_label but should be steps.check-label.outputs.result. The GitHub script action uses the implicit outputs.result for the return value, not outputs.has_label. This may cause the label check to not work correctly. 3. Test Plan Not CompleteAll test plan items in the PR description are unchecked. These should be completed before merging. Recommendations SummaryMust Fix Before Merge:
Nice to Have:
Overall AssessmentStatus: Needs fixes before merge This is a well-designed solution to a real problem. The implementation is clean and user-friendly. However, the output variable mismatch needs to be fixed as it may cause the label check to fail. After addressing these issues and completing testing, this will be a great addition to the project. |
Code Review: Make /run-skipped-ci persistent using full-ci labelOverviewThis PR implements a smart solution to a real workflow problem - making full CI runs persistent across commits. The approach using a GitHub label is elegant and follows GitHub Actions best practices. ✅ Strengths1. Clean Architecture
2. User Experience
3. Security
4. Robustness
🔍 Issues & SuggestionsHigh Priority1. Missing The workflow adds a label but doesn't have the # .github/workflows/run-skipped-ci.yml:22-25
permissions:
contents: read
pull-requests: write
actions: writeAdd permissions:
contents: read
pull-requests: write
actions: write
issues: write # Required for adding labelsReference: stop-run-skipped-ci.yml:18-20 already has this correctly. 2. Inconsistent comment reactions
Consider using consistent reactions (both 'rocket' or both 'eyes') for better UX. Medium Priority3. Label check code duplication The exact same 20-line label-checking script appears in 5 different workflow files. Consider:
4. Complex exclude logic may be fragile The exclude conditions in matrix jobs are quite complex. This is hard to read and test. Consider:
5. No cleanup on PR close When a PR is closed/merged, the Low Priority6. Message formatting inconsistencies In run-skipped-ci.yml changes:
The message strings have inconsistent indentation. While functional, consistent formatting improves maintainability. 7. Documentation mentions non-existent files The PR description says "All workflows updated" but doesn't mention that
🎯 Testing RecommendationsBeyond the test plan in the PR description, also verify:
📊 Code Quality Metrics
🔒 Security Assessment✅ No security concerns identified
💡 Future EnhancementsConsider in follow-up PRs:
Verdict: ✅ Approve with minor fixesThis is a well-designed solution that solves a real problem. The issues identified are mostly minor polish items. The critical fix needed is adding Once that's addressed, this PR will significantly improve the developer experience when debugging CI failures. Required before merge:
Nice to have:
Great work on this feature! 🚀 |
This commit enhances the /run-skipped-ci feature based on code review feedback: **Critical Fixes:** - Fix GitHub Actions output encoding to use explicit string values - Add result-encoding: string to all label check steps - Return 'true'/'false' strings instead of booleans for consistent shell comparison - Fixes potential silent failures in label detection across 6 workflow files **New Features:** - Add automatic full-ci label cleanup when PRs are merged - Add welcome message to new PRs explaining CI control commands - Comprehensive documentation in CONTRIBUTING.md **Files Changed:** - .github/workflows/main.yml - Fixed output encoding - .github/workflows/examples.yml - Fixed output encoding - .github/workflows/pro-*.yml (3 files) - Fixed output encoding - .github/workflows/cleanup-full-ci-label.yml - New: Auto-remove label on merge - .github/workflows/pr-welcome-message.yml - New: Welcome message for PRs - CONTRIBUTING.md - Added CI Control Commands documentation The full-ci label now persists correctly across commits and provides clear user guidance through automated messages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
432-432: Add language identifiers to fenced code blocks.Fenced code blocks at lines 432 and 454 should specify the code language for better rendering and syntax highlighting.
Apply this diff to add language specifiers:
- /run-skipped-ci + ```bash + /run-skipped-ci + ```and
- /stop-run-skipped-ci + ```bash + /stop-run-skipped-ci + ```Also applies to: 454-454
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cleanup-full-ci-label.yml(1 hunks).github/workflows/examples.yml(2 hunks).github/workflows/main.yml(3 hunks).github/workflows/pr-welcome-message.yml(1 hunks).github/workflows/pro-integration-tests.yml(1 hunks).github/workflows/pro-lint.yml(1 hunks).github/workflows/pro-package-tests.yml(1 hunks).github/workflows/stop-run-skipped-ci.yml(1 hunks)CONTRIBUTING.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pro-integration-tests.yml
- .github/workflows/pro-lint.yml
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
432-432: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
454-454: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: markdown-link-check
🔇 Additional comments (10)
.github/workflows/cleanup-full-ci-label.yml (1)
1-35: Clean cleanup workflow implementation.The trigger condition, permissions, and error handling are appropriate for label cleanup on merge. The 404 gracefully handles cases where the label isn't present, and non-failure on other errors prevents blocking PR merges for cleanup issues.
CONTRIBUTING.md (2)
424-472: Verify no duplication of CI Control Commands section.The AI summary indicates this documentation block may be duplicated. Please confirm whether the "CI Control Commands" section appears only once in the file to avoid user confusion.
478-478: Example path updated appropriately.The Install Generator gem path example now uses a concrete relative path, which is clearer than the placeholder.
.github/workflows/pr-welcome-message.yml (1)
1-45: Welcome message workflow is well-structured.The message clearly explains CI behavior, documents the new commands, and sets proper user expectations. The reference to automatic cleanup aligns with the cleanup-full-ci-label.yml workflow.
.github/workflows/main.yml (2)
39-58: Label detection step is consistent across workflows.The check-label step correctly guards against non-PR contexts, fetches labels, and returns a string boolean. This implementation is consistent with the same pattern in pro-package-tests.yml and examples.yml.
94-97: Matrix exclusion logic is complex but functional.The multi-condition check for excluding minimum dependency variants is correct, though the ternary expression is quite lengthy. This pattern appears consistently across multiple jobs and workflows, which aids maintainability through consistency.
.github/workflows/stop-run-skipped-ci.yml (2)
22-56: Access control implementation is appropriate.The permission check verifies the user has 'admin' or 'write' access before allowing label removal. The exit early pattern (lines 52-56) prevents subsequent steps from running, which is the correct approach.
64-125: ✅ YAML syntax verified and code changes approved.The YAML validation confirms that the workflow file syntax is correct. The error handling implementation properly distinguishes between 404 (label already removed) and other errors, with appropriate user-friendly messaging for each case.
.github/workflows/pro-package-tests.yml (1)
27-63: Consistent label-based CI control implementation.The check-label step and detect-changes logic mirror the implementation in main.yml and examples.yml, maintaining consistency across workflows. The condition correctly activates full CI when either force_run is true or the full-ci label is present.
.github/workflows/examples.yml (1)
32-95: Label-based CI control consistently applied across generator tests.The check-label step and matrix exclusion logic are consistent with main.yml and pro-package-tests.yml, ensuring uniform CI behavior. The minimum dependency exclusion correctly respects the full-ci label.
Code Review - PR #2007: Make /run-skipped-ci persistent using full-ci labelSummaryThis PR enhances the CI control system by making the ✅ Strengths1. Excellent Architecture
2. User Experience
3. Robust Implementation
🔍 Detailed AnalysisCode Quality: Excellent
Best Practices: Well Applied
Security: Good
🤔 Considerations & Suggestions1. Code Duplication (Minor)The label-checking logic is duplicated across 5 workflows. While this is acceptable for maintainability, consider: Recommendation: Keep current approach unless duplication becomes a maintenance burden. The current approach is clearer and more maintainable for this scale. 2. Edge Case: Label Race Condition (Low Priority)If a user runs Impact: Minimal - workflows will run once, label will be removed 3. Welcome Message EnhancementThe welcome message posts on every new PR, even for contributors familiar with the system. Recommendation: Consider as future enhancement if welcome messages become noise for regular contributors 🧪 Test CoverageManual Testing Recommended:
Edge Cases to Test:
🚀 Performance ConsiderationsImpact: Minimal
📝 DocumentationExcellent documentation:
Minor suggestion: 🔐 Security ReviewNo security concerns identified:
🎯 RecommendationsPriority 1 (Before Merge):
Priority 2 (Nice to Have):
Priority 3 (Future Enhancements):
✅ Final VerdictLGTM - This is a well-designed, well-implemented solution that solves a real problem. Code Quality: ⭐⭐⭐⭐⭐ (5/5) The PR is ready to merge. All suggestions are optional enhancements, not blockers. 📚 Additional NotesWorkflow Logic Correctness: Matrix Exclusion Logic:
Great work! 🎉 |
- Remove cleanup workflow: Preserve full-ci label on merged PRs as historical record - Reduce code duplication: Create reusable composite action for label checking - Fix label addition timing: Add label within main workflow step with better error handling - Improve UX: Add permissions note to welcome message and clarify label persistence - Better error handling: Use core.setFailed() instead of throw in stop workflow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2007: Make /run-skipped-ci persistent using full-ci labelThank you for this well-structured PR! The implementation of persistent full CI mode using labels is a solid solution to the problem. Here's my detailed feedback: ✅ StrengthsArchitecture & Design
Implementation Quality
🔍 Issues & Concerns1. CRITICAL: Missing Label Cleanup on Merge
|
Code ReviewOverall AssessmentThis is a well-designed improvement to the CI workflow that solves a real problem. The implementation is solid, but I've identified several areas for improvement. ✅ Strengths
🐛 Potential Issues1. Label Persistence on Merge (Medium Priority)Issue: The PR description states the label should persist as a "historical record" after merge, but there's no workflow to clean it up. This could cause issues if PRs are reopened or labels accumulate. Recommendation: Add a cleanup workflow that removes the 2. Error Handling Inconsistency (Low Priority)File: The workflow uses 3. Race Condition in Label Addition (Low Priority)File: If 📝 Code Quality Suggestions
🔒 Security Considerations✅ Good: Both workflows properly check for write access 🧪 Test CoverageMissing: The PR mentions a test plan but doesn't include automated tests. Consider manual verification checklist at minimum. 📊 Performance Considerations✅ Good: The label check is efficient (single API call) 🎯 Recommendations PriorityHigh Priority (Fix before merge):
Medium Priority (Consider for this PR):
Low Priority (Future improvements):
✨ Final VerdictApprove with minor fixes required. This PR significantly improves the developer experience by making full CI mode persistent. The implementation is sound, but needs the small fixes noted above before merging. Great work on the comprehensive documentation updates and consistent implementation across all workflows! 🎉 Review generated by Claude Code |
Summary
/run-skipped-cinow adds a persistentfull-cilabel to the PR/stop-run-skipped-cicommand removes the label to disable full CI modefull-cilabel in addition toforce_runinputProblem
Previously, when running
/run-skipped-ci:Solution
The
full-cilabel acts as a persistent flag:/run-skipped-cicommanddetect-changesjobs/stop-run-skipped-cicommandWorkflows Updated
main.yml- Main integration testsexamples.yml- Generator testspro-integration-tests.yml- Pro integration testspro-package-tests.yml- Pro package testspro-lint.yml- Pro lintingAll workflows now:
full-cilabel on PRsUsage
Test Plan
/run-skipped-ciadds the label and triggers workflows/stop-run-skipped-ciremoves the label🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Chores
Documentation
New Features