Skip to content

Commit 22e8dbc

Browse files
bert-ebert-e
committed
Merge branch 'improvement/ARSN-561/claude' into tmp/octopus/w/8.3/improvement/ARSN-561/claude
2 parents c61f9db + 4be7ad2 commit 22e8dbc

File tree

3 files changed

+194
-0
lines changed

3 files changed

+194
-0
lines changed

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

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
---
2+
name: review-pr
3+
description: Review a PR on arsenal (Scality's shared S3 utilities library for TypeScript/Node.js)
4+
argument-hint: <pr-number-or-url>
5+
disable-model-invocation: true
6+
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 *)
7+
---
8+
9+
<!-- markdownlint-disable MD013 -->
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+
2. **Read changed files** to understand the full context around each change (not just the diff hunks).
38+
39+
3. **Analyze the changes** against these criteria:
40+
41+
| Area | What to check |
42+
|------|---------------|
43+
| TypeScript correctness | Proper typing, no unnecessary `any`, consistent use of strict mode, correct generics usage |
44+
| Async error handling | Uncaught promise rejections, missing error callbacks, swallowed errors in streams. Double callbacks in try/catch blocks — watch for `try { cb(); } catch(err) { cb(err); }` where an exception after the first `cb()` triggers a second call |
45+
| Async/await migration | 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) |
46+
| Stream handling | Backpressure, proper cleanup on error, no leaked file descriptors |
47+
| Dependency pinning | Git-based deps (werelogs, sproxydclient, 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 |
49+
| Model compatibility | Changes to data models (ObjectMD, BucketInfo, etc.) must preserve backward compatibility with older `mdModelVersion` |
50+
| Error handling | Proper use of ArsenalError, correct `.is` proxy checks, no swallowed errors |
51+
| API contract | Breaking changes to exported interfaces in `index.ts`, renamed or removed public methods |
52+
| Config changes | Backward compatibility, default values, constants changes |
53+
| Security | Injection risks, auth bypass, improper input validation, OWASP-relevant issues |
54+
| Breaking changes | Anything that changes public APIs or interfaces exported by this library |
55+
| Test coverage | New logic should have corresponding tests, existing tests should not be removed without justification |
56+
57+
4. **Deliver your review:**
58+
59+
### If CI mode: post to GitHub
60+
61+
#### Part A: Inline file comments
62+
63+
For each specific issue, post a comment on the exact file and line:
64+
65+
```bash
66+
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>"
67+
```
68+
69+
**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.
70+
71+
Each inline comment must:
72+
73+
- Be short and direct — say what's wrong, why it's wrong, and how to fix it in 1-3 sentences
74+
- No filler, no complex words, no long explanations
75+
- When the fix is a concrete line change (not architectural), include a GitHub suggestion block so the author can apply it in one click:
76+
77+
````text
78+
```suggestion
79+
corrected-line-here
80+
```
81+
````
82+
83+
Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem.
84+
Example with a suggestion block:
85+
86+
```bash
87+
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' ...
88+
```
89+
90+
- When the comment contains a suggestion block, use `$'...'` quoting with `\n` for code fence boundaries. Escape single quotes as `\'` (e.g., `don\'t`)
91+
- End with: `— Claude Code`
92+
93+
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.
94+
95+
#### Part B: Summary comment
96+
97+
```bash
98+
gh pr comment <number> --repo <owner/repo> --body "LGTM<br><br>Review by Claude Code"
99+
```
100+
101+
**The command must stay on a single bash line.** Never use newlines in bash commands — use `<br>` for line breaks in comment bodies.
102+
103+
Do not describe or summarize the PR. For each issue, state the problem on one line, then list one or more suggestions below it:
104+
105+
```
106+
- <issue>
107+
- <suggestion>
108+
- <suggestion>
109+
```
110+
111+
If no issues: just say "LGTM". End with: `Review by Claude Code`
112+
113+
### If local mode: output the review as text
114+
115+
Do NOT post anything to GitHub. Instead, output the review directly as text.
116+
117+
For each issue found, output:
118+
119+
```
120+
**<file_path>:<line_number>** — <what's wrong and how to fix it>
121+
```
122+
123+
When the fix is a concrete line change, include a fenced code block showing the suggested replacement.
124+
125+
At the end, output a summary section listing all issues. If no issues: just say "LGTM".
126+
127+
End with: `Review by Claude Code`
128+
129+
## What NOT to do
130+
131+
- Do not comment on markdown formatting preferences
132+
- Do not suggest refactors unrelated to the PR's purpose
133+
- Do not praise code — only flag problems or stay silent
134+
- If no issues are found, post only a summary saying "LGTM"
135+
- Do not flag style issues already covered by ESLint or the project's linting config

‎.github/workflows/review.yaml‎

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

‎CLAUDE.md‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# arsenal
2+
3+
This is **Scality's shared utilities library** for S3 infrastructure
4+
components. It is a TypeScript/Node.js library providing foundational
5+
modules used across CloudServer, Backbeat, and other S3 platform
6+
services. It contains:
7+
8+
- Algorithms library: listing, caching, streaming, sorting (`lib/algos/`)
9+
- AWS v2/v4 authentication and Vault integration (`lib/auth/`)
10+
- Data models: BucketInfo, ObjectMD, ARN, and others (`lib/models/`)
11+
- S3 middleware: tagging, legal hold, lifecycle, conditional headers (`lib/s3middleware/`)
12+
- Storage backends: metadata (MongoDB, BucketClient, file)
13+
and data (file, AWS, Azure, GCP) (`lib/storage/`)
14+
- IAM policy evaluation (`lib/policyEvaluator/`)
15+
- S3 route definitions (`lib/s3routes/`)
16+
- Object versioning logic (`lib/versioning/`)
17+
- Custom error types with proxy-based `.is` checking (`lib/errors/`)
18+
- Git-based internal deps: werelogs, sproxydclient, httpagent
19+
20+
## Tech stack
21+
22+
- **Language:** TypeScript (compiled to ES2021/CommonJS)
23+
- **Runtime:** Node.js >= 20
24+
- **Build:** tsc + Babel
25+
- **Test:** Jest + ts-jest, Sinon, mongodb-memory-server
26+
- **Lint:** ESLint 9 flat config with @scality/eslint-config-scality
27+
- **CI:** GitHub Actions (lint, unit tests, functional tests, coverage)
28+
29+
## Key commands
30+
31+
```bash
32+
yarn install # Install deps and build (prepare hook)
33+
yarn test # Run Jest unit tests
34+
yarn lint # Run ESLint
35+
yarn build # Compile TypeScript
36+
```
37+
38+
## Conventions
39+
40+
- All imports at the top of the file, never inside functions or blocks
41+
- Strict TypeScript (`strict: true`)
42+
- Test files use `.spec.js` or `.spec.ts` suffix
43+
- Custom `ArsenalError` with `error.is.NoSuchEntity` proxy pattern
44+
- Metadata model version tracking (`mdModelVersion = 7`)

0 commit comments

Comments
 (0)