Conversation
|
WalkthroughThis pull request introduces trigger source and action tracking across the Trigger.dev platform. Changes include: adding an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli-v3/src/mcp/tools/tasks.ts (1)
101-118:⚠️ Potential issue | 🟠 MajorDon't drop the selected environment from the trigger path.
This switch replaces the environment-aware MCP client flow with
ctx.getCliApiClient(input.branch), buttriggerTaskRun()only sendstaskIdplus the payload.input.environmentno longer affects the trigger request, so the tool can now trigger a run in a different environment than the caller selected. Please keep this path environment-scoped or validate the task againstinput.environmentbefore triggering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/mcp/tools/tasks.ts` around lines 101 - 118, The current flow uses ctx.getCliApiClient(input.branch) and then calls cliApiClient.triggerTaskRun(input.taskId, ...) which ignores input.environment, allowing triggers to run in the wrong environment; update the path to be environment-scoped by either obtaining an environment-specific client (use the environment-aware MCP client instead of ctx.getCliApiClient, e.g., call the existing environment-scoped client factory with input.environment) or validate the task's environment before calling triggerTaskRun, and/or include the environment when invoking triggerTaskRun so input.environment is honored; reference ctx.getCliApiClient, input.branch, input.environment, triggerTaskRun, and input.taskId to locate where to change.
🧹 Nitpick comments (3)
apps/webapp/app/v3/runEngineHandlers.server.ts (1)
753-754: Consider propagatingtriggerSourcefrom batch metadata.The current logic infers
triggerSourcebased onmeta.parentRunId:
parentRunIdpresent →"sdk"- Otherwise →
"api"This works for most cases, but batch items created from the dashboard would be misattributed as
"api". Consider whether the batch metadata (meta) should carry the originaltriggerSourcefrom the batch creation call site, allowing accurate attribution regardless of whether it's a nested trigger.💡 Suggested approach
{ triggerVersion: meta.triggerVersion, traceContext: meta.traceContext as Record<string, unknown> | undefined, spanParentAsLink: meta.spanParentAsLink, batchId, batchIndex: itemIndex, realtimeStreamsVersion: meta.realtimeStreamsVersion, planType: meta.planType, - triggerSource: meta.parentRunId ? "sdk" : "api", + triggerSource: meta.triggerSource ?? (meta.parentRunId ? "sdk" : "api"), triggerAction: "trigger", },This would require adding
triggerSourceto the batch metadata type and propagating it from batch creation endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/runEngineHandlers.server.ts` around lines 753 - 754, The current assignment of triggerSource using meta.parentRunId is too coarse; modify the logic where triggerSource is set (the object with keys triggerSource and triggerAction in run handling) to prefer a triggerSource value coming from meta.triggerSource if present, falling back to meta.parentRunId ? "sdk" : "api" only when meta.triggerSource is undefined; update the batch metadata type to include triggerSource at creation points and ensure batch creation endpoints populate meta.triggerSource so attribution is preserved across nested/batch items (refer to meta.triggerSource, meta.parentRunId, and the triggerSource assignment in the handler).apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (1)
471-471: Consider logging when annotation parsing fails.Using
safeParse(...).datasilently discards validation errors. While this provides graceful degradation for API responses, consider adding a warning log when parsing fails to help detect data corruption or schema drift.♻️ Optional: Add logging for parse failures
- annotations: run.annotations ? RunAnnotations.safeParse(run.annotations).data : undefined, + annotations: run.annotations + ? (() => { + const parsed = RunAnnotations.safeParse(run.annotations); + if (!parsed.success) { + logger.warn("Failed to parse run annotations", { + runId: run.friendlyId, + error: parsed.error.message, + }); + } + return parsed.data; + })() + : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts` at line 471, ApiRetrieveRunPresenter currently drops validation errors by using RunAnnotations.safeParse(...).data; change to capture the parse result (e.g., const parsed = RunAnnotations.safeParse(run.annotations)) and only set annotations to parsed.data when parsed.success is true, and when parsed.success is false emit a warning log that includes the run identifier (e.g., run.id or run.runId) and parsed.error to surface schema drift or corruption; use the presenter/module logger (or console.warn if no logger exists) so failures are recorded for investigation.apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (1)
39-39: Constrainx-trigger-sourceto the supported values.This header now flows into persisted run annotations, but the schema still accepts any string. That will let typos and arbitrary values leak into annotations through both the single-trigger and batch routes that reuse
HeadersSchema. Please validate it against a bounded enum/union at the API boundary instead of rawz.string().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/api.v1.tasks`.$taskId.trigger.ts at line 39, HeadersSchema currently allows any string for the "x-trigger-source" header, letting typos/arbitrary values propagate to persisted run annotations; update the HeadersSchema so "x-trigger-source" is validated against a bounded set of supported values (e.g., replace z.string().nullish() with a z.enum([...]).nullish() or z.union of literal()s listing the allowed trigger source names) and ensure the same validated HeadersSchema is used by both the single-trigger and batch routes that consume it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/runEngine/services/batchTrigger.server.ts`:
- Around line 683-684: The pre-failed fallback path calls
TriggerFailedTaskService without the trigger attribution fields, causing
unannotated runs; update that fallback to compute the same triggerSource and
triggerAction values used in the normal TriggerTaskService.call() path (e.g.,
triggerSource: parentRunId ? "sdk" : options?.triggerSource ?? "api",
triggerAction: options?.triggerAction ?? "trigger") and pass them into the
TriggerFailedTaskService invocation so fallback-created runs include the same
attribution metadata.
---
Outside diff comments:
In `@packages/cli-v3/src/mcp/tools/tasks.ts`:
- Around line 101-118: The current flow uses ctx.getCliApiClient(input.branch)
and then calls cliApiClient.triggerTaskRun(input.taskId, ...) which ignores
input.environment, allowing triggers to run in the wrong environment; update the
path to be environment-scoped by either obtaining an environment-specific client
(use the environment-aware MCP client instead of ctx.getCliApiClient, e.g., call
the existing environment-scoped client factory with input.environment) or
validate the task's environment before calling triggerTaskRun, and/or include
the environment when invoking triggerTaskRun so input.environment is honored;
reference ctx.getCliApiClient, input.branch, input.environment, triggerTaskRun,
and input.taskId to locate where to change.
---
Nitpick comments:
In `@apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts`:
- Line 471: ApiRetrieveRunPresenter currently drops validation errors by using
RunAnnotations.safeParse(...).data; change to capture the parse result (e.g.,
const parsed = RunAnnotations.safeParse(run.annotations)) and only set
annotations to parsed.data when parsed.success is true, and when parsed.success
is false emit a warning log that includes the run identifier (e.g., run.id or
run.runId) and parsed.error to surface schema drift or corruption; use the
presenter/module logger (or console.warn if no logger exists) so failures are
recorded for investigation.
In `@apps/webapp/app/routes/api.v1.tasks`.$taskId.trigger.ts:
- Line 39: HeadersSchema currently allows any string for the "x-trigger-source"
header, letting typos/arbitrary values propagate to persisted run annotations;
update the HeadersSchema so "x-trigger-source" is validated against a bounded
set of supported values (e.g., replace z.string().nullish() with a
z.enum([...]).nullish() or z.union of literal()s listing the allowed trigger
source names) and ensure the same validated HeadersSchema is used by both the
single-trigger and batch routes that consume it.
In `@apps/webapp/app/v3/runEngineHandlers.server.ts`:
- Around line 753-754: The current assignment of triggerSource using
meta.parentRunId is too coarse; modify the logic where triggerSource is set (the
object with keys triggerSource and triggerAction in run handling) to prefer a
triggerSource value coming from meta.triggerSource if present, falling back to
meta.parentRunId ? "sdk" : "api" only when meta.triggerSource is undefined;
update the batch metadata type to include triggerSource at creation points and
ensure batch creation endpoints populate meta.triggerSource so attribution is
preserved across nested/batch items (refer to meta.triggerSource,
meta.parentRunId, and the triggerSource assignment in the handler).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a02d1583-d9b9-4fe1-a917-545c50feb60c
📒 Files selected for processing (23)
apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.tsapps/webapp/app/routes/api.v1.runs.$runParam.replay.tsapps/webapp/app/routes/api.v1.tasks.$taskId.trigger.tsapps/webapp/app/routes/api.v1.tasks.batch.tsapps/webapp/app/routes/api.v2.tasks.batch.tsapps/webapp/app/routes/resources.taskruns.$runParam.replay.tsapps/webapp/app/runEngine/services/batchTrigger.server.tsapps/webapp/app/runEngine/services/triggerTask.server.tsapps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/scheduleEngine.server.tsapps/webapp/app/v3/services/batchTriggerV3.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.server.tsapps/webapp/app/v3/services/bulk/performBulkAction.server.tsapps/webapp/app/v3/services/replayTaskRun.server.tsapps/webapp/app/v3/services/testTask.server.tsapps/webapp/app/v3/services/triggerTask.server.tsinternal-packages/database/prisma/schema.prismainternal-packages/run-engine/src/engine/index.tsinternal-packages/run-engine/src/engine/types.tspackages/cli-v3/src/apiClient.tspackages/cli-v3/src/mcp/context.tspackages/cli-v3/src/mcp/tools/tasks.tspackages/core/src/v3/schemas/api.ts
| triggerSource: parentRunId ? "sdk" : options?.triggerSource ?? "api", | ||
| triggerAction: options?.triggerAction ?? "trigger", |
There was a problem hiding this comment.
Batch fallback runs still lose trigger annotations.
These fields are only attached on the normal TriggerTaskService.call() path. The pre-failed fallback at Lines 567-580 still calls TriggerFailedTaskService without triggerSource / triggerAction, so any batch item that fails validation, entitlement, or queue checks will create an unannotated run. Please thread the same attribution fields through that fallback path too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/runEngine/services/batchTrigger.server.ts` around lines 683 -
684, The pre-failed fallback path calls TriggerFailedTaskService without the
trigger attribution fields, causing unannotated runs; update that fallback to
compute the same triggerSource and triggerAction values used in the normal
TriggerTaskService.call() path (e.g., triggerSource: parentRunId ? "sdk" :
options?.triggerSource ?? "api", triggerAction: options?.triggerAction ??
"trigger") and pass them into the TriggerFailedTaskService invocation so
fallback-created runs include the same attribution metadata.
1 participant
wip