Conversation
- Remove dead_code gate from build_userspace_sh() in xtask - Fix Atomic::<u32>::new(0) ambiguity in std thread/vibix.rs (E0034) - Add C-ABI syscall shims to sh (fork, pipe, dup2, execve, etc.) using raw inline asm to avoid double-linking vibix_abi - Extend ext2_image::build() with build_with_extras() to install extra binaries (e.g. /bin/sh) alongside /init - Update init to fork+exec /bin/sh after the hello cycle - Add "init: launching /bin/sh" smoke marker - Add `cargo xtask sh` integration test subcommand - Expand vibix_libc with missing POSIX symbols (fork, wait4, kill, dup, dup2, pipe, access, setpgid, fcntl, sigaction) Closes #883 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements userspace shell binary launch and integration. It adds C-ABI syscall shims to enable the shell binary to call POSIX functions, extends the vibix libc with process/signal operations, modifies init to fork and exec ChangesShell Launch Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/syscalls.rs`:
- Around line 186-188: The syscall wrapper sigaction currently calls raw3 with
SYS_SIGACTION but must call the 4-argument rt_sigaction; change sigaction to
call raw4 (or the 4-arg syscall helper) and pass the correct fourth argument:
the sigset size (the size of sigset_t on x86_64, e.g. sizeof(sigset_t)/8 bytes
as used elsewhere). Update the call site referenced by sigaction (replace
raw3(SYS_SIGACTION, signum as u64, act as u64, oldact as u64) with the 4-arg
variant including the sigsetsize) so job::install_interactive_signals() can
succeed.
In `@base/vibix_libc/src/signal.rs`:
- Around line 7-17: The rt_sigaction syscall (SYS_SIGACTION) requires a fourth
sigsetsize argument; update the sigaction function to pass the sigset size as
the fourth parameter (e.g., compute sigsetsize =
core::mem::size_of::<sigset_t>() or the equivalent byte size for the platform,
cast to the syscall integer type) when calling syscall!(SYS_SIGACTION, signum,
act, oldact, sigsetsize), keeping the rest of the function (including
syscall_ret) unchanged.
In `@xtask/src/main.rs`:
- Around line 2865-2997: The test currently treats seeing SUCCESS_MARKER ("init:
launching /bin/sh") as enough; instead, after detecting SUCCESS_MARKER you must
drive the spawned QEMU's stdin to verify a real shell is running: when building
the Command in spawn, set .stdin(Stdio::piped()) and capture child.stdin (e.g.
let mut stdin = child.stdin.take().ok_or("no stdin pipe")?); then in the main
recv loop (where you match rx.recv_timeout) do not set success = true
immediately on SUCCESS_MARKER — write a short probe like "echo hello\n" to stdin
and wait (within HARD_CAP or a shorter per-probe timeout) for the probe output
"hello" on rx before setting success; update failure cases to include probe
timeouts and keep using TICK, HARD_CAP, SUCCESS_MARKER, PANIC_MARKER, rx, tx,
child, reader_handle to locate where to change behavior.
- Around line 1743-1752: The current code always builds and attaches a
deterministic ext2 image (via build_userspace_init, build_userspace_sh and
ext2_image::build_with_extras) even when no --root is provided; change the flow
so the ext2 image is only constructed and passed to run when the user explicitly
requested the ext2/root path (i.e., when the --root option or equivalent flag is
present). Locate the block that calls build_userspace_init(),
build_userspace_sh() and ext2_image::build_with_extras() and guard it with the
condition used to detect the ext2/root mode so the non-ext2 "plain cargo xtask
run" path continues to run without attaching the ext2 image.
🪄 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: 93e6e145-d427-452e-9d2b-4a9400efe666
📒 Files selected for processing (11)
base/init/src/main.rsbase/sh/src/main.rsbase/sh/src/syscalls.rsbase/vibix_libc/src/fcntl.rsbase/vibix_libc/src/lib.rsbase/vibix_libc/src/signal.rsbase/vibix_libc/src/unistd.rslibrary/std/src/sys/thread/vibix.rstests/fixtures/ext2_image.sha256xtask/src/ext2_image.rsxtask/src/main.rs
| #[no_mangle] | ||
| pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 { | ||
| unsafe { cvt(raw3(SYS_SIGACTION, signum as u64, act as u64, oldact as u64)) as i32 } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Linux x86_64, what syscall does number 13 map to, what arguments does rt_sigaction take, and what sigsetsize value is expected?
💡 Result:
On Linux x86_64, syscall number 13 maps to rt_sigaction. [1] Arguments taken by rt_sigaction (in order): 1) int signum 2) const struct sigaction *act 3) struct sigaction *oldact 4) size_t sigsetsize (These correspond to the documented parameter list for the system call.) [2][3] Expected sigsetsize value: the argument is currently required to have the value sizeof(sigset_t); otherwise the kernel returns EINVAL. [2]
Citations:
- 1: https://chromium.googlesource.com/chromiumos/docs/+/master/constants/syscalls.md
- 2: http://www.man7.org/linux/man-pages/man2/rt_sigaction.2.html
- 3: https://aquasecurity.github.io/tracee/v0.16/docs/events/builtin/syscalls/rt_sigaction/
🏁 Script executed:
# Check SYS_SIGACTION value and verify raw4 exists
cd base/sh/src && head -50 syscalls.rs | grep -E "SYS_SIGACTION|raw"Repository: dburkart/vibix
Length of output: 264
🏁 Script executed:
# Get the full syscall number definitions and raw helpers
cd base/sh/src && sed -n '1,150p' syscalls.rs | grep -E "SYS_SIGACTION|fn raw[0-9]"Repository: dburkart/vibix
Length of output: 344
🏁 Script executed:
# Check the sigaction function implementation
cd base/sh/src && sed -n '180,195p' syscalls.rsRepository: dburkart/vibix
Length of output: 533
🏁 Script executed:
# Verify job::install_interactive_signals() location in main.rs
cd base/sh/src && grep -n "install_interactive_signals" main.rsRepository: dburkart/vibix
Length of output: 105
Pass the required fourth argument to rt_sigaction.
Line 187 is issuing syscall 13 via raw3, but on x86_64 that entry point is rt_sigaction, which takes a fourth sigsetsize argument. Without it, job::install_interactive_signals() in base/sh/src/main.rs Line 69 will fail with EINVAL, preventing the shell from installing its SIGINT/SIGTSTP handlers.
Suggested fix
#[no_mangle]
pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 {
- unsafe { cvt(raw3(SYS_SIGACTION, signum as u64, act as u64, oldact as u64)) as i32 }
+ const KERNEL_SIGSET_SIZE: u64 = core::mem::size_of::<u64>() as u64;
+ unsafe {
+ cvt(raw4(
+ SYS_SIGACTION,
+ signum as u64,
+ act as u64,
+ oldact as u64,
+ KERNEL_SIGSET_SIZE,
+ )) as i32
+ }
}📝 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.
| #[no_mangle] | |
| pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 { | |
| unsafe { cvt(raw3(SYS_SIGACTION, signum as u64, act as u64, oldact as u64)) as i32 } | |
| #[no_mangle] | |
| pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 { | |
| const KERNEL_SIGSET_SIZE: u64 = core::mem::size_of::<u64>() as u64; | |
| unsafe { | |
| cvt(raw4( | |
| SYS_SIGACTION, | |
| signum as u64, | |
| act as u64, | |
| oldact as u64, | |
| KERNEL_SIGSET_SIZE, | |
| )) as i32 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/sh/src/syscalls.rs` around lines 186 - 188, The syscall wrapper
sigaction currently calls raw3 with SYS_SIGACTION but must call the 4-argument
rt_sigaction; change sigaction to call raw4 (or the 4-arg syscall helper) and
pass the correct fourth argument: the sigset size (the size of sigset_t on
x86_64, e.g. sizeof(sigset_t)/8 bytes as used elsewhere). Update the call site
referenced by sigaction (replace raw3(SYS_SIGACTION, signum as u64, act as u64,
oldact as u64) with the 4-arg variant including the sigsetsize) so
job::install_interactive_signals() can succeed.
| const SYS_SIGACTION: u64 = 13; | ||
| const SYS_KILL: u64 = 62; | ||
|
|
||
| /// Install or query a signal handler. | ||
| /// | ||
| /// # Safety | ||
| /// `act` and `oldact` must be valid pointers to `sigaction` structs (or null). | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 { | ||
| let ret = syscall!(SYS_SIGACTION, signum, act, oldact); | ||
| syscall_ret(ret) as i32 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Linux x86_64 syscall 13 rt_sigaction
💡 Result:
On Linux x86_64, syscall number 13 is rt_sigaction, which examines and/or changes the action taken by a process when it receives a specific signal [1][2][3][4]. This replaced the older sigaction syscall to support larger sigset_t for real-time signals added in Linux 2.2 [1][5].
Citations:
- 1: https://man7.org/linux/man-pages/man2/rt_sigaction.2.html
- 2: https://filippo.io/linux-syscall-table/
- 3: https://blog.rchapman.org/posts/Linux_System_Call_Table_for_x86_64/
- 4: https://github.com/torvalds/linux/blob/master/arch/x86/entry/syscalls/syscall_64.tbl
- 5: https://man.archlinux.org/man/rt_sigaction.2.en
🏁 Script executed:
# Check if there's any syscall table or documentation in the repo
fd -i syscall . -type f | head -20Repository: dburkart/vibix
Length of output: 229
🌐 Web query:
rt_sigaction syscall signature x86_64 sigsetsize argument
💡 Result:
rt_sigaction (x86_64 Linux) system call signature: - syscall number: 13 [1] - C/kernel signature (argument order): long sys_rt_sigaction(int sig, const struct sigaction *act, struct sigaction *oact, size_t sigsetsize) [2] What the sigsetsize argument is (and what value it must be): - sigsetsize is the size in bytes of the signal sets in act.sa_mask and oldact.sa_mask [2][3][4]. - For correctness, the man page states this argument is currently required to be sizeof(sigset_t); otherwise the kernel returns EINVAL [2][3][4]. Practical note for x86_64 callers: - “sigsetsize” should be sizeof(sigset_t) from your C library headers/ABI, because rt_sigaction expects the kernel to receive that exact size in bytes [2][3][4].
Citations:
- 1: https://cigix.me/syscalls
- 2: https://www.bczl.xyz/linux/doc/man-pages/man2/rt_sigaction.2.html
- 3: https://man7.org/linux/man-pages/man2/rt_sigaction.2.html
- 4: https://manpages.ubuntu.com/manpages/jammy/en/man2/sigaction.2.html
Add the required sigsetsize argument to the rt_sigaction syscall.
Syscall 13 on Linux x86_64 is rt_sigaction, which requires four arguments: signum, act, oldact, and sigsetsize. The current implementation passes only three arguments, causing the kernel to reject the call with EINVAL. This will cause signal handling to fail.
-const SYS_SIGACTION: u64 = 13;
+const SYS_RT_SIGACTION: u64 = 13;
+const SIGSET_SIZE: usize = 8;
/// Install or query a signal handler.
///
/// # Safety
/// `act` and `oldact` must be valid pointers to `sigaction` structs (or null).
#[no_mangle]
pub unsafe extern "C" fn sigaction(signum: i32, act: *const u8, oldact: *mut u8) -> i32 {
- let ret = syscall!(SYS_SIGACTION, signum, act, oldact);
+ let ret = syscall!(SYS_RT_SIGACTION, signum, act, oldact, SIGSET_SIZE);
syscall_ret(ret) as i32
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/vibix_libc/src/signal.rs` around lines 7 - 17, The rt_sigaction syscall
(SYS_SIGACTION) requires a fourth sigsetsize argument; update the sigaction
function to pass the sigset size as the fourth parameter (e.g., compute
sigsetsize = core::mem::size_of::<sigset_t>() or the equivalent byte size for
the platform, cast to the syscall integer type) when calling
syscall!(SYS_SIGACTION, signum, act, oldact, sigsetsize), keeping the rest of
the function (including syscall_ret) unchanged.
| let init_bin = build_userspace_init()?; | ||
| let img = ext2_image::build(&workspace_root(), Some(&init_bin), true)?; | ||
| let sh_bin = build_userspace_sh()?; | ||
| let extras: Vec<(&Path, &str)> = vec![(&sh_bin, "/bin/sh")]; | ||
| let img = ext2_image::build_with_extras( | ||
| &workspace_root(), | ||
| Some(&init_bin), | ||
| &extras, | ||
| true, | ||
| )?; | ||
| (img, Vec::new()) |
There was a problem hiding this comment.
Keep plain cargo xtask run on the non-ext2 path.
Lines 1743-1752 now attach the deterministic ext2 image even when --root is absent. That silently changes the default run behavior away from the documented scratch-disk / auto-root path and can hide regressions that only reproduce outside the ext2-root flow.
Suggested fix
None => {
- let init_bin = build_userspace_init()?;
- let sh_bin = build_userspace_sh()?;
- let extras: Vec<(&Path, &str)> = vec![(&sh_bin, "/bin/sh")];
- let img = ext2_image::build_with_extras(
- &workspace_root(),
- Some(&init_bin),
- &extras,
- true,
- )?;
- (img, Vec::new())
+ (ensure_test_disk()?, Vec::new())
}🤖 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 `@xtask/src/main.rs` around lines 1743 - 1752, The current code always builds
and attaches a deterministic ext2 image (via build_userspace_init,
build_userspace_sh and ext2_image::build_with_extras) even when no --root is
provided; change the flow so the ext2 image is only constructed and passed to
run when the user explicitly requested the ext2/root path (i.e., when the --root
option or equivalent flag is present). Locate the block that calls
build_userspace_init(), build_userspace_sh() and ext2_image::build_with_extras()
and guard it with the condition used to detect the ext2/root mode so the
non-ext2 "plain cargo xtask run" path continues to run without attaching the
ext2 image.
| const HARD_CAP: Duration = Duration::from_secs(120); | ||
| const SUCCESS_MARKER: &str = "init: launching /bin/sh"; | ||
| const PANIC_MARKER: &str = "KERNEL PANIC:"; | ||
|
|
||
| let kernel = build(opts)?; | ||
| let init_bin = build_userspace_init()?; | ||
| let sh_bin = build_userspace_sh()?; | ||
|
|
||
| // Install init as /init and sh as /bin/sh in the ext2 rootfs image. | ||
| let extras: Vec<(&Path, &str)> = vec![(&sh_bin, "/bin/sh")]; | ||
| let disk = ext2_image::build_with_extras( | ||
| &workspace_root(), | ||
| Some(&init_bin), | ||
| &extras, | ||
| true, | ||
| )?; | ||
| let iso = workspace_root().join("target").join("vibix-sh.iso"); | ||
| make_iso_with_cmdline(&kernel, &iso, "iso_sh", "root=/dev/vda")?; | ||
|
|
||
| let mut child = Command::new("qemu-system-x86_64") | ||
| .args([ | ||
| "-M", | ||
| "q35", | ||
| "-cpu", | ||
| "max", | ||
| "-m", | ||
| "256M", | ||
| "-serial", | ||
| "stdio", | ||
| "-display", | ||
| "none", | ||
| "-no-reboot", | ||
| "-no-shutdown", | ||
| "-device", | ||
| "isa-debug-exit,iobase=0xf4,iosize=0x04", | ||
| ]) | ||
| .args(virtio_blk_args(&disk)) | ||
| .arg("-cdrom") | ||
| .arg(&iso) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::null()) | ||
| .spawn()?; | ||
|
|
||
| let pid = child.id(); | ||
| let stdout = child.stdout.take().ok_or("no stdout pipe")?; | ||
|
|
||
| let (cancel_tx, cancel_rx) = std::sync::mpsc::channel::<()>(); | ||
| let hard_pid = pid; | ||
| let hard_watchdog = std::thread::spawn(move || { | ||
| if let Err(std::sync::mpsc::RecvTimeoutError::Timeout) = cancel_rx.recv_timeout(HARD_CAP) { | ||
| let _ = Command::new("kill").arg(hard_pid.to_string()).status(); | ||
| } | ||
| }); | ||
|
|
||
| let (tx, rx) = std::sync::mpsc::channel::<String>(); | ||
| let reader_handle = std::thread::spawn(move || { | ||
| let mut reader = std::io::BufReader::new(stdout); | ||
| let mut line = String::new(); | ||
| loop { | ||
| line.clear(); | ||
| match reader.read_line(&mut line) { | ||
| Ok(0) => break, | ||
| Ok(_) => { | ||
| if tx.send(line.clone()).is_err() { | ||
| break; | ||
| } | ||
| } | ||
| Err(_) => break, | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| let start = Instant::now(); | ||
| let mut success = false; | ||
| let mut failure: Option<String> = None; | ||
| let mut tail: VecDeque<String> = VecDeque::with_capacity(64); | ||
| const TICK: Duration = Duration::from_millis(200); | ||
|
|
||
| loop { | ||
| match rx.recv_timeout(TICK) { | ||
| Ok(line) => { | ||
| print!("{line}"); | ||
| if tail.len() == 64 { | ||
| tail.pop_front(); | ||
| } | ||
| tail.push_back(line.clone()); | ||
|
|
||
| if line.contains(PANIC_MARKER) { | ||
| failure = Some(format!("kernel panic: {}", line.trim_end())); | ||
| break; | ||
| } | ||
| if line.contains(SUCCESS_MARKER) { | ||
| success = true; | ||
| break; | ||
| } | ||
| } | ||
| Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { | ||
| if start.elapsed() > HARD_CAP { | ||
| failure = Some(format!("hard cap exceeded ({HARD_CAP:?}) without success")); | ||
| break; | ||
| } | ||
| } | ||
| Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { | ||
| if !success && failure.is_none() { | ||
| failure = Some(format!("QEMU exited before `{SUCCESS_MARKER}` marker")); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let _ = Command::new("kill").arg(pid.to_string()).status(); | ||
| drop(cancel_tx); | ||
| let _ = hard_watchdog.join(); | ||
| let _ = reader_handle.join(); | ||
| let _ = child.wait(); | ||
|
|
||
| match (success, failure) { | ||
| (true, _) => { | ||
| println!("→ sh: `{SUCCESS_MARKER}` in {:?} ✓", start.elapsed()); | ||
| Ok(()) | ||
| } | ||
| (false, Some(msg)) => { | ||
| eprintln!("--- captured serial (tail) ---"); | ||
| for line in &tail { | ||
| eprint!("{line}"); | ||
| } | ||
| eprintln!("------------------------------"); | ||
| Err(format!("sh: {msg}").into()) | ||
| } | ||
| (false, None) => Err("sh: terminated with no success and no failure marker".into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Drive the shell over serial before declaring success.
SUCCESS_MARKER is init: launching /bin/sh, but init prints that before the shell fork/execve happens. This test will still pass if /bin/sh is missing, execve fails immediately, the prompt never appears, or serial stdin is broken. The linked objective was to wait for the prompt and round-trip a command such as echo hello.
🤖 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 `@xtask/src/main.rs` around lines 2865 - 2997, The test currently treats seeing
SUCCESS_MARKER ("init: launching /bin/sh") as enough; instead, after detecting
SUCCESS_MARKER you must drive the spawned QEMU's stdin to verify a real shell is
running: when building the Command in spawn, set .stdin(Stdio::piped()) and
capture child.stdin (e.g. let mut stdin = child.stdin.take().ok_or("no stdin
pipe")?); then in the main recv loop (where you match rx.recv_timeout) do not
set success = true immediately on SUCCESS_MARKER — write a short probe like
"echo hello\n" to stdin and wait (within HARD_CAP or a shorter per-probe
timeout) for the probe output "hello" on rx before setting success; update
failure cases to include probe timeouts and keep using TICK, HARD_CAP,
SUCCESS_MARKER, PANIC_MARKER, rx, tx, child, reader_handle to locate where to
change behavior.
… target The vibix PAL's _start entry point was passing argc=0, argv=null to main(), which meant std::env::args() always returned empty on vibix. This prevented the shell from seeing its -c flag when exec'd by init. - Replace the hardcoded _start function with a global_asm stub that reads the real argc/argv from the SysV AMD64 initial stack layout - Add a vibix-specific args module (sys/args/vibix.rs) that stores and retrieves argc/argv via atomics, matching the pattern used by unix targets - Register the vibix args module in sys/args/mod.rs (before the unix branch) - Forward argc/argv from the PAL init() to the args module - Guard std::env::vars() with #[cfg(not(target_os = "vibix"))] in the shell (the unsupported env module panics on vibix) - Guard mod syscalls with #[cfg(not(test))] so host tests can mock symbols - Set is_tty = true unconditionally (vibix serial is not a POSIX tty) - Update ext2 image hash for the rebuilt rootfs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ext2-image CI check builds the base image without extra binaries. The previous commit incorrectly updated the hash to match a build that included /bin/sh. Restore the hash to match the base image. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/main.rs`:
- Around line 7-8: The runtime shims module is currently exposed to all non-test
builds via #[cfg(not(test))] and should be limited to vibix target builds;
update the cfg on the mod syscalls declaration (the mod syscalls in
base/sh/src/main.rs) to only compile on the vibix target (for example use a cfg
like all(target_vendor = "vibix", not(test)) — or target_os = "vibix" if your
vibix target is set as the OS) so the raw syscall shims are not pulled into
non-vibix binaries.
- Around line 63-67: The code currently sets let is_tty = true; which forces
interactive mode for all hosts; change it so only vibix targets force
interactive mode by computing is_tty conditionally (e.g. replace the line in
main.rs with: let is_tty = if running_on_vibix() { true } else {
atty::is(atty::Stream::Stdin) };), add a small helper running_on_vibix() that
detects vibix either via a compile-time cfg/feature (cfg!(feature = "vibix") or
cfg!(target_os = "vibix")) or a clear runtime marker like an environment
variable (e.g. VIBIX_SERIAL), and keep references to is_tty and the new
running_on_vibix() helper so only vibix forces interactive behavior while all
other hosts use proper isatty detection.
In `@library/std/src/sys/args/vibix.rs`:
- Line 37: The call to CStr::from_ptr is using ptr which is a *const u8; change
the argument to CStr::from_ptr(ptr.cast()) so the pointer is explicitly cast to
*const c_char (matching wasip1.rs at its use), i.e., locate the statement with
CStr::from_ptr and replace the raw ptr with ptr.cast() to satisfy the expected
*const c_char/*const i8 type.
🪄 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: 4f14152d-7d97-4d44-93d9-c544ecd091c7
📒 Files selected for processing (5)
base/sh/src/main.rslibrary/std/src/sys/args/mod.rslibrary/std/src/sys/args/vibix.rslibrary/std/src/sys/pal/vibix/mod.rstests/fixtures/ext2_image.sha256
| #[cfg(not(test))] | ||
| mod syscalls; |
There was a problem hiding this comment.
Scope syscall shims to vibix-only builds.
#[cfg(not(test))] pulls raw syscall POSIX shims into all non-test binaries. That can unintentionally override host libc symbols and introduce host-specific breakage. Restrict this module to vibix target builds.
Suggested fix
-#[cfg(not(test))]
+#[cfg(all(target_os = "vibix", not(test)))]
mod syscalls;📝 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.
| #[cfg(not(test))] | |
| mod syscalls; | |
| #[cfg(all(target_os = "vibix", not(test)))] | |
| mod syscalls; |
🤖 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/main.rs` around lines 7 - 8, The runtime shims module is
currently exposed to all non-test builds via #[cfg(not(test))] and should be
limited to vibix target builds; update the cfg on the mod syscalls declaration
(the mod syscalls in base/sh/src/main.rs) to only compile on the vibix target
(for example use a cfg like all(target_vendor = "vibix", not(test)) — or
target_os = "vibix" if your vibix target is set as the OS) so the raw syscall
shims are not pulled into non-vibix binaries.
| // On vibix, the serial console is not a POSIX tty, so `isatty()` | ||
| // would return false even for the primary interactive session. | ||
| // Always treat stdin-mode as interactive to ensure the prompt | ||
| // appears over the serial console. | ||
| let is_tty = true; |
There was a problem hiding this comment.
Don’t force interactive mode on all targets.
let is_tty = true; makes non-interactive host stdin sessions behave as interactive (prompt output + interactive signal behavior), which can break pipelines/scripts. Keep forced interactive behavior only for vibix.
Suggested fix
- let is_tty = true;
+ #[cfg(target_os = "vibix")]
+ let is_tty = true;
+ #[cfg(not(target_os = "vibix"))]
+ let is_tty = std::env::var_os("TERM").is_some();📝 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.
| // On vibix, the serial console is not a POSIX tty, so `isatty()` | |
| // would return false even for the primary interactive session. | |
| // Always treat stdin-mode as interactive to ensure the prompt | |
| // appears over the serial console. | |
| let is_tty = true; | |
| // On vibix, the serial console is not a POSIX tty, so `isatty()` | |
| // would return false even for the primary interactive session. | |
| // Always treat stdin-mode as interactive to ensure the prompt | |
| // appears over the serial console. | |
| #[cfg(target_os = "vibix")] | |
| let is_tty = true; | |
| #[cfg(not(target_os = "vibix"))] | |
| let is_tty = std::env::var_os("TERM").is_some(); |
🤖 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/main.rs` around lines 63 - 67, The code currently sets let is_tty
= true; which forces interactive mode for all hosts; change it so only vibix
targets force interactive mode by computing is_tty conditionally (e.g. replace
the line in main.rs with: let is_tty = if running_on_vibix() { true } else {
atty::is(atty::Stream::Stdin) };), add a small helper running_on_vibix() that
detects vibix either via a compile-time cfg/feature (cfg!(feature = "vibix") or
cfg!(target_os = "vibix")) or a clear runtime marker like an environment
variable (e.g. VIBIX_SERIAL), and keep references to is_tty and the new
running_on_vibix() helper so only vibix forces interactive behavior while all
other hosts use proper isatty detection.
Explicitly cast *const u8 to *const c_char for type safety, matching the pattern used by wasip1.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
build_userspace_sh()and fix the pre-existingAtomic::new(0)ambiguity (E0034) in the std fork'sthread/vibix.rsbase/sh/src/syscalls.rs) using raw inline asm, providing POSIX symbols (fork,pipe,dup2,execve,kill,wait4,sigaction, etc.) without double-linkingvibix_abiext2_image::build()with a newbuild_with_extras()variant that installs extra binaries (e.g./bin/sh) alongside/init/bin/shafter the existing hello fork+exec+wait cycleinit: launching /bin/shas a required smoke marker (43 markers total)cargo xtask shintegration test subcommandvibix_libcwith missing POSIX symbols for future useCloses #883
Test plan
cargo xtask buildsucceedscargo xtask smokepasses with all 43 markers (including newinit: launching /bin/sh)cargo test -p xtask-- 71 tests)cargo testinbase/sh/-- 399 tests)