Step 2: Process spawning — the I/O foundation#2
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 46 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
Reviewer's GuideIntroduces the foundational process-spawning module in Rust, with options and result types for external commands, plus documentation for the learning step and wiring the module into main. Sequence diagram for run_process and run_shell invocation flowsequenceDiagram
actor CLIUser
participant Main
participant ProcessModule
participant OSProcess
CLIUser->>Main: execute wt-cli
Main->>ProcessModule: run_process("git", ["status", "--porcelain"], &options)
ProcessModule->>OSProcess: Command::new("git").args(args).output()
alt process_start_error
OSProcess-->>ProcessModule: Err(io::Error)
ProcessModule-->>Main: Err(String)
Main-->>CLIUser: print error and exit
else process_started
OSProcess-->>ProcessModule: Ok(Output)
ProcessModule->>ProcessModule: build ProcessResult
alt non_zero_exit_and_not_allowed
ProcessModule->>ProcessModule: format_failure(command, args, &result)
ProcessModule-->>Main: Err(String)
Main-->>CLIUser: print formatted failure
else success_or_allowed_failure
ProcessModule-->>Main: Ok(ProcessResult)
Main-->>CLIUser: print stdout / continue
end
end
CLIUser->>Main: execute shell command
Main->>ProcessModule: run_shell("echo hello", &options)
ProcessModule->>OSProcess: Command::new("sh").arg("-c").arg(command).output()
OSProcess-->>ProcessModule: Ok(Output)
ProcessModule-->>Main: Ok(ProcessResult)
Main-->>CLIUser: print stdout
Updated class diagram for the new process module typesclassDiagram
class ProcessOptions {
+Option~String~ cwd
+bool allow_failure
+Option~Vec~env
+new() ProcessOptions
+default() ProcessOptions
}
class ProcessResult {
+String stdout
+String stderr
+i32 status
}
class ProcessModule {
+run_process(command: &str, args: &[&str], options: &ProcessOptions) Result~ProcessResult, String~
+run_shell(command: &str, options: &ProcessOptions) Result~ProcessResult, String~
-format_failure(command: &str, args: &[&str], result: &ProcessResult) String
}
ProcessModule --> ProcessOptions : uses
ProcessModule --> ProcessResult : returns
ProcessOptions ..|> ProcessOptions : Default
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lessons/02-process-spawning.md" line_range="43" />
<code_context>
+
+`Command::new("name")` creates a command builder. You chain methods to configure it (args, working directory, environment). `.output()` runs it and captures stdout/stderr. `.status()` runs it and returns just the exit code.
+
+**The key difference:** In Node, spawnSync returns synchronously. In Rust, `.output()` and `.status()` are also synchronous (they block the thread). If you need async, that's `.spawn()` + tokio — but we don't need that here. wt-cli is a CLI tool, blocking is fine.
+
+**Common gotcha:** `.output()` returns `Result<Output, io::Error>`. You must handle the outer Result (did the process even start?) separately from the exit status (did the process succeed?). Two layers of "can go wrong."
</code_context>
<issue_to_address>
**nitpick (typo):** Consider capitalizing "Tokio" to match the usual project name capitalization.
Here it’s referring to the Tokio async runtime, which is conventionally capitalized in Rust documentation. Matching that convention will keep the docs consistent with the broader ecosystem.
```suggestion
**The key difference:** In Node, spawnSync returns synchronously. In Rust, `.output()` and `.status()` are also synchronous (they block the thread). If you need async, that's `.spawn()` + Tokio — but we don't need that here. wt-cli is a CLI tool, blocking is fine.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| `Command::new("name")` creates a command builder. You chain methods to configure it (args, working directory, environment). `.output()` runs it and captures stdout/stderr. `.status()` runs it and returns just the exit code. | ||
|
|
||
| **The key difference:** In Node, spawnSync returns synchronously. In Rust, `.output()` and `.status()` are also synchronous (they block the thread). If you need async, that's `.spawn()` + tokio — but we don't need that here. wt-cli is a CLI tool, blocking is fine. |
There was a problem hiding this comment.
nitpick (typo): Consider capitalizing "Tokio" to match the usual project name capitalization.
Here it’s referring to the Tokio async runtime, which is conventionally capitalized in Rust documentation. Matching that convention will keep the docs consistent with the broader ecosystem.
| **The key difference:** In Node, spawnSync returns synchronously. In Rust, `.output()` and `.status()` are also synchronous (they block the thread). If you need async, that's `.spawn()` + tokio — but we don't need that here. wt-cli is a CLI tool, blocking is fine. | |
| **The key difference:** In Node, spawnSync returns synchronously. In Rust, `.output()` and `.status()` are also synchronous (they block the thread). If you need async, that's `.spawn()` + Tokio — but we don't need that here. wt-cli is a CLI tool, blocking is fine. |
There was a problem hiding this comment.
Pull request overview
Adds the Step 2 lesson (“Process Spawning — the I/O foundation”) describing how to implement the foundational process execution layer in the Rust rewrite.
Changes:
- Introduces a new lesson markdown for process spawning, error handling with
Result, andstd::process::Command. - Specifies the intended API (
ProcessOptions,ProcessResult,run_process,run_shell) and a suggested test skeleton.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Update `src/main.rs` to include the module: | ||
| ```rust | ||
| mod types; |
There was a problem hiding this comment.
The snippet suggests adding mod types; in src/main.rs, but this repository currently doesn't have src/types.rs. Following these instructions as-is will cause a compile error. Either remove mod types; from the step-2 instructions or explicitly note that step 1 must be completed (and types.rs added) before making this change.
| mod types; |
| Define a `ProcessOptions` struct: | ||
| - `cwd: Option<String>` — working directory (None = current dir) | ||
| - `allow_failure: bool` — if true, don't error on non-zero exit | ||
| - `env: Option<Vec<(String, String)>>` — extra environment variables | ||
|
|
There was a problem hiding this comment.
ProcessOptions defines env, but the task steps for run_process/run_shell don't mention applying it to the Command builder. Add an explicit instruction to call .envs(...) / .env(...) when options.env is Some, otherwise implementers may ignore this option.
| ## Your Task | ||
|
|
||
| **File:** `src/process.rs` (create this file) | ||
|
|
There was a problem hiding this comment.
PR description says to implement src/process.rs, update src/main.rs, and make tests pass, but this PR currently only adds the lesson markdown (no Rust code changes in src/). If this PR is meant to be doc-only, the description should be updated; otherwise the missing code/test changes need to be included before merge.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dce02b9dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mod types; | ||
| mod process; |
There was a problem hiding this comment.
mod types; from step instructions
The lesson tells learners to add mod types; in src/main.rs, but in this repo state there is no src/types.rs, so following the snippet verbatim triggers Rust module resolution error E0583 and prevents cargo test from running for the process-spawning task. Either omit that line in this step or explicitly state that Step 1 (creating types.rs) is a required prerequisite.
Useful? React with 👍 / 👎.
| - `cwd: Option<String>` — working directory (None = current dir) | ||
| - `allow_failure: bool` — if true, don't error on non-zero exit | ||
| - `env: Option<Vec<(String, String)>>` — extra environment variables |
There was a problem hiding this comment.
env options into spawned commands
The task defines ProcessOptions.env as “extra environment variables,” but the implementation bullets for run_process and run_shell never require applying them to std::process::Command. As written, a learner can satisfy all instructions while silently ignoring caller-provided environment overrides, which makes the API misleading and breaks scenarios that depend on per-command env injection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274aee630d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let output = cmd.output().map_err(|e| e.to_string())?; | ||
| let status = output.status.code().unwrap_or(0); |
There was a problem hiding this comment.
The suggested implementation let status = output.status.code().unwrap_or(0); converts signal-terminated children (code() == None) into exit status 0, and the next check (status != 0) then treats those runs as success when allow_failure is false. In practice, a subprocess killed by SIGTERM/SIGKILL would be reported as Ok(...), which can let later steps proceed on a failed command; please map None to a non-zero status (or read the signal) so these cases are surfaced as errors.
Useful? React with 👍 / 👎.
Rust concepts: std::process::Command, Result<T,E>, the ? operator, String vs &str Source files: src/core/process.ts (79 lines) This PR is documentation only — no Rust code to review. Read the lesson, implement the task, make the tests pass.
Added the full original process.ts and a line-by-line translation walkthrough explaining spawnSync→Command, throw→Result, shell:true→sh -c, and Default trait vs empty object defaults.
OctavianTocan
force-pushed
the
rust-mentor/step-2-process-spawning
branch
from
274aee6 to
8c423db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c423db9c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - Otherwise return `Ok(ProcessResult { stdout, stderr, status })` | ||
|
|
||
| **`pub fn run_shell(command: &str, options: &ProcessOptions) -> Result<ProcessResult, String>`** | ||
| - Same as run_process but uses `Command::new("sh").arg("-c").arg(command)` to run through a shell |
There was a problem hiding this comment.
run_shell
This step requires Command::new("sh").arg("-c"), which only works on Unix-like systems and diverges from the source behavior (shell: true) that uses the OS default shell. On Windows environments where sh is absent, a learner can follow the instructions exactly and still get runtime Err(...) for shell commands, breaking the foundational process API for all later steps. Either mark the lesson as Unix-only or teach a cfg(target_os) split (for example cmd /C on Windows).
Useful? React with 👍 / 👎.
Learning step — doc only. Read
lessons/02-process-spawning.md, implement the task insrc/process.rs, make the tests pass.Rust concepts: std::process::Command, Result<T,E>, the ? operator, String vs &str
Source: src/core/process.ts (79 lines)
What you'll build: The foundational I/O module that all git/shell operations use
When tests pass, merge this PR.
Summary by Sourcery
Documentation: