feat: add microVM execution environments

[JAORMX]

Summary

Adds microvm-local, an opt-in execution environment for local Git repositories on Linux amd64 with KVM.

• Configure trusted execution.default_placement: microvm-local; the local backend verifies its runtime and becomes ready for that session.
• Sessions and isolated delegated children receive distinct Git worktrees inside a repository-scoped VM. Providers, MCP, hooks, memory, identity, and credentials remain on the host.
• Schedules support independent placements whose VM and worktree lifetime is schedule-owned, and borrowed placements whose lifetime remains session-owned. The first claim durably retains the worktree; deletion atomically cleans placement metadata and records pending recovery when backend cleanup must be retried.
• Runtime artifacts and release evidence are verified before a VM starts. Guest workloads run as an unprivileged mapped user.
• Guest IPv4 egress is permitted by default and can be tightened to deny-all or an explicit allowlist. External IPv6 is not currently supported.
• Session placement, logical worktrees, and retained dirty worktrees are visible through mecated microvm status / mecatui microvm status.
• Repository developers can exercise the source tree through a separate, explicitly unsupported microvm_dev workflow. 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, 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:actions
• task ac-trace-strict — landed MicroVM plan: 24 ACs, 0 failures
• task e2e:microvm — Linux amd64 KVM journey
• task microvm:dev:e2e — tagged developer activation and Linux amd64 KVM coverage
• Direct developer journey, twice from fresh isolated state: prepared the local descriptor/bundle, built tagged mecated, created a microvm-local session, and used a real OpenRouter model to invoke guest Bash and verify the guest worktree, Wolfi OS, and Git revision.
• go run ./cmd/mecademo

Review

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.md
ADR: docs/adr/0342-microvm-execution-environments.md

Closes #526
Closes #527
Closes #528
Closes #529
Closes #530
Closes #531
Closes #532
Closes #533
Closes #534
Closes #535

[jhrozek]

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,100 lines in the module, -162/-194 in 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

1. internal/app/build.go:2254 — prepareSessionDiscovery re-roots cfg.Workspace before 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, even projectIngestionAdmittedForRoot — the guard written specifically to prevent this — returns true for an arbitrary root. This fires on any CreateSession with a non-launch-root workspace, so it is a live regression on the existing served path, not a microVM-only issue.
2. 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).
3. .github/workflows/release.yml:211 — the release is created non-draft before any artifact exists, on a fail-fast: false matrix where two of three cells are guaranteed to fail (cross-arch docker run with no QEMU, package-microvm-release.sh:81). The first v* 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.
4. internal/adapter/server/service.go:1897 — DeleteSession removes 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.
5. 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,61 asserts strings this Service rejects), and the parse verifies grammar rather than policy.
6. .github/workflows/microvm-e2e.yml:38 — id-token: write on a pull_request job that runs PR-authored code and never signs keylessly.
7. .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 from SHA256SUMS. 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.

[jhrozek]

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

[jhrozek]

