From 45108ee5f7eb192d1a78b812732071f06bed7073 Mon Sep 17 00:00:00 2001 From: Thomas Flament Date: Thu, 18 Jun 2026 20:24:24 +0200 Subject: [PATCH 1/2] ci: adopt shared review skill, move criteria to copilot-instructions Drop the vendored .claude/skills/review-pr/SKILL.md and rely on the shared scality-skills marketplace skill loaded by the reusable review workflow. Move CloudServer's repo-specific review criteria into .github/copilot-instructions.md (read by the marketplace skill), keeping CLAUDE.md as repo context. Matches the vault2 layout. Issue: CLDSRV-930 --- .claude/skills/review-pr/SKILL.md | 188 ------------------------------ .github/copilot-instructions.md | 20 ++++ 2 files changed, 20 insertions(+), 188 deletions(-) delete mode 100644 .claude/skills/review-pr/SKILL.md create mode 100644 .github/copilot-instructions.md diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md deleted file mode 100644 index bac57a847f..0000000000 --- a/.claude/skills/review-pr/SKILL.md +++ /dev/null @@ -1,188 +0,0 @@ ---- -name: review-pr -description: >- - Review a PR on cloudserver (S3-compatible object - storage server in Node.js) -argument-hint: -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: $ARGUMENTS - -## Determine PR target - -Parse `$ARGUMENTS` 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 --repo \ - --json title,body,headRefOid,author,files - gh pr diff --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: - - - **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). - - **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). Watch for the anti-pattern: - `try { cb(); } catch(err) { cb(err); }` where an exception after - the first `cb()` triggers a second call. - - **S3/API contract** — breaking changes to request/response - formats, new error codes, header handling, missing XML response - fields. - - **Dependency pinning** — git-based deps (arsenal, vaultclient, - bucketclient, werelogs, utapi, scubaclient) must pin to a tag, - not a branch. - - **Logging** — proper use of werelogs logger, no `console.log` in - production code, log levels match severity. - - **Stream handling** — backpressure, proper cleanup on error - (`.destroy()`), no leaked file descriptors, missing error event - handlers. - - **Config changes** — backward compatibility with existing env - vars and `config.json`, default values. - - **Security** — command injection, header injection, XML external - entity attacks, path traversal, SSRF in multi-backend requests. - - **Breaking changes** — changes to public S3 API behavior, - metadata schema changes, env var renames without backward compat. - - **Test quality** — no `.only()` tests (eslint enforces this), - assertions match the behavior being tested, `require()`/`import` - at top of file (never inside `describe` or functions). - -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//pulls//comments" \ - -f body="Your comment

— Claude Code" \ - -f path="path/to/file" \ - -F line= \ - -f side="RIGHT" \ - -f commit_id="" -``` - -**The command must stay on a single bash line.** Never use newlines in -bash commands — use `
` for line breaks in comment bodies. Never put -`
` inside code blocks or 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: - - ````text - ```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 update.\ -

\n```suggestion\n\ - /plugin update shared-guidelines@hub\n\ - /plugin update scality-skills@hub\n\ - ```\n

— Claude Code' ... - ``` - -- When the comment contains a suggestion block, use `$'...'` quoting - with `\n` for code fence boundaries. Escape single quotes 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 --repo \ - --body "LGTM

Review by Claude Code" -``` - -**The command must stay on a single bash line.** Never use newlines in -bash commands — use `
` for line breaks in comment bodies. - -Do not describe or summarize the PR. For each issue, state the problem -on one line, then list one or more suggestions below it: - -```text -- - - - - -``` - -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: - -```text -**:** — -``` - -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) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..a11cf001e5 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,20 @@ + + +# Code review instructions + +Repo context lives in [CLAUDE.md](../CLAUDE.md) — read it first. + +When reviewing a PR, analyze the changes against these criteria: + +| Area | What to check | +| --------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 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). | +| 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). Watch for the anti-pattern: `try { cb(); } catch(err) { cb(err); }` where an exception after the first `cb()` triggers a second call. | +| S3/API contract | breaking changes to request/response formats, new error codes, header handling, missing XML response fields. | +| Dependency pinning | git-based deps (arsenal, vaultclient, bucketclient, werelogs, utapi, scubaclient) must pin to a tag, not a branch. | +| Logging | proper use of werelogs logger, no `console.log` in production code, log levels match severity. | +| Stream handling | backpressure, proper cleanup on error (`.destroy()`), no leaked file descriptors, missing error event handlers. | +| Config changes | backward compatibility with existing env vars and `config.json`, default values. | +| Security | command injection, header injection, XML external entity attacks, path traversal, SSRF in multi-backend requests. | +| Breaking changes | changes to public S3 API behavior, metadata schema changes, env var renames without backward compat. | +| Test quality | no `.only()` tests (eslint enforces this), assertions match the behavior being tested, `require()`/`import` at top of file (never inside `describe` or functions). | From c17953d0ce4e773cf61445a3f3518f07680c2fe9 Mon Sep 17 00:00:00 2001 From: Thomas Flament Date: Fri, 19 Jun 2026 11:54:35 +0200 Subject: [PATCH 2/2] ci: modernize review workflow to latest standard Forward secrets via `secrets: inherit`, add a dynamic run-name, and add a workflow_dispatch trigger (with pr_number input) for manual re-runs plus the allowed-tools passthrough. The review job now also runs on workflow_dispatch (guarded off only for pull_request_target and dependabot). Keeps the existing dependency-bump job. Issue: CLDSRV-930 --- .github/workflows/review.yml | 48 +++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index dc8c08d788..7659931083 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -1,29 +1,31 @@ name: Code Review +# yamllint disable rule:line-length +run-name: "Code Review for #${{ github.event.pull_request.number || inputs.pr_number }}${{ github.event.pull_request.title && format(' : {0}', github.event.pull_request.title) }}" +# yamllint enable rule:line-length on: - pull_request: - types: [opened, synchronize, labeled, unlabeled] - pull_request_target: - types: [opened, synchronize] + pull_request: + types: [opened, synchronize, labeled, unlabeled] + pull_request_target: + types: [opened, synchronize] + workflow_dispatch: + inputs: + pr_number: + description: Pull Request number to review + required: true jobs: - review: - if: github.event_name == 'pull_request' && github.actor != 'dependabot[bot]' - 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 }} + review: + if: github.event_name != 'pull_request_target' && github.actor != 'dependabot[bot]' + uses: scality/workflows/.github/workflows/claude-code-review.yml@v2 + with: + allowed-tools: >- + ${{ github.event_name == 'workflow_dispatch' && '"Bash(gh api repos/*/contents)"' || '' }} + secrets: inherit - review-dependency-bump: - if: github.event_name == 'pull_request_target' && github.actor == 'dependabot[bot]' - uses: scality/workflows/.github/workflows/claude-code-dependency-review.yml@v2 - with: - ACTIONS_APP_ID: ${{ vars.ACTIONS_APP_ID }} - 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 }} - ACTIONS_APP_PRIVATE_KEY: ${{ secrets.ACTIONS_APP_PRIVATE_KEY }} + review-dependency-bump: + if: github.event_name == 'pull_request_target' && github.actor == 'dependabot[bot]' + uses: scality/workflows/.github/workflows/claude-code-dependency-review.yml@v2 + with: + ACTIONS_APP_ID: ${{ vars.ACTIONS_APP_ID }} + secrets: inherit