Skip to content

Commit f1d9d12

Browse files
bert-ebert-e
committed
Merge branch 'improvement/CLDSRV-862/claude-review' into q/9.2
2 parents 172ab3c + 9d8021a commit f1d9d12

File tree

3 files changed

+213
-1
lines changed

3 files changed

+213
-1
lines changed

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

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
---
2+
name: review-pr
3+
description: >-
4+
Review a PR on cloudserver (S3-compatible object
5+
storage server in Node.js)
6+
argument-hint: <pr-number-or-url>
7+
disable-model-invocation: true
8+
allowed-tools: >-
9+
Bash(gh repo view *), Bash(gh pr view *),
10+
Bash(gh pr diff *), Bash(gh pr comment *),
11+
Bash(gh api *), Bash(git diff *),
12+
Bash(git log *), Bash(git show *)
13+
---
14+
15+
# Review GitHub PR
16+
17+
You are an expert code reviewer. Review this PR: $ARGUMENTS
18+
19+
## Determine PR target
20+
21+
Parse `$ARGUMENTS` to extract the repo and PR number:
22+
23+
- If arguments contain `REPO:` and `PR_NUMBER:` (CI mode), use those
24+
values directly.
25+
- If the argument is a GitHub URL (starts with `https://github.com/`),
26+
extract `owner/repo` and the PR number from it.
27+
- If the argument is just a number, use the current repo from
28+
`gh repo view --json nameWithOwner -q .nameWithOwner`.
29+
30+
## Output mode
31+
32+
- **CI mode** (arguments contain `REPO:` and `PR_NUMBER:`): post inline
33+
comments and summary to GitHub.
34+
- **Local mode** (all other cases): output the review as text directly.
35+
Do NOT post anything to GitHub.
36+
37+
## Steps
38+
39+
1. **Fetch PR details:**
40+
41+
```bash
42+
gh pr view <number> --repo <owner/repo> \
43+
--json title,body,headRefOid,author,files
44+
gh pr diff <number> --repo <owner/repo>
45+
```
46+
47+
2. **Read changed files** to understand the full context around each
48+
change (not just the diff hunks).
49+
50+
3. **Analyze the changes** against these criteria:
51+
52+
- **Async error handling** — uncaught promise rejections, missing
53+
error callbacks, swallowed errors in streams. Double callbacks
54+
in try/catch blocks (callback called in try then again in catch).
55+
- **Async/await migration** — when code is migrated from callbacks
56+
to async/await, verify: no leftover `callback` or `next` params,
57+
no mixed callback + promise patterns, proper try/catch around
58+
awaited calls, errors are re-thrown or handled (not silently
59+
swallowed). Watch for the anti-pattern:
60+
`try { cb(); } catch(err) { cb(err); }` where an exception after
61+
the first `cb()` triggers a second call.
62+
- **S3/API contract** — breaking changes to request/response
63+
formats, new error codes, header handling, missing XML response
64+
fields.
65+
- **Dependency pinning** — git-based deps (arsenal, vaultclient,
66+
bucketclient, werelogs, utapi, scubaclient) must pin to a tag,
67+
not a branch.
68+
- **Logging** — proper use of werelogs logger, no `console.log` in
69+
production code, log levels match severity.
70+
- **Stream handling** — backpressure, proper cleanup on error
71+
(`.destroy()`), no leaked file descriptors, missing error event
72+
handlers.
73+
- **Config changes** — backward compatibility with existing env
74+
vars and `config.json`, default values.
75+
- **Security** — command injection, header injection, XML external
76+
entity attacks, path traversal, SSRF in multi-backend requests.
77+
- **Breaking changes** — changes to public S3 API behavior,
78+
metadata schema changes, env var renames without backward compat.
79+
- **Test quality** — no `.only()` tests (eslint enforces this),
80+
assertions match the behavior being tested, `require()`/`import`
81+
at top of file (never inside `describe` or functions).
82+
83+
4. **Deliver your review:**
84+
85+
### If CI mode: post to GitHub
86+
87+
#### Part A: Inline file comments
88+
89+
For each specific issue, post a comment on the exact file and line:
90+
91+
```bash
92+
gh api -X POST \
93+
-H "Accept: application/vnd.github+json" \
94+
"repos/<owner/repo>/pulls/<number>/comments" \
95+
-f body="Your comment<br><br>— Claude Code" \
96+
-f path="path/to/file" \
97+
-F line=<line_number> \
98+
-f side="RIGHT" \
99+
-f commit_id="<headRefOid>"
100+
```
101+
102+
**The command must stay on a single bash line.** Never use newlines in
103+
bash commands — use `<br>` for line breaks in comment bodies. Never put
104+
`<br>` inside code blocks or suggestion blocks.
105+
106+
Each inline comment must:
107+
108+
- Be short and direct — say what's wrong, why it's wrong, and how to
109+
fix it in 1-3 sentences
110+
- No filler, no complex words, no long explanations
111+
- When the fix is a concrete line change (not architectural), include
112+
a GitHub suggestion block so the author can apply it in one click:
113+
114+
````text
115+
```suggestion
116+
corrected-line-here
117+
```
118+
````
119+
120+
Only suggest when you can show the exact replacement. For
121+
architectural or design issues, just describe the problem.
122+
Example with a suggestion block:
123+
124+
```bash
125+
gh api ... -f body=$'Missing the update.\
126+
<br><br>\n```suggestion\n\
127+
/plugin update shared-guidelines@hub\n\
128+
/plugin update scality-skills@hub\n\
129+
```\n<br><br>— Claude Code' ...
130+
```
131+
132+
- When the comment contains a suggestion block, use `$'...'` quoting
133+
with `\n` for code fence boundaries. Escape single quotes as `\'`
134+
(e.g., `don\'t`)
135+
- End with: `— Claude Code`
136+
137+
Use the line number from the **new version** of the file (the line
138+
number you'd see after the PR is merged), which corresponds to the
139+
`line` parameter in the GitHub API.
140+
141+
#### Part B: Summary comment
142+
143+
```bash
144+
gh pr comment <number> --repo <owner/repo> \
145+
--body "LGTM<br><br>Review by Claude Code"
146+
```
147+
148+
**The command must stay on a single bash line.** Never use newlines in
149+
bash commands — use `<br>` for line breaks in comment bodies.
150+
151+
Do not describe or summarize the PR. For each issue, state the problem
152+
on one line, then list one or more suggestions below it:
153+
154+
```text
155+
- <issue>
156+
- <suggestion>
157+
- <suggestion>
158+
```
159+
160+
If no issues: just say "LGTM". End with: `Review by Claude Code`
161+
162+
### If local mode: output the review as text
163+
164+
Do NOT post anything to GitHub. Instead, output the review directly
165+
as text.
166+
167+
For each issue found, output:
168+
169+
```text
170+
**<file_path>:<line_number>** — <what's wrong and how to fix it>
171+
```
172+
173+
When the fix is a concrete line change, include a fenced code block
174+
showing the suggested replacement.
175+
176+
At the end, output a summary section listing all issues. If no issues:
177+
just say "LGTM".
178+
179+
End with: `Review by Claude Code`
180+
181+
## What NOT to do
182+
183+
- Do not comment on markdown formatting preferences
184+
- Do not suggest refactors unrelated to the PR's purpose
185+
- Do not praise code — only flag problems or stay silent
186+
- If no issues are found, post only a summary saying "LGTM"
187+
- Do not flag style issues already covered by the project's linter
188+
(eslint, biome, pylint, golangci-lint)

‎.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: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
1-
# CLAUDE.md
1+
# cloudserver
2+
3+
This is a **Node.js implementation of the S3 protocol**. It contains:
4+
5+
- S3 API route handlers (`lib/api/`) — 80+ operations
6+
- Authentication and authorization layers (`lib/auth/`)
7+
- Stream-based data path for object storage (`lib/data/`)
8+
- Multi-backend support: file, memory, AWS S3, Azure, GCP, Sproxyd
9+
- Metadata abstraction: LevelDB, MongoDB, bucketd (`lib/metadata/`)
10+
- Git-based internal deps: arsenal, vaultclient, bucketclient, werelogs, utapi, scubaclient
11+
- CommonJS modules, callback-based async (migrating to async/await), Mocha tests
212

313
This file provides guidance to Claude Code (claude.ai/code) when working with
414
code in this repository.

0 commit comments

Comments
 (0)