fix: get the NVX Copilot smoke test to genuinely pass on real KVM hardware - #9010
Merged
Merged
Conversation
The workflow invoked awf with '--env AWF_NVX_SMOKE_MARKER' (a bare variable name), but AWF's --env flag only accepts KEY=VALUE pairs (validateAgentOptions in src/commands/validators/agent-options.ts). This caused awf to abort with 'Invalid environment variable format' before the microVM ever booted, which cascaded into all four smoke checks failing (microvm-run, guest-assertions, workspace-copy-back, copilot-inference) — confirmed by running the workflow against main (run 36156409881). Despite all four checks failing, the workflow's agent step incorrectly called noop (pass) instead of filing an issue, masking the failure. That agent-judgment gap is tracked separately; this fix addresses the underlying root cause so the smoke test can actually exercise NVX's per-run env passthrough. Fix: pass '--env "AWF_NVX_SMOKE_MARKER=$marker"' directly, and drop the now-redundant AWF_NVX_SMOKE_MARKER=$marker prefix on the sudo invocation (COPILOT_GITHUB_TOKEN is still preserved via --preserve-env for the guest credential-leak check). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The source and generated workflow consistently apply the required environment-variable format.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes the NVX smoke workflow’s invalid --env argument.
Changes:
- Passes the marker as
KEY=VALUE. - Removes redundant
sudoenvironment assignment. - Regenerates the workflow lock file.
| File | Description |
|---|---|
.github/workflows/smoke-nvx-copilot.md |
Corrects marker environment passthrough. |
.github/workflows/smoke-nvx-copilot.lock.yml |
Synchronizes generated workflow output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
'Restore artifact permissions' only chmod'd the downloaded artifacts; actions/download-artifact restores them owned by the runner user, but NVX's preflight (assertTrustedFile in src/nvx/preflight.ts) requires every trusted artifact (openvmm, kernel, initramfs, manifest, bundle) to be root-owned (uid 0). Confirmed via a real run on this PR branch (36157990756): awf aborted with 'NVX artifact manifest must be a non-empty root-owned regular file', cascading into all four smoke checks failing again despite the earlier --env fix. Fix: sudo chown root:root the five trusted artifacts before chmod. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
Once the trusted artifacts are chowned to root:root, the unprivileged
runner user can no longer chmod them ('Operation not permitted'),
which failed the 'Restore artifact permissions' step outright
(confirmed via run 36158981813 on this branch). Prefix both chmod
calls with sudo, matching the chown.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
1 similar comment
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
The smoke test's 'Build workflow-attested NVX artifacts' job builds and self-attests its own manifest with actions/attest-build-provenance, passing --nvx-signer-workflow pointing at itself. But assertTrustedSignerWorkflow only accepted two hardcoded workflows (release.yml and nvx-phase-3b-live-kvm.yml), so awf aborted with 'Untrusted NVX artifact signer workflow: .../smoke-nvx-copilot.lock.yml' before the microVM ever booted — confirmed via a real run on this PR branch (36159425271), cascading into all four smoke checks failing yet again after the earlier chown/env fixes. Add smoke-nvx-copilot.lock.yml as a third trusted signer, following the same pattern as the existing nvx-phase-3b-live-kvm.yml validation workflow entry, and update the corresponding preflight test coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (3 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/nvx/artifact-manifest.ts |
87.7% → 87.9% (+0.18%) | 87.7% → 87.9% (+0.18%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
…NVX guest check The smoke test's guest script banned any env var named GH_TOKEN, GITHUB_TOKEN, COPILOT_GITHUB_TOKEN, OPENAI_API_KEY, or ANTHROPIC_API_KEY from reaching the NVX guest at all. But --enable-api-proxy intentionally sets COPILOT_GITHUB_TOKEN to a fixed, non-secret placeholder (COPILOT_PLACEHOLDER_TOKEN) before the guest ever sees it, so Copilot CLI's auth precheck passes while the real token stays isolated in the api-proxy sidecar (src/services/credentials/copilot-credential-env.ts). That placeholder's mere presence was tripping the guest's check as a false-positive "credential leak", even though the real invariant (no real secret value reaches the guest) already holds and is enforced host-side by assertNoProviderSecrets() in src/microvm/guest-environment.ts. Replace the blanket ban on COPILOT_GITHUB_TOKEN with a check that its value, if present, is exactly the known isolation placeholder -- so a genuine leaked credential still fails the check, while the expected placeholder no longer does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (3 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/nvx/artifact-manifest.ts |
87.7% → 87.9% (+0.18%) | 87.7% → 87.9% (+0.18%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
The default NVX scratch overlay is 128 MB (NVX_MIN_SCRATCH_BYTES). Running the pinned @github/copilot-linuxmusl-x64 binary inside the guest was hitting ENOSPC on that overlay (repeated across all 3 retries per awf.log), too small for Copilot CLIs own runtime cache/extraction on top of the rest of the writable overlays needs. Pass --nvx-scratch-bytes 536870912 (512 MB) explicitly for this smoke test guest invocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (3 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/nvx/artifact-manifest.ts |
87.7% → 87.9% (+0.18%) | 87.7% → 87.9% (+0.18%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
lpcox
changed the title
The inner awf invocation's api-proxy sidecar (the container that serves the guest's copilot-inference check) previously wrote its logs to the default ephemeral workDir path, which is discarded when the runner job ends -- since only the fixed $data_dir tree gets bundled into the workflow's uploaded artifacts, those logs were never actually available for post-run inspection even though the api-proxy container itself always runs on the host, not inside the microVM guest. Pass --proxy-logs-dir "$data_dir/logs/inner-proxy-logs" so the sidecar's token-tracker-audit.jsonl and other logs land under the directory that is already chowned back to the runner user and uploaded as part of this step's evidence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 92.65% | 92.65% | ➡️ +0.00% |
| Statements | 91.13% | 91.13% | ➡️ +0.00% |
| Functions | 89.11% | 89.11% | ➡️ +0.00% |
| Branches | 84.26% | 84.25% | 📉 -0.01% |
📁 Per-file Coverage Changes (3 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/nvx/one-shot-adapter.ts |
83.5% → 82.9% (-0.60%) | 80.3% → 79.8% (-0.56%) |
src/nvx/artifact-manifest.ts |
87.7% → 87.9% (+0.18%) | 87.7% → 87.9% (+0.18%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
$data_dir (/tmp/gh-aw/agent/smoke-nvx-copilot) is not part of the compiler's hardcoded "Upload agent artifacts" path list, so writing the inner api-proxy sidecar's logs there (previous commit) is not by itself enough to make them inspectable after the job ends -- only what this step prints to its own stdout survives in the raw Actions log, which is also how scenarios.jsonl has been inspectable all along. Cat the inner sidecar's token-tracker-audit.jsonl to stdout at the end of the step, mirroring the existing `cat "$results"` for scenarios.jsonl, so future runs have a durable, greppable record of the real API calls the guest's copilot-inference check made through the proxy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
Preserve scenario results, AWF logs, Copilot response, workspace proof, and inner api-proxy audit data together as a downloadable run artifact. Keep proxy JSONL out of the agent output stream. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
gh-aw moved auditDir from the --audit-dir CLI flag into the generated awf-config.json (logging.auditDir), so the postprocess regex that injected --session-state-dir stopped matching any lock file. Without the flag, AWF wrote session state to its ephemeral work dir, the 'Copy Copilot session state' step found nothing, and the step summary parser saw only Copilot's plain-text process log -- rendering an empty agentic conversation under 'Log format not recognized as Copilot JSON array or JSONL.' Anchor the injection on the current 'awf --config' invocation form, keep the legacy --audit-dir pattern for older locks, and replace the misleading 'already present (or no awf invocation found)' message with a real warning. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened
#8983 merged NVX-to-Cloud-Hypervisor parity work (live workspace export, write-policy narrowing, per-run env passthrough,
--container-workdir) plus a new smoke test,smoke-nvx-copilot.md, meant to prove it on real KVM hardware. The PR author flagged that nothing was runtime-verified — the dev sandbox has no KVM/OpenVMM.I ran the workflow for real verification, finding and fixing the following issues by checking raw scenario output from each hardware run rather than trusting the agent's own summary:
Root causes found and fixed
--envbare-name format — the workflow passed--env AWF_NVX_SMOKE_MARKER(a bare name, expecting host-env inheritance), but AWF's--envonly acceptsKEY=VALUE. Fixed by passing--env "AWF_NVX_SMOKE_MARKER=$marker".Artifacts not root-owned — downloaded artifacts are runner-owned, while NVX preflight requires trusted artifacts to be root-owned. Fixed by applying
sudo chown root:rootbefore the existingchmodcalls, and usingsudo chmodafterward.Untrusted signer workflow —
assertTrustedSignerWorkflow()did not trust the smoke workflow's self-attested manifest. AddedNVX_SMOKE_SIGNER_WORKFLOWas a trusted entry and test coverage.False-positive credential-leak check — the guest check rejected the deliberate non-secret
COPILOT_GITHUB_TOKENisolation placeholder. It now asserts the known placeholder value, so a real leaked credential still fails.Guest scratch overlay too small — the default 128 MB overlay ran out of space running Copilot CLI. The smoke test now requests 512 MB.
Smoke evidence was not retained as a run artifact — configured
--proxy-logs-dir, stopped printing proxy JSONL into the agent stdout stream, and added a dedicatednvx-smoke-evidenceartifact. It includesscenarios.jsonl,logs/awf.log, all inner proxy logs (includingtoken-tracker-audit.jsonl),copilot-response.txt, andworkspace-proof.txt.Copilot step summary rendered an empty conversation — gh-aw moved
auditDirfrom the--audit-dirCLI flag into the generatedawf-config.json(logging.auditDir). This repo's postprocessing injected--session-state-dirby regex-matching that flag, so the injection silently no-op'd for every lock file in the repo. Without it, AWF wrote session state to its ephemeral work dir, the "Copy Copilot session state files to logs" step loggedNo session state found, and the parser saw only Copilot's plain-textprocess-*.log— producing "Log format not recognized as Copilot JSON array or JSONL." The injection now anchors on the currentawf --configinvocation, retains the legacy--audit-dirpattern, and emits a real warning instead of the misleading "already present (or no awf invocation found)" message.Verification
Run 36183640892 succeeded on real KVM hardware. The Copilot log parser reported “Copilot log parsed successfully”, and
agent_output.jsonis a valid Copilot output envelope. All four smoke checks passed:{"check":"microvm-run","status":"PASS","detail":"awf exited 0"} {"check":"guest-assertions","status":"PASS","detail":"guest confirmed /workspace export, --container-workdir, and env passthrough"} {"check":"workspace-copy-back","status":"PASS","detail":"guest write reached the host workspace"} {"check":"copilot-inference","status":"PASS","detail":"Copilot responded through the API proxy"}The downloadable
nvx-smoke-evidenceartifact contains the actual Copilot proof response (NVX-COPILOT-PROOF) and inner API-proxy JSONL showing a successful Copilot/chat/completionsrequest (status: 200,result: ok, modelclaude-sonnet-5, 80 input and 16 output tokens). This keeps proxy telemetry separate from Copilot conversation output while making both available in the run artifacts.Session-state fix
Verified against run 36187332444.
sandbox/agent/logs/session-state/<id>/events.jsonlis now present in theagentartifact, the "not recognized" message no longer appears in the run, and replaying gh-aw's ownparse_copilot_log.cjsagainst that file locally yields 24 parsed entries (previously 0). The step summary now renders a real agentic conversation.Because the injection had broken repo-wide, recompilation applies the flag to all 70 lock files (one line each).
Unrelated failure found while verifying
Runs 36187332444 and 36188425539 failed all four checks in a single cascade from a pre-existing TOCTOU race in
src/nvx/confinement.ts, which this branch does not modify. Tracked in #9012 and intentionally left out of this PR.Known follow-up (not in this PR)
Earlier runs showed the agent could call
noop(pass) despite failed checks. The final run's reported checks match the recorded scenario file; earlier false-pass behavior remains a possible follow-up for stricter agent validation.The workflow was recompiled after changes with
gh aw compile smoke-nvx-copilotandpostprocess-smoke-workflows.ts; the final smoke run validates the compiled workflow on real hardware.