feat: add microVM execution environments - #580
Conversation
39f6a33 to
0a8f9b2
Compare
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Panel review — the consequential subset
Adversarial review of this PR across three orthogonal axes — Spec (does it implement what was asked), Standards (does it follow this repo's documented conventions), and Domain (what specialist reviewers say) — run as three panels over a 202-file diff, with 19 agents total.
Note on scope of this post: the full review produced 84 inline comments. GitHub's secondary rate limiter refuses a review that creates that much content at once, so this posts the 23 consequential ones: every ship-blocker and every hard standards violation. Four further findings are folded into the comment they pair with. The remaining ~57 — mechanical fixes, judgement calls, polish, and the reuse findings (
net: -1,100lines in the module,-162/-194in host wiring) — are summarised below and available in full on request.
Findings are not merged across axes: Spec, Standards and Domain are orthogonal by design, so each comment is tagged with the panel and axis it came from. cross-confirmed marks a finding two or more independent reviewers reached separately — highest confidence. Where two reviewers disagreed, both views are preserved rather than resolved.
The seven I would fix before merge
internal/app/build.go:2254—prepareSessionDiscoveryre-rootscfg.Workspacebefore the project-trust gate is evaluated, so a session-selected root inherits a trust decision made for a different repo. Because the re-root happens first, evenprojectIngestionAdmittedForRoot— the guard written specifically to prevent this — returns true for an arbitrary root. This fires on anyCreateSessionwith a non-launch-root workspace, so it is a live regression on the existing served path, not a microVM-only issue.- Three daemon concurrency defects, none with a test —
environment/microvm/runtime.go:350(unlocked struct fields behind a locked map lookup),registry.go:94(a flock giving no intra-process exclusion on the durable registry),control/multiplex.go:255(an uncancellable write that can pin a Bash tool call forever). .github/workflows/release.yml:211— the release is created non-draft before any artifact exists, on afail-fast: falsematrix where two of three cells are guaranteed to fail (cross-archdocker runwith no QEMU,package-microvm-release.sh:81). The firstv*tag after this merges produces a public, partial, half-signed release — the exact outcome this workflow's own comments say it is designed to prevent.internal/adapter/server/service.go:1897—DeleteSessionremoves the session record without destroying the VM, and the verb that could destroy it has no wire surface, so it becomes permanently unreachable for that id. The docs describe the deletion sequence as working behaviour.internal/adapter/server/environment_profile.go:59— the ADR-0224 §5 egress disclosure crosses an independently-versioned module boundary as an English sentence, re-derived by prefix/suffix parsing, with a mismatch failing session creation. It is already inconsistent in-tree (client_profile_test.go:36,61asserts strings this Service rejects), and the parse verifies grammar rather than policy..github/workflows/microvm-e2e.yml:38—id-token: writeon apull_requestjob that runs PR-authored code and never signs keylessly..github/scripts/install-microvm-release.sh:45— the installer verifies no signature; the digest it checks comes from the same unsigned manifest, which is itself absent fromSHA256SUMS. Two docs claim it verifies.
Cheapest high-value fixes
Four comments below carry one-click suggestion blocks: CombinedOutput() → Output() in gitexec.run() (a git warning currently corrupts a captured tar), both ADR 0108 → 0224 corrections, and the ADR status flip to Accepted. Also cheap: delete three id-token: write lines, and the two-line VersionValid guard at client.go:448.
Not posted inline — the remaining ~57
Spec (Panel 3): docs/usage/http-sse-api.md:125 says the microVM backend is unwired when build.go:1574-1624 wires it · docs/usage/microvm-environments.md:207 documents a deletion sequence with no operator surface · AC8.1's three-platform live matrix is one platform in practice (microvm-e2e.yml:123 — the arm64 and macOS live cells are workflow_dispatch-gated and default to false) · the release builds and signs a mecatl-owned execution image the ADR does not authorise · go-microvm version skew (ADR says v0.0.39, go.mod says v0.0.40, and internal/apicheck/microvm_module_contract_test.go:19 pins the string from a root-module test that the next dependabot bump will break).
Standards: no depguard rule for environment/microvm in .golangci.yml, so the allowlist ADR 0093 says "travels with the modules" does not exist for this one · two missing ADR-0027 List-1 resource rows (Service.sessionEnvironmentInfo, microVMClients.byEndpoint — the latter never closed) · Config.EnvironmentSessionResolver is undocumented and contradicts ADR 0224 §1 · prepareSessionDiscovery is an unsanctioned per-session catalog delta · AGENTS.md not updated for the new environment/ module tier.
Domain, mechanical: dead + lossy LifecycleWorkspace/LifecycleExec proxy arms (~117 lines, and the copy collapses three distinct error codes into "internal") · proxyWorkspace replace mutates before validating · QuotaKind is a parallel vocabulary for admission.Resource with 9 of 11 values never emitted · Stat returns success on a malformed response · no deadline on teardown paths (CloseSession uses bare context.Background()) · Build mutates the caller's maps · unbounded daemon error text reaching API clients · five hand-written cleanup marks · reply-frame echo copied at 7 sites · configgen documents five config keys the strict decoder deliberately rejects.
Domain, judgement calls: egress allowlist is hostname-only with no post-resolution IP denylist (SSRF; flagged because it is the claimed control failing open, not the documented deferral) · two live IdentityAllocators, one hardcoding Generation: 1 · the neutral server layer now speaks microVM vocabulary, and those names have reached the proto · the host-side wire Workspace is the one ADR-0208 implementation with no conformance coverage · Taskfile.yml:232 selects live coverage by a hardcoded 19-name regex, and go test -run '<no match>' exits 0 · the installer's archive preflight has no negative test on any of its ten controls · a third frame-codec copy that re-types the wire shapes as map[string]any, hiding renames.
Checked and deliberately not flagged
Recorded so it is not re-litigated: the environment/ module tier is justified (authn/oidc is the precedent) · the "three-profile fan-out" does not exist — tool profile and placement alias are orthogonal axes pinned by a test — and should not be unified · the mirrored wire structs in internal/adapter/microvm are essential module-graph isolation · guest errorResponse vs host remoteError are deliberate inverses across a process boundary · the +37-line append to frozen ADR-0027 is legitimate (AGENTS.md names that file explicitly) · engine/adapter/fsconformance's additions are the right shape and placement · the 1,739-line e2e suite is real fault injection, not mock theatre · forker.KindRouter is a real abstraction introduced when its second implementation arrived · ~8,000 lines of per-scenario test setup should stay explicit · no host credential crosses into the guest, and the codec framing, artifact supply chain, peer-credential fail-closed behaviour, and operator-tier config gating were all verified sound.
Each axis is orthogonal — verify each independently before shipping.
|
|
||
| go 1.26.6 | ||
|
|
||
| replace github.com/stacklok/mecatl/engine => ../../engine |
There was a problem hiding this comment.
[Panel 1 · Standards · HARD VIOLATION] Committed replace directive in a published module.
ADR 0093 (Decision): "replace directives live in go.work (dev) and the root go.mod (the monorepo consumer), NEVER in a published provider go.mod — a committed replace breaks downstream go get."
Every sibling opt-in module obeys this: authn/oidc/go.mod and all four provider/*/go.mod require engine at a real version with no replace — the Taskfile even notes "Its go.mod deliberately has no replace directive". This PR already adds use ./environment/microvm to go.work, so the replace is redundant in dev and harmful on publish.
Related (not inline-commentable — .golangci.yml is unchanged): there is no depguard rule for environment/microvm. grep 'environment/microvm' .golangci.yml returns zero hits, while the config carries file-scoped strict rules for **/authn/oidc/**.go and each **/provider/*/**.go. ADR 0093 claims "Each provider shares the root .golangci.yml depguard allowlist … so the layering rule travels with the modules" — with no matching rule, this module has no allowlist at all.
| if err != nil { | ||
| return err | ||
| } | ||
| if generation.services != nil { |
There was a problem hiding this comment.
[Panel 1 · Domain · HIGH] runtimeGeneration.services is raced.
runtime.go:310-312 writes it under r.mu; Services() (:322), Reattach (:334) and Destroy (:366) read it — and this Detach write clears it — without the lock. composition.go:130 spawns a goroutine per connection, so a LifecycleWorkspace/LifecycleExec proxy can read services while a concurrent LifecycleDetach nils it: nil deref, or use of a closed control.Client. generation() (:386-397) locks only the map lookup; every field access after it is unguarded.
Fix: lock the struct, or make runtimeGeneration immutable after negotiate.
Source: go-architect
| return result, err | ||
| } | ||
|
|
||
| func (r *FileRegistry) withLock(ctx context.Context, fn func(*registryDocument) error) error { |
There was a problem hiding this comment.
[Panel 1 · Domain · HIGH] FileRegistry's flock gives no intra-process exclusion — lost updates on the durable registry.
All callers share one *flock.Flock. gofrs/flock@v0.13.0/flock_unix.go:141-147 returns (true, nil) immediately when that instance is already locked, so two goroutines in the same daemon both "acquire", read the same document, and the first defer Unlock() (:108) drops the file lock while the second is still mid-transaction. The type's own doc comment (:17-19) claiming one authoritative set is also wrong for that window. validRecordTransition catches state regressions but not two concurrent create appends.
One sync.Mutex on FileRegistry fixes it.
No test covers this: the concurrent case (lifecycle_reconcile_test.go:185-189) uses an in-memory registry, and the two-FileRegistry case (:197-225) is sequential.
Source: go-architect
| } | ||
| } | ||
|
|
||
| func (c *Client) write(frame multiplexFrame) error { |
There was a problem hiding this comment.
[Panel 1 · Domain · HIGH] The multiplex client cannot be cancelled if the guest stops reading — this can pin a Bash tool call permanently.
This write has no deadline and no ctx, and Stream holds requestMu across it (:178-194, needed only to satisfy the server's monotonic-ID check at :405). If the guest stops draining, every caller — including the cancel write at :208 — blocks on writeMu indefinitely and ctx is inert.
Worse, :203-252: after sending cancel it sets ctxDone = nil and then waits for an end/error frame forever, so a hung guest handler pins the caller goroutine — and via guestexec/exec.go:174, a Bash tool call — permanently.
Needs a write deadline plus a bounded post-cancel wait.
Sources: go-architect; secure-code-reviewer (CWE-400, on the read-loop half)
| return microvm.CreateRequest{Owner: request.Owner, SessionID: request.SessionID, Profile: request.Profile, | ||
| Worktree: worktree.Request{Source: request.SourceCheckout, WorktreePath: names.WorktreePath, MetadataPath: names.MetadataPath, Branch: names.Branch}, | ||
| ArtifactRequests: artifactRequests, Resources: usage, | ||
| ProfileStatus: microvm.EnforcedProfileStatus{Profile: request.Profile, GuestEgress: egress.Status(), HostEgress: "not constrained: LLM providers, WebFetch, WebSearch, MCP, hooks, OCI pulls, telemetry"}}, nil |
There was a problem hiding this comment.
[Panel 1+2 · Domain · HIGH · cross-confirmed] The egress disclosure is a prose sentence used as a load-bearing cross-module protocol constant.
hostServiceEgressStatus (internal/adapter/server/environment_profile.go:59) is a prose literal retyped byte-for-byte here, and network.go:157,159 mints "deny-all (IPv6 disabled)" / "allowlist (%d destinations; IPv6 disabled)" which the host prefix/suffix-parses at environment_profile.go:61-72. A mismatch on either string fails session creation with ErrFailedPrecondition (:141).
The fact is structured — policy mode, destination count, IPv6 state — but crosses an independently-versioned module boundary as an English sentence. Two things a module boundary is supposed to permit (improving a human-facing message; host/daemon version skew) become total loss of the microVM capability.
It is already broken in-tree: internal/adapter/microvm/client_profile_test.go:36,61 asserts a successful Provision with HostEgress: "host services not constrained" and GuestEgress: "daemon enforced deny-all (IPv6 disabled)" — both of which the Service rejects. Nothing in CI ties the two modules together (the daemon's own test asserts only HostEgress != ""; the one default-run cross-layer test uses a hand-rolled fake with the literal hardcoded; the real-daemon suite is behind //go:build microvm_e2e).
It also buys no security: validGuestProcessEgressStatus accepts allowlist (1 destinations; IPv6 disabled) regardless of what was actually configured — the host verifies grammar, not policy.
Recommended: one versioned struct {mode, destinations, ipv6_enabled}; validate the invariant (mode=="allowlist" ⇒ destinations>0) and render prose at the presentation layer. The host-scope sentence is a host-side constant — asking the daemon to echo it back so the host can compare it to itself is a round-trip that can only ever fail.
Sources: software-architect + go-architect (both HIGH)
| timeout-minutes: 45 | ||
| permissions: | ||
| contents: read | ||
| id-token: write |
There was a problem hiding this comment.
[Panel 3 · Domain · HIGH · cross-confirmed] id-token: write on a pull_request-triggered job that executes PR-authored code and never signs keylessly.
Same unused grant at :128 and :183. The live suite runs cosign in keyed mode with an ephemeral local key — environment/microvm/e2e/prepare.sh:152 does COSIGN_PASSWORD= cosign generate-key-pair — and sign-microvm-release-evidence.sh takes the --key branch, so no step ever reads ACTIONS_ID_TOKEN_REQUEST_URL. (docs/usage/microvm-environments.md claims "live CI cells continue to exercise keyless OIDC"; they do not — this grant looks like a leftover from that intent.)
Impact: PR-authored code (this job runs ./environment/microvm/e2e/prepare.sh and task e2e:microvm verbatim) can mint a Fulcio-backed OIDC token with subject repo:stacklok/mecatl:pull_request, then (a) cosign sign-blob arbitrary bytes with a certificate attributable to this repository in the public Rekor log, and (b) exchange that token against any cloud/vault trust relationship whose subject condition is repo-scoped rather than workflow-and-ref-scoped — the single most common OIDC federation misconfiguration.
Fork PRs are capped read-only by GitHub, so the reachable case is a same-repo head branch — including one pushed by a coding agent running in CI.
Not Critical because the published artifact chain is not forgeable this way: microvmd admission pins an exact --certificate-identity and --certificate-oidc-issuer (environment/microvm/artifact_sigstore.go:86-87), and the documented operator regexp is anchored to release.yml@refs/tags/v*, so certs from this workflow do not satisfy it. That containment is what keeps this HIGH rather than Critical.
Fix: delete the three id-token: write lines (the permissions: blocks then reduce to the inherited contents: read). If a keyless cell is wanted later, put it in a separate job gated if: github.event_name != 'pull_request'.
Verification: add forbid 'id-token: write' "$e2e" to microvm-ci-release_test.sh — it already has a working forbid helper and this is exactly the drift it should catch. Independently, audit every OIDC trust policy naming this repo for a sub condition narrower than repo:stacklok/mecatl:*.
CWE-269 / CWE-250 / CICD-SEC-4 / CICD-SEC-5.
Sources: secure-code-reviewer (HIGH) + devops-expert (M4)
|
|
||
| while IFS=" " read -r kind payload payload_digest reference digest provenance bundle; do | ||
| archive="$assets/$payload" | ||
| test "sha256:$(sha256_file "$archive")" = "$payload_digest" |
There was a problem hiding this comment.
[Panel 3 · Domain · HIGH · cross-confirmed] The installer verifies no signature — the digest it checks comes from the same unsigned manifest.
This compares the archive against $payload_digest, which was read out of the manifest. The script never invokes cosign verify-blob: it reads the provenance / sigstore_bundle names from the manifest and copies their paths into microvmd-artifacts.json — a projection, not a verification. So the trust chain at install time is self-referential.
Compounding it, SHA256SUMS-$platform (package-microvm-release.sh:102,126) lists the two binaries and the three tarballs but not microvm-release-<platform>.json, so the documented sha256sum --check gives an operator zero integrity on the one file that feeds this installer.
Both .github/workflows/README.md:163-166 and docs/usage/microvm-environments.md claim the installer verifies, and the documented sequence places the install step before the cosign verify-blob snippet.
Fix — verify the statement, then bind it to the manifest, before extractall:
cosign verify-blob --bundle "$assets/$bundle" \
--certificate-identity "$MICROVM_RELEASE_IDENTITY" \
--certificate-oidc-issuer https://token.actions.githubusercontent.com \
"$assets/$provenance"
# then assert the statement's subject digest equals the manifest's tree digestRequire the identity to be supplied (env or --identity) and abort if empty — do not default to a permissive value. Also add the manifest and this script to SHA256SUMS-$platform.
Execution stays fail-closed today because microvmd admission re-verifies the signed in-toto subject against the recomputed tree digest under an exact identity/issuer policy — which is why this is not Critical. But attacker-chosen bytes are still materialized into a privileged host path with no cryptographic check having run.
Verification: flip one byte of a payload tarball and assert the installer exits non-zero; swap a bundle and assert rejection.
CWE-347 / CWE-494 / CICD-SEC-3 / SLSA v1 verification requirements.
Sources: secure-code-reviewer + devops-expert (H2)
Folded in — two more defects in this same script:
:80(HIGH) — the shipped installer cannot run standalone. It resolves the digest tool via"$(dirname -- "$0")/../../environment/microvm", butpackage-microvm-release.sh:41-45copies this script intodist/<platform>/, where that path does not exist. Underset -eit aborts, so it is fail-closed — but the shipped installer is unusable for every operator following the documented flow, and the predictable response to a broken verifying installer is a manualtar -xzfthat skips every control here. The e2e never catches it becauseMECATL_MICROVM_INSTALLER(Taskfile.yml:240) points at the in-repo copy.:78(HIGH) —filter="fully_trusted"is a Python 3.12+ kwarg; stock macOS/usr/bin/python3is 3.9.6 and raisesTypeError, breaking the installer ondarwin-arm64, a platform this release ships. Drop the kwarg —"fully_trusted"is the legacy default, and the preflight at:56-74is the real defence.
| # authn/oidc is the opt-in caller-identity adapter module (ADR 0206), | ||
| # separate from both the dependency-free engine and provider modules. | ||
| - cd authn/oidc && go build ./... | ||
| # environment/microvm is the opt-in local microVM runtime module (ADR 0108). |
There was a problem hiding this comment.
[Panel 3 · Standards · HARD VIOLATION] Wrong ADR number.
ADR 0108 is Read skill assets on demand by logical name; the microVM ADR is 0224. docs/design/README.md treats a citation as a load-bearing claim, and docs/lint does not scan Taskfile.yml, so nothing catches this.
| # environment/microvm is the opt-in local microVM runtime module (ADR 0108). | |
| # environment/microvm is the opt-in local microVM runtime module (ADR 0224). |
A grep of ADR 0108|adr/0108|0108- across .github/, Taskfile.yml, docs/ and user-docs/ confirms this and user-docs/deployment/microvm-environments.md:151 are the only two mis-citations — every other 0108 hit is a legitimate reference to the real ADR.
Source: Standards axis (also independently found by the Spec axis)
|
|
||
| For configuration and recovery details, see the | ||
| [operator guide](https://github.com/stacklok/mecatl/blob/main/docs/usage/microvm-environments.md) | ||
| and [ADR 0108](https://github.com/stacklok/mecatl/blob/main/docs/adr/0108-microvm-execution-environments.md). |
There was a problem hiding this comment.
[Panel 3 · Standards + Spec · HARD VIOLATION] Dead link to an ADR file that does not and will not exist — and both CI gates miss it.
docs/adr/0108-microvm-execution-environments.md is not a file; 0108 is 0108-on-demand-logical-skill-assets.md. The correct ADR is 0224.
Because this is an absolute external GitHub URL, matlatl check . --strict treats it as external and Docusaurus onBrokenLinks: 'throw' does not resolve it either — so neither task docs:check nor task site:build can catch it. That makes AC8.5's proof ("verify: inspection — task docs and task site:build prove the documented surface is linked") vacuous for the one link an operator follows to reach the trust contract.
| and [ADR 0108](https://github.com/stacklok/mecatl/blob/main/docs/adr/0108-microvm-execution-environments.md). | |
| and [ADR 0224](https://github.com/stacklok/mecatl/blob/main/docs/adr/0224-microvm-execution-environments.md). |
The neighbouring docs/usage/microvm-environments.md GitHub URL is correct, for reference.
Sources: Standards axis + Spec axis (independently)
| @@ -0,0 +1,216 @@ | |||
| # ADR 0224 — Local microVM execution environments | |||
|
|
|||
| - Status: Proposed | |||
There was a problem hiding this comment.
[Panel 3 · Standards + Spec · HARD VIOLATION] ADR Status contradicts the shipped state.
This says Proposed (dated 2026-08-14), while docs/acceptance/microvm-execution-environments.md says Status: landed, 2026-08-15, docs/acceptance/README.md says "landed", and the code is merged. Every comparable shipped ADR (0214, 0217, 0218, 0221) is Accepted.
Per AGENTS.md the ADR is the frozen why of shipped behaviour, so shipped work under a Proposed ADR is a documentation-lifecycle contradiction. docs/lint's header gate checks the header exists, not that the value is current, so nothing catches a stale one.
| - Status: Proposed | |
| - Status: Accepted |
The header is otherwise well-formed (Status / Date / Scope / Supersedes / Superseded by, Context / Decision / Consequences / See also).
Sources: Standards axis + Spec axis (independently)
9c310d6 to
e1365d5
Compare
38e2267 to
74253a4
Compare
ab8b47c to
6d65d53
Compare
3a62bff to
df457f1
Compare
Co-Authored-By: mecatl <noreply@stacklok.com>
df457f1 to
4908e4a
Compare
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
2 participants
Summary
Adds
microvm-local, an opt-in execution environment for local Git repositories on Linux amd64 with KVM.environment_profile: microvm-localwhen creating a session; the local backend verifies its runtime and becomes ready for that session.mecated microvm status/mecatui microvm status.microvm_devworkflow. It requires a local descriptor and an explicit acknowledgement; ordinary source builds and published binaries retain their normal behavior.Current support is local, single-operator Git repositories on Linux amd64 with KVM. Linux arm64, macOS live operation, remote/multi-user placement, schedules, non-Git sources, repository-VM deletion, and advanced recovery controls are not yet available.
Verification
task lint,task build,task api:check,task docs,task site:build,task lint:actions,task test:actionstask ac-trace-strict— landed MicroVM plan: 24 ACs, 0 failurestask e2e:microvm— Linux amd64 KVM journeytask microvm:dev:e2e— tagged developer activation and Linux amd64 KVM coveragemecated, created amicrovm-localsession, and used a real OpenRouter model to invoke guest Bash and verify the guest worktree, Wolfi OS, and Git revision.go run ./cmd/mecademoReview
Spec, standards, security, architecture, DevOps, dependency reuse, and duplication reviews were run through multiple repair waves. Final security re-review reported no Critical/High blockers.
Acceptance
Plan:
docs/acceptance/microvm-execution-environments.mdADRs:
docs/adr/0290-microvm-execution-environments.md,docs/adr/0291-microvm-development-release-activation.mdCloses #526
Closes #527
Closes #528
Closes #529
Closes #530
Closes #531
Closes #532
Closes #533
Closes #534
Closes #535