crates/minimald: create scaffolding for building up SSH channels - #235

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/connection.rs`:
- Around line 375-376: Fix the typo in the inline comment near the channel_eof
note: change "propergation" to "propagation" in the comment that reads "// NOTE:
channel_eof left at default impl, implementing channel_eof prevents propergation
to channel handle" (search for the comment mentioning channel_eof to locate it).
- Line 95: The doc comment for the method that "Consumes a pending session,
returning its state and leaving [`ChannelInner::Finished`] in its place"
references a non-existent variant; update the doc to reference the actual enum
variant `ChannelInner::Taken` (or match the enum variant name used in the code)
so the documentation matches the implementation and avoids confusion when
locating the `ChannelInner` variant.
- Around line 351-364: The spawned async task currently uses println! for
logging the result of rpc::GetVersion.handle(...).await; replace println! with a
tracing macro (e.g., tracing::debug! or tracing::info!) so the output respects
structured logging and levels. Concretely, in the spawn async move block around
rpc::GetVersion.handle(c_hnd, |_req| { ... }).await, assign the awaited result
to a local variable (e.g., let resp = ... .await) and then call
tracing::debug!("handle response: {:?}", resp); (ensure tracing is in scope or
fully qualify the macro). This replaces the println! usage and preserves the
same message content.

In `@crates/minimald/src/main.rs`:
- Around line 30-36: The fallback that constructs
PathBuf::from("~/.local/state") (in minimal_state_dir) is incorrect because "~"
is not expanded; change the fallback to use dirs::home_dir() and join the
".local/state" and "minimal" segments (e.g., dirs::home_dir().map(|h|
h.join(".local").join("state")).unwrap_or_else(||
PathBuf::from(".local/state")).join("minimal")), and make the analogous change
for the cache fallback used in minimal_cache_dir (replace
PathBuf::from("~/.local/cache") with building from dirs::home_dir() and joining
".local/cache" with a safe fallback).

---

Nitpick comments:
In `@crates/minimald/src/connection.rs`:
- Around line 177-185: The call to Channel::handle_channel_close on the value
returned by self.channels.remove(&id) is pointless because the removed channel
is dropped immediately; either call handle_channel_close before removal or drop
the call entirely. Fix handle_channel_close by locating the function
handle_channel_close in connection.rs and change the flow in
handle_channel_close(&mut self, id: ChannelId) to first borrow the channel
mutably (e.g., self.channels.get_mut(&id) or use Entry API), call
c.handle_channel_close() to perform any cleanup, and only then remove the
channel (self.channels.remove(&id)); if handle_channel_close only sets an
internal closed flag and no external cleanup is required, remove the call
instead.

In `@crates/minimald/src/rpc.rs`:
- Around line 7-44: In OneshotSshRpc::handle, remove the two commented-out calls
to stream.flush() and stream.shutdown() (the lines containing
stream.flush().await.map_err(russh::Error::from)?; and
stream.shutdown().await.map_err(russh::Error::from)?;) OR replace them with a
short comment explaining why they are intentionally omitted (e.g., the
underlying RuChannel stream already closes/flushes when dropped or those
operations caused issues), so the repository has no unexplained commented-out
code.

In `@crates/minimald/src/server.rs`:
- Around line 121-127: Replace the bare unwrap() used when obtaining the host
key inside async fn run()—specifically the call self.state.host_key().unwrap()
used to populate the russh_config keys vector—with expect("...") and a short
reason; update the call to self.state.host_key().expect("host key must be
present when starting SSH server") (or similar) so the code documents the
invariant and provides a clear panic message if the key is missing.

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/connection.rs`:
- Around line 134-139: Change Connection::from_socket to return
Result<(ConnectionHandle, RunningSession<ConnectionHandler>), ConnectionError>
instead of unwrapping run_stream; propagate russh::server::run_stream(...)
errors (map them into ConnectionError with context) and return Err on failure
rather than panicking. For the RPC startup, capture the tokio::task::JoinHandle
returned when calling tokio::spawn for rpc::GetVersion.handle(...).await and
either .await the JoinHandle (and log/map any JoinError or inner
ConnectionError) or spawn a small task that logs the error if it fails, so the
Result<(), ConnectionError> is not dropped. Finally, either justify or remove
#[allow(dead_code)] on RequestedPty, Pending, ChannelInner, Channel, Connection,
and ConnectionError with a short comment explaining why they remain unused or
remove the attribute if no longer needed.
- Around line 354-364: The spawned task currently calls
rpc::GetVersion.handle(...).await and discards its Result; update the spawn body
so the Result from rpc::GetVersion.handle is matched/handled (e.g., use if let
Err(e) = ... or match) and log or otherwise handle the ConnectionError instead
of ignoring it; specifically modify the async move block around
rpc::GetVersion.handle(c_hnd, |_req| { ... }).await to capture the returned
Result and call the appropriate logger (or connection error handler) with the
error details to ensure failures are not silently dropped.

