[kobewl]

Problem

Three places ask the same question of a query: is it a clean read? The MCP read-only mode, the MCP approval gate and, since #1463, a saved read-only connection. The host can only answer it for SQL. A non-SQL plugin such as MongoDB (db.users.find({})) comes out unknown, which counts as a write: the MCP read-only mode blocks it, writes_only approval queues it, and a saved read-only connection rejects it before the plugin is ever asked.

What changed

This reworks the PR after @NewtTheWolf's review. It builds on #1463 instead of next to it.

• No parallel execution guarantee. read_only_execution and my second RpcDriver::execute_read_only_query are gone. The guarantee is main's capabilities.read_only_queries (or readonly) and main's RpcDriver::execute_read_only_query, unchanged: legacy plugins keep their behaviour, an intrinsically read-only driver keeps execute_query, and a failed protected call never falls back to it. plugins/driver.rs only gains classify_query, its response parser and a test module.
• One new capability, query_classification. It gates an optional classify_query RPC that replies { "kind": "select" | "write" | "ddl" | "unknown" } for the exact text about to run. Opt-in, default false, so existing plugins see no new RPC.
• One classification function for every policy. application::read_only::classify_query(driver, params, query) (moved from mcp/query_kind.rs) is what the MCP gates and the saved read-only connection both ask. The MongoDB grammar stays out of core: ai_activity.rs and ai_activity_tests.rs are identical to main.
• The saved-connection preflight carries the plugin's verdict. read_only::validate_query is now async and takes the driver. The editor (single queries and batches), exports and the transfers reach it, directly or through read_only::execute_query. Before, it read the text as SQL, so a valid MongoDB read was rejected before the plugin was consulted.
• MCP classifies once per text. tool_run_query feeds one verdict to the saved policy, the MCP read-only mode and the approval gate, through read_only::validate_kind (the same policy for a verdict already in hand). A query edited in the approval dialog is classified again, and the edited text is what the gates and the final check see.

How the host decides (fail closed)

