feat: add durable plan sharding - #204
Conversation
6efd88a to
582745a
Compare
582745a to
671a436
Compare
671a436 to
4b66a91
Compare
4b66a91 to
cc5f88c
Compare
cc5f88c to
9e8d108
Compare
9e8d108 to
601b11f
Compare
601b11f to
1ed8a3d
Compare
1ed8a3d to
d895d50
Compare
d895d50 to
1e2e5a8
Compare
dc8028e to
418abef
Compare
04fed6b to
67cdf44
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The experiment workflow’s merge job is missing a dependency install step (and inconsistent pnpm setup), which is likely to break CI execution.
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 |
|---|---|
.github/workflows/experiment.yml — The experiment workflow merge job runs pnpm run build without installing dependencies, and it… |
| - name: set up pnpm | ||
| uses: pnpm/setup@703c52620218391530e48b9e8870d5c0082e1b9b # v2.1.0 | ||
| with: | ||
| cache: true | ||
| require-lockfile: true | ||
| - name: set up Node.js | ||
| uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| - name: build project | ||
| run: pnpm run build |
9b90d80 to
6bd0145
Compare
6bd0145 to
f25acb0
Compare
f25acb0 to
c673f37
Compare
c673f37 to
a727bd5
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The experiment workflow’s merge job is missing a dependency install step and is inconsistent with the pnpm setup used in the other jobs, which is likely to break CI.
Review tier: Lite
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
.github/workflows/experiment.yml — The experiment workflow merge job runs pnpm run build without installing dependencies, and it… View comment |
Suppressed comments (1)
.github/workflows/experiment.yml:129
- The merge job builds without installing dependencies, and it uses pnpm/setup (v2) while the other jobs use pnpm/action-setup. This will likely fail at
pnpm run buildand makes the workflow inconsistent across jobs; align the merge job with the plan/run jobs by using pnpm/action-setup, enabling pnpm caching on setup-node, and runningpnpm install --frozen-lockfilebefore the build.
- name: set up Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
node-version-file: '.nvmrc'
- name: build project
Add reusable plan artifacts, deterministic shard execution, result merging, and four-way benchmark and experiment workflows. 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7eb8999b-2277-4d41-915f-fb35cac43373
Use output directories to derive shard manifest names, rename the merge CLI mode, and keep result merging with durable plan logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2a354dad-88d9-4648-85d5-0a48ff1f03cf
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep parent directory creation for plan and merged outputs without redundant existence checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 983b31cf-cd77-4cfe-854f-3c7fb34543a1
a727bd5 to
4c9bb02
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The experiment workflow’s merge job uses a different pnpm setup path than the plan/run jobs, which risks inconsistent pnpm behavior between shards and merge.
Review tier: Lite
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
.github/workflows/experiment.yml — The experiment workflow merge job runs pnpm run build without installing dependencies, and it… View comment |
Suppressed comments (1)
.github/workflows/experiment.yml:124
- The merge job uses
pnpm/setupwhile the plan/run jobs usepnpm/action-setup+setup-nodepnpm caching. Mixing pnpm setup actions in the same workflow can lead to different pnpm versions/install behavior between shards and the merge job, which risks non-reproducible merges or unexpected failures when the action defaults change. Consider aligning the merge job to the same pnpm+Node setup pattern as the other jobs (including explicitpnpm install --frozen-lockfile).
- name: set up pnpm
uses: pnpm/setup@703c52620218391530e48b9e8870d5c0082e1b9b # v2.1.0
with:
cache: true
require-lockfile: true
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The updated experiment workflow merge job is likely to fail on clean runners (missing dependency installation and inconsistent pnpm setup), and shard-merge should validate trial reference paths before producing a merged manifest.
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/plan.ts — mergeTrialReferences currently accepts trial file references verbatim. A shard output that… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
.github/workflows/experiment.yml — The experiment workflow merge job runs pnpm run build without installing dependencies, and it… View comment |
Suppressed comments (1)
.github/workflows/experiment.yml:130
- The experiment workflow merge job runs
pnpm run buildwithout installing dependencies, and it usespnpm/setupwhile the other jobs usepnpm/action-setup+setup-nodepnpm cache. This is likely to fail in a clean runner becausenode_moduleswill be missing.
- name: set up pnpm
uses: pnpm/setup@703c52620218391530e48b9e8870d5c0082e1b9b # v2.1.0
with:
cache: true
require-lockfile: true
- name: set up Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
node-version-file: '.nvmrc'
- name: build project
run: pnpm run build
| function mergeTrialReferences(target: Record<string, string>, source: Record<string, string>): void { | ||
| for (const [trialId, reference] of Object.entries(source)) { | ||
| if (trialId in target) { | ||
| throw new Error(`Cannot merge duplicate trial id: ${trialId}`) | ||
| } | ||
|
|
||
| target[trialId] = reference | ||
| } | ||
| } |


Follow up to #203.
This PR adds a durable execution plan that can be created once, shared across jobs, and replayed without rebuilding trial order. Plans store stable trial IDs plus the scenario, treatment, model, and benchmark capability references needed to resolve each trial from the current configuration.
The benchmark and experiment workflows now use the same four-shard flow: create
plan.json, run each shard from that plan intooutput-{shard}.json, then combine those manifests into the standard portableoutput.json. The merge keeps the existing per-trial file references instead of reading and rewriting every trial result, which keeps the shard handoff small and works with the split output layout from #212.This is the fifth entry in stack 214. #199 adds the benchmark scenarios last.
Changelog
New
--plan [path]and--from-plan [path]CLI modes.--shard <order/total>for deterministic plan sharding.--merge-resultswith--output-dirfor combiningoutput-*.jsonmanifests.Changed
output-{shard}.jsonmanifests that reference split trial result files.plan,shard, andmergemodes.Removed