Skip to content

fix(mcp): apply search_graph label filter inside the BM25 candidate window (#2386) - #2423

Open
harshitaajoshi wants to merge 5 commits into
DeusData:mainfrom
harshitaajoshi:fix/2386-bm25-label-window
Open

harshitaajoshi wants to merge 5 commits into
DeusData:mainfrom
harshitaajoshi:fix/2386-bm25-label-window

Conversation

@harshitaajoshi

@harshitaajoshi harshitaajoshi commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2386

What does this PR do?

In query (BM25) mode, search_graph applied the label filter after the 2000 row FTS5 candidate window. When a token is common but the requested label is rare among its matches, those rows fell outside the window and the call returned 0 rows while reporting a saturated window.

This PR moves the label into candidate selection, as suggested in the issue:

  • When label is set, the MATCH expression becomes (<tokens>) AND label : "<Label>", so the window is filled with rows of that label. The outer n.label = ? check stays as the exact filter (FTS5 matching is case insensitive, so the column filter is only a superset pre-filter).
  • The ranked query, the count query and the saturation probe share the same MATCH string, so total, total_relation and candidate_window_saturated describe the label scoped window. A rare label no longer gets a false gte.
  • Only a plain identifier label ([A-Za-z0-9_], up to 64 chars) is spliced in, as a quoted FTS5 string. Anything else keeps the previous unscoped MATCH, so a label can never inject FTS5 syntax.

Ranking stays the same

The added label : "..." phrase is itself scored by bm25(), and its contribution depends on row length, so on its own it can reorder near ties. To avoid that, the label column weight in BM25_WEIGHTS is now a bound parameter: 1.0 as before, 0.0 when the MATCH is scoped. Every candidate carries the label in that case, so a label hit cannot distinguish them anyway.

I checked this on an index of this repo's src/ (6.9k nodes) by running the ranked SQL old vs new for 20 common tokens across every label with results (75 pairs):

Variant Same result set Same top 10 order
Label phrase, weight 1.0 75 / 75 73 / 75
Label phrase, weight 0.0 (this PR) 75 / 75 75 / 75

The one intended difference: when a query token is the label word itself (for example query=function label=Function), that token's label column hit no longer scores, since all candidates share it.

Tests

  • tool_search_graph_bm25_label_filter_selects_candidate_window: 2001 Functions and 3 lower ranked Classes share a token. label=Class must return all 3 with total:3, total_relation:eq and no saturation flag; label=Function must still report a saturated window; a label containing FTS5 syntax must stay in BM25 mode and return 0 rows. Fails on current main (0 Classes returned).
  • tool_search_graph_bm25_label_scope_keeps_rank_order: a label filtered query lists its rows in the same relative order as the unfiltered query. Fails if the label weight is left at 1.0.

Local run after rebasing on current main: full suite 9646 passed, 11 skipped. lint-ci passes apart from one cppcheck constParameterCallback note in src/daemon/application.c, which is already on main and not touched here.

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci), plus lint-tidy-diff clean on the changed lines
  • New behavior is covered by a test (reproduce-first for bug fixes)

Written with the help of Claude Code. I reviewed and ran everything above myself.

@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

daidaiJ pushed a commit to daidaiJ/codebase-memory-mcp that referenced this pull request Sep 29, 2026
…mics & retention, upstream watchlist

- docs/ROADMAP.md: community-facing overview of both phases (T0-T7), upstream
  issue/PR watchlist, and the explicit not-doing list
