Repository navigation
Support Cloud Hypervisor primaries with static script enclaves - #67772
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
🟡 Changes recommended
The version gate targets unsupported AWF releases, and some conflicting runtime overrides bypass validation.
2 open findings
What changed in this PR
Enables experimental Cloud Hypervisor primaries with static Cloud Hypervisor script enclaves while preserving artifact and MCP gateway wiring.
Changes:
- Allows matching Cloud Hypervisor primary/enclave runtimes.
- Adds mismatch validation and compilation coverage.
- Documents requirements and acceptance boundaries.
| File | Description |
|---|---|
pkg/workflow/sandbox_validation.go |
Removes the blanket enclave rejection. |
pkg/workflow/enclaves_runtime.go |
Adds combination-specific validation. |
pkg/workflow/enclaves_runtime_test.go |
Tests runtime compatibility rules. |
pkg/workflow/enclave_cloud_hypervisor_test.go |
Verifies generated setup and gateway wiring. |
pkg/workflow/cloud_hypervisor_test.go |
Updates sandbox validation expectations. |
docs/src/content/docs/experimental/enclaves.md |
Documents the experimental combination. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| case AgentRuntimeDockerSudoIptables: | ||
| return errors.New("enclaves cloud-hypervisor runtime is incompatible with sandbox.agent.runtime: docker-sudo-iptables; use sandbox.agent.runtime: docker") | ||
| case "sbx", AgentRuntimeCloudHypervisor, "nvx": | ||
| case "sbx", "nvx": |
There was a problem hiding this comment.
Addressed in 695f9a9: the combined Cloud Hypervisor primary/script-enclave path now requires AWF v0.28.51+, separately from the v0.28.47 standalone enclave minimum. The default AWF version and sample/tests were updated.
| switch runtime { | ||
| case "sbx", "nvx": | ||
| return fmt.Errorf("enclaves cloud-hypervisor runtime is incompatible with --container-runtime %s; use a Docker primary agent runtime", runtime) | ||
| case "docker", "cloud-hypervisor": | ||
| if runtime != primaryRuntime { | ||
| return fmt.Errorf("enclaves cloud-hypervisor runtime requires --container-runtime %s to match sandbox.agent.runtime", primaryRuntime) | ||
| } | ||
| } |
There was a problem hiding this comment.
Addressed in 695f9a9: --container-runtime is now rejected unless its value exactly matches the primary runtime, including unknown, gvisor, and missing values. Regression tests cover both argument forms and duplicate overrides.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (113 new lines in 📄 Draft ADR committed:
🔍 Decision inferred from the diff
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot address review feedback |
There was a problem hiding this comment.
Request changes
This relaxes the Cloud Hypervisor validation too far: a Cloud Hypervisor primary can now slip through with default/Docker enclaves that never selected the supported runtime.
Blocking theme
The new implementation only runs the Cloud Hypervisor enclave checks when at least one enclave already declares `runtime: cloud-hypervisor`. Because this PR also removes the previous hard rejection for Cloud Hypervisor primaries with enclaves, unsupported primary+default-runtime enclave combinations now validate successfully instead of failing closed.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 55.9 AIC · ⌖ 5.64 AIC · ⊞ 21.1K
Comment /review to run again
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in 695f9a9: the unified primary/script-enclave path now requires AWF v0.28.51+, while standalone VM enclaves retain v0.28.47 support. All mismatched or missing runtime overrides are rejected, as are Docker/default enclaves under a Cloud Hypervisor primary. Updated regression tests, sample, and ADR. Build, focused tests, lint, impacted-test gate, workflow drift checks, secret scan, and fallback code review passed. Automated review was unavailable; CodeQL skipped analysis due to database size. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd on the validation path for Cloud Hypervisor primary + enclave compatibility. Requesting changes: one real validation gap that lets a declared-incompatible configuration compile successfully.
📋 Key Themes & Highlights
Key Themes
- Validation gap:
validateCloudHypervisorEnclaveschecks the primary/enclave runtime match only through--container-runtimeAWF args (validateCloudHypervisorEnclaveAWFArgs), but never validates thatenclave.Runtimeitself equalscloud-hypervisorwhen the primary iscloud-hypervisor. A Docker-runtime (or default/omitted-runtime) enclave combined with a Cloud Hypervisor primary compiles without error, contradicting both the PR description and the new docs text ("A Cloud Hypervisor primary may be combined with static script enclaves when both explicitly selectcloud-hypervisor"). - Test coverage masks the gap: every new/updated test case uses the
cloudHypervisorEnclaveWorkflowDatahelper, which unconditionally forces every enclave'sRuntimetocloud-hypervisorbefore the mutation under test runs. No test exercises a CH primary paired with a Docker/default-runtime enclave, so TDD's red-green loop never surfaced this. - Reproduced directly: compiling a markdown workflow with
sandbox.agent.runtime: cloud-hypervisorand an enclave omittingruntime:produces a lock file containing both the CH primary's--container-runtime cloud-hypervisorand a Docker-shaped enclave config side-by-side.
Positive Highlights
- ✅ Clean extraction of
validateCloudHypervisorEnclaveAWFArgsto share the AWF-arg parsing between the pre-existing Docker-only path and the new dual-runtime path. - ✅ Good coverage of the already-handled failure modes (agent enclave + CH primary, conflicting
--container-runtimeargs in both directions). - ✅ Documentation clearly states the experimental scope and acceptance boundary.
Flagged the gap inline with a suggested fix and regression test. Everything else in the diff looks solid.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 360.7 AIC · ⌖ 13.7 AIC · ⊞ 10.3K
Comment /matt to run again
| return err | ||
| } | ||
| for i, enclave := range workflowData.Enclaves { | ||
| if enclave.Runtime != string(AgentRuntimeCloudHypervisor) { |
There was a problem hiding this comment.
[/tdd] With a Cloud Hypervisor primary, validateCloudHypervisorEnclaves never checks that each enclave's runtime is actually cloud-hypervisor; it only rejects a non-cloud-hypervisor enclave when the enclave list already contains at least one cloud-hypervisor entry (gated by hasCloudHypervisorEnclaves at line 42). A workflow with sandbox.agent.runtime: cloud-hypervisor and an enclave that omits runtime (or sets runtime: docker) compiles successfully today, emitting a lock file that still stages the CH bundle/enclave-artifact steps and --container-runtime cloud-hypervisor for the primary while the enclave config silently requests Docker — exactly the Docker/VM mismatch this PR's validation is meant to prevent.
💡 Reproduction and suggested fix
Verified by compiling (with the private cloudHypervisorEnclaveWorkflowData test helper) a sandbox.agent.runtime: cloud-hypervisor workflow whose single enclave had Runtime: "" and Runtime: "docker": both pass validateEnclavesConfig with err == nil, and compiling an equivalent markdown workflow produces a lock file containing both --container-runtime cloud-hypervisor for the primary and the Docker-shaped enclave entry.
Add an explicit check once a CH primary is detected, e.g. in validateCloudHypervisorEnclaves:
if isCloudHypervisorRuntime(workflowData) {
for i, enclave := range workflowData.Enclaves {
if enclave.Runtime != string(AgentRuntimeCloudHypervisor) {
return fmt.Errorf("enclaves[%d].runtime must be cloud-hypervisor when sandbox.agent.runtime is cloud-hypervisor; Docker and default enclave runtimes are not supported with a Cloud Hypervisor primary", i)
}
}
}Add a regression test (e.g. in enclaves_runtime_test.go) asserting this combination is rejected, matching the existing cloudHypervisorEnclaveWorkflowData helper pattern.
@copilot please address this.
There was a problem hiding this comment.
Already addressed in 695f9a9: the enclave validation explicitly rejects each non-Cloud-Hypervisor runtime, including omitted and Docker values, with regression tests for both cases.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 695f9a9
|
…ling-cloud-hypervisor-enclave Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
Thanks for this contribution, Copilot! 👋 This PR looks ready for review — it includes tests for the Cloud Hypervisor + enclave feature, updated docs, and a clear description. The focused 292-line diff is easy to review. Great work!
|
|
🎉 This pull request is included in a new release. Release: |
The compiler rejected Cloud Hypervisor primary agents combined with enclaves, blocking compiler-owned workflows targeting AWF’s unified workload backend. This change enables the same-backend combination for static script enclaves while keeping it experimental; compilation is not evidence of Linux/KVM acceptance or production readiness.
v0.28.47+, experimental status, and the acceptance boundary.