Pull request review

Review the change as a maintainer would: is it correct, does it respect the invariants this repository documents, and will it still be right after the generators and CI run. Report findings; do not fix them unless the user asks.

1. Load the change

gh pr view <number> --json title,body,baseRefName,headRefName,files,additions,deletions
gh pr diff <number>
gh pr checks <number>

For a local branch use git diff main...HEAD and git log main..HEAD. Read every changed file in full where the diff alone does not show the surrounding logic.

2. What to check, in this order

Correctness first. Trace the changed code paths with concrete inputs. Look for nil dereferences, wrong error handling, off-by-one in ordering logic, requeue loops, non-idempotent reconciles, and status updates that skip observedGeneration.

Repository invariants (from AGENTS.md; a change that breaks one is a bug even if tests pass):

• Controller ordering and ownership: Available=True on a CCLInfraPlane implies its AppProject exists; release is monotonic; ownership is by annotation, never ownerReference; no ApplicationSet or RollingSync; AppProject sourceRepos is the catalog only, never *; cluster Secrets stay in the Argo CD namespace.
• RBAC markers live in the package of the operator that needs them; a marker in the wrong package grants the wrong operator.
• The operator never installs Argo CD.
• Catalog packages: no mirror of the API types, pointer optionals with omitempty, non-pointer required fields without omitempty, snake_case directories, a new component registered in catalogs/components/components.go (and, for cluster_base, in cluster_base.go, and in the ip verifier registry; every chart in the drift and validator registries); hack/pyschema itself is not edited per chart or per CRD.
• The ip operator verifies and never installs; a Verifier reads live objects only and a kind it reads has a matching RBAC marker in internal/controller/ip.
• CRDs: minimal spec surface, nothing that duplicates metadata or an operator flag, metav1.Condition for status.
• Leader election IDs and the -cp / -ip split are untouched.

Generated files. If the PR touches api/, any +kubebuilder: marker or doc comment on an API type, config/, catalogs/**/*.go or hack/pyschema/, confirm the generated outputs are in the PR and match. Run the generators locally on the PR head when in doubt:

make manifests generate && git status --short
make helm-generate && git status --short charts/
make python-generate && git status --short python/

Hand edits under charts/*/chart/templates/, config/crd/bases/, config/cp/rbac/role.yaml, zz_generated.*.go or the generated Python packages are findings: they will be overwritten.

Tests. New behaviour needs an envtest spec in internal/controller/cp/; a new RBAC need is only proven by make test-e2e. Table tests are for states envtest cannot reach, not for coverage. Check that assertions test the contract, not the implementation.

Code shape. A reader follows the call chain without detours. Check each new type, interface, name, guard and comment against the shape the repository already uses:

• Dependencies are reconciler fields where nil means the default, as Drift is. A struct that only bundles dependencies, or an interface for a concept the code invents, is a finding.
• A function that does I/O, blocks, starts goroutines or logs takes a context.Context as its first parameter. A context stored in a struct is a finding.
• Names read plainly at the call site and follow the package's pattern. An external API's terms stay inside its client, such as Argo CD's "manifest" inside internal/argocd.
• Every branch runs on some real path. A guard the callers already rule out, or a constant that copies the API package's, is a finding.
• Comments are brief and plain: a sentence or two on why, beside the code they explain. A rule DESIGN.md already states gets a pointer there, not a retelling.

Docs and metadata. A user-visible change adds a fragment changelog.d/<short-topic>.md in the format of changelog.d/README.md. Outside a release PR, any edit to CHANGELOG.md is a finding: only the release skill moves fragments into the changelog. Amending an existing fragment in changelog.d/ is expected, whoever wrote it, when the PR changes or reverts the unreleased behaviour it describes; check the amended text matches what now ships. A stale fragment the PR contradicts but leaves unchanged is a finding. AGENTS.md, DESIGN.md and README.md still describe what the code now does. The PR title is a valid Conventional Commit and the body follows the plain-text format (prose, blank line, hyphen bullets, no Markdown), since it becomes the commit message.

CI. lint, test, test-e2e and test-python have paths: filters; a skipped check is expected for PRs outside their inputs, but a PR that changes an input and shows the check skipped means the filter is stale. That is a finding against the filter.

3. Report

Rank findings most severe first. For each: file and line, one sentence stating the defect, and the concrete scenario in which it goes wrong. Separate hard findings from suggestions, and say explicitly what was verified by running something versus by reading. If nothing is wrong, say so plainly and name what was checked.

Do not post comments or a review to GitHub unless the user explicitly asks. When asked, post with gh pr review <number> --comment --body-file <file> (or --approve / --request-changes only when the user chooses one) and keep the same plain-text style as commit bodies.