diff --git a/src/agent_env/a2a_agent/a2a_agent.py b/src/agent_env/a2a_agent/a2a_agent.py index a0dc1707..9621f340 100644 --- a/src/agent_env/a2a_agent/a2a_agent.py +++ b/src/agent_env/a2a_agent/a2a_agent.py @@ -177,9 +177,9 @@ async def add_skill(deployed: "DeployedA2AAgent", skill: "Skill") -> dict: description=artifact.description, object_url=artifact.skill_object_url, ) - if skill.s3_url is not None: + if skill.object_url is not None: return await A2AAgent.register_skill( - deployed, name=skill.name, description=skill.description, object_url=skill.s3_url + deployed, name=skill.name, description=skill.description, object_url=skill.object_url ) return await A2AAgent.register_skill( deployed, name=skill.name, description=skill.description, skill_md=skill.to_skill_md() diff --git a/src/agent_env/a2a_agent/object_transfer.py b/src/agent_env/a2a_agent/object_transfer.py index 4dcc2b37..a7419cca 100644 --- a/src/agent_env/a2a_agent/object_transfer.py +++ b/src/agent_env/a2a_agent/object_transfer.py @@ -669,9 +669,4 @@ async def fetch_trajectory( return FetchedTrajectory(object_url=upload.object_url) if not isinstance(body, Mapping): return FetchedTrajectory() - if body.get("trajectory") is None and body.get("trajectory_s3_prefix"): - raise RuntimeError( - "trajectory get answered with trajectory_s3_prefix, which agent-env does not read; " - "the agent must return the trajectory inline or take the object form" - ) return FetchedTrajectory(inline=body.get("trajectory")) diff --git a/src/agent_env/a2a_agent/validator.py b/src/agent_env/a2a_agent/validator.py index 82b08b05..457184ed 100644 --- a/src/agent_env/a2a_agent/validator.py +++ b/src/agent_env/a2a_agent/validator.py @@ -171,14 +171,14 @@ async def validate( skill_object_url = await asyncio.to_thread( A2AAgentValidator._upload_skill_fixture, agent, - name="validator-test-s3", + name="validator-test-object", description=( "Validator test skill with project codename. " "Use when asked about the project codename." ), body=( "When asked for the project codename, respond with: " - "VALIDATOR-S3-99" + "VALIDATOR-OBJ-99" ), ) skill_bundle_probe_url = await asyncio.to_thread( @@ -255,7 +255,7 @@ async def validate( VerifyA2ATrajectoryStep(id=f"{task_id}-trajectory", version=None, a2a_agent_id=agent.id, depends_on=[TaskStepDependency(task_step_id=mcp_step_id)], fail_task_on_error=False), AddSkillsTaskStep(id=skill_chain_id, version=None, depends_on=deploy_agent_dep, fail_task_on_error=False, skills=[ Skill(name="validator-test", description="Validator test skill with secret code. Use when asked about the company secret code.", body="When asked for the company secret code, respond with: VALIDATOR-42"), - Skill(name="validator-test-s3", description="Validator test skill with project codename. Use when asked about the project codename.", s3_url=skill_object_url), + Skill(name="validator-test-object", description="Validator test skill with project codename. Use when asked about the project codename.", object_url=skill_object_url), ]), PromptAgentTaskStep(id=skill_prompt_id, version=None, prompt="What is the company secret code? What is the project codename?", prompt_id=skill_prompt_id, timeout_seconds=120, depends_on=[TaskStepDependency(task_step_id=skill_chain_id)], fail_task_on_error=False), @@ -263,7 +263,7 @@ async def validate( depends_on=[TaskStepDependency(task_step_id=skill_prompt_id)], fail_task_on_error=False, criteria=[ {"id": "secret_code_inline", "description": "Response contains VALIDATOR-42"}, - {"id": "secret_code_s3", "description": "Response contains VALIDATOR-S3-99"}, + {"id": "secret_code_object", "description": "Response contains VALIDATOR-OBJ-99"}, ]), VerifyA2ASkillConfigStep( id=f"{task_id}-skill-config", diff --git a/src/agent_env/artifact/artifacts/skill.py b/src/agent_env/artifact/artifacts/skill.py index 830a169a..14db8f9d 100644 --- a/src/agent_env/artifact/artifacts/skill.py +++ b/src/agent_env/artifact/artifacts/skill.py @@ -72,8 +72,8 @@ def validate( 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) + skill_md = fetch_skill_md(object_url) + frontmatter, _body = parse_skill_md(skill_md) _validate_frontmatter(frontmatter, expected_name=expected_name) @classmethod @@ -87,7 +87,7 @@ def put(cls, id: str, *, skill_dir: Path) -> "SkillArtifact": skill_md_bytes = skill_md_path.read_bytes() cls.validate(skill_md=skill_md_bytes, expected_name=id) - frontmatter, _body = _parse_skill_md(skill_md_bytes) + frontmatter, _body = parse_skill_md(skill_md_bytes) files: dict[str, Path] = {} for p in sorted(skill_dir.rglob("*")): @@ -125,7 +125,7 @@ def get_skill_files(self) -> "FileArtifactUniverse": return FileArtifactUniverse.get(self.skill_files_id) -def _parse_skill_md(data: bytes) -> tuple[dict, str]: +def parse_skill_md(data: bytes) -> tuple[dict, str]: text = data.decode("utf-8") if not text.startswith("---"): raise ValueError("SKILL.md must begin with a YAML frontmatter block delimited by '---'") @@ -191,7 +191,7 @@ def _validate_frontmatter(fm: dict, expected_name: str) -> None: raise ValueError("SKILL.md frontmatter 'metadata' must map strings to strings") -def _fetch_skill_md(object_url: str) -> bytes: +def fetch_skill_md(object_url: str) -> bytes: prefix = object_url if object_url.endswith("/") else object_url + "/" skill_md_url = prefix + _SKILL_MD_FILENAME try: @@ -217,3 +217,8 @@ def download_skill(object_url: str) -> Iterator[Path]: if count == 0: raise ValueError(f"No objects found under {object_url}") yield tmp_path + + +# Their former private names, which callers outside agent-env import and patch. +_parse_skill_md = parse_skill_md +_fetch_skill_md = fetch_skill_md diff --git a/src/agent_env/cli/a2a_agent/add_skill.py b/src/agent_env/cli/a2a_agent/add_skill.py index 0fc4e20c..1ec60762 100644 --- a/src/agent_env/cli/a2a_agent/add_skill.py +++ b/src/agent_env/cli/a2a_agent/add_skill.py @@ -5,15 +5,17 @@ import click from agent_env.a2a_agent import A2AAgent +from agent_env.artifact.artifacts.skill import fetch_skill_md, parse_skill_md from agent_env.a2a_agent.store import get_a2a_agent_instance_store from agent_env.store.base import NotFoundError +from agent_env.cli.utils import deprecated_option, renamed_value from agent_env.task_step.task_steps.add_skills import Skill # --------------------------------------------------------------------------- # agent-env a2a-agent add-skill --instance-id --skill-artifact-id code-review # agent-env a2a-agent add-skill --instance-id --skill-md-path ./SKILL.md -# agent-env a2a-agent add-skill --instance-id --skill-s3-url s3://bucket/prefix/ +# agent-env a2a-agent add-skill --instance-id --skill-object-url s3://bucket/prefix/ # --------------------------------------------------------------------------- @@ -40,23 +42,26 @@ help="Local SKILL.md file to register inline", ) @click.option( - "--skill-s3-url", - "skill_s3_url", + "--skill-object-url", + "skill_object_url", default=None, - help="S3 prefix containing a skill directory", + help="Object-store prefix containing a skill directory", ) +@deprecated_option("--skill-s3-url", "skill_s3_url", "--skill-object-url") def add_skill( instance_id: str, skill_artifact_id: Optional[str], skill_artifact_version: Optional[int], skill_md_path: Optional[Path], + skill_object_url: Optional[str], skill_s3_url: Optional[str], ): """Register a skill against a deployed A2A agent's /ext/skill-config endpoint.""" - sources = [bool(skill_artifact_id), bool(skill_md_path), bool(skill_s3_url)] + skill_object_url = renamed_value("--skill-object-url", skill_object_url, "--skill-s3-url", skill_s3_url) + sources = [bool(skill_artifact_id), bool(skill_md_path), bool(skill_object_url)] if sum(sources) != 1: click.echo( - "Error: specify exactly one of --skill-artifact-id, --skill-md-path, --skill-s3-url", + "Error: specify exactly one of --skill-artifact-id, --skill-md-path, --skill-object-url", err=True, ) raise SystemExit(1) @@ -72,16 +77,12 @@ def add_skill( if skill_artifact_id is not None: skill = Skill(skill_artifact_id=skill_artifact_id, skill_artifact_version=skill_artifact_version) click.echo(f"Registering SkillArtifact '{skill_artifact_id}' v{skill_artifact_version or 'latest'}...") - elif skill_s3_url is not None: - from agent_env.artifact.artifacts.skill import _fetch_skill_md, _parse_skill_md - - frontmatter, _ = _parse_skill_md(_fetch_skill_md(skill_s3_url)) - skill = Skill(name=frontmatter["name"], description=frontmatter["description"], s3_url=skill_s3_url) - click.echo(f"Registering skill from {skill_s3_url} (name={skill.name})...") + elif skill_object_url is not None: + frontmatter, _ = parse_skill_md(fetch_skill_md(skill_object_url)) + skill = Skill(name=frontmatter["name"], description=frontmatter["description"], object_url=skill_object_url) + click.echo(f"Registering skill from {skill_object_url} (name={skill.name})...") else: - from agent_env.artifact.artifacts.skill import _parse_skill_md - - frontmatter, body = _parse_skill_md(skill_md_path.read_bytes()) + frontmatter, body = parse_skill_md(skill_md_path.read_bytes()) skill = Skill( name=frontmatter["name"], description=frontmatter["description"], diff --git a/src/agent_env/cli/artifact/cli.py b/src/agent_env/cli/artifact/cli.py index dfc85fcd..8d703c4a 100644 --- a/src/agent_env/cli/artifact/cli.py +++ b/src/agent_env/cli/artifact/cli.py @@ -47,7 +47,7 @@ def cli_put( click.echo( f"Created CliArtifact: id={result.id} version={result.version} " f"command_name={result.command_name} entrypoint={result.entrypoint} " - f"cli_files_id={result.cli_files_id} cli_s3_url={result.cli_object_url}" + f"cli_files_id={result.cli_files_id} cli_object_url={result.cli_object_url}" ) diff --git a/src/agent_env/cli/artifact/file_artifact_universe.py b/src/agent_env/cli/artifact/file_artifact_universe.py index 6d070aa3..9a6452c9 100644 --- a/src/agent_env/cli/artifact/file_artifact_universe.py +++ b/src/agent_env/cli/artifact/file_artifact_universe.py @@ -6,6 +6,7 @@ import click from agent_env.artifact import FileArtifact, FileArtifactUniverse +from agent_env.cli.utils import deprecated_option, renamed_value from agent_env.store.ids import fs_safe, is_local_id, key_segment @@ -67,14 +68,16 @@ def put(universe_id: str, file_artifact_ids: Tuple[str, ...]): help="Local directory; every file under it is uploaded into the bundle (relative path preserved)", ) @click.option( - "--s3-url", - "s3_url", + "--prefix-url", + "prefix_url", default=None, help="Object-store prefix to upload the bundle under. Defaults to the version's own prefix in the configured " "object store (artifacts/file_artifact_universe///).", ) -def put_bundled(universe_id: str, file_dir: Path, s3_url: Optional[str]): +@deprecated_option("--s3-url", "s3_url", "--prefix-url") +def put_bundled(universe_id: str, file_dir: Path, prefix_url: Optional[str], s3_url: Optional[str]): """Upload every file under --file-dir as a single bundled FileArtifactUniverse.""" + prefix_url = renamed_value("--prefix-url", prefix_url, "--s3-url", s3_url) files: dict[str, Path] = {} for p in sorted(file_dir.rglob("*")): if p.is_file(): @@ -83,13 +86,13 @@ def put_bundled(universe_id: str, file_dir: Path, s3_url: Optional[str]): click.echo(f"Error: no files found under {file_dir}", err=True) raise SystemExit(1) - click.echo(f"Uploading {len(files)} file(s)" + (f" under {s3_url}..." if s3_url else "...")) + click.echo(f"Uploading {len(files)} file(s)" + (f" under {prefix_url}..." if prefix_url else "...")) for rel in files: click.echo(f" {rel}") - universe = FileArtifactUniverse.put_bundled(id=universe_id, files=files, prefix_url=s3_url) + universe = FileArtifactUniverse.put_bundled(id=universe_id, files=files, prefix_url=prefix_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}" + f"file_count={len(universe.file_artifact_ids)} bundle_object_url={universe.bundle_object_url}" ) diff --git a/src/agent_env/cli/artifact/skill.py b/src/agent_env/cli/artifact/skill.py index 40764c41..c12d72ce 100644 --- a/src/agent_env/cli/artifact/skill.py +++ b/src/agent_env/cli/artifact/skill.py @@ -5,7 +5,8 @@ import click from agent_env.artifact import SkillArtifact -from agent_env.artifact.artifacts.skill import _parse_skill_md, download_skill +from agent_env.artifact.artifacts.skill import parse_skill_md, download_skill +from agent_env.cli.utils import deprecated_option, renamed_value # --------------------------------------------------------------------------- @@ -42,21 +43,23 @@ def skill(): help="Local skill directory containing SKILL.md at the root", ) @click.option( - "--s3-url", - "s3_url", + "--prefix-url", + "prefix_url", default=None, - help="S3 prefix containing the skill (s3://bucket/prefix/) — alternative to --skill-dir", + help="Object-store prefix containing the skill — alternative to --skill-dir", ) -def skill_put(artifact_id: str, skill_dir: Optional[Path], s3_url: Optional[str]): - """Upload a skill as a SkillArtifact, from a local directory or an S3 prefix.""" - if (skill_dir is None) == (s3_url is None): - click.echo("Error: specify exactly one of --skill-dir or --s3-url", err=True) +@deprecated_option("--s3-url", "s3_url", "--prefix-url") +def skill_put(artifact_id: str, skill_dir: Optional[Path], prefix_url: Optional[str], s3_url: Optional[str]): + """Upload a skill as a SkillArtifact, from a local directory or an object-store prefix.""" + prefix_url = renamed_value("--prefix-url", prefix_url, "--s3-url", s3_url) + if (skill_dir is None) == (prefix_url is None): + click.echo("Error: specify exactly one of --skill-dir or --prefix-url", err=True) raise SystemExit(1) - source_cm = download_skill(s3_url) if s3_url else contextlib.nullcontext(skill_dir) + source_cm = download_skill(prefix_url) if prefix_url else contextlib.nullcontext(skill_dir) with source_cm as source_dir: - if s3_url: - click.echo(f"Downloaded skill from {s3_url} to {source_dir}") + if prefix_url: + click.echo(f"Downloaded skill from {prefix_url} to {source_dir}") file_count = sum(1 for p in source_dir.rglob("*") if p.is_file()) click.echo(f"Uploading SkillArtifact bundle ({file_count} file(s))...") @@ -65,7 +68,7 @@ def skill_put(artifact_id: str, skill_dir: Optional[Path], s3_url: Optional[str] f"Created SkillArtifact: id={result.id} version={result.version} " f"spec_version={result.agent_skills_spec_version} " f"skill_files_id={result.skill_files_id} " - f"skill_s3_url={result.skill_object_url}" + f"skill_object_url={result.skill_object_url}" ) click.echo() click.secho(_BANNER, fg="magenta", bold=True) @@ -83,7 +86,7 @@ def skill_put(artifact_id: str, skill_dir: Optional[Path], s3_url: Optional[str] for k, v in result.skill_metadata.items(): click.echo(f" {click.style(k, fg='cyan')}: {click.style(v, fg='green')}") - _, body = _parse_skill_md((source_dir / _SKILL_MD_FILENAME).read_bytes()) + _, body = parse_skill_md((source_dir / _SKILL_MD_FILENAME).read_bytes()) preview = body[:_BODY_PREVIEW_CHARS] truncated = len(body) > _BODY_PREVIEW_CHARS click.secho(f"Body (first {_BODY_PREVIEW_CHARS} chars):", fg="cyan", bold=True) diff --git a/src/agent_env/cli/env/mcp_server.py b/src/agent_env/cli/env/mcp_server.py index 67572c1f..516e1a85 100644 --- a/src/agent_env/cli/env/mcp_server.py +++ b/src/agent_env/cli/env/mcp_server.py @@ -409,5 +409,5 @@ def create_cli(env_id: str, env_version: int | None, command_name: str | None, f click.echo( f"Created CliArtifact: id={artifact.id} version={artifact.version} " f"command_name={artifact.command_name} entrypoint={artifact.entrypoint} " - f"cli_s3_url={artifact.cli_object_url}" + f"cli_object_url={artifact.cli_object_url}" ) diff --git a/src/agent_env/cli/utils.py b/src/agent_env/cli/utils.py index 7df4225e..496b7736 100644 --- a/src/agent_env/cli/utils.py +++ b/src/agent_env/cli/utils.py @@ -32,6 +32,24 @@ def build_platform_option(f): )(f) +def deprecated_option(old: str, dest: str, replacement: str): + """A hidden `old` flag, the deprecated spelling of `replacement`: using it notes the replacement on stderr. + Merge the two with :func:`renamed_value`.""" + def note(ctx: click.Context, param: click.Parameter, value: str | None) -> str | None: + if value is not None: + click.echo(f"Warning: {old} is deprecated and will be removed; use {replacement}.", err=True) + return value + + return click.option(old, dest, default=None, hidden=True, callback=note) + + +def renamed_value(new: str, new_value: str | None, old: str, old_value: str | None) -> str | None: + """The value of flag `new`, or of its deprecated spelling `old`; giving both is a usage error.""" + if new_value is not None and old_value is not None: + raise click.UsageError(f"{old} is the deprecated spelling of {new}; give only {new}") + return old_value if new_value is None else new_value + + def refuse_unwritable_ids(*ids: str) -> None: """Refuse, before anything is built, an id the store a put writes it to wouldn't take, such as an @local id the image's suffix makes too long.""" 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 b82dc4f9..3c632f4e 100644 --- a/src/agent_env/task_step/task_steps/add_skills.py +++ b/src/agent_env/task_step/task_steps/add_skills.py @@ -5,12 +5,13 @@ import hashlib import logging import re -from dataclasses import dataclass +from dataclasses import InitVar, dataclass from typing import ClassVar, Optional from agent_env.task_step.context import TaskStepContext from agent_env.entity_refs import EntityRef from agent_env.task_step.task_step import TaskStep, TaskStepDependency +from agent_env.utils.deprecation import OMITTED, renamed_keyword logger = logging.getLogger(__name__) @@ -23,10 +24,12 @@ class Skill: A skill can come from three mutually-exclusive sources: - **inline**: body + frontmatter fields, rendered into SKILL.md at send time - - **s3_url**: object-store prefix containing a skill directory with its own SKILL.md, + - **object_url**: object-store prefix containing a skill directory with its own SKILL.md, sent as a bundle of read grants - **skill_artifact_id**: a SkillArtifact in the store; name/description/skill_object_url are resolved from it at send time + + ``s3_url`` is the deprecated spelling of ``object_url``: it still constructs one, and reads as it. """ name: Optional[str] = None description: Optional[str] = None @@ -35,26 +38,31 @@ class Skill: compatibility: Optional[str] = None metadata: Optional[dict[str, str]] = None allowed_tools: Optional[str] = None - s3_url: Optional[str] = None + s3_url: InitVar[Optional[str]] = OMITTED skill_artifact_id: Optional[str] = None skill_artifact_version: Optional[int] = None - - def __post_init__(self): - modes = [bool(self.body), bool(self.s3_url), bool(self.skill_artifact_id)] + object_url: Optional[str] = None + + def __post_init__(self, s3_url: Optional[str]): + if s3_url is not OMITTED and s3_url == self.object_url: + s3_url = OMITTED # how dataclasses.replace copies a Skill: it reads s3_url back off the original + # Five frames up: through the dataclass's generated __init__ to its caller. + self.object_url = renamed_keyword("Skill", "object_url", self.object_url, "s3_url", s3_url, stacklevel=5) + modes = [bool(self.body), bool(self.object_url), bool(self.skill_artifact_id)] if sum(modes) != 1: - raise ValueError("Skill must have exactly one of: body (inline), s3_url, skill_artifact_id") + raise ValueError("Skill must have exactly one of: body (inline), object_url, skill_artifact_id") if self.skill_artifact_id is not None: - if any([self.body, self.s3_url, self.license, self.compatibility, self.metadata, self.allowed_tools]): + if any([self.body, self.object_url, self.license, self.compatibility, self.metadata, self.allowed_tools]): raise ValueError( - "Skill with skill_artifact_id must not have body/s3_url/license/compatibility/metadata/allowed_tools" + "Skill with skill_artifact_id must not have body/object_url/license/compatibility/metadata/allowed_tools" ) else: if not self.name or not self.description: - raise ValueError("Inline or s3_url skills require name and description") - if self.s3_url and any([self.body, self.license, self.compatibility, self.metadata, self.allowed_tools]): + raise ValueError("Inline or object_url skills require name and description") + if self.object_url and any([self.body, self.license, self.compatibility, self.metadata, self.allowed_tools]): raise ValueError( - "Skill with s3_url must not have body/license/compatibility/metadata/allowed_tools — " - "the S3 skill directory has its own SKILL.md" + "Skill with object_url must not have body/license/compatibility/metadata/allowed_tools — " + "the skill directory has its own SKILL.md" ) def validate(self) -> None: @@ -62,8 +70,8 @@ def validate(self) -> None: if self.skill_artifact_id is not None: return - if self.s3_url is not None: - SkillArtifact.validate(object_url=self.s3_url, expected_name=self.name) + if self.object_url is not None: + SkillArtifact.validate(object_url=self.object_url, expected_name=self.name) return SkillArtifact.validate(skill_md=self.to_skill_md().encode("utf-8"), expected_name=self.name) @@ -87,10 +95,11 @@ def to_skill_md(self) -> str: return "\n".join(lines) def to_dict(self) -> dict: + """Both spellings of ``object_url``: a worker on an older version reads only ``s3_url``.""" d: dict = {} for field_name in ( "name", "description", "body", "license", "compatibility", - "metadata", "allowed_tools", "s3_url", + "metadata", "allowed_tools", "s3_url", "object_url", "skill_artifact_id", "skill_artifact_version", ): value = getattr(self, field_name) @@ -104,7 +113,7 @@ def from_dict(cls, data: dict) -> Skill: name=data.get("name"), description=data.get("description"), body=data.get("body"), license=data.get("license"), compatibility=data.get("compatibility"), metadata=data.get("metadata"), - allowed_tools=data.get("allowed_tools"), s3_url=data.get("s3_url"), + allowed_tools=data.get("allowed_tools"), object_url=data.get("s3_url") or data.get("object_url"), skill_artifact_id=data.get("skill_artifact_id"), skill_artifact_version=data.get("skill_artifact_version"), ) @@ -164,6 +173,12 @@ def _build_skill_for_loaded_file_artifact_universe(universe_id: str, entry: dict ) + +# ``s3_url`` reads and writes ``object_url``. Set once the dataclass is built, so the ``s3_url=`` init argument keeps +# its default. +Skill.s3_url = property(lambda self: self.object_url, lambda self, value: setattr(self, "object_url", value)) + + class AddSkillsTaskStep(TaskStep): type: ClassVar[str] = "add_skills" entity_refs = ( diff --git a/src/agent_env/task_step/task_steps/deploy_agent.py b/src/agent_env/task_step/task_steps/deploy_agent.py index 47943f01..2cfbcff5 100644 --- a/src/agent_env/task_step/task_steps/deploy_agent.py +++ b/src/agent_env/task_step/task_steps/deploy_agent.py @@ -69,15 +69,15 @@ def _live_mcp_url(env_id: str) -> str: def _skill_fields(skill: dict) -> dict: - """A ``skills`` entry as ``A2AAgent.register_skill`` arguments. ``content`` and ``s3_uri`` are - older names for ``skill_md`` and ``skill_s3_url``; ``skill_md`` is sent verbatim.""" + """A ``skills`` entry as ``A2AAgent.register_skill`` arguments. ``content`` is an older name for + ``skill_md``, and ``skill_s3_url`` and ``s3_uri`` for ``object_url``; ``skill_md`` is sent verbatim.""" skill_md = skill.get("skill_md", skill.get("content")) return { "name": skill.get("name", ""), "description": skill.get("description", ""), "skill_md": skill_md, "object_url": ( - skill.get("skill_s3_url", skill.get("s3_uri")) if skill_md is None else None + skill.get("object_url", skill.get("skill_s3_url", skill.get("s3_uri"))) if skill_md is None else None ), } @@ -195,11 +195,6 @@ def to_dict(self) -> dict: @classmethod def from_dict(cls, data: dict) -> DeployAgentTaskStep: - if data.get("agent_changelog_s3_prefix"): - raise ValueError( - "agent_changelog_s3_prefix is no longer supported: an S3-form changelog cannot " - "be applied; capture it again and set agent_changelog_object_url" - ) return cls( **cls._base_from_dict(data), env_ids=data.get("env_ids"), @@ -587,7 +582,7 @@ async def _load_snapshot( ) if not universe.bundle_object_url: raise RuntimeError( - f"FileArtifactUniverse '{universe.id}' v{universe.version} has no bundle_s3_url; " + f"FileArtifactUniverse '{universe.id}' v{universe.version} has no bundle_object_url; " "snapshot universes must be created via put_existing or put_bundled" ) load_method, load_path = A2AAgent.operation(snapshot_ext, "load") diff --git a/src/agent_env/utils/deprecation.py b/src/agent_env/utils/deprecation.py index c361fc67..9fcd7b31 100644 --- a/src/agent_env/utils/deprecation.py +++ b/src/agent_env/utils/deprecation.py @@ -52,14 +52,15 @@ def warn_deprecated(old: str, new: str, *, kind: str = "symbol", stacklevel: int OMITTED: Any = object() -def renamed_keyword(owner: str, new: str, new_value: Any, old: str, old_value: Any) -> Any: +def renamed_keyword(owner: str, new: str, new_value: Any, old: str, old_value: Any, *, stacklevel: int = 4) -> Any: """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.""" + warning. Passing both is a TypeError, as a repeated keyword is. ``stacklevel`` 4 names the caller of a function + that calls this directly.""" 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}=") - warn_deprecated(f"{owner}({old}=)", f"{new}=", kind="keyword", stacklevel=4) + warn_deprecated(f"{owner}({old}=)", f"{new}=", kind="keyword", stacklevel=stacklevel) return old_value diff --git a/tst/unit/a2a_agent/add_skill_test.py b/tst/unit/a2a_agent/add_skill_test.py index d5636253..30c3ac0b 100644 --- a/tst/unit/a2a_agent/add_skill_test.py +++ b/tst/unit/a2a_agent/add_skill_test.py @@ -117,7 +117,7 @@ async def test_add_skill_negotiates_bundle_from_legacy_config_spelling( ) -> None: await A2AAgent.add_skill( _deployed(bundle=True, config_key="config"), - Skill(name="review", description="Review work", s3_url=prefix), + Skill(name="review", description="Review work", object_url=prefix), ) assert agent.requests[0]["url"] == "https://agent.example.test/custom/skills" @@ -177,7 +177,7 @@ async def test_an_object_skill_is_refused_by_an_agent_without_the_bundle_form( with pytest.raises(RuntimeError, match="skill add: the agent does not advertise the object form"): await A2AAgent.add_skill( _deployed(bundle=False), - Skill(name="review", description="Review work", s3_url=prefix), + Skill(name="review", description="Review work", object_url=prefix), ) assert agent.requests == [] @@ -194,7 +194,7 @@ async def test_an_object_skill_on_a_store_without_grants_is_refused_before_any_r with pytest.raises(RuntimeError, match="does not issue transfer grants"): await A2AAgent.add_skill( _deployed(bundle=True, legacy=legacy), - Skill(name="review", description="Review work", s3_url=prefix), + Skill(name="review", description="Review work", object_url=prefix), ) assert agent.requests == [] @@ -207,7 +207,7 @@ async def test_an_object_skill_is_refused_when_the_stores_grants_do_not_reach_th with pytest.raises(RuntimeError, match="grants do not reach agents on the 'modal' sandbox provider"): await A2AAgent.add_skill( _deployed(bundle=True, sandbox_type="modal"), - Skill(name="review", description="Review work", s3_url=prefix), + Skill(name="review", description="Review work", object_url=prefix), ) assert agent.requests == [] @@ -241,12 +241,14 @@ async def test_inline_skill_does_not_require_bundle_support( @pytest.mark.asyncio -async def test_legacy_deploy_skill_dict_negotiates_a_portable_bundle( - store: GrantingObjectStore, prefix: str, agent: _Agent +@pytest.mark.parametrize("key", ["object_url", "skill_s3_url", "s3_uri"]) +async def test_a_deploy_skill_dict_naming_an_object_negotiates_a_portable_bundle( + store: GrantingObjectStore, prefix: str, agent: _Agent, key: str ) -> None: + """``object_url``, or either of its older names, which stored steps may still carry.""" await A2AAgent.register_skill( _deployed(bundle=True), - **_skill_fields({"name": "review", "description": "Review work", "s3_uri": prefix}), + **_skill_fields({"name": "review", "description": "Review work", key: prefix}), ) assert set(agent.requests[0]["json"]) == {"name", "description", "skill_bundle"} @@ -317,7 +319,7 @@ async def test_a_bundle_skill_error_keeps_the_agents_body_out_of_logs( with pytest.raises(httpx.HTTPStatusError) as raised: await A2AAgent.add_skill( _deployed(bundle=True), - Skill(name="review", description="Review work", s3_url=prefix), + Skill(name="review", description="Review work", object_url=prefix), ) assert str(raised.value) == "skill add (review) failed with HTTP 400" @@ -384,6 +386,6 @@ async def post(self, url, *, json, timeout): monkeypatch.setattr(httpx.AsyncClient, "post", post) - await A2AAgent.add_skill(_deployed(bundle=True), Skill(name="review", description="Review work", s3_url=prefix)) + await A2AAgent.add_skill(_deployed(bundle=True), Skill(name="review", description="Review work", object_url=prefix)) assert on_loop and not any(on_loop) diff --git a/tst/unit/a2a_agent/object_transfer_test.py b/tst/unit/a2a_agent/object_transfer_test.py index 84619ddf..cc680292 100644 --- a/tst/unit/a2a_agent/object_transfer_test.py +++ b/tst/unit/a2a_agent/object_transfer_test.py @@ -233,11 +233,10 @@ def _answering(monkeypatch, body: dict) -> None: @pytest.mark.asyncio -async def test_fetch_trajectory_refuses_an_answer_that_only_names_an_s3_prefix(monkeypatch): +async def test_fetch_trajectory_reads_no_trajectory_from_an_answer_that_only_names_an_s3_prefix(monkeypatch): _answering(monkeypatch, {"trajectory_s3_prefix": "s3://b/t/"}) - with pytest.raises(RuntimeError, match="answered with trajectory_s3_prefix"): - await fetch_trajectory("https://agent.test/ext/trajectory", {"task_id": "t"}) + assert await fetch_trajectory("https://agent.test/ext/trajectory", {"task_id": "t"}) == FetchedTrajectory() @pytest.mark.asyncio diff --git a/tst/unit/store/local_id_acceptance_test.py b/tst/unit/store/local_id_acceptance_test.py index 3a8d1cc6..df9d5412 100644 --- a/tst/unit/store/local_id_acceptance_test.py +++ b/tst/unit/store/local_id_acceptance_test.py @@ -204,7 +204,7 @@ def stop(self, **kwargs): skills = f"a2a_validator/validator_skill/{key_segment(agent.id)}-v1" assert sorted(get_config().get_object_store_for(agent.id).list("a2a_validator/")) == [ f"{fixtures}/clip.mp4", f"{fixtures}/red.png", - *(f"{skills}/{name}/SKILL.md" for name in ("validator-probe-bundle", "validator-test-s3")), + *(f"{skills}/{name}/SKILL.md" for name in ("validator-probe-bundle", "validator-test-object")), ] assert configured.list("") == [] assert _local().count("tasks", Filter()) == 1 diff --git a/tst/unit/task_step/skill_object_url_test.py b/tst/unit/task_step/skill_object_url_test.py new file mode 100644 index 00000000..a64d1a69 --- /dev/null +++ b/tst/unit/task_step/skill_object_url_test.py @@ -0,0 +1,102 @@ +"""``Skill.object_url`` and the neutral skill flags replace their S3-named spellings. + +Stored add_skills steps carry ``s3_url``, so it is read forever and written beside ``object_url`` while workers on +older versions read only it. The ``s3_url`` keyword and the S3-named CLI flags still work, with a warning.""" + +import dataclasses +import logging +import warnings + +import pytest +from click.testing import CliRunner + +from agent_env.artifact import FileArtifactUniverse, SkillArtifact +from agent_env.artifact.artifacts import skill as skill_module +from agent_env.cli import cli +from agent_env.task_step.task_steps.add_skills import Skill + +_PREFIX = "mem://bucket/skills/review/" + + +def test_a_skill_named_by_object_url_carries_both_spellings_without_a_warning(): + with warnings.catch_warnings(): + warnings.simplefilter("error", DeprecationWarning) + skill = Skill(name="review", description="Review work", object_url=_PREFIX) + assert (skill.object_url, skill.s3_url) == (_PREFIX, _PREFIX) + assert skill.to_dict() == {"name": "review", "description": "Review work", "s3_url": _PREFIX, "object_url": _PREFIX} + + +def test_the_s3_url_keyword_still_works_and_is_counted(caplog): + with caplog.at_level(logging.WARNING, logger="agent_env.utils.deprecation"), \ + pytest.warns(DeprecationWarning, match=r"Skill\(s3_url=\)"): + skill = Skill(name="review", description="Review work", s3_url=_PREFIX) + assert (skill.object_url, skill.s3_url) == (_PREFIX, _PREFIX) + assert [r.deprecated_symbol for r in caplog.records if getattr(r, "event", None) == "agent_env_deprecated_symbol"] == [ + "Skill(s3_url=)" + ] + + +def test_an_inline_skill_reads_s3_url_as_none(): + """Readers outside agent-env check ``skill.s3_url is None``.""" + assert Skill(name="review", description="Review work", body="Inline").s3_url is None + + +def test_the_two_spellings_disagreeing_is_an_error_and_agreeing_is_a_copy(): + with pytest.raises(TypeError, match="got both object_url= and its deprecated spelling s3_url="): + Skill(name="review", description="Review work", object_url=_PREFIX, s3_url="mem://bucket/other/") + skill = Skill(name="review", description="Review work", object_url=_PREFIX) + assert dataclasses.replace(skill) == skill + + +@pytest.mark.parametrize("stored", [ + {"s3_url": _PREFIX}, + {"object_url": _PREFIX}, + {"s3_url": _PREFIX, "object_url": "mem://bucket/stale/"}, +], ids=["legacy-key", "neutral-key", "legacy-wins"]) +def test_a_stored_skill_loads_keyed_either_way(stored): + with warnings.catch_warnings(): + warnings.simplefilter("error", DeprecationWarning) + skill = Skill.from_dict({"name": "review", "description": "Review work", **stored}) + assert skill.object_url == _PREFIX + + +def test_the_skill_md_helpers_are_public_and_keep_their_private_names(): + assert skill_module._parse_skill_md is skill_module.parse_skill_md + assert skill_module._fetch_skill_md is skill_module.fetch_skill_md + frontmatter, body = skill_module.parse_skill_md(b"---\nname: demo\ndescription: A demo.\n---\nBody\n") + assert (frontmatter["name"], body.strip()) == ("demo", "Body") + + +def _bundle_dir(tmp_path): + (tmp_path / "files").mkdir() + (tmp_path / "files" / "a.txt").write_text("A") + return str(tmp_path / "files") + + +@pytest.mark.parametrize("flag,notice", [("--prefix-url", False), ("--s3-url", True)]) +def test_put_bundled_takes_its_prefix_under_either_flag(local_stores, tmp_path, flag, notice): + prefix = local_stores.get_object_store().object_url("bundles/cli/") + result = CliRunner().invoke(cli, ["artifact", "file-artifact-universe", "put-bundled", "--id", "cli-bundle", + "--file-dir", _bundle_dir(tmp_path), flag, prefix]) + assert result.exit_code == 0, result.output + assert f"bundle_object_url={prefix.rstrip('/')}/" in result.stdout + assert ("--s3-url is deprecated" in result.stderr) is notice + assert FileArtifactUniverse.get("cli-bundle").bundle_object_url == prefix.rstrip("/") + "/" + + +def test_giving_a_flag_under_both_spellings_is_a_usage_error(local_stores, tmp_path): + prefix = local_stores.get_object_store().object_url("bundles/cli/") + result = CliRunner().invoke(cli, ["artifact", "file-artifact-universe", "put-bundled", "--id", "cli-bundle", + "--file-dir", _bundle_dir(tmp_path), "--prefix-url", prefix, "--s3-url", prefix]) + assert result.exit_code == 2 + assert "--s3-url is the deprecated spelling of --prefix-url" in result.output + + +@pytest.mark.parametrize("flag", ["--prefix-url", "--s3-url"]) +def test_skill_put_reads_a_skill_from_a_prefix_under_either_flag(local_stores, flag): + store = local_stores.get_object_store() + store.put("skills/demo/SKILL.md", b"---\nname: demo\ndescription: A demo skill.\n---\nBody\n") + result = CliRunner().invoke(cli, ["artifact", "skill", "put", "--id", "demo", flag, store.object_url("skills/demo/")]) + assert result.exit_code == 0, result.output + assert "skill_object_url=" in result.stdout + assert SkillArtifact.get("demo").skill_name == "demo" diff --git a/tst/unit/task_step/stored_s3_form_fields_test.py b/tst/unit/task_step/stored_s3_form_fields_test.py index 0541a6ea..d99f9a3b 100644 --- a/tst/unit/task_step/stored_s3_form_fields_test.py +++ b/tst/unit/task_step/stored_s3_form_fields_test.py @@ -1,5 +1,5 @@ """Stored step documents written before the S3 transfer forms were removed: a field that only -named an S3 location is dropped, and an S3-form changelog source is refused rather than skipped.""" +named an S3 location is dropped, an S3-form changelog source included (none is left in stored tasks).""" import pytest @@ -15,18 +15,8 @@ def test_a_stored_skill_config_check_with_an_s3_probe_url_still_loads(): assert VerifyA2ASkillConfigStep.from_dict({**document, "skill_s3_url": "s3://bucket/prefix/"}).to_dict() == document -@pytest.mark.parametrize("cutoff", [None, 3], ids=["whole-changelog", "with-cutoff"]) -def test_a_stored_deploy_step_with_an_s3_changelog_source_is_refused(cutoff): - document = { - **DeployAgentTaskStep(id="d", version=None).to_dict(), - "agent_changelog_s3_prefix": "s3://bucket/agent_changelog/run-1/solver", - "agent_changelog_toolcall_position_exclusive": cutoff, - } - with pytest.raises(ValueError, match="agent_changelog_s3_prefix is no longer supported"): - DeployAgentTaskStep.from_dict(document) - - -def test_a_stored_deploy_step_with_an_empty_s3_changelog_field_still_loads(): +@pytest.mark.parametrize("prefix", ["s3://bucket/agent_changelog/run-1/solver", None], ids=["set", "empty"]) +def test_a_stored_deploy_step_with_an_s3_changelog_field_loads_without_it(prefix): step = DeployAgentTaskStep(id="d", version=None) document = step.to_dict() - assert DeployAgentTaskStep.from_dict({**document, "agent_changelog_s3_prefix": None}).to_dict() == document + assert DeployAgentTaskStep.from_dict({**document, "agent_changelog_s3_prefix": prefix}).to_dict() == document