|
| 1 | +# REVIEW.md — Trigger.dev OSS |
| 2 | + |
| 3 | +Repo-specific signal for anyone (human or agent) reviewing a PR in this codebase. Calibrates what counts as critical, what to always check, and what to skip. |
| 4 | + |
| 5 | +## What makes a 🔴 Important finding here |
| 6 | + |
| 7 | +Reserve 🔴 for things that would page someone or block a rollback. In this codebase, that means: |
| 8 | + |
| 9 | +- **Rolling-deploy breakage.** Old and new versions of the webapp/supervisor run side-by-side during deploys. A change is broken if: |
| 10 | + - A Lua script's behavior changes for a given key set without versioning (rename the script with a behavior-descriptive suffix like `Tracked` rather than `V2` — both versions must coexist safely). |
| 11 | + - A Redis data shape used by both versions changes in place. New shapes need a new key namespace. |
| 12 | + - A migration is not backward-compatible with the prior image. |
| 13 | +- **Schema / migration safety.** Prisma migrations must be backward-compatible with the prior deploy. Adding NOT NULL without a default, dropping a column an old image still reads, renaming a column — all 🔴. |
| 14 | +- **ClickHouse migration ordering + idempotency.** Goose runs in strict mode in the deploy pipeline and refuses to apply a missing version below the current version — slotting a new file in below the latest already-applied version blocks the deploy. New ClickHouse migration files MUST use the next available number (`max(files in internal-packages/clickhouse/schema/) + 1`); if main has added migrations while you've been on a branch, renumber yours. DDL must also be idempotent (`ADD COLUMN IF NOT EXISTS`, `DROP COLUMN IF EXISTS`, `CREATE TABLE IF NOT EXISTS`, `ADD INDEX IF NOT EXISTS`) so a partial / `--allow-missing` apply elsewhere doesn't fail on retry. Either fault is 🔴 — both break test/prod deploys. Rules live in `internal-packages/clickhouse/CLAUDE.md`. |
| 15 | +- **Queue / concurrency correctness.** RunQueue, MarQS (V1, legacy), redis-worker — any change to enqueue / dequeue / locking semantics. Re-derive the invariant on paper before flagging or accepting. |
| 16 | +- **Missing index on a hot table.** New Prisma queries against `TaskRun`, `TaskRunExecutionSnapshot`, `JobRun`, `Project`, etc. must use an existing index. Check `internal-packages/database/prisma/schema.prisma` for the relevant `@@index` lines — don't guess and don't propose `EXPLAIN`. |
| 17 | +- **Recovery-path queries.** Any `TaskRun.findFirst` / `findMany` added to a schedule, run-recovery, or restart loop. Recovery fan-outs (Redis crash, restart storms) turn "rare indexed query" into a DB incident. 🔴 even if indexed. |
| 18 | +- **Aggregations on hot tables.** No `COUNT` / `GROUP BY` on `TaskRun` or other tables that can reach billions of rows. Use Redis or ClickHouse for counts. |
| 19 | +- **Prod Redis blast-radius.** New code paths that `SCAN` with broad patterns (`*foo*`) on prod-shaped Redis, or `EVAL` Lua with `SCAN` loops inside. Both are 🔴. |
| 20 | +- **`@trigger.dev/core` direct import** from anywhere outside the SDK package. Always import from `@trigger.dev/sdk`. Core direct imports are 🔴 — they break the public API contract. |
| 21 | +- **Heavy execute-deps imported into request-handler bundles.** Specifically `chat.handover` and similar split-bundle entry points must not transitively import the agent task's execute path. Watch for new imports added at module top-level of route files. |
| 22 | +- **V1 engine code modified in a "V2 only" PR.** The `apps/webapp/app/v3/` directory contains both. If the PR description says V2-only but it touches `triggerTaskV1`, `cancelTaskRunV1`, `MarQS`, etc. — 🔴. |
| 23 | + |
| 24 | +## Performance (always review) |
| 25 | + |
| 26 | +Every PR gets a performance pass — not just the ones that look perf-sensitive. For each new query or unit of work, weigh three things: (a) the size of the table it hits, (b) whether it sits on a hot path, (c) whether the data it walks can be deep or wide (run trees, batches). The 🔴 bullets above on indexes, recovery-path queries, aggregations, and Redis `SCAN` are part of this pass — the rest below extends it. |
| 27 | + |
| 28 | +**Treat these tables as large — no scans, no `COUNT` / `GROUP BY`, no unbounded fetch:** |
| 29 | + |
| 30 | +- **Postgres — the `TaskRun` family:** `TaskRun`, `TaskRunExecutionSnapshot`, `Waitpoint`, `BatchTaskRun` and their join tables. Assume billions of rows. |
| 31 | +- **ClickHouse — `task_events_v1` / `task_events_v2`.** Partitioned by `toDate(inserted_at)`; `ORDER BY (environment_id, toUnixTimestamp(start_time), trace_id)`. Note `span_id` / `parent_span_id` are NOT in the sort key — span-id lookups can't skip granules, only `environment_id` + a `start_time` window can. |
| 32 | + |
| 33 | +**Hot paths — extra scrutiny on any added query or work:** |
| 34 | + |
| 35 | +- **Trigger + batch trigger** (`triggerTask.server.ts`, `batchTriggerV3.server.ts`) — see `apps/webapp/CLAUDE.md`; do not add DB queries to these. |
| 36 | +- **Dequeue / RunQueue** (`dequeueSystem.ts`, run-queue read/lock paths) — runs on every execution. |
| 37 | +- **Execution-snapshot creation in the run engine** — any engine function that writes a `TaskRunExecutionSnapshot` runs per state transition; a new query there multiplies by run volume. |
| 38 | +- **OTEL ingestion** (`otel.v1.traces.ts`, `otel.v1.logs.ts`) — write volume scales with customer span counts. |
| 39 | +- **Trace + run-list reads** (trace view, run list, span detail) — read paths over the large tables above. |
| 40 | + |
| 41 | +**Deep / wide shapes — one run can explode into a huge tree or batch; code that walks them is the trap:** |
| 42 | + |
| 43 | +- Trace span subtrees (deeply nested child runs → deep span trees). |
| 44 | +- Batch + parent/child fan-out (one run triggers thousands of children). |
| 45 | +- Waitpoint / run-dependency chains. |
| 46 | +- Tag / attribute many-to-many joins against the run/event tables. |
| 47 | + |
| 48 | +**Anti-patterns (severity):** |
| 49 | + |
| 50 | +- **Per-level fan-out that re-scans a large table once per tree depth** → 🔴. A BFS issuing one query per level (e.g. `parent_span_id IN {thisLevel}`) re-reads the same granules D times for a depth-D tree. Prefer one windowed query + an in-memory tree build. |
| 51 | +- **Dropping the partition-pruning predicate** — `inserted_at` for ClickHouse, the `createdAt` window for partitioned Postgres — to "widen" a lookup → 🔴. Without it the query scans every partition. Keep a bounded window even for ancestor / backfill lookups. |
| 52 | +- **Unbounded `IN (...)` built from a result set** (a BFS frontier, a batch's child ids) → 🟡. It can reach the row cap (`MAXIMUM_TRACE_SUMMARY_VIEW_COUNT` defaults to 25k). Cap or chunk to ≤1–2k ids per query. |
| 53 | +- **Sequential per-level round-trips** where one recursive or windowed query would do → 🟡. N levels = N round-trip latencies stacked. |
| 54 | +- **Replacing a single bounded query with a multi-query walk for _every_ call** (not just a rare fallback) → 🔴 on a hot read path, 🟡 elsewhere. Keep the cheap single-query path; branch into the expensive walk only when the cheap one comes up short. |
| 55 | + |
| 56 | +## Always check |
| 57 | + |
| 58 | +- **Tests use testcontainers, not mocks.** Vitest with `redisTest` / `postgresTest` / `containerTest` from `@internal/testcontainers`. Any new `vi.mock(...)` on Redis, Postgres, BullMQ, or other infra is wrong here — 🔴 if added in production-path tests, 🟡 if isolated unit test. |
| 59 | +- **User-facing public-package changes have a changeset.** `pnpm run changeset:add` produces `.changeset/*.md`. Changesets are user-facing release notes, not a catalog of every change: required when a `packages/*` or `integrations/*` change is something a user would notice or act on, skipped for internal-only changes, refactors, chores, and packages not consumed independently (e.g. `@trigger.dev/redis-worker`). Missing on a user-facing change → 🟡; missing on a breaking change → 🔴. Do not flag a missing note when the change is not user-facing. |
| 60 | +- **User-facing server-only changes have `.server-changes/*.md`.** Required for user-facing `apps/webapp/`, `apps/supervisor/` edits in a PR with no package or integration change that requires a changeset; skip internal-only or admin-only changes, refactors, and chores. Body should be 1-2 sentences (it has to fit as one bullet in a future changelog). Missing on a user-facing change → 🟡. |
| 61 | +- **Lua script naming.** Coexisting scripts use behavior-descriptive suffixes (`Tracked`), never `V2`. Old name must keep working until the next deploy clears it. |
| 62 | +- **RunQueue payload shape.** V2 run-queue payload's `projectId` is consumed by `workerQueueResolver` for override matching. If a PR drops it from the payload, 🔴. |
| 63 | +- **`safeSend` scope.** Defensive IPC wrappers belong on loop / interval / handler contexts, not one-shot terminal sends. If the PR adds `safeSend` to a single terminal call for consistency, 🟡 with a "remove this" suggestion. |
| 64 | +- **Zod version.** Pinned to `3.25.76` monorepo-wide. New package adding zod with a different version or range — 🔴. |
| 65 | + |
| 66 | +## Skip (do NOT flag) |
| 67 | + |
| 68 | +- Anything oxfmt / oxlint catches. CI enforces both via the `code-quality` check. |
| 69 | +- TypeScript style preferences (`type` vs `interface`) — already covered by repo standards. |
| 70 | +- Test coverage exhortations as a generic suggestion. Only flag missing tests when a specific code path is genuinely untested and the path has prior incidents. |
| 71 | +- `agentcrumbs` markers (`// @crumbs`, `// #region @crumbs`) and `agentcrumbs` imports — these are temporary debug instrumentation stripped before merge. |
| 72 | +- `// removed comments for removed code`, renamed `_unused` vars, re-exported types as "backwards compatibility shims" — also covered by repo standards. |
| 73 | +- Suggestions to "add error handling" without naming a specific scenario that breaks. |
| 74 | +- Documentation prose nitpicks in `docs/*` MDX files unless factually wrong. |
| 75 | + |
| 76 | +## Things V1/legacy that should NOT block a PR |
| 77 | + |
| 78 | +The `apps/webapp/app/v3/` directory name is misleading — most code there is V2. Only specific files are V1-only legacy: `MarQS` queue, `triggerTaskV1`, `cancelTaskRunV1`, and a handful of others (see `apps/webapp/CLAUDE.md` for the exact list). Don't flag "you should refactor this to use V2" on those — they're frozen. |
| 79 | + |
| 80 | +## Confidence calibration for this repo |
| 81 | + |
| 82 | +The most common false-positive pattern: speculating about race conditions in code paths the agent doesn't have runtime visibility into. If the only evidence is "this *could* race", drop it. If you can point to a specific interleaving with file:line for each step, surface it. |
0 commit comments