|
| 1 | +# GitHub Copilot Review Instructions |
| 2 | + |
| 3 | +This is an Nx monorepo. Source code lives under `packages/`. Key packages: |
| 4 | + |
| 5 | +- `packages/server/api` — Fastify API server |
| 6 | +- `packages/server/shared` — shared server utilities (logging, caching, encryption) |
| 7 | +- `packages/engine` — workflow execution engine |
| 8 | +- `packages/react-ui` — React 18 frontend (Vite + TailwindCSS) |
| 9 | +- `packages/shared` — shared types/utilities |
| 10 | +- `packages/blocks/` — 50+ integration blocks (AWS, Azure, Slack, etc.) |
| 11 | +- `packages/ui-components` — reusable component library |
| 12 | + |
| 13 | +--- |
| 14 | + |
| 15 | +Review every pull request from the five perspectives below. For each perspective: |
| 16 | + |
| 17 | +- Only comment on issues relevant to the current diff |
| 18 | +- Avoid repeating the same feedback across sections |
| 19 | +- Prefer actionable comments with specific examples |
| 20 | +- Distinguish between **required fixes** and **optional improvements** |
| 21 | +- Avoid nitpicks unless they affect readability, correctness, security, or maintainability |
| 22 | + |
| 23 | +--- |
| 24 | + |
| 25 | +## 1. Experienced Software Developer |
| 26 | + |
| 27 | +Review code readability, maintainability, naming, structure, and simplicity. |
| 28 | + |
| 29 | +- Identify duplicated logic, unnecessary complexity, and places where abstractions could be improved |
| 30 | +- Suggest idiomatic TypeScript/JavaScript patterns where applicable |
| 31 | +- Point out edge cases and assumptions that may make the code fragile |
| 32 | +- Flag unclear variable or function names, overly long functions, and missing or misleading comments |
| 33 | + |
| 34 | +## 2. Security Reviewer |
| 35 | + |
| 36 | +Look for security risks in the diff. |
| 37 | + |
| 38 | +- Unsafe input handling, injection risks (SQL, shell, eval), and XSS vectors |
| 39 | +- Secrets, tokens, or credentials hardcoded or logged |
| 40 | +- Missing or insufficient authorization checks on new API routes |
| 41 | +- User-controlled data reaching sensitive operations |
| 42 | +- Insecure defaults, overly permissive CORS/CSP, or unsafe dependency usage |
| 43 | +- Suggest safer alternatives with concrete examples |
| 44 | + |
| 45 | +## 3. Performance Reviewer |
| 46 | + |
| 47 | +Identify performance and reliability concerns — only when meaningful, not as premature optimization. |
| 48 | + |
| 49 | +- Inefficient loops, unnecessary database or API calls within loops, and N+1 query patterns |
| 50 | +- Avoidable memory allocations or large in-memory data processing |
| 51 | +- Blocking operations in async contexts |
| 52 | +- Scalability concerns for data or traffic growth |
| 53 | +- Suggest improvements only when the gain is clear and the change is practical |
| 54 | + |
| 55 | +## 4. Testing Reviewer |
| 56 | + |
| 57 | +Assess whether the change has sufficient test coverage. |
| 58 | + |
| 59 | +- Flag missing unit, integration, or edge-case tests for non-trivial logic |
| 60 | +- Highlight untested error paths, boundary conditions, permission checks, and regression risks |
| 61 | +- Flag tests that only assert `toBeTruthy()` or `toBeDefined()` without meaningful assertions |
| 62 | +- Prefer concrete missing test scenarios (e.g., "no test covers the case where X is null") over vague suggestions |
| 63 | + |
| 64 | +## 5. Product / User Impact Reviewer |
| 65 | + |
| 66 | +Consider how the change affects users and existing behavior. |
| 67 | + |
| 68 | +- Behavior changes that could break existing workflows or integrations |
| 69 | +- Backwards compatibility risks with stored data, API contracts, or configuration |
| 70 | +- Error messages that are unclear or unhelpful to end users |
| 71 | +- Changes that should be documented, communicated, or behind a feature flag |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +## Review Summary |
| 76 | + |
| 77 | +End every review with a summary comment containing: |
| 78 | + |
| 79 | +1. A brief list of all issues found, grouped by severity: |
| 80 | + - **Blocking** — must be fixed before merge (security issues, broken logic, missing auth, data loss risks) |
| 81 | + - **Non-blocking** — recommended improvements (readability, test gaps, minor issues) |
| 82 | +2. A clear merge recommendation: |
| 83 | + - **Ready to merge** |
| 84 | + - **Merge after addressing non-blocking comments** |
| 85 | + - **Do not merge** (only when blocking issues are present) |
0 commit comments