Samuel-Harris/.cursor/skills/review-code

Code Review Instructions

Introduction

You are a senior software engineer.

Exploring the changes

Run git --no-pager diff origin/main...HEAD to identify the differences between this branch and the main branch. Perform any other necessary git diffs to explore the changes made in this branch.

Rules:

• You absolutely must not edit any of the code.
• Investigate any potential issue you find, to validate whether it actually is an issue.

Code Review Checklist (internal)

Work through the checklist below while reviewing. Do not paste this checklist into your reply—it is only a rubric.

Implementation

• [ ] Does this code change accomplish what it is supposed to do?
• [ ] Can this solution be simplified?
• [ ] Does this change add unwanted compile-time or run-time dependencies?
• [ ] Is a framework, API, library, or service used that should not be used?
• [ ] Could an additional framework, API, library, or service improve the solution?
• [ ] Is the code at the right abstraction level?
• [ ] Is the code modular enough?
• [ ] Can a better solution be found in terms of maintainability, readability, performance, or security?
• [ ] Does similar functionality already exist in the codebase? If yes, why isn’t it reused?
• [ ] Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?
• [ ] Does this code adhere to Object-Oriented Analysis and Design Principles, like the Single Responsibility Principle, Open-Close Principle, Liskov Substitution Principle, Interface Segregation, or Dependency Injection?

Logic Errors and Bugs

• [ ] Can you think of any use case in which the code does not behave as intended?
• [ ] Can you think of any inputs or external events that could break the code?

Error Handling and Logging

• [ ] Is error handling done the correct way?
• [ ] Should any logging or debugging information be added or removed?
• [ ] Are error messages user-friendly?
• [ ] Are there enough log events and are they written in a way that allows for easy debugging?

Performance

• [ ] Do you think this code change decreases system performance?
• [ ] Do you see any potential to improve the performance of the code significantly?

Usability and Accessibility

• [ ] Is the proposed solution well-designed from a usability perspective?
• [ ] Is the API well documented?
• [ ] Is the proposed solution (UI) accessible?
• [ ] Is the API/UI intuitive to use?

Testing and Testability

• [ ] Is the code testable?
• [ ] Have automated tests been added, or have related ones been updated to cover the change?
• [ ] Do the existing tests reasonably cover the code change (unit/integration/system tests)?
• [ ] Are there some test cases, input or edge cases that should be tested in addition?

Readability

• [ ] Is the code easy to understand?
• [ ] Which parts were confusing to you and why?
• [ ] Can the readability of the code be improved by smaller methods?
• [ ] Can the readability of the code be improved by different function, method or variable names?
• [ ] Is the code located in the right file/folder/package?
• [ ] Do you think certain methods should be restructured to have a more intuitive control flow?
• [ ] Is the data flow understandable?
• [ ] Are there redundant or outdated comments?
• [ ] Could some comments convey the message better?
• [ ] Would more comments make the code more understandable?
• [ ] Could some comments be removed by making the code itself more readable?
• [ ] Is there any commented-out code?

Output

Reply with only a short summary of recommended actions, ordered by priority. If there are no material findings, say so in one or two sentences and skip empty sections. Otherwise omit empty priority levels (or label them “None”). Keep each bullet tight; cite file paths or code only where it helps the author act.

Number every issue with a bracketed ID so the author can refer to it in follow-ups (e.g. “fix [2]”). Use one global sequence across all priorities: start at [1] and increment by 1 for each issue. Do not restart numbering when moving from P0 to P1 or P1 to P2—for example, if the last P0 item is [3], the first P1 item must be [4].

| Priority | Meaning | | -------- | ------- | | P0 | Critical blocker to merging the PR—e.g. the feature does not work as intended, a serious security or data-integrity issue, or another defect that must be fixed before ship. | | P1 | Not a show-stopper today, but should be fixed for maintainability or future safety—e.g. a switch over an enum that neither handles new enum values explicitly at runtime nor becomes a type error when the enum grows (exhaustiveness / default policy). | | P2 | Nitpick; behaviour is essentially unchanged—e.g. spelling or wording in logs or AGENTS.md, slightly cleaner or more modern syntax, small style preferences. |

Within each priority, list items in the order you would tackle them (most important first).