From 96b5f919c5961a18da3206a6769864116e955a03 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 7 Oct 2026 10:09:25 -0700 Subject: [PATCH 1/3] feat(artifact): write neutral twins for the S3-named artifact keys and keywords Artifact docs keep their S3-named keys (s3_url, tar_gz_s3_url, build_context_s3_url, bundle_s3_url, skill_s3_url, cli_s3_url) and now also carry the attribute's own name beside each (object_url, tar_gz_object_url, ...). A doc keyed either way loads, and every dump writes both, from the attribute, so readers of either spelling keep working; a field a dump excludes stays excluded. Readers on older versions ignore the extra key. The put helpers take neutral keywords: FileArtifactUniverse.put(bundle_object_url=), put_bundled(prefix_url=), put_existing(prefix_url=), DockerImageArtifact.put_tar(tar_gz_object_url=, build_context_object_url=) and SkillArtifact.validate(object_url=). The S3-named ones still work, through utils.deprecation's warn_deprecated, whose log line counts their callers; so do the sandbox's write_file_from_s3 / load_s3_file aliases now. Core's own callers move to the neutral spellings. FileArtifact.put_existing no longer splits :///: it refuses a trailing slash and leaves the rest to the store. The explorer reads either key. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/agent_env/a2a_agent/validator.py | 6 +- src/agent_env/artifact/artifact.py | 7 ++ src/agent_env/artifact/artifacts/cli.py | 23 ++-- .../artifact/artifacts/docker_image.py | 46 +++++-- src/agent_env/artifact/artifacts/file.py | 34 ++++-- .../artifacts/file_artifact_universe.py | 65 ++++++---- src/agent_env/artifact/artifacts/skill.py | 32 +++-- .../cli/artifact/file_artifact_universe.py | 2 +- src/agent_env/env/snapshot_store.py | 4 +- src/agent_env/explorer/routers/common.py | 2 +- .../providers/sandbox_providers/sandbox.py | 8 +- .../snapshot_utils/agent_state_capture.py | 2 +- .../task_step/task_steps/add_skills.py | 2 +- .../task_step/task_steps/collect_artifacts.py | 20 ++-- .../task_step/task_steps/snapshot_env.py | 2 +- .../run_container_unit_tests_verifier.py | 8 +- src/agent_env/utils/deprecation.py | 13 +- .../artifact/test_docker_image_github_push.py | 6 +- tst/unit/artifact/test_object_url_keys.py | 112 ++++++++++++++++++ tst/unit/conftest.py | 7 ++ tst/unit/explorer/explorer_app_test.py | 8 +- .../sandbox_providers/vm_sandbox_test.py | 15 ++- .../serialization/golden/artifact/cli.json | 1 + .../golden/artifact/docker_image.json | 2 + .../serialization/golden/artifact/file.json | 1 + .../artifact/file_artifact_universe.json | 1 + .../serialization/golden/artifact/skill.json | 1 + .../serialization/test_golden_wire_format.py | 11 +- tst/unit/store/derived_ids_test.py | 8 +- .../store/local_backends_composition_test.py | 19 +-- tst/unit/store/local_id_acceptance_test.py | 4 +- tst/unit/store/routing_test.py | 4 +- tst/unit/task_step/capture_stubs.py | 10 +- .../run_container_unit_tests_verifier_test.py | 16 +-- 34 files changed, 367 insertions(+), 135 deletions(-) create mode 100644 tst/unit/artifact/test_object_url_keys.py diff --git a/src/agent_env/a2a_agent/validator.py b/src/agent_env/a2a_agent/validator.py index 05f07ec0..82b08b05 100644 --- a/src/agent_env/a2a_agent/validator.py +++ b/src/agent_env/a2a_agent/validator.py @@ -727,7 +727,7 @@ def _upload_install_test_image_fixture(agent: "A2AAgent"): ts = int(time.time()) universe_id = derive_id(agent.id, f"validate-install-image-v{agent.version}-{ts}") config = get_config() - s3_url = config.get_object_store_for(universe_id).object_url(f"{config.get_artifact_key_prefix()}a2a_validator/install_test_image/{ts}/") + prefix_url = config.get_object_store_for(universe_id).object_url(f"{config.get_artifact_key_prefix()}a2a_validator/install_test_image/{ts}/") with tempfile.NamedTemporaryFile("w", suffix=".Dockerfile", delete=False) as f: f.write("FROM ubuntu:24.04\n") @@ -736,9 +736,9 @@ def _upload_install_test_image_fixture(agent: "A2AAgent"): universe = FileArtifactUniverse.put_bundled( id=universe_id, files={"Dockerfile": Path(tmp_path)}, - s3_url=s3_url, + prefix_url=prefix_url, ) - logger.info(f"Uploaded install-test image universe '{universe.id}' v{universe.version} to {s3_url}") + logger.info(f"Uploaded install-test image universe '{universe.id}' v{universe.version} to {prefix_url}") return universe finally: try: diff --git a/src/agent_env/artifact/artifact.py b/src/agent_env/artifact/artifact.py index 4cc229a9..2e817a35 100644 --- a/src/agent_env/artifact/artifact.py +++ b/src/agent_env/artifact/artifact.py @@ -14,6 +14,13 @@ from agent_env.artifact.store import ArtifactQuery +def _write_twin(data: dict[str, Any], legacy: str, neutral: str, value: Any) -> None: + """Write a renamed field under both its legacy and its neutral key, when the dump has it under either: readers + of either spelling keep working, and a field the dump excluded stays excluded.""" + if legacy in data or neutral in data: + data[legacy] = data[neutral] = value + + class Artifact(BaseModel): """Base class for all environment artifacts. Immutable - each modification creates a new version.""" diff --git a/src/agent_env/artifact/artifacts/cli.py b/src/agent_env/artifact/artifacts/cli.py index c43dac48..4e0362e8 100644 --- a/src/agent_env/artifact/artifacts/cli.py +++ b/src/agent_env/artifact/artifacts/cli.py @@ -1,7 +1,7 @@ """CliArtifact — packages a CLI tool (entrypoint + supporting files) as a versioned artifact. A CliArtifact references one FileArtifactUniverse bundling every file in the CLI -directory tree, colocated under a single S3 prefix (`artifacts/cli///`). +directory tree, colocated under a single object-store prefix (`artifacts/cli///`). The `entrypoint` field is a relative path within that prefix that consumers (e.g. an agent gateway) chmod +x and symlink onto PATH. """ @@ -9,11 +9,11 @@ from __future__ import annotations from pathlib import Path -from typing import TYPE_CHECKING, Literal, Optional +from typing import TYPE_CHECKING, Any, Literal, Optional -from pydantic import Field +from pydantic import ConfigDict, Field, model_serializer -from agent_env.artifact.artifact import Artifact +from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.config import get_config from agent_env.store.ids import derive_id, key_segment @@ -22,6 +22,8 @@ class CliArtifact(Artifact): + model_config = ConfigDict(populate_by_name=True) + type: Literal["cli"] = "cli" cli_files_id: str = Field(description="ID of the FileArtifactUniverse bundling every file in the CLI tree") @@ -32,6 +34,13 @@ class CliArtifact(Artifact): env_id: Optional[str] = Field(default=None, description="Source env id this CLI was generated from, if any") env_version: Optional[int] = Field(default=None, description="Source env version this CLI was generated from, if any") + @model_serializer(mode="wrap") + def _serialize(self, handler: Any) -> dict[str, Any]: + # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. + data = handler(self) + _write_twin(data, "cli_s3_url", "cli_object_url", self.cli_object_url) + return data + @classmethod def put( cls, @@ -62,15 +71,15 @@ def put( store = get_artifact_store() version = store.next_version(id) key = f"{get_config().get_artifact_key_prefix()}artifacts/cli/{key_segment(id)}/{version}/" - cli_s3_url = get_config().get_object_store_for(id).object_url(key) + cli_object_url = get_config().get_object_store_for(id).object_url(key) - universe = FileArtifactUniverse.put_bundled(id=derive_id(id, "files"), files=files, s3_url=cli_s3_url) + universe = FileArtifactUniverse.put_bundled(id=derive_id(id, "files"), files=files, prefix_url=cli_object_url) instance = cls( id=id, version=version, cli_files_id=universe.id, - cli_s3_url=cli_s3_url, + cli_object_url=cli_object_url, entrypoint=entrypoint, command_name=command_name, env_id=env_id, diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index b0aa7400..7632cc67 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/src/agent_env/artifact/artifacts/docker_image.py @@ -1,4 +1,4 @@ -"""Docker image artifact for storing Docker images in S3.""" +"""Docker image artifact for storing Docker images in the object store.""" from __future__ import annotations @@ -16,14 +16,15 @@ from dataclasses import dataclass from importlib.metadata import version as pkg_version from pathlib import Path -from typing import Callable, ClassVar, Literal +from typing import Any, Callable, ClassVar, Literal from urllib.parse import urlparse -from pydantic import Field +from pydantic import ConfigDict, Field, model_serializer -from agent_env.artifact.artifact import Artifact +from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.store.ids import fs_safe, image_repository, is_local_id from agent_env.store.image_store.oci_registry_credentials import is_loopback_host, registry_host_from_ref +from agent_env.utils.deprecation import renamed_keyword logger = logging.getLogger(__name__) @@ -62,7 +63,9 @@ def _git_clone_commands(owner: str, repo: str, ref: str | None, token: str | Non ] class DockerImageArtifact(Artifact): - """A Docker image artifact stored as tar.gz in S3.""" + """A Docker image artifact stored as tar.gz in the object store.""" + + model_config = ConfigDict(populate_by_name=True) DOCKER_SAVE_TIMEOUT_SECONDS: ClassVar[int] = 900 @@ -72,6 +75,14 @@ class DockerImageArtifact(Artifact): tar_gz_object_url: str = Field(alias="tar_gz_s3_url", description="Object-store locator of the tar.gz file") build_context_object_url: str | None = Field(default=None, alias="build_context_s3_url", description="Object-store locator of the build context tar.gz") + @model_serializer(mode="wrap") + def _serialize(self, handler: Any) -> dict[str, Any]: + # Dual-write, from the attributes: `alias=` emits one spelling, which one depending on the caller's `by_alias`. + data = handler(self) + _write_twin(data, "tar_gz_s3_url", "tar_gz_object_url", self.tar_gz_object_url) + _write_twin(data, "build_context_s3_url", "build_context_object_url", self.build_context_object_url) + return data + @classmethod def put( cls, @@ -150,8 +161,8 @@ def put( id, description=description, image_name=image_ref, - tar_gz_s3_url=tar_gz_object_url, - build_context_s3_url=build_context_object_url, + tar_gz_object_url=tar_gz_object_url, + build_context_object_url=build_context_object_url, ) @classmethod @@ -161,13 +172,22 @@ def put_tar( *, description: str, image_name: str, - tar_gz_s3_url: str, + tar_gz_object_url: str | None = None, + build_context_object_url: str | None = None, + tar_gz_s3_url: str | None = None, build_context_s3_url: str | None = None, ) -> "DockerImageArtifact": from agent_env.artifact.store import get_artifact_store from agent_env.config import get_config - for url in (tar_gz_s3_url, build_context_s3_url): + owner = "DockerImageArtifact.put_tar" + tar_gz_object_url = renamed_keyword(owner, "tar_gz_object_url", tar_gz_object_url, "tar_gz_s3_url", tar_gz_s3_url) + build_context_object_url = renamed_keyword( + owner, "build_context_object_url", build_context_object_url, "build_context_s3_url", build_context_s3_url + ) + if tar_gz_object_url is None: + raise TypeError(f"{owner}() missing required keyword argument: 'tar_gz_object_url'") + for url in (tar_gz_object_url, build_context_object_url): if url: get_config().check_object_url(id, url) store = get_artifact_store() @@ -177,8 +197,8 @@ def put_tar( version=version, description=description, image_name=image_name, - tar_gz_s3_url=tar_gz_s3_url, - build_context_s3_url=build_context_s3_url, + tar_gz_object_url=tar_gz_object_url, + build_context_object_url=build_context_object_url, ) return store.put_document(instance) @@ -311,8 +331,8 @@ async def put_from_github( id=id, description=f"Built from GitHub: {dockerfile_github_url}", image_name=image_ref, - tar_gz_s3_url=tar_gz_object_url, - build_context_s3_url=build_context_object_url, + tar_gz_object_url=tar_gz_object_url, + build_context_object_url=build_context_object_url, ) logger.info(f"put_from_github: created artifact id={artifact.id} version={artifact.version}") diff --git a/src/agent_env/artifact/artifacts/file.py b/src/agent_env/artifact/artifacts/file.py index b5b104e7..8756d296 100644 --- a/src/agent_env/artifact/artifacts/file.py +++ b/src/agent_env/artifact/artifacts/file.py @@ -1,14 +1,14 @@ -"""Generic file artifact for storing arbitrary files in S3.""" +"""Generic file artifact for storing arbitrary files in the object store.""" from __future__ import annotations import mimetypes import os -from typing import TYPE_CHECKING, ClassVar, Literal, Self +from typing import TYPE_CHECKING, Any, ClassVar, Literal, Self -from pydantic import Field +from pydantic import ConfigDict, Field, model_serializer -from agent_env.artifact.artifact import Artifact +from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.config import get_config if TYPE_CHECKING: @@ -16,7 +16,7 @@ class FileArtifact(Artifact): - """A generic file artifact that stores any file in S3. + """A generic file artifact that stores any file in the object store. Clients define their own schemas and upload files (JSON, images, etc.). When loaded, returns raw bytes for the client to parse as needed. @@ -34,6 +34,8 @@ class FileArtifact(Artifact): emails = json.loads(data) # Client parses as needed """ + model_config = ConfigDict(populate_by_name=True) + toml_keys: ClassVar[dict[str, type]] = {"description": str} # what an artifact.toml may set type: Literal["file"] = "file" description: str = Field(description="Human-readable description of the artifact contents") @@ -41,6 +43,13 @@ class FileArtifact(Artifact): content_type: str = Field(description="MIME type of the file") object_url: str = Field(alias="s3_url", description="Object-store locator where the file is stored") + @model_serializer(mode="wrap") + def _serialize(self, handler: Any) -> dict[str, Any]: + # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. + data = handler(self) + _write_twin(data, "s3_url", "object_url", self.object_url) + return data + @classmethod def put( cls, @@ -63,7 +72,7 @@ def put( # Use put_object_file (boto3 managed multipart upload) instead of # put_object (s3.put_object — hard 5 GB single-object limit). Streams # directly from disk so large files don't get loaded into RAM either. - s3_url = store.put_object_file( + stored_url = store.put_object_file( artifact_type="file", id=id, version=version, @@ -79,7 +88,7 @@ def put( description=description, filename=filename, content_type=content_type, - s3_url=s3_url, + object_url=stored_url, ) return store.put_document(instance) @@ -99,7 +108,7 @@ def put_bytes( store = get_artifact_store() version = store.next_version(id) - s3_url = store.put_object( + stored_url = store.put_object( artifact_type="file", id=id, version=version, @@ -114,7 +123,7 @@ def put_bytes( description=description, filename=filename, content_type=content_type, - s3_url=s3_url, + object_url=stored_url, ) return store.put_document(instance) @@ -174,7 +183,7 @@ def put_at( description=description, filename=filename, content_type=content_type, - s3_url=stored_url, + object_url=stored_url, ) return artifact_store.put_document(instance) @@ -196,8 +205,7 @@ def put_existing( from agent_env.artifact.store import get_artifact_store store = get_config().get_object_store_to_write(object_url, id) - location = object_url.partition("://")[2] or object_url - if "/" not in location or location.endswith("/"): + if object_url.endswith("/"): raise ValueError(f"object_url must point at an object, not a prefix: {object_url!r}") metadata = store.get_object_metadata_at(object_url) if metadata is None: @@ -215,6 +223,6 @@ def put_existing( description=description, filename=filename, content_type=content_type, - s3_url=object_url, + object_url=object_url, ) return artifact_store.put_document(instance) diff --git a/src/agent_env/artifact/artifacts/file_artifact_universe.py b/src/agent_env/artifact/artifacts/file_artifact_universe.py index cabb0224..7fec4c29 100644 --- a/src/agent_env/artifact/artifacts/file_artifact_universe.py +++ b/src/agent_env/artifact/artifacts/file_artifact_universe.py @@ -5,14 +5,16 @@ import hashlib import logging from pathlib import Path -from typing import TYPE_CHECKING, ClassVar, Literal, Optional +from typing import TYPE_CHECKING, Any, ClassVar, Literal, Optional -from pydantic import Field +from pydantic import ConfigDict, Field, model_serializer +from agent_env.artifact.artifact import _write_twin from agent_env.artifact.ref import ArtifactRef from agent_env.artifact.universe import Universe from agent_env.config import get_config from agent_env.store.ids import derive_id +from agent_env.utils.deprecation import renamed_keyword logger = logging.getLogger(__name__) @@ -31,6 +33,8 @@ class FileArtifactUniverse(Universe): load every ``FileArtifact`` document. """ + model_config = ConfigDict(populate_by_name=True) + toml_keys: ClassVar[dict[str, type]] = {} # what an artifact.toml may set type: Literal["file_artifact_universe"] = "file_artifact_universe" file_artifact_refs: Optional[dict[str, ArtifactRef]] = Field( @@ -63,20 +67,31 @@ class FileArtifactUniverse(Universe): ), ) + @model_serializer(mode="wrap") + def _serialize(self, handler: Any) -> dict[str, Any]: + # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. + data = handler(self) + _write_twin(data, "bundle_s3_url", "bundle_object_url", self.bundle_object_url) + return data + @classmethod def put( cls, id: str, *, file_artifacts: dict[str, "FileArtifact"], + bundle_object_url: str | None = None, bundle_s3_url: str | None = None, ) -> "FileArtifactUniverse": from agent_env.artifact.store import get_artifact_store + bundle_object_url = renamed_keyword( + "FileArtifactUniverse.put", "bundle_object_url", bundle_object_url, "bundle_s3_url", bundle_s3_url + ) if not file_artifacts: raise ValueError("file_artifacts must be non-empty") - if bundle_s3_url: - get_config().check_object_url(id, bundle_s3_url) + if bundle_object_url: + get_config().check_object_url(id, bundle_object_url) store = get_artifact_store() version = store.next_version(id) @@ -87,7 +102,7 @@ def put( fname: fa.as_ref() for fname, fa in file_artifacts.items() }, file_artifact_ids={fname: fa.id for fname, fa in file_artifacts.items()}, - bundle_s3_url=bundle_s3_url, + bundle_object_url=bundle_object_url, ) return store.put_document(instance) @@ -127,19 +142,21 @@ def put_bundled( id: str, *, files: dict[str, Path], + prefix_url: str | None = None, s3_url: str | None = None, ) -> "FileArtifactUniverse": - """Upload ``files`` (bundle key -> local path) under ``s3_url`` and register them as one universe. - Without ``s3_url``, each call writes under a prefix of its own (``ArtifactStore.attempt_prefix``).""" + """Upload ``files`` (bundle key -> local path) under ``prefix_url`` and register them as one universe. + Without ``prefix_url``, each call writes under a prefix of its own (``ArtifactStore.attempt_prefix``).""" from agent_env.artifact.artifacts.file import FileArtifact from agent_env.artifact.store import get_artifact_store + prefix_url = renamed_keyword("FileArtifactUniverse.put_bundled", "prefix_url", prefix_url, "s3_url", s3_url) if not files: raise ValueError("files must be non-empty") - if s3_url is None: - s3_url = get_artifact_store().attempt_prefix(cls.model_fields["type"].default, id) - if not s3_url.endswith("/"): - s3_url += "/" + if prefix_url is None: + prefix_url = get_artifact_store().attempt_prefix(cls.model_fields["type"].default, id) + if not prefix_url.endswith("/"): + prefix_url += "/" file_artifacts: dict[str, FileArtifact] = {} for rel_path, local_path in files.items(): @@ -152,12 +169,12 @@ def put_bundled( id=derive_id(id, path_hash), description=f"Bundled file '{rel_path}' of FileArtifactUniverse '{id}'", file_path=str(local_path), - object_url=s3_url + rel_path, + object_url=prefix_url + rel_path, filename=rel_path.rsplit("/", 1)[-1], ) file_artifacts[rel_path] = fa - return cls.put(id=id, file_artifacts=file_artifacts, bundle_s3_url=s3_url) + return cls.put(id=id, file_artifacts=file_artifacts, bundle_object_url=prefix_url) @classmethod def from_toml(cls, data: dict, ctx: AuthoringContext) -> "FileArtifactUniverse": @@ -170,26 +187,30 @@ def put_existing( cls, id: str, *, - s3_url: str, + prefix_url: str | None = None, + s3_url: str | None = None, ) -> "FileArtifactUniverse": """Wrap files that already exist under an object-store prefix as a universe. Unlike ``put_bundled`` (which uploads local files), this lists the - objects already present under *s3_url* and registers each one as a + objects already present under *prefix_url* and registers each one as a ``FileArtifact`` via ``FileArtifact.put_existing``. No bytes are downloaded or re-uploaded. Useful when another process (e.g. an agent's snapshot extension) has already written the files. """ from agent_env.artifact.artifacts.file import FileArtifact - if not s3_url.endswith("/"): - s3_url += "/" + prefix_url = renamed_keyword("FileArtifactUniverse.put_existing", "prefix_url", prefix_url, "s3_url", s3_url) + if prefix_url is None: + raise TypeError("FileArtifactUniverse.put_existing() missing required keyword argument: 'prefix_url'") + if not prefix_url.endswith("/"): + prefix_url += "/" - store = get_config().get_object_store_at(s3_url) + store = get_config().get_object_store_at(prefix_url) file_artifacts: dict[str, FileArtifact] = {} - for object_url in store.list_at(s3_url): - rel_path = object_url[len(s3_url):] + for object_url in store.list_at(prefix_url): + rel_path = object_url[len(prefix_url):] if not rel_path: continue path_hash = hashlib.sha256(rel_path.encode("utf-8")).hexdigest()[:16] @@ -201,6 +222,6 @@ def put_existing( file_artifacts[rel_path] = fa if not file_artifacts: - raise ValueError(f"No files found under prefix {s3_url!r}") + raise ValueError(f"No files found under prefix {prefix_url!r}") - return cls.put(id=id, file_artifacts=file_artifacts, bundle_s3_url=s3_url) + return cls.put(id=id, file_artifacts=file_artifacts, bundle_object_url=prefix_url) diff --git a/src/agent_env/artifact/artifacts/skill.py b/src/agent_env/artifact/artifacts/skill.py index b9275794..45c679b4 100644 --- a/src/agent_env/artifact/artifacts/skill.py +++ b/src/agent_env/artifact/artifacts/skill.py @@ -12,15 +12,16 @@ import re import tempfile from pathlib import Path -from typing import TYPE_CHECKING, Iterator, Literal, Optional +from typing import TYPE_CHECKING, Any, Iterator, Literal, Optional import yaml -from pydantic import Field +from pydantic import ConfigDict, Field, model_serializer -from agent_env.artifact.artifact import Artifact +from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.config import get_config from agent_env.store.base import ObjectNotFoundError from agent_env.store.ids import derive_id, key_segment +from agent_env.utils.deprecation import renamed_keyword if TYPE_CHECKING: from agent_env.artifact.artifacts.file_artifact_universe import FileArtifactUniverse @@ -35,6 +36,8 @@ class SkillArtifact(Artifact): + model_config = ConfigDict(populate_by_name=True) + type: Literal["skill"] = "skill" skill_files_id: str = Field(description="ID of the FileArtifactUniverse bundling every file in the skill") @@ -48,18 +51,27 @@ class SkillArtifact(Artifact): allowed_tools: Optional[str] = Field(default=None) skill_metadata: Optional[dict[str, str]] = Field(default=None) + @model_serializer(mode="wrap") + def _serialize(self, handler: Any) -> dict[str, Any]: + # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. + data = handler(self) + _write_twin(data, "skill_s3_url", "skill_object_url", self.skill_object_url) + return data + @classmethod def validate( cls, *, skill_md: Optional[bytes] = None, + object_url: Optional[str] = None, s3_url: Optional[str] = None, expected_name: str, ) -> None: - if (skill_md is None) == (s3_url is None): - raise ValueError("specify exactly one of skill_md or s3_url") - if s3_url is not None: - skill_md = _fetch_skill_md(s3_url) + object_url = renamed_keyword("SkillArtifact.validate", "object_url", object_url, "s3_url", s3_url) + if (skill_md is None) == (object_url is None): + raise ValueError("specify exactly one of skill_md or object_url") + if object_url is not None: + skill_md = _fetch_skill_md(object_url) frontmatter, _body = _parse_skill_md(skill_md) _validate_frontmatter(frontmatter, expected_name=expected_name) @@ -84,19 +96,19 @@ def put(cls, id: str, *, skill_dir: Path) -> "SkillArtifact": store = get_artifact_store() version = store.next_version(id) key = f"{get_config().get_artifact_key_prefix()}artifacts/skill/{key_segment(id)}/{version}/" - skill_s3_url = get_config().get_object_store_for(id).object_url(key) + skill_object_url = get_config().get_object_store_for(id).object_url(key) universe = FileArtifactUniverse.put_bundled( id=derive_id(id, "files"), files=files, - s3_url=skill_s3_url, + prefix_url=skill_object_url, ) instance = cls( id=id, version=version, skill_files_id=universe.id, - skill_s3_url=skill_s3_url, + skill_object_url=skill_object_url, agent_skills_spec_version=AGENT_SKILLS_SPEC_VERSION, skill_name=frontmatter["name"], description=frontmatter["description"], diff --git a/src/agent_env/cli/artifact/file_artifact_universe.py b/src/agent_env/cli/artifact/file_artifact_universe.py index 9702344e..6d070aa3 100644 --- a/src/agent_env/cli/artifact/file_artifact_universe.py +++ b/src/agent_env/cli/artifact/file_artifact_universe.py @@ -86,7 +86,7 @@ def put_bundled(universe_id: str, file_dir: Path, s3_url: Optional[str]): click.echo(f"Uploading {len(files)} file(s)" + (f" under {s3_url}..." if s3_url else "...")) for rel in files: click.echo(f" {rel}") - universe = FileArtifactUniverse.put_bundled(id=universe_id, files=files, s3_url=s3_url) + universe = FileArtifactUniverse.put_bundled(id=universe_id, files=files, prefix_url=s3_url) click.echo( f"Created FileArtifactUniverse: id={universe.id} version={universe.version} " f"file_count={len(universe.file_artifact_ids)} bundle_s3_url={universe.bundle_object_url}" diff --git a/src/agent_env/env/snapshot_store.py b/src/agent_env/env/snapshot_store.py index 6de95905..279abdd7 100644 --- a/src/agent_env/env/snapshot_store.py +++ b/src/agent_env/env/snapshot_store.py @@ -140,7 +140,7 @@ async def create(cls, instance_id: str, on_progress: ProgressCallback | None = N log("check_changelog", f"Changelog: {'clean' if is_clean else 'dirty'}", 30) log("snapshot_db", "Exporting servicedb data...", 40) - tar_gz_s3_url = await _snapshot_servicedb(multi_env._sandbox, env_id, universe_id, log) + tar_gz_object_url = await _snapshot_servicedb(multi_env._sandbox, env_id, universe_id, log) log("store_artifact", "Storing Docker image artifact...", 70) logger.info("Storing snapshot as DockerImageArtifact...") @@ -152,7 +152,7 @@ async def create(cls, instance_id: str, on_progress: ProgressCallback | None = N f"universe={universe_id} v{universe_version}" ), image_name=image_tag, - tar_gz_s3_url=tar_gz_s3_url, + tar_gz_object_url=tar_gz_object_url, ) logger.info(f"Artifact: id={artifact.id} version={artifact.version}") log("store_artifact", f"Artifact: id={artifact.id} version={artifact.version}", 80) diff --git a/src/agent_env/explorer/routers/common.py b/src/agent_env/explorer/routers/common.py index dcd144a6..f654aabb 100644 --- a/src/agent_env/explorer/routers/common.py +++ b/src/agent_env/explorer/routers/common.py @@ -73,7 +73,7 @@ def _enrich_universe(doc: dict, store, collection: str) -> dict: "artifact_id": rid, "version": ver, "content_type": fa.get("content_type") if fa else None, - "object_url": fa.get("s3_url") if fa else None, + "object_url": (fa.get("object_url") or fa.get("s3_url")) if fa else None, }) doc["files"] = files elif t in _UNIVERSE_ENVIRONMENT_TYPES: diff --git a/src/agent_env/providers/sandbox_providers/sandbox.py b/src/agent_env/providers/sandbox_providers/sandbox.py index 86030ae8..86985046 100644 --- a/src/agent_env/providers/sandbox_providers/sandbox.py +++ b/src/agent_env/providers/sandbox_providers/sandbox.py @@ -15,7 +15,6 @@ import tempfile import time import uuid -import warnings import weakref from abc import ABC, abstractmethod from dataclasses import dataclass, replace @@ -23,6 +22,7 @@ from typing import IO, TYPE_CHECKING, Any, AsyncIterator, Callable, Iterable, Optional from agent_env.config import get_config +from agent_env.utils.deprecation import warn_deprecated from agent_env.utils.paths import validate_relative_filename if TYPE_CHECKING: @@ -181,9 +181,7 @@ async def write_file_from_object(self, object_url: str, destination_path: str) - async def write_file_from_s3(self, s3_url: str, destination_path: str) -> None: """Deprecated: ``write_file_from_object``.""" - warnings.warn( - "Sandbox.write_file_from_s3 is deprecated; use write_file_from_object", DeprecationWarning, stacklevel=2 - ) + warn_deprecated("Sandbox.write_file_from_s3", "write_file_from_object", kind="method") await self.write_file_from_object(s3_url, destination_path) async def write_file_from_url(self, url: str, destination_path: str) -> None: @@ -342,7 +340,7 @@ async def load_object_file(self, object_url: str, destination_path: str) -> None async def load_s3_file(self, s3_url: str, destination_path: str) -> None: """Deprecated: ``load_object_file``.""" - warnings.warn("VmSandbox.load_s3_file is deprecated; use load_object_file", DeprecationWarning, stacklevel=2) + warn_deprecated("VmSandbox.load_s3_file", "load_object_file", kind="method") await self.load_object_file(s3_url, destination_path) async def _download_object_to_vm(self, object_url: str, vm_path: str) -> None: diff --git a/src/agent_env/task_step/snapshot_utils/agent_state_capture.py b/src/agent_env/task_step/snapshot_utils/agent_state_capture.py index fa8c4182..0ab32b82 100644 --- a/src/agent_env/task_step/snapshot_utils/agent_state_capture.py +++ b/src/agent_env/task_step/snapshot_utils/agent_state_capture.py @@ -119,7 +119,7 @@ async def capture_workspace( # Unserialized — a concurrent version race on one artifact id is # `ArtifactStore.put_document`'s retry to absorb. universe = await asyncio.to_thread( - FileArtifactUniverse.put_existing, id=artifact_id, s3_url=capture_prefix + FileArtifactUniverse.put_existing, id=artifact_id, prefix_url=capture_prefix ) # `put_existing` always sets it, but the field is Optional on the artifact, and a # row carrying None here would read as an ungradable capture rather than an error. diff --git a/src/agent_env/task_step/task_steps/add_skills.py b/src/agent_env/task_step/task_steps/add_skills.py index 403d83b8..b82dc4f9 100644 --- a/src/agent_env/task_step/task_steps/add_skills.py +++ b/src/agent_env/task_step/task_steps/add_skills.py @@ -63,7 +63,7 @@ def validate(self) -> None: if self.skill_artifact_id is not None: return if self.s3_url is not None: - SkillArtifact.validate(s3_url=self.s3_url, expected_name=self.name) + SkillArtifact.validate(object_url=self.s3_url, expected_name=self.name) return SkillArtifact.validate(skill_md=self.to_skill_md().encode("utf-8"), expected_name=self.name) diff --git a/src/agent_env/task_step/task_steps/collect_artifacts.py b/src/agent_env/task_step/task_steps/collect_artifacts.py index 9fefa90b..06384e63 100644 --- a/src/agent_env/task_step/task_steps/collect_artifacts.py +++ b/src/agent_env/task_step/task_steps/collect_artifacts.py @@ -730,7 +730,7 @@ async def _collect_via_controller(self, context, store, artifact_id, version): try: # The upload owns the file from here: a cancel must not remove it under the upload. - s3_url = await finish_on_thread( + object_url = await finish_on_thread( functools.partial( _upload_file, store, local_path, artifact_id, version, object_name, content_type, ), @@ -738,11 +738,11 @@ async def _collect_via_controller(self, context, store, artifact_id, version): if_never_run=functools.partial(_remove, local_path), ) - collected[key] = s3_url + collected[key] = object_url self._register_file_artifact( - store, file_artifacts, artifact_id, key, object_name, content_type, s3_url, context, + store, file_artifacts, artifact_id, key, object_name, content_type, object_url, context, ) - logger.info(f"Collected {source_path} -> {s3_url} ({size} bytes) via controller") + logger.info(f"Collected {source_path} -> {object_url} ({size} bytes) via controller") except Exception as e: logger.warning(f"Failed to collect {source_path} via controller: {e}") hard_failures.append(source_path) @@ -931,15 +931,15 @@ async def _collect_items(self, provider, sandbox, container, items, context, sto ext = os.path.splitext(object_name)[1] content_type = _CONTENT_TYPES.get(ext, "application/octet-stream") - s3_url = await self._collect_file( + object_url = await self._collect_file( sandbox, container, source_path, object_name, store, artifact_id, version, content_type, ) - collected[key] = s3_url + collected[key] = object_url self._register_file_artifact( - store, file_artifacts, artifact_id, key, object_name, content_type, s3_url, context, + store, file_artifacts, artifact_id, key, object_name, content_type, object_url, context, ) - logger.info(f"Collected {source_path} -> {s3_url} ({file_size} bytes)") + logger.info(f"Collected {source_path} -> {object_url} ({file_size} bytes)") except Exception as e: logger.warning(f"Failed to collect {source_path}: {e}") @@ -956,7 +956,7 @@ async def _collect_items(self, provider, sandbox, container, items, context, sto await provider.close() return collected, file_artifacts, hard_failures - def _register_file_artifact(self, store, file_artifacts, artifact_id, key, object_name, content_type, s3_url, context): + def _register_file_artifact(self, store, file_artifacts, artifact_id, key, object_name, content_type, object_url, context): """Register a FileArtifact document pointing at the already-uploaded S3 object. We do NOT re-upload the bytes — FileArtifact.object_url is just a URL reference, and the object is already in S3 under the collected_artifacts @@ -973,7 +973,7 @@ def _register_file_artifact(self, store, file_artifacts, artifact_id, key, objec description=f"Collected artifact '{object_name}' from task instance {context.instance_id or artifact_id}", filename=object_name.split("/")[-1], content_type=content_type, - s3_url=s3_url, + object_url=object_url, ) try: store.put_document(fa) diff --git a/src/agent_env/task_step/task_steps/snapshot_env.py b/src/agent_env/task_step/task_steps/snapshot_env.py index d732af65..7e46a258 100644 --- a/src/agent_env/task_step/task_steps/snapshot_env.py +++ b/src/agent_env/task_step/task_steps/snapshot_env.py @@ -597,7 +597,7 @@ def _register_uploaded_bundle(): description=f"State snapshot of {environment_name} from env {env.id}", filename=filename, content_type=content_type, - s3_url=object_url, + object_url=object_url, ) fa = store.put_document(fa) return EnvironmentArtifact.put( diff --git a/src/agent_env/task_step/task_steps/verifiers/run_container_unit_tests_verifier.py b/src/agent_env/task_step/task_steps/verifiers/run_container_unit_tests_verifier.py index 16f3eba7..bac7faf6 100644 --- a/src/agent_env/task_step/task_steps/verifiers/run_container_unit_tests_verifier.py +++ b/src/agent_env/task_step/task_steps/verifiers/run_container_unit_tests_verifier.py @@ -313,14 +313,14 @@ async def execute(self, context: TaskStepContext) -> TaskStepContext: text=stdout, artifact_id=derive_id(base, f"verifier-stdout-{self.id}-{stamp}"), description=f"stdout of {self.verifier_id} from step {self.id}", - s3_url=store.object_url(f"{outputs}/stdout.txt"), + object_url=store.object_url(f"{outputs}/stdout.txt"), ) stderr_artifact = await asyncio.to_thread( self._upload_text_artifact, text=stderr, artifact_id=derive_id(base, f"verifier-stderr-{self.id}-{stamp}"), description=f"stderr of {self.verifier_id} from step {self.id}", - s3_url=store.object_url(f"{outputs}/stderr.txt"), + object_url=store.object_url(f"{outputs}/stderr.txt"), ) # 8. Record on context (shape compatible with aggregate_verifiers). @@ -406,7 +406,7 @@ def _parse_reward(raw: Any) -> Optional[float]: return max(0.0, min(1.0, value)) @staticmethod - def _upload_text_artifact(text: str, artifact_id: str, description: str, s3_url: str): + def _upload_text_artifact(text: str, artifact_id: str, description: str, object_url: str): from agent_env.artifact.artifacts.file import FileArtifact with tempfile.NamedTemporaryFile(mode="w", suffix=".txt", delete=False) as tmp: @@ -417,7 +417,7 @@ def _upload_text_artifact(text: str, artifact_id: str, description: str, s3_url: id=artifact_id, description=description, file_path=tmp_path, - object_url=s3_url, + object_url=object_url, ) finally: try: diff --git a/src/agent_env/utils/deprecation.py b/src/agent_env/utils/deprecation.py index f05ba798..76bc62f4 100644 --- a/src/agent_env/utils/deprecation.py +++ b/src/agent_env/utils/deprecation.py @@ -1,4 +1,4 @@ -"""Deprecation shims for the service_* -> environment_* rename. +"""Deprecation shims for renamed names: the service_* -> environment_* rename and the S3-named ones. Two things make this less trivial than ``warnings.warn``: @@ -48,6 +48,17 @@ def warn_deprecated(old: str, new: str, *, kind: str = "symbol", stacklevel: int ) +def renamed_keyword(owner: str, new: str, new_value: Any, old: str, old_value: Any) -> Any: + """The value of keyword ``new``, taking the deprecated keyword ``old`` in its place with a warning. Passing both + is a TypeError, as a repeated keyword is.""" + if old_value is None: + return new_value + if new_value is not None: + raise TypeError(f"{owner}() got both {new}= and its deprecated spelling {old}=") + warn_deprecated(f"{owner}({old}=)", f"{new}=", kind="keyword", stacklevel=4) + return old_value + + def deprecated_names(mapping: dict[str, Any], *, kind: str = "symbol") -> Callable[[str], Any]: """Build a module ``__getattr__`` serving ``{old_name: canonical_object}`` with a warning. diff --git a/tst/unit/artifact/test_docker_image_github_push.py b/tst/unit/artifact/test_docker_image_github_push.py index f0426f40..b30fbcc3 100644 --- a/tst/unit/artifact/test_docker_image_github_push.py +++ b/tst/unit/artifact/test_docker_image_github_push.py @@ -118,7 +118,7 @@ async def test_registry_login_and_upload_signing_run_off_the_event_loop(build): async def test_the_build_tarballs_are_uploaded_under_the_fixture_prefix(build, monkeypatch): monkeypatch.setattr(get_config(), "fixture_prefix", "fx") result = await DockerImageArtifact.put_from_github("server", "https://github.com/example/repo/blob/main/Dockerfile") - image, context = result.artifact.tar_gz_s3_url, result.artifact.build_context_s3_url + image, context = result.artifact.tar_gz_object_url, result.artifact.build_context_object_url assert image.startswith("mem://bucket/fx/github-builds/server/") and image.endswith(".tar.gz") assert context.startswith("mem://bucket/fx/github-builds/server/") and context.endswith("-context.tar.gz") @@ -132,8 +132,8 @@ async def test_a_store_that_signs_nothing_gets_the_build_tarballs_copied_off_the result = await DockerImageArtifact.put_from_github(image_id, "https://github.com/example/repo/blob/main/Dockerfile") - assert store.get(result.artifact.tar_gz_s3_url) == vm.files["/tmp/image.tar.gz"] - assert store.get(result.artifact.build_context_s3_url) == vm.files["/tmp/build-context.tar.gz"] + assert store.get(result.artifact.tar_gz_object_url) == vm.files["/tmp/image.tar.gz"] + assert store.get(result.artifact.build_context_object_url) == vm.files["/tmp/build-context.tar.gz"] assert not any("curl" in script for script in vm.scripts) diff --git a/tst/unit/artifact/test_object_url_keys.py b/tst/unit/artifact/test_object_url_keys.py new file mode 100644 index 00000000..0633d773 --- /dev/null +++ b/tst/unit/artifact/test_object_url_keys.py @@ -0,0 +1,112 @@ +"""The S3-named artifact keys and keywords get neutral twins. + +A stored doc keyed either way loads, and every dump writes both keys (the additive wire contract: old readers keep +reading the legacy key). The S3-named keywords of the put helpers still work for one window, warning through +``warn_deprecated``, whose log line is what counts their remaining callers.""" + +import logging + +import pytest + +from agent_env.artifact.artifacts.cli import CliArtifact +from agent_env.artifact.artifacts.docker_image import DockerImageArtifact +from agent_env.artifact.artifacts.file import FileArtifact +from agent_env.artifact.artifacts.file_artifact_universe import FileArtifactUniverse +from agent_env.artifact.artifacts.skill import SkillArtifact + +_CASES = { + "file": (FileArtifact, {"description": "d", "filename": "a.json", "content_type": "application/json"}, + [("s3_url", "object_url")]), + "docker_image": (DockerImageArtifact, {"description": "d", "image_name": "img:1"}, + [("tar_gz_s3_url", "tar_gz_object_url"), ("build_context_s3_url", "build_context_object_url")]), + "file_artifact_universe": (FileArtifactUniverse, {}, [("bundle_s3_url", "bundle_object_url")]), + "skill": (SkillArtifact, {"skill_files_id": "s__files", "agent_skills_spec_version": "0.1", "skill_name": "s", + "description": "d"}, [("skill_s3_url", "skill_object_url")]), + "cli": (CliArtifact, {"cli_files_id": "c__files", "entrypoint": "bin/c", "command_name": "c"}, + [("cli_s3_url", "cli_object_url")]), +} + + +@pytest.mark.parametrize("spelling", ["legacy", "neutral"]) +@pytest.mark.parametrize("case", _CASES.values(), ids=_CASES.keys()) +def test_a_doc_keyed_either_way_loads_and_dumps_both_keys(case, spelling): + cls, fields, pairs = case + urls = {neutral: f"mem://bucket/{neutral}" for _legacy, neutral in pairs} + keyed = {(legacy if spelling == "legacy" else neutral): urls[neutral] for legacy, neutral in pairs} + artifact = cls.model_validate({"id": "x", "version": 1, **fields, **keyed}) + + assert {neutral: getattr(artifact, neutral) for _legacy, neutral in pairs} == urls + for by_alias in (True, False): + dumped = artifact.model_dump(by_alias=by_alias) + assert all(dumped[legacy] == dumped[neutral] == urls[neutral] for legacy, neutral in pairs) + + +def test_an_excluded_field_stays_excluded(): + artifact = FileArtifact(id="x", description="d", filename="a", content_type="t", object_url="mem://b/a") + assert {"s3_url", "object_url"}.isdisjoint(artifact.model_dump(exclude={"object_url"})) + + +def _bundle(tmp_path, local_stores): + path = tmp_path / "a.txt" + path.write_bytes(b"A") + return {"a.txt": path}, local_stores.get_object_store().object_url("bundles/one/") + + +def test_the_universe_helpers_take_their_s3_named_keywords_with_a_warning(local_stores, tmp_path): + files, prefix = _bundle(tmp_path, local_stores) + + with pytest.warns(DeprecationWarning, match=r"put_bundled\(s3_url=\)"): + bundled = FileArtifactUniverse.put_bundled(id="bundled", files=files, s3_url=prefix) + with pytest.warns(DeprecationWarning, match=r"put_existing\(s3_url=\)"): + existing = FileArtifactUniverse.put_existing(id="existing", s3_url=prefix) + with pytest.warns(DeprecationWarning, match=r"FileArtifactUniverse.put\(bundle_s3_url=\)"): + put = FileArtifactUniverse.put(id="put", file_artifacts=bundled.get_file_artifacts(), bundle_s3_url=prefix) + + assert bundled.bundle_object_url == existing.bundle_object_url == prefix.rstrip("/") + "/" + assert put.bundle_object_url == prefix + + +def test_put_tar_takes_its_s3_named_keywords_with_a_warning(local_stores): + store = local_stores.get_object_store() + image, context = store.object_url("images/i.tar.gz"), store.object_url("images/context.tar.gz") + + with pytest.warns(DeprecationWarning) as caught: + artifact = DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_s3_url=image, + build_context_s3_url=context) + + assert (artifact.tar_gz_object_url, artifact.build_context_object_url) == (image, context) + assert [str(w.message).split(" is deprecated")[0] for w in caught] == [ + "DockerImageArtifact.put_tar(tar_gz_s3_url=)", "DockerImageArtifact.put_tar(build_context_s3_url=)" + ] + + +def test_skill_validate_takes_its_s3_named_keyword_with_a_warning(local_stores): + store = local_stores.get_object_store() + store.put("skills/demo/SKILL.md", b"---\nname: demo\ndescription: A demo skill.\n---\nBody\n") + + with pytest.warns(DeprecationWarning, match=r"validate\(s3_url=\)"): + SkillArtifact.validate(s3_url=store.object_url("skills/demo"), expected_name="demo") + + +def test_both_spellings_of_a_keyword_is_an_error(local_stores): + url = local_stores.get_object_store().object_url("images/i.tar.gz") + with pytest.raises(TypeError, match="got both tar_gz_object_url= and its deprecated spelling tar_gz_s3_url="): + DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_object_url=url, tar_gz_s3_url=url) + + +def test_a_required_keyword_is_still_required(): + with pytest.raises(TypeError, match="missing required keyword argument: 'tar_gz_object_url'"): + DockerImageArtifact.put_tar("img", description="d", image_name="img:1") + with pytest.raises(TypeError, match="missing required keyword argument: 'prefix_url'"): + FileArtifactUniverse.put_existing(id="u") + + +def test_a_deprecated_keyword_is_counted_in_the_log(local_stores, caplog): + url = local_stores.get_object_store().object_url("images/i.tar.gz") + with caplog.at_level(logging.WARNING, logger="agent_env.utils.deprecation"), pytest.warns(DeprecationWarning): + DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_s3_url=url) + + [record] = [r for r in caplog.records if getattr(r, "event", None) == "agent_env_deprecated_symbol"] + assert (record.deprecated_symbol, record.replacement, record.kind) == ( + "DockerImageArtifact.put_tar(tar_gz_s3_url=)", "tar_gz_object_url=", "keyword" + ) diff --git a/tst/unit/conftest.py b/tst/unit/conftest.py index e8583a78..beaa828a 100644 --- a/tst/unit/conftest.py +++ b/tst/unit/conftest.py @@ -23,6 +23,7 @@ from agent_env.store import LocalFilesystemObjectStore from agent_env.store.document_store.sqlite_document_store import LocalSqliteDocumentStore from agent_env.store.routing import enable_namespace_routing +from agent_env.utils.deprecation import reset_deprecation_state _PLACEHOLDER_AWS_ENV = { "AWS_ACCESS_KEY_ID": "testing", @@ -54,6 +55,12 @@ def _docker_answers(monkeypatch): monkeypatch.setattr("agent_env.env.bootstrap.docker_unreachable", lambda: None) +@pytest.fixture(autouse=True) +def deprecations_fire_again(): + """``warn_deprecated`` warns once per process; each test sees its own first use.""" + reset_deprecation_state() + + @pytest.fixture(autouse=True) def isolated_state_root(tmp_path_factory, monkeypatch): """A fresh per-user state root for every unit test, overriding the run's.""" diff --git a/tst/unit/explorer/explorer_app_test.py b/tst/unit/explorer/explorer_app_test.py index 62972764..0035c9cf 100644 --- a/tst/unit/explorer/explorer_app_test.py +++ b/tst/unit/explorer/explorer_app_test.py @@ -150,9 +150,11 @@ def test_type_filter_matches_documents_stored_under_an_aliased_spelling(tmp_path reset_config() -def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_path, monkeypatch): +@pytest.mark.parametrize("url_key", ["s3_url", "object_url"]) +def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_path, monkeypatch, url_key): """Enrichment picks its branch from the type, so a renamed universe must still - resolve its refs — otherwise the detail response silently loses `files`.""" + resolve its refs — otherwise the detail response silently loses `files`. Its files + keep their url whichever key their documents store it under.""" cfg = tmp_path / ".agentenv" / "config.toml" cfg.parent.mkdir(parents=True, exist_ok=True) cfg.write_text( @@ -166,7 +168,7 @@ def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_p set_document_store(store) set_runner(LocalRunner(workers=1)) run_store.ensure_indexes() - store.insert("artifacts", {"id": "f1", "version": 1, "type": "file", "s3_url": "s3://b/a.txt", + store.insert("artifacts", {"id": "f1", "version": 1, "type": "file", url_key: "s3://b/a.txt", "content_type": "text/plain", "created_at_utc": "2026-01-01T00:00:00Z"}) store.insert("artifacts", {"id": "u1", "version": 1, "type": "old_universe", "file_artifact_refs": {"a.txt": {"id": "f1", "version": 1}}, diff --git a/tst/unit/providers/sandbox_providers/vm_sandbox_test.py b/tst/unit/providers/sandbox_providers/vm_sandbox_test.py index 3701e725..511ea945 100644 --- a/tst/unit/providers/sandbox_providers/vm_sandbox_test.py +++ b/tst/unit/providers/sandbox_providers/vm_sandbox_test.py @@ -1,6 +1,7 @@ import asyncio import hashlib import io +import logging import threading import time from concurrent.futures import ThreadPoolExecutor @@ -442,14 +443,18 @@ async def test_a_failed_docker_cp_raises_with_its_stderr(): @pytest.mark.asyncio -async def test_the_s3_named_methods_are_deprecated_aliases(signing_store): +async def test_the_s3_named_methods_are_deprecated_aliases(signing_store, caplog): + """They delegate, and each use is counted: the log line, not the warning, is what reaches production logs.""" sandbox = _RecordingVmSandbox() - with pytest.warns(DeprecationWarning, match="use load_object_file"): - await sandbox.load_s3_file("s3://bucket/a.json", "/tmp/a.json") - with pytest.warns(DeprecationWarning, match="use write_file_from_object"): - await sandbox.write_file_from_s3("s3://bucket/b.json", "/work/b.json") + with caplog.at_level(logging.WARNING, logger="agent_env.utils.deprecation"): + with pytest.warns(DeprecationWarning, match="use load_object_file"): + await sandbox.load_s3_file("s3://bucket/a.json", "/tmp/a.json") + with pytest.warns(DeprecationWarning, match="use write_file_from_object"): + await sandbox.write_file_from_s3("s3://bucket/b.json", "/work/b.json") assert any("https://signed/a.json" in s for s in sandbox.scripts) assert any("https://signed/b.json" in s for s in sandbox.scripts) + counted = [r.deprecated_symbol for r in caplog.records if getattr(r, "event", None) == "agent_env_deprecated_symbol"] + assert counted == ["VmSandbox.load_s3_file", "Sandbox.write_file_from_s3"] @pytest.mark.asyncio diff --git a/tst/unit/serialization/golden/artifact/cli.json b/tst/unit/serialization/golden/artifact/cli.json index 4838dc65..0efa25b6 100644 --- a/tst/unit/serialization/golden/artifact/cli.json +++ b/tst/unit/serialization/golden/artifact/cli.json @@ -1,5 +1,6 @@ { "cli_files_id": "slack-cli-files", + "cli_object_url": "s3://artifact-bucket/artifacts/cli/slack-cli/3/", "cli_s3_url": "s3://artifact-bucket/artifacts/cli/slack-cli/3/", "command_name": "slack", "entrypoint": "bin/slack", diff --git a/tst/unit/serialization/golden/artifact/docker_image.json b/tst/unit/serialization/golden/artifact/docker_image.json index 66a2c101..09ee9103 100644 --- a/tst/unit/serialization/golden/artifact/docker_image.json +++ b/tst/unit/serialization/golden/artifact/docker_image.json @@ -1,8 +1,10 @@ { + "build_context_object_url": null, "build_context_s3_url": null, "description": "slack-mcp:v5 image", "id": "slack-mcp-image", "image_name": "slack-mcp:v5", + "tar_gz_object_url": "s3://artifact-bucket/artifacts/docker_image/slack-mcp-image/5/image.tar.gz", "tar_gz_s3_url": "s3://artifact-bucket/artifacts/docker_image/slack-mcp-image/5/image.tar.gz", "type": "docker_image", "version": 5 diff --git a/tst/unit/serialization/golden/artifact/file.json b/tst/unit/serialization/golden/artifact/file.json index cb84d5fa..5ac03b35 100644 --- a/tst/unit/serialization/golden/artifact/file.json +++ b/tst/unit/serialization/golden/artifact/file.json @@ -3,6 +3,7 @@ "description": "Slack universe seed data", "filename": "slack.json", "id": "slack-universe-data", + "object_url": "s3://artifact-bucket/artifacts/file/slack-universe-data/2/slack.json", "s3_url": "s3://artifact-bucket/artifacts/file/slack-universe-data/2/slack.json", "type": "file", "version": 2 diff --git a/tst/unit/serialization/golden/artifact/file_artifact_universe.json b/tst/unit/serialization/golden/artifact/file_artifact_universe.json index e48f9999..325add64 100644 --- a/tst/unit/serialization/golden/artifact/file_artifact_universe.json +++ b/tst/unit/serialization/golden/artifact/file_artifact_universe.json @@ -1,4 +1,5 @@ { + "bundle_object_url": "s3://artifact-bucket/artifacts/file_artifact_universe/tutorial-files/4/bundle.tar.gz", "bundle_s3_url": "s3://artifact-bucket/artifacts/file_artifact_universe/tutorial-files/4/bundle.tar.gz", "file_artifact_ids": { "README.md": "readme-file" diff --git a/tst/unit/serialization/golden/artifact/skill.json b/tst/unit/serialization/golden/artifact/skill.json index 08578cad..2cb49bfb 100644 --- a/tst/unit/serialization/golden/artifact/skill.json +++ b/tst/unit/serialization/golden/artifact/skill.json @@ -10,6 +10,7 @@ "category": "documents" }, "skill_name": "pdf-filler", + "skill_object_url": "s3://artifact-bucket/artifacts/skill/pdf-filler/2/", "skill_s3_url": "s3://artifact-bucket/artifacts/skill/pdf-filler/2/", "type": "skill", "version": 2 diff --git a/tst/unit/serialization/test_golden_wire_format.py b/tst/unit/serialization/test_golden_wire_format.py index 66f0e0ec..d9994fef 100644 --- a/tst/unit/serialization/test_golden_wire_format.py +++ b/tst/unit/serialization/test_golden_wire_format.py @@ -333,7 +333,16 @@ def test_website_env_to_dict_writes_both_name_spellings_and_no_version_key(): assert "environment_version" not in doc -_AE3_TWINS = {"environment_name": "service_name", "environment_artifact_refs": "service_artifact_refs"} +_AE3_TWINS = { + "environment_name": "service_name", + "environment_artifact_refs": "service_artifact_refs", + "object_url": "s3_url", + "tar_gz_object_url": "tar_gz_s3_url", + "build_context_object_url": "build_context_s3_url", + "bundle_object_url": "bundle_s3_url", + "skill_object_url": "skill_s3_url", + "cli_object_url": "cli_s3_url", +} @pytest.mark.parametrize("wire,keys", [(_artifact_wire, ARTIFACT_FIXTURES), (_env_wire, ENV_FIXTURES)]) diff --git a/tst/unit/store/derived_ids_test.py b/tst/unit/store/derived_ids_test.py index c509aef1..1e2d6f4b 100644 --- a/tst/unit/store/derived_ids_test.py +++ b/tst/unit/store/derived_ids_test.py @@ -167,7 +167,7 @@ def test_an_env_snapshot_is_named_after_its_env(local_stores, tmp_path, monkeypa image = _written_by(lambda: EnvSnapshot.create("inst-1"), monkeypatch, DockerImageArtifact, "put_tar") assert (image["id"], image["image_name"]) == (artifact_id, image_tag) - assert re.search(rf"/{re.escape(key)}-[0-9a-f]{{16}}\.tar\.gz$", image["tar_gz_s3_url"]) + assert re.search(rf"/{re.escape(key)}-[0-9a-f]{{16}}\.tar\.gz$", image["tar_gz_object_url"]) assert f"docker build --platform linux/amd64 -t {image_tag} /tmp/snapshot-build" in sandbox.scripts @@ -249,9 +249,9 @@ def test_verifier_outputs_are_named_and_keyed_after_the_run(local_stores, monkey monkeypatch.setattr(verifier_module.uuid, "uuid4", lambda: uuid.UUID(hex="0123456789ab" + "0" * 20)) uploads: list[tuple[str, str]] = [] - def record(text, artifact_id, description, s3_url): - uploads.append((artifact_id, s3_url)) - return SimpleNamespace(id=artifact_id, version=1, object_url=s3_url) + def record(text, artifact_id, description, object_url): + uploads.append((artifact_id, object_url)) + return SimpleNamespace(id=artifact_id, version=1, object_url=object_url) monkeypatch.setattr(verifier_module.RunContainerUnitTestsVerifierTaskStep, "_upload_text_artifact", staticmethod(record)) ctx = TaskStepContext(instance_id=instance_id) diff --git a/tst/unit/store/local_backends_composition_test.py b/tst/unit/store/local_backends_composition_test.py index 12966764..dba68db6 100644 --- a/tst/unit/store/local_backends_composition_test.py +++ b/tst/unit/store/local_backends_composition_test.py @@ -70,13 +70,18 @@ def test_file_artifact_put_existing(local_stores, tmp_path): assert FileArtifact.get("existing").load() == b"already-here" -@pytest.mark.parametrize("url", ["fake://home", "fake://home/", "fake://home/dir/"]) -def test_put_existing_refuses_a_url_that_names_no_object(local_stores, url): +@pytest.mark.parametrize("url,refusal", [ + ("fake://home", "does not exist"), + ("fake://home/", "not a prefix"), + ("fake://home/dir/", "not a prefix"), +]) +def test_put_existing_refuses_a_url_that_names_no_object(local_stores, url, refusal): + """A prefix is refused by its trailing slash; anything else is left to the store, which holds no object there.""" store = FakeObjectStore() store.put("dir/x.bin", b"v") set_object_store(store) - with pytest.raises(ValueError, match="not a prefix"): + with pytest.raises(ValueError, match=refusal): FileArtifact.put_existing(id="prefix", description="d", object_url=url) @@ -123,7 +128,7 @@ def test_universe_put_bundled(local_stores, tmp_path): } prefix = store.object_url("artifacts/universe/bundled/1/") - uni = FileArtifactUniverse.put_bundled(id="bundled", files=files, s3_url=prefix) + uni = FileArtifactUniverse.put_bundled(id="bundled", files=files, prefix_url=prefix) loaded = {rel: fa.load() for rel, fa in FileArtifactUniverse.get(uni.id).get_file_artifacts().items()} assert loaded == {"a.txt": b"AAA", "sub/b.txt": b"BBB"} @@ -135,7 +140,7 @@ def test_universe_put_existing_lists_via_list_at(local_stores, tmp_path): store.put_file("artifacts/universe/existing/1/nested/two.txt", _write(tmp_path, "two.txt", b"2")) prefix = store.object_url("artifacts/universe/existing/1/") - uni = FileArtifactUniverse.put_existing(id="uni-existing", s3_url=prefix) + uni = FileArtifactUniverse.put_existing(id="uni-existing", prefix_url=prefix) loaded = {rel: fa.load() for rel, fa in FileArtifactUniverse.get(uni.id).get_file_artifacts().items()} assert loaded == {"one.txt": b"1", "nested/two.txt": b"2"} @@ -147,7 +152,7 @@ def test_a_skill_prefix_without_skill_md_is_refused_through_the_stores_not_found set_object_store(store) with pytest.raises(ValueError, match="SKILL.md not found"): - SkillArtifact.validate(s3_url=store.object_url("skills/demo"), expected_name="demo") + SkillArtifact.validate(object_url=store.object_url("skills/demo"), expected_name="demo") def test_skill_artifact_put_and_download(local_stores, tmp_path): @@ -341,7 +346,7 @@ def test_get_many_writes_each_local_id_into_its_own_encoded_directory(local_stor store = local_stores.get_object_store() for uid, name in (("@local/t/A", "1"), ("@local/t/A/1", "f")): prefix = store.object_url(f"artifacts/file_artifact_universe/{key_segment(uid)}/1/") - FileArtifactUniverse.put_bundled(id=uid, files={name: _write(tmp_path, f"src-{name}", b"x")}, s3_url=prefix) + FileArtifactUniverse.put_bundled(id=uid, files={name: _write(tmp_path, f"src-{name}", b"x")}, prefix_url=prefix) out = tmp_path / "out" res = CliRunner().invoke( diff --git a/tst/unit/store/local_id_acceptance_test.py b/tst/unit/store/local_id_acceptance_test.py index f8cf94ea..3a8d1cc6 100644 --- a/tst/unit/store/local_id_acceptance_test.py +++ b/tst/unit/store/local_id_acceptance_test.py @@ -213,7 +213,7 @@ def stop(self, **kwargs): def test_a_put_validates_an_local_entity_like_any_other(local_stores, monkeypatch): image_url = local_stores.get_object_store().put("artifacts/docker_image/srv-img/1/x.tar.gz", b"x") - image = DockerImageArtifact.put_tar("srv-img", description="d", image_name="reg/srv:v1", tar_gz_s3_url=image_url) + image = DockerImageArtifact.put_tar("srv-img", description="d", image_name="reg/srv:v1", tar_gz_object_url=image_url) MCPServerEnv.put(id="srv", docker_image_artifact=image, environment_name="svc") _validation_runs_nothing(monkeypatch) @@ -285,7 +285,7 @@ def test_a_put_with_an_local_id_builds_and_writes_its_images_under_ids_derived_f lambda dockerfile, context, tag, **kwargs: tags.append(tag)) tarball = local_stores.get_object_store().put("image.tar.gz", b"x") monkeypatch.setattr(DockerImageArtifact, "put", classmethod(lambda cls, id, *, description, image_name, **kwargs: ( - cls.put_tar(id, description=description, image_name=image_name, tar_gz_s3_url=tarball)))) + cls.put_tar(id, description=description, image_name=image_name, tar_gz_object_url=tarball)))) argv = [arg.format(dockerfile=dockerfile) for arg in argv] flag = "--frontend-dockerfile" if "--backend-dockerfile" in argv else "--dockerfile" diff --git a/tst/unit/store/routing_test.py b/tst/unit/store/routing_test.py index f8f29eda..3c11ef69 100644 --- a/tst/unit/store/routing_test.py +++ b/tst/unit/store/routing_test.py @@ -434,9 +434,9 @@ def test_registering_a_bare_artifact_at_a_local_url_is_refused(object_stores): _, local = object_stores url = local.put("k/image.tar.gz", b"x") with pytest.raises(ValueError, match="configured object store"): - DockerImageArtifact.put_tar("bare-image", description="d", image_name="img:v1", tar_gz_s3_url=url) + DockerImageArtifact.put_tar("bare-image", description="d", image_name="img:v1", tar_gz_object_url=url) with pytest.raises(ValueError, match="configured object store"): - FileArtifactUniverse.put("bare-universe", file_artifacts={"a": object()}, bundle_s3_url=local.object_url("k/")) + FileArtifactUniverse.put("bare-universe", file_artifacts={"a": object()}, bundle_object_url=local.object_url("k/")) def test_a_key_both_namespaces_recorded_reads_as_the_readers_own(stores): diff --git a/tst/unit/task_step/capture_stubs.py b/tst/unit/task_step/capture_stubs.py index f5aae855..d18aae25 100644 --- a/tst/unit/task_step/capture_stubs.py +++ b/tst/unit/task_step/capture_stubs.py @@ -50,10 +50,10 @@ def agent_card( class _StubUniverse: - def __init__(self, id: str, version: int, s3_url: str): + def __init__(self, id: str, version: int, prefix_url: str): self.id = id self.version = version - self.bundle_object_url = s3_url + self.bundle_object_url = prefix_url class _StubObjectStore: @@ -153,11 +153,11 @@ def get_artifact_key_prefix(self): class _FAU: @staticmethod - def put_existing(*, id: str, s3_url: str): + def put_existing(*, id: str, prefix_url: str): # Mirrors the real one: version is minted by the store, and the # bundle url is the prefix that was actually written. - rec.universes.append(s3_url) - return _StubUniverse(id=id, version=rec._version, s3_url=s3_url) + rec.universes.append(prefix_url) + return _StubUniverse(id=id, version=rec._version, prefix_url=prefix_url) monkeypatch.setattr(fau_mod, "FileArtifactUniverse", _FAU) diff --git a/tst/unit/task_step/verifiers/run_container_unit_tests_verifier_test.py b/tst/unit/task_step/verifiers/run_container_unit_tests_verifier_test.py index 395d8372..449b4d03 100644 --- a/tst/unit/task_step/verifiers/run_container_unit_tests_verifier_test.py +++ b/tst/unit/task_step/verifiers/run_container_unit_tests_verifier_test.py @@ -221,9 +221,9 @@ def test_stdout_and_stderr_are_kept_under_the_fixture_prefix(local_stores, monke monkeypatch.setattr(sandbox_provider, "get_sandbox_provider", lambda: provider) uploaded: list[str] = [] - def record(text, artifact_id, description, s3_url): - uploaded.append(s3_url) - return SimpleNamespace(id=artifact_id, version=1, object_url=s3_url) + def record(text, artifact_id, description, object_url): + uploaded.append(object_url) + return SimpleNamespace(id=artifact_id, version=1, object_url=object_url) monkeypatch.setattr(Step, "_upload_text_artifact", staticmethod(record)) ctx = TaskStepContext() @@ -243,9 +243,9 @@ def test_stdout_and_stderr_upload_off_the_event_loop(local_stores, monkeypatch): monkeypatch.setattr(sandbox_provider, "get_sandbox_provider", lambda: provider) on_loop: list[bool] = [] - def record(text, artifact_id, description, s3_url): + def record(text, artifact_id, description, object_url): on_loop.append(on_event_loop()) - return SimpleNamespace(id=artifact_id, version=1, object_url=s3_url) + return SimpleNamespace(id=artifact_id, version=1, object_url=object_url) monkeypatch.setattr(Step, "_upload_text_artifact", staticmethod(record)) ctx = TaskStepContext() @@ -268,9 +268,9 @@ def test_runs_and_retries_in_one_second_keep_their_outputs_apart(local_stores, m monkeypatch.setattr(verifier_module.time, "time", lambda: 1_790_000_000.0) uploads: list[tuple[str, str]] = [] - def record(text, artifact_id, description, s3_url): - uploads.append((artifact_id, s3_url)) - return SimpleNamespace(id=artifact_id, version=1, object_url=s3_url) + def record(text, artifact_id, description, object_url): + uploads.append((artifact_id, object_url)) + return SimpleNamespace(id=artifact_id, version=1, object_url=object_url) monkeypatch.setattr(Step, "_upload_text_artifact", staticmethod(record)) From 9e469d9935185e7998ee0d90d0ee495a004d37a9 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 7 Oct 2026 10:26:32 -0700 Subject: [PATCH 2/3] fix(artifact): keep the models' serialization schema, read the legacy key first, and size no S3 bucket - The twin-writing wrap serializers lose their `-> dict[str, Any]` return annotation: pydantic builds the serialization JSON schema from it, so every twinned model (and EnvironmentArtifact / EnvironmentUniverseArtifact, which already had it) served a bare object without its fields to anything rendering that schema, such as a FastAPI response model. - The explorer reads a universe file's s3_url before its object_url, as the model does: while both are written, a raw-doc writer that knows only s3_url would leave the twin stale. - S3ObjectStore treats a URL without a key as no object instead of sending S3 an empty-key HEAD, which botocore refuses as a malformed request; put_existing on a bucket URL is refused as "does not exist" again rather than with a ParamValidationError. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/agent_env/artifact/artifacts/cli.py | 3 ++- src/agent_env/artifact/artifacts/docker_image.py | 3 ++- src/agent_env/artifact/artifacts/environment.py | 3 ++- .../artifact/artifacts/environment_universe.py | 3 ++- src/agent_env/artifact/artifacts/file.py | 3 ++- .../artifact/artifacts/file_artifact_universe.py | 3 ++- src/agent_env/artifact/artifacts/skill.py | 3 ++- src/agent_env/explorer/routers/common.py | 3 ++- .../store/object_store/s3_object_store.py | 2 ++ tst/unit/artifact/test_object_url_keys.py | 16 ++++++++++++++++ tst/unit/explorer/explorer_app_test.py | 12 ++++++++---- tst/unit/store/s3_object_store_test.py | 15 ++++++++++++++- 12 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/agent_env/artifact/artifacts/cli.py b/src/agent_env/artifact/artifacts/cli.py index 4e0362e8..18c2935e 100644 --- a/src/agent_env/artifact/artifacts/cli.py +++ b/src/agent_env/artifact/artifacts/cli.py @@ -34,8 +34,9 @@ class CliArtifact(Artifact): env_id: Optional[str] = Field(default=None, description="Source env id this CLI was generated from, if any") env_version: Optional[int] = Field(default=None, description="Source env version this CLI was generated from, if any") + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. data = handler(self) _write_twin(data, "cli_s3_url", "cli_object_url", self.cli_object_url) diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index 7632cc67..0991512e 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/src/agent_env/artifact/artifacts/docker_image.py @@ -75,8 +75,9 @@ class DockerImageArtifact(Artifact): tar_gz_object_url: str = Field(alias="tar_gz_s3_url", description="Object-store locator of the tar.gz file") build_context_object_url: str | None = Field(default=None, alias="build_context_s3_url", description="Object-store locator of the build context tar.gz") + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # Dual-write, from the attributes: `alias=` emits one spelling, which one depending on the caller's `by_alias`. data = handler(self) _write_twin(data, "tar_gz_s3_url", "tar_gz_object_url", self.tar_gz_object_url) diff --git a/src/agent_env/artifact/artifacts/environment.py b/src/agent_env/artifact/artifacts/environment.py index 838f40e4..c8819521 100644 --- a/src/agent_env/artifact/artifacts/environment.py +++ b/src/agent_env/artifact/artifacts/environment.py @@ -44,8 +44,9 @@ class EnvironmentArtifact(Artifact): description="Legacy unpinned wrapped-FileArtifact id", ) + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # `@model_serializer(mode="wrap")` is the only hook invoked by both # `model_dump` and the Rust-implemented `model_dump_json`. data = handler(self) diff --git a/src/agent_env/artifact/artifacts/environment_universe.py b/src/agent_env/artifact/artifacts/environment_universe.py index d427888a..684603eb 100644 --- a/src/agent_env/artifact/artifacts/environment_universe.py +++ b/src/agent_env/artifact/artifacts/environment_universe.py @@ -63,8 +63,9 @@ class EnvironmentUniverseArtifact(Universe): description="Legacy unpinned metadata FileArtifact ids", ) + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # `@model_serializer(mode="wrap")` is the only hook invoked by both # `model_dump` and the Rust-implemented `model_dump_json`. data = handler(self) diff --git a/src/agent_env/artifact/artifacts/file.py b/src/agent_env/artifact/artifacts/file.py index 8756d296..900339d0 100644 --- a/src/agent_env/artifact/artifacts/file.py +++ b/src/agent_env/artifact/artifacts/file.py @@ -43,8 +43,9 @@ class FileArtifact(Artifact): content_type: str = Field(description="MIME type of the file") object_url: str = Field(alias="s3_url", description="Object-store locator where the file is stored") + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. data = handler(self) _write_twin(data, "s3_url", "object_url", self.object_url) diff --git a/src/agent_env/artifact/artifacts/file_artifact_universe.py b/src/agent_env/artifact/artifacts/file_artifact_universe.py index 7fec4c29..3b34b5cd 100644 --- a/src/agent_env/artifact/artifacts/file_artifact_universe.py +++ b/src/agent_env/artifact/artifacts/file_artifact_universe.py @@ -67,8 +67,9 @@ class FileArtifactUniverse(Universe): ), ) + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. data = handler(self) _write_twin(data, "bundle_s3_url", "bundle_object_url", self.bundle_object_url) diff --git a/src/agent_env/artifact/artifacts/skill.py b/src/agent_env/artifact/artifacts/skill.py index 45c679b4..16fa6ef2 100644 --- a/src/agent_env/artifact/artifacts/skill.py +++ b/src/agent_env/artifact/artifacts/skill.py @@ -51,8 +51,9 @@ class SkillArtifact(Artifact): allowed_tools: Optional[str] = Field(default=None) skill_metadata: Optional[dict[str, str]] = Field(default=None) + # No return annotation: pydantic builds the serialization schema from one, and a dict drops the fields. @model_serializer(mode="wrap") - def _serialize(self, handler: Any) -> dict[str, Any]: + def _serialize(self, handler: Any): # Dual-write, from the attribute: `alias=` emits one spelling, which one depending on the caller's `by_alias`. data = handler(self) _write_twin(data, "skill_s3_url", "skill_object_url", self.skill_object_url) diff --git a/src/agent_env/explorer/routers/common.py b/src/agent_env/explorer/routers/common.py index f654aabb..e5d3f327 100644 --- a/src/agent_env/explorer/routers/common.py +++ b/src/agent_env/explorer/routers/common.py @@ -73,7 +73,8 @@ def _enrich_universe(doc: dict, store, collection: str) -> dict: "artifact_id": rid, "version": ver, "content_type": fa.get("content_type") if fa else None, - "object_url": (fa.get("object_url") or fa.get("s3_url")) if fa else None, + # Legacy key first, as the model reads it: a raw-doc writer that knows only s3_url leaves the twin stale. + "object_url": (fa.get("s3_url") or fa.get("object_url")) if fa else None, }) doc["files"] = files elif t in _UNIVERSE_ENVIRONMENT_TYPES: diff --git a/src/agent_env/store/object_store/s3_object_store.py b/src/agent_env/store/object_store/s3_object_store.py index 40249860..c6aa9453 100644 --- a/src/agent_env/store/object_store/s3_object_store.py +++ b/src/agent_env/store/object_store/s3_object_store.py @@ -264,6 +264,8 @@ def _put_file(self, bucket: str, key: str, file_path: str, content_type: str) -> return self._url(bucket, key) def _head(self, bucket: str, key: str) -> ObjectMetadata | None: + if not key: # a bucket, which S3 would refuse as a malformed request rather than report missing + return None try: head = self._s3.head_object(Bucket=bucket, Key=key) except ClientError as e: diff --git a/tst/unit/artifact/test_object_url_keys.py b/tst/unit/artifact/test_object_url_keys.py index 0633d773..6730c63f 100644 --- a/tst/unit/artifact/test_object_url_keys.py +++ b/tst/unit/artifact/test_object_url_keys.py @@ -10,6 +10,8 @@ from agent_env.artifact.artifacts.cli import CliArtifact from agent_env.artifact.artifacts.docker_image import DockerImageArtifact +from agent_env.artifact.artifacts.environment import EnvironmentArtifact +from agent_env.artifact.artifacts.environment_universe import EnvironmentUniverseArtifact from agent_env.artifact.artifacts.file import FileArtifact from agent_env.artifact.artifacts.file_artifact_universe import FileArtifactUniverse from agent_env.artifact.artifacts.skill import SkillArtifact @@ -41,6 +43,20 @@ def test_a_doc_keyed_either_way_loads_and_dumps_both_keys(case, spelling): assert all(dumped[legacy] == dumped[neutral] == urls[neutral] for legacy, neutral in pairs) +def test_the_legacy_key_wins_when_the_two_disagree(): + """Only a raw-doc writer outside the model can make them differ, and today's know only the legacy key.""" + doc = {"id": "x", "version": 1, "description": "d", "filename": "a", "content_type": "t"} + assert FileArtifact.model_validate({**doc, "s3_url": "mem://b/new", "object_url": "mem://b/stale"}).object_url == "mem://b/new" + + +@pytest.mark.parametrize("cls", [*(case[0] for case in _CASES.values()), EnvironmentArtifact, EnvironmentUniverseArtifact], + ids=lambda cls: cls.__name__) +def test_the_serialization_schema_keeps_the_fields(cls): + """A wrap serializer's return annotation would replace the schema; an annotated dict leaves no fields.""" + serialization = cls.model_json_schema(mode="serialization") + assert set(serialization.get("properties", {})) == set(cls.model_json_schema(mode="validation")["properties"]) + + def test_an_excluded_field_stays_excluded(): artifact = FileArtifact(id="x", description="d", filename="a", content_type="t", object_url="mem://b/a") assert {"s3_url", "object_url"}.isdisjoint(artifact.model_dump(exclude={"object_url"})) diff --git a/tst/unit/explorer/explorer_app_test.py b/tst/unit/explorer/explorer_app_test.py index 0035c9cf..6f491252 100644 --- a/tst/unit/explorer/explorer_app_test.py +++ b/tst/unit/explorer/explorer_app_test.py @@ -150,11 +150,15 @@ def test_type_filter_matches_documents_stored_under_an_aliased_spelling(tmp_path reset_config() -@pytest.mark.parametrize("url_key", ["s3_url", "object_url"]) -def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_path, monkeypatch, url_key): +@pytest.mark.parametrize("stored", [ + {"s3_url": "s3://b/a.txt"}, + {"object_url": "s3://b/a.txt"}, + {"s3_url": "s3://b/a.txt", "object_url": "s3://b/stale.txt"}, +], ids=["legacy-key", "neutral-key", "legacy-wins"]) +def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_path, monkeypatch, stored): """Enrichment picks its branch from the type, so a renamed universe must still resolve its refs — otherwise the detail response silently loses `files`. Its files - keep their url whichever key their documents store it under.""" + keep their url whichever key their documents store it under, the legacy one first.""" cfg = tmp_path / ".agentenv" / "config.toml" cfg.parent.mkdir(parents=True, exist_ok=True) cfg.write_text( @@ -168,7 +172,7 @@ def test_detail_route_enriches_a_universe_stored_under_an_aliased_spelling(tmp_p set_document_store(store) set_runner(LocalRunner(workers=1)) run_store.ensure_indexes() - store.insert("artifacts", {"id": "f1", "version": 1, "type": "file", url_key: "s3://b/a.txt", + store.insert("artifacts", {"id": "f1", "version": 1, "type": "file", **stored, "content_type": "text/plain", "created_at_utc": "2026-01-01T00:00:00Z"}) store.insert("artifacts", {"id": "u1", "version": 1, "type": "old_universe", "file_artifact_refs": {"a.txt": {"id": "f1", "version": 1}}, diff --git a/tst/unit/store/s3_object_store_test.py b/tst/unit/store/s3_object_store_test.py index 8a0c0aeb..ac58b18b 100644 --- a/tst/unit/store/s3_object_store_test.py +++ b/tst/unit/store/s3_object_store_test.py @@ -6,7 +6,8 @@ from botocore.stub import Stubber from moto import mock_aws -from agent_env.store import S3ObjectStore +from agent_env.artifact.artifacts.file import FileArtifact +from agent_env.store import S3ObjectStore, set_object_store from tst.store import object_conformance BUCKET = "conformance" @@ -58,3 +59,15 @@ def test_a_read_error_other_than_a_missing_object_is_not_mapped(operation, read, def test_the_client_keeps_a_connection_for_every_default_executor_thread(store): assert store._s3.meta.config.max_pool_connections == 32 + + +@pytest.mark.parametrize("url", [f"s3://{BUCKET}", f"s3://{BUCKET}/"]) +def test_a_bucket_url_names_no_object(url): + """Without a HEAD: S3 refuses an empty key as a malformed request, not as a missing object.""" + client = boto3.client("s3", region_name="us-east-1") + with Stubber(client): # nothing queued, so any request fails the test + store = S3ObjectStore(client, BUCKET) + assert store.get_object_metadata_at(url) is None + set_object_store(store) + with pytest.raises(ValueError, match="does not exist"): + FileArtifact.put_existing(id="bucket-root", description="d", object_url=url.rstrip("/")) From f6ab12640bb42f100c1757c6e051621f62ee2b24 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 7 Oct 2026 10:37:11 -0700 Subject: [PATCH 3/3] fix(artifact): an S3-named keyword passed as None still counts as passed renamed_keyword took an old keyword passed as None for one left out, so a caller passing an optional old keyword through (build_context_s3_url=None) was neither warned nor counted, and would break unseen when the keyword is removed; with the new spelling also set it skipped the both-spellings TypeError. The old keywords now default to deprecation.OMITTED instead of None. Two tests still passing put_bundled / put_existing the old s3_url= keyword move to prefix_url=. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/agent_env/artifact/artifacts/docker_image.py | 6 +++--- .../artifact/artifacts/file_artifact_universe.py | 8 ++++---- src/agent_env/artifact/artifacts/skill.py | 4 ++-- src/agent_env/utils/deprecation.py | 10 +++++++--- .../file_artifact_universe_artifact_test.py | 4 ++-- tst/unit/artifact/test_object_url_keys.py | 15 +++++++++++++-- tst/unit/store/local_backends_composition_test.py | 2 +- 7 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/agent_env/artifact/artifacts/docker_image.py b/src/agent_env/artifact/artifacts/docker_image.py index 0991512e..8a77cec9 100644 --- a/src/agent_env/artifact/artifacts/docker_image.py +++ b/src/agent_env/artifact/artifacts/docker_image.py @@ -24,7 +24,7 @@ from agent_env.artifact.artifact import Artifact, _write_twin from agent_env.store.ids import fs_safe, image_repository, is_local_id from agent_env.store.image_store.oci_registry_credentials import is_loopback_host, registry_host_from_ref -from agent_env.utils.deprecation import renamed_keyword +from agent_env.utils.deprecation import OMITTED, renamed_keyword logger = logging.getLogger(__name__) @@ -175,8 +175,8 @@ def put_tar( image_name: str, tar_gz_object_url: str | None = None, build_context_object_url: str | None = None, - tar_gz_s3_url: str | None = None, - build_context_s3_url: str | None = None, + tar_gz_s3_url: str | None = OMITTED, + build_context_s3_url: str | None = OMITTED, ) -> "DockerImageArtifact": from agent_env.artifact.store import get_artifact_store from agent_env.config import get_config diff --git a/src/agent_env/artifact/artifacts/file_artifact_universe.py b/src/agent_env/artifact/artifacts/file_artifact_universe.py index 3b34b5cd..ac3c3834 100644 --- a/src/agent_env/artifact/artifacts/file_artifact_universe.py +++ b/src/agent_env/artifact/artifacts/file_artifact_universe.py @@ -14,7 +14,7 @@ from agent_env.artifact.universe import Universe from agent_env.config import get_config from agent_env.store.ids import derive_id -from agent_env.utils.deprecation import renamed_keyword +from agent_env.utils.deprecation import OMITTED, renamed_keyword logger = logging.getLogger(__name__) @@ -82,7 +82,7 @@ def put( *, file_artifacts: dict[str, "FileArtifact"], bundle_object_url: str | None = None, - bundle_s3_url: str | None = None, + bundle_s3_url: str | None = OMITTED, ) -> "FileArtifactUniverse": from agent_env.artifact.store import get_artifact_store @@ -144,7 +144,7 @@ def put_bundled( *, files: dict[str, Path], prefix_url: str | None = None, - s3_url: str | None = None, + s3_url: str | None = OMITTED, ) -> "FileArtifactUniverse": """Upload ``files`` (bundle key -> local path) under ``prefix_url`` and register them as one universe. Without ``prefix_url``, each call writes under a prefix of its own (``ArtifactStore.attempt_prefix``).""" @@ -189,7 +189,7 @@ def put_existing( id: str, *, prefix_url: str | None = None, - s3_url: str | None = None, + s3_url: str | None = OMITTED, ) -> "FileArtifactUniverse": """Wrap files that already exist under an object-store prefix as a universe. diff --git a/src/agent_env/artifact/artifacts/skill.py b/src/agent_env/artifact/artifacts/skill.py index 16fa6ef2..830a169a 100644 --- a/src/agent_env/artifact/artifacts/skill.py +++ b/src/agent_env/artifact/artifacts/skill.py @@ -21,7 +21,7 @@ from agent_env.config import get_config from agent_env.store.base import ObjectNotFoundError from agent_env.store.ids import derive_id, key_segment -from agent_env.utils.deprecation import renamed_keyword +from agent_env.utils.deprecation import OMITTED, renamed_keyword if TYPE_CHECKING: from agent_env.artifact.artifacts.file_artifact_universe import FileArtifactUniverse @@ -65,7 +65,7 @@ def validate( *, skill_md: Optional[bytes] = None, object_url: Optional[str] = None, - s3_url: Optional[str] = None, + s3_url: Optional[str] = OMITTED, expected_name: str, ) -> None: object_url = renamed_keyword("SkillArtifact.validate", "object_url", object_url, "s3_url", s3_url) diff --git a/src/agent_env/utils/deprecation.py b/src/agent_env/utils/deprecation.py index 76bc62f4..c361fc67 100644 --- a/src/agent_env/utils/deprecation.py +++ b/src/agent_env/utils/deprecation.py @@ -48,10 +48,14 @@ def warn_deprecated(old: str, new: str, *, kind: str = "symbol", stacklevel: int ) +# The default of a deprecated keyword, so passing it as None still counts as passing it. +OMITTED: Any = object() + + def renamed_keyword(owner: str, new: str, new_value: Any, old: str, old_value: Any) -> Any: - """The value of keyword ``new``, taking the deprecated keyword ``old`` in its place with a warning. Passing both - is a TypeError, as a repeated keyword is.""" - if old_value is None: + """The value of keyword ``new``, taking the deprecated keyword ``old`` (default ``OMITTED``) in its place with a + warning. Passing both is a TypeError, as a repeated keyword is.""" + if old_value is OMITTED: return new_value if new_value is not None: raise TypeError(f"{owner}() got both {new}= and its deprecated spelling {old}=") diff --git a/tst/integration/artifact/artifacts/file_artifact_universe_artifact_test.py b/tst/integration/artifact/artifacts/file_artifact_universe_artifact_test.py index b18315ad..afa32f11 100644 --- a/tst/integration/artifact/artifacts/file_artifact_universe_artifact_test.py +++ b/tst/integration/artifact/artifacts/file_artifact_universe_artifact_test.py @@ -226,7 +226,7 @@ def test_put_bundled_pins_versions(self): universe = FileArtifactUniverse.put_bundled( id=_unique_id("fau_bundled"), files={"nested/a.json": EMAIL_ARTIFACT_PATH}, - s3_url=get_config().get_object_store().object_url(f"test/{uuid.uuid4().hex}/"), + prefix_url=get_config().get_object_store().object_url(f"test/{uuid.uuid4().hex}/"), ) assert universe.file_artifact_refs is not None ref = universe.file_artifact_refs["nested/a.json"] @@ -242,7 +242,7 @@ def test_put_existing_pins_versions(self): universe = FileArtifactUniverse.put_existing( id=_unique_id("fau_existing"), - s3_url=store.object_url(prefix), + prefix_url=store.object_url(prefix), ) assert universe.file_artifact_refs is not None diff --git a/tst/unit/artifact/test_object_url_keys.py b/tst/unit/artifact/test_object_url_keys.py index 6730c63f..47060cfd 100644 --- a/tst/unit/artifact/test_object_url_keys.py +++ b/tst/unit/artifact/test_object_url_keys.py @@ -104,10 +104,21 @@ def test_skill_validate_takes_its_s3_named_keyword_with_a_warning(local_stores): SkillArtifact.validate(s3_url=store.object_url("skills/demo"), expected_name="demo") -def test_both_spellings_of_a_keyword_is_an_error(local_stores): +@pytest.mark.parametrize("old", ["url", None], ids=["old-set", "old-none"]) +def test_both_spellings_of_a_keyword_is_an_error(local_stores, old): url = local_stores.get_object_store().object_url("images/i.tar.gz") with pytest.raises(TypeError, match="got both tar_gz_object_url= and its deprecated spelling tar_gz_s3_url="): - DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_object_url=url, tar_gz_s3_url=url) + DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_object_url=url, + tar_gz_s3_url=url if old else None) + + +def test_an_old_keyword_passed_as_none_is_still_counted(local_stores): + """A caller passing an optional old keyword through, as None, still breaks when the keyword is removed.""" + url = local_stores.get_object_store().object_url("images/i.tar.gz") + with pytest.warns(DeprecationWarning, match=r"put_tar\(build_context_s3_url=\)"): + artifact = DockerImageArtifact.put_tar("img", description="d", image_name="img:1", tar_gz_object_url=url, + build_context_s3_url=None) + assert artifact.build_context_object_url is None def test_a_required_keyword_is_still_required(): diff --git a/tst/unit/store/local_backends_composition_test.py b/tst/unit/store/local_backends_composition_test.py index dba68db6..0ee370ce 100644 --- a/tst/unit/store/local_backends_composition_test.py +++ b/tst/unit/store/local_backends_composition_test.py @@ -269,7 +269,7 @@ def test_a_docker_image_names_its_repository_and_tarball_from_the_encoded_id( ), lambda tmp_path: FileArtifactUniverse.put_bundled( id=HOSTILE, files={"p.txt": Path(_write(tmp_path, "p.txt", b"x"))}, - s3_url=get_config().get_object_store().object_url("bundle/"), + prefix_url=get_config().get_object_store().object_url("bundle/"), ), ], ids=["docker_image", "file", "file_put_at", "universe_put_bundled"]) def test_an_local_id_outside_the_cli_is_refused_before_any_image_or_object_is_written(local_stores, monkeypatch, tmp_path, put):