♻️ refactor(cli): validate startup arguments

lsp-server/src/cli/launcher.cppm

@@ -8,18 +8,43 @@ import std;
 import lsp.core.server;
 import lsp.utils.args_parser;
 
+namespace
+{
+    void SetupLogger(const lsp::utils::ServerConfig& config)
+    {
+        spdlog::set_pattern("%Y-%m-%d %H:%M:%S.%e [%t] [%^%l%$] %v");
+        auto logger = config.log_file.empty() ? spdlog::stderr_logger_mt("console_logger") : spdlog::basic_logger_mt("file_logger", config.log_file);
+        logger->set_level(config.log_level);
+        spdlog::set_default_logger(logger);
+        spdlog::set_level(config.log_level);
+    }
+}
+
 export int Run(int argc, char* argv[])
 {
-    lsp::utils::ArgsParser& args_parser = lsp::utils::ArgsParser::Instance();
-    auto& config = args_parser.Parse(argc, argv);
-
-    if (config.show_help)
+    auto parsed = lsp::utils::ParseArgs(argc, argv);
+    if (!parsed)
     {
-        lsp::utils::ArgsParser::PrintHelp(argv[0]);
+        std::cerr << "[TSL-LSP] Argument error: " << parsed.error() << '\n';
+        return 2;
+    }
+
+    if (parsed->action == lsp::utils::ParseAction::kShowHelp)
+    {
+        lsp::utils::PrintHelp(std::cout, argv[0]);
         return 0;
     }
 
-    lsp::utils::ArgsParser::SetupLogger(config);
+    const auto& config = parsed->config;
+    try
+    {
+        SetupLogger(config);
+    }
+    catch (const std::exception& error)
+    {
+        std::cerr << "[TSL-LSP] Failed to initialize logger: " << error.what() << '\n';
+        return 1;
+    }
 
     try
     {
@@ -27,16 +52,18 @@ export int Run(int argc, char* argv[])
         lsp::core::LspServer server(config.thread_count, config.interpreter_path);
         server.Run();
     }
-    catch (const std::exception& e)
+    catch (const std::exception& error)
     {
-        std::cerr << "[TSL-LSP] Server fatal error: " << e.what() << std::endl;
-        spdlog::error("Server fatal error: {}", e.what());
+        std::cerr << "[TSL-LSP] Server fatal error: " << error.what() << '\n';
+        spdlog::error("Server fatal error: {}", error.what());
+        spdlog::shutdown();
         return 1;
     }
     catch (...)
     {
-        std::cerr << "[TSL-LSP] Server unknown fatal error" << std::endl;
+        std::cerr << "[TSL-LSP] Server unknown fatal error\n";
         spdlog::error("Server unknown fatal error");
+        spdlog::shutdown();
         return 1;
     }
 

lsp-server/src/utils/args_parser.cppm

@@ -1,141 +1,131 @@
 module;
 
 export module lsp.utils.args_parser;
-import spdlog;
 
+import spdlog;
 import std;
 
 export namespace lsp::utils
 {
     struct ServerConfig
     {
-        bool use_stderr = false;
-        bool show_help = false;
         std::size_t thread_count = 4;
         spdlog::level::level_enum log_level = spdlog::level::info;
         std::string log_file;
         std::string interpreter_path;
     };
 
-    class ArgsParser
+    enum class ParseAction
     {
-    public:
-        ArgsParser(const ArgsParser&) = delete;
-        ArgsParser& operator=(const ArgsParser&) = delete;
-        static ArgsParser& Instance();
-
-        const ServerConfig& Parse(int argc, char* argv[]);
-        const ServerConfig& GetConfig() const;
-
-        static void SetupLogger(const ServerConfig& config);
-        static void PrintHelp(const std::string& program_name);
-
-    private:
-        ArgsParser() = default;
-        ~ArgsParser() = default;
-
-        ServerConfig config_;
+        kRun,
+        kShowHelp,
     };
+
+    struct ParseResult
+    {
+        ParseAction action = ParseAction::kRun;
+        ServerConfig config;
+    };
+
+    std::expected<ParseResult, std::string> ParseArgs(int argc, char* const argv[]);
+    void PrintHelp(std::ostream& output, std::string_view program_name);
 }
 
 namespace lsp::utils
 {
-    ArgsParser& ArgsParser::Instance()
+    namespace
     {
-        static ArgsParser instance;
-        return instance;
+        constexpr std::size_t kMinThreadCount = 1;
+        constexpr std::size_t kMaxThreadCount = 256;
+
+        std::optional<spdlog::level::level_enum> ParseLogLevel(std::string_view value)
+        {
+            if (value == "trace")
+                return spdlog::level::trace;
+            if (value == "debug")
+                return spdlog::level::debug;
+            if (value == "info")
+                return spdlog::level::info;
+            if (value == "warn")
+                return spdlog::level::warn;
+            if (value == "error")
+                return spdlog::level::err;
+            if (value == "off")
+                return spdlog::level::off;
+            return std::nullopt;
+        }
     }
 
-    const ServerConfig& ArgsParser::Parse(int argc, char* argv[])
+    std::expected<ParseResult, std::string> ParseArgs(int argc, char* const argv[])
     {
-        config_ = ServerConfig{};
-        // Default to stderr so LSP stdio (stdout) stays clean.
-        config_.use_stderr = true;
+        for (int i = 1; i < argc; ++i)
+        {
+            if (std::string_view(argv[i]) == "--help")
+                return ParseResult{ .action = ParseAction::kShowHelp, .config = {} };
+        }
 
+        ParseResult result;
+        constexpr std::string_view kLogPrefix = "--log=";
         constexpr std::string_view kLogFilePrefix = "--log-file=";
         constexpr std::string_view kThreadsPrefix = "--threads=";
         constexpr std::string_view kInterpreterPrefix = "--interpreter=";
 
         for (int i = 1; i < argc; ++i)
         {
-            std::string arg = argv[i];
-
-            if (arg == "--help")
+            const std::string_view argument = argv[i];
+            if (argument.starts_with(kLogPrefix))
             {
-                config_.show_help = true;
-                return config_;
+                const auto value = argument.substr(kLogPrefix.size());
+                auto level = ParseLogLevel(value);
+                if (!level)
+                    return std::unexpected("Invalid value for --log: '" +
+                                           std::string(value) + "'");
+                result.config.log_level = *level;
             }
-
-            if (arg == "--log=trace")
-                config_.log_level = spdlog::level::trace;
-            else if (arg == "--log=debug")
-                config_.log_level = spdlog::level::debug;
-            else if (arg == "--log=info")
-                config_.log_level = spdlog::level::info;
-            else if (arg == "--log=warn")
-                config_.log_level = spdlog::level::warn;
-            else if (arg == "--log=error")
-                config_.log_level = spdlog::level::err;
-            else if (arg == "--log=off")
-                config_.log_level = spdlog::level::off;
-            else if (arg == "--log-stderr")
-                config_.use_stderr = true;
-            else if (arg == "--log-stdout")
-                config_.use_stderr = false;
-            else if (arg.starts_with(kLogFilePrefix))
-                config_.log_file = arg.substr(kLogFilePrefix.size());
-            else if (arg == "--use-stdio")
-                config_.use_stderr = true;
-            else if (arg.starts_with(kThreadsPrefix))
+            else if (argument.starts_with(kLogFilePrefix))
             {
-                auto value = arg.substr(kThreadsPrefix.size());
-                config_.thread_count = std::max<std::size_t>(1, static_cast<std::size_t>(std::stoi(value)));
+                const auto value = argument.substr(kLogFilePrefix.size());
+                if (value.empty())
+                    return std::unexpected("--log-file requires a non-empty path");
+                result.config.log_file = value;
             }
-            else if (arg.starts_with(kInterpreterPrefix))
+            else if (argument.starts_with(kThreadsPrefix))
             {
-                config_.interpreter_path = arg.substr(kInterpreterPrefix.size());
+                const auto value = argument.substr(kThreadsPrefix.size());
+                std::size_t count = 0;
+                const auto [end, error] =
+                    std::from_chars(value.data(), value.data() + value.size(), count);
+                if (error != std::errc{} || end != value.data() + value.size() ||
+                    count < kMinThreadCount || count > kMaxThreadCount)
+                {
+                    return std::unexpected("Invalid value for --threads: '" +
+                                           std::string(value) + "' (expected 1..256)");
+                }
+                result.config.thread_count = count;
+            }
+            else if (argument.starts_with(kInterpreterPrefix))
+            {
+                const auto value = argument.substr(kInterpreterPrefix.size());
+                if (value.empty())
+                    return std::unexpected("--interpreter requires a non-empty path");
+                result.config.interpreter_path = value;
+            }
+            else
+            {
+                return std::unexpected("Unknown argument: " + std::string(argument));
             }
         }
-
-        return config_;
+        return result;
     }
 
-    const ServerConfig& ArgsParser::GetConfig() const
+    void PrintHelp(std::ostream& output, std::string_view program_name)
     {
-        return config_;
-    }
-
-    void ArgsParser::SetupLogger(const ServerConfig& config)
-    {
-        spdlog::set_pattern("%Y-%m-%d %H:%M:%S.%e [%t] [%^%l%$] %v");
-
-        if (!config.log_file.empty())
-        {
-            auto file_logger = spdlog::basic_logger_mt("file_logger", config.log_file);
-            file_logger->set_level(config.log_level);
-            spdlog::set_default_logger(file_logger);
-        }
-        else
-        {
-            auto console_logger = config.use_stderr ? spdlog::stderr_logger_mt("console_logger") : spdlog::stdout_logger_mt("console_logger");
-            console_logger->set_level(config.log_level);
-            spdlog::set_default_logger(console_logger);
-        }
-
-        spdlog::set_level(config.log_level);
-    }
-
-    void ArgsParser::PrintHelp(const std::string& program_name)
-    {
-        std::cout << "Usage: " << program_name << " [options]\\n\\n"
-                  << "Options:\\n"
-                  << "  --help                Show this help message\\n"
-                  << "  --log=<level>         Set log level (trace, debug, info, warn, error, off)\\n"
-                  << "  --log-stderr          Output logs to stderr (default)\\n"
-                  << "  --log-stdout          Output logs to stdout\\n"
-                  << "  --log-file=<path>     Output logs to specified file\\n"
-                  << "  --use-stdio           Alias for --log-stderr (keep stdout clean for LSP)\\n"
-                  << "  --threads=<count>     Number of worker threads\\n"
-                  << "  --interpreter=<path>  Custom interpreter path\\n";
+        output << "Usage: " << program_name << " [options]\n\n"
+               << "Options:\n"
+               << "  --help                Show this help message\n"
+               << "  --log=<level>         Set log level (trace, debug, info, warn, error, off)\n"
+               << "  --log-file=<path>     Output logs to specified file\n"
+               << "  --threads=<count>     Number of worker threads (1-256)\n"
+               << "  --interpreter=<path>  Custom interpreter path\n";
     }
 }

lsp-server/test/CMakeLists.txt

@@ -28,6 +28,10 @@ if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_scheduler/CMakeLists.txt)
   add_subdirectory(test_scheduler)
 endif()
 
