add claude pr review skills - #4946
Conversation
|
Maybe review this as well, how it could be integrated or potential merge conflict? |
- Unstage .agent/ files before committing so skill files never land in a fix commit - Replace hardcoded --base develop with <BASE_BRANCH> placeholder and add instruction to target the same branch checked out in Step 1
Greptile SummaryThis PR introduces a suite of five Claude agent skills ( Key findings from the review:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([GitHub Issue Filed]) --> B[isaaclab-issue-triage]
B --> C{Valid bug report?}
C -->|Not a bug| D[Skip]
C -->|Missing steps| E[Comment requesting steps → STOP]
C -->|Valid| F[isaaclab-bug-reproduce]
F --> G{Reproduces at reported commit?}
G -->|No| H[Comment: cannot reproduce → STOP]
G -->|Yes| I{Still broken on latest develop?}
I -->|No| J[Comment: fixed on latest, close issue → STOP]
I -->|Yes| K[isaaclab-bug-fix]
K --> L[Branch isaaclab-bot/fix-issue-N]
L --> M[Implement fix]
M --> N[Write regression test]
N --> O[Update changelog & version]
O --> P[Run pre-commit]
P --> Q[Commit & push]
Q --> R[Open PR, comment on issue]
S([PR has reviewer comments]) --> T[isaaclab-pr-respond]
T --> U{Comment type?}
U -->|Question| V[Post direct reply]
U -->|Improvement| W[Implement → pre-commit → commit → push → reply with hash]
U -->|New issue| X{Prior fix in recent PRs?}
X -->|Found| Y[Comment pointing to other PR → STOP]
X -->|Not found - in scope| W
X -->|Not found - out of scope| K
Z([PR has merge conflicts]) --> AA[isaaclab-pr-resolve-conflicts]
AA --> AB{Already mergeable?}
AB -->|Yes| AC[Report & stop]
AB -->|No| AD[Rebase onto target]
AD --> AE[Resolve conflicts]
AE --> AF[pre-commit + tests]
AF --> AG[force-push with --force-with-lease]
AG --> AH[Comment on PR]
Last reviewed commit: "add merge conflict r..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ```bash | ||
| git checkout develop # return to develop | ||
| git stash pop # restore any stashed changes (if applicable) | ||
| rm -rf "$AGENT_TMPDIR" # remove temp copy of workflow files |
There was a problem hiding this comment.
git stash pop may fail or corrupt state
git stash in Step 1 outputs "No local changes to save" and creates no stash entry when the working tree is clean. The corresponding git stash pop in Step 7 will then either:
- Error out with
"No stash entries found.", or worse, - Pop a pre-existing, unrelated stash entry that was on the stack before the skill ran — silently restoring files that were not stashed by this workflow.
The fix is to save the stash result and only pop conditionally:
| ```bash | |
| git checkout develop # return to develop | |
| git stash pop # restore any stashed changes (if applicable) | |
| rm -rf "$AGENT_TMPDIR" # remove temp copy of workflow files | |
| git stash list --format='%gd' | grep -q '^stash@{0}' && STASH_CREATED=false || STASH_CREATED=false | |
| git stash && STASH_CREATED=true || STASH_CREATED=false | |
| git checkout <COMMIT_HASH> |
Then in Step 7:
git checkout develop # return to develop
[ "$STASH_CREATED" = true ] && git stash pop # only pop if we actually stashed
rm -rf "$AGENT_TMPDIR"
Or more simply, capture the output:
STASH_OUTPUT=$(git stash)
# ...later...
echo "$STASH_OUTPUT" | grep -q "No local changes" || git stash pop| git reset HEAD -- .agent/ | ||
| git commit -m "$(cat <<'EOF' | ||
| <Short imperative description of the improvement> | ||
|
|
||
| Address reviewer feedback: <one-line summary of what was requested>. | ||
| EOF | ||
| )" | ||
| ``` |
There was a problem hiding this comment.
The heredoc template uses <<'EOF' (not <<-'EOF'), so the indentation of lines inside it is preserved literally in the commit message. When an agent renders this template, the subject line and body will start with leading spaces, producing a malformed commit message.
Compare with the correctly-unindented heredocs in isaaclab-bug-fix/SKILL.md (Step 6) and isaaclab-pr-resolve-conflicts/SKILL.md (Step 4b), which do not indent the heredoc content.
| git reset HEAD -- .agent/ | |
| git commit -m "$(cat <<'EOF' | |
| <Short imperative description of the improvement> | |
| Address reviewer feedback: <one-line summary of what was requested>. | |
| EOF | |
| )" | |
| ``` | |
| git commit -m "$(cat <<'EOF' | |
| <Short imperative description of the improvement> | |
| Address reviewer feedback: <one-line summary of what was requested>. | |
| EOF | |
| )" |
| ```bash | ||
| git push -u origin HEAD | ||
| ``` |
There was a problem hiding this comment.
AGENTS.md (line 269) states: "Network access (e.g., git push) is blocked by the sandbox. Use dangerouslyDisableSandbox: true so the user gets an approval prompt."
This step issues git push -u origin HEAD without any mention of the sandbox override. An agent reading only this skill file — without having read AGENTS.md first — will not know to apply the flag and the push will silently fail.
The same gap exists in:
.agent/skills/isaaclab-pr-respond/SKILL.mdat line 268 (git push origin <HEAD_REF_NAME>) and line 220 (git push -u origin HEAD).agent/skills/isaaclab-pr-resolve-conflicts/SKILL.mdat line 165 (git push --force-with-lease origin <HEAD_REF_NAME>)
Consider adding a note to each git push / gh pr create step, e.g.:
# Network access requires sandbox override — use dangerouslyDisableSandbox: true
git push -u origin HEAD
Search open and recently merged PRs by issue number and keywords before creating a branch. Comment on the issue and stop if an existing PR already addresses it, avoiding duplicate work. Update SKILLS.md overview diagram and skill description to reflect the new Step 0.
There was a problem hiding this comment.
Did you consider forcing the agent to use https://github.com/anthropics/claude-code/tree/main/plugins/pr-review-toolkit
| # Search open PRs by issue number and keywords from the issue title | ||
| gh pr list --repo isaac-sim/IsaacLab --state open --limit 50 \ | ||
| --json number,title,body,headRefName \ | ||
| | jq '.[] | select(.body | test("#<NUMBER>"; "i"))' | ||
|
|
||
| gh search prs --repo isaac-sim/IsaacLab --state open \ | ||
| "<keyword1> <keyword2>" --limit 20 \ | ||
| --json number,title,body,headRefName | ||
|
|
||
| # Search recently merged PRs (last 60 days) | ||
| gh pr list --repo isaac-sim/IsaacLab --state merged --limit 100 \ | ||
| --json number,title,body,mergedAt \ | ||
| | jq '.[] | select(.body | test("#<NUMBER>"; "i"))' | ||
|
|
||
| gh search prs --repo isaac-sim/IsaacLab --state merged \ | ||
| "<keyword1> <keyword2>" --limit 20 \ | ||
| --json number,title,body,mergedAt |
There was a problem hiding this comment.
This forces users to have the github cli. The agent will find a work around, but maybe we could be explicit about it?
| Existing PR found? | ||
| ├─ YES — open PR already addresses this issue | ||
| │ └─ Comment on the issue pointing to the open PR, then STOP: | ||
| │ gh issue comment <NUMBER> --repo isaac-sim/IsaacLab \ | ||
| │ --body "This appears to be addressed in PR #<OTHER> (_<title>_). Tracking there." | ||
| ├─ YES — merged PR already contains the fix | ||
| │ └─ Comment on the issue that the fix is already on develop, then STOP: | ||
| │ gh issue comment <NUMBER> --repo isaac-sim/IsaacLab \ | ||
| │ --body "This was fixed in PR #<OTHER> (merged <DATE>). The fix is available on \`develop\`." | ||
| └─ NO — no existing PR → proceed to Step 1 |
There was a problem hiding this comment.
Should the agent still check if he can repro the issue? on the branch / develop. Sometimes, the fix is not complete
| EOF | ||
| )" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I would add another step here. By running:
- superpower:request-code-review
- https://github.com/anthropics/claude-code/tree/main/plugins/pr-review-toolkit
For self review of the code.
Doing two rounds with clear context could be good.
|
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
IsaacLab Bot Review — PR #4946
This PR adds a solid set of Claude agent skills for automating maintainer workflows (issue triage → bug reproduction → bug fix → PR review response → merge conflict resolution). The skill chaining design is well thought out and the decision trees are clear.
Summary
What works well:
- Clean separation of concerns — each skill is self-contained with clear inputs, decision trees, and exit conditions
- The
.agent/SKILLS.mdoverview with the ASCII decision-tree flowcharts is excellent for discoverability - Commit message and changelog conventions are properly enforced in each skill
- The
_isaac_simsymlink guidance for tracing into Isaac Sim internals is a nice touch git reset HEAD -- .agent/guard in every commit step prevents accidental inclusion of skill files
Key issues to address (details in inline comments):
-
git stashrace condition inisaaclab-bug-reproduce— the unconditionalgit stash popin Step 7 can pop an unrelated stash or error out if no stash was created. greptile flagged this too. Needs conditional pop. -
Sandbox/network note missing from push steps —
AGENTS.mdline 269 says network access requiresdangerouslyDisableSandbox: true, but none of thegit push/gh pr createsteps in the skills reference this. An agent reading only a skill file will hit silent push failures. -
Self-review step missing from
isaaclab-bug-fix— @AntoineRichard and the PR author both noted the need for a self-review cycle (e.g.,superpower:request-code-reviewor the Anthropic PR review toolkit plugin) before pushing. This would catch flaws before human reviewers spend time. -
Post-fix verification missing from
isaaclab-bug-fix— Step 3 says "write a regression test" but the skill doesn't explicitly re-run the original reproduction steps after the fix to confirm the bug is actually resolved end-to-end. The "Important Notes" mention runtime verification but it's not a numbered step in the workflow. -
ghCLI dependency not declared — All skills assumeghis installed and authenticated, but this isn't listed as a prerequisite except inisaaclab-issue-triage. Should be in SKILLS.md or each skill's prerequisites. -
Heredoc indentation in
isaaclab-pr-respond— The commit message heredoc at line ~147 uses<<'EOF'with indented content, which will produce leading spaces in the commit subject. Compare with the correctly unindented heredocs inisaaclab-bug-fixStep 6.
Minor suggestions
- The
isaaclab-bug-reproduceskill copiesAGENTS.md,CLAUDE.md, and.agent/to a temp dir before checkout — consider also preserving.claude/if it exists, for completeness isaaclab-pr-resolve-conflictsStep 4 could benefit from a note about handling submodule conflicts (e.g.,_isaac_simsymlink changes)- Consider adding a
--dry-runor--no-pushmode hint so maintainers can preview what the agent would do without actually pushing
| # Restore workflow files (same as above) | ||
| cp -r "$AGENT_TMPDIR/.agent" .agent 2>/dev/null || true | ||
| cp "$AGENT_TMPDIR/AGENTS.md" AGENTS.md 2>/dev/null || true | ||
| cp "$AGENT_TMPDIR/CLAUDE.md" CLAUDE.md 2>/dev/null || true |
There was a problem hiding this comment.
The unconditional git stash pop in Step 7 is a footgun — if the working tree was clean when git stash ran, no stash entry was created, and git stash pop will either error or pop an unrelated stash.
Simple fix:
STASH_OUT=$(git stash 2>&1)
# ... later in Step 7 ...
echo "$STASH_OUT" | grep -q 'No local changes' || git stash popgreptile flagged the same thing.
|
|
||
| ## Type of change | ||
|
|
||
| - Bug fix (non-breaking change which fixes an issue) |
There was a problem hiding this comment.
This git push (and the gh pr create below it) will silently fail in a sandboxed Claude environment. Per AGENTS.md line 269: "Network access is blocked by the sandbox. Use dangerouslyDisableSandbox: true."
Suggest adding a note here and at every other git push / gh pr create across all skills:
# NOTE: Requires dangerouslyDisableSandbox: true for network access
git push -u origin HEADSame applies to:
isaaclab-pr-respond/SKILL.mdlines 220, 268isaaclab-pr-resolve-conflicts/SKILL.mdline 165
|
|
||
| ```bash | ||
| git add -A | ||
| git reset HEAD -- .agent/ |
There was a problem hiding this comment.
The workflow goes straight from "write regression test" to "update changelog" without an explicit step to re-run the original reproduction steps and confirm the bug is actually fixed end-to-end.
The "Important Notes" section mentions runtime verification, but it should be a first-class workflow step between Step 3 and Step 4:
### Step 3b: Verify the fix
Re-run the original reproduction steps from the issue to confirm the bug no longer occurs:
```bash
./isaaclab.sh -p <original_repro_command>Also run a short smoke test of the affected workflow (e.g., num_steps=10) to catch regressions beyond the specific bug.
| ├─ YES — confirmed overlap → Go to Step C-3 (comment pointing to other PR) | ||
| └─ NO — no relevant prior PR → Go to Step C-4 (implement fix in current PR) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This heredoc uses <<'EOF' with indented content — the leading spaces will appear literally in the commit message subject line:
<Short imperative description of the improvement>
Either unindent the heredoc body (like isaaclab-bug-fix Step 6 does) or use <<-'EOF' with tab indentation.
| @@ -0,0 +1,112 @@ | |||
| # IsaacLab Agent Skills Overview | |||
There was a problem hiding this comment.
Good index file. Consider adding a Prerequisites section at the top listing shared requirements:
ghCLI installed and authenticated (gh auth status)- Git repo cloned with
originremote pointing toisaac-sim/IsaacLab dangerouslyDisableSandbox: truefor any skill that pushes or creates PRs
Right now only isaaclab-issue-triage lists prerequisites — having them centralized avoids repetition and ensures agents always check.
| Determine the base branch: use the branch that was checked out in Step 1 (e.g. `develop`, `main`). The PR must target the same upstream branch the fix branch was created from. | ||
|
|
||
| Create PR using the project template: | ||
|
|
There was a problem hiding this comment.
+1 to @AntoineRichard's suggestion about adding a self-review step. After pre-commit but before committing, the agent should review its own changes with fresh context. This could be:
- A
superpower:request-code-reviewcall - The Anthropic PR review toolkit plugin
- Or simply a structured self-review prompt: "Review this diff for correctness, edge cases, and style violations before committing."
Two rounds of review (implement → review → fix → commit) would catch a lot of issues before human reviewers see the PR.
|
@pascal-roth I see claude superpowers have a "/systematic-debugging" I'm wondering if that could be used as a base for bugfix? |
|
From my experience, I am managing skills like claude memory such that it's single skill with refs to actual action/subskill file, in which way agent has more context of what's available and can selectively load if needed. e.g. /isaaclab pr review, /isaaclab issue xxx. The skill.md file remains a content table to just hold a bunch of links, plus some minimal description. The drawback is that it does not auto complete to exact skill name, but the agent can figure out the action needed based on the instruction provided. |
|
Hi @pascal-roth — thanks for putting this one up! 🙏 We're doing a cleanup pass over the Isaac Lab PR backlog, which had grown past 400 open pull requests, and we're closing out the ones that have gone quiet so the queue is reviewable again. Why this PR is being closed: Here is exactly what we found on this PR when we reviewed the backlog:
It was picked up by the sweep because it has been open for about 6 months. It was then put in the "close" bucket because the author has been silent for about 5 months — which is the signal we used to tell apart pull requests that are still being worked on from ones that have genuinely been set aside. We deliberately did not close pull requests that were approved and ready to land, or that were small and clearly still fixing a live bug — there were 27 of those, and we are merging them rather than closing them. No judgement on the change itself — this is purely backlog hygiene. If this is still wanted, please reopen it or re-submit against 🤖 This comment was drafted with AI assistance as part of a maintainer-led sweep of the Isaac Lab pull request backlog. A maintainer is behind this cleanup — but if this closure looks wrong, it may well be, so please push back and we'll take another look. |
4 participants
Description
Update 19.05:
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there