refactor: BottleImages dataclass, prelaunch_checks, build_or_load_images

Addresses review comments 3098, 3099, 3100 on PR #336: - Add BottleImages(agent, sidecar) dataclass to backend/__init__.py. Docker/macOS backends use str image refs; smolmachines uses Path artifacts. Replaces the singular `image` variable from the canonical pattern in comment 3100. - Replace _image_stale_checks/skip_stale with public prelaunch_checks(). CLI now calls backend.prelaunch_checks(plan) before backend.launch(plan); if StaleImageError is raised and the operator confirms, launch proceeds without re-checking. Removes the while-True/skip_stale retry loop. - Add abstract _build_or_load_images(plan) -> BottleImages to BottleBackend. launch() calls it then passes images to _launch_impl. Each backend implements both methods. - Fix comment 3098 (macos-container): _build_images is removed. build_or_load_images() has separate fresh/cached code paths — the cached path never calls a build helper. - Update _start_bundle (smolmachines) to accept sidecar_artifact: Path directly. Sidecar artifact resolution moves to _sidecar_from_path(), called by build_or_load_images alongside _agent_from_path().
2026-07-10 20:25:24 +00:00
parent ecd260b5ef
commit 715e474306
14 changed files with 243 additions and 228 deletions
@@ -276,6 +276,18 @@ PlanT = TypeVar("PlanT", bound=BottlePlan)
 CleanupT = TypeVar("CleanupT", bound=BottleCleanupPlan)


+@dataclass(frozen=True)
+class BottleImages:
+    """Resolved image references (or artifact paths) for a bottle launch.
+
+    For Docker/macOS-container backends, `agent` and `sidecar` are string
+    image refs. For the smolmachines backend they are Path objects pointing
+    to pre-built `.smolmachine` artifacts."""
+
+    agent: "str | Path"
+    sidecar: "str | Path"
+
+
 class BottleBackend(ABC, Generic[PlanT, CleanupT]):
    """Abstract base for selectable bottle backends. Concrete subclasses
    (e.g. DockerBottleBackend) own their own prepare/launch impls.
@@ -435,26 +447,27 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]):
        prompt file, Dockerfile path, and guest home all live on
        `agent_provision_plan` — the source of truth."""

+    def prelaunch_checks(self, plan: PlanT) -> None:
+        """Raise StaleImageError if any cached image used by this plan is stale.
+        No-op default; backends override to call the shared check_stale*
+        helpers on their image/artifact timestamps. Called by the CLI before
+        launch so the operator can be prompted outside the launch context."""
+
    @contextmanager
-    def launch(
-        self, plan: PlanT, *, skip_stale: bool = False
-    ) -> Generator[Bottle, None, None]:
-        """Template: optionally check for stale cached images, then delegate
-        to `_launch_impl`. Pass `skip_stale=True` to bypass the stale check
-        (used by the interactive CLI after the operator confirms)."""
-        if not skip_stale:
-            self._image_stale_checks(plan)
-        with self._launch_impl(plan) as bottle:
+    def launch(self, plan: PlanT) -> Generator[Bottle, None, None]:
+        """Template: build or load images, then delegate to _launch_impl."""
+        images = self._build_or_load_images(plan)
+        with self._launch_impl(plan, images) as bottle:
            yield bottle

-    def _image_stale_checks(self, plan: PlanT) -> None:
-        """Raise StaleImageError if any cached image used by this plan is stale.
-        No-op default; backends override to call the shared `check_stale*`
-        helpers on their image/artifact timestamps."""
+    @abstractmethod
+    def _build_or_load_images(self, plan: PlanT) -> "BottleImages":
+        """Return the agent and sidecar image references (or artifact paths)
+        for this plan, building fresh images when the policy requires it."""

    @abstractmethod
-    def _launch_impl(self, plan: PlanT) -> AbstractContextManager[Bottle]:
-        """Build/run the bottle and yield a handle; tear down on exit."""
+    def _launch_impl(self, plan: PlanT, images: "BottleImages") -> AbstractContextManager[Bottle]:
+        """Bring up the bottle using pre-resolved images; yield a handle; tear down on exit."""

    def provision(self, plan: PlanT, bottle: "Bottle") -> str | None:
        """Copy host-side files (CA cert, prompt, skills, .git) into
@@ -31,7 +31,7 @@ from ...env import ResolvedEnv
 from ...git_gate import GitGatePlan
 from ...supervise import SupervisePlan
 from ...manifest import Manifest
-from .. import ActiveAgent, BottleBackend, BottleSpec
+from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec
 from . import cleanup as _cleanup
 from . import enumerate as _enumerate
 from . import launch as _launch
@@ -85,12 +85,15 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup
            stage_dir=stage_dir,
        )

-    def _image_stale_checks(self, plan: DockerBottlePlan) -> None:
+    def prelaunch_checks(self, plan: DockerBottlePlan) -> None:
        _launch.stale_checks(plan)

+    def _build_or_load_images(self, plan: DockerBottlePlan) -> BottleImages:
+        return _launch.build_or_load_images(plan)
+
    @contextmanager
-    def _launch_impl(self, plan: DockerBottlePlan) -> Generator[DockerBottle, None, None]:
-        with _launch.launch(plan, provision=self.provision) as bottle:
+    def _launch_impl(self, plan: DockerBottlePlan, images: BottleImages) -> Generator[DockerBottle, None, None]:
+        with _launch.launch(plan, images, provision=self.provision) as bottle:
            yield bottle

    def supervise_mcp_url(self, plan: DockerBottlePlan) -> str:
@@ -43,6 +43,7 @@ from ...git_gate import (
 )
 from ...image_cache import check_stale
 from ...log import die, info, warn
+from .. import BottleImages
 from . import network as network_mod
 from . import util as docker_mod
 from .bottle import DockerBottle
@@ -71,16 +72,50 @@ from .sidecar_bundle import SIDECAR_BUNDLE_IMAGE
 _REPO_DIR = str(Path(__file__).resolve().parent.parent.parent.parent)


+def build_or_load_images(plan: DockerBottlePlan) -> BottleImages:
+    """Resolve the agent and sidecar image refs for this plan.
+
+    Returns the committed snapshot if one exists, the local cached images
+    when the policy is 'cached', or builds fresh images and returns those."""
+    committed = read_committed_image(plan.slug)
+    if committed and docker_mod.image_exists(committed):
+        info(f"using committed image {committed!r}")
+        return BottleImages(agent=committed, sidecar=SIDECAR_BUNDLE_IMAGE)
+    if plan.spec.image_policy == "cached":
+        if not docker_mod.image_exists(plan.image):
+            die(
+                f"cached agent image {plan.image!r} not found; "
+                "run without --cached-images to build it"
+            )
+        if not docker_mod.image_exists(SIDECAR_BUNDLE_IMAGE):
+            die(
+                f"cached sidecar image {SIDECAR_BUNDLE_IMAGE!r} not found; "
+                "run without --cached-images to build it"
+            )
+        info(f"using cached agent image {plan.image!r}")
+        info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
+        return BottleImages(agent=plan.image, sidecar=SIDECAR_BUNDLE_IMAGE)
+    docker_mod.build_image(plan.image, _REPO_DIR, dockerfile=plan.dockerfile_path)
+    return BottleImages(agent=plan.image, sidecar=SIDECAR_BUNDLE_IMAGE)
+
+
@contextmanager
 def launch(
    plan: DockerBottlePlan,
+    images: BottleImages,
    *,
    provision: Callable[[DockerBottlePlan, "DockerBottle"], str | None],
 ) -> Generator[DockerBottle, None, None]:
-    """Build, launch, and provision a Docker bottle via compose.
-    Teardown on exit."""
+    """Launch and provision a Docker bottle via compose. Teardown on exit."""
    stack = ExitStack()

+    # Stamp the resolved agent image ref into the plan so compose rendering
+    # picks up the right image (may be a committed snapshot or cached ref).
+    plan = dataclasses.replace(
+        plan,
+        agent_provision=dataclasses.replace(plan.agent_provision, image=str(images.agent)),
+    )
+
    _bottle_for_revoke = plan.manifest.bottle
    _git_gate_dir_for_revoke = git_gate_state_dir(plan.slug)

@@ -97,36 +132,6 @@ def launch(
        )

    try:
-        # Step 1: agent image. Use a committed snapshot when one exists
-        # and is present in the local daemon; otherwise build from the
-        # Dockerfile. Sidecar images get built lazily by `docker compose
-        # up` via the renderer's `build:` directives.
-        committed = read_committed_image(plan.slug)
-        cached_policy = plan.spec.image_policy == "cached"
-        if committed and docker_mod.image_exists(committed):
-            info(f"using committed image {committed!r}")
-            plan = dataclasses.replace(
-                plan,
-                agent_provision=dataclasses.replace(plan.agent_provision, image=committed),
-            )
-        elif cached_policy:
-            if not docker_mod.image_exists(plan.image):
-                die(
-                    f"cached agent image {plan.image!r} not found; "
-                    "run without --cached-images to build it"
-                )
-            if not docker_mod.image_exists(SIDECAR_BUNDLE_IMAGE):
-                die(
-                    f"cached sidecar image {SIDECAR_BUNDLE_IMAGE!r} not found; "
-                    "run without --cached-images to build it"
-                )
-            info(f"using cached agent image {plan.image!r}")
-            info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
-        else:
-            docker_mod.build_image(
-                plan.image, _REPO_DIR,
-                dockerfile=plan.dockerfile_path,
-            )

        internal_network = network_mod.network_name_for_slug(plan.slug)
        egress_network = network_mod.network_egress_name_for_slug(plan.slug)
@@ -12,7 +12,7 @@ from ...env import ResolvedEnv
 from ...git_gate import GitGatePlan
 from ...supervise import SupervisePlan
 from ...manifest import Manifest
-from .. import ActiveAgent, BottleBackend, BottleSpec
+from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec
 from . import cleanup as _cleanup
 from . import enumerate as _enumerate
 from . import launch as _launch
@@ -67,14 +67,17 @@ class MacosContainerBottleBackend(
            stage_dir=stage_dir,
        )

-    def _image_stale_checks(self, plan: MacosContainerBottlePlan) -> None:
+    def prelaunch_checks(self, plan: MacosContainerBottlePlan) -> None:
        _launch.stale_checks(plan)

+    def _build_or_load_images(self, plan: MacosContainerBottlePlan) -> BottleImages:
+        return _launch.build_or_load_images(plan)
+
    @contextmanager
    def _launch_impl(
-        self, plan: MacosContainerBottlePlan
+        self, plan: MacosContainerBottlePlan, images: BottleImages
    ) -> Generator[MacosContainerBottle, None, None]:
-        with _launch.launch(plan, provision=self.provision) as bottle:
+        with _launch.launch(plan, images, provision=self.provision) as bottle:
            yield bottle

    def prepare_cleanup(self) -> MacosContainerBottleCleanupPlan:
@@ -34,6 +34,7 @@ from ...git_gate import (
 )
 from ...image_cache import check_stale
 from ...log import die, info, warn
+from .. import BottleImages
 from ...supervise import DB_PATH_IN_CONTAINER, SUPERVISE_PORT
 from ...util import expand_tilde
 from ..docker.egress import EGRESS_CA_IN_CONTAINER, EGRESS_PORT
@@ -72,17 +73,51 @@ def sidecar_container_name(slug: str) -> str:
    return f"bot-bottle-sidecars-{slug}"


+def build_or_load_images(plan: MacosContainerBottlePlan) -> BottleImages:
+    """Resolve agent and sidecar image refs. Builds the sidecar when needed,
+    but never builds the agent when policy is 'cached'."""
+    committed = read_committed_image(plan.slug)
+    if committed and container_mod.image_exists(committed):
+        info(f"using committed image {committed!r}")
+        if plan.spec.image_policy != "cached":
+            container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
+        return BottleImages(agent=committed, sidecar=SIDECAR_BUNDLE_IMAGE)
+    if plan.spec.image_policy == "cached":
+        if not container_mod.image_exists(plan.image):
+            die(
+                f"cached agent image {plan.image!r} not found; "
+                "run without --cached-images to build it"
+            )
+        if not container_mod.image_exists(SIDECAR_BUNDLE_IMAGE):
+            die(
+                f"cached sidecar image {SIDECAR_BUNDLE_IMAGE!r} not found; "
+                "run without --cached-images to build it"
+            )
+        info(f"using cached agent image {plan.image!r}")
+        info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
+        return BottleImages(agent=plan.image, sidecar=SIDECAR_BUNDLE_IMAGE)
+    container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
+    container_mod.build_image(plan.image, _REPO_DIR, dockerfile=plan.dockerfile_path)
+    return BottleImages(agent=plan.image, sidecar=SIDECAR_BUNDLE_IMAGE)
+
+
@contextmanager
 def launch(
    plan: MacosContainerBottlePlan,
+    images: BottleImages,
    *,
    provision: Callable[[MacosContainerBottlePlan, "MacosContainerBottle"], str | None],
 ) -> Generator[MacosContainerBottle, None, None]:
-    """Build, run, provision, and yield an Apple Container bottle."""
+    """Run, provision, and yield an Apple Container bottle."""
    stack = ExitStack()
    bottle_for_revoke = plan.manifest.bottle
    git_gate_dir_for_revoke = git_gate_state_dir(plan.slug)

+    plan = dataclasses.replace(
+        plan,
+        agent_provision=dataclasses.replace(plan.agent_provision, image=str(images.agent)),
+    )
+
    def teardown() -> None:
        teardown_exc: BaseException | None = None
        try:
@@ -96,7 +131,6 @@ def launch(

    try:
        plan = _mint_certs(plan)
-        plan = _build_images(plan)

        internal_network = internal_network_name(plan.slug)
        egress_network = egress_network_name(plan.slug)
@@ -149,35 +183,6 @@ def _mint_certs(plan: MacosContainerBottlePlan) -> MacosContainerBottlePlan:
    return dataclasses.replace(plan, egress_plan=egress_plan)


-def _build_images(plan: MacosContainerBottlePlan) -> MacosContainerBottlePlan:
-    cached = plan.spec.image_policy == "cached"
-    committed = read_committed_image(plan.slug)
-    if committed and container_mod.image_exists(committed):
-        info(f"using committed image {committed!r}")
-        if not cached:
-            container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
-        return dataclasses.replace(
-            plan,
-            agent_provision=dataclasses.replace(plan.agent_provision, image=committed),
-        )
-    if cached:
-        if not container_mod.image_exists(plan.image):
-            die(
-                f"cached agent image {plan.image!r} not found; "
-                "run without --cached-images to build it"
-            )
-        if not container_mod.image_exists(SIDECAR_BUNDLE_IMAGE):
-            die(
-                f"cached sidecar image {SIDECAR_BUNDLE_IMAGE!r} not found; "
-                "run without --cached-images to build it"
-            )
-        info(f"using cached agent image {plan.image!r}")
-        info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
-        return plan
-    container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
-    container_mod.build_image(plan.image, _REPO_DIR, dockerfile=plan.dockerfile_path)
-    return plan
-

 def stale_checks(plan: MacosContainerBottlePlan) -> None:
    """Raise StaleImageError if a cached image is older than the configured