[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

[jhrozek]

[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

[jhrozek]

[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)

[jhrozek]

[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)

[jhrozek]

[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)

[jhrozek]

[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 digest

Require 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", but package-microvm-release.sh:41-45 copies this script into dist/<platform>/, where that path does not exist. Under set -e it 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 manual tar -xzf that skips every control here. The e2e never catches it because MECATL_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/python3 is 3.9.6 and raises TypeError, breaking the installer on darwin-arm64, a platform this release ships. Drop the kwarg — "fully_trusted" is the legacy default, and the preflight at :56-74 is the real defence.

[jhrozek]

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

Suggested change

	# 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)

[jhrozek]

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

Suggested change

	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)

[jhrozek]

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

Suggested change

	- 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)

[jhrozek]

Automated batch review of the microvm-execution-environments feature, split into subsystem slices (artifact/release verification, network egress, repository-scoped VM lifecycle) because the full PR (343 files, +44k) doesn't fit in one review pass. ~40% of the diff has been covered so far; the remainder (microvmmanager/microvm host adapters, guest control/agent/exec, workspace/worktree/virtiofs, CLI/app wiring, docs/CI) has not yet been reviewed.

This review submits every CRITICAL/HIGH finding plus a curated subset of MEDIUM findings that are either directly reachable (not just latent), contradict a claim made in the PR description/ADR, or are a root cause that resolves several other findings at once. Lower-priority findings (pure test-coverage gaps, architecture/encapsulation nits, LOW/INFO items) are being tracked separately and are not included here.

Headline: two independent findings in the artifact trust chain (tree-hash framing collision, wrong-digest Sigstore check) mean the supply-chain verification this feature is built around likely doesn't behave as intended for real signed artifacts. The repository-deletion crash-recovery gap is a second, independent blocker.

[jhrozek]

[CRITICAL] digestTree's custom tree-hash has no framing between a file's content and the next entry — different trees can produce the same digest

Each entry writes NUL-terminated writeHashField(h, rel) + writeHashField(h, mode) (lines 774-775), and for a symlink a NUL-terminated target (line 784) — but for a regular file, io.Copy(h, file) (this line) writes the content with no trailing delimiter before the next entry's fields begin.

Example: a legitimate two-file tree x="AAA", y="BBB" (mode M) serializes to x\0 M\0 AAAy\0 M\0 BBB. A single file x with content literally AAAy\0M\0BBB produces the byte-identical stream on a one-file tree — no hash collision needed, just control over file naming/content, which is exactly what an untrusted resolver/registry response provides.

This digest is what admit() (line 452) and loadVerifiedEntry() (line 492) check materialized payload bytes against, and per ArtifactTreeDigest's own doc comment (line 743-744) is the canonical identity attestation subjects bind to. This framing bug defeats the content-integrity guarantee the whole verification chain depends on.

Recommendation: length-prefix every variable-length field (path, mode, symlink target, file content) instead of relying on NUL termination for some and none for others — e.g. an 8-byte big-endian length prefix before each field, or hash file content to a fixed-size digest first and write that into the tree hash.

Confidence: 9/10 (cross-checked by construction, not just reading).

[jhrozek]

[HIGH] Sigstore verification is bound to the wrong digest — subject is a hash of the statement, not the artifact digest

Cross-checked against the vendored github.com/stacklok/toolhive-core@v0.0.46/container/verifier source. Its doc comments are explicit that the second parameter to VerifyBundleOffline/VerifyBundleOfflineWithKey is artifactDigest — "the ARTIFACT's own manifest digest... It is never the digest of a simple-signing payload" — and Bundle.DigestHex is documented as "the artifact's digest, NOT the digest of the blob the signature covers." The library even has a dedicated error (annotatePayloadDigestMisuse) naming this exact mistake for the simple-signing case.

Here, subject is computed as sha256(statement) — a hash of the in-toto attestation payload — and passed as the artifact digest (lines 56, 62). The real artifact digest (request.Digest / evidence.Attestation.SubjectDigest, already used separately elsewhere in verifyArtifact) is never passed into this Verify call at all — the EvidenceVerifier.Verify(ctx, statement, bundle []byte, identity, issuer string) error interface has no digest parameter.

Impact: for a real attestation bundle (empty SimpleSigningPayload), the library's in-toto Subject-digest check will not match a legitimately signed artifact — every genuinely, correctly-signed artifact would fail verification. This fails closed, but a security control that always rejects legitimate input is exactly the kind that gets bypassed or disabled under pressure in production. Untested against the real SigstoreVerifier — artifact_test.go/admission_test.go only exercise fake EvidenceVerifier stand-ins.

Recommendation: widen EvidenceVerifier.Verify to take the artifact digest explicitly (request.Digest) and pass that — not a hash of the statement — as artifactDigest/subject. Add a test against the real SigstoreVerifier with a real bundle.

Confidence: 9/10 (confirmed against the actual vendored dependency source, not inferred).

[jhrozek]

[HIGH] No clamp on setuid/setgid/world-writable mode bits when materializing artifact content into the verified cache and launch snapshot

copyTree reproduces the full info.Mode() (not .Perm()) onto destination directories (here, and line 691) and copyFileToRoot writes files with the source's mode taken verbatim (lines 723, 740) — not masked, so setuid/setgid/sticky bits pass through Chmod too.

This is functionally necessary for execution-image (the guest rootfs legitimately needs setuid /usr/bin/sudo, sticky /tmp, etc.), but the same code path also handles runtime, firmware, and guest-agent — artifacts executed directly on the host. Nothing here checks kind and strips dangerous bits for the host-executed kinds. Verification only checks that the pinned identity signed the exact content+mode digest — not what that mode is.

Critically, this preserves those bits into both the verified cache entry and, via snapshotArtifacts/LockAndValidate (the code added specifically to defend the launch snapshot against a "non-cooperating writer"), the private per-launch snapshot directory — weakening exactly the guarantee that mechanism exists to provide.

Not currently exploitable in isolation: every ancestor directory on both paths is hardcoded 0o700, so a lower-privileged local user can't traverse in today. But that containment is implicit and scattered across literals, not an enforced invariant — and if the signing pipeline is ever compromised, a validly-signed host-executed artifact with a setuid-root helper would be faithfully materialized bit-intact.

Recommendation: for host-executed kinds (runtime, firmware, guest-agent), explicitly reject or mask ModeSetuid/ModeSetgid/world-writable bits during copyTree/copyFileToRoot regardless of the signed content's mode. If mode must stay in the digest for execution-image fidelity, compute/verify the digest with the clamp already applied. Add a test asserting the verified-cache and launch-snapshot roots (and every ancestor) are never group/other accessible.

Confidence: 8/10 — found independently by two reviewers.

[jhrozek]

[HIGH] Prepared.Cleanup is a non-idempotent 4-step sequence gated on the thing step 1 destroys — a crash mid-cleanup is unretryable forever

Cleanup performs four independent, non-transactional removals: (1) worktreeParent.RemoveAll(worktreeName) (this line), (2) gitAdminParent.RemoveAll(gitAdminName), (3) removeWorktreeBound (git worktree prune + git branch -D), (4) metadataParent.RemoveAll(metadataName). bindCleanupTarget (called just above, line 429) cannot re-bind unless the worktree directory still exists and self-identifies (worktreeParent.Open, sameOpenDirectory, validateRemovalTarget, git symbolic-ref HEAD run inside the worktree). After step 1 succeeds, all of those preconditions are permanently false.

Impact: any failure or crash at steps 2-4 leaks .git/worktrees/<admin> and — because branch deletion is step 3 — the mecatl/<logicalID> branch in the operator's real repository, permanently, one per failed delete, visible in git branch/git worktree list. Worse, the caller never even reaches Cleanup again on retry: LogicalEnvironment.DeletePreservingDirty runs git status --porcelain in the worktree path first, which errors once the worktree is gone, so a retried delete dies at the dirty-check and the ref becomes permanently undeletable.

Recommendation: make Cleanup resumable — bind whatever still exists, treat a per-step target-missing as success, keep identity checks conditional on presence — or reorder so the identity-bearing worktree directory is removed last. Have DeletePreservingDirty treat a missing worktree as "clean, resume cleanup" rather than a hard error.

Confidence: 10/10.

[jhrozek]

[HIGH] attachment.json actively lies after an interrupted delete — the in-flight marker is in-memory only

delete() sets record.deleting = true in memory (line 292), releases the mutex, and performs all physical destruction (attachment.Logical.DeletePreservingDirty, this line) with nothing durable recording the attempt. The error path just resets deleting = false (line 305/321/331). The durable write happens only after physical cleanup, either via removeRepositoryAttachmentRecord (clean case) or persistRepositoryAttachment (retained case).

Two ways this produces an on-disk lie: (a) a crash/partial failure during the underlying worktree.Prepared.Cleanup leaves attachment.json still reading deleted:false, worktree_retained:false — "live" — while the worktree is gone or half-gone; (b) a crash between a fully successful cleanup and the record update leaves the metadata file intact even though the backing worktree/metadata dirs are already removed.

loadRecords (repository_attachment_store.go) does no existence check — it validates by string equality against the expected path, never Lstat — so the phantom record loads as healthy. A retry then dies in the git status pre-check (see the linked worktree.go Cleanup finding), and Reattach dies at os.Lstat(worktreePath). No reconciliation path exists for this record shape.

Recommendation: persist an intermediate durable state (e.g. cleanup_pending) on repositoryAttachmentDocument before touching the backend, then clean, then finalize; have loadRecords route a cleanup_pending record into a resume-cleanup path instead of loading it as live.

Confidence: 10/10.

[jhrozek]

[MEDIUM] A zero-valued resource limit is silently treated as "unbounded," not "deny"

checkUsage only enforces a bound when item.limit > 0 (this line); any ResourceUsage field an operator leaves at Go's zero value is treated as "no limit" for every dimension. The natural reading of an omitted/zero field is "cap at zero" (deny), not "uncapped," and the doc comment on AdmissionLimits ("applies every positive bound") reads as informational rather than the operative security contract.

Impact: an operator who intends to bound e.g. RAM per user but forgets a field silently gets no ceiling for that dimension — unbounded local resource exhaustion (host DoS) from a single tenant.

Recommendation: require every dimension to be explicitly set via a validating constructor (reject zero as ambiguous, use an explicit sentinel for "unbounded"), or at minimum make the "0 = unbounded" contract loud in field-level doc comments plus a startup WARN enumerating unlimited resources.

Confidence: 8/10.

[jhrozek]

[MEDIUM] The Verify() → LockAndValidate() → Launch() handoff, which exists specifically to close a TOCTOU window, is enforced only by convention

Provisioner.Provision() correctly chains Verify → launchVerified → LockAndValidate (re-verify + copy to a private snapshot) → Launch. But Launcher (this interface) is public, VerifiedArtifacts/VerifiedArtifact have every field exported (including Path/Source), and Verify() itself is public and returns VerifiedArtifacts directly — usable standalone per its own doc comment ("admits ... without crossing the VM launch seam"). Nothing in the type system stops a future caller holding both a Launcher and the output of Verify() from calling Launch directly, skipping the re-validation-under-lock and copy-to-private-snapshot step — reopening exactly the race that two-step design was built to prevent.

Latent today (nothing else in the module holds a Launcher reference yet), but nothing enforces it stays that way.

Recommendation: don't expose Launcher/Launch as a standalone capability from the package's public surface. Either make Launcher.Launch accept an unexported type only LockAndValidate can produce, so the seam is enforced by the type system, or keep Launcher out of the intended external API and document Provisioner.Provision as the only sanctioned entry point.

Confidence: 8/10.

[jhrozek]

[MEDIUM] Boot-deferred IPv6 enforcement reports the identical status string whether it actually ran or is still pending — a false-safety signal on the exact control this feature exists to provide

NewHostedBootNetworkController() (this line) builds a controller with guest == nil and guestEnforcedBoot = true. In start(), the guard at line 134 is satisfied so Start() proceeds; the guest.DisableIPv6 call at line 161 is then skipped entirely because c.guest is nil. Start() still returns success with NetworkHandle.GuestEgress set from policy.status() (line 167), which for deny-all/allowlist unconditionally renders "...IPv6 disabled" (lines 187-190) — the exact same string it would produce on the path where DisableIPv6 actually ran and succeeded. NetworkHandle carries no field indicating IPv6 disablement is deferred/unverified for this construction.

The doc comment (lines 93-95) correctly states the owning runtime must verify that service before reporting readiness — documented, but not enforced or signalled by the API surface itself.

Impact: any current or future caller that reads handle.GuestEgress (or serializes it into a status/readiness response) without independently performing that out-of-band verification will report "IPv6 disabled" for a guest whose IPv6 stack is, at that moment, still fully enabled and unfiltered.

Recommendation: make the deferred state visible in the type system — e.g. a NetworkHandle.GuestIPv6EnforcementPending bool, or a distinct status string ("IPv6 disablement pending guest verification") when guestEnforcedBoot && guest == nil — so a caller that ignores the doc comment still gets an honest signal.

Confidence: 8/10.

[jhrozek]

[MEDIUM] status() mislabels an unrecognized egress mode as "allowlist"; goMicroVMHosts() correctly rejects the same value

status() has explicit cases for EgressPermissive/EgressDenyAll and a default (this line) that renders allowlist (N destinations; ...) for any other mode string, including an invalid/unknown one. goMicroVMHosts() (line 194) spells out case EgressAllowlist explicitly and its own default (line 210-211) returns unknown guest egress policy mode %q — the two switches classify the same value differently.

The value is operator-reachable: GuestEgressPolicy.Mode is a free-form string decoded straight from daemon config, with no validation at decode or construction. On the enforced path, goMicroVMHosts() runs before provider.Start inside start(), so Start/StartForDoctor never actually return the bad string in practice — but Status() is exported and called on paths that never go through start() first (before any VM boots), so a misconfigured daemon can surface and persist a fabricated claim of enforcement for a mode that's about to be rejected.

Also: mode is independently re-derived at four sites in this file with no single authoritative resolution point (start()'s inlined != EgressPermissive checks, tightened(), status(), goMicroVMHosts()) — a future mode addition only needs to miss one of them to silently diverge.

Recommendation (minimum fix): add the explicit case EgressAllowlist here and make default honest (fmt.Sprintf("invalid guest egress mode %q (rejected)", p.Mode)). Add a table test asserting status() and goMicroVMHosts() classify every mode (including a bogus one) identically — that test fails today. Longer-term, unify all four derivations behind one resolve() function.

Confidence: 9/10.

[jhrozek]

[MEDIUM] Duplicate/overlapping hostname allow-entries silently collapse to the first match — the second rule is dead with no validation error

goMicroVMHosts() performs no dedup/collision check across policy.Allow entries (this loop). The underlying go-microvm library's matcher (egress.Policy.matchHost) walks the host list in order and returns the first entry whose name matches — for two exact-name duplicates that's the first literal; for a wildcard listed before a literal it covers, the wildcard wins outright. The DNS interceptor then uses the same first-match lookup to decide which port/protocol restriction becomes the enforced dynamic firewall rule.

So two EgressDestinations for the same hostname with different Port/Protocol — a very natural config shape, e.g. {host,443,TCP} and {host,53,UDP} — result in only the first ever being enforced; the second is silently dead, with no error raised anywhere.

Verified fail-closed, not fail-open: this code never emits an unrestricted rule, so it can't accidentally widen access beyond the full configured set — not exploitable by a guest to reach something it shouldn't. But it's a real config-correctness bug in a security-control path, discoverable only via a failed guest connection.

Recommendation: track seen hostnames (post-normalization) here and reject a policy where two Allow entries normalize to the same literal name, or where an earlier wildcard would shadow a later literal — fail closed with a clear validation error instead of silently dropping an entry.

Confidence: 9/10 (cross-checked against the vendored go-microvm library source).

[jhrozek]

Second round of the batched review (see the first round's summary comment on this PR for context on why it's split). This round covers internal/adapter/microvm (host client), internal/adapter/microvmmanager (daemon bootstrap/egress/userns), the VM runtime/daemon/forker core, and the legacy per-session lifecycle/reconcile path.

As with round 1, this submits the HIGH-severity and structural findings plus a curated subset of MEDIUM findings that are directly reachable or point at a design question worth a decision. Lower-priority items (dead-code cleanup candidates already noted below, resource-consumption nits, LOW/INFO items) are being tracked separately.

Two items in this round aren't ordinary bug reports and are surfaced here as open questions rather than fixes to make: (1) most of the VM runtime's "standard" session-per-VM path — including the file that implements the ADR 0211 EnvironmentForker contract — appears to have no production caller; (2) two independent reviewers, looking from different sides (host preflight vs. VM launch wiring), were each unable to verify that the guest-workload "unprivileged mapped user" claim in the PR description is actually enforced end-to-end. Both are presented as findings for the author to weigh, not conclusions.

[jhrozek]

[HIGH] Fork has no rollback on partial daemon-side failure, unlike the sibling create path

This validation check (rejecting an invalid child binding) mirrors what provisionPlacement does on its own create path — but provisionPlacement rolls back the daemon-side resource on a malformed response, and this function doesn't. If the daemon genuinely forks a child but returns a malformed/invalid binding, the child generation is left orphaned with no cleanup attempt from this side.

Also worth noting: this only validates Owner/EnvironmentID/Generation on the returned binding, not SessionID/Ref — the .-delimited ref encoding (SessionID+"."+EnvironmentID) is only safe because SessionID is asserted dot-free at mint time, and that assertion is never re-checked against the daemon's returned value here.

Worth considering: mirror provisionPlacement's rollback pattern on the error path, and validate the full binding tuple including SessionID against the original claim.

Confidence: 9/10.

[jhrozek]

[HIGH] Exec on a non-logical- (legacy/foreign) session ref bypasses the repository guard other operations get, and the fallthrough panics on a nil registry

This guard only routes to the repository-scoped exec handler when EnvironmentID has the logical- prefix; otherwise it falls through to d.boundRecord (line 362), which calls d.registry.Lookup (line 850). In production, DaemonConfig.Registry is never set — the daemon is always constructed via the repository-only path (confirmed against environment/microvm/cmd/mecatl-microvmd/main.go:216-219) — so d.registry is a nil interface and this call panics.

Every other lifecycle operation correctly fails closed with ErrEnvironmentUnavailable on a non-repository binding; exec is the one path routed around that check (ServeConn, daemon.go:323, dispatches LifecycleExec to handleExecStream directly, bypassing handleAuthenticated's guard entirely).

Reachable without any malicious intent: a session persisted before the repository model, or a corrupted/foreign EnvironmentID, triggers this on the first exec call. RuntimeDaemon.Serve runs each connection in its own goroutine with only defer conn.Close() — no recover — so this one panic takes down the daemon process, and with it every other live VM's control plane on the host.

Worth considering: apply the same repository-vs-legacy guard here that other operations already have, and/or add a recover in the per-connection goroutine so a future handler defect can't take the whole daemon down.

Confidence: 9/10.

[jhrozek]

[HIGH] Worktree physical cleanup has no crash-idempotent recovery path, unlike its VM-side sibling in the same reconciler

The VM-destruction half of cleanup (cleanupRuntime, elsewhere in this file) is deliberately idempotent to a crash: if Inspect reports the VM handle already gone, it treats a re-Destroy call as success. cleanupWorktree (this function) has no equivalent. It calls Dirty() (runs git status against the worktree path) then, if clean, Cleanup() (opens the worktree directory, verifies identity, removes it).

If a crash happens after the physical removal succeeds but before record.WorktreeDeleted = true is persisted a few lines below, the record reloads still needing cleanup — but both Dirty() and Cleanup() hard-error against a now-missing directory rather than treating "already gone" as success. The record can never reach Destroyed.

No physical resource leak (the disk space was actually freed) — but the durable record wedges permanently in a pending state, and nothing currently reconciles it short of manual registry-file editing.

Worth considering: give the worktree side an "already gone" convention symmetric to what cleanupRuntime already has for the VM side.

A related note on test coverage: the crash-recovery test that appears to cover this case sets a fixture flag directly rather than exercising the real path-removal code, so the scenario it's meant to prove isn't actually exercised.

Confidence: 9/10.

[jhrozek]

[HIGH] No fence between a reconciler sweep and a live session's own Create — a provisioning record is treated as unconditionally cleanable

This case unconditionally marks any record in EnvironmentProvisioning as eligible for cleanup, with no check for creator liveness or a minimum record age. A reconcile pass (crash-recovery sweep) can therefore race and destroy a VM/worktree that an in-flight Create call is still actively provisioning.

Worth considering: gate provisioning-state cleanup on either creator/lease liveness or a minimum age threshold, so a genuinely-in-progress create isn't mistaken for an abandoned one.

Confidence: 8/10.

[jhrozek]

[HIGH] AdmissionUsage can be released twice — the monotonic transition guard permits destroyed → destroyed

This release call has no guard against having already run for this record. Since registry.go's transition guard allows a destroyed record to be saved again as destroyed (it only rejects regressions, not repeated identical states), two racing reconcile passes — two daemon instances, or an inline reconcile inside Delete racing an independent sweep — can each reach this line for the same record and each call Release on the same usage, double-crediting the admission-quota accounting the AdmissionController depends on.

Worth considering: add a durable AdmissionReleased flag to the record, made monotonic by the same transition guard, and gate this call on it.

Confidence: 8/10.

[jhrozek]

[MEDIUM] The daemon config/policy shape is hand-duplicated across three independent declarations, with a fail-open drift risk

This desired map, the cfg map built in writeDaemonConfig (this file), and the anonymous struct in egress.go's readGuestEgressPolicy each independently re-declare the same JSON shape. configuredRequestCompatible (this function) only compares keys it itself constructs into desired — so a future field added to writeDaemonConfig's config but forgotten here would mean an already-configured host is reported "compatible" with a new policy it doesn't actually have, and the daemon is never restarted to pick up the real intent. The existing tests cover today's one egress key but wouldn't catch this class of regression on a new field.

Worth considering: a single shared constructor for this shape (or typed values with real JSON tags) used by both the read and write sides, so the two can't independently drift.

Confidence: 9/10.

[jhrozek]

[MEDIUM] Daemon-controlled error text reaches model-visible tool output without going through the codebase's own neutralization step

This formats response.ErrorText — daemon-controlled content — directly into the returned error for every error code except one (repositoryLogicalRootUnavailableCategory, handled specially just above). That error then flows into tool-result rendering (e.g. subagent and parallel-branch result formatting) before reaching the point where the codebase documents that every delegation-result arm neutralizes model-influenced text.

Worth considering: either extend the existing suppression to every error code, or route these two error-producing call sites through the existing neutralization helper before the text becomes part of a tool result.

Confidence: 8/10.

[jhrozek]

[MEDIUM] LibkrunBackend.instances is insert-only — entries are never removed

This map is populated on every VM start but nothing in Stop/Remove/Destroy (in this file or in repository_runtime.go's rollback/abort paths, which only clean their own separate bookkeeping) ever deletes from it. Since this backend is shared by both the repository path and the (apparently unreachable) standard path, every VM ever booted stays pinned in this map for the daemon's entire process lifetime — a live memory/handle-count growth, plus the possibility of a lookup returning a stale/destroyed generation's instance. The only thing preventing that stale instance from being reattached to today is a downstream liveness check rejecting it as a side effect, not a guarantee this map itself provides.

Worth considering: remove the entry on the corresponding cleanup path.

Confidence: 9/10.

[jhrozek]

[MEDIUM] Stop — privileged PID-signaling logic — appears to have no production caller anywhere in the repository

This method (and its platform-specific process-identity helpers) is part of the Operations interface, but grep across the repo turns up no caller in Manager, cmd/mecated, or internal/microvvmcmd (which only wires Doctor/Status/Delete). It's also the most security-sensitive code in this package — it reads a PID from a file and sends it a signal.

Worth considering: either an explicit decision to wire this in (with a pointer to where), or removing it until something needs it — unused-but-privileged code tends to get adopted later by someone who doesn't re-review its defenses at that point.

Confidence: 9/10.

[jhrozek]

[MEDIUM] The monotonic-transition guard covers 6 of roughly 25 durable record fields

This function checks state rank, tombstone, ParentRef/ForkBase, and VMDeleted/WorktreeDeleted monotonicity — but fields like Owner, WorktreePath, VMID, RunnerPID, and ProcessIdentity are freely overwritable within a generation with no check at all. This is latent today (nothing observed writes these outside the intended path), but WorktreePath feeds a destructive git worktree remove and RunnerPID/ProcessIdentity gate cleanup eligibility — so an unguarded write to either would have real consequences if it ever happened.

Worth considering: inverting this to an explicit allow-list of fields that may change within a generation, rather than a guard that only checks a subset.

Confidence: 9/10.

[jhrozek]

Third round of the batched review, covering the guest-side subsystem: environment/microvm/{control,guestagent,guestexec,workspace,virtiofs,gitexec} — the actual host↔guest trust boundary, which only had incidental coverage in earlier rounds. As with the prior rounds, this submits HIGH-severity/structural findings plus a curated subset of MEDIUM findings that are directly reachable or point at a design question worth a decision; LOW/INFO items and findings not independently confirmed reachable are being tracked separately.

One resolved open question worth noting explicitly: the "guest workloads run as an unprivileged mapped user" claim, flagged as unverifiable across two prior review rounds (looking from the host-preflight side and the VM-launch-wiring side), was traced end-to-end in this round from the remaining angle — the actual guest-side exec/privilege-drop code. No bypass was found: the drop is applied on the sole exec call site, unconditionally, with no TOCTOU, verified in part directly against the Go stdlib's fork/exec source. That question is now closed.

A new finding worth flagging as connecting two earlier rounds: the host-side git apply merge-back path (child-environment merge, environment/microvm/operational.go) writes directly to a live worktree with no participation in the guest's own RPC-based version/CAS protocol — a different angle on the same code area as an earlier round's finding that the live merge path takes no per-parent lock. That earlier finding was about two merges racing each other; this one is about a merge racing the parent session's own in-flight guest writes. Both point at the same underlying gap.

[jhrozek]

[HIGH] A deadline-free blocking write is held under the request lock, and can wedge the entire client with no way for a caller's context to intervene

Stream takes c.requestMu.Lock() here and holds it across c.write(...) (below, around line 191) — a call into Codec.Write/the transport with no deadline and no context awareness. One connection is shared by both the workspace and exec services for a given environment (confirmed via the real caller, guestagent.Connect), so this isn't confined to one service.

When the underlying send buffer fills (a slow or stalled peer), every subsequent Call/Stream on this client parks on requestMu.Lock() — not selectable against its own caller's context, so per-call deadlines are never observed. The cancellation path for an already in-flight request also needs to acquire a write lock behind the same stalled writer, so cancelling doesn't rescue it either. The result is a permanent hang of all workspace and exec operations for that environment, not a bounded timeout.

Worth considering: move the wire write behind a single writer goroutine fed by a buffered/selectable channel, so enqueueing (which can stay under requestMu to preserve write ordering) is separated from the actual blocking I/O, which can then be interrupted by ctx or connection close. A smaller alternative: attach a write deadline derived from the connection's lifetime context before each write.

Confidence: 9/10.

[jhrozek]

[HIGH] The guest's connect to the host has no timeout and cannot be interrupted by context cancellation — a slow/absent host listener hangs the whole guest agent at boot

ctx.Err() is checked once, immediately before this line — a snapshot, not a live cancellation. unix.Connect is a raw syscall on a freshly-opened fd, before it's wrapped by anything the runtime poller (or a later ctx.Done()) could act on. If the host companion process is slow to start listening, momentarily backlogged, or crashes after binding but before accepting — exactly the kind of timing window most likely right after guest boot — the calling goroutine blocks in this syscall indefinitely.

Traced into the sole caller (cmd/mecatl-guest-agent/main.go): the dial runs synchronously on the main goroutine with a signal.NotifyContext(SIGINT, SIGTERM) context that has no deadline attached anywhere. If the host never accepts, the guest agent process hangs at startup with no observable progress, and the first SIGINT/SIGTERM is silently absorbed (it just cancels a context nothing is reading while blocked in the syscall) — only a second signal or SIGKILL actually terminates it.

Worth noting: this same package already has the right pattern elsewhere — repository_channel.go's authentication wraps its I/O in context.AfterFunc(ctx, stream.Close) specifically so a stuck read/write can be interrupted by closing the stream when ctx fires. This dial has no analogous backstop.

Worth considering: either drive the connect through a non-blocking fd + the runtime poller (mirroring how net.Dial handles this for AF_INET), or race the syscall in a goroutine and close the fd from the calling side if ctx fires first, plus attach a bounded per-dial timeout at the one caller.

Confidence: 8/10.

[jhrozek]

[HIGH] Unregister acquires a per-registration lock while still holding the server-wide lock — one stuck request can freeze every other worktree in the VM

The sequence here is: take s.mu.Lock() (line 149, server-wide), then — while still holding it — take registration.mu.Lock() (this line). That per-registration lock is held as a read-lock for the entire duration of any dispatched Workspace/Exec handler call, which can be an arbitrarily long-running command with no wall-clock bound (guestexec.Limits bounds output size and concurrency, not duration).

So: one in-flight long/hung request on worktree A, combined with an ordinary Unregister(A) call (e.g. normal session teardown racing a slow command — not an adversarial scenario), blocks Unregister on the per-registration lock while still holding the server-wide one. Every other operation needing that lock — Register, Probe, and the connection-dispatch resolver that runs on every new connection handshake — blocks behind it too, because Go's RWMutex starves new readers once a writer is waiting. Against a peer that simply stops reading its own data connection while a command is still producing output, this isn't a transient stall but a permanent wedge of the whole guest agent for every tenant sharing that repository VM, not just the one being unregistered.

Worth considering: this file's own Close() function already has the right shape to use as a template — delete the registration from the map and release the server-wide lock first, then acquire the per-registration lock afterward to wait out any in-flight handler before actually closing it. The map deletion alone is enough to keep new dispatch from resolving to the torn-down registration.

Confidence: 9/10 — independently identified by two separate reviewers in this round, converging on the same fix.

[jhrozek]

[MEDIUM] This nonce ceiling is a hard, global lockout, and one real code path never frees a slot — a long-lived guest can permanently lose the ability to authenticate

Once len(v.used) >= maxCapabilityNonces (4096), every future Verify call fails — including a fresh, correctly-signed, never-used capability — until enough entries are removed via Forget.

The repository-scoped guest path correctly pairs every Register/Unregister with a Forget call, keeping it bounded across a session's lifetime. But the separate, standalone (non-repository) guest path constructs its own CapabilityVerifier and, as far as this review could trace, never calls Forget on it anywhere in the codebase. Every successful negotiate/reconnect on that path — a daemon restart reattaching, a transient vsock dial retry, any renegotiation — permanently consumes one of the 4096 slots for the life of that guest agent process.

Impact: a long-lived guest VM that reconnects on this path more than 4096 times over its lifetime becomes permanently unable to authenticate to its own control channel, recoverable only by restarting the VM (and losing whatever state the sandboxed environment held). Since only one live capability is meaningful per generation at a time on this path, forgetting the previous nonce once a fresh one is successfully consumed would keep this bounded.

Worth considering: either wire cleanup into this path the same way the repository path does, or replace the hard ceiling with TTL/LRU eviction so exhaustion degrades gracefully instead of becoming a permanent, restart-requiring lockout.

Confidence: 8/10.

[jhrozek]

[MEDIUM] The single shared read loop means a slow consumer on one service stalls concurrent calls on the other, compounding the request-lock finding above

readLoop dispatches each frame inline through a channel with exactly one frame of buffer slack; once that's full, the loop parks delivering to one in-flight request's receive callback, and the codec isn't advanced for any other concurrent request on the same connection — including the other service sharing this client. receive here is arbitrary caller work (e.g. exec output is handed directly to a caller-supplied io.Writer), so a slow consumer on the exec side can stall concurrent workspace calls, and is also what fills the send buffer that the write-lock finding above depends on to actually wedge.

This may be an accepted consequence of the single-reader design rather than a bug to fix outright — worth documenting explicitly on Stream (that receive runs on the shared reader goroutine and must not block) if it's staying as-is, since it's the kind of thing a future caller will get wrong without that warning.

Confidence: 9/10.

[jhrozek]

[MEDIUM] NewGuest validates the binding it's given and then discards it — this package performs no generation check of its own on subsequent requests

The binding passed here (including its generation) is checked for completeness and then never stored on the resulting Guest — nothing in this package's request handling re-checks it per call. All generation fencing for this guest-side component is transitive, relying entirely on whichever multiplexer authenticated the connection this handler is registered on.

The signature strongly implies this type is generation-fenced on its own; it isn't. Concretely, the binding_mismatch error this package defines is never actually produced by anything here — which is itself a sign the enforcement this parameter implies isn't actually happening at this layer.

This is bounded today by os.Root confinement (worst case would be cross-generation access to a worktree, not arbitrary host filesystem access), and depends on whether the connection-routing layer (a different part of this review, environment/microvm/control) could ever hand a mismatched-generation handler a live, authenticated connection — worth confirming that can't happen, since if it can this escalates.

Worth considering: either store the binding and check it per-request (returning the binding_mismatch error that already exists for exactly this), or drop the parameter and document explicitly that authentication is the control layer's job alone — the current half-measure implies a guarantee this file doesn't provide.

Confidence: 9/10.

[jhrozek]

[MEDIUM] The version/CAS check here has a real lost-update window against a writer outside this RPC service, and the guarantee is weaker than the main engine's equivalent — undocumented either way

This function reads, compares the caller's version against current content, writes to a temp file, then re-checks the version a second time immediately before rename — correctly closing the window against another caller of this same service (serialized by this type's own lock). But the final rename itself is unconditional: a write landing between that second check and the rename from an out-of-process writer sharing the same underlying files — concretely, a host-side git apply/git checkout writing across virtio-fs to the same worktree — would be silently clobbered, with no error to either side.

This is a real difference from the main engine's Workspace/FileVersion CAS protocol (documented in this repo's AGENTS.md, ADR 0208), which guarantees a model-visible mutation is never silently overwritten. That guarantee doesn't hold here across the host/guest boundary, and nothing currently documents that it doesn't.

Whether this is reachable in practice depends on whether host-side git operations (e.g. a child-environment merge-back) can ever run concurrently against a worktree while its VM is still live and being written to by the guest — worth confirming with whoever owns that lifecycle code, since it determines whether this is a live gap or purely theoretical.

Worth considering: if host-side writers and a live guest genuinely never overlap in time, say so in a comment here (reducing this to documentation); if they can, this needs either a shared lock across the host/guest boundary or an explicit, named limitation in the relevant ADR.

Confidence: 8/10.

[jhrozek]

[MEDIUM] The overlap check this function's own error is documented to provide only catches exact duplicates, not nesting

The doc comment on ErrUnsafeMount (a few lines above) says it rejects an "overlapping" host mount input, but this check (and its guest-path counterpart just below) only compares canonicalized paths for exact string equality. A nested pair — e.g. the metadata directory living inside the worktree directory, which is the ordinary layout for a non-detached .git — passes this check without being caught, on either the host or guest path axis.

Impact: in a nested-metadata scenario, the read-write git-metadata mount and the workspace mount would alias the same underlying bytes under two separate guest paths with two independent virtio-fs device caches — quietly breaking the isolation this mount plan's own documented design assumes (that guest exec pins GIT_DIR to the separately-mounted metadata path specifically so the worktree's own .git file is never consulted). Reachability depends on whether the worktree-preparation code (reviewed separately) always produces a genuinely detached metadata directory outside the worktree tree — but this function is the validation boundary that's supposed to catch it either way, per its own doc comment.

Worth considering: after canonicalization, reject any pair where one path is a path-prefix of another (on both the host and guest axis), not just exact matches — or narrow the doc comment to "duplicate" and state explicitly where non-overlap is actually guaranteed instead.

Confidence: 9/10.

[jhrozek]

[MEDIUM] This type's own read-ledger appears to be dead code, built against a superseded version of the Workspace contract

These fields (and the RecordRead/RecordedVersion methods built on them, further down this file) match the shape this repo's AGENTS.md still documents for the main engine's tool.Workspace — but as of a newer ADR, that responsibility moved out to a separately-composed tool.ReadLedger on the Environment, and tool.Workspace no longer requires these methods at all. Every real construction site for this particular Workspace type (checked directly) wires in a plain in-memory ledger implementation as the Environment's ReadLedger instead — never this type's own map. The reference filesystem adapters elsewhere in the codebase have already dropped the equivalent methods entirely.

This doesn't look like a security bug — the actual read-before-write evidence used for real CAS decisions appears to be tracked correctly elsewhere — but it's actively misleading: a maintainer reading this file's doc comments would reasonably conclude this is the load-bearing version-tracking store for microVM sessions, when apparently nothing calls into it. Worth checking whether other parts of this PR made the same assumption about which contract version applies.

Worth considering: remove these fields and methods if confirmed unused, or wire them in if there's a reason this type specifically needs its own ledger rather than relying on the composed one.

Confidence: 9/10.

[jhrozek]

[MEDIUM-HIGH, pending confirmation] The child-environment merge-back writes to the parent worktree with no participation in the guest's own version/CAS protocol — connects to an earlier finding on this same merge path

This git apply runs directly against the parent's worktree from the host process, over virtio-fs, with no lock shared with the guest-side Guest.replace version/CAS path (reviewed separately) that the guest itself uses to serialize its own writes to the same files. A comment elsewhere in this merge path states a daemon-held parent lock covers concurrent merges — which is true for two merges racing each other, but doesn't address this merge racing the parent session's own in-flight guest writes (e.g. a live model-driven edit, or a still-running background command inside the guest writing to the same worktree at the moment this patch is applied).

This is the same code area as an earlier round's finding that this merge path takes no per-parent lock (a different angle: siblings racing each other rather than a merge racing the parent's own activity) — both point at the same underlying gap in this path's write serialization.

