fix(graph): make concerns/reflection writes actually atomic

[pradeepmouli]

Fixes #67 (Bug 2 of 2). Depends on #68 (this branch is stacked on it) -- the diff will shrink to just this PR's changes once #68 merges.

What

write_concerns and reflection's equivalent write_resolves_to issued BEGIN TRANSACTION/COMMIT through raw_query, believing that made their delete-then-recreate loop atomic. It never did:

• KuzuBackend::raw_query opens a fresh connection per call, so transaction-control statements issued through it can never span more than the one statement they're attached to.
• Neo4jBackend::raw_query no-ops the same statements for a different reason (Neo4j transactions are driver-level, not Cypher).

Both backends' raw_query already document this and deliberately no-op those statements rather than error -- but neither caller knew that, so every individual DETACH DELETE/CREATE in the loop auto-committed independently. A crash mid-loop today already means the old concerns/RESOLVES_TO edges are gone and only some of the new ones landed -- live data-loss exposure, not hypothetical.

Fix

• GraphStore::transaction<T>(&self, f: impl FnOnce(&Connection) -> Result<T>) -> Result<T> -- opens one connection, issues a real BEGIN TRANSACTION, runs f against it, commits on Ok, rolls back on Err.
• Two new GraphBackend trait methods, replace_concerns and replace_resolves_to, following the same "backend owns the transaction" design as the existing write_calls_service_edges. New Concern/ResolvesToEdge structs live in backend.rs (mirroring CallsServiceEdge) rather than reusing the analysis-pass types (ConcernMatch/ReflectionSite), keeping those out of the storage layer.
• KuzuBackend implements both via the new store.transaction().
• Neo4jBackend implements both as a single chained Cypher statement (delete + UNWIND-based recreate) -- matches write_calls_service_edges's existing precedent of "one auto-committed statement, no driver-level transaction needed" for that backend.
• write_calls_service_edges itself also migrates onto store.transaction(), removing its hand-rolled BEGIN/loop/COMMIT/ROLLBACK boilerplate (same behavior, no longer duplicated).
• The free write_concerns/write_resolves_to functions in concerns/mod.rs/reflection/mod.rs are deleted; their one call site each now builds the new backend-layer structs and calls the trait method directly.

Testing

New file tests/concerns_reflection_atomic_writes.rs, 7 tests:

