Skip to content

Commit bcc756d

Browse files
fix(mcp): resolve saved-but-inactive connections in bridge (#132)
* fix(mcp): resolve saved-but-inactive connections in bridge MCP bridge resolve_connection only checked in-memory connections, so connections saved in .store.dat but not yet activated in the UI session failed with misleading errors. Reuse capabilities::sql::resolve_adapter, which reads the store in-process and connects — credentials never leave the app. McpPolicy authorization still runs upstream in invoke_with_policy. Also make get_connection_id errors actionable: distinguish 'no connection provided' (point to sqlkit__list_connections / Settings → MCP Bridge) from a malformed internal config. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * chore: sync Cargo.lock sqlkit version to 0.8.4 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix(mcp): remove get_store_value stub capability The handler always returned {"value": null} without reading the store — dead code that misleads agents. MCP should not expose internal app store data, so remove the capability and its registration. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * feat(mcp): split SQL write tools by risk level sqlkit__execute_query was a single Elevated capability carrying all SQL — INSERT/UPDATE/DELETE/DDL ran with no Destructive gate, and parallel_ok blocked concurrent reads. Split by statement class so McpPolicy can gate each risk level: - sqlkit__execute_query: read-only (SELECT/SHOW/EXPLAIN) → Safe, parallel - sqlkit__execute_write: INSERT/UPDATE/MERGE → Elevated - sqlkit__execute_delete: DELETE/TRUNCATE → Destructive - sqlkit__execute_ddl: CREATE/ALTER/DROP → Destructive classify_sql parses with sqlparser (dialect-aware). execute_query now rejects write/delete/ddl statements with actionable guidance pointing to the split tools. New module sql_write.rs shares resolve_adapter and execute_on_adapter from sql.rs. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> --------- Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
1 parent f3972bf commit bcc756d

7 files changed

Lines changed: 492 additions & 61 deletions

File tree

src-tauri/Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src-tauri/src/capabilities/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ pub mod commands;
22
pub mod mysql;
33
pub mod postgres;
44
pub mod sql;
5+
pub mod sql_write;
56
pub mod sqlite;
67
pub mod sqlkit;
78
pub mod sqlserver;

src-tauri/src/capabilities/sql.rs

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ fn app_handle() -> AppHandle {
2020
.clone()
2121
}
2222

23-
async fn resolve_adapter(connection_id: &str) -> Result<ActiveConnection, String> {
23+
pub(crate) async fn resolve_adapter(connection_id: &str) -> Result<ActiveConnection, String> {
2424
let app = app_handle();
2525

2626
// Check if already connected
@@ -72,7 +72,7 @@ async fn resolve_adapter(connection_id: &str) -> Result<ActiveConnection, String
7272
Ok(adapter)
7373
}
7474

75-
async fn execute_on_adapter(adapter: &ActiveConnection, sql: &str) -> Result<QueryResult, String> {
75+
pub(crate) async fn execute_on_adapter(adapter: &ActiveConnection, sql: &str) -> Result<QueryResult, String> {
7676
match adapter {
7777
ActiveConnection::Postgres(a) => a
7878
.lock()
@@ -131,12 +131,19 @@ async fn execute_on_adapter(adapter: &ActiveConnection, sql: &str) -> Result<Que
131131
}
132132
}
133133

134-
fn get_connection_id(config: Option<&Value>) -> Result<String, String> {
135-
config
136-
.and_then(|c| c.get("connectionId"))
137-
.and_then(|v| v.as_str())
138-
.map(|s| s.to_string())
139-
.ok_or_else(|| "Missing connectionId in connection config".to_string())
134+
pub(crate) fn get_connection_id(config: Option<&Value>) -> Result<String, String> {
135+
match config {
136+
None => Err(
137+
"No connection was provided for this tool call. Supply a connection_id \
138+
(list them with sqlkit__list_connections) or enable it in Settings → MCP Bridge"
139+
.to_string(),
140+
),
141+
Some(c) => c
142+
.get("connectionId")
143+
.and_then(|v| v.as_str())
144+
.map(|s| s.to_string())
145+
.ok_or_else(|| "Connection config is missing the 'connectionId' field".to_string()),
146+
}
140147
}
141148

142149
// ---------------------------------------------------------------------------
@@ -169,6 +176,11 @@ impl CapabilityHandler for ExecuteQueryHandler {
169176
.ok_or_else(|| "Missing 'sql' argument".to_string())?;
170177
let adapter = resolve_adapter(&conn_id).await?;
171178

179+
// Read-only guard: reject write/delete/ddl statements with actionable
180+
// guidance so agents migrate to the split write tools.
181+
let db_type = crate::capabilities::sql_write::adapter_db_type(&adapter);
182+
crate::capabilities::sql_write::ensure_read_only(&db_type, sql)?;
183+
172184
// Check connection quality and warn the AI agent about flaky connections
173185
let mut guardian_warning: Option<String> = None;
174186
if let Some(guardian) = crate::GUARDIAN.get() {
@@ -676,17 +688,17 @@ fn connection_id_schema() -> Value {
676688
pub fn register_sql_tools(reg: &mut CapabilityRegistry) {
677689
reg.register(Capability {
678690
name: "sqlkit__execute_query",
679-
description: "Execute an arbitrary SQL query and return the result set. Supports SELECT, INSERT, UPDATE, DELETE, DDL, and any other SQL statement.",
691+
description: "Execute a read-only SQL query (SELECT, SHOW, EXPLAIN) and return the result set. Write statements (INSERT/UPDATE/MERGE), deletes (DELETE/TRUNCATE), and DDL (CREATE/ALTER/DROP) are rejected — use sqlkit__execute_write, sqlkit__execute_delete, or sqlkit__execute_ddl respectively.",
680692
handler: Arc::new(ExecuteQueryHandler),
681693
input_schema: json!({"type": "object", "properties": {
682694
"connection_id": connection_id_schema(),
683-
"sql": {"type": "string", "description": "The SQL query to execute"}
695+
"sql": {"type": "string", "description": "The read-only SQL query (SELECT/SHOW/EXPLAIN)"}
684696
}, "required": ["connection_id", "sql"]}),
685-
risk_level: RiskLevel::Elevated,
697+
risk_level: RiskLevel::Safe,
686698
required_permission: "read",
687699
source_kind: SourceKind::SqlDatabase,
688700
tags: &["agent"],
689-
parallel_ok: false,
701+
parallel_ok: true,
690702
});
691703

692704
reg.register(Capability {
@@ -777,3 +789,27 @@ pub fn register_sql_tools(reg: &mut CapabilityRegistry) {
777789
parallel_ok: true,
778790
});
779791
}
792+
793+
#[cfg(test)]
794+
mod tests {
795+
use super::*;
796+
797+
#[test]
798+
fn get_connection_id_returns_id_when_present() {
799+
let config = json!({ "connectionId": "conn-1" });
800+
assert_eq!(get_connection_id(Some(&config)), Ok("conn-1".to_string()));
801+
}
802+
803+
#[test]
804+
fn get_connection_id_explains_missing_config() {
805+
let err = get_connection_id(None).unwrap_err();
806+
assert!(err.contains("connection_id"), "got: {}", err);
807+
assert!(err.contains("Settings → MCP Bridge"), "got: {}", err);
808+
}
809+
810+
#[test]
811+
fn get_connection_id_rejects_config_without_field() {
812+
let err = get_connection_id(Some(&json!({ "host": "x" }))).unwrap_err();
813+
assert!(err.contains("connectionId"), "got: {}", err);
814+
}
815+
}

0 commit comments

Comments
 (0)