feat: Improve execute terminal commands tool

tools/BuildProjectTool.cpp

@@ -112,7 +112,8 @@ QJsonObject BuildProjectTool::getDefinition(LLMCore::ToolSchemaFormat format) co
 
 LLMCore::ToolPermissions BuildProjectTool::requiredPermissions() const
 {
-    return LLMCore::ToolPermission::None;
+    return LLMCore::ToolPermission::FileSystemRead 
+         | LLMCore::ToolPermission::FileSystemWrite;
 }
 
 QFuture<QString> BuildProjectTool::executeAsync(const QJsonObject &input)

tools/ExecuteTerminalCommandTool.cpp

@@ -30,6 +30,7 @@
 #include <QPromise>
 #include <QRegularExpression>
 #include <QSharedPointer>
+#include <QTimer>
 
 namespace QodeAssist::Tools {
 
@@ -50,16 +51,7 @@ QString ExecuteTerminalCommandTool::stringName() const
 
 QString ExecuteTerminalCommandTool::description() const
 {
-    const QStringList allowed = getAllowedCommands();
-    const QString allowedList = allowed.isEmpty() ? "none" : allowed.join(", ");
-
-    return QString(
-               "Execute a terminal command in the project directory. "
-               "Only commands from the allowed list can be executed. "
-               "Currently allowed commands: %1. "
-               "The command will be executed in the root directory of the active project. "
-               "Returns the command output (stdout and stderr) or an error message if the command fails.")
-        .arg(allowedList);
+    return getCommandDescription();
 }
 
 QJsonObject ExecuteTerminalCommandTool::getDefinition(LLMCore::ToolSchemaFormat format) const
@@ -67,19 +59,18 @@ QJsonObject ExecuteTerminalCommandTool::getDefinition(LLMCore::ToolSchemaFormat
     QJsonObject definition;
     definition["type"] = "object";
 
-    const QStringList allowed = getAllowedCommands();
-    const QString allowedList = allowed.isEmpty() ? "none" : allowed.join(", ");
+    const QString commandDesc = getCommandDescription();
 
     QJsonObject properties;
     properties["command"] = QJsonObject{
         {"type", "string"},
-        {"description",
-         QString("The terminal command to execute. Must be one of the allowed commands: %1")
-             .arg(allowedList)}};
+        {"description", commandDesc}};
 
     properties["args"] = QJsonObject{
         {"type", "string"},
-        {"description", "Optional arguments for the command (default: empty)"}};
+        {"description", 
+         "Optional arguments for the command. Arguments with spaces should be properly quoted. "
+         "Example: '--file \"path with spaces.txt\" --verbose'"}};
 
     definition["properties"] = properties;
     definition["required"] = QJsonArray{"command"};
@@ -100,7 +91,9 @@ QJsonObject ExecuteTerminalCommandTool::getDefinition(LLMCore::ToolSchemaFormat
 
 LLMCore::ToolPermissions ExecuteTerminalCommandTool::requiredPermissions() const
 {
-    return LLMCore::ToolPermission::None;
+    return LLMCore::ToolPermission::FileSystemRead 
+         | LLMCore::ToolPermission::FileSystemWrite 
+         | LLMCore::ToolPermission::NetworkAccess;
 }
 
 QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &input)
@@ -113,6 +106,22 @@ QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp
         return QtFuture::makeReadyFuture(QString("Error: Command parameter is required."));
     }
 
+    if (command.length() > MAX_COMMAND_LENGTH) {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command too long (%1 chars)")
+                        .arg(command.length()));
+        return QtFuture::makeReadyFuture(
+            QString("Error: Command exceeds maximum length of %1 characters.")
+                .arg(MAX_COMMAND_LENGTH));
+    }
+
+    if (args.length() > MAX_ARGS_LENGTH) {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Arguments too long (%1 chars)")
+                        .arg(args.length()));
+        return QtFuture::makeReadyFuture(
+            QString("Error: Arguments exceed maximum length of %1 characters.")
+                .arg(MAX_ARGS_LENGTH));
+    }
+
     if (!isCommandAllowed(command)) {
         LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' is not allowed")
                         .arg(command));
