feat: add environment variable manager with encryption and templates #344
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. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive environment variable management subsystem for Cortex, adding encrypted storage, per-app isolation, template-based validation, import/export capabilities, and a new CLI command group with 13+ subcommands. New EnvironmentManager, EncryptionManager, EnvironmentStorage, and EnvironmentValidator classes orchestrate functionality. Also includes documentation, demo script, and extensive test coverage, plus numerous cosmetic formatting adjustments across multiple files. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Manager as EnvironmentManager
participant Validator as EnvironmentValidator
participant Storage as EnvironmentStorage
participant Crypto as EncryptionManager
participant FS as Filesystem
User->>CLI: cortex env set app KEY=value --encrypt
CLI->>Manager: set_variable(app, KEY, value, encrypt=true)
Manager->>Validator: validate(value, var_type)
Validator-->>Manager: valid=True
Manager->>Crypto: encrypt(value)
Crypto->>Crypto: _get_fernet()
Crypto-->>Manager: encrypted_value
Manager->>Storage: save(app, {KEY: EnvironmentVariable(...)})
Storage->>FS: write ~/.cortex/environments/app.json
FS-->>Storage: success
Storage-->>Manager: success
Manager-->>CLI: EnvironmentVariable object
CLI-->>User: ✓ Variable encrypted and stored
sequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Manager as EnvironmentManager
participant Storage as EnvironmentStorage
participant Crypto as EncryptionManager
participant FS as Filesystem
User->>CLI: cortex env get app KEY
CLI->>Manager: get_variable(app, KEY, decrypt=true)
Manager->>Storage: load(app)
Storage->>FS: read ~/.cortex/environments/app.json
FS-->>Storage: JSON content
Storage-->>Manager: {KEY: EnvironmentVariable(encrypted=true)}
alt Variable is encrypted
Manager->>Crypto: decrypt(encrypted_value)
Crypto-->>Manager: plaintext_value
else Variable is plaintext
Manager-->>Manager: use plaintext_value
end
Manager-->>CLI: plaintext_value (or [encrypted] if no decrypt)
CLI-->>User: Variable value displayed
sequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Manager as EnvironmentManager
participant Template as EnvironmentTemplate
participant Storage as EnvironmentStorage
participant Validator as EnvironmentValidator
participant Crypto as EncryptionManager
User->>CLI: cortex env template apply django app --encrypt API_KEY
CLI->>Manager: apply_template(django, app, values, encrypt_keys=[API_KEY])
Manager->>Manager: get_template(django)
Manager-->>Manager: retrieve django EnvironmentTemplate
loop For each TemplateVariable
Manager->>Validator: validate(value, var_type)
Validator-->>Manager: valid or error
end
loop For each variable in encrypt_keys
Manager->>Crypto: encrypt(value)
Crypto-->>Manager: encrypted_value
end
Manager->>Storage: save(app, all_variables)
Storage-->>Manager: success
Manager-->>CLI: ValidationResult(valid=True, errors=[])
CLI-->>User: ✓ Template applied with X variables set
sequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Manager as EnvironmentManager
participant Storage as EnvironmentStorage
participant FS as Filesystem
participant OS as os.environ
User->>CLI: cortex env import app < .env.local --encrypt SECRET_KEY
CLI->>Manager: import_env(app, content, encrypt_keys=[SECRET_KEY])
Manager->>Manager: parse .env-like content
loop For each KEY=VALUE pair
alt KEY in encrypt_keys
Manager->>Crypto: encrypt(VALUE)
Crypto-->>Manager: encrypted_value
else plaintext
Manager-->>Manager: keep plaintext
end
end
Manager->>Storage: save(app, parsed_variables)
Storage->>FS: write ~/.cortex/environments/app.json
FS-->>Storage: success
Storage-->>Manager: success
Manager-->>CLI: (count, errors)
CLI-->>User: ✓ Imported 8 variables, encrypted 2
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 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)cortex/cli.py (1)
⏰ 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)
🔇 Additional comments (6)
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: 9
🧹 Nitpick comments (9)
examples/env_demo.py (1)
32-37: Unused import:get_env_manager.The
get_env_managerfunction is imported but never used in the demo script. The demo explicitly creates its ownEnvironmentManagerinstance with temporary storage.🔎 Proposed fix
from cortex.env_manager import ( EnvironmentManager, EnvironmentStorage, EncryptionManager, - get_env_manager, )docs/ENV_MANAGEMENT.md (1)
37-39: Add language specifier to fenced code block.Per static analysis, fenced code blocks should have a language specified for proper syntax highlighting.
🔎 Proposed fix
-``` +```text ~/.cortex/environments/<app>.json</details> </blockquote></details> <details> <summary>tests/test_env_manager.py (1)</summary><blockquote> `919-927`: **Test name is misleading - these are sequential writes, not concurrent.** The test `test_concurrent_writes_same_app` performs sequential writes in a loop, which doesn't actually test concurrent access. To properly test concurrency, you'd need threads or async operations. Consider renaming to `test_rapid_sequential_writes_same_app` or implementing actual concurrent testing with threads if race condition testing is intended. </blockquote></details> <details> <summary>cortex/cli.py (3)</summary><blockquote> `748-751`: **Redundant import of `sys` module.** The `sys` module is already imported at line 4 of this file. This local import is unnecessary. <details> <summary>🔎 Proposed fix</summary> ```diff def env(self, args: argparse.Namespace) -> int: """Handle environment variable management commands.""" - import sys - env_mgr = get_env_manager()
914-917: Redundant import ofsysmodule.Same issue as in the
env()method -sysis already imported at the module level (line 4).🔎 Proposed fix
def _env_import(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int: """Import environment variables from .env format.""" - import sys - app = args.app
856-868: Consider using the public API for decryption.The code directly accesses
env_mgr.encryption.decrypt()instead of usingenv_mgr.get_variable(app, var.key, decrypt=True). While this works, using the public API would be more maintainable.🔎 Proposed fix
for var in sorted(variables, key=lambda v: v.key): if var.encrypted: if show_encrypted: try: - value = env_mgr.encryption.decrypt(var.value) + value = env_mgr.get_variable(app, var.key, decrypt=True) console.print(f" {var.key}: {value} [dim](decrypted)[/dim]") except Exception: console.print(f" {var.key}: [red][decryption failed][/red]")cortex/env_manager.py (3)
507-519: Consider handling decryption failures gracefully.
fernet.decrypt()raisescryptography.fernet.InvalidTokenif the encrypted value is corrupted or the key has changed. Currently, this exception will propagate uncaught, potentially causing confusing error messages for users.🔎 Proposed fix
def decrypt(self, encrypted_value: str) -> str: """ Decrypt a value. Args: encrypted_value: Base64-encoded encrypted value Returns: Decrypted plaintext value + + Raises: + ValueError: If decryption fails (corrupted data or wrong key) """ fernet = self._get_fernet() - decrypted = fernet.decrypt(encrypted_value.encode("utf-8")) - return decrypted.decode("utf-8") + try: + decrypted = fernet.decrypt(encrypted_value.encode("utf-8")) + return decrypted.decode("utf-8") + except Exception as e: + raise ValueError(f"Failed to decrypt value: {e}") from e
893-896: Handle escaped characters when parsing quoted values.The import logic strips quotes but doesn't unescape special characters (e.g.,
\"→",\\→\,\n→ newline). This creates an asymmetry with any proper export implementation.🔎 Proposed fix
# Handle quoted values if (value.startswith('"') and value.endswith('"')) or \ (value.startswith("'") and value.endswith("'")): value = value[1:-1] + # Unescape common escape sequences for double-quoted values + if value: + value = value.replace("\\n", "\n") + value = value.replace('\\"', '"') + value = value.replace("\\\\", "\\")
1065-1071: Clarify validation logic for encrypted variables.The current approach validates encrypted values using a placeholder string, then ignores the result. This is confusing and inefficient. Consider skipping validation for encrypted values explicitly.
🔎 Proposed fix
# Validate all variable types for var in variables.values(): - is_valid, error = EnvironmentValidator.validate( - var.value if not var.encrypted else "placeholder", - var.var_type, - ) - if not is_valid and not var.encrypted: - errors.append(f"{var.key}: {error}") + # Skip validation for encrypted values (would require decryption) + if var.encrypted: + continue + is_valid, error = EnvironmentValidator.validate(var.value, var.var_type) + if not is_valid: + errors.append(f"{var.key}: {error}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/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.pyexamples/env_demo.pycortex/env_manager.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.py
🧬 Code graph analysis (4)
tests/test_env_manager.py (1)
cortex/env_manager.py (36)
EnvironmentManager(650-1087)EnvironmentStorage(530-647)EnvironmentVariable(56-83)EnvironmentValidator(368-442)EncryptionManager(445-527)EnvironmentTemplate(98-122)TemplateVariable(87-94)ValidationResult(126-130)VariableType(45-52)get_env_manager(1090-1092)to_dict(64-72)to_dict(104-110)from_dict(75-83)from_dict(113-122)validate(386-442)encrypt(493-505)decrypt(507-519)is_key_available(521-527)save(582-616)load(556-580)delete_app(618-633)list_apps(635-647)list_apps(813-820)set_variable(674-723)get_variable(725-752)get_variable_info(754-766)list_variables(768-779)delete_variable(781-799)clear_app(801-811)export_env(822-853)import_env(855-914)load_to_environ(916-938)list_templates(942-949)get_template(951-961)apply_template(963-1043)validate_app(1045-1087)
examples/env_demo.py (2)
tests/test_env_manager.py (2)
env_manager(64-68)storage(52-54)cortex/env_manager.py (16)
EnvironmentManager(650-1087)EnvironmentStorage(530-647)EncryptionManager(445-527)get_env_manager(1090-1092)set_variable(674-723)get_variable(725-752)list_variables(768-779)delete_variable(781-799)encrypt(493-505)decrypt(507-519)list_templates(942-949)get_template(951-961)apply_template(963-1043)export_env(822-853)import_env(855-914)load_to_environ(916-938)
cortex/env_manager.py (1)
tests/test_env_manager.py (1)
storage(52-54)
cortex/cli.py (1)
cortex/env_manager.py (18)
EnvironmentManager(650-1087)get_env_manager(1090-1092)encrypt(493-505)set_variable(674-723)get_variable(725-752)decrypt(507-519)get_variable_info(754-766)list_variables(768-779)delete_variable(781-799)export_env(822-853)import_env(855-914)clear_app(801-811)list_templates(942-949)get_template(951-961)apply_template(963-1043)list_apps(635-647)list_apps(813-820)load_to_environ(916-938)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 905-905: ruff check failed: UP024 Replace aliased errors with OSError.
🪛 GitHub Check: Lint
cortex/env_manager.py
[failure] 378-378: Ruff (W293)
cortex/env_manager.py:378:1: W293 Blank line contains whitespace
[failure] 370-370: Ruff (W293)
cortex/env_manager.py:370:1: W293 Blank line contains whitespace
[failure] 111-111: Ruff (W293)
cortex/env_manager.py:111:1: W293 Blank line contains whitespace
[failure] 103-103: Ruff (W293)
cortex/env_manager.py:103:1: W293 Blank line contains whitespace
[failure] 73-73: Ruff (W293)
cortex/env_manager.py:73:1: W293 Blank line contains whitespace
[failure] 63-63: Ruff (W293)
cortex/env_manager.py:63:1: W293 Blank line contains whitespace
[failure] 24-24: Ruff (UP035)
cortex/env_manager.py:24:1: UP035 Import from collections.abc instead: Callable
cortex/cli.py
[failure] 955-955: Ruff (UP024)
cortex/cli.py:955:16: UP024 Replace aliased errors with OSError
[failure] 924-924: Ruff (UP015)
cortex/cli.py:924:39: UP015 Unnecessary mode argument
[failure] 905-905: Ruff (UP024)
cortex/cli.py:905:20: UP024 Replace aliased errors with OSError
🪛 markdownlint-cli2 (0.18.1)
docs/ENV_MANAGEMENT.md
37-37: 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 (12)
examples/env_demo.py (1)
379-427: Well-structured demo implementation.Good use of
tempfile.TemporaryDirectorycontext manager for clean isolation and automatic cleanup. The demo effectively showcases all environment management features.docs/ENV_MANAGEMENT.md (1)
293-334: Good security documentation with appropriate warnings.The encryption section properly documents key protection, backup considerations, and provides a practical key rotation procedure. Using
shred -ufor secure deletion of temporary files is a good practice.tests/test_env_manager.py (3)
44-68: Well-designed test fixtures.The fixtures properly use temporary directories for isolation, ensuring tests don't interfere with each other or the real filesystem. Good separation between storage-only, encryption-only, and full manager fixtures.
597-618: Good test hygiene for os.environ manipulation.The test properly cleans environment variables both before and after the test, preventing test pollution.
855-870: Comprehensive factory function testing.Good coverage of the
get_env_managerfactory, verifying both instance type and default path configuration.cortex/cli.py (2)
1245-1339: Well-structured CLI argument parsing.The argparse configuration is comprehensive, with clear help text and proper option definitions. The nested subparser structure for
env templatecommands follows good CLI design patterns.
959-976: Good UX pattern for destructive operations.The confirmation prompt with
--forceflag bypass is a good pattern for protecting against accidental data loss.cortex/env_manager.py (5)
30-42: LGTM!The lazy loading pattern for the optional
cryptographydependency is well implemented with a clear error message guiding users to install the package.
133-365: LGTM!The built-in templates are well-designed with appropriate variable types, sensible defaults, and helpful descriptions. Good coverage of common application environments.
1090-1092: LGTM!The factory function is straightforward. Creating a new instance per call is appropriate for CLI usage.
86-94: LGTM!The
TemplateVariabledataclass is well-structured with appropriate defaults and type hints.
916-938: LGTM!The
load_to_environmethod correctly decrypts encrypted values before loading them intoos.environ.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive environment variable management capabilities to Cortex, including encrypted storage for secrets, template-based configuration, validation, and import/export functionality. The implementation provides both programmatic API access and CLI commands for managing per-application environment configurations.
Key changes:
- Environment variable storage with Fernet encryption for secrets
- Built-in templates for common frameworks (Node.js, Python, Django, Flask, Docker, AWS, Database)
- Validation system supporting URLs, ports, booleans, integers, and paths
- Import/export functionality for .env file format
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
cortex/env_manager.py |
Core implementation with storage, encryption, validation, and template management |
tests/test_env_manager.py |
Comprehensive test suite covering all major functionality with >80% coverage target |
cortex/cli.py |
CLI integration adding env command group with subcommands for all operations |
docs/ENV_MANAGEMENT.md |
Detailed user documentation with examples, security considerations, and API reference |
examples/env_demo.py |
Interactive demonstration script showcasing all features |
requirements.txt |
Added cryptography>=41.0.0 dependency for encryption support |
💡 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 (13)
tests/test_env_manager.py (4)
15-15: Remove unused importjson.The
jsonmodule is imported but not used anywhere in this test file.Proposed fix
-import json
20-20: Remove unused mock imports.
MagicMock,mock_open, andpatchare imported but not used in this test file.Proposed fix
-from unittest.mock import MagicMock, mock_open, patch
34-34: Remove unused importVariableType.
VariableTypeis imported but not used in the tests.Proposed fix
- VariableType,
338-343: Consider making the intentional side-effect explicit.The
storagevariable is created but not used directly - the test relies on the constructor's side effect of creating directories. Consider using_prefix or a comment to make the intent clear.Proposed fix
def test_create_storage(self, temp_dir): """Test creating storage creates directories.""" base_path = temp_dir / "environments" - storage = EnvironmentStorage(base_path=base_path) + EnvironmentStorage(base_path=base_path) # Constructor creates directory assert base_path.exists()cortex/env_manager.py (9)
628-631: Handle temp file cleanup more safely.If the
os.replacecall at line 626 fails, the cleanup at lines 629-630 assumestemp_pathexists. Ifmkstemporos.fdopenfailed before assignment, or the file was already removed,os.unlinkcould raise. Wrap the cleanup in a try-except to avoid masking the original exception. (Previously flagged by Copilot.)🔎 Proposed fix
except Exception: # Clean up temp file on failure - if os.path.exists(temp_path): + try: os.unlink(temp_path) + except (FileNotFoundError, NameError): + # Temp file never created or already removed + pass raise
894-914: Support dots in variable names and unescape quoted values.Two issues in
import_env:
- Line 902 regex
^([A-Za-z_][A-Za-z0-9_]*)=does not allow dots in variable names (e.g.,my.var.name=value). Many systems (Java/Spring) use dots. Consider^([A-Za-z_][A-Za-z0-9_.]*)=.- Lines 911-914 strip quotes but don't unescape internal escaped quotes. For example,
"hello\"world"becomeshello\"worldinstead ofhello"world. Properly unescape after stripping quotes.(Previously flagged by Copilot.)
🔎 Proposed fix
# Parse KEY=value - match = re.match(r"^([A-Za-z_][A-Za-z0-9_]*)=(.*)$", line) + match = re.match(r"^([A-Za-z_][A-Za-z0-9_.]*)=(.*)$", line) if not match: errors.append(f"Line {line_num}: Invalid format") continue key = match.group(1) value = match.group(2) # Handle quoted values - if (value.startswith('"') and value.endswith('"')) or ( - value.startswith("'") and value.endswith("'") - ): - value = value[1:-1] + if (value.startswith('"') and value.endswith('"')): + inner = value[1:-1] + # Unescape backslash and double-quote sequences + inner = inner.replace('\\"', '"').replace('\\\\', '\\') + value = inner + elif (value.startswith("'") and value.endswith("'")): + inner = value[1:-1] + # Unescape backslash and single-quote sequences + inner = inner.replace("\\'", "'").replace('\\\\', '\\') + value = inner
24-24: Remove unused importCallable.Static analysis (Ruff UP035) flags that
Callableis imported but never used. Per past review comments, remove it from the import statement.🔎 Proposed fix
-from typing import Any, Callable +from typing import Any
57-86: Fix trailing whitespace on blank lines.Static analysis (Ruff W293) flags trailing whitespace on blank lines 63 and 73. Per PEP 8 and coding guidelines, blank lines should contain no trailing spaces.
🔎 Proposed fix
var_type: str = "string" - + def to_dict(self) -> dict[str, Any]:} - + @classmethod
101-125: Fix trailing whitespace on blank lines.Static analysis (Ruff W293) flags trailing whitespace on lines 103 and 111.
🔎 Proposed fix
variables: list[TemplateVariable] = field(default_factory=list) - + def to_dict(self) -> dict[str, Any]:"variables": [asdict(v) for v in self.variables], } - + @classmethod
372-381: Fix trailing whitespace and URL host pattern.Two issues:
- Static analysis (Ruff W293) flags trailing whitespace on lines 370 and 378.
- The URL_PATTERN host part
[^\s/$.?#]requires exactly one character, rejecting valid single-char hostnames like "http://a". Use[^\s/$.?#]+to allow one or more characters. (Previously flagged by Copilot.)🔎 Proposed fix
class EnvironmentValidator: """Validates environment variable values based on type.""" - + # URL pattern: protocol://host[:port][/path] URL_PATTERN = re.compile( r"^[a-zA-Z][a-zA-Z0-9+.-]*://" # scheme - r"[^\s/$.?#]" # at least one character for host + r"[^\s/$.?#]+" # one or more characters for host r"[^\s]*$", # rest of URL re.IGNORECASE, ) - + # Port: 1-65535
469-492: Fix race condition in encryption key creation.Multiple processes could simultaneously call
_ensure_key_exists(), leading to a race between checking if the key exists (line 471) and creating it (lines 482-490). Useos.O_EXCLwithos.O_CREATto atomically create the file, failing if it already exists, then retry reading if creation fails due to the file existing. (Previously flagged by Copilot.)🔎 Proposed fix
def _ensure_key_exists(self) -> bytes: """Ensure encryption key exists, create if needed.""" if self.key_path.exists(): return self.key_path.read_bytes() # Create new key Fernet = _get_fernet() key = Fernet.generate_key() # Ensure parent directory exists self.key_path.parent.mkdir(parents=True, exist_ok=True) - # Write key with secure permissions (atomic write) - fd = os.open( - str(self.key_path), - os.O_WRONLY | os.O_CREAT | os.O_TRUNC, - stat.S_IRUSR | stat.S_IWUSR, # 0600 - ) - try: + try: + # Try to create the key file atomically; fail if it already exists + fd = os.open( + str(self.key_path), + os.O_WRONLY | os.O_CREAT | os.O_EXCL, + stat.S_IRUSR | stat.S_IWUSR, # 0600 + ) + except FileExistsError: + # Another process created the key; use that key + return self.key_path.read_bytes() + + try: os.write(fd, key) finally: os.close(fd) return key
650-662: App name mismatch:list_apps()returns sanitized filenames.
list_apps()returnspath.stem, which is the sanitized filename. If an app name contained special characters (e.g., "my/app:name" → "my_app_name.json"), the original name cannot be recovered from the filename alone. This breaks the ability to work with apps that have special characters. Consider reading the JSON file to retrieve the originalappfield stored at line 608. (Previously flagged by CodeRabbit.)🔎 Proposed fix
def list_apps(self) -> list[str]: """ List all applications with stored environments. Returns: List of application names """ apps = [] for path in self.base_path.glob("*.json"): - # Extract app name from filename - app_name = path.stem - apps.append(app_name) + # Load the file to get the original app name + try: + with open(path, "r", encoding="utf-8") as f: + data = json.load(f) + apps.append(data.get("app", path.stem)) + except (json.JSONDecodeError, KeyError, OSError): + # Fall back to filename if file is corrupted + apps.append(path.stem) return sorted(apps)
864-868: Escape special characters in quoted values.When exporting values containing double quotes, backslashes, or newlines, the code wraps them in quotes (line 866) but does not escape internal special characters. This produces invalid .env syntax. For example,
KEY="hello "world""orKEY="line1\nline2"(with literal backslash-n) instead of escaped sequences. Escape backslashes first, then quotes, and replace actual newlines with\n. (Previously flagged by CodeRabbit and Copilot.)🔎 Proposed fix
Add an escape helper method:
+ def _escape_value(self, value: str) -> str: + """Escape special characters for .env format.""" + # Escape backslashes first, then quotes and newlines + value = value.replace("\\", "\\\\") + value = value.replace('"', '\\"') + value = value.replace("\n", "\\n") + return valueThen update the export logic:
if var.description: lines.append(f"# {var.description}") # Quote values with special characters if any(c in var.value for c in " \t\n'\"$`\\"): - lines.append(f'{var.key}="{var.value}"') + lines.append(f'{var.key}="{self._escape_value(var.value)}"') else: lines.append(f"{var.key}={var.value}")
🧹 Nitpick comments (4)
cortex/installation_history.py (2)
458-458: Consider: Coding guidelines require dry-run by default for install operations.The
rollbackmethod defaults todry_run=False, which means it will execute destructive package operations by default. As per the coding guidelines for**/*install*.pyfiles, dry-run should be the default.🔎 Suggested change
- def rollback(self, install_id: str, dry_run: bool = False) -> tuple[bool, str]: + def rollback(self, install_id: str, dry_run: bool = True) -> tuple[bool, str]:This would require callers to explicitly pass
dry_run=Falseto execute real operations, preventing accidental package modifications.Based on coding guidelines for
**/*install*.py.
534-541: Consider: Coding guidelines require explicit user confirmation before sudo execution.The rollback executes sudo commands (e.g.,
sudo apt-get remove -y) without prompting the user for confirmation. The-yflag auto-confirms apt prompts, and there's no interactive confirmation step in the code.Consider adding an explicit confirmation prompt before executing privileged operations:
if not dry_run: print(f"The following {len(actions)} action(s) will be executed:") for action in actions: print(f" {action}") confirm = input("Proceed? [y/N]: ").strip().lower() if confirm != 'y': return (False, "Rollback cancelled by user")Based on coding guidelines for
**/*install*.py.cortex/cli.py (2)
748-750: Remove redundant import.
sysis already imported at line 4; the local import at line 750 is unnecessary.🔎 Proposed fix
def env(self, args: argparse.Namespace) -> int: """Handle environment variable management commands.""" - import sys - env_mgr = get_env_manager()
787-789: Narrow exception handling to specific types.Catching all exceptions can hide unexpected errors. Catch specific exception types (
ValueError,ImportError,OSError) separately to provide clearer error messages, as suggested in past review comments.🔎 Proposed fix
- except Exception as e: + except (ValueError, ImportError, OSError) as e: self._print_error(f"Environment operation failed: {e}") return 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
cortex/cli.pycortex/coordinator.pycortex/doctor.pycortex/env_manager.pycortex/hardware_detection.pycortex/installation_history.pycortex/installation_verifier.pycortex/kernel_features/accelerator_limits.pycortex/kernel_features/kv_cache_manager.pycortex/kernel_features/model_lifecycle.pycortex/llm_router.pycortex/logging_system.pycortex/progress_indicators.pycortex/progress_tracker.pycortex/transaction_history.pydocs/ENV_MANAGEMENT.mdexamples/env_demo.pyexamples/parallel_llm_demo.pyexamples/progress_demo.pyexamples/standalone_demo.pyrequirements.txtsrc/intent/llm_agent.pysrc/intent/planner.pytest_parallel_llm.pytests/test_cli.pytests/test_coordinator.pytests/test_env_manager.pytests/test_interpreter.pytests/test_llm_router.pytests/test_logging_system.pytests/unit/test_progress_tracker.py
💤 Files with no reviewable changes (5)
- tests/test_coordinator.py
- tests/test_interpreter.py
- tests/test_cli.py
- src/intent/llm_agent.py
- src/intent/planner.py
✅ Files skipped from review due to trivial changes (15)
- cortex/hardware_detection.py
- examples/standalone_demo.py
- tests/test_llm_router.py
- examples/parallel_llm_demo.py
- tests/unit/test_progress_tracker.py
- cortex/kernel_features/kv_cache_manager.py
- cortex/kernel_features/accelerator_limits.py
- cortex/progress_indicators.py
- cortex/llm_router.py
- cortex/transaction_history.py
- tests/test_logging_system.py
- examples/progress_demo.py
- cortex/installation_verifier.py
- cortex/logging_system.py
- cortex/doctor.py
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.txt
- docs/ENV_MANAGEMENT.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/coordinator.pycortex/kernel_features/model_lifecycle.pycortex/progress_tracker.pytests/test_env_manager.pycortex/cli.pyexamples/env_demo.pytest_parallel_llm.pycortex/installation_history.pycortex/env_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
cortex/installation_history.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/installation_history.py
🧬 Code graph analysis (1)
test_parallel_llm.py (1)
cortex/llm_router.py (4)
query_multiple_packages(725-772)diagnose_errors_parallel(775-821)check_hardware_configs_parallel(824-872)acomplete(441-505)
⏰ 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: Build Package
🔇 Additional comments (35)
cortex/kernel_features/model_lifecycle.py (1)
97-97: Minor consistency improvement; unrelated to PR scope.The change from single quotes to double quotes for the comma separator is purely stylistic and improves consistency with the rest of the file, which predominantly uses double quotes. Functionally, both produce identical output. However, this change appears unrelated to the PR's stated objectives (environment variable management system with encryption and templates).
cortex/installation_history.py (1)
693-693: LGTM!The formatting change adding spaces around the minus operator (
len(r.packages) - 2) follows PEP 8 style for arithmetic operators.cortex/progress_tracker.py (1)
584-588: LGTM!The multi-line formatting of the
add_taskcall arguments improves readability. No functional change.cortex/coordinator.py (2)
74-79: LGTM!Minor formatting change in the f-string expression (
{i + 1}with spaces). No functional impact.
253-253: LGTM!Consistent formatting update in the log message.
test_parallel_llm.py (8)
51-56: LGTM!Correctly using a plain string instead of an f-string when no interpolation is needed.
98-100: LGTM!Formatting improvements: plain string and consistent spacing in arithmetic expression.
111-114: LGTM!Adding a blank line before
traceback.print_exc()improves readability.
173-173: LGTM!Consolidating function call arguments to a single line is cleaner.
184-184: LGTM!
195-195: LGTM!
231-233: LGTM!The multi-line formatting of the
acompletecall with dictionary unpacking is readable.
243-246: LGTM!Consistent use of plain string where no interpolation is needed.
examples/env_demo.py (9)
1-46: LGTM!Well-structured demo file with proper module docstring, imports, and helper function. The Rich console integration provides clear visual output.
48-102: LGTM!Basic operations demo covers set/get/list/delete with clear console output. Good use of table formatting for variable listing.
104-155: LGTM!Encryption demo clearly shows the encrypt/decrypt workflow and how encrypted values appear in storage vs when decrypted.
157-209: LGTM!Validation demo effectively shows both successful validation and proper error handling for invalid values.
211-256: LGTM!Template demo shows listing, inspection, and application of templates with proper null checking for template retrieval.
258-313: LGTM!Export/import demo covers both encrypted and non-encrypted scenarios, including the import workflow with error reporting.
315-338: LGTM!App isolation demo effectively shows that the same key can hold different values across different applications.
340-376: LGTM!Load-to-environ demo properly handles cleanup of test environment variables both before and after the demonstration, preventing test pollution.
378-430: LGTM!Main function properly uses temporary directory for demo isolation, ensuring no persistent state is left behind. The usage examples at the end provide helpful guidance for users.
tests/test_env_manager.py (12)
43-68: LGTM!Well-designed fixtures providing proper test isolation with temporary directories.
75-140: LGTM!Comprehensive tests for
EnvironmentVariabledataclass covering creation, serialization, and deserialization.
147-256: LGTM!Thorough validation tests covering all variable types, edge cases, and custom pattern validation with proper error handling.
263-328: LGTM!Excellent encryption tests including security-sensitive permission checks (0o600) and edge cases like empty strings and unicode characters.
344-418: LGTM!Storage tests thoroughly cover save/load, app isolation, deletion, listing, and special character handling in app names.
579-591: Line numbering appears correct.The invalid line "invalid line without equals" is on line 3 of the content (counting the empty first line as line 1), so the assertion
"Line 3" in errors[0]is correct.
425-614: LGTM!Comprehensive
EnvironmentManagertests covering CRUD operations, encryption, validation, export/import workflows, and loading toos.environ. Good cleanup of test environment variables.
621-756: LGTM!Template tests comprehensively cover listing, retrieval, application with defaults/custom values, validation of required fields, and encryption support.
763-840: LGTM!Template dataclass and ValidationResult tests properly verify serialization/deserialization and result construction.
847-866: LGTM!Factory function tests verify proper path defaults for storage and encryption key locations.
873-939: LGTM!Edge case tests provide excellent coverage for special characters, unicode, multiline values, long strings, and concurrent access patterns.
946-1020: LGTM!Integration tests validate complete workflows including export/import cycles, template customization, and multi-app isolation with proper cleanup verification.
cortex/cli.py (1)
947-952: Reconsider exit code for partial imports.Currently, if any variables are imported successfully but warnings/errors exist, the method returns
1(failure). This means a partial import with invalid lines is marked as failed even though some variables succeeded. Consider returning0whencount > 0to indicate partial success, or clarify the intended behavior. (Previously flagged by Copilot.)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_env_manager.py (2)
579-591: Verify line numbering in error messages.The test expects the error message to contain "Line 3" for the invalid line. However, the actual line numbering depends on how the implementation counts lines:
- If all lines (including empty ones) are counted: line 3 is correct ✓
- If only non-empty lines are counted: the invalid line would be line 2 ✗
Please verify that the implementation in
env_manager.import_env()counts lines consistently with this expectation.#!/bin/bash # Verify how import_env counts lines in error messages rg -n -A5 -B5 "import_env.*def|Line \d+" cortex/env_manager.py
15-15: Remove unused imports.The following imports are not used anywhere in the test file:
- Line 15:
json- Line 20:
MagicMock,mock_open,patch- Line 34:
VariableTypeRemoving unused imports improves code clarity and complies with PEP 8.
🔎 Proposed fix
-import json import os-from unittest.mock import MagicMock, mock_open, patch - import pytestEnvironmentValidator, EnvironmentVariable, TemplateVariable, ValidationResult, - VariableType, get_env_manager,Also applies to: 20-20, 34-34
🧹 Nitpick comments (2)
tests/test_env_manager.py (2)
410-418: Consider testing additional failure scenarios.The test suite is comprehensive, but consider adding tests for these additional edge cases:
- Corrupted storage files: Test behavior when JSON files are malformed
- Corrupted encryption key: Test behavior when the key file is corrupted
- File permission errors: Test behavior when storage directory is read-only
get_variable_info()with non-existent variable: Test return value/behaviorThese would improve robustness and error-handling coverage.
906-912: Consider documenting the rationale for test boundaries.The test uses a 100KB value as a "very long" boundary. While this is reasonable, consider adding a comment explaining the choice (e.g., "Tests large values typical of certificates or API responses" or "Tests values exceeding typical environment variable sizes").
This helps future maintainers understand test intent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/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.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Package
🔇 Additional comments (5)
tests/test_env_manager.py (5)
338-343: Test correctly verifies directory creation side effect.The
storagevariable on line 341 is intentionally created to trigger directory creation as a side effect. The assertion on line 342 verifies this side effect occurred. This is a valid test pattern for initialization behavior.The past review comment suggesting the variable is unused is incorrect.
38-48: Excellent test organization and structure.The test suite demonstrates strong organization:
- Clear sectioning with comment markers
- Logical grouping by component (EnvironmentVariable, Validator, Storage, Manager, Templates)
- Descriptive test names following pytest conventions
- Comprehensive docstrings for all test methods
- Proper fixture usage for test isolation
- Dedicated integration test suite
This structure makes the test suite maintainable and easy to navigate.
Also applies to: 75-76, 147-148, 263-264, 335-336, 425-426, 621-622, 763-764, 820-821, 847-848, 873-874, 946-947
621-756: Comprehensive template test coverage.The template tests thoroughly cover:
- Built-in template validation (nodejs, python, django, flask, docker, database, aws)
- Template lifecycle (list, get, apply)
- Default and custom value handling
- Required variable validation
- Type validation during application
- Encryption integration
- Error cases (missing templates, invalid values)
- Template-based validation
This aligns well with the PR objectives for template support.
285-296: Good security practice: verify key file permissions.The test correctly verifies that encryption keys are created with 0600 permissions, which is essential for security. This ensures only the file owner can read the key.
946-1020: Integration tests validate PR objectives effectively.The integration test suite validates key requirements from issue #95:
- Per-application environment isolation (lines 996-1020)
- Template-driven setup workflow (lines 977-994)
- Complete variable lifecycle with encryption (lines 949-976)
These tests ensure the system works cohesively beyond unit-level validation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 31 changed files in this pull request and generated 6 comments.
💡 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cortex/dependency_check.py (5)
15-22: Critical: Missingcryptographydependency.The PR introduces encryption features for environment variable management using Fernet encryption, but
cryptographyis not included in theREQUIRED_DEPENDENCIESlist. This will cause runtime failures when users attempt to use encryption features.🔎 Add cryptography to the dependencies list
REQUIRED_DEPENDENCIES = [ ("pyyaml", "yaml"), ("rich", "rich"), ("anthropic", "anthropic"), ("openai", "openai"), ("python-dotenv", "dotenv"), ("requests", "requests"), + ("cryptography", "cryptography"), ]
24-29: Add docstring for public function.The function lacks a docstring explaining its purpose, parameters, and return value.
🔎 Add docstring
def check_dependency(package_name: str, import_name: str) -> DependencyStatus: + """ + Check if a dependency is installed by attempting to import it. + + Args: + package_name: The package name as used by pip (e.g., 'pyyaml'). + import_name: The name to use for importing (e.g., 'yaml'). + + Returns: + DependencyStatus with package name, installation status, and import name. + """ try: __import__(import_name) return DependencyStatus(package_name, True, import_name) except ImportError: return DependencyStatus(package_name, False, import_name)As per coding guidelines, docstrings are required for all public APIs.
31-32: Add docstring for public function.The function lacks a docstring explaining its purpose and return value.
🔎 Add docstring
def get_missing_dependencies() -> list[str]: + """ + Get a list of package names for dependencies that are not installed. + + Returns: + List of package names (for pip installation) that are missing. + """ return [pkg for pkg, imp in REQUIRED_DEPENDENCIES if not check_dependency(pkg, imp).installed]As per coding guidelines, docstrings are required for all public APIs.
34-56: Add docstring for public function.The function lacks a docstring explaining its purpose, parameters, and return value.
🔎 Add docstring
def format_installation_instructions(missing: list[str]) -> str: + """ + Format user-friendly installation instructions for missing dependencies. + + Args: + missing: List of missing package names. + + Returns: + Formatted multi-line string with installation instructions. + """ packages = " ".join(missing) return f"""As per coding guidelines, docstrings are required for all public APIs.
58-62: Add docstring for public function.The function lacks a docstring explaining its purpose and side effects (exits the program).
🔎 Add docstring
def verify_dependencies_or_exit() -> None: + """ + Verify all required dependencies are installed. + + Exits the program with status code 1 and prints installation + instructions if any dependencies are missing. + """ missing = get_missing_dependencies() if missing: print(format_installation_instructions(missing), file=sys.stderr) sys.exit(1)As per coding guidelines, docstrings are required for all public APIs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/dependency_check.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/dependency_check.py
🔇 Additional comments (1)
cortex/dependency_check.py (1)
10-13: LGTM! Good separation of concerns.Adding the
import_namefield correctly distinguishes between the package name (for pip installation) and the import name (for runtime checks). This is essential for packages likepyyamlwhere the package name differs from the import name.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/cli.py (3)
751-792: Consider more specific exception handling.The broad
Exceptioncatch at line 790 can hide important error details from users. Based on the operations being performed and past review feedback, consider catching specific exception types to provide more helpful error messages.🔎 Proposed refinement
- except Exception as e: - self._print_error(f"Environment operation failed: {e}") - return 1 + except (ValueError, ImportError, OSError) as e: + self._print_error(f"Environment operation failed: {e}") + return 1 + except Exception as e: + self._print_error(f"Unexpected error: {e}") + if self.verbose: + import traceback + traceback.print_exc() + return 1
794-826: LGTM with minor suggestion: Verify ImportError source.The method properly handles errors and provides good user feedback. However, line 824 assumes all
ImportErrorexceptions are related to thecryptographylibrary. Consider verifying the error message or catching this more specifically.🔎 Optional improvement
except ImportError as e: - self._print_error(str(e)) - cx_print("Install with: pip install cryptography", "info") + error_msg = str(e) + self._print_error(error_msg) + if "cryptography" in error_msg.lower(): + cx_print("Install with: pip install cryptography", "info") return 1
919-962: Consider return value consistency for partial imports.The method now correctly uses
OSErrorand omits the unnecessary"r"mode (addressing previous review feedback). However, there's a potential inconsistency: whencount > 0anderrorsexist, the method prints a success message (line 951) but returns exit code 1 (line 955). This treats partial imports as failures despite successfully importing some variables.Based on previous review feedback, consider treating partial imports with warnings as successful:
🔎 Proposed refinement
if count > 0: cx_print(f"✓ Imported {count} variable(s) to '{app}'", "success") + # Treat partial imports with warnings as success + return 0 else: cx_print("No variables imported", "info") - - return 0 if not errors else 1 + # Only return failure if nothing was imported and there were errors + return 1 if errors else 0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/env_manager.py (12)
EnvironmentManager(665-1105)get_env_manager(1108-1110)encrypt(502-514)set_variable(689-738)get_variable(740-767)decrypt(516-534)export_env(837-870)import_env(872-932)list_templates(960-967)get_template(969-979)apply_template(981-1061)load_to_environ(934-956)
⏰ 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 (17)
cortex/cli.py (17)
12-12: LGTM: Import is correct and necessary.The import of
EnvironmentManagerandget_env_manageris properly scoped and aligns with the new environment management functionality.
827-846: LGTM: Method implementation is clean and correct.The
_env_getmethod properly retrieves and displays environment variables, with appropriate handling of encrypted values and clear user feedback.
848-879: LGTM: Well-structured list display with proper error handling.The method handles both encrypted and unencrypted variables appropriately, includes proper error handling for decryption failures, and provides a clean user experience.
881-891: LGTM: Clean and straightforward implementation.The delete operation is simple and correct, with appropriate user feedback.
893-917: LGTM: Export functionality is well-implemented.The method correctly handles both file and stdout output, with proper error handling using
OSError(addressing previous review feedback).
964-981: LGTM: Clear operation includes appropriate safety measures.The confirmation prompt (unless
--forceis used) is a good safeguard against accidental data loss.
983-997: LGTM: Template command routing is clean.Simple and effective delegation to template-specific handlers.
999-1012: LGTM: Template listing is well-formatted.Clear presentation of available templates with helpful usage guidance.
1014-1037: LGTM: Template details display is user-friendly.The method provides clear visualization of template structure, including required fields and defaults.
1039-1072: LGTM: Template application is well-implemented.The method properly parses user input, handles validation, and provides clear feedback on success or failure.
1074-1087: LGTM: App listing is straightforward and effective.Clear display of applications with environment variable counts.
1089-1100: LGTM: Load operation is simple and correct.Direct delegation to the environment manager with appropriate user feedback.
1252-1342: LGTM: Comprehensive argument parsing for env commands.The argument parser configuration is well-structured, with appropriate constraints and helpful descriptions for all environment management commands and subcommands.
1387-1388: LGTM: Command dispatch is properly integrated.The env command is correctly wired into the main command dispatcher, following the established pattern.
469-469: LGTM: Minor formatting consistency improvement.
598-598: LGTM: Consistent string formatting.
1125-1126: LGTM: Help table updated correctly.The notify and env commands are appropriately added to the rich help display.
- Replace broad Exception blocks with specific exceptions (ValueError, OSError, ImportError) - Add verbose traceback printing for unexpected exceptions - Only suggest pip install cryptography when ImportError mentions it - Return success (0) for partial imports with warnings - Remove unused sys import in env() method
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1-1462: Run Black to fix formatting violations and unblock CI pipeline.The file contains 71 lines exceeding Black's 88-character limit. Run
python -m black cortex/cli.pyto automatically reformat the file according to the project's Black configuration.
🧹 Nitpick comments (1)
cortex/cli.py (1)
560-568: Consider extracting the exception handling pattern.The try-except pattern with verbose traceback support is repeated ~8 times. While correct, you could reduce duplication with a decorator or helper method.
Example refactoring approach
def _handle_errors(self, func): """Decorator to standardize exception handling.""" def wrapper(*args, **kwargs): try: return func(*args, **kwargs) except (ValueError, ImportError, OSError) as e: self._print_error(str(e)) return 1 except Exception as e: self._print_error(f"Unexpected error: {e}") if self.verbose: import traceback traceback.print_exc() return 1 return wrapperThen apply as:
@_handle_errorsabove methods likecache_stats,history, etc.Also applies to: 629-637, 656-664, 691-699, 746-754, 836-844
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/env_manager.py (12)
EnvironmentManager(665-1105)get_env_manager(1108-1110)encrypt(502-514)set_variable(689-738)get_variable(740-767)decrypt(516-534)export_env(837-870)import_env(872-932)list_templates(960-967)get_template(969-979)apply_template(981-1061)load_to_environ(934-956)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black' to format.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (7)
cortex/cli.py (7)
12-12: LGTM: Environment manager imports.The imports are correctly structured and align with the external
cortex/env_manager.pymodule API.
459-475: LGTM: Exception handling improvements.The exception handling correctly uses
OSError(addressing past review feedback), and the verbose traceback support is a helpful debugging feature.Also applies to: 532-544
799-844: LGTM: Well-structured env command dispatcher.The main
env()method provides clear routing to all subcommands with appropriate error handling. The implementation covers all requirements from the PR objectives.
846-1154: LGTM: Comprehensive env helper implementations.All 13 helper methods are well-implemented with:
- Clear parameter handling
- Appropriate use of the EnvironmentManager API
- Good user feedback (success/error messages)
- Proper encryption handling
- Confirmation prompts for destructive operations
The implementation aligns well with the acceptance criteria from issue #95.
1306-1396: LGTM: Comprehensive argument parsing for env commands.The argparse configuration is thorough and well-structured:
- All subcommands properly defined
- Appropriate argument types and choices
- Clear help text for users
- Template subcommand nesting handled correctly
1441-1442: LGTM: Env command routing.The routing correctly dispatches the
envcommand tocli.env(args), completing the integration.
1449-1458: LGTM: Enhanced exception handling in main().The updated exception handling in
main()properly catches specific exception types and provides verbose tracebacks when requested, improving debuggability.
PR Review - Environment Variable Manager ✅Tested locally and the core functionality works well. Here are my findings: ✅ What WorksCore Functionality
Templates System ⭐
Import/Export
Security
🔧 How to Use Encrypted VariablesThe encryption feature works, but requires understanding the flag system: For # Hidden by default (secure)
cortex env list myapp
# → API_KEY: (empty/hidden)
# Show decrypted value
cortex env get myapp API_KEY --decrypt
# → API_KEY: secret123
cortex env list myapp --decrypt
# → API_KEY: secret123 (decrypted)For # Safe export (excludes encrypted values)
cortex env export myapp > .env
# → # API_KEY=[encrypted - use --include-encrypted to export]
# Export with secrets (when needed)
cortex env export myapp --include-encrypted > .env.production
# → API_KEY="secret123"
|
|
4 participants
Closes #95
Overview
This pull request introduces a complete environment variable management system for Cortex.
It enables applications and services to securely store, validate, and manage configuration values using a unified command-line interface.
The solution is designed to be safe for production use and supports encryption, validation, templates, and runtime loading.
It includes full documentation, extensive tests, and example workflows to ensure reliability and ease of adoption.
What This PR Adds
Environment Variable Management
• Per-application isolated environments
• Persistent storage of environment variables
• Optional encryption for sensitive values
• Safe loading into process runtime
CLI Support (cortex env)
• Create, read, update, delete environment variables
• Import and export .env files
• Clear environments safely
• List applications with stored environments
Environment Templates
• Built-in templates for common stacks (Node.js, Python, Django, Docker)
• Required variables and default values enforced
• Automatic validation when applying templates
Validation
• URL, port, and required-field validation
• Prevents invalid configuration from being saved
Security
• Encrypted secrets using Fernet symmetric encryption
• Encryption keys stored locally with restricted permissions
• Encrypted values are hidden unless explicitly decrypted
CLI Commands and Usage
Set an environment variable
cortex env set
Set an encrypted variable
cortex env set --encrypt
Get a variable value
cortex env get
List all variables for an app
cortex env list
Delete a variable
cortex env delete
Export environment variables
cortex env export
cortex env export --output
Import variables from a file
cortex env import
cortex env import --encrypt-keys KEY1,KEY2
Load variables into the current process
cortex env load
List applications with stored environments
cortex env apps
Clear an application environment
cortex env clear
cortex env clear --force
Environment templates
cortex env template list
cortex env template show
cortex env template apply
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.