fix: four harness bugs (profile mutation, launch expectations, result folding, boot wait)

[markg-github]

Four pre-existing harness bugs, none of which need any knowledge of ID blocks to review. Split out so they can merge ahead of, and independently of, the ID block work in #247.

+59/-6 across 4 files, one commit per fix.

---

1. cli.py — re-read the VM profile from context between steps

Why a test would need to change the profile mid-run. The vm_profile a test module declares is static — it is evaluated before the run begins. Any launch parameter that can only be computed during the run has no other way in than ctx.profile.

Examples: an ID block cannot be built until the guest has been measured, and the guest cannot be measured until the run is underway, so it is necessarily compute-then-relaunch. Likewise a policy derived from observed platform state (check whether SMT is active, then set the policy bit to match), a host_data value derived from something measured earlier, or a guest image chosen by a preceding step.

The rule this establishes: static launch configuration belongs in vm_profile; anything a step computes belongs in ctx.profile.

The bug. execute_test assigned the loop-local profile onto ctx.profile at the top of every iteration. A callable step that replaces the profile — dataclasses.replace on a frozen VMProfile returns a new object, so in-place mutation is impossible — had that replacement overwritten before the next step ran. The second channel silently did not work, and any handler reconfiguring the guest for later steps was a no-op.

Does this generalise to other context fields? Not today, and the reason is worth recording. profile is the only StepContext field with step → harness dataflow. launch has the identical overwrite shape (ctx.launch = launch each iteration) but flows the other way — the harness produces it and steps consume it — so overwriting is correct there. step_results is a list mutated in place, so assignment never clobbers it. The remaining fields are static per test.

The fix is therefore correctly scoped, but the trap is general: any future context field intended to carry a value out of a step will hit exactly this bug.

2. runner.py — honour expected_result when a launch raises

vm_launch has two distinct failure modes, and only one of them raises:

Failure mode	Signal	Old handling
QEMU exits immediately	raises VMLaunchError	error, unconditionally
QEMU runs, guest never boots	returns ok=False	exit_code=1, expected_result honoured

The ok=False path was already correct. So a step declaring expected_result="exit_code:1" passed or errored depending on how the launch failed — something a test author can neither control nor predict.

Concretely: a policy the platform cannot satisfy is rejected at SNP_LAUNCH_START before guest memory is loaded, so QEMU dies instantly and raises → error. A launch rejected later, after QEMU is already running, returns ok=False → pass. Two tests asserting the same thing, disagreeing purely because of rejection timing.

This aligns the raise path with the return path that already worked. Steps that declare no expectation still default to exit_code:0 and still report error, so existing behavior is unchanged.

3. cli.py — stop a later test result masking a worse earlier one

if tr.result != "pass":
    overall = tr.result

Every non-passing test overwrites overall, so the certification reports the result of the last non-passing test rather than the worst one:

test1 error  ->  overall = "error"
test2 fail   ->  overall = "fail"     <- the error is now lost
test3 pass   ->  unchanged
                 final: "fail"

Why it matters. fail and error prescribe different responses. fail means the platform failed a check — investigate the platform. error means the harness could not run the check at all (module import failure, timeout, crash) — investigate the harness. Reporting fail when an error occurred sends the reader down the wrong path.

Why it is subtle, and why it has survived. Today it changes only the label, never the verdict: both states are non-passing, so the certification does not pass either way, and _highest_certified_level is unaffected because it inspects per-test results rather than this fold.

It becomes actively harmful the moment a non-passing state exists that should not dominate. The stacked PR adds skip: a skipped test running after a failed one would overwrite fail with skip, converting "something failed" into "something was not assessed" — which reads as benign and is not.

Fold with an explicit severity ordering instead. One point worth a reviewer's opinion: the ordering pass < skip < fail < error ranks error above fail, following the usual test-framework convention that could-not-run outranks failed. That is a judgment call, not a derivation.

4. vm_profile.py / guest_vsock.py — stop waiting for a guest whose QEMU has exited

QEMU liveness is checked exactly once, at wait_ready_seconds. wait_for_guest then polls vsock until vsock_boot_timeout (180s) holding no reference to the process. If QEMU exits after that single check, the harness spends the remaining timeout pinging a VM that no longer exists.

Measured on an EPYC 9654, sampling the process at 1 Hz during a launch the firmware rejects:

+14.6s ..  +17.2s   QEMU alive
+17.2s ..           gone
step ran +13.8s .. +195.9s

QEMU lived 2.6s; the step took 182.1s. Reproduced identically under two BIOS versions.

The diagnostics matter more than the 179 seconds. Firmware had already reported the cause — SNP_LAUNCH_FINISH ret=-5 fw_error=11 'Bad measurement' — and the harness discarded it in favour of Vsock agent on CID ... not ready after 180.0s. A step asserting exit_code:1 is satisfied by that timeout just as well as by a real rejection, so such a test cannot fail for the right reason. The message now carries QEMU's exit code and stderr tail.

process defaults to None, preserving behavior for any caller that does not pass it.

---

Verification

Unit, local: the severity fold across six orderings; the liveness check with a fake process (dies at 2s → aborts at 2.0s; no handle → full timeout and the original message, i.e. backward compatible).

On hardware, this branch alone (EPYC 9654, Genoa) — the pre-existing c3_0_0_0 attestation-test against four guest images, since the point of a fixes-only PR is that existing behavior still works:

Image	Result	Secs
centos-10	pass	16
fedora-41	pass	19
ubuntu-25.10	pass	19
ubuntu-26.04	pass	19

11/11 steps passing on each — real guest launch, report retrieval, KDS certificate and VCEK fetch, chain verification, signature/TCB verification, measurement comparison. certified_level: 3.0.0-0.

