m-a-r-i-b / groq-terminal-ai

6 stars 1 forks source link

Potentially Unsafe and Incomplete Execution of Commands with `pyautogui`** #1

Open saademad200 opened 2 months ago

saademad200 commented 2 months ago

Problem Description:

The current implementation automatically writes and executes terminal commands using pyautogui.write(). There are two key issues:

  1. Unreviewed execution: Commands are written directly into the terminal without giving the user the opportunity to review them before execution.
  2. Incomplete execution:
    • When a single command is generated, it is not properly executed because it is not followed by an "Enter" keypress.
    • In the case of multiple commands, all but the last command are executed automatically. The user has to manually press "Enter" to execute the last command, which disrupts automation and workflow consistency.

Current Code:

command = ""
for chunk in chat_completion:
    chunk_content = chunk.choices[0].delta.content
    if chunk_content is not None:
        command += chunk_content
        pyautogui.write(chunk_content)

Risks:

Proposed Fix:

  1. Command Review: Print the generated command(s) for manual review before execution.
  2. Confirmation Step: Ask the user to confirm before proceeding with the execution.
  3. Proper Command Execution:
    • Ensure that even single commands are followed by an "Enter" keypress.
    • For multiple commands, split them by newline and execute each one individually, ensuring all commands, including the last one, are executed automatically.

Suggested Code Update:

command = ""
for chunk in chat_completion:
    chunk_content = chunk.choices[0].delta.content
    if chunk_content is not None:
        command += chunk_content

# Output the complete command for review
print(f"{command}")

# Split the command by newlines for execution, but keep it unchanged in the command variable
commands = command.splitlines()

# Ask for confirmation before executing the commands
if input("Do you want to execute this/these command(s)? [y/N]: ").lower() == 'y':
    for cmd in commands:
        pyautogui.write(cmd)
        pyautogui.press("enter")  # Ensure each command is executed by pressing "Enter" after it
else:
    print("Command execution aborted.")

Benefits of the Fix:

Severity:

m-a-r-i-b commented 2 months ago

I appreciate the intent for improvement. Here are my comments,

1. Unreviewed execution: Commands are written directly into the terminal without giving the user the opportunity to review them before execution. The command is indeed written to the terminal, however it is intentionally not executed to let the user review/modify it before execution. Item 2 below actually confirms this.

2. Incomplete execution: - When a single command is generated, it is not properly executed because it is not followed by an "Enter" keypress. This is by design, see comment above.

- Incomplete command execution: Single commands do not execute, and in the case of multiple commands, the last one is skipped unless manually entered by the user, leading to potential errors in workflows. I admit to have not tested this for "multiple commands" and I see two concerns around that .

  1. In correct multi line output: I trust/rely-on the LLM to structure multi-line commands into a single cohesive one with appropriate '\' breaks. However, if this can be addressed in a more "deterministic" way, I'd be happy to have that fix.
  2. Automatic execution of N-1 commands in case of multiple command output: If in the case of multiple commands, all but the last command is being executed then its not the intended behavior. No command should be auto executed. Would love a fix, if this really is the case.

MISC : I like the idea of printing commands to terminal and letting user execute them by entering their way through, but this will bare the user with the option to modify the commands at hand if they wanted to.


The package should have explained its design philosophy but in the haste of things I left that out. I hope the response above adds some clarity.