Conversation
65594c4 to
1e0625e
Compare
1e0625e to
d61c13c
Compare
Stacked on #89 — base is `feature/ld-judges-tool-trajectory`, so the diff is just this change. Retarget to `main` when #89 merges. ## Problem #89 gave the trajectory to the **offline** judge only. The two online paths built their own `message_history` and neither included it — so a trajectory rubric silently degraded to grading prose when run online, and a judge grading the same response saw a different conversation depending on which path reached it. They had already drifted before the trajectory made it visible: | Path | `message_history` was | | --- | --- | | Offline evals | row input + **trajectory** + output + format block | | Online inline (`run_judges`) | user input + output + format block | | Online deferred (`run_judge`) | output + format block — **no input at all** | A deferred judge was grading a response with no request beside it. That is a pre-existing bug this PR also fixes. ## Change **`judge_scoring.build_message_history` is now the only place a history is built** — in the module that already owns the `{score, reasoning}` contract, for exactly the same reason. All three paths call it, and a test asserts the inline and deferred paths produce **byte-identical** output for one row. Online capture happens in `execute_and_track` / `execute_and_stream`, which now return the rendered trajectory alongside `response` and `track_data`. `client.py` and the two per-node `graph.py` judge runs thread it through. `JudgeTask` gains `user_input` and `trajectory` — plain strings, since every field on it has to survive pickling to a worker thread; a test pins that. `trajectory.py` moves from `evaluations/` to the package root, since it is no longer evaluations-specific. ## The `NativeTool` decision you asked about **Recording is composed *inside* `wrap_tool_handlers`, on the original tool map**, so the recorder still sees a `NativeTool` as a `NativeTool` and skips it — identically to offline. Wrapping the tracked map instead was the tempting option, because native calls *are* locally observable online: `wrap_tool_handlers` substitutes a callable tracking stub. But that stub returns nothing, so recording it would show a judge **a tool call with an empty result** while the provider's real result stayed invisible — worse than not showing it. Tests assert the native tool is absent from the online trajectory and that `$ld:ai:tool_call` still fires underneath the recorder. Tell me if you'd rather natives appear online with an explicit "result not observable" marker; it's a small change now that one function owns the rendering. ## Graph-level judges get no trajectory, deliberately `graph_judge` grades a final answer produced across several nodes. Splicing their trajectories together would describe a conversation that never happened, so it gets `""`. Per-node judges inside a graph do get their own node's. ## Validation - `uv run pytest -q` — **1282 passed**, 11 skipped - `uv run mypy packages/client/src/launchdarkly_ai_server` — clean; `ruff check` / `format --check` — clean - 13 new tests in `test_judge_message_history.py`: the builder's ordering and skipping, the trajectory reaching both online paths, inline-vs-deferred agreement, `JudgeTask` picklability, online capture through the real `execute_and_track`, native-tool exclusion, and `$ld:ai:tool_call` surviving the composition Spec follow-up for `ai-sdks-monorepo` §3.13/§3.14 to come once this shape is agreed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
bbc59c7 to
c69920d
Compare
`ld-stg.launchdarkly.com` appeared in four places in this **public** repository. Spotted in the diff of #89, but present on `main` independently of it and in three files that PR does not touch — so it is fixed here, on its own, rather than gated behind a feature branch. | File | Was | | --- | --- | | `packages/ai/README.md` | "`LD_UI_BASE_URI` (for example, `https://ld-stg.launchdarkly.com` in staging)" | | `packages/client/README.md` | env var table: "staging: `https://ld-stg.launchdarkly.com`" | | `packages/client/tests/test_evaluations.py` ×2 | the host as a test fixture value | ## Change **The READMEs now say what the option is for, without naming a host.** That is the part a reader actually needs — and the part that was missing: > Evaluation-run links use the explicit `ui_base_uri` option or `LD_UI_BASE_URI`, defaulting to `https://app.launchdarkly.com`; set it when the project is not in production, or a run created elsewhere still links to the production app. Naming LaunchDarkly's own non-production host helped nobody: an external reader cannot reach it, and an internal one does not learn it from an SDK README. **The tests move to `ui.staging.example.com`**, which is the convention the rest of that file already follows — `api.staging.example.com`, `relay.example.com`, `other.example.com`, `ui.example.com`. `ld-stg` was the only outlier. It stays distinct from `ui.example.com` on purpose: that test asserts the explicit option beats the environment variable, which needs two different values to mean anything. ## Scope check Grepped `ld-stg`, `stg.launchdarkly`, and `launchdarkly-stg` across the whole tree — these four were all of them, and the tree is now clean. The sibling `ai-sdks-monorepo` (internal) and `ai-sdk-evaluations-example` (private) never mentioned it. ## Validation `uv run pytest -q` — 1250 passed, 11 skipped. `ruff check` clean. Not a draft: it is four lines, self-contained, and the sooner it is off a public `main` the better. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/launchdarkly/python-ai-sdk/pull/96" target="_blank"><picture><source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img src="https://static.devin.ai/assets/gh-devin-review-light.svg?v=4" alt="Devin Review"></picture></a> <!-- devin-review-badge-end --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Removes **`ld-stg.launchdarkly.com`** from the public tree and replaces it with guidance that does not name LaunchDarkly’s internal staging app. > > **README updates** in `packages/ai/README.md` and `packages/client/README.md` now describe **`LD_UI_BASE_URI` / `ui_base_uri`** as controlling evaluation-run links (default **`https://app.launchdarkly.com`**) and say to set it for non-production projects so runs do not still point at the production app—without listing a staging URL. > > **Tests** in `test_ui_base_uri_precedence_and_api_base_isolation` use **`https://ui.staging.example.com`**, matching the file’s existing **`*.example.com`** staging fixtures and staying distinct from **`ui.example.com`** for the explicit-vs-env precedence assertion. > > No runtime or API behavior changes—documentation and test data only. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 00b8dba. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Handler packages record tool traffic onto spans and return only
{output, usage}, so by the time a criterion ran the calls a row made on
its way to that output were gone -- which made "did the agent call the
right tool, in the right order, with the right arguments?" an unaskable
question of an SDK-run evaluation that had just run the agent that
answered it.
The runner now records the trajectory itself, wrapping the caller's tool
implementations once per row before handing them to the handler. Wrapping
is what covers every handler package without changing any of them: a
handler still resolves a tool by the key the model named and calls it.
The trajectory reaches judges through message_history, interleaved
between the row input and the generated output -- which is where it
happened, and which is the variable every judge cloned from the AI
Library's default templates already references, so a trajectory rubric
needs no new judge template. There is deliberately no standalone
trajectory variable: message_history is already the transcript variable,
and a second overlapping one only invited a rubric to interpolate both
and pay for the trajectory twice. A run with no observable tools adds no
block, so judges authored before this read exactly the history they read
before.
Three properties are pinned by tests. The recorder observes and never
intervenes: a wrapped tool returns and raises what the original did, and
calls past the recording cap still execute and are only counted. A
recorder belongs to one row, since rows generate concurrently against one
shared tool map. And a tool result stays literal in the judge prompt --
it is a new injection surface, closed by the existing rule that the judge
config is passed unrendered for the handler's single template pass.
Native provider tools are passed through unwrapped and left out of the
rendered "tools available" line: they execute inside the provider, so
naming a tool whose use cannot be shown would invite a judge to conclude
the model ignored it.
Nothing about the trajectory is added to any event payload.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The trajectory reached only the offline evaluations judge. The two online
paths built their own message_history and neither included it, so the
same judge grading the same response saw a different conversation
depending on which path reached it -- and a trajectory rubric silently
degraded to grading prose when run online.
They had already drifted before the trajectory made it visible:
offline row input + trajectory + output + format block
inline user input + + output + format block
deferred + output + format block
The deferred path carried no input at all, so a background judge graded
a response with no request beside it.
judge_scoring.build_message_history is now the only place a history is
built, in the module that already owns the {score, reasoning} contract
for the same reason. All three paths call it, and a test asserts the
inline and deferred paths produce byte-identical output for one row.
Capture online happens in execute_and_track and execute_and_stream,
which return the rendered trajectory alongside response and track_data.
client.py and the two per-node graph.py judge runs thread it through.
JudgeTask gains user_input and trajectory -- plain strings, since every
field on it has to survive pickling to a worker thread.
Recording is composed *inside* wrap_tool_handlers, on the original tool
map, so the recorder still sees a NativeTool as a NativeTool and skips
it. Wrapping the tracked map instead would have recorded the sync
callable stub that wrapper substitutes for a native tool, showing a
judge a call with an empty result while the provider's real result
stayed invisible. Both paths now treat natives identically, and
$ld:ai:tool_call still fires underneath -- both asserted.
trajectory.py moves from evaluations/ to the package root: it is no
longer evaluations-specific.
A graph-level judge deliberately gets no trajectory. It grades a final
answer produced across several nodes, and splicing their trajectories
would describe a conversation that never happened.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c69920d to
16ca5d6
Compare
json.dumps escapes non-ASCII by default, so a tool that returned "café" or "東京" reached the judge's message_history as "café" and "東京". The judge then had to grade a tool result through escape noise, and a rubric asking about non-English content was reading something the model never produced. ensure_ascii=False. Key order stays sorted, so one language's own output remains deterministic for its tests; byte-for-byte agreement with another SDK is explicitly not the goal, but showing the judge the characters the tool actually returned is. A string result was already passed through unescaped, so the JSON path was the only one doing this -- the two now agree, and a test pins both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5517990. Configure here.
| self._complete(slot, result=result) | ||
| return result | ||
|
|
||
| return wrapper |
There was a problem hiding this comment.
Wrapper changes sync tool calls
Medium Severity
_record always replaces a tool with an async def wrapper, and _run_rows now hands that map to every offline handler. A custom handler that invoked a sync tool without await (the natural call for a sync function) now gets a coroutine object as the result and can finish the row with garbage output instead of raising.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5517990. Configure here.
Three findings from the Devin and Cursor reviews on #89, all real. Sync tools no longer become async. TrajectoryRecorder._record wrapped every implementation in an `async def`, matching what §3.8's tracking wrapper does. Online that was already the shape, so nothing changed there -- but offline these implementations used to be passed through untouched, and a caller's own handler may invoke a sync tool directly and use the value, which is the natural call for a sync function. It silently received a coroutine object instead and could finish the row with its repr. A sync tool now stays sync, an async one stays async, and a sync callable returning an awaitable hands back an awaitable that records on completion so a pending coroutine's repr never reaches a judge. Only tools the config offered are recorded or described. The recorder derived "Tools available" from the whole implementation map, but online config() merges a Registry's tools into that map while the flag variation decides what the model sees. A registry holding ten tools made every judge read ten as available and penalise an agent for ignoring eight it was never offered. execute_and_track and execute_and_stream now pass the config's tool keys; offline passes nothing, because the runner resolves the config's tools from the same map and the two agree by construction. Synthetic graph-routing tools are skipped. graph.route injects __handoff_* onto a multi-edge node's config, and §3.8 already excludes them from $ld:ai:tool_call for the reason that applies here too: they are not tools the agent was given, and a per-node judge is scored against the node's original config, which does not list them. Showing them invited a judge to grade a handoff as tool use and to read "Handoff to X recorded" as a tool result. The prefix now has one definition, in trajectory.py, which tracking.py uses as well. Tests that awaited a sync tool were asserting the behaviour being removed and now call it synchronously; the online ones still await, since §3.8's wrapper is still a coroutine function there, and that difference is now commented where it could confuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| if len(args) == 1 and not kwargs: | ||
| return args[0] |
There was a problem hiding this comment.
🟡 Mutated tool arguments mislead judges
When a tool mutates its argument mapping, _call_arguments retains the same object and records its post-call contents. Judges then grade different arguments from those the model supplied.
Learn more
The recorder reserves a call before invoking the tool, but _call_arguments returns mutable positional arguments by reference. A tool can mutate that object before render_trajectory serializes it. The recorded trajectory then describes the mutated state, not the call boundary.
Example: The model calls lookup_order with {"id": "A1"}. The implementation runs order_id = args.pop("id") and returns successfully. The trajectory renders arguments: {} although the model supplied A1.
Recommended fix: Capture a stable snapshot in _reserve without changing the object passed to the tool. Handle values that cannot be copied or serialized so trajectory capture remains observational and never fails the invocation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _truncate(text: str) -> str: | ||
| if len(text) <= MAX_RECORDED_VALUE_CHARS: | ||
| return text | ||
| return text[:MAX_RECORDED_VALUE_CHARS] + _TRUNCATION_SUFFIX |
There was a problem hiding this comment.
This repository is public; the spec these comments cited by section number is not. Every reference is now to the code a reader can actually open -- wrap_tool_handlers, build_message_history, graph.route -- which is more useful here anyway. Also cut the prose back. The trajectory module had a 47-line docstring arguing for its own existence; it is 13 now, and the comments that restated what the next line does are gone. What is kept is the reasoning a reader cannot recover from the code: why recording composes inside the tracking wrapper, why a sync tool must stay sync, why a native tool is skipped even where it is observable. No behaviour change. 1340 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| rendered = json.dumps( | ||
| value, | ||
| sort_keys=True, | ||
| separators=(",", ":"), | ||
| default=str, | ||
| ensure_ascii=False, | ||
| ) | ||
| except (TypeError, ValueError): | ||
| rendered = str(value) |
There was a problem hiding this comment.
🟡 Unrenderable tool values fail invocations
When a tool value’s string conversion raises, _render_value lets that exception escape after generation succeeds. invoke() then fails, and streaming omits its final event despite successful provider output.
Learn more
Trajectory rendering runs after the handler has returned successfully. json.dumps(..., default=str) can propagate any exception raised by a value's __str__, but this block catches only TypeError and ValueError. The fallback str(value) can also raise, so recording introduces a new failure after provider work and tool side effects have completed.
Example: A tool returns an object whose __str__ raises RuntimeError("closed"). A custom handler consumes that object and returns valid output, but execute_and_track raises while rendering the trajectory instead of returning the output.
Recommended fix: Make _render_value total: catch exceptions from both JSON encoding and fallback conversion, then emit a fixed placeholder or safe type name. Add invoke and streaming tests proving malformed representations cannot suppress a successful response.
Was this helpful? React with 👍 or 👎 to provide feedback.
The paragraph narrated how the three paths used to disagree. The rule above it already says they must not, which is the part a reader needs; the history belongs in the PR, not the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>