• replace_concerns_rolls_back_atomically_on_mid_batch_failure -- forces a mid-batch failure (two Concerns with the same id, colliding on Kùzu's primary key) and asserts a pre-existing concern survives untouched, proving the whole batch rolls back together. This fails against the pre-fix code (the old per-row let _ = swallowed the error entirely and always returned Ok, let alone rolled back).
• Direct commit/rollback tests for GraphStore::transaction().
• Basic correctness tests for both new trait methods (creates edges, replaces rather than accumulates, empty-input still clears old data).

All pass under both default and --features remote (neo4j). cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings both clean under both feature sets. Pre-commit hook's perf suite (write_lock_perf, groups_watch_perf, index_perf) all pass.

Note: the Neo4j-side Cypher hasn't been run against a live instance -- matches this codebase's existing precedent (the reference write_calls_service_edges Neo4j test is #[ignore]d for the same reason).

[murari316]

Automated review (Fable, adversarially verified). Recommendation: request-changes.

The PR fixes a real, well-diagnosed atomicity bug: BEGIN/COMMIT issued through raw_query were deliberately no-op'd by both backends, so the delete-then-recreate loops for Concern nodes and RESOLVES_TO edges auto-committed per statement and a mid-loop failure lost data. The Kuzu side is solid — a proper GraphStore::transaction helper, migration of write_calls_service_edges onto it, and a genuine regression test that forces a mid-batch failure and proves rollback. However, the new Neo4j implementations have a classic Cypher cardinality bug (MATCH...DELETE chained into UNWIND without an aggregating WITH) that makes them silently write nothing on an empty DB and duplicate rows otherwise, and that path has zero test coverage; the new write path also skips the repo's advisory write-lock invariant.

---

[BLOCKER] correctness — crates/infigraph-core/src/graph/neo4j_backend.rs:~1879 (new replace_concerns query)

The chained Cypher MATCH (c:Concern) DETACH DELETE c WITH 1 AS _cleared UNWIND $concerns ... is broken in both cardinality directions. (1) If zero Concern nodes exist, MATCH yields zero rows, so every downstream clause (UNWIND/CREATE) executes zero times — on a fresh remote project the method writes nothing, and since the DB then still has zero Concern nodes, it never writes anything on any subsequent run either. (2) If N Concern nodes exist, WITH 1 AS _cleared is a per-row projection (no aggregation/DISTINCT), so it carries N rows into UNWIND and each new concern is CREATEd N times — silent duplication, or a hard failure on every run if the remote schema has a uniqueness constraint on Concern.id. Fix: WITH count(*) AS _cleared (aggregation over zero rows yields exactly one row) or wrap the delete in a CALL { MATCH (c:Concern) DETACH DELETE c } subquery. The write_calls_service_edges precedent cited in the PR body doesn't have this trap because it is append-only UNWIND with no leading MATCH-DELETE.

Failure scenario: Fresh Neo4j remote project, first detect_cross_cutting run with 5 matches: replace_concerns returns Ok but creates 0 Concern nodes and 0 HAS_CONCERN edges, forever. Alternatively, DB with 10 existing concerns and a new batch of 5: 50 Concern nodes are created (each concern 10x).

[BLOCKER] correctness — crates/infigraph-core/src/graph/neo4j_backend.rs:~1909 (new replace_resolves_to query)

Same cardinality defect as replace_concerns: MATCH ()-[r:RESOLVES_TO]->() DELETE r WITH 1 AS _cleared UNWIND $edges .... Zero existing RESOLVES_TO edges (the normal first-run state) means zero rows reach UNWIND and no edges are ever created; N existing edges means every new edge is created N times. Here the duplicates are relationships, which no node uniqueness constraint would catch — silent duplication is guaranteed on the multiplication path. Same fix: WITH count(*) AS _cleared or a CALL subquery for the delete.

Failure scenario: First detect_reflection run against a remote (Neo4j) project resolves 3 reflection sites; replace_resolves_to returns Ok but the graph gains zero RESOLVES_TO edges, and every later run also writes nothing because the edge count stays at zero.

[MAJOR] concurrency — crates/infigraph-core/src/graph/store.rs:~127 (new GraphStore::transaction); also kuzu_backend.rs replace_concerns/replace_resolves_to (~L483, ~L506)

The new write path never acquires the advisory write lock, violating the documented repo invariant ('the graph DB is single-writer — write paths take an advisory lock; a new write path needs one too'; compare GraphStore::remove_file at store.rs:117-121 which takes it). The gap is admittedly pre-existing — the old raw_query-based path didn't lock either — but this PR formalizes the path as first-class trait methods and, by replacing per-statement auto-commits with one long explicit transaction, materially widens the contention window. Caution on the fix: don't take the lock inside transaction() itself — write_calls_service_edges now routes through it and its callers (e.g. link_cross_service_calls in group mode) may already hold the WriteLock, and the file-based lock is not reentrant. Acquire it in replace_concerns/replace_resolves_to, or document 'caller must hold WriteLock' and take it in tool_detect_cross_cutting / tool_detect_reflection.

Failure scenario: MCP server with an in-process/cross-process file watcher: tool_detect_cross_cutting runs replace_concerns' multi-statement transaction while a watcher upsert (which does hold the lock) is mid-flight. Two concurrent writers on a single-writer Kuzu DB — the second write transaction fails or, per this PR's own store_util doc comment, leaves connection transaction state wedged for subsequent statements.

[MAJOR] test-coverage — crates/infigraph-core/tests/concerns_reflection_atomic_writes.rs:1-265

The new test suite is Kuzu-only. The Neo4j implementations of replace_concerns/replace_resolves_to — exactly where the two blocker bugs above live — have zero coverage, not even the #[ignore]d live-instance tests that tests/neo4j_backend.rs already provides for write_calls_service_edges (test_neo4j_write_calls_service_edges_* at L338-385). Even an ignored test exercising 'empty DB then write' and 'write twice' would have caught both the zero-row wipeout and the row multiplication. Add the equivalent #[ignore]d neo4j tests alongside the existing precedent.

Failure scenario: CI passes on both feature sets (the neo4j feature only compiles the code), the PR merges, and the first remote-mode user gets a graph where detect_cross_cutting/detect_reflection silently persist nothing.

[MINOR] error-handling — crates/infigraph-core/src/graph/store.rs:~135 (transaction COMMIT arm)

If f succeeds but COMMIT fails, transaction() returns the error without attempting ROLLBACK, leaving an open transaction on the connection. The connection is dropped immediately so Kuzu should discard it, but this PR's own copy_edges_with_bad_record_retry doc comment documents that a failed statement can leave a connection's transaction bookkeeping wedged ('Invalid transaction type to rollback'). A let _ = conn.query("ROLLBACK") in the commit-failure arm (or a comment stating drop semantics are relied upon) would make the contract explicit.

Failure scenario: COMMIT fails under resource pressure; if the Connection is ever held longer than this scope in a future refactor, the next statement on it fails with a confusing transaction-state error rather than the real cause.

[NIT] correctness — crates/infigraph-core/src/graph/kuzu_backend.rs:~490 (replace_concerns loop); same in neo4j replace_concerns

If a Concern's symbol_id doesn't exist, the Concern node is CREATEd but the follow-up MATCH...CREATE edge silently matches nothing, leaving an orphan Concern node with no HAS_CONCERN edge. This is pre-existing behavior carried over from the old write_concerns, so no change required — but since errors are no longer swallowed, this would be a natural moment to either skip or error on unknown symbols.

Failure scenario: A concern computed against a stale symbol id persists as an orphan node that raw Concern queries return but symbol-joined queries never surface.

[NIT] scope — crates/infigraph-core/src/embed/mod.rs:~401

The #[allow(clippy::chunks_exact_to_as_chunks)] in load_embeddings is an unrelated drive-by (presumably to keep floating-stable clippy green), and the scip/store_util fresh-connection changes are a second, distinct bug fix (connection wedging) bundled into an atomicity PR. Understandable given the stacked-PR setup, but worth a mention in the PR description or splitting; the store_util signature change is at least well documented.

Failure scenario: No runtime failure; review/bisect noise only.

Generated with Claude Code (Fable). Findings verified by an adversarial refute pass; false positives removed.