Skip to content

fix(mcp): isolate semantic-only JSON search - #1319

Open
JJordan0C wants to merge 4 commits into
DeusData:mainfrom
JJordan0C:fix/search-graph-semantic-only-results
Open

fix(mcp): isolate semantic-only JSON search#1319
JJordan0C wants to merge 4 commits into
DeusData:mainfrom
JJordan0C:fix/search-graph-semantic-only-results

Conversation

@JJordan0C

Copy link
Copy Markdown

What does this PR do?

Fixes #1295.

search_graph already skipped the structural query for semantic-only calls in
the default response format, but format:"json" still ran an unfiltered
structural search. On a 21,221-node index that made the JSON response report all
21,221 nodes and include unrelated groups alongside the semantic matches.

This change shares the structural-filter detection between both output paths
and skips cbm_store_search for semantic-only JSON calls. Mixed
semantic/structural searches keep their existing behavior.

The regression test covers both output formats. The end-to-end reproduction now
returns total: 0, count: 0, and no structural groups while retaining
semantic rows.

Validation:

  • Full test runner: 6,792 passed, 4 platform-only skips
  • Full ASan/UBSan test runner: 6,792 passed, 4 platform-only skips
  • CI lint gate: cppcheck 2.20.0, clang-format 20, NOLINT policy
  • Production binary reproduction against the repository's full index

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)

The JSON search_graph path always ran the structural search, even when semantic_query was the only filter. Reuse the shared structural-filter check so semantic-only JSON calls retain semantic hits without returning unrelated graph nodes.\n\nAdd regression coverage for both default and JSON output.

Signed-off-by: JJordan0K <69581081+JJordan0C@users.noreply.github.com>
@JJordan0C
JJordan0C requested a review from DeusData as a code owner July 28, 2026 16:44
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 28, 2026
@DeusData DeusData added bug Something isn't working duplicate This issue or pull request already exists parsing/quality Graph extraction bugs, false positives, missing edges editor/integration Editor compatibility and CLI integration priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 28, 2026
@DeusData

DeusData commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the contribution and for covering the semantic-only JSON path with end-to-end regression tests. This is now triaged as a high-priority MCP output correctness bug for 0.9.1-rc. Current main already contains an overlapping fix and regression coverage for #1295, so review will determine whether this branch still contains any distinct work. Our community PR queue is currently quite full, so it may take a little time before we can complete that review. We are doing our best to support community contributions and will return with code-grounded feedback as capacity opens.

@DeusData

Copy link
Copy Markdown
Owner

Reviewed in full. The fix is sound, the three red checks are not your fault, and you were right that #1295 was not fully fixed. Sorry this sat — and sorry that our issue close implied the work was already done.

The catch is a good one. 0e24f84 fixed semantic-only isolation for the default tree output but never touched the legacy_json branch, so on today's main the format:"json" path still runs cbm_store_search unconditionally at src/mcp/mcp.c:3555. With no structural filters that matches the whole graph — which is exactly the total: 21143 dump in #1295, first entries coming from .github/issue-labeler.yml. Your shared has_structural_filters hoist keeps the two paths from drifting apart again, which is the right shape rather than patching one branch.

Good instinct running run_semantic_query_core before the document is built, too: the type-error path now returns before any allocation instead of discarding a completed structural search, and since the core sets *out_vresults=NULL / *out_vcount=0 up front and only transfers on success, nothing leaks on that path. I also checked the skipped-search case — out = {0} is safe for cbm_store_search_free (count-0 loop, free(NULL)), and the emitter produces total:0 / count:0 / groups:[].

The JSON half of your regression test is properly binding: on current main it fails with total:1 and unrelated_node present. Pinning the already-shipped default-format contract alongside it is welcome rather than redundant.

On the three failing checks — they are ours, not yours.

Your run tested a merge against main as of 28 July 17:34, which was mid-way through the Windows single-binary transition. Both failures come from that window and were fixed on main the same evening:

  • pr-smoke (windows-latest) failed at FAIL: invalid Windows PATH smoke seam fell back to the live registry (scripts/smoke-test.sh:923). Fixed by 2740208, whose commit message describes that exact guard — it restores a non-zero exit status for a failed install --dry-run plan check.
  • test / test-windows-guards failed on section_cold_storm / "cold-storm client 3 failed (racing daemon spawn)". Fixed by 5f2c146 (pinning Windows daemon startup interleaving instead of polling, plus a use-after-free it exposed) and 0a22fb1.
  • ci-ok is just the aggregator of those two.

That guard has been green on several fully-green runs against current main since. And a diff confined to handle_search_graph plus a unit test has no causal path to daemon spawn or CLI install code.

What we need from you: rebase onto current main (or git merge origin/main) and push. Your branch is roughly 75 commits behind. One thing worth knowing — a plain "Re-run failed jobs" will not help here: it re-tests the same stale merge commit and will fail identically. The push is what refreshes the merge ref.

Once that run is green this is a merge candidate.

One cosmetic note, no action needed: your DCO sign-off name reads JJordan0K while your login is JJordan0C. The noreply email matches your account and the dco gate passed, so it is fine — just flagging it in case it was a typo you would rather fix.

Thank you for pushing on this after the issue was closed. You were right to.

@DeusData

Copy link
Copy Markdown
Owner

Correction to my earlier comment: I cited the wrong commit SHA.

I said the default-path semantic-only isolation landed in 0e24f84. That is wrong. 0e24f84 is not on main at all — it lives on a local review branch and is itself an earlier rebase of PR #938, which carries the same title. Citing it as a merged fix was my error.

The isolation that is on main came from 4843a34 (the TOON compact-output work), and it is live today at src/mcp/mcp.c:3540bool semantic_only = sq_present && !has_filters; with the guard just below it.

Nothing about the substance of my review changes. The default (tree) path is isolated on main; the format:"json" path still runs cbm_store_search unconditionally, so the #1295 residual is real and this PR is the fix for it. The rebase request and the warning about "Re-run failed jobs" re-testing the same stale merge commit both still stand.

Apologies for the noise — a wrong SHA in a review is the kind of thing that wastes someone's time when they go looking for it.

…ntic-only-results

Signed-off-by: JJordan0K <69581081+JJordan0C@users.noreply.github.com>
…ntic-only-results

Signed-off-by: JJordan0K <69581081+JJordan0C@users.noreply.github.com>
@JJordan0C

Copy link
Copy Markdown
Author

Hi @DeusData, the branch has been updated with the latest main, and the refreshed CI is now fully green. Thanks again for the detailed review — this should be ready for another look.

@DeusData DeusData removed the duplicate This issue or pull request already exists label Aug 3, 2026
@DeusData

DeusData commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Thank you for rebasing this and getting the checks green. The remaining semantic-only JSON search case is distinct from the older broad-search report, so I removed the incorrect duplicate label and kept this at high priority in 0.9.1-rc. It is back in the maintainer review queue, which is currently full, so the final re-review may take a little time.

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

Labels

bug Something isn't working editor/integration Editor compatibility and CLI integration parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

search_graph: when only semantic_query is given, the results field returns the entire graph unfiltered

2 participants