fix(wizard): allow setup to continue when API key is already present in ENV #356
Conversation
📝 WalkthroughWalkthroughAdds provider-aware CLI and wizard flows with per-provider API-key detection/validation, multi-provider probing for install (sequential/optional parallel), new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
| @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 | ||
| 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) | ||
| result = self.cli.install("docker", dry_run=True) | ||
|
|
||
| self.assertEqual(result, 0) | ||
| mock_interpreter.parse.assert_called_once_with("install docker") | ||
| self.assertEqual(result, 0) | ||
| mock_interpreter.parse.assert_called_once_with("install docker") |
There was a problem hiding this comment.
Critical: Incompatible mixing of unittest decorators and pytest fixtures.
This test mixes unittest and pytest patterns incorrectly:
- Unused decorators: Lines 67-68 have
@patch.dictand@patchdecorators that are never used in the function body. - Invalid fixture usage: Line 69 accepts a
monkeypatchparameter (a pytest fixture), but the class inherits fromunittest.TestCase, which doesn't support pytest fixtures. - Redundant environment setup: Line 70 uses
monkeypatch.setenv, but line 67's@patch.dictdecorator already sets the environment (though it's overridden).
This will cause the test to fail because monkeypatch won't be injected properly.
🔎 Proposed fix - Option 1: Pure unittest style
Remove monkeypatch and use only decorators:
@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
+ @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)
+ result = self.cli.install("docker", dry_run=True)
- self.assertEqual(result, 0)
- mock_interpreter.parse.assert_called_once_with("install docker")
+ self.assertEqual(result, 0)
+ mock_interpreter.parse.assert_called_once_with("install docker")🔎 Alternative fix - Option 2: Convert to pytest style
If switching to pytest, remove unittest.TestCase inheritance and convert all tests:
-class TestCortexCLI(unittest.TestCase):
-
- def setUp(self):
- self.cli = CortexCLI()
+@pytest.fixture
+def cli():
+ return CortexCLI()
@patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-openai-key-123"}, clear=True)
- def test_install_dry_run(self, monkeypatch):
+ def test_install_dry_run(cli, 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)
+ result = cli.install("docker", dry_run=True)
- self.assertEqual(result, 0)
+ assert result == 0
mock_interpreter.parse.assert_called_once_with("install docker")Note: Option 1 is recommended for consistency with the rest of the test file's unittest style.
🤖 Prompt for AI Agents
In tests/test_cli.py around lines 67 to 79, the test mixes unittest decorators
and a pytest fixture (monkeypatch) causing injection failures; convert it to a
pure unittest-style test by removing the monkeypatch parameter and the inline
monkeypatch.setenv call, keep the @patch.dict(os.environ, {"OPENAI_API_KEY":
"sk-test-openai-key-123"}, clear=True) and
@patch("cortex.cli.CommandInterpreter") decorators, adjust the test signature to
def test_install_dry_run(self, mock_interpreter_class):, create the Mock
interpreter inside the function as currently done and assert calls as before;
alternatively, if you prefer pytest style, remove the unittest.TestCase
inheritance and the decorators and use monkeypatch fixture only, but prefer the
former for consistency.
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)
cortex/first_run_wizard.py
Outdated
| from cortex.utils.api_key_validator import ( | ||
| validate_anthropic_api_key, | ||
| validate_openai_api_key, | ||
| ) |
There was a problem hiding this comment.
Remove unused imports or implement actual API key validation.
The imported validate_anthropic_api_key and validate_openai_api_key functions are never called in this file. The is_valid_api_key() function (lines 104-111) only checks key prefixes without validating that the keys actually work. Consider either:
- Removing these unused imports, or
- Using these validators in
get_valid_api_key()to ensure keys are not only well-formed but also functional before continuing the wizard.
🔎 Option 1: Remove unused imports
-from cortex.utils.api_key_validator import (
- validate_anthropic_api_key,
- validate_openai_api_key,
-)🔎 Option 2: Use validators in get_valid_api_key()
def get_valid_api_key(env_var: str, key_type: str) -> str | None:
key = read_key_from_env_file(env_var)
- if key and is_valid_api_key(key, key_type):
+ if key and is_valid_api_key(key, key_type):
+ # Validate the key actually works
+ if key_type == "anthropic" and not validate_anthropic_api_key(key):
+ return None
+ if key_type == "openai" and not validate_openai_api_key(key):
+ return None
os.environ[env_var] = key
return key
os.environ.pop(env_var, None)
return None🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 21-24, the imports
validate_anthropic_api_key and validate_openai_api_key are unused; either remove
them or wire them into get_valid_api_key() so API keys are actually tested. If
choosing to validate, update get_valid_api_key() to call the appropriate
validator for the detected provider (awaiting if the validators are async),
handle exceptions and false returns (log or surface an error to the wizard), and
only accept/return the key when the validator confirms it works; otherwise
prompt the user again. If you prefer not to validate here, remove the two
imports and any related dead code.
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.
| def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None: | ||
| """ | ||
| Get a valid API key from .env file first, then environment variable. | ||
| Treats blank keys as missing. | ||
| """ | ||
| key_from_file = read_key_from_env_file(env_var) | ||
|
|
||
| env_path = get_env_file_path() | ||
| logger.debug(f"Checking {env_var} in {env_path}: '{key_from_file}'") | ||
|
|
||
| if key_from_file is not None and len(key_from_file) > 0: | ||
| if is_valid_api_key(key_from_file, key_type): | ||
| os.environ[env_var] = key_from_file | ||
| return key_from_file | ||
| return None | ||
|
|
||
| if env_var in os.environ: | ||
| del os.environ[env_var] | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def detect_available_providers() -> list[str]: | ||
| """Detect available providers based on valid (non-blank) API keys in .env file.""" | ||
| providers = [] | ||
|
|
||
| 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.
🧩 Analysis chain
🏁 Script executed:
wc -l cortex/first_run_wizard.pyRepository: cortexlinux/cortex
Length of output: 92
🏁 Script executed:
head -200 cortex/first_run_wizard.py | cat -nRepository: cortexlinux/cortex
Length of output: 7205
🏁 Script executed:
sed -n '146,179p' cortex/first_run_wizard.py | cat -nRepository: cortexlinux/cortex
Length of output: 1389
🏁 Script executed:
sed -n '378,417p' cortex/first_run_wizard.py | cat -nRepository: cortexlinux/cortex
Length of output: 1685
🏁 Script executed:
# Find helper function definitions
rg -n "def (read_key_from_env_file|is_valid_api_key|get_env_file_path|save_key_to_env_file|detect_available_providers|run)" cortex/first_run_wizard.pyRepository: cortexlinux/cortex
Length of output: 439
🏁 Script executed:
# Verify the exact behavior: check if env var is actually deleted when .env is missing
sed -n '146,166p' cortex/first_run_wizard.py | cat -nRepository: cortexlinux/cortex
Length of output: 868
🏁 Script executed:
# Also check if there are any other calls to get_valid_api_key that might be relevant
rg -n "get_valid_api_key|detect_available_providers" cortex/first_run_wizard.pyRepository: cortexlinux/cortex
Length of output: 539
get_valid_api_key() contradicts its docstring and deletes environment-only API keys instead of honoring them.
The function's docstring (lines 147-149) states: "Get a valid API key from .env file first, then environment variable. Treats blank keys as missing." However, the implementation only consults the .env file and deletes any existing os.environ[env_var] when the file does not contain a non-blank value:
if key_from_file is not None and len(key_from_file) > 0:
if is_valid_api_key(key_from_file, key_type):
os.environ[env_var] = key_from_file
return key_from_file
return None
if env_var in os.environ:
del os.environ[env_var] # ← Deletes env-only keys
return NoneAdditionally, run() (lines 23-27) unconditionally deletes OPENAI_API_KEY and ANTHROPIC_API_KEY from os.environ when the .env file has those keys blank or missing, and detect_available_providers() relies solely on get_valid_api_key() for detection.
As a result, users who export API keys in their shell environment (the scenario in issue #355) cannot use the wizard—their keys are not recognized and are actively deleted from the environment. To fix this:
- Check
.envfirst and sync valid keys toos.environ - Fall back to
os.environif.envhas no valid key - Only clear env vars if both sources are invalid
Example fix:
def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None:
key_from_file = read_key_from_env_file(env_var)
- ...
- if key_from_file is not None and len(key_from_file) > 0:
- if is_valid_api_key(key_from_file, key_type):
- os.environ[env_var] = key_from_file
- return key_from_file
- return None
-
- if env_var in os.environ:
- del os.environ[env_var]
-
- return None
+ if key_from_file and is_valid_api_key(key_from_file, key_type):
+ os.environ[env_var] = key_from_file
+ return key_from_file
+
+ env_val = os.environ.get(env_var)
+ if env_val and is_valid_api_key(env_val, key_type):
+ return env_val
+
+ os.environ.pop(env_var, None)
+ return NoneRemove the unconditional env var deletions in run().
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 146-179, get_valid_api_key currently
only checks the .env file and deletes environment-only keys, contradicting its
docstring and breaking users who export keys in their shell; change the logic
to: read .env first and if a valid key exists, sync it to os.environ and return
it; if .env has no valid key, check os.environ and if the environment key is
valid return it (do not delete it); only remove/clear the env var if both .env
and os.environ have no valid key; also remove the unconditional deletions of
OPENAI_API_KEY and ANTHROPIC_API_KEY in run() and ensure
detect_available_providers continues to call get_valid_api_key to detect
providers.
| ## 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.
| # 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.
| table.add_row("notify", "Manage desktop notifications") | ||
| table.add_row("env", "Manage environment variables") | ||
| table.add_row("notify", "Manage desktop notifications") |
There was a problem hiding this comment.
Remove duplicate 'notify' entry in help table.
Line 1246 duplicates the "notify" command already shown at line 1244. Remove the duplicate entry.
🔎 Proposed fix
table.add_row("rollback <id>", "Undo installation")
table.add_row("notify", "Manage desktop notifications")
table.add_row("env", "Manage environment variables")
- table.add_row("notify", "Manage desktop notifications")
table.add_row("cache stats", "Show LLM cache statistics")📝 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.
| table.add_row("notify", "Manage desktop notifications") | |
| table.add_row("env", "Manage environment variables") | |
| table.add_row("notify", "Manage desktop notifications") | |
| table.add_row("rollback <id>", "Undo installation") | |
| table.add_row("notify", "Manage desktop notifications") | |
| table.add_row("env", "Manage environment variables") | |
| table.add_row("cache stats", "Show LLM cache statistics") |
🤖 Prompt for AI Agents
In cortex/cli.py around lines 1244-1246, the help table contains a duplicate
entry for "notify" (lines 1244 and 1246). Remove the redundant
table.add_row("notify", "Manage desktop notifications") call (the second
occurrence) so the help table only lists "notify" once; keep the other entries
(e.g., "env") unchanged.
| elif args.command == "env": | ||
| return cli.env(args) | ||
| elif args.command == "env": | ||
| return cli.env(args) |
There was a problem hiding this comment.
Remove duplicate 'env' command dispatch.
Lines 1538-1539 duplicate the "env" command handling from lines 1536-1537. Remove the duplicate elif block.
🔎 Proposed fix
elif args.command == "env":
return cli.env(args)
- elif args.command == "env":
- return cli.env(args)
else:
parser.print_help()📝 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.
| elif args.command == "env": | |
| return cli.env(args) | |
| elif args.command == "env": | |
| return cli.env(args) | |
| elif args.command == "env": | |
| return cli.env(args) | |
| else: | |
| parser.print_help() |
🤖 Prompt for AI Agents
In cortex/cli.py around lines 1536 to 1539 there is a duplicated elif block
handling args.command == "env". Remove the redundant second elif (lines
1538-1539) so the command dispatch only calls and returns cli.env(args) once;
ensure no extra blank lines remain and run tests/lint to confirm no formatting
or unreachable-code issues.
| 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
- Update _get_api_key to return 'ollama-local' when no key found and provider is ollama or no cloud keys set - Change default provider to 'ollama' when no API keys available - Add skip option to wizard for subsequent runs - Fix test mocking for install dry run test - Format code with Black and ensure Ruff compliance Resolves CI test failures for API key fallback and install commands.
- Add proper validation for existing OPENAI_API_KEY in wizard - Test both format and API functionality before accepting keys - Prevent prompting when valid key exists in environment - Update test expectations for offline mode behavior - Add missing Mock import in wizard tests Fixes issue where wizard would ignore valid OpenAI API keys and repeatedly prompt for manual input.
- Add CommandInterpreter mock to test_install_no_api_key - Ensures test passes when no API keys are available but Ollama fallback works - Fixes test failure: 'No commands generated with any available provider'
- Remove @patch.object(CortexCLI, '_get_api_key', return_value=None) - Test now relies on the fixed install() method that doesn't require API keys - Ensures test passes when install works without API key validation
- Add load_env() call to ensure consistent API key loading from .env files - Implement proper non-interactive mode that auto-configures without prompts - Fix config file path handling to use instance paths instead of global paths - Mark setup complete when user chooses to skip configuration - All 47 tests pass with consistent behavior
- Remove auto-skip behavior when provider is already configured - Always display provider selection menu with available options - Add 'Keep current provider (skip setup)' option when re-running wizard - Show current provider status clearly in menu - Allow users to switch providers anytime via wizard
- Provider selection menu now always displays OpenAI, Anthropic, and Ollama options, regardless of API key presence. - If a selected provider's API key is blank or missing in .env, the wizard prompts the user to enter and save a valid key. - Ensures a consistent onboarding flow and allows users to set or update any provider at any time.
… collection - Renamed cortex/utils/api_key_test.py to cortex/utils/api_key_validator.py - Renamed test_anthropic_api_key to validate_anthropic_api_key - Renamed test_openai_api_key to validate_openai_api_key - Updated imports in first_run_wizard.py and test_first_run_wizard.py - Fixes pytest fixture 'api_key' not found error
- Renamed cortex/utils/api_key_test.py to cortex/utils/api_key_validator.py - Renamed test_* functions to validate_* to prevent pytest auto-discovery - Updated all imports in first_run_wizard.py and tests - All 47 tests now pass
- 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
- Always show menu letting user choose API provider - Display '✓ (key found)' next to providers with existing keys - User can now select provider even when keys already exist - Only auto-select in non-interactive mode
jaysurse
force-pushed
the
fix/cortex-wizard-not-running
branch
from
f8ac7e5 to
82595c8
Compare
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)
.env.example (1)
1-69: Resolve merge conflict markers and restore a single valid.env.example.This file still contains Git conflict markers (
<<<<<<< HEAD,=======,>>>>>>> ...) and malformed keys, which breaks tooling (dotenv-linter) and is a merge blocker. Keep one canonical example block and remove the conflict section.Proposed minimal fix (keep existing detailed template)
-<<<<<<< HEAD # Cortex Linux Environment Configuration @@ # - /etc/cortex/.env (Linux only) # -======= -# Example .env file for Cortex -# Copy this to .env and fill in your API keys - -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)If you still want a short example at the bottom, re-add it without conflict markers and ensure any sample keys don’t look like real secrets (e.g.,
OPENAI_API_KEY=test-openai-key).
♻️ Duplicate comments (8)
examples/parallel_llm_demo.py (2)
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. 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)
216-218: Remove trailing whitespace on blank lines to pass linter.Lines 216 and 218 contain trailing whitespace, causing W293 linter failures and blocking CI/CD pipeline.
🔎 Proposed fix
- + f" Time saved: {elapsed_seq - elapsed_par:.2f}s ({((elapsed_seq - elapsed_par) / elapsed_seq * 100):.1f}%)" - + )test_parallel_llm.py (1)
1-314: Clarify role of this script and relocate accordingly.This looks like a manual test/demo harness rather than a proper unit test module, but it lives at the repo root with a
test_*.pyname, which is confusing and may interact oddly with CI test discovery.Consider:
- Moving it under
tests/and converting to pytest/unittest style (no__main__, assertions instead of print-and-bool), or- Moving it to
examples/orscripts/(e.g.,examples/parallel_llm_demo.py) and treating it as a standalone demo.cortex/cli.py (3)
1102-1116: Remove duplicatenotifyrow from Rich help table.
show_rich_help()adds"notify"twice to the commands table (lines 1113 and 1115), which clutters the help output.Suggested cleanup
table.add_row("rollback <id>", "Undo installation") table.add_row("notify", "Manage desktop notifications") table.add_row("env", "Manage environment variables") - table.add_row("notify", "Manage desktop notifications")
1387-1390: Remove duplicateenvcommand dispatch inmain().The
elif args.command == "env": return cli.env(args)branch appears twice back-to-back, which is redundant and slightly obscures the control flow.Proposed change
- elif args.command == "env": - return cli.env(args) - elif args.command == "env": - return cli.env(args) + elif args.command == "env": + return cli.env(args)
41-67: Avoid returning an Ollama key for non-Ollama providers.
_get_api_key()can return"ollama-local"even when_get_provider()is"openai"or"claude"and no API keys are set, because of theor not (OPENAI_API_KEY or ANTHROPIC_API_KEY)condition. That means you may end up sending"ollama-local"as an OpenAI/Claude key, which is misleading and makes misconfiguration harder to diagnose.Safer fallback pattern
- if provider == "ollama" or not ( - os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY") - ): - return "ollama-local" + if provider == "ollama": + # Local provider: no external API key needed + return "ollama-local" + + # For explicit non-ollama providers with no valid key, fail fast + if not ( + os.environ.get("OPENAI_API_KEY") or os.environ.get("ANTHROPIC_API_KEY") + ): + self._print_error("No API keys configured. Set OPENAI_API_KEY/ANTHROPIC_API_KEY or use CORTEX_PROVIDER=ollama.") + return NoneThis keeps the Ollama fallback for the Ollama provider, while making misconfigurations explicit instead of silently substituting an unrelated key.
examples/env_demo.py (1)
1-2: Fix shebang /__future__order to satisfy Python and Ruff.Shebang must be the very first line; move
from __future__ import annotationsbelow the module docstring (before other imports) to fix the I001 error and keep the file executable.Proposed reorder
-from __future__ import annotations -#!/usr/bin/env python3 -""" -Environment Variable Manager Demo +#!/usr/bin/env python3 +""" +Environment Variable Manager Demo @@ -Run this script to see the env manager in action: - python examples/env_demo.py -""" - -import os +Run this script to see the env manager in action: + python examples/env_demo.py +""" + +from __future__ import annotations + +import ostests/test_cli.py (1)
66-78: Fix mixed mocking style and incorrectmonkeypatchusage intest_install_dry_run.This test still mixes unittest-style
@patchdecorators with amonkeypatchparameter (which isn’t injected inunittest.TestCase), and double-patchesCommandInterpreter. The current signature will receive the mock from the decorator as themonkeypatchargument, somonkeypatch.setenvwill fail.Proposed pure-unittest fix
- @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")
🧹 Nitpick comments (2)
tests/test_cli_extended.py (1)
33-37: Confirm intended semantics for installs without API keys across test suites.These changes align extended tests with the new behavior (_get_api_key falling back to
"ollama-local"and install() succeeding when using the local provider), and the tighterassert_called_once_with("install docker")matches the updated CLI implementation.However,
tests/test_cli.TestCortexCLI.test_install_no_api_keystill expectsinstall("docker")to return1when no API key is set. That contradicts the behavior being asserted here (return0via ollama fallback).You may want to:
- Decide whether “no API key” should succeed via Ollama or fail fast, and
- Align both
tests/test_cli.pyandtests/test_cli_extended.pywith that single, documented behavior.Also applies to: 76-84, 106-125
cortex/cli.py (1)
331-403: Provider probing loop ininstall()looks good, but unusedapi_keyand broad exception mask could be clarified.The multi-provider probing (
providers_to_try) and loop overCommandInterpreterlook consistent with the tests and correctly short-circuit on the first provider that yields commands, falling back when exceptions occur.Two nits:
api_keyis assigned in the loop but never used afterwards; it can be dropped unless you plan to surface it later (e.g., for telemetry).- Catching
(RuntimeError, Exception)is effectively the same as catchingException; if you don’t need special handling forRuntimeError, consider justexcept Exception as e:to make intent clearer.
📜 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)
- cortex/utils/api_key_validator.py
- tests/test_env_manager.py
- cortex/env_loader.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_cli.pytests/installer/test_parallel_install.pytests/test_cli_extended.pytest_parallel_llm.pyexamples/env_demo.pycortex/first_run_wizard.pycortex/cli.pyexamples/parallel_llm_demo.pytests/test_first_run_wizard.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli.pytests/installer/test_parallel_install.pytests/test_cli_extended.pytests/test_first_run_wizard.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:
tests/installer/test_parallel_install.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 **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
tests/test_cli.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 **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/first_run_wizard.py
🧬 Code graph analysis (5)
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(57-72)install(331-594)
test_parallel_llm.py (1)
cortex/llm_router.py (8)
LLMRouter(76-846)TaskType(32-42)check_hardware_configs_parallel(979-1027)diagnose_errors_parallel(930-976)query_multiple_packages(880-927)acomplete(543-609)complete_batch(765-846)set_rate_limit(534-541)
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(880-927)
tests/test_first_run_wizard.py (1)
cortex/first_run_wizard.py (1)
_step_api_setup(640-701)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 1-1: [KeyWithoutValue] The <<<<<<< HEAD key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 1-1: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 63-63: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 68-68: [UnorderedKey] The ANTHROPIC_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
[warning] 69-69: [IncorrectDelimiter] The >>>>>>> 34c66df (feat: seamless onboarding—auto-create .env from .env.example, improved wizard flow) key has incorrect delimiter
(IncorrectDelimiter)
[warning] 69-69: [KeyWithoutValue] The >>>>>>> 34c66df (feat: seamless onboarding—auto-create .env from .env.example, improved wizard flow) key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 69-69: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 69-69: [LowercaseKey] The >>>>>>> 34c66df (feat: seamless onboarding—auto-create .env from .env.example, improved wizard flow) key should be in uppercase
(LowercaseKey)
[warning] 69-69: [UnorderedKey] The >>>>>>> 34c66df (feat: seamless onboarding—auto-create .env from .env.example, improved wizard flow) key should go before the ANTHROPIC_API_KEY key
(UnorderedKey)
🪛 GitHub Actions: CI
examples/env_demo.py
[error] 1-1: 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.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (3)
cortex/cli.py (1)
738-779:envsubcommand handler is structurally solid.The
env()method cleanly routes subcommands to dedicated helpers and wraps the entire operation in a singletry/except, producing a user-friendly error on unexpected failures. Type hints and docstrings on the helpers follow the coding guidelines.cortex/first_run_wizard.py (2)
318-352: Guard_prompt_for_api_keyagainst non-interactive mode to avoid infinite loops.
_prompt_for_api_key()loops until a non-blank, correctly prefixed key is entered. Whenself.interactiveisFalse,_prompt()immediately returns the default (empty string), so this loop will never make progress ifrun()is ever invoked in non-interactive mode.[ suggest_recommended_refactor ]
Suggested non-interactive guard
def _prompt_for_api_key(self, key_type: str) -> str | None: """Prompt user for a valid API key, rejecting blank inputs.""" + if not self.interactive: + # In non-interactive mode we cannot prompt; let caller handle None. + return None @@ - while True: + while True: key = self._prompt(f"Enter your {provider_name} API key (or 'q' to cancel): ")This keeps current interactive behavior but ensures non-interactive flows fail fast instead of hanging.
463-562: New provider selection + dry-run verification flow looks solid.The main provider-selection branch in
run():
- Correctly distinguishes Anthropic/OpenAI/Ollama and a “skip reconfiguration” path.
- Reuses existing keys when present, only prompting to replace them when requested.
- Saves keys preferentially to
.envwith a shell-profile fallback.- Sets
self.config["api_provider"]/["api_key_configured"]consistently.- Verifies the chosen provider via a
cortex install "<example>"dry run with the appropriateforced_providerargument.Assuming the ENV-handling fixes above, this flow nicely satisfies the wizard UX goals.
| def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None: | ||
| """ | ||
| Get a valid API key from .env file first, then environment variable. | ||
| Treats blank keys as missing. | ||
| """ | ||
| key_from_file = read_key_from_env_file(env_var) | ||
|
|
||
| env_path = get_env_file_path() | ||
| logger.debug(f"Checking {env_var} in {env_path}: '{key_from_file}'") | ||
|
|
||
| if key_from_file is not None and len(key_from_file) > 0: | ||
| if is_valid_api_key(key_from_file, key_type): | ||
| os.environ[env_var] = key_from_file | ||
| return key_from_file | ||
| return None | ||
|
|
||
| if env_var in os.environ: | ||
| del os.environ[env_var] | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def detect_available_providers() -> list[str]: | ||
| """Detect available providers based on valid (non-blank) API keys in .env file.""" | ||
| providers = [] | ||
|
|
||
| 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"): |
There was a problem hiding this comment.
ENV-exported API keys are still deleted/ignored, so the wizard doesn’t honor existing keys.
The combination of get_valid_api_key() and the early loop in run() still removes shell-exported API keys instead of using them:
-
get_valid_api_key()only checks the.envfile; when no valid entry is found it does:if env_var in os.environ: del os.environ[env_var] return None
-
run()then callsread_key_from_env_file()and, if the file has no value, again deletesOPENAI_API_KEY/ANTHROPIC_API_KEYfromos.environ.
This directly contradicts the PR objective: a user who has only export OPENAI_API_KEY=... (no .env) will have that key removed from the process environment as soon as cortex wizard runs, and detect_available_providers() will never see it.
Suggested fix: honor ENV first and stop deleting valid shell keys
- Make
get_valid_api_key()prefer.envbut fall back toos.environwithout deleting valid ENV-only keys:
def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None:
@@
- if key_from_file is not None and len(key_from_file) > 0:
- if is_valid_api_key(key_from_file, key_type):
- os.environ[env_var] = key_from_file
- return key_from_file
- return None
-
- if env_var in os.environ:
- del os.environ[env_var]
-
- return None
+ # Prefer .env if it contains a valid, non-blank key
+ if key_from_file and is_valid_api_key(key_from_file, key_type):
+ os.environ[env_var] = key_from_file
+ return key_from_file
+
+ # Fall back to existing environment value
+ env_value = os.environ.get(env_var)
+ if env_value and is_valid_api_key(env_value, key_type):
+ return env_value
+
+ # Neither source has a valid key; optionally clear an invalid env var
+ os.environ.pop(env_var, None)
+ return None- Remove the explicit deletion loop in
run(); letload_dotenv()+get_valid_api_key()handle precedence:
- for key_name in ["OPENAI_API_KEY", "ANTHROPIC_API_KEY"]:
- file_value = read_key_from_env_file(key_name)
- if file_value is None or len(file_value.strip()) == 0:
- if key_name in os.environ:
- del os.environ[key_name]
-
- available_providers = detect_available_providers()
+ # Do not delete shell-exported keys; detect_available_providers()
+ # will consult both .env and os.environ via get_valid_api_key().
+ available_providers = detect_available_providers()With these changes, a user who has only exported an API key in their shell will see that provider listed as available and can proceed through the wizard without re-entering the key, satisfying issue #355.
Also applies to: 392-407
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 146-176 (and also apply to logic at
392-407): update get_valid_api_key() so it checks the .env value first but if
that value is None or blank, fall back to checking os.environ for the key
(validate that value with is_valid_api_key) and do NOT delete os.environ
entries; remove the block that deletes env vars in run() (lines ~392-407) so we
no longer remove shell-exported API keys and instead let
load_dotenv()+get_valid_api_key() decide precedence.
| # Create 3 independent commands using Python's time.sleep (Windows-compatible) | ||
| commands = [ | ||
| "python -c \"import time; time.sleep(0.1); print('Task 1')\"", | ||
| "python -c \"import time; time.sleep(0.1); print('Task 2')\"", | ||
| "python -c \"import time; time.sleep(0.1); print('Task 3')\"", | ||
| "python3 -c \"import time; time.sleep(0.1); print('Task 1')\"", | ||
| "python3 -c \"import time; time.sleep(0.1); print('Task 2')\"", | ||
| "python3 -c \"import time; time.sleep(0.1); print('Task 3')\"", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and view the relevant lines
if [ -f "tests/installer/test_parallel_install.py" ]; then
echo "=== File content (lines 15-25) ==="
sed -n '15,25p' tests/installer/test_parallel_install.py
echo ""
echo "=== All instances of python/python3 in this file ==="
grep -n "python" tests/installer/test_parallel_install.py | head -20
else
echo "File not found, checking for installer test files:"
find tests -name "*parallel*" -o -name "*install*" 2>/dev/null
fiRepository: cortexlinux/cortex
Length of output: 148
🏁 Script executed:
# Check for CI configuration files that specify Windows testing
echo "=== Checking for CI/CD configurations ==="
find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null | while read file; do
echo "File: $file"
grep -i "windows\|os:" "$file" | head -10
doneRepository: cortexlinux/cortex
Length of output: 268
🏁 Script executed:
# Check for other test files and their python command patterns
echo "=== Pattern of python commands in other test files ==="
rg -t py "python[0-9]? -c|python -c" tests/ --max-count=5 -A 1Repository: cortexlinux/cortex
Length of output: 148
🏁 Script executed:
# Check project documentation for platform support
echo "=== Checking README and docs for platform mentions ==="
find . -maxdepth 2 -name "README*" -o -name "CONTRIBUTING*" 2>/dev/null | while read file; do
[ -f "$file" ] && echo "=== $file ===" && head -50 "$file" | grep -i "python\|windows\|platform" || true
doneRepository: cortexlinux/cortex
Length of output: 273
🏁 Script executed:
# Read the specific test file - just the relevant section
cat tests/installer/test_parallel_install.py 2>/dev/null | head -50Repository: cortexlinux/cortex
Length of output: 1888
🏁 Script executed:
# Check CI file for Windows testing
grep -i "windows\|ubuntu\|macos\|runs-on" .github/workflows/ci.yml 2>/dev/nullRepository: cortexlinux/cortex
Length of output: 169
🏁 Script executed:
# Quick search for python3 vs python in test file
grep -c "python3 -c" tests/installer/test_parallel_install.py 2>/dev/null
grep -c "python -c" tests/installer/test_parallel_install.py 2>/dev/nullRepository: cortexlinux/cortex
Length of output: 148
🏁 Script executed:
# Check if there are any Windows-specific test markers or skips
grep -r "windows\|platform\|skipif" tests/installer/ 2>/dev/null | grep -v ".pyc"Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Check setup.py, pyproject.toml, or similar for platform info
cat pyproject.toml 2>/dev/null | grep -A 10 -i "python\|platform" | head -30Repository: cortexlinux/cortex
Length of output: 801
🏁 Script executed:
# Look for other command execution patterns in the codebase
grep -r "subprocess\|Popen\|run\|shell=True" tests/installer/ 2>/dev/null | head -10Repository: cortexlinux/cortex
Length of output: 964
🏁 Script executed:
# Verify the exact OS declaration in pyproject.toml
grep -A 2 "Operating System" pyproject.tomlRepository: cortexlinux/cortex
Length of output: 128
Remove or clarify the misleading "Windows-compatible" comment.
The project explicitly declares "Operating System :: POSIX :: Linux" in pyproject.toml and runs tests only on Ubuntu. The python3 command is appropriate for Linux environments with Python 3.10+. The comment on line 18 stating "Windows-compatible" is misleading since Windows support is not in scope for this project. Update the comment to accurately reflect the Linux-only requirement (e.g., "Create 3 independent commands using Python's time.sleep") or remove it entirely.
Also applies to: 45-47, 73-75, 101-104, 124-124, 141-141, 154-154, 180-180, 200-202, 221-221, 250-253, 279-281
🤖 Prompt for AI Agents
In tests/installer/test_parallel_install.py around lines 18-22, the inline
comment saying "Windows-compatible" is misleading for a Linux-only project;
remove that phrase or replace the comment with an accurate statement such as
"Create 3 independent commands using Python's time.sleep" (or simply "Create 3
independent commands using Python's time.sleep") and apply the same change to
the other occurrences listed (lines 45-47, 73-75, 101-104, 124, 141, 154, 180,
200-202, 221, 250-253, 279-281) so that comments do not claim Windows
compatibility.
| @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test-key-12345678"}) | ||
| def test_step_api_setup_existing_anthropic_key(self, wizard): | ||
| """Test API setup with existing Anthropic key.""" | ||
| result = wizard._step_api_setup() | ||
|
|
||
| assert result.success is True | ||
| assert wizard.config.get("api_provider") == "anthropic" | ||
|
|
||
| @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key-12345678"}, clear=True) | ||
| def test_step_api_setup_existing_openai_key(self, wizard): | ||
| """Test API setup with existing OpenAI key.""" | ||
| result = wizard._step_api_setup() | ||
|
|
||
| assert result.success is True | ||
| assert wizard.config.get("api_provider") == "openai" | ||
|
|
There was a problem hiding this comment.
Replace realistic-looking API key placeholders to satisfy Gitleaks.
The hard-coded values "sk-ant-test-key-12345678" and "sk-test-key-12345678" resemble real API keys and trigger Gitleaks. The tests only need non-empty values, so you can use safer placeholders that don’t match secret patterns.
Suggested safer test values
- @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test-key-12345678"})
+ @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "test-anthropic-api-key"})
@@
- @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key-12345678"}, clear=True)
+ @patch.dict(os.environ, {"OPENAI_API_KEY": "test-openai-api-key"}, clear=True)These still make _step_api_setup treat the keys as “present” without tripping secret scanners.
📝 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.
| @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test-key-12345678"}) | |
| def test_step_api_setup_existing_anthropic_key(self, wizard): | |
| """Test API setup with existing Anthropic key.""" | |
| result = wizard._step_api_setup() | |
| assert result.success is True | |
| assert wizard.config.get("api_provider") == "anthropic" | |
| @patch.dict(os.environ, {"OPENAI_API_KEY": "sk-test-key-12345678"}, clear=True) | |
| def test_step_api_setup_existing_openai_key(self, wizard): | |
| """Test API setup with existing OpenAI key.""" | |
| result = wizard._step_api_setup() | |
| assert result.success is True | |
| assert wizard.config.get("api_provider") == "openai" | |
| @patch.dict(os.environ, {"ANTHROPIC_API_KEY": "test-anthropic-api-key"}) | |
| def test_step_api_setup_existing_anthropic_key(self, wizard): | |
| """Test API setup with existing Anthropic key.""" | |
| result = wizard._step_api_setup() | |
| assert result.success is True | |
| assert wizard.config.get("api_provider") == "anthropic" | |
| @patch.dict(os.environ, {"OPENAI_API_KEY": "test-openai-api-key"}, clear=True) | |
| def test_step_api_setup_existing_openai_key(self, wizard): | |
| """Test API setup with existing OpenAI key.""" | |
| result = wizard._step_api_setup() | |
| assert result.success is True | |
| assert wizard.config.get("api_provider") == "openai" |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 269-269: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In tests/test_first_run_wizard.py around lines 261 to 276, the hard-coded API
key strings ("sk-ant-test-key-12345678" and "sk-test-key-12345678") look like
real secrets and trigger Gitleaks; replace them with non-secret placeholders
that are non-empty but do not match secret/key patterns (e.g.,
"ANTHROPIC_KEY_PRESENT" and "OPENAI_KEY_PRESENT" or similar), keeping the
patch.dict usage and test logic unchanged so _step_api_setup still detects a
present key.
|
@Suyashd999 This PR fulfills the requirements of Issue #356, once the Lint Formatting Issues are solved, its ready to merge. |
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
examples/env_demo.py (1)
1-2: [DUPLICATE] Fix import ordering: shebang must be first.The
from __future__ import annotationsimport is placed before the shebang, violating Python syntax and PEP 8. This issue was already flagged in a previous review.examples/parallel_llm_demo.py (2)
39-40: [DUPLICATE] Remove duplicate API call that doubles costs.Line 39 calls
query_multiple_packagesbut the result is immediately overwritten by the identical call on line 40. This issue was already flagged in a previous review.
216-218: [DUPLICATE] Remove trailing whitespace to pass linter.Lines 216 and 218 contain trailing whitespace causing W293 linter failures. This was already flagged in a previous review.
🧹 Nitpick comments (3)
cortex/dependency_check.py (1)
26-67: Consider adding docstrings to public functions.Per the coding guidelines, docstrings are required for all public APIs. The following functions would benefit from docstrings describing their purpose, parameters, and return values:
check_dependency(lines 26-31)get_missing_dependencies(lines 34-35)format_installation_instructions(lines 38-60)verify_dependencies_or_exit(lines 63-67)This is a pre-existing gap across the file and can be addressed in a follow-up refactoring.
cortex/utils/api_key_validator.py (2)
10-28: Enhance docstring per coding guidelines.The function lacks detailed parameter and return documentation required by the coding guidelines for public APIs.
🔎 Proposed docstring enhancement
def validate_anthropic_api_key(api_key: str) -> bool: - """Validate Anthropic (Claude) API key by making a minimal request.""" + """Validate Anthropic (Claude) API key by making a minimal request. + + Makes a minimal POST request to the Anthropic API to verify the key works. + + Args: + api_key: The Anthropic API key to validate (should start with 'sk-ant-') + + Returns: + True if the API key is valid (HTTP 200 response), False otherwise. + """ try:Based on coding guidelines.
31-45: Enhance docstring per coding guidelines.The function lacks detailed parameter and return documentation required by the coding guidelines for public APIs.
🔎 Proposed docstring enhancement
def validate_openai_api_key(api_key: str) -> bool: - """Validate OpenAI API key by making a minimal request.""" + """Validate OpenAI API key by making a minimal request. + + Makes a minimal POST request to the OpenAI chat completions API to verify the key works. + + Args: + api_key: The OpenAI API key to validate (should start with 'sk-') + + Returns: + True if the API key is valid (HTTP 200 response), False otherwise. + """ try:Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/dependency_check.pycortex/first_run_wizard.pycortex/llm/interpreter.pycortex/utils/api_key_validator.pyexamples/env_demo.pyexamples/parallel_llm_demo.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/llm/interpreter.pyexamples/env_demo.pycortex/first_run_wizard.pyexamples/parallel_llm_demo.pycortex/dependency_check.pycortex/utils/api_key_validator.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 (2)
cortex/first_run_wizard.py (1)
cortex/utils/api_key_validator.py (2)
validate_anthropic_api_key(10-28)validate_openai_api_key(31-45)
examples/parallel_llm_demo.py (1)
cortex/llm_router.py (1)
query_multiple_packages(880-927)
⏰ 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). (5)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
cortex/dependency_check.py (1)
46-46: LGTM - Formatting improvement.The change to use
", ".join(missing)produces clean, comma-separated package names in the user-facing error message, improving readability.cortex/llm/interpreter.py (1)
118-119: LGTM! Clean formatting adjustment.The Ollama client initialization has been reformatted to place
api_keyandbase_urlon separate lines. This improves readability with no functional changes.
| # 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.
Remove unused imports.
The imported validator functions validate_anthropic_api_key and validate_openai_api_key are never called in this file. Either remove these unused imports or integrate the validators into the API key verification flow.
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 21 to 22, the imports
validate_anthropic_api_key and validate_openai_api_key are unused; remove these
two imports from the file (or alternatively, if API key validation is desired
here, call the appropriate validator(s) in the key verification flow and
handle/report validation errors). Ensure no other code references these names
and run linting/tests to confirm the unused-import error is resolved.
| def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None: | ||
| """ | ||
| Get a valid API key from .env file first, then environment variable. | ||
| Treats blank keys as missing. | ||
| """ | ||
| key_from_file = read_key_from_env_file(env_var) | ||
|
|
||
| env_path = get_env_file_path() | ||
| logger.debug(f"Checking {env_var} in {env_path}: '{key_from_file}'") | ||
|
|
||
| if key_from_file is not None and len(key_from_file) > 0: | ||
| if is_valid_api_key(key_from_file, key_type): | ||
| os.environ[env_var] = key_from_file | ||
| return key_from_file | ||
| return None | ||
|
|
||
| if env_var in os.environ: | ||
| del os.environ[env_var] | ||
|
|
||
| return None |
There was a problem hiding this comment.
CRITICAL: Function deletes shell-exported API keys, contradicting PR objectives.
This function claims to check "from .env file first, then environment variable" (line 148) but never checks os.environ for existing keys. Lines 162-163 explicitly delete any environment-exported API key when the .env file doesn't contain a valid value:
if env_var in os.environ:
del os.environ[env_var] # ← Deletes shell-exported keys!This is the exact bug the PR is supposed to fix (issue #355: wizard exits when API key is exported in shell). Users who export OPENAI_API_KEY=... in their shell will have their keys deleted and the wizard will fail.
🔎 Proposed fix to honor environment-exported keys
def get_valid_api_key(env_var: str, key_type: str = "generic") -> str | None:
"""
Get a valid API key from .env file first, then environment variable.
Treats blank keys as missing.
"""
key_from_file = read_key_from_env_file(env_var)
env_path = get_env_file_path()
logger.debug(f"Checking {env_var} in {env_path}: '{key_from_file}'")
if key_from_file is not None and len(key_from_file) > 0:
if is_valid_api_key(key_from_file, key_type):
os.environ[env_var] = key_from_file
return key_from_file
- return None
+ # .env file has invalid key, fall through to check os.environ
+ # Check environment variable as fallback
+ env_value = os.environ.get(env_var)
+ if env_value and is_valid_api_key(env_value.strip(), key_type):
+ return env_value.strip()
+
+ # No valid key in either location
if env_var in os.environ:
del os.environ[env_var]
return None🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 146-165, the function currently
removes shell-exported API keys instead of honoring them; change the logic so
that if the key from the .env file is missing or blank the function next checks
os.environ for env_var, and if that env value exists and is non-empty and passes
is_valid_api_key(key, key_type) return it (do not delete it); only overwrite
os.environ when you have a valid key_from_file, and do not delete an existing
environment-exported key when the .env file is empty or invalid—log the outcome
instead.
| for key_name in ["OPENAI_API_KEY", "ANTHROPIC_API_KEY"]: | ||
| file_value = read_key_from_env_file(key_name) | ||
| if file_value is None or len(file_value.strip()) == 0: | ||
| if key_name in os.environ: | ||
| del os.environ[key_name] |
There was a problem hiding this comment.
CRITICAL: Code explicitly deletes shell-exported API keys.
This loop unconditionally deletes OPENAI_API_KEY and ANTHROPIC_API_KEY from os.environ if they're not present in the .env file, even when users have exported them in their shell:
for key_name in ["OPENAI_API_KEY", "ANTHROPIC_API_KEY"]:
file_value = read_key_from_env_file(key_name)
if file_value is None or len(file_value.strip()) == 0:
if key_name in os.environ:
del os.environ[key_name] # ← Breaks shell-exported keys!This directly contradicts the PR objective: "allow setup to continue when API key is already present in ENV." Remove this deletion logic and let detect_available_providers() discover keys from either source.
🔎 Proposed fix
- for key_name in ["OPENAI_API_KEY", "ANTHROPIC_API_KEY"]:
- file_value = read_key_from_env_file(key_name)
- if file_value is None or len(file_value.strip()) == 0:
- if key_name in os.environ:
- del os.environ[key_name]
-
available_providers = detect_available_providers()After fixing get_valid_api_key() to check os.environ, remove this deletion block entirely.
🤖 Prompt for AI Agents
In cortex/first_run_wizard.py around lines 398 to 402, the loop removes
OPENAI_API_KEY and ANTHROPIC_API_KEY from os.environ when the keys are missing
or empty in the .env file; this deletes shell-exported keys and contradicts the
intended behavior. Remove this deletion block entirely so environment variables
are left intact and ensure get_valid_api_key() is adjusted to check os.environ
first (falling back to .env) so detect_available_providers() can find keys from
either source.
2 participants
Closes #355
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
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.