Thanks for the semantic_query fix for #915. Triage: high-priority query correctness bug.

Review should verify semantic-only calls put vector-ranked hits in the primary results array, combined structural+semantic behavior stays compatible, and total/has_more report the semantic candidate set rather than the broad structural fallback.

The shape of this is right: one semantic-query collection feeding either the primary results (semantic-only) or the legacy semantic_results sidecar (combined calls), correct total/has_more/offset pagination on the vector path, tool description updated, and the non-array validation error preserved. 19/19 green.

One change requested: the refactor quietly switched yyjson_mut_obj_add_strcpy → yyjson_mut_obj_add_str in emit_vector_results. add_str borrows the pointer, so correctness now depends on yy_doc_to_str() always running before semantic_query_output_free(&sem) — which both call sites currently honor, but nothing enforces: any future reorder (or an early-return path added between emit and serialize) becomes a use-after-free in a public MCP handler. Please either restore add_strcpy (the copy cost is trivial at ≤ limit results and buys memory-safety by construction), or if you keep the borrow, add a loud comment at emit_vector_results + both call sites pinning the serialize-before-free invariant. strcpy is my preference.

With that addressed this is merge-ready.

Implemented your preference and restored yyjson_mut_obj_add_strcpy for all four vector-result strings.

Thanks @dpersek — reviewed in depth. The core fix is right and we want it: a semantic-only search_graph call (semantic query, no structural filters) should return the semantic hits as the primary results, not an unfiltered structural dump — that's the #915 bug, and aligning the JSON path with the TOON path is correct. The stable qsort+node_id pagination is a good call too.

Two changes before we can merge, both about not shipping a scale/contract foot-gun:

1. Bound the semantic-only fetch. Right now the paginated semantic-only path sets fetch_limit = candidate_total, so it re-scores the entire Function/Method/Class vector set and qsorts all of it on every call (unbounded by limit). On a large graph that's a heavy per-query cost on the hot path. Please keep a bounded page window (e.g. the limit*5 prefilter the combined path still uses, or a hard cap) rather than a full-corpus rescan.
2. Make total meaningful. total currently counts the whole corpus (no score threshold), so has_more is ~always true and a client paginates through the near-zero-cosine tail. Please make total/has_more reflect a thresholded match set (like bm25/regex total counts actual matches).

Also please rebase onto current main (the base has moved) and re-run so we can re-review at the new SHA. The output-shape change itself (results→vector, drop semantic_results sidecar for semantic-only, add search_mode/total/has_more) is accepted. Appreciate this — it's a real fix.

Reviewed properly at last — apologies this took so long, because the delay is most of the problem here.

Your diagnosis of #915 was exactly right, and it is worth saying plainly: the unfiltered structural search being prepended, letting vector-less nodes win the ranking alphabetically, is precisely the bug. Main's default-path isolation follows the direction this PR established, and it is live today at src/mcp/mcp.c:3540. Your test coverage — pagination edges, INT_MAX offset, the combined-call sidecar staying untouched — is genuinely thorough.

What happened is that main moved out from under you, twice. Since your last rebase in mid-July, TOON was pruned in favour of the tree format, and then format:"json" was unified onto that same tree model. This PR's JSON half builds a bespoke legacy results object — a second JSON dialect that main deliberately deleted. And the #1295 residual it also addresses is now being fixed by #1319 within the current architecture, in about 30 lines. So this branch needs a ground-up rework rather than a rebase, and that is our review latency showing, not a deficiency in your work.

Two pieces here are still genuinely valuable and are not on main:

1. Honest pagination for semantic results — total, offset/limit, and has_more. Today semantic-only output has no offset support and no truncation signal at all, so hits beyond limit are silently invisible. That is itself a findability gap, and closing it is a win.
2. Deterministic ordering — your qsort with a node-id tie-break, replacing the O(n²) re-sort, gives page-stable results. Also worth having on its own.

Both would make a strong focused follow-up against current main.

Two things I would want split out and decided explicitly, because they cut results rather than reorder them:

• VS_MIN_MATCH_SCORE (0.10). PRIMARY searches would silently drop every candidate below that cosine score. For a weak-signal query, the sub-threshold hits are the answer — so a query that returns something today could return nothing. The threshold is not mentioned in the PR body prose, only in the schema text.
• CBM_VECTOR_SEARCH_CANDIDATE_CAP (250). Pagination can never reach past that window. The metadata stays honest about it, but deep hits become unreachable. Fair mitigation: main already fetches limit × 5, so at the default limit of 50 the window is the same 250 — it only bites above that.

