From df047dfaef7f60d926881c8198d6a2a4591d194b Mon Sep 17 00:00:00 2001 From: Paul O'Fallon <505519+pofallon@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:15:54 +0000 Subject: [PATCH] feat(registry): versioned structured host registry (registry v2) Replace the colon-delimited `~/.config/remo/known_hosts` store with a versioned JSON `registry.json` (format v2, named per-type fields, no positional overloading) behind a single accessor `core/registry.py` that owns parse/serialize/validate/advisory-lock/migrate for the CLI, providers, and web service. - Lazy, lossless, idempotent CLI migration (known_hosts -> registry.json, original renamed to known_hosts.v1.bak); tolerant per-entry migration skips+reports a v2-invalid legacy entry instead of aborting. - `core/known_hosts.py` slimmed to thin delegates (public API unchanged). - `remo add` accepts IPv6 literals (bracketed and bare multi-colon); the colon-safety field rejections are gone (JSON has no positional overloading). - Web read paths (discovery/state/check) collapse onto the shared read-only accessor; advisory `fcntl` locking wraps every read-modify-write with graceful degradation. - Setup API adoption mirror moves to payload v2 with a `payload_versions` capability handshake (still accepts v1); push delta-cache bumped to cache_version: 2. `replace_registry` uses true wholesale-replace semantics so the service removes (not renames) any stale legacy mirror and a malformed stale mirror can't 500 an otherwise-valid apply. - Docs (README, web-session-interface, CLAUDE.md) updated to registry.json. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HAPB5GA4NVUgKFsVFVJDxN --- .specify/feature.json | 2 +- CLAUDE.md | 11 +- README.md | 4 +- docs/web-session-interface.md | 8 +- pyproject.toml | 4 +- .../checklists/requirements.md | 36 + .../contracts/mirror-payload-v2.md | 77 ++ .../contracts/registry-accessor-api.md | 92 ++ .../contracts/registry-file-v2.md | 130 +++ specs/015-registry-v2/data-model.md | 113 +++ specs/015-registry-v2/plan.md | 114 +++ specs/015-registry-v2/quickstart.md | 118 +++ specs/015-registry-v2/research.md | 115 +++ specs/015-registry-v2/spec.md | 195 ++++ specs/015-registry-v2/tasks.md | 181 ++++ src/remo_cli/__main__.py | 4 +- src/remo_cli/cli/main.py | 16 + src/remo_cli/core/config.py | 31 + src/remo_cli/core/known_hosts.py | 236 ++--- src/remo_cli/core/registry.py | 835 ++++++++++++++++++ src/remo_cli/core/web_adopt.py | 101 ++- src/remo_cli/providers/added.py | 66 +- src/remo_cli/web/api/setup.py | 267 ++++-- src/remo_cli/web/check.py | 69 +- src/remo_cli/web/discovery.py | 80 +- src/remo_cli/web/state.py | 48 +- tests/conftest.py | 82 ++ .../integration/test_registry_concurrency.py | 98 ++ .../test_setup_payload_versions.py | 269 ++++++ tests/integration/test_web_adopt_e2e.py | 60 +- tests/perf/test_registry_perf.py | 86 ++ tests/unit/core/test_known_hosts.py | 42 +- tests/unit/core/test_registry_format.py | 415 +++++++++ tests/unit/core/test_registry_locking.py | 141 +++ tests/unit/core/test_registry_migration.py | 364 ++++++++ tests/unit/core/test_web_adopt_payload.py | 41 +- tests/unit/core/test_web_push.py | 28 +- tests/unit/providers/test_added_add.py | 80 +- .../test_provider_registry_entries.py | 234 +++++ tests/unit/web/conftest.py | 6 + tests/unit/web/test_check.py | 4 +- tests/unit/web/test_csp.py | 5 +- tests/unit/web/test_registry_readonly.py | 212 +++++ tests/unit/web/test_setup_api.py | 136 ++- tests/unit/web/test_setup_auth.py | 4 +- 45 files changed, 4772 insertions(+), 488 deletions(-) create mode 100644 specs/015-registry-v2/checklists/requirements.md create mode 100644 specs/015-registry-v2/contracts/mirror-payload-v2.md create mode 100644 specs/015-registry-v2/contracts/registry-accessor-api.md create mode 100644 specs/015-registry-v2/contracts/registry-file-v2.md create mode 100644 specs/015-registry-v2/data-model.md create mode 100644 specs/015-registry-v2/plan.md create mode 100644 specs/015-registry-v2/quickstart.md create mode 100644 specs/015-registry-v2/research.md create mode 100644 specs/015-registry-v2/spec.md create mode 100644 specs/015-registry-v2/tasks.md create mode 100644 src/remo_cli/core/registry.py create mode 100644 tests/integration/test_registry_concurrency.py create mode 100644 tests/integration/test_setup_payload_versions.py create mode 100644 tests/perf/test_registry_perf.py create mode 100644 tests/unit/core/test_registry_format.py create mode 100644 tests/unit/core/test_registry_locking.py create mode 100644 tests/unit/core/test_registry_migration.py create mode 100644 tests/unit/providers/test_provider_registry_entries.py create mode 100644 tests/unit/web/test_registry_readonly.py diff --git a/.specify/feature.json b/.specify/feature.json index 636e1e4..d526599 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/014-register-ssh-host" + "feature_directory": "specs/015-registry-v2" } diff --git a/CLAUDE.md b/CLAUDE.md index 7c74edb..5858f4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,6 @@ # remo Development Guidelines -Auto-generated from all feature plans. Last updated: 2026-07-13 +Auto-generated from all feature plans. Last updated: 2026-07-25 ## Constitution @@ -10,10 +10,11 @@ See `.specify/memory/constitution.md` for project principles and non-negotiable - Ansible 2.14+ / YAML + `ansible.builtin`, `community.general` (existing), Incus CLI (local) (002-incus-container-support) - N/A (Incus storage pools already configured by 001-bootstrap-incus-host) (002-incus-container-support) - Python 3.11+ + Click (CLI framework), InquirerPy (interactive picker), boto3 (AWS, optional), hcloud (Hetzner, optional) (003-python-cli-rewrite) -- Flat file (`~/.config/remo/known_hosts`, colon-delimited) (003-python-cli-rewrite) +- Versioned JSON registry (`~/.config/remo/registry.json`, format v2 — named fields per type, no positional overloading; single accessor `core/registry.py` owns parse/serialize/validate/lock/migrate for CLI, providers, and the web service). Legacy `~/.config/remo/known_hosts` (colon-delimited) is read-only migration input, lazily migrated to v2 on first CLI read and renamed to `known_hosts.v1.bak`. (003-python-cli-rewrite; superseded by 015-registry-v2) - Cross-provider snapshot model (`models/snapshot.py`) + shared helpers in `core/snapshot.py` (name generator, validator, table formatter, destroy-time cleanup hook). No new runtime deps. (005-provider-snapshots) - FastAPI/Uvicorn + WebSockets (backend, optional `web` extra), TypeScript/Vite/React + ghostty-web (frontend), Bash (`remo-host` host command templated by Ansible) (010-web-session-interface) -- Stdlib `urllib.request` CLI setup client + token-gated `/api/v1/setup/*` FastAPI surface; service state in flat files under the writable `REMO_HOME` volume (`web-identity/` keypair + service known_hosts, `~/.config/remo/web-service.json` saved credentials) (011-web-adopt) +- Stdlib `urllib.request` CLI setup client + token-gated `/api/v1/setup/*` FastAPI surface; service state in flat files under the writable `REMO_HOME` volume (`web-identity/` keypair + service known_hosts, `~/.config/remo/web-service.json` saved credentials, `cache_version: 2`) (011-web-adopt; payload versioning updated by 015-registry-v2) +- `core/registry.py`: stdlib `json` (format), `fcntl` (advisory locking via a `registry.lock` sidecar), `os.replace` (atomic writes). No new runtime deps. Setup API mirror payload moved to v2 (`contracts/mirror-payload-v2.md`) with a `payload_versions` capability handshake; an upgraded service still accepts v1 payloads. (015-registry-v2) - Ansible 2.14+ / YAML + `ansible.builtin`, `community.general` (for zypper module) (001-bootstrap-incus-host) @@ -43,7 +44,8 @@ src/remo_cli/ # Python CLI package (src layout, hatchling build) │ ├── config.py # REMO_HOME, paths, read-only registry accessor │ ├── output.py # Colored output, confirm() │ ├── validation.py # Name, port, region, tool validation -│ ├── known_hosts.py # Flat-file host registry +│ ├── registry.py # Registry v2 accessor: parse/serialize/validate/lock/migrate (registry.json + legacy known_hosts) +│ ├── known_hosts.py # Thin delegates onto registry.py (public API unchanged: get/save/remove/clear_known_hosts*) │ ├── ssh.py # build_ssh_base_cmd(), SSH options, terminal reset, timezone │ ├── remo_host_client.py # Versioned remo-host protocol client (shared by CLI + web) │ ├── web_adopt.py # Workstation-side adoption/push engine (stdlib HTTP, keyscan trust verify, authorized_keys mgmt, --via tunnel) @@ -188,6 +190,7 @@ Provider SDKs (boto3, hcloud) are lazy-imported with clear error messages if mis - Ansible 2.14+ / YAML: Follow standard conventions plus Constitution principles ## Recent Changes +- 015-registry-v2: Replaced the colon-delimited `known_hosts` registry with a versioned JSON `registry.json` (format v2, named per-type fields, no positional overloading) via a single new accessor `core/registry.py` (parse/serialize/validate/advisory-lock/migrate) shared by the CLI, providers, and web service; lazy, lossless, idempotent CLI migration (`known_hosts` → `known_hosts.v1.bak`); `core/known_hosts.py` slimmed to thin delegates; the setup API's adoption mirror moved to payload v2 with a `payload_versions` capability handshake (an upgraded service still accepts v1 payloads); push delta-cache bumped to `cache_version: 2`. - 011-web-adopt: Added CLI-to-web adoption — unconfigured boot with service-scoped ed25519 identity, token-gated `/api/v1/setup/*` (REMO_WEB_API_TOKEN, fail-closed), `remo web adopt`/`remo web push` (registry mirror + workstation-verified host keys + idempotent `remo-web@` authorized_keys entries), AwaitingAdoption SPA page. - 010-web-session-interface: Added remo-web Docker service (FastAPI + React/ghostty-web) brokering browser terminal sessions across all Remo-managed instances via a new remo-host SSH command; web extra + remo web {serve,check} CLI group. - 005-provider-snapshots: Added cross-provider snapshot CLI (`remo

