Skip to content

Integrate libkrun backend: fix launcher bugs and add doctor diagnostics#370

Merged
aniketmaurya merged 10 commits into
CelestoAI:mainfrom
SauravMishraaa:feature/libkrun-integration
Jun 16, 2026
Merged

Integrate libkrun backend: fix launcher bugs and add doctor diagnostics#370
aniketmaurya merged 10 commits into
CelestoAI:mainfrom
SauravMishraaa:feature/libkrun-integration

Conversation

@SauravMishraaa

@SauravMishraaa SauravMishraaa commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Integrate libkrun as third VM backend

Summary

Complete the libkrun integration, adding KVM-based process isolation via Hypervisor.framework on macOS and KVM on Linux as a third runtime backend alongside QEMU and Firecracker.

Key changes:

  • Fix launcher bugs: move variable definitions before use, use -listen-vfkit unixgram:// (vfkit protocol, not HTTP REST) for gvproxy networking on macOS
  • Detect that libkrun on Darwin requires the ARM64 Image kernel format (Hypervisor.framework rejects ELF), while Linux libkrun uses ELF like Firecracker
  • Replace removed _find_krunvm_binary with _find_libkrun_library for ctypes FFI library loading
  • Extend smolvm doctor --backend libkrun with comprehensive prerequisite checks: gvproxy (macOS), Rust musl target, hypervisor entitlement (macOS), KVM (Linux)

Result: VMs now boot end-to-end with SSH access. Each doctor failure includes an actionable fix command.

Related Issues

Testing

  • smolvm create --backend libkrun — VM boots, SSH accessible
  • smolvm ssh sbx-riemann — shell works end-to-end
  • smolvm doctor --backend libkrun — all checks pass on macOS ARM64
  • uv run pytest tests/test_custom_boot_api.py::TestEnsureBaseKernelForBackend tests/test_vm.py::TestSmolVMCreate::test_check_prerequisites_libkrun_only_checks_library_and_ssh — 6 passed
  • ✅ Full suite: 1307 passed, 3 pre-existing failures (macOS Unix socket path length limit, unrelated to this work)

Checklist

  • Scope is focused and limited to this change
  • Tests added/updated for behavior changes (platform-dependent kernel format, library prerequisite check)
  • Docs updated in CLAUDE.md (CLAUDE.md already documents the three-backend model)
  • No secrets or sensitive data included
  • Code follows SmolVM style (NOUN-VERB CLI structure, progressive disclosure in doctor output, no half-finished implementations)

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced diagnostic checks for libkrun backend including gvproxy availability, Rust target validation, and macOS hypervisor entitlement verification
    • Native libkrun library integration for improved VM startup
  • Improvements

    • Better macOS support with optimized kernel format handling for libkrun
    • More robust VM build process with improved staging and cleanup

- Fix launcher: move variable definitions before use, use -listen-vfkit unixgram:// for gvproxy
- libkrun on Darwin uses Hypervisor.framework, which requires the ARM64 Image kernel format
- Replace removed _find_krunvm_binary with _find_libkrun_library in prerequisite check
- Add doctor checks: gvproxy (macOS), Rust musl target, hypervisor entitlement (macOS), KVM (Linux)
- Update tests to reflect new platform-dependent kernel-format behavior and FFI library check
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f19a13a-f8a9-4504-b15b-9187f0eea5eb

📥 Commits

Reviewing files that changed from the base of the PR and between 0d08bec and eb7bbf7.

📒 Files selected for processing (9)
  • src/smolvm/host/doctor.py
  • src/smolvm/images/builder.py
  • src/smolvm/images/published.py
  • src/smolvm/runtime/_libkrun_ffi.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/vm.py
  • tests/test_custom_boot_api.py
  • tests/test_vm.py
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/smolvm/images/published.py
  • tests/test_vm.py
  • tests/test_custom_boot_api.py
  • src/smolvm/runtime/_libkrun_ffi.py
  • src/smolvm/images/builder.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/host/doctor.py
  • src/smolvm/vm.py

📝 Walkthrough

Walkthrough

Replaces the krunvm CLI shell-out for the libkrun backend with a native Python ctypes FFI layer (_libkrun_ffi.py, KrunContext), a dedicated subprocess launcher (_libkrun_launcher.py) that manages gvproxy on macOS, JSON config generation in SmolVMManager, OS-specific kernel format selection, reworked readiness wait logic, updated doctor diagnostics, and image builder staging directory fixes.

Changes

libkrun Native FFI Backend Replacement

Layer / File(s) Summary
ctypes FFI layer: KrunContext and kernel-format constants
src/smolvm/runtime/_libkrun_ffi.py
New module defines kernel-format integer constants, OS-specific shared-library discovery and loading, is_available() probe, and KrunContext RAII wrapper binding all C function signatures with disk/kernel/vsock/env configuration methods, start_enter() execution, and a double-free–safe close() with context-manager support.
gvproxy discovery and _libkrun_launcher entrypoint
src/smolvm/runtime/_libkrun_launcher.py
New python -m entrypoint locates gvproxy via PATH or Homebrew Cellar candidates, starts it bound to a unixgram socket, parses a JSON config into KrunContext calls, wires krun_set_gvproxy_path on macOS, invokes ctx.start_enter(), and cleans up process and socket in a finally block.
OS-specific kernel format and libkrun networking constants
src/smolvm/images/published.py, src/smolvm/vm.py
_kernel_format_for_vmm returns "image" for libkrun on Darwin; vm.py introduces LIBKRUN_GUEST_IP/LIBKRUN_GATEWAY_IP constants used in user-mode NetworkConfig construction, plus a sys import for launcher invocation.
SmolVMManager: prerequisite check, config build/write, and launcher invocation
src/smolvm/vm.py
Removes _find_krunvm_binary; adds _find_libkrun_library (FFI probe), _build_libkrun_config (OS-aware JSON payload), and _write_libkrun_config (exclusive 0600 file in socket_dir). Both sync and async _start_libkrun variants now write the config and spawn smolvm.runtime._libkrun_launcher via sys.executable.
Libkrun readiness wait: capped settle window and fail-fast exit
src/smolvm/runtime/libkrun.py
_wait_for_runtime and _async_wait_for_runtime rewritten to cap the wait to min(0.5, boot_timeout), raise a VM-specific SmolVMError with return_code and pid on early process exit, and return successfully if the process is still alive after the window—dropping the prior timeout-kill error path.
Doctor diagnostics: libkrun library, gvproxy, Rust musl target, Hypervisor entitlement
src/smolvm/host/doctor.py
Adds three new check helpers (_check_gvproxy, Rust musl target via rustup, codesign Hypervisor entitlement). generate_doctor_report() for BACKEND_LIBKRUN switches from krunvm-command wiring to _libkrun_ffi.is_available(), always includes SSH and Rust musl checks, and branches on Darwin (gvproxy + entitlement) vs Linux (KVM).
Image builder: Docker tar mount and deterministic temp directory
src/smolvm/images/builder.py
_create_ext4_with_docker mounts tar_path.parent at /work/in instead of the tar file directly. _do_build inlines the guest-agent Dockerfile snippet earlier, replaces TemporaryDirectory with a UUID-suffixed staging dir under image_dir, and removes it via shutil.rmtree in a finally block.
Tests: OS-specific kernel format and prerequisite checks
tests/test_custom_boot_api.py, tests/test_vm.py
test_custom_boot_api.py replaces one OS-agnostic libkrun kernel test with Linux→ELF and Darwin→image variants. test_vm.py renames the prerequisite test and patches _find_libkrun_library instead of _find_krunvm_binary.

Sequence Diagram(s)

sequenceDiagram
  participant SmolVMManager as SmolVMManager (vm.py)
  participant launcher as _libkrun_launcher (subprocess)
  participant gvproxy as gvproxy (macOS only)
  participant KrunContext as KrunContext (_libkrun_ffi)
  participant libkrun as libkrun native lib

  SmolVMManager->>SmolVMManager: _find_libkrun_library() via is_available()
  SmolVMManager->>SmolVMManager: _build_libkrun_config(vm_info)
  SmolVMManager->>SmolVMManager: _write_libkrun_config → config.json (0600)
  SmolVMManager->>launcher: sys.executable -m smolvm.runtime._libkrun_launcher config.json
  launcher->>KrunContext: configure vm/kernel/disks/vsock/env
  alt macOS
    launcher->>gvproxy: _start_gvproxy(sock_path, ssh_port)
    gvproxy-->>launcher: socket file created
    launcher->>KrunContext: krun_set_gvproxy_path(ctx_id, sock_path)
  end
  launcher->>KrunContext: ctx.start_enter()
  KrunContext->>libkrun: krun_start_enter(ctx_id)
  libkrun-->>KrunContext: exit code
  KrunContext-->>launcher: rc
  launcher->>gvproxy: kill (finally)
  launcher-->>SmolVMManager: process exit code
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • CelestoAI/SmolVM#143: Both PRs modify the BACKEND_LIBKRUN diagnostics branch in doctor.py — the retrieved PR adds krunvm-based doctor checks, while this PR rewrites that same generate_doctor_report() branch to use FFI-based library probing and platform-specific checks instead.

Suggested reviewers

  • aniketmaurya

Poem

