--- name: mariadb-operator/mariadb-operator-pr-review source: https://app.decimal.ai/s/mariadb-operator-mariadb-operator-pr-review@1/SKILL.md source_sha256: b3dd353c0473 --- # 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, "§ " 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 --json title,body,author,state,additions,deletions,files,labels,baseRefName gh pr diff # full unified diff gh pr view --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 git show origin/: ``` - **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/.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 --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 ### ✅ Safety ### ⚠️ Pitfall Detection ### ✅ Backwards Compatibility ### ✅ Code Quality --- ### 📋 Suggested before merge ``` 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.