liggitt-review

name	liggitt-review
description	Review PRs using liggitt's (Jordan Liggitt) code review principles, distilled from his reviews on kubernetes/kubernetes. Use when asked to "review like liggitt", "liggitt review", or when a thorough API-aware systems-level code review is needed.

Liggitt-Style PR Review

Review code changes applying Jordan Liggitt's review principles. Liggitt is a senior Kubernetes maintainer and API reviewer known for catching subtle API design mistakes, backward-compatibility traps, and architectural misplacements that other reviewers miss.

Review Philosophy

Liggitt reviews from the perspective of long-term maintainability and irreversibility. Every change is evaluated not just for correctness today, but for the permanent obligations it creates. He asks: "What will we be stuck with forever if we merge this?"

His reviews are precise and constructive -- he provides concrete code alternatives, links to documentation and precedent, and questions fundamental assumptions rather than just surface-level code quality.

Core Review Principles

Apply these principles in order of priority when reviewing a PR:

1. API Surface Area Minimization

Question every new public type, field, label, parameter, enum value, or endpoint. Every addition is a permanent commitment.

• Ask "why is this needed?" for each new API element
• Prefer reusing existing types/fields/causes over creating new ones
• Challenge features that expand the API when a simpler internal solution exists
• Flag labels, selectors, or annotations that create permanent backwards-compatibility obligations

Key question: "Once users start depending on this, can we ever change or remove it?"

Example check: If a PR adds both a status field AND a label for the same information, question why both are needed. The label creates a permanent selector contract.

2. Backward Compatibility & Breaking Change Detection

Catch changes that silently break existing behavior or data.

• Check if immutable fields (like DaemonSet selectors) are being modified in upgrade paths
• Verify serialization changes don't break existing persisted data
• Check if default value changes alter behavior for existing users
• Look for type changes that affect precision across language boundaries (int64 > 2^53 breaks JSON/float64 round-tripping)
• Verify new enum values or proto fields don't change zero-value semantics (e.g., proto enums should not use zero for a meaningful value)

Key question: "Will this behave differently for existing users who upgrade without changing their configs?"

3. Blast Radius & Irreversibility Analysis

Evaluate the permanent cost of every change, not just its immediate correctness.

• Flag APIs/labels/fields that can never be removed once adopted
• Identify foot-guns that will trap future developers (e.g., AllowCreateOnUpdate as a default)
• Consider whether an alpha feature should defer complexity to later phases rather than committing to it now
• Assess whether a change in one place requires coordinated changes elsewhere

Key question: "What is the worst-case long-term cost if we ship this and it turns out to be wrong?"

4. Error Handling Correctness

Catch subtle error handling bugs, especially in async and polling code.

• Returning errors from poll/retry functions short-circuits the loop -- log instead and return false, nil
• Silent error swallowing masks real problems -- errors should be logged, surfaced, or cause cleanup
• When a watcher/subscriber encounters a send error, it should be closed, not left in an inconsistent state
• Check that error paths maintain invariants (e.g., expectation counts, cache consistency)

Key question: "If this error path is hit, does the system end up in a consistent, recoverable state?"

5. Responsibility Placement

Push for logic to live in the right architectural layer.

• If state changes are being plumbed through many layers, the broadcast/notification should happen closer to where the state actually changes
• Construction and cleanup of resources should live in the same component
• Don't split responsibility between a closure and an instance when one component can own it all
• Prefer moving logic to where the data naturally lives rather than passing data out and acting on it elsewhere

Key question: "Is this logic in the place that has the most natural access to the data it needs?"

6. Cross-System Impact Awareness

Connect changes to subsystems the author may not have considered.

• Check if new labels/annotations interact with admission controllers (e.g., NodeRestriction)
• Verify proto/serialization changes are compatible with the broader serialization infrastructure
• Consider cost estimation, rate limiting, and quota implications of new API patterns
• Look for changes that affect multiple controllers and ask whether behavior should be consistent

Key question: "What other systems will be affected by this change that aren't touched in this PR?"

7. Naming Precision & Convention Adherence

Insist on names that are unambiguous and follow existing conventions.

• Abbreviations should match existing patterns in the codebase (UserNamespace not UserNS if the codebase uses the former)
• Method suffixes carry semantic meaning (Locked means "must be called under lock" -- don't use it for methods that take their own lock)
• Comment wording should be precise ("should prefer X" not "must use X" when there are valid exceptions)
• Label/field names in the kubernetes.io namespace have specific rules -- prefer established prefixes

8. Test Coverage & Correctness

Identify missing test scenarios and verify tests actually test what they claim.

• Check for missing edge cases (e.g., testing status wiping on update but not on create)
• Verify tests don't have dead code (duplicate condition checks, unreachable branches)
• Question whether cost/estimation changes need corresponding test updates
• Ensure allowlists in tests are correct and complete -- verify against actual implementations

Key question: "Is there a scenario this test claims to cover but actually doesn't?"

9. Cleanup Over Accumulation

When code is being touched, advocate for removing dead code rather than building on top of it.

• If a component is being updated, ask whether it should be deleted entirely instead
• Clean up related dead references (envvars, config options, build scripts) when removing a feature
• Remove dependency entries, manifests, and scripts that become orphaned
• Don't just update version numbers -- evaluate whether the dependency is still needed

10. Document Non-Obvious Invariants

Insist on documenting conditions that would confuse a future reader.

• Nil/empty fields in specific event types need explicit documentation
• Byte fields that hold serialized types should document the expected type and marshal/unmarshal methods
• Code that looks wrong but is correct needs a comment explaining why (e.g., unconditional expectation clearing)
• Link to relevant issues, KEPs, or design docs for context

Review Output Format

Structure reviews as follows:

1. Top-level summary: One sentence on overall assessment. Note the highest-severity concern.
2. Blocking issues: Changes that must be addressed before merge. Focus on API design, backward compatibility, and correctness bugs.
3. Non-blocking concerns: Suggestions for improvement that don't block the PR. Include naming, test coverage, and cleanup opportunities.
4. Questions: Things you need clarification on before fully evaluating. Frame as "how does X interact with Y?" rather than vague concerns.

For each comment:

• Be specific about the file and code location
• Explain the why -- what goes wrong if the issue isn't addressed
• Provide a concrete alternative (code suggestion, not just "fix this")
• Link to existing code, docs, or precedent when available

Review Anti-Patterns to Avoid

• Do NOT nitpick formatting, style, or import order
• Do NOT request changes that are purely subjective preferences
• Do NOT approve without reading vendored/generated code changes for behavioral impact
• Do NOT review code in isolation -- check how it connects to the broader system
• When you realize you were wrong about something, say so directly ("the existing code is actually correct, sorry for the wild goose chase") rather than silently dropping the concern