Fix code review: env test races, zsh completions, reserved names, audit logging, OAuth port ordering

Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Author: avelino

docs/reference/cli.md

@@ -262,7 +262,7 @@ mcp add filesystem
 Fails if:
 - Server not found in registry
 - Server already exists in config
-- Name is reserved (`search`, `add`, `remove`, `list`, `help`, `version`, `serve`, `logs`, `config`, `completions`)
+- Name is reserved (`search`, `add`, `remove`, `update`, `list`, `help`, `version`, `serve`, `logs`, `acl`, `config`, `completions`, `healthcheck`)
 
 ### `mcp add --url <url> <name>`
 

src/audit.rs

@@ -251,9 +251,22 @@ impl AuditLogger {
                             Utc::now().timestamp_millis(),
                             uuid::Uuid::new_v4()
                         );
-                        if let Ok(doc) = serde_json::to_value(&entry) {
-                            if let Ok(db) = writer_pool.acquire() {
-                                let _ = db.put(&key, &doc, None);
+                        match serde_json::to_value(&entry) {
+                            Ok(doc) => match writer_pool.acquire() {
+                                Ok(db) => {
+                                    if let Err(e) = db.put(&key, &doc, None) {
+                                        tracing::warn!(error = ?e, "failed to write audit entry");
+                                    }
+                                }
+                                Err(e) => {
+                                    tracing::warn!(
+                                        error = format!("{e:#}"),
+                                        "failed to acquire db for audit"
+                                    );
+                                }
+                            },
+                            Err(e) => {
+                                tracing::warn!(error = %e, "failed to serialize audit entry");
                             }
                         }
                     }

src/auth/oauth.rs

@@ -72,7 +72,12 @@ pub async fn run_oauth_flow(server_url: &str) -> Result<String> {
 
     let metadata = discover_auth_server(&key).await?;
 
-    let client_id = match get_or_register_client(&key, &metadata).await {
+    // Bind callback listener BEFORE client registration so the redirect_uri
+    // in the registration request matches the actual port we're listening on.
+    let (listener, port) = bind_callback_listener().await?;
+    let redirect_uri = format!("http://localhost:{port}/callback");
+
+    let client_id = match get_or_register_client(&key, &metadata, &redirect_uri).await {
         Ok(id) => id,
         Err(e) => {
             tracing::warn!(error = format!("{e:#}"), "OAuth registration not available");
@@ -83,9 +88,6 @@ pub async fn run_oauth_flow(server_url: &str) -> Result<String> {
     let (code_verifier, code_challenge) = generate_pkce();
     let state = generate_random_string(32);
 
-    let (listener, port) = bind_callback_listener().await?;
-    let redirect_uri = format!("http://localhost:{port}/callback");
-
     let scopes = metadata.scopes_supported.join(" ");
     let mut auth_url = Url::parse(&metadata.authorization_endpoint)?;
     auth_url
@@ -235,6 +237,7 @@ async fn discover_auth_server(server_url: &str) -> Result<AuthServerMetadata> {
 async fn get_or_register_client(
     server_key_str: &str,
     metadata: &AuthServerMetadata,
+    redirect_uri: &str,
 ) -> Result<String> {
     let auth_store = store::load_auth_store()?;
     if let Some(reg) = auth_store.clients.get(server_key_str) {
@@ -249,7 +252,7 @@ async fn get_or_register_client(
     let http = reqwest::Client::new();
     let body = serde_json::json!({
         "client_name": "mcp",
-        "redirect_uris": ["http://localhost:8085/callback"],
+        "redirect_uris": [redirect_uri],
         "grant_types": ["authorization_code", "refresh_token"],
         "response_types": ["code"],
         "token_endpoint_auth_method": "none"

src/cli/completions.rs

@@ -176,7 +176,7 @@ _mcp() {{
             _describe 'subcommand' subcommands
             ;;
         args)
-            case $words[1] in
+            case $words[2] in
                 config)
                     _values 'config subcommand' 'path[Show config file path]' 'edit[Open config in editor]'
                     ;;

src/cli/config.rs

@@ -72,24 +72,61 @@ fn handle_config_edit() -> Result<()> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::sync::{Mutex, OnceLock};
+
+    fn env_lock() -> &'static Mutex<()> {
+        static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+        LOCK.get_or_init(|| Mutex::new(()))
+    }
+
+    struct EnvRestore {
+        editor: Option<String>,
+        visual: Option<String>,
+    }
+
+    impl EnvRestore {
+        fn capture() -> Self {
+            Self {
+                editor: std::env::var("EDITOR").ok(),
+                visual: std::env::var("VISUAL").ok(),
+            }
+        }
+    }
+
+    impl Drop for EnvRestore {
+        fn drop(&mut self) {
+            match &self.editor {
+                Some(v) => std::env::set_var("EDITOR", v),
+                None => std::env::remove_var("EDITOR"),
+            }
+            match &self.visual {
+                Some(v) => std::env::set_var("VISUAL", v),
+                None => std::env::remove_var("VISUAL"),
+            }
+        }
+    }
 
     #[test]
     fn test_resolve_editor_from_env() {
+        let _guard = env_lock().lock().unwrap();
+        let _restore = EnvRestore::capture();
         std::env::set_var("EDITOR", "nano");
         assert_eq!(resolve_editor(), "nano");
-        std::env::remove_var("EDITOR");
     }
 
     #[test]
     fn test_resolve_editor_visual_fallback() {
+        let _guard = env_lock().lock().unwrap();
+        let _restore = EnvRestore::capture();
         std::env::remove_var("EDITOR");
         std::env::set_var("VISUAL", "code");
         assert_eq!(resolve_editor(), "code");
-        std::env::remove_var("VISUAL");
     }
 
     #[test]
     fn test_resolve_editor_default() {
+        let _guard = env_lock().lock().unwrap();
+        let _restore = EnvRestore::capture();
         std::env::remove_var("EDITOR");
         std::env::remove_var("VISUAL");
         let editor = resolve_editor();
@@ -102,7 +139,6 @@ mod tests {
 
     #[test]
     fn test_config_path_json_output() {
-        // Verify the JSON mode produces valid JSON with expected fields
         let path = config::config_path().unwrap();
         let json = serde_json::json!({
             "path": path.to_string_lossy(),

src/config.rs

@@ -13,13 +13,16 @@ const RESERVED_NAMES: &[&str] = &[
     "search",
     "add",
     "remove",
+    "update",
     "list",
     "help",
     "version",
     "serve",
     "logs",
+    "acl",
     "config",
     "completions",
+    "healthcheck",
 ];
 
 #[derive(Debug, Default, Deserialize, Clone, PartialEq)]

src/serve/http.rs

@@ -136,10 +136,14 @@ pub async fn run_http(config: Config, bind_addr: &str, insecure: bool) -> Result
     let auth_provider = server_auth::build_auth_provider(&config.server_auth)?;
     let acl = config.server_auth.acl.clone();
 
-    let pool = crate::db::create_pool(&config.audit).unwrap_or_else(|e| {
-        tracing::warn!(error = format!("{e:#}"), "failed to create db pool");
+    let pool = if config.audit.output == crate::audit::AuditOutput::File {
+        crate::db::create_pool(&config.audit).unwrap_or_else(|e| {
+            tracing::warn!(error = format!("{e:#}"), "failed to create db pool");
+            Arc::new(crate::db::DbPool::disabled())
+        })
+    } else {
         Arc::new(crate::db::DbPool::disabled())
-    });
+    };
     let audit = AuditLogger::open(&config.audit, pool.clone()).unwrap_or(AuditLogger::Disabled);
     let cache_store = ToolCacheStore::new(pool);
     let mut server = ProxyServer::new(

src/serve/stdio.rs

@@ -14,11 +14,22 @@ use super::discovery::discover_pending_backends;
 use super::dispatch::dispatch_request;
 use super::proxy::{shutdown_clients_in_parallel, ProxyServer, SharedProxy};
 
-pub async fn run_stdio(config: Config) -> Result<()> {
-    let pool = crate::db::create_pool(&config.audit).unwrap_or_else(|e| {
-        tracing::warn!(error = format!("{e:#}"), "failed to create db pool");
+pub async fn run_stdio(mut config: Config) -> Result<()> {
+    // In stdio mode, stdout is the JSON-RPC transport. Redirect audit to stderr
+    // to avoid interleaving audit JSON lines with protocol responses.
+    if config.audit.output == crate::audit::AuditOutput::Stdout {
+        tracing::warn!("audit output=stdout conflicts with stdio transport, redirecting to stderr");
+        config.audit.output = crate::audit::AuditOutput::Stderr;
+    }
+
+    let pool = if config.audit.output == crate::audit::AuditOutput::File {
+        crate::db::create_pool(&config.audit).unwrap_or_else(|e| {
+            tracing::warn!(error = format!("{e:#}"), "failed to create db pool");
+            Arc::new(crate::db::DbPool::disabled())
+        })
+    } else {
         Arc::new(crate::db::DbPool::disabled())
-    });
+    };
     let audit = AuditLogger::open(&config.audit, pool.clone()).unwrap_or(AuditLogger::Disabled);
     let cache_store = ToolCacheStore::new(pool);
     let mut server = ProxyServer::new(