feat: session-specific sandbox wiring - #425

crates/sandbox2/src/lib.rs (1)

224-240: 💤 Low value

Consider extracting a helper to derive the session working-directory name.

The pattern working_name_override.as_ref().cloned().unwrap_or_else(|| SESSION_DEFAULT_WD.to_string()) is repeated in three places (rootfs init, command_inner, and new_container). A small helper on WdSetup would centralize this logic and simplify call sites.

♻️ Example helper method

impl WdSetup {
    /// Returns the working directory name for Session layouts.
    /// Panics if called on non-Session variants.
    pub(crate) fn session_wd_name(&self) -> String {
        match self {
            Self::Session { working_name_override, .. } => {
                working_name_override
                    .clone()
                    .unwrap_or_else(|| SESSION_DEFAULT_WD.to_string())
            }
            _ => panic!("session_wd_name called on non-Session variant"),
        }
    }
}

🤖 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/lib.rs` around lines 224 - 240, The pattern for deriving
the session working-directory name
(working_name_override.as_ref().cloned().unwrap_or_else(||
SESSION_DEFAULT_WD.to_string())) is repeated in three places. Create a helper
method called session_wd_name on the WdSetup enum that encapsulates this logic
by matching on the Session variant and returning the working directory name.
Then replace all three occurrences of this pattern (in the rootfs init section
shown, in command_inner, and in new_container) with calls to this new helper
method to centralize and simplify the logic.

crates/minimald/src/session.rs (1)

34-38: 💤 Low value

Add common derives to SessionPaths.

The struct is public and would benefit from Debug and Clone derives for debugging and flexible usage patterns.

♻️ Suggested change

+#[derive(Debug, Clone)]
 pub struct SessionPaths {
     pub working: DaemonAbsPath,
     pub cache: DaemonAbsPath,
     pub home: DaemonAbsPath,
 }

🤖 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 34 - 38, Add the `Debug` and
`Clone` derives to the `SessionPaths` struct definition. Place a
`#[derive(Debug, Clone)]` attribute above the struct declaration to enable
debugging output and cloning of struct instances, which improves usability for a
public struct.

crates/sessions/src/store.rs (1)

106-120: 💤 Low value

Consider extracting the common base path computation.

All three path methods (workspace_path, home_path, cache_path) duplicate the base path computation. Additionally, DaemonRelPath::try_new(&self.key.dir_key) is called on self.key.dir_key which is already a DaemonRelPath (line 80), making the conversion redundant.

♻️ Suggested refactor to reduce duplication

 impl SessionObject for DiskSession {
     type Key = DiskSessionKey;

+    fn session_base(&self) -> DaemonAbsPath {
+        sub_path!(self.minimal_state_dir, "sessions").join(&self.key.dir_key)
+    }
+
     fn record(&self) -> &Record {
         &self.record
     }
     fn key(&self) -> &DiskSessionKey {
         &self.key
     }
     fn workspace_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "tree")
+        sub_path!(self.session_base(), "tree")
     }
     fn home_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "home")
+        sub_path!(self.session_base(), "home")
     }
     fn cache_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "cache")
+        sub_path!(self.session_base(), "cache")
     }
 }

🤖 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/sessions/src/store.rs` around lines 106 - 120, The methods
workspace_path, home_path, and cache_path contain duplicated code for computing
the base path using sub_path and DaemonRelPath::try_new. Extract this common
base path computation into a private helper method to eliminate duplication.
Additionally, since self.key.dir_key is already a DaemonRelPath (as shown on
line 80), remove the redundant DaemonRelPath::try_new wrapper call and directly
use self.key.dir_key in the path computation. Update all three methods to call
the new helper method instead of repeating the base path logic.

crates/minimald/src/env_min_helper.sh (1)

131-136: 💤 Low value

build and test subcommands ignore additional arguments.

min build foo and min test bar will ignore foo and bar respectively, running only min run build or min run test. If this is intentional for simplicity, consider adding a comment. Otherwise, forward the arguments.

Suggested fix if arguments should be forwarded

         build)
-            min_run build
+            min_run build "$@"
             ;;
         test)
-            min_run test
+            min_run test "$@"
             ;;

🤖 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/env_min_helper.sh` around lines 131 - 136, The build and
test case statements in the switch block are not forwarding additional
command-line arguments to the min_run function. In the build case and test case
blocks, modify the min_run calls to append the remaining arguments (using the
appropriate shell variable for all arguments passed after the subcommand) so
that commands like `min build foo` will properly forward `foo` to min_run
instead of discarding it.

crates/minimald/src/env.rs (2)

410-455: ⚖️ Poor tradeoff

Consider reducing repetition in add- command handlers.*

The four add-* branches share nearly identical structure: parse packages, handle errors, call install(), then return the mode. This could be extracted into a helper, though the current implementation is clear and functional.

🤖 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/env.rs` around lines 410 - 455, Extract the repeated
logic from the four add-* command handlers (add-session, add-build, add-runtime,
add-task) into a helper method. The common pattern in each branch is: parse
packages using parse_pkgs_line, handle errors with the same error message
format, call install, and return a result. Create a helper method that accepts
the package string and a closure or enum variant to construct the appropriate
AddDepMode result, then replace each of the four match arms with a call to this
helper to eliminate the duplication while preserving the unique AddDepMode
construction logic for each command type.

---

254-258: 💤 Low value

Chained .unwrap() calls on path conversion could panic.

If sandbox.rootfs() returns a path that isn't valid UTF-8, the Utf8PathBuf::try_from(...).unwrap() will panic. While sandbox rootfs paths are typically UTF-8, consider using expect() with a descriptive message or propagating the error for robustness.

Suggested improvement

         let channel = SessionChannel {
-            rootfs: DaemonAbsPath::try_new(
-                Utf8PathBuf::try_from(sandbox.rootfs().to_path_buf()).unwrap(),
-            )
-            .unwrap(),
+            rootfs: DaemonAbsPath::try_new(
+                Utf8PathBuf::try_from(sandbox.rootfs().to_path_buf())
+                    .expect("sandbox rootfs must be valid UTF-8"),
+            )
+            .expect("sandbox rootfs must be absolute"),

🤖 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/env.rs` around lines 254 - 258, Replace the chained
`.unwrap()` calls in the SessionChannel initialization (specifically in the
rootfs field where DaemonAbsPath::try_new and Utf8PathBuf::try_from are used)
with `.expect()` calls that include descriptive error messages. The inner
`.unwrap()` from Utf8PathBuf::try_from should explain that the sandbox rootfs
path must be valid UTF-8, and the outer `.unwrap()` from DaemonAbsPath::try_new
should explain that the rootfs path must be an absolute path. This provides
better debugging information if the conversions fail instead of a generic panic
message.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/sessions/src/store.rs (2)

123-137: 💤 Low value

Consider extracting the common base path computation.

The base path calculation is duplicated across workspace_path, home_path, and cache_path. A private helper would reduce duplication.

♻️ Suggested refactor

+    fn session_base_path(&self) -> DaemonAbsPath {
+        sub_path!(self.minimal_state_dir, "sessions")
+            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap())
+    }
+
     fn workspace_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "tree")
+        sub_path!(self.session_base_path(), "tree")
     }
     fn home_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "home")
+        sub_path!(self.session_base_path(), "home")
     }
     fn cache_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "cache")
+        sub_path!(self.session_base_path(), "cache")
     }

🤖 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/sessions/src/store.rs` around lines 123 - 137, The base path
computation is duplicated identically across the workspace_path, home_path, and
cache_path methods. Extract this common computation into a private helper method
that returns the base DaemonAbsPath (which combines self.minimal_state_dir,
"sessions", and the dir_key). Then refactor each of the three methods to call
this helper instead of duplicating the base path calculation logic.

---

240-240: 💤 Low value

Method signature could be tightened.

write_record doesn't mutate self, so &self would suffice. Also, short: &str is more idiomatic than short: &String.

♻️ Suggested signature

-    fn write_record(&mut self, short: &String, record: &Record) -> Result<(), std::io::Error> {
+    fn write_record(&self, short: &str, record: &Record) -> Result<(), std::io::Error> {

🤖 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/sessions/src/store.rs` at line 240, The write_record method signature
has two issues that can be tightened. First, change the receiver from &mut self
to &self since the method does not actually mutate self. Second, change the
parameter short from &String to &str, which is the more idiomatic Rust pattern
for accepting string data. These changes make the API clearer about the method's
intent and follow Rust conventions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/sessions/src/store.rs (2)

