Merged
Conversation
📝 WalkthroughWalkthroughEvaluator execution routing changed from evaluator-slug-scoped endpoints to experiment/run/task-scoped endpoints; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/traceloop/sdk/evaluator/evaluator.py (1)
102-113:⚠️ Potential issue | 🔴 CriticalRequired
experiment_slugbreaks an existing caller path.This signature change is not fully propagated:
packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py(context snippet Line 195-213) still callsrun_experiment_evaluator(...)withoutexperiment_slug, which will raise a runtimeTypeError.Proposed follow-up fix (caller update)
- result = await self._evaluator.run_experiment_evaluator( + result = await self._evaluator.run_experiment_evaluator( evaluator_slug=slug, + experiment_slug="guardrail", task_id="guardrail", experiment_id="guardrail", experiment_run_id="guardrail", input=data, timeout_in_sec=120, evaluator_version=evaluator_version, evaluator_config=evaluator_config, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py` around lines 102 - 113, The new required parameter experiment_slug in run_experiment_evaluator breaks existing callers (e.g., guardrails.py) — make the change non-breaking by reverting experiment_slug to an optional parameter with a default (e.g., None) in the run_experiment_evaluator signature or update all callers to pass the new argument; specifically, modify the function definition of run_experiment_evaluator to accept experiment_slug: Optional[str] = None (or update the caller in guardrails.py to pass a valid experiment_slug when invoking run_experiment_evaluator) and add any necessary None-handling inside run_experiment_evaluator where experiment_slug is used.packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (1)
193-207:⚠️ Potential issue | 🟠 MajorPlease add regression tests for the route/signature migration.
These changes alter both evaluator invocation wiring and task endpoint paths. With the PR checklist item still unchecked, this should be covered by tests to prevent silent runtime/API regressions.
Also applies to: 441-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py` around lines 193 - 207, Add regression tests that exercise the migrated evaluator invocation wiring and the task endpoint path/signature changes: write tests that call the code path that uses _evaluator.trigger_experiment_evaluator (and the branch that previously populated eval_results[evaluator_slug]) to ensure the evaluator invocation is passed the correct evaluator_slug, experiment_slug, evaluator_config, task_id and experiment_run_id; additionally add integration-style tests hitting the task endpoint(s) that were renamed/moved to verify request routing and payload signature still reach the new handler and that the evaluator trigger is invoked with expected params. Use mocks/spies for trigger_experiment_evaluator to assert call arguments and include both success and error paths so changes to wiring or routes will fail tests if regressed.
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py (1)
105-105: Docstrings should includeexperiment_slugin Args.Both public method signatures now require
experiment_slug, but the parameter documentation wasn’t updated.Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py` at line 105, Update the docstrings for the public methods in this module that now accept the parameter experiment_slug: add an "experiment_slug: str" entry to each method's Args section describing what the slug represents and how it's used (match the existing docstring style/format in evaluator.py); ensure both docstrings for the two public methods whose signatures include experiment_slug are updated so the Args block lists experiment_slug alongside the other parameters.
🤖 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/traceloop/sdk/evaluator/evaluator.py`:
- Around line 102-113: The new required parameter experiment_slug in
run_experiment_evaluator breaks existing callers (e.g., guardrails.py) — make
the change non-breaking by reverting experiment_slug to an optional parameter
with a default (e.g., None) in the run_experiment_evaluator signature or update
all callers to pass the new argument; specifically, modify the function
definition of run_experiment_evaluator to accept experiment_slug: Optional[str]
= None (or update the caller in guardrails.py to pass a valid experiment_slug
when invoking run_experiment_evaluator) and add any necessary None-handling
inside run_experiment_evaluator where experiment_slug is used.
In `@packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py`:
- Around line 193-207: Add regression tests that exercise the migrated evaluator
invocation wiring and the task endpoint path/signature changes: write tests that
call the code path that uses _evaluator.trigger_experiment_evaluator (and the
branch that previously populated eval_results[evaluator_slug]) to ensure the
evaluator invocation is passed the correct evaluator_slug, experiment_slug,
evaluator_config, task_id and experiment_run_id; additionally add
integration-style tests hitting the task endpoint(s) that were renamed/moved to
verify request routing and payload signature still reach the new handler and
that the evaluator trigger is invoked with expected params. Use mocks/spies for
trigger_experiment_evaluator to assert call arguments and include both success
and error paths so changes to wiring or routes will fail tests if regressed.
---
Nitpick comments:
In `@packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py`:
- Line 105: Update the docstrings for the public methods in this module that now
accept the parameter experiment_slug: add an "experiment_slug: str" entry to
each method's Args section describing what the slug represents and how it's used
(match the existing docstring style/format in evaluator.py); ensure both
docstrings for the two public methods whose signatures include experiment_slug
are updated so the Args block lists experiment_slug alongside the other
parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 069e810a-c03a-4763-8fe3-9bf822a5b0fd
📒 Files selected for processing (2)
packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.pypackages/traceloop-sdk/traceloop/sdk/experiment/experiment.py
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/traceloop/sdk/guardrails/guardrails.py (1)
202-212:⚠️ Potential issue | 🟠 MajorAdd a regression test for the new experiment-scoped call contract.
Line 204 introduces a required routing parameter (
experiment_slug) in this guardrails path. Please add a unit test that verifiesGuardrails.execute_evaluator()forwardsexperiment_slug="guardrail"and the expected dummy IDs toEvaluator.run_experiment_evaluator(...), to prevent silent breakage on future API route changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py` around lines 202 - 212, Add a unit test that mocks the Evaluator.run_experiment_evaluator and asserts Guardrails.execute_evaluator forwards the new experiment-scoped routing parameters: ensure the test calls Guardrails.execute_evaluator (with suitable dummy input `data`) and verifies the mock was called with experiment_slug="guardrail", evaluator_slug equal to the passed `slug`, and the expected dummy IDs for task_id, experiment_id, and experiment_run_id all set to "guardrail", plus the evaluator_version, evaluator_config, timeout_in_sec=120 and input=data; use the Guardrails.execute_evaluator and Evaluator.run_experiment_evaluator symbols to locate and instrument the call (patch/magicmock) and add this test to the guardrails tests suite.
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py (1)
202-207: Extract the repeated"guardrail"context ID into a constant.Using one constant here reduces typo/divergence risk across
experiment_slug,task_id,experiment_id, andexperiment_run_id.♻️ Proposed refactor
+GUARDRAIL_CONTEXT_ID = "guardrail" ... result = await self._evaluator.run_experiment_evaluator( evaluator_slug=slug, - experiment_slug="guardrail", - task_id="guardrail", - experiment_id="guardrail", - experiment_run_id="guardrail", + experiment_slug=GUARDRAIL_CONTEXT_ID, + task_id=GUARDRAIL_CONTEXT_ID, + experiment_id=GUARDRAIL_CONTEXT_ID, + experiment_run_id=GUARDRAIL_CONTEXT_ID, input=data, timeout_in_sec=120, evaluator_version=evaluator_version,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py` around lines 202 - 207, The four repeated literal "guardrail" context IDs passed into self._evaluator.run_experiment_evaluator (experiment_slug, task_id, experiment_id, experiment_run_id) should be consolidated into a single constant (e.g., GUARDRAIL_CONTEXT = "guardrail") declared near the top of the module or class; replace the four literal usages in the call to _evaluator.run_experiment_evaluator with that constant to avoid duplication and typos (update any other occurrences in guardrails.py that use the same literal to use the constant as well).
🤖 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/traceloop/sdk/guardrails/guardrails.py`:
- Around line 202-212: Add a unit test that mocks the
Evaluator.run_experiment_evaluator and asserts Guardrails.execute_evaluator
forwards the new experiment-scoped routing parameters: ensure the test calls
Guardrails.execute_evaluator (with suitable dummy input `data`) and verifies the
mock was called with experiment_slug="guardrail", evaluator_slug equal to the
passed `slug`, and the expected dummy IDs for task_id, experiment_id, and
experiment_run_id all set to "guardrail", plus the evaluator_version,
evaluator_config, timeout_in_sec=120 and input=data; use the
Guardrails.execute_evaluator and Evaluator.run_experiment_evaluator symbols to
locate and instrument the call (patch/magicmock) and add this test to the
guardrails tests suite.
---
Nitpick comments:
In `@packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py`:
- Around line 202-207: The four repeated literal "guardrail" context IDs passed
into self._evaluator.run_experiment_evaluator (experiment_slug, task_id,
experiment_id, experiment_run_id) should be consolidated into a single constant
(e.g., GUARDRAIL_CONTEXT = "guardrail") declared near the top of the module or
class; replace the four literal usages in the call to
_evaluator.run_experiment_evaluator with that constant to avoid duplication and
typos (update any other occurrences in guardrails.py that use the same literal
to use the constant as well).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b431977d-c1dd-4ba3-b59c-90b621aab12d
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/guardrails/guardrails.py
OzBenSimhonTraceloop
approved these changes
Apr 12, 2026
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
2 participants
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.
feat(instrumentation): ...orfix(instrumentation): ....Summary by CodeRabbit