sec(cli): introduce secure command execution wrapper and cross-platfo…#348
sec(cli): introduce secure command execution wrapper and cross-platfo…#348vedant-kawale-27 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesSecure subprocess wrapper and CLI hardening
Sequence DiagramsequenceDiagram
participant Caller
participant run as run_cmd.run()
participant subprocess
Caller->>run: run(args, check_args=True)
run->>run: validate args against ALLOWED_ARG_RE
alt forbidden chars found
run-->>Caller: raise CommandError(invalid chars)
end
run->>subprocess: subprocess.run(args, shell=False, capture_output=True, text=True)
alt returncode != 0
subprocess-->>run: CompletedProcess(returncode>0)
run-->>Caller: raise CommandError(returncode, stdout, stderr)
else FileNotFoundError / OSError
subprocess-->>run: raise exception
run-->>Caller: raise CommandError("execution failed")
else success
subprocess-->>run: CompletedProcess(returncode=0)
run-->>Caller: return CompletedProcess
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/_utils/run_cmd.py`:
- Around line 78-104: The logging calls are exposing sensitive information that
could persist in logs. At line 78, the logger.debug call logs the full command
string which could contain tokens/secrets in the arguments. More critically, at
lines 99-104, the logger.error call logs complete stdout and stderr outputs
which could leak sensitive data. Remove or redact the sensitive payload details
from these logging calls. Either eliminate the full command string logging
entirely, log only the command name (args[0]) instead of the full args, and
reduce the stderr/stdout logging to a summary message at ERROR level (or move it
to DEBUG level) without including the raw output payloads.
In `@agentwatch/cli/_utils/speech.py`:
- Around line 33-52: The TTS subprocess calls in the speech synthesis function
lack timeouts, which can cause them to hang indefinitely. Add a timeout
parameter to all three run_cmd.run() calls: the macOS 'say' command call, the
Windows PowerShell SpeechSynthesis call, and the Linux espeak call. Choose an
appropriate timeout value (in seconds) that allows sufficient time for speech
synthesis to complete while preventing indefinite hangs. Apply the same timeout
consistently across all three platforms to ensure uniform behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e149ad-48b3-41e1-b6fe-0045727cd601
📒 Files selected for processing (12)
README.mdagentwatch/cli/_utils/__init__.pyagentwatch/cli/_utils/run_cmd.pyagentwatch/cli/_utils/speech.pyagentwatch/cli/animator.pyagentwatch/cli/demo.pyagentwatch/cli/main.pyagentwatch/cli/verify_env.pytests/test_channels.pytests/test_cli_status.pytests/test_cost.pytests/test_run_cmd.py
💤 Files with no reviewable changes (1)
- tests/test_channels.py
|
hee @sreerevanth review the pr, and if everything looks good approve it |
🧪 PR Test Results
Python 3.12 · commit 854bae2 |
|
hee @sreerevanth if everything is good to go then merge this pr and add SSocbadge and hardness level badge |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/animator.py`:
- Around line 83-92: The subprocess.Popen call that executes the PowerShell
command is building the command string with unsanitized user input interpolated
directly, creating a script-injection vulnerability. Replace this legacy direct
subprocess.Popen execution with a secure wrapper function designed for safe
PowerShell command execution. Ensure that the user variable is properly escaped
and quoted within the PowerShell command to prevent injection attacks, or use
the secure wrapper that handles this escaping automatically. Reference the
secure execution path mentioned in the review and apply it to this code block
instead of the direct Popen call at line 87.
In `@agentwatch/cli/main.py`:
- Around line 851-852: The refresh_rate parameter in the function signature
(around line 851) uses typer.Option without any constraints, but the code
divides by this value later at lines 937-938, causing a division-by-zero crash
if --refresh 0 is passed. Add validation to the typer.Option call for the
refresh_rate parameter by using the gt (greater than) constraint to ensure the
value must be greater than 0, which will prevent invalid inputs at the CLI level
rather than causing runtime errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c0e89d-1c10-4736-8287-157dd96f8352
📒 Files selected for processing (3)
agentwatch/cli/animator.pyagentwatch/cli/main.pyagentwatch/cli/verify_env.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/cli/verify_env.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/animator.py`:
- Around line 83-92: The subprocess.Popen call that executes the PowerShell
command is building the command string with unsanitized user input interpolated
directly, creating a script-injection vulnerability. Replace this legacy direct
subprocess.Popen execution with a secure wrapper function designed for safe
PowerShell command execution. Ensure that the user variable is properly escaped
and quoted within the PowerShell command to prevent injection attacks, or use
the secure wrapper that handles this escaping automatically. Reference the
secure execution path mentioned in the review and apply it to this code block
instead of the direct Popen call at line 87.
In `@agentwatch/cli/main.py`:
- Around line 851-852: The refresh_rate parameter in the function signature
(around line 851) uses typer.Option without any constraints, but the code
divides by this value later at lines 937-938, causing a division-by-zero crash
if --refresh 0 is passed. Add validation to the typer.Option call for the
refresh_rate parameter by using the gt (greater than) constraint to ensure the
value must be greater than 0, which will prevent invalid inputs at the CLI level
rather than causing runtime errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c0e89d-1c10-4736-8287-157dd96f8352
📒 Files selected for processing (3)
agentwatch/cli/animator.pyagentwatch/cli/main.pyagentwatch/cli/verify_env.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/cli/verify_env.py
🛑 Comments failed to post (2)
agentwatch/cli/animator.py (1)
83-92:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace legacy
subprocess.PopenPowerShell execution with the secure wrapper and escape user input.
Line 84builds a PowerShell command string with interpolateduser, andLine 87executes it via directsubprocess.Popen. This bypasses the new hardened execution path and leaves a script-injection surface in PowerShell command parsing.Proposed direction
- if sys.platform == "win32": - cmd = f"Add-Type -AssemblyName System.speech; $synth = New-Object System.Speech.Synthesis.SpeechSynthesizer; $synth.Rate = 1; $synth.Speak('Welcome {user}, to Agent Watch.')" - subprocess.Popen( - ["powershell", "-WindowStyle", "Hidden", "-Command", cmd], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - creationflags=subprocess.CREATE_NO_WINDOW, - ) + if sys.platform == "win32": + from agentwatch.cli._utils.run_cmd import run + safe_user = user.replace("'", "''") + cmd = ( + "Add-Type -AssemblyName System.Speech; " + "$synth = New-Object System.Speech.Synthesis.SpeechSynthesizer; " + "$synth.Rate = 1; " + f"$synth.Speak('Welcome {safe_user}, to Agent Watch.')" + ) + run(["powershell", "-WindowStyle", "Hidden", "-Command", cmd], check_args=False)🧰 Tools
🪛 ast-grep (0.43.0)
[error] 86-91: Command coming from incoming request
Context: subprocess.Popen( # nosec # noqa: S603, S607
["powershell", "-WindowStyle", "Hidden", "-Command", cmd], # noqa: S607
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
creationflags=subprocess.CREATE_NO_WINDOW,
)
Note: [CWE-20].(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/animator.py` around lines 83 - 92, The subprocess.Popen call that executes the PowerShell command is building the command string with unsanitized user input interpolated directly, creating a script-injection vulnerability. Replace this legacy direct subprocess.Popen execution with a secure wrapper function designed for safe PowerShell command execution. Ensure that the user variable is properly escaped and quoted within the PowerShell command to prevent injection attacks, or use the secure wrapper that handles this escaping automatically. Reference the secure execution path mentioned in the review and apply it to this code block instead of the direct Popen call at line 87.agentwatch/cli/main.py (1)
851-852:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
--refreshto prevent runtime crash.
Line 937divides byrefresh_rate, butLine 851does not constrain the value. Passing--refresh 0crashes with division-by-zero.Suggested fix
- refresh_rate: float = typer.Option(1.0, "--refresh", help="Refresh rate in seconds"), + refresh_rate: float = typer.Option( + 1.0, "--refresh", min=0.1, help="Refresh rate in seconds (must be > 0)" + ),Also applies to: 937-938
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/main.py` around lines 851 - 852, The refresh_rate parameter in the function signature (around line 851) uses typer.Option without any constraints, but the code divides by this value later at lines 937-938, causing a division-by-zero crash if --refresh 0 is passed. Add validation to the typer.Option call for the refresh_rate parameter by using the gt (greater than) constraint to ensure the value must be greater than 0, which will prevent invalid inputs at the CLI level rather than causing runtime errors.
|
hee @sreerevanth check it |
|
hee @sreerevanth everything is green and if everything is good merge it and add SSoC badge and hardness badge |
|
hey @vedant-kawale-27 Thanks for the contribution. The centralized command wrapper is a good direction, but this PR currently mixes security hardening with speech synthesis, CLI UX changes, demo updates, and documentation updates. I'd like to keep the scope focused on the command-execution hardening itself. Before merge: Remove or separate unrelated speech/animation changes into a follow-up PR. Once those are addressed, I'll take another look. |
Summary of Work Done
hee @sreerevanth check it |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_run_cmd.py (1)
35-60:⚠️ Potential issue | 🟠 MajorDocument or restrict characters allowed by ALLOWED_ARG_RE regex.
The current regex pattern allows several characters (
$,&, parentheses, braces) that enable command substitution and chaining patterns ($(command),${var},&&,||). Whilesubprocess.run(..., shell=False)prevents shell interpretation at the subprocess level, the defense-in-depth validation should either:
- Add explicit tests documenting that these characters are intentionally allowed (with a comment explaining why they are safe despite their shell metacharacter status), or
- Restrict the regex to remove
$,&, and other unnecessary characters if they are not required by legitimate use casesThe current test coverage for
;,|,>, and backticks should be complemented with tests clarifying the behavior of$(...),${...},&&, and||patterns to prevent future confusion about what the validator actually blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_run_cmd.py` around lines 35 - 60, The test_run_cmd_invalid_arguments function tests forbidden characters like semicolons, pipes, redirects, and backticks, but does not test command substitution patterns like `$(command)`, `${var}`, or logical operators like `&&` and `||` that may be allowed by the ALLOWED_ARG_RE regex. Add test cases in test_run_cmd_invalid_arguments that verify the behavior for these patterns (either confirming they raise CommandError or documenting with comments why they are intentionally allowed despite being shell metacharacters). If these characters should be blocked, update the ALLOWED_ARG_RE regex pattern to exclude `$`, `&`, parentheses, and braces, ensuring the validation strategy is explicit and well-tested for all potentially dangerous shell metacharacters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/main.py`:
- Around line 851-858: Remove the redundant runtime validation check for
refresh_rate that checks if refresh_rate <= 0. The typer.Option definition with
min=0.1 already enforces that refresh_rate must be greater than or equal to 0.1
before the command function executes, making the subsequent if statement that
raises typer.BadParameter unnecessary. Delete the entire if block that validates
refresh_rate <= 0 since Typer will handle this validation at the parameter
level.
---
Outside diff comments:
In `@tests/test_run_cmd.py`:
- Around line 35-60: The test_run_cmd_invalid_arguments function tests forbidden
characters like semicolons, pipes, redirects, and backticks, but does not test
command substitution patterns like `$(command)`, `${var}`, or logical operators
like `&&` and `||` that may be allowed by the ALLOWED_ARG_RE regex. Add test
cases in test_run_cmd_invalid_arguments that verify the behavior for these
patterns (either confirming they raise CommandError or documenting with comments
why they are intentionally allowed despite being shell metacharacters). If these
characters should be blocked, update the ALLOWED_ARG_RE regex pattern to exclude
`$`, `&`, parentheses, and braces, ensuring the validation strategy is explicit
and well-tested for all potentially dangerous shell metacharacters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5632d8f-92f2-4a02-bce2-3788629e6393
📒 Files selected for processing (8)
agentwatch/cli/_utils/__init__.pyagentwatch/cli/_utils/run_cmd.pyagentwatch/cli/animator.pyagentwatch/cli/demo.pyagentwatch/cli/main.pyagentwatch/cli/verify_env.pytests/test_cli_status.pytests/test_run_cmd.py
💤 Files with no reviewable changes (2)
- agentwatch/cli/verify_env.py
- agentwatch/cli/demo.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/cli/_utils/run_cmd.py
|
hee @sreerevanth check it |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/_utils/__init__.py`:
- Around line 9-11: Remove the stale import of run_validated_command from the
run_cmd module on line 9, as this symbol does not exist in
agentwatch/cli/_utils/run_cmd.py and is causing an ImportError. Update the
__all__ list on line 11 to export only the symbols that actually exist in the
run_cmd module (such as run and CommandError instead of the non-existent
run_validated_command) to properly maintain the import contract for the _utils
package.
In `@agentwatch/cli/_utils/run_cmd.py`:
- Around line 149-154: The validation logic in the cmd_args checks currently
only enforces that arguments are provided as a list of strings, but is missing
the allowlist-based character validation that was part of the original security
model. Add validation after the existing type checks that enforces an allowlist
of permitted characters for each argument in cmd_args to properly harden the
command execution against untrusted CLI input. This allowlist validation should
examine each string argument and ensure all characters match the allowed set
defined by the module's security model before the subprocess.run call occurs.
- Around line 169-174: The exception handling in the except blocks for
subprocess.TimeoutExpired and subprocess.CalledProcessError is re-raising the
raw subprocess exceptions, which breaks the wrapper's contract. Instead of
re-raising these exceptions, convert them to CommandError with structured fields
that capture the relevant error information (such as the timeout duration from
TimeoutExpired and the return code and stderr from CalledProcessError). This
ensures callers only need to handle CommandError and have access to the
underlying error details through CommandError's structured fields.
In `@agentwatch/cli/main.py`:
- Around line 1554-1564: The RollbackEngine is instantiated with no state
initialization before calling rollback_session(), which causes the method to
fail because _session_checkpoints is empty and undiscoverable. Before calling
rollback_session() on the newly created RollbackEngine instance, add a
state-hydration step to load or index the existing checkpoints (such as invoking
a load method or similar initialization routine that populates
_session_checkpoints), or alternatively refactor to use the existing API or
server-based rollback path that already maintains active engine state to avoid
creating an empty RollbackEngine instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e89a2cf9-7bdd-4974-8e60-cc82dcdcc779
📒 Files selected for processing (6)
README.mdagentwatch/cli/_utils/__init__.pyagentwatch/cli/_utils/run_cmd.pyagentwatch/cli/demo.pyagentwatch/cli/main.pytests/test_run_cmd.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- agentwatch/cli/demo.py
- tests/test_run_cmd.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/cli/_utils/__init__.py`:
- Around line 9-11: Remove the stale import of run_validated_command from the
run_cmd module on line 9, as this symbol does not exist in
agentwatch/cli/_utils/run_cmd.py and is causing an ImportError. Update the
__all__ list on line 11 to export only the symbols that actually exist in the
run_cmd module (such as run and CommandError instead of the non-existent
run_validated_command) to properly maintain the import contract for the _utils
package.
In `@agentwatch/cli/_utils/run_cmd.py`:
- Around line 149-154: The validation logic in the cmd_args checks currently
only enforces that arguments are provided as a list of strings, but is missing
the allowlist-based character validation that was part of the original security
model. Add validation after the existing type checks that enforces an allowlist
of permitted characters for each argument in cmd_args to properly harden the
command execution against untrusted CLI input. This allowlist validation should
examine each string argument and ensure all characters match the allowed set
defined by the module's security model before the subprocess.run call occurs.
- Around line 169-174: The exception handling in the except blocks for
subprocess.TimeoutExpired and subprocess.CalledProcessError is re-raising the
raw subprocess exceptions, which breaks the wrapper's contract. Instead of
re-raising these exceptions, convert them to CommandError with structured fields
that capture the relevant error information (such as the timeout duration from
TimeoutExpired and the return code and stderr from CalledProcessError). This
ensures callers only need to handle CommandError and have access to the
underlying error details through CommandError's structured fields.
In `@agentwatch/cli/main.py`:
- Around line 1554-1564: The RollbackEngine is instantiated with no state
initialization before calling rollback_session(), which causes the method to
fail because _session_checkpoints is empty and undiscoverable. Before calling
rollback_session() on the newly created RollbackEngine instance, add a
state-hydration step to load or index the existing checkpoints (such as invoking
a load method or similar initialization routine that populates
_session_checkpoints), or alternatively refactor to use the existing API or
server-based rollback path that already maintains active engine state to avoid
creating an empty RollbackEngine instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e89a2cf9-7bdd-4974-8e60-cc82dcdcc779
📒 Files selected for processing (6)
README.mdagentwatch/cli/_utils/__init__.pyagentwatch/cli/_utils/run_cmd.pyagentwatch/cli/demo.pyagentwatch/cli/main.pytests/test_run_cmd.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- agentwatch/cli/demo.py
- tests/test_run_cmd.py
🛑 Comments failed to post (4)
agentwatch/cli/_utils/__init__.py (1)
9-11:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove stale
run_validated_commandexport; it breaks_utilsimport contract.Line 9 imports a symbol that is not present in
agentwatch/cli/_utils/run_cmd.py, and Line 11 overwrites__all__to that stale name. This can raiseImportErrorand hidesrun/CommandErrorfrom package exports.Suggested fix
-from .run_cmd import run_validated_command - -__all__ = ["run_validated_command"] +__all__ = ["run", "CommandError"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/_utils/__init__.py` around lines 9 - 11, Remove the stale import of run_validated_command from the run_cmd module on line 9, as this symbol does not exist in agentwatch/cli/_utils/run_cmd.py and is causing an ImportError. Update the __all__ list on line 11 to export only the symbols that actually exist in the run_cmd module (such as run and CommandError instead of the non-existent run_validated_command) to properly maintain the import contract for the _utils package.agentwatch/cli/_utils/run_cmd.py (2)
149-154:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore allowlist-based argument validation before
subprocess.run.The current validation only enforces
list[str]; it no longer enforces the argument-character allowlist described by this module’s security model. That weakens command hardening for untrusted CLI input paths.Suggested patch
def run_validated_command( cmd_args: list[str], check: bool = True, timeout: int | float | None = 30.0 ) -> tuple[int, str, str]: @@ if not all(isinstance(arg, str) for arg in cmd_args): raise ValueError("All command arguments must be strings.") + + for arg in cmd_args: + if not ALLOWED_ARG_RE.fullmatch(arg): + raise CommandError(f"Invalid characters in command argument: {arg!r}")Also applies to: 157-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/_utils/run_cmd.py` around lines 149 - 154, The validation logic in the cmd_args checks currently only enforces that arguments are provided as a list of strings, but is missing the allowlist-based character validation that was part of the original security model. Add validation after the existing type checks that enforces an allowlist of permitted characters for each argument in cmd_args to properly harden the command execution against untrusted CLI input. This allowlist validation should examine each string argument and ensure all characters match the allowed set defined by the module's security model before the subprocess.run call occurs.
169-174:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
CommandErrorconsistently instead of re-raising raw subprocess exceptions.Re-raising
TimeoutExpired/CalledProcessErrorbreaks the wrapper’s contract and forces callers to handle multiple exception types. Convert these toCommandErrorwith structured fields.Suggested patch
- except subprocess.TimeoutExpired as e: - logger.error(f"Command timed out after {timeout} seconds: {e}") - raise - except subprocess.CalledProcessError as e: - logger.error(f"Command failed with exit code {e.returncode}: {e.stderr}") - raise + except subprocess.TimeoutExpired as e: + raise CommandError( + f"Command timed out after {timeout} seconds", + stdout=e.stdout, + stderr=e.stderr, + ) from e + except subprocess.CalledProcessError as e: + raise CommandError( + f"Command failed with exit code {e.returncode}", + returncode=e.returncode, + stdout=e.stdout, + stderr=e.stderr, + ) from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/_utils/run_cmd.py` around lines 169 - 174, The exception handling in the except blocks for subprocess.TimeoutExpired and subprocess.CalledProcessError is re-raising the raw subprocess exceptions, which breaks the wrapper's contract. Instead of re-raising these exceptions, convert them to CommandError with structured fields that capture the relevant error information (such as the timeout duration from TimeoutExpired and the return code and stderr from CalledProcessError). This ensures callers only need to handle CommandError and have access to the underlying error details through CommandError's structured fields.agentwatch/cli/main.py (1)
1554-1564: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
session rollbacklikely always fails due to unhydratedRollbackEnginestate.Line 1554 constructs a new
RollbackEngine()with empty indexes, and Line 1563 callsrollback_session()immediately. Inagentwatch/rollback/engine.py,rollback_session()reads_session_checkpoints; without a load/index step, existing checkpoints are not discoverable.A reliable fix is to add a state-hydration path before rollback (or invoke the API/server rollback path that already has active engine state), then call
rollback_session().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/cli/main.py` around lines 1554 - 1564, The RollbackEngine is instantiated with no state initialization before calling rollback_session(), which causes the method to fail because _session_checkpoints is empty and undiscoverable. Before calling rollback_session() on the newly created RollbackEngine instance, add a state-hydration step to load or index the existing checkpoints (such as invoking a load method or similar initialization routine that populates _session_checkpoints), or alternatively refactor to use the existing API or server-based rollback path that already maintains active engine state to avoid creating an empty RollbackEngine instance.
could have pinged me in discord |
|
hee @sreerevanth cleared the problem that cause the CI to fail |
Type of Change
Description
This PR addresses a potential command-injection vulnerability in the AgentWatch CLI by introducing a secure, centralized command execution wrapper (
agentwatch/cli/_utils/run_cmd.py) and integrating it with cross-platform speech synthesis alerts.Proposed Changes
Central Secure Command Utility (
agentwatch/cli/_utils/run_cmd.py):subprocess.run(..., shell=False).;,|,<,>,`to prevent command injection.check_argsparameter (defaultTrue) to allow bypassing validation for internally-controlled scripts (like PowerShell commands) that require semicolons.CommandErrorexception carrying thereturncode,stdout, andstderron command failures.DEBUGlevel.Speech Synthesis with Graceful Fallback (
agentwatch/cli/_utils/speech.py):speak(message)across platform speech commands:saycommand.espeakcommand.Falseif voice synthesis tools are not installed or fail.Visual Spinner & Audible Blocks (
agentwatch/cli/animator.py):CLIAnimatorsupplying visual spinner animations and safety block voice triggers.speakalerts intoverify_env.py(diagnostics completion),main.py(tool block trigger), anddemo.py(safety demo tool block).Documentation:
README.mdfile with a usage example of the newrunhelper under the Safety Engine section.Related Issue
Closes #332
How Has This Been Tested?
tests/test_run_cmd.pycovering validation rules, exceptions, and platform speech mocks..venv\Scripts\pytest(All 449 unit tests passed).venv\Scripts\ruff check agentwatch//.venv\Scripts\ruff format --check agentwatch/(All checks passed)agentwatch.exe serve,agentwatch.exe safety) and the interactive demo (demo.py) on Windows to confirm no encoding crashes.Checklist
Summary by CodeRabbit
New Features
CommandErrorfor failures.Bug Fixes
Improvements
Tests