spec(minvmd-host-daemon): minvmd macOS VM provider host daemon - #313
Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive planning specification for Changesminvmd Host Daemon Specification
Sequence Diagram(s)sequenceDiagram
participant minimal_cli as minimal (CLI)
participant minvmd as minvmd (host daemon)
participant libkrun as libkrun (FFI)
participant vm as microVM (guest)
participant minimald as minimald (in-VM service)
minimal_cli->>minvmd: connect to host UDS (ssh.sock)
minvmd->>libkrun: invoke VM boot (kernel + rootfs)
libkrun->>vm: start VM
vm->>minvmd: vsock READY marker
minvmd->>vm: bridge UDS↔vsock (forward connections)
minimal_cli->>minimald: reach minimald via bridged vsock
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md (5)
410-411: ⚡ Quick winClarify the security implications of deferred image provenance.
The statement "Image provenance is deferred — v0.1 reads kernel + rootfs from caller-supplied paths" understates the security posture: v0.1 implicitly trusts the caller to provide non-malicious kernel and rootfs images, with no integrity verification.
Consider rephrasing to make the trust assumption explicit: "v0.1 trusts caller-supplied kernel and rootfs paths without integrity verification; provenance checking is deferred to a future version."
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around lines 410 - 411, Update the sentence that currently reads "Image provenance is deferred — v0.1 reads kernel + rootfs from caller-supplied paths" to explicitly state the trust model and lack of integrity checks; i.e., change it to something like "v0.1 trusts caller-supplied kernel and rootfs paths without integrity verification; provenance and integrity checking are deferred to a future version" so readers understand the security implication.
246-251: 💤 Low valueForward-reference the lifecycle state enum.
R4.1 mentions "lifecycle enum" in
state.tomlbut doesn't define the enum values. The definition appears later in Technical Considerations (lines 394-395) asNotProvisioned | Stopped | Starting | Running | Stopping.For better readability, consider adding a forward reference here: "lifecycle enum (see Technical Considerations)" or defining the enum inline within R4.1.
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around lines 246 - 251, Update R4.1 to forward-reference or inline the lifecycle enum so readers know the allowed values: modify the R4.1 sentence that currently refers to "lifecycle enum" in state.toml to either append "(see Technical Considerations for lifecycle enum: NotProvisioned | Stopped | Starting | Running | Stopping)" or expand it inline to list those enum values; ensure you reference the same symbol names (state.toml, lifecycle enum, and the enum values NotProvisioned, Stopped, Starting, Running, Stopping) to keep the spec consistent with the later Technical Considerations section.
198-203: ⚡ Quick winClarify XDG_RUNTIME_DIR fallback conditions.
R3.2 specifies the host UDS path as
$XDG_RUNTIME_DIR/minimal/minimald.sockwith fallback to~/.minimal/local/minimald.sock, but doesn't state when the fallback is used. Is it whenXDG_RUNTIME_DIRis unset, or when it's inaccessible?Additionally, the requirement states
minvmd"shall create the parent directory (mode 0700)" but doesn't clarify whether this means:
- Create with mode 0700 if absent (scoped to creation), or
- Enforce mode 0700 on pre-existing directories (verify and chmod)
Based on learnings, the pattern is (1) — mode is set only on creation. Consider making this explicit: "shall create the parent directory with mode 0700 (if absent)" to match the R4.5 phrasing pattern.
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around lines 198 - 203, Clarify R3.2 by stating when the fallback path is used and that mode-setting applies only on creation: update the R3.2 text for the host UDS path to say that the socket path is $XDG_RUNTIME_DIR/minimal/minimald.sock and that if XDG_RUNTIME_DIR is unset or not usable the implementation shall use the fallback ~/.minimal/local/minimald.sock; and change the parent-directory requirement for minvmd to read something like "minvmd shall create the parent directory with mode 0700 if absent" so it is clear minvmd must set 0700 on creation but not forcibly chmod existing directories (reference symbols: R3.2, minvmd, XDG_RUNTIME_DIR, mode 0700, fallback ~/.minimal/local/minimald.sock).
149-153: 💤 Low valueClarify process spawning terminology.
R2.3 uses "exec-spawn" to describe the child process creation, which mixes two distinct Unix syscall concepts (exec replaces the current process; spawn/fork creates a new one). The design considerations section (lines 316-319) clarifies this is a standard parent-supervises-child fork model.
Consider revising to "spawn" or "fork-exec-spawn" for precision.
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around lines 149 - 153, R2.3's phrase "exec-spawn" is ambiguous; update the text for precision to reflect the fork-plus-exec parent-supervises-child model—replace "exec-spawn a hidden `minvmd __krun-vmm` child" with a clearer term like "fork-exec (spawn) a hidden `minvmd __krun-vmm` child" and keep the rest of the sentence referencing `krun_start_enter`, the parent writing `vmm.pid`, and signal handling unchanged so readers understand the parent supervises the child process.
21-27: ⚡ Quick winClarify scope: existing vs. remaining work.
The context states "The crate already exists...with FFI bindings (
src/krun/raw.rs), safe wrappers (src/krun/ctx.rs)...and a CLI skeleton," yet Unit 1 requirements (R1.2, R1.3) use "shall" to define those same FFI bindings and safe wrappers. This creates ambiguity: are R1.2/R1.3 validation criteria for existing code, or are they defining new work?Consider adding a sentence clarifying whether Unit 1 is landing greenfield code or refining/completing existing scaffolding.
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around lines 21 - 27, Update the spec to remove ambiguity by explicitly stating whether Unit 1 (R1.2 and R1.3) are acceptance criteria for existing implementations or requirements to be implemented anew: mention the existing crate crates/minvmd/ and its files src/krun/raw.rs and src/krun/ctx.rs (and the CLI skeleton) and then state whether R1.2 (FFI bindings) and R1.3 (safe wrappers) are being validated as already-present or must be completed/landed as part of this work; add one sentence after the crate description clarifying "validation vs. new work" so readers know if Unit 1 is checking current scaffolding or specifying new deliverables.
🤖 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.
Nitpick comments:
In `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md`:
- Around line 410-411: Update the sentence that currently reads "Image
provenance is deferred — v0.1 reads kernel + rootfs from caller-supplied paths"
to explicitly state the trust model and lack of integrity checks; i.e., change
it to something like "v0.1 trusts caller-supplied kernel and rootfs paths
without integrity verification; provenance and integrity checking are deferred
to a future version" so readers understand the security implication.
- Around line 246-251: Update R4.1 to forward-reference or inline the lifecycle
enum so readers know the allowed values: modify the R4.1 sentence that currently
refers to "lifecycle enum" in state.toml to either append "(see Technical
Considerations for lifecycle enum: NotProvisioned | Stopped | Starting | Running
| Stopping)" or expand it inline to list those enum values; ensure you reference
the same symbol names (state.toml, lifecycle enum, and the enum values
NotProvisioned, Stopped, Starting, Running, Stopping) to keep the spec
consistent with the later Technical Considerations section.
- Around line 198-203: Clarify R3.2 by stating when the fallback path is used
and that mode-setting applies only on creation: update the R3.2 text for the
host UDS path to say that the socket path is
$XDG_RUNTIME_DIR/minimal/minimald.sock and that if XDG_RUNTIME_DIR is unset or
not usable the implementation shall use the fallback
~/.minimal/local/minimald.sock; and change the parent-directory requirement for
minvmd to read something like "minvmd shall create the parent directory with
mode 0700 if absent" so it is clear minvmd must set 0700 on creation but not
forcibly chmod existing directories (reference symbols: R3.2, minvmd,
XDG_RUNTIME_DIR, mode 0700, fallback ~/.minimal/local/minimald.sock).
- Around line 149-153: R2.3's phrase "exec-spawn" is ambiguous; update the text
for precision to reflect the fork-plus-exec parent-supervises-child
model—replace "exec-spawn a hidden `minvmd __krun-vmm` child" with a clearer
term like "fork-exec (spawn) a hidden `minvmd __krun-vmm` child" and keep the
rest of the sentence referencing `krun_start_enter`, the parent writing
`vmm.pid`, and signal handling unchanged so readers understand the parent
supervises the child process.
- Around line 21-27: Update the spec to remove ambiguity by explicitly stating
whether Unit 1 (R1.2 and R1.3) are acceptance criteria for existing
implementations or requirements to be implemented anew: mention the existing
crate crates/minvmd/ and its files src/krun/raw.rs and src/krun/ctx.rs (and the
CLI skeleton) and then state whether R1.2 (FFI bindings) and R1.3 (safe
wrappers) are being validated as already-present or must be completed/landed as
part of this work; add one sentence after the crate description clarifying
"validation vs. new work" so readers know if Unit 1 is checking current
scaffolding or specifying new deliverables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e55207e7-4bf4-460c-badd-9a4665f7a678
📒 Files selected for processing (1)
docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md
|
/revise Address code rabbit concerns and adhere to conventional commits as per the guidelines of this report. Ensure additional agents working from this spec adhere to repo conventions as well |
|
Applied
|
This comment has been minimized.
This comment has been minimized.
- Clarify Unit 1 scope: R1.2/R1.3 are acceptance criteria for existing scaffolding, not greenfield deliverables - Fix R2.3 terminology: replace ambiguous exec-spawn with fork-exec - Clarify R3.2 XDG_RUNTIME_DIR fallback condition (unset) and parent-directory creation (mode 0700 if absent) - Inline lifecycle enum values in R4.1 for readability - Make image provenance trust model explicit in Security Considerations - Expand Repository Standards with conventional commits and Rust coding standards references for downstream agents Refs: #311 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4b3501d to
ac03593
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md`:
- Around line 402-405: The spec currently hardcodes /opt/homebrew/lib as the
default libkrun rpath via build.rs (emitting cargo:rustc-link-search and
cargo:rustc-link-arg=-Wl,-rpath) with only a LIBKRUN_PREFIX env override; update
the spec to use architecture-aware defaults or Homebrew-prefix discovery
instead: state that build.rs should detect platform/arch (e.g., ARM64 vs x86_64)
and prefer /opt/homebrew/lib on Apple Silicon and /usr/local/lib on Intel, or
invoke Homebrew prefix discovery (e.g., `$(brew --prefix)` style) when present,
and document that LIBKRUN_PREFIX can still override this behavior; reference
build.rs, the cargo:rustc-link-search and cargo:rustc-link-arg=-Wl,-rpath
emissions, and the LIBKRUN_PREFIX env var so implementers know where to change
defaults.
- Around line 71-72: The spec currently contradicts itself about Linux behavior
for minvmd (one place says “not be installed” and other places describe a
“compile no-op shim”); pick a single canonical contract (either “not installed
at all on Linux” or “installed as a no-op shim”) and update all
occurrences—including the sentence at the current diff and the other mentions
around lines referenced (51-53 and 96-97)—so the document consistently states
the chosen behavior, updates packaging/CLI notes to match (remove the alternate
outcome), and adjusts any examples or installation steps that assume the other
behavior; search for “minvmd”, “not be installed”, and “compile no-op shim” to
locate and change every instance.
🪄 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: 698b5b47-987b-46c2-8b2b-7152bd37f690
📒 Files selected for processing (1)
docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md
| - As a Linux user, I want `minvmd` to not be installed at all so that my | ||
| install path stays simple. |
There was a problem hiding this comment.
Clarify Linux behavior: “not installed” conflicts with “compile no-op shim.”
This currently defines two different Linux outcomes (no installation vs installed stub). Please resolve to one contract so packaging and CLI integration are unambiguous.
Also applies to: 51-53, 96-97
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around
lines 71 - 72, The spec currently contradicts itself about Linux behavior for
minvmd (one place says “not be installed” and other places describe a “compile
no-op shim”); pick a single canonical contract (either “not installed at all on
Linux” or “installed as a no-op shim”) and update all occurrences—including the
sentence at the current diff and the other mentions around lines referenced
(51-53 and 96-97)—so the document consistently states the chosen behavior,
updates packaging/CLI notes to match (remove the alternate outcome), and adjusts
any examples or installation steps that assume the other behavior; search for
“minvmd”, “not be installed”, and “compile no-op shim” to locate and change
every instance.
| - **libkrun linking** — pinned to a known-good version (v1.18.0). | ||
| `build.rs` emits `cargo:rustc-link-search` and | ||
| `cargo:rustc-link-arg=-Wl,-rpath` pointing at `/opt/homebrew/lib` | ||
| with a `LIBKRUN_PREFIX` env override. |
There was a problem hiding this comment.
Default libkrun link path risks breaking x86_64 macOS builds.
Using /opt/homebrew/lib as the default is Apple Silicon-specific; Intel Homebrew commonly uses /usr/local/lib. Since x86_64 support is a stated goal, specify architecture-aware defaults (or Homebrew-prefix discovery) in the spec instead of relying on manual env override.
Also applies to: 51-52
🤖 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 `@docs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md` around
lines 402 - 405, The spec currently hardcodes /opt/homebrew/lib as the default
libkrun rpath via build.rs (emitting cargo:rustc-link-search and
cargo:rustc-link-arg=-Wl,-rpath) with only a LIBKRUN_PREFIX env override; update
the spec to use architecture-aware defaults or Homebrew-prefix discovery
instead: state that build.rs should detect platform/arch (e.g., ARM64 vs x86_64)
and prefer /opt/homebrew/lib on Apple Silicon and /usr/local/lib on Intel, or
invoke Homebrew prefix discovery (e.g., `$(brew --prefix)` style) when present,
and document that LIBKRUN_PREFIX can still override this behavior; reference
build.rs, the cargo:rustc-link-search and cargo:rustc-link-arg=-Wl,-rpath
emissions, and the LIBKRUN_PREFIX env var so implementers know where to change
defaults.
sdd-validate · Spec boundary · Clean pass ✅Boundary: Spec (adds
No findings. All spec gates passed clean.
|
Adds the specification for the
minvmdmacOS VM provider host daemon atdocs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.md.Translated from the plan document in #311 (
plan:provided).Demoable Units
ssh.sock— R3.1–R3.5Next step
Please review and merge this spec PR to advance #311 from the spec phase into triage. Merging the spec PR will close the spec sub-issue (
Closeskeyword is added bysdd-pr-sanitize).#311
Closes #312
Summary by CodeRabbit