crates/minimal{2,d}: scaffold binary crates for Minimal One - #227
Merged
Conversation
📝 WalkthroughWalkthroughThis PR scaffolds two new crates in the workspace: ChangesWorkspace Scaffolding: minimal2 CLI and minimald SSH Daemon
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
twitchyliquid64
force-pushed
the
tom/scaffolding
branch
from
May 24, 2026 23:09
fa2c6c6 to
0b83bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
crates/minimal2/src/main.rs (1)
47-48: ⚡ Quick winReplace
unwrap()withexpect(...)for invariant parsing.These are constant directives; if they ever fail, the panic should state why the invariant holds.
Suggested fix
EnvFilter::new("info") - .add_directive("topiary=off".parse().unwrap()) - .add_directive("libcgroups=off".parse().unwrap()) + .add_directive("topiary=off".parse().expect("hardcoded directive is valid")) + .add_directive("libcgroups=off".parse().expect("hardcoded directive is valid"))As per coding guidelines, "Only use
unwrap()andpanic!()for broken invariants; in production code useexpect(\"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/minimal2/src/main.rs` around lines 47 - 48, Replace the two invocations that call .parse().unwrap() inside the add_directive chain with .parse().expect(...) so failures produce a clear invariant message; specifically update the calls around add_directive("topiary=off".parse().unwrap()) and add_directive("libcgroups=off".parse().unwrap()) to use expect with brief reasons (e.g. "parsing constant directive 'topiary=off' must succeed" and "parsing constant directive 'libcgroups=off' must succeed") so any panic explains why the constant parse is guaranteed.
🤖 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 `@Cargo.toml`:
- Line 78: Update the workspace dependency entry for russh from a semver range
to an exact pinned version: change the russh specification in
[workspace.dependencies] from "0.61" to the exact version resolved in Cargo.lock
(e.g. "=0.61.1") so the workspace inherits a stable release.
In `@crates/minimal2/build.rs`:
- Around line 3-8: The build script currently calls
Command::new("git").args(["rev-parse","--short","HEAD"]).output().unwrap() and
String::from_utf8(...).unwrap(), which will panic in non-git environments;
change build.rs to treat git metadata as best-effort by checking the Command
output/result and its status, handling errors from output() and from
String::from_utf8(), trimming whitespace, and falling back to a stable
placeholder like "unknown" (or similar) when git is unavailable; apply the same
defensive pattern to the other git-hash block referenced (lines ~20-25) so no
unwrap() or panic occurs for recoverable failures and the printed
cargo:rustc-env=GIT_HASH=... always emits a safe value.
In `@crates/minimal2/src/main.rs`:
- Line 9: The clap command attribute is incorrectly setting name = "minimal"
while the crate/executable is "minimal2", causing generated completion scripts
to target the wrong binary; update the #[command(...)] attribute on the main
function (and the other occurrences around the attribute at lines referenced) to
use the correct binary name "minimal2" or remove the explicit name so clap
derives it automatically, then regenerate completions so they bind to "minimal2"
instead of "minimal".
- Line 44: The async function signature `async fn main() -> Result<(), ()>`
should return an application-grade error type; change it to use
`anyhow::Result<()>` (i.e., `async fn main() -> anyhow::Result<()>`) and update
call sites to propagate errors with `.context(...)` or `.with_context(...)` at
boundaries; also add the `anyhow` dependency to the crate if missing and ensure
any `Err(())` or `Err` returns are converted to context-rich `anyhow::Error`
values before propagating from `main`.
In `@crates/minimald/build.rs`:
- Around line 3-8: The build script currently panics and writes a
newline-tainted value into GIT_HASH by calling Command::new("git")...
.output().unwrap(), String::from_utf8(...).unwrap(), and later using
git_hash.strip_suffix("\n").unwrap(); change these to handle errors without
panicking: run the git command with proper error handling (propagate or fallback
to a default like "unknown"), check output.status and use
String::from_utf8_lossy or String::from_utf8 with match/unwrap_or_else to
capture failure, trim the resulting git_hash with .trim_end() (instead of
strip_suffix with unwrap) before printing
println!("cargo:rustc-env=GIT_HASH={}", git_hash); and apply the same pattern to
crates/minimal/build.rs and crates/minimal2/build.rs so LONG_VERSION no longer
depends on a newline and no calls can panic.
In `@crates/minimald/src/connection.rs`:
- Around line 42-45: The call to russh::server::run_stream(...).await.unwrap()
inside from_socket can panic per-connection; change from_socket to propagate
that error by replacing the unwrap with proper error mapping/return: await the
future, handle the Result from russh::server::run_stream(...).await and convert
its Err into your ConnectionError (e.g. with map_err or ? after an
Into<ConnectionError> conversion), so ConnectionHandler and from_socket return
Result<..., ConnectionError> instead of panicking. Locate the unwrap in
from_socket and replace it with a mapped return of ConnectionError referencing
russh::server::run_stream and ConnectionHandler.
In `@crates/minimald/src/main.rs`:
- Around line 117-132: Replace the blocking std::fs calls inside the async main
with their async equivalents: use tokio::fs::create_dir_all and
tokio::fs::remove_file and .await their Results; map errors into MainError::IO
as before (e.g. if let Err(e) =
tokio::fs::create_dir_all(cli.minimal_dir()).await { if e.kind() !=
std::io::ErrorKind::AlreadyExists { return Err(MainError::IO(e, "creating
minimal dir")); } } and similarly await
tokio::fs::remove_file(cli.listen_on()).await and ignore NotFound, otherwise
return MainError::IO("socket already in use")); add the tokio::fs imports and
keep the rest (UnixListener::bind(...).map_err(|e| MainError::IO(...))?)
unchanged.
- Around line 145-147: The current main uses .await.unwrap() on
Server::run_on_uds(...).await which can panic; change main to return Result<(),
MainError> (or the existing MainError type) and propagate the std::io::Error by
mapping it into MainError instead of unwrapping—replace .await.unwrap() with
either ? (if MainError implements From<std::io::Error>) or .await.map_err(|e|
MainError::Io(e))? and return the Result from main so listener/runtime failures
are returned as typed MainError; update the signature of main and any callers
accordingly and reference Server::run_on_uds and the MainError type when making
the change.
In `@crates/minimald/src/server.rs`:
- Around line 64-70: The infinite accept loop spawns session_fut into
session_set without ever reaping completed tasks, causing unbounded
accumulation; modify the loop that calls session_set.spawn(session_fut) to also
drain completed tasks (e.g., call session_set.join_next()/try_join_next() in a
non-blocking/select branch or periodically poll and drop results) and ensure you
abort_all() on shutdown if needed; look for the JoinSet variable session_set and
the spawned future session_fut in Connection::from_socket to add the
join_next/try_join_next logic or an abort_all() cleanup path.
- Around line 58-60: Replace the panic-causing unwrap on host-key creation by
propagating KeyError into the existing IO Result path: change the usage of
self.config.host_key().unwrap() in creating russh::server::Config.keys to call
self.config.host_key().map_err(|e|
std::io::Error::new(std::io::ErrorKind::Other, e))? so the function returns Err
on failure (keep the surrounding function returning Result<(), std::io::Error>),
and update any signature if needed to allow ? propagation; additionally update
the program entry in main where Server::run_on_uds(...).await.unwrap() is called
so main returns a Result and uses ? on Server::run_on_uds(...).await instead of
unwrap(), allowing startup errors from Server::run_on_uds and Config::host_key()
to be returned rather than panicking.
---
Nitpick comments:
In `@crates/minimal2/src/main.rs`:
- Around line 47-48: Replace the two invocations that call .parse().unwrap()
inside the add_directive chain with .parse().expect(...) so failures produce a
clear invariant message; specifically update the calls around
add_directive("topiary=off".parse().unwrap()) and
add_directive("libcgroups=off".parse().unwrap()) to use expect with brief
reasons (e.g. "parsing constant directive 'topiary=off' must succeed" and
"parsing constant directive 'libcgroups=off' must succeed") so any panic
explains why the constant parse is guaranteed.
🪄 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: 028a6b10-14b3-4055-8fa2-2690e5cabfb8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/minimal2/Cargo.tomlcrates/minimal2/build.rscrates/minimal2/src/main.rscrates/minimald/Cargo.tomlcrates/minimald/build.rscrates/minimald/src/connection.rscrates/minimald/src/main.rscrates/minimald/src/server.rs
| } | ||
|
|
||
| #[tokio::main] | ||
| async fn main() -> Result<(), ()> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use application-grade error type for main.
Result<(), ()> drops error context and makes propagation/diagnostics weak; prefer anyhow::Result<()> at app boundaries.
Suggested fix
+use anyhow::Result;
@@
-async fn main() -> Result<(), ()> {
+async fn main() -> Result<()> {As per coding guidelines, "In application crates, use anyhow::Result and attach .context(...) / .with_context(|| ...) at layer boundaries".
📝 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.
Suggested change
| async fn main() -> Result<(), ()> { | |
| use anyhow::Result; | |
| async fn main() -> Result<()> { |
🤖 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/minimal2/src/main.rs` at line 44, The async function signature `async
fn main() -> Result<(), ()>` should return an application-grade error type;
change it to use `anyhow::Result<()>` (i.e., `async fn main() ->
anyhow::Result<()>`) and update call sites to propagate errors with
`.context(...)` or `.with_context(...)` at boundaries; also add the `anyhow`
dependency to the crate if missing and ensure any `Err(())` or `Err` returns are
converted to context-rich `anyhow::Error` values before propagating from `main`.
Comment on lines
+42
to
+45
| russh::server::run_stream(c, s, ConnectionHandler(h)) | ||
| .await | ||
| .unwrap(), | ||
| ) |
There was a problem hiding this comment.
Protocol setup error is unwrapped and can panic per connection.
Return/map this error into ConnectionError instead of panicking from from_socket.
As per coding guidelines, “Only use unwrap() and panic!() for broken invariants… never for recoverable conditions.”
🤖 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 42 - 45, The call to
russh::server::run_stream(...).await.unwrap() inside from_socket can panic
per-connection; change from_socket to propagate that error by replacing the
unwrap with proper error mapping/return: await the future, handle the Result
from russh::server::run_stream(...).await and convert its Err into your
ConnectionError (e.g. with map_err or ? after an Into<ConnectionError>
conversion), so ConnectionHandler and from_socket return Result<...,
ConnectionError> instead of panicking. Locate the unwrap in from_socket and
replace it with a mapped return of ConnectionError referencing
russh::server::run_stream and ConnectionHandler.
Comment on lines
+117
to
+132
| if let Err(e) = std::fs::create_dir_all(cli.minimal_dir()) | ||
| && e.kind() != std::io::ErrorKind::AlreadyExists | ||
| { | ||
| return Err(MainError::IO(e, "creating minimal dir")); | ||
| } | ||
|
|
||
| // If we got this far we need to launch minimald. | ||
| // | ||
| // Listen on the UDS socket. | ||
| if let Err(e) = std::fs::remove_file(cli.listen_on()) | ||
| && e.kind() != std::io::ErrorKind::NotFound | ||
| { | ||
| return Err(MainError::IO(e, "socket already in use")); | ||
| } | ||
| let listener = | ||
| UnixListener::bind(cli.listen_on()).map_err(|e| MainError::IO(e, "listening to socket"))?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and confirm async context + std::fs usage
FILE="crates/minimald/src/main.rs"
echo "== File header (imports + main) =="
sed -n '1,220p' "$FILE" | nl -ba | sed -n '1,220p'
echo
echo "== Lines 100-160 (target region) =="
sed -n '100,170p' "$FILE" | nl -ba
echo
echo "== rg: std::fs usage in file =="
rg -n "std::fs::" "$FILE" || true
echo
echo "== rg: tokio::fs usage in file =="
rg -n "tokio::fs::" "$FILE" || trueRepository: gominimal/minimal
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/minimald/src/main.rs"
echo "== File header (imports + main) =="
sed -n '1,220p' "$FILE" | cat -n
echo
echo "== Lines 100-170 (target region) =="
sed -n '100,170p' "$FILE" | cat -n
echo
echo "== rg: std::fs usage in file =="
rg -n "std::fs::" "$FILE" || true
echo
echo "== rg: tokio::fs usage in file =="
rg -n "tokio::fs::" "$FILE" || trueRepository: gominimal/minimal
Length of output: 7766
Use tokio::fs in async main to avoid blocking Tokio threads.
crates/minimald/src/main.rs uses std::fs::create_dir_all(...) (lines 117-121) and std::fs::remove_file(...) (lines 126-130) inside #[tokio::main] async fn main(). Switch these to tokio::fs::{create_dir_all, remove_file} and .await the calls.
🤖 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 117 - 132, Replace the blocking
std::fs calls inside the async main with their async equivalents: use
tokio::fs::create_dir_all and tokio::fs::remove_file and .await their Results;
map errors into MainError::IO as before (e.g. if let Err(e) =
tokio::fs::create_dir_all(cli.minimal_dir()).await { if e.kind() !=
std::io::ErrorKind::AlreadyExists { return Err(MainError::IO(e, "creating
minimal dir")); } } and similarly await
tokio::fs::remove_file(cli.listen_on()).await and ignore NotFound, otherwise
return MainError::IO("socket already in use")); add the tokio::fs imports and
keep the rest (UnixListener::bind(...).map_err(|e| MainError::IO(...))?)
unchanged.
Comment on lines
+145
to
+147
| .await | ||
| .unwrap(); | ||
|
|
There was a problem hiding this comment.
Avoid unwrap() on the server future result in main.
crates/minimald/src/main.rs currently does .await.unwrap() on Server::run_on_uds(...).await (a Result<(), std::io::Error>), which can panic the daemon on recoverable listener/runtime failures; propagate a typed MainError instead by mapping the std::io::Error into MainError and returning it.
🤖 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 145 - 147, The current main uses
.await.unwrap() on Server::run_on_uds(...).await which can panic; change main to
return Result<(), MainError> (or the existing MainError type) and propagate the
std::io::Error by mapping it into MainError instead of unwrapping—replace
.await.unwrap() with either ? (if MainError implements From<std::io::Error>) or
.await.map_err(|e| MainError::Io(e))? and return the Result from main so
listener/runtime failures are returned as typed MainError; update the signature
of main and any callers accordingly and reference Server::run_on_uds and the
MainError type when making the change.
Comment on lines
+58
to
+60
| let russh_config = Arc::new(russh::server::Config { | ||
| keys: vec![self.config.host_key().unwrap()], | ||
| auth_rejection_time_initial: Some(std::time::Duration::ZERO), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server.rs: context around lines 50-75 =="
nl -ba crates/minimald/src/server.rs | sed -n '45,80p'
echo
echo "== server.rs: locate host_key definition =="
rg -n "fn host_key\\b|host_key\\(" crates/minimald/src/server.rs
echo
echo "== main.rs: check how server startup result is handled =="
nl -ba crates/minimald/src/main.rs | sed -n '1,140p'
echo
echo "== Project-wide: find other unwrap() uses on server startup/config =="
rg -n "\\.unwrap\\(\\)" crates/minimald/src | head -n 50Repository: gominimal/minimal
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server.rs: context around lines 50-75 =="
awk 'NR>=45 && NR<=80 {printf "%5d:%s\n", NR, $0}' crates/minimald/src/server.rs
echo
echo "== server.rs: locate host_key definition =="
rg -n "fn host_key\\b|host_key\\(" crates/minimald/src/server.rs
echo
echo "== main.rs: check how server startup result is handled =="
awk 'NR>=1 && NR<=180 {printf "%5d:%s\n", NR, $0}' crates/minimald/src/main.rs
echo
echo "== Project-wide: find other unwrap() uses on server startup/config =="
rg -n "\\.unwrap\\(\\)" crates/minimald/src | head -n 80Repository: gominimal/minimal
Length of output: 7420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=1 && NR<=120 {printf "%5d:%s\n", NR, $0}' crates/minimald/src/server.rsRepository: gominimal/minimal
Length of output: 2770
Host-key generation errors should propagate instead of panicking in server startup
crates/minimald/src/server.rs uses self.config.host_key().unwrap() even though Config::host_key() returns Result<PrivateKey, KeyError> and can fail for HostKey::Ephemeral (PrivateKey::random(...) ?) or for HostKey::Path/Raw (file/parse errors). Map KeyError into the existing Result<(), std::io::Error> path and use ? instead of unwrap().
keys: vec![self.config.host_key().unwrap()],Also, crates/minimald/src/main.rs currently ends with .await.unwrap() on Server::run_on_uds(...), so startup errors will still panic unless main is adjusted to handle/return the 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/minimald/src/server.rs` around lines 58 - 60, Replace the
panic-causing unwrap on host-key creation by propagating KeyError into the
existing IO Result path: change the usage of self.config.host_key().unwrap() in
creating russh::server::Config.keys to call self.config.host_key().map_err(|e|
std::io::Error::new(std::io::ErrorKind::Other, e))? so the function returns Err
on failure (keep the surrounding function returning Result<(), std::io::Error>),
and update any signature if needed to allow ? propagation; additionally update
the program entry in main where Server::run_on_uds(...).await.unwrap() is called
so main returns a Result and uses ? on Server::run_on_uds(...).await instead of
unwrap(), allowing startup errors from Server::run_on_uds and Config::host_key()
to be returned rather than panicking.
Comment on lines
+64
to
+70
| let mut session_set = JoinSet::new(); | ||
| loop { | ||
| let (socket, _) = self.listener.accept().await?; | ||
| let (_conn_hnd, session_fut) = | ||
| Connection::from_socket(socket, russh_config.clone(), true).await; | ||
| session_set.spawn(session_fut); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion around the reported lines
sed -n '1,140p' crates/minimald/src/server.rs
# Find all JoinSet-related usage in this file
rg -n "JoinSet|join_next|try_join_next|abort_all|abort|len\(\)" crates/minimald/src/server.rs
# Also search the whole crate for JoinSet usage patterns in case draining happens elsewhere
rg -n "JoinSet" crates/minimaldRepository: gominimal/minimal
Length of output: 2552
Drain JoinSet (or otherwise collect/abort tasks) to prevent unbounded accumulation of completed task outputs.
session_set.spawn(session_fut); is in an infinite accept loop, but there are no join_next/try_join_next calls (or abort_all) to reap completed sessions, so their outputs can accumulate over time.
crates/minimald/src/server.rs:64-70
🤖 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/server.rs` around lines 64 - 70, The infinite accept loop
spawns session_fut into session_set without ever reaping completed tasks,
causing unbounded accumulation; modify the loop that calls
session_set.spawn(session_fut) to also drain completed tasks (e.g., call
session_set.join_next()/try_join_next() in a non-blocking/select branch or
periodically poll and drop results) and ensure you abort_all() on shutdown if
needed; look for the JoinSet variable session_set and the spawned future
session_fut in Connection::from_socket to add the join_next/try_join_next logic
or an abort_all() cleanup path.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
crates/minimal2/src/main.rs (2)
43-44: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
anyhow::Resultfor application main.Returning
Result<(), ()>discards all error context. As per coding guidelines, application crates should useanyhow::Resultand attach context at layer boundaries.♻️ Proposed fix
#[tokio::main] -async fn main() -> Result<(), ()> { +async fn main() -> anyhow::Result<()> {As per coding guidelines, "In application crates, use
anyhow::Resultand attach.context(...) / .with_context(|| ...)at layer boundaries".🤖 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/minimal2/src/main.rs` around lines 43 - 44, Change the main function to return anyhow::Result<()> instead of Result<(), ()> and import anyhow::Result at top; update the #[tokio::main] async fn main signature to use anyhow::Result and ensure any fallible calls inside main use .context(...) / .with_context(...) at layer boundaries before returning Err so errors preserve context (refer to the async fn main and any top-level error returns in main).
8-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand name doesn't match binary name.
Line 9 sets
name = "minimal"but this crate builds theminimal2binary. Shell completion scripts generated by line 62 will bind tominimalinstead ofminimal2, making them unusable with this executable.🔧 Proposed fix
#[derive(Parser)] -#[command(name = "minimal", version = env!("CARGO_PKG_VERSION"), long_version = env!("LONG_VERSION"))] +#[command(name = "minimal2", version = env!("CARGO_PKG_VERSION"), long_version = env!("LONG_VERSION"))] #[command(about = "The Minimal CLI")] struct Cli {🤖 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/minimal2/src/main.rs` around lines 8 - 17, The CLI Command name is hardcoded to "minimal" on the Cli struct (#[command(name = "minimal")]) which mismatches this crate's binary name; update the #[command(...)] attribute on struct Cli (or remove the name field) so the command name matches the actual binary (e.g., "minimal2") or let clap derive the binary name by omitting the name key; ensure this change also fixes generated shell completions that rely on the command name.crates/minimal2/build.rs (2)
20-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle git hash formatting without panicking.
Line 24 calls
.strip_suffix("\n").unwrap(), which panics if the git output doesn't end with a newline. This is part of the broader git-handling issue already flagged.🤖 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/minimal2/build.rs` around lines 20 - 25, The current println! in build.rs uses git_hash.strip_suffix("\n").unwrap() which can panic if git_hash doesn't end with a newline; change this to a non-panicking trim such as using git_hash.trim_end() (or trim_end_matches('\n')) when interpolating into the println! so the value is cleaned safely without unwrap and potential panic; update the println! invocation that references git_hash, is_clean, and CARGO_PKG_VERSION accordingly.
3-8:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle git command failures without panicking.
The git command can fail in non-git environments (e.g., source tarball, shallow clone). Current code panics on
.unwrap()for bothCommand::output()andString::from_utf8(). As per coding guidelines, only useunwrap()for broken invariants, never for recoverable conditions.🤖 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/minimal2/build.rs` around lines 3 - 8, The build script currently unwraps Command::new(...).output() and String::from_utf8(...), causing panics in non-git environments; change the logic around Command::new(...).args(...).output() and the git_hash variable so failures are handled gracefully (e.g., match or use .ok()/.and_then()), convert stdout with String::from_utf8_lossy or handle the UTF-8 error and default to a safe value like "unknown", trim whitespace/newline from the resulting string, then call println!("cargo:rustc-env=GIT_HASH={}", git_hash) with that safe value instead of unwrapping.Cargo.toml (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
russhto an exact version.The workspace dependency uses a semver range
"0.61"instead of an exact pin like"=0.61.1". As per coding guidelines, pin dependency versions in the workspace Cargo.toml.📌 Proposed fix
-russh = "0.61" +russh = "=0.61.1"As per coding guidelines, "Pin dependency versions in the workspace Cargo.toml; individual crate-level Cargo.tomls should inherit via
workspace = trueand never specify their own versions".🤖 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 `@Cargo.toml` at line 78, Update the workspace dependency declaration for russh from a semver range to an exact pinned version: change the version string for russh to an exact pin (e.g., "=0.61.1") in the workspace Cargo.toml so crate members inherit the pinned version via workspace = true rather than using a range.crates/minimald/src/connection.rs (1)
42-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t panic on per-connection protocol setup failure.
Line 44 unwraps
run_stream(...).await; returnResultfromfrom_socketand map the error intoConnectionErrorinstead.As per coding guidelines, “Only use
unwrap()andpanic!()for broken invariants… never for recoverable conditions.”🤖 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 42 - 45, The call to russh::server::run_stream(...).await is currently unwrapped inside from_socket which will panic on recoverable protocol setup failures; change from_socket to return a Result (e.g., Result<_, ConnectionError>), remove the .unwrap(), and map any error from run_stream into the ConnectionError type (propagate with ? or map_err) so ConnectionHandler and callers can handle the failure instead of panicking.crates/minimald/src/main.rs (2)
145-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate server runtime errors instead of panicking in
main.Line 146 unwraps a recoverable
ResultfromServer::run_on_uds(...).As per coding guidelines, “Only use
unwrap()andpanic!()for broken invariants… never for recoverable conditions.”🤖 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 145 - 146, Replace the .await.unwrap() on Server::run_on_uds(...) with proper error propagation: change main to return a Result (e.g., fn main() -> Result<(), Box<dyn std::error::Error>> or use tokio::main async fn main() -> anyhow::Result<()>), then use the ? operator on the await call (i.e., .await?) or explicitly map/return the Err; this ensures Server::run_on_uds(...) errors are propagated instead of causing a panic. Ensure you update any imports/signature (anyhow or Box<dyn Error>) to match the chosen error type.
117-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
tokio::fsin asyncmainto avoid blocking runtime threads.Line 117 and Line 126 perform blocking filesystem calls inside
#[tokio::main].As per coding guidelines, “In async contexts, avoid blocking operations: do not use std::fs … use tokio::fs …”.
🤖 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 117 - 130, Replace the blocking std::fs calls in async main with their async tokio equivalents: call tokio::fs::create_dir_all(cli.minimal_dir()).await and tokio::fs::remove_file(cli.listen_on()).await instead of std::fs::create_dir_all and std::fs::remove_file; adapt the error handling around the awaited Results so you inspect the io::ErrorKind on the Err(e) from the await (e.g., match or if let Err(e) = ... { if e.kind() != ErrorKind::AlreadyExists { return Err(MainError::IO(e, "creating minimal dir")); } }) and similarly check ErrorKind::NotFound for the remove_file case, ensuring you pass the awaited error into MainError::IO.crates/minimald/src/server.rs (2)
58-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate host-key construction errors instead of unwrapping.
Line 59 can panic on invalid key material or generation/read failures; return an
io::Errorfromrun()instead.As per coding guidelines, “Only use
unwrap()andpanic!()for broken invariants… never for recoverable conditions.”🤖 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/server.rs` around lines 58 - 60, The code currently calls self.config.host_key().unwrap() while building the russh::server::Config in run(), which can panic on invalid or failed key construction; change run() to return a Result (e.g., std::io::Result<...>) and replace the unwrap with propagation (use ? or map_err to convert the host_key() error into an io::Error), then use the propagated key when constructing russh_config so failures return an io::Error instead of panicking.
64-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrain spawned sessions from
JoinSetto avoid unbounded accumulation.The accept loop keeps spawning tasks but never reaps completed entries (
join_next/try_join_next), so completed task outputs can pile up.🤖 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/server.rs` around lines 64 - 70, The loop spawns tasks into session_set (JoinSet) but never reaps completed tasks; modify the accept loop that creates Connection::from_socket and session_set.spawn(session_fut) to also poll and drain session_set (e.g., call session_set.join_next() or try_join_next() in a non-blocking/select manner) so finished sessions are awaited and removed; implement this using tokio::select! (or equivalent) between self.listener.accept() and session_set.join_next() to both accept new connections and reap completed session_fut results, handling any errors from the joined tasks.crates/minimald/build.rs (1)
3-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle git metadata fallibility without panics (and trim hash once).
Line 6, Line 7, and Line 24 can panic on recoverable build-time conditions, and Line 8 currently exports a newline-tainted hash.
Suggested fix
fn main() { - let output = Command::new("git") + let git_hash = Command::new("git") .args(["rev-parse", "--short", "HEAD"]) .output() - .unwrap(); - let git_hash = String::from_utf8(output.stdout).unwrap(); - println!("cargo:rustc-env=GIT_HASH={}", git_hash); + .ok() + .filter(|o| o.status.success()) + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_owned()) + .filter(|s| !s.is_empty()) + .unwrap_or_else(|| "unknown".to_string()); + println!("cargo:rustc-env=GIT_HASH={git_hash}"); @@ - git_hash.strip_suffix("\n").unwrap(), + git_hash, ); }As per coding guidelines, “Only use
unwrap()andpanic!()for broken invariants; in production code useexpect("why the invariant holds")instead of unwrap; never for recoverable conditions.”Also applies to: 24-24
🤖 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/build.rs` around lines 3 - 8, The build script currently calls Command::new("git") and uses unwrap() on the output and on String::from_utf8, and prints git_hash with a trailing newline; change this to robust, non-panicking handling: run the git rev-parse command and check output.status.success() (or match the Result) instead of unwrap(), attempt to decode output.stdout with String::from_utf8 and handle the Err by falling back to a safe value like "unknown", and always trim the decoded hash via .trim() before assigning git_hash so the exported GIT_HASH has no newline; update the code around the Command invocation, the output variable, and the git_hash assignment/println! to implement these checks and fallbacks (referencing the Command::new(...) call, the output variable, and the git_hash/println! usage).
🧹 Nitpick comments (3)
crates/minimal2/src/main.rs (1)
45-54: 💤 Low valueUse
expectfor hardcoded parse directives.Lines 47-48 call
.unwrap()on.parse()for hardcoded string literals"topiary=off"and"libcgroups=off". These represent broken invariants if they fail to parse. Replace with.expect("hardcoded directive must parse")to clarify intent.♻️ Proposed fix
let filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| { EnvFilter::new("info") - .add_directive("topiary=off".parse().unwrap()) - .add_directive("libcgroups=off".parse().unwrap()) + .add_directive("topiary=off".parse().expect("hardcoded directive must parse")) + .add_directive("libcgroups=off".parse().expect("hardcoded directive must parse")) });As per coding guidelines, "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/minimal2/src/main.rs` around lines 45 - 54, Replace the .parse().unwrap() calls used when adding hardcoded directives to the EnvFilter so failures clearly express the invariant; in the block that constructs the EnvFilter (symbol: EnvFilter::try_from_default_env and method chain .add_directive(...parse()...)), change both .parse().unwrap() invocations for the literals "topiary=off" and "libcgroups=off" to .parse().expect("hardcoded directive must parse") so the intention and invariant are explicit.crates/minimal2/build.rs (1)
15-15: ⚡ Quick winUse
expectwith message for Cargo environment variables.
PROFILEandCARGO_PKG_VERSIONare set by Cargo and represent broken invariants if missing. Replace.unwrap()with.expect("message explaining the invariant")to aid debugging if the impossible happens.♻️ Proposed fix
- if !is_clean && std::env::var("PROFILE").unwrap() == "release" { + if !is_clean && std::env::var("PROFILE").expect("PROFILE set by cargo") == "release" { println!("cargo::warning=Building a release binary from an unclean working tree!!"); println!("cargo::warning=Always build production binaries from a clean checkout."); } println!( "cargo:rustc-env=LONG_VERSION={} ({}{})", - std::env::var("CARGO_PKG_VERSION").unwrap().as_str(), + std::env::var("CARGO_PKG_VERSION").expect("CARGO_PKG_VERSION set by cargo").as_str(), if is_clean { "" } else { "dirty " }, git_hash.strip_suffix("\n").unwrap(), );As per coding guidelines, "use
expect("why the invariant holds")instead of unwrap".Also applies to: 22-22
🤖 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/minimal2/build.rs` at line 15, Replace usages of std::env::var(...).unwrap() with std::env::var(...).expect("...") to document the Cargo-provided invariant; specifically change the PROFILE check in the conditional that uses is_clean (std::env::var("PROFILE")) and the other occurrence that reads CARGO_PKG_VERSION (std::env::var("CARGO_PKG_VERSION")) so each expect message explains why the variable must be present (e.g., "PROFILE is set by Cargo" / "CARGO_PKG_VERSION is set by Cargo") and keep the surrounding logic in the if block unchanged.crates/minimald/src/connection.rs (1)
16-17: ⚡ Quick winDocument or remove
#[allow(dead_code)]suppressions.These lint suppressions need an inline justification comment, or should be removed if temporary scaffolding is no longer needed.
As per coding guidelines, “Justify any
#[allow(...)]attributes with a comment explaining why the lint is suppressed.”Also applies to: 55-56
🤖 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 16 - 17, The #[allow(dead_code)] on the Connection struct (and the other identical suppression at the later occurrence) must either be removed if the code is used, or annotated with a brief justification; update the attribute above pub struct Connection (and the other #[allow(dead_code)]) to either delete it when the struct/items are actually referenced, or replace it with an inline comment like // Allow dead_code: used only in tests/feature-flagged builds (or similar), so authors and linters understand why the suppression exists. Ensure the justification mentions the specific reason (e.g., "only referenced in tests" or "placeholder for future API") and keep the attribute immediately adjacent to the item it documents (the Connection struct and the other suppressed item).
🤖 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/minimal2/build.rs`:
- Around line 10-14: The current is_clean computation reads o.stdout.is_empty()
without verifying the git Command succeeded; change the mapping for the
Command::output() result used to set is_clean (the closure handling the Output
from Command::new("git")...output()) to first test o.status.success() and only
then check o.stdout.is_empty(), e.g. produce true only when both
o.status.success() and o.stdout.is_empty() are true, and treat failures
(non-success status or Err from output()) as not clean.
In `@crates/minimald/src/main.rs`:
- Around line 31-33: The fallback PathBuf constructed with
PathBuf::from("~/.cache") will not expand '~'; replace that fallback so it uses
the real home directory: call dirs::home_dir(), map it to home.join(".cache"),
and then fallback to a sensible local path if home isn't available, then
.join("minimal"); in other words, change the expression using dirs::cache_dir()
/ PathBuf::from("~/.cache") / .join("minimal") to use dirs::home_dir().map(|h|
h.join(".cache")).unwrap_or_else(|| PathBuf::from(".cache")).join("minimal") (or
similar) so the cache path resolves to $HOME/.cache/minimal instead of a literal
~/.cache/minimal.
- Around line 110-115: The code currently moves cli.command when matching (if
let Command::Completions(...) = cli.command) causing later uses of cli to fail;
change the pattern to match by reference (e.g., match &cli.command or if let
Command::Completions(CompletionsArgs { shell }) = &cli.command) so cli is not
moved, and adjust how you pass shell into clap_complete::generate (use a
borrowed or cloned/ dereferenced shell as appropriate). Apply the same
reference-based match fix to the other occurrence (the block around
Command::Completions at lines 141-144).
---
Duplicate comments:
In `@Cargo.toml`:
- Line 78: Update the workspace dependency declaration for russh from a semver
range to an exact pinned version: change the version string for russh to an
exact pin (e.g., "=0.61.1") in the workspace Cargo.toml so crate members inherit
the pinned version via workspace = true rather than using a range.
In `@crates/minimal2/build.rs`:
- Around line 20-25: The current println! in build.rs uses
git_hash.strip_suffix("\n").unwrap() which can panic if git_hash doesn't end
with a newline; change this to a non-panicking trim such as using
git_hash.trim_end() (or trim_end_matches('\n')) when interpolating into the
println! so the value is cleaned safely without unwrap and potential panic;
update the println! invocation that references git_hash, is_clean, and
CARGO_PKG_VERSION accordingly.
- Around line 3-8: The build script currently unwraps Command::new(...).output()
and String::from_utf8(...), causing panics in non-git environments; change the
logic around Command::new(...).args(...).output() and the git_hash variable so
failures are handled gracefully (e.g., match or use .ok()/.and_then()), convert
stdout with String::from_utf8_lossy or handle the UTF-8 error and default to a
safe value like "unknown", trim whitespace/newline from the resulting string,
then call println!("cargo:rustc-env=GIT_HASH={}", git_hash) with that safe value
instead of unwrapping.
In `@crates/minimal2/src/main.rs`:
- Around line 43-44: Change the main function to return anyhow::Result<()>
instead of Result<(), ()> and import anyhow::Result at top; update the
#[tokio::main] async fn main signature to use anyhow::Result and ensure any
fallible calls inside main use .context(...) / .with_context(...) at layer
boundaries before returning Err so errors preserve context (refer to the async
fn main and any top-level error returns in main).
- Around line 8-17: The CLI Command name is hardcoded to "minimal" on the Cli
struct (#[command(name = "minimal")]) which mismatches this crate's binary name;
update the #[command(...)] attribute on struct Cli (or remove the name field) so
the command name matches the actual binary (e.g., "minimal2") or let clap derive
the binary name by omitting the name key; ensure this change also fixes
generated shell completions that rely on the command name.
In `@crates/minimald/build.rs`:
- Around line 3-8: The build script currently calls Command::new("git") and uses
unwrap() on the output and on String::from_utf8, and prints git_hash with a
trailing newline; change this to robust, non-panicking handling: run the git
rev-parse command and check output.status.success() (or match the Result)
instead of unwrap(), attempt to decode output.stdout with String::from_utf8 and
handle the Err by falling back to a safe value like "unknown", and always trim
the decoded hash via .trim() before assigning git_hash so the exported GIT_HASH
has no newline; update the code around the Command invocation, the output
variable, and the git_hash assignment/println! to implement these checks and
fallbacks (referencing the Command::new(...) call, the output variable, and the
git_hash/println! usage).
In `@crates/minimald/src/connection.rs`:
- Around line 42-45: The call to russh::server::run_stream(...).await is
currently unwrapped inside from_socket which will panic on recoverable protocol
setup failures; change from_socket to return a Result (e.g., Result<_,
ConnectionError>), remove the .unwrap(), and map any error from run_stream into
the ConnectionError type (propagate with ? or map_err) so ConnectionHandler and
callers can handle the failure instead of panicking.
In `@crates/minimald/src/main.rs`:
- Around line 145-146: Replace the .await.unwrap() on Server::run_on_uds(...)
with proper error propagation: change main to return a Result (e.g., fn main()
-> Result<(), Box<dyn std::error::Error>> or use tokio::main async fn main() ->
anyhow::Result<()>), then use the ? operator on the await call (i.e., .await?)
or explicitly map/return the Err; this ensures Server::run_on_uds(...) errors
are propagated instead of causing a panic. Ensure you update any
imports/signature (anyhow or Box<dyn Error>) to match the chosen error type.
- Around line 117-130: Replace the blocking std::fs calls in async main with
their async tokio equivalents: call
tokio::fs::create_dir_all(cli.minimal_dir()).await and
tokio::fs::remove_file(cli.listen_on()).await instead of std::fs::create_dir_all
and std::fs::remove_file; adapt the error handling around the awaited Results so
you inspect the io::ErrorKind on the Err(e) from the await (e.g., match or if
let Err(e) = ... { if e.kind() != ErrorKind::AlreadyExists { return
Err(MainError::IO(e, "creating minimal dir")); } }) and similarly check
ErrorKind::NotFound for the remove_file case, ensuring you pass the awaited
error into MainError::IO.
In `@crates/minimald/src/server.rs`:
- Around line 58-60: The code currently calls self.config.host_key().unwrap()
while building the russh::server::Config in run(), which can panic on invalid or
failed key construction; change run() to return a Result (e.g.,
std::io::Result<...>) and replace the unwrap with propagation (use ? or map_err
to convert the host_key() error into an io::Error), then use the propagated key
when constructing russh_config so failures return an io::Error instead of
panicking.
- Around line 64-70: The loop spawns tasks into session_set (JoinSet) but never
reaps completed tasks; modify the accept loop that creates
Connection::from_socket and session_set.spawn(session_fut) to also poll and
drain session_set (e.g., call session_set.join_next() or try_join_next() in a
non-blocking/select manner) so finished sessions are awaited and removed;
implement this using tokio::select! (or equivalent) between
self.listener.accept() and session_set.join_next() to both accept new
connections and reap completed session_fut results, handling any errors from the
joined tasks.
---
Nitpick comments:
In `@crates/minimal2/build.rs`:
- Line 15: Replace usages of std::env::var(...).unwrap() with
std::env::var(...).expect("...") to document the Cargo-provided invariant;
specifically change the PROFILE check in the conditional that uses is_clean
(std::env::var("PROFILE")) and the other occurrence that reads CARGO_PKG_VERSION
(std::env::var("CARGO_PKG_VERSION")) so each expect message explains why the
variable must be present (e.g., "PROFILE is set by Cargo" / "CARGO_PKG_VERSION
is set by Cargo") and keep the surrounding logic in the if block unchanged.
In `@crates/minimal2/src/main.rs`:
- Around line 45-54: Replace the .parse().unwrap() calls used when adding
hardcoded directives to the EnvFilter so failures clearly express the invariant;
in the block that constructs the EnvFilter (symbol:
EnvFilter::try_from_default_env and method chain .add_directive(...parse()...)),
change both .parse().unwrap() invocations for the literals "topiary=off" and
"libcgroups=off" to .parse().expect("hardcoded directive must parse") so the
intention and invariant are explicit.
In `@crates/minimald/src/connection.rs`:
- Around line 16-17: The #[allow(dead_code)] on the Connection struct (and the
other identical suppression at the later occurrence) must either be removed if
the code is used, or annotated with a brief justification; update the attribute
above pub struct Connection (and the other #[allow(dead_code)]) to either delete
it when the struct/items are actually referenced, or replace it with an inline
comment like // Allow dead_code: used only in tests/feature-flagged builds (or
similar), so authors and linters understand why the suppression exists. Ensure
the justification mentions the specific reason (e.g., "only referenced in tests"
or "placeholder for future API") and keep the attribute immediately adjacent to
the item it documents (the Connection struct and the other suppressed item).
🪄 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: d3192761-7bfd-4bb3-8fbf-468769c45f5f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/minimal2/Cargo.tomlcrates/minimal2/build.rscrates/minimal2/src/main.rscrates/minimald/Cargo.tomlcrates/minimald/build.rscrates/minimald/src/connection.rscrates/minimald/src/main.rscrates/minimald/src/server.rsdeny.toml
✅ Files skipped from review due to trivial changes (2)
- crates/minimal2/Cargo.toml
- crates/minimald/Cargo.toml
Comment on lines
+10
to
+14
| let is_clean = std::process::Command::new("git") | ||
| .args(["status", "--porcelain=v1"]) | ||
| .output() | ||
| .map(|o| o.stdout.is_empty()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Check git command success before reading stdout.
Line 13 checks o.stdout.is_empty() but doesn't verify o.status.success(). If the git command fails, stdout might be empty but that doesn't mean the tree is clean—it means the command failed.
🛡️ Proposed fix
let is_clean = std::process::Command::new("git")
.args(["status", "--porcelain=v1"])
.output()
- .map(|o| o.stdout.is_empty())
+ .map(|o| o.status.success() && o.stdout.is_empty())
.unwrap_or(false);📝 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.
Suggested change
| let is_clean = std::process::Command::new("git") | |
| .args(["status", "--porcelain=v1"]) | |
| .output() | |
| .map(|o| o.stdout.is_empty()) | |
| .unwrap_or(false); | |
| let is_clean = std::process::Command::new("git") | |
| .args(["status", "--porcelain=v1"]) | |
| .output() | |
| .map(|o| o.status.success() && o.stdout.is_empty()) | |
| .unwrap_or(false); |
🤖 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/minimal2/build.rs` around lines 10 - 14, The current is_clean
computation reads o.stdout.is_empty() without verifying the git Command
succeeded; change the mapping for the Command::output() result used to set
is_clean (the closure handling the Output from Command::new("git")...output())
to first test o.status.success() and only then check o.stdout.is_empty(), e.g.
produce true only when both o.status.success() and o.stdout.is_empty() are true,
and treat failures (non-success status or Err from output()) as not clean.
Comment on lines
+31
to
+33
| dirs::cache_dir() | ||
| .unwrap_or_else(|| PathBuf::from("~/.cache")) | ||
| .join("minimal") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the referenced lines
ls -la crates/minimald/src/main.rs
nl -ba crates/minimald/src/main.rs | sed -n '1,120p'
# Search for cache_dir and "~/.cache" usage across the repo
rg -n "dirs::cache_dir\(|~\/\.cache|~/.cache" -S .
# Check Cargo.toml for deps (dirs, etc.)
ls -la crates/minimald/Cargo.toml
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la crates/minimald/src/main.rs
nl -ba crates/minimald/src/main.rs | sed -n '1,140p'
rg -n "dirs::cache_dir\(|~\/\.cache|~/.cache" -S crates/minimald
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 199
🏁 Script executed:
# Actual execution
set -euo pipefail
ls -la crates/minimald/src/main.rs
nl -ba crates/minimald/src/main.rs | sed -n '1,140p'
rg -n "dirs::cache_dir\(|~\/\.cache|~/.cache" -S crates/minimald
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la crates/minimald/src/main.rs
nl -ba crates/minimald/src/main.rs | sed -n '1,160p'
echo "---- repo-wide cache_dir + ~/.cache hits ----"
rg -n "dirs::cache_dir\(|~\/\.cache|~/.cache" -S crates
echo "---- crates/minimald Cargo.toml ----"
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 199
🏁 Script executed:
trueRepository: gominimal/minimal
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- crates/minimald/src/main.rs (numbered) ----"
cat -n crates/minimald/src/main.rs | sed -n '1,160p'
echo "---- hits for literal \"~/.cache\" and dirs::cache_dir() in crates/minimald ----"
rg -n --hidden --glob '!**/target/**' 'dirs::cache_dir\(|~\/\.cache|~/.cache' crates/minimald
echo "---- hits for dirs::home_dir() in crates/minimald ----"
rg -n --hidden --glob '!**/target/**' 'dirs::home_dir\(\)' crates/minimald
echo "---- repo-wide hits for \"~/.cache\" ----"
rg -n --hidden --glob '!**/target/**' '~/.cache' .
echo "---- repo-wide hits for dirs::home_dir() ----"
rg -n --hidden --glob '!**/target/**' 'dirs::home_dir\(\)' .
echo "---- crates/minimald/Cargo.toml ----"
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 5923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- hits for dirs::cache_dir() in crates/minimald ----"
rg -n --hidden --glob '!**/target/**' 'dirs::cache_dir\(\)' crates/minimald
echo "---- hits for literal \"~/.cache\" in crates/minimald ----"
rg -n --hidden --glob '!**/target/**' '~/.cache' crates/minimald
echo "---- hits for dirs::home_dir() in crates/minimald ----"
rg -n --hidden --glob '!**/target/**' 'dirs::home_dir\(\)' crates/minimald
echo "---- hits for literal \"~/.cache\" repo-wide ----"
rg -n --hidden --glob '!**/target/**' '~/.cache' .
echo "---- crates/minimald/Cargo.toml ----"
cat crates/minimald/Cargo.tomlRepository: gominimal/minimal
Length of output: 498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- crates/minimald/Cargo.toml ----"
cat crates/minimald/Cargo.toml
echo "---- repo-wide hits for dirs::home_dir() ----"
rg -n --hidden --glob '!**/target/**' 'dirs::home_dir\(\)' .Repository: gominimal/minimal
Length of output: 536
"~/.cache" fallback is not expanded and will use a literal ~ directory.
At lines 31-33, PathBuf::from("~/.cache") does not expand ~, so the cache path becomes wrong when dirs::cache_dir() returns None. Build the fallback from dirs::home_dir() (e.g., $HOME/.cache) instead.
🤖 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 31 - 33, The fallback PathBuf
constructed with PathBuf::from("~/.cache") will not expand '~'; replace that
fallback so it uses the real home directory: call dirs::home_dir(), map it to
home.join(".cache"), and then fallback to a sensible local path if home isn't
available, then .join("minimal"); in other words, change the expression using
dirs::cache_dir() / PathBuf::from("~/.cache") / .join("minimal") to use
dirs::home_dir().map(|h| h.join(".cache")).unwrap_or_else(||
PathBuf::from(".cache")).join("minimal") (or similar) so the cache path resolves
to $HOME/.cache/minimal instead of a literal ~/.cache/minimal.
norrietaylor
approved these changes
May 25, 2026
This was referenced May 26, 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.
Fixes: gominimal/inbox#157
minimal2, the Minimal One client, with a tokio executor + CLI args/commands scaffoldingminimald, the Minimal One daemon, with a tokio executor, CLI args/commands scaffolding, barebones ssh service listening, and the beginnings of managing the lifecycle/state of the connectionSummary by CodeRabbit
New Features
Chores