docs(skills): aiperf-code-review clones the PR to a temp dir with per-PR artifacts#1112
docs(skills): aiperf-code-review clones the PR to a temp dir with per-PR artifacts#1112ajcasagrande wants to merge 1 commit into
Conversation
ajcasagrande
requested review from
FrankD412,
debermudez,
ganeshku1,
ilana-n,
lkomali and
matthewkotila
as code owners
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@15caf929c00b6a95f3fc01ca41e2899f8b5ccfa6Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@15caf929c00b6a95f3fc01ca41e2899f8b5ccfa6Last updated for commit: |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| - Purpose: review work that exists only in the invoking workspace — uncommitted changes and/or local commits not pushed to any remote. A temp clone cannot see this code, so the clone is skipped. | ||
| - Applies only when the user explicitly asks to review the current workspace (wording like "in place", "my working tree", "my uncommitted/unpushed changes", "don't clone"). Convenience, speed, or "the branch is already checked out" are never triggers — if the code under review is pushed, use the clone. | ||
| - Review the invoking working tree exactly as it stands, including uncommitted changes. Do not switch branches, check out refs, or otherwise mutate the tree. | ||
| - Diff basis in this mode: `git diff origin/main...` (merge-base to the working tree, no second ref) so uncommitted and unpushed work is included. Runtime reproduction also runs from the working tree. |
There was a problem hiding this comment.
git diff origin/main... resolves the omitted side to HEAD, so in-place reviews miss staged and unstaged changes despite claiming to include the working tree. Fix: use git diff "$(git merge-base origin/main HEAD)" for tracked working-tree changes and separately inspect untracked files when present.
🤖 AI Fix
Update .agents/skills/aiperf-code-review/SKILL.md in the In-place mode section to replace the git diff origin/main... instruction with git diff "$(git merge-base origin/main HEAD)" for tracked working-tree changes and add a note to inspect untracked files from git status --short when present.
WalkthroughThe skill instructions now require reviewing pushed PRs from a fresh temp clone with per-PR artifact scoping, while adding an explicit in-place mode for uncommitted or unpushed work. Reproduction output handling and final response requirements were updated to match the two modes. ChangesCode Review Skill Workflow
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/aiperf-code-review/SKILL.md (1)
63-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the final-response checklist mode-aware.
The in-place workflow has no temp clone and usually no GitHub review URL, but the final-response bullets still require both. That forces impossible output for a supported mode. Split the closing instructions by mode, or explicitly say to omit the clone path and review URL when no PR review was posted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/aiperf-code-review/SKILL.md around lines 63 - 77, The final-response checklist in the SKILL document is not mode-aware and still պահանջs a temp clone path and GitHub review URL even for in-place workflow. Update the closing instructions near the “Final response to me” section to branch by mode: keep clone path and review URL only for clone-mode reviews, and explicitly omit them when no PR review was posted in in-place mode. Use the existing “GitHub deliverable” and “Final response to me” sections as the anchor for the edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/aiperf-code-review/SKILL.md:
- Around line 10-13: The fallback branch slug in the PR identifier logic can
contain path separators and is being interpolated into both the mktemp template
and artifacts paths without sanitization. Update the PR-id derivation in
SKILL.md to normalize the branch slug into a filesystem-safe token before using
it in any path-related interpolation, and keep the sanitized value consistently
used wherever `<pr-id>` is referenced.
---
Outside diff comments:
In @.agents/skills/aiperf-code-review/SKILL.md:
- Around line 63-77: The final-response checklist in the SKILL document is not
mode-aware and still պահանջs a temp clone path and GitHub review URL even for
in-place workflow. Update the closing instructions near the “Final response to
me” section to branch by mode: keep clone path and review URL only for
clone-mode reviews, and explicitly omit them when no PR review was posted in
in-place mode. Use the existing “GitHub deliverable” and “Final response to me”
sections as the anchor for the edit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9c30914a-bde5-4625-9edc-f6607a0fccff
📒 Files selected for processing (1)
.agents/skills/aiperf-code-review/SKILL.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…-PR artifacts Review always runs from a fresh temp-directory clone of the PR head — never the user's working tree — diffed against its merge-base with origin/main (three-dot, matching the diff GitHub shows on the PR). The living document and runtime receipts are scoped to artifacts/pr-<number>/ in the invoking repo instead of loose top-level paths. One escape hatch: on explicit user request only, in-place mode reviews uncommitted/unpushed code that a clone cannot see, diffing merge-base to the working tree without mutating it. In that mode the deliverable is the living document; inline PR comments require an explicit ask and all reviewed changes on the PR head, since comments cannot anchor to unpushed lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
ajcasagrande
force-pushed
the
ajc/aiperf-code-review-temp-clone
branch
from
4c88649 to
15caf92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/aiperf-code-review/SKILL.md (1)
71-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the final checklist mode-aware.
The temp-clone path and GitHub review URL only exist in clone mode; in explicit in-place mode, this checklist is impossible to satisfy even though the earlier section says the living document is the deliverable there. Split the final-response requirements by mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/aiperf-code-review/SKILL.md around lines 71 - 77, Make the final checklist mode-aware in the SKILL.md final response guidance: the current “Final response to me” section assumes clone-mode artifacts everywhere, but in explicit in-place mode there is no temp clone path or GitHub review URL. Update the instructions around the final-response requirements to split clone mode vs in-place mode, using the existing mode terminology in SKILL.md so the agent only asks for temp clone path and review URL when those actually exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/aiperf-code-review/SKILL.md:
- Around line 8-10: The setup flow in SKILL.md should not call gh pr view before
a branch-based fallback is available, because that fails on branches without an
open PR and prevents the fallback path from being used. Rework the PR resolution
logic so the branch slug is derived first, then attempt gh pr view only when
needed, and use the existing PR identifier handling around <pr-id> to decide
whether to use pr-<number> or branch-<slug>. Keep the fallback path available
even when gh pr view exits non-zero.
---
Outside diff comments:
In @.agents/skills/aiperf-code-review/SKILL.md:
- Around line 71-77: Make the final checklist mode-aware in the SKILL.md final
response guidance: the current “Final response to me” section assumes clone-mode
artifacts everywhere, but in explicit in-place mode there is no temp clone path
or GitHub review URL. Update the instructions around the final-response
requirements to split clone mode vs in-place mode, using the existing mode
terminology in SKILL.md so the agent only asks for temp clone path and review
URL when those actually exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95f3a37f-2b77-4ffa-8f9b-99ffbe84c8b8
📒 Files selected for processing (1)
.agents/skills/aiperf-code-review/SKILL.md
| Setup — ALWAYS review from a fresh temp-directory clone: | ||
| - Resolve the PR under review: use the PR number the user gave; otherwise run `gh pr view --json number,headRefName,url` on the current branch. | ||
| - Derive the PR identifier `<pr-id>` as `pr-<number>` (e.g. `pr-482`). If no PR exists yet, fall back to `branch-<slug>` where `<slug>` is the branch name with every character outside `[A-Za-z0-9._-]` (including `/`) replaced by `-` (e.g. `ajc/new-feature` -> `branch-ajc-new-feature`). `<pr-id>` is interpolated into filesystem paths and must never contain a path separator. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
branch=$(git branch --show-current)
if gh pr view --json number >/dev/null 2>&1; then
echo "open PR found on $branch"
else
echo "no open PR on $branch; fallback must not depend on gh pr view"
fiRepository: ai-dynamo/aiperf
Length of output: 209
🏁 Script executed:
sed -n '1,40p' .agents/skills/aiperf-code-review/SKILL.mdRepository: ai-dynamo/aiperf
Length of output: 5202
Resolve the branch slug before calling gh pr view. If the branch has no open PR, gh pr view exits non-zero and the setup never reaches the branch-<slug> fallback. Query GitHub only after the branch-based path is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/aiperf-code-review/SKILL.md around lines 8 - 10, The setup
flow in SKILL.md should not call gh pr view before a branch-based fallback is
available, because that fails on branches without an open PR and prevents the
fallback path from being used. Rework the PR resolution logic so the branch slug
is derived first, then attempt gh pr view only when needed, and use the existing
PR identifier handling around <pr-id> to decide whether to use pr-<number> or
branch-<slug>. Keep the fallback path available even when gh pr view exits
non-zero.
1 participant
Summary
Reworks the
aiperf-code-reviewskill so reviews are isolated and artifacts are organized per PR:mktemp -d "${TMPDIR:-/tmp}/...", macOS $TMPDIR-aware), never the invoking working tree — even when the PR branch is already checked out.origin/main(git diff origin/main...HEAD), matching the diff GitHub renders on the PR instead of pulling in unrelated upstream changes.artifacts/pr-<number>/code-review.mdand runtime receipts toartifacts/pr-<number>/repro-runtime-YYYYMMDD/, rooted in the invoking repo so they survive clone cleanup; loose top-level artifacts are forbidden.git diff origin/main...) without mutating it. In that mode the deliverable is the living document, since inline PR comments cannot anchor to unpushed lines.Testing
Wording verified with fresh-context readers across trigger scenarios: default requests always clone (speed/convenience never trigger in-place mode), explicit "in place / don't clone" requests resolve to the working tree with unchanged artifact paths, and the GitHub-deliverable carve-out resolves consistently.
🤖 Generated with Claude Code
Summary by CodeRabbit