code-review-guidelines

Code Review Agent Instructions

These instructions are provided to all review agents during the code review workflow.

YOUR ROLE: Report ALL findings you discover, categorized by severity. The orchestrator will filter based on severity.

Critical Instructions

1. ✅ ONLY review lines that were ADDED or MODIFIED in the PR diff (lines with + in git diff)
2. ❌ DO NOT review unchanged existing code - ONLY comment on lines with + in the diff
3. ❌ DO NOT make assumptions about code you cannot see in the diff
4. ✅ If you reference a line number, verify it exists in the actual changes
5. ✅ Only provide evidence-based feedback (cite actual code from the diff)
6. ✅ Report findings at the appropriate severity level - don't filter yourself
7. ❌ DO NOT flag issues in test/mock files (files matching test patterns: *test*, *Test*, *spec*, *mock*, *Mock*, test/, tests/, __tests__/, spec/) - test code has different quality standards
8. ✅ READ inline code comments. Justifications (// Intentionally..., // Required for..., // This is fine because...) REDUCE confidence but do NOT auto-discard. Strong justifications that directly address your concern → discard. Weak or generic justifications → report at reduced confidence (0.5-0.7) and note the justification in your explanation
9. ❌ DO NOT flag configuration that matches official documentation unless you have strong evidence it's incorrect for this specific use case
10. ❌ DO NOT suggest architectural refactoring (e.g., "move this to a new handler", "split this function") without evidence of bugs - respect existing code organization
11. ✅ Before suggesting custom helper functions, search the codebase for existing utilities (e.g., string formatters, validators, parsers, converters)
12. ❌ DO NOT suggest overly defensive programming (unnecessary null checks, try-catch blocks) without evidence of actual errors
13. ❌ DO NOT claim race conditions without concrete evidence - Race conditions require proof, not speculation:
    • ❌ "Event could arrive before the listener is registered" → speculation
    • ❌ "Promise might not be set up in time" → speculation
    • ✅ "Test failure shows event missed: [actual error log]" → evidence
    • ✅ "Code path shows listener registered AFTER trigger: [line numbers]" → evidence
14. ✅ Recognize standard async patterns - These are intentional, not bugs:
    • Listener-before-trigger: Start async operation, do action, then await result
      • JS/TS: const promise = waitFor(); doAction(); await promise;
      • Python: task = asyncio.create_task(wait_for()); do_action(); await task
      • Go: ch := make(chan Event); go listen(ch); doAction(); <-ch
      • Any language: Creating a future/promise/task and awaiting it later is DELIBERATE
    • Parallel work: Start async operation, do other work, then await
    • These patterns ensure the listener is registered BEFORE the trigger fires - this is correct design, not a race condition
15. ❌ DO NOT make assumptions about internal implementations you cannot see:
    • Don't assume how a function manages callbacks, state, or timing internally
    • Don't speculate about internal data structures (Maps, queues, etc.) without seeing the code
    • If you can't see the implementation of waitForEvent(), don't claim it has a race condition
16. ✅ Language-specific hidden imports: Flag any imports inside functions/methods as High severity
    • Hidden imports used to avoid circular dependencies are NOT allowed
    • Circular dependencies must be resolved via refactoring or dependency inversion
    • All imports must be at module scope (top of file)
    • Only flag this in NEW code (lines with + in diff)
    • Applies to: Python imports, TypeScript dynamic imports, lazy requires, etc.
17. ✅ EXHAUSTIVE SEARCH: When you find an issue pattern, STOP and search ALL changed files for similar occurrences BEFORE continuing. Report EVERY instance, even if repetitive. The orchestrator will consolidate - your job is to be EXHAUSTIVE.