Create skill for scaffolding a external workflow#635
Conversation
Signed-off-by: Mimi Liao <mimil@nvidia.com>
mimiliaogo
requested review from
alexmillane,
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside,
xyao-nv and
zhx06
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds a Claude skill definition file (.claude/skills/arena-scaffold-external-workflow/SKILL.md) that documents how to scaffold external projects that build on IsaacLab-Arena. The skill is documentation/instructions for an AI assistant, not executable code. It describes creating directory structures, Docker configurations, and git submodule setups for external workflows.
Architecture Impact
Self-contained. This is a skill definition file for Claude Code (an AI coding assistant) that lives in .claude/skills/. It doesn't affect Isaac Lab runtime, imports, or any executable code paths. The scaffolding instructions reference Arena's Docker infrastructure but don't modify it.
Implementation Verdict
Minor fixes needed — documentation inconsistencies and a potential issue with the pyproject.toml template.
Test Coverage
Not applicable for a skill documentation file. However, the skill itself includes a verification step (lines 282-298) that validates the scaffolded project works correctly.
CI Status
No CI checks available. For a documentation-only file in .claude/skills/, this is acceptable, though a markdown lint check would be beneficial.
Findings
🟡 Warning: SKILL.md:11 — Inconsistent command name
The skill is named arena-scaffold-external-workflow in the frontmatter (line 2), but the invocation command is /arena-create-external-workflow. These should match for discoverability. The completion message (line 298) also uses arena-create-external-workflow.
name: arena-scaffold-external-workflow # line 2
/arena-create-external-workflow <project_name> # line 11
🟡 Warning: SKILL.md:95-97 — pyproject.toml include pattern may not work as intended
The pattern "isaaclab_arena_environments*" in [tool.setuptools.packages.find] won't match <project_name>/isaaclab_arena_environments because setuptools finds packages relative to the project root. The nested package should be discovered automatically via "<project_name>*" since it's under that directory. Including it separately suggests it should be a top-level package, which contradicts the directory structure shown in lines 61-67.
include = ["<project_name>*", "isaaclab_arena_environments*"]Either move isaaclab_arena_environments to be a top-level directory, or remove it from the include list since <project_name>* should capture nested packages.
🔵 Improvement: SKILL.md:18-19 — Grammar issue in prerequisites
"Before running this skill, make sure user must have" should be "Before running this skill, the user must have" or "Before running this skill, make sure the user has".
🔵 Improvement: SKILL.md:131 — Dockerfile default value mismatch
The Dockerfile ARG ARENA_IMAGE=isaaclab_arena:latest uses :latest as default, but the run_docker.sh instructions (lines 163-165) explain that DOCKER_VERSION_TAG will be something like cuda_gr00t_gn16. The default value will never be used in practice since it's always overridden, but having :latest as a default is misleading since that tag likely won't exist.
🔵 Improvement: SKILL.md:287-288 — Key path table inconsistency
The table shows "IsaacLab" with container path /workspaces/isaaclab_arena/submodules/IsaacLab and host source "(baked into image)", but earlier documentation (lines 245-248) explains that the submodule must be initialized on the host because it's volume-mounted at runtime. The host source column should reference submodules/IsaacLab-Arena/submodules/IsaacLab/.
🔵 Improvement: SKILL.md:134-143 — Missing WORKDIR instruction in Dockerfile
The Dockerfile template doesn't set WORKDIR /workspaces/<project_name>, which would be helpful for users who exec into the container and expect to land in their project directory. The base image likely sets a different working directory.
mimiliaogo
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new Claude skill to scaffold an external IsaacLab-Arena–based workflow/project, including project structure, packaging stub, and Docker build/run instructions that reuse the Arena base image.
Changes:
- Introduces
/arena-…skill documentation for scaffolding an external workflow project layout. - Specifies a thin project
Dockerfilethat builds fromisaaclab_arena:latestand installs the new package editable. - Describes how to patch Arena’s
run_docker.shto build a two-stage image flow and mount both Arena + project sources.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [tool.setuptools.packages.find] | ||
| include = ["<project_name>*", "isaaclab_arena_environments*"] | ||
| ``` | ||
|
|
||
| The `include` list must cover both your top-level package and `isaaclab_arena_environments` so they are both importable after `pip install -e`. | ||
|
|
There was a problem hiding this comment.
The suggested pyproject.toml package discovery guidance is inconsistent with the directory layout shown in Step 3/Host layout. With isaaclab_arena_environments/ nested under <project_name>/, it will be discovered as <project_name>.isaaclab_arena_environments and already matches <project_name>*; the separate include pattern isaaclab_arena_environments* won’t match anything unless it’s a top-level package. Please either adjust the layout to make isaaclab_arena_environments top-level, or simplify the include list and update the explanation accordingly.
| **Change 3 — Replace the Arena volume mount and add the project mount** (in `DOCKER_RUN_ARGS`): | ||
|
|
||
| Replace: | ||
| ```bash | ||
| "-v" "$SCRIPT_DIR/..:${WORKDIR}" | ||
| ``` | ||
| With: | ||
| ```bash | ||
| "-v" "$ARENA_DIR:${WORKDIR}" # Arena submodule (live source) | ||
| "-v" "$SCRIPT_DIR/..:${PROJECT_WORKDIR}" # Project root (live source) | ||
| ``` |
There was a problem hiding this comment.
Change 3’s “Replace” snippet doesn’t match Arena’s current docker/run_docker.sh in this repo: it mounts the repo with "-v" ".:${WORKDIR}" (not $SCRIPT_DIR/..). As written, someone following these instructions won’t find the line to replace. Update the guidance to reference the actual mount line from Arena’s script (and then show the updated two-mount version).
Greptile SummaryThis PR adds a new Claude Code skill (
Confidence Score: 3/5Not safe to merge as-is — the Dockerfile instructions will produce a broken image that fails to build. Step 4 Change 1 inserts .claude/skills/arena-create-external-workflow/SKILL.md — specifically the Step 4 Dockerfile instructions around Important Files Changed
Reviews (4): Last reviewed commit: "Rework external workflow skill: /opt/are..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /arena-create-external-workflow <project_name> | ||
| ``` | ||
|
|
||
| `<project_name>` is both the Python package name (e.g. `my_robot_tasks`) and the Docker image name. Use `snake_case`. |
There was a problem hiding this comment.
The frontmatter declares name: arena-scaffold-external-workflow, which means Claude Code registers the slash command as /arena-scaffold-external-workflow. The "How to invoke" section however documents /arena-create-external-workflow — a completely different name. Users following the docs will get a "skill not found" error.
| --file $ARENA_DIR/docker/Dockerfile.isaaclab_arena \ | ||
| $ARENA_DIR | ||
| fi | ||
|
|
||
| # ── Stage 2: Project image ──────────────────────────────────────────────────── | ||
| # Note: no --pull here — isaaclab_arena is a local image, not on Docker Hub. |
There was a problem hiding this comment.
The instruction tells Claude to replace "-v" "$SCRIPT_DIR/..:${WORKDIR}", but the actual Arena run_docker.sh line 129 reads "-v" ".:${WORKDIR}" (a bare ., not $SCRIPT_DIR/..). When Claude reads the real file and tries to match this "before" text it will fail to locate the target, producing an incorrect or skipped patch.
The correct "before" text should be:
"-v" ".:${WORKDIR}"
| if [ "$(docker images -q $DOCKER_IMAGE_NAME:$DOCKER_VERSION_TAG 2>/dev/null)" ] && \ | ||
| [ "$FORCE_REBUILD" = false ]; then | ||
| echo "Docker image $DOCKER_IMAGE_NAME:$DOCKER_VERSION_TAG already exists. Not rebuilding." | ||
| else | ||
| docker build $NO_CACHE --progress=plain \ | ||
| --build-arg ARENA_IMAGE="${ARENA_IMAGE_NAME}:${DOCKER_VERSION_TAG}" \ |
There was a problem hiding this comment.
The instruction shows "-v" "$SCRIPT_DIR/../submodules/Isaac-GR00T:..." as the line to replace, but the actual Arena run_docker.sh line 176 uses "-v" "./submodules/Isaac-GR00T:${WORKDIR}/submodules/Isaac-GR00T" (relative path, no $SCRIPT_DIR). The quoted "before" won't match, so Claude will leave the line untouched and the GR00T mount will still reference the wrong path at runtime.
| └── docker/ | ||
| ``` | ||
|
|
||
| **Why this matters:** Arena's `run_docker.sh` mounts the Arena repo at `/workspaces/isaaclab_arena`. Your project mounts alongside it at `/workspaces/<project_name>`. They are siblings, not nested. |
There was a problem hiding this comment.
Can we follow https://isaac-sim.github.io/IsaacLab-Arena/release/0.2.0/pages/arena_in_your_repo/external_installation.html#dockerfile
to layout the container workspace?
Just to be consistent with our doc.
There was a problem hiding this comment.
The layout and Dockerfile mentioned in the doc doesn't work out of box without re-writing the existing Dockerfile. I decided to use two stages of image build since we don't want to maintain two similar Dockerfiles. I'm open to discuss what's a better solution.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
xyao-nv
left a comment
xyao-nv
left a comment
There was a problem hiding this comment.
Much cleaner than the last MR! Thanks for cleaning it up!
One concern regarding the discrepancy from our doc vs what is implemented here, how Arena submodule is copied into the container and installed.
Would like to have this skill consistent with the doc to avoiding confusing users. Or any concern from our doc's dockerfile suggestion? We are also update the doc too.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This is a follow-up review. The new commit addresses the grammar issue from the previous review (line 18-19 now reads "make sure the user has the following in place").
Previous Findings Status
- ✅ Grammar issue (line 18-19) — Addressed. Now reads "make sure the user has the following in place".
- ⏸️ Command name inconsistency — Still present (
arena-scaffold-external-workflowvs/arena-create-external-workflow). - ⏸️ pyproject.toml include pattern — Still present, may cause package discovery issues.
- ⏸️ Other improvements — Dockerfile default tag, path table inconsistency, missing WORKDIR — unchanged.
Implementation Verdict
Minor fixes needed — the grammar fix is good, but the command name inconsistency and pyproject.toml pattern issues remain unaddressed from the previous review.
Redesign skill to bake IsaacLab-Arena into the image at build time (cp -r into submodules/, COPY to /opt/arena) rather than using git submodules and a two-stage image build. Adds entrypoint.sh as a required output, decouples ISAACLAB_PATH from WORKDIR via ARENA_DIR, and updates all three import checks to verify /opt/arena paths. Signed-off-by: Mimi Liao <mimil@nvidia.com>
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds a Claude skill for scaffolding external IsaacLab-Arena workflows. The skill provides comprehensive documentation for creating Docker-based projects that build on Arena. This is a documentation/skill file only—no runtime code changes.
Architecture Impact
Self-contained. This is a Claude skill definition file (.claude/skills/) that provides instructions for Claude to follow when users invoke the command. No impact on Isaac Lab runtime, APIs, or existing functionality.
Implementation Verdict
Minor fixes needed
Test Coverage
N/A — This is a documentation/skill file, not executable code. The skill includes verification steps for users to validate the scaffolded project works correctly.
CI Status
No CI checks available yet.
Findings
🟡 Warning: SKILL.md:1-3 — Skill name inconsistency between filename and frontmatter
The directory is named arena-scaffold-external-workflow but the frontmatter declares name: arena-create-external-workflow, and the invocation command is /arena-create-external-workflow. This inconsistency could cause confusion about the canonical command name. Either rename the directory to arena-create-external-workflow/ or update the frontmatter and invocation to use arena-scaffold-external-workflow.
🟡 Warning: SKILL.md:106 — pyproject.toml package discovery may fail
[tool.setuptools.packages.find]
where = ["."]
include = ["<project_name>*"]The where = ["."] combined with the directory structure places pyproject.toml inside <project_name>/<project_name>/pyproject.toml, but the package is at <project_name>/<project_name>/ (same level). This means where should be [".."] to find the package, or the pyproject.toml should be at the project root (<project_name>/pyproject.toml). The current layout will likely fail to discover the package.
🔵 Improvement: SKILL.md:341 — Typo in completion message
arena-create-external-workflow complete — <project_name> createed.
"createed" should be "created".
🔵 Improvement: SKILL.md:324-327 — Verification paths inconsistent with layout
The verification ls command checks <project_name>/<project_name>/pyproject.toml but Step 2 shows writing to <project_name>/pyproject.toml (the header says <project_name>/pyproject.toml, not <project_name>/<project_name>/pyproject.toml). Clarify the actual intended path.
Redesign the scaffolding skill so that IsaacLab-Arena is baked into the docker image at /opt/arena (never mounted), and only the user's project is bind-mounted at /workspaces/<project_name>. Decouples ISAACLAB_PATH from WORKDIR via a new ARENA_DIR variable. - Replace git submodule + --reference with a plain cp -r from a local Arena clone, avoiding the alternates trap for teammates. - Restructure Dockerfile guidance as deltas from Arena's source rather than duplicating the full file, so the skill stays in sync with Arena. - Add docker/entrypoint.sh step (one-line change from Arena's: use ARENA_DIR instead of WORKDIR for the _isaac_sim symlink). - Simplify run_docker.sh to a single-stage build; drop the Arena and Isaac-GR00T volume mounts since both are now baked. - Drop the isaaclab_arena_environments package conflict by registering external environments via --external_environment_class_path instead of namespace shadowing. - Move pyproject.toml to project root for standard find_packages discovery; exclude submodules/ from the package scan. - Rename skill directory to match the invocation command. Signed-off-by: Mimi Liao <mimil@nvidia.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
All previous findings have been addressed in this update.
Follow-up Assessment
✅ Previous concern about skill name inconsistency — The directory has been renamed from arena-scaffold-external-workflow/ to arena-create-external-workflow/, matching the frontmatter and invocation command.
✅ Previous concern about pyproject.toml package discovery — The document now clearly shows pyproject.toml at the project root (<project_name>/pyproject.toml) in Step 2, and the layout diagram confirms this placement. The where = ["."] is now correct.
✅ Previous typo "createed" — Fixed to "created" on line 343.
✅ Previous verification path inconsistency — The verification ls command (line 324) now correctly references <project_name>/pyproject.toml (not <project_name>/<project_name>/pyproject.toml), consistent with the Step 2 instructions.
CI Status
The "Cleanup artifacts: failure" appears unrelated to this PR's changes (it's a workflow maintenance task, not a code issue).
Implementation Verdict
Ship it
| git init | ||
|
|
||
| # Copy Arena into the project — used only at docker build time, not at runtime | ||
| cp -r ~/IsaacLab-Arena submodules/IsaacLab-Arena |
There was a problem hiding this comment.
pseudo link might be better, right? Single place for src code.
xyao-nv
left a comment
xyao-nv
left a comment
There was a problem hiding this comment.
LGTC. Thank you for patiently iteraley with me over the past few weeks. +1 given.
Passing to my coworker @cvolkcvolk for another +1 before merging.
|
Just curious, @mimiliaogo how do you expect skills to be tested on CI? Any practice your team is doing? |
Uh oh!
There was an error while loading. Please reload this page.