fix: make validating VM lifecycle fail closed

Author: abrichr

.github/actionlint.yaml

@@ -0,0 +1,3 @@
+self-hosted-runner:
+  labels:
+    - openadapt-parallels

.github/workflows/validate-claims.yml

@@ -33,7 +33,9 @@ on:
 
 concurrency:
   group: validate-claims-${{ github.workflow }}-${{ github.ref }}
-  cancel-in-progress: true
+  # A pull-request structure check is safe to supersede. A physical Parallels
+  # run is not: cancellation could interrupt its exact-snapshot cleanup.
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 permissions:
   contents: read
@@ -90,7 +92,7 @@ jobs:
   # These selected tests require macOS, local Parallels, a running configured
   # Windows VM, and a known base snapshot. They cannot run on ubuntu-latest or
   # against an unrelated hosted VM endpoint. A repository owner enables the
-  # job only after a self-hosted runner has all three labels below and the two
+  # job only after a self-hosted runner has all four labels below and the two
   # required repository variables are configured.
   #
   # A skipped job means "not proven this run" and produces no report. A green
@@ -101,8 +103,11 @@ jobs:
     if: >-
       ${{ vars.OPENADAPT_PARALLELS_VALIDATION_ENABLED == 'true' &&
           (github.event_name == 'schedule' || inputs.run_infra == true) }}
-    runs-on: [self-hosted, macos, arm64]
+    runs-on: [self-hosted, macos, arm64, openadapt-parallels]
     timeout-minutes: 90
+    concurrency:
+      group: openadapt-parallels-validation
+      cancel-in-progress: false
     env:
       OAFLOW_PARALLELS_VM_UUID: ${{ vars.OAFLOW_PARALLELS_VM_UUID }}
       OAFLOW_PARALLELS_BASE_SNAPSHOT_ID: ${{ vars.OAFLOW_PARALLELS_BASE_SNAPSHOT_ID }}
@@ -135,15 +140,29 @@ jobs:
       - name: Install validation dependencies
         run: python -m pip install -e .[dev]
 
+      - name: Prove configured VM and base are current before mutation
+        run: |
+          python -c '
+          import os
+          from openadapt_flow.backends.parallels_vm import ParallelsVM
+          vm = ParallelsVM(os.environ["OAFLOW_PARALLELS_VM_UUID"])
+          vm.require_host_free_space(
+              storage_path=os.environ["OAFLOW_PARALLELS_STORAGE_PATH"]
+          )
+          vm.require_current_snapshot(
+              os.environ["OAFLOW_PARALLELS_BASE_SNAPSHOT_ID"]
+          )
+          '
+
       - name: Run scoped desktop and pixel evidence
         env:
           OAFLOW_PARALLELS_E2E: "1"
           OAFLOW_CITRIX_PIXEL_E2E: "1"
         run: |
           mkdir -p runs
           python -m pytest -q \
-            tests/e2e/test_parallels_desktop_e2e.py \
             tests/e2e/test_citrix_pixel_e2e.py \
+            tests/e2e/test_parallels_desktop_e2e.py \
             --junitxml=runs/validating-junit.xml --basetemp=runs/ci
 
       - name: Require every selected evidence file to pass

docs/desktop_windows_runbook.md

@@ -2,8 +2,8 @@
 
 How to bring the **desktop** path to parity with the **web** path — record →
 compile → replay with the structural (UIA) + vision ladder, identity, and
-effect verification — and run the **snapshot-safe live proof** in one pass on a
-machine with the Parallels Windows 11 VM.
+effect verification — and run the **snapshot-safe live proof** in three counted
+trials on a machine with the Parallels Windows 11 VM.
 
 This is the operator runbook. The design/rationale lives in
 `openadapt_flow/backends/win_agent/README.md` (the agent) and
@@ -126,25 +126,34 @@ fire.
 
 ---
 
-## 4. Run the snapshot-safe live proof (one pass)
+## 4. Run the snapshot-safe live proof (three counted trials)
 
 The opt-in e2e (`tests/e2e/test_parallels_desktop_e2e.py`) does the whole loop
-against the built-in Windows **Calculator** (deterministic, no PHI): snapshot →
-ensure VM up → launch the agent in session 1 → record→compile→replay via
-`WindowsBackend` → assert the UIA structural rung fires and the run completes →
-**revert to the snapshot**.
+against the deterministic Patient Notes fixture: prove one exact configured
+base snapshot is current → revert to that base → ensure the VM is running →
+create one exact trial snapshot → launch the typed agent in session 1 →
+record→compile→replay via `WindowsBackend` → assert the UIA structural rung and
+independent database oracle → restore and verify the configured base → delete
+only the exact trial snapshot → suspend and verify the VM. It repeats this
+contract three times.
 
 ```bash
 # On the Mac with the Parallels VM:
-OAFLOW_PARALLELS_E2E=1 pytest -q tests/e2e/test_parallels_desktop_e2e.py
-# Optional: OAFLOW_PARALLELS_VM_UUID='{...}' to target a different VM.
+export OAFLOW_PARALLELS_E2E=1
+export OAFLOW_PARALLELS_VM_UUID='{exact-vm-uuid}'
+export OAFLOW_PARALLELS_BASE_SNAPSHOT_ID='{exact-base-snapshot-uuid}'
+export OAFLOW_WINDOWS_UIA_CANDIDATE_COMMIT='40-character-git-commit'
+export OAFLOW_PARALLELS_STORAGE_PATH="$PWD"
+pytest -q tests/e2e/test_parallels_desktop_e2e.py
 ```
 
-**Snapshot safety, guaranteed:** the test takes a **fresh** snapshot before it
-touches the guest and reverts to it in a `finally` block. It **never deletes**
-the VM or any snapshot, and it is **skipped entirely** unless
-`OAFLOW_PARALLELS_E2E=1`. Always confirm your VM has a known-good snapshot before
-running anything against it.
+**Snapshot safety contract:** the test refuses before mutation unless the exact
+configured base exists and is current. It retains each created trial snapshot
+ID in memory. Cleanup restores and verifies the base before it deletes only
+that exact ID. A restore, verification, deletion, evidence-write, or final
+suspend failure rejects the trial. The harness never uses a snapshot name,
+wildcard, child-recursive delete, or VM delete operation. It is skipped entirely
+unless `OAFLOW_PARALLELS_E2E=1`.
 
 ---
 

openadapt_flow/backends/parallels_vm.py

@@ -278,6 +278,29 @@ def list_snapshots(self) -> list[SnapshotInfo]:
                 out.append(SnapshotInfo(snapshot_id=sid, current=bool(star)))
         return out
 
+    def require_current_snapshot(self, snapshot_id: str) -> None:
+        """Prove one exact preserved snapshot exists and is current.
+
+        Qualification callers use this read-only check before they resume,
+        focus, snapshot, deploy, or actuate on a dedicated VM. It prevents a
+        run from preserving and later deleting an unrelated current state.
+        """
+
+        snapshot_pattern = (
+            r"\{[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-"
+            r"[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\}"
+        )
+        if re.fullmatch(snapshot_pattern, snapshot_id) is None:
+            raise ValueError("snapshot_id must be one exact braced UUID")
+        snapshots = self.list_snapshots()
+        match = next(
+            (item for item in snapshots if item.snapshot_id == snapshot_id), None
+        )
+        if match is None:
+            raise ParallelsError("preserved base snapshot is missing")
+        if not match.current:
+            raise ParallelsError("preserved base snapshot is not current")
+
     def revert(self, snapshot_id: str) -> None:
         """Revert to a snapshot (the per-run clean-state reset)."""
         self._run(["snapshot-switch", self.uuid, "-i", snapshot_id])

public-artifacts.json

@@ -71,6 +71,10 @@
       "path": ".github/ISSUE_TEMPLATE/feature_request.yml",
       "sha256": "bb032fd5942477be558870dbf2c6615d258b6600ac2b389c5c4bf7c55b856da6"
     },
+    {
+      "path": ".github/actionlint.yaml",
+      "sha256": "f58f146d55a022c530f1dd35e5da6a2e9ea0cd5cbcac0e6b0e50995a8b45cf20"
+    },
     {
       "path": ".github/dependabot.yml",
       "sha256": "40a11fc00e1010db80d8788ee8c6125b51b14d642be018cddb761ea209f66095"
@@ -121,7 +125,7 @@
     },
     {
       "path": ".github/workflows/validate-claims.yml",
-      "sha256": "75265f62b9606ee7eca54a989f72d09dd7a0f16016e5883fd723a09d825d990b"
+      "sha256": "368accbf68954e8e481fc6d7d36229b9cb70c267ec302e01657aa6537900a9e6"
     },
     {
       "path": ".pre-commit-config.yaml",

tests/e2e/test_citrix_pixel_e2e.py

@@ -34,11 +34,14 @@
     * Requires macOS with Screen-Recording AND Accessibility granted to the
       driving app; SKIPS (never fails, never fabricates) when input cannot be
       delivered — a dropped synthetic click must never look like success.
-    * SNAPSHOT-FIRST, REVERT-AFTER; NEVER deletes the VM or ANY snapshot.
-    * Requires the Parallels VM window to be open + resumable; SKIPS otherwise.
-
-Env overrides: ``OAFLOW_PARALLELS_VM_UUID``, ``OAFLOW_CITRIX_WINDOW_TITLE``
-(default "Windows 11").
+    * Requires one exact preserved base snapshot to be current before mutation.
+    * Creates one exact owned snapshot, restores the base, verifies it, and
+      deletes only that owned snapshot. A cleanup failure fails the proof.
+    * Requires the Parallels VM window to be open + resumable.
+
+Env overrides: ``OAFLOW_PARALLELS_VM_UUID``,
+``OAFLOW_PARALLELS_BASE_SNAPSHOT_ID``, ``OAFLOW_PARALLELS_STORAGE_PATH``, and
+``OAFLOW_CITRIX_WINDOW_TITLE`` (default "Windows 11").
 """
 
 from __future__ import annotations
@@ -63,6 +66,8 @@
 
 _SCRIPT_DIR = Path(__file__).resolve().parents[2] / "scripts" / "desktop"
 WINDOW_TITLE = os.environ.get("OAFLOW_CITRIX_WINDOW_TITLE", "Windows 11")
+BASE_SNAPSHOT_ENV = "OAFLOW_PARALLELS_BASE_SNAPSHOT_ID"
+HOST_STORAGE_PATH_ENV = "OAFLOW_PARALLELS_STORAGE_PATH"
 
 
 # -- environment guards (skip cleanly, never fail spuriously) ----------------
@@ -141,6 +146,52 @@ def _db_get(vm, pid: int) -> dict:
     return json.loads((r.stdout or "null").strip() or "null")
 
 
+def _restore_pixel_vm(
+    vm,
+    *,
+    base_snapshot_id: str,
+    owned_snapshot_id: str | None,
+    vm_touched: bool,
+) -> None:
+    """Restore the exact base, delete only the owned snapshot, and suspend."""
+
+    from openadapt_flow.backends.parallels_vm import ParallelsError
+
+    if owned_snapshot_id is None and not vm_touched:
+        return
+    cleanup_errors: list[Exception] = []
+    if owned_snapshot_id is not None:
+        try:
+            vm.restore_base_and_delete_owned_snapshot(
+                base_snapshot_id=base_snapshot_id,
+                owned_snapshot_id=owned_snapshot_id,
+            )
+        except Exception as exc:  # noqa: BLE001
+            cleanup_errors.append(exc)
+    elif vm_touched:
+        try:
+            vm.revert(base_snapshot_id)
+        except Exception as exc:  # noqa: BLE001
+            cleanup_errors.append(exc)
+    try:
+        state = vm.status()
+        if state in {"running", "paused"}:
+            vm.suspend()
+            state = vm.status()
+        if state != "suspended":
+            raise ParallelsError(
+                f"preserved base did not finish suspended (state={state!r})"
+            )
+        vm.require_current_snapshot(base_snapshot_id)
+    except Exception as exc:  # noqa: BLE001
+        cleanup_errors.append(exc)
+    if cleanup_errors:
+        details = "; ".join(repr(error) for error in cleanup_errors)
+        raise RuntimeError(
+            f"pixel proof failed to restore its exact base: {details}"
+        ) from cleanup_errors[0]
+
+
 # -- the proof ---------------------------------------------------------------
 
 
@@ -151,7 +202,10 @@ def test_citrix_pixel_only_record_replay_identity_verify_halt(tmp_path) -> None:
         record_desktop_demo,
         structural_armed_coverage,
     )
-    from openadapt_flow.backends.parallels_vm import DEFAULT_VM_UUID, ParallelsVM
+    from openadapt_flow.backends.parallels_vm import (
+        DEFAULT_VM_UUID,
+        ParallelsVM,
+    )
     from openadapt_flow.backends.remote_display import (
         RemoteDisplayBackend,
         RemoteDisplayError,
@@ -162,29 +216,46 @@ def test_citrix_pixel_only_record_replay_identity_verify_halt(tmp_path) -> None:
     from openadapt_flow.runtime.effects.onscreen import OnScreenReadbackVerifier
 
     uuid = os.environ.get("OAFLOW_PARALLELS_VM_UUID", DEFAULT_VM_UUID)
+    base_snapshot_id = os.environ.get(BASE_SNAPSHOT_ENV)
+    if not base_snapshot_id:
+        raise RuntimeError(
+            f"{BASE_SNAPSHOT_ENV} is required for snapshot-safe qualification"
+        )
     vm = ParallelsVM(uuid)
-
-    # The VM must already be running (Parallels Standard cannot prlctl-start; a
-    # suspended VM is resumed by the operator or the GUI play button). Skip
-    # cleanly rather than fail if it is not reachable.
-    if vm.status() != "running" or not _guest_ready(vm):
-        pytest.skip("Parallels VM not running / guest tools unreachable")
-
-    backend = RemoteDisplayBackend(
-        owner_substr="Parallels Desktop", title_substr=WINDOW_TITLE
-    )
+    snap_id: str | None = None
+    vm_touched = False
+    active_error: BaseException | None = None
     try:
-        backend.ensure_foreground()
-    except RemoteDisplayError as e:
-        pytest.skip(f"remote-display client window not foregroundable: {e}")
+        storage_path = os.environ.get(HOST_STORAGE_PATH_ENV, os.getcwd())
+        vm.require_host_free_space(storage_path=storage_path)
+        # This read-only check must pass before the first mutation. Reverting
+        # the same current id then binds the live working state to that exact
+        # base before resume, focus, snapshot, deployment, or actuation.
+        vm.require_current_snapshot(base_snapshot_id)
+        vm_touched = True
+        vm.revert(base_snapshot_id)
+        vm.require_current_snapshot(base_snapshot_id)
+        vm.ensure_running()
+        if vm.status() != "running" or not _guest_ready(vm):
+            raise RuntimeError("Parallels VM or guest tools did not become ready")
+
+        backend = RemoteDisplayBackend(
+            owner_substr="Parallels Desktop", title_substr=WINDOW_TITLE
+        )
+        try:
+            backend.ensure_foreground()
+        except RemoteDisplayError as exc:
+            raise RuntimeError(
+                "remote-display client window is not foregroundable"
+            ) from exc
+
+        snap_id = vm.snapshot(
+            f"oaflow-citrix-{int(time.time())}", description="citrix pixel e2e"
+        )
+        # A snapshot on a running VM briefly disturbs guest tools.
+        if not _guest_ready(vm):
+            raise RuntimeError("guest tools did not recover after pixel snapshot")
 
-    # SNAPSHOT FIRST — reverted in finally (never deleted).
-    snap_id = vm.snapshot(
-        f"oaflow-citrix-{int(time.time())}", description="citrix pixel e2e"
-    )
-    # A snapshot on a running VM briefly disturbs guest tools; wait for it back.
-    _guest_ready(vm)
-    try:
         # ---- deploy + launch the stand-in clinical app (pixel target) ------
         _deploy_and_launch(vm, drift="none")
         backend.ensure_foreground()
@@ -323,8 +394,18 @@ def driver(rec) -> None:
         assert not amb_report.success, (
             "run must HALT under render drift rather than click a guessed target"
         )
+    except BaseException as exc:
+        active_error = exc
+        raise
     finally:
         try:
-            vm.revert(snap_id)  # never delete — revert only
-        except Exception as e:  # noqa: BLE001
-            print(f"[citrix] WARNING: revert to {snap_id} failed: {e}")
+            _restore_pixel_vm(
+                vm,
+                base_snapshot_id=base_snapshot_id,
+                owned_snapshot_id=snap_id,
+                vm_touched=vm_touched,
+            )
+        except Exception as cleanup_error:
+            if active_error is None:
+                raise
+            active_error.add_note(f"cleanup error: {cleanup_error!r}")

tests/e2e/test_parallels_desktop_e2e.py

@@ -416,7 +416,13 @@ def test_desktop_record_compile_replay_structural(tmp_path, trial: int) -> None:
         evidence_row["host_free_bytes_before"] = vm.require_host_free_space(
             storage_path=storage_path
         )
+        # Refuse before the first mutation when this dedicated VM is not already
+        # bound to the reviewed base. Reverting that same current id then proves
+        # the live working state starts from the exact preserved snapshot.
+        vm.require_current_snapshot(base_snapshot_id)
         vm_touched = True
+        vm.revert(base_snapshot_id)
+        vm.require_current_snapshot(base_snapshot_id)
         vm.ensure_running()
         # Snapshot before any guest deployment/recording. This id is retained
         # in memory and is the only snapshot the trial may later delete.

tests/test_ci_workflow_contract.py

@@ -151,13 +151,20 @@ def test_validating_refresh_uses_exact_macos_parallels_substrate_and_scope() ->
     start = claims.index("  refresh-validating-evidence:")
     refresh = claims[start:]
 
-    assert "runs-on: [self-hosted, macos, arm64]" in refresh
+    assert "cancel-in-progress: ${{ github.event_name == 'pull_request' }}" in claims
+    assert "runs-on: [self-hosted, macos, arm64, openadapt-parallels]" in refresh
     assert "OPENADAPT_PARALLELS_VALIDATION_ENABLED" in refresh
+    assert "group: openadapt-parallels-validation" in refresh
+    assert "cancel-in-progress: false" in refresh
     assert "runs-on: ubuntu-latest" not in refresh
     assert "oa-vm" not in refresh
     assert "command -v prlctl" in refresh
     assert "OAFLOW_PARALLELS_BASE_SNAPSHOT_ID" in refresh
+    assert "vm.require_current_snapshot" in refresh
     assert "--ci-job validating --junit runs/validating-junit.xml" in refresh
+    assert refresh.index("tests/e2e/test_citrix_pixel_e2e.py") < refresh.index(
+        "tests/e2e/test_parallels_desktop_e2e.py"
+    )
     for path in (
         "tests/e2e/test_parallels_desktop_e2e.py",
         "tests/e2e/test_citrix_pixel_e2e.py",