@@ -124,6 +133,23 @@ QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp
                 .arg(allowedList));
     }
 
+    if (!isCommandSafe(command)) {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' contains unsafe characters")
+                        .arg(command));
+        return QtFuture::makeReadyFuture(
+            QString("Error: Command '%1' contains potentially dangerous characters. "
+                    "Only alphanumeric characters, hyphens, underscores, and dots are allowed.")
+                .arg(command));
+    }
+
+    if (!args.isEmpty() && !areArgumentsSafe(args)) {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Arguments contain unsafe patterns: '%1'")
+                        .arg(args));
+        return QtFuture::makeReadyFuture(
+            QString("Error: Arguments contain potentially dangerous patterns (command chaining, "
+                    "redirection, or pipe operators)."));
+    }
+
     auto *project = ProjectExplorer::ProjectManager::startupProject();
     QString workingDir;
 
@@ -136,9 +162,20 @@ QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp
         workingDir = QDir::currentPath();
     }
 
-    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Executing command '%1' with args '%2'")
+    QDir dir(workingDir);
+    if (!dir.exists() || !dir.isReadable()) {
+        LOG_MESSAGE(
+            QString("ExecuteTerminalCommandTool: Working directory '%1' is not accessible")
+                .arg(workingDir));
+        return QtFuture::makeReadyFuture(
+            QString("Error: Working directory '%1' does not exist or is not accessible.")
+                .arg(workingDir));
+    }
+
+    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Executing command '%1' with args '%2' in '%3'")
                     .arg(command)
-                    .arg(args));
+                    .arg(args.isEmpty() ? "(no args)" : args)
+                    .arg(workingDir));
 
     auto promise = QSharedPointer<QPromise<QString>>::create();
     QFuture<QString> future = promise->future();
@@ -147,40 +184,85 @@ QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp
     QProcess *process = new QProcess();
     process->setWorkingDirectory(workingDir);
     process->setProcessChannelMode(QProcess::MergedChannels);
+    
+    process->setReadChannel(QProcess::StandardOutput);
+
+    QTimer *timeoutTimer = new QTimer();
+    timeoutTimer->setSingleShot(true);
+    timeoutTimer->setInterval(COMMAND_TIMEOUT_MS);
+    
+    auto outputSize = QSharedPointer<qint64>::create(0);
+
+    QObject::connect(timeoutTimer, &QTimer::timeout, [process, promise, command, args, timeoutTimer]() {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1 %2' timed out after %3ms")
+                        .arg(command)
+                        .arg(args)
+                        .arg(COMMAND_TIMEOUT_MS));
+        
+        process->terminate();
+        
+        QTimer::singleShot(1000, process, [process]() {
+            if (process->state() == QProcess::Running) {
+                LOG_MESSAGE("ExecuteTerminalCommandTool: Forcefully killing process after timeout");
+                process->kill();
+            }
+        });
+        
+        promise->addResult(QString("Error: Command '%1 %2' timed out after %3 seconds. "
+                                   "The process has been terminated.")
+                               .arg(command)
+                               .arg(args.isEmpty() ? "" : args)
+                               .arg(COMMAND_TIMEOUT_MS / 1000));
+        promise->finish();
+        process->deleteLater();
+        timeoutTimer->deleteLater();
+    });
 
     QObject::connect(
         process,
         QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
-        [process, promise, command, args](int exitCode, QProcess::ExitStatus exitStatus) {
-            const QString output = QString::fromUtf8(process->readAll());
+        [this, process, promise, command, args, timeoutTimer, outputSize](
+            int exitCode, QProcess::ExitStatus exitStatus) {
+            timeoutTimer->stop();
+            timeoutTimer->deleteLater();
+
+            const QByteArray rawOutput = process->readAll();
+            *outputSize += rawOutput.size();
+            const QString output = sanitizeOutput(QString::fromUtf8(rawOutput), *outputSize);
+
+            const QString fullCommand = args.isEmpty() ? command : QString("%1 %2").arg(command).arg(args);
 
             if (exitStatus == QProcess::NormalExit) {
                 if (exitCode == 0) {
-                    LOG_MESSAGE(
-                        QString("ExecuteTerminalCommandTool: Command completed successfully"));
+                    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' completed "
+                                        "successfully (output size: %2 bytes)")
+                                    .arg(fullCommand)
+                                    .arg(*outputSize));
                     promise->addResult(
-                        QString("Command '%1 %2' executed successfully.\n\nOutput:\n%3")
-                            .arg(command)
-                            .arg(args)
+                        QString("Command '%1' executed successfully.\n\nOutput:\n%2")
+                            .arg(fullCommand)
                             .arg(output.isEmpty() ? "(no output)" : output));
                 } else {
-                    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command failed with exit "
-                                        "code %1")
-                                    .arg(exitCode));
+                    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' failed with "
+                                        "exit code %2 (output size: %3 bytes)")
+                                    .arg(fullCommand)
+                                    .arg(exitCode)
+                                    .arg(*outputSize));
                     promise->addResult(
-                        QString("Command '%1 %2' failed with exit code %3.\n\nOutput:\n%4")
-                            .arg(command)
-                            .arg(args)
+                        QString("Command '%1' failed with exit code %2.\n\nOutput:\n%3")
+                            .arg(fullCommand)
                             .arg(exitCode)
                             .arg(output.isEmpty() ? "(no output)" : output));
                 }
             } else {
-                LOG_MESSAGE("ExecuteTerminalCommandTool: Command crashed or was terminated");
+                LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Command '%1' crashed or was "
+                                    "terminated (output size: %2 bytes)")
+                                .arg(fullCommand)
+                                .arg(*outputSize));
                 const QString error = process->errorString();
                 promise->addResult(
-                    QString("Command '%1 %2' crashed or was terminated.\n\nError: %3\n\nOutput:\n%4")
-                        .arg(command)
-                        .arg(args)
+                    QString("Command '%1' crashed or was terminated.\n\nError: %2\n\nOutput:\n%3")
+                        .arg(fullCommand)
                         .arg(error)
                         .arg(output.isEmpty() ? "(no output)" : output));
             }
