better linting approach

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)
+  2. Shell script linting via shellcheck (run in container)
-  3. Common issues:
+  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
+// Run shellcheck on all shell scripts in a directory using Docker
-ValidationResult check_bash_syntax(const std::filesystem::path& script_path) {
+// Returns: number of issues found (0 = all passed)
-    ValidationResult result;
+int run_shellcheck(const std::filesystem::path& template_path, std::vector<std::filesystem::path>& scripts) {
-    result.passed = true;
+    if (scripts.empty()) {
-
+        return 0;
-    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();
     }
 
-    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<std::string> 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<std::string> 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<std::string> 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
+    // Step 3: Shellcheck validation (run in container)
-    info << "=== Syntax Validation (bash -n) ===" << std::endl;
+    info << "=== Shellcheck Linting ===" << std::endl;
-    for (const auto& script : shell_scripts) {
+    int shellcheck_issues = run_shellcheck(template_path, shell_scripts);
-        ValidationResult result = check_bash_syntax(script);
+    if (shellcheck_issues > 0) {
-        if (result.passed) {
+        warnings += shellcheck_issues;
-            info << "  ✓ " << result.message << std::endl;
-        } else {
-            error << "  ✗ " << result.message << std::endl;
-            if (!result.details.empty()) {
-                error << "    " << result.details << std::endl;
-            }
-            errors++;
-        }
     }
     std::cout << std::endl;
 
-    // Step 4: Linting - check common.sh sourcing (only for main scripts)
+    // Step 4: Dropshell-specific checks - common.sh sourcing (only for main scripts)
-    info << "=== Linting: common.sh sourcing ===" << std::endl;
+    info << "=== Dropshell: common.sh sourcing ===" << std::endl;
     std::vector<std::string> 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) {