fix: address @awsarron review feedback

- assets/manager.py: remove 'import os as _os' alias — just import os at top
- simulation/models.py: clarify _model vs _data vs _backend_state roles in
  docstring so backend implementers know which to use (each has a distinct
  purpose: core model handle, core state handle, catch-all dict)
- utils.py: decouple get_base_dir() from STRANDS_ASSETS_DIR. Introduce
  STRANDS_BASE_DIR as the explicit override for the base directory so
  user_robots.json no longer lands in an unexpected parent of the assets
  path. STRANDS_ASSETS_DIR now *only* moves the assets subtree.
- registry/user_registry.py: update module docstring to reflect the new
  STRANDS_BASE_DIR semantics
- tests/test_user_registry.py: update fixture + integration tests to set
  STRANDS_BASE_DIR and STRANDS_ASSETS_DIR independently; add tests asserting
  that STRANDS_ASSETS_DIR does *not* move the registry / base dir

Author: cagataycali

strands_robots/assets/manager.py

@@ -8,6 +8,7 @@
 """
 
 import logging
+import os
 from pathlib import Path
 
 from strands_robots.registry import (
@@ -73,14 +74,12 @@ def _has_meshes(directory: Path) -> bool:
     if cached is not None:
         return cached
 
-    import os as _os
-
     def _walk(path: str) -> bool:
         try:
-            with _os.scandir(path) as it:
+            with os.scandir(path) as it:
                 for entry in it:
                     if entry.is_file(follow_symlinks=False):
-                        ext = _os.path.splitext(entry.name)[1].lower()
+                        ext = os.path.splitext(entry.name)[1].lower()
                         if ext in _MESH_EXTS:
                             return True
                     elif entry.is_dir(follow_symlinks=False) and _walk(entry.path):

strands_robots/registry/user_registry.py

@@ -5,9 +5,13 @@
 alongside the asset cache.
 
 File location (in priority order):
-    1. ``$STRANDS_ASSETS_DIR/user_robots.json``
+    1. ``$STRANDS_BASE_DIR/user_robots.json``
     2. ``~/.strands_robots/user_robots.json``
 
+Note:
+    ``STRANDS_ASSETS_DIR`` only controls where *assets* live, not the
+    user registry. Use ``STRANDS_BASE_DIR`` to relocate user metadata.
+
 At load time the user overlay is merged *on top of* the package
 ``robots.json`` — user entries win on name collision, so you can also
 override built-in robots locally.

strands_robots/simulation/models.py

@@ -100,10 +100,22 @@ class TrajectoryStep:
 class SimWorld:
     """Complete simulation world state.
 
