feat: Zero-config API key auto-detection (#255) #436
Conversation
divanshu-go
requested review from
Anshgrover23 and
mikejmorgan-ai
as code owners
|
Warning Rate limit exceeded@divanshu-go has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a zero‑config API key detector (new module) that auto-discovers Anthropic/OpenAI keys from env vars and common config locations, caches discovery, can prompt/save keys to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Cortex CLI
participant Detector as APIKeyDetector
participant Env as Environment
participant FS as File System
User->>CLI: run command
CLI->>CLI: _get_api_key()
CLI->>Detector: setup_api_key()
Detector->>FS: read cache (~/.cortex/.api_key_cache)
alt cache hit
FS-->>Detector: cached provider/key hint
Detector-->>CLI: return key & provider
else cache miss
Detector->>Env: check ANTHROPIC_API_KEY / OPENAI_API_KEY
alt env found
Env-->>Detector: key
Detector->>FS: write cache & session env
Detector-->>CLI: return key & provider
else
Detector->>FS: read ~/.cortex/.env
alt found
FS-->>Detector: env content
Detector->>Detector: extract & infer provider
Detector->>FS: write cache
Detector-->>CLI: return key & provider
else
Detector->>FS: read ~/.config/anthropic/...credentials.json
alt found
FS-->>Detector: json content
Detector->>Detector: extract & cache
Detector-->>CLI: return key & provider
else
Detector->>FS: read ~/.config/openai/...credentials.json
alt found
FS-->>Detector: json content
Detector->>Detector: extract & cache
Detector-->>CLI: return key & provider
else
Detector->>FS: read ./ .env
alt found
FS-->>Detector: env content
Detector->>Detector: extract & cache
Detector-->>CLI: return key & provider
else
Detector->>User: prompt for manual key & save?
User-->>Detector: provide key / choice
Detector->>FS: optionally save ~/.cortex/.env and cache
Detector-->>CLI: return key & provider
end
end
end
end
end
end
CLI->>CLI: set _detected_provider and continue
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)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cortex/api_key_detector.py (4)
56-106:detect()modifies globalos.environas a side effect.The method sets
os.environ[env_var] = key(lines 80, 103) when extracting keys from files. While this enables downstream code to read the key from the environment, it's an implicit side effect that could surprise callers. Consider documenting this behavior in the docstring.🔎 Suggested docstring update
def detect(self) -> Tuple[bool, Optional[str], Optional[str], Optional[str]]: """ Auto-detect API key from common locations. + Note: + When a key is found in a file, it is also set in os.environ + for the current session so downstream code can access it. + Returns: Tuple of (found, key, provider, source)
247-257: JSON extraction doesn't validate key format before returning.
_extract_from_jsonreturns any value found under known field names without validating it's actually a valid API key. If a JSON file has{"api_key": "some-random-value"}, this would be returned and later fail validation elsewhere.🔎 Add key validation before returning
def _extract_from_json(self, content: str, env_var: str) -> Optional[str]: """Extract API key from JSON content.""" try: data = json.loads(content) # Look for key in common locations for key_field in API_KEY_FIELD_NAMES + [env_var]: if key_field in data: - return data[key_field] + value = data[key_field] + if self._is_valid_key(value): + return value except (json.JSONDecodeError, ValueError): pass return None
259-265: Regex pattern may match keys with trailing comments incorrectly.The pattern
rf"^{env_var}\s*=\s*(.+?)(?:\s*#.*)?$"uses a non-greedy.+?which should work, but consider that.strip('"\'')on line 264 only strips one layer of quotes. A value like"'sk-ant-test'"would become'sk-ant-test'(still quoted).This edge case is unlikely in practice, so flagging as optional.
364-374:_update_or_append_keyuses simple string matching for replacement.The check
if f"{var_name}=" in existingwill match partial variable names (e.g.,MY_ANTHROPIC_API_KEY=would match when looking forANTHROPIC_API_KEY=). The regex replacement on line 369 with^anchor mitigates this for the replacement itself, but the conditional could give false positives.🔎 Use regex for the existence check too
def _update_or_append_key(self, existing: str, var_name: str, key: str) -> str: """Update existing key or append new one to file content.""" - if f"{var_name}=" in existing: + pattern = rf"^{var_name}=" + if re.search(pattern, existing, re.MULTILINE): # Replace existing - pattern = rf"^{var_name}=.*$" - return re.sub(pattern, f"{var_name}={key}", existing, flags=re.MULTILINE) + replace_pattern = rf"^{var_name}=.*$" + return re.sub(replace_pattern, f"{var_name}={key}", existing, flags=re.MULTILINE) else: # Append if existing and not existing.endswith("\n"): existing += "\n" return existing + f"{var_name}={key}\n"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdcortex/api_key_detector.pycortex/cli.pydocs/ZERO_CONFIG_API_KEYS.mdtests/test_api_key_detector.pytests/test_cli.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_api_key_detector.pytests/test_cli.pycortex/cli.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.pytests/test_cli.py
🧬 Code graph analysis (3)
tests/test_cli.py (1)
cortex/cli.py (2)
_get_api_key(46-68)_get_provider(70-90)
cortex/cli.py (2)
cortex/api_key_detector.py (2)
auto_detect_api_key(396-404)setup_api_key(407-432)cortex/first_run_wizard.py (1)
_print_error(746-748)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(23-27)
🪛 LanguageTool
docs/ZERO_CONFIG_API_KEYS.md
[grammar] ~252-~252: Ensure spelling is correct
Context: ...N_WIZARD.md](FIRST_RUN_WIZARD.md). ### Environment Management API keys can also be manage...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ZERO_CONFIG_API_KEYS.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (24)
cortex/api_key_detector.py (5)
1-25: Well-documented module with clear structure.The module docstring clearly explains the detection priority and purpose. Imports are appropriate for the functionality.
26-39: Key pattern order matters for correct provider detection.The
KEY_PATTERNSdictionary iterates in insertion order (Python 3.7+), andsk-ant-is checked beforesk-. This is correct since OpenAI keys also start withsk-, but Anthropic keys have the more specificsk-ant-prefix.
108-168:prompt_for_keyhandles user interaction cleanly.The method properly handles EOFError/KeyboardInterrupt, validates key format, and offers Ollama as a no-key alternative. The flow is intuitive.
One minor observation: the save prompt on line 155 uses
print()directly while other messages usecx_print(). This is acceptable since it's a prompt requiring inline response.
302-323: Cache stores key hint but not the full key — good security practice.The
key_hintonly stores the first 10 characters, which is sufficient to identify the key type without exposing the full secret.
396-431: Convenience functions provide clean public API.
auto_detect_api_key()andsetup_api_key()are well-designed entry points. Thesetup_api_key()function correctly skips the "Found" message for the canonical location and delegates saving to_maybe_save_found_key.README.md (3)
104-107: API key path updated to canonical location.The documentation now correctly points to
~/.cortex/.envas the save location, consistent with the new detector implementation.
113-114: Zero-config note provides helpful guidance.The added note clearly explains that existing Claude/OpenAI CLI users get automatic detection, with a link to detailed documentation.
259-276: Troubleshooting section updated with actionable guidance.The expanded troubleshooting covers multiple fallback options (direct config, Ollama, Claude CLI auto-detect) and links to the new zero-config documentation.
tests/test_cli.py (4)
14-21: Test isolation with temporary directories is correctly implemented.The setup creates a temp directory for cache isolation, and teardown properly cleans up. This prevents tests from interfering with real user configuration.
23-30: Test properly mocks home directory and save prompt.The test correctly patches
Path.home, clears environment variables, and mocks the save prompt input to "n" to avoid side effects.
43-50: Test verifies Ollama fallback behavior.When no API key is found and user selects option "3" (Ollama), the test confirms the CLI returns "ollama-local" as expected.
52-70: Provider detection tests call_get_api_keyfirst to populate_detected_provider.This correctly reflects the new flow where
_get_provider()depends on state set by_get_api_key(). The test comments explain this dependency.docs/ZERO_CONFIG_API_KEYS.md (3)
1-23: Comprehensive documentation for the zero-config feature.The overview clearly explains the feature's purpose and benefits. The feature list provides a quick summary of capabilities.
152-172: Security section clearly documents file permissions and key handling.The documentation properly explains the 600 permissions, key validation by prefix, and that full keys are never logged.
260-274: Testing section provides clear instructions.The test command and coverage outline help contributors understand how to verify the feature.
tests/test_api_key_detector.py (5)
1-16: Well-structured test module with clear docstring.The imports are appropriate, and the docstring explains the test scope.
19-72: Helper methods reduce test boilerplate effectively.The fixture and helper methods (
_setup_detector_with_home,_setup_config_file,_detect_with_mocked_home,_assert_found_key,_assert_env_contains) encapsulate common patterns and make individual tests more readable.
127-142: Test for current directory detection creates isolated environment.The test properly mocks both
Path.cwd()andPath.home()to ensure the detector finds the key from the current directory and not from the user's actual home.
143-158: Priority order test verifies ~/.cortex/.env takes precedence over config files.This correctly validates the documented detection priority.
325-378: Convenience function tests properly mock internal methods.The tests for
auto_detect_api_keyandsetup_api_keyuse appropriate mocking strategies to isolate behavior.Note: The test on line 332 uses a complex lambda to mock
__init__, which works but is slightly fragile. Consider usingpatch.objecton instance attributes instead for clarity.cortex/cli.py (4)
9-9: Import added for new API key detector functions.The import is correctly placed with other cortex imports.
46-68:_get_api_keynow uses the unified detection flow.The method correctly:
- Checks explicit provider overrides first (fake/ollama)
- Delegates to
setup_api_key()for auto-detection with prompts- Stores
_detected_providerfor later use by_get_provider()- Provides helpful guidance when no key is found
70-90:_get_providercorrectly maps detected provider to CLI provider names.The mapping from internal names (anthropic → claude, openai → openai) is handled properly. The method also falls back to environment variables and ultimately to Ollama for offline mode.
119-186: Newnotifymethod handles subcommands gracefully.The method:
- Validates subcommand presence (line 123)
- Validates DND time format (lines 163-168)
- Properly routes to NotificationManager
The comment on line 145-146 acknowledges using a private method
_save_config(). This is acceptable for now but consider adding a publicsave()method to NotificationManager in a future refactor.
divanshu-go
force-pushed
the
feat-api-key
branch
from
b850118 to
d0ef757
Compare
CLA Verification PassedAll contributors have signed the CLA.
|
divanshu-go
force-pushed
the
feat-api-key
branch
from
d0ef757 to
0599ffc
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements zero-config API key auto-detection for Cortex, eliminating the need for manual environment variable setup by automatically discovering API keys from common locations used by Claude CLI and OpenAI CLI.
Key Changes:
- New
APIKeyDetectorclass that checks 5 prioritized locations for API keys - Interactive prompt system for selecting providers and entering keys when none are found
- Secure caching mechanism to avoid repeated file system checks
- Automatic save prompt to consolidate keys into
~/.cortex/.envwith 600 permissions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
cortex/api_key_detector.py |
New module implementing the core auto-detection logic with caching, file parsing (JSON and .env formats), and secure key storage |
cortex/cli.py |
Updated _get_api_key() and _get_provider() to integrate with the new auto-detection system, replacing direct validation calls |
tests/test_api_key_detector.py |
Comprehensive test suite covering detection from all locations, caching, priority order, file parsing, and user prompts |
tests/test_cli.py |
Updated CLI tests to mock the new auto-detection flow and properly isolate test environments with temporary directories |
docs/ZERO_CONFIG_API_KEYS.md |
Detailed documentation explaining detection priority, file formats, security features, troubleshooting, and usage examples |
README.md |
Added quick reference to zero-config feature with link to detailed documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/ZERO_CONFIG_API_KEYS.md (1)
50-53: Add language specifier to fenced code block.The code block on line 51 is missing a language specifier, as flagged by markdownlint.
🧹 Nitpick comments (7)
cortex/api_key_detector.py (4)
56-106: Side effect of setting environment variables should be documented.The
detect()method mutatesos.environwhen a key is found from a file (lines 80, 103). While this is useful for session-wide availability, it's a side effect that callers might not expect from a "detect" method. Consider adding this behavior to the docstring.🔎 Suggested docstring update
def detect(self) -> Tuple[bool, Optional[str], Optional[str], Optional[str]]: """ Auto-detect API key from common locations. Returns: Tuple of (found, key, provider, source) - found: True if key was found - key: The API key (or None) - provider: "anthropic" or "openai" (or None) - source: Where the key was found (or None) + + Note: + When a key is found from a file, it is also set in os.environ + for session-wide availability. """
129-132: Nested ternary expression reduces readability.The nested ternary on line 129 is harder to follow. A simple if-elif would be clearer.
🔎 Suggested refactor
- provider = "anthropic" if choice == "1" else "openai" if choice == "2" else None - if not provider: - cx_print("Invalid choice.", "warning") - return (False, None, None) + if choice == "1": + provider = "anthropic" + elif choice == "2": + provider = "openai" + else: + cx_print("Invalid choice.", "warning") + return (False, None, None)
155-155: Inconsistent use ofprint()vscx_print().Lines 155 and 183 use raw
print()while the rest of the module usescx_print()for user-facing messages. Consider usingconsole.print()orcx_print()for consistent styling.
196-209: Return type hint could be more specific.The return type
list[Tuple]is vague. A more precise type hint improves IDE support and code clarity. As per coding guidelines, type hints are required.🔎 Suggested fix
- def _get_check_locations(self) -> list[Tuple]: + def _get_check_locations(self) -> list[Tuple[str | Path, list[str]]]:cortex/cli.py (1)
143-153: Calling private method_save_config()is a code smell.The code correctly calls
_save_config()but this is a private method ofNotificationManager. Consider adding a publicsave()orpersist()method toNotificationManagerto maintain proper encapsulation. The existing comment acknowledges this.tests/test_api_key_detector.py (2)
328-337: Complex mock for__init__could be simplified.The lambda on line 332 that patches
__init__is clever but hard to read. Consider usingpatch.objecton thedetectmethod directly instead, or creating a proper mock class.🔎 Alternative approach
def test_auto_detect_api_key(self): """Test auto_detect_api_key function.""" with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test"}, clear=True): with patch("cortex.api_key_detector.APIKeyDetector.detect") as mock_detect: mock_detect.return_value = (True, "sk-ant-test", "anthropic", "environment") found, key, provider, source = auto_detect_api_key() assert found is True assert key == "sk-ant-test" assert provider == "anthropic"
291-298: Consider adding test for Ollama local mode selection.The
prompt_for_keytests cover Anthropic selection but not the Ollama option (choice "3"). Consider adding a test to verify(True, "ollama-local", "ollama")is returned.🔎 Suggested test
@patch("cortex.api_key_detector.cx_print") def test_prompt_for_key_ollama(self, mock_print, detector): """Test selecting Ollama local mode.""" with patch("builtins.input", return_value="3"): entered, key, provider = detector.prompt_for_key() assert entered is True assert key == "ollama-local" assert provider == "ollama"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdcortex/api_key_detector.pycortex/cli.pydocs/ZERO_CONFIG_API_KEYS.mdtests/test_api_key_detector.pytests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/test_cli.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pytests/test_api_key_detector.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.py
🧬 Code graph analysis (3)
cortex/cli.py (1)
cortex/api_key_detector.py (2)
auto_detect_api_key(396-404)setup_api_key(407-432)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (10)
APIKeyDetector(42-393)auto_detect_api_key(396-404)setup_api_key(407-432)detect(56-106)_extract_key_from_file(211-245)_cache_key_location(302-323)_save_key_to_env(376-393)_maybe_save_found_key(170-194)_get_provider_from_var(291-293)prompt_for_key(108-168)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(23-27)
🪛 LanguageTool
docs/ZERO_CONFIG_API_KEYS.md
[grammar] ~252-~252: Ensure spelling is correct
Context: ...N_WIZARD.md](FIRST_RUN_WIZARD.md). ### Environment Management API keys can also be manage...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ZERO_CONFIG_API_KEYS.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
cortex/api_key_detector.py (7)
1-16: Module docstring is clear and informative.The module-level documentation clearly explains the purpose, detection order, and caching behavior.
26-36: Key pattern ordering is correct but relies on insertion order.The
sk-ant-prefix is listed beforesk-, ensuring Anthropic keys are correctly identified before the more general OpenAI prefix. This works due to Python 3.7+ dict insertion order preservation, but consider adding a comment to make this dependency explicit.
170-194: LGTM!The logic correctly avoids prompting to save when the key is already in the canonical location. Exception handling for path resolution edge cases is appropriate.
247-265: LGTM!The JSON and env-format extraction methods handle their respective formats correctly. The regex pattern on line 261 properly handles inline comments.
302-323: LGTM! Secure caching implementation.The cache stores only a key hint (first 10 chars) rather than the full key, and sets 0o600 permissions. Silent exception handling is appropriate for non-critical caching operations.
376-393: LGTM!The save logic correctly handles both updating existing keys and appending new ones. Error handling with a warning message is appropriate - the operation shouldn't crash if save fails.
396-431: LGTM!The convenience functions provide a clean public API. The
setup_api_keyfunction correctly orchestrates detection and prompting with appropriate messaging.docs/ZERO_CONFIG_API_KEYS.md (1)
1-274: Documentation is comprehensive and well-organized.The documentation covers all aspects of the zero-config feature including detection locations, file formats, usage examples, configuration, security considerations, and troubleshooting. The examples are practical and helpful.
cortex/cli.py (4)
9-9: LGTM!The import of
auto_detect_api_keyandsetup_api_keyfrom the new detector module is correct.
46-68: LGTM!The updated
_get_api_key()method cleanly integrates with the new detector module. The flow handles explicit providers first, then auto-detection, with helpful error messages when no key is found.
70-90: LGTM!The provider resolution logic correctly prioritizes explicit provider settings, then uses the detected provider from auto-detection, with sensible fallbacks.
119-186: Notify command implementation is solid.The command handles all subcommands properly with appropriate validation. The time format validation for DND and the graceful handling of missing subcommands are good additions.
tests/test_api_key_detector.py (7)
1-16: LGTM!The module docstring and imports are well-organized. Using pytest fixtures and tempfile for isolation is the correct approach.
22-72: Well-designed test fixtures and helpers.The fixtures provide proper isolation with temporary directories, and the helper methods reduce duplication while keeping tests readable.
73-90: LGTM!Tests for environment variable detection cover both Anthropic and OpenAI providers with proper assertions.
91-99: Minor redundant patching but tests work correctly.The test at line 98 calls
_detect_with_mocked_homewhich patchesPath.homeagain inside an existingPath.homepatch. This is harmless but slightly redundant. The test logic is correct.
207-215: Security-critical permission test is well-implemented.Testing that the cache file has 0o600 permissions ensures the security requirement is verified. Good practice.
261-283: Good test coverage for save prompt behavior.Tests verify both the positive case (user accepts save) and the skip case (key already in canonical location). Mocking
_save_key_to_envverifies the correct method is called.
339-378: Good test coverage forsetup_api_keyfunction.Tests cover the three main paths: successful auto-detection, fallback to manual prompt, and complete failure. The mock assertions verify the correct methods are called.
There was a problem hiding this comment.
@divanshu-go Kindly complete your CLA verification by following the pattern of the PR #401 & address bot comments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_api_key_detector.py (2)
12-12: Remove unused import.The
MagicMockimport is not used in this test file.Proposed fix
-from unittest.mock import patch, MagicMock +from unittest.mock import patch
334-343: Refactor complex lambda expression for mocking.The lambda at line 338 uses
setattrchained withor, which is a code smell. This relies onsetattrreturningNoneto chain operations and is hard to read.Proposed fix
def test_auto_detect_api_key(self): """Test auto_detect_api_key function.""" with tempfile.TemporaryDirectory() as tmpdir: - # Use fresh detector with isolated cache - with patch("cortex.api_key_detector.APIKeyDetector.__init__", lambda self, cache_dir=None: setattr(self, "cache_dir", Path(tmpdir)) or setattr(self, "cache_file", Path(tmpdir) / ".api_key_cache")): - with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test"}, clear=True): - found, key, provider, source = auto_detect_api_key() - assert found is True - assert key == "sk-ant-test" - assert provider == "anthropic" + def mock_init(self, cache_dir=None): + self.cache_dir = Path(tmpdir) + self.cache_file = Path(tmpdir) / ".api_key_cache" + + with patch("cortex.api_key_detector.APIKeyDetector.__init__", mock_init): + with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test"}, clear=True): + found, key, provider, source = auto_detect_api_key() + assert found is True + assert key == "sk-ant-test" + assert provider == "anthropic"
🧹 Nitpick comments (6)
tests/test_api_key_detector.py (6)
41-77: Add type hints to helper methods.Per coding guidelines, type hints are required in Python code. The helper methods lack return type annotations.
Proposed fix
- def _setup_detector_with_home(self, temp_home, cortex_env_content=None): + def _setup_detector_with_home(self, temp_home: Path, cortex_env_content: Optional[str] = None) -> APIKeyDetector: """Helper to create detector with mocked home directory.""" - def _setup_config_file(self, temp_home, config_path_parts, content): + def _setup_config_file(self, temp_home: Path, config_path_parts: tuple, content: str) -> None: """Helper to create a config file with content.""" - def _detect_with_mocked_home(self, detector, temp_home): + def _detect_with_mocked_home(self, detector: APIKeyDetector, temp_home: Path) -> Tuple[bool, Optional[str], Optional[str], Optional[str]]: """Helper to run detect with mocked home directory.""" - def _assert_found_key(self, result, expected_key, expected_provider): + def _assert_found_key(self, result: Tuple[bool, Optional[str], Optional[str], Optional[str]], expected_key: str, expected_provider: str) -> None: """Helper to assert successful key detection.""" - def _assert_env_contains(self, temp_home, expected_lines, unexpected_lines=None): + def _assert_env_contains(self, temp_home: Path, expected_lines: list[str], unexpected_lines: Optional[list[str]] = None) -> None: """Helper to assert .env file content."""As per coding guidelines.
97-105: Remove redundant Path.home patching.The
_detect_with_mocked_homehelper already patchesPath.homeinternally (line 59), so the outer patch at line 103 is redundant and adds unnecessary nesting.Proposed fix
def test_detect_from_cortex_env_file(self, temp_home): """Test detection from ~/.cortex/.env file.""" detector = self._setup_detector_with_home( temp_home, "ANTHROPIC_API_KEY=sk-ant-fromfile123\n" ) - with patch("pathlib.Path.home", return_value=temp_home): - result = self._detect_with_mocked_home(detector, temp_home) - self._assert_found_key(result, "sk-ant-fromfile123", "anthropic") + result = self._detect_with_mocked_home(detector, temp_home) + self._assert_found_key(result, "sk-ant-fromfile123", "anthropic")
107-118: Remove redundant Path.home patching.Same issue as in
test_detect_from_cortex_env_file: the outer patch at line 116 is redundant since_detect_with_mocked_homealready patchesPath.home.Proposed fix
def test_detect_from_anthropic_config(self, temp_home): """Test detection from ~/.config/anthropic (Claude CLI location).""" detector = self._setup_detector_with_home(temp_home) self._setup_config_file( temp_home, (".config", "anthropic", "credentials.json"), json.dumps(TEST_CREDENTIALS_ANTHROPIC) ) - with patch("pathlib.Path.home", return_value=temp_home): - result = self._detect_with_mocked_home(detector, temp_home) - self._assert_found_key(result, TEST_CREDENTIALS_ANTHROPIC["api_key"], "anthropic") + result = self._detect_with_mocked_home(detector, temp_home) + self._assert_found_key(result, TEST_CREDENTIALS_ANTHROPIC["api_key"], "anthropic")
120-131: Remove redundant Path.home patching.Same redundant patching pattern as previous tests.
Proposed fix
def test_detect_from_openai_config(self, temp_home): """Test detection from ~/.config/openai.""" detector = self._setup_detector_with_home(temp_home) self._setup_config_file( temp_home, (".config", "openai", "credentials.json"), json.dumps(TEST_CREDENTIALS_OPENAI) ) - with patch("pathlib.Path.home", return_value=temp_home): - result = self._detect_with_mocked_home(detector, temp_home) - self._assert_found_key(result, TEST_CREDENTIALS_OPENAI["api_key"], "openai") + result = self._detect_with_mocked_home(detector, temp_home) + self._assert_found_key(result, TEST_CREDENTIALS_OPENAI["api_key"], "openai")
149-164: Remove redundant Path.home patching.Same redundant patching pattern as previous tests.
Proposed fix
def test_priority_order(self, temp_home): """Test that detection respects priority order.""" # Create keys in multiple locations detector = self._setup_detector_with_home( temp_home, "ANTHROPIC_API_KEY=sk-ant-cortex\n" ) self._setup_config_file( temp_home, (".config", "anthropic", "credentials.json"), json.dumps(TEST_CREDENTIALS_ANTHROPIC_ALT) ) - with patch("pathlib.Path.home", return_value=temp_home): - # Should find cortex/.env first (higher priority) - result = self._detect_with_mocked_home(detector, temp_home) - self._assert_found_key(result, "sk-ant-cortex", "anthropic") + # Should find cortex/.env first (higher priority) + result = self._detect_with_mocked_home(detector, temp_home) + self._assert_found_key(result, "sk-ant-cortex", "anthropic")
175-188: Consider using context manager for temporary file cleanup.While the try/finally pattern ensures cleanup, using a context manager with
delete=Trueand accessing the file by name would be cleaner and more Pythonic.Example refactor
def test_extract_key_from_env_file(self, detector): """Test extracting key from .env format file.""" - with tempfile.NamedTemporaryFile(mode="w", suffix=".env", delete=False) as f: + with tempfile.NamedTemporaryFile(mode="w", suffix=".env", delete=False) as tmp: - f.write("ANTHROPIC_API_KEY=sk-ant-test123\n") - f.write("OTHER_VAR=value\n") - f.flush() - - try: - key = detector._extract_key_from_file( - Path(f.name), "ANTHROPIC_API_KEY" - ) - assert key == "sk-ant-test123" - finally: - os.unlink(f.name) + tmp.write("ANTHROPIC_API_KEY=sk-ant-test123\n") + tmp.write("OTHER_VAR=value\n") + tmp.flush() + tmp_path = Path(tmp.name) + + try: + key = detector._extract_key_from_file(tmp_path, "ANTHROPIC_API_KEY") + assert key == "sk-ant-test123" + finally: + tmp_path.unlink()Or use
tempfile.TemporaryDirectory()with a known filename inside it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/api_key_detector.pytests/test_api_key_detector.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_api_key_detector.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.py
🧬 Code graph analysis (2)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (9)
APIKeyDetector(45-409)auto_detect_api_key(412-420)setup_api_key(423-448)detect(59-83)_extract_key_from_file(227-261)_cache_key_location(318-339)_save_key_to_env(392-409)_maybe_save_found_key(186-210)prompt_for_key(124-184)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(29-33)
divanshu-go
force-pushed
the
feat-api-key
branch
from
a6a235c to
40ad96c
Compare
divanshu-go
force-pushed
the
feat-api-key
branch
2 times, most recently
from
da79b2d to
e50e09f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
docs/ZERO_CONFIG_API_KEYS.md (1)
50-53: Add language specifier to fenced code block.The code block is missing a language specifier.
🔎 Add language specifier
**Raw key** (single line): -``` +```text sk-ant-api03-...</details> </blockquote></details> <details> <summary>cortex/cli.py (1)</summary><blockquote> `9-9`: **Remove unused import.** The `auto_detect_api_key` import is not used in this file. Only `setup_api_key` is utilized. <details> <summary>🔎 Remove unused import</summary> ```diff -from cortex.api_key_detector import auto_detect_api_key, setup_api_key +from cortex.api_key_detector import setup_api_keytests/test_api_key_detector.py (2)
12-12: Remove unused import.
MagicMockis imported but not used in this test file.🔎 Remove unused import
-from unittest.mock import MagicMock, patch +from unittest.mock import patch
316-330: Refactor complex lambda expression for better readability.The lambda expression for mocking
APIKeyDetector.__init__chainssetattrcalls usingor, which is hard to read and relies onsetattrreturningNone. Consider using a proper mock factory function or fixture.🔎 Suggested refactor
def test_auto_detect_api_key(self): """Test auto_detect_api_key function.""" with tempfile.TemporaryDirectory() as tmpdir: - # Use fresh detector with isolated cache - with patch( - "cortex.api_key_detector.APIKeyDetector.__init__", - lambda self, cache_dir=None: setattr(self, "cache_dir", Path(tmpdir)) - or setattr(self, "cache_file", Path(tmpdir) / ".api_key_cache"), - ): + def mock_init(self, cache_dir=None): + self.cache_dir = Path(tmpdir) + self.cache_file = Path(tmpdir) / ".api_key_cache" + + with patch("cortex.api_key_detector.APIKeyDetector.__init__", mock_init): with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test"}, clear=True): found, key, provider, source = auto_detect_api_key() assert found is True assert key == "sk-ant-test" assert provider == "anthropic"cortex/api_key_detector.py (6)
26-30: Pattern ordering relies on dictionary insertion order.The
KEY_PATTERNSdictionary has overlapping prefixes ("sk-ant-" and "sk-"). While the current order correctly checks the more specific pattern first due to Python 3.7+ dict ordering, this could be fragile. The matching methods (_is_valid_key,_get_provider_from_key) should explicitly sort by prefix length to ensure robustness.
108-168: Consider adding retry loop for invalid provider choice.When a user enters an invalid choice (not "1", "2", or "3"), the method prints a warning and returns, requiring the user to restart the entire command. A retry loop would improve user experience by allowing immediate correction.
291-304: Sort prefixes by length to handle overlapping patterns.Both
_is_valid_keyand_get_provider_from_keyshould explicitly sort patterns by prefix length (descending) to ensure more specific patterns like "sk-ant-" are checked before "sk-".🔎 Example refactor for _get_provider_from_key
def _get_provider_from_key(self, key: str) -> str | None: - """Get provider name from API key format.""" - for prefix, provider in KEY_PATTERNS.items(): + """Get provider name from API key format. + + Checks longer prefixes first to handle overlapping patterns. + """ + for prefix, provider in sorted( + KEY_PATTERNS.items(), key=lambda item: len(item[0]), reverse=True + ): if key.startswith(prefix): return provider return None
306-351: Cache operations are not thread-safe.The cache file operations lack file locking or atomic write operations (write to temp, then rename). While this is generally acceptable for a CLI tool that runs sequentially, concurrent Cortex processes could face race conditions. Consider adding locking for production environments with high concurrency.
411-436: Consider using brand name mapping for display.Line 427 uses
provider.upper()which gives "ANTHROPIC" and "OPENAI". Consider using a mapping dictionary (e.g.,{"anthropic": "Anthropic", "openai": "OpenAI"}) for proper brand capitalization.
251-261: Add explanatory comment to exception handler.The except clause on lines 259-260 silently passes without explanation. While the behavior is correct, an explanatory comment would improve code clarity.
🔎 Add comment
except (json.JSONDecodeError, ValueError): + # Content is not valid JSON; return None to let callers try other formats pass return None
🧹 Nitpick comments (1)
cortex/cli.py (1)
70-90: Consider documenting the method call order dependency.The
_get_providermethod relies on_detected_providerbeing set by_get_api_key. While the current usage pattern ensures_get_api_keyis called first, this implicit dependency is not documented and could lead to subtle bugs if the call order changes.Consider adding a docstring comment noting this dependency, or refactoring to make the dependency more explicit (e.g., by having
_get_provideraccept the detected provider as an optional parameter).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdcortex/api_key_detector.pycortex/cli.pydocs/ZERO_CONFIG_API_KEYS.mdtests/test_api_key_detector.pytests/test_cli.pytests/test_cli_extended.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tests/test_cli_extended.py
- tests/test_cli.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pytests/test_api_key_detector.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.py
🧬 Code graph analysis (3)
cortex/cli.py (1)
cortex/api_key_detector.py (2)
auto_detect_api_key(400-408)setup_api_key(411-436)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (9)
APIKeyDetector(42-397)auto_detect_api_key(400-408)setup_api_key(411-436)detect(56-106)_extract_key_from_file(215-249)_cache_key_location(306-327)_save_key_to_env(380-397)_maybe_save_found_key(170-198)prompt_for_key(108-168)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(23-27)
🪛 LanguageTool
docs/ZERO_CONFIG_API_KEYS.md
[grammar] ~261-~261: Ensure spelling is correct
Context: ...N_WIZARD.md](FIRST_RUN_WIZARD.md). ### Environment Management API keys can also be manage...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ZERO_CONFIG_API_KEYS.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (7)
- GitHub Check: Test (Python 3.10)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Analyze Python
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (4)
cortex/cli.py (2)
46-68: LGTM!The API key retrieval flow is well-structured with clear provider handling and helpful error messages guiding users to either run the wizard or use offline mode.
120-186: LGTM with acknowledged technical debt.The notify command implementation includes proper input validation (time format checking) and error handling. The comment on lines 145-146 acknowledges that using the private
_save_configmethod is technical debt. While ideally a public method should be added toNotificationManager, the current approach is acceptable for this PR.tests/test_api_key_detector.py (1)
19-371: Excellent test coverage and structure.The test suite is comprehensive, covering environment detection, file-based detection, priority ordering, caching, persistence, and user prompts. The use of helper methods and fixtures makes the tests readable and maintainable.
cortex/api_key_detector.py (1)
56-106: LGTM!The detection logic is well-structured, checking cached locations first for performance, then falling back to priority-ordered checks. Setting environment variables for the session (lines 80, 103) ensures consistency within the process.
divanshu-go
force-pushed
the
feat-api-key
branch
from
e50e09f to
5bf671b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
cortex/cli.py (1)
9-9: Remove unused import.The
auto_detect_api_keyfunction is imported but never used in this file. Onlysetup_api_keyis called (line 57).🔎 Remove unused import
-from cortex.api_key_detector import auto_detect_api_key, setup_api_key +from cortex.api_key_detector import setup_api_keycortex/api_key_detector.py (5)
32-35: Consider explicit prefix length sorting for clarity.The KEY_PATTERNS dictionary relies on insertion order to match "sk-ant-" before "sk-". While this works correctly, explicitly sorting by prefix length (longest first) in matching logic would make the behavior more predictable and maintainable.
Based on past review comments.
374-395: Cache operations are acceptable for CLI usage.The cache file operations lack thread-safety mechanisms (no file locking), but this is reasonable for a CLI tool that typically runs sequentially. The silent error handling (line 393-395) ensures resilience if issues occur.
For completeness, consider adding atomic write operations (write to temp, then rename) if concurrent access becomes a concern in the future.
Based on past review comments.
179-198: Consider adding retry loop for invalid provider choice.When a user enters an invalid choice (not "1", "2", or "3"), the method returns
None(line 197), requiring the user to restart the entire command. Adding a retry loop would improve user experience.Based on past review comments.
478-504: Consider brand-consistent provider capitalization.Line 495 uses
provider.upper()which converts "openai" to "OPENAI", but the brand name is "OpenAI". Consider using a display name mapping for consistency:PROVIDER_DISPLAY_NAMES = {"anthropic": "Claude (Anthropic)", "openai": "OpenAI"}Based on past review comments.
319-329: Add explanatory comment to exception handler.The exception handler at lines 327-328 silently catches JSON decode errors. While the behavior is correct (returning None to try other formats), an explanatory comment would improve maintainability.
🔎 Add comment
except (json.JSONDecodeError, ValueError): + # Content is not valid JSON; return None to try other extraction methods passBased on past review comments.
tests/test_api_key_detector.py (2)
12-12: Remove unused import.
MagicMockis imported but never used in this test file. Onlypatchis utilized.🔎 Remove unused import
-from unittest.mock import MagicMock, patch +from unittest.mock import patchBased on past review comments.
316-329: Consider refactoring complex lambda to a helper function.The lambda expression at lines 321-323 chains
setattrcalls usingor, which works but is hard to read. Consider extracting to a helper function for clarity.🔎 Refactor to helper function
+ def test_auto_detect_api_key(self): + """Test auto_detect_api_key function.""" + def mock_detector_init(self, cache_dir=None): + self.cache_dir = Path(tmpdir) + self.cache_file = Path(tmpdir) / ".api_key_cache" + with tempfile.TemporaryDirectory() as tmpdir: - # Use fresh detector with isolated cache with patch( "cortex.api_key_detector.APIKeyDetector.__init__", - lambda self, cache_dir=None: setattr(self, "cache_dir", Path(tmpdir)) - or setattr(self, "cache_file", Path(tmpdir) / ".api_key_cache"), + mock_detector_init, ):Based on past review comments.
docs/ZERO_CONFIG_API_KEYS.md (1)
50-53: Add language specifier to code block.The code block is missing a language specifier, which markdownlint flagged. Add
textafter the opening fence.🔎 Add language specifier
**Raw key** (single line): -``` +```text sk-ant-api03-...</details> Based on past review comments and static analysis. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>cortex/cli.py (1)</summary><blockquote> `46-68`: **Initialize `_detected_provider` in `__init__`.** The attribute `self._detected_provider` is set at line 61 but is never initialized in the `__init__` method (lines 36-39). If `_get_provider()` is called before `_get_api_key()`, line 77 will use `getattr(self, "_detected_provider", None)` which safely returns `None`, but explicit initialization in `__init__` would make the code clearer and more maintainable. <details> <summary>🔎 Initialize in __init__</summary> ```diff def __init__(self, verbose: bool = False): self.spinner_chars = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"] self.spinner_idx = 0 self.verbose = verbose + self._detected_provider = Nonetests/test_cli_extended.py (2)
28-29: Add type hint to tearDown method.The
tearDownmethod is missing a return type hint. Per coding guidelines, type hints are required in Python code.🔎 Proposed fix
- def tearDown(self): + def tearDown(self) -> None: self._temp_dir.cleanup()
47-47: Consider parameterized test or constant for menu choice.Hard-coding
"3"for the Ollama menu option is brittle if the prompt changes. Consider using a constant or adding a comment that references the expected menu structure.💡 Example: add a constant
# At module level OLLAMA_MENU_CHOICE = "3" # Expected menu option for Ollama in setup_api_key prompt # In test with patch("builtins.input", return_value=OLLAMA_MENU_CHOICE):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdcortex/api_key_detector.pycortex/cli.pydocs/ZERO_CONFIG_API_KEYS.mdtests/test_api_key_detector.pytests/test_cli.pytests/test_cli_extended.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 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/cli.pytests/test_cli_extended.pytests/test_cli.pytests/test_api_key_detector.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_cli_extended.pytests/test_cli.pytests/test_api_key_detector.py
🧬 Code graph analysis (5)
cortex/cli.py (2)
cortex/api_key_detector.py (2)
auto_detect_api_key(468-476)setup_api_key(479-504)cortex/first_run_wizard.py (1)
_print_error(746-748)
tests/test_cli_extended.py (1)
cortex/cli.py (2)
_get_api_key(46-68)_get_provider(70-90)
tests/test_cli.py (1)
cortex/cli.py (2)
_get_api_key(46-68)_get_provider(70-90)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (6)
detect(61-79)_extract_key_from_file(283-317)_cache_key_location(374-395)_save_key_to_env(448-465)_maybe_save_found_key(238-266)prompt_for_key(158-177)
cortex/api_key_detector.py (1)
cortex/branding.py (1)
cx_print(49-69)
🪛 LanguageTool
docs/ZERO_CONFIG_API_KEYS.md
[grammar] ~261-~261: Ensure spelling is correct
Context: ...N_WIZARD.md](FIRST_RUN_WIZARD.md). ### Environment Management API keys can also be manage...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ZERO_CONFIG_API_KEYS.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (10)
cortex/cli.py (2)
70-90: LGTM!The provider resolution logic correctly prioritizes explicit configuration, then auto-detected provider, then environment variables, with a sensible fallback to Ollama for offline mode.
120-186: Notification management implementation looks good.The
notifymethod handles all subcommands with proper validation:
- Missing subcommand handled (lines 123-125)
- Time format validation for DND window (lines 162-168)
- Consistent error handling throughout
Note: This notification feature appears unrelated to the PR's stated objective of zero-config API key detection. Consider whether this should be in a separate PR.
cortex/api_key_detector.py (1)
47-504: Type hints and docstrings are complete.All public APIs have appropriate type hints and docstrings as required by coding guidelines. Private methods include type hints, which is good practice.
docs/ZERO_CONFIG_API_KEYS.md (1)
1-283: Documentation is comprehensive and well-structured.The documentation clearly explains:
- Auto-detection flow and priority order
- Supported file formats with examples
- Usage patterns including prompts and fallbacks
- Troubleshooting guidance
- Integration with other Cortex features
The content accurately reflects the implementation in cortex/api_key_detector.py.
tests/test_cli.py (3)
14-21: LGTM - Proper test isolation.The setUp and tearDown methods create filesystem isolation using a temporary directory, preventing tests from interfering with the user's actual configuration files.
23-46: Test updates correctly reflect new auto-detection flow.The tests now:
- Patch
Path.homefor filesystem isolation (lines 25, 35, 43)- Mock user input for provider selection when no key found (line 44)
- Expect "ollama-local" for Ollama fallback (line 46)
This aligns with the new auto-detection behavior introduced in cortex/api_key_detector.py.
48-64: Provider tests correctly establish preconditions.Tests call
_get_api_key()before_get_provider()(lines 52, 62) to populate_detected_provider, which reflects the actual usage pattern in the CLI where API key detection happens before provider resolution.tests/test_api_key_detector.py (1)
19-371: Comprehensive test coverage.The test suite covers all major functionality:
- Detection from all 5 locations with correct priority order
- File format extraction (JSON, .env, raw)
- Cache operations with permission checks
- Key persistence with append/replace logic
- User prompts and provider selection
- Error handling and edge cases
This meets the >80% coverage requirement specified in coding guidelines.
tests/test_cli_extended.py (2)
51-65: Test pattern correctly reflects internal state dependencies.The tests appropriately call
_get_api_key()before_get_provider()to populate the internal_detected_providerattribute. This coupling is well-documented in the comments and reflects the actual CLI flow.
67-81: Test correctly validates override and fallback behavior.This test properly checks that explicit
CORTEX_PROVIDERtakes precedence, and that removing it causes the system to fall back to auto-detected providers. The pattern of repopulating internal state after removing the override is correct.
…ved API key selection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cortex/api_key_detector.py (1)
368-381: Prefix ordering concern applies here.As noted in the earlier comment on
KEY_PATTERNS, these methods should sort prefixes by length (descending) to ensure correct matching of overlapping prefixes like"sk-ant-"vs"sk-".
🧹 Nitpick comments (1)
cortex/api_key_detector.py (1)
31-35: Prefix ordering relies on dict insertion order, which is fragile.Both
"sk-ant-"and"sk-"prefixes start with"sk-". The methods_is_valid_key(line 370) and_get_provider_from_key(line 378) iterate overKEY_PATTERNSand return on the first match. If iteration order changes or is not what you expect, an Anthropic key could incorrectly match"sk-"(OpenAI).Sort prefixes by length (descending) when matching to ensure the more specific prefix is checked first, as suggested in the past review comments.
🔎 Proposed fix: Sort by length in matching methods
def _is_valid_key(self, value: str) -> bool: """Check if a value matches known API key patterns.""" - return any(value.startswith(prefix) for prefix in KEY_PATTERNS.keys()) + return any(value.startswith(prefix) for prefix in sorted(KEY_PATTERNS.keys(), key=len, reverse=True)) ... def _get_provider_from_key(self, key: str) -> str | None: """Get provider name from API key format.""" - for prefix, provider in KEY_PATTERNS.items(): + for prefix, provider in sorted(KEY_PATTERNS.items(), key=lambda x: len(x[0]), reverse=True): if key.startswith(prefix): return provider return None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/api_key_detector.pytests/test_api_key_detector.pytests/test_cli.pytests/test_cli_extended.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_cli.py
- tests/test_api_key_detector.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/api_key_detector.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 (1)
cortex/cli.py (2)
_get_api_key(46-68)_get_provider(70-90)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(23-27)
⏰ 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.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (14)
cortex/api_key_detector.py (9)
1-24: LGTM!Module docstring clearly documents the detection priority and caching behavior. Imports are appropriate for the functionality.
53-85: LGTM!Clean initialization with sensible defaults. The
detect()method has clear cache-first semantics with proper fallback.
87-110: LGTM!Cache validation logic correctly handles both environment and file-based sources. Setting the environment variable on line 108 ensures session consistency.
112-162: LGTM!Well-decomposed location checking with clear separation between environment and file-based sources. The session-level environment variable setting is appropriately documented.
164-245: LGTM!Good implementation of the interactive prompt flow with retry loop for invalid choices (addressing past feedback). Error handling for EOF/KeyboardInterrupt is consistent throughout.
277-366: LGTM!Comprehensive extraction logic covering JSON, KEY=value, and raw key formats. The prioritized location list matches the documented detection order. JSON error handling is properly addressed.
383-428: LGTM with acknowledged caveat.Cache operations are not thread-safe (as noted in past comments), but this is acceptable for a CLI tool typically run sequentially. File permissions are correctly set to 0o600.
430-474: LGTM!Well-decomposed file handling with proper permissions (0o600). The
_update_or_append_keymethod correctly handles both updating existing keys and appending new ones.
477-514: LGTM!Clean public API with
auto_detect_api_key()for simple detection andsetup_api_key()for the full interactive flow. The logic to suppress the "Found" message for the canonical location is a nice UX touch.tests/test_cli_extended.py (5)
22-29: LGTM!Good test isolation using a temporary directory for cache. The manual cleanup in
tearDownis appropriate for the unittest pattern.
43-51: Good test coverage for Ollama fallback.Using
PROVIDER_MENU_CHOICES["ollama"]instead of hardcoding"3"is a nice touch for maintainability. Note: thePath.homepatch target issue flagged above also applies here (line 48).
53-83: LGTM — tests correctly populate internal state before assertions.The pattern of calling
_get_api_key()before asserting on_get_provider()correctly reflects the CLI's internal state flow where detection populates_detected_provider. Thetest_get_provider_overridetest covers both explicit override and fallback to detected provider.Note: Apply the
Path.homepatch target fix from the earlier comment to lines 55, 63, and 75.
85-328: LGTM!Remaining tests provide good coverage for CLI printing methods, install scenarios, and main function entry points. The use of
@patch.objectto mock CLI methods directly is appropriate.
31-36: The patch target"pathlib.Path.home"is functionally correct and will work as intended.When
api_key_detector.pyimports withfrom pathlib import Path(line 21), it creates a reference to the samepathlib.Pathclass. Patching"pathlib.Path.home"patches the classmethod at the source, which affects all references to it, including those inapi_key_detector.py. Tests will not access the real home directory.The identical patch pattern appears in
tests/test_api_key_detector.py(lines 97, 110, etc.), suggesting this approach is functional and established in your test suite.While patching
"cortex.api_key_detector.Path.home"would follow unittest.mock best practices for patching at the point of use, the current approach is not broken.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_api_key_detector.py (1)
73-80: Consider addingclear=Truefor environment isolation.Line 84 in the OpenAI test uses
clear=Trueto ensure a clean environment, but line 75 in this test doesn't. For consistency and to prevent potential test pollution from the host environment, consider addingclear=Truehere as well.🔎 Proposed fix
- with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test123"}): + with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "sk-ant-test123"}, clear=True):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/api_key_detector.pytests/test_api_key_detector.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_api_key_detector.pycortex/api_key_detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.py
🧬 Code graph analysis (2)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (10)
APIKeyDetector(53-493)auto_detect_api_key(496-504)setup_api_key(507-533)detect(67-85)_extract_key_from_file(292-326)_cache_key_location(397-420)_save_key_to_env(473-493)_maybe_save_found_key(247-275)_get_provider_from_var(372-374)prompt_for_key(164-183)
cortex/api_key_detector.py (2)
cortex/branding.py (1)
cx_print(49-69)tests/test_api_key_detector.py (1)
detector(23-27)
⏰ 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.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (19)
tests/test_api_key_detector.py (7)
1-16: LGTM!The module structure, imports, and documentation are well-organized. The imports are appropriate for the test suite's needs.
19-72: Well-structured test fixtures and helpers.The fixture design and helper methods effectively reduce duplication and improve test readability. The use of temporary directories ensures test isolation.
91-142: Comprehensive coverage of file-based detection sources.The tests effectively validate detection from all documented file locations (cortex env, anthropic config, openai config, and current directory), with proper isolation using temporary directories and environment mocking.
143-166: Priority ordering and edge cases well-tested.The priority test validates the documented detection order, and the no-key-found test properly handles the negative case.
167-210: Thorough testing of extraction and caching mechanisms.The tests validate both file format parsing (env and JSON) and cache behavior, including important security aspects like file permissions.
211-272: Comprehensive persistence testing.The save tests cover all key scenarios (new, append, replace) and properly validate the skip logic when keys are already in the canonical location.
273-371: Thorough testing of user interaction and public API.The tests comprehensively validate user prompts, provider detection, and the convenience functions (
auto_detect_api_key,setup_api_key), covering both success and failure paths.cortex/api_key_detector.py (12)
1-51: Clear documentation and well-organized constants.The module docstring effectively explains the detection priority, and the constants are logically grouped. The PROVIDER_DISPLAY_NAMES dictionary ensures consistent branding as suggested in past reviews.
53-86: Clean class design with clear separation of concerns.The initialization and main detection flow are well-structured, with cache-first optimization and clear documentation of the return tuple.
87-111: Proper cache validation with source handling.The cache checking logic correctly validates both environment and file-based sources, and appropriately sets keys in the environment for the current session when loaded from files.
112-162: Well-structured location checking with proper caching.The detection logic correctly prioritizes sources, caches results, and maintains session-wide environment variables for discovered keys.
164-230: Excellent user interaction with retry logic.The prompt flow implements the retry loop suggested in past reviews, provides clear guidance, and properly validates key formats against the selected provider.
232-276: Smart save prompting with good UX.The save logic appropriately skips prompting for environment variables and keys already in the canonical location. The f-string issues noted in past reviews have been corrected.
277-290: Priority order matches documented behavior.The location list correctly implements the documented detection priority, with appropriate environment variable mappings for each source.
292-366: Robust key extraction with multiple format support.The extraction logic properly handles JSON, env-file, and raw key formats with appropriate fallback behavior. The explanatory comment for JSON exception handling addresses past review feedback.
368-381: Provider detection works correctly.The validation and provider detection logic functions properly. While past reviews suggested sorting prefixes by length to handle overlapping patterns more robustly, the current implementation correctly relies on dictionary insertion order (guaranteed in Python 3.7+) to check more specific patterns first.
383-444: Thread-safe file operations with atomic writes.The atomic write implementation addresses the concurrency concerns raised in past reviews, using the write-to-temp-then-rename pattern for both cache and env file operations. Secure file permissions (0o600) are properly enforced.
446-493: Comprehensive env file handling with atomic operations.The env file methods properly handle read/update/write cycles with atomic operations, addressing past concurrency concerns. The error handling provides user feedback without crashing the application.
495-533: Clean public API with thoughtful UX details.The convenience functions provide a well-designed interface. The decision to only announce keys found in non-default locations avoids unnecessary noise for the typical workflow.
… improved accuracy
|
|
@Anshgrover23 Can I get a review on this PR ? |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@divanshu-go Great PR, nicely done.
2 participants
Related Issue
Closes #255
Summary
Implements automatic API key detection from common locations, eliminating the need for manual environment variable setup.
Detection Priority
ANTHROPIC_API_KEY,OPENAI_API_KEY)~/.cortex/.env~/.config/anthropic/credentials.json(Claude CLI)~/.config/openai/credentials.json(OpenAI CLI).envin current directoryChecklist
pytest tests/)Demo Video
When NO API key present in system
Screen.Recording.Jan.2.2026.MERGE.mp4
When API key present in system
Screen.Recording.2026-01-02.at.4.35.10.PM.mov
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.