By order of the Peaky Blinders, the krunvm man is gone.
We don't use shells no more, we bind the library direct.
KrunContext opens, the gvproxy wakes at dawn,
The settle window caps at half a second — respect.
The doctor checks your entitlements, your musl, your nerve.
We built this ourselves. That's what Shelbys deserve. 🎩

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The linked issue #356 addresses browser and desktop sandbox APIs, unrelated to libkrun backend integration. The PR closes #356 but implements libkrun functionality instead of the linked issue's objectives. Link the correct issue addressing libkrun backend integration, or clarify why #356 is closed by this unrelated work.
Docstring Coverage ⚠️ Warning Docstring coverage is 34.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: integrating libkrun backend, fixing launcher bugs, and adding doctor diagnostics.
Description check ✅ Passed The PR description covers all template sections: summary of changes, related issues, testing evidence, and completion of the checklist. It provides clear context about the libkrun integration work.
Out of Scope Changes check ✅ Passed All code changes are directly scoped to libkrun backend integration: FFI bindings, launcher implementation, diagnostic checks, kernel format handling, and VM startup refactoring. No unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/smolvm/vm.py (1)

2988-2997: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Async path doesn't use libkrun-specific IP constants.

By order of the Peaky Blinders, the sync create path at lines 1613-1617 correctly uses LIBKRUN_GUEST_IP and LIBKRUN_GATEWAY_IP when backend == BACKEND_LIBKRUN, but this async path always uses QEMU_GUEST_IP and QEMU_GATEWAY_IP. That's a contract mismatch, mate. The async and sync paths need to behave the same.

🐛 Proposed fix
                 mac_seed = ((ssh_host_port or SSH_PORT_START) % 65534) + 1
                 guest_mac = self.network.generate_mac(mac_seed)
+                guest_ip = LIBKRUN_GUEST_IP if backend == BACKEND_LIBKRUN else QEMU_GUEST_IP
+                gateway_ip = LIBKRUN_GATEWAY_IP if backend == BACKEND_LIBKRUN else QEMU_GATEWAY_IP
                 network_config = NetworkConfig(
-                    guest_ip=QEMU_GUEST_IP,
-                    gateway_ip=QEMU_GATEWAY_IP,
+                    guest_ip=guest_ip,
+                    gateway_ip=gateway_ip,
                     netmask=QEMU_NETMASK,
                     tap_device="usernet",
                     guest_mac=guest_mac,
                     ssh_host_port=ssh_host_port,
                 )
🤖 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 `@src/smolvm/vm.py` around lines 2988 - 2997, The async path creating the
NetworkConfig object uses hardcoded QEMU_GUEST_IP and QEMU_GATEWAY_IP constants
for the guest_ip and gateway_ip parameters, but it should conditionally use
LIBKRUN_GUEST_IP and LIBKRUN_GATEWAY_IP when the backend is BACKEND_LIBKRUN,
matching the behavior of the sync create path at lines 1613-1617. Add a
conditional check before creating the NetworkConfig object that evaluates the
backend type and assigns the appropriate IP constants to guest_ip and gateway_ip
based on whether backend equals BACKEND_LIBKRUN.
src/smolvm/images/builder.py (1)

1825-1831: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the actual tar_path filename instead of hardcoding rootfs.tar.

With the new parent-directory mount on Line 1842, extraction now silently depends on the tar being named rootfs.tar. That undercuts the function contract that already accepts any tar_path.

🔧 Suggested fix
         rootfs_path.unlink(missing_ok=True)
+        tar_name = shlex.quote(tar_path.name)
         rootfs_name = shlex.quote(rootfs_path.name)
         shell_cmd = (
             "set -e; "
             "apk add --no-cache e2fsprogs tar >/dev/null; "
             "mkdir -p /work/rootfs-staging; "
-            "tar -xf /work/in/rootfs.tar -C /work/rootfs-staging; "
+            f"tar -xf /work/in/{tar_name} -C /work/rootfs-staging; "
             f"mke2fs -d /work/rootfs-staging -t ext4 -F /work/out/{rootfs_name} "
             f"{rootfs_size_mb}M >/dev/null"
         )

Also applies to: 1841-1842

🤖 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 `@src/smolvm/images/builder.py` around lines 1825 - 1831, The shell command in
this code section hardcodes rootfs.tar in the tar extraction step, but the
function accepts a tar_path parameter that could contain a different filename.
Replace the hardcoded rootfs.tar in the tar -xf command with the actual tar_path
variable to honor the function contract and ensure extraction works with any
provided tar file path.
🧹 Nitpick comments (6)
tests/test_custom_boot_api.py (2)

579-588: ⚡ Quick win

Make the monkeypatch path explicit, by order of the Peaky Blinders.

Right, listen here. This monkeypatch on line 583—monkeypatch.setattr("platform.system", lambda: "Linux")—it'll work, no question. But it's not how we do business in this family. Look at line 611, you'll see the proper form: "smolvm.kernels.platform.machine". Module-qualified. Explicit. No ambiguity.

Your code under test sits in smolvm.images.published, yeah? So patch it there: "smolvm.images.published.platform.system". Keeps it consistent with the rest of the tests, and any man reading this code knows exactly what's being mocked. No surprises.

Proper monkeypatch, Shelby style
-    monkeypatch.setattr("platform.system", lambda: "Linux")
+    monkeypatch.setattr("smolvm.images.published.platform.system", lambda: "Linux")
🤖 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 `@tests/test_custom_boot_api.py` around lines 579 - 588, In the
test_libkrun_selects_elf_kernel_on_linux method, change the monkeypatch call
from using the unqualified "platform.system" to the fully qualified module path
"smolvm.images.published.platform.system". This makes it explicit about where
the platform.system function is being patched and keeps it consistent with other
monkeypatch calls in the test suite like the one at line 611 that uses
"smolvm.kernels.platform.machine". The change ensures clarity and prevents
ambiguity about which module's platform.system is being mocked.

590-601: ⚡ Quick win

Make the monkeypatch path explicit here as well.

Same matter as the Linux test above, line 596. You're patching "platform.system" when you should be qualifying it: "smolvm.images.published.platform.system". Keeps the pattern consistent, keeps the tests clean. That's how we maintain order.

Proper monkeypatch, consistent with line 611
-    monkeypatch.setattr("platform.system", lambda: "Darwin")
+    monkeypatch.setattr("smolvm.images.published.platform.system", lambda: "Darwin")
🤖 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 `@tests/test_custom_boot_api.py` around lines 590 - 601, In the
test_libkrun_selects_image_kernel_on_darwin method, the monkeypatch.setattr call
uses an unqualified path for the platform.system attribute. Change the
monkeypatch path from "platform.system" to
"smolvm.images.published.platform.system" to explicitly qualify where
platform.system is being imported and used, maintaining consistency with the
pattern used elsewhere in the test file (as referenced in line 611).
src/smolvm/runtime/_libkrun_launcher.py (1)

42-42: ⚡ Quick win

Missing space after comma.

Small thing, but we keep our house in order.

-def _start_gvproxy(sock_path: str,ssh_host_port: int) -> subprocess.Popen:
+def _start_gvproxy(sock_path: str, ssh_host_port: int) -> subprocess.Popen:
🤖 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 `@src/smolvm/runtime/_libkrun_launcher.py` at line 42, In the function
definition of _start_gvproxy, there is a missing space after the comma between
the sock_path and ssh_host_port parameters. Add a space after the comma
following the str type annotation to comply with Python style conventions and
maintain consistent code formatting throughout the codebase.
src/smolvm/vm.py (1)

1529-1566: ⚡ Quick win

Commented-out code and a typo in the comment.

Right. Line 2562 has commented-out code with _socket_dir which doesn't exist — it's self.socket_dir. Either way, dead code tells no tales. Remove it.

♻️ Proposed fix
     def _write_libkrun_config(self, vm_info: VMInfo, payload: dict[str, Any]) -> Path:
-        # config_path = self._socket_dir / f"libkrun-{vm_info.vm_id}.json"
         config_path = self.socket_dir / f"libkrun-{vm_info.vm_id}.json"
🤖 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 `@src/smolvm/vm.py` around lines 1529 - 1566, Remove the commented-out code
that contains the `_socket_dir` reference, which appears as dead code and is
incorrect anyway (should be `self.socket_dir` if it were active). This dead code
provides no value and should be cleaned up entirely rather than left as a
commented remnant.
src/smolvm/runtime/_libkrun_ffi.py (1)

62-95: ⚡ Quick win

krun_set_gvproxy_path FFI wiring is duplicated in the launcher instead of centralized in the FFI module. The root cause is that _libkrun() doesn't set up argtypes/restype for krun_set_gvproxy_path, forcing the launcher to do it inline before each call. This family keeps its contracts in one place.

  • src/smolvm/runtime/_libkrun_ffi.py#L62-L95: Add lib.krun_set_gvproxy_path.argtypes = [ctypes.c_uint32, ctypes.c_char_p] and lib.krun_set_gvproxy_path.restype = ctypes.c_int32 alongside the other function declarations.
  • src/smolvm/runtime/_libkrun_launcher.py#L112-L114: Remove the inline argtypes/restype assignment since it will be handled by _libkrun().
🤖 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 `@src/smolvm/runtime/_libkrun_ffi.py` around lines 62 - 95, The
`krun_set_gvproxy_path` function's FFI type declarations are duplicated inline
in the launcher instead of being centralized in the FFI module like all other
function declarations. In `src/smolvm/runtime/_libkrun_ffi.py` at lines 62-95,
add the argtypes and restype declarations for `lib.krun_set_gvproxy_path` (with
argtypes set to [ctypes.c_uint32, ctypes.c_char_p] and restype set to
ctypes.c_int32) in the _libkrun() function alongside the other function
declarations. Then in `src/smolvm/runtime/_libkrun_launcher.py` at lines
112-114, remove the inline argtypes and restype assignments for
`krun_set_gvproxy_path` since they will now be centrally managed in the FFI
module.
src/smolvm/runtime/libkrun.py (1)

