Add CI debugging tools for efficient local failure reproduction #1973
Conversation
Adds two powerful scripts to replicate CI failures locally: 1. bin/ci-rerun-failures: - Fetches actual CI failures from GitHub using gh CLI - Runs only failed jobs locally (no wasted time on passing tests) - Waits for in-progress CI or searches previous commits - Maps CI job names to local rake commands automatically - Supports --previous flag and specific PR numbers 2. bin/ci-run-failed-specs: - Runs only specific failing RSpec examples - Parses RSpec output from CI to extract spec paths - Auto-detects spec/dummy directory when needed - Deduplicates spec paths - Accepts input from clipboard, arguments, or files These tools eliminate the wait-for-CI-feedback loop by running exactly what failed, exactly how CI runs it. Documentation added to CLAUDE.md with usage examples.
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 20 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 (4)
WalkthroughThis PR adds development tools and documentation for replicating and debugging CI failures locally. It introduces three new Bash scripts for managing CI configurations, re-running failed CI jobs, and executing failed RSpec tests. Documentation guides developers through common CI debugging workflows and system testing issues. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
Adds additional CI debugging tools:
1. bin/ci-switch-config:
- Switch between latest (Ruby 3.4, Node 22, Shakapacker 9.3.0, React 19)
and minimum (Ruby 3.2, Node 20, Shakapacker 8.2.0, React 18) configs
- Check current configuration status
- Helps debug failures specific to minimum dependency versions
2. SWITCHING_CI_CONFIGS.md:
- Complete guide to using ci-switch-config
- Troubleshooting common issues
- Understanding CI configuration matrix
3. spec/dummy/TESTING_LOCALLY.md:
- Local testing tips and workflows
- Known issues (e.g., Ruby 3.4.3 + OpenSSL 3.6 SSL errors)
- When to rely on CI as source of truth
Updated CLAUDE.md to reference these new resources.
Pull Request Review: CI Debugging ToolsI've reviewed this PR and overall it's an excellent addition that will significantly improve the developer experience. The tools are well-designed, thoroughly documented, and follow good bash scripting practices. ✅ StrengthsCode Quality
Documentation
Functionality
🔍 Potential Issues & Suggestions1. Security: Command Injection Risk (Medium Priority)Location: bin/ci-rerun-failures:242, bin/ci-run-failed-specs:148 Both scripts use eval with user-controlled input. While relatively controlled, eval can be risky. Recommendation: Consider safer alternatives like direct command execution for rspec specs. 2. Portability: macOS-specific Command (Low Priority)Multiple examples use pbpaste which only exists on macOS. Linux users would need xclip or xsel. Recommendation: Add a note about cross-platform alternatives. 3. Error Handling: Missing Dependency Checks (Low Priority)Scripts depend on external tools (gh, jq, yarn, bundle) but don't verify they're installed upfront. 4. Feature Gap: No Dry-Run ModeConsider adding --dry-run support, especially for ci-switch-config. 🧪 Test CoverageRecommendation: Add shellcheck linting to CI workflow. 🎯 Overall AssessmentVerdict: ✅ Approve with minor suggestions This is a high-quality PR that:
The issues identified are mostly minor and don't block merging. RecommendationMerge after considering:
🙏 Great Work!This tooling will significantly speed up the development workflow. The level of documentation and attention to user experience is exemplary. Reviewed by: Claude Code |
![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
🧹 Nitpick comments (2)
bin/ci-run-failed-specs (1)
125-148: Consider using array for command arguments instead of string concatenation.The current approach builds a command string with single-quoted arguments and uses
eval, which could fail if spec paths contain single quotes (though rare in practice). A more robust approach uses Bash arrays:-# Build rspec command -RSPEC_CMD="bundle exec rspec" -for spec in "${UNIQUE_SPECS[@]}"; do - RSPEC_CMD="$RSPEC_CMD '$spec'" -done +# Build rspec command as array +RSPEC_CMD=("bundle" "exec" "rspec") +for spec in "${UNIQUE_SPECS[@]}"; do + RSPEC_CMD+=("$spec") +done -echo -e "${BLUE}Command:${NC} cd $WORKING_DIR && $RSPEC_CMD" +echo -e "${BLUE}Command:${NC} cd $WORKING_DIR && bundle exec rspec ${UNIQUE_SPECS[*]}" echo "" # Confirm (read from /dev/tty to handle piped input) if [ -t 0 ]; then read -p "Run these specs now? [Y/n] " -n 1 -r else read -p "Run these specs now? [Y/n] " -n 1 -r < /dev/tty fi echo if [[ ! $REPLY =~ ^[Yy]$ ]] && [[ ! -z $REPLY ]]; then echo "Cancelled." exit 0 fi # Run the specs cd "$WORKING_DIR" -eval "$RSPEC_CMD" +"${RSPEC_CMD[@]}" RESULT=$?This avoids
evaland handles special characters in paths more safely.bin/ci-rerun-failures (1)
188-190: Consider using a cleaner approach for checking empty arrays.The
set +u/set -uworkaround is functional but can be replaced with a more idiomatic approach:-# Check if any commands were found (handle empty array with set -u) -set +u -NUM_COMMANDS=${#COMMANDS_TO_RUN[@]} -set -u +# Check if any commands were found +NUM_COMMANDS="${#COMMANDS_TO_RUN[@]:-0}"Or alternatively, check directly in the conditional:
-# Check if any commands were found (handle empty array with set -u) -set +u -NUM_COMMANDS=${#COMMANDS_TO_RUN[@]} -set -u - -if [ "$NUM_COMMANDS" -eq 0 ]; then +if [ "${#COMMANDS_TO_RUN[@]:-0}" -eq 0 ]; thenBoth approaches handle empty arrays gracefully without toggling shell options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(1 hunks)SWITCHING_CI_CONFIGS.md(1 hunks)bin/ci-rerun-failures(1 hunks)bin/ci-run-failed-specs(1 hunks)bin/ci-switch-config(1 hunks)spec/dummy/TESTING_LOCALLY.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/dummy/TESTING_LOCALLY.md
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/dummy/TESTING_LOCALLY.md
🪛 LanguageTool
SWITCHING_CI_CONFIGS.md
[grammar] ~246-~246: There seems to be a noun/verb agreement error. Did you mean “installs” or “installed”?
Context: ...m ruby asdf reshim nodejs ``` ### Yarn install fails If you get package resolution er...
(SINGULAR_NOUN_VERB_AGREEMENT)
⏰ 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). (7)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (5)
spec/dummy/TESTING_LOCALLY.md (1)
1-73: Excellent documentation for local testing guidance.This documentation provides clear, actionable guidance for developers encountering environment-specific SSL issues. The structure is logical, moving from problem identification to workarounds and solutions. The recommendation to use CI as the source of truth for system tests is pragmatic.
SWITCHING_CI_CONFIGS.md (1)
1-299: Comprehensive guide for switching CI configurations.This documentation provides excellent coverage of the CI configuration switching workflow, including prerequisites, detailed steps, troubleshooting, and integration with other tools. The examples are clear and the troubleshooting section addresses common issues developers might encounter.
bin/ci-rerun-failures (1)
1-269: Well-structured CI failure reproduction script.This script provides excellent automation for reproducing CI failures locally. The logic for detecting failures, mapping to local commands, and executing them is sound. The user experience is thoughtful with confirmations, progress indicators, and helpful tips.
bin/ci-switch-config (1)
37-285: Well-designed configuration switching implementation.The script provides a robust mechanism for switching between CI configurations. The version manager detection, dependency management, and user guidance are all well-implemented. The functions are clearly separated and the overall flow is logical.
CLAUDE.md (1)
51-117: Excellent integration of CI debugging tools into the development guide.The new section provides clear, actionable documentation for the CI debugging workflow. The structure progresses logically from configuration switching to failure detection to targeted test execution. The feature lists with emojis make it easy to scan capabilities, and the cross-references guide users to more detailed documentation.
Changes set -e to set -euo pipefail for more robust error handling:
- -u: Treat unset variables as errors
- -o pipefail: Fail if any command in a pipeline fails
Updates variable handling to avoid -u failures:
- Initialize SHAKAPACKER_GEM, SHAKAPACKER_NPM, REACT_ROOT, REACT_DUMMY
with empty strings before conditional assignment
- Add || echo "" to grep/sed commands to prevent pipeline failures
- Use ${VAR:-default} syntax for safe variable expansion
- Explicitly name REPLY variable in read commands
This matches the error handling strictness of other scripts in the PR
(bin/ci-rerun-failures, bin/ci-run-failed-specs).
Code Review: CI Debugging ToolsI've reviewed this PR and it's an excellent addition to the developer tooling! The scripts are well-designed and will significantly improve the CI debugging workflow. Here's my detailed feedback: ✅ Strengths1. Excellent Code Quality
2. Smart Design Decisions
3. Great Documentation
🔍 Potential Issues & SuggestionsSecurity Concerns1. Command Injection Risk (bin/ci-rerun-failures:242)if eval "$cmd"; thenIssue: Using Recommendation: Consider using functions or case statements instead of eval: case "$cmd" in
"lint-js-and-ruby")
bundle exec rubocop && yarn run eslint && yarn start format.listDifferent
;;
# ... other cases
esac2. Command Injection Risk (bin/ci-run-failed-specs:148)eval "$RSPEC_CMD"Issue: User-provided spec paths are directly interpolated into the command. Malicious input like Recommendation: Pass specs as array arguments instead: cd "$WORKING_DIR"
bundle exec rspec "${UNIQUE_SPECS[@]}"Bug: Array Access Without Bounds Check (bin/ci-run-failed-specs:118)if [[ "${UNIQUE_SPECS[0]}" == *"spec/system"* ]] || [[ "${UNIQUE_SPECS[0]}" == *"spec/helpers"* ]]; thenIssue: Accessing array index without checking if array is non-empty first (despite check at line 97, defensive coding suggests rechecking). Recommendation: Add bounds check for safety. Reliability Issues3. Hardcoded Ruby Command (bin/ci-switch-config:134)ruby script/convertIssue: Executes Recommendation: Document that current Ruby must be compatible with 4. Git Restore Error Handling (bin/ci-switch-config:214)git restore Gemfile.development_dependencies package.json spec/dummy/package.json packages/react-on-rails-pro/package.json 2>/dev/null || trueIssue: Silently fails if files don't exist or aren't tracked. User won't know if restoration failed. Recommendation: Check exit status and warn user if restoration fails. Usability Improvements5. Missing jq Dependency CheckAll three scripts use Recommendation: Add early check with helpful installation instructions. 6. Missing gh Dependency Check
Recommendation: Add helpful error message if 🎯 Performance ConsiderationsGood
Minor Optimization
📝 Documentation QualityExcellent
Suggestions
🧪 Test CoverageMissing: No automated tests for these bash scripts. Recommendation: Consider adding:
🔐 Security AssessmentOverall: Low risk for current use case (developer tooling), but be aware:
📊 Overall AssessmentQuality Score: 8.5/10 This is high-quality developer tooling that will genuinely improve productivity. The code is clean, well-structured, and thoughtfully designed. The security issues I've identified are low-risk in the current context but worth addressing before these patterns are copied elsewhere. Recommendations for MergeBefore merging (high priority):
Nice to have (can be follow-up PRs):
🎉 ConclusionExcellent work! This PR adds genuinely useful tools that follow bash best practices. The code is clean, the UX is thoughtful, and the documentation is outstanding. The security issues are low-risk for developer tooling but should be addressed to prevent these patterns from spreading. Once those fixes are in, this will be a valuable addition to the project. Special kudos for:
|
1. Security: Remove eval from bin/ci-run-failed-specs
- Use bash array instead of string for RSPEC_CMD
- Execute command directly with "${RSPEC_CMD[@]}" (safer)
- Eliminates command injection risk from spec paths
2. Security: Document safe eval usage in bin/ci-rerun-failures
- Add comment explaining eval is safe (commands from predefined JOB_MAP)
- Fix REPLY variable to use explicit naming
3. Error handling: Add dependency checks
- bin/ci-rerun-failures: Check for gh and jq
- bin/ci-run-failed-specs: Check for bundle
- Provide clear error messages with install instructions
4. Portability: Document cross-platform clipboard alternatives
- Add Linux alternatives (xclip, wl-clipboard) in CLAUDE.md
- Maintain pbpaste as primary example for macOS
5. Consistency: Fix REPLY variable handling
- Explicitly name REPLY in all read commands
- Use ${REPLY} with quotes for -u compatibility
All scripts now have consistent error handling, better security,
and clearer documentation for cross-platform usage.
Code Review: PR #1973 - CI Debugging ToolsOverviewExcellent addition! These tools address a real pain point in the development workflow by eliminating the wait-for-CI feedback loop. The implementation is well-thought-out and provides significant value. ✅ Strengths1. Code Quality
2. Security Considerations
3. Robustness
4. Documentation
🔍 Minor Observations1. Bash Array Handling (
|
| Aspect | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐⭐ | Clean, well-structured bash |
| Security | ⭐⭐⭐⭐⭐ | Proper input handling |
| Documentation | ⭐⭐⭐⭐⭐ | Comprehensive and clear |
| User Experience | ⭐⭐⭐⭐⭐ | Intuitive with helpful prompts |
| Error Handling | ⭐⭐⭐⭐⭐ | Robust with fallbacks |
| Maintainability | ⭐⭐⭐⭐☆ | Could benefit from JOB_MAP docs |
✅ Recommendation
APPROVE - This is production-ready code that provides significant value to the development workflow. The minor observations above are suggestions for future improvements, not blockers.
Key Wins:
- Eliminates CI wait times for debugging
- Surgical test execution saves time
- Excellent error handling and UX
- Comprehensive documentation
- Secure implementation
Next Steps:
- Consider adding shellcheck to CI for bash scripts
- Document JOB_MAP maintenance in CONTRIBUTING.md
- Monitor usage and gather feedback for future enhancements
Great work! 🚀
Review conducted following CLAUDE.md guidelines with focus on code quality, security, performance, and best practices.
Problem
Debugging CI failures currently requires:
This is slow and inefficient.
Solution
Adds two powerful scripts that automate CI failure reproduction:
1.
bin/ci-rerun-failuresAutomatically detects and re-runs only failed CI jobs:
Features:
ghCLI2.
bin/ci-run-failed-specsRuns only specific failing RSpec examples:
Features:
Benefits
Documentation
Added comprehensive usage guide to CLAUDE.md with examples.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is
Summary by CodeRabbit
Documentation
Chores