Impact, pending confirmation: if the engine's mutate-serial dispatch guarantee only bounds tool-call-boundary concurrency and not a spawned background process still writing inside the guest, a merge landing at the wrong moment could silently race and lose either write, with no detection.

This needs an answer specifically: is a merge into a given parent guaranteed to never run while that parent's own guest is independently still writing (e.g. via a backgrounded long-running command), or is that window actually reachable? That answer determines whether this is a real, live race or something already prevented by a scheduling guarantee elsewhere in the codebase.

Confidence: 8/10.

[jhrozek]

macOS guest command execution doesn't work yet, and it's a design question, not a bug

The stacked fixes for the local-dev platform gates are in #1629. This is about something separate I hit while actually testing that stack on real Apple Silicon hardware: once the platform gate is open, running a command inside the guest still fails on macOS, and the reason needs a decision, not a patch.

What's actually broken

The guest runs as a fixed unprivileged UID, 65532, and that depends on go-microvm's WithUserNamespaceUID — Linux user namespaces — to map that UID onto the daemon's real identity so virtiofs permission checks pass. On non-Linux platforms that option is a documented no-op. I confirmed this live: a Shell command inside a macOS guest can't even chdir into the virtiofs-mounted workspace, because the host directory is owned by the real macOS user (mode 0700) and the guest's unmapped 65532 fails the guest kernel's own permission check.

