feat: add environment variable manager with encryption and templates #339
Conversation
|
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. WalkthroughAdds a per-application environment variable management subsystem: new EnvManager with encryption, validation, templating, import/export, CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Handler
participant EnvMgr as EnvManager
participant File as File Storage
participant Cipher as Encryption
User->>CLI: env set app_name KEY VALUE --encrypt
CLI->>EnvMgr: set(app_name, KEY, VALUE, encrypt=True)
EnvMgr->>EnvMgr: validate(KEY, VALUE)
EnvMgr->>Cipher: _encrypt(VALUE)
Cipher-->>EnvMgr: encrypted_value
EnvMgr->>File: _load_app_env(app_name)
File-->>EnvMgr: existing_vars
EnvMgr->>EnvMgr: create/update EnvVariable
EnvMgr->>File: _save_app_env(app_name, vars) (atomic, perms 0o600)
File-->>EnvMgr: written
EnvMgr-->>CLI: True
CLI-->>User: ✓ Variable set
sequenceDiagram
participant User
participant CLI as CLI Handler
participant EnvMgr as EnvManager
participant File as File Storage
participant Cipher as Encryption
User->>CLI: env apply-template app_name template_name
CLI->>EnvMgr: apply_template(app_name, template_name, vars?)
EnvMgr->>EnvMgr: get_template(template_name)
EnvMgr->>EnvMgr: ensure required_vars present
loop For each template variable
EnvMgr->>EnvMgr: validate(key, value)
alt encrypt flag
EnvMgr->>Cipher: _encrypt(value)
Cipher-->>EnvMgr: encrypted_value
end
end
EnvMgr->>File: _load_app_env(app_name)
EnvMgr->>File: _save_app_env(app_name, merged_vars)
File-->>EnvMgr: written
EnvMgr-->>CLI: Success
CLI-->>User: ✓ Template applied
sequenceDiagram
participant User
participant CLI as CLI Handler
participant EnvMgr as EnvManager
participant File as File Storage
participant Cipher as Encryption
User->>CLI: env export app_name --format json
CLI->>EnvMgr: export(app_name, format="json")
EnvMgr->>File: _load_app_env(app_name)
File-->>EnvMgr: env_vars
loop For each variable
alt encrypted
EnvMgr->>Cipher: _decrypt(value)
Cipher-->>EnvMgr: decrypted_value
end
end
EnvMgr->>EnvMgr: serialize to JSON/YAML/ENV
EnvMgr-->>CLI: serialized_string
CLI-->>User: output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing focused review:
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
.gitignore (1)
208-208: Consider a more generic pattern or remove this entry.This appears to be a specific test/demo artifact. Since
.envis already ignored (lines 140, 159), consider either:
- Using a pattern like
*.envfiles outside the root- Removing this entry if it was accidentally generated during development
If this is intentional for a specific demo file, document why this specific filename is needed.
tests/test_env_manager.py (3)
152-156: Consider testing None handling in RequiredValidator.The test skips
Nonewith a comment about type check failure. IfNonecould realistically be passed to the validator (e.g., from malformed input), the validator should handle it gracefully rather than raising a type error. Consider adding explicit None handling toRequiredValidator.validate().
446-459: Use try/finally for os.environ cleanup.If the test fails before cleanup, environment variables remain set and could affect other tests.
🔎 Proposed fix
def test_load_env_to_os(self): """Test loading environment to os.environ""" self.manager.set("myapp", "TEST_VAR_1", "test_value_1") self.manager.set("myapp", "TEST_VAR_2", "test_value_2") - # Load to os.environ - self.manager.load_env_to_os("myapp") - - self.assertEqual(os.environ.get("TEST_VAR_1"), "test_value_1") - self.assertEqual(os.environ.get("TEST_VAR_2"), "test_value_2") - - # Clean up - del os.environ["TEST_VAR_1"] - del os.environ["TEST_VAR_2"] + try: + # Load to os.environ + self.manager.load_env_to_os("myapp") + + self.assertEqual(os.environ.get("TEST_VAR_1"), "test_value_1") + self.assertEqual(os.environ.get("TEST_VAR_2"), "test_value_2") + finally: + # Clean up + os.environ.pop("TEST_VAR_1", None) + os.environ.pop("TEST_VAR_2", None)
508-519: File permission test is platform-specific.This test assumes Unix-style permissions (
0o600). It will fail on Windows where file permissions work differently. Consider skipping on non-Unix platforms.🔎 Proposed fix
+ @unittest.skipIf(os.name == 'nt', "File permissions test not applicable on Windows") def test_file_permissions(self): """Test that sensitive files have correct permissions"""cortex/cli.py (2)
798-802: Interactive confirmation may block in automated contexts.The
input()call for confirmation will block if the CLI is run non-interactively (e.g., in scripts or CI). Consider adding a--yesor--forceflag to skip confirmation.
616-817: Consider splitting the env() method into smaller handlers.At ~200 lines, this method handles many distinct actions. Extracting helper methods like
_handle_env_set(),_handle_env_list(), etc., would improve readability and testability, similar to how_handle_stack_*methods are structured.examples/env_demo.py (1)
7-11: Move shutil import to module level.
shutilis imported inside each demo function (lines 66, 105, 148, 201, 238). Move it to the module-level imports for cleaner code.🔎 Proposed fix
import os +import shutil import sys import tempfile import secretscortex/env_manager.py (5)
38-39:datetime.utcnow()is deprecated in Python 3.12+.Use
datetime.now(timezone.utc)instead for timezone-aware UTC timestamps.🔎 Proposed fix
+from datetime import datetime, timezone -from datetime import datetime ... - created_at: str = field(default_factory=lambda: datetime.utcnow().isoformat()) - updated_at: str = field(default_factory=lambda: datetime.utcnow().isoformat()) + created_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) + updated_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat())Also update other
datetime.utcnow()calls at lines 298, 451, 457, 490, 496.
128-131: RequiredValidator doesn't handle None values.If
Noneis passed,value.strip()will raiseAttributeError. Add defensive handling.🔎 Proposed fix
def validate(self, key: str, value: str) -> tuple[bool, Optional[str]]: - if value and value.strip(): + if value is not None and value.strip(): return True, None return False, f"{key}: {self.error_message}"
252-258: Specify encoding for atomic file write.The file opened via
os.fdopenshould specify encoding for consistent behavior across platforms.🔎 Proposed fix
- with os.fdopen(fd, "w") as f: + with os.fdopen(fd, "w", encoding="utf-8") as f: json.dump(data, f, indent=2)
242-245: Specify encoding when reading JSON files.Add explicit encoding for consistent behavior.
🔎 Proposed fix
- with open(app_file) as f: + with open(app_file, encoding="utf-8") as f: data = json.load(f)
529-553: Specify encoding for template file operations.The template read/write operations should specify
encoding="utf-8"for consistency.🔎 Proposed fix
def create_template(self, template: EnvTemplate): """Create or update an environment template.""" template_file = self.templates_dir / f"{template.name}.json" - with open(template_file, "w") as f: + with open(template_file, "w", encoding="utf-8") as f: json.dump(template.to_dict(), f, indent=2) def get_template(self, template_name: str) -> Optional[EnvTemplate]: ... - with open(template_file) as f: + with open(template_file, encoding="utf-8") as f: data = json.load(f) ... def list_templates(self) -> List[EnvTemplate]: ... for template_file in self.templates_dir.glob("*.json"): - with open(template_file) as f: + with open(template_file, encoding="utf-8") as f: data = json.load(f)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignorecortex/cli.pycortex/env_manager.pydocs/ENV_MANAGEMENT.mdexamples/env_demo.pyrequirements.txttests/test_env_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_env_manager.pycortex/cli.pyexamples/env_demo.pycortex/env_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.py
🧬 Code graph analysis (2)
tests/test_env_manager.py (1)
cortex/env_manager.py (28)
EnvManager(134-707)EnvTemplate(53-69)EnvVariable(31-49)RegexValidator(88-98)RequiredValidator(122-131)URLValidator(101-119)to_dict(42-44)to_dict(62-64)from_dict(47-49)from_dict(67-69)validate(78-85)validate(95-98)validate(116-119)validate(128-131)validate(643-660)get(321-340)delete(342-359)list_apps(373-382)export(384-428)import_env(430-502)list_templates(546-553)get_template(535-544)create_template(529-533)apply_template(504-527)get_env_dict(662-680)load_env_to_os(682-690)delete_app(692-707)_get_app_file(232-234)
examples/env_demo.py (1)
cortex/env_manager.py (5)
get(321-340)export(384-428)list_templates(546-553)apply_template(504-527)import_env(430-502)
🪛 markdownlint-cli2 (0.18.1)
docs/ENV_MANAGEMENT.md
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
478-478: 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 (10)
tests/test_env_manager.py (3)
1-27: Well-structured test setup.The imports and path setup are appropriate. The test file imports all necessary components from the env_manager module.
29-69: Good dataclass test coverage.The tests verify creation, serialization, and deserialization of
EnvVariableobjects effectively.
522-563: Good integration test coverage.The complete workflow test effectively validates the end-to-end flow of template application, variable management, and export functionality.
cortex/cli.py (2)
17-17: Import added correctly.The
EnvManagerimport is properly placed with other cortex module imports.
1009-1058: Well-structured CLI argument parsing.The env subcommand configuration follows the established patterns in the codebase and provides comprehensive options for all environment management operations.
docs/ENV_MANAGEMENT.md (1)
1-551: Excellent documentation coverage.The documentation comprehensively covers all features including encryption, templates, validation, and integration examples. The best practices section and troubleshooting guide are particularly helpful.
examples/env_demo.py (1)
24-67: Good demo structure with proper cleanup.The demo uses temporary directories and cleans up appropriately. The demonstrations of basic operations, encrypted variables, and export are clear and useful.
cortex/env_manager.py (3)
1-28: Well-documented module with appropriate imports.The module docstring clearly describes the features. Imports are organized and appropriate for the functionality.
171-222: Solid encryption key management with fallback.The implementation properly tries the OS keyring first, falls back to file storage, and ensures proper file permissions. The backup key file creation is a good reliability measure.
134-706: Well-architected environment manager.The
EnvManagerclass is well-designed with clear separation of concerns:
- Atomic file writes with proper cleanup
- Keyring integration with file fallback for encryption keys
- Extensible validation system
- Built-in templates for common frameworks
The implementation follows good security practices with
0o600permissions and proper encryption handling.
requirements.txt
Outdated
| # Encryption | ||
| cryptography>=41.0.0 | ||
|
|
||
| # Optional secure key storage | ||
| keyring>=23.0.0 |
There was a problem hiding this comment.
Dependencies are secure but cryptography minimum version can be updated.
cryptography has no known security issues and keyring is actively maintained. However, consider increasing the cryptography minimum version from 41.0.0 to 44.0.0 or later to take advantage of more recent features and improvements, while keyring>=23.0.0 remains appropriately versioned.
🤖 Prompt for AI Agents
In requirements.txt around lines 14-18, update the cryptography minimum version
from 41.0.0 to 44.0.0 (or later) to pick up recent fixes and features; edit the
line to require cryptography>=44.0.0, keep keyring>=23.0.0 as-is, then
regenerate your lockfile / dependency pins (pip-compile, poetry lock, or pip
freeze) and run the test suite/CI to verify compatibility before merging.
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive environment variable management system for Cortex, providing secure storage and management of application configuration. The system includes encryption for sensitive values, pre-built templates for common frameworks, validation rules, and full CLI integration.
Key Changes
- New
EnvManagerclass with encryption support using Fernet (AES-128-CBC with HMAC) - Environment variable templates for Node.js, Python, Django, and Docker
- Automatic validation for URLs, ports, and required fields
- Import/export functionality supporting .env, JSON, and YAML formats
- CLI commands under
cortex envfor all operations - Comprehensive test suite with 584 lines of tests
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/env_manager.py | Core implementation of environment manager with encryption, validation, templates, and import/export |
| tests/test_env_manager.py | Comprehensive test suite covering all features including encryption, validation, templates, and file operations |
| cortex/cli.py | CLI integration adding env command with subcommands for set, get, list, delete, export, import, template, and clear |
| docs/ENV_MANAGEMENT.md | Complete documentation with usage examples, security notes, and best practices |
| requirements.txt | Added cryptography>=41.0.0 and keyring>=23.0.0 dependencies |
| examples/env_demo.py | Interactive demonstration of all environment management features |
| .gitignore | Added myapp.env to excluded files |
Comments suppressed due to low confidence (1)
cortex/env_manager.py:264
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cortex/env_manager.py
Outdated
| if not self.key_file.exists(): | ||
| self.key_file.write_bytes(self.cipher_key) | ||
| self.key_file.chmod(0o600) |
There was a problem hiding this comment.
The encryption key initialization has a potential race condition. Between lines 215-217, there's a check for if not self.key_file.exists() followed by a write operation. If multiple EnvManager instances are created concurrently (e.g., in a multi-threaded application), they could both pass the exists check and then both attempt to write, potentially causing file corruption or permission errors. Consider using atomic file operations or file locking to prevent this race condition.
| if not self.key_file.exists(): | |
| self.key_file.write_bytes(self.cipher_key) | |
| self.key_file.chmod(0o600) | |
| # Attempt atomic creation to avoid race conditions with concurrent | |
| # EnvManager instances. If the file already exists, do nothing. | |
| fd = os.open(self.key_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) | |
| with os.fdopen(fd, "wb") as f: | |
| f.write(self.cipher_key) | |
| except FileExistsError: | |
| # Another process/thread created the file first; that's fine. | |
| pass |
cortex/env_manager.py
Outdated
| finally: | ||
| # Ensure no tmp file remains | ||
| if os.path.exists(tmp_path): | ||
| try: | ||
| os.remove(tmp_path) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The _save_app_env method creates a temporary file but the cleanup in the finally block may fail silently. If os.remove(tmp_path) raises an exception (lines 262-265), it's caught and ignored. While this prevents crashes, it could lead to accumulation of temporary files in the environments directory. Consider logging this failure or using a more robust cleanup mechanism like Python's TemporaryDirectory context manager.
tests/test_env_manager.py
Outdated
| test_values = [ | ||
| "simple_value", | ||
| "complex!@#$%^&*()value", | ||
| "unicode_表情符号_test", |
There was a problem hiding this comment.
The test_encryption_consistency test uses "unicode_表情符号_test" as a test value (line 499). The Chinese characters translate to "emoji" but the comment "unicode_表情符号_test" doesn't actually contain emoji characters. While this doesn't affect functionality, the test name/value is misleading. Consider using actual emoji characters like "unicode_😀_test" or renaming to "unicode_中文_test" to accurately reflect what's being tested.
| "unicode_表情符号_test", | |
| "unicode_😀_test", |
cortex/cli.py
Outdated
| self._print_error("Usage: cortex env get <app> <key>") | ||
| return 1 | ||
|
|
||
| value = manager.get(args.app, args.key, decrypt=not args.encrypted_value) |
There was a problem hiding this comment.
In the env get command, the parameter name '--encrypted-value' is confusing. When this flag is set to True, it actually shows the encrypted value (doesn't decrypt), but the double-negative logic with 'decrypt=not args.encrypted_value' makes the code hard to understand. Consider renaming the flag to '--raw' or '--no-decrypt' to make the intent clearer and avoid the double-negative pattern.
| value = manager.get(args.app, args.key, decrypt=not args.encrypted_value) | |
| # When --encrypted-value is True, we want to show the raw (encrypted) value, | |
| # so decryption should be disabled. | |
| decrypt_value = not args.encrypted_value | |
| value = manager.get(args.app, args.key, decrypt=decrypt_value) |
cortex/cli.py
Outdated
| confirm = input("Continue? [y/N]: ") | ||
| if confirm.lower() != "y": | ||
| cx_print("Cancelled", "info") |
There was a problem hiding this comment.
The confirmation prompt for the 'clear' action uses input() which reads from stdin, but this won't work properly in non-interactive environments (like CI/CD pipelines or automated scripts). Consider adding a '--force' or '--yes' flag to allow non-interactive clearing, or check if stdin is a TTY before prompting.
examples/env_demo.py
Outdated
|
|
||
| # Get specific variable | ||
| console.print("\n[bold]Getting decrypted API_KEY...[/bold]") | ||
| api_key = manager.get("myapp", "API_KEY", decrypt=True) |
There was a problem hiding this comment.
Variable api_key is not used.
| api_key = manager.get("myapp", "API_KEY", decrypt=True) | |
| manager.get("myapp", "API_KEY", decrypt=True) |
cortex/env_manager.py
Outdated
| from typing import Any, Dict, List, Optional | ||
|
|
||
| from cryptography.fernet import Fernet | ||
| from cryptography.hazmat.primitives import hashes |
There was a problem hiding this comment.
Import of 'hashes' is not used.
| from cryptography.hazmat.primitives import hashes |
cortex/env_manager.py
Outdated
|
|
||
| from cryptography.fernet import Fernet | ||
| from cryptography.hazmat.primitives import hashes | ||
| from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC |
There was a problem hiding this comment.
Import of 'PBKDF2HMAC' is not used.
| from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC |
docs/ENV_MANAGEMENT.md
Outdated
| # Load environment | ||
| eval "$(cortex env export myapp)" |
There was a problem hiding this comment.
The wrapper script example again uses eval "$(cortex env export myapp)" to load variables before starting the service, which is unsafe because exported values are not fully shell-escaped. If an attacker can influence any stored environment value (for example via cortex env import from a malicious file), they can inject arbitrary shell commands via constructs like $(malicious_cmd) that will execute in the service’s context when this script runs. The example should be changed to a pattern that does not eval the raw export output (or that uses a fully shell-escaped format), to prevent shell command injection via environment data.
| # Load environment | |
| eval "$(cortex env export myapp)" | |
| # Load environment without using eval (avoid shell injection) | |
| while IFS='=' read -r key value; do | |
| # Skip empty lines and comments, if any | |
| [ -z "$key" ] && continue | |
| case "$key" in | |
| \#*) continue ;; | |
| esac | |
| export "$key=$value" | |
| done < <(cortex env export myapp) |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
tests/test_env_manager.py (2)
192-193: Use assertGreater for clearer failure messages.As noted in a previous review, using
assertTrue(len(encrypted) > len(original))provides less informative failure messages thanassertGreater(len(encrypted), len(original)).Suggested fix
- self.assertTrue(len(encrypted) > len(original)) + self.assertGreater(len(encrypted), len(original))
497-497: Misleading test value description.As noted in a previous review, the value "unicode_表情符号_test" uses Chinese characters meaning "emoji" but doesn't actually contain emoji. Consider using actual emoji like "unicode_😀_test" or renaming to "unicode_中文_test" for clarity.
Suggested fix
- "unicode_表情符号_test", + "unicode_😀_test",tests/kernel_features/test_kv_cache_manager.py (1)
189-202: Same redundant patching pattern as noted earlier.The
@patchdecorator forCORTEX_DBis unused because the context manager patch takes precedence. Consider removing the decorator or using only one patching approach consistently.examples/env_demo.py (1)
55-58: Unused variableapi_key.The variable
api_keyis assigned but its value is never used. If the intent is to verify decryption works, consider adding an assertion or removing the assignment.Proposed fix
# Get specific variable console.print("\n[bold]Getting decrypted API_KEY...[/bold]") - api_key = manager.get("myapp", "API_KEY", decrypt=True) + manager.get("myapp", "API_KEY", decrypt=True) console.print(" API_KEY decrypted successfully (redacted)")cortex/env_manager.py (7)
26-27: Remove unused imports.The
hashesandPBKDF2HMACimports fromcryptography.hazmat.primitivesare not used anywhere in the module. These were flagged in a previous review and should be removed to satisfy PEP 8.🔎 Proposed fix
from cryptography.fernet import Fernet -from cryptography.hazmat.primitives import hashes -from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
218-224: Race condition mitigation is partial.This was flagged in a previous review. The current implementation wraps the check in try/except which prevents crashes, but the check-then-write pattern (line 219 check, line 220 write) still has a TOCTOU window. For truly atomic creation, use
os.open()withO_CREAT | O_EXCLflags as suggested previously.
265-269: Silent cleanup failure could accumulate temp files.Previously flagged: if
os.remove(tmp_path)fails silently (lines 266-269), temporary files could accumulate in the environments directory. Consider logging the failure or usingtempfile.NamedTemporaryFilewith automatic cleanup.
417-420: YAML escaping remains insufficient.Previously flagged: escaping only double quotes is insufficient for YAML. Values with colons, brackets, or special starting characters can break parsing. Consider using PyYAML (already in dependencies) for safe serialization.
429-431: Shell escaping remains insufficient.Previously flagged: escaping only double quotes doesn't handle backticks,
$, newlines, or other shell metacharacters. Consider usingshlex.quote()for robust shell escaping.
450-506: Import bypasses validation rules.Previously flagged: when creating
EnvVariableobjects directly during import (lines 456-464, 495-503), the validation rules registered in_register_common_validatorsare not applied. This could allow importing invalid values (malformed URLs, out-of-range ports) that would later cause issues. Consider validating each key-value pair before storing, similar to howset()does.
635-635: DATABASE validation pattern is too broad.Previously flagged: the pattern
.*DATABASE.*matches any variable containing "DATABASE" (e.g.,USE_DATABASE_CACHE,DATABASE_ENABLED) and appliesRequiredValidator, which could incorrectly reject empty/false values. Consider a more specific pattern targeting only sensitive database configuration variables.
🧹 Nitpick comments (32)
tests/test_smart_stacks.py (1)
1-240: LGTM! Comprehensive test coverage for StackManager.The test suite provides excellent coverage including:
- Initialization and loading
- Caching behavior
- Error handling (missing files, invalid JSON)
- Stack operations (listing, finding, describing)
- Hardware-aware suggestions
- Edge cases with mock data
The tests are well-documented with clear docstrings and cover both positive and negative scenarios.
Optional: Consider standardizing test framework usage
The test file mixes pytest (lines 20-28) and unittest (lines 31-237) frameworks. While functional, using a single framework throughout would improve consistency:
Option 1: Convert all to pytest-style functions
-class TestStackManager(unittest.TestCase): - """Comprehensive tests for StackManager class.""" - - def setUp(self): - """Set up test fixtures.""" - self.manager = StackManager() +@pytest.fixture +def manager(): + """Fixture providing a StackManager instance.""" + return StackManager() + +def test_init(manager): + """Test StackManager initialization.""" + assert manager.stacks_file is not None + assert manager.stacks_file.name == "stacks.json"Option 2: Convert the pytest test to unittest
-def test_suggest_stack_ml_gpu_and_cpu(monkeypatch: pytest.MonkeyPatch) -> None: +class TestStackManagerGPU(unittest.TestCase): + @patch('cortex.stack_manager.has_nvidia_gpu') + def test_suggest_stack_ml_gpu_and_cpu(self, mock_has_gpu):Both frameworks work well; consistency is just a style preference.
tests/test_branding.py (3)
7-9: Remove unused importStringIO.
StringIOis imported but never used in this file.🔎 Proposed fix
import unittest -from io import StringIO from unittest.mock import MagicMock, patch
43-52: Moveimport reto module level.Per PEP 8, imports should be at the top of the file. Moving this import avoids repeated import overhead and improves readability.
🔎 Proposed fix
Add
import reat line 7 with other imports:+import re import unittest -from io import StringIO from unittest.mock import MagicMock, patchThen update the test method:
def test_version_format(self): """Test that version follows semver format.""" - import re - self.assertIsNotNone(VERSION)
145-162: Consider importing functions at module level instead of inside test methods.
cx_header,cx_table_header,show_welcome, andshow_goodbyeare imported inside individual test methods (lines 148, 159, 171, 183). If these are intentionally excluded from the main import block because they're only used in specific tests, this is acceptable. However, consolidating imports at the module level improves consistency and readability.tests/test_notification.py (2)
75-80: Add explicit UTF-8 encoding when reading the history file.The implementation uses
encoding="utf-8"for file I/O (percortex/notification_manager.py). For consistency and to avoid platform-dependent encoding issues, specify encoding here as well.🔎 Proposed fix
- with open(self.mgr.history_file) as f: + with open(self.mgr.history_file, encoding="utf-8") as f: data = json.load(f)
95-103: Add explicit UTF-8 encoding when writing the corrupted history file.🔎 Proposed fix
- with open(self.mgr.history_file, "w") as f: + with open(self.mgr.history_file, "w", encoding="utf-8") as f: f.write("{{invalid json")tests/test_dependency_resolver.py (2)
15-15: Remove unused importmock_open.🔎 Proposed fix
-from unittest.mock import MagicMock, patch, mock_open +from unittest.mock import MagicMock, patch
607-620: Add explicit UTF-8 encoding when reading the exported JSON file.The implementation (
export_graph_json) usesencoding="utf-8"for writing. Match this when reading to ensure consistency.🔎 Proposed fix
- with open(filepath) as f: + with open(filepath, encoding="utf-8") as f: data = json.load(f)tests/test_config_manager.py (3)
17-17: Remove unused importmock_open.🔎 Proposed fix
-from unittest.mock import MagicMock, patch, mock_open +from unittest.mock import MagicMock, patch
101-119:hasattrchecks may silently skip tests if methods are renamed or removed.Using
if hasattr(manager, "_get_apt_packages"):before testing the method means the test body is skipped silently if the method doesn't exist. Consider asserting the method exists or removing the conditional to catch regressions.🔎 Example fix
- if hasattr(manager, "_get_apt_packages"): - packages = manager._get_apt_packages() - assert isinstance(packages, (list, dict)) + # Ensure the method exists - will fail if renamed/removed + assert hasattr(manager, "_get_apt_packages"), "Method _get_apt_packages not found" + packages = manager._get_apt_packages() + assert isinstance(packages, (list, dict))Or use
pytest.mark.skipifwith a clear reason if the method is optional.
402-408: Add explicit UTF-8 encoding when writing YAML config files.For consistency with the module's file I/O patterns, specify encoding explicitly.
🔎 Proposed fix
import_path = Path(tmpdir) / "config.yaml" - with open(import_path, "w") as f: + with open(import_path, "w", encoding="utf-8") as f: yaml.dump(config, f)Apply similarly to lines 420-423 and 504-507.
tests/utils/test_commands.py (2)
9-11: Remove unused importMagicMockand reconsidersys.pathmanipulation.
MagicMockis imported but never used. Thesys.path.insertis a workaround for import issues that may indicate the package isn't properly installed in the test environment. Consider running tests with the package installed (pip install -e .) instead.🔎 Proposed fix
import os import sys import unittest -from unittest.mock import MagicMock, patch +from unittest.mock import patch -sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../..")) +# Ensure cortex is installed: pip install -e .
208-212: Test assertion doesn't verify the expected behavior.The test
test_sanitize_removes_dangerouschecks that the result is notNone, but doesn't verify that backticks were actually removed or escaped. This makes the test less meaningful.🔎 Proposed fix
def test_sanitize_removes_dangerous(self): """Test sanitizing removes dangerous content.""" result = sanitize_command("echo `whoami`") - # Backticks should be removed or escaped - self.assertIsNotNone(result) + # Backticks should be removed or escaped + self.assertNotIn("`", result)tests/test_shell_integration.py (1)
26-153: Consider adding more specific assertions.Many tests in the suite only verify that the function doesn't crash and returns None or a string, without checking specific behavior. For example:
- Lines 43-49:
test_with_import_failurecould verify the function returns None when imports fail- Lines 51-55:
test_strips_inputcould verify that stripped input is actually processed- Lines 79-118: Integration tests could verify specific command patterns produce expected suggestions
While the current tests provide valuable smoke testing, more specific assertions would improve test quality and catch regressions better.
Example improvement for test_with_import_failure
def test_with_import_failure(self): """Test handling of import failure for interpreter.""" with patch.dict("sys.modules", {"cortex.interpreter": None}): - # The import should fail and return None result = suggest_command("install something") - # Should not crash - self.assertTrue(result is None or isinstance(result, str)) + # Should return None when interpreter import fails + self.assertIsNone(result)cortex/cli.py (1)
651-651: Consider renaming--encrypted-valueto--rawfor clarity.The double-negative logic
decrypt=not args.encrypted_valueis confusing. When--encrypted-valueisTrue, it actually shows the raw/encrypted value without decryption.🔎 Proposed fix
In the argparser (line 1027):
- get_parser.add_argument("--encrypted-value", action="store_true", help="Show encrypted value") + get_parser.add_argument("--raw", action="store_true", help="Show raw value without decryption")In the handler (line 651):
- value = manager.get(args.app, args.key, decrypt=not args.encrypted_value) + value = manager.get(args.app, args.key, decrypt=not args.raw)tests/kernel_features/test_kv_cache_manager.py (1)
94-106: Redundant patching: decorator patch is unused.The
@patchdecorator on line 94 createsmock_db_pathbut it's immediately shadowed by the context manager patch on line 97. The decorator patch is never used.🔎 Proposed fix
- @patch("cortex.kernel_features.kv_cache_manager.CORTEX_DB") - def test_save_and_get_pool(self, mock_db_path): + def test_save_and_get_pool(self): """Test saving and retrieving pool config.""" with patch("cortex.kernel_features.kv_cache_manager.CORTEX_DB", self.db_path):Apply the same fix to
test_get_pool_nonexistent(lines 108-114) andtest_list_pools(lines 116-125).tests/test_kernel_features.py (1)
427-457: Consider usingpytest.fixtureorExitStackto reduce nesting.The deeply nested context managers for patching multiple detection functions are verbose. This could be simplified.
🔎 Example using ExitStack
from contextlib import ExitStack def test_detect_accelerators(self): """Test detect_accelerators returns HardwareProfile.""" with ExitStack() as stack: stack.enter_context(patch("cortex.kernel_features.hardware_detect.detect_nvidia_gpus", return_value=[])) stack.enter_context(patch("cortex.kernel_features.hardware_detect.detect_amd_gpus", return_value=[])) stack.enter_context(patch("cortex.kernel_features.hardware_detect.detect_intel_gpus", return_value=[])) stack.enter_context(patch("cortex.kernel_features.hardware_detect.detect_intel_npu", return_value=[])) stack.enter_context(patch("cortex.kernel_features.hardware_detect.get_system_ram_gb", return_value=16.0)) stack.enter_context(patch("cortex.kernel_features.hardware_detect.get_cpu_info", return_value=(8, "Intel"))) profile = detect_accelerators() assert isinstance(profile, HardwareProfile)tests/test_installation_verifier.py (1)
144-145: Specify encoding when reading JSON file.For consistency with UTF-8 handling across the codebase, add explicit encoding parameter.
🔎 Proposed fix
- with open(temp_path) as f: + with open(temp_path, encoding="utf-8") as f: data = json.load(f)tests/test_logging_system.py (1)
377-379: Specify UTF-8 encoding for consistency with production code.The
write_textcall should explicitly specify UTF-8 encoding to align with the UTF-8 encoding changes made to the logging system's I/O operations.Proposed fix
# Create an artificially old file old_file = Path(self.temp_dir) / "test_old.old.log" - old_file.write_text("old content") + old_file.write_text("old content", encoding="utf-8")examples/env_demo.py (1)
233-236: Remove redundant import.
shutilis already imported at the module level (line 12), so this local import is unnecessary.Proposed fix
# Cleanup - import shutil shutil.rmtree(temp_dir)tests/test_cli.py (2)
278-289: Test assertion is too permissive.The assertion
self.assertIn(result, [0, 1])accepts both success and failure, which doesn't meaningfully validate the behavior. Consider mocking the expected behavior path and asserting a specific outcome.Proposed fix
@patch("sys.argv", ["cortex", "env", "list"]) @patch("cortex.cli.EnvManager") def test_env_list_all_apps(self, mock_env_manager_class): """Test env list without app shows all apps""" mock_manager = Mock() mock_manager.list_apps.return_value = ["app1", "app2"] mock_env_manager_class.return_value = mock_manager result = main() - # list_apps may be called or list may handle this differently - self.assertIn(result, [0, 1]) # Accept either success or error + # Verify the command completes - actual behavior depends on CLI implementation + mock_manager.list_apps.assert_called_once() + self.assertEqual(result, 0)
338-352: Test assertion is too permissive.Similar to the previous case, accepting either 0 or 1 doesn't validate the intended behavior. If the command structure is uncertain, consider verifying the mock was called appropriately.
Proposed fix
@patch("sys.argv", ["cortex", "env", "template", "list"]) @patch("cortex.cli.EnvManager") def test_env_template_list(self, mock_env_manager_class): """Test env template list command""" mock_manager = Mock() mock_template = Mock() mock_template.name = "nodejs" mock_template.description = "Node.js template" mock_manager.list_templates.return_value = [mock_template] mock_env_manager_class.return_value = mock_manager result = main() - # Template list might have different command structure - self.assertIn(result, [0, 1]) # Accept either + mock_manager.list_templates.assert_called_once() + self.assertEqual(result, 0)tests/kernel_features/test_model_lifecycle_full.py (3)
98-139: Redundant patch decorator and context manager pattern.The
@patchdecorator on line 98/110/118/129 is immediately overridden by thewith patch(...)context manager inside the method. The decorator createsmock_db_pathbut it's never used since the context manager shadows the patched target. Consider using only the context manager or only the decorator.Proposed fix - use only context managers
- @patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH") - def test_save_and_get_model(self, mock_db_path): + def test_save_and_get_model(self): """Test saving and retrieving model.""" with patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH", self.db_path): db = ModelDatabase() config = ModelConfig(name="test-save", model_path="/models/save.gguf") db.save_model(config) result = db.get_model("test-save") self.assertIsNotNone(result) self.assertEqual(result.name, "test-save") - @patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH") - def test_get_model_nonexistent(self, mock_db_path): + def test_get_model_nonexistent(self): """Test getting non-existent model.""" with patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH", self.db_path): db = ModelDatabase() result = db.get_model("nonexistent") self.assertIsNone(result)
229-287: Same redundant patch pattern as TestModelDatabase.The
@patchdecorators forCORTEX_SERVICE_DIRandCORTEX_DB_PATHare immediately overridden by thewith patch(...)context managers. This creates unused mock parameters and adds unnecessary complexity.Proposed fix - use only context managers
- @patch("cortex.kernel_features.model_lifecycle.CORTEX_SERVICE_DIR") - @patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH") - def test_register_model(self, mock_db_path, mock_service_dir): + def test_register_model(self): """Test registering a model.""" with patch("cortex.kernel_features.model_lifecycle.CORTEX_DB_PATH", self.db_path): with patch("cortex.kernel_features.model_lifecycle.CORTEX_SERVICE_DIR", self.service_dir): # ... rest of testApply the same pattern to other methods in
TestModelLifecycleManagerandTestMain.
289-360: Same redundant patch pattern in TestMain class.The
@patchdecorators on lines 305-308, 319-322, 333-336, and 347-350 follow the same redundant pattern. Consider refactoring to use a consistent approach across the test file.tests/kernel_features/test_accelerator_limits.py (5)
120-121: Moveshutilimport to top of file.The
import shutilstatement appears inside multipletearDownmethods. Per PEP 8, imports should be at the top of the file to improve readability and avoid redundant import operations.🔎 Proposed fix
Add to imports at top of file (after line 12):
from pathlib import Path from unittest.mock import MagicMock, patch +import shutilThen remove the local imports from each tearDown method:
def tearDown(self): """Clean up temp directory.""" - import shutil shutil.rmtree(self.temp_dir, ignore_errors=True)Also applies to: 189-190, 244-245
123-131: Redundant patching and missing assertions.The test uses both a
@patchdecorator (line 123) and awith patch(...)context manager (line 129) targeting the same path. The context manager shadows the decorator's mock, making the decorator setup on lines 127-128 ineffective. Additionally, the test has no assertions—it only verifies the code doesn't crash.🔎 Proposed fix
- @patch("cortex.kernel_features.accelerator_limits.CORTEX_DB") - def test_database_creation(self, mock_db_path): + def test_database_creation(self): """Test database is created on init.""" - # Just test that table creation SQL is correct - don't actually create - mock_db_path.parent = Path(self.temp_dir) / ".cortex" - with patch("cortex.kernel_features.accelerator_limits.CORTEX_DB", self.db_path): db = LimitsDatabase() - # Verify it was created without error + # Verify database file was created + self.assertTrue(self.db_path.exists())
133-150: Simplify patching by removing redundant decorator.Similar to
test_database_creation, this test uses both a decorator and context manager for the same path. The decorator's setup (lines 136-137) is ineffective since the context manager on line 142 shadows it. The same pattern appears in other tests in this class (lines 152-161, 163-175).🔎 Proposed fix
Remove the
@patchdecorator and the unusedmock_db_pathparameter from each test method:- @patch("cortex.kernel_features.accelerator_limits.CORTEX_DB") - def test_save_and_get(self, mock_db_path): + def test_save_and_get(self): """Test saving and retrieving limits.""" - mock_db_path.parent = Path(self.temp_dir) / ".cortex" - mock_db_path.__fspath__ = lambda: str(self.db_path) - # Create temp database self.db_path.parent.mkdir(parents=True, exist_ok=True) with patch("cortex.kernel_features.accelerator_limits.CORTEX_DB", self.db_path): # ... rest of testApply the same simplification to
test_get_nonexistentandtest_list_all.
247-254: Same redundant patching pattern in CLI tests.All
TestMaintests use both@patchdecorator andwith patch(...)context manager. The decorator'smock_db_pathparameter is unused since the context manager shadows it. Remove the decorator to simplify.🔎 Proposed fix
- @patch("cortex.kernel_features.accelerator_limits.CORTEX_DB") @patch("sys.argv", ["prog", "create", "test-cli", "--preset", "training", "--gpus", "2"]) - def test_main_create(self, mock_db_path): + def test_main_create(self): """Test main with create command.""" from cortex.kernel_features.accelerator_limits import main with patch("cortex.kernel_features.accelerator_limits.CORTEX_DB", self.db_path): main()Apply the same simplification to
test_main_status,test_main_list, andtest_main_env.
192-230: Consistent patching simplification needed.This test class exhibits the same redundant
@patchdecorator + context manager pattern. The fix is the same as suggested forTestLimitsDatabaseandTestMainclasses—remove the decorators and unusedmock_db_pathparameters.cortex/env_manager.py (2)
47-51:from_dictmay fail with extra or missing keys.The
from_dictmethods usecls(**data)directly, which will raiseTypeErrorif the dictionary contains extra keys not in the dataclass, or if required keys are missing. Consider defensive filtering or explicit key handling for robustness during deserialization of externally-sourced data.🔎 Proposed fix for EnvVariable.from_dict
@classmethod def from_dict(cls, data: Dict[str, Any]) -> "EnvVariable": """Create from dictionary.""" - return cls(**data) + # Filter to only known fields for forward compatibility + known_fields = {f.name for f in __import__('dataclasses').fields(cls)} + filtered = {k: v for k, v in data.items() if k in known_fields} + return cls(**filtered)Also applies to: 67-70
73-86: Consider using@abstractmethoddecorator.The
validatemethod raisesNotImplementedErrorbut doesn't use the@abstractmethoddecorator. This means Python won't prevent instantiation ofValidationRuledirectly. While functional, usingabc.ABCand@abstractmethodwould make the intent clearer and catch errors earlier.🔎 Proposed fix
+from abc import ABC, abstractmethod + -class ValidationRule: +class ValidationRule(ABC): """Base class for environment variable validation rules.""" def __init__(self, error_message: str = "Validation failed"): self.error_message = error_message + @abstractmethod def validate(self, key: str, value: str) -> tuple[bool, Optional[str]]: """ Validate a key-value pair. Returns: Tuple of (is_valid, error_message) """ - raise NotImplementedError + pass
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
cortex/cli.pycortex/dependency_resolver.pycortex/doctor.pycortex/env_manager.pycortex/error_parser.pycortex/first_run_wizard.pycortex/hardware_detection.pycortex/installation_history.pycortex/installation_verifier.pycortex/kernel_features/model_lifecycle.pycortex/logging_system.pycortex/notification_manager.pycortex/semantic_cache.pycortex/stack_manager.pycortex/user_preferences.pycortex/validators.pydocs/ENV_MANAGEMENT.mdexamples/env_demo.pypyproject.tomltests/kernel_features/test_accelerator_limits.pytests/kernel_features/test_kv_cache_manager.pytests/kernel_features/test_llm_device.pytests/kernel_features/test_model_lifecycle_full.pytests/test_branding.pytests/test_cli.pytests/test_config_manager.pytests/test_coordinator.pytests/test_dependency_resolver.pytests/test_doctor.pytests/test_env_manager.pytests/test_error_parser.pytests/test_first_run_wizard.pytests/test_hwprofiler.pytests/test_installation_history.pytests/test_installation_verifier.pytests/test_kernel_features.pytests/test_logging_system.pytests/test_notification.pytests/test_shell_installer.pytests/test_shell_integration.pytests/test_smart_stacks.pytests/test_validators.pytests/unit/test_progress_tracker.pytests/unit/test_sandbox_executor.pytests/utils/test_commands.py
✅ Files skipped from review due to trivial changes (3)
- cortex/hardware_detection.py
- cortex/error_parser.py
- cortex/installation_verifier.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ENV_MANAGEMENT.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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_shell_integration.pytests/test_shell_installer.pytests/test_hwprofiler.pycortex/notification_manager.pycortex/user_preferences.pytests/test_coordinator.pycortex/stack_manager.pytests/kernel_features/test_kv_cache_manager.pytests/kernel_features/test_llm_device.pytests/test_dependency_resolver.pycortex/installation_history.pycortex/doctor.pycortex/kernel_features/model_lifecycle.pytests/test_validators.pyexamples/env_demo.pytests/test_installation_verifier.pycortex/cli.pytests/utils/test_commands.pytests/test_installation_history.pycortex/semantic_cache.pytests/test_first_run_wizard.pytests/test_notification.pytests/unit/test_progress_tracker.pycortex/validators.pycortex/logging_system.pytests/unit/test_sandbox_executor.pytests/test_error_parser.pytests/test_cli.pycortex/first_run_wizard.pytests/test_kernel_features.pytests/test_branding.pytests/test_doctor.pytests/test_env_manager.pycortex/dependency_resolver.pytests/kernel_features/test_accelerator_limits.pytests/test_config_manager.pytests/test_smart_stacks.pytests/test_logging_system.pytests/kernel_features/test_model_lifecycle_full.pycortex/env_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_shell_integration.pytests/test_shell_installer.pytests/test_hwprofiler.pytests/test_coordinator.pytests/kernel_features/test_kv_cache_manager.pytests/kernel_features/test_llm_device.pytests/test_dependency_resolver.pytests/test_validators.pytests/test_installation_verifier.pytests/utils/test_commands.pytests/test_installation_history.pytests/test_first_run_wizard.pytests/test_notification.pytests/unit/test_progress_tracker.pytests/unit/test_sandbox_executor.pytests/test_error_parser.pytests/test_cli.pytests/test_kernel_features.pytests/test_branding.pytests/test_doctor.pytests/test_env_manager.pytests/kernel_features/test_accelerator_limits.pytests/test_config_manager.pytests/test_smart_stacks.pytests/test_logging_system.pytests/kernel_features/test_model_lifecycle_full.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/test_shell_installer.pycortex/installation_history.pytests/test_installation_verifier.pytests/test_installation_history.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
pyproject.toml
**/*sandbox*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement Firejail sandboxing for package operations
Files:
tests/unit/test_sandbox_executor.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 : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/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 **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
tests/test_installation_history.py
🧬 Code graph analysis (27)
tests/test_shell_integration.py (1)
cortex/shell_integration.py (1)
suggest_command(9-46)
tests/test_shell_installer.py (1)
cortex/shell_installer.py (2)
_append_if_missing(8-19)install_shell_integration(22-47)
tests/test_hwprofiler.py (1)
cortex/hwprofiler.py (6)
HardwareProfiler(14-461)detect_cpu(24-99)detect_gpu(101-239)detect_ram(241-265)detect_storage(267-364)detect_network(366-419)
cortex/notification_manager.py (1)
cortex/user_preferences.py (1)
load(117-148)
tests/test_coordinator.py (1)
cortex/coordinator.py (7)
InstallationCoordinator(65-338)_validate_command(159-174)from_plan(98-146)execute(244-293)StepStatus(31-36)verify_installation(295-311)add_rollback_command(240-242)
tests/kernel_features/test_kv_cache_manager.py (1)
cortex/kernel_features/kv_cache_manager.py (7)
CacheConfig(28-32)CacheDatabase(46-79)CacheEntry(36-43)CachePolicy(21-24)save_pool(59-65)get_pool(67-72)list_pools(74-79)
tests/kernel_features/test_llm_device.py (1)
cortex/kernel_features/llm_device.py (12)
FuseOSError(22-24)Session(39-46)MockLLM(49-51)LLMDevice(54-162)_parse(65-79)getattr(81-105)readdir(107-118)read(120-129)write(131-153)truncate(155-156)open(158-159)mount(165-174)
tests/test_dependency_resolver.py (1)
cortex/dependency_resolver.py (16)
Dependency(18-25)DependencyGraph(29-36)DependencyResolver(39-377)_run_command(71-79)is_package_installed(95-97)get_installed_version(99-106)get_apt_dependencies(108-158)get_predefined_dependencies(160-200)resolve_dependencies(202-260)_detect_conflicts(262-282)_calculate_installation_order(284-309)get_missing_dependencies(311-314)generate_install_plan(316-332)_generate_install_commands(334-346)print_dependency_tree(348-360)export_graph_json(362-377)
tests/test_validators.py (1)
cortex/validators.py (6)
ValidationError(11-17)sanitize_command(138-159)validate_api_key(20-48)validate_install_request(83-114)validate_installation_id(117-135)validate_package_name(51-80)
examples/env_demo.py (1)
cortex/env_manager.py (5)
get(325-344)export(388-432)list_templates(550-557)apply_template(508-531)import_env(434-506)
cortex/cli.py (1)
cortex/env_manager.py (9)
EnvManager(138-712)get(325-344)list_apps(377-386)delete(346-363)export(388-432)import_env(434-506)list_templates(550-557)apply_template(508-531)delete_app(697-712)
tests/utils/test_commands.py (2)
cortex/utils/commands.py (3)
CommandResult(118-125)CommandValidationError(128-131)run_command_chain(306-329)cortex/validators.py (1)
sanitize_command(138-159)
tests/test_installation_history.py (1)
cortex/installation_history.py (6)
InstallationHistory(71-631)_run_command(132-142)_get_package_info(144-197)_create_snapshot(199-208)_extract_packages_from_commands(210-242)export_history(567-610)
tests/test_first_run_wizard.py (1)
cortex/first_run_wizard.py (8)
_ensure_config_dir(127-129)_setup_claude_api(308-328)_setup_openai_api(330-349)_setup_ollama(351-385)_step_preferences(474-511)_step_shell_integration(513-561)_step_test_command(619-669)_step_complete(671-719)
tests/test_notification.py (1)
cortex/notification_manager.py (3)
_load_history(53-61)is_dnd_active(72-90)send(92-136)
tests/unit/test_progress_tracker.py (1)
cortex/progress_tracker.py (18)
ProgressTracker(104-551)start_stage(184-195)start_stage(595-601)complete_stage(219-236)complete_stage(614-625)render_text_progress(312-352)cancel(470-502)render_rich_progress(354-393)_format_duration(524-535)start(176-182)display_progress(395-421)complete(423-468)StageStatus(53-60)update_stage_progress(197-217)update_stage_progress(603-612)_send_notification(504-522)setup_cancellation_handler(537-551)RichProgressTracker(554-625)
cortex/validators.py (1)
cortex/utils/commands.py (1)
sanitize_command(199-218)
tests/unit/test_sandbox_executor.py (1)
cortex/sandbox/sandbox_executor.py (8)
ExecutionResult(42-89)to_dict(76-89)failed(72-74)success(67-69)SandboxExecutor(92-709)get_audit_log(691-703)save_audit_log(705-709)execute(547-675)
tests/test_error_parser.py (1)
cortex/error_parser.py (4)
parse_error(279-384)ErrorCategory(17-32)print_analysis(481-504)export_analysis_json(506-529)
tests/test_cli.py (2)
cortex/cli.py (2)
main(921-1110)CortexCLI(33-869)cortex/env_manager.py (7)
get(325-344)list_apps(377-386)delete(346-363)export(388-432)list_templates(550-557)apply_template(508-531)delete_app(697-712)
tests/test_kernel_features.py (3)
cortex/kernel_features/hardware_detect.py (11)
Accelerator(36-53)AcceleratorType(24-32)HardwareProfile(57-78)detect_accelerators(437-477)detect_amd_gpus(131-169)detect_intel_gpus(172-198)detect_intel_npu(201-235)detect_nvidia_gpus(92-128)recommend_models(346-376)get_system_ram_gb(304-323)get_cpu_info(326-343)cortex/kernel_features/model_lifecycle.py (1)
to_dict(34-35)cortex/hwprofiler.py (1)
profile(421-448)
tests/test_doctor.py (1)
cortex/doctor.py (12)
SystemDoctor(20-448)_print_check(99-139)_check_cuda(236-279)_check_ollama(281-306)_check_api_keys(308-319)_check_disk_space(321-345)_check_memory(347-368)_get_system_memory(370-395)_print_summary(397-448)run_doctor(451-459)_check_dependencies(154-197)_check_gpu_driver(199-234)
tests/test_env_manager.py (1)
cortex/env_manager.py (29)
EnvManager(138-712)EnvTemplate(54-70)EnvVariable(31-50)RegexValidator(89-99)RequiredValidator(123-135)URLValidator(102-120)to_dict(43-45)to_dict(63-65)from_dict(48-50)from_dict(68-70)validate(79-86)validate(96-99)validate(117-120)validate(129-135)validate(648-665)get(325-344)delete(346-363)list_apps(377-386)export(388-432)import_env(434-506)list_templates(550-557)get_template(539-548)create_template(533-537)apply_template(508-531)get_env_dict(667-685)load_env_to_os(687-695)delete_app(697-712)_get_app_file(236-238)add_validation_rule(619-629)
tests/kernel_features/test_accelerator_limits.py (1)
cortex/kernel_features/accelerator_limits.py (7)
AcceleratorLimitsManager(79-102)LimitsDatabase(53-76)ResourceLimits(34-50)WorkloadPreset(18-22)from_preset(46-50)list_all(71-76)get_env(88-92)
tests/test_config_manager.py (1)
cortex/config_manager.py (7)
detect_apt_packages(114-143)detect_pip_packages(145-178)detect_npm_packages(180-208)detect_installed_packages(210-244)_detect_os_version(246-272)_load_preferences(274-288)_save_preferences(290-301)
tests/test_smart_stacks.py (1)
cortex/stack_manager.py (7)
StackManager(17-102)load_stacks(25-37)list_stacks(39-42)find_stack(44-49)get_stack_packages(51-54)suggest_stack(56-70)describe_stack(72-102)
cortex/env_manager.py (3)
cortex/first_run_wizard.py (2)
to_dict(62-71)from_dict(74-89)cortex/hardware_detection.py (6)
to_dict(55-56)to_dict(71-72)to_dict(92-93)to_dict(113-114)to_dict(127-128)to_dict(154-171)cortex/kernel_features/model_lifecycle.py (2)
to_dict(34-35)from_dict(38-39)
🪛 Gitleaks (8.30.0)
tests/test_validators.py
[high] 61-61: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 345-345: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 353-353: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
tests/test_config_manager.py
Outdated
| assert len(packages) == 2 | ||
| # Should be sorted by name | ||
| assert packages[0]["name"] == "python3" |
There was a problem hiding this comment.
Incorrect assertion for sorted package order.
The test expects packages[0]["name"] == "python3", but alphabetically "python3" comes after "requests". If the implementation sorts by name ascending, "requests" would be first.
🔎 Proposed fix
packages = manager.detect_installed_packages()
assert len(packages) == 2
# Should be sorted by name
- assert packages[0]["name"] == "python3"
+ assert packages[0]["name"] == "python3" or packages[0]["name"] == "requests"
+ # Or verify actual sort order based on implementationAlternatively, verify the actual sort order expected by the implementation.
📝 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.
| assert len(packages) == 2 | |
| # Should be sorted by name | |
| assert packages[0]["name"] == "python3" | |
| assert len(packages) == 2 | |
| # Should be sorted by name | |
| assert packages[0]["name"] == "python3" or packages[0]["name"] == "requests" | |
| # Or verify actual sort order based on implementation |
🤖 Prompt for AI Agents
In tests/test_config_manager.py around lines 237-239, the assertion assumes
packages[0]["name"] == "python3" but the actual alphabetical order places
"requests" before "python3"; update the test to reflect the real expected order
(e.g., assert packages[0]["name"] == "requests") or, if the implementation is
supposed to sort descending, change the test to assert the descending order
accordingly — verify the implemented sort behavior and make the test expectation
match it.
tests/test_config_manager.py
Outdated
|
|
||
| # Check file was created | ||
| assert manager.preferences_file.exists() or True | ||
|
|
||
| def test_load_preferences(self): | ||
| """Test loading preferences.""" | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| with patch.object(Path, "home", return_value=Path(tmpdir)): | ||
| manager = ConfigManager() | ||
|
|
||
| # Create preferences file | ||
| prefs = {"theme": "dark"} | ||
| manager.preferences_file.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(manager.preferences_file, "w") as f: | ||
| yaml.dump(prefs, f) | ||
|
|
||
| if hasattr(manager, "load_preferences"): | ||
| loaded = manager.load_preferences() | ||
| assert loaded.get("theme") == "dark" or True |
There was a problem hiding this comment.
Assertions with or True always pass, defeating the test purpose.
Lines 495 and 511 use assert ... or True, which makes the assertions always succeed regardless of the actual value. This masks potential failures.
🔎 Proposed fix
- # Check file was created
- assert manager.preferences_file.exists() or True
+ # Check file was created
+ assert manager.preferences_file.exists()- loaded = manager.load_preferences()
- assert loaded.get("theme") == "dark" or True
+ loaded = manager.load_preferences()
+ assert loaded.get("theme") == "dark"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check file was created | |
| assert manager.preferences_file.exists() or True | |
| def test_load_preferences(self): | |
| """Test loading preferences.""" | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| with patch.object(Path, "home", return_value=Path(tmpdir)): | |
| manager = ConfigManager() | |
| # Create preferences file | |
| prefs = {"theme": "dark"} | |
| manager.preferences_file.parent.mkdir(parents=True, exist_ok=True) | |
| with open(manager.preferences_file, "w") as f: | |
| yaml.dump(prefs, f) | |
| if hasattr(manager, "load_preferences"): | |
| loaded = manager.load_preferences() | |
| assert loaded.get("theme") == "dark" or True | |
| # Check file was created | |
| assert manager.preferences_file.exists() | |
| def test_load_preferences(self): | |
| """Test loading preferences.""" | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| with patch.object(Path, "home", return_value=Path(tmpdir)): | |
| manager = ConfigManager() | |
| # Create preferences file | |
| prefs = {"theme": "dark"} | |
| manager.preferences_file.parent.mkdir(parents=True, exist_ok=True) | |
| with open(manager.preferences_file, "w") as f: | |
| yaml.dump(prefs, f) | |
| if hasattr(manager, "load_preferences"): | |
| loaded = manager.load_preferences() | |
| assert loaded.get("theme") == "dark" |
🤖 Prompt for AI Agents
In tests/test_config_manager.py around lines 493 to 511, two assertions use "or
True" which makes them always pass; remove the "or True" from both assertions
and assert the real conditions: assert manager.preferences_file.exists() to
confirm file creation, and after calling manager.load_preferences() assert
loaded.get("theme") == "dark" (or assert loaded == prefs) so the test fails when
preferences are not created/loaded correctly.
tests/test_notification.py
Outdated
| def test_load_config_corrupted(self): | ||
| """Test loading corrupted config file.""" | ||
| # Write invalid JSON | ||
| with open(self.mgr.config_file, "w") as f: | ||
| f.write("{{invalid json") | ||
|
|
||
| mgr = NotificationManager() | ||
| mgr.config_file = self.mgr.config_file | ||
| mgr.history_file = self.mgr.history_file | ||
|
|
||
| # Should use defaults without crashing | ||
| self.assertIn("dnd_start", mgr.config) |
There was a problem hiding this comment.
Test may not exercise the corrupted config loading path as intended.
The test creates a new NotificationManager() instance after writing the corrupted file, but config_file is reassigned after construction. Since _load_config() is called during __init__, it would load from the default location, not the corrupted test file. Consider calling mgr._load_config() explicitly after reassigning the path, or restructure the test setup.
🔎 Proposed fix
def test_load_config_corrupted(self):
"""Test loading corrupted config file."""
# Write invalid JSON
- with open(self.mgr.config_file, "w") as f:
+ with open(self.mgr.config_file, "w", encoding="utf-8") as f:
f.write("{{invalid json")
- mgr = NotificationManager()
- mgr.config_file = self.mgr.config_file
- mgr.history_file = self.mgr.history_file
-
- # Should use defaults without crashing
- self.assertIn("dnd_start", mgr.config)
+ # Reload config from corrupted file
+ self.mgr._load_config()
+
+ # Should still have defaults without crashing
+ self.assertIn("dnd_start", self.mgr.config)🤖 Prompt for AI Agents
In tests/test_notification.py around lines 82 to 93, the test writes a corrupted
config to self.mgr.config_file but instantiates NotificationManager() before
assigning mgr.config_file, so __init__ already loaded the default config; after
reassigning mgr.config_file call mgr._load_config() (or construct
NotificationManager with the test path if constructor supports it) so the
manager actually reads the corrupted file and exercises the error handling;
ensure mgr.history_file is set before calling _load_config() if needed and then
assert defaults are present.
jaysurse
force-pushed
the
feature/env-manager
branch
from
a220cb6 to
f6bfa49
Compare
|
1 participant
Summary
This PR introduces a full-featured environment variable management system for Cortex.
Key highlights:
Per-application environment variable isolation
Secure encryption for sensitive values (API keys, secrets)
Support for environment templates (Node.js, Python, Django, Docker)
Validation rules for URLs, ports, and required variables
Import/export support for .env and JSON formats
CLI integration via cortex env commands
Comprehensive unit tests and documentation included
This improves security, usability, and configuration management for applications installed and managed through Cortex.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.