Skip to content

Commit 28eee11

Browse files
nicolas2bertnicolas2bert
committed
BB-748 add claude code review
1 parent 18f8666 commit 28eee11

File tree

3 files changed

+162
-0
lines changed

3 files changed

+162
-0
lines changed

‎.claude/skills/review-pr/SKILL.md‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
<!-- markdownlint-disable -->
2+
3+
---
4+
name: review-pr
5+
description: Review a PR on backbeat (Node.js async queue and job manager for S3C and Artesca)
6+
argument-hint: <pr-number-or-url>
7+
disable-model-invocation: true
8+
allowed-tools: Read, 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 *)
9+
---
10+
11+
# Review GitHub PR
12+
13+
You are an expert code reviewer. Review this PR: $ARGUMENTS
14+
15+
## Determine PR target
16+
17+
Parse `$ARGUMENTS` to extract the repo and PR number:
18+
19+
- If arguments contain `REPO:` and `PR_NUMBER:` (CI mode), use those values directly.
20+
- If the argument is a GitHub URL (starts with `https://github.com/`), extract `owner/repo` and the PR number from it.
21+
- If the argument is just a number, use the current repo from `gh repo view --json nameWithOwner -q .nameWithOwner`.
22+
23+
## Output mode
24+
25+
- **CI mode** (arguments contain `REPO:` and `PR_NUMBER:`): post inline comments and summary to GitHub.
26+
- **Local mode** (all other cases): output the review as text directly. Do NOT post anything to GitHub.
27+
28+
## Steps
29+
30+
1. **Fetch PR details:**
31+
32+
```bash
33+
gh pr view <number> --repo <owner/repo> --json title,body,headRefOid,author,files
34+
gh pr diff <number> --repo <owner/repo>
35+
```
36+
37+
1. **Read changed files** to understand the full context around each change (not just the diff hunks).
38+
39+
1. **Analyze the changes** against these criteria:
40+
41+
| Area | What to check |
42+
|------|---------------|
43+
| Async error handling | Uncaught promise rejections, missing error callbacks, swallowed errors in streams. Double callbacks in try/catch blocks (callback called in try then again in catch) |
44+
| Async/await usage | New or modified code should use async/await instead of callbacks when possible. When code is migrated from callbacks to async/await, verify: no leftover callback or next params, no mixed callback + promise patterns, proper try/catch around awaited calls, errors are re-thrown or handled (not silently swallowed). Watch for the anti-pattern: `try { cb(); } catch(err) { cb(err); }` where an exception after the first `cb()` triggers a second call |
45+
| Kafka consumer/producer | Correct topic configuration, proper offset commits, consumer group handling, message serialization. Verify `onEntryCommittable` is always reachable. Check circuit breaker thresholds when adding new downstream topics |
46+
| Stream handling | Backpressure, proper cleanup on error, no leaked file descriptors, correct pipe chains |
47+
| Dependency pinning | Git-based deps (arsenal, vaultclient, bucketclient, werelogs, breakbeat, httpagent) must pin to a tag, not a branch |
48+
| Logging | Proper use of werelogs, no `console.log` in production code, log levels match severity. Include enough context (bucket, object key, version, offset) for production troubleshooting |
49+
| Prometheus metrics | New metrics follow existing naming conventions (`s3_backbeat_*`), correct metric types (counter vs gauge vs histogram), bounded label cardinality — avoid per-connector or per-bucket labels that explode with scale |
50+
| Config changes | Backward compatibility, Joi schema updates match new fields, environment variable naming, default values. Env var overrides in `lib/Config.js` must stay consistent with the config file schema |
51+
| MongoDB / Redis resilience | Reconnection handling, proper timeouts on external calls, no indefinite waits. Network errors to MongoDB must not cause stuck tasks or silent data loss |
52+
| Extension architecture | Changes respect the pluggable extension pattern, no cross-extension coupling |
53+
| Security | Command injection, prototype pollution, unsafe deserialization, credential exposure in config/env vars, OWASP-relevant issues for Node.js |
54+
| Breaking changes | Anything that changes public APIs, Kafka message formats, inter-service contracts, or oplog/change stream projections |
55+
56+
1. **Deliver your review:**
57+
58+
### If CI mode: post to GitHub
59+
60+
#### Part A: Inline file comments
61+
62+
For each specific issue, post a comment on the exact file and line:
63+
64+
```bash
65+
gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/comments" -f body="Your comment<br><br>— Claude Code" -f path="path/to/file" -F line=<line_number> -f side="RIGHT" -f commit_id="<headRefOid>"
66+
```
67+
68+
**The command must stay on a single bash line.** Never use newlines in bash commands — use `<br>` for line breaks in comment bodies. Never put `<br>` inside code blocks or suggestion blocks.
69+
70+
Each inline comment must:
71+
72+
- Be short and direct — say what's wrong, why it's wrong, and how to fix it in 1-3 sentences
73+
- No filler, no complex words, no long explanations
74+
- When the fix is a concrete line change (not architectural), include a GitHub suggestion block so the author can apply it in one click:
75+
76+
````text
77+
```suggestion
78+
corrected-line-here
79+
```
80+
````
81+
82+
Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem.
83+
Example with a suggestion block:
84+
85+
```bash
86+
gh api ... -f body=$'Missing the shared-guidelines update command.<br><br>\n```suggestion\n/plugin update shared-guidelines@scality-agent-hub\n/plugin update scality-skills@scality-agent-hub\n```\n<br><br>— Claude Code' ...
87+
```
88+
89+
- When the comment contains a suggestion block, use `$'...'` quoting with `\n` for code fence boundaries. Escape single quotes as `\'` (e.g., `don\'t`)
90+
- End with: `— Claude Code`
91+
92+
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.
93+
94+
#### Part B: Summary comment
95+
96+
```bash
97+
gh pr comment <number> --repo <owner/repo> --body "LGTM<br><br>Review by Claude Code"
98+
```
99+
100+
**The command must stay on a single bash line.** Never use newlines in bash commands — use `<br>` for line breaks in comment bodies.
101+
102+
Do not describe or summarize the PR. For each issue, state the problem on one line, then list one or more suggestions below it:
103+
104+
```text
105+
- <issue>
106+
- <suggestion>
107+
- <suggestion>
108+
```
109+
110+
If no issues: just say "LGTM". End with: `Review by Claude Code`
111+
112+
### If local mode: output the review as text
113+
114+
Do NOT post anything to GitHub. Instead, output the review directly as text.
115+
116+
For each issue found, output:
117+
118+
```text
119+
**<file_path>:<line_number>** — <what's wrong and how to fix it>
120+
```
121+
122+
When the fix is a concrete line change, include a fenced code block showing the suggested replacement.
123+
124+
At the end, output a summary section listing all issues. If no issues: just say "LGTM".
125+
126+
End with: `Review by Claude Code`
127+
128+
## What NOT to do
129+
130+
- Do not comment on markdown formatting preferences
131+
- Do not suggest refactors unrelated to the PR's purpose
132+
- Do not praise code — only flag problems or stay silent
133+
- If no issues are found, post only a summary saying "LGTM"
134+
- Do not flag style issues already covered by the project's linter (eslint with eslint-config-scality)

‎.github/workflows/review.yml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
name: Code Review
2+
3+
on:
4+
pull_request:
5+
types: [opened, synchronize]
6+
7+
jobs:
8+
review:
9+
uses: scality/workflows/.github/workflows/claude-code-review.yml@v2
10+
secrets:
11+
GCP_WORKLOAD_IDENTITY_PROVIDER: ${{ secrets.GCP_WORKLOAD_IDENTITY_PROVIDER }}
12+
GCP_SERVICE_ACCOUNT: ${{ secrets.GCP_SERVICE_ACCOUNT }}
13+
ANTHROPIC_VERTEX_PROJECT_ID: ${{ secrets.ANTHROPIC_VERTEX_PROJECT_ID }}
14+
CLOUD_ML_REGION: ${{ secrets.CLOUD_ML_REGION }}

‎CLAUDE.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<!-- markdownlint-disable MD013 -->
2+
3+
# Backbeat
4+
5+
This is a **Node.js asynchronous queue and job manager** for Scality's S3C and Artesca products. It processes metadata updates and dispatches background tasks via Kafka. It contains:
6+
7+
- Kafka consumers/producers (`lib/BackbeatConsumer.js`, `lib/BackbeatProducer.js`)
8+
- Pluggable extensions for replication, lifecycle, notifications, ingestion, GC (`extensions/`)
9+
- Queue population from MongoDB oplog and Metadata (raft) oplog (`lib/queuePopulator/`)
10+
- Management API and routes (`lib/api/`)
11+
- Configuration management with Joi validation (`lib/Config.js`)
12+
- Git-based internal deps: arsenal, vaultclient, bucketclient, werelogs, breakbeat, httpagent
13+
- CommonJS modules with callback-based async patterns
14+
- Mocha + Sinon test suites (`tests/unit/`, `tests/functional/`, `tests/behavior/`)

0 commit comments

Comments
 (0)