• Without query_classification: no RPC, the text is read as SQL, as on main.
• With it, the text is never read as SQL. An RPC error (-32601 included), a transport error, a timeout, a result without a valid kind and an unknown kind are all unknown.
• A select only counts when the driver also has an execution guarantee (read_only_queries or readonly). Otherwise it is unknown: a classifier alone does not stop a write at execution.
• A saved read-only connection still needs read_only_queries or readonly (main's validate_support), so a classifier-only driver cannot enter read-only mode at all.
• A read that passes runs through main's execute_read_only_query: the protected RPC for read_only_queries, the ordinary path for a readonly driver. A failed protected call is never retried through execute_query.

Contract surface kept in sync

plugins/PLUGIN_GUIDE.md (new "Query Classification" section, capability rows, a cross-reference in "Optional protected read-only queries", registry rollout note), plugins/manifest.schema.json, plugins/tabularium-extensions.schema.json and packages/web-ui/src/types/plugins.ts, with a Vitest test for the two schemas next to columnStatsSchema.test.ts.

Also corrected the pre-existing execute_query example in the guide while touching it: the host sends limit / page / schema (not page_size) and a plugin result must carry columns, rows and affected_rows. A result written from the old example does not deserialize on the host side.

Tests

24 new Rust tests and 2 Vitest cases. They combine the MCP read-only mode and saved read-only connections, as requested in review. Every scenario reads the call log of a fake plugin, because "blocked" has to mean that nothing reached its executor.

• application/read_only/plugin_tests.rs (9), the saved read-only connection: a classified read with the guarantee runs through the protected RPC after its verdict; writes, DDL, unknown text, SQL-looking text and a failing classifier are refused with only classify_query reaching the plugin; a failed protected call is not retried through execute_query; a classifier-only driver cannot enter read-only mode; a readonly driver keeps execute_query; a SQL plugin is unchanged. One test drives application::queries::execute_query / execute_query_batch against a saved connection, the real editor entry points, including a batch that is refused as a whole before its first statement is sent.
• mcp/read_only_tests.rs (1 test, several scenarios, run in a child process with throwaway storage like database_scope_is_enforced_before_driver_access): the MCP read-only mode; a saved read-only connection; the approval gate with nobody to approve (a guaranteed read runs, a classifier-only select still waits); queries edited in the approval dialog (into another read, into a write / DDL / unknown text / failing classification on a saved read-only connection, into a write while the MCP read-only mode is on); a legacy SQL plugin unchanged.
• application/read_only/classification/tests.rs (7) and plugins/query_classification_tests.rs (7): the verdict table, every failure mode, and the exact text and connection on the wire.
• application/read_only/tests.rs: main's SQL preflight test is now async.

I checked that these fail when the code is wrong. Each of these seven changes makes at least one test fail (and each was reverted): the final MCP check uses the original kind instead of the edited one; a select is trusted without a guarantee; the saved-connection preflight reads the text as SQL again; a failed protected call falls back to execute_query; validate_kind lets every kind through; a batch is not validated up front; MCP classifies the text as SQL.

Run locally:

• cd src-tauri && cargo test --locked --no-fail-fast: lib 2531 passed, 0 failed, 18 ignored; the integration test crates compile (the database-backed ones are ignored). On macOS I set TMPDIR to a path without symlinks: with the default /var/folders/... two theme tests (application::persistence::tests::persists_and_deletes_custom_themes_in_the_shared_config_directory, application::themes::tests::announces_committed_personal_theme_changes) fail with "Theme paths must not contain symbolic links", because /var is a symlink. They are unrelated to this change.
• cargo check --locked --no-default-features --bin tabularis and the Tauri boundary grep from CI: pass.
• pnpm run typecheck, pnpm run lint (ESLint, Prettier for the whole repo, theme tokens), web:boundaries, web:packaging, web:docs, web:manual-parity, test:web-contract: pass.
• pnpm test: 555 files, 8829 tests passed.
• rustfmt is clean on the files this PR adds. A few other files on main have formatting differences already; I left them alone.

Behaviour to know

• Cost. MCP classifies a text once. A saved read-only connection classifies in the preflight and again inside the protected execution, so the editor, a batch statement and an export page each cost two classify_query calls for a plugin that declares the capability. SQL drivers never make the call. Folding the two into one would mean passing the verdict into read_only::execute_query, which every transfer and export caller uses, so I left it out of this PR.
• Host-generated SQL. The COUNT(*) wrapper of the editor and the transfer pagers are classified like any other text. A plugin that declares query_classification is asked about them, and answers for what its executor would do with that exact text.

Scope and follow-ups

• This is the host contract only. The MongoDB plugins still have to implement classify_query and execute_read_only_query and declare the capabilities, in their own repositories. Until then MongoDB behaves as on main.
• Before a plugin that declares query_classification is published, the boolean has to be added to the driver kind's capabilities.properties in the Tabularium registry (noted in the guide, like the other optional capabilities).

For the maintainers to decide

• trusted_kind in application/read_only/classification.rs downgrades a plugin's select to unknown unless the driver has an execution guarantee. If you would rather trust the plugin's select, it is a one-line change.
• I merged origin/main into the branch instead of rebasing, to avoid force-pushing a branch with review history. The PR is squash merged, so I expect the history not to matter. I am happy to rebase if you prefer.

[kilo-code-bot]

WARNING: $out / $merge detection is a raw text scan and can fail open

This classifier is the only read-only / approval gate for MongoDB, and aggregate is classified select unless the literal bytes $out or $merge appear in key position. The plugin parses the pipeline as JSON/EJSON, which allows spellings that decode to the same stage name but do not contain those bytes, e.g. {"\u0024out": "archive"} (JSON unicode escape for $). db.users.aggregate([{"\u0024out": "archive"}]) would be classified select, bypassing read-only mode and the writes_only approval gate while writing to a collection.

Since the doc comment states the design goal is to never widen what a gate lets through, consider failing closed: e.g. treat an aggregate as a read only if the arguments contain no backslash escapes outside plain string values (or simply parse the args as JSON and inspect the actual stage keys), and return unknown/write when the args cannot be parsed. Add a test for the escaped-key case.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The only change since the last review (205d9c99) is a docs update to plugins/PLUGIN_GUIDE.md. It corrects the execute_query request and response examples. I checked it against RpcDriver::execute_query in src-tauri/src/plugins/driver.rs and the QueryResult struct in src-tauri/src/models.rs.

• Request: limit, page and schema match what the host sends. page_size is gone, as it should be.
• Response: columns, rows and affected_rows are required. truncated defaults to false, and pagination, server_time_ms and fetch_time_ms are optional.
• Example handler: it no longer uses the old total_count and execution_time_ms fields.

The earlier findings ($out/$merge escapes and similar) applied to the removed host-side classifier and no longer apply. One older inline comment on src-tauri/src/ai_activity.rs is outdated (line: null), so I did not carry it forward.

Files Reviewed (1 file)

• plugins/PLUGIN_GUIDE.md

Previous Review Summaries (4 snapshots, latest commit 205d9c9)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 205d9c9)

