fix: address PR #84 follow-up items (#105) (#106)

Author: cagataycali

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,
             }
         )

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)

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}"