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.

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.

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.

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.

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.

Direction call resolved, and it goes your way: we're adopting trail semantics. Thank you for the patience while that sat — and my apologies for the previous review, which asked you to split out just the repeated-variable fix and take the rest to a maintainer decision. That decision has now been made, so please disregard the split request; the larger change is wanted.

The reasoning, since you argued the openCypher case and deserve to know it landed: the earlier choice to keep variable-length traversal on shortest-path semantics (closing #797) was made for scale on call graphs, not because relationship-uniqueness was thought wrong. Your point stands that a query which reuses an edge to manufacture a longer path is returning something that isn't a path. Correct-by-default wins, with the blowup managed rather than avoided by approximation.

Your isolation work is what makes that affordable: keeping cbm_store_bfs untouched for trace_path/MCP reachability and putting trail tracking in a separate cbm_store_bfs_trail means the expensive semantics only apply where Cypher asks for them. That was the right instinct and it's why this is mergeable at all.

Three gaps to close before it lands. Each is small, and each is a case where the current version could be quietly wrong rather than loudly wrong:

1. The row cap counts trails, not nodes. ST_BFS_MAX_CTE_ROWS = 4096 bounds rows in the recursive CTE, but trails are combinatorial in a hub-heavy graph — a single well-connected node can burn the entire budget before the traversal reaches anything interesting. The failure mode is that a query returns zero rows where main returns real matches, with nothing to indicate truncation happened. A cap on distinct nodes visited (or a depth-aware budget) degrades toward "fewer results" instead of "no results".

2. The truncation warning is logged but never surfaced. cypher.trail_truncated goes to the log; it never reaches the result's warning field, so an API caller sees a short answer and no reason for it. Silent truncation in a query engine is the kind of thing people build wrong conclusions on top of. Please plumb it through to the response.

3. The trail branch dropped the n.id tiebreak in its ORDER BY. The non-trail branch documents that tiebreak as required for deterministic pagination, and this project treats a test whose result depends on row order as broken by construction — so the trail path needs it too.

One small honesty note on the tests, and it's a compliment rather than a complaint: cypher_exec_var_length_no_reuse_self_loop already passes on main, so it pins the new engine rather than reproducing the bug. Worth a comment saying so, so a future reader doesn't mistake it for the regression test. cypher_exec_variable_length_repeated_node_var_unifies and the updated ..._issue797 (1 → 2 rows) are the genuinely binding pair.

You'll also need a rebase — main has moved and the branch conflicts, including in the function you refactored.

If you'd rather hand it off at this point, say so and I'll take the three fixes on with you credited as co-author; you've already done the hard part and the design thinking. But you've reworked this once already and done it well, so it's yours first if you want it.

Rebased onto current upstream main at dfe67cc and pushed signed head b3d31ca.

Addressed the three requested trail-budget issues:

• Retained the 4096 recursive-row hard ceiling and added a depth-aware budget; the recursive queue prioritizes deeper hops so bounded truncation yields partial deep results instead of consuming the budget in a shallow layer.
• Propagated traversal truncation through the traversal result into the Cypher/MCP warning field.
• Restored deterministic trail ordering by hop, then node id.

Verification on the exact head:

• make -f Makefile.cbm test-focused TEST_SUITES="cypher store_search": cypher 251 passed; store_search 68 passed with ASan/UBSan.
• All commits are SSH-signed and carry the required DCO sign-off.

The exact-head workflow 32382650055 has completed. All code-relevant checks are green, including CodeQL, security/static, lint, package wrappers, Linux/macOS/Windows smoke, ASan/LSan/MSan/TSan, diagnostic and Unix/Windows test shards.

The only failure is the Windows guard tests/windows/test_daemon_stability.py: daemon start --port=<occupied> reports that the daemon did not accept the UI configuration and exits red. This PR changes only Cypher/store traversal code and does not touch the daemon/UI path. I have not confirmed the same guard on an independent base-branch run, so I am describing it as unrelated to this diff rather than calling it a baseline failure.

I attempted to rerun the failed job, but GitHub rejected the request because the contributor lacks repository admin rights. Could a maintainer rerun test-windows-guards or confirm whether this is a base-branch/runner issue?

Rerun kicked off — and you were right not to call it a baseline failure without proof, so let me give you the proof.

tests/windows/test_daemon_stability.py is a known Windows-only flake and it has nothing to do with your diff. I root-caused it earlier today, on a different thread, so here is the actual mechanism rather than a shrug:

The harness kills daemons with taskkill /F. On Windows a hard kill leaves the process's byte-range locks held until the OS reclaims them, and the documented behaviour is that this happens "as resources allow" — not promptly, and not deterministically. The next client to start hits ERROR_LOCK_VIOLATION, reads it as BUSY, and gets refused. That is why the symptom is Windows-only, why a different subsection fails each run, why the cascade shows up as skips, and why a three-line README PR has been seen failing this same job. #1772 is the fix in flight: it waits out a held startup transition instead of giving up at a 10-second deadline.

So: unrelated to Cypher, unrelated to your traversal changes, and not something you could have fixed. Thank you for flagging it precisely instead of hand-waving it.

On the three gaps — (b) and (c) are closed, and I verified both rather than taking the summary on trust.

(c) ordering — ORDER BY bfs.hop, n.id is back on the trail branch's outer query. Done.

(b) truncation reaching the API — the whole chain is there: truncated on the store result, set where cte_rows > cte_row_limit, picked up into the thread-local in expand_var_length, and the warning condition widened from depth-clamp-only to g_cypher_depth_clamped > 0 || g_cypher_trail_truncated, with distinct messages for clamped / truncated / both. store_bfs_trail_warns_when_path_rows_are_truncated pins it through the log sink. That is exactly the shape I wanted.

(a) is where I want to keep talking, and I want to be fair about it first. You took the depth-aware budget, which was the parenthetical alternative I offered — my words were "a cap on distinct nodes visited (or a depth-aware budget)" — so this is a legitimate reading of the ask, not a dodge, and you said plainly in your comment that that is what you did. bfs_cte_row_limit_for_depth scaling 801 by depth up to the 4096 ceiling, plus ORDER BY 2 DESC to stop a wide shallow layer eating the budget before the recursion reaches depth, is a genuinely clever answer to the problem I described. And cypher_exec_var_length_truncation_surfaces_warning asserting row_count > 0 is precisely the "degrades to fewer results, not zero" guarantee I asked for.

Two things I would still like closed, one of which I think matters more than the original point:

1. The zero-rows case may survive at hub scale. The counting unit is still CTE rows, so the budget is consumed by trails. ORDER BY 2 DESC prioritizes depth, but SQLite still has to generate hop-1 rows before it can extend any of them — so a single hub whose hop-1 fan-out alone exceeds the budget (>1602 at depth 2) can still exhaust the CTE before a hop-2 row exists. Your 18-node dense fixture is well below that. I would like either a test at hub scale, or an argument for why the queue ordering makes it unreachable — I genuinely am not certain which, and I would rather you tell me than I guess.

2. Truncation is currently nondeterministic, and I think this is the more important one. Your comment on the query says the outer ordering stays deterministic, and it does. But ORDER BY 2 DESC gives no tie-break among equal-hop rows, so which trails survive the LIMIT is unspecified — and since SELECT DISTINCT then projects those trails down to nodes, two runs over an identical graph can return different node sets once truncation kicks in. This project treats a test's verdict as a pure function of (code, test, platform, seed); a query whose truncated output varies between runs on identical input cuts against that badly, and it would be very unpleasant to debug in the field. Adding n.id — or the edge path — as a secondary key in the recursive ORDER BY should pin it for free.

One small leftover: cypher_exec_var_length_no_reuse_self_loop still has no comment marking it as a pin for the new engine rather than a reproduction. It already passes on main, and a future reader will otherwise assume it once failed. One line is enough.

Fix those and I will merge. Thank you for staying with this through a direction reversal that was my fault, not yours — the semantics call you argued for from the start is the one we ended up at.

Rerun came back green — passed on the same head with no code change, which confirms the flake reading. Your CI is clean; nothing there for you to chase.

That leaves only the four review items from my last comment, of which the truncation-determinism one is the one I care most about.

Closing the loop: the Windows startup-coordination fix has merged as 7b6363d0 (#1772). That is the fix for the byte-range-lock flake you hit — the daemon now waits out a held startup transition instead of giving up at a fixed deadline.

Notably it merged with test / test-windows-guards green, which is the job that had been flaking. Future runs on your branch should stop showing it once you pick up current main. Thank you for reporting it precisely rather than just re-running.

Addressed the remaining review items in signed-off head 7583376fd03f0731d07c75ca7f028eba1ff2e18d:

• Recursive trail queue ordering now uses hop DESC, node id ASC, edge path ASC, so truncation selection is deterministic even when multiple paths share a hop.
• Added a high-fanout hub regression with 4,100 direct leaves plus a two-hop branch; the bounded traversal reports truncation while still surfacing the deeper target instead of exhausting before any deeper row is generated.
• Added the requested comment documenting the no-reuse self-loop regression contract.
• Added the repository-required DCO sign-off to the review-fix commit.

Focused verification after the change:

• make -f Makefile.cbm test-focused TEST_SUITES="store_search cypher" build succeeded
• focused runner: 69 store_search tests and 183 cypher tests passed (252 total)
• git diff --check origin/main...HEAD passed

The hosted matrix is rerunning on the new head.

The exact head 7583376fd03f0731d07c75ca7f028eba1ff2e18d is ready for re-review.

• The trail traversal remains isolated from shared reachability BFS, preserving the non-Cypher callers' behavior.
• The trail budget is depth-aware, truncation is propagated into the Cypher/MCP warning surface, and result ordering is deterministic by hop then node id.
• The exact-head hosted matrix is green: 34 checks pass with 0 failures and 0 pending jobs.
• Focused local verification remains green: the Cypher suite reports 251 passed and the store-search suite reports 68 passed with ASan/UBSan.

Please re-review the current head.