-
Notifications
You must be signed in to change notification settings - Fork 22
no more inquirer or clicker #980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
⚡️ Codeflash found optimizations for this PR📄 55% (0.55x) speedup for
|
PR Review: Replace inquirer with inquirer-textualThanks for this substantial refactoring! This modernizes the CLI experience and improves code organization. Overall, this is a great improvement, but I've identified some security issues and areas for enhancement. 🔴 Critical Issues (Must Fix Before Merge)1. API Key Exposure in Error MessagesFile: codeflash/cli_cmds/validators.py:59 The APIKeyValidator shows invalid API keys in full in error messages. This could leak keys to logs, terminal history, or screenshots. Fix: Never display user-provided secrets in error messages. 2. Path Traversal VulnerabilityFile: codeflash/cli_cmds/validators.py:22-27 The PathExistsValidator doesn't prevent malicious paths like ../../../../etc/passwd. Users could probe filesystem structure outside the project directory. Fix: Add explicit path traversal checks to ensure paths are relative and don't contain parent directory references. 3. Empty Git Remotes Edge CaseFile: codeflash/cli_cmds/cmd_init.py (~line 170) When len(git_remotes) == 0, the code sets git_remote = "", which may cause issues downstream. Handle the no-remotes case explicitly. 🟡 High Priority Issues4. Type Safety: Missing Return Type AnnotationsFile: codeflash/cli_cmds/themed_prompts.py (lines 91-182) All functions have disabled type checking (noqa: ANN201, type: ignore). This reduces IDE support and type safety. 5. Lost Stack Traces in Error HandlingFile: codeflash/cli_cmds/cli.py:255 Changed from logger.exception() to logger.error(), losing stack traces for debugging. 6. Overly Broad Exception HandlingFile: codeflash/cli_cmds/validators.py:184 Using bare 'except Exception' could hide security-relevant or unexpected errors. 🟢 Medium Priority Issues
✅ Strengths
📊 Overall AssessmentThis is a substantial improvement that modernizes the CLI with better organization and UX. The refactoring is well-executed, but security issues must be addressed before merging. Recommendation: Approve with required changes Before Merging:
Follow-up (can be separate PR):
Great work on this refactoring! The new architecture is much cleaner and more maintainable. 🚀 |
Code Review: PR #980 - Replace inquirer with inquirer-textualThanks for this refactoring! The migration to 🔴 Critical Issues - Must Fix1. Missing Return Type Annotations (
|
PR Type
Enhancement, Other
Description
Replace Inquirer with Textual prompts
Add themed prompt wrapper module
Update CLI flows to new API
Adjust dependencies and mypy config
Diagram Walkthrough
File Walkthrough
cli_common.py
Remove legacy inquirer helpers from CLI commoncodeflash/cli_cmds/cli_common.py
cmd_init.py
Port init workflow to inquirer-textual promptscodeflash/cli_cmds/cmd_init.py
themed_prompts.py
Add themed prompt wrapper around inquirer-textualcodeflash/cli_cmds/themed_prompts.py
pyproject.toml
Update dependencies and mypy config for new promptspyproject.toml