Status: No Issues Found | Recommendation: Merge

The PR changed design since the last review. The host-side classify_mongo_shell heuristic is removed. Plugins now classify their own queries through an opt-in classify_query RPC, with an optional execute_read_only_query RPC. Because the host classifier is gone, the earlier findings ($out/$merge escapes, comment-hidden operations, path grammar) no longer apply. I found no references to the removed code.

Checked against current code:

• classify_for_driver (src-tauri/src/mcp/query_kind.rs) fails closed. A plugin that declares query_classification is never read as SQL, and an error, an unreadable reply or -32601 all give unknown.
• A select verdict only counts when the driver also declares read_only_execution, so a clean read always runs through the read-only path.
• RpcDriver::execute_read_only_query never falls back to execute_query once the capability is declared.
• parse_query_kind accepts exactly the four lowercase kinds.
• In tool_run_query, classification runs after the database override and again on a query edited during approval, so an approver cannot turn a read into a write.
• The new DriverCapabilities fields are opt-in and skipped in serialization, so legacy manifests are unchanged. All three built-in drivers and the integration-test manifest set them to false, and other test fixtures use ..Default::default().
• The schemas, PLUGIN_GUIDE.md and plugins.ts types are consistent with each other.

The plugin RPC contract is documented, and the tests cover opt-in, failure, fail-closed and no-fallback behavior.

Files Reviewed (15 files)

• src-tauri/src/mcp/query_kind.rs
• src-tauri/src/mcp/query_kind/tests.rs
• src-tauri/src/mcp/mod.rs
• src-tauri/src/plugins/driver.rs
• src-tauri/src/plugins/query_classification_tests.rs
• src-tauri/src/drivers/driver_trait.rs
• src-tauri/src/drivers/mysql/mod.rs
• src-tauri/src/drivers/postgres/mod.rs
• src-tauri/src/drivers/sqlite/mod.rs
• src-tauri/tests/postgres_integration/parity.rs
• packages/web-ui/src/types/plugins.ts
• packages/web-ui/tests/utils/queryClassificationSchema.test.ts
• plugins/PLUGIN_GUIDE.md
• plugins/manifest.schema.json
• plugins/tabularium-extensions.schema.json

Previous review (commit 43e5f5c)

Status: No Issues Found | Recommendation: Merge

The incremental changes harden classify_mongo_shell against parser differences between the two registered MongoDB plugins. Verified against current code:

• Any line starting with // now returns None (unknown). This closes the db.users\n//.find(\n.drop() comment-hiding payload.
• mongo_operation takes the first dot and rejects whitespace, / and \ in the path. Collection names containing dots, such as db.a.b.find, now fail closed to unknown.
• A doubled trailing ;; fails the ends_with(')') check and returns unknown.
• estimatedDocumentCount is classified as a read and dropIndex as DDL.
• The new tests cover these cases, including comment-hidden writes and the path grammar.