@@ -19,7 +19,7 @@ from ...env import ResolvedEnv
 from ...git_gate import GitGatePlan
 from ...supervise import SupervisePlan
 from ...manifest import Manifest
-from .. import ActiveAgent, BottleBackend, BottleSpec
+from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec
 from . import cleanup as _cleanup
 from . import enumerate as _enumerate
 from . import launch as _launch
@@ -77,14 +77,17 @@ class SmolmachinesBottleBackend(
            stage_dir=stage_dir,
        )

-    def _image_stale_checks(self, plan: SmolmachinesBottlePlan) -> None:
+    def prelaunch_checks(self, plan: SmolmachinesBottlePlan) -> None:
        _launch.stale_checks(plan)

+    def _build_or_load_images(self, plan: SmolmachinesBottlePlan) -> BottleImages:
+        return _launch.build_or_load_images(plan)
+
    @contextmanager
    def _launch_impl(
-        self, plan: SmolmachinesBottlePlan
+        self, plan: SmolmachinesBottlePlan, images: BottleImages
    ) -> Generator[SmolmachinesBottle, None, None]:
-        with _launch.launch(plan, provision=self.provision) as bottle:
+        with _launch.launch(plan, images, provision=self.provision) as bottle:
            yield bottle

    def supervise_mcp_url(self, plan: SmolmachinesBottlePlan) -> str:
@@ -42,6 +42,7 @@ from ...bottle_state import (
    git_gate_state_dir,
    read_committed_image,
 )
+from .. import BottleImages
 from . import loopback_alias as _loopback
 from . import port_forward as _forward
 from . import sidecar_bundle as _bundle
@@ -81,13 +82,32 @@ _GIT_HTTP_PORT = 9420
 _SUPERVISE_PORT = SUPERVISE_PORT


+def build_or_load_images(plan: SmolmachinesBottlePlan) -> BottleImages:
+    """Return pre-built or freshly built smolmachine artifact paths."""
+    return BottleImages(
+        agent=_agent_from_path(plan),
+        sidecar=_sidecar_from_path(plan),
+    )
+
+
+def _sidecar_from_path(plan: SmolmachinesBottlePlan) -> Path:
+    """Return the sidecar bundle artifact path, building it if needed."""
+    if _image_policy(plan) == "cached":
+        return _cached_smolmachine(_bundle.SIDECAR_BUNDLE_IMAGE, label="sidecar")
+    return _ensure_smolmachine(
+        _bundle.SIDECAR_BUNDLE_IMAGE,
+        dockerfile=_bundle.SIDECAR_BUNDLE_DOCKERFILE,
+    )
+
+
@contextmanager
 def launch(
    plan: SmolmachinesBottlePlan,
+    images: BottleImages,
    *,
    provision: Callable[[SmolmachinesBottlePlan, "SmolmachinesBottle"], str | None],
 ) -> Generator[SmolmachinesBottle, None, None]:
