fix(optimization): run official benchmark reproducibly#429
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — ab7e69f5
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-24T17:43:23Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 129.8s (2 bridge agents) |
| Total | 129.8s |
💰 Value — sound-with-nits
Replaces a silently-broken scheduled workflow with one that installs pinned GEPA/SkillOpt sources and uses the env vars the benchmark actually reads, plus splits one cost knob into per-method and whole-run limits — a sound, in-grain fix with one minor test-robustness nit.
- What it does: Two things. (1) Deletes .github/workflows/empirical-gate.yml and adds .github/workflows/official-optimizer-benchmark.yml: the new workflow runs
uv sync --frozen --group skillopt-source --group gepa-sourceto install the pinned upstream optimizers, passes the env vars the current benchmark reads (SKILLOPT_EPOCHS, SKILLOPT_BATCH_SIZE, MAX_OPTIMIZER_MODEL_COST_USD, MAX_TOTAL_COST_USD), hard-fails t - Goals it achieves: Make the weekly official-optimizer comparison reproducible and not silently broken: the old workflow passed POPULATION/GENERATIONS/EPOCHS which the benchmark never reads, never installed the Python optimizer sources, and only warned (didn't error) when the result artifact was missing — so it could 'succeed' while producing nothing comparable. Secondary goals: equal, separately-auditable cost contr
- Assessment: Sound. The split of MAX_RUN_COST_USD into two knobs is semantically correct: costCeiling (src/campaign/presets/compare-optimization-methods.ts:447) guards the whole run (optimization + final test), while OptimizerModelBudget.maxCostUsd caps each method's own model spend — conflating them under one number was the real defect. The new workflow reuses existing machinery in-grain: the skillopt-source/
- Better / existing approach: No existing equivalent to reuse — this is the first test in the repo to read a .github/workflows/*.yml (git grep found none), and no other workflow does pinned-source install + cost-metadata gating. The only materially-available improvement is test robustness: the contract test uses raw string substring/regex assertions on the YAML (tests/official-optimizer-benchmark-workflow.test.ts:16-44) rather
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound-with-nits
A coherent fix that replaces a stale scheduled workflow with one wired to the controls the benchmark actually reads, correctly separates per-method optimizer spend from whole-run spend, and locks the workflow↔program contract with a test.
- Integration: Fully wired and reachable. The new .github/workflows/official-optimizer-benchmark.yml (added in this same commit ab7e69f, replacing the deleted empirical-gate.yml) is the live caller: it sets SKILLOPT_EPOCHS, SKILLOPT_BATCH_SIZE, MAX_OPTIMIZER_MODEL_COST_USD, MAX_TOTAL_COST_USD as env vars (workflow:108-111) and runs
pnpm tsx examples/compare-optimization-methods/index.ts(workflow:113), which r - Fit with existing patterns: Fits the grain. The example already used positiveNumberEnv/positiveIntegerEnv + optimizerModelBudgetFromEnv; this PR keeps that pattern and only renames one control into two. The two-budget split is strictly more correct than the prior single MAX_RUN_COST_USD, which previously served BOTH as the per-method optimizer budget default AND the whole-run ceiling (old index.ts used MAX_RUN_COST_USD for o
- Real-world viability: Holds up. Paid runs are gated behind explicit MODEL + PRICE_IN + PRICE_OUT (workflow:88-91), neutral-skip when no key secret exists (workflow:93-96), and the artifact upload fails closed on a missing result (workflow:121). The shared CostLedger enforces the ceiling atomically across concurrent methods (cost-ledger.ts:364-371). The stale-name guard test uses regex
^\s+NAME:so SKILLOPT_EPOCHS is - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Workflow contract test asserts on raw YAML strings instead of parsed structure [better-architecture] ``
tests/official-optimizer-benchmark-workflow.test.ts:24-37 uses
${name}:substring checks and^\s+${staleName}:regex on the whole-file text, so it can't distinguish whether a required env var (e.g. MAX_OPTIMIZER_MODEL_COST_USD) is wired into the correct step's env block versus appearing in a comment, a description, or the wrong step. The same guard against drift could be made structural by parsing the YAML (js-yaml is a common choice) and asserting that each required name appears in the 'Com
🎯 Usefulness Audit
🟡 Companion README still documents the removed MAX_RUN_COST_USD [ergonomics] ``
examples/compare-optimization-methods/README.md:107 documents
MAX_RUN_COST_USD(default 12), which this PR replaces with MAX_OPTIMIZER_MODEL_COST_USD (5) + MAX_TOTAL_COST_USD (20). A caller copying the README sets an env var the renamed example no longer reads. Update the table in the same PR so docs match the shipped controls.
🟡 Sibling benchmarks still conflate the two budgets this PR separates [problem-fit] ``
examples/benchmarks/gsm8k/compare-optimization-methods.ts:83,371,419 and examples/benchmarks/appworld/run-bench.ts:78,400,429 still use a single MAX_RUN_COST_USD as both per-method optimizer budget and whole-run costCeiling — the exact conflation this PR fixes for the main example. Not a blocker (each example is independent), but they're the natural next adopters of the two-budget split.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 7 non-blocking findings — ab7e69f5
Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-24T17:54:37Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — c714bf63
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-24T18:07:46Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 169.8s (2 bridge agents) |
| Total | 169.8s |
💰 Value — sound
Splits a conflated cost knob into distinct per-method and whole-run ceilings, and replaces a stale CI workflow with a reproducible, contract-tested benchmark harness — correct and in-grain.
- What it does: Splits the single
MAX_RUN_COST_USDenv var (previously fed to BOTH the per-method optimizer-model budget viaoptimizerModelBudgetFromEnvAND the whole-runcostCeiling) into two distinct controls:MAX_OPTIMIZER_MODEL_COST_USD(equal per-method optimizer-model spend) andMAX_TOTAL_COST_USD(whole-run stop). Replaces the oldempirical-gate.ymlworkflow — which passed dead inputs (`POPULAT - Goals it achieves: Three goals, all read from the change: (1) Fix a budget-accounting bug — the old code gave each method an optimizer-model spend cap equal to the total-run ceiling, so two methods could collectively spend 2× the 'total' on optimizer calls alone before any final scoring. (2) Make the scheduled benchmark actually reproducible — install the exact pinned upstream optimizers, require model+price metadat
- Assessment: The core insight is correct and well-grounded. The preset at
src/campaign/presets/compare-optimization-methods.ts:234documentscostCeilingas the shared limit across 'every method's optimizer and evaluation calls plus final scoring', while each method'soptimizer.budget.maxCostUsd(fromoptimizerModelBudgetFromEnv) is a separate per-method surface. Feeding both surfaces the same single va - Better / existing approach: none — this is the right approach. The budget split matches two genuinely distinct surfaces in the preset API. The workflow↔benchmark contract test is the correct lightweight guardrail against drift. The
yamldevDep is justified solely by that test. I checked whether the env-var triple (MAX_OPTIMIZER_MODEL_COST_USD/MAX_TOTAL_COST_USD/GEPA_MAX_PROPOSER_COST_USD) repeated across three sc - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
Replaces a genuinely broken scheduled benchmark (feeding env vars the benchmark ignores, silently warning on missing artifacts, conflating two cost limits) with a correct, reproducible, fail-loud equivalent locked by a contract test.
- Integration: Fully wired and reachable. The new .github/workflows/official-optimizer-benchmark.yml:7-9 runs on a weekly cron + workflow_dispatch, installs two real pinned uv dependency groups (clients/python/pyproject.toml:42-47 defines skillopt-source and gepa-source at pinned commits), invokes a real entry point (examples/compare-optimization-methods/index.ts:372-373 writes comparison.json to .evolve/...), a
- Fit with existing patterns: Fits the codebase's grain precisely. compareOptimizationMethods already separates a whole-run costCeiling (src/campaign/presets/compare-optimization-methods.ts:235) from per-method optimizer-model budgets (examples/_shared/optimizer-model-budget.ts via optimizerModelBudgetFromEnv). The old code fed the SAME MAX_RUN_COST_USD number into both (examples diffs), conflating two distinct limits; this PR
- Real-world viability: Holds up on non-happy paths. Verified the old workflow was actively feeding dead inputs: grep shows examples/compare-optimization-methods/ and examples/_shared/ never read POPULATION/GENERATIONS/EPOCHS, so the old POPULATION/GENERATIONS/EPOCHS inputs were no-ops. The new workflow fail-closes on missing model/price metadata before any paid call ([ -z "$MODEL" ] / [ -z "$PRICE_IN" ] / [ -z "$PRICE_O
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 10 non-blocking findings — c714bf63
Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-24T18:19:36Z · immutable trace
What changed
Checks
pnpm exec vitest run tests/official-optimizer-benchmark-workflow.test.tspnpm typecheck:examplespnpm typecheck(commit hook)pnpm exec biome check tests/official-optimizer-benchmark-workflow.test.ts examples/compare-optimization-methods/index.tsactionlint .github/workflows/official-optimizer-benchmark.ymlgit merge-tree --write-tree origin/main HEAD