Add agent intent to arena graph spec conversion

[qianl-nv]

Summary

Add AssetResolver and IntentResolver converting agent output to arena spec

Detailed description

• What was the reason for the change?
  This implements Part 2b of the Agentic Env Gen pipeline, converting agent output (IntentSpec) to UnresolvedArenaEnvGraphSpec
• What has been changed?
  Add match_asset to find loose match of names to exact object from the asset registry
  Add ik tags to embodiments. This allows matching a robot family to its ik variant if multiple variants exist.
  Add IntentCompiler for converting the agent generated intent spec to an arena env spec
• What is the impact of this change?
  This adds a building block in the agentic env gen pipeline. It has no impact on existing functions

To run: python -m isaaclab_arena_examples.agentic_environment_genenration.try_environment_intent_schema

[isaaclab-review-bot]

Code Review: Add resolver for agent intent to arena graph spec conversion

Overall, this is a well-structured implementation with good separation of concerns. The trace-based error reporting pattern (instead of exceptions) is a solid choice for a pipeline where partial resolution is acceptable. The test suite is comprehensive and well-documented. Below are specific findings:

---

1. 🔴 ObjectRelationLibraryRegistry() instantiated per spatial constraint (Performance / Correctness)

File: intent_resolver.py, line ~143 (inside _build_spatial_constraint)

relation_registry = ObjectRelationLibraryRegistry()

This creates a new registry instance for every spatial constraint in the initial state graph. If ObjectRelationLibraryRegistry uses SingletonMeta (like AssetRegistry), this is merely wasteful. If it doesn't, it could be re-parsing/re-loading data on each call.

Suggestion: Instantiate once in __init__ or resolve() and reuse:

def __init__(self, registry: AssetRegistry | None = None) -> None:
    self.registry = registry or AssetRegistry()
    self._relation_registry = ObjectRelationLibraryRegistry()
    ...

---

2. 🟡 resolve_embodiment hardcodes fallback without checking registry (Silent failure risk)

File: asset_resolver.py, lines ~109-111

self.trace.append(TraceEvent("embodiment.miss", name, None, note="falling back to franka_ik"))
return "franka_ik"

If "franka_ik" is not registered in the catalog (e.g., in a deployment with a different robot fleet), the downstream _resolve_embodiment_node will create a node named "franka_ik" that doesn't correspond to any actual asset. The fallback assumption isn't validated.

Suggestion: Either verify "franka_ik" is registered before returning it, or make the fallback configurable:

FALLBACK_EMBODIMENT = "franka_ik"
# ...
if not self.registry.is_registered(FALLBACK_EMBODIMENT):
    self.trace.append(TraceEvent("embodiment.fallback_missing", name, None))
    return name  # or raise

---

3. 🟡 Node ID collision when multiple items resolve to same query string

File: intent_resolver.py, line ~96

id=item.instance_name or item.query,

If two Item objects share the same query (e.g., two "bowl" items for different positions) and neither has an instance_name, both would produce the same node ID. Since nodes is a list (not a dict), duplicates would silently appear, and known_ids (a set) would mask the second.

Suggestion: Add duplicate-ID detection after building nodes, or document that callers must provide instance_name for duplicate queries:

seen_ids: set[str] = set()
node_id = item.instance_name or item.query
if node_id in seen_ids:
    self.trace.append(TraceEvent("item.duplicate_id", node_id, None))
    continue  # or auto-suffix
seen_ids.add(node_id)

---

4. 🟡 _LLM_GENERATED_DIR written unconditionally without mkdir

File: try_environment_intent_schema.py, line ~133

out_path = _LLM_GENERATED_DIR / f"{env_graph_spec.env_name}_proposal.yaml"
env_graph_spec.write_yaml(out_path)

If the llm_generated/ directory doesn't exist, this will raise FileNotFoundError. The path is resolved relative to the source tree (Path(__file__).resolve().parents[2]), so first-time runs in a fresh checkout would fail.

Suggestion:

out_path.parent.mkdir(parents=True, exist_ok=True)
env_graph_spec.write_yaml(out_path)

---

5. 🟢 _pool_for returns empty list on empty intersection — consider trace event

File: asset_resolver.py, lines ~145-150

def _pool_for(self, tags: list[str]) -> list[str]:
    assets = None
    for tag in tags:
        tagged = {a.name for a in self.registry.get_assets_by_tag(tag)}
        assets = tagged if assets is None else assets & tagged
    return sorted(assets or [])