This isn't something that slipped through review. AC6.1 in the acceptance plan and ADR 0345 both scope the namespace-mapping mechanism to Linux amd64 on purpose, and the two tests that reject any ownership-override mount (virtiofs_test.go:60-61, runtime_test.go:240-241) are pinning that scoped decision — they all landed in the original feature commit, before macOS was ever in scope. Nobody rejected an alternative; there wasn't one to consider yet.

The obvious fix isn't a good one here

A sibling project, brood-box (same org, same go-microvm dependency), already solves this on macOS with VirtioFSMount.OverrideUID/OverrideGID — it stamps an xattr on every file before boot telling libkrun's virtiofs server to report a different owner to the guest than the real one. Same trick Podman uses on macOS. Looked like a drop-in fix, so I dug into it properly.

It doesn't actually buy the same security property, though not for the reason you'd expect. Reading libkrun's own source: macOS's virtiofs backend has no credential-switching at all, no set_creds, unlike Linux — every host operation already runs as the daemon's real identity no matter what UID the guest claims. The xattr never controlled host-side access on macOS; it just changes what the guest is told. So adopting it wouldn't weaken anything that currently exists there. It's more that it's a worse practical fit for mecatl specifically: go-microvm only stamps read-write mounts today, so mecatl's read-only git-objects mount would stay broken and git with it; and mecatl does host-side git apply into a live parent worktree during subagent merges, which would leave those files unstamped and unreadable to the parent guest. Podman never hits that because its rootfs is static.

