feat(webapp): mollifier API GET read-fallback — synthetic primitives + route wiring#3755
Conversation
|
WalkthroughThis pull request implements a mollifier API read-fallback that serves synthetic run, trace, and span responses for runs present only in the mollifier buffer. It adds synthetic builders (span/trace and UI SpanRun), integrates a Postgres-first lookup with mollifier fallback in the retrieve presenter, updates /trace, /spans/:spanId, and /events routes to branch on resolved source and return synthetic bodies for buffered runs, provides redirect-info reconstruction for buffered snapshots, and adds comprehensive Vitest coverage. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
31f4726 to
b05929b
Compare
1838229 to
af0aeeb
Compare
b05929b to
b89da52
Compare
0919f7a to
f36c576
Compare
74fdf6d to
c6fa61f
Compare
047b240 to
e57bc5e
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
242ba73 to
6a8404d
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f4131eb to
d8f6cf7
Compare
6a8404d to
bc9f4e2
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d8f6cf7 to
796a2c0
Compare
bc9f4e2 to
637e8c0
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
796a2c0 to
b139391
Compare
637e8c0 to
65219db
Compare
b139391 to
d153042
Compare
65219db to
ccdcd9c
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0753300 to
a1e1ad8
Compare
…utes + align workerQueue default Addresses three Devin review findings on PR #3755: - api.v1.runs.\$runId.spans.\$spanId.ts and api.v1.runs.\$runId.trace.ts: the buffered-run response branch hardcoded isError:false and only checked CANCELED for isPartial, so a FAILED buffered run rendered as "still in progress" — SDK consumers would poll forever. Now derives both flags from CANCELED and FAILED, matching syntheticTrace.server.ts. - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer: workerQueue defaulted to "main" while syntheticSpanRun.server.ts uses "". The API response's `region` is sourced via `run.workerQueue || undefined`, so "main" was advertising a region the run hadn't yet been assigned to. Aligned to "" so unassigned buffered runs coerce to region: undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ae52e5e to
0afa415
Compare
…ns in events endpoint `api.v1.runs.$runId.events.ts` calls `ApiRetrieveRunPresenter.findRun`, which now returns a buffered fallback. The route then proceeds to `getRunEvents` against ClickHouse with `run.traceId` (empty string for buffered runs) and `run.taskEventStore` (`"taskEvent"` default) — a guaranteed-empty round trip, since the mollifier gate intercepts BEFORE any trace event is written. Devin's analysis on PR #3755 flagged this as a wasted ClickHouse round trip per request hitting the events endpoint with a buffered runId. Add an `isBuffered: boolean` flag to `FoundRun`. The PG branch of `findRun` sets `isBuffered: false` on the spread Prisma row; the buffered branch's `synthesiseFoundRunFromBuffer` sets `isBuffered: true`. The events route short-circuits on the flag and returns `{ events: [] }` with status 200, matching the semantically-correct behaviour without the ClickHouse hop. Gating on the explicit flag (rather than probing surrogates like `traceId === ""`) keeps the contract clear for future callers that need the same "PG-only" gate — they can introspect the same field. Includes a unit test in `mollifierSynthesiseFoundRun.test.ts` pinning `isBuffered: true` on the synth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ route wiring Synthesise QUEUED/FAILED responses from the mollifier buffer when a TaskRun row hasn't landed in Postgres yet. Wires the synthesis into: - ApiRetrieveRunPresenter - v1 trace GET route - v1 spans GET route - attempts route gains a GET loader (fixes pre-existing Remix 400) Stacked on the trigger-time decisions PR. The readFallback infra itself lives on the trigger PR (consumed by IdempotencyKeyConcern); this PR adds the route-level synthetic-rendering primitives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the ad-hoc \`as Record<string, unknown>\` + \`typeof === "string"\` checks in \`findBufferedRunRedirectInfo\` with a Zod \`safeParse\` against a schema for the subset of fields the redirect needs (envSlug / projectSlug / orgSlug / optional spanId). Wrong-typed or missing fields now collapse into a single parse-fail branch that logs the structured issue list and returns null. Adds a regression test for the structural-vs-typeof distinction: \`environment.slug: 42\` (number) is now rejected, where the previous \`typeof slug === "string"\` chain would silently accept any string- typed value but had no defence against shape drift in other fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l enum \`SyntheticRun.machinePreset\` is a plain string sourced from the mollifier snapshot, but \`SpanRun.machinePreset\` is the typed \`MachinePresetName\` enum (micro / small-1x / small-2x / medium-1x / medium-2x / large-1x / large-2x). The direct assignment failed \`tsc --noEmit\` and CI typecheck. Validate via \`MachinePresetName.safeParse\` and collapse unknown values to \`undefined\` so a stale preset returned by the buffer doesn't bleed into the UI as a typed-but-unknown value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The synthetic SpanRun/trace builders for buffered runs hardcoded non-terminal state, so a CANCELED or FAILED buffered run rendered as a healthy in-progress run: - syntheticSpanRun: FAILED now maps to SYSTEM_FAILURE (matching ApiRetrieveRunPresenter.bufferedStatusToTaskRunStatus); isFinished is true for CANCELED/FAILED; isError is true for FAILED; the error block is synthesised as STRING_ERROR and statusReason carries the message. - syntheticSpanRun: drop the empty-string spanId/taskIdentifier relationship stubs (blank task name + misleading `?span=` jump) since the snapshot only carries friendly IDs. - syntheticTrace: FAILED now renders as an errored, non-partial, "failed" root span instead of executing/partial. CANCELED stays "completed", matching RunPresenter's derivation. - tests: cover the CANCELED and FAILED terminal paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…truction
Addresses the higher-confidence read-fallback review findings:
- attempts GET loader: rebuilt on createLoaderApiRoute so it matches the
sibling read routes — accepts JWTs with run/task/tag/batch resource
scoping (was bare authenticateApiRequest, rejecting PUBLIC_JWT and doing
no scope check), and 404s with `x-should-retry: true` so SDK pollers keep
retrying a not-yet-materialised run instead of giving up.
- batch reconstruction: the snapshot embeds the batch as `{ id, index }`
(engine.trigger shape), but readFallback read a non-existent flat
`batchId`, so SyntheticRun.batchId was always undefined. Read it from
`snapshot.batch.id` (the internal cuid). synthesiseFoundRunFromBuffer now
populates `batch` from it, and the spans/trace buffer-path authorization
pushes the batch resource — so batch-scoped JWTs authorise against
buffered runs and the retrieve response reports the correct batchId.
- metadata: coerce a non-string buffered metadata defensively (JSON
stringify + warn) instead of silently dropping to null, mirroring
synthesisePayload. In practice metadata is always a string, so this is a
no-op guard, but it surfaces format drift to ops.
- tests: cover batchId extraction from the nested batch object and its
absence for non-batched runs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comments referenced internal phase labels ("Phase A2", "Phase A5") from
the development plan rather than describing what the code does. Replaced
with self-contained prose; the surrounding explanations were already
correct and are preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…derivation syntheticTrace.server.ts shipped without a test file; this adds one, covering the identity-field passthrough, taskIdentifier-and-spanId defaults, the three rootSpanStatus branches (QUEUED→executing, CANCELED→completed, FAILED→failed) with their isPartial/isError/isCancelled flags, the 1ms duration floor, rootStartedAt mapping, and the single-span trace shape (empty events/timelineEvents, empty linkedRunIdBySpanId, undefined overridesBySpanId/queuedDuration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…utes + align workerQueue default Addresses three Devin review findings on PR #3755: - api.v1.runs.\$runId.spans.\$spanId.ts and api.v1.runs.\$runId.trace.ts: the buffered-run response branch hardcoded isError:false and only checked CANCELED for isPartial, so a FAILED buffered run rendered as "still in progress" — SDK consumers would poll forever. Now derives both flags from CANCELED and FAILED, matching syntheticTrace.server.ts. - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer: workerQueue defaulted to "main" while syntheticSpanRun.server.ts uses "". The API response's `region` is sourced via `run.workerQueue || undefined`, so "main" was advertising a region the run hadn't yet been assigned to. Aligned to "" so unassigned buffered runs coerce to region: undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…drop scaffolding GET on attempts Two related cleanups on the mollifier read surface: 1. Extract the buffered-run response bodies for the spans-detail and trace endpoints into pure helpers (apps/webapp/app/v3/mollifier/syntheticApiResponses.server.ts: buildSyntheticSpanDetailBody, buildSyntheticTraceBody). The route bodies were carrying the only copy of the terminal-state derivation (CANCELED / FAILED → isError / isPartial / isCancelled) with no unit coverage; extracting them lets us pin the contract directly. The route files now just authenticate, resolve, validate the spanId, and forward — no body shape logic in routes. 2. Drop the GET loader on api.v1.runs.\$runParam.attempts.ts. It was added in this PR solely to fix a pre-existing Remix "no loader" 400 on a URL no SDK consumer was actually calling, and to give the mollifier-parity script a stable assertion target. The detailed attempt list lives on the v3 retrieve endpoint — the GET was scaffolding rather than product surface, and Devin's review flagged it as such. Reverted to action-only. Tests: 16 cases in apps/webapp/test/mollifierSyntheticApiResponses.test.ts covering QUEUED / CANCELED / FAILED for each body, plus identity and default-field passthrough. Pins the FAILED-terminal-state regression that shipped briefly with isPartial:true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pin synthesise contract with regression tests synthesiseFoundRunFromBuffer hardcoded `scheduleId: null`, which dropped the schedule field from the retrieve-API response for any scheduled trigger that landed in the mollifier buffer. Scheduled triggers go through the same TriggerTaskService path as API triggers and the gate doesn't bypass them, so the snapshot does carry scheduleId; the synthesis was just throwing it away. Forward `buffered.scheduleId ?? null` so resolveSchedule() can hydrate the schedule object from PG (the Schedule row exists before the trigger fires). Exported synthesiseFoundRunFromBuffer + the FoundRun type from the presenter and added apps/webapp/test/mollifierSynthesiseFoundRun.test.ts (16 cases). The test file pins the snapshot→FoundRun mapping that previously had no direct coverage — the new scheduleId forwarding plus earlier-session regressions (batch reconstruction, workerQueue default "", FAILED→SYSTEM_FAILURE status mapping, STRING_ERROR shape, defensive metadata coercion, idempotency defaults, execution-state zero defaults). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ns in events endpoint `api.v1.runs.$runId.events.ts` calls `ApiRetrieveRunPresenter.findRun`, which now returns a buffered fallback. The route then proceeds to `getRunEvents` against ClickHouse with `run.traceId` (empty string for buffered runs) and `run.taskEventStore` (`"taskEvent"` default) — a guaranteed-empty round trip, since the mollifier gate intercepts BEFORE any trace event is written. Devin's analysis on PR #3755 flagged this as a wasted ClickHouse round trip per request hitting the events endpoint with a buffered runId. Add an `isBuffered: boolean` flag to `FoundRun`. The PG branch of `findRun` sets `isBuffered: false` on the spread Prisma row; the buffered branch's `synthesiseFoundRunFromBuffer` sets `isBuffered: true`. The events route short-circuits on the flag and returns `{ events: [] }` with status 200, matching the semantically-correct behaviour without the ClickHouse hop. Gating on the explicit flag (rather than probing surrogates like `traceId === ""`) keeps the contract clear for future callers that need the same "PG-only" gate — they can introspect the same field. Includes a unit test in `mollifierSynthesiseFoundRun.test.ts` pinning `isBuffered: true` on the synth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PG-resident SYSTEM_FAILURE rows always have completedAt populated by the engine, so SDK consumers that stop polling when isCompleted=true and then read finishedAt see a real timestamp. The buffer-synth path was returning completedAt=null for FAILED (only the CANCELED branch filled it from buffered.cancelledAt), producing isCompleted:true + finishedAt:undefined — exactly the inconsistency the synth shape exists to prevent. Fall back to buffered.createdAt for the SYSTEM_FAILURE case (the buffer entry has no separate failedAt; createdAt is the best-available proxy for when the terminal state landed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Symmetric fix to the just-applied `ApiRetrieveRunPresenter` change. PG-resident SYSTEM_FAILURE rows always have `completedAt` populated; the synthetic span run was returning null for FAILED, so the run-detail panel and any caller using `isFinished && completedAt` saw a finished run with no completion timestamp. Fall back to `run.createdAt` when `isFailed` is true (the buffer entry has no separate failedAt — createdAt is the best proxy for when the terminal state landed). Regression locked in `mollifierSyntheticSpanRun.test.ts` — the FAILED test now asserts `completedAt === NOW`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`.server-changes/mollifier-reads.md` mentioned an "attempts" endpoint but no `api.v1.runs.$runParam.attempts.ts` changes exist in this PR (it's a POST/action route for creating attempts, not a read endpoint). The actual added endpoints are retrieve, trace, spans, and events — fix the changelog to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…unTags test Two CodeRabbit findings on PR #3755: 1. `buildSyntheticSpanRun` was using `run.metadata ? prettyPrintPacket(...) : undefined`. The truthy check drops `""` (and any other intentionally-empty packet), even though the payload branch above already uses a nullish check and preserves empty-string payloads. Align the two branches with `typeof !== "undefined" && !== null` so empty metadata renders consistently. 2. The "forwards runTags from the snapshot tags array" assertion in `mollifierSynthesiseFoundRun.test.ts` used the same value for `tags` and `runTags` in its fixture. That would silently pass if `synthesiseFoundRunFromBuffer` accidentally read `runTags` instead of `tags`. Use distinct values so the assertion actually locks the mapping down. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…shape on buffer read-fallback
`readFallback.server.ts` was checking `Array.isArray(idempotencyKeyOptionsRaw)` and
returning `undefined` otherwise. The SDK and Prisma both serialise this
field as `{ key, scope }` (per `IdempotencyKeyOptionsSchema`); the
array form never matches, so `SyntheticRun.idempotencyKeyOptions` was
always `undefined` for buffered runs.
Downstream effect: `getUserProvidedIdempotencyKey` falls back to
`run.idempotencyKey` when options are absent — which is the *hash*,
not the user-supplied key. PG-resident runs return the user's key
(options parse succeeds); buffered runs were returning the hash. The
API's `idempotencyKey` field silently flipped values at the drainer-
materialisation boundary.
Parse via `IdempotencyKeyOptionsSchema.safeParse` so the buffered
response matches PG-resident behaviour from the moment the run is
buffered.
Type change is local to `SyntheticRun` — `synthesiseFoundRunFromBuffer`
and `buildSyntheticSpanRun` both forward via `?? null` into Prisma
`JsonValue | null` destinations, which accept the new object shape.
Two regression tests on `mollifierReadFallback.test.ts`:
1. Canonical `{ key, scope }` object parses to the schema-shaped data.
2. Legacy array form (the previous bug) and other invalid shapes
return undefined so downstream falls back to the hash, matching
how PG-resident runs handle malformed/missing options.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/test/mollifierReadFallback.test.ts (1)
440-463: 💤 Low valueMinor test overlap with lines 330-350.
This test ("extracts batchId from the nested snapshot.batch object (not the flat key)") covers essentially the same positive case as lines 330-350 ("extracts batchId from the snapshot's nested batch object (engine.trigger shape)"). Consider consolidating into a single test or ensuring they test meaningfully different aspects—the regression comment here adds value but the assertions are identical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/test/mollifierReadFallback.test.ts` around lines 440 - 463, The two tests duplicate the same positive assertion about extracting batchId from the nested snapshot.batch object; consolidate or differentiate them by keeping one of the tests (either "extracts batchId from the nested snapshot.batch object (not the flat key)" or "extracts batchId from the snapshot's nested batch object (engine.trigger shape)") and removing the other, or modify one to cover a distinct scenario (e.g., ensure readFallback returns undefined when only flat batchId is present or when batch is missing). Update the assertions around findRunByIdWithMollifierFallback and the BufferEntry payload accordingly so each test verifies a unique behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/webapp/test/mollifierReadFallback.test.ts`:
- Around line 440-463: The two tests duplicate the same positive assertion about
extracting batchId from the nested snapshot.batch object; consolidate or
differentiate them by keeping one of the tests (either "extracts batchId from
the nested snapshot.batch object (not the flat key)" or "extracts batchId from
the snapshot's nested batch object (engine.trigger shape)") and removing the
other, or modify one to cover a distinct scenario (e.g., ensure readFallback
returns undefined when only flat batchId is present or when batch is missing).
Update the assertions around findRunByIdWithMollifierFallback and the
BufferEntry payload accordingly so each test verifies a unique behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af31bf01-2000-4465-a39f-d934a4cb1de2
📒 Files selected for processing (2)
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when circular dependencies cannot be resolved otherwise, code splitting is needed for performance, or the module must be loaded conditionally at runtime.
Import from@trigger.dev/coreusing subpaths only - never import from the root.
When writing Trigger.dev tasks, always import from@trigger.dev/sdk. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Add agentcrumbs markers (//@Crumbsor `#region `@crumbs) as you write code, not just when debugging. They stay on the branch throughout development and are stripped byagentcrumbs stripbefore merge.
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/v3/mollifier/readFallback.server.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Code formatting must be enforced using Prettier before committing
Files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/mollifierReadFallback.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testableFor testable code, never import
env.server.tsin test files. Pass configuration as options instead (e.g.,realtimeClient.server.tstakes config as constructor arg,realtimeClientGlobal.server.tscreates singleton with env config)
Files:
apps/webapp/test/mollifierReadFallback.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptive describe and it blocks
Unit tests should use vitest framework
Tests should avoid mocks or stubs and use helpers from@internal/testcontainerswhen Redis or Postgres are needed
**/*.test.{ts,tsx,js,jsx}: Never mock anything in tests - use testcontainers instead.
Test files should be placed next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
apps/webapp/test/mollifierReadFallback.test.ts
🧠 Learnings (11)
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-03-29T19:16:28.864Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3291
File: apps/webapp/app/v3/featureFlags.ts:53-65
Timestamp: 2026-03-29T19:16:28.864Z
Learning: When reviewing TypeScript code that uses Zod v3, treat `z.coerce.*()` schemas as their direct Zod type (e.g., `z.coerce.boolean()` returns a `ZodBoolean` with `_def.typeName === "ZodBoolean"`) rather than a `ZodEffects`. Only `.preprocess()`, `.refine()`/`.superRefine()`, and `.transform()` are expected to wrap schemas in `ZodEffects`. Therefore, in reviewers’ logic like `getFlagControlType`, do not flag/unblock failures that require unwrapping `ZodEffects` when the input schema is a `z.coerce.*` schema.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.ts
📚 Learning: 2026-05-05T09:38:02.512Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3523
File: apps/webapp/app/routes/api.v3.batches.ts:178-181
Timestamp: 2026-05-05T09:38:02.512Z
Learning: When reviewing code that catches `ServiceValidationError` in `*.server.ts` files, do not blindly forward `error.status` to HTTP responses, because SVEs may be thrown with non-default statuses (e.g., 400/500) and forwarding them can cause client-visible behavioral regressions (e.g., surfacing 500s to clients). Prefer a safe default response status of `error.status ?? 422`, but only after confirming via the reachable call graph that the caught `ServiceValidationError` instances are expected to carry those non-default statuses; otherwise, normalize to `422` to avoid unexpected client-visible 5xx behavior.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.tsapps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-05-14T08:21:07.614Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3614
File: apps/webapp/app/v3/mollifier/mollifierGate.server.ts:48-52
Timestamp: 2026-05-14T08:21:07.614Z
Learning: When using Trigger.dev v3 feature flags in the webapp, prefer the existing per-org gating mechanism supported by `flag()` via the `overrides` argument. Pass `Organization.featureFlags` (from `environment.organization.featureFlags`) as the `overrides` value; overrides must take precedence over the global `featureFlag` row. Do not require schema changes or add an `orgId` field to `FlagsOptions` for per-org gating—use the overrides pattern consistently (e.g., in gate flows like `resolveOrgFlag` and any server code that threads `environment.organization.featureFlags` into the gate call).
Applied to files:
apps/webapp/app/v3/mollifier/readFallback.server.ts
📚 Learning: 2026-05-07T12:25:18.271Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3531
File: apps/webapp/test/sentryTraceContext.server.test.ts:9-47
Timestamp: 2026-05-07T12:25:18.271Z
Learning: In the triggerdotdev/trigger.dev webapp test suite, it is acceptable to leave `createInMemoryTracing()` calls that register a global `NodeTracerProvider` without `afterEach`/`afterAll` teardown. Do not flag this as a test-ordering risk when the code follows the established pattern used across webapp tests (e.g., replication service/benchmark/backfiller tests). This is considered safe because `trace.getActiveSpan()` when called outside a `context.with(...)` block reads `AsyncLocalStorage.getStore()` (undefined when no `run()` scope exists), so it falls back to `ROOT_CONTEXT` with no attached span—regardless of which provider is registered.
Applied to files:
apps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/mollifierReadFallback.test.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/test/mollifierReadFallback.test.ts
🔇 Additional comments (9)
apps/webapp/app/v3/mollifier/readFallback.server.ts (3)
3-4: LGTM!
55-63: LGTM!
158-170: LGTM!apps/webapp/test/mollifierReadFallback.test.ts (6)
140-141: LGTM!Also applies to: 164-164
198-232: LGTM!
234-263: LGTM!
330-367: LGTM!
465-485: LGTM!
487-534: LGTM!
Summary
Synthesise QUEUED/FAILED responses from the mollifier buffer when a TaskRun row hasn't landed in Postgres yet. Wires the synthesis into:
ApiRetrieveRunPresenterThe
readFallbackinfra itself lives on the trigger PR (consumed byIdempotencyKeyConcern); this PR adds the route-level synthetic-rendering primitives.Stacked on the replay PR.
Test plan