pr-review

name	pr-review
description	Reviews pull requests for the Confluent Terraform provider. Use when reviewing PRs, doing self-review before sharing with the team, or when the user mentions "review PR", "help with PR", "review changes", "self-review", "review local changes", or "check my PR". Focuses on SDKv2 schema correctness, resource registration, deprecation/backward-compat, generated-resource (cli-terraform-generator) regeneration safety, state upgraders, and the acceptance/WireMock/live test tiers.
allowed-tools	["Read","Bash","Grep","Glob","Task"]

PR Review Skill

Reviews pull requests for the Confluent Terraform provider (Go + terraform-plugin-sdk/v2), focusing on the failure modes that corrupt user state or break backward compatibility and that no linter here will catch (this repo has no golangci-lint — only make checkfmt).

The review-relevant conventions live in .claude/rules/resource-schema.md and .claude/rules/acceptance-tests.md, which auto-load when you edit matching files. This skill applies them to a diff; consult the rules and existing sibling resources rather than re-deriving conventions.

Two Review Modes

The mode is selected by the invocation context, not by the user. If the user supplies a PR number/URL or asks about someone else's PR, run Formal Review Mode. Otherwise (no PR number, working from a local branch, phrases like "self-review" or "check my PR") run Self-Review Mode. When ambiguous, ask which mode to use.

Self-Review Mode (for PR authors)

Use when: the author wants to check their own changes before sharing with the team.

Goals:

• Catch issues early, before formal review
• Verify a new resource/data source is registered and its schema is internally consistent
• Confirm stored-shape changes ship a SchemaVersion bump + state upgrader
• Confirm acceptance-test coverage (WireMock-backed where possible)

Formal Review Mode (for reviewers)

Use when: a reviewer needs to evaluate a PR from another team member.

Goals:

• Quickly understand the scope and purpose of changes
• Identify state-safety and backward-compatibility risks
• Provide constructive feedback grounded in SDKv2 + Confluent conventions
• Verify the PR template checklist is honored

Review Process

Step 1: Gather Information

For local changes (self-review):

git diff master --name-only
git diff master --stat
git diff master

For GitHub PRs:

gh pr view <PR_NUMBER> --json number,title,body,author,baseRefName,headRefName,additions,deletions,changedFiles,state,reviewDecision
gh pr view --json number,title,body,author,baseRefName,headRefName,additions,deletions,changedFiles,state,reviewDecision
gh pr diff <PR_NUMBER>
gh pr view <PR_NUMBER> --json reviews,comments
gh issue view <ISSUE_NUMBER> --json body,comments

Step 2: Filter Files for Review

SKIP or skim (generated or infrastructure):

