Apply codex review nits to rename pass

Use a path-separator-terminated source prefix in
``_rename_builder_labels`` so two source paths that share a string
prefix (e.g. ``/Sources/protoA`` and ``/Sources/protoAB``) cannot
cross-contaminate when both feed the same set of envs. The world-id
guard alone is not sufficient in that case because both sources
contribute to the same ``world_roots``; only the boundary on
``startswith`` separates them. Slice index for ``swap`` keeps the
original "after the source root" semantics, so no other behavior
changes.

Tighten the TODO on ``_scope_custom_frequencies`` to point a future
reader at the exact SHA in ``source/isaaclab_newton/setup.py``'s
``newton @ git+...@<sha>`` URL, since Newton's package version
string does not encode the SHA. Drop the internal-TODO bullet from
the user-facing CHANGELOG entry and instead document the boundary-
prefix tightening there alongside the original rename fix.

Add tests:

* ``test_sparse_env_ids`` — non-contiguous env ids map to the right
  per-env root.
* ``TestRenamePass2Generality`` — multiple coexisting custom
  frequencies, multiple string columns at one frequency, and a
  registered-but-empty string column all behave correctly.
* ``TestRenameMultiSource`` — explicit regression for the prefix
  overlap fix; both sources feed the same envs so the world-id
  guard cannot mask the boundary bug.

Author: hujc7

source/isaaclab_newton/docs/CHANGELOG.rst

@@ -12,14 +12,10 @@ Fixed
   now also walks string-typed custom-attribute columns whose frequency declares a
   ``references="world"`` companion, rewriting their per-row source-path prefix to
   the destination world root in the same pass that handles built-in label arrays.
-  Adds ``constraint_mimic`` to that built-in pass for completeness.
-
-* Added a ``TODO`` to :func:`~isaaclab_newton.cloner.newton_replicate._scope_custom_frequencies`
-  pointing at the upstream Newton fix
-  (`newton-physics/newton#2659 <https://github.com/newton-physics/newton/pull/2659>`_,
-  commit ``1d22547``). Once the IsaacLab Newton pin includes that commit,
-  ``parse_usd`` scopes the walk to ``root_path`` itself and the wrapper becomes
-  a redundant identity check on already-in-scope prims.
+  Adds ``constraint_mimic`` to that built-in pass for completeness. The prefix
+  match uses a path-separator boundary so a source path that is a string prefix
+  of another (e.g. ``/Sources/protoA`` vs ``/Sources/protoAB``) does not
+  cross-contaminate during the rename.
 
 
 0.5.26 (2026-04-29)

source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py

@@ -33,11 +33,15 @@ def _scope_custom_frequencies(builder: ModelBuilder, root_path: str) -> None:
     Frequencies with ``usd_prim_filter = None`` are skipped; Newton excludes them
     from the traversal loop entirely so they carry no contamination risk.
 
-    TODO: Remove once IsaacLab's Newton pin includes
-    https://github.com/newton-physics/newton/pull/2659 (commit 1d22547, "Scope
-    USD custom-frequency parsing"). After that bump, ``parse_usd`` scopes the
-    walk to ``root_path`` itself and this wrapper becomes a redundant identity
-    check on already-in-scope prims. Safe to keep until the bump lands.
+    TODO: Remove once IsaacLab's Newton pin includes upstream commit
+    ``1d22547`` (https://github.com/newton-physics/newton/pull/2659,
+    "Scope USD custom-frequency parsing"). To check, look at the SHA in
+    ``source/isaaclab_newton/setup.py``'s ``newton @ git+https://github.com/
+    newton-physics/newton.git@<sha>`` URL and confirm it is at or after
+    ``1d22547e704855bfbb67777e281ac23eaab95af3``. After that bump,
+    ``parse_usd`` scopes the walk to ``root_path`` itself and this wrapper
+    becomes a redundant identity check on already-in-scope prims. Safe to
+    keep until the bump lands.
 
     Args:
         builder: A proto ``ModelBuilder`` that has already had custom attributes
