Validate local client selectors and ports

crates/clickhousectl/src/local/cli.rs

@@ -98,22 +98,23 @@ CONTEXT FOR AGENTS:
   Two connection modes:
   1. Named server: `clickhousectl local client --name dev` — looks up port and version from a
      locally managed server started via `clickhousectl local server start`. Defaults to \"default\".
-  2. Explicit host/port: `clickhousectl local client --host myhost --port 9000` — connects to any
-     ClickHouse server directly, bypassing local server lookup.
+  2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults
+     to localhost (for example, `local client --port 9000`); a missing port defaults to 9000.
+  Named and direct selectors cannot be combined.
   --query and --queries-file execute SQL inline or from a file.
   Additional clickhouse-client args can be passed after --.
   Related: `clickhousectl local server start` to start a local server, `clickhousectl local server list` to see servers.")]
     Client {
         /// Server name to connect to (default: "default")
-        #[arg(long, short)]
+        #[arg(long, short, conflicts_with_all = ["host", "port"])]
         name: Option<String>,
 
         /// Host to connect to (bypasses local server lookup)
         #[arg(long)]
         host: Option<String>,
 
         /// TCP port to connect to (bypasses local server lookup if set)
-        #[arg(long, short)]
+        #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))]
         port: Option<u16>,
 
         /// Execute a SQL query
@@ -420,26 +421,28 @@ CONTEXT FOR AGENTS:
   1. Named server: `clickhousectl local postgres client --name dev` — looks up the host port
      and credentials from a locally managed Postgres started via `local postgres start`.
      Defaults to \"default\".
-  2. Explicit host/port: `clickhousectl local postgres client --host myhost --port 5432`.
+  2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults
+     to 127.0.0.1; a missing port defaults to 5432.
+  Managed selectors (--name and --version) cannot be combined with direct selectors.
   If `psql` is on PATH on the host, it is execed directly. Otherwise, falls back to running
   `psql` inside the container via Docker exec (no host psql required).
   --query and --queries-file pass through to psql (-c / -f).
   Additional psql args can be passed after --.")]
     Client {
         /// Server name to connect to (default: "default")
-        #[arg(long, short)]
+        #[arg(long, short, conflicts_with_all = ["host", "port"])]
         name: Option<String>,
 
         /// Postgres version to disambiguate when multiple share a name
-        #[arg(long, short = 'v')]
+        #[arg(long, short = 'v', conflicts_with_all = ["host", "port"])]
         version: Option<String>,
 
         /// Host to connect to (bypasses local server lookup)
         #[arg(long)]
         host: Option<String>,
 
         /// TCP port to connect to (bypasses local server lookup if set)
-        #[arg(long, short)]
+        #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))]
         port: Option<u16>,
 
         /// Execute a single SQL query
@@ -483,13 +486,42 @@ mod tests {
     use clap::Parser;
 
     fn local_command(args: &[&str]) -> LocalCommands {
+        try_local_command(args).unwrap()
+    }
+
+    fn try_local_command(args: &[&str]) -> Result<LocalCommands, clap::Error> {
         let mut argv = vec!["clickhousectl", "local"];
         argv.extend_from_slice(args);
-        let cli = Cli::try_parse_from(argv).unwrap();
+        let cli = Cli::try_parse_from(argv)?;
         let Commands::Local(local) = cli.command else {
             panic!("expected local command");
         };
-        local.command
+        Ok(local.command)
+    }
+
+    fn client_selectors(
+        postgres: bool,
+        selectors: &[&str],
+    ) -> (Option<String>, Option<String>, Option<u16>) {
+        let mut args = if postgres {
+            vec!["postgres", "client"]
+        } else {
+            vec!["client"]
+        };
+        args.extend_from_slice(selectors);
+
+        match local_command(&args) {
+            LocalCommands::Client {
+                name, host, port, ..
+            }
+            | LocalCommands::Postgres {
+                command:
+                    PostgresCommands::Client {
+                        name, host, port, ..
+                    },
+            } => (name, host, port),
+            _ => panic!("expected local client command"),
+        }
     }
 
     #[test]
@@ -506,6 +538,151 @@ mod tests {
         assert!(help.contains("`clickhouse format`"), "{help}");
     }
 
+    #[test]
+    fn client_selector_matrix_accepts_unambiguous_modes_and_port_bounds() {
+        let cases = [
+            (&[][..], None, None, None),
+            (&["--name", "dev"][..], Some("dev"), None, None),
+            (
+                &["--host", "db.example"][..],
+                None,
+                Some("db.example"),
+                None,
+            ),
+            (&["--port", "1"][..], None, None, Some(1)),
+            (&["--port", "65535"][..], None, None, Some(65535)),
+            (
+                &["--host", "db.example", "--port", "9440"][..],
+                None,
+                Some("db.example"),
+                Some(9440),
+            ),
+            (
+                &["--port", "9440", "--host", "db.example"][..],
+                None,
+                Some("db.example"),
+                Some(9440),
+            ),
+        ];
+
+        for postgres in [false, true] {
+            for (selectors, expected_name, expected_host, expected_port) in cases {
+                let (name, host, port) = client_selectors(postgres, selectors);
+                assert_eq!(name.as_deref(), expected_name, "selectors: {selectors:?}");
+                assert_eq!(host.as_deref(), expected_host, "selectors: {selectors:?}");
+                assert_eq!(port, expected_port, "selectors: {selectors:?}");
+            }
+        }
+    }
+
+    #[test]
+    fn postgres_client_version_accepts_managed_modes() {
+        let cases = [
+            (&["--version", "17"][..], None),
+            (&["--name", "dev", "--version", "17"][..], Some("dev")),
+            (&["--version", "17", "--name", "dev"][..], Some("dev")),
+        ];
+
+        for (selectors, expected_name) in cases {
+            let mut args = vec!["postgres", "client"];
+            args.extend_from_slice(selectors);
+            let LocalCommands::Postgres {
+                command:
+                    PostgresCommands::Client {
+                        name,
+                        version,
+                        host,
+                        port,
+                        ..
+                    },
+            } = local_command(&args)
+            else {
+                panic!("expected postgres client command");
+            };
+
+            assert_eq!(name.as_deref(), expected_name, "selectors: {selectors:?}");
+            assert_eq!(version.as_deref(), Some("17"), "selectors: {selectors:?}");
+            assert_eq!(host, None, "selectors: {selectors:?}");
+            assert_eq!(port, None, "selectors: {selectors:?}");
+        }
+    }
+
+    #[test]
+    fn postgres_client_version_rejects_direct_modes_in_either_order() {
+        let cases = [
+            &["--version", "17", "--host", "db.example"][..],
+            &["--host", "db.example", "--version", "17"][..],
+            &["--version", "17", "--port", "5432"][..],
+            &["--port", "5432", "--version", "17"][..],
+        ];
+
+        for selectors in cases {
+            let mut args = vec!["postgres", "client"];
+            args.extend_from_slice(selectors);
+            let error = try_local_command(&args)
+                .err()
+                .expect("managed and direct selectors should conflict");
+            assert_eq!(
+                error.kind(),
+                clap::error::ErrorKind::ArgumentConflict,
+                "selectors: {selectors:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn client_selector_matrix_rejects_named_and_direct_modes_in_either_order() {
+        let cases = [
+            &["--name", "dev", "--host", "db.example"][..],
+            &["--host", "db.example", "--name", "dev"][..],
+            &["--name", "dev", "--port", "9000"][..],
+            &["--port", "9000", "--name", "dev"][..],
+            &["--name", "dev", "--host", "db.example", "--port", "9000"][..],
+            &["--host", "db.example", "--port", "9000", "--name", "dev"][..],
+        ];
+
+        for postgres in [false, true] {
+            for selectors in cases {
+                let mut args = if postgres {
+                    vec!["postgres", "client"]
+                } else {
+                    vec!["client"]
+                };
+                args.extend_from_slice(selectors);
+                let error = try_local_command(&args)
+                    .err()
+                    .expect("selectors should conflict");
+                assert_eq!(
+                    error.kind(),
+                    clap::error::ErrorKind::ArgumentConflict,
+                    "selectors: {selectors:?}"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn client_ports_reject_zero_and_nonnumeric_values() {
+        for postgres in [false, true] {
+            for port in ["0", "not-a-port"] {
+                let mut args = if postgres {
+                    vec!["postgres", "client"]
+                } else {
+                    vec!["client"]
+                };
+                args.extend(["--port", port]);
+                let error = try_local_command(&args)
+                    .err()
+                    .expect("port should be invalid");
+                assert_eq!(
+                    error.kind(),
+                    clap::error::ErrorKind::ValueValidation,
+                    "port: {port}"
+                );
+            }
+        }
+    }
+
     #[test]
     fn parses_remove_without_force() {
         let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"])

crates/clickhousectl/tests/local_client_selectors_test.rs

@@ -0,0 +1,110 @@
+//! End-to-end coverage for local client selector validation (issue #466).
+
+use std::os::unix::fs::PermissionsExt;
+use std::path::{Path, PathBuf};
+use std::process::{Command, Output};
+
+const VERSION: &str = "25.12.9.61";
+
+fn clickhousectl_binary() -> PathBuf {
+    PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl"))
+}
+
+fn install_fake_clickhouse(home: &Path) {
+    let binary = home
+        .join(".clickhouse/versions")
+        .join(VERSION)
+        .join("clickhouse");
+    std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version dir");
+    std::fs::write(&binary, b"#!/bin/sh\nprintf '%s\\n' \"$@\"\n").expect("write fake ClickHouse");
+    let mut permissions = std::fs::metadata(&binary).unwrap().permissions();
+    permissions.set_mode(0o755);
+    std::fs::set_permissions(binary, permissions).expect("make fake ClickHouse executable");
+    std::fs::write(home.join(".clickhouse/default"), VERSION).expect("write default version");
+}
+
+fn run(project: &Path, home: &Path, args: &[&str]) -> Output {
+    Command::new(clickhousectl_binary())
+        .env("DO_NOT_TRACK", "1")
+        .env("HOME", home)
+        .current_dir(project)
+        .args(args)
+        .output()
+        .expect("run clickhousectl")
+}
+
+#[test]
+fn clickhouse_direct_client_defaults_missing_host_or_port() {
+    let project = tempfile::tempdir().expect("create project tempdir");
+    let home = tempfile::tempdir().expect("create home tempdir");
+    install_fake_clickhouse(home.path());
+
+    let cases = [
+        (
+            &["local", "client", "--host", "db.example"][..],
+            &["client", "--host", "db.example", "--port", "9000"][..],
+        ),
+        (
+            &["local", "client", "--port", "1"][..],
+            &["client", "--host", "localhost", "--port", "1"][..],
+        ),
+        (
+            &["local", "client", "--port", "65535"][..],
+            &["client", "--host", "localhost", "--port", "65535"][..],
+        ),
+    ];
+
+    for (args, expected) in cases {
+        let output = run(project.path(), home.path(), args);
+        assert!(
+            output.status.success(),
+            "args: {args:?}\nstderr: {}",
+            String::from_utf8_lossy(&output.stderr)
+        );
+        let forwarded: Vec<_> = String::from_utf8(output.stdout)
+            .expect("fake ClickHouse output should be UTF-8")
+            .lines()
+            .map(str::to_string)
+            .collect();
+        assert_eq!(forwarded, expected, "args: {args:?}");
+    }
+}
+
+#[test]
+fn invalid_client_selectors_are_usage_errors_before_resolution() {
+    let project = tempfile::tempdir().expect("create project tempdir");
+    let home = tempfile::tempdir().expect("create home tempdir");
+    let cases = [
+        &["local", "client", "--name", "dev", "--host", "db.example"][..],
+        &["local", "client", "--port", "9000", "--name", "dev"][..],
+        &["local", "client", "--port", "0"][..],
+        &[
+            "local",
+            "postgres",
+            "client",
+            "--host",
+            "db.example",
+            "--name",
+            "dev",
+        ][..],
+        &[
+            "local", "postgres", "client", "--name", "dev", "--port", "5432",
+        ][..],
+        &["local", "postgres", "client", "--port", "0"][..],
+    ];
+
+    for args in cases {
+        let output = run(project.path(), home.path(), args);
+        assert_eq!(output.status.code(), Some(2), "args: {args:?}");
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        assert!(
+            stderr.contains("error:"),
+            "args: {args:?}\nstderr: {stderr}"
+        );
+        assert!(
+            !stderr.contains("No default version configured")
+                && !stderr.contains("Server 'dev' not found"),
+            "args: {args:?}\nstderr: {stderr}"
+        );
+    }
+}