• vendor/**
• .semaphore/**, .goreleaser*.yml, .golicense.hcl — release/CI

DO review carefully (small file, big blast radius):

• internal/provider/provider.go (ResourcesMap / DataSourcesMap registration)
• internal/provider/constants.go (shared deprecation messages)
• internal/provider/state_upgraders*.go
• go.mod / go.sum (new dependencies; note the SDK is v2, not the framework)
• docs/**, examples/** (hand-maintained; drift silently)

Detect a generator PR first — it changes how you review. Much of this repo's current churn is replacing hand-written resources with cli-terraform-generator output. A PR is a generator PR when any of these hold: it touches .tf-generation-checksum, it carries the cli-terraform-generator label (applied automatically by .github/workflows/label-generated-code.yaml), or a changed *.go file starts with // Code generated by cli-terraform-generator ...; DO NOT EDIT.. If so, run the Generated-resource review in Step 5 — the failure modes and the place a fix belongs are different from hand-written code.

Step 3: Categorize the Changes

Category	File patterns	What to check
Resources	internal/provider/resource_*.go	Schema correctness, ForceNew, CRUD, registration, deprecation
Data sources	internal/provider/data_source_*.go	Computed shape, registration
Provider wiring	internal/provider/provider.go	New entries in ResourcesMap / DataSourcesMap, providerConfigure
Shared constants	internal/provider/constants.go	Deprecation-message reuse; extracted literals live here
Generated code	files headed // Code generated by ...	Behavior-preserving regen; fix belongs upstream (see Step 5)
Error/wait helpers	internal/provider/utils.go, utils_wait.go	Swallowed errors, 404 detection, response body, terminal states
State upgraders	internal/provider/state_upgraders*.go	SchemaVersion bump paired with an upgrader
Acceptance tests	internal/provider/*_test.go (non-live)	TF_ACC gating, WireMock setup, attribute assertions
Live tests	internal/provider/*_live_test.go	live_test,all build tags, ...Live / ...DriftDetection naming
Docs / examples	docs/**, examples/**	Match current schema; run the docs-drift skill
Project rules/skills	.claude/rules/**, .claude/skills/**	Frontmatter, path globs, trigger phrases

Step 4: Check Critical Requirements

IMPORTANT: only review lines that were actually changed in the PR diff. Context lines are for understanding, not for review. Do not flag pre-existing issues in unchanged code.

1. Registration completeness (MANDATORY for new resources/data sources)

• New resource has an entry in ResourcesMap (new data source → DataSourcesMap) in provider.go
• Type name is consistent across the map key, the file name, and the docs page

Red flags: a new resource_*.go with no provider.go change → the resource does not exist to Terraform; a mismatched type name → broken resource address.

2. Schema attribute correctness

• Required / Optional / Computed combination is valid and matches API semantics (no Required+Computed; server-assigned values are Computed; optional-with-default is Optional+Default or Optional+Computed)
• ForceNew: true on attributes the API cannot update in place — and not on attributes it can
• Sensitive: true on secrets
• ValidateFunc/ValidateDiagFunc where values are constrained
• DiffSuppressFunc where the API canonicalizes a value (case-folding a region/enum via strings.EqualFold, server-side query normalization, etc.) so a benign server rewrite doesn't show as permanent drift — and, conversely, that a newly-added suppressor isn't hiding a change the user actually made. This is one of the most-discussed schema behaviors in real reviews; match the sibling that already suppresses the same kind of value.
• Nested blocks follow the sibling-resource Elem pattern

3. Deprecation and backward compatibility

• No removed/renamed attribute or resource without a deprecation cycle
• Deprecations reuse a constants.go message (e.g. deprecationMessageMajorRelease3)
• No newly-added ForceNew or changed default that would recreate existing users' resources unexpectedly (unless gated on a major release and called out)

4. State upgraders

• Any change to an existing resource's stored shape bumps SchemaVersion and adds a matching StateUpgrader. A shape change with no upgrader corrupts existing state on the next apply — treat a missing upgrader as a blocker.

5. Testing

• New behavior has an acceptance test (resource.Test/resource.ParallelTest, TF_ACC=1), WireMock-backed where possible so it runs in CI without credentials
• The test asserts on the changed attributes, not merely that apply succeeds
• A live test is added/updated when real API behavior matters and WireMock can't cover it

6. Error handling and async status

The shared helpers in utils.go and the provision-status switches in utils_wait.go are where the maintainer's highest-value catches land, and no linter guards them.

• Errors are surfaced, not swallowed — a non-404 error in a Read must not be discarded as "resource gone" (that silently deletes it from state). Deletion detection keys on the real status via the utils.go helpers (isNonKafkaRestApiResourceNotFound, ResponseHasStatusForbiddenDueToInvalidAPIKey), not on a proxy like schema presence.
• Response bodies are handled correctly — close the original io.ReadCloser before overwriting response.Body, and don't assume a body can be read twice.
• Wait/poll loops (utils_wait.go) handle terminal failure states (ERROR, DISCONNECTED, FAILED) explicitly, not just the success state, and don't gate the whole attribute-set block on Status != nil in a way that leaves a resource showing perpetual diff while still PROVISIONING.

For a diff heavy in catch/fallback/error-classification logic, hand off to the pr-review-toolkit:silent-failure-hunter agent rather than eyeballing it.

Step 5: Check Project-Specific Patterns

Docs drift (no tfplugindocs in CI)

• Schema changes are reflected in docs/resources/*.md / docs/data-sources/*.md, and examples/ still validates. Invoke the docs-drift skill to compare docs/examples against the current Go schema maps.

Consistency with siblings

• A new resource follows the structure of the closest existing resource_*.go (CRUD function layout, error handling via diag.Diagnostics, import support, WireMock test scaffold). When in doubt, read the nearest sibling end to end before flagging a deviation.

Generated / regenerated resources (cli-terraform-generator)

When Step 2 flags a generator PR, the review shifts from "is this code correct" to "is this regeneration a faithful, behavior-preserving swap." Most of these PRs claim behavior is unchanged; the review's job is to verify that claim against the diff.

• Behavior-preservation claims are checked, not trusted. Regressions hide in generated output that reviewers have caught repeatedly: GetOkExists vs GetOk (the former sends an explicit zero, changing the request), a TypeList missing its Elem, or a dropped Optional: true on an element (which silently changes a TypeSet's hash). Diff the effective schema (type / required / optional / computed / forcenew / validation) against master, not just the prose.
• ValidateFunc divergence is a spec-fidelity question, not a code bug. The generator derives validators purely from the OpenAPI minispec, so a StringLenBetween(0, N) where you'd expect StringIsNotEmpty usually means the spec declares no minLength. The fix belongs in the spec (add minLength: 1) and a regenerate — not a hand-edit here. Same for odd attribute descriptions copied verbatim from the spec.
• The fix usually lives upstream. Generator template bugs → cli-terraform-generator; wrong validation / descriptions → the minispec. A generated file carries DO NOT EDIT; a hand-patch to it is lost on the next regenerate. Expect the author to link the upstream PR, and ask for it if a "fixed in the generator" claim has no link.
• Generated tests are intentionally additive. A regeneration may ship existence-only generated tests alongside kept hand-written exact-value tests. Confirm the exact-value coverage still exists somewhere before flagging the generated test as too shallow (see the acceptance-tests rule).

Constants and hardcoded values

• Repeated literals (page sizes, org/env IDs in live tests, shared enums, poll intervals) are pulled into constants.go (or constants_live_test.go for live-test-only values) rather than scattered. The near-universal convention here is that these live in one place — "extract this to a constant" is a recurring review ask.

Examples conventions (examples/**)

Sibling examples are the spec. Before flagging or approving an example, read the closest existing examples/configurations/* directory and match it:

• terraform.tfvars is committed directly with placeholder values — not a .tfvars.example variant.
• No tutorial scaffolding: drop step-numbered banner comments (# ----- 1. Create ... -----) and per-block explanatory prose that no sibling example carries.
• Placeholders are realistic (match the width/shape the schema description shows), and variables.tf defaults don't duplicate the same assignments already in terraform.tfvars.
• Resource display_names follow the sibling naming (app-manager, app-manager-kafka-api-key, …) rather than inventing a new scheme.

Step 6: Check PR Hygiene

• PR description fills in the template: release notes, What / Blast Radius (resources/data sources, terraform import), live-testing evidence, breaking-change status, feature-flag enablement
• The description's claims match the diff. "Behavior identical", "no schema change", and "error messages unchanged" are exactly the assertions that have turned out to be wrong on these PRs — spot-check the load-bearing claim against the actual change rather than taking it as read.
• One logical change per PR; no unrelated changes bundled in
• No secrets in the diff (.tfvars with real credentials, API keys, cluster IDs beyond fixtures)

What NOT to Flag

Tooling-owned style (avoid nitpicking)

• Formatting and import order — owned by make checkfmt (goimports). Don't hand-flag it.
• "This might fail CI" — the Semaphore build/test catches it; the author fixes it.

Comment Preservation (but not accuracy)

• Don't nitpick comment style. But a comment or log/error message that the diff has made stale, wrong, or misleading is fair game — reviewers here routinely ask for exactly that. Prefer "update it" over "delete it": comments explain "why", so update them when the code changes.
• User-facing wording is in scope. Log lines and error messages ship to customers. Flag a message that has become inaccurate or non-inclusive (e.g. a "Importing Cloud API Key" log that now also handles Global and Tableflow keys), and ask for a comment to be expanded where the reasoning behind a non-obvious check would otherwise be lost.

Output Format

For Self-Review

## Self-Review Summary

### Changes Overview

[Brief summary of what changed]

### Critical Requirements Checklist

- [ ] Registration (ResourcesMap / DataSourcesMap): [status, location of any gap]
- [ ] Schema correctness (Required/Optional/Computed, ForceNew, Sensitive): [status]
- [ ] Deprecation / backward compatibility: [status]
- [ ] SchemaVersion bump + state upgrader (if shape changed): [status]
- [ ] Acceptance/live test coverage: [status]

### Issues to Address Before PR

1. [High-priority issue with file:line]
2. [Medium-priority issue with file:line]

### Suggestions (Optional)

- [Nice-to-have improvements]

### Ready for Review?

[Yes / Not yet, with reasoning]

For Formal Review

## PR Review: #{number} - {title}

**Author:** {author}
**Branch:** {headRefName} → {baseRefName}
**Changes:** +{additions} / -{deletions} across {changedFiles} files

### Summary

[2-3 sentence summary of what the PR does and why]

### Changed Components

- [Categorized list of changed files, excluding generated/infra]

### Findings

#### Issues (Must Fix)

- [ ] **[category]**: [description] - `file:line`

#### Suggestions (Consider)

- [ ] **[category]**: [description] - `file:line`

#### Positive Observations

- [Good patterns, thorough tests, well-written code]

### Test Coverage Assessment

- **New tests added:** [Yes/No, list test files; WireMock vs live]
- **Coverage gaps:** [Untested attributes or edge cases]

### State & Compatibility Notes

[Any concern about forced recreation, missing state upgrader, or un-deprecated removal]

### Recommendation

**[APPROVE / REQUEST CHANGES / NEEDS DISCUSSION]**

[Brief rationale]

Review Categories

Use these labels in findings:

Category	Description
registration	Resource/data source not in ResourcesMap/DataSourcesMap, or name mismatch
schema	Wrong Required/Optional/Computed, missing ForceNew/Sensitive/DiffSuppressFunc
compatibility	Un-deprecated removal/rename; unexpected forced recreation
regression	A "behavior-preserving" regeneration/refactor that actually changed behavior
generated	Generator/spec-fidelity issue; fix belongs in cli-terraform-generator or the minispec
state	Stored-shape change with no SchemaVersion bump / state upgrader
errors	Swallowed error, mis-detected 404, or mishandled response body / wait status
testing	Missing acceptance/live coverage; test asserts nothing about the change
docs	Schema/doc/example drift; missing CHANGELOG entry
examples	examples/** diverges from the sibling convention
messages	Stale/inaccurate/non-inclusive log or error message, or a comment left wrong
secrets	Credentials or real IDs in the diff
consistency	Diverges from the closest sibling resource without reason
style	Naming/conventions where make checkfmt does not enforce

Tips

• Start with the PR description and the PR template checklist.
• For a new resource, trace it end to end: constructor exists → registered in provider.go → schema attributes make sense → CRUD returns diag.Diagnostics → WireMock acceptance test present → docs page updated. A missing link usually means the resource is dead, unsafe, or untested.
• Use Task with the Explore agent for blast-radius questions (e.g. "which resources share this attribute before I change its ForceNew").
• Companion skill: docs-drift validates docs/ and examples/ against the current Go schema maps — run it whenever schema attributes change, since tfplugindocs is not in CI.
• When suggesting a simpler alternative, confirm it exists on terraform-plugin-sdk/v2 (this repo does not use the plugin framework) before posting.