|
| 1 | +--- |
| 2 | +name: review-pr |
| 3 | +description: Review a PR on s3utils (S3 utility scripts for Scality S3C/Artesca operations) |
| 4 | +argument-hint: <pr-number-or-url> |
| 5 | +disable-model-invocation: true |
| 6 | +allowed-tools: Bash(gh repo view *), Bash(gh pr view *), Bash(gh pr diff *), Bash(gh pr comment *), Bash(gh api *), Bash(git diff *), Bash(git log *), Bash(git show *) |
| 7 | +--- |
| 8 | + |
| 9 | +# Review GitHub PR |
| 10 | + |
| 11 | +You are an expert code reviewer. Review this PR: |
| 12 | + |
| 13 | +## Determine PR target |
| 14 | + |
| 15 | +Parse the argument to extract the repo and PR number: |
| 16 | + |
| 17 | +- If arguments contain `REPO:` and `PR_NUMBER:` (CI mode), use those values directly. |
| 18 | +- If the argument is a GitHub URL (starts with `https://github.com/`), extract `owner/repo` and the PR number from it. |
| 19 | +- If the argument is just a number, use the current repo from `gh repo view --json nameWithOwner -q .nameWithOwner`. |
| 20 | + |
| 21 | +## Output mode |
| 22 | + |
| 23 | +- **CI mode** (arguments contain `REPO:` and `PR_NUMBER:`): post inline comments and summary to GitHub. |
| 24 | +- **Local mode** (all other cases): output the review as text directly. Do NOT post anything to GitHub. |
| 25 | + |
| 26 | +## Steps |
| 27 | + |
| 28 | +1. **Fetch PR details:** |
| 29 | + |
| 30 | +```bash |
| 31 | +gh pr view <number> --repo <owner/repo> --json title,body,headRefOid,author,files |
| 32 | +gh pr diff <number> --repo <owner/repo> |
| 33 | +``` |
| 34 | + |
| 35 | +2. **Read changed files** to understand the full context around each change (not just the diff hunks). |
| 36 | + |
| 37 | +3. **Analyze the changes** against these criteria: |
| 38 | + |
| 39 | +| Area | What to check | |
| 40 | +|------|---------------| |
| 41 | +| Async error handling | Uncaught promise rejections, missing error callbacks, swallowed errors in streams, missing `.on('error')` handlers | |
| 42 | +| Stream handling | Backpressure issues, proper cleanup on error, no leaked file descriptors, correct use of transform/pipeline | |
| 43 | +| Dependency pinning | Git-based deps (`arsenal`, `vaultclient`, `bucketclient`, `werelogs`, `httpagent`) must pin to a tag, not a branch | |
| 44 | +| Logging | Proper use of `werelogs` — no `console.log` in production code, log levels match severity | |
| 45 | +| Async/await usage | Prefer `async`/`await` over raw promise chains (`.then`/`.catch`) and callbacks for new code; ensure `await` is not missing on async calls | |
| 46 | +| Import placement | All `require()` statements must be at the top of the file, never inside functions, blocks, or `describe` scopes | |
| 47 | +| Config & env vars | Backward compatibility of environment variables, sensible defaults, documented new variables | |
| 48 | +| Production safety | Dry-run support preserved, resumption markers (`KEY_MARKER`, `VERSION_ID_MARKER`) handled correctly, batch limits respected | |
| 49 | +| Security | No credentials or secrets in code, safe handling of user-supplied input, OWASP-relevant issues | |
| 50 | +| Breaking changes | Changes to script CLI arguments, environment variable contracts, or client interfaces | |
| 51 | +| Test coverage | New logic should have corresponding unit tests, mocks should be realistic | |
| 52 | + |
| 53 | +4. **Deliver your review:** |
| 54 | + |
| 55 | +### If CI mode: post to GitHub |
| 56 | + |
| 57 | +#### Part A: Inline file comments |
| 58 | + |
| 59 | +For each specific issue, post a comment on the exact file and line: |
| 60 | + |
| 61 | +```bash |
| 62 | +gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/comments" -f body=$'Your comment\n\n— Claude Code' -f path="path/to/file" -F line=<line_number> -f side="RIGHT" -f commit_id="<headRefOid>" |
| 63 | +``` |
| 64 | + |
| 65 | +**The command must stay on a single bash line.** Use `$'...'` quoting for the `-f body=` value, with `\n` for line breaks. Never use `<br>` — it renders as literal text inside code blocks and suggestion blocks. |
| 66 | + |
| 67 | +Each inline comment must: |
| 68 | +- Be short and direct — say what's wrong, why it's wrong, and how to fix it in 1-3 sentences |
| 69 | +- No filler, no complex words, no long explanations |
| 70 | +- When the fix is a concrete line change (not architectural), include a GitHub suggestion block so the author can apply it in one click: |
| 71 | + ```` |
| 72 | + ```suggestion |
| 73 | + corrected-line-here |
| 74 | + ``` |
| 75 | + ```` |
| 76 | + Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem. |
| 77 | + Example with a suggestion block: |
| 78 | + ```bash |
| 79 | + gh api ... -f body=$'Missing the shared-guidelines update command.\n\n```suggestion\n/plugin update shared-guidelines@scality-agent-hub\n/plugin update scality-skills@scality-agent-hub\n```\n\n— Claude Code' ... |
| 80 | + ``` |
| 81 | +- Escape single quotes inside `$'...'` as `\'` (e.g., `don\'t`) |
| 82 | +- End with: `— Claude Code` |
| 83 | + |
| 84 | +Use the line number from the **new version** of the file (the line number you'd see after the PR is merged), which corresponds to the `line` parameter in the GitHub API. |
| 85 | + |
| 86 | +#### Part B: Summary comment |
| 87 | + |
| 88 | +```bash |
| 89 | +gh pr comment <number> --repo <owner/repo> --body $'LGTM\n\nReview by Claude Code' |
| 90 | +``` |
| 91 | + |
| 92 | +**The command must stay on a single bash line.** Use `$'...'` quoting with `\n` for line breaks. |
| 93 | + |
| 94 | +Do not describe or summarize the PR. For each issue, state the problem on one line, then list one or more suggestions below it: |
| 95 | + |
| 96 | +``` |
| 97 | +- <issue> |
| 98 | + - <suggestion> |
| 99 | + - <suggestion> |
| 100 | +``` |
| 101 | + |
| 102 | +If no issues: just say "LGTM". End with: `Review by Claude Code` |
| 103 | + |
| 104 | +### If local mode: output the review as text |
| 105 | + |
| 106 | +Do NOT post anything to GitHub. Instead, output the review directly as text. |
| 107 | + |
| 108 | +For each issue found, output: |
| 109 | + |
| 110 | +``` |
| 111 | +**<file_path>:<line_number>** — <what's wrong and how to fix it> |
| 112 | +``` |
| 113 | + |
| 114 | +When the fix is a concrete line change, include a fenced code block showing the suggested replacement. |
| 115 | + |
| 116 | +At the end, output a summary section listing all issues. If no issues: just say "LGTM". |
| 117 | + |
| 118 | +End with: `Review by Claude Code` |
| 119 | + |
| 120 | +## What NOT to do |
| 121 | + |
| 122 | +- Do not comment on markdown formatting preferences |
| 123 | +- Do not suggest refactors unrelated to the PR's purpose |
| 124 | +- Do not praise code — only flag problems or stay silent |
| 125 | +- If no issues are found, post only a summary saying "LGTM" |
| 126 | +- Do not flag style issues already covered by the project's linter (eslint, biome, pylint, golangci-lint) |
0 commit comments