Minimald scaffolding, including sessions actor, session actor, session store + generics, and RPC tests - #253

crates/minimald/src/main.rs (3)

28-49: ⚡ Quick win

Replace unwrap() calls with expect("reason") to document invariants.

Multiple unwrap() calls in path construction can obscure why they're safe. Per guidelines, use expect("why the invariant holds") to document the assumptions:

• Line 32: try_new on an absolute path (validated by is_absolute() check)
• Line 35: from_cwd() can fail if CWD is invalid or non-UTF8
• Line 36: try_new on a relative path (validated by !is_absolute() branch)
• Lines 40-47: from_path_buf fails on non-UTF8 paths; try_new validates absoluteness

♻️ Suggested improvements

             Some(d) => {
                 if d.is_absolute() {
-                    DaemonAbsPath::try_new(d.clone()).unwrap()
+                    DaemonAbsPath::try_new(d.clone()).expect("validated absolute above")
                 } else {
                     DaemonAbsPath::from_cwd()
-                        .unwrap()
-                        .join(&DaemonRelPath::try_new(d).unwrap())
+                        .expect("CWD should be valid UTF-8 path")
+                        .join(&DaemonRelPath::try_new(d).expect("validated relative above"))
                 }
             }
             None => DaemonAbsPath::try_new(
                 Utf8PathBuf::from_path_buf(
                     dirs::state_dir()
                         .unwrap_or_else(|| PathBuf::from("~/.local/state"))
                         .join("minimal"),
                 )
-                .unwrap(),
+                .expect("XDG state dir should be valid UTF-8"),
             )
-            .unwrap(),
+            .expect("XDG state dir is absolute"),

As per coding guidelines: "Only use unwrap() and panic!() for broken invariants; in production code use expect("why the invariant holds") instead of unwrap".

🤖 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/main.rs` around lines 28 - 49, In minimal_state_dir,
replace the bare unwrap() calls with expect("...") that document the invariant
for each use: for DaemonAbsPath::try_new(d.clone()) inside the is_absolute()
branch use expect("absolute path validated by is_absolute()"), for
DaemonAbsPath::from_cwd() use expect("current working directory must be valid
UTF-8"), for DaemonRelPath::try_new(d) in the relative branch use
expect("relative path validated by !is_absolute()"), and for
Utf8PathBuf::from_path_buf(...) and the outer DaemonAbsPath::try_new(...) in the
None branch add expect messages like expect("state dir path is valid UTF-8") and
expect("constructed state dir is absolute") respectively so each panic documents
the assumed invariant.

---

53-63: 💤 Low value

Same unwrap() → expect() improvement needed here.

Apply the same pattern as minimal_state_dir() to document why these invariants hold.

♻️ Suggested fix

     pub fn minimal_cache_dir(&self) -> DaemonAbsPath {
         DaemonAbsPath::try_new(
             Utf8PathBuf::from_path_buf(
                 dirs::cache_dir()
                     .unwrap_or_else(|| PathBuf::from("~/.local/cache"))
                     .join("minimal"),
             )
-            .unwrap(),
+            .expect("XDG cache dir should be valid UTF-8"),
         )
-        .unwrap()
+        .expect("XDG cache dir is 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/main.rs` around lines 53 - 63, Replace the chained
unwraps in minimal_cache_dir with explicit expect messages mirroring
minimal_state_dir: when calling dirs::cache_dir().unwrap_or_else(...) and when
converting to Utf8PathBuf::from_path_buf(...).unwrap() and
DaemonAbsPath::try_new(...).unwrap(), use expect(...) with short explanations
(e.g., why a fallback path exists and why the path must be valid UTF‑8 /
absolute) so each invariant is documented; locate the minimal_cache_dir function
and update the unwraps on dirs::cache_dir()/Utf8PathBuf::from_path_buf(...) and
DaemonAbsPath::try_new(...) to expect(...) with descriptive messages.

---

182-186: 💤 Low value

Use structured logging fields instead of string interpolation.