What I'd do instead: make the guest's UID match the host's

Two things made this click. First, the "fixed 65532" requirement is stricter than it needs to be — the actual security property is "not guest-root," not "specifically this number." A guest process at the host's own UID is exactly as unprivileged as one at 65532. Second, guest init already runs as root (the guest agent already chowns things to 0:0/65532 at boot), so it can set its own workload UID to whatever the host tells it, for free, on either platform.

If the guest just runs as the host's real UID, the guest kernel's permission check passes honestly — no xattr walk, no staleness window, no read-only-mount gap. This is also just what Lima does. I traced the actual guest-side code (process_linux.go's configureWorkloadIdentity) and it's a plain numeric credential drop, no /etc/passwd entry involved, so this is a shallow change, not a deep one.

I spiked it and it actually works

Hardcoded guestexec.DefaultWorkloadIdentity() to return my real host UID/GID, dropped the one assertion in repository_rootfs.go that rejects anything but 65532, rebuilt the guest agent, and ran a real command through the HTTP API against a real booted VM:

--- id ---
uid=501 gid=20(dialout) groups=20(dialout)
--- uname ---
Linux localhost 6.12.91 #1 SMP ... aarch64 GNU/Linux
--- os-release ---
ID=wolfi
NAME="Wolfi"
...
--- workspace write ---
-rw-r--r-- 1 501 dialout 6 Sep 16 20:54 ./probe.txt
hello
[exit code: 0]

Real Linux/Wolfi kernel, not the host. UID matches the host exactly. File write and read-back through virtiofs both worked cleanly. (dialout is just what the guest's own /etc/group happens to call GID 20 — the check is purely numeric, no passwd/group baking needed, which matches the "shallow change" read.)

Open questions before this is a real plan

• Should Linux switch to the same approach, or keep the namespace mapping it already has? The namespace approach can juggle multiple distinct guest identities sharing one mount; UID convergence can't, everyone becomes "whoever the daemon's real user is." Doesn't matter for mecatl today, since it's always one VM, one workload, one UID, never shared — but that's worth deciding on purpose rather than losing by default.
• AC6.1 needs to be honest about what's platform-specific: the "never widened to world-readable/writable" half holds either way, the "namespace-mapping" half is a Linux-only mechanism and shouldn't be implied for macOS.
• The two guard tests should become platform-conditional rather than deleted — keep them asserting zero ownership overrides on Linux, since a silent downgrade from namespace-mapping to something else there would be a real regression.
• Two things worth raising with go-microvm/libkrun upstream regardless of what mecatl does: read-only mounts never get OverrideUID stamped today, which is a real gap for anyone using that mechanism; and libkrun already has PermissionSemantics::LinuxSimplified implemented in its macOS FUSE code (always use the requesting process's own UID/GID) but it isn't exposed through the public API or go-microvm's bindings yet — that's UID convergence, already built, just not reachable.

I can turn this into a Plan/Interface PR if that's the right next step. Posting this now mostly to hand over what I found with enough evidence to make that call, not to propose landing the spike as-is.