fix(store): report a failed COUNT read instead of returning zero

[kavish-19]

Fixes #2012.

Symptom

cbm_store_count_nodes() treats every sqlite3_step() result other than SQLITE_ROW as a count of zero. A read that failed — SQLITE_CORRUPT, SQLITE_BUSY, SQLITE_IOERR — is indistinguishable from a project that genuinely holds no nodes, and index_status renders it as the positive assertion status: "empty". A user or agent reading that concludes the repository was never indexed and kicks off a multi-minute re-index, while the corruption is never surfaced.

Root cause

    bind_text(stmt, SKIP_ONE, project);
    int count = 0;
    if (sqlite3_step(stmt) == SQLITE_ROW) {
        count = sqlite3_column_int(stmt, 0);
    }
    sqlite3_reset(stmt);
    return count;          /* a failed step returns 0, same as an empty table */

There is no else. Both functions already carry an error channel — each returns CBM_STORE_ERR when prepare_cached fails — so callers already receive a negative value from this API today. Only the step result was never reported through it.

Correction (was wrong in the first version of this description). I originally claimed the consumer was already written for a negative count, quoting a if (nodes < 0) { degraded = true; ... } guard. That guard is in build_index_success_response — index_repository's response builder — not in handle_index_status. I found it by grep and did not check the enclosing function. Thanks to @DeusData for catching it.

handle_index_status (src/mcp/mcp.c) reads the counts directly and renders:

yyjson_mut_obj_add_str(doc, root, "status", nodes > 0 ? "ready" : "empty");
...
if (nodes == 0) { /* "Project is empty. Re-run index_repository..." */ }

So the store change on its own would have left the corrupt database from #2012 answering nodes: -1, edges: -1, status: "empty" — the same false all-clear, now with a bare -1 as the count and no hint at all, since nodes == 0 is false. That is why this PR now carries a second commit.

The fix

Two commits, one claim — a failed read must never read as an empty project:

1. fix(store) — cbm_store_count_nodes / cbm_store_count_edges initialise count to CBM_STORE_ERR, so a non-row step is reported through the error channel each already uses for a failed prepare. A successful step overwrites it, so the healthy path is unchanged.
2. fix(mcp) — handle_index_status treats a negative node or edge count as a failed read: status: "error", the negative numbers suppressed rather than emitted as counts, and a hint naming the table that could not be read in place of the "Project is empty" re-index advice.

I included count_edges because handle_index_status reads the two as a pair — fixing only the node side would leave a corrupt edges table contributing a bare -1 to the same response. The issue's own reproduction shows the pair contradicting itself (nodes: 0 beside edges: 8).

Not changed: cbm_store_count_edges_by_type has the same shape but is not read by index_status, and cbm_store_count_vectors has the shape without an existing error channel — giving it one would be a new contract. Both felt like separate changes rather than part of this one; happy to follow up if you want them.

Verification

Store half — harness linked against src/store/store.c, inserting a node then dropping the tables after the statements are cached, so the step fails rather than the prepare:

BEFORE (main)   healthy  nodes=1 edges=0
                dropped  nodes=0  edges=0     <- indistinguishable from "empty"

AFTER  (branch) healthy  nodes=1 edges=0      <- healthy path unchanged
                dropped  nodes=-1 edges=-1

index_status half — every branch of the new decision, checked directly:

in(nodes=  8,edges=  8) -> status=ready  nodes=8 edges=8  hint=(none)
in(nodes=  0,edges=  0) -> status=empty  nodes=0 edges=0  hint=Project is empty...
in(nodes= -1,edges=  8) -> status=error  nodes=0 edges=0  hint=nodes table could not be read
in(nodes=  8,edges= -1) -> status=error  nodes=0 edges=0  hint=edges table could not be read
in(nodes= -1,edges= -1) -> status=error  nodes=0 edges=0  hint=nodes and edges tables could not be read

The healthy and genuinely-empty paths are untouched; only a negative count changes behaviour.

Tests added:

• tests/test_store_nodes.c → store_count_failed_read_is_not_zero
• tests/test_mcp.c → tool_index_status_reports_an_unreadable_count_as_an_error, using the in-memory server as you suggested: call index_status once (caching the statements), drop the tables, call again, and assert status: "error", no negative counts, and a hint naming the table.

What I could not run locally, and why

I could not run scripts/test.sh or scripts/lint.sh on this machine, and I would rather say so than imply a green run:

• scripts/build.sh fails at internal/cbm/preprocessor.cpp with fatal error: 'cctype' file not found. The C++ standard-library headers are missing from this machine's Command Line Tools (the SDK contains .../MacOSX.sdk/usr/include/c++/v1/cctype, but clang does not resolve it even with -isysroot). It is a local toolchain fault, unrelated to this change, and it blocks the test runner because that also links preprocessor.o.
• clang-format, clang-tidy and cppcheck are not installed here.

What I did instead: both changed files compile clean under the project's own warning set (-std=c11 -Wall -Wextra -Werror -Wno-unused-parameter -Wno-sign-compare), and I followed the surrounding formatting by hand. CI is the authority on the full suite and the linters. If it flags anything, I will fix it promptly.

[github-actions]

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.

[DeusData]