Neither is wrong; both trade recall for precision, and that is a product call we want to make deliberately rather than inherit. Splitting them out lets each be judged on its own.

Also riding along undeclared: a store-API signature change (cbm_vector_search_policy_t plus a total_count out-param — permanent surface for what is presentation-layer paging), and has_filters semantics changes for include_connected and empty-string labels.

Suggested path: let #1319 take the #1295 JSON residual, and open a focused PR against current main with the pagination and deterministic ordering. If you would rather not carry that yourself, we can distill those two pieces with Co-Authored-By credit to you — your call, and I would rather ask than assume.

One correction I owe you for accuracy: I initially attributed main's isolation to a different commit while reviewing this. The correct one is 4843a34. Mentioning it because a wrong SHA sends people looking in the wrong place.

Thanks for the thorough review and for spelling out how main moved.

I’d like to carry the focused follow-up. I’ll base it on current main, leave #1319’s isolation work alone, and limit it to deterministic ordering plus pagination metadata.

One implementation question before I start: should pagination be defined within the existing bounded limit × 5 candidate window, or would you prefer a fixed candidate window to be decided explicitly as part of the pagination PR? I’ll leave the score threshold out.

Sorry for the wait on this — you asked a good question and then heard nothing.

Short answer: a fixed candidate window, not limit × 5. And that isn't a preference, it's the only one of the two that can support pagination correctly.

The problem with limit × 5 is that the candidate pool becomes a function of the page size, so the thing you are paginating through changes shape depending on how you ask for it. Concretely:

• limit=10, offset=0 searches a 50-candidate pool; limit=50, offset=0 searches a 250-candidate pool. Those are different result sets, so the same query returns different top-10 items depending on a parameter that is supposed to control how many you see, not what exists.
• Paging past the window silently ends. limit=10, offset=60 needs 60+ candidates but the window is 50, so you get an empty page that is indistinguishable from "no more matches" — the exact class of silent-truncation bug the current has_more already suffers from.

A fixed window gives you one stable candidate set per query, which makes offset/has_more mean what a caller expects and makes page N independent of the limit used to get there. It also matches the doctrine already written into the BM25 path in mcp.c, where the n.id tie-break exists specifically so that "offset pages [are] contractually stable across calls" — semantic search should not have weaker guarantees than text search.

So: fetch a fixed number of ranked candidates, sort deterministically (score DESC, id ASC as the tie-break — please keep that, it's the part that makes the whole thing reproducible), then slice by offset/limit, and set has_more from whether the slice reached the end of the window.

On the window size itself — you're right to treat it as a decision rather than an accident, and I'd rather it were explicit in the PR than inherited. My suggestion is to state it as a named constant with a comment saying what it costs and what it buys, and to document plainly in the tool's response semantics that results are ranked within a bounded window rather than the full corpus. 250 (what your earlier branch used) is a reasonable starting point; if you have a view from having profiled the vector scan, I'd weight that over my guess. What matters most is that the number is visible and justified in one place, not that it is any particular value.

And please make the truncation legible. If a query fills the window, the response should say so rather than just setting has_more. A caller that cannot tell "these are the best matches" from "these are the best matches we looked at" will eventually build something wrong on the difference.

Leaving the score threshold out of this PR is the right call — agreed, and thank you for splitting it.

Ping me when the PR is up and I'll review it promptly this time rather than letting it sit. Your original diagnosis on #915 was correct and it deserved a faster turnaround than it got.

@dpersek — a ping and a status change, with thanks attached.

Converted to draft: until the focused follow-up is ready, this is honestly a draft — the headline fix you drove already shipped on main (4843a34 and #1319, 2026-08-17), so what remains open here is exactly the replacement we agreed on 1 August: deterministic ordering plus fixed-window pagination, score threshold explicitly out. The 20 August terms stand — build it against current main and ping when it is up; the re-review will be prompt.

And the thanks, because the record deserves it: you diagnosed #915 correctly, pushed the design conversation until it converged, and then — when main moved twice under your branch — chose the harder, better path of a ground-up rework over a patched rebase, declining the offered distillation to keep the work yours. The shipped fix exists because you kept at it. The draft state means "awaiting the agreed follow-up", nothing less friendly than that.

Oh my bad! I remediated this last week right after your comment and committed it locally, got sidetracked and forgot to push it. Will do a quick re-review and get it updated!

Updated this draft with the ground-up current-main follow-up we agreed on: fixed 250-candidate pagination, deterministic score/id ordering, explicit truncation metadata, and no score threshold. The branch was independently reviewed and the focused MCP suite is green. Ready for your re-review when convenient.