Pr Reviewer

Local Review

Perform systematic review with actionable, validated feedback only. Use this skill as an explicit local self-review step before handoff, not as a generic replacement for native PR review tools. Run it before /done when a coding session produced changes worth checking.

Reference Files

Scope

• Default target: staged or uncommitted local changes
• Secondary target: current branch diff against base when the working tree is clean or the user asks for branch review
• Explicit PR requests are secondary: keep the same review criteria, but treat them as a handoff path rather than the main workflow
• Keep the skill focused on concrete bugs, missing validation/tests that clearly matter, and repository instruction-file compliance
• Do not use this skill for inbound PR comments or thread resolution; use pr-comments for that

Security audit mode (whole-codebase)

Triggered by an explicit security ask ("security audit", "find vulnerabilities", "deepsec", "security review", "threat model", "audit for security"). This mode overrides the default diff scope — it proactively sweeps the relevant subsystem or the whole repo by vulnerability class, not just changed lines. Everything else about the skill is unchanged: it stays report-only and uses the same three-tier output.

• Scope: the subsystem the user names, or the whole codebase if unscoped. Diff status is irrelevant here — review code as it stands, not just what changed.
• Always load `references/security-checklist.md` and walk the threat-model lens and vulnerability-class sweep it defines, rather than waiting for a diff to touch a sensitive area.
• Walk by vulnerability class, not by file — for each OWASP-aligned class, search the codebase for the pattern (e.g. query construction, auth checks, deserialization, secret handling) and confirm each hit against the actual code before reporting.
• High-signal bar still applies — report only concrete, exploitable findings with a path/line and a plausible attack; never speculative "could be risky" items. Drop anything you can't tie to a real exploit path.
• Output: the same Must fix before push / Should fix soon / Ready for handoff tiers, with each finding naming the vulnerability class, the location, and the exploit path.

Workflow

Copy this checklist to track progress:

Review progress:
- [ ] Discover local review target
- [ ] Gather context and scoped instruction files
- [ ] Choose the local review path
- [ ] Validate findings
- [ ] Produce the review report

1. Discover the review target:

• If staged or unstaged changes exist, review those first
• Otherwise review the current branch diff against its base
• Only switch to a PR handoff summary when the user explicitly points at an existing PR
• Record the current branch and changed files so the report is grounded in the local session

2. Gather context:

• Capture the change intent from the session, recent commits, or the user's request
• Load relevant repository instruction files (AGENTS.md / CLAUDE.md as applicable, including any in nested package/MFE directories whose code is in the diff)
• Apply only in-scope instruction-file rules for the changed paths
• Run the project's lint, type check, and test commands (from package.json scripts) to capture current status — note pre-existing failures so they are distinguishable from regressions caused by the change. Include the lint/type-check/test status in the report so the reader knows the baseline at review time

3. Choose the local review path:

• Local self-review is the default: current diff/branch with a local report in chat
• Existing PR requests are secondary: apply the same validation bar, then produce a concise handoff summary instead of changing the main workflow
• For large changes, shard by subsystem and keep the final report consolidated
• Optional, non-trivial diffs only: if an independent review CLI is already installed (e.g. codex exec, droid exec), you may run it read-only as a different-model second opinion and fold any validated findings into the same report. This hedges against Claude reviewing Claude-authored code. Skip silently if none is installed — never add it as a dependency
• Treat external-engine output as advisory: validate every finding against the actual diff before including it, and drop anything this skill would not have flagged on its own

4. Validate issues:

• Re-check exact lines before reporting
• Keep only high-confidence issues; drop speculative or duplicate items
• Confirm each issue still applies to the latest diff and maps to a changed line
• Collapse multiple comments that share the same root cause into one finding

5. Produce the report:

• Default output: a local review report in chat
• Organize findings into Must fix before push, Should fix soon, and Ready for handoff
• Do not post inline comments, resolve threads, or handle inbound review feedback from this skill
• Hand off inbound PR feedback to pr-comments

High signal only

Flag only when certain:

• Code will fail to compile (syntax, types, imports)
• Code will produce incorrect behavior (clear logic or state errors)
• Code introduces a concrete security risk with direct exploit path — load references/security-checklist.md for the three-tier classification when the diff touches auth, input handling, external APIs, or environment configuration
• Code introduces a measurable performance regression — load references/performance-checklist.md for common bottleneck patterns when the diff touches data fetching, rendering, images, or dependencies
• Changed behavior is clearly missing a necessary regression or validation test, including: a new component or hook shipped with no co-located test file, or an existing test where every assertion is a render-only presence check (expect(getByText(...)).toBeInTheDocument()) with no user interaction or branch coverage
• Bug fix without a failing test that reproduces it first (Prove-It Pattern: if the fix is correct, a test for the bug should fail before and pass after)
• Test code over-abstracts shared setup to the point where individual tests are unreadable without tracing helpers (prefer DAMP — Descriptive And Meaningful Phrases — over DRY in test code)
• Lint, type check, or tests fail as a result of the change (distinguish from pre-existing failures captured in step 2)
• Unambiguous instruction-file violation (quote rule, verify scope)
• YAGNI violation: code adds abstractions, config systems, or extension points not justified by a current requirement (three similar lines is better than a premature abstraction)
• KISS violation: implementation is more complex than the problem demands — a simpler approach exists that achieves the same result
• Code exhibits AI-generated patterns (over-commenting, unnecessary wrapping, type bypasses, premature abstraction) — load references/ai-slop-patterns.md for the detection catalog
• File pushed past ~1000 lines by the diff when the new code could be extracted into a focused module
• Ad-hoc conditionals or feature-specific branches inserted into unrelated shared code paths
• Bespoke helper duplicating an existing canonical utility in the codebase
• Logic placed in the wrong layer when there is a clear canonical home elsewhere

Never flag:

• Style, quality, or subjective preferences
• Pre-existing issues unrelated to the change
• Potential issues dependent on unknown inputs
• Unrealistic edge cases or speculative risks that don't map to a concrete exploit or repro path
• Broad rewrites or architectural changes beyond the diff's intent
• Linter-only issues likely caught automatically
• Explicitly silenced violations

Output format

Read references/comment-examples.md before producing the report if you need a formatting refresher.

When the structural quality rubric is loaded, structural findings use the same tiers — presumptive blockers from the rubric map to Must fix before push, and other structural issues map to Should fix soon.

Default local output:

## Local review

### Must fix before push
- [<severity>] `path/to/file.ts:line` <short factual title>
  Why: <one to two sentences with concrete impact>
  Fix: <committable fix or clear implementation guidance>

### Should fix soon
- [<severity>] `path/to/file.ts:line` <short factual title>
  Why: <one to two sentences with concrete impact>
  Fix: <committable fix or clear implementation guidance>

### Ready for handoff
- <brief readiness summary>

If the user explicitly points at an existing PR, adapt the same validated findings into a concise handoff summary:

## PR handoff summary

- [<severity>] `path/to/file.ts:line` <short factual title>
  Why: <one to two sentences with concrete impact>
  Fix: <committable fix or clear implementation guidance>

Summary (if no issues):

## Local review

### Must fix before push
- None.

### Should fix soon
- None.

### Ready for handoff
- No blocking issues found. Checked for high-confidence bugs, missing validation/tests, and instruction-file compliance on the current local changes.

Anti-patterns

• Starting by asking for a PR number when local changes are available -> review the local diff first
• Teaching inline review comments as the default output -> keep the main path local-first
• "This might cause issues" -> "Variable x is undefined at src/foo.ts:45, causing ReferenceError at runtime."
• "Consider refactoring" -> "Violates instruction-file rule '<quoted rule>' in scoped file src/foo.ts."
• Multiple comments for the same root cause -> one comment linking all affected locations
• Pasting an external engine's output verbatim -> re-validate each finding against changed lines first; advisory input is not a finding until you confirm it

Boundary with simplify (private)

This skill produces a report only — findings organized by severity (Must fix before push, Should fix soon, Ready for handoff) with no automatic file edits. The working tree is unchanged when the skill finishes.

Use the private simplify skill instead when you want fixes applied automatically: it fans out four concurrent agents over the diff and edits files in-place, then re-runs lint/type-check/tests to verify. Both skills cover reuse, quality, and efficiency issues; the difference is report-only vs fix-in-place.

Related skills

• done for session capture after the review is complete
• pr-babysitter for triaging and resolving inbound review threads after feedback has been left
• simplify (private) for automatic fix-in-place rather than a review report

Every flagged issue should be something a senior engineer would catch.

AI Slop Patterns

Detection catalog for AI-generated code patterns that pass lint and tests but read as machine-written. Load when the user asks to deslop, clean up AI code, remove slop, or when reviewing AI-assisted code changes.

Focus on patterns that are distinctively AI-generated, not general code quality issues (those belong in structural-quality-rubric.md).

Over-commenting

Comments that restate what the code already says.

Flag:

• JSDoc on functions with self-documenting names and types
• // Handle the error above a catch block
• // Return the result above a return statement
• // Initialize variables above const declarations
• Block comments explaining a single obvious line
• // Destructure the props above destructuring assignments

Fix: Delete the comment. If removing it would confuse a reader, the code needs a better name, not a comment.

Unnecessary error handling

Wrapping infallible operations in try-catch or guarding against impossible states.

Flag:

• try-catch around pure computations (string manipulation, array mapping, object destructuring)
• Null checks on values the type system guarantees are non-null
• || [] on a value already typed as an array
• ?? undefined (no-op — undefined is already the default)
• || '' on a string (not string | undefined) type
• Catch blocks that just rethrow without modification
• Error boundaries wrapping components that cannot throw

Fix: Remove the guard. Trust the type system and framework guarantees. Only validate at system boundaries (user input, external APIs, network responses).

Type bypasses

Casting or suppressing types instead of fixing the underlying type issue.

Flag:

• as any — always a smell; fix the type or narrow with a type guard
• as unknown as T — double-cast to force an incompatible type
• @ts-ignore / @ts-expect-error without an explanation comment
• Unnecessary type assertions on values that already match the target type
• ! (non-null assertion) when the value could genuinely be null
• Generics defaulting to any (useState<any>())

Fix: Fix the type at its source. If the upstream type is wrong, fix upstream. If a third-party type is wrong, use a targeted .d.ts override.

Premature abstraction

Abstractions created before repetition justifies them.

Flag:

• Helper functions called from exactly one site
• Wrapper classes with a single method that delegates to the wrapped object
• Config objects consumed by one function
• Factory functions that always return the same variant
• Custom hooks that are thin wrappers around a single useState or useEffect
• utils/ files with one export
• Constants extracted for a value used once

Fix: Inline the abstraction. Three similar lines is better than a premature helper. Extract only when the same logic appears 3+ times.

Verbose naming

Names that repeat information already conveyed by the type system or context.

Flag:

• userArray, nameString, isLoadingBoolean — type is in the name
• handleOnClickButton — redundant event + element in handler name
• fetchDataFromAPIAndTransformResponse — implementation in the name
• getUserByIdFromDatabase — storage detail in the name
• IUserInterface, UserType — type-system prefix/suffix on types
• setIsLoadingToTrue — value in the setter name

Fix: Use the simplest name that is unambiguous in context. users, name, loading, handleClick, fetchUser, getUser.

Structural bloat

Files, exports, and patterns that add surface area without value.

Flag:

• Barrel files (index.ts) that re-export everything from a directory
• Empty utility files with boilerplate but no logic
• Files with only type re-exports (export type { Foo } from './foo')
• Dead code behind if (false) or // @deprecated with no removal date
• Duplicate type definitions when a shared type exists
• Separate files for a single small constant or type

Fix: Delete the file or inline the content. Co-locate small types and constants with their consumer.

Defensive excess

Guarding against states that the language or framework prevents.

Flag:

• ?. optional chaining on values that cannot be null (non-optional props, required function parameters, const assignments from non-nullable sources)
• Array.isArray() check on a value typed as T[]
• typeof x === 'function' on a value typed as a function
• if (x !== null && x !== undefined) when the type is not nullable
• try { JSON.parse(knownValidJSON) } on a value that is always valid JSON
• Fallback UI for error states that cannot occur in the component's data flow

Fix: Remove the guard. If the type system says it's safe, it's safe. If you're unsure, fix the type — don't add a runtime check.

Template residue

Placeholder content left behind from AI generation.

Flag:

• // TODO: implement or // TODO: Add error handling with no implementation
• // Add your logic here
• Generic error messages: "An error occurred", "Something went wrong", "Failed to process request"
• Console.log statements used for debugging: console.log('here'), console.log(data)
• Commented-out code blocks with no explanation
• Empty function bodies or stub returns (return null, return undefined, return {})

Fix: Either implement the functionality or delete the placeholder. Replace generic error messages with specific, actionable messages.

Applying fixes

When reviewing for slop:

1. Read the diff with slop detection in mind — don't fix pre-existing patterns outside the diff 2. Group findings by category, not by file 3. Prioritize behavioral preservation — deslop changes should never alter runtime behavior 4. Apply the codebase's existing conventions, not an ideal standard 5. When in doubt about whether something is slop or intentional, check git blame — if the same author wrote it recently in an AI-assisted session, it's likely slop

Output Examples

End-of-session self-review

## Local review

### Must fix before push
- [major] `src/profile/page.tsx:42` Missing null guard before dereferencing `profile`
  Why: `profile` can be `null` on the first render, so `profile.id` throws before the loading state can complete.
  Fix: Guard `profile` before dereferencing, or move the access into the branch that handles loaded data.

### Should fix soon
- [minor] `src/components/profile-card.tsx:18` API mapping bypasses repository mapper rule
  Why: The changed code casts the API payload directly in the component, but the scoped instruction file requires explicit mapping in a dedicated mapper.
  Fix: Move the payload transformation into the existing mapper module and pass the mapped UI model to the component.

### Ready for handoff
- Not ready until the must-fix items are addressed.

Ready for handoff

## Local review

### Must fix before push
- None.

### Should fix soon
- None.

### Ready for handoff
- No blocking issues found. Checked for high-confidence bugs, missing validation/tests, and instruction-file compliance on the current local changes.

PR handoff summary

## PR handoff summary

- [major] `src/profile/page.tsx:42` Missing null guard before dereferencing `profile`
  Why: `profile` can be `null` on the first render, so `profile.id` throws before the loading state can complete.
  Fix: Guard `profile` before dereferencing, or move the access into the branch that handles loaded data.

Performance Checklist

Common bottlenecks to check during review. Load when the diff touches data fetching, rendering, images, dependencies, or bundle-affecting imports.

Contents

• Performance budgets
• Database and API bottlenecks
• React and frontend bottlenecks
• Image and asset bottlenecks
• Bundle size bottlenecks

Performance Budgets

Database and API

React and Frontend

Images and Assets

Bundle Size

Red Flags

Flag these during review even without profiling data — they are almost always problems:

• N+1 query pattern (query inside a loop)
• List endpoint without pagination
• Images without explicit dimensions
• React.memo/useMemo/useCallback applied without evidence of re-render cost
• API response returning full objects when the client uses two fields

Review Modes

This skill complements native review tools by narrowing scope and enforcing repository-specific rules. Default to local self-review mode.

Preferred order

1. Review the local staged or unstaged diff and return the report in chat. 2. If the working tree is clean, review the current branch diff against base and return the report in chat. 3. Only switch to a PR handoff summary when the user asks to review an existing PR and local-first output is not enough.

Keep the value-add narrow

Use this skill when the review needs:

• high-confidence bug filtering
• repository instruction-file compliance
• concise, committable feedback before handoff
• a final local check before commit, push, PR, or /done

Do not use this skill for:

• inbound PR comments or thread resolution
• default inline review-comment generation
• broad style commentary
• architecture brainstorming
• low-confidence “might be an issue” feedback

Security Checklist

Three-tier classification for security-relevant changes. Load when the diff touches auth, input handling, external APIs, file uploads, or environment configuration — and always in pr-reviewer's Security audit mode (whole-codebase).

Contents

• Always do
• Ask first
• Never do
• OWASP quick reference
• Threat-model lens (audit mode)
• Vulnerability-class sweep (audit mode)

How to use this file

• Diff review (default): use Always do / Ask first / Never do / OWASP quick reference to classify the security-relevant lines in the change.
• Security audit mode (whole-codebase): additionally run the Threat-model lens and the Vulnerability-class sweep below across the named subsystem or whole repo. Walk by class, confirm each hit against real code, and report only concrete exploit paths.

Always Do

Apply these to every change that handles user input, authentication, or external data:

• Parameterize queries — never interpolate user input into SQL, ORM, or NoSQL queries
• Validate and sanitize input — use schema validation (Zod, Yup) at system boundaries; reject unexpected shapes early
• Encode output — escape user-supplied content before rendering in HTML, URLs, or shell commands; use framework auto-escaping (React JSX, Next.js Server Components) and avoid dangerouslySetInnerHTML
• Use HTTPS everywhere — enforce TLS for all external calls; reject plain HTTP in API clients
• Hash passwords with bcrypt/scrypt/argon2 — never store plaintext, MD5, or SHA-family hashes for passwords
• Set security headers — Content-Security-Policy, Strict-Transport-Security, X-Content-Type-Options: nosniff, X-Frame-Options: DENY
• Secure cookies — HttpOnly, Secure, SameSite=Strict (or Lax with justification)
• Audit dependencies — run npm audit or equivalent; flag known vulnerabilities in the diff's dependency changes

Ask First

Flag these for explicit human confirmation before merging — the reviewer needs to verify intent and scope:

• Auth flow changes — login, logout, session management, token refresh, OAuth callback
• Sensitive data storage — PII, payment info, health data, credentials; verify encryption at rest
• External service integrations — new API keys, webhook endpoints, third-party SDKs
• CORS configuration changes — verify allowed origins are intentional and minimal
• File upload handling — validate file type, size limits, storage location; never serve uploads from the app domain without scanning
• Rate limiting changes — verify thresholds protect against abuse without blocking legitimate users
• Permission or role changes — elevation, new roles, access control modifications
• Environment variable additions — confirm no secrets are hardcoded; verify they're in .env.example but not committed in .env

Never Do

Automatic critical severity if found in the diff:

• Commit secrets — API keys, tokens, passwords, private keys in source
• Log sensitive data — PII, tokens, passwords, or full request bodies in production logs
• Client-side-only validation — always validate server-side; client validation is UX, not security
• Disable security headers — removing CSP, HSTS, or X-Frame-Options without documented justification
• Use `eval()` or `innerHTML` with user data — use safe alternatives (JSON.parse, textContent, sanitized HTML)
• Store auth tokens in `localStorage` — use HttpOnly cookies; localStorage is accessible to any XSS
• Expose stack traces in production — use generic error messages; log details server-side only
• Trust client-sent IDs for authorization — always verify ownership server-side

OWASP Quick Reference

Threat-model lens (audit mode)

Before sweeping for bugs, frame what you're protecting. Spend a few minutes mapping these so the sweep is targeted, not generic:

• Assets — what's worth stealing or breaking here? Credentials, PII, payment data, tenant isolation, admin capability.
• Entry points — where does untrusted input enter? HTTP routes, webhooks, file uploads, message queues, CLI args, env, third-party callbacks.
• Trust boundaries — where does data cross from less-trusted to more-trusted (client→server, tenant→tenant, user→admin, external API→internal)? Every boundary is a place to check authz and validation.
• Actors — anonymous user, authenticated user, other tenant, insider, compromised dependency. For each finding ask "which actor reaches this, and what do they gain?"

A finding only matters if a real actor reaches a real asset through a real entry point. Use this to drop speculative items.

Vulnerability-class sweep (audit mode)

Walk the codebase one class at a time. For each, search for the pattern, then confirm each hit against the actual code before reporting. Suggested search anchors are starting points, not exhaustive.

For each confirmed hit, report it through the standard three-tier output with the vulnerability class, location, and exploit path.

Severity Rubric

Use the smallest severity that still matches the concrete impact. Map severities into the local review report like this:

• critical and major -> Must fix before push
• minor -> Should fix soon
• no qualifying issue -> Ready for handoff

Critical

Use when the change introduces:

• a certain compile or type failure
• a direct security issue with an obvious exploit path
• a guaranteed crash or broken core flow

Major

Use when the change introduces:

• a clear functional regression in normal usage
• incorrect state transitions or data handling
• an unambiguous instruction-file violation that meaningfully changes behavior or reviewability
• a file pushed past ~1000 lines when the new code could be extracted (structural rubric loaded)
• ad-hoc feature logic scattered into shared code paths, making them harder to reason about (structural rubric loaded)

Minor

Use when the change introduces:

• a narrow but real bug
• a constrained edge-case regression
• a clearly missing but non-blocking regression or validation test
• a non-blocking instruction-file violation with clear scope
• a bespoke helper where a canonical utility already exists (structural rubric loaded)
• an unnecessary abstraction layer that adds indirection without clarity (structural rubric loaded)

Do not report

Drop the finding instead of assigning a low severity when it is:

• speculative
• stylistic
• pre-existing and unrelated to the diff
• likely to be caught automatically by lint or typecheck without extra reviewer value

Structural Quality Rubric

Unusually strict review focused on implementation quality, maintainability, and codebase health. Load when the user asks for a structural quality review, thermo-nuclear review, deep code quality audit, or when reviewing large diffs that touch module boundaries.