- .plan/spec.md: upstream issue sweep cross-checked against this base —
  T1 gains a prerequisite (BM25 label filter, issue DeusData#2386 / PR DeusData#2423, still
  reproducible at mcp.c:3882), T5 gains fail-loud COUNT semantics (DeusData#2012,
  still reproducible at store.c:3077), T2/T7 gain rebase watchlist entries
  and freshness-anchor constraints (DeusData#1714, DeusData#1213, DeusData#867)
- discussion DeusData#2025 recorded as the hardest adoption evidence for B4/B5
…indow (DeusData#2386)

Signed-off-by: harshitaajoshi <j.harshitaa06@gmail.com>
@harshitaajoshi
harshitaajoshi force-pushed the fix/2386-bm25-label-window branch from ba5e7dd to 19374f2 Compare October 13, 2026 19:29
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX labels Oct 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the BM25 label-window fix and the ranking comparison. I have read the exact-filter and saturation details; this remains pending review, including the documented label-word scoring difference.

Brings the branch up to current main (246 commits) before the zero-result follow-up.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…t zero

In query (BM25) mode an explicit label naming File, Folder, Variable or
Project returned 0 rows -- often with an exact "eq" total -- although those
nodes are in nodes_fts and match the query: the noise exclusion meant for
unlabelled queries was applied to labelled queries too. It now applies only
when no label is given (one shared SQL fragment for the ranked and the count
query), so such a label is served from its own label-scoped candidate window
like any other label. Unlabelled queries are unchanged.

A label no node carries as spelled (labels are case-sensitive) still answers
0 rows, as structural mode does, but no longer silently: the result carries a
hint naming the stored spelling, or get_graph_schema when the label is
unknown. An existing label with no matching row stays a plain zero.

Measured on Django and meilisearch indexes (81 / 114 label queries): zero
results 18 -> 0 and 21 -> 0 (8 and 18 of them reported an exact "eq" total);
every other label returns byte-identical results, and unlabelled queries are
byte-identical to main.

Refs DeusData#2386

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you @harshitaajoshi -- this is a careful fix, and it is exactly the approach #2386 asked for. Keeping the exact n.label = ? check so the MATCH is only a superset pre-filter, sharing one MATCH string between the ranked query, the count and the saturation probe, and neutralizing the scored label : "..." phrase with the bound weight (with a test that fails at 1.0) are the details that make it safe to merge.

While re-checking it on real indexes we found one more way a labelled query in BM25 mode came back empty. Your label-scoped window is what makes that fixable, so we pushed two small commits on top of your branch (your commit is untouched):

  1. A merge of current main. The branch was 246 commits behind; there were no conflicts, and this lets CI run against today's tree.

  2. fix(mcp): serve every named label in BM25 search_graph, never a silent zero

    • label=File, Folder, Variable and Project still returned 0 rows in query mode, often with total_relation: "eq", although those nodes are in nodes_fts and match the query. The cause was the noise exclusion meant for unlabelled queries (n.label NOT IN ('File','Folder','Variable','Project')), which also applied when the caller named one of these labels. It now applies only when no label is given (one shared SQL fragment for the ranked and the count query), so such a label gets its own label-scoped window like any other label. Unlabelled queries are unchanged.
    • A label that no node carries as spelled (labels are case-sensitive, e.g. label=class) still returns 0 rows, as structural search does, but no longer silently: the result carries a hint naming the stored spelling, or pointing to get_graph_schema when the label is unknown. An existing label with no matching row stays a plain zero.
    • Two tests in the mcp suite: one query per excluded label plus the unlabelled case, and one for the hint. Both fail on the branch without the change.

What we measured on Django, meilisearch and kubernetes indexes, comparing your head (merged with main) against the fix-up:

  • Labelled queries that returned 0 rows although matching nodes exist: 18 -> 0 (of 81 label queries), 21 -> 0 (of 114) and 21 -> 0 (of 130). The four labels now return 704, 581 and 716 rows across those queries.
  • Every other label: byte-identical results to your head. Unlabelled queries: byte-identical to main.
  • The graph itself is unchanged; this is a search-only change.

The same window effect also hits file_pattern, kind and namespace, which filter after the window and cannot go into the MATCH the way the label does. That is outside this PR's label scope, so we will handle it in a separate follow-up PR.

Thanks again for the reproduction test and the ranking comparison -- they made this review easy.

@harshitaajoshi

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the review and for the follow-up commits. Making the noise exclusion apply only to unlabelled queries makes sense, and the hint for a misspelled label is a nice touch.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for the BM25 label-window fix and the follow-up discussion. I merged current main into the branch in fc18b9da0 to pick up the shared CI fixes. The combined-tree check preserved the reviewed patch and its surrounding context, with only line positions moving. Fresh GitHub CI will validate this head; the separately agreed post-window filter follow-up remains separate.

@DeusData

Copy link
Copy Markdown
Owner

Thank you again for the BM25 label-window fix. Our October updates record the additional named-label handling and baseline refresh as completed, so no repeat contributor work is requested. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you again, @harshitaajoshi. We merged current main into the branch (one signed-off merge commit, 760cb9c; your commits are unchanged) so CI validates your fix against today's tree. The one red check on the previous run (the UI test job) failed while downloading a package during npm install, before any test ran, so it had nothing to do with your change. Nothing is needed from you; please pull before pushing again.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

search_graph BM25: label filter applies after the FTS5 candidate window

2 participants