Skip to content

fix(campaign): compare optimization methods on untouched test data#355

Merged
drewstone merged 6 commits into
mainfrom
fix/proposer-split-contamination
Jul 23, 2026
Merged

fix(campaign): compare optimization methods on untouched test data#355
drewstone merged 6 commits into
mainfrom
fix/proposer-split-contamination

Conversation

@drewstone

@drewstone drewstone commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What changed

  • replace the misleading compareProposers API with compareOptimizationMethods, which compares complete optimization procedures rather than candidate generators
  • require non-empty, pairwise-disjoint train, selection, and final test scenario sets before any method runs
  • keep final test data out of OptimizationMethodInput and wait for every method to finish before final scoring starts
  • give methods the same baseline, runner, judges, seed, directories, and execution defaults while preserving independent scenarios and spend limits
  • report paired scenario scores, simultaneous confidence intervals, pairwise comparisons, and complete versus unknown optimization and test cost
  • provide GEPA reflection, GEPA Pareto, SkillOpt, and FAPO method adapters
  • add custom token pricing for OpenAI-compatible model IDs that are not in the package pricing table
  • update AppWorld, GSM8K, examples, package exports, README, and comparison guidance to the new contract

Why

The old comparison allowed SkillOpt to accept edits on holdoutScenarios and then ranked methods on those same scenarios. That made the final ranking optimistically biased. The new API separates candidate generation, candidate selection, and final method comparison by construction.

Breaking changes

  • compareProposers becomes compareOptimizationMethods
  • proposers becomes methods
  • ProposerEntry becomes OptimizationMethod
  • built-in *Entry factories become *Method factories
  • callers pass trainScenarios, selectionScenarios, and testScenarios
  • runSkillOpt({ holdoutScenarios }) becomes runSkillOpt({ selectionScenarios })

Verification

  • pnpm test: 3,366 passed, 2 skipped
  • pnpm lint: 529 source files checked, 0 errors
  • pnpm typecheck
  • pnpm typecheck:examples
  • pnpm build
  • pnpm verify:package
  • AppWorld worker tests: 5 passed
  • README quickstart executed through @tangle-network/agent-eval/contract
  • docs scan: 7 files, 0 findings
  • local docs links: 7 files, all resolved
  • git merge-tree --write-tree origin/main HEAD

The checked-in June result remains labeled as historical and selection-contaminated. This PR does not claim a fresh method ranking from that artifact.

…ntamination

# Conflicts:
#	CHANGELOG.md
#	examples/benchmarks/gsm8k/compare-proposers.ts
#	src/campaign/presets/compare-proposers.ts
#	src/campaign/presets/run-skill-opt.ts
…ntamination

# Conflicts:
#	CHANGELOG.md
#	docs/concepts.md
#	docs/improvement-glossary.md
#	docs/self-improvement-map.md
#	examples/compare-proposers-canonical/README.md
…ntamination

# Conflicts:
#	CHANGELOG.md
#	src/llm-client.test.ts
#	src/llm-client.ts
@drewstone drewstone changed the title fix(campaign): isolate proposer selection from test fix(campaign): compare optimization methods on untouched test data Jul 23, 2026
@drewstone
drewstone marked this pull request as ready for review July 23, 2026 00:47

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 2b2df3c1

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-23T00:47:28Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 2b2df3c1

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-23T00:47:46Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 2 (2 low)
Heuristic 0.1s
Duplication 0.0s
Interrogation 228.9s (2 bridge agents)
Total 229.0s

💰 Value — sound

Replaces the methodologically flawed compareProposers (which let optimizers touch their own final-test data) with compareOptimizationMethods that structurally isolates train/selection/test partitions — the correct fix, built in the grain of the codebase.

  • What it does: Introduces compareOptimizationMethods: methods receive only train+selection scenarios in a frozen OptimizationMethodInput (test data is physically absent), all methods finish before any final-test scoring begins, and partitions must be non-empty and pairwise-disjoint (enforced up front). Adds simultaneous Bonferroni-adjusted bootstrap CIs across all method-vs-baseline and method-vs-method contrast
  • Goals it achieves: Eliminate optimistic ranking bias: the old compareProposers passed holdoutScenarios into ProposerEntry closures, and SkillOpt adaptively accepted edits on those same scenarios it was later ranked on — the final ranking was contaminated by construction. The new API makes the three-way train/selection/test split a structural invariant rather than a caller convention, so no optimizer can ever see the
  • Assessment: This is the standard ML train/validation/test methodology enforced at the type and runtime level — the right design. It reuses existing infrastructure (runCampaign, runImprovementLoop, runSkillOpt, pairedBootstrap, cost-ledger, campaignBreakdown) rather than reinventing; the builtin methods correctly delegate selectionScenarios → the loop's holdoutScenarios gate axis. The structural isolation (tes
  • Better / existing approach: none — this is the right approach. Searched for existing equivalents: runImprovementLoop is the single-method loop (complementary, not duplicative — the builtin methods compose it); the old compareProposers was the flawed predecessor being replaced. The train/val/test split with structural input isolation is the canonical correct design; no simpler alternative achieves the same guarantee.
  • 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 the contaminated compareProposers with a train/selection/test-partitioned compareOptimizationMethods that structurally prevents the selection-on-test bias it fixes, wired through builtins, examples, tests, and the public surface.

  • Integration: Fully reachable: exported from src/campaign/index.ts:196-208 and re-exported at root; four builtin method adapters (builtin-optimization-methods.ts:47-161) compose runImprovementLoop/runSkillOpt; three runnable examples (canonical, GSM8K, AppWorld) consume it; old compareProposers/*Entry factories fully removed and rejected with fail-closed errors; 722-line test file plus 289-line builtin test exe
  • Fit with existing patterns: Matches the established pattern exactly — uses runCampaign as the measurement primitive, the existing CostLedger, pairedBootstrap, surfaceContentHash for dedup, and runImprovementLoop/runSkillOpt as the procedure bodies. No competing implementation; this IS the canonical comparison primitive, replacing the flawed predecessor.
  • Real-world viability: Holds up: mapConcurrent bounds optimization concurrency; align() throws on missing scenario scores instead of fabricating zeros; structuredClone isolates scenario mutations between methods AND from caller-owned data (verified by tests at compare-optimization-methods.test.ts:429-517); cost completeness propagates through accountingComplete/incompleteReasons; Bonferroni-derived minimum resamples sca
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added examples/benchmarks/gsm8k/compare-optimization-methods.ts

  • console.log('GSM8K: gepa-reflection vs gepa-pareto vs skill-opt')

🟡 Cruft: magic number added examples/compare-optimization-methods/index.ts

+const round = (n: number) => Math.round(n * 1000) / 1000


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.

value-audit · 20260723T005200Z

@drewstone
drewstone merged commit 69cd851 into main Jul 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants