feat(shell): implement ish/sh/ash POSIX-subset shell interpreter

[IamKenae]

Summary

Implemented ish (Idle Shell), a pure Rust POSIX-subset shell interpreter with zero external dependencies, registered as three applet aliases: ish, sh, and ash.

Features

Core Shell Functionality

• Lexer & Parser: Complete tokenization and AST parsing for POSIX shell syntax
• Executor: Full command execution with variable expansion and environment handling
• Built-in Commands: cd, exit, export, unset, alias, unalias, read, pwd
• In-memory Applet Dispatching: Direct function calls to IdleBox applets (no fork/exec overhead)

Shell Features

• Environment variable expansion (``, ${VAR})
• Pipes (|) with OS-level stream handling
• Logical operators (&&, ||) with short-circuit evaluation
• Command separators (;)
• Redirections (>, >>, <, 2>, 2>>)
• Control flow: if/then/elif/else/fi, for/in/do/done, while/do/done
• Alias expansion
• Special variable: 0 (last exit code)

Operating Modes

• Interactive REPL: With ish$ prompt
• Script execution: ish script.sh (with shebang support)
• Command mode: ish -c "command"

Known Limitations

This is a POSIX-subset shell interpreter. The following POSIX features are not implemented:

1. Builtins with redirections: Builtins (cd, export, etc.) with redirections will fail because they execute in-process and don't support fd redirection without dup2. This is a design trade-off to maintain zero external dependencies.

2. Pipelines with builtins/applets: Multi-element pipelines always spawn external commands. If a command is only available as an IdleBox applet (not as a system binary), it won't work in pipelines on minimal systems without that binary.

3. Background execution (&): Lexed but not implemented. Commands run synchronously.

4. Missing POSIX features: No here-docs, case, $(...), functions, ``/0, `cd -`, `OLDPWD`/`PWD` tracking.

Implementation Details

Architecture

• lexer.rs: Tokenizes shell input (strings, quotes, operators, keywords)
• parser.rs: Builds AST from tokens (commands, pipelines, lists, control flow)
• evaluator.rs: Executes AST nodes with proper pipeline and redirection handling
• builtins.rs: Shell built-in commands implementation
• ish.rs: Main applet with REPL, script, and command modes

Performance

Traditional shells use fork() + exec() for every command. ish achieves significant performance improvements for IdleBox applets through in-memory dispatching:

• IdleBox applets: Direct function call (no process creation)
• External commands: Standard fork() + exec() via std::process::Command

Testing

• Unit tests: 122 tests covering lexer, parser, builtins, and evaluator
• Integration tests: 20 comprehensive tests covering all operating modes
• All tests passing: 100% pass rate on Unix systems
• Cross-platform: Windows-compatible with appropriate #[cfg(unix)] gates

Documentation

Added bilingual documentation (English & Chinese):

• idlebox-docs/en/applets/ish.md, sh.md, ash.md
• idlebox-docs/zh/applets/ish.md, sh.md, ash.md

Example Usage

# Interactive mode
idlebox ish
ish$ echo "Hello, World!"
Hello, World!
ish$ export GREETING=Hi
ish$ echo $GREETING
Hi

# Command mode
idlebox ish -c "echo hello | grep hello"
# Output: hello

# Script mode
idlebox ish script.sh

# Using aliases
idlebox sh -c "echo hello"
idlebox ash -c "echo hello"

All three commands (ish, sh, ash) provide identical functionality.

[github-actions]

Here is my review of the ish/sh/ash shell interpreter PR.

---

Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

BUGS (must fix)

1. Single-quoted strings expand variables (evaluator.rs:276-319)

The evaluator's expand_variables runs on all strings unconditionally. When the lexer encounters '$HOME', single-quote handling preserves the literal $HOME token, but the evaluator then expands it. POSIX requires single quotes to prevent all expansion.

echo '$HOME'   # should print: $HOME
                # currently prints: /home/user

Fix: either tag tokens with a "came from single quotes" flag so the evaluator skips them, or move expansion entirely into the lexer (handling single-quote suppression there) and remove it from the evaluator.

2. read builtin bypasses shell state (builtins.rs:191)

std::env::set_var(var_name, line) sets the process environment directly instead of state.set_var(...). The variable won't appear in ShellState.env, so subsequent $VAR expansion in the shell won't see it.

// builtins.rs:191 — currently:
std::env::set_var(var_name, line);
// should be:
state.set_var(var_name.to_string(), line.to_string());

This also avoids the std::env::set_var soundness concern in recent Rust editions.

3. Builtins with redirections silently fall through to external execution (evaluator.rs:61-69)

execute_command short-circuits to execute_command_with_redirects whenever redirections are present, which always spawns an external process. Builtins like cd, export, alias will fail with "command not found" when redirected.

cd /tmp > /dev/null    # tries to spawn system 'cd', fails
export FOO=bar > file  # tries to spawn system 'export', fails

The redirect file descriptors should be set up, the builtin executed in-process, then the descriptors restored. Or at minimum, route builtins through the redirect path with appropriate fd handling.

SIGNIFICANT GAPS

4. Missing $? special variable (evaluator.rs:276-319)

expand_variables only handles $VAR and ${VAR}. The $? special variable (last exit code) is core POSIX shell and heavily used in scripts. $#, $@, $* are also absent. At minimum, $? should be added:

Some('?') => {
    chars.next();
    result.push_str(&self.state.last_exit_code.to_string());
}

5. No 2>> stderr append redirect (lexer.rs:87-98)

2> is handled but 2>> is not. A 2 immediately followed by >> would lex as 2 (word) then >> (append), which is incorrect — the 2 should be consumed as the fd specifier.

6. Lexer peek_at is fragile (lexer.rs:128-130)

peek_at independently indexes into self.raw using self.pos, maintaining a parallel cursor alongside the Peekable<Chars> iterator. This works for ASCII but is error-prone if anyone changes the advance logic. Consider using a lookahead buffer or just peeking through the iterator.

CODE QUALITY

7. Three identical applet structs (ish.rs:6-80)

IshApplet, ShApplet, AshApplet have identical run implementations. A single struct with a name field (or a macro) eliminates the duplication:

