feat(eval): make the recall gate measure code, not corpus drift - #216
Merged
Conversation
A drop in precision@K used to be unattributable: the baseline snapshot pinned old code AND an old corpus, so drift and regression were indistinguishable and the only offered remedy was --update-baseline, which discards the signal. The gate now compares the recorded corpus fingerprint against the live one and words the failure accordingly. Confounded failures still block; they just point at the two-run comparison instead of at reseeding.
check_gate's cyclomatic complexity (13) went over its quality-gate budget (12) after the code/confounded attribution branch landed. Adjudicated: extract, don't bump the baseline. _gate_failure_message() carries the two wordings verbatim — same text, same --against remedy — check_gate now just calls it and stays under budget.
Extract _run_gate() so the corpus_fingerprint wiring into check_gate is testable without a full evaluation. The pre-push hook's failure message was updated separately (machine-local, untracked) to point at the attribution instead of a generic regression message.
The saved-baseline gate cannot approve a ranking change: its baseline pins old code AND an old corpus. --against evaluates the same live index twice, once with the working tree's code and once with the code at a git ref, so the corpus term cancels. Both runs pass --no-cache because the result cache lives in state_dir and is shared across worktrees. The ref side runs through PYTHONPATH + python -m memo because the installed console script would run the global uv tool in both halves.
C1: --against forced `force = True`, which only suppresses the cache READ. The write at `if not no_cache:` still ran, poisoning the shared eval cache with the working tree's numbers; a pre-seeded cache entry reproduced a false PASS (0.10 vs ref 0.99 reported as 0.990 vs 0.990). Now `--against` sets `no_cache = True` instead, extracted into `_resolve_cache_flags` to keep eval_recall_cmd's complexity under budget. C2: a relative --labels path resolved inside the ref worktree's cwd, not the caller's, silently scoring the two sides against different label sets. Fixed both halves: `--labels` now uses `resolve_path=True`, and `compare_rows` takes current/ref `labels_fingerprint` as an identity guard (mirroring check_gate's), refusing to compare when they differ. `run_against` now returns an `AgainstRun(rows, labels_fingerprint)` so the ref side's fingerprint reaches the comparison. I1: `_worktree_dest` used `repo_root / ".git" / ...`, which breaks with `Not a directory` from any linked worktree (.git is a file there) — this plan itself runs from one. Switched to `tempfile.mkdtemp()`, and `repo_root` is now resolved via `git rev-parse --show-toplevel` instead of counting `__file__` parents, which returns nonsense under the installed uv tool. I2: --quick/--max-prompts silently scoped only the current side, making the two runs' prompt counts (and therefore the comparison) incomparable. Now rejected via `_validate_against_flags`, extracted for the same complexity- budget reason as C1's fix. I3: the PYTHONPATH-prepend test asserted `.startswith(wt_src)`, which can't distinguish prepend from append when the ambient PYTHONPATH is empty (true under `uv run pytest`) — an append-bugged implementation would pass it verbatim. Now pins a non-empty ambient PYTHONPATH and asserts full equality. m1: compare_rows checked only precision/noise. Added the same avoid_at_k (coverage, must not drop) / avoid_leak_at_k (leakage, must not rise) floors check_gate already enforces, so this module doesn't reintroduce the "vacuously true forever" bug --update-baseline was once fixed for. m3: git and subprocess failures were `check=True, capture_output=True` with stderr discarded, surfacing only "exit status N". Added _run_git/_default_runner wrappers that raise with the captured stderr attached. Verified I1 and m3 against real git (not just mocks): reproduced "Not a directory" from a linked worktree's .git file, confirmed the tempfile-based dest fix works there, and confirmed a bad --against ref now raises with "fatal: invalid reference" instead of a bare exit-status error.
HIGH-1: the round-1 regression test for C1 (cache poisoning) used ts=0.0 in its fake stale-cache entry, which the TTL check rejects unconditionally — evaluate_calls could never be empty regardless of whether the fix worked. Fixed to a fresh ts (matching the existing "fresh"/"expired" idiom already in this file) and renamed the class to say what it actually is. Verified by mutation: a full revert of C1 now makes the read assertion itself fail (evaluate_calls == []), not just the write assertion. MEDIUM-1: added CliRunner tests for --against combined with --quick, --max-prompts, and both — the one round-1 fix that shipped without a RED step. MEDIUM-2: json.loads, git, and subprocess failures inside eval_against.py used to escape as raw tracebacks, making exit=1 from a broken pipeline indistinguishable from a genuine ranking regression. Added AgainstError (a module-local exception, keeping eval_against.py a click-free leaf) and translate it to click.ClickException in a new cli_eval._run_against helper, which also relocates the try/except out of eval_recall_cmd for complexity budget reasons. json parse failures now include a raw[:200] excerpt. LOW-1: _add_worktree ran outside run_against's try/finally, so a failure there (bad ref, already-occupied dest) skipped _remove_worktree and leaked the scratch dir tempfile.mkdtemp() had already created. Moved it inside the try, and _remove_worktree now falls back to shutil.rmtree when git itself never registered the path as a worktree. Verified against real git: an already-occupied dest now gets removed even though `git worktree remove` itself fails on it. LOW-2: --against combined with --gate or --update-baseline used to exit silently before either's effect ran. Extended _validate_against_flags to reject both, with tests. LOW-3: _worktree_dest(repo_root) hadn't used its parameter since round 1's tempfile.mkdtemp() switch. Dropped it. LOW-4: the label-fingerprint guard failed OPEN on a missing ref fingerprint (only checked for a MISMATCH). Now fails closed — once the current side has a fingerprint to give, an absent ref fingerprint refuses the comparison too, worded distinctly from a mismatch. Deferred-1: src/memo/__main__.py doesn't exist at origin/master until this branch merges, so any ref older than it would fail deep inside the -m memo subprocess with an opaque error. run_against now pre-flights with `git cat-file -e <ref>:src/memo/__main__.py` and names the first commit that adds it. Verified against a real two-commit repo (one before, one after adding the file). Deferred-2: AgainstResult carried no avoid_at_k/avoid_leak_at_k fields, so --against --json couldn't explain an m1 floor failure. Widened it with current_avoid/ref_avoid/current_leak/ref_leak. Deferred-3: compare_rows used `.get(key) or 0.0` for a missing ref avoid_leak_at_k, making it maximally STRICT; check_gate's own convention defaults a missing baseline leak to 1.0 (permissive). Aligned compare_rows with check_gate's `.get(key, default)` convention and pinned it with a test. compare_rows and _validate_against_flags/_resolve_cache_flags grew past ruff's C901 threshold with these additions; extracted _label_fingerprint_guard/_diff_parts (eval_against.py) and _run_against (cli_eval.py) to stay under budget without raising any existing entry in eval/quality_baseline.json.
…s the entrypoint (round 3) MEDIUM (regression introduced by round 2): `git cat-file -e <ref>:path` exits nonzero identically for "ref exists but lacks that path" and "ref does not exist", so `_ref_has_main_entrypoint` misattributed a typo'd ref to "predates the entrypoint" and told the user to check out a commit unrelated to their actual mistake. At 329b4ca the same input produced an accurate `fatal: invalid reference`. Added `_ref_exists()` (`git rev-parse --verify -q <ref>^{commit}`), checked first in run_against, raising a distinct "unknown ref" error before the entrypoint check ever runs. Verified against a real two-commit repo that the two error paths now produce the reviewer's expected messages exactly. Also closed the four items the reviewer named as deferred: 1. `_remove_worktree` now takes `added: bool`. When `_add_worktree` itself failed, `dest` was never registered as a worktree, so `git worktree remove` on it always failed too — printing a "not a working tree" warning that's pure noise since the caller already knows why. Skip straight to `shutil.rmtree` in that case; only shell out to git (and only warn on failure) when bookkeeping says the worktree WAS added. 2. `compare_rows` called bare `float()` on ref-payload values in 12 places with no guard. Added `_as_float(value, key)`, raising `AgainstError` naming the offending key on a non-numeric value — the same class of bug MEDIUM-2 fixed for the git/subprocess/JSON layers, one level up. 3. The guard/no-rows early returns left `ref_leak=0.0` (via AgainstResult's dataclass default) while the "config not evaluated" branch used `1.0` for the same "no real ref value" situation. Standardized on 1.0 — `avoid_leak_at_k` is a CEILING metric, so "unknown" should read as the permissive end, matching check_gate's own convention (already how this branch treats a ref that's genuinely missing the key). Left current_leak and the other three placeholders untouched — not the asymmetry this item was about. 4. `_first_commit_with_main_entrypoint` used `--follow`, which would make its SHA best-effort under a future rename of __main__.py. Dropped it — the file has never been renamed, so `--diff-filter=A` alone is exact. The current-side avoid/leak default asymmetry was explicitly left alone per the plan owner's instruction (proven identical on every reachable input by the round-2 differential run; widening it would be speculative).
…s fingerprint (round 4) Root cause: `_search_for_eval` called `mem.search()` without `_track_usage= False`. Each eval sweep runs ~300 searches; search_ops.py's `_stage_record_usage` writes an access-log row (access_count, last_accessed) for every hit unless told not to. `fingerprint_corpus` hashed memvec.db's mtime, which those writes move — so the eval harness moved its own corpus fingerprint just by running, making `corpus_changed` True on essentially every gate run and the [code] attribution unreachable in practice. This also silently inflated `access_count` on whichever memories the eval surfaced — the same signal `memo usefulness` and `dead_weight()` read to decide what's noise. Fix 1 (root cause): `_search_for_eval` now passes `_track_usage=False` through the same signature-introspection guard already used for `disable_reranker`/`_trace`, so test stubs that don't accept the kwarg stay unaffected. Added `test_evaluate_does_not_inflate_access_count` (mock_memory, real store — asserts access_count stays 0 after an eval sweep, contrasted with a real mem.search() call that DOES bump it). Verified by mutation: reverting to a no-op left access_count at 4 instead of 0. Fix 2 (robustness): `fingerprint_corpus` no longer reads db file mtime at all — any reader sharing the DB (recall daemon, memo watch, chat server) can still move mtime mid-gate regardless of fix 1. Replaced it with `count()` (already soft-delete aware) + `MAX(meta.updated)` over live rows, mirroring the `(row_count, max_updated_ts)` idiom already used elsewhere in this codebase (dream_utils._corpus_fingerprint, ask_ops._corpus_version) — access_count/last_accessed and roi_score live in separate `access`/ `memory_health` tables this query never touches, so a read never moves it, while a save/edit/delete of an actual memory does. Added four tests: stable across an eval sweep, stable across a bare `store.touch()`, moves on a save, moves on an edit, moves on a delete. Verified by mutation both ways: reverting to the old mtime implementation kept 4/5 passing by coincidence (count() alone already differs on add/delete) but incidentally exposed a latent bug in the old approach too — its integer-truncated mtime doesn't reliably detect a rapid edit within the same wall-clock second, which content-based MAX(updated) does not have. Both fixes are independently verified: the eval-sweep fingerprint-stability test still passes even with fix 1 reverted (fix 2 alone already ignores mtime), confirming the two layers guard against different failure modes as intended — fix 1 for eval's own writes, fix 2 for every other writer sharing the same DB file.
_validate_against_flags rejected --quick/--max-prompts/--gate/--update-baseline combined with --against, each guarding a silent no-op, but --graph-ab was missing. Execution order makes it costly, not just silent: the `if graph_ab:` block runs unconditionally BEFORE `if against_ref:`, so --graph-ab --against <ref> ran two extra eval_recall.evaluate() sweeps against the live corpus and built graph_ab_payload, then _run_against's sys.exit() discarded it before the printing block was ever reached. Added the same rejection, mirroring the existing guards' shape and comment style, plus test_against_rejects_graph_ab matching the pattern of test_against_rejects_quick/_max_prompts/_gate/_update_baseline.
…bution # Conflicts: # tests/test_cli_eval.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The recall regression gate could not approve a ranking change. It compared
(current code, current corpus)against a machine-local baseline snapshot taken at(old code, old corpus)— the two deltas confounded, so a corpus-driven drop looked identical to a code regression. The documented remedy was--update-baseline, which resets the very number being defended.This makes the gate attributable, and adds the comparison a ranking change actually has to clear.
What changed
The baseline records the corpus it was measured on.
baseline_payloadpersists acorpus_fingerprintalongside the existing metrics. Backward compatible: a baseline lacking the key stays non-enforcing, matching howavoid_at_kandconfigalready degrade.check_gateblames code or drift. A failure is now tagged[code · …]when the corpus is unchanged (attributable to the diff) or[confounded · …]when it moved. Confounded failures still block — blocking was never the defect, the message was, because it pointed at re-baselining. It now points at the comparison below.memo eval recall --against <ref>. Evaluates the same live corpus twice — working-tree code vs the code at a git ref — so the corpus term cancels and the remaining delta is the diff. Two traps it has to survive, both covered by tests: the eval result cache lives instate_dirand is shared across every worktree on the machine, so both sides run--no-cache; and the installedmemois a global uv tool, so the ref side goes throughPYTHONPATH=<worktree>/src python -m memo(hence the new__main__.py) rather than silently evaluating the same code twice.Evaluation stopped writing access rows.
_search_for_evalnow passes_track_usage=False, andfingerprint_corpusis content-based (count:MAX(updated)) instead of mtime-based.Why that last one matters most
Everything above passed three rounds of adversarial review — mutation testing, a 206,624-case differential over
compare_rows, verification against real git repos — and the feature was still inert in practice._search_for_evalcalledmem.search()without suppressing usage tracking. Each sweep ran ~301 searches, each writing access-log rows intomemvec.db— the same file whose mtime fedcorpus_fingerprint. The harness moved the fingerprint by running, socorpus_changedwas true on essentially every gate run and the[code]tag could never fire.No unit test could see it: they all passed
corpus-123andcorpus-456as strings. It surfaced only when the end-to-end verification ran the real thing against the live corpus and looked at which tag came out.There is a second bug underneath: the eval had been inflating
access_counton its own label set, corrupting the exact signalmemo usefulnessreports anddead_weight()uses to decide what memory is noise. Two more instances of that class (eval_ab.py,cli_eval.py'seval_tokensclosure) are being fixed on a separate branch.Verified live, not only in tests
Against the production corpus (10,595 records):
10595:2026-08-07T20:27:50.247-03:00, bothPASS — prec@k 0.697MEMO_RETRIEVAL_BOOST=0:FAIL [code · …] — precision@k 0.200 < baseline 0.697. The corpus is unchanged since the baseline, so this drop is attributable to the diff.— the acceptance criterion, and the outcome that did not reproduce before the last fix--against HEADon a no-op diff:PASS — prec@k 0.697 vs ref 0.697— identical both sides, no cache leakaccesstable byte-identicalReading order for review
eval_recall.py—baseline_payload,check_gate's attribution,_gate_failure_message, and thefingerprint_corpus/_search_for_evalfixeval_against.py— the new module;compare_rowsis where every verdict landscli_eval.py—_run_gate,_resolve_cache_flags,_validate_against_flags, and the--againstbranchThe riskiest surface is
compare_rows. It fails closed on a missing or mismatchedlabels_fingerprint, pins the comparison to the ref's own best config so a different config winning locally cannot mask a regression, and enforces theavoid@k/avoid_leak@kfloors that the baseline gate already enforces.Test plan
pytest tests/test_eval_against.py tests/test_cli_eval.py tests/test_eval_recall.py tests/test_dev_audit.py -q→ 142 passedruff check,ruff format --check,mypy src/memo(515 files) cleanscripts/quality_gate.py→ 179 complexity / 187 exception budgets,eval/quality_baseline.jsonuntouched across the whole branchUnblocks Phase 2 of
docs/SPECS/2026-08-07-repair-program-design.md, which is a ranking change and therefore unverifiable without this.🤖 Generated with Claude Code