Implement shell command execution with PATH search for /bin/sh#892
Merged
Conversation
Add exec.rs — the core shell executor that bridges the parser AST to runtime behavior. The executor handles: - Simple command dispatch: expand words (variables, field splitting, globs), check for builtins, otherwise fork+execve external commands - PATH search: iterate $PATH directories, access(X_OK) each candidate, exec the first executable match - Pipeline execution: create pipes between stages, fork each stage, wire stdin/stdout via dup2, wait for all children - List execution: sequential (;), AND (&&), OR (||) with short-circuit evaluation per POSIX - Subshell execution: fork, execute list in child, wait - Variable assignments: bare assignments update the environment - Exit status ($?) propagation through pipelines and lists - Command-not-found handling: diagnostic to stderr, exit status 127 - EXIT_REQUESTED sentinel preservation so `exit N` works correctly Update main.rs to wire the executor into a functional shell with: - stdin line-reading mode - sh -c "command" support - Environment variable import from the process environment Includes 27 host unit tests covering all execution paths. Closes #880 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a POSIX-style shell executor and CLI front-end: implements ChangesShell Executor & Frontend
sequenceDiagram
actor User
participant main as main.rs
participant Parser
participant Executor as exec.rs
participant Builtin as builtins
participant OS as Kernel/OS
User->>main: provide input ("cmd1 | cmd2")
main->>Parser: parse(input)
Parser-->>main: AST List / Pipeline
main->>Executor: execute_list(list, env)
Executor->>Executor: execute_pipeline(pipeline)
Executor->>OS: pipe(), fork() [stage 1]
OS-->>Executor: child_pid_1
Executor->>OS: fork() [stage 2]
OS-->>Executor: child_pid_2
Executor->>OS: dup2(pipe_fds) to wire stdin/stdout
rect rgba(100,150,200,0.5)
Note over OS,Builtin: Stage 1 may run builtin or exec external
OS->>Builtin: run_builtin? (if builtin)
Builtin-->>OS: produce output (to pipe)
end
rect rgba(150,100,200,0.5)
Note over Executor,OS: Stage 2 resolves command via $PATH and execve
Executor->>OS: resolve_command("cmd2")
OS->>OS: execve(...)
end
Executor->>OS: waitpid(child_pid_1)
Executor->>OS: waitpid(child_pid_2)
OS-->>Executor: exit_status (last stage)
Executor-->>main: return status
main-->>User: exit code / output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
base/sh/src/exec.rs (1)
203-213: 💤 Low valueConsider handling signal termination in wait status extraction.
The current extraction assumes the child exited normally. Per POSIX, if the child was killed by a signal, the exit status should be
128 + signal_number. The same pattern appears at lines 266 and 521.This is acceptable for initial implementation, but worth tracking.
♻️ Optional: Add signal handling
for (i, &pid) in child_pids.iter().enumerate() { let mut wstatus: i32 = 0; unsafe { wait4(pid, &mut wstatus, 0, std::ptr::null()); } - let exit_code = ((wstatus as u32) >> 8) & 0xFF; + let exit_code = if (wstatus & 0x7f) == 0 { + // Normal exit: WEXITSTATUS + ((wstatus as u32) >> 8) & 0xFF + } else { + // Killed by signal: 128 + signal_number + 128 + (wstatus & 0x7f) as u32 + }; if i == child_pids.len() - 1 { last_status = exit_code as i32; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/sh/src/exec.rs` around lines 203 - 213, The loop that reads wait4 statuses (the child_pids iteration) assumes normal exit by extracting exit_code with a right shift; instead detect termination method using libc helpers (e.g., WIFEXITED, WEXITSTATUS, WIFSIGNALED, WTERMSIG) and set last_status to WEXITSTATUS when WIFEXITED is true or to (128 + WTERMSIG) when WIFSIGNALED is true; apply the same fix to the analogous wait/status extraction sites mentioned (the similar blocks around the current code and at the other occurrences) so signal-terminated children produce 128+signal_number rather than a bogus value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/main.rs`:
- Around line 85-87: The code in run_input unconditionally assigns
env.last_status = status after calling execute_list, which clobbers the real
exit code set by the exit builtin (which returns EXIT_REQUESTED). Change the
assignment so env.last_status is only updated when status != EXIT_REQUESTED (or
equivalently preserve env.last_status if status == EXIT_REQUESTED); update the
block around execute_list / env.last_status in run_input to conditionally set
env.last_status to avoid overwriting the exit code written by exit.
---
Nitpick comments:
In `@base/sh/src/exec.rs`:
- Around line 203-213: The loop that reads wait4 statuses (the child_pids
iteration) assumes normal exit by extracting exit_code with a right shift;
instead detect termination method using libc helpers (e.g., WIFEXITED,
WEXITSTATUS, WIFSIGNALED, WTERMSIG) and set last_status to WEXITSTATUS when
WIFEXITED is true or to (128 + WTERMSIG) when WIFSIGNALED is true; apply the
same fix to the analogous wait/status extraction sites mentioned (the similar
blocks around the current code and at the other occurrences) so
signal-terminated children produce 128+signal_number rather than a bogus value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01c28125-ff97-4181-a655-36154b9a9dcf
📒 Files selected for processing (2)
base/sh/src/exec.rsbase/sh/src/main.rs
run_input() was unconditionally overwriting env.last_status with the return value from execute_list(), which clobbers the real exit code (e.g. 42) with the EXIT_REQUESTED sentinel (-1) when the exit builtin is used. Guard the assignment so env.last_status is only updated for non-sentinel return values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/main.rs`:
- Around line 52-72: The stdin loop in main() currently returns 0 on EOF,
dropping the last command's status; after the loop ends (the for line in
stdin.lock().lines() loop that calls run_input and checks for EXIT_REQUESTED),
call std::process::exit(env.last_status) so the process exits with the last
command status instead of falling through to 0; ensure this happens only after
the loop completes (and not inside the Err(_) break), preserving existing
EXIT_REQUESTED behavior and use of env.last_status.
- Around line 39-40: When handling the result of run_input in main (the variable
status), detect the EXIT_REQUESTED sentinel and, if seen, exit using
env.last_status instead of the raw sentinel; otherwise exit(status) as before —
i.e., change the exit logic after run_input(input, &mut env) to mirror the stdin
branch: if status == EXIT_REQUESTED { std::process::exit(env.last_status as i32)
} else { std::process::exit(status) } (referencing run_input, EXIT_REQUESTED,
and env.last_status).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04988c76-5b30-4290-ba28-f382848030a7
📒 Files selected for processing (1)
base/sh/src/main.rs
Address two CodeRabbit review comments: 1. In -c mode, if the command calls `exit`, the sentinel EXIT_REQUESTED was passed directly to std::process::exit instead of the actual exit code stored in env.last_status. 2. On stdin EOF the shell fell through to implicit exit(0), discarding the last command's exit status. Now exits with env.last_status. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
Addressed both remaining CodeRabbit review comments in 64b272f:
|
This was referenced May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #880
Summary
base/sh/src/exec.rs— the core shell executor that takes the parser AST and runs commands$PATHsearch viaaccess(X_OK)on each candidate path&&/||and sequential;$?) correctly, including theEXIT_REQUESTEDsentinel for theexitbuiltinmain.rsto wire the executor into a functional shell with stdin line-reading andsh -csupportTest plan
cargo testinbase/sh/— 366 tests pass (including 27 new exec tests)cargo xtask build— kernel + userspace builds cleancargo xtask test— all host unit tests pass; only pre-existingrwlock_concurrent_readersQEMU flake