Add service dependency checking to bin/dev #2098
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 9 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 (6)
WalkthroughThis PR introduces service dependency checking for the bin/dev startup process. A new ServiceChecker utility validates that external services (Redis, PostgreSQL, Elasticsearch, etc.) are running before the development server starts. Configuration is read from Changes
Sequence DiagramsequenceDiagram
participant dev as bin/dev
participant sm as ServerManager
participant sc as ServiceChecker
participant fs as Filesystem
participant sh as Shell
dev->>sm: start server
sm->>sc: check_services(".dev-services.yml")
sc->>fs: load .dev-services.yml
fs-->>sc: config (or nil)
alt config exists and has services
loop for each service
sc->>sh: run check_command
sh-->>sc: stdout + exit status
sc->>sc: verify expected_output
sc->>sc: record result (pass/fail)
end
alt all services pass
sc->>sc: print_all_services_ok()
sc-->>sm: true
else some services fail
sc->>sc: print_failures_summary()
sc->>sc: show start_command & install_hint
sc-->>sm: false
end
else no config or no services
sc-->>sm: true (skip checks)
end
alt check_services returned false
sm->>dev: exit 1
else check_services returned true
sm->>dev: continue startup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 Review - PR #2098: Add service dependency checking to bin/devOverviewThis is an excellent addition that significantly improves the developer experience. The implementation is well-thought-out, follows the project's conventions, and provides real value without breaking existing functionality. ✅ Strengths1. Excellent Design Decisions
2. Code Quality
3. Test CoverageThe test suite is comprehensive (11 tests) covering:
4. Documentation
🔍 Potential Issues & Suggestions1. Security: Command Injection Risk
|
Code Review - PR #2098: Service Dependency CheckingHi @justin808! 👋 I've reviewed this PR and it's a well-designed feature with strong implementation. Here's my detailed feedback: ✅ Strengths1. Architecture & Design
2. Code Quality
3. User Experience
🔍 Issues & SuggestionsCritical Issues1. Command Injection Vulnerability
|
Code Review for PR #2098Excellent work! This PR introduces a well-designed, optional feature. I found a few security and edge case considerations: Security Concerns1. Shell Command Injection Risk (MEDIUM) - service_checker.rb:105 The command is passed directly to the shell. Use Shellwords.split to prevent injection: def run_check_command(command)
require "open3"
require "shellwords"
command_args = Shellwords.split(command)
stdout, stderr, status = Open3.capture3(*command_args, err: %i[child out])
[stdout + stderr, status]
rescue StandardError
["", nil]
endThis matches the pattern in server_manager.rb:234. 2. YAML Loading Security (LOW) - service_checker.rb:80 Use YAML.safe_load_file instead of YAML.load_file. Other Observations
RecommendationAPPROVE with minor suggestions. Must fix before merge: Use Shellwords.split for shell injection prevention. Other items can be follow-up PRs. Great work on test coverage and UX! |
Code ReviewThis is a well-designed feature that adds service dependency checking to ✅ StrengthsArchitecture & Design
Code Quality
Documentation
🔍 Issues & Recommendations1. CRITICAL: Missing RBS Type SignaturePer
Required actions: # 1. Create sig/react_on_rails/dev/service_checker.rbs
# 2. Add to Steepfile after line 34:
check "lib/react_on_rails/dev/service_checker.rb"
# 3. Validate signatures
bundle exec rake rbs:validateSuggested RBS signature: # sig/react_on_rails/dev/service_checker.rbs
module ReactOnRails
module Dev
class ServiceChecker
def self.check_services: (?config_path: String) -> bool
private
def self.config_has_services?: (Hash[String, untyped]?) -> bool
def self.check_and_report_services: (Hash[String, untyped], String) -> bool
def self.collect_service_failures: (Hash[String, untyped]) -> Array[Hash[Symbol, untyped]]
def self.report_results: (Array[Hash[Symbol, untyped]]) -> bool
def self.load_config: (String) -> Hash[String, untyped]?
def self.check_service: (String, Hash[String, untyped]) -> bool
def self.run_check_command: (String) -> [String, Process::Status?]
def self.print_services_header: (String) -> void
def self.print_service_ok: (String, String?) -> void
def self.print_service_failed: (String, String?) -> void
def self.print_all_services_ok: () -> void
def self.print_failures_summary: (Array[Hash[Symbol, untyped]]) -> void
end
end
end2. Missing Changelog EntryThis is a user-visible feature and should be documented in Suggested entry under #### Added
- **Service Dependency Checking for bin/dev**: Added optional `.dev-services.yml` configuration to validate required external services (Redis, PostgreSQL, Elasticsearch, etc.) are running before `bin/dev` starts the development server. Provides clear error messages with start commands and install hints when services are missing. Zero impact if not configured - backwards compatible with all existing installations. [PR 2098](https://github.com/shakacode/react_on_rails/pull/2098) by [justin808](https://github.com/justin808)3. Potential Security Concern: Command InjectionThe services:
evil:
check_command: "rm -rf / &"Mitigation: This is a local config file (not user input), so risk is low. However, consider adding a warning in the template: # SECURITY WARNING: Commands in this file are executed during bin/dev startup.
# Only add commands from trusted sources. This file should not be committed
# if it contains sensitive information.4. Minor: Test Coverage GapThe tests mock
Suggested additional test: context "when check_command outputs to stderr" do
it "captures stderr in output" do
config = {
"services" => {
"test_service" => {
"check_command" => "echo 'ERROR' >&2",
"expected_output" => "ERROR"
}
}
}
with_temp_config(config) do |path|
expect(described_class.check_services(config_path: path)).to be true
end
end
end5. Minor: Hardcoded Config FilenameThe integration in config_path = ENV["DEV_SERVICES_CONFIG"] || ".dev-services.yml"
exit 1 unless ServiceChecker.check_services(config_path: config_path)This would allow advanced users to have environment-specific configs (though this is probably overkill for v1). 6. Documentation: .gitignore RecommendationThe template mentions:
Consider updating the generator to add a comment to # lib/generators/react_on_rails/base_generator.rb
append_to_file ".gitignore", "\n# Uncomment if your .dev-services.yml contains secrets:\n# .dev-services.yml\n"📋 Action ItemsBefore merge:
Testing recommendations: # Verify RBS signatures
bundle exec rake rbs:validate
# Run tests
rake run_rspec:gem
# Test manually with dummy app
cd spec/dummy
cp .dev-services.yml.example .dev-services.yml
# Edit to add a failing service
bin/dev
# Should show helpful error messages🎯 Overall AssessmentRating: Excellent (with required RBS additions) This is a thoughtful, well-implemented feature that solves a real problem. The code is clean, well-tested, and the UX is excellent. The only critical issue is the missing RBS signature, which is required per project conventions. Once that's added, this is ready to merge. Great work! 🚀 Review performed by Claude Code following React on Rails conventions from |
Code Review FeedbackThis is a well-implemented feature with excellent test coverage and documentation. Here's my detailed review: ✅ Strengths1. Excellent Code Quality
2. Great User Experience
3. Strong Documentation
4. Robust Error Handling
🔍 Issues & Suggestions🚨 CRITICAL: Security Vulnerability - Command InjectionLocation: def run_check_command(command)
require "open3"
stdout, stderr, status = Open3.capture3(command, err: %i[child out])Problem: This code passes the command directly to a shell, enabling command injection if user config contains malicious commands: services:
malicious:
check_command: "echo harmless; rm -rf /important/data"Impact: High severity - arbitrary command execution during Fix: Use array syntax to avoid shell interpolation: def run_check_command(command)
require "open3"
# Split command into array to avoid shell injection
# Still allows legitimate commands like "redis-cli ping"
cmd_array = command.shellsplit
stdout, stderr, status = Open3.capture3(*cmd_array, err: %i[child out])
rescue ArgumentError => e
# shellsplit may raise on malformed input
["", nil]
endNote: The security warning in the YAML is good, but doesn't prevent injection. Defense in depth requires both warnings AND safe execution.
|
Introduces a new service checking system that validates required external
services (like Redis, PostgreSQL, Elasticsearch) are running before starting
the development server.
Key Features:
- Declarative YAML configuration (.dev-services.yml)
- Cross-platform support (macOS and Linux)
- Helpful error messages with start commands
- Zero impact when no config file exists
- Suggests removing services from Procfile
Implementation:
- New ReactOnRails::Dev::ServiceChecker module
- Integrated into ServerManager before Procfile execution
- Checks services in both HMR and static development modes
- Comprehensive test coverage (11 tests)
Configuration Format:
services:
redis:
check_command: "redis-cli ping"
expected_output: "PONG"
start_command: "redis-server"
install_hint: "brew install redis"
description: "Redis (for caching)"
User Experience:
- When services pass: Silent success, Procfile starts normally
- When services fail: Clear error with start instructions and tips
Generator Changes:
- .dev-services.yml.example now included in new installations
- Comprehensive documentation and common examples included
Benefits:
- Self-documenting service requirements
- Reduces confusion when services are missing
- Encourages removing services from Procfile (cleaner separation)
- Better onboarding for new developers
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2098: Add service dependency checking to bin/devOverall AssessmentThis is a well-implemented feature that adds significant value to the developer experience. The code is clean, well-tested, and follows best practices. I have some suggestions for improvements around security, error handling, and documentation. ✅ Strengths
🔒 Security Concerns (High Priority)1. Command Injection RiskThe Current warning: # ⚠️ SECURITY WARNING:
# Commands in this file are executed during bin/dev startup.Recommendation: # ⚠️ SECURITY WARNING:
# Commands in this file are executed with shell access during bin/dev startup.
# NEVER add this file to version control if it contains sensitive commands.
# Only add commands from trusted sources - malicious commands could compromise your system.
# Consider adding .dev-services.yml to .gitignore if it contains machine-specific paths.2. Consider Adding Command ValidationWhile this is developer-controlled config, consider validating that commands don't contain suspicious patterns: def validate_command(command)
# Warn on potentially dangerous patterns
dangerous_patterns = ['; rm', '&& rm', '| sh', 'eval', 'exec']
dangerous_patterns.each do |pattern|
if command.include?(pattern)
puts Rainbow("⚠️ Warning: Command contains potentially dangerous pattern: #{pattern}").yellow
puts Rainbow(" Command: #{command}").yellow
end
end
end🐛 Potential Bugs1. Missing
|
- Add RBS type signature for ServiceChecker at sig/react_on_rails/dev/service_checker.rbs - Add ServiceChecker to Steepfile for type checking - Validate RBS signatures pass - Add changelog entry under Unreleased section - Add security warnings to .dev-services.yml.example templates - Add 3 new tests for stderr capture (14 tests total, all passing) Addresses feedback from PR review: - RBS type signatures required per CLAUDE.md - Security concerns about command execution - Test coverage for stderr output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
jg-/bin-dev-service-deps
branch
from
ecb2b31 to
df8e2ee
Compare
Code Review for PR #2098: Service Dependency CheckingI've completed a thorough review of this PR. Overall, this is excellent work with well-thought-out design, comprehensive testing, and good documentation. ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/react_on_rails/dev/service_checker_spec.rb (1)
31-41: Specs thoroughly exercise ServiceChecker; make helpers more self-containedThe scenarios here give solid coverage of the public behavior, including invalid YAML, mixed stdout/stderr, missing
check_command, and multiple services. Nice job.Two minor nits:
capture_stdoutrelies onStringIObeing available from elsewhere. Adding an explicitrequire "stringio"(either here or in the shared spec helper) would make this file self-contained.- In the “invalid YAML” context you invoke
check_servicestwice (once for the output expectation and once for thebe trueexpectation). This is functionally fine but a bit redundant; you could instead capture the return value inside the output expectation block and assert on it there.These are small cleanups and not blockers.
Also applies to: 188-255
lib/react_on_rails/dev/server_manager.rb (1)
7-7: ServiceChecker integration into bin/dev static and HMR modes looks correctWiring
ServiceChecker.check_servicesinto bothrun_static_developmentandrun_development(with an immediateexit 1on failure) cleanly enforces “don’t start dev processes when required services are down” while remaining a no-op when.dev-services.ymlis missing or empty. Requiringservice_checkerat the top of this file and using the bareServiceCheckerconstant inside theReactOnRails::Devnamespace is also appropriate.If you want behavior parity across all modes, you could optionally run the same service check at the start of
run_production_likeso “prod”/“production-assets” also refuse to start when dependencies are unavailable, but that’s purely a UX consistency tweak.Also applies to: 515-542, 544-553
lib/react_on_rails/dev/service_checker.rb (1)
31-77: ServiceChecker core flow is solid; consider tightening config validationThe
check_servicespipeline (load_config→config_has_services?→check_and_report_services) behaves well:
- Missing/invalid config or no services: no-op with optional warning.
- Well-formed
servicesmap: per-service checks with clear success/failure output and a concise boolean result.- Command handling (
run_check_command) and expected_output matching are defensive (graceful on exceptions, stderr merged into output).One small robustness improvement you might consider:
- In
config_has_services?/collect_service_failures, assumeservicesis aHashexplicitly (e.g.,return false unless config["services"].is_a?(Hash)). Today a misconfiguredservices:that’s not a hash could raise at runtime when you callservices.eachorempty?.This isn’t a blocker, since typical configs will be hashes and invalid YAML is already handled defensively, but it would make error modes a bit friendlier.
lib/generators/react_on_rails/templates/base/base/.dev-services.yml.example (1)
29-29: Consider platform-agnostic PostgreSQL start command.The
pg_ctl -D /usr/local/var/postgres startcommand uses a macOS-specific Homebrew path. Linux users typically have PostgreSQL data directories at/var/lib/postgresql/dataor usepg_ctlclusteron Debian/Ubuntu systems. While the install hints mention both platforms, the start command only works on macOS.Consider one of these approaches:
Option 1: Use a more generic command that works across platforms:
- start_command: "pg_ctl -D /usr/local/var/postgres start" + start_command: "pg_ctl start (or: sudo service postgresql start on Linux)"Option 2: Add a comment noting platform differences:
start_command: "pg_ctl -D /usr/local/var/postgres start" + # Note: On Linux, use: sudo service postgresql startOption 3: Reference environment variable or config:
- start_command: "pg_ctl -D /usr/local/var/postgres start" + start_command: "pg_ctl start (configure PGDATA env var or use system service)"Also applies to: 66-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)Steepfile(1 hunks)lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/.dev-services.yml.example(1 hunks)lib/react_on_rails/dev/server_manager.rb(3 hunks)lib/react_on_rails/dev/service_checker.rb(1 hunks)sig/react_on_rails/dev/service_checker.rbs(1 hunks)spec/dummy/.dev-services.yml.example(1 hunks)spec/react_on_rails/dev/service_checker_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/base_generator.rbSteepfilelib/generators/react_on_rails/templates/base/base/.dev-services.yml.example
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/base_generator.rbSteepfilelib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
Steepfilelib/react_on_rails/dev/service_checker.rbspec/react_on_rails/dev/service_checker_spec.rbsig/react_on_rails/dev/service_checker.rbslib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
Steepfilelib/react_on_rails/dev/service_checker.rbsig/react_on_rails/dev/service_checker.rbslib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/dev/service_checker.rbsig/react_on_rails/dev/service_checker.rbs
📚 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/react_on_rails/dev/service_checker_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
sig/react_on_rails/dev/service_checker.rbs
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
sig/react_on_rails/dev/service_checker.rbslib/react_on_rails/dev/server_manager.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/dev/server_manager.rb
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/react_on_rails/dev/server_manager.rb
🧬 Code graph analysis (2)
spec/react_on_rails/dev/service_checker_spec.rb (1)
lib/react_on_rails/dev/service_checker.rb (2)
check_services(26-172)check_services(31-38)
lib/react_on_rails/dev/server_manager.rb (1)
lib/react_on_rails/dev/service_checker.rb (2)
check_services(26-172)check_services(31-38)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (8)
Steepfile (1)
26-36: Add service_checker.rb to Steep type-checking scopeIncluding
lib/react_on_rails/dev/service_checker.rbhere is correct and keeps the dev utilities consistently covered by Steep.spec/dummy/.dev-services.yml.example (1)
1-42: Example config matches ServiceChecker contract and clearly documents risksThe template structure and keys align with
ServiceChecker’s expectations, and the security warning plus guidance about copying to.dev-services.ymland using.gitignoreare good guardrails. No changes needed.lib/generators/react_on_rails/base_generator.rb (1)
42-52: Generator now scaffolds .dev-services.yml.exampleAdding
.dev-services.yml.exampletobase_filescleanly wires the new template into the install flow, and the path matches the existingbase_path. Safe to ship as an example file while keeping real.dev-services.ymluser-specific.CHANGELOG.md (1)
26-29: Changelog entry accurately describes the new bin/dev service checksThe new “Service Dependency Checking for bin/dev” bullet is clear, matches the implementation (optional
.dev-services.yml, external service validation, helpful errors, zero-impact when absent), and follows existing changelog conventions.lib/react_on_rails/dev/service_checker.rb (1)
79-110: Graceful handling of invalid YAML and failed commands
load_configandrun_check_commandare appropriately defensive: invalid YAML results in a clear warning and skipping checks, and any exception running the shell command yields["", nil], whichcheck_serviceinterprets as a failure. That matches the documented behavior (“print warnings and continue without blocking bin/dev”) and the specs.lib/generators/react_on_rails/templates/base/base/.dev-services.yml.example (2)
1-51: Excellent documentation and security awareness!The header documentation is thorough and well-structured. The security warning about command execution is particularly important and well-placed. The field descriptions and usage instructions provide clear guidance for developers.
56-68: Template structure is clean and ready for developer customization.The commented-out service examples provide a good starting point. The empty
services:block makes it clear that developers should uncomment only what they need.sig/react_on_rails/dev/service_checker.rbs (1)
1-22: Well-designed type signatures for service checking utility.The RBS signatures are clean and appropriate:
- Clear public API with a single entry point (
check_services)- Pragmatic use of
untypedfor Hash values given the dynamic nature of YAML configuration- Proper encapsulation with private helper methods
- Consistent return types (bool for checks, void for printing, tuples for command results)
- Method naming follows Ruby conventions (predicate methods ending in
?)The type signatures align well with the described functionality of loading YAML configs, executing shell commands, and reporting results.
Code ReviewThank you for this well-crafted PR! This is a thoughtful addition that addresses a real developer experience pain point. Overall, the implementation is solid, well-tested, and follows the project conventions. Here's my detailed feedback: ✅ Strengths1. Excellent Design Decisions
2. Strong Test Coverage
3. Code Quality
4. Documentation
🔍 Suggestions for Improvement1. Security: Command Injection Risk
|
- Updated bin/dev --help with SERVICE DEPENDENCIES section - Added comprehensive documentation in docs/building-features/process-managers.md - Includes configuration examples, field descriptions, and sample output - Added security note about command execution - Documents zero-impact behavior when .dev-services.yml doesn't exist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code improvements: - Extract configuration keys as CONFIG_KEYS constant for maintainability - Use constants throughout instead of magic strings - More specific exception handling (Errno::ENOENT vs StandardError) - Add debug logging for unexpected errors (enabled with DEBUG env var) Security improvements: - Enhanced security warnings in .dev-services.yml.example files - Added best practices section about avoiding shell metacharacters - Documented risks of complex shell scripts and chained commands - Emphasized keeping commands simple and focused All tests passing (14/14). Zero RuboCop offenses. Addresses feedback from PR review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Service Dependency Checking for bin/devI've reviewed this PR and it looks excellent! This is a well-designed, well-tested feature that adds real value to the developer experience. Here's my detailed feedback: ✅ Strengths1. Excellent Code Quality
2. Comprehensive Test Coverage
3. Great User Experience
4. Proper Integration
5. Security Awareness
🔍 Potential Improvements1. Command Injection Risk (Minor)In stdout, stderr, status = Open3.capture3(command, err: %i[child out])While the security warning in the config file is good, consider: Option A: Use shell=false with array form to prevent injection: # Instead of: check_command: "redis-cli ping"
# Use: check_command: ["redis-cli", "ping"]
stdout, stderr, status = Open3.capture3(*command, err: %i[child out])Option B: Add input validation to warn about potentially dangerous characters: def validate_command_safety(command)
dangerous_chars = /[;&|$`]/
return unless command.match?(dangerous_chars)
puts Rainbow("⚠️ Warning: check_command contains shell metacharacters").yellow
puts Rainbow(" This could be a security risk. Only use trusted commands.").yellow
endGiven this is a developer tool and the config file has warnings, this is low priority but worth considering. 2. Timeout for Long-Running Commands (Minor)Some commands might hang indefinitely. Consider adding a timeout: def run_check_command(command)
require 'timeout'
require 'open3'
Timeout.timeout(5) do # 5 second timeout
stdout, stderr, status = Open3.capture3(command, err: %i[child out])
output = stdout + stderr
[output, status]
end
rescue Timeout::Error
["", nil]
rescue StandardError
["", nil]
end3. RBS Type Signature Precision (Very Minor)In # Current
def self.load_config: (String) -> Hash[String, untyped]?
# More precise
def self.load_config: (String) -> { "services" => Hash[String, Hash[String, String]] }?This is very minor since RBS is still being adopted. 4. Documentation Enhancement (Optional)The documentation is excellent, but could mention:
Example addition to docs: #### Debugging Service Checks
If a service check fails unexpectedly:
1. Run the check command manually to see full output:
```bash
redis-cli ping
Docker ServicesFor services running in Docker: services:
redis-docker:
check_command: "docker exec my-redis redis-cli ping"
expected_output: "PONG"
start_command: "docker compose up -d redis"
description: "Redis (running in Docker)" |
Code Review - PR #2098: Service Dependency CheckingThank you for this well-designed feature! The implementation is clean, well-tested, and thoughtfully documented. Here's my detailed review: ✅ StrengthsExcellent Design
Code Quality
Documentation
🔒 Security Concerns (CRITICAL)1. Shell Injection VulnerabilityIssue: The current implementation passes commands directly to the shell: stdout, stderr, status = Open3.capture3(command, err: %i[child out])This executes services:
evil:
check_command: "echo 'PONG'; rm -rf /"This would execute both commands. Recommendation: Use array form to bypass shell interpretation: def run_check_command(command)
require "open3"
# Split command into array to avoid shell interpretation
# This is safer but less flexible - document the tradeoff
cmd_parts = Shellwords.split(command)
stdout, stderr, status = Open3.capture3(*cmd_parts, err: %i[child out])
output = stdout + stderr
[output, status]
rescue Errno::ENOENT
["", nil]
rescue ArgumentError => e
# Shellwords.split can raise ArgumentError on unbalanced quotes
warn "Invalid command format: #{e.message}" if ENV["DEBUG"]
["", nil]
rescue StandardError => e
warn "Unexpected error checking service: #{e.message}" if ENV["DEBUG"]
["", nil]
endAlternative: If shell features are needed (pipes, redirects), add explicit documentation that this file should be .gitignored and treat it like a script with appropriate warnings. 2. Path Traversal RiskIssue: def check_services(config_path: ".dev-services.yml")
return true unless File.exist?(config_path)
config = load_config(config_path)While currently only called with hardcoded values, this could be exploited if extended. Recommendation: Add path validation: def check_services(config_path: ".dev-services.yml")
# Ensure path is relative and doesn't contain traversal sequences
if config_path.include?("..") || File.absolute_path?(config_path)
warn "Invalid config path: #{config_path}"
return true
end
return true unless File.exist?(config_path)
# ...
end3. YAML DeserializationCurrent code: YAML.load_file(config_path)Recommendation: Use def load_config(config_path)
YAML.safe_load(File.read(config_path), permitted_classes: [], permitted_symbols: [], aliases: true)
rescue Psych::DisallowedClass => e
puts Rainbow("⚠️ #{config_path} contains unsafe YAML: #{e.message}").yellow
puts Rainbow(" Continuing without service checks...").yellow
puts ""
nil
rescue StandardError => e
puts Rainbow("⚠️ Failed to load #{config_path}: #{e.message}").yellow
puts Rainbow(" Continuing without service checks...").yellow
puts ""
nil
end🐛 Potential Bugs1. Silent Nil Hash Accessdef config_has_services?(config)
config && config[CONFIG_KEYS[:services]] && \!config[CONFIG_KEYS[:services]].empty?
endIf Recommendation: Add type checking: def config_has_services?(config)
config.is_a?(Hash) &&
config[CONFIG_KEYS[:services]].is_a?(Hash) &&
\!config[CONFIG_KEYS[:services]].empty?
end2. Race Condition in Service CheckingServices might stop between the check and Procfile startup. This is acceptable for development, but worth documenting. Recommendation: Add a note in the template: # Note: Service checks are performed once before startup. Services that stop
# after this check will not be detected. This is intended for development use only.🎯 Performance Considerations1. Sequential Service Checkingservices.each do |name, service_config|
if check_service(name, service_config)
# ...
end
endFor multiple services, this runs sequentially. Consider parallel execution for faster startup: require 'concurrent'
def collect_service_failures(services)
futures = services.map do |name, service_config|
Concurrent::Future.execute do
success = check_service(name, service_config)
{ name: name, config: service_config, success: success }
end
end
results = futures.map(&:value)
# Print results in order
failures = []
results.each do |result|
if result[:success]
print_service_ok(result[:name], result[:config][CONFIG_KEYS[:description]])
else
failures << { name: result[:name], config: result[:config] }
print_service_failed(result[:name], result[:config][CONFIG_KEYS[:description]])
end
end
failures
endNote: This adds a dependency. For simplicity, sequential is fine for development. Consider adding a TODO comment. 2. Timeout HandlingLong-running or hanging check commands will block startup. Recommendation: Add timeout support: def run_check_command(command, timeout: 5)
require "open3"
require "timeout"
Timeout.timeout(timeout) do
stdout, stderr, status = Open3.capture3(command, err: %i[child out])
output = stdout + stderr
[output, status]
end
rescue Timeout::Error
warn "Service check timed out after #{timeout}s: #{command}" if ENV["DEBUG"]
["", nil]
rescue Errno::ENOENT
["", nil]
rescue StandardError => e
warn "Unexpected error checking service: #{e.message}" if ENV["DEBUG"]
["", nil]
end📝 Minor Improvements1. Missing
|
| Category | Status | Notes |
|---|---|---|
| Code Quality | ✅ Excellent | Clean, well-structured |
| Security | Shell injection vulnerability needs fixing | |
| Performance | ✅ Good | Sequential is fine for dev; timeout recommended |
| Testing | ✅ Excellent | Comprehensive, could add security tests |
| Documentation | ✅ Excellent | Clear and thorough |
| Type Safety | ✅ Good | RBS included, could be more specific |
🔧 Action Items
Must Fix Before Merge:
- ✋ Fix shell injection vulnerability - use
Shellwords.splitor document security implications - ✋ Use
YAML.safe_loadinstead ofload_file
Recommended:
3. Add timeout support for check commands
4. Add path validation for config_path
5. Add type checking in config_has_services?
6. Add security-focused tests
7. Enhance RBS signatures with specific types
Nice to Have:
8. Parallel service checking (document as TODO if not implementing)
9. More detailed security documentation in template
Overall: This is a valuable feature with excellent implementation. The security issues are fixable and should be addressed before merge. Once the critical security items are resolved, this will be a great addition! 🎉
cc: @justin808
Code Review - PR #2098: Add Service Dependency CheckingI've reviewed this PR and found it to be well-implemented, well-tested, and ready to merge. This is a valuable feature that improves the developer experience. Below is my detailed analysis: ✅ Strengths1. Excellent Code Quality
2. Outstanding Test Coverage
3. Great User Experience
4. Security Awareness
5. Excellent Integration
📋 Minor Observations (Non-Blocking)1. Command Execution Security
|
Code Review - PR #2098: Service Dependency CheckingExcellent work on this feature! The implementation is well-designed, thoroughly tested, and follows the repository's best practices. Here's my detailed review: ✅ Strengths1. Code Quality & Architecture
2. Security Awareness
3. Test Coverage
4. User Experience
5. Documentation
🔍 Observations & Minor Suggestions1. Platform-Specific Commands (Low Priority)The PostgreSQL start_command: "pg_ctl -D /usr/local/var/postgres start"Suggestion: Consider adding a comment noting platform differences: start_command: "pg_ctl -D /usr/local/var/postgres start # macOS; Linux: sudo service postgresql start"This is already somewhat addressed by the Files:
2. Config Validation Robustness (Nice-to-Have)Current code assumes services: "not a hash"It would fail when calling Suggestion: Add explicit type checking in def config_has_services?(config)
config &&
config[CONFIG_KEYS[:services]].is_a?(Hash) &&
!config[CONFIG_KEYS[:services]].empty?
endThis is a minor edge case since invalid configs are rare, but would make error modes friendlier. File: lib/react_on_rails/dev/service_checker.rb:52-54 3. Test Helper Self-Containment (Nice-to-Have)The require "stringio"would make the test file completely self-contained. File: spec/react_on_rails/dev/service_checker_spec.rb:1-3 4. Consistency Across Modes (Optional Enhancement)Service checking currently runs in:
But NOT in:
Question: Should production-like mode also check services? It might be valuable for consistency, but could also be intentionally excluded if developers want to test prod builds without full service dependencies. File: lib/react_on_rails/dev/server_manager.rb:514-530 ✨ Best Practices Adherence✅ RBS signatures added and validated (per CLAUDE.md requirements) 🎯 Performance Considerations
🔒 Security Assessment✅ Security warnings are prominent and comprehensive Note: The security model is appropriate for a development tool. Users control the YAML file, so command execution is expected behavior. Warnings are sufficient. 📊 Test Quality AssessmentCoverage: Excellent - 14 tests covering:
Test Design:
🚀 RecommendationAPPROVE - This PR is ready to merge with only minor, non-blocking suggestions above. Rationale:
The minor suggestions above are enhancements for future consideration, not blockers. 🎉 SummaryThis is a high-quality contribution that adds real value for developers working with external service dependencies. The implementation demonstrates careful attention to:
Great work! 🚀 Generated via Claude Code review |
Improvements: - Add service checking to production-like mode for consistency All three modes (hmr, static, production-like) now check services - Add type validation for services config Explicitly check that services is a Hash to prevent runtime errors Handles edge case where user might write 'services: "invalid"' - Add StringIO require to test file Makes test file completely self-contained - Improve platform-specific command documentation Added inline comment showing Linux alternative for PostgreSQL start command Both .dev-services.yml.example files now show: # macOS; Linux: sudo service postgresql start All tests passing (14/14). Zero RuboCop offenses. Addresses final observations from PR review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2098: Service Dependency CheckingSummaryExcellent PR that adds a well-designed service dependency checking system for ✅ Strengths1. Excellent Design & User Experience
2. Code Quality
3. Documentation
🔍 Issues & SuggestionsSecurity Concerns (Medium Priority)
# lib/react_on_rails/dev/service_checker.rb:114-117
def run_check_command(command)
require "open3"
stdout, stderr, status = Open3.capture3(command, err: %i[child out])
# ...
endRisk: If a malicious Mitigation suggestions:
Code Quality (Low Priority)
# Line 99-111 (from diff)
def check_service(_name, config)
check_command = config[CONFIG_KEYS[:check_command]]
expected_output = config[CONFIG_KEYS[:expected_output]]
return false if check_command.nil?
output, status = run_check_command(check_command)
return false if status.nil?
return status.success? if expected_output.nil?
status.success? && output.include?(expected_output)
endIssue: If Fix: Add nil check: status.success? && output&.include?(expected_output)
CONFIG_FILE = ".dev-services.yml"
CONFIG_FILE_EXAMPLE = ".dev-services.yml.example"This would make it easier to change the filename convention later if needed. Testing (Low Priority)
# spec/react_on_rails/dev/service_checker_spec.rb
def with_temp_config(config_hash)
# ...
end
def capture_stdout
# ...
endSuggestion: Consider extracting to
Documentation (Minor)
install_hint: "brew install redis (macOS) or apt-get install redis-server (Linux)"Suggestion: Consider showing Linux-first example or Docker example for cross-platform teams.
🎯 Performance ConsiderationsCurrent: Services are checked sequentially in a loop (line 66-75 in diff). Potential optimization (future enhancement): Check services in parallel using threads for faster startup: failures = services.map do |name, config|
Thread.new { check_service(name, config) ? nil : { name: name, config: config } }
end.map(&:value).compactThis is not critical since service checks are typically fast (< 100ms each), but could improve UX for apps with many services. 🧪 Test Coverage AssessmentExcellent coverage with 11 test scenarios:
Missing test cases (nice-to-have):
🔐 Security Checklist
Recommendation: This is acceptable for a development-only tool, but document clearly that:
📋 Suggested Changes (Priority Order)High Priority:
Medium Priority: Low Priority (Future enhancements): ✨ Overall AssessmentRating: 9/10 - Outstanding PR that solves a real problem elegantly. This is production-ready code with excellent tests and documentation. The security concerns are minor for a dev-only tool, but worth addressing with clearer warnings and potentially safer command execution. The design philosophy (optional, zero-impact, helpful errors) is exactly right for this use case. This will significantly improve the developer experience for teams with external service dependencies. Recommendation: ✅ Approve with minor suggestions - The security items can be addressed in follow-up or before merge, but they don't block the core functionality. 🎉 Nice Touches
Great work! 🚀 |
Security improvements: - Clarify that Open3.capture3 doesn't invoke shell for simple commands - Update all security warnings to explain shell metacharacters won't work - Document recommended .gitignore approach (commit .example, gitignore actual) - Add execution order documentation (services → hook → Procfile) - Enhanced security note in docs with best practices Code robustness: - Add safe navigation operator for nil check: output&.include?(expected_output) - Prevents NoMethodError if output is nil - Add ArgumentError rescue for invalid command formats - Improved inline documentation about command execution Documentation improvements: - Expanded security note with IMPORTANT callout - Added recommended approach section - Added execution order section - Clarified that shell features will fail (not just "avoid") All tests passing (14/14). Zero RuboCop offenses. Addresses security and robustness feedback from PR review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2098: Add service dependency checking to bin/devOverall Assessment: ✅ Excellent Quality This is a well-designed, thoroughly tested, and production-ready feature. The code follows best practices and integrates seamlessly with the existing codebase. ✅ Strengths1. Excellent Code Quality
2. Comprehensive Test Coverage
3. Security Awareness
4. User Experience
5. Type Safety
6. Documentation
🔍 Suggestions1. Security: YAML Load (Low Priority)Location: lib/react_on_rails/dev/service_checker.rb:92-93 Consider using YAML.safe_load_file instead of YAML.load_file for defense-in-depth: YAML.safe_load_file(config_path, permitted_classes: [Symbol], aliases: true)Severity: Low (dev-only tool, user controls file), but an easy security win. 2. Documentation EnhancementConsider adding a code comment in run_check_command documenting that commands are executed via shell, for future maintainers. 3. Future Enhancement: Timeout SupportIf a check_command hangs indefinitely, it will block bin/dev startup. Consider adding timeout support in a future PR. Not a blocker. 🎯 Best Practices Followed✅ Backwards Compatibility: Zero impact when config doesn't exist 📋 Testing VerificationTest Coverage Analysis:
Excellent test coverage! ✅ Final RecommendationAPPROVE - Ready to Merge This PR is production-ready. Code quality is excellent, tests are comprehensive, and the feature is well-designed. Optional follow-ups (can be separate PR):
Great work on this feature! 🎉 It solves a real pain point in an elegant, user-friendly way. Review Summary:
RuboCop: Passing ✅ |
Code Review - PR #2098: Service Dependency CheckingThank you for this well-implemented feature! Overall, this is high-quality code with excellent test coverage and documentation. Here's my detailed review: ✅ Strengths1. Excellent Design Choices
2. Comprehensive Test Coverage
3. Documentation Quality
4. Code Quality
🔍 Observations & Minor Suggestions1. Security Consideration (already well-handled)The implementation correctly uses One enhancement to consider: You could add validation to detect and warn about shell metacharacters in the config file: UNSAFE_CHARS = /[;&|$`()<>]/.freeze
if check_command =~ UNSAFE_CHARS
warn "Warning: Shell metacharacters detected in check_command. These won't work as expected."
endThis would be purely educational, not security-critical, since 2. YAML.load_file Deprecation (low priority)Line 93 uses def load_config(config_path)
YAML.safe_load_file(config_path, permitted_classes: [], permitted_symbols: [], aliases: true)
rescue StandardError => e
# ...
endHowever, given that this is a trusted local config file (not user input), the current approach is acceptable. 3. Minor: Unused Parameter (already handled correctly)Line 101: 4. RBS Type Signature - Minor ImprovementIn
This is purely for type precision, not a functional issue. 5. Integration Point (works correctly)The integration in exit 1 unless ServiceChecker.check_servicesThis is correct and follows the fail-fast principle. The exit happens before process manager starts, which is exactly right. ✅ 🎯 Best Practices AdherenceChecking against CLAUDE.md requirements:
🚀 Performance Considerations
🔒 Security AssessmentOverall: Excellent security posture ✅ Commands executed via Risk level: Low 📋 Testing RecommendationsBefore merge, verify:
📊 Summary
✅ RecommendationAPPROVE - This is production-ready code that follows all project conventions and best practices. The suggestions above are minor enhancements, not blockers. Great work on this feature! It solves a real problem (confusing service startup errors) in an elegant, opt-in way. Reviewed by: Claude Code |
Summary
This PR introduces a service dependency checking system that validates required external services (like Redis, PostgreSQL, Elasticsearch) are running before
bin/devstarts the development server.Problem
Many applications depend on external services that must be running before the dev server starts. Currently, there are two approaches:
redis: redis-server) - Works but clutters the ProcfileNeither approach is ideal. The Procfile is meant for application processes, not infrastructure services.
Solution
A new
.dev-services.ymlconfiguration file that:Key Features
✅ Declarative configuration - Simple YAML format
✅ Self-documenting - Includes
.dev-services.yml.exampletemplate✅ Helpful errors - Clear instructions on how to start missing services
✅ Cross-platform - Works on macOS and Linux
✅ Zero impact - If no config exists, bin/dev works exactly as before
✅ Test coverage - 11 comprehensive tests, all passing
Configuration Example
User Experience
When all services are running:
When services are missing:
Implementation Details
New Files
lib/react_on_rails/dev/service_checker.rb- Core service checking logicspec/react_on_rails/dev/service_checker_spec.rb- Comprehensive test suitelib/generators/react_on_rails/templates/base/base/.dev-services.yml.example- Template for new projectsspec/dummy/.dev-services.yml.example- Example for dummy appModified Files
lib/react_on_rails/dev/server_manager.rb- Integrated service checks before starting Procfilelib/generators/react_on_rails/base_generator.rb- Added.dev-services.yml.exampleto generated filesService Checking Flow
.dev-services.ymlexists (if not, continue normally)check_commandexpected_output(if specified)Benefits
🎯 Self-documenting - New developers see exactly what services are needed
🎯 Cleaner Procfiles - Remove infrastructure services, keep only app processes
🎯 Better onboarding - Clear instructions instead of cryptic failures
🎯 Flexible - Each app can define its own requirements
🎯 Optional - Zero impact on existing installations
Test Plan
Breaking Changes
None. This is purely additive. If
.dev-services.ymldoesn't exist,bin/devbehaves exactly as before.Future Enhancements
Potential future improvements:
🤖 Generated with Claude Code
Summary by CodeRabbit
bin/dev: An optional.dev-services.ymlconfiguration file validates external services (Redis, PostgreSQL, Elasticsearch, etc.) are running before the development server starts. Provides clear error messages with startup and installation guidance when services are missing. Fully backward compatible—disabled by default.✏️ Tip: You can customize this high-level summary in your review settings.