extract supervisor interface layer, enforce agent/supervisor import boundary (#986)

Author: odesenfans

.github/workflows/test-using-pytest.yml

@@ -49,6 +49,14 @@ jobs:
         run: |
           hatch run linting:typing
 
+      - name: Check the agent/supervisor/contract import boundary
+        # Runs in the testing env (import-linter needs the project importable).
+        # sudo so it shares the root testing env with the proto-check / unit-test
+        # steps below, matching their comment about env races.
+        run: |
+          sudo python3 -m pip install --upgrade --ignore-installed hatch hatch-vcs coverage "virtualenv<21"
+          sudo hatch run testing:imports
+
       - name: Download and build required files for running tests. Copied from packaging/Makefile.
         run: |
           sudo useradd jailman

.gitignore

@@ -31,3 +31,4 @@ node_modules
 /kernels/linux-*.tar
 /kernels/linux-*.tar.sign
 /.worktrees/
+.import_linter_cache/

docs/plans/2026-06-19-agent-supervisor-boundary-design.md

@@ -0,0 +1,139 @@
+# The code move (PR-3: physical relocation, behavior-neutral)
+
+**Status:** Design / approved scope
+**Date:** 2026-06-19
+**Owner:** Olivier Desenfans
+**Repo:** `aleph-im/aleph-vm`
+**Parent design:** `docs/plans/2026-05-28-aleph-vm-architecture-backport-design.md`
+**Predecessors:** `2026-06-19-agent-supervisor-boundary-design.md` (PR-1),
+`2026-06-19-controller-split-by-concern-design.md` (PR-2)
+
+## 1. Context
+
+After PR-1 (contract layer + enforced boundary) and PR-2 (controllers no longer
+carry an Aleph-aware personality, views catch only `SupervisorError`), the code
+is in the right *layers* but not yet under the right *names and directories*.
+The agent still lives in a package called `orchestrator/`; the supervisor-owned
+running controller still lives in a neutral-looking top-level `controllers/`.
+
+PR-3 performs the physical move so the directory tree matches the architecture.
+It is purely mechanical: renames, file moves, and import updates. No logic
+changes, so it is gated on "moves only + green suite", like PR-1.
+
+This is safe to do last precisely because PR-1 and PR-2 already separated the
+concerns; moving files around now cannot smuggle in a coupling, because the
+import-linter would reject it.
+
+## 2. Moves
+
+### 2.1 `orchestrator/` -> `agent/`
+
+The agent package gets its real name. This is the bulk of the diff (every
+internal import of `aleph.vm.orchestrator...` updates), but it is mechanical.
+
+Packaging follow-through (service *names* are unchanged, only module targets):
+
+- `pyproject.toml`: `scripts.aleph-vm = "aleph.vm.agent.cli:main"`.
+- `packaging/.../aleph-vm-agent.service`: `ExecStart=python3 -m aleph.vm.agent
+  --print-settings`.
+- `aleph/vm/agent/__main__.py` (moved) keeps the same CLI surface.
+
+### 2.2 Agent-side downloader -> `agent/`
+
+The message->spec downloader extracted in PR-2 moves into `agent/` next to
+`translate.py` / `qemu_build.py` (which PR-1 already placed agent-side).
+
+### 2.3 `controllers/` -> `supervisor/controllers/`
+
+After PR-2, `controllers/` holds only the running controller, the runtime
+resource holder (`from_spec`), and management (`QemuVmClient`, `backup`,
+`snapshots`). It is the supervisor's execution worker.
+
+**Decision (2026-06-19): move it to `supervisor/controllers/`.** The controller
+is spawned by the supervisor (via its `SystemDManager`) and is logically the
+supervisor's execution worker; nesting reflects that, keeps the supervisor
+subtree together, and lines up with parent-design Phase 2, where the Python
+hypervisor and its controllers are swapped for Rust as one unit. Use `git mv` so
+rename detection keeps the diff reviewable.
+
+Entry-point follow-through:
+
+- `aleph-vm-controller@.service`: `ExecStart=/usr/bin/python3 -m
+  aleph.vm.supervisor.controllers --config=/var/lib/aleph/vm/%i-controller.json`.
+  The service *name* and the `%i-controller.json` config path are unchanged, so
+  operators and the on-disk config contract are unaffected; only the module
+  target moves. The unit file and the module ship in the same `.deb`, so the
+  rename is internally consistent within any version (already-running controller
+  instances keep running on their old invocation until their next start, which
+  uses the new, consistent unit).
+- `aleph/vm/supervisor/controllers/__main__.py` (moved) keeps the same
+  `--config` CLI surface.
+
+### 2.4 `configuration.py` (already in `contract/` from PR-1)
+
+`controllers/configuration.py`, the on-disk config-file schema (agent
+`qemu_build` writes it, the controller reads it, `local.py` removes it), is
+**already moved to `contract/configuration.py` in PR-1**. It had to be: once
+controllers nests under `supervisor/` here, leaving the schema in
+`supervisor/controllers/` would make agent-side `qemu_build` import
+`supervisor/controllers`, a forbidden `agent -> supervisor` edge. PR-1 does the
+move (its deps are clean: stdlib, pydantic, foundation), so PR-3 has nothing to
+do for it beyond confirming the import paths are already `contract.*`.
+
+## 3. Enforcement and cleanup
+
+- Update the import-linter config to the final package names. The only remaining
+  documented residual is `agent -> {pool, models}` (the `VmExecution`/`VmPool`
+  cleave, a separate adjacent effort). Every other ignore entry from PR-1 is gone
+  by now (PR-2 removed the controller ones).
+- Optionally leave thin re-export shims at the old `aleph.vm.orchestrator.*`
+  paths for one release if any out-of-repo tooling imports them; otherwise a hard
+  rename. Recommend a hard rename inside the repo (update all call sites) and
+  shims only if a concrete external importer is found.
+
+## 4. Testing strategy
+
+- Behavior-neutral: the existing suite is the oracle, run it green (known
+  env-only exceptions excepted).
+- **Launch smoke test**: confirm all three entry points still import and start:
+  `python -m aleph.vm.agent --print-settings`, `python -m aleph.vm.supervisor`,
+  and the controller `python -m aleph.vm.supervisor.controllers --config=<sample>`.
+- `mypy` baseline unchanged; import-linter passes under the final names.
+- Grep for stale `aleph.vm.orchestrator` references across `packaging/`, docs,
+  CI workflows, and tests; none should remain (or only intentional shims).
+
+## 5. Risks
+
+| Risk | Mitigation |
+| ---- | ---------- |
+| A missed `orchestrator` reference in packaging/CI breaks a service at deploy | §4 grep sweep across packaging, systemd units, CI, and the Makefile; launch smoke test. |
+| The rename collides badly with in-flight branches | Land PR-1/PR-2 first; do the rename in one commit; rebase dependents once, immediately. |
+| External tooling imports `aleph.vm.orchestrator` | Search for real importers; add re-export shims only if found, with a deprecation note. |
+| Reviewers cannot see logic vs move in a huge diff | Keep PR-3 strictly mechanical (no logic edits); use `git mv` so rename detection keeps the diff reviewable. |
+
+## 6. Sequence recap and what remains after
+
+The sequence:
+
+0. **PR-0** (behavior-neutral): rename `vm_hash` -> `vm_id` on the supervisor-side
+   objects (`VmExecution`, `VmPool`, `local.py`) and the agent call sites reading
+   that attribute. See `2026-06-19-supervisor-vm-id-rename-design.md`.
+1. **PR-1** (behavior-neutral): contract layer, misfiled agent code out of
+   `supervisor/`, three back-references fixed, import-linter.
+2. **PR-2** (behavior-affecting): split the `Resources` dual personality, finish
+   the wire-error vocabulary, remove the two `controllers` residuals.
+3. **PR-3** (behavior-neutral): `orchestrator/` -> `agent/`, `controllers/` ->
+   `supervisor/controllers/`, final import-linter names. (`configuration.py` ->
+   `contract/` already happened in PR-1.)
+
+After PR-3 the only remaining cross-boundary coupling is `agent -> {pool,
+models}`: the `VmExecution`/`VmPool` cleave from the parent design §4, which is a
+separate effort and not part of this sequence. The cosmetic work of this sequence
+is then done; the next architectural milestone is that cleave, after which the
+Python hypervisor can be swapped for the Rust one (parent design Phase 2).
+
+## 7. Next step
+
+Implementation plan (writing-plans) per PR, authored when its predecessor lands
+(PR-2's plan after PR-1 merges, PR-3's after PR-2 merges), since each plan's
+exact import edits depend on the prior PR's final state.

docs/plans/2026-06-19-agent-supervisor-code-move-design.md

@@ -0,0 +1,161 @@
+# Controller split by concern (PR-2: behavior-affecting)
+
+**Status:** Design / approved scope
+**Date:** 2026-06-19
+**Owner:** Olivier Desenfans
+**Repo:** `aleph-im/aleph-vm`
+**Parent design:** `docs/plans/2026-05-28-aleph-vm-architecture-backport-design.md` (§4, A.6)
+**Predecessor:** `docs/plans/2026-06-19-agent-supervisor-boundary-design.md` (PR-1)
+
+## 1. Context
+
+PR-1 establishes the `contract` layer and an import-enforced boundary, but it
+leaves `controllers/` as a *shared* base layer because two pieces of it are used
+on both sides. PR-1 records those as documented residuals:
+
+- `orchestrator -> controllers` for the `Resources` classes, and
+- `orchestrator -> controllers` for the controller exception types caught by the
+  HTTP views.
+
+PR-2 removes both residuals. It is the smaller half of what the parent design
+called the §4 cleave, because the hardest piece (moving volume download out of
+`controllers.setup()`) **already happened** during the spec work: `translate.py`
+(agent) calls `resources.download_all()` and resolves every path before building
+the `CreateVmSpec`; the running controller no longer downloads. What remains is
+to stop the controller code from carrying a second, Aleph-aware personality, and
+to finish the wire-error vocabulary so the views stop reaching into controller
+internals.
+
+PR-2 changes behavior (it splits a class and changes which exception types the
+views catch), so it is gated on tests rather than on "moves only".
+
+**Out of scope (still): the `VmExecution` / `VmPool` cleave.** PR-2 does not
+split the god-objects. The controller split does not require it: download is
+already agent-side and the spec path is in place. The `orchestrator ->
+{pool, models}` residual from PR-1 stays until that separate, adjacent effort.
+
+## 2. The two entanglements PR-2 removes
+
+### 2.1 The `Resources` dual personality
+
+`AlephQemuResources` (and the firecracker `AlephProgramResources` /
+`AlephFirecrackerResources`, plus `AlephQemuConfidentialResources`) are
+constructed two different ways:
+
+- **Agent / download path**: `AlephQemuResources(message_content, namespace)`
+  followed by `download_all()`. This reads the Aleph message, downloads runtime
+  and volumes, creates writable qcow2 images, and feeds `translate.py`'s
+  `CreateVmSpec`. It is Aleph-aware (holds `message_content`).
+- **Supervisor / runtime path**: `AlephQemuResources.from_spec(spec, namespace)`.
+  No download, `message_content=None`; it just exposes the attribute surface the
+  running controller and pool read (`rootfs_path`, `volumes`, `gpus`,
+  `kernel_image_path`). `from_spec` even does a local
+  `from aleph.vm.supervisor.types import DiskRole` to read the spec.
+
+One class, two lives. The download half is agent; the runtime half is supervisor.
+
+**Target:** two types.
+
+- An **agent-side downloader** owns `message_content`, `download_runtime`,
+  `download_volumes`, `download_all`, and the writable-image creation. It lives
+  agent-side and its product is the `CreateVmSpec` (resolved paths). It replaces
+  the `AlephQemuResources(message)` use in `translate.py`.
+- A **supervisor-side runtime holder** is the message-free
+  `VmResources`/`from_spec` shape the controller reads. It stays controller-side
+  (post-PR-3, supervisor-side). After PR-1 it reads `DiskRole` from `contract`,
+  not via a local `supervisor.types` import.
+
+The shared `controllers/resources.py::VmResources` attribute surface stays as the
+common base for the runtime holder. The downloader composes or subclasses it;
+choice deferred to the plan (see §5).
+
+### 2.2 The wire-error vocabulary (parent A.6)
+
+The views are in a hybrid state today: `orchestrator/views/__init__.py` imports
+controller and hypervisor exception types (`ResourceDownloadError`,
+`VmSetupError`, `FileTooLargeError`, `MicroVMFailedInitError`,
+`HostNotFoundError`, `InsufficientResourcesError`) and lists them in the
+`vm_creation_exceptions` catch tuples, *and* it already catches `SupervisorError`
+/ `InternalSupervisorError` from the boundary. `operator.py` similarly imports
+`controllers.qemu.backup` types.
+
+The supervisor side already has the mapping: `supervisor/error_mapping.py` (split
+out in PR-1) translates every internal backend exception to the closed
+`SupervisorError` set via `translating_errors()`.
+
+**Target:** every Supervisor boundary method wraps its work in
+`translating_errors()` (audit that the create / operate / backup paths the views
+exercise are covered), so the boundary only ever raises `SupervisorError`. The
+views then:
+
+- drop the `from aleph.vm.controllers...` and
+  `from aleph.vm.hypervisors...` exception imports,
+- catch `contract.errors.SupervisorError` (optionally branching on `.code` /
+  `ErrorCode` for the few distinct HTTP statuses), and
+- map `ErrorCode -> HTTP status` in one small helper.
+
+`operator.py`'s direct use of `controllers.qemu.backup` / `QemuVmClient` is the
+same shape: those operations move behind boundary methods that raise
+`SupervisorError`, and the view stops importing controller types. (If any backup
+RPC is not yet on the boundary, that surfaces as a plan task.)
+
+## 3. Result
+
+After PR-2:
+
+- `controllers/` no longer carries Aleph-aware download personalities; it holds
+  the runtime holder + the running controller + management (`QemuVmClient`,
+  `backup`, `snapshots`) only.
+- The agent imports its own downloader and catches only `SupervisorError`. The
+  two `orchestrator -> controllers` residuals from PR-1 are deleted from the
+  import-linter ignore list, and the linter is tightened to forbid them.
+- `controllers/` is now unambiguously a supervisor-owned package (its
+  config-file schema, `configuration.py`, already moved to `contract/` in PR-1).
+  This is what makes PR-3's physical move into `supervisor/controllers/`
+  mechanical.
+
+The only remaining documented residual is `orchestrator -> {pool, models}`,
+owned by the separate `VmExecution`/`VmPool` cleave.
+
+## 4. Testing strategy
+
+Behavior changes, so tests lead:
+
+- **Error-mapping tests** (the riskiest surface): for each internal backend
+  exception, assert `translate_exception` yields the right `SupervisorError` /
+  `ErrorCode`, and that the view maps that code to the same HTTP status the
+  current code returns. Lock the create-failure HTTP contract (503 for
+  insufficient resources, the 4xx/5xx for setup/download/file-too-large) with
+  view-level tests before changing the catch sites, so the refactor is provably
+  status-preserving.
+- **Downloader / runtime-holder split**: keep the existing
+  `translate` and `test_qemu_instance` coverage green; the downloader must
+  produce byte-identical spec paths, and `from_spec` must build the same runtime
+  holder. Add a unit test that the runtime holder has no `message_content` /
+  download surface.
+- **Confidential**: `AlephQemuConfidentialResources` follows the same split;
+  keep the SEV-path tests (and the testnet #27 SEV run before merge, as with the
+  confidential-init work).
+- Full supervisor suite green (known env-only exceptions excepted); `mypy`
+  baseline unchanged; import-linter passes with the two residuals removed.
+
+## 5. Open questions (resolved in the implementation plan)
+
+- **Downloader vs runtime holder relationship**: does the agent downloader
+  subclass `VmResources` (sharing the attribute surface) or compose a separate
+  type that emits spec fields? Composition keeps the agent free of the
+  controller base class; subclassing is less code.
+- **`make_writable_volume` placement**: creating the writable qcow2 runs
+  `qemu-img` on the host, which is arguably supervisor/infra work, but today it
+  runs in the agent download phase. Keep it in the downloader (status quo,
+  behavior-neutral) or move it behind the boundary (larger, defer)? Recommend
+  keeping it in the downloader for PR-2 and noting it for the cleave.
+- **Backup/QMP coverage on the boundary**: confirm `operator.py`'s backup and
+  `QemuVmClient` uses already have boundary methods that raise `SupervisorError`;
+  any gap is a prerequisite task inside PR-2 (or a thin precursor PR).
+
+## 6. Next step
+
+Implementation plan (writing-plans) covering the `Resources` split, the
+boundary `translating_errors()` audit, the view error-mapping helper, the
+residual-removal in the import-linter config, and the test checklist above.