99-114: ⚡ Quick win

Strip the commented-out wait paths and keep one source of truth.

Right now, those dead blocks muddy a critical startup path for no gain. Git keeps the history — best to cut them.

Also applies to: 151-164

🤖 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 `@src/smolvm/runtime/libkrun.py` around lines 99 - 114, Remove the
commented-out code blocks that represent dead code and clutter the repository.
Delete the entire commented-out _wait_for_runtime method block (lines 99-114 in
src/smolvm/runtime/libkrun.py) and also delete the similar commented-out block
at lines 151-164 in the same file. Since version control history is preserved by
git, these dead blocks serve no purpose and should be removed to keep the
codebase clean and maintainable.
🤖 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 `@src/smolvm/host/doctor.py`:
- Around line 185-194: The check at line 185 is insufficient because it allows
the code to proceed when cargo exists on PATH but rustup_bin is None. When the
code then attempts to execute the rustup command at line 194 using the fallback
"rustup" string, it will fail since rustup_bin being None means rustup was not
found on PATH. Fix the condition to require that rustup_bin is not None instead
of checking only for the presence of cargo, ensuring that rustup is actually
available before attempting to run rustup target list commands.

In `@src/smolvm/runtime/_libkrun_launcher.py`:
- Around line 115-118: The error message in the krun_set_gvproxy_path failure
block only reports that the call failed with return code rc but does not provide
any guidance to the user on how to recover from this error. Update the error
message printed to sys.stderr to include actionable recovery suggestions, such
as checking whether gvproxy is running, verifying the libkrun installation, or
other relevant troubleshooting steps that would help the user resolve the
underlying issue.
- Around line 23-28: Remove the hardcoded version-pinned path
`/opt/homebrew/Cellar/podman/5.8.3/libexec/podman/gvproxy` from the
_GVPROXY_CANDIDATES list, as it will break when podman is upgraded. The
version-agnostic symlinked path `/opt/homebrew/libexec/podman/gvproxy`
(currently the second entry) should be prioritized as the first candidate
instead, allowing gvproxy discovery to work across podman version upgrades.

