feat: split trial output files by joshblack · Pull Request #212 · primer/agent-eval

joshblack · 2026-09-04T02:17:46Z

This PR updates benchmark and experiment result bundles so each trial is stored in its own JSON file inside its artifact directory. output.json is now a compact manifest with run metadata and trial file references.

This keeps the top-level result file small while preserving the complete portable bundle. The CLI and website now use file-aware read and write APIs, and trial references are validated against resolved filesystem paths so symlinks cannot escape the bundle directory.

This is the first open entry in stack 214, followed by #201.

Changelog

New

Add benchmark and experiment APIs for reading and writing portable result bundles.
Add one artifacts/<trial-id>/<trial-id>.json file for each trial.

Changed

Update CLI output to write compact manifests with trial file references.
Update the website to load referenced trial files.
Update result bundle documentation for the new layout.
Validate trial file paths and resolved symlink targets before reading or writing bundle data.

Removed

Remove the legacy benchmark and experiment serialize and deserialize APIs.
Remove support for embedded trial output and the website's legacy result format.

Copilot

Copilot review overview

🟡 Changes recommended

script/setup calls a migration script that is not present in the repo, and readTrialFiles/writeTrialFiles need path-escape validation to avoid directory traversal and broken bundle portability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	`packages/agent-eval/src/result-files.ts` — `writeTrialFiles` can produce trial references that escape the output directory (e.g., when a trial…
	`script/setup` — `script/setup` invokes `script/migrate-result-output.js`, but that file is not present in the…

What changed in this PR

This PR changes the @primer/agent-eval result bundle format so each trial is written to its own JSON file under the trial’s artifact directory, while output.json becomes a compact manifest that references those per-trial files. It updates the CLI and website to use new file-aware read/write APIs, and documents the new bundle layout.

Changes:

Add shared result-file helpers plus read() / write() APIs for benchmark and experiment outputs.
Update the CLI and website loaders to read/write the new manifest + per-trial JSON layout.
Update result-bundle documentation and add a setup-time migration hook for retained bundles.

File	Description
website/src/runs.ts	Switch experiment run loading from manual JSON parsing to the new `read()` API.
website/src/benchmark-results.ts	Switch benchmark output loading to the new `read()` API.
script/setup	Adds a post-download migration step for retained results (currently references a missing script).
packages/agent-eval/src/result-files.ts	New shared helper for writing/reading per-trial JSON files referenced by a manifest.
packages/agent-eval/src/index.ts	Re-export new read/write APIs and `ResultFileOptions` type.
packages/agent-eval/src/experiment.ts	Add experiment `read()` / `write()` implementation using per-trial files.
packages/agent-eval/src/experiment-output.test.ts	Add coverage for experiment manifest + per-trial write/read roundtrip.
packages/agent-eval/src/cli.ts	Use new `write*Output()` APIs instead of serializing monolithic `output.json`.
packages/agent-eval/src/benchmark.ts	Add benchmark `read()` / `write()` implementation using per-trial files.
packages/agent-eval/src/benchmark.test.ts	Add coverage for benchmark manifest + per-trial write/read roundtrip.
packages/agent-eval/README.md	Document new bundle layout (manifest + per-trial JSON files).
.changeset/violet-bugs-vanish.md	Changeset documenting the new bundle layout and APIs as a minor release.

Suppressed comments (1)

packages/agent-eval/src/result-files.ts:47

readTrialFiles resolves the trial filepath directly from a manifest-provided reference without checking that it stays within the bundle directory. This allows ../ traversal (or absolute paths) to read arbitrary files when output.json is untrusted. Normalize path separators and reject references that resolve outside outputDirectory before reading.

      const trialFilePath = path.resolve(outputDirectory, reference)
      const contents = await host.fs.readFile(trialFilePath, 'utf-8')
      const trial = parse(JSON.parse(contents))

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🔵 Needs a closer look

script/setup now calls script/migrate-result-output.js, but that script is not present in the repository, so setup will fail at runtime.

Review tier: Lite
Findings: 2

Pre-existing issues (2)

Severity	Finding
	`script/setup` — `script/setup` invokes `script/migrate-result-output.js`, but that file is not present in the… View comment
	`packages/agent-eval/src/result-files.ts` — `writeTrialFiles` can produce trial references that escape the output directory (e.g., when a trial… View comment

Suppressed comments (2)

script/setup:41

script/setup invokes script/migrate-result-output.js, but that file does not exist in the repository checkout. As written, setup will fail at runtime with a Node "module not found" error after downloading artifacts.

  node "$repository_root/script/migrate-result-output.js" "$repository_root/results"

packages/agent-eval/src/result-files.ts:46

readTrialFiles resolves trial references with path.resolve(outputDirectory, reference) without validating the reference. A crafted output.json could use ../ segments or an absolute path to read files outside the result bundle directory.

      const trialFilePath = path.resolve(outputDirectory, reference)
      const contents = await host.fs.readFile(trialFilePath, 'utf-8')

Copilot

Copilot review overview

🟡 Changes recommended

The website loaders appear to drop backward compatibility for previously generated result bundles, and the new path containment checks don’t address symlink-based escapes despite the security claim in the PR description.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 · 2

New issues introduced by this change (3)

Severity	Finding
	`packages/agent-eval/src/trial.ts` — `resolvePathWithinDirectory` blocks `..` and absolute paths, but it does not prevent symlink-based…
	`website/src/benchmark-results.ts` — This switches benchmark loading to the new `read(output.json)` format only; runs produced before…
	`website/src/runs.ts` — The website now exclusively uses the new file-based `read(output.json)` API; older experiment…

Issues resolved since last review (2)

Severity	Finding
	`script/setup` — `script/setup` invokes `script/migrate-result-output.js`, but that file is not present in the… View resolved comment
	`packages/agent-eval/src/result-files.ts` — `writeTrialFiles` can produce trial references that escape the output directory (e.g., when a trial… View resolved comment

Copilot

Copilot review overview

🟡 Changes recommended

The new bundle path validation can be bypassed via symlinks and the new read() APIs currently won’t load legacy (embedded-trials) output bundles, breaking upgrades for existing results.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 · 4

New issues introduced by this change (2)

Severity	Finding
	`packages/agent-eval/src/benchmark.ts` — benchmark.read() currently only supports the new manifest format (trials: Record<string,string>).…
	`packages/agent-eval/src/experiment.ts` — experiment.read() currently only supports the new manifest format (trials: Record<string,string>).…

Pre-existing issues (3)

Severity	Finding
	`packages/agent-eval/src/trial.ts` — `resolvePathWithinDirectory` blocks `..` and absolute paths, but it does not prevent symlink-based… View comment
	`website/src/runs.ts` — The website now exclusively uses the new file-based `read(output.json)` API; older experiment… View comment
	`website/src/benchmark-results.ts` — This switches benchmark loading to the new `read(output.json)` format only; runs produced before… View comment

Suppressed comments (1)

packages/agent-eval/src/trial.ts:85

Path traversal protection in resolvePathWithinDirectory is purely lexical (path.resolve/path.relative) and does not account for symlinks. If an attacker can place a symlink inside the output directory (e.g. artifacts/ -> /etc), readTrialFiles/writeTrialFiles can still read/write outside the bundle even though the resolved path appears within the directory.

  const resolvedDirectory = path.resolve(directory)
  const resolvedFilepath = path.resolve(resolvedDirectory, ...normalizedFilepath.split(path.posix.sep))
  const relativePath = path.relative(resolvedDirectory, resolvedFilepath)
  if (relativePath === '..' || relativePath.startsWith(`..${path.sep}`) || path.isAbsolute(relativePath)) {
    throw new Error(`${description} "${filepath}" must be within the output directory`)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d12a97a-b8d3-45ad-a09a-3c76d1411705

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91f94506-3717-4a44-8761-1a4d33e0f319

Prevent trial file references from escaping through symbolic links and continue reading embedded and legacy result formats. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

It changes persisted output formats and introduces filesystem/symlink validation paths that should get final human verification.

Review tier: Lite
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	`packages/agent-eval/src/cli.ts` — `writeBenchmarkOutput()` already creates the parent directory for `env.outputPath` (via…
	`website/src/runs.ts` — `find()` reads `output.json` and then `parseOutput()` calls `read(outputFile)`, which re-reads the…

Issues resolved since last review (5)

Severity	Finding
	`packages/agent-eval/src/experiment.ts` — experiment.read() currently only supports the new manifest format (trials: Record<string,string>).… View resolved comment
	`packages/agent-eval/src/benchmark.ts` — benchmark.read() currently only supports the new manifest format (trials: Record<string,string>).… View resolved comment
	`website/src/runs.ts` — The website now exclusively uses the new file-based `read(output.json)` API; older experiment… View resolved comment
	`website/src/benchmark-results.ts` — This switches benchmark loading to the new `read(output.json)` format only; runs produced before… View resolved comment
	`packages/agent-eval/src/trial.ts` — `resolvePathWithinDirectory` blocks `..` and absolute paths, but it does not prevent symlink-based… View resolved comment

Suppressed comments (1)

packages/agent-eval/src/cli.ts:189

writeExperimentOutput() creates the parent directory for env.outputPath, so the existsSync/mkdir pre-check is redundant and can be removed to reduce duplicated filesystem work.

  await writeExperimentOutput(env.outputPath, output)

Drop embedded result compatibility and require portable result bundles through the read and write APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 983b31cf-cd77-4cfe-854f-3c7fb34543a1

Copilot

Copilot review overview

🟡 Changes recommended

writeTrialFiles does not validate that trialId is a safe single path segment, enabling path traversal within the bundle and potential overwrites (e.g., ../output.json).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`packages/agent-eval/src/trial.ts` — writeTrialFiles uses the trialId as part of the destination filename (path.join(...,…

Issues resolved since last review (2)

Severity	Finding
	`website/src/runs.ts` — `find()` reads `output.json` and then `parseOutput()` calls `read(outputFile)`, which re-reads the… View resolved comment
	`packages/agent-eval/src/cli.ts` — `writeBenchmarkOutput()` already creates the parent directory for `env.outputPath` (via… View resolved comment

+    [...trials].map(async ([trialId, trial]) => {
+      if (trial.id !== trialId) {
+        throw new Error(`Trial map key "${trialId}" does not match trial id "${trial.id}"`)
+      }
+


Copilot

Copilot review overview

🟡 Changes recommended

The website loaders now depend on reading many referenced trial files, and without local try/catch a single incomplete/corrupt bundle can throw and break listing/building pages.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 3

New issues introduced by this change (2)

Severity	Finding
	`website/src/benchmark-results.ts` — benchmark.read() now loads referenced per-trial files; if a bundle is incomplete or corrupted, this…
	`website/src/runs.ts` — Reading a run now pulls in per-trial files via experiment.read(). If any trial reference is…

Pre-existing issues (1)

Severity	Finding
	`packages/agent-eval/src/trial.ts` — writeTrialFiles uses the trialId as part of the destination filename (path.join(...,… View comment

 async function readBenchmarkOutput(candidate: OutputCandidate, benchmarkId: string): Promise<BenchmarkOutput | null> {
-  const contents = await fs.readFile(candidate.filepath, 'utf-8')
-  const parsed: unknown = JSON.parse(contents)
-  const output = deserialize(parsed)
+  const output = await read(candidate.filepath)
  return output.benchmarkId === benchmarkId ? output : null


  const outputFile = path.join(directory, 'output.json')
-  const contents = await fs.readFile(outputFile, 'utf-8')
-  const output = parseOutput(contents)
+  const output = normalizeOutput(await read(outputFile))
  if (output.experiment.id !== experimentId) {


This was referenced Sep 4, 2026

feat: update project to support new experiment and benchmark structure #176

Merged

perf: prebuild local sandbox images #201

Merged

joshblack force-pushed the feat/split-trial-output branch 4 times, most recently from 1daaa0a to 85a914c Compare September 4, 2026 03:32

joshblack marked this pull request as ready for review September 4, 2026 03:36

Copilot AI lite review requested due to automatic review settings September 4, 2026 03:36

Copilot started reviewing on behalf of joshblack September 4, 2026 03:36 View session

joshblack mentioned this pull request Sep 4, 2026

feat: add durable plan sharding #204

Merged

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/agent-eval/src/result-files.ts Outdated

Comment thread script/setup Outdated

Copilot AI review requested due to automatic review settings September 4, 2026 03:48

joshblack force-pushed the feat/split-trial-output branch from e02cf06 to fb45587 Compare September 4, 2026 03:48

Copilot started reviewing on behalf of joshblack September 4, 2026 03:49 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 04:04

Copilot started reviewing on behalf of joshblack September 4, 2026 04:05 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/agent-eval/src/trial.ts

Comment thread website/src/benchmark-results.ts

Comment thread website/src/runs.ts

Copilot AI review requested due to automatic review settings September 4, 2026 04:13

joshblack force-pushed the feat/split-trial-output branch from dce88f9 to 768914b Compare September 4, 2026 04:13

Copilot started reviewing on behalf of joshblack September 4, 2026 04:13 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/agent-eval/src/benchmark.ts

Comment thread packages/agent-eval/src/experiment.ts

Copilot AI review requested due to automatic review settings September 4, 2026 04:28

Copilot started reviewing on behalf of joshblack September 4, 2026 04:28 View session

joshblack force-pushed the feat/split-trial-output branch from 5fd303a to 872e7ff Compare September 4, 2026 04:29

Base automatically changed from feat/add-benchmark-support to main September 4, 2026 04:32

joshblack added 2 commits September 3, 2026 23:34

feat: split trial output files

cdbae63

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d12a97a-b8d3-45ad-a09a-3c76d1411705

fix: nest trial output in artifact directories

88369d9

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d12a97a-b8d3-45ad-a09a-3c76d1411705

joshblack and others added 4 commits September 3, 2026 23:34

fix: migrate all retained result bundles

502dd0a

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d12a97a-b8d3-45ad-a09a-3c76d1411705

Delete script/migrate-result-output.js

b8312f7

fix: validate trial output paths

5eb826f

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91f94506-3717-4a44-8761-1a4d33e0f319

fix: preserve result bundle compatibility

05ada4d

Prevent trial file references from escaping through symbolic links and continue reading embedded and legacy result formats. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

joshblack force-pushed the feat/split-trial-output branch from 872e7ff to 05ada4d Compare September 4, 2026 04:34

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/agent-eval/src/cli.ts

Comment thread website/src/runs.ts Outdated

Copilot AI review requested due to automatic review settings September 4, 2026 04:34

Copilot started reviewing on behalf of joshblack September 4, 2026 04:35 View session

refactor: remove legacy output APIs

d99a20e

Drop embedded result compatibility and require portable result bundles through the read and write APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 983b31cf-cd77-4cfe-854f-3c7fb34543a1

joshblack merged commit 6414031 into main Sep 4, 2026
8 checks passed

joshblack deleted the feat/split-trial-output branch September 4, 2026 04:41

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/agent-eval/src/trial.ts

Comment on lines +169 to +173

[...trials].map(async ([trialId, trial]) => {

if (trial.id !== trialId) {

throw new Error(`Trial map key "${trialId}" does not match trial id "${trial.id}"`)

}

Copilot AI review requested due to automatic review settings September 4, 2026 04:41

Copilot started reviewing on behalf of joshblack September 4, 2026 04:41 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: split trial output files - #212

feat: split trial output files#212
joshblack merged 7 commits into
mainfrom
feat/split-trial-output

joshblack commented Sep 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

joshblack commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

New

Changed

Removed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joshblack commented Sep 4, 2026 •

edited

Loading