-    """Build + run the bottle and yield a handle; tear everything
+    """Run the bottle from pre-built images and yield a handle; tear everything
    down on exit. Errors during bringup unwind any partial state
    via the ExitStack."""
    stack = ExitStack()
@@ -95,11 +115,9 @@ def launch(
        loopback_ip, network = _allocate_resources(plan, stack)
        plan = _mint_certs(plan)
        proxy_host = loopback_ip
-        plan = _start_bundle(plan, network, proxy_host, stack)
+        plan = _start_bundle(plan, network, proxy_host, Path(images.sidecar), stack)

-        agent_from_path = _agent_from_path(plan)
-
-        _launch_vm(plan, agent_from_path, proxy_host, stack)
+        _launch_vm(plan, Path(images.agent), proxy_host, stack)
        _init_vm(plan)

        bottle = SmolmachinesBottle(
@@ -176,24 +194,18 @@ def _start_bundle(
    plan: SmolmachinesBottlePlan,
    network: str,
    proxy_host: str,
+    sidecar_artifact: Path,
    stack: ExitStack,
 ) -> SmolmachinesBottlePlan:
-    """Build the BundleLaunchSpec, start the sidecar VM, wrap its raw
-    smolVM-published loopback ports with per-bottle forwarders, stamp
-    agent URLs from those forwarder ports, and register teardown."""
+    """Build the BundleLaunchSpec, start the sidecar VM from the pre-resolved
+    artifact, wrap its raw smolVM-published loopback ports with per-bottle
+    forwarders, stamp agent URLs from those forwarder ports, and register teardown."""
    plan = _provision_git_gate_keys(plan)
    bundle_spec = _bundle_launch_spec(plan, network, proxy_host)
    token_env = _resolve_token_env(plan, dict(os.environ))
-    if _image_policy(plan) == "cached":
-        artifact = _cached_smolmachine(bundle_spec.image, label="sidecar")
-    else:
-        artifact = _ensure_smolmachine(
-            bundle_spec.image,
-            dockerfile=_bundle.SIDECAR_BUNDLE_DOCKERFILE,
-        )
    launch = _bundle.start_bundle_vm(
        bundle_spec,
-        from_path=artifact,
+        from_path=sidecar_artifact,
        host_env={**os.environ, **token_env},
    )
    stack.callback(_bundle.stop_bundle_vm, plan.slug)