fix(mcp): non-mutating query-only store resolve; annotate the ten read-only tools

[LazyXuan]

Problem

The ten pure query tools (search_graph, query_graph, trace_path,
get_code_snippet, get_graph_schema, get_architecture, search_code,
index_status, check_index_coverage, detect_changes) are annotated
readOnlyHint=false / destructiveHint=true. Plan-mode clients (Claude
Code, Codex, ZCode, ...) gate tool availability on these hints, so the
entire read-only analysis surface is unavailable while planning.

The annotations were deliberately conservative (see the previous test
comment): resolve_store()'s corrupt-store recovery quarantines database
files - a filesystem mutation reachable from a "query".

Fix

This closes the gap that comment pointed at, rather than flipping the
booleans blindly:

• resolve_store_internal() gains an allow_autorecovery flag. The
  query-only path (resolve_store()) passes false: a failed integrity
  check is classified read-only (no mutation lease, no quarantine). A
  confirmed CORRUPT database is reported - the tool reply now says
  "project store is corrupt (left untouched); run index_repository to
  rebuild it" instead of a misleading "project not found" - and left in
  place. TRANSIENT verdicts stay retryable-busy, as before.
• Write-side opens (index_repository, manage_adr updates) keep the
  existing blocking recovery, quarantine, and post-lease generation
  re-check semantics byte-for-byte.
• The ten query tools flip to readOnlyHint=true / destructiveHint=false,
  which is now honest, so plan-mode clients can expose them.

Tests

• Annotations expectation table flipped.
• tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested: the
  query branch now asserts no lease is taken, the corrupt DB stays in
  place, and the reply names the corruption. The manage_adr branch is
  unchanged.
• tool_corrupt_store_cleanup_guard_denial_preserves_db_and_wal: the
  query path requests no lease at all (begin_count 0).
• The four write-side recovery tests (generation re-check after lease
  wait, unique backup naming, publish failure, published-WAL snapshot
  completeness) are driven through manage_adr update - the write path
  where that recovery lives.
• query_graph / search_graph return "project not found" inconsistently; DBs valid on disk but CLI looks up wrong inode #704 ghost-db test expects the corruption message for the 0-byte ghost.

Local Windows verification: mcp suite 202 passed / 0 failed / 7 skipped
(POSIX-only skips). No issue exists for this yet as far as I could find -
happy to adjust if there is a preferred direction.

[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]

Thank you for grounding the MCP read-only annotations in an actually non-mutating corrupt-store path rather than only changing the hints. This touches both client safety metadata and store-recovery semantics, so we need more time to review the boundary carefully. The contribution queue is quite full, but we will come back with grounded feedback as soon as possible.

[DeusData]

Approved. The important thing here is what you did not do: flip the booleans.

The annotations were conservative for a real reason — resolve_store()'s corrupt-store recovery quarantines database files, which is a filesystem mutation reachable from something calling itself a query. Flipping readOnlyHint without closing that would have made the annotation a lie, and readOnlyHint=true is exactly the claim a plan-mode client trusts when deciding what to run without asking. You closed the gap and then made the annotation true, which is the only order that works.

The split is clean and I checked both sides. resolve_store() passes allow_autorecovery = false; the two write-side call sites pass true. So index_repository and manage_adr keep their blocking recovery, quarantine and post-lease generation re-check unchanged, while the query path cannot reach them.

And the query path is structurally read-only, not merely disciplined. On a failed integrity check it closes the store and reopens through cbm_store_open_path_query() before asking for a verdict. That is a query-only handle — the corrupt database is inspected without a writable open at all, rather than through a writable handle that everyone promises not to write to. Structure beats intent here, and you chose structure.

Reporting the corruption honestly is the other half of the fix. "project store is corrupt (left untouched); run index_repository to rebuild it" is a far better answer than "project not found", which sent people looking for a configuration problem that did not exist. Keeping TRANSIENT retryable-busy preserves the distinction that matters.

Your tests target the right property. Asserting begin_count 0 on the query branch — no lease requested at all — is stronger than asserting the file survived, because it catches an attempt rather than only a completed mutation. Driving the four write-side recovery tests through manage_adr update, where that recovery actually lives, keeps them meaningful after the split.

One thing worth stating plainly rather than leaving implied: this closes the mutation the prior comment identified, and your lease assertion guards the lease path — but neither independently re-proves the ten tools are read-only in every other respect. That was already the codebase's position before this PR, so it is not a regression and I am not asking you to prove it. Just worth having said, since readOnlyHint=true is now a promise made to clients rather than an internal note.

Please rebase — this touches src/mcp/mcp.c and #1703 landed there earlier today, so it is currently conflicting. main also went briefly broken and was repaired by #1993.

Noting there was no issue for this and offering to adjust direction was the right way to raise it. There is now: the annotations were wrong, and this makes them right.

[DeusData]

Merged as f37a47eb. Thank you — this was a careful piece of work and it got better under review rather than just greener.

The reason this landed instead of a one-line annotation flip: readOnlyHint=true is a claim a client trusts, and a plan-mode client uses it to decide what it may run without asking the user. The conservative destructiveHint=true was there for a real reason — resolve_store()'s corrupt-store recovery quarantines database files, a filesystem mutation genuinely reachable from a "query". You closed that gap before making the claim, which is the only correct order. Reopening through cbm_store_open_path_query() so the corrupt DB is inspected without a writable open at all is structural rather than disciplined: it does not depend on any future caller remembering a rule.

Your last commit deserves specific credit. The mutation-guard suite was failing on tsan and lsan-macos while the behaviour suite passed everywhere, and the diagnosis was right — the assertion ran after the test's own teardown had unlinked the path, so cbm_file_exists() was observing cleanup rather than the query-only resolve it was meant to pin. Capturing query_db_left_in_place at the moment the property actually holds, and asserting on that snapshot, fixes the test's design. It would have been much easier to move an assertion, widen a tolerance, or add a retry until the schedulers agreed; those make a red disappear without making the test mean anything. This repo has a standing rule that a test's verdict must be a function of the code rather than of the machine it ran on, and your fix is exactly that.

Two smaller things worth noting for anyone reading this later: the tests assert begin_count == 0, so they catch a lease being requested, not merely one that completed a mutation — that is the stronger assertion. And you were precise about the boundary in flagging that compare_graphs bypasses resolve_store() entirely rather than quietly implying full coverage.

The residual I raised earlier stands as a note and not a defect: this closes the mutation that was identified and the lease assertion guards the lease path, but neither independently re-proves the ten tools are read-only in every other respect. That was already the codebase's position before this PR, so it is not a regression introduced here.

Thanks also for the rebase — src/mcp/mcp.c moved under you while this was open.