Skip to content

fix(mcp): restore scalable project discovery - #1181

Open
tmonestudio wants to merge 1 commit into
DeusData:mainfrom
tmonestudio:codex/fix-list-projects-coverage-scope
Open

fix(mcp): restore scalable project discovery#1181
tmonestudio wants to merge 1 commit into
DeusData:mainfrom
tmonestudio:codex/fix-list-projects-coverage-scope

Conversation

@tmonestudio

@tmonestudio tmonestudio commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Restores scalable and deterministic list_projects discovery:

  • sorts database names and paginates before opening project databases;
  • makes expensive node/edge/Git details opt-in through include_details;
  • preserves the existing metadata_only=true compatibility path;
  • skips qsort for an empty cache;
  • fails explicitly on realloc/strdup allocation failure instead of returning a misleading truncated success response.

The previously included check_index_coverage scope-copy fix has been removed from this branch because it already shipped on current main in f18ed73 via #1148.

Reproduce-first evidence

  • Production cache (632 databases): old main exceeded 60 seconds for limit=50, include_details=false; the paginated implementation returned 50/632 in 3.54 seconds.
  • Tests cover deterministic first/second pages, metadata_only compatibility, and explicit include_details.

Checklist

  • Rebased on current main
  • Coverage-scope duplicate removed
  • Empty-cache sort guarded
  • Allocation failures return an explicit MCP error
  • Every commit is signed off (git commit -s)

@tmonestudio
tmonestudio requested a review from DeusData as a code owner July 19, 2026 23:21
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 22, 2026
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 22, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks, and sorry for the wait. Queued for review.

Mechanically in good shape: MERGEABLE with 28 checks green, +95/-12 across two files.

Project discovery scalability has come up from a few directions recently, so a targeted fix there is welcome.

@DeusData

Copy link
Copy Markdown
Owner

An update on this one, and an apology for how long it has sat.

Half of your PR has just shipped — and you found the bug independently, which deserves saying plainly.

The yyjson_mut_obj_add_stradd_strcpy fix for the coverage scope field landed on main tonight in f18ed73 (via #1148). Your diagnosis and @aaiyer's were the same and both correct: scope is declared inside the yyjson_arr_foreach body and was being stored by pointer, so by serialization time every entry aliased one dead, reused stack slot — multi-scope check_index_coverage responses returned the last label for all entries, on top of stack-use-after-scope UB. Two people finding the same non-obvious bug from different directions is a good signal it was worth fixing, and you were first to report it here.

The practical consequence is that main now contains that exact line, so this branch will conflict on it.

What is still live in this PR: the list_projects pagination. That part has not shipped and is not duplicated. It is with the maintainer, because it changes the default contract of a public MCP tool — today list_projects returns every project with nodes/edges/size/branch, and this makes the default 50-capped and detail-less with the rest behind include_details. metadata_only=true stays compatible, but the old default does not, so existing consumers would silently lose fields. That is a findability-versus-cost decision on a public surface and it is genuinely his call, not mine. The underlying problem you are solving is real — opening 632 project databases to answer one list request is not defensible.

Two defects worth fixing regardless of which way that decision goes:

  1. qsort(db_names, 0, …) is called with db_names == NULL when the cache holds no .db files. Passing a null pointer to qsort is undefined behaviour even with a zero count — this is the exact class we just fixed elsewhere in the tree (pass_similarity calls qsort with a null base for an empty fingerprint set #1367/fix(similarity): skip empty fingerprint sort #1368), so it is worth guarding.
  2. A realloc/strdup failure currently breaks out of the loop, which produces a truncated list alongside a total/has_more computed for the full set. The caller cannot tell a short answer from a complete one, which is worse than failing outright.

What would help most right now: rebase on current main and drop the add_strcpy hunk (it is already there), leaving this PR as purely the list_projects change. That makes the remaining decision a clean one and gets rid of the conflict.

Thank you for the report and for the fix — the scope bug was corrupting output from a tool we ship and our own tooling calls, and it is genuinely better for having been caught twice.

Add deterministic pagination and opt-in project details while preserving the metadata_only compatibility alias.

Fail list_projects atomically on name allocation errors and skip qsort for empty project caches.

Signed-off-by: tmonestudio <tmonestudio@users.noreply.github.com>
@tmonestudio
tmonestudio force-pushed the codex/fix-list-projects-coverage-scope branch from 2588aaa to 434b319 Compare July 31, 2026 23:02
@tmonestudio

Copy link
Copy Markdown
Contributor Author

Rebased on current main and removed the now-duplicated coverage-scope production change and tests, so this PR is now purely the list_projects pagination change.

I also addressed both defects from the review:

  • empty caches no longer call qsort with a null base;
  • realloc/strdup failure now frees partial state and returns an explicit MCP error instead of a truncated successful response with inconsistent total/has_more.

The pagination tests now cover deterministic page order, the second page, metadata_only=true, and include_details=true. The final signed-off head is 434b319f, and the PR description now reflects only the remaining live change. Full CI is running.

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

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants