engineering
Code Review Checklist
Systematic code review checklist covering correctness, security, performance, maintainability, and testing — with severity-based prioritization and actionable feedback templates.
code-reviewchecklistsecurityqualitypull-requests
Works well with agents
Works well with skills
$ npx skills add The-AI-Directory-Company/(…) --skill code-review-checklistcode-review-checklist/
api-endpoint-review.md
Markdown
| 1 | # Code Review: POST /api/v2/workspaces/:id/invitations |
| 2 | |
| 3 | **PR**: #1847 — Add workspace invitation endpoint |
| 4 | **Author**: @danielk |
| 5 | **Reviewer**: @sarahc |
| 6 | **Ticket**: TEAM-312 — Allow workspace admins to invite users by email |
| 7 | **Blast radius**: Workspace service, email service, user permissions |
| 8 | |
| 9 | --- |
| 10 | |
| 11 | ## First Pass: Intent |
| 12 | |
| 13 | This PR adds an endpoint for workspace admins to invite new members by email. It creates an invitation record, sends an email via the notification service, and handles the case where the invitee already has an account. The endpoint should NOT modify existing member roles or allow duplicate invitations. |
| 14 | |
| 15 | --- |
| 16 | |
| 17 | ## Correctness |
| 18 | |
| 19 | - [x] Logic matches the stated requirement |
| 20 | - [x] Edge cases handled: duplicate email, self-invitation, already-a-member |
| 21 | - [ ] **[Blocker]** Error path on line 47 returns `res.json({ error: "failed" })` with a 200 status code. The client cannot distinguish success from failure. Return `res.status(500).json({ error: "invitation_send_failed", message: "..." })`. |
| 22 | - [x] State mutations are atomic — invitation record and audit log are created in a transaction |
| 23 | - [ ] **[Major]** No check for the case where the workspace has reached its seat limit (plan allows 10 members). An admin on the free plan could send unlimited invitations. Add a seat count check before creating the invitation. |
| 24 | |
| 25 | ## Security |
| 26 | |
| 27 | - [x] User input is validated — email format checked via zod schema |
| 28 | - [ ] **[Blocker]** Authorization check uses `req.user.workspaceId` from the JWT but does not verify it matches `:id` in the URL path. A user in workspace A can send invitations for workspace B by crafting the URL. Compare `req.user.workspaceId === req.params.id` or use the existing `requireWorkspaceMember` middleware. |
| 29 | - [x] No secrets hardcoded or logged |
| 30 | - [x] SQL uses parameterized queries via Prisma |
| 31 | - [ ] **[Major]** The invitation token is generated with `Math.random().toString(36)`. This is cryptographically weak and predictable. Use `crypto.randomBytes(32).toString('hex')` instead. |
| 32 | - [x] Rate limiting already applied via the global API rate limiter (50 req/min) |
| 33 | |
| 34 | ## Performance |
| 35 | |
| 36 | - [x] Database queries use indexes — `invitations` table indexed on `(workspace_id, email)` |
| 37 | - [x] No N+1 queries — single insert operation |
| 38 | - [ ] **[Minor]** The endpoint queries `SELECT * FROM users WHERE email = ?` to check if the invitee exists. Only `id` and `email` are used. Select only needed columns to avoid transferring unnecessary data. |
| 39 | - [x] No unbounded data loading |
| 40 | |
| 41 | ## Maintainability |
| 42 | |
| 43 | - [ ] **[Minor]** The function `handleInvite` is 94 lines. Extract email-sending logic into a separate `sendInvitationEmail` function — it is reused in the "resend invitation" endpoint on line 201. |
| 44 | - [x] Names describe intent — `createWorkspaceInvitation`, `checkExistingMember` |
| 45 | - [ ] **[Nit]** Magic string `"pending"` on line 62 — extract to a constant `INVITATION_STATUS.PENDING` alongside the existing `INVITATION_STATUS.ACCEPTED` on line 15. |
| 46 | - [x] Complex permission logic has comments explaining the business rules |
| 47 | |
| 48 | ## Testing |
| 49 | |
| 50 | - [x] Happy path covered — admin creates invitation, record persists, email sent |
| 51 | - [ ] **[Major]** No test for the authorization bypass — add a test where user from workspace A attempts to create an invitation in workspace B and expects a 403. |
| 52 | - [ ] **[Minor]** No test for expired invitation re-send behavior. The ticket mentions invitations expire after 7 days, but no test verifies that re-sending resets the expiry. |
| 53 | - [x] Tests are deterministic — email service is mocked |
| 54 | - [x] Test names describe scenarios: `"returns 409 when email already invited"` |
| 55 | |
| 56 | ## Compatibility and Deployment |
| 57 | |
| 58 | - [x] Database migration adds `invitations` table — backward compatible, no drops |
| 59 | - [x] Endpoint is versioned under `/api/v2/` |
| 60 | - [ ] **[Minor]** No feature flag. Since this touches permissions and email sending, consider gating behind `ENABLE_WORKSPACE_INVITATIONS` for the initial rollout. |
| 61 | - [x] Email template path is environment-configured |
| 62 | |
| 63 | --- |
| 64 | |
| 65 | ## Summary |
| 66 | |
| 67 | **Verdict**: Request changes — 2 blockers must be fixed before merge. |
| 68 | |
| 69 | | Severity | Count | Key Findings | |
| 70 | |----------|-------|-------------| |
| 71 | | Blocker | 2 | Auth bypass via URL path mismatch; error response returns 200 | |
| 72 | | Major | 3 | No seat limit check; weak token generation; missing auth test | |
| 73 | | Minor | 4 | Column over-selection; extract email function; magic string; missing feature flag | |
| 74 | | Nit | 1 | Extract status constant | |
| 75 |