+if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_args_parser/CMakeLists.txt)
+  add_subdirectory(test_args_parser)
+endif()
+
 if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_semantic/CMakeLists.txt)
   add_subdirectory(test_semantic)
 endif()
@@ -48,6 +52,10 @@ if(BUILD_TESTS)
       COMMAND ${PYTHON3_EXECUTABLE}
         ${CMAKE_CURRENT_LIST_DIR}/run_lsp_json_tests.py
         --server $<TARGET_FILE:tsl-server>)
+    add_test(NAME test_cli_startup
+      COMMAND ${PYTHON3_EXECUTABLE}
+        ${CMAKE_CURRENT_LIST_DIR}/test_cli_startup.py
+        --server $<TARGET_FILE:tsl-server>)
   else()
     message(WARNING "python3 not found; skipping test_lsp_json registration")
   endif()

lsp-server/test/test_args_parser/CMakeLists.txt

@@ -0,0 +1,40 @@
+cmake_minimum_required(VERSION 4.0)
+
+project(test_args_parser LANGUAGES CXX)
+
+set(CMAKE_CXX_STANDARD 23)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_EXPERIMENTAL_CXX_MODULE_DYNDEP 1)
+
+find_package(spdlog CONFIG REQUIRED)
+find_package(fmt CONFIG REQUIRED)
+
+add_executable(test_args_parser main.cc test_args_parser.cppm)
+if(TARGET std_module)
+  add_dependencies(test_args_parser std_module)
+endif()
+
+target_sources(
+  test_args_parser
+  PRIVATE
+  FILE_SET cxx_modules TYPE CXX_MODULES
+                       BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}
+                                 ${CMAKE_CURRENT_SOURCE_DIR}/../../src
+                       FILES test_args_parser.cppm
+                             ../../src/bridge/spdlog.cppm
+                             ../../src/utils/args_parser.cppm)
+
+target_compile_definitions(test_args_parser PRIVATE SPDLOG_HEADER_ONLY
+                                                       FMT_HEADER_ONLY)
+target_link_libraries(test_args_parser PRIVATE spdlog::spdlog_header_only
+                                               fmt::fmt-header-only)
+target_compile_options(
+  test_args_parser
+  PRIVATE -Wall -Wextra -Wpedantic
+          -Wno-import-implementation-partition-unit-in-interface-unit
+          $<$<CONFIG:Debug>:-g -O0>
+          $<$<CONFIG:Release>:-O3>)
+
+if(BUILD_TESTS)
+  add_test(NAME test_args_parser COMMAND $<TARGET_FILE:test_args_parser>)
+endif()

