From 67c80d5b5404ef2fc7fb1873a8704b120c29736b Mon Sep 17 00:00:00 2001 From: Alex Rodriguez <131964409+ezekiel-alexrod@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:46:00 +0200 Subject: [PATCH] ci: add the Claude PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calls the shared review workflows from `scality/workflows`, pinned by digest: `claude-code-review` on human pull requests, `claude-code-dependency-review` on Renovate branches through `pull_request_target` so the bump job gets the secrets it needs. `ACTIONS_APP_PRIVATE_KEY` is passed explicitly — a reusable workflow inherits no secret, and without that key the shared workflow skips the agent-hub marketplace, the `scality-skills` plugin never loads and the review silently does nothing. The skill reads its criteria from `.claude/REVIEW.md`, added here: what this repo is, the invariants worth failing a review over, and the findings the linters already own. --- .claude/REVIEW.md | 43 ++++++++++++++++++++++++++++++++++++ .github/workflows/review.yml | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 .claude/REVIEW.md create mode 100644 .github/workflows/review.yml diff --git a/.claude/REVIEW.md b/.claude/REVIEW.md new file mode 100644 index 0000000..bf8e3bb --- /dev/null +++ b/.claude/REVIEW.md @@ -0,0 +1,43 @@ +# Review criteria + +Read by the `/review-pr` skill (Scality agent hub) and by anyone reviewing by hand. +Flag problems only — see "What not to flag" at the end. + +## What this repo is + +`disk-management-agent` is a Kubernetes node agent that discovers physical drives +behind hardware RAID controllers — through `github.com/scality/raidmgmt` — and +publishes them as cluster-scoped `DiscoveredPhysicalDisk` custom resources +(`api/v1alpha1`) for platforms and operators to consume as an inventory. It runs +privileged on the node. Errors use `github.com/scality/go-errors`; tests are testify +for use cases and Ginkgo for the controller, plus `test/e2e`. + +## Criteria + +| Area | What to check | +|------|---------------| +| Read-only on hardware | Discovery must never mutate controller or drive state: no volume create/delete, no configuration write, no destructive `raidmgmt` call. A new call into the library must be a read operation — this agent's blast radius is physical storage. | +| Privileged surface | The manager already runs with `privileged: true`; any widening (new host mount, new capability, new device access) must be justified in the PR. Prefer the narrowest access that works. | +| Custom resource identity | `DiscoveredPhysicalDisk` names must be deterministic and stable for the same physical drive across restarts, reboots and enumeration order changes — a name derived from an unstable index causes CR churn and duplicate inventory entries. Check for collisions between nodes. | +| Lifecycle & garbage collection | A drive that disappears leads to its CR being removed or marked accordingly; no orphan CRs, no resurrection of a deleted CR, ownership/finalizers consistent with a cluster-scoped resource written by many nodes. | +| Reconcile idempotency & requeue | Reconciling the same object twice changes nothing; requeue delays (e.g. `cacheNotReadyRequeueDelay`) are bounded and never a hot loop; transient errors are returned rather than swallowed into a bare requeue. | +| Error handling | `github.com/scality/go-errors`: failures wrap a package-level sentinel (`errors.Wrap(ErrX, …, errors.CausedBy(rawErr))`) so `errors.Is` keeps matching the category, rather than `fmt.Errorf`/`%w`. No dropped `err`. | +| raidmgmt usage | Pinned to a tagged release in `go.mod`, never a branch. "Not supported by this implementation" must be handled as an expected outcome, not a fatal error. Parsed vendor output is guarded against missing fields and nil dereferences. | +| Partial hardware failure | One controller failing, absent or returning garbage degrades that entry only — the agent keeps publishing what it could discover instead of crash-looping the DaemonSet. | +| Status conventions | Status via the status subresource only, once per reconcile, standard `metav1.Condition` fields. Status and spec fields consumed downstream are a contract. | +| RBAC scoping | `+kubebuilder:rbac` markers match the verbs the code actually uses; `config/rbac` regenerated and committed. | +| CRD compatibility | `v1alpha1` changes stay additive (no removed field, no newly required field, no narrowed validation) unless the PR explains the migration; validation expressed with kubebuilder markers. | +| Generated code in sync | After editing `api/` types or markers, `zz_generated.deepcopy.go` and `config/crd` are regenerated (`make generate manifests`) and committed in the same PR. | +| Context propagation | The reconcile `ctx` is threaded through every client call and every discovery call; long hardware probes respect cancellation instead of blocking a worker. | +| Tests | Use-case logic covered with testify table-driven tests against fakes of the `raidmgmt` boundary; controller behaviour with Ginkgo/envtest; user-visible behaviour with e2e. A new parsing or naming rule needs a test on the produced CR. | +| Docs | Behaviour, CRD or flag changes update `README.md`; conventions update `CONTRIBUTING.md`. | +| Breaking changes | Call out changes to the CR naming scheme, the CRD schema, published fields, flags and env vars — consumers build inventory on them. | + +## What not to flag + +- Anything the linters already own: `golangci-lint` (`.golangci.yml`), `gofmt`, + `goimports` — formatting, import order, unused variables, naming. +- Generated files (`zz_generated.*`, `config/crd`) except when they are stale with + respect to the sources changed in the same PR. +- Markdown or comment wording preferences. +- Refactors unrelated to the PR's purpose. diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml new file mode 100644 index 0000000..311076e --- /dev/null +++ b/.github/workflows/review.yml @@ -0,0 +1,39 @@ +name: Code Review + +on: + pull_request: + types: [opened, synchronize, labeled, unlabeled] + pull_request_target: + types: [opened, synchronize] + +jobs: + review: + # May also trigger on bump branches when a human updates them (acceptable double-review). + if: github.event_name == 'pull_request' && github.actor != 'scality-renovate[bot]' + uses: scality/workflows/.github/workflows/claude-code-review.yml@7d91dc4ebbe136b54518aa42a1ad6014f8f91526 # v2.9.1 + 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 }} + # The review prompt is a slash command shipped by the scality-skills plugin, + # which the shared workflow clones from the agent-hub marketplace with this + # app key. Without it the plugin is skipped and the review silently does + # nothing. + ACTIONS_APP_PRIVATE_KEY: ${{ secrets.ACTIONS_APP_PRIVATE_KEY }} + + review-dependency-bump: + # pr.user.login catches bump PRs updated by a human, where github.actor is no longer the bot. + if: >- + github.event_name == 'pull_request_target' && + (github.actor == 'scality-renovate[bot]' || + github.event.pull_request.user.login == 'scality-renovate[bot]') + uses: scality/workflows/.github/workflows/claude-code-dependency-review.yml@7d91dc4ebbe136b54518aa42a1ad6014f8f91526 # v2.9.1 + 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 }}