fix(cypher): enforce variable-length path semantics

[jstar0]

What does this PR do?

Part of #797.

This fixes two variable-length Cypher path semantics and bounds the traversal expansion introduced by the trail check:

• repeated node variables now unify with an existing binding, so MATCH (f)-[:CALLS*1..2]->(f) only matches paths that return to the same node;
• variable-length traversal now rejects paths that reuse the same edge id, so a single self-loop cannot fabricate longer paths;
• cbm_store_bfs_trail caps rows admitted to its recursive CTE, so tracking edge ids cannot enumerate an unbounded number of simple paths before the outer LIMIT while shared BFS keeps its reachability behavior.

Summary

Variable-length Cypher paths now follow the same binding and simple-path semantics as the fixed-length executor cases covered by the existing tests, while shared BFS keeps its node-reachability behavior and Cypher trail expansion has a hard recursive-row bound.

Changes

• Enforce existing target-node bindings when expanding variable-length relationships.
• Track edge ids in recursive traversal and reject reusing the same edge within one path.
• Cap recursive CTE rows only inside cbm_store_bfs_trail.
• Add regressions for repeated node variables, self-loop edge reuse, uncapped shared BFS reachability, and the trail recursive row cap.

Verification

make -f Makefile.cbm test
make -f Makefile.cbm lint-ci

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects
  unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (make -f Makefile.cbm test)
• Lint passes (make -f Makefile.cbm lint-ci)
• New behavior is covered by a test (reproduce-first for bug fixes)

[DeusData]

