From 34649c7cf0e4d6757563867c5e9eda64d4b20392 Mon Sep 17 00:00:00 2001 From: Alex Rodriguez <131964409+ezekiel-alexrod@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:25:02 +0200 Subject: [PATCH 1/2] ci: pass the app key to the review job A reusable workflow inherits no secret: the review job only sees what the caller lists under `secrets:`. Without `ACTIONS_APP_PRIVATE_KEY` the shared workflow skips the agent-hub marketplace, so the `scality-skills` plugin is never loaded and its `/review-pr` skill is unavailable. --- .github/workflows/review.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 46289c2..87b2882 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -16,6 +16,11 @@ jobs: 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. From 15d63d963e25ce4865db9e0663e33d80e8aabe7c Mon Sep 17 00:00:00 2001 From: Alex Rodriguez <131964409+ezekiel-alexrod@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:25:02 +0200 Subject: [PATCH 2/2] ci: replace the forked review skill with review criteria MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `/review-pr` skill of the `scality-skills` plugin reads its criteria from `.claude/REVIEW.md`, so the repo only has to describe what matters here. Keeping a fork of the whole skill freezes its mechanics: this copy predates the check-run summary and the helper scripts, and it still signs comments and posts a summary comment, both of which the current skill forbids. The criteria table moves over unchanged — it was the only repo-specific part of the fork. The steps, the posting commands and the output modes now come from upstream and stay up to date on their own. --- .claude/REVIEW.md | 42 +++++++++++ .claude/skills/review-pr/SKILL.md | 119 ------------------------------ 2 files changed, 42 insertions(+), 119 deletions(-) create mode 100644 .claude/REVIEW.md delete mode 100644 .claude/skills/review-pr/SKILL.md diff --git a/.claude/REVIEW.md b/.claude/REVIEW.md new file mode 100644 index 0000000..c11df6a --- /dev/null +++ b/.claude/REVIEW.md @@ -0,0 +1,42 @@ +# 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 + +`node-warden-operator` is a cluster-scoped Kubernetes operator that watches Node +conditions and remediates the affected nodes, driven by a generic +`NodeRemediationPolicy` custom resource. The decision logic is a pure functional +core in `internal/remediation` (`Decide(facts, now) -> Plan`); the reconciler only +gathers facts and applies the plan. Architecture: `DESIGN.md`. + +## Criteria + +| Area | What to check | +|------|---------------| +| Functional core purity | The decision logic in `internal/remediation` (`Decide(facts, now) -> Plan`) must stay I/O-free: no client/API calls, no clock reads (time enters via `now`), no logging or other side effects. Decisions belong here, not inlined into `Reconcile`. | +| Reconcile idempotency | `Reconcile` must be safe to run repeatedly for the same object, hold no state across calls, and converge to the desired state regardless of the starting point. | +| Taint remediation safety | Taint apply/remove is read-modify-write with conflict retry (not Server-Side Apply); touches only the policy's own taint key; stays `NoExecute` and reversible; must not evict kubelet-managed static pods (apiserver, etcd, scheduler, controller-manager). | +| Debounce & guard | Debounce derived from the condition's `lastTransitionTime`; `guard.maxAffectedFraction` respected and within `[0,1]`; `Unknown`/stale conditions handled explicitly, never treated as healthy. | +| Watch predicates | Predicates must filter noise (kubelet heartbeats, lease/heartbeat-only condition churn, the controller's own status writes) to avoid self-trigger loops and needless reconciles. | +| RBAC scoping | `+kubebuilder:rbac` markers grant least privilege and match what the code actually reads/writes; `config/rbac` regenerated after changes. | +| Status subresource | Status written via the status subresource, once per reconcile, using standard `metav1.Condition` conventions (type/status/reason/lastTransitionTime). | +| CRD / API compatibility | `v1alpha1` changes stay backward compatible where possible; invariants enforced by CEL validation markers (e.g. `taint.effect` restricted to `NoExecute`, fraction in `[0,1]`, at least one remediation set) rather than only in Go. | +| Generated code in sync | After editing `api/` types or kubebuilder markers, `zz_generated.deepcopy.go` and `config/crd` must be regenerated (`make generate manifests`) and committed in the same PR. | +| Error wrapping | Wrap with `fmt.Errorf("...: %w", err)` (not `%v`); don't swallow errors; return them so controller-runtime can requeue. | +| Context propagation | Thread the `ctx` from `Reconcile` through every client call; respect cancellation; don't spawn detached background contexts. | +| Logging | Use the `logr` logger from `logf.FromContext(ctx)` with structured key/values (logcheck enforces the k8s logging conventions); no `fmt.Print*` or stdlib `log`. | +| Concurrency | Any goroutines have clear exit conditions and no leaks; shared state is guarded. | +| Docs sync | Behavior / CRD / flags / output -> `README.md`; architecture or a design decision -> `DESIGN.md`; conventions or workflow -> `CONTRIBUTING.md`. Flag docs left stale by the change. | +| Security | No secrets, tokens, or keys in code or samples; HTTP/2 stays disabled unless intentionally enabled; the metrics endpoint stays behind authn/authz. | +| Breaking changes | Anything that changes the CRD schema, public Go APIs, flags, or the manager's behavior in a non-additive way. | + +## What not to flag + +- Anything the linters already own: `golangci-lint` (errcheck, gocyclo, revive, + staticcheck, logcheck, depguard, misspell…), `gofmt`, `goimports`. +- 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/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md deleted file mode 100644 index 63cac5a..0000000 --- a/.claude/skills/review-pr/SKILL.md +++ /dev/null @@ -1,119 +0,0 @@ ---- -name: review-pr -description: Review a PR on node-warden-operator (a cluster-scoped Kubernetes operator that remediates unhealthy node conditions via a NodeRemediationPolicy CRD) -argument-hint: -disable-model-invocation: true -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 *) ---- - -# 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: - -| Area | What to check | -|------|---------------| -| Functional core purity | The decision logic in `internal/remediation` (`Decide(facts, now) -> Plan`) must stay I/O-free: no client/API calls, no clock reads (time enters via `now`), no logging or other side effects. Decisions belong here, not inlined into `Reconcile`. | -| Reconcile idempotency | `Reconcile` must be safe to run repeatedly for the same object, hold no state across calls, and converge to the desired state regardless of the starting point. | -| Taint remediation safety | Taint apply/remove is read-modify-write with conflict retry (not Server-Side Apply); touches only the policy's own taint key; stays `NoExecute` and reversible; must not evict kubelet-managed static pods (apiserver, etcd, scheduler, controller-manager). | -| Debounce & guard | Debounce derived from the condition's `lastTransitionTime`; `guard.maxAffectedFraction` respected and within `[0,1]`; `Unknown`/stale conditions handled explicitly, never treated as healthy. | -| Watch predicates | Predicates must filter noise (kubelet heartbeats, lease/heartbeat-only condition churn, the controller's own status writes) to avoid self-trigger loops and needless reconciles. | -| RBAC scoping | `+kubebuilder:rbac` markers grant least privilege and match what the code actually reads/writes; `config/rbac` regenerated after changes. | -| Status subresource | Status written via the status subresource, once per reconcile, using standard `metav1.Condition` conventions (type/status/reason/lastTransitionTime). | -| CRD / API compatibility | `v1alpha1` changes stay backward compatible where possible; invariants enforced by CEL validation markers (e.g. `taint.effect` restricted to `NoExecute`, fraction in `[0,1]`, at least one remediation set) rather than only in Go. | -| Generated code in sync | After editing `api/` types or kubebuilder markers, `zz_generated.deepcopy.go` and `config/crd` must be regenerated (`make generate manifests`) and committed in the same PR. | -| Error wrapping | Wrap with `fmt.Errorf("...: %w", err)` (not `%v`); don't swallow errors; return them so controller-runtime can requeue. | -| Context propagation | Thread the `ctx` from `Reconcile` through every client call; respect cancellation; don't spawn detached background contexts. | -| Logging | Use the `logr` logger from `logf.FromContext(ctx)` with structured key/values (logcheck enforces the k8s logging conventions); no `fmt.Print*` or stdlib `log`. | -| Concurrency | Any goroutines have clear exit conditions and no leaks; shared state is guarded. | -| Docs sync | Behavior / CRD / flags / output -> `README.md`; architecture or a design decision -> `DESIGN.md`; conventions or workflow -> `CONTRIBUTING.md`. Flag docs left stale by the change. | -| Security | No secrets, tokens, or keys in code or samples; HTTP/2 stays disabled unless intentionally enabled; the metrics endpoint stays behind authn/authz. | -| Breaking changes | Anything that changes the CRD schema, public Go APIs, flags, or the manager's behavior in a non-additive way. | - -4. **Deliver your review:** - -### If CI mode: post to GitHub - -#### Part A: Inline file comments - -For each issue, post a comment on the exact file and line. Keep comments short (1-3 sentences), end with `— Claude Code`. Use line numbers from the **new version** of the file. - -**Without suggestion block** — single-line command, `
` for line breaks: -```bash -gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="Issue description.

— Claude Code" -f path="file" -F line=42 -f side="RIGHT" -f commit_id="" -``` - -**With suggestion block** — use a heredoc (`-F body=@-`) so code renders correctly: -```bash -gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -F body=@- -f path="file" -F line=42 -f side="RIGHT" -f commit_id="" <<'COMMENT_BODY' -Issue description. - -```suggestion -first line of suggested code -second line of suggested code -``` - -— Claude Code -COMMENT_BODY -``` - -Only suggest when you can show the exact replacement. For architectural or design issues, just describe the problem. - -#### Part B: Summary comment - -Single-line command, `
` for line breaks. No markdown headings — they render as giant bold text. Flat bullet list only: - -```bash -gh pr comment --repo --body "- file:line — issue
- file:line — issue

Review by Claude Code" -``` - -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: - -``` -**:** — -``` - -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 (golangci-lint: errcheck, gocyclo, revive, staticcheck, logcheck, depguard, misspell, ...)