feat(cli_tools): Add execute helper function to run shell commands #96
Conversation
📝 WalkthroughWalkthroughAdds a top-level re-export and a new execute(...) utility that spawns a platform shell, runs a command, forwards stdin/stdout/stderr and termination signals to the child, and returns the child's exit code. Adds an async dependency and tests with a driver (some tests are Unix-only). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/cli_tools/lib/src/execute/execute.dart (1)
52-58: Potential issue: stdin may not complete before process exits.If the child process exits before
stdincompletes,stdinSubscription?.cancel()is called afterprocess.stdin.close(). This ordering is fine, but if the stdin stream is infinite or long-running, the process could exit while stdin is still being forwarded. Consider cancelling the stdin subscription before closingprocess.stdinto ensure clean teardown.await [ stdout.addStream(process.stdout), stderr.addStream(process.stderr), ].wait; + await stdinSubscription?.cancel(); await process.stdin.close(); - await stdinSubscription?.cancel(); await sigSubscription.cancel();packages/cli_tools/test/execute_test.dart (1)
51-51: Error message assertion may be fragile across systems.The assertion
contains('not found')assumes a specific shell error message format. Different bash versions or locales may produce different messages (e.g., "command not found", "not found", or localized equivalents).Consider relaxing to just check for non-zero exit code, or use a case-insensitive/broader pattern:
- expect(result.stderr, contains('not found')); + expect(result.stderr.toLowerCase(), contains('not found'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/cli_tools/lib/execute.dart(1 hunks)packages/cli_tools/lib/src/execute/execute.dart(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/cli_tools/test/execute_driver.dart(1 hunks)packages/cli_tools/test/execute_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 57
File: packages/config/test/better_command_runner/default_flags_test.dart:1-1
Timestamp: 2025-08-07T07:55:17.269Z
Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.
Applied to files:
packages/cli_tools/lib/execute.dart
📚 Learning: 2025-06-12T14:55:38.006Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 47
File: lib/src/config/options.dart:552-567
Timestamp: 2025-06-12T14:55:38.006Z
Learning: The codebase relies on a recent version of `package:args` where `ArgParser.addFlag` accepts the `hideNegatedUsage` parameter.
Applied to files:
packages/cli_tools/pubspec.yaml
🔇 Additional comments (4)
packages/cli_tools/lib/execute.dart (1)
1-1: LGTM!Clean re-export following Dart package conventions for exposing the public API.
packages/cli_tools/test/execute_driver.dart (1)
5-5: LGTM with caveat on argument handling.The driver correctly invokes
executeand propagates the exit code. Note thatargs.join(' ')will not preserve quoting for arguments containing spaces, but this is acceptable for the test use case where commands are passed as single-string arguments.packages/cli_tools/test/execute_test.dart (1)
19-19: Top-level Future eagerly executescompileDriver().The
_exefuture is created at import time, meaning the driver is compiled before any test runs. This is likely intentional for efficiency but worth noting—if compilation fails, all tests will fail during setup rather than with a clear test failure message.packages/cli_tools/pubspec.yaml (1)
19-19: LGTM!The
asyncpackage is a well-maintained official Dart package, andStreamGroup.mergeis appropriately used in the execute implementation for merging signal streams. The constraint^2.10.0is compatible with the SDK requirement of^3.3.0.
nielsenko
force-pushed
the
feat-execute
branch
from
0c1243c to
ae91cb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli_tools/test/execute_driver.dart (1)
5-5: Slightly more idiomaticmainand note on argument joiningFunctionally this is fine for a test driver. If you want to align with typical Dart style and make behavior a bit clearer, you could:
- Declare
mainasFuture<void>instead ofvoidwithasync.- Consider documenting that
argsare rejoined with a single space and that this is only meant to receive a single command line string (so callers should pass the command already assembled as one argument).Example:
-import 'package:cli_tools/execute.dart'; - -void main(final List<String> args) async => exit(await execute(args.join(' '))); +import 'package:cli_tools/execute.dart'; + +Future<void> main(final List<String> args) async { + // Expects the command as a single argument; tests pass it pre‑assembled. + final command = args.join(' '); + exit(await execute(command)); +}packages/cli_tools/lib/src/execute/execute.dart (1)
33-59: Ensure signal & stdin subscriptions are always cleaned up (usetry/finally)Right now, if streaming to
stdout/stderrthrows (e.g., broken pipe), theawait [].waitwill error and the subsequent cleanup (stdinSubscription?.cancel(),process.stdin.close(),sigSubscription.cancel()) is skipped. That can leave the signal subscription and the child process stdin open.Wrapping the streaming section in a
try/finallykeeps behavior the same on success but guarantees cleanup on error:// Stream output directly to terminal - await [ - stdout.addStream(process.stdout), - stderr.addStream(process.stderr), - ].wait; - await stdinSubscription?.cancel(); - await process.stdin.close(); - await sigSubscription.cancel(); - - return await process.exitCode; + try { + await [ + stdout.addStream(process.stdout), + stderr.addStream(process.stderr), + ].wait; + } finally { + await stdinSubscription?.cancel(); + await process.stdin.close(); + await sigSubscription.cancel(); + } + + return await process.exitCode;This makes
executemore robust in error conditions (e.g., when the parent’s stdout/stderr are no longer writable) without changing normal-path behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/cli_tools/lib/execute.dart(1 hunks)packages/cli_tools/lib/src/execute/execute.dart(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/cli_tools/test/execute_driver.dart(1 hunks)packages/cli_tools/test/execute_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli_tools/lib/execute.dart
- packages/cli_tools/pubspec.yaml
- packages/cli_tools/test/execute_test.dart
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Nice extension! Had two comments!
|
This was extracted from serverpod/serverpod#4416 for common use |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/execute/execute.dart (1)
1-2: Consider consolidating the duplicatedart:ioimport.The library is imported twice (with and without an alias). You can simplify by removing one import and using a consistent approach.
Option 1: Remove the aliased import and use unqualified references:
import 'dart:io'; -import 'dart:io' as io; import 'package:async/async.dart';Then update lines 29-30:
- stdout ??= io.stdout; - stderr ??= io.stderr; + stdout ??= stdout; + stderr ??= stderr;Wait, that won't work because of the parameter shadowing.
Option 2 (preferred): Keep only the aliased import:
-import 'dart:io'; import 'dart:io' as io;And ensure all dart:io references use the
io.prefix for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/lib/src/execute/execute.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-12T14:55:38.006Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 47
File: lib/src/config/options.dart:552-567
Timestamp: 2025-06-12T14:55:38.006Z
Learning: For this project, avoid review comments that simply repeat compile-time errors the Dart analyzer already reports; focus on issues not caught by the analyzer.
Applied to files:
packages/cli_tools/lib/src/execute/execute.dart
📚 Learning: 2025-10-10T08:50:09.902Z
Learnt from: indraneel12
Repo: serverpod/cli_tools PR: 0
File: :0-0
Timestamp: 2025-10-10T08:50:09.902Z
Learning: PR #71 in serverpod/cli_tools adds support for displaying OptionGroup names in CLI usage output. The implementation does NOT include fallback group names or numbered defaults for anonymous groups - it only displays the group.name when present and shows groupless options first without any header.
Applied to files:
packages/cli_tools/lib/src/execute/execute.dart
🔇 Additional comments (5)
packages/cli_tools/lib/src/execute/execute.dart (5)
22-28: Function signature is well-designed.The signature provides good flexibility with optional parameters for stdin, stdout, stderr, and working directory while keeping the command parameter required.
29-34: Platform-appropriate shell selection.The default parameter handling and shell selection logic correctly distinguishes between Windows (cmd) and Unix-like systems (bash).
36-43: Well-justified shell invocation approach.The comment clearly explains why invoking the shell explicitly is preferred over
runInShell: trueto avoid edge cases with quoting and spaces. The implementation is correct.
45-53: Signal forwarding correctly handles Windows platform differences.The implementation properly guards
ProcessSignal.sigtermwith a Windows check, preventing runtime exceptions on Windows while still forwarding SIGINT on all platforms. This addresses the earlier review feedback.
55-72: Resource management and cleanup are handled correctly.The implementation properly:
- Forwards stdin with appropriate error handling
- Streams stdout/stderr in parallel using
.wait- Cleans up all subscriptions and streams in the correct order
- Returns the child process exit code
The sequence ensures that output is fully consumed before closing streams and cancelling signal handlers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/execute/execute.dart (1)
57-62: Consider logging stdin stream errors for debugging.The empty
onErrorhandler silently swallows any errors from the stdin stream. While errors are unlikely, having at least debug-level logging would make troubleshooting easier if stdin forwarding issues occur.For example:
final stdinSubscription = stdin?.listen( process.stdin.add, cancelOnError: true, onError: (final error, final stackTrace) { // Log or report the error for debugging purposes stderr?.writeln('Warning: stdin stream error: $error'); }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/lib/src/execute/execute.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-12T14:55:38.006Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 47
File: lib/src/config/options.dart:552-567
Timestamp: 2025-06-12T14:55:38.006Z
Learning: For this project, avoid review comments that simply repeat compile-time errors the Dart analyzer already reports; focus on issues not caught by the analyzer.
Applied to files:
packages/cli_tools/lib/src/execute/execute.dart
📚 Learning: 2025-10-10T08:50:09.902Z
Learnt from: indraneel12
Repo: serverpod/cli_tools PR: 0
File: :0-0
Timestamp: 2025-10-10T08:50:09.902Z
Learning: PR #71 in serverpod/cli_tools adds support for displaying OptionGroup names in CLI usage output. The implementation does NOT include fallback group names or numbered defaults for anonymous groups - it only displays the group.name when present and shows groupless options first without any header.
Applied to files:
packages/cli_tools/lib/src/execute/execute.dart
🔇 Additional comments (8)
packages/cli_tools/lib/src/execute/execute.dart (8)
1-4: LGTM!The imports are appropriate. The
ionamespace alias helps distinguish local parameters from global stdio, and theasyncpackage is correctly used for stream merging.
6-23: LGTM!The documentation is comprehensive and clearly explains the function's behavior, including signal forwarding, stream handling, and the fact that it runs shell commands.
24-30: LGTM!The function signature is well-designed with appropriate optional parameters for customization while maintaining sensible defaults.
31-33: LGTM!The default values are appropriate and correctly use the
ionamespace to access global stdio streams.
35-36: LGTM!The platform-specific shell selection is correct (
cmd /con Windows,bash -con Unix-like systems).
38-45: LGTM!The process start logic is correct. The comment helpfully explains why invoking the shell directly is preferred over
runInShell: trueto handle quoting and spacing edge cases.
47-55: LGTM!The signal forwarding logic correctly handles platform differences by conditionally including SIGTERM only on non-Windows platforms. The Windows SIGTERM issue from previous reviews has been properly addressed.
64-73: LGTM!The output streaming and cleanup sequence is correct:
- Waits for all child output to be captured
- Cancels stdin subscription
- Closes child stdin to signal EOF
- Cancels signal subscription
- Returns the exit code
This ensures proper resource cleanup and all output is captured before returning.
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Looks good! Remember to include scope in the PR, title:
feat(cli_tools)
That will enable the changelog and version scripts to work correctly.
christerswahn
changed the title
2 participants
Add
executehelper for running shell commands with proper signal forwarding, stdin passthrough, and real-time output streaming - unlikeProcess.runwhich buffers output and doesn't forward signals.Summary by CodeRabbit
New Features
Exports
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.