Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new helper to verify CLI commands in Slackbot and integrates it into the core assistant directives.
- Introduces
check_cli_commandinsearch.pyfor running and validating Prefect CLI commands. - Imports and enforces use of
check_cli_commandincore.pydirectives.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| examples/slackbot/src/slackbot/search.py | Added new check_cli_command function to run CLI commands with timeout, output capture, and error handling. |
| examples/slackbot/src/slackbot/core.py | Imported check_cli_command and updated mission directives to require its use before suggesting CLI commands. |
Comments suppressed due to low confidence (2)
examples/slackbot/src/slackbot/search.py:231
- New functionality is added without corresponding tests; consider adding unit tests to cover success, timeout, not-found, and error cases.
def check_cli_command(command: str, args: list[str] | None = None) -> str:
examples/slackbot/src/slackbot/core.py:42
- [nitpick] This directive mixes sentence case and full uppercase; consider revising to match existing style for readability and consistency.
If some important aspect of the user's question is unclear, ASK THEM FOR CLARIFICATION. ADMIT WHEN YOU CANNOT FIND THE ANSWER.
| args = [] | ||
|
|
||
| # Construct the full command | ||
| full_command = command.split() + args |
There was a problem hiding this comment.
Using str.split() can mis-handle quoted arguments; consider using shlex.split(command) to correctly parse complex command strings.
| full_command = command.split() + args | |
| full_command = shlex.split(command) + args |
| if result.stdout: | ||
| output += result.stdout | ||
| if result.stderr: | ||
| output += f"\n[stderr]: {result.stderr}" |
There was a problem hiding this comment.
[nitpick] When stderr is present but stdout is empty, this prepends a newline; you may want to trim or conditionally format to avoid leading whitespace.
| output += f"\n[stderr]: {result.stderr}" | |
| output += f"{'\n' if result.stdout else ''}[stderr]: {result.stderr}" |
| if not output.strip(): | ||
| output = f"Command ran successfully with exit code {result.returncode} but produced no output" | ||
|
|
||
| return output[:2000] # Limit output length to prevent huge responses |
There was a problem hiding this comment.
The 2000 character limit is a magic number; consider extracting it into a named constant or configuring it elsewhere for clarity.
| return output[:2000] # Limit output length to prevent huge responses | |
| return output[:MAX_OUTPUT_LENGTH] # Limit output length to prevent huge responses |
No description provided.