feat: Add interactive commands to chat CLI #21091
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The code changes introduce an interactive command system for the chat CLI, enhancing usability. I've identified potential path traversal vulnerabilities and suggested improvements to exception handling and code maintainability to ensure the new functionality is robust and secure.
| filename = args[ | ||
| 0] if args else f"chat_{datetime.now():%Y%m%d_%H%M%S}.json" |
There was a problem hiding this comment.
The filename is taken directly from user input and used in a file path operation. This can lead to a path traversal vulnerability, potentially allowing a user to write files to unintended locations. I suggest validating that the resolved path is within the current working directory.
| filename = args[ | |
| 0] if args else f"chat_{datetime.now():%Y%m%d_%H%M%S}.json" | |
| filename = args[0] if args else f"chat_{datetime.now():%Y%m%d_%H%M%S}.json" | |
| # Prevent path traversal by ensuring the resolved path is within the current directory. | |
| save_path = os.path.abspath(filename) | |
| if not save_path.startswith(os.path.join(os.getcwd(), '')): | |
| return "Error: Saving files outside the current directory is not allowed." |
| if not args: | ||
| return "Usage: /load <filename>" | ||
|
|
||
| filename = args[0] |
There was a problem hiding this comment.
The filename from user input is not sanitized, creating a path traversal vulnerability. A user could potentially load a sensitive file from an arbitrary location if they have read permissions. Restrict file loading to the current directory or a designated safe directory to mitigate this risk.
filename = args[0]
# Prevent path traversal by ensuring the resolved path is within the current directory.
load_path = os.path.abspath(filename)
if not load_path.startswith(os.path.join(os.getcwd(), '')):
return "Error: Loading files from outside the current directory is not allowed."| except Exception as e: | ||
| return f"Error listing models: {e}" |
There was a problem hiding this comment.
Catching a broad Exception can mask underlying issues, such as network errors or authentication failures. Catch specific exceptions (e.g., openai.APIError) to provide more precise error feedback and avoid hiding bugs.
except openai.APIError as e:
return f"Error listing models: {e}"| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silently passing on all exceptions with except Exception: pass can hide critical errors. At a minimum, the exception should be logged so that the user is aware that fetching the model list failed and why.
| except Exception: | |
| pass | |
| except Exception as e: | |
| print(f"Warning: Could not fetch available models for completion: {e}") |
vllm/entrypoints/cli/openai.py
Outdated
| if result == "__EXIT__": | ||
| break | ||
|
|
||
| # Check for the special retry signal | ||
| if result.startswith("__RETRY__"): | ||
| # The command has already modified | ||
| # the conversation history. | ||
| # We just need to use the content | ||
| # provided in the signal | ||
| input_message = result[len("__RETRY__"):] | ||
| # Treat this as a normal chat message now | ||
| is_command_run = False | ||
| else: | ||
| print(result) |
There was a problem hiding this comment.
Using magic strings like __EXIT__ and __RETRY__ for control flow is brittle and makes the code harder to maintain. Define these as constants in chat_commands.py and import them here.
| if result == "__EXIT__": | |
| break | |
| # Check for the special retry signal | |
| if result.startswith("__RETRY__"): | |
| # The command has already modified | |
| # the conversation history. | |
| # We just need to use the content | |
| # provided in the signal | |
| input_message = result[len("__RETRY__"):] | |
| # Treat this as a normal chat message now | |
| is_command_run = False | |
| else: | |
| print(result) | |
| if result == EXIT_COMMAND_SIGNAL: | |
| break | |
| # Check for the special retry signal | |
| if result.startswith(RETRY_COMMAND_SIGNAL_PREFIX): | |
| # The command has already modified | |
| # the conversation history. | |
| # We just need to use the content | |
| # provided in the signal | |
| input_message = result[len(RETRY_COMMAND_SIGNAL_PREFIX):] | |
| # Treat this as a normal chat message now | |
| is_command_run = False | |
| else: | |
| print(result) |
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Signed-off-by: reidliu41 <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
Currently, the chat CLI is very basic. Common actions like changing the model, adjusting
the system prompt, or even clearing the screen require restarting the entire
application. This can be inefficient for anyone using the CLI to test models and prompts
interactively.
This PR introduces an interactive command system, accessible via /, that allows for
real-time interaction and control over the chat session, transforming it into a more
efficient tool.
Session Management:
Model & Prompt Control:
Conversation Workflow:
Persistence:
Readline Enhancements: Integrates the readline module for persistent command history
and basic tab-completion for commands.
Fully Optional: The entire feature can be disabled at startup with the --disable-commands flag
Test Plan
Test Result
(Optional) Documentation Update