Adds skills for Isaac Lab#5769
Conversation
kellyguo11
requested review from
Mayankm96,
hhansen-bdai and
jtigue-bdai
as code owners
github-actions
Bot
added
documentation
Greptile SummaryThis PR adds a repo-owned agent skills framework to Isaac Lab, including a
Confidence Score: 3/5The skills content and validator logic are well-constructed, but the CI workflow never runs the test suite and the README references a skill directory that does not exist — both of which should be resolved before merging. The two functional gaps (tests only compiled, not run; missing build-environments skill directory) mean the CI gate provides less protection than intended and the README misleads contributors about available skills from day one.
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PR touches skills/ or tools/skills/] --> B[skills-check.yml triggered]
B --> C[actions/checkout]
C --> D[setup-python 3.12]
D --> E["python3 tools/skills/cli.py check"]
E --> F{errors?}
F -- Yes --> G[CI fails]
F -- No --> H["py_compile cli.py + test_validate.py"]
H --> I[CI passes]
subgraph "cli.py validate_all()"
E --> J[iter_skills: glob */*/SKILL.md]
J --> K[Check duplicate names]
K --> L[Skill.validate per skill]
L --> M[_validate_metadata]
L --> N[_validate_body: sections, line count]
L --> O[_validate_links: broken links, depth]
L --> P[_validate_backtick_paths]
L --> Q[_validate_reference_files: rglob *.md]
L --> R{audience == user?}
R -- Yes --> S[_validate_user_evaluations]
S --> T[Check evaluations.md exists]
T --> U[Check 3+ scenarios with Query/Expected/Failures]
L --> V[_validate_scripts]
end
style H fill:#f9a,stroke:#c00
style I fill:#f9a,stroke:#c00
Reviews (1): Last reviewed commit: "Adds skills for Isaac Lab" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 | ||
| with: | ||
| python-version: "3.12" | ||
|
|
||
| - name: Verify skills | ||
| run: python3 tools/skills/cli.py check | ||
|
|
||
| - name: Compile skills validator | ||
| run: python3 -m py_compile tools/skills/cli.py tools/skills/test/test_validate.py |
There was a problem hiding this comment.
The workflow compiles test_validate.py with py_compile but never actually runs the tests. Any regression in validator logic — incorrect scenario counting, broken link detection, frontmatter parsing edge cases — will be silently missed in CI. The docs and pyproject.toml both describe how to run pytest tools/skills/, but no pytest step exists here. Adding python3 -m pytest tools/skills/ after the check step would run all nine test cases, including test_validate_current_repo_skills which validates every skill in the repo on each PR.
| - `user/domain-randomization-events/`: implement domain randomization through Isaac Lab event terms. | ||
| - `user/build-environments/`: create direct and manager-based Isaac Lab environments from task requirements. | ||
| - `user/train-rl-agents/`: configure and run Isaac Lab reinforcement learning workflows. | ||
| - `user/use-sensors-actuators/`: add sensors, sensor observations, and actuator models to tasks. |
There was a problem hiding this comment.
user/build-environments/ but the skill directory is never created
The catalog section presents user/build-environments/ as a current user skill alongside the other eight shipped skills, but no skills/user/build-environments/ directory appears in this PR. Only user/import-robot-urdf-mjcf/ is explicitly called out as planned. Anyone following the README to locate this skill will find nothing. Either move this entry to "Planned user skills" or include the directory (with at least a stub SKILL.md) in this PR.
| if "evaluations.md" not in body: | ||
| errors.append(f"{_display_path(self.path)}: user-facing skills must link to evaluations.md from SKILL.md") |
There was a problem hiding this comment.
The current guard matches any occurrence of the literal string "evaluations.md" in the body, including text like "Do not edit evaluations.md" or a code fence. A SKILL.md that references the file only in prose would silently pass. Checking that a proper markdown link exists is more reliable.
| if "evaluations.md" not in body: | |
| errors.append(f"{_display_path(self.path)}: user-facing skills must link to evaluations.md from SKILL.md") | |
| if not re.search(r"(?<!!)\[[^\]]+\]\(evaluations\.md\)", body): | |
| errors.append(f"{_display_path(self.path)}: user-facing skills must link to evaluations.md from SKILL.md") |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Well-structured introduction of a repo-owned agent skills framework with thorough validation tooling. The design philosophy—keeping skills as routing/sequencing guides rather than duplicating docs—is sound. The validator enforces good hygiene (frontmatter, sections, link validity, portable paths, evaluation quality).
A few items to address:
1. 🔴 CI Failure: Missing skills/user/build-environments/ directory
The Skills Check CI is failing because skills/user/train-rl-agents/SKILL.md references `skills/user/build-environments/` in a backtick path, but that directory doesn't exist in this PR:
skills/user/train-rl-agents/SKILL.md: backtick path does not exist: skills/user/build-environments/
Additionally, skills/README.md lists user/build-environments/ in the active catalog section (not the "Planned" section). Either:
- Include the
build-environmentsskill in this PR, or - Move it to the "Planned user skills" section in the README and remove the backtick-path reference from
train-rl-agents/SKILL.md(e.g., mention it as a planned skill name without the path assertion).
2. 🟡 Broken Links CI failure is pre-existing
The "Check for Broken Links" failure is in docs/source/setup/ecosystem.rst (not modified by this PR). This is a pre-existing issue on the base branch and not blocking for this PR specifically.
3. 🟡 Frontmatter parser doesn't handle quoted multi-line or special YAML
The custom YAML frontmatter parser in cli.py uses simple split(":", 1) parsing. This works for the current skills but would silently break on:
- Values containing colons (e.g.,
description: Use when: something happens) - Multi-line YAML strings or block scalars
Currently all existing descriptions avoid colons after "Use when" by natural language, but this is fragile. Consider either:
- Adding a note in the authoring guidelines that descriptions must not contain bare colons beyond the first, or
- Using
yaml.safe_load()(stdlib-free alternative: document the constraint clearly)
Since the current skills all pass validation, this is non-blocking but worth documenting as a known limitation.
4. 🟡 validate_all runs _parse_frontmatter twice per skill
In validate_all(), frontmatter is parsed once to collect names for duplicate detection, then skill.validate() parses it again internally. For 11 skills this is negligible, but a minor refactor (e.g., caching the parse result or extracting it once) would be cleaner as the skill count grows.
5. 💡 Consider pinning the actions/checkout and actions/setup-python by tag + hash comment
The workflow pins actions by full SHA (good security practice), but the comment # v6 / # v5 could drift from the actual pinned SHA over time. Consider using the format:
uses: actions/checkout@de0fac2e... # v6.x.ywith the specific minor version to make future audits easier. Minor nit.
Verdict: The skills check CI failure (#1 above) needs to be resolved before merge. The architecture, validation tooling, skill content quality, and documentation integration are all solid.
Update (1f78c4d): All critical issues from the initial review have been addressed:
- ✅ #1 Fixed —
skills/user/create-environments/now exists with full SKILL.md and evaluations.md; README and cross-references updated. - ✅ #3 Partially addressed — Validator now explicitly rejects block scalars (
|and>) with a clear error, and the docs note the YAML subset constraint. - ✅ Inline: tests not run — CI now installs pytest and runs
python3 -m pytest tools/skills/. - ✅ Inline: weak evaluations.md check — Now uses proper markdown link detection via
LINK_RE. - ✅ Inline: README catalog — Correctly lists
user/create-environments/. - 🧹 Bonus — Removed dead
ThreeDWorldlink from ecosystem.rst (addresses pre-existing broken link issue #2).
No new issues found in the incremental changes. LGTM.
Update (6ff9f18): Validated each skill by spawning agents to follow them on representative user queries. The new revision adds examples.md to all skills that were missing them, and adds reference.md to setup-troubleshooting. Results:
Agent Validation Results
| Skill | Query Tested | Result |
|---|---|---|
train-rl-agents |
"Train Cartpole with RSL-RL on GPU" | ✅ Correct command, config path, pre-flight check produced |
domain-randomization-events |
"Randomize friction for PhysX and Newton" | ✅ Correctly identified backend differences, chose reset mode, recommended PresetCfg |
create-environments |
"Quadruped with custom commands: direct or manager-based?" | ✅ Correctly recommended direct workflow with reasoning |
setup-troubleshooting |
"ModuleNotFoundError: isaaclab_tasks" | 🟡 Correct diagnostic workflow but referenced docs don't cover this specific module |
pr-workflow |
"Preparing a PR after fixing schemas.py" | ✅ Complete checklist produced with correct commands |
Gaps Found During Validation
-
domain-randomization-events: No concretePresetCfgcode example showing how to wire backend-specific event configs. The skill describes what to do ("use PresetCfg") but not the import path or syntax pattern. An agent inferred the pattern correctly but had to guess at imports. -
setup-troubleshooting: The skill correctly routes todocs/source/refs/troubleshooting.rst, but that doc doesn't coverisaaclab_tasksimport failures (onlyisaacsim,isaaclab_physx, etc.). The skill's own philosophy says "update docs first" — this is a docs gap, not a skill gap. -
train-rl-agents: Missing--resume/checkpoint flag syntax for RSL-RL. The workflow says "resume or load a checkpoint only after initial run writes expected artifacts" but doesn't show the command. -
All user skills: None show import paths for the config classes they reference. An agent with access to the codebase can resolve these, but a standalone user following the skill would need to search.
Verdict
The skills framework is well-designed and the updated revision addresses the examples gap from the initial review. All skills produce correct guidance when followed by an agent. The remaining gaps are minor (missing import examples, one docs gap) and don't block merge. The domain-randomization and migration skills are production-quality; the rest meet the bar for an initial seed.
No blocking concerns.
Update (70215f9): All 4 previously flagged gaps have been addressed in this commit:
| Previous Gap | Fix |
|---|---|
| No PresetCfg code example in domain-randomization | ✅ Added complete PresetCfg pattern with imports to examples.md |
isaaclab_tasks missing from troubleshooting docs |
✅ Added to docs/source/refs/troubleshooting.rst AND setup-troubleshooting/reference.md |
| No checkpoint resume in train-rl examples | ✅ Added --resume --load_run RUN_NAME --checkpoint model_100.pt example |
| Import paths missing | ✅ New use-presets skill provides complete import paths in reference.md |
New use-presets Skill Validation
Spawned an agent to follow the new skill on: "Add Newton MJWarp support to a PhysX-only locomotion task."
Result: ✅ PASS — Agent produced complete, correct code:
- Correct
PresetCfgwrapper withdefault,physx, andnewton_mjwarpvariants - Correct import paths (
from isaaclab_tasks.utils import PresetCfg, etc.) - Proper placement in env config (
sim: SimulationCfg = SimulationCfg(physics=PhysicsCfg())) - Correct smoke-test commands with
physics=physxandphysics=newton_mjwarp
The skill's reference.md provides the definition pattern with imports, examples.md shows concrete physics/domain/combined examples, and evaluations.md covers 4 scenarios including "no preset needed."
Final Summary (all 12 skills validated)
All 12 skills (3 developer, 9 user) have been validated by spawning agents. Every skill produces correct, actionable guidance. The pr-workflow skill has minor gaps (test discovery conventions, changelog filename convention) but these are non-blocking for the initial seed. No remaining concerns.
1 participant
Description
Summary
Details
skills/as the canonical home for Isaac Lab agent skills.tools/skills/cli.pyto validate skill frontmatter, required sections, links, portable paths, user evaluations, and scenario quality.docs/source/overview/developer-guide/agent_skills.rstand linked it from the developer guide..github/workflows/skills-check.ymlso skill validation runs only when skills, validator code, or the skills workflow change.AGENTS.mdwith high-level skill ownership, structure, and validation guidance.Validation
python tools/skills/cli.py checkValidated 11 skills.python -m py_compile tools/skills/cli.py tools/skills/test/test_validate.pyType of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there