@@ -216,17 +220,22 @@ def _rename_builder_labels(
     """
     # per-source, per-world renaming (strict prefix swap), compact style preserved
     for i, src_path in enumerate(sources):
-        src_prefix_len = len(src_path.rstrip("/"))
+        # Boundary-terminated prefix prevents over-matching when one source path is a
+        # prefix of another (e.g. ``/Sources/protoA`` vs ``/Sources/protoAB``).
+        src_prefix = src_path.rstrip("/") + "/"
+        src_prefix_len = len(src_prefix) - 1  # slice index for ``swap`` keeps the leading "/"
         swap = lambda name, new_root: new_root + name[src_prefix_len:]  # noqa: E731
         world_cols = torch.nonzero(mapping[i], as_tuple=True)[0].tolist()
         # Map Newton world IDs (sequential) to destination paths using env_ids
         world_roots = {int(env_ids[c]): destinations[i].format(int(env_ids[c])) for c in world_cols}
 
         def _rename_pair(values, worlds):
+            # ``values`` and ``worlds`` are required to be the same length by the call
+            # sites above; ``min`` is defensive against a future builder oddity.
             n = min(len(values), len(worlds))
             for k in range(n):
                 world_id = int(worlds[k])
-                if world_id in world_roots and isinstance(values[k], str) and values[k].startswith(src_path):
+                if world_id in world_roots and isinstance(values[k], str) and values[k].startswith(src_prefix):
                     values[k] = swap(values[k], world_roots[world_id])
 
         # Pass 1: built-in label arrays. Each has a paired ``*_world`` int column.

source/isaaclab_newton/test/cloner/test_rename_builder_labels.py

@@ -139,6 +139,141 @@ def test_world_outside_env_ids_left_untouched(self):
         self._rename(b)
         self.assertEqual(b.body_label[-1], f"{_SRC}/body_99")
 
+    def test_sparse_env_ids(self):
+        """Non-contiguous ``env_ids`` (e.g. [10, 20, 30]) must rewrite using the right per-env root."""
+        worlds = [10, 20, 30]
+        b = newton.ModelBuilder()
+        SolverMuJoCo.register_custom_attributes(b)
+        _inject_builtins(b, ("body",), _SRC, worlds)
+        env_ids = torch.tensor(worlds, dtype=torch.int32)
+        mapping = torch.ones(1, len(worlds), dtype=torch.bool)
+        _rename_builder_labels(b, [_SRC], [_DST], env_ids, mapping)
+        for k, w in enumerate(b.body_world):
+            self.assertEqual(b.body_label[k], f"/World/envs/env_{int(w)}/body_{int(w)}")
+
+
+class TestRenamePass2Generality(unittest.TestCase):
+    """Pass 2 must generalize across coexisting frequencies and multiple string columns."""
+
+    def setUp(self):
+        self.worlds = [0, 1]
+        self.env_ids = torch.tensor(self.worlds, dtype=torch.int32)
+        self.mapping = torch.ones(1, len(self.worlds), dtype=torch.bool)
+
+    def _register_synthetic_freq(self, builder, freq_name, world_attr_name, str_attr_names):
+        """Register a ``syn:<freq_name>`` custom frequency, a ``references="world"`` int companion, and one or more ``dtype=str`` columns at it."""
+        freq = f"syn:{freq_name}"
+        builder.add_custom_frequency(newton.ModelBuilder.CustomFrequency(name=freq_name, namespace="syn"))
+        builder.add_custom_attribute(
+            newton.ModelBuilder.CustomAttribute(
+                name=world_attr_name,
+                frequency=freq,
+                dtype=int,
+                default=0,
+                namespace="syn",
+                references="world",
+            )
+        )
+        for n in str_attr_names:
+            builder.add_custom_attribute(
+                newton.ModelBuilder.CustomAttribute(
+                    name=n,
+                    frequency=freq,
+                    dtype=str,
+                    default="",
+                    namespace="syn",
+                )
+            )
+
+    def _populate(self, builder, freq, world_attr_name, str_attr_names, worlds):
+        wa = builder.custom_attributes[f"syn:{world_attr_name}"]
+        if wa.values is None:
+            wa.values = []
+        for w in worlds:
+            wa.values.append(w)
+        for n in str_attr_names:
+            sa = builder.custom_attributes[f"syn:{n}"]
+            if sa.values is None:
+                sa.values = []
+            for w in worlds:
+                sa.values.append(f"{_SRC}/{n}_{w}")
+        builder._custom_frequency_counts[freq] = builder._custom_frequency_counts.get(freq, 0) + len(worlds)
+
+    def test_two_coexisting_custom_frequencies(self):
+        """Each registered ``references='world'`` companion must drive its own frequency's str columns."""
+        b = newton.ModelBuilder()
+        self._register_synthetic_freq(b, "freqA", "freqA_world", ["freqA_label"])
+        self._register_synthetic_freq(b, "freqB", "freqB_world", ["freqB_label"])
+        self._populate(b, "syn:freqA", "freqA_world", ["freqA_label"], self.worlds)
+        self._populate(b, "syn:freqB", "freqB_world", ["freqB_label"], self.worlds)
+        _rename_builder_labels(b, [_SRC], [_DST], self.env_ids, self.mapping)
+        for n in ("freqA_label", "freqB_label"):
+            wa = b.custom_attributes[f"syn:{n.split('_')[0]}_world"].values
+            sa = b.custom_attributes[f"syn:{n}"].values
+            for k, w in enumerate(wa):
+                self.assertEqual(sa[k], f"/World/envs/env_{int(w)}/{n}_{int(w)}")
+
+    def test_multiple_string_columns_at_one_frequency(self):
+        """Two str columns sharing one frequency must both be rewritten using the shared world companion."""
+        b = newton.ModelBuilder()
+        self._register_synthetic_freq(b, "freqA", "freqA_world", ["freqA_label", "freqA_alt"])
+        self._populate(b, "syn:freqA", "freqA_world", ["freqA_label", "freqA_alt"], self.worlds)
+        _rename_builder_labels(b, [_SRC], [_DST], self.env_ids, self.mapping)
+        wa = b.custom_attributes["syn:freqA_world"].values
+        for n in ("freqA_label", "freqA_alt"):
+            sa = b.custom_attributes[f"syn:{n}"].values
+            for k, w in enumerate(wa):
+                self.assertEqual(sa[k], f"/World/envs/env_{int(w)}/{n}_{int(w)}")
+
+    def test_empty_values_pass_through(self):
+        """A registered-but-empty string column must not crash the rename pass."""
+        b = newton.ModelBuilder()
+        self._register_synthetic_freq(b, "freqA", "freqA_world", ["freqA_label"])
+        # values stay None (registered, never populated)
+        _rename_builder_labels(b, [_SRC], [_DST], self.env_ids, self.mapping)
+        # Fully populate after the no-op rename: ensures the early-return guard didn't corrupt state.
+        self._populate(b, "syn:freqA", "freqA_world", ["freqA_label"], self.worlds)
+        self.assertEqual(len(b.custom_attributes["syn:freqA_label"].values), len(self.worlds))
+
+
+class TestRenameMultiSource(unittest.TestCase):
+    """Multi-source handling must not cross-contaminate when source paths share a string prefix."""
+
+    def test_prefix_overlap_does_not_cross_contaminate(self):
+        """Sources whose paths share a string prefix and that both feed the same envs must not cross-rename.
+
+        Common IL pattern: a robot proto and an object proto both feed every env. If the two source
+        paths share a string prefix (``/Sources/protoA`` and ``/Sources/protoAB``), iter 0
+        (``src=protoA``) sees the protoAB rows for the same world ids it owns and would over-match
+        them under a non-boundary ``startswith``. The world-id guard alone does not catch this case
+        because both sources contribute to the same set of worlds.
+        """
+        sources = ["/Sources/protoA", "/Sources/protoAB"]
+        # 2 envs, both fed by both sources.
+        env_ids = torch.tensor([0, 1], dtype=torch.int32)
+        mapping = torch.tensor([[1, 1], [1, 1]], dtype=torch.bool)
+        b = newton.ModelBuilder()
+        SolverMuJoCo.register_custom_attributes(b)
+        # One body row from each source per env: 4 rows total, world ids interleaved.
+        b.body_label.extend(
+            [
+                f"{sources[0]}/body",  # row 0: protoA, world 0
+                f"{sources[1]}/body",  # row 1: protoAB, world 0
+                f"{sources[0]}/body",  # row 2: protoA, world 1
+                f"{sources[1]}/body",  # row 3: protoAB, world 1
+            ]
+        )
+        b.body_world.extend([0, 0, 1, 1])
+        _rename_builder_labels(b, sources, ["/World/envs/env_{}", "/World/envs/env_{}"], env_ids, mapping)
+        # Each row must end up under its own per-env root with the suffix preserved verbatim.
+        # Without the "/" boundary on ``startswith``, iter 0 (src=protoA) would match rows 1 and 3
+        # because ``/Sources/protoAB/body``.startswith(``/Sources/protoA``) is True, rewriting them
+        # to ``/World/envs/env_<w>/B/body`` (wrong suffix).
+        self.assertEqual(b.body_label[0], "/World/envs/env_0/body")
+        self.assertEqual(b.body_label[1], "/World/envs/env_0/body")
+        self.assertEqual(b.body_label[2], "/World/envs/env_1/body")
+        self.assertEqual(b.body_label[3], "/World/envs/env_1/body")
+
 
 if __name__ == "__main__":
     unittest.main()