feat(evals): group trajectories by run and cross-link Braintrust experiments#2177
Conversation
|
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 4/5
- This PR is likely safe to merge, with one moderate-risk edge case rather than a broad functional regression.
- In
packages/evals/index.eval.ts,experiment.jsonmay be written under a different root than trajectory outputs whenEVAL_TRAJECTORY_ROOTis set, which can cause confusing or inconsistent eval artifacts for users relying on aligned output locations. - Because the issue is scoped to a specific environment-variable path configuration (severity 5/10, high confidence) and not a clear break of core flows, the overall merge risk stays moderate-low.
- Pay close attention to
packages/evals/index.eval.ts- ensureexperiment.jsonand trajectory files resolve to the same root whenEVAL_TRAJECTORY_ROOTis enabled.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/evals/index.eval.ts">
<violation number="1" location="packages/evals/index.eval.ts:567">
P2: `experiment.json` can be written to a different root than the actual trajectories when `EVAL_TRAJECTORY_ROOT` is set.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Runner as Eval Runner (index.eval.ts / runner.ts)
participant TG as trajectoryGroup.ts
participant TR as TrajectoryRecorder
participant PH as Persist Adapter (persistTrajectory.ts)
participant FS as Local Filesystem
participant BT as Braintrust SDK (Eval())
Note over Runner,BT: Eval run setup — stamp env vars before tasks execute
Runner->>TG: buildTrajectoryGroupSlug(experimentName, model?)
TG-->>Runner: "experimentName__model" (or just "experimentName")
Runner->>Runner: set process.env.EVAL_TRAJECTORY_GROUP = slug
Runner->>Runner: set process.env.EVAL_EXPERIMENT_NAME / MODEL / PROVIDER
Note over TR,FS: Per-task trajectory persistence
Runner->>BT: Eval() — execute all tasks
BT-->>Runner: EvaluationResult (hashed experiment name/id/URLs)
alt For each task
alt TrajectoryRecorder path (framework runner)
Runner->>TR: recordTrajectory(taskSpec, runId)
TR->>TG: resolveTrajectoryDir(root, taskId, runId)
TG-->>TR: "<root>/<group>/<taskId>/<runId>"
TR->>FS: write trajectory files to computed dir
TR->>TG: writeTrajectoryMetadata(dir, {task, runId, status})
TG->>FS: write metadata.json (experiment, model, provider, env, task, runId, status)
else PersistAdapter path (external harness)
Runner->>PH: persistAdapterTrajectory(opts)
PH->>TG: resolveTrajectoryDir(root, taskId, runId)
TG-->>PH: "<root>/<group>/<taskId>/<runId>"
PH->>FS: write trajectory files
PH->>TG: writeTrajectoryMetadata(dir, {task, runId, status})
TG->>FS: write metadata.json
end
end
Note over Runner,FS: Post-run — Braintrust experiment link
Runner->>TG: resolveTrajectoryRoot()
TG-->>Runner: "<root>" (default ./.trajectories)
Runner->>TG: writeExperimentLink(root, resolvedBraintrustMetadata)
TG->>FS: mkdir <root>/<group>/
TG->>FS: write experiment.json (hashed name, id, URLs, requested name)
Note over TG,FS: Best-effort — errors silently ignored
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ed1530d to
76d71c1
Compare
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/evals/framework/trajectoryRecorder.ts">
<violation number="1" location="packages/evals/framework/trajectoryRecorder.ts:281">
P2: The new order-free flow still loses data when `persistResult()` runs before `finish()`: `finish()` overwrites `task_data.json` and drops `result`. If this order is supported, preserve/merge existing `task_data.json` during `finish()` or re-apply `result` after writing trajectory files.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const scoresDir = path.join(this.outputDir, "scores"); | ||
| // Route through the shared reservation so scores land in the same dir as | ||
| // the trajectory regardless of finish()/persistResult() call order. | ||
| const { directory } = await this.ensureReserved(); |
There was a problem hiding this comment.
P2: The new order-free flow still loses data when persistResult() runs before finish(): finish() overwrites task_data.json and drops result. If this order is supported, preserve/merge existing task_data.json during finish() or re-apply result after writing trajectory files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/framework/trajectoryRecorder.ts, line 281:
<comment>The new order-free flow still loses data when `persistResult()` runs before `finish()`: `finish()` overwrites `task_data.json` and drops `result`. If this order is supported, preserve/merge existing `task_data.json` during `finish()` or re-apply `result` after writing trajectory files.</comment>
<file context>
@@ -225,13 +248,38 @@ export class TrajectoryRecorder {
- const scoresDir = path.join(this.outputDir, "scores");
+ // Route through the shared reservation so scores land in the same dir as
+ // the trajectory regardless of finish()/persistResult() call order.
+ const { directory } = await this.ensureReserved();
+ const scoresDir = path.join(directory, "scores");
await fs.mkdir(scoresDir, { recursive: true });
</file context>
There was a problem hiding this comment.
Does not reproduce — leaving as-is. persistResult() writes <dir>/scores/result.json, while finish() → writeTrajectoryDir() writes task_data.json, trajectory.json and screenshots/ and never touches scores/. The result keys writeTrajectoryDir does write are step.toolOutput.result (per-step tool output), which is unrelated to the EvaluationResult that persistResult() persists — so a persistResult()-before-finish() ordering would not drop it.
Independently, the order is never reversed at the only call site (verifierAdapter.ts: await recorder.finish(...) then await recorder.persistResult(...)). The related real issue was the shared reservation, which is fixed in f3e4824.
There was a problem hiding this comment.
Re-checked this with a test rather than leaving it at my earlier reading — pushed in the latest commit.
The test runs persistResult() before finish() (the reverse of the adapter's order) and asserts the score survives, every trajectory file is present, and the reservation did not split. It passes, so the reported data loss does not reproduce:
persistResult()writes<dir>/scores/result.json.finish()→writeTrajectoryDir()writestask_data.json,trajectory.json,screenshots/— and never touchesscores/.- The
resultkeyswriteTrajectoryDirdoes write arestep.toolOutput.result(per-step tool output), unrelated to theEvaluationResultthatpersistResult()persists.
So there is no merge/re-apply needed. Keeping the test regardless of the disagreement: the PR advertises that result persistence is order-free, and an advertised ordering guarantee with no test is one refactor away from being false. Happy to be shown a concrete failing sequence if you think I have missed one.
Persist trajectories under <root>/<experiment>__<model>/<task>/<runId> instead of scattered per-task timestamps, write a metadata.json into each trajectory dir, and write an experiment.json at the group root linking the local run to the resolved Braintrust experiment (name, id, URLs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
google_maps_3 rubric now grants full/partial credit around the 'Open now' filter vs 24-hour businesses, kfc_tenders_combo requires scheduling the pickup, and the sf_library_card tasks get realistic default step budgets (1->10, 20->25). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ones Trajectory dirs were keyed by a millisecond timestamp runId; two trials of the same task in one run (or a re-run reusing a fixed runId) could compute the same <group>/<task>/<runId> path and silently clobber each other — writeTrajectoryDir's recursive mkdir never noticed. reserveTrajectoryDir now atomically creates the leaf dir (non-recursive mkdir, EEXIST retry with -2/-3 suffixes); the recorder reserves once via ensureReserved(), so finish() stays idempotent and finish()/persistResult() are order-free. The trajectory group is captured at recorder construction (the runner restamps EVAL_TRAJECTORY_GROUP per experiment), the adapter persist path uses the same env-aware resolveTrajectoryRoot() as the recorder, and metadata.json records runDir + attempt so N on-disk trajectories map legibly to N trials. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ning The healthline newsletter task now stops before finalizing the subscription (no real sign-up side effect); sf_library_card gets refined instructions; webmd_ovulation_calculator includes the year in the period date so January runs don't point ~11 months into the future. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a9d5c7b to
cd40dc5
Compare
Prettier 3.5.3 line-wrapping only, no semantic change. Unblocks the failing Lint check, which was cancelling the rest of the Tests workflow.
…cross-link generateExperimentName is deterministic (evalName ?? category ?? "all"), so the group slug was a per-SUITE bucket, not a per-run one. Since writeExperimentLink wrote a fixed <root>/<group>/experiment.json, re-running the same suite overwrote it -- leaving the earlier run's trajectories (still present under their own runId leaves) labeled with the NEWER run's Braintrust experiment id. Wrong provenance is worse than none. - Add generateRunToken() and append it to the group slug, so the layout is now <root>/<experiment>__<model>__<runToken>/<task.id>/<runId>/. The token is generated exactly once per run in each entrypoint and reused for the completion-time link, so a run can never split across two group dirs. - Gate writeExperimentLink on shouldPersistTrajectory(undefined). It previously wrote unconditionally, so CI runs and core-tier runs (which never persist trajectories) still created a stray, otherwise-empty .trajectories tree. The gate lives inside writeExperimentLink so a future caller cannot forget it, and gates ONLY the cross-link -- the recorder/adapter still decide persistence independently. - writeExperimentLink now takes an explicit group instead of re-reading process.env at completion time, matching reserveTrajectoryDir's discipline. - Update stale layout docs (tui/commands/verify.ts examples, verifier types.ts). - New tests/framework/trajectoryGroup.test.ts covers the anti-clobber property: two run tokens keep separate experiment.json files, explicit group beats env, and the link is a no-op when persistence is off.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…thmetic Addresses PR review comments. ensureReserved() (cubic P1) did check-then-await: it tested `this.reserved`, then awaited reserveTrajectoryDir() before caching the result. Two overlapping callers therefore both observed no reservation, both reserved, and one recorder's artifacts split across two run dirs. Confirmed by mutation test: with the old implementation the new test fails with expected [ 'run-1', 'run-1-2' ] to deeply equal [ 'run-1' ] Production call sites happen to await finish() before persistResult(), so it was not firing today -- but nothing enforced that. Now the in-flight PROMISE is cached (assigned synchronously, before any await), so overlapping callers share one reservation. A rejection clears the cache so a later attempt can retry. Extract the webmd ovulation date arithmetic into framework/taskDates.ts and test it (cubic P3). The original hardcoded 'Mar 1' silently rotted out of WebMD's ~90-day accepted window; the replacement logic has two traps worth locking down -- month-end overflow (setMonth() on Mar 31 asks for 'Feb 31' and overflows forward) and the January rollback to December of the PRIOR year -- so the tests cover both, plus a sweep asserting the generated date is always in the past and within the 90-day window. Not changed, with reasons: - cubic P2 (finish() overwrites task_data.json, dropping the result) does not reproduce: persistResult() writes scores/result.json, while writeTrajectoryDir writes task_data.json/trajectory.json/screenshots and never touches scores/. The 'result' keys it does write are step.toolOutput.result, unrelated to EvaluationResult. The call order is also never reversed. - No changeset: @browserbasehq/stagehand-evals is private, and the only packages/core edit is a doc comment, so nothing ships a version bump.
|
@miguelg719 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Architecture diagram
sequenceDiagram
participant Entrypoint as "Eval Entrypoint (runner.ts / index.eval.ts)"
participant TG as trajectoryGroup.ts
participant Recorder as TrajectoryRecorder
participant Adapter as persistAdapterTrajectory
participant FS as "Filesystem (disk)"
participant Braintrust as "Braintrust SDK"
Note over Entrypoint,FS: NEW: Run-scoped trajectory grouping
Entrypoint->>TG: generateRunToken()
TG-->>Entrypoint: "20260715-110342"
Entrypoint->>TG: buildTrajectoryGroupSlug(experiment, model, runToken)
TG-->>Entrypoint: "agent__openai_gpt-4.1-mini__20260715-110342"
Entrypoint->>Entrypoint: set process.env.EVAL_TRAJECTORY_GROUP = group
Note over Entrypoint: Also sets EVAL_EXPERIMENT_NAME, EVAL_MODEL_OVERRIDE, EVAL_PROVIDER
par For each task
Entrypoint->>Recorder: new TrajectoryRecorder(taskSpec)
Note over Recorder: Captures group from env at construction (snapshot)
Recorder->>Recorder: ensureReserved() - single-flight caching
Recorder->>TG: reserveTrajectoryDir(root, taskId, runId, group)
critical Atomic directory creation
TG->>FS: mkdir <root>/<group>/<taskId>/<runId> (non-recursive)
alt Success
FS-->>TG: OK
else Directory exists (EEXIST)
loop attempt=2..N
TG->>FS: mkdir <root>/<group>/<taskId>/<runId>-<attempt>
alt Success
FS-->>TG: OK
else EEXIST
Note over TG,FS: EEXIST, retry
end
end
end
end
TG-->>Recorder: { directory, attempt }
Recorder->>FS: writeTrajectoryDir(directory, trajectory)
Recorder->>TG: writeTrajectoryMetadata(directory, { task, runId, runDir, attempt, status })
TG-->>FS: writes metadata.json
Recorder->>FS: persistResult(...) - also uses same reservation
opt adapter/harness persist
Adapter->>TG: resolveTrajectoryDir(root, taskId, runId)
Adapter->>TG: reserveTrajectoryDir(...)
Adapter->>FS: writeTrajectoryDir + writeTrajectoryMetadata
end
end
Entrypoint->>Braintrust: Eval() - resolves experiment name (hashed)
Braintrust-->>Entrypoint: summary (experimentName, experimentId, URLs)
Entrypoint->>TG: writeExperimentLink(root, group, {...})
Note over Entrypoint,TG: Cross-link local trajectories to Braintrust experiment
alt Persistence enabled (shouldPersistTrajectory)
TG->>FS: mkdir <root>/<group>
TG->>FS: write <root>/<group>/experiment.json
else Persistence disabled (CI, core-tier)
TG-->>Entrypoint: No-op (no stray .trajectories/)
end
Note over Recorder,FS: Collision avoidance: re-run with same runId -> `run-1-2` dir
Note over Entrypoint: Generated run token ensures runs never clobber each other’s experiment.json
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…g + link
Addresses the second round of review findings, plus issues a holistic pass
turned up before they were raised.
Model provenance (P1). Both entrypoints built the group slug and every
trajectory's metadata.json `model` from the run-global EVAL_MODEL_OVERRIDE. That
var does NOT drive the model matrix -- cli-legacy sets only it, and getModelList()
never reads it -- so `--model X` labelled every trajectory `model: X` while the
testcases actually ran the default matrix (gemini-2.5-flash, gpt-4.1-mini,
claude-haiku-4-5). Same false-provenance class as the experiment.json clobber.
Now resolveUnambiguousModel() derives it from the testcases that will actually
run and omits it unless it is unambiguous for the whole run; metadata reads a new
EVAL_TRAJECTORY_MODEL (the resolved model) instead of the merely-requested one.
Absent beats wrong.
Run token collision (P2). YYYYMMDD-HHMMSS could collide for two same-suite runs
started in the same second, reintroducing the clobber the token exists to prevent.
Added random entropy: 20260716-110342-9f3a1c.
Group slug path escape. sanitizeSlug whitelisted ".", so a caller-supplied
EVAL_TRAJECTORY_GROUP of ".." resolved to ".." and wrote <root>/../<task> -- a
level ABOVE the trajectory root, silently, since the link write is best-effort.
"." collapsed the group into the root, re-scattering trajectories and sharing one
experiment.json across every run: both states this module exists to prevent.
Pure-dot slugs now fall back to the "default" floor.
Empty group dirs (P3). Gating the link on shouldPersistTrajectory fixed CI but not
local runs that persist nothing: core-only, and legacy runs of non-agent
categories (act/extract never construct a recorder). writeExperimentLink now also
requires the group dir to exist -- it is created by the first trajectory
reservation, so its absence means nothing was recorded. That is a universal
backstop needing no tier signal; the runner additionally passes an explicit
{ persist } for core-only, as requested.
REPL env leak. The entrypoints stamp EVAL_TRAJECTORY_GROUP / EVAL_EXPERIMENT_NAME
/ EVAL_TRAJECTORY_MODEL straight onto process.env, but withEnvOverrides restored
only the keys it set -- contradicting its own docstring about not leaking between
REPL commands. It now snapshots and restores the run-stamped keys too, fixing both
call sites without touching them.
Also: legacy testcases are now materialized before Eval() (the group's model can
only come from them, and the stamp must precede any recorder construction). Kept
INSIDE the existing try so a dataset-load failure is still reported rather than
escaping as an unhandled rejection. Braintrust invokes data() exactly once, so
returning the cached array is equivalent -- and strictly more deterministic, since
generateFilteredTestcases routes through random sampling.
Docs: persistTrajectory no longer presents the group as a guaranteed structure.
Tests: +7. Every one mutation-tested -- reverting the entropy, the pure-dot guard,
the dir-existence backstop, the unambiguous-model guard, or the env restore each
fails its test. Replaced a vacuous core-only assertion: resolveUnambiguousModel
(["none","NONE"]) returns undefined from the size check whether or not the
sentinel filter exists, so it proved nothing; ["none"] (the real core-only shape)
is the discriminating input.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Addresses the review note that 3 random bytes (2^24) leave a ~3% birthday collision across 1000 simultaneous same-second starts of the same experiment+model -- which would silently share a group and clobber its experiment.json, the exact bug the token exists to prevent. The premise is unrealistic (real concurrency is 2-8 runs => ~0.0002%), but the width is free, so size it for the pathological case: 8 bytes drops even the 1000-run case to ~3e-14. Did NOT take the suggested alternative of atomically reserving the group dir. That would require the entrypoint to create <root>/<group>/ before any task runs, which defeats writeExperimentLink's "group dir exists => something was recorded" backstop and reintroduces the empty-group dirs just fixed. Reservation stays where it can be atomic without that cost: the per-trajectory leaf (reserveTrajectoryDir). The existing token tests inject entropy, so they would not have caught the default width shrinking. Added a test asserting the real default (16 hex chars) plus distinctness across 200 calls; it fails when reverted to 3 bytes.
The PR advertises that finish() is idempotent and result persistence is order-free, and a review comment disputed the latter: it claimed finish() overwrites task_data.json and drops the result when persistResult() runs first. I had rejected that from reading the code. Replacing that argument with evidence: this test runs persistResult() BEFORE finish() and asserts the score survives, all trajectory files are present, and the reservation did not split. It passes, so the claim does not reproduce -- persistResult() writes scores/result.json while writeTrajectoryDir writes task_data.json / trajectory.json / screenshots and never touches scores/. The 'result' keys it does write are step.toolOutput.result, unrelated to EvaluationResult. Worth having regardless of the disagreement: an advertised ordering guarantee with no test is one refactor away from being false.
The comments added across this PR over-explained: trajectoryGroup.ts was 49% comment lines and taskDates.ts 68%, against ~17-25% in comparable framework files (verifierAdapter.ts, rubricCache.ts). Much of it restated what the code says or narrated the fix -- which belongs in the commit history, not the source. Kept the non-obvious why (why a pure-dot slug is rejected, why the run token must be generated once, why the group is passed explicitly rather than re-read from env, why metadata reads EVAL_TRAJECTORY_MODEL and not EVAL_MODEL_OVERRIDE, why the reservation caches a promise rather than its value). Cut the enumeration and the defensive justification around it. Also moves the persistResult() doc comment back onto persistResult(), where an earlier edit had stranded it above ensureReserved(). Comments only -- no code lines changed; 393/393 tests still pass.
What
Stacked on #2138.
<root>/<experiment>__<model>/<task.id>/<runId>/instead of scattered per-task timestamp dirs, so a run's trajectories live in one folder and concurrent multi-model runs of the same suite never interleave on disk.metadata.jsoninto every trajectory dir (experiment, model, provider, environment, task, runId, status) so a trajectory's run never has to be reverse-engineered after the fact.experiment.jsonat the group root cross-linking the local trajectories to the resolved Braintrust experiment (hashed name, id, project, URLs) onceEval()resolves.framework/trajectoryGroup.tsowns the slugging/layout/metadata helpers;TrajectoryRecorder, the external-harness persister, and both eval entrypoints (index.eval.ts,framework/runner.ts) consume it.Why
Mapping on-disk trajectories back to their Braintrust experiment previously required guessing by timestamp, and concurrent runs of different models wrote into the same root with no on-disk marker — making post-hoc analysis error-prone.
Notes
.trajectories/*keeps working (one extra path level); the trajectoryRecorder unit test asserts the new layout +metadata.json.Summary by cubic
Groups eval trajectories by experiment, unambiguous model, and a run-unique token, and cross-links each run to its Braintrust experiment. Adds atomic dir reservation, single-flight caching, and env hygiene so runs never overwrite, mislabel, or leak; the run token uses 64‑bit entropy and result persistence is order-free (also trims internal comments; no logic changes).
New Features
<root>/<experiment>[__<model>]__<runToken>/<task.id>/<runId>/; writemetadata.jsonper trajectory and anexperiment.jsonat the group root linking to the resolved Braintrust experiment; implemented inframework/trajectoryGroup.tsand used by the recorder, adapter persister, runner, andindex.eval.ts.EVAL_TRAJECTORY_MODEL), not the requested override; stampEVAL_EXPERIMENT_NAME/EVAL_TRAJECTORY_GROUP;writeExperimentLink()takes an explicit group, respectsEVAL_TRAJECTORY_ROOT, and only writes if persistence is on and the group dir exists.taskDates.tsfor safe rolling dates (WebMD); update Healthline to stop before final subscribe; clarify Google Maps rubric; KFC requires pickup scheduling; SFPL uses the live application URL; defaultmaxSteps:agent/sf_library_card1→10,agent/sf_library_card_multiple20→25. TUIverifyhelp and docs reflect the new layout.Bug Fixes
reserveTrajectoryDiradds-2/-3on collision) with single-flight caching in the recorder;finish()is idempotent; result persistence is order-free (now covered by tests);metadata.jsonrecordsrunDirandattempt.".","..") to avoid path escapes; fall back to the"default"group.withEnvOverrides()snapshots/restores run-stamped env (EVAL_TRAJECTORY_GROUP,EVAL_EXPERIMENT_NAME,EVAL_TRAJECTORY_MODEL) so runs don’t leak between commands; recorder and adapter both honorEVAL_TRAJECTORY_ROOT.Written for commit 2d17642. Summary will update on new commits.