From 5c17b7ded092bb6c0eaf6a9196bff0f087b7eac3 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 28 Oct 2026 18:55:49 -0700 Subject: [PATCH 1/4] fix(gateway): write the compose file as data and refuse names it can't hold A gateway deploy wrote docker-compose.yml and init-schemas.sql through quoted heredocs and spliced each env's environment_name and image reference into them, and the snapshot reload passed the names to `docker compose up` unquoted. A name holding the heredoc terminator, a newline or a shell metacharacter ran as shell on the VM host (the operator's machine on the local provider) or rewrote the compose file. - Both files are written as one shlex-quoted printf argument (utils/shell.write_file_script), keeping the exec count and retries; the snapshot reload quotes each service name. - utils/names checks an environment name against Compose's service-name characters with a letter or digit first, and an image against Docker's reference grammar (without bracketed IPv6 registries, which an unquoted compose value reads as a YAML list). - The names are checked when written (EnvStore.put_document through a new Env.put_problem hook, DockerImageArtifact.put_tar/put_ref, bundle env.toml authoring) and again at deploy (the gateway topology, the compose renderer and the local_postgres state services). from_dict is unchanged, so stored documents still load. - Includes the LocalSandbox import fix in test_artifact_staging.py that main needs after the provider move. Co-Authored-By: Claude Opus 5.5 --- .../artifact/artifacts/docker_image.py | 5 + src/agent_env/bundle/authoring.py | 4 +- src/agent_env/env/env.py | 5 + src/agent_env/env/envs/mcp_server.py | 4 + src/agent_env/env/envs/multi_env.py | 9 +- src/agent_env/env/envs/website.py | 4 + src/agent_env/env/store.py | 2 + .../env_providers/env_gateway_provider.py | 32 +++-- .../providers/env_state/local_postgres.py | 14 ++- src/agent_env/utils/names.py | 35 ++++++ src/agent_env/utils/shell.py | 8 ++ tst/unit/artifact/test_docker_image_pulled.py | 6 + .../artifact/test_docker_image_put_ref.py | 10 ++ tst/unit/bundle/env_toml_test.py | 9 +- tst/unit/env/envs/test_artifact_staging.py | 2 +- .../env/envs/test_environment_name_alias.py | 19 +++ .../envs/test_mcp_server_container_mode.py | 2 +- .../test_multi_env_snapshot_reload_name.py | 27 +++++ tst/unit/env/envs/test_multi_env_vm_sizing.py | 7 +- .../env_gateway_provider_test.py | 113 ++++++++++++++++++ 20 files changed, 292 insertions(+), 25 deletions(-) create mode 100644 src/agent_env/utils/names.py create mode 100644 src/agent_env/utils/shell.py diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index fd64c9d68..6888174d2 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/src/agent_env/artifact/artifacts/docker_image.py @@ -25,6 +25,7 @@ from agent_env.store.image_store.registry_api import NoSuchImageError, pin_digest, pinned_from from agent_env.utils.docker.context import BuildContext from agent_env.utils.docker.build import DEFAULT_BUILD_PLATFORM, build_image, normalize_platform +from agent_env.utils.names import image_reference_problem logger = logging.getLogger(__name__) @@ -193,6 +194,8 @@ def put_tar( owner = "DockerImageArtifact.put_tar" if not tar_gz_object_url: raise ValueError(f"{owner}(): tar_gz_object_url is empty") + if problem := image_reference_problem(image_name): + raise ValueError(f"{owner}(): {problem}") for url in (tar_gz_object_url, build_context_object_url): if url: get_config().check_object_url(id, url) @@ -223,6 +226,8 @@ def put_ref(cls, id: str, *, description: str, image_name: str, earlier_pin: str from agent_env.artifact.store import get_artifact_store from agent_env.config import get_config + if problem := image_reference_problem(image_name): + raise ValueError(problem) if not names_registry(image_name): raise ValueError(f"{image_name!r} doesn't name its registry; spell it out, as in " f"docker.io/library/{image_name}") diff --git a/src/agent_env/bundle/authoring.py b/src/agent_env/bundle/authoring.py index bca0fb3cc..6ddcb2337 100644 --- a/src/agent_env/bundle/authoring.py +++ b/src/agent_env/bundle/authoring.py @@ -22,7 +22,7 @@ from agent_env.providers.env_providers.env_gateway_provider import EnvironmentGatewayProvider from agent_env.providers.env_providers.env_provider import _env_provider_class from agent_env.utils.card_naming import card_names_in_files - +from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import os_reason from ._fs import relative, show, with_article @@ -174,6 +174,8 @@ def accepted_env(self, data: dict, env_class: type[Env], *, problems.append(self.config_problem(problem)) elif name is not None: fields["environment_name"] = name + if name_problem := environment_name_problem(name): + problems.append(self.config_problem(name_problem)) if (name in GATEWAY_SERVICE_NAMES and provider is not None and issubclass(provider, EnvironmentGatewayProvider)): problems.append(self.config_problem( diff --git a/src/agent_env/env/env.py b/src/agent_env/env/env.py index c92199f7f..713a83599 100644 --- a/src/agent_env/env/env.py +++ b/src/agent_env/env/env.py @@ -336,6 +336,11 @@ def merge_metadata(self, updates: dict[str, Any], retries: int = 3) -> None: def to_dict(self) -> dict[str, Any]: return {"id": self.id, "type": self.type, "version": self.version, "metadata": self.metadata} + def put_problem(self) -> str | None: + """Why the store refuses to write this env, None when it doesn't. Checked on write only, so a document already + stored still loads.""" + return None + @classmethod def from_dict(cls, data: dict[str, Any]) -> "Env": raise NotImplementedError(f"{cls.__name__} must implement from_dict") diff --git a/src/agent_env/env/envs/mcp_server.py b/src/agent_env/env/envs/mcp_server.py index 29104edae..a2c3b7bc1 100644 --- a/src/agent_env/env/envs/mcp_server.py +++ b/src/agent_env/env/envs/mcp_server.py @@ -25,6 +25,7 @@ from agent_env.env.gateway import GatewayMode from agent_env.providers.sandbox_providers.sandbox_provider import SANDBOX_MODE_CONTAINER from agent_env.attribution import Attribution +from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: @@ -73,6 +74,9 @@ def to_dict(self) -> dict: base["env_provider_type"] = self.env_provider_type return base + def put_problem(self) -> str | None: + return environment_name_problem(self.environment_name) + @classmethod def from_dict(cls, data: dict) -> MCPServerEnv: artifact_ref = data["docker_image_artifact"] diff --git a/src/agent_env/env/envs/multi_env.py b/src/agent_env/env/envs/multi_env.py index b64be5552..8b5bd3928 100644 --- a/src/agent_env/env/envs/multi_env.py +++ b/src/agent_env/env/envs/multi_env.py @@ -3,6 +3,7 @@ import logging import os +import shlex from typing import TYPE_CHECKING, Any, Callable, ClassVar, Optional logger = logging.getLogger(__name__) @@ -43,6 +44,7 @@ def _snapshot_after_load_default() -> bool: from agent_env.store.ids import derive_id from agent_env.providers.sandbox_providers.sandbox_provider import SANDBOX_MODE_CONTAINER from agent_env.attribution import Attribution +from agent_env.utils.shell import write_file_script class MultiEnv(Env): @@ -635,10 +637,7 @@ async def _load_from_snapshot(self, snapshot) -> None: compose_content = compose_content.replace( " volumes:\n - ./init-schemas.sql:/docker-entrypoint-initdb.d/init-schemas.sql\n", "", ) - write_script = f'''cat > {DOCKER_COMPOSE_PATH} << 'COMPOSE_EOF' -{compose_content} -COMPOSE_EOF''' - await self._sandbox.exec_script(write_script) + await self._sandbox.exec_script(write_file_script(DOCKER_COMPOSE_PATH, compose_content)) # Remove old servicedb container and its anonymous volume, then start fresh from snapshot image logger.info("Recreating servicedb container with snapshot image...") @@ -668,7 +667,7 @@ async def _load_from_snapshot(self, snapshot) -> None: environment_names = list(dict.fromkeys( [env.environment_name for env in self.mcp_server_envs] + [env.environment_name for env in self.website_envs] )) - svc_list = " ".join(environment_names + [GATEWAY_SERVICE_NAME, PGWEB_SERVICE_NAME, DB_MCP_SERVICE_NAME]) + svc_list = " ".join(map(shlex.quote, environment_names + [GATEWAY_SERVICE_NAME, PGWEB_SERVICE_NAME, DB_MCP_SERVICE_NAME])) logger.info(f"Recreating services: {svc_list}") await gw._compose_up(self._sandbox, f"--force-recreate {svc_list}") await gw._wait_for_gateway(self._sandbox, self._sandbox.host_port(AGENT_ENV_GATEWAY_MCP_PORT)) diff --git a/src/agent_env/env/envs/website.py b/src/agent_env/env/envs/website.py index 622c07133..8eb9a3eed 100644 --- a/src/agent_env/env/envs/website.py +++ b/src/agent_env/env/envs/website.py @@ -23,6 +23,7 @@ ) from agent_env.env.gateway import GatewayMode from agent_env.attribution import Attribution +from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: from agent_env.bundle.authoring import AuthoringContext @@ -87,6 +88,9 @@ def to_dict(self) -> dict: base["env_provider_type"] = self.env_provider_type return base + def put_problem(self) -> str | None: + return environment_name_problem(self.environment_name) + @classmethod def from_dict(cls, data: dict) -> WebsiteEnv: backend_ref = data["backend_docker_image_artifact"] diff --git a/src/agent_env/env/store.py b/src/agent_env/env/store.py index ccdf265f3..afa5d5b49 100644 --- a/src/agent_env/env/store.py +++ b/src/agent_env/env/store.py @@ -95,6 +95,8 @@ def next_version(self, id: str) -> int: return self._versioned.next_version(id) def put_document(self, env: _E) -> _E: + if problem := env.put_problem(): + raise ValueError(f"Can't store env {env.id!r}: {problem}") store = self._doc_store existing = store.find_one( ENVS_COLLECTION, Filter.of(id=env.id), sort=Sort.by("version", descending=True) diff --git a/src/agent_env/providers/env_providers/env_gateway_provider.py b/src/agent_env/providers/env_providers/env_gateway_provider.py index e6fb04717..5aa5cf662 100644 --- a/src/agent_env/providers/env_providers/env_gateway_provider.py +++ b/src/agent_env/providers/env_providers/env_gateway_provider.py @@ -57,6 +57,8 @@ Accepts, SandboxProvider, ) +from agent_env.utils.names import environment_name_problem, image_reference_problem +from agent_env.utils.shell import write_file_script if TYPE_CHECKING: from collections.abc import Awaitable, Callable @@ -367,6 +369,11 @@ def create_docker_compose( env-network: driver: bridge """ + problems = _compose_name_problems(mcp_servers, website_configs, sidecars, images=(gateway_image,)) + if mcp_server_name and ("\n" in mcp_server_name or "\r" in mcp_server_name): + problems.append(f"MCP server name {mcp_server_name!r} must be one line") + if problems: + raise ValueError(f"Can't render the gateway's docker-compose.yml: {'; '.join(problems)}") lines = ["services:"] # MCP servers and website backends are both DB clients (one schema each). @@ -587,6 +594,8 @@ async def deploy( from agent_env.providers.env_state import LOCAL_POSTGRES_STATE_TYPE, acquire_state_for_deploy topology = _gateway_topology(env) + if problems := _compose_name_problems(topology.mcp_servers, topology.website_configs): + raise ValueError(f"Can't deploy {env.id!r} behind a gateway: {'; '.join(problems)}") state_instance = await acquire_state_for_deploy( env_state_type=env_state_type, ttl_seconds=ttl_seconds, name_hint=env.id, env_state_instance_id=env_state_instance_id, ) @@ -816,17 +825,9 @@ async def _stand_up(_spec): if self._needs_local_postgres: init_script = local_store_spec.init_sql logger.info(f"Generated init-schemas.sql:\n{init_script}") - init_script_path = f"{GATEWAY_APP_DIR}/init-schemas.sql" - write_init_script = f'''cat > {init_script_path} << 'INIT_EOF' -{init_script} -INIT_EOF''' - await sandbox.exec_script(write_init_script, max_retries=2) - - # Write docker-compose.yml - write_compose_script = f'''cat > {DOCKER_COMPOSE_PATH} << 'COMPOSE_EOF' -{compose_content} -COMPOSE_EOF''' - await sandbox.exec_script(write_compose_script, max_retries=2) + await sandbox.exec_script(write_file_script(f"{GATEWAY_APP_DIR}/init-schemas.sql", init_script), max_retries=2) + + await sandbox.exec_script(write_file_script(DOCKER_COMPOSE_PATH, compose_content), max_retries=2) # Start MCP services with docker compose logger.info("Starting MCP services...") @@ -1501,6 +1502,15 @@ def _gateway_topology(env: Env) -> _GatewayTopology: ) +def _compose_name_problems(mcp_servers: list[MCPServerConfig], website_configs: list[WebsiteConfig] | None, + sidecars: list[SidecarConfig] | None = None, *, images: tuple[str, ...] = ()) -> list[str]: + """Why the environment names and images of a gateway deploy can't go into its compose file and shell lines.""" + names = [c.environment_name for c in [*mcp_servers, *(website_configs or []), *(sidecars or [])]] + refs = [*images, *(s.image for s in mcp_servers), *(s.image for s in sidecars or []), + *(ref for w in website_configs or [] for ref in (w.backend_image, w.frontend_image))] + return [problem for problem in [*map(environment_name_problem, names), *map(image_reference_problem, refs)] if problem] + + # Compose `environment:` keys whose values are secrets and must be masked before the # rendered compose is logged. Matches the key to the left of the first '=' on an # ` - KEY=VALUE` env line. diff --git a/src/agent_env/providers/env_state/local_postgres.py b/src/agent_env/providers/env_state/local_postgres.py index 45c348d23..b962c625f 100644 --- a/src/agent_env/providers/env_state/local_postgres.py +++ b/src/agent_env/providers/env_state/local_postgres.py @@ -38,6 +38,7 @@ StateContext, LOCAL_POSTGRES_STATE_TYPE, ) +from agent_env.utils.names import image_reference_problem if TYPE_CHECKING: from collections.abc import Awaitable, Callable @@ -234,7 +235,7 @@ def render_healthcheck_service( hc = spec.healthcheck return [ f" {self.env_state_docker_service_name()}:", - f" image: {cfg.db_image}", + f" image: {_checked_image(cfg.db_image)}", " environment:", *[f" - {key}={value}" for key, value in spec.env.items()], " healthcheck:", @@ -266,7 +267,7 @@ def render_sidecar_containers( if cfg.db_web_image: lines.extend([ f" {PGWEB_SERVICE_NAME}:", - f" image: {cfg.db_web_image}", + f" image: {_checked_image(cfg.db_web_image)}", " restart: unless-stopped", " depends_on:", f" {DATABASE_SERVICE_NAME}:", @@ -286,7 +287,7 @@ def render_sidecar_containers( if cfg.db_mcp_image: lines.extend([ f" {DB_MCP_SERVICE_NAME}:", - f" image: {cfg.db_mcp_image}", + f" image: {_checked_image(cfg.db_mcp_image)}", " restart: unless-stopped", " depends_on:", f" {DATABASE_SERVICE_NAME}:", @@ -394,3 +395,10 @@ async def install_changelog_triggers( exit_code, _out, err = await store_exec(argv) if exit_code != 0: raise RuntimeError(f"Changelog trigger install failed for '{environment_name}': {err[-500:]}") + + +def _checked_image(ref: str) -> str: + """``ref``, refused when it isn't an image reference: the compose file it goes into reads it as YAML.""" + if problem := image_reference_problem(ref): + raise ValueError(f"Can't render the state store's compose services: {problem}") + return ref diff --git a/src/agent_env/utils/names.py b/src/agent_env/utils/names.py new file mode 100644 index 000000000..46fcd98a5 --- /dev/null +++ b/src/agent_env/utils/names.py @@ -0,0 +1,35 @@ +"""The names a gateway deploy writes into its compose file and shell lines: an environment name becomes a compose service, +a container hostname and a schema, an image reference an ``image:`` line, so neither may carry anything else.""" + +from __future__ import annotations + +import re + +# Compose's service-name characters, with a letter or digit first so no name reads as a command-line option. +_ENVIRONMENT_NAME = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]*") + +# Docker's reference grammar (distribution/reference): [registry/]path[:tag][@digest], without its bracketed IPv6 +# registries, which an unquoted compose value reads as the start of a YAML list. +_DOMAIN_LABEL = r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?" +_DOMAIN = rf"{_DOMAIN_LABEL}(?:\.{_DOMAIN_LABEL})*(?::[0-9]+)?" +_PATH_COMPONENT = r"[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*" +_TAG = r"[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}" +_DIGEST = r"[A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*:[0-9A-Fa-f]{32,}" +_IMAGE_REFERENCE = re.compile( + rf"(?:{_DOMAIN}/)?{_PATH_COMPONENT}(?:/{_PATH_COMPONENT})*(?::{_TAG})?(?:@{_DIGEST})?" +) + + +def environment_name_problem(name: str) -> str | None: + """Why ``name`` can't be an environment name, None when it can.""" + if isinstance(name, str) and _ENVIRONMENT_NAME.fullmatch(name): + return None + return (f"environment name {name!r} must start with a letter or digit and hold only letters, digits, " + f"'_', '.' and '-'") + + +def image_reference_problem(ref: str) -> str | None: + """Why ``ref`` isn't an image reference Docker takes, None when it is.""" + if isinstance(ref, str) and _IMAGE_REFERENCE.fullmatch(ref): + return None + return f"image name {ref!r} isn't an image reference ([registry/]repository[:tag][@digest])" diff --git a/src/agent_env/utils/shell.py b/src/agent_env/utils/shell.py new file mode 100644 index 000000000..6c1d4c436 --- /dev/null +++ b/src/agent_env/utils/shell.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import shlex + + +def write_file_script(path: str, content: str) -> str: + """A script that writes ``content`` and a newline to ``path``, the content one quoted argument so no byte of it is shell.""" + return f"printf '%s\\n' {shlex.quote(content)} > {shlex.quote(path)}" diff --git a/tst/unit/artifact/test_docker_image_pulled.py b/tst/unit/artifact/test_docker_image_pulled.py index 1a2c7a123..100358b26 100644 --- a/tst/unit/artifact/test_docker_image_pulled.py +++ b/tst/unit/artifact/test_docker_image_pulled.py @@ -85,3 +85,9 @@ def test_one_with_no_tar_gz_has_no_bytes_to_load(): def test_put_tar_refuses_an_empty_tar_gz(): with pytest.raises(ValueError, match="tar_gz_object_url is empty"): DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_object_url="") + + +@pytest.mark.parametrize("name", ["img:1\n privileged: true", "img 1", ""]) +def test_put_tar_refuses_a_name_that_isnt_an_image_reference(name): + with pytest.raises(ValueError, match="isn't an image reference"): + DockerImageArtifact.put_tar("img", description="d", image_name=name, tar_gz_object_url="s3://bucket/img.tar.gz") diff --git a/tst/unit/artifact/test_docker_image_put_ref.py b/tst/unit/artifact/test_docker_image_put_ref.py index 422f1dfac..e56f48780 100644 --- a/tst/unit/artifact/test_docker_image_put_ref.py +++ b/tst/unit/artifact/test_docker_image_put_ref.py @@ -76,6 +76,16 @@ def test_a_name_without_its_registry_is_refused_before_any_request(local_stores, DockerImageArtifact.get("tool-image") +@pytest.mark.parametrize("ref", ["ghcr.io/org/tool:v1\n privileged: true", "ghcr.io/org/to ol:v1", "ghcr.io/org/Tool:v1"]) +def test_a_name_that_isnt_an_image_reference_is_refused_before_any_request(local_stores, resolved, ref): + with pytest.raises(ValueError, match="isn't an image reference"): + DockerImageArtifact.put_ref("tool-image", description="d", image_name=ref) + + assert resolved == [] + with pytest.raises(NotFoundError): + DockerImageArtifact.get("tool-image") + + @pytest.mark.parametrize("ref", ["localhost:5000/team/img:v1", "127.0.0.1:5000/img", "host.docker.internal:5000/img", "0.0.0.0:5000/img"]) def test_a_registry_on_this_machine_is_refused_for_a_shared_store(local_stores, resolved, ref): diff --git a/tst/unit/bundle/env_toml_test.py b/tst/unit/bundle/env_toml_test.py index f904b6357..9ba1468c4 100644 --- a/tst/unit/bundle/env_toml_test.py +++ b/tst/unit/bundle/env_toml_test.py @@ -187,6 +187,12 @@ def _website(name, toml=""): "envs/x: env.toml: unknown key 'metadata'; a mcp_server env takes image, environment_name, env_provider_type, " "type and id"), ({**_mcp("x"), "envs/x/env.toml": 'environment_name = ""\n'}, "envs/x: env.toml: environment_name can't be empty"), + ({**_mcp("x"), "envs/x/env.toml": 'environment_name = "slack\\nCOMPOSE_EOF\\ntouch pwned"\n'}, + "envs/x: env.toml: environment name 'slack\\nCOMPOSE_EOF\\ntouch pwned' must start with a letter or digit and " + "hold only letters, digits, '_', '.' and '-'"), + ({**_mcp("x"), "envs/x/env.toml": 'environment_name = "a b"\n'}, + "envs/x: env.toml: environment name 'a b' must start with a letter or digit and hold only letters, digits, " + "'_', '.' and '-'"), ({**_mcp("x"), "envs/x/env.toml": 'environment_name = "gateway"\n'}, "envs/x: env.toml: environment_name 'gateway' is one a gateway deploy names its own containers (db-mcp, gateway, " "pgweb, servicedb, website-browser); choose another"), @@ -213,7 +219,8 @@ def _website(name, toml=""): ({"envs/x/env.toml": 'type = "service_db"\n'}, "envs/x: a service_db env isn't written from a bundle: config names the one every deploy uses " "(default_service_db_env_id), and a run builds it when it's missing"), -], ids=["no-card", "several-cards", "store-image-unnamed", "metadata", "empty-name", "reserved-name", +], ids=["no-card", "several-cards", "store-image-unnamed", "metadata", "empty-name", "name-escapes-its-line", + "name-with-a-space", "reserved-name", "website-on-server", "build-args", "multi-without-envs", "multi-name", "multi-wrong-child", "multi-same-names", "multi-same-container", "gateway", "service-db"]) diff --git a/tst/unit/env/envs/test_artifact_staging.py b/tst/unit/env/envs/test_artifact_staging.py index 13cb6042e..84dbf61b8 100644 --- a/tst/unit/env/envs/test_artifact_staging.py +++ b/tst/unit/env/envs/test_artifact_staging.py @@ -14,7 +14,7 @@ from agent_env.env.envs.mcp_server import MCPServerEnv from agent_env.env.envs.website import WebsiteEnv -from agent_env.providers.sandbox_providers.local_sandbox import LocalSandbox +from agent_env.providers.sandbox_providers.local_docker.provider import LocalSandbox _SHELL_NAMES = [ ("email", "x;touch pwned;#.json"), diff --git a/tst/unit/env/envs/test_environment_name_alias.py b/tst/unit/env/envs/test_environment_name_alias.py index 85365160a..85f151635 100644 --- a/tst/unit/env/envs/test_environment_name_alias.py +++ b/tst/unit/env/envs/test_environment_name_alias.py @@ -204,3 +204,22 @@ def test_a_positional_call_that_passed_service_version_raises(call): def test_positional_arguments_up_to_the_name_still_bind(): assert MCPServerEnv("e", 1, _art(), "email").environment_name == "email" assert WebsiteEnv("w", 1, _art(), _art(), "email").environment_name == "email" + + +@pytest.mark.parametrize("cls,doc", _READERS) +@pytest.mark.parametrize("name", ["slack\nCOMPOSE_EOF\ntouch pwned", "slack\n privileged: true", "a b", "-x", "it's"]) +def test_a_name_that_isnt_one_is_not_written_and_a_stored_one_still_loads(cls, doc, name, stub_artifact_get, local_stores): + from agent_env.env.store import get_env_store + + env = cls.from_dict(doc(environment_name=name)) + with pytest.raises(ValueError, match="Can't store env .* must start with a letter or digit"): + get_env_store().put_document(env) + assert env.environment_name == name + + +@pytest.mark.parametrize("cls,doc", _READERS) +@pytest.mark.parametrize("name", ["slack", "Items.v2_x-1", "google_calendar"]) +def test_a_name_compose_takes_is_written(cls, doc, name, stub_artifact_get, local_stores): + from agent_env.env.store import get_env_store + + assert get_env_store().put_document(cls.from_dict({**doc(environment_name=name), "version": None})).version == 1 diff --git a/tst/unit/env/envs/test_mcp_server_container_mode.py b/tst/unit/env/envs/test_mcp_server_container_mode.py index 1a9e7d178..4e2b7574d 100644 --- a/tst/unit/env/envs/test_mcp_server_container_mode.py +++ b/tst/unit/env/envs/test_mcp_server_container_mode.py @@ -308,7 +308,7 @@ async def server_path(env, sandbox_provider, **kwargs): def _env(env_provider_type: str = "gateway") -> MCPServerEnv: - return MCPServerEnv(id="mcp-email", version=1, docker_image_artifact=MagicMock(), environment_name="email", + return MCPServerEnv(id="mcp-email", version=1, docker_image_artifact=MagicMock(image_name="mcp-email"), environment_name="email", env_provider_type=env_provider_type) diff --git a/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py b/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py index c2a4113a8..20018da45 100644 --- a/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py +++ b/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py @@ -10,6 +10,8 @@ from agent_env.env.envs.multi_env import MultiEnv from agent_env.env.gateway import AGENT_ENV_GATEWAY_MCP_PORT from agent_env.providers.env_providers import EnvironmentGatewayProvider +from agent_env.providers.env_providers.constants import DOCKER_COMPOSE_PATH +from agent_env.utils.shell import write_file_script class _Stop(Exception): @@ -57,3 +59,28 @@ async def test_snapshot_reload_waits_for_the_gateway_where_it_is_published(): await env._load_from_snapshot(snapshot) assert wait.call_args.args[1] == AGENT_ENV_GATEWAY_MCP_PORT + 1000 + + +@pytest.mark.asyncio +async def test_snapshot_reload_writes_its_compose_as_one_argument_and_names_each_service_as_one(): + env = MultiEnv(id="crm-suite", version=1, mcp_server_envs=[], name="crm") + env.mcp_server_envs = [MagicMock(environment_name="a b")] + env._sandbox = MagicMock(mode="vm", load_docker_images=AsyncMock(), exec_script=AsyncMock(return_value=""), + host_ips=("127.0.0.1",), host_port=lambda port: port) + db_image = MagicMock(spec=DockerImageArtifact, image_name="snap:1") + snapshot = MagicMock(db_image_artifact_id="snap", db_image_artifact_version=1) + compose = "services:\nCOMPOSE_EOF\ntouch pwned\n" + + with patch("agent_env.artifact.Artifact.get", return_value=db_image), \ + patch("agent_env.env.env.Env.get", return_value=MagicMock()), \ + patch("agent_env.config.get_config", return_value=MagicMock(default_gateway_env_id="gw-id", default_service_db_env_id="db-id")), \ + patch("agent_env.providers.env_state.LocalPostgresStateProvider"), \ + patch.object(EnvironmentGatewayProvider, "create_docker_compose", return_value=compose), \ + patch.object(EnvironmentGatewayProvider, "_wait_for_gateway", side_effect=_Stop): + with pytest.raises(_Stop): + await env._load_from_snapshot(snapshot) + + scripts = [call.args[0] for call in env._sandbox.exec_script.await_args_list] + assert write_file_script(DOCKER_COMPOSE_PATH, compose) in scripts + assert not any("<<" in script for script in scripts) + assert any(script.endswith("--force-recreate 'a b' gateway pgweb db-mcp") for script in scripts) diff --git a/tst/unit/env/envs/test_multi_env_vm_sizing.py b/tst/unit/env/envs/test_multi_env_vm_sizing.py index 4c484560a..569ee2e18 100644 --- a/tst/unit/env/envs/test_multi_env_vm_sizing.py +++ b/tst/unit/env/envs/test_multi_env_vm_sizing.py @@ -22,8 +22,11 @@ def _env(n_mcp: int = 0, n_web: int = 0) -> MultiEnv: - env = MultiEnv(id="e", version=1, mcp_server_envs=[MagicMock() for _ in range(n_mcp)]) - env.website_envs = [MagicMock() for _ in range(n_web)] + env = MultiEnv(id="e", version=1, mcp_server_envs=[ + MagicMock(environment_name=f"mcp{i}", **{"docker_image_artifact.image_name": f"mcp{i}"}) for i in range(n_mcp)]) + env.website_envs = [MagicMock(environment_name=f"web{i}", **{"backend_docker_image_artifact.image_name": f"web{i}-be", + "frontend_docker_image_artifact.image_name": f"web{i}-fe"}) + for i in range(n_web)] return env diff --git a/tst/unit/providers/env_providers/env_gateway_provider_test.py b/tst/unit/providers/env_providers/env_gateway_provider_test.py index c162fa482..c3425d59f 100644 --- a/tst/unit/providers/env_providers/env_gateway_provider_test.py +++ b/tst/unit/providers/env_providers/env_gateway_provider_test.py @@ -9,6 +9,7 @@ import logging import re import socket +import subprocess from datetime import datetime from types import SimpleNamespace @@ -45,6 +46,7 @@ from agent_env.providers.env_state import LocalPostgresStateProvider from agent_env.config import get_config, set_document_store from agent_env.store import EcrImageStore, NotFoundError +from agent_env.utils.shell import write_file_script from tst.unit.providers.env_state.fakes import ( EXTERNAL_READINESS_SERVICE, EXTERNAL_STATE_TYPE, @@ -1610,6 +1612,117 @@ def test_compose_writes_the_env_name_literally(name, rendered): assert f" - MCP_SERVER_NAME={rendered}\n" in compose +_DIGEST = "sha256:" + "0" * 64 +_ESCAPES = ["slack\nCOMPOSE_EOF\ntouch pwned", "slack\n privileged: true", "a b", "-x", "$x", "it's", "[::1]:5000/x", ""] +_NAME_FIELDS = ["server_name", "website_name", "sidecar_name"] +_IMAGE_FIELDS = ["server_image", "backend_image", "frontend_image", "sidecar_image", "gateway_image"] + + +@pytest.mark.parametrize("field, value", [(field, value) for field in _NAME_FIELDS + _IMAGE_FIELDS for value in _ESCAPES] + + [(field, "a:b") for field in _NAME_FIELDS] + + [("mcp_server_name", value) for value in _ESCAPES if "\n" in value]) +def test_compose_refuses_a_name_or_image_that_would_change_what_it_says(field, value): + from agent_env.env.envs.service_db import ServiceDBConfig + + values = {"server_name": "slack", "server_image": "mcp-slack", "website_name": "shop", "backend_image": "shop-be", + "frontend_image": "shop-fe", "sidecar_name": "proxy", "sidecar_image": "proxy", "gateway_image": "gw", + "mcp_server_name": "crm", field: value} + with pytest.raises(ValueError, match="Can't render the gateway's docker-compose.yml"): + EnvironmentGatewayProvider().create_docker_compose( + mcp_servers=[MCPServerConfig(image=values["server_image"], environment_name=values["server_name"])], + website_configs=[WebsiteConfig(backend_image=values["backend_image"], frontend_image=values["frontend_image"], + environment_name=values["website_name"])], + sidecars=[SidecarConfig(environment_name=values["sidecar_name"], image_artifact=_image(values["sidecar_image"]), + container_port=80, host_port=8080)], + gateway_image=values["gateway_image"], + state_provider=LocalPostgresStateProvider(service_db_config=ServiceDBConfig()), + state_instance=LocalPostgresStateProvider.default_instance(), + mcp_server_name=values["mcp_server_name"], + ) + + +@pytest.mark.parametrize("name, image", [ + ("slack", "mcp-slack"), ("Items.v2_x-1", "localhost:5000/local/items-0123abcd:v1"), + ("google_calendar", f"ghcr.io/acme/calendar:1.2@{_DIGEST}"), ("crm", f"public.ecr.aws/acme/crm@{_DIGEST}"), + ("db", "a:b"), ("x", "084828598639.dkr.ecr.us-west-2.amazonaws.com/team/x__y:latest"), +]) +def test_compose_takes_the_names_and_images_compose_and_docker_take(name, image): + from agent_env.env.envs.service_db import ServiceDBConfig + + compose = yaml.safe_load(EnvironmentGatewayProvider().create_docker_compose( + mcp_servers=[MCPServerConfig(image=image, environment_name=name)], gateway_image="gw", + state_provider=LocalPostgresStateProvider(service_db_config=ServiceDBConfig()), + state_instance=LocalPostgresStateProvider.default_instance(), + )) + assert compose["services"][name]["image"] == image + + +@pytest.mark.parametrize("field", ["db_image", "db_web_image", "db_mcp_image"]) +def test_compose_refuses_a_state_store_image_that_would_change_what_it_says(field): + from agent_env.env.envs.service_db import ServiceDBConfig + + config = ServiceDBConfig() + setattr(config, field, "postgres:16\n privileged: true") + with pytest.raises(ValueError, match="state store's compose services"): + EnvironmentGatewayProvider().create_docker_compose( + mcp_servers=[MCPServerConfig(image="mcp-slack", environment_name="slack")], gateway_image="gw", + state_provider=LocalPostgresStateProvider(service_db_config=config), + state_instance=LocalPostgresStateProvider.default_instance(), + ) + + +@pytest.mark.parametrize("bad", ["slack\nCOMPOSE_EOF\ntouch pwned", "a b"]) +@pytest.mark.asyncio +async def test_a_gateway_deploy_refuses_a_name_that_isnt_one_before_anything_starts(bad): + gp = EnvironmentGatewayProvider() + env = MCPServerEnv(id="mail", version=1, docker_image_artifact=_loadable("mcp-mail"), environment_name=bad) + with patch("agent_env.providers.env_state.acquire_state_for_deploy", new=AsyncMock()) as acquire, \ + patch.object(gp, "_launch", new=AsyncMock()) as launch: + with pytest.raises(ValueError, match="Can't deploy 'mail' behind a gateway"): + await gp.deploy(env, _vm_provider()) + acquire.assert_not_awaited() + launch.assert_not_awaited() + + +def test_a_written_file_holds_its_content_as_it_is_and_runs_none_of_it(tmp_path): + content = "services:\nCOMPOSE_EOF\ntouch pwned\nINIT_EOF\n$(touch pwned) `touch pwned` 'it''s' %s \\n" + target = tmp_path / "docker-compose.yml" + subprocess.run(["bash", "-c", write_file_script(str(target), content)], cwd=tmp_path, check=True) + assert target.read_text() == content + "\n" + assert not (tmp_path / "pwned").exists() + + +@pytest.mark.asyncio +async def test_a_vm_deploy_writes_its_compose_file_as_one_quoted_argument(): + from agent_env.env.gateway import GatewayMode + from agent_env.providers.env_state import DatabaseStateProvider + + gp = EnvironmentGatewayProvider() + gp._state_provider = MagicMock(spec=DatabaseStateProvider, prepare=AsyncMock()) + gp._state_instance = MagicMock() + sandbox = AsyncMock() + sandbox_provider = MagicMock(create_vm=AsyncMock(return_value=sandbox)) + compose = "services:\nCOMPOSE_EOF\ntouch pwned\n" + + class _Stop(Exception): + """Abort at compose up: the files are written by then.""" + + with patch("agent_env.env.env.Env.get", return_value=MagicMock(docker_image_artifact=_loadable("gw"))), \ + patch("agent_env.config.get_config", return_value=MagicMock(default_gateway_env_id="gw-id")), \ + patch.object(gp, "create_docker_compose", return_value=compose), \ + patch.object(gp, "_compose_up", side_effect=_Stop): + with pytest.raises(_Stop): + await gp._deploy_via_vm( + sandbox_provider=sandbox_provider, mcp_servers=[MCPServerConfig(image="mcp-slack", environment_name="slack")], + mcp_server_images=[_loadable("mcp-slack")], gateway_port=18765, website_configs=None, website_images=None, + gateway_mode=GatewayMode.PERFORMANCE, ttl_seconds=60, disk_size_gb=10, + ) + + scripts = [call.args[0] for call in sandbox.exec_script.await_args_list] + assert write_file_script(f"{GATEWAY_APP_DIR}/docker-compose.yml", compose) in scripts + assert not any("<<" in script for script in scripts) + + _PROBE_CARD = {"name": "env1234", "children_environments": [ {"name": "a", "capabilities": {"tools": [{"name": "a_list"}, {"name": "a_get"}]}}, {"name": "b", "capabilities": {"tools": [{"name": "b_send"}]}}, From aedfee7a2d310342e04617581998b6bba7249a9c Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 28 Oct 2026 22:04:04 -0700 Subject: [PATCH 2/4] refactor(env): drop the write-time env name check The gateway refuses a name at deploy and bundle authoring refuses it in env.toml, so Env.put_problem only moved the SDK and CLI error earlier at the cost of a base-class hook. Co-Authored-By: Claude Opus 5.5 --- src/agent_env/env/env.py | 5 ----- src/agent_env/env/envs/mcp_server.py | 4 ---- src/agent_env/env/envs/website.py | 4 ---- src/agent_env/env/store.py | 2 -- .../env/envs/test_environment_name_alias.py | 19 ------------------- 5 files changed, 34 deletions(-) diff --git a/src/agent_env/env/env.py b/src/agent_env/env/env.py index 713a83599..c92199f7f 100644 --- a/src/agent_env/env/env.py +++ b/src/agent_env/env/env.py @@ -336,11 +336,6 @@ def merge_metadata(self, updates: dict[str, Any], retries: int = 3) -> None: def to_dict(self) -> dict[str, Any]: return {"id": self.id, "type": self.type, "version": self.version, "metadata": self.metadata} - def put_problem(self) -> str | None: - """Why the store refuses to write this env, None when it doesn't. Checked on write only, so a document already - stored still loads.""" - return None - @classmethod def from_dict(cls, data: dict[str, Any]) -> "Env": raise NotImplementedError(f"{cls.__name__} must implement from_dict") diff --git a/src/agent_env/env/envs/mcp_server.py b/src/agent_env/env/envs/mcp_server.py index a2c3b7bc1..29104edae 100644 --- a/src/agent_env/env/envs/mcp_server.py +++ b/src/agent_env/env/envs/mcp_server.py @@ -25,7 +25,6 @@ from agent_env.env.gateway import GatewayMode from agent_env.providers.sandbox_providers.sandbox_provider import SANDBOX_MODE_CONTAINER from agent_env.attribution import Attribution -from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: @@ -74,9 +73,6 @@ def to_dict(self) -> dict: base["env_provider_type"] = self.env_provider_type return base - def put_problem(self) -> str | None: - return environment_name_problem(self.environment_name) - @classmethod def from_dict(cls, data: dict) -> MCPServerEnv: artifact_ref = data["docker_image_artifact"] diff --git a/src/agent_env/env/envs/website.py b/src/agent_env/env/envs/website.py index 8eb9a3eed..622c07133 100644 --- a/src/agent_env/env/envs/website.py +++ b/src/agent_env/env/envs/website.py @@ -23,7 +23,6 @@ ) from agent_env.env.gateway import GatewayMode from agent_env.attribution import Attribution -from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: from agent_env.bundle.authoring import AuthoringContext @@ -88,9 +87,6 @@ def to_dict(self) -> dict: base["env_provider_type"] = self.env_provider_type return base - def put_problem(self) -> str | None: - return environment_name_problem(self.environment_name) - @classmethod def from_dict(cls, data: dict) -> WebsiteEnv: backend_ref = data["backend_docker_image_artifact"] diff --git a/src/agent_env/env/store.py b/src/agent_env/env/store.py index afa5d5b49..ccdf265f3 100644 --- a/src/agent_env/env/store.py +++ b/src/agent_env/env/store.py @@ -95,8 +95,6 @@ def next_version(self, id: str) -> int: return self._versioned.next_version(id) def put_document(self, env: _E) -> _E: - if problem := env.put_problem(): - raise ValueError(f"Can't store env {env.id!r}: {problem}") store = self._doc_store existing = store.find_one( ENVS_COLLECTION, Filter.of(id=env.id), sort=Sort.by("version", descending=True) diff --git a/tst/unit/env/envs/test_environment_name_alias.py b/tst/unit/env/envs/test_environment_name_alias.py index 85f151635..85365160a 100644 --- a/tst/unit/env/envs/test_environment_name_alias.py +++ b/tst/unit/env/envs/test_environment_name_alias.py @@ -204,22 +204,3 @@ def test_a_positional_call_that_passed_service_version_raises(call): def test_positional_arguments_up_to_the_name_still_bind(): assert MCPServerEnv("e", 1, _art(), "email").environment_name == "email" assert WebsiteEnv("w", 1, _art(), _art(), "email").environment_name == "email" - - -@pytest.mark.parametrize("cls,doc", _READERS) -@pytest.mark.parametrize("name", ["slack\nCOMPOSE_EOF\ntouch pwned", "slack\n privileged: true", "a b", "-x", "it's"]) -def test_a_name_that_isnt_one_is_not_written_and_a_stored_one_still_loads(cls, doc, name, stub_artifact_get, local_stores): - from agent_env.env.store import get_env_store - - env = cls.from_dict(doc(environment_name=name)) - with pytest.raises(ValueError, match="Can't store env .* must start with a letter or digit"): - get_env_store().put_document(env) - assert env.environment_name == name - - -@pytest.mark.parametrize("cls,doc", _READERS) -@pytest.mark.parametrize("name", ["slack", "Items.v2_x-1", "google_calendar"]) -def test_a_name_compose_takes_is_written(cls, doc, name, stub_artifact_get, local_stores): - from agent_env.env.store import get_env_store - - assert get_env_store().put_document(cls.from_dict({**doc(environment_name=name), "version": None})).version == 1 From d174e9dc03764c7a893abf99751afab14e14ddff Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 28 Oct 2026 22:11:15 -0700 Subject: [PATCH 3/4] refactor(gateway): keep each name check with the code it guards The environment name rule is the gateway's compose service name rule, so it lives in env_gateway_provider, which bundle authoring already imports. The image reference grammar sits with names_registry and registry_host_from_ref in oci_registry_credentials. utils/names goes. Co-Authored-By: Claude Opus 5.5 --- .../artifact/artifacts/docker_image.py | 3 +- src/agent_env/bundle/authoring.py | 3 +- .../env_providers/env_gateway_provider.py | 15 +++++++- .../providers/env_state/local_postgres.py | 2 +- .../image_store/oci_registry_credentials.py | 21 +++++++++++ src/agent_env/utils/names.py | 35 ------------------- 6 files changed, 38 insertions(+), 41 deletions(-) delete mode 100644 src/agent_env/utils/names.py diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index 6888174d2..af595cb71 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/src/agent_env/artifact/artifacts/docker_image.py @@ -21,11 +21,10 @@ from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.store.ids import fs_safe, image_repository, is_local_id, local_image_repository from agent_env.store.image_store.local_registry_image_store import LocalRegistryImageStore -from agent_env.store.image_store.oci_registry_credentials import is_loopback_host, names_registry, registry_host_from_ref +from agent_env.store.image_store.oci_registry_credentials import image_reference_problem, is_loopback_host, names_registry, registry_host_from_ref from agent_env.store.image_store.registry_api import NoSuchImageError, pin_digest, pinned_from from agent_env.utils.docker.context import BuildContext from agent_env.utils.docker.build import DEFAULT_BUILD_PLATFORM, build_image, normalize_platform -from agent_env.utils.names import image_reference_problem logger = logging.getLogger(__name__) diff --git a/src/agent_env/bundle/authoring.py b/src/agent_env/bundle/authoring.py index 6ddcb2337..f981e6ec9 100644 --- a/src/agent_env/bundle/authoring.py +++ b/src/agent_env/bundle/authoring.py @@ -19,10 +19,9 @@ from agent_env.env.env import Env from agent_env.env.envs._deployment import provider_refusal from agent_env.providers.env_providers.constants import GATEWAY_SERVICE_NAMES -from agent_env.providers.env_providers.env_gateway_provider import EnvironmentGatewayProvider +from agent_env.providers.env_providers.env_gateway_provider import EnvironmentGatewayProvider, environment_name_problem from agent_env.providers.env_providers.env_provider import _env_provider_class from agent_env.utils.card_naming import card_names_in_files -from agent_env.utils.names import environment_name_problem from agent_env.utils.paths import os_reason from ._fs import relative, show, with_article diff --git a/src/agent_env/providers/env_providers/env_gateway_provider.py b/src/agent_env/providers/env_providers/env_gateway_provider.py index 5aa5cf662..4f37de392 100644 --- a/src/agent_env/providers/env_providers/env_gateway_provider.py +++ b/src/agent_env/providers/env_providers/env_gateway_provider.py @@ -57,7 +57,7 @@ Accepts, SandboxProvider, ) -from agent_env.utils.names import environment_name_problem, image_reference_problem +from agent_env.store.image_store.oci_registry_credentials import image_reference_problem from agent_env.utils.shell import write_file_script if TYPE_CHECKING: @@ -1502,6 +1502,19 @@ def _gateway_topology(env: Env) -> _GatewayTopology: ) +# Compose's service-name characters, with a letter or digit first so no name reads as a command-line option. +_ENVIRONMENT_NAME = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]*") + + +def environment_name_problem(name: str) -> str | None: + """Why ``name`` can't be an environment name, None when it can: a gateway deploy makes it a compose service, a + container hostname and a schema, so it may carry nothing else.""" + if isinstance(name, str) and _ENVIRONMENT_NAME.fullmatch(name): + return None + return (f"environment name {name!r} must start with a letter or digit and hold only letters, digits, " + f"'_', '.' and '-'") + + def _compose_name_problems(mcp_servers: list[MCPServerConfig], website_configs: list[WebsiteConfig] | None, sidecars: list[SidecarConfig] | None = None, *, images: tuple[str, ...] = ()) -> list[str]: """Why the environment names and images of a gateway deploy can't go into its compose file and shell lines.""" diff --git a/src/agent_env/providers/env_state/local_postgres.py b/src/agent_env/providers/env_state/local_postgres.py index b962c625f..154d1e120 100644 --- a/src/agent_env/providers/env_state/local_postgres.py +++ b/src/agent_env/providers/env_state/local_postgres.py @@ -38,7 +38,7 @@ StateContext, LOCAL_POSTGRES_STATE_TYPE, ) -from agent_env.utils.names import image_reference_problem +from agent_env.store.image_store.oci_registry_credentials import image_reference_problem if TYPE_CHECKING: from collections.abc import Awaitable, Callable diff --git a/src/agent_env/store/image_store/oci_registry_credentials.py b/src/agent_env/store/image_store/oci_registry_credentials.py index 7ae258953..083d94962 100644 --- a/src/agent_env/store/image_store/oci_registry_credentials.py +++ b/src/agent_env/store/image_store/oci_registry_credentials.py @@ -6,6 +6,7 @@ import binascii import json import logging +import re from abc import ABC, abstractmethod from collections.abc import Mapping from dataclasses import dataclass, field @@ -90,6 +91,26 @@ def registry_host_from_ref(ref: str) -> str | None: return None +# Docker's reference grammar (distribution/reference): [registry/]path[:tag][@digest], without its bracketed IPv6 +# registries, which an unquoted compose value reads as the start of a YAML list. +_DOMAIN_LABEL = r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?" +_DOMAIN = rf"{_DOMAIN_LABEL}(?:\.{_DOMAIN_LABEL})*(?::[0-9]+)?" +_PATH_COMPONENT = r"[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*" +_TAG = r"[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}" +_DIGEST = r"[A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*:[0-9A-Fa-f]{32,}" +_IMAGE_REFERENCE = re.compile( + rf"(?:{_DOMAIN}/)?{_PATH_COMPONENT}(?:/{_PATH_COMPONENT})*(?::{_TAG})?(?:@{_DIGEST})?" +) + + +def image_reference_problem(ref: str) -> str | None: + """Why ``ref`` isn't an image reference Docker takes, None when it is: a compose ``image:`` line holds it + unquoted, so it may carry nothing else.""" + if isinstance(ref, str) and _IMAGE_REFERENCE.fullmatch(ref): + return None + return f"image name {ref!r} isn't an image reference ([registry/]repository[:tag][@digest])" + + def is_loopback_host(host: str | None) -> bool: """Whether a registry ``host``, with or without its port, names this machine: a loopback address, the unspecified ``0.0.0.0``, or ``host.docker.internal``, which only this machine's containers resolve.""" diff --git a/src/agent_env/utils/names.py b/src/agent_env/utils/names.py deleted file mode 100644 index 46fcd98a5..000000000 --- a/src/agent_env/utils/names.py +++ /dev/null @@ -1,35 +0,0 @@ -"""The names a gateway deploy writes into its compose file and shell lines: an environment name becomes a compose service, -a container hostname and a schema, an image reference an ``image:`` line, so neither may carry anything else.""" - -from __future__ import annotations - -import re - -# Compose's service-name characters, with a letter or digit first so no name reads as a command-line option. -_ENVIRONMENT_NAME = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]*") - -# Docker's reference grammar (distribution/reference): [registry/]path[:tag][@digest], without its bracketed IPv6 -# registries, which an unquoted compose value reads as the start of a YAML list. -_DOMAIN_LABEL = r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?" -_DOMAIN = rf"{_DOMAIN_LABEL}(?:\.{_DOMAIN_LABEL})*(?::[0-9]+)?" -_PATH_COMPONENT = r"[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*" -_TAG = r"[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}" -_DIGEST = r"[A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*:[0-9A-Fa-f]{32,}" -_IMAGE_REFERENCE = re.compile( - rf"(?:{_DOMAIN}/)?{_PATH_COMPONENT}(?:/{_PATH_COMPONENT})*(?::{_TAG})?(?:@{_DIGEST})?" -) - - -def environment_name_problem(name: str) -> str | None: - """Why ``name`` can't be an environment name, None when it can.""" - if isinstance(name, str) and _ENVIRONMENT_NAME.fullmatch(name): - return None - return (f"environment name {name!r} must start with a letter or digit and hold only letters, digits, " - f"'_', '.' and '-'") - - -def image_reference_problem(ref: str) -> str | None: - """Why ``ref`` isn't an image reference Docker takes, None when it is.""" - if isinstance(ref, str) and _IMAGE_REFERENCE.fullmatch(ref): - return None - return f"image name {ref!r} isn't an image reference ([registry/]repository[:tag][@digest])" From aca228479be69004e45a79e14a1e6933894ca7d0 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 28 Oct 2026 22:19:42 -0700 Subject: [PATCH 4/4] refactor(gateway): write host files through a provider method The snapshot reload already drives the gateway through its private helpers, so the write is EnvironmentGatewayProvider._write_file beside _compose_up rather than a utils module. The reload's compose write now retries like the deploy's; the write only replaces the file. Co-Authored-By: Claude Opus 5.5 --- src/agent_env/env/envs/multi_env.py | 3 +-- .../providers/env_providers/env_gateway_provider.py | 11 ++++++++--- src/agent_env/utils/shell.py | 8 -------- .../env/envs/test_multi_env_snapshot_reload_name.py | 4 ++-- .../env_providers/env_gateway_provider_test.py | 11 +++++++---- 5 files changed, 18 insertions(+), 19 deletions(-) delete mode 100644 src/agent_env/utils/shell.py diff --git a/src/agent_env/env/envs/multi_env.py b/src/agent_env/env/envs/multi_env.py index 8b5bd3928..0e2e3758e 100644 --- a/src/agent_env/env/envs/multi_env.py +++ b/src/agent_env/env/envs/multi_env.py @@ -44,7 +44,6 @@ def _snapshot_after_load_default() -> bool: from agent_env.store.ids import derive_id from agent_env.providers.sandbox_providers.sandbox_provider import SANDBOX_MODE_CONTAINER from agent_env.attribution import Attribution -from agent_env.utils.shell import write_file_script class MultiEnv(Env): @@ -637,7 +636,7 @@ async def _load_from_snapshot(self, snapshot) -> None: compose_content = compose_content.replace( " volumes:\n - ./init-schemas.sql:/docker-entrypoint-initdb.d/init-schemas.sql\n", "", ) - await self._sandbox.exec_script(write_file_script(DOCKER_COMPOSE_PATH, compose_content)) + await gw._write_file(self._sandbox, DOCKER_COMPOSE_PATH, compose_content) # Remove old servicedb container and its anonymous volume, then start fresh from snapshot image logger.info("Recreating servicedb container with snapshot image...") diff --git a/src/agent_env/providers/env_providers/env_gateway_provider.py b/src/agent_env/providers/env_providers/env_gateway_provider.py index 4f37de392..90b1fbe1e 100644 --- a/src/agent_env/providers/env_providers/env_gateway_provider.py +++ b/src/agent_env/providers/env_providers/env_gateway_provider.py @@ -58,7 +58,6 @@ SandboxProvider, ) from agent_env.store.image_store.oci_registry_credentials import image_reference_problem -from agent_env.utils.shell import write_file_script if TYPE_CHECKING: from collections.abc import Awaitable, Callable @@ -825,9 +824,9 @@ async def _stand_up(_spec): if self._needs_local_postgres: init_script = local_store_spec.init_sql logger.info(f"Generated init-schemas.sql:\n{init_script}") - await sandbox.exec_script(write_file_script(f"{GATEWAY_APP_DIR}/init-schemas.sql", init_script), max_retries=2) + await self._write_file(sandbox, f"{GATEWAY_APP_DIR}/init-schemas.sql", init_script) - await sandbox.exec_script(write_file_script(DOCKER_COMPOSE_PATH, compose_content), max_retries=2) + await self._write_file(sandbox, DOCKER_COMPOSE_PATH, compose_content) # Start MCP services with docker compose logger.info("Starting MCP services...") @@ -1122,6 +1121,12 @@ async def read_trajectory(self, sandbox: VmSandbox) -> list[dict]: events.append(json.loads(line)) return events + @staticmethod + async def _write_file(sandbox: VmSandbox, path: str, content: str) -> None: + """Write ``content`` and a newline to ``path`` on the sandbox host, the content one quoted argument so no byte + of it is shell.""" + await sandbox.exec_script(f"printf '%s\\n' {shlex.quote(content)} > {shlex.quote(path)}", max_retries=2) + async def _compose_up(self, sandbox: VmSandbox, args: str = "") -> None: """``docker compose up -d`` the stack, whose failure names each service that went down and ends with its output, which compose's own error leaves out.""" diff --git a/src/agent_env/utils/shell.py b/src/agent_env/utils/shell.py deleted file mode 100644 index 6c1d4c436..000000000 --- a/src/agent_env/utils/shell.py +++ /dev/null @@ -1,8 +0,0 @@ -from __future__ import annotations - -import shlex - - -def write_file_script(path: str, content: str) -> str: - """A script that writes ``content`` and a newline to ``path``, the content one quoted argument so no byte of it is shell.""" - return f"printf '%s\\n' {shlex.quote(content)} > {shlex.quote(path)}" diff --git a/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py b/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py index 20018da45..3ee8f8da2 100644 --- a/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py +++ b/tst/unit/env/envs/test_multi_env_snapshot_reload_name.py @@ -2,6 +2,7 @@ from __future__ import annotations +import shlex from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -11,7 +12,6 @@ from agent_env.env.gateway import AGENT_ENV_GATEWAY_MCP_PORT from agent_env.providers.env_providers import EnvironmentGatewayProvider from agent_env.providers.env_providers.constants import DOCKER_COMPOSE_PATH -from agent_env.utils.shell import write_file_script class _Stop(Exception): @@ -81,6 +81,6 @@ async def test_snapshot_reload_writes_its_compose_as_one_argument_and_names_each await env._load_from_snapshot(snapshot) scripts = [call.args[0] for call in env._sandbox.exec_script.await_args_list] - assert write_file_script(DOCKER_COMPOSE_PATH, compose) in scripts + assert ["printf", "%s\\n", compose, ">", DOCKER_COMPOSE_PATH] in map(shlex.split, scripts) assert not any("<<" in script for script in scripts) assert any(script.endswith("--force-recreate 'a b' gateway pgweb db-mcp") for script in scripts) diff --git a/tst/unit/providers/env_providers/env_gateway_provider_test.py b/tst/unit/providers/env_providers/env_gateway_provider_test.py index c3425d59f..1f882a41c 100644 --- a/tst/unit/providers/env_providers/env_gateway_provider_test.py +++ b/tst/unit/providers/env_providers/env_gateway_provider_test.py @@ -8,6 +8,7 @@ import json import logging import re +import shlex import socket import subprocess from datetime import datetime @@ -46,7 +47,6 @@ from agent_env.providers.env_state import LocalPostgresStateProvider from agent_env.config import get_config, set_document_store from agent_env.store import EcrImageStore, NotFoundError -from agent_env.utils.shell import write_file_script from tst.unit.providers.env_state.fakes import ( EXTERNAL_READINESS_SERVICE, EXTERNAL_STATE_TYPE, @@ -1684,10 +1684,13 @@ async def test_a_gateway_deploy_refuses_a_name_that_isnt_one_before_anything_sta launch.assert_not_awaited() -def test_a_written_file_holds_its_content_as_it_is_and_runs_none_of_it(tmp_path): +@pytest.mark.asyncio +async def test_a_written_file_holds_its_content_as_it_is_and_runs_none_of_it(tmp_path): content = "services:\nCOMPOSE_EOF\ntouch pwned\nINIT_EOF\n$(touch pwned) `touch pwned` 'it''s' %s \\n" target = tmp_path / "docker-compose.yml" - subprocess.run(["bash", "-c", write_file_script(str(target), content)], cwd=tmp_path, check=True) + host = MagicMock(exec_script=AsyncMock(side_effect=lambda script, **_: subprocess.run( + ["bash", "-c", script], cwd=tmp_path, check=True))) + await EnvironmentGatewayProvider._write_file(host, str(target), content) assert target.read_text() == content + "\n" assert not (tmp_path / "pwned").exists() @@ -1719,7 +1722,7 @@ class _Stop(Exception): ) scripts = [call.args[0] for call in sandbox.exec_script.await_args_list] - assert write_file_script(f"{GATEWAY_APP_DIR}/docker-compose.yml", compose) in scripts + assert ["printf", "%s\\n", compose, ">", f"{GATEWAY_APP_DIR}/docker-compose.yml"] in map(shlex.split, scripts) assert not any("<<" in script for script in scripts)