Repository navigation
Do not review: CI test - #585
Closed
norrietaylor wants to merge 40 commits into
Closed
norrietaylor wants to merge 40 commits into
norrietaylor wants to merge 40 commits into
Conversation
`minimal activate` hardcoded NetworkMode::default() (HostNet) with empty policy, so NoNet/OwnIp and ingress port mappings had no CLI surface and were reachable only via the netns proofs constructing a Record directly. - `--network <no-net|host-net|own-ip>` (default host-net; no-flag behavior unchanged) via a local CliNetworkMode ValueEnum, keeping the sessions crate free of a clap dependency. - `--ingress EXT:INT[/PROTO]` (repeatable; PROTO defaults tcp) parsed into sessions::PortMapping; non-tcp/udp and malformed specs rejected at parse time. - Surface the daemon's typed CreateSession validation error (e.g. ingress on a non-own-ip session) instead of a generic failure line. Daemon-side validate_policy is unchanged and remains the enforcement point. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
On DM1/3/4 (a libkrun VM) gvproxy must run on the host, owned by minvmd, not inside the guest where it has no host uplink (issue #572). Wire up the previously-dead host-side switch: - HostGvproxy: an RAII supervisor that spawns the host gvproxy switch (reusing GvproxyConfig::spawn) on a dedicated current-thread tokio runtime, so minvmd's synchronous boot/run supervisor can own an async switch lifecycle and tear it down on drop. Started with an empty static-lease table — the guest configures each PTask's switch IP statically, so minvmd need not own the per-PTask address book. - run --foreground spawns + supervises the host gvproxy for an own-IP VM before booting the VMM child, so the -listen switch socket exists when libkrun dials it; the handle is held for the VM's lifetime. - VmConfig::apply registers the per-PTask shuttle vsock bridge for an own-IP VM: add_vsock_port2(VSOCK_GVPROXY_SHUTTLE_PORT, switch_sock, listen = false) — the guest shuttle connects to AF_VSOCK CID 2 and libkrun splices it to the host gvproxy -listen socket, carrying raw L2 frames (no second TCP/IP stack). - net::shuttle: the shuttle vsock port + host switch-socket path resolver (placed beside the minimald bridge socket). - image::resolve_gvproxy_path reads MINVMD_GVPROXY_BIN (fallback to the fixed install path); cmd::own_ip_requested reads MINVMD_VM_OWN_IP so the parent's gvproxy-spawn decision and the VMM child's VM network mode stay in lock-step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In a libkrun VM (DM1/3/4) minimald-in-guest must NOT spawn gvproxy locally — gvproxy runs on the host (owned by minvmd) and the guest only relays raw L2 frames to it (issue #572). Previously the DM2 spawn-local path ran verbatim in the guest, so an OwnIp PTask got a 100.64 switch IP but no egress (gvproxy NAT'd into a dead-end netns). - net::SwitchTransport enum (LocalSpawn vs HostShuttle { cid, port }) makes "spawn locally" and "relay to the host switch" mutually exclusive in the type system. GvproxySwitch::with_transport selects it; in HostShuttle mode attach() skips the local gvproxy spawn/config and only tracks the attach count. - switch::attach_to_switch_vsock relays a PTask tap to the host gvproxy over AF_VSOCK (CID 2 : shuttle port); it shares a transport-agnostic spawn_relay with the DM2 UDS attach_to_switch — same HyperKit-framed raw-L2 relay, so exactly one gVisor stack stays in the path. - session_host::attach_own_ip branches on the switch transport: DM2 uses the local control socket, DM1/3/4 the vsock shuttle. Static ingress on a VM own-IP PTask is skipped with a warning (gvproxy's port-forward API is host-side and unreachable over the frame-only shuttle); egress, the #572 focus, needs no such call. - DM signal: the `vsock` listen-arg is exactly the libkrun-VM boundary (DM1/3/4) vs the UDS daemon (DM2); threaded through Config::in_microvm into the transport selection. DM2 behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`just up` materializes the guest kernel + rootfs, builds the networking-enabled guest initramfs, builds minvmd (codesigned on macOS), builds the minimal CLI, then runs `minimal ls` to autospawn the supervised minvmd and reach minimald over the UDS bridge. Recipes branch on $(uname -s): - macOS: `minimal materialize`, Homebrew libkrun, codesign. - Linux: fetch-artifact.sh / fetch-libkrun.sh with LIBKRUN_PREFIX + LD_LIBRARY_PATH exported so build.rs links the real implementation; no codesign; requires durable kvm group membership. build-initramfs.sh gains a FEATURES env hook so the guest minimald is compiled with networking-proxy,networking-wg (the HTTPS mTLS proxy and WireGuard mesh peer); it previously passed no --features. artifacts is idempotent (skips a present artifact) and clean removes only the bring-up files, never the shared .scratch directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract `generate_mfile` from `minimal init` (cmd_init) into mctx::scaffold and add `scaffold_default_mfile`, which resolves the latest gominimal/pkgs @main commit and writes a default shell-stack minimal.toml into a directory that has none. minimald's session `context()` now scaffolds such a file when the session workspace lacks one, so `attach` no longer fails with "minimal.toml not found" on an empty workspace. Scaffolding is lazy (on first attach / context build), not on activate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends the issue #572 per-PTask vsock shuttle to the guest's root network namespace, so minimald itself has egress (default route + DNS) and can fetch the upstream pkgs repo — e.g. the clone that scaffolding a session's minimal.toml performs. minvmd: - spawn the host gvproxy switch for every VM (best-effort: skip with a warning when the gvproxy binary is absent, so the boot/session e2e lanes still boot without egress), not just own-IP VMs. - register the gvproxy shuttle vsock port unconditionally; the root tap and any own-IP PTask taps are all L2 clients on the one switch. minimald: - bring up a primary `eth0` tap in the root netns at boot, attached to the host gvproxy over the vsock shuttle. Configure address/up/default route via AF_INET ioctls (the generic rootfs ships no iproute2), and install /etc/resolv.conf by bind-mounting from the writable /run tmpfs (the rootfs is read-only). Hold the relay for the server's lifetime. - reserve a daemon switch address (`broadcast - 2`) from the top of the subnet so the PTask range still starts at `network + 2`. justfile: `up` fetches gvproxy and exports MINVMD_GVPROXY_BIN so the host switch comes up; gvproxy fetch is idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ary copy The session-workspace scaffolding is a stop-gap for the workspace-upload gap, so keep it isolated rather than refactoring shared code. Revert the extraction of `generate_mfile` from `minimal::cmd_init` (cmd_init keeps its own copy) and instead duplicate the generator inside mctx::scaffold, marked TEMPORARY throughout. Deleting one module then removes the workaround cleanly, with no cmd_init unpicking. mctx now exports only `scaffold_default_mfile`; `generate_mfile` and its constants are private to the module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…com/gominimal/minimal into feat/572-root-netns-egress
… on DM1
Unify the forwarder API behind a `ControlChannel { Unix | Vsock }`:
- DM2 keeps the local unix control socket.
- DM1/3/4 reach the host gvproxy's `-listen` socket over the same vsock
shuttle port the L2 `/connect` relay uses (it serves
`/services/forwarder/*` too), so `post_json` rides vsock there.
Replaces the prior "skip ingress with a warning on the VM path" branch in
`attach_own_ip`: own-IP PTasks in a libkrun VM now get their static
ingress forwards published on the host loopback (gvproxy binds
127.0.0.1:ext -> ptask-switch-ip:int), torn down on session end.
`post_json` is refactored into a transport-generic `exchange` over any
AsyncRead+AsyncWrite stream, shared by the unix and vsock channels.
Verification of the live data path is pending a scriptable interactive
attach: `attach -c` runs the exec path (daemon host netns) and never
mints the own-IP session host, so it cannot exercise this path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…HG1)
Interactive `minimal2 attach` panicked the daemon and never delivered a
working shell. Root causes, fixed here:
- rustls had no process-default CryptoProvider, so the first rustls user
on the attach path (the remote-cache HTTPS client during a session
build) panicked ("no process-level CryptoProvider"), killing the
session actor; the caller then panicked at session.rs:398 on the lost
reply. Install ring as the default provider at minimald startup, and
make `rustls` a normal dep (it is always linked via reqwest anyway).
- The session actor dropping its reply now surfaces as an AttachError
instead of `expect`-panicking the worker.
- `minimal2 attach` (interactive) requested no PTY, so the daemon's
shell_request rejected it whenever stdin was not a tty (automation).
Force one with `ssh -tt` for the interactive path.
- The read-only guest rootfs left `/tmp` unwritable, so remote-cache
staging (tempfile) failed with EROFS; mount a tmpfs at `/tmp`.
With these, attach reaches the in-sandbox session build. Remaining
blocker is cache capacity (the session package set overflows the
tmpfs cache) — the seeded-cache-disk work, tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two in-sandbox-shell bring-up fixes uncovered while exercising the interactive attach path: - Bump the microVM from 1024 to 4096 MiB. The in-VM session build stages packages into a RAM-backed tmpfs cache (/run/minimal/cache); 1024 MiB overflowed (StorageFull) unpacking large packages. Stop-gap until the persistent seeded cache disk lands. - Exec the session shell at /usr/bin/bash, not /bin/bash: the bash package installs to --prefix=/usr and the generic rootfs has no /bin/bash, so the old path failed with ENOENT. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The interactive session shell's server-side `Pty::open` does posix_openpt then opens `/dev/pts/N`, which ENOENT'd because the guest mounted devtmpfs but never mounted devpts at `/dev/pts`. Mount it in `enter_rootfs` (best-effort, so a pty failure degrades to a warning rather than turning boot into READY-only). Matches the "devpts+/tmp" guest-compose fix. Also label the previously-bare error sites in the session launch (container build / pty open / build command) so a future failure names the failing step instead of surfacing as an opaque "session spawn: No such file or directory". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Park point for the HG1 in-sandbox-shell investigation. - devpts: mount with ptmxmode=0666 and repoint /dev/ptmx -> pts/ptmx so posix_openpt and /dev/pts/N share one instance (verified correct in the guest). This removed the pty-creation failure. - log the session process exit code in the host mainloop. Finding (parked): the in-sandbox shell on minvmd fails with the sandbox process exiting 125 — hakoniwa cannot construct the container inside the libkrun guest (works natively on DM2). Not the pty (devpts fixed), not bash, not the package closure (125 != 127). Next: capture hakoniwa's exact setup error (debug logging / sandbox stderr); suspect unprivileged userns or pivot_root for pid-1 in the guest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uest
Two bugs blocked the interactive sandbox shell (HG1) on minvmd while it
worked natively on DM2:
1. exit 125 — guest pid-1 entered the rootfs with a bare chroot(/newroot),
leaving its root dir != the mount-namespace root. The kernel then
refused hakoniwa's unshare(CLONE_NEWUSER) with EPERM (a "chroot
environment" per user_namespaces(7)), so every sandbox build failed.
Enter the rootfs the switch_root(8) way: MS_MOVE /newroot onto / then
chroot("."), so the new root is the namespace root and the unprivileged
userns unshare succeeds. (pivot_root can't move the initramfs source.)
2. vt100 panic — the attach client requests a 0x0 PTY, so WinSize::from
built a 0-cell grid and vt100-ctt panicked on bash's first write,
killing the session worker. Clamp 0 rows/cols to 24x80 (corrected by
the first window_change).
Also log hakoniwa's exit reason on a non-zero sandbox exit (the reason
field was discarded, which made the 125 opaque).
Verified via the full interactive session path (no attach -c): a host-net
session runs an in-sandbox command and curls example.com -> 200 with
eth0+route; a no-net session runs in-sandbox but has no egress (curl 000)
and only lo — namespace isolation holds. No exit 125 / EPERM / panic in
the guest log.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Formatting-only; the ingress-over-vsock commit (ControlChannel) shipped unformatted closures. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… host loopback
Slice 2 of the networking host-exposure unit (unblocks TC3/TC7 on DM1).
Start minimald's two host-side proxies for the daemon's lifetime in
Server::run (both DM1 and DM2), consolidating the startup that previously
only bound (never served) the egress proxy on DM2:
- B5 egress/DNS proxy on :7654 (always);
- B8 mTLS reverse proxy on :7655 (networking-proxy feature), using the
CertAuthority's server config.
Both route by Host header through the sessions manager's PTask hostname
registry, now shared as Arc<RwLock<HostnameRegistry>> (a HostRoute impl on
the RwLock) and reachable via a new ManagerHandle::hostnames() accessor.
Bind address: in a microVM (DM1) the proxies bind the daemon switch IP
(100.64.255.253) so the host gvproxy forward can reach them, then minimald
publishes 127.0.0.1:{7654,7655} -> daemon-ip:{7654,7655} on the macOS host
via the gvproxy forwarder over the vsock control channel (reusing
policy::post_json / ControlChannel::Vsock). On native Linux (DM2) they bind
host loopback directly and need no expose. All startup/expose is
best-effort: a bind or expose failure warns and the daemon keeps serving.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Executable test plan (test-plan.sh) + companion doc (test-plan.md) for the networking epic (#478). Every per-session networking assertion runs INSIDE the sandbox via the full interactive session path (expect-driven `minimal2 attach`, never `attach -c`, which bypasses the sandbox). One reusable in-session driver; host->PTask TCs keep an in-session socat backend alive across the host check; switch-IP via /proc/net/fib_trie (no `ip` in-session); idempotent cleanup. Cert dir is OS-aware (macOS vs XDG). Last run on this branch: TC1/TC1b/TC5/TC6/TC9 PASS, TC7 no-cert->401 PASS; TC2/3/4/7(200)/8 blocked on own-ip PTask attach, which shells out to ip/nsenter (absent from the guest rootfs) in move_tap_into_netns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A panic in one session task could poison the hostname registry RwLock, after which every proxy route resolution silently returned 502. Recover the guard via into_inner() so a poisoned lock still resolves routes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In the microVM the host-side proxies bound the guest switch IP, coupling listener startup to eth0 readiness. Bind 0.0.0.0 instead so the listeners always come up, and only publish a port on the host loopback once its listener has actually bound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scaffold_default_mfile does a blocking git clone inside the async session-actor task; wrap it in block_in_place so it moves other futures off the worker thread instead of stalling them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A clean session exit (code 0) is routine; log it at debug and reserve the warning for non-zero exits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An own-IP VM has no network without the gvproxy switch. Rather than warn and boot a session that silently has no egress, hard-error when the binary is absent and own-ip was requested; point at MINVMD_GVPROXY_BIN. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
apply_ingress/remove_ingress now take a &ControlChannel; wrap the switch control socket in ControlChannel::Unix at the call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
That work is folded into this integration; remove the now-stale issue references from code comments. Comment-only, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-existing formatting drift flagged by CI `cargo fmt --check`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the hand-written Default impl with #[derive(Default)] + #[default] on LocalSpawn; fixes clippy::derivable_impls under -D warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match the sibling SwitchTransport enum and the coding standard for public enums that may grow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A present-but-unlaunchable gvproxy binary (wrong arch, not executable) hard-failed every VM boot, including plain HostNet boots. Now a spawn failure only bails for own-IP VMs and otherwise warns and boots without guest egress, matching the missing-binary path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
own_ip_requested only matched "1"/"true"/"TRUE", so "True", "yes", "on", or padded values silently fell through to a HostNet VM. Trim and lowercase before matching the accepted truthy set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The spawned serve/serve_https tasks dropped their io::Result, so a fatal accept-loop error left the daemon running with a dead proxy and no log. Wrap each task to log the error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
try_wait already warns on a non-zero exit with the richer hakoniwa diagnostics; the mainloop warning double-logged the same event. Keep a single routine debug line in mainloop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…anic
Making rustls non-optional pulled the `ring` provider into the
default-feature build, which already carries `aws-lc-rs` via
google-cloud-auth. With two providers compiled, rustls can no longer
auto-select one and panics ("no process-level CryptoProvider") the first
time a config is built without an explicit install — breaking the mctx
`env_channel_add_session` test under `cargo nextest run --workspace`.
Restore rustls to optional (only `networking-proxy` pulls it) and gate
the startup provider install under that feature — the one configuration
where both providers are present and an explicit install is required.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Introduce a `Network` trait (with `HostNet`/`NoNet` built-ins and a `NetGuard` for explicit teardown) so network setup is configured via the Config builder and applied by the Sandbox, rather than living only in the minimald session host. `Config::with_network` injects a custom impl; `new_container` prefers it for the netns-isolation decision and `Sandbox::attach_network` invokes its post-spawn wiring. Additive and behaviour-preserving: with no custom Network set, the existing `network_mode` path is unchanged. Consumer migration (minimald own-IP switch attach → an injected Network; tasks via mctx) follows. Refs #581 review (twitchyliquid64): networking belongs in sandbox2 so it works for tasks and any sandbox, not just minimald sessions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The two crates each had a type named `GvproxySwitch` with different roles, which obscured the gvproxy ownership model. Rename for clarity: - minvmd: `GvproxySwitch` -> `GvproxySupervisor` (owns + supervises the host gvproxy process via pidfd on DM1/3/4). - minimald: `GvproxySwitch` -> `SwitchClient` (an IP allocator + attach refcounter; on DM2 it spawns gvproxy locally, on DM1/3/4 it spawns no process and relays taps to the host-owned switch over vsock). No behaviour change. Refs #581 review (twitchyliquid64): clarifies the gvproxy object lifecycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 1 minute and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThe PR adds own-IP networking, gvproxy shuttle plumbing, and ingress forwarding across minvmd and minimald. It also scaffolds missing workspace mfiles, updates session attach handling, changes the minimal2 CLI and timeout behavior, and expands build, CI, and networking test support. ChangesNetworking bring-up and support tooling
Sequence Diagram(s)sequenceDiagram
participant m2 as "minimal2 cmd_activate"
participant mh as "sessions::ManagerHandle"
participant sha as "session_host::attach_own_ip"
participant ats as "net::switch::attach_to_switch"
participant atsv as "net::switch::attach_to_switch_vsock"
participant pol as "net::policy::apply_ingress"
participant gp as "gvproxy"
m2->>mh: CreateSession with network and ingress policy
mh->>sha: start session attachment
alt SwitchTransport::LocalSpawn
sha->>ats: attach tap
else SwitchTransport::HostShuttle
sha->>atsv: attach tap
end
sha->>pol: apply_ingress
pol->>gp: POST ExposeRequest / UnexposeRequest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Comment |
Extract the per-PTask switch attach (lease, tap, move-into-netns, frame relay, static ingress) out of the session host into `GvproxyNetwork`, an `impl sandbox2::Network`. The session launcher now constructs it and calls `Network::attach` rather than the inline `attach_own_ip`, and the returned `NetGuard` is torn down explicitly in `mainloop` at session end instead of via a runtime-fragile `Drop` schedule. This puts the networking logic behind the shared sandbox2 abstraction so the same path can serve tasks, not only minimald sessions, and keeps the gvproxy process owner (the daemon-scoped SwitchClient / minvmd host supervisor) outside sandbox2 (spec R1.4). Refs #581 review (twitchyliquid64): networking belongs in sandbox2 and should not be session-only; teardown made explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci-linux-kvm.yml (1)
203-219: 🩺 Stability & Availability | 🟡 MinorExtend the status poll window.
run --detach --timeout 30only bounds the initial host UDS connect wait; the READY wait still uses the 60s default. The 75 × 0.2s status loop caps out at ~15s, which is shorter than that READY budget and can fail beforeStarting -> Runningon slower KVM boots. Consider widening the 10BC8 poll to match the READY timeout.🤖 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 @.github/workflows/ci-linux-kvm.yml around lines 203 - 219, The status polling in the CI KVM workflow is too short and can exit before the VM reaches Running on slower boots. Update the poll loop in the status check section to wait long enough to match the READY timeout used by the detach flow, and keep the same jq-based running-state check around the existing "$BIN" status --json probe.
🧹 Nitpick comments (2)
crates/minimal2/src/main.rs (1)
489-498: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSurfacing the daemon's typed validation error is the right call. Matching
Errorable::Err { error }propagates the daemon's policy/network-mode message (e.g. ingress on a non-OwnIpsession, privileged host port) instead of a generic line. Note the--ingressdoc says it "Requires--network own-ip" but that is only enforced server-side viavalidate_policy; relying on the surfaced daemon error is acceptable, though a local pre-check would give faster feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal2/src/main.rs` around lines 489 - 498, Add a client-side pre-check in main’s session creation flow so `--ingress` is rejected earlier when the selected network mode is not `own-ip`, rather than waiting for the daemon response. Keep the existing `minimald_rpc::Errorable::Err { error }` handling in the session creation match so `CreateSession failed: {error}` still surfaces the daemon’s typed `validate_policy` message for other validation failures.crates/minimald/src/session_host.rs (1)
683-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTransport-aware attach and teardown are consistent. The
match transportfor the relay attach (Lines 723-728) and theControlChannelconstruction (Lines 742-747) line up withSwitchTransport/ControlChannel, and the ingress-apply failure path correctly drops the relay before detaching so no half-configured PTask survives.Minor: the
transportis matched twice (tap attach, then control-channel build). If these ever diverge it would be a silent mismatch. Optional consolidation — derive theControlChannelonce up front and reuse it for both — but the current form is readable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/session_host.rs` around lines 683 - 779, The transport handling in attach_own_ip is duplicated, which can silently drift between the relay attach and ControlChannel setup. Consolidate the SwitchTransport-to-ControlChannel mapping once in attach_own_ip, then reuse that single derived value for both attach_to_switch/attach_to_switch_vsock and the control construction so the logic stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimal2/src/autospawn.rs`:
- Around line 38-43: The spawn timeout parsing in spawn_timeout_secs currently
accepts 0 as a valid override, which can propagate a zero timeout into the
autospawn flow. Update the parsing logic in spawn_timeout_secs so that
MINIMAL_SPAWN_TIMEOUT_SECS values of 0 (and any non-positive input) are rejected
and the code falls back to DEFAULT_SPAWN_TIMEOUT_SECS or a minimum allowed
value, keeping the existing trim/parse behavior for valid positive integers.
In `@crates/minimald/src/net/switch.rs`:
- Around line 314-321: Wrap the vsock handshake in `switch.rs` so
`tokio_vsock::VsockStream::connect` and the initial `AsyncWriteExt::write_all`
of `CONNECT_REQUEST` are both covered by a `tokio::time::timeout`. Update the
relay setup path around `spawn_relay` to return a best-effort error on timeout
instead of waiting indefinitely, and keep the async trait disambiguation for
`VsockStream` unchanged. Use the existing connect-and-split flow in this
function as the place to apply the deadline.
In `@crates/minimald/src/session.rs`:
- Around line 312-317: The `context()` path currently uses
`tokio::task::block_in_place`, which can panic on current-thread runtimes such
as default `#[tokio::test]` callers. Replace the blocking section in
`session::context()` around `mctx::scaffold_default_mfile` with `spawn_blocking`
or otherwise move the scaffolding outside the async task, and keep the existing
`map_err`/`String` error handling intact so empty-workspace cases return an
error instead of panicking.
In `@crates/minvmd/src/cmd/mod.rs`:
- Around line 60-66: The ready_timeout() helper currently accepts a parsed value
of 0, which can make boot time out immediately; update the parsing in
ready_timeout() to reject zero and fall back to DEFAULT_READY_TIMEOUT_SECS,
matching the validation style used by vm_ram_mib(). Keep the fix localized to
the READY_TIMEOUT_ENV handling so that boot.rs continues to receive a non-zero
Duration from ready_timeout().
In `@crates/minvmd/src/net.rs`:
- Around line 922-950: The readiness signal in spawn is sent too early after
obtaining the child PID, so gvproxy can be marked ready before the switch socket
is actually listening. Update the spawn flow in GvproxyConfig::spawn to wait for
switch_sock to become connectable, or otherwise race that wait against
exit.recv(), before sending Ok(pid) through ready_tx so run only proceeds once
the switch is usable.
- Around line 1405-1440: The HostGvproxy tests are using a real binary
placeholder that does not accept the argv passed by HostGvproxy::spawn, so the
process may exit immediately instead of behaving like a long-lived supervised
gvproxy. Replace the direct use of sleep in
host_gvproxy_spawns_supervises_and_stops and host_gvproxy_drop_stops_the_switch
with a fake gvproxy helper that accepts and ignores the gvproxy arguments while
staying alive long enough for supervision and stop/drop behavior to be verified.
Keep the assertions focused on HostGvproxy::spawn, pid(), stop(), and drop()
semantics.
In `@crates/minvmd/src/net/shuttle.rs`:
- Around line 72-78: The test in switch_sock_sits_beside_the_bridge_socket
mutates XDG_RUNTIME_DIR via std::env, which is process-global and can race with
parallel tests. Refactor the path logic used by resolve_switch_sock into a pure
helper that derives the socket path from an explicit runtime dir argument, and
have the test assert against that helper directly; if env mutation must remain,
guard the set_var/remove_var pair with the existing test mutex.
In `@justfile`:
- Around line 126-140: The up recipe is missing a pinned MINVMD_VM_RAM_MIB, so
x86_64 Linux hosts can fall back to the unsafe default RAM size and hit the same
MMIO-hole boot failure CI avoids. Update the justfile’s up recipe to set
MINVMD_VM_RAM_MIB for x86_64 Linux, mirroring the existing CI safeguard, and
keep the change scoped alongside the other MINVMD_* environment exports in up.
---
Outside diff comments:
In @.github/workflows/ci-linux-kvm.yml:
- Around line 203-219: The status polling in the CI KVM workflow is too short
and can exit before the VM reaches Running on slower boots. Update the poll loop
in the status check section to wait long enough to match the READY timeout used
by the detach flow, and keep the same jq-based running-state check around the
existing "$BIN" status --json probe.
---
Nitpick comments:
In `@crates/minimal2/src/main.rs`:
- Around line 489-498: Add a client-side pre-check in main’s session creation
flow so `--ingress` is rejected earlier when the selected network mode is not
`own-ip`, rather than waiting for the daemon response. Keep the existing
`minimald_rpc::Errorable::Err { error }` handling in the session creation match
so `CreateSession failed: {error}` still surfaces the daemon’s typed
`validate_policy` message for other validation failures.
In `@crates/minimald/src/session_host.rs`:
- Around line 683-779: The transport handling in attach_own_ip is duplicated,
which can silently drift between the relay attach and ControlChannel setup.
Consolidate the SwitchTransport-to-ControlChannel mapping once in attach_own_ip,
then reuse that single derived value for both
attach_to_switch/attach_to_switch_vsock and the control construction so the
logic stays consistent.
🪄 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: 435cea8e-037a-4034-b48e-2148be48282e
📒 Files selected for processing (30)
.github/workflows/ci-linux-kvm.ymlcrates/mctx/src/lib.rscrates/mctx/src/scaffold.rscrates/minimal2/src/autospawn.rscrates/minimal2/src/main.rscrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minimald/src/net/mod.rscrates/minimald/src/net/policy.rscrates/minimald/src/net/proxy.rscrates/minimald/src/net/switch.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/minimald/src/test_harness.rscrates/minimald/tests/netns.rscrates/minvmd/src/cmd/boot.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rscrates/minvmd/src/cmd/status.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/image.rscrates/minvmd/src/net.rscrates/minvmd/src/net/shuttle.rscrates/minvmd/src/vm.rsdocs/specs/03-spec-networking/test-plan.mddocs/specs/03-spec-networking/test-plan.shjustfilescripts/build-initramfs.sh
`run_with_cancel` now attaches the configured `Network` to each invocation's freshly-unshared netns after spawn and tears it down explicitly once the invocation completes (both the cancel and completion arms). No-op for `HostNet`/`NoNet` and when no custom `Network` is set, so existing build/task consumers are unaffected. This is the structural completion of the #581 review (twitchyliquid64): networking is now applied by the shared run path, so any sandbox consumer — tasks and builds, not only minimald sessions — gets it through the abstraction. Add `Error::Network` for attach failures; the borrow is scoped to `&dyn Network` (Send+Sync) so it imposes no `C: Sync` on the run future. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The host gvproxy switch spawns with an empty static-lease table (the guest assigns PTask IPs statically over the vsock shuttle), so the in-process lease/attach machinery was never exercised in production — only by its own tests. Remove `PtaskAttachment`, `attach_ptask`, `allocate_ptask`, `detach_ptask`, `allocate_ip`, and the now-dead `next_index`/`attached_count` fields (and the `subnet` field, read only by `allocate_ip`), plus their tests. The supervisor keeps its real job: spawn, pidfd-supervise, and stop the one host gvproxy process. Refs #581 review (twitchyliquid64). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
force-pushed
the
fix/vm-bringup-timeouts-ram
branch
from
June 26, 2026 18:41
0952088 to
cfb03c2
Compare
…itives The gvproxy switch subnet, MAC derivation, wire constants, and `-config` rendering had drifted into parallel copies in minimald and minvmd (the SwitchSubnet types had even diverged: /8..=/29 vs /1..=/30 validation, Option vs non-Option accessors, separate error types). Collapse them into one definition in `minimald-rpc` (the crate both already depend on), unified on minimald's design (the richer one the netns path uses): SwitchSubnet (/8..=/29, non-Option accessors, daemon_ip), MacAddr, GATEWAY_MAC, DEFAULT_MTU, DEFAULT_SUBNET, the VSOCK constants, and render_gvproxy_config (tuple leases). Both crates re-export them, so `minimald::net::*` / `minvmd::net::*` keep working. minvmd adapts: its render drops the now-unnecessary Option handling and its SwitchSubnet API tests (which asserted the old /1,/30,Option behaviour) are dropped — the shared type is tested in minimald-rpc. No behaviour change for the deployed /16 subnet. Refs #581 review (twitchyliquid64). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rable
`just up` / boot e2e could not bring up the VM for two distinct,
environment-dependent reasons; both are now configurable with safe,
arch-aware defaults.
1. Timeouts too short. A cold multi-GiB VM on the generic guest kernel
takes ~22-28s to reach userspace (longer under load), but the
READY-marker wait (boot/run) was a fixed 5s and the autospawn UDS wait
8s -- both firing before the kernel reached /init.
- minvmd: cmd::ready_timeout() (env MINVMD_READY_TIMEOUT_SECS, default
60s), shared by boot and run; the timeout error names the override.
- minimal2: autospawn UDS wait via MINIMAL_SPAWN_TIMEOUT_SECS (default
75s). All three helpers reject 0 -- a zero timeout would give the
boot no readiness window / make recv_timeout fail instantly.
2. x86_64 boot panic. A 4096 MiB guest boots on aarch64 but on x86_64
exactly 4 GiB straddles the 32-bit MMIO/PCI hole (~3-4 GiB),
mis-placing the initramfs so the kernel finds no /init and panics in
prepare_namespace ("VFS: cannot open root device"). Guest RAM is now
cmd::vm_ram_mib() (env MINVMD_VM_RAM_MIB; status reports the resolved
value), and DEFAULT_VM_RAM_MIB is arch-conditional -- 2048 on x86_64
(CI-proven, hole-safe), 4096 on aarch64 -- so every boot path (CI,
`just up`, any host) is safe without per-lane pinning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
force-pushed
the
fix/vm-bringup-timeouts-ram
branch
from
June 26, 2026 18:58
cfb03c2 to
a261d89
Compare
norrietaylor
added a commit
that referenced
this pull request
Jun 26, 2026
- Bound the host-shuttle vsock connect + `/connect` upgrade with a 5s timeout so an unresponsive/absent host gvproxy fails the OwnIp attach fast instead of stalling bring-up. - Guard the scaffold `block_in_place` by runtime flavor: it panics on a current-thread runtime, so fall back to a plain blocking call there. - RenameSession now re-points the HostNet hostname route to the new name (was stranded under the launch name). Refs #581/#585 review (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
added a commit
that referenced
this pull request
Jun 26, 2026
HostGvproxy::spawn now polls gvproxy's `-listen` socket (5s) before sending Ok(pid), so a switch that died during startup is surfaced as an error rather than reported network-ready. Also factor switch-socket derivation into a pure helper so its test no longer mutates process-global XDG_RUNTIME_DIR. Refs #585 review (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
1 of 2 tasks
norrietaylor
added a commit
that referenced
this pull request
Aug 30, 2026
…them
`ignore` did not ignore anything. `resolve_task_env` skipped a matched
name with `continue`, so it was merely *absent* from the map the client
sends. But the daemon applies that map by insertion and never removes,
so the declaration `minimal.toml` made survived untouched:
- an ignored `env_vars.X = { inherit = true }` was still resolved
daemon-side through `std::env::var` (crates/minimald/src/env.rs) —
the exact indirection #585 exists to delete. It either failed the
spawn, or handed the task whatever unrelated value the daemon
happened to have under that name.
- an ignored literal was untouched entirely; `ignore` did nothing to it.
Both contradict what `docs/reference/tasks.md` claims. Absence is not a
signal on this wire, because absence already means "an older client sent
nothing, resolve it yourself". A drop has to be said out loud.
So the client says it. `minimald_rpc::taskenv` gains `TASK_ENV_DROP`,
carrying the matched names as a JSON array, and a `TaskEnv { set, drop }`
pairing the two halves that have to travel together. The daemon removes
before it applies.
`TASK_ENV_DROP` is deliberately not under `TASK_ENV_PREFIX`: a name that
started with it would be stripped by `from_channel_env` and delivered to
the task as a variable called `DROP`. A test pins that.
Skew stays inert in both directions. No drop list means remove nothing,
which is the behaviour of every client that predates this.
The `ignore` test asserted only that the name was absent from the
resolved map — true of the broken version too, which is why the whole
suite passed over it. It now asserts the name is on the drop list.
Refs: gominimal/inbox#585
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
norrietaylor
added a commit
that referenced
this pull request
Aug 31, 2026
Client-side resolution reads the user's own shell. That is the point of #585 — and it is also what makes a task's `env_vars` a way to ask for the user's secrets. Before it, `env_vars.AWS_SECRET_ACCESS_KEY = { inherit = true }` resolved against the daemon's environment and got nothing. After it, the same line hands a project a real credential. The first cut applied only `deny` and `ignore`, and argued `allow` away on ergonomics: routing project-declared names through it hard-fails a scripted `min task run` until each is allowlisted. But that is the mitigation working, not a defect in it — and a project you have not read is exactly what the policy exists to constrain. `[session.vars]` from the same `minimal.toml` has always passed the full gate under `Source::Project`; a task's `env_vars` were the inconsistency, not a deliberate exception to it. Every declared name now goes through `VarsPolicy::check` with project provenance — the composer's own call, not a reimplementation, so the precedence cannot drift. `deny` fails, `ignore` drops, `allow` carries, and anything else is referred to `PolicyHooks`: the interactive prompt on a terminal, `NoPromptHook` everywhere else, the same two lanes an activate already uses in this function. The check runs before the lookup. That ordering is the property, not an implementation detail — a gate that resolved first and asked afterwards would already be holding the value. A test drives it with an env closure that panics if called. Names are sorted, so which variable a user is asked about first does not depend on `HashMap` iteration order. An allowlisted name is allowed for every project, not only the one at hand. A per-project trust concept — like the path allowlist that gates project lifecycle hooks — would close that, and is left as follow-up work rather than bolted on here. BREAKING CHANGE: `min task run` on a task whose `env_vars` name anything outside `[vars] allow` now prompts on a terminal, and fails without one. Scripted and CI callers must allowlist the names the task declares; the error prints the exact `[vars]` block to add. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
norrietaylor
added a commit
that referenced
this pull request
Sep 1, 2026
…olicy (#1312) * fix(task): resolve task env_vars on the client, not the daemon A task declaring `env_vars.X = { inherit = true }` failed at spawn with `inheriting environment variable 'X': environment variable not found`, even with X exported in the invoking shell. Both resolution sites read `std::env::var` — crates/minimald/src/env.rs and crates/mctx/src/env.rs. Reached from the task path, that is the daemon's own environment, and on macOS the daemon is not even on the same side of the VM boundary as the shell that typed the command. The user's export could never be visible to it. `[session.vars]` used the identical syntax and worked, because activation resolves it client-side before the value is shipped; the two paths diverged in one project, in one run. Resolve a task's env_vars where the values exist. `min task run` reads the task declaration it already loads for the declared-task check, resolves each entry against its own environment, and carries the results to the daemon as channel env requests under `MINIMAL_TASK_ENV_` — the same mechanism that already carries MINIMAL_SESSION_ID and TRACEPARENT. The exec dispatch strips the prefix and applies the values over the task's own declarations, so no `inherit` reaches the daemon-side resolver. The prefix carries the name and the SSH request carries the value, both verbatim, so nothing is escaped and any string a shell can export round-trips exactly. A variable declared `inherit` but unset now fails on the client, naming the task and the variable, instead of surfacing as a spawn failure after a session has already been built and uploaded. Version skew is inert in both directions: an older daemon accepts the env requests and ignores them, a newer daemon sees an empty set from an older client. Either way it falls back to resolving `inherit` itself, which is the pre-change behaviour. That same fallback still serves in-box `min run <task>`, which has no invoking client to read from — documented as the remaining gap rather than silently left to surprise. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(minimal): verify a task's env_vars against the user policy Client-side resolution reads a task's `env_vars` out of the invoking shell and ships the values into the box. Nothing consulted the user's `[vars]` policy on the way, so a `deny` rule — the one control that exists to say "this name never leaves my shell" — was silent on the path that actually reads the shell. Every declared name is now checked before its value is read, in the precedence `VarsPolicy::check` uses: `deny` first, then `ignore`. - `deny` fails the run, naming the task, the variable, and the file the rule lives in. Checked before the lookup, so the value is never read; checked before `ignore`, so a name matching both resolves as denied rather than being quietly dropped. - `ignore` drops the variable and lets the task run. Also checked before the lookup, so an ignored `inherit` need not be set at all. `allow` is deliberately not applied. A task's `env_vars` are declared in the project's own `minimal.toml` and named on the command line, and this path has no prompt: routing them through `allow` would hard-fail every scripted `min task run` until each name were allowlisted, which is the case client-side resolution exists to unblock. `deny` and `ignore` carry no prompt leg, so they apply cleanly. The policy is read before `ensure_daemon`, alongside the declared-task and unset-inherit checks, so a denied variable fails with no session built. It replaces the later read that fed the loadout gating rather than adding a second one. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SumDkFxhZfJZcHbQXH1dFN * docs(tasks): qualify the unset-inherit error against the ignore rule The unset-inherit paragraph states the error absolutely, and the `[vars]` policy section fifteen lines below introduces an exception to it: an ignored name is dropped rather than looked up, so it need not be set. A reader hits the absolute claim first and has no reason to keep going. Forward-references the policy section rather than restating it. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): name ignored task vars on the wire so the daemon drops them `ignore` did not ignore anything. `resolve_task_env` skipped a matched name with `continue`, so it was merely *absent* from the map the client sends. But the daemon applies that map by insertion and never removes, so the declaration `minimal.toml` made survived untouched: - an ignored `env_vars.X = { inherit = true }` was still resolved daemon-side through `std::env::var` (crates/minimald/src/env.rs) — the exact indirection #585 exists to delete. It either failed the spawn, or handed the task whatever unrelated value the daemon happened to have under that name. - an ignored literal was untouched entirely; `ignore` did nothing to it. Both contradict what `docs/reference/tasks.md` claims. Absence is not a signal on this wire, because absence already means "an older client sent nothing, resolve it yourself". A drop has to be said out loud. So the client says it. `minimald_rpc::taskenv` gains `TASK_ENV_DROP`, carrying the matched names as a JSON array, and a `TaskEnv { set, drop }` pairing the two halves that have to travel together. The daemon removes before it applies. `TASK_ENV_DROP` is deliberately not under `TASK_ENV_PREFIX`: a name that started with it would be stripped by `from_channel_env` and delivered to the task as a variable called `DROP`. A test pins that. Skew stays inert in both directions. No drop list means remove nothing, which is the behaviour of every client that predates this. The `ignore` test asserted only that the name was absent from the resolved map — true of the broken version too, which is why the whole suite passed over it. It now asserts the name is on the drop list. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): name `min session run` among the daemon-resolved forms The paragraph scoped the fix to `min task run` and named one exception, the in-box `min run <task>`. `min session run <session> <task>` is a second host-invoked way to run a declared task, and it resolves `inherit` daemon-side too: it goes over the attach transport, whose `SendEnv` allowlist carries `MINIMAL_SESSION_ID`, `LANG`, `LC_*` and `TZ` — never the resolved task env. A reader had no way to tell the two apart, and would reasonably assume the fix covered both. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): resolve no env_vars for an echo task An echo task's `env_vars` are read by nobody. The daemon answers one straight from the declaration and returns (crates/minimald/src/exec.rs, the `as_echo` fast path) before it reaches the only place `task.vars` is consumed. So a task declaring both `action = "echo ..."` and `env_vars.X = { inherit = true }` ran fine with `X` unset — the value was never looked at. Client-side resolution broke that: it hard-failed the run, telling the user to export a variable nothing reads. The policy check inherited the same false positive, denying a run over a name the task never sees. `resolve_task_env` now returns an empty set for an echo task before it looks at anything. Placed there rather than at the call site so the rule sits with the resolution it qualifies, and can be tested directly. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(minimal): declare the echo fixture with the key mfile parses `action = 'echo hi'` is not how an echo task is written. `TaskAction` is `#[serde(flatten)]` and its echo variant is the bare `echo` key, so the fixture's `[tasks.t]` never materialised and the test panicked in `task_from_toml` without reaching the guard it exists to cover. Asserts the fixture parses as an echo task before anything else. Without that, a fixture that quietly stopped being one would leave every assertion below passing for the wrong reason — the same shape of hole that let the no-op `ignore` through a full green CI run. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(minimal)!: gate a task's env_vars on the full user policy Client-side resolution reads the user's own shell. That is the point of #585 — and it is also what makes a task's `env_vars` a way to ask for the user's secrets. Before it, `env_vars.AWS_SECRET_ACCESS_KEY = { inherit = true }` resolved against the daemon's environment and got nothing. After it, the same line hands a project a real credential. The first cut applied only `deny` and `ignore`, and argued `allow` away on ergonomics: routing project-declared names through it hard-fails a scripted `min task run` until each is allowlisted. But that is the mitigation working, not a defect in it — and a project you have not read is exactly what the policy exists to constrain. `[session.vars]` from the same `minimal.toml` has always passed the full gate under `Source::Project`; a task's `env_vars` were the inconsistency, not a deliberate exception to it. Every declared name now goes through `VarsPolicy::check` with project provenance — the composer's own call, not a reimplementation, so the precedence cannot drift. `deny` fails, `ignore` drops, `allow` carries, and anything else is referred to `PolicyHooks`: the interactive prompt on a terminal, `NoPromptHook` everywhere else, the same two lanes an activate already uses in this function. The check runs before the lookup. That ordering is the property, not an implementation detail — a gate that resolved first and asked afterwards would already be holding the value. A test drives it with an env closure that panics if called. Names are sorted, so which variable a user is asked about first does not depend on `HashMap` iteration order. An allowlisted name is allowed for every project, not only the one at hand. A per-project trust concept — like the path allowlist that gates project lifecycle hooks — would close that, and is left as follow-up work rather than bolted on here. BREAKING CHANGE: `min task run` on a task whose `env_vars` name anything outside `[vars] allow` now prompts on a terminal, and fails without one. Scripted and CI callers must allowlist the names the task declares; the error prints the exact `[vars]` block to add. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): name the echo exception in the policy section The section says every name a task declares is checked, and an `echo` task's are not: it answers from the declaration without building an environment, so nothing is looked up and there is nothing to gate. States it as the exception it is, and why it is not a hole — a reader who tries `echo` with `env_vars` and sees no prompt should find that documented rather than surprising. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(minimal): refuse, don't fake-approve, on the non-interactive gate `NoPromptHook` answers every unapproved item with `AllowOnce`. That is deliberate and safe where it came from: `cmd_activate` only submits a verdict, and checks the summary before it does, so a fake approval never becomes a real one. Its own comment says exactly that. This caller reads values. An `AllowOnce` from that hook flowed into the resolver's last pass and called `env(name)` on a project-declared name the policy had refused — so on a fresh install in CI, a `minimal.toml` naming `env_vars.AWS_SECRET_ACCESS_KEY = { inherit = true }` had it read out of the user's shell before anything looked at the summary and bailed. Nothing reached the daemon; the `TaskEnv` is dropped on that path. But check-before-read is the property the gate exists for, and it was broken in the unattended lane the gate matters most in. `RefuseAndRecord` records the names and returns `Abort`, so the resolver stops before the read. The caller builds the same `[vars] allow` snippet from those names through `toml_edit`, so the error is unchanged. The existing test could not catch this: it drives `resolve_task_env` with a hand-rolled double, never the hook the call site uses, and the double refused where the real one approved. The new test drives `RefuseAndRecord` itself with an env closure that panics if called — verified by reverting the hook to the fake-approving shape and watching it fail in that closure. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(sessions): extract the var gate so tasks don't copy it The task path grew its own classify/refer/apply loop because every reusable piece of the composer's was `pub(crate)`. That is not a reason to have two, and review caught it. `gate_names` is the gate with values factored out: `(name, source)` in, one `NameVerdict` each out, the same three passes. `gate_vars` is now that plus the value plumbing, and `crates/minimal/src/task.rs` calls it directly, dropping its duplicate loop and its `TaskVar` provenance shim. Factoring on names rather than exposing `gate_vars` is what lets the two share at all. `gate_vars` takes `ResolvedVar`, which already holds the value — the session path resolves and then gates. The task path must gate first, because it resolves against the invoking user's shell and reading an unapproved name is the leak the gate exists to stop. A gate that never sees a value serves both. Two behaviours are preserved deliberately in `gate_vars`: literals still skip the policy entirely (`carries_user_data`), and the emitted list keeps input order, so an ungated literal does not jump ahead of a gated var declared before it. Checked by mutation, not just by a green run: making `gate_names` treat an undecidable name as allowed fails 4 tests in `sessions` and 4 in `minimal`, so the branch is pinned from both sides. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): drop gate_names' panic and document its errors `gate_names` broke clippy in two lanes. `missing_panics_doc` and `missing_errors_doc` do not fire on a `pub(crate)` function, so making the gate public exposed both; every other `pub` fn in the crate already carries the sections. Rather than document a panic that should not exist, the panic is gone. Pass 1 seeds an undecided item as `Denied` and queues it, and pass 3 overwrites every one — so the `Option` and its `expect` are unnecessary. The placeholder is fail-closed by construction: an edit that ever left an entry unwritten would make the caller refuse the name rather than carry it, which is the right direction for this gate and better than the `expect` it replaces. Adds the `# Errors` section naming the three real paths: no hook for an item needing one, hook abort, hook contract violation. I ran clippy over the crates I set out to change and not over `sessions`, which is the crate this commit's parent exists to change. `just clippy` scopes to the workspace for a reason. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tasks): use the tree's anchor form, and say why sort_unstable Two review points, both about something reading as other than it is. The policy section anchored itself with a raw `<a id="policy"></a>`. Every other anchor in `docs/` — 28 of them — uses the `{#slug}` form on the heading, and this was the only raw HTML one in the tree. The `[below](#policy)` link added alongside it depends on that anchor resolving, and the published docs render from a different repo whose renderer need not treat the two alike. The ordering comment sat directly above `sort_unstable` while talking about a fixed order, which reads as a contradiction. It is not one: `sort_unstable` describes what becomes of *equal* elements, and these are map keys, so there are none. Says so, rather than leaving the next reader to re-derive it. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sessions): stop the batch at the first denial, before any hook runs Extracting `gate_names` lost the short-circuit. The old pass 1 called `apply_decision(d, ..)?`, which returned `Err` from inside the loop on the first `Denied`; the extracted version recorded a verdict and carried on, so a batch holding a denied name and an unapproved one consulted the hook before the denial ever surfaced. Caught in review. Both call sites were affected, and the session one is a regression in behaviour that predates this branch: - `min task run` non-interactively, with `A` denied and `B` unlisted: the hook is asked about `B` alone, and the operator is told to allow `B` — never `A`. They add `B`, re-run, and meet `A`'s denial for the first time. - Session composition, interactively: the user is prompted about `B`, and an "always allow" answer *writes a rule to user_policy.toml*, before the composition fails over `A`. A persisted policy change for a run that could never have succeeded. `gate_names` now returns `ComposeError::Denied` from the first denial. `NameVerdict` loses that variant — a denial ends the batch, so it is no longer something a caller can be handed — and `task.rs` matches the error to keep naming the task, the variable, and the policy file. The placeholder for a queued item is `Ignored`, still the fail-closed direction. `sessions`' three existing deny tests are all single-item, so nothing covered a mixed batch. Adds one to each crate: the `sessions` one drives a hook that panics if consulted, so the ordering is enforced rather than described; the `minimal` one checks the operator is told about the denial and that the hook recorded nothing. Verified by mutation. Restoring the non-short-circuiting pass 1 fails 4 tests in `sessions` and 3 in `minimal`. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(mfile)!: order a task's env_vars, and drop the sort for it Review asked why the ordering comment sat above `sort_unstable`, and whether the sort should be stable instead. Neither: sort stability governs what happens to *equal* elements, and these are map keys, so `sort()` and `sort_unstable()` produce the same bytes. The real question was why the carrier was unordered at all. #1319 (gominimal/inbox#552) already answered that for the collections carrying package-derived sandbox setup — `HashMap` → `BTreeMap`, and the compensating `entries.sort_by_key(...)` deleted. A task's `env_vars` was not in that scope. This finishes the row. `mfile::Task::vars` becomes a `BTreeMap`, and the carriers it flows through follow so the guarantee is not re-lost at a boundary: `mctx::MakeEnvArgs::env_vars`, `minimald::env`'s `env_vars` field and `with_env_vars`, and the `decode::stacks` constructor that builds one. `resolve_task_env` then takes its order off the map and its sort is gone. Ordering was already deterministic for the prompt and the denial — that was the sort's doing. What it did not cover, and this does: the daemon `?`s on the first unresolvable `inherit` (`crates/minimald/src/env.rs`), so a task with two unset inherited vars named a different one in its error from run to run. The resulting environment was never affected — each key is inserted once into a map — so this is about diagnostics, not about what the sandbox sees. Records the rule in docs/rust-coding-standards.md, framed by the trigger rather than as a blanket ban: `HashMap` stays right for lookup that is never iterated. It also names the trap that makes "just sort it" look sufficient — a later stable sort preserves hash order faithfully, which is how `min init` picks between two equal-priority stacks today (gominimal/inbox#604). Follow-ups filed for the carriers still unordered: gominimal/inbox#603 (OCI image output vars), gominimal/inbox#604 (stack tables), and gominimal/inbox#605 (the task table and the sorts compensating for it). BREAKING CHANGE: `mfile::Task::vars`, `mctx::MakeEnvArgs::env_vars` and `minimald::env::EnvArgs::with_env_vars` take `BTreeMap` where they took `HashMap`. Refs: gominimal/inbox#585 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Nov 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #581 (base:
feat/networking-host-exposure) to exercise CI — primarily the x86_64 Linux/KVM lane — on two VM-bring-up fixes.What
/init. Now env-configurable with generous defaults:MINVMD_READY_TIMEOUT_SECS(default 60s) — shared byboot/run.MINIMAL_SPAWN_TIMEOUT_SECS(default 75s) — autospawn UDS wait (an upper bound; only costs time on a slow/failed boot)./initand panics inprepare_namespace(VFS: cannot open root device) before any minimald code. Guest RAM is nowcmd::vm_ram_mib()(envMINVMD_VM_RAM_MIB, default 4096 — aarch64 cache headroom unchanged);ci-linux-kvmpins the x86_64 lane to a hole-safe 2048.statusnow reports the resolved value.Why a stacked test PR
The RAM fix was validated on aarch64 only — this dev host can't run x86_64 libkrun guests. The x86_64 KVM CI lane (
minvmd-linux-kvm-e2e→boot_e2e) is the only thing that can confirm theVFS cannot mount rootpanic is gone. This PR exists to run that lane on the fix.If the x86_64 lane goes green, fold
6a13cfa5into #581.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes