Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions .claude/skills/review-pr/SKILL.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the SKILL.md file needed for the CI review workflow? Or are we adding it to use locally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is needed for the CI review workflow and also made to be used locally.
The reusable workflow use the skill to review: https://github.com/scality/agent-hub/blob/main/.github/workflows/claude-code-review.yml#L39

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was duplication between the review-pr SKILL.md in this repo, and https://github.com/scality/agent-hub/blob/main/.claude/skills/review-pr/SKILL.md. But actually (if I understand correctly), /setup-review-pr creates a review-pr in each repo, and when Claude is started by the CI review workflow, it reads the review-pr SKILL.md in the repo.

Not sure if/how it would be possible, but a future improvement could be to have a global review-pr SKILL in the agent-hub repo + local instructions in each repo.
That way, we can make improvements to the global review-pr skill and then get them into every repo by bumping the version.

Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
---
name: review-pr
description: Review a PR on s3utils (S3 utility scripts for Scality S3C/Artesca operations)
argument-hint: <pr-number-or-url>
disable-model-invocation: true
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 *)
---

# Review GitHub PR

You are an expert code reviewer. Review this PR:

## Determine PR target

Parse the argument to extract the repo and PR number:

- If arguments contain `REPO:` and `PR_NUMBER:` (CI mode), use those values directly.
- If the argument is a GitHub URL (starts with `https://github.com/`), extract `owner/repo` and the PR number from it.
- If the argument is just a number, use the current repo from `gh repo view --json nameWithOwner -q .nameWithOwner`.

## Output mode

- **CI mode** (arguments contain `REPO:` and `PR_NUMBER:`): post inline comments and summary to GitHub.
- **Local mode** (all other cases): output the review as text directly. Do NOT post anything to GitHub.

## Steps

1. **Fetch PR details:**

```bash
gh pr view <number> --repo <owner/repo> --json title,body,headRefOid,author,files
gh pr diff <number> --repo <owner/repo>
```

2. **Read changed files** to understand the full context around each change (not just the diff hunks).

3. **Analyze the changes** against these criteria:

| Area | What to check |
|------|---------------|
| Async error handling | Uncaught promise rejections, missing error callbacks, swallowed errors in streams, missing `.on('error')` handlers |
| Stream handling | Backpressure issues, proper cleanup on error, no leaked file descriptors, correct use of transform/pipeline |
| Dependency pinning | Git-based deps (`arsenal`, `vaultclient`, `bucketclient`, `werelogs`, `httpagent`) must pin to a tag, not a branch |
| Logging | Proper use of `werelogs` — no `console.log` in production code, log levels match severity |
| 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 |
| Import placement | All `require()` statements must be at the top of the file, never inside functions, blocks, or `describe` scopes |
| Config & env vars | Backward compatibility of environment variables, sensible defaults, documented new variables |
| Production safety | Dry-run support preserved, resumption markers (`KEY_MARKER`, `VERSION_ID_MARKER`) handled correctly, batch limits respected |
| Security | No credentials or secrets in code, safe handling of user-supplied input, OWASP-relevant issues |
| Breaking changes | Changes to script CLI arguments, environment variable contracts, or client interfaces |
| Test coverage | New logic should have corresponding unit tests, mocks should be realistic |

4. **Deliver your review:**

### If CI mode: post to GitHub

#### Part A: Inline file comments

For each specific issue, post a comment on the exact file and line:

```bash
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>"
```

**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.

Each inline comment must:
- Be short and direct — say what's wrong, why it's wrong, and how to fix it in 1-3 sentences
- No filler, no complex words, no long explanations
- When the fix is a concrete line change (not architectural), include a GitHub suggestion block so the author can apply it in one click:
````
```suggestion
corrected-line-here
```
````
Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem.
Example with a suggestion block:
```bash
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' ...
```
- Escape single quotes inside `$'...'` as `\'` (e.g., `don\'t`)
- End with: `— Claude Code`

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.

#### Part B: Summary comment

```bash
gh pr comment <number> --repo <owner/repo> --body $'LGTM\n\nReview by Claude Code'
```

**The command must stay on a single bash line.** Use `$'...'` quoting with `\n` for line breaks.

Do not describe or summarize the PR. For each issue, state the problem on one line, then list one or more suggestions below it:

```
- <issue>
- <suggestion>
- <suggestion>
```

If no issues: just say "LGTM". End with: `Review by Claude Code`

### If local mode: output the review as text

Do NOT post anything to GitHub. Instead, output the review directly as text.

For each issue found, output:

```
**<file_path>:<line_number>** — <what's wrong and how to fix it>
```

When the fix is a concrete line change, include a fenced code block showing the suggested replacement.

At the end, output a summary section listing all issues. If no issues: just say "LGTM".

End with: `Review by Claude Code`

## What NOT to do

- Do not comment on markdown formatting preferences
- Do not suggest refactors unrelated to the PR's purpose
- Do not praise code — only flag problems or stay silent
- If no issues are found, post only a summary saying "LGTM"
- Do not flag style issues already covered by the project's linter (eslint, biome, pylint, golangci-lint)
14 changes: 14 additions & 0 deletions .github/workflows/review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
name: Code Review

on:
pull_request:
types: [opened, synchronize]

jobs:
review:
uses: scality/workflows/.github/workflows/claude-code-review.yml@v2
secrets:
GCP_WORKLOAD_IDENTITY_PROVIDER: ${{ secrets.GCP_WORKLOAD_IDENTITY_PROVIDER }}
GCP_SERVICE_ACCOUNT: ${{ secrets.GCP_SERVICE_ACCOUNT }}
ANTHROPIC_VERTEX_PROJECT_ID: ${{ secrets.ANTHROPIC_VERTEX_PROJECT_ID }}
CLOUD_ML_REGION: ${{ secrets.CLOUD_ML_REGION }}
Loading