# Mz Pr Review

> Local code review of current branch vs Materialize standards. Trigger: "review my code", "review my changes", "check my diff", "does this look ok", "what do you think of this PR", "code review", or look over changes before merging. Also PR number + wants feedback on quality, style, correctness.

- Skill: `materializeinc/mz-pr-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add materializeinc/mz-pr-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/materializeinc/mz-pr-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: materializeinc (https://skillmd.com/u/materializeinc)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/materializeinc/mz-pr-review

---


Perform a local code review of the current branch's changes against Materialize project standards.

## Steps

1. Parse arguments from: $ARGUMENTS — a PR number, base branch, or nothing.
2. Get the diff using the first method that works:
   - **PR number given** (e.g. `123`): `gh pr diff 123`
   - **git available**: `git diff <base>...HEAD` (default base: `main`)
   - **jj available**: `jj diff -r <revset>` (default: diff from trunk)
3. Get the file list from the same diff (add `--stat` for git, `--stat` for jj, or `gh pr diff 123 --stat` for PR).
4. Review the diff against the checklists below.
5. Present findings organized as: **Blocking**, **Strong suggestions**, **Nits**.

## Review checklist

The overall developer guide for reviewing changes is defined in `doc/developer/guide-changes.md`, always read and follow its guidance.

### Tests
- Every behavior change has at least one new or modified test.
- SQL/query behavior → look for `.slt` in `test/sqllogictest/`.
- Wire/protocol behavior → look for `.pt` in `test/pgtest/`.
- Rust logic/types/APIs → add or extend a Rust unit test in the crate (e.g. `#[cfg(test)]` or `tests/`); run with `cargo test -p mz-<crate>`.
- Prefer testing observable behavior (SQL results, wire protocol) over implementation details.
- Red flag: behavior change with no test changes.
- For more testing guidelines, read `doc/developer/guide-testing.md`

### Code style (Rust)
- **Imports:** `std` → external crates → `crate::`; one `use` per module; prefer `crate::` over `super::` in non-test code.
- **Errors:** Structured with `thiserror`; no bare `anyhow!("...")`. `Display` should not print full error chain.
- **Async:** Use `ore::task::spawn` / `spawn_blocking`, not raw `tokio::spawn`.
- **Tests:** `#[mz_ore::test]`; panic in tests rather than returning `Result`.

### Code style (SQL)
- Keywords capitalized (`SELECT`, `FROM`); identifiers lowercase.
- No space between function name and `(`.

### Error messages
- Primary: short, factual, lowercase first letter, no trailing punctuation.
- Detail/hint: complete sentences, capitalized, period.
- No "unable", "bad", "illegal", "unknown"; say what kind of object.

### Sensitive data handling
- Types holding passwords, keys, tokens, or credentials should use `mz_ore::secure::{SecureString, SecureVec}` or `zeroize::Zeroizing<T>` (from `mz_ore::secure`).
- Sensitive types should **not** derive `Clone` or `Debug` (use custom `Debug` that redacts).
- Stack-local buffers holding derived keys, nonces, or HMAC outputs should be wrapped in `Zeroizing<T>`.
- See `doc/developer/generated/ore/secure.md` for full guidance and `src/ssh-util/src/keys.rs` for a reference implementation.

### Architecture
- **Simplicity:** No incidental complexity; simplify redundant logic.
- **No special casing:** Prefer composable design over extra booleans/branches.
- **Encapsulation:** sql-parser = grammar only (no semantic validation); sql = planning + semantics.
- **Dependencies:** New crates must be justified.
- For more design guidelines read: `doc/developer/best-practices.md`

### Polish
- No leftover `// XXX`, `// FIXME`, `dbg!`, `println!`, or commented-out code.
- No unrelated formatting changes in untouched code.
- New public items should have doc comments.

### Release notes

Release notes are auto-generated per release from PR descriptions (the [`/mz-release-notes` skill](https://github.com/MaterializeInc/mz-skills/blob/main/.agents/skills/mz-release-notes/SKILL.md));
authors don't write them. So for a user-visible change, check that the PR *description* states the
user-observable effect in user-facing terms — not only implementation detail — so the classifier includes it.

## One semantic change rule

The PR should do one thing. If it spans multiple CODEOWNERS areas (e.g. sql-parser + sql planner), consider suggesting a split.

## Rules

- Review the code, not the author. Explain the *why* behind suggestions.
- Use **nit:** for preferences where reasonable people could disagree.
- If the PR improves overall codebase health and blocking items are addressed, say so.
- Do NOT make any changes — this is read-only review.