Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughEvaluator execution endpoints and experiment task endpoints/payloads were updated: evaluator run now posts to a standalone Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts`:
- Around line 227-229: The run() method endpoint was changed to
"/v2/evaluators/:id/execute" but tests still assert the old
"/v2/evaluators/:id/executions" URL; update the assertions in the standalone
evaluators tests that check the request URL to expect
"https://api.traceloop.com/v2/evaluators/:id/execute" instead of the old path so
they match the post call made in Evaluator.run (the client.post call using
`/v2/evaluators/${encodeURIComponent(identifier)}/execute`).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbad6579-412f-4ff6-a753-e88548c5167d
📒 Files selected for processing (2)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.tspackages/traceloop-sdk/src/lib/client/experiment/experiment.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
366-404:⚠️ Potential issue | 🟠 MajorValidate
experimentRunIdandtaskIdbefore building the new task URL.Line 402 now requires both values in the path, but this method still only guards
experimentIdandtaskResult. If either ID is missing, the client will POST to/runs/undefined/tasks/undefinedinstead of failing fast with a clear local error.Suggested fix
- if (!experimentId || !taskResult) { - throw new Error("experimentId, evaluator, and taskResult are required"); + if (!experimentId || !experimentRunId || !taskId || !evaluator || !taskResult) { + throw new Error( + "experimentId, experimentRunId, taskId, evaluator, and taskResult are required", + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts` around lines 366 - 404, The method currently validates only experimentId and taskResult but then builds a URL using experimentRunId and taskId; add explicit validation for experimentRunId and taskId (e.g., check they are truthy) alongside the existing guard for experimentId/taskResult and throw a clear Error if missing, before calling createInputSchemaMapping or this.client.post so the client doesn't POST to /runs/undefined/tasks/undefined; update the error message to mention experimentRunId and taskId (referencing the variables experimentRunId, taskId and the client.post call) so failures fail fast locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts`:
- Around line 366-404: The method currently validates only experimentId and
taskResult but then builds a URL using experimentRunId and taskId; add explicit
validation for experimentRunId and taskId (e.g., check they are truthy)
alongside the existing guard for experimentId/taskResult and throw a clear Error
if missing, before calling createInputSchemaMapping or this.client.post so the
client doesn't POST to /runs/undefined/tasks/undefined; update the error message
to mention experimentRunId and taskId (referencing the variables
experimentRunId, taskId and the client.post call) so failures fail fast locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb443df9-d29f-4fdf-af8f-4920c753ad7b
📒 Files selected for processing (1)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts (1)
11-19:⚠️ Potential issue | 🟠 MajorRequire all route params in the public request types.
runExperimentEvaluator()/triggerExperimentEvaluator()now target/v2/experiments/${experimentSlug}/runs/${experimentRunId}/tasks/${taskId}. LeavingexperimentRunIdoptional onEvaluatorRunOptions, andexperimentRunId/taskIdoptional onTriggerEvaluatorRequest, lets SDK consumers build requests that type-check but still produce/runs/undefined/tasks/undefinedat runtime.Suggested contract fix
export interface EvaluatorRunOptions { experimentId: string; experimentSlug: string; - experimentRunId?: string; + experimentRunId: string; taskId: string; taskResult: Record<string, any>; evaluator: EvaluatorDetails; waitForResults?: boolean; timeout?: number; } export interface TriggerEvaluatorRequest { experimentId: string; experimentSlug: string; - experimentRunId?: string; - taskId?: string; + experimentRunId: string; + taskId: string; evaluator: EvaluatorDetails; taskResult: Record<string, any>; metadata?: Record<string, any>; }Also applies to: 31-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts` around lines 11 - 19, The public request types allow building requests with undefined route params; make experimentRunId and taskId required so callers cannot produce /runs/undefined/tasks/undefined: update the EvaluatorRunOptions interface to mark experimentRunId as non-optional and update TriggerEvaluatorRequest (and any related request type around lines 31-37) to make experimentRunId and taskId required, and verify runExperimentEvaluator/triggerExperimentEvaluator signatures use these non-optional properties so the router string interpolation always receives defined values.packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
365-405:⚠️ Potential issue | 🟠 MajorReject missing
experimentRunIdandtaskIdbefore building the task URL.This method now posts to
/v2/experiments/${experimentSlug}/runs/${experimentRunId}/tasks/${taskId}, but it only validatesexperimentSlugandtaskResult. A JS caller—or any caller passingundefinedthrough—will silently hit/runs/undefined/tasks/undefinedinstead of getting a local error. Please validate the run/task identifiers here as well, and make the error message match the actual checks.Suggested guard update
- if (!experimentSlug || !taskResult) { - throw new Error("experimentSlug, evaluator, and taskResult are required"); + if (!experimentSlug || !experimentRunId || !taskId || !taskResult) { + throw new Error( + "experimentSlug, experimentRunId, taskId, and taskResult are required", + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts` around lines 365 - 405, The code currently only checks experimentSlug and taskResult but later uses experimentRunId and taskId to build the URL; update the same method to validate that experimentRunId and taskId are present (not null/undefined/empty) before constructing the payload/URL and throw a clear Error mentioning which identifier is missing; ensure the thrown message aligns with the actual checks (e.g., "experimentSlug, experimentRunId, taskId, evaluator, and taskResult are required" or separate errors for run vs task), and keep references to the existing variables experimentRunId, taskId, experimentSlug, taskResult and evaluatorName so the validation runs prior to calling this.client.post.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts`:
- Around line 365-405: The code currently only checks experimentSlug and
taskResult but later uses experimentRunId and taskId to build the URL; update
the same method to validate that experimentRunId and taskId are present (not
null/undefined/empty) before constructing the payload/URL and throw a clear
Error mentioning which identifier is missing; ensure the thrown message aligns
with the actual checks (e.g., "experimentSlug, experimentRunId, taskId,
evaluator, and taskResult are required" or separate errors for run vs task), and
keep references to the existing variables experimentRunId, taskId,
experimentSlug, taskResult and evaluatorName so the validation runs prior to
calling this.client.post.
In `@packages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts`:
- Around line 11-19: The public request types allow building requests with
undefined route params; make experimentRunId and taskId required so callers
cannot produce /runs/undefined/tasks/undefined: update the EvaluatorRunOptions
interface to mark experimentRunId as non-optional and update
TriggerEvaluatorRequest (and any related request type around lines 31-37) to
make experimentRunId and taskId required, and verify
runExperimentEvaluator/triggerExperimentEvaluator signatures use these
non-optional properties so the router string interpolation always receives
defined values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c591851b-bfbc-4ae1-854d-0ce8f0dcf0e5
📒 Files selected for processing (3)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.tspackages/traceloop-sdk/src/lib/client/experiment/experiment.tspackages/traceloop-sdk/src/lib/interfaces/evaluator.interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/traceloop-sdk/src/lib/client/experiment/experiment.ts
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/traceloop-sdk/src/lib/client/evaluator/evaluator.ts (1)
365-390:⚠️ Potential issue | 🟠 MajorValidate
evaluatorbefore dereferencing it.Line 380 can throw (
evaluator.name) whenrequest.evaluatoris missing, because Lines 374-376 currently don’t validateevaluatordespite the error message saying it is required.💡 Suggested fix
- if (!experimentSlug || !taskResult) { - throw new Error("experimentSlug, evaluator, and taskResult are required"); - } + if (!experimentSlug || !taskResult || !evaluator) { + throw new Error("experimentSlug, evaluator, and taskResult are required"); + } // Handle string, EvaluatorWithVersion, and EvaluatorWithConfig types const evaluatorName = typeof evaluator === "string" ? evaluator : evaluator.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts` around lines 365 - 390, The code dereferences request.evaluator without validating it; update the initial validation to require evaluator (e.g., check request.evaluator !== undefined/null) before extracting properties, or add a guard before computing evaluatorName/evaluatorVersion/evaluatorConfig so you never access evaluator.name on undefined; adjust the thrown Error message to match (include "evaluator" alongside experimentSlug and taskResult), and use the existing local symbols evaluatorName, evaluatorVersion, evaluatorConfig to compute values only after the null/undefined check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts`:
- Line 410: The interpolated route string in evaluator.ts uses experimentSlug,
experimentRunId, and taskId directly; wrap each dynamic segment with URL-safe
encoding (e.g., apply encodeURIComponent to experimentSlug, experimentRunId, and
taskId before building the template string used for
`/v2/experiments/.../runs/.../tasks/...`) so reserved URL characters won’t break
or redirect the request; update the code that constructs that path (the template
string using `/v2/experiments/${...}/runs/${...}/tasks/${...}`) to use the
encoded values.
---
Outside diff comments:
In `@packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts`:
- Around line 365-390: The code dereferences request.evaluator without
validating it; update the initial validation to require evaluator (e.g., check
request.evaluator !== undefined/null) before extracting properties, or add a
guard before computing evaluatorName/evaluatorVersion/evaluatorConfig so you
never access evaluator.name on undefined; adjust the thrown Error message to
match (include "evaluator" alongside experimentSlug and taskResult), and use the
existing local symbols evaluatorName, evaluatorVersion, evaluatorConfig to
compute values only after the null/undefined check passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01a855fd-6054-4138-ba49-deee5e977fbc
📒 Files selected for processing (1)
packages/traceloop-sdk/src/lib/client/evaluator/evaluator.ts
2 participants
Summary by CodeRabbit
Chores
Breaking Changes