fix(wizard): allow setup to continue when API key is already present in ENV #356
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces provider-aware API key management and a comprehensive first-run wizard for interactive setup and reconfiguration. Core enhancements include provider-scoped key retrieval in the CLI, multi-provider installation flow with fallback logic, a refactored first-run wizard with environment persistence, new API key validation utilities, and expanded environment loader search paths. Test coverage is substantially extended across multiple modules, and demo files are updated with new async/parallel testing infrastructure. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as CortexCLI
participant Detector as api_key_detector
participant Wizard as FirstRunWizard
participant Storage as Env/Storage
User->>CLI: install(software, forced_provider=None)
CLI->>CLI: Detect initial_provider
CLI->>Detector: detect() for available providers
Detector-->>CLI: provider list + keys
CLI->>CLI: Build providers_to_try (primary + secondary if both have keys)
loop For each provider in providers_to_try
CLI->>CLI: _get_api_key_for_provider(provider)
CLI->>Detector: detect_api_key(provider)
Detector-->>CLI: api_key or None
alt Key Found
CLI->>CLI: Build CommandInterpreter with key
CLI->>CLI: Execute install command
CLI-->>User: Success
else No Key
CLI->>CLI: Try next provider
end
end
alt No providers succeeded
CLI-->>User: Error: No valid provider found
end
sequenceDiagram
participant User
participant CLI as CortexCLI
participant Wizard as FirstRunWizard
participant Validator as api_key_validator
participant Storage as Env/Storage
User->>CLI: wizard()
CLI->>Wizard: FirstRunWizard(interactive=True)
Wizard->>Wizard: Show welcome banner
Wizard->>Wizard: detect_available_providers()
Wizard->>User: Present provider options (Anthropic, OpenAI, Ollama)
User->>Wizard: Select provider
Wizard->>Storage: read_key_from_env_file()
alt Existing key found
Wizard->>Validator: validate_anthropic_api_key() or validate_openai_api_key()
Validator-->>Wizard: Valid/Invalid
Wizard->>User: Confirm use or replace
else No existing key
Wizard->>User: Prompt for API key
User->>Wizard: Enter key
end
Wizard->>Storage: save_key_to_env_file() or shell config or encrypted storage
Wizard->>Wizard: Run dry-run verification
Wizard-->>User: Setup complete / Success
Wizard-->>CLI: Return 0
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
jaysurse
force-pushed
the
fix/cortex-wizard-not-running
branch
from
71f1fe9 to
53a9e46
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/first_run_wizard.py (1)
329-329: Fix Anthropic API key format validation at line 329.The validation currently checks only for
startswith("sk-"), but Anthropic API keys specifically use the format"sk-ant-api03-". The current validation is too permissive and would incorrectly accept OpenAI keys (which also start with"sk-") as valid Claude keys. Update the validation at line 329 to check for the correct Anthropic prefix.
🧹 Nitpick comments (2)
cortex/first_run_wizard.py (1)
262-268: Consider validating existing API keys before using them.The code detects existing API keys in the environment but doesn't validate that they're actually valid or properly formatted. Invalid or expired keys would cause runtime failures later when attempting API calls.
💡 Suggested validation approach
You could add a helper method to validate keys before accepting them:
def _validate_existing_key(self, key: str, provider: str) -> bool: """Validate that an existing API key has the correct format.""" if not key: return False # Basic format checks if provider == "anthropic" and not key.startswith("sk-ant-"): return False if provider == "openai" and not key.startswith("sk-"): return False return TrueThen use it when checking:
# Check for existing API keys existing_claude = os.environ.get("ANTHROPIC_API_KEY") +existing_claude = existing_claude if self._validate_existing_key(existing_claude, "anthropic") else None existing_openai = os.environ.get("OPENAI_API_KEY") +existing_openai = existing_openai if self._validate_existing_key(existing_openai, "openai") else Nonecortex/cli.py (1)
773-773: Minor unrelated change: notify command added to help.This line adds the notify command to the help table, which is unrelated to the main PR objective of fixing the wizard's API key detection. While the change itself is fine, it's generally better practice to keep PRs focused on a single objective.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/first_run_wizard.py
🧰 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.pycortex/first_run_wizard.py
⏰ 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)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (6)
cortex/first_run_wizard.py (4)
271-278: LGTM! Clear visual feedback for existing keys.The status indicators provide clear feedback to users about which API keys are already configured, improving the user experience.
281-295: Non-interactive auto-selection logic is correct.The logic appropriately auto-selects the provider when an API key is detected in non-interactive mode, which aligns with the PR objectives. The same validation concern mentioned earlier applies here as well.
301-305: LGTM! Streamlined flow for existing Claude keys.The code correctly skips the API key prompt when a key is already present in the environment, improving the user experience as intended by the PR.
308-312: LGTM! Consistent handling for existing OpenAI keys.The implementation mirrors the Claude key handling and correctly uses existing keys from the environment.
cortex/cli.py (2)
12-12: LGTM! Clean import.The import statement is properly placed and follows Python conventions.
745-748: LGTM! Clean integration with FirstRunWizard.The wizard command properly instantiates and runs the FirstRunWizard, which now includes the enhanced API key detection logic. The implementation is straightforward and returns appropriate exit codes.
|
working on it - effiti |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/env_loader.py (1)
18-23: Remove duplicate imports.Lines 22-23 duplicate the imports from lines 18-19, causing redefinition errors flagged by Ruff (F811) and breaking import formatting (I001).
🔎 Proposed fix
import os from pathlib import Path -import os -from pathlib import Path - def get_env_file_locations() -> list[Path]:
🧹 Nitpick comments (4)
cortex/first_run_wizard.py (4)
17-32: Fix import organization.The import block is flagged as unsorted/unformatted by Ruff (I001). Organize imports per PEP 8: standard library, then third-party, then local imports, each group alphabetically sorted.
🔎 Example organization
# Standard library import json import logging import os import random import shutil import subprocess import sys from dataclasses import dataclass, field from datetime import datetime from enum import Enum from pathlib import Path from typing import Any # Local imports from cortex.utils.api_key_test import test_anthropic_api_key, test_openai_api_key
325-349: Consider adding retry limit to prevent infinite loops.The
whileloop at line 329 will continue indefinitely if the user repeatedly enters invalid keys. Consider adding a retry limit to prevent frustration.🔎 Proposed improvement
# Validate and prompt for key lazily if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + max_attempts = 3 + attempts = 0 - while not key or not key.startswith("sk-ant-"): + while (not key or not key.startswith("sk-ant-")) and attempts < max_attempts: + attempts += 1 print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") if key and key.startswith("sk-ant-"): self._save_env_var("ANTHROPIC_API_KEY", key) os.environ["ANTHROPIC_API_KEY"] = key + if not key or not key.startswith("sk-ant-"): + print("\n❌ Failed to configure valid API key after multiple attempts.") + return FalseApply similar logic to the OpenAI validation loop at line 354.
465-465: Remove redundant import.The
test_anthropic_api_keyfunction is already imported at the module level (line 32). This inline import on line 465 is unnecessary.🔎 Proposed fix
if self.interactive: do_test = self._prompt("Would you like to test your Claude API key now? [Y/n]: ", default="y") if do_test.strip().lower() in ("", "y", "yes"): print("\nTesting Claude API key...") - from cortex.utils.api_key_test import test_anthropic_api_key if test_anthropic_api_key(existing_claude):Apply the same fix for line 485 (test_openai_api_key).
765-765: Remove trailing whitespace.Line 765 contains trailing whitespace, flagged by Ruff (W293).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/env_loader.pycortex/first_run_wizard.pycortex/utils/api_key_test.py
🧰 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/utils/api_key_test.pycortex/cli.pycortex/first_run_wizard.pycortex/env_loader.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/first_run_wizard.py (2)
needs_setup(246-248)run(284-385)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(4-28)test_openai_api_key(30-52)
🪛 GitHub Actions: CI
cortex/env_loader.py
[error] 18-18: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: lint
cortex/utils/api_key_test.py
[failure] 1-2: Ruff (I001)
cortex/utils/api_key_test.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/first_run_wizard.py
[failure] 17-32: Ruff (I001)
cortex/first_run_wizard.py:17:1: I001 Import block is un-sorted or un-formatted
[failure] 2-3: Ruff (I001)
cortex/first_run_wizard.py:2:5: I001 Import block is un-sorted or un-formatted
[failure] 765-765: Ruff (W293)
cortex/first_run_wizard.py:765:1: W293 Blank line contains whitespace
cortex/env_loader.py
[failure] 23-23: Ruff (F811)
cortex/env_loader.py:23:21: F811 Redefinition of unused Path from line 19
[failure] 22-22: Ruff (F811)
cortex/env_loader.py:22:8: F811 Redefinition of unused os from line 18
🪛 GitHub Check: Lint
cortex/utils/api_key_test.py
[failure] 1-2: Ruff (I001)
cortex/utils/api_key_test.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/first_run_wizard.py
[failure] 17-32: Ruff (I001)
cortex/first_run_wizard.py:17:1: I001 Import block is un-sorted or un-formatted
[failure] 2-3: Ruff (I001)
cortex/first_run_wizard.py:2:5: I001 Import block is un-sorted or un-formatted
[failure] 765-765: Ruff (W293)
cortex/first_run_wizard.py:765:1: W293 Blank line contains whitespace
cortex/env_loader.py
[failure] 23-23: Ruff (F811)
cortex/env_loader.py:23:21: F811 Redefinition of unused Path from line 19
[failure] 22-22: Ruff (F811)
cortex/env_loader.py:22:8: F811 Redefinition of unused os from line 18
⏰ 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (10)
cortex/env_loader.py (1)
44-50: LGTM: Graceful handling of missing cortex package.The try/except block appropriately handles the case where the cortex package may not be importable, allowing the env loader to work in various installation contexts.
cortex/cli.py (6)
63-78: LGTM: Well-designed fallback logic.The refactored API key retrieval appropriately handles multiple scenarios: provider-specific keys, completed setup without keys (Ollama fallback), and unconfigured systems (helpful guidance). This aligns well with the PR objective to improve the wizard experience.
312-318: LGTM: Appropriate parameter addition.The
forced_providerparameter enables the wizard to test specific providers during onboarding, supporting the PR's goal of improving setup flow.
339-384: LGTM: Solid multi-provider fallback implementation.The refactored install flow appropriately tries multiple providers with graceful fallback. The debug logging helps troubleshoot provider selection issues, and the error handling correctly continues to the next provider on failure.
770-777: LGTM: Clean wizard integration.The wizard method properly integrates with
FirstRunWizardand returns appropriate exit codes.
802-802: LGTM: Help documentation updated.The notify command is appropriately added to the help table.
47-61: Add comprehensive docstring.Per coding guidelines, public methods require complete docstrings with Args and Returns sections.
🔎 Proposed docstring
def _get_api_key_for_provider(self, provider: str) -> str | None: - """Get API key for a specific provider.""" + """Get API key for a specific provider. + + Args: + provider: Provider name ('ollama', 'fake', 'claude', 'openai'). + + Returns: + API key string if found and valid, None otherwise. + Returns 'ollama-local' for ollama and 'fake-key' for fake provider. + """Based on coding guidelines.
Likely an incorrect or invalid review comment.
cortex/first_run_wizard.py (2)
214-217: Add docstring.Per coding guidelines, all methods should have docstrings describing their purpose.
🔎 Proposed addition
def _setup_ollama(self) -> StepResult: + """Configure Ollama as the AI provider (no API key required). + + Returns: + StepResult indicating success with provider set to 'ollama'. + """ print("\nOllama selected. No API key required.")Based on coding guidelines.
Likely an incorrect or invalid review comment.
142-161: Add docstring and consider making package list configurable.This method lacks a docstring entirely, violating coding guidelines. Additionally, the hardcoded package list may not suit all use cases.
🔎 Proposed improvements
- def _install_suggested_packages(self): - """Offer to install suggested packages and run the install if user agrees.""" + def _install_suggested_packages(self, suggestions: list[str] | None = None): + """Offer to install suggested packages and run the install if user agrees. + + Args: + suggestions: List of package names to suggest. Defaults to basic packages. + """ + if suggestions is None: + suggestions = ["python", "numpy", "requests"] - suggestions = ["python", "numpy", "requests"]Based on coding guidelines.
Likely an incorrect or invalid review comment.
cortex/utils/api_key_test.py (1)
38-38: gpt-3.5-turbo remains valid for API tests.Confirmed: "gpt-3.5-turbo" is still a valid and functional model in the OpenAI API as of December 2025. The base model remains available despite some deprecated variants. Tests using this model will continue to work.
cortex/utils/api_key_test.py
Outdated
| # Minimal harmless request (model name may need to be updated) | ||
| data = { | ||
| "model": "claude-3-opus-20240229", # or another available model |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
current valid model names Anthropic Claude API December 2024 2025
💡 Result:
Here are concise lists of Claude API model names documented by Anthropic for the two dates you asked.
Models available/active around December 2024 (per Anthropic docs in late‑2024):
- claude-3-haiku-20240307.
- claude-3-5-haiku-20241022.
(Several earlier v1/v1.x and claude-instant variants were already retired by late 2024.) [1][2]
Models available/active by December 2025 (current Anthropic model lineup includes):
- claude-sonnet-4-20250514.
- claude-opus-4-20250514.
- claude-opus-4-1-20250805.
- (newer 4.x releases such as claude-sonnet-4-5 and claude-opus-4-5 are also listed by Anthropic/news sources with API names like claude-sonnet-4-5-20250929 and claude-opus-4-5-20251101). [3][4]
Sources: Anthropic model docs & deprecation pages and recent Anthropic announcements.
- [1] Anthropic model deprecations / status.
- [2] Anthropic models overview (models list and API IDs).
- [3] Anthropic API "List Models" / Get Model pages.
- [4] Anthropic announcements / press coverage for Sonnet/Opus 4.x (Nov–2025 releases).
If you want, I can fetch the live /v1/models output for your API key to show exactly which model IDs your account can access right now.
Replace hardcoded model name with a currently available Anthropic Claude API model.
The model "claude-3-opus-20240229" is no longer available in the Anthropic API as of December 2025. Current available models include claude-sonnet-4-20250514, claude-opus-4-20250514, and newer releases like claude-opus-4-5-20251101. Update this test to use a valid, currently supported model to avoid API errors when the test runs.
🤖 Prompt for AI Agents
In cortex/utils/api_key_test.py around lines 12 to 14, the test hardcodes the
deprecated model "claude-3-opus-20240229"; replace that string with a currently
supported Anthropic model such as "claude-opus-4-20250514" (or
"claude-sonnet-4-20250514" / "claude-opus-4-5-20251101") so the request
succeeds, or better yet read the model name from a test constant or env var and
default it to "claude-opus-4-20250514".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
388-397: PotentialUnboundLocalErrorforinstall_idin exception handlers.
install_idis initialized toNoneat line 389, but this initialization is inside thetryblock. If an exception occurs before line 389 executes (e.g., at line 391 during_extract_packages_from_commands), theexcepthandlers at lines 543-557 will referenceinstall_idwhich would raiseUnboundLocalError.Move the
install_id = Noneinitialization before thetryblock to ensure it's always defined.🔎 Proposed fix
if not commands: self._print_error( "No commands generated with any available provider. Please try again with a different request." ) return 1 + install_id = None try: - install_id = None # Extract packages from commands for tracking
♻️ Duplicate comments (6)
cortex/utils/api_key_test.py (3)
1-3: Remove unusedosimport.The
osmodule is imported but never used in this file.
6-25: Update deprecated model and improve exception handling.The previous review correctly identified:
- The model
claude-3-opus-20240229is deprecated- The bare
except Exceptionis overly broad- The docstring is incomplete per coding guidelines
These issues remain unaddressed.
28-42: Improve docstring and exception handling.The previous review correctly identified:
- The bare
except Exceptionis overly broad- The docstring is incomplete per coding guidelines
These issues remain unaddressed.
cortex/first_run_wizard.py (3)
1-10: Module-level environment loading withoverride=Trueintroduces side effects.This was flagged in a previous review. The module-level
.envloading withoverride=Trueconflicts with the centralizedenv_loader.load_env()approach incli.pyand can cause unexpected behavior during testing and import.
50-63: Enhance docstring with return value details.Per coding guidelines, this public function should document its return value format.
172-200: Fix Claude API key validation inconsistency.Line 178 validates that the API key starts with
"sk-", butdetect_available_providers()(line 55) specifically checks for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation.
🧹 Nitpick comments (4)
cortex/cli.py (2)
47-61: Enhance docstring per coding guidelines.Per coding guidelines, public API functions require complete docstrings with Args and Returns sections.
🔎 Proposed improvement
def _get_api_key_for_provider(self, provider: str) -> str | None: - """Get API key for a specific provider.""" + """Get API key for a specific provider. + + Args: + provider: The provider name ('ollama', 'fake', 'claude', or 'openai'). + + Returns: + The API key string if found and valid, None otherwise. + """
53-60: Potential prefix overlap between Claude and OpenAI key validation.The OpenAI check (line 59) uses
startswith("sk-")which would also match Anthropic keys that start with"sk-ant-". While theelifstructure prevents this from being an issue whenprovider == "claude"is checked first, consider using a more specific prefix like"sk-proj-"or excluding"sk-ant-"for clarity and future-proofing.🔎 Proposed fix for clearer validation
elif provider == "openai": key = os.environ.get("OPENAI_API_KEY") - if key and key.strip().startswith("sk-"): + if key and key.strip().startswith("sk-") and not key.strip().startswith("sk-ant-"): return key.strip()cortex/first_run_wizard.py (2)
148-170: Add docstring with Args/Returns per coding guidelines.The
_install_suggested_packagesmethod has a brief docstring but is missing Args and Returns sections. Since this method has no parameters (other than self) and no return value, at minimum clarify that it returnsNone.🔎 Proposed improvement
def _install_suggested_packages(self): - """Offer to install suggested packages and run the install if user agrees.""" + """Offer to install suggested packages and run the install if user agrees. + + Prompts the user to install sample packages (python, numpy, requests) + to verify Cortex is working correctly. Executes installations via + subprocess if the user agrees. + """
354-382: Anthropic key validation correctly uses"sk-ant-"prefix.The lazy validation loop for Anthropic keys (lines 354-358) correctly checks for the
"sk-ant-"prefix, which is consistent withdetect_available_providers(). This is good.However, if the user declines the dry-run test (line 365), the code proceeds silently without any confirmation message. Consider adding a brief acknowledgment like "Skipping dry run test."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/env_loader.pycortex/first_run_wizard.pycortex/utils/api_key_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/env_loader.py
🧰 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/utils/api_key_test.pycortex/first_run_wizard.pycortex/cli.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 1-1: test_step_hardware_detection: AttributeError: FirstRunWizard object has no attribute '_step_hardware_detection'.
[error] 1-1: test_detect_hardware_full/test_detect_nvidia_gpu: AttributeError: FirstRunWizard lacks '_detect_hardware' method.
cortex/cli.py
[error] 370-370: test_install_unexpected_error: Unexpected exception during install command execution. Unexpected
[error] 370-370: test_install_value_error: ValueError: Invalid input during install command parsing.
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (2)
cortex/cli.py (1)
772-779: Wizard integration looks correct.The wizard command now properly instantiates
FirstRunWizardwithinteractive=Trueand returns the appropriate exit code based on success.cortex/first_run_wizard.py (1)
416-426: Ollama branch and completion message look good.The Ollama handling (lines 416-417) and the final success message (lines 423-425) are appropriate. The provider is saved to config and the user is informed of successful setup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/cli.py (3)
47-61: Consider enhancing the docstring with parameter and return details.The docstring is minimal. While this is a private method, adding parameter descriptions and return value details would improve maintainability.
🔎 Suggested docstring enhancement
- def _get_api_key_for_provider(self, provider: str) -> str | None: - """Get API key for a specific provider.""" + def _get_api_key_for_provider(self, provider: str) -> str | None: + """Get API key for a specific provider. + + Args: + provider: The provider name (ollama, fake, claude, openai) + + Returns: + API key string if found and valid, None otherwise + """
63-78: Consider adding a user-visible warning for the Ollama fallback.The fallback to Ollama (line 74) when setup is complete but no valid API key is found only logs in verbose mode. Users running in normal mode won't see any indication that Cortex has switched providers, which could lead to confusion about which provider is being used.
💡 Suggested improvement
Consider adding a user-visible message before the fallback:
if not wizard.needs_setup(): # Setup complete, but no valid key - use Ollama as fallback self._debug("Setup complete but no valid API key; falling back to Ollama") + cx_print("No API key found; using Ollama (local) provider", "info") return "ollama-local"
339-386: Multi-provider fallback logic is well-implemented, but exception handling can be simplified.The provider loop successfully addresses the past review concern by catching
Exception(line 378), which includesValueErrorandRuntimeError. However, specifying bothRuntimeErrorandExceptionis redundant sinceRuntimeErroris a subclass ofException.🔎 Simplification suggestion
- except (RuntimeError, Exception) as e: + except Exception as e: self._debug(f"API call failed with {try_provider}: {e}") continueThis catches all exception types (including
ValueError,RuntimeError, and any otherException) as intended, while being more concise.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pytests/installer/test_parallel_install.py
✅ Files skipped from review due to trivial changes (1)
- tests/installer/test_parallel_install.py
🧰 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 (1)
cortex/first_run_wizard.py (3)
FirstRunWizard(147-862)needs_setup(264-266)run(302-426)
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
cortex/cli.py (2)
318-318: Good addition of explicit provider control.The
forced_providerparameter enables explicit provider selection for testing and user control, which aligns well with the multi-provider fallback logic introduced in the install flow.
776-779: Excellent fix - wizard now properly continues when API key is present.This change addresses the core issue from #355. The wizard now runs the complete
FirstRunWizard.run()flow, which detects available providers (including those with API keys already in ENV), allows provider confirmation/selection, and continues the setup process rather than exiting prematurely. This matches the expected behavior described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_cli_extended.py (1)
86-101: Critical: Tests fail due to missing mock for_get_api_key_for_provider.The pipeline failures indicate that
test_install_dry_run,test_install_no_execute, andtest_install_with_execute_success(at lines 100, 122, 153) are failing with "No commands generated with any available provider."The root cause is that the new multi-provider installation flow in
cortex/cli.py(lines 339-387) calls_get_api_key_for_provider(try_provider)directly instead of_get_api_key(). The existing mocks for_get_api_keyand_get_providerdon't cover this new method, causing the provider loop to find no valid keys and return an error.🔎 Proposed fix
Add a mock for
_get_api_key_for_provideror use environment setup that works with the new logic:@patch.object(CortexCLI, "_get_provider", return_value="openai") @patch.object(CortexCLI, "_get_api_key", return_value="sk-test-key") + @patch.object(CortexCLI, "_get_api_key_for_provider", return_value="sk-test-key") @patch.object(CortexCLI, "_animate_spinner", return_value=None) @patch.object(CortexCLI, "_clear_line", return_value=None) @patch("cortex.cli.CommandInterpreter") def test_install_dry_run( self, mock_interpreter_class, _mock_clear_line, _mock_spinner, + _mock_get_api_key_for_provider, _mock_get_api_key, _mock_get_provider, ) -> None:Apply similar changes to lines 108-124 and 131-154 for the other failing tests.
Alternatively, use
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key"}, clear=True)to set up the environment so_get_api_key_for_providerreturns a valid key naturally.
♻️ Duplicate comments (2)
cortex/first_run_wizard.py (2)
168-196: Critical: Claude API key validation still inconsistent with provider detection.Line 174 validates the API key with
startswith("sk-"), butdetect_available_providers()at line 51 specifically checks for"sk-ant-"for Anthropic/Claude keys. This inconsistency could allow invalid Claude keys (such as OpenAI keys starting with"sk-") to pass validation here, leading to API errors when the key is actually used.This issue was flagged in a previous review as addressed, but the inconsistency remains.
🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
46-59: Enhance docstring to document return value and detection logic.Per coding guidelines requiring docstrings for all public APIs, this function's docstring should document:
- Return type and possible values
- Detection criteria for each provider
- Any ordering or priority considerations
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic (Claude) API key: ANTHROPIC_API_KEY environment variable starting with 'sk-ant-' + - OpenAI API key: OPENAI_API_KEY environment variable starting with 'sk-' + - Ollama: ollama binary available in system PATH + + Returns: + list[str]: List of available provider names. Possible values are 'anthropic', + 'openai', and/or 'ollama'. Returns empty list if no providers detected. + """Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/cli.pycortex/env_loader.pycortex/first_run_wizard.pytests/test_cli.pytests/test_cli_extended.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/env_loader.py
🧰 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_cli.pytests/test_cli_extended.pycortex/cli.pycortex/first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli.pytests/test_cli_extended.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
tests/test_cli.py
🧬 Code graph analysis (3)
tests/test_cli.py (4)
tests/test_cli_extended.py (1)
test_install_dry_run(86-101)cortex/llm/interpreter.py (1)
parse(227-288)cortex/packages.py (1)
parse(383-427)cortex/cli.py (1)
install(312-557)
tests/test_cli_extended.py (2)
tests/test_cli.py (1)
test_get_api_key_not_found(32-35)cortex/cli.py (1)
_get_api_key(63-78)
cortex/cli.py (2)
cortex/first_run_wizard.py (2)
FirstRunWizard(143-940)run(298-454)cortex/llm/interpreter.py (2)
CommandInterpreter(18-298)parse(227-288)
🪛 GitHub Actions: CI
tests/test_cli_extended.py
[error] 100-100: pytest test_install_dry_run failed: No commands generated with any available provider. Please try again with a different request.
[error] 122-122: pytest test_install_no_execute failed: No commands generated with any available provider. Please try again with a different request.
[error] 153-153: pytest test_install_with_execute_success failed: No commands generated with any available provider. Please try again with a different request.
⏰ 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (7)
tests/test_cli_extended.py (1)
33-36: LGTM! Test correctly reflects environment-driven provider resolution.The test now sets
CORTEX_PROVIDER="ollama"and expects"ollama-local"as the return value, which aligns with the updated_get_api_key()logic incortex/cli.pythat returns"ollama-local"when the provider is Ollama or when no cloud API keys are available.cortex/first_run_wizard.py (2)
298-454: Provider detection and setup flow looks solid.The refactored
run()method implements a well-structured onboarding flow:
- Detects available providers before prompting
- Auto-selects when only one provider is available
- Shows menu with status indicators for multiple providers
- Validates API keys lazily (only when needed)
- Offers optional dry-run testing to verify setup
- Handles circular imports correctly with local CortexCLI import
The logic is sound and the user experience improvements are clear.
588-636: LGTM! Hardware detection implementation is clean and robust.The
_detect_hardware()and_step_hardware_detection()methods properly:
- Import hardware detection functionality from the appropriate module
- Convert hardware info using
asdict()for clean serialization- Handle exceptions gracefully with sensible fallback values
- Display hardware information with conditional formatting based on availability
The code is well-structured and handles edge cases appropriately.
cortex/cli.py (4)
47-61: LGTM! Provider-specific key retrieval is clean and well-structured.The
_get_api_key_for_provider()method properly:
- Handles all supported providers (ollama, fake, claude, openai)
- Validates API key prefixes consistently with
detect_available_providers()infirst_run_wizard.py- Returns appropriate default values for local/test providers
- Returns
Nonewhen no valid key is found, enabling graceful fallbackThe implementation is focused and maintainable.
63-78: LGTM! Updated API key retrieval properly supports multi-provider setup.The refactored
_get_api_key()method:
- Delegates to the new
_get_api_key_for_provider()for cleaner separation- Implements appropriate fallback to
"ollama-local"for offline mode- Provides helpful user guidance when no valid key is found
- Returns
Noneto signal failure, allowing callers to handle appropriatelyThis aligns with the PR objective of allowing setup to continue when API keys are present in the environment.
339-387: Multi-provider installation flow is well-designed and resilient.The enhanced
install()method implements robust provider failover:
- Tries the initial provider first (explicit
forced_provideror auto-detected)- Adds fallback provider (OpenAI ↔ Claude) when both keys are available
- Loops through providers until commands are successfully generated
- Exception handling at line 378 properly catches both
RuntimeErrorand genericException- Provides clear error message when no provider succeeds
The implementation improves reliability by automatically falling back to alternative providers when API calls fail.
Note: This change requires test updates in
test_cli_extended.py(already flagged separately) to mock_get_api_key_for_provider.
776-779: LGTM! Wizard method now executes the actual first-run flow.The updated
wizard()method properly:
- Instantiates
FirstRunWizardwithinteractive=Truefor user interaction- Executes the actual wizard flow via
wizard.run()- Returns appropriate exit codes (0 for success, 1 for failure)
This aligns with the PR objective of allowing setup to continue when API keys are already present in the environment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cortex/first_run_wizard.py (2)
46-59: Enhance docstring with return value documentation.Per coding guidelines requiring docstrings for all public APIs, this function should document its return value format and the detection criteria.
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic API key (ANTHROPIC_API_KEY starting with 'sk-ant-') + - OpenAI API key (OPENAI_API_KEY starting with 'sk-') + - Ollama installation (ollama binary in PATH) + + Returns: + List of available provider names: 'anthropic', 'openai', and/or 'ollama'. + """
168-196: Fix Claude API key validation inconsistency.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()(line 51) and therun()method (line 381) specifically check for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation in_setup_claude_api.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
🧹 Nitpick comments (3)
cortex/first_run_wizard.py (3)
1-6: Module-level dotenv loading still present.This try/except block at module level still executes on import but the
load_dotenvfunction is never called within this block. The importedload_dotenvandPathare unused here. Consider removing this dead code or clarifying its purpose.🔎 Proposed fix
-try: - from pathlib import Path - - from dotenv import load_dotenv -except ImportError: - pass """ First-Run Wizard Module for Cortex Linux
144-166: Add return type hint.Per coding guidelines, type hints are required. This method should have a return type annotation.
🔎 Proposed fix
- def _install_suggested_packages(self): + def _install_suggested_packages(self) -> None: """Offer to install suggested packages and run the install if user agrees."""
544-544: Remove redundant imports.
test_anthropic_api_keyandtest_openai_api_keyare already imported at module level (lines 29). These inline imports at lines 544 and 572 are redundant.🔎 Proposed fix
if do_test.strip().lower() in ("", "y", "yes"): print("\nTesting Claude API key...") - from cortex.utils.api_key_test import test_anthropic_api_key - if test_anthropic_api_key(existing_claude):if do_test.strip().lower() in ("", "y", "yes"): print("\nTesting OpenAI API key...") - from cortex.utils.api_key_test import test_openai_api_key - if test_openai_api_key(existing_openai):Also applies to: 572-572
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/first_run_wizard.pytests/test_cli_extended.pytests/test_first_run_wizard.py
🧰 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_cli_extended.pytests/test_first_run_wizard.pycortex/first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli_extended.pytests/test_first_run_wizard.py
🧬 Code graph analysis (3)
tests/test_cli_extended.py (2)
tests/test_cli.py (1)
test_get_api_key_not_found(32-35)cortex/cli.py (1)
_get_api_key(63-78)
tests/test_first_run_wizard.py (3)
cortex/cli.py (1)
wizard(772-779)cortex/llm/interpreter.py (1)
parse(227-288)cortex/packages.py (1)
parse(383-427)
cortex/first_run_wizard.py (2)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)cortex/hardware_detection.py (1)
detect_hardware(646-648)
🪛 GitHub Actions: CI
tests/test_first_run_wizard.py
[error] 521-521: ruff check . --output-format=github: W293 Blank line contains whitespace in tests/test_first_run_wizard.py:521:1.
🪛 GitHub Check: lint
tests/test_first_run_wizard.py
[failure] 521-521: Ruff (W293)
tests/test_first_run_wizard.py:521:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
tests/test_first_run_wizard.py
[failure] 521-521: Ruff (W293)
tests/test_first_run_wizard.py:521:1: W293 Blank line contains whitespace
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (7)
cortex/first_run_wizard.py (3)
596-608: LGTM!The
_detect_hardwaremethod properly handles exceptions and provides a sensible fallback structure. The inline import avoids circular dependency issues.
610-644: LGTM!The hardware detection step properly handles the case where GPU list may be empty and provides clear output formatting.
298-318: Provider detection logic addresses the core issue.The refactored
run()method now properly detects available providers via environment variables before prompting, which fixes the reported bug where the wizard exited early despite valid API keys being present. The auto-selection for single providers and menu for multiple providers provides a good UX.tests/test_first_run_wizard.py (1)
518-530: Test structure looks good.The integration test properly mocks the
CommandInterpreterand verifies the wizard flow completes successfully with correct file creation. The use ofsk-ant-testprefix aligns with the validation logic.tests/test_cli_extended.py (3)
33-36: LGTM!The test now explicitly sets
CORTEX_PROVIDER=ollamato test the Ollama fallback path, which correctly returns"ollama-local". This is clearer than the previous implicit behavior.
81-100: Good test isolation improvements.The addition of
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key"}, clear=True)ensures tests are isolated from ambient environment variables. The more specificparse.assert_called_once_with("install docker")assertions improve test clarity and catch regressions more effectively.Also applies to: 102-121, 123-151
76-79: The mock is ineffective;_get_api_keyis never called byinstall().The test mocks
_get_api_keyto returnNone, but theinstall()method never calls this method. Instead, it calls_get_api_key_for_provider(), which means the mock has no effect on the code execution. The return code of0is correct, but it results from the default behavior of theinstall()method whenexecute=Falseanddry_run=False(both default), not from graceful handling of missing API keys. The test setup should mock_get_api_key_for_provider()instead to actually test missing API key scenarios.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_cli_extended.py (1)
76-83: Consider renaming this test to reflect the ollama fallback behavior.The test name
test_install_no_api_keysuggests that installation should fail when no API key is present. However, the test now validates that installation succeeds (returns0) because the system falls back toollama-localwhen no API key is configured.Consider renaming to something like
test_install_fallback_to_ollamaortest_install_with_ollama_fallbackto better convey the expected behavior and avoid confusion for future maintainers.🔎 Suggested rename
- def test_install_no_api_key(self, mock_interpreter_class) -> None: + def test_install_with_ollama_fallback(self, mock_interpreter_class) -> None: mock_interpreter = Mock() mock_interpreter.parse.return_value = ["apt update", "apt install docker"] mock_interpreter_class.return_value = mock_interpretercortex/cli.py (3)
47-61: API key validation is minimal - consider strengthening the checks.The current validation only checks that API keys start with the expected prefix (
"sk-ant-"for Anthropic,"sk-"for OpenAI). Keys that are exactly the prefix (e.g.,"sk-ant-"with no additional characters) would pass validation but are invalid.While invalid keys will ultimately fail when the API is called, adding a minimum length check would catch obvious issues earlier and provide better error messages to users.
🔎 Suggested improvement
if provider == "claude": key = os.environ.get("ANTHROPIC_API_KEY") - if key and key.strip().startswith("sk-ant-"): + if key and key.strip().startswith("sk-ant-") and len(key.strip()) > 8: return key.strip() elif provider == "openai": key = os.environ.get("OPENAI_API_KEY") - if key and key.strip().startswith("sk-"): + if key and key.strip().startswith("sk-") and len(key.strip()) > 10: return key.strip()
63-78: The fallback logic is correct but could be simplified for clarity.The condition on lines 70-72 uses a double-negative that makes the logic harder to follow:
if provider == "ollama" or not ( os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY") ):Consider refactoring for improved readability:
🔎 Suggested simplification
def _get_api_key(self) -> str | None: """Get API key for the current provider.""" provider = self._get_provider() key = self._get_api_key_for_provider(provider) if key: return key - # If provider is ollama or no key is set, always fallback to ollama-local - if provider == "ollama" or not ( - os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY") - ): + # If provider is ollama or no cloud API keys are available, fallback to ollama-local + has_cloud_key = bool(os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY")) + if provider == "ollama" or not has_cloud_key: return "ollama-local" # Otherwise, prompt user for setup self._print_error("No valid API key found.")
376-378: Redundant exception types in the exception handler.The exception handler on line 376 catches both
RuntimeErrorandException:except (RuntimeError, Exception) as e:Since
Exceptionis the parent class ofRuntimeError(and all other standard exceptions), specifying both is redundant. CatchingExceptionalone would have the same effect and be more concise.🔎 Suggested simplification
- except (RuntimeError, Exception) as e: + except Exception as e: self._debug(f"API call failed with {try_provider}: {e}") continue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pytests/test_cli_extended.py
🧰 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_cli_extended.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli_extended.py
🧬 Code graph analysis (2)
tests/test_cli_extended.py (2)
tests/test_cli.py (2)
test_get_api_key_not_found(32-35)test_install_no_api_key(63-65)cortex/cli.py (2)
_get_api_key(63-78)install(312-555)
cortex/cli.py (1)
cortex/first_run_wizard.py (1)
run(298-462)
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (4)
tests/test_cli_extended.py (2)
33-36: LGTM! Environment-driven provider testing is correctly implemented.The test now properly validates that when
CORTEX_PROVIDERis set toollama, the API key resolver returns"ollama-local", which aligns with the updated fallback behavior incortex/cli.py.
85-85: LGTM! Environment-based API key setup improves test realism.The addition of
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key"}, clear=True)to these tests correctly simulates environment-driven provider selection, which aligns with the PR's objective to detect API keys from the environment. The updated assertions verifying specific parse inputs (e.g.,"install docker") add precision to the test coverage.Also applies to: 104-104, 106-106, 125-125, 127-127
cortex/cli.py (2)
339-384: LGTM! Multi-provider fallback improves resilience.The addition of multi-provider probing logic is a significant improvement. When the initial provider (cloud-based) fails, the system automatically attempts the alternative cloud provider if its API key is available. This increases the likelihood of successful command generation and enhances the user experience.
The
forced_providerparameter also enables the wizard's dry-run testing flow as described in the PR objectives.
770-777: LGTM! Wizard integration addresses the PR's core objective.This change implements the actual first-run wizard flow via
FirstRunWizard(interactive=True), which directly addresses issue #355. The wizard will now:
- Detect API keys already present in the environment
- Allow the setup to continue (provider confirmation and subsequent steps)
- Prevent premature exit when a key exists
This is the key fix described in the PR objectives and resolves the bug where the wizard exited early despite a valid API key being exported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/test_first_run_wizard.py (1)
514-516: Remove redundant decorator.Line 515 sets
ANTHROPIC_API_KEYbut line 516 immediately overrides it withclear=True. The first@patch.dictdecorator is redundant.🔎 Proposed fix
@patch("subprocess.run") - @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-test-12345678"}) @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test"}, clear=True) @patch("cortex.cli.CommandInterpreter") def test_complete_wizard_flow(self, mock_interpreter_class, mock_run, wizard):cortex/first_run_wizard.py (2)
174-174: Fix Claude API key validation inconsistency.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()at line 51 specifically checks for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
414-446: Add API validation for Anthropic keys to match OpenAI rigor.The Anthropic flow (lines 418-423) only validates the key format (
sk-ant-prefix) but doesn't test if the key actually works. In contrast, the OpenAI flow at lines 451 and 459 callstest_openai_api_key()to verify the key is valid. This asymmetry could allow users to proceed with invalid Anthropic keys, leading to failures later in the workflow.🔎 Proposed fix
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + # Valid existing key, proceed + pass + else: + key = None while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): self._save_env_var("ANTHROPIC_API_KEY", key) os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = NoneBased on past review feedback noting validation asymmetry.
🧹 Nitpick comments (1)
cortex/first_run_wizard.py (1)
46-59: Consider enhancing the docstring with return value details.Per coding guidelines requiring docstrings for all public APIs, this function would benefit from documenting its return value format and detection criteria.
🔎 Suggested docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic API key (ANTHROPIC_API_KEY starting with 'sk-ant-') + - OpenAI API key (OPENAI_API_KEY starting with 'sk-') + - Ollama installation (ollama binary in PATH) + + Returns: + List of available provider names: 'anthropic', 'openai', and/or 'ollama'. + """Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/first_run_wizard.pytests/test_first_run_wizard.py
🧰 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:
cortex/first_run_wizard.pytests/test_first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_first_run_wizard.py
🧬 Code graph analysis (2)
cortex/first_run_wizard.py (7)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)cortex/kernel_features/accelerator_limits.py (2)
get(66-69)status(94-102)cortex/env_loader.py (1)
load_env(61-113)cortex/kernel_features/kv_cache_manager.py (1)
status(127-134)cortex/kernel_features/model_lifecycle.py (1)
status(135-143)cortex/hardware_detection.py (1)
detect_hardware(646-648)cortex/logging_system.py (2)
info(198-200)warning(202-204)
tests/test_first_run_wizard.py (2)
cortex/cli.py (1)
wizard(770-777)cortex/llm/interpreter.py (1)
parse(227-288)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[warning] 336-336: Ruff: W293 Blank line contains whitespace.
🪛 GitHub Check: lint
cortex/first_run_wizard.py
[failure] 336-336: Ruff (W293)
cortex/first_run_wizard.py:336:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/first_run_wizard.py
[failure] 336-336: Ruff (W293)
cortex/first_run_wizard.py:336:1: W293 Blank line contains whitespace
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
tests/test_first_run_wizard.py (1)
522-524: LGTM: Mock setup appropriate for testing.The CommandInterpreter mock correctly simulates the parse() method returning a command list, which aligns with the integration test requirements for non-interactive wizard flow.
cortex/first_run_wizard.py (4)
144-166: LGTM: Package suggestion helper is well-implemented.The method appropriately prompts users, handles installation via subprocess with proper error handling, and uses
sys.executableto ensure the correct Python interpreter is invoked.
198-226: LGTM: OpenAI setup method includes proper validation.The method correctly validates both the key format and functionality using
test_openai_api_key(), providing a good user experience with immediate feedback and optional package suggestions.
633-645: LGTM: Hardware detection properly delegates to specialized module.The method cleanly delegates to
cortex.hardware_detection.detect_hardware()and provides a safe fallback with unknown values if detection fails, which prevents wizard failure due to hardware detection issues.
1-6: Remove unused import ofload_dotenvthat is imported but never called.Lines 1-6 import
load_dotenvbut this import is unused—the environment variables are loaded viaload_env()called at line 303. The unused import should be removed; keeping it only imports a function without executing any module-level side effects.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cortex/first_run_wizard.py (3)
46-59: Docstring still missing return value documentation.The previous review requested enhancement of this docstring to document the return value format (list of provider names: 'anthropic', 'openai', 'ollama'). This public API function should comply with the coding guidelines requiring complete docstrings for all public APIs.
Based on coding guidelines.
168-196: CRITICAL: Claude API key validation inconsistency remains unresolved.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()at line 51 specifically checks for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
414-446: Anthropic key validation is less rigorous than OpenAI.The OpenAI flow (lines 447-464) validates the key using
test_openai_api_key()before accepting it (line 451), but the Anthropic flow only checks the prefix format (sk-ant-) without validating the key works. This asymmetry could allow users to proceed with invalid Anthropic keys that have the correct prefix but are expired, revoked, or otherwise non-functional.🔎 Proposed fix to add API validation for Anthropic
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + # Valid existing key, proceed + pass + else: + key = None while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): self._save_env_var("ANTHROPIC_API_KEY", key) os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (3)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)cortex/env_loader.py (1)
load_env(61-113)cortex/hardware_detection.py (1)
detect_hardware(646-648)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted, 110 files would be left unchanged. Run 'black . --exclude "(venv|.venv|build|dist)"' to fix.
⏰ 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (4)
cortex/first_run_wizard.py (4)
302-304: Good fix: Using centralized environment loading.This properly addresses the previous concern about module-level
.envloading. The code now uses the centralizedload_env()fromcortex.env_loaderwithin therun()method, avoiding import-time side effects and ensuring consistent override behavior.
298-499: Excellent refactoring that addresses the core issue.The refactored
run()method successfully addresses issue #355 by:
- Detecting available providers via
detect_available_providers()which checks environment variables- Allowing the wizard to continue when API keys are already present in the environment
- Auto-configuring in non-interactive mode when a provider is available
- Providing a clear interactive menu showing which providers are available
The logic properly prevents premature wizard exit when valid API keys exist in the environment.
633-645: Well-implemented hardware detection with proper error handling.This implementation properly delegates to
cortex.hardware_detection.detect_hardwareand includes appropriate error handling with a safe fallback. The use ofasdict()to convert the dataclass result is correct.
28-43: Good additions for enhanced testing and user experience.The API key test utility imports and
DRY_RUN_EXAMPLESconstant enhance the wizard's ability to validate API keys and provide varied dry-run test scenarios, improving the first-time user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cortex/first_run_wizard.py (3)
46-59: Enhance docstring per coding guidelines.The docstring should document the return value format and detection logic per the requirement that "Docstrings required for all public APIs."
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic: ANTHROPIC_API_KEY starting with 'sk-ant-' + - OpenAI: OPENAI_API_KEY starting with 'sk-' + - Ollama: ollama binary in PATH + + Returns: + List of available provider names ('anthropic', 'openai', 'ollama'). + """Based on coding guidelines.
168-196: Fix Claude API key validation inconsistency.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()(line 51) specifically checks for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
464-496: Add API validation for Anthropic to match OpenAI validation rigor.The OpenAI flow (lines 501-514) validates the key using
test_openai_api_key()before accepting it, but the Anthropic flow (lines 468-473) only checks the prefix format (sk-ant-) without validating the key works. This asymmetry could allow users to proceed with invalid Anthropic keys.🔎 Proposed fix to add API validation for Anthropic
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + # Valid existing key, proceed + pass + else: + key = None while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): self._save_env_var("ANTHROPIC_API_KEY", key) os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = None
🧹 Nitpick comments (1)
cortex/first_run_wizard.py (1)
144-167: Add return type hint.The method has a docstring but is missing a return type hint. Per coding guidelines, type hints are required.
🔎 Proposed fix
- def _install_suggested_packages(self): + def _install_suggested_packages(self) -> None: """Offer to install suggested packages and run the install if user agrees."""Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 319-319: Ruff: W293 Blank line contains whitespace.
🪛 GitHub Check: lint
cortex/first_run_wizard.py
[failure] 421-421: Ruff (W293)
cortex/first_run_wizard.py:421:1: W293 Blank line contains whitespace
[failure] 417-417: Ruff (W293)
cortex/first_run_wizard.py:417:1: W293 Blank line contains whitespace
[failure] 413-413: Ruff (W293)
cortex/first_run_wizard.py:413:1: W293 Blank line contains whitespace
[failure] 409-409: Ruff (W291)
cortex/first_run_wizard.py:409:36: W291 Trailing whitespace
[failure] 404-404: Ruff (W293)
cortex/first_run_wizard.py:404:1: W293 Blank line contains whitespace
[failure] 359-359: Ruff (W293)
cortex/first_run_wizard.py:359:1: W293 Blank line contains whitespace
[failure] 353-353: Ruff (W291)
cortex/first_run_wizard.py:353:49: W291 Trailing whitespace
[failure] 348-348: Ruff (W293)
cortex/first_run_wizard.py:348:1: W293 Blank line contains whitespace
[failure] 344-344: Ruff (W293)
cortex/first_run_wizard.py:344:1: W293 Blank line contains whitespace
[failure] 319-319: Ruff (W293)
cortex/first_run_wizard.py:319:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/first_run_wizard.py
[failure] 421-421: Ruff (W293)
cortex/first_run_wizard.py:421:1: W293 Blank line contains whitespace
[failure] 417-417: Ruff (W293)
cortex/first_run_wizard.py:417:1: W293 Blank line contains whitespace
[failure] 413-413: Ruff (W293)
cortex/first_run_wizard.py:413:1: W293 Blank line contains whitespace
[failure] 409-409: Ruff (W291)
cortex/first_run_wizard.py:409:36: W291 Trailing whitespace
[failure] 404-404: Ruff (W293)
cortex/first_run_wizard.py:404:1: W293 Blank line contains whitespace
[failure] 359-359: Ruff (W293)
cortex/first_run_wizard.py:359:1: W293 Blank line contains whitespace
[failure] 353-353: Ruff (W291)
cortex/first_run_wizard.py:353:49: W291 Trailing whitespace
[failure] 348-348: Ruff (W293)
cortex/first_run_wizard.py:348:1: W293 Blank line contains whitespace
[failure] 344-344: Ruff (W293)
cortex/first_run_wizard.py:344:1: W293 Blank line contains whitespace
[failure] 319-319: Ruff (W293)
cortex/first_run_wizard.py:319:1: W293 Blank line contains whitespace
⏰ 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
cortex/first_run_wizard.py (1)
683-695: Add return type hint (already specified in signature).The return type is already correctly specified as
dict[str, Any]in the signature. Good adherence to coding guidelines.
cortex/first_run_wizard.py
Outdated
| # Detect available providers (only count valid ones) | ||
| available_providers = [] | ||
| providers = detect_available_providers() | ||
|
|
There was a problem hiding this comment.
FIX REQUIRED: Remove trailing whitespace to pass CI.
Multiple lines contain trailing whitespace or blank lines with whitespace, causing the linter to fail. These must be fixed before merge.
Run this command to fix:
#!/bin/bash
# Remove trailing whitespace
sed -i 's/[[:space:]]*$//' cortex/first_run_wizard.pyAlso applies to: 344-344, 348-348, 353-353, 359-359, 404-404, 409-409, 413-413, 417-417, 421-421
🧰 Tools
🪛 GitHub Actions: CI
[error] 319-319: Ruff: W293 Blank line contains whitespace.
🪛 GitHub Check: lint
[failure] 319-319: Ruff (W293)
cortex/first_run_wizard.py:319:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
[failure] 319-319: Ruff (W293)
cortex/first_run_wizard.py:319:1: W293 Blank line contains whitespace
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 319, 344, 348, 353, 359, 404, 409,
413, 417 and 421 there are trailing whitespace characters and blank lines with
spaces causing CI lint failures; remove trailing whitespace on those lines (or
run a repository-wide trailing-whitespace removal command) so no line ends with
spaces and save the file, then re-run the linter to confirm the issue is
resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cortex/cli.py (2)
124-191: Consider enhancing the docstring for this public method.While the basic docstring is present, it could be more comprehensive to document the
argsparameter structure and the various subcommands (config, enable, disable, dnd, send) this method handles.📝 Suggested docstring enhancement
def notify(self, args): - """Handle notification commands""" + """Handle notification commands. + + Args: + args: Namespace object with notify_action and related subcommand arguments. + Supported actions: config, enable, disable, dnd, send + + Returns: + int: 0 on success, 1 on error + """As per coding guidelines, docstrings are required for all public APIs.
372-411: Multi-provider probing logic is sound, but exception handling could be simplified.The provider fallback logic correctly attempts each provider in sequence and handles failures gracefully. However, the exception handling at line 409 catches both
RuntimeErrorandException, which is redundant sinceExceptionis the parent class ofRuntimeError.🔧 Optional simplification
- except (RuntimeError, Exception) as e: + except Exception as e:Catching
Exceptionalone is sufficient and clearer, as it inherently catchesRuntimeError,ValueError, and other exception types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 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/first_run_wizard.py (2)
FirstRunWizard(143-1035)run(298-549)cortex/llm/interpreter.py (2)
CommandInterpreter(18-298)parse(227-288)
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (4)
cortex/cli.py (4)
13-13: LGTM: FirstRunWizard importThe import is correctly placed and necessary for the wizard integration at line 808.
48-62: Provider-aware API key resolution looks correct.The method correctly handles provider-specific key retrieval with appropriate fallbacks for ollama and fake providers. The prefix validation for Anthropic (
sk-ant-) and OpenAI (sk-) keys provides basic validation.
64-79: API key fallback logic aligns with PR objectives.The updated method now detects existing API keys for the current provider and falls back to
ollama-localwhen appropriate. This allows the wizard to continue setup when keys are already present, addressing the core issue in #355.
803-810: Wizard integration correctly addresses the PR objectives.The wizard method now executes the actual
FirstRunWizardflow, which (per the relevant code snippets) will detect available providers, validate existing API keys, and continue the setup flow rather than exiting prematurely. This directly fixes issue #355 where the wizard would exit even when API keys were already exported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cortex/first_run_wizard.py (2)
168-198: REGRESSION: Claude API key validation is still inconsistent.Line 174 validates that the Claude API key starts with
"sk-", but Anthropic keys should start with"sk-ant-"(as correctly checked indetect_available_providers()on line 51). This was flagged in a previous review and marked as addressed, but the issue is still present.This allows invalid Claude keys (including OpenAI keys) to pass validation.
🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
477-508: REGRESSION: Anthropic key validation remains less rigorous than OpenAI.The Anthropic validation loop (lines 481-485) only checks the prefix format without calling
test_anthropic_api_key(), while the OpenAI flow (lines 513, 521) properly validates keys withtest_openai_api_key(). This asymmetry was flagged in a previous review and marked as addressed, but is still present.Users could proceed with syntactically valid but non-working Anthropic keys.
🔎 Proposed fix to add API validation
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + # Valid existing key, proceed + pass + else: + key = None while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = None
🧹 Nitpick comments (1)
cortex/first_run_wizard.py (1)
329-344: Consider documenting the two-tier validation approach.The code performs basic validation here (prefix + length checks) and defers full API validation to the provider setup flows. While this approach works, the inconsistency in later API validation (Anthropic lacks full validation) makes the overall flow confusing.
Once the API validation asymmetry is fixed, consider adding a comment here to clarify that this is "quick validation" and full API tests happen during provider setup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.examplecortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 1-1: black --check . --exclude "(venv|.venv|build|dist)" failed: 1 file would be reformatted. Run 'black --write' to fix formatting.
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (8)
.env.example (1)
1-5: LGTM! Static analysis ordering hint can be safely ignored.The file structure is clear and provides helpful guidance for users. The ordering of keys (OpenAI before Anthropic) is logical and does not need to be alphabetical.
cortex/first_run_wizard.py (7)
307-317: Good fix: Environment loading is now centralized.The environment loading has been properly moved into the
run()method and uses the centralizedload_env()fromcortex.env_loader. This resolves the previous concerns about module-level side effects and ensures consistent behavior.
347-385: Excellent UX: Demo mode provides a smooth fallback.The demo mode flow handles the "no API keys" scenario gracefully:
- Non-interactive mode auto-enables demo for automation
- Interactive mode provides clear guidance with links to API key sources
- Allows users to explore Cortex before committing to API setup
This directly addresses the PR objective of improving the first-time user experience.
411-474: Well-designed interactive provider selection.The provider menu provides excellent UX:
- Dynamic options based on available providers
- Context about current configuration
- Skip option to preserve existing setup
- Clear status indicators
The flow naturally guides users through the selection process.
486-508: Smart integration: Dry run testing validates the full stack.The dry run testing (here and for OpenAI on lines 526-548) provides real validation of the setup by:
- Testing actual API connectivity
- Using randomized examples for variety
- Providing clear success/failure feedback
- Preventing progression with broken configuration
This is a significant improvement over just API key format validation.
594-601: PR objective achieved: Existing API keys are now detected and used.This code directly fixes the reported issue where the wizard would exit even with valid API keys in the environment. The wizard now:
- Detects existing keys from environment variables
- Shows status indicators in the menu
- Tests existing keys before prompting for new ones
- Allows the setup flow to continue
This resolves issue #355.
694-742: Robust hardware detection with proper error handling.The hardware detection implementation:
- Delegates to a dedicated module for separation of concerns
- Provides safe defaults on failure
- Handles edge cases (missing GPU, multiple GPUs)
- Presents results clearly to users
144-167: Nice onboarding touch: Package suggestions help validate setup.The suggested package installation provides immediate value and validates that Cortex is working end-to-end. The implementation properly:
- Uses subprocess for isolation
- Captures and displays output
- Handles errors gracefully
jaysurse
force-pushed
the
fix/cortex-wizard-not-running
branch
from
4af01e5 to
822098d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cortex/first_run_wizard.py (2)
168-198: Fix Claude API key validation to match provider detection logic.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()at line 49-51 specifically checks for"sk-ant-"for Anthropic/Claude keys. This inconsistency allows invalid Claude keys (e.g., OpenAI keys starting with just "sk-") to pass the initial validation.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
483-514: Add API validation for Anthropic keys to match OpenAI validation rigor.The Anthropic key validation only checks the prefix format (
sk-ant-) without validating that the key actually works. The OpenAI flow (lines 519-528) validates keys usingtest_openai_api_key()before accepting them. This asymmetry could allow users to proceed with invalid Anthropic keys.🔎 Proposed fix to add API validation
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + # Valid existing key, proceed + pass + else: + key = None while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = None
🧹 Nitpick comments (3)
cortex/first_run_wizard.py (3)
46-59: Enhance docstring to document return value and detection logic.Per coding guidelines requiring docstrings for all public APIs, this function should document its return value format and detection criteria.
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic: ANTHROPIC_API_KEY environment variable starting with 'sk-ant-' + - OpenAI: OPENAI_API_KEY environment variable starting with 'sk-' + - Ollama: ollama binary present in PATH + + Returns: + List of available provider names ('anthropic', 'openai', 'ollama'). + Empty list if no providers are detected. + """ providers = []Based on coding guidelines.
331-346: Consider API validation for provider detection to prevent false positives.The provider validation only performs format checks (prefix and length), not actual API validation. This could result in invalid keys being counted as available providers, leading to confusing UX when users select a provider that subsequently fails.
Compare with lines 519-523 where OpenAI keys are validated using
test_openai_api_key(). Consider applying similar validation here for more accurate provider detection.🔎 Proposed enhancement
# Validate providers - only count ones with working API keys for provider in providers: if provider == "ollama": available_providers.append(provider) # Ollama doesn't need validation elif provider == "openai": key = os.environ.get("OPENAI_API_KEY", "") - if key.startswith("sk-") and len(key) > 20: # Basic validation + if key.startswith("sk-") and test_openai_api_key(key): available_providers.append(provider) elif provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY", "") - if key.startswith("sk-ant-") and len(key) > 20: # Basic validation + if key.startswith("sk-ant-") and test_anthropic_api_key(key): available_providers.append(provider)
714-748: Consider documenting single GPU limitation.Line 733 takes only the first GPU from the list. For systems with multiple GPUs, this might be unexpected behavior. Consider either displaying all GPUs or documenting this limitation.
🔎 Possible enhancement to show all GPUs
print(" • CPU:", cpu.get("model", "Unknown")) if gpu_list: - gpu = gpu_list[0] # Take first GPU - gpu_vendor = gpu.get("vendor", "unknown") - gpu_model = gpu.get("model", "Detected") - if gpu_vendor != "unknown": - print(f" • GPU: {gpu_model} ({gpu_vendor})") - else: - print(" • GPU: Detected") + for idx, gpu in enumerate(gpu_list): + gpu_vendor = gpu.get("vendor", "unknown") + gpu_model = gpu.get("model", "Detected") + prefix = " • GPU" if len(gpu_list) == 1 else f" • GPU {idx + 1}" + if gpu_vendor != "unknown": + print(f"{prefix}: {gpu_model} ({gpu_vendor})") + else: + print(f"{prefix}: Detected") else: print(" • GPU: Not detected")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.examplecortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
⏰ 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)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (3)
cortex/first_run_wizard.py (2)
306-319: Good approach: centralized environment loading at runtime.The environment loading is now properly centralized within the
run()method, avoiding import-time side effects. Creating.envfrom.env.examplewhen missing is helpful for first-time users.
515-531: Good practice: validating API keys before accepting them.The OpenAI flow properly validates keys using
test_openai_api_key()before accepting them, providing better UX by catching invalid keys early. This approach should be consistently applied to all providers..env.example (1)
1-5: LGTM! Example file is clear and functional.The
.env.examplefile provides clear placeholders for API keys with helpful comments. The static analysis warning about key ordering is pedantic and can be safely ignored for an example file—the order doesn't affect functionality.
jaysurse
force-pushed
the
fix/cortex-wizard-not-running
branch
from
822098d to
4be3bb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cortex/first_run_wizard.py (3)
46-59: Enhance docstring with return value documentation.Per coding guidelines requiring docstrings for all public APIs, this function should document what it returns and the criteria for each provider.
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on API keys and installations.""" + """Detect available providers based on API keys and installations. + + Checks for: + - Anthropic API key (ANTHROPIC_API_KEY starting with 'sk-ant-') + - OpenAI API key (OPENAI_API_KEY starting with 'sk-') + - Ollama installation (ollama binary in PATH) + + Returns: + List of available provider names: 'anthropic', 'openai', and/or 'ollama'. + """ providers = []
483-514: Anthropic key validation is less rigorous than OpenAI.The OpenAI flow (lines 516-531) validates the key using
test_openai_api_key()before accepting it, but the Anthropic flow (lines 483-514) only checks the prefix format (sk-ant-) without actually validating the key works viatest_anthropic_api_key(). This asymmetry could allow users to proceed with invalid Anthropic keys.🔎 Proposed fix to add API validation for Anthropic
if provider == "anthropic": key = os.environ.get("ANTHROPIC_API_KEY") if key: key = key.strip() - while not key or not key.startswith("sk-ant-"): + if key.startswith("sk-ant-") and test_anthropic_api_key(key): + pass # Valid existing key + else: + key = None + while not key or not key.startswith("sk-ant-"): print("\nNo valid Anthropic API key found.") key = self._prompt("Enter your Claude (Anthropic) API key: ") - if key and key.startswith("sk-ant-"): + if key and key.startswith("sk-ant-") and test_anthropic_api_key(key): os.environ["ANTHROPIC_API_KEY"] = key + else: + print("Invalid API key. Please try again.") + key = None
168-198: Claude API key validation is inconsistent with provider detection.Line 174 validates that the API key starts with
"sk-", butdetect_available_providers()(line 51) specifically checks for"sk-ant-"for Anthropic keys. This inconsistency could allow invalid Claude keys to pass validation in_setup_claude_api()while failing detection elsewhere.🔎 Proposed fix
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-ant-"): print("\n⚠ Invalid API key format") return StepResult(success=True, data={"api_provider": "none"})
🧹 Nitpick comments (3)
cortex/first_run_wizard.py (3)
1-6: Module-level import side effects removed but docstring placement is unusual.The module-level
load_dotenvcall from previous commits has been removed (addressed per past review). However, the try/except block at lines 1-6 importsPathandload_dotenvbut doesn't use them before the module docstring at line 7. ThePathimport is redundant since it's imported again at line 25.🔎 Proposed fix - remove redundant try/except block
-try: - from pathlib import Path - - from dotenv import load_dotenv -except ImportError: - pass """ First-Run Wizard Module for Cortex Linux
144-166: Unused environment copy and missing return type hint.
Line 152:
env = os.environ.copy()is assigned but the copy is not modified before passing tosubprocess.run(). You can passos.environdirectly or remove the variable.Per coding guidelines, type hints are required. This method should have a return type annotation.
🔎 Proposed fix
- def _install_suggested_packages(self): + def _install_suggested_packages(self) -> None: """Offer to install suggested packages and run the install if user agrees.""" suggestions = ["python", "numpy", "requests"] print("\nTry installing a package to verify Cortex is ready:") for pkg in suggestions: print(f" cortex install {pkg}") resp = self._prompt("Would you like to install these packages now? [Y/n]: ", default="y") if resp.strip().lower() in ("", "y", "yes"): - env = os.environ.copy() for pkg in suggestions: print(f"\nInstalling {pkg}...") try: result = subprocess.run( [sys.executable, "-m", "cortex.cli", "install", pkg], capture_output=True, text=True, - env=env, )
648-648: Remove redundant imports.
test_anthropic_api_keyandtest_openai_api_keyare already imported at line 29. The local imports at lines 648 and 676 are unnecessary.🔎 Proposed fix
if do_test.strip().lower() in ("", "y", "yes"): print("\nTesting Claude API key...") - from cortex.utils.api_key_test import test_anthropic_api_key - if test_anthropic_api_key(existing_claude):if do_test.strip().lower() in ("", "y", "yes"): print("\nTesting OpenAI API key...") - from cortex.utils.api_key_test import test_openai_api_key - if test_openai_api_key(existing_openai):Also applies to: 676-676
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.examplecortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (3)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)cortex/env_loader.py (1)
load_env(61-113)cortex/hardware_detection.py (1)
detect_hardware(656-658)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (7)
cortex/first_run_wizard.py (6)
200-230: LGTM!The OpenAI API setup method correctly validates the key prefix and tests the key when in interactive mode.
232-235: LGTM, but consider adding return type hint.The Ollama setup is straightforward. Per coding guidelines, type hints are required, which this method already has in its signature via the
-> StepResultannotation.
499-514: LGTM!The dry-run test flow with lazy CLI import to avoid circular dependencies is well-structured. Error handling appropriately returns
Falseon failures.
700-712: LGTM!The hardware detection delegation is well-implemented with appropriate error handling and fallback values.
714-748: LGTM!The hardware detection step properly handles the GPU list structure and provides clear, user-friendly output.
302-319: Core fix for PR objective: Environment loading before provider detection.This change properly addresses the issue from #355. By calling
load_env()beforedetect_available_providers(), API keys from.envfiles are now available when the wizard runs, preventing premature exit when valid keys exist..env.example (1)
1-5: LGTM!The example file correctly demonstrates the expected API key formats with appropriate placeholders. The static analysis hint about alphabetical key ordering is a minor stylistic preference that can be ignored.
jaysurse
force-pushed
the
fix/cortex-wizard-not-running
branch
from
4be3bb2 to
3552863
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cortex/first_run_wizard.py (2)
204-215: Enhance docstring with return value details.Per coding guidelines requiring docstrings for all public APIs, this function should document its return value format.
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on valid (non-blank) API keys in .env file.""" + """Detect available providers based on valid (non-blank) API keys in .env file. + + Checks for: + - Anthropic API key (ANTHROPIC_API_KEY starting with 'sk-ant-') + - OpenAI API key (OPENAI_API_KEY starting with 'sk-') + - Ollama installation (ollama binary in PATH) + + Returns: + List of available provider names: 'anthropic', 'openai', and/or 'ollama'. + """ providers = []Based on coding guidelines.
1-8: Module-level dotenv loading still has issues flagged in past reviews.This pattern was previously flagged for causing import-time side effects and using
override=Truewhich contradicts the centralized loader's design. Additionally, the imports within the try block trigger an I001 linter failure.If you must keep this pattern, at minimum fix the import sorting to pass CI. However, consider removing this block entirely and relying on the centralized
load_env()approach as previously suggested.🔎 Proposed fix to address linter error
try: - from pathlib import Path from dotenv import load_dotenv + from pathlib import Path # Load from parent directory .env as well load_dotenv(dotenv_path=Path.cwd().parent / ".env", override=True) load_dotenv(dotenv_path=Path.cwd() / ".env", override=True) except ImportError: pass
🧹 Nitpick comments (3)
.env.example (1)
1-5: LGTM!Clear and helpful example file for users to configure their API keys. The placeholder values appropriately indicate the expected key format for each provider.
Nit: Consider alphabetically ordering keys (
ANTHROPIC_API_KEYbeforeOPENAI_API_KEY) to satisfy dotenv-linter'sUnorderedKeyrule, though this is purely a style preference for example files.cortex/first_run_wizard.py (2)
534-547: Consider extracting duplicate dry-run verification logic.The dry-run verification code for Anthropic (lines 534-547) and OpenAI (lines 572-585) is nearly identical. Consider extracting this into a helper method to reduce duplication.
🔎 Proposed helper method
def _verify_provider_setup(self, provider: str) -> bool: """Run a dry-run verification for the given provider.""" random_example = random.choice(DRY_RUN_EXAMPLES) print(f'\nVerifying setup with dry run: cortex install "{random_example}"...') try: from cortex.cli import CortexCLI cli = CortexCLI() result = cli.install(random_example, execute=False, dry_run=True, forced_provider=provider) if result != 0: print("\n❌ Dry run failed. Please check your API key and network.") return False print("\n✅ API key verified successfully!") return True except Exception as e: print(f"\n❌ Error during verification: {e}") return FalseThen call it as:
if not self._verify_provider_setup("claude"): return False
393-412: Add return type hint.Missing return type annotation. Per coding guidelines, type hints are required.
- def _install_suggested_packages(self): + def _install_suggested_packages(self) -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.examplecortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (2)
cortex/utils/api_key_test.py (2)
test_anthropic_api_key(6-25)test_openai_api_key(28-42)cortex/cli.py (1)
_print_error(107-108)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 2-2: Ruff check failed (I001): Import block is unsorted or un-formatted. Run 'ruff format' or fix imports and re-run.
🪛 GitHub Check: lint
cortex/first_run_wizard.py
[failure] 96-96: Ruff (W293)
cortex/first_run_wizard.py:96:1: W293 Blank line contains whitespace
[failure] 90-90: Ruff (W293)
cortex/first_run_wizard.py:90:1: W293 Blank line contains whitespace
[failure] 84-84: Ruff (W293)
cortex/first_run_wizard.py:84:1: W293 Blank line contains whitespace
[failure] 78-78: Ruff (UP015)
cortex/first_run_wizard.py:78:29: UP015 Unnecessary mode argument
[failure] 76-76: Ruff (W293)
cortex/first_run_wizard.py:76:1: W293 Blank line contains whitespace
[failure] 73-73: Ruff (W293)
cortex/first_run_wizard.py:73:1: W293 Blank line contains whitespace
[failure] 62-62: Ruff (W293)
cortex/first_run_wizard.py:62:1: W293 Blank line contains whitespace
[failure] 58-58: Ruff (W293)
cortex/first_run_wizard.py:58:1: W293 Blank line contains whitespace
[failure] 17-32: Ruff (I001)
cortex/first_run_wizard.py:17:1: I001 Import block is un-sorted or un-formatted
[failure] 2-3: Ruff (I001)
cortex/first_run_wizard.py:2:5: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
cortex/first_run_wizard.py
[failure] 96-96: Ruff (W293)
cortex/first_run_wizard.py:96:1: W293 Blank line contains whitespace
[failure] 90-90: Ruff (W293)
cortex/first_run_wizard.py:90:1: W293 Blank line contains whitespace
[failure] 84-84: Ruff (W293)
cortex/first_run_wizard.py:84:1: W293 Blank line contains whitespace
[failure] 78-78: Ruff (UP015)
cortex/first_run_wizard.py:78:29: UP015 Unnecessary mode argument
[failure] 76-76: Ruff (W293)
cortex/first_run_wizard.py:76:1: W293 Blank line contains whitespace
[failure] 73-73: Ruff (W293)
cortex/first_run_wizard.py:73:1: W293 Blank line contains whitespace
[failure] 62-62: Ruff (W293)
cortex/first_run_wizard.py:62:1: W293 Blank line contains whitespace
[failure] 58-58: Ruff (W293)
cortex/first_run_wizard.py:58:1: W293 Blank line contains whitespace
[failure] 17-32: Ruff (I001)
cortex/first_run_wizard.py:17:1: I001 Import block is un-sorted or un-formatted
[failure] 2-3: Ruff (I001)
cortex/first_run_wizard.py:2:5: I001 Import block is un-sorted or un-formatted
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (1)
cortex/first_run_wizard.py (1)
414-603: Core wizard flow correctly addresses the PR objective.The
run()method now properly:
- Reloads the .env file to get fresh values (lines 427-434)
- Detects available providers from .env (line 445)
- Shows provider selection menu with status indicators (lines 463-491)
- Handles existing valid keys without prompting (lines 513, 528-529 for Anthropic; 551, 566-567 for OpenAI)
- Only prompts for key entry when the key is missing or blank
This correctly fixes issue #355 where the wizard exited early despite a valid API key being present.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cortex/utils/api_key_validator.py (1)
1-1: Remove unusedosimport.The
osmodule is imported but never used in this file. This also resolves the I001 import formatting issue mentioned in past reviews.🔎 Proposed fix
-import os - import requests
🧹 Nitpick comments (3)
cortex/utils/api_key_validator.py (1)
6-24: Consider logging validation failures for debugging.The broad exception handling silently returns
Falsewithout logging. While acceptable for a simple validator, adding optional debug logging could help troubleshoot API connectivity issues during setup.Example enhancement
def validate_anthropic_api_key(api_key: str) -> bool: """Validate Anthropic (Claude) API key by making a minimal request.""" try: headers = { "x-api-key": api_key, "anthropic-version": "2023-06-01", "content-type": "application/json", } data = { "model": "claude-3-opus-20240229", "max_tokens": 1, "messages": [{"role": "user", "content": "Hello"}], } resp = requests.post( "https://api.anthropic.com/v1/messages", headers=headers, json=data, timeout=10 ) return resp.status_code == 200 except Exception as e: logger.debug(f"Anthropic API key validation failed: {e}") return Falsecortex/first_run_wizard.py (2)
42-87: Consider adding error handling for malformed .env files.The
read_key_from_env_filefunction handles basic parsing but could be more robust when encountering malformed entries (e.g., multiple=signs, unclosed quotes).Example enhancement
For line 72, consider using
line.partition("=")more carefully or adding a try-except around the quote stripping to handle edge cases like unbalanced quotes.
168-179: Enhance docstring with return value details.Per coding guidelines requiring docstrings for all public APIs, this function should document its return value format and detection criteria.
🔎 Proposed docstring enhancement
def detect_available_providers() -> list[str]: - """Detect available providers based on valid (non-blank) API keys in .env file.""" + """Detect available providers based on valid (non-blank) API keys in .env file. + + Checks for: + - Anthropic API key (ANTHROPIC_API_KEY starting with 'sk-ant-') + - OpenAI API key (OPENAI_API_KEY starting with 'sk-') + - Ollama installation (ollama binary in PATH) + + Returns: + List of available provider names: 'anthropic', 'openai', and/or 'ollama'. + """Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/first_run_wizard.pycortex/utils/api_key_validator.pytests/test_first_run_wizard.py
🧰 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:
cortex/utils/api_key_validator.pytests/test_first_run_wizard.pycortex/first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_first_run_wizard.py
🧬 Code graph analysis (2)
tests/test_first_run_wizard.py (1)
cortex/first_run_wizard.py (1)
_step_api_setup(640-653)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_validator.py (2)
validate_anthropic_api_key(6-24)validate_openai_api_key(27-41)
🪛 GitHub Actions: CI
cortex/first_run_wizard.py
[error] 772-772: Command 'ruff check . --output-format=github' failed. Ruff error: F822 Undefined name 'test_anthropic_api_key' in all.
🪛 GitHub Check: lint
cortex/utils/api_key_validator.py
[failure] 46-46: Ruff (W292)
cortex/utils/api_key_validator.py:46:46: W292 No newline at end of file
tests/test_first_run_wizard.py
[failure] 545-545: Ruff (W292)
tests/test_first_run_wizard.py:545:34: W292 No newline at end of file
cortex/first_run_wizard.py
[failure] 781-781: Ruff (W292)
cortex/first_run_wizard.py:781:67: W292 No newline at end of file
[failure] 773-773: Ruff (F822)
cortex/first_run_wizard.py:773:5: F822 Undefined name test_openai_api_key in __all__
[failure] 772-772: Ruff (F822)
cortex/first_run_wizard.py:772:5: F822 Undefined name test_anthropic_api_key in __all__
🪛 GitHub Check: Lint
cortex/utils/api_key_validator.py
[failure] 46-46: Ruff (W292)
cortex/utils/api_key_validator.py:46:46: W292 No newline at end of file
tests/test_first_run_wizard.py
[failure] 545-545: Ruff (W292)
tests/test_first_run_wizard.py:545:34: W292 No newline at end of file
cortex/first_run_wizard.py
[failure] 781-781: Ruff (W292)
cortex/first_run_wizard.py:781:67: W292 No newline at end of file
[failure] 773-773: Ruff (F822)
cortex/first_run_wizard.py:773:5: F822 Undefined name test_openai_api_key in __all__
[failure] 772-772: Ruff (F822)
cortex/first_run_wizard.py:772:5: F822 Undefined name test_anthropic_api_key in __all__
🪛 Gitleaks (8.30.0)
tests/test_first_run_wizard.py
[high] 269-269: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
tests/test_first_run_wizard.py (1)
261-276: LGTM! Improved test coverage for provider-specific API key detection.The refactored tests now explicitly verify both Anthropic and OpenAI key handling, with proper environment isolation using
clear=True. This aligns well with the wizard's environment-based provider discovery.cortex/first_run_wizard.py (5)
21-22: Imports look good.The import of validation utilities from
cortex.utils.api_key_validatoris correct. However, note that the__all__export list at lines 772-773 references undefined names (see separate comment).
318-351: Good validation logic with clear user guidance.The
_prompt_for_api_keymethod properly validates key format and rejects blank inputs, addressing the core issue from #355. The user guidance for obtaining keys is helpful.
378-577: Well-structured provider selection flow.The refactored
run()method implements the fix described in the PR objectives:
- Detects existing API keys in environment
- Prevents premature wizard exit when keys are present
- Allows continuation of setup flow with provider confirmation
The dry-run verification at lines 498-511 and 548-561 is a nice touch for validating the setup.
498-498: Verify noqa comment justification.The
# noqa: S311comment suppresses the bandit security warning about usingrandom.choiceinstead ofsecrets.choice. Since this is only selecting an example string for display (not for cryptographic purposes), the suppression is appropriate.
640-653: Legacy test compatibility maintained correctly.The
_step_api_setupmethod properly maintains backward compatibility with existing tests while the mainrun()method implements the new provider selection flow. This is the method that tests intest_first_run_wizard.pyare exercising.
cortex/utils/api_key_validator.py
Outdated
|
|
||
| # Aliases for backward compatibility | ||
| test_anthropic_api_key = validate_anthropic_api_key | ||
| test_openai_api_key = validate_openai_api_key No newline at end of file |
There was a problem hiding this comment.
Fix missing newline at end of file to pass CI.
The linter is failing because the file does not end with a newline character (W292).
🔎 Proposed fix
# Aliases for backward compatibility
test_anthropic_api_key = validate_anthropic_api_key
test_openai_api_key = validate_openai_api_key
+📝 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.
| test_openai_api_key = validate_openai_api_key | |
| test_openai_api_key = validate_openai_api_key | |
🧰 Tools
🪛 GitHub Check: lint
[failure] 46-46: Ruff (W292)
cortex/utils/api_key_validator.py:46:46: W292 No newline at end of file
🪛 GitHub Check: Lint
[failure] 46-46: Ruff (W292)
cortex/utils/api_key_validator.py:46:46: W292 No newline at end of file
🤖 Prompt for AI Agents
In cortex/utils/api_key_validator.py around line 46, the file is missing a
trailing newline which causes linter CI error W292; fix it by adding a single
newline character at the end of the file so the last line ends with '\n' and
save the file.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/first_run_wizard.py
🧰 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/first_run_wizard.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_validator.py (2)
validate_anthropic_api_key(10-28)validate_openai_api_key(34-48)
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_first_run_wizard.py (1)
277-287: Consider removing redundant environment cleanup.Lines 281-282 explicitly pop
ANTHROPIC_API_KEYandOPENAI_API_KEYafter line 277 already clears the entire environment withclear=True. The explicitpop()calls are no-ops and can be safely removed.🔎 Proposed simplification
@patch.dict(os.environ, {}, clear=True) def test_step_api_setup_no_key(self, wizard): """Test API setup without existing key.""" - # Remove any existing keys - os.environ.pop("ANTHROPIC_API_KEY", None) - os.environ.pop("OPENAI_API_KEY", None) - result = wizard._step_api_setup() assert result.success is True
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_first_run_wizard.py
🧰 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_first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_first_run_wizard.py
🧬 Code graph analysis (1)
tests/test_first_run_wizard.py (1)
cortex/cli.py (1)
wizard(803-810)
🪛 Gitleaks (8.30.0)
tests/test_first_run_wizard.py
[high] 269-269: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (2)
tests/test_first_run_wizard.py (2)
261-267: LGTM! Test correctly validates Anthropic key detection.The renamed test clearly validates that the wizard detects an existing
ANTHROPIC_API_KEYand sets the provider accordingly. This aligns with the PR objective of allowing the wizard to continue when API keys are already present in the environment.
269-275: LGTM! Test correctly validates OpenAI key detection.The new test appropriately validates that the wizard detects an existing
OPENAI_API_KEYand sets the provider to "openai". The use ofclear=Trueensures proper test isolation.Note: The static analysis tool flagged the mock API key as sensitive, but this is a false positive—it's test data, not a real credential.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
requirements.txt (1)
9-25: PyYAML is declared three times with inconsistent versions.The file contains three conflicting PyYAML entries:
- Line 9:
PyYAML>=6.0.0- Line 21:
pyyaml>=6.0.0- Line 25:
PyYAML==6.0.3This creates confusion and the pinned version (6.0.3) will conflict with the flexible constraints. Consolidate to a single entry.
🔎 Suggested fix
# Configuration PyYAML>=6.0.0 # Environment variable loading from .env files python-dotenv>=1.0.0 # Encryption for environment variable secrets cryptography>=42.0.0 # Terminal UI rich>=13.0.0 -# Configuration -pyyaml>=6.0.0 - # Type hints for older Python versions typing-extensions>=4.0.0 -PyYAML==6.0.3cortex/cli.py (1)
1243-1253: Remove duplicatenotifyrow from rich help.
show_rich_help()currently addsnotifytwice andenvonce:table.add_row("notify", "Manage desktop notifications") table.add_row("env", "Manage environment variables") table.add_row("notify", "Manage desktop notifications")Drop the duplicate
notifyrow so the help table stays clean:- table.add_row("notify", "Manage desktop notifications") - table.add_row("env", "Manage environment variables") - table.add_row("notify", "Manage desktop notifications") + table.add_row("notify", "Manage desktop notifications") + table.add_row("env", "Manage environment variables")
🧹 Nitpick comments (9)
examples/parallel_llm_demo.py (1)
215-219: Awkward multi-line formatting with stray empty lines.The print statement spans multiple lines with seemingly empty lines inside the parentheses, which hurts readability. Consider consolidating or using proper line continuation.
🔎 Suggested cleanup
print( - - f" Time saved: {elapsed_seq - elapsed_par:.2f}s ({((elapsed_seq - elapsed_par) / elapsed_seq * 100):.1f}%)" - + f" Time saved: {elapsed_seq - elapsed_par:.2f}s " + f"({((elapsed_seq - elapsed_par) / elapsed_seq * 100):.1f}%)" )test_parallel_llm.py (2)
146-149: Accessing private semaphore attribute is fragile.
router._rate_limit_semaphore._valuerelies on internal implementation details. If the router's rate-limiting mechanism changes, this test will break silently or give misleading results.Consider either exposing a public method to query rate limit state or simply verifying the behavior (e.g., that 5 requests complete without error when limited to 2 concurrent).
17-18: Thesys.pathinsertion is functional but unclear; prefer explicit absolute path.The current code with
"."works correctly since the file is at the repository root and the cortex module is importable. However, the approach is ambiguous and relies on path normalization. Use an explicit absolute path approach instead:# Add parent directory to path -sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".")) +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))This is clearer, more maintainable, and remains functionally equivalent.
cortex/env_manager.py (3)
77-86:from_dictmay raise unclearKeyErrorif data is malformed.If
data["key"]ordata["value"]is missing, a rawKeyErrorwill propagate without context. Consider validating required keys or wrapping in a more descriptive error.🔎 Optional defensive check
@classmethod def from_dict(cls, data: dict[str, Any]) -> EnvironmentVariable: """Create from dictionary.""" + if "key" not in data or "value" not in data: + raise ValueError("EnvironmentVariable requires 'key' and 'value' fields") return cls( key=data["key"], value=data["value"], encrypted=data.get("encrypted", False), description=data.get("description", ""), var_type=data.get("var_type", "string"), )
536-542:is_key_available()has a side effect: it may create the key file.The method name suggests a read-only check, but
_ensure_key_exists()will create the key if it doesn't exist. This could be surprising to callers who only want to query availability.Consider renaming to
ensure_key_available()or checkingself.key_path.exists()first before calling_ensure_key_exists().
910-914: Quoted value parsing doesn't handle escape sequences.The
.envimport strips outer quotes but doesn't process escape sequences like\"or\n. Values containing escaped quotes or newlines will be imported incorrectly.Consider using a library like
python-dotenvfor parsing, or document this limitation:# Note: Escape sequences within quoted values are not processeddocs/NETWORK_CONFIG.md (1)
25-58: Add code-fence languages to satisfy markdownlint (MD040).Several fenced blocks use bare triple backticks (architecture diagram at line 25, cache path at line 281, apt proxy path at line 316, apt proxy contents at line 321). markdownlint (MD040) flags these.
Recommend annotating them with a language, e.g.:
-``` +```text ┌─────────────────────────────────────────────────────────────────┐ ...and similarly
```textfor the cache path and apt-related blocks.This keeps rendering unchanged while making linters and editors happier.
Also applies to: 281-326
tests/test_env_manager.py (1)
850-866: Avoid writing to the real$HOMEinTestGetEnvManagertests.
test_get_env_manager_uses_default_pathsasserts default paths underPath.home() / ".cortex", which means the tests will touch the caller’s actual home dir (~/.cortex/environments,~/.cortex/.env_key).To keep tests hermetic and side‑effect free, consider patching
Path.home()totemp_dirin this test (or using a fixture) so you still validate the default-relative paths without hitting the real filesystem.cortex/network_config.py (1)
257-290: Ensure TCP socket is always closed incheck_connectivity.
check_connectivity()creates a rawsocket.socket, callsconnect(), and only closes it on the success path:sock = socket.socket(...) sock.settimeout(timeout) sock.connect(("8.8.8.8", 53)) sock.close()If
connect()raises, the function falls through to the HTTP fallback without closing the socket. It will be cleaned up eventually, but using a context manager orfinallyis safer and more idiomatic:- try: - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.settimeout(timeout) - sock.connect(("8.8.8.8", 53)) - sock.close() - return True - except OSError: + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + sock.settimeout(timeout) + sock.connect(("8.8.8.8", 53)) + return True + except OSError: # Connection failed - try HTTP endpoints passNot critical here, but it avoids leaking sockets under repeated failures.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
cortex/cli.pycortex/coordinator.pycortex/env_manager.pycortex/first_run_wizard.pycortex/hardware_detection.pycortex/installation_history.pycortex/installation_verifier.pycortex/kernel_features/accelerator_limits.pycortex/kernel_features/kv_cache_manager.pycortex/kernel_features/model_lifecycle.pycortex/logging_system.pycortex/network_config.pycortex/progress_indicators.pycortex/progress_tracker.pycortex/transaction_history.pydocs/ENV_MANAGEMENT.mddocs/NETWORK_CONFIG.mddocs/NETWORK_CONFIG_TESTS.mdexamples/env_demo.pyexamples/parallel_llm_demo.pyexamples/progress_demo.pyexamples/standalone_demo.pyrequirements.txtsrc/intent/llm_agent.pysrc/intent/planner.pytest_parallel_llm.pytests/test_cli.pytests/test_coordinator.pytests/test_env_manager.pytests/test_interpreter.pytests/test_llm_router.pytests/test_logging_system.pytests/test_network_config.pytests/unit/test_progress_tracker.py
💤 Files with no reviewable changes (4)
- tests/test_coordinator.py
- src/intent/llm_agent.py
- tests/test_interpreter.py
- src/intent/planner.py
✅ Files skipped from review due to trivial changes (14)
- cortex/hardware_detection.py
- cortex/logging_system.py
- cortex/progress_indicators.py
- cortex/kernel_features/kv_cache_manager.py
- examples/standalone_demo.py
- cortex/kernel_features/accelerator_limits.py
- cortex/kernel_features/model_lifecycle.py
- docs/ENV_MANAGEMENT.md
- examples/progress_demo.py
- tests/test_llm_router.py
- cortex/installation_history.py
- cortex/coordinator.py
- cortex/progress_tracker.py
- tests/test_logging_system.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cli.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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_env_manager.pytest_parallel_llm.pytests/unit/test_progress_tracker.pycortex/installation_verifier.pycortex/env_manager.pycortex/network_config.pycortex/first_run_wizard.pycortex/cli.pytests/test_network_config.pycortex/transaction_history.pyexamples/env_demo.pyexamples/parallel_llm_demo.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.pytests/unit/test_progress_tracker.pytests/test_network_config.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
cortex/installation_verifier.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/first_run_wizard.py
🧬 Code graph analysis (6)
tests/test_env_manager.py (1)
cortex/env_manager.py (36)
EncryptionManager(454-542)EnvironmentManager(665-1105)EnvironmentStorage(545-662)EnvironmentTemplate(102-125)EnvironmentValidator(372-451)EnvironmentVariable(58-86)TemplateVariable(90-98)ValidationResult(129-134)VariableType(46-54)get_env_manager(1108-1110)to_dict(67-75)to_dict(109-115)from_dict(78-86)from_dict(118-125)validate(392-451)encrypt(502-514)decrypt(516-534)is_key_available(536-542)save(597-631)load(571-595)delete_app(633-648)list_apps(650-662)list_apps(828-835)set_variable(689-738)get_variable(740-767)get_variable_info(769-781)list_variables(783-794)delete_variable(796-814)clear_app(816-826)export_env(837-870)import_env(872-932)load_to_environ(934-956)list_templates(960-967)get_template(969-979)apply_template(981-1061)validate_app(1063-1105)
test_parallel_llm.py (1)
cortex/llm_router.py (8)
LLMRouter(75-695)TaskType(32-42)check_hardware_configs_parallel(828-876)diagnose_errors_parallel(779-825)query_multiple_packages(729-776)acomplete(445-509)complete_batch(614-695)set_rate_limit(436-443)
cortex/env_manager.py (1)
tests/test_env_manager.py (1)
storage(51-53)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_validator.py (2)
validate_anthropic_api_key(10-28)validate_openai_api_key(34-48)
tests/test_network_config.py (1)
cortex/network_config.py (19)
add_proxy_auth(586-613)check_proxy_auth(544-570)prompt_proxy_credentials(573-583)_detect_env_proxy(129-136)_detect_gnome_proxy(138-180)_detect_system_proxy(182-214)_parse_proxy_url(216-218)detect_vpn(222-253)check_connectivity(257-289)detect_network_quality(291-307)configure_apt_proxy(311-347)configure_pip_proxy(349-362)get_httpx_proxy_config(364-384)cleanup_apt_proxy(460-477)cache_package_list(388-408)get_cached_packages(410-443)enable_offline_fallback(445-458)auto_configure(479-516)print_summary(520-538)
examples/parallel_llm_demo.py (1)
cortex/llm_router.py (1)
query_multiple_packages(729-776)
🪛 markdownlint-cli2 (0.18.1)
docs/NETWORK_CONFIG.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
281-281: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
316-316: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
321-321: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/NETWORK_CONFIG_TESTS.md
30-30: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (12)
tests/unit/test_progress_tracker.py (1)
491-491: LGTM! PEP 8 formatting improvement.The addition of spaces around the
+operator improves PEP 8 compliance and aligns the test expectations with the updated progress tracker formatting.cortex/transaction_history.py (1)
359-361: LGTM!The consolidation of the rollback warning message into a single f-string improves readability without changing behavior.
test_parallel_llm.py (1)
1-26: New test script for parallel LLM calls looks well-structured.The script provides good coverage of async completion, batch processing, rate limiting, and helper functions. The API key checks before running tests and the summary at the end are helpful.
As per coding guidelines, consider adding docstrings to all public functions (currently only brief docstrings are present).
cortex/env_manager.py (2)
612-631: Atomic write implementation is well done.Using
tempfile.mkstempfollowed byos.replaceensures atomic updates to app environment files. Good defensive programming with cleanup in the except block.
1-12: Comprehensive environment management module with solid design.The module provides well-structured components for per-app environment storage, encryption, validation, and templating. The lazy loading of cryptography and atomic file operations are good practices.
Consider adding an
__all__export list to explicitly define the public API surface.examples/env_demo.py (2)
25-26: Path manipulation for example script is acceptable.The
sys.path.insertto enable imports from the parent directory is a common pattern for example scripts that aren't installed as packages.
378-427: Well-structured demo with proper cleanup.The demo uses a temporary directory for storage, preventing any persistent side effects. The cleanup of test environment variables in
demo_load_to_environis good practice. The Rich-based UI provides clear, readable output.cortex/installation_verifier.py (1)
311-313: LGTM!The partial verification message consolidation is a minor cleanup with no functional change.
docs/NETWORK_CONFIG_TESTS.md (1)
1-31: Comprehensive test documentation with clear structure.The documentation provides excellent coverage of the test suite organization, individual test descriptions, and guidance for running tests and adding new ones. The tables are well-formatted and informative.
cortex/cli.py (2)
868-1224: Env subcommand wiring and error handling look solid.The new
enventrypoint and helpers (_env_set/_env_get/_env_list/_env_delete/_env_export/_env_import/_env_clear/_env_template*,_env_list_apps,_env_load) are consistently typed, respectEnvironmentManager’s API, and handle common error conditions (ValueError/OSError, missing files, decryption failures) without crashing the CLI. The non‑zero exit codes on failure paths are appropriate.
1285-1307: Network auto‑config is safely best‑effort.Creating
NetworkConfig(auto_detect=False)and only callingdetect(check_quality=True)/auto_configure()for network‑sensitive commands, all wrapped in a broadtry/exceptthat only warns on failure, is a good compromise between functionality and startup robustness. This shouldn’t block the CLI even if detection fails.tests/test_network_config.py (1)
26-691: Comprehensive coverage ofNetworkConfigbehaviors.This suite exercises all major paths in
NetworkConfigand its helpers: proxy/VPN detection, connectivity and quality checks, apt/pip/httpx configuration, caching/offline behavior, and integration flows. The mocking ofsubprocess,socket,requests, and filesystem access matches the implementation details incortex/network_config.py, so regressions here should be caught early.
| ## Related Documentation | ||
|
|
||
| - [Network Configuration Module](./modules/network_config.md) | ||
| - [Graceful Degradation](./GRACEFUL_DEGRADATION.md) | ||
| - [Troubleshooting](./TROUBLESHOOTING.md) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the referenced documentation path exists
fd -t f "network_config.md" docs/Repository: cortexlinux/cortex
Length of output: 84
🏁 Script executed:
# Also search for NETWORK_CONFIG files
fd -t f "NETWORK_CONFIG" docs/Repository: cortexlinux/cortex
Length of output: 113
🏁 Script executed:
# Search for references to these paths in the documentation
rg "network_config|NETWORK_CONFIG" docs/ -t mdRepository: cortexlinux/cortex
Length of output: 1503
Fix the broken documentation link.
The link to ./modules/network_config.md references a non-existent path. The correct reference is ./NETWORK_CONFIG.md (same level as this file). This is confirmed by the early reference in the same file that correctly uses ./NETWORK_CONFIG.md.
🤖 Prompt for AI Agents
In docs/NETWORK_CONFIG_TESTS.md around lines 225 to 229, the "Network
Configuration Module" link points to ./modules/network_config.md which doesn't
exist; update that link to ./NETWORK_CONFIG.md (same directory as this file) so
it matches the correct earlier reference and resolves the broken link.
| responses = await query_multiple_packages(router, packages, max_concurrent=5) | ||
| responses = await query_multiple_packages(router, packages, max_concurrent=5) |
There was a problem hiding this comment.
Duplicate async call wastes API quota and skews timing measurements.
The same query_multiple_packages call is executed twice in sequence—only the second result is used while the first is discarded. This doubles API costs and inflates the measured elapsed time, undermining the demo's purpose of showing parallel speedup.
🔎 Proposed fix
- responses = await query_multiple_packages(router, packages, max_concurrent=5)
responses = await query_multiple_packages(router, packages, max_concurrent=5)📝 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.
| responses = await query_multiple_packages(router, packages, max_concurrent=5) | |
| responses = await query_multiple_packages(router, packages, max_concurrent=5) | |
| responses = await query_multiple_packages(router, packages, max_concurrent=5) |
🤖 Prompt for AI Agents
In examples/parallel_llm_demo.py around lines 40-41, there's a duplicated await
of query_multiple_packages that calls the API twice but only uses the second
result; remove the redundant call so the function is awaited only once (or if
you intended two runs, store results separately and measure each run
independently), and update any timing/elapsed calculation to reflect the single
intended call to avoid doubled API usage and incorrect speed measurements.
requirements.txt
Outdated
| # Encryption for environment variable secrets | ||
| cryptography>=42.0.0 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n requirements.txtRepository: cortexlinux/cortex
Length of output: 647
🌐 Web query:
cryptography library latest version 2024 2025
💡 Result:
If you mean the Python "cryptography" (pyca/cryptography) package:
- Latest (current) release: v46.0.3 — released Oct 15, 2025. [1][2]
- Key 2025 releases: v45.0.0 (May 17, 2025), v46.0.0 (Sep 16, 2025). [2]
- Key 2024 release: v44.0.0 — released Nov 27, 2024. [2]
Sources: PyPI release page for cryptography [1]; release history summary. [2]
References
[1] PyPI: cryptography (release files & dates).
[2] Release history listing for cryptography (versions/dates).
Cryptography dependency is appropriate, but requirements.txt has conflicting PyYAML entries.
The cryptography>=42.0.0 dependency supports Fernet-based encryption. Version 42.0.0 (Dec 2023) is a reasonable minimum that allows for newer patch and minor releases (current is v46.0.3).
However, requirements.txt contains three conflicting PyYAML entries that must be consolidated:
- Line 9:
PyYAML>=6.0.0 - Line 21:
pyyaml>=6.0.0(lowercase casing) - Line 25:
PyYAML==6.0.3(pinned version)
Remove duplicates and resolve the version constraint conflict (decide between >=6.0.0 or ==6.0.3).
🤖 Prompt for AI Agents
In requirements.txt around lines 14 to 16, consolidate the conflicting PyYAML
entries: remove the duplicate lines (the lowercase "pyyaml>=6.0.0" and the other
"PyYAML>=6.0.0") and keep a single, consistently-cased entry for PyYAML pinned
to a specific version for reproducibility (replace remaining entry with
"PyYAML==6.0.3"), or if you prefer loose constraints use one entry
"PyYAML>=6.0.0"—but do not keep multiple entries or differing casings.
62abc66 to
6dbe41d
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
examples/parallel_llm_demo.py (1)
40-41: Duplicate async call wastes API quota and skews timing measurements.The same
query_multiple_packagescall is executed twice in sequence—only the second result is used while the first is discarded. Remove line 40 to execute the function once.🔎 Proposed fix
- responses = await query_multiple_packages(router, packages, max_concurrent=5) responses = await query_multiple_packages(router, packages, max_concurrent=5)cortex/cli.py (1)
66-81: Provider/key mismatch risk remains from previous review.The fallback logic at lines 73-76 can still return
"ollama-local"while_get_provider()returns"openai"or"claude"whenCORTEX_PROVIDERis explicitly set but the corresponding API key is missing. This causes the mismatch flagged in the previous review.Consider restricting the ollama-local fallback to only when
provider == "ollama"(remove theor not (...)clause) to ensure provider and key remain consistent.cortex/first_run_wizard.py (1)
21-22: Remove unused API key validator imports.The
validate_anthropic_api_keyandvalidate_openai_api_keyfunctions are imported but never called. The wizard only performs prefix validation (is_valid_api_keychecksstartswith) without verifying that keys actually work with the respective APIs.Either remove these unused imports or integrate them into the key validation flow (e.g., in
_prompt_for_api_keyor during the dry-run verification) to catch invalid keys earlier.🔎 Remove unused imports
-# Import API key validation utilities -from cortex.utils.api_key_validator import validate_anthropic_api_key, validate_openai_api_key -
🧹 Nitpick comments (4)
test_parallel_llm.py (1)
29-252: Consider pytest integration if this becomes part of the test suite.The test functions return booleans and print results, which works for standalone execution but doesn't integrate with pytest's reporting. If you move this to
tests/, consider:
- Using
assertstatements instead of returning booleans- Using
pytest.skip()for missing API keys instead of conditional returns- Leveraging pytest fixtures for shared setup (router initialization)
cortex/utils/api_key_validator.py (1)
10-48: API key validation makes real API calls—ensure users understand the implications.Both validation functions make actual HTTP requests to provider APIs. This is appropriate for validation but means:
- Validation has latency (10s timeout per provider)
- Invalid keys consume network resources
- Validation may fail due to network issues, not just invalid keys
Consider documenting this behavior if these functions are called frequently or in performance-sensitive paths.
cortex/cli.py (1)
869-871: Remove redundant import.
sysis already imported at line 4. The re-import at line 871 is unnecessary.🔎 Proposed fix
def env(self, args: argparse.Namespace) -> int: """Handle environment variable management commands.""" - import sys - env_mgr = get_env_manager()cortex/first_run_wizard.py (1)
463-561: Consider extracting common provider setup logic.The Anthropic (lines 463-512) and OpenAI (lines 513-561) setup blocks contain nearly identical logic:
- Check for existing key via
get_valid_api_key()- Prompt to replace or keep existing key
- Prompt for new key if missing
- Save to
.envor shell config fallback- Run dry-run verification
Consider extracting a helper method like
_setup_api_provider(provider_name, env_var, key_type, forced_provider)to reduce ~90 lines of duplication and improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.env.examplecortex/cli.pycortex/env_loader.pycortex/first_run_wizard.pycortex/utils/api_key_validator.pyexamples/env_demo.pyexamples/parallel_llm_demo.pytest_parallel_llm.pytests/installer/test_parallel_install.pytests/test_cli.pytests/test_cli_extended.pytests/test_env_manager.pytests/test_first_run_wizard.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_cli.py
- tests/test_env_manager.py
- tests/installer/test_parallel_install.py
🧰 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:
cortex/env_loader.pytests/test_first_run_wizard.pytest_parallel_llm.pytests/test_cli_extended.pycortex/cli.pycortex/utils/api_key_validator.pycortex/first_run_wizard.pyexamples/env_demo.pyexamples/parallel_llm_demo.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_first_run_wizard.pytests/test_cli_extended.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/first_run_wizard.py
🧬 Code graph analysis (5)
tests/test_first_run_wizard.py (1)
cortex/first_run_wizard.py (1)
_step_api_setup(640-701)
test_parallel_llm.py (1)
cortex/llm_router.py (6)
LLMRouter(75-695)TaskType(32-42)check_hardware_configs_parallel(828-876)diagnose_errors_parallel(779-825)query_multiple_packages(729-776)complete_batch(614-695)
tests/test_cli_extended.py (2)
tests/test_cli.py (2)
test_get_api_key_not_found(31-34)test_install_no_api_key(62-64)cortex/cli.py (2)
_get_api_key(66-81)install(347-610)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_validator.py (2)
validate_anthropic_api_key(10-28)validate_openai_api_key(34-48)
examples/parallel_llm_demo.py (1)
cortex/llm_router.py (1)
query_multiple_packages(729-776)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
🪛 GitHub Actions: CI
examples/env_demo.py
[error] 1-1: Ruff check failed: I001 Import block is un-sorted or un-formatted.
🪛 GitHub Check: lint
examples/env_demo.py
[failure] 1-1: Ruff (I001)
examples/env_demo.py:1:1: I001 Import block is un-sorted or un-formatted
examples/parallel_llm_demo.py
[failure] 218-218: Ruff (W293)
examples/parallel_llm_demo.py:218:1: W293 Blank line contains whitespace
[failure] 216-216: Ruff (W293)
examples/parallel_llm_demo.py:216:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
examples/env_demo.py
[failure] 1-1: Ruff (I001)
examples/env_demo.py:1:1: I001 Import block is un-sorted or un-formatted
examples/parallel_llm_demo.py
[failure] 218-218: Ruff (W293)
examples/parallel_llm_demo.py:218:1: W293 Blank line contains whitespace
[failure] 216-216: Ruff (W293)
examples/parallel_llm_demo.py:216:1: W293 Blank line contains whitespace
🪛 Gitleaks (8.30.0)
tests/test_first_run_wizard.py
[high] 269-269: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (9)
cortex/env_loader.py (1)
32-56: Expanded .env search locations improve environment-driven configuration.The addition of parent directory and cortex package directory search locations, along with safe ImportError handling, aligns well with the PR's environment-driven configuration goals.
.env.example (1)
1-5: Example .env file supports environment-driven configuration.The sample environment file clearly demonstrates the expected API key format and aligns with the PR's environment-driven configuration approach.
tests/test_first_run_wizard.py (1)
261-287: Test coverage expanded for multi-provider API key detection.The addition of
test_step_api_setup_existing_openai_keyand the update totest_step_api_setup_no_keyproperly test the environment-driven provider detection logic introduced in this PR.Note: The Gitleaks warning on line 269 is a false positive—this is a test fixture, not a real secret.
tests/test_cli_extended.py (2)
33-83: Test updates properly reflect environment-driven API key resolution.The updates to test expectations (returning
"ollama-local"instead ofNone, and returning0instead of1for install without explicit API keys) correctly align with the new provider-aware fallback logic introduced in this PR.
85-125: Environment variable patches support new configuration flow.The addition of
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key"}, clear=True)decorators throughout these tests properly isolates test execution and validates the environment-driven provider selection logic.cortex/cli.py (3)
50-64: Provider-specific API key retrieval with format validation.The new
_get_api_key_for_providermethod cleanly separates provider-specific key logic and includes defensive format validation for Claude and OpenAI keys.
374-420: Provider probing with fallback improves resilience.The provider probing loop with fallback to alternative providers (lines 387-413) enhances resilience when one provider fails. The exception handling now properly catches all exceptions, addressing the previous review concern.
869-1218: Comprehensive environment variable management implementation.The new
envmethod with its extensive subcommand surface (set/get/list/delete/export/import/clear/template/apps/load) is well-structured with clean separation of concerns via helper methods. Error handling is consistent throughout.cortex/utils/api_key_validator.py (1)
10-28: Add type hint for return value.Per coding guidelines, type hints are required in Python code. Add
-> boolto the function signature.🔎 Proposed fix
-def validate_anthropic_api_key(api_key: str) -> bool: +def validate_anthropic_api_key(api_key: str) -> bool:Wait, the signature already has
-> boolbased on the AI summary. Let me check the actual code...Looking at the annotated code, line 10 shows:
def validate_anthropic_api_key(api_key: str) -> bool:This already has the type hint. I should skip this comment.
Likely an incorrect or invalid review comment.
| from __future__ import annotations | ||
| #!/usr/bin/env python3 |
There was a problem hiding this comment.
Fix the import ordering: shebang must be first.
The from __future__ import annotations import is placed before the shebang, which violates Python syntax requirements and PEP 8. The shebang (#!/usr/bin/env python3) must be the absolute first line of the file, followed by the module docstring, then from __future__ imports, then other imports.
This is causing the CI pipeline to fail with Ruff I001 error.
🔎 Proposed fix to correct the import ordering
-from __future__ import annotations
-
#!/usr/bin/env python3
"""
Environment Variable Manager Demo
This script demonstrates the environment variable management features
of Cortex, including:
- Setting and getting variables
- Encrypting sensitive values
- Using templates
- Importing and exporting
- Variable validation
Run this script to see the env manager in action:
python examples/env_demo.py
"""
+from __future__ import annotations
+
import os
import sys
import tempfile🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Ruff check failed: I001 Import block is un-sorted or un-formatted.
🪛 GitHub Check: lint
[failure] 1-1: Ruff (I001)
examples/env_demo.py:1:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
[failure] 1-1: Ruff (I001)
examples/env_demo.py:1:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In examples/env_demo.py around lines 1-2, the shebang and future import are
reversed causing a Ruff I001 error; move the shebang (#!/usr/bin/env python3) to
be the very first line of the file, then place any module docstring (if
present), then the from __future__ import annotations line, followed by the rest
of imports so the file follows Python syntax and PEP 8 ordering.
| #!/usr/bin/env python3 | ||
| """ | ||
| Quick test script to verify parallel LLM calls are working. | ||
| Run this to test: | ||
| 1. Async completion works | ||
| 2. Batch processing works | ||
| 3. Rate limiting works | ||
| 4. Helper functions work | ||
| """ | ||
|
|
||
| import asyncio | ||
| import os | ||
| import sys | ||
| import time | ||
|
|
||
| # Add parent directory to path | ||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".")) | ||
|
|
||
| from cortex.llm_router import ( | ||
| LLMRouter, | ||
| TaskType, | ||
| check_hardware_configs_parallel, | ||
| diagnose_errors_parallel, | ||
| query_multiple_packages, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify file location: test suite vs. standalone demo.
This file is located at the repository root, which is unconventional for test files. Consider:
- If this is part of the test suite, move it to
tests/test_parallel_llm.pyand integrate with pytest. - If this is a standalone demo or verification script, move it to
examples/orscripts/.
The current location may cause confusion about its purpose and whether it's executed by CI.
🤖 Prompt for AI Agents
In test_parallel_llm.py lines 1-27: the file sits at the repo root which
confuses its intent (test vs demo) and may be picked up by CI; if this is a test
move it to tests/test_parallel_llm.py and convert it to a pytest-compatible test
(use assertions, fixtures, and avoid top-level execution), otherwise move it to
examples/ or scripts/ (e.g., examples/parallel_llm_demo.py) and keep its
executable header and README usage notes; update imports/paths if moved and
adjust project config (pytest.ini or CI) to include or exclude the new location
accordingly.
- The wizard command was only showing a static message instead of running - Now properly calls FirstRunWizard.run() for full interactive setup - Wizard detects existing API keys and proceeds through all setup steps
There was a problem hiding this comment.
@jaysurse Kindle Updated your branch and merge with main,.
| @@ -1,3 +1,4 @@ | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Remove this comments.
|
|
||
| OPENAI_API_KEY=sk-your-openai-key-here | ||
| ANTHROPIC_API_KEY=sk-ant-your-anthropic-key-here | ||
| >>>>>>> 34c66df (feat: seamless onboarding—auto-create .env from .env.example, improved wizard flow) |
There was a problem hiding this comment.
remove this one as well.
Resolved conflicts in: - cortex/cli.py: kept improved _get_api_key_for_provider method - tests/test_cli_extended.py: kept simplified test matching new implementation
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @cortex/cli.py:
- Around line 972-973: Remove the redundant local "import sys" statement in
cortex/cli.py (the duplicate import of the sys module found inside the
function/block near the CLI logic); sys is already imported at module top, so
delete that local import to avoid duplication and rely on the module-level sys
import instead.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1-1: Fix Black formatting to pass CI.The pipeline indicates Black formatting check failed. Run
black .to reformat the code before merging.
♻️ Duplicate comments (1)
cortex/cli.py (1)
63-78: Provider/key mismatch remains a concern forask()command.As noted in a past review, when
CORTEX_PROVIDER=openaiis set butOPENAI_API_KEYis missing, this returns"ollama-local"while_get_provider()still returns"openai". This causes a mismatch in theask()command flow (lines 532-561) where both values are passed toAskHandler.The
install()method now uses_get_api_key_for_provider()directly, mitigating the issue there, butask()still relies on this potentially mismatched pair.Consider either:
- Making
_get_api_key()returnNonefor non-ollama providers without valid keys (letting the error path handle it), or- Updating
ask()to use_get_api_key_for_provider()directly likeinstall()does.
🧹 Nitpick comments (2)
cortex/cli.py (2)
598-609: Unusedprovidervariable and questionable"dummy-key"fallback.
Line 599: The
providervariable is assigned but never used after the loop. The# noqa: F841suggests awareness, but consider removing the assignment if truly unused.Line 605: Using
"dummy-key"when_get_api_key_for_providerreturnsNonewill cause authentication failures with cloud providers. While the exception handler catches this and moves to the next provider, it's inefficient and produces unnecessary API errors.Consider skipping providers with no valid key:
🔎 Proposed fix
for try_provider in providers_to_try: try: - try_api_key = self._get_api_key_for_provider(try_provider) or "dummy-key" + try_api_key = self._get_api_key_for_provider(try_provider) + if not try_api_key: + self._debug(f"No API key for {try_provider}, skipping") + continue self._debug(f"Trying provider: {try_provider}")
627-629: Exception handling is functional but could be more specific.Catching
(RuntimeError, Exception)is redundant sinceExceptionincludesRuntimeError. The broad catch allows fallback on any error but may hide unexpected issues. Consider explicitly listing expected exceptions:except (ValueError, RuntimeError, OSError) as e:This is a minor style suggestion; the current implementation works correctly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pytests/test_cli.pytests/test_cli_extended.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cli.py
🧰 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_cli_extended.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli_extended.py
🧬 Code graph analysis (2)
tests/test_cli_extended.py (2)
tests/test_cli.py (1)
test_get_api_key_not_found(40-48)cortex/cli.py (2)
_get_api_key(63-78)install(563-826)
cortex/cli.py (3)
cortex/first_run_wizard.py (2)
FirstRunWizard(268-875)run(389-599)cortex/llm/interpreter.py (2)
CommandInterpreter(18-388)parse(320-378)cortex/branding.py (1)
cx_print(49-69)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed: 1 file would be reformatted. Run 'black .' to format.
⏰ 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)
- GitHub Check: Agent
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (8)
tests/test_cli_extended.py (4)
43-46: LGTM! Simplified test aligns with provider-first resolution.The test now correctly uses
CORTEX_PROVIDER=ollamato bypass API key detection, which matches the updated_get_api_keylogic that returns"ollama-local"when provider is explicitly set toollama. The removal ofPath.homepatching and user input simulation is appropriate since the explicit provider takes precedence.
95-102: LGTM! Test reflects new fallback behavior.The updated test correctly expects
install()to succeed (return 0) whenCommandInterpreterreturns valid commands, even without an explicit API key. This aligns with the new multi-provider probing logic that falls back toollama-localwhen no cloud provider keys are available.
104-144: LGTM! Environment-driven test setup is consistent.The addition of
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key"}, clear=True)to these tests properly aligns with the environment-driven API key resolution. This approach is more realistic than mocking internal methods and matches the production behavior.
144-144: Good addition of parse assertion.Adding the
assert_called_once_with("install docker")assertion verifies that the interpreter receives the expected input, improving test coverage and consistency withtest_install_dry_run.cortex/cli.py (4)
21-21: LGTM!The
FirstRunWizardimport is correctly placed with other cortex module imports and is necessary for the updated wizard functionality.
47-61: LGTM with minor observation on key validation.The provider-specific key retrieval logic is well-structured. The prefix validation (
sk-ant-for Anthropic,sk-for OpenAI) provides basic key format checking.Note: OpenAI keys may use various prefixes (e.g.,
sk-proj-), but the currentsk-check covers these cases adequately.
961-968: LGTM! Clean wizard delegation.The wizard method now properly delegates to
FirstRunWizardwithinteractive=True, returning appropriate exit codes. This aligns with the PR objective to improve the first-time user experience.
1043-1043: LGTM!The simplified install instruction for cryptography is cleaner and more direct.
| import sys | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant import sys.
sys is already imported at the module level (line 4). This local import is unnecessary.
🔎 Proposed fix
def env(self, args: argparse.Namespace) -> int:
"""Handle environment variable management commands."""
- import sys
-
env_mgr = get_env_manager()🤖 Prompt for AI Agents
In @cortex/cli.py around lines 972-973, Remove the redundant local "import sys"
statement in cortex/cli.py (the duplicate import of the sys module found inside
the function/block near the CLI logic); sys is already imported at module top,
so delete that local import to avoid duplication and rely on the module-level
sys import instead.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #355 by fixing the wizard logic to detect and honor API keys already present in environment variables or .env files. The wizard now allows setup to continue instead of exiting prematurely when a valid key is detected, improving the first-time user experience.
Key Changes
- Enhanced API key detection logic that checks both
.envfiles and shell-exported environment variables - Refactored wizard flow to offer provider selection with indicators for detected keys
- Added comprehensive test coverage for API key validation and environment variable priority handling
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/first_run_wizard.py | Major refactoring of wizard flow with new API key detection, validation, and environment file handling functions |
| cortex/cli.py | Updated provider fallback logic and wizard integration; modified env import return behavior |
| cortex/utils/api_key_validator.py | New file with API validation utilities for Anthropic and OpenAI keys |
| cortex/env_loader.py | Extended .env file search locations to include parent directory and package directory |
| tests/test_first_run_wizard.py | Added comprehensive tests for API key validation, environment variable priority, and wizard flow |
| tests/test_cli.py | Modified test to use updated API key detection approach |
| tests/test_cli_extended.py | Simplified tests by removing redundant mocks and using environment variables directly |
| tests/installer/test_parallel_install.py | Updated Python command from 'python' to 'python3' for better compatibility |
| test_parallel_llm.py | New test file for parallel LLM functionality verification |
| examples/parallel_llm_demo.py | Contains duplicate line that needs to be removed |
| examples/env_demo.py | Moved future import to top of file per PEP 8 |
| .env.example | Contains unresolved merge conflict markers |
| cortex/llm/interpreter.py | Minor formatting improvement (added line break for readability) |
| cortex/dependency_check.py | Minor formatting improvement (standardized string joining) |
| tests/test_env_manager.py | Updated URL validation test (sftp→ftp) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cortex/first_run_wizard.py
Outdated
| self.config["api_provider"] = "anthropic" | ||
| self.config["api_key_configured"] = True | ||
|
|
||
| random_example = random.choice(DRY_RUN_EXAMPLES) # noqa: S311 |
There was a problem hiding this comment.
The noqa comment "S311" is used to suppress the security warning about using random.choice for cryptographic purposes. However, this is not a security-sensitive context (just selecting a random example for display), so the comment is appropriate. For consistency with Python security best practices, consider using secrets.choice() instead if this code might be used in any security-sensitive context in the future, or document why random.choice is sufficient here.
| providers_to_try.append(other_provider) | ||
|
|
||
| commands = None | ||
| provider = None # noqa: F841 - assigned in loop |
There was a problem hiding this comment.
The noqa comment states "assigned in loop", but the variable 'provider' is initialized to None and assigned in the loop at line 622. However, if the loop completes without finding any valid commands, 'provider' remains None and is never used afterward. The noqa comment appears to be suppressing a false positive from the linter. Consider removing this variable initialization entirely since it's not used outside the loop - only try_provider is needed.
cortex/first_run_wizard.py
Outdated
| key_from_env = os.environ.get(env_var) | ||
| if key_from_env is not None and len(key_from_env.strip()) > 0: | ||
| key_from_env = key_from_env.strip() |
There was a problem hiding this comment.
Redundant condition. Line 168 checks 'key_from_env is not None and len(key_from_env.strip()) > 0', but this could be simplified. Since you're immediately stripping the value on line 169, you could combine these operations: 'key_from_env = os.environ.get(env_var, "").strip()' and then just check 'if key_from_env:'.
| key_from_env = os.environ.get(env_var) | |
| if key_from_env is not None and len(key_from_env.strip()) > 0: | |
| key_from_env = key_from_env.strip() | |
| key_from_env = os.environ.get(env_var, "").strip() | |
| if key_from_env: |
cortex/first_run_wizard.py
Outdated
| from typing import Any | ||
|
|
||
| # Import API key validation utilities | ||
| from cortex.utils.api_key_validator import validate_anthropic_api_key, validate_openai_api_key |
There was a problem hiding this comment.
The API key validation utilities validate_anthropic_api_key and validate_openai_api_key are imported but never used in this file. The code uses a simpler format-based validation (checking prefixes like 'sk-ant-' and 'sk-') instead of making actual API calls to validate the keys. Either remove these unused imports or update the validation logic to use these functions for more robust validation.
| self._print_error(str(e)) | ||
| if "cryptography" in str(e).lower(): | ||
| cx_print("Install with: pip install cryptography", "info") | ||
| cx_print("Install with: pip install cryptography", "info") |
There was a problem hiding this comment.
The removed conditional check 'if "cryptography" in str(e).lower()' was previously used to provide helpful context only when the error was specifically about the cryptography library. Now the "Install with: pip install cryptography" message is shown for ALL ImportError exceptions, even if they're unrelated to cryptography. This could mislead users. Consider restoring the conditional check to only show the installation hint when it's actually relevant.
| cx_print("Install with: pip install cryptography", "info") | |
| if "cryptography" in str(e).lower(): | |
| cx_print("Install with: pip install cryptography", "info") |
|
|
||
| print(f"Processing {len(requests)} requests with max_concurrent=2...") | ||
| start = time.time() | ||
| responses = await router.complete_batch(requests, max_concurrent=2) |
There was a problem hiding this comment.
Variable responses is not used.
| start_time = time.time() | ||
|
|
||
| responses = await query_multiple_packages(router, packages, max_concurrent=5) | ||
| responses = await query_multiple_packages(router, packages, max_concurrent=5) |
|
|
||
| cortex_dir = Path(cortex.__file__).parent / ".env" | ||
| locations.append(cortex_dir) | ||
| except ImportError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except ImportError: | |
| except ImportError: | |
| # The cortex package may not be installed; skip its optional .env location. |
cortex/first_run_wizard.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as e: | |
| logger.warning(f"Error reading existing .env file at {env_path}: {e}") |
| # Load .env file but don't override existing shell-exported keys | ||
| # This preserves keys set via `export ANTHROPIC_API_KEY=xxx` | ||
| load_dotenv(dotenv_path=env_path, override=False) | ||
| except ImportError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except ImportError: | |
| except ImportError: | |
| # python-dotenv is optional; if it's not installed, we simply | |
| # skip loading from .env and rely on existing environment vars. |
- Apply black formatting to cli.py - Fix test_no_key_found to also patch Path.cwd() to prevent finding local .env file
|
@Suyashd999 @jaysurse Not needed now, See for more info: #355 (comment) |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jaysurse Kindly resolve conflicts.
- Add backward compatibility functions to api_key_detector - Update wizard to allow reconfiguration after setup - Fix wizard to continue flow when keys are found - Update tests for proper mocking
There was a problem hiding this comment.
@jaysurse CI checks failing.Kindly complete your CLA verification, making it draft until then.
- Rename test_install_no_api_key to test_install_with_openai_key - Fix duplicate function name causing CI lint failure
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_env_manager.py (1)
156-168: Confirm the intended set of “valid URL” schemes (sftp vs ftps).This test change tightens/changes accepted schemes; make sure
EnvironmentValidator.validate(..., "url")intentionally allowsftps://(and whethersftp://should remain valid or be explicitly rejected/untested).tests/test_cli.py (1)
83-120: Fix CI blocker: duplicate test name (Ruff F811) + invalidmonkeypatchusage inunittest.TestCase.Proposed fix
@@ @patch.dict(os.environ, {}, clear=True) @patch("cortex.cli.CommandInterpreter") def test_install_no_api_key(self, mock_interpreter_class): @@ self.assertEqual(result, 0) - @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-openai-key-123"}, clear=True) - @patch("cortex.cli.CommandInterpreter") - def test_install_no_api_key(self, mock_interpreter_class): - # Should work with Ollama (no API key needed) - mock_interpreter = Mock() - mock_interpreter.parse.return_value = ["apt update", "apt install docker"] - mock_interpreter_class.return_value = mock_interpreter - - result = self.cli.install("docker") - # Should succeed with Ollama as fallback provider - self.assertEqual(result, 0) - - @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-openai-key-123"}, clear=True) - @patch("cortex.cli.CommandInterpreter") - def test_install_dry_run(self, monkeypatch): - monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-key-123") - with patch("cortex.cli.CommandInterpreter") as mock_interpreter_class: - mock_interpreter = Mock() - mock_interpreter.parse.return_value = ["apt update", "apt install docker"] - mock_interpreter_class.return_value = mock_interpreter - - result = self.cli.install("docker", dry_run=True) - - self.assertEqual(result, 0) - mock_interpreter.parse.assert_called_once_with("install docker") + @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-openai-key-123"}, clear=True) + @patch("cortex.cli.CommandInterpreter") + def test_install_dry_run(self, mock_interpreter_class): + mock_interpreter = Mock() + mock_interpreter.parse.return_value = ["apt update", "apt install docker"] + mock_interpreter_class.return_value = mock_interpreter + + result = self.cli.install("docker", dry_run=True) + + self.assertEqual(result, 0) + mock_interpreter.parse.assert_called_once_with("install docker")cortex/cli.py (1)
645-716: Don’t attempt API providers with placeholder keys; skip them when no real key exists.
try_api_key = ... or "dummy-key"means you’ll potentially call provider clients with invalid credentials and rely on exception control flow. It’s safer and faster to only attemptclaude/openaiwhen a real key exists (while still allowingollama/fake).Proposed fix
for try_provider in providers_to_try: try: - try_api_key = self._get_api_key_for_provider(try_provider) or "dummy-key" + try_api_key = self._get_api_key_for_provider(try_provider) + if try_provider not in ["ollama", "fake"] and not try_api_key: + self._debug(f"Skipping provider {try_provider}: no API key configured") + continue self._debug(f"Trying provider: {try_provider}") interpreter = CommandInterpreter(api_key=try_api_key, provider=try_provider)
🤖 Fix all issues with AI agents
In @cortex/first_run_wizard.py:
- Around line 223-260: The detect_available_providers function currently returns
early after get_detected_provider(), which can miss other valid providers;
remove the early return and instead always aggregate providers by checking
detect_api_key(provider) (for each SUPPORTED_PROVIDERS) and/or
get_valid_api_key("ANTHROPIC_API_KEY","anthropic") and
get_valid_api_key("OPENAI_API_KEY","openai") so all valid providers are
collected, and still include shutil.which("ollama") if present; keep the
try/except around get_detected_provider() but merge its result into the
providers list without exiting early.
- Around line 105-153: save_key_to_env_file currently writes plaintext keys
directly and can produce partial/corrupted files and loose permissions; fix it
by performing an atomic write to a temp file in the same directory and then
atomically replacing the target with os.replace (or similar) after fsyncing the
temp file, and ensure the final .env file has strict permissions (chmod 0o600)
and the ~/.cortex directory is created with restrictive perms (0o700). Implement
this in save_key_to_env_file: create parent dir with mode 0o700, write new_lines
to a temporary file in that directory (fsync and close), os.replace temp →
env_path, then os.chmod(env_path, 0o600); catch and log OSErrors as before and
return False on failure.
In @tests/test_first_run_wizard.py:
- Around line 204-215: The test test_get_valid_api_key_no_key_anywhere currently
patches detect_api_key via "cortex.first_run_wizard.detect_api_key", which will
fail if the api_key_detector module isn't present; instead modify the test to
avoid relying on that import by using monkeypatch to set detect_api_key on the
first_run_wizard module with raising=False (e.g., import cortex.first_run_wizard
as frw and call monkeypatch.setattr(frw, "detect_api_key", lambda *a, **k: None,
raising=False)); keep mocking read_key_from_env_file as before and call
get_valid_api_key unchanged.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/api_key_detector.pycortex/cli.pycortex/first_run_wizard.pytests/test_cli.pytests/test_cli_extended.pytests/test_env_manager.pytests/test_first_run_wizard.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cli_extended.py
🧰 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_env_manager.pycortex/api_key_detector.pytests/test_first_run_wizard.pycortex/cli.pycortex/first_run_wizard.pytests/test_cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.pytests/test_first_run_wizard.pytests/test_cli.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/first_run_wizard.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
tests/test_cli.py
🧬 Code graph analysis (3)
cortex/cli.py (3)
cortex/first_run_wizard.py (2)
FirstRunWizard(340-978)run(670-764)cortex/installation_history.py (1)
InstallationHistory(74-632)cortex/llm/interpreter.py (2)
CommandInterpreter(20-373)parse(305-363)
cortex/first_run_wizard.py (2)
cortex/api_key_detector.py (2)
detect_api_key(698-711)get_detected_provider(714-722)cortex/env_manager.py (1)
get_env_manager(1108-1110)
tests/test_cli.py (1)
tests/test_cli_extended.py (2)
test_install_no_api_key(96-102)test_install_dry_run(110-125)
🪛 GitHub Actions: CI
tests/test_cli.py
[error] 97-97: F811 Redefinition of unused test_install_no_api_key from line 85: test_install_no_api_key redefined here
🪛 GitHub Check: lint
tests/test_cli.py
[failure] 97-97: Ruff (F811)
tests/test_cli.py:97:9: F811 Redefinition of unused test_install_no_api_key from line 85: test_install_no_api_key redefined here
🪛 GitHub Check: Lint
tests/test_cli.py
[failure] 97-97: Ruff (F811)
tests/test_cli.py:97:9: F811 Redefinition of unused test_install_no_api_key from line 85: test_install_no_api_key redefined here
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (4)
cortex/first_run_wizard.py (2)
564-591: Dry-run verification flow is a good UX improvement.
592-630: Keeping existing env-exported keys and continuing the flow matches the PR objective.tests/test_first_run_wizard.py (1)
13-24: Key-resolution test coverage is solid and matches the new priority rules.Also applies to: 134-216
cortex/cli.py (1)
1041-1048: Wizard delegation is clean and keeps CLI flow simple.
| # Convenience functions for backward compatibility | ||
| def detect_api_key(provider: str) -> str | None: | ||
| """ | ||
| Detect API key for a specific provider. | ||
| Args: | ||
| provider: The provider name ('anthropic', 'openai') | ||
| Returns: | ||
| The API key or None if not found | ||
| """ | ||
| found, key, detected_provider, source = auto_detect_api_key() | ||
| if found and detected_provider == provider: | ||
| return key | ||
| return None | ||
|
|
||
|
|
||
| def get_detected_provider() -> str | None: | ||
| """ | ||
| Get the detected provider name. | ||
| Returns: | ||
| The provider name or None if not detected | ||
| """ | ||
| found, key, provider, source = auto_detect_api_key() | ||
| return provider if found else None | ||
|
|
||
|
|
||
| SUPPORTED_PROVIDERS = ["anthropic", "openai", "ollama"] |
There was a problem hiding this comment.
detect_api_key() is not truly provider-scoped (and docstring disagrees with SUPPORTED_PROVIDERS).
Right now detect_api_key(provider) returns a key only if the globally-detected provider matches provider, which can hide other available keys (e.g., both keys present but only Anthropic is “detected”). Also the docstring doesn’t mention "ollama" even though it’s now “supported”.
Proposed direction (keep backward-compat but make semantics predictable)
def detect_api_key(provider: str) -> str | None:
@@
- found, key, detected_provider, source = auto_detect_api_key()
- if found and detected_provider == provider:
- return key
- return None
+ # Provider-scoped check: look at the specific env var first, then fall back to full detection.
+ provider = provider.lower().strip()
+ if provider == "anthropic":
+ key = os.environ.get("ANTHROPIC_API_KEY")
+ return key if key else None
+ if provider == "openai":
+ key = os.environ.get("OPENAI_API_KEY")
+ return key if key else None
+ if provider == "ollama":
+ # Presence-based; callers can treat "ollama-local" as configured.
+ return "ollama-local" if os.environ.get("CORTEX_PROVIDER", "").lower() == "ollama" else None
+
+ found, key, detected_provider, _source = auto_detect_api_key()
+ return key if found and detected_provider == provider else None(If you want this to search non-env locations per provider too, the clean solution is to add a real provider-scoped search inside APIKeyDetector rather than reusing the global detect() path.)
| def _get_api_key_for_provider(self, provider: str) -> str | None: | ||
| """Get API key for a specific provider.""" | ||
| if provider == "ollama": | ||
| return "ollama-local" | ||
| if provider == "fake": | ||
| return "fake-key" | ||
| if provider == "claude": | ||
| key = os.environ.get("ANTHROPIC_API_KEY") | ||
| if key and key.strip().startswith("sk-ant-"): | ||
| return key.strip() | ||
| elif provider == "openai": | ||
| key = os.environ.get("OPENAI_API_KEY") | ||
| if key and key.strip().startswith("sk-"): | ||
| return key.strip() | ||
| return None | ||
|
|
||
| # 2. Try auto-detection + prompt to save (setup_api_key handles both) | ||
| success, key, detected_provider = setup_api_key() | ||
| if success: | ||
| self._debug(f"Using {detected_provider} API key") | ||
| # Store detected provider so _get_provider can use it | ||
| self._detected_provider = detected_provider | ||
| def _get_api_key(self) -> str | None: | ||
| """Get API key for the current provider.""" | ||
| provider = self._get_provider() | ||
| key = self._get_api_key_for_provider(provider) | ||
| if key: | ||
| return key | ||
|
|
||
| # Still no key | ||
| self._print_error("No API key found or provided") | ||
| # If provider is ollama or no key is set, always fallback to ollama-local | ||
| if provider == "ollama" or not ( | ||
| os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY") | ||
| ): | ||
| return "ollama-local" | ||
| # Otherwise, prompt user for setup | ||
| self._print_error("No valid API key found.") | ||
| cx_print("Run [bold]cortex wizard[/bold] to configure your API key.", "info") | ||
| cx_print("Or use [bold]CORTEX_PROVIDER=ollama[/bold] for offline mode.", "info") | ||
| return None |
There was a problem hiding this comment.
Align key validation with wizard rules and avoid misclassifying Anthropic keys as OpenAI.
Proposed fix
def _get_api_key_for_provider(self, provider: str) -> str | None:
"""Get API key for a specific provider."""
@@
elif provider == "openai":
key = os.environ.get("OPENAI_API_KEY")
- if key and key.strip().startswith("sk-"):
+ if key and key.strip().startswith("sk-") and not key.strip().startswith("sk-ant-"):
return key.strip()
return None| def save_key_to_env_file(key_name: str, key_value: str) -> bool: | ||
| """Save an API key to the .env file. | ||
| Saves to ~/.cortex/.env which is checked by api_key_detector (priority #2). | ||
| Args: | ||
| key_name: The environment variable name. | ||
| key_value: The value to save. | ||
| Returns: | ||
| True if saved successfully, False otherwise. | ||
| """ | ||
| env_path = get_env_file_path() | ||
| env_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| lines: list[str] = [] | ||
| key_found = False | ||
|
|
||
| if env_path.exists(): | ||
| try: | ||
| with open(env_path) as f: | ||
| lines = f.readlines() | ||
| except OSError: | ||
| pass | ||
|
|
||
| new_lines: list[str] = [] | ||
| for line in lines: | ||
| stripped = line.strip() | ||
| if stripped and not stripped.startswith("#") and "=" in stripped: | ||
| existing_key = stripped.split("=")[0].strip() | ||
| if existing_key == key_name: | ||
| new_lines.append(f'{key_name}="{key_value}"\n') | ||
| key_found = True | ||
| continue | ||
| new_lines.append(line) | ||
|
|
||
| if not key_found: | ||
| if new_lines and not new_lines[-1].endswith("\n"): | ||
| new_lines.append("\n") | ||
| new_lines.append(f'{key_name}="{key_value}"\n') | ||
|
|
||
| try: | ||
| with open(env_path, "w") as f: | ||
| f.writelines(new_lines) | ||
| return True | ||
| except OSError as e: | ||
| logger.warning("Error saving to .env file: %s", e) | ||
| return False | ||
|
|
There was a problem hiding this comment.
Secure the ~/.cortex/.env write path (permissions + atomic write).
save_key_to_env_file() writes plaintext API keys but doesn’t enforce 0600 or atomic replace, so keys can be overexposed or the file can be corrupted on partial writes.
Proposed fix
def save_key_to_env_file(key_name: str, key_value: str) -> bool:
@@
- try:
- with open(env_path, "w") as f:
- f.writelines(new_lines)
- return True
+ try:
+ tmp_path = env_path.with_suffix(env_path.suffix + ".tmp")
+ with open(tmp_path, "w", encoding="utf-8") as f:
+ f.writelines(new_lines)
+ os.chmod(tmp_path, 0o600)
+ os.replace(tmp_path, env_path)
+ os.chmod(env_path, 0o600)
+ return True
except OSError as e:
logger.warning("Error saving to .env file: %s", e)
return False🤖 Prompt for AI Agents
In @cortex/first_run_wizard.py around lines 105 - 153, save_key_to_env_file
currently writes plaintext keys directly and can produce partial/corrupted files
and loose permissions; fix it by performing an atomic write to a temp file in
the same directory and then atomically replacing the target with os.replace (or
similar) after fsyncing the temp file, and ensure the final .env file has strict
permissions (chmod 0o600) and the ~/.cortex directory is created with
restrictive perms (0o700). Implement this in save_key_to_env_file: create parent
dir with mode 0o700, write new_lines to a temporary file in that directory
(fsync and close), os.replace temp → env_path, then os.chmod(env_path, 0o600);
catch and log OSErrors as before and return False on failure.
| def detect_available_providers() -> list[str]: | ||
| """Detect available providers based on valid API keys. | ||
| Uses api_key_detector if available to check all supported locations. | ||
| Returns: | ||
| List of available provider names ('anthropic', 'openai', 'ollama'). | ||
| """ | ||
| providers = [] | ||
|
|
||
| # Use api_key_detector if available | ||
| if HAS_API_KEY_DETECTOR: | ||
| try: | ||
| detected = get_detected_provider() | ||
| if detected: | ||
| providers.append(detected) | ||
| # Check for additional providers | ||
| for provider in SUPPORTED_PROVIDERS: | ||
| if provider not in providers: | ||
| key = detect_api_key(provider) | ||
| if key: | ||
| providers.append(provider) | ||
| if shutil.which("ollama") and "ollama" not in providers: | ||
| providers.append("ollama") | ||
| return providers | ||
| except Exception as e: | ||
| logger.debug("api_key_detector detection failed: %s", e) | ||
|
|
||
| # Fallback detection | ||
| if get_valid_api_key("ANTHROPIC_API_KEY", "anthropic"): | ||
| providers.append("anthropic") | ||
| if get_valid_api_key("OPENAI_API_KEY", "openai"): | ||
| providers.append("openai") | ||
| if shutil.which("ollama"): | ||
| providers.append("ollama") | ||
|
|
||
| return providers | ||
|
|
There was a problem hiding this comment.
Provider availability detection can miss valid providers when api_key_detector is present.
detect_available_providers() returns early after get_detected_provider(), but auto_detect_api_key() is priority-based and may only ever “detect” one provider even if multiple keys exist. This can hide OpenAI availability when Anthropic is also present (and vice versa).
Minimal fix: don’t early-return; always aggregate via get_valid_api_key()
def detect_available_providers() -> list[str]:
@@
- # Use api_key_detector if available
- if HAS_API_KEY_DETECTOR:
- try:
- detected = get_detected_provider()
- if detected:
- providers.append(detected)
- # Check for additional providers
- for provider in SUPPORTED_PROVIDERS:
- if provider not in providers:
- key = detect_api_key(provider)
- if key:
- providers.append(provider)
- if shutil.which("ollama") and "ollama" not in providers:
- providers.append("ollama")
- return providers
- except Exception as e:
- logger.debug("api_key_detector detection failed: %s", e)
+ # Optionally consult api_key_detector for telemetry, but don't let it short-circuit
+ # provider aggregation (it's "best match" detection, not "all providers").
+ if HAS_API_KEY_DETECTOR:
+ try:
+ detected = get_detected_provider()
+ if detected and detected not in providers:
+ providers.append(detected)
+ except Exception as e:
+ logger.debug("api_key_detector detection failed: %s", e)
# Fallback detection
if get_valid_api_key("ANTHROPIC_API_KEY", "anthropic"):
providers.append("anthropic")
if get_valid_api_key("OPENAI_API_KEY", "openai"):
providers.append("openai")
if shutil.which("ollama"):
providers.append("ollama")
return providers🤖 Prompt for AI Agents
In @cortex/first_run_wizard.py around lines 223 - 260, The
detect_available_providers function currently returns early after
get_detected_provider(), which can miss other valid providers; remove the early
return and instead always aggregate providers by checking
detect_api_key(provider) (for each SUPPORTED_PROVIDERS) and/or
get_valid_api_key("ANTHROPIC_API_KEY","anthropic") and
get_valid_api_key("OPENAI_API_KEY","openai") so all valid providers are
collected, and still include shutil.which("ollama") if present; keep the
try/except around get_detected_provider() but merge its result into the
providers list without exiting early.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jaysurse Pull latest changes and updated your branch.
Kindly address all comments.
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jaysurse jay@cortex.local CLA is missing.
5 participants
Closes #547
Summary
This PR fixes an issue where cortex wizard exits early even when a valid API key is already exported in the environment.
Currently, the wizard always prompts the user to export an API key and stops execution. This change updates the wizard logic to detect existing API keys and continue the setup flow instead of exiting.
What was fixed
Detect API keys already present in environment variables
Prevent premature wizard exit when a key exists
Allow the wizard to continue setup (provider confirmation / next steps)
Improve first-time user experience
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.