When tags is an empty list, assets remains None and the method returns []. This is handled by callers (e.g., item.tag_pool_empty trace), but calling _pool_for([]) from resolve_name with required_tag=None goes to self.registry.get_all_keys() instead — which is correct but relies on the caller never passing an empty tag list to _pool_for. A defensive check or docstring note would clarify intent.

---

6. 🟢 Test file is excellent — minor gap in resolve_name fuzzy path coverage

File: tests/test_resolver.py

The test suite thoroughly covers item resolution strategies, embodiment fallbacks, spatial constraints, and task wiring. One gap: resolve_name with required_tag=None (the fallback-to-all-keys path) is exercised only implicitly via the background resolver. A direct unit test for this branch would strengthen coverage, e.g.:

def test_resolve_name_fuzzy_without_tag():
    resolver = _make_resolver()
    cls = resolver.asset_resolver.resolve_name("maple_tabl", required_tag=None)
    assert cls is not None and cls.name == "maple_table"

---

Summary: Clean implementation with strong test coverage. The main actionable items are the per-constraint registry instantiation (#1) and the potential for node ID collisions (#3). The other findings are lower-priority hardening improvements.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds AssetResolver and IntentResolver classes for converting agent-generated EnvironmentIntentSpec to ArenaEnvGraphSpec as part of the agentic environment generation pipeline. The implementation is well-structured with comprehensive tracing for debugging and a defensive "never raise" design that logs errors instead of failing. The test coverage is thorough with 530 lines of pure-Python unit tests covering all resolution strategies.

Design Assessment

Design is sound. The resolver architecture appropriately separates concerns:

• AssetResolver handles catalog binding (exact → substring → fuzzy matching)
• IntentResolver orchestrates the full conversion pipeline with proper trace aggregation

The "trace events" pattern for error tracking is a good choice for this LLM-in-the-loop workflow where partial success needs to be communicated back to the agent.

Findings

🟡 Warning: ObjectRelationLibraryRegistry instantiated on every constraint — intent_resolver.py:143

A new ObjectRelationLibraryRegistry() instance is created inside _build_spatial_constraint which is called for each spatial relation. If this registry has initialization overhead (singleton lookup, catalog loading), this could be inefficient for specs with many constraints.

Consider moving the registry instantiation to resolve() or caching it as an instance attribute.

🟡 Warning: Hardcoded fallback embodiment may not exist — asset_resolver.py:117

When embodiment resolution fails completely, the code falls back to "franka_ik" unconditionally. If franka_ik isn't registered in the actual registry (e.g., in a test or reduced deployment), the downstream code will receive a string that doesn't correspond to a real asset.

Consider validating that the fallback exists, or making it configurable.

🔵 Suggestion: Type hint return could be more specific — asset_resolver.py:52

The resolve_item method returns type | None but the actual return is the asset class from the registry. If the registry returns something more specific (e.g., a protocol or base class), consider tightening the return type for better IDE support.

🔵 Suggestion: _derive_env_name could handle edge cases — intent_resolver.py:95

If spec.tasks is empty, first_kind becomes "task", resulting in generic names like llm_gen_table_task. This is handled but might benefit from a comment explaining the design intent for empty-task specs.

Test Coverage

• Coverage adequate: Yes — The 530-line test file comprehensively covers:
  • All resolution strategies (exact, substring, fuzzy, tag-pool relaxation, miss)
  • Embodiment family defaults and fallback behavior
  • Spatial constraint construction (binary/unary relations, skip paths)
  • Task spec wiring and multi-task scenarios
  • Error tracking via has_resolution_errors
  • Edge cases (empty state graph, instance name overrides)
• Test quality: Good — Tests use proper dependency injection via FakeAssetRegistry, isolating the resolver logic from Isaac Sim/Kit dependencies.

CI Status

Pre-commit checks are currently in progress.

Verdict

Minor fixes needed

The implementation is well-designed and thoroughly tested. The warnings above are optimizations/robustness improvements rather than correctness issues. The resolver correctly handles all expected inputs and degrades gracefully on unexpected ones.

Update (commit 5021dd3): Previous concerns addressed. No new issues in the incremental changes.

Status of previous findings:

• ✅ ObjectRelationLibraryRegistry per-constraint instantiation — Fixed. The entire ObjectRelationLibraryRegistry usage was removed from _build_spatial_constraint. Kind validation now happens upstream in SpatialRelationSpec._validate_kind_and_arity, eliminating the per-constraint registry lookup entirely.

• ✅ Hardcoded IK_DEFAULTS dict — Fixed. The static IK_DEFAULTS mapping was replaced with a dynamic tag-based lookup (_pool_for(["embodiment", "ik"])). Embodiment classes now carry tags = ["embodiment", "ik"], making family → IK variant resolution data-driven.

• ⚠️ Hardcoded fallback to "franka_ik" — Still present (not changed in this commit). The original inline comment stands.

Other improvements in this commit:

• Comprehensive docstrings added to all AssetResolver public methods
• Test suite refactored: monolithic test_resolver.py split into test_asset_resolver.py + test_intent_resolver.py with shared fixtures in _resolver_test_helpers.py
• Removed dead code: "in" relation skip logic and "relation.initial.unsupported_kind" error stage (validation moved to model layer)---

Update (commit 5d5bb67): Major feature commit adding UnresolvedArenaEnvGraphSpec.resolve() and TaskTransition system. No new issues found.

New in this commit:

• task_transition.py: Declarative TaskTransition / Relocate / SetState dataclasses describing how task success changes env-graph state
• UnresolvedArenaEnvGraphSpec.resolve(): Chains sequential tasks into state specs (N tasks → N+1 states with is_delta=True deltas)
• ArenaEnvGraphSpecBase extracted as shared base class for ArenaEnvGraphSpec and UnresolvedArenaEnvGraphSpec
• PickAndPlaceTask.success_state_transition() and RotateRevoluteJointTask.success_state_transition() implemented
• TaskBase.success_state_transition() abstract base with NotImplementedError fallback
• ArenaEnvGraphStateSpec.is_delta field added to distinguish full snapshots from differential states
• graph_spec_utils.py refactored: assert_references_exist → assert_constraint_references + assert_task_wiring (cleaner separation)
• Comprehensive new tests: test_task_transitions.py, test_arena_env_graph_spec.py (resolve chain tests), groundtruth init YAML
• try_environment_intent_schema.py now invokes the full resolve pipeline and writes YAML output

Status of previous findings:

• ⚠️ Hardcoded fallback to "franka_ik" — Still present, unchanged. Original inline comment stands.
• ✅ All other previously-noted issues remain addressed.---

Update (commit 415f840): All remaining concerns addressed. Clean incremental improvement.

Status of previous findings:

• ✅ Hardcoded fallback to "franka_ik" — Fixed! resolve_embodiment now returns None on miss instead of silently falling back. The caller (_resolve_embodiment_node) correctly handles None by omitting the node. This is a much cleaner design — the caller decides what to do when resolution fails.

• ✅ Embodiment node ID = resolved name (P1 issue) — Fixed! _resolve_embodiment_node now assigns id=query (original agent-emitted name) consistent with other node types. Tasks referencing the robot by its original name will now resolve correctly against known_ids.

Other improvements in this commit:

• _build_task_specs → _trace_tasks: Tasks now pass through without being needlessly copied — the method only emits trace events for unknown param references. Cleaner separation of concerns.
• _MAX_CANDIDATES module-level constant replaces magic number 10 throughout asset_resolver.py
• _ERROR_TRACE_STAGES expanded to include "embodiment.miss" (now that it is a true terminal failure)
• frozenset → mutable set for _ERROR_TRACE_STAGES (minor style; no functional impact)
• Docstrings reformatted to inline comments (style preference)
• mkdir(parents=True, exist_ok=True) guards output directory in try_environment_intent_schema.py
• Tests updated to validate new node-ID semantics and embodiment-miss behavior

No new issues found. The incremental changes are focused and well-tested.---

Update (commit 29c97fd): Minor test and import maintenance. No new issues.

Changes:

• Import path updated: arena_env_graph_spec → arena_env_graph_types (module rename)
• Test test_has_resolution_errors_false_when_only_relaxation_or_fallback split into two focused tests:
  • test_has_resolution_errors_false_when_only_tag_relaxation — validates tag-pool relaxation is still non-fatal
  • test_has_resolution_errors_true_when_embodiment_unknown — validates unknown embodiment IS a resolution error (consistent with embodiment.miss being in _ERROR_TRACE_STAGES)

Status: ✅ All previous findings remain addressed. Tests correctly reflect the embodiment-miss-as-error semantic from the prior commit.---

Update (b82faad): Clean rename/refactor commit. No new issues.

Changes:

• AssetResolver → AssetMatcher, IntentResolver → IntentCompiler (class + file renames)
• Test helpers renamed accordingly (_intent_compiler_test_helpers.py)
• Embodiment tags = ["embodiment", "ik"] added to DroidDifferentialIK, FrankaIK, G1WBCPink, GR1T2Pink
• ArenaEnvGraphSpecBase extracted; from_yaml/from_dict/write_yaml/to_dict/nodes_by_id moved to base
• graph_spec_utils: assert_references_exist split into assert_constraint_references + assert_task_wiring
• try_environment_intent_schema.py now runs the full compile → resolve → write-YAML pipeline
• Test YAML groundtruth updated to use delta states (is_delta: true)

Status: ✅ All previous findings remain addressed. No new issues introduced.---

Update (0f0e89b): Architecture simplification — AssetMatcher class inlined as module-level functions. No new issues.

Changes:

• asset_matcher.py deleted; TraceEvent + matching logic moved into intent_compiler.py as match_asset() and _fuzzy_match() free functions
• IntentCompiler no longer owns an AssetMatcher instance; _resolve_*_node methods call match_asset() directly and extend self.trace with returned events
• _ASSET_ERROR_STAGES expanded: empty_pool stages now correctly treated as resolution errors for all three categories (background, embodiment, item)
• Test consolidation: test_asset_matcher.py + _intent_compiler_test_helpers.py merged into test_intent_compiler.py with inlined fixtures
• New tests added: test_embodiment_required_tag_pool_empty_records_error, test_item_required_tag_pool_empty_records_error
• Integration test test_get_assets_with_all_tags added to test_asset_registry.py

Assessment: Good simplification. The functional approach (match_asset returning (result, events) tuple) is cleaner than the stateful class approach — trace ownership is now explicit at call sites. The expanded error stages correctly catch empty-pool scenarios that were previously silent omissions.

Status: ✅ All previous findings remain addressed. No new issues introduced.

[greptile-apps]

Greptile Summary

This PR adds AssetRegistry.get_assets_with_all_tags, the match_asset fuzzy-resolution helper, and IntentCompiler — a new compilation stage that converts an LLM-produced EnvironmentIntentSpec into an UnresolvedArenaEnvGraphSpec. It also tags four embodiment classes with "ik" so the compiler can prefer IK variants when the agent specifies a robot family name, and moves the development script to the examples package.

• match_asset resolves free-text robot/object names to registered asset keys via exact, substring, and difflib fuzzy matching, narrowed first by preferred tags then by required tags.
• IntentCompiler.compile wires resolved nodes into graph node specs, validates spatial constraints and task params against known node IDs, and emits structured trace events for every resolution step.
• Tests cover all resolution paths with a fake registry, keeping suite independent of Isaac Sim.

Confidence Score: 4/5

Safe to merge with one fix: G1WBCAgilePinkEmbodiment is missing the ik tag and will resolve incorrectly for agile G1 queries.

G1WBCAgilePinkEmbodiment, which uses PINK IK control, was not tagged ik alongside the other three IK embodiments changed in this PR. Any agent query targeting the agile G1 IK variant will silently fall back to the full embodiment pool and may select the wrong robot. The rest of the PR is logically sound and well covered.

isaaclab_arena/embodiments/g1/g1.py - G1WBCAgilePinkEmbodiment is missing the ik tag

Important Files Changed

Filename	Overview
isaaclab_arena/agentic_environment_generation/asset_matcher.py	New file: 3-stage fuzzy asset resolution (exact to preferred-tag substring/difflib to required-tag fallback) with structured trace events.
isaaclab_arena/agentic_environment_generation/intent_compiler.py	New file: IntentCompiler converts EnvironmentIntentSpec to UnresolvedArenaEnvGraphSpec; node IDs correctly use the agent query string.
isaaclab_arena/embodiments/g1/g1.py	Adds tags=[embodiment,ik] to G1WBCPinkEmbodiment but omits the same tag on G1WBCAgilePinkEmbodiment, which also uses PINK IK control.
isaaclab_arena/assets/registries.py	Adds get_assets_with_all_tags - correct all-tags intersection; empty-list case returns full catalog as documented.
isaaclab_arena/tests/test_intent_compiler.py	Comprehensive unit tests for IntentCompiler covering happy path, error traces, env_name override, and constraint wiring.
isaaclab_arena/tests/test_asset_matcher.py	Unit tests for match_asset covering all resolution stages, including pool relaxation and embodiment IK expansion.
isaaclab_arena_examples/agentic_environment_generation/try_environment_intent_schema.py	Moved dev script now includes IntentCompiler integration and _safe_filename_stem for path sanitization.

_{Reviews (6): Last reviewed commit: "Append asset-matcher trace events in pla..." | Re-trigger Greptile}

[qianl-nv]

self review

[alexmillane]

Looks clean!

A bunch of nits, nothing substantial!

[qianl-nv]

Thanks for the helpful nits Alex! Implemented all your comments except the UnresolvedArenaEnvGraphSpec renamimg, opening a separate MR for it.