feat: system role management and package recommendations #542
Conversation
📝 WalkthroughWalkthroughAdds a new RoleManager for sensing, redacting, and persisting system roles; integrates Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant RoleManager
participant FS as File System
participant AI as AI Inference
User->>CLI: cortex role detect
CLI->>RoleManager: get_system_context()
RoleManager->>FS: probe PATH, read shell histories, check GPUs, read install history DB
RoleManager-->>CLI: SystemContext
CLI->>AI: build prompt (nonce, timestamp, patterns)
AI-->>CLI: recommendations (3-5 roles/packages)
CLI-->>User: display detected roles & guidance
sequenceDiagram
participant User
participant CLI
participant RoleManager
participant FS as File System
participant AI as AI Inference
User->>CLI: cortex role set <role_slug>
CLI->>RoleManager: save_role(role_slug)
RoleManager->>FS: acquire .env.lock (fcntl/msvcrt)
RoleManager->>FS: read current .env
RoleManager->>FS: write .env.tmp
RoleManager->>FS: rename .env.tmp -> .env (atomic)
RoleManager->>FS: remove .env.lock
RoleManager-->>CLI: confirmation
CLI->>AI: fetch tailored recommendations based on new role
CLI-->>User: display persisted role and recommendations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 45-55: The ML Workstation role detection is too broad because
"python3" in the "binaries" list will match almost any Linux machine; update the
ROLE_ML_WORKSTATION entry so its "binaries" list removes "python3" and instead
uses more specific indicators (e.g., keep "nvidia-smi" and "nvcc" and optionally
add "conda" and "jupyter" or other ML-framework-specific executables) so
detection relies on GPU/tooling presence rather than a generic Python install.
In @docs/ROLES.md:
- Around line 31-34: Remove 'python3' from the ML Workstation detection
signatures and replace it with more specific ML-related binaries (e.g.,
'jupyter', 'conda', maybe 'pip' if desired) in the role detection signatures
array in role_manager.py (the ML workstation signatures/list used to detect that
role), then update the corresponding ML Workstation bullet in ROLES.md to list
those specific tools instead of 'python3' so the docs match the implementation.
🧹 Nitpick comments (4)
tests/test_role_management.py (2)
23-27: Patch target should reference the module under test.The patch should target
cortex.role_manager.shutil.whichrather thanshutil.whichto ensure the mock is applied where the function is looked up. Currently, this works becauseshutilis imported as a module (notfrom shutil import which), but the explicit target is more robust.♻️ Suggested improvement
- with patch("shutil.which", side_effect=lambda x: "/usr/bin/nginx" if x == "nginx" else None): + with patch("cortex.role_manager.shutil.which", side_effect=lambda x: "/usr/bin/nginx" if x == "nginx" else None):
126-142: Test should verify the corrected JSON structure after recovery.The test validates that
learn_packagehandles malformed JSON, but it should also assert that the resulting file contains valid JSON with the learned package.♻️ Suggested improvement
manager = RoleManager(env_path=env_path) # The method should catch the error and overwrite/initialize a valid structure manager.learn_package("ml-workstation", "numpy") assert learned_file.exists() + + # Verify the file now contains valid JSON with the learned package + data = json.loads(learned_file.read_text()) + assert "numpy" in data.get("ml-workstation", [])cortex/role_manager.py (2)
68-70: Shallow copy ofROLE_DEFINITIONSshares nested dictionaries.
dict.copy()creates a shallow copy, so the nested dictionaries (role configs) are still shared with the class constant. If custom roles modify existing role entries, it could mutateROLE_DEFINITIONS.Consider using
copy.deepcopy()for full isolation.♻️ Suggested improvement
+import copy + class RoleManager: ... def __init__(self, env_path: Path | None = None) -> None: ... - self.roles = self.ROLE_DEFINITIONS.copy() + self.roles = copy.deepcopy(self.ROLE_DEFINITIONS)
12-12: Remove unusedOptionalimport.The code uses the
| Noneunion syntax (Python 3.10+) throughout, so theOptionalimport fromtypingis unused.♻️ Suggested fix
-from typing import Any, Optional +from typing import Any
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdcortex/cli.pycortex/role_manager.pydocs/ROLES.mdtests/test_role_management.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_role_management.pycortex/cli.pycortex/role_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.py
🧬 Code graph analysis (1)
tests/test_role_management.py (1)
cortex/role_manager.py (6)
detect_active_roles(91-107)save_role(109-132)get_saved_role(134-150)get_all_slugs(167-174)get_recommendations_by_slug(152-165)learn_package(176-213)
⏰ 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: Test (Python 3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (9)
docs/ROLES.md (1)
1-113: Documentation is comprehensive and well-structured.The documentation covers all key aspects: usage, features (detection, recommendations, learning, custom roles), examples, and technical implementation details. The thread-safe persistence explanation aligns with the actual implementation.
README.md (1)
70-70: README updates accurately reflect the new role management feature.The additions to features table, command reference, and project structure are consistent with the implementation.
Also applies to: 152-152, 226-226
cortex/cli.py (2)
269-323: Role management CLI implementation looks solid.The implementation properly:
- Validates role slugs against the allowed list before saving
- Handles missing subcommands gracefully
- Catches and reports exceptions from
save_role- Returns appropriate exit codes
The structure follows the existing patterns in the codebase (e.g.,
notify,sandboxmethods).
1876-1885: Argument parser setup is correct.The subparser configuration follows the established pattern and provides appropriate help text.
tests/test_role_management.py (2)
10-15: Fixture is well-designed for isolation.Using
tmp_pathensures each test gets a clean, isolated directory. Good practice for file I/O tests.
84-103: Error handling test is comprehensive.Testing that
save_roleraisesRuntimeErrorwith the expected message when file I/O fails ensures the error contract is maintained.cortex/role_manager.py (3)
1-57: Well-designed public API with good documentation.The class structure is clean with:
- Clear constant definitions for role slugs
- Comprehensive docstrings on all public methods
- Proper type hints throughout
- Good separation between public API and private implementation details
Also applies to: 91-108, 134-174
215-238: File locking implementation is correct for atomic writes.The pattern of:
- Acquiring exclusive lock via
fcntl.flock- Reading existing content
- Modifying via callback
- Writing to temp file with restrictive permissions (0o600)
- Atomically replacing via
Path.replace()- Releasing lock in
finallyThis is a solid approach for process-safe configuration updates.
6-6: This review comment is not applicable to this codebase. Cortex is explicitly designed as "an AI layer for Linux Debian/Ubuntu" and has no stated Windows support requirements. Thefcntlmodule is appropriate for this Linux-only project, and no platform check or fallback mechanism is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/ROLES.md (1)
95-107: Technical implementation snippet is incomplete/illustrative.The code snippet references
temp_fileandself.env_filewithout showing where they're defined, and themodifierfunction signature isn't shown. While this is acceptable for high-level documentation, consider either:
- Adding a comment like
# Simplified illustrationat the top, or- Linking to the actual implementation with
See role_manager.py:_locked_read_modify_write for full detailscortex/role_manager.py (1)
214-216: Thetarget_fileparameter successfully addresses previous concurrency concerns.The addition of the
target_fileparameter eliminates the fragile pattern of temporarily swappingself.env_file. This is now properly isolated and thread-safe.However, the
modifier_func: Anytype hint could be more precise for better type safety.♻️ Strengthen type hint for modifier_func
+from typing import Callable + def _locked_read_modify_write( - self, key: str, value: str, modifier_func: Any, target_file: Path | None = None + self, key: str, value: str, modifier_func: Callable[[str, str, str], str], target_file: Path | None = None ) -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/role_manager.pydocs/ROLES.mdtests/test_role_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_role_management.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/role_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). (3)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
cortex/role_manager.py (1)
59-59: No action needed. The project explicitly requires Python 3.10+ (requires-python = ">=3.10"in pyproject.toml), making PEP 604 union syntax (Path | None,str | None) the correct and recommended approach for this codebase.Likely an incorrect or invalid review comment.
docs/ROLES.md (1)
55-63: The custom role JSON schema in the documentation is correct as written. The"Frontend-Dev"key follows the expected pattern where role dictionary keys are human-readable names (matching the structure of built-in roles like"Web Server","Database Server"). Theslugfield within each role object is distinct from the key and serves as the machine-readable identifier. The implementation inrole_manager.py(lines 86-88) confirms this by merging custom_data directly viaself.roles.update(custom_data), and the test casetest_custom_role_loading(lines 46-62) validates that custom roles use human-readable names as keys (e.g.,"DevOps"). No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 1-16: Remove the unused top-level import of the os module in
role_manager.py: delete the "import os" statement (the symbol os) from the
imports block and ensure no other code references it; after removal, run
linter/tests to verify there are no unused-import warnings or accidental
dependencies on os.
🧹 Nitpick comments (4)
cortex/role_manager.py (4)
74-91: Movejsonimport to module level.Importing
jsoninside the function (line 83) is inefficient and non-idiomatic. Python best practice is to place all imports at the module level unless there's a specific reason for lazy loading (like avoiding circular imports or heavy optional dependencies). The same issue appears in thelearn_packagemethod at line 189.♻️ Proposed fix
At the module level (after line 6):
import copy import fcntl +import json import loggingThen remove the local imports:
if not self.custom_roles_file.exists(): return - import json - try:
111-134: Consider validatingrole_slugbefore persisting.The method persists
role_slugwithout checking if it's a valid slug fromget_all_slugs(). While the CLI may validate this upstream, adding validation here would makeRoleManagermore robust when used programmatically or in future integrations.🛡️ Suggested validation
def save_role(self, role_slug: str) -> None: """ Persists the selected role to the Cortex environment file securely. Args: role_slug: The machine-readable identifier to save. + + Raises: + ValueError: If role_slug is not a valid role identifier. + RuntimeError: If the role cannot be persisted to disk. """ + if role_slug not in self.get_all_slugs(): + raise ValueError(f"Invalid role slug: {role_slug}. Valid slugs: {self.get_all_slugs()}") def modifier(existing_content: str, key: str, value: str) -> str:
154-167: LGTM! Linear search is fine for current scale.The linear search is acceptable given the small number of roles. If the number of roles grows significantly, consider building a reverse lookup dictionary (
slug -> config) for O(1) access.
178-213: Apply the samejsonimport fix here.Line 189 imports
jsoninside the modifier function. This should be moved to the module level as suggested in the earlier comment for_load_custom_roles.Otherwise, the thread-safe implementation using
target_fileproperly addresses the past review concern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/role_manager.pydocs/ROLES.md
🧰 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/role_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). (4)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (13)
cortex/role_manager.py (6)
19-58: LGTM! Well-structured role definitions.The class constants and ROLE_DEFINITIONS dictionary are well-organized. The fix to use more specific ML binaries (
conda,jupyter) instead of the genericpython3addresses the previous concern effectively.
60-72: LGTM! Solid initialization with defensive copying.The deep copy of
ROLE_DEFINITIONSprevents accidental mutation of the class constant, and the automatic loading of custom roles is a nice touch. The type hints follow modern Python conventions.
93-109: LGTM! Efficient role detection.The use of
any()withshutil.which()is efficient and idiomatic. The per-role exception handling allows partial detection success, which is appropriate for this use case.
136-152: LGTM! Appropriate error handling.The method correctly returns
Nonewhen the file doesn't exist or can't be read, which is appropriate for an optional configuration value.
169-176: LGTM! Clean and efficient.Straightforward list comprehension that clearly expresses intent.
215-255: LGTM! Robust thread-safe implementation.The implementation correctly uses:
- Advisory locking with
fcntl.flockfor process-safe access- Atomic writes via temp file and
replace()to prevent corruption- Restrictive file permissions (
0o600) for security- Guaranteed lock release in the
finallyblockThis properly addresses the past review concern about temporary file swapping.
docs/ROLES.md (7)
1-23: LGTM! Clear introduction and usage examples.The documentation provides a clear overview and practical command examples that align with the CLI implementation.
25-42: LGTM! Detection criteria match implementation.The ML Workstation detection binaries now correctly list specific ML tools (
nvidia-smi,nvcc,jupyter,conda) rather than the genericpython3, matching the fix inrole_manager.py.
44-50: LGTM! Accurate description of learning behavior.The documentation correctly describes how Cortex learns from installation patterns and stores them in
learned_roles.json.
52-63: LGTM! Clear custom role example.The JSON example is valid and demonstrates the expected structure. The example shows a "Frontend-Dev" role with slug "frontend", which appropriately illustrates the distinction between display name and machine-readable identifier.
65-87: LGTM! Helpful examples with expected output.The examples clearly demonstrate both detection and role-setting workflows with realistic output that matches the implementation.
89-105: LGTM! Good technical documentation.The section accurately describes the thread-safe implementation and provides a helpful code snippet illustrating the locking mechanism. The reference to the actual implementation helps developers locate the full code.
107-111: LGTM! Useful cross-references.The related commands section helps users discover complementary features and understand how role management fits into the broader Cortex ecosystem.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cortex/role_manager.py (2)
83-89: Consider more specific exception handling.The broad
Exceptioncatch could mask specific errors likePermissionErrororJSONDecodeError. While logging provides visibility, catching more specific exceptions would make debugging easier and allow for different handling strategies.♻️ Proposed refactor
try: # Atomic read of user-defined roles custom_data = json.loads(self.custom_roles_file.read_text()) # Merge custom roles into the active roles dictionary self.roles.update(custom_data) - except Exception as e: - logger.error(f"Failed to load custom roles from {self.custom_roles_file}: {e}") + except json.JSONDecodeError as e: + logger.error(f"Invalid JSON in {self.custom_roles_file}: {e}") + except (OSError, PermissionError) as e: + logger.error(f"Cannot read {self.custom_roles_file}: {e}")
123-125: Minor formatting improvement.The error message string can be simplified for better readability.
♻️ Proposed refactor
if role_slug not in self.get_all_slugs(): - raise ValueError( - f"Invalid role slug: {role_slug}. " f"Valid slugs: {self.get_all_slugs()}" - ) + raise ValueError( + f"Invalid role slug: {role_slug}. Valid slugs: {self.get_all_slugs()}" + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/role_manager.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/role_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). (3)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (9)
cortex/role_manager.py (9)
1-16: LGTM! Module imports and docstring are well-organized.The imports are clean and the module docstring clearly describes the purpose. The previously flagged unused
osimport has been removed.
19-58: LGTM! Role definitions are well-structured.The ROLE_DEFINITIONS are clear and the ML Workstation detection now uses specific indicators (nvidia-smi, nvcc, conda, jupyter) instead of the generic
python3binary. This addresses the previous concern about overly broad detection.
60-72: LGTM! Initialization is defensive and well-documented.Using
copy.deepcopyto avoid mutating the class constant is a good defensive practice, and the initialization logic is clear.
91-107: LGTM! Role detection is resilient and efficient.The use of
shutil.whichfor binary detection is appropriate, and the per-role exception handling ensures that one misconfigured role doesn't break detection for others.
127-142: LGTM! Role persistence logic is robust.The modifier function correctly handles both updating existing keys and appending new ones, with appropriate edge case handling for trailing newlines.
144-160: LGTM! Role retrieval is defensive with appropriate fallbacks.The method correctly handles missing files and parse errors by returning
None, which is a reasonable default for optional configuration.
162-184: LGTM! Recommendation and slug retrieval methods are straightforward.Both
get_recommendations_by_slugandget_all_slugshave clear logic and appropriate return types.
186-219: LGTM! Package learning implementation is thread-safe and robust.The use of the
target_fileparameter addresses the previous concern about temporarily swappingself.env_file. The modifier function correctly handles malformed JSON and prevents duplicate entries.
221-261: LGTM! File locking implementation is secure and atomic.The implementation correctly uses
fcntlfor process-safe locking, atomic writes via temporary file, and restrictive file permissions (0o600). The lock is properly released in thefinallyblock.
|
@Anshgrover23 can i get a review on this PR please ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/role_manager.py (3)
74-91: Consider validating custom role structure before merging.Custom roles are merged without validating that they contain required keys (
slug,binaries,recommendations). Malformed entries will causeKeyErrorexceptions later indetect_active_rolesorget_recommendations_by_slug.♻️ Proposed validation
custom_data = json.loads(self.custom_roles_file.read_text()) - # Merge custom roles into the active roles dictionary - self.roles.update(custom_data) + # Validate and merge custom roles into the active roles dictionary + required_keys = {"slug", "binaries", "recommendations"} + for role_name, config in custom_data.items(): + if not isinstance(config, dict) or not required_keys.issubset(config.keys()): + logger.warning(f"Skipping invalid custom role '{role_name}': missing required keys") + continue + self.roles[role_name] = config
138-142: Chain the original exception for better debugging.The
RuntimeErrorloses the original exception context. Usefrom eto preserve the traceback for easier debugging.♻️ Proposed fix
except Exception as e: logger.error(f"Failed to save system role: {e}") - raise RuntimeError(f"Could not persist role to {self.env_file}") + raise RuntimeError(f"Could not persist role to {self.env_file}") from e
186-219: Learning mechanism is well-implemented.Good use of the
target_fileparameter for thread-safe writes tolearned_roles.json. Silent failure is appropriate for this non-critical feature.One edge case: if
learned_roles.jsonis manually edited anddata[key]becomes a non-list type, lines 207-208 would fail. Consider a defensive check if you want extra robustness.♻️ Optional defensive check
if key not in data: data[key] = [] + elif not isinstance(data[key], list): + data[key] = [] # Reset corrupted entry # Only add the package if it's not already recorded
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/role_manager.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/role_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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (8)
cortex/role_manager.py (8)
1-17: Imports and module setup look good.All imports are used, and the module docstring clearly describes the purpose. Note that
fcntlis Unix-only, which limits Windows compatibility—acceptable if Cortex targets Linux systems only.
19-58: Class structure and role definitions are well-designed.Good use of class-level constants to prevent string typos. The
ROLE_DEFINITIONSdictionary is well-typed and the detection binaries are appropriately specific for each role.
60-72: Initialization is correctly implemented.Good use of
copy.deepcopyto avoid mutating the class-levelROLE_DEFINITIONS. Type hints and docstring are properly provided.
93-109: Detection logic is correctly implemented.Using
shutil.which()withany()short-circuit evaluation is efficient and cross-platform. The broad exception handler ensures one malformed role config doesn't break detection of others.
144-160: LGTM!Safe error handling with fallback to
None, and correct regex pattern for parsing the.envformat.
162-175: LGTM!The linear search is appropriate given the small number of roles, and returning an empty list is a safe fallback.
177-184: LGTM!Simple helper method correctly implemented.
221-261: Thread-safe file operations are correctly implemented.Excellent implementation of the atomic write pattern with proper advisory locking. Key strengths:
- Uses
target_fileparameter to avoid mutating instance state (addressing past review).- Exclusive lock with proper release in
finallyblock.- Atomic swap via
temp_file.replace(target).- Secure permissions with
chmod(0o600).
CLA Verification PassedAll contributors have signed the CLA.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cortex/role_manager.py (1)
211-220: Remove dead code.Lines 215-216 are unreachable. The condition
if key not in datais already handled by the first check on line 212 (if key not in data or not isinstance(data[key], list)), so this second block will never execute.♻️ Proposed fix
# Ensure the entry is a list, even if the file was manually corrupted or edited if key not in data or not isinstance(data[key], list): data[key] = [] - if key not in data: - data[key] = [] - # Only add the package if it's not already recorded if value not in data[key]: data[key].append(value)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/role_manager.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/role_manager.pycortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/role_manager.py (5)
RoleManager(19-273)detect_active_roles(100-116)get_all_slugs(185-192)save_role(118-150)get_recommendations_by_slug(170-183)
⏰ 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: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (14)
cortex/cli.py (4)
26-26: LGTM!Import is correctly placed and follows the existing pattern.
273-327: Well-structured command handler.The
rolemethod follows established CLI patterns with proper type hints, docstring, graceful error handling, and validation. The dual validation (CLI-level on lines 305-308 and RoleManager-level insave_role) provides defense in depth.
2265-2274: LGTM!CLI parser additions follow established patterns and provide clear help text for both subcommands.
2589-2590: LGTM!Command routing is correctly integrated with the existing structure.
cortex/role_manager.py (10)
1-17: LGTM with platform consideration.Module is well-organized with a clear docstring. Note that
fcntlis Unix-only, which aligns with Cortex's Linux focus but will prevent Windows usage.
19-58: LGTM!Class structure is clean with well-documented constants. Using typed class constants for role slugs prevents typos and enables IDE autocomplete.
60-72: LGTM!Good defensive programming with
copy.deepcopyto prevent mutation of the class constant. Type hints and docstring are complete.
74-98: LGTM!Robust custom role loading with proper validation and graceful error handling. Invalid roles are skipped with warnings rather than failing the entire load.
100-116: LGTM!Detection logic is robust with per-role error handling. Using
shutil.whichis the correct portable approach for checking binary availability.
118-150: LGTM!Well-designed save logic with input validation, atomic updates, and proper exception chaining for debugging context. The modifier function correctly handles both existing key updates and new key appends.
152-168: LGTM!Defensive implementation that gracefully returns
Noneon errors rather than crashing.
170-183: LGTM!Returns empty list for unknown slugs, which is a safer API design than returning
None.
185-192: LGTM!Simple and correct implementation.
233-273: LGTM!Solid thread-safe and process-safe file update implementation. The pattern of atomic write via temp file +
replace()prevents corruption, and thetry/finallyensures the lock is always released.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 145-154: The modifier function uses a plain substring check (if
f"{key}=" in existing_content) which can false-positive when the key appears
inside another value; instead use regex detection consistently: compute pattern
= rf"^{key}=.*$" and use re.search(pattern, existing_content,
flags=re.MULTILINE) to decide whether to call re.sub or append; update the
modifier function (symbol: modifier) to replace the substring check with
re.search, leaving the existing re.sub call and the append logic intact.
In @README.md:
- Line 152: Update the README entry for the "cortex role detect" command to
clarify it only analyzes and displays AI recommendations and does not persist
changes; explicitly state that only "cortex role set" persists a role (via
manager.save_role()), and adjust the table cell text to remove "and set system
role" so it accurately reflects the behavior implemented in the detect handler
(which only shows recommendations).
🧹 Nitpick comments (3)
cortex/role_manager.py (2)
36-72: LGTM with a minor suggestion! Robust error handling for shell history.The defensive error handling and
errors="ignore"encoding strategy are appropriate for handling potentially corrupted history files. The early return on first match is intentional and works correctly.Optional refinement: Consider documenting shell priority order
Since the method returns on the first existing history file,
.bash_historytakes precedence over.zsh_history. Consider adding a note in the docstring if this priority is intentional:""" Senses user activity patterns from local shell history files. + + Priority order: .bash_history, then .zsh_history (returns on first match).
182-219: LGTM! Solid atomic write implementation with advisory locking.The file locking and atomic swap pattern correctly prevents data corruption from concurrent writes. The
0o600permissions appropriately restrict access to the user only.Consider documenting advisory locking limitation
The
fcntladvisory locking only works if all processes cooperate. Consider adding a note in the docstring:""" Standardized utility for atomic, thread-safe file updates. Implements an advisory locking mechanism using fcntl and an atomic swap pattern. Note: Advisory locks only prevent concurrent modifications from processes that also use fcntl locking. """This is informational only - the implementation is correct for Cortex's use case.
tests/test_role_management.py (1)
1-153: LGTM! Comprehensive test coverage for the new feature.The test suite covers critical scenarios including:
- Happy path context gathering
- Error handling and graceful degradation
- File persistence and updates
- Edge cases (missing files, no history, idempotency)
The tests are well-structured with clear docstrings and appropriate mocking. The claimed 82.79% coverage is well-supported by these tests.
Optional: Consider testing key collision edge case
Related to my comment on
cortex/role_manager.pylines 145-154, you might add a test for the edge case where a key name appears in another variable's value:def test_save_role_key_in_value_edge_case(temp_cortex_dir): """Ensure key detection doesn't match key names within other values.""" env_path = temp_cortex_dir / ".env" env_path.write_text("OTHER_KEY=contains_CORTEX_SYSTEM_ROLE_text\n") manager = RoleManager(env_path=env_path) manager.save_role("web-server") content = env_path.read_text() # Should have exactly one CORTEX_SYSTEM_ROLE at line start assert content.count("CORTEX_SYSTEM_ROLE=") == 1 assert "CORTEX_SYSTEM_ROLE=web-server" in content
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pr-title-check.ymlREADME.mdcortex/cli.pycortex/role_manager.pydocs/ROLES.mdtests/test_role_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ROLES.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pytests/test_role_management.pycortex/role_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.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 **/*sandbox*.py : Implement Firejail sandboxing for package operations
Applied to files:
README.md
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/role_manager.py (3)
RoleManager(12-219)get_system_context(74-128)save_role(130-161)
tests/test_role_management.py (1)
cortex/role_manager.py (4)
get_system_context(74-128)_get_shell_patterns(36-72)save_role(130-161)get_saved_role(163-180)
⏰ 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: Test (Python 3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (23)
.github/workflows/pr-title-check.yml (1)
7-9: LGTM! Good security practice.Explicitly limiting permissions to
pull-requests: readfollows the principle of least privilege for thepull_request_targettrigger. This is the correct approach for workflows that only need to read PR metadata.README.md (4)
70-70: LGTM! Feature description is clear and accurate.The description "Autonomous role detection and contextual package suggestions" accurately reflects the capabilities of the new RoleManager module.
208-211: LGTM! Architecture diagram update is clear.The addition of the "AI Role Sensing (Context Layer)" box appropriately represents the new RoleManager component in the system architecture.
227-227: LGTM! Project structure accurately reflects the new module.The addition of
role_manager.pyto the project structure is accurate and appropriately documented.
346-346: LGTM! Checklist item appropriately marks feature completion.The completed checklist item accurately reflects the implementation of the role management feature.
cortex/role_manager.py (4)
1-24: LGTM! Clean imports and well-documented class structure.The imports are minimal and appropriate, type hints follow modern Python conventions (
|union syntax), and the class docstring clearly explains the "sensing layer" purpose.
26-34: LGTM! Constructor design supports testability.The optional
env_pathparameter enables dependency injection for testing while providing a sensible default for production use.
74-128: LGTM! Comprehensive context gathering with clear design rationale.The "Persona Pollution" comment is valuable for understanding the design choice to use
active_roleinstead ofrole_history. The fallback to "undefined" when no role is saved is appropriate.
163-180: LGTM! Robust read logic with appropriate error handling.The method gracefully handles missing files and read errors by returning
None, which aligns with the "undefined" role fallback inget_system_context.cortex/cli.py (5)
26-26: LGTM! Import follows existing patterns.The RoleManager import is clean and consistent with other module imports in the file.
293-327: LGTM!detectaction correctly shows recommendations without persisting.The implementation appropriately separates detection (analysis and display) from persistence. This aligns with the expected behavior where
detectis exploratory andsetis definitive.Note: There's a minor documentation inconsistency in README.md line 152 that suggests
detectalso sets the role, but the implementation correctly only displays recommendations.
2316-2336: LGTM! CLI integration follows established patterns.The argument parser setup for the
rolecommand is consistent with other subcommand structures in the CLI, with clear help text and appropriate required arguments.
2650-2651: LGTM! Command routing is straightforward.The routing for the
rolecommand follows the same pattern as other commands and correctly delegates tocli.role(args).
329-379: LGTM! Solid implementation with error handling and pragmatic optimizations.The cache-busting timestamp (line 347) and structured prompt format (lines 351-362) function as designed. The timestamp embeds a unique identifier into the prompt itself, which changes the semantic cache key (
f"ask:{question}") on each call, ensuring fresh AI recommendations for each role change. The structured example format is a legitimate prompt engineering optimization for smaller models. No evidence found that these model-specific approaches degrade the user experience.tests/test_role_management.py (9)
1-16: LGTM! Test setup provides proper isolation.The
temp_cortex_dirfixture correctly uses pytest'stmp_pathto create isolated test environments, preventing pollution of the user's home directory.
18-46: LGTM! Comprehensive test of context gathering.The test effectively validates all components of the system context dict (binaries, has_gpu, patterns, active_role) with appropriate mocking of external dependencies.
48-64: LGTM! Validates graceful error handling.The test confirms that
_get_shell_patternsdoesn't crash when history files are inaccessible, which is important for robustness across different Linux environments.
66-84: LGTM! Validates atomic role updates.The test confirms that updating an existing role replaces the value rather than appending, which is critical for maintaining .env file integrity.
86-97: LGTM! Validates preservation of existing env variables.This test ensures that the role management doesn't interfere with other environment variables, which is essential for a production-ready .env file manager.
99-115: LGTM! Comprehensive error handling validation.The test verifies both the error message clarity and proper exception chaining, which aids debugging when file operations fail.
117-123: LGTM! Validates missing file handling.The test confirms that
get_saved_rolegracefully returnsNonefor missing files, which is the expected behavior for first-time users.
125-135: LGTM! Validates idempotent save operations.The test confirms that repeatedly saving the same role doesn't create duplicate entries, which is important for file integrity and prevents bloat.
137-153: LGTM! Validates behavior for minimal environments.The test confirms that the system gracefully handles first-time users or minimal installations where no history or binaries are detected.
Kesavaraja67
force-pushed
the
feat/role-management
branch
from
cc72a45 to
154121f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 130-162: The save_role flow allows unsanitized role_slug into the
.env via modifier and re.sub which enables newline or '=' injection and
misinterprets backslashes; validate/sanitize the role_slug at the start of
save_role (e.g., reject or canonicalize any value containing newline, '\r' or
'=' and trim/whitelist characters) and raise a clear ValueError before calling
_locked_read_modify_write; in modifier, make the regex safe by using re.escape
on the key when building pattern and use a safe replacement (e.g., supply a
function or escape the replacement) so backslashes in role_slug aren’t treated
as backreferences; keep error handling that raises RuntimeError from the
original exception and reference CONFIG_KEY and env_file as before.
🧹 Nitpick comments (7)
tests/test_role_management.py (4)
10-15: Good fixture; consider typing for readability.
Optional:def temp_cortex_dir(tmp_path: Path) -> Path:to make intent explicit.
18-46: Mocks are a bit too broad; this test can pass for the “wrong” reason.
Patchingcortex.role_manager.Path.exists/read_textglobally affects both shell history and.envreads insideget_saved_role(), soactive_role == "undefined"is currently an artifact of the patched history text not containing the key—not necessarily a true “no saved role” scenario. Consider targeting mocks to just the history paths (e.g., patchPath.home()totmp_pathand create real.bash_history,.zsh_history,.envfiles), or patchRoleManager.get_saved_roleseparately to isolate concerns.
99-115: Good assertion of exception chaining; patch scope is very wide.
Patchingbuiltins.openwill also affectPath.touch()/Path.write_text()internals, so the failure may occur earlier than the comment implies (not necessarily “file lock”). If you want to specifically test lock acquisition failure, patchfcntl.flockor patchopenonly when opening the lock file (via a small wrapper/indirection), but current test still validates the CLI-facing contract (RuntimeErrorwith__cause__).
125-135: Idempotency test is good, but doesn’t cover pre-existing duplicates.
If you intend to “self-heal” user-edited.envfiles with multipleCORTEX_SYSTEM_ROLElines, add a test for that; otherwise current coverage is fine.cortex/role_manager.py (3)
74-129:get_system_context()looks coherent; consider avoiding doublewhich("nvidia-smi").
Minor: you already computedetected_binaries;has_gpucould be derived from"nvidia-smi" in detected_binariesto avoid an extra PATH lookup.
163-181:get_saved_role()regex should escape the key and consider common.envvariants.
If you keep this generic (takesCONFIG_KEY), preferre.escape(self.CONFIG_KEY). Optionally supportexport CORTEX_SYSTEM_ROLE=...if you expect users to edit the file manually.
182-219: Atomic write approach is solid; considerfsync()if durability matters.
If you need crash-durability guarantees (power loss after write, before rename), you’ll want to flush andos.fsync()the temp file (and possibly the directory). If best-effort is fine, current approach is OK.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdcortex/cli.pycortex/role_manager.pydocs/ROLES.mdtests/test_role_management.py
✅ Files skipped from review due to trivial changes (1)
- docs/ROLES.md
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_role_management.pycortex/role_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.py
🧬 Code graph analysis (1)
tests/test_role_management.py (1)
cortex/role_manager.py (5)
RoleManager(12-219)get_system_context(74-128)_get_shell_patterns(36-72)save_role(130-161)get_saved_role(163-180)
⏰ 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 (6)
tests/test_role_management.py (5)
48-64: Failure-handling coverage is good (PermissionError ⇒ []).
This is a reasonable resilience test for varied environments.
66-84: Nice coverage of “update existing key, don’t duplicate”.
This validates the expected steady-state.envinvariant.
86-97: Good guard against clobbering unrelated env vars.
117-123: Good missing-file behavior coverage.
137-153: Good first-run behavior coverage (“undefined” + empty patterns).cortex/role_manager.py (1)
1-8: No action required. The project explicitly declares Python >=3.10 (requires-python = ">=3.10"in pyproject.toml) and POSIX::Linux support ("Operating System :: POSIX :: Linux"in classifiers). The use of PEP 604 union syntax (Path | None) and thefcntlmodule are both fully compatible with the declared runtime and platform matrix, which is validated by CI testing against Python 3.10, 3.11, and 3.12 on ubuntu-latest.Likely an incorrect or invalid review comment.
|
@Anshgrover23 i have addressed the change you have mentioned and also fixed the code which was suggested by the coderabbitai. Can you review my PR now... |
There was a problem hiding this comment.
@Kesavaraja67 Missing "Learning from installations" feature. Also use Professional English for conversations.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 331-340: The code calling manager.save_role(role_slug) in the
"set" branch must catch ValueError from RoleManager and show a user-friendly
message; wrap the save_role call in a try/except ValueError as e, call
self._print_error (or cx_print) with a clear message including str(e) (e.g.,
"Invalid role slug: <msg>") and return a non-zero status (1) so the CLI exits
gracefully; let other exceptions bubble up to the existing generic handler.
- Around line 276-329: The role() method doesn't handle args.role_action being
None and can return None; add an explicit branch to handle a missing subcommand
(check action = getattr(args, "role_action", None) and if action is None) that
prints a brief usage/error (e.g., via console.print or cx_print) and returns a
non-zero int (e.g., return 2) so the CLI always returns an int; place this
branch before the existing if action == "detect": block or as an else: at the
end to ensure role() always returns an int.
🧹 Nitpick comments (2)
cortex/role_manager.py (1)
48-59: Consider refining theexportredaction pattern.The pattern
r"(?i)export\s+[^\s]+=[^\s]+"will redact allexportcommands, including benign ones likeexport PATH=/usr/binorexport EDITOR=vim. This may hide useful technical context from the AI.Consider targeting only sensitive-looking exports:
♻️ Proposed refinement
- r"(?i)export\s+[^\s]+=[^\s]+", # Catch environment variable exports + r"(?i)export\s+(?:[^\s]*(?:key|token|secret|password|passwd|credential|auth)[^\s]*)=[^\s]+", # Catch sensitive exportstests/test_role_management.py (1)
173-195: Consider adding edge case tests for slug validation boundaries.The slug validation regex requires slugs to start and end with alphanumeric characters. Consider adding tests for boundary cases like slugs ending with
-or_(e.g.,"dev-") which should be rejected.💡 Optional test additions
def test_save_role_slug_boundary_validation(temp_cortex_dir): """Verify slugs must start and end with alphanumeric chars.""" env_path = temp_cortex_dir / ".env" manager = RoleManager(env_path=env_path) # Single character should be valid manager.save_role("m") assert manager.get_saved_role() == "m" # Slugs ending with dash/underscore should be invalid with pytest.raises(ValueError): manager.save_role("dev-") with pytest.raises(ValueError): manager.save_role("dev_") # Slugs starting with dash/underscore should be invalid with pytest.raises(ValueError): manager.save_role("-dev")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdcortex/cli.pycortex/role_manager.pydocs/ROLES.mdtests/test_role_management.py
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/ROLES.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/role_manager.pytests/test_role_management.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.py
🧬 Code graph analysis (3)
cortex/role_manager.py (1)
cortex/logging_system.py (2)
warning(204-206)debug(196-198)
tests/test_role_management.py (1)
cortex/role_manager.py (4)
get_system_context(89-141)_get_shell_patterns(36-87)save_role(143-178)get_saved_role(180-196)
cortex/cli.py (2)
cortex/role_manager.py (3)
RoleManager(12-227)get_system_context(89-141)save_role(143-178)cortex/first_run_wizard.py (1)
_print_error(778-780)
⏰ 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.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (14)
cortex/cli.py (2)
2304-2323: LGTM!The role subparser structure is well-defined and follows the established patterns in the codebase. Help text is clear and descriptive.
4-8: LGTM!The new imports are appropriately added for the cache-busting nonce generation (
random,string) and request ID creation (uuid). TheRoleManagerimport enables the new role management functionality.Also applies to: 29-29
cortex/role_manager.py (5)
12-35: LGTM!The class is well-documented with a clear docstring explaining its purpose as a "sensing layer" for AI inference. The initialization handles defaults appropriately and uses proper type hints.
89-141: LGTM!The sensing layer implementation is thorough with good multi-vendor GPU support and a comprehensive list of signature binaries. The method aggregates system facts cleanly for AI inference.
143-178: LGTM!The slug validation regex correctly handles short slugs (single char), uppercase, dashes, and underscores while preventing injection attacks. The modifier function properly escapes the key and uses a lambda to avoid backreference issues. Exception chaining with
from eprovides good debuggability.
180-196: LGTM!The method correctly handles the missing file case and catches exceptions gracefully. The regex pattern safely matches the config key at line start.
198-227: LGTM with minor observation.The atomic write implementation is solid with proper advisory locking, temp file usage, restricted permissions (0o600), and atomic swap via
replace(). The lock file (.lock) is created but never cleaned up, which could accumulate over time, though this is a minor concern for CLI usage.tests/test_role_management.py (7)
1-15: LGTM!Clean fixture setup providing proper isolation for file I/O tests. The
temp_cortex_dirfixture correctly creates a.cortexsubdirectory matching the real structure.
18-46: LGTM!Good test structure using real files in the temp directory combined with targeted mocks. Validates the key aspects of the sensing layer including binary detection, GPU flag, shell patterns, and default role state.
49-68: LGTM!Good defensive test verifying that permission errors during history file access don't crash the sensing layer but return an empty list gracefully.
71-138: LGTM!Excellent coverage of the persistence layer including atomic updates, idempotency, exception chaining, and graceful handling of missing files. The tests properly verify that existing environment variables are preserved.
141-156: LGTM!Tests the first-time user scenario where no shell history or binaries exist. The broad
Path.existspatch is acceptable here since it correctly simulates a clean environment.
159-195: LGTM!Critical edge case testing that ensures the regex anchoring works correctly (key in value doesn't match) and that the slug validation accepts valid variants while rejecting injection attempts.
198-224: LGTM!Thorough test for PII redaction verifying that sensitive data (API keys, secrets) are scrubbed while safe technical signals (
ls -la) are preserved for AI context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cortex/cli.py (2)
379-379: Unreachable return statement.Line 379 returns
1as a safety fallback, but all code paths above already return a value (lines 286, 334, 339, 348, 376). This line is unreachable.♻️ Remove unreachable code
console.print("[bold cyan] cortex install <package_name>[/bold cyan]\n") return exit_code - - # Safety fallback to ensure an integer return status. - return 1
4-5: Consider documenting cache-busting necessity or reconsidering its use.The
random,string, anduuidimports are used to generate unique identifiers (nonce, timestamp, req_id) in LLM prompts at lines 305–306 and 354 for cache-busting (as noted in the code comments).While this approach is intentional, it has trade-offs:
- Adds overhead to every role detection/set operation
- Makes testing and debugging harder due to non-deterministic prompts
- Its necessity depends on your LLM provider's caching behavior
If cache-busting is required, add a code comment explaining why your LLM provider necessitates this approach. Otherwise, consider removing these imports and the cache-busting logic.
cortex/role_manager.py (2)
90-142: Consider caching system context for performance.The
get_system_context()method scans for 26 binaries usingshutil.which()on every call (line 128). For frequently used commands likecortex role detect, this could add noticeable overhead.Consider implementing a simple cache with a TTL (e.g., 5 minutes) to avoid rescanning the system on every call:
from functools import lru_cache import time @lru_cache(maxsize=1) def _get_cached_context_with_time(self, cache_time): # Implementation here ... def get_system_context(self): # Cache for 5 minutes cache_time = int(time.time() / 300) return self._get_cached_context_with_time(cache_time)Alternatively, if real-time accuracy is critical, the current implementation is acceptable.
199-228: Solid atomic write implementation with advisory locking.The
_locked_read_modify_write()method implements a correct atomic file update pattern:
- Creates parent directories safely (line 210)
- Uses advisory file locking with fcntl (line 216)
- Writes to temporary file with secure permissions (lines 221-223)
- Performs atomic rename (line 226)
- Ensures lock release in finally block (line 228)
One consideration: the lock acquisition at line 216 blocks indefinitely. For a local config file this is likely acceptable, but if you anticipate contention, consider using
fcntl.LOCK_EX | fcntl.LOCK_NBwith retry logic.Would you like me to generate an implementation with lock timeout handling?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/role_manager.pytests/test_role_management.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/role_manager.pycortex/cli.pytests/test_role_management.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (13)
cortex/cli.py (3)
276-286: LGTM!The method signature includes proper type hints and has a clear docstring. The guard clause for missing action is appropriate and provides a helpful error message.
2317-2336: LGTM!The argument parser setup for role commands is well-structured with clear help text and proper argument definitions.
2651-2652: LGTM!Command routing for the role command follows the established pattern and correctly delegates to the
cli.role(args)method.cortex/role_manager.py (5)
24-34: LGTM!The
__init__method is well-structured with proper type hints, a clear docstring, and sensible defaults for configuration paths.
36-88: Solid PII redaction implementation.The shell pattern sensing with PII redaction is well-implemented:
- Comprehensive regex patterns for sensitive data (API keys, tokens, passwords, headers)
- Proper error handling with logging
- UTF-8 encoding with error handling for corrupted data
- Reasonable limit of 15 recent commands
The redaction logic at lines 77-81 correctly applies multiple patterns and filters out "cortex role set" commands to avoid circular references.
144-179: Well-implemented atomic role persistence.The
save_role()method has several strong qualities:
- Robust slug validation preventing injection attacks (line 157)
- Proper exception handling with chaining (line 179)
- Atomic file updates via
_locked_read_modify_write()(line 176)- Correct regex escaping to prevent pattern injection (line 163)
The modifier function correctly handles both update and append scenarios with proper newline management.
181-197: LGTM!The
get_saved_role()method correctly handles missing files and parse errors, with appropriate logging and fallback behavior.
1-1: Add platform guard forfcntlimport or document Linux-only requirement.The
fcntlmodule is Unix-specific and will raiseImportErroron Windows. While the codebase explicitly targets Linux (documented incortex/config_manager.py),role_manager.pyimports and usesfcntl.flock()(lines 216, 228) without guards. Either wrap the import in a try-except block, add a platform check (consistent with patterns used elsewhere in the codebase likecortex/env_loader.pyandcortex/sandbox/sandbox_executor.py), or add clear documentation stating Linux-only support.tests/test_role_management.py (5)
10-15: LGTM!The
temp_cortex_dirfixture provides proper test isolation by creating a temporary.cortexdirectory that doesn't pollute the user's home directory.
18-46: Excellent test isolation and coverage.This test correctly isolates the filesystem using patches and temporary directories while creating real files for realistic testing. The assertions comprehensively verify binary detection, GPU status, pattern extraction, and active role.
49-68: LGTM!This test correctly verifies that
_get_shell_patterns()handles permission errors gracefully by returning an empty list instead of crashing.
71-88: LGTM!This test verifies the atomic update logic correctly replaces existing role values without creating duplicate entries.
91-245: Comprehensive test suite with excellent coverage.The remaining tests provide thorough coverage of:
- Preserving unrelated environment variables (91-101)
- Error handling with proper exception chaining (104-119)
- Missing file scenarios (122-126)
- Idempotent operations (129-138)
- Edge cases like keys in values (159-170)
- Slug validation with various formats (173-195, 227-245)
- PII redaction robustness (198-224)
The test suite achieves >80% coverage as required and follows pytest best practices with clear docstrings, proper fixtures, and focused assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cortex/cli.py (1)
336-376: Suggest refining prompt and error formatting.The
setaction correctly validates input and handles errors fromRoleManager.save_role(). Two refinements:
Line 347:
str(e)is redundant in f-strings, which automatically convert exceptions to strings.Line 364: The prompt instruction "Do NOT repeat previous suggestions" assumes LLM state persistence, but each CLI invocation is stateless. Without conversation history or stored suggestions, this instruction may be ineffective.
♻️ Proposed fix for redundant str() call
- self._print_error(f"Invalid role slug: {str(e)}") + self._print_error(f"Invalid role slug: {e}")
📜 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 (3)
cortex/role_manager.py (3)
RoleManager(12-228)get_system_context(90-142)save_role(144-179)cortex/first_run_wizard.py (1)
_print_error(778-780)cortex/branding.py (1)
cx_print(49-69)
⏰ 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: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (5)
cortex/cli.py (5)
4-5: LGTM - Secure and purposeful imports.The new imports support the role management feature:
secrets+stringenable cryptographically secure nonce generationuuidprovides unique request identifiersRoleManagerintegration adds the sensing layerAll imports are actively used in the
role()method.Also applies to: 8-8, 29-29
276-286: LGTM - Clean command validation.The method signature follows PEP 8 with proper type hints and docstring. The subcommand validation pattern is consistent with other CLI handlers in this file (e.g.,
notify()at line 209,sandbox()at line 505).
288-334: Well-structured role detection with secure nonce generation.The
detectaction correctly delegates sensing toRoleManager.get_system_context(), which handles errors internally. The cryptographically secure nonce generation (line 305) usingsecrets.choice()is best practice for cache-busting identifiers.The LLM prompt is verbose but appears intentional for guiding the AI response format.
2317-2337: LGTM - Standard argparse configuration.The role command parser follows the established pattern used throughout the file (e.g.,
notifyat line 2299,sandboxat line 2355). The help text clearly describes each subcommand's purpose.
2651-2652: LGTM - Standard command routing.The role command routing follows the consistent pattern used for all other commands in the
main()function.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 184-200: get_saved_role currently returns an empty string when the
.env contains "CORTEX_SYSTEM_ROLE="; update the method (in get_saved_role) to
normalize whitespace-only/empty matches to None by extracting the captured value
from match.group(1).strip() into a variable and returning that variable if
truthy, otherwise return None; keep the existing regex and error handling so
non-matching or invalid/empty configured roles are treated the same as a missing
file.
🧹 Nitpick comments (5)
cortex/cli.py (2)
307-307: Consider usingsecrets.token_urlsafe()for cleaner nonce generation.The current loop-based approach works but is more verbose than necessary. The standard library provides a more idiomatic way to generate random strings.
♻️ Cleaner nonce generation
- nonce = "".join(secrets.choice(string.ascii_letters + string.digits) for _ in range(12)) + nonce = secrets.token_urlsafe(9)[:12] # Generate 12 URL-safe characters
311-326: Extract hardcoded prompt templates to improve maintainability.The prompt construction spans 15+ lines with embedded formatting requirements. If prompt engineering evolves or multiple commands need similar prompts, this becomes difficult to maintain.
Consider extracting prompts to a dedicated module (e.g.,
cortex/prompts.py) or constants at the module level with f-string formatting. This centralizes prompt management and makes A/B testing or prompt versioning easier.cortex/role_manager.py (3)
27-37: Consider validatingenv_pathfor earlier error detection.The
__init__method accepts an optionalenv_pathbut doesn't validate it. If a caller passes an invalid path (e.g., a directory, or a location where the parent directory doesn't exist or isn't writable), the error won't surface untilsave_role()is called later.For better error locality, consider validating that the parent directory exists or can be created:
def __init__(self, env_path: Path | None = None) -> None: self.env_file = env_path or (Path.home() / ".cortex" / ".env") # Validate parent directory if not self.env_file.parent.exists(): logger.warning("Parent directory for env_file does not exist: %s", self.env_file.parent) self.history_db = Path.home() / ".cortex" / "history.db"This is optional since
_locked_read_modify_writealready doesmkdir(parents=True, exist_ok=True)on line 213.
51-63: Consider adding patterns for common cloud provider credentials.The current redaction patterns are comprehensive, but some common environment variables might slip through:
AWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEY(AWS)GITHUB_TOKEN,GITLAB_TOKEN(Git hosting)GOOGLE_APPLICATION_CREDENTIALS(GCP)While the generic
tokenpattern on line 54 may catch some of these, adding explicit patterns improves coverage:sensitive_patterns = [ # ... existing patterns ... r"(?i)AWS_(?:ACCESS_KEY_ID|SECRET_ACCESS_KEY)\s*[:=]\s*[^\s]+", r"(?i)(?:GITHUB|GITLAB)_TOKEN\s*[:=]\s*[^\s]+", r"(?i)GOOGLE_APPLICATION_CREDENTIALS\s*[:=]\s*[^\s]+", ]
93-145: Consider using TypedDict for better type safety in return value.The method returns
dict[str, Any], which loses type information. The caller (CLI) accesses specific keys likecontext.get("binaries")andcontext.get("has_gpu"), but the IDE and type checker can't validate these.Define a TypedDict for the return type:
from typing import TypedDict class SystemContext(TypedDict): binaries: list[str] has_gpu: bool patterns: list[str] active_role: str has_install_history: bool def get_system_context(self) -> SystemContext: # ... existing implementation ...This improves IDE autocomplete and catches typos at type-checking time.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/role_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pycortex/role_manager.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/role_manager.py (2)
get_system_context(93-145)save_role(147-182)cortex/first_run_wizard.py (1)
_print_error(778-780)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (7)
cortex/cli.py (3)
2316-2336: LGTM! Well-structured argument parser for role management.The subparser setup is clean with descriptive help text. The
detectandsetsubcommands are properly defined, and the requiredrole_slugargument forsetensures correct usage.
2650-2651: LGTM! Proper routing for the role command.The routing correctly delegates to
cli.role(args)when the command is "role".
288-290: No error handling wrapper needed forget_system_context().The method is already designed with comprehensive internal error handling. The helper methods
_get_shell_patterns()andget_saved_role()both catch exceptions and return safe defaults (empty list and None respectively). The base operations (shutil.which(),Path.exists()) do not raise exceptions. Since all failure paths are handled internally, the method cannot raise exceptions and will always return a dict with safe default values.Likely an incorrect or invalid review comment.
cortex/role_manager.py (4)
1-4: Document that thread-safe locking is unavailable on Windows.The fcntl guard on lines 1-4 allows the code to run on Windows, but lines 219-220 and 232-233 only apply locking when
fcntlis available. On Windows,fcntlisNone, so the locking is skipped, which means concurrent writes to the.envfile are not protected.While the PR objectives emphasize "Thread-Safe Persistence" using fcntl advisory locking, this guarantee only holds on POSIX systems. Since the project is "cortexlinux," this may be acceptable, but the limitation should be documented in the class or method docstring.
Consider adding a note to the class docstring or raising an informative warning on non-POSIX systems:
if fcntl is None: logger.warning("Thread-safe locking unavailable on this platform")Also applies to: 219-220, 232-233
147-183: LGTM! Robust validation and atomic persistence.The role slug validation on line 160 correctly enforces:
- Alphanumeric start and end
- Optional middle characters (alphanumeric, dash, underscore)
- Rejects empty strings, trailing dashes, and malicious input
The modifier function safely updates the config file using regex replacement with proper escaping, and error handling provides clear context when persistence fails.
227-227: Fixed permissions ensure security but don't preserve originals.Line 227 sets the file permissions to
0o600(user read/write only), which is a secure default for a.envfile containing potentially sensitive configuration. However, this doesn't preserve the original permissions if the user had customized them.For a
.envfile, this is likely the correct approach for security, but it's worth noting as intentional behavior.
202-233: LGTM! Solid atomic write implementation with advisory locking.The implementation correctly:
- Creates parent directories as needed (line 213)
- Uses advisory locking for concurrency control (lines 220, 233)
- Writes to a temporary file before atomic swap (lines 225-230)
- Ensures cleanup in a finally block (lines 231-233)
The atomic
replace()operation on line 230 is POSIX-compliant and ensures data integrity even if the process crashes mid-write.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cortex/cli.py (2)
5-5: Unused import:stringmodule is not referenced.The
stringmodule is imported but never used in the code. Consider removing it to keep imports clean.🧹 Proposed fix
import argparse import logging import os import secrets -import string import sys import time import uuid
307-308: Redundant slice in nonce generation.
secrets.token_urlsafe(9)already produces exactly 12 characters (9 bytes encoded as base64), so the[:12]slice is unnecessary. While harmless, it can be simplified.♻️ Proposed simplification
- nonce = secrets.token_urlsafe(9)[:12] + nonce = secrets.token_urlsafe(9)cortex/role_manager.py (4)
62-77: Comprehensive PII redaction patterns with room for improvement.The regex patterns cover major credential types well. However, consider these additions:
- Private keys:
r"(?i)-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----"- Database URLs:
r"(?i)(?:postgres|mysql|mongodb)://[^@]+@"(captures user:pass in connection strings)- SSH passwords:
r"(?i)sshpass\s+-p\s+[^\s]+"♻️ Proposed additional patterns
r"(?i)AWS_(?:ACCESS_KEY_ID|SECRET_ACCESS_KEY)\s*[:=]\s*[^\s]+", r"(?i)(?:GITHUB|GITLAB)_TOKEN\s*[:=]\s*[^\s]+", r"(?i)GOOGLE_APPLICATION_CREDENTIALS\s*[:=]\s*[^\s]+", + r"(?i)-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----", + r"(?i)(?:postgres|mysql|mongodb)://[^@\s]+:[^@\s]+@", + r"(?i)sshpass\s+-p\s+[^\s]+", ]
94-98: Minor performance consideration: repeated regex matching.The comprehension at line 95 runs all 16+ regex patterns against each command. For 15 commands, this is ~240 regex operations. While acceptable for this small scale, consider precompiling patterns if this becomes a bottleneck:
compiled_patterns = [re.compile(p) for p in sensitive_patterns] # Then use: any(p.search(cmd) for p in compiled_patterns)This is a micro-optimization and not necessary now given the small input size.
209-213: Consider explicit encoding for cross-platform consistency.Line 210 uses
read_text()without specifying encoding, which defaults to the system's locale encoding. On Windows, this could be cp1252 or other non-UTF-8 encodings, potentially causing issues if the role slug contains non-ASCII characters.♻️ Proposed fix
try: - content = self.env_file.read_text() + content = self.env_file.read_text(encoding="utf-8") match = re.search(rf"^{self.CONFIG_KEY}=(.*)$", content, re.MULTILINE)
225-226: Expand docstring for critical atomic write operation.The current docstring is minimal for such a critical operation. Consider adding details about:
- The atomic write-then-swap pattern
- Advisory locking behavior (POSIX only)
- The purpose of the modifier_func parameter
- Exception handling expectations
📝 Proposed enhanced docstring
def _locked_read_modify_write( self, key: str, value: str, modifier_func: Callable[[str, str, str], str], target_file: Path | None = None, ) -> None: """ Performs an atomic, thread-safe file update with advisory locking. Uses a write-to-temp-then-replace pattern to ensure atomicity. On POSIX systems, fcntl advisory locks prevent concurrent writes. On Windows, locking is unavailable but atomicity is still provided by the OS-level file replace operation. Args: key: Configuration key to update value: New value for the key modifier_func: Function that takes (existing_content, key, value) and returns the modified content target_file: Optional override for the target file path (defaults to self.env_file) Raises: OSError: If file operations fail Exception: Propagates exceptions from modifier_func """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/role_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pycortex/role_manager.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/role_manager.py (3)
RoleManager(25-249)get_system_context(107-160)save_role(162-197)cortex/first_run_wizard.py (1)
_print_error(778-780)
⏰ 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: Test (Python 3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (13)
cortex/cli.py (6)
276-286: LGTM! Well-structured subcommand validation.The method correctly validates the presence of a subcommand and returns early with a clear error message.
288-303: Consider error handling for context retrieval.The
get_system_context()call could potentially raise exceptions (e.g., permission errors when reading shell history). While the RoleManager handles these internally with logging, consider wrapping this in a try-except block to provide user-friendly error messages if the context gathering fails entirely.As per coding guidelines, the RoleManager methods may raise exceptions that should be caught here for better UX.
311-326: Verify prompt injection safety with shell patterns.The prompt embeds
patterns_strwhich contains user shell history. While RoleManager redacts sensitive patterns, confirm that the redaction is comprehensive enough to prevent prompt injection or exposure of unintended commands.Based on learnings, shell history can contain arbitrary user input. Review the redaction patterns in RoleManager._get_shell_patterns() to ensure coverage of edge cases.
338-350: Excellent error handling for role validation.The try-except block properly catches
ValueErrorfromsave_role()and provides a user-friendly error message, returning an appropriate exit code.
2316-2336: LGTM! Clear and well-structured command definitions.The role command parser is properly configured with descriptive help text and appropriate subcommands. The
role_slugargument for the "set" subcommand is correctly marked as required.
2650-2651: LGTM! Consistent command routing.The dispatch logic for the "role" command follows the same pattern as other commands in the CLI.
cortex/role_manager.py (7)
1-4: Advisory locking disabled on Windows - acceptable trade-off.The graceful fallback for
fcntl(unavailable on Windows) means file locking is silently disabled on that platform. Since these are advisory locks for process-safe writes, and Cortex appears to be Linux-focused (per PR objectives), this is an acceptable design choice. Just be aware that concurrent writes on Windows could theoretically corrupt the config file.Based on learnings, this approach is consistent with Linux-first design philosophy.
15-23: LGTM! Well-structured TypedDict.The
SystemContextTypedDict provides clear type hints and documentation for the system context structure, aligning with the coding guidelines requirement for type hints.
37-47: LGTM! Clean constructor with sensible defaults.The constructor follows coding guidelines with type hints and docstrings. The default paths are appropriate, and the optional
env_pathparameter enables testing flexibility.
87-87: Good practice: error-tolerant encoding.Using
errors="ignore"when reading shell history is appropriate since these files may contain binary data or encoding issues.
107-160: Excellent system context aggregation.The method effectively collects system signals with:
- Comprehensive binary detection covering multiple domains (web, DB, ML, dev ops)
- Multi-vendor GPU detection (NVIDIA, AMD, Intel)
- Clean use of
shutil.which()for PATH scanning- Appropriate fallback handling for
get_saved_role()withor "undefined"
162-197: Robust role persistence with excellent validation.The method demonstrates best practices:
- Line 175: Strong regex validation prevents injection attacks and malformed slugs
- Lines 179-191: The modifier function correctly handles both update and append cases with proper newline handling
- Line 186: Using a lambda prevents backslash escaping issues in regex substitution
- Lines 193-197: Proper exception wrapping with context preservation
228-249: Excellent atomic write implementation.The method demonstrates best practices for safe file updates:
- Line 229: Creates parent directories as needed
- Lines 234-236: Uses advisory locking (POSIX) with proper fallback
- Line 243: Sets restrictive 0o600 permissions on temp file (user-only access)
- Line 246: Uses
Path.replace()for atomic swap (guaranteed by OS)- Lines 247-249: Ensures lock release in finally block
The write-to-temp-then-replace pattern prevents corruption if the process crashes mid-write.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 222-270: In _locked_read_modify_write, the file reads/writes use
platform-default encodings; update calls to target.read_text() and
temp_file.write_text() to explicitly pass encoding="utf-8" (e.g., existing =
target.read_text(encoding="utf-8") and temp_file.write_text(updated,
encoding="utf-8")), preserving the rest of the logic (including chmod and
replace) so file operations are consistently UTF-8 encoded.
🧹 Nitpick comments (2)
cortex/role_manager.py (1)
49-107: Consider expanding PII redaction patterns for cloud credentials.The current redaction patterns are solid and cover common cases (API keys, tokens, passwords, SSH keys, database URLs). However, you could enhance coverage by adding:
- Azure credentials:
(?i)AZURE_(?:CLIENT_SECRET|TENANT_ID|SUBSCRIPTION_ID)- GCP service accounts:
(?i)\.json.*service_account- Docker registry credentials:
(?i)docker\s+login.*-p\s+[^\s]+- SSH agent forwarding:
(?i)ssh-add.*-kThese additions would improve protection against accidental credential exposure in shell history.
🔐 Proposed additional patterns
r"(?i)-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----", r"(?i)(?:postgres|mysql|mongodb)://[^@\s]+:[^@\s]+@", r"(?i)sshpass\s+-p\s+[^\s]+", + r"(?i)AZURE_(?:CLIENT_SECRET|TENANT_ID|SUBSCRIPTION_ID)\s*[:=]\s*[^\s]+", + r"(?i)GCP_(?:SERVICE_ACCOUNT|CREDENTIALS)\s*[:=]\s*[^\s]+", + r"(?i)docker\s+login.*-p\s+[^\s]+", ]cortex/cli.py (1)
4-5: Remove unused import.The
stringmodule is imported on line 5 but doesn't appear to be used anywhere in the role management implementation. Removing unused imports keeps the codebase clean and passes linters.🧹 Proposed cleanup
import secrets -import string import sys
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/role_manager.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/role_manager.pycortex/cli.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (11)
cortex/role_manager.py (6)
1-12: LGTM! Clean imports and proper cross-platform handling.The conditional
fcntlimport with fallback is the correct pattern for cross-platform compatibility (Windows doesn't have fcntl). Module-level logger initialization follows best practices.
15-22: LGTM! Well-defined typed dictionary.Clear structure with proper type annotations and documentation. Provides a strong contract for system context data.
25-47: LGTM! Well-documented class with proper initialization.The class docstring clearly describes its role as a "sensing layer". Type hints and documentation meet requirements. Default path handling is appropriate.
109-162: LGTM! Comprehensive system sensing with multi-vendor support.The signal detection covers a broad range of use cases (Web, DB, ML, DevOps, Development) and includes multi-vendor GPU detection. The implementation correctly uses
shutil.which()for PATH-based detection and returns a properly typed SystemContext dictionary.
164-199: LGTM! Robust role slug validation and atomic persistence.The validation regex correctly allows professional slug formats (including single characters, mixed case, and separators). The modifier function properly handles both update and append cases with correct newline handling. Good use of lambda to avoid regex replacement gotchas.
201-220: LGTM! Clean read logic with proper normalization.Explicit UTF-8 encoding ensures consistency. The normalization of whitespace-only values to None (lines 216-217) is a good defensive practice.
cortex/cli.py (5)
276-286: LGTM! Proper method signature and validation.Type hints and docstring are present as required. Early validation of the subcommand prevents downstream errors.
288-336: LGTM! Well-designed AI prompt with effective cache busting.The cache-busting strategy using cryptographic randomness (line 307) and microsecond timestamps (line 308) is effective for preventing stale LLM responses. The prompt structure is clear and provides comprehensive context for AI inference.
The prompt design follows good practices:
- Unique session identifiers prevent caching
- Structured environmental context
- Clear, specific task requirements
- Primed response format
338-378: LGTM! Robust error handling and well-structured recommendations flow.The error handling properly catches
ValueErrorfrom role slug validation and provides user-friendly feedback. The unique request ID generation (line 356) effectively prevents cached responses. The advisory prompt clearly instructs the AI to provide fresh, role-specific recommendations.
2316-2336: LGTM! Clean CLI argument structure.The argument parser setup is well-organized with clear help messages. The
detectandsetsubcommands are properly configured with appropriate arguments.
2650-2651: LGTM! Standard command routing.The routing follows the established pattern used by other CLI commands.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 117-170: In get_system_context, the list comprehension
detected_binaries = [bin for bin in signals if shutil.which(bin)] shadows the
built-in bin(); rename the loop variable (e.g., signal or s) to avoid masking
bin and potential confusion; update the comprehension to detected_binaries =
[signal for signal in signals if shutil.which(signal)] and ensure any related
references remain consistent (get_system_context, signals, detected_binaries,
shutil.which).
- Around line 209-229: get_saved_role currently assumes UTF-8 and a strict
"CORTEX_SYSTEM_ROLE=VALUE" format; update get_saved_role to read env_file bytes
and decode with errors='replace' (or open with encoding="utf-8",
errors="replace") to avoid decode exceptions, then use a more tolerant regex
anchored per-line that allows optional leading "export", optional spaces around
the key and "=", and optional surrounding single or double quotes around the
value (use self.CONFIG_KEY in the pattern), normalize the captured group by
stripping whitespace and surrounding quotes, and return None when no meaningful
value is found while still logging non-fatal errors via logger.error.
- Around line 49-115: _get_shell_patterns currently reads raw shell history and
returns full commands (even when not matched) and recompiles regexes each call;
add an opt-out and stronger minimization plus precompilation: introduce a
module-level precompiled_sensitive_patterns (precompile all entries from
sensitive_patterns) and replace the inline list with that; add an env/config
flag (e.g., SENSE_SHELL_HISTORY or CORTEX_SENSE_HISTORY) check at the start of
_get_shell_patterns to return [] immediately when disabled; when enabled,
transform non-sensitive commands into coarse-grained intent tokens (map common
verbs like install, start, kubectl/helm, git, docker -> "intent:install",
"intent:k8s", "intent:git", etc.) instead of returning raw commands, and keep
exact "<redacted>" for matched sensitive patterns; preserve the existing
exception handling and keep filtering out "cortex role set".
🧹 Nitpick comments (1)
cortex/role_manager.py (1)
1-4: Makefcntlimport/type explicit for type-checkers and readability.Right now
fcntlflips between “module” andNone, which can confuse static analysis; consider annotating it (or importingModuleType) to keep tooling happy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/role_manager.py
🚧 Files skipped from review as they are similar to previous changes (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/role_manager.py
🧬 Code graph analysis (1)
cortex/role_manager.py (4)
cortex/transaction_history.py (1)
search(429-463)cortex/logging_system.py (2)
warning(204-206)debug(196-198)cortex/graceful_degradation.py (1)
match(280-313)cortex/kernel_features/llm_device.py (1)
open(158-159)
⏰ 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.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
cortex/role_manager.py (1)
172-208: Role slug validation + atomic update hook look solid.The
re.fullmatch(...)validation (Line 185-187) prevents newline/=injection into the env file, and the modifier function cleanly updates-or-appends the key.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 257-314: Modify _locked_read_modify_write to avoid lock/temp
filename collisions by using the full target filename when creating companion
files (e.g., create lock_file with target.parent.joinpath(f"{target.name}.lock")
and temp_file with target.parent.joinpath(f"{target.name}.tmp") instead of
with_suffix), and add symmetric error handling around temp_file.chmod(0o600)
(wrap it in a try/except like the lock_file.chmod block) so chmod failures on
some platforms are ignored safely; keep the existing locking, read/modify/write,
replace and cleanup logic intact.
In @tests/test_role_management.py:
- Around line 288-297: The test docstring for
test_get_shell_patterns_corrupted_data claims it verifies errors="replace" but
the code in role_manager._get_shell_patterns reads history files with
errors="ignore"; update either the test docstring to state it verifies
errors="ignore" or change the file-reading call inside _get_shell_patterns to
use errors="replace" to match the docstring; locate the history file read in
_get_shell_patterns and make the corresponding change (or edit the test
docstring) so the comment and implementation are consistent.
🧹 Nitpick comments (6)
tests/test_role_management.py (3)
56-76: Patch scope is a bit broad; consider targeting the instance path read only.
patch.object(Path, "read_text", ...)affects allPath.read_textcalls in this test context. It works here, but it’s easy to accidentally expand coverage later and make it flaky.
148-164: Avoid patchingcortex.role_manager.Path.existsif you only need “no history”.
This also forcesself.history_db.exists()false (maybe intended), and can mask regressions inget_saved_role()behavior if the test expands. Prefer patchingPath.home+ “no history files exist” on disk.
205-232: Redaction test is valuable; consider asserting per-line redaction.
Right now it only checks substrings don’t appear. Adding an assertion like “both sensitive lines become<redacted>” would make leaks harder to reintroduce.cortex/role_manager.py (3)
11-25: Conditional imports are fine; consider logging when no locking backend is available.
Iffcntl is Noneandmsvcrt is None, updates become non-atomic under concurrency with no signal to the user.
202-229:save_role()updates can unexpectedly dropexport/ quotes from existing files.
If a user already hasexport CORTEX_SYSTEM_ROLE="foo", your replace path rewrites it toCORTEX_SYSTEM_ROLE=bar. If you want to preserve formatting, capture and re-emit the prefix/quote style.
230-255: Parsing is “tolerant”, but still misses common cases (leading spaces, duplicates, inline comments).
Suggestions:
- allow
^\s*before optionalexport- if multiple matches exist, prefer the last (shell semantics)
- optionally ignore trailing
# commentwhen unquoted
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/role_manager.pytests/test_role_management.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/role_manager.pytests/test_role_management.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.py
🧬 Code graph analysis (1)
tests/test_role_management.py (1)
cortex/role_manager.py (4)
get_system_context(156-200)_get_shell_patterns(87-154)save_role(202-228)get_saved_role(230-255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
tests/test_role_management.py (5)
10-23: Fixtures look clean and properly isolate$HOME/I/O.
Usingtmp_pathand injectingenv_pathkeeps tests hermetic.
25-54: Good “fact gathering” coverage; mocks are scoped and realistic.
Patchingcortex.role_manager.Path.home(module-local) + writing a real history file avoids globalPathsurprises.
78-109: Role persistence tests are strong (update vs append + idempotency).
These assertions map well to the “single key, no duplication” contract.
111-127: Nice: verifies exception chaining (raise ... from e).
This makes CLI error reporting much easier to debug.
274-286: Windows fallback is exercised, but it doesn’t validate real locking constraints.
Sincemsvcrtis mocked, this won’t catch issues like “locking a 1-byte range on an empty lock file”. Consider adding an assertion that the lock file is non-empty (or update implementation to ensure it), then assert the locking call args.cortex/role_manager.py (1)
156-200:get_system_context()looks consistent and easy to test.
Nice touch avoiding built-in shadowing (signal).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @cortex/role_manager.py:
- Around line 301-387: The Windows locking path in _locked_read_modify_write
uses msvcrt.locking(lock_fd.fileno(), ...) without positioning the file handle,
so the byte-range lock may be applied at an unexpected offset; before calling
msvcrt.locking(...) for both acquire and release, seek the lock_fd to a known
offset (e.g., lock_fd.seek(0)) and optionally truncate/ensure at least one byte
is present so the 1-byte lock refers to a stable region; update references to
lock_fd in the acquire and release branches (msvcrt.locking(..., msvcrt.LK_LOCK,
1) and msvcrt.locking(..., msvcrt.LK_UNLCK, 1)) to perform the seek(0) first so
locking/unlocking target the same byte.
In @tests/test_role_management.py:
- Around line 36-66: The test patches Path.home only after creating the
RoleManager, so any home-derived attributes set in RoleManager.__init__ (e.g.,
history_db) still point to the real home; fix by applying the Path.home patch
before instantiating RoleManager or by explicitly overriding manager.history_db
(and any other home-derived attributes) to point to temp_cortex_dir after
construction; locate RoleManager.__init__, the test function
test_get_system_context_fact_gathering, and references to
manager.history_db/get_system_context to update the test accordingly.
- Around line 156-170: The test's caplog assertion is brittle because it doesn't
set a capture level or logger; before calling role_manager.save_role in
test_locked_write_concurrency_degraded_logging, call
caplog.set_level(logging.WARNING, logger="cortex.role_manager") (and add an
import for the logging module if missing) so the warning from the role_manager
logger is reliably captured; alternatively use caplog.set_level(logging.WARNING)
to capture warnings from the root logger, then assert "No file locking backend
available" in caplog.text.
🧹 Nitpick comments (4)
tests/test_role_management.py (2)
11-34: Good isolation for.envwrites; consider also isolatinghistory_dbto avoid host leakage.
RoleManager(env_path=...)still deriveshistory_dbfrom the real home dir, soget_system_context()["has_install_history"]can depend on the machine running tests (even if you’re not asserting it today).
249-264: Windows locking test: good intent; consider asserting call args for the locked byte range.
Today it only checks.called. If the implementation ever accidentally locks 0 bytes or unlocks incorrectly, this won’t catch it.cortex/role_manager.py (2)
77-86:env_pathoverride should probably also relocatehistory_db.
Right nowRoleManager(env_path=some/dir/.env)still usesPath.home()/.cortex/history.db, which is surprising and complicates test isolation.Proposed refactor
class RoleManager: @@ def __init__(self, env_path: Path | None = None) -> None: @@ - self.env_file = env_path or (Path.home() / ".cortex" / ".env") - self.history_db = Path.home() / ".cortex" / "history.db" + base_dir = (env_path.parent if env_path else (Path.home() / ".cortex")) + self.env_file = env_path or (base_dir / ".env") + self.history_db = base_dir / "history.db"
87-187:_get_shell_patterns()is generally robust, but Windows path minimization is incomplete.
You only basename-strip when"/" in verb; on Windows you can leak local paths withC:\...(backslashes). Consider normalizing both separators before emittingintent:{verb}.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/role_manager.pytests/test_role_management.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/role_manager.pytests/test_role_management.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_role_management.py
🧬 Code graph analysis (2)
cortex/role_manager.py (1)
cortex/logging_system.py (2)
warning(204-206)debug(196-198)
tests/test_role_management.py (1)
cortex/role_manager.py (4)
get_system_context(188-232)_get_shell_patterns(87-186)save_role(234-266)get_saved_role(268-299)
⏰ 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: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (4)
tests/test_role_management.py (1)
68-97: Nice coverage of privacy hardening behaviors (zsh format, env assignment stripping, path minimization, malformed quoting).
This is the right level of “behavioral contract” testing for_get_shell_patterns().cortex/role_manager.py (3)
1-25: Conditionalfcntl/msvcrtimport pattern looks solid and testable.
Keeping these as module globals also makes your monkeypatch-based tests straightforward.
29-37:SystemContextis clear and appropriately minimal.
The fields align with what you actually populate inget_system_context().
268-300:get_saved_role()parsing is tolerant and matches the tests well.
Picking the last match is the right call for typical shell override semantics.
@Kesavaraja67 Please correct the English and ensure the comments are written in a clear, professional manner, in line with our contribution guidelines. |
|
@Anshgrover23, I’ve cleaned up the documentation and comments to meet the contribution guidelines. Everything is now written in a more formal, professional tone. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_role_management.py (1)
409-426: Clarify test intent: This tests defensive cleanup, not a real failure path.The
breaking_modifiermanually creates the temp file (line 419-420), but in the actual_locked_read_modify_writeimplementation,temp_file.write_text()is called aftermodifier_func()(line 414 in role_manager.py). So the temp file wouldn't exist if the modifier crashes.This test validates that the
finallyblock defensively cleans up orphaned temp files, which is valuable for scenarios like crashes duringtemp_file.write_text()ortemp_file.replace(). Consider updating the docstring to clarify this tests defensive cleanup rather than a specific failure mode.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/role_manager.pytests/test_role_management.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/role_manager.pytests/test_role_management.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_role_management.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/role_manager.py
🧬 Code graph analysis (2)
cortex/role_manager.py (1)
cortex/context_memory.py (1)
Pattern(46-56)
tests/test_role_management.py (1)
cortex/role_manager.py (5)
get_system_context(193-249)_get_shell_patterns(98-191)save_role(251-332)get_saved_role(334-360)_locked_read_modify_write(362-432)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (17)
tests/test_role_management.py (7)
17-54: Well-designed hermetic test fixtures.The
temp_cortex_dirandrole_managerfixtures provide excellent test isolation, ensuring tests don't interact with the actual user configuration. The use ofenv_path.parentto derivebase_dirandhome_pathis a clean approach for redirecting all derived paths (history.db, shell history) to the temporary directory.
57-107: Solid integration test covering the sensing layer.This test effectively validates the aggregation of multiple data sources (binary detection, shell history, and installation database). The JSON array format for packages aligns with the schema expected by
_get_learned_patterns.
110-138: Comprehensive privacy hardening validation.Good coverage of Zsh metadata stripping, environment variable assignments, and cross-platform path normalization. The double-escaped backslashes correctly simulate a Windows path in the history file.
194-227: Excellent Windows locking precision test.This test validates the critical
seek(0)requirement for deterministic Windows byte-range locking. The assertions confirm both the seek calls and the 1-byte lock parameter, which is essential for correct cross-platform behavior.
267-284: Thorough slug validation testing.Good coverage of valid variants (including single-char "a" and uppercase slugs) and malicious inputs. The invalid slugs correctly test the regex boundaries for leading/trailing special characters and injection attempts.
353-389: Well-structured module reload test with proper isolation.The use of
reload_lockprevents race conditions during parallel test execution, and the cleanup in thefinallyblock ensures other tests aren't affected. This is a necessary but tricky test for cross-platform import handling.
470-486: Good privacy boundary test.This test validates the "safe harbor" logic that prevents leaking descriptive filenames that could contain sensitive project information. The 20-character limit is a reasonable threshold for command name complexity.
cortex/role_manager.py (10)
12-28: Clean cross-platform conditional imports.The explicit type annotations (
fcntl: ModuleType | None) and platform-specific import guards are well-structured. Only attempting to importmsvcrton Windows (line 22) is a sensible optimization.
31-50: Well-defined typed interface.The
SystemContextTypedDict provides a clear contract for the sensing layer output. Good documentation of the class purpose.
52-75: Comprehensive PII detection patterns.Good coverage of credential types including cloud provider secrets (AWS, GCP, Azure), CI/CD tokens (GitHub, GitLab), and database connection strings. Precompilation is efficient for repeated pattern matching.
77-96: Good path isolation for testing.The constructor properly isolates test paths from production by using
env_path.parentas the base directory. This ensureshistory_dbis also redirected during tests. As per coding guidelines, audit logging goes to~/.cortex/history.db.
98-191: Well-implemented privacy-preserving shell history parsing.Excellent layered approach to privacy: opt-out mechanism, PII redaction, environment variable stripping, path normalization, and command name length limits. The 15-command window balances recency with context.
193-249: Solid system sensing implementation.Good coverage of binary signals across server software, ML tools, and DevOps utilities. The aggregation of shell patterns and learned installation patterns provides rich context for AI inference.
251-332: Robust role persistence with audit trail.Good implementation with:
- Strict input validation preventing injection
- Atomic updates preserving user formatting
- Audit logging to
history.dbas required by coding guidelines- Non-blocking audit failures to ensure primary operation succeeds
334-360: Tolerant role parsing.Good defensive implementation that handles user-edited files with various formats (export prefix, quotes, comments) and corrupted data gracefully.
362-432: Correct cross-platform atomic file updates.The implementation properly handles:
- POSIX advisory locking via
fcntl.flock- Windows byte-range locking with mandatory
seek(0)for deterministic behavior- Atomic replacement via
Path.replace()(same-directory guarantee)- Lock release in inner
finallyensures no stale locks- Temp file cleanup in outer
finallyprevents orphaned files
434-505: Good learning loop implementation.The method properly handles:
- Missing database file (early return)
- Malformed JSON data (skip and continue)
- Deduplication while maintaining recency order
- Appropriate logging levels (INFO for success, ERROR for failures)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 274-296: The role method lacks audit logging for both 'detect' and
'set' actions; after a successful detect suggestion and after a successful role
save in role(self, args) (use action from getattr(args, "role_action") and
role_slug from args.role_slug), instantiate InstallationHistory and call
history.record_installation with an appropriate InstallationType (e.g.,
InstallationType.CONFIG or add a new type), the affected packages/roles list
(e.g., [role_slug] or detected roles), the executed command string(s) (e.g.,
[f"cortex role set {role_slug}"] or the detect invocation), and datetime.now(),
mirroring the pattern used in install(), so every role detect/set is recorded to
~/.cortex/history.db.
🧹 Nitpick comments (2)
cortex/cli.py (2)
329-335: Misleading comment about CLI parser.The prompt instructs the AI that "FAILURE TO COMPLY WILL BREAK THE CLI PARSER," but the response from
self.ask(question)is simply displayed to the user without any parsing. Consider either:
- Removing the misleading comment about breaking a parser, or
- Actually parsing the response if structured output is needed
- f"--- STRICT RESPONSE FORMAT ---\n" - f"YOUR RESPONSE MUST START WITH THE NUMBER '1.' AND CONTAIN ONLY THE LIST. " - f"DO NOT PROVIDE INTRODUCTIONS. DO NOT PROVIDE REASONING. DO NOT PROVIDE A SUMMARY. " - f"FAILURE TO COMPLY WILL BREAK THE CLI PARSER.\n\n" + f"--- PREFERRED RESPONSE FORMAT ---\n" + f"Please start with '1.' and provide only the list of roles. " + f"Omit introductions, reasoning, and summaries.\n\n"
391-391: Consider adding a helpful error message for the fallback case.While this line should be unreachable in normal operation (argparse restricts to
detect/set), adding an error message would aid debugging if reached:+ else: + self._print_error(f"Unknown role action: {action}") return 1
📜 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 for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
cortex/cli.py (5)
6-6: LGTM!The new imports are appropriate and both are used within the
rolemethod.Also applies to: 27-27
347-389: LGTM!The
setaction has proper validation and error handling for bothValueError(invalid slug) andRuntimeError(persistence failure). The request ID generation combines a timestamp with a short UUID suffix, which provides sufficient uniqueness for session tracking purposes.
2159-2160: LGTM!The help table entries for
roleandstackcommands are clear and consistent with the existing format.
2330-2351: LGTM!The argparse definitions for the role command are well-structured with clear help strings and appropriate inline comments explaining the purpose of each subcommand.
2666-2667: LGTM!The command routing for
rolefollows the established pattern and correctly delegates to thecli.role(args)method.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Kesavaraja67 Left some major comments.
Kesavaraja67
force-pushed
the
feat/role-management
branch
from
70086a7 to
3f304ad
Compare
…fensive DB checks
…7/cortex into feat/role-management
|
@Anshgrover23 I have addressed the mentioned changes and also updated the PR description video and attached the video below : Video.Project.5.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 366-423: The 'set' branch calls self._get_api_key() and passes its
result into AskHandler without validating it, risking a None API key; update the
block around the calls to self._get_api_key(), self._get_provider(), and
AskHandler(...) to explicitly check the api_key (from _get_api_key) and if
missing call the same error path used elsewhere (print an error and return
non-zero) before constructing AskHandler and calling handler.ask(rec_question),
ensuring the flow uses the validated api_key and does not proceed when None.
- Around line 300-364: The code creates an AskHandler using a possibly None API
key from _get_api_key() which can cause runtime failures; before instantiating
AskHandler, check the result of self._get_api_key() and if it is None or empty,
emit a clear error message (via cx_print or console.print/processLogger), return
a non-zero exit code (or abort the detect flow) and avoid calling
AskHandler(provider=...) and handler.ask; ensure you reference _get_api_key(),
provider via _get_provider(), AskHandler, and handler.ask when implementing this
early validation and error path.
🧹 Nitpick comments (7)
docs/ROLES.md (2)
94-107: Documentation code snippet is incomplete and may confuse readers.The Python snippet illustrating the atomic write pattern is missing context and doesn't match the actual implementation. It shows
temp_file.replace(target)without showing wheretemp_fileortargetare defined, and omits the cross-platform Windows locking (msvcrt) that the implementation supports.Consider either:
- Removing the code snippet and referring readers to the source
- Providing a complete, accurate example that matches the implementation
44-45: Hardcoded coverage percentage will become stale.The "91.11%" coverage statistic will drift as the codebase evolves. Consider either removing the specific percentage or noting it was accurate at time of initial implementation.
tests/test_role_management.py (3)
57-108: Test setup creates DB in temp directory but RoleManager may look elsewhere.The test seeds
db_path = temp_cortex_dir / "history.db"but theRoleManagerinstance on line 70 deriveshistory_dbfromenv_path.parent. Sinceenv_path = temp_cortex_dir / ".env", the manager'shistory_dbwill betemp_cortex_dir / "history.db"— which matches.However, the test doesn't verify that
context["has_install_history"]isTrue, which would confirm the learning loop correctly read the seeded database.💡 Consider adding assertion for has_install_history
# Verify SQLite learning loop sensing. assert "installed:docker-ce" in context["patterns"] assert "installed:kubectl" in context["patterns"] + # Confirm the learning loop detected installation history. + assert context["has_install_history"] is True
353-389: Module reload test is fragile and may cause test pollution.This test manipulates
sys.modulesand reloads the module to test conditional imports. While thereload_lockhelps with parallel execution, module reloads can still cause subtle issues:
- Other tests running concurrently might see the modified module state
- The
finallyblock restores modules but relies on monkeypatch cleanupConsider isolating this test in a subprocess or marking it to run serially.
409-426: Test creates temp file outside the locked write mechanism.The
breaking_modifiercreatestemp_cortex_dir / ".env.tmp"directly, but the actual_locked_read_modify_writemethod creates its own temp file. The assertion on line 426 checks that the externally-created file doesn't exist, but this doesn't actually test cleanup of the method's internal temp file.The test logic is correct for verifying that the
finallyblock in_locked_read_modify_writecleans up temp files, since the method will create.env.tmpbefore calling the modifier. However, the modifier'stemp_file.touch()is redundant.💡 Simplify the breaking modifier
def breaking_modifier(content, key, value): """Modifier function that intentionally crashes after creating a temp file.""" - temp_file = temp_cortex_dir / ".env.tmp" - temp_file.touch() raise RuntimeError("Unexpected crash during write")cortex/role_manager.py (2)
55-78: Comprehensive PII redaction patterns.The precompiled regex patterns cover a broad range of sensitive data including API keys, tokens, passwords, authorization headers, cloud provider credentials, and database connection strings. This is a solid privacy-hardening implementation.
Consider adding patterns for:
- SSH keys in arguments (e.g.,
ssh -i /path/to/key)- JWT tokens (Base64-encoded with
.separators)
266-269: Consider stricter minimum length for role slugs.The regex
[a-zA-Z0-9](?:[a-zA-Z0-9_-]*[a-zA-Z0-9])?allows single-character slugs like"a". While technically valid, this may be too permissive for meaningful role identifiers.If intentional, this is fine. If you want to require at least 2 characters:
💡 Optional: Require minimum 2-character slugs
- if not re.fullmatch(r"[a-zA-Z0-9](?:[a-zA-Z0-9_-]*[a-zA-Z0-9])?", role_slug): + if not re.fullmatch(r"[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]|[a-zA-Z0-9]{2}", role_slug):Or simpler:
- if not re.fullmatch(r"[a-zA-Z0-9](?:[a-zA-Z0-9_-]*[a-zA-Z0-9])?", role_slug): + if len(role_slug) < 2 or not re.fullmatch(r"[a-zA-Z0-9](?:[a-zA-Z0-9_-]*[a-zA-Z0-9])?", role_slug):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdcortex/cli.pycortex/installation_history.pycortex/role_manager.pydocs/ROLES.mdtests/test_role_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/installation_history.pycortex/cli.pytests/test_role_management.pycortex/role_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_role_management.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.pycortex/role_manager.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/role_manager.py (3)
RoleManager(44-507)get_system_context(196-252)save_role(254-332)
tests/test_role_management.py (1)
cortex/role_manager.py (5)
get_system_context(196-252)_get_shell_patterns(101-194)save_role(254-332)get_saved_role(334-360)_locked_read_modify_write(362-432)
🔇 Additional comments (15)
cortex/installation_history.py (1)
35-35: LGTM!Clean addition of the
CONFIGenum member to support audit logging of role management operations. This aligns with the coding guideline to log all operations to~/.cortex/history.db.tests/test_role_management.py (3)
17-34: LGTM!Well-designed fixture providing hermetic filesystem isolation. This prevents tests from interacting with actual user configuration, which is essential for reliable CI/CD.
37-54: LGTM!The
role_managerfixture correctly leveragestemp_cortex_dirto isolate all derived paths includinghistory_db, ensuring complete test isolation.
267-284: Single-character slug "a" is valid but consider if this is intentional.The test validates that
"a"is a valid slug. While the regex[a-zA-Z0-9](?:[a-zA-Z0-9_-]*[a-zA-Z0-9])?technically allows single alphanumeric characters, this may be too permissive for meaningful role identifiers.If single-character slugs should be rejected, update the regex to require a minimum length.
cortex/cli.py (3)
276-298: LGTM!Well-structured method with clear docstring, proper subcommand handling, and early return for missing action. The implementation follows the existing CLI patterns.
351-358: Audit logging records role detection events appropriately.Using
InstallationType.CONFIGfor role-related operations aligns with the coding guideline to log all operations to~/.cortex/history.db. The logged command accurately reflects the user action.
2366-2387: LGTM!Clean parser definitions for the
rolecommand group with appropriate help text. Therole_slugargument forsetcorrectly requires a value.cortex/role_manager.py (8)
1-14: LGTM!Clean imports with appropriate standard library modules. Type hints are properly included.
15-31: LGTM!Conditional imports for cross-platform locking are well-structured. The explicit type annotations for
fcntlandmsvcrtasModuleType | Nonesatisfy type checkers while maintaining runtime flexibility.
34-42: LGTM!The
SystemContextTypedDict provides clear type documentation for the sensing layer output, improving code maintainability and IDE support.
80-99: LGTM!The
__init__method correctly handles both production and test scenarios. Usingenv_path.parentas the base directory during tests ensures proper isolation of all derived paths.
101-194: Well-implemented privacy-preserving shell history extraction.The method correctly:
- Respects
CORTEX_SENSE_HISTORYopt-out- Strips Zsh extended history metadata
- Redacts sensitive patterns before processing
- Uses
errors="replace"for corrupted data resilience- Falls back to simple split on malformed quoting
- Extracts only basenames to prevent path leakage
- Maps known commands to generalized intents
254-332: Robust role persistence with audit logging.The
save_rolemethod:
- Validates slug format before proceeding
- Uses atomic file update via
_locked_read_modify_write- Preserves existing formatting (export prefix, quotes, comments)
- Logs changes to
history.dbfor audit complianceThe audit logging failure is properly handled as non-blocking, which prevents secondary failures from breaking the primary operation. Based on learnings, this aligns with the requirement to log all operations to
~/.cortex/history.db.
362-432: Solid cross-platform atomic file locking implementation.Key strengths:
- Creates lock file with proper permissions (0o600)
- Writes non-empty byte for Windows byte-range locking compatibility
- Uses
seek(0)before msvcrt operations for deterministic locking- Implements write-to-temp-and-swap pattern for atomicity
- Cleans up temp file in
finallyblockOne consideration: the lock file is never cleaned up after operations. This is generally fine (lock files are small), but you may want to document this behavior.
434-507: LGTM!The
_get_learned_patternsmethod correctly:
- Checks for database existence before connecting
- Validates table existence before querying
- Parses JSON package arrays safely
- Maintains recency order while deduplicating
- Limits output to 5 most recent unique packages
- Logs sensing success for audit purposes
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 423-427: The call to handler.ask(rec_question) in the set
subcommand lacks exception handling and can crash on provider errors; wrap the
AskHandler creation and its ask() invocation (referencing self._get_provider(),
AskHandler, and ask) in a try/except block that catches provider/AI errors
(e.g., Exception or a more specific provider error type), log or display a
user-friendly error via console or logger, and ensure the subcommand
returns/continues gracefully instead of propagating the exception.
- Around line 350-354: Wrap the AskHandler call and rendering in a try/except
block: call AskHandler(api_key=api_key, provider=provider) and
handler.ask(question) inside try, then console.print(Markdown(answer)) on
success; catch ValueError, RuntimeError, and ImportError raised by handler.ask
and handle them by logging or printing a clear error via console.print
(including the exception message) and then exit or return to avoid an unhandled
crash; ensure you reference AskHandler, handler.ask, and
console.print(Markdown(answer)) when applying the change.
🧹 Nitpick comments (1)
cortex/cli.py (1)
362-363: Consider using timezone-aware datetime for consistency.
datetime.now()returns a naive datetime, butrole_manager.pyusesdatetime.now(timezone.utc)for audit logging (line 320 in relevant snippet). Using naive datetimes can cause issues when comparing timestamps across different parts of the system.♻️ Suggested fix
+from datetime import datetime, timezone ... history.record_installation( InstallationType.CONFIG, ["system-detection"], ["cortex role detect"], - datetime.now(), + datetime.now(timezone.utc), ) ... history.record_installation( InstallationType.CONFIG, [role_slug], [f"cortex role set {role_slug}"], - datetime.now(), + datetime.now(timezone.utc), )Also applies to: 386-387
📜 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 for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/role_manager.py (2)
get_system_context(196-252)save_role(254-332)cortex/first_run_wizard.py (1)
_print_error(778-780)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
cortex/cli.py (5)
6-6: LGTM!All new imports (
uuid,Markdown,RoleManager) are properly used in the new role management functionality.Also applies to: 11-12, 29-29
2205-2206: LGTM!Help table entries for
roleandstackcommands are appropriately added with clear descriptions.
2376-2398: LGTM!The argparse configuration for the
rolecommand follows existing patterns and provides clear help text for bothdetectandsetsubcommands.
2712-2713: LGTM!Command routing for
rolefollows the established pattern and correctly delegates tocli.role(args).
356-363: Good implementation of audit logging.Both
detectandsetoperations are properly recorded viaInstallationHistory.record_installation(), complying with the coding guideline to log all operations to~/.cortex/history.db. Additionally, theRoleManager.save_role()method records role changes to a separaterole_changestable for audit purposes.Also applies to: 380-387
Kesavaraja67
left a comment
Kesavaraja67
left a comment
There was a problem hiding this comment.
Addressed the mentioned changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Line 318: Timestamps in the CLI are created as naive datetimes in the detect
flow but as UTC-aware datetimes in the set action; change all uses of
datetime.now() that build `timestamp` (e.g., in the detect subcommand and any
other places that set `timestamp`) to use timezone-aware values via
datetime.now(timezone.utc), add the timezone import if missing (from datetime
import timezone), and ensure any comparisons or parsing of stored timestamps
convert to UTC-aware datetimes (e.g., by using .replace(tzinfo=timezone.utc) or
.astimezone(timezone.utc)) so all timestamp generation and comparisons are
consistently UTC-aware.
🧹 Nitpick comments (1)
cortex/cli.py (1)
343-362: Consider extracting duplicate AI invocation logic into a helper method.The pattern of obtaining API key, getting provider, creating
AskHandler, callingask(), and rendering Markdown with identical error handling is duplicated betweendetect(lines 343-362) andset(lines 425-444).♻️ Suggested helper method
def _ask_ai_and_render(self, question: str) -> int: """Invoke AI with question and render response as Markdown. Returns: int: 0 on success, 1 on failure. """ api_key = self._get_api_key() if not api_key: self._print_error( "No API key found. Please configure an API provider or use Ollama." ) return 1 provider = self._get_provider() try: handler = AskHandler(api_key=api_key, provider=provider) answer = handler.ask(question) console.print(Markdown(answer)) return 0 except ImportError as e: self._print_error(str(e)) cx_print( "Install the required SDK or set CORTEX_PROVIDER=ollama for local mode.", "info" ) return 1 except (ValueError, RuntimeError) as e: self._print_error(str(e)) return 1Then in
detectandset, replace the duplicate blocks with:if self._ask_ai_and_render(question) != 0: return 1Also applies to: 425-444
📜 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 for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/role_manager.py (3)
RoleManager(44-507)get_system_context(196-252)save_role(254-332)cortex/installation_history.py (2)
record_installation(259-317)InstallationType(27-35)
⏰ 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). (6)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
cortex/cli.py (6)
6-12: LGTM!Imports are appropriate for the new role management functionality. The
Markdownimport fromrich.markdownenables proper rendering of AI responses, andtimezonesupports timestamp consistency.Also applies to: 29-29
276-298: LGTM!Method structure follows the established pattern for CLI subcommand handlers. Proper docstring with type hints as per coding guidelines, and safe handling of missing action.
379-454: LGTM!The
setaction properly validates input, handles bothValueErrorandRuntimeErrorfromsave_role(), and logs the operation tohistory.dbfor audit compliance. The error handling for unknown commands is correctly structured.
2222-2223: LGTM!Help table entries are concise and follow the existing style.
2393-2414: LGTM!Parser definitions follow the established pattern for subcommand groups. The
role_slugargument includes helpful examples in the help text.
2729-2730: LGTM!Command routing correctly delegates to
cli.role(args).
|
|
@Anshgrover23 Addressed all the suggestions mentioned by the coderabbit and the PR is ready for the final review. |
Anshgrover23
changed the title
3 participants
Related Issue
Closes #119
Summary
This PR implements a robust System Role Management architecture, enabling Cortex to dynamically adapt to a machine's "personality" (e.g., ML Workstation, Web Server). It replaces static configurations with an AI-driven sensing layer, a privacy-hardened shell history parser, and a persistent local learning feedback loop.
Key Features
git,docker) to intent signals while masking unknown or long script names asintent:unknownto prevent filename leaks.msvcrtlocking on Windows with mandatoryseek(0)calls to target a stable 1-byte region, ensuring deterministic locking.fcntl.flockfor robust concurrency control on Unix-based systems.~/.cortex/history.db, capturing the timestamp, configuration key, previous value, and new value.exportprefixes, and inline comments.temp_cortex_dirandrole_managerfixture suite that isolates all file I/O and database operations from the real user home directory.Demo
Video.Project.5.1.mp4
AI Disclosure
Used Gemini 3 Flash as a Capable AI Thought Partner to:
RoleManagersensing logic and privacy-hardening filters.seek(0)precision.pytestsuite to validate audit logging and "Last-One-Wins" formatting preservation.Checklist
pytest tests/test_role_management.py.ruff check(with no undefined variables) andblackformatting.history.db.README.mdand added role-specific documentation.Summary by CodeRabbit
New Features
CLI
Documentation
Tests
Other
✏️ Tip: You can customize this high-level summary in your review settings.