# Design Principles

> **Conformance:** L0 / L1 / L2 (see `README.md`)
> **Audience:** Human engineers AND AI coding agents.

> **📦 Post-extraction note:** This document was originally written when VManager was the only consumer. Citations like `src/vmanager/exceptions.py` and the L1–L4 layering map below describe VManager's structure as the canonical example; the **base class and category bases now live in `src/mgf/common/exceptions/`** (VManager subclasses them under `vmanager.exceptions` for project-specific concretes). The principles themselves are language-of-code — they apply identically to `mgf.common`, VManager, and any future consumer.

---

## Rules box (read this first)

| ID | Rule |
|---|---|
| **DP-01** | **Layering MUST be enforced.** UI MUST NOT be imported by core. Core MUST NOT import third-party SDKs that have a provider in front of them. CI lints this. |
| **DP-02** | Every third-party I/O dependency (DB, hypervisor, cloud, ML model, OS process, hardware) **MUST sit behind an abstract base class** with at least one production and one mock implementation. |
| **DP-03** | Domain types passed between modules **MUST be frozen** (frozen dataclass / `pydantic` `model_config = ConfigDict(frozen=True)`). Bare strings or dicts MUST NOT cross a module boundary as identity. |
| **DP-04** | Every public function **MUST be type-annotated**. `mypy --strict` MUST pass on `src/`. No `Any` without a justification comment. |
| **DP-05** | Tests **MUST mirror `src/` 1:1** and **MUST run with no production credentials, no network, no real third-party daemon**. Use mocks via the provider ABC. |
| **DP-06** | **Security, reliability, performance, and error reporting are first-class concerns**, not afterthoughts. Every PR description MUST address all four when touching new surface. |
| **DP-07** | Rich inline documentation: every public class/function **MUST have a docstring** explaining *intent and invariants*. Why-comments inline are encouraged; what-comments are forbidden. |
| **DP-08** | Code **SHOULD** be replaceable. For each major component (UI toolkit, hypervisor, DB, etc.) the doc near it MUST describe the swap recipe. |
| **DP-09** | Cancellation **MUST be explicit**. Long-running ops take an explicit `CancellationToken`-like parameter; never a thread-global. |
| **DP-10** | **No silent fallbacks.** If a fallback path exists (e.g., keyring → encrypted file), the active backend MUST be queryable and SHOULD be logged at startup. |
| **DP-11** | **Async tasks MUST have an owner.** Every `asyncio.create_task(...)` result MUST be stored in a structured collection or container that ensures the task is awaited or cancelled at scope exit. Bare `create_task(...)` without retention is a defect — exceptions vanish, GC may collect the task mid-flight. |
| **DP-12** | **Time MUST be UTC at every boundary.** `datetime.now()` is forbidden; use `datetime.now(UTC)`. Serialised form is ISO 8601 with explicit `Z` or `+00:00`. Local-zone display happens only at the UI rendering layer. Naive datetimes (no `tzinfo`) MUST NOT cross any module boundary. |
| **DP-13** | **Every owned external resource MUST be released deterministically.** Files, sockets, subprocesses, locks, GUI widgets, OS handles MUST be acquired inside a context manager (`with`) or paired with a `try / finally`. Reliance on garbage collection for resource release is a defect. |

---

## 1. The five non-negotiable principles

### 1.1 Strict layering

Every project that adopts these standards organizes around **layers that flow downward only**:

```
┌──────────────────────────────────────────────────────────────┐
│ Layer 1 — Entry points: CLI / GUI / mobile shell / daemon │
│ Imports from L2, L3, L4. Never from a sibling entry point. │
└──────────────────────────────────────────────────────────────┘
                             │
                             ▼
┌──────────────────────────────────────────────────────────────┐
│ Layer 2 — Application / orchestration / use cases │
│ Coordinates L3 services to fulfill a user-visible action. │
│ Imports from L3 + L4 only. │
└──────────────────────────────────────────────────────────────┘
                             │
                             ▼
┌──────────────────────────────────────────────────────────────┐
│ Layer 3 — Domain services + providers (the seams) │
│ Pure logic + ABCs that abstract third-party I/O. │
│ Imports from L4 only. │
└──────────────────────────────────────────────────────────────┘
                             │
                             ▼
┌──────────────────────────────────────────────────────────────┐
│ Layer 4 — Foundation: typed config, exceptions, domain │
│ types, common utilities (logging setup, shell wrapper). │
│ Imports nothing from the project except other L4 modules. │
└──────────────────────────────────────────────────────────────┘
                             │
                             ▼
┌──────────────────────────────────────────────────────────────┐
│ Layer 5 — Platform: third-party SDKs, OS, hardware │
│ Touched ONLY through provider implementations in L3. │
└──────────────────────────────────────────────────────────────┘
```

**MUSTs:**
- Imports MUST flow downward only. CI lints this with [`import-linter`](https://import-linter.readthedocs.io/) or equivalent.
- A module in L3 MUST NOT know which L1 entry point invoked it. The same code path serves CLI, GUI, and headless service.
- Sibling modules at the same layer MUST NOT cross-import unless one is documented as the canonical "root" of the layer (see `cli/groups/vm.py` in VManager — it is the documented root for `cli/groups/vm_*.py` siblings).

**AS-IS in VManager:**
- L1 = `src/vmanager/cli/`, `src/vmanager/gui/`
- L2 = `src/vmanager/lab/orchestrator.py`, `src/vmanager/scheduler/runner.py`, `src/vmanager/operations/*.py`, `src/vmanager/lifecycle/*.py`, `src/vmanager/monitor/*.py`
- L3 = `src/vmanager/providers/`, `src/vmanager/configurator/`, `src/vmanager/creator/`, `src/vmanager/network/`, `src/vmanager/storage/`, `src/vmanager/vault/`
- L4 = `src/vmanager/config.py`, `src/vmanager/exceptions.py`, `src/vmanager/domain.py`, `src/vmanager/common/`
- L5 = `libvirt`, `qemu-img`, `cloud-init`, `paramiko`, OS

The dependency rule is documented in `docs/ARCHITECTURE.md`. CI does **not** lint it today (gap — adding `import-linter` is recommended; see `COMMON_COMPONENTS.md` gap plan).

---

### 1.2 Replaceability through provider ABCs

> "Every external dependency that you cannot easily replace is a future architectural debt."

Every third-party dependency that performs I/O — a hypervisor, a DB, a cloud SDK, a hardware driver, an ML model, an OS-process toolchain — **MUST sit behind an abstract base class** in the application's `providers/` (or `drivers/`, `adapters/`) package.

**Why:**
- **Testability.** Unit tests run against an in-memory mock without needing the real daemon installed.
- **Replaceability.** Switching backends is a new class, not a refactor.
- **Failure injection.** Mocks ship a `FailurePlan` so chaos tests verify error-path behavior.

**Pattern:**

```python
# src/<app>/providers/base.py
from __future__ import annotations
from abc import ABC, abstractmethod
from typing import Protocol, runtime_checkable

from <app>.domain import EntityHandle, EntityState # frozen domain types only
from <app>.exceptions import ProviderError

class ResourceProvider(ABC):
    """Single seam between core code and the underlying platform.

    Every method either returns a value or raises a typed
    :class:`<app>.exceptions.ProviderError` subclass. Implementations
    MUST translate every third-party exception at the seam.
    """

    @abstractmethod
    def get(self, name: str) -> EntityHandle: ...

    @abstractmethod
    def state(self, handle: EntityHandle) -> EntityState: ...

    # ... etc
```

```python
# src/<app>/providers/mock_provider.py
class MockProvider(ResourceProvider):
    """In-memory fake. Deterministic. Used by every unit test.

    Ships a :class:`FailurePlan` hook so tests can register targeted
    exceptions on specific method calls — chaos / degradation testing
    without touching the real backend.
    """
```

```python
# src/<app>/providers/<vendor>_provider.py
class VendorProvider(ResourceProvider):
    """Production implementation. Translates vendor exceptions
    via :func:`_translate` at every method boundary."""
```

**MUSTs:**
- The ABC's abstract method set MUST be locked in by a test (`EXPECTED_METHODS = frozenset(...)`) so adding/removing methods breaks deliberately and forces both the production and mock impls to be updated in the same commit.
- Every implementation method MUST translate vendor exceptions to the typed app exception hierarchy at the boundary.
- The ABC MUST be the single import path into the platform from anywhere in core. CI MUST lint that the third-party SDK is imported only inside the corresponding provider implementation.

**AS-IS in VManager:**
- `src/vmanager/providers/base.py::VMProvider` — the ABC.
- `src/vmanager/providers/libvirt_provider.py` — the **only** file that imports `libvirt`.
- `src/vmanager/providers/mock_provider.py` — in-memory mock with `FailurePlan`.
- Lock-in test: `tests/unit/test_provider_base.py::EXPECTED_METHODS`.

---

### 1.3 Domain types as the lingua franca

A domain object passed between modules **MUST be a frozen dataclass** (or a frozen pydantic model). Bare strings, dicts, and tuples MUST NOT cross module boundaries as identity.

**Why:**
- Refactor safety — renaming a field is a single mypy-checked change.
- Self-documenting — a function signature `start(provider, handle: VMHandle)` says far more than `start(provider, name: str)`.
- Resolve once — the cost of looking up a name is paid once at the entry point, not on every call.

**Pattern:**

```python
# src/<app>/domain.py
from __future__ import annotations
from dataclasses import dataclass
from enum import StrEnum

class EntityStatus(StrEnum):
    READY = "ready"
    BUSY = "busy"
    FAILED = "failed"

@dataclass(frozen=True, slots=True)
class EntityHandle:
    """Stable handle to a remote entity.

    UUID is the identity (immutable). Name is the human label (may
    change). Status is display-only — readers that need live state
    MUST call :meth:`ResourceProvider.state(handle)`.
    """
    uuid: str
    name: str
    status: EntityStatus
```

**MUSTs:**
- Domain types **MUST** be frozen.
- Any field used for "live state" reading inside a domain object MUST be annotated as display-only in the docstring; readers MUST go through the provider for fresh state. (See `VMHandle.state` in VManager.)
- Domain types MUST live in a single `domain.py` (or `domain/` package) at L4 and import nothing from the project except `exceptions.py` and other domain types.

**AS-IS in VManager:**
- `src/vmanager/domain.py` defines `VMHandle`, `VMStatus`, `VMState`, `VMMetrics`, `HostMetrics`, `SnapshotInfo`, `NetworkInfo`, `LeaseInfo`, `StoragePoolInfo`, `VolumeInfo`. All frozen except `VMState` (mutated in place by the libvirt event callback — documented exception).
- The convention is followed across every public API (lifecycle, operations, monitor, GUI workers).

---

### 1.4 Testability is non-negotiable

**MUSTs:**
- `tests/` MUST mirror `src/` 1:1.
- Tests MUST run with **no** real third-party daemon, **no** production credentials, **no** network calls.
- External side effects (subprocess, network, filesystem outside `tmp_path`, time, randomness) MUST be injectable. Use `field(default_factory=lambda: real_function)` with a **module-level alias** for the import — see VManager's `lab/orchestrator.py` and the cross-cutting note in `docs/claude/CLAUDE.md` about why this shape is required for `monkeypatch.setattr` to reach the dependency.
- Coverage gate: ≥80% on each module's new surface.
- Each unit test file MUST run in isolation (no test order dependencies, no shared fixture state across files).

**SHOULDs:**
- Property tests (Hypothesis) for any code that round-trips data (parse/serialize, save/load).
- Integration tests (still without real third-party daemons — use two mocks for cross-provider scenarios) for critical pipelines.
- A real-platform smoke script (read-only by default) for the L1 → L2 path. See `scripts/smoke-real-libvirt.py` in VManager — never mutates state; only observes.

**AS-IS in VManager:**
- `tests/unit/` mirrors `src/vmanager/`.
- Every test exercises `MockProvider`. CI runs without `python3-libvirt`.
- 1877 tests as of 2026-04-15 (per `docs/claude/CLAUDE.md`).
- `scripts/smoke-real-libvirt.py` is the read-only real-libvirt smoke runner.

---

### 1.5 First-class concerns

The four concerns below are evaluated in **every** code review and **every** design discussion. You do not get to defer them to "v2."

#### Security
- Subprocess invocation MUST use `argv` lists, not strings. `shell=True` is forbidden in core code. (See `src/vmanager/common/shell.py`.)
- User-supplied templates MUST go through `string.Template.safe_substitute` + `shlex.split`, not f-strings or shell. (See `src/vmanager/hooks/executor.py`.)
- Secrets MUST NOT live in config files. They live in the system keyring (libsecret/Keychain/Credential Manager) or, as a fallback, in an encrypted file (Fernet AES-128). (See `src/vmanager/vault/`.)
- File writes that may carry private data MUST set mode 0o600.
- Deserialization of YAML/JSON from untrusted sources MUST use `yaml.safe_load`. Pickle is forbidden for inter-process state.

#### Reliability
- Top-level error handlers wired in every entry point — see `ERROR_HANDLING.md` rule **EH-04**.
- Atomic writes (temp file + rename) for any state file the application owns. Partial writes from a crash MUST NOT corrupt state.
- Idempotent operations where possible. Re-running a failed scheduler tick MUST NOT double-fire actions (use cursor + `INSERT OR REPLACE` idiom).
- Background loops (event subscribers, schedulers, alert evaluators) MUST be designed so a single failure cannot halt the loop. The "fire-one-never-raises" pattern is canonical (see `src/vmanager/hooks/executor.py`, `src/vmanager/scheduler/runner.py`).

#### Performance
- Hot loops MUST NOT do I/O. Lookups in inner loops (config values, environment variables, file paths) MUST be cached.
- `ProgressReporter.report()` and `is_cancelled()` MUST be cheap (no IO, no locks). They are called per output line of streamed subprocess.
- Long-running operations MUST yield. UI threads MUST never block more than 16 ms (one frame at 60 Hz).
- Database writes batched where possible. SQLite MUST use WAL mode. (See `src/vmanager/monitor/history.py`.)

#### Error reporting
- See `ERROR_HANDLING.md`. Every unhandled exception MUST surface in three places: (1) a structured log record at ERROR level with the full traceback, (2) a local crash report file under the app's state dir, (3) the configured remote sink (if enabled).

---

## 2. UI / threading model (mandatory for desktop & mobile apps)

### 2.1 The worker pattern

Every blocking operation invoked from a UI **MUST** run off the UI thread.

For Qt/PySide6, the canonical pattern is:

```python
# src/<app>/gui/workers/core_worker.py
from __future__ import annotations
from typing import Any

from PySide6.QtCore import QThread, Signal

from <app>.exceptions import AppError
from <app>.common.progress import CancellationToken, ProgressReporter

class CoreWorker(QThread):
    """Base class for every blocking GUI operation.

    Subclasses override :meth:`do_work` and return a result. The base
    class wires:
      - progress(str)             — human-readable status messages
      - finished_with_result(obj) — one shot on success
      - error(str)                — one shot on any exception
      - cancellation              — via :class:`CancellationToken`

    Worker threads MUST NOT touch QWidgets. Slots in the GUI thread
    receive results via auto-queued signal connections.
    """

    progress = Signal(str)
    finished_with_result = Signal(object)
    error = Signal(str)

    def __init__(self) -> None:
        super().__init__()
        self.cancellation = CancellationToken()
        self.reporter: ProgressReporter = _QtProgressBridge(self)

    def cancel(self) -> None:
        self.cancellation.cancel()

    def do_work(self) -> Any:
        raise NotImplementedError

    def run(self) -> None:
        try:
            result = self.do_work()
        except AppError as e:
            self.error.emit(str(e))
        except Exception as e:  # rule EH-06: GUI workers catch ALL
            # Re-route through the application's crash reporter so
            # untyped errors don't vanish; then surface as a string.
            from <app>.observability.crash_reporter import report_now
            report_now(e, source="gui-worker")
            self.error.emit(f"unexpected error: {e}")
```

**MUSTs:**
- UI workers MUST catch **every** exception (typed AND untyped) — see rule **EH-06**. An untyped exception that crashes the worker silently is a defect.
- The signal name MUST NOT collide with `QThread.finished` — use `finished_with_result`.
- Cancellation MUST be cooperative via `CancellationToken`, not via thread interruption.
- One-shot work uses `QThread` subclassing; long-lived work that needs an event loop on the worker thread uses `QObject` + `moveToThread`. (See `src/vmanager/gui/workers/metrics_worker.py` — the QTimer requires this.)

**AS-IS in VManager:**
- `src/vmanager/gui/workers/core_worker.py::CoreWorker` is the base. Concrete: `lifecycle_workers.py`, `snapshot_workers.py`, `vm_create_worker.py`, `image_download_worker.py`, `monitor_worker.py`, `metrics_worker.py`, `lab_workers.py`, `ova_workers.py`.
- **Gap:** today's `CoreWorker` only catches `VManagerError`, not bare `Exception`. The standard above is stricter — VManager MUST close this gap (see `ERROR_HANDLING.md` gap plan).

### 2.2 Designer-first UI

Every dialog, window, and reusable widget **MUST** be designed in a `.ui` XML file (Qt Designer) and loaded at runtime. Hand-built widget trees in Python are forbidden.

**Why:** designers (or future engineers) can edit the UI without re-reading every controller. Layouts are visible in a tool. Diffs are auditable.

**Pattern:**

```python
# src/<app>/gui/ui_loader.py
from importlib.resources import files
from PySide6.QtUiTools import QUiLoader

def load_ui(filename: str, package: str = "<app>.gui.ui") -> QWidget:
    """Resolve a .ui file via importlib.resources so it works from
    both source tree AND wheel install. Never use Path(__file__).parent."""
    ...
```

**MUSTs:**
- `.ui` files live under `<app>/gui/ui/`.
- Loaded via `importlib.resources` — NOT `Path(__file__).parent` (breaks zip-imported wheels).
- A controller class wraps the loaded widget; it MUST NOT subclass QWidget. Tests register the wrapped widget via `qtbot.addWidget(ctrl.window)`.
- Errors during load (missing `.ui`, missing child widget) MUST raise a typed `GuiLoadError(AppError)` so the CLI exit-code mapper handles them.

**AS-IS in VManager:** fully implemented. See `src/vmanager/gui/ui/`, `src/vmanager/gui/ui_loader.py`, every window in `src/vmanager/gui/windows/`.

---

## 3. Configuration injection

> "Pass dependencies; don't reach for them."

**MUSTs:**
- `AppConfig` (or the project's equivalent `Settings` class — see `CONFIGURATION.md`) MUST be loaded **once** at the entry point and passed down by constructor injection.
- No module other than the entry point reads `config.yaml` / env vars / `.env` directly.
- Long-lived service objects (database handle, provider, history store) MUST be created from config at the entry point and injected into consumers as constructor args. Factories on a Click context (`ctx.obj["provider_factory"]`) are acceptable when construction is expensive and not every command needs it.

**AS-IS in VManager:** the rule is upheld — every module that needs config takes it as a constructor arg. The CLI top-level group at `src/vmanager/cli/__main__.py` loads it once via the click `main` callback. The GUI at `src/vmanager/gui/app.py` loads it identically inside its Qt entry point.

---

## 4. Documentation discipline

**MUSTs:**
- Every public class and function MUST have a docstring stating intent + invariants. (Module docstrings explain the *why* of the file, not what.)
- A docstring that says "returns the result" or "the foo of the bar" MUST be deleted or rewritten. It's worse than nothing.
- Every non-obvious *why* MUST be a comment. The rule is: if a future reader (human or AI) will not be able to predict the choice from the code, write the comment.
- Every architectural rule that took a debugging session to discover MUST land in `docs/claude/CLAUDE.md` (project-specific) or `docs/standards/` (cross-project).

**MUST NOTs:**
- Comments that paraphrase the next line. (`# increment counter` then `counter += 1`.)
- Stale TODO comments without a date or owner.
- ASCII-art separators that aren't meaningful section breaks.

---

## 5. Replaceability path table

For each major external dependency, the replaceability story MUST be documented near the abstraction. Template:

| Dependency | Abstraction | Files that change to swap | Effort |
|---|---|---|---|
| Hypervisor (libvirt) | `providers/base.py::VMProvider` | `providers/<new_provider>.py` (+ tests) | New file, ~1.5–2 KLOC |
| GUI toolkit (PySide6) | `gui/workers/core_worker.py` + `gui/ui_loader.py` | All of `gui/` (controllers + loader); core untouched | Rewrite of `gui/`, ~3 KLOC |
| CLI framework (Click) | `cli/__main__.py` (decorators) | All of `cli/`; core untouched | Mechanical rewrite of `cli/`, ~2 KLOC |
| State DB (SQLite) | `monitor/history.py::History` | One file (the History class) | ~500 LOC |

**MUSTs:**
- Every abstraction in this table MUST be documented in the corresponding module's docstring.
- The table itself MUST live in the project's `ARCHITECTURE.md` (or equivalent) and be updated when a new dependency is added.

---

## 6. Performance budgets

| Operation | Budget |
|---|---|
| UI thread per frame | ≤16 ms (60 fps target) |
| Background worker progress callback | ≤1 ms (no IO, no locks) |
| Single SQLite write in foreground | ≤10 ms |
| Top-level command import (e.g., `vmanager --help`) | ≤200 ms |
| Per-record log emission | ≤100 µs (no IO; write goes to a queue or the buffered handler) |

**SHOULDs:**
- Profile any new loop with `cProfile` or a tracing profiler before merging.
- Use `time.perf_counter()` instrumentation around suspected hot paths during development.

---

## 7. Concurrency rules

**MUSTs:**
- Every shared mutable structure MUST declare its threading model in its docstring (single-threaded / thread-safe via lock / lock-free with atomic ops).
- A lock acquisition order MUST be documented when more than one lock exists. Acquire in the documented order to prevent deadlock.
- Background callbacks (libvirt event callbacks, signal slots, GUI timers) MUST NOT hold long-running locks.
- Any cross-thread message passing MUST use Qt signals (in GUI code) or `queue.Queue` (in pure-Python code). Mutating a shared list from two threads without a lock is a defect.

**AS-IS in VManager:**
- `src/vmanager/monitor/state.py::StateStore` uses a `threading.RLock`; the docstring documents the rule "never hold external locks around StateStore calls."
- `src/vmanager/gui/workers/monitor_worker.py` runs on the main thread because libvirt event callbacks fire on libvirt's own background thread regardless — the `Qt.AutoConnection` queue handles the cross-thread hop.

---

## 8. Cancellation model

**MUSTs:**
- Every long-running operation **MUST** accept an explicit `CancellationToken`-like object.
- `ProgressReporter.is_cancelled()` is the canonical check inside loops.
- On cancellation, the operation MUST exit cleanly: kill subprocesses with SIGTERM, close sockets, release locks, leave state files unchanged or roll back.
- A cancellation MUST raise a typed exception (e.g., `CommandError(returncode=-15, stderr="cancelled by user")`), not a bare `KeyboardInterrupt`.

**AS-IS in VManager:**
- `src/vmanager/common/progress.py::CancellationToken` + `ProgressReporter`.
- Wired through `common/shell.py`, `common/ssh.py`, image downloader, lab orchestrator, metrics poller, all GUI workers.

---

## 9. Anti-patterns (forbidden)

| Anti-pattern | Why forbidden | Right shape |
|---|---|---|
| `except Exception: pass` | Hides bugs forever | Translate to typed exception, or log and re-raise, or use `contextlib.suppress(SpecificError)` in cleanup paths only |
| `except: ...` (bare `except`) | Catches `KeyboardInterrupt`, `SystemExit` — never what you want | Always specify the exception type |
| `subprocess.run(cmd_string, shell=True)` | Shell injection risk | Use `argv` list + `shell=False`; pre-validate user input |
| Re-raising without `from e` | Loses the original cause; debugging becomes archaeology | `raise NewError(...) from e` always |
| Module-level `Path("~/...").expanduser()` | Captures `$HOME` at import time; breaks tests that monkeypatch HOME per test | Wrap in a `_default_X()` function called at use-time |
| Global mutable state | Untestable, racy | Constructor-inject; use a dataclass holding the state |
| `print()` for diagnostics | Bypasses logging; output mixes with user-facing CLI output | Use the logger; reserve `print` / `click.echo` for user-facing output |
| String formatting inside log messages (`LOG.info(f"{x}")`) | Loses structured fields; eager format even when level is filtered | `LOG.info("processing %s", x)` (lazy %-style) |
| Catching `Exception` in a worker but not in a top-level handler | Untyped exceptions leak to the UI layer or crash the thread silently | Both layers catch — see rule **EH-06** |
| Hand-built Qt widget trees in Python | Unreviewable; designers can't touch | `.ui` files + `QUiLoader` |

---

## 10. Self-review checklist (every PR)

Before requesting review, the author MUST be able to answer YES to all of these:

1. Does every new public function have a type annotation and a docstring stating intent + invariants?
2. Does `mypy --strict` pass on `src/`?
3. Does `ruff check src tests` produce zero diagnostics?
4. Did I add at least one test for every new code path? (Mirror in `tests/unit/`.)
5. Do I touch any external dependency directly, or only through its provider ABC?
6. If I touch error paths: does every translation chain `from e`? Does every `except` either re-raise typed or log + continue with documented invariant?
7. If I add a config field: does it have a typed default? Is it consumed only via the `Settings` object?
8. If I add a log call: does it use lazy formatting? Does it include structured context (`extra={...}` for fields)? At ERROR, does it include `exc_info=True` / use `LOG.exception`?
9. If I add a long-running op: does it accept a `CancellationToken`? Does it call `is_cancelled()` in its inner loop?
10. If this is GUI code: did I run the app and exercise the new feature in a real window? Did I write a `pytest-qt` test for the controller?

---

## 11. Async / concurrency rules (rule DP-11)

Async code multiplies the failure surface — a task that crashes
silently is far harder to find than a synchronous exception. The
rules below keep the surface tractable.

### 11.1 Task lifecycle ownership

Every `asyncio.create_task(...)` MUST be retained:

```python
# WRONG — task not retained; if GC collects it, the work disappears
async def kick_off():
    asyncio.create_task(do_work())   # vanishing task
    return

# RIGHT — task retained; awaited at a known point
async def kick_off(tasks: set[asyncio.Task]) -> None:
    task = asyncio.create_task(do_work())
    tasks.add(task)
    task.add_done_callback(tasks.discard)
```

Or use `asyncio.TaskGroup` (Python 3.11+) when the lifetime is
naturally scoped:

```python
async with asyncio.TaskGroup() as tg:
    tg.create_task(do_work_a())
    tg.create_task(do_work_b())
# All tasks awaited at the `async with` exit. Any exception cancels siblings.
```

`TaskGroup` is the preferred shape for new code. The retained-set
shape is the fallback for tasks whose lifetime is the application,
not a function scope.

### 11.2 Cancellation model in async code

Async cancellation is **cooperative** — `task.cancel()` raises
`asyncio.CancelledError` at the next `await` point. The ops MUST:

1. **Re-raise `CancelledError`** in their `except` blocks unless
   they own the cancellation (e.g., a top-level supervisor that
   converts cancellation to a typed `OperationError`).

   ```python
   try:
       result = await long_op()
   except asyncio.CancelledError:
       LOG.info("operation cancelled")
       raise
   ```

2. **Clean up before re-raising.** Close sockets, cancel child
   tasks, release locks. The `try / finally` shape:

   ```python
   try:
       result = await long_op()
       return result
   finally:
       await client.aclose()
   ```

3. **Bound cleanup with a shielded timeout.** Cleanup that itself
   uses `await` must not block forever:

   ```python
   try:
       async with asyncio.timeout(5):
           await client.aclose()
   except TimeoutError:
       LOG.warning("aclose timed out; abandoning")
   ```

### 11.3 No mixing event loops

A process MUST run a single asyncio event loop. `asyncio.run(...)`
is the entry point for short-lived async code; long-lived async
code (servers, daemons) creates the loop explicitly with
`asyncio.new_event_loop()` + `loop.run_until_complete(...)`.

Calling synchronous blocking code from an async context MUST go
through `asyncio.to_thread(...)` — never block the loop directly:

```python
# RIGHT — runs the blocking call on the threadpool
data = await asyncio.to_thread(blocking_read, path)

# WRONG — blocks the loop; every other coroutine waits
data = blocking_read(path)
```

### 11.4 Async + Qt

Qt has its own event loop. The two-loop integration MUST go through
[`qasync`](https://pypi.org/project/qasync/) (or equivalent
`PySide6.QtAsyncio`) — never two raw event loops in one process.

For most GUI work, the `CoreWorker(QThread)` pattern (§2.1) is
preferable to async because it interoperates cleanly with Qt
signals and the existing `CancellationToken` plumbing. Choose
async only when the underlying I/O is async-native (modern HTTP
client, async DB driver) and the gain outweighs the integration
cost.

### 11.5 Timeouts everywhere

Per rule **SC-07**, every external `await` MUST have a finite
timeout. Use `asyncio.timeout(...)` as the canonical shape:

```python
async with asyncio.timeout(30):
    response = await client.get(url)
```

A timeout MUST raise a typed exception
(`GuestUnreachableError`, `CommandError`), not a bare
`asyncio.TimeoutError`. The translation seam (rule **EH-02**) does
the conversion.

### 11.6 Async error handling cross-reference

See [`ERROR_HANDLING.md`](ERROR_HANDLING.md) §13 for the full
async-error-path patterns: `gather(return_exceptions=True)`
discipline, the asyncio `loop.set_exception_handler` requirement
(rule **EH-04**), and the contextvars-based correlation propagation.

---

## 12. Time and dates (rule DP-12)

Naive datetimes are the second most common source of cross-timezone
bugs (after off-by-one errors).

### 12.1 The two acceptable forms

**At every boundary** (config, log records, persisted state, IPC,
HTTP, file metadata) time is one of:

1. **`datetime` with `tzinfo=UTC`** in memory.
2. **ISO 8601 string** (`2026-04-25T14:32:01.234Z` or
   `2026-04-25T14:32:01.234+00:00`) in serialised form.

```python
# RIGHT
from datetime import UTC, datetime

now = datetime.now(UTC)
serialised = now.isoformat()

# WRONG
from datetime import datetime
now = datetime.now() # naive — no tzinfo
serialised = str(now) # implementation-defined format
```

### 12.2 Local time at the UI only

The UI is the **only** layer that converts UTC → local time, and it
does so at render time. The model layer never holds a local-zone
datetime:

```python
# In the GUI controller:
def _format_for_display(self, ts: datetime) -> str:
    assert ts.tzinfo is not None, "naive datetime in display layer"
    local = ts.astimezone()  # converts to system local zone
    return local.strftime("%Y-%m-%d %H:%M")
```

### 12.3 Monotonic time for durations

Wall-clock time is **not monotonic** — NTP can skew it backward, a
user can reset the clock, leap seconds happen. For measuring
elapsed durations, use `time.monotonic()`:

```python
# RIGHT — durations
start = time.monotonic()
do_thing()
elapsed_s = time.monotonic() - start

# WRONG
start = datetime.now(UTC)
do_thing()
elapsed = (datetime.now(UTC) - start).total_seconds() # NTP-jittery
```

Wall-clock `datetime` is the right choice for **timestamps** (when
something happened); monotonic is the right choice for **durations**
(how long something took).

### 12.4 Pydantic + datetime

Pydantic models that carry timestamps MUST use `datetime` with a
field validator enforcing tz-awareness:

```python
from datetime import UTC, datetime
from pydantic import BaseModel, field_validator

class Event(BaseModel):
    ts: datetime

    @field_validator("ts")
    @classmethod
    def _ensure_utc(cls, v: datetime) -> datetime:
        if v.tzinfo is None:
            raise ValueError("ts MUST be timezone-aware")
        return v.astimezone(UTC)
```

### 12.5 Serialised form

| Form | Example | When |
|---|---|---|
| ISO 8601 with `Z` | `2026-04-25T14:32:01.234Z` | Default for logs, config, state |
| ISO 8601 with offset | `2026-04-25T14:32:01.234+00:00` | When stdlib `.isoformat()` is the producer |
| Unix epoch (float) | `1745605921.234` | Inter-process time without parsing overhead |
| RFC 7231 (HTTP `Date`) | `Sat, 25 Apr 2026 14:32:01 GMT` | HTTP headers only |

ISO 8601 with `Z` is the default for any new structured field. The
JSON formatter at `mgf.common.observability.json_formatter` already
emits this shape for `ts`.

### 12.6 Anti-patterns

| Anti-pattern | Why forbidden | Right shape |
|---|---|---|
| `datetime.utcnow()` | Returns a NAIVE datetime in UTC clothing | `datetime.now(UTC)` |
| `datetime.fromtimestamp(x)` | Defaults to local zone | `datetime.fromtimestamp(x, UTC)` |
| `time.time()` for timestamps in records | Loses sub-second precision and tz info | `datetime.now(UTC)` |
| `pytz.timezone(...)` (legacy library) | Easy to misuse via `.localize()` vs `.replace()` | stdlib `zoneinfo.ZoneInfo("Region/City")` |
| Comparing naive and aware datetimes | `TypeError` at runtime | Make both aware |
| String-arithmetic on ISO timestamps | Brittle | Parse with `datetime.fromisoformat`, do math, format back |

---

## 13. Resource management (rule DP-13)

Every external resource (file, socket, subprocess, lock, GUI
widget, OS handle, libvirt connection, paramiko channel) MUST be
released deterministically. Reliance on garbage collection for
resource release is a defect — GC timing is non-deterministic, and
under `filterwarnings = ["error"]` (rule **TS-04**), the
`ResourceWarning` from GC-finalised resources fails the test suite.

### 13.1 The context manager rule

Every resource type MUST expose a context manager:

```python
# Read a file — context manager
with path.open("rb") as fh:
    data = fh.read()

# HTTP client — context manager
with httpx.Client(timeout=10) as client:
    response = client.get(url)

# Subprocess — context manager via subprocess.Popen
with subprocess.Popen([...]) as proc:
    out, err = proc.communicate(timeout=30)

# Lock
with self._lock:
    self._mutate()
```

If a resource the project owns lacks a `__enter__` / `__exit__`,
add one. `contextlib.contextmanager` is the cheap path:

```python
from contextlib import contextmanager

@contextmanager
def database_session(uri: str) -> Iterator[Session]:
    sess = _connect(uri)
    try:
        yield sess
    finally:
        sess.close()
```

### 13.2 Cleanup ordering

When multiple resources need cleanup in a specific order, use
`contextlib.ExitStack`:

```python
with ExitStack() as stack:
    fh = stack.enter_context(path.open("rb"))
    client = stack.enter_context(httpx.Client(timeout=10))
    proc = stack.enter_context(subprocess.Popen([...]))
    # On exit: proc closed → client closed → fh closed (LIFO).
    do_work(fh, client, proc)
```

LIFO matches the natural "innermost holds the latest reference"
intuition. When you need a different order, document it.

### 13.3 Class-owned resources

A class that owns a resource for its lifetime MUST itself be a
context manager:

```python
class CredentialVault:
    def __enter__(self) -> CredentialVault:
        self._open_keyring_session()
        return self

    def __exit__(self, *exc_info) -> None:
        self._close_keyring_session()

# Caller:
with CredentialVault() as vault:
    cred = vault.get("vm:prod-1")
```

For long-lived classes whose lifetime spans the application
process (e.g., the `Settings` object), explicit `close()` plus an
`atexit.register` is acceptable. The class MUST still tolerate
double-close (idempotent) and MUST log a WARNING if it is GC'd
without `close()` being called — the warning is the operator's
signal that a leak was caught by GC.

### 13.4 Async resources

Async resources use `async with`:

```python
async with httpx.AsyncClient(timeout=10) as client:
    response = await client.get(url)
```

The `__aenter__` / `__aexit__` protocol is the async analogue.
`AsyncExitStack` is the analogue of `ExitStack` for cleanup
ordering.

### 13.5 GUI widgets

QWidget instances are managed by the Qt parent-child tree — when
the parent is destroyed, children follow. The application MUST NOT
hold references to widgets outside that tree (a long-lived list of
widgets keeps them alive past their intended scope, manifests as
memory growth + stale slot calls).

The exception: top-level windows MUST be retained by the
application object (`QApplication` instance attribute) until
explicitly closed; otherwise Qt destroys them at scope exit.

### 13.6 Anti-patterns

| Anti-pattern | What's wrong | Right shape |
|---|---|---|
| `fh = path.open(...)` then no close | GC'd on next collection; `ResourceWarning` | `with path.open(...) as fh:` |
| `httpx.get(url)` (no client) | Implicit client per call; no connection reuse; hard to mock | `with httpx.Client(...) as client: client.get(url)` |
| `lock.acquire(); ...; lock.release()` (no try/finally) | Exception leaves the lock held | `with lock:` |
| `subprocess.Popen([...]).stdout` then drop | Zombie processes pile up | `with subprocess.Popen([...]) as proc:` |
| Manual `__del__` for cleanup | GC ordering is undefined; cycles defeat it | Context manager + explicit close |

---

## 14. Mechanical enforcement matrix

A MUST that no tool can check for relies entirely on code review
and will rot. The standards aim to make every MUST checkable.
Today's state for the design rules:

| Rule | Mechanical check | Notes |
|---|---|---|
| DP-01 layering | `import-linter` contracts (`mgf-common-is-the-federation-leaf`, per-project layered contracts) | The cornerstone enforcement; CI MUST run `lint-imports` |
| DP-02 provider ABC | `EXPECTED_METHODS` lock-in test + `import-linter` "vendor SDK only in <vendor>_provider.py" | Pinned per provider |
| DP-03 frozen domain types | code review + grep for `@dataclass` without `frozen=True` in `domain/` | A small lint script is the cheap path |
| DP-04 typed signatures | `mypy --strict` on `src/` | Required CI gate |
| DP-05 test mirror + no real I/O | `tests/test_mirror.py` + `import-linter` forbidding network/SDKs from `tests/unit/` | Rule TS-01 + TS-02 |
| DP-06 first-class concerns | code review checklist (§10) | + the SC-* matrix in `SECURITY_STANDARD.md` §15 |
| DP-07 docstrings | code review + ruff `D101`/`D102` rules where adopted | Pragmatic — not every helper needs one |
| DP-08 replaceability | docstring on each abstraction + the §5 table | Review-only |
| DP-09 explicit cancellation | code review + grep for `threading.Event` used as a global flag | The `CancellationToken` type is the canonical answer |
| DP-10 no silent fallback | code review + `audit=true` log entry per backend choice | Pinned by SC-17 audit-event tests |
| DP-11 task ownership | `ruff` rule `RUF006` (unawaited task) on by default | + grep for bare `asyncio.create_task` |
| DP-12 UTC time | `ruff` rule `DTZ` family (`DTZ001`–`DTZ005`) | Enable in `pyproject.toml`; `datetime.utcnow()` and `datetime.now()` without tz fail lint |
| DP-13 resource release | `ruff` rule `SIM115` ("use a context manager for opening files") + `RUF100` cleanup | + filterwarnings catches GC-finalised resources |

Adopt every mechanical check above in `pyproject.toml`:

```toml
[tool.ruff.lint]
select = [
    # ... existing ...
    "DTZ",   # rule DP-12 — naive datetime usage
    "RUF006", # rule DP-11 — unawaited tasks
    "SIM115", # rule DP-13 — open() without context manager
    "ASYNC", # general asyncio correctness
]
```

A new MUST proposed for inclusion in a future version of this doc
MUST come with a mechanical check or a written justification for
why it is review-only.

---

## Appendix A — Module-level alias for injectable defaults

This is the single most-violated subtle rule in VManager. Documenting it here so future projects do not re-discover it the hard way.

When a dataclass takes an injectable callable that defaults to a sibling-module function, **import the module, not the function**:

```python
# WRONG — the test patch cannot reach this binding
from <app>.monitor.health import wait_until_healthy

@dataclass
class Orchestrator:
    wait_fn: Callable[..., None] = field(
        default_factory=lambda: wait_until_healthy
    )
```

```python
# RIGHT — re-resolves through the module attribute every call,
# so monkeypatch.setattr(health, "wait_until_healthy", fake) works
from <app>.monitor import health as health_module

@dataclass
class Orchestrator:
    wait_fn: Callable[..., None] = field(
        default_factory=lambda: health_module.wait_until_healthy
    )
```

The `from X import Y` form binds `Y` once in the importing module's namespace at import time. `monkeypatch.setattr(X, "Y", fake)` rebinds `X.Y` but does not touch the orchestrator's bound name. The module-alias form re-resolves through `X.Y` every time the factory runs.

**Locked in by:** `tests/unit/lab/test_lab_orchestrator.py`, `tests/unit/scheduler/test_scheduler_runner.py`. See VManager `docs/claude/CLAUDE.md` Cross-cutting patterns §1 for the full story.

---

*End of `DESIGN_PRINCIPLES.md`.*
