Implement shell I/O redirection runtime#890
Conversation
Add the redirect module that applies parsed Redirect AST nodes to the process fd table at runtime, conforming to POSIX.1-2024 section 2.7. Implemented: - Output redirection: > (truncate), >> (append) - Input redirection: < - Read-write redirection: <> - Fd-specific redirections: N>, N<, N>> - Fd duplication: >&N, <&N, >&- (close fd) - Here-documents via pipe: <<DELIM body materialized through pipe() - Tab stripping for <<- variant (strip_heredoc_tabs) - SavedFds for undoing redirections (builtin support) - Left-to-right application order per POSIX Runtime uses extern C declarations for open/close/dup/dup2/pipe/write/ fcntl, provided by vibix_libc through the std toolchain. Pure logic (fd resolution, flag computation, dup target parsing, heredoc tab stripping) is fully tested via 37 host-side unit tests. Closes #879 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new internal module ChangesShell I/O Redirection Runtime
Sequence DiagramsequenceDiagram
actor Caller
participant apply_redirects
participant save_fd
participant open_syscall as open()
participant dup2_syscall as dup2()
participant apply_heredoc
participant pipe_syscall as pipe()
participant write_syscall as write()
Caller->>apply_redirects: apply_redirects(&[Redirect])
activate apply_redirects
loop each redirect (left-to-right)
apply_redirects->>apply_redirects: resolve_fd()
alt file redirection
apply_redirects->>save_fd: save_fd(target_fd)
save_fd->>dup (syscall): dup(target_fd)
save_fd->>save_fd: set FD_CLOEXEC
save_fd-->>apply_redirects: saved copy
apply_redirects->>open_syscall: open(path, flags, mode)
open_syscall-->>apply_redirects: opened_fd
apply_redirects->>dup2_syscall: dup2(opened_fd, target_fd)
else dup/close redirection
apply_redirects->>apply_redirects: parse_dup_target()
apply_redirects->>dup2_syscall: dup2(source_fd, target_fd) or close(target_fd)
else here-doc
apply_redirects->>save_fd: save_fd(target_fd)
apply_redirects->>apply_heredoc: apply_heredoc(target_fd, body)
apply_heredoc->>pipe_syscall: pipe()
pipe_syscall-->>apply_heredoc: r,w fds
apply_heredoc->>write_syscall: write(w_fd, body)
apply_heredoc->>dup2_syscall: dup2(r_fd, target_fd)
end
end
apply_redirects-->>Caller: SavedFds
deactivate apply_redirects
Caller->>SavedFds: restore()
activate SavedFds
loop reverse saved entries
SavedFds->>dup2_syscall: dup2(saved_copy, original_fd)
SavedFds->>SavedFds: close(saved_copy)
end
SavedFds-->>Caller: Ok
deactivate SavedFds
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base/sh/src/redirect.rs (1)
531-537: 💤 Low valueConsider rejecting negative fd numbers in
parse_dup_target.The test documents that
"-1"parses asDupTarget::Fd(-1), which will fail at runtime with EBADF. While this is caught at runtime, validating upfront provides clearer error messages to users (e.g., "invalid fd target: -1" instead of a cryptic "dup2(-1, 2) failed: errno 9").💡 Optional: Reject negative fds during parsing
pub fn parse_dup_target(target: &str) -> Result<DupTarget, RedirectError> { if target == "-" { Ok(DupTarget::Close) } else { target - .parse::<RawFd>() - .map(DupTarget::Fd) + .parse::<u32>() + .map(|n| DupTarget::Fd(n as RawFd)) .map_err(|_| RedirectError::InvalidFdTarget { target: target.to_string(), }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/sh/src/redirect.rs` around lines 531 - 537, The test and comment indicate parse_dup_target currently accepts negative integers and returns DupTarget::Fd(-1); change parse_dup_target to validate parsed integers and reject negative fd numbers by returning an Err with a clear message (e.g., "invalid fd target: -1") instead of producing DupTarget::Fd(-1), update any tests that expect DupTarget::Fd(-1) to assert an error, and ensure the error path is produced from the same function (parse_dup_target) so callers get a user-friendly parse error rather than an EBADF at dup time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/redirect.rs`:
- Around line 229-236: strip_heredoc_tabs currently uses body.lines() which
drops a trailing newline; update strip_heredoc_tabs to preserve the final
newline by either iterating with body.split_inclusive('\n') and trimming leading
tabs on each chunk (so existing newlines are kept), or keep the current lines()
approach but append a '\n' to the joined result when body.ends_with('\n'); apply
this change inside the strip_heredoc_tabs function to ensure here-document
bodies that end with a newline still end with a newline after processing.
---
Nitpick comments:
In `@base/sh/src/redirect.rs`:
- Around line 531-537: The test and comment indicate parse_dup_target currently
accepts negative integers and returns DupTarget::Fd(-1); change parse_dup_target
to validate parsed integers and reject negative fd numbers by returning an Err
with a clear message (e.g., "invalid fd target: -1") instead of producing
DupTarget::Fd(-1), update any tests that expect DupTarget::Fd(-1) to assert an
error, and ensure the error path is produced from the same function
(parse_dup_target) so callers get a user-friendly parse error rather than an
EBADF at dup time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44d8a868-d407-4df4-beb3-14ffbf0885ac
📒 Files selected for processing (2)
base/sh/src/main.rsbase/sh/src/redirect.rs
Address CodeRabbit review finding — body.lines() drops a trailing newline, which matters for here-document bodies. Append '\n' back when the input ended with one. Add two tests covering both cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the trailing-newline bug in |
Closes #879
Summary
base/sh/src/redirect.rsimplementing POSIX.1-2024 §2.7 I/O redirection runtime>,>>,<,<>,N>,N<,N>>,>&N,<&N,>&-,<<(here-docs), and<<-(tab-stripping here-docs)SavedFdsfor undoing redirections on builtinsopen/close/dup/dup2/pipe/write/fcntlprovided by vibix_libc through the std toolchainTest plan
cargo testinbase/sh/— 218 tests pass (37 new redirect tests)cargo xtask build— clean buildcargo xtask test— passes (only pre-existingrwlock_concurrent_readersflake)