---

Nitpick comments:
In `@crates/minimald/src/connection.rs`:
- Line 37: There are unexplained #[allow(dead_code)] attributes in this module;
for each occurrence of #[allow(dead_code)] either remove the attribute and
tighten the API (make the item private or delete unused functions/structs), or
keep it but add a one-line justification comment immediately next to the
attribute (e.g., "// allow(dead_code): exported for benches/tests/feature-gate
X" or "// allow(dead_code): used only in FFI/extern tests") — alternatively
prefer using #[cfg(test)] or #[cfg(feature = "...")] on the item instead of
blanket dead_code suppression; update the occurrences of #[allow(dead_code)]
accordingly so every suppression has a clear justification or is eliminated.

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/connection.rs`:
- Around line 314-315: Remove the ownerless TODOs in the request handlers by
replacing the ambiguous "// TODO: handle exec request" (near
session.channel_failure(id)? and the similar note on lines 324-325) with an
explicit, tracked action: either implement the exec handling now or mark the
path as intentionally unsupported and/or reference a tracked issue and owner.
Concretely, update the handler around session.channel_failure(id)? to do one of
the following: implement the exec request logic in the function that processes
exec requests, or replace the TODO with a clear comment like "intentionally
unsupported" plus an issue/owner tag (e.g., "ISSUE-1234 `@owner`") and ensure the
code returns a deterministic response (e.g., call session.channel_failure(id)?
and return an appropriate Err or Ok) so there are no ownerless TODOs left; apply
the same treatment to the other similar spot at lines 324-325.

In `@crates/minimald/src/lib.rs`:
- Line 7: Add a short justification comment immediately above the
#[allow(dead_code)] attribute that explains why the lint is suppressed (for
example: this struct is scaffolding for a higher layer / used only by tests /
will be referenced by FFI later), and include the specific item name it applies
to (the struct declared below the attribute); if the attribute is no longer
needed because the struct is now used, remove the #[allow(dead_code)] instead.

In `@crates/minimald/src/main.rs`:
- Around line 160-163: The logged SSH command in the tracing::info! call
currently suggests an interactive session ("ssh ... local") which is
unsupported; update the message emitted where tracing::info! is called (using
cli.listen_on().display() as the address) to either (a) log a transport-only
debug command that matches the supported SSH flow, e.g. use the subsystem form
with -sT and the minimald RPC name (minimald-v1-GetVersion) instead of an
interactive shell, or (b) explicitly annotate the existing example as
"transport-only debugging" so callers know shell/exec will be rejected; adjust
the tracing::info! message text accordingly.
- Around line 81-82: The help text for the CLI flag --minimal-dir incorrectly
references $XDG_STATE_DIR; update the docstring above the #[arg(long)] for the
--minimal-dir argument to read $XDG_STATE_HOME/minimal (since dirs::state_dir()
uses $XDG_STATE_HOME with fallback ~/.local/state), i.e., change the comment
"default: $XDG_STATE_DIR/minimal" to "default: $XDG_STATE_HOME/minimal" next to
the --minimal-dir arg so the help output is accurate.

---

Duplicate comments:
In `@crates/minimald/src/main.rs`:
- Around line 26-30: The fallback branches in minimal_state_dir (and the similar
minimal_cache_dir) currently use PathBuf::from("~/.local/...") which treats "~"
literally; change them to construct the fallback from dirs::home_dir() and then
join the ".local/state" (for minimal_state_dir) or ".local/cache" (for
minimal_cache_dir) and then "minimal" so the user's actual home directory is
used; update the unwrap_or_else paths to call dirs::home_dir(), build the proper
joined PathBuf, and only fall back to a literal if home_dir() is None.

---

Nitpick comments:
In `@crates/minimald/src/lib.rs`:
- Around line 8-13: Add the Rust attribute #[non_exhaustive] to the public
struct RequestedPty so new fields can be added later without breaking downstream
consumers; locate the declaration of pub struct RequestedPty and place
#[non_exhaustive] immediately above it (keeping the struct public and its
existing fields unchanged).

In `@crates/minimald/src/main.rs`:
- Around line 134-155: The startup code in async fn main uses blocking std::fs
calls; replace std::fs::create_dir_all and std::fs::remove_file with their async
tokio::fs equivalents and await them so startup I/O doesn't block the runtime.
Concretely, call tokio::fs::create_dir_all(cli.minimal_state_dir()).await and
tokio::fs::create_dir_all(parent).await for the socket parent, and
tokio::fs::remove_file(cli.listen_on()).await when removing the socket; keep the
existing error checks (e.kind() comparisons) and return Err(MainError::IO(e,
"...")) on failure to preserve behavior, and adjust any control flow to
accommodate the .await points inside async fn main.

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/connection.rs`:
- Around line 252-253: The protocol tracing currently prints sensitive contents:
change calls to protocol_trace! that log env_request and exec_request payloads
so they never include secret values; for example, in the env_request site (where
id, var_name, var_value are used) log only id and var_name and replace var_value
with a constant like "<redacted>" or omit it entirely, and in the exec_request
site stop serializing the full payload and instead log only metadata (command
name, args count, id) or a redacted summary; update the protocol_trace!
invocations referenced by env_request and exec_request to avoid dumping raw
values (also apply the same redaction approach to the other occurrences
mentioned).

In `@crates/minimald/src/lib.rs`:
- Around line 7-23: RequestedPty and ChannelConfig are exported as pub but all
fields are private, unnecessarily widening the public API; change both struct
declarations from pub struct to pub(crate) struct (and similarly change any
pub(crate) field visibility only if you intend external access later), keeping
derives and #[allow(dead_code)] intact—this confines the types to the crate
until you add curated constructors/accessors and re-export them from the crate
root via pub use.

In `@crates/minimald/src/main.rs`:
- Around line 134-149: The startup uses blocking std::fs::create_dir_all inside
the async main; replace those blocking calls with async
tokio::fs::create_dir_all(...).await (and tokio::fs::remove_file(...).await
where applicable) so they don't block the Tokio runtime. Update the two call
sites that reference std::fs::create_dir_all with tokio::fs equivalents and
await them (the ones using cli.minimal_state_dir() and cli.listen_on() parent),
preserve the existing error handling that maps to MainError::IO (return
Err(MainError::IO(e, "..."))), and add a tokio::fs import if needed. Ensure you
still check parent.is_some() and e.kind() != std::io::ErrorKind::AlreadyExists
when converting the awaited error into MainError::IO.

---

Nitpick comments:
In `@crates/minimald/src/connection.rs`:
- Around line 54-76: The Channel struct's closed flag is currently unused
because handle_channel_close() removes the channel from the container before
setting closed = true, making the c.closed checks in pending_config_mut() and
take() unreachable; fix by either (A) preserving entries when a channel is
closed and set self.closed = true inside Channel::handle_channel_close() before
or instead of removal so pending_config_mut() and take() can observe closed, or
(B) remove the dead flag and the c.closed branches from pending_config_mut() and
take() (and delete the closed field and any references) so behavior is
consistent; locate Channel::new_session, Channel::handle_channel_close,
pending_config_mut, and take to apply one coherent approach.