In `@src/smolvm/runtime/libkrun.py`:
- Around line 127-129: The error messages in the wait helper functions are not
actionable for operators. Modify the wait helper functions to accept a vm_id
parameter, then update all error messages (including those for "libkrun process
exited before VM became ready" and similar launch-failure scenarios) to be
single-sentence messages that include a specific, actionable recovery command
with the actual sandbox name interpolated from the vm_id. Each error message
should name the recovery action and include the exact command the operator
should run, rather than just describing what went wrong.
- Around line 139-144: The async path in the while loop (around lines 139-144)
checks process.returncode directly without calling poll() first, which means the
returncode won't be updated if the process has exited. In the sync version at
lines 124 and 132, poll() is correctly called before checking returncode. Apply
the same pattern to the async path: call process.poll() before checking
process.returncode at line 140 (the initial check in the while loop
condition/body) and at line 147 (the second check mentioned in the comment).
This ensures the Popen object's returncode state is refreshed and dead processes
are properly detected.

In `@src/smolvm/vm.py`:
- Around line 24-38: The imports `from concurrent.futures import process` and
`from pyexpat import errors` are not being used anywhere in the file and should
be removed to clean up the import section. Delete both of these unused import
statements from the import block at the top of the file.
- Around line 3295-3296: The asyncio.create_task() call for
_close_log_when_done() is not storing the returned task object, which causes
exceptions to be silently swallowed. Capture the task returned by
asyncio.create_task(_close_log_when_done()) into a variable and add it to the
_background_tasks collection so exceptions can be properly tracked and the task
can be cancelled during cleanup.

---

Outside diff comments:
In `@src/smolvm/images/builder.py`:
- Around line 1825-1831: The shell command in this code section hardcodes
rootfs.tar in the tar extraction step, but the function accepts a tar_path
parameter that could contain a different filename. Replace the hardcoded
rootfs.tar in the tar -xf command with the actual tar_path variable to honor the
function contract and ensure extraction works with any provided tar file path.

In `@src/smolvm/vm.py`:
- Around line 2988-2997: The async path creating the NetworkConfig object uses
hardcoded QEMU_GUEST_IP and QEMU_GATEWAY_IP constants for the guest_ip and
gateway_ip parameters, but it should conditionally use LIBKRUN_GUEST_IP and
LIBKRUN_GATEWAY_IP when the backend is BACKEND_LIBKRUN, matching the behavior of
the sync create path at lines 1613-1617. Add a conditional check before creating
the NetworkConfig object that evaluates the backend type and assigns the
appropriate IP constants to guest_ip and gateway_ip based on whether backend
equals BACKEND_LIBKRUN.

---

Nitpick comments:
In `@src/smolvm/runtime/_libkrun_ffi.py`:
- Around line 62-95: The `krun_set_gvproxy_path` function's FFI type
declarations are duplicated inline in the launcher instead of being centralized
in the FFI module like all other function declarations. In
`src/smolvm/runtime/_libkrun_ffi.py` at lines 62-95, add the argtypes and
restype declarations for `lib.krun_set_gvproxy_path` (with argtypes set to
[ctypes.c_uint32, ctypes.c_char_p] and restype set to ctypes.c_int32) in the
_libkrun() function alongside the other function declarations. Then in
`src/smolvm/runtime/_libkrun_launcher.py` at lines 112-114, remove the inline
argtypes and restype assignments for `krun_set_gvproxy_path` since they will now
be centrally managed in the FFI module.

In `@src/smolvm/runtime/_libkrun_launcher.py`:
- Line 42: In the function definition of _start_gvproxy, there is a missing
space after the comma between the sock_path and ssh_host_port parameters. Add a
space after the comma following the str type annotation to comply with Python
style conventions and maintain consistent code formatting throughout the
codebase.

In `@src/smolvm/runtime/libkrun.py`:
- Around line 99-114: Remove the commented-out code blocks that represent dead
code and clutter the repository. Delete the entire commented-out
_wait_for_runtime method block (lines 99-114 in src/smolvm/runtime/libkrun.py)
and also delete the similar commented-out block at lines 151-164 in the same
file. Since version control history is preserved by git, these dead blocks serve
no purpose and should be removed to keep the codebase clean and maintainable.

In `@src/smolvm/vm.py`:
- Around line 1529-1566: Remove the commented-out code that contains the
`_socket_dir` reference, which appears as dead code and is incorrect anyway
(should be `self.socket_dir` if it were active). This dead code provides no
value and should be cleaned up entirely rather than left as a commented remnant.

In `@tests/test_custom_boot_api.py`:
- Around line 579-588: In the test_libkrun_selects_elf_kernel_on_linux method,
change the monkeypatch call from using the unqualified "platform.system" to the
fully qualified module path "smolvm.images.published.platform.system". This
makes it explicit about where the platform.system function is being patched and
keeps it consistent with other monkeypatch calls in the test suite like the one
at line 611 that uses "smolvm.kernels.platform.machine". The change ensures
clarity and prevents ambiguity about which module's platform.system is being
mocked.
- Around line 590-601: In the test_libkrun_selects_image_kernel_on_darwin
method, the monkeypatch.setattr call uses an unqualified path for the
platform.system attribute. Change the monkeypatch path from "platform.system" to
"smolvm.images.published.platform.system" to explicitly qualify where
platform.system is being imported and used, maintaining consistency with the
pattern used elsewhere in the test file (as referenced in line 611).
🪄 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: 31ed06b2-ee19-49ee-b442-0e9ba8063916

📥 Commits

Reviewing files that changed from the base of the PR and between 85a7e0a and c6db49b.

📒 Files selected for processing (9)
  • src/smolvm/host/doctor.py
  • src/smolvm/images/builder.py
  • src/smolvm/images/published.py
  • src/smolvm/runtime/_libkrun_ffi.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/vm.py
  • tests/test_custom_boot_api.py
  • tests/test_vm.py

Comment thread src/smolvm/host/doctor.py Outdated
Comment thread src/smolvm/runtime/_libkrun_launcher.py Outdated
Comment thread src/smolvm/runtime/_libkrun_launcher.py
Comment thread src/smolvm/runtime/libkrun.py Outdated
Comment thread src/smolvm/runtime/libkrun.py
Comment thread src/smolvm/vm.py Outdated
Comment thread src/smolvm/vm.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/smolvm/vm.py (2)

2572-2575: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make the libkrun-missing start error actionable.

These two start-path errors still stop at “installed and loadable”, so a failed launch tells the operator what’s wrong but not the exact command to fix it. Use one sentence with the host-specific install command here so the recovery is immediate. As per coding guidelines, "User-facing error and warning messages must name the recovery action and include the exact recovery command with the actual sandbox name interpolated" and "User-facing error and warning messages should be one sentence maximum; if reaching for a third sentence, the message is probably explaining non-actionable consequences."

Also applies to: 3256-3259

🤖 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 `@src/smolvm/vm.py` around lines 2572 - 2575, The error message for missing
libkrun library in the SmolVMError at lines 2572-2575 does not provide an
actionable recovery command, making it unclear how to fix the issue. Update this
error message to be one sentence that includes the exact host-specific install
command needed to resolve the problem, with the actual sandbox name interpolated
into the message. Apply the same fix to the similar libkrun-missing error at
lines 3256-3259, ensuring both messages follow the same actionable format with
the specific recovery command included.

Source: Coding guidelines


2976-2995: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The async libkrun path is wearing QEMU’s network suit.

This branch still hardcodes QEMU_GUEST_IP and QEMU_GATEWAY_IP, so async-created libkrun sandboxes persist the wrong guest/gateway pair and _resolve_boot_args() injects an ip= value that does not match libkrun’s 192.168.127.x network. Mirror the sync branch’s backend-aware selection here.

🐛 Proposed fix
                 mac_seed = ((ssh_host_port or SSH_PORT_START) % 65534) + 1
                 guest_mac = self.network.generate_mac(mac_seed)
+                guest_ip = LIBKRUN_GUEST_IP if backend == BACKEND_LIBKRUN else QEMU_GUEST_IP
+                gateway_ip = (
+                    LIBKRUN_GATEWAY_IP if backend == BACKEND_LIBKRUN else QEMU_GATEWAY_IP
+                )
                 network_config = NetworkConfig(
-                    guest_ip=QEMU_GUEST_IP,
-                    gateway_ip=QEMU_GATEWAY_IP,
+                    guest_ip=guest_ip,
+                    gateway_ip=gateway_ip,
                     netmask=QEMU_NETMASK,
                     tap_device="usernet",
                     guest_mac=guest_mac,
🤖 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 `@src/smolvm/vm.py` around lines 2976 - 2995, The network configuration
creation within the `if not self._uses_host_tap_networking(effective_config,
backend):` block is hardcoding QEMU_GUEST_IP and QEMU_GATEWAY_IP for all
backends, but libkrun requires different network values (192.168.127.x range).
Replace the hardcoded QEMU IP constants with backend-aware selection that
chooses the appropriate guest_ip and gateway_ip values based on the backend
parameter, mirroring the approach used in the sync branch. This will ensure the
correct network configuration is applied to the NetworkConfig object regardless
of whether the backend is QEMU or libkrun.
♻️ Duplicate comments (1)
src/smolvm/runtime/libkrun.py (1)

105-117: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fold the recovery command into one sentence, by order.

These wait-helper failures are still emitted as two sentences, so they still miss the repo’s user-facing message contract. Make the recovery command part of the same sentence in both sync and async paths. As per coding guidelines, "User-facing error and warning messages should be one sentence maximum; if reaching for a third sentence, the message is probably explaining non-actionable consequences."

🐛 Proposed fix
-                    f"VM '{vm_id}' failed to start. Run 'smolvm delete {vm_id}' to remove it.",
+                    f"VM '{vm_id}' failed to start; run 'smolvm delete {vm_id}' to remove it.",

Also applies to: 123-137

🤖 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 `@src/smolvm/runtime/libkrun.py` around lines 105 - 117, The error messages in
SmolVMError exceptions are split across two sentences, violating the coding
guideline that user-facing messages should be one sentence maximum. Combine the
VM failure message and the recovery command into a single sentence in both the
while-loop timeout case and the final poll check case. Reword the messages so
that the recovery instruction ("Run 'smolvm delete {vm_id}' to remove it")
becomes part of the main sentence rather than a separate actionable statement,
ensuring consistency across all similar error messages in the module.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@src/smolvm/vm.py`:
- Around line 2572-2575: The error message for missing libkrun library in the
SmolVMError at lines 2572-2575 does not provide an actionable recovery command,
making it unclear how to fix the issue. Update this error message to be one
sentence that includes the exact host-specific install command needed to resolve
the problem, with the actual sandbox name interpolated into the message. Apply
the same fix to the similar libkrun-missing error at lines 3256-3259, ensuring
both messages follow the same actionable format with the specific recovery
command included.
- Around line 2976-2995: The network configuration creation within the `if not
self._uses_host_tap_networking(effective_config, backend):` block is hardcoding
QEMU_GUEST_IP and QEMU_GATEWAY_IP for all backends, but libkrun requires
different network values (192.168.127.x range). Replace the hardcoded QEMU IP
constants with backend-aware selection that chooses the appropriate guest_ip and
gateway_ip values based on the backend parameter, mirroring the approach used in
the sync branch. This will ensure the correct network configuration is applied
to the NetworkConfig object regardless of whether the backend is QEMU or
libkrun.

---

Duplicate comments:
In `@src/smolvm/runtime/libkrun.py`:
- Around line 105-117: The error messages in SmolVMError exceptions are split
across two sentences, violating the coding guideline that user-facing messages
should be one sentence maximum. Combine the VM failure message and the recovery
command into a single sentence in both the while-loop timeout case and the final
poll check case. Reword the messages so that the recovery instruction ("Run
'smolvm delete {vm_id}' to remove it") becomes part of the main sentence rather
than a separate actionable statement, ensuring consistency across all similar
error messages in the module.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 65e5481d-a7aa-43db-9ee9-6ab2a5fae4f0

📥 Commits

Reviewing files that changed from the base of the PR and between c6db49b and b4c82b5.

📒 Files selected for processing (4)
  • src/smolvm/host/doctor.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/vm.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/smolvm/host/doctor.py
  • src/smolvm/runtime/_libkrun_launcher.py

@SauravMishraaa

Copy link
Copy Markdown
Contributor Author

@aniketmaurya Could you review this?

@coderabbitai
coderabbitai Bot requested a review from aniketmaurya June 15, 2026 13:14

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (3)
src/smolvm/vm.py (3)

3142-3144: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the libkrun network constants in async create as well, mate.

The sync path switches libkrun to 192.168.127.2/192.168.127.1, but async_create() still persists the QEMU guest/gateway IPs; async-created libkrun VMs will boot with the wrong ip= kernel argument and SSH forwarding won’t line up.

🐛 Keep async networking in step with sync networking
                 mac_seed = ((ssh_host_port or SSH_PORT_START) % 65534) + 1
                 guest_mac = self.network.generate_mac(mac_seed)
+                guest_ip = LIBKRUN_GUEST_IP if backend == BACKEND_LIBKRUN else QEMU_GUEST_IP
+                gateway_ip = LIBKRUN_GATEWAY_IP if backend == BACKEND_LIBKRUN else QEMU_GATEWAY_IP
                 network_config = NetworkConfig(
-                    guest_ip=QEMU_GUEST_IP,
-                    gateway_ip=QEMU_GATEWAY_IP,
+                    guest_ip=guest_ip,
+                    gateway_ip=gateway_ip,
                     netmask=QEMU_NETMASK,
                     tap_device="usernet",
                     guest_mac=guest_mac,
🤖 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 `@src/smolvm/vm.py` around lines 3142 - 3144, The async_create() method is
initializing NetworkConfig with QEMU_GUEST_IP and QEMU_GATEWAY_IP, but the sync
path uses libkrun network constants instead. Update the NetworkConfig
initialization in async_create() to use the same libkrun network constants
(192.168.127.2 for guest IP and 192.168.127.1 for gateway IP) that the sync path
uses, ensuring async-created libkrun VMs boot with the correct kernel arguments
and SSH forwarding alignment.

2713-2718: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t write launcher config to a predictable /tmp path.

By default socket_dir is /tmp, so libkrun-{vm_id}.json can be pre-created as a symlink and write_text() will follow it; make the file name unguessable and create it with exclusive 0600 permissions.

🛡️ Safer config-file creation
     def _write_libkrun_config(self, vm_info: VMInfo, payload: dict[str, Any]) -> Path:
-        # config_path = self._socket_dir / f"libkrun-{vm_info.vm_id}.json"
-        config_path = self.socket_dir / f"libkrun-{vm_info.vm_id}.json"
+        config_path = self.socket_dir / f"libkrun-{vm_info.vm_id}-{uuid4().hex}.json"
         config_path.parent.mkdir(parents=True, exist_ok=True)
-        config_path.write_text(json.dumps(payload))
+        fd = os.open(config_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
+        with os.fdopen(fd, "w", encoding="utf-8") as config_file:
+            json.dump(payload, config_file)
         return config_path
🤖 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 `@src/smolvm/vm.py` around lines 2713 - 2718, The _write_libkrun_config method
writes the launcher configuration to a predictable path in /tmp based only on
the vm_id, which is vulnerable to symlink attacks. Replace the predictable
filename generation and write_text() call with a method that creates the file
with exclusive 0600 permissions, making the filename unguessable (for example,
using tempfile.NamedTemporaryFile or a UUID-based name) to prevent an attacker
from pre-creating a symlink that would be followed by write_text().

2726-2729: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Give the libkrun failure a recovery command, not just a verdict.

Both start paths know vm_info.vm_id, so the error can tell the user exactly what to run instead of leaving them with “installed and loadable.”

🗣️ Make the sync and async messages actionable
         if not self._find_libkrun_library():
             raise SmolVMError(
-                "libkrun backend requires libkrun (>=1.9) to be installed and loadable"
+                f"Cannot start sandbox '{vm_info.vm_id}' because libkrun is not ready; "
+                f"run 'smolvm doctor --backend libkrun', then run "
+                f"'smolvm start {vm_info.vm_id}'."
             )

Apply the same message in _async_start_libkrun().

As per coding guidelines, “User-facing error and warning messages must name the recovery action and include the exact recovery command with the actual sandbox name interpolated.”

Also applies to: 3420-3423

🤖 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 `@src/smolvm/vm.py` around lines 2726 - 2729, The libkrun error messages lack
actionable recovery commands for users. In the SmolVMError raised when
_find_libkrun_library() fails (around lines 2726-2729), replace the generic
"installed and loadable" message with a concrete recovery command that includes
the actual sandbox name using vm_info.vm_id that is available in that context.
Apply the same actionable error message to the corresponding libkrun failure
check in the _async_start_libkrun() method (around lines 3420-3423). Both
messages should follow the coding guideline requiring user-facing errors to name
the recovery action and include the exact recovery command with the actual
sandbox name interpolated.

Source: Coding guidelines

🤖 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 `@src/smolvm/vm.py`:
- Around line 725-726: The subprocess.run() call with bare string "cp" triggers
a Ruff S603 violation. Replace the bare string "cp" with which("cp") to resolve
the executable path. Then add a targeted noqa comment (# noqa: S603) on the
subprocess.run() line to suppress the warning. This follows the same pattern
already established elsewhere in the codebase for similar executable resolution
with other commands like e2fsck and resize2fs.

---

Outside diff comments:
In `@src/smolvm/vm.py`:
- Around line 3142-3144: The async_create() method is initializing NetworkConfig
with QEMU_GUEST_IP and QEMU_GATEWAY_IP, but the sync path uses libkrun network
constants instead. Update the NetworkConfig initialization in async_create() to
use the same libkrun network constants (192.168.127.2 for guest IP and
192.168.127.1 for gateway IP) that the sync path uses, ensuring async-created
libkrun VMs boot with the correct kernel arguments and SSH forwarding alignment.
- Around line 2713-2718: The _write_libkrun_config method writes the launcher
configuration to a predictable path in /tmp based only on the vm_id, which is
vulnerable to symlink attacks. Replace the predictable filename generation and
write_text() call with a method that creates the file with exclusive 0600
permissions, making the filename unguessable (for example, using
tempfile.NamedTemporaryFile or a UUID-based name) to prevent an attacker from
pre-creating a symlink that would be followed by write_text().
- Around line 2726-2729: The libkrun error messages lack actionable recovery
commands for users. In the SmolVMError raised when _find_libkrun_library() fails
(around lines 2726-2729), replace the generic "installed and loadable" message
with a concrete recovery command that includes the actual sandbox name using
vm_info.vm_id that is available in that context. Apply the same actionable error
message to the corresponding libkrun failure check in the _async_start_libkrun()
method (around lines 3420-3423). Both messages should follow the coding
guideline requiring user-facing errors to name the recovery action and include
the exact recovery command with the actual sandbox name interpolated.
🪄 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: 1be1115e-69ff-4bfb-8249-f4ddb8a18020

📥 Commits

Reviewing files that changed from the base of the PR and between b4c82b5 and a9c458d.

📒 Files selected for processing (4)
  • src/smolvm/images/builder.py
  • src/smolvm/images/published.py
  • src/smolvm/vm.py
  • tests/test_vm.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/smolvm/images/published.py
  • tests/test_vm.py
  • src/smolvm/images/builder.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

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 (3)
src/smolvm/vm.py (3)

3142-3144: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the libkrun network constants in async create as well, mate.

The sync path switches libkrun to 192.168.127.2/192.168.127.1, but async_create() still persists the QEMU guest/gateway IPs; async-created libkrun VMs will boot with the wrong ip= kernel argument and SSH forwarding won’t line up.

🐛 Keep async networking in step with sync networking
                 mac_seed = ((ssh_host_port or SSH_PORT_START) % 65534) + 1
                 guest_mac = self.network.generate_mac(mac_seed)
+                guest_ip = LIBKRUN_GUEST_IP if backend == BACKEND_LIBKRUN else QEMU_GUEST_IP
+                gateway_ip = LIBKRUN_GATEWAY_IP if backend == BACKEND_LIBKRUN else QEMU_GATEWAY_IP
                 network_config = NetworkConfig(
-                    guest_ip=QEMU_GUEST_IP,
-                    gateway_ip=QEMU_GATEWAY_IP,
+                    guest_ip=guest_ip,
+                    gateway_ip=gateway_ip,
                     netmask=QEMU_NETMASK,
                     tap_device="usernet",
                     guest_mac=guest_mac,
🤖 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 `@src/smolvm/vm.py` around lines 3142 - 3144, The async_create() method is
initializing NetworkConfig with QEMU_GUEST_IP and QEMU_GATEWAY_IP, but the sync
path uses libkrun network constants instead. Update the NetworkConfig
initialization in async_create() to use the same libkrun network constants
(192.168.127.2 for guest IP and 192.168.127.1 for gateway IP) that the sync path
uses, ensuring async-created libkrun VMs boot with the correct kernel arguments
and SSH forwarding alignment.

2713-2718: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t write launcher config to a predictable /tmp path.

By default socket_dir is /tmp, so libkrun-{vm_id}.json can be pre-created as a symlink and write_text() will follow it; make the file name unguessable and create it with exclusive 0600 permissions.

🛡️ Safer config-file creation
     def _write_libkrun_config(self, vm_info: VMInfo, payload: dict[str, Any]) -> Path:
-        # config_path = self._socket_dir / f"libkrun-{vm_info.vm_id}.json"
-        config_path = self.socket_dir / f"libkrun-{vm_info.vm_id}.json"
+        config_path = self.socket_dir / f"libkrun-{vm_info.vm_id}-{uuid4().hex}.json"
         config_path.parent.mkdir(parents=True, exist_ok=True)
-        config_path.write_text(json.dumps(payload))
+        fd = os.open(config_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
+        with os.fdopen(fd, "w", encoding="utf-8") as config_file:
+            json.dump(payload, config_file)
         return config_path
🤖 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 `@src/smolvm/vm.py` around lines 2713 - 2718, The _write_libkrun_config method
writes the launcher configuration to a predictable path in /tmp based only on
the vm_id, which is vulnerable to symlink attacks. Replace the predictable
filename generation and write_text() call with a method that creates the file
with exclusive 0600 permissions, making the filename unguessable (for example,
using tempfile.NamedTemporaryFile or a UUID-based name) to prevent an attacker
from pre-creating a symlink that would be followed by write_text().

2726-2729: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Give the libkrun failure a recovery command, not just a verdict.

Both start paths know vm_info.vm_id, so the error can tell the user exactly what to run instead of leaving them with “installed and loadable.”

🗣️ Make the sync and async messages actionable
         if not self._find_libkrun_library():
             raise SmolVMError(
-                "libkrun backend requires libkrun (>=1.9) to be installed and loadable"
+                f"Cannot start sandbox '{vm_info.vm_id}' because libkrun is not ready; "
+                f"run 'smolvm doctor --backend libkrun', then run "
+                f"'smolvm start {vm_info.vm_id}'."
             )

Apply the same message in _async_start_libkrun().

As per coding guidelines, “User-facing error and warning messages must name the recovery action and include the exact recovery command with the actual sandbox name interpolated.”

Also applies to: 3420-3423

🤖 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 `@src/smolvm/vm.py` around lines 2726 - 2729, The libkrun error messages lack
actionable recovery commands for users. In the SmolVMError raised when
_find_libkrun_library() fails (around lines 2726-2729), replace the generic
"installed and loadable" message with a concrete recovery command that includes
the actual sandbox name using vm_info.vm_id that is available in that context.
Apply the same actionable error message to the corresponding libkrun failure
check in the _async_start_libkrun() method (around lines 3420-3423). Both
messages should follow the coding guideline requiring user-facing errors to name
the recovery action and include the exact recovery command with the actual
sandbox name interpolated.

Source: Coding guidelines

🤖 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 `@src/smolvm/vm.py`:
- Around line 725-726: The subprocess.run() call with bare string "cp" triggers
a Ruff S603 violation. Replace the bare string "cp" with which("cp") to resolve
the executable path. Then add a targeted noqa comment (# noqa: S603) on the
subprocess.run() line to suppress the warning. This follows the same pattern
already established elsewhere in the codebase for similar executable resolution
with other commands like e2fsck and resize2fs.

---

Outside diff comments:
In `@src/smolvm/vm.py`:
- Around line 3142-3144: The async_create() method is initializing NetworkConfig
with QEMU_GUEST_IP and QEMU_GATEWAY_IP, but the sync path uses libkrun network
constants instead. Update the NetworkConfig initialization in async_create() to
use the same libkrun network constants (192.168.127.2 for guest IP and
192.168.127.1 for gateway IP) that the sync path uses, ensuring async-created
libkrun VMs boot with the correct kernel arguments and SSH forwarding alignment.
- Around line 2713-2718: The _write_libkrun_config method writes the launcher
configuration to a predictable path in /tmp based only on the vm_id, which is
vulnerable to symlink attacks. Replace the predictable filename generation and
write_text() call with a method that creates the file with exclusive 0600
permissions, making the filename unguessable (for example, using
tempfile.NamedTemporaryFile or a UUID-based name) to prevent an attacker from
pre-creating a symlink that would be followed by write_text().
- Around line 2726-2729: The libkrun error messages lack actionable recovery
commands for users. In the SmolVMError raised when _find_libkrun_library() fails
(around lines 2726-2729), replace the generic "installed and loadable" message
with a concrete recovery command that includes the actual sandbox name using
vm_info.vm_id that is available in that context. Apply the same actionable error
message to the corresponding libkrun failure check in the _async_start_libkrun()
method (around lines 3420-3423). Both messages should follow the coding
guideline requiring user-facing errors to name the recovery action and include
the exact recovery command with the actual sandbox name interpolated.
🪄 Autofix (Beta)

❌ Autofix failed (check again to retry)

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: 1be1115e-69ff-4bfb-8249-f4ddb8a18020

📥 Commits

Reviewing files that changed from the base of the PR and between b4c82b5 and a9c458d.

📒 Files selected for processing (4)
  • src/smolvm/images/builder.py
  • src/smolvm/images/published.py
  • src/smolvm/vm.py
  • tests/test_vm.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/smolvm/images/published.py
  • tests/test_vm.py
  • src/smolvm/images/builder.py
🛑 Comments failed to post (1)
src/smolvm/vm.py (1)

725-726: ⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/smolvm/vm.py | sed -n '720,735p'

Repository: CelestoAI/SmolVM

Length of output: 818


🏁 Script executed:

# Check for which() import and related utilities
grep -n "from shutil import" src/smolvm/vm.py | head -5

Repository: CelestoAI/SmolVM

Length of output: 42


🏁 Script executed:

# Check if there's already a _copy_sparse_preserving method
grep -n "_copy_sparse_preserving\|def.*copy" src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 533


🏁 Script executed:

# Run ruff check to see actual violations
cd . && python3 -m pip install -q ruff 2>/dev/null && python3 -m ruff check src/smolvm/vm.py --select S603,S607 2>&1 | head -30

Repository: CelestoAI/SmolVM

Length of output: 1536


🏁 Script executed:

# Check imports at the top of the file
head -50 src/smolvm/vm.py | grep -E "^import|^from"

Repository: CelestoAI/SmolVM

Length of output: 646


🏁 Script executed:

# Search for all subprocess.run calls to see if this is a pattern issue
grep -n "subprocess\.run" src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 554


🏁 Script executed:

# Check if shutil.which is already used in the codebase
grep -n "which\|from shutil" src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 503


🏁 Script executed:

# Verify the actual Ruff configuration and S603/S607 rules
if [ -f pyproject.toml ]; then grep -A 10 "tool.ruff" pyproject.toml | head -20; fi
if [ -f ruff.toml ]; then cat ruff.toml | head -20; fi

Repository: CelestoAI/SmolVM

Length of output: 905


🏁 Script executed:

# Look at how which() is actually used in the codebase to understand the pattern
sed -n '628,635p' src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 251


🏁 Script executed:

# Check the which function definition
grep -n "def which" src/smolvm/utils.py

Repository: CelestoAI/SmolVM

Length of output: 103


🏁 Script executed:

# Verify the full context of the cp subprocess call to understand the flow better
sed -n '715,750p' src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 1468


🏁 Script executed:

# Check how which() is used with other subprocess.run calls
sed -n '833,845p' src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 574


🏁 Script executed:

# Check a subprocess.run call that uses which() to see the pattern
sed -n '1243,1260p' src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 659


🏁 Script executed:

# Look at line 857 subprocess.run without which to see contrast
sed -n '855,865p' src/smolvm/vm.py

Repository: CelestoAI/SmolVM

Length of output: 665


Sort the Ruff violation before it breaks the build.

The subprocess.run() at line 725 triggers Ruff S603 because "cp" is a bare string in the command list. Use which() to resolve the executable—it's already in the codebase—then add a targeted noqa comment. This follows the pattern you've already established elsewhere (lines 833–839 show the same approach with e2fsck and resize2fs).

+        cp = which("cp")
+        if cp is None:
+            SmolVMManager._copy_sparse_preserving(source_path, target_path)
+            return
+
         result = subprocess.run(
-            ["cp", "--reflink=auto", "--sparse=always", str(source_path), str(target_path)],
+            [str(cp), "--reflink=auto", "--sparse=always", str(source_path), str(target_path)],  # noqa: S603
             capture_output=True,
             text=True,
             check=False,
         )
🧰 Tools
🪛 Ruff (0.15.15)

[error] 725-725: subprocess call: check for execution of untrusted input

(S603)


[error] 726-726: Starting a process with a partial executable path

(S607)

🤖 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 `@src/smolvm/vm.py` around lines 725 - 726, The subprocess.run() call with bare
string "cp" triggers a Ruff S603 violation. Replace the bare string "cp" with
which("cp") to resolve the executable path. Then add a targeted noqa comment (#
noqa: S603) on the subprocess.run() line to suppress the warning. This follows
the same pattern already established elsewhere in the codebase for similar
executable resolution with other commands like e2fsck and resize2fs.

Sources: Coding guidelines, Linters/SAST tools

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found.

@yanurag-dev

yanurag-dev commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@SauravMishraaa Thanks for the PR !!

I have given some comments please verify it once and did you test these changes on mac-os device ?

@yanurag-dev

Copy link
Copy Markdown
Contributor

@CodeRabbit perform full review and verify my findings too.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details and all review comments
gh pr view 370 --repo CelestoAI/SmolVM --json title,body,state,author,additions,deletions,changedFiles

echo "=== PR REVIEW COMMENTS ==="
gh api repos/CelestoAI/SmolVM/pulls/370/comments --jq '.[] | {user: .user.login, body: .body, path: .path, line: .line, diff_hunk: .diff_hunk}'

echo "=== PR ISSUE COMMENTS ==="
gh api repos/CelestoAI/SmolVM/issues/370/comments --jq '.[] | {user: .user.login, body: .body}'

Length of output: 69564


🏁 Script executed:

#!/bin/bash
# Read the key modified files
echo "=== _libkrun_ffi.py ==="
cat src/smolvm/runtime/_libkrun_ffi.py

echo "=== _libkrun_launcher.py ==="
cat src/smolvm/runtime/_libkrun_launcher.py

echo "=== libkrun.py ==="
cat src/smolvm/runtime/libkrun.py

echo "=== vm.py (libkrun sections) ==="
rg -n "libkrun|krunvm|LIBKRUN" src/smolvm/vm.py -C3

echo "=== doctor.py ==="
cat src/smolvm/host/doctor.py

echo "=== published.py ==="
cat src/smolvm/images/published.py

echo "=== builder.py ==="
cat src/smolvm/images/builder.py

Length of output: 160216



@yanurag-dev — Right then. I've pulled the full code and done a proper review. By order of the Peaky Blinders, here's what I found.


Full Review — PR #370: Integrate libkrun Backend

✅ Previously Flagged Issues (Addressed in commit b4c82b5)

The following issues were raised in my earlier review pass and confirmed fixed:

Issue File Status
Hardcoded podman/5.8.3/ version path in _GVPROXY_CANDIDATES _libkrun_launcher.py ✅ Fixed — now uses glob pattern for Cellar and static fallbacks
rustup_bin is None and which("cargo") is None insufficient guard doctor.py ✅ Fixed — now just if rustup_bin is None:
krun_set_gvproxy_path failure message lacked recovery guidance _libkrun_launcher.py ✅ Fixed — now includes brew install libkrun suggestion
Unused from concurrent.futures import process / from pyexpat import errors vm.py ✅ Fixed
Async process.returncode read without poll() libkrun.py ✅ Fixed — now uses process.poll() if hasattr(process, "poll") else process.returncode
asyncio.create_task(_close_log_when_done()) not stored vm.py ✅ Fixed per b4c82b5
Error messages not actionable (no vm_id recovery command) libkrun.py ✅ Fixed — error now includes smolvm delete {vm_id}

⚠️ Issues Still Present in Current Code

1. Dead commented-out code in src/smolvm/runtime/libkrun.py

Lines 100–113 contain the entire old _wait_for_runtime implementation commented out. This is dead weight — the history's in git.

# Remove the entire commented block:
#    # def _wait_for_runtime(self, process: Any, boot_timeout: float) -> None:
#    #     import time
#    # ...
#    #     raise SmolVMError(
#    #         "Timed out waiting for libkrun runtime to become ready",
#    #         {"timeout_seconds": boot_timeout, "pid": process.pid},
#    #     )

2. Stale commented-out volume-mount line in src/smolvm/images/builder.py

There's a commented-out old Docker volume mount sitting right above the replacement:

# f"{tar_path.resolve()}:/work/in/rootfs.tar:ro",   # ← remove this
f"{tar_path.parent.resolve()}:/work/in:ro",

3. Predictable socket path — minor security concern

In _libkrun_launcher.py:

sock_path = f"/tmp/krun-gvproxy-{os.getpid()}.sock"

PID-based names in /tmp are predictable. A symlink attack or socket hijack is possible in multi-user environments. Prefer tempfile.mktemp(suffix=".sock", prefix="krun-gvproxy-", dir="/tmp") or use a random hex suffix like uuid4().hex[:8].

4. Dead fallback branch after early-return fix in doctor.py

After the if rustup_bin is None: return early exit was added, the rustup_bin or "rustup" fallback on the subprocess.run call can never be triggered. It reads:

result = subprocess.run(
    [rustup_bin or "rustup", "target", "list", "--installed"],  # `or "rustup"` is dead

Simplify to [rustup_bin, ...] — the or "rustup" path is unreachable now.

5. Missing space in function signature (style)

In _libkrun_launcher.py:

def _start_gvproxy(sock_path: str,ssh_host_port: int) -> subprocess.Popen:

Missing space after the comma. Minor, but inconsistent with PEP 8 and the rest of the codebase.

6. _libkrun private function leaking across module boundary

_libkrun_launcher.py imports:

from smolvm.runtime._libkrun_ffi import KERNEL_FORMAT_RAW, KrunContext, _libkrun

The _libkrun() function is a private internal loader. The launcher accessing it to call krun_set_gvproxy_path bypasses KrunContext's interface. Consider either exposing krun_set_gvproxy_path as a proper KrunContext method, or exporting _libkrun with a less misleading name like get_libkrun_lib().


❌ Pre-merge Check Failures Still Outstanding

Check Detail
Wrong linked issue PR closes #356 (browser/desktop sandbox API design) — no relation to libkrun. Should link #142 or #283 per the walkthrough's "Possibly related issues".
Docstring coverage 35.29% against an 80% required threshold. New files _libkrun_ffi.py and _libkrun_launcher.py have sparse docstrings — only KrunContext has one.

🔍 Verification of macOS Testing

Per the PR description: smolvm create --backend libkrun, smolvm ssh sbx-riemann, and smolvm doctor --backend libkrun were all verified on macOS ARM64. The test suite (1307 passed, 3 pre-existing failures unrelated to this PR) was also run.


Summary

The substance of this PR is solid — the architecture (ctypes FFI, dedicated launcher subprocess, JSON config handoff) is clean and the macOS/Linux platform split is handled correctly. The issues above are mostly cleanup: dead code, a minor security note on the socket path, and a misleading linked issue. Fix those and it's ready.

🐇 ≽^• ⩊ •^≼

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot added the codex label Jun 15, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/smolvm/images/builder.py (1)

1829-1829: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

The shell expects rootfs.tar, but that name isn't enforced anywhere now.

Right, listen carefully. The old mount explicitly placed the file at /work/in/rootfs.tar regardless of its actual name. The new mount preserves the original filename — so if tar_path.name ever differs from rootfs.tar, the shell command on line 1829 will fail silently.

All current callers use rootfs.tar, so nothing's broken today. But this is a hidden contract waiting to bite someone. Either use tar_path.name dynamically or add a guard.

🛡️ Proposed fix using dynamic filename
+        tar_name = shlex.quote(tar_path.name)
         rootfs_name = shlex.quote(rootfs_path.name)
         shell_cmd = (
             "set -e; "
             "apk add --no-cache e2fsprogs tar >/dev/null; "
             "mkdir -p /work/rootfs-staging; "
-            "tar -xf /work/in/rootfs.tar -C /work/rootfs-staging; "
+            f"tar -xf /work/in/{tar_name} -C /work/rootfs-staging; "
             f"mke2fs -d /work/rootfs-staging -t ext4 -F /work/out/{rootfs_name} "
             f"{rootfs_size_mb}M >/dev/null"
         )

Also applies to: 1841-1842

🤖 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 `@src/smolvm/images/builder.py` at line 1829, The shell command at line 1829
uses a hardcoded filename `/work/in/rootfs.tar` for the tar extraction, but the
actual file mounted at that location may have a different name (from
`tar_path.name`). Replace the hardcoded `rootfs.tar` reference with
`tar_path.name` to dynamically use the actual filename being mounted. Apply the
same fix at lines 1841-1842 where similar hardcoded filename references exist.
src/smolvm/vm.py (1)

3145-3152: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

By order of the Peaky Blinders, this async path's got a serious problem, mate.

The sync create() at lines 1752-1757 conditionally uses LIBKRUN_GUEST_IP and LIBKRUN_GATEWAY_IP when backend == BACKEND_LIBKRUN. This async path doesn't — it always uses QEMU_GUEST_IP and QEMU_GATEWAY_IP. That means any libkrun VM created via async_create() gets the wrong network configuration and won't be able to reach its gateway.

🐛 Proposed fix
             if not self._uses_host_tap_networking(effective_config, backend):
                 if (
                     effective_config.internet_settings is not None
                     and not effective_config.internet_settings.is_allow_all_domains
                 ):
                     logger.warning(
                         "internet_settings domain allowlist is not supported "
                         "with the %s backend (user-mode networking)",
                         backend,
                     )
                 mac_seed = ((ssh_host_port or SSH_PORT_START) % 65534) + 1
                 guest_mac = self.network.generate_mac(mac_seed)
+                guest_ip = LIBKRUN_GUEST_IP if backend == BACKEND_LIBKRUN else QEMU_GUEST_IP
+                gateway_ip = LIBKRUN_GATEWAY_IP if backend == BACKEND_LIBKRUN else QEMU_GATEWAY_IP
                 network_config = NetworkConfig(
-                    guest_ip=QEMU_GUEST_IP,
-                    gateway_ip=QEMU_GATEWAY_IP,
+                    guest_ip=guest_ip,
+                    gateway_ip=gateway_ip,
                     netmask=QEMU_NETMASK,
                     tap_device="usernet",
                     guest_mac=guest_mac,
                     ssh_host_port=ssh_host_port,
                 )
🤖 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 `@src/smolvm/vm.py` around lines 3145 - 3152, The async NetworkConfig
initialization in the async path does not conditionally select IP addresses
based on the backend type, unlike the sync create() method. Add a conditional
check for backend == BACKEND_LIBKRUN to the NetworkConfig creation around line
3145-3152, and use LIBKRUN_GUEST_IP and LIBKRUN_GATEWAY_IP when the backend is
libkrun, while using QEMU_GUEST_IP and QEMU_GATEWAY_IP otherwise. This matches
the behavior already implemented in the sync create() method at lines 1752-1757
and ensures libkrun VMs get the correct network configuration.
🧹 Nitpick comments (4)
src/smolvm/images/builder.py (2)

1890-1892: ⚡ Quick win

Move the import to the top of the file, where it belongs.

By order of the Peaky Blinders, imports go at the module level with the other imports — not scattered inside functions like loose hay in a stable.

♻️ Proposed fix

Add uuid to the imports at the top of the file (around line 30-35):

 import tarfile
 import tempfile
 import typing
 import urllib.error
 import urllib.request
+import uuid
 from contextlib import contextmanager

Then remove the inline import:

-        import uuid
         tmp_path = image_dir / f".build-{uuid.uuid4().hex[:8]}"
🤖 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 `@src/smolvm/images/builder.py` around lines 1890 - 1892, Move the `uuid`
import statement from inside the function (where it currently appears with
`import uuid`) to the module-level imports at the top of the file where other
imports are declared. Remove the inline `import uuid` statement from inside the
function and add it to the import section at the top of the file with the other
module-level imports.

1841-1841: ⚡ Quick win

Remove the commented-out line, mate. Git remembers everything.

Dead code clutters the file. The old volume mount is preserved in version history if anyone needs it.

♻️ Proposed fix
-                    # f"{tar_path.resolve()}:/work/in/rootfs.tar:ro",
                     f"{tar_path.parent.resolve()}:/work/in:ro",
🤖 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 `@src/smolvm/images/builder.py` at line 1841, Remove the entire commented-out
line containing the volume mount string with
tar_path.resolve():/work/in/rootfs.tar:ro. This commented code clutters the file
and is unnecessary since git version history preserves it if needed later.
src/smolvm/host/doctor.py (1)

192-198: ⚡ Quick win

The or "rustup" fallback is dead code now, mate.

Right, here's the situation. After the early return at lines 185-191, rustup_bin is guaranteed to be non-None. The or "rustup" fallback on line 194 can never execute. Clean it up — dead branches are how confusion breeds.

🧹 Proposed cleanup
     try:
         result = subprocess.run(
-            [rustup_bin or "rustup", "target", "list", "--installed"],
+            [rustup_bin, "target", "list", "--installed"],
             capture_output=True,
             text=True,
             check=False,
             timeout=5,
         )
🤖 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 `@src/smolvm/host/doctor.py` around lines 192 - 198, The `or "rustup"` fallback
in the subprocess.run call is unreachable dead code because rustup_bin is
guaranteed to be non-None after the early return check at lines 185-191. Remove
the fallback and replace the list argument from [rustup_bin or "rustup", ...] to
use rustup_bin directly, since it is guaranteed to have a value at this point in
the function.
src/smolvm/vm.py (1)

2687-2688: 💤 Low value

Now listen, we don't need to import what's already in the family.

platform is already imported at line 27. This redundant import inside the function is unnecessary weight.

♻️ Suggested fix
     def _build_libkrun_config(self, vm_info: VMInfo) -> dict[str, Any]:
-        import platform
         from smolvm.runtime._libkrun_ffi import KERNEL_FORMAT_ELF, KERNEL_FORMAT_RAW
🤖 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 `@src/smolvm/vm.py` around lines 2687 - 2688, Remove the redundant `import
platform` statement from lines 2687-2688, as the platform module is already
imported at the module level (line 27). Keep only the import statement for
`smolvm.runtime._libkrun_ffi` in this location since that import is specific to
this function's scope.
🤖 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 `@src/smolvm/runtime/_libkrun_launcher.py`:
- Line 91: The sock_path variable uses os.getpid() which creates a predictable
socket name vulnerable to symlink attacks on multi-user systems. Replace the
PID-based suffix in the sock_path assignment with a cryptographically random
suffix or use tempfile.mktemp() instead to generate an unpredictable socket path
that cannot be predicted or hijacked by other users.
- Around line 76-78: The error message printed to stderr when the gvproxy socket
fails to appear only states the problem but omits recovery guidance required by
coding guidelines. Enhance the print statement that outputs to sys.stderr (the
one starting with "gvproxy socket never appeared") to include both the recovery
action the user should take and the exact command they can execute to resolve
the issue, following the guideline that user-facing error messages must name the
recovery action and include the exact recovery command.

In `@src/smolvm/vm.py`:
- Around line 1315-1319: The error message in the libkrun library check does not
include the specific recovery command needed to fix the issue. Update the error
message appended when _find_libkrun_library() returns false to include the exact
command users should run to install libkrun, such as the appropriate package
manager command for their system (e.g., apt-get, brew, etc.), so they have
clear, actionable steps to resolve the problem rather than just a general
description.

In `@tests/test_vm.py`:
- Line 210: The mock return value for _find_libkrun_library on line 210 uses
Path("/usr/lib/libkrun.so"), but the actual _find_libkrun_library function
returns a bool, not a Path object. While the test passes because Path is truthy,
this is a type mismatch. Change the mock return value from
Path("/usr/lib/libkrun.so") to True to match the actual function's return type
contract.

---

Outside diff comments:
In `@src/smolvm/images/builder.py`:
- Line 1829: The shell command at line 1829 uses a hardcoded filename
`/work/in/rootfs.tar` for the tar extraction, but the actual file mounted at
that location may have a different name (from `tar_path.name`). Replace the
hardcoded `rootfs.tar` reference with `tar_path.name` to dynamically use the
actual filename being mounted. Apply the same fix at lines 1841-1842 where
similar hardcoded filename references exist.

In `@src/smolvm/vm.py`:
- Around line 3145-3152: The async NetworkConfig initialization in the async
path does not conditionally select IP addresses based on the backend type,
unlike the sync create() method. Add a conditional check for backend ==
BACKEND_LIBKRUN to the NetworkConfig creation around line 3145-3152, and use
LIBKRUN_GUEST_IP and LIBKRUN_GATEWAY_IP when the backend is libkrun, while using
QEMU_GUEST_IP and QEMU_GATEWAY_IP otherwise. This matches the behavior already
implemented in the sync create() method at lines 1752-1757 and ensures libkrun
VMs get the correct network configuration.

---

Nitpick comments:
In `@src/smolvm/host/doctor.py`:
- Around line 192-198: The `or "rustup"` fallback in the subprocess.run call is
unreachable dead code because rustup_bin is guaranteed to be non-None after the
early return check at lines 185-191. Remove the fallback and replace the list
argument from [rustup_bin or "rustup", ...] to use rustup_bin directly, since it
is guaranteed to have a value at this point in the function.

In `@src/smolvm/images/builder.py`:
- Around line 1890-1892: Move the `uuid` import statement from inside the
function (where it currently appears with `import uuid`) to the module-level
imports at the top of the file where other imports are declared. Remove the
inline `import uuid` statement from inside the function and add it to the import
section at the top of the file with the other module-level imports.
- Line 1841: Remove the entire commented-out line containing the volume mount
string with tar_path.resolve():/work/in/rootfs.tar:ro. This commented code
clutters the file and is unnecessary since git version history preserves it if
needed later.

In `@src/smolvm/vm.py`:
- Around line 2687-2688: Remove the redundant `import platform` statement from
lines 2687-2688, as the platform module is already imported at the module level
(line 27). Keep only the import statement for `smolvm.runtime._libkrun_ffi` in
this location since that import is specific to this function's scope.
🪄 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: 1a5f9665-e460-4512-98fb-c6038765fb3e

📥 Commits

Reviewing files that changed from the base of the PR and between 997741e and 0d08bec.

📒 Files selected for processing (9)
  • src/smolvm/host/doctor.py
  • src/smolvm/images/builder.py
  • src/smolvm/images/published.py
  • src/smolvm/runtime/_libkrun_ffi.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/vm.py
  • tests/test_custom_boot_api.py
  • tests/test_vm.py

Comment thread src/smolvm/runtime/_libkrun_launcher.py
Comment thread src/smolvm/runtime/_libkrun_launcher.py Outdated
Comment thread src/smolvm/vm.py
Comment thread tests/test_vm.py Outdated
Comment thread src/smolvm/images/builder.py Outdated
Comment thread src/smolvm/images/builder.py
Comment thread src/smolvm/images/builder.py
Comment thread src/smolvm/host/doctor.py Outdated
Comment thread src/smolvm/runtime/libkrun.py
Comment thread src/smolvm/runtime/_libkrun_launcher.py Outdated
@yanurag-dev

Copy link
Copy Markdown
Contributor

@SauravMishraaa There are few comments mentioned by coderabbitai about dead code address them too.

@coderabbitai coderabbitai Bot added the CI label Jun 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
src/smolvm/host/doctor.py (1)

185-185: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Trailing whitespace, mate — the pipeline's not having it.

Right, there's a space after that colon that's causing Ruff to fail the build. Remove it and we can proceed with business.

🔧 Proposed fix
-    if rustup_bin is None: 
+    if rustup_bin is None:
🤖 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 `@src/smolvm/host/doctor.py` at line 185, Remove the trailing whitespace that
appears after the colon in the condition check `if rustup_bin is None:`. This
whitespace at the end of the line is causing Ruff linting checks to fail during
the build process.

Source: Pipeline failures

🧹 Nitpick comments (1)
src/smolvm/host/doctor.py (1)

194-194: ⚡ Quick win

Dead code, Tommy — the fallback can never run.

Listen carefully. At line 185, if rustup_bin is None, we return early. So by the time we reach line 194, rustup_bin is guaranteed to have a value. The or "rustup" is dead weight — cluttering the operation. Clean it up.

♻️ Proposed fix
-            [rustup_bin or "rustup", "target", "list", "--installed"],
+            [rustup_bin, "target", "list", "--installed"],
🤖 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 `@src/smolvm/host/doctor.py` at line 194, The `or "rustup"` fallback in the
rustup_bin reference at line 194 is unreachable dead code because an early
return at line 185 ensures rustup_bin is never None at that point. Remove the
`or "rustup"` part and use rustup_bin directly in the list passed to the
subprocess call, since its value is guaranteed to exist by this line.
🤖 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 `@src/smolvm/host/doctor.py`:
- Line 724: The fix parameter assignment in the conditional expression is
exceeding Ruff's 100 character line limit at 149 characters. Break this long
line by splitting the string literal across multiple lines using implicit string
concatenation or explicit line continuation. The fix string contains brew and
dnf installation commands separated by a newline character; consider
restructuring the assignment to keep each logical part (the conditional, brew
command, and dnf command) within the character limit while maintaining the
readability of the installation instructions.

---

Outside diff comments:
In `@src/smolvm/host/doctor.py`:
- Line 185: Remove the trailing whitespace that appears after the colon in the
condition check `if rustup_bin is None:`. This whitespace at the end of the line
is causing Ruff linting checks to fail during the build process.

---

Nitpick comments:
In `@src/smolvm/host/doctor.py`:
- Line 194: The `or "rustup"` fallback in the rustup_bin reference at line 194
is unreachable dead code because an early return at line 185 ensures rustup_bin
is never None at that point. Remove the `or "rustup"` part and use rustup_bin
directly in the list passed to the subprocess call, since its value is
guaranteed to exist by this line.
🪄 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: ed1059cc-f4f0-422a-a940-483f399c993b

📥 Commits

Reviewing files that changed from the base of the PR and between 0d08bec and 1868ecb.

📒 Files selected for processing (7)
  • src/smolvm/host/doctor.py
  • src/smolvm/images/builder.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/vm.py
  • tests/test_custom_boot_api.py
  • tests/test_vm.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • tests/test_vm.py
  • tests/test_custom_boot_api.py
  • src/smolvm/runtime/libkrun.py
  • src/smolvm/runtime/_libkrun_launcher.py
  • src/smolvm/vm.py

Comment thread src/smolvm/host/doctor.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR completes the libkrun backend integration by switching libkrun VM startup from the krunvm CLI to an in-tree ctypes + launcher subprocess approach, adds richer smolvm doctor --backend libkrun diagnostics, and updates kernel-format selection to reflect macOS Hypervisor.framework constraints.

Changes:

  • Replace krunvm CLI launching with a Python launcher (_libkrun_launcher) and ctypes bindings (_libkrun_ffi) plus JSON config handoff.
  • Add/expand libkrun prerequisite diagnostics in smolvm doctor (library loadability, gvproxy on macOS, Rust musl target, hypervisor entitlement, KVM on Linux).
  • Make libkrun kernel-format selection OS-aware (ELF on Linux; Image on macOS) and adjust libkrun usernet IP defaults.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/smolvm/vm.py libkrun prerequisites + launcher-based startup config; libkrun-specific usernet IP defaults
src/smolvm/runtime/_libkrun_ffi.py New ctypes binding layer for loading and calling libkrun
src/smolvm/runtime/_libkrun_launcher.py New child-process entrypoint that configures libkrun and gvproxy (macOS) then blocks in krun_start_enter
src/smolvm/runtime/libkrun.py Adjust libkrun runtime readiness/wait logic to fail-fast on early exits
src/smolvm/host/doctor.py New/expanded doctor checks for libkrun dependencies and platform-specific requirements
src/smolvm/images/published.py Kernel-format selection updated for libkrun on macOS vs Linux
src/smolvm/images/builder.py Fix Docker rootfs extraction mount behavior and use an explicit build temp dir under image_dir
tests/test_vm.py Update libkrun prerequisite test to reflect library-based detection
tests/test_custom_boot_api.py Add libkrun kernel-format tests split by Linux vs Darwin behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/smolvm/runtime/_libkrun_launcher.py Outdated
Comment thread src/smolvm/vm.py
aniketmaurya and others added 2 commits June 16, 2026 10:25
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@aniketmaurya
aniketmaurya enabled auto-merge (squash) June 16, 2026 16:38
@aniketmaurya
aniketmaurya merged commit 5af9161 into CelestoAI:main Jun 16, 2026
7 checks passed
@SauravMishraaa
SauravMishraaa deleted the feature/libkrun-integration branch June 16, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants