From 339d40f8c9bee47a62b08d9aca0857588bf8be6a Mon Sep 17 00:00:00 2001 From: didericis Date: Tue, 12 May 2026 10:51:19 -0400 Subject: [PATCH] refactor(backend): lift host-side validation onto the base class Make BottleBackend.prepare a template method that runs a cross-backend _validate step (agent exists, named skills present on host, SSH IdentityFiles resolve) and then delegates to a subclass-implemented _resolve_plan for backend-specific resolution. A future backend that overrides _resolve_plan can no longer forget to validate skills or SSH keys; the validation runs unconditionally via prepare. Backends with additional preconditions can override _validate and chain via super(). Co-Authored-By: Claude Opus 4.7 --- claude_bottle/backend/__init__.py | 55 +++++++++++++++++-- claude_bottle/backend/docker/backend.py | 55 ++++--------------- .../backend/docker/provision/skills.py | 10 ++-- 3 files changed, 66 insertions(+), 54 deletions(-) diff --git a/claude_bottle/backend/__init__.py b/claude_bottle/backend/__init__.py index dc70223..ef9fd4c 100644 --- a/claude_bottle/backend/__init__.py +++ b/claude_bottle/backend/__init__.py @@ -34,10 +34,12 @@ from abc import ABC, abstractmethod from contextlib import AbstractContextManager from dataclasses import dataclass from pathlib import Path -from typing import Any, Generic, TypeVar +from typing import Any, Generic, Sequence, TypeVar from ..log import die -from ..manifest import Manifest +from ..manifest import Manifest, SshEntry +from ..util import expand_tilde +from .util import host_skill_dir @dataclass(frozen=True) @@ -127,10 +129,53 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]): name: str - @abstractmethod def prepare(self, spec: BottleSpec, *, stage_dir: Path) -> PlanT: - """Resolve names, validate host-side prerequisites, write - scratch files. No remote/runtime resources created yet.""" + """Template method: run cross-backend host-side validation, then + delegate to the subclass's `_resolve_plan` for the + backend-specific resolution (names, scratch files, etc.). The + validation step is enforced here so a future backend cannot + accidentally skip it. No remote/runtime resources are created.""" + self._validate(spec) + return self._resolve_plan(spec, stage_dir=stage_dir) + + def _validate(self, spec: BottleSpec) -> None: + """Cross-backend pre-launch checks. Confirms the agent exists, + the named skills are present on the host, and every SSH + IdentityFile resolves. Subclasses with additional preconditions + should override and call `super()._validate(spec)` first.""" + manifest = spec.manifest + manifest.require_agent(spec.agent_name) + agent = manifest.agents[spec.agent_name] + bottle = manifest.bottle_for(spec.agent_name) + self._validate_skills(agent.skills) + self._validate_ssh_entries(bottle.ssh) + + def _validate_skills(self, skills: Sequence[str]) -> None: + """Each named skill must be a directory under the host's + `~/.claude/skills/`. The check is purely host-side, so the + default impl covers every backend.""" + for name in skills: + path = host_skill_dir(name) + if not os.path.isdir(path): + die( + f"skill '{name}' not found on host at {path}. " + f"Create it under ~/.claude/skills/, then re-run." + ) + + def _validate_ssh_entries(self, entries: Sequence[SshEntry]) -> None: + """Each entry's IdentityFile must exist on the host (after + expanding leading ~). Shape is already enforced by Manifest + validation; this only checks file presence.""" + for entry in entries: + key = expand_tilde(entry.IdentityFile) + if not os.path.isfile(key): + die(f"ssh key file not found for host '{entry.Host}': {key}") + + @abstractmethod + def _resolve_plan(self, spec: BottleSpec, *, stage_dir: Path) -> PlanT: + """Backend-specific plan resolution: image/container names, + env-file, prompt-file, proxy plan, runtime detection. Called by + `prepare` after `_validate` succeeds.""" @abstractmethod def launch(self, plan: PlanT) -> AbstractContextManager[Bottle]: diff --git a/claude_bottle/backend/docker/backend.py b/claude_bottle/backend/docker/backend.py index 9f2d3c7..4c64fd2 100644 --- a/claude_bottle/backend/docker/backend.py +++ b/claude_bottle/backend/docker/backend.py @@ -1,11 +1,9 @@ """DockerBottleBackend — the Docker implementation of BottleBackend. -Methods: - .prepare(spec, stage_dir=...) -> DockerBottlePlan - .launch(plan) -> ContextManager[DockerBottle] - .prepare_cleanup() -> DockerBottleCleanupPlan - .cleanup(plan) -> None - .list_active() -> None +The base class's `prepare` template runs cross-backend host-side +validation, then calls this module's `_resolve_plan` for the Docker- +specific resolution. Other public methods are backend-implemented as +declared on `BottleBackend`. """ from __future__ import annotations @@ -16,15 +14,12 @@ import subprocess import sys from contextlib import ExitStack, contextmanager from pathlib import Path -from typing import Generator, Sequence +from typing import Generator from ... import pipelock from ...env import ResolvedEnv, resolve_env from ...log import die, info -from ...manifest import SshEntry -from ...util import expand_tilde from .. import BottleBackend, BottleSpec -from ..util import host_skill_dir from . import network as network_mod from . import util as docker_mod from .bottle import DockerBottle @@ -63,14 +58,15 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup def __init__(self) -> None: self._proxy = DockerPipelockProxy() - def prepare(self, spec: BottleSpec, *, stage_dir: Path) -> DockerBottlePlan: - """Resolve names, validate, write scratch files. No Docker - resources are created; the only side effects are host-side - files under stage_dir and a probe of `docker info`.""" + def _resolve_plan(self, spec: BottleSpec, *, stage_dir: Path) -> DockerBottlePlan: + """Resolve Docker-specific names, write scratch files. No + Docker resources are created; the only side effects are + host-side files under stage_dir and a probe of `docker info`. + Cross-backend validation has already run via the base class's + `prepare` template.""" docker_mod.require_docker() manifest = spec.manifest - manifest.require_agent(spec.agent_name) agent = manifest.agents[spec.agent_name] bottle = manifest.bottle_for(spec.agent_name) @@ -109,11 +105,6 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup f"clean up old containers with 'docker rm -f '" ) - if agent.skills: - self.validate_skills(list(agent.skills)) - if bottle.ssh: - self.validate_ssh_entries(bottle.ssh) - env_file = stage_dir / "agent.env" prompt_file = stage_dir / "prompt.txt" prompt_file.write_text("") @@ -266,33 +257,9 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup def provision_prompt(self, plan: DockerBottlePlan, target: str) -> str | None: return _prompt.provision_prompt(plan, target) - def validate_skills(self, skills: list[str]) -> None: - """Fail loudly if any named skill is missing from the host's - ~/.claude/skills/. Called from `prepare` before the y/N so the - user doesn't get a launch prompt for a plan that's already - known to break.""" - for name in skills: - path = host_skill_dir(name) - if not os.path.isdir(path): - die( - f"skill '{name}' not found on host at {path}. " - f"Create it under ~/.claude/skills/, then re-run." - ) - def provision_skills(self, plan: DockerBottlePlan, target: str) -> None: _skills.provision_skills(plan, target) - def validate_ssh_entries(self, entries: Sequence[SshEntry]) -> None: - """Each entry's IdentityFile must exist on the host (after - expanding leading ~). Host and IdentityFile shape are already - enforced by Manifest validation. Called from `prepare` before - the y/N so the user doesn't get prompted for a plan with a - missing key.""" - for entry in entries: - key = expand_tilde(entry.IdentityFile) - if not os.path.isfile(key): - die(f"ssh key file not found for host '{entry.Host}': {key}") - def provision_ssh(self, plan: DockerBottlePlan, target: str) -> None: _ssh.provision_ssh(plan, target) diff --git a/claude_bottle/backend/docker/provision/skills.py b/claude_bottle/backend/docker/provision/skills.py index 410b76e..63c2a8d 100644 --- a/claude_bottle/backend/docker/provision/skills.py +++ b/claude_bottle/backend/docker/provision/skills.py @@ -1,10 +1,10 @@ """Copy host-side skill directories into a running Docker bottle. -Skills are validated on the host before launch by -`DockerBottleBackend.validate_skills`; this module assumes that -validation has already run. A skill disappearing between validation -and copy still dies loudly rather than silently producing a partial -container.""" +Skills are validated on the host before launch by the base class's +`BottleBackend._validate_skills` (called from `prepare`); this module +assumes that validation has already run. A skill disappearing between +validation and copy still dies loudly rather than silently producing +a partial container.""" from __future__ import annotations