fix(mcp): add stable semantic pagination

[dpersek]

What does this PR do?

Adds the focused follow-up agreed in #938 for semantic-query pagination and deterministic ordering:

• ranks a fixed 250-candidate semantic window by score descending, then node ID ascending, before applying offset/limit
• reports namespaced semantic total, has_more, and truncated metadata in tree and JSON output
• preserves semantic-only isolation and combined structural/BM25 behavior
• keeps all scores eligible; this PR adds no score threshold
• returns honest empty pages and fails closed on invalid semantic arrays or vector-store errors

The bounded window is explicit: truncated=true means 250 candidates filled the window, not that the response represents the full corpus.

Fixes #915.

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)

Local focused proof passes (209 passed, 6 skipped). The full local suite is baseline-limited by five CLI assertions that reproduce unchanged on pristine main; two later daemon timing failures passed an immediate focused 57-test rerun.

[DeusData]

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.

[DeusData]

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.

[dpersek]

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

[DeusData]

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.

[DeusData]

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.

[dpersek]

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.

[DeusData]

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.

[DeusData]

@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.

[dpersek]

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!

[dpersek]

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.

[DeusData]

Thank you for this one, and for sticking with it across three rounds — the rework you pushed on 08-28 does exactly what we agreed on 08-20, and it passes its suite (I built be9faf9c and ran mcp: 209 passed, 0 failed).

I owe you an honest account of why it is being closed anyway. On 08-13, a week before I asked you for the fixed 250-result window, a lossless design for the same problem was already being built on our lean-output branch (#1597): a certified top-K prefix with dedicated semantic_limit / semantic_offset, ORDER BY score DESC, id ASC, and query + semantic_query rejected as mutually exclusive. That branch merged today as 3c7427ef, and it rewrote every function this PR touches, so nothing here merges mechanically and its window semantics contradict what shipped. That is on us, not on you — you built the spec you were given.

Four things in your diff are still better than what main has, and they are being landed on main's contract with you as co-author:

• extract_semantic_keywords rejecting non-string elements (main silently drops them)
• failing closed on a vector-store error instead of rendering it as "0 matches"
• hoisting the keyword/node norms out of the per-pair loop in vs_min_cosine_score
• the strdup NULL checks in vs_append_result

I'll link the PR that carries them here. If you would rather open that one yourself, say so and it is yours — the offer stands either way, and the original fix for #915 already went out in de4ec9b5 (#1319).

[DeusData]

The four residuals are up as #2067 with you as co-author: non-string keyword elements are rejected, a store error during the vector scan now fails closed as "semantic search failed" (with a sqlite_master probe so a not-yet-built vector index still reads as empty + hint, as main intended), the keyword/node norms are hoisted out of the pair loop, and the strdup results are checked. It merges on green. Thank you again.

[DeusData]

Landed on main as 7b0f553 (#2067). Thanks again for the groundwork here.