fix(eval): build and reuse the requested vector index safely - #757
Merged
Conversation
… number `run_eval` built the graph and called `run_post_processing(store)`, but that step's embedding refresh is a refresh, not a bootstrap: `refresh_embeddings()` returns early on a graph with no existing vectors, deliberately, so that no build path can silently load a model or incur API cost. Nothing else in the eval framework populated the index. So every benchmark that puts a natural-language question through `hybrid_search` — `agent_baseline`, `search_quality`, `multi_hop_retrieval` — fell through to FTS5, which scores a full sentence against no matching document and returns nothing. Every row came back `status="no_graph_results"`, and `aggregate()` excludes those rows, so the run reported `ok_rows: 0` / `median: None` instead of an error. Reproduced on the shipped fastapi config: all three `agent_questions` returned 0 hits against a healthy graph (6287 nodes, FTS5 populated with 6287 rows). After building the index, the same three questions return 28.1x / 82.4x / 68.0x. That is why no `agent_baseline` CSVs exist under `evaluate/results/` even though README.md cites their path. Changes: - `run_eval(embed=..., embedding_provider=..., embedding_model=...)` with a new `_build_embedding_index()` that mirrors `tools.docs.embed_graph` but reuses the runner's already-open store instead of opening a second connection to the same database. Default off, so the cost invariant that `refresh_embeddings` protects is unchanged. - `_warn_if_semantic_index_missing()` runs before the benchmarks and names the affected ones, so the failure announces itself instead of arriving as an empty aggregate. - `agent_baseline.aggregate()` reports `no_graph_results_rows` and `no_baseline_match_rows`. Excluded rows are now counted, not just dropped: "no result" and "every query failed" had the same signature before. - `eval --embed` / `--embed-provider` / `--embed-model` on the CLI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Running `pytest` left entries like this in the developer's own
`~/.code-review-graph/registry.json`:
{"path": "/tmp/pytest-of-<user>/pytest-0/test_registry_data_dir_overrid0/project",
"data_dir": "/tmp/pytest-of-<user>/pytest-0/.../external"}
Two routes reached the real home directory:
- `Registry()` defaults to `~/.code-review-graph/registry.json`, and
`incremental.get_data_dir()` constructs one internally. Tests covering
data-dir resolution therefore both read and wrote the registry of whoever
ran the suite. Besides the pollution, that made those tests depend on
machine state: a developer with a registered repo could get a different
result from one without.
- `daemon.py` built `CONFIG_PATH`, `PID_PATH`, `STATE_PATH` and
`DaemonConfig.log_dir` from `Path.home()` as import-time constants.
An import-time constant cannot be redirected after the fact, which is the
core of the problem: by the time a fixture could set anything, the value is
already frozen. So the paths resolve per call now.
- `constants.crg_home()` reads `$CRG_HOME`, falling back to
`~/.code-review-graph`. Same convention as the existing `CRG_DATA_DIR`.
- `registry.default_registry_path()` and the four `daemon.default_*()`
helpers route through it.
- `daemon.CONFIG_PATH` / `PID_PATH` / `STATE_PATH` keep working through a
PEP 562 module `__getattr__`, so nothing that imported them breaks.
- `tests/conftest.py` points `$CRG_HOME` at a tmp directory for every test.
Autouse and unconditional — an opt-in fixture stops protecting a test the
day someone forgets to request it.
Verified by deleting `~/.code-review-graph` and running the full suite: it
is no longer recreated. Ten regression tests cover both modules, including
that the values are not frozen at import and that the legacy attribute names
still resolve.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4 tasks
code-review-graph reviewOverall risk: 0.65 (MEDIUM) — 68 changed function(s)/class(es), 0 affected flow(s), 20 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~56,816 fewer tokens (~86%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
This was referenced Jul 27, 2026
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
2 participants
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.
Integrates both contributor-authored commits from #710 against current
main, preserving their original authors, then fixes the remaining end-to-end gaps found during maintainer validation.The original branch built a custom provider/model index but benchmark searches still queried the default embedding identity, producing
no_graph_results. It also leftGraphStoreopen when embedding bootstrap failed. This integration propagates the exact provider/model through agent-baseline, search-quality, and multi-hop searches, closes the store on failure, and keeps both the Unicode registry tests and isolated-home behavior.Exact-head validation:
Incorporates #710.
Closes #711.
Closes #712.