Line 182 should use structured fields for machine-parseable logs. The multi-line debug message on lines 183-186 is fine as-is since it's user-facing guidance.

♻️ Suggested fix

-    tracing::info!("Started listening on {}", cli.listen_on());
+    tracing::info!(path = %cli.listen_on(), "Started listening");

As per coding guidelines: "Use structured logging with tracing: tracing::info!(pkg = %name, "building") instead of interpolated strings".

🤖 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/main.rs` around lines 182 - 186, Replace the interpolated
log with a structured tracing field: change the call that uses
tracing::info!("Started listening on {}", cli.listen_on()) to use a named field
such as tracing::info!(listen_on = %cli.listen_on(), "Started listening on");
leave the multi-line ssh guidance block (the tracing::info! that prints the
debug command) unchanged.

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

19-38: ⚡ Quick win

Mark Record as #[non_exhaustive] to keep API evolution non-breaking.

Given this is a public record type, future field additions are likely.

Suggested patch

 #[derive(Debug, Clone, Serialize, Deserialize)]
+#[non_exhaustive]
 pub struct Record {

As per coding guidelines, "Apply #[non_exhaustive] to public enums and structs that may grow".

🤖 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/lib.rs` around lines 19 - 38, Public struct Record should
be marked #[non_exhaustive] so adding fields later won't be a breaking change;
add the #[non_exhaustive] attribute immediately above the Record definition
(e.g., above #[derive(...)] for Record) and ensure any external pattern matches
using Record are updated to use a wildcard (_) or the struct update pattern
where appropriate (since consumers will now need to account for possible future
fields).

crates/common/src/renameat2.rs (1)

75-84: ⚡ Quick win

test_rename_noreplace does not exercise the NOREPLACE failure path.

The current test only validates rename-to-missing-target. To verify RENAME_NOREPLACE, pre-create new and assert AlreadyExists.

Suggested patch

     fn test_rename_noreplace() {
         let tmp = TempDir::new().unwrap();
         let old = tmp.path().join("old");
         let new = tmp.path().join("new");

         fs::write(&old, "test").unwrap();
+        fs::write(&new, "existing").unwrap();

-        renameat2_cwd(&old, &new, RENAME_NOREPLACE).unwrap();
-        assert!(new.exists());
+        let err = renameat2_cwd(&old, &new, RENAME_NOREPLACE).unwrap_err();
+        assert_eq!(err.kind(), std::io::ErrorKind::AlreadyExists);
+        assert_eq!(fs::read_to_string(&new).unwrap(), "existing");
     }

As per coding guidelines, "Write tests that verify behavior, not implementation; one concept per 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/common/src/renameat2.rs` around lines 75 - 84, The test
test_rename_noreplace currently only verifies a successful rename to a missing
target; update it to also exercise the NOREPLACE failure path by pre-creating
the destination path (new) before calling renameat2_cwd(&old, &new,
RENAME_NOREPLACE) and assert that the call returns an AlreadyExists (or
equivalent) error; ensure the test explicitly checks the error kind rather than
implementation details so both success and failure behaviors of RENAME_NOREPLACE
are covered by separate assertions in the same test.

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

113-118: ⚡ Quick win

Use expect with invariant explanation instead of unwrap.

Per coding guidelines, unwrap() should be replaced with expect("why the invariant holds") for broken invariants.

Proposed fix

     fn workspace_path(&self) -> DaemonAbsPath {
         self.minimal_state_dir
             .sub_path("sessions")
-            .join(&DaemonRelPath::try_new(&self.key.dir_key).unwrap())
+            .join(&DaemonRelPath::try_new(&self.key.dir_key)
+                .expect("dir_key is always a valid relative path component"))
             .sub_path("tree")
     }

As per coding guidelines: "Only use unwrap() and panic!() for broken invariants; in production code use expect("why the invariant holds") instead of unwrap".

🤖 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 113 - 118, In workspace_path(),
replace the call to DaemonRelPath::try_new(&self.key.dir_key).unwrap() with
.expect("explain why dir_key must be a valid DaemonRelPath here") so that the
invariant failure produces a clear message; update the expect string to
succinctly state the invariant (e.g., "dir_key was validated on construction and
must be a valid DaemonRelPath") referencing workspace_path and
DaemonRelPath::try_new to locate the change.

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

197-218: 💤 Low value

Cross-platform path separator check may be incomplete.

On Windows, both \ and / are valid path separators, but MAIN_SEPARATOR_STR is only \. A caller could pass "foo/bar" on Windows and bypass the assertion. If this crate is Linux-only (implied by the renameat2 usage elsewhere), consider documenting that assumption or adding a / check for defense-in-depth.

Proposed fix to check both separators

     pub fn sub_path(&self, dir: &'static str) -> AbsPath<R> {
         assert!(
-            !dir.contains(std::path::MAIN_SEPARATOR_STR),
-            ".subdir(\"{dir}\") contains path separators"
+            !dir.contains(std::path::MAIN_SEPARATOR_STR) && !dir.contains('/'),
+            ".sub_path(\"{dir}\") contains path separators"
         );
         assert!(dir != "..", ".subdir(\"..\") attempts path traversal");

🤖 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/paths.rs` around lines 197 - 218, The assertion in
sub_path does not catch Windows-style '/' separators because it only checks
std::path::MAIN_SEPARATOR_STR; update the validation in fn sub_path to reject
both '/' and the platform MAIN_SEPARATOR (or explicitly check dir.contains('/')
|| dir.contains(std::path::MAIN_SEPARATOR_STR)) and keep the existing ".." check
and panic message; make sure the error message remains clear and references
sub_path/subdir usage so callers see why the input was rejected.

crates/minimald/src/rpc.rs (4)

92-94: 💤 Low value

Use structured logging fields instead of format interpolation.

As per coding guidelines, use structured fields with tracing: tracing::warn!(rpc = Self::NAME, error = %e, "RPC handler failed") instead of interpolated format strings.

♻️ Suggested fix

-            tracing::warn!("RPC handler for {} failed: {}", Self::NAME, e);
+            tracing::warn!(rpc = Self::NAME, error = %e, "RPC handler failed");

🤖 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/rpc.rs` around lines 92 - 94, Replace the interpolated
log in the error branch with structured tracing fields: in the RPC error
handling where you currently use tracing::warn!("RPC handler for {} failed: {}",
Self::NAME, e), change it to use structured fields (e.g. tracing::warn!(rpc =
Self::NAME, error = %e, "RPC handler failed")) so the RPC name and error are
separate fields; update the call in the same block that references Self::NAME
and variable e.

---

236-241: 💤 Low value

Spawned task handles are silently dropped.

The JoinHandle returned by spawn is discarded. If a handler panics, it won't be observed. Consider either:

1. Adding a brief comment explaining this is intentional fire-and-forget
2. Collecting handles for observability

Since handlers log their own errors, this is likely intentional but warrants a comment.

🤖 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/rpc.rs` around lines 236 - 241, The spawned JoinHandle
returned by spawn for GetVersion.handle, ListSessions.handle and
GetSessionRecord.handle is currently discarded which hides panics; either
explicitly mark this as intentional fire-and-forget by adding a concise comment
above the match stating that handlers own their error logging and panics are
acceptable/ignored, or collect the JoinHandles (e.g., push them into a
Vec<JoinHandle<...>> or an observability task registry) so panics can be
awaited/inspected; update the match site accordingly and reference the spawn
calls and the individual handler entry points (GetVersion.handle,
ListSessions.handle, GetSessionRecord.handle) when making the change.

---

138-140: 💤 Low value

Same structured logging improvement applies here.

♻️ Suggested fix

-            tracing::warn!("RPC handler for {} failed: {}", Self::NAME, e);
+            tracing::warn!(rpc = Self::NAME, error = %e, "RPC handler failed");

🤖 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/rpc.rs` around lines 138 - 140, Replace the
string-formatted warning in the RPC error branch with structured tracing fields:
when checking the result (if let Err(e) = res) log using tracing::warn! with
named fields for the handler name (Self::NAME) and the error (e) instead of
interpolating them into the message so the log system can index them; update the
log invocation in the block that currently contains tracing::warn!("RPC handler
for {} failed: {}", Self::NAME, e) to use structured fields like handler =
Self::NAME and error = ?e (or %e) and a short message such as "RPC handler
failed".

---

188-190: 💤 Low value

Same structured logging improvement applies here.

♻️ Suggested fix

-            tracing::warn!("RPC handler for {} failed: {}", Self::NAME, e);
+            tracing::warn!(rpc = Self::NAME, error = %e, "RPC handler failed");

🤖 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/rpc.rs` around lines 188 - 190, The current warn log in
the error branch of the RPC handler uses string interpolation which loses
structured fields; change the logging in the block that checks if let Err(e) =
res (the code referencing Self::NAME) to use structured tracing fields instead —
e.g., pass the handler name and the error as separate fields (handler =
Self::NAME, error = %e) in the tracing::warn! call so the error is recorded as a
structured field rather than embedded in a formatted message.

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

364-367: ⚡ Quick win

Inconsistent lock access pattern.

This method still uses self.0.0.lock() while other methods now use self.0.lock(). Use the new helper for consistency.

♻️ Suggested fix

     async fn channel_close(&mut self, id: ChannelId, _: &mut Session) -> Result<(), Self::Error> {
         protocol_trace!("Got channel_close on channel {id}");
-        self.0.0.lock().await.handle_channel_close(id)
+        self.0.lock().await.handle_channel_close(id)
     }

🤖 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/connection.rs` around lines 364 - 367, In channel_close,
the lock is acquired via the old field path self.0.0.lock().await; update it to
use the new helper field by calling self.0.lock().await and then invoke
handle_channel_close(id) on the locked value so the method mirrors the other
methods' access pattern (refer to function channel_close and the helper accessor
self.0).

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

50-57: 💤 Low value

Improve error discard justification placement.

The comment on line 54 explains the discard but would be clearer directly on line 55 where the discard occurs.

♻️ Suggested fix

     pub async fn workspace_path(&self) -> DaemonAbsPath {
         let (send, recv) = oneshot::channel();
-        // Ignore send errors - the recv will also fail.
-        let _ = self.0.send(SessionMessage::GetWorkspacePath(send)).await;
+        let _ = self.0.send(SessionMessage::GetWorkspacePath(send)).await; // If send fails, recv below will also fail.
         recv.await.expect("corresponding session is dead")
     }

🤖 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 50 - 57, Move the explanatory
comment about ignoring send errors so it sits directly above the discard
statement in SessionHandle::workspace_path (i.e., immediately before the `_ =
self.0.send(SessionMessage::GetWorkspacePath(send)).await;` line) rather than
earlier; keep the comment text but relocate it to clarify that we intentionally
ignore send errors because the corresponding recv will also fail when the
session is dead.

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/paths.rs (1)

269-277: 💤 Low value

Consider harmonizing error message phrasing with doc comment.

The doc comment (line 268) says "non-UTF8" while the error message uses "not-utf8". For consistency, consider using the same phrasing and capitalization in both places.

♻️ Proposed fix

     pub fn from_cwd() -> Result<Self, std::io::Error> {
         let Ok(cwd) = Utf8PathBuf::from_path_buf(std::env::current_dir()?) else {
-            return Err(std::io::Error::other("cwd contains not-utf8 characters"));
+            return Err(std::io::Error::other("cwd contains non-UTF-8 characters"));
         };

🤖 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/paths.rs` around lines 269 - 277, The error message in
from_cwd (which uses Utf8PathBuf::from_path_buf and returns
std::io::Error::other) uses "cwd contains not-utf8 characters" but the doc
comment uses "non-UTF8"; update the returned error string to match the doc
comment phrasing and capitalization (e.g., "cwd contains non-UTF8 characters")
so the message is consistent with the documentation.

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

30-57: ⚡ Quick win

Replace bare .unwrap() calls with .expect("reason") to document invariants.

Multiple .unwrap() calls without context make debugging difficult if they fail. Per coding guidelines, use .expect("why the invariant holds") for assumptions about the environment.

♻️ Proposed refactor with explicit reasons

     pub fn minimal_state_dir(&self) -> DaemonAbsPath {
         match &self.global_args.minimal_dir {
-            Some(p) => p.resolve().unwrap(),
+            Some(p) => p.resolve().expect("--minimal-dir should resolve to absolute path"),
             None => DaemonAbsPath::try_new(
                 Utf8PathBuf::from_path_buf(
                     dirs::state_dir()
-                        .unwrap_or_else(|| dirs::home_dir().unwrap().join(".local/state"))
+                        .unwrap_or_else(|| {
+                            dirs::home_dir()
+                                .expect("home directory must exist for default state dir")
+                                .join(".local/state")
+                        })
                         .join("minimal"),
                 )
-                .unwrap(),
+                .expect("state directory path must be valid UTF-8"),
             )
-            .unwrap(),
+            .expect("state directory must be absolute"),
         }
     }

Apply the same pattern to minimal_cache_dir().

🤖 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/main.rs` around lines 30 - 57, The code in
minimal_state_dir and minimal_cache_dir uses multiple bare .unwrap() calls;
replace each .unwrap() with .expect("...") giving a short reason why the value
must exist (e.g., "state dir fallback must exist", "UTF-8 conversion should
succeed", or "DaemonAbsPath creation must succeed") so failures emit clear
diagnostics. Update the unwraps on dirs::state_dir()/dirs::cache_dir()
fallbacks, Utf8PathBuf::from_path_buf(...).unwrap(), and
DaemonAbsPath::try_new(...).unwrap() within the minimal_state_dir() and
minimal_cache_dir() functions to use .expect(...) with context strings
referencing the specific invariant being assumed.

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

92-94: 💤 Low value

Use structured logging fields instead of interpolated strings.

Per coding guidelines, prefer structured tracing fields for machine-parseable logs.

♻️ Example fix (apply to lines 140, 191 as well)

-            tracing::warn!("RPC handler for {} failed: {}", Self::NAME, e);
+            tracing::warn!(rpc = %Self::NAME, error = %e, "RPC handler failed");

As per coding guidelines: "Use structured logging with tracing: tracing::info!(pkg = %name, "building") instead of interpolated strings like info!("building {name}")"

🤖 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/rpc.rs` around lines 92 - 94, Replace the interpolated
string in the tracing::warn call with structured fields: pass the handler name
and the error as named fields (e.g., name = %Self::NAME, error = %e) to
tracing::warn! so the log is machine-parseable; update the tracing calls in this
block (the current tracing::warn!("RPC handler for {} failed: {}", Self::NAME,
e)) and make the same structured-field change for the other similar tracing
calls referenced in the review (lines shown around the other occurrences, e.g.,
the calls at the locations you noted earlier).

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

39-44: 💤 Low value

Add justification comment for discarded send result.

Line 43 discards the oneshot send result without a comment. The ManagerHandle methods (lines 171, 182, 193) include comments like // Ignore send errors - the recv will also fail. for the same pattern. Adding a similar comment here maintains consistency and satisfies the guideline against silent error discards.

Suggested fix

     pub async fn handle<F>(self, fut: F)
     where
         F: Future<Output = Result<T, ResponseError>>,
     {
+        // If the receiver dropped, the caller no longer cares about the result.
         let _ = self.0.send(fut.await);
     }

🤖 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/sessions.rs` around lines 39 - 44, The oneshot send
result in the async function handle (the call self.0.send(fut.await)) is being
discarded without explanation; add a brief comment above or inline with that
send — e.g. mirroring other ManagerHandle methods — such as "// Ignore send
errors - the recv will also fail." to justify swallowing the send error and
maintain consistency with ManagerHandle's other methods (which use the same
rationale).

Fix all unresolved CodeRabbit comments on this PR:

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