snapshot {create,list,restore,delete}`) + destroy-time cleanup hook across Incus / Proxmox / AWS / Hetzner. diff --git a/README.md b/README.md index e4ccbbc..d981dbf 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ remo completion zsh >> ~/.zshrc remo completion fish > ~/.config/fish/completions/remo.fish ``` -After re-loading your shell, `remo proxmox info --name ` will suggest registered container names from your `known_hosts` registry. +After re-loading your shell, `remo proxmox info --name ` will suggest registered container names from your `registry.json` registry. --- @@ -454,7 +454,7 @@ rm -rf ~/.config/remo | Path | Contents | |------|----------| -| `~/.config/remo/` | Runtime state: `known_hosts` (environment registry) | +| `~/.config/remo/` | Runtime state: `registry.json` (environment registry, format v2); `known_hosts.v1.bak` (pre-upgrade backup, if you upgraded from an older remo) | **Note:** Uninstalling remo does not destroy any cloud resources (EC2 instances, Hetzner VMs, Incus or Proxmox containers). Run `remo destroy` first if you want to tear those down. diff --git a/docs/web-session-interface.md b/docs/web-session-interface.md index 24136fd..850a2cb 100644 --- a/docs/web-session-interface.md +++ b/docs/web-session-interface.md @@ -43,7 +43,7 @@ second launcher; it reuses the same host-side scripts (`project-launch`) that th ## Architecture ```text -~/.config/remo/known_hosts (read-only mount) +~/.config/remo/registry.json (read-only mount; legacy known_hosts also read in place) │ ▼ remo-web (FastAPI + Uvicorn) @@ -268,7 +268,9 @@ bind-mount deployment to this release changes nothing (FR-005/SC-005). Everything the adopted service owns lives in one place: `/web-identity/` — `id_ed25519` + `id_ed25519.pub` (the service keypair, generated once via `ssh-keygen` and **never** silently regenerated while the files exist), `known_hosts` (service-managed instance host keys), and -`state.json` (the deployment id). The registry stays at its usual path (`~/.config/remo/known_hosts`). +`state.json` (the deployment id). The registry stays at its usual path (`~/.config/remo/registry.json`, +format v2 — a legacy `known_hosts` file is still read in place if present, e.g. right after an +in-place service upgrade, before the next adoption push replaces it). From these artifacts the service derives one of four states: | State | Derivation | `remo web check` | `GET /api/v1/ready` | Browser | @@ -344,7 +346,7 @@ only its named state volume. | Mount | Purpose | Why read-only | |---|---|---| -| `${HOME}/.config/remo:/home/remo/.config/remo:ro` | The Remo **registry** (`known_hosts`) — provider type, instance name, address, SSH user, access mode, region. | This is metadata, **not authentication material** (see below). The service never needs to write it; FR-004 requires hot-reload without a container restart, not mutation. | +| `${HOME}/.config/remo:/home/remo/.config/remo:ro` | The Remo **registry** (`registry.json`, format v2 — or legacy `known_hosts`, read in place) — provider type, instance name, address, SSH user, access, and per-type fields (e.g. AWS instance id/region). | This is metadata, **not authentication material** (see below). The service never needs to write it; FR-004 requires hot-reload without a container restart, not mutation. | | `${HOME}/.ssh/id_ed25519:/home/remo/.ssh/id_ed25519:ro` (+ `config`, `known_hosts`) | The **SSH identity** that actually authenticates to every instance. | The service only ever needs to *use* this key, never modify it; read-only limits blast radius if the container is compromised. | | `${HOME}/.aws:/home/remo/.aws:ro` (optional, commented out by default) | AWS credentials/profile for any registered instance using the SSM access mode. | Same reasoning — read-only, and only mounted at all if you actually have SSM-routed instances. | diff --git a/pyproject.toml b/pyproject.toml index da3aca7..cbc42dc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,8 +38,8 @@ web = [ ] [project.scripts] -remo-cli = "remo_cli.cli.main:cli" -remo = "remo_cli.cli.main:cli" +remo-cli = "remo_cli.cli.main:main" +remo = "remo_cli.cli.main:main" [tool.hatch.build.targets.wheel] packages = ["src/remo_cli"] diff --git a/specs/015-registry-v2/checklists/requirements.md b/specs/015-registry-v2/checklists/requirements.md new file mode 100644 index 0000000..1d06830 --- /dev/null +++ b/specs/015-registry-v2/checklists/requirements.md @@ -0,0 +1,36 @@ +# Specification Quality Checklist: Versioned Structured Host Registry (Registry v2) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-07-25 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- The spec deliberately names two existing internal artifacts — the legacy file's location/format and the in-memory host model (KnownHost) — because they are the scope boundary the user's feature description fixed ("only serialization and parsing move"). These are constraints on WHAT changes, not prescriptions of HOW; no new technology, library, or code structure is prescribed. +- All ambiguities were resolved with documented defaults in the Assumptions section (migration timing, backup handling, downgrade posture, supported upgrade-skew direction, file naming deferred to design). No open clarifications remain. +- Validation performed 2026-07-25: all items pass on first iteration. diff --git a/specs/015-registry-v2/contracts/mirror-payload-v2.md b/specs/015-registry-v2/contracts/mirror-payload-v2.md new file mode 100644 index 0000000..cf5534e --- /dev/null +++ b/specs/015-registry-v2/contracts/mirror-payload-v2.md @@ -0,0 +1,77 @@ +# Contract: Adoption Mirror Payload v2 & Version Negotiation + +Governs the workstation→service registry mirror (push) after Registry v2. Existing endpoint semantics (pairing-code gating, dormant-404, validate-all-before-write, atomic ordered apply) are unchanged; this contract changes the payload schema and adds version negotiation. + +## 1. `GET /api/v1/setup/status` — capability advertisement + +Response gains one field: + +```json +{ + "state": "unconfigured | adopted | mount_configured | broken", + "deployment_id": "…", + "public_key_available": true, + "registry_instances": 5, + "payload_versions": [1, 2] +} +``` + +- `payload_versions`: payload versions this service accepts on `PUT /setup/registry`. +- **Workstation rule (FR-021)**: read `payload_versions` before pushing; absence of the field means `[1]` (pre-v2 service). If the workstation's payload version (2) is not listed, ABORT before any instance processing or PUT, with: *"this remo-web deployment only accepts registry payload v1 — upgrade the remo-web container image, then re-run the push."* No mutation of any kind (instances are not keyscanned/authorized either — fail truly fast). + +## 2. `PUT /api/v1/setup/registry` — payload schemas + +### v2 payload (new canonical; what an upgraded CLI sends) + +```json +{ + "version": 2, + "registry": [ { …hostEntry per registry-file-v2.md… } ], + "host_keys": { "": ["", …] } +} +``` + +- `registry` entries use the exact `hostEntry` schema from [registry-file-v2.md](registry-file-v2.md) — no overloaded fields on the wire (FR-020). +- `host_keys` semantics unchanged: keys must reference registry entries; entries with `access: "ssm"` MUST NOT have host keys (both ends enforce, as today). +- Validation is all-or-nothing before any write, as today. The colon/newline field checks that existed to protect the legacy line format are replaced by the v2 validation rules (V2–V6); the newline/control-character ban stays. + +### v1 payload (accepted for backward compatibility, FR-022) + +```json +{ "version": 1, "registry": [{ "type": "...", "name": "...", "host": "...", "user": "...", "instance_id": "", "access_mode": "", "region": "" }], "host_keys": { … } } +``` + +- An upgraded service MUST continue accepting v1 payloads from a not-yet-upgraded workstation, mapping entries through the same legacy→v2 mapper used by file migration (data-model §4), then storing v2. +- SSM classification of v1 entries follows the legacy implicit rule, exactly as the old service computed it. + +### Unknown versions + +- `version` not in `payload_versions` → `400` with `{"error": {"code": "unsupported_payload_version", "supported": [1, 2], …}}`. No partial application; the service's existing mirror stays intact and served (FR-021). + +## 3. Apply sequence (service side) + +Ordered, each step atomic (`os.replace`), crash-convergent (a crash mid-sequence leaves a readable superset; re-push converges): + +1. Write service trust file `web-identity/known_hosts` (unchanged from today). +2. Write `registry.json` (v2) via `core.registry.replace_registry`. +3. Remove any legacy `known_hosts` mirror file left from a pre-upgrade push (service-owned replaceable state — not user data; removal keeps the service out of the both-files-present state permanently). + +The service stores v2 regardless of received payload version. Empty registry still requires `?allow_empty=true`. + +## 4. Compatibility matrix (FR-021/FR-022, SC-006) + +| Workstation | Service | Outcome | +|-------------|---------|---------| +| v2 (new) | new (`payload_versions: [1,2]`) | v2 push, normal flow | +| v2 (new) | old (no `payload_versions`) | abort before any mutation, remediation names the service as the side to upgrade | +| v1 (old) | new | v1 payload accepted, mapped, stored as v2; old CLI needs no changes | +| v1 (old) | old | unchanged legacy behavior (out of this feature's scope) | +| any | any, mount_configured | 409 `mount_configured`, unchanged from today | + +## 5. Push delta cache (workstation side) + +`~/.config/remo/web-service.json`: + +- Adds `"cache_version": 2`; loaders treat any other/missing value as an empty cache (one-time full re-verification push after upgrade — FR-026, clarification #4). +- `fingerprint` = SHA-256 over the canonical sorted-key JSON of the entry's v2 `hostEntry` object (replaces the legacy 7-field digest). +- Unchanged: keyed by `deployment_id`, non-secret, 0600, atomic writes, cached `host_keys` re-sent on every push because the PUT is wholesale. diff --git a/specs/015-registry-v2/contracts/registry-accessor-api.md b/specs/015-registry-v2/contracts/registry-accessor-api.md new file mode 100644 index 0000000..5a249fc --- /dev/null +++ b/specs/015-registry-v2/contracts/registry-accessor-api.md @@ -0,0 +1,92 @@ +# Contract: Registry Accessor API (`remo_cli.core.registry`) + +The single module owning parse/serialize/validate/lock/migrate for the registry (FR-012). All consumers — CLI, providers, web service — go through this surface. Signatures are the contract; bodies are implementation. + +## Types + +```python +@dataclass(frozen=True) +class RegistryView: + hosts: list[KnownHost] # parsed, known-type entries (in-memory model unchanged) + warnings: list[str] # per-entry tolerant-read diagnostics (FR-014, FR-025) + source_format: str # "v2" | "legacy" | "empty" + unknown_entries: int # count of preserved unknown-type entries + +class RegistryError(Exception): ... # base — accessor NEVER raises SystemExit (FR-013) +class RegistryReadError(RegistryError): ... # unreadable file / invalid top-level document +class RegistryValidationError(RegistryError): ... # V1–V6 failed; names field + entry +class RegistryBusyError(RegistryError): ... # lock not acquired within timeout (FR-017) +class RegistryNewerVersionError(RegistryError): ... # file version > supported (FR-023) +``` + +## Read + +```python +def read_registry(*, readonly: bool = False) -> RegistryView +``` + +- `readonly=True` (web service, `remo web check`): never creates directories or files, never migrates, never locks; reads `registry.json` if present else legacy `known_hosts` in place; per-entry problems become `warnings`, structural problems raise `RegistryReadError` / `RegistryNewerVersionError` (caller decides — the web service maps them to the `broken` state). +- `readonly=False` (CLI default): identical read semantics PLUS triggers migration when the source is legacy (see below). Uses the mkdir-capable path helpers. +- Never returns partial torn state: reads see complete old or complete new content (writes are `os.replace`-atomic, FR-018). + +## Write + +```python +def mutate_registry(mutator: Callable[[list[KnownHost]], list[KnownHost]]) -> RegistryView +``` + +The ONLY write primitive. Sequence: acquire lock (≤ 5 s, else `RegistryBusyError`) → read current state (migrating first if legacy) → apply `mutator` → validate all entries (V1–V6; on failure `RegistryValidationError`, disk untouched) → serialize (sorted, deterministic) → atomic write → release lock. Unknown-type entries pass through untouched (mutator never sees them; they are re-emitted verbatim). + +```python +def replace_registry(hosts: list[KnownHost], *, allow_empty: bool = False) -> RegistryView +``` + +Wholesale replacement (web setup PUT apply). Same lock/validate/atomic pipeline; refuses empty without `allow_empty` (mirrors existing setup guard). + +## Migration + +```python +def migrate_if_needed() -> MigrationReport | None # called internally by non-readonly paths +``` + +- CLI-only trigger (clarification #1); no-op when `registry.json` exists (idempotent, FR-010). +- Under lock: re-check → write v2 → rename legacy to `known_hosts.v1.bak` (non-clobbering suffixes) → report. +- `MigrationReport`: entries migrated, backup path, skipped lines (verbatim), push-cache-reset notice flag. The CLI prints it in plain language (FR-025); the accessor itself never prints. +- Both-present resolution per data-model §6: equivalent → complete rename silently; divergent → warning in `RegistryView.warnings`, v2 wins, never merge (FR-024). + +## Locking + +```python +@contextmanager +def registry_lock(timeout_s: float = 5.0): ... +``` + +`fcntl.flock` on `${REMO_HOME}/registry.lock`; `RegistryBusyError` on timeout; one-time warning + unlocked fallback where flock is unsupported (FR-019). Exposed for multi-step callers (migration); ordinary writers just use `mutate_registry`. + +## Compatibility delegates (unchanged public surface, FR-015) + +`core/known_hosts.py` keeps its public functions as thin delegates so existing call sites in `providers/*` and `cli/*` are untouched this feature: + +| Existing function | Delegates to | +|-------------------|--------------| +| `get_known_hosts()` | `read_registry().hosts` | +| `save_known_host(h)` | `mutate_registry(upsert by (type, name))` | +| `remove_known_host(t, n)` | `mutate_registry(drop by (type, name))` | +| `clear_known_hosts_by_type(t)` / `_by_prefix(p)` | `mutate_registry(filter)` | +| `resolve_remo_host_by_name` / `guard_not_added_ssh_host` / `get_aws_region` | unchanged logic over `read_registry().hosts` (their `SystemExit` behavior is unchanged this feature — they are CLI-boundary helpers; the accessor beneath them never exits) | + +Deleted (parser duplication collapse, FR-012 / SC-003): +- `web/discovery.py:_read_known_hosts_readonly` → `read_registry(readonly=True)` +- `web/api/setup.py:_read_registry_readonly` → `read_registry(readonly=True)` +- `core/known_hosts.py` line-parsing internals → the accessor's legacy codec (single implementation, shared with migration and payload-v1 mapping) + +## Guarantees summary + +| Guarantee | Mechanism | +|-----------|-----------| +| No torn reads | same-dir temp file + `os.replace` | +| No lost updates | all writes inside `registry_lock` RMW | +| No side effects in readonly mode | readonly path uses no-mkdir helpers, no lock file, no migration | +| No process termination from the accessor | error taxonomy instead of `SystemExit` | +| Unknown content survives | unknown-type entries round-trip verbatim | +| Deterministic files | sorted entries, sorted keys, fixed indent | diff --git a/specs/015-registry-v2/contracts/registry-file-v2.md b/specs/015-registry-v2/contracts/registry-file-v2.md new file mode 100644 index 0000000..4dd9f0c --- /dev/null +++ b/specs/015-registry-v2/contracts/registry-file-v2.md @@ -0,0 +1,130 @@ +# Contract: Registry File Format v2 + +**File**: `${REMO_HOME}/registry.json` (default `~/.config/remo/registry.json`) +**Encoding**: UTF-8, pretty-printed JSON (2-space indent), entries sorted by `(type, name)`, trailing newline. +**Lock sidecar**: `${REMO_HOME}/registry.lock` (never replaced; content irrelevant). +**Legacy backup**: `${REMO_HOME}/known_hosts.v1.bak` (+ `.1`, `.2`… suffixes when a backup already exists). + +## JSON Schema (draft 2020-12) + +```json +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "remo registry v2", + "type": "object", + "required": ["version", "hosts"], + "properties": { + "version": { "const": 2 }, + "hosts": { + "type": "array", + "items": { "$ref": "#/$defs/hostEntry" } + } + }, + "$defs": { + "hostEntry": { + "type": "object", + "required": ["type", "name", "host", "user", "access"], + "properties": { + "type": { "type": "string", "minLength": 1 }, + "name": { "type": "string", "minLength": 1 }, + "host": { "type": "string", "minLength": 1 }, + "user": { "type": "string", "minLength": 1 }, + "access": { "enum": ["direct", "ssm"] }, + "incus": { + "type": "object", + "properties": { "host_user": { "type": "string" } }, + "additionalProperties": false + }, + "proxmox": { + "type": "object", + "properties": { + "vmid": { "type": "string" }, + "node_user": { "type": "string" } + }, + "additionalProperties": false + }, + "aws": { + "type": "object", + "properties": { + "instance_id": { "type": "string" }, + "region": { "type": "string" } + }, + "additionalProperties": false + }, + "ssh": { + "type": "object", + "properties": { + "port": { "type": "integer", "minimum": 1, "maximum": 65535 }, + "identity_file": { "type": "string" } + }, + "additionalProperties": false + } + } + } + } +} +``` + +Additional invariants not expressible in the schema above: + +- The nested per-type object's key MUST equal the entry's `type` (an `aws` entry may not carry a `proxmox` object). +- `access: "ssm"` is only valid for `type: "aws"`. +- `(type, name)` is unique across `hosts`. +- Entries with a `type` outside the known set are VALID (readers preserve them verbatim on rewrite and skip them in listings); their unknown nested content is preserved untouched. +- Readers MUST reject the document if `version` > 2 ("written by a newer version") and MUST NOT modify the file. +- No control characters or newlines in any string field. + +## Example + +```json +{ + "version": 2, + "hosts": [ + { + "type": "aws", + "name": "buildbox", + "host": "203.0.113.7", + "user": "remo", + "access": "ssm", + "aws": { "instance_id": "i-0abc123def456", "region": "us-east-1" } + }, + { + "type": "hetzner", + "name": "dev1", + "host": "2001:db8::7", + "user": "remo", + "access": "direct" + }, + { + "type": "incus", + "name": "nuc/dev1", + "host": "dev1.incus", + "user": "remo", + "access": "direct", + "incus": { "host_user": "paul" } + }, + { + "type": "proxmox", + "name": "pve1/dev2", + "host": "10.0.0.42", + "user": "remo", + "access": "direct", + "proxmox": { "vmid": "104", "node_user": "root" } + }, + { + "type": "ssh", + "name": "nas", + "host": "nas.lan", + "user": "admin", + "access": "direct", + "ssh": { "port": 2222, "identity_file": "/home/paul/.ssh/id_nas" } + } + ] +} +``` + +Note the second entry: an IPv6 `host` — representable here, corrupting in the legacy colon format. That asymmetry is the point of this contract. + +## Legacy format (read/migrate only) + +`${REMO_HOME}/known_hosts`, one entry per line: `TYPE:NAME:HOST:USER[:INSTANCE_ID[:ACCESS_MODE[:REGION]]]` with per-type slot overloading. Writers of this format are removed by this feature; readers survive for in-place readonly consumption (web service) and as migration input. Mapping tables: [data-model.md §4](../data-model.md). diff --git a/specs/015-registry-v2/data-model.md b/specs/015-registry-v2/data-model.md new file mode 100644 index 0000000..4245068 --- /dev/null +++ b/specs/015-registry-v2/data-model.md @@ -0,0 +1,113 @@ +# Data Model: Versioned Structured Host Registry (Registry v2) + +Companion to [research.md](research.md); the normative on-disk shape lives in [contracts/registry-file-v2.md](contracts/registry-file-v2.md). + +## 1. RegistryDocument + +The top-level content of `${REMO_HOME}/registry.json`. + +| Field | Type | Rules | +|-------|------|-------| +| `version` | int | Required. `2` for this feature. Values > supported range → `RegistryNewerVersionError` on read (FR-023). | +| `hosts` | list[HostEntry] | Required (may be empty). Serialized sorted by `(type, name)` for deterministic diffs. | + +## 2. HostEntry (on-disk) + +Common fields — present on every entry, identical meaning for every type (FR-002): + +| Field | Type | Rules | +|-------|------|-------| +| `type` | str | Required, non-empty. Known types: `incus`, `proxmox`, `aws`, `hetzner`, `ssh`. Unknown types are preserved on rewrite, skipped in listings (FR-014). | +| `name` | str | Required, non-empty. Unique within the registry (uniqueness key: `(type, name)`). Incus/proxmox names keep the existing `host/container` and `node/container` display convention — that is a *naming* convention, not field overloading. | +| `host` | str | Required, non-empty. Hostname, IPv4, or IPv6 literal — any legitimate value round-trips (FR-005). | +| `user` | str | Required, non-empty. SSH login user on the target. | +| `access` | str enum | Required: `"direct"` \| `"ssm"` (FR-004). `"ssm"` is only valid for `type: aws` (validation rule V6). | +| `` | object | Optional nested per-type object; key MUST equal `type` (rule V4). Absent when the type has no extra fields for that entry. | + +Per-type nested objects (FR-003): + +| Type | Nested field | Type | Meaning (legacy slot it replaces) | +|------|--------------|------|-----------------------------------| +| `incus` | `host_user` | str | SSH user on the Incus host machine (legacy `instance_id`) | +| `proxmox` | `vmid` | str | Proxmox container VMID (legacy `instance_id`) | +| `proxmox` | `node_user` | str | SSH user on the Proxmox node (legacy `region`) | +| `aws` | `instance_id` | str | EC2 instance id (legacy `instance_id`) | +| `aws` | `region` | str | AWS region (legacy `region`) | +| `hetzner` | — | — | No type-specific fields today; nested object absent | +| `ssh` | `port` | int | SSH port (legacy `instance_id`; stored as an int in v2) | +| `ssh` | `identity_file` | str | Identity file path (legacy `region`) | + +All nested fields are optional-per-entry unless the provider always writes them; validation enforces *shape* (right names, right types, right parent) rather than presence, matching today's tolerance for partially-filled entries. + +## 3. KnownHost mapping (in-memory model — unchanged, FR-015) + +`models/host.py:KnownHost` keeps its seven string slots. The accessor maps at the serialization boundary only: + +| KnownHost slot | v2 source/target by type | +|----------------|--------------------------| +| `type`, `name`, `host`, `user` | Common fields, verbatim | +| `instance_id` | `incus.host_user` / `proxmox.vmid` / `aws.instance_id` / `str(ssh.port)` / `""` | +| `access_mode` | `access` — normalized: in-memory value is always `"direct"` or `"ssm"` after a v2 load (the legacy implicit-empty convention no longer leaks upward) | +| `region` | `aws.region` / `proxmox.node_user` / `ssh.identity_file` / `""` | + +`KnownHost.from_line`/`to_line` survive strictly as the **legacy codec**, used for: legacy-file parsing, migration input, and payload-v1 compatibility on the service side. No new call sites may use them for the canonical store. + +## 4. Migration mapping (legacy line → HostEntry) + +Keyed on `type` FIRST (research R5 — `instance_id` is meaningless without it): + +1. Fields 1–4 → `type`, `name`, `host`, `user` (verbatim). +2. `access` := `"ssm"` iff the legacy entry is classified SSM by the existing semantics (AWS with instance id and empty-or-`ssm` access mode); else `"direct"`. +3. Overloaded slots → nested object per the §2 table; empty legacy slots produce absent nested fields. +4. Unparseable lines (< 4 fields, empty required fields): NOT migrated — retained in the renamed backup and reported verbatim in the migration notice (FR-009). +5. Lines whose `type` is unknown: migrated as an unknown-type entry `{type, name, host, user, access: "direct", "_legacy_fields": [f5, f6, f7]}` so nothing is dropped (FR-014); listed as skipped in output. + +## 5. Validation rules (write path, FR-016) + +| # | Rule | Failure message names | +|---|------|----------------------| +| V1 | `version` == 2 on serialize | — (internal invariant) | +| V2 | `type`, `name`, `host`, `user` non-empty strings; no control characters or newlines | field + entry name | +| V3 | `(type, name)` unique across `hosts` | duplicate name + type | +| V4 | Nested object key equals `type`; no unrecognized nested keys for known types | field + entry | +| V5 | `ssh.port` integer in 1–65535 | port value | +| V6 | `access == "ssm"` only when `type == "aws"` | entry + explanation | +| V7 | Values are representable in JSON (always true for str/int) — no escaping caveats remain; colon-content checks are deleted, not relaxed | — | + +Validation failure → `RegistryValidationError` before any disk write; file unchanged. + +## 6. File-state model & transitions + +States of `${REMO_HOME}` (drives FR-007/010/011/024 and web/state.py probes): + +| State | `registry.json` | `known_hosts` | CLI read behavior | Web read behavior | +|-------|-----------------|---------------|-------------------|-------------------| +| S0 empty | absent | absent | empty registry, no writes, no backup | empty registry | +| S1 legacy | absent | present | parse legacy → **migrate** (→ S2) | parse legacy in place, log format | +| S2 migrated | present | absent (backup present) | parse v2 | parse v2 | +| S3 both (equivalent) | present | present, host-set equal¹ | complete rename silently (→ S2) | parse v2, log that legacy is ignored | +| S4 both (divergent) | present | present, differing | parse v2 + warning (never merge) | parse v2, log that legacy is ignored | +| S5 newer | present, version > 2 | any | `RegistryNewerVersionError` | maps to `broken` config state | + +¹ *Equivalence* (S3 vs S4): the legacy entries after legacy→v2 mapping form the same set as the v2 file's entries, compared on all v2 fields (`type`, `name`, `host`, `user`, `access`, full per-type object); unparseable lines and warnings are excluded from the comparison (research R6.4). + +Transitions: S1→S2 (CLI migration, under lock, atomic); S3→S2 (rename completion); S4 persists until the user resolves it manually. The web service never causes a transition except via the setup PUT apply (writes `registry.json`, removes legacy mirror file — service-owned state only, research R9). + +## 7. PushCache v2 (`~/.config/remo/web-service.json`) + +| Field | Type | Rules | +|-------|------|-------| +| `cache_version` | int | `2`. Any other value (or absence) → cache treated as empty (research R10), producing the one-time full re-verification push (FR-026). | +| `push_cache` | dict | `{deployment_id: {host_name: {fingerprint, host_keys}}}` — structure unchanged from today. | +| `fingerprint` | str | SHA-256 over the canonical sorted-key JSON serialization of the entry's v2 object (replaces the legacy 7-field concatenation). | + +## 8. Error taxonomy (accessor — no `SystemExit`, FR-013) + +| Exception | Raised when | Typical CLI handling | +|-----------|-------------|----------------------| +| `RegistryValidationError` | V1–V6 fail on write | print message, exit 1 at CLI boundary | +| `RegistryBusyError` | lock not acquired within 5 s | print "registry busy", exit 1 | +| `RegistryNewerVersionError` | file version > supported | print upgrade guidance, exit 1 | +| `RegistryReadError` | file unreadable / top-level JSON invalid | CLI: message + exit 1; web: `broken` state | + +Per-entry problems on read are **warnings on the returned view, never exceptions** (tolerant read, FR-014). diff --git a/specs/015-registry-v2/plan.md b/specs/015-registry-v2/plan.md new file mode 100644 index 0000000..a16f1d9 --- /dev/null +++ b/specs/015-registry-v2/plan.md @@ -0,0 +1,114 @@ +# Implementation Plan: Versioned Structured Host Registry (Registry v2) + +**Branch**: `015-registry-v2` | **Date**: 2026-07-25 | **Spec**: [spec.md](spec.md) + +**Input**: Feature specification from `/specs/015-registry-v2/spec.md` + +## Summary + +Replace the colon-delimited `~/.config/remo/known_hosts` flat file with a versioned JSON registry (`registry.json`, format version 2) whose entries carry explicitly named per-type fields (no overloaded slots) and an explicit `access` attribute. A single new accessor module (`core/registry.py`) owns parsing, serialization, validation, advisory locking, and migration for every consumer — CLI, providers, and web service — collapsing today's three independent parsers. The CLI migrates lazily on first read (legacy file renamed in place as backup); the web service reads both formats in place and never migrates. The adopt/push mirror payload moves to version 2 with an advertised-capability handshake so version skew fails fast before mutation, while an upgraded service still accepts v1 payloads. The `KnownHost` dataclass remains the in-memory model; only serialization, parsing, and storage semantics change. + +## Technical Context + +**Language/Version**: Python 3.11+ (`from __future__ import annotations`, type hints; existing codebase conventions) + +**Primary Dependencies**: Stdlib only for this feature — `json` (format), `fcntl` (advisory locking), `os.replace` (atomic writes). No new runtime dependencies. Web-side touches use the existing FastAPI/Pydantic surface (already in the `web` extra). + +**Storage**: Flat file `${REMO_HOME}/registry.json` (human-readable, diffable JSON, 2-space indent, deterministic entry ordering). Legacy `${REMO_HOME}/known_hosts` retained only as migration source / renamed backup. Sidecar lock file `${REMO_HOME}/registry.lock`. + +**Testing**: pytest (existing suite layout under `tests/`). New: migration matrix tests, round-trip property tests, multiprocess concurrency stress test, setup-API version-skew contract tests. + +**Target Platform**: Linux + macOS workstations (CLI); Linux containers (web service). `fcntl` is available on both; Windows is not a supported platform today. + +**Project Type**: Single Python package (`src/remo_cli`, src layout) + existing FastAPI web sub-package. No frontend changes. + +**Performance Goals**: Registry read/parse/write overhead < 100 ms per command invocation at 200 entries (SC-008). Lock acquisition bounded at 5 s (FR-017). + +**Constraints**: No database, no new runtime deps; file must remain human-diffable (SC-007); readonly consumers must never write or mkdir (FR-011/FR-013); every write atomic via same-directory temp file + `os.replace` (FR-018); migration crash-safe and idempotent (FR-010). + +**Scale/Scope**: ≤ 200 registry entries (personal-fleet tool). ~10 write call sites in `providers/*`, 3 read-path implementations to collapse, 2 web endpoints (setup status/PUT) and the push engine (`core/web_adopt.py`) to move to payload v2. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +The constitution (v1.0.0) is Ansible-centric; Principle I (Defensive Variable Access) is not applicable — this feature touches no Ansible code. The remaining principles apply as follows and are satisfied by the design: + +| Principle | Application to this feature | Status | +|-----------|-----------------------------|--------| +| II. Test All Conditional Paths | Migration has many branches (legacy-only / v2-only / both-present / garbage lines / newer-version / readonly). The migration test matrix (quickstart §3, data-model §6) exercises every branch on fresh state AND pre-existing state, matching the "fresh systems AND systems with existing state" rule. | PASS | +| III. Idempotent by Default | Migration is idempotent and crash-safe (FR-010): re-runs are no-ops, interrupted runs converge on next read; backups are never clobbered (FR-009); all writes check-then-act under a lock. | PASS | +| IV. Fail Fast with Clear Messages | Validation happens before anything touches disk (FR-016); version-skew push aborts before mutation with which-side-to-upgrade remediation (FR-021); "registry busy", "written by a newer version", and skipped-line warnings all name the problem and the fix (FR-025). | PASS | +| V. Documentation Reflects Reality | README registry documentation, `docs/web-session-interface.md` (adoption payload), and CLAUDE.md's "Flat file (colon-delimited)" technology line must be updated in the same change set; called out as explicit tasks. | PASS (tracked) | + +No violations → Complexity Tracking table not required. + +**Post-Phase-1 re-check (2026-07-25)**: Design artifacts introduce no new projects, no new dependencies, and no deviations from the above. GATE still PASS. + +## Project Structure + +### Documentation (this feature) + +```text +specs/015-registry-v2/ +├── plan.md # This file +├── research.md # Phase 0 output +├── data-model.md # Phase 1 output +├── quickstart.md # Phase 1 output +├── contracts/ +│ ├── registry-file-v2.md # On-disk format contract (JSON Schema + examples) +│ ├── registry-accessor-api.md # Python API contract for core/registry.py +│ └── mirror-payload-v2.md # Setup API payload v2 + version-negotiation contract +└── tasks.md # Phase 2 output (/speckit-tasks — NOT created by /speckit-plan) +``` + +### Source Code (repository root) + +```text +src/remo_cli/ +├── core/ +│ ├── registry.py # NEW — single accessor: parse/serialize/validate/lock/migrate +│ ├── known_hosts.py # SLIMMED — public functions become thin delegates to registry.py +│ ├── config.py # get_registry_path()/readonly variants added beside known_hosts paths +│ └── web_adopt.py # Payload v2, capability handshake, push-cache v2 reset +├── models/ +│ └── host.py # KnownHost unchanged as model; from_line/to_line retained for +│ # legacy parse + payload-v1 compatibility only (marked legacy) +├── providers/ +│ ├── incus.py # Write call sites move to registry mutate API (mechanical) +│ ├── hetzner.py # " +│ ├── aws.py # " +│ ├── proxmox.py # " +│ └── added.py # " +└── web/ + ├── discovery.py # Drop private parser → registry.read_registry(readonly=True) + ├── state.py # Registry probe accepts registry.json OR legacy known_hosts + ├── check.py # Same probe update; verify output mentions format found + └── api/ + └── setup.py # Drop private parser; PUT accepts payload v1+v2, stores v2; + # status advertises payload_versions + +tests/ +├── unit/ +│ ├── core/ +│ │ ├── test_registry_format.py # NEW — round-trip, validation, tolerant parse, versions +│ │ ├── test_registry_migration.py # NEW — migration matrix (all types × field combos) +│ │ └── test_registry_locking.py # NEW — lock timeout, degradation, crash atomicity +│ ├── providers/ +│ │ ├── test_provider_registry_entries.py # NEW — per-provider save-path fixtures (R5 risk pin) +│ │ └── test_added_ipv6.py # NEW — IPv6 added-host end-to-end +│ └── web/ +│ └── test_registry_readonly.py # NEW — both formats, ro volume, parity, broken mapping +├── integration/ +│ ├── test_registry_concurrency.py # NEW — multiprocess lost-update stress +│ └── test_setup_payload_versions.py # NEW — v1/v2/unknown payloads against setup API +├── perf/ +│ └── test_registry_perf.py # NEW — 200-entry round-trip < 100 ms (SC-008) +└── (existing tests updated where they fabricate legacy known_hosts files) +``` + +**Structure Decision**: Single-project src layout (existing). The feature adds one new core module and three contract documents; everything else is modification-in-place. No new packages, services, or build steps. + +## Complexity Tracking + +> Not required — Constitution Check passed with no violations. diff --git a/specs/015-registry-v2/quickstart.md b/specs/015-registry-v2/quickstart.md new file mode 100644 index 0000000..ff12e19 --- /dev/null +++ b/specs/015-registry-v2/quickstart.md @@ -0,0 +1,118 @@ +# Quickstart: Validating Registry v2 + +Runnable scenarios proving the feature end-to-end. Schema details: [contracts/registry-file-v2.md](contracts/registry-file-v2.md); API surface: [contracts/registry-accessor-api.md](contracts/registry-accessor-api.md); push wire contract: [contracts/mirror-payload-v2.md](contracts/mirror-payload-v2.md). + +## Prerequisites + +```bash +uv sync --all-extras # includes web extra for service-side scenarios +export REMO_HOME=$(mktemp -d) # isolate every scenario from your real config +``` + +## 1. Fresh install (S0 → S2) + +```bash +uv run remo incus list +``` + +**Expected**: empty listing, no error, no migration notice; `$REMO_HOME` contains no backup file; `registry.json` is created only once a host is actually saved (no spurious writes on read). + +## 2. Migration from a populated legacy file (S1 → S2) — the P1 scenario + +```bash +cat > "$REMO_HOME/known_hosts" <<'EOF' +incus:nuc/dev1:dev1.incus:remo:paul:direct +proxmox:pve1/dev2:10.0.0.42:remo:104:direct:root +aws:buildbox:203.0.113.7:remo:i-0abc123def456:ssm:us-east-1 +hetzner:dev1:198.51.100.9:remo +ssh:nas:nas.lan:admin:2222:direct:/home/paul/.ssh/id_nas +incus:old/box:box.incus:remo:paul:ssm +proxmox:old/pct:10.0.0.9:remo:101::root +this line is garbage +EOF +uv run remo incus list +``` + +The first five lines match what current providers actually write (every save path sets `access_mode` explicitly — `direct` or `ssm`). The two `old/…` lines are legacy variants that only exist in files written by older versions or by hand: a non-AWS line carrying a literal `ssm` (the `to_line` back-fill quirk when `instance_id` was set with an empty access mode) and a 7-field line with an empty access-mode slot. Migration's type-first rule must classify both as `access: "direct"`. + +**Expected**: +- One-time migration notice: 7 entries migrated, backup name (`known_hosts.v1.bak`), the garbage line quoted as skipped, and the "next `remo web push` will re-verify all instances" note. +- `registry.json` exists and validates against the contract schema; the proxmox entry has `proxmox.vmid: "104"` and `proxmox.node_user: "root"`; the aws entry has `access: "ssm"`; the ssh entry has `ssh.port: 2222` (integer) and `ssh.identity_file`; both `old/…` legacy-variant entries have `access: "direct"` despite their odd access-mode slots. +- `known_hosts` is gone; `known_hosts.v1.bak` is byte-identical to the original (including the garbage line). +- Re-running any command produces **no** second migration notice and no new backup (idempotency). + +## 3. Automated migration matrix + +```bash +uv run pytest tests/unit/core/test_registry_migration.py -v +``` + +**Expected**: green across the matrix — all 5 types × optional-field combinations (4/6/7-field lines), garbage lines, unknown types (preserved, not dropped), empty vs missing file, pre-existing backup (numeric-suffix, never clobbered), both-present equivalent (silent rename completion) and divergent (v2 wins + warning, no merge), newer-version file (clear error, file untouched). Provider save-path fixtures pin the exact bytes each provider writes today (research R5 risk). + +## 4. Values that corrupted the legacy format (US2) + +```bash +uv run remo add v6box 'admin@[2001:db8::7]' # OpenSSH-style brackets, optional [:port] +uv run remo add v6bare 2001:db8::8 --user admin # bare IPv6 TARGET (no port suffix) also accepted +uv run remo incus list # or `remo add`'s own listing +``` + +**Expected**: both IPv6 addresses round-trip intact in `registry.json` and in listings; `remo remove v6box` cleans up. Note: today `remo add`'s TARGET parser splits `[user@]host[:port]` at the first colon (providers/added.py), so IPv6 fails before the registry is even involved — T020 fixes the parser (bracket form plus bare-IPv6 heuristic) as part of this story. Unit round-trip/property tests: + +```bash +uv run pytest tests/unit/core/test_registry_format.py -v +``` + +## 5. Concurrency: no lost updates (US4) + +```bash +uv run pytest tests/integration/test_registry_concurrency.py -v +``` + +**Expected**: multiprocess writers upserting disjoint entry sets in a loop; final registry always contains the union; file parses cleanly on every iteration; a deliberately held lock makes a second writer fail with "registry busy" after ~5 s (not proceed on stale data); SIGKILL mid-write leaves the previous complete state. + +## 6. Web service: readonly consumption of both formats (US3) + +```bash +# Legacy file on a read-only mount — service must read in place, never write +chmod 555 "$REMO_HOME" +REMO_WEB_ALLOWED_HOSTS=127.0.0.1 uv run remo web check +chmod 755 "$REMO_HOME" +``` + +**Expected**: `remo web check` reports the registry as readable (naming which format it found), performs zero writes/mkdirs against `$REMO_HOME` (verify mtimes unchanged), and one seeded malformed entry degrades to a warning, not a failure. Repeat with a `registry.json` in place of the legacy file — identical host set reported. Parser-collapse check (SC-003): + +```bash +grep -rn "from_line\|partition(\":\")" src/remo_cli/web/ # expect no private registry parsing left +``` + +## 7. Push payload versions & skew (US5) + +```bash +uv run pytest tests/integration/test_setup_payload_versions.py -v +``` + +**Expected** (per the [compatibility matrix](contracts/mirror-payload-v2.md#4-compatibility-matrix-fr-021fr-022-sc-006)): +- v1 payload → accepted, stored as `registry.json` v2, legacy mirror file removed. +- v2 payload → accepted; wire entries match the file schema exactly. +- version 3 payload → 400 `unsupported_payload_version`, prior mirror intact and served. +- Status includes `payload_versions: [1, 2]`; the push flow aborts before any keyscan/PUT when the field is missing (old-service simulation), with remediation naming the service as the side to upgrade. +- After a real (or test-harness) migration, `web-service.json` without `cache_version: 2` is treated as empty → first push re-verifies every instance and is idempotent on immediate re-run. + +## 8. Performance guard (SC-008) + +```bash +uv run pytest tests/perf/test_registry_perf.py -v +``` + +**Expected**: generated 200-entry registry read+validate+write round-trip under 100 ms. + +## 9. Full gates + +```bash +uv run pytest +uv run mypy src/remo_cli +uv run ruff check src/remo_cli +``` + +**Expected**: all green; docs updated in the same change set (README registry section, `docs/web-session-interface.md` payload examples, CLAUDE.md storage line) per Constitution Principle V. diff --git a/specs/015-registry-v2/research.md b/specs/015-registry-v2/research.md new file mode 100644 index 0000000..c62ab3f --- /dev/null +++ b/specs/015-registry-v2/research.md @@ -0,0 +1,115 @@ +# Phase 0 Research: Versioned Structured Host Registry (Registry v2) + +All Technical Context unknowns are resolved below. Each entry records the decision, rationale, and alternatives considered. Line references describe the codebase as of branch point (`main` @ 5eb6848). + +## R1. File format and naming + +**Decision**: JSON. Canonical file `${REMO_HOME}/registry.json` — a single object `{"version": 2, "hosts": [...]}` — pretty-printed with 2-space indent, `ensure_ascii=False`, entries sorted by `(type, name)`, trailing newline. + +**Rationale**: +- Stdlib `json` reads AND writes — no new runtime dependency (project convention: "No new runtime deps", see 005-provider-snapshots precedent; pyproject keeps deps minimal). +- Pretty-printed JSON with one key per line is line-diffable (SC-007): a changed host attribute is a one-line diff; sorted entry order makes diffs deterministic and prevents spurious reorder noise. +- The web setup API already exchanges registry entries as JSON (Pydantic models in `web/api/setup.py`), so file format and wire format share one representation. + +**Alternatives considered**: +- **TOML**: stdlib `tomllib` is read-only (Python 3.11); writing needs `tomli-w` — a new dependency. Rejected. +- **YAML**: needs PyYAML (new dep), ambiguous scalar typing (the Norway problem) in a file where values are security-relevant hostnames. Rejected. +- **JSON Lines**: no natural place for a top-level version field; per-entry version repeats. Rejected. +- **Keep colon format with escaping**: preserves the corrupting format's core problem (positional overloading) and adds an escaping scheme no other tool understands. Rejected. + +## R2. Backup naming and non-clobbering + +**Decision**: At migration, the legacy file is renamed in place: `known_hosts` → `known_hosts.v1.bak` (same directory, `os.rename`). If `known_hosts.v1.bak` already exists, the new backup gets a numeric suffix (`known_hosts.v1.bak.1`, `.2`, …) — never overwrite (FR-009). + +**Rationale**: Same-directory rename is atomic on POSIX and preserves the file byte-for-byte, including lines migration could not interpret. The `.v1.bak` name states what it is; numeric suffixes handle the re-migration edge case (rollback → downgrade writes a fresh legacy file → re-upgrade migrates again). + +**Alternatives**: leave legacy untouched at original path (rejected in clarification — makes both-present the permanent state and neuters FR-024); move to a `backups/` subdir (more moving parts, breaks the "everything in one config dir" convention). + +## R3. Advisory locking mechanism + +**Decision**: `fcntl.flock(LOCK_EX | LOCK_NB)` on a dedicated sidecar file `${REMO_HOME}/registry.lock`, acquired in a retry loop (50 ms interval) with a 5-second total budget (FR-017, clarification #3), then `RegistryBusyError` with the holder-agnostic message "registry is busy — another remo process is writing; retry in a moment". Lock wraps the whole read-modify-write; plain reads do not take the lock (atomic `os.replace` writes make un-locked reads always see a complete file, FR-018). + +**Rationale**: +- The lock must live on a file that is never replaced — `registry.json` itself is swapped by `os.replace` on every write, which would make a lock on it meaningless across writers. A sidecar avoids that trap. +- `flock` is available on Linux and macOS (both supported platforms; Windows is not supported today) and is advisory, matching FR-017's scope (same-machine cooperation, not enforcement). +- Read paths staying lock-free keeps readonly mode truly side-effect-free (FR-013): a readonly consumer on a read-only volume could not create/open a lock file anyway. + +**Degradation (FR-019)**: if `flock` raises `OSError` (`ENOLCK`, `EOPNOTSUPP` — e.g. some network filesystems), emit a one-time warning ("registry locking unavailable on this filesystem; concurrent writes may race") and proceed unlocked — current behavior, no regression. + +**Alternatives**: `os.open(O_CREAT|O_EXCL)` lockfile protocol (needs stale-lock reaping after crashes — flock releases automatically on process death); `lockf`/`fcntl` byte-range locks (same availability, more foot-guns with fd inheritance); third-party `filelock` (new dependency). Rejected. + +## R4. Entry schema shape: flat common fields + per-type object + +**Decision**: Each host entry has common fields (`type`, `name`, `host`, `user`, `access`) plus at most one nested object named after the type carrying type-specific fields (see data-model.md for the full field tables): + +```json +{ + "type": "proxmox", + "name": "pve1/dev1", + "host": "pve1.lan", + "user": "remo", + "access": "direct", + "proxmox": { "vmid": "104", "node_user": "root" } +} +``` + +**Rationale**: The nested per-type object makes field ownership self-describing (a `vmid` can only appear under `proxmox`), gives validation a natural shape ("the nested key must equal `type`; its fields must match that type's table"), and lets unknown-type entries round-trip wholesale (FR-014): the serializer re-emits the raw object untouched. + +**Alternatives**: fully flat entries with type-prefixed field names (`proxmox_vmid`) — noisier, weaker validation story; a generic `extra: {}` bag — reintroduces "meaning depends on type" ambiguity the feature exists to kill. Rejected. + +## R5. `access` attribute semantics and legacy derivation + +**Decision**: `access` is a required enum: `"direct"` or `"ssm"` (FR-004). Migration derives it by reusing the exact existing semantics — an entry is SSM iff the current `is_direct_access`/`to_line` logic classifies it as SSM (today that is only AWS-with-instance-id-and-empty-or-ssm-access-mode) — so migration is a pure re-encoding of current behavior, never a behavior change. + +**Verified during analysis (2026-07-25)**: every *current* provider save path sets `access_mode` explicitly — incus `"direct"` (providers/incus.py:285, :649), proxmox `"direct"` (proxmox.py:372, :807), added-ssh `"direct"` (added.py:277), aws `"ssm"` or tag-derived (aws.py:464, :621, :752) — so freshly written legacy lines always carry a literal access mode. The `to_line` back-fill (`access_mode="ssm"` whenever `instance_id` is set and `access_mode` is empty, models/host.py) is therefore a *latent* quirk affecting only files written by older remo versions or by hand. The migration mapper keys on `type` FIRST, and the test matrix (T015/T016, quickstart §2 `old/…` fixture lines) must include both legacy variants: a non-AWS line with literal `ssm`, and a 7-field line with an empty access-mode slot — both map to `access: "direct"`. + +## R6. Migration flow, crash-safety, and both-present resolution + +**Decision** (CLI only — clarification #1): + +1. Read path resolution order: `registry.json` exists → parse v2 (done). Else `known_hosts` exists → parse legacy tolerantly; if invoked via the CLI (write-capable accessor default), migrate. +2. Migration, under the registry lock: re-check state (another process may have migrated during the lock wait) → write `registry.json` atomically (temp file + `os.replace`) → rename `known_hosts` → `known_hosts.v1.bak` → print notice (entries migrated, backup name, any skipped lines verbatim, and the one-time "next `remo web push` will re-verify all instances" note per FR-026). +3. **Ordering rationale**: write-new-then-rename-old means a crash at any point leaves data reachable: crash before the write → legacy still authoritative; crash between write and rename → both files present. +4. **Both-present resolution (FR-024)**: `registry.json` always wins. On CLI read with both present: if the legacy file's parsed host set is **equivalent** to the v2 host set, this is an interrupted migration — silently complete the rename. Equivalence is defined as: the set of legacy entries after legacy→v2 mapping equals the v2 file's entry set, compared on ALL v2 fields (`type`, `name`, `host`, `user`, `access`, and the full per-type object); unparseable legacy lines and read warnings are excluded from the comparison. Any inequality (rollback-then-re-upgrade divergence) → warn: which file is in use, that the legacy file is being ignored, and how to resolve (delete or re-add hosts) — never merge (edge case list). +5. The web service never migrates and never resolves both-present — it reads `registry.json` if present, else legacy, and logs which format it used (FR-011, FR-025). + +**Idempotency (FR-010)**: step 1 makes completed migrations no-ops; step 2's re-check under lock makes concurrent first-runs safe; step 4 converges the interrupted case. + +## R7. Newer-version file handling + +**Decision**: `version` field greater than 2 → `RegistryNewerVersionError` ("this registry was written by a newer remo (format N); upgrade remo or restore the backup"), file untouched (FR-023). CLI surfaces it and exits non-zero at the CLI boundary (the accessor itself never calls `sys.exit` — FR-013). Web service maps it to the `broken` configuration-state path so readiness/`remo web check` report it with the same remediation text. + +## R8. Single accessor and call-site strategy + +**Decision**: New module `src/remo_cli/core/registry.py` owns everything (contract: `contracts/registry-accessor-api.md`). Existing public functions in `core/known_hosts.py` (`get_known_hosts`, `save_known_host`, `remove_known_host`, `clear_known_hosts_by_type`, `clear_known_hosts_by_prefix`, resolver/guard helpers) become thin delegates so the ~30 existing call sites in `providers/*` and `cli/*` keep working unchanged (FR-015). The three parser sites collapse: +- `core/known_hosts.py` internal parsing → delegates to `registry` +- `web/discovery.py:68-103` private parser → `registry.read_registry(readonly=True)` +- `web/api/setup.py:165-180` private parser → same +`web/state.py` and `web/check.py` registry probes accept either file (R6.5). + +**Rationale**: delegation keeps this feature's diff mechanical at call sites, deferring the larger write-API rework (clear-then-resave → reconcile) to roadmap Spec 2, which is explicitly out of scope here. The one new write primitive `mutate_registry(fn)` (lock → read → apply → validate → atomic write) is introduced and used by the delegates internally so every existing write becomes lost-update-safe (FR-017) without changing provider code semantics. + +## R9. Mirror payload v2 and version negotiation + +**Decision** (contract: `contracts/mirror-payload-v2.md`): +- Payload v2: `{"version": 2, "registry": [], "host_keys": {...}}` — same entry schema as the file (R4). +- `GET /api/v1/setup/status` gains `"payload_versions": [1, 2]`. The workstation reads it before pushing; a service response without the field implies `[1]` (older service) → the CLI aborts before any mutation with "the remo-web deployment only speaks registry format 1 — upgrade the container image, then re-run push" (FR-021). +- The upgraded service's `PUT /setup/registry` accepts **both** v1 and v2 payloads; v1 entries are mapped through the same legacy→v2 mapper used by file migration, and the service always stores v2 (FR-022). Unknown versions → 400 with a body naming supported versions. +- On a successful apply, the service writes `registry.json` and removes any legacy `known_hosts` mirror file in the same apply sequence (the mirror is service-owned replaceable state, not user data; ordering: service known_hosts trust file first, `registry.json` second, legacy-file removal last — a crash mid-sequence still leaves a readable superset, converging on re-push). + +**Rationale**: capability advertisement via status (which the push flow already fetches first — `core/web_adopt.py:1106`) gives fail-fast skew detection with zero extra round trips; accept-old-payload on the service preserves the "upgrade service first" direction from the spec's assumptions. + +## R10. Push delta-cache reset + +**Decision**: `~/.config/remo/web-service.json` gets `"cache_version": 2` and fingerprints computed over the canonical v2 entry serialization (sorted-key JSON of the entry object). The loader discards any cache whose `cache_version` != 2 — same discard-on-unknown-shape behavior the file already has for the obsolete 011 format (`core/web_adopt.py:761-786`). Result: first push after upgrade sees an empty cache, treats all instances as changed, re-keyscans and re-authorizes (idempotent — the `remo-web@` marker replacement makes re-authorization a byte-level no-op), per clarification #4 and FR-026. + +## R11. Performance (SC-008) + +**Decision**: no special engineering needed — parsing a 200-entry JSON file is single-digit milliseconds with stdlib `json`. Add one regression test that generates a 200-entry registry and asserts read+write round-trip completes under the budget, to catch accidentally quadratic validation. + +## R12. What is explicitly NOT changing (scope guards) + +- Provider sync/write *semantics* (clear-then-resave patterns) — Spec 2 (sync-as-reconcile) territory; this feature only makes them lost-update-safe via the shared mutate primitive. +- `KnownHost` in-memory field layout and its overloaded-slot accessors — call sites keep working; per-type meaning now lives in the (de)serialization mapping tables (FR-015). +- The adopt/push workflow shape (verbs, keyscan, authorized_keys management) — Spec 3 territory; only the payload schema and cache format change here. +- `remo-host` protocol, SSH option building, discovery/terminal flows — untouched. diff --git a/specs/015-registry-v2/spec.md b/specs/015-registry-v2/spec.md new file mode 100644 index 0000000..0c0ebf1 --- /dev/null +++ b/specs/015-registry-v2/spec.md @@ -0,0 +1,195 @@ +# Feature Specification: Versioned Structured Host Registry (Registry v2) + +**Feature Branch**: `015-registry-v2` + +**Created**: 2026-07-25 + +**Status**: Draft + +**Input**: User description: "Replace the colon-delimited flat-file host registry (~/.config/remo/known_hosts) with a versioned structured registry file (e.g. registry.json with {\"version\": 2, \"hosts\": [...]}) that eliminates per-type field overloading. Today the KnownHost fields are polymorphic by provider type: instance_id holds an Incus host SSH user, a Proxmox VMID, an EC2 instance id, or an SSH port depending on type, and region holds an AWS region, a Proxmox node user, or an identity-file path. The v2 format must give each provider type explicitly named, typed fields so no field is ever overloaded, and must make the \"SSM access\" rule an explicit stored attribute instead of the implicit \"instance_id set + access_mode empty\" convention currently re-derived in models/host.py, core/web_adopt.py, and web/api/setup.py. Requirements: (1) transparent one-time migration from the legacy colon format on first read, preserving all existing entries including ssh-type \"added\" hosts; (2) a single registry accessor module used by ALL consumers — CLI, providers, and the web service — with an explicit readonly mode that never mkdirs, never raises SystemExit, and parses tolerantly, so the three independent parser implementations (core/known_hosts.py, web/discovery.py, web/api/setup.py) collapse into one; (3) input validation on write (reject or escape values that would corrupt the format — the legacy format silently corrupts on any value containing a colon, e.g. IPv6 literals); (4) advisory file locking around read-modify-write sequences so concurrent writers (two syncs, or CLI sync racing an adopted web service's registry PUT) cannot drop each other's entries; (5) the adopt/push mirror payload and the web service's registry PUT must speak the v2 format, with a compatibility story for a workstation and web service on different versions. The KnownHost dataclass survives as the in-memory model; only serialization and parsing move. The file must remain human-readable and diffable (no database)." + +## Clarifications + +### Session 2026-07-25 + +- Q: Which component performs the legacy→v2 migration? → A: Only the CLI migrates; the web service reads both formats in place tolerantly and never migrates, regardless of write access. +- Q: What happens to the legacy file after migration? → A: It is renamed in place to a backup name in the same directory (exact name is a design-phase decision), so "both files present" signals an anomaly rather than the permanent normal state. +- Q: How long is the bounded wait for the registry lock? → A: 5 seconds by default, then fail with a clear "registry busy" message. +- Q: What happens to the web-push delta cache after migration? → A: It is reset; the first push after upgrade treats all instances as changed and re-verifies/re-authorizes them (idempotent, one-time, mentioned in the migration notice). +- Q: What registry scale must remain performant? → A: Up to 200 entries with no perceptible added latency (under 100 ms registry overhead per command). + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Existing User Upgrades Seamlessly (Priority: P1) + +An operator with an existing registry containing a mix of environments — Incus containers, Proxmox containers, AWS instances (SSM-accessed), Hetzner servers, and manually added SSH hosts — upgrades remo. The first time they run any remo command, the registry is migrated to the new versioned format automatically. Every environment they had registered is still present, every command (shell, cp, list, sync, snapshot, web push) behaves exactly as before, and they never see a migration prompt or perform a manual step. + +**Why this priority**: Migration safety is the make-or-break requirement. If a single existing entry is lost or altered during upgrade, users lose access to running infrastructure they depend on. Nothing else in this feature matters if upgrade is not lossless and invisible. + +**Independent Test**: Populate a legacy-format registry with at least one entry of every environment type (including all optional-field combinations: with/without instance id, access mode, region, and ssh-type hosts with ports and identity paths). Upgrade, run a read command, and verify the new-format file contains an equivalent entry for every original line and that connecting to each host still works. + +**Acceptance Scenarios**: + +1. **Given** a legacy registry with entries of all environment types, **When** the user runs any remo command that reads the registry, **Then** the registry is converted to the versioned format with every entry preserved, and the command's output is identical to what it would have shown before migration. +2. **Given** a legacy registry containing an ssh-type "added" host with a custom port and identity file, **When** migration runs, **Then** the migrated entry stores the port and identity file under explicitly named attributes and connections to that host still use them. +3. **Given** a legacy registry containing unparseable garbage lines alongside valid lines, **When** migration runs, **Then** valid entries are migrated, the original file is preserved as a backup, and the user is informed that unrecognized lines were set aside rather than silently discarded. +4. **Given** a system that has already migrated, **When** any subsequent command runs, **Then** no migration occurs again and no legacy file is re-created. + +--- + +### User Story 2 - Registry Values Can No Longer Corrupt the File (Priority: P2) + +An operator registers a host whose address is an IPv6 literal, or adds an SSH host whose identity file path contains unusual characters. The registry stores and returns these values faithfully. Attempts to store a value that cannot be represented are rejected at write time with a clear message — never silently written in a way that corrupts the file or truncates the value. + +**Why this priority**: The legacy format silently corrupts on any value containing a colon. This is a live data-loss bug class; the new format's core promise is that any legitimate value round-trips exactly. + +**Independent Test**: Register hosts with IPv6 addresses, paths containing spaces and special characters, and boundary-length names; confirm each value reads back byte-identical and that a deliberately invalid value (e.g., containing a newline or empty required field) is rejected with an actionable error before anything is written. + +**Acceptance Scenarios**: + +1. **Given** a host whose address is an IPv6 literal, **When** it is saved and read back, **Then** the address is intact and connections use it correctly. +2. **Given** an attempt to save an entry with an invalid or unrepresentable value, **When** the write is attempted, **Then** it is rejected with a message naming the field and the problem, and the registry file is unchanged. +3. **Given** any environment type, **When** its entry is inspected, **Then** every attribute lives under a name that states what it is (e.g., a VMID is stored as a VMID, an SSH port as a port), and whether the host is reached via direct SSH or a brokered channel (SSM) is an explicit stored attribute, not inferred from which other fields happen to be empty. + +--- + +### User Story 3 - One Registry Reader for CLI and Web Service (Priority: P3) + +The web service (running in a container, possibly with the registry mounted read-only) and the CLI both read the registry through the same accessor. The web service's read path never attempts to create directories or files, never terminates the process on a bad entry, and tolerates individually malformed entries by skipping them while surfacing the rest — identical tolerant behavior to the CLI's read path, because it is the same code. + +**Why this priority**: Today three independent parser implementations exist and can drift apart. Collapsing them removes a whole category of "CLI sees the host but the web service doesn't" bugs, and is a prerequisite for the format change to be safe (a format change with three parsers means three migration bugs). + +**Independent Test**: Point the web service at a registry file on a read-only volume in both legacy and new formats; verify discovery lists the same hosts the CLI lists, no write is ever attempted against the read-only volume, and a corrupted single entry degrades to "that entry skipped" rather than a service failure. + +**Acceptance Scenarios**: + +1. **Given** a registry on a read-only volume in the legacy format, **When** the web service reads it, **Then** all entries are visible to discovery without any migration write being attempted, and the file is left untouched. +2. **Given** a registry with one malformed entry among valid ones, **When** either the CLI or the web service reads it, **Then** both surface the same set of valid entries and neither crashes or exits. +3. **Given** the codebase after this feature, **When** the registry parsing logic is located, **Then** it exists in exactly one module, and the previously independent read implementations delegate to it. + +--- + +### User Story 4 - Concurrent Writers Don't Lose Entries (Priority: P4) + +An operator runs `remo aws sync` while a `remo incus sync` is still in flight in another terminal — or the adopted web service applies a pushed registry update at the same moment the CLI modifies the registry. When all operations complete, the registry contains the union of what each writer intended: no writer's entries have been silently dropped by another writer's read-modify-write cycle. + +**Why this priority**: Lost-update races exist today but are low-frequency for a single operator. They become more likely as the web service also writes the registry (adoption pushes) and as sync operations grow. This must be fixed before higher-level features (reconcile-based sync, drift tracking) build on the registry. + +**Independent Test**: Launch multiple concurrent registry mutations targeting different environment types in a loop; verify the end state always contains every expected entry and the file is never left in a partially written or unparseable state. + +**Acceptance Scenarios**: + +1. **Given** two concurrent operations each adding/updating entries for different environment types, **When** both complete, **Then** the registry contains both writers' results. +2. **Given** a writer that holds the registry lock, **When** a second writer arrives, **Then** the second writer waits briefly and proceeds, or fails with a clear "registry is busy" message after a bounded wait — it never proceeds on stale data. +3. **Given** a process killed mid-write, **When** the next reader opens the registry, **Then** it sees the complete previous state (never a torn/partial file). + +--- + +### User Story 5 - Workstation and Web Service on Different Versions (Priority: P5) + +An operator upgrades their workstation CLI before upgrading their remo-web container (or vice versa), then runs a registry push. The push either succeeds correctly or fails fast with a message that names the version mismatch and tells the operator which side to upgrade. At no point does a mismatch silently corrupt the service's mirrored registry or strand the service in a broken state. + +**Why this priority**: Version skew between workstation and service is guaranteed to happen in the field (they are upgraded independently), but it is an upgrade-window scenario rather than a daily-use path. + +**Independent Test**: Run a push from a new-format workstation against an old-format service and the reverse; verify each combination either interoperates or produces a single clear remediation message, and the service remains healthy afterwards. + +**Acceptance Scenarios**: + +1. **Given** an upgraded workstation and an older web service, **When** the workstation pushes the registry, **Then** the outcome is either a successful, correct mirror or an explicit version-mismatch error naming the remediation ("upgrade the web service") — never a partial or corrupted mirror. +2. **Given** an upgraded web service and an older workstation, **When** the older workstation pushes a legacy-format payload, **Then** the service accepts it and stores it correctly in the new format. +3. **Given** a version-mismatch rejection, **When** the operator checks the service, **Then** the service still serves its previous registry state and reports healthy. + +--- + +### Edge Cases + +- Both a legacy file and a new-format file exist side by side (e.g., interrupted migration, or a rollback then re-upgrade): the system must define a single winner deterministically, tell the user which was used, and never merge them silently. +- The new-format file declares a version newer than the running remo understands (user downgraded the CLI): reads must fail with a clear "this registry was written by a newer remo" message rather than misparsing; the file must not be modified. +- The registry directory is on a read-only volume and still contains only a legacy file: readonly consumers must parse it in place indefinitely — migration must not be a precondition for reading. +- An entry has an environment type the running version doesn't recognize (written by a newer version or a future provider): readers must preserve it on rewrite and skip it in listings rather than dropping or crashing. +- Empty registry file vs. missing registry file: both must be handled as "no hosts registered" without error, and neither state may cause a spurious migration or backup. +- The migration backup already exists from a previous migration attempt: a re-migration must not overwrite the older backup silently. +- Lock acquisition on filesystems that do not support advisory locking (some network mounts): the system must degrade to current (unlocked) behavior with a one-time warning rather than refusing to operate. +- A legacy entry whose overloaded fields are ambiguous or contradictory (e.g., an entry that doesn't match any known per-type shape): migration must preserve it in the backup and report it, never guess destructively. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Format** + +- **FR-001**: The registry MUST be stored as a single human-readable, diffable text file carrying an explicit format version, replacing the colon-delimited flat file as the canonical store. +- **FR-002**: Each registry entry MUST store every attribute under an explicitly named field whose meaning does not depend on the entry's environment type; no field may be overloaded to mean different things for different types. +- **FR-003**: Each environment type (Incus, Proxmox, AWS, Hetzner, manually added SSH) MUST have its own defined set of named attributes covering everything the legacy format encoded for that type (including: Incus host SSH user; Proxmox VMID and node user; AWS instance id and region; SSH port and identity-file path for added hosts). +- **FR-004**: The access method (direct SSH vs. brokered SSM channel) MUST be an explicit stored attribute of each entry, replacing the implicit "instance id present + access mode empty" convention everywhere it is currently re-derived. +- **FR-005**: The stored format MUST support any legitimate field value, including values containing colons (IPv6 literals), spaces, and other characters that corrupt the legacy format; values MUST round-trip byte-identically. +- **FR-006**: The file MUST remain reviewable and diffable in version-control and backup tooling; a database or binary store is out of scope. + +**Migration** + +- **FR-007**: On first read by the CLI, a legacy-format registry MUST be migrated to the new format automatically, with zero user action and zero behavior change to the invoking command beyond an informational notice (which also mentions the one-time web-push re-verification, see FR-026). The CLI is the only component that performs migration. +- **FR-008**: Migration MUST preserve 100% of parseable legacy entries across all environment types and optional-field combinations, mapping each overloaded legacy field to its explicit named attribute. +- **FR-009**: Migration MUST preserve the original legacy file by renaming it in place to a backup name in the same directory, and MUST report (not silently discard) any lines it could not interpret; a pre-existing backup from an earlier attempt MUST NOT be silently overwritten. +- **FR-010**: Migration MUST be idempotent and crash-safe: an interrupted migration leaves the legacy file authoritative and intact, and the next read completes the migration; a completed migration is never re-run. +- **FR-011**: The web service and all other non-CLI consumers MUST be able to read both the legacy and new formats in place without writing anything and MUST never migrate, regardless of whether their volume is writable; migration MUST NOT be a precondition for reading. + +**Single accessor** + +- **FR-012**: Exactly one module MUST own parsing, serialization, validation, and locking for the registry; the CLI, provider logic, and the web service MUST all consume the registry through it, eliminating the three existing independent parser implementations. +- **FR-013**: The accessor MUST offer an explicit read-only mode that never creates directories or files, never terminates the calling process, and reports problems as returned errors/exceptions the caller chooses how to handle. +- **FR-014**: Reads MUST be tolerant at entry granularity: an individually malformed entry is skipped and surfaced as a warning/diagnostic, while remaining entries are returned; rewrites MUST preserve entries of unrecognized environment types rather than dropping them. +- **FR-015**: The in-memory host model (the KnownHost dataclass and its consumers) MUST retain its role; only serialization, parsing, and storage semantics change. Existing call sites that read or mutate hosts continue to work against the same in-memory shape or type-safe equivalents. + +**Validation on write** + +- **FR-016**: Every write MUST validate entries before anything touches disk: required fields present and non-empty, values representable in the format, and per-type attributes consistent with the entry's type; a failed validation rejects the write with a message naming the field and problem, leaving the file unchanged. + +**Concurrency** + +- **FR-017**: All read-modify-write sequences MUST be serialized via advisory locking so concurrent writers (two syncs; CLI mutation racing a web-service registry update) cannot drop each other's entries; a writer either acquires the lock within a bounded wait (5 seconds by default) or fails with a clear "registry busy" message — it never writes based on stale reads. +- **FR-018**: Every write MUST remain atomic from a reader's perspective: readers see either the complete old state or the complete new state, never a torn file — including when a writer is killed mid-operation. +- **FR-019**: On filesystems where advisory locking is unavailable, the system MUST degrade gracefully to unlocked operation with a one-time warning rather than refusing to work. + +**Workstation ↔ web service compatibility** + +- **FR-020**: The adopt/push mirror payload and the web service's registry-update endpoint MUST carry the registry format version and exchange entries in the explicit per-type representation (no overloaded fields on the wire). +- **FR-021**: A version mismatch between workstation and service MUST fail fast, before any mutation, with a message identifying which side is older and what to upgrade; the service's existing mirror MUST remain intact and served after a rejected push. +- **FR-022**: An upgraded web service MUST accept pushes from a not-yet-upgraded workstation (previous payload version) and store them correctly in the new format, so the service can be upgraded first without breaking the operator's push workflow. +- **FR-023**: When the registry file declares a format version newer than the running software understands, reads MUST fail with an explicit "written by a newer version" error and the file MUST NOT be modified. +- **FR-026**: Migration MUST reset the web-push change-detection cache: the first push after upgrade treats every instance as changed and re-verifies/re-authorizes it. This re-verification MUST be idempotent and require no manual cleanup, and the migration notice MUST mention that the next push will re-verify all instances. + +**Precedence & observability** + +- **FR-024**: When both a legacy file and a new-format file are present, the system MUST deterministically select one (new format wins), inform the user which was used and why, and never merge the two silently. +- **FR-025**: Migration, backup creation, skipped-entry warnings, and lock-wait events MUST be reported to the user in plain language; none of these may pass silently. + +### Key Entities + +- **Registry**: The single versioned document listing every environment remo manages from this workstation (or mirrored to a web service). Carries an explicit format version and a collection of host entries. Human-readable and diffable. +- **Host Entry**: One managed environment. Common attributes: environment type, display name, address, login user, access method (direct / brokered). Type-specific attributes live in a named per-type group (e.g., VMID and node user for Proxmox; instance id and region for AWS; port and identity file for added SSH hosts). No attribute's meaning varies by type. +- **Legacy Registry**: The colon-delimited flat file being replaced. Exists only as a migration source and preserved backup after upgrade. +- **Migration Backup**: The legacy file, renamed in place to a backup name in the same directory at migration time, content untouched — including any lines migration could not interpret. +- **Mirror Payload**: The versioned representation of the registry (plus per-host trust material) that the workstation pushes to an adopted web service; speaks the same explicit per-type representation as the file format. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: 100% of parseable legacy registry entries — across all five environment types and all optional-field combinations — survive migration with equivalent connection behavior, verified by an automated migration test matrix. +- **SC-002**: An existing user upgrading remo performs zero manual migration steps: the first command they run after upgrade works, and every subsequent command's output matches pre-upgrade behavior for the same registry. +- **SC-003**: Registry parsing/serialization logic exists in exactly one place: a codebase search finds no independent registry parser outside the single accessor module (down from three today). +- **SC-004**: Hosts with previously-corrupting values (IPv6 addresses, colon-containing paths) can be registered, listed, connected to, and pushed to a web service with values intact end to end. +- **SC-005**: A concurrency stress test (repeated concurrent mutations from multiple writers) completes with zero lost entries and zero unparseable file states across all iterations. +- **SC-006**: Every workstation/service version-skew combination (new→old, old→new) in the push flow ends in either a correct mirror or a single actionable error message; zero combinations end in silent divergence or a broken service. +- **SC-007**: The registry file remains reviewable: a person can read a diff of a registry change and state which host and which attribute changed, without tooling. +- **SC-008**: Registries of up to 200 entries add no perceptible latency: registry read/parse/write overhead stays under 100 ms per command invocation. + +## Assumptions + +- The registry remains a workstation-owned local file; multi-workstation shared registries, remote registries, and databases are out of scope. Advisory locking targets same-machine concurrency only. +- The new-format file lives in the same configuration directory as the legacy file under a new file name; the legacy file is renamed in place to a backup name at migration time. Exact file and backup naming are design-phase decisions. +- Migration happens lazily on the CLI's first registry read, not via a separate migration command; the web service — whether on a read-only mount or an adopted writable volume — reads legacy files in place indefinitely and never migrates. +- Downgrade support is not provided: after migration, an older remo version will not find the legacy file at its expected path. The renamed backup allows manual restoration, and this is documented; automated back-migration is out of scope. +- The web service's registry-update endpoint already carries a payload version field; this feature increments it. "Upgrade the service first, workstation second" is the supported skew direction (the service accepts old payloads; the workstation does not down-convert for old services beyond failing with clear remediation). +- The scope of "consumers" is the existing codebase's three read paths and all current write paths; changing sync semantics, adopt/push workflow behavior, or provider dispatch is explicitly out of scope (covered by separate roadmap features). +- Entry-level tolerant parsing (skip bad entries, keep good ones) matches today's read behavior and remains the desired policy; strict all-or-nothing validation applies only to writes. diff --git a/specs/015-registry-v2/tasks.md b/specs/015-registry-v2/tasks.md new file mode 100644 index 0000000..16c4ad8 --- /dev/null +++ b/specs/015-registry-v2/tasks.md @@ -0,0 +1,181 @@ +# Tasks: Versioned Structured Host Registry (Registry v2) + +**Input**: Design documents from `/specs/015-registry-v2/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/, quickstart.md + +**Tests**: INCLUDED — the spec mandates automated verification (SC-001 migration matrix, SC-005 concurrency stress, SC-006 skew matrix; quickstart scenarios reference the test files). + +**Organization**: Grouped by user story (US1–US5 from spec.md) after shared Setup/Foundational phases. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies on incomplete tasks) +- **[Story]**: US1–US5 (user-story phases only) + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Test scaffolding and path plumbing every later task uses. + +- [X] T001 Add registry test fixtures to tests/conftest.py: isolated `REMO_HOME` tmp-dir fixture, legacy known_hosts line builder (all 5 types, 4/6/7-field variants), and v2 registry.json builder matching contracts/registry-file-v2.md +- [X] T002 Add `get_registry_path()`, `get_registry_path_readonly()`, `get_registry_backup_path()`, and `get_registry_lock_path()` beside the existing known_hosts path helpers in src/remo_cli/core/config.py (readonly variants must not mkdir) + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: The `core/registry.py` accessor skeleton — codec, validation, read/write pipeline — that every user story builds on. + +**⚠️ CRITICAL**: No user story work can begin until this phase is complete. + +- [X] T003 Create src/remo_cli/core/registry.py with the error taxonomy (`RegistryError`, `RegistryReadError`, `RegistryValidationError`, `RegistryBusyError`, `RegistryNewerVersionError`) and the frozen `RegistryView` dataclass per contracts/registry-accessor-api.md — no `SystemExit` anywhere in the module +- [X] T004 Implement the v2 serializer in src/remo_cli/core/registry.py: KnownHost → hostEntry mapping (data-model.md §3), entries sorted by (type, name), sorted keys, 2-space indent, trailing newline, same-directory temp file + `os.replace` atomic write +- [X] T005 Implement the v2 parser in src/remo_cli/core/registry.py: hostEntry → KnownHost mapping with normalized in-memory `access_mode` ("direct"/"ssm"), `version > 2` → `RegistryNewerVersionError`, per-entry tolerant parse → `RegistryView.warnings`, unknown-type entries preserved verbatim for re-emission (FR-014) +- [X] T006 Implement the legacy codec in src/remo_cli/core/registry.py: tolerant colon-line parse (reusing `KnownHost.from_line`) plus the legacy→v2 mapping table keyed on type FIRST with the SSM classification rule (data-model.md §4, research R5) — this single implementation later serves migration AND payload-v1 mapping +- [X] T007 Implement validation rules V1–V6 (data-model.md §5) as a `validate_hosts()` helper in src/remo_cli/core/registry.py; failure raises `RegistryValidationError` naming field + entry +- [X] T008 Implement `read_registry(readonly=...)` in src/remo_cli/core/registry.py: resolution order registry.json → legacy → empty (file-state table, data-model.md §6), readonly mode using the no-mkdir path helpers with zero side effects (FR-011/FR-013); migration trigger left as a stub for US1 +- [X] T009 Implement `mutate_registry(mutator)` and `replace_registry(hosts, allow_empty=False)` in src/remo_cli/core/registry.py: read → apply → validate → atomic write, unknown-type entries passed through untouched (locking arrives in US4; keep the lock call site as a no-op seam) +- [X] T010 [P] Foundational unit tests in tests/unit/core/test_registry_format.py: v2 round-trip fidelity, deterministic serialization (byte-identical re-serialize), unknown-type preservation, newer-version rejection with file untouched, tolerant read warnings, validation rule rejections V2–V6 + +**Checkpoint**: Accessor reads/writes v2 and reads legacy in memory — no consumer wired yet, no files change shape for users. + +--- + +## Phase 3: User Story 1 - Existing User Upgrades Seamlessly (Priority: P1) 🎯 MVP + +**Goal**: Lazy, lossless, idempotent CLI migration; all CLI/provider call sites routed through the accessor; after this phase the system runs on v2 end-to-end. + +**Independent Test**: quickstart.md §2 — populate a legacy file with all 5 types + a garbage line, run `remo incus list`, verify the one-time notice, valid registry.json, byte-identical `known_hosts.v1.bak`, and silent idempotent re-runs. + +### Implementation for User Story 1 + +- [X] T011 [US1] Implement `migrate_if_needed()` + `MigrationReport` in src/remo_cli/core/registry.py: write-v2-then-rename ordering, `known_hosts.v1.bak` with non-clobbering numeric suffixes, skipped-lines capture, idempotent no-op when registry.json exists (FR-007/009/010, research R6) +- [X] T012 [US1] Implement both-present resolution in src/remo_cli/core/registry.py: host-set equivalence → silently complete the rename (S3→S2); divergence → v2 wins + warning, never merge (S4, FR-024) +- [X] T013 [US1] Convert src/remo_cli/core/known_hosts.py public functions (`get_known_hosts`, `save_known_host`, `remove_known_host`, `clear_known_hosts_by_type`, `clear_known_hosts_by_prefix`) into thin delegates to the accessor per contracts/registry-accessor-api.md, deleting the module's internal line-parsing/atomic-write code; resolver/guard helpers (`resolve_remo_host_by_name`, `guard_not_added_ssh_host`, `get_aws_region`) unchanged in behavior over `read_registry().hosts` +- [X] T014 [US1] Surface `MigrationReport` through the delegates at the CLI boundary using src/remo_cli/core/output.py: plain-language notice with migrated count, backup name, skipped lines verbatim, and the one-time "next `remo web push` will re-verify all instances" note (FR-025/FR-026) +- [X] T015 [P] [US1] Migration matrix tests in tests/unit/core/test_registry_migration.py: all 5 types × 4/6/7-field combos, garbage lines, unknown types preserved, empty vs missing file, pre-existing backup suffixing, interrupted-migration convergence (S3), divergent both-present warning (S4), completed migration never re-runs — matrix MUST include the two legacy access-mode variants that no current writer produces but old files can contain: a non-AWS line with literal `ssm` in the access-mode slot (the `to_line` back-fill quirk) and a 7-field line with an empty access-mode slot; both map to `access: "direct"` via the type-first rule (quickstart §2 `old/…` lines) +- [X] T016 [P] [US1] Provider save-path fixture tests in tests/unit/providers/test_provider_registry_entries.py: pin the exact KnownHost field usage of each provider save call (providers/incus.py, proxmox.py, aws.py, hetzner.py, added.py) and assert the legacy→v2 mapper classifies each correctly — the research R5 risk pin (especially the `to_line` implicit-SSM quirk vs incus/proxmox `instance_id` overloads) +- [X] T017 [US1] Update existing tests that fabricate legacy known_hosts files (grep tests/ for `known_hosts` fixtures, e.g. tests/unit/test_host_model.py, tests/unit/web/, tests/integration/test_web_adopt_e2e.py) to run against the accessor era: legacy fixtures stay valid as migration inputs; assertions move to v2 where they check written output + +**Checkpoint**: MVP — an upgraded user's first command migrates losslessly; everything runs on v2. Two scope caveats: (1) FR-017 (advisory locking) is deliberately deferred to Phase 6 — writes here are as-unlocked-as-today; (2) this checkpoint is shippable ONLY for CLI-only usage — a deployment running the web service must also take US3's T021/T023, because migration renames the legacy file the pre-US3 web readers look for (see Dependencies). + +--- + +## Phase 4: User Story 2 - Registry Values Can No Longer Corrupt the File (Priority: P2) + +**Goal**: Any legitimate value (IPv6, colon-containing paths) round-trips; invalid values are rejected pre-write with named-field errors. + +**Independent Test**: quickstart.md §4 — `remo add v6box 2001:db8::7 --user admin` round-trips intact; invalid entries are rejected leaving the file unchanged. + +### Implementation for User Story 2 + +- [X] T018 [US2] Wire `validate_hosts()` into every write path (delegates in src/remo_cli/core/known_hosts.py and `replace_registry` in src/remo_cli/core/registry.py) asserting reject-before-write leaves disk untouched (FR-016); delete any CLI-side colon-safety workarounds made obsolete by the format (check src/remo_cli/core/validation.py and providers/added.py) +- [X] T019 [P] [US2] Value-fidelity tests extending tests/unit/core/test_registry_format.py: IPv6 literals, colon-containing identity paths, spaces/special characters, boundary-length names — byte-identical round-trips; rejection-message tests assert field + entry are named +- [X] T020 [US2] Fix `remo add` TARGET parsing for IPv6 in src/remo_cli/providers/added.py (today `rest.partition(":")` splits an IPv6 literal at its first colon, failing before the registry is involved): accept OpenSSH-style bracket form `[user@][v6][:port]` and treat a bare bracket-less TARGET containing multiple colons as a host with no port suffix; then end-to-end IPv6 added-host test in tests/unit/providers/test_added_ipv6.py — add both forms, list, connect argv built correctly (`build_ssh_base_cmd` receives the intact literal) + +**Checkpoint**: US1 + US2 — the data-loss bug class is closed. + +--- + +## Phase 5: User Story 3 - One Registry Reader for CLI and Web Service (Priority: P3) + +**Goal**: The three parser implementations collapse into the accessor; the web service reads both formats in place with zero side effects. + +**Independent Test**: quickstart.md §6 — web check against a read-only volume in each format shows the same host set as the CLI, no writes, malformed entry degrades to a warning; grep finds no private registry parsing under src/remo_cli/web/. + +### Implementation for User Story 3 + +- [X] T021 [US3] Replace `_read_known_hosts_readonly()` in src/remo_cli/web/discovery.py with `registry.read_registry(readonly=True)`; per-entry warnings logged, structural errors surfaced as the existing degraded behavior +- [X] T022 [P] [US3] Replace `_read_registry_readonly()` in src/remo_cli/web/api/setup.py (read/status path only — PUT payload changes are US5) with `registry.read_registry(readonly=True)` +- [X] T023 [P] [US3] Update the registry probe in src/remo_cli/web/state.py to accept registry.json OR legacy known_hosts (either present ⇒ registry present) and map `RegistryNewerVersionError`/`RegistryReadError` to the `broken` state (data-model.md §6) +- [X] T024 [US3] Update src/remo_cli/web/check.py to report which registry format was found and emit the newer-version remediation text on `RegistryNewerVersionError` (FR-025) +- [X] T025 [P] [US3] Web readonly tests in tests/unit/web/test_registry_readonly.py: both formats on a chmod-555 REMO_HOME (no writes/mkdirs — assert mtimes), CLI-vs-web host-set parity, single malformed entry degrades per-entry, broken-state mapping for newer-version files + +**Checkpoint**: SC-003 achieved — one parser; web behavior identical across formats. + +--- + +## Phase 6: User Story 4 - Concurrent Writers Don't Lose Entries (Priority: P4) + +**Goal**: Advisory locking serializes all read-modify-write sequences; bounded wait; graceful degradation; crash atomicity proven. + +**Independent Test**: quickstart.md §5 — multiprocess writers always converge to the union; a held lock produces "registry busy" after ~5 s; SIGKILL mid-write leaves the previous complete state. + +### Implementation for User Story 4 + +- [X] T026 [US4] Implement `registry_lock(timeout_s=5.0)` in src/remo_cli/core/registry.py: `fcntl.flock(LOCK_EX|LOCK_NB)` on the sidecar lock file, 50 ms retry loop, `RegistryBusyError` with plain-language message on timeout, one-time warning + unlocked fallback on `ENOLCK`/`EOPNOTSUPP` (FR-017/FR-019, research R3) +- [X] T027 [US4] Wrap `mutate_registry`, `replace_registry`, and `migrate_if_needed` in `registry_lock` (filling the T009/T011 no-op seam), including the re-check-after-acquire so concurrent first-runs and concurrent migrations converge (FR-010/FR-017) +- [X] T028 [P] [US4] Lock unit tests in tests/unit/core/test_registry_locking.py: timeout → `RegistryBusyError` at ~5 s, monkeypatched flock `OSError` → one-time warning + unlocked proceed, kill-mid-write leaves complete prior state (atomicity, FR-018) +- [X] T029 [US4] Multiprocess stress test in tests/integration/test_registry_concurrency.py: N processes upserting disjoint entry sets in a loop → final union always intact, file parses on every iteration (SC-005) + +**Checkpoint**: Lost-update race class closed for CLI-and-service cohabitation. + +--- + +## Phase 7: User Story 5 - Workstation and Web Service on Different Versions (Priority: P5) + +**Goal**: Payload v2 with fail-fast version negotiation; upgraded service accepts v1 payloads; delta cache resets once after migration. + +**Independent Test**: quickstart.md §7 — the compatibility matrix in contracts/mirror-payload-v2.md §4 passes row by row. + +### Implementation for User Story 5 + +- [X] T030 [US5] Update `PUT /api/v1/setup/registry` in src/remo_cli/web/api/setup.py: accept payload v1 AND v2 (Pydantic models per contracts/mirror-payload-v2.md §2), map v1 through the accessor's legacy mapper (T006), store v2 via `replace_registry`, remove any legacy mirror file as apply step 3, return 400 `unsupported_payload_version` (mirror intact) for unknown versions; replace the legacy colon/newline field checks with V2–V6 validation for v2 payloads (FR-020/021/022) +- [X] T031 [US5] Add `payload_versions: [1, 2]` to `GET /api/v1/setup/status` in src/remo_cli/web/api/setup.py (contracts/mirror-payload-v2.md §1) +- [X] T032 [US5] Update the push flow in src/remo_cli/core/web_adopt.py: build payload v2 from v2 entries; read `payload_versions` from status (absent ⇒ [1]) and abort BEFORE any keyscan/authorize/PUT with the upgrade-the-service remediation when 2 is unsupported (FR-021, fail truly fast) +- [X] T033 [US5] Implement push-cache v2 in src/remo_cli/core/web_adopt.py: `cache_version: 2` field, fingerprint = SHA-256 of canonical sorted-key JSON of the v2 entry, any other/missing cache_version ⇒ empty cache (one-time full re-verification push, FR-026, research R10) +- [X] T034 [P] [US5] Payload contract tests in tests/integration/test_setup_payload_versions.py: v1 accepted → stored as v2 + legacy mirror removed; v2 accepted with schema-exact entries; v3 → 400 with mirror intact and served; missing `payload_versions` → workstation aborts pre-keyscan with remediation; stale cache_version ⇒ full re-verify, idempotent on immediate re-push (SC-006) + +**Checkpoint**: All five stories complete; every skew combination lands on the matrix. + +--- + +## Phase 8: Polish & Cross-Cutting Concerns + +- [X] T035 [P] Performance regression test in tests/perf/test_registry_perf.py: generated 200-entry registry read+validate+write round-trip < 100 ms (SC-008, research R11) +- [X] T036 [P] Documentation sync (Constitution V): README registry/format section, docs/web-session-interface.md adoption payload examples → v2, CLAUDE.md Active Technologies line ("Flat file (colon-delimited)" → versioned registry.json) and registry references +- [X] T037 Execute quickstart.md scenarios 1–8 end-to-end in a scratch REMO_HOME; fix any drift between quickstart commands and reality +- [X] T038 Full gates: `uv run pytest`, `uv run mypy src/remo_cli`, `uv run ruff check src/remo_cli` — all green + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Setup (Phase 1)**: no dependencies +- **Foundational (Phase 2)**: needs T001–T002; **blocks all user stories** +- **US1 (Phase 3)**: needs Phase 2. Delivers the MVP; T013 (delegates) is the single riskiest wiring task — land T011/T012 before it so migration exists when call sites route through the accessor +- **US2 (Phase 4)**: needs Phase 2 + T013 (write paths routed). Otherwise independent of US1's migration internals +- **US3 (Phase 5)**: needs Phase 2 only (readonly read path) — can run in parallel with US1/US2 apart from merge conflicts in setup.py (none: US3 touches read path, US5 touches PUT). **Release-blocking caveat**: T021 (discovery) and T023 (state probe) MUST land before any release that includes US1's migration if the deployment uses the web service — post-migration, the pre-US3 web readers look only at the legacy path and would see an empty registry +- **US4 (Phase 6)**: needs T009/T011 (the seams it fills). Independent of US2/US3/US5 +- **US5 (Phase 7)**: needs T006 (legacy mapper) + T009 (`replace_registry`); T032/T033 also touch core/web_adopt.py sequentially (same file — not [P] with each other) +- **Polish (Phase 8)**: after all desired stories + +### Story completion order (sequential solo path) + +US1 → US2 → US3 → US4 → US5 (priority order). Each checkpoint is independently shippable. + +### Parallel Opportunities + +- Phase 2: T010 in parallel with the tail of T007–T009 (different files) +- US1: T015 + T016 in parallel (different test files) once T011–T013 exist +- US3: T022 + T023 + T025 in parallel; T021 and T024 sequential only against their own files +- Cross-story (if parallelized): US3 (web read path) alongside US1/US2 (CLI write path) — disjoint files except tests/conftest.py +- US5: T030 and T031 are sequential (same file: web/api/setup.py); T034 parallel once T030–T033 exist + +## Parallel Example: User Story 1 + +```bash +# After T011–T014 land, run both test-authoring tasks concurrently: +Task: "Migration matrix tests in tests/unit/core/test_registry_migration.py" +Task: "Provider save-path fixture tests in tests/unit/providers/test_provider_registry_entries.py" +``` + +## Implementation Strategy + +**MVP first (US1)**: Phases 1–3 produce a shippable increment for CLI-only usage — users migrate losslessly and run on v2. If the release includes the web service, US3's T021/T023 are part of the MVP (see the US3 release-blocking caveat above). STOP and validate quickstart §2–§3 before proceeding. + +**Incremental delivery**: each subsequent story closes one risk class (US2 data loss → US3 parser drift → US4 races → US5 skew) and each ends at a runnable checkpoint. US4's locking and US5's payload work are the most isolated — good candidates to defer if the branch needs to ship early, since their absence matches today's behavior (unlocked writes, v1 payloads). + +**Notes**: commit after each task or logical group; T013 and T030 are the two tasks most likely to surface hidden call-site assumptions — run the full suite immediately after each. diff --git a/src/remo_cli/__main__.py b/src/remo_cli/__main__.py index dbd3255..37b285e 100644 --- a/src/remo_cli/__main__.py +++ b/src/remo_cli/__main__.py @@ -1,5 +1,5 @@ """Enable `python -m remo`.""" -from remo_cli.cli.main import cli +from remo_cli.cli.main import main -cli() +main() diff --git a/src/remo_cli/cli/main.py b/src/remo_cli/cli/main.py index df6052e..e5b778a 100644 --- a/src/remo_cli/cli/main.py +++ b/src/remo_cli/cli/main.py @@ -2,6 +2,8 @@ from __future__ import annotations +import sys + import click import remo_cli @@ -98,3 +100,17 @@ def _register_commands() -> None: _register_commands() + + +def main() -> None: + """Console-script entry point: run the CLI, translating registry accessor + errors (data-model.md §8) into a plain-language message and exit code 1 + instead of an uncaught traceback (FR-013: the accessor itself never exits). + """ + from remo_cli.core.registry import RegistryError + + try: + cli() + except RegistryError as e: + click.echo(f"Error: {e}", err=True) + sys.exit(1) diff --git a/src/remo_cli/core/config.py b/src/remo_cli/core/config.py index f165af2..f29652c 100644 --- a/src/remo_cli/core/config.py +++ b/src/remo_cli/core/config.py @@ -148,6 +148,37 @@ def get_known_hosts_path_readonly() -> Path: return get_remo_home_readonly() / "known_hosts" +def get_registry_path() -> Path: + """Return the path to the remo registry.json (v2) file. + + Creates the parent config directory if it does not exist. + """ + return get_remo_home() / "registry.json" + + +def get_registry_path_readonly() -> Path: + """Return the path to registry.json WITHOUT creating its parent. + + Mirrors :func:`get_registry_path` but built on + :func:`get_remo_home_readonly` — safe against a read-only bind mount. + """ + return get_remo_home_readonly() / "registry.json" + + +def get_registry_backup_path() -> Path: + """Return the path where the legacy known_hosts file is renamed to on migration.""" + return get_remo_home_readonly() / "known_hosts.v1.bak" + + +def get_registry_lock_path() -> Path: + """Return the path to the registry's advisory-lock sidecar file. + + Never replaced by an atomic write (unlike ``registry.json`` itself), + so a lock held on it remains meaningful across writers. + """ + return get_remo_home_readonly() / "registry.lock" + + def is_verbose() -> bool: """Return True if REMO_VERBOSE is set to '1'.""" return os.environ.get("REMO_VERBOSE") == "1" diff --git a/src/remo_cli/core/known_hosts.py b/src/remo_cli/core/known_hosts.py index 63ba3a1..b9ac3c0 100644 --- a/src/remo_cli/core/known_hosts.py +++ b/src/remo_cli/core/known_hosts.py @@ -1,171 +1,108 @@ -"""Flat-file registry of registered development environments.""" +"""Public host-registry API — thin delegates onto :mod:`core.registry`. + +Every existing call site in ``providers/*`` and ``cli/*`` keeps working +unchanged (FR-015); the accessor in :mod:`remo_cli.core.registry` owns all +parsing, serialization, validation, locking, and migration. +""" from __future__ import annotations import os import sys -import tempfile -from pathlib import Path -from remo_cli.core.config import get_known_hosts_path +from remo_cli.core.registry import ( + MigrationReport, + migrate_if_needed, + mutate_registry, + read_registry, +) from remo_cli.models.host import KnownHost +_migration_notice_shown = False + + +def _print_migration_notice(report: MigrationReport) -> None: + """Print the one-time plain-language migration notice (FR-025/FR-026).""" + from remo_cli.core.output import print_info, print_warning + + global _migration_notice_shown + if _migration_notice_shown: + return + _migration_notice_shown = True + + print_info( + f"Migrated {report.migrated_count} registry entr" + f"{'y' if report.migrated_count == 1 else 'ies'} to the new registry.json " + f"format (backup saved as {report.backup_path.name})." + ) + if report.skipped_lines: + print_warning( + f"Skipped {len(report.skipped_lines)} unrecognized line(s) during " + f"migration (left untouched in the backup):" + ) + for line in report.skipped_lines: + print_warning(f" {line!r}") + print_info( + "Note: the next `remo web push` will re-verify all instances (the " + "registry format changed)." + ) + + +def _migrate_and_notify() -> None: + report = migrate_if_needed() + if report is not None: + _print_migration_notice(report) + def save_known_host(host: KnownHost) -> None: - """Add or replace a host entry in the registry. + """Add or replace a host entry in the registry (upsert by (type, name)).""" + _migrate_and_notify() - Ensures the registry file and its parent directory exist. Any existing - entry with the same (type, name) pair is removed before the new entry is - appended, so each (type, name) pair remains unique. + def _upsert(hosts: list[KnownHost]) -> list[KnownHost]: + kept = [h for h in hosts if not (h.type == host.type and h.name == host.name)] + kept.append(host) + return kept - The write is performed atomically: lines are filtered into a temp file in - the same directory, then renamed over the registry file. - """ - registry_path = get_known_hosts_path() - registry_path.parent.mkdir(parents=True, exist_ok=True) - - if not registry_path.exists(): - registry_path.touch() - - # Read all existing lines, dropping any entry that matches (type, name). - kept_lines: list[str] = [] - with registry_path.open() as fh: - for raw_line in fh: - line = raw_line.rstrip("\n") - if not line: - kept_lines.append(line) - continue - try: - existing = KnownHost.from_line(line) - except ValueError: - # Preserve lines that cannot be parsed. - kept_lines.append(line) - continue - if existing.type == host.type and existing.name == host.name: - # Drop the stale entry; the new one will be appended below. - continue - kept_lines.append(line) - - kept_lines.append(host.to_line()) - - _write_lines_atomically(registry_path, kept_lines) + mutate_registry(_upsert) def remove_known_host(type: str, name: str) -> None: - """Remove the entry matching (type, name) from the registry. + """Remove the entry matching (type, name) from the registry, if present.""" + _migrate_and_notify() - Does nothing if the registry file does not exist or if no matching entry - is found. - """ - registry_path = get_known_hosts_path() - if not registry_path.exists(): - return + def _drop(hosts: list[KnownHost]) -> list[KnownHost]: + return [h for h in hosts if not (h.type == type and h.name == name)] - kept_lines: list[str] = [] - with registry_path.open() as fh: - for raw_line in fh: - line = raw_line.rstrip("\n") - if not line: - kept_lines.append(line) - continue - try: - existing = KnownHost.from_line(line) - except ValueError: - kept_lines.append(line) - continue - if existing.type == type and existing.name == name: - continue - kept_lines.append(line) - - _write_lines_atomically(registry_path, kept_lines) + mutate_registry(_drop) def get_known_hosts(type_filter: str | None = None) -> list[KnownHost]: - """Return all registered hosts, optionally filtered by type. - - Returns an empty list if the registry file does not exist. Lines that are - empty or that fail to parse are silently skipped. - """ - registry_path = get_known_hosts_path() - if not registry_path.exists(): - return [] - - hosts: list[KnownHost] = [] - with registry_path.open() as fh: - for raw_line in fh: - line = raw_line.strip() - if not line: - continue - try: - host = KnownHost.from_line(line) - except ValueError: - continue - if type_filter is not None and host.type != type_filter: - continue - hosts.append(host) - + """Return all registered hosts, optionally filtered by type.""" + _migrate_and_notify() + hosts = read_registry(readonly=False).hosts + if type_filter is not None: + hosts = [h for h in hosts if h.type == type_filter] return hosts def clear_known_hosts_by_type(type: str) -> None: - """Remove all entries whose type equals *type*. + """Remove all entries whose type equals *type*.""" + _migrate_and_notify() - Does nothing if the registry file does not exist. - """ - registry_path = get_known_hosts_path() - if not registry_path.exists(): - return + def _filter(hosts: list[KnownHost]) -> list[KnownHost]: + return [h for h in hosts if h.type != type] - kept_lines: list[str] = [] - with registry_path.open() as fh: - for raw_line in fh: - line = raw_line.rstrip("\n") - if not line: - kept_lines.append(line) - continue - try: - existing = KnownHost.from_line(line) - except ValueError: - kept_lines.append(line) - continue - if existing.type == type: - continue - kept_lines.append(line) - - _write_lines_atomically(registry_path, kept_lines) + mutate_registry(_filter) def clear_known_hosts_by_prefix(type: str, prefix: str) -> None: - """Remove entries where type matches and name starts with *prefix*. - - Used during incus sync to remove stale container entries for a particular - Incus host before re-populating them. For example:: + """Remove entries where type matches and name starts with *prefix*.""" + _migrate_and_notify() - clear_known_hosts_by_prefix("incus", "myhost/") + def _filter(hosts: list[KnownHost]) -> list[KnownHost]: + return [h for h in hosts if not (h.type == type and h.name.startswith(prefix))] - Does nothing if the registry file does not exist. - """ - registry_path = get_known_hosts_path() - if not registry_path.exists(): - return - - kept_lines: list[str] = [] - with registry_path.open() as fh: - for raw_line in fh: - line = raw_line.rstrip("\n") - if not line: - kept_lines.append(line) - continue - try: - existing = KnownHost.from_line(line) - except ValueError: - kept_lines.append(line) - continue - if existing.type == type and existing.name.startswith(prefix): - continue - kept_lines.append(line) - - _write_lines_atomically(registry_path, kept_lines) + mutate_registry(_filter) def get_aws_region(name: str) -> str: @@ -264,28 +201,3 @@ def resolve_remo_host_by_name(name: str) -> KnownHost: f"Error: no environment named '{name}' found in the registry.\n" "The registry is empty. Use 'remo add' to register an environment." ) - - -# --------------------------------------------------------------------------- -# Internal helpers -# --------------------------------------------------------------------------- - - -def _write_lines_atomically(path: Path, lines: list[str]) -> None: - """Write *lines* to *path* atomically via a temp file + rename. - - A temporary file is created in the same directory as *path* so that the - ``os.replace`` rename is guaranteed to be on the same filesystem (and - therefore atomic on POSIX systems). - """ - dir_ = path.parent - fd, tmp_path_str = tempfile.mkstemp(dir=dir_, prefix=".known_hosts_tmp_") - tmp_path = Path(tmp_path_str) - try: - with os.fdopen(fd, "w") as fh: - for line in lines: - fh.write(line + "\n") - os.replace(tmp_path, path) - except Exception: - tmp_path.unlink(missing_ok=True) - raise diff --git a/src/remo_cli/core/registry.py b/src/remo_cli/core/registry.py new file mode 100644 index 0000000..8844d14 --- /dev/null +++ b/src/remo_cli/core/registry.py @@ -0,0 +1,835 @@ +"""Versioned structured host registry accessor (Registry v2). + +Single module owning parse/serialize/validate/lock/migrate for the registry +(FR-012). Every consumer — CLI, providers, web service — goes through this +surface. See specs/015-registry-v2/contracts/registry-accessor-api.md for the +authoritative contract. + +This module never raises :class:`SystemExit` (FR-013); callers at the CLI or +web boundary translate the error taxonomy below into user-facing behavior. +""" + +from __future__ import annotations + +import errno +import fcntl +import json +import os +import tempfile +import time +from collections.abc import Callable +from contextlib import contextmanager +from dataclasses import dataclass, replace +from pathlib import Path +from typing import Any + +from remo_cli.core.config import ( + get_known_hosts_path, + get_known_hosts_path_readonly, + get_registry_backup_path, + get_registry_lock_path, + get_registry_path, + get_registry_path_readonly, +) +from remo_cli.models.host import KnownHost + +SUPPORTED_VERSION = 2 +KNOWN_TYPES = frozenset({"incus", "proxmox", "aws", "hetzner", "ssh"}) + + +# --------------------------------------------------------------------------- +# Error taxonomy (FR-013: never SystemExit) +# --------------------------------------------------------------------------- + + +class RegistryError(Exception): + """Base class for all registry accessor errors.""" + + +class RegistryReadError(RegistryError): + """The registry file is unreadable or its top-level document is invalid.""" + + +class RegistryValidationError(RegistryError): + """A write would violate validation rules V1-V6; disk is left untouched.""" + + +class RegistryBusyError(RegistryError): + """The advisory lock could not be acquired within the timeout.""" + + +class RegistryNewerVersionError(RegistryError): + """The registry file was written by a newer, unsupported format version.""" + + +# --------------------------------------------------------------------------- +# Public types +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class RegistryView: + """The result of a registry read.""" + + hosts: list[KnownHost] + warnings: list[str] + source_format: str # "v2" | "legacy" | "empty" + unknown_entries: int + + +@dataclass(frozen=True) +class MigrationReport: + """The result of a completed lazy migration (S1 -> S2).""" + + migrated_count: int + backup_path: Path + skipped_lines: list[str] + + +@dataclass +class _LegacyParseResult: + hosts: list[KnownHost] + unknown_raw: list[dict[str, Any]] + skipped_lines: list[str] + warnings: list[str] + + +@dataclass +class _ParsedDocument: + hosts: list[KnownHost] + unknown_raw: list[dict[str, Any]] + warnings: list[str] + source_format: str + + +# --------------------------------------------------------------------------- +# Entry <-> KnownHost mapping (data-model.md §2/§3) +# --------------------------------------------------------------------------- + + +def known_host_to_entry(host: KnownHost) -> dict[str, Any]: + """Serialize a :class:`KnownHost` into a v2 hostEntry dict (key order fixed).""" + entry: dict[str, Any] = { + "type": host.type, + "name": host.name, + "host": host.host, + "user": host.user, + "access": host.access_mode or "direct", + } + + nested: dict[str, Any] = {} + if host.type == "incus": + if host.instance_id: + nested["host_user"] = host.instance_id + elif host.type == "proxmox": + if host.instance_id: + nested["vmid"] = host.instance_id + if host.region: + nested["node_user"] = host.region + elif host.type == "aws": + if host.instance_id: + nested["instance_id"] = host.instance_id + if host.region: + nested["region"] = host.region + elif host.type == "ssh": + if host.instance_id: + try: + nested["port"] = int(host.instance_id) + except ValueError: + pass + if host.region: + nested["identity_file"] = host.region + # hetzner: no nested fields today. + + if nested: + entry[host.type] = nested + + return entry + + +def entry_to_known_host(entry: dict[str, Any]) -> KnownHost | None: + """Parse a KNOWN-type hostEntry dict into a :class:`KnownHost`. + + Returns ``None`` if the entry's shape does not match the contract (the + caller turns this into a tolerant-read warning, never an exception). + """ + type_ = entry.get("type") + name = entry.get("name") + host = entry.get("host") + user = entry.get("user") + access = entry.get("access") + + if not ( + isinstance(type_, str) + and isinstance(name, str) + and isinstance(host, str) + and isinstance(user, str) + and isinstance(access, str) + ): + return None + if not (type_ and name and host and user): + return None + if access not in ("direct", "ssm"): + return None + if type_ not in KNOWN_TYPES: + return None + + instance_id = "" + region = "" + + if type_ == "incus": + nested = entry.get("incus") + if isinstance(nested, dict): + instance_id = str(nested.get("host_user", "") or "") + elif type_ == "proxmox": + nested = entry.get("proxmox") + if isinstance(nested, dict): + instance_id = str(nested.get("vmid", "") or "") + region = str(nested.get("node_user", "") or "") + elif type_ == "aws": + nested = entry.get("aws") + if isinstance(nested, dict): + instance_id = str(nested.get("instance_id", "") or "") + region = str(nested.get("region", "") or "") + elif type_ == "ssh": + nested = entry.get("ssh") + if isinstance(nested, dict): + port = nested.get("port", "") + instance_id = str(port) if port != "" else "" + region = str(nested.get("identity_file", "") or "") + # hetzner: nothing further. + + return KnownHost( + type=type_, + name=name, + host=host, + user=user, + instance_id=instance_id, + access_mode=access, + region=region, + ) + + +def legacy_fields_to_entry( + type_: str, + name: str, + host: str, + user: str, + instance_id: str, + access_mode: str, + region: str, +) -> dict[str, Any]: + """Map legacy 7-field values to a v2 hostEntry dict. + + Keyed on ``type_`` FIRST (research R5): ``instance_id`` is meaningless + without knowing the type. Shared by CLI migration and setup-API v1 + payload mapping (research R8/R9) — the single legacy->v2 mapper. + """ + if type_ not in KNOWN_TYPES: + entry: dict[str, Any] = { + "type": type_, + "name": name, + "host": host, + "user": user, + "access": "direct", + } + legacy_fields = [instance_id, access_mode, region] + if any(legacy_fields): + entry["_legacy_fields"] = legacy_fields + return entry + + access = "direct" + if type_ == "aws" and (access_mode == "ssm" or (instance_id and not access_mode)): + access = "ssm" + + entry = { + "type": type_, + "name": name, + "host": host, + "user": user, + "access": access, + } + + nested: dict[str, Any] = {} + if type_ == "incus": + if instance_id: + nested["host_user"] = instance_id + elif type_ == "proxmox": + if instance_id: + nested["vmid"] = instance_id + if region: + nested["node_user"] = region + elif type_ == "aws": + if instance_id: + nested["instance_id"] = instance_id + if region: + nested["region"] = region + elif type_ == "ssh": + if instance_id: + try: + nested["port"] = int(instance_id) + except ValueError: + pass + if region: + nested["identity_file"] = region + + if nested: + entry[type_] = nested + + return entry + + +# --------------------------------------------------------------------------- +# Validation (data-model.md §5, rules V1-V6) +# --------------------------------------------------------------------------- + + +def _has_forbidden_chars(value: str) -> bool: + return any(ord(c) < 0x20 or ord(c) == 0x7F for c in value) + + +def _validate_single_host(h: KnownHost) -> str | None: + """Validate one host's fields (rules V2-V6). Returns an error message + naming the field + entry, or ``None`` when valid. Does NOT check + cross-entry uniqueness (V1) — that is the caller's responsibility. + """ + entry_label = f"{h.type}:{h.name}" + for field_name, value in ( + ("type", h.type), + ("name", h.name), + ("host", h.host), + ("user", h.user), + ): + if not value: + return f"{field_name} must not be empty (entry: {entry_label})" + if _has_forbidden_chars(value): + return ( + f"{field_name} contains control characters or newlines " + f"(entry: {entry_label})" + ) + for field_name, value in ( + ("instance_id", h.instance_id), + ("region", h.region), + ): + if value and _has_forbidden_chars(value): + return ( + f"{field_name} contains control characters or newlines " + f"(entry: {entry_label})" + ) + + # An empty access_mode is KnownHost's dataclass default and has always + # meant "direct" (matches known_host_to_entry's own `or "direct"` + # normalization) — accept it here rather than rejecting every call site + # that relies on the default instead of setting it explicitly. + effective_access = h.access_mode or "direct" + if effective_access not in ("direct", "ssm"): + return ( + f"access must be 'direct' or 'ssm', got {h.access_mode!r} " + f"(entry: {entry_label})" + ) + if effective_access == "ssm" and h.type != "aws": + return f"access 'ssm' is only valid for type 'aws' (entry: {entry_label})" + + if h.type == "ssh" and h.instance_id: + try: + port = int(h.instance_id) + except ValueError: + return f"ssh port must be an integer (entry: {entry_label})" + if not (1 <= port <= 65535): + return f"ssh port {port} is out of range 1-65535 (entry: {entry_label})" + + return None + + +def validate_hosts(hosts: list[KnownHost]) -> None: + """Validate rules V1-V6. Raises :class:`RegistryValidationError` naming + field + entry on the first violation found; callers must not write to + disk when this raises (FR-016). + """ + seen: set[tuple[str, str]] = set() + for h in hosts: + error = _validate_single_host(h) + if error is not None: + raise RegistryValidationError(error) + key = (h.type, h.name) + if key in seen: + raise RegistryValidationError( + f"duplicate entry for type={h.type!r} name={h.name!r}" + ) + seen.add(key) + + +# --------------------------------------------------------------------------- +# Legacy codec (tolerant colon-line parse) +# --------------------------------------------------------------------------- + + +def _parse_legacy_lines(lines: list[str]) -> _LegacyParseResult: + hosts: list[KnownHost] = [] + unknown_raw: list[dict[str, Any]] = [] + skipped_lines: list[str] = [] + warnings: list[str] = [] + + for raw_line in lines: + line = raw_line.rstrip("\n") + if not line.strip(): + continue + try: + kh = KnownHost.from_line(line) + except ValueError: + skipped_lines.append(line) + warnings.append(f"skipped unparseable legacy line: {line!r}") + continue + + if not (kh.type and kh.name and kh.host and kh.user): + skipped_lines.append(line) + warnings.append( + f"skipped legacy line with an empty required field: {line!r}" + ) + continue + + entry = legacy_fields_to_entry( + kh.type, kh.name, kh.host, kh.user, kh.instance_id, kh.access_mode, kh.region + ) + if kh.type in KNOWN_TYPES: + known_host = entry_to_known_host(entry) + if known_host is None: + skipped_lines.append(line) + warnings.append(f"skipped malformed legacy line: {line!r}") + continue + hosts.append(known_host) + else: + unknown_raw.append(entry) + + return _LegacyParseResult( + hosts=hosts, unknown_raw=unknown_raw, skipped_lines=skipped_lines, warnings=warnings + ) + + +def _read_legacy_file(path: Path) -> _ParsedDocument: + try: + text = path.read_text(encoding="utf-8") + except OSError as e: + raise RegistryReadError(f"could not read {path}: {e}") from e + result = _parse_legacy_lines(text.splitlines()) + return _ParsedDocument( + hosts=result.hosts, + unknown_raw=result.unknown_raw, + warnings=result.warnings, + source_format="legacy", + ) + + +# --------------------------------------------------------------------------- +# v2 file codec +# --------------------------------------------------------------------------- + + +def _read_v2_file(path: Path) -> _ParsedDocument: + try: + text = path.read_text(encoding="utf-8") + except OSError as e: + raise RegistryReadError(f"could not read {path}: {e}") from e + + try: + doc = json.loads(text) + except json.JSONDecodeError as e: + raise RegistryReadError(f"{path} is not valid JSON: {e}") from e + + if not isinstance(doc, dict): + raise RegistryReadError(f"{path}: top-level document must be a JSON object") + + version = doc.get("version") + if not isinstance(version, int) or isinstance(version, bool): + raise RegistryReadError(f"{path}: missing or invalid 'version' field") + if version > SUPPORTED_VERSION: + raise RegistryNewerVersionError( + f"{path} was written by a newer version of remo (format {version}); " + f"upgrade remo, or restore the {get_registry_backup_path().name} backup" + ) + if version < SUPPORTED_VERSION: + raise RegistryReadError(f"{path}: unsupported registry version {version}") + + raw_hosts = doc.get("hosts") + if not isinstance(raw_hosts, list): + raise RegistryReadError(f"{path}: 'hosts' field must be a list") + + hosts: list[KnownHost] = [] + unknown_raw: list[dict[str, Any]] = [] + warnings: list[str] = [] + + for i, raw_entry in enumerate(raw_hosts): + if not isinstance(raw_entry, dict): + warnings.append(f"skipped malformed entry at index {i}: not an object") + continue + type_ = raw_entry.get("type") + if not isinstance(type_, str) or not type_: + warnings.append(f"skipped entry at index {i}: missing or invalid 'type'") + continue + if type_ not in KNOWN_TYPES: + unknown_raw.append(raw_entry) + continue + known_host = entry_to_known_host(raw_entry) + if known_host is None: + name = raw_entry.get("name", "?") + warnings.append( + f"skipped malformed {type_} entry {name!r}: does not match the " + f"expected shape" + ) + continue + hosts.append(known_host) + + return _ParsedDocument( + hosts=hosts, unknown_raw=unknown_raw, warnings=warnings, source_format="v2" + ) + + +def _write_v2_file(path: Path, hosts: list[KnownHost], unknown_raw: list[dict[str, Any]]) -> None: + entries = [known_host_to_entry(h) for h in hosts] + list(unknown_raw) + entries.sort(key=lambda e: (str(e.get("type", "")), str(e.get("name", "")))) + doc = {"version": SUPPORTED_VERSION, "hosts": entries} + text = json.dumps(doc, indent=2, ensure_ascii=False) + "\n" + _atomic_write_text(path, text) + + +def _atomic_write_text(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp(dir=path.parent, prefix=".registry_tmp_") + tmp_path = Path(tmp_name) + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(text) + os.replace(tmp_path, path) + except Exception: + tmp_path.unlink(missing_ok=True) + raise + + +def _non_clobbering_backup_path() -> Path: + base = get_registry_backup_path() + if not base.exists(): + return base + i = 1 + while True: + candidate = base.with_name(f"{base.name}.{i}") + if not candidate.exists(): + return candidate + i += 1 + + +def _canonical_entries(hosts: list[KnownHost], unknown_raw: list[dict[str, Any]]) -> set[str]: + canon = {json.dumps(known_host_to_entry(h), sort_keys=True) for h in hosts} + canon |= {json.dumps(u, sort_keys=True) for u in unknown_raw} + return canon + + +def _entries_equivalent(legacy_result: _LegacyParseResult, v2_parsed: _ParsedDocument) -> bool: + """Data-model §6 footnote: equivalence excludes unparseable lines/warnings.""" + legacy_set = _canonical_entries(legacy_result.hosts, legacy_result.unknown_raw) + v2_set = _canonical_entries(v2_parsed.hosts, v2_parsed.unknown_raw) + return legacy_set == v2_set + + +_DIVERGENCE_WARNING = ( + "both registry.json and known_hosts are present and their contents differ; " + "registry.json is authoritative and known_hosts is being ignored (never " + "merged). Delete known_hosts if it is stale, or re-add any hosts that are " + "missing from registry.json, to resolve this." +) +_BOTH_PRESENT_READONLY_NOTE = ( + "known_hosts is also present alongside registry.json; registry.json is " + "authoritative and known_hosts is being ignored." +) + + +def _resolve_both_present( + v2_parsed: _ParsedDocument, legacy_path: Path, *, readonly: bool +) -> _ParsedDocument: + if readonly: + # Read-only callers never reconcile or rename; they only note that a + # legacy file is being ignored. So there is no need to read it at all — + # skipping the read also removes any window for a concurrent migration + # (which renames the legacy file under the lock) to turn a pure read + # into a `RegistryReadError` crash. + return replace(v2_parsed, warnings=[*v2_parsed.warnings, _BOTH_PRESENT_READONLY_NOTE]) + + # The `legacy_exists` check in `_read_document` ran unlocked, so the file + # may have been renamed away by a concurrent migration before we read it. + # Treat a vanished/unreadable legacy file as "nothing to reconcile" — + # registry.json is authoritative regardless — rather than crashing. + legacy_result = _try_parse_legacy_lines_from_path(legacy_path) + if legacy_result is None: + return v2_parsed + + if _entries_equivalent(legacy_result, v2_parsed): + try: + with registry_lock(): + if legacy_path.exists() and get_registry_path().exists(): + os.rename(legacy_path, _non_clobbering_backup_path()) + except RegistryBusyError: + pass # best-effort; will retry to complete on a future command + return v2_parsed + + return replace(v2_parsed, warnings=[*v2_parsed.warnings, _DIVERGENCE_WARNING]) + + +def _resolve_both_present_locked(v2_parsed: _ParsedDocument, legacy_path: Path) -> _ParsedDocument: + """Same as :func:`_resolve_both_present` for a non-readonly caller that + already holds ``registry_lock()`` (avoids a self-deadlock on re-entry).""" + legacy_result = _try_parse_legacy_lines_from_path(legacy_path) + if legacy_result is None: + return v2_parsed + if _entries_equivalent(legacy_result, v2_parsed): + if legacy_path.exists(): + os.rename(legacy_path, _non_clobbering_backup_path()) + return v2_parsed + return replace(v2_parsed, warnings=[*v2_parsed.warnings, _DIVERGENCE_WARNING]) + + +def _parse_legacy_lines_from_path(path: Path) -> _LegacyParseResult: + try: + text = path.read_text(encoding="utf-8") + except OSError as e: + raise RegistryReadError(f"could not read {path}: {e}") from e + return _parse_legacy_lines(text.splitlines()) + + +def _try_parse_legacy_lines_from_path(path: Path) -> _LegacyParseResult | None: + """Like :func:`_parse_legacy_lines_from_path`, but returns ``None`` when the + file cannot be read (e.g. a concurrent migration renamed it away, or it is + momentarily unreadable). Used only by both-present reconciliation, where an + unreadable legacy file simply means "nothing to reconcile against". + """ + try: + text = path.read_text(encoding="utf-8") + except OSError: + return None + return _parse_legacy_lines(text.splitlines()) + + +# --------------------------------------------------------------------------- +# Locking (research R3) +# --------------------------------------------------------------------------- + +_lock_degradation_warned = False + + +def _warn_lock_unavailable_once() -> None: + global _lock_degradation_warned + if _lock_degradation_warned: + return + _lock_degradation_warned = True + from remo_cli.core.output import print_warning + + print_warning( + "registry locking unavailable on this filesystem; concurrent writes may race." + ) + + +@contextmanager +def registry_lock(timeout_s: float = 5.0) -> Any: + """Advisory lock on the sidecar ``registry.lock`` file (FR-017/FR-019). + + ``fcntl.flock(LOCK_EX | LOCK_NB)`` with a 50ms retry loop up to + *timeout_s*, then :class:`RegistryBusyError`. Degrades to an unlocked + proceed (with a one-time warning) when the filesystem does not support + ``flock`` (e.g. some network filesystems). + """ + lock_path = get_registry_lock_path() + lock_path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(str(lock_path), os.O_CREAT | os.O_RDWR, 0o644) + acquired = False + try: + deadline = time.monotonic() + timeout_s + while True: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + acquired = True + break + except OSError as e: + if e.errno in (errno.ENOLCK, errno.EOPNOTSUPP): + _warn_lock_unavailable_once() + break + if time.monotonic() >= deadline: + raise RegistryBusyError( + "registry is busy — another remo process is writing; " + "retry in a moment" + ) from None + time.sleep(0.05) + yield + finally: + if acquired: + try: + fcntl.flock(fd, fcntl.LOCK_UN) + except OSError: + pass + os.close(fd) + + +# --------------------------------------------------------------------------- +# Migration (research R6) +# --------------------------------------------------------------------------- + + +def _migrate_locked() -> MigrationReport | None: + """Perform the migration; caller MUST already hold ``registry_lock()``.""" + registry_path = get_registry_path() + legacy_path = get_known_hosts_path() + + if registry_path.exists(): + return None + if not legacy_path.exists(): + return None + + legacy_result = _parse_legacy_lines_from_path(legacy_path) + + # Tolerant migration (FR-009/FR-014): a legacy entry that parses but fails + # v2 validation (e.g. a duplicate (type, name), or an out-of-range ssh + # port) is skipped and reported — never fatal. Aborting the whole migration + # over one bad entry would brick every subsequent CLI command until the + # user hand-edited known_hosts. The original bytes are preserved verbatim + # in the renamed backup regardless, so nothing is lost. + valid_hosts: list[KnownHost] = [] + seen: set[tuple[str, str]] = set() + skipped = list(legacy_result.skipped_lines) + for h in legacy_result.hosts: + error = _validate_single_host(h) + if error is not None: + skipped.append(f"{h.type}:{h.name} — {error}") + continue + key = (h.type, h.name) + if key in seen: + skipped.append( + f"{h.type}:{h.name} — duplicate entry for type={h.type!r} name={h.name!r}" + ) + continue + seen.add(key) + valid_hosts.append(h) + + _write_v2_file(registry_path, valid_hosts, legacy_result.unknown_raw) + backup_path = _non_clobbering_backup_path() + os.rename(legacy_path, backup_path) + + return MigrationReport( + migrated_count=len(valid_hosts) + len(legacy_result.unknown_raw), + backup_path=backup_path, + skipped_lines=skipped, + ) + + +def migrate_if_needed() -> MigrationReport | None: + """CLI-only trigger; no-op when ``registry.json`` already exists (FR-010).""" + if get_registry_path().exists(): + return None + if not get_known_hosts_path().exists(): + return None + with registry_lock(): + return _migrate_locked() + + +# --------------------------------------------------------------------------- +# Read +# --------------------------------------------------------------------------- + + +def _read_document(*, readonly: bool, allow_migrate: bool) -> _ParsedDocument: + if readonly: + registry_path = get_registry_path_readonly() + legacy_path = get_known_hosts_path_readonly() + else: + registry_path = get_registry_path() + legacy_path = get_known_hosts_path() + + registry_exists = registry_path.exists() + legacy_exists = legacy_path.exists() + + if not registry_exists and not legacy_exists: + return _ParsedDocument(hosts=[], unknown_raw=[], warnings=[], source_format="empty") + + if not registry_exists and legacy_exists: + if allow_migrate: + migrate_if_needed() + return _read_document(readonly=readonly, allow_migrate=False) + return _read_legacy_file(legacy_path) + + v2_parsed = _read_v2_file(registry_path) + if legacy_exists: + v2_parsed = _resolve_both_present(v2_parsed, legacy_path, readonly=readonly) + return v2_parsed + + +def read_registry(*, readonly: bool = False) -> RegistryView: + """Read the registry. See contracts/registry-accessor-api.md for semantics.""" + parsed = _read_document(readonly=readonly, allow_migrate=not readonly) + return RegistryView( + hosts=parsed.hosts, + warnings=parsed.warnings, + source_format=parsed.source_format, + unknown_entries=len(parsed.unknown_raw), + ) + + +# --------------------------------------------------------------------------- +# Write +# --------------------------------------------------------------------------- + + +def _current_document_locked() -> _ParsedDocument: + """Read current state while already holding ``registry_lock()``. + + Assumes ``_migrate_locked()`` has already run in this same lock scope. + """ + registry_path = get_registry_path() + legacy_path = get_known_hosts_path() + + if not registry_path.exists(): + return _ParsedDocument(hosts=[], unknown_raw=[], warnings=[], source_format="empty") + + v2_parsed = _read_v2_file(registry_path) + if legacy_path.exists(): + v2_parsed = _resolve_both_present_locked(v2_parsed, legacy_path) + return v2_parsed + + +def mutate_registry(mutator: Callable[[list[KnownHost]], list[KnownHost]]) -> RegistryView: + """The only read-modify-write primitive (FR-017 lost-update-safe).""" + with registry_lock(): + _migrate_locked() + current = _current_document_locked() + new_hosts = mutator(list(current.hosts)) + validate_hosts(new_hosts) + _write_v2_file(get_registry_path(), new_hosts, current.unknown_raw) + return RegistryView( + hosts=new_hosts, + warnings=[], + source_format="v2", + unknown_entries=len(current.unknown_raw), + ) + + +def replace_registry(hosts: list[KnownHost], *, allow_empty: bool = False) -> RegistryView: + """Wholesale replacement (web setup PUT apply). + + Web wholesale-replace semantics: this deliberately does NOT migrate or + merge any legacy ``known_hosts`` file. The only caller (web setup + ``_apply_payload``) removes the legacy mirror file explicitly afterward + (contracts/mirror-payload-v2.md §3), so migrating it here would (a) leave a + stray ``known_hosts.v1.bak`` on the service volume and (b) let a *malformed* + stale mirror abort an otherwise-valid apply with a validation error. Only + unknown-type entries already in ``registry.json`` are preserved. + """ + if not hosts and not allow_empty: + raise RegistryValidationError( + "refusing to write an empty registry without allow_empty=True" + ) + with registry_lock(): + registry_path = get_registry_path() + unknown_raw = _read_v2_file(registry_path).unknown_raw if registry_path.exists() else [] + validate_hosts(hosts) + _write_v2_file(registry_path, hosts, unknown_raw) + return RegistryView( + hosts=hosts, + warnings=[], + source_format="v2", + unknown_entries=len(unknown_raw), + ) diff --git a/src/remo_cli/core/web_adopt.py b/src/remo_cli/core/web_adopt.py index c3d177c..175986c 100644 --- a/src/remo_cli/core/web_adopt.py +++ b/src/remo_cli/core/web_adopt.py @@ -31,11 +31,14 @@ The service has no registry-read endpoint, so "unchanged since the last push" is decided workstation-side by a **non-secret** cache -(``~/.config/remo/web-service.json``) mapping each service ``deployment_id`` to -``{instance name -> {fingerprint, host_keys}}`` — no URL and no code are ever -stored. The ``fingerprint`` is a SHA256 over the canonical registry-entry fields -(type/name/host/user/instance_id/access_mode/region) and ``host_keys`` are the -verified known_hosts lines pushed for that instance. +(``~/.config/remo/web-service.json``, ``cache_version: 2``) mapping each +service ``deployment_id`` to ``{instance name -> {fingerprint, host_keys}}`` — +no URL and no code are ever stored. The ``fingerprint`` is a SHA256 over the +canonical sorted-key JSON of the instance's v2 hostEntry (registry-file-v2.md) +and ``host_keys`` are the verified known_hosts lines pushed for that instance. +Any cache without ``cache_version: 2`` (including every pre-015 file, which had +no version field at all) is treated as empty, forcing one full +re-verification push after an upgrade (research R10). On ``remo web push``, a direct-access instance whose current fingerprint matches the cache for the service's ``deployment_id`` skips keyscan + authorize @@ -69,6 +72,7 @@ from pathlib import Path from typing import Any, Literal +from remo_cli.core import registry from remo_cli.core.config import get_known_hosts_path_readonly, get_remo_home_readonly from remo_cli.core.known_hosts import get_known_hosts from remo_cli.core.output import ( @@ -88,8 +92,10 @@ # Constants # --------------------------------------------------------------------------- -#: Adoption payload schema version (contracts/setup-api.md). -PAYLOAD_VERSION = 1 +#: Adoption payload schema version this workstation SENDS (specs/015-registry-v2/ +#: contracts/mirror-payload-v2.md §2) — the v2 hostEntry shape, no overloaded +#: fields on the wire. +PAYLOAD_VERSION = 2 #: Default service port assumed by --via when the target URL names none. DEFAULT_SERVICE_PORT = 8080 @@ -169,6 +175,12 @@ class TunnelError(AdoptError): """The --via SSH tunnel could not be established (FR-018).""" +class UnsupportedPayloadVersionError(AdoptError): + """The service does not advertise support for this workstation's payload + version (specs/015-registry-v2/contracts/mirror-payload-v2.md §1, FR-021). + Raised BEFORE any instance processing or mutation — fail truly fast.""" + + # --------------------------------------------------------------------------- # T015 — Setup-API HTTP client (stdlib urllib.request, research R9) # --------------------------------------------------------------------------- @@ -320,25 +332,13 @@ def is_direct_access(host: KnownHost) -> bool: """True when the entry is reached over plain SSH (not SSM-routed). SSM entries appear in the pushed ``registry`` mirror but must never carry - ``host_keys`` entries and are never key-authorized (FR-012). Mirrors - ``KnownHost.to_line``: an entry with an ``instance_id`` and no explicit - ``access_mode`` defaults to SSM. + ``host_keys`` entries and are never key-authorized (FR-012). Registry v2 + entries always carry an explicit, normalized ``access`` of ``"direct"`` or + ``"ssm"`` (the legacy implicit-empty-access-mode-means-ssm quirk is + resolved at the accessor boundary — see core/registry.py — so no fallback + inference is needed here anymore). """ - if host.access_mode == "ssm": - return False - return not (host.instance_id and not host.access_mode) - - -def _registry_entry(host: KnownHost) -> dict[str, str]: - return { - "type": host.type, - "name": host.name, - "host": host.host, - "user": host.user, - "instance_id": host.instance_id, - "access_mode": host.access_mode, - "region": host.region, - } + return host.access_mode != "ssm" def build_adoption_payload( @@ -365,11 +365,27 @@ def build_adoption_payload( } return { "version": PAYLOAD_VERSION, - "registry": [_registry_entry(h) for h in hosts], + "registry": [registry.known_host_to_entry(h) for h in hosts], "host_keys": filtered_keys, } +def _check_payload_version_supported(status: dict[str, Any]) -> None: + """FR-021: abort before any mutation if the service doesn't speak our + payload version. ``payload_versions`` absent on ``GET /setup/status`` + means a pre-015 service, which only ever spoke v1. + """ + versions = status.get("payload_versions") + if not isinstance(versions, list): + versions = [1] + if PAYLOAD_VERSION not in versions: + raise UnsupportedPayloadVersionError( + f"this remo-web deployment only accepts registry payload " + f"v{max(versions) if versions else 1} — upgrade the remo-web " + "container image, then re-run the push." + ) + + def _empty_registry_message() -> str: return ( f"the local registry ({get_known_hosts_path_readonly()}) is empty. " @@ -721,19 +737,26 @@ class CachedInstance: def instance_fingerprint(host: KnownHost) -> str: - """SHA256 over the canonical registry-entry fields of *host*. + """SHA256 over the canonical sorted-key JSON of *host*'s v2 hostEntry. - Any change to the fields the service mirrors (host, user, access mode, …) - changes the fingerprint, forcing the full keyscan+authorize treatment on - the next push. + Any change to the fields the service mirrors (host, user, access, per-type + nested fields, …) changes the fingerprint, forcing the full + keyscan+authorize treatment on the next push. (research R10: replaces the + legacy 7-field digest.) """ - canonical = json.dumps(_registry_entry(host), sort_keys=True) + canonical = json.dumps(registry.known_host_to_entry(host), sort_keys=True) return hashlib.sha256(canonical.encode("utf-8")).hexdigest() #: Push cache shape: deployment_id -> {instance name -> CachedInstance}. PushCache = dict[str, dict[str, "CachedInstance"]] +#: Push-cache file format version (research R10). Any other/missing value is +#: treated as an empty cache — a one-time full re-verification push after a +#: registry-format upgrade (FR-026), since fingerprints are computed +#: differently under each cache format. +PUSH_CACHE_VERSION = 2 + def push_cache_path() -> Path: """Path of the non-secret push cache (``~/.config/remo/web-service.json``).""" @@ -763,8 +786,10 @@ def load_push_cache() -> PushCache: Files written by the 011 credential format (top-level ``url``/``token`` + name-keyed ``push_cache``) do not match the deployment-keyed shape and are - ignored (they parse to an empty cache), so the next push simply retries in - full and the next save overwrites the stale file — no secret is ever read. + ignored. Files without ``cache_version: 2`` (either the pre-015 format, + which had no version field at all, or a future incompatible one) are also + treated as empty (research R10) — the next push simply retries in full and + the next save overwrites the stale file — no secret is ever read. """ path = push_cache_path() try: @@ -773,6 +798,8 @@ def load_push_cache() -> PushCache: return {} if not isinstance(parsed, dict): return {} + if parsed.get("cache_version") != PUSH_CACHE_VERSION: + return {} raw_cache = parsed.get("push_cache") if not isinstance(raw_cache, dict): return {} @@ -796,6 +823,7 @@ def save_push_cache(cache: PushCache) -> Path: path.parent.mkdir(parents=True, exist_ok=True) payload = json.dumps( { + "cache_version": PUSH_CACHE_VERSION, "push_cache": { deployment_id: { name: {"fingerprint": c.fingerprint, "host_keys": c.host_keys} @@ -1102,11 +1130,13 @@ def _adopt_flow( allow_empty: bool, interactive: bool, ) -> AdoptResult: - # Step 1: status precheck (FR-017). + # Step 1: status precheck (FR-017), then the payload-version skew gate + # (FR-021) — BEFORE any instance processing or mutation. status = client.get_status() state = str(status.get("state", "unknown")) if state == "mount_configured": raise MountConfiguredError(_MOUNT_CONFIGURED_MSG) + _check_payload_version_supported(status) print_info( f"Service state: {state} " f"({status.get('registry_instances', 0)} instances currently registered)" @@ -1216,9 +1246,12 @@ def _push_flow( interactive: bool, ) -> AdoptResult: # Step 1: status precheck (FR-017) — a mount-configured service is read-only. + # Then the payload-version skew gate (FR-021) — BEFORE any instance + # processing or mutation. status = client.get_status() if str(status.get("state", "unknown")) == "mount_configured": raise MountConfiguredError(_MOUNT_CONFIGURED_MSG) + _check_payload_version_supported(status) # Step 2: service identity + the push cache entry for this deployment. identity = client.get_identity() diff --git a/src/remo_cli/providers/added.py b/src/remo_cli/providers/added.py index afa03fb..10151c1 100644 --- a/src/remo_cli/providers/added.py +++ b/src/remo_cli/providers/added.py @@ -36,17 +36,16 @@ def _reject_unsafe_field(label: str, value: str) -> None: - """Reject a registry field that would corrupt the colon-delimited line. + """Reject a control character in a registry field value. - The known-hosts store is both ``:``-delimited and newline-delimited, so a - field containing ``:`` shifts every later field on reload and a control - character (newline/tab/…) can inject or truncate a line. Neither must ever - be persisted (FR-013). + The registry (registry.json, format v2) rejects control characters and + newlines in any string field (data-model.md V2); checking here gives a + friendlier, ``add``-specific error message before the value is parsed any + further. Colons are unrestricted in the JSON registry — the previous + colon-delimited format's positional-overloading problem no longer exists. """ if any(ord(c) < 0x20 or ord(c) == 0x7F for c in value): raise ValueError(f"{label} contains control characters") - if ":" in value: - raise ValueError(f"{label} must not contain ':'") def _find_name_conflict(name: str) -> KnownHost | None: @@ -86,8 +85,11 @@ def parse_ssh_target( (:data:`DEFAULT_ADDED_HOST_USER`) is applied; when no port is present, :data:`DEFAULT_SSH_PORT` is used. - Un-bracketed IPv6 literals (and the bracketed ``[::1]:22`` form, which is out - of scope for this feature) are rejected so a malformed line is never written. + IPv6 literals are accepted two ways (US2, T020): the OpenSSH-style + bracketed form ``[v6][:port]`` (e.g. ``[2001:db8::7]:2222``), and a bare + bracket-less TARGET containing more than one colon, which is treated as + a host with no port suffix (a legal ``host:port`` has exactly one colon, + so 2+ colons unambiguously means an un-bracketed IPv6 literal). Raises ------ @@ -106,23 +108,35 @@ def parse_ssh_target( user_part = "" rest = raw - # IPv6: a bracketed form, or more than one colon in the host portion, is an - # IPv6 literal — reject with guidance (a legal host:port has exactly one ':'). if rest.startswith("["): - raise ValueError( - f"'{target}': IPv6 literals are not supported. " - "Use a hostname or an '~/.ssh/config' alias." - ) - if rest.count(":") > 1: - raise ValueError( - f"'{target}': IPv6 literals are not supported. " - "Use a hostname or an '~/.ssh/config' alias." - ) - - if rest.count(":") == 1: + closing = rest.find("]") + if closing == -1: + raise ValueError(f"'{target}': unmatched '[' in IPv6 literal") + host_part = rest[1:closing] + remainder = rest[closing + 1 :] + if remainder and not remainder.startswith(":"): + raise ValueError( + f"'{target}': unexpected characters after ']': {remainder!r}" + ) + port_str = remainder[1:] if remainder else "" + if port_str: + try: + embedded_port: int | None = int(port_str) + except ValueError: + raise ValueError( + f"'{target}': port '{port_str}' is not a number" + ) from None + else: + embedded_port = None + elif rest.count(":") > 1: + # Bare (bracket-less) IPv6 literal — no port suffix is representable + # without brackets, so the whole remainder is the host. + host_part = rest + embedded_port = None + elif rest.count(":") == 1: host_part, _, port_str = rest.partition(":") try: - embedded_port: int | None = int(port_str) + embedded_port = int(port_str) except ValueError: raise ValueError( f"'{target}': port '{port_str}' is not a number" @@ -239,9 +253,9 @@ def add( _reject_unsafe_field("identity path", identity) except ValueError: print_error( - "Invalid --identity: the path must not contain ':' or control " - "characters (they would corrupt the colon-delimited registry " - "line). Use an '~/.ssh/config' IdentityFile entry for such a key." + "Invalid --identity: the path must not contain control " + "characters. Use an '~/.ssh/config' IdentityFile entry for " + "such a key." ) return 2 diff --git a/src/remo_cli/web/api/setup.py b/src/remo_cli/web/api/setup.py index fc7d4ed..ca25423 100644 --- a/src/remo_cli/web/api/setup.py +++ b/src/remo_cli/web/api/setup.py @@ -20,14 +20,18 @@ T011/T012/T013), all inheriting the router-level token dependency: - ``GET /status`` -- configuration state + identity presence; cheap, pollable. + Also advertises ``payload_versions`` (specs/015-registry-v2/contracts/ + mirror-payload-v2.md §1) so the workstation can fail fast on version skew. - ``GET /identity`` -- deployment id + public key; generates the service identity on first call when unconfigured (idempotent, FR-002); ``409 {"reason": "mount_configured"}`` when the deployment is mount-configured. -- ``PUT /registry`` -- the `AdoptionPayload` mirror. Validates EVERYTHING - before writing anything (FR-019), then applies atomically: service - known_hosts file first, registry file last (research R5), each via - temp-file + ``os.replace``. Live terminal sessions are never touched -- - they hold their own SSH processes; this is file replacement only. +- ``PUT /registry`` -- the `AdoptionPayload` mirror. Accepts payload v1 (legacy + colon-shaped fields) AND v2 (registry-file-v2.md hostEntry shape); v1 entries + are mapped through the same legacy->v2 mapper the CLI migration uses + (specs/015-registry-v2/contracts/mirror-payload-v2.md §2). Validates + EVERYTHING before writing anything (FR-019), then applies atomically: + service known_hosts file first, registry.json (v2) second, any stale legacy + mirror file removed last (research R9). - ``POST /verify`` -- JSON wrapper around `web.check.run_checks()` with instance checks included (sync route: FastAPI runs it in a threadpool, so the ~5s-per-unreachable-instance round-trips never block the event loop). @@ -36,15 +40,18 @@ from __future__ import annotations import logging +import os import re +import tempfile +from pathlib import Path from typing import Any from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import JSONResponse -from pydantic import BaseModel, Field, ValidationError +from pydantic import BaseModel, ConfigDict, Field, ValidationError -from remo_cli.core.config import get_known_hosts_path, get_known_hosts_path_readonly -from remo_cli.core.known_hosts import _write_lines_atomically +from remo_cli.core import registry +from remo_cli.core.config import get_known_hosts_path from remo_cli.models.host import KnownHost from remo_cli.web import check as web_check from remo_cli.web.config import WebSettings @@ -58,6 +65,8 @@ logger = logging.getLogger("remo_cli.web.setup") +SUPPORTED_PAYLOAD_VERSIONS: list[int] = [1, 2] + def _get_settings(request: Request) -> WebSettings: """The app-wide `WebSettings` (set in `create_app()`), like health.py.""" @@ -103,7 +112,7 @@ async def require_pairing_code(request: Request) -> None: # --------------------------------------------------------------------------- -# Request/response models (contracts/setup-api.md shapes) +# Request/response models (contracts/setup-api.md, mirror-payload-v2.md shapes) # --------------------------------------------------------------------------- @@ -112,6 +121,7 @@ class SetupStatusResponse(BaseModel): deployment_id: str | None public_key_available: bool registry_instances: int + payload_versions: list[int] = Field(default_factory=lambda: list(SUPPORTED_PAYLOAD_VERSIONS)) class IdentityResponse(BaseModel): @@ -119,8 +129,8 @@ class IdentityResponse(BaseModel): public_key: str -class RegistryEntryIn(BaseModel): - """One `AdoptionPayload.registry` entry -- mirrors `models/host.py:KnownHost`.""" +class RegistryEntryV1In(BaseModel): + """One v1 `AdoptionPayload.registry` entry -- the legacy colon-shaped fields.""" type: str name: str @@ -131,11 +141,36 @@ class RegistryEntryIn(BaseModel): region: str = "" -class AdoptionPayloadIn(BaseModel): - """`PUT /registry` body (data-model.md AdoptionPayload) -- a full mirror.""" +class AdoptionPayloadV1In(BaseModel): + """v1 `PUT /registry` body (accepted for backward compatibility, FR-022).""" + + version: int + registry: list[RegistryEntryV1In] + host_keys: dict[str, list[str]] = Field(default_factory=dict) + + +class RegistryEntryV2In(BaseModel): + """One v2 hostEntry -- exact schema from registry-file-v2.md. + + ``extra="allow"`` so the per-type nested object (``incus``/``proxmox``/ + ``aws``/``ssh``, keyed by ``type``) round-trips without a dedicated + sub-model per type; shape is validated by :func:`registry.entry_to_known_host`. + """ + + model_config = ConfigDict(extra="allow") + + type: str + name: str + host: str + user: str + access: str + + +class AdoptionPayloadV2In(BaseModel): + """v2 `PUT /registry` body (canonical; what an upgraded CLI sends).""" version: int - registry: list[RegistryEntryIn] + registry: list[RegistryEntryV2In] host_keys: dict[str, list[str]] = Field(default_factory=dict) @@ -162,22 +197,24 @@ class VerifyResponse(BaseModel): # --------------------------------------------------------------------------- -def _read_registry_readonly() -> list[KnownHost]: - """Parse the registry with no mkdir side effects; unreadable/absent -> [].""" +def _write_lines_atomically(path: Path, lines: list[str]) -> None: + """Write *lines* to *path* atomically via a same-directory temp file + rename. + + Used only for the service's own SSH ``known_hosts`` trust file (not the + remo registry, which goes through :mod:`core.registry`'s own atomic writer). + """ + dir_ = path.parent + dir_.mkdir(parents=True, exist_ok=True) + fd, tmp_path_str = tempfile.mkstemp(dir=dir_, prefix=".known_hosts_tmp_") + tmp_path = Path(tmp_path_str) try: - text = get_known_hosts_path_readonly().read_text() - except OSError: - return [] - hosts: list[KnownHost] = [] - for raw_line in text.splitlines(): - line = raw_line.strip() - if not line: - continue - try: - hosts.append(KnownHost.from_line(line)) - except ValueError: - continue - return hosts + with os.fdopen(fd, "w") as fh: + for line in lines: + fh.write(line + "\n") + os.replace(tmp_path, path) + except Exception: + tmp_path.unlink(missing_ok=True) + raise def _mount_configured_response() -> JSONResponse: @@ -190,9 +227,17 @@ def _invalid_payload(detail: str) -> JSONResponse: ) -def _is_ssm_entry(entry: RegistryEntryIn) -> bool: - """Mirrors `KnownHost.to_line`'s default: instance_id set + no explicit mode -> ssm.""" - return entry.access_mode == "ssm" or bool(entry.instance_id and not entry.access_mode) +def _unsupported_payload_version(version: Any) -> JSONResponse: + return JSONResponse( + status_code=400, + content={ + "error": { + "code": "unsupported_payload_version", + "supported": list(SUPPORTED_PAYLOAD_VERSIONS), + "received": version, + } + }, + ) #: Plausible OpenSSH key-type token, e.g. ssh-ed25519, ecdsa-sha2-nistp256, @@ -224,31 +269,46 @@ def _known_hosts_line_error(line: str) -> str | None: return None -def _validate_payload(payload: AdoptionPayloadIn) -> str | None: - """All semantic `AdoptionPayload` rules (data-model.md); error detail or None. - - All-or-nothing: callers write NOTHING unless this returns None (FR-019). - The empty-registry guard is separate (its own 422 reason). - """ - if payload.version != 1: - return f"unsupported payload version {payload.version} (expected 1)" - - names: set[str] = set() - for index, entry in enumerate(payload.registry): +def _map_v1_entries(entries: list[RegistryEntryV1In]) -> tuple[list[KnownHost], str | None]: + """Map v1 entries through the shared legacy->v2 mapper (data-model.md §4).""" + hosts: list[KnownHost] = [] + for index, entry in enumerate(entries): for field_name in ("type", "name", "host", "user"): if not getattr(entry, field_name).strip(): - return f"registry[{index}]: field {field_name!r} must be non-empty" - for field_name in ("type", "name", "host", "user", "instance_id", "access_mode", "region"): - value = getattr(entry, field_name) - if ":" in value or "\n" in value: - return ( - f"registry[{index}].{field_name}: value {value!r} cannot contain " - "':' or newline (colon-delimited registry format)" - ) - names.add(entry.name) - - ssm_names = {entry.name for entry in payload.registry if _is_ssm_entry(entry)} - for name, lines in payload.host_keys.items(): + return [], f"registry[{index}]: field {field_name!r} must be non-empty" + v2_entry = registry.legacy_fields_to_entry( + entry.type, + entry.name, + entry.host, + entry.user, + entry.instance_id, + entry.access_mode, + entry.region, + ) + known_host = registry.entry_to_known_host(v2_entry) + if known_host is None: + return [], f"registry[{index}]: unrecognized type {entry.type!r}" + hosts.append(known_host) + return hosts, None + + +def _map_v2_entries(entries: list[RegistryEntryV2In]) -> tuple[list[KnownHost], str | None]: + hosts: list[KnownHost] = [] + for index, entry in enumerate(entries): + raw = entry.model_dump() + known_host = registry.entry_to_known_host(raw) + if known_host is None: + return [], f"registry[{index}]: does not match the expected hostEntry shape" + hosts.append(known_host) + return hosts, None + + +def _validate_host_keys( + hosts: list[KnownHost], host_keys: dict[str, list[str]] +) -> str | None: + names = {h.name for h in hosts} + ssm_names = {h.name for h in hosts if h.access_mode == "ssm"} + for name, lines in host_keys.items(): if name not in names: return f"host_keys entry {name!r} does not reference any registry entry" if name in ssm_names: @@ -260,37 +320,30 @@ def _validate_payload(payload: AdoptionPayloadIn) -> str | None: return None -def _apply_payload(payload: AdoptionPayloadIn, settings: WebSettings) -> None: - """Atomic two-file apply: service known_hosts FIRST, registry LAST (R5). +def _apply_payload( + hosts: list[KnownHost], host_keys: dict[str, list[str]], settings: WebSettings +) -> None: + """Ordered, crash-convergent apply (contracts/mirror-payload-v2.md §3): - Each file is replaced via temp-file + ``os.replace`` (the - `core/known_hosts.py` atomic pattern). A crash between the two writes - leaves a superset of needed host keys and the old registry -- safe, and - convergent on re-push (FR-015). Never touches live terminal sessions: - established SSH processes hold their own file descriptors. + 1. service trust file (``web-identity/known_hosts``) -- unchanged from today. + 2. ``registry.json`` (v2) via :func:`core.registry.replace_registry`. + 3. remove any legacy ``known_hosts`` mirror file left from a pre-upgrade + push (service-owned replaceable state, not user data). + + A crash mid-sequence leaves a readable superset; re-push converges. """ known_hosts_lines: list[str] = [] - for entry in payload.registry: - for line in payload.host_keys.get(entry.name, []): + for host in hosts: + for line in host_keys.get(host.name, []): known_hosts_lines.append(line.strip()) identity_dir = settings.web_identity_dir identity_dir.mkdir(mode=0o700, parents=True, exist_ok=True) _write_lines_atomically(settings.service_known_hosts_path, known_hosts_lines) - registry_lines = [ - KnownHost( - type=entry.type, - name=entry.name, - host=entry.host, - user=entry.user, - instance_id=entry.instance_id, - access_mode=entry.access_mode, - region=entry.region, - ).to_line() - for entry in payload.registry - ] - _write_lines_atomically(get_known_hosts_path(), registry_lines) + registry.replace_registry(hosts, allow_empty=True) + + get_known_hosts_path().unlink(missing_ok=True) # --------------------------------------------------------------------------- @@ -303,11 +356,13 @@ def get_status(request: Request) -> SetupStatusResponse: """`GET /api/v1/setup/status` -- service mode + identity presence. Cheap.""" settings = _get_settings(request) identity = load_service_identity(settings) # no side effects + registry_instances = len(registry.read_registry(readonly=True).hosts) return SetupStatusResponse( state=detect_state(settings).value, deployment_id=(identity.deployment_id or None) if identity else None, public_key_available=identity is not None, - registry_instances=len(_read_registry_readonly()), + registry_instances=registry_instances, + payload_versions=list(SUPPORTED_PAYLOAD_VERSIONS), ) @@ -335,17 +390,32 @@ def put_registry( ) -> RegistryApplyResponse | JSONResponse: """`PUT /api/v1/setup/registry` -- apply the adoption mirror atomically. - Validates the FULL payload before writing anything (FR-019); a - mount-configured deployment is read-only via this API (409, FR-017); an - empty registry requires the explicit ``allow_empty=true`` opt-out - (defense-in-depth for the CLI-side FR-016 guard). + Accepts payload v1 (legacy fields, mapped through the shared legacy->v2 + mapper) and v2 (registry-file-v2.md hostEntry shape); always stores v2 + (FR-020/FR-022). Validates the FULL payload before writing anything + (FR-019); a mount-configured deployment is read-only via this API (409, + FR-017); an empty registry requires the explicit ``allow_empty=true`` + opt-out (defense-in-depth for the CLI-side FR-016 guard); an unsupported + ``version`` is rejected with the prior mirror left completely intact + (400 ``unsupported_payload_version``, FR-021). """ settings = _get_settings(request) if detect_state(settings) is ConfigurationState.MOUNT_CONFIGURED: return _mount_configured_response() + version = body.get("version") + if version not in SUPPORTED_PAYLOAD_VERSIONS: + return _unsupported_payload_version(version) + try: - payload = AdoptionPayloadIn.model_validate(body) + if version == 1: + payload_v1 = AdoptionPayloadV1In.model_validate(body) + hosts, error = _map_v1_entries(payload_v1.registry) + host_keys = payload_v1.host_keys + else: + payload_v2 = AdoptionPayloadV2In.model_validate(body) + hosts, error = _map_v2_entries(payload_v2.registry) + host_keys = payload_v2.host_keys except ValidationError as exc: detail = "; ".join( f"{'.'.join(str(part) for part in err['loc'])}: {err['msg']}" @@ -353,28 +423,41 @@ def put_registry( ) return _invalid_payload(detail or "malformed payload") - error = _validate_payload(payload) if error is not None: return _invalid_payload(error) - if not payload.registry and not allow_empty: + if not hosts and not allow_empty: return JSONResponse(status_code=422, content={"reason": "empty_registry"}) try: - _apply_payload(payload, settings) - except OSError as exc: + registry.validate_hosts(hosts) + except registry.RegistryValidationError as exc: + return _invalid_payload(str(exc)) + + host_keys_error = _validate_host_keys(hosts, host_keys) + if host_keys_error is not None: + return _invalid_payload(host_keys_error) + + try: + _apply_payload(hosts, host_keys, settings) + except (OSError, registry.RegistryError) as exc: + # OSError: a filesystem write failed. RegistryError: a lock timeout + # (RegistryBusyError) or an unreadable/newer-version registry.json on + # the service volume -- either way a clean 500, never an uncaught + # traceback (the hosts themselves were already validated above). logger.error("registry apply failed: %s", exc) raise HTTPException(status_code=500, detail="failed to apply registry") from exc logger.info( - "adoption mirror applied: %d registry entries, %d instances with host keys", - len(payload.registry), - len(payload.host_keys), + "adoption mirror applied (payload v%d): %d registry entries, %d instances with host keys", + version, + len(hosts), + len(host_keys), ) return RegistryApplyResponse( applied=True, - registry_instances=len(payload.registry), - host_key_instances=len(payload.host_keys), + registry_instances=len(hosts), + host_key_instances=len(host_keys), ) diff --git a/src/remo_cli/web/check.py b/src/remo_cli/web/check.py index 42d04af..645705f 100644 --- a/src/remo_cli/web/check.py +++ b/src/remo_cli/web/check.py @@ -45,18 +45,15 @@ SshTransportError, get_capabilities, ) -from remo_cli.core.config import get_known_hosts_path_readonly +from remo_cli.core.config import get_known_hosts_path_readonly, get_registry_path_readonly +from remo_cli.core.registry import RegistryNewerVersionError, RegistryReadError, read_registry from remo_cli.core.ssh import build_ssh_base_cmd from remo_cli.models.host import KnownHost from remo_cli.web import health from remo_cli.web.config import WebSettings from remo_cli.web.operator_auth import OperatorAuthConfigError, build_operator_auth_provider from remo_cli.web.state import ConfigurationState, detect_state -from remo_cli.web.discovery import ( - _classify_ssh_transport, - _looks_like_missing_remo_host, - _read_known_hosts_readonly, -) +from remo_cli.web.discovery import _classify_ssh_transport, _looks_like_missing_remo_host __all__ = ["CheckResult", "all_passed", "format_results", "run_checks"] @@ -181,16 +178,57 @@ def _operator_auth_check(settings: WebSettings) -> CheckResult: ) -def _registry_check(hosts: list[KnownHost]) -> CheckResult: +def _registry_check(hosts: list[KnownHost], source_format: str) -> CheckResult: + """Build the passing "registry" line. Only called when `_read_registry_for_check` + reports no failure (its own missing/unreadable/newer-version FAIL lines + are built there instead, so this always reports success).""" + path = get_registry_path_readonly() if source_format == "v2" else get_known_hosts_path_readonly() + return CheckResult( + "registry", + True, + f"readable at {path} ({len(hosts)} instances, format={source_format})", + ) + + +def _read_registry_for_check() -> tuple[list[KnownHost], str, CheckResult | None]: + """Read the registry via the accessor for `run_checks`. + + Returns ``(hosts, source_format, failure)``. *failure* is ``None`` unless + the registry is unusable: `health._check_registry()` first gates the + plain missing/unreadable-*file* cases (byte-level readability, EACCES-safe + against a read-only or unmounted volume) -- when it isn't "ok" we don't + even attempt to parse. Once bytes are readable, `read_registry()` can + still fail structurally, principally `RegistryNewerVersionError` + (`registry.json` written by a newer, unsupported format version), which + must FAIL this specific check with clear remediation rather than silently + reporting zero instances (FR-025). A defensive `OSError` catch covers the + same EACCES-on-``Path.exists()`` edge case `health._check_registry()` + guards against (e.g. a TOCTOU race between the two checks). + """ status = health._check_registry() path = get_known_hosts_path_readonly() - if status == "ok": - return CheckResult("registry", True, f"readable at {path} ({len(hosts)} instances)") if status == "missing": - return CheckResult("registry", False, f"not found at {path}", _REMEDIATE_REGISTRY_MISSING) - return CheckResult( - "registry", False, f"{path} exists but is not readable", _REMEDIATE_REGISTRY_MISSING - ) + return [], "empty", CheckResult( + "registry", False, f"not found at {path}", _REMEDIATE_REGISTRY_MISSING + ) + if status == "unreadable": + return [], "unknown", CheckResult( + "registry", False, f"{path} exists but is not readable", _REMEDIATE_REGISTRY_MISSING + ) + + try: + view = read_registry(readonly=True) + except RegistryNewerVersionError as exc: + # The exception message already names the remediation (upgrade remo, + # or restore the known_hosts.v1.bak backup) -- surface it as-is + # rather than duplicating it in a separate `remediation` field. + return [], "unknown", CheckResult("registry", False, str(exc)) + except (RegistryReadError, OSError) as exc: + return [], "unknown", CheckResult( + "registry", False, f"{path} exists but is not readable: {exc}", + _REMEDIATE_REGISTRY_MISSING, + ) + return view.hosts, view.source_format, None def _ssh_identity_check(settings: WebSettings) -> CheckResult: @@ -312,12 +350,13 @@ def run_checks( _executable_check("ssh", "ssh"), ] - hosts = _read_known_hosts_readonly() + hosts, source_format, registry_failure = _read_registry_for_check() + registry_result = registry_failure or _registry_check(hosts, source_format) results = [ _configuration_check(state), _operator_auth_check(settings), - _registry_check(hosts), + registry_result, _ssh_identity_check(settings), _runtime_dir_check(settings.ssh_control_dir), _executable_check("ssh", "ssh"), diff --git a/src/remo_cli/web/discovery.py b/src/remo_cli/web/discovery.py index 9c5d6f6..5c95ae7 100644 --- a/src/remo_cli/web/discovery.py +++ b/src/remo_cli/web/discovery.py @@ -1,11 +1,12 @@ """Concurrent per-instance session discovery. -Reads the read-only registry (:func:`~remo_cli.core.config.get_known_hosts_path_readonly`), -runs `remo-host capabilities`/`sessions list` against every registered -instance concurrently (bounded by `WebSettings.discovery_concurrency`, each -call bounded by `WebSettings.discovery_timeout_s`), and maintains an -in-memory TTL cache of the resulting `DiscoverySnapshot` per instance plus a -flattened `SessionTarget` index (see data-model.md and R1/R10). +Reads the registry via the read-only accessor +(:func:`~remo_cli.core.registry.read_registry`, ``readonly=True``), runs +`remo-host capabilities`/`sessions list` against every registered instance +concurrently (bounded by `WebSettings.discovery_concurrency`, each call +bounded by `WebSettings.discovery_timeout_s`), and maintains an in-memory TTL +cache of the resulting `DiscoverySnapshot` per instance plus a flattened +`SessionTarget` index (see data-model.md and R1/R10). `remo_host_client`'s functions are synchronous (`subprocess.run`-based); this module runs them in the default thread-pool executor via @@ -20,10 +21,11 @@ import asyncio import hashlib +import logging import time from datetime import datetime, timezone -from remo_cli.core.config import get_known_hosts_path_readonly +from remo_cli.core.registry import RegistryError, read_registry from remo_cli.core.remo_host_client import ( IncompatibleProtocolError, MalformedResponseError, @@ -43,6 +45,8 @@ __all__ = ["DiscoveryService", "derive_instance_id"] +logger = logging.getLogger("remo_cli.web.discovery") + def _now_iso() -> str: return datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") @@ -65,40 +69,34 @@ def derive_instance_id(host: KnownHost) -> str: # --------------------------------------------------------------------------- -def _read_known_hosts_readonly() -> list[KnownHost]: - """Read the registry via the read-only-safe path (no ``mkdir`` side effect). - - `core.known_hosts.get_known_hosts()` is built on `get_known_hosts_path()`, - which goes through `get_remo_home()` and therefore *creates* the config - directory as a side effect — unsafe against a read-only-mounted registry - (R10). This re-implements the same tolerant line-parsing - (`KnownHost.from_line`, skipping blank/unparseable lines, never raising) - directly against `get_known_hosts_path_readonly()` instead. +def _read_registry_hosts_readonly() -> list[KnownHost]: + """Read the registry via the read-only accessor (no side effects). + + Delegates to `core.registry.read_registry(readonly=True)`, which reads + `registry.json` if present, else the legacy `known_hosts` file in place, + with no ``mkdir``/write/lock side effects — safe against a + read-only-mounted registry (R10). Per-entry problems come back as + `RegistryView.warnings` (logged, never raised); structural problems + (`RegistryReadError`, `RegistryNewerVersionError`) are caught here and + degrade to "no known hosts", the same resilience the old + line-parsing implementation had for a missing/unreadable file — + discovery must never crash the whole service over one bad registry. + + Also catches a plain `OSError`: `Path.exists()`/`read_text()` (used + internally by the accessor) raise on EACCES rather than swallowing it + (only ENOENT-ish errors are swallowed), so an entirely untraversable + `REMO_HOME` -- e.g. bind-mounted from a host directory this uid cannot + traverse -- surfaces as a raw `OSError`, not a `RegistryError` subclass. """ - path = get_known_hosts_path_readonly() try: - if not path.exists(): - return [] - raw_lines = path.read_text().splitlines() - except OSError: - # `Path.exists()`/`read_text()` raise on EACCES (only ENOENT-ish - # errors are swallowed). An unreadable registry -- e.g. bind-mounted - # from a host directory this uid cannot traverse -- must not escape - # as a traceback from every caller, `remo web check` included. - # Degrade to "no instances"; `health._check_registry` separately - # reports the mount as unreadable, with remediation. + view = read_registry(readonly=True) + except (RegistryError, OSError) as exc: + logger.warning("registry read failed, degrading to empty host list: %s", exc) return [] - hosts: list[KnownHost] = [] - for raw_line in raw_lines: - line = raw_line.strip() - if not line: - continue - try: - hosts.append(KnownHost.from_line(line)) - except ValueError: - continue - return hosts + for warning in view.warnings: + logger.warning("registry: %s", warning) + return view.hosts # --------------------------------------------------------------------------- @@ -372,11 +370,11 @@ def find_host(self, instance_type: str, instance_name: str) -> KnownHost | None: `KnownHost` (only the `(type, name)` strings), but opening a terminal needs the full record (`.host`, `.user`, `.access_mode`, ...) to build the SSH command. Rather than duplicate registry-reading logic elsewhere, - this re-reads the read-only registry with the same tolerant parsing as - `_read_known_hosts_readonly()` and returns the first `(type, name)` + this re-reads the read-only registry via + `_read_registry_hosts_readonly()` and returns the first `(type, name)` match, or ``None`` if the instance is no longer registered. """ - for host in _read_known_hosts_readonly(): + for host in _read_registry_hosts_readonly(): if host.type == instance_type and host.name == instance_name: return host return None @@ -408,7 +406,7 @@ async def refresh(self, instance_id: str | None = None, *, force: bool = True) - if not force and instance_id is None and self._is_fresh(): return - hosts = _read_known_hosts_readonly() + hosts = _read_registry_hosts_readonly() if instance_id is not None: hosts = [h for h in hosts if derive_instance_id(h) == instance_id] if not hosts: diff --git a/src/remo_cli/web/state.py b/src/remo_cli/web/state.py index 52f6e67..e21e999 100644 --- a/src/remo_cli/web/state.py +++ b/src/remo_cli/web/state.py @@ -26,7 +26,12 @@ from enum import Enum from pathlib import Path -from remo_cli.core.config import get_known_hosts_path_readonly, get_remo_home_readonly +from remo_cli.core.config import ( + get_known_hosts_path_readonly, + get_registry_path_readonly, + get_remo_home_readonly, +) +from remo_cli.core.registry import RegistryError, read_registry from remo_cli.web.config import WebSettings logger = logging.getLogger("remo_cli.web.state") @@ -69,6 +74,39 @@ def _probe_file(path: Path) -> str: return "ok" +def _probe_registry() -> str: + """Classify registry presence as ``absent`` / ``ok`` / ``unreadable``. + + Per data-model.md §6, the registry can be EITHER `registry.json` OR the + legacy `known_hosts` file — either present satisfies "registry present". + Byte-level readability is checked first (cheap, and matches + `_probe_file`'s EACCES-safety); only once both candidate files are at + least byte-readable (or absent) do we actually parse via + `core.registry.read_registry(readonly=True)`, so a file that exists and + is readable as bytes but is semantically invalid (e.g. `{"version": 99, + ...}`, a newer-format file) is still classified ``unreadable`` here — + the same bucket `detect_state` maps to `BROKEN`. + """ + registry_probe = _probe_file(get_registry_path_readonly()) + legacy_probe = _probe_file(get_known_hosts_path_readonly()) + + if "unreadable" in (registry_probe, legacy_probe): + return "unreadable" + if registry_probe == "absent" and legacy_probe == "absent": + return "absent" + + # Also catches a plain `OSError`: the accessor's own `Path.exists()`/ + # `read_text()` calls raise on EACCES rather than swallowing it, so an + # untraversable directory that slips past the byte-level probes above + # (e.g. a TOCTOU race) must still classify as "unreadable", never crash + # `detect_state`. + try: + read_registry(readonly=True) + except (RegistryError, OSError): + return "unreadable" + return "ok" + + def _home_writable(home: Path) -> bool: """Whether the service can write into (or create) ``REMO_HOME``. @@ -117,7 +155,11 @@ def detect_state(settings: WebSettings | None = None) -> ConfigurationState: Derivation (research R2): - ``broken``: any required artifact present but unreadable, or a - half-pair service keypair (exactly one of the two key files). + half-pair service keypair (exactly one of the two key files). A + registry file that parses at the byte level but is structurally + invalid per `core.registry` (e.g. a `registry.json` written by a + newer, unsupported format version -- `RegistryNewerVersionError`) is + also classified here (015-registry-v2, data-model.md §6, S5). - ``mount_configured``: registry present AND (``REMO_HOME`` not writable OR a user SSH identity resolves). Explicit mounts are the operator's stated intent, so this wins even when a service keypair also exists @@ -128,7 +170,7 @@ def detect_state(settings: WebSettings | None = None) -> ConfigurationState: """ settings = settings or WebSettings() - registry = _probe_file(get_known_hosts_path_readonly()) + registry = _probe_registry() private = _probe_file(settings.service_private_key_path) public = _probe_file(settings.service_public_key_path) diff --git a/tests/conftest.py b/tests/conftest.py index 67853d9..9781047 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ """Shared fixtures for remo tests.""" +import json import os import tempfile @@ -24,3 +25,84 @@ def tmp_config_dir(tmp_path): def mock_subprocess(mocker): """Mock subprocess.run for testing commands that shell out.""" return mocker.patch("subprocess.run") + + +# --------------------------------------------------------------------------- +# Registry v2 fixtures (015-registry-v2) +# --------------------------------------------------------------------------- + + +def legacy_line( + type_: str, + name: str, + host: str, + user: str, + instance_id: str | None = None, + access_mode: str | None = None, + region: str | None = None, +) -> str: + """Build one colon-delimited legacy registry line. + + Field count follows how many of the optional trailing args are passed + (``None`` omits it and everything after it), matching the legacy format's + 4/6/7-field variants: ``TYPE:NAME:HOST:USER[:INSTANCE_ID[:ACCESS_MODE[:REGION]]]``. + """ + parts = [type_, name, host, user] + if instance_id is not None: + parts.append(instance_id) + if access_mode is not None: + parts.append(access_mode) + if region is not None: + parts.append(region) + return ":".join(parts) + + +LEGACY_FIXTURE_LINES: dict[str, str] = { + "incus": legacy_line("incus", "nuc/dev1", "dev1.incus", "remo", "paul", "direct"), + "proxmox": legacy_line( + "proxmox", "pve1/dev2", "10.0.0.42", "remo", "104", "direct", "root" + ), + "aws": legacy_line( + "aws", "buildbox", "203.0.113.7", "remo", "i-0abc123def456", "ssm", "us-east-1" + ), + "hetzner": legacy_line("hetzner", "dev1", "198.51.100.9", "remo"), + "ssh": legacy_line( + "ssh", "nas", "nas.lan", "admin", "2222", "direct", "/home/paul/.ssh/id_nas" + ), + # Legacy access-mode variants no current writer produces but old files can + # contain (research R5) — both must map to access: "direct" (type-first rule). + "incus_implicit_ssm": legacy_line("incus", "old/box", "box.incus", "remo", "paul", "ssm"), + "proxmox_empty_access": legacy_line( + "proxmox", "old/pct", "10.0.0.9", "remo", "101", "", "root" + ), +} + + +def write_legacy_registry(config_dir, lines: list[str]) -> None: + """Write *lines* (plus a trailing newline) to config_dir/known_hosts.""" + (config_dir / "known_hosts").write_text("\n".join(lines) + "\n") + + +def build_v2_host_entry( + type_: str, + name: str, + host: str, + user: str, + access: str = "direct", + **nested_fields: str | int, +) -> dict: + """Build one v2 hostEntry dict matching contracts/registry-file-v2.md. + + ``nested_fields`` (e.g. ``instance_id="i-abc", region="us-east-1"``) are + wrapped under the ``type_``-named nested object when any are given. + """ + entry: dict = {"type": type_, "name": name, "host": host, "user": user, "access": access} + if nested_fields: + entry[type_] = dict(nested_fields) + return entry + + +def write_v2_registry(config_dir, hosts: list[dict], version: int = 2) -> None: + """Write a v2 registry.json document built from a list of hostEntry dicts.""" + doc = {"version": version, "hosts": hosts} + (config_dir / "registry.json").write_text(json.dumps(doc, indent=2) + "\n") diff --git a/tests/integration/test_registry_concurrency.py b/tests/integration/test_registry_concurrency.py new file mode 100644 index 0000000..1076dec --- /dev/null +++ b/tests/integration/test_registry_concurrency.py @@ -0,0 +1,98 @@ +"""Multiprocess stress test for the registry advisory lock (T029, SC-005). + +N worker processes each upsert their own disjoint set of entries into the +shared registry.json concurrently, in a tight loop. If `mutate_registry()`'s +locking (`core/registry.py`, research.md R3) were broken, concurrent +read-modify-write cycles would race and lose updates from other workers. +This test asserts the final registry contains exactly N * M entries (no lost +updates) and that the file is valid, parseable JSON with unique (type, name) +keys throughout -- proving the atomic-write guarantee holds under concurrent +writers, not just single-process correctness. + +Uses `multiprocessing` (not threads) to exercise real separate-process file +locking, since `fcntl.flock` semantics matter across processes, not just +across threads in one interpreter. The worker function is module-level (not +a nested/local function) so it can be pickled for the `spawn` start method as +well as the default `fork` start method used on Linux. +""" + +from __future__ import annotations + +import json +import multiprocessing +import os +from pathlib import Path + +WORKERS = 6 +ITERATIONS = 15 + + +def _worker(remo_home: str, worker_id: int, iterations: int) -> None: + """Runs in a separate process: upsert this worker's own disjoint entries. + + REMO_HOME must be set *before* importing remo_cli, since core.config + resolves it at call time via os.environ -- setting it first in the + worker (rather than relying on fixture-side os.environ mutation + propagating across the process boundary) keeps this correct regardless + of multiprocessing start method (fork vs spawn). + """ + os.environ["REMO_HOME"] = remo_home + + from remo_cli.core.known_hosts import get_known_hosts, save_known_host + from remo_cli.models.host import KnownHost + + for i in range(iterations): + save_known_host( + KnownHost( + type="ssh", + name=f"worker{worker_id}-{i}", + host="1.2.3.4", + user="remo", + access_mode="direct", + ) + ) + # Nice-to-have: a concurrent reader must never observe a torn file. + # get_known_hosts() only succeeds if registry.json parsed as valid + # JSON, so simply not raising here is the assertion. + get_known_hosts() + + +def test_concurrent_upserts_never_lose_entries(tmp_path, monkeypatch): + """N processes x M iterations upserting disjoint entries never lose + entries to a lost-update race, and the file never ends up corrupt.""" + remo_home = str(tmp_path / "remo") + Path(remo_home).mkdir(parents=True, exist_ok=True) + + ctx = multiprocessing.get_context() + processes = [ + ctx.Process(target=_worker, args=(remo_home, worker_id, ITERATIONS)) + for worker_id in range(WORKERS) + ] + + for p in processes: + p.start() + for p in processes: + p.join(timeout=60) + + for p in processes: + assert not p.is_alive(), "worker process did not finish within the timeout" + assert p.exitcode == 0, f"worker process exited with code {p.exitcode}" + + # Read the final state back in THIS process (fresh REMO_HOME resolution). + monkeypatch.setenv("REMO_HOME", remo_home) + from remo_cli.core.known_hosts import get_known_hosts + + hosts = get_known_hosts() + assert len(hosts) == WORKERS * ITERATIONS + + registry_path = Path(remo_home) / "registry.json" + doc = json.loads(registry_path.read_text()) + entries = doc["hosts"] + assert len(entries) == WORKERS * ITERATIONS + + keys = [(e["type"], e["name"]) for e in entries] + assert len(keys) == len(set(keys)), "duplicate (type, name) entries after concurrent writes" + + expected_names = {f"worker{w}-{i}" for w in range(WORKERS) for i in range(ITERATIONS)} + actual_names = {e["name"] for e in entries} + assert actual_names == expected_names diff --git a/tests/integration/test_setup_payload_versions.py b/tests/integration/test_setup_payload_versions.py new file mode 100644 index 0000000..35b4bca --- /dev/null +++ b/tests/integration/test_setup_payload_versions.py @@ -0,0 +1,269 @@ +"""Payload version compatibility matrix (015-registry-v2, T034). + +Asserts the full compatibility matrix from +specs/015-registry-v2/contracts/mirror-payload-v2.md §4 against a live-in- +process FastAPI app (`TestClient`, no real subprocess needed since this is a +wire-contract test, not an SSH/keyscan end-to-end test — see +tests/integration/test_web_adopt_e2e.py for the live-subprocess flow): + +* v1 payload -> accepted, stored as registry.json v2, legacy mirror removed. +* v2 payload -> accepted; wire entries match the file schema exactly. +* v3 payload -> 400 unsupported_payload_version; prior mirror intact & served. +* missing ``payload_versions`` on status -> the workstation (`core.web_adopt`) + aborts BEFORE any keyscan/authorize/PUT (FR-021). +* stale/missing push-cache ``cache_version`` -> treated as empty (one-time + full re-verification push), idempotent on an immediate re-push. +""" + +from __future__ import annotations + +import json +import time +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +from remo_cli.core import web_adopt +from remo_cli.models.host import KnownHost +from remo_cli.web import app as app_module +from remo_cli.web.config import WebSettings +from remo_cli.web.pairing import PairingSession + +pytest.importorskip("fastapi") +pytest.importorskip("uvicorn") + +_ORIGIN = "http://testserver" +_TOKEN = "unit-test-setup-token" +_AUTH = {"Authorization": f"Bearer {_TOKEN}", "Origin": _ORIGIN} + +_VALID_KEY_LINE = "10.0.0.5 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFakeFixtureKeyMaterial0000" + + +class _NoopDiscovery: + async def refresh(self, instance_id: str | None = None, *, force: bool = True) -> None: + return None + + +def _inject_session(application, code: str = _TOKEN) -> None: + application.state.pairing_manager._session = PairingSession( + code=code, identity=None, origin="adopt", last_activity=time.monotonic(), ttl_s=1e9 + ) + + +def _client(tmp_path) -> TestClient: + settings = WebSettings( + allowed_hosts=["testserver", "localhost", "127.0.0.1"], + allowed_origins=[_ORIGIN], + operator_auth="none", + ssh_control_dir=str(tmp_path / "ssh-ctrl"), + ) + application = app_module.create_app(settings) + application.state.discovery_service = _NoopDiscovery() + _inject_session(application) + return TestClient(application, base_url=_ORIGIN) + + +def _v1_payload(**overrides: Any) -> dict[str, Any]: + payload: dict[str, Any] = { + "version": 1, + "registry": [ + {"type": "incus", "name": "dev", "host": "10.0.0.5", "user": "remo"}, + ], + "host_keys": {"dev": [_VALID_KEY_LINE]}, + } + payload.update(overrides) + return payload + + +def _v2_payload(**overrides: Any) -> dict[str, Any]: + payload: dict[str, Any] = { + "version": 2, + "registry": [ + { + "type": "incus", + "name": "dev", + "host": "10.0.0.5", + "user": "remo", + "access": "direct", + }, + ], + "host_keys": {"dev": [_VALID_KEY_LINE]}, + } + payload.update(overrides) + return payload + + +# --------------------------------------------------------------------------- +# Service-side: v1/v2/v3 compatibility matrix +# --------------------------------------------------------------------------- + + +def test_v1_payload_accepted_stored_as_v2_and_legacy_mirror_removed(tmp_path, monkeypatch): + monkeypatch.setenv("REMO_HOME", str(tmp_path / "remo")) + remo_home = tmp_path / "remo" + remo_home.mkdir() + # A stray legacy mirror left by a pre-upgrade push (service-owned + # replaceable state) must be removed once the service applies a payload. + stale_legacy = remo_home / "known_hosts" + stale_legacy.write_text("incus:stale:1.2.3.4:remo\n") + + with _client(tmp_path) as client: + resp = client.put("/api/v1/setup/registry", json=_v1_payload(), headers=_AUTH) + assert resp.status_code == 200 + assert resp.json()["applied"] is True + + registry_json = remo_home / "registry.json" + doc = json.loads(registry_json.read_text()) + assert doc["version"] == 2 + assert doc["hosts"] == [ + {"type": "incus", "name": "dev", "host": "10.0.0.5", "user": "remo", "access": "direct"} + ] + assert not stale_legacy.exists() + + +def test_v2_payload_accepted_wire_entries_match_file_schema(tmp_path, monkeypatch): + monkeypatch.setenv("REMO_HOME", str(tmp_path / "remo")) + with _client(tmp_path) as client: + resp = client.put("/api/v1/setup/registry", json=_v2_payload(), headers=_AUTH) + assert resp.status_code == 200 + assert resp.json() == {"applied": True, "registry_instances": 1, "host_key_instances": 1} + + doc = json.loads((tmp_path / "remo" / "registry.json").read_text()) + assert doc["hosts"] == _v2_payload()["registry"] + + +def test_v3_payload_rejected_mirror_intact_and_served(tmp_path, monkeypatch): + monkeypatch.setenv("REMO_HOME", str(tmp_path / "remo")) + with _client(tmp_path) as client: + # Establish a baseline mirror first. + first = client.put("/api/v1/setup/registry", json=_v1_payload(), headers=_AUTH) + assert first.status_code == 200 + baseline = (tmp_path / "remo" / "registry.json").read_text() + + resp = client.put( + "/api/v1/setup/registry", json=_v1_payload(version=3), headers=_AUTH + ) + assert resp.status_code == 400 + assert resp.json() == { + "error": { + "code": "unsupported_payload_version", + "supported": [1, 2], + "received": 3, + } + } + # Prior mirror unchanged and still served over GET /status. + assert (tmp_path / "remo" / "registry.json").read_text() == baseline + status = client.get("/api/v1/setup/status", headers=_AUTH).json() + assert status["registry_instances"] == 1 + assert status["payload_versions"] == [1, 2] + + +# --------------------------------------------------------------------------- +# Workstation-side: fail-fast on version skew (FR-021) +# --------------------------------------------------------------------------- + + +def _ssm_free_host() -> KnownHost: + return KnownHost(type="incus", name="dev", host="10.0.0.5", user="remo") + + +@pytest.fixture +def api_client(mocker): + client = mocker.MagicMock() + client.get_status.return_value = {"state": "adopted", "registry_instances": 1} + mocker.patch("remo_cli.core.web_adopt.SetupApiClient", return_value=client) + return client + + +def test_push_aborts_before_any_mutation_when_service_omits_payload_versions( + tmp_config_dir, api_client, mocker +): + """An old (pre-015) service's status has no `payload_versions` field at + all -- implies [1] -- so the workstation must abort before any keyscan, + authorize, or PUT (fail truly fast, no partial mutation of any kind).""" + mocker.patch( + "remo_cli.core.web_adopt.get_known_hosts", return_value=[_ssm_free_host()] + ) + scan = mocker.patch("remo_cli.core.web_adopt.scan_and_verify_host_key") + authorize = mocker.patch("remo_cli.core.web_adopt.authorize_service_key") + + with pytest.raises(web_adopt.UnsupportedPayloadVersionError, match="upgrade the remo-web"): + web_adopt.run_push("http://web.example:8080", "code", interactive=False) + + scan.assert_not_called() + authorize.assert_not_called() + api_client.get_identity.assert_not_called() + api_client.put_registry.assert_not_called() + + +def test_push_proceeds_when_service_advertises_v2(tmp_config_dir, api_client, mocker): + api_client.get_status.return_value = { + "state": "adopted", + "registry_instances": 1, + "payload_versions": [1, 2], + } + mocker.patch( + "remo_cli.core.web_adopt.get_known_hosts", return_value=[_ssm_free_host()] + ) + api_client.get_identity.return_value = { + "deployment_id": "dep-1", + "public_key": "ssh-ed25519 AAAAfixture remo-web@dep-1", + } + mocker.patch( + "remo_cli.core.web_adopt.scan_and_verify_host_key", + return_value=web_adopt.HostKeyScan("trusted", lines=[_VALID_KEY_LINE]), + ) + mocker.patch( + "remo_cli.core.web_adopt.authorize_service_key", return_value=(True, "") + ) + api_client.put_registry.return_value = {"registry_instances": 1, "host_key_instances": 1} + api_client.post_verify.return_value = {"all_passed": True, "results": []} + + result = web_adopt.run_push("http://web.example:8080", "code", interactive=False) + + api_client.put_registry.assert_called_once() + payload = api_client.put_registry.call_args.args[0] + assert payload["version"] == 2 + assert result.outcomes[0].outcome == web_adopt.OUTCOME_ADOPTED + + +# --------------------------------------------------------------------------- +# Push delta cache: stale/missing cache_version treated as empty (research R10) +# --------------------------------------------------------------------------- + + +def test_stale_cache_version_forces_full_reverify_then_idempotent(tmp_config_dir): + cache_path = web_adopt.push_cache_path() + # Pre-015 shape: no "cache_version" key at all. + cache_path.write_text( + json.dumps( + { + "push_cache": { + "dep-1": { + "dev": {"fingerprint": "f" * 64, "host_keys": [_VALID_KEY_LINE]} + } + } + } + ) + ) + + loaded = web_adopt.load_push_cache() + assert loaded == {} # discarded wholesale -> first push re-verifies everything + + # Saving now stamps cache_version: 2; a second load is idempotent. + web_adopt.save_push_cache( + { + "dep-1": { + "dev": web_adopt.CachedInstance( + fingerprint=web_adopt.instance_fingerprint(_ssm_free_host()), + host_keys=[_VALID_KEY_LINE], + ) + } + } + ) + reloaded = json.loads(cache_path.read_text()) + assert reloaded["cache_version"] == 2 + assert web_adopt.load_push_cache()["dep-1"]["dev"].fingerprint == web_adopt.instance_fingerprint( + _ssm_free_host() + ) diff --git a/tests/integration/test_web_adopt_e2e.py b/tests/integration/test_web_adopt_e2e.py index e77e8cb..6773ef8 100644 --- a/tests/integration/test_web_adopt_e2e.py +++ b/tests/integration/test_web_adopt_e2e.py @@ -51,7 +51,9 @@ import pytest +from remo_cli.core import registry as core_registry from remo_cli.core import web_adopt +from remo_cli.core.known_hosts import save_known_host from remo_cli.models.host import KnownHost # --------------------------------------------------------------------------- @@ -96,10 +98,17 @@ def _web_extra_available() -> bool: ) _HOSTS = [_DIRECT, _SSM, _UNREACHABLE] -#: What the service-side registry mirror must contain, byte-for-byte -#: (colon-delimited `KnownHost.to_line()` per entry, payload order). +#: Legacy colon-delimited text -- used ONLY to seed the WORKSTATION-side +#: (CLI-side) registry fixture; still valid as migration input (015-registry-v2). _EXPECTED_SERVICE_REGISTRY = "".join(h.to_line() + "\n" for h in _HOSTS) +#: What the SERVICE-side registry.json (v2) must contain: hostEntry dicts +#: sorted by (type, name), matching core/registry.py's serializer (015-registry-v2). +_EXPECTED_SERVICE_HOSTS = [ + core_registry.known_host_to_entry(h) + for h in sorted(_HOSTS, key=lambda h: (h.type, h.name)) +] + #: Canned "scanned & workstation-trusted" host keys for the direct instance. #: Structurally valid known_hosts lines (plausible key type + base64) so the #: service's PUT-side payload validation accepts them unmodified. @@ -156,7 +165,8 @@ def identity_dir(self) -> Path: @property def registry_path(self) -> Path: - return self.remo_home / "known_hosts" + """The service's registry.json (v2) — 015-registry-v2 write target.""" + return self.remo_home / "registry.json" def mint(self) -> str: """Mint a fresh pairing code over HTTP (network-restricted posture).""" @@ -363,6 +373,7 @@ def test_fresh_boot_is_unconfigured_with_generated_identity(service: LiveService "deployment_id": state["deployment_id"], "public_key_available": True, "registry_instances": 0, + "payload_versions": [1, 2], } assert not service.registry_path.exists() @@ -422,9 +433,11 @@ def test_full_adopt_then_idempotent_rerun( assert config_lines and config_lines[0]["passed"] assert "adopted" in config_lines[0]["detail"] - # Service-side end state: full colon-delimited registry mirror (all 3 - # entries) + service known_hosts containing exactly the canned lines. - assert service.registry_path.read_text() == _EXPECTED_SERVICE_REGISTRY + # Service-side end state: full v2 registry mirror (all 3 entries) + + # service known_hosts containing exactly the canned lines. + doc = json.loads(service.registry_path.read_text()) + assert doc["version"] == 2 + assert doc["hosts"] == _EXPECTED_SERVICE_HOSTS service_known_hosts = service.identity_dir / "known_hosts" assert service_known_hosts.read_text() == _EXPECTED_SERVICE_KNOWN_HOSTS @@ -526,7 +539,8 @@ def test_registry_put_preserves_established_sessions( # The path now serves the NEW content... new_bytes = service.registry_path.read_bytes() assert new_bytes != original_bytes - assert b"incus:webbox:192.0.2.10:dev" in new_bytes + assert b'"host": "192.0.2.10"' in new_bytes + assert b'"user": "dev"' in new_bytes # ...while the held fd still reads the ORIGINAL content: the PUT # swapped the directory entry (os.replace), never the old inode. assert _read_fd_fully(held_fd) == original_bytes @@ -572,7 +586,8 @@ def test_push_after_adopt_processes_only_the_new_instance( assert cache_path.stat().st_mode & 0o777 == 0o600 saved = json.loads(cache_path.read_text()) # No secret is persisted (FR-019): no url, no token/code, no top-level id. - assert set(saved) == {"push_cache"} + assert set(saved) == {"cache_version", "push_cache"} + assert saved["cache_version"] == 2 dep = result.deployment_id # Delta cache is deployment-keyed and seeded with the adopted instance. assert set(saved["push_cache"]) == {dep} @@ -583,8 +598,11 @@ def test_push_after_adopt_processes_only_the_new_instance( adoption_ssh_mocks["authorized"].clear() # ---- A NEW direct-access instance is registered workstation-side ---- - registry = workstation / "known_hosts" - registry.write_text(registry.read_text() + _NEW_DIRECT.to_line() + "\n") + # (the workstation's legacy known_hosts was already migrated to + # registry.json by the adopt call above, so this goes through the public + # registry API rather than appending a colon line to a file that no + # longer exists.) + save_known_host(_NEW_DIRECT) # ---- Push with a freshly minted code (FR-018/FR-019) ----------------- push = web_adopt.run_push(service.url, service.mint(), interactive=False) @@ -616,17 +634,23 @@ def test_push_after_adopt_processes_only_the_new_instance( # Service-side end state: the mirror gained exactly the new entry, and # the service known_hosts holds the cached webbox lines (reused from the - # delta cache) plus the new instance's freshly scanned lines, in - # registry order. - assert ( - service.registry_path.read_text() - == _EXPECTED_SERVICE_REGISTRY + _NEW_DIRECT.to_line() + "\n" - ) + # delta cache) plus the new instance's freshly scanned lines. + expected_hosts_after_push = [ + core_registry.known_host_to_entry(h) + for h in sorted([*_HOSTS, _NEW_DIRECT], key=lambda h: (h.type, h.name)) + ] + doc = json.loads(service.registry_path.read_text()) + assert doc["hosts"] == expected_hosts_after_push + # Registry v2 always writes entries sorted by (type, name); the wire + # payload built from get_known_hosts() follows the same order, so the + # service known_hosts lines land "incus/newbox" (n < w) before + # "incus/webbox" -- a deterministic ordering change from the old + # append-order flat file, not a bug. assert ( service.identity_dir / "known_hosts" - ).read_text() == _EXPECTED_SERVICE_KNOWN_HOSTS + "".join( + ).read_text() == "".join( line + "\n" for line in _NEW_CANNED_HOST_KEY_LINES - ) + ) + _EXPECTED_SERVICE_KNOWN_HOSTS status, setup = service.setup_status() assert status == 200 diff --git a/tests/perf/test_registry_perf.py b/tests/perf/test_registry_perf.py new file mode 100644 index 0000000..17334ae --- /dev/null +++ b/tests/perf/test_registry_perf.py @@ -0,0 +1,86 @@ +"""Performance regression test for the registry v2 accessor (015-registry-v2, T035). + +SC-008 (spec.md, quickstart.md §8): registry read+validate+write overhead +stays under 100ms per command invocation at 200 entries. Catches an +accidentally-quadratic validation or serialization path before it ships. +""" + +from __future__ import annotations + +import time + +from remo_cli.core.registry import mutate_registry, read_registry, validate_hosts +from remo_cli.models.host import KnownHost + +_TYPES = ("incus", "proxmox", "aws", "hetzner", "ssh") + + +def _generate_hosts(count: int) -> list[KnownHost]: + hosts: list[KnownHost] = [] + for i in range(count): + type_ = _TYPES[i % len(_TYPES)] + if type_ == "incus": + hosts.append( + KnownHost( + type=type_, name=f"host{i}/dev{i}", host=f"dev{i}.incus", + user="remo", instance_id="paul", access_mode="direct", + ) + ) + elif type_ == "proxmox": + hosts.append( + KnownHost( + type=type_, name=f"pve{i}/dev{i}", host=f"10.0.{i // 256}.{i % 256}", + user="remo", instance_id=str(100 + i), access_mode="direct", + region="root", + ) + ) + elif type_ == "aws": + hosts.append( + KnownHost( + type=type_, name=f"cloud{i}", host=f"203.0.113.{i % 256}", + user="remo", instance_id=f"i-{i:017x}", access_mode="ssm", + region="us-east-1", + ) + ) + elif type_ == "hetzner": + hosts.append( + KnownHost( + type=type_, name=f"web{i}", host=f"198.51.100.{i % 256}", + user="remo", access_mode="direct", + ) + ) + else: # ssh + hosts.append( + KnownHost( + type=type_, name=f"box{i}", host=f"box{i}.lan", user="admin", + instance_id=str(2200 + (i % 100)), access_mode="direct", + region=f"/home/paul/.ssh/id_{i}", + ) + ) + return hosts + + +def test_200_entry_round_trip_under_100ms(tmp_config_dir): + hosts = _generate_hosts(200) + + start = time.perf_counter() + validate_hosts(hosts) + mutate_registry(lambda _current: hosts) + view = read_registry() + elapsed_ms = (time.perf_counter() - start) * 1000 + + assert len(view.hosts) == 200 + assert elapsed_ms < 100, f"200-entry registry round-trip took {elapsed_ms:.1f}ms (budget: 100ms)" + + +def test_200_entry_read_only_is_well_under_budget(tmp_config_dir): + hosts = _generate_hosts(200) + mutate_registry(lambda _current: hosts) + + start = time.perf_counter() + for _ in range(10): + view = read_registry() + elapsed_ms = ((time.perf_counter() - start) / 10) * 1000 + + assert len(view.hosts) == 200 + assert elapsed_ms < 100, f"200-entry registry read averaged {elapsed_ms:.1f}ms (budget: 100ms)" diff --git a/tests/unit/core/test_known_hosts.py b/tests/unit/core/test_known_hosts.py index d9495d3..81216f3 100644 --- a/tests/unit/core/test_known_hosts.py +++ b/tests/unit/core/test_known_hosts.py @@ -1,9 +1,8 @@ """Tests for remo.core.known_hosts registry module.""" -import os - import pytest +from remo_cli.core.config import get_registry_backup_path from remo_cli.core.known_hosts import ( clear_known_hosts_by_prefix, clear_known_hosts_by_type, @@ -22,17 +21,27 @@ def _read_registry(config_dir) -> str: - """Read the raw known_hosts file content.""" + """Read the raw known_hosts file content (legacy-format fixture input).""" return (config_dir / "known_hosts").read_text() def _write_registry(config_dir, content: str) -> None: - """Write raw content to the known_hosts file.""" + """Write raw legacy-format content to the known_hosts file. + + Used only to seed a legacy fixture as MIGRATION INPUT; ``save_known_host`` + et al. write ``registry.json`` (v2), not this file (FR-015 delegate). + """ (config_dir / "known_hosts").write_text(content) def _make_host(type_="incus", name="myhost/dev", host="10.0.0.1", user="remo", **kwargs): - """Convenience factory for KnownHost instances.""" + """Convenience factory for KnownHost instances. + + Defaults ``access_mode`` to ``"direct"`` since registry v2 validation + (V6) requires it to be ``"direct"`` or ``"ssm"`` — the legacy implicit- + empty convention no longer round-trips (data-model.md §3). + """ + kwargs.setdefault("access_mode", "direct") return KnownHost(type=type_, name=name, host=host, user=user, **kwargs) @@ -45,11 +54,11 @@ class TestSaveKnownHost: """Adding and replacing entries in the registry.""" def test_creates_file_if_missing(self, tmp_config_dir): - """Registry file is created if it does not already exist.""" - kh_path = tmp_config_dir / "known_hosts" - assert not kh_path.exists() + """Registry file (registry.json, v2) is created if it does not already exist.""" + registry_path = tmp_config_dir / "registry.json" + assert not registry_path.exists() save_known_host(_make_host()) - assert kh_path.exists() + assert registry_path.exists() hosts = get_known_hosts() assert len(hosts) == 1 assert hosts[0].name == "myhost/dev" @@ -86,23 +95,26 @@ def test_preserves_other_entries(self, tmp_config_dir): assert hetzner_hosts[0].host == "5.6.7.8" def test_preserves_unparseable_lines(self, tmp_config_dir): - """Lines that cannot be parsed are preserved in the file.""" + """Unparseable legacy lines are never dropped: they survive verbatim in + the renamed migration backup (known_hosts.v1.bak), not the new + registry.json (data-model.md §4 rule 4 / §6 S1->S2).""" _write_registry(tmp_config_dir, "# comment line\nbadline\nincus:h/c:10.0.0.1:remo\n") save_known_host(_make_host(type_="hetzner", name="web1", host="5.5.5.5")) - raw = _read_registry(tmp_config_dir) + raw = get_registry_backup_path().read_text() assert "# comment line" in raw assert "badline" in raw hosts = get_known_hosts() assert len(hosts) == 2 def test_preserves_empty_lines(self, tmp_config_dir): - """Empty lines in the file are preserved.""" + """Empty lines in the legacy file survive verbatim in the migration + backup (the rename is a byte-for-byte copy of the original file).""" _write_registry(tmp_config_dir, "incus:h/c:10.0.0.1:remo\n\n\n") save_known_host(_make_host(type_="hetzner", name="web1", host="5.5.5.5")) - raw = _read_registry(tmp_config_dir) - # There should still be empty lines present in the output. + raw = get_registry_backup_path().read_text() + # There should still be empty lines present in the backup. lines = raw.split("\n") - empty_lines = [l for l in lines if l.strip() == ""] + empty_lines = [line for line in lines if line.strip() == ""] assert len(empty_lines) >= 2 diff --git a/tests/unit/core/test_registry_format.py b/tests/unit/core/test_registry_format.py new file mode 100644 index 0000000..755ee81 --- /dev/null +++ b/tests/unit/core/test_registry_format.py @@ -0,0 +1,415 @@ +"""Foundational tests for the v2 registry file format (T010) plus value- +fidelity tests for IPv6/colon-containing values (T019, US2). + +Covers: round-trip fidelity, deterministic serialization, unknown-type +preservation, newer-version rejection, tolerant-read warnings, and +validation rules V2-V6 (data-model.md §5). +""" + +from __future__ import annotations + +import json + +import pytest + +from remo_cli.core.config import get_registry_path +from remo_cli.core.registry import ( + RegistryNewerVersionError, + RegistryValidationError, + mutate_registry, + read_registry, + replace_registry, + validate_hosts, +) +from remo_cli.models.host import KnownHost +from tests.conftest import build_v2_host_entry, write_v2_registry + + +def _all_type_hosts() -> list[KnownHost]: + """One host per known type, exercising every nested field shape.""" + return [ + KnownHost( + type="incus", + name="nuc/dev1", + host="dev1.incus", + user="remo", + instance_id="paul", + access_mode="direct", + ), + KnownHost( + type="proxmox", + name="pve1/dev2", + host="10.0.0.42", + user="remo", + instance_id="104", + access_mode="direct", + region="root", + ), + KnownHost( + type="aws", + name="buildbox", + host="203.0.113.7", + user="remo", + instance_id="i-0abc123def456", + access_mode="ssm", + region="us-east-1", + ), + KnownHost( + type="hetzner", + name="dev1", + host="198.51.100.9", + user="remo", + access_mode="direct", + ), + KnownHost( + type="ssh", + name="nas", + host="nas.lan", + user="admin", + instance_id="2222", + access_mode="direct", + region="/home/paul/.ssh/id_nas", + ), + ] + + +class TestV2RoundTripFidelity: + """Every KnownHost field survives a replace_registry -> read_registry cycle.""" + + def test_all_types_round_trip_exactly(self, tmp_config_dir): + hosts = _all_type_hosts() + replace_registry(hosts, allow_empty=True) + view = read_registry() + assert view.source_format == "v2" + assert len(view.hosts) == len(hosts) + by_key = {(h.type, h.name): h for h in view.hosts} + for original in hosts: + assert by_key[(original.type, original.name)] == original + + +class TestDeterministicSerialization: + """Re-serializing the same host list twice must produce byte-identical output.""" + + def test_reserialize_produces_identical_bytes(self, tmp_config_dir): + hosts = _all_type_hosts() + replace_registry(hosts, allow_empty=True) + text1 = get_registry_path().read_text() + replace_registry(hosts, allow_empty=True) + text2 = get_registry_path().read_text() + assert text1 == text2 + + def test_output_is_sorted_indented_with_trailing_newline(self, tmp_config_dir): + hosts = _all_type_hosts() + replace_registry(hosts, allow_empty=True) + text = get_registry_path().read_text() + assert text.endswith("\n") + doc = json.loads(text) + pairs = [(e["type"], e["name"]) for e in doc["hosts"]] + assert pairs == sorted(pairs) + + +class TestUnknownTypePreservation: + """Entries whose type is unrecognized round-trip verbatim (FR-014).""" + + def test_unknown_entry_counted_and_known_entry_still_parses(self, tmp_config_dir): + entries = [ + build_v2_host_entry("docker", "mybox", "1.2.3.4", "remo"), + build_v2_host_entry("incus", "nuc/dev1", "dev1.incus", "remo", host_user="paul"), + ] + write_v2_registry(tmp_config_dir, entries) + + view = read_registry() + + assert view.unknown_entries == 1 + assert len(view.hosts) == 1 + assert view.hosts[0].type == "incus" + assert view.hosts[0].name == "nuc/dev1" + + def test_unknown_entry_survives_a_write_it_never_saw(self, tmp_config_dir): + entries = [ + build_v2_host_entry("docker", "mybox", "1.2.3.4", "remo"), + build_v2_host_entry("incus", "nuc/dev1", "dev1.incus", "remo", host_user="paul"), + ] + write_v2_registry(tmp_config_dir, entries) + + mutate_registry(lambda hosts: hosts) # identity mutator + + raw = get_registry_path().read_text() + doc = json.loads(raw) + docker_entries = [e for e in doc["hosts"] if e["type"] == "docker"] + assert len(docker_entries) == 1 + assert docker_entries[0]["name"] == "mybox" + assert docker_entries[0]["host"] == "1.2.3.4" + assert docker_entries[0]["user"] == "remo" + + +class TestNewerVersionRejection: + """A registry.json written by a newer format version is rejected untouched.""" + + def test_rejects_and_leaves_file_untouched(self, tmp_config_dir): + doc = {"version": 3, "hosts": []} + registry_file = tmp_config_dir / "registry.json" + registry_file.write_text(json.dumps(doc, indent=2) + "\n") + before = registry_file.read_text() + before_mtime = registry_file.stat().st_mtime + + with pytest.raises(RegistryNewerVersionError): + read_registry() + + after = registry_file.read_text() + assert before == after + assert registry_file.stat().st_mtime == before_mtime + + +class TestTolerantReadWarnings: + """Per-entry problems on read are warnings, never exceptions (FR-014).""" + + def test_missing_required_field_warns_but_valid_entry_survives(self, tmp_config_dir): + entries = [ + { + "type": "incus", + "name": "bad/entry", + "host": "1.2.3.4", + # "user" missing + "access": "direct", + }, + build_v2_host_entry("hetzner", "web1", "5.6.7.8", "remo"), + ] + write_v2_registry(tmp_config_dir, entries) + + view = read_registry() + + assert view.warnings + names = {h.name for h in view.hosts} + assert "web1" in names + assert "bad/entry" not in names + + +class TestValidationRuleRejections: + """Validation rules V2-V6 (data-model.md §5).""" + + def test_v2_empty_required_field_rejected(self): + with pytest.raises(RegistryValidationError): + validate_hosts( + [KnownHost(type="incus", name="", host="1.2.3.4", user="remo")] + ) + + def test_v2_control_character_rejected(self): + with pytest.raises(RegistryValidationError): + validate_hosts( + [KnownHost(type="incus", name="bad\nname", host="1.2.3.4", user="remo")] + ) + + def test_v2_newline_in_host_rejected(self): + with pytest.raises(RegistryValidationError): + validate_hosts( + [KnownHost(type="incus", name="ok", host="1.2.3.4\n", user="remo")] + ) + + def test_v3_duplicate_type_and_name_rejected(self): + hosts = [ + KnownHost(type="incus", name="dup", host="1.1.1.1", user="remo"), + KnownHost(type="incus", name="dup", host="2.2.2.2", user="remo"), + ] + with pytest.raises(RegistryValidationError): + validate_hosts(hosts) + + def test_v3_same_name_different_type_is_allowed(self): + hosts = [ + KnownHost( + type="incus", name="dup", host="1.1.1.1", user="remo", access_mode="direct" + ), + KnownHost( + type="hetzner", name="dup", host="2.2.2.2", user="remo", access_mode="direct" + ), + ] + validate_hosts(hosts) # must not raise + + def test_v6_ssm_only_valid_for_aws(self): + with pytest.raises(RegistryValidationError): + validate_hosts( + [ + KnownHost( + type="incus", + name="x", + host="1.1.1.1", + user="remo", + access_mode="ssm", + ) + ] + ) + + def test_v6_ssm_valid_for_aws(self): + validate_hosts( + [ + KnownHost( + type="aws", + name="x", + host="1.1.1.1", + user="remo", + instance_id="i-abc", + access_mode="ssm", + ) + ] + ) # must not raise + + @pytest.mark.parametrize("bad_port", ["0", "65536", "not-a-number"]) + def test_v5_ssh_port_out_of_range_or_non_numeric_rejected(self, bad_port): + with pytest.raises(RegistryValidationError): + validate_hosts( + [ + KnownHost( + type="ssh", + name="x", + host="1.1.1.1", + user="remo", + instance_id=bad_port, + access_mode="direct", + ) + ] + ) + + @pytest.mark.parametrize("good_port", ["1", "65535", "22"]) + def test_v5_ssh_port_in_range_accepted(self, good_port): + validate_hosts( + [ + KnownHost( + type="ssh", + name="x", + host="1.1.1.1", + user="remo", + instance_id=good_port, + access_mode="direct", + ) + ] + ) # must not raise + + def test_replace_registry_rejects_before_write_disk_untouched(self, tmp_config_dir): + registry_file = tmp_config_dir / "registry.json" + assert not registry_file.exists() + + with pytest.raises(RegistryValidationError): + replace_registry( + [KnownHost(type="incus", name="", host="1.1.1.1", user="remo")], + allow_empty=True, + ) + + assert not registry_file.exists() + + def test_mutate_registry_rejects_before_write_disk_unchanged(self, tmp_config_dir): + good = [ + KnownHost( + type="hetzner", name="web1", host="5.5.5.5", user="remo", access_mode="direct" + ) + ] + replace_registry(good, allow_empty=True) + before = get_registry_path().read_text() + + def bad_mutator(hosts: list[KnownHost]) -> list[KnownHost]: + return [*hosts, KnownHost(type="incus", name="", host="1.1.1.1", user="remo")] + + with pytest.raises(RegistryValidationError): + mutate_registry(bad_mutator) + + after = get_registry_path().read_text() + assert before == after + + +class TestIPv6AndColonValueFidelity: + """T019 (US2): the whole point of the new format — legitimate values that + corrupted the legacy colon-delimited format round-trip byte-identically.""" + + def test_ipv6_host_round_trips(self, tmp_config_dir): + host = KnownHost( + type="hetzner", name="v6box", host="2001:db8::7", user="remo", access_mode="direct" + ) + replace_registry([host], allow_empty=True) + + view = read_registry() + assert view.hosts[0].host == "2001:db8::7" + + raw1 = get_registry_path().read_text() + replace_registry([host], allow_empty=True) + raw2 = get_registry_path().read_text() + assert raw1 == raw2 + + def test_colon_containing_identity_file_round_trips(self, tmp_config_dir): + host = KnownHost( + type="ssh", + name="nas", + host="nas.lan", + user="admin", + instance_id="2222", + access_mode="direct", + region="/home/paul/some:weird/path", + ) + replace_registry([host], allow_empty=True) + + got = read_registry().hosts[0] + assert got.region == "/home/paul/some:weird/path" + + doc = json.loads(get_registry_path().read_text()) + entry = doc["hosts"][0] + assert entry["ssh"]["identity_file"] == "/home/paul/some:weird/path" + + def test_colon_containing_aws_region_round_trips(self, tmp_config_dir): + host = KnownHost( + type="aws", + name="buildbox", + host="1.2.3.4", + user="remo", + instance_id="i-abc", + access_mode="ssm", + region="us-east-1:special", + ) + replace_registry([host], allow_empty=True) + + got = read_registry().hosts[0] + assert got.region == "us-east-1:special" + + def test_spaces_and_special_characters_round_trip(self, tmp_config_dir): + host = KnownHost( + type="ssh", + name="odd", + host="my host (lab)", + user="admin user", + instance_id="2222", + access_mode="direct", + region="/path with spaces/id_rsa", + ) + replace_registry([host], allow_empty=True) + + got = read_registry().hosts[0] + assert got.host == "my host (lab)" + assert got.user == "admin user" + assert got.region == "/path with spaces/id_rsa" + + def test_boundary_length_name_round_trips(self, tmp_config_dir): + name = "a" * 63 + host = KnownHost( + type="hetzner", name=name, host="1.2.3.4", user="remo", access_mode="direct" + ) + replace_registry([host], allow_empty=True) + + got = read_registry().hosts[0] + assert got.name == name + assert len(got.name) == 63 + + +class TestEmptyAccessModeAccepted: + """Several providers (hetzner.py's create()/sync()) construct a fresh + KnownHost with access_mode left at its dataclass default (""), then pass it + straight to save_known_host() -> mutate_registry() -> validate_hosts(). + An empty access_mode has always meant "direct" (matches + known_host_to_entry()'s own `host.access_mode or "direct"` normalization), + so validate_hosts() must accept it rather than requiring every call site to + set access_mode explicitly.""" + + def test_replace_registry_accepts_the_real_hetzner_write_shape(self, tmp_config_dir): + host = KnownHost(type="hetzner", name="dev1", host="198.51.100.9", user="remo") + assert host.access_mode == "" # the exact shape providers/hetzner.py constructs + + replace_registry([host], allow_empty=True) # must not raise + reloaded = read_registry().hosts + assert len(reloaded) == 1 + assert reloaded[0].access_mode == "direct" diff --git a/tests/unit/core/test_registry_locking.py b/tests/unit/core/test_registry_locking.py new file mode 100644 index 0000000..7e8bc14 --- /dev/null +++ b/tests/unit/core/test_registry_locking.py @@ -0,0 +1,141 @@ +"""Tests for the advisory locking mechanism in remo_cli.core.registry (T028). + +Covers research.md R3: timeout -> RegistryBusyError, degraded (unlocked) +proceed with a one-time warning when flock is unavailable on the filesystem, +and atomicity of the underlying write primitive under a simulated +mid-write crash (FR-018 -- readers never see a torn file). +""" + +from __future__ import annotations + +import errno +import fcntl +import os +import time + +import pytest + +from remo_cli.core import registry +from remo_cli.core.known_hosts import save_known_host +from remo_cli.models.host import KnownHost + + +# ----------------------------------------------------------------------- +# Timeout -> RegistryBusyError +# ----------------------------------------------------------------------- + + +def test_lock_timeout_raises_registry_busy_error(tmp_config_dir): + """An externally-held exclusive flock causes registry_lock() to time out.""" + lock_path = registry.get_registry_lock_path() + lock_path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(str(lock_path), os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(fd, fcntl.LOCK_EX) + try: + start = time.monotonic() + with pytest.raises(registry.RegistryBusyError): + with registry.registry_lock(timeout_s=0.3): + pass # pragma: no cover - never reached, lock is held externally + elapsed = time.monotonic() - start + + # Bounded wall-clock wait: close to the explicit short timeout, not the + # production 5s default, and not near-instant either (retry loop ran). + assert 0.25 <= elapsed <= 2.0 + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + + +# ----------------------------------------------------------------------- +# Degraded (unlocked) proceed on flock OSError +# ----------------------------------------------------------------------- + + +def test_flock_oserror_degrades_to_unlocked_proceed_with_one_time_warning( + tmp_config_dir, monkeypatch, capsys +): + """flock raising ENOLCK degrades to an unlocked proceed, warned only once.""" + monkeypatch.setattr(registry, "_lock_degradation_warned", False) + + real_flock = fcntl.flock + + def fake_flock(fd, operation): + if operation & fcntl.LOCK_EX and not (operation & fcntl.LOCK_UN): + raise OSError(errno.ENOLCK, "no locks available") + return real_flock(fd, operation) + + monkeypatch.setattr(fcntl, "flock", fake_flock) + + # First call: does not raise RegistryBusyError, proceeds unlocked, warns. + entered = False + with registry.registry_lock(timeout_s=1.0): + entered = True + assert entered + + first_output = capsys.readouterr().out + assert "registry locking unavailable" in first_output + + # Second call in the same process: warning is one-time, must not repeat. + entered_again = False + with registry.registry_lock(timeout_s=1.0): + entered_again = True + assert entered_again + + second_output = capsys.readouterr().out + assert "registry locking unavailable" not in second_output + + +# ----------------------------------------------------------------------- +# Atomicity: a failed rename must never leave a torn/partial file +# ----------------------------------------------------------------------- + + +def test_atomic_write_failure_leaves_prior_registry_state_intact(tmp_config_dir, monkeypatch): + """A crash between temp-file write and os.replace() must not corrupt + or partially overwrite the previously-committed registry.json, and must + not leave a leftover temp file behind (FR-018).""" + host = KnownHost( + type="ssh", name="known-good", host="10.0.0.5", user="remo", access_mode="direct" + ) + save_known_host(host) + + registry_path = registry.get_registry_path() + original_content = registry_path.read_text() + assert "known-good" in original_content + + def raising_replace(*args, **kwargs): + raise OSError("simulated crash between temp-file write and rename") + + monkeypatch.setattr(os, "replace", raising_replace) + + with pytest.raises(OSError): + registry._atomic_write_text(registry_path, "this must never land on disk") + + # Original file is byte-for-byte untouched. + assert registry_path.read_text() == original_content + + # No leftover temp file survives the failed write. + leftover = list(registry_path.parent.glob(".registry_tmp_*")) + assert leftover == [] + + +def test_atomic_write_failure_when_target_never_existed_leaves_nothing( + tmp_config_dir, monkeypatch +): + """Same crash-mid-write scenario, but for a target path that never + existed: no file should appear at the target, and no temp file should + be left behind.""" + target = registry.get_registry_path().parent / "fresh_never_written.json" + assert not target.exists() + + def raising_replace(*args, **kwargs): + raise OSError("simulated crash between temp-file write and rename") + + monkeypatch.setattr(os, "replace", raising_replace) + + with pytest.raises(OSError): + registry._atomic_write_text(target, "should never land on disk") + + assert not target.exists() + leftover = list(target.parent.glob(".registry_tmp_*")) + assert leftover == [] diff --git a/tests/unit/core/test_registry_migration.py b/tests/unit/core/test_registry_migration.py new file mode 100644 index 0000000..92d93d7 --- /dev/null +++ b/tests/unit/core/test_registry_migration.py @@ -0,0 +1,364 @@ +"""Migration matrix tests (T015): legacy known_hosts -> registry.json v2. + +Covers the full state-transition matrix from data-model.md §6 (S0-S5): all +5 types across their 4/6/7-field legacy variants, garbage/unknown lines, +empty vs missing legacy files, pre-existing backup suffixing, interrupted +(S3) and divergent (S4) both-present resolution, idempotent completed +migration, and the two legacy access-mode variants from research R5. +""" + +from __future__ import annotations + +import json + +from remo_cli.core.config import get_registry_backup_path, get_registry_path +from remo_cli.core.registry import migrate_if_needed, read_registry +from tests.conftest import ( + LEGACY_FIXTURE_LINES, + build_v2_host_entry, + legacy_line, + write_legacy_registry, + write_v2_registry, +) + + +def _registry_doc(config_dir) -> dict: + return json.loads((config_dir / "registry.json").read_text()) + + +def _entry(doc: dict, type_: str, name: str) -> dict: + return next(e for e in doc["hosts"] if e["type"] == type_ and e["name"] == name) + + +class TestFieldVariantMigrationMatrix: + """All 5 types x their 4/6/7-field legacy variants map to the right + nested v2 fields (contracts/registry-file-v2.md's per-type table).""" + + def test_all_types_and_field_variants(self, tmp_config_dir): + lines = [ + # incus: instance_id -> incus.host_user; region has no v2 home. + legacy_line("incus", "h1/c1", "1.1.1.1", "remo"), + legacy_line("incus", "h1/c2", "1.1.1.2", "remo", "paul", "direct"), + legacy_line("incus", "h1/c3", "1.1.1.3", "remo", "paul", "direct", "somenode"), + # proxmox: instance_id -> proxmox.vmid, region -> proxmox.node_user. + legacy_line("proxmox", "p/c1", "2.2.2.1", "remo"), + legacy_line("proxmox", "p/c2", "2.2.2.2", "remo", "104", "direct"), + legacy_line("proxmox", "p/c3", "2.2.2.3", "remo", "105", "direct", "root"), + # aws: instance_id -> aws.instance_id, region -> aws.region. + legacy_line("aws", "a1", "3.3.3.1", "remo"), + legacy_line("aws", "a2", "3.3.3.2", "remo", "i-002", "ssm"), + legacy_line("aws", "a3", "3.3.3.3", "remo", "i-003", "ssm", "us-west-2"), + # hetzner: no nested fields, ever. + legacy_line("hetzner", "h1", "4.4.4.1", "remo"), + legacy_line( + "hetzner", "h2", "4.4.4.2", "remo", "ignored_id", "direct", "ignored_region" + ), + # ssh: instance_id -> ssh.port (int), region -> ssh.identity_file. + legacy_line("ssh", "s1", "nas1.lan", "admin"), + legacy_line("ssh", "s2", "nas2.lan", "admin", "2022", "direct"), + legacy_line("ssh", "s3", "nas3.lan", "admin", "2023", "direct", "/home/x/id_rsa"), + ] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert report.migrated_count == len(lines) + doc = _registry_doc(tmp_config_dir) + + e = _entry(doc, "incus", "h1/c1") + assert e["access"] == "direct" + assert "incus" not in e + + e = _entry(doc, "incus", "h1/c2") + assert e["access"] == "direct" + assert e["incus"] == {"host_user": "paul"} + + e = _entry(doc, "incus", "h1/c3") + assert e["access"] == "direct" + assert e["incus"] == {"host_user": "paul"} # region dropped: no v2 home + + e = _entry(doc, "proxmox", "p/c1") + assert e["access"] == "direct" + assert "proxmox" not in e + + e = _entry(doc, "proxmox", "p/c2") + assert e["proxmox"] == {"vmid": "104"} + + e = _entry(doc, "proxmox", "p/c3") + assert e["proxmox"] == {"vmid": "105", "node_user": "root"} + + e = _entry(doc, "aws", "a1") + assert e["access"] == "direct" + assert "aws" not in e + + e = _entry(doc, "aws", "a2") + assert e["access"] == "ssm" + assert e["aws"] == {"instance_id": "i-002"} + + e = _entry(doc, "aws", "a3") + assert e["access"] == "ssm" + assert e["aws"] == {"instance_id": "i-003", "region": "us-west-2"} + + e = _entry(doc, "hetzner", "h1") + assert "hetzner" not in e + + e = _entry(doc, "hetzner", "h2") + assert "hetzner" not in e # extra fields silently ignored, no error + + e = _entry(doc, "ssh", "s1") + assert "ssh" not in e + + e = _entry(doc, "ssh", "s2") + assert e["ssh"] == {"port": 2022} + + e = _entry(doc, "ssh", "s3") + assert e["ssh"] == {"port": 2023, "identity_file": "/home/x/id_rsa"} + + +class TestGarbageAndUnknownLines: + def test_garbage_line_not_migrated_and_preserved_in_backup(self, tmp_config_dir): + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo"), "this line is garbage"] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert "this line is garbage" in report.skipped_lines + backup_text = get_registry_backup_path().read_text() + assert "this line is garbage" in backup_text + + view = read_registry() + assert len(view.hosts) == 1 + assert view.hosts[0].name == "web1" + + def test_short_line_not_migrated(self, tmp_config_dir): + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo"), "a:b:c"] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert "a:b:c" in report.skipped_lines + backup_text = get_registry_backup_path().read_text() + assert "a:b:c" in backup_text + + def test_unknown_type_line_preserved_not_dropped(self, tmp_config_dir): + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo"), "docker:mybox:1.2.3.4:remo"] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert report.migrated_count == 2 # 1 known + 1 unknown, nothing dropped + + view = read_registry() + assert view.unknown_entries == 1 + doc = _registry_doc(tmp_config_dir) + docker_entries = [e for e in doc["hosts"] if e["type"] == "docker"] + assert len(docker_entries) == 1 + assert docker_entries[0]["name"] == "mybox" + assert docker_entries[0]["host"] == "1.2.3.4" + + +class TestTolerantMigration: + """A legacy entry that parses but fails v2 validation is skipped and + reported, never fatal (FR-009/FR-014) — migrating one bad entry must not + brick every subsequent CLI command. The original bytes survive in the + backup regardless.""" + + def test_validation_failing_entry_skipped_not_fatal(self, tmp_config_dir): + bad = legacy_line("ssh", "box", "nas.lan", "admin", "999999", "direct") # port OOR + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo"), bad] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert report.migrated_count == 1 # only the valid hetzner entry + assert any("ssh:box" in s and "out of range" in s for s in report.skipped_lines) + # The bad line's original bytes are preserved verbatim in the backup. + assert bad in get_registry_backup_path().read_text() + + view = read_registry() + assert [h.name for h in view.hosts] == ["web1"] + + def test_duplicate_type_name_deduped_not_fatal(self, tmp_config_dir): + lines = [ + legacy_line("hetzner", "dup", "5.5.5.5", "remo"), + legacy_line("hetzner", "dup", "6.6.6.6", "remo"), # same (type, name) + ] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert report.migrated_count == 1 # first occurrence kept + assert any("duplicate" in s for s in report.skipped_lines) + + doc = _registry_doc(tmp_config_dir) + dups = [e for e in doc["hosts"] if e["name"] == "dup"] + assert len(dups) == 1 + assert dups[0]["host"] == "5.5.5.5" # first wins + + +class TestBothPresentResilience: + """Read-only both-present reads must not depend on the legacy file being + readable, and non-readonly reads must degrade (not crash) if the legacy + file becomes unreadable — the concurrent-migration-rename race window.""" + + def test_readonly_both_present_ignores_unreadable_legacy(self, tmp_config_dir): + write_v2_registry(tmp_config_dir, [build_v2_host_entry("hetzner", "web1", "5.5.5.5", "remo")]) + legacy = tmp_config_dir / "known_hosts" + legacy.write_text(legacy_line("hetzner", "web1", "5.5.5.5", "remo") + "\n") + legacy.chmod(0o000) + try: + view = read_registry(readonly=True) + finally: + legacy.chmod(0o600) + assert [h.name for h in view.hosts] == ["web1"] + + def test_nonreadonly_both_present_degrades_on_unreadable_legacy(self, tmp_config_dir): + write_v2_registry(tmp_config_dir, [build_v2_host_entry("hetzner", "web1", "5.5.5.5", "remo")]) + legacy = tmp_config_dir / "known_hosts" + legacy.write_text(legacy_line("hetzner", "web1", "5.5.5.5", "remo") + "\n") + legacy.chmod(0o000) + try: + view = read_registry(readonly=False) # must not raise RegistryReadError + finally: + legacy.chmod(0o600) + assert [h.name for h in view.hosts] == ["web1"] + + +class TestEmptyVsMissingLegacyFile: + def test_empty_known_hosts_file_migrates_to_empty_registry(self, tmp_config_dir): + (tmp_config_dir / "known_hosts").write_text("") + + report = migrate_if_needed() + + assert report is not None + assert report.migrated_count == 0 + assert get_registry_path().exists() + + view = read_registry() + assert view.hosts == [] + assert view.source_format == "v2" + + def test_missing_known_hosts_file_needs_no_migration(self, tmp_config_dir): + assert not (tmp_config_dir / "known_hosts").exists() + + report = migrate_if_needed() + + assert report is None + assert not get_registry_path().exists() + + view = read_registry() + assert view.source_format == "empty" + assert view.hosts == [] + assert not get_registry_path().exists() # read alone never creates the file + + +class TestPreExistingBackupSuffixing: + def test_existing_backup_is_never_clobbered(self, tmp_config_dir): + pre_existing = tmp_config_dir / "known_hosts.v1.bak" + pre_existing.write_text("PRE-EXISTING BACKUP CONTENT\n") + + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo")] + write_legacy_registry(tmp_config_dir, lines) + + report = migrate_if_needed() + + assert report is not None + assert report.backup_path.name == "known_hosts.v1.bak.1" + assert pre_existing.read_text() == "PRE-EXISTING BACKUP CONTENT\n" + assert report.backup_path.read_text() == "\n".join(lines) + "\n" + + +class TestInterruptedMigrationConvergence: + """S3 (data-model.md §6): registry.json and known_hosts both present with + equivalent content, as if migration wrote v2 then crashed before the + rename. read_registry() must silently complete the rename.""" + + def test_equivalent_both_present_completes_rename_silently(self, tmp_config_dir): + lines = [legacy_line("hetzner", "web1", "5.5.5.5", "remo")] + write_legacy_registry(tmp_config_dir, lines) + entry = build_v2_host_entry("hetzner", "web1", "5.5.5.5", "remo", access="direct") + write_v2_registry(tmp_config_dir, [entry]) + + view = read_registry(readonly=False) + + assert view.warnings == [] + assert len(view.hosts) == 1 + assert view.hosts[0].name == "web1" + assert not (tmp_config_dir / "known_hosts").exists() + assert get_registry_backup_path().exists() + + +class TestDivergentBothPresent: + """S4 (data-model.md §6): registry.json and known_hosts both present with + differing content. v2 wins, never merged, warning surfaced, known_hosts + left in place (no automatic resolution).""" + + def test_divergent_warns_v2_wins_never_merges(self, tmp_config_dir): + write_legacy_registry( + tmp_config_dir, [legacy_line("hetzner", "web1", "5.5.5.5", "remo")] + ) + write_v2_registry( + tmp_config_dir, + [build_v2_host_entry("hetzner", "web1", "9.9.9.9", "remo", access="direct")], + ) + + view = read_registry(readonly=False) + + assert any("differ" in w for w in view.warnings) + assert len(view.hosts) == 1 + assert view.hosts[0].host == "9.9.9.9" # v2 wins + assert (tmp_config_dir / "known_hosts").exists() # never renamed away + assert (tmp_config_dir / "registry.json").exists() + + +class TestCompletedMigrationNeverReruns: + def test_second_call_is_a_true_no_op(self, tmp_config_dir): + write_legacy_registry( + tmp_config_dir, [legacy_line("hetzner", "web1", "5.5.5.5", "remo")] + ) + report1 = migrate_if_needed() + assert report1 is not None + backup_path = report1.backup_path + mtime_before = backup_path.stat().st_mtime + + report2 = migrate_if_needed() + + assert report2 is None + assert backup_path.stat().st_mtime == mtime_before + assert not (tmp_config_dir / "known_hosts.v1.bak.1").exists() + + read_registry(readonly=False) # a normal read must not re-trigger anything + assert not (tmp_config_dir / "known_hosts.v1.bak.1").exists() + assert backup_path.stat().st_mtime == mtime_before + + +class TestLegacyAccessModeVariants: + """research R5 — the two legacy access-mode variants no current writer + produces but old files can contain. Both must map to access: "direct" + (only aws may ever be "ssm"), despite superficially looking SSM-ish.""" + + def test_incus_implicit_ssm_maps_to_direct(self, tmp_config_dir): + write_legacy_registry(tmp_config_dir, [LEGACY_FIXTURE_LINES["incus_implicit_ssm"]]) + + report = migrate_if_needed() + + assert report is not None + doc = _registry_doc(tmp_config_dir) + e = _entry(doc, "incus", "old/box") + assert e["access"] == "direct" + + def test_proxmox_empty_access_maps_to_direct(self, tmp_config_dir): + write_legacy_registry(tmp_config_dir, [LEGACY_FIXTURE_LINES["proxmox_empty_access"]]) + + report = migrate_if_needed() + + assert report is not None + doc = _registry_doc(tmp_config_dir) + e = _entry(doc, "proxmox", "old/pct") + assert e["access"] == "direct" diff --git a/tests/unit/core/test_web_adopt_payload.py b/tests/unit/core/test_web_adopt_payload.py index c6e1489..195323d 100644 --- a/tests/unit/core/test_web_adopt_payload.py +++ b/tests/unit/core/test_web_adopt_payload.py @@ -1,4 +1,5 @@ -"""Tests for the adoption payload builder in remo_cli.core.web_adopt (011-web-adopt, T022). +"""Tests for the adoption payload builder in remo_cli.core.web_adopt (011-web-adopt, T022; +updated for 015-registry-v2 T032/T033: the payload is now v2-shaped). Covers build_adoption_payload full-mirror semantics (FR-008), SSM host-key exclusion (FR-012), the empty-registry guard (FR-016), the no-private-key @@ -21,7 +22,9 @@ # Helpers # ----------------------------------------------------------------------- -ALL_FIELDS = ("type", "name", "host", "user", "instance_id", "access_mode", "region") +#: v2 hostEntry common fields (registry-file-v2.md) — every entry always +#: carries these; per-type nested fields (e.g. "aws") are additional. +COMMON_FIELDS = ("type", "name", "host", "user", "access") def _make_host(type_="incus", name="myhost/dev", host="10.0.0.1", user="remo", **kwargs): @@ -64,12 +67,12 @@ def _sample_hosts() -> list[KnownHost]: class TestFullMirror: - """The payload mirrors the complete registry, every field, version 1.""" + """The payload mirrors the complete registry, every field, current version.""" - def test_version_is_one(self): + def test_version_matches_payload_version(self): payload = build_adoption_payload(_sample_hosts()) - assert payload["version"] == 1 assert payload["version"] == PAYLOAD_VERSION + assert payload["version"] == 2 def test_every_registry_entry_present(self): hosts = _sample_hosts() @@ -77,13 +80,16 @@ def test_every_registry_entry_present(self): assert len(payload["registry"]) == len(hosts) assert [e["name"] for e in payload["registry"]] == [h.name for h in hosts] - def test_all_seven_knownhost_fields_mirrored(self): + def test_common_fields_mirrored_as_v2_hostentry(self): hosts = _sample_hosts() payload = build_adoption_payload(hosts) for host, entry in zip(hosts, payload["registry"]): - assert set(entry.keys()) == set(ALL_FIELDS) - for field_name in ALL_FIELDS: - assert entry[field_name] == getattr(host, field_name) + assert set(COMMON_FIELDS) <= set(entry.keys()) + assert entry["type"] == host.type + assert entry["name"] == host.name + assert entry["host"] == host.host + assert entry["user"] == host.user + assert entry["access"] == (host.access_mode or "direct") def test_ssm_entry_fields_mirrored_verbatim(self): """SSM entries are excluded from host_keys but fully present in registry.""" @@ -94,9 +100,8 @@ def test_ssm_entry_fields_mirrored_verbatim(self): "name": "devbox-ssm", "host": "3.14.15.92", "user": "remo", - "instance_id": "i-0abc123def", - "access_mode": "ssm", - "region": "us-west-2", + "access": "ssm", + "aws": {"instance_id": "i-0abc123def", "region": "us-west-2"}, } def test_host_keys_default_to_empty_dict(self): @@ -220,7 +225,7 @@ def test_payload_carries_nothing_beyond_registry_and_provided_keys(self, identit assert set(payload.keys()) == {"version", "registry", "host_keys"} for entry in payload["registry"]: - assert set(entry.keys()) == set(ALL_FIELDS) + assert set(COMMON_FIELDS) <= set(entry.keys()) assert payload["host_keys"] == {"web1": [HOST_KEY_LINE]} # Every host-key line in the payload is one the caller provided. for lines in payload["host_keys"].values(): @@ -247,7 +252,11 @@ def test_ssm_mode_is_not_direct(self): host = _make_host(type_="aws", name="devbox", instance_id="i-0abc", access_mode="ssm") assert is_direct_access(host) is False - def test_instance_id_with_default_empty_mode_classifies_as_ssm(self): - """A 5-field legacy entry (instance_id, no explicit mode) defaults to SSM.""" + def test_instance_id_with_default_empty_mode_classifies_as_direct(self): + """Registry v2: access is always explicit ("direct"/"ssm") on any + accessor-sourced KnownHost; an empty access_mode (the dataclass + default) means "direct" — the old implicit-empty-means-ssm legacy + quirk is resolved at the accessor boundary (core/registry.py), not + inferred here anymore.""" host = _make_host(type_="aws", name="devbox", instance_id="i-0abc", access_mode="") - assert is_direct_access(host) is False + assert is_direct_access(host) is True diff --git a/tests/unit/core/test_web_push.py b/tests/unit/core/test_web_push.py index dd86fe7..c17b483 100644 --- a/tests/unit/core/test_web_push.py +++ b/tests/unit/core/test_web_push.py @@ -78,7 +78,11 @@ def api_client(mocker): client = mocker.MagicMock() client.base_url = URL client.token = CODE - client.get_status.return_value = {"state": "adopted", "registry_instances": 2} + client.get_status.return_value = { + "state": "adopted", + "registry_instances": 2, + "payload_versions": [1, 2], + } client.get_identity.return_value = {"deployment_id": DEPLOYMENT_ID, "public_key": PUBLIC_KEY} client.put_registry.return_value = {"registry_instances": 2, "host_key_instances": 1} client.post_verify.return_value = {"all_passed": True, "results": []} @@ -160,6 +164,7 @@ def test_junk_entries_dropped(self, tmp_config_dir): push_cache_path().write_text( json.dumps( { + "cache_version": 2, "push_cache": { DEPLOYMENT_ID: { "good": {"fingerprint": "f" * 64, "host_keys": [KEY_LINE_NODE1]}, @@ -198,13 +203,30 @@ def test_is_sha256_hex(self): {"host": "10.0.0.99"}, {"user": "other"}, {"instance_id": "i-0abc123"}, - {"access_mode": "direct"}, - {"region": "us-east-1"}, + {"access_mode": "ssm"}, ], ) def test_any_field_change_changes_fingerprint(self, change): + # NOTE: an empty access_mode ("" — the KnownHost dataclass default) and + # an explicit "direct" both serialize to the same v2 "access": "direct" + # (known_host_to_entry normalizes), so they are NOT expected to produce + # different fingerprints — hence "ssm" here instead of "direct". assert instance_fingerprint(_make_host()) != instance_fingerprint(_make_host(**change)) + def test_region_change_changes_fingerprint_for_types_that_use_it(self): + # "region" only maps to a nested v2 field for proxmox/aws/ssh — not + # incus (test_any_field_change_changes_fingerprint's default type), + # which has no v2 slot for it at all. + base = _make_host( + type_="proxmox", name="pve1/dev", instance_id="104", + access_mode="direct", region="root", + ) + changed = _make_host( + type_="proxmox", name="pve1/dev", instance_id="104", + access_mode="direct", region="admin", + ) + assert instance_fingerprint(base) != instance_fingerprint(changed) + # --------------------------------------------------------------------------- # Adopt seeds the push cache (no consent, no url/token) diff --git a/tests/unit/providers/test_added_add.py b/tests/unit/providers/test_added_add.py index e65ae4a..a7b20e5 100644 --- a/tests/unit/providers/test_added_add.py +++ b/tests/unit/providers/test_added_add.py @@ -1,9 +1,12 @@ -"""Tests for providers/added.py add() + target parsing (feature 014, US1). - -Covers target parsing (default user, overrides, IPv6/bracketed rejection), -identity-colon rejection, create-writes-one-entry, provider-name collision -refusal (FR-010), and FR-006 resolvability/picker inclusion of an added host. -SSH/registry are mocked except the FR-006 test, which uses a real temp registry. +"""Tests for providers/added.py add() + target parsing (feature 014, US1; +updated for 015-registry-v2 T020: IPv6 literals and colons are now accepted — +the registry.json format has no positional colon-overloading to protect +against, so only control characters remain rejected). + +Covers target parsing (default user, overrides, IPv6 bracket/bare forms), +create-writes-one-entry, provider-name collision refusal (FR-010), and FR-006 +resolvability/picker inclusion of an added host. SSH/registry are mocked +except the FR-006 test, which uses a real temp registry. """ from __future__ import annotations @@ -38,14 +41,28 @@ def test_user_override_wins(self) -> None: def test_port_override_wins(self) -> None: assert added.parse_ssh_target("host:22", port_override=2022)[2] == 2022 - @pytest.mark.parametrize("bad", ["::1", "fe80::1", "user@2001:db8::1"]) - def test_ipv6_literal_rejected(self, bad: str) -> None: - with pytest.raises(ValueError, match="IPv6"): - added.parse_ssh_target(bad) - - def test_bracketed_ipv6_rejected(self) -> None: - with pytest.raises(ValueError, match="IPv6"): - added.parse_ssh_target("user@[2001:db8::1]:22") + @pytest.mark.parametrize( + ("target", "expected_host"), + [("::1", "::1"), ("fe80::1", "fe80::1"), ("user@2001:db8::1", "2001:db8::1")], + ) + def test_bare_ipv6_literal_accepted_no_port(self, target: str, expected_host: str) -> None: + """A bracket-less multi-colon TARGET is an IPv6 literal with no port + suffix (a legal host:port has exactly one colon).""" + _, host, port = added.parse_ssh_target(target) + assert host == expected_host + assert port == 22 + + def test_bracketed_ipv6_with_port_accepted(self) -> None: + user, host, port = added.parse_ssh_target("user@[2001:db8::1]:22") + assert (user, host, port) == ("user", "2001:db8::1", 22) + + def test_bracketed_ipv6_without_port_accepted(self) -> None: + user, host, port = added.parse_ssh_target("admin@[2001:db8::7]") + assert (user, host, port) == ("admin", "2001:db8::7", 22) + + def test_unmatched_bracket_rejected(self) -> None: + with pytest.raises(ValueError, match="unmatched"): + added.parse_ssh_target("user@[2001:db8::1") def test_non_numeric_port_rejected(self) -> None: with pytest.raises(ValueError, match="not a number"): @@ -63,10 +80,10 @@ def test_empty_user_before_at_rejected(self) -> None: with pytest.raises(ValueError): added.parse_ssh_target("@host") - def test_user_with_colon_rejected(self) -> None: - # A ':' in the user would shift every later registry field on reload. - with pytest.raises(ValueError, match="user must not contain"): - added.parse_ssh_target("host", user_override="ev:il") + def test_user_with_colon_accepted(self) -> None: + # registry.json has no positional colon-overloading to protect + # against (unlike the old colon-delimited known_hosts format). + assert added.parse_ssh_target("host", user_override="ev:il")[0] == "ev:il" def test_user_with_newline_rejected(self) -> None: # A newline would inject a second registry line. @@ -128,16 +145,18 @@ def test_identity_stored_in_region(self, mocker) -> None: assert save.call_args.args[0].region == "/home/dev/.ssh/id" - def test_identity_with_colon_rejected_no_write(self, mocker) -> None: + def test_identity_with_colon_accepted(self, mocker) -> None: + # registry.json has no positional colon-overloading to protect + # against, so a colon in an identity path is stored intact. mocker.patch("remo_cli.providers.added.get_known_hosts", return_value=[]) save = mocker.patch("remo_cli.providers.added.save_known_host") - err = mocker.patch("remo_cli.providers.added.print_error") + mocker.patch("remo_cli.providers.added.print_success") + mocker.patch("remo_cli.providers.added.print_info") rc = added.add(name="box", target="dev@host", identity="/bad:path/key") - assert rc == 2 - save.assert_not_called() - assert err.called + assert rc == 0 + assert save.call_args.args[0].region == "/bad:path/key" def test_identity_with_newline_rejected_no_write(self, mocker) -> None: # A newline in the identity would inject a forged registry line. @@ -150,12 +169,25 @@ def test_identity_with_newline_rejected_no_write(self, mocker) -> None: assert rc == 2 save.assert_not_called() + def test_bare_ipv6_target_accepted(self, mocker) -> None: + mocker.patch("remo_cli.providers.added.get_known_hosts", return_value=[]) + save = mocker.patch("remo_cli.providers.added.save_known_host") + mocker.patch("remo_cli.providers.added.print_success") + mocker.patch("remo_cli.providers.added.print_info") + + rc = added.add(name="box", target="::1") + + assert rc == 0 + entry = save.call_args.args[0] + assert entry.host == "::1" + assert entry.user == "remo" + def test_malformed_target_rejected_no_write(self, mocker) -> None: mocker.patch("remo_cli.providers.added.get_known_hosts", return_value=[]) save = mocker.patch("remo_cli.providers.added.save_known_host") mocker.patch("remo_cli.providers.added.print_error") - rc = added.add(name="box", target="::1") + rc = added.add(name="box", target="host:nope") assert rc == 2 save.assert_not_called() diff --git a/tests/unit/providers/test_provider_registry_entries.py b/tests/unit/providers/test_provider_registry_entries.py new file mode 100644 index 0000000..9fe78b8 --- /dev/null +++ b/tests/unit/providers/test_provider_registry_entries.py @@ -0,0 +1,234 @@ +"""Provider save-path fixture tests (T016). + +Pins the EXACT KnownHost field usage of each provider's save call site +(providers/incus.py, proxmox.py, aws.py, hetzner.py, added.py) and proves +the legacy<->v2 mapping round-trips it losslessly two ways: + +1. Straight to v2 (known_host_to_entry -> entry_to_known_host) — what + happens once these call sites are routed through the accessor. +2. Through today's legacy codec (to_line -> from_line -> legacy_fields_to_entry + -> entry_to_known_host) — simulating "a legacy file written by today's + code, then migrated". This is the research R5 risk pin: prove today's + actual writer output survives migration, including the aws to_line() + implicit-SSM back-fill quirk. +""" + +from __future__ import annotations + +from remo_cli.core.registry import ( + entry_to_known_host, + known_host_to_entry, + legacy_fields_to_entry, +) +from remo_cli.models.host import KnownHost + + +def _v2_round_trip(host: KnownHost) -> KnownHost | None: + return entry_to_known_host(known_host_to_entry(host)) + + +def _legacy_round_trip(host: KnownHost) -> KnownHost | None: + """Simulate: today's writer emits a legacy line, migration parses it.""" + line = host.to_line() + parsed = KnownHost.from_line(line) + entry = legacy_fields_to_entry( + parsed.type, + parsed.name, + parsed.host, + parsed.user, + parsed.instance_id, + parsed.access_mode, + parsed.region, + ) + return entry_to_known_host(entry) + + +class TestIncusSaveShape: + """providers/incus.py create()/sync(): instance_id holds the Incus HOST's + SSH user (-> incus.host_user); region is never set.""" + + def _make(self) -> KnownHost: + return KnownHost( + type="incus", + name="nuc/dev1", + host="dev1.incus", + user="remo", + instance_id="paul", + access_mode="direct", + ) + + def test_v2_round_trip(self): + host = self._make() + assert _v2_round_trip(host) == host + + def test_legacy_round_trip(self): + host = self._make() + assert _legacy_round_trip(host) == host + + +class TestProxmoxSaveShape: + """providers/proxmox.py create()/sync(): instance_id holds the numeric + VMID (-> proxmox.vmid); region holds the Proxmox NODE's SSH user + (-> proxmox.node_user, confusingly named "region" in the old schema).""" + + def _make(self) -> KnownHost: + return KnownHost( + type="proxmox", + name="pve1/dev2", + host="10.0.0.42", + user="remo", + instance_id="104", + access_mode="direct", + region="root", + ) + + def test_v2_round_trip(self): + host = self._make() + assert _v2_round_trip(host) == host + + def test_legacy_round_trip(self): + host = self._make() + assert _legacy_round_trip(host) == host + + +class TestAwsSaveShape: + """providers/aws.py create(): access_mode is always literally "ssm" on + create (sync/other paths can set it from an instance tag, but it is + always non-empty).""" + + def _make(self) -> KnownHost: + return KnownHost( + type="aws", + name="buildbox", + host="203.0.113.7", + user="remo", + instance_id="i-0abc123def456", + access_mode="ssm", + region="us-east-1", + ) + + def test_v2_round_trip(self): + host = self._make() + assert _v2_round_trip(host) == host + + def test_legacy_round_trip(self): + host = self._make() + assert _legacy_round_trip(host) == host + + +class TestHetznerSaveShape: + """providers/hetzner.py create()/sync(): instance_id, access_mode, and + region all default — access_mode is "" at construction, not "direct" + explicitly. known_host_to_entry serializes "" as "access": "direct" + (`host.access_mode or "direct"`), and a v2/legacy round-trip normalizes + the in-memory value to the explicit "direct" (data-model.md §3: the + legacy implicit-empty convention no longer leaks upward after a v2/ + accessor load) — so this is the one shape that is NOT byte-for-field + identical post-round-trip, by design.""" + + def _make(self) -> KnownHost: + return KnownHost( + type="hetzner", + name="dev1", + host="198.51.100.9", + user="remo", + ) + + def _normalized(self) -> KnownHost: + return KnownHost( + type="hetzner", + name="dev1", + host="198.51.100.9", + user="remo", + instance_id="", + access_mode="direct", + region="", + ) + + def test_constructed_with_empty_access_mode(self): + host = self._make() + assert host.access_mode == "" + assert host.instance_id == "" + assert host.region == "" + + def test_v2_entry_serializes_empty_access_mode_as_direct(self): + entry = known_host_to_entry(self._make()) + assert entry["access"] == "direct" + assert "hetzner" not in entry + + def test_v2_round_trip_normalizes_access_mode_to_direct(self): + assert _v2_round_trip(self._make()) == self._normalized() + + def test_legacy_round_trip_normalizes_access_mode_to_direct(self): + assert _legacy_round_trip(self._make()) == self._normalized() + + +class TestSshAddedSaveShape: + """providers/added.py add(): instance_id is the port AS A STRING + (-> ssh.port as an int in v2); region is the optional identity file + path (-> ssh.identity_file).""" + + def _make(self) -> KnownHost: + return KnownHost( + type="ssh", + name="nas", + host="nas.lan", + user="admin", + instance_id="2222", + access_mode="direct", + region="/home/paul/.ssh/id_nas", + ) + + def test_v2_round_trip(self): + host = self._make() + assert _v2_round_trip(host) == host + + def test_legacy_round_trip(self): + host = self._make() + assert _legacy_round_trip(host) == host + + def test_port_str_converts_to_v2_int(self): + entry = known_host_to_entry(self._make()) + assert entry["ssh"]["port"] == 2222 + assert isinstance(entry["ssh"]["port"], int) + + +class TestAwsImplicitSsmBackfillQuirk: + """research.md R5: KnownHost.to_line() back-fills access_mode="ssm" when + instance_id is set and access_mode is empty. For type=aws this quirk is + semantically CORRECT (an aws entry carrying an instance id but no + explicit access mode should still be treated as ssm) — contrast with + the non-aws "incus_implicit_ssm" fixture in test_registry_migration.py, + which carries superficially similar legacy bytes but must map to + "direct" (only aws may ever be "ssm").""" + + def _make(self) -> KnownHost: + return KnownHost( + type="aws", + name="buildbox", + host="203.0.113.7", + user="remo", + instance_id="i-abc", + access_mode="", + ) + + def test_to_line_backfills_ssm(self): + line = self._make().to_line() + assert line == "aws:buildbox:203.0.113.7:remo:i-abc:ssm" + + def test_migrated_line_classifies_as_ssm(self): + line = self._make().to_line() + parsed = KnownHost.from_line(line) + entry = legacy_fields_to_entry( + parsed.type, + parsed.name, + parsed.host, + parsed.user, + parsed.instance_id, + parsed.access_mode, + parsed.region, + ) + assert entry["access"] == "ssm" + reconstructed = entry_to_known_host(entry) + assert reconstructed is not None + assert reconstructed.access_mode == "ssm" diff --git a/tests/unit/web/conftest.py b/tests/unit/web/conftest.py index 7216f15..fdf9526 100644 --- a/tests/unit/web/conftest.py +++ b/tests/unit/web/conftest.py @@ -54,8 +54,14 @@ def __init__(self, home: Path, user_home: Path, monkeypatch: pytest.MonkeyPatch) @property def registry_path(self) -> Path: + """Legacy colon-delimited known_hosts path — read-in-place input format.""" return self.home / "known_hosts" + @property + def v2_registry_path(self) -> Path: + """registry.json (v2) — where PUT /setup/registry now writes (015-registry-v2).""" + return self.home / "registry.json" + @property def web_identity_dir(self) -> Path: return self.home / "web-identity" diff --git a/tests/unit/web/test_check.py b/tests/unit/web/test_check.py index 0e61181..928de67 100644 --- a/tests/unit/web/test_check.py +++ b/tests/unit/web/test_check.py @@ -25,7 +25,7 @@ from remo_cli.cli.main import cli from remo_cli.web import check as check_module from remo_cli.web import health -from remo_cli.web.discovery import _read_known_hosts_readonly +from remo_cli.web.discovery import _read_registry_hosts_readonly from remo_cli.web.check import all_passed, format_results, run_checks from remo_cli.web.config import WebSettings @@ -265,7 +265,7 @@ def test_read_known_hosts_degrades_to_empty(self, tmp_config_dir): _write_registry(tmp_config_dir, ["incus:dev:127.0.0.1:remo"]) tmp_config_dir.chmod(0o000) try: - assert _read_known_hosts_readonly() == [] + assert _read_registry_hosts_readonly() == [] finally: tmp_config_dir.chmod(0o700) diff --git a/tests/unit/web/test_csp.py b/tests/unit/web/test_csp.py index 20a52d5..20a2394 100644 --- a/tests/unit/web/test_csp.py +++ b/tests/unit/web/test_csp.py @@ -119,7 +119,8 @@ def test_originless_setup_request_bypasses_origin_check(tmp_path, monkeypatch): # /api/v1/setup/* — this is what lets the `remo web adopt` CLI (and its # --via tunnel) talk to a live service. The bearer dependency still gates # the route: with a token configured and presented, the request reaches - # domain validation (422 for a garbage body), never 403. + # domain validation (400 unsupported_payload_version for a body with no + # "version" field), never 403. settings = _settings() settings.operator_auth = "none" settings.web_identity_dir = tmp_path / "web-identity" @@ -132,7 +133,7 @@ def test_originless_setup_request_bypasses_origin_check(tmp_path, monkeypatch): json={}, headers={"Authorization": f"Bearer {code}"}, ) - assert resp.status_code == 422 + assert resp.status_code == 400 def test_setup_request_with_disallowed_origin_still_rejected(): diff --git a/tests/unit/web/test_registry_readonly.py b/tests/unit/web/test_registry_readonly.py new file mode 100644 index 0000000..99232c1 --- /dev/null +++ b/tests/unit/web/test_registry_readonly.py @@ -0,0 +1,212 @@ +"""Web readonly registry tests (T025, US3). + +Covers quickstart.md §6: `remo web check`/discovery against a read-only +volume must see the same host set in either registry format, with zero +writes/mkdirs, and must degrade gracefully (never crash) on a per-entry +malformed record or on a structurally newer-format `registry.json` -- the +latter mapping all the way through to `web.state.detect_state()` returning +`ConfigurationState.BROKEN` (T023). +""" + +from __future__ import annotations + +import os + +import pytest + +from remo_cli.core.registry import RegistryNewerVersionError, read_registry +from remo_cli.web.state import ConfigurationState, detect_state +from tests.conftest import ( + LEGACY_FIXTURE_LINES, + build_v2_host_entry, + write_legacy_registry, + write_v2_registry, +) + +skip_if_root = pytest.mark.skipif(os.geteuid() == 0, reason="root bypasses permission bits") + +# The two "legacy access-mode variant" fixture entries exist to pin the +# migration mapper's type-first rule (research R5) and are covered there +# (test_registry_migration.py); the readonly parity/no-side-effects tests +# here only need the five ordinary types. +_STANDARD_LEGACY_TYPES = ("incus", "proxmox", "aws", "hetzner", "ssh") + + +def _host_key(h) -> tuple: + """A hashable/sortable projection of every `KnownHost` field.""" + return (h.type, h.name, h.host, h.user, h.instance_id, h.access_mode, h.region) + + +def _host_keys(hosts) -> set[tuple]: + return {_host_key(h) for h in hosts} + + +def _snapshot_mtimes(directory) -> dict[str, float]: + return {p.name: p.stat().st_mtime for p in directory.iterdir()} + + +def _snapshot_names(directory) -> set[str]: + return {p.name for p in directory.iterdir()} + + +# --------------------------------------------------------------------------- +# Both formats on a read-only (chmod 555) REMO_HOME: no side effects +# --------------------------------------------------------------------------- + + +@skip_if_root +class TestReadonlyNoSideEffects: + def test_legacy_format_on_readonly_dir(self, tmp_path, monkeypatch): + config_dir = tmp_path / "remo" + config_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(config_dir)) + + lines = [LEGACY_FIXTURE_LINES[t] for t in _STANDARD_LEGACY_TYPES] + write_legacy_registry(config_dir, lines) + + names_before = _snapshot_names(config_dir) + mtimes_before = _snapshot_mtimes(config_dir) + + config_dir.chmod(0o555) + try: + view = read_registry(readonly=True) + finally: + config_dir.chmod(0o755) + + assert view.source_format == "legacy" + assert len(view.hosts) == len(_STANDARD_LEGACY_TYPES) + assert {h.type for h in view.hosts} == set(_STANDARD_LEGACY_TYPES) + + # No side effects: nothing created, nothing touched. + assert _snapshot_names(config_dir) == names_before + assert _snapshot_mtimes(config_dir) == mtimes_before + assert not (config_dir / "registry.json").exists() + assert not (config_dir / "registry.lock").exists() + + def test_v2_format_on_readonly_dir(self, tmp_path, monkeypatch): + config_dir = tmp_path / "remo" + config_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(config_dir)) + + entries = [ + build_v2_host_entry("incus", "nuc/dev1", "dev1.incus", "remo", host_user="paul"), + build_v2_host_entry( + "aws", + "buildbox", + "203.0.113.7", + "remo", + access="ssm", + instance_id="i-0abc123", + region="us-east-1", + ), + ] + write_v2_registry(config_dir, entries) + + names_before = _snapshot_names(config_dir) + mtimes_before = _snapshot_mtimes(config_dir) + + config_dir.chmod(0o555) + try: + view = read_registry(readonly=True) + finally: + config_dir.chmod(0o755) + + assert view.source_format == "v2" + assert {h.name for h in view.hosts} == {"nuc/dev1", "buildbox"} + + assert _snapshot_names(config_dir) == names_before + assert _snapshot_mtimes(config_dir) == mtimes_before + assert not (config_dir / "registry.lock").exists() + + +# --------------------------------------------------------------------------- +# CLI-vs-web host-set parity +# --------------------------------------------------------------------------- + + +class TestCliVsWebParity: + def test_legacy_fixture_parses_identically_readonly_and_migrating( + self, tmp_path, monkeypatch + ): + lines = [LEGACY_FIXTURE_LINES[t] for t in _STANDARD_LEGACY_TYPES] + + # "Web" read: readonly=True, in its own REMO_HOME -- never migrates. + web_dir = tmp_path / "web-remo" + web_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(web_dir)) + write_legacy_registry(web_dir, lines) + web_view = read_registry(readonly=True) + + # "CLI" read: readonly=False, in a SEPARATE REMO_HOME so migration's + # side effect (rewriting registry.json + renaming the backup) can't + # interfere with the web read above. + cli_dir = tmp_path / "cli-remo" + cli_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(cli_dir)) + write_legacy_registry(cli_dir, lines) + cli_view = read_registry(readonly=False) + + assert web_view.source_format == "legacy" + assert cli_view.source_format == "v2" # migrated on the non-readonly read + assert (cli_dir / "registry.json").exists() + assert not (cli_dir / "known_hosts").exists() # renamed to the .v1.bak backup + + assert _host_keys(web_view.hosts) == _host_keys(cli_view.hosts) + + +# --------------------------------------------------------------------------- +# Single malformed entry degrades to a warning, not a crash +# --------------------------------------------------------------------------- + + +class TestMalformedEntryDegrades: + def test_v2_entry_missing_user_degrades_to_warning(self, tmp_path, monkeypatch): + config_dir = tmp_path / "remo" + config_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(config_dir)) + + good_entry = build_v2_host_entry("incus", "nuc/dev1", "dev1.incus", "remo") + bad_entry = { + "type": "incus", + "name": "nuc/dev2", + "host": "dev2.incus", + # "user" is missing entirely. + "access": "direct", + } + write_v2_registry(config_dir, [good_entry, bad_entry]) + + view = read_registry(readonly=True) + + assert len(view.hosts) == 1 + assert view.hosts[0].name == "nuc/dev1" + assert any("nuc/dev2" in w or "index" in w for w in view.warnings) + + +# --------------------------------------------------------------------------- +# Newer-version registry.json -> RegistryNewerVersionError -> BROKEN state +# --------------------------------------------------------------------------- + + +class TestNewerVersionMapsToBroken: + def test_read_registry_raises_newer_version_error(self, tmp_path, monkeypatch): + config_dir = tmp_path / "remo" + config_dir.mkdir() + monkeypatch.setenv("REMO_HOME", str(config_dir)) + write_v2_registry(config_dir, [], version=99) + + with pytest.raises(RegistryNewerVersionError): + read_registry(readonly=True) + + def test_detect_state_reports_broken_for_newer_version_registry( + self, tmp_path, monkeypatch + ): + config_dir = tmp_path / "remo" + config_dir.mkdir() + user_home = tmp_path / "user-home" + user_home.mkdir() + monkeypatch.setenv("REMO_HOME", str(config_dir)) + monkeypatch.setenv("HOME", str(user_home)) + monkeypatch.delenv("REMO_WEB_SSH_IDENTITY_FILE", raising=False) + write_v2_registry(config_dir, [], version=99) + + assert detect_state() is ConfigurationState.BROKEN diff --git a/tests/unit/web/test_setup_api.py b/tests/unit/web/test_setup_api.py index afda8a3..9d665a4 100644 --- a/tests/unit/web/test_setup_api.py +++ b/tests/unit/web/test_setup_api.py @@ -15,6 +15,7 @@ from __future__ import annotations +import json from typing import Any import pytest @@ -22,7 +23,6 @@ from remo_cli.web import app as app_module from remo_cli.web import check as web_check_module -from remo_cli.web.api import setup as setup_api _ORIGIN = "http://testserver" _TOKEN = "unit-test-setup-token" @@ -97,7 +97,25 @@ def _payload(**overrides: Any) -> dict[str, Any]: return payload -_EXPECTED_REGISTRY_TEXT = "incus:dev:10.0.0.5:remo\naws:cloud:3.4.5.6:remo:i-0abc:ssm:us-east-1\n" +#: Expected v2 registry.json hosts (sorted by (type, name)) for `_payload()`'s +#: default two entries, once mapped through the v1->v2 legacy mapper. +_EXPECTED_V2_HOSTS = [ + { + "type": "aws", + "name": "cloud", + "host": "3.4.5.6", + "user": "remo", + "access": "ssm", + "aws": {"instance_id": "i-0abc", "region": "us-east-1"}, + }, + { + "type": "incus", + "name": "dev", + "host": "10.0.0.5", + "user": "remo", + "access": "direct", + }, +] def _service_known_hosts(state_dir): @@ -105,8 +123,9 @@ def _service_known_hosts(state_dir): def _assert_nothing_written(state_dir) -> None: - """FR-019 all-or-nothing: neither target file appears after a rejected PUT.""" + """FR-019 all-or-nothing: no target file appears after a rejected PUT.""" assert not state_dir.registry_path.exists() + assert not state_dir.v2_registry_path.exists() assert not _service_known_hosts(state_dir).exists() @@ -129,6 +148,7 @@ def test_status_unconfigured_without_identity(state_dir): "deployment_id": None, "public_key_available": False, "registry_instances": 0, + "payload_versions": [1, 2], } @@ -143,6 +163,7 @@ def test_status_unconfigured_with_identity(state_dir): "deployment_id": "dep12345", "public_key_available": True, "registry_instances": 0, + "payload_versions": [1, 2], } @@ -156,6 +177,7 @@ def test_status_adopted(state_dir): "deployment_id": "dep12345", "public_key_available": True, "registry_instances": 1, + "payload_versions": [1, 2], } @@ -172,6 +194,7 @@ def test_status_mount_configured_has_null_identity(state_dir, layout): "deployment_id": None, "public_key_available": False, "registry_instances": 1, + "payload_versions": [1, 2], } @@ -264,9 +287,12 @@ def test_put_registry_happy_path_applies_mirror_and_flips_to_adopted(state_dir): "host_key_instances": 1, } - # First-class file contents: service known_hosts + colon-delimited registry. + # First-class file contents: service known_hosts + v2 registry.json. assert _service_known_hosts(state_dir).read_text() == _VALID_KEY_LINE + "\n" - assert state_dir.registry_path.read_text() == _EXPECTED_REGISTRY_TEXT + doc = json.loads(state_dir.v2_registry_path.read_text()) + assert doc["version"] == 2 + assert doc["hosts"] == _EXPECTED_V2_HOSTS + assert not state_dir.registry_path.exists() # legacy mirror never written # The PUT does not end the session (verify is the terminal step, FR-007). status = client.get("/api/v1/setup/status", headers=_AUTH).json() @@ -317,14 +343,14 @@ def test_put_registry_empty_with_allow_empty_succeeds(state_dir): "registry_instances": 0, "host_key_instances": 0, } - assert state_dir.registry_path.read_text() == "" + doc = json.loads(state_dir.v2_registry_path.read_text()) + assert doc == {"version": 2, "hosts": []} assert _service_known_hosts(state_dir).read_text() == "" @pytest.mark.parametrize( ("body", "detail_fragment"), [ - pytest.param(_payload(version=2), "unsupported payload version 2", id="wrong-version"), pytest.param( _payload(host_keys={"ghost": [_VALID_KEY_LINE]}), "does not reference any registry entry", @@ -342,13 +368,19 @@ def test_put_registry_empty_with_allow_empty_succeeds(state_dir): ), pytest.param( _payload( - registry=[{"type": "incus", "name": "a:b", "host": "10.0.0.5", "user": "remo"}], + registry=[ + {"type": "incus", "name": "a\nb", "host": "10.0.0.5", "user": "remo"} + ], host_keys={}, ), - "cannot contain", - id="colon-in-registry-field", + "control characters", + id="control-character-in-registry-field", + ), + pytest.param( + _payload(version=2, registry=[{"type": "incus", "name": "dev", "host": "10.0.0.5"}]), + "user", + id="v2-entry-missing-required-field", ), - pytest.param({"registry": "nope"}, "", id="structurally-malformed-body"), ], ) def test_put_registry_invalid_payload_writes_nothing(state_dir, body, detail_fragment): @@ -364,6 +396,68 @@ def test_put_registry_invalid_payload_writes_nothing(state_dir, body, detail_fra _assert_nothing_written(state_dir) +@pytest.mark.parametrize( + ("body", "received"), + [ + pytest.param(_payload(version=3), 3, id="version-3-not-yet-supported"), + pytest.param({"registry": "nope"}, None, id="missing-version-field"), + ], +) +def test_put_registry_unsupported_version_writes_nothing(state_dir, body, received): + """FR-021: an unknown/missing payload version is rejected with 400 and the + prior mirror left completely intact — no partial application at all.""" + state_dir.unconfigured() + with _client(state_dir) as client: + resp = client.put("/api/v1/setup/registry", json=body, headers=_AUTH) + assert resp.status_code == 400 + assert resp.json() == { + "error": { + "code": "unsupported_payload_version", + "supported": [1, 2], + "received": received, + } + } + _assert_nothing_written(state_dir) + + +def test_put_registry_v2_payload_happy_path(state_dir): + """A v2-shaped payload (registry-file-v2.md hostEntry, no overloaded fields) + is accepted and stored exactly as-is (FR-020).""" + state_dir.write_keypair() + state_dir.write_state_json() + v2_body = { + "version": 2, + "registry": [ + { + "type": "incus", + "name": "dev", + "host": "10.0.0.5", + "user": "remo", + "access": "direct", + }, + { + "type": "aws", + "name": "cloud", + "host": "3.4.5.6", + "user": "remo", + "access": "ssm", + "aws": {"instance_id": "i-0abc", "region": "us-east-1"}, + }, + ], + "host_keys": {"dev": [_VALID_KEY_LINE]}, + } + with _client(state_dir) as client: + resp = client.put("/api/v1/setup/registry", json=v2_body, headers=_AUTH) + assert resp.status_code == 200 + assert resp.json() == { + "applied": True, + "registry_instances": 2, + "host_key_instances": 1, + } + doc = json.loads(state_dir.v2_registry_path.read_text()) + assert doc["hosts"] == _EXPECTED_V2_HOSTS + + # --------------------------------------------------------------------------- # PUT /api/v1/setup/registry — atomicity on mid-apply failure (research R5) # --------------------------------------------------------------------------- @@ -373,16 +467,19 @@ def test_put_registry_mid_apply_failure_is_safe_and_converges(state_dir, monkeyp state_dir.write_keypair() state_dir.write_state_json() - real_write = setup_api._write_lines_atomically + from remo_cli.core import registry as registry_module + + real_write = registry_module._atomic_write_text fail_registry_write = {"active": True} - def flaky_write(path, lines): - # Host-keys file is written first; fail only the registry (second) write. - if fail_registry_write["active"] and path == state_dir.registry_path: + def flaky_write(path, text): + # Service known_hosts is written first (setup.py's own helper, not + # patched here); fail only the registry.json (v2) write. + if fail_registry_write["active"] and path == state_dir.v2_registry_path: raise OSError("disk full") - real_write(path, lines) + real_write(path, text) - monkeypatch.setattr(setup_api, "_write_lines_atomically", flaky_write) + monkeypatch.setattr(registry_module, "_atomic_write_text", flaky_write) with _client(state_dir) as client: resp = client.put("/api/v1/setup/registry", json=_payload(), headers=_AUTH) @@ -391,7 +488,7 @@ def flaky_write(path, lines): # Crash between writes: host keys may exist (documented-safe superset, # apply order R5), but the registry must be untouched/absent. - assert not state_dir.registry_path.exists() + assert not state_dir.v2_registry_path.exists() assert _service_known_hosts(state_dir).read_text() == _VALID_KEY_LINE + "\n" # A subsequent successful push converges to the full mirror. (The @@ -401,7 +498,8 @@ def flaky_write(path, lines): resp = client.put("/api/v1/setup/registry", json=_payload(), headers=_AUTH) assert resp.status_code == 200 assert resp.json()["applied"] is True - assert state_dir.registry_path.read_text() == _EXPECTED_REGISTRY_TEXT + doc = json.loads(state_dir.v2_registry_path.read_text()) + assert doc["hosts"] == _EXPECTED_V2_HOSTS status = client.get("/api/v1/setup/status", headers=_AUTH).json() assert status["state"] == "adopted" diff --git a/tests/unit/web/test_setup_auth.py b/tests/unit/web/test_setup_auth.py index 28b9f37..318a0aa 100644 --- a/tests/unit/web/test_setup_auth.py +++ b/tests/unit/web/test_setup_auth.py @@ -42,7 +42,7 @@ _SETUP_ROUTES_VALID_CODE = [ ("GET", "/api/v1/setup/status", 200), ("GET", "/api/v1/setup/identity", 200), - ("PUT", "/api/v1/setup/registry", 422), # got PAST the gate into domain code + ("PUT", "/api/v1/setup/registry", 400), # got PAST the gate into domain code ("POST", "/api/v1/setup/verify", 200), ] @@ -94,7 +94,7 @@ def test_valid_code_reaches_route_handler(state_dir, method, path, expected): assert resp.status_code == expected assert resp.status_code != 404 if path.endswith("/registry"): - assert resp.json()["reason"] == "invalid_payload" + assert resp.json()["error"]["code"] == "unsupported_payload_version" def test_valid_code_accepts_case_insensitive_scheme(state_dir):