diff --git a/strands_robots/assets/manager.py b/strands_robots/assets/manager.py index f8a2b0b..ca610a6 100644 --- a/strands_robots/assets/manager.py +++ b/strands_robots/assets/manager.py @@ -111,6 +111,49 @@ def _resolve_candidates(asset_dir_name: str, xml_file: str, name: str) -> list[P return candidates +def is_robot_asset_present(name: str) -> bool: + """Check whether a robot's model XML exists on disk without triggering downloads. + + Pure filesystem check — no auto-download, no mesh walk, no network. + Use this for status queries (e.g. ``download_assets(action="status")``) + where you need to quickly check presence without side effects. + + Args: + name: Robot name (canonical or alias). + + Returns: + True if the model XML file exists on at least one search path. + """ + info = get_robot(name) + if not info or "asset" not in info: + return False + + asset = info["asset"] + xml_file: str = str(asset["model_xml"]) + asset_dir_name: str = str(asset["dir"]) + + # Check user-registered path first + user_path = info.get("_user_asset_path") + if user_path: + try: + user_model = safe_join(Path(user_path), xml_file) + if user_model.exists(): + return True + except ValueError: + pass + + # Check standard search paths + for search_dir in get_search_paths(): + try: + model_path = safe_join(search_dir, f"{asset_dir_name}/{xml_file}") + if model_path.exists(): + return True + except ValueError: + continue + + return False + + def resolve_model_path( name: str, prefer_scene: bool = False, @@ -250,21 +293,28 @@ def get_robot_info(name: str) -> dict | None: def list_available_robots() -> list[dict]: """List all available robot models with their info. + Uses :func:`is_robot_asset_present` for a fast filesystem-only check + per robot instead of the heavier :func:`resolve_model_path` which can + trigger auto-downloads and mesh cache walks. + Returns: List of dicts with name, description, joints, category, available, path. """ robots = [] for r in list_robots(mode="sim"): - path = resolve_model_path(r["name"]) - info = get_robot(r["name"]) or {} + name = r["name"] + present = is_robot_asset_present(name) + info = get_robot(name) or {} + # Only resolve full path when asset is present — avoids download attempts + path = resolve_model_path(name) if present else None robots.append( { - "name": r["name"], + "name": name, "description": r.get("description", ""), "joints": r.get("joints"), "category": r.get("category", ""), "dir": info.get("asset", {}).get("dir", ""), - "available": path is not None, + "available": present, "path": str(path) if path else None, } ) diff --git a/strands_robots/utils.py b/strands_robots/utils.py index b56f3f5..f61de52 100644 --- a/strands_robots/utils.py +++ b/strands_robots/utils.py @@ -171,12 +171,11 @@ def get_search_paths() -> list[Path]: Order (local assets take priority over defaults): 1. User asset dir (``STRANDS_ASSETS_DIR`` or ``~/.strands_robots/assets/``) - 2. ``CWD/assets`` (project-local) + 2. ``CWD/assets`` (project-local, deduplicated if it resolves to the same dir) """ paths: list[Path] = [] user_cache = get_assets_dir() - if user_cache not in paths: - paths.append(user_cache) + paths.append(user_cache) cwd_assets = Path.cwd() / "assets" if cwd_assets not in paths: paths.append(cwd_assets) diff --git a/tests/test_registry_resolves.py b/tests/test_registry_resolves.py new file mode 100644 index 0000000..122e42e --- /dev/null +++ b/tests/test_registry_resolves.py @@ -0,0 +1,110 @@ +"""Integration test: every robot in robots.json resolves to an existing file. + +Walks the full registry and asserts that every ``asset.dir / asset.model_xml`` +path is a valid relative path that would resolve under the search directories +**if** the assets were downloaded. This catches: + - Typos in ``robots.json`` (e.g. ``asimov_v0.xml`` → ``xmls/asimov.xml``) + - Upstream layout regressions in robot_descriptions / MuJoCo Menagerie + - Missing ``dir`` or ``model_xml`` keys in sim-capable robots + - Path traversal sequences in registry entries + +The test does NOT require downloaded assets or GPU — it only validates the +registry metadata itself (directory/file names, path safety). Run it in the +unit or integ hatch env. + +Added as follow-up to PR #84 review (issue #105, task 2). +""" + +import json +from pathlib import Path + +import pytest + +# ───────────────────────────────────────────────────────────────────── +# Load registry directly to avoid import side effects +# ───────────────────────────────────────────────────────────────────── + +_REGISTRY_PATH = Path(__file__).resolve().parent.parent / "strands_robots" / "registry" / "robots.json" + + +def _load_registry() -> dict: + """Load robots.json and return the robots dict.""" + with open(_REGISTRY_PATH) as f: + data = json.load(f) + return data.get("robots", data) + + +_ROBOTS = _load_registry() + +# Robots that have simulation assets (asset.dir + asset.model_xml). +# Hardware-only robots (e.g. lekiwi, reachy2) have no 'asset' key. +_SIM_ROBOTS = {name: info for name, info in _ROBOTS.items() if "asset" in info} +_SIM_ROBOT_NAMES = list(_SIM_ROBOTS.keys()) + + +# ───────────────────────────────────────────────────────────────────── +# Tests for ALL robots (sim + hardware-only) +# ───────────────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize("name", list(_ROBOTS.keys()), ids=list(_ROBOTS.keys())) +def test_registry_entry_is_well_formed(name: str) -> None: + """Every robot must have a description and category.""" + info = _ROBOTS[name] + assert "description" in info, f"Robot '{name}' missing 'description'" + assert "category" in info, f"Robot '{name}' missing 'category'" + + +@pytest.mark.parametrize("name", list(_ROBOTS.keys()), ids=list(_ROBOTS.keys())) +def test_registry_resolve_via_api(name: str) -> None: + """Verify the registry API can look up each robot without errors.""" + from strands_robots.registry import get_robot, resolve_name + + canonical = resolve_name(name) + assert canonical is not None, f"resolve_name({name!r}) returned None" + + info = get_robot(name) + assert info is not None, f"get_robot({name!r}) returned None" + + +# ───────────────────────────────────────────────────────────────────── +# Tests for sim-capable robots only (have 'asset' key) +# ───────────────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize("name", _SIM_ROBOT_NAMES, ids=_SIM_ROBOT_NAMES) +def test_sim_robot_has_required_asset_fields(name: str) -> None: + """Sim robots must have asset.dir and asset.model_xml.""" + asset = _SIM_ROBOTS[name]["asset"] + assert "dir" in asset, f"Robot '{name}' missing 'asset.dir'" + assert "model_xml" in asset, f"Robot '{name}' missing 'asset.model_xml'" + assert isinstance(asset["dir"], str) and asset["dir"], f"Robot '{name}' has empty 'asset.dir'" + assert isinstance(asset["model_xml"], str) and asset["model_xml"], f"Robot '{name}' has empty 'asset.model_xml'" + + +@pytest.mark.parametrize("name", _SIM_ROBOT_NAMES, ids=_SIM_ROBOT_NAMES) +def test_sim_robot_paths_are_safe(name: str) -> None: + """No registry path should contain traversal sequences.""" + asset = _SIM_ROBOTS[name]["asset"] + dir_name = asset.get("dir", "") + model_xml = asset.get("model_xml", "") + scene_xml = asset.get("scene_xml", "") + + for field, value in [("dir", dir_name), ("model_xml", model_xml), ("scene_xml", scene_xml)]: + if not value: + continue + assert ".." not in value, f"Robot '{name}' asset.{field} contains '..': {value!r}" + assert not value.startswith("/"), f"Robot '{name}' asset.{field} is absolute: {value!r}" + + +@pytest.mark.parametrize("name", _SIM_ROBOT_NAMES, ids=_SIM_ROBOT_NAMES) +def test_sim_robot_xml_has_xml_extension(name: str) -> None: + """model_xml and scene_xml should end with .xml.""" + asset = _SIM_ROBOTS[name]["asset"] + model_xml = asset.get("model_xml", "") + scene_xml = asset.get("scene_xml", "") + + if model_xml: + assert model_xml.endswith(".xml"), f"Robot '{name}' model_xml doesn't end with .xml: {model_xml!r}" + if scene_xml: + assert scene_xml.endswith(".xml"), f"Robot '{name}' scene_xml doesn't end with .xml: {scene_xml!r}"