From 118559d3a8a3c5bdfe9dbf76506c6278b6c57ce0 Mon Sep 17 00:00:00 2001 From: Nadi Adatepe Date: Wed, 22 Jul 2026 11:16:13 +0200 Subject: [PATCH 1/2] fix(eval): build the vector index so semantic benchmarks can report a number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- code_review_graph/cli.py | 25 ++++ .../eval/benchmarks/agent_baseline.py | 10 ++ code_review_graph/eval/runner.py | 110 +++++++++++++++ tests/test_eval.py | 126 ++++++++++++++++++ 4 files changed, 271 insertions(+) diff --git a/code_review_graph/cli.py b/code_review_graph/cli.py index 4337a124e..c9b416d43 100644 --- a/code_review_graph/cli.py +++ b/code_review_graph/cli.py @@ -840,6 +840,28 @@ def main() -> None: eval_cmd.add_argument("--all", action="store_true", dest="run_all", help="Run all benchmarks") eval_cmd.add_argument("--report", action="store_true", help="Generate report from results") eval_cmd.add_argument("--output-dir", default=None, help="Output directory for results") + eval_cmd.add_argument( + "--embed", + action="store_true", + help=( + "Build the vector index after each graph build. Required by the " + "agent_baseline, search_quality and multi_hop_retrieval " + "benchmarks: without it their natural-language questions hit " + "FTS5 only and return zero results (default: disabled)" + ), + ) + eval_cmd.add_argument( + "--embed-provider", + choices=["local", "openai", "google", "minimax"], + default=None, + help="Provider for --embed (default: local, needs " + "code-review-graph[embeddings])", + ) + eval_cmd.add_argument( + "--embed-model", + default=None, + help="Model for --embed (default: the provider's own default)", + ) # detect-changes detect_cmd = sub.add_parser( @@ -1262,6 +1284,9 @@ def main() -> None: repos=repos, benchmarks=benchmarks, output_dir=getattr(args, "output_dir", None), + embed=getattr(args, "embed", False), + embedding_provider=getattr(args, "embed_provider", None), + embedding_model=getattr(args, "embed_model", None), ) print(f"\nCompleted {len(results)} benchmark(s).") print("Run 'code-review-graph eval --report' to generate tables.") diff --git a/code_review_graph/eval/benchmarks/agent_baseline.py b/code_review_graph/eval/benchmarks/agent_baseline.py index 25ce18151..9a1efbf4e 100644 --- a/code_review_graph/eval/benchmarks/agent_baseline.py +++ b/code_review_graph/eval/benchmarks/agent_baseline.py @@ -180,10 +180,20 @@ def aggregate(results: list[dict]) -> dict: """Aggregate over rows where both sides of the comparison exist.""" ok = [r for r in results if r.get("status") == "ok"] ratios = [float(r["baseline_to_graph_ratio"]) for r in ok] + no_graph = sum(1 for r in results if r.get("status") == "no_graph_results") return { "total_rows": len(results), "ok_rows": len(ok), "error_rows": sum(1 for r in results if r.get("status") == "error"), + # Excluded rows are reported, not just dropped: a run where the graph + # answered nothing must not be readable as "no result" when it is + # really "every query failed". A high no_graph_results_rows count + # against a populated graph means the vector index is missing — + # re-run the eval with --embed. + "no_graph_results_rows": no_graph, + "no_baseline_match_rows": sum( + 1 for r in results if r.get("status") == "no_baseline_match" + ), "median_baseline_to_graph_ratio": ( round(statistics.median(ratios), 1) if ratios else None ), diff --git a/code_review_graph/eval/runner.py b/code_review_graph/eval/runner.py index d2440085a..fa71d4585 100644 --- a/code_review_graph/eval/runner.py +++ b/code_review_graph/eval/runner.py @@ -4,6 +4,7 @@ import csv import logging +import sqlite3 import subprocess from datetime import date from pathlib import Path @@ -144,10 +145,99 @@ def write_csv(results: list[dict], path: Path) -> None: writer.writerows(results) +#: Benchmarks that put a natural-language question through ``hybrid_search``. +#: Without a vector index these fall back to FTS5, which scores a full +#: sentence against no document and returns nothing. +SEMANTIC_BENCHMARKS = frozenset( + {"agent_baseline", "search_quality", "multi_hop_retrieval"}, +) + + +def _embedding_count(store) -> int | None: + """Return the number of stored vectors, or None if the table is absent. + + Only a missing table is treated as "no index". A lock or a malformed + database is a different failure and must not be reported to the user as + "re-run with --embed", which would send them after the wrong problem. + """ + try: + row = store._conn.execute("SELECT count(*) FROM embeddings").fetchone() + except sqlite3.OperationalError as exc: + if "no such table" in str(exc).lower(): + return None + raise + return int(row[0]) if row else 0 + + +def _build_embedding_index( + store, + db_path, + provider: str | None, + model: str | None, +) -> None: + """Bootstrap the vector index for an already-built graph. + + Mirrors ``tools.docs.embed_graph``, but reads the graph through the + runner's already-open ``GraphStore`` rather than opening a second one. + ``EmbeddingStore`` still opens its own connection to the same database — + that is safe here because the two are used sequentially, not + concurrently: vectors are written and orphans purged through the + embedding connection, then nodes are read back through the graph + connection. Both run in autocommit (``isolation_level=None``), so the + reads see committed data with no transaction snapshot in between. + """ + from code_review_graph.embeddings import EmbeddingStore, embed_all_nodes + + try: + emb_store = EmbeddingStore(db_path, provider=provider, model=model) + except ValueError as exc: + logger.error(" embedding index unavailable: %s", exc) + return + + try: + if not emb_store.available: + logger.error( + " embedding provider %r is not available — install " + "code-review-graph[embeddings] for the local provider, or " + "check the cloud provider's environment variables. " + "Semantic benchmarks will report no_graph_results.", + provider or "local", + ) + return + embedded = embed_all_nodes(store, emb_store) + logger.info( + " embedding index: %d new vector(s), %d total", + embedded, + emb_store.count(), + ) + finally: + emb_store.close() + + +def _warn_if_semantic_index_missing(store, benchmark_names: list[str]) -> None: + """Warn before running a semantic benchmark against an unindexed graph. + + The failure is otherwise silent: rows come back ``no_graph_results`` and + ``aggregate()`` excludes them, so the run reports ``median: None`` rather + than an error. + """ + requested = SEMANTIC_BENCHMARKS.intersection(benchmark_names) + if not requested or _embedding_count(store): + return + logger.warning( + " no vector index — %s will score natural-language questions " + "against FTS5 alone and return zero hits. Re-run with --embed.", + ", ".join(sorted(requested)), + ) + + def run_eval( repos: list[str] | None = None, benchmarks: list[str] | None = None, output_dir: str | Path | None = None, + embed: bool = False, + embedding_provider: str | None = None, + embedding_model: str | None = None, ) -> dict[str, list[dict]]: """Run evaluation benchmarks across repositories. @@ -155,6 +245,14 @@ def run_eval( repos: List of repo config names to evaluate (None = all). benchmarks: List of benchmark names to run (None = all). output_dir: Directory for CSV output files. + embed: Build the vector index after the graph build. Default off, + because the local provider loads a model and cloud providers + transmit source-derived text and may incur API cost. Benchmarks + that put a natural-language question through ``hybrid_search`` + (``agent_baseline``, ``search_quality``, ``multi_hop_retrieval``) + need this — FTS5 alone matches nothing on a full sentence. + embedding_provider: Provider for the index (default ``local``). + embedding_model: Exact model (default: provider's own default). Returns: Dict mapping ``{repo}_{benchmark}`` to list of result dicts. @@ -202,6 +300,18 @@ def run_eval( for warning in pp_result.get("warnings", []): logger.warning(" postprocessing: %s", warning) + # run_post_processing's embedding step is a refresh, not a bootstrap: + # refresh_embeddings() returns early on a graph with no existing + # vectors, by design, so no build path can silently load a model or + # incur API cost. The eval framework therefore has to build the index + # explicitly, or every semantic query returns zero hits and the + # affected rows are dropped from the aggregate as "no_graph_results". + if embed: + _build_embedding_index( + store, db_path, embedding_provider, embedding_model, + ) + _warn_if_semantic_index_missing(store, benchmark_names) + for bench_name in benchmark_names: if bench_name not in BENCHMARK_REGISTRY: logger.warning("Unknown benchmark: %s", bench_name) diff --git a/tests/test_eval.py b/tests/test_eval.py index b6c030f0a..9a67a0e93 100644 --- a/tests/test_eval.py +++ b/tests/test_eval.py @@ -963,3 +963,129 @@ def test_reporter_impact_f1_skips_error_and_co_change_rows(): # the co-change row (different metric) must not pollute the column. assert "0.5" in tables assert "0.9" not in tables + + +# -- Semantic index guard (agent_baseline and friends) --------------------- + + +def test_agent_baseline_aggregate_reports_excluded_rows(): + """A run where the graph answered nothing must not read as 'no result'. + + ``ok_rows == 0`` with ``median is None`` is ambiguous on its own: it looks + the same whether zero questions were asked or every query came back empty. + The excluded-row counts disambiguate it. + """ + from code_review_graph.eval.benchmarks import agent_baseline + + results = [ + {"status": "no_graph_results", "baseline_to_graph_ratio": ""}, + {"status": "no_graph_results", "baseline_to_graph_ratio": ""}, + {"status": "no_baseline_match", "baseline_to_graph_ratio": ""}, + ] + agg = agent_baseline.aggregate(results) + + assert agg["ok_rows"] == 0 + assert agg["median_baseline_to_graph_ratio"] is None + assert agg["no_graph_results_rows"] == 2 + assert agg["no_baseline_match_rows"] == 1 + + +def test_agent_baseline_aggregate_counts_zero_on_a_healthy_run(): + from code_review_graph.eval.benchmarks import agent_baseline + + agg = agent_baseline.aggregate([ + {"status": "ok", "baseline_to_graph_ratio": "10.0"}, + {"status": "ok", "baseline_to_graph_ratio": "20.0"}, + ]) + + assert agg["ok_rows"] == 2 + assert agg["no_graph_results_rows"] == 0 + assert agg["no_baseline_match_rows"] == 0 + assert agg["median_baseline_to_graph_ratio"] == 15.0 + + +def test_warns_when_semantic_benchmark_runs_without_a_vector_index(caplog): + """The silent-zero path must announce itself before the benchmark runs.""" + import logging + + from code_review_graph.eval.runner import _warn_if_semantic_index_missing + + with tempfile.TemporaryDirectory() as tmpdir: + repo_path = _make_repo(tmpdir) + store = _build_store(repo_path) + try: + with caplog.at_level(logging.WARNING): + _warn_if_semantic_index_missing(store, ["agent_baseline"]) + finally: + store.close() + + assert "no vector index" in caplog.text + assert "--embed" in caplog.text + + +def test_no_warning_for_benchmarks_that_do_not_use_semantic_search(caplog): + import logging + + from code_review_graph.eval.runner import _warn_if_semantic_index_missing + + with tempfile.TemporaryDirectory() as tmpdir: + repo_path = _make_repo(tmpdir) + store = _build_store(repo_path) + try: + with caplog.at_level(logging.WARNING): + _warn_if_semantic_index_missing(store, ["token_efficiency"]) + finally: + store.close() + + assert "no vector index" not in caplog.text + + +def test_no_warning_once_the_index_is_populated(caplog, monkeypatch): + import logging + + from code_review_graph.eval import runner + + monkeypatch.setattr(runner, "_embedding_count", lambda store: 42) + + with caplog.at_level(logging.WARNING): + runner._warn_if_semantic_index_missing(object(), ["agent_baseline"]) + + assert "no vector index" not in caplog.text + + +def test_embedding_count_reraises_non_missing_table_errors(): + """A lock or a corrupt database must not be reported as 'no index'. + + Reclassifying it would tell the user to re-run with --embed and send + them after the wrong problem. + """ + import sqlite3 + + from code_review_graph.eval.runner import _embedding_count + + class _Boom: + class _Conn: + @staticmethod + def execute(*_args, **_kwargs): + raise sqlite3.OperationalError("database is locked") + + _conn = _Conn() + + with pytest.raises(sqlite3.OperationalError, match="locked"): + _embedding_count(_Boom()) + + +def test_embedding_count_returns_none_for_a_missing_table(): + import sqlite3 + + from code_review_graph.eval.runner import _embedding_count + + class _NoTable: + class _Conn: + @staticmethod + def execute(*_args, **_kwargs): + raise sqlite3.OperationalError("no such table: embeddings") + + _conn = _Conn() + + assert _embedding_count(_NoTable()) is None From 541ff80ce9043e566a7315775ddfb574de32406d Mon Sep 17 00:00:00 2001 From: Nadi Adatepe Date: Wed, 22 Jul 2026 11:16:13 +0200 Subject: [PATCH 2/2] fix(tests): stop the suite writing into the real home directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running `pytest` left entries like this in the developer's own `~/.code-review-graph/registry.json`: {"path": "/tmp/pytest-of-/pytest-0/test_registry_data_dir_overrid0/project", "data_dir": "/tmp/pytest-of-/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 --- code_review_graph/constants.py | 26 ++++++++++ code_review_graph/daemon.py | 88 +++++++++++++++++++++++++++------- code_review_graph/registry.py | 17 +++++-- tests/conftest.py | 36 ++++++++++++++ tests/test_daemon.py | 71 +++++++++++++++++++++++++++ tests/test_registry.py | 74 ++++++++++++++++++++++++++++ 6 files changed, 291 insertions(+), 21 deletions(-) create mode 100644 tests/conftest.py diff --git a/code_review_graph/constants.py b/code_review_graph/constants.py index 993ca3801..2b5e9c74a 100644 --- a/code_review_graph/constants.py +++ b/code_review_graph/constants.py @@ -4,6 +4,7 @@ import math import os +from pathlib import Path def _bounded_float_env( @@ -71,3 +72,28 @@ def _bounded_float_env( IMPACT_SCORE_FLOOR = _bounded_float_env( "CRG_IMPACT_SCORE_FLOOR", 0.05, lower=0.0, upper=1.0, ) + + +#: Overrides the per-user state directory that holds ``registry.json``, +#: ``watch.toml``, ``daemon.pid``, ``daemon-state.json`` and ``logs/``. +#: Follows the same convention as CRG_DATA_DIR. +CRG_HOME_ENV = "CRG_HOME" + +_DEFAULT_CRG_HOME = Path.home() / ".code-review-graph" + + +def crg_home() -> Path: + """Return the per-user state directory for code-review-graph. + + ``$CRG_HOME`` wins when set and non-empty; otherwise + ``~/.code-review-graph``. + + Resolved per call rather than captured in a module-level constant. An + import-time constant cannot be redirected afterwards, which is what let + the test suite write into the real home directory of whoever ran it: by + the time a fixture set the variable, the value had already been frozen. + """ + override = os.environ.get(CRG_HOME_ENV, "").strip() + if override: + return Path(override).expanduser() + return _DEFAULT_CRG_HOME diff --git a/code_review_graph/daemon.py b/code_review_graph/daemon.py index 1ade7efbb..451b2d445 100644 --- a/code_review_graph/daemon.py +++ b/code_review_graph/daemon.py @@ -32,15 +32,71 @@ except ImportError: tomllib = None # type: ignore[assignment] +from .constants import crg_home + logger = logging.getLogger(__name__) # --------------------------------------------------------------------------- # Config file location # --------------------------------------------------------------------------- -CONFIG_PATH: Path = Path.home() / ".code-review-graph" / "watch.toml" -PID_PATH: Path = Path.home() / ".code-review-graph" / "daemon.pid" -STATE_PATH: Path = Path.home() / ".code-review-graph" / "daemon-state.json" +def default_config_path() -> Path: + """Path to ``watch.toml`` under the per-user state directory.""" + return crg_home() / "watch.toml" + + +def default_pid_path() -> Path: + """Path to the daemon PID file.""" + return crg_home() / "daemon.pid" + + +def default_state_path() -> Path: + """Path to the persisted daemon state.""" + return crg_home() / "daemon-state.json" + + +def default_log_dir() -> Path: + """Directory for per-repo daemon logs.""" + return crg_home() / "logs" + + +# These four were module-level constants built from Path.home(). They resolve +# per call now so $CRG_HOME can redirect them: an import-time constant is +# frozen before any caller — a test fixture, a sandboxed run — gets the chance +# to set the variable, which is how the test suite ended up writing into the +# real home directory of whoever ran it. +# +# The PEP 562 shim below keeps the old attribute names working for anything +# that already imported them: both ``daemon.CONFIG_PATH`` and +# ``from …daemon import CONFIG_PATH`` route through ``__getattr__``, and +# ``__dir__`` keeps them visible to introspection. Not covered: ``import *`` +# (this module defines no ``__all__``, and adding one would change what the +# star exports for every other name) and static analysers, which cannot see +# dynamic attributes. Both are acceptable — these were never public API, and +# the alternative is deleting the names outright. +_LAZY_PATHS = { + "CONFIG_PATH": default_config_path, + "PID_PATH": default_pid_path, + "STATE_PATH": default_state_path, +} + + +def __getattr__(name: str) -> Path: + if name in _LAZY_PATHS: + return _LAZY_PATHS[name]() + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + +def __dir__() -> list[str]: + """Include the lazy names so ``dir()`` and tab-completion still find them. + + ``__getattr__`` alone covers attribute access and ``from … import X``, + but names absent from module globals are otherwise invisible to + ``dir()``, ``from … import *`` and static analysers. + """ + return sorted(set(globals()) | set(_LAZY_PATHS)) + + _HEALTH_CHECK_INTERVAL = 30 # --------------------------------------------------------------------------- @@ -66,7 +122,7 @@ class DaemonConfig: session_name: str = "crg-watch" """Logical daemon name (used in log messages and status output).""" - log_dir: Path = field(default_factory=lambda: Path.home() / ".code-review-graph" / "logs") + log_dir: Path = field(default_factory=default_log_dir) """Directory for per-repo log files.""" poll_interval: int = 2 @@ -85,7 +141,7 @@ def load_config(path: Path | None = None) -> DaemonConfig: """Load daemon configuration from a TOML file. Args: - path: Explicit config path. Falls back to :data:`CONFIG_PATH`. + path: Explicit config path. Falls back to :func:`default_config_path`. Returns: A fully-validated :class:`DaemonConfig`. @@ -99,7 +155,7 @@ def load_config(path: Path | None = None) -> DaemonConfig: "Install it with: pip install tomli" ) - config_path = path or CONFIG_PATH + config_path = path or default_config_path() if not config_path.exists(): logger.info("Config file not found at %s — using defaults", config_path) @@ -225,9 +281,9 @@ def save_config(config: DaemonConfig, path: Path | None = None) -> None: Args: config: The daemon configuration to persist. - path: Explicit config path. Falls back to :data:`CONFIG_PATH`. + path: Explicit config path. Falls back to :func:`default_config_path`. """ - config_path = path or CONFIG_PATH + config_path = path or default_config_path() config_path.parent.mkdir(parents=True, exist_ok=True) config_path.write_text(_serialize_toml(config), encoding="utf-8") logger.info("Config saved to %s", config_path) @@ -248,7 +304,7 @@ def add_repo_to_config( Args: repo_path: Path to the repository (will be resolved to absolute). alias: Optional short name. Derived from dirname if *None*. - config_path: Explicit config file path. Falls back to :data:`CONFIG_PATH`. + config_path: Explicit config file path. Falls back to :func:`default_config_path`. Returns: The updated :class:`DaemonConfig`. @@ -294,7 +350,7 @@ def remove_repo_from_config( Args: path_or_alias: Either the absolute/relative repo path or its alias. - config_path: Explicit config file path. Falls back to :data:`CONFIG_PATH`. + config_path: Explicit config file path. Falls back to :func:`default_config_path`. Returns: The updated :class:`DaemonConfig`. @@ -323,14 +379,14 @@ def remove_repo_from_config( def write_pid(pid: int | None = None, path: Path | None = None) -> None: """Write the current (or given) PID to the PID file.""" - pid_path = path or PID_PATH + pid_path = path or default_pid_path() pid_path.parent.mkdir(parents=True, exist_ok=True) pid_path.write_text(str(pid or os.getpid()), encoding="utf-8") def read_pid(path: Path | None = None) -> int | None: """Read the daemon PID from disk. Returns None if missing/invalid.""" - pid_path = path or PID_PATH + pid_path = path or default_pid_path() if not pid_path.exists(): return None try: @@ -341,7 +397,7 @@ def read_pid(path: Path | None = None) -> int | None: def clear_pid(path: Path | None = None) -> None: """Remove the PID file.""" - pid_path = path or PID_PATH + pid_path = path or default_pid_path() try: pid_path.unlink(missing_ok=True) except OSError: @@ -455,7 +511,7 @@ def load_state(path: Path | None = None) -> dict[str, Any]: Returns a dict mapping alias to ``{"pid": int, "path": str}``. Returns an empty dict if the file is missing or corrupt. """ - state_path = path or STATE_PATH + state_path = path or default_state_path() if not state_path.exists(): return {} try: @@ -599,8 +655,8 @@ def __init__( config_path: Path | None = None, ) -> None: self._config: DaemonConfig = config or load_config(config_path) - self._config_path: Path = config_path or CONFIG_PATH - self._state_path: Path = STATE_PATH + self._config_path: Path = config_path or default_config_path() + self._state_path: Path = default_state_path() self._children: dict[str, subprocess.Popen[bytes]] = {} self._current_repos: dict[str, WatchRepo] = {} self._config_watcher: ConfigWatcher | None = None diff --git a/code_review_graph/registry.py b/code_review_graph/registry.py index 888867dd0..70a322471 100644 --- a/code_review_graph/registry.py +++ b/code_review_graph/registry.py @@ -13,22 +13,29 @@ from collections import OrderedDict from pathlib import Path +from .constants import crg_home + logger = logging.getLogger(__name__) -# Default registry path -_REGISTRY_DIR = Path.home() / ".code-review-graph" -_REGISTRY_PATH = _REGISTRY_DIR / "registry.json" +def default_registry_path() -> Path: + """Return the full path to ``registry.json``. + + Lives under :func:`~code_review_graph.constants.crg_home`, so ``$CRG_HOME`` + redirects it along with the rest of the per-user state. + """ + return crg_home() / "registry.json" class Registry: """Manages a JSON-based registry of code-review-graph repositories. Each entry stores the repo path and an optional alias. - The registry lives at ``~/.code-review-graph/registry.json``. + The registry lives at ``~/.code-review-graph/registry.json``, or under + ``$CRG_HOME`` when that is set. """ def __init__(self, path: Path | None = None) -> None: - self._path = path or _REGISTRY_PATH + self._path = path or default_registry_path() self._path.parent.mkdir(parents=True, exist_ok=True) self._lock = threading.Lock() self._repos: list[dict[str, str]] = [] diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 000000000..64c2ca1b1 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,36 @@ +"""Shared test fixtures. + +Keeps code-review-graph's own per-user state out of the developer's real +home directory. Scoped deliberately: the editor-integration installers in +``skills.py`` write to other user-level locations (``~/.codex``, +``~/.cursor``, ``~/.config/opencode``) that are outside CRG state and are +not covered here — those tests patch ``Path.home()`` themselves. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture(autouse=True) +def isolated_crg_home(tmp_path_factory, monkeypatch): + """Redirect the per-user state directory into a temporary directory. + + ``~/.code-review-graph`` holds ``registry.json``, ``watch.toml``, + ``daemon.pid``, ``daemon-state.json`` and ``logs/``. Two paths reached + the real one: + + * ``Registry()`` defaults there, and ``incremental.get_data_dir()`` + constructs one internally — so any test touching data-dir resolution + both read and wrote the registry of whoever ran the suite. That put + pytest tmp paths into a developer's home directory, and made those + tests depend on machine state: a developer with a registered repo + could get different results from one without. + * ``daemon`` built its config/PID/state paths from ``Path.home()``. + + Autouse and unconditional: an opt-in fixture would silently stop + protecting a test the day someone forgets to request it. + """ + home = tmp_path_factory.mktemp("crg-home") + monkeypatch.setenv("CRG_HOME", str(home)) + return home diff --git a/tests/test_daemon.py b/tests/test_daemon.py index ff4776529..a607c1875 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -1279,3 +1279,74 @@ def test_handle_logs_reads_lines(self, tmp_path): assert mock_print.call_count == 3 printed_lines = [str(c.args[0]) for c in mock_print.call_args_list] assert printed_lines == ["line3", "line4", "line5"] + + +class TestPerUserStateLocation: + """Daemon state must follow $CRG_HOME, not a frozen Path.home().""" + + def test_defaults_live_under_crg_home(self, tmp_path, monkeypatch): + from code_review_graph import daemon + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "state")) + + assert daemon.default_config_path() == tmp_path / "state" / "watch.toml" + assert daemon.default_pid_path() == tmp_path / "state" / "daemon.pid" + assert daemon.default_state_path() == tmp_path / "state" / "daemon-state.json" + assert daemon.default_log_dir() == tmp_path / "state" / "logs" + + def test_defaults_are_not_frozen_at_import(self, tmp_path, monkeypatch): + """The original bug: a module constant captured $HOME at import time. + + The autouse conftest fixture sets CRG_HOME before any test runs, so a + constant would already hold the wrong value and no later override + could move it. + """ + from code_review_graph import daemon + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "first")) + first = daemon.default_pid_path() + monkeypatch.setenv("CRG_HOME", str(tmp_path / "second")) + + assert daemon.default_pid_path() != first + assert daemon.default_pid_path() == tmp_path / "second" / "daemon.pid" + + def test_legacy_constant_names_still_resolve(self, tmp_path, monkeypatch): + """CONFIG_PATH/PID_PATH/STATE_PATH kept working via the PEP 562 shim.""" + from code_review_graph import daemon + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "state")) + + assert daemon.CONFIG_PATH == tmp_path / "state" / "watch.toml" + assert daemon.PID_PATH == tmp_path / "state" / "daemon.pid" + assert daemon.STATE_PATH == tmp_path / "state" / "daemon-state.json" + + def test_unknown_attribute_still_raises(self): + from code_review_graph import daemon + + with pytest.raises(AttributeError, match="no attribute 'NOPE'"): + _ = daemon.NOPE + + def test_bare_daemon_config_logs_under_crg_home(self, tmp_path, monkeypatch): + """DaemonConfig()'s default_factory must not point at the real home.""" + from code_review_graph.daemon import DaemonConfig + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "state")) + + assert DaemonConfig().log_dir == tmp_path / "state" / "logs" + + def test_legacy_names_are_visible_to_dir(self): + """__getattr__ alone leaves the names invisible to introspection.""" + from code_review_graph import daemon + + names = dir(daemon) + assert "CONFIG_PATH" in names + assert "PID_PATH" in names + assert "STATE_PATH" in names + # The real module globals are still there too. + assert "WatchDaemon" in names + + def test_legacy_names_work_through_from_import(self, tmp_path, monkeypatch): + monkeypatch.setenv("CRG_HOME", str(tmp_path / "state")) + from code_review_graph.daemon import CONFIG_PATH + + assert CONFIG_PATH == tmp_path / "state" / "watch.toml" diff --git a/tests/test_registry.py b/tests/test_registry.py index 1589aa3c4..973d01def 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -334,3 +334,77 @@ def test_backward_compatibility(self): data_dir = Path(self.tmp_dir) / "data" entry = self.registry.set_data_dir(str(repo), str(data_dir)) assert entry["data_dir"] == str(data_dir.resolve()) + + +class TestRegistryLocationIsolation: + """The registry must never fall back to the real home directory in tests.""" + + def test_default_path_follows_the_env_override(self, tmp_path, monkeypatch): + from code_review_graph.registry import default_registry_path + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "elsewhere")) + assert default_registry_path() == tmp_path / "elsewhere" / "registry.json" + + def test_override_is_read_per_call_not_at_import(self, tmp_path, monkeypatch): + """A module-level constant would freeze the value at first import. + + The autouse fixture sets CRG_HOME before any test runs, so an + import-time constant would capture the wrong directory and every later + override would be ignored. + """ + from code_review_graph.registry import default_registry_path + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "first")) + first = default_registry_path() + monkeypatch.setenv("CRG_HOME", str(tmp_path / "second")) + assert default_registry_path() != first + assert default_registry_path() == tmp_path / "second" / "registry.json" + + def test_blank_override_falls_back_to_home(self, monkeypatch): + from code_review_graph.constants import crg_home + + monkeypatch.setenv("CRG_HOME", " ") + assert crg_home() == Path.home() / ".code-review-graph" + + def test_bare_registry_writes_under_the_override(self, tmp_path, monkeypatch): + """Registry() with no path argument must land in the sandbox. + + This is the leak that put pytest tmp paths into a developer's real + ~/.code-review-graph/registry.json. + """ + # Point Path.home() at a fake home too, so the assertion that nothing + # was written there needs no access to the developer's real one. + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setattr(Path, "home", classmethod(lambda cls: fake_home)) + + sandbox = tmp_path / "sandbox" + monkeypatch.setenv("CRG_HOME", str(sandbox)) + + repo = tmp_path / "project" + repo.mkdir() + (repo / ".git").mkdir() + + Registry().register(str(repo), alias="leaky") + + sandboxed = sandbox / "registry.json" + assert sandboxed.exists() + assert "leaky" in sandboxed.read_text(encoding="utf-8") + assert not (fake_home / ".code-review-graph").exists() + + def test_get_data_dir_uses_the_sandboxed_registry(self, tmp_path, monkeypatch): + """incremental.get_data_dir() builds its own Registry() internally.""" + from code_review_graph.incremental import get_data_dir + + monkeypatch.setenv("CRG_HOME", str(tmp_path / "sandbox")) + monkeypatch.delenv("CRG_DATA_DIR", raising=False) + + repo = tmp_path / "project" + repo.mkdir() + (repo / ".git").mkdir() + external = tmp_path / "external" + + Registry().set_data_dir(str(repo), str(external)) + + assert get_data_dir(repo) == external.resolve() + assert (tmp_path / "sandbox" / "registry.json").exists()