feat(minimald): support arbitrary session exec - #1173
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds session-scoped SSH command execution, shared runtime environment propagation, and Linux namespace injection. It centralizes sandbox command environments, refactors session host launching, adds integration tests, and enables the ChangesSession execution
Build dependency
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SSHClient
participant SessionExec
participant SessionHandle
participant HostHandle
participant NsenterShim
SSHClient->>SessionExec: submit non-min command
SessionExec->>SessionHandle: ensure_host
SessionExec->>HostHandle: command_in_session
HostHandle->>NsenterShim: run with session pidfd and namespaces
NsenterShim-->>SSHClient: return process output and exit status
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
crates/minimald/src/exec.rs (1)
495-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the session shell path instead of redefining it.
SESSION_SHELLis/usr/bin/bash.sandbox2::config::Config::command_envsetsSHELLto the same literal for the session layout. Two crates now hold the same hardcoded path. If the session shell moves, the two values diverge silently, and the injected command runs a shell that does not match$SHELL.Export the path from
sandbox2and use it here.🤖 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 `@crates/minimald/src/exec.rs` around lines 495 - 498, Remove the local SESSION_SHELL definition in the session exec logic and expose the shared shell-path constant from sandbox2::config for external use. Update the session launcher and injected-command setup to reference that exported constant, ensuring command execution and SHELL configuration remain synchronized.crates/minimald/src/session.rs (1)
1329-1334: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate the record read failure instead of unwrapping it.
launch_hostcallsself.record.record().await.unwrap(). A store read error panics the session actor task. Every caller then sees only "session actor terminated", and the session becomes unusable until the daemon restarts.
ensure_hostalready reads the record fallibly at lines 1382-1386 and maps the error toAttachError::LoadoutFailed. Do the same here so a transient store failure returns an error to the caller.♻️ Proposed change
- let record = self.record.record().await.unwrap(); + let record = self + .record + .record() + .await + .map_err(AttachError::LoadoutFailed)?;🤖 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 `@crates/minimald/src/session.rs` around lines 1329 - 1334, Update launch_host to handle self.record.record().await errors without unwrapping, mapping the failure to AttachError::LoadoutFailed consistently with ensure_host. Preserve the existing successful record flow and return the mapped error to the caller so the session actor remains running.crates/sandbox2/src/config.rs (1)
279-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
BoundDirHOME source with the synthesized passwd entry.
Config::build(line 613) derives the synthesized/etc/passwdhome fromstd::env::home_dir().command_envderives it fromstd::env::var("HOME"). If the two disagree, the sandbox user's passwd home does not match$HOMEinside the sandbox. Use one source for both.♻️ Proposed change
- WdSetup::BoundDir { .. } => match std::env::var("HOME") { - Ok(h) => set("HOME", &h), - Err(_) => set("HOME", "/state/home"), - }, + WdSetup::BoundDir { .. } => match std::env::home_dir() + .and_then(|p| p.to_str().map(String::from)) + { + Some(h) => set("HOME", &h), + None => set("HOME", "/state/home"), + },🤖 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 `@crates/sandbox2/src/config.rs` around lines 279 - 286, Update the BoundDir branch in command_env to derive HOME from the same source as Config::build’s synthesized passwd entry, namely std::env::home_dir(), and preserve the existing /state/home fallback when no home directory is available. Remove the separate std::env::var("HOME") lookup so both values remain consistent.
🤖 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 `@crates/minimald/src/exec.rs`:
- Around line 876-893: Redact or avoid recording raw command arguments in the
`tracing::info!` request log and the `exec_span` created in the non-`min`
dispatch path. Reuse the project’s existing command-redaction mechanism if
available; otherwise log only a safe command identifier while preserving session
and dispatch context.
- Around line 516-522: Update the session command lifecycle around Tok ioProcess
and shim_main so dropping the client or shim terminates the in-session process
tree, not just the __nsenter shim. Propagate termination from shim_main to the
spawned child and its descendants, while preserving the existing piped stdio and
kill_on_drop behavior.
In `@crates/minimald/src/lib.rs`:
- Line 13: Gate the `nsenter` module declaration in `lib.rs` with
`#[cfg(target_os = "linux")]`, and apply the same condition to every
`minimald::nsenter` reference in `main.rs`. Ensure non-Linux builds neither
compile nor access the Linux-specific module.
In `@crates/minimald/src/main.rs`:
- Around line 476-485: Update the late-parsed Nsenter arm in async_main’s
command match to return a descriptive error instead of calling unreachable!, and
make the diagnostic state that Nsenter must be intercepted before entering the
daemon runtime. Remove the inaccurate claim that CLI arguments are not parsed
during interception; leave the Completions and other command handling unchanged.
In `@crates/minimald/src/nsenter.rs`:
- Around line 480-485: Validate args.pidfd in shim_main before calling
OwnedFd::from_raw_fd, rejecting values below PIDFD_FD with the proposed
NsenterError::BadPidfd variant. Update the safety comment to state the validated
descriptor invariant and remove the inaccurate claim that arbitrary wrong
descriptors are harmless; preserve adoption only for accepted pidfd values.
---
Nitpick comments:
In `@crates/minimald/src/exec.rs`:
- Around line 495-498: Remove the local SESSION_SHELL definition in the session
exec logic and expose the shared shell-path constant from sandbox2::config for
external use. Update the session launcher and injected-command setup to
reference that exported constant, ensuring command execution and SHELL
configuration remain synchronized.
In `@crates/minimald/src/session.rs`:
- Around line 1329-1334: Update launch_host to handle self.record.record().await
errors without unwrapping, mapping the failure to AttachError::LoadoutFailed
consistently with ensure_host. Preserve the existing successful record flow and
return the mapped error to the caller so the session actor remains running.
In `@crates/sandbox2/src/config.rs`:
- Around line 279-286: Update the BoundDir branch in command_env to derive HOME
from the same source as Config::build’s synthesized passwd entry, namely
std::env::home_dir(), and preserve the existing /state/home fallback when no
home directory is available. Remove the separate std::env::var("HOME") lookup so
both values remain consistent.
🪄 Autofix
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: b1efdc6a-adc2-43cf-976b-0999265902cc
📒 Files selected for processing (11)
Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/exec.rscrates/minimald/src/lib.rscrates/minimald/src/main.rscrates/minimald/src/nsenter.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/tests/nsenter_integration.rscrates/sandbox2/src/config.rscrates/sandbox2/src/lib.rs
twitchyliquid64
force-pushed
the
tom/sftp
branch
from
August 5, 2026 02:01
f289622 to
f3ee61d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/tests/nsenter_integration.rs`:
- Around line 17-22: Update the nsenter integration test documentation near the
module-level command to use the repository’s applicable just recipe instead of
invoking cargo directly, while forwarding MINIMALD_NSENTER_TEST=1 and the
ignored-test arguments. If no suitable recipe exists, add one and reference it
in the command.
- Around line 51-72: Update the sandbox helper around sandbox to return a local
RAII guard owning hakoniwa::Child instead of the raw child; implement Drop for
the guard to call kill and then wait, ensuring the /bin/sleep process is cleaned
up even when tests panic. Preserve the existing process spawning and command
configuration.
🪄 Autofix
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: ed5d2a89-a9f0-45f3-88fe-0425073d365c
📒 Files selected for processing (11)
Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/exec.rscrates/minimald/src/lib.rscrates/minimald/src/main.rscrates/minimald/src/nsenter.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/tests/nsenter_integration.rscrates/sandbox2/src/config.rscrates/sandbox2/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- Cargo.toml
- crates/sandbox2/src/lib.rs
- crates/minimald/src/main.rs
- crates/minimald/src/lib.rs
- crates/sandbox2/src/config.rs
- crates/minimald/src/exec.rs
- crates/minimald/src/session.rs
- crates/minimald/src/session_host.rs
- crates/minimald/src/nsenter.rs
- crates/minimald/src/env.rs
Comment on lines
+17
to
+22
| //! `#[ignore]`, and additionally early-returns unless `MINIMALD_NSENTER_TEST` is | ||
| //! set, so neither a plain `cargo test` run nor the ignored-test sweep attempts | ||
| //! namespace work on a host that may not allow it. Needs unprivileged user | ||
| //! namespaces (no root, unlike the netns proofs), a kernel with | ||
| //! `CONFIG_PROC_CHILDREN`, and `setns` pidfd support (Linux 5.8+): | ||
| //! `MINIMALD_NSENTER_TEST=1 cargo test -p minimald --test nsenter_integration -- --include-ignored` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a just recipe in the test command.
Line 22 directs contributors to run cargo test directly. Replace it with the applicable just recipe that forwards MINIMALD_NSENTER_TEST. Add a recipe if none exists.
As per coding guidelines, build and test only through just; use repository recipes rather than hand-written cargo commands, and add a recipe when none exists.
🤖 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 `@crates/minimald/tests/nsenter_integration.rs` around lines 17 - 22, Update
the nsenter integration test documentation near the module-level command to use
the repository’s applicable just recipe instead of invoking cargo directly,
while forwarding MINIMALD_NSENTER_TEST=1 and the ignored-test arguments. If no
suitable recipe exists, add one and reference it in the command.
Source: Coding guidelines
twitchyliquid64
enabled auto-merge (rebase)
August 5, 2026 02:11
jtnkminimal
approved these changes
Aug 5, 2026
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes it possible to launch arbitrary commands within the session.
min attach <name or id> -c 'pwd'Summary by CodeRabbit
New Features
Bug Fixes
Note
Add arbitrary SSH exec support for commands run inside session namespaces
nsenter-based injection pathway incrates/minimald/src/nsenter.rsthat re-execs theminimaldbinary as a shim tosetnsinto the session's namespaces, then spawns the target program with the correct cwd and environment.minsubcommands are now routed into the session sandbox viaSessionExecincrates/minimald/src/exec.rsinstead of being rejected.SessionGuardtrait andSessionEnvironmentstruct soHostcan read current working directory and environment variables when building in-session commands.SessionHandle::ensure_hostso a sandbox host can be started without attaching a terminal, enabling headless exec.Sandbox::command_cwdandSandbox::command_envfromsandbox2config so the injection path uses the same environment logic as direct container invocations.pidfd_openand a singlesetnscall; if the session leader exits between pid resolution and injection, the shim will fail with aNsenterError.Macroscope summarized f3ee61d.