---
name: mariadb-operator-pr-review
description: >
  Perform a structured maintainer-style PR review for the mariadb-operator repository. Gathers PR context,
  triages the change, and evaluates correctness, safety, pitfalls, backwards compatibility and code quality
  against the project conventions documented in AGENTS.md. Use whenever the user mentions a mariadb-operator
  PR number or URL and wants any kind of judgment on it — "review this PR", "what do you think of #1234",
  "is this safe to merge", "assess this diff", "take a look at this change" — even if they don't literally
  say "review".
license: Apache-2.0
metadata:
  author: mariadb-operator
  version: "1.0"
compatibility: Requires GitHub API access (gh CLI or MCP tools) and the mariadb-operator repository checkout.
allowed-tools: Read, Grep, Glob, Bash(git:*), Bash(gh:*)
---

# mariadb-operator PR Review

You are a maintainer of mariadb-operator performing an initial review of a pull request. Evaluate the PR for
correctness, safety, pitfalls, backwards compatibility and code quality, then give an overall assessment.

## AGENTS.md is the source of truth

**Read `AGENTS.md` at the repository root before reviewing.** It is the authoritative description of this
project's architecture, patterns, gotchas and guardrails. This skill deliberately does **not** restate those
rules — it tells you *how to run a review that checks a diff against them*. Throughout, "§ <name>" refers to a
section of AGENTS.md. When a check below says "verify against § X", open that section and compare the diff to it.

## GitHub credentials

Whenever this skill calls GitHub — fetching PR context or delivering a review — pick the access method in this
order, falling through only when the previous one is unavailable:

1. **Project-scoped GitHub MCP tools** (names like `mcp__github-mariadb-operator__*`). These may show up as
   deferred tools — if so, load their schema with `ToolSearch` (e.g.
   `ToolSearch({query: "select:mcp__github-mariadb-operator__get_file_contents", max_results: 1})`) before
   calling them.
2. **`gh` CLI with the project-specific token**, if the `mariadb-operator` MCP server isn't connected. Use
   `GITHUB_MARIADB_OPERATOR_TOKEN` explicitly (`GH_TOKEN="$GITHUB_MARIADB_OPERATOR_TOKEN" gh ...`) rather than
   the ambient `gh auth` session.
3. **Generic GitHub MCP tools** (`mcp__github__*`), if neither of the above is available.
4. **`gh` CLI with default credentials** (plain `gh auth`, no explicit token) as the last resort, if none of the above work.

---

## Step 0 — Gather context

Given a PR URL or number, collect it via the access method chosen in the [GitHub credentials](#github-credentials)
section (the `gh` invocations below assume that method — add the `GH_TOKEN` override or swap in the equivalent
MCP tool as appropriate):

```bash
gh pr view <n> --json title,body,author,state,additions,deletions,files,labels,baseRefName
gh pr diff <n>                       # full unified diff
gh pr view <n> --json reviews,comments   # existing review discussion
```

Then, from the local checkout, pull the surrounding context the diff alone can't show:

- **Pre-change versions** of touched files in `api/v1alpha1/`, `internal/controller/`, `internal/webhook/`,
  `pkg/controller/` — you need the *before* to judge behavioral change:
  ```bash
  git fetch origin <baseRefName>
  git show origin/<baseRefName>:<path/to/file.go>
  ```
- **Blast radius**: `git grep` for callers of any new or changed function signature, and for users of any
  renamed/removed field or type.
- The relevant **`docs/<feature>.md`** (§ Feature Map) — cheaper than reading code to learn intended semantics.

For large PRs, spend attention proportional to risk: read HA/backup/CRD/webhook/builder changes line by line first,
then controllers, then everything else. Diff stats (`gh pr diff <n> --name-only`) tell you where to start.

Don't read generated files to review their contents (§ Token Savers) — for review, only their *presence
or absence* in the diff matters: a change to `api/v1alpha1/` types with no regenerated artifacts is a quick reject.

## Step 1 — Triage

- Classify the PR: feature, bugfix, refactor, docs/examples-only, dependency bump, or generated-files-only.
- Note which subsystems it touches and map them to § Feature Map + the phase list in § Phase-based reconciliation.
- Separate *behavioral* changes from purely *structural* ones (renames, formatting, moves).
- Read the existing review discussion fetched in Step 0: don't re-raise settled points, and where earlier feedback
  asked for a change, check whether later commits actually addressed it.
- Scale the review to the change: a dependabot bump or a docs typo does not need the full five-dimension pass —
  say so and move on. Reserve depth for anything touching reconciliation, HA (`pkg/replication`, `pkg/galera`),
  backup/restore/PITR, CRD schemas, webhooks, or `pkg/builder`.
- Verify facts against the codebase only when a claim is *load-bearing for this change* — i.e. the change's
  correctness or safety depends on it. Do not spot-check environmental invariants (module path, linter versions
  and thresholds, whether a documented list matches the code today) — CI owns those, and confirming them adds
  no signal to the review. For docs/guidance/tooling PRs (`AGENTS.md`, skills, READMEs) the content *is* claims
  about the repo: sample only the few a reader would act on and that would cause real harm if wrong; do not
  exhaustively re-verify every statement.

**Quick rejects** — if any fire, surface it prominently up front:

- CRD types in `api/v1alpha1/` changed, but the regenerated artifacts are missing from the diff
  (§ Codegen and generated files).
- New behavior with no accompanying tests, or none tagged for the PR smoke set (§ Testing).
- RBAC markers added/changed without the manual Helm chart promotion — no CI job catches this
  (§ Gotchas → Chart RBAC is NOT generated).
- StatefulSet Pod template touched without the feature requiring it (§ Safety Guardrails → Rolling restarts).
- Complexity the change doesn't need (§ Simplicity): a new abstraction, config option, spec field, flag or dependency with no concrete use case, or machinery for a corner case that can't realistically occur. Treat this like any other defect — name the simpler alternative.

## Step 2 — Evaluate five dimensions

For each dimension give a verdict of **PASS / CONCERN / FAIL**. **On PASS, state the verdict and stop** — a
one-line reason at most, and even that names the area at a high level, never the individual checks, `file:line`s,
or AGENTS.md rules you verified. Do not enumerate what you checked or restate the diff. Spend words only where
there is something to fix: every CONCERN/FAIL gets specific `file:line` references, the failure scenario, and a
brief restatement of the AGENTS.md rule that was violated (name the § section) so the author sees which convention
the finding is grounded in. A wall of green justifications is noise the author has to read past — the checks below
are what *you* run, not what you report back.

**On PASS, do not explicitly enumerate what you checked.** This holds even when the checks were interesting or non-trivial to run: listing the facts you verified, the files or lines you spot-checked, the claims that held, or the rules that were satisfied is a PASS violation regardless of how it is phrased. Write a single one-line summary naming the area at a high level — the ✅ in the section heading already carries the verdict, so do not repeat `✅ PASS` in the body. The enumeration belongs in your working notes, never in the output.

**Quality bar for findings.** A review's value comes from a few findings the author will act on, not from volume.
Before raising anything, ask: *would a maintainer block or comment on this?* Every CONCERN/FAIL needs (a) a
`file:line` reference, (b) the concrete failure scenario — what input or cluster state makes it go wrong — and
(c) severity honestly stated. If you cannot articulate the failure scenario, it's an observation, not a finding —
either verify it in the code (read callers, check the pre-change version) or drop it. Style opinions that
golangci-lint doesn't enforce are not findings.

Each dimension below names *what to assess* and *which AGENTS.md section(s) hold the rules*. AGENTS.md is the
source of truth — read those sections and check the diff against them. Restate a rule in the output only when a
finding relies on it, and then only briefly (see below); never recap the rules you checked on a PASS.

### A. Correctness
Does the code do what the PR claims, with sound control flow, valid API usage, and correct handling of edge cases
(nil/empty/zero, boundaries, races, pointer/generic/interface use)? Where the diff touches a pattern documented in
§ Architecture and Code Patterns, verify it follows that pattern rather than re-implementing or bypassing it.

### B. Safety
Check the diff against the applicable subsections of § Safety Guardrails, with line-by-line scrutiny of the areas
it flags as dangerous.

### C. Pitfall detection
Confirm the PR steps on none of § Gotchas and Non-obvious Rules, and re-check it against § Kubernetes best
practices. Look for load-bearing assumptions that hold today but may not,
and test scenarios the diff omits (§ Testing).

### D. Backwards compatibility
Check the diff against § Safety Guardrails → Backward compatibility. Classify each finding as **additive** (safe),
**behavioral** (risky), or **breaking** — `v1alpha1` permits change, but breaking changes still need communication
and migration guidance.

### E. Code quality
Is the code well-written, maintainable and consistent with the surrounding codebase? Don't hand-verify what
golangci-lint/CI already checks (§ CI — what a PR must pass); flag only what lint cannot see. Confirm tests are
present and tiered per § Testing, and that the change is appropriately scoped (no dead code, leftover debug, or
over-engineering).

## Step 3 — Verdict

Pick one:

- **Approve** — no substantive issues; correct, safe, well-written.
- **Approve with notes** — correct and safe, but operational considerations worth recording before merge.
- **Request changes** — correctness or safety issues that must be fixed first.
- **Block** — flawed approach, severe safety risk, or a breaking change without migration.

Assign a risk level: **LOW / MEDIUM / HIGH**. Anything touching HA sequencing, backup/restore/PITR, or CRD
compatibility starts at MEDIUM and rises with blast radius.

---

## What not to flag

- Contents of generated files as code-quality issues — they are mechanical outputs. (Their *absence* from a
  CRD-changing PR is still a valid finding.)
- Anything § Gotchas declares intentional.
- Missing godoc on internal functions — not a project convention.
- Dependabot Go-module bumps — trusted automation.
- Preemptive `docs/` or `examples/` updates without matching code.
- Auto-formatted whitespace differences — CI enforces formatting.

## Delivery

**By default, reply the review back to the user in your response — do not post it to GitHub.** Post it (e.g.
`gh pr comment`, using the access method from the [GitHub credentials](#github-credentials) section) only when
the user explicitly asks you to.

## Output format

Render the review as **GitHub-flavored Markdown** — clean enough to drop straight into a PR comment. Use the
emoji legend below so the verdict is scannable at a glance. For any dimension that is **PASS**, write a single
one-line summary naming the area at a high level — the ✅ in the section heading already carries the verdict, so
do not repeat `✅ PASS` in the body. No `file:line` lists, no recap of what passed. Reserve justification,
references and failure scenarios for **CONCERN / FAIL**.

**Emoji legend:**

- Dimension verdict: ✅ PASS · ⚠️ CONCERN · ❌ FAIL
- Overall assessment: ✅ Approve · 📝 Approve with notes · 🔧 Request changes · 🚫 Block
- Risk level: 🟢 LOW · 🟡 MEDIUM · 🔴 HIGH

Use this exact structure:

```markdown
## 🔍 PR Review
<1-2 sentences: what the PR does and why>

### Verdict at a glance

| Dimension | Result |
|---|---|
| Correctness | ✅ PASS |
| Safety | ✅ PASS |
| Pitfall Detection | ⚠️ CONCERN |
| Backwards Compatibility | ✅ PASS |
| Code Quality | ✅ PASS |

**Overall:** 📝 Approve with notes — **Risk:** 🟡 MEDIUM

---

### ✅ Correctness
<verdict; on CONCERN/FAIL add justification with `file:line` references and failure scenario>

### ✅ Safety
<verdict; on CONCERN/FAIL add justification with `file:line` references and failure scenario>

### ⚠️ Pitfall Detection
<verdict; on CONCERN/FAIL add justification with `file:line` references and failure scenario>

### ✅ Backwards Compatibility
<verdict; on CONCERN/FAIL bullets classifying each finding as additive / behavioral / breaking>

### ✅ Code Quality
<verdict; on CONCERN/FAIL add justification with `file:line` references>

---

### 📋 Suggested before merge
<actionable items as a checklist, or "None">
```

Match the emoji in each dimension heading and in the summary table to that dimension's actual verdict, and set
the **Overall** and **Risk** emoji accordingly — the table and the section headings must agree.