refactor(minimald): coalesce code paths between UDS and vsock listeners - #400
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughUnifies server transport via a Listener trait, uses Connection::from_stream for all accept paths, centralizes micro‑VM guest mount/enter_rootfs flow (fixed /newroot), adds a Linux nix workspace flag, and adds AbsPath -> CwdRelative conversion. ChangesVSOCK and Micro-VM Server Refactor
Sequence DiagramsequenceDiagram
participant Init as InitProcess
participant Main as async_main
participant Guest as guest_setup
participant Keys as known_hosts_writer
participant Server as Server::run
Init->>Main: detect_microvm_mode
Main->>Guest: mount_dev_and_enter_rootfs
Guest-->>Main: setup_result
Main->>Keys: learn_host_key_for_instance
Keys-->>Main: known_hosts_path_ready
Main->>Server: start_with_uds_or_vsock_listener
Server->>Server: accept() -> (stream, addr)
Server->>Server: Connection::from_stream(stream, L::IS_LOCAL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
twitchyliquid64
force-pushed
the
tom/minimald
branch
from
June 12, 2026 21:12
a673b86 to
6b80418
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/test_harness.rs (1)
83-84: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale constructor reference in comment.
Line 83 still references
Connection::from_socket, but the code now usesConnection::from_stream. Keeping this aligned avoids confusion during future transport refactors.Suggested edit
- // `russh::server::run_stream` (called inside `Connection::from_socket`) + // `russh::server::run_stream` (called inside `Connection::from_stream`)🤖 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/test_harness.rs` around lines 83 - 84, The comment referencing the old constructor is stale: update the text that mentions `Connection::from_socket` to instead reference `Connection::from_stream` (the function currently used) so the comment aligns with the actual code path (look for the comment block mentioning `russh::server::run_stream` and change the constructor name accordingly).
🧹 Nitpick comments (1)
crates/paths/src/lib.rs (1)
962-966: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a focused unit test for this new conversion path.
This new public
From<AbsPath<R>>path is untested directly; adding one assertion here would lock in behavior and prevent regressions.Proposed test addition
@@ #[test] fn cwd_relative_resolve_passes_absolute_through_unchanged() { let cli: CwdRelative<Host> = "/etc/minimal".parse().unwrap(); let resolved = cli .resolve() .expect("resolve does not consult cwd for abs paths"); assert_eq!(resolved.as_str(), "/etc/minimal"); } + + #[test] + fn cwd_relative_from_abs_path_preserves_abs_variant() { + let abs = HostAbsPath::try_new("/etc/minimal").unwrap(); + let cli: CwdRelative<Host> = abs.clone().into(); + assert!(cli.is_absolute()); + assert_eq!(cli.as_either().as_abs(), Some(&abs)); + assert_eq!(cli.resolve().unwrap(), abs); + }🤖 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/paths/src/lib.rs` around lines 962 - 966, Add a unit test that constructs an AbsPath (for a concrete R used by other tests), converts it via CwdRelative::from(abs) or abs.into(), and asserts the resulting CwdRelative contains the original AbsPath wrapped in EitherPath::Abs; reference the types AbsPath, CwdRelative, the From<AbsPath<R>> impl, and the EitherPath::Abs variant so the test verifies the conversion path and value are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/server.rs`:
- Around line 194-208: The JoinSet named session_set is never drained so
completed session tasks accumulate; modify the accept loop that spawns
session_set.spawn(...) to also poll and remove completed tasks (e.g., call
session_set.join_next() or a non-blocking poll variant inside the loop or via
tokio::select) and handle their results (log errors already done for
session_fut) so entries are removed from session_set; locate the accept loop
around listener.accept().await and the Connection::from_stream(...) ->
session_fut to add periodic draining of session_set to prevent unbounded growth.
---
Outside diff comments:
In `@crates/minimald/src/test_harness.rs`:
- Around line 83-84: The comment referencing the old constructor is stale:
update the text that mentions `Connection::from_socket` to instead reference
`Connection::from_stream` (the function currently used) so the comment aligns
with the actual code path (look for the comment block mentioning
`russh::server::run_stream` and change the constructor name accordingly).
---
Nitpick comments:
In `@crates/paths/src/lib.rs`:
- Around line 962-966: Add a unit test that constructs an AbsPath (for a
concrete R used by other tests), converts it via CwdRelative::from(abs) or
abs.into(), and asserts the resulting CwdRelative contains the original AbsPath
wrapped in EitherPath::Abs; reference the types AbsPath, CwdRelative, the
From<AbsPath<R>> impl, and the EitherPath::Abs variant so the test verifies the
conversion path and value are preserved.
🪄 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: a7a092c2-44e3-472a-8549-a01a5c30b729
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/minimald/Cargo.tomlcrates/minimald/src/connection.rscrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minimald/src/server.rscrates/minimald/src/test_harness.rscrates/paths/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/minimald/src/connection.rs
twitchyliquid64
force-pushed
the
tom/minimald
branch
from
June 12, 2026 21:22
6b80418 to
c7d808f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/guest.rs`:
- Line 124: The log message incorrectly states "pivot_root" while the code
actually calls libc::chroot; update the tracing::info! invocation to accurately
reflect the operation (e.g., "switched to upstream rootfs (chroot)" or "changed
rootfs via chroot") so logs match the libc::chroot call and avoid misleading
debugging; locate the tracing::info! call near the libc::chroot usage in
guest.rs and change the message string accordingly.
🪄 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: b7a1b341-512c-4ae2-b444-d6e429e5c693
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/minimald/Cargo.tomlcrates/minimald/src/connection.rscrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minimald/src/server.rscrates/minimald/src/test_harness.rscrates/paths/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/minimald/src/connection.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/paths/src/lib.rs
- crates/minimald/src/test_harness.rs
- crates/minimald/Cargo.toml
- crates/minimald/src/main.rs
- crates/minimald/src/server.rs
| std::env::set_current_dir("/")?; | ||
| tracing::info!(device, "entered upstream rootfs (chroot)"); | ||
|
|
||
| tracing::info!(device, "switched to upstream rootfs (pivot_root)"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix misleading root-switch log text.
Line 124 reports pivot_root, but this function calls libc::chroot (Line 119). Please align the message with actual behavior to avoid misleading ops/debugging signals.
Suggested patch
- tracing::info!(device, "switched to upstream rootfs (pivot_root)");
+ tracing::info!(device, "entered upstream rootfs (chroot)");📝 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
| tracing::info!(device, "switched to upstream rootfs (pivot_root)"); | |
| tracing::info!(device, "entered upstream rootfs (chroot)"); |
🤖 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/guest.rs` at line 124, The log message incorrectly states
"pivot_root" while the code actually calls libc::chroot; update the
tracing::info! invocation to accurately reflect the operation (e.g., "switched
to upstream rootfs (chroot)" or "changed rootfs via chroot") so logs match the
libc::chroot call and avoid misleading debugging; locate the tracing::info! call
near the libc::chroot usage in guest.rs and change the message string
accordingly.
twitchyliquid64
force-pushed
the
tom/minimald
branch
from
June 13, 2026 03:41
c7d808f to
b7e244b
Compare
norrietaylor
approved these changes
Jun 13, 2026
twitchyliquid64
force-pushed
the
tom/minimald
branch
from
June 13, 2026 03:50
b7e244b to
80d7d01
Compare
twitchyliquid64
enabled auto-merge (rebase)
June 13, 2026 04:00
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.
Servermethods generic over different kinds of listenersknown_hostsfile so clients can connect without disabling host key verification + the associated warningSummary by CodeRabbit
Refactor
Improvements
Tests
Chores