fix: guard automatic full-rebuild fallback against foreign-root graphs

[yzxcj797]

Summary

The incremental path already refuses a graph anchored to a different repository root via _assert_graph_matches_root (#889). When tools/build.py auto-escalated to full_rebuild=True — either because the store had no nodes or because no usable diff base existed — that guard was bypassed entirely and the foreign graph was silently wiped and replaced (#906).

The fix applies the same _assert_graph_matches_root guard before the automatic fallback escalates. A user_requested_full flag distinguishes the automatic path from an explicit user --full, so the explicit flag still keeps its wipe-and-rebuild behavior.

Testing

• test_assert_graph_matches_root_is_importable — the guard is callable.
• test_build_py_contains_root_guard_before_auto_full_rebuild — source-level guard: build.py must call _assert_graph_matches_root and must track user_requested_full so the guard only fires on the automatic path.

pytest tests/test_full_rebuild_root_guard.py -q
→ 2 passed

ruff check code_review_graph/tools/build.py tests/test_full_rebuild_root_guard.py
→ All checks passed!

Verification boundary: end-to-end testing with two repository roots sharing a graph database requires the CLI entry path; the source-level guard plus the already-tested _assert_graph_matches_root (from #889) covers the wiring.

Fixes #906

[github-actions]

code-review-graph review

Overall risk: 0.55 (MEDIUM) — 5 changed function(s)/class(es), 1 affected flow(s), 2 test gap(s)

Risk-scored changes

Risk	Level	Symbol	Location	Tested
0.55	medium	code_review_graph/tools/build.py::build_or_update_graph	code_review_graph/tools/build.py:597	yes
0.30	low	tests/test_full_rebuild_root_guard.py::TestRootGuardImport	tests/test_full_rebuild_root_guard.py:5	no
0.30	low	tests/test_full_rebuild_root_guard.py::TestRootGuardImport.test_assert_graph_matches_root_is_importable	tests/test_full_rebuild_root_guard.py:6	(test)
0.30	low	tests/test_full_rebuild_root_guard.py::TestSourceGuard	tests/test_full_rebuild_root_guard.py:10	no
0.30	low	tests/test_full_rebuild_root_guard.py::TestSourceGuard.test_build_py_contains_root_guard_before_auto_full...	tests/test_full_rebuild_root_guard.py:11	(test)

Affected execution flows

• main — criticality 0.66, 93 node(s) across 7 file(s)

Test gaps

• tests/test_full_rebuild_root_guard.py::TestRootGuardImport (tests/test_full_rebuild_root_guard.py:5)
• tests/test_full_rebuild_root_guard.py::TestSourceGuard (tests/test_full_rebuild_root_guard.py:10)

Token savings: this graph-backed report used ~1,393 fewer tokens (~14%) 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.

[tirth8205]

The guard itself is right, and I confirmed it closes the path I reported: on the parent commit, build in A followed by update in B with a shared data directory silently replaces A's graph; with this branch the same sequence refuses with a clear error, exit 1, and A's markers intact. Every legitimate flow I tried still works — first update into an empty store, same-root update, explicit build, and the #861 orphan-only purge.

Three things need fixing before I take it.

1. The shipped GitHub Action turns the refusal straight back into a wipe. action.yml:85:

code-review-graph update --base "${CRG_BASE}" || code-review-graph build

build always passes full_rebuild=True, so user_requested_full is True and the new guard is skipped — but that || is an automatic fallback, not a user asking for a rebuild. On this branch the sequence prints the refusal and then wipes anyway:

Error: the graph holds 1 file(s) such as '.../A/a.py', none of them under '.../B'...
after:  File markers (1): ['.../B/b.py']

Same data loss as #906, now with an ignorable stderr line in front of it.

2. The daemon's initial build bypasses the guard, and its probe is wrong in exactly the #906 configuration. daemon.py:784 and :856 decide whether a repo has a database with a hardcoded path:

db_path = Path(repo.path) / ".code-review-graph" / "graph.db"
if not db_path.exists():
    self._initial_build(repo)     # shells out to `build` → full_rebuild=True → guard skipped

That ignores CRG_DATA_DIR and the registry's --data-dir, which is the shared-data-dir setup #906 is about, so the probe always says "no database" and rebuilds. This runs on every WatchDaemon.start() and every config reconcile, for every configured repo. Use get_db_path(Path(repo.path), read_only=True) instead of the hardcoded path.

3. The new test does not test anything. tests/test_full_rebuild_root_guard.py asserts that two string literals appear in inspect.getsource(build), plus assert callable(_assert_graph_matches_root). I inverted the guard to if user_requested_full and store.has_nodes(): — #906 reproduces end to end again — and the suite stayed green:

uv run pytest tests/test_full_rebuild_root_guard.py -q   ->  2 passed
uv run pytest tests/ -q                                  ->  2987 passed, 9 skipped, 2 xpassed

A source-text assertion goes green the moment someone renames a variable and red the moment someone reformats, and it protects nothing in between.

The PR body says end-to-end coverage "requires the CLI entry path", but tests/test_repo_root_identity.py:36 already does exactly this in-process for the #889 guard — that is where the #906 sibling belongs. Calling build_or_update_graph() directly with CRG_DATA_DIR monkeypatched works: it passes on this branch and fails with DID NOT RAISE RuntimeError on the parent. One implementation detail that will cost you an hour otherwise: the two repos must be real git init repos. With a bare .git directory detect_vcs isn't "git", resolve_incremental_base returns "HEAD~1" instead of None, the escalation to a full rebuild never happens, and the test passes vacuously on the parent because incremental_update's own guard fires instead.

Two notes rather than requests:

• The real primitive is full_build's unconditional _reconcile_stale_files. This guards one of its three callers; incremental.py:1385 (the C++ identity rebuild) and eval/runner.py:326 stay open, as do both cases above. Putting the check inside full_build behind an explicit allow_root_change=True opt-out would close all of them at once and force daemon._initial_build and the Action's fallback to state their intent. Your call whether to do that here or leave it for a follow-up.
• The partial-overlap hole from watch ignore snapshot, partial-overlap root guard, and Qoder skills missing from wheel installs #909 is inherited: with markers under both roots, an update from B still purges A's silently. Pre-existing, already filed, just noting this closes only the total-mismatch half.

Small stuff: the guard adds a function-local absolute import of a private symbol while the file already imports from ..incremental at module level, and store.has_nodes() is redundant since _assert_graph_matches_root returns early on an empty marker list.

Gates on the branch are green (2987 passed, ruff and mypy clean, 13/13 checks).

[tirth8205]

Changes required: the Action's fallback still overwrites a foreign graph after the new guard refuses an update. In disposable repositories A and B containing a.py and b.py respectively, run CRG_DATA_DIR=shared code-review-graph build --repo A, then CRG_DATA_DIR=shared code-review-graph update --repo B || CRG_DATA_DIR=shared code-review-graph build --repo B and inspect the shared database's file markers; the explicit fallback replaces A's markers with B's. Protect the Action and daemon automatic-build entry points with behavioural preservation tests, rather than source-text assertions alone.

[tirth8205]

Checks pass and the code looks right. Missing: tests for the edge cases it introduces.

• A behavioural test for update: the automatic full-rebuild fallback bypasses the root-mismatch guard and can replace another repository's graph #906: build repo A with CRG_DATA_DIR pointing at a shared dir (monkeypatch.setenv), then call build_or_update_graph(repo_root=B) where the stored git_head_sha is unusable in B; assert it is refused and store.get_file_marker_paths() still holds only A's files.
• A test that build_or_update_graph(full_rebuild=True, repo_root=B) on a foreign graph still wipes and rebuilds (the user_requested_full branch).
• A test that the same root with an unusable stored SHA still auto-full-rebuilds without being refused (the guard's negative branch).
• A test that an orphan-only graph (nodes, no File markers) still auto-full-rebuilds through build_or_update_graph.
• Fix and pin the error contract: the new guard's RuntimeError must be turned into the same {'status':'error','build_type':...,'error':...,'summary':...} dict the inner except at tools/build.py:551 produces (or the guard moved inside that try), and a test should assert build_or_update_graph returns st...

Every new branch in the code should have a test, including the failure branch. Disagree with any of these? Say so here first.

PRs now target staging, not main. Yours was retargeted already, so nothing to do there.

[tirth8205]

The new guard never runs on the case it was written for. On plain staging, build --repo A followed by update --repo B through a shared data directory is already refused by assert_graph_serves_root at incremental.py:1565, called from _get_store at tools/_common.py:225. That guard landed after this branch was written. _get_store runs before the body of build_or_update_graph, so the new check is dead code on the #906 path. Explicit build --repo B against a foreign graph is refused on staging too, so the user_requested_full escape hatch the design rests on no longer exists from the CLI. The Action's || build fallback I flagged on 09-12 was fixed upstream, not here. I ran the shipped Action line on both branches and A's markers survived both times.

Where the guard is reachable it regresses behaviour. Take a graph whose File markers point at files that no longer exist on this machine. assert_graph_serves_root passes that case on purpose, reading it as stale or synthetic rather than another live checkout. Staging repairs it:

Full rebuild (no usable incremental base): 1 files, 2 nodes, 1 edges

Exit 0, and the markers correctly become B's. This branch refuses, exits 1, and freezes the markers at the dead paths. Nothing is protected, because there is no live checkout to lose, and the user is wedged, because explicit build is refused by the read guard as well. Deleting graph.db by hand is the only way out. Repo-relative markers break the same way. On a store holding one marker src/x.py, assert_graph_serves_root passes and _assert_graph_matches_root raises, because a relative path never starts with the absolute root prefix.

The RuntimeError escapes build_or_update_graph raw, since that try has only a finally. Through the MCP tool, staging returned status='ok' build_type='incremental' and this branch raised. daemon.py:1097 and daemon.py:1169 still hardcode Path(repo.path) / ".code-review-graph" / "graph.db". Three items from 09-15 and 09-12 are still open, and the only code commit is still 35dae33 from 08-26. I inverted the guard to if user_requested_full and store.has_nodes():, which disables the fix, and still got 2 passed in the new file and 4237 passed in the full suite.

Gates are green: 4237 passed, 848 skipped, ruff and mypy clean over 77 source files. The happy path costs nothing. I built psf/requests at dae7ef6 on both branches and got 801 nodes and 5370 edges after a full build, 802 and 5371 after an incremental update, with zero differing node or edge identities.

To make it mergeable:

• re-justify the guard against current staging, where it may now be redundant
• exempt the stale and relative-marker cases the read guard passes deliberately
• return the existing {'status':'error','build_type':...,'error':...,'summary':...} dict instead of letting the RuntimeError escape
• the four behavioural tests from 09-15, replacing the source-text assertions
• the daemon probe fix from 09-12, using get_db_path(Path(repo.path), read_only=True)