GEPA eval-cache hits now carry output/trace (no more hollow reflective dataset) - #210
GEPA eval-cache hits now carry output/trace (no more hollow reflective dataset)#210OsherElhadad wants to merge 2 commits into
Conversation
…e dataset)
The eval cache stored only {reward, feedback}, so `_eval_minibatch` rebuilt a
cache-hit `Score` with `raw={"cached": True}` and no output/trace. GEPA re-samples
parents constantly, so the parent minibatch that FEEDS reflection frequently hit
the cache — and `_write_reflection` then emitted `- Agent output:` (empty) for those
failing tasks. GEPA's headline advantage (rich reflection over failures) was being
degraded by GEPA's own cache: a blind hill-climb on cached parents.
Fix (issue #111 Option A, pointer flavour): each cache entry gains `rollout_file`,
the name of the rollout json under `rollouts/<split>/` that produced the score. On a
hit, `_replay_cached` re-reads that json for the real output/trace and hardlinks it
under the current eval tag, so `rollouts/<split>/*__<tag>__t0.json` is complete
either way and the tag-pinned `trajectories/` dir exists for a cached minibatch too.
A pointer flavour, not a payload flavour, because the traces are ALREADY persisted
in the run dir — duplicating them into eval_cache.json would grow the cache with the
trace size while the reflection stayed truncated. A pointer costs ~42 bytes/entry,
keeps reflection untruncated, and the hardlink makes a hit free on disk.
A hit whose pointer does not resolve (a pre-#111 cache, or pruned rollouts) is
treated as a MISS and re-run: paying one rollout beats serving an empty
"Agent output:" as if it were a trace. Replayed output/trace go through the same
`_short` truncation a fresh eval uses, so prompt size stays bounded.
|
❌ Automatic Labeling Failed An error occurred while trying to automatically label this pull request. Please check the workflow logs for details and add labels manually. |
🔬 EvidenceEvery command + its literal output. Python 1. Fail-before (stash only the source change, keep the test)2. Pass-after3. Real end-to-end, zero API cost —
|
🔍 Review — PR #210Verdict: CHANGES REQUESTED The diagnosis and the content of the fix are right: I independently confirmed the reflective dataset is now genuinely per-task correct (not merely non-empty), truncation is symmetric, and the cached REFLECTION.md is byte-identical to an uncached one. Every claim in the evidence comment reproduced. But the re-materialization mechanism has two real defects: an unvalidated Blocking1. Worse, The #142 guard catches the write (good — the run aborts loudly), but the read/exfiltration is silent and the link is created regardless. Cost accounting also reads Fix — confine the pointer, one guard at the top of rfile = str(cached.get("rollout_file") or "")
# rollout_file is a BARE FILENAME under out_dir. The cache is plain on-disk JSON, so a
# tampered/absolute/../ entry must not read outside rollouts/ (#142's threat model).
if not rfile or Path(rfile).name != rfile:
return None
2. Consequence: the audit trail this PR exists to make trustworthy can be retroactively rewritten with another candidate's output — no event, no warning. Fix — make the fresh-eval write break the link instead of truncating it. rfile = f"{task.id}__{tag}__t0.json"
(out_dir / rfile).unlink(missing_ok=True) # never truncate a hardlinked inode: a cache
# hit may have linked a PRIOR iteration here
(out_dir / rfile).write_text(...)Add a test that pins it: link a rollout under a second tag, write a fresh rollout to that second tag, assert the first tag's bytes are unchanged. Non-blocking3. Fix: overwrite when the existing record's 4. 5. Untested critical paths. The 5 new tests cover the happy path, the pointer format, dangling/legacy MISS, and truncation — genuinely content-asserting, not non-emptiness theatre ( Nits6. 7. Hardlink audit
Is the reflection CORRECT?Yes — per-task correspondence holds, and I tested it adversarially rather than taking the non-emptiness evidence at face value. Cache populated in order The re-materialized files carry the matching record, so filename ↔ Same in the real merged-tree gepa run's fully-cached iteration ( Truncation is symmetric (5 KB output + a nested-dict trace, fresh vs cached): Both go through the same Determinism: byte-identical. Cached vs uncached The only divergence is the extra Merged-tree resultMy own merge, Real zero-API gepa run on The "trajectories/ OMITTED" branch fires 0 times, and the VERBATIM claim is truthful — the dir holds exactly this minibatch's four tasks with matching inputs (see above). #199's honest-absence path remains correct for a pre-#111 / pruned cache. The author's "self-correcting, no #199 edit needed" reasoning holds. Recommended merge order: Verification I re-ranBoth blocking findings are contained in |
…stop archived rollouts being rewritten Review fixes for #210. The reflective-dataset content was correct; the re-materialization *mechanism* had two exploitable defects. BLOCKING 1 — `rollout_file` was unvalidated. `eval_cache.json` is plain JSON in the optimizer-writable run dir, so every field it carries is untrusted input — exactly the threat #142 models (whose guard hashes `.capevolve/project` and excludes the run dir, so it does not cover this file). A crafted `../../../../secrets.json` or absolute pointer read any json-shaped file on disk straight into REFLECTION.md — i.e. into the optimizer LLM's prompt — and hardlinked the target's inode into the run dir. `_replay_cached` now requires a bare filename (`Path(rfile).name != rfile` rejects absolute, `../` and `sub/` in one comparison — an allowlist of the only legitimate shape, not a denylist of bad ones) and confines the resolved path to the rollouts dir as a second layer. A malformed `reward` now degrades to an honest MISS instead of raising `ValueError` mid-loop. BLOCKING 2 — a cache hit hardlinks a rollout under the new tag, and the fresh-eval writer used `write_text`, which truncates the *shared inode*. On a torn iteration + `--resume` (tags repeat, since `_save()` only runs at step end) a fresh write silently rewrote a PREVIOUS iteration's archived rollout with another candidate's output — no event, no warning, falsifying the evidence trail a sealed number rests on. Both writers into `rollouts/` now use `rundir._atomic_write` (tmp + `os.replace`), which replaces the directory entry instead of truncating, so an archived record is immutable once written. Hardlink sharing is retained, so a cache hit is still free on disk. Also: drop the stale `not dst.exists()` short-circuit, which let `trajectories/` serve a PRIOR parent's rollout while REFLECTION.md showed the current one, under #199's unconditional "SAME minibatch VERBATIM" claim; log `rollout_rematerialize_failed` instead of a silent `pass` when both link and copy fail; and correct the EvalCache docstring, which promised a 3-key entry shape while `rollout_file` is optional. 9 regression tests: path traversal (3 shapes), malformed reward, hardlink write-through, tag reuse, EXDEV copy fallback, loud re-materialization failure, and the `_copy_step_trajectories` no-write-through invariant.
🔧 Review fixesAll 7 findings addressed. Both blocking exploits re-run against the fixed code and closed. Nothing in the validated reflective-dataset work was churned — re-proved below. Exploits closed
Exploit 1 — before → after (same probe, same canary): Exploit 2 + finding 3 — before → after: Threat-model note
nlink evidenceSharing is retained (a cache hit is still free on disk) — what changed is that a fresh write replaces the directory entry instead of truncating the shared inode. Same 3-tag replay, before and after: Real zero-API gepa e2e ( Response to all 7 findings
VerificationBoth exploits re-run and closed (literal output above). Re-proved everything already validated, so none of it regressed: Suite + compileall: Merged tree ( Both exploits also verified closed on the merged tree ( Conflicts: still 3, still all #199↔#197 import lines, keep-both ( Merge order
Files touched by these fixes
|
The review made `verify` pass on a broken benchmark in 9 of 12 attacks. Its mechanical claims were all true — it really does run `live()` -> `run_target()` -> `score()` twice for every val task — but it drew no conclusion from the results. This fixes the seven blocking findings plus the third-author protection gap. verify now concludes: - HEADROOM is a hard failure, not a note. A seed capability already scoring 1.0 has nothing to optimize, which is the one thing `baseline` exists to confirm and the signature of the two commonest reward hacks (a score() wired to a constant, a run() returning task.target or reading the answer key off disk). A genuinely saturated reference fixture opts out loudly with allow_saturated_baseline: true. - A DEGENERATE-SCORER PROBE scores a synthetically correct rollout against a deliberately wrong one and requires the rewards to differ. Correct-vs-wrong is a property of the scorer alone, so it catches a score() that ignores its input at any baseline — which the headroom rule cannot see. - SPLITS must be genuinely disjoint with a non-empty train, asserted on the REALIZED split. train == val == test passed the old floor; #99 found the repo's own headline tau^2 number came from exactly that. Built as a real Splits rather than a throwaway type("S", (), ...). - CONTAINMENT is an allowlist, the shape PR #210 used at gepa.py: every path key must be a plain relative path whose resolved parent is inside the project dir, checked once in load_manifest so no use site can bypass it. `target_module: ../../pwned.py` previously EXECUTED code outside the project dir during verify, from a location #142's guard structurally cannot hash. Denylists have failed six times in this batch; this is not a seventh. - PROTECTED PATHS are asserted on what protect.resolve_protected() actually resolves from the GENERATED capevolve.yaml — the artifact the runtime guard reads — not on what benchmark.yaml claims. Weakening only the spec left verify reporting OK with rep.protected == ['adapters/adapter.py'], the same wrong-artifact bug as #189. - protected_paths is now ADDITIVE: unioned with the layout defaults and #197's globs, never substituted. #197's own list replaces its defaults wholesale, so declaring four paths silently switched off the *gold* answer-key globs. Union is the only default that fails safe. Plus an UNDER-DECLARATION SWEEP flagging any .py or answer-key-ish file under project/ (outside capability_path/) the guard would not hash: a third author's helpers.py / scorer2.py were silently unprotected and tampering all three went undetected. - The STAMP is evidence, not a differently-located claim. verified.json now records the grader and manifest hashes alongside the dataset, and `benchmark list` RE-CHECKS every one: a hand-written stamp (no steps, no hashes) and a stale one both read verified: false with the reason in stale_reason. The dataset_sha256 was written and never compared. - A score() without `scoring: custom` is a hard error. It silently overrode the declared mode, so both the manifest and `benchmark list` reported a grading mode that was not in effect — the unknown-key hard error, one level deeper. - --description is emitted as a quoted YAML scalar. A newline redefined manifest keys. Also: content-duplicate task rows are refused (fresh ids on identical rows split cleanly, so val became a copy of train); tasks(split) honours its argument instead of handing the sealed test split to any caller; an empty or uncompilable regex target is a dataset error rather than a free 1.0 / a mid-eval crash; numeric uses math.isclose(rel_tol=...) and accepts scientific notation; and --refresh KEEPS a hand-edited adapters/adapter.py instead of clobbering it, so overriding one generated hook is a supported edit rather than work the next manifest change deletes. All 12 attacks now fail. 220 passed (was 203, + 17 new tests), 0 failed apart from the known #200 dashboard port flake; compileall clean.
Closes #111
Problem
EvalCachestored only{reward, feedback}, so on a cache hit_eval_minibatchbuilt aScorewithraw={"cached": True}and nooutput/trace. GEPA re-samples parents constantly, so the parent minibatch that feeds reflection frequently hit the cache — and_write_reflectionthen emitted- Agent output:(empty) for exactly those failing tasks.Reflection quality is the whole point of GEPA (arXiv:2507.19457). On cached parents it degraded to a blind hill-climb, and the expensive optimizer call was spent on a blank dataset.
Fix
Issue #111 Option A, pointer flavour:
EvalCache.put(..., rollout_file=...)— each entry gainsrollout_file, the name of the rollout json underrollouts/<split>/that produced that score.gepa._replay_cached(...)— on a hit, re-read that json for the realoutput/traceand rebuild the fullScore; hardlink it under the current eval tag sorollouts/<split>/*__<tag>__t0.jsonis complete either way and the tag-pinnedtrajectories/dir exists for a cached minibatch too.Agent output:as if it were a trace — so a score-only entry can never hollow out a reflective dataset again.Why a pointer, not the payload. The traces are already persisted in the run dir. Copying them into
eval_cache.jsonwould grow the cache by the trace size and leave reflection truncated (whatever bound you pick). A pointer costs ~42 bytes/entry, keeps reflection untruncated, and the hardlink makes a cache hit free on disk. Replayedoutput/tracestill go through the same_short(n=1500)truncation a fresh eval uses, so prompt size stays bounded regardless of trace size.Cache format + size impact
Measured on the pasted end-to-end run (3 GEPA iterations, toy_calc, 8 cache entries, 24 rollout files):
Before
After
eval_cache.jsonrollouts/The per-entry cost is a fixed filename, independent of trace size — a 50 KB trace still costs 42 bytes of cache.
test_trace_is_bounded_in_the_reflective_signalpins both halves (replayed trace truncated, cache stays <1 KB with a 50 KB trace).Before / after: the reflective dataset on a cache-hit iteration
Same run, iteration 2, whose parent minibatch was 4/4 served from the cache (
{"kind":"minibatch","tag":"mb_p_0001","fired":0,"cached":4}) and 0/4 passing — the exact scenario in the issue.BEFORE —
work/gepa_0002/REFLECTION.mdEvery "Generated Outputs" blank, no
Trajectory:line at all. Note the scorer feedback quotes the output the optimizer was not shown.AFTER — same file, same iteration
Real per-task output and trajectory, from a 100% cache hit, at zero extra rollout cost.
Coordination with #199 / #197
#199 (issue #109) and this PR compose exactly as its author described: #199 made the absence honest; this makes the traces available, so the honest-absence path becomes a genuine edge case (stale/pruned cache) instead of the normal cached path.
I did not edit #199's text — that would conflict for no benefit. Instead this PR makes it self-correcting, because both #199 branches are already conditional on the dir existing:
gepa._gepa_block(..., has_trajectories=...)— the"This minibatch was served entirely from the eval cache, so NO rollout files were persisted for it"branch is now not taken on a cached minibatch (the rollouts ARE there), so the prompt makes the truthful"./trajectories/ holds the SAME minibatch rollouts VERBATIM and untruncated"claim instead.harness._copy_step_trajectories— theoptimizer_context_warningevent with"no rollouts persisted for the pinned eval tag (fully-cached minibatch); trajectories/ OMITTED"now fires 0 times on the merged tree (was once per cached iteration).Verified on a tree with all three PRs merged:
Two stale comments for #199's author (theirs to change, one word each, no behavior):
harness._copy_step_trajectoriessays "the eval cache stores only reward+feedback — see #111" and "A fully-cached minibatch writes no rollout files". Both are false after this PR; the surrounding fallback logic remains correct and still-needed for a pre-#111 / pruned cache.#197 (issue #142): untouched. Both
protect.verifyguards in_eval_minibatch(pre and post) survive verbatim — I only changed the cache-hit branch inside the loop and thecache.putcall.Expected merge order: #199 → #197 → this PR (any order works; conflicts are import-line only and between #199 and #197, not with this PR — see Verification).
Verification
179 baseline + 5 new (
core/tests/test_gepa_cache_traces.py), 0 failed.Fail-before/pass-after, by stashing only the source change:
Merged with both sibling branches (#199 then #197), conflicts resolved (import lines only, all three between #199 and #197 — none in my hunks):
Full commands + untruncated output in the
## 🔬 Evidencecomment below.