[YoseptF]

Note: I jumped the gun and opened this before raising an issue. I have now raised #1110 as CONTRIBUTING.md asks and am keeping this as a draft until it is discussed.

MCP reads on built-in drivers run in a read-only transaction, so the database rejects any write the classifier misses. Plugin drivers didn't get this: RpcDriver used the trait default, which is a plain execute_query.

RpcDriver::execute_read_only_query now calls a new execute_read_only_query plugin method and falls back to execute_query when the plugin answers "method not found", so existing plugins behave as before. The PostgreSQL plugin implements it in TabularisDB/tabularis-postgresql-plugin#151.

How to test

1. Build the plugin from feat: add execute_read_only_query for database-enforced read-only reads tabularis-postgresql-plugin#151 (cargo build --release) and replace the installed postgresql-plugin binary with it.
2. Add a connection on the PostgreSQL plugin driver and keep MCP read-only mode on for it.
3. From an MCP client, run_query with SHOW transaction_read_only:
   • main: off
   • this branch: on
4. Put the released plugin back and repeat: the query still returns (off), via the fallback.

Unit tests: cargo test --lib read_only_query (forwarding, the -32601 fallback, and other errors still surfacing).

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Overview

PR #1109 adds read-only transaction support for MCP reads on plugin drivers. Previously only built-in drivers ran MCP reads in a read-only transaction; RpcDriver used the trait default (plain execute_query). This PR adds RpcDriver::execute_read_only_query that calls a new execute_read_only_query plugin RPC method, falling back to execute_query when the plugin returns a -32601 (method not found) error so older plugins behave unchanged.

Changes verified:

1. driver_trait.rs — Doc comment updated from "plugin drivers have no read-only mode" to "for drivers without a read-only mode" to reflect that RpcDriver now overrides the method. Accurate.

2. driver.rs — execute_read_only_query impl (lines 1203–1225):

   • Correctly calls call_detailed (not call) to preserve PluginCallError with structured error codes — call would stringify and lose the -32601 distinguishing code, breaking the fallback.
   • Uses the is_method_not_found helper (lines 42–49), which checks Remote.code == -32601 and falls back to a string heuristic for Transport errors. This matches the pattern used by get_table_query_template (line 1552) and get_schema_snapshot (line 1563).
   • Falls back to self.execute_query(...) for method-not-found; propagates all other errors via error.to_string() (verified against PluginCallError::Display in rpc.rs:26-32, which writes only the message).
   • Fallback params match execute_query's with_primary_database(params) pattern (line 1199).

3. driver.rs — Tests (lines 3221–3285):

   • read_only_result() produces a valid QueryResult JSON (columns, rows=[[1]], affected_rows, pagination: null; truncated has #[serde(default)] in models.rs:511).
   • rpc_driver_forwards_execute_read_only_query — uses test_driver (success-only path); asserts method name, query param, and schema param. Correct.
   • rpc_driver_execute_read_only_query_falls_back_when_the_plugin_lacks_it — uses test_driver_detailed (needed to return PluginCallError::Remote with code: -32601); verifies fallback to execute_query. Correct.
   • rpc_driver_execute_read_only_query_propagates_other_errors — uses test_driver_detailed with code: -32603 (non-method-not-found); verifies error message propagation. Correct.

No existing inline comments or reviews were present on this PR. No issues identified.

Files Reviewed (2 files)

• src-tauri/src/drivers/driver_trait.rs - doc comment update
• src-tauri/src/plugins/driver.rs - new method + 3 unit tests

Fix these issues in Kilo Cloud

---

_{Reviewed by free · Input: 196.9K · Output: 14.5K · Cached: 546.7K}

[aesslinger]

Reviewed this alongside its companion plugin PR (tabularis-postgresql-plugin#151). Focused on the host-side wiring here.

This side is necessary — and correct

The fallback design is the right shape. MCP reads on plugin connections today hit the trait default execute_read_only_query, which is plain execute_query. So the classifier is the only safeguard — a classifier-missed write (a mutating function, a data-modifying CTE like WITH t AS (INSERT …) SELECT … which reads as read) reaches the database. This PR overrides RpcDriver::execute_read_only_query to actually call the plugin's method, with a -32601 fallback to execute_query so plugins without the method keep working. Without it, even after the plugin PR merges, MCP reads still get no read-only guarantee.

The -32601 fallback handshake is sound

I verified both ends agree on the "method not found" signal, so the fallback won't silently break for old plugins:

• Plugin side: unmatched methods fall through to not_implemented → error_response(id, -32601, …) (this repo's rpc.rs).
• Host side: is_method_not_found matches on remote.code == -32601 (plus a transport-message fallback). ✓

A plugin that lacks execute_read_only_query answers -32601 → host falls back to execute_query → same behavior as today. A plugin that has it answers the read-only result. Non--32601 errors surface as-is (the third unit test covers this).

Scope is contained

90-line RpcDriver override + a 2-line trait-doc tweak. It doesn't touch the trait default or any builtin driver, so blast radius is just the plugin adapter. The three unit tests cover the three real paths (forwarding, -32601 fallback, other-errors-propagate).

Both PRs are required; neither lights up alone

Same companion pattern as 114/119/123. Without this PR, the plugin method is never called. Without the plugin PR, the call returns -32601 and falls back to execute_query — same as today. The -32601 fallback means merge order isn't strict, but coordinating the release avoids a window where the host calls a missing method on every MCP read (wasteful but harmless — each call is one extra round-trip).

Suggested merge order: plugin #151 (or at least its contract) lands first or together with this one, so a released host never calls a method the released plugin lacks.

One thing to confirm on the plugin side

The plugin PR (#151) ports the builtin's BEGIN READ ONLY/ROLLBACK but omits the builtin's InFlight Drop guard — a panic between BEGIN and ROLLBACK would return the connection to the pool with the read-only transaction still open (RecyclingMethod::Fast resets nothing), so the next borrower inherits it. The builtin deliberately closes the connection in that case. I raised this on #151; flagging here only because the fallback means a released host calling a fixed plugin is the desirable end state, so both PRs landing in their final form matters.

---

Net: this PR is needed and good to go. It's the companion that makes the plugin-side work light up, and it carries the safe backward-compat fallback. Just coordinate the merge/release with #151.

[NewtTheWolf]

Thanks for the host-side wiring and the forwarding/fallback tests. The protected plugin RPC has now landed through #1463 with the read_only_queries capability. Legacy plugins retain their existing path until they opt in; an opted-in plugin must fail closed if its protected RPC fails. Companion plugins can advertise that capability once their implementation is ready. #1324 covers plugin-owned query classification separately.

I am closing this draft as superseded by that host implementation. Any remaining gap can be handled as a focused follow-up against the current contract.

Since this is your first PR here, you are welcome to join us on Discord: https://discord.com/invite/K2hmhfHRSt