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 <noreply@anthropic.com>
This commit is contained in:
@@ -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]:
|
||||
|
||||
@@ -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 <name>'"
|
||||
)
|
||||
|
||||
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)
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user