The earlier $out/$merge escape finding remains fixed: args are parsed with serde_json, and the fallback fails closed.

Note: the MongoDB plugins are external and were not available, so their grammars were assumed from the PR description and code comments.

Files Reviewed (2 files)

• src-tauri/src/ai_activity.rs
• src-tauri/src/ai_activity_tests.rs

Previous review (commit c2ca213)

Status: No Issues Found | Recommendation: Merge

The previously reported $out/$merge escape bypass is fixed: mongo_aggregate_writes now parses the args with serde_json and inspects decoded keys, and falls back to a fail-closed path (any backslash is treated as a write, otherwise the text scan) when the args are not valid JSON. New tests cover escaped keys (\u0024out, \u0024merge, fully escaped spelling), bare $out alongside shell helpers, and a benign escaped value staying a read.

Note: the MongoDB plugin is external and was not available, so its grammar was assumed from the PR description and code comments.

Files Reviewed (2 files)

• src-tauri/src/ai_activity.rs
• src-tauri/src/ai_activity_tests.rs

Previous review (commit 98d977b)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
src-tauri/src/ai_activity.rs	577	$out/$merge detection is a raw text scan; JSON-escaped keys such as "\u0024out" decode to the same stage in the plugin but are classified select, bypassing read-only mode and the writes_only approval gate. Fail closed (parse the args or reject escapes) and add a test.

Files Reviewed (2 files)

• src-tauri/src/ai_activity.rs - 1 issue
• src-tauri/src/ai_activity_tests.rs

Note: the MongoDB plugin is external and was not available, so its grammar was assumed from the PR description.

Fix these issues in Kilo Cloud

---

_{Reviewed by claude-sonnet-5.5 · Input: 252 · Output: 1.4K · Cached: 167.9K}

_{Review guidance: REVIEW.md from base branch main}

[kobewl]

Thanks — that was a real bypass, not a false positive. Fixed in c2ca213.

Confirmed the mechanism in the plugin: execute_query passes the aggregate args through parse_pipeline → parse_bson, which does serde_json::from_str on the (preprocessed) text. So db.users.aggregate([{ "\u0024out": "archive" }]) decodes to a genuine $out stage and runs, while the raw-text scan saw no write stage and classified it select.

The check now parses the args as JSON and inspects the decoded keys:

• "\u0024out", "\u0024merge", and per-character escapes ("\u0024\u006f\u0075\u0074") all classify as write.
• Keys are compared after decoding, so $output is still just a field.
• Args that are not valid JSON (ones holding ObjectId(…) / ISODate(…) shell helpers, which the plugin rewrites before parsing) fall back to the text scan, and any backslash in that path counts as a write — it may be an escape the scan cannot decode, so that path fails closed too.
• A read whose only escape is inside a value ({"path": "a\\b"}) is still a select.

New test classify_mongo_aggregate_write_stages_hidden_by_json_escapes covers these. I verified it fails against the previous commit (returns select) and passes now, so it actually pins the hole shut.

cargo test --lib --no-default-features -- ai_activity: 112 passed, 0 failed.

[HongTin2104]

Requesting changes for a read-only bypass. It only affects the MongoDB Atlas plugin, not the one this PR mirrors.

The README lists two MongoDB plugins. classify_mongo_shell mirrors utils/query_parse.rs from danielnuld/tabularis-mongodb-plugin. TabularisDB/tabularis-mongodb-plugin (the Atlas one) parses differently in src/main.rs:

• execute_query first runs strip_comment_lines, which drops every line whose trimmed start is //.
• parse_shell_query takes the collection up to the first . and the operation from there to the first (. It also ignores anything after the balanced ).

The classifier is host-wide and does not know which plugin it is guarding. Because of that, a comment line can show the classifier one operation while the plugin runs another:

db.users
//.find(
.drop()