Now carries two commits: the trajectory capture, and #98 (merged in here) which made every judge path share one
message_history.Problem
A judge can only grade what it is shown. Handler packages record tool traffic onto OpenTelemetry spans and return only
{output, usage}, so by the time a judge ran, the calls made on the way to that output were gone. "Did the agent call the right tool, in the right order, with the right arguments?" was an unaskable question of an SDK that had just run the agent that answered it.Underneath that, the three judge paths had already drifted. Each built
message_historywith its own inline join:message_historywasrun_judges)run_judge)A deferred judge was grading a response with no request beside it. That is a pre-existing bug, fixed here as a consequence of unifying.
Approach
Capture by wrapping the tool map, which both paths already own — so every handler package is covered without changing any of them, and a custom caller-supplied handler is covered too. A handler still resolves a tool by the key the model named and calls it.
evaluations/runner.py.tracking.execute_and_track/execute_and_stream, which now return the rendered trajectory alongsideresponseandtrack_data.judge_scoring.build_message_historyis the only place a history is built — in the module that already owns the{score, reasoning}contract, for exactly the same reason. It orders the conversation the way it happened: input → trajectory → output → format block, skipping empty parts. All three paths call it.What a judge now sees:
Verified live against a real LaunchDarkly judge: the judge's reasoning cited the call, its arguments, and its result — all three only visible via the trajectory.
Properties pinned by tests
{{expected_output}}.message_historyit read before.Design notes
{{tool_trajectory}}too; it overlappedmessage_historyand bought nothing, while inviting a rubric to interpolate both and pay for the trajectory twice. Confirmed live — a real judge config's own scaffolding already interpolates{{message_history}}. A test pins its absence. Sincemessage_historyis what judges cloned from the AI Library's default templates read, an existing judge becomes a trajectory judge by editing its rubric text alone.wrap_tool_handlerssubstitutes a callable tracking stub, so a native call is locally observable — but the stub returns nothing, so recording it would show a judge a call with an empty result while the provider's real result stayed invisible. Recording is therefore composed inside that wrapper, on the original map. Tests assert both the exclusion and that$ld:ai:tool_callstill fires underneath.JudgeTaskgainsuser_inputandtrajectoryas plain strings — every field on it has to survive pickling to a worker thread; a test pins that.graph_judgegrades an answer produced across several nodes, and splicing their trajectories would describe a conversation that never happened. Per-node judges get their own node's.trajectory.pysits at the package root, not underevaluations/, since it is no longer evaluations-specific.Deliberately out of scope
Scorers cannot see the trajectory.
Scorer.fn(row, output)is the contract, and the trajectory is not dataset-owned so it does not belong onDatasetRow. A deterministic check like "calledlookup_orderexactly once" is a natural follow-up but needs a contract change, not a quiet signature widening.Validation
uv run pytest -q— 1282 passed, 11 skippeduv run mypy packages/client/src/launchdarkly_ai_server— clean;ruff check/format --check— cleanai-sdk-evaluations-example(launchdarkly-labs/ai-sdk-evaluations-example#5), including a live run against a real judgeLanguage-agnostic spec: launchdarkly/ai-sdks-monorepo#13 — being restructured to describe the shared flow rather than the offline phase alone, now that this shape is settled. Submodule pointer: launchdarkly/ai-sdks-monorepo#14.
🤖 Generated with Claude Code
Note
Overview
Judges can now grade tool use as well as final answers by recording each invocation’s tool-call trajectory and folding it into
{{message_history}}(input → trajectory → output → JSON format block), with no new template variable or event payload fields.A new
TrajectoryRecorderwraps tool implementations before handlers run (online inexecute_and_track/ streaming, offline once per concurrent eval row) without changing tool behavior; it renders ordered calls with limits (50 calls, 2k chars per arg/result), skips native/handoff/unexposed tools, and keeps trajectories out of generation wire events.judge_scoring.build_message_historyreplaces per-path inline joins so inline, deferred (JudgeTasknow carriesuser_inputand picklabletrajectory), offline evals, and per-node graph judges share the same transcript; deferred judges previously saw output only.Reviewed by Cursor Bugbot for commit a137de8. Bugbot is set up for automated code reviews on this repo. Configure here.