Pull Request Review

Review pull requests thoroughly before merging.

What to Check

• Correctness: Does the code work as described? Are there logic errors or missing edge cases?
• Code Quality: Flag duplicated logic, overly complex functions, or poor naming.
• Tests: Do tests cover the new functionality and edge cases? Flag untested branches.
• Security: Look for specific red flags:
  • Hardcoded secrets or API keys (e.g., strings matching sk-, AKIA, password =)
  • SQL/command injection via unsanitised user input (e.g., query = "SELECT * FROM users WHERE id = " + user_input)
  • Missing authentication/authorisation checks on new endpoints
  • Unvalidated redirects or exposed internal paths
• Performance: Flag N+1 queries, unbounded loops over large datasets, or missing indexes on queried columns.

Review Process

1. Read the PR description to understand intent and scope
2. Review each changed file in the diff
3. Identify bugs, anti-patterns, and security issues (see above)
4. Check test coverage for new and changed code paths
5. Leave inline comments on specific lines (see format below)
6. Validation checkpoint: Confirm that every changed file has been reviewed and every flagged issue has been recorded before proceeding — do not write the summary until this check passes
7. Write a summary with overall assessment and approval status

Inline Comment Format

When flagging an issue on a specific line, use this format:

**[FILE path/to/file.py, Line 42]** — SEVERITY: Brief issue label
Explanation of the problem and why it matters.
Suggestion: `corrected_code_snippet_here`

Example — security issue:

**[FILE app/routes/users.py, Line 17]** — HIGH: SQL Injection Risk
User input is concatenated directly into the query string, allowing an attacker to manipulate the query.
Suggestion: Use a parameterised query instead:
`cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))`

Example — code quality issue:

**[FILE utils/parser.py, Line 88]** — LOW: Magic Number
The value `86400` is used without explanation.
Suggestion: Replace with a named constant: `SECONDS_IN_A_DAY = 86400`

Worked Example

Given a diff like:

+ def get_user(user_id):
+     query = "SELECT * FROM users WHERE id = " + user_id
+     return db.execute(query)

Expected review output:

[FILE db/users.py, Line 3] — HIGH: SQL Injection Risk
user_id is concatenated directly into the SQL string. A malicious value like 1 OR 1=1 would expose all users.
Suggestion: return db.execute("SELECT * FROM users WHERE id = %s", (user_id,))

Summary: One blocking security issue found. No tests were added for this function. Please address the injection vulnerability and add unit tests before merging.

REQUEST_CHANGES

Output Format

End every review with a summary paragraph followed by one of:

• APPROVE: Code is ready to merge
• REQUEST_CHANGES: One or more blocking issues must be addressed
• COMMENT: Providing non-blocking feedback without a merge decision