Add POSIX compatibility spec and acceptance tests for TruShell v3#77
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main.rs (1)
165-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the empty-command guard.
The
if/elseexpression with an earlyreturnin theifbranch is valid but non-idiomatic. A plain early return is clearer.♻️ Proposed refactor
fn execute_system_command(cmd: &str, args: &[String]) { - let command_name = if cmd.is_empty() { - return; - } else { - cmd - }; + if cmd.is_empty() { + return; + } + let command_name = cmd; let child = Command::new(command_name)🤖 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 `@src/main.rs` around lines 165 - 170, In execute_system_command, replace the if/else assignment for command_name with a standalone early return when cmd is empty, then use cmd directly for subsequent command execution.tests/posix_compatibility.rs (2)
4-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to prevent hanging tests in CI.
wait_with_output()blocks indefinitely. If the shell fails to exit (e.g., a regression inexithandling), the test will hang forever in CI. Consider spawning the process in a thread with a timeout, or using a crate likewait-timeout.♻️ Proposed refactor using a timeout thread
use std::io::Write; use std::process::{Command, Stdio}; +use std::time::Duration; fn run_shell(input: &str) -> std::process::Output { let mut child = Command::new(env!("CARGO")) .arg("run") .arg("--quiet") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() .expect("failed to spawn trushell"); if let Some(stdin) = child.stdin.as_mut() { stdin .write_all(input.as_bytes()) .expect("failed to write shell input"); } drop(child.stdin.take()); - child.wait_with_output().expect("failed to read shell output") + let timeout = Duration::from_secs(10); + let result = child.wait_timeout(timeout).expect("failed to wait on process"); + match result { + Some(status) => { + let stdout = child.stdout.take().expect("failed to take stdout"); + let stderr = child.stderr.take().expect("failed to take stderr"); + std::process::Output { + status, + stdout: std::io::Read::bytes(&mut std::io::BufReader::new(stdout)).map(|b| b.unwrap()).collect(), + stderr: std::io::Read::bytes(&mut std::io::BufReader::new(stderr)).map(|b| b.unwrap()).collect(), + } + } + None => { + let _ = child.kill(); + panic!("shell did not exit within {timeout:?}"); + } + } }Alternatively, add the
wait-timeoutcrate toCargo.tomlfor a cleaner API.🤖 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 `@tests/posix_compatibility.rs` around lines 4 - 21, Update run_shell to enforce a finite timeout when waiting for the spawned shell process, using a timeout-capable approach such as wait-timeout or a dedicated waiting thread; ensure timed-out processes are terminated and the test fails clearly instead of blocking indefinitely.
41-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for empty quoted arguments.
The current test verifies that non-empty quoted arguments are preserved, but does not cover the case where a quoted argument is empty (e.g.,
printf '%s\n' ""). This would catch thesplit_posix_wordsbug where empty quoted arguments are dropped.🤖 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 `@tests/posix_compatibility.rs` around lines 41 - 56, Add a test alongside quoted_arguments_are_preserved_for_external_commands that runs an external printf command with an empty quoted argument, such as printf '%s\n' "", then assert successful execution and verify the output contains the expected blank argument. This should exercise split_posix_words and ensure empty quoted arguments are preserved rather than dropped.
🤖 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 `@src/main.rs`:
- Around line 116-162: The split_posix_words tokenizer silently accepts
unterminated quotes. After processing the input, check whether quote_mode is
still Some and emit an appropriate error or warning instead of treating the
remaining text as valid input; update callers as needed to propagate or handle
this failure while preserving normal word parsing.
- Around line 116-162: Update split_posix_words to track whether parsing is
currently inside a word separately from current’s contents, using an in_word
flag set when encountering quotes, escapes, or non-whitespace characters. Push
and reset the word on whitespace and at end of input whenever in_word is true,
so empty quoted arguments such as "" and '' are preserved.
In `@tests/posix_compatibility.rs`:
- Around line 24-26: Align the HOME fallback in
cd_without_arguments_uses_the_home_directory with the shell’s fallback behavior
in main, using "." when HOME is unset, or skip the path assertion when HOME is
unavailable. Ensure the test’s expected pwd output matches the directory
selected by cd without arguments.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 165-170: In execute_system_command, replace the if/else assignment
for command_name with a standalone early return when cmd is empty, then use cmd
directly for subsequent command execution.
In `@tests/posix_compatibility.rs`:
- Around line 4-21: Update run_shell to enforce a finite timeout when waiting
for the spawned shell process, using a timeout-capable approach such as
wait-timeout or a dedicated waiting thread; ensure timed-out processes are
terminated and the test fails clearly instead of blocking indefinitely.
- Around line 41-56: Add a test alongside
quoted_arguments_are_preserved_for_external_commands that runs an external
printf command with an empty quoted argument, such as printf '%s\n' "", then
assert successful execution and verify the output contains the expected blank
argument. This should exercise split_posix_words and ensure empty quoted
arguments are preserved rather than dropped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c3bca44-4815-4e6f-b724-1bb014783b20
📒 Files selected for processing (3)
POSIX_COMPATIBILITY_SPEC.mdsrc/main.rstests/posix_compatibility.rs
Comment on lines
+116
to
+162
| fn split_posix_words(input: &str) -> Vec<String> { | ||
| let mut words = Vec::new(); | ||
| let mut current = String::new(); | ||
| let mut chars = input.chars().peekable(); | ||
| let mut quote_mode: Option<char> = None; | ||
|
|
||
| while let Some(ch) = chars.next() { | ||
| match quote_mode { | ||
| Some('"') => match ch { | ||
| '\\' => { | ||
| if let Some(next) = chars.next() { | ||
| current.push(next); | ||
| } | ||
| } | ||
| '"' => quote_mode = None, | ||
| _ => current.push(ch), | ||
| }, | ||
| Some('\'') => { | ||
| if ch == '\'' { | ||
| quote_mode = None; | ||
| } else { | ||
| current.push(ch); | ||
| } | ||
| } | ||
| None => match ch { | ||
| '\'' => quote_mode = Some('\''), | ||
| '"' => quote_mode = Some('"'), | ||
| '\\' => { | ||
| if let Some(next) = chars.next() { | ||
| current.push(next); | ||
| } | ||
| } | ||
| ch if ch.is_whitespace() => { | ||
| if !current.is_empty() { | ||
| words.push(std::mem::take(&mut current)); | ||
| } | ||
| } | ||
| _ => current.push(ch), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| if !current.is_empty() { | ||
| words.push(current); | ||
| } | ||
|
|
||
| words |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unterminated quotes are silently accepted.
If a quote is opened but never closed, the tokenizer treats all remaining input as part of the current word without any warning. POSIX shells report a syntax error for unterminated quotes. Consider emitting an error or warning when quote_mode is still Some at the end of the loop.
🤖 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 `@src/main.rs` around lines 116 - 162, The split_posix_words tokenizer silently
accepts unterminated quotes. After processing the input, check whether
quote_mode is still Some and emit an appropriate error or warning instead of
treating the remaining text as valid input; update callers as needed to
propagate or handle this failure while preserving normal word parsing.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Empty quoted arguments are silently dropped.
split_posix_words only pushes a word when !current.is_empty(), so "" or '' produce no word at all. For example, echo "" hello yields ["echo", "hello"] instead of ["echo", "", "hello"]. This violates the POSIX compatibility spec's requirement to preserve quoted arguments for external commands (POSIX_COMPATIBILITY_SPEC.md, Section 2).
The root cause is that the function conflates "word has content" with "word was started." A quote pair explicitly starts a word even when it produces an empty string.
🐛 Proposed fix: track `in_word` to preserve empty quoted arguments
fn split_posix_words(input: &str) -> Vec<String> {
let mut words = Vec::new();
let mut current = String::new();
let mut chars = input.chars().peekable();
let mut quote_mode: Option<char> = None;
+ let mut in_word = false;
while let Some(ch) = chars.next() {
match quote_mode {
Some('"') => match ch {
'\\' => {
if let Some(next) = chars.next() {
current.push(next);
+ in_word = true;
}
}
- '"' => quote_mode = None,
+ '"' => {
+ quote_mode = None;
+ in_word = true;
+ }
_ => current.push(ch),
},
Some('\'') => {
if ch == '\'' {
- quote_mode = None,
+ quote_mode = None;
+ in_word = true;
} else {
current.push(ch);
}
}
None => match ch {
- '\'' => quote_mode = Some('\''),
- '"' => quote_mode = Some('"'),
+ '\'' => {
+ quote_mode = Some('\'');
+ in_word = true;
+ }
+ '"' => {
+ quote_mode = Some('"');
+ in_word = true;
+ }
'\\' => {
if let Some(next) = chars.next() {
current.push(next);
+ in_word = true;
}
}
ch if ch.is_whitespace() => {
- if !current.is_empty() {
+ if in_word {
words.push(std::mem::take(&mut current));
+ in_word = false;
}
}
- _ => current.push(ch),
+ _ => {
+ current.push(ch);
+ in_word = true;
+ }
},
}
}
- if !current.is_empty() {
+ if in_word {
words.push(current);
}
words
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn split_posix_words(input: &str) -> Vec<String> { | |
| let mut words = Vec::new(); | |
| let mut current = String::new(); | |
| let mut chars = input.chars().peekable(); | |
| let mut quote_mode: Option<char> = None; | |
| while let Some(ch) = chars.next() { | |
| match quote_mode { | |
| Some('"') => match ch { | |
| '\\' => { | |
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| } | |
| } | |
| '"' => quote_mode = None, | |
| _ => current.push(ch), | |
| }, | |
| Some('\'') => { | |
| if ch == '\'' { | |
| quote_mode = None; | |
| } else { | |
| current.push(ch); | |
| } | |
| } | |
| None => match ch { | |
| '\'' => quote_mode = Some('\''), | |
| '"' => quote_mode = Some('"'), | |
| '\\' => { | |
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| } | |
| } | |
| ch if ch.is_whitespace() => { | |
| if !current.is_empty() { | |
| words.push(std::mem::take(&mut current)); | |
| } | |
| } | |
| _ => current.push(ch), | |
| }, | |
| } | |
| } | |
| if !current.is_empty() { | |
| words.push(current); | |
| } | |
| words | |
| fn split_posix_words(input: &str) -> Vec<String> { | |
| let mut words = Vec::new(); | |
| let mut current = String::new(); | |
| let mut chars = input.chars().peekable(); | |
| let mut quote_mode: Option<char> = None; | |
| let mut in_word = false; | |
| while let Some(ch) = chars.next() { | |
| match quote_mode { | |
| Some('"') => match ch { | |
| '\\' => { | |
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| in_word = true; | |
| } | |
| } | |
| '"' => { | |
| quote_mode = None; | |
| in_word = true; | |
| } | |
| _ => current.push(ch), | |
| }, | |
| Some('\'') => { | |
| if ch == '\'' { | |
| quote_mode = None; | |
| in_word = true; | |
| } else { | |
| current.push(ch); | |
| } | |
| } | |
| None => match ch { | |
| '\'' => { | |
| quote_mode = Some('\''); | |
| in_word = true; | |
| } | |
| '"' => { | |
| quote_mode = Some('"'); | |
| in_word = true; | |
| } | |
| '\\' => { | |
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| in_word = true; | |
| } | |
| } | |
| ch if ch.is_whitespace() => { | |
| if in_word { | |
| words.push(std::mem::take(&mut current)); | |
| in_word = false; | |
| } | |
| } | |
| _ => { | |
| current.push(ch); | |
| in_word = true; | |
| } | |
| }, | |
| } | |
| } | |
| if in_word { | |
| words.push(current); | |
| } | |
| words | |
| } |
🤖 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 `@src/main.rs` around lines 116 - 162, Update split_posix_words to track
whether parsing is currently inside a word separately from current’s contents,
using an in_word flag set when encountering quotes, escapes, or non-whitespace
characters. Push and reset the word on whitespace and at end of input whenever
in_word is true, so empty quoted arguments such as "" and '' are preserved.
Comment on lines
+24
to
+26
| fn cd_without_arguments_uses_the_home_directory() { | ||
| let home = std::env::var("HOME").unwrap_or_else(|_| "/root".to_string()); | ||
| let output = run_shell("cd\npwd\nexit\n"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
HOME fallback mismatch between test and shell.
The test falls back to "/root" when HOME is unset, but the shell (src/main.rs line 40) falls back to ".". If HOME is unset, the test expects /root in pwd output while the shell changes to the current directory, causing a spurious failure. Align the fallbacks or skip the assertion when HOME is unset.
🤖 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 `@tests/posix_compatibility.rs` around lines 24 - 26, Align the HOME fallback
in cd_without_arguments_uses_the_home_directory with the shell’s fallback
behavior in main, using "." when HOME is unset, or skip the path assertion when
HOME is unavailable. Ensure the test’s expected pwd output matches the directory
selected by cd without arguments.
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.
This change introduces a compatibility specification for POSIX-style shell behavior and adds acceptance tests covering core cases such as cd, quoted arguments, and external-command fallback.
Related issue
Related to
POSIX compatibility spec & acceptance testsTask of Discussion #75Checklist
Summary by CodeRabbit
New Features
cdwithout arguments now navigates to the home directory.exitcleanly terminates the interactive shell.Documentation