@@ -189,25 +271,92 @@ QFuture<QString> ExecuteTerminalCommandTool::executeAsync(const QJsonObject &inp
             process->deleteLater();
         });
 
-    QObject::connect(process, &QProcess::errorOccurred, [process, promise, command, args](
+    QObject::connect(process, &QProcess::errorOccurred, [process, promise, command, args, timeoutTimer](
                                                              QProcess::ProcessError error) {
-        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Process error occurred: %1").arg(error));
-        const QString errorString = process->errorString();
-        promise->addResult(QString("Error executing command '%1 %2': %3")
-                               .arg(command)
-                               .arg(args)
-                               .arg(errorString));
+        if (promise->future().isFinished()) {
+            return;
+        }
+
+        timeoutTimer->stop();
+        timeoutTimer->deleteLater();
+
+        const QString fullCommand = args.isEmpty() ? command : QString("%1 %2").arg(command).arg(args);
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Process error occurred for '%1': %2 (%3)")
+                        .arg(fullCommand)
+                        .arg(error)
+                        .arg(process->errorString()));
+        
+        QString errorMessage;
+        switch (error) {
+        case QProcess::FailedToStart:
+            errorMessage = QString("Failed to start command '%1'. The command may not exist or "
+                                   "you may not have permission to execute it.")
+                               .arg(fullCommand);
+            break;
+        case QProcess::Crashed:
+            errorMessage = QString("Command '%1' crashed during execution.").arg(fullCommand);
+            break;
+        case QProcess::Timedout:
+            errorMessage = QString("Command '%1' timed out.").arg(fullCommand);
+            break;
+        case QProcess::WriteError:
+            errorMessage = QString("Write error occurred while executing '%1'.").arg(fullCommand);
+            break;
+        case QProcess::ReadError:
+            errorMessage = QString("Read error occurred while executing '%1'.").arg(fullCommand);
+            break;
+        default:
+            errorMessage = QString("Unknown error occurred while executing '%1': %2")
+                               .arg(fullCommand)
+                               .arg(process->errorString());
+            break;
+        }
+        
+        promise->addResult(QString("Error: %1").arg(errorMessage));
         promise->finish();
         process->deleteLater();
     });
 
-    if (args.isEmpty()) {
-        process->start(command, QStringList());
-    } else {
-        QStringList argList = args.split(QRegularExpression("\\s+"), Qt::SkipEmptyParts);
-        process->start(command, argList);
+    QString fullCommand = command;
+    if (!args.isEmpty()) {
+        fullCommand += " " + args;
     }
 
+    QStringList splitCommand = QProcess::splitCommand(fullCommand);
+    if (splitCommand.isEmpty()) {
+        LOG_MESSAGE("ExecuteTerminalCommandTool: Failed to parse command");
+        promise->addResult(QString("Error: Failed to parse command '%1'").arg(fullCommand));
+        promise->finish();
+        process->deleteLater();
+        timeoutTimer->deleteLater();
+        return future;
+    }
+    
+    const QString program = splitCommand.takeFirst();
+    process->start(program, splitCommand);
+
+    if (!process->waitForStarted(PROCESS_START_TIMEOUT_MS)) {
+        LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Failed to start command '%1' within %2ms")
+                        .arg(fullCommand)
+                        .arg(PROCESS_START_TIMEOUT_MS));
+        const QString errorString = process->errorString();
+        promise->addResult(QString("Error: Failed to start command '%1': %2\n\n"
+                                   "Possible reasons:\n"
+                                   "- Command not found in PATH\n"
+                                   "- Insufficient permissions\n"
+                                   "- Invalid command syntax")
+                               .arg(fullCommand)
+                               .arg(errorString));
+        promise->finish();
+        process->deleteLater();
+        timeoutTimer->deleteLater();
+        return future;
+    }
+
+    LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Process started successfully (PID: %1)")
+                    .arg(process->processId()));
+
+    timeoutTimer->start();
     return future;
 }
 
@@ -217,24 +366,127 @@ bool ExecuteTerminalCommandTool::isCommandAllowed(const QString &command) const
     return allowed.contains(command, Qt::CaseInsensitive);
 }
 
+bool ExecuteTerminalCommandTool::isCommandSafe(const QString &command) const
+{
+    static const QRegularExpression safePattern("^[a-zA-Z0-9._/-]+$");
+    return safePattern.match(command).hasMatch();
+}
+
+bool ExecuteTerminalCommandTool::areArgumentsSafe(const QString &args) const
+{
+    if (args.isEmpty()) {
+        return true;
+    }
+
+    static const QStringList dangerousPatterns = {
+        ";",   // Command separator
+        "&&",  // AND operator
+        "||",  // OR operator  
+        "|",   // Pipe operator
+        ">",   // Output redirection
+        ">>",  // Append redirection
+        "<",   // Input redirection
+        "`",   // Command substitution
+        "$(",  // Command substitution
+        "$()",  // Command substitution
+        "\\n", // Newline (could start new command)
+        "\\r"  // Carriage return
+    };
+
+    for (const QString &pattern : dangerousPatterns) {
+        if (args.contains(pattern)) {
+            LOG_MESSAGE(QString("ExecuteTerminalCommandTool: Dangerous pattern '%1' found in args")
+                            .arg(pattern));
+            return false;
+        }
+    }
+
+    return true;
+}
+
+QString ExecuteTerminalCommandTool::sanitizeOutput(const QString &output, qint64 totalSize) const
+{
+    if (totalSize > MAX_OUTPUT_SIZE) {
+        const QString truncated = output.left(MAX_OUTPUT_SIZE / 2);
+        return QString("%1\n\n... [Output truncated: exceeded maximum size of %2 MB. "
+                       "Total output size was %3 bytes] ...")
+            .arg(truncated)
+            .arg(MAX_OUTPUT_SIZE / (1024 * 1024))
+            .arg(totalSize);
+    }
+
+    return output;
+}
+
 QStringList ExecuteTerminalCommandTool::getAllowedCommands() const
 {
-    const QString commandsStr
-        = Settings::toolsSettings().allowedTerminalCommands().trimmed();
+    static QString cachedCommandsStr;
+    static QStringList cachedCommands;
+
+    QString commandsStr;
+
+    // Get commands for current OS
+#ifdef Q_OS_LINUX
+    commandsStr = Settings::toolsSettings().allowedTerminalCommandsLinux().trimmed();
+#elif defined(Q_OS_MACOS)
+    commandsStr = Settings::toolsSettings().allowedTerminalCommandsMacOS().trimmed();
+#elif defined(Q_OS_WIN)
+    commandsStr = Settings::toolsSettings().allowedTerminalCommandsWindows().trimmed();
+#else
+    commandsStr = Settings::toolsSettings().allowedTerminalCommandsLinux().trimmed(); // fallback
+#endif
+
+    // Return cached result if settings haven't changed
+    if (commandsStr == cachedCommandsStr && !cachedCommands.isEmpty()) {
+        return cachedCommands;
+    }
+
+    // Update cache
+    cachedCommandsStr = commandsStr;
+    cachedCommands.clear();
 
     if (commandsStr.isEmpty()) {
         return QStringList();
     }
 
     const QStringList rawCommands = commandsStr.split(',', Qt::SkipEmptyParts);
-    QStringList commands;
-    commands.reserve(rawCommands.size());
+    cachedCommands.reserve(rawCommands.size());
     
     for (const QString &cmd : rawCommands) {
-        commands.append(cmd.trimmed());
+        const QString trimmed = cmd.trimmed();
+        if (!trimmed.isEmpty()) {
+            cachedCommands.append(trimmed);
+        }
     }
 
-    return commands;
+    return cachedCommands;
+}
+
+QString ExecuteTerminalCommandTool::getCommandDescription() const
+{
+    const QStringList allowed = getAllowedCommands();
+    const QString allowedList = allowed.isEmpty() ? "none" : allowed.join(", ");
+
+#ifdef Q_OS_LINUX
+    const QString osInfo = " Running on Linux.";
+#elif defined(Q_OS_MACOS)
+    const QString osInfo = " Running on macOS.";
+#elif defined(Q_OS_WIN)
+    const QString osInfo = " Running on Windows.";
+#else
+    const QString osInfo = "";
+#endif
+
+    return QString(
+               "Execute a terminal command in the project directory. "
+               "Only commands from the allowed list can be executed. "
+               "Currently allowed commands for this OS: %1. "
+               "The command will be executed in the root directory of the active project. "
+               "Commands have a %2 second timeout. "
+               "Returns the command output (stdout and stderr) or an error message if the command fails.%3")
+        .arg(allowedList)
+        .arg(COMMAND_TIMEOUT_MS / 1000)
+        .arg(osInfo);
 }
 
 } // namespace QodeAssist::Tools

tools/ExecuteTerminalCommandTool.hpp

@@ -40,7 +40,18 @@ public:
 
 private:
     bool isCommandAllowed(const QString &command) const;
+    bool isCommandSafe(const QString &command) const;
+    bool areArgumentsSafe(const QString &args) const;
     QStringList getAllowedCommands() const;
+    QString getCommandDescription() const;
+    QString sanitizeOutput(const QString &output, qint64 maxSize) const;
+    
+    // Constants for production safety
+    static constexpr int COMMAND_TIMEOUT_MS = 30000; // 30 seconds
+    static constexpr qint64 MAX_OUTPUT_SIZE = 10 * 1024 * 1024; // 10 MB
+    static constexpr int MAX_COMMAND_LENGTH = 1024;
+    static constexpr int MAX_ARGS_LENGTH = 4096;
+    static constexpr int PROCESS_START_TIMEOUT_MS = 3000;
 };
 
 } // namespace QodeAssist::Tools

tools/ToolsFactory.cpp

@@ -158,11 +158,11 @@ QJsonArray ToolsFactory::getToolsDefinitions(
             }
         }
 
-        // if (requiredPerms.testFlag(LLMCore::ToolPermission::NetworkAccess)) {
-        //     if (!settings.allowNetworkAccess()) {
-        //         hasPermission = false;
-        //     }
-        // }
+        if (requiredPerms.testFlag(LLMCore::ToolPermission::NetworkAccess)) {
+            if (!settings.allowNetworkAccess()) {
+                hasPermission = false;
+            }
+        }
 
         if (hasPermission) {
             toolsArray.append(it.value()->getDefinition(format));