123-137: 💤 Low value

Consider extracting the common base path computation.

The base path calculation is duplicated across workspace_path, home_path, and cache_path. A private helper would reduce duplication.

♻️ Suggested refactor

+    fn session_base_path(&self) -> DaemonAbsPath {
+        sub_path!(self.minimal_state_dir, "sessions")
+            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap())
+    }
+
     fn workspace_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "tree")
+        sub_path!(self.session_base_path(), "tree")
     }
     fn home_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "home")
+        sub_path!(self.session_base_path(), "home")
     }
     fn cache_path(&self) -> DaemonAbsPath {
-        let base = sub_path!(self.minimal_state_dir, "sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap());
-        sub_path!(base, "cache")
+        sub_path!(self.session_base_path(), "cache")
     }

🤖 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/sessions/src/store.rs` around lines 123 - 137, The base path
computation is duplicated identically across the workspace_path, home_path, and
cache_path methods. Extract this common computation into a private helper method
that returns the base DaemonAbsPath (which combines self.minimal_state_dir,
"sessions", and the dir_key). Then refactor each of the three methods to call
this helper instead of duplicating the base path calculation logic.

---

240-240: 💤 Low value

Method signature could be tightened.

write_record doesn't mutate self, so &self would suffice. Also, short: &str is more idiomatic than short: &String.

♻️ Suggested signature

-    fn write_record(&mut self, short: &String, record: &Record) -> Result<(), std::io::Error> {
+    fn write_record(&self, short: &str, record: &Record) -> Result<(), std::io::Error> {

🤖 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/sessions/src/store.rs` at line 240, The write_record method signature
has two issues that can be tightened. First, change the receiver from &mut self
to &self since the method does not actually mutate self. Second, change the
parameter short from &String to &str, which is the more idiomatic Rust pattern
for accepting string data. These changes make the API clearer about the method's
intent and follow Rust conventions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/sessions/src/store.rs (1)

340-360: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

In-memory index mutation before disk write risks inconsistent state on failure.

If write_record fails after line 351 removes the old name from the in-memory index, the process retains a corrupted view where the old name is gone but the rename never completed on disk. Defer in-memory mutations until after both disk writes succeed.

🐛 Proposed fix to reorder operations

 fn rename(&mut self, key: &Self::Key, new_name: String) -> Result<(), std::io::Error> {
     if self.index.find_by_name(&new_name).is_some() {
         return Err(std::io::Error::new(
             AlreadyExists,
             format!("a session with the name `{new_name}` already exists"),
         ));
     }

     let mut obj = self.get(key)?;
     let short = obj.key.dir_key.to_string();
-    if let Some(old_name) = &obj.record.name {
-        self.index.name_to_id.remove(old_name);
-    }
+    let old_name = obj.record.name.clone();

     obj.record.name = Some(new_name.clone());
     self.write_record(&short, &obj.record)?;

+    // Only mutate in-memory index after disk writes succeed
+    if let Some(old_name) = &old_name {
+        self.index.name_to_id.remove(old_name);
+    }
     self.index.name_to_id.insert(new_name, obj.record.id);
     self.flush_index()?;
     Ok(())
 }

🤖 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/sessions/src/store.rs` around lines 340 - 360, The in-memory index
mutations are happening before the disk writes are guaranteed to succeed,
creating a risk of inconsistent state if a write fails. In the rename function,
the removal of the old name from self.index.name_to_id (before write_record) and
the insertion of the new name into self.index.name_to_id (before flush_index)
should be deferred until after both write_record and flush_index have completed
successfully. Reorder the operations so that all in-memory index updates happen
only after the disk operations succeed to ensure consistency.