pub struct ShellApplet(&'static str);
impl Applet for ShellApplet {
    fn name(&self) -> &'static str { self.0 }
    fn run(&self, args: &[String]) -> ... { /* shared impl */ }
}

8. Parser clones every token on every peek/advance (parser.rs:356-368)

peek() and advance() both .clone() the token. For a shell that may process large scripts, use indices into the token vec or store tokens by reference to avoid allocation pressure.

9. expand_variables logic is duplicated between lexer and evaluator

The lexer's expand_variable preserves $VAR as a literal string, and the evaluator's expand_variables expands it. This two-pass design works but is confusing — the evaluator doesn't know whether a $ came from inside single quotes. Consolidating expansion into one phase would fix bug #1 and simplify the code.

MINOR

• No here-doc (<<), no case, no subshells $(...), no functions — these are acceptable omissions for an initial implementation, but the PR description claims "POSIX-compatible" which overstates the current coverage. Consider calling it "POSIX-subset" or listing supported features explicitly.
• run_script (ish.rs:133-138) reads the entire file into memory. Fine for typical scripts but worth noting for very large files.

WHAT LOOKS GOOD

• Lexer/parser/evaluator separation is clean and testable.
• Pipeline implementation correctly uses OS pipes and waits for children.
• In-memory applet dispatching for builtins is a nice optimization.
• Logical operators (&&, ||) and control flow (if/for/while) are implemented correctly.
• The test suite is comprehensive for the features implemented.
• Integration tests cover all three applet aliases and multiple operating modes.

opencode session  |  github run

[github-actions]

Here's my review of the ish/sh/ash shell interpreter PR.

---

Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Confirmed Bugs

1. Builtins with redirections silently drop the redirect
evaluator.rs:99-101 — execute_command_with_redirects tries the builtin before setting up file descriptors. If the builtin succeeds, it returns immediately with no redirect applied.

echo "data" > /dev/null   # works (echo isn't a builtin)
pwd > /dev/null           # prints to stdout; redirect silently lost
export > out.txt          # same — output goes to stdout
cd /tmp > /dev/null       # cd succeeds; redirect ignored

Fix: open files, duplicate fds with nix::dup2 or std::os::unix::io::AsRawFd + raw syscall (or unsafe { libc::dup2 }), run the builtin, restore fds. Since this is a zero-dep project, the simplest portable fix is to skip builtins in the redirect path and fall through to external execution, or note this as a known limitation.

2. export VAR (without =) reads from std::env, not shell state
builtins.rs:120 — When export is called with a variable name (no =), it does std::env::var(arg). Variables set via read or export VAR=val only exist in ShellState.env, not in the process environment. So export VAR won't "see" them and won't mark them for export.

FOO=bar
export FOO          # should mark FOO for export, but doesn't

Fix: replace std::env::var(arg) with state.get_var(arg).cloned(), and set the variable in both state.env and std::env::set_var if the goal is true POSIX export behavior.

3. echo '$HOME' expands variables in concatenated single-quoted segments
lexer.rs:174-176 — When read_word encounters a ' inside an unquoted word (e.g., hello'$HOME'), it calls read_single_quoted_string and appends the literal text, but the overall token is still Token::Word (not LiteralWord). The evaluator then expands $HOME in it.

echo '$HOME'       # works (produces LiteralWord)
echo hello'$HOME'  # broken: produces Word("hello$HOME"), evaluator expands it

Fix: track whether any segment was single-quoted and set the literal flag accordingly in parse_simple_command, or propagate a had_single_quotes flag from the lexer.

Correct Previous Review (verified)

• $? is implemented (evaluator.rs:316-319). ✅
• 2>> is handled (lexer.rs:106-108). ✅
• read does use state.set_var (builtins.rs:191). ✅ (The previous review's claim was wrong.)
• The three applet structs are already consolidated into one ShellApplet with a name field. ✅

Significant Gaps

4. Background execution (&) is parsed but never executed.
The Amp token is produced by the lexer but the evaluator has no handler. sleep 5 & runs synchronously. For an initial implementation, either implement backgrounding or emit a warning.

5. $#, $@, $*, $!, $- are not implemented.
$? is the only special variable. These are heavily used in POSIX scripts. At minimum, $# and $@ should be added for script usability.

6. No here-docs (<<), case, subshells $(...), command substitution backticks, or functions.
Acceptable for an initial implementation, but the PR description says "POSIX-compatible" which overstates coverage. Recommend: "POSIX-subset shell" or list supported features explicitly.

Code Quality

7. Parser clones every token on every peek()/advance() (parser.rs:406-417).
peek() does .cloned().unwrap_or(...) and advance() clones + increments. For large scripts this is allocation-heavy. Consider using indices into the token vec (the pos field is already there) and returning references or copying only when needed.

8. peek_at in the lexer maintains a parallel cursor (lexer.rs:136-138).
self.raw.chars().nth(self.pos + offset) is O(n) per call and fragile alongside the Peekable<Chars> iterator. Consider a small lookahead buffer or just peeking two chars through the iterator.

9. Pipeline executor only handles Ast::Command (evaluator.rs:151).
If a pipeline element is Ast::List, Ast::If, etc., it returns an error. Pipeline elements should be able to be any command-like AST node.

Minor

• run_script reads the entire file into memory — fine for typical scripts, worth a comment.
• No set -e / set -o support — fine for now.
• builtin_exit in a pipeline doesn't actually stop execution of sibling pipeline processes.
• The REPL prompt is hardcoded to ish$ — consider including the username or cwd for usability.

What Looks Good

• Clean lexer/parser/evaluator separation with good test coverage.
• Pipeline implementation correctly uses OS pipes and waits for children.
• In-memory applet dispatching avoids fork/exec overhead for builtins.
• Control flow (if/for/while) and logical operators (&&/||) are correctly implemented.
• 56 unit tests + 20 integration tests covering all three aliases and all operating modes.
• ShellApplet is well-structured with a single struct and const constructor.

opencode session  |  github run

[github-actions]

Good — all tests pass and clippy is clean. Here's my review:

---

Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bugs

1. Builtins with redirections silently execute without redirections (evaluator.rs:60-68, 99-102)

When any redirection is present, execute_command routes to execute_command_with_redirects, which tries the builtin first (line 99). If the builtin matches, it returns immediately — the redirect is never applied. cd /tmp > /dev/null succeeds but the redirect is silently dropped; export FOO=bar > file writes to stdout instead of the file.

The comment at line 95-98 acknowledges this, which is fine for now. But the current code does try the builtin inside the redirect path, which gives the false impression it handles redirections. Either:

• Remove the builtin check from execute_command_with_redirects and let builtins fall through to external execution (consistent failure, less confusing), or
• Set a hard boundary: redirect path only handles external commands.

2. Single-quoted variables expanded by evaluator in mixed segments (lexer.rs:193-196)

echo hello'$HOME' — read_word finds ' and appends the literal $HOME, but returns (word, has_single_quotes=true). The caller at line 135-139 uses has_quotes to decide LiteralWord vs Word. So this case is actually handled correctly via has_single_quotes. ✅ Verified.

However, echo '$HOME' where the token starts with ' goes to line 120 → read_single_quoted_string directly, producing Token::LiteralWord("$HOME"). This is also correct. ✅

The earlier review concern about single-quote expansion is not a bug in the current code.

3. Lexer peek_at has O(n) complexity (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the beginning of the string each call. For ASCII input this is acceptable, but the '2' branch (line 102) calls it on every 2 character. Not a correctness issue, but worth a comment or a two-char lookahead buffer for clarity.

4. Lexer '2' handling has a subtle edge case (lexer.rs:102-111)

peek_at(1) uses self.pos which is maintained by advance(). This works correctly because the lexer hasn't advanced past the 2 yet. Verified that 2>> produces ErrAppend correctly. ✅

Correctness Issues

5. execute_command_with_redirects applies self.state.env after opening files (evaluator.rs:136-138)

The environment is set on the ProcessCommand which is correct, but this also means child processes inherit the full shell state. This is standard behavior.

6. Pipeline only handles Ast::Command nodes (evaluator.rs:157, 201-203)

echo "foo" | grep foo works, but if true; then echo a; fi | grep a fails with "pipeline elements must be commands". This is a reasonable limitation for an initial shell but should be documented.

Rust Idioms

7. Unnecessary return before Ok(...) in several places (e.g., evaluator.rs:61-62, evaluator.rs:87)

Not harmful but slightly non-idiomatic. Minor.

8. ShellState::new() copies all env vars into a HashMap (builtins.rs:14-17)

This is fine for correctness (shell owns its env copy) but means env changes in the shell don't propagate to the process and vice versa. This is correct shell behavior.

Cross-Platform

9. builtin_cd uses std::env::set_current_dir (builtins.rs:77)

This is the correct portable approach — works on Linux, macOS, and Windows. ✅

10. Pipeline implementation uses std::process (evaluator.rs:151-206)

Uses Stdio::piped() which is cross-platform. However, the PR description implies POSIX pipes; the actual implementation is platform-agnostic. ✅

Lightweight Principle

11. No new dependencies added ✅

The shell implementation is pure Rust with only tempfile as a test dependency. Good.

12. read_to_string for script execution (ish.rs:113)

Loads entire file into memory. Fine for typical shell scripts; no concern for a lightweight shell.

Code Quality

13. Tests are comprehensive — 56 unit tests + 18 integration tests, all passing. The test coverage for the features implemented is solid.

14. Architecture is clean — lexer/parser/evaluator/builtins separation follows established shell implementation patterns and is easy to extend.

Summary

Severity	Count	Items
Bug (fix)	1	Builtins with redirections silently ignore redirects
Known limitation	2	Pipeline only handles Commands; no $#, $@, here-docs, case, functions
Minor	3	peek_at O(n); unnecessary return; pipeline error message

The single-bug to fix is #1: either remove the builtin call from the redirect path (consistent "builtins don't support redirections" behavior) or document it prominently. The rest is solid for an initial implementation. Downgrading the PR description from "POSIX-compatible" to "POSIX-subset shell" would set accurate expectations.

opencode session  |  github run

[github-actions]

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bugs (must fix)

1. Builtins with redirections silently ignore redirections (evaluator.rs:99-106)
When a builtin like export, pwd, or alias has redirections, it executes in-process without applying the fds. export FOO=bar > file still writes to stdout.

// Current: builtin runs, redirect is skipped entirely
if let Ok(code) = execute_builtin(&mut self.state, name, args) {
    self.state.last_exit_code = code;
    return Ok(code);
}

Fix: apply redirections before calling the builtin, or route builtins through the external path with fd setup/restore.

2. Pipeline executes builtins as external processes (evaluator.rs:155-201)
Every pipeline element spawns ProcessCommand::new(&name). If name is a builtin (e.g., echo, pwd), it tries to exec an external binary instead of calling the builtin. echo hello | cat would fail if there's no system echo.

3. Zombie processes leaked on pipeline errors (evaluator.rs:144-208)
If a later pipeline command fails to spawn, previously spawned children are never waited on. Use a cleanup pattern or scopeguard to ensure wait() is called on all spawned children before returning.

4. \r not treated as whitespace (lexer.rs:166-174)
On Windows-created files with \r\n endings, carriage returns are included in tokens. Add ch == '\r' to skip_whitespace.

5. exit with non-numeric arg silently uses 1 (builtins.rs:96)
POSIX requires printing an error and returning 2 for non-numeric arguments to exit.

6. export VAR (no =) is a no-op (builtins.rs:119-123)
When FOO is in state.env, state.set_var(arg.clone(), value.clone()) reads the value and writes it back unchanged. There's no export-tracking mechanism. At minimum, export FOO with no prior assignment should set it to empty string.

7. EOF after \ in double-quoted string silently drops backslash (lexer.rs:245-257)
If advance() returns None after a \ inside "...", the backslash is silently dropped and no error is raised.

Significant Gaps

8. Zero integration tests for sh/ash/ish
The tests/integration_test.rs file has no end-to-end tests for any shell applet variant. The unit tests in tests/ish.rs cover internal logic but not idlebox ish -c "..." invocation.

9. No shebang handling (ish.rs:110)
run_script passes the full file to the parser. A #!/bin/sh line causes a parse error. Skip the first line if it starts with #!.

10. $#, $@, $*, $!, $- not implemented
Only $? is handled as a special variable. These are heavily used in POSIX scripts.

Code Quality

11. Parser clones every token on peek()/advance() (parser.rs:407-418)
Each peek() and advance() clones the token (including its String contents). Use indices into the token vec or a cursor-based approach to avoid allocation pressure.

12. Non-deterministic export and alias output (builtins.rs:108-111, 143-146)
HashMap iteration order is random. Sort output for POSIX compliance and reproducibility.

13. Alphabetical ordering broken in dispatcher (dispatcher.rs:86-88, 97-99)
printenv/printf/ps and sort/sleep are out of order, breaking idlebox list output.

14. pub use sh::ShellApplet out of alphabetical order (applets/mod.rs:117)
Should be between sha512sum and sleep, matching the mod sh; placement.

Recommendation

Address items 1-4 before merge. Items 5-7 are straightforward fixes. Item 8 (integration tests) should be added given the complexity of a shell. Items 10-14 are acceptable follow-ups but should be tracked as issues. Consider updating the PR description from "POSIX-compatible" to "POSIX-subset" to accurately reflect current coverage (no here-docs, case, subshells, functions, command substitution).

opencode session  |  github run

[github-actions]

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

This is a solid initial shell with clean lexer/parser/evaluator architecture and good test coverage. Here are the issues worth addressing before merge, filtered to what actually matters.

Must-Fix Bugs

1. Builtins with redirections silently drop the redirect (evaluator.rs:60-68, 99-102)

When redirections are present, execute_command routes to execute_command_with_redirects, which tries builtins first — without applying any redirections. export FOO=bar > file writes to stdout; pwd > /dev/null prints to stdout. The comment at line 95-98 acknowledges this, but the code still tries the builtin, giving the false impression it works.

Fix: remove the builtin call from execute_command_with_redirects entirely so builtins fall through to external execution (which will fail with "command not found" consistently), or skip to external path when builtins are encountered. Alternatively, if the project is zero-dep, document this as a known limitation prominently.

2. Pipeline elements are always spawned as external processes (evaluator.rs:154-203)

Every pipeline element goes through ProcessCommand::new(&name). If name is a builtin like echo, pwd, or cd, it tries to exec an external binary instead of calling the in-process builtin. echo hello | cat would fail on a minimal system without a system echo. The non-pipeline path (execute_command) dispatches builtins and applets, but the pipeline path only spawns processes.

Fix: add a builtin/applet dispatch check inside the pipeline loop before spawning.

3. Pipeline leaks zombie processes on spawn failure (evaluator.rs:178-180)

If a later pipeline element fails to spawn(), previously spawned children are never wait()ed on — they become zombies. The let _ = child.wait() only runs on the success path after all elements are spawned.

Fix: use a cleanup pattern or ensure wait() is called in a scope guard / error path.

Significant Issues

4. Child process environment replaces parent entirely (evaluator.rs:171-173, 362-364)

cmd.env(key, value) for each var in self.state.env replaces the entire environment. If a var is unset from shell state, it disappears from child processes too. More critically, cmd.env_clear() is not called first, but the env() call with individual keys does not add to the parent environment — it starts from scratch. This means system-required vars like PATH, HOME, LANG would be missing if ShellState doesn't have them. Since ShellState::new() copies all env vars, this works at startup but breaks if the shell state diverges.

5. export with no args prints in non-deterministic order (builtins.rs:104-108)

HashMap iteration is random. POSIX requires sorted output. This will cause test flakiness and user confusion.

6. exit with non-numeric arg silently uses 1 (builtins.rs:93)

POSIX requires printing an error and returning exit code 2 for non-numeric arguments to exit.

7. \r not treated as whitespace (lexer.rs:166-174)

On Windows-created files with \r\n line endings, carriage returns will be included in tokens, causing parse failures. Add ch == '\r' to skip_whitespace.

Minor / Follow-Up

• peek_at is O(n) per call — acceptable but worth a comment
• No shebang handling in run_script — #!/bin/sh is parsed as a comment (works by coincidence, not intentionally)
• REPL prompt is hardcoded to ish$ regardless of which alias invoked it
• Dead code at evaluator.rs:206 (Ok(0) after the loop)
• PR description says "POSIX-compatible" — should say "POSIX-subset" given missing features (no case, functions, $(), here-docs, cd -, $@/$#/$*, etc.)

opencode session  |  github run

[github-actions]

Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Must-Fix Bugs

1. Pipeline never dispatches builtins or idlebox applets (evaluator.rs:154-203)

Every pipeline element goes through ProcessCommand::new(&name). If echo is an idlebox applet or a shell builtin, echo hello | cat tries to exec a system binary instead of using the in-process dispatch. The non-pipeline path (execute_command) correctly tries execute_builtin → dispatch_applet → execute_external, but the pipeline path skips the first two.

Fix: add a check inside the pipeline loop before spawning — if the command is a single-element pipeline (already handled at line 147), use execute_command; for multi-element pipelines, the last element should at minimum try applet dispatch if it's the last command. Or more practically: refactor execute_command so the pipeline can call its internal dispatch logic.

2. Builtins with redirections silently succeed without applying redirects (evaluator.rs:95-102)

The comment at line 95-98 documents this, but the code at line 99-101 still tries the builtin and returns Ok(code) if it succeeds — the redirect is silently dropped. pwd > /dev/null prints to stdout. export FOO=bar > file writes to stdout.

Fix: remove the builtin early-return from execute_command_with_redirects so builtins fall through to external execution (which fails with "command not found" consistently), or skip the redirect path entirely when the command is a known builtin.

3. \r not treated as whitespace (lexer.rs:166-174)

skip_whitespace only checks ' ' and '\t'. On Windows-created files with \r\n line endings, \r is included in token values, causing parse failures. Add || ch == '\r' to skip_whitespace.

Significant Issues

4. exit with non-numeric arg silently uses 1 (builtins.rs:93)

args[0].parse::<i32>().unwrap_or(1) should print an error and return exit code 2 per POSIX.

5. Zombie processes on pipeline spawn failure (evaluator.rs:178-180)

If a later pipeline element fails to spawn(), previously spawned children are never wait()ed on. Collect all children first, spawn in a loop with cleanup on error, or use a scope guard.

6. export/alias with no args output in non-deterministic order (builtins.rs:104-108, 139-143)

HashMap iteration order is random. Sort output for reproducibility and POSIX compliance.

7. No shebang handling (ish.rs:112-118)

run_script passes the full file to the parser. #!/bin/sh is parsed as a comment by coincidence (the # at position 0 triggers skip_comment). This works by accident, not intent. Add explicit shebang skipping.

Code Quality

8. Parser clones every token on peek() and advance() (parser.rs:403-415)

Each call clones the token including its String contents. Use indices into the token vec (the pos field already exists) and return references.

9. peek_at is O(n) (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the start each call. Acceptable for shell scripts but consider a 2-char lookahead buffer.

Minor

• PR description says "POSIX-compatible" — should say "POSIX-subset" (no here-docs, case, $(...), functions, $@/$#)
• REPL prompt is hardcoded ish$ regardless of alias name (ish.rs:67)
• Pipeline elements limited to Ast::Command — if true; then echo a; fi | cat errors

What Looks Good

• Clean lexer/parser/evaluator separation with good test coverage (56 unit + 18 integration tests)
• Single ShellApplet struct with const constructor, no duplication
• Pipeline correctly uses OS pipes and waits for children
• In-memory applet dispatching for non-pipeline commands avoids fork/exec
• Control flow (if/for/while) and logical operators (&&/||) work correctly
• Zero new dependencies, pure Rust
• Cross-platform std::env::set_current_dir for cd, std::process::Command for pipelines

opencode session  |  github run

[github-actions]

Now I have a complete picture. Here's my consolidated review:

---

Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bugs (must fix before merge)

1. Builtins with redirections silently execute without applying redirections (evaluator.rs:99-102)

execute_command_with_redirects tries the builtin at line 99 before setting up file descriptors. If the builtin matches, it returns immediately — the redirect is never applied:

cd /tmp > /dev/null    # cd succeeds, redirect silently dropped
export FOO=bar > file  # writes to stdout instead of file
pwd > /dev/null        # prints to stdout

The comment at line 95-98 acknowledges this, but the code still attempts the builtin inside the redirect path, giving the false impression it works. Fix: remove the execute_builtin call from execute_command_with_redirects entirely — let builtins fall through to the external command path with consistent "command not found" error, or document this as a known limitation prominently. The current behavior where some builtins work and some silently lose their redirects is worse than a consistent failure.

2. Pipeline only handles Ast::Command — silently fails for anything else (evaluator.rs:157, 201-203)

if true; then echo a; fi | grep a returns Err("pipeline elements must be commands"). While this is a known limitation, the parse_pipeline accepts any parse_command result (which includes if, for, while). The parser happily builds a pipeline containing control flow, then the evaluator rejects it at runtime.

Fix: Either restrict the parser to only accept simple commands as pipeline elements, or return a clear error at parse time. Currently, a script will parse successfully but fail at execution.

3. \r not treated as whitespace (lexer.rs:166-174)

Windows-created files with \r\n line endings will include \r in tokens. echo hello\r will print hello\r not hello. Add ch == '\r' to skip_whitespace:

if ch == ' ' || ch == '\t' || ch == '\r' {

Also add '\r' as a break condition in read_word at line 192.

4. exit with non-numeric arg silently uses 1 (builtins.rs:93)

POSIX requires printing an error and returning exit status 2 for non-numeric arguments to exit. Current code does parse::<i32>().unwrap_or(1):

$ exit abc
# should: "exit: abc: numeric argument required", exit 2
# currently: silently exits with 1

5. export VAR (no =) is a no-op (builtins.rs:116-119)

When FOO is in state.env, state.set_var(arg.clone(), value.clone()) reads the value and writes it back unchanged. This is a no-op. POSIX export FOO without prior assignment should set the variable (even to empty string) and mark it for export. Since there's no export-tracking mechanism here, export FOO on an unset variable does nothing.

export FOO    # should set FOO="" or at minimum not error
echo $FOO     # nothing

6. Alias values not quoted properly (builtins.rs:141)

alias ll='ls -l' stores ls -l including the quotes. When printed back, it shows alias ll='ls -l' which is correct by coincidence. But the value stored is actually 'ls -l' (with literal quotes) not ls -l. This means alias expansion (if it were implemented) would pass wrong arguments. The eq_pos split at line 148-149 doesn't strip surrounding quotes from the value.

Significant design issues

7. Aliases are stored but never expanded (builtins.rs:39-45, evaluator.rs:31-83)

The alias subsystem (set_alias, unset_alias, builtin_alias) works in isolation, but execute_command never checks aliases before dispatching. alias ll='ls -l'; ll will try to run ll as an external command. Either implement expansion in the evaluator (check state.aliases before dispatch) or remove the alias builtin to avoid confusion.

8. export output is non-deterministic (builtins.rs:104-109)

HashMap iteration order is random. export with no args produces different output on each run. POSIX export lists variables in sorted order. Use BTreeMap or sort before printing.

9. Pipeline zombie leak on spawn failure (evaluator.rs:199)

If cmd_proc.spawn()? fails mid-pipeline, previously spawned children are never waited on. Use a cleanup loop in the error path:

children.push(cmd_proc.spawn().map_err(|e| {
    for mut child in children { let _ = child.wait(); }
    e
})?);

Cross-platform

10. read_double_quoted_string escape handling is non-POSIX (lexer.rs:246-257)

read_double_quoted_string converts \n to newline, \t to tab, \r to carriage return. POSIX double-quoted strings only interpret \", \\, \$, and \` — they do NOT interpret \n, \t, \r. This is a bashism:

echo "hello\nworld"   # POSIX: prints "hello\nworld"
                      # ish:   prints "hello<newline>world"

This will break scripts that contain literal backslash sequences in double-quoted strings.

11. & token is lexed but never evaluated (evaluator.rs:154-203)

The Amp token is produced by the lexer but the evaluator never handles background execution. sleep 5 & runs synchronously. This should either be implemented or the lexer should not produce Amp (or the parser should error on it).

Code quality

12. Parser clones every token on peek() and advance() (parser.rs:403-415)

peek() does .cloned().unwrap_or(...) and advance() clones the token. For large scripts, this allocates a new String on every token access. Use indices into the token vec and return &Token from peek, cloning only when the value is consumed.

13. peek_at in the lexer is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the beginning of the string each call. For the '2' branch this is called on every 2 character. Consider a two-char lookahead buffer.

14. Inconsistent pub visibility (sh/mod.rs:1-7)

All sub-modules (builtins, evaluator, lexer, parser) are pub, exposing internal types. Only ShellApplet should be pub from the module's perspective. Use pub(crate) for internal modules.

15. Description says "POSIX-compatible" (ish.rs:22)

The shell lacks: here-docs (<<), case, subshells $(...), command substitution backticks, functions, $#/$@/$*, proper alias expansion, proper export semantics, set/unset -o, etc. Downgrade to "POSIX-subset shell interpreter" or list supported features.

Minor

• run_script reads entire file into memory (ish.rs:113) — fine for typical scripts but add a comment.
• No shebang (#!) handling — #!/bin/sh causes a parse error.
• REPL prompt is hardcoded to ish$ — consider including cwd for usability.
• run_command discards the exit code on error (ish.rs:108) — ? propagates the error string but loses the numeric code.
• HashMap for ShellState.env means export with no args has non-deterministic output (also mentioned above).

What looks good

• Clean lexer/parser/evaluator/builtins separation — easy to extend.
• Pipeline implementation correctly uses OS pipes and waits for children.
• ShellApplet is well-structured: single struct with name field, const constructor.
• Control flow (if/for/while) and logical operators (&&/||) are correctly implemented.
• expand_variables correctly handles $? and ${VAR} syntax.
• Single-quote expansion works correctly via LiteralWord tagging.
• 56 unit tests + 20 integration tests covering all three aliases.
• Zero new runtime dependencies — pure Rust.
• Good separation of concerns between lexer, parser, evaluator, and builtins.

Summary

Severity	#	Items
Bugs	6	Builtins+redirects silent drop, pipeline AST restriction mismatch, \r in tokens, exit error handling, export VAR no-op, alias quote stripping
Design gaps	5	Aliases not expanded, non-deterministic export, pipeline zombie leak, & not evaluated, double-quote escape handling
Code quality	4	Parser token cloning, lexer peek_at O(n), pub visibility, "POSIX-compatible" claim

The most critical fix is #1 (builtins+redirects). It silently produces wrong behavior. Everything else is either a clear failure or a design choice that can be tracked as follow-up issues. Downgrading the description from "POSIX-compatible" to "POSIX-subset" would set accurate expectations for this initial implementation.

opencode session  |  github run

[github-actions]

PR Review: feat(shell): ish/sh/ash POSIX shell interpreter

Must-Fix (4)

1. Builtins with redirections silently lose the redirect — evaluator.rs:60-68, 95-101

When redirections are present, execute_command routes to execute_command_with_redirects, which always spawns an external process via ProcessCommand::new(name). Builtins are never consulted. pwd > /dev/null prints to stdout; export FOO=bar > file writes to stdout.

Fix: either set up fd redirections around the in-process builtin call, or remove the builtin attempt from the redirect path entirely so builtins fail consistently with "command not found" rather than silently dropping redirects.

2. Pipeline never dispatches builtins or applets — evaluator.rs:148-203

Every pipeline element goes through ProcessCommand::new(&name). echo hello | cat tries to exec a system echo binary instead of using the in-memory applet dispatch. On a minimal system without /usr/bin/echo, this fails.

Fix: add a builtin/applet check inside the pipeline loop before spawning. For single-element pipelines, execute_command already handles this (line 147). For multi-element pipelines, each element should try in-process dispatch first.

3. \r not treated as whitespace — lexer.rs:166-174, 192

skip_whitespace only checks ' ' and '\t'. read_word break set also lacks '\r'. On Windows-created files with CRLF line endings, \r is included in token values. echo hello\r passes hello\r as an argument.

Fix: add '\r' to both skip_whitespace and read_word's break conditions.

4. Double-quote escape sequences are non-POSIX — lexer.rs:246-257

read_double_quoted_string converts \n to newline, \t to tab, \r to CR. POSIX double-quoted strings only interpret \", \\, \$, \`, and \<newline>. echo "hello\nworld" should print hello\nworld, not hello<newline>world.

Fix: remove \n/\t/\r handling; only handle the five POSIX-recognized escapes.

Should-Fix (4)

5. Missing POSIX special variables — evaluator.rs:297-343

$? works, but $#, $@, $*, $0-$9, $-, ${?} are unhandled (expand to empty). $@ and $# are heavily used in scripts.

6. Background execution & is lexed but never parsed — parser.rs:100-109, lexer.rs:82

Amp token is produced but the parser doesn't handle it. sleep 10 & produces a parse error.

7. export/alias output is non-deterministic — builtins.rs:104-108, 139-143

HashMap iteration order is random. POSIX requires sorted output. Use BTreeMap or sort before printing.

8. Pipeline zombie leak on spawn failure — evaluator.rs:178-180

If a later pipeline element fails to spawn(), previously spawned children are never wait()ed on.

Minor / Follow-Up (7)

• Mixed quoting ('hello'$FOO): entire word marked as LiteralWord, unquoted portion not expanded. Fix: track per-segment quoting.
• cd missing OLDPWD/PWD tracking and cd - — builtins.rs:66-84
• Parser clones every token on peek()/advance() — parser.rs:403-414. Use indices + references.
• for var; do (without in) not supported — parser.rs:308
• Internal modules pub should be pub(crate) — sh/mod.rs:1-5
• REPL prompt hardcoded ish$ regardless of alias — ish.rs:67
• No multi-line REPL support — interactive if/for entered line-by-line fails

Description Accuracy

The PR says "POSIX-compatible" but the shell lacks: here-docs, case, $(...), functions, $@/$#, proper alias expansion, cd -, and & background execution. Recommend "POSIX-subset shell interpreter."

What Looks Good

• Clean lexer/parser/evaluator/builtins separation
• In-memory applet dispatching avoids fork/exec for builtins
• ShellApplet uses a single struct with name field — no duplication
• read builtin correctly uses state.set_var (not std::env::set_var)
• exit with non-numeric arg correctly prints error and returns 2
• Pipeline uses OS pipes correctly, waits on children
• Zero new dependencies
• 56 unit tests + 18+ integration tests covering all three aliases
• Cross-platform: std::env::set_current_dir, std::process::Command, proper #[cfg(unix)] gates on tests

opencode session  |  github run

[github-actions]

Now I have a clear picture of the actual code. Here's my consolidated review:

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bugs (must fix)

1. Builtins with redirections always fail as "command not found" (evaluator.rs:60-68)

When any redirection is present, execute_command short-circuits to execute_command_with_redirects, which only spawns external processes via ProcessCommand::new(name). Builtins never execute — they fail with a spawn error. pwd > /dev/null and export FOO=bar > file will fail, not silently succeed. The test test_ish_stdout_redirect only works because echo exists as a system binary.

Fix: either (a) try builtins inside execute_command_with_redirects after setting up fd redirections, or (b) accept this as a documented limitation and add a clear error message for builtins with redirects.

2. Pipeline never dispatches builtins or idlebox applets (evaluator.rs:148-197)

Multi-element pipelines always spawn external processes. echo hello | grep hello works only if a system echo binary exists. On a minimal system, this fails. The single-element pipeline path (line 138-140) correctly delegates to execute(), but multi-element pipelines skip the builtin/applet check entirely.

Fix: add execute_builtin / dispatch_applet checks inside the pipeline loop before spawning.

3. Aliases stored but never expanded (builtins.rs:39-45, evaluator.rs:31-83)

alias ll='ls -l' stores the alias, but execute_command never checks state.aliases before dispatching. ll will fail with "command not found". Either implement alias expansion in the evaluator, or remove the alias builtin to avoid confusion.

4. echo "hello\nworld" expands \n to newline (lexer.rs:248)

The \n match arm in read_double_quoted_string catches actual newline characters (\<newline> = line continuation), which is POSIX-correct. However, the match arm '\n' also matches the character n after \ if... wait — no, \n in the match is the newline char, not n. Let me re-verify:

The match at line 248 is '\\' | '"' | '$' | '' | '\n'. In Rust, '\n'is the newline character. So\n(backslash + letter-n) falls to the_branch and is preserved literally.<newline>matches'\n'` and the newline is consumed (line continuation). This is actually POSIX-correct. Disregard this item.

Significant issues

5. export VAR (no =) is a no-op for unset variables (builtins.rs:119-127)

When VAR doesn't exist in state.env, the else if branch at line 124 does nothing. POSIX export FOO on an unset variable should set FOO to empty string.

// Current: does nothing if VAR is unset
} else if let Some(value) = state.get_var(arg) {
    state.set_var(arg.clone(), value.clone());
}
// Should:
} else {
    state.get_or_insert_empty(arg);  // or similar
}

6. & is lexed but the parser produces a parse error (parser.rs:100-108)

The lexer produces Token::Amp, but parse_list doesn't handle it — it falls to the _ => break branch. sleep 5 & parses as just sleep 5 (the & is silently dropped since is_at_end returns false at that point... actually, looking more carefully at parse_list line 122-146: the loop checks for Token::And and Token::Or but not Token::Amp. After parsing sleep 5, peek() returns Amp, which hits _ => break. Then parse_program at line 100-109 also hits _ => break. So & is silently ignored — the command runs synchronously. This should either implement backgrounding or emit a warning.

7. cd doesn't track OLDPWD or PWD (builtins.rs:66-84)

POSIX cd should set OLDPWD to the previous directory and PWD to the new one. Also, cd - (return to previous directory) is unsupported.

8. Pipeline error message doesn't distinguish control flow (evaluator.rs:202)

if true; then echo a; fi | grep a parses successfully (the parser allows parse_if as a pipeline element via parse_command), but the evaluator returns "pipeline elements must be commands". The parser should reject non-command AST nodes in pipelines at parse time.

Code quality

9. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the start each time. The '2' branch calls this on every 2 character. Acceptable for shell scripts but consider a 2-char lookahead buffer for clarity.

10. Child environment replaces parent entirely (evaluator.rs:127-129, 162-164)

cmd.env(key, value) for each var in state.env starts from an empty environment. ShellState::new() copies all process env vars at startup, so this works initially. But if a var is unset from shell state, it disappears from child processes too — correct POSIX behavior. However, system-required vars (PATH, HOME) set only in the process env would be missing if ShellState doesn't capture them. Since ShellState::new() does copy them, this is fine at startup. Just worth noting.

11. REPL prompt is hardcoded (ish.rs:67)

ish$ is used regardless of which alias (ish, sh, ash) invoked the shell. Should use self.name to show the correct prompt.

What looks good

• Clean lexer/parser/evaluator/builtins separation
• Single ShellApplet struct with const constructor — no duplication
• Pipeline correctly uses OS pipes and waits on children (including error cleanup at line 190-194)
• Control flow (if/for/while) and logical operators (&&/||) implemented correctly
• $? special variable handled correctly
• exit with non-numeric arg correctly prints error and returns 2
• read builtin uses state.set_var (not std::env::set_var)
• export/alias with no args output sorted (line 112, 147)
• Shebang handling in run_script (line 114-117)
• \r already handled in skip_whitespace and read_word break set
• Zero new runtime dependencies
• 56 unit + 18 integration tests covering all three aliases
• Cross-platform: std::env::set_current_dir, std::process::Command

Recommendation

Address items 1-3 before merge — they cause silent failures or broken functionality. Items 5-8 are straightforward fixes. Update the PR description from "POSIX-compatible" to "POSIX-subset" (no here-docs, case, $(...), functions, cd -, $@/$#).

opencode session  |  github run

[github-actions]

Now I have the complete picture. Here's my consolidated review:

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bugs

1. Builtins with redirections always fail as "command not found" (evaluator.rs:72-80)

When any redirection is present, execute_command short-circuits to execute_command_with_redirects, which only spawns external processes via ProcessCommand::new(name). Builtins like cd, pwd, export are never consulted:

# These all fail with "command not found":
cd /tmp > /dev/null
export FOO=bar > file
pwd > /dev/null

The non-pipeline path has the correct priority chain (builtin → applet → external) but is bypassed entirely when redirects exist. Fix: refactor execute_command_with_redirects to try builtins after setting up fd redirections, or restructure execute_command to always try builtins/applets regardless of redirect presence.

2. Pipeline never dispatches builtins or idlebox applets (evaluator.rs:160-227)

Every pipeline element uses ProcessCommand::new(&name). Multi-element pipelines like echo hello | grep hello only work because echo exists as a system binary. On a minimal system without /usr/bin/echo, this fails. The non-pipeline path (execute_command line 82-90) correctly tries builtins → applets → external, but the pipeline path skips the first two.

Fix: add builtin/applet dispatch checks inside the pipeline loop before spawning for each element.

3. export VAR (no =) is a no-op for unset variables (builtins.rs:119-127)

} else if let Some(value) = state.get_var(arg) {
    state.set_var(arg.clone(), value.clone()); // reads and rewrites same value
}
// If VAR is unset, nothing happens — no else branch

POSIX export FOO on an unset variable should set it to empty string. When FOO is already set, the current code just reads and rewrites the same value (a no-op, but at least doesn't break). The unset case is the real bug.

Fix: add an else branch that inserts an empty string for unset variables.

4. Pipeline allows control flow but evaluator rejects it (parser.rs:167-174, evaluator.rs:222-227)

The parser's parse_command() accepts if/for/while as pipeline elements, but the evaluator's pipeline handler only processes Ast::Command — everything else triggers "pipeline elements must be commands". A script will parse successfully but fail at runtime.

Fix: either restrict parse_pipeline to only accept simple commands (error at parse time), or document this as a known limitation.

Should-Fix

5. Aliases stored but alias value may contain surrounding quotes (builtins.rs:155-158)

When the user types alias ll='ls -l', the shell parser gives the argument as the literal string ll='ls -l' (the quotes survive lexing since it's an unquoted word containing ' as literal characters via the lexer's read_word). The eq_pos split produces value = "'ls -l'" with literal quote characters. The print-back at line 149 wraps in quotes again, so alias output looks correct by coincidence, but the stored value has wrong quoting.

This needs testing: alias ll='ls -l'; ll would pass 'ls -l' (with literal quotes) to the command dispatch. The fix depends on how the parser handles this — verify that alias ll='ls -l' correctly stores ls -l without quotes.

6. Amp token silently dropped (parser.rs:100-109)

The lexer produces Token::Amp for &, but parse_list doesn't handle it — it hits _ => break. The & is silently ignored and the command runs synchronously. Either implement background execution, emit a warning, or remove Amp from the lexer.

Code Quality

7. Parser clones every token on peek() and advance() (parser.rs:403-415)

peek() does .cloned().unwrap_or(...) and advance() clones the token (including its String). For large scripts this causes allocation pressure. Since the parser already uses a pos index, returning &Token from peek() would eliminate cloning. The clone can happen only in advance() where the value is consumed.

8. Internal modules use pub instead of pub(crate) (sh/mod.rs:1-5)

All sub-modules (builtins, evaluator, lexer, parser) are pub, leaking internal types. Only ShellApplet should be publicly accessible.

9. REPL prompt hardcoded to ish$ (ish.rs:67)

The prompt is always ish$ regardless of which alias (sh, ash) invoked the shell. Use self.name field. This requires passing the name into run_repl — currently the name is only on the ShellApplet struct, not threaded through to the function.

Minor / Follow-Up

• Description accuracy: PR says "POSIX-compatible" but lacks here-docs (<<), case, $(...), backticks, functions, $@/$#/$*, cd -. Recommend "POSIX-subset shell interpreter."
• peek_at is O(n) (lexer.rs:154-156) — acceptable for shell scripts but worth a comment.
• Pipeline only handles Ast::Command — documented above as bug feat(install): add safe preflight and dry-run #4.
• No multi-line REPL — interactive if/for entered line-by-line will fail.
• for var; do (without in) unsupported — POSIX allows this form.

Verified Correct (contradicting earlier reviews)

• $? is implemented (evaluator.rs:342-344) ✓
• 2>> is handled (lexer.rs:105-111) ✓
• read uses state.set_var (builtins.rs:190) ✓
• Single ShellApplet struct with name field (ish.rs:6-14) ✓
• \r treated as whitespace (lexer.rs:168) ✓
• exit with non-numeric arg prints error, returns 2 (builtins.rs:93-98) ✓
• export/alias output is sorted (builtins.rs:112, 147) ✓
• Shebang handling (ish.rs:114-117) ✓
• echo "hello\nworld" preserves \n literally (test lexer.rs:659-670) ✓
• Pipeline zombie cleanup on spawn failure (evaluator.rs:214-218) ✓
• echo hello | grep hello works — system echo exists in CI ✓

Summary

Severity	#	Issue
Bug	1	Builtins fail with redirect (always spawns external)
Bug	2	Pipeline skips builtin/applet dispatch
Bug	3	export VAR no-op when unset
Bug	4	Control flow in pipeline fails at runtime, not parse time
Should-fix	5	Alias value quoting needs verification
Should-fix	6	& silently ignored
Code quality	7-9	Token cloning, pub visibility, REPL prompt
Follow-up	—	Description accuracy, multi-line REPL, for without in

Items 1-2 are the most impactful — they make basic shell usage with redirections and pipelines unreliable. The rest are straightforward fixes.

opencode session  |  github run

[github-actions]

Now I have the full picture. Here's my consolidated review after verifying each issue against the actual code:

---

Consolidated Review: feat(shell): ish/sh/ash POSIX shell interpreter

Bugs (must fix)

1. Builtins with redirections always fail as spawn errors (evaluator.rs:72-80, 107)

When any redirection is present, execute_command routes to execute_command_with_redirects, which only spawns external processes via ProcessCommand::new(name) (line 107). There is no builtin check in this path. Builtins like cd, export, alias, pwd will fail with a spawn error — cd /tmp > /dev/null fails because there's no system cd binary. This is worse than silently dropping the redirect.

Fix: try builtins inside execute_command_with_redirects after setting up fd redirections (using dup2 or equivalent), or document this as a known limitation and skip the redirect path for builtins with a clear error.

2. Pipeline never dispatches builtins or idlebox applets (evaluator.rs:177)

Every pipeline element goes through ProcessCommand::new(&name). If echo is an idlebox applet (not a system binary), echo hello | cat fails on a minimal system. The single-element pipeline path (line 150-151) correctly delegates to execute(), but multi-element pipelines skip builtin/applet dispatch entirely.

Fix: add execute_builtin / dispatch_applet checks inside the pipeline loop before spawning, or refactor so the pipeline can reuse the dispatch logic from execute_command.

Design Issues

3. Aliases are stored and expanded, but never checked in pipelines (evaluator.rs:160-175)

Alias expansion IS implemented in execute_command (lines 36-45) and execute_pipeline (lines 166-174). This was a false positive in some earlier reviews. However, the pipeline expansion is a copy-paste of the non-pipeline version — consider extracting it into a helper.

4. & is lexed but silently ignored (parser.rs:119-149)

The Amp token is produced by the lexer but parse_list doesn't handle it — it hits _ => break. sleep 5 & parses as just sleep 5 (the & is silently dropped). Either implement background execution or remove the token from the lexer.

5. export VAR (no =) is a no-op for unset variables (builtins.rs:124-126)

When VAR doesn't exist in state.env, the else if branch at line 124 does nothing. POSIX export FOO on an unset variable should set FOO to empty string. Also, when VAR exists, state.set_var(arg.clone(), value.clone()) reads and writes the same value — a no-op that was supposed to "mark for export" but has no export-tracking mechanism.

6. cd missing OLDPWD/PWD tracking and cd - (builtins.rs:66-84)

POSIX cd should set OLDPWD to the previous directory and PWD to the new one. Also, cd - (return to previous directory) is unsupported.

Code Quality

7. Parser clones every token on peek()/advance() (parser.rs:403-415)

peek() does .cloned().unwrap_or(...) and advance() clones the token including its String contents. For large scripts, this allocates a new String on every token access. Use indices into the token vec and return &Token from peek, cloning only when the value is consumed.

8. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the start each time. The '2' branch (line 102) calls this on every 2 character. Consider a two-char lookahead buffer.

9. REPL prompt hardcoded to ish$ (ish.rs:67)

The run_repl function doesn't receive the applet name. When invoked as idlebox sh, the prompt still shows ish$. Pass the name through or use self.name.

10. Internal modules are pub (mod.rs:1-5)

All sub-modules (builtins, evaluator, lexer, parser) are pub, exposing internal types. Only ShellApplet should be pub from the module's perspective. Use pub(crate) for internal modules.

11. Pipeline elements restricted to Ast::Command (evaluator.rs:160, 223-228)

The parser allows parse_if, parse_for, parse_while as pipeline elements via parse_command (parser.rs:167-175), but the evaluator rejects them at runtime with "pipeline elements must be commands". The parser should reject non-command AST nodes in pipelines at parse time.

Minor / Follow-Up

• PR description says "POSIX-compatible" — should say "POSIX-subset" (no here-docs, case, $(...), functions, cd -, $@/$#)
• No shebang handling — #!/bin/sh is parsed as a comment by coincidence (works but not intentional)
• read_to_string for script execution loads entire file into memory — fine for typical scripts, add a comment
• Missing $#, $@, $*, $0-$9 — only $? is implemented as a special variable
• No multi-line REPL support — interactive if/for entered line-by-line fails

What Looks Good

• Clean lexer/parser/evaluator/builtins separation — easy to extend
• Single ShellApplet struct with const constructor — no duplication
• Pipeline correctly uses OS pipes, waits on children, and cleans up on spawn failure (lines 213-221)
• Control flow (if/for/while) and logical operators (&&/||) implemented correctly
• $? expansion and single-quote literal handling work correctly
• exit with non-numeric arg correctly prints error and returns 2
• read builtin uses state.set_var (not std::env::set_var)
• \r is handled in both skip_whitespace and read_word break set
• Double-quote escape handling is POSIX-correct (only \\, \", $, `, \<newline> are interpreted)
• Shebang stripping in run_script (line 114-117)
• Zero new runtime dependencies
• 56 unit tests + 18+ integration tests covering all three aliases
• Cross-platform: std::env::set_current_dir, std::process::Command

Summary

Severity	#	Items
Bugs	2	Builtins+redirects fail as spawn errors; pipeline skips builtin/applet dispatch
Design gaps	3	& silently ignored; export VAR no-op; cd missing POSIX features
Code quality	5	Parser token cloning; peek_at O(n); hardcoded prompt; pub visibility; pipeline AST mismatch
Minor	5	PR description accuracy; no shebang handling; file read; missing special vars; no multi-line REPL

The two bugs (#1, #2) are the main concern. Everything else is either a design choice or an acceptable limitation for an initial implementation.

opencode session  |  github run

[github-actions]

Review complete. The two critical bugs to address before merge are the builtin+redirect spawn failure and the pipeline's missing builtin/applet dispatch. The remaining items are straightforward fixes or documented limitations.

opencode session  |  github run

[github-actions]

Now I have the full picture. Let me compile a consolidated review based on actual code verification.

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Verified Correct (contradicting earlier reviews)

• Builtins with redirections work correctly (evaluator.rs:120-178) — saves fds via dup, applies dup2, executes in-process, restores fds. The earlier reviews were wrong.
• Pipeline dispatches builtins/applets (evaluator.rs:296-310, 338-381) — both last and non-last pipeline elements check is_builtin_or_applet before spawning.
• export VAR (no =) handles unset variables (builtins.rs:127-129) — sets to empty string if not present.
• & returns a clear parse error (parser.rs:144-146) — "background execution (&) is not supported".
• \r treated as whitespace (lexer.rs:168, 192).
• Double-quote escapes are POSIX-correct (lexer.rs:248) — only \\, \", $, `, \n are interpreted.
• $? is implemented (evaluator.rs:561-563).
• 2>> is handled (lexer.rs:105-107).
• read uses state.set_var (builtins.rs:194).
• Exit with non-numeric arg returns 2 (builtins.rs:96-98).
• export/alias output is sorted (builtins.rs:112-113, 150-151).
• Shebang handling (ish.rs:115-116).
• Pipeline zombie cleanup on spawn failure (evaluator.rs:214-218, 331-333, 404-409, 430-438).
• Single ShellApplet struct (ish.rs:6-14).
• pub(crate) visibility for internal modules (mod.rs:1-5).
• REPL prompt uses applet name (ish.rs:65).

Bugs

1. Pipeline only handles Ast::Command nodes (evaluator.rs:284, 441-446)

parse_command() accepts if/for/while as pipeline elements (parser.rs:172-174), but execute_pipeline only processes Ast::Command — all other nodes hit Err("pipeline elements must be commands"). The parser should restrict pipeline elements to simple commands, or the evaluator should handle other AST types.

2. Aliases stored but alias values include surrounding quotes (builtins.rs:159-162)

When the user types alias ll='ls -l', the parser lexes the argument as a word containing ll=ls -l (the shell doesn't strip the quotes from the value). The eq_pos split stores value = "ls -l" which is actually correct by coincidence — the single quotes are consumed by the read_single_quoted_string path in the lexer only when the value starts with '. However, if someone types alias ll=ls\ -l (escaped space), the value includes the backslash. The alias value stored is whatever the parser provides after word expansion. This needs a test to confirm edge cases.

3. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the beginning of the string each time. Called on every '2' character (line 102). Not a correctness issue for typical shell inputs, but worth a comment or a 2-char lookahead buffer.

Design Gaps (acceptable for initial implementation, but track as follow-ups)

• No $#, $@, $*, $0-$9 — only $? is handled as a special variable.
• No here-docs (<<), case, $(...), backticks, functions.
• cd missing OLDPWD/PWD tracking and cd - (builtins.rs:67-85).
• Parser clones every token on peek()/advance() (parser.rs:406-418) — allocates a String per access. Use indices + references for large scripts.
• No multi-line REPL — interactive if/for entered line-by-line fails.
• for var; do (without in) not supported (parser.rs:311 always expects In).

Description Accuracy

The PR says "POSIX-compatible" (ish.rs:22) but the shell lacks here-docs, case, $(...), functions, $@/$#/$*, cd -, and background execution. Recommend "POSIX-subset shell interpreter."

What Looks Good

• Clean lexer/parser/evaluator/builtins separation.
• In-memory applet dispatching avoids fork/exec for non-pipeline builtins.
• Pipeline correctly uses OS pipes, handles builtins via temp file capture on Unix.
• Control flow (if/for/while) and logical operators (&&/||) work correctly.
• Zero new runtime dependencies, pure Rust.
• 56 unit tests + 18 integration tests covering all three aliases.
• Cross-platform: std::env::set_current_dir, std::process::Command, proper #[cfg(unix)] gates on tests.

Summary

Severity	#	Issue
Bug	1	Pipeline allows control flow nodes but evaluator rejects them at runtime
Needs verification	1	Alias value quoting edge cases
Minor	1	peek_at O(n)
Follow-up	7	Missing POSIX features, description accuracy, parser cloning

The code is significantly better than earlier reviews suggested. The two most-cited "bugs" (builtins+redirects, pipeline dispatch) are actually implemented correctly. The single real bug is the pipeline/AST mismatch (#1), which should be fixed by restricting parse_pipeline to simple commands. Everything else is either a documented limitation or a follow-up item.

opencode session  |  github run

[github-actions]

Now I have the full picture after verifying every claim against the actual source code. Here's my consolidated review:

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Correctness

1. Pipeline allows control-flow AST nodes that the evaluator rejects at runtime (parser.rs:170-174, evaluator.rs:439-444)

parse_command() dispatches to parse_if/parse_for/parse_while, which get accepted as pipeline elements by parse_pipeline. But the evaluator's execute_pipeline only handles Ast::Command — everything else returns Err("pipeline elements must be commands"). Scripts like if true; then echo a; fi | cat parse successfully but fail at execution.

Fix: restrict parse_pipeline to only accept Token::Word/LiteralWord entries (i.e., call parse_simple_command instead of parse_command), or at minimum validate at parse time and return an error.

2. Non-Unix builtins with redirections silently try external execution (evaluator.rs:213-255)

The #[cfg(not(unix))] fallback for execute_command_with_redirects doesn't check is_builtin_or_applet — it spawns ProcessCommand::new(name) directly. On Windows, pwd > /dev/null will try to exec a system pwd binary. The Unix path (lines 108-178) correctly does fd save/dup2/restore. This is a real cross-platform gap.

Fix: either add the same builtin dispatch logic to the non-unix path (using std::process::Stdio::from_raw_fd or platform equivalents), or document this as a known limitation with a clear error.

3. peek_at uses O(n) indexing (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the string start on every call. Called on every 2 character (line 102). Not a correctness issue for typical shell inputs, but consider a small lookahead buffer or just self.raw[self.pos..].chars().nth(offset) for O(1) on ASCII.

Rust Idioms

4. Parser clones every token on peek() and advance() (parser.rs:406-418)

Each peek() does .cloned().unwrap_or(Token::Eof) and advance() clones the full token including its String. For large scripts this creates allocation pressure on every token access. Since pos already tracks position, peek() could return &Token and advance() could mem::replace or move out of the vec.

Design (acceptable for initial PR, track as follow-ups)

5. Missing POSIX special variables: $#, $@, $*, $!, $- — only $? is implemented. These are heavily used in scripts.

6. No cd - / OLDPWD/PWD tracking (builtins.rs:67-84): POSIX cd should track previous directory.

7. for var; do (without in) not supported — POSIX allows this to iterate over positional parameters.

8. No here-docs, case, $(...), backticks, functions: Acceptable for v1, but the description should say "POSIX-subset" not "POSIX-compatible" (ish.rs:22).

What's verified correct (contradicting earlier review comments)

• Builtins with redirections work correctly on Unix via fd save/dup2/restore (evaluator.rs:120-178) ✓
• Pipeline dispatches builtins/applets for both last and non-last elements (evaluator.rs:296-309, 337-378) ✓
• $? is implemented (evaluator.rs:559-562) ✓
• 2>> is handled (lexer.rs:105-107) ✓
• \r treated as whitespace (lexer.rs:168, 192) ✓
• Double-quote escapes are POSIX-correct — \<newline> consumed for line continuation, other \X preserved (lexer.rs:248) ✓
• Single ShellApplet struct with name field, no duplication (ish.rs:6-14) ✓
• pub(crate) visibility for internal modules ✓
• REPL prompt uses applet name (ish.rs:65) ✓
• exit with non-numeric arg prints error and returns 2 (builtins.rs:96-98) ✓
• export/alias with no args output sorted (builtins.rs:112-113, 150-151) ✓
• export VAR (no =) sets to empty string if unset (builtins.rs:127-129) ✓
• read uses state.set_var not std::env::set_var (builtins.rs:194) ✓
• & returns clear error message (parser.rs:144-146) ✓
• Shebang handling in run_script (ish.rs:115-116) ✓
• Pipeline zombie cleanup on spawn failure (evaluator.rs:297-299, 330-332, 402-409, 430-438) ✓
• Alias expansion in both pipeline and non-pipeline paths (evaluator.rs:40-51, 288, 166-174) ✓
• Zero new dependencies ✓
• 56 unit + 18 integration tests covering all three aliases ✓

Summary

Severity	#	Issue
Bug	1	Pipeline allows if/for/while as elements, evaluator rejects at runtime
Cross-platform gap	1	Non-Unix: builtins with redirects try external execution
Minor perf	2	peek_at O(n); parser clones every token
Follow-up	5	Missing POSIX features, description accuracy

The code is solid. The two bugs cited most heavily in earlier reviews (builtins+redirects, pipeline dispatch) are actually implemented correctly. The real issue is the parser-evaluator mismatch on pipeline elements (#1) and the Windows fallback (#2). Everything else is acceptable for an initial implementation. Recommend fixing items 1-2 before merge, downgrading description to "POSIX-subset".

opencode session  |  github run

[github-actions]

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

After verifying every claim against the actual source code, most of the issues raised in earlier review comments were incorrect. Here's what the code actually does:

Verified Correct (earlier reviews were wrong)

• Builtins with redirections work on Unix (evaluator.rs:108-211) — saves fds via dup, applies dup2, executes in-process, restores fds.
• Pipeline dispatches builtins/applets (evaluator.rs:296-325, 337-425) — checks is_builtin_or_applet before spawning for both last and non-last elements.
• & returns a clear error (parser.rs:144-146) — "background execution (&) is not supported".
• \r treated as whitespace (lexer.rs:168, 192).
• Double-quote escapes are POSIX-correct — only \\, \", $, `, \<newline> interpreted.
• $? implemented, 2>> handled, read uses state.set_var, exit returns 2 for non-numeric args, export/alias sorted output, shebang handling, single ShellApplet struct, pub(crate) visibility, REPL uses applet name.

Actual Issues

1. cd missing OLDPWD/PWD tracking (builtins.rs:67-85)

POSIX cd should set OLDPWD to the previous directory and PWD to the new one. Also, cd - (return to previous directory) is unsupported.

2. Non-Unix pipeline: non-last builtins not captured (evaluator.rs:397-425)

On Windows, non-last pipeline elements with builtins fall through to external process spawn — the fd-redirection trick is Unix-only. The last-element path handles this, but middle elements don't.

3. Parser clones every token on peek()/advance() (parser.rs:405-417)

Each call clones the token including its String. Use indices + &Token from peek(), clone only in advance() where the value is consumed.

4. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from the start each time. Consider a 2-char lookahead buffer or self.raw[self.pos..].chars().nth(offset).

5. Missing POSIX special variables — $#, $@, $*, $!, $- not implemented (only $?).

6. Description says "POSIX-compatible" (ish.rs:22) — should say "POSIX-subset" (no here-docs, case, $(...), functions, cd -, &).

Minor / Follow-up

• cd builtin missing OLDPWD/PWD tracking and cd - support
• No multi-line REPL — interactive if/for entered line-by-line fails
• for var; do (without in) not supported
• Parser token cloning is allocation-heavy for large scripts
• No here-docs, case, subshells, functions — acceptable for v1

Summary

Severity	#	Issue
Bug	1	cd missing OLDPWD/PWD tracking
Cross-platform gap	1	Non-unix pipeline non-last builtins
Code quality	2	Parser token cloning; peek_at O(n)
Missing features	2	Special variables; description accuracy

The code is solid for an initial implementation. The two most-cited issues across reviews (builtins+redirects, pipeline dispatch) are actually implemented correctly. Fix the cd tracking and consider the parser optimization. Downgrade description to "POSIX-subset."

opencode session  |  github run

[github-actions]

Now I have the full picture. Here's my consolidated review.

---

Review: feat(shell): ish/sh/ash POSIX shell interpreter

Bugs

1. is_builtin_or_applet has side effects — executes commands with empty args (evaluator.rs:273-286)

fn is_builtin_or_applet(&self, name: &str) -> bool {
    // ...
    match self.dispatcher.dispatch(name, &[]) {  // <-- side effect!
        Ok(_) => true,
        Err(e) => !e.to_string().contains("applet not found"),
    }
}

This is called at line 353 for non-last pipeline elements to decide the execution path. self.dispatcher.dispatch("echo", &[]) actually runs echo with empty args, printing a spurious empty line to stdout before the real execution. Any applet that produces output or modifies state will misbehave when checked this way.

Fix: replace with a name-only check, e.g. iterate dispatcher.applet_names() or add an is_known_applet(name) method.

2. Non-Unix execute_command_with_redirects ignores redirections for builtins/applets (evaluator.rs:224-236)

On Windows, pwd > /dev/null runs the builtin without applying any redirection — output goes to stdout. The Unix path correctly does dup/dup2/restore. Either document this as a known limitation or add a warning when builtins are invoked with redirects on non-Unix.

3. Non-Unix pipeline: non-last builtins fall through to external spawn (evaluator.rs:397-425)

On Windows, echo hello | grep hello spawns echo as an external process for the non-last element instead of using in-process dispatch. The Unix path uses temp-file capture. Acceptable for v1 but should be documented.

Significant Issues

4. cd missing OLDPWD/PWD tracking and cd - (builtins.rs:67-84)

POSIX cd should set OLDPWD to the previous directory and PWD to the new one. cd - (return to previous directory) is unsupported. This breaks scripts that use cd /some/dir; cd -.

5. Pipeline only captures Ast::Command nodes (evaluator.rs:300, 455-459)

The parser restricts pipeline elements to simple commands (parse_pipeline calls parse_simple_command), so this isn't a runtime surprise. However, the evaluator's if let Ast::Command(cmd) check + fallback Err("pipeline elements must be commands") is dead code — worth removing or adding an assertion for clarity.

6. is_builtin_or_applet uses a hardcoded name list (evaluator.rs:274-278, 369-377)

The builtin names are duplicated between is_builtin_or_applet and the pipeline's matches! check. If a new builtin is added, both lists need updating. Extract a constant or method.

Code Quality

7. Parser clones every token on peek() and advance() (parser.rs:405-417)

peek() clones the token including its String. Since pos already tracks position, peek() could return &Token and advance() could move out of the vec. Acceptable for v1 but allocation-heavy for large scripts.

8. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from string start. Called on every 2 character. Consider a 2-char lookahead buffer.

Minor

• Description says "POSIX-compatible" (ish.rs:22, ish.rs:45) — should say "POSIX-subset" (no here-docs, case, $(...), functions, cd -, $@/$#).
• mod.rs:3 uses pub mod ish — could be pub(crate) to match other submodules.
• pipeline field on Pipeline is Vec<Ast> but can only contain Ast::Command — consider a Vec<Command> type.

What's Verified Correct (contradicting earlier review comments)

• Builtins with redirections work on Unix via fd save/dup2/restore ✓
• Pipeline dispatches builtins/applets for both last and non-last elements ✓
• $? is implemented ✓
• 2>> is handled ✓
• \r treated as whitespace ✓
• Double-quote escapes are POSIX-correct ✓
• Single ShellApplet struct with name field ✓
• pub(crate) visibility for internal modules ✓
• REPL prompt uses applet name ✓
• exit with non-numeric arg returns 2 ✓
• export/alias output is sorted ✓
• export VAR (no =) sets to empty string if unset ✓
• read uses state.set_var ✓
• & returns clear error ✓
• Shebang handling ✓
• Pipeline zombie cleanup on spawn failure ✓
• 56 unit + 20 integration tests ✓

Summary

Severity	#	Issue
Bug	1	is_builtin_or_applet has side effects (executes applet with empty args)
Cross-platform gap	2	Non-Unix: builtins+redirects ignored; non-last pipeline builtins
Design gap	1	cd missing OLDPWD/PWD tracking
Code quality	3	Parser token cloning; peek_at O(n); hardcoded builtin name lists
Minor	3	Description accuracy; pub vs pub(crate); pipeline type restriction

Fix item 1 before merge — the side-effect bug causes spurious output in pipelines. The rest are acceptable follow-ups.

opencode session  |  github run

[github-actions]

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

Bug (must fix)

1. is_builtin_or_applet has side effects — executes applets with empty args (evaluator.rs:273-286)

fn is_builtin_or_applet(&self, name: &str) -> bool {
    // ...
    match self.dispatcher.dispatch(name, &[]) {  // <-- actually runs the applet!
        Ok(_) => true,
        Err(e) => !e.to_string().contains("applet not found"),
    }
}

This is called at line 338 for non-last pipeline elements to decide execution path. self.dispatcher.dispatch("echo", &[]) runs echo with empty args, printing a spurious empty line to stdout. Any pipeline like echo hello | cat triggers echo twice: once during the check, once during actual execution.

Fix: add is_known_applet(name: &str) -> bool to Dispatcher that only checks APPLETS.iter().any(|e| e.applet.name() == name) without calling dispatch.

Cross-platform gap

2. Non-Unix execute_command_with_redirects drops redirects for builtins (evaluator.rs:224-236)

On Windows, builtins execute without applying any fd redirection — pwd > /dev/null prints to stdout. The Unix path correctly does fd save/dup2/restore. Either add the same logic for Windows (not feasible without dup2) or emit an error when builtins have redirects on non-Unix.

Should fix

3. cd missing OLDPWD/PWD tracking (builtins.rs:67-84)

POSIX cd sets OLDPWD to the previous directory and PWD to the new one. cd - (return to previous) is unsupported. This is a core POSIX shell feature.

4. Pipeline elements restricted to Ast::Command but parser allows others (parser.rs:163-176, evaluator.rs:222-227)

parse_pipeline calls parse_simple_command, which correctly restricts to simple commands. The evaluator's error path "pipeline elements must be commands" is dead code. Minor cleanup, not a bug.

Code quality

5. Parser clones every token on peek() and advance() (parser.rs:405-417)

peek() does .cloned().unwrap_or(Token::Eof) and advance() clones the token including its String. Since pos tracks position, peek() could return &Token and advance() could use mem::replace or move out. Acceptable for v1 but allocation-heavy for large scripts.

6. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from string start. Called on every 2 character (line 102). A 2-char lookahead buffer would be cleaner.

Description accuracy

7. PR says "POSIX-compatible" (ish.rs:22) — the shell lacks here-docs (<<), case, $(...), backticks, functions, $@/$#/$*, cd -, for var; do (without in). Recommend "POSIX-subset shell interpreter."

What's verified correct (contradicting earlier reviews)

• Builtins with redirections work on Unix via fd save/dup2/restore (evaluator.rs:108-178) ✓
• Pipeline dispatches builtins/applets for last and non-last elements (evaluator.rs:296-325, 337-425) ✓
• $? implemented (evaluator.rs:559-562) ✓
• 2>> handled (lexer.rs:105-107) ✓
• \r treated as whitespace (lexer.rs:168, 192) ✓
• Double-quote escapes POSIX-correct (lexer.rs:248) ✓
• Single ShellApplet struct with name field ✓
• pub(crate) visibility for internal modules ✓
• REPL prompt uses applet name (ish.rs:65) ✓
• exit with non-numeric arg returns 2 (builtins.rs:96-98) ✓
• export/alias output sorted (builtins.rs:112-113, 150-151) ✓
• export VAR sets to empty string if unset (builtins.rs:127-129) ✓
• read uses state.set_var (builtins.rs:194) ✓
• & returns clear error (parser.rs:144-146) ✓
• Shebang handling (ish.rs:115-116) ✓
• Pipeline zombie cleanup on spawn failure (evaluator.rs:297-299, 330-332) ✓
• Zero new dependencies, 56 unit + 18 integration tests ✓

Summary

Severity	#	Issue
Bug	1	is_builtin_or_applet executes applets as side effect
Cross-platform	1	Non-Unix: builtins with redirects silently drop them
Should fix	1	cd missing OLDPWD/PWD tracking and cd -
Code quality	2	Parser token cloning; peek_at O(n)
Description	1	"POSIX-compatible" should be "POSIX-subset"

Fix item 1 before merge — the side-effect bug causes spurious output in pipelines. The rest are acceptable follow-ups.

opencode session  |  github run

[github-actions]

Now I have the full picture. Here's my review:

---

PR Review: feat(shell): implement ish/sh/ash POSIX shell interpreter

This is a solid initial shell implementation. Clean architecture, good test coverage, and most of the earlier review claims about bugs were incorrect after verifying the actual code. Here's what actually matters.

Must-Fix

1. Non-Unix pipeline: non-last builtins silently run as external processes (evaluator.rs:393-421)

The #[cfg(not(unix))] fallback for non-last pipeline elements skips is_builtin_or_applet and always spawns an external process. On Windows, echo hello | cat would try to exec a system echo binary for the first element. The Unix path correctly uses temp-file capture with fd redirection. Either add equivalent logic for non-Unix, or at minimum check is_builtin_or_applet and return a clear error for non-last builtins on non-Unix platforms.

2. cd missing OLDPWD/PWD tracking and cd - (builtins.rs:67-84)

POSIX cd must set OLDPWD to the previous directory and PWD to the new one. cd - (return to previous directory) is unsupported. These are core POSIX features that scripts rely on:

cd /tmp
cd -          # should return to /tmp
echo $OLDPWD  # should be /tmp

Should-Fix

3. Missing POSIX special variables (evaluator.rs:550-597)

Only $? is handled. $#, $@, $*, $0-$9, $- expand to empty. $@ and $# are heavily used in scripts. At minimum add $#, $@, $*, and $0.

4. & silently produces a parse error (parser.rs:144-146)

The error message "background execution (&) is not supported" is correct, but it's returned as a parse error which is confusing. The user typed valid shell syntax; this should be an evaluation-time error or at minimum a distinct error type. Also worth considering: just silently ignoring & (running synchronously) may be more practical for a lightweight shell than breaking on it.

5. Description says "POSIX-compatible" (ish.rs:22, 45)

The shell lacks: here-docs (<<), case, $(...), backticks, functions, $@/$#/$*, cd -, OLDPWD/PWD tracking, background execution, for var; do (without in). Recommend: "POSIX-subset shell interpreter".

Code Quality

6. Parser clones every token on peek()/advance() (parser.rs:405-417)

peek() does .cloned().unwrap_or(Token::Eof) and advance() clones the token including its String. Since pos already tracks position, peek() could return &Token and advance() could move out of the vec. Acceptable for v1 but allocation-heavy for large scripts.

7. peek_at is O(n) per call (lexer.rs:154-156)

self.raw.chars().nth(self.pos + offset) scans from string start each call. Called on every 2 character. Consider a 2-char lookahead buffer.

Verified Correct (earlier reviews were wrong)

• Builtins with redirections work on Unix via fd save/dup2/restore (evaluator.rs:120-178)
• Pipeline dispatches builtins/applets for both last and non-last elements
• is_builtin_or_applet is side-effect-free — calls applet_names(), not dispatch()
• $? is implemented
• 2>> is handled
• \r treated as whitespace
• Double-quote escapes are POSIX-correct
• Single ShellApplet struct with name field
• pub(crate) visibility for internal modules
• REPL prompt uses applet name
• exit with non-numeric arg returns 2
• export/alias output is sorted
• export VAR sets to empty string if unset
• read uses state.set_var
• Shebang handling
• Pipeline zombie cleanup on spawn failure
• 56 unit tests + 18 integration tests

Summary

Severity	#	Issue
Must-fix	1	Non-Unix pipeline: non-last builtins skip dispatch
Must-fix	1	cd missing OLDPWD/PWD tracking and cd -
Should-fix	3	Missing $@/$#; & error UX; description accuracy
Code quality	2	Parser token cloning; peek_at O(n)

The code is well-structured and the core shell functionality works correctly. Fix items 1-2 and downgrade the description to "POSIX-subset" before merge. The rest are reasonable follow-ups.

opencode session  |  github run