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 now caps rows admitted to the recursive CTE, so tracking edge ids cannot enumerate an unbounded number of simple paths before the outer LIMIT.

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 traversal keeps a hard bound on recursive row expansion.

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 inside cbm_store_bfs.
• Add regressions for repeated node variables, self-loop edge reuse, and the BFS 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.