lsp-server/test/test_args_parser/main.cc

@@ -0,0 +1,6 @@
+import lsp.test.args_parser;
+
+int main()
+{
+    return Run();
+}

lsp-server/test/test_args_parser/test_args_parser.cppm

@@ -0,0 +1,131 @@
+module;
+
+export module lsp.test.args_parser;
+
+import spdlog;
+import std;
+
+import lsp.utils.args_parser;
+
+namespace
+{
+    void Expect(bool condition, std::string_view message)
+    {
+        if (!condition)
+            throw std::runtime_error(std::string(message));
+    }
+
+    auto Parse(std::initializer_list<std::string_view> arguments)
+    {
+        std::vector<std::string> storage{ "tsl-server" };
+        storage.reserve(arguments.size() + 1);
+        for (auto argument : arguments)
+            storage.emplace_back(argument);
+
+        std::vector<char*> argv;
+        argv.reserve(storage.size());
+        for (auto& argument : storage)
+            argv.push_back(argument.data());
+
+        return lsp::utils::ParseArgs(static_cast<int>(argv.size()), argv.data());
+    }
+
+    void ExpectError(std::initializer_list<std::string_view> arguments,
+                     std::string_view fragment)
+    {
+        auto result = Parse(arguments);
+        Expect(!result.has_value(), "arguments should be rejected");
+        Expect(result.error().find(fragment) != std::string::npos,
+               "error should identify the invalid argument");
+    }
+}
+
+export int Run()
+{
+    int failures = 0;
+    auto RunCase = [&failures](std::string_view name, auto test) {
+        try
+        {
+            test();
+            std::cout << "[PASS] " << name << '\n';
+        }
+        catch (const std::exception& error)
+        {
+            ++failures;
+            std::cout << "[FAIL] " << name << ": " << error.what() << '\n';
+        }
+    };
+
+    RunCase("defaults", [] {
+        auto result = Parse({});
+        Expect(result.has_value(), "default arguments should parse");
+        Expect(result->action == lsp::utils::ParseAction::kRun,
+               "default action should run");
+        Expect(result->config.thread_count == 4, "default thread count should be 4");
+        Expect(result->config.log_level == spdlog::level::info,
+               "default log level should be info");
+    });
+
+    RunCase("valid values and last occurrence", [] {
+        auto result = Parse({ "--threads=8", "--threads=16", "--log=debug", "--log-file=server.log", "--interpreter=/opt/tsl" });
+        Expect(result.has_value(), "valid arguments should parse");
+        Expect(result->config.thread_count == 16, "last thread count should win");
+        Expect(result->config.log_level == spdlog::level::debug,
+               "debug log level should parse");
+        Expect(result->config.log_file == "server.log", "log file should parse");
+        Expect(result->config.interpreter_path == "/opt/tsl",
+               "interpreter path should parse");
+    });
+
+    RunCase("all log levels", [] {
+        const std::array levels{
+            std::pair{ "trace", spdlog::level::trace },
+            std::pair{ "debug", spdlog::level::debug },
+            std::pair{ "info", spdlog::level::info },
+            std::pair{ "warn", spdlog::level::warn },
+            std::pair{ "error", spdlog::level::err },
+            std::pair{ "off", spdlog::level::off },
+        };
+        for (const auto& [name, level] : levels)
+        {
+            const std::string argument = std::string("--log=") + name;
+            auto result = Parse({ argument });
+            Expect(result.has_value(), "known log level should parse");
+            Expect(result->config.log_level == level, "log level should match");
+        }
+    });
+
+    RunCase("help takes precedence", [] {
+        auto result = Parse({ "--unknown", "--help", "--threads=bad" });
+        Expect(result.has_value(), "help should bypass other validation");
+        Expect(result->action == lsp::utils::ParseAction::kShowHelp,
+               "help action should be returned");
+    });
+
+    RunCase("thread validation", [] {
+        Expect(Parse({ "--threads=1" }).has_value(), "one thread should parse");
+        Expect(Parse({ "--threads=256" }).has_value(), "256 threads should parse");
+        for (auto argument : { "--threads=", "--threads=0", "--threads=-1", "--threads=257", "--threads=12junk", "--threads=18446744073709551616" })
+            ExpectError({ argument }, "--threads");
+    });
+
+    RunCase("other invalid values", [] {
+        for (auto argument : { "--log=verbose", "--log-file=", "--interpreter=", "--log-stdout", "--log-stderr", "--use-stdio", "--unknown" })
+            ExpectError({ argument }, "--");
+    });
+
+    RunCase("help formatting", [] {
+        std::ostringstream output;
+        lsp::utils::PrintHelp(output, "tsl-server");
+        const auto text = output.str();
+        Expect(text.contains("Usage: tsl-server [options]\n\nOptions:\n"),
+               "help should contain physical line breaks");
+        Expect(!text.contains("\\n"), "help should not contain escaped newline text");
+        Expect(!text.contains("--log-stdout"), "removed stdout flag should be absent");
+        Expect(!text.contains("--log-stderr"), "removed stderr flag should be absent");
+        Expect(!text.contains("--use-stdio"), "removed stdio alias should be absent");
+    });
+
+    std::cout << "Failures: " << failures << '\n';
+    return failures == 0 ? 0 : 1;
+}

lsp-server/test/test_cli_startup.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+
+import argparse
+import subprocess
+import tempfile
+import unittest
+from pathlib import Path
+
+
+SERVER: Path
+
+
+def run_server(*arguments: str) -> subprocess.CompletedProcess[bytes]:
+    return subprocess.run(
+        [str(SERVER), *arguments],
+        input=b"",
+        capture_output=True,
+        check=False,
+    )
+
+
+class CliStartupTest(unittest.TestCase):
+    def test_help_uses_real_newlines(self) -> None:
+        result = run_server("--help")
+        self.assertEqual(0, result.returncode)
+        self.assertIn(b"\n\nOptions:\n", result.stdout)
+        self.assertNotIn(b"\\n", result.stdout)
+
+    def test_invalid_threads_return_argument_error(self) -> None:
+        for argument in ("--threads=", "--threads=-1", "--threads=12junk"):
+            with self.subTest(argument=argument):
+                result = run_server(argument)
+                self.assertEqual(2, result.returncode)
+                self.assertIn(b"--threads", result.stderr)
+                self.assertNotIn(b"TSL-LSP server starting", result.stderr)
+
+    def test_removed_stdout_flag_is_rejected(self) -> None:
+        result = run_server("--log-stdout")
+        self.assertEqual(2, result.returncode)
+        self.assertEqual(b"", result.stdout)
+
+    def test_default_logger_never_writes_stdout(self) -> None:
+        result = run_server()
+        self.assertEqual(0, result.returncode)
+        self.assertEqual(b"", result.stdout)
+        self.assertIn(b"TSL-LSP server starting", result.stderr)
+
+    def test_log_file_failure_is_controlled(self) -> None:
+        with tempfile.TemporaryDirectory() as temp_dir:
+            result = run_server(f"--log-file={temp_dir}")
+        self.assertEqual(1, result.returncode)
+        self.assertIn(b"Failed to initialize logger", result.stderr)
+
+
+if __name__ == "__main__":
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--server", type=Path, required=True)
+    args, unittest_args = parser.parse_known_args()
+    SERVER = args.server.resolve()
+    unittest.main(argv=[__file__, *unittest_args])