From 7ffcfabc8f0c38482ff2725cc8ef5f97e5cfaa8e Mon Sep 17 00:00:00 2001 From: j Date: Sun, 28 Dec 2025 00:11:05 +1300 Subject: [PATCH] better linting approach --- source/src/commands/validate.cpp | 295 +++++++++---------------------- 1 file changed, 83 insertions(+), 212 deletions(-) diff --git a/source/src/commands/validate.cpp b/source/src/commands/validate.cpp index b908110..ec193e6 100644 --- a/source/src/commands/validate.cpp +++ b/source/src/commands/validate.cpp @@ -37,11 +37,9 @@ struct ValidateCommandRegister { R"( Validates a dropshell template by checking: 1. Required files exist (install.sh, uninstall.sh, config files) - 2. Shell script syntax (bash -n) - 3. Common issues: + 2. Shell script linting via shellcheck (run in container) + 3. Dropshell-specific checks: - Scripts source common.sh - - Error handling patterns (_die usage) - - Proper quoting of variables Example: ds validate my-template @@ -71,29 +69,84 @@ struct ValidationResult { std::string details; }; -// Check shell script syntax using bash -n -ValidationResult check_bash_syntax(const std::filesystem::path& script_path) { - ValidationResult result; - result.passed = true; - - std::string command = "bash -n " + script_path.string() + " 2>&1"; - std::string output; - - ordered_env_vars empty_env; - bool success = execute_local_command(".", command, empty_env, &output, cMode::Silent); - - if (!success || !output.empty()) { - result.passed = false; - result.message = "Syntax error in " + script_path.filename().string(); - result.details = output; - } else { - result.message = "Syntax OK: " + script_path.filename().string(); +// Run shellcheck on all shell scripts in a directory using Docker +// Returns: number of issues found (0 = all passed) +int run_shellcheck(const std::filesystem::path& template_path, std::vector& scripts) { + if (scripts.empty()) { + return 0; } - return result; + int total_issues = 0; + + for (const auto& script : scripts) { + // Get relative path from template_path + std::string rel_path = std::filesystem::relative(script, template_path).string(); + + // Run shellcheck in container + // -f gcc gives us parseable output: file:line:col: severity: message + // -s bash specifies bash dialect + // -e SC1091 excludes "not following sourced file" warnings (common.sh is external) + std::string command = "docker run --rm -v " + template_path.string() + ":/mnt:ro koalaman/shellcheck:stable " + "-f gcc -s bash -e SC1091 /mnt/" + rel_path + " 2>&1"; + + FILE* pipe = popen(command.c_str(), "r"); + if (!pipe) { + error << "Failed to run shellcheck on " << script.filename() << std::endl; + total_issues++; + continue; + } + + char buffer[512]; + std::string output; + while (fgets(buffer, sizeof(buffer), pipe) != nullptr) { + output += buffer; + } + + int result = pclose(pipe); + + if (result != 0 && !output.empty()) { + // Parse and display shellcheck output + // Format: /mnt/file.sh:line:col: severity: message + std::istringstream iss(output); + std::string line; + int script_issues = 0; + + while (std::getline(iss, line)) { + if (line.empty()) continue; + + // Replace /mnt/ with actual filename for clarity + size_t mnt_pos = line.find("/mnt/"); + if (mnt_pos != std::string::npos) { + line = line.substr(mnt_pos + 5); // Skip "/mnt/" + } + + // Color based on severity + if (line.find(": error:") != std::string::npos) { + error << " " << line << std::endl; + script_issues++; + } else if (line.find(": warning:") != std::string::npos) { + warning << " " << line << std::endl; + script_issues++; + } else if (line.find(": note:") != std::string::npos) { + info << " " << line << std::endl; + } else { + warning << " " << line << std::endl; + script_issues++; + } + } + + if (script_issues > 0) { + total_issues += script_issues; + } + } else { + info << " ✓ " << script.filename().string() << std::endl; + } + } + + return total_issues; } -// Check if script sources common.sh +// Check if script sources common.sh (dropshell-specific check) ValidationResult check_common_sh_sourced(const std::filesystem::path& script_path) { ValidationResult result; result.passed = false; @@ -121,133 +174,6 @@ ValidationResult check_common_sh_sourced(const std::filesystem::path& script_pat return result; } -// Check for risky commands without error handling -ValidationResult check_error_handling(const std::filesystem::path& script_path) { - ValidationResult result; - result.passed = true; - - std::ifstream file(script_path); - if (!file.is_open()) { - result.message = "Could not open " + script_path.filename().string(); - result.passed = false; - return result; - } - - // Commands that should have error handling - std::vector risky_commands = { - "docker compose", - "docker run", - "mkdir ", - "cp ", - "mv ", - "rm ", - "curl ", - "wget ", - "pip install", - "pip3 install", - "apt-get", - "apt ", - "git clone", - "git pull" - }; - - std::vector issues; - std::string line; - int line_num = 0; - - while (std::getline(file, line)) { - line_num++; - - // Skip comments and empty lines - std::string trimmed = trim(line); - if (trimmed.empty() || trimmed[0] == '#') continue; - - // Skip lines that already have error handling - if (line.find("|| _die") != std::string::npos || - line.find("|| exit") != std::string::npos || - line.find("|| return") != std::string::npos || - line.find("|| true") != std::string::npos || - line.find("|| echo") != std::string::npos || - line.find("if ") == 0 || line.find("if !") != std::string::npos || - line.find("&&") != std::string::npos) { - continue; - } - - for (const auto& cmd : risky_commands) { - if (line.find(cmd) != std::string::npos) { - // Check if it's inside a conditional or has error handling - issues.push_back("Line " + std::to_string(line_num) + ": '" + cmd + "' without error handling"); - break; - } - } - } - - if (!issues.empty()) { - result.passed = false; - result.message = "Missing error handling in " + script_path.filename().string(); - for (const auto& issue : issues) { - result.details += " " + issue + "\n"; - } - result.details += "Consider adding '|| _die \"error message\"' after commands that can fail"; - } else { - result.message = "Error handling OK: " + script_path.filename().string(); - } - - return result; -} - -// Check for unquoted variables in critical places -ValidationResult check_variable_quoting(const std::filesystem::path& script_path) { - ValidationResult result; - result.passed = true; - - std::ifstream file(script_path); - if (!file.is_open()) { - result.message = "Could not open " + script_path.filename().string(); - result.passed = false; - return result; - } - - std::vector issues; - std::string line; - int line_num = 0; - - // Pattern for unquoted variables in risky contexts (paths, arguments) - std::regex unquoted_path_var(R"((cd|mkdir|rm|cp|mv|cat|ls)\s+\$[A-Za-z_][A-Za-z0-9_]*[^\"'\s])"); - std::regex unquoted_in_test(R"(\[\s+(-[a-z]\s+)?\$[A-Za-z_][A-Za-z0-9_]*\s+)"); - - while (std::getline(file, line)) { - line_num++; - - // Skip comments - std::string trimmed = trim(line); - if (trimmed.empty() || trimmed[0] == '#') continue; - - // Check for unquoted variables in file operations - if (std::regex_search(line, unquoted_path_var)) { - issues.push_back("Line " + std::to_string(line_num) + ": Potentially unquoted variable in path operation"); - } - - // Check for unquoted variables in test expressions - if (std::regex_search(line, unquoted_in_test)) { - issues.push_back("Line " + std::to_string(line_num) + ": Potentially unquoted variable in test expression"); - } - } - - if (!issues.empty()) { - result.passed = false; - result.message = "Potential quoting issues in " + script_path.filename().string(); - for (const auto& issue : issues) { - result.details += " " + issue + "\n"; - } - result.details += "Use \"$VAR\" instead of $VAR to handle paths with spaces"; - } else { - result.message = "Variable quoting OK: " + script_path.filename().string(); - } - - return result; -} - // ---------------------------------------------------------------------------- // Main validation handler // ---------------------------------------------------------------------------- @@ -294,24 +220,16 @@ int validate_handler(const CommandContext& ctx) { } } - // Step 3: Syntax validation - info << "=== Syntax Validation (bash -n) ===" << std::endl; - for (const auto& script : shell_scripts) { - ValidationResult result = check_bash_syntax(script); - if (result.passed) { - info << " ✓ " << result.message << std::endl; - } else { - error << " ✗ " << result.message << std::endl; - if (!result.details.empty()) { - error << " " << result.details << std::endl; - } - errors++; - } + // Step 3: Shellcheck validation (run in container) + info << "=== Shellcheck Linting ===" << std::endl; + int shellcheck_issues = run_shellcheck(template_path, shell_scripts); + if (shellcheck_issues > 0) { + warnings += shellcheck_issues; } std::cout << std::endl; - // Step 4: Linting - check common.sh sourcing (only for main scripts) - info << "=== Linting: common.sh sourcing ===" << std::endl; + // Step 4: Dropshell-specific checks - common.sh sourcing (only for main scripts) + info << "=== Dropshell: common.sh sourcing ===" << std::endl; std::vector main_scripts = {"install.sh", "uninstall.sh", "start.sh", "stop.sh", "status.sh", "backup.sh", "restore.sh"}; for (const auto& script_name : main_scripts) { std::filesystem::path script_path = template_path / script_name; @@ -330,53 +248,6 @@ int validate_handler(const CommandContext& ctx) { } std::cout << std::endl; - // Step 5: Linting - error handling - info << "=== Linting: Error Handling ===" << std::endl; - for (const auto& script_name : main_scripts) { - std::filesystem::path script_path = template_path / script_name; - if (std::filesystem::exists(script_path)) { - ValidationResult result = check_error_handling(script_path); - if (result.passed) { - info << " ✓ " << result.message << std::endl; - } else { - warning << " ⚠ " << result.message << std::endl; - if (!result.details.empty()) { - // Print each line of details - std::istringstream iss(result.details); - std::string detail_line; - while (std::getline(iss, detail_line)) { - warning << " " << detail_line << std::endl; - } - } - warnings++; - } - } - } - std::cout << std::endl; - - // Step 6: Linting - variable quoting - info << "=== Linting: Variable Quoting ===" << std::endl; - for (const auto& script_name : main_scripts) { - std::filesystem::path script_path = template_path / script_name; - if (std::filesystem::exists(script_path)) { - ValidationResult result = check_variable_quoting(script_path); - if (result.passed) { - info << " ✓ " << result.message << std::endl; - } else { - warning << " ⚠ " << result.message << std::endl; - if (!result.details.empty()) { - std::istringstream iss(result.details); - std::string detail_line; - while (std::getline(iss, detail_line)) { - warning << " " << detail_line << std::endl; - } - } - warnings++; - } - } - } - std::cout << std::endl; - // Summary maketitle("Validation Summary"); if (errors == 0 && warnings == 0) {