Draft
Conversation
🦋 Changeset detectedLatest commit: 4150bc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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). (19)
WalkthroughThe PR propagates trigger metadata (triggerSource and triggerAction) from entry points (API routes, CLI, MCP, scheduler, dashboard) through service calls into an annotations object, and persists it on TaskRun via a new optional JSON column. It adds schemas/types for annotations, extends TriggerParams and related service option types, updates multiple service and route invocations to pass trigger metadata, and modifies the API client to emit an x-trigger-source header. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
myftija
force-pushed
the
run-annotations
branch
from
c039d65 to
7b9d2bb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wip