fix(algorithm): one shared scratch-file list so GEPA snapshots are clean (#110) - #211
fix(algorithm): one shared scratch-file list so GEPA snapshots are clean (#110)#211OsherElhadad wants to merge 3 commits into
Conversation
…ean (#110) GEPA's candidate snapshots were dirty: both `run_dir.snapshot()` calls in gepa.py omitted `ignore=`, so every accepted candidate carried FOCUS.md, REFLECTION.md, LEDGER.md, JOURNAL.md and RUNMAP.md alongside the capability — 8 files / 32 KB where hill-climb stored 3 files, and every dashboard candidate-vs-parent diff showed scratch churn instead of the real edit. Root cause is not the missing `ignore=` argument, it is that the scratch-name list was copy-pasted into four modules and desynced. FOCUS.md/REFLECTION.md were already in cache._IGNORE_NAMES and gepa._NON_COMPONENT but never in harness._SNAPSHOT_IGNORE, so even after PR #199 made _SNAPSHOT_IGNORE derived from optimizer_context.INJECTED_* (which fixed LEDGER/JOURNAL/RUNMAP), GEPA snapshots still leaked GEPA's own scratch. Same class of bug as #109's `kind == "step"` filter and #189's counts. Fixed once: rundir.SCRATCH_NAMES is the single definition, at the bottom of the import graph, read by all four consumers — harness._SNAPSHOT_IGNORE, cache._IGNORE_NAMES, gepa._NON_COMPONENT and skillopt._SCAFFOLDING (which still had the literal inline, twice). A newly-injected scratch file now lands in all four automatically. INSTRUCTIONS.md/PROCESS.md stay deliberately OUT of SCRATCH_NAMES: they are snapshotted for explainability and filtered at diff time only. Verified end to end on examples/toy_calc with the mock optimizer: a GEPA candidate went from 8 files / 32 KB to 3 files / 12 KB and now matches hill-climb's snapshot contents exactly (INSTRUCTIONS.md, PROCESS.md, prompt.txt) — 5 stray files per accepted iteration eliminated. Tests: +3 (the shared-constant invariant, plus an end-to-end snapshot-cleanliness check parameterized over gepa and hill-climb so the two can't diverge again). Both fail on origin/main AND on top of #199; hill-climb passes throughout. 182 passed, 0 failed. compileall core skills clean. Zero new runtime deps. Closes #110
🔬 EvidenceEvery command and its literal output. Python: a venv at 1. Prove the defect on unmodified
|
skillberry-bot
added
algorithm
|
🏷️ Automatic Labeling I've analyzed this pull request and added the following labels:
These labels were selected based on the PR title, description, and changed files. If you believe any labels are incorrect or missing, feel free to adjust them manually. |
🔍 Review — PR #211Verdict: CHANGES REQUESTED — 1 blocking finding. The core fix is right and the scoping is honest (I reproduced the fail-on- The one blocking issue is a side effect of the unification that the PR's own evidence can't see: Blocking1.
Net delta to
Reproduction — a capability with a nested On Three compounding consequences:
Fix (small). Split the destructive consumer from the non-destructive ones. # rundir.py:43
SCRATCH_NAMES = (
"LEDGER.md", "JOURNAL.md", "RUNMAP.md",
"FOCUS.md", "REFLECTION.md",
)
LEGACY_SCRATCH_NAMES = ("REJECTED.md", "MEMORY.md", "STATE.md") # no live writer; filter-onlythen keep the legacy trio only in the three read-side consumers ( Even better and equally cheap, since Non-blocking2 findings. 2. 3. Nits4. 5. Excluded-set auditVerdict on the 5 files this PR removes from GEPA snapshots (plus
Nothing excluded is irreplaceable evidence. The
On the KEPT set: confirmed correct and confirmed non-nominal. Is the four-way unification sound?Sound for three of four; the fourth is where finding #1 lives. The four sets were already byte-identical in intent on
So: not over-unified across the board — over-unified in exactly one direction, Cache invalidationOld eval caches still behave correctly — no mass misses, no invalidation. Since The one stale-hit hazard is finding #1's, and it is introduced by the snapshot change, not the cache change: a capability file whose basename collides with a Merged-tree resultI merged all four myself (#211 base → #199 → #197 → #210). The author's report of the #199 conflicts is accurate — 4 files, all adjacent-import / adjacent-constant unions, no semantic overlap. #197 was 3 files, all pure 230 passed, 0 failed (the author's 225 was the 3-way; #210's #210 hardlinks do not land in a snapshot and do not trip the tamper guard.
SecurityClean. Nothing new is committed — the diff only removes files from snapshots. The two kept files carry no secrets: Merge order
Note for the rebase: post-#199, Verification I re-ranFail-before on Fail-before on top of #199 — the honest-scoping claim — reproduces verbatim: The tests genuinely fail on top of #199, not just on Real zero-API run, 8 → 3 confirmed, and gepa/hill-climb list identical contents. Materialize-from-snapshot / replay check — the snapshot is a faithful record of what was scored, for a capability with no name collisions: Cache invalidation: Import-graph / placement claim — verified, and no import-time set mutation. Live writers for the names added to the destructive list (basis for finding #1): |
…hot + cache hash Review fix for #211. The four-way unification made harness._SNAPSHOT_IGNORE — the one DESTRUCTIVE consumer — take the full union. Three of the added names (MEMORY.md, STATE.md, REJECTED.md) have no live writer in core/, so their only real-world referent is a capability file that shares the name, and snapshot() silently deleted it from the candidate, from every descendant iteration, and WITHOUT busting the eval-cache key (cache ignored the same name) — a stale hit on a mutilated candidate. Regression vs main. - rundir: SCRATCH_NAMES (live writers) vs LEGACY_SCRATCH_NAMES (no writer, filter-only) + NON_CAPABILITY_NAMES union. Destructive consumer takes the subset; every read-side filter takes the union. - rundir.snapshot: root-anchored ignore callable. shutil.ignore_patterns matches by basename at EVERY depth; every entry is a root-level framework injection, so a nested src/prompts/STATE.md can no longer be caught. - cache.hash_candidate_dir: root-anchored too, so deleting a nested colliding capability file DOES change the key (closes the stale-hit hazard at the root). - dashboard._DIFF_SKIP + harness._CAP_DIFF_SKIP now derive from NON_CAPABILITY_NAMES — they are read-side filters asking the same question, and were the last hardcoded copies (review finding #2). - test_gepa: pin the live/legacy split and that all five read-side filters equal the union; new test_snapshot_ignore_excludes_legacy_names_and_is_root_anchored; drop the IndexError-prone parts[] check subsumed by the exact-set assert.
…mory->prompt framing Review fixes for #212: 1. cache.py — restore main's intro paragraph verbatim (the sentence #211 rewrites) and confine the correction to a separate "Scope: GEPA only" paragraph, so the #211 conflict is a single textual hunk whose wrong resolution can no longer restore the false maybe_cached_score line — that line's removal now auto-merges outside the conflict region. 2. test_w1_engine.py — new guard pinning that no doc under core/ or skills/ cites maybe_cached_score, so a revert of the docstring fix fails a test. 3. skillopt SKILL.md / concepts.md / skillopt.py — the rejected/history jsonl are dashboard audit records, write-only, never prompt input. 4. MemoryPanel.tsx — drop the "do-not-re-propose" framing this PR disproved.
A filter may legitimately add its own read-context names (post-#199 the cache and component lists fold in optimizer_context.INJECTED_NAMES); the invariant that matters is that none of them DROPS a shared name.
🔧 Review fixesAll 5 findings addressed. The blocking one was real and I missed it: I checked that the predicate was one concept and never checked that the operation was. Fixed at the root, plus the deeper root-anchoring you suggested. Merge order restated: #199 → #197 → #211 (this fix) → #210. #210 merges clean and its hardlinks go to 1. BLOCKING — destructive snapshot exclusion deleting a capability file — FIXEDConfirmed exactly as described. The split ( SCRATCH_NAMES = ( # live writers → safe for the DESTRUCTIVE consumer
"LEDGER.md", "JOURNAL.md", "RUNMAP.md",
"FOCUS.md", "REFLECTION.md",
)
LEGACY_SCRATCH_NAMES = ("REJECTED.md", "MEMORY.md", "STATE.md") # no live writer; filter-only
NON_CAPABILITY_NAMES = frozenset(
{"INSTRUCTIONS.md", "PROCESS.md"} | set(SCRATCH_NAMES) | set(LEGACY_SCRATCH_NAMES))
Root-anchoring: yes, done here, not flagged. You framed it as "even better and equally cheap" and I agree — but I'd go further: the split alone only fixes the three names we know about today. Basename-matching-at-any-depth stays a live footgun for every name added later, and the failure mode is silent data loss. The lazy fix is the root-cause fix: one guard in the shared function is smaller than remembering this rule forever at every call site. names = set(ignore or ())
ig = (lambda d, cs: names if Path(d) == src_dir else set()) if names else None
shutil.copytree(src_dir, dst, ignore=ig)I also root-anchored rel = p.relative_to(cdir)
if len(rel.parts) == 1 and p.name in _IGNORE_NAMES:
continueEvery ignored name is a root-level framework injection, so anchoring is strictly more correct — and now snapshot and cache agree on where a name counts, which is what made the stale hit possible. The two remaining basename-at-any-depth filters ( Data-loss probe — before / afterYour probe, reproduced: seed capability with nested BEFORE ( (The probe can't even reach the cache check on the old code — the file it wanted to delete was already deleted for it.) AFTER ( Cache-key evidence — your
|
… edit (#129 review) Review of #222 found the constraint block was actively misdirecting on every REAL agent optimizer: it told the optimizer "you already tried this, re-proposed 2x" about approaches it had never proposed. Four blocking fixes, all reproduced fail-before/pass-after. B1 — the signature was dominated by framework-injected read-context. A capability diff compares a SNAPSHOT parent (INJECTED_* already stripped by _SNAPSHOT_IGNORE) against the LIVE workdir (not stripped), so every injected CLAUDE.md / .claude/skills/<x>/SKILL.md read as a capability ADDITION, sorted to the front, and truncated the real edit away entirely. _CAP_DIFF_SKIP omitted INJECTED_NAMES and _capability_files filtered a hardcoded 3-of-9 subset of INJECTED_DIRS. Both sets are now derived, never enumerated — in harness, dashboard and skillopt alike. Only `mock` (no registry skills_dir) hid this, which is why 13/13 tests passed over a broken path; the new test runs run_step with optimizer_name="claude-code". B2 — head-first truncation made the signature not a function of the edit: two different edits sharing a long prefix (any realistic SKILL.md or system prompt) collapsed to one signature. Overflow now closes with a sha256 digest of the whole normalized body, so it stays stable under cosmetic variation AND distinct whenever the bytes differ. B3 — eviction was by FIRST appearance, so the single most predictive row (a dead end the optimizer JUST re-proposed) was dropped while newer one-offs were kept, contradicting the block's own "12 most recent" wording. A repeat now requeues its row. B4 — skillopt._changed_components was a fifth, pre-drift copy of the scratch list. It now derives from the one shared definition like the other four. SCRATCH_NAMES end state (rebased onto #211): rundir.SCRATCH_NAMES / LEGACY_SCRATCH_NAMES / NON_CAPABILITY_NAMES is the sole definition, split by OPERATION so the one destructive consumer never takes a retired name; optimizer_context.SCRATCH_NAMES is deleted. The five read-side filters compose NON_CAPABILITY_NAMES with INJECTED_NAMES/DIRS. Also: control + bidi-override chars stripped from the signature once (N3); the row shows "(latest <cid>)" so a repeat count cannot be misread as the first candidate's (N2); a frontend test for the new `approaches` field (N4); N5 noted with its upgrade path; the function-body cap_instructions import, the <8 KB bound comment and the 70/30 tail-slice docstring corrected (nits).
Closes #110
GEPA's candidate snapshots were dirty. Both
run_dir.snapshot()calls ingepa.pyomittedignore=, so every accepted candidate carried the optimizer's scratch alongside the capability — 8 files / 32 KB where hill-climb stored 3 files — and every dashboard candidate-vs-parent diff showed scratch churn instead of the real edit.Before / after (real runs,
examples/toy_calc+mockoptimizer)candidates/after a 3-iteration run:origin/mainFOCUS.mdINSTRUCTIONS.mdJOURNAL.mdLEDGER.mdPROCESS.mdREFLECTION.mdRUNMAP.mdprompt.txtFOCUS.mdINSTRUCTIONS.mdPROCESS.mdREFLECTION.mdprompt.txtINSTRUCTIONS.mdPROCESS.mdprompt.txtINSTRUCTIONS.mdPROCESS.mdprompt.txtGEPA now agrees with hill-climb exactly on what is excluded. 5 stray files per accepted iteration eliminated; 32 KB → 12 KB (−62%).
What #199 already fixed vs what this PR adds
#199 (issue #109) did most of the mechanical fix and I built on it rather than duplicating it: it added
ignore=_SNAPSHOT_IGNOREto bothgepa.pysnapshot calls, made_SNAPSHOT_IGNOREderived fromoptimizer_context.INJECTED_DIRS/INJECTED_NAMES, and folded the same lists intocache.pyandgepa._NON_COMPONENT. That eliminatedLEDGER.md/JOURNAL.md/RUNMAP.md/prior_iterations/.What genuinely remained:
FOCUS.mdandREFLECTION.md— GEPA's own per-iteration scratch — still landed in every snapshot. They were listed incache._IGNORE_NAMESandgepa._NON_COMPONENTbut never in_SNAPSHOT_IGNORE. Verified empirically: the new tests fail on top oforigin/fix/issue-109-optimizer-context, not just onmain(output pasted below).Root cause and the shared-constant decision
The missing
ignore=argument was the symptom. The cause is that the scratch-name literal was copy-pasted into four modules and desynced — exactly the failure mode that produced #109'skind == "step"filter and #189's counts. #199 correctly unified the injected read-context half (INJECTED_*), but the scratch-file half stayed as four independent literals, andskillopt._changed_componentsstill had it inline twice.So I made it one definition:
rundir.SCRATCH_NAMES— a single tuple, placed inrundir.pybecause that module is at the bottom of the import graph (stdlib +.splitsonly), so every consumer can import it eagerly with no cycle and no import-order dependence.harness._SNAPSHOT_IGNORE,cache._IGNORE_NAMES,gepa._NON_COMPONENT,skillopt._SCAFFOLDING(new named constant replacing two inline literals).A newly-injected scratch file added to
SCRATCH_NAMESnow lands in all four automatically. I deliberately did not put it inoptimizer_contextnext toINJECTED_*: those two lists mean different things (INJECTED_*is read-context the harness copies in;SCRATCH_NAMESis state the harness/algorithm writes) andoptimizer_contextsits higher in the import graph thancache/rundirneed.INSTRUCTIONS.mdandPROCESS.mdare deliberately excluded fromSCRATCH_NAMES:PROCESS.mdis the candidate's per-iteration explainability record and is meant to be snapshotted. Both are filtered at diff time only (dashboard._DIFF_SKIP/harness._CAP_DIFF_SKIP). A test pins that.Expected merge order
#199→#197→ this PR (any order works, but this is the order verified). My diff is written to apply after both. All merge conflicts are trivial adjacent-import unions; I resolved them locally and ran the combined suite: 225 passed, 0 failed with all three branches merged. #197's tamper guard is untouched (no overlapping lines).Verification
Full suite on this branch:
179 baseline + 3 new, 0 failed.
New tests fail on
origin/main(the defect):…and still fail on top of #199 (what this PR adds):
The
hill-climbparametrization passes in all three states — it was already correct, and now the two algorithms are pinned to agree.Zero new runtime deps. Tests in
core/tests/per CONTRIBUTING.md.Full commands + raw output in the
## 🔬 Evidencecomment below.Files touched
core/cap_evolve/rundir.py— new sharedSCRATCH_NAMEScore/cap_evolve/harness.py—_SNAPSHOT_IGNOREderives from itcore/cap_evolve/gepa.py—ignore=_SNAPSHOT_IGNOREon both snapshot calls;_NON_COMPONENTderivescore/cap_evolve/cache.py—_IGNORE_NAMESderivescore/cap_evolve/skillopt.py— two inline literals →_SCAFFOLDINGcore/tests/test_gepa.py— +3 regression tests