Thanks for this, @jstar0 — the semantics here are exactly right. The node-variable unification (matching the fixed-length #627 behavior) and the no-repeated-relationship "trail" semantics are both correct, and the two regression tests are genuine guards: they fail on the old executor and pass only on the fix. Security, scope, and DCO all check out.

One blocking issue before we can merge, on the store.c change to cbm_store_bfs.

Pulling edge_path into the recursive CTE row changes the UNION dedup key from (node_id, hop) to (node_id, hop, edge_path). Because edge_path is distinct per path, UNION no longer collapses the many paths that reach a node — the CTE now enumerates every simple path up to max_depth instead of doing a bounded node-BFS. On a hub-heavy graph at depth 10 that's on the order of b^d intermediate rows, each carrying a growing TEXT path, and the outer ORDER BY bfs.hop LIMIT N can't rein it in (SQLite materializes the full CTE before it orders/limits).

Why this reaches beyond var-length Cypher: cbm_store_bfs is shared — it also backs the trace_call_path MCP tool, whose depth is client-controlled (and currently unclamped). So this turns a polynomial traversal into a potential exponential blow-up on a widely-used, untrusted-input path.

The PR notes the expensive-expansion guard is deferred — the catch is that this change is the amplifier that guard is meant to contain, so merging it now ships the blow-up ahead of its bound. Could you fold the bound into this same PR? A hard cap on the number of enumerated paths / CTE rows inside the recursion (or the deferred guard itself) would do it. Happy to think through the shape with you.

Once that's in, this is good to go — the correctness work is solid. Thanks again.

[DeusData]

Thanks for the focused #797 fix. Triage: high-priority Cypher/stability bug.

Review focus is exactly where the risk sits: variable binding semantics, edge-reuse prevention, and bounding recursive expansion without breaking valid variable-length path queries. The added regressions are the right shape; we will keep this tied to #797 while review finishes.

[DeusData]

Thanks — the two Cypher-level semantics fixes are right and well-tested: repeated-variable unification in expand_var_length is exactly how bindings should behave, and rejecting same-edge reuse matches Cypher's trail semantics. Security-wise the diff is clean (only integer interpolations added to the SQL; no new injection surface).

One structural concern blocks the merge as-is: the edge-path tracking lives in the shared cbm_store_bfs, which is also the engine for trace_path and the neighbor lookups (src/mcp/mcp.c:1580, :3055). Two consequences for those reachability consumers:

1. Adding edge_path to the CTE row changes UNION dedup from per-(node, hop) to per-path — on a dense call graph the recursion now enumerates simple paths instead of visiting nodes once, which is combinatorial. The new LIMIT 4096 contains the blow-up but does it silently: trace_path on a large project (our benchmark graphs run to millions of edges) can exhaust 4096 CTE rows in the first hops and return fewer reachable nodes than before, with no signal. The existing tests pass because the fixtures are small.
2. The edge_path string concat + instr scan adds per-row cost proportional to path length on the hottest traversal query we have.

Suggested shape: keep cbm_store_bfs reachability semantics untouched (per-node dedup, no edge_path), and apply the trail check only for the var-length Cypher expansion — e.g. a bool trail parameter (or a separate cbm_store_bfs_trail) that only expand_var_length sets, so trace_path/neighbors keep their current complexity and completeness. If the cap stays, please also surface truncation (even just a log/flag) rather than silently cutting — and a before/after sanity check of trace_path node counts on a large real repo would settle the regression question.

Happy to merge once the trail behavior is scoped to the Cypher path executor — the binding-unification part could even land on its own if you want to split it.

[jstar0]

Addressed the traversal review and rebased onto current main.

• Restored shared cbm_store_bfs to node-BFS semantics without the trail-row cap.
• Scoped relationship-trail enumeration to Cypher variable-length expansion via cbm_store_bfs_trail.
• Added regressions proving shared BFS returns all 4,200 reachable nodes and trail truncation emits a partial-result warning only when the budget is actually exceeded.
• Updated the #797 regression to include both valid length-two relationship-unique trails while still rejecting relationship reuse.

Local verification: 6008 passed, 1 skipped; make -f Makefile.cbm lint-ci passes.

[DeusData]

Reviewed in depth — we're adopting the trail semantics. Thank you for this; the repeated-node-var unification and self-loop edge-reuse rejection are genuine openCypher-correctness fixes that our current shortest-path var-length mode gets wrong, and I especially appreciate that you scoped it cleanly: the new cbm_store_bfs_trail is used only by the Cypher variable-length executor, so trace_path and the other cbm_store_bfs callers keep their shortest-path reachability behavior untouched, and the recursive-row cap bounds the enumeration. That containment is what made this an easy call.

One thing before merge: the PR is CONFLICTING against current main. #797 (shortest-path var-length + advertised depth clamp) landed since you opened this — it reworked the same cbm_store_bfs path. Could you rebase onto current main? The rebase should be mostly mechanical since your work is a separate function, but please make sure after the rebase that:

• cbm_store_bfs retains [bug] query_graph Cypher: var-length paths reuse edges and repeated node variables aren't unified — a single self-loop fabricates 100-hop paths; heavy expansion crashes server #797's MIN(hop) GROUP BY shortest-path finalization for its non-Cypher callers (trace_path etc.), and only the Cypher var-length site routes through cbm_store_bfs_trail;
• the [bug] query_graph Cypher: var-length paths reuse edges and repeated node variables aren't unified — a single self-loop fabricates 100-hop paths; heavy expansion crashes server #797 depth-clamp warning path still fires for var-length queries that exceed the ceiling.

Once it's rebased and green I'll re-verify the behavioral change end-to-end (result multiplicity on a multi-path fixture + the self-cycle case) and merge. Genuinely nice work on one of the subtler parts of the query engine.

[DeusData]

Reviewed in depth. There is a genuinely good fix in here that I would like to take, and a much larger change riding alongside it that is a maintainer decision rather than a bug fix. Let me separate them clearly, because I think the framing matters more than the verdict.

First, you are right about openCypher. With relationship-uniqueness-only semantics, *2..2 from the #797 fixture really does yield {mid, leaf}, and main's leaf-only assertion is not spec-conformant. I want to say that plainly before anything else, because it would be easy to read the rest of this as "you misread the spec". You did not.

But main's behaviour is a deliberate divergence, not an oversight. Commit 73a3c34 (11 July, closing #797) chose shortest-path semantics on purpose, and its message says why: "deliberately shortest-path semantics, not openCypher trail enumeration — right for reachability/depth audits on call graphs and linear instead of exponential." Your PR opened on 5 July, before that landed, and was rebased over it on the 18th — so it now functions as a reversal of a closed decision, including rewriting that issue's regression test from 1 row to 2. The description does not mention any of that, which I suspect is simply because the rebase happened mechanically rather than because anything was hidden.

That is a maintainer call, so I have taken it upstream rather than deciding it in review.

The separable fix I do want. Your repeated-variable unification in expand_var_length — filtering hops to an already-bound target instead of overwriting the binding — is a real gap and the right fix. It brings variable-length in line with the #627 behaviour that process_edges already has for fixed-length, and your test is properly binding (red on main). Would you split that into its own PR? It stands entirely on its own merits and I would review it quickly. If you would rather not, we can distill it with Co-Authored-By credit to you.

The blast-radius care in this PR deserves credit too. Keeping the shared BFS, trace_path, and impact paths on the linear SQL, verifying it byte-equivalent, and adding a 4200-leaf pin test proving reachability stays uncapped — that is exactly the right instinct when forking a core query path, and it made this review far easier than it could have been.

If the trail direction is taken, two things would be required first, and they are the two properties this codebase most deliberately protects:

1. The row cap must not be silent. The trail CTE caps at 801 rows for the Cypher path, and that cap counts trails — a combinatorial quantity — not nodes, which is linear. So a chain of ten diamond segments produces 2¹⁰ = 1024 trails and *20..20 returns zero rows where main returns the end node; a hub with fan-out above 801 exhausts the budget at hop 1 and *2..2 through it returns zero where main returned up to 100. Parallel routes between layers are ubiquitous in real call graphs. Right now truncation is a log line (cypher.trail_truncated) and never reaches result->warning — so through the MCP surface an agent receives a confidently wrong partial answer. The warning channel already exists; 73a3c34 built it, and cbm_store_bfs_multi documents "never a silent cap".
2. The total order needs restoring. The trail SQL orders by bfs.hop alone, dropping the n.id tiebreak that the non-trail branch documents as required for deterministic pagination and reproducible trace output. Which 100 of many rows survive becomes unspecified.

Two smaller notes: per-(node,hop) rows change non-DISTINCT results and count() in a way that matches neither the old semantics nor openCypher's per-path counts; and cypher_exec_var_length_no_reuse_self_loop already passes on current main, so it is a pin for the new engine rather than a reproduction — worth labelling as such.

To be concrete about what happens next: the unification fix I would like as its own PR now. The semantics question is with the maintainer and I will come back to you with a real answer either way.