Fix clone-plan source resolution for nested destination templates

[ooctipus]

Summary

• resolve_clone_plan_source raised a ValueError whenever a path expression was owned by two nested clone-plan destination templates — e.g. a camera cloned under a robot at /World/envs/env_{}/Robot/ee_link/palm_link/Camera, which matches both the robot template /World/envs/env_{}/Robot and the camera template. This crashed scene creation for any sensor/asset mounted beneath another cloned asset.
• The function now selects the most specific (longest-matching) template, mirroring the "nearest owner wins" logic already used by iter_clone_plan_matches. It only raises when a path is owned by multiple distinct, equally specific templates (a genuine ambiguity).
• The same-template OR-merge coverage check is unchanged and now scoped to the winning template's rows.

Test plan

• test_resolve_clone_plan_source_nested_templates_pick_most_specific — nested ancestor/descendant templates resolve to the descendant; ancestor-only paths still resolve against the ancestor.
• test_resolve_clone_plan_source_ambiguous_templates_raise — two distinct, equally specific templates still raise ValueError.
• test_resolve_clone_plan_source_merges_same_template_rows — heterogeneous source rows sharing one template OR-merge their masks (split env coverage resolves).
• test_resolve_clone_plan_source_partial_coverage_raises — merged mask with an uncovered env raises NotImplementedError.
• Tests pass on CPU and CUDA: ./isaaclab.sh -p -m pytest source/isaaclab/test/sim/test_cloner.py -k "resolve_clone_plan_source or iter_clone_plan_matches"

[isaaclab-review-bot]

Code Review — PR #5929

Summary: Fixes resolve_clone_plan_source to handle nested clone-plan destination templates by selecting the most specific (longest-matching) template rather than raising a ValueError.

---

✅ Strengths

• Correct specificity heuristic. Using shortest suffix length (= longest matched prefix) as a proxy for "nearest owner" is sound and mirrors the logic in iter_clone_plan_matches.
• Backwards compatible. The ambiguity error is still raised for genuinely conflicting templates (equally specific, distinct templates), so callers that rely on this error continue to work.
• Solid test coverage. Four new tests cover: nested resolution, ambiguity error, same-template OR-merge, and partial coverage failure.
• Changelog fragment included. Well-written description of the fix.

---

🔍 Findings

1. Non-deterministic matching_suffix for multi-row same-template (Low severity)

matching_suffix = next(suffix for template, suffix, _ in candidates if template == matching_template)

When the winning template appears in multiple rows, next(...) returns the suffix from whichever candidate was appended first. This works correctly because get_suffix(path_expr, destination_template) is deterministic — given the same path_expr and destination_template, the suffix is always identical regardless of which row it came from. No bug, but adding a brief comment clarifying why any row's suffix suffices would help future readers.

2. Consider sorting owning_templates in error message (Nit)

raise ValueError(f"path_expr {path_expr!r}: matches multiple destination templates {sorted(owning_templates)}.")

sorted(owning_templates) is already used — good, this gives a stable error message. 👍

3. Minor: list comprehension builds full candidate list before filtering (Nit)

The old code bailed early on the first ambiguity. The new code collects all candidates first, then filters. For typical clone plans with a handful of templates, this is negligible. If clone plans can grow very large (hundreds of templates), a two-pass approach might be worth noting. However, in practice this is unlikely to matter — the number of destination templates is typically very small.

4. Test test_resolve_clone_plan_source_ambiguous_templates_raise uses unusual path semantics

The test uses "/World/envs/{}/Robot" and "/World/{}/env_0/Robot" as competing templates. While this correctly exercises the equal-suffix-length ambiguity path, the second template (/World/{}/env_0/Robot) is somewhat unusual in real usage. This is fine for unit testing but worth noting that real ambiguity would be rare.

---

🏁 Verdict

Looks good. The fix is well-reasoned, correctly handles the nested template case, and has comprehensive test coverage. The implementation is clean and the docstring/changelog updates accurately describe the behaviour change. No blocking issues found.

Minor CI checks are still pending (Docker builds, installation tests) but core checks (pre-commit, build wheel, changelog fragments) all pass.

[greptile-apps]

Greptile Summary

This PR fixes resolve_clone_plan_source crashing with ValueError when a path expression is owned by nested clone-plan destination templates (e.g. a camera mounted beneath a cloned robot). The old code raised on the first duplicate-template match; the fix collects all matching templates, picks the most specific (shortest-suffix / longest-prefix) owner, and only raises when two distinct, equally-specific templates are tied — a true ambiguity.

• cloner_utils.py: Replaces the early-raise pattern with a two-pass candidate-collection + min_suffix_len selection that mirrors the "nearest owner wins" logic in iter_clone_plan_matches, while preserving the OR-merge coverage check scoped to the winning template's rows.
• test_cloner.py: Adds four new parameterized unit tests covering nested resolution, genuine ambiguity, same-template OR-merge, and partial-coverage rejection.
• changelog.d/: Adds a changelog entry accurately describing the behavioural change.

Confidence Score: 5/5

Safe to merge. The change is narrowly scoped to resolve_clone_plan_source, backward-compatible for non-nested plans, and well-covered by new tests.

The fix replaces a single eager-raise with a two-pass min-suffix-length selection. The new logic is equivalent to iter_clone_plan_matches for non-nested plans and correctly generalises to nested ones. The four new unit tests exercise every branching outcome including the OR-merge coverage path. No regressions were introduced in existing logic paths.

No files require special attention.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/cloner/cloner_utils.py	Core fix: replaces early-return/raise logic in resolve_clone_plan_source with candidate collection + min-suffix-length selection so nested destination templates resolve to the most specific (innermost) owner rather than immediately raising ValueError.
source/isaaclab/test/sim/test_cloner.py	Adds four targeted tests covering: nested templates → most-specific wins, genuinely ambiguous same-level templates → ValueError, same-template OR-merge, and partial coverage → NotImplementedError.
source/isaaclab/changelog.d/fix-cloner_resolve_plan_source.rst	Changelog entry describing the bug fix; accurately reflects the behaviour change.

_{Reviews (1): Last reviewed commit: "Add changelog fragment for clone-plan so..." | Re-trigger Greptile}