Conversation
Map a completed observability run into a defineEval fixture, persist an EvalDataset row, and let `agent-native eval promote --write` emit the *.eval.ts file the CI gate already discovers. Truncated or prompt-less runs fail closed. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
There was a problem hiding this comment.
Builder reviewed your changes and found 6 potential issues 🔴
Review Details
Code Review Summary
PR #5465 adds a useful end-to-end path from completed observability runs to CI eval fixtures: a pure trace mapper, a scoped hosted action and route, CLI promotion with optional file output, dataset persistence, provenance, and a dashboard control. The separation between mapping and filesystem emission is sound, and the implementation adds meaningful focused tests and localized documentation. I classified this as high risk because it introduces a mutating authenticated API, per-user data isolation, and a new persistence path.
Key Findings
- 🔴 High: Real production runs do not appear to contain the
user-messageevent required by the mapper, so promotion fails closed for normal runs. - 🔴 High: Existing
agent_eval_datasetstables are not migrated to add the newuser_idcolumn. - 🟡 Medium: Repeated requests are not idempotent and create duplicate datasets.
- 🟡 Medium: Legacy tool-error events can be promoted as successful tool calls.
- 🟡 Medium: CLI
--writepersists before writing the fixture, allowing partial promotions. - 🟡 Medium: Promotion loads the entire unbounded event history.
The ownership check before loading run data and the typed failure responses are good defensive patterns. 🧪 Browser testing: Will run after this review (PR touches UI code)
| }); | ||
| if (!result.ok) refuse(result.error); | ||
|
|
||
| await insertEvalDataset(result.value.dataset); |
There was a problem hiding this comment.
🟡 Repeated promotion inserts duplicate datasets
Each promotion generates a fresh UUID and unconditionally inserts a dataset, while the default logical name is deterministic. Retries, double-clicks, or concurrent requests for the same user/run therefore create duplicate dataset rows; the added name lookup is unused and cannot make concurrent writes safe. Add a database-enforced per-owner/source identity with an atomic upsert or return the existing promotion.
Additional Info
Reported independently by 4 of 4 review agents.
| if (args.write) { | ||
| const target = path.resolve(process.cwd(), args.write); | ||
| await fs.mkdir(path.dirname(target), { recursive: true }); | ||
| await fs.writeFile(target, generateEvalModuleSource(result.eval), "utf8"); |
There was a problem hiding this comment.
🟡 CLI can persist a dataset without its fixture
runPromote() commits the dataset through the store helper before creating the --write file. If the target is unwritable, a directory, or the process is interrupted, the command fails after persistence and leaves a partial promotion; retrying then creates another dataset. Write/validate before committing, or make persistence idempotent and recoverable.
Additional Info
Reported by 1 of 4 review agents; confirmed ordering in the CLI path.
| const [run, events, spans] = await Promise.all([ | ||
| getRunById(runId), | ||
| getRunEventsSince(runId, 0), |
There was a problem hiding this comment.
🟡 Promotion loads the entire unbounded event history
The action calls getRunEventsSince(runId, 0) with no limit and then parses the complete event history, while also embedding prior turns into the generated fixture. Long production runs can make the hosted request or CLI disproportionately slow or exhaust memory. Bound the promotion input or query only the needed event data, with an explicit truncation/error contract rather than silently processing an unbounded trace.
Additional Info
Reported by 1 of 4 review agents; clearly actionable performance risk for production traces.
|
great idea @mvanhorn - can you take a look at the automated review comments and fix anything oyu agree with? and share any screnshots of any UI you updated (if any)? |
…ssification Promotion looked for a "user-message" event, which is not in the AgentChatEvent union and is never emitted, so every real run failed with no_user_prompt. Read the prompt from the durable thread instead, keeping the old path as a fallback. Extract isToolDoneFailure so trace instrumentation and promotion classify legacy tool errors by the same rule. Previously the event fallback only honoured isError/status, so a legacy failure whose result began with "Error" was promoted as a successful tool and gained a usesTool() scorer. Adds a regression test pinning the existing user_id ALTER for agent_eval_datasets, which already runs via the USER_SCOPED_TABLES loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hbhAdQ6qFfwMgCNscPHwS
|
Thanks @steve8708, went through all six. Pushed 9ea9396 with three fixed and three left alone deliberately. Fixed: The prompt extraction one is the important find. Legacy tool errors were also real. The event fallback only honoured One correction: the Left alone, because each is a decision rather than an oversight: Duplicate datasets on repeated promotion is real, but the fix asks for a unique key on owner plus source, which decides what a dataset is: one canonical case per run, or a fresh snapshot each promote. That is yours to pick. The CLI committing before Unbounded event loading is a real bound on long runs, but what should happen past the limit (truncate, refuse, paginate) is a product call. Happy to do any of the three once you say which way. Verification: eval and observability suites pass, 251 across 15 files. I could not run a full |
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Incremental Code Review Summary (Updated Commits)
The PR was updated with substantial fixes addressing most of the original 6 blocking issues:
✅ Fixed issues:
- User-message event missing — Now loads thread data via
getThread()and parses thread messages to extract the user prompt when events don't contain a user-message event. - Schema migration missing — Uses
ensureColumnExists()to additive-migrate theuser_idcolumn on existingagent_eval_datasetstables before first insert. - Legacy tool errors — New shared
isToolDoneFailure()intool-done-error.tscorrectly handles legacy string-prefix failures + boolean flag. - CLI partial write behavior —
fs.writeFile()is atomic; the dataset is not yet persisted when the file write happens (fixed relative ordering).
Risk: Medium — This is backend data handling with straightforward scoping. Three NEW bugs were discovered during incremental review that must be fixed before merge.
| const datasetName = | ||
| input.options?.datasetName?.trim() || `from-trace:${runId}`; | ||
| const dataset: EvalDataset = { | ||
| id: crypto.randomUUID(), |
There was a problem hiding this comment.
🔴 Missing crypto import for randomUUID()
Line 481 calls crypto.randomUUID() without importing the crypto module. At runtime, this will crash with ReferenceError: crypto is not defined. Add import { randomUUID } from "node:crypto"; at the top and change line 481 to id: randomUUID(), (or use import crypto from "crypto"; and keep the current call, matching the pattern in resources/store.ts).
Additional Info
Verified via grep: the file has no crypto import despite using randomUUID() on line 481.
| }); | ||
| if (!result.ok) refuse(result.error); | ||
|
|
||
| await insertEvalDataset(result.value.dataset); |
There was a problem hiding this comment.
🟡 No idempotency check before inserting eval dataset
Direct insert of dataset without checking if one with the same name already exists. getEvalDatasetByName() is available and scoped by user_id, but never called. Retries or double-clicks create duplicate dataset rows. Check for existing dataset before insert and return it if found, making promotion idempotent: const existing = await getEvalDatasetByName(...); if (existing) return {...existing...};
Additional Info
Reported by 1 of 3 review agents; confirmed against store.ts exports.
|
|
||
| const [run, events, spans] = await Promise.all([ | ||
| getRunById(runId), | ||
| getRunEventsSince(runId, 0), |
There was a problem hiding this comment.
🟡 Unbounded event history loaded into memory
getRunEventsSince(runId, 0) fetches ALL events from sequence 0 with no limit parameter. Long production runs can load thousands of events into memory, causing performance issues or OOM in serverless. Add a limit parameter (e.g., { limit: 10000 }), or paginate events in chunks, with an explicit truncation error contract.
Additional Info
Reported by 1 of 3 review agents; confirmed against the call site.
Summary
CI evals (
agent-native eval/defineEval) only run prompts someone typed into a*.eval.tsfile. Observability already stores the production run that actually broke (traces, tool spans, post-hoc scores) and even hasEvalDatasethelpers on the store, but nothing promotes a completed trace into a CI case. The two layers are documented as complementary and never meet.This adds
promote-trace-eval: map a completed run's user prompt and successful tool names into adefineEvalfixture, persist it as anEvalDatasetrow, and letagent-native eval promote <runId> --write evals/from-trace.eval.tsemit the file the CI gate already discovers. Truncated, aborted, or prompt-less runs fail closed (not_found/run_not_completed/no_user_prompt/no_signal).The hosted action and
POST /_agent-native/observability/traces/:runId/promotenever write the app tree.--writeis the only filesystem hop.Surfaces
packages/core/src/eval/from-trace.ts(no DB import)promote-trace-eval(defineAction, scoped to the calling user)POST /_agent-native/observability/traces/:runId/promote(same auth as GET trace)agent-native eval promote <runId> [--write path] [--json] [--must-contain text]promote-trace-evalis grouped under existinglabsinCORE_ACTION_GROUPSrather than adding a newobservabilityFRAMEWORK_TOOL_GROUPSmember. That union is filtered at thirteen agent-chat composition sites; a dedicated group is a follow-up.Tests
pnpm --filter @agent-native/core exec vitest --runon:src/eval/from-trace.spec.tssrc/observability/actions/promote-trace-eval.spec.tssrc/cli/eval.spec.tssrc/observability/routes.spec.tssrc/client/observability/ObservabilityDashboard.spec.tsxsrc/observability/store.spec.ts51 tests passed in those files. Existing
agent-native eval [pattern]parse stays the same when argv[0] is notpromote.Evidence
*.eval.tsas complementary, no promotion pathLangSmith and Langfuse both treat "add this trace to a dataset" as the move that turns a production miss into a regression test. Agent-Native already has both ends of that move.
insertEvalDatasetexisted with no HTTP or CLI caller.Patch changeset on
@agent-native/core. Docs: From a production trace.Demo