• Classifier: the first ( is on the comment line, so path is users\n//.find. rfind('.') gives find, the text ends with ), there is no ; and no quoted call, so the result is select.
• Atlas plugin: the comment line is stripped, leaving db.users\n.drop(). The collection is users and the operation is drop, so the collection is dropped.

The same works with .deleteMany({}), .updateMany(...) and the other write operations. Read-only mode and writes_only approval would both let these through. I checked this by porting both parsers (yours and the Atlas plugin's) to a small script, not by running the plugin against a server, so please double-check it on your side.

Some fixes that keep this fail-closed for both plugins:

• Return None when path (the text before the first () contains any whitespace or any /. Neither plugin's documented grammar needs either, and that alone rules out the comment trick.
• Or return None when any line of the query starts with // after trimming, which matches what the Atlas plugin strips.

A regression test with the payload above would be good either way.

Smaller divergences, all fail-closed today, just noting them:

• The Atlas plugin uses trim_end_matches(';'), so it strips every trailing semicolon, while this PR strips exactly one. db.users.find({});; is therefore unknown here and still runs there. This is only a false positive.
• db.a.b.find({}) classifies as select here (last dot). The Atlas plugin reads it as collection a, operation b.find, and rejects it as unsupported. This is harmless now, but it shows why the "first dot vs. last dot" split should be pinned down.
• The Atlas plugin also accepts estimatedDocumentCount and dropIndex, which aren't mapped here. Both stay unknown, which is safe. estimatedDocumentCount could be added to the read list if you want.

Apart from this, the approach is good. Defaulting to unknown and decoding JSON before looking for $out/$merge are the right calls, and the escape-handling fix in c2ca213 is solid.

[kobewl]

Fixed in 43e5f5c — thanks, this was a real bypass and I'd only mirrored one of the two plugins.

I confirmed your reading of the Atlas parser: src/main.rs has strip_comment_lines (drops any line whose trimmed start is //) and parse_shell_query takes the collection up to the first .. I ported both parsers to a script and the payload splits exactly as you described:

db.users
//.find(
.drop()

atlas      -> ('users', 'drop')
danielnuld -> ('users\n//', 'find')

So the fix makes the classifier accept only what both parsers read identically:

• Refuse any // line. The Atlas plugin drops it, the other keeps it, so the two disagree on what the statement even is.
• Split on the first dot, the stricter reading. db.a.b.find({}) is now the Atlas plugin's unsupported b.find (was select).
• Reject whitespace and / in the path. That alone rules out the comment trick, and neither grammar needs them.
• Strip exactly one trailing ;, so db.c.find({});; stays unknown rather than running as a read on the Atlas plugin.

Regression tests in classify_mongo_rejects_comment_hidden_operations cover your payload plus .deleteMany/.updateMany variants, a leading and a trailing // line, and db.users .find({}). I verified the suite fails against the previous commit (returns select) and passes now.

On your smaller notes:

• estimatedDocumentCount → added to the read list.
• dropIndex → added to DDL. I'd missed both; the Atlas dispatch_shell_query match arm lists them and the other plugin doesn't, so mapping them can only make the gates stricter for the plugin that does support them.
• first dot vs. last dot → now pinned by classify_mongo_path_matches_the_stricter_plugin_grammar, with a comment explaining why the first dot is the safe choice.

cargo test --lib --no-default-features -- ai_activity: 114 passed, 0 failed.

One thing worth raising, since it's the root of both this and the $out issue: the host has no way to know which plugin will execute the query, so the classifier has to assume the union of every parser's grammar. That works, but it means adding a MongoDB plugin can quietly widen what the gates let through. A capabilities field declaring the query language (or the parser flavour) would let the host fail closed per-connection instead. Happy to do that as a follow-up if you think it's worth it — it's a larger change than this PR.

[debba]

Thanks for tackling MongoDB reads being blocked by the MCP gates, especially the regression cases for escaped aggregation write stages and disagreements between the two plugin parsers.

Since this is your first PR here, we would be happy to have you join us on Discord if you would like to get involved: https://discord.com/invite/K2hmhfHRSt

After looking at where this belongs, I would like us to keep the MongoDB grammar and operation classification in the driver plugin rather than add them to the core SQL classifier. The MCP code already resolves the driver before checking read-only mode and approval, so that is where a generic driver classification hook could be used. The current plugin contract does not provide that hook yet, so this needs a small host-side contract extension as well.

Could you rework this around an optional plugin classification RPC, with an explicit capability and a conservative fallback for unsupported methods, errors or unknown results? The core should continue to enforce read-only and approval policy, including reclassifying a query edited during approval. MongoDB-specific parsing and its tests should live alongside the parser in each plugin.

Please also account for execution-time protection: the current default execute_read_only_query implementation delegates to execute_query for plugins, so classification alone does not provide database-enforced read-only execution. We should define that guarantee in the plugin contract rather than assume it is already present. The manifest schema, frontend capability types and plugin guide should stay in sync.

I am holding off on end-to-end checks and merging this version while we settle that boundary. The problem is worth fixing; I would prefer a plugin extension that other non-SQL drivers can use too.

[kobewl]

Friendly ping on this one — no rush, just flagging something that may be blocking it.

The five main workflows on this PR are sitting in action_required, so GitHub is waiting for a maintainer to approve them before they run:

• CI
• Integration Tests (PostgreSQL)
• Integration Tests (MySQL)
• Format
• Preview Build

That is the standard first-contributor gate for fork PRs, not a failure. Only "PR title" and "Kilo Code Review" have actually executed so far. Approving them would let the automated suite run and take the manual end-to-end check off your plate — that's the part I can't do from the fork side.

What I have verified locally: cargo test --lib --no-default-features -- ai_activity → 114 passed, 0 failed. The full suite is 2168 passed / 2 failed, and both failures (application::persistence::tests::persists_and_deletes_custom_themes_in_the_shared_config_directory, application::themes::tests::announces_committed_personal_theme_changes) reproduce on unmodified main in this environment — they fail with "Theme paths must not contain symbolic links", unrelated to this change. I could not run the desktop build or the Playwright E2E here, which is presumably what your real-app check covers.

One housekeeping note: the branch is now 45 commits behind main. I checked the merge and it applies cleanly with no conflicts, so nothing is needed — but say the word if you'd prefer a rebase onto current main before the E2E pass.

Still available for the capabilities follow-up (declaring the query language so the host can fail closed per-connection) if you think it's worth pursuing after this lands.

[debba]

@kobewl ouch my fault, started work flows right now

[kobewl]

@debba — quick status question rather than a nudge. CI is green and the merge state is CLEAN, but there is still a CHANGES_REQUESTED from @HongTin2104 on the review list.

Both points from that review were addressed in 43e5f5c, and the reviewer's own suggestions are what the code now implements:

• Comment-hidden operations (db.users\n//.find(\n.drop()): any // line now returns None, and the path is split on the first dot, rejecting whitespace and /. Covered by classify_mongo_rejects_comment_hidden_operations.
• First dot vs. last dot: pinned to the first dot, the stricter of the two plugins. Covered by classify_mongo_path_matches_the_stricter_plugin_grammar.
• estimatedDocumentCount / dropIndex: mapped as read and DDL respectively.

CI confirms it: test, web-e2e, test-postgres, test-mysql (5.7 and 8.4) and Prettier all pass on 6e9795b after your merge of main.

So I think the review is stale rather than open work. Is dismissing it something you need to do on your side, or is it simply waiting in the merge queue? Happy to adjust anything if I've misread the state.

Also — thanks for merging main in. That saved the rebase I had offered.

[kobewl]

@debba — following up on my question from 11-06, now with something concrete rather than a guess.

I noticed #1392 was merged yesterday and its review history reads COMMENTED, COMMENTED, CHANGES_REQUESTED, COMMENTED, DISMISSED — so a stale CHANGES_REQUESTED gets dismissed before merge rather than blocking it. This PR still carries the one from @HongTin2104 opened on 10-29, and it's the only thing standing between CLEAN and mergeable.

That review asked for two things, and both are what the code now does (all in 43e5f5c):

• Reject // lines and split on the first dot, rejecting whitespace and / in the path — the reviewer's own first suggested fix. Test: classify_mongo_rejects_comment_hidden_operations.
• Pin first-dot vs. last-dot, to the stricter interpretation. Test: classify_mongo_path_matches_the_stricter_plugin_grammar.

estimatedDocumentCount (read) and dropIndex (DDL) were added at the same time, also from that review.

CI on 6e9795b is fully green — test, web-e2e, test-postgres, test-mysql 5.7 and 8.4, Prettier — and the merge state is CLEAN, so nothing else is outstanding.

Would you be able to dismiss that review so this can go in? If there's something in it you still want changed, tell me and I'll do it — I just want to be sure it isn't being read as open work.

Thanks for clearing the backlog yesterday, incidentally — 23 merges in a day is a lot of review.

[debba]

Thanks for following up and for addressing the parser discrepancies raised in the earlier review. My architectural request is still outstanding, though: #1324 (review)

The current PR still adds MongoDB-specific parsing and operation classification to the core SQL classifier. Please rework this around an optional plugin classification RPC with an explicit capability, keeping the MongoDB grammar and its regression tests alongside the parser in each plugin. Unsupported methods, errors and unknown results must fall back conservatively.

The core should continue to enforce read-only and approval policy, including reclassifying queries edited during approval. Please also define the execution-time read-only guarantee in the plugin contract: the current default execute_read_only_query implementation delegates to execute_query for plugins, so classification alone is not database-enforced read-only execution. Keep the manifest schema, frontend capability types and plugin guide in sync.

The green CI and the fixes to the earlier review do not resolve this boundary concern, so dismissing that review is not the only remaining step before merge. If useful, the host contract extension and the plugin implementations can be split into coordinated PRs.

[kobewl]

Reworked per your review. The MongoDB grammar is out of core — ai_activity.rs and ai_activity_tests.rs are identical to main again — and the classification belongs to the plugins now. Point by point:

• Optional classification RPC with an explicit capability: capabilities.query_classification gates a new optional classify_query RPC that replies {"kind": "select" | "write" | "ddl" | "unknown"}. Unsupported methods (-32601), transport errors, timeouts, unreadable results and kinds this host doesn't know are all unknown. A driver that declared the capability is never read as SQL, so there is no SQL fallback for its language.
• Core keeps the gates: read-only mode, the approval gate and the re-classification of a query edited in the approval dialog stay in tool_run_query; both classification sites go through one function, mcp/query_kind.rs.
• MongoDB parsing lives in the plugins: it moved out with its tests. What remains here is the generic contract, usable by any non-SQL driver.
• Execution-time guarantee in the plugin contract: a second capability, read_only_execution, gates an optional execute_read_only_query RPC. The trait default still delegates to execute_query, so plugins that don't opt in see no change; a driver that declares it is never retried through execute_query — a -32601 there fails the query. A select from classify_query only lets a query skip the gates when the driver also declares read_only_execution, so classification alone is never the only guard for an unapproved query. That downgrade is one line to drop if you'd rather trust the plugin's select.
• Contract surface in sync: the guide (new "Query Safety for MCP" section, capability rows, registry rollout note), both manifest schemas, and the frontend capability types, with a Vitest test for the schemas. I also corrected the pre-existing execute_query example while there (limit/schema in, the required affected_rows out) because the new RPC section refers to it.
• Split PRs: this is the host contract only. classify_query/execute_read_only_query and the two manifest capabilities follow in the plugin repositories, coordinated with this one.

Verified locally: cargo test (lib 2439 passed), cargo check --no-default-features, the Tauri boundary grep, pnpm run typecheck/lint and the web checks all pass; details in the PR description.

[NewtTheWolf]

Thanks for moving the MongoDB grammar out of core and adding an opt-in classifier with fail-closed error handling. I checked de7d105, including reclassification after an approval edit. Typecheck, the four schema tests and all 2,448 Rust library tests pass locally. The original request for a generic plugin-owned classifier is addressed.

There is now an integration update needed after #1463 landed. Main already defines capabilities.read_only_queries and RpcDriver::execute_read_only_query. Please rebase and reuse that execution guarantee, preserving its intrinsically read-only and legacy-plugin behavior, rather than adding a parallel read_only_execution flag and competing override. The current merge conflicts in the driver implementation.

Please also carry the plugin verdict through the saved-connection read-only preflight. Main's application/read_only.rs currently classifies that path as SQL; otherwise a valid MongoDB read can still be rejected before the new plugin classifier helps. Add combined regressions for MCP read-only mode and saved read-only connections: a classified read with the execution guarantee is allowed, writes/unknown/errors are blocked, and a failed protected RPC never falls back to ordinary execution. I am keeping this open for that combined revision.

[kobewl]

Thanks, this is the combined revision. I merged origin/main into the branch instead of rebasing, so no force-push; I can rebase if you prefer (the PR is squash merged). Point by point:

• Reuse main's execution guarantee. read_only_execution and my second RpcDriver::execute_read_only_query are gone. The guarantee is main's capabilities.read_only_queries (or readonly) and main's RpcDriver::execute_read_only_query, unchanged: legacy plugins, intrinsically read-only drivers and the no-fallback rule behave as on main. The conflict in the driver implementation is resolved by keeping main's method as it is, and plugins/driver.rs only gains classify_query, its parser and a test module. One capability is left, query_classification.
• Carry the verdict through the saved-connection preflight. The classification moved to application::read_only::classify_query, and read_only::validate_query is now async and takes the driver. The editor (single queries and batches), exports and the transfers reach it, so a valid MongoDB read is no longer rejected as SQL before the plugin is asked. MCP classifies each text once and applies the same policy through read_only::validate_kind; a query edited in the approval dialog is classified again.
• Combined regressions. application/read_only/plugin_tests.rs covers the saved read-only connection, including the real editor entry points (application::queries::execute_query / execute_query_batch). mcp/read_only_tests.rs runs the MCP read-only mode, a saved read-only connection, the approval gate and approval edits end to end. Together they check that a classified read with the guarantee runs through the protected RPC, that writes, DDL, unknown text and a failing classifier never reach an executor, and that a failed protected call is not retried through execute_query. Each scenario reads the plugin's call log. I also broke the code in seven ways (for example by reading the text as SQL again, or by falling back to execute_query) and each change made at least one of these tests fail.

Local runs are in the description: cargo test (lib 2531 passed), the server-only cargo check, the Tauri boundary grep, typecheck, lint and the full Vitest suite all pass.

One trade-off to look at: a saved read-only connection classifies in the preflight and again inside the protected execution, so a plugin that declares query_classification sees two cheap calls per statement (MCP makes one). Passing the verdict into read_only::execute_query would avoid it, but every transfer and export caller uses that function, so I left it for a follow-up unless you want it here.

Side note, not part of this PR: on main plugins/tabularium-extensions.schema.json has no read_only_queries entry (manifest.schema.json does). I only added query_classification to both files.

[NewtTheWolf]

Re-reviewed e97fe771beb47ddd94161ba5cb392b1b000dd423. This addresses my combined revision request: the parallel capability/override is gone, the existing read_only_queries/readonly execution guarantee is preserved, and saved-connection preflight now uses the plugin verdict. The combined MCP/editor tests check reads, writes, unknown/error verdicts, approval edits and failure of the protected RPC without an ordinary-execution fallback. I found no remaining blocker in that correction.

The complete Rust library suite passes with one test thread (2,540 passed, 19 ignored), as do server-only checking, the manifest-schema tests, frontend typecheck and lint. The default parallel library run was not clean: registry reconciliation observed readonly-export-* fixtures, and the Kubernetes tunnel tests reported an occupied port followed by a poisoned lock. Those failures remain recorded separately; I am not claiming that parallel run passed. I did not test a released MongoDB plugin against a live server.

@debba, please re-review the updated architecture against your outstanding review. I am leaving that blocking review for you to resolve before merge. The registry rollout and matching plugin implementations remain coordinated follow-ups, as documented in the guide.