Skip to main content

PR review

Use this checklist. It captures the load-bearing things that are easy to miss.

Always check​

  • Branch name follows convention (feat/, fix/, etc.).
  • Description explains why, not just what.
  • No .env*, dist/, or secrets committed.
  • No console.log left in production code paths.
  • No process.env access outside main.ts / migrate.ts / seed.ts.

Schema and migrations​

  • New tables or columns? Migration file present in drizzle/?
  • Migration SQL inspected manually? Statements look minimal?
  • No drizzle-kit push artifact (snapshot-only changes without an SQL file).
  • FKs declared with onDelete policy?
  • Unique constraints in place where stated by the API design?
  • auth.ts schema NOT hand-edited? Regenerated via pnpm db:auth-schema?

Endpoints​

  • @ApiTags(...) set on every new controller.
  • @ApiProperty() on every public DTO field (otherwise spec is incomplete).
  • Guard applied for protected endpoints (PlatformAdminGuard etc.).
  • Bearer + cookie both accepted where appropriate.
  • Pagination follows cursor pattern, not offset.
  • Error envelope shape consistent (code, message, optional details).

Auth / RBAC​

  • If new action gated, added to statement in src/auth/permissions.ts.
  • Frontend src/lib/permissions.ts mirror updated in the SAME PR.
  • No DB call from guards (they must trust the session).

Frontend​

  • Components using AgentCard pass a unique layoutIdPrefix.
  • No hand-edits to src/components/ui/ (shadcn-managed).
  • Tailwind classes use design tokens (hsl(var(--primary))), not literal hex values.
  • themeFor(category) + defaultsFor(category) paired when rendering agent UI.
  • Live API used for new data displays (don't add new mock arrays).

Stripe​

  • Paid tier endpoints handle STRIPE_SECRET_KEY absence (throw 503, don't crash).
  • Webhook handler is idempotent (relies on stripe_events.stripeEventId UNIQUE).
  • Signature verification is in place.
  • Raw body read, not JSON-parsed body.

OAuth​

  • New scopes (if added) documented in OAuth Federation.
  • redirectUris updates also flow into oauth_clients.redirect_uris.
  • PKCE required; no fallback paths added.

Tests​

  • Unit test or integration test for new business logic.
  • Tests don't depend on a specific machine clock (use injected clocks).
  • Tests don't hit real Stripe / PostHog (mock them).

Docs​

  • If the PR adds/removes endpoints, openapi.json updated and docs site pnpm refresh-api run.
  • If the PR changes a load-bearing convention, CONVENTIONS.md / this site updated.
  • If the PR introduces a new concept, a Concepts page added.

Performance​

  • No N+1 queries in service methods.
  • DB transactions used for multi-write paths.
  • No unbounded list queries (always LIMIT).

Security​

  • No new endpoints leak PII in audit logs (request bodies excluded).
  • No new endpoints accept user-supplied URLs without allow-list (open redirect).
  • No new logs of secrets, tokens, or full webhook payloads.

How to disagree​

If reviewer and author disagree:

  • For load-bearing decisions, escalate. Don't merge.
  • For style preferences, defer to the author (it's their PR).
  • For load-bearing conventions documented above, the docs win — update the docs if the convention should change.