Not tested on hardware: level 3.0.0-1 (snphost-config-commit), which issues SNP_COMMIT. That was skipped deliberately on a machine mid-way through a firmware-stepping exercise rather than because of anything in this PR.

Fixes 1, 2 and 4 are exercised end-to-end by the ID block test in the stacked PR: with all four in place a suite that took ~200s takes 22s, and the negative-launch steps assert real firmware rejections instead of timeouts.

Follow-up not included

/tmp/guest-error.log is a single fixed path clobbered by every launch; recovering the evidence in fix 4 required sampling it externally at 1 Hz. Writing it per-launch into ctx.artifact_dir would make this diagnosable from artifacts alone.

[Copilot]

Pull request overview

This PR fixes four pre-existing issues in the SEV verification harness to make multi-step VM runs more reliable: it preserves step-driven profile updates, correctly applies expected_result to launch exceptions, folds certification status using a severity ordering, and aborts guest-boot waits early when QEMU exits.

Changes:

• cli.py: Preserve step-updated ctx.profile across steps; fold certification outcome via severity ordering.
• runner.py: Treat VMLaunchError as a checkable launch outcome by honoring expected_result.
• vm_profile.py / guest_vsock.py: Pass the QEMU process handle to vsock wait logic to stop waiting when QEMU has already exited, and surface QEMU stderr context.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
sev_verify/cli.py	Re-reads ctx.profile each step; adds severity-based folding for overall certification result.
sev_verify/runner.py	Honors expected_result even when vm_launch raises VMLaunchError.
sev_verify/vm_profile.py	Forwards the QEMU process handle into boot readiness checks and appends QEMU stderr tail on exit.
sev_verify/guest_vsock.py	Adds optional process-aware early abort for guest readiness waits.

Suppressed comments (1)

sev_verify/guest_vsock.py:185

• Same typing consistency issue as wait_for_guest: annotate process as subprocess.Popen[bytes] | None to match VMLaunchResult.process and the actual subprocess.Popen usage in vm_profile.

def check_guest_ready(
    profile: VMProfile,
    *,
    timeout: float | None = None,
    poll_interval: float = 2.0,
    process: subprocess.Popen | None = None,
) -> tuple[bool, str]:

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

sev_verify/vm_profile.py:244

• process is dereferenced unconditionally (process.poll()), but check_guest_ready() accepts process=None and VMLaunchResult.process is typed as optional in context. If process can be None in this code path, this will raise AttributeError and mask the real boot failure. Consider guarding process explicitly (e.g., if process is not None and process.poll() is not None:) before accessing it.

            booted, boot_error = check_guest_ready(self, process=process)
            if not booted:
                ok = False
                message = f"Guest did not boot: {boot_error}"
                if process.poll() is not None:
                    # QEMU's own diagnosis is far more useful than "agent not
                    # ready" — for a rejected launch it names the firmware
                    # error, e.g. "SNP_LAUNCH_FINISH ... 'Bad measurement'".
                    stderr_tail = _read_guest_errors(self.guest_error_log).strip()
                    if stderr_tail:
                        message = f"{message}\nQEMU stderr:\n{stderr_tail}"

sev_verify/guest_vsock.py:146

• subprocess.Popen[bytes] in annotations can be a runtime TypeError (“type 'Popen' is not subscriptable”) unless this module uses postponed evaluation of annotations (e.g., from __future__ import annotations) or otherwise avoids evaluating the subscript at runtime. To keep this safe across runtime configurations, consider changing the annotation to an unsubscripted subprocess.Popen | None, or using a typing alias that won’t be evaluated at runtime.

    process: subprocess.Popen[bytes] | None = None,

sev_verify/runner.py:190

• _check_expected_values() documents checking exit_code and stdout, but this path passes str(exc) as the “stdout” argument while also writing it to stderr. That makes expected_result=stdout_contains:... effectively match against an error message, which is inconsistent with the documented semantics and with where the output is stored in StepResult. Consider either (a) extending _check_expected_values to check stderr (e.g., a stderr_contains kind or a generalized “output_contains”), or (b) storing the exception message in stdout when using stdout_contains expectations so the behavior is internally consistent.

        # A launch that fails hard is still a launch outcome, so honour the
        # step's expected_result. Reporting "error" unconditionally made it
        # impossible to write a step that expects a launch to be refused —
        # QEMU exiting immediately raises rather than returning ok=False.
        passed = _check_expected_values(step, 1, str(exc))
        return (
            StepResult(
                step=step,
                result="pass" if passed else "error",
                exit_code=1,
                stderr=str(exc),
                duration_ms=duration_ms,
            ),

sev_verify/cli.py:509

• The comparison on line 509 is hard to read and likely to violate typical line-length linting. Consider splitting into named locals (e.g., current_sev, candidate_sev) to improve readability and ease future edits (such as changing how unknown results are handled).

def _worse_result(current: str, candidate: str) -> str:
    """Return whichever of the two results is more severe."""
    unknown_severity = max(_RESULT_SEVERITY.values()) + 1
    if _RESULT_SEVERITY.get(candidate, unknown_severity) > _RESULT_SEVERITY.get(current, unknown_severity):
        return candidate
    return current

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

sev_verify/runner.py:182

• VMLaunchError includes QEMU’s real exit code (see vm_profile.py:218-221), but this handler hard-codes 1 both for the expected_result check and the returned StepResult.exit_code. That can misreport failures and makes expected_result=exit_code:<actual> impossible for hard-fail launches. Consider extracting the real code from the exception message and using it for both _check_expected_values and StepResult.exit_code (falling back to 1 if parsing fails).

        passed = _check_expected_values(step, 1, str(exc))

[amd-aliem]

lgtm, you might need to re-run one of the image build CI tests

[markg-github]

Thank you, @amd-aliem