Thanks for this — the store half is exactly right. I reproduced the defect on main (a failed COUNT step really does come back as 0), built the PR merged onto current main and ran 15 suites across store, dump, graph-buffer, artifact, incremental, mcp, httpd and index-resilience: 894 passed, 0 failed, and your new test goes red the moment the store change is reverted, so it binds. Lint is clean and the sign-off is in place.

One thing keeps me from merging it as "Closes #2012", and it is in the PR description rather than the code: the consumer is not already written for a negative count. The guard you quote lives in index_repository's response builder (build_index_success_response, src/mcp/mcp.c ~10185), which does map nodes < 0 to degraded. handle_index_status (src/mcp/mcp.c:6647) is a different path and still does this at ~6669–6674 and ~6691:

int nodes = cbm_store_count_nodes(store, project);
int edges = cbm_store_count_edges(store, project);
...
yyjson_mut_obj_add_str(doc, root, "status", nodes > 0 ? "ready" : "empty");
...
if (nodes == 0) { /* "Project is empty. Re-run index_repository..." hint */ }

So after your change the corrupt database from #2012 would answer index_status with nodes: -1, edges: -1, status: "empty" — the false "empty" the issue is about still fires, only now with a bare -1 in the output and no hint at all. Could you add, in this same PR:

• in handle_index_status: if nodes < 0 || edges < 0 → status: "error" plus a hint that names the unreadable table (e.g. "the nodes table could not be read; the database may be corrupt — re-run index_repository or remove the project cache"), and do not emit the negative numbers as counts (omit them or emit 0 with the error status)
• a test for that path next to your store test — the in-memory server in tests/test_mcp.c can produce the failed read the same way your store test does

Leave /api/project-health alone; that server is being replaced. list_projects / get_architecture printing -1 is visible but harmless and can stay for now. With that, this closes #2012 for real and I'll merge it on green. Welcome aboard — this is a good first change.

[kavish-19]

You're right, and thank you for checking the consumer rather than taking my word for it.

I verified the correction before acting on it: the guard I quoted is at build_index_success_response (index_repository's response builder), and handle_index_status at mcp.c:6647 is a separate path that does exactly what you describe — nodes > 0 ? "ready" : "empty" and a hint guarded on nodes == 0. So the store change on its own would have turned the #2012 database into nodes: -1, edges: -1, status: "empty" — the same false all-clear, now with a bare -1 and no hint, which is worse than what it replaced. I found that guard by grep and never checked its enclosing function; the PR description asserted it as fact. That was my error and I've corrected the description rather than quietly editing it.

Pushed as a second commit, rebased onto current main:

• handle_index_status now treats a negative node or edge count as a failed read: status: "error", the negative numbers suppressed instead of emitted as counts, and a hint naming the table that could not be read ("The nodes table could not be read; the database may be corrupt. Re-run index_repository(repo_path=...) or remove the project cache and re-index.") in place of the "Project is empty" advice. Both-unreadable gets a combined message.
• tests/test_mcp.c → tool_index_status_reports_an_unreadable_count_as_an_error, next to the existing index_status test and using the in-memory server as you suggested: call index_status once so the count statements are cached, drop nodes and edges, call again, and assert status: "error", no negative counts, and a hint that names the table and is not the empty-project one.

Left alone as you asked: /api/project-health, list_projects, get_architecture.

Every branch of the new decision, checked directly:

in(nodes=  8,edges=  8) -> status=ready  nodes=8 edges=8  hint=(none)
in(nodes=  0,edges=  0) -> status=empty  nodes=0 edges=0  hint=Project is empty...
in(nodes= -1,edges=  8) -> status=error  nodes=0 edges=0  hint=nodes table could not be read
in(nodes=  8,edges= -1) -> status=error  nodes=0 edges=0  hint=edges table could not be read
in(nodes= -1,edges= -1) -> status=error  nodes=0 edges=0  hint=nodes and edges tables could not be read

The healthy and genuinely-empty paths are unchanged; only a negative count behaves differently.

One thing I still cannot do on this machine, so you know what my "verified" covers: scripts/build.sh dies at internal/cbm/preprocessor.cpp with fatal error: 'cctype' file not found — the C++ standard-library headers are missing from this box's Command Line Tools — and that blocks the test runner, which links preprocessor.o too. clang-format/clang-tidy/cppcheck are not installed either. So both changed files compile clean under the project's own warning set (-Wall -Wextra -Werror -Wno-unused-parameter -Wno-sign-compare), and the table above is the decision logic exercised directly, but the two new tests have not been executed by me — CI is their first real run. If either fails I'll fix it promptly rather than leaving it to you.

[DeusData]

The index_status change is exactly what was needed — thank you. One CI gate is red: cppcheck (a lint check, our CI runs cppcheck 2.20) flags the new hint selection: src/mcp/mcp.c:6705:30: style: Condition 'nodes<0' is always false [knownConditionTrueFalse]. It is the nested ternary — after the 'nodes < 0 && edges < 0' branch, cppcheck reasons about the remaining cases and rejects the second 'nodes < 0' test. Our rule for linter findings is to refactor rather than suppress: pick the hint with a plain if / else-if / else chain into a 'const char *hint' (both-unreadable, nodes-unreadable, else edges-unreadable), then add it once — or use a single message that names 'the nodes and/or edges table'. Everything else on the run is green. Keep the sign-off and I'll merge on green.