The core question is not "will this code break?" but "should this code exist in this form?"

Contents

• Core philosophy
• Non-negotiable standards
• Primary review questions
• What to flag aggressively
• Preferred remedies
• Approval bar
• Presumptive blockers
• Tone
• Anti-rationalizations

Core Philosophy

Push hard for ambitious structural simplification. Do not stop at "this could be a bit cleaner." Look for code judo moves — re-organizations that use the existing architecture more effectively and make the change dramatically simpler and more elegant. Prefer the solution that makes the code feel inevitable in hindsight. If there is a path to delete complexity rather than rearrange it, push hard for that path.

Rethink how to structure and implement the changes to meaningfully improve code quality without impacting behavior. Improve abstractions, modularity, reduce spaghetti code, improve succinctness and legibility. If there is a clear path to improving the implementation that involves restructuring some of the codebase, go for it.

Non-Negotiable Standards

1. Ambitious structural simplification

Look for opportunities to reframe the change so whole branches, helpers, modes, conditionals, or layers disappear entirely. If you see a path to delete complexity rather than rearrange it, push hard for that path. Prefer refactors that remove moving pieces altogether over refactors that merely spread the same complexity around.

2. 1000-line file threshold

Do not let a PR push a file from under 1000 lines to over 1000 lines without a very strong reason. Prefer extracting helpers, subcomponents, modules, or local abstractions instead of letting a file sprawl. If the diff crosses that threshold, explicitly flag it for decomposition. Only waive when there is a compelling structural reason and the resulting file is still clearly organized.

3. No spaghetti branching growth

Be highly suspicious of new ad-hoc conditionals, scattered special cases, or one-off branches inserted into unrelated flows. If a change adds random if-statements in unrelated places, treat that as a design problem, not a stylistic nit. Prefer pushing the logic into a dedicated abstraction, helper, state machine, policy object, or separate module.

4. Bias toward cleaning the design

If behavior can stay the same while the structure becomes meaningfully cleaner, push for the cleaner version. Do not rubber-stamp "it works" implementations that leave the codebase messier. Strongly prefer simplifications that remove moving pieces altogether over refactors that merely spread the same complexity around.

5. Direct, boring, maintainable over hacky or magical

Treat brittle, ad-hoc, or "magic" behavior as a code-quality problem. Be skeptical of generic mechanisms that hide simple data-shape assumptions. Flag thin abstractions, identity wrappers, or pass-through helpers that add indirection without buying clarity.

6. Type and boundary cleanliness

Question unnecessary optionality, unknown, any, or cast-heavy code when a clearer type boundary could exist. Prefer explicit typed models or shared contracts over loosely-shaped ad-hoc objects. If a branch relies on silent fallback to paper over an unclear invariant, ask whether the boundary should be made explicit instead.

7. Keep logic in the canonical layer

Call out feature logic leaking into shared paths or implementation details leaking through APIs. Prefer existing canonical utilities and helpers over bespoke one-offs. Push code toward the right package, service, or module instead of normalizing architectural drift.

8. Orchestration simplicity

Treat unnecessary sequential orchestration and non-atomic updates as design smells when the cleaner structure is obvious. If independent work is serialized for no good reason, flag it. If related updates can leave state half-applied, push for a more atomic structure. Do not over-index on micro-optimizations, but flag avoidable orchestration complexity that makes the implementation more brittle.

Primary Review Questions

For every meaningful change, ask:

Structural simplification:

• Is there a code judo move that would make this dramatically simpler?
• Can this change be reframed so fewer concepts, branches, or helper layers are needed?
• Does this improve or worsen the local architecture?

Complexity and branching:

• Did the diff add branching complexity where a better abstraction should exist?
• Did a previously cohesive module become more coupled, more stateful, or harder to scan?
• Are there repeated conditionals that signal a missing model or missing helper?
• Is the implementation direct and legible, or does it rely on special cases and incidental control flow?

Boundary and abstraction quality:

• Is this logic living in the right file and layer?
• Did this change enlarge a file or component past a healthy size boundary?
• Is this abstraction actually earning its keep, or is it just a wrapper?
• Is this logic living in the canonical layer, or did the diff leak details across a boundary?

Type contracts:

• Did the diff introduce casts, optionality, or ad-hoc object shapes that obscure the real invariant?
• Is this orchestration more sequential or less atomic than it needs to be?

What to Flag Aggressively

• A complicated implementation where a cleaner reframing could delete whole categories of complexity
• Refactors that move code around but fail to reduce the number of concepts a reader must hold in their head
• A file crossing 1000 lines due to the PR, especially if the new code could be split out
• New conditionals bolted onto unrelated code paths
• One-off booleans, nullable modes, or flags that complicate existing control flow
• Feature-specific logic leaking into general-purpose modules
• Generic "magic" handling that hides simple structure and makes the code harder to reason about
• Thin wrappers or identity abstractions that add indirection without simplifying anything
• Unnecessary casts, any, unknown, or optional params that muddy the real contract
• Copy-pasted logic instead of extracted helpers
• Narrow edge-case handling implemented in the middle of an already busy function
• Refactors that technically pass tests but make the code less modular or less readable
• "Temporary" branching that is likely to become permanent debt
• Bespoke helpers where the codebase already has a canonical utility for the job
• Logic added in the wrong layer or package when it should live somewhere more central
• Sequential async flow where obviously independent work could be simpler with parallel execution

Preferred Remedies

When you identify a structural problem, prefer suggestions like:

• Delete a whole layer of indirection rather than polishing it
• Reframe the state model so conditionals disappear instead of getting centralized
• Change the ownership boundary so the feature becomes a natural extension of an existing abstraction
• Turn special-case logic into a simpler default flow with fewer exceptions
• Extract a helper or pure function
• Split a large file into smaller focused modules
• Move feature-specific logic behind a dedicated abstraction
• Replace condition chains with a typed model or explicit dispatcher
• Separate orchestration from business logic
• Collapse duplicate branches into a single clearer flow
• Delete wrappers that do not meaningfully clarify the API
• Reuse the existing canonical helper instead of introducing a near-duplicate
• Make type boundaries more explicit so the control flow gets simpler
• Move the logic to the package, module, or layer that already owns the concept
• Parallelize independent work when that also simplifies the orchestration
• Restructure related updates into a more atomic flow when partial state would be harder to reason about

Do not be satisfied with "maybe rename this" feedback when the real issue is structural. Do not be satisfied with a merely cleaner version of the same messy idea if there is a plausible path to a much simpler idea.

Approval Bar

Do not approve merely because behavior seems correct. The bar is:

• No clear structural regression
• No obvious missed opportunity to make the implementation dramatically simpler when such a path is visible
• No unjustified file-size explosion
• No obvious spaghetti growth from special-case branching
• No obviously hacky or magical abstraction that makes the code harder to reason about
• No unnecessary wrapper, cast, or optionality churn obscuring the real design
• No clear architecture-boundary leak or avoidable canonical-helper duplication
• No missed opportunity for an obvious decomposition that would materially improve maintainability

Presumptive Blockers

Treat these as Must fix before push unless the author can justify them clearly:

• The diff preserves a lot of incidental complexity when there is a plausible code judo move that would delete it
• The diff pushes a file from below 1000 lines to above 1000 lines
• The diff adds ad-hoc branching that makes an existing flow more tangled
• The diff solves a local problem by scattering feature checks across shared code
• The diff adds an unnecessary abstraction, wrapper, or cast-heavy contract that makes the design more indirect
• The diff duplicates an existing helper or puts logic in the wrong layer when there is a clear canonical home
• The diff adds orchestration complexity that's clearly avoidable

Tone

Be direct, serious, and demanding about quality. Do not be rude, but do not soften major maintainability issues into mild suggestions. If the code is making the codebase messier, say so clearly. If the implementation missed an opportunity for a dramatic simplification, say that clearly too.

Effective phrases:

• "this pushes the file past 1k lines — can we decompose this first?"
• "this adds another special-case branch into an already busy flow — can we move this behind its own abstraction?"
• "this works, but it makes the surrounding code more spaghetti — let's keep the behavior and restructure the implementation"
• "this feels like feature logic leaking into a shared path — can we isolate it?"
• "this abstraction seems unnecessary — can we just keep the direct flow?"
• "why does this need a cast / optional here? can we make the boundary more explicit instead?"
• "this looks like a bespoke helper for something we already have — can we reuse the canonical one?"
• "i think there's a code judo move here — can we reframe this so these branches disappear?"
• "this refactor moves complexity around but doesn't really delete it — is there a way to make the model itself simpler?"

Anti-Rationalizations