From 1ffdda1b76e165636f112ee671f867bbdcc67286 Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Mon, 20 Jul 2026 13:02:43 -0400 Subject: [PATCH 1/9] feat(mcp): bind connection at server registration via --project-path/--name/--dsn-env (REF-297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decouple `plenum mcp` connection resolution from the launcher's cwd. The long-lived MCP server is started from arbitrary directories (Claude Desktop, editors, daemons), so cwd is not a meaningful signal for which project's config to use. New `plenum mcp` flags, captured into an McpBinding at startup: - --project-path : pins config resolution to that project. Loads the project's LOCAL `.plenum/config.json` via the new config::resolve_connection_in_project (not CWD's local config), plus global config keyed by that path — so it works regardless of launcher cwd. - --name : selects a named connection within the resolved project. - --dsn-env : reads the DSN from ONLY the named env var at connection time. No ambient credential pickup (DATABASE_URL, PGPASSWORD, ...). Per-call tool arguments still take precedence; the binding only supplies the default when a call omits its own connection selectors. Safety / correctness: - --dsn-env conflicts with --project-path/--name (rejected at the CLI via clap and defensively in McpBinding::new). - When no binding flag is given and cwd has no config, tool calls fail with a structured error naming exactly which flags to add — never a silent fallback. Tests: - 9 unit tests (src/mcp.rs) covering binding construction/conflicts and credential-sourcing safety (named-var-only, no ambient fallback). - 9 integration tests (tests/mcp_bound_connection.rs) driving the compiled binary over stdio: project-path pinning from local and global config, name selection, dsn-env resolution, no-ambient-DATABASE_URL, structured no-config error, and CLI conflict rejection. Verified: cargo build --all-targets, cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (all offline suites), and scripts/test-live.sh (MySQL 8.0/8.4, PostgreSQL 16) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config/mod.rs | 130 ++++++++---- src/lib.rs | 3 +- src/main.rs | 38 +++- src/mcp.rs | 276 ++++++++++++++++++++++--- tests/mcp_bound_connection.rs | 373 ++++++++++++++++++++++++++++++++++ 5 files changed, 749 insertions(+), 71 deletions(-) create mode 100644 tests/mcp_bound_connection.rs diff --git a/src/config/mod.rs b/src/config/mod.rs index 0822d83..09745b9 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -457,55 +457,88 @@ pub fn load_with_precedence() -> Result { } } -/// Resolve a connection by project path and name -/// -/// Searches in merged view (both local and global configs). -/// Returns an error if the connection is not found. -/// Returns a tuple of (`ConnectionConfig`, `is_readonly`). +/// Resolve a connection for a specific project directory. /// -/// # Parameters -/// - `project_path`: Optional project path. If None, uses current working directory. -/// - `name`: Optional connection name. If None, uses the project's default connection. -pub fn resolve_connection( - project_path: Option<&str>, +/// Unlike [`resolve_connection`], this loads the local config from +/// `/.plenum/config.json` rather than from the current working +/// directory. Required when `plenum mcp --project-path ` is invoked from +/// a different directory than the project (e.g. MCP clients that start the +/// server from `$HOME`). +pub fn resolve_connection_in_project( + project_path: &str, name: Option<&str>, ) -> Result<(ConnectionConfig, bool)> { - // Determine project path (use provided or get current) - let path = match project_path { - Some(p) => p.to_string(), - None => get_current_project_path()?, - }; + // Load local config from the project directory itself, not from CWD. + let local_path = PathBuf::from(project_path).join(".plenum").join("config.json"); + let global_path = global_config_path()?; + let registry = load_with_explicit_local(&local_path, &global_path)?; - // Search in merged view (both local and global) - let registry = load_with_precedence()?; + // The project key stored by load_registry is the canonicalized parent of + // `.plenum/config.json`. Canonicalize the given path to match. + let canon = PathBuf::from(project_path) + .canonicalize() + .ok() + .and_then(|p| p.to_str().map(str::to_string)) + .unwrap_or_else(|| project_path.to_string()); - // Look up project in registry - let project = registry.projects.get(&path).ok_or_else(|| { + resolve_from_registry(®istry, &canon, name) +} + +/// Load a merged registry using an explicit local config path instead of CWD. +/// +/// Merging semantics are identical to [`load_with_precedence`]: local wins for +/// the same project path, global provides everything else. +fn load_with_explicit_local(local_path: &Path, global_path: &Path) -> Result { + match (local_path.exists(), global_path.exists()) { + (false, false) => Ok(ConnectionRegistry::default()), + (true, false) => load_registry(local_path), + (false, true) => load_registry(global_path), + (true, true) => { + let global_registry = load_registry(global_path)?; + let local_registry = load_registry(local_path)?; + let mut merged = global_registry; + for (project_path, local_project) in local_registry.projects { + let project_entry = merged.projects.entry(project_path).or_default(); + for (conn_name, conn) in local_project.connections { + project_entry.connections.insert(conn_name, conn); + } + if local_project.default.is_some() { + project_entry.default = local_project.default; + } + } + Ok(merged) + } + } +} + +/// Look up a named (or default) connection from a registry for a given project path. +fn resolve_from_registry( + registry: &ConnectionRegistry, + path: &str, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + let project = registry.projects.get(path).ok_or_else(|| { PlenumError::config_error(format!( "No connections found for project path '{path}'. Run 'plenum connect' to create one." )) })?; - // Determine connection name (use provided or project's default) let conn_name = match name { Some(n) => n.to_string(), - None => { - // Use project's default - project - .default - .as_ref() - .ok_or_else(|| { - let available: Vec<_> = project.connections.keys().collect(); - PlenumError::config_error(format!( - "No default connection set for project '{path}'. Available connections: {available:?}. \ - Specify one with --name or set a default in the config." + None => project + .default + .as_ref() + .ok_or_else(|| { + let available: Vec<_> = project.connections.keys().collect(); + PlenumError::config_error(format!( + "No default connection set for project '{path}'. \ + Available connections: {available:?}. \ + Specify one with --name or set a default in the config." )) - })? - .clone() - } + })? + .clone(), }; - // Look up connection by name within project let stored = project.connections.get(&conn_name).ok_or_else(|| { let available: Vec<_> = project.connections.keys().collect(); let default_info = match &project.default { @@ -513,14 +546,39 @@ pub fn resolve_connection( None => String::new(), }; PlenumError::config_error(format!( - "Connection '{conn_name}' not found for project '{path}'. Available connections: {available:?}{default_info}" + "Connection '{conn_name}' not found for project '{path}'. \ + Available connections: {available:?}{default_info}" )) })?; - // Resolve environment variables and get readonly flag stored.resolve() } +/// Resolve a connection by project path and name +/// +/// Searches in merged view (both local and global configs). +/// Returns an error if the connection is not found. +/// Returns a tuple of (`ConnectionConfig`, `is_readonly`). +/// +/// # Parameters +/// - `project_path`: Optional project path. If None, uses current working directory. +/// - `name`: Optional connection name. If None, uses the project's default connection. +pub fn resolve_connection( + project_path: Option<&str>, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + // Determine project path (use provided or get current) + let path = match project_path { + Some(p) => p.to_string(), + None => get_current_project_path()?, + }; + + // Search in merged view (both local and global) + let registry = load_with_precedence()?; + + resolve_from_registry(®istry, &path, name) +} + /// Save a connection to a config file /// /// # Parameters diff --git a/src/lib.rs b/src/lib.rs index b800d32..40a9027 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,7 +42,8 @@ pub mod output; // JSON output envelopes (Phase 1.2) // MCP server (Phase 7) - M pub use capability::validate_query; pub use config::{ list_connections, list_connections_for_project, list_connections_raw, resolve_connection, - save_connection, ConfigLocation, ConnectionRegistry, KeychainEntry, StoredConnection, + resolve_connection_in_project, save_connection, ConfigLocation, ConnectionRegistry, + KeychainEntry, StoredConnection, }; pub use dsn::{parse_dsn, redact_dsn}; pub use engine::{ diff --git a/src/main.rs b/src/main.rs index ec2d7c0..66a17fa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -371,7 +371,20 @@ enum Commands { /// Start MCP server (hidden from help, for AI agent integration) #[command(hide = true)] - Mcp, + Mcp { + /// Pin config resolution to this project path (overrides the launcher's cwd) + #[arg(long)] + project_path: Option, + + /// Pin to a named connection within the resolved project + #[arg(long)] + name: Option, + + /// Read the DSN from this named environment variable at connection time. + /// Only the named variable is read — never an ambient fallback. + #[arg(long, conflicts_with_all = ["project_path", "name"])] + dsn_env: Option, + }, } #[tokio::main] @@ -571,7 +584,9 @@ async fn main() { ) .await } - Some(Commands::Mcp) => handle_mcp().await, + Some(Commands::Mcp { project_path, name, dsn_env }) => { + handle_mcp(project_path, name, dsn_env).await + } None => { // No subcommand provided let error_envelope = ErrorEnvelope::new( @@ -1787,10 +1802,25 @@ async fn handle_query( } #[allow(clippy::future_not_send)] -async fn handle_mcp() -> std::result::Result<(), i32> { +async fn handle_mcp( + project_path: Option, + name: Option, + dsn_env: Option, +) -> std::result::Result<(), i32> { // Phase 7: MCP server using manual JSON-RPC 2.0 implementation // Follows the proven pattern from reflex-search (no unstable rmcp dependency) - match plenum::mcp::serve().await { + // + // Bind the connection source at server registration so resolution is + // decoupled from the launcher's cwd (REF-297). + let binding = match plenum::mcp::McpBinding::new(project_path, name, dsn_env) { + Ok(binding) => binding, + Err(e) => { + eprintln!("MCP server error: {e}"); + return Err(1); + } + }; + + match plenum::mcp::serve(binding).await { Ok(()) => Ok(()), Err(e) => { // MCP server errors go to stderr (not stdout, which is for JSON-RPC) diff --git a/src/mcp.rs b/src/mcp.rs index 449395d..056e04a 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -58,6 +58,57 @@ use crate::engine::postgres::PostgresEngine; #[cfg(feature = "sqlite")] use crate::engine::sqlite::SqliteEngine; +// ============================================================================ +// Connection Binding (captured at server registration) +// ============================================================================ + +/// Connection binding captured from `plenum mcp` flags at server registration. +/// +/// The MCP server is long-lived and its launcher's cwd is not a meaningful +/// signal (Claude Desktop, editors, and daemons start it from arbitrary +/// directories). This binding pins config resolution to an explicit source so +/// resolution never silently depends on cwd. +/// +/// Per-call tool arguments (`dsn`, `connection`, `engine`, ...) still take +/// precedence; the binding only supplies the default a call omits. +#[derive(Debug, Clone, Default)] +pub struct McpBinding { + /// `--project-path`: pins config resolution to this project path. + project_path: Option, + /// `--name`: pins to a named connection within the resolved project. + name: Option, + /// `--dsn-env`: reads the DSN from this named environment variable at + /// connection time. Only the named variable is ever read — never an ambient + /// fallback such as `DATABASE_URL` or `PGPASSWORD`. + dsn_env: Option, +} + +impl McpBinding { + /// Construct a binding from `plenum mcp` flags, rejecting conflicting + /// combinations. + /// + /// `--dsn-env` names a complete one-off connection source, so it cannot be + /// combined with the saved-config selectors `--project-path` / `--name`. + /// + /// # Errors + /// Returns an error when `--dsn-env` is combined with `--project-path` or + /// `--name`. + pub fn new( + project_path: Option, + name: Option, + dsn_env: Option, + ) -> Result { + if dsn_env.is_some() && (project_path.is_some() || name.is_some()) { + return Err(anyhow!( + "'--dsn-env' cannot be combined with '--project-path' or '--name': \ + a DSN environment variable is a complete connection source, \ + not a saved-config selector" + )); + } + Ok(Self { project_path, name, dsn_env }) + } +} + // ============================================================================ // JSON-RPC 2.0 Structures // ============================================================================ @@ -156,7 +207,7 @@ impl CallToolResult { /// /// Returns an error if stdio communication fails or if there's a fatal error. #[allow(clippy::future_not_send)] -pub async fn serve() -> Result<()> { +pub async fn serve(binding: McpBinding) -> Result<()> { let stdin = io::stdin(); let reader = stdin.lock(); let mut stdout = io::stdout(); @@ -183,7 +234,7 @@ pub async fn serve() -> Result<()> { continue; } - let response = handle_request(request).await; + let response = handle_request(request, &binding).await; let response_json = serde_json::to_string(&response)?; writeln!(stdout, "{response_json}")?; stdout.flush()?; @@ -195,11 +246,11 @@ pub async fn serve() -> Result<()> { /// Handle a JSON-RPC request /// /// Routes the request to the appropriate handler based on the method name. -async fn handle_request(request: JsonRpcRequest) -> JsonRpcResponse { +async fn handle_request(request: JsonRpcRequest, binding: &McpBinding) -> JsonRpcResponse { let result = match request.method.as_str() { "initialize" => handle_initialize(request.params), "tools/list" => handle_list_tools(), - "tools/call" => handle_call_tool(request.params).await, + "tools/call" => handle_call_tool(request.params, binding).await, _ => Err(anyhow!("Unknown method: {}", request.method)), }; @@ -480,15 +531,15 @@ fn handle_list_tools() -> Result { /// Handle tools/call request /// /// Routes the tool call to the appropriate tool implementation. -async fn handle_call_tool(params: Option) -> Result { +async fn handle_call_tool(params: Option, binding: &McpBinding) -> Result { let params = params.ok_or_else(|| anyhow!("Missing params"))?; let name = params["name"].as_str().ok_or_else(|| anyhow!("Missing tool name"))?; let arguments = ¶ms["arguments"]; match name { - "connect" => tool_connect(arguments).await, - "introspect" => tool_introspect(arguments).await, - "query" => tool_query(arguments).await, + "connect" => tool_connect(arguments, binding).await, + "introspect" => tool_introspect(arguments, binding).await, + "query" => tool_query(arguments, binding).await, _ => Err(anyhow!("Unknown tool: {name}")), } } @@ -501,8 +552,8 @@ async fn handle_call_tool(params: Option) -> Result { /// /// Tests a database connection and returns server metadata. /// Stateless and read-only — no config is saved or mutated. -async fn tool_connect(args: &Value) -> Result { - let (config, _is_readonly) = resolve_connection_from_args(args)?; +async fn tool_connect(args: &Value, binding: &McpBinding) -> Result { + let (config, _is_readonly) = resolve_connection_from_args(args, binding)?; let connection_info = match config.engine { #[cfg(feature = "sqlite")] @@ -549,9 +600,9 @@ async fn tool_connect(args: &Value) -> Result { /// /// Introspects database schema and returns table/column information. /// When `diff_against` is provided, computes a structural schema diff instead. -async fn tool_introspect(args: &Value) -> Result { +async fn tool_introspect(args: &Value, binding: &McpBinding) -> Result { // Resolve base connection config - let (config, _is_readonly) = resolve_connection_from_args(args)?; + let (config, _is_readonly) = resolve_connection_from_args(args, binding)?; // Get optional database and schema modifiers (shared by both paths) let database = args.get("target_database").and_then(|v| v.as_str()); @@ -703,12 +754,12 @@ fn parse_introspect_operation(args: &Value) -> Result Result { +async fn tool_query(args: &Value, binding: &McpBinding) -> Result { // Extract SQL let sql = args["sql"].as_str().ok_or_else(|| anyhow!("Missing required field: sql"))?; // Resolve connection config - let (mut config, _is_readonly) = resolve_connection_from_args(args)?; + let (mut config, _is_readonly) = resolve_connection_from_args(args, binding)?; // Apply target_database override if provided if let Some(target_db) = args.get("target_database").and_then(|v| v.as_str()) { @@ -823,16 +874,21 @@ fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result Result<(ConnectionConfig, bool)> { +fn resolve_connection_from_args( + args: &Value, + binding: &McpBinding, +) -> Result<(ConnectionConfig, bool)> { // Scenario 0: DSN one-off URL (mutually exclusive with connection and engine) if let Some(dsn_str) = args.get("dsn").and_then(|v| v.as_str()) { if args.get("connection").and_then(|v| v.as_str()).is_some() { @@ -853,9 +909,14 @@ fn resolve_connection_from_args(args: &Value) -> Result<(ConnectionConfig, bool) if has_connection { let connection = args["connection"].as_str().unwrap(); - // Use None for project_path (defaults to current directory) - let (mut config, is_readonly) = crate::resolve_connection(None, Some(connection)) - .map_err(|e| anyhow!("Failed to resolve connection '{connection}': {e}"))?; + // When --project-path is bound, look up the named connection inside that + // project's local config (not CWD). Without a bound path, fall back to CWD. + let (mut config, is_readonly) = if let Some(path) = &binding.project_path { + crate::config::resolve_connection_in_project(path, Some(connection)) + } else { + crate::resolve_connection(None, Some(connection)) + } + .map_err(|e| anyhow!("Failed to resolve connection '{connection}': {e}"))?; // Apply overrides if let Some(eng) = args.get("engine").and_then(|v| v.as_str()) { @@ -896,16 +957,58 @@ fn resolve_connection_from_args(args: &Value) -> Result<(ConnectionConfig, bool) return Ok((config, false)); // Explicit connections are never readonly } - // Scenario 3: Auto-resolve default connection for current project - // Use None for both project_path (current directory) and connection_name (use default) - let (config, is_readonly) = crate::resolve_connection(None, None).map_err(|e| { - anyhow!( - "No connection or engine specified, and failed to auto-resolve default connection: {e}. \ - Either provide 'connection' (named), 'engine' (explicit), or ensure a default connection exists for this project." - ) - })?; + // Scenario 3: Fall back to the server-registration binding. + resolve_from_binding(binding) +} + +/// Resolve a connection from the server-registration binding alone (no per-call +/// connection selectors were provided). +/// +/// Priority: `--dsn-env` (explicit env-sourced DSN) → `--project-path` / `--name` +/// (or the cwd default when neither is bound). When nothing resolves and no +/// binding flags were given, returns a structured error naming exactly which +/// flags to add — never a silent fallback. +fn resolve_from_binding(binding: &McpBinding) -> Result<(ConnectionConfig, bool)> { + // `--dsn-env`: read the DSN from ONLY the named variable. Plenum never reads + // an ambient credential source (DATABASE_URL, PGPASSWORD, ...) that the + // caller did not explicitly name. + if let Some(var) = &binding.dsn_env { + let dsn = std::env::var(var).map_err(|_| { + anyhow!( + "--dsn-env variable '{var}' is not set in the environment. \ + Plenum reads only the named variable and never falls back to \ + other environment variables." + ) + })?; + let config = + parse_dsn(&dsn).map_err(|e| anyhow!("{} (from --dsn-env {})", e.message(), var))?; + return Ok((config, false)); + } + + // `--project-path`: load the local config from the project directory itself, + // not from the launcher's cwd — that's the whole point of the flag. + if let Some(path) = &binding.project_path { + return crate::config::resolve_connection_in_project(path, binding.name.as_deref()) + .map_err(|e| anyhow!("Failed to resolve bound connection: {e}")); + } - Ok((config, is_readonly)) + // `--name` only (no --project-path): use cwd as project, pick the named connection. + // No flags at all: use cwd + project default. + crate::resolve_connection(None, binding.name.as_deref()).map_err(|e| { + if binding.name.is_none() { + // Nothing was bound and cwd has no usable config — name the flags exactly. + anyhow!( + "No connection could be resolved: no connection selectors were passed to this \ + tool call and 'plenum mcp' was started without a connection binding. \ + Restart the server with one of: '--project-path ' to pin the project, \ + '--name ' to select a saved connection, or \ + '--dsn-env ' to read a DSN from a named environment variable. \ + Underlying error: {e}" + ) + } else { + anyhow!("Failed to resolve bound connection: {e}") + } + }) } /// Validate database connection @@ -999,3 +1102,116 @@ async fn execute_query( } } } + +// ============================================================================ +// Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + + // ── McpBinding construction / conflict rules ──────────────────────────── + + #[test] + fn binding_rejects_dsn_env_with_project_path() { + let err = McpBinding::new(Some("/p".to_string()), None, Some("PLENUM_DSN".to_string())) + .unwrap_err() + .to_string(); + assert!(err.contains("--dsn-env"), "error should name --dsn-env: {err}"); + assert!(err.contains("--project-path"), "error should name --project-path: {err}"); + } + + #[test] + fn binding_rejects_dsn_env_with_name() { + let err = McpBinding::new(None, Some("prod".to_string()), Some("PLENUM_DSN".to_string())) + .unwrap_err() + .to_string(); + assert!(err.contains("--dsn-env"), "error should name --dsn-env: {err}"); + assert!(err.contains("--name"), "error should name --name: {err}"); + } + + #[test] + fn binding_allows_project_path_and_name_together() { + let binding = + McpBinding::new(Some("/p".to_string()), Some("prod".to_string()), None).unwrap(); + assert_eq!(binding.project_path.as_deref(), Some("/p")); + assert_eq!(binding.name.as_deref(), Some("prod")); + } + + #[test] + fn binding_allows_dsn_env_alone_and_empty() { + assert!(McpBinding::new(None, None, Some("PLENUM_DSN".to_string())).is_ok()); + assert!(McpBinding::default().dsn_env.is_none()); + } + + // ── --dsn-env resolution (credential-sourcing safety) ─────────────────── + + #[test] + fn dsn_env_reads_only_the_named_variable() { + // A named var holding a valid DSN resolves; nothing else is consulted. + std::env::set_var("PLENUM_TEST_DSN_ENV_OK", "sqlite::memory:"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_OK".to_string())).unwrap(); + let (config, is_readonly) = resolve_from_binding(&binding).unwrap(); + assert_eq!(config.engine, DatabaseType::SQLite); + assert!(!is_readonly); + std::env::remove_var("PLENUM_TEST_DSN_ENV_OK"); + } + + #[test] + fn dsn_env_never_falls_back_to_ambient_credentials() { + // Set a plausible ambient credential source, then point --dsn-env at a + // DIFFERENT, unset variable. Plenum must NOT pick up the ambient one. + std::env::set_var("PLENUM_TEST_AMBIENT_DSN", "postgres://u:p@host:5432/db"); + std::env::remove_var("PLENUM_TEST_DSN_ENV_UNSET"); + + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_UNSET".to_string())).unwrap(); + let err = resolve_from_binding(&binding).unwrap_err().to_string(); + + assert!( + err.contains("PLENUM_TEST_DSN_ENV_UNSET"), + "error should name the missing var: {err}" + ); + assert!( + !err.contains("PLENUM_TEST_AMBIENT_DSN"), + "must not reference the ambient var: {err}" + ); + assert!(!err.contains("host"), "must not leak the ambient DSN value: {err}"); + std::env::remove_var("PLENUM_TEST_AMBIENT_DSN"); + } + + #[test] + fn dsn_env_with_invalid_dsn_is_rejected() { + std::env::set_var("PLENUM_TEST_DSN_ENV_BAD", "not-a-valid-dsn"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_BAD".to_string())).unwrap(); + let err = resolve_from_binding(&binding).unwrap_err().to_string(); + assert!(err.contains("PLENUM_TEST_DSN_ENV_BAD"), "error names the var: {err}"); + std::env::remove_var("PLENUM_TEST_DSN_ENV_BAD"); + } + + // ── Per-call args still take precedence over the binding ──────────────── + + #[test] + fn per_call_dsn_overrides_binding_and_is_used_directly() { + // Even with a --dsn-env binding, an explicit per-call `dsn` wins. + std::env::set_var("PLENUM_TEST_DSN_ENV_BINDING", "sqlite::memory:"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_BINDING".to_string())).unwrap(); + let args = serde_json::json!({ "dsn": "sqlite:/tmp/explicit.db" }); + let (config, _) = resolve_connection_from_args(&args, &binding).unwrap(); + assert_eq!(config.engine, DatabaseType::SQLite); + assert_eq!(config.file.as_deref(), Some(std::path::Path::new("/tmp/explicit.db"))); + std::env::remove_var("PLENUM_TEST_DSN_ENV_BINDING"); + } + + #[test] + fn per_call_dsn_and_connection_are_mutually_exclusive() { + let args = serde_json::json!({ "dsn": "sqlite::memory:", "connection": "prod" }); + let err = + resolve_connection_from_args(&args, &McpBinding::default()).unwrap_err().to_string(); + assert!(err.contains("mutually exclusive"), "got: {err}"); + } +} diff --git a/tests/mcp_bound_connection.rs b/tests/mcp_bound_connection.rs new file mode 100644 index 0000000..1be7ff4 --- /dev/null +++ b/tests/mcp_bound_connection.rs @@ -0,0 +1,373 @@ +//! End-to-end coverage for the `plenum mcp` connection-binding flags (REF-297): +//! `--project-path`, `--name`, and `--dsn-env`. +//! +//! The MCP server is decoupled from the launcher's cwd: the binding captured at +//! `plenum mcp` startup supplies the default connection source for tool calls +//! that omit their own connection selectors. Every test drives the compiled +//! `plenum mcp` binary over stdio. +//! +//! Contract notes exercised here: +//! - Errors from a tool call surface as JSON-RPC error responses (the server's +//! existing error surface); the `error.message` is what callers read. +//! - `--dsn-env` reads the DSN at connection time and consults ONLY the named +//! variable — never an ambient source such as `DATABASE_URL`. +//! - `--project-path` re-keys config resolution; global config is keyed by +//! project path, so a global connection resolves regardless of cwd. + +use serde_json::{json, Value}; +use std::io::{BufRead, BufReader, Write}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +const TIMEOUT: Duration = Duration::from_secs(10); + +// ─── helpers ──────────────────────────────────────────────────────────────── + +/// Create a unique, empty scratch directory with no `.plenum` config. +fn scratch(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("plenum-bind-{}-{tag}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create scratch dir"); + dir +} + +/// Spawn `plenum mcp ` with the given env overrides and cwd. +/// +/// `xdg_config_home` isolates global-config lookup to a per-test directory so +/// the developer's real `~/.config/plenum` never influences the result. +fn spawn_mcp( + extra_args: &[&str], + envs: &[(&str, &str)], + cwd: &Path, + xdg_config_home: &Path, +) -> Child { + let bin = env!("CARGO_BIN_EXE_plenum"); + let mut cmd = Command::new(bin); + cmd.arg("mcp").args(extra_args); + cmd.env("XDG_CONFIG_HOME", xdg_config_home); + for (k, v) in envs { + cmd.env(k, v); + } + cmd.current_dir(cwd) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn plenum mcp") +} + +fn read_line_json(reader: &mut BufReader) -> Option { + let deadline = Instant::now() + TIMEOUT; + while Instant::now() < deadline { + let mut buf = String::new(); + match reader.read_line(&mut buf) { + Ok(0) | Err(_) => return None, + Ok(_) => { + let t = buf.trim(); + if t.is_empty() { + continue; + } + return serde_json::from_str(t).ok(); + } + } + } + None +} + +/// Drive the handshake and a single `connect` tool call (no connection args), +/// returning the JSON-RPC response for the call. +fn handshake_then_connect(child: &mut Child) -> Value { + let mut stdin = child.stdin.take().expect("stdin"); + let stdout = child.stdout.take().expect("stdout"); + let mut reader = BufReader::new(stdout); + + let init = json!({ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { "protocolVersion": "2024-11-05", "capabilities": {}, + "clientInfo": { "name": "test", "version": "0" } } + }); + let notif = json!({ "jsonrpc": "2.0", "method": "notifications/initialized" }); + let call = json!({ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": { "name": "connect", "arguments": {} } + }); + + writeln!(stdin, "{init}").unwrap(); + writeln!(stdin, "{notif}").unwrap(); + writeln!(stdin, "{call}").unwrap(); + stdin.flush().unwrap(); + drop(stdin); + + let mut response = None; + while let Some(v) = read_line_json(&mut reader) { + if v.get("id") == Some(&json!(2)) { + response = Some(v); + break; + } + } + response.expect("no response for connect tool call") +} + +/// Create an on-disk `SQLite` database at `path`. Plenum opens connections +/// read-only, so the file must already exist for `connect` to succeed. +fn create_sqlite_db(path: &Path) { + let conn = rusqlite::Connection::open(path).expect("create sqlite db"); + conn.execute_batch("CREATE TABLE IF NOT EXISTS probe (id INTEGER PRIMARY KEY)") + .expect("seed sqlite db"); +} + +/// Write a global config registry (`{ "projects": { : {...} } }`) into +/// `/plenum/connections.json`. +fn write_global_config(xdg: &Path, registry: &Value) { + let dir = xdg.join("plenum"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("connections.json"), registry.to_string()).unwrap(); +} + +fn error_message(resp: &Value) -> String { + resp["error"]["message"] + .as_str() + .unwrap_or_else(|| panic!("expected a JSON-RPC error response, got: {resp}")) + .to_string() +} + +fn assert_no_error(resp: &Value) { + assert!( + resp.get("error").is_none(), + "expected a successful tool result, got JSON-RPC error: {:?}", + resp.get("error") + ); +} + +// ─── Criterion 4: no binding + no config → structured error naming the flags ── + +#[test] +fn no_binding_and_no_config_names_the_required_flags() { + let cwd = scratch("noflags"); + let xdg = scratch("noflags-xdg"); // empty → no global config + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("--project-path"), "must name --project-path: {msg}"); + assert!(msg.contains("--name"), "must name --name: {msg}"); + assert!(msg.contains("--dsn-env"), "must name --dsn-env: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Credential safety: no ambient pickup of DATABASE_URL ───────────────────── + +#[test] +fn database_url_is_never_used_as_an_ambient_source() { + let cwd = scratch("ambient"); + let xdg = scratch("ambient-xdg"); + // A valid ambient DSN is present but no --dsn-env flag names it. + let mut child = spawn_mcp(&[], &[("DATABASE_URL", "sqlite::memory:")], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + // Must fail (not silently resolve from DATABASE_URL), and the error must not + // reference the ambient variable it was never told to read. + let msg = error_message(&resp); + assert!(!msg.contains("DATABASE_URL"), "must not reference the ambient var: {msg}"); + assert!(msg.contains("--dsn-env"), "should point the operator at the flags: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 3: --dsn-env resolves the DSN from ONLY the named variable ───── + +#[test] +fn dsn_env_resolves_connection_from_named_variable() { + let cwd = scratch("dsnenv-ok"); + let xdg = scratch("dsnenv-ok-xdg"); + let db = cwd.join("app.db"); + create_sqlite_db(&db); + let dsn = format!("sqlite:{}", db.display()); + + let mut child = + spawn_mcp(&["--dsn-env", "PLENUM_ITEST_DSN"], &[("PLENUM_ITEST_DSN", &dsn)], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + let content = resp["result"]["content"][0]["text"].as_str().unwrap_or(""); + assert!(!content.is_empty(), "expected ConnectionInfo content, got empty: {resp}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn dsn_env_unset_variable_errors_and_names_the_variable() { + let cwd = scratch("dsnenv-unset"); + let xdg = scratch("dsnenv-unset-xdg"); + // The var is deliberately not provided to the child. + let mut child = spawn_mcp(&["--dsn-env", "PLENUM_ITEST_UNSET"], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("PLENUM_ITEST_UNSET"), "error must name the missing var: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 1: --project-path pins resolution regardless of launcher cwd ─── + +#[test] +fn project_path_pins_resolution_regardless_of_cwd() { + let cwd = scratch("projpath-cwd"); // empty: no local config here + let xdg = scratch("projpath-xdg"); + let db = xdg.join("proj.db"); + create_sqlite_db(&db); + let proj = format!("/plenum-itest-proj-{}", std::process::id()); + + // Global config keyed by the project path — resolvable from any cwd. + write_global_config( + &xdg, + &json!({ + "projects": { + proj.clone(): { + "connections": { + "default": { "engine": "sqlite", "file": db.to_str().unwrap() } + }, + "default": "default" + } + } + }), + ); + + let mut child = spawn_mcp(&["--project-path", &proj], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn project_path_loads_local_config_from_the_project_directory() { + // The core of `resolve_connection_in_project`: a LOCAL `.plenum/config.json` + // living inside the pinned project is loaded even when the server is launched + // from an unrelated cwd with no config of its own. + let proj = scratch("localcfg-proj"); + let launch_cwd = scratch("localcfg-launch"); // different, config-less cwd + let xdg = scratch("localcfg-xdg"); // empty global config + let db = proj.join("app.db"); + create_sqlite_db(&db); + + let plenum_dir = proj.join(".plenum"); + std::fs::create_dir_all(&plenum_dir).unwrap(); + std::fs::write( + plenum_dir.join("config.json"), + json!({ + "connections": { "default": { "engine": "sqlite", "file": db.to_str().unwrap() } }, + "default": "default" + }) + .to_string(), + ) + .unwrap(); + + let proj_str = proj.to_str().unwrap(); + let mut child = spawn_mcp(&["--project-path", proj_str], &[], &launch_cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&proj); + let _ = std::fs::remove_dir_all(&launch_cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criteria 2 + 5: --name selects a connection within the bound project ───── + +#[test] +fn name_selects_connection_and_combines_with_project_path() { + let cwd = scratch("name-cwd"); + let xdg = scratch("name-xdg"); + let good_db = xdg.join("prod.db"); + create_sqlite_db(&good_db); + let proj = format!("/plenum-itest-named-{}", std::process::id()); + + // Default ("staging") points at an unopenable path; "prod" is valid. + // Selecting --name prod must succeed, proving the name overrides the default. + write_global_config( + &xdg, + &json!({ + "projects": { + proj.clone(): { + "connections": { + "staging": { + "engine": "sqlite", + "file": "/plenum-nonexistent-dir/staging.db" + }, + "prod": { "engine": "sqlite", "file": good_db.to_str().unwrap() } + }, + "default": "staging" + } + } + }), + ); + + let mut child = spawn_mcp(&["--project-path", &proj, "--name", "prod"], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 5: --dsn-env conflicts with saved-config selectors ───────────── + +#[test] +fn dsn_env_conflicts_with_project_path_at_cli() { + let bin = env!("CARGO_BIN_EXE_plenum"); + let output = Command::new(bin) + .args(["mcp", "--dsn-env", "PLENUM_DSN", "--project-path", "/tmp/x"]) + .stdin(Stdio::null()) + .output() + .expect("run plenum mcp"); + + assert!(!output.status.success(), "conflicting flags must be rejected"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("dsn-env") || stderr.contains("dsn_env"), + "conflict error should mention --dsn-env: {stderr}" + ); +} + +#[test] +fn dsn_env_conflicts_with_name_at_cli() { + let bin = env!("CARGO_BIN_EXE_plenum"); + let output = Command::new(bin) + .args(["mcp", "--dsn-env", "PLENUM_DSN", "--name", "prod"]) + .stdin(Stdio::null()) + .output() + .expect("run plenum mcp"); + + assert!(!output.status.success(), "conflicting flags must be rejected"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("dsn-env") || stderr.contains("dsn_env"), + "conflict error should mention --dsn-env: {stderr}" + ); +} From c020773fe7961d52f740410ce19d0a2880e8c8db Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Mon, 20 Jul 2026 13:14:40 -0400 Subject: [PATCH 2/9] feat(mcp): save param + credential-reference fields on connect/introspect/query (REF-298) Restore the reference-based connect design so agents can persist connection config without ever handling plaintext secrets. - connect: reject inline plaintext `password` with a CAPABILITY_VIOLATION; add `save` ("local"|"global"), `project_path`, `name`, and the credential-reference fields `password_env`/`password_command`/`keychain_service`+`keychain_account`. Save mode persists by reference (never plaintext) via a new project-path-aware `save_connection_in_project` (local writes go to /.plenum/config.json, independent of the MCP server's cwd). - introspect & query: expose the same credential-reference fields for one-off explicit connections; the secret is resolved from the reference at connect time and never passes through the tool args. - Enforce reference invariants at the MCP boundary: at most one source, keychain_service/keychain_account required together. Tests: unit tests for parse_credential_refs, one-off reference resolution, and save-by-reference; config test for save_connection_in_project; E2E tests driving the real `plenum mcp` binary for inline-password rejection and reference-based local save. Full offline suite (350) + live MySQL/Postgres suites (56) green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config/mod.rs | 118 ++++++++++ src/mcp.rs | 391 ++++++++++++++++++++++++++++++++-- tests/mcp_bound_connection.rs | 148 +++++++++++++ 3 files changed, 644 insertions(+), 13 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index 09745b9..8fe7a06 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -645,6 +645,66 @@ pub fn save_connection( Ok(()) } +/// Save a connection scoped to an explicit project directory. +/// +/// Unlike [`save_connection`], the local config file is written to +/// `/.plenum/config.json` rather than `/.plenum/config.json`. +/// This is required by the MCP server, which is long-lived and started from an +/// arbitrary cwd (Claude Desktop, editors, daemons) — the project path must be +/// explicit, never inferred from cwd. +/// +/// Credentials are persisted only as references (`password_env`, +/// `password_command`, `keychain_entry`); no plaintext secret is ever written. +/// +/// Only one of `password_env`, `password_command`, or `keychain_entry` may be `Some`. +pub fn save_connection_in_project( + project_path: &str, + name: Option, + config: ConnectionConfig, + password_env: Option, + password_command: Option, + keychain_entry: Option, + location: ConfigLocation, +) -> Result<()> { + let conn_name = name.unwrap_or_else(|| "default".to_string()); + + // Local config lives under the project directory itself; global is per-user. + let config_path = match location { + ConfigLocation::Local => PathBuf::from(project_path).join(".plenum").join("config.json"), + ConfigLocation::Global => global_config_path()?, + }; + + // Registry key: `save_registry` (local path) and `load_registry` both key by + // the canonicalized project root. Match that so lookups line up. + let key = PathBuf::from(project_path) + .canonicalize() + .ok() + .and_then(|p| p.to_str().map(String::from)) + .unwrap_or_else(|| project_path.to_string()); + + let mut registry = if config_path.exists() { + load_registry(&config_path)? + } else { + ConnectionRegistry::default() + }; + + let project = registry.projects.entry(key).or_default(); + let is_first_connection = project.connections.is_empty(); + + project.connections.insert( + conn_name.clone(), + StoredConnection { config, password_env, password_command, keychain_entry, readonly: None }, + ); + + if is_first_connection { + project.default = Some(conn_name); + } + + save_registry(&config_path, ®istry)?; + + Ok(()) +} + /// List all available connections /// /// Returns a Vec of tuples: (`project_path`, `connection_name`, config) @@ -1654,4 +1714,62 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().message().contains("Only one password source")); } + + // --- save_connection_in_project tests --- + + #[test] + fn test_save_connection_in_project_local_writes_by_reference() { + // Local save must write to /.plenum/config.json and persist the + // credential reference (password_env) with NO plaintext password. + let project = std::env::temp_dir().join(format!( + "plenum-save-itest-{}-{:p}", + std::process::id(), + &0u8 + )); + let _ = fs::remove_dir_all(&project); + fs::create_dir_all(&project).unwrap(); + let project_str = project.to_str().unwrap().to_string(); + + let config = ConnectionConfig { + engine: DatabaseType::Postgres, + host: Some("db.example.com".to_string()), + port: Some(5432), + user: Some("agent".to_string()), + password: None, + database: Some("app".to_string()), + file: None, + tls: None, + }; + + save_connection_in_project( + &project_str, + Some("prod".to_string()), + config, + Some("DB_PASSWORD".to_string()), + None, + None, + ConfigLocation::Local, + ) + .unwrap(); + + let written = fs::read_to_string(project.join(".plenum").join("config.json")).unwrap(); + assert!(written.contains("\"password_env\""), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD"), "env var name stored: {written}"); + assert!(!written.contains("\"password\":"), "no plaintext password: {written}"); + // First connection becomes the default pointer. + assert!(written.contains("\"default\"")); + assert!(written.contains("prod")); + + // Round-trip: load the file back and confirm the connection is keyed by + // the canonicalized project root with its reference intact. + let canon = project.canonicalize().unwrap(); + let registry = load_registry(&project.join(".plenum").join("config.json")).unwrap(); + let stored_project = registry.projects.get(canon.to_str().unwrap()).unwrap(); + assert_eq!(stored_project.default.as_deref(), Some("prod")); + let stored = stored_project.connections.get("prod").unwrap(); + assert_eq!(stored.password_env.as_deref(), Some("DB_PASSWORD")); + assert!(stored.config.password.is_none()); + + let _ = fs::remove_dir_all(&project); + } } diff --git a/src/mcp.rs b/src/mcp.rs index 056e04a..eab2cfe 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -46,6 +46,7 @@ use serde_json::Value; use std::io::{self, BufRead, Write}; use std::path::PathBuf; +use crate::config::{ConfigLocation, KeychainEntry, StoredConnection}; use crate::{parse_dsn, redact_dsn, Capabilities, ConnectionConfig, DatabaseEngine, DatabaseType}; // Import database engines @@ -343,6 +344,22 @@ fn handle_list_tools() -> Result { "type": "string", "description": "DISCOURAGED: SQLite/DuckDB database file path. Only for one-off sqlite/duckdb explicit connections or as override. Prefer using saved connections." }, + "password_env": { + "type": "string", + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with password_command and keychain_service/keychain_account." + }, + "password_command": { + "type": "string", + "description": "Credential reference for one-off explicit connections: shell command whose stdout (trimmed) is the password. Mutually exclusive with password_env and keychain_service/keychain_account." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env and password_command." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain account name. Must be paired with keychain_service." + }, "list_databases": { "type": "boolean", "description": "Operation: List all databases. Returns {\"type\": \"database_list\", \"databases\": [\"db1\", \"db2\", ...]}. Requires wildcard connection (database=\"*\"). MySQL/Postgres only. Mutually exclusive with other operations." @@ -453,6 +470,22 @@ fn handle_list_tools() -> Result { "type": "string", "description": "DISCOURAGED: File path to SQLite/DuckDB database file. Only for one-off sqlite/duckdb explicit connections. Can be relative or absolute path. Example: './app.db', '/var/lib/data.duckdb'. Prefer using saved connections." }, + "password_env": { + "type": "string", + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with password_command and keychain_service/keychain_account." + }, + "password_command": { + "type": "string", + "description": "Credential reference for one-off explicit connections: shell command whose stdout (trimmed) is the password. Mutually exclusive with password_env and keychain_service/keychain_account." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env and password_command." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain account name. Must be paired with keychain_service." + }, "max_rows": { "type": "number", "description": "CRITICAL: Maximum number of rows to return from SELECT queries. Due to MCP's 25k token response limit, this parameter is effectively REQUIRED for all queries against tables of unknown size. Omitting this will cause tool failure on large tables. Start small and increase if needed: Use 10 for initial exploration/preview, 50-100 for small known tables, 100-500 for medium tables (only after confirming size with COUNT(*) query). Even with columnar format (30-50% token reduction), a 100-row result with 10+ columns can approach token limits. Always prefer smaller limits initially." @@ -484,7 +517,7 @@ fn handle_list_tools() -> Result { }, { "name": "connect", - "description": "Test a database connection and return server metadata (version, database name, user). Opens a connection, verifies liveness, returns ConnectionInfo, then disconnects immediately. Stateless and read-only — no config is saved or mutated. Use to health-check a saved connection before running queries, or to verify credentials after setup. IMPORTANT CONNECTION WORKFLOW: (1) RECOMMENDED: Auto-resolve (omit all connection params) — uses project's default saved connection, (2) COMMON: Named connection (use 'connection' param only) — references saved connection by name, (3) DISCOURAGED: Explicit credentials (engine + host/user/password) — ONLY for one-off checks. Possible error codes: CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", + "description": "Test a database connection OR save a connection config by reference. TWO MODES: (A) TEST (default, no 'save'): opens a connection, verifies liveness, returns ConnectionInfo, then disconnects. Stateless — no config mutated. (B) SAVE ('save': \"local\"|\"global\"): persists the connection config BY REFERENCE to .plenum/config.json (local) or ~/.config/plenum/connections.json (global), then returns a saved confirmation. No live connection required — provisions connections offline. CREDENTIAL SAFETY (non-negotiable): inline plaintext 'password' is REJECTED with CAPABILITY_VIOLATION. Source secrets ONLY by reference: 'password_env' (env var name), 'password_command' (shell command), or 'keychain_service'+'keychain_account' (OS keychain). The plaintext secret NEVER passes through Plenum — only the reference string is stored. Use TEST mode to health-check a saved connection; use SAVE mode to register a new one. Possible error codes: CAPABILITY_VIOLATION (inline plaintext password), CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", "inputSchema": { "type": "object", "properties": { @@ -520,6 +553,35 @@ fn handle_list_tools() -> Result { "file": { "type": "string", "description": "DISCOURAGED: SQLite/DuckDB database file path. Only for explicit one-off tests." + }, + "password_env": { + "type": "string", + "description": "Credential reference: name of the environment variable that holds the password (e.g. \"DB_PASSWORD\"). The reference string is stored/used, never the secret value. Mutually exclusive with password_command and keychain_service/keychain_account. Use this instead of inline 'password' (which is rejected)." + }, + "password_command": { + "type": "string", + "description": "Credential reference: shell command whose stdout (trimmed) is the password (e.g. \"op read op://vault/db/password\"). The command string is stored/used, never the secret value. Mutually exclusive with password_env and keychain_service/keychain_account." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference: OS keychain service name. Must be paired with keychain_account. The password is looked up from the platform keychain at connection time. Mutually exclusive with password_env and password_command." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference: OS keychain account name. Must be paired with keychain_service." + }, + "save": { + "type": "string", + "enum": ["local", "global"], + "description": "SAVE MODE: persist this connection config by reference. \"local\" writes .plenum/config.json (team-shareable, per-project); \"global\" writes ~/.config/plenum/connections.json (per-user). Requires explicit 'engine' + connection params. Credentials are stored only as references (password_env/password_command/keychain_*) — never plaintext. Omit to run in TEST mode instead." + }, + "project_path": { + "type": "string", + "description": "SAVE MODE modifier: project path to key the saved connection under. Defaults to the server's bound --project-path, or the current working directory. Local saves are written to /.plenum/config.json." + }, + "name": { + "type": "string", + "description": "SAVE MODE modifier: name to store the connection under (e.g. \"prod\", \"staging\"). Defaults to \"default\". The first connection saved for a project becomes its default." } } } @@ -550,9 +612,33 @@ async fn handle_call_tool(params: Option, binding: &McpBinding) -> Result /// MCP Tool: connect /// -/// Tests a database connection and returns server metadata. -/// Stateless and read-only — no config is saved or mutated. +/// Two modes: +/// - Without `save`: tests a database connection and returns server metadata. +/// Stateless — no config is mutated. +/// - With `save` (`"local"` | `"global"`): persists the connection config **by +/// reference** to `.plenum/config.json` or the global registry, then returns a +/// saved confirmation. No live connection is required, so agents can provision +/// connections offline. +/// +/// Inline plaintext passwords are rejected unconditionally: callers must source +/// secrets via `password_env`, `password_command`, or `keychain_service` + +/// `keychain_account`. This guarantees the plaintext secret never passes through +/// Plenum. async fn tool_connect(args: &Value, binding: &McpBinding) -> Result { + // Capability boundary: the MCP connect tool never handles plaintext secrets. + if args.get("password").and_then(|v| v.as_str()).is_some() { + return Err(anyhow!( + "CAPABILITY_VIOLATION: inline plaintext 'password' is not permitted on the MCP \ + connect tool. Reference the secret instead via 'password_env', 'password_command', \ + or 'keychain_service' + 'keychain_account'." + )); + } + + // Save mode: persist by reference and return without opening a connection. + if let Some(save_raw) = args.get("save") { + return tool_connect_save(args, binding, save_raw); + } + let (config, _is_readonly) = resolve_connection_from_args(args, binding)?; let connection_info = match config.engine { @@ -596,6 +682,79 @@ async fn tool_connect(args: &Value, binding: &McpBinding) -> Result { CallToolResult::success(connection_info) } +/// Persist a connection config by reference for the `connect` tool's save mode. +/// +/// The stored config carries no plaintext password (inline `password` was already +/// rejected by the caller); credentials are recorded only as references. +fn tool_connect_save(args: &Value, binding: &McpBinding, save_raw: &Value) -> Result { + let location = match save_raw.as_str() { + Some("local") => ConfigLocation::Local, + Some("global") => ConfigLocation::Global, + _ => { + return Err(anyhow!("Invalid 'save' value. Must be \"local\" or \"global\"")); + } + }; + + // Saving requires explicit connection parameters — you cannot re-save a + // connection selected purely by name, and a DSN would carry inline secrets. + let engine_str = args.get("engine").and_then(|v| v.as_str()).ok_or_else(|| { + anyhow!("'save' requires explicit connection params, starting with 'engine'") + })?; + + let (password_env, password_command, keychain_entry) = parse_credential_refs(args)?; + let has_ref = password_env.is_some() || password_command.is_some() || keychain_entry.is_some(); + + // Build the config to store. `allow_missing_password` keeps the config free of + // any inline secret — the reference is the sole password authority. + let config = build_connection_config_from_args(args, engine_str, true)?; + + // Project path: explicit arg wins, else the server-registration binding, else cwd. + let project_path = match args.get("project_path").and_then(|v| v.as_str()) { + Some(p) => p.to_string(), + None => match &binding.project_path { + Some(p) => p.clone(), + None => { + crate::config::get_current_project_path().map_err(|e| anyhow!("{}", e.message()))? + } + }, + }; + + let name = args.get("name").and_then(|v| v.as_str()).map(String::from); + + crate::config::save_connection_in_project( + &project_path, + name.clone(), + config, + password_env.clone(), + password_command.clone(), + keychain_entry.clone(), + location, + ) + .map_err(|e| anyhow!("Failed to save connection: {}", e.message()))?; + + let credential_source = if password_env.is_some() { + "password_env" + } else if password_command.is_some() { + "password_command" + } else if keychain_entry.is_some() { + "keychain" + } else { + "none" + }; + + CallToolResult::success(serde_json::json!({ + "saved": true, + "location": match location { + ConfigLocation::Local => "local", + ConfigLocation::Global => "global", + }, + "name": name.unwrap_or_else(|| "default".to_string()), + "project_path": project_path, + "credential_source": credential_source, + "has_reference": has_ref, + })) +} + /// MCP Tool: introspect /// /// Introspects database schema and returns table/column information. @@ -820,8 +979,64 @@ async fn tool_query(args: &Value, binding: &McpBinding) -> Result { // Helper Functions (Stateless) // ============================================================================ -/// Build `ConnectionConfig` from JSON arguments -fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result { +/// Extract credential-reference arguments from a tool call. +/// +/// Recognizes `password_env`, `password_command`, and the keychain pair +/// `keychain_service` + `keychain_account`. These name where a secret lives; +/// the plaintext secret itself never passes through Plenum. +/// +/// Enforces the same invariants the config layer does: +/// - at most one credential source may be specified; +/// - `keychain_service` and `keychain_account` must be supplied together. +/// +/// Returns `(password_env, password_command, keychain_entry)` — all `None` when +/// no reference was provided. +fn parse_credential_refs( + args: &Value, +) -> Result<(Option, Option, Option)> { + let password_env = args.get("password_env").and_then(|v| v.as_str()).map(String::from); + let password_command = args.get("password_command").and_then(|v| v.as_str()).map(String::from); + let service = args.get("keychain_service").and_then(|v| v.as_str()).map(String::from); + let account = args.get("keychain_account").and_then(|v| v.as_str()).map(String::from); + + let keychain_entry = match (service, account) { + (Some(service), Some(account)) => Some(KeychainEntry { service, account }), + (None, None) => None, + _ => { + return Err(anyhow!( + "'keychain_service' and 'keychain_account' must be provided together" + )); + } + }; + + let source_count = + [password_env.is_some(), password_command.is_some(), keychain_entry.is_some()] + .iter() + .filter(|&&b| b) + .count(); + + if source_count > 1 { + return Err(anyhow!( + "Only one credential reference is allowed: \ + password_env, password_command, or keychain_service/keychain_account" + )); + } + + Ok((password_env, password_command, keychain_entry)) +} + +/// Build `ConnectionConfig` from JSON arguments. +/// +/// When `allow_missing_password` is true the inline `password` field is +/// optional — the caller is expected to supply a credential reference +/// (`password_env` / `password_command` / `keychain_service`+`keychain_account`) +/// that resolves the secret at connection time. The resulting config carries +/// `password: None` so the reference is the sole authority. +fn build_connection_config_from_args( + args: &Value, + engine_str: &str, + allow_missing_password: bool, +) -> Result { let engine_type = match engine_str { "postgres" => DatabaseType::Postgres, "mysql" => DatabaseType::MySQL, @@ -844,20 +1059,30 @@ fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result { let file_str = args["file"] @@ -953,7 +1178,28 @@ fn resolve_connection_from_args( // Scenario 2: Explicit connection parameters if has_engine { let engine_str = args["engine"].as_str().unwrap(); - let config = build_connection_config_from_args(args, engine_str)?; + let (password_env, password_command, keychain_entry) = parse_credential_refs(args)?; + let has_ref = + password_env.is_some() || password_command.is_some() || keychain_entry.is_some(); + + let config = build_connection_config_from_args(args, engine_str, has_ref)?; + + // A credential reference sources the secret at connection time; resolve it + // now so the one-off connection carries the real password. The plaintext + // never came through Plenum — only the reference string did. + if has_ref { + let stored = StoredConnection { + config, + password_env, + password_command, + keychain_entry, + readonly: None, + }; + let (resolved, is_readonly) = + stored.resolve().map_err(|e| anyhow!("{}", e.message()))?; + return Ok((resolved, is_readonly)); + } + return Ok((config, false)); // Explicit connections are never readonly } @@ -1214,4 +1460,123 @@ mod tests { resolve_connection_from_args(&args, &McpBinding::default()).unwrap_err().to_string(); assert!(err.contains("mutually exclusive"), "got: {err}"); } + + // ── Credential-reference parsing (REF-298) ────────────────────────────── + + #[test] + fn parse_credential_refs_none_when_absent() { + let (env, cmd, kc) = parse_credential_refs(&serde_json::json!({})).unwrap(); + assert!(env.is_none() && cmd.is_none() && kc.is_none()); + } + + #[test] + fn parse_credential_refs_env() { + let (env, cmd, kc) = + parse_credential_refs(&serde_json::json!({ "password_env": "DB_PASSWORD" })).unwrap(); + assert_eq!(env.as_deref(), Some("DB_PASSWORD")); + assert!(cmd.is_none() && kc.is_none()); + } + + #[test] + fn parse_credential_refs_keychain_requires_both() { + let err = parse_credential_refs(&serde_json::json!({ "keychain_service": "svc" })) + .unwrap_err() + .to_string(); + assert!(err.contains("keychain_service") && err.contains("keychain_account"), "got: {err}"); + } + + #[test] + fn parse_credential_refs_rejects_multiple_sources() { + let args = serde_json::json!({ "password_env": "A", "password_command": "echo b" }); + let err = parse_credential_refs(&args).unwrap_err().to_string(); + assert!(err.contains("Only one credential reference"), "got: {err}"); + } + + // ── One-off explicit connection sourced from a credential reference ────── + + #[test] + fn one_off_explicit_resolves_password_from_env_reference() { + // The reference string is passed; Plenum reads the secret from the env + // var at resolution time. No plaintext ever appeared in the args. + std::env::set_var("PLENUM_TEST_ONEOFF_PWD", "s3cret-from-env"); + let args = serde_json::json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "PLENUM_TEST_ONEOFF_PWD" + }); + let (config, _ro) = resolve_connection_from_args(&args, &McpBinding::default()).unwrap(); + assert_eq!(config.engine, DatabaseType::Postgres); + assert_eq!(config.password.as_deref(), Some("s3cret-from-env")); + std::env::remove_var("PLENUM_TEST_ONEOFF_PWD"); + } + + #[test] + fn one_off_explicit_missing_env_reference_is_error() { + std::env::remove_var("PLENUM_TEST_ONEOFF_MISSING"); + let args = serde_json::json!({ + "engine": "postgres", + "host": "h", "port": 5432, "user": "u", "database": "d", + "password_env": "PLENUM_TEST_ONEOFF_MISSING" + }); + let err = + resolve_connection_from_args(&args, &McpBinding::default()).unwrap_err().to_string(); + assert!(err.contains("PLENUM_TEST_ONEOFF_MISSING"), "names missing var: {err}"); + } + + // ── connect save mode persists by reference (no plaintext) ────────────── + + #[test] + fn connect_save_local_writes_reference_no_plaintext() { + let project = + std::env::temp_dir().join(format!("plenum-mcp-save-{}-{:p}", std::process::id(), &0u8)); + let _ = std::fs::remove_dir_all(&project); + std::fs::create_dir_all(&project).unwrap(); + + let args = serde_json::json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "DB_PASSWORD", + "save": "local", + "name": "prod", + "project_path": project.to_str().unwrap(), + }); + + let result = tool_connect_save(&args, &McpBinding::default(), &args["save"]).unwrap(); + // Tool result envelope wraps a JSON text block; assert the confirmation shape. + let text = result["content"][0]["text"].as_str().unwrap(); + assert!(text.contains("\"saved\": true"), "confirmation: {text}"); + assert!(text.contains("\"location\": \"local\""), "location: {text}"); + + let written = std::fs::read_to_string(project.join(".plenum").join("config.json")).unwrap(); + assert!(written.contains("password_env"), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD")); + assert!(!written.contains("\"password\":"), "no plaintext: {written}"); + assert!(written.contains("prod")); + + let _ = std::fs::remove_dir_all(&project); + } + + #[test] + fn connect_save_rejects_missing_engine() { + let args = serde_json::json!({ "save": "local", "connection": "prod" }); + let err = tool_connect_save(&args, &McpBinding::default(), &args["save"]) + .unwrap_err() + .to_string(); + assert!(err.contains("engine"), "got: {err}"); + } + + #[test] + fn connect_save_rejects_invalid_location() { + let args = serde_json::json!({ "save": "cloud", "engine": "sqlite", "file": "/tmp/x.db" }); + let err = tool_connect_save(&args, &McpBinding::default(), &args["save"]) + .unwrap_err() + .to_string(); + assert!(err.contains("local") && err.contains("global"), "got: {err}"); + } } diff --git a/tests/mcp_bound_connection.rs b/tests/mcp_bound_connection.rs index 1be7ff4..c05a9b2 100644 --- a/tests/mcp_bound_connection.rs +++ b/tests/mcp_bound_connection.rs @@ -109,6 +109,48 @@ fn handshake_then_connect(child: &mut Child) -> Value { response.expect("no response for connect tool call") } +/// Drive the handshake and a single tool call with explicit `arguments`, +/// returning the JSON-RPC response for the call. +fn handshake_then_call(child: &mut Child, tool: &str, arguments: &Value) -> Value { + let mut stdin = child.stdin.take().expect("stdin"); + let stdout = child.stdout.take().expect("stdout"); + let mut reader = BufReader::new(stdout); + + let init = json!({ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { "protocolVersion": "2024-11-05", "capabilities": {}, + "clientInfo": { "name": "test", "version": "0" } } + }); + let notif = json!({ "jsonrpc": "2.0", "method": "notifications/initialized" }); + let call = json!({ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": { "name": tool, "arguments": arguments.clone() } + }); + + writeln!(stdin, "{init}").unwrap(); + writeln!(stdin, "{notif}").unwrap(); + writeln!(stdin, "{call}").unwrap(); + stdin.flush().unwrap(); + drop(stdin); + + let mut response = None; + while let Some(v) = read_line_json(&mut reader) { + if v.get("id") == Some(&json!(2)) { + response = Some(v); + break; + } + } + response.expect("no response for tool call") +} + +/// Extract the JSON-decoded text payload from a successful tool result. +fn tool_result_json(resp: &Value) -> Value { + let text = resp["result"]["content"][0]["text"] + .as_str() + .unwrap_or_else(|| panic!("expected a tool result text block, got: {resp}")); + serde_json::from_str(text).expect("tool result text is valid JSON") +} + /// Create an on-disk `SQLite` database at `path`. Plenum opens connections /// read-only, so the file must already exist for `connect` to succeed. fn create_sqlite_db(path: &Path) { @@ -371,3 +413,109 @@ fn dsn_env_conflicts_with_name_at_cli() { "conflict error should mention --dsn-env: {stderr}" ); } + +// ─── REF-298: credential safety on the MCP connect tool ────────────────────── + +#[test] +fn connect_rejects_inline_plaintext_password() { + // Inline plaintext 'password' must be rejected with a capability error, + // directing the caller to reference-based credential sourcing. + let cwd = scratch("inline-pw"); + let xdg = scratch("inline-pw-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "localhost", + "port": 5432, + "user": "u", + "database": "d", + "password": "hunter2" + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("CAPABILITY_VIOLATION"), "must be a capability error: {msg}"); + assert!(msg.contains("password_env"), "must point to references: {msg}"); + // The plaintext secret must never be echoed back. + assert!(!msg.contains("hunter2"), "must not leak the secret: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_save_local_persists_by_reference() { + // save: "local" writes /.plenum/config.json storing only the + // credential reference — never the plaintext secret. + let cwd = scratch("save-local"); + let xdg = scratch("save-local-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "DB_PASSWORD", + "save": "local", + "name": "prod", + "project_path": cwd.to_str().unwrap() + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + let data = tool_result_json(&resp); + assert_eq!(data["saved"], json!(true)); + assert_eq!(data["location"], json!("local")); + assert_eq!(data["name"], json!("prod")); + assert_eq!(data["credential_source"], json!("password_env")); + + let written = + std::fs::read_to_string(cwd.join(".plenum").join("config.json")).expect("config written"); + assert!(written.contains("password_env"), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD"), "env var name stored: {written}"); + assert!(!written.contains("\"password\":"), "no plaintext password: {written}"); + assert!(written.contains("prod")); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_save_requires_a_credential_reference_shape() { + // A keychain reference needs BOTH service and account; supplying only one + // is rejected before anything is written. + let cwd = scratch("save-badkc"); + let xdg = scratch("save-badkc-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "h", "port": 5432, "user": "u", "database": "d", + "keychain_service": "svc", + "save": "local", + "project_path": cwd.to_str().unwrap() + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("keychain_account"), "must name the missing field: {msg}"); + assert!(!cwd.join(".plenum").join("config.json").exists(), "nothing should be written"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} From a06aa2dcdb19c13c88dd57d8bc98af62701e2d80 Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Mon, 20 Jul 2026 13:27:36 -0400 Subject: [PATCH 3/9] feat(connect): wizard indirect credential sources + mcp_stanza output (REF-299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interactive wizard now offers plaintext, env var, shell command, and OS keychain credential sources — the user always types the source name; the wizard never scans the environment, keychain, or filesystem. Indirect sources are validated at connect time with errors that name the exact field to fix. plenum connect save output now includes a ready-to-paste mcp_stanza (mcpServers block) reproducing the exact --project-path/--name binding for the saved connection, so operators can wire up the MCP server without docs. Co-Authored-By: Paperclip --- src/lib.rs | 5 +- src/main.rs | 289 +++++++++++++++++++++++++++++++++++++++---- src/output.rs | 91 ++++++++++++++ tests/cli_connect.rs | 58 +++++++++ 4 files changed, 421 insertions(+), 22 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 40a9027..14cd3b1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -53,7 +53,10 @@ pub use engine::{ SchemaInfo, TableDiff, TableInfo, TimeOnlyResult, ViewDiff, ViewInfo, }; pub use error::{PlenumError, Result}; -pub use output::{ErrorEnvelope, ErrorInfo, Metadata, SuccessEnvelope, CONTRACT_VERSION}; +pub use output::{ + ErrorEnvelope, ErrorInfo, McpServerConfig, McpStanza, Metadata, SuccessEnvelope, + CONTRACT_VERSION, +}; #[cfg(test)] mod tests { diff --git a/src/main.rs b/src/main.rs index 66a17fa..31ef239 100644 --- a/src/main.rs +++ b/src/main.rs @@ -749,7 +749,7 @@ async fn handle_connect( .await } else { // Interactive mode: show picker - interactive_connect_picker().await.map(|(n, p, c, l)| (n, p, c, None, None, None, l)) + interactive_connect_picker().await }; match result { @@ -762,9 +762,25 @@ async fn handle_connect( keychain_entry, location, )) => { + // Resolve the effective project path up front so the saved connection + // and the emitted mcp_stanza reference the exact same path. When + // proj_path is None, save_connection would fall back to the current + // directory; resolve it here so both stay in lockstep. + let effective_project_path = match proj_path { + Some(p) => p, + None => match plenum::config::get_current_project_path() { + Ok(p) => p, + Err(e) => { + let envelope = ErrorEnvelope::from_error("", "connect", &e); + output_error(&envelope); + return Err(1); + } + }, + }; + // Save connection match plenum::save_connection( - proj_path, + Some(effective_project_path.clone()), Some(conn_name.clone()), config.clone(), password_env, @@ -775,6 +791,10 @@ async fn handle_connect( Ok(()) => { // Build success response let elapsed_ms = start.elapsed().as_millis() as u64; + let mcp_stanza = plenum::McpStanza::for_saved_connection( + &effective_project_path, + &conn_name, + ); let data = serde_json::json!({ "connection_name": conn_name, "engine": config.engine.as_str(), @@ -783,6 +803,7 @@ async fn handle_connect( ConfigLocation::Global => "global", }, "message": format!("Connection '{}' saved successfully", conn_name), + "mcp_stanza": mcp_stanza, }); let envelope = SuccessEnvelope::new( @@ -971,8 +992,7 @@ async fn handle_connect_test( } /// Interactive connection picker (when no args provided) -async fn interactive_connect_picker( -) -> Result<(String, Option, ConnectionConfig, ConfigLocation)> { +async fn interactive_connect_picker() -> Result { use dialoguer::Select; // Get current project path @@ -1032,13 +1052,142 @@ async fn interactive_connect_picker( // Ask for save location let location = prompt_save_location()?; - Ok((name.clone(), None, config.clone(), location)) + // Re-saving an existing connection preserves its stored credential + // reference as-is; the picker never re-prompts for or scans credentials. + Ok((name.clone(), None, config.clone(), None, None, None, location)) + } +} + +/// A credential source chosen in the interactive wizard. +/// +/// Each variant carries only what the user explicitly typed — an env var name, +/// a command string, or a keychain service/account. The wizard never scans the +/// environment, keychain, or filesystem to populate these; there is no ambient +/// pickup or discovery. +#[derive(Debug, Clone, PartialEq, Eq)] +enum WizardCredential { + /// Plaintext password typed directly. + Plaintext(String), + /// Name of an environment variable to read at connection time. + Env(String), + /// Shell command whose stdout yields the password at connection time. + Command(String), + /// OS keychain service + account reference. + Keychain { service: String, account: String }, +} + +/// Split a wizard credential choice into the persisted fields +/// `(password, password_env, password_command, keychain_entry)`. +/// +/// Only the user-typed reference is propagated. For indirect sources the inline +/// `password` is `None` (resolved at use time); nothing here reads the +/// environment, keychain, or filesystem. +fn wizard_credential_fields( + cred: WizardCredential, +) -> (Option, Option, Option, Option) { + match cred { + WizardCredential::Plaintext(p) => (Some(p), None, None, None), + WizardCredential::Env(var) => (None, Some(var), None, None), + WizardCredential::Command(cmd) => (None, None, Some(cmd), None), + WizardCredential::Keychain { service, account } => { + (None, None, None, Some(KeychainEntry { service, account })) + } + } +} + +/// Validate that an indirect credential source resolves now, so the wizard fails +/// fast instead of persisting a broken reference. +/// +/// Error messages name the exact field the user must fix and never suggest a +/// discovered alternative (no "did you mean" from a scan of the environment or +/// keychain). +fn validate_wizard_credential(cred: &WizardCredential) -> Result<()> { + match cred { + WizardCredential::Plaintext(_) => Ok(()), + WizardCredential::Env(var) => match std::env::var(var) { + Ok(v) if v.is_empty() => Err(PlenumError::invalid_input(format!( + "Environment variable {var} is set but empty — set {var} or choose a different credential source" + ))), + Ok(_) => Ok(()), + Err(_) => Err(PlenumError::invalid_input(format!( + "Environment variable {var} is not set — set {var} or choose a different credential source" + ))), + }, + WizardCredential::Command(cmd) => plenum::config::run_password_command_pub(cmd) + .map(|_| ()) + .map_err(|e| { + PlenumError::invalid_input(format!( + "password command failed validation: {} — fix the command or choose a different credential source", + e.message() + )) + }), + WizardCredential::Keychain { service, account } => { + plenum::config::lookup_keychain_password_pub(service, account).map(|_| ()).map_err(|e| { + PlenumError::invalid_input(format!( + "keychain lookup for service '{service}' / account '{account}' failed: {} — fix the service/account or choose a different credential source", + e.message() + )) + }) + } + } +} + +/// Prompt for how the password should be provided. The user always *types* the +/// source reference; the wizard never lists or suggests discovered credentials. +fn prompt_wizard_credential() -> Result { + use dialoguer::{Input, Password, Select}; + + let choices = vec![ + "Enter password directly", + "Environment variable (you type the name)", + "Shell command (you type the command)", + "OS keychain (you type service + account)", + ]; + let idx = Select::new() + .with_prompt("How should the password be provided?") + .items(&choices) + .default(0) + .interact() + .map_err(|e| PlenumError::invalid_input(format!("Selection failed: {e}")))?; + + match idx { + 0 => { + let pw: String = Password::new() + .with_prompt("Password") + .interact() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Plaintext(pw)) + } + 1 => { + let var: String = Input::new() + .with_prompt("Environment variable name (e.g. PGPASSWORD)") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Env(var)) + } + 2 => { + let cmd: String = Input::new() + .with_prompt("Shell command to produce the password") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Command(cmd)) + } + _ => { + let service: String = Input::new() + .with_prompt("Keychain service name") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + let account: String = Input::new() + .with_prompt("Keychain account name") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Keychain { service, account }) + } } } /// Interactive connection wizard -async fn interactive_connect_wizard( -) -> Result<(String, Option, ConnectionConfig, ConfigLocation)> { +async fn interactive_connect_wizard() -> Result { use dialoguer::{Input, Select}; eprintln!("\n=== Create New Database Connection ===\n"); @@ -1052,8 +1201,8 @@ async fn interactive_connect_wizard( .map_err(|e| PlenumError::invalid_input(format!("Selection failed: {e}")))?; let engine = parse_engine(engine_choices[engine_idx])?; - // Build config based on engine type - let config = match engine { + // Build config and resolve any indirect credential reference based on engine type. + let (config, password_env, password_command, keychain_entry) = match engine { DatabaseType::Postgres | DatabaseType::MySQL => { let host: String = Input::new() .with_prompt("Host") @@ -1072,21 +1221,30 @@ async fn interactive_connect_wizard( .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - let password: String = dialoguer::Password::new() - .with_prompt("Password") - .interact() - .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + // Offer plaintext or an indirect credential source, validating that + // the chosen source resolves before we persist the reference. + let cred = prompt_wizard_credential()?; + validate_wizard_credential(&cred)?; let database: String = Input::new() .with_prompt("Database name") .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - if engine == DatabaseType::Postgres { - ConnectionConfig::postgres(host, port, user, password, database) - } else { - ConnectionConfig::mysql(host, port, user, password, database) - } + let (password, p_env, p_cmd, keychain) = wizard_credential_fields(cred); + + let config = ConnectionConfig { + engine, + host: Some(host), + port: Some(port), + user: Some(user), + // None for indirect sources — resolved at use time, never stored inline. + password, + database: Some(database), + file: None, + tls: None, + }; + (config, p_env, p_cmd, keychain) } DatabaseType::SQLite | DatabaseType::DuckDB => { let file: String = Input::new() @@ -1094,11 +1252,13 @@ async fn interactive_connect_wizard( .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - if engine == DatabaseType::DuckDB { + let config = if engine == DatabaseType::DuckDB { ConnectionConfig::duckdb(PathBuf::from(file)) } else { ConnectionConfig::sqlite(PathBuf::from(file)) - } + }; + // File-based engines have no authentication credential. + (config, None, None, None) } }; @@ -1113,7 +1273,7 @@ async fn interactive_connect_wizard( let location = prompt_save_location()?; // Use None for project_path (will default to current directory) - Ok((name, None, config, location)) + Ok((name, None, config, password_env, password_command, keychain_entry, location)) } /// Non-interactive connect (with CLI args) @@ -2017,3 +2177,90 @@ fn parse_engine(engine: &str) -> Result { ))), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn wizard_env_credential_stores_only_typed_name() { + // Capability test: the typed env var name is stored verbatim; no inline + // password value is produced, and nothing is read from the environment. + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Env("PGPASSWORD".to_string())); + assert_eq!(p_env.as_deref(), Some("PGPASSWORD")); + assert!(password.is_none(), "indirect source must not store an inline password"); + assert!(p_cmd.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_command_credential_stores_only_typed_command() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Command("op read secret".to_string())); + assert_eq!(p_cmd.as_deref(), Some("op read secret")); + assert!(password.is_none()); + assert!(p_env.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_keychain_credential_stores_only_typed_service_account() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Keychain { + service: "plenum-db".to_string(), + account: "app".to_string(), + }); + let entry = keychain.expect("keychain entry stored"); + assert_eq!(entry.service, "plenum-db"); + assert_eq!(entry.account, "app"); + assert!(password.is_none()); + assert!(p_env.is_none()); + assert!(p_cmd.is_none()); + } + + #[test] + fn wizard_plaintext_credential_stores_inline_password() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Plaintext("hunter2".to_string())); + assert_eq!(password.as_deref(), Some("hunter2")); + assert!(p_env.is_none()); + assert!(p_cmd.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_validate_env_missing_names_the_field_to_fix() { + // A missing env var must fail fast and name the exact variable — never + // suggest a discovered alternative. + let var = "PLENUM_WIZARD_TEST_MISSING_XYZ"; + // Ensure it is unset for the duration of this assertion. + assert!(std::env::var(var).is_err(), "test precondition: var must be unset"); + let err = validate_wizard_credential(&WizardCredential::Env(var.to_string())) + .expect_err("missing env var must fail validation"); + let msg = err.message(); + assert!(msg.contains(var), "error should name the missing variable, got {msg:?}"); + assert!(msg.contains("not set"), "error should explain it is unset, got {msg:?}"); + } + + #[test] + fn wizard_validate_env_present_passes() { + let var = "PLENUM_WIZARD_TEST_PRESENT_ABC"; + std::env::set_var(var, "resolved-secret"); + let result = validate_wizard_credential(&WizardCredential::Env(var.to_string())); + std::env::remove_var(var); + assert!(result.is_ok(), "present env var should pass validation"); + } + + #[test] + fn wizard_validate_env_empty_names_the_field_to_fix() { + let var = "PLENUM_WIZARD_TEST_EMPTY_DEF"; + std::env::set_var(var, ""); + let result = validate_wizard_credential(&WizardCredential::Env(var.to_string())); + std::env::remove_var(var); + let err = result.expect_err("empty env var must fail"); + let msg = err.message(); + assert!(msg.contains(var), "error should name the empty variable, got {msg:?}"); + assert!(msg.contains("empty"), "error should explain it is empty, got {msg:?}"); + } +} diff --git a/src/output.rs b/src/output.rs index 60a11a7..165c928 100644 --- a/src/output.rs +++ b/src/output.rs @@ -9,6 +9,8 @@ //! //! Output is stable, versioned, and suitable for programmatic parsing by agents. +use std::collections::BTreeMap; + use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -95,6 +97,63 @@ impl ErrorEnvelope { } } +/// A single MCP server entry within an [`McpStanza`]. +/// +/// Mirrors the shape MCP hosts expect under `mcpServers.`: an executable +/// plus the exact argv used to launch Plenum's MCP server for the saved connection. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct McpServerConfig { + /// Executable to launch (always `"plenum"`). + pub command: String, + + /// Arguments passed to the executable. Reflects the W1 launch flags + /// (`mcp --project-path --name `) for the connection just saved. + pub args: Vec, +} + +/// Ready-to-paste `mcpServers` configuration block emitted after `plenum connect` +/// saves a connection. +/// +/// The agent operator can copy this verbatim into an MCP host config (e.g. +/// `claude_desktop_config.json`) to wire up the Plenum MCP server for the saved +/// connection without consulting setup documentation. +/// +/// Only the connection's project path and name are encoded — never a credential +/// or secret. Indirect credential sources remain resolved at connection time by +/// the stored connection reference. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct McpStanza { + /// Map of MCP server name to launch config. Always contains a single + /// `"plenum"` entry. + #[serde(rename = "mcpServers")] + pub mcp_servers: BTreeMap, +} + +impl McpStanza { + /// Build the stanza for a connection that was just saved under + /// `project_path` with the given `name`. + /// + /// The MCP server binds resolution explicitly to `--project-path`/`--name` + /// (never inferring from cwd), so the emitted argv reproduces exactly that + /// binding. + #[must_use] + pub fn for_saved_connection(project_path: &str, name: &str) -> Self { + let server = McpServerConfig { + command: "plenum".to_string(), + args: vec![ + "mcp".to_string(), + "--project-path".to_string(), + project_path.to_string(), + "--name".to_string(), + name.to_string(), + ], + }; + let mut mcp_servers = BTreeMap::new(); + mcp_servers.insert("plenum".to_string(), server); + Self { mcp_servers } + } +} + /// Error information structure #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct ErrorInfo { @@ -289,6 +348,38 @@ mod tests { assert!(!envelope.ok); } + #[test] + fn test_mcp_stanza_shape_reflects_project_path_and_name() { + let stanza = McpStanza::for_saved_connection("/home/user/project1", "staging"); + let json = serde_json::to_value(&stanza).unwrap(); + + // Ready-to-paste block is keyed by `mcpServers` -> `plenum`. + let server = &json["mcpServers"]["plenum"]; + assert_eq!(server["command"], "plenum"); + assert_eq!( + server["args"], + serde_json::json!([ + "mcp", + "--project-path", + "/home/user/project1", + "--name", + "staging" + ]) + ); + } + + #[test] + fn test_mcp_stanza_encodes_only_path_and_name_no_secret() { + // The stanza must never carry a credential — only the connection reference. + let stanza = McpStanza::for_saved_connection("/tmp/proj", "prod"); + let json = serde_json::to_string(&stanza).unwrap(); + assert!(!json.contains("password")); + assert!(!json.contains("dsn")); + // Single server entry named "plenum". + assert_eq!(stanza.mcp_servers.len(), 1); + assert!(stanza.mcp_servers.contains_key("plenum")); + } + #[test] fn test_contract_version_constant() { assert_eq!(CONTRACT_VERSION, "1"); diff --git a/tests/cli_connect.rs b/tests/cli_connect.rs index f0cea98..5eb7235 100644 --- a/tests/cli_connect.rs +++ b/tests/cli_connect.rs @@ -194,6 +194,64 @@ fn connect_rejects_password_and_password_env_together() { cleanup(&dir); } +#[test] +fn connect_save_emits_ready_to_paste_mcp_stanza() { + let dir = unique_tmp_dir("stanza"); + let db_path = create_sqlite_db(&dir); + let canonical = std::fs::canonicalize(&dir).expect("canonicalize scratch dir"); + let canonical = canonical.to_str().expect("utf-8 path").to_string(); + + let (code, stdout, _stderr) = run_connect( + &dir, + &[], + &[ + "--name", + "staging", + "--engine", + "sqlite", + "--file", + db_path.to_str().unwrap(), + "--save", + "local", + ], + ); + + assert_eq!(code, 0, "expected success, stdout={stdout}"); + + let envelope: Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("non-JSON stdout {stdout:?}: {e}")); + assert_eq!(envelope.get("ok").and_then(Value::as_bool), Some(true)); + + // A ready-to-paste mcpServers block must be present in the connect data. + let server = envelope + .pointer("/data/mcp_stanza/mcpServers/plenum") + .expect("mcp_stanza.mcpServers.plenum present in connect output"); + assert_eq!(server.get("command").and_then(Value::as_str), Some("plenum")); + + let args: Vec = server + .get("args") + .and_then(Value::as_array) + .expect("args array present") + .iter() + .map(|v| v.as_str().unwrap_or_default().to_string()) + .collect(); + + // Reflects the W1 launch flags for the connection just saved. + assert_eq!( + args, + vec![ + "mcp".to_string(), + "--project-path".to_string(), + canonical, + "--name".to_string(), + "staging".to_string(), + ], + "stanza args should reproduce the exact --project-path/--name binding" + ); + + cleanup(&dir); +} + // ============================================================================ // --test (connection ping) tests // ============================================================================ From 1d66853d53afda86351991eda02052b0ab7ed392 Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Mon, 20 Jul 2026 13:54:09 -0400 Subject: [PATCH 4/9] fix(connect): persist --save local under explicit --project-path (REF-303) `plenum connect --project-path X --save local` run from a different cwd reported success but persisted nothing: the local config format is single-project and keyed by its own directory, so writing under cwd while keying the entry under an explicit --project-path silently dropped the connection. Use save_connection_in_project so --save local targets /.plenum/config.json, and route CLI connection resolution through --project-path consistently. Adds a regression test. Surfaced during REF-303 whole-flow end-to-end verification of REF-292. Co-Authored-By: Paperclip --- src/lib.rs | 4 +-- src/main.rs | 32 ++++++++++++++++---- tests/cli_connect.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 14cd3b1..e2c1afb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,8 +42,8 @@ pub mod output; // JSON output envelopes (Phase 1.2) // MCP server (Phase 7) - M pub use capability::validate_query; pub use config::{ list_connections, list_connections_for_project, list_connections_raw, resolve_connection, - resolve_connection_in_project, save_connection, ConfigLocation, ConnectionRegistry, - KeychainEntry, StoredConnection, + resolve_connection_in_project, save_connection, save_connection_in_project, ConfigLocation, + ConnectionRegistry, KeychainEntry, StoredConnection, }; pub use dsn::{parse_dsn, redact_dsn}; pub use engine::{ diff --git a/src/main.rs b/src/main.rs index 31ef239..ac3ccf2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -778,9 +778,14 @@ async fn handle_connect( }, }; - // Save connection - match plenum::save_connection( - Some(effective_project_path.clone()), + // Save connection. Use the project-scoped writer so `--save local` + // targets `/.plenum/config.json` rather than the + // current working directory. The local config format is + // single-project and keyed by its own directory, so writing under + // cwd while keying the entry under an explicit `--project-path` + // would silently drop the connection. + match plenum::save_connection_in_project( + &effective_project_path, Some(conn_name.clone()), config.clone(), password_env, @@ -1532,7 +1537,7 @@ async fn handle_introspect( // --diff-against-project-path overrides; otherwise fall back to the primary project path // (or current directory when neither is specified). let target_proj = diff_against_project_path.as_deref().or(project_path.as_deref()); - let target_config = match plenum::resolve_connection(target_proj, Some(&target_name)) { + let target_config = match resolve_connection_cli(target_proj, Some(&target_name)) { Ok((cfg, _)) => cfg, Err(e) => { let envelope = ErrorEnvelope::from_error(config.engine.as_str(), "introspect", &e); @@ -2052,6 +2057,23 @@ fn build_tls_config( /// This helper resolves a connection from config or builds one from CLI arguments. /// Precedence: Named connection at project path → CLI arguments only /// Returns a tuple of (`ConnectionConfig`, `is_readonly`). +/// Resolve a saved connection honoring an explicit `--project-path`. +/// +/// When a project path is given, the connection is resolved from that project's +/// own `.plenum/config.json` (merged with the global registry), matching the +/// documented `--project-path /other/project` semantics and the way +/// `plenum mcp --project-path` binds. When no project path is given, resolution +/// falls back to the current working directory's local config. +fn resolve_connection_cli( + project_path: Option<&str>, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + match project_path { + Some(path) => plenum::resolve_connection_in_project(path, name), + None => plenum::resolve_connection(None, name), + } +} + fn build_connection_config( name: Option<&str>, project_path: Option<&str>, @@ -2075,7 +2097,7 @@ fn build_connection_config( let mut resolved_connection: Option<(ConnectionConfig, bool)> = if should_try_resolve { // Try to load connection from config - match plenum::resolve_connection(project_path, name) { + match resolve_connection_cli(project_path, name) { Ok(cfg_tuple) => Some(cfg_tuple), Err(_) if has_explicit_args => None, // Ignore error if explicit args provided as fallback Err(e) => return Err(e), // Propagate error if no fallback diff --git a/tests/cli_connect.rs b/tests/cli_connect.rs index 5eb7235..1663c5e 100644 --- a/tests/cli_connect.rs +++ b/tests/cli_connect.rs @@ -252,6 +252,75 @@ fn connect_save_emits_ready_to_paste_mcp_stanza() { cleanup(&dir); } +#[test] +fn connect_save_local_with_explicit_project_path_persists_to_that_project() { + // Regression: `connect --project-path X --save local` run from a DIFFERENT + // cwd must persist the connection under X's own `.plenum/config.json`, not + // silently drop it. The local config format is single-project and keyed by + // its own directory, so a save that keys the entry under X while writing the + // file under cwd loses the connection entirely (save reports ok, nothing + // persisted). The save must target /.plenum/config.json. + let cwd = unique_tmp_dir("proj_cwd"); + let project = unique_tmp_dir("proj_target"); + let db_path = create_sqlite_db(&project); + + let (code, stdout, _stderr) = run_connect( + &cwd, + &[], + &[ + "--name", + "smoke", + "--project-path", + project.to_str().unwrap(), + "--engine", + "sqlite", + "--file", + db_path.to_str().unwrap(), + "--save", + "local", + ], + ); + + assert_eq!(code, 0, "expected success, stdout={stdout}"); + let envelope: Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("non-JSON stdout {stdout:?}: {e}")); + assert_eq!(envelope.get("ok").and_then(Value::as_bool), Some(true)); + + // The connection must land in the TARGET project's local config, keyed under + // its own directory (single-project local format). + let target_cfg = project.join(".plenum").join("config.json"); + let contents = std::fs::read_to_string(&target_cfg).unwrap_or_else(|e| { + panic!( + "connection must persist to /.plenum/config.json ({target_cfg:?}): {e}" + ) + }); + let parsed: Value = serde_json::from_str(&contents).expect("local config is valid JSON"); + assert!( + parsed.pointer("/connections/smoke").is_some(), + "saved connection 'smoke' must be present in target config, got {contents}" + ); + + // It must NOT have been written under the unrelated cwd. + assert!( + !cwd.join(".plenum").join("config.json").exists(), + "connection must not leak into the working directory's config" + ); + + // And it must be resolvable via the same explicit project path. + let (test_code, test_stdout, _) = run_connect( + &cwd, + &[], + &["--test", "--project-path", project.to_str().unwrap(), "--name", "smoke"], + ); + assert_eq!( + test_code, 0, + "saved connection must resolve by project-path+name, stdout={test_stdout}" + ); + + cleanup(&cwd); + cleanup(&project); +} + // ============================================================================ // --test (connection ping) tests // ============================================================================ From 545e4ae1e2fcf4ed0ac29ebf8dab3be15e2a12fb Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Mon, 20 Jul 2026 13:58:42 -0400 Subject: [PATCH 5/9] refactor(connect): type mcp_stanza save envelope + name ConnectArgs struct (REF-304) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking follow-ups from the REF-292 SRE review: 1. Type the connect --save success `data` through a new `ConnectSaveResult` struct instead of building it inline via `serde_json::json!` in main.rs. Wired into generate-schemas (schemas/connect_save_success.json) and tests/schema_drift.rs, so the emitted stanza shape — including the nested McpStanza — is now covered by schema-drift and can no longer drift silently. 2. Convert the 7-field `type ConnectArgs = (...)` tuple alias into a named struct with documented fields for readability across the three resolution paths (picker, wizard, non-interactive) and the save step. Output is byte-identical: cli_connect integration tests pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- schemas/connect_save_success.json | 178 ++++++++++++++++++++++++++++++ src/bin/generate_schemas.rs | 6 +- src/lib.rs | 4 +- src/main.rs | 85 +++++++++----- src/output.rs | 46 ++++++++ tests/schema_drift.rs | 15 ++- 6 files changed, 303 insertions(+), 31 deletions(-) create mode 100644 schemas/connect_save_success.json diff --git a/schemas/connect_save_success.json b/schemas/connect_save_success.json new file mode 100644 index 0000000..c9ddc44 --- /dev/null +++ b/schemas/connect_save_success.json @@ -0,0 +1,178 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SuccessEnvelope_for_ConnectSaveResult", + "description": "Success envelope for operation results\n\nGeneric over the data type to support different operation return values.", + "type": "object", + "required": [ + "command", + "data", + "engine", + "meta", + "ok" + ], + "properties": { + "command": { + "description": "Command that was executed (connect, introspect, query)", + "type": "string" + }, + "data": { + "description": "Operation-specific data", + "allOf": [ + { + "$ref": "#/definitions/ConnectSaveResult" + } + ] + }, + "engine": { + "description": "Database engine used for this operation (postgres, mysql, sqlite)", + "type": "string" + }, + "meta": { + "description": "Execution metadata", + "allOf": [ + { + "$ref": "#/definitions/Metadata" + } + ] + }, + "ok": { + "description": "Always true for success envelopes", + "type": "boolean" + } + }, + "definitions": { + "ConnectSaveResult": { + "description": "Success `data` payload emitted by `plenum connect --save` after a connection is persisted.\n\nTyping this envelope — rather than assembling it inline via `serde_json::json!` at the call site — keeps the emitted shape, including the nested [`McpStanza`], under `generate-schemas` + `tests/schema_drift.rs` coverage so the stanza cannot drift without a failing test.", + "type": "object", + "required": [ + "connection_name", + "engine", + "mcp_stanza", + "message", + "saved_to" + ], + "properties": { + "connection_name": { + "description": "Name the connection was saved under.", + "type": "string" + }, + "engine": { + "description": "Database engine of the saved connection (postgres, mysql, sqlite).", + "type": "string" + }, + "mcp_stanza": { + "description": "Ready-to-paste MCP server stanza for the saved connection.", + "allOf": [ + { + "$ref": "#/definitions/McpStanza" + } + ] + }, + "message": { + "description": "Human-readable confirmation message.", + "type": "string" + }, + "saved_to": { + "description": "Where the connection was persisted: `\"local\"` or `\"global\"`.", + "type": "string" + } + } + }, + "McpServerConfig": { + "description": "A single MCP server entry within an [`McpStanza`].\n\nMirrors the shape MCP hosts expect under `mcpServers.`: an executable plus the exact argv used to launch Plenum's MCP server for the saved connection.", + "type": "object", + "required": [ + "args", + "command" + ], + "properties": { + "args": { + "description": "Arguments passed to the executable. Reflects the W1 launch flags (`mcp --project-path --name `) for the connection just saved.", + "type": "array", + "items": { + "type": "string" + } + }, + "command": { + "description": "Executable to launch (always `\"plenum\"`).", + "type": "string" + } + } + }, + "McpStanza": { + "description": "Ready-to-paste `mcpServers` configuration block emitted after `plenum connect` saves a connection.\n\nThe agent operator can copy this verbatim into an MCP host config (e.g. `claude_desktop_config.json`) to wire up the Plenum MCP server for the saved connection without consulting setup documentation.\n\nOnly the connection's project path and name are encoded — never a credential or secret. Indirect credential sources remain resolved at connection time by the stored connection reference.", + "type": "object", + "required": [ + "mcpServers" + ], + "properties": { + "mcpServers": { + "description": "Map of MCP server name to launch config. Always contains a single `\"plenum\"` entry.", + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/McpServerConfig" + } + } + } + }, + "Metadata": { + "description": "Execution metadata included in all responses", + "type": "object", + "required": [ + "contract_version", + "execution_ms" + ], + "properties": { + "contract_version": { + "description": "Output contract version — agents use this to guard against silent breaking changes", + "type": "string" + }, + "execution_ms": { + "description": "Execution time in milliseconds", + "type": "integer", + "format": "uint64", + "minimum": 0.0 + }, + "has_more": { + "description": "Whether more rows are available beyond this page (query results only)", + "type": [ + "boolean", + "null" + ] + }, + "next_offset": { + "description": "Offset to pass as --offset for the next page (present only when `has_more` is true)", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, + "rows_returned": { + "description": "Number of rows returned (for query results, None for other operations)", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, + "rows_truncated": { + "description": "Whether the result set was capped by `max_rows` (query results only)", + "type": [ + "boolean", + "null" + ] + }, + "truncated_by": { + "description": "Why the result was truncated: \"bytes\" when `max_bytes` triggered the cut; absent for row-count truncation or when rows are not truncated", + "type": [ + "string", + "null" + ] + } + } + } + } +} diff --git a/src/bin/generate_schemas.rs b/src/bin/generate_schemas.rs index 4491764..5c94594 100644 --- a/src/bin/generate_schemas.rs +++ b/src/bin/generate_schemas.rs @@ -6,7 +6,10 @@ //! Run this whenever output types change to keep the checked-in schemas in sync. //! The drift test in `tests/schema_drift.rs` fails if schemas are stale. -use plenum::{ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, SuccessEnvelope}; +use plenum::{ + ConnectSaveResult, ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, + SuccessEnvelope, +}; use schemars::schema_for; use std::fs; @@ -14,6 +17,7 @@ fn main() { let schemas: &[(&str, schemars::schema::RootSchema)] = &[ ("schemas/error_envelope.json", schema_for!(ErrorEnvelope)), ("schemas/connect_success.json", schema_for!(SuccessEnvelope)), + ("schemas/connect_save_success.json", schema_for!(SuccessEnvelope)), ("schemas/introspect_success.json", schema_for!(SuccessEnvelope)), ("schemas/query_success.json", schema_for!(SuccessEnvelope)), ]; diff --git a/src/lib.rs b/src/lib.rs index e2c1afb..cfb43d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -54,8 +54,8 @@ pub use engine::{ }; pub use error::{PlenumError, Result}; pub use output::{ - ErrorEnvelope, ErrorInfo, McpServerConfig, McpStanza, Metadata, SuccessEnvelope, - CONTRACT_VERSION, + ConnectSaveResult, ErrorEnvelope, ErrorInfo, McpServerConfig, McpStanza, Metadata, + SuccessEnvelope, CONTRACT_VERSION, }; #[cfg(test)] diff --git a/src/main.rs b/src/main.rs index ac3ccf2..2454f2e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,17 +30,24 @@ use plenum::engine::postgres::PostgresEngine; #[cfg(feature = "sqlite")] use plenum::engine::sqlite::SqliteEngine; -/// Resolved arguments for `plenum connect`: -/// connection name, project path, config, `password_env`, `password_command`, `keychain_entry`, save location. -type ConnectArgs = ( - String, - Option, - ConnectionConfig, - Option, - Option, - Option, - ConfigLocation, -); +/// Resolved arguments for `plenum connect`, produced by the interactive picker, +/// interactive wizard, or non-interactive flag path and consumed by the save step. +struct ConnectArgs { + /// Connection name to save under. + name: String, + /// Explicit project path, or `None` to default to the current directory. + project_path: Option, + /// Resolved connection configuration. + config: ConnectionConfig, + /// Indirect credential source: environment variable name. + password_env: Option, + /// Indirect credential source: shell command producing the password. + password_command: Option, + /// Indirect credential source: OS keychain entry. + keychain_entry: Option, + /// Where to persist the connection (local or global). + location: ConfigLocation, +} /// Plenum - Agent-First Database Control CLI #[derive(Parser)] @@ -753,15 +760,15 @@ async fn handle_connect( }; match result { - Ok(( - conn_name, - proj_path, + Ok(ConnectArgs { + name: conn_name, + project_path: proj_path, config, password_env, password_command, keychain_entry, location, - )) => { + }) => { // Resolve the effective project path up front so the saved connection // and the emitted mcp_stanza reference the exact same path. When // proj_path is None, save_connection would fall back to the current @@ -800,16 +807,16 @@ async fn handle_connect( &effective_project_path, &conn_name, ); - let data = serde_json::json!({ - "connection_name": conn_name, - "engine": config.engine.as_str(), - "saved_to": match location { - ConfigLocation::Local => "local", - ConfigLocation::Global => "global", + let data = plenum::ConnectSaveResult { + message: format!("Connection '{conn_name}' saved successfully"), + connection_name: conn_name, + engine: config.engine.as_str().to_string(), + saved_to: match location { + ConfigLocation::Local => "local".to_string(), + ConfigLocation::Global => "global".to_string(), }, - "message": format!("Connection '{}' saved successfully", conn_name), - "mcp_stanza": mcp_stanza, - }); + mcp_stanza, + }; let envelope = SuccessEnvelope::new( config.engine.as_str(), @@ -1059,7 +1066,15 @@ async fn interactive_connect_picker() -> Result { // Re-saving an existing connection preserves its stored credential // reference as-is; the picker never re-prompts for or scans credentials. - Ok((name.clone(), None, config.clone(), None, None, None, location)) + Ok(ConnectArgs { + name: name.clone(), + project_path: None, + config: config.clone(), + password_env: None, + password_command: None, + keychain_entry: None, + location, + }) } } @@ -1278,7 +1293,15 @@ async fn interactive_connect_wizard() -> Result { let location = prompt_save_location()?; // Use None for project_path (will default to current directory) - Ok((name, None, config, password_env, password_command, keychain_entry, location)) + Ok(ConnectArgs { + name, + project_path: None, + config, + password_env, + password_command, + keychain_entry, + location, + }) } /// Non-interactive connect (with CLI args) @@ -1440,7 +1463,15 @@ async fn non_interactive_connect( } }; - Ok((conn_name, project_path, config, password_env, password_command, keychain_entry, location)) + Ok(ConnectArgs { + name: conn_name, + project_path, + config, + password_env, + password_command, + keychain_entry, + location, + }) } /// Prompt user for save location diff --git a/src/output.rs b/src/output.rs index 165c928..79fb1d7 100644 --- a/src/output.rs +++ b/src/output.rs @@ -154,6 +154,31 @@ impl McpStanza { } } +/// Success `data` payload emitted by `plenum connect --save` after a connection +/// is persisted. +/// +/// Typing this envelope — rather than assembling it inline via +/// `serde_json::json!` at the call site — keeps the emitted shape, including the +/// nested [`McpStanza`], under `generate-schemas` + `tests/schema_drift.rs` +/// coverage so the stanza cannot drift without a failing test. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct ConnectSaveResult { + /// Name the connection was saved under. + pub connection_name: String, + + /// Database engine of the saved connection (postgres, mysql, sqlite). + pub engine: String, + + /// Where the connection was persisted: `"local"` or `"global"`. + pub saved_to: String, + + /// Human-readable confirmation message. + pub message: String, + + /// Ready-to-paste MCP server stanza for the saved connection. + pub mcp_stanza: McpStanza, +} + /// Error information structure #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct ErrorInfo { @@ -380,6 +405,27 @@ mod tests { assert!(stanza.mcp_servers.contains_key("plenum")); } + #[test] + fn test_connect_save_result_shape() { + let data = ConnectSaveResult { + connection_name: "staging".to_string(), + engine: "postgres".to_string(), + saved_to: "local".to_string(), + message: "Connection 'staging' saved successfully".to_string(), + mcp_stanza: McpStanza::for_saved_connection("/home/user/project1", "staging"), + }; + let json = serde_json::to_value(&data).unwrap(); + + assert_eq!(json["connection_name"], "staging"); + assert_eq!(json["engine"], "postgres"); + assert_eq!(json["saved_to"], "local"); + assert_eq!(json["message"], "Connection 'staging' saved successfully"); + // Nested stanza is carried through under the same `mcp_stanza` key the + // inline `serde_json::json!` builder used, keyed by `mcpServers` -> `plenum`. + assert_eq!(json["mcp_stanza"]["mcpServers"]["plenum"]["command"], "plenum"); + assert!(!json.to_string().contains("password")); + } + #[test] fn test_contract_version_constant() { assert_eq!(CONTRACT_VERSION, "1"); diff --git a/tests/schema_drift.rs b/tests/schema_drift.rs index 9842d96..1219603 100644 --- a/tests/schema_drift.rs +++ b/tests/schema_drift.rs @@ -3,7 +3,10 @@ //! //! When this test fails, run `cargo run --bin generate-schemas` to regenerate. -use plenum::{ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, SuccessEnvelope}; +use plenum::{ + ConnectSaveResult, ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, + SuccessEnvelope, +}; use schemars::schema_for; fn expected_schema(schema: &schemars::schema::RootSchema) -> String { @@ -37,6 +40,16 @@ fn connect_success_schema_not_stale() { ); } +#[test] +fn connect_save_success_schema_not_stale() { + let generated = expected_schema(&schema_for!(SuccessEnvelope)); + let on_disk = on_disk("connect_save_success.json"); + assert_eq!( + on_disk, generated, + "schemas/connect_save_success.json is stale — run: cargo run --bin generate-schemas" + ); +} + #[test] fn introspect_success_schema_not_stale() { let generated = expected_schema(&schema_for!(SuccessEnvelope)); From 6403f78d335f8adb9e2823115f1383909ef1798f Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Tue, 6 Oct 2026 20:22:05 -0400 Subject: [PATCH 6/9] fix(mcp): reject password_command on all MCP tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `password_command` is executed via `sh -c`. Accepting it from MCP tool arguments gave any connected agent arbitrary shell execution on the host, which breaks Plenum's least-privilege execution surface. - handle_call_tool rejects `password_command` with CAPABILITY_VIOLATION for connect, introspect, and query before any tool logic runs — including named-connection calls, so the field is never silently ignored. - Remove `password_command` from the three tool input schemas and from parse_credential_refs; MCP credential references are now `password_env` or `keychain_service` + `keychain_account`. - The CLI `--password-command` flag and the connect wizard are unchanged: a human types the command there. Residual risk (documented in SECURITY.md): a connection already saved with `password_command` still runs it when resolved through the MCP server; the command comes from a config file, not from tool arguments. Tests: 5 E2E tests drive `plenum mcp` with a marker-file command and assert CAPABILITY_VIOLATION, that the command never ran, and nothing was written. Offline suite (502) and live MySQL 8.0/8.4 + PostgreSQL 16 suites (56) green. Co-Authored-By: Claude Opus 5.5 --- src/mcp.rs | 107 ++++++++++++++++------------------ tests/mcp_bound_connection.rs | 84 ++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 58 deletions(-) diff --git a/src/mcp.rs b/src/mcp.rs index eab2cfe..8d6b9a3 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -346,15 +346,11 @@ fn handle_list_tools() -> Result { }, "password_env": { "type": "string", - "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with password_command and keychain_service/keychain_account." - }, - "password_command": { - "type": "string", - "description": "Credential reference for one-off explicit connections: shell command whose stdout (trimmed) is the password. Mutually exclusive with password_env and keychain_service/keychain_account." + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with keychain_service/keychain_account." }, "keychain_service": { "type": "string", - "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env and password_command." + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env." }, "keychain_account": { "type": "string", @@ -472,15 +468,11 @@ fn handle_list_tools() -> Result { }, "password_env": { "type": "string", - "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with password_command and keychain_service/keychain_account." - }, - "password_command": { - "type": "string", - "description": "Credential reference for one-off explicit connections: shell command whose stdout (trimmed) is the password. Mutually exclusive with password_env and keychain_service/keychain_account." + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with keychain_service/keychain_account." }, "keychain_service": { "type": "string", - "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env and password_command." + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env." }, "keychain_account": { "type": "string", @@ -517,7 +509,7 @@ fn handle_list_tools() -> Result { }, { "name": "connect", - "description": "Test a database connection OR save a connection config by reference. TWO MODES: (A) TEST (default, no 'save'): opens a connection, verifies liveness, returns ConnectionInfo, then disconnects. Stateless — no config mutated. (B) SAVE ('save': \"local\"|\"global\"): persists the connection config BY REFERENCE to .plenum/config.json (local) or ~/.config/plenum/connections.json (global), then returns a saved confirmation. No live connection required — provisions connections offline. CREDENTIAL SAFETY (non-negotiable): inline plaintext 'password' is REJECTED with CAPABILITY_VIOLATION. Source secrets ONLY by reference: 'password_env' (env var name), 'password_command' (shell command), or 'keychain_service'+'keychain_account' (OS keychain). The plaintext secret NEVER passes through Plenum — only the reference string is stored. Use TEST mode to health-check a saved connection; use SAVE mode to register a new one. Possible error codes: CAPABILITY_VIOLATION (inline plaintext password), CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", + "description": "Test a database connection OR save a connection config by reference. TWO MODES: (A) TEST (default, no 'save'): opens a connection, verifies liveness, returns ConnectionInfo, then disconnects. Stateless — no config mutated. (B) SAVE ('save': \"local\"|\"global\"): persists the connection config BY REFERENCE to .plenum/config.json (local) or ~/.config/plenum/connections.json (global), then returns a saved confirmation. No live connection required — provisions connections offline. CREDENTIAL SAFETY (non-negotiable): inline plaintext 'password' is REJECTED with CAPABILITY_VIOLATION. Source secrets ONLY by reference: 'password_env' (env var name) or 'keychain_service'+'keychain_account' (OS keychain). 'password_command' is REJECTED with CAPABILITY_VIOLATION because it executes a shell command. The plaintext secret NEVER passes through Plenum — only the reference string is stored. Use TEST mode to health-check a saved connection; use SAVE mode to register a new one. Possible error codes: CAPABILITY_VIOLATION (inline plaintext password or password_command), CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", "inputSchema": { "type": "object", "properties": { @@ -556,15 +548,11 @@ fn handle_list_tools() -> Result { }, "password_env": { "type": "string", - "description": "Credential reference: name of the environment variable that holds the password (e.g. \"DB_PASSWORD\"). The reference string is stored/used, never the secret value. Mutually exclusive with password_command and keychain_service/keychain_account. Use this instead of inline 'password' (which is rejected)." - }, - "password_command": { - "type": "string", - "description": "Credential reference: shell command whose stdout (trimmed) is the password (e.g. \"op read op://vault/db/password\"). The command string is stored/used, never the secret value. Mutually exclusive with password_env and keychain_service/keychain_account." + "description": "Credential reference: name of the environment variable that holds the password (e.g. \"DB_PASSWORD\"). The reference string is stored/used, never the secret value. Mutually exclusive with keychain_service/keychain_account. Use this instead of inline 'password' (which is rejected)." }, "keychain_service": { "type": "string", - "description": "Credential reference: OS keychain service name. Must be paired with keychain_account. The password is looked up from the platform keychain at connection time. Mutually exclusive with password_env and password_command." + "description": "Credential reference: OS keychain service name. Must be paired with keychain_account. The password is looked up from the platform keychain at connection time. Mutually exclusive with password_env." }, "keychain_account": { "type": "string", @@ -573,7 +561,7 @@ fn handle_list_tools() -> Result { "save": { "type": "string", "enum": ["local", "global"], - "description": "SAVE MODE: persist this connection config by reference. \"local\" writes .plenum/config.json (team-shareable, per-project); \"global\" writes ~/.config/plenum/connections.json (per-user). Requires explicit 'engine' + connection params. Credentials are stored only as references (password_env/password_command/keychain_*) — never plaintext. Omit to run in TEST mode instead." + "description": "SAVE MODE: persist this connection config by reference. \"local\" writes .plenum/config.json (team-shareable, per-project); \"global\" writes ~/.config/plenum/connections.json (per-user). Requires explicit 'engine' + connection params. Credentials are stored only as references (password_env/keychain_*) — never plaintext. Omit to run in TEST mode instead." }, "project_path": { "type": "string", @@ -598,6 +586,17 @@ async fn handle_call_tool(params: Option, binding: &McpBinding) -> Result let name = params["name"].as_str().ok_or_else(|| anyhow!("Missing tool name"))?; let arguments = ¶ms["arguments"]; + // Capability boundary: `password_command` runs a shell command, so accepting + // it from tool arguments would let an agent execute arbitrary commands on the + // host. Reject it for every tool before anything runs. + if arguments.get("password_command").is_some() { + return Err(anyhow!( + "CAPABILITY_VIOLATION: 'password_command' is not permitted on MCP tools because \ + it executes a shell command. Reference the secret via 'password_env' or \ + 'keychain_service' + 'keychain_account'." + )); + } + match name { "connect" => tool_connect(arguments, binding).await, "introspect" => tool_introspect(arguments, binding).await, @@ -621,16 +620,15 @@ async fn handle_call_tool(params: Option, binding: &McpBinding) -> Result /// connections offline. /// /// Inline plaintext passwords are rejected unconditionally: callers must source -/// secrets via `password_env`, `password_command`, or `keychain_service` + -/// `keychain_account`. This guarantees the plaintext secret never passes through -/// Plenum. +/// secrets via `password_env` or `keychain_service` + `keychain_account`. This +/// guarantees the plaintext secret never passes through Plenum. async fn tool_connect(args: &Value, binding: &McpBinding) -> Result { // Capability boundary: the MCP connect tool never handles plaintext secrets. if args.get("password").and_then(|v| v.as_str()).is_some() { return Err(anyhow!( "CAPABILITY_VIOLATION: inline plaintext 'password' is not permitted on the MCP \ - connect tool. Reference the secret instead via 'password_env', 'password_command', \ - or 'keychain_service' + 'keychain_account'." + connect tool. Reference the secret instead via 'password_env' or \ + 'keychain_service' + 'keychain_account'." )); } @@ -701,8 +699,8 @@ fn tool_connect_save(args: &Value, binding: &McpBinding, save_raw: &Value) -> Re anyhow!("'save' requires explicit connection params, starting with 'engine'") })?; - let (password_env, password_command, keychain_entry) = parse_credential_refs(args)?; - let has_ref = password_env.is_some() || password_command.is_some() || keychain_entry.is_some(); + let (password_env, keychain_entry) = parse_credential_refs(args)?; + let has_ref = password_env.is_some() || keychain_entry.is_some(); // Build the config to store. `allow_missing_password` keeps the config free of // any inline secret — the reference is the sole password authority. @@ -726,7 +724,7 @@ fn tool_connect_save(args: &Value, binding: &McpBinding, save_raw: &Value) -> Re name.clone(), config, password_env.clone(), - password_command.clone(), + None, keychain_entry.clone(), location, ) @@ -734,8 +732,6 @@ fn tool_connect_save(args: &Value, binding: &McpBinding, save_raw: &Value) -> Re let credential_source = if password_env.is_some() { "password_env" - } else if password_command.is_some() { - "password_command" } else if keychain_entry.is_some() { "keychain" } else { @@ -981,21 +977,19 @@ async fn tool_query(args: &Value, binding: &McpBinding) -> Result { /// Extract credential-reference arguments from a tool call. /// -/// Recognizes `password_env`, `password_command`, and the keychain pair -/// `keychain_service` + `keychain_account`. These name where a secret lives; -/// the plaintext secret itself never passes through Plenum. +/// Recognizes `password_env` and the keychain pair `keychain_service` + +/// `keychain_account`. (`password_command` is rejected earlier, in +/// `handle_call_tool`, because it executes a shell command.) These name where +/// a secret lives; the plaintext secret itself never passes through Plenum. /// /// Enforces the same invariants the config layer does: /// - at most one credential source may be specified; /// - `keychain_service` and `keychain_account` must be supplied together. /// -/// Returns `(password_env, password_command, keychain_entry)` — all `None` when -/// no reference was provided. -fn parse_credential_refs( - args: &Value, -) -> Result<(Option, Option, Option)> { +/// Returns `(password_env, keychain_entry)` — both `None` when no reference +/// was provided. +fn parse_credential_refs(args: &Value) -> Result<(Option, Option)> { let password_env = args.get("password_env").and_then(|v| v.as_str()).map(String::from); - let password_command = args.get("password_command").and_then(|v| v.as_str()).map(String::from); let service = args.get("keychain_service").and_then(|v| v.as_str()).map(String::from); let account = args.get("keychain_account").and_then(|v| v.as_str()).map(String::from); @@ -1009,27 +1003,21 @@ fn parse_credential_refs( } }; - let source_count = - [password_env.is_some(), password_command.is_some(), keychain_entry.is_some()] - .iter() - .filter(|&&b| b) - .count(); - - if source_count > 1 { + if password_env.is_some() && keychain_entry.is_some() { return Err(anyhow!( "Only one credential reference is allowed: \ - password_env, password_command, or keychain_service/keychain_account" + password_env or keychain_service/keychain_account" )); } - Ok((password_env, password_command, keychain_entry)) + Ok((password_env, keychain_entry)) } /// Build `ConnectionConfig` from JSON arguments. /// /// When `allow_missing_password` is true the inline `password` field is /// optional — the caller is expected to supply a credential reference -/// (`password_env` / `password_command` / `keychain_service`+`keychain_account`) +/// (`password_env` / `keychain_service`+`keychain_account`) /// that resolves the secret at connection time. The resulting config carries /// `password: None` so the reference is the sole authority. fn build_connection_config_from_args( @@ -1178,9 +1166,8 @@ fn resolve_connection_from_args( // Scenario 2: Explicit connection parameters if has_engine { let engine_str = args["engine"].as_str().unwrap(); - let (password_env, password_command, keychain_entry) = parse_credential_refs(args)?; - let has_ref = - password_env.is_some() || password_command.is_some() || keychain_entry.is_some(); + let (password_env, keychain_entry) = parse_credential_refs(args)?; + let has_ref = password_env.is_some() || keychain_entry.is_some(); let config = build_connection_config_from_args(args, engine_str, has_ref)?; @@ -1191,7 +1178,7 @@ fn resolve_connection_from_args( let stored = StoredConnection { config, password_env, - password_command, + password_command: None, keychain_entry, readonly: None, }; @@ -1465,16 +1452,16 @@ mod tests { #[test] fn parse_credential_refs_none_when_absent() { - let (env, cmd, kc) = parse_credential_refs(&serde_json::json!({})).unwrap(); - assert!(env.is_none() && cmd.is_none() && kc.is_none()); + let (env, kc) = parse_credential_refs(&serde_json::json!({})).unwrap(); + assert!(env.is_none() && kc.is_none()); } #[test] fn parse_credential_refs_env() { - let (env, cmd, kc) = + let (env, kc) = parse_credential_refs(&serde_json::json!({ "password_env": "DB_PASSWORD" })).unwrap(); assert_eq!(env.as_deref(), Some("DB_PASSWORD")); - assert!(cmd.is_none() && kc.is_none()); + assert!(kc.is_none()); } #[test] @@ -1487,7 +1474,11 @@ mod tests { #[test] fn parse_credential_refs_rejects_multiple_sources() { - let args = serde_json::json!({ "password_env": "A", "password_command": "echo b" }); + let args = serde_json::json!({ + "password_env": "A", + "keychain_service": "svc", + "keychain_account": "acct" + }); let err = parse_credential_refs(&args).unwrap_err().to_string(); assert!(err.contains("Only one credential reference"), "got: {err}"); } diff --git a/tests/mcp_bound_connection.rs b/tests/mcp_bound_connection.rs index c05a9b2..53d0859 100644 --- a/tests/mcp_bound_connection.rs +++ b/tests/mcp_bound_connection.rs @@ -519,3 +519,87 @@ fn connect_save_requires_a_credential_reference_shape() { let _ = std::fs::remove_dir_all(&cwd); let _ = std::fs::remove_dir_all(&xdg); } + +// ─── password_command is not accepted from MCP tool arguments ──────────────── +// +// `password_command` runs a shell command (`sh -c`). Accepting it from tool +// arguments would let any agent execute arbitrary commands on the host, so +// every MCP tool rejects it before anything runs. The CLI flag and wizard +// remain available to humans. + +/// Call `tool` with `password_command` set to a command that creates a marker +/// file, then assert a capability error and that the command never ran. +fn assert_password_command_rejected(tool: &str, tag: &str, mut arguments: Value) { + let cwd = scratch(tag); + let xdg = scratch(&format!("{tag}-xdg")); + let marker = cwd.join("executed"); + arguments["password_command"] = json!(format!("touch {}", marker.display())); + + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call(&mut child, tool, &arguments); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("CAPABILITY_VIOLATION"), "must be a capability error: {msg}"); + assert!(msg.contains("password_command"), "must name the rejected field: {msg}"); + assert!(!marker.exists(), "password_command must never execute"); + assert!(!cwd.join(".plenum").exists(), "nothing should be written"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_rejects_password_command() { + assert_password_command_rejected( + "connect", + "pwcmd-connect", + json!({ "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d" }), + ); +} + +#[test] +fn connect_save_rejects_password_command() { + let cwd = scratch("pwcmd-save-path"); + assert_password_command_rejected( + "connect", + "pwcmd-save", + json!({ + "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d", + "save": "global", "project_path": cwd.to_str().unwrap() + }), + ); + let _ = std::fs::remove_dir_all(&cwd); +} + +#[test] +fn introspect_rejects_password_command() { + assert_password_command_rejected( + "introspect", + "pwcmd-introspect", + json!({ "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d" }), + ); +} + +#[test] +fn query_rejects_password_command() { + assert_password_command_rejected( + "query", + "pwcmd-query", + json!({ + "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d", + "sql": "SELECT 1" + }), + ); +} + +#[test] +fn password_command_is_rejected_even_with_a_named_connection() { + // A named-connection call must not silently ignore the field either. + assert_password_command_rejected( + "query", + "pwcmd-named", + json!({ "name": "dev", "sql": "SELECT 1" }), + ); +} From 0780d149ee3a68787a112802c1b995f63117387d Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Tue, 6 Oct 2026 20:22:06 -0400 Subject: [PATCH 7/9] docs: document MCP binding, credential references, and auth-revamp changes - README: "Zero to Query" CLI-first and MCP-first paths; `plenum mcp` --project-path/--name/--dsn-env binding; cwd-coupling warning; MCP tool field reference; credential sourcing safety rules. password_command is documented as CLI-only. - SECURITY.md: MCP credential references, password_command rejection and its residual risk for saved configs, and MCP connection binding. - CHANGELOG: entries for REF-297, REF-298, REF-299, REF-303, REF-304 and the password_command rejection. - CLAUDE.md: MCP connection binding flags and credential-reference rules. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 +++++ CLAUDE.md | 14 ++++++ README.md | 125 +++++++++++++++++++++++++++++++++++++++++++++++---- SECURITY.md | 21 +++++++++ 4 files changed, 162 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8ef1e7..567c011 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `plenum mcp` connection binding at server registration: `--project-path`, `--name`, and `--dsn-env` decouple resolution from the launcher's cwd ([REF-297](/REF/issues/REF-297)) +- MCP `connect` save mode (`save`: `"local"` | `"global"`, `project_path`, `name`) that persists connections by credential reference; `password_env` and `keychain_service`/`keychain_account` accepted on `connect`, `introspect`, and `query` ([REF-298](/REF/issues/REF-298)) +- `plenum connect` wizard offers plaintext, env var, shell command, and OS keychain credential sources ([REF-299](/REF/issues/REF-299)) +- `mcp_stanza` in `plenum connect --save` output: a ready-to-paste `mcpServers` block for the saved connection ([REF-299](/REF/issues/REF-299)) +- JSON Schema `schemas/connect_save_success.json` for the `connect --save` envelope, covered by schema drift ([REF-304](/REF/issues/REF-304)) - `--diff-against` flag for `plenum introspect` — read-only schema diff against a prior snapshot ([REF-281](/REF/issues/REF-281)) - Live-DB test harness: Docker Compose fixtures, vendor seed SQL per engine, gated test suites, and `scripts/test-live.sh` orchestration ([REF-275](/REF/issues/REF-275)) - MySQL 8.0 and 8.4 live test matrix: connect, introspect, query, safety, and envelope coverage ([REF-276](/REF/issues/REF-276)) @@ -27,11 +32,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `plenum connect --project-path X --save local` reported success but persisted nothing when run from another directory; it now writes `X/.plenum/config.json` ([REF-303](/REF/issues/REF-303)) - PostgreSQL: correct NULL detection, composite foreign-key introspection, and view definitions ([REF-277](/REF/issues/REF-277)) - MySQL: route text-protocol statements correctly; classify `EXPLAIN`; surface timeout as a first-class error ([REF-258](/REF/issues/REF-258)) - Build: vendor OpenSSL for hermetic release builds ([REF-258](/REF/issues/REF-258)) - Capability: reject `WITH`-CTE DML bypasses across all engines ([REF-41](/REF/issues/REF-41)) +### Security + +- MCP `connect` rejects inline plaintext `password` with `CAPABILITY_VIOLATION` ([REF-298](/REF/issues/REF-298)) +- All MCP tools reject `password_command` with `CAPABILITY_VIOLATION`: it executes a shell command, so accepting it from tool arguments gave agents arbitrary command execution. The CLI flag and wizard still offer it. + ### Changed - SQLite: tighten read-only `PRAGMA` allowlist ([REF-44](/REF/issues/REF-44)) diff --git a/CLAUDE.md b/CLAUDE.md index d462053..dde3deb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -266,6 +266,20 @@ Plenum is exposed via a local MCP server. The Plenum CLI remains the execution boundary. +**Connection binding (`plenum mcp` flags):** +- `--project-path ` — resolve config for that project, not the server's cwd +- `--name ` — select a named connection within that project +- `--dsn-env ` — read the DSN from exactly that env var; never from ambient vars such as `DATABASE_URL` +- `--dsn-env` conflicts with `--project-path` and `--name` +- Connection fields in a tool call override the binding +- No binding and no cwd config → structured error naming the flags to add + +**Credential references on MCP tools:** +- Accepted: `password_env`, `keychain_service` + `keychain_account` (at most one source per call) +- `connect` rejects inline plaintext `password` with `CAPABILITY_VIOLATION` +- Every tool rejects `password_command` with `CAPABILITY_VIOLATION` — it executes a shell command, so it is CLI-only (a human types it) +- `connect` with `save` (`"local"` | `"global"`) stores the reference, never the secret + --- ## Rust Architecture Expectations diff --git a/README.md b/README.md index 174468f..3bd8008 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,70 @@ cargo build --release - Rust 1.70 or later - Supported platforms: Linux, macOS, Windows +## Zero to Query + +Two paths from nothing to a working query result. + +### Path A — CLI-first (recommended for most setups) + +**1. Save a connection:** + +```bash +# Save a PostgreSQL connection to the current project (password via env var — never plaintext) +plenum connect --name dev --engine postgres \ + --host localhost --port 5432 --user myuser \ + --password-env PGPASSWORD --database mydb --save local +``` + +**2. Query immediately:** + +```bash +plenum query --sql "SELECT COUNT(*) FROM users" +# => {"ok":true,"engine":"postgres","command":"query","data":{"rows":[{"count":42}]},"meta":{...}} +``` + +Plenum resolves the `dev` connection automatically from `.plenum/config.json` because it is the default for this project directory. + +--- + +### Path B — MCP-first (for AI coding agents via Claude Desktop or editors) + +Register Plenum in your MCP client config with the project path bound at registration time. **You must supply `--project-path`** — see the [cwd-Coupling Warning](#cwd-coupling-warning) below. + +**Option 1 — use a saved named connection:** + +```json +{ + "mcpServers": { + "plenum": { + "command": "plenum", + "args": ["mcp", "--project-path", "/abs/path/to/your/project", "--name", "dev"] + } + } +} +``` + +Run `plenum connect` first (Path A) to save the connection, then add the stanza above. + +**Option 2 — use a DSN from a named env var (no saved config needed):** + +```json +{ + "mcpServers": { + "plenum": { + "command": "plenum", + "args": ["mcp", "--dsn-env", "DATABASE_URL"] + } + } +} +``` + +`DATABASE_URL` is read at connection time from that env var only. No ambient credential scanning. + +Once the MCP server is running the agent can call the `query` tool without any further setup. + +--- + ## Usage Plenum provides exactly three commands: @@ -355,29 +419,63 @@ Agents should: ## MCP Integration -Plenum exposes functionality via MCP (Model Context Protocol) server: +Plenum exposes all three CLI commands as MCP tools via a local MCP server. The server is started with `plenum mcp` and connection binding is controlled at registration time: ```bash -# Start MCP server (hidden command, for AI agent use) -plenum mcp +plenum mcp --project-path /abs/path/to/project # pin to a project (recommended) +plenum mcp --project-path /abs/path/to/project --name prod # pin to a named connection +plenum mcp --dsn-env DATABASE_URL # read DSN from a named env var ``` -Configure in your MCP client: +### cwd-Coupling Warning + +When `plenum mcp` is started **without** `--project-path` or `--dsn-env`, connection resolution falls back to the server process's working directory. MCP clients (Claude Desktop, editors, daemons) launch `plenum mcp` from arbitrary directories — often not your project root. The result is a failed resolution, not a wrong connection: the tool call fails with a structured error that names the missing flag to add. + +**Always supply `--project-path` or `--dsn-env` in your `mcpServers` stanza.** + +### mcpServers configuration + ```json { "mcpServers": { "plenum": { "command": "plenum", - "args": ["mcp"] + "args": ["mcp", "--project-path", "/abs/path/to/your/project", "--name", "dev"] } } } ``` -Each CLI command maps to an MCP tool: -- `connect` → Validate and save database connections -- `introspect` → Retrieve schema information -- `query` → Execute constrained SQL queries +### MCP tools + +Each CLI command maps to one MCP tool. Tool-call arguments mirror the CLI flags and take precedence over the binding set at server start. + +#### `connect` tool + +Validates and persists a database connection **by credential reference**. Inline plaintext passwords and `password_command` are rejected with `CAPABILITY_VIOLATION` at the MCP boundary — never pass raw secrets through tool arguments. + +| Field | Description | +|-------|-------------| +| `engine` | `postgres`, `mysql`, `sqlite`, or `duckdb` | +| `host`, `port`, `user`, `database` | Server coordinates (postgres/mysql) | +| `file` | File path (sqlite/duckdb) | +| `password_env` | Name of env var holding the password | +| `keychain_service` + `keychain_account` | OS keychain lookup pair (both required together) | +| `save` | `"local"` (`.plenum/config.json`) or `"global"` (`~/.config/plenum/connections.json`) | +| `project_path` | Absolute path of the project to save the connection under | +| `name` | Connection name (defaults to `"default"`) | + +At most one credential-reference source (`password_env` or the keychain pair) may be supplied per call. + +`password_command` is rejected on every MCP tool because it executes a shell command. Accepting it from tool arguments would let an agent run arbitrary commands on the host. Use `--password-command` from the CLI (`plenum connect`) instead — a human types it there. + +#### `introspect` tool + +Retrieves schema information. Accepts the same credential-reference fields (`password_env`, `keychain_service`/`keychain_account`) for one-off explicit connections — secrets are resolved at connect time and never appear in tool arguments or logs. + +#### `query` tool + +Executes a constrained, read-only SQL query. Accepts the same credential-reference fields as `introspect`. ## Architecture @@ -402,6 +500,15 @@ Credentials are stored as **plaintext JSON** in config files: - Secure config files with OS-level permissions (`chmod 600`) - Avoid `--password` CLI flag (visible in process listings) +#### Credential Sourcing Safety Rules + +Plenum's credential sourcing is deterministic and reference-based. These rules hold unconditionally: + +1. **Explicit reference only** — The MCP `connect` tool rejects inline plaintext passwords with `CAPABILITY_VIOLATION`. Credentials flow through Plenum only as named references (`password_env`, `keychain_service`/`keychain_account`). MCP tools reject `password_command` because it executes a shell command; it is available only from the CLI. The CLI `--password` flag is available but strongly discouraged (visible in process listings and shell history). +2. **No ambient pickup** — `--dsn-env` reads exactly the one env var you name. Plenum never scans `DATABASE_URL`, `PGPASSWORD`, or any other ambient variable on its own. +3. **No scanning** — Plenum reads only the two fixed config file paths (`.plenum/config.json` and `~/.config/plenum/connections.json`). No directory walking, no inference from arbitrary paths. +4. **Deterministic resolution** — Given the same `--project-path`, `--name`, and env vars, the same connection is always resolved. The precedence order is: explicit CLI flags → local config → global config → structured error. + #### Security Reporting For detailed security documentation, threat model, and vulnerability reporting, see **[SECURITY.md](SECURITY.md)**. diff --git a/SECURITY.md b/SECURITY.md index 921398d..6d4e777 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -172,6 +172,27 @@ When using the MCP server, credentials are passed per-invocation via JSON-RPC: **Security Note**: MCP communication over stdio is local-only (no network exposure). +### MCP Credential References + +The MCP `connect` tool rejects an inline plaintext `password` with `CAPABILITY_VIOLATION`. Agents reference the secret instead: + +- `password_env` — the name of an environment variable that holds the password. +- `keychain_service` + `keychain_account` — an OS keychain entry (both are required together). + +**`password_command` is rejected on every MCP tool** (`connect`, `introspect`, `query`) with `CAPABILITY_VIOLATION`. It executes a shell command via `sh -c`. Accepting it from tool arguments would give any connected agent arbitrary command execution on the host. The CLI flag `--password-command` and the `plenum connect` wizard still offer it, because a human types the command there. + +**Residual risk — saved configs:** a connection already saved with `password_command` still runs that command when the MCP server resolves it. The command comes from a config file, not from tool arguments. Review `password_command` entries in `.plenum/config.json` before you commit or pull that file, because it is team-shareable. + +### MCP Connection Binding + +`plenum mcp` binds its default connection at server start: + +- `--project-path ` — resolve config for that project, independent of the server's working directory. +- `--name ` — select a named connection within that project. +- `--dsn-env ` — read the DSN from exactly the named environment variable. Plenum never reads `DATABASE_URL`, `PGPASSWORD`, or any other ambient variable on its own. + +`--dsn-env` cannot be combined with `--project-path` or `--name`. Connection fields in a tool call override the binding. + --- ## SQL Injection & Query Validation From ddb894775ea88d67f3526d14faf6971222127bba Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Tue, 6 Oct 2026 21:21:23 -0400 Subject: [PATCH 8/9] fix(duckdb): support duckdb 1.10506.0 non-exhaustive Value CI failed to compile: Cargo.lock is not tracked, so CI resolved duckdb 1.10506.0, where `duckdb::types::Value` is `#[non_exhaustive]` and gains `UHugeInt(u128)` and `Geometry(Vec)`. The match in duckdb_value_to_json had no wildcard arm (E0004). - UHUGEINT converts to a string, like HUGEINT, to preserve precision. - GEOMETRY (WKB bytes) converts to Base64, like BLOB. - duckdb_value_to_json now returns Result: an unknown future variant is a structured QUERY_FAILED error naming the type, never a guessed value. - Raise the duckdb minimum to 1.10506.0 (the new variants exist only there). Tests: unit tests for UHugeInt and Geometry conversion; an E2E query of the UHUGEINT maximum. Verified with all dependencies updated to latest (matching CI): fmt, clippy -D warnings, cargo test --all-features (505 passed), and live MySQL 8.0/8.4 + PostgreSQL 16 suites (56 passed). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + Cargo.toml | 2 +- src/engine/duckdb/mod.rs | 79 +++++++++++++++++++++++++++++++++------- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 567c011..e4a53c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- DuckDB: compile against `duckdb` 1.10506.0, where `Value` is `#[non_exhaustive]`; `UHUGEINT` converts to a string, `GEOMETRY` (WKB) to Base64, and an unknown value type is a structured `QUERY_FAILED` error instead of a guess - `plenum connect --project-path X --save local` reported success but persisted nothing when run from another directory; it now writes `X/.plenum/config.json` ([REF-303](/REF/issues/REF-303)) - PostgreSQL: correct NULL detection, composite foreign-key introspection, and view definitions ([REF-277](/REF/issues/REF-277)) - MySQL: route text-protocol statements correctly; classify `EXPLAIN`; surface timeout as a first-class error ([REF-258](/REF/issues/REF-258)) diff --git a/Cargo.toml b/Cargo.toml index 43849ad..39c33a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,7 +65,7 @@ postgres-native-tls = { version = "0.5", optional = true } # TLS for tokio-post native-tls = { version = "0.2", optional = true, features = ["vendored"] } # Platform TLS (OpenSSL/Secure Transport/SChannel) mysql_async = { version = "0.34", default-features = false, features = ["default-rustls"], optional = true } # MySQL native async driver rusqlite = { version = "0.32", features = ["bundled"], optional = true } # SQLite native driver with bundled lib -duckdb = { version = "1.10504.0", features = ["bundled"], optional = true } # DuckDB native driver with bundled lib +duckdb = { version = "1.10506.0", features = ["bundled"], optional = true } # DuckDB native driver with bundled lib # BLOB encoding (Base64) - used by SQLite and PostgreSQL base64 = "0.22" diff --git a/src/engine/duckdb/mod.rs b/src/engine/duckdb/mod.rs index 3ef9037..22fc08e 100644 --- a/src/engine/duckdb/mod.rs +++ b/src/engine/duckdb/mod.rs @@ -689,7 +689,7 @@ fn execute_query( let value_ref = row.get_ref(idx).map_err(|e| { PlenumError::query_failed(format!("Failed to read column {idx}: {e}")) })?; - values.push(duckdb_value_to_json(&value_ref.to_owned())); + values.push(duckdb_value_to_json(&value_ref.to_owned())?); } rows_data.push(values); pos += 1; @@ -748,14 +748,17 @@ fn f64_to_json(f: f64) -> serde_json::Value { /// represent natively are stringified deterministically: /// - `HUGEINT` / `UHUGEINT` and `DECIMAL` → string (preserves precision) /// - `TIMESTAMP` / `DATE` / `TIME` → ISO-8601 string -/// - `BLOB` → Base64 string +/// - `BLOB` and `GEOMETRY` (WKB bytes) → Base64 string /// - `INTERVAL` → object with `months` / `days` / `nanos` /// - Nested types (`LIST`, `ARRAY`, `STRUCT`, `MAP`, `UNION`, `ENUM`) convert /// recursively to JSON arrays / objects. -fn duckdb_value_to_json(value: &Value) -> serde_json::Value { +/// +/// `duckdb::types::Value` is `#[non_exhaustive]`. A variant this function does +/// not know is a structured error, never a guessed representation. +fn duckdb_value_to_json(value: &Value) -> Result { use serde_json::Value as Json; - match value { + let json = match value { Value::Null => Json::Null, Value::Boolean(b) => Json::Bool(*b), Value::TinyInt(i) => Json::Number((*i).into()), @@ -764,6 +767,7 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { Value::BigInt(i) => Json::Number((*i).into()), // HUGEINT exceeds JSON's i64 range; preserve precision as a string Value::HugeInt(i) => Json::String(i.to_string()), + Value::UHugeInt(i) => Json::String(i.to_string()), Value::UTinyInt(i) => Json::Number((*i).into()), Value::USmallInt(i) => Json::Number((*i).into()), Value::UInt(i) => Json::Number((*i).into()), @@ -774,7 +778,7 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { Value::Decimal(d) => Json::String(d.to_string()), Value::Timestamp(unit, v) => format_timestamp(*unit, *v), Value::Text(s) | Value::Enum(s) => Json::String(s.clone()), - Value::Blob(b) => { + Value::Blob(b) | Value::Geometry(b) => { use base64::Engine; Json::String(base64::engine::general_purpose::STANDARD.encode(b)) } @@ -786,31 +790,40 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { "nanos": nanos, }), Value::List(items) | Value::Array(items) => { - Json::Array(items.iter().map(duckdb_value_to_json).collect()) + Json::Array(items.iter().map(duckdb_value_to_json).collect::>()?) } Value::Struct(map) => { - let obj: serde_json::Map = - map.iter().map(|(k, v)| (k.clone(), duckdb_value_to_json(v))).collect(); + let obj = map + .iter() + .map(|(k, v)| Ok((k.clone(), duckdb_value_to_json(v)?))) + .collect::>>()?; Json::Object(obj) } Value::Map(map) => { - let obj: serde_json::Map = map + let obj = map .iter() .map(|(k, v)| { let key = match k { Value::Text(s) | Value::Enum(s) => s.clone(), - other => match duckdb_value_to_json(other) { + other => match duckdb_value_to_json(other)? { Json::String(s) => s, j => j.to_string(), }, }; - (key, duckdb_value_to_json(v)) + Ok((key, duckdb_value_to_json(v)?)) }) - .collect(); + .collect::>>()?; Json::Object(obj) } - Value::Union(inner) => duckdb_value_to_json(inner), - } + Value::Union(inner) => duckdb_value_to_json(inner)?, + other => { + return Err(PlenumError::query_failed(format!( + "Unsupported DuckDB value type in result set: {:?}", + other.data_type() + ))); + } + }; + Ok(json) } #[cfg(test)] @@ -1181,6 +1194,44 @@ mod tests { let _ = std::fs::remove_file(&temp_file); } + #[test] + fn uhugeint_converts_to_a_string() { + // UHUGEINT exceeds JSON's number range; preserve precision as a string. + let json = duckdb_value_to_json(&Value::UHugeInt(u128::MAX)).expect("convert"); + assert_eq!(json, serde_json::json!("340282366920938463463374607431768211455")); + } + + #[test] + fn geometry_converts_to_base64_wkb() { + // GEOMETRY arrives as WKB bytes; encode like BLOB. + let json = + duckdb_value_to_json(&Value::Geometry(vec![0xDE, 0xAD, 0xBE, 0xEF])).expect("convert"); + assert_eq!(json, serde_json::json!("3q2+7w==")); + } + + #[tokio::test] + async fn test_execute_uhugeint_max_value() { + let temp_file = fixture_path("uhugeint"); + let _ = std::fs::remove_file(&temp_file); + { + let conn = Connection::open(&temp_file).expect("create"); + conn.execute_batch( + "CREATE TABLE big (v UHUGEINT); + INSERT INTO big VALUES (340282366920938463463374607431768211455)", + ) + .expect("seed"); + } + + let config = ConnectionConfig::duckdb(temp_file.clone()); + let caps = Capabilities::default(); + let qr = DuckDbEngine::execute(&config, "SELECT v FROM big", &[], &caps) + .await + .expect("query failed"); + assert_eq!(qr.rows[0][0], serde_json::json!("340282366920938463463374607431768211455")); + + let _ = std::fs::remove_file(&temp_file); + } + #[tokio::test] async fn test_introspect_foreign_keys() { let temp_file = fixture_path("fk"); From e5fc9c0ac533913277557b8878737fe9ede767ea Mon Sep 17 00:00:00 2001 From: therecluse26 Date: Tue, 6 Oct 2026 21:46:57 -0400 Subject: [PATCH 9/9] fix(clippy): satisfy Rust 1.99 lints unused_async_trait_impl and assert_is_empty CI runs clippy on unpinned `stable`, now Rust 1.99, which adds two lints: - `clippy::unused_async_trait_impl` (split from `unused_async` in 1.98) fires on the 3 DatabaseEngine methods in the SQLite and DuckDB impls. Their drivers are synchronous, but the trait requires `async fn`, so the lint cannot be satisfied without hand-rolled `impl Future` bodies. Allow it on the two impl blocks. `#[allow(unknown_lints)]` keeps older toolchains (e.g. 1.97) from rejecting the new lint name under -D warnings. Same approach as the closed PR #14 (8407dbe). - `clippy::assert_is_empty` (new in 1.99) fires on 13 test asserts. Rewrite them as assert_eq!/assert_ne! so a failure shows the value. Verified: clippy --all-targets --all-features -D warnings clean on both 1.97 and 1.99; fmt on both; cargo test --all-features (505 passed); live MySQL 8.0/8.4 + PostgreSQL 16 suites (56 passed). Co-Authored-By: Claude Opus 5.5 --- src/diff.rs | 12 ++++++------ src/engine/duckdb/mod.rs | 8 +++++++- src/engine/mysql/mod.rs | 6 +++--- src/engine/postgres/mod.rs | 6 +++--- src/engine/sqlite/mod.rs | 6 ++++++ 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 9acf39a..0d72e3a 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -450,8 +450,8 @@ mod tests { fn test_identical_tables_produce_empty_diff() { let tables = vec![make_table("users", vec![make_col("id", "int", false)])]; let (added, removed, changed) = diff_tables(&tables, &tables); - assert!(added.is_empty()); - assert!(removed.is_empty()); + assert_eq!(added, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(changed.is_empty()); } @@ -464,7 +464,7 @@ mod tests { ]; let (added, removed, _) = diff_tables(&base, &target); assert_eq!(added, vec!["orders"]); - assert!(removed.is_empty()); + assert_eq!(removed, Vec::::new()); } #[test] @@ -475,7 +475,7 @@ mod tests { ]; let target = vec![make_table("users", vec![make_col("id", "int", false)])]; let (added, removed, _) = diff_tables(&base, &target); - assert!(added.is_empty()); + assert_eq!(added, Vec::::new()); assert_eq!(removed, vec!["orders"]); } @@ -592,8 +592,8 @@ mod tests { let views = vec![make_view("v_active", Some("SELECT 1"), vec![make_col("id", "int", false)])]; let (added, removed, changed) = diff_views(&views, &views); - assert!(added.is_empty()); - assert!(removed.is_empty()); + assert_eq!(added, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(changed.is_empty()); } diff --git a/src/engine/duckdb/mod.rs b/src/engine/duckdb/mod.rs index 22fc08e..6246b35 100644 --- a/src/engine/duckdb/mod.rs +++ b/src/engine/duckdb/mod.rs @@ -39,6 +39,12 @@ pub struct DuckDbEngine; /// Default schema used when no `--schema` is provided. const DEFAULT_SCHEMA: &str = "main"; +// `DatabaseEngine` declares these methods as returning futures, so each impl must +// be `async` whether or not its body awaits. The DuckDB driver is synchronous, so +// no method here awaits. `clippy::unused_async_trait_impl` (Rust 1.98+) flags +// that; `unknown_lints` keeps older toolchains from rejecting the new lint name. +#[allow(unknown_lints)] +#[allow(clippy::unused_async_trait_impl)] impl DatabaseEngine for DuckDbEngine { async fn validate_connection(config: &ConnectionConfig) -> Result { let file_path = extract_file_path(config)?; @@ -1461,7 +1467,7 @@ mod tests { assert!(result.is_ok(), "native explain failed: {:?}", result.err()); let qr = result.unwrap(); assert!(qr.plan.is_none()); - assert!(!qr.rows.is_empty()); + assert_ne!(qr.rows.len(), 0); } // ========================================================================= diff --git a/src/engine/mysql/mod.rs b/src/engine/mysql/mod.rs index 329e884..4724cce 100644 --- a/src/engine/mysql/mod.rs +++ b/src/engine/mysql/mod.rs @@ -1317,7 +1317,7 @@ mod tests { assert!(result.is_ok(), "Connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("MySQL") || info.server_info.contains("MariaDB")); } @@ -1337,7 +1337,7 @@ mod tests { assert!(result.is_ok(), "Wildcard connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("MySQL") || info.server_info.contains("MariaDB")); assert_eq!(info.connected_database, "(no database selected)"); } @@ -1360,7 +1360,7 @@ mod tests { let query_result = result.unwrap(); assert_eq!(query_result.columns.len(), 1); - assert!(!query_result.rows.is_empty()); + assert_ne!(query_result.rows.len(), 0); } #[tokio::test] diff --git a/src/engine/postgres/mod.rs b/src/engine/postgres/mod.rs index a28d276..3c829ce 100644 --- a/src/engine/postgres/mod.rs +++ b/src/engine/postgres/mod.rs @@ -1308,7 +1308,7 @@ mod tests { assert!(result.is_ok(), "Connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("PostgreSQL")); assert_eq!(info.connected_database, "postgres"); assert_eq!(info.user, "postgres"); @@ -1330,7 +1330,7 @@ mod tests { assert!(result.is_ok(), "Wildcard connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("PostgreSQL")); assert!(info.connected_database.contains("wildcard mode")); assert!(info.connected_database.contains("postgres")); @@ -1360,7 +1360,7 @@ mod tests { let query_result = result.unwrap(); assert_eq!(query_result.columns.len(), 1); - assert!(!query_result.rows.is_empty()); + assert_ne!(query_result.rows.len(), 0); } #[tokio::test] diff --git a/src/engine/sqlite/mod.rs b/src/engine/sqlite/mod.rs index 038b50b..7279951 100644 --- a/src/engine/sqlite/mod.rs +++ b/src/engine/sqlite/mod.rs @@ -31,6 +31,12 @@ use crate::error::{PlenumError, Result}; /// `SQLite` database engine implementation pub struct SqliteEngine; +// `DatabaseEngine` declares these methods as returning futures, so each impl must +// be `async` whether or not its body awaits. The SQLite driver is synchronous, so +// no method here awaits. `clippy::unused_async_trait_impl` (Rust 1.98+) flags +// that; `unknown_lints` keeps older toolchains from rejecting the new lint name. +#[allow(unknown_lints)] +#[allow(clippy::unused_async_trait_impl)] impl DatabaseEngine for SqliteEngine { async fn validate_connection(config: &ConnectionConfig) -> Result { // Validate config is for SQLite