Feature/config wizard #329
Conversation
WalkthroughThis PR introduces a new "config" subcommand to the Cortex CLI that enables interactive configuration. The implementation adds a public config method to the CortexCLI class and wires the subcommand into the argument parser for command dispatch. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
pavanimanchala53
force-pushed
the
feature/config-wizard
branch
from
9676e3f to
fe307a6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
cortex/roles/loader.py (2)
6-7: Add a docstring to the exception class.Per coding guidelines, docstrings are required for public APIs. Adding a brief docstring improves clarity.
🔎 Proposed fix
class RoleNotFoundError(Exception): - pass + """Raised when a requested role configuration file cannot be found."""
10-14: Add return type hint.Per coding guidelines, type hints are required in Python code.
🔎 Proposed fix
-def get_roles_dir(): +def get_roles_dir() -> str: """ Returns the directory where built-in roles are stored. """ return os.path.dirname(__file__)cortex/llm/interpreter.py (2)
28-30: Consider more specific type annotation for cache parameter.The cache type was changed from
SemanticCache | NonetoAny | None, which reduces type safety. While this provides flexibility, it eliminates compile-time type checking for cache operations.🔎 Consider retaining the specific type
- cache: Any | None = None, + cache: "SemanticCache | None" = None,This preserves type safety while avoiding circular imports via the string annotation.
91-93: Simplify system_prompt check.Using
getattr(self, "system_prompt", "")is defensive, butself.system_promptis always initialized in__init__(line 39). A direct attribute check would be clearer and equally safe.🔎 Simplified version
- if getattr(self, "system_prompt", ""): + if self.system_prompt: return f"{self.system_prompt}\n\n{base_prompt}" return base_promptcortex/cli.py (2)
37-38: Misleading comment about lazy initialization.The comment "Lazy initialization" at line 37 is inaccurate since
self.roleis assigned immediately at line 38. Consider updating or removing the comment.🔎 Suggested fix
- self.prefs_manager = None - self.role = role # Lazy initialization + self.prefs_manager = None # Lazy initialization + self.role = role
184-274: Use cx_print() for consistency with the rest of the CLI.The
config()method uses plainprint()statements, while the rest of the CLI usescx_print()for styled, consistent output. This creates an inconsistent user experience.Consider replacing print statements with cx_print for status messages:
print("\n🧠 CORTEX INTERACTIVE SETUP")→cx_print("🧠 CORTEX INTERACTIVE SETUP", "info")print("✔ GPU detected: ...")→cx_print(f"✔ GPU detected: {hw.gpu}", "success")print("❌ Invalid choice...")→self._print_error("Invalid choice. Please re-run cortex config.")This maintains visual consistency across all Cortex commands.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cortex/cli.py(6 hunks)cortex/llm/interpreter.py(4 hunks)cortex/llm_router.py(3 hunks)cortex/roles/default.yaml(1 hunks)cortex/roles/devops.yaml(1 hunks)cortex/roles/loader.py(1 hunks)cortex/roles/security.yaml(1 hunks)examples/parallel_llm_demo.py(3 hunks)test_parallel_llm.py(9 hunks)tests/test_llm_router.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_llm_router.pytest_parallel_llm.pycortex/roles/loader.pyexamples/parallel_llm_demo.pycortex/llm_router.pycortex/llm/interpreter.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_llm_router.py
🧬 Code graph analysis (5)
tests/test_llm_router.py (1)
cortex/llm_router.py (1)
check_hardware_configs_parallel(824-872)
test_parallel_llm.py (1)
cortex/llm_router.py (4)
query_multiple_packages(725-772)diagnose_errors_parallel(775-821)check_hardware_configs_parallel(824-872)acomplete(441-505)
examples/parallel_llm_demo.py (1)
cortex/llm_router.py (1)
query_multiple_packages(725-772)
cortex/llm/interpreter.py (1)
cortex/roles/loader.py (1)
load_role(17-32)
cortex/cli.py (1)
cortex/llm/interpreter.py (1)
CommandInterpreter(17-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Package
🔇 Additional comments (16)
tests/test_llm_router.py (1)
710-710: Stylistic formatting only; logic unchanged.The multi-line function call has been collapsed to a single line while preserving all arguments and test assertions. No functional impact.
cortex/llm_router.py (3)
762-765: Whitespace refactor improves readability without semantic change.The nested dictionary entry has been reformatted across multiple lines with an explicit trailing comma. This is a purely stylistic improvement that aligns with similar patterns in the file.
804-804: String concatenation split for clarity; no behavioral change.The system prompt has been split across two lines using implicit string literal concatenation (a standard Python pattern). The resulting string is identical; this improves readability of long prompts.
862-865: Consistent dict formatting; no functional impact on hardware config logic.The nested dictionary entry mirrors the reformatting at lines 762–765, maintaining consistent style across parallel helper functions while preserving all routing and configuration logic.
test_parallel_llm.py (4)
51-51: Unnecessary f-string prefixes removed; code quality improvement.Lines 51, 98, 146, and 243 remove
ffrom string literals that contain no interpolation. This is a minor but consistent improvement in code clarity and aligns with PEP 8 best practices.Also applies to: 98-98, 146-146, 243-243
112-113: Whitespace formatting adds visual clarity to exception handling.A blank line inserted before
traceback.print_exc()improves readability of the error handling block without altering behavior or semantics.
173-173: Multi-line function calls collapsed to single lines; consistent with test refactors.The calls to
query_multiple_packages(),diagnose_errors_parallel(), andcheck_hardware_configs_parallel()have been collapsed to single-line invocations. Arguments remain unchanged, and test assertions are unaffected. This matches a similar refactor intests/test_llm_router.py:710.Also applies to: 184-184, 195-195
231-233: Unconventional dict filtering pattern in test—functional but verbose.The dict comprehension
**{k: v for k, v in req.items() if k != "task_type"}followed by explicittask_type=req["task_type"]is a roundabout way to pass request arguments. A simpler approach would be direct unpacking:await router.acomplete(**req). However, since this is a test utility and the current pattern is functional, no change is required.examples/parallel_llm_demo.py (1)
15-15: LGTM!The formatting changes improve readability: the blank line after imports follows PEP 8 conventions, the function call consolidation on line 39 is cleaner, and the multi-line string formatting on lines 213-215 maintains line length standards.
Also applies to: 39-39, 213-215
cortex/roles/devops.yaml (1)
1-6: LGTM!The DevOps role definition follows a consistent structure with other roles and provides appropriate system prompt guidance emphasizing reliability, automation, scalability, and idempotent/infrastructure-as-code approaches.
cortex/roles/security.yaml (1)
1-7: LGTM!The security role definition provides appropriate guidance for a security-focused assistant, emphasizing safety, least privilege, risk mitigation, and warnings for destructive actions. The structure is consistent with other role files.
cortex/roles/default.yaml (1)
1-6: LGTM!The default role definition provides appropriate general-purpose guidance and serves as the essential fallback for the role loading system. The structure is consistent with other role definitions.
cortex/llm/interpreter.py (1)
213-218: LGTM - Formatting consistency improvement.The explicit formatting of the method signature improves readability without changing behavior.
cortex/cli.py (3)
433-433: LGTM - Role integration with CommandInterpreter.The interpreter is now correctly instantiated with the
roleparameter, aligning with the updated constructor signature incortex/llm/interpreter.py.
908-912: LGTM - Role CLI argument integration.The
--roleargument is properly defined with appropriate default value and help text.
1005-1013: LGTM - Config command dispatching.The config subcommand is properly wired with role propagation to CortexCLI and correct dispatch to
cli.config().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
cortex/cli.py (6)
187-213: Add error handling for hardware detection.The
detect_hardware()call on line 189 lacks error handling. If hardware detection fails or raises an exception, the config wizard will crash. This is a critical issue because the config wizard should be resilient and allow users to proceed with limited hardware information.Wrap the call in a try/except block and set
hw = Noneon failure, then check forhwbefore accessing attributes throughout the method (lines 191-212).
226-230: Critical: Provider naming mismatch will cause ValueError.Line 227 uses
"anthropic"in the provider_map, but theAPIProviderenum incortex/llm/interpreter.pydefinesCLAUDE = "claude". WhenCommandInterpreteris instantiated on line 443, the call toAPIProvider(provider.lower())will fail with aValueErrorbecause "anthropic" is not a valid enum value.🔎 Fix provider naming
provider_map = { - "1": "anthropic", + "1": "claude", "2": "openai", "3": "ollama", }
249-251: Strengthen API key validation.The validation on line 249 only checks length (
< 10), which is insufficient. This weak validation could accept invalid keys like "1234567890". Real API keys have specific formats that should be validated.Consider validating known provider formats:
- OpenAI keys typically start with
sk-and are ~51 characters- Anthropic keys start with
sk-ant-Alternatively, perform a minimal test API call to verify the key actually works.
239-256: Critical: API key is collected but never saved.The wizard prompts for and validates the API key (lines 242-253), but the
config_datadictionary (lines 268-279) doesn't include it. Users will still need to manually configure API keys via environment variables, defeating a key purpose of the interactive setup wizard.Per PR objective #4: "Configure and validate API keys if needed" and acceptance criteria: "API keys are validated" and "Configuration is persisted to
~/.cortex/config.yaml".Consider either:
- Saving the API key securely in the config file (with appropriate file permissions 0600)
- Creating a
.envfile with the API key- Providing clear instructions about setting the environment variable
Note: If storing API keys, ensure proper file permissions for security.
263-285: Add error handling, save role, and run verification test.Multiple issues with the configuration saving:
- No error handling for
mkdir()(line 264) or file write operations (lines 281-282) - filesystem errors will crash the wizard- Missing role in config - The collected
rolefromself.roleis not saved to the config file (lines 268-279)- No verification test - Despite PR objective #7 ("Run a quick verification test as part of setup"), no verification is performed
Wrap file operations in try/except blocks, add
"role": self.roletoconfig_data, and implement a verification test before returning success.
932-935: Fix typo in help text.Line 934 contains a typo: "Contex" should be "Cortex".
🔎 Fix typo
# config command config_parser = subparsers.add_parser( - "config", help="Interactive setup wizard for Contex Configuration" + "config", help="Interactive setup wizard for Cortex configuration" )
🧹 Nitpick comments (1)
cortex/cli.py (1)
34-39: Fix misleading comment on line 38.The comment
# Lazy initializationis incorrect. Line 38 performs direct assignment (self.role = role), not lazy initialization. Lazy initialization would involve creating the object only when first accessed (e.g., in a property or getter method).🔎 Suggested fix
self.spinner_chars = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"] self.spinner_idx = 0 - self.prefs_manager = None # Lazy initialization - self.role = role # Lazy initialization + self.prefs_manager = None # Initialized lazily in _get_prefs_manager() + self.role = role self.verbose = verbose self.offline = False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/hardware_detection.py (1)
detect_hardware(646-648)cortex/llm/interpreter.py (1)
CommandInterpreter(17-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Package
🔇 Additional comments (4)
cortex/cli.py (4)
443-443: LGTM: Role parameter correctly propagated.The
CommandInterpreterinstantiation correctly passes theroleparameter fromself.role, enabling role-based system prompts as designed. The parameters align with theCommandInterpreter.__init__signature shown in the relevant code snippets.
918-922: LGTM: Well-defined role argument.The
--roleCLI argument is properly configured with a sensible default and clear help text. The examples (default, security, devops) align with the role YAML definitions mentioned in the AI summary.
1015-1015: LGTM: CLI wiring is correct.The
CortexCLIinstantiation correctly wires bothargs.verboseandargs.rolefrom the parsed command-line arguments, enabling the role-based functionality throughout the application.
1022-1023: LGTM: Config command dispatch.The config command is properly routed to
cli.config(), following the established pattern for other subcommands in the CLI.
pavanimanchala53
force-pushed
the
feature/config-wizard
branch
from
4128b85 to
1530f55
Compare
|
|
I am currently working on this issue and will update this thread as I progress. - effiti |
|
@pavanimanchala53 this is duplicate of |
3 participants
Summary
Adds an interactive
cortex configcommand to guide first-time setup.fixes #247
Features
~/.cortex/config.yamlUsage