[#256] First-run wizard for seamless onboarding #282
Conversation
mikejmorgan-ai
added
enhancement
WalkthroughIntroduces a comprehensive first-run onboarding wizard module for Cortex Linux that executes a multi-step interactive setup flow covering API configuration, hardware detection, preferences, shell integration, and connection testing. Includes state persistence, non-interactive mode support, and auto-execution on first run. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| existing_openai = os.environ.get("OPENAI_API_KEY") | ||
|
|
||
| if existing_claude: | ||
| print(f"✓ Found existing Claude API key: {existing_claude[:8]}...") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, the code should avoid displaying any sensitive information, including any substring of the API key or password. Instead of partially displaying the key, the output should only confirm the existence of a key, e.g., by displaying a generic message such as "Found existing Claude API key" without printing any part of the key itself.
Steps:
- In the
FirstRunWizard._step_api_setupmethod, near line 265, replace the line that printsf"✓ Found existing Claude API key: {existing_claude[:8]}..."with a generic message that avoids displaying the key at all. - The same approach should be checked for the next block (OpenAI API key on line 274), and adapted similarly if it also reveals part of a sensitive key.
No extra methods or imports are necessary; this is purely substituting a message.
| @@ -262,7 +262,7 @@ | ||
| existing_openai = os.environ.get("OPENAI_API_KEY") | ||
|
|
||
| if existing_claude: | ||
| print(f"✓ Found existing Claude API key: {existing_claude[:8]}...") | ||
| print("✓ Found existing Claude API key.") | ||
| self.config["api_provider"] = "anthropic" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( | ||
| @@ -271,7 +271,7 @@ | ||
| ) | ||
|
|
||
| if existing_openai: | ||
| print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...") | ||
| print("✓ Found existing OpenAI API key.") | ||
| self.config["api_provider"] = "openai" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( |
| ) | ||
|
|
||
| if existing_openai: | ||
| print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix, avoid printing any portion of the API key to the console. Instead, indicate that an API key has been found without showing part of its value. For line 274, change the print statement to e.g. "✓ Found existing OpenAI API key."—without interpolation of any part of the key. The same fix should be applied to line 265, which prints the first 8 characters of the Anthropic key—this is the same type of leak, and should be fixed.
This change is local to cortex/first_run_wizard.py, affecting only the print statements in the _step_api_setup method (lines 265 and 274). No additional imports or methods are required.
| @@ -262,7 +262,7 @@ | ||
| existing_openai = os.environ.get("OPENAI_API_KEY") | ||
|
|
||
| if existing_claude: | ||
| print(f"✓ Found existing Claude API key: {existing_claude[:8]}...") | ||
| print("✓ Found existing Claude API key.") | ||
| self.config["api_provider"] = "anthropic" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( | ||
| @@ -271,7 +271,7 @@ | ||
| ) | ||
|
|
||
| if existing_openai: | ||
| print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...") | ||
| print("✓ Found existing OpenAI API key.") | ||
| self.config["api_provider"] = "openai" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( |
|
|
||
| try: | ||
| with open(config_file, 'a') as f: | ||
| f.write(export_line) |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this problem, we should avoid writing API keys directly to the user's shell configuration files in plaintext. Instead, sensitive secrets should be stored in an encrypted file, or (preferably) not stored at all; environment variables should be set only for the current process/session when possible. For CLI onboarding flows, a reasonable pattern is to use the Python keyring library (a well-known secure credential store) to persist secrets securely on the user's system.
Specifically, the _save_env_var method should be modified:
- For API keys (
OPENAI_API_KEY,ANTHROPIC_API_KEY), save the secret using thekeyringPython library (which stores credentials using OS-level secret storage). - Optionally, continue to store non-sensitive envvars in the shell config as before.
- Add a check within
_save_env_varfor common sensitive names and, for those, usekeyring.set_password. - On setup, set the environment variable for the session from the credential store if needed.
- You must import
keyringat the top of the file.
| @@ -18,6 +18,7 @@ | ||
| from pathlib import Path | ||
| from enum import Enum | ||
| import logging | ||
| import keyring | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| @@ -767,19 +768,24 @@ | ||
| return default | ||
|
|
||
| def _save_env_var(self, name: str, value: str): | ||
| """Save environment variable to shell config.""" | ||
| shell = os.environ.get('SHELL', '/bin/bash') | ||
| shell_name = os.path.basename(shell) | ||
| config_file = self._get_shell_config(shell_name) | ||
|
|
||
| export_line = f'\nexport {name}="{value}"\n' | ||
|
|
||
| """Save environment variable securely. For sensitive keys, use keyring.""" | ||
| # List of sensitive env vars to store securely | ||
| SENSITIVE_VARS = {"OPENAI_API_KEY", "ANTHROPIC_API_KEY"} | ||
|
|
||
| try: | ||
| with open(config_file, 'a') as f: | ||
| f.write(export_line) | ||
|
|
||
| # Also set for current session | ||
| os.environ[name] = value | ||
| if name in SENSITIVE_VARS: | ||
| # Store securely using keyring | ||
| keyring.set_password("CortexFirstRunWizard", name, value) | ||
| # Set for current session only | ||
| os.environ[name] = value | ||
| else: | ||
| shell = os.environ.get('SHELL', '/bin/bash') | ||
| shell_name = os.path.basename(shell) | ||
| config_file = self._get_shell_config(shell_name) | ||
| export_line = f'\nexport {name}="{value}"\n' | ||
| with open(config_file, 'a') as f: | ||
| f.write(export_line) | ||
| os.environ[name] = value | ||
| except Exception as e: | ||
| logger.warning(f"Could not save env var: {e}") | ||
|
|
| @@ -1,4 +1,5 @@ | ||
| # Cortex Linux - Core Dependencies | ||
| keyring==25.7.0 | ||
|
|
||
| # LLM Provider APIs | ||
| anthropic>=0.18.0 |
| Package | Version | Security advisories |
| keyring (pypi) | 25.7.0 | None |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import json | ||
| import shutil | ||
| import subprocess | ||
| from typing import Optional, Dict, Any, List, Callable |
| import pytest | ||
| import json | ||
| import os | ||
| from pathlib import Path |
| import json | ||
| import os | ||
| from pathlib import Path | ||
| from datetime import datetime |
| import os | ||
| from pathlib import Path | ||
| from datetime import datetime | ||
| from unittest.mock import Mock, patch, MagicMock |
| if 'model name' in line: | ||
| info['cpu'] = line.split(':')[1].strip() | ||
| break | ||
| except: |
| kb = int(line.split()[1]) | ||
| info['ram_gb'] = round(kb / 1024 / 1024, 1) | ||
| break | ||
| except: |
| info['gpu'] = line.split(':')[-1].strip() | ||
| info['gpu_vendor'] = 'intel' | ||
| break | ||
| except: |
| if len(lines) > 1: | ||
| parts = lines[1].split() | ||
| info['disk_gb'] = int(parts[3].rstrip('G')) | ||
| except: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
docs/FIRST_RUN_WIZARD.md (2)
118-136: Add language specifiers to fenced code blocks.Several code blocks are missing language identifiers for syntax highlighting. This affects readability and accessibility.
Apply these changes:
-``` +```text 1. Go to https://console.anthropic.com 2. Create an API key 3. Enter key in wizardOpenAI:
-+text
- Go to https://platform.openai.com
- Create an API key
- Enter key in wizard
**Ollama (Local):** -``` +```text 1. Install Ollama 2. Pull llama3.2 model 3. No API key needed--- `349-363`: **Add language specifier to architecture diagram.** The ASCII architecture diagram is missing a language identifier. Apply this change: ```diff -``` +```text ┌─────────────────────────────────────────────────────────┐ │ FirstRunWizard │ │ ┌─────────────┐ ┌─────────────┐ ┌─────────────────┐ │tests/test_first_run_wizard.py (2)
344-362: Remove unused lambda parameter.The lambda function has an unused parameter
sthat can be removed for cleaner code.Apply this change:
- mock_open.return_value.__enter__.return_value.__iter__ = lambda s: iter([ + mock_open.return_value.__enter__.return_value.__iter__ = lambda: iter([ "model name : Intel Core i7-9700K\n" ])
430-434: Remove unused fixture parameter.The
tmp_pathparameter is not used in this test method and can be removed.Apply this change:
- def test_needs_first_run(self, tmp_path): + def test_needs_first_run(self): """Test needs_first_run function."""cortex/first_run_wizard.py (1)
160-220: Consider refactoring to reduce complexity.The
run()method has high cognitive complexity (29 vs. recommended 15) due to nested conditionals and state management logic. While functional, this could be improved by extracting helper methods for step execution and skip handling.Consider extracting:
- Step execution logic into
_execute_step(step, handler)- Skip handling into
_process_skip_directive(result, steps)- Error handling into
_handle_step_failure(result)This would improve maintainability and testability while preserving the current behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/first_run_wizard.py(1 hunks)docs/FIRST_RUN_WIZARD.md(1 hunks)tests/test_first_run_wizard.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/first_run_wizard.py (1)
tests/test_first_run_wizard.py (6)
wizard(160-168)wizard(251-258)wizard(339-342)wizard(383-386)wizard(458-464)wizard(512-519)
tests/test_first_run_wizard.py (1)
cortex/first_run_wizard.py (29)
FirstRunWizard(94-784)WizardState(37-81)WizardStep(25-33)StepResult(85-91)needs_first_run(788-790)run_wizard(793-796)get_config(799-805)mark_completed(46-49)mark_skipped(51-54)is_completed(56-58)to_dict(60-69)from_dict(72-81)_ensure_config_dir(118-120)needs_setup(122-124)save_state(138-144)load_state(126-136)save_config(146-152)mark_setup_complete(154-158)_clear_screen(730-733)_print_header(748-752)_prompt(758-767)_step_welcome(221-244)_step_api_setup(246-302)_step_hardware_detection(388-418)_step_preferences(483-520)_step_shell_integration(522-568)_step_test_command(626-679)_step_complete(681-727)run(160-219)
🪛 GitHub Check: CodeQL
cortex/first_run_wizard.py
[failure] 265-265: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
[failure] 274-274: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
[failure] 779-779: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
🪛 GitHub Check: SonarCloud Code Analysis
cortex/first_run_wizard.py
[failure] 464-464: Specify an exception class to catch or reraise the exception
[failure] 478-478: Specify an exception class to catch or reraise the exception
[failure] 416-416: Define a constant instead of duplicating this literal "\nPress Enter to continue: " 3 times.
[failure] 442-442: Specify an exception class to catch or reraise the exception
[failure] 431-431: Specify an exception class to catch or reraise the exception
[failure] 420-420: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
[failure] 160-160: Refactor this function to reduce its Cognitive Complexity from 29 to the 15 allowed.
🪛 Gitleaks (8.30.0)
tests/test_first_run_wizard.py
[high] 266-266: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 522-522: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
docs/FIRST_RUN_WIZARD.md
118-118: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
349-349: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
cortex/first_run_wizard.py
134-134: Do not catch blind exception: Exception
(BLE001)
143-143: Do not catch blind exception: Exception
(BLE001)
151-151: Do not catch blind exception: Exception
(BLE001)
362-362: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
363-363: Starting a process with a partial executable path
(S607)
376-376: Starting a process with a partial executable path
(S607)
431-431: Do not use bare except
(E722)
442-442: Do not use bare except
(E722)
448-448: Starting a process with a partial executable path
(S607)
464-464: Do not use bare except
(E722)
470-470: Starting a process with a partial executable path
(S607)
478-478: Do not use bare except
(E722)
651-651: Starting a process with a partial executable path
(S607)
665-665: Starting a process with a partial executable path
(S607)
671-671: Do not catch blind exception: Exception
(BLE001)
733-733: Starting a process with a shell, possible injection detected
(S605)
765-765: Consider moving this statement to an else block
(TRY300)
783-783: Do not catch blind exception: Exception
(BLE001)
tests/test_first_run_wizard.py
349-349: Unused lambda argument: s
(ARG005)
430-430: Unused method argument: tmp_path
(ARG002)
487-487: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
🔇 Additional comments (19)
tests/test_first_run_wizard.py (7)
25-44: LGTM!Enum tests provide good coverage of expected steps and naming conventions.
46-125: LGTM!State management tests thoroughly validate the dataclass behavior including mutation methods and serialization/deserialization.
127-154: LGTM!StepResult tests validate the dataclass correctly handles success states, data payloads, and skip directives.
156-245: LGTM!Excellent use of fixtures to create isolated test environments with temporary paths. The tests thoroughly validate wizard initialization, state persistence, and configuration management.
247-333: LGTM!Step handler tests provide good coverage with appropriate mocking of external dependencies (environment variables, subprocess calls). The use of test API keys in patches is appropriate for testing.
379-425: LGTM!Shell integration tests validate completion script generation for multiple shells and correct config file path resolution.
454-553: LGTM!Excellent coverage of edge cases (corrupted files, readonly filesystem, user interrupts) and integration scenarios (complete flow, resume from saved state). The
chmod 0o755on line 487 is appropriate cleanup after testing readonly behavior.cortex/first_run_wizard.py (12)
1-23: LGTM!Module structure is clean with appropriate imports and clear documentation linking to the issue.
25-34: LGTM!WizardStep enum provides clear step definitions matching the documented wizard flow.
36-82: LGTM!WizardState dataclass provides robust state tracking with proper mutation methods and serialization/deserialization support.
84-92: LGTM!StepResult dataclass provides a clear interface for step outcomes with support for skip directives.
94-159: LGTM!Class initialization and configuration management are well-structured. The broad exception handling in save/load operations is appropriate for graceful degradation, with proper warning logs for diagnostics.
347-386: Review shell=True subprocess usage for security.Line 362 uses
subprocess.run()withshell=Trueto execute a curl command, which static analysis flags as a potential security risk. While the command string is hardcoded (not user input), this pattern is generally discouraged.Verify whether the shell pipe is necessary, or if this can be refactored to avoid
shell=True:# Option 1: Download script first, then execute import urllib.request script_url = "https://ollama.ai/install.sh" try: with urllib.request.urlopen(script_url) as response: script = response.read() subprocess.run(["sh", "-c", script], check=True) except Exception as e: print(f"\n✗ Failed to install Ollama: {e}") return StepResult(success=True, data={"api_provider": "none"}) # Option 2: Use requests library if available import requests response = requests.get("https://ollama.ai/install.sh") subprocess.run(["sh"], input=response.text, text=True, check=True)However, given this is for initial setup and the URL is hardcoded, the current implementation may be acceptable if documented.
483-520: LGTM!Preferences configuration handles both interactive and non-interactive modes cleanly with sensible defaults.
522-625: LGTM!Shell integration implementation properly detects shell type, generates appropriate completion scripts, and modifies shell configuration files safely. The completion scripts for bash, zsh, and fish are well-structured.
626-679: LGTM!Test command step includes appropriate timeout handling (30s) and graceful fallback to basic apt functionality if the cortex command isn't available.
681-727: LGTM!Completion step provides a comprehensive summary of configuration and helpful usage examples for new users.
730-767: LGTM!Helper methods for UI rendering and user input are well-implemented with appropriate error handling for EOF and KeyboardInterrupt in the
_promptmethod.
787-814: LGTM!Module-level convenience functions provide a clean public API. The main guard properly handles both automatic first-run detection and forced re-runs.
| # Check for existing API keys | ||
| existing_claude = os.environ.get("ANTHROPIC_API_KEY") | ||
| existing_openai = os.environ.get("OPENAI_API_KEY") | ||
|
|
||
| if existing_claude: | ||
| print(f"✓ Found existing Claude API key: {existing_claude[:8]}...") | ||
| self.config["api_provider"] = "anthropic" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( | ||
| success=True, | ||
| data={"api_provider": "anthropic"} | ||
| ) | ||
|
|
||
| if existing_openai: | ||
| print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...") | ||
| self.config["api_provider"] = "openai" | ||
| self.config["api_key_configured"] = True | ||
| return StepResult( | ||
| success=True, | ||
| data={"api_provider": "openai"} | ||
| ) | ||
|
|
There was a problem hiding this comment.
Critical: Remove API key logging.
Lines 265 and 274 log partial API keys to stdout, which can expose sensitive credentials in logs, terminal history, or screen recordings. This violates security best practices for credential handling.
Apply this diff:
if existing_claude:
- print(f"✓ Found existing Claude API key: {existing_claude[:8]}...")
+ print("✓ Found existing Claude API key")
self.config["api_provider"] = "anthropic"
self.config["api_key_configured"] = True
return StepResult(
success=True,
data={"api_provider": "anthropic"}
)
if existing_openai:
- print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...")
+ print("✓ Found existing OpenAI API key")
self.config["api_provider"] = "openai"Based on CodeQL analysis, this is a clear-text logging vulnerability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check for existing API keys | |
| existing_claude = os.environ.get("ANTHROPIC_API_KEY") | |
| existing_openai = os.environ.get("OPENAI_API_KEY") | |
| if existing_claude: | |
| print(f"✓ Found existing Claude API key: {existing_claude[:8]}...") | |
| self.config["api_provider"] = "anthropic" | |
| self.config["api_key_configured"] = True | |
| return StepResult( | |
| success=True, | |
| data={"api_provider": "anthropic"} | |
| ) | |
| if existing_openai: | |
| print(f"✓ Found existing OpenAI API key: {existing_openai[:8]}...") | |
| self.config["api_provider"] = "openai" | |
| self.config["api_key_configured"] = True | |
| return StepResult( | |
| success=True, | |
| data={"api_provider": "openai"} | |
| ) | |
| # Check for existing API keys | |
| existing_claude = os.environ.get("ANTHROPIC_API_KEY") | |
| existing_openai = os.environ.get("OPENAI_API_KEY") | |
| if existing_claude: | |
| print("✓ Found existing Claude API key") | |
| self.config["api_provider"] = "anthropic" | |
| self.config["api_key_configured"] = True | |
| return StepResult( | |
| success=True, | |
| data={"api_provider": "anthropic"} | |
| ) | |
| if existing_openai: | |
| print("✓ Found existing OpenAI API key") | |
| self.config["api_provider"] = "openai" | |
| self.config["api_key_configured"] = True | |
| return StepResult( | |
| success=True, | |
| data={"api_provider": "openai"} | |
| ) |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 265-265: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
[failure] 274-274: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 260-281, remove the partial API key
prints that expose credentials (lines 265 and 274) and replace them with
non-sensitive messages such as "✓ Found existing Claude API key" or "✓ Found
existing OpenAI API key" without including any portion of the key; ensure you do
not log environment variable values anywhere, keep setting
self.config["api_provider"] and self.config["api_key_configured"] as before and
return the same StepResult data (only the provider), and run a quick grep to
verify no other code paths print or log API key values.
| def _setup_claude_api(self) -> StepResult: | ||
| """Set up Claude API.""" | ||
| print("\nTo get a Claude API key:") | ||
| print(" 1. Go to https://console.anthropic.com") | ||
| print(" 2. Sign up or log in") | ||
| print(" 3. Create an API key\n") | ||
|
|
||
| api_key = self._prompt("Enter your Claude API key: ") | ||
|
|
||
| if not api_key or not api_key.startswith("sk-"): | ||
| print("\n⚠ Invalid API key format") | ||
| return StepResult(success=True, data={"api_provider": "none"}) | ||
|
|
||
| # Save to shell profile | ||
| self._save_env_var("ANTHROPIC_API_KEY", api_key) | ||
|
|
||
| self.config["api_provider"] = "anthropic" | ||
| self.config["api_key_configured"] = True | ||
|
|
||
| print("\n✓ Claude API key saved!") | ||
| return StepResult(success=True, data={"api_provider": "anthropic"}) | ||
|
|
||
| def _setup_openai_api(self) -> StepResult: | ||
| """Set up OpenAI API.""" | ||
| print("\nTo get an OpenAI API key:") | ||
| print(" 1. Go to https://platform.openai.com") | ||
| print(" 2. Sign up or log in") | ||
| print(" 3. Create an API key\n") | ||
|
|
||
| api_key = self._prompt("Enter your OpenAI API key: ") | ||
|
|
||
| if not api_key or not api_key.startswith("sk-"): | ||
| print("\n⚠ Invalid API key format") | ||
| return StepResult(success=True, data={"api_provider": "none"}) | ||
|
|
||
| self._save_env_var("OPENAI_API_KEY", api_key) | ||
|
|
||
| self.config["api_provider"] = "openai" | ||
| self.config["api_key_configured"] = True | ||
|
|
||
| print("\n✓ OpenAI API key saved!") | ||
| return StepResult(success=True, data={"api_provider": "openai"}) | ||
|
|
There was a problem hiding this comment.
Strengthen API key validation.
The API key validation only checks if the key starts with "sk-", which is insufficient. This accepts malformed or test keys that will fail later during actual API calls.
Consider adding:
- Minimum length validation (Anthropic and OpenAI keys are typically 40+ characters)
- Format validation (alphanumeric plus specific allowed characters)
- Optional: Test API call to validate key before saving
Example enhancement:
def _setup_claude_api(self) -> StepResult:
"""Set up Claude API."""
print("\nTo get a Claude API key:")
print(" 1. Go to https://console.anthropic.com")
print(" 2. Sign up or log in")
print(" 3. Create an API key\n")
api_key = self._prompt("Enter your Claude API key: ")
- if not api_key or not api_key.startswith("sk-"):
+ if not api_key or not api_key.startswith("sk-") or len(api_key) < 20:
print("\n⚠ Invalid API key format")
return StepResult(success=True, data={"api_provider": "none"})Similar changes should be applied to _setup_openai_api (lines 326-346).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 304-346, strengthen API key
validation in both _setup_claude_api and _setup_openai_api: keep the existing
prefix check but also enforce a minimum length (e.g., >=40), validate allowed
characters with a regex (alphanumeric plus hyphen/underscore if needed), and
fail early if the pattern doesn't match; optionally perform a lightweight test
API call to verify the key before saving and only call _save_env_var and set
config flags when the key passes validation/test; preserve the user messages but
update the invalid-key message to be more specific when length/format checks
fail.
| def _detect_hardware(self) -> Dict[str, Any]: | ||
| """Detect system hardware.""" | ||
| info = {} | ||
|
|
||
| # CPU | ||
| try: | ||
| with open('/proc/cpuinfo') as f: | ||
| for line in f: | ||
| if 'model name' in line: | ||
| info['cpu'] = line.split(':')[1].strip() | ||
| break | ||
| except: | ||
| info['cpu'] = 'Unknown' | ||
|
|
||
| # RAM | ||
| try: | ||
| with open('/proc/meminfo') as f: | ||
| for line in f: | ||
| if 'MemTotal' in line: | ||
| kb = int(line.split()[1]) | ||
| info['ram_gb'] = round(kb / 1024 / 1024, 1) | ||
| break | ||
| except: | ||
| info['ram_gb'] = 0 | ||
|
|
||
| # GPU | ||
| try: | ||
| result = subprocess.run( | ||
| ['lspci'], | ||
| capture_output=True, | ||
| text=True | ||
| ) | ||
| for line in result.stdout.split('\n'): | ||
| if 'VGA' in line or '3D' in line: | ||
| if 'NVIDIA' in line.upper(): | ||
| info['gpu'] = line.split(':')[-1].strip() | ||
| info['gpu_vendor'] = 'nvidia' | ||
| elif 'AMD' in line.upper(): | ||
| info['gpu'] = line.split(':')[-1].strip() | ||
| info['gpu_vendor'] = 'amd' | ||
| elif 'Intel' in line.upper(): | ||
| info['gpu'] = line.split(':')[-1].strip() | ||
| info['gpu_vendor'] = 'intel' | ||
| break | ||
| except: | ||
| info['gpu'] = 'None detected' | ||
|
|
||
| # Disk | ||
| try: | ||
| result = subprocess.run( | ||
| ['df', '-BG', '/'], | ||
| capture_output=True, | ||
| text=True | ||
| ) | ||
| lines = result.stdout.strip().split('\n') | ||
| if len(lines) > 1: | ||
| parts = lines[1].split() | ||
| info['disk_gb'] = int(parts[3].rstrip('G')) | ||
| except: | ||
| info['disk_gb'] = 0 | ||
|
|
||
| return info |
There was a problem hiding this comment.
Replace bare except blocks with specific exception handling.
Lines 431, 442, 464, and 478 use bare except: clauses which catch all exceptions including KeyboardInterrupt and SystemExit. This can make the wizard impossible to interrupt and hides programming errors.
Apply this diff:
# CPU
try:
with open('/proc/cpuinfo') as f:
for line in f:
if 'model name' in line:
info['cpu'] = line.split(':')[1].strip()
break
- except:
+ except (IOError, OSError) as e:
+ logger.debug(f"Could not read CPU info: {e}")
info['cpu'] = 'Unknown'
# RAM
try:
with open('/proc/meminfo') as f:
for line in f:
if 'MemTotal' in line:
kb = int(line.split()[1])
info['ram_gb'] = round(kb / 1024 / 1024, 1)
break
- except:
+ except (IOError, OSError, ValueError, IndexError) as e:
+ logger.debug(f"Could not read RAM info: {e}")
info['ram_gb'] = 0
# GPU
try:
result = subprocess.run(
['lspci'],
capture_output=True,
text=True
)
for line in result.stdout.split('\n'):
if 'VGA' in line or '3D' in line:
if 'NVIDIA' in line.upper():
info['gpu'] = line.split(':')[-1].strip()
info['gpu_vendor'] = 'nvidia'
elif 'AMD' in line.upper():
info['gpu'] = line.split(':')[-1].strip()
info['gpu_vendor'] = 'amd'
elif 'Intel' in line.upper():
info['gpu'] = line.split(':')[-1].strip()
info['gpu_vendor'] = 'intel'
break
- except:
+ except (subprocess.SubprocessError, OSError) as e:
+ logger.debug(f"Could not detect GPU: {e}")
info['gpu'] = 'None detected'
# Disk
try:
result = subprocess.run(
['df', '-BG', '/'],
capture_output=True,
text=True
)
lines = result.stdout.strip().split('\n')
if len(lines) > 1:
parts = lines[1].split()
info['disk_gb'] = int(parts[3].rstrip('G'))
- except:
+ except (subprocess.SubprocessError, OSError, ValueError, IndexError) as e:
+ logger.debug(f"Could not detect disk space: {e}")
info['disk_gb'] = 0This allows graceful degradation while preserving the ability to interrupt the program and providing better diagnostics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _detect_hardware(self) -> Dict[str, Any]: | |
| """Detect system hardware.""" | |
| info = {} | |
| # CPU | |
| try: | |
| with open('/proc/cpuinfo') as f: | |
| for line in f: | |
| if 'model name' in line: | |
| info['cpu'] = line.split(':')[1].strip() | |
| break | |
| except: | |
| info['cpu'] = 'Unknown' | |
| # RAM | |
| try: | |
| with open('/proc/meminfo') as f: | |
| for line in f: | |
| if 'MemTotal' in line: | |
| kb = int(line.split()[1]) | |
| info['ram_gb'] = round(kb / 1024 / 1024, 1) | |
| break | |
| except: | |
| info['ram_gb'] = 0 | |
| # GPU | |
| try: | |
| result = subprocess.run( | |
| ['lspci'], | |
| capture_output=True, | |
| text=True | |
| ) | |
| for line in result.stdout.split('\n'): | |
| if 'VGA' in line or '3D' in line: | |
| if 'NVIDIA' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'nvidia' | |
| elif 'AMD' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'amd' | |
| elif 'Intel' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'intel' | |
| break | |
| except: | |
| info['gpu'] = 'None detected' | |
| # Disk | |
| try: | |
| result = subprocess.run( | |
| ['df', '-BG', '/'], | |
| capture_output=True, | |
| text=True | |
| ) | |
| lines = result.stdout.strip().split('\n') | |
| if len(lines) > 1: | |
| parts = lines[1].split() | |
| info['disk_gb'] = int(parts[3].rstrip('G')) | |
| except: | |
| info['disk_gb'] = 0 | |
| return info | |
| def _detect_hardware(self) -> Dict[str, Any]: | |
| """Detect system hardware.""" | |
| info = {} | |
| # CPU | |
| try: | |
| with open('/proc/cpuinfo') as f: | |
| for line in f: | |
| if 'model name' in line: | |
| info['cpu'] = line.split(':')[1].strip() | |
| break | |
| except (IOError, OSError): | |
| info['cpu'] = 'Unknown' | |
| # RAM | |
| try: | |
| with open('/proc/meminfo') as f: | |
| for line in f: | |
| if 'MemTotal' in line: | |
| kb = int(line.split()[1]) | |
| info['ram_gb'] = round(kb / 1024 / 1024, 1) | |
| break | |
| except (IOError, OSError, ValueError, IndexError): | |
| info['ram_gb'] = 0 | |
| # GPU | |
| try: | |
| result = subprocess.run( | |
| ['lspci'], | |
| capture_output=True, | |
| text=True | |
| ) | |
| for line in result.stdout.split('\n'): | |
| if 'VGA' in line or '3D' in line: | |
| if 'NVIDIA' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'nvidia' | |
| elif 'AMD' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'amd' | |
| elif 'Intel' in line.upper(): | |
| info['gpu'] = line.split(':')[-1].strip() | |
| info['gpu_vendor'] = 'intel' | |
| break | |
| except (subprocess.SubprocessError, OSError): | |
| info['gpu'] = 'None detected' | |
| # Disk | |
| try: | |
| result = subprocess.run( | |
| ['df', '-BG', '/'], | |
| capture_output=True, | |
| text=True | |
| ) | |
| lines = result.stdout.strip().split('\n') | |
| if len(lines) > 1: | |
| parts = lines[1].split() | |
| info['disk_gb'] = int(parts[3].rstrip('G')) | |
| except (subprocess.SubprocessError, OSError, ValueError, IndexError): | |
| info['disk_gb'] = 0 | |
| return info |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 464-464: Specify an exception class to catch or reraise the exception
[failure] 478-478: Specify an exception class to catch or reraise the exception
[failure] 442-442: Specify an exception class to catch or reraise the exception
[failure] 431-431: Specify an exception class to catch or reraise the exception
[failure] 420-420: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
🪛 Ruff (0.14.8)
431-431: Do not use bare except
(E722)
442-442: Do not use bare except
(E722)
448-448: Starting a process with a partial executable path
(S607)
464-464: Do not use bare except
(E722)
470-470: Starting a process with a partial executable path
(S607)
478-478: Do not use bare except
(E722)
| def _save_env_var(self, name: str, value: str): | ||
| """Save environment variable to shell config.""" | ||
| shell = os.environ.get('SHELL', '/bin/bash') | ||
| shell_name = os.path.basename(shell) | ||
| config_file = self._get_shell_config(shell_name) | ||
|
|
||
| export_line = f'\nexport {name}="{value}"\n' | ||
|
|
||
| try: | ||
| with open(config_file, 'a') as f: | ||
| f.write(export_line) | ||
|
|
||
| # Also set for current session | ||
| os.environ[name] = value | ||
| except Exception as e: | ||
| logger.warning(f"Could not save env var: {e}") |
There was a problem hiding this comment.
Critical: Clear-text storage of API keys in shell configuration.
Line 779 writes API keys directly to shell configuration files (e.g., ~/.bashrc) in clear text. This creates several security risks:
- Credential exposure: Anyone with file system access can read the keys
- Version control leaks: Shell configs are sometimes committed to dotfile repos
- Log exposure: Shell configs may be included in bug reports or system logs
- No encryption: Keys are stored without any protection
Recommended solutions:
Option 1: Use system keyring (most secure)
# Add dependency: keyring
import keyring
def _save_env_var(self, name: str, value: str):
"""Save API key to system keyring."""
try:
keyring.set_password("cortex", name, value)
print(f"✓ {name} saved securely to system keyring")
# Add retrieval instructions to shell config
shell_name = os.path.basename(os.environ.get('SHELL', '/bin/bash'))
config_file = self._get_shell_config(shell_name)
# Add a helper to load from keyring
helper_line = f'''
# Cortex: Load API key from keyring
if command -v python3 &> /dev/null; then
export {name}="$(python3 -c 'import keyring; print(keyring.get_password("cortex", "{name}"))')"
fi
'''
with open(config_file, 'a') as f:
f.write(helper_line)
os.environ[name] = value
except Exception as e:
logger.warning(f"Could not save to keyring: {e}")Option 2: Use encrypted config file
# Store in encrypted config with user password/key
from cryptography.fernet import Fernet
def _save_api_key_encrypted(self, name: str, value: str):
"""Save API key to encrypted config."""
# Implementation would use Fernet or similar to encrypt the config file
passOption 3: Document manual secure storage
If automated secure storage is out of scope, at minimum:
- Document the security implications in the wizard output
- Suggest users manually configure secure storage
- Add a warning that keys are stored in plain text
Based on CodeQL analysis, this is a clear-text storage vulnerability that should be addressed before production use.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 779-779: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
🪛 Ruff (0.14.8)
783-783: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 769-784, the method currently appends
API keys in clear text to the user shell config; replace that behavior by
storing secrets in the system keyring and only adding a non-sensitive helper to
the shell (or prompting the user) instead of writing the secret itself.
Concretely: add keyring as a dependency and call keyring.set_password("cortex",
name, value) to persist the secret, set os.environ[name]=value for the current
session, and append to the shell config a small helper snippet that uses a
python keyring.get_password("cortex", name) invocation to export the variable at
login (do not write the secret value itself). Also handle errors and fall back
to printing a clear, explicit instruction/warning to the user if the system
keyring is unavailable, rather than storing the key in plain text.
|
Implements #256 with complete tests and docs. Closes #256
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.