-    Backend-independent state with engine-specific internals isolated in
-    ``_model``, ``_data``, and ``_backend_state`` — all typed as ``Any``
-    or ``dict`` so that each backend can store its own native handles
-    without leaking implementation details into this base module.
+    Backend-independent state with engine-specific internals kept in three
+    escape hatches, each with a distinct role so backend implementers know
+    which to use:
+
+    * ``_model``: the physics engine's **core model handle** — the single
+      compiled/loaded representation of the scene (e.g. ``mujoco.MjModel``,
+      Isaac's ``Scene``, PyBullet's body registry). Every backend has one.
+    * ``_data``: the physics engine's **core simulation state handle** —
+      the mutable per-step state companion to ``_model``
+      (e.g. ``mujoco.MjData``, Isaac's ``World``). Every backend has one.
+    * ``_backend_state``: a **catch-all dict** for everything else the
+      backend needs to persist — generated XML, temp dirs, recording
+      buffers, caches, etc. Prefer this over adding new fields here.
+
+    All three are typed ``Any``/``dict`` so nothing leaks engine-specific
+    types into this base module.
     """
 
     robots: dict[str, SimRobot] = field(default_factory=dict)
@@ -115,16 +127,15 @@ class SimWorld:
     status: SimStatus = SimStatus.IDLE
     sim_time: float = 0.0
     step_count: int = 0
-    # Engine-specific internals (set after world is built by the backend).
-    # Each backend stores its own native handles here.
+    # Engine core handles — set after the backend builds the world.
+    # Use these for the primary model/state objects only; put everything
+    # else in ``_backend_state`` below.
     _model: Any = None  # Engine-specific model handle (e.g. MjModel, Scene)
     _data: Any = None  # Engine-specific data handle (e.g. MjData, World)
-    # Backend-specific state bag — backends store format-specific data here
-    # instead of polluting this base class with implementation details.
-    # Recording state (``_recording``, ``_trajectory``, ``_dataset_recorder``)
-    # and engine-specific handles (e.g. MuJoCo ``xml``, ``robot_base_xml``,
-    # ``tmpdir``) all go in this dict rather than being declared as separate
-    # fields on the base class.
+    # Catch-all for backend-specific state that isn't the core model/data.
+    # Examples: generated XML strings, temp dirs, recording buffers
+    # (``_recording``, ``_trajectory``, ``_dataset_recorder``), caches, etc.
+    # Prefer this over adding new fields to ``SimWorld``.
     _backend_state: dict[str, Any] = field(default_factory=dict)
     # Physics state checkpoints (used by save_state/restore_state in PR #85).
     # Kept as a top-level field — requested by @yinsong1986 during review to

strands_robots/utils.py

@@ -64,18 +64,25 @@ def require_optional(
 def get_base_dir() -> Path:
     """Get the base directory for strands-robots user data.
 
-    If ``STRANDS_ASSETS_DIR`` is set, returns its parent
-    (the assets dir is a subdirectory of the base).
-    Otherwise returns ``~/.strands_robots/``.
+    Resolution (in priority order):
+
+    1. ``STRANDS_BASE_DIR`` env var — explicit override. Use this when
+       you want to relocate *all* strands-robots user data (assets,
+       user registry, caches) to a non-default location.
+    2. ``~/.strands_robots/`` — default.
+
+    Note:
+        ``STRANDS_ASSETS_DIR`` **only** controls the assets subdirectory
+        (see :func:`get_assets_dir`). It does *not* move the base dir,
+        so user-level metadata like ``user_robots.json`` always lands in
+        a predictable location rather than wherever the assets happen
+        to be pointed.
 
     Returns:
         Path to the base directory (created if needed).
     """
-    custom = os.getenv("STRANDS_ASSETS_DIR")
-    if custom:
-        d = Path(custom).parent
-    else:
-        d = DEFAULT_BASE_DIR
+    custom = os.getenv("STRANDS_BASE_DIR")
+    d = Path(custom) if custom else DEFAULT_BASE_DIR
     d.mkdir(parents=True, exist_ok=True)
     return d
 

tests/test_user_registry.py

@@ -35,9 +35,15 @@
 
 @pytest.fixture(autouse=True)
 def _isolate_registry(tmp_path, monkeypatch):
-    """Point STRANDS_ASSETS_DIR to a temp dir and clear caches for every test."""
+    """Point STRANDS_BASE_DIR + STRANDS_ASSETS_DIR to temp dirs for every test.
+
+    ``STRANDS_BASE_DIR`` controls where ``user_robots.json`` lives.
+    ``STRANDS_ASSETS_DIR`` controls where robot asset directories live.
+    The two are independent — the base dir is not derived from the assets dir.
+    """
     assets_dir = tmp_path / "assets"
     assets_dir.mkdir()
+    monkeypatch.setenv("STRANDS_BASE_DIR", str(tmp_path))
     monkeypatch.setenv("STRANDS_ASSETS_DIR", str(assets_dir))
     _invalidate_cache()
     yield
@@ -288,20 +294,34 @@ def test_import_error_returns_data_unchanged(self):
 
 
 # ===========================================================================
-# STRANDS_ASSETS_DIR integration
+# STRANDS_BASE_DIR integration
 # ===========================================================================
 
 
-class TestStrandsAssetsDirIntegration:
-    """Registry file location respects STRANDS_ASSETS_DIR env var."""
+class TestStrandsBaseDirIntegration:
+    """Registry file location respects STRANDS_BASE_DIR env var.
+
+    STRANDS_ASSETS_DIR intentionally does NOT move the registry — it only
+    controls where asset directories live. See utils.get_base_dir() docstring.
+    """
 
-    def test_registry_file_in_parent_of_assets_dir(self, tmp_path):
-        custom = tmp_path / "custom_assets"
+    def test_registry_file_lives_in_base_dir(self, tmp_path):
+        custom = tmp_path / "custom_base"
         custom.mkdir()
-        with mock.patch.dict(os.environ, {"STRANDS_ASSETS_DIR": str(custom)}):
-            assert _get_user_registry_path().parent == custom.parent
+        with mock.patch.dict(os.environ, {"STRANDS_BASE_DIR": str(custom)}, clear=False):
+            assert _get_user_registry_path().parent == custom
+
+    def test_assets_dir_does_not_move_registry(self, tmp_path, monkeypatch):
+        """Setting only STRANDS_ASSETS_DIR must not change the registry location."""
+        monkeypatch.delenv("STRANDS_BASE_DIR", raising=False)
+        custom_assets = tmp_path / "custom_assets"
+        custom_assets.mkdir()
+        monkeypatch.setenv("STRANDS_ASSETS_DIR", str(custom_assets))
+        # Registry should land under the default base, not the assets dir.
+        assert ".strands_robots" in str(_get_user_registry_path())
 
     def test_defaults_to_dot_strands_robots(self, monkeypatch):
+        monkeypatch.delenv("STRANDS_BASE_DIR", raising=False)
         monkeypatch.delenv("STRANDS_ASSETS_DIR", raising=False)
         assert ".strands_robots" in str(_get_user_registry_path())
 
@@ -328,17 +348,29 @@ def test_custom(self, tmp_path, monkeypatch):
 
 
 class TestGetBaseDir:
-    """get_base_dir() returns parent of STRANDS_ASSETS_DIR or ~/.strands_robots/."""
+    """get_base_dir() returns STRANDS_BASE_DIR or ~/.strands_robots/.
+
+    It is independent of STRANDS_ASSETS_DIR by design — the base dir holds
+    user metadata (user_robots.json) and should not move just because the
+    user repoints the asset cache.
+    """
 
     def test_default(self, monkeypatch):
+        monkeypatch.delenv("STRANDS_BASE_DIR", raising=False)
         monkeypatch.delenv("STRANDS_ASSETS_DIR", raising=False)
         assert str(get_base_dir()).endswith(".strands_robots")
 
     def test_custom(self, tmp_path, monkeypatch):
-        custom = tmp_path / "custom_assets"
+        custom = tmp_path / "custom_base"
         custom.mkdir()
-        monkeypatch.setenv("STRANDS_ASSETS_DIR", str(custom))
-        assert get_base_dir() == tmp_path
+        monkeypatch.setenv("STRANDS_BASE_DIR", str(custom))
+        assert get_base_dir() == custom
+
+    def test_assets_dir_does_not_move_base(self, tmp_path, monkeypatch):
+        """STRANDS_ASSETS_DIR must not affect get_base_dir()."""
+        monkeypatch.delenv("STRANDS_BASE_DIR", raising=False)
+        monkeypatch.setenv("STRANDS_ASSETS_DIR", str(tmp_path / "assets"))
+        assert str(get_base_dir()).endswith(".strands_robots")
 
 
 class TestResolveAssetPath: