feat: add tiered approval modes withexecution policy, CLI support, an… #459
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a tiered approval system (ApprovalMode enum and policies), persistent user preferences and CLI config to set approval mode, confirmation prompting, and coordinator enforcement of policies during execution; includes docs and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CortexCLI
participant Prefs as UserPreferences
participant Policy as ApprovalPolicy
participant Coord as InstallationCoordinator
participant Confirm as confirm_action()
participant Exec as Executor
User->>CLI: run command
CLI->>Prefs: load()
Prefs-->>CLI: approval_mode
CLI->>Policy: get_approval_policy(mode)
Policy-->>CLI: policy instance
CLI->>Coord: create coordinator (policy)
User->>Coord: execute(steps)
loop per step
Coord->>Policy: allow(action_type)?
alt disallowed
Coord->>Exec: mark skipped
else allowed
Coord->>Policy: requires_confirmation(action_type)?
alt requires confirmation
Coord->>Confirm: prompt user
Confirm->>User: "[y/N]?"
User-->>Confirm: response
Confirm-->>Coord: boolean
alt confirmed
Coord->>Exec: execute action
else declined
Coord->>Exec: mark skipped
end
else no confirmation
Coord->>Exec: execute action
end
end
end
Coord-->>User: final report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
7dd0388 to
de256da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (11)
cortex/approval_policy.py (3)
37-43: Add docstring to SuggestPolicy class.Per coding guidelines, docstrings are required for all public APIs. The concrete policy classes should document their specific behavior.
🔎 Proposed addition
class SuggestPolicy(ApprovalPolicy): + """ + Policy for SUGGEST mode: only allows displaying information. + All actions except 'show' are blocked, and no confirmations are required. + """ def allow(self, action: ActionType) -> bool: return action == "show"As per coding guidelines, docstrings are required for all public APIs.
45-51: Add docstring to AutoEditPolicy class.Per coding guidelines, docstrings are required for all public APIs.
🔎 Proposed addition
class AutoEditPolicy(ApprovalPolicy): + """ + Policy for AUTO_EDIT mode: allows file edits and shell commands. + Shell commands require explicit user confirmation before execution. + """ def allow(self, action: ActionType) -> bool: return action in {"show", "file_edit", "shell_command"}As per coding guidelines, docstrings are required for all public APIs.
53-59: Add docstring to FullAutoPolicy class.Per coding guidelines, docstrings are required for all public APIs.
🔎 Proposed addition
class FullAutoPolicy(ApprovalPolicy): + """ + Policy for FULL_AUTO mode: allows all actions without confirmation. + Commands execute automatically without user prompts. + """ def allow(self, action: ActionType) -> bool: return TrueAs per coding guidelines, docstrings are required for all public APIs.
tests/test_installation_coordinator_approval.py (2)
6-12: Consider a more robust mock for UserPreferences.The current approach using
type("Prefs", (), {...})()to create an anonymous object works but is fragile. Consider using a proper dataclass, named mock object, orunittest.mock.Mockfor better maintainability and clarity.🔎 Alternative approach using dataclass
+from dataclasses import dataclass + +@dataclass +class MockPreferences: + approval_mode: ApprovalMode + def test_suggest_mode_blocks_execution(monkeypatch): # Force suggest mode monkeypatch.setattr( UserPreferences, "load", - lambda: type("Prefs", (), {"approval_mode": ApprovalMode.SUGGEST})(), + lambda: MockPreferences(approval_mode=ApprovalMode.SUGGEST), )Or using
unittest.mock:from unittest.mock import Mock def test_suggest_mode_blocks_execution(monkeypatch): mock_prefs = Mock() mock_prefs.approval_mode = ApprovalMode.SUGGEST monkeypatch.setattr(UserPreferences, "load", lambda: mock_prefs) # ... rest of test
6-22: Consider adding test docstring and expanding coverage.The test function is missing a docstring (required per coding guidelines). Additionally, consider adding tests for AUTO_EDIT and FULL_AUTO modes to ensure comprehensive coverage of the approval policy integration.
🔎 Suggested additions
def test_suggest_mode_blocks_execution(monkeypatch): """Test that SUGGEST mode blocks shell command execution.""" # ... existing test def test_auto_edit_mode_requires_confirmation(monkeypatch): """Test that AUTO_EDIT mode allows execution but requires confirmation.""" # ... test implementation def test_full_auto_mode_executes(monkeypatch): """Test that FULL_AUTO mode executes commands without prompts.""" # ... test implementationAs per coding guidelines, docstrings are required for all public APIs (including test functions for clarity).
tests/test_approval_policy.py (3)
5-10: Add docstring and consider testing all action types.The test function is missing a docstring (required per coding guidelines). Additionally, consider testing the "file_edit" action type to ensure comprehensive coverage of SuggestPolicy behavior.
🔎 Proposed enhancements
def test_suggest_policy(): + """Test that SUGGEST mode only allows 'show' actions and blocks all others.""" policy = get_approval_policy(ApprovalMode.SUGGEST) assert policy.allow("show") assert not policy.allow("shell_command") + assert not policy.allow("file_edit") assert not policy.requires_confirmation("shell_command") + assert not policy.requires_confirmation("file_edit")As per coding guidelines, docstrings are required for all public APIs.
13-17: Add docstring and consider testing file_edit behavior.The test function is missing a docstring. Additionally, consider testing that "file_edit" is allowed and doesn't require confirmation in AUTO_EDIT mode.
🔎 Proposed enhancements
def test_auto_edit_policy(): + """Test that AUTO_EDIT mode allows all actions but requires confirmation for shell commands.""" policy = get_approval_policy(ApprovalMode.AUTO_EDIT) + assert policy.allow("show") + assert policy.allow("file_edit") assert policy.allow("shell_command") + assert not policy.requires_confirmation("show") + assert not policy.requires_confirmation("file_edit") assert policy.requires_confirmation("shell_command")As per coding guidelines, docstrings are required for all public APIs.
20-24: Add docstring and consider testing all action types.The test function is missing a docstring. Additionally, consider testing "show" and "file_edit" actions to ensure comprehensive coverage of FullAutoPolicy.
🔎 Proposed enhancements
def test_full_auto_policy(): + """Test that FULL_AUTO mode allows all actions without requiring confirmation.""" policy = get_approval_policy(ApprovalMode.FULL_AUTO) + assert policy.allow("show") + assert policy.allow("file_edit") assert policy.allow("shell_command") + assert not policy.requires_confirmation("show") + assert not policy.requires_confirmation("file_edit") assert not policy.requires_confirmation("shell_command")As per coding guidelines, docstrings are required for all public APIs.
cortex/coordinator.py (1)
174-186: Confirmation prompt triggers for every step in multi-step installations.In
AUTO_EDITmode,requires_confirmation("shell_command")returnsTrue, causing the user to be prompted for each step in the installation loop. For a 10-step Docker install, this means 10 confirmation prompts.Consider prompting once before the loop in
execute()rather than per-step, or making the prompt message more informative (e.g., include the command being executed).🔎 Option 1: Move confirmation to execute() method (prompt once)
In
execute()method around line 260, add a single upfront confirmation:def execute(self) -> InstallationResult: """Run each installation step and capture structured results.""" start_time = time.time() failed_step_index = None # Single confirmation for all steps in AUTO_EDIT mode if self.approval_policy.requires_confirmation("shell_command"): if not confirm_action(f"Execute {len(self.steps)} planned shell commands?"): # Mark all as skipped and return for step in self.steps: step.status = StepStatus.SKIPPED step.error = "User declined execution" return InstallationResult( success=False, steps=self.steps, total_duration=0, error_message="User declined execution", ) # ... rest of execute logicThen remove the
requires_confirmationcheck from_execute_command.cortex/cli.py (2)
1008-1024: Add docstring to_config_setmethod.Per coding guidelines, docstrings are required for all public APIs. While this is a private method, adding a brief docstring would improve maintainability.
🔎 Proposed fix
def _config_set(self, args: argparse.Namespace) -> int: + """Set a configuration value (currently only approval-mode is supported).""" key = args.key value = args.value
42-43: Remove unusedself.approval_policyfromCortexCLI.__init__.
self.approval_policyis assigned at line 43 but never referenced withinCortexCLI.InstallationCoordinatorindependently loads and uses its own approval policy, making this attribute unnecessary dead code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cortex/approval.pycortex/approval_policy.pycortex/cli.pycortex/confirm.pycortex/coordinator.pycortex/user_preferences.pydocs/approval_modes.mdtests/test_approval_policy.pytests/test_installation_coordinator_approval.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/confirm.pycortex/user_preferences.pytests/test_installation_coordinator_approval.pycortex/approval.pytests/test_approval_policy.pycortex/approval_policy.pycortex/cli.pycortex/coordinator.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_installation_coordinator_approval.pytests/test_approval_policy.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
tests/test_installation_coordinator_approval.py
🧬 Code graph analysis (6)
cortex/user_preferences.py (1)
cortex/approval.py (2)
ApprovalMode(4-26)from_string(15-26)
tests/test_installation_coordinator_approval.py (3)
cortex/coordinator.py (2)
InstallationCoordinator(54-358)execute(260-309)cortex/user_preferences.py (1)
UserPreferences(16-56)cortex/approval.py (1)
ApprovalMode(4-26)
tests/test_approval_policy.py (2)
cortex/approval.py (1)
ApprovalMode(4-26)cortex/approval_policy.py (9)
get_approval_policy(64-75)allow(24-26)allow(38-39)allow(46-47)allow(54-55)requires_confirmation(29-31)requires_confirmation(41-42)requires_confirmation(49-50)requires_confirmation(57-58)
cortex/approval_policy.py (1)
cortex/approval.py (1)
ApprovalMode(4-26)
cortex/cli.py (6)
cortex/approval.py (2)
ApprovalMode(4-26)from_string(15-26)cortex/user_preferences.py (3)
UserPreferences(16-56)load(24-43)save(45-56)cortex/approval_policy.py (1)
get_approval_policy(64-75)cortex/env_manager.py (2)
load(571-595)save(597-631)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/kernel_features/accelerator_limits.py (1)
save(62-67)
cortex/coordinator.py (4)
cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (2)
UserPreferences(16-56)load(24-43)cortex/approval_policy.py (9)
get_approval_policy(64-75)allow(24-26)allow(38-39)allow(46-47)allow(54-55)requires_confirmation(29-31)requires_confirmation(41-42)requires_confirmation(49-50)requires_confirmation(57-58)cortex/first_run_wizard.py (1)
run(169-228)
🪛 GitHub Actions: CI
cortex/approval.py
[error] 26-26: W292 No newline at end of file.
🪛 GitHub Check: lint
cortex/confirm.py
[failure] 6-6: Ruff (W292)
cortex/confirm.py:6:36: W292 No newline at end of file
cortex/user_preferences.py
[failure] 24-24: Ruff (UP037)
cortex/user_preferences.py:24:56: UP037 Remove quotes from type annotation
[failure] 6-6: Ruff (UP035)
cortex/user_preferences.py:6:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 1-8: Ruff (I001)
cortex/user_preferences.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/approval.py
[failure] 26-26: Ruff (W292)
cortex/approval.py:26:23: W292 No newline at end of file
cortex/approval_policy.py
[failure] 75-75: Ruff (W292)
cortex/approval_policy.py:75:59: W292 No newline at end of file
[failure] 1-4: Ruff (I001)
cortex/approval_policy.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/cli.py
[failure] 1-28: Ruff (I001)
cortex/cli.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/coordinator.py
[failure] 1-15: Ruff (I001)
cortex/coordinator.py:1:1: I001 Import block is un-sorted or un-formatted
[failure] 88-88: Ruff (W293)
cortex/coordinator.py:88:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/confirm.py
[failure] 6-6: Ruff (W292)
cortex/confirm.py:6:36: W292 No newline at end of file
cortex/user_preferences.py
[failure] 24-24: Ruff (UP037)
cortex/user_preferences.py:24:56: UP037 Remove quotes from type annotation
[failure] 6-6: Ruff (UP035)
cortex/user_preferences.py:6:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 1-8: Ruff (I001)
cortex/user_preferences.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/approval.py
[failure] 26-26: Ruff (W292)
cortex/approval.py:26:23: W292 No newline at end of file
cortex/approval_policy.py
[failure] 75-75: Ruff (W292)
cortex/approval_policy.py:75:59: W292 No newline at end of file
[failure] 1-4: Ruff (I001)
cortex/approval_policy.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/cli.py
[failure] 1-28: Ruff (I001)
cortex/cli.py:1:1: I001 Import block is un-sorted or un-formatted
cortex/coordinator.py
[failure] 1-15: Ruff (I001)
cortex/coordinator.py:1:1: I001 Import block is un-sorted or un-formatted
[failure] 88-88: Ruff (W293)
cortex/coordinator.py:88:1: W293 Blank line contains whitespace
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (5)
cortex/coordinator.py (2)
240-244: Consider whether rollback should also require confirmation in AUTO_EDIT mode.Rollback executes shell commands but doesn't check
requires_confirmation(). InAUTO_EDITmode, this means rollback commands run without user approval while regular execution requires confirmation.If this is intentional (rollback is a recovery operation), consider adding a comment explaining the design decision.
318-321: LGTM - verification correctly respects approval policy.Blocking verification commands in
SUGGESTmode is consistent with the policy design, since verification still executes shell commands.cortex/cli.py (3)
984-1006: LGTM -config()method follows existing CLI patterns.The error handling and subcommand routing is consistent with other CLI methods like
env().
1674-1684: LGTM - config subparser follows existing CLI patterns.The argument structure is clear and matches the documented CLI usage:
cortex config set approval-mode <mode>.
1965-1966: LGTM - command routing is consistent.
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
de256da to
e0af257
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
cortex/cli.py (1)
9-10: Import order appears resolved.A previous review flagged unsorted imports (Ruff I001). The current imports appear to be properly alphabetized within their group (cortex.approval, cortex.approval_policy, ... cortex.user_preferences). If the linter still reports I001, verify that all imports between lines 9-28 are sorted correctly.
#!/bin/bash # Verify import sorting in cortex/cli.py rg -n "^from cortex\." cortex/cli.py | head -20Also applies to: 27-27
cortex/coordinator.py (2)
12-15: Import order appears resolved.A previous review flagged unsorted imports (Ruff I001). The current imports appear properly alphabetized (approval_policy, confirm, user_preferences, validators). If the linter still reports I001, there may be issues with the broader import block structure.
#!/bin/bash # Verify all imports in cortex/coordinator.py are properly sorted rg -n "^(import|from)" cortex/coordinator.py | head -20
85-88: Remove trailing whitespace on blank line (W293).This issue was flagged in a previous review and remains unresolved. Line 88 contains trailing whitespace, causing a linter failure.
🔎 Proposed fix
# 🔐 Load approval policy once prefs = UserPreferences.load() self.approval_policy = get_approval_policy(prefs.approval_mode) - +cortex/approval.py (1)
14-24: Missing newline at end of file (W292).This issue was flagged in a previous review and remains unresolved. The file should end with a blank line per PEP 8 and the W292 linter rule.
🔎 Proposed fix
raise ValueError( f"Invalid approval mode '{value}'. Valid modes are: {valid}" ) from exc +docs/approval_modes.md (2)
33-33: Capitalize the sentence.This grammatical issue was flagged in a previous review and remains unresolved. The sentence should start with a capital letter.
🔎 Proposed fix
-commands are executed only when --execute flag is provided +Commands are executed only when --execute flag is provided
36-36: Fix invalid bash syntax in the example.This issue was flagged in a previous review and remains unresolved. The example has two problems: a double space after "set" and a comma incorrectly joining two commands.
🔎 Proposed fix
Separate the commands properly:
-cortex config set approval-mode full-auto,cortex install pandas --execute +cortex config set approval-mode full-auto +cortex install pandas --executeOr as a single line with proper shell operator:
-cortex config set approval-mode full-auto,cortex install pandas --execute +cortex config set approval-mode full-auto && cortex install pandas --executecortex/user_preferences.py (3)
22-42: Uncaught ValueError breaks fail-safe loading.The
load()method catchesjson.JSONDecodeErrorandOSErrorbut notValueError. If the JSON contains an invalidapproval_modevalue (e.g.,"invalid"),ApprovalMode.from_string()raisesValueErrorwhich propagates uncaught, breaking the fail-safe pattern.Additionally, the return type annotation on line 23 uses unnecessary quotes.
🔎 Proposed fix
@classmethod - def load(cls, path: Path = DEFAULT_CONFIG_PATH) -> "UserPreferences": + def load(cls, path: Path = DEFAULT_CONFIG_PATH) -> UserPreferences: """ Load user preferences from disk. Falls back to defaults if config does not exist. """ if not path.exists(): return cls() try: with path.open("r", encoding="utf-8") as f: data: dict[str, Any] = json.load(f) - except (json.JSONDecodeError, OSError): + approval_mode = ApprovalMode.from_string( + data.get("approval_mode", ApprovalMode.SUGGEST.value) + ) + except (json.JSONDecodeError, OSError, ValueError): # Corrupt config → fail safe return cls() - return cls( - approval_mode=ApprovalMode.from_string( - data.get("approval_mode", ApprovalMode.SUGGEST.value) - ) - ) + return cls(approval_mode=approval_mode)
44-55: Add missing newline at end of file.The file is missing a trailing newline after line 55, which is required by PEP 8.
🔎 Proposed fix
with path.open("w", encoding="utf-8") as f: json.dump(data, f, indent=2) +
1-8: CRITICAL: Fix import error blocking pipeline.Line 6 attempts to import
dictfromtyping, which is invalid and causes pipeline failures. The built-indicttype does not need to be imported. Additionally,asdicton line 4 is imported but never used.🔎 Proposed fix
from __future__ import annotations import json -from dataclasses import asdict, dataclass +from dataclasses import dataclass from pathlib import Path -from typing import Any, dict +from typing import Any from cortex.approval import ApprovalModecortex/approval_policy.py (2)
1-4: Verify import order matches linter expectations.The linter flags the import block as unsorted (I001), though the imports appear to follow PEP 8 conventions: standard library imports (
abc,typing) are alphabetically sorted and separated from local imports (cortex.approval) by a blank line.Run your linter/formatter to ensure the imports match the project's specific style configuration.
63-74: Add missing newline at end of file.The file is missing a trailing newline after line 74, as flagged by the linter.
🔎 Proposed fix
raise ValueError(f"Unsupported approval mode: {mode}") +
🧹 Nitpick comments (2)
tests/test_installation_coordinator_approval.py (1)
6-22: Test logic is correct; consider cleaner mocking approach.The test validates that SUGGEST mode blocks execution as expected. The monkeypatch works but the anonymous class creation via
type()is somewhat opaque.🔎 Optional: More explicit mock
For improved readability, consider using a proper UserPreferences instance or a dedicated test fixture:
def test_suggest_mode_blocks_execution(monkeypatch): # Create a real UserPreferences object with SUGGEST mode mock_prefs = UserPreferences() mock_prefs.approval_mode = ApprovalMode.SUGGEST monkeypatch.setattr(UserPreferences, "load", lambda path=None: mock_prefs) coordinator = InstallationCoordinator( commands=["echo hello"], descriptions=["test command"], ) result = coordinator.execute() assert not result.success assert result.steps[0].status.value == "skipped"cortex/approval_policy.py (1)
63-73: Consider usingeliffor slightly better clarity.The factory uses three separate
ifstatements. While functionally correct, usingelifwould make the mutual exclusivity more explicit and avoid redundant checks after a match.🔎 Proposed refactor
def get_approval_policy(mode: ApprovalMode) -> ApprovalPolicy: """ Factory method to get the correct approval policy. """ if mode == ApprovalMode.SUGGEST: return SuggestPolicy(mode) - if mode == ApprovalMode.AUTO_EDIT: + elif mode == ApprovalMode.AUTO_EDIT: return AutoEditPolicy(mode) - if mode == ApprovalMode.FULL_AUTO: + elif mode == ApprovalMode.FULL_AUTO: return FullAutoPolicy(mode) - - raise ValueError(f"Unsupported approval mode: {mode}") + else: + raise ValueError(f"Unsupported approval mode: {mode}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cortex/approval.pycortex/approval_policy.pycortex/cli.pycortex/confirm.pycortex/coordinator.pycortex/user_preferences.pydocs/approval_modes.mdtests/test_approval_policy.pytests/test_installation_coordinator_approval.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/confirm.py
- tests/test_approval_policy.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/coordinator.pycortex/approval.pytests/test_installation_coordinator_approval.pycortex/user_preferences.pycortex/approval_policy.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_installation_coordinator_approval.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
tests/test_installation_coordinator_approval.py
🧬 Code graph analysis (5)
cortex/coordinator.py (3)
cortex/approval_policy.py (9)
get_approval_policy(63-74)allow(23-25)allow(37-38)allow(45-46)allow(53-54)requires_confirmation(28-30)requires_confirmation(40-41)requires_confirmation(48-49)requires_confirmation(56-57)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (2)
UserPreferences(15-55)load(23-42)
tests/test_installation_coordinator_approval.py (3)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/coordinator.py (2)
InstallationCoordinator(54-357)execute(259-308)cortex/user_preferences.py (1)
UserPreferences(15-55)
cortex/user_preferences.py (1)
cortex/approval.py (2)
ApprovalMode(4-24)from_string(15-24)
cortex/approval_policy.py (1)
cortex/approval.py (1)
ApprovalMode(4-24)
cortex/cli.py (3)
cortex/approval.py (2)
ApprovalMode(4-24)from_string(15-24)cortex/approval_policy.py (1)
get_approval_policy(63-74)cortex/user_preferences.py (3)
UserPreferences(15-55)load(23-42)save(44-55)
🪛 GitHub Actions: CI
cortex/user_preferences.py
[error] 6-6: ImportError: cannot import name 'dict' from 'typing'. Use 'dict' (builtin) or 'typing.Dict' instead of importing 'dict' from typing.
🪛 GitHub Actions: Cortex Automation
cortex/user_preferences.py
[error] 6-6: ImportError: cannot import name 'dict' from 'typing'. Did you mean: 'Dict'?
🔇 Additional comments (12)
cortex/approval.py (1)
1-12: LGTM: Clean enum definition with proper string mixin.The ApprovalMode enum is well-structured with the str mixin pattern, enabling direct string comparison and JSON serialization. The three modes (SUGGEST, AUTO_EDIT, FULL_AUTO) align with the PR objectives.
cortex/cli.py (5)
42-43: LGTM: Clean approval policy integration.The initialization properly loads user preferences and derives the approval policy, consistent with the pattern in cortex/coordinator.py.
984-1007: LGTM: Well-structured config command handler.The method follows established patterns in the CLI, with proper error handling and routing to the _config_set implementation.
1009-1021: LGTM: Correct implementation with proper validation.The method correctly validates the config key, leverages
ApprovalMode.from_string()for type-safe parsing (which raises ValueError for invalid modes), and persists the change. The restriction to "approval-mode" is appropriate for the initial implementation.
1671-1677: LGTM: Clean CLI argument setup.The config command parser follows the established pattern for subcommands with proper argument definitions.
1958-1959: LGTM: Proper command routing.The routing integrates the config command into the main dispatcher correctly.
cortex/coordinator.py (4)
85-87: LGTM: Clean approval policy initialization.The coordinator correctly loads user preferences once during initialization and derives the appropriate approval policy. This centralized approach ensures consistent policy enforcement across all operations.
173-185: LGTM: Correct approval policy enforcement.The approval checks are properly implemented:
- Commands are blocked if
allow("shell_command")returns False (SUGGEST mode)- User confirmation is requested when
requires_confirmation("shell_command")is True (AUTO_EDIT mode)- Commands proceed without prompts in FULL_AUTO mode
The step status is correctly set to SKIPPED with descriptive error messages, ensuring the failure reason is transparent in results.
239-242: LGTM: Consistent policy enforcement in rollback.The rollback flow correctly checks the approval policy before executing rollback commands. Note that rollback only checks
allow()without confirmation prompts, which is appropriate since rollback is triggered automatically on installation failure rather than being user-initiated.
317-320: LGTM: Proper approval gating for verification commands.The verification flow correctly checks the approval policy and marks blocked verifications as failed while continuing to process remaining verification commands. This provides complete feedback on which verifications were blocked versus which failed for other reasons.
cortex/approval_policy.py (2)
6-30: LGTM: Clean abstraction for approval policies.The
ActionTypealias andApprovalPolicyABC provide a well-structured foundation for the approval system. The abstract methods clearly define the contract for concrete policies.
36-57: LGTM: Policy implementations correctly enforce tiered approval.The three concrete policies accurately implement the PR objectives:
SuggestPolicy: read-only (display proposed actions)AutoEditPolicy: applies file edits automatically, gates shell commands with confirmationFullAutoPolicy: executes all actions without prompts
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
e0af257 to
d84efae
Compare
CLA Verification PassedAll contributors have signed the CLA.
|
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
1436ab2 to
ef01140
Compare
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
cdf055b to
311590c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cortex/coordinator.py (1)
12-16: LGTM on the new imports.The imports for the approval system are correctly added and enable the new tiered approval functionality.
Note: A past review flagged import ordering (Ruff I001). Verify this has been addressed in the current version.
cortex/approval.py (1)
1-24: LGTM on the ApprovalMode enum implementation.Clean design using
str, Enuminheritance for JSON serialization compatibility. Thefrom_stringclassmethod provides helpful error messages listing valid modes.Note: A past review flagged missing newline at end of file (W292). Verify this has been addressed.
cortex/approval_policy.py (1)
1-10: LGTM on type definitions.
ActionTypeas aLiteralunion provides good type safety. The three action types align with the PR objectives.Note: A past review flagged import sorting (I001) and missing EOF newline. Verify these have been addressed.
🧹 Nitpick comments (4)
cortex/coordinator.py (2)
67-67: Add type hint forapproval_policyparameter.Per coding guidelines, type hints are required in Python code. The parameter lacks a type annotation.
- approval_policy=None, + approval_policy: "ApprovalPolicy | None" = None,You'll need to add
from __future__ import annotationsat the top or use a string literal for the forward reference, and importApprovalPolicyfromcortex.approval_policy.
185-190: Confirmation prompts on every step in auto-edit mode.In
auto-editmode,requires_confirmation("shell_command")returnsTrue, so the user will be prompted for each step. For a 10-step installation, this means 10 prompts.Consider either:
- Prompting once before the execution loop starts (in
execute())- Including the specific command in the prompt for clarity:
f"Execute: {step.command}?"The current generic message "Execute planned shell commands?" is also misleading since it prompts per-command.
cortex/cli.py (1)
1020-1032: Add docstring to_config_setmethod.Per coding guidelines, docstrings are required for all public APIs. While this is a private method, it's a helper that would benefit from documentation.
def _config_set(self, args: argparse.Namespace) -> int: + """Set a configuration value (currently supports 'approval-mode').""" key = args.keycortex/approval_policy.py (1)
63-74: Factory function is correct but has unreachable code path.The
ValueErrorat line 74 is unreachable whenmodeis a validApprovalModeenum (since all three values are handled). This is defensive programming which is fine, but you could simplify with a dict mapping:_POLICY_MAP = { ApprovalMode.SUGGEST: SuggestPolicy, ApprovalMode.AUTO_EDIT: AutoEditPolicy, ApprovalMode.FULL_AUTO: FullAutoPolicy, } def get_approval_policy(mode: ApprovalMode) -> ApprovalPolicy: policy_cls = _POLICY_MAP.get(mode) if policy_cls is None: raise ValueError(f"Unsupported approval mode: {mode}") return policy_cls(mode)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cortex/approval.pycortex/approval_policy.pycortex/cli.pycortex/confirm.pycortex/coordinator.pycortex/user_preferences.pydocs/approval_modes.mdtests/test_approval_policy.pytests/test_installation_coordinator_approval.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/approval_modes.md
- cortex/confirm.py
- tests/test_approval_policy.py
- tests/test_installation_coordinator_approval.py
- cortex/user_preferences.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/approval.pycortex/coordinator.pycortex/approval_policy.py
🧬 Code graph analysis (3)
cortex/cli.py (4)
cortex/user_preferences.py (3)
UserPreferences(15-55)load(23-42)save(44-55)cortex/approval_policy.py (1)
get_approval_policy(63-74)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/approval.py (2)
ApprovalMode(4-24)from_string(15-24)
cortex/coordinator.py (4)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/approval_policy.py (9)
get_approval_policy(63-74)allow(23-25)allow(37-38)allow(45-46)allow(53-54)requires_confirmation(28-30)requires_confirmation(40-41)requires_confirmation(48-49)requires_confirmation(56-57)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (1)
UserPreferences(15-55)
cortex/approval_policy.py (1)
cortex/approval.py (1)
ApprovalMode(4-24)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 42-42: Undefined name 'get_approval_policy' (F821) detected by ruff during 'ruff check . --output-format=github'.
🪛 GitHub Actions: Cortex Automation
cortex/cli.py
[error] 42-42: F821 Undefined name 'get_approval_policy'.
🪛 GitHub Check: lint
cortex/cli.py
[failure] 42-42: Ruff (F821)
cortex/cli.py:42:32: F821 Undefined name get_approval_policy
[failure] 1028-1028: Ruff (F821)
cortex/cli.py:1028:31: F821 Undefined name ApprovalMode
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 42-42: Ruff (F821)
cortex/cli.py:42:32: F821 Undefined name get_approval_policy
[failure] 1028-1028: Ruff (F821)
cortex/cli.py:1028:31: F821 Undefined name ApprovalMode
🔇 Additional comments (6)
cortex/coordinator.py (2)
244-256: Rollback skips confirmation prompt in auto-edit mode.Unlike
_execute_command, rollback only checksallow()but notrequires_confirmation(). This means inauto-editmode, rollback commands execute without prompting.This may be intentional (rollback is recovery from failure), but verify this is the desired behavior. If rollback should also require confirmation in
auto-editmode, add:if self.approval_policy.requires_confirmation("shell_command"): if not confirm_action(f"Execute rollback: {cmd}?"): self._log("Rollback declined by user") continue
322-325: Verification commands skip confirmation in auto-edit mode.Verification bypasses the
requires_confirmationcheck. This is reasonable since verification commands are typically read-only diagnostics (e.g.,docker --version), but worth noting for consistency documentation.cortex/cli.py (2)
1682-1688: LGTM on config subparser setup.The argument parser correctly wires up
cortex config set <key> <value>as specified in the PR objectives.
1969-1970: LGTM on config command dispatch.Correctly routes the
configcommand to the handler.cortex/approval_policy.py (2)
13-30: LGTM on the abstract base class.Clean design with clear contracts for
allow()andrequires_confirmation().
36-57: LGTM on policy implementations.The three policies correctly implement the tiered approval modes per the PR objectives:
suggest: display only, no executionauto-edit: allow with user confirmation for shell commandsfull-auto: execute automatically
| prefs = UserPreferences.load() | ||
| self.approval_policy = get_approval_policy(prefs.approval_mode) |
There was a problem hiding this comment.
self.approval_policy appears unused in CortexCLI.
The approval policy is loaded and stored, but the install() method creates a new InstallationCoordinator without passing this policy (line 734). The coordinator then defaults to FULL_AUTO, ignoring the user's configured preference.
Either:
- Pass the policy to the coordinator:
InstallationCoordinator(..., approval_policy=self.approval_policy) - Remove the unused attribute if not intended
coordinator = InstallationCoordinator(
commands=commands,
descriptions=[f"Step {i + 1}" for i in range(len(commands))],
timeout=300,
stop_on_error=True,
progress_callback=progress_callback,
+ approval_policy=self.approval_policy,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prefs = UserPreferences.load() | |
| self.approval_policy = get_approval_policy(prefs.approval_mode) | |
| coordinator = InstallationCoordinator( | |
| commands=commands, | |
| descriptions=[f"Step {i + 1}" for i in range(len(commands))], | |
| timeout=300, | |
| stop_on_error=True, | |
| progress_callback=progress_callback, | |
| approval_policy=self.approval_policy, | |
| ) |
🧰 Tools
🪛 GitHub Actions: CI
[error] 42-42: Undefined name 'get_approval_policy' (F821) detected by ruff during 'ruff check . --output-format=github'.
🪛 GitHub Actions: Cortex Automation
[error] 42-42: F821 Undefined name 'get_approval_policy'.
🪛 GitHub Check: lint
[failure] 42-42: Ruff (F821)
cortex/cli.py:42:32: F821 Undefined name get_approval_policy
🪛 GitHub Check: Lint
[failure] 42-42: Ruff (F821)
cortex/cli.py:42:32: F821 Undefined name get_approval_policy
🤖 Prompt for AI Agents
In cortex/cli.py around lines 41-42, self.approval_policy is set but never used;
update the install() method (around the InstallationCoordinator instantiation at
~line 734) to pass the loaded policy into the coordinator by adding
approval_policy=self.approval_policy to its constructor call so the user's
preference is respected, or if the attribute is unnecessary remove the
assignment and any unused references to self.approval_policy.
33628c0 to
467e77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cortex/confirm.py (1)
1-6: Duplicate: Add docstring and fix missing newline at EOF.This issue was already flagged in a previous review. The function is missing a docstring (required per coding guidelines for all public APIs) and the file is missing a trailing newline.
cortex/approval_policy.py (1)
1-4: Duplicate: Fix import sorting and add missing newline at EOF.This issue was already flagged in a previous review. The import block needs alphabetical sorting and the file needs a trailing newline at the end.
cortex/cli.py (1)
43-44: Duplicate:self.approval_policyis loaded but never used.This issue was already flagged in a previous review. The approval policy is loaded from user preferences but never passed to
InstallationCoordinatorin theinstall()method (around line 736). As a result, the coordinator defaults toFULL_AUTOmode, ignoring the user's configured preference.
🧹 Nitpick comments (1)
cortex/coordinator.py (1)
67-67: Consider documenting the default FULL_AUTO policy behavior.The coordinator defaults to
FULL_AUTOwhen noapproval_policyis passed. While this provides backward compatibility, it means callers must explicitly pass a policy to enforce restrictions. This default contributed to the test failure intest_installation_coordinator_approval.py.Consider either:
- Adding a parameter docstring documenting this default behavior
- Making the parameter required to force explicit policy decisions
- Loading user preferences by default instead of hardcoding FULL_AUTO
🔎 Suggested enhancement
def __init__( self, commands: list[str], descriptions: list[str] | None = None, timeout: int = 300, stop_on_error: bool = True, enable_rollback: bool = False, log_file: str | None = None, progress_callback: Callable[[int, int, InstallationStep], None] | None = None, approval_policy=None, ): - """Initialize an installation run with optional logging and rollback.""" + """Initialize an installation run with optional logging and rollback. + + Args: + approval_policy: Approval policy to enforce. If None, defaults to FULL_AUTO + for backward compatibility. Pass an explicit policy to enforce restrictions. + """Also applies to: 87-92
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cortex/approval.pycortex/approval_policy.pycortex/cli.pycortex/confirm.pycortex/coordinator.pycortex/user_preferences.pydocs/approval_modes.mdtests/test_approval_policy.pytests/test_installation_coordinator_approval.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_approval_policy.py
- cortex/user_preferences.py
- docs/approval_modes.md
- cortex/approval.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_installation_coordinator_approval.pycortex/cli.pycortex/coordinator.pycortex/confirm.pycortex/approval_policy.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_installation_coordinator_approval.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
tests/test_installation_coordinator_approval.py
🧬 Code graph analysis (3)
cortex/cli.py (4)
cortex/approval.py (2)
ApprovalMode(4-24)from_string(15-24)cortex/approval_policy.py (1)
get_approval_policy(63-74)cortex/user_preferences.py (3)
UserPreferences(15-55)load(23-42)save(44-55)cortex/first_run_wizard.py (1)
_print_error(746-748)
cortex/coordinator.py (4)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/approval_policy.py (9)
get_approval_policy(63-74)allow(23-25)allow(37-38)allow(45-46)allow(53-54)requires_confirmation(28-30)requires_confirmation(40-41)requires_confirmation(48-49)requires_confirmation(56-57)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (1)
UserPreferences(15-55)
cortex/approval_policy.py (1)
cortex/approval.py (1)
ApprovalMode(4-24)
🪛 GitHub Actions: CI
tests/test_installation_coordinator_approval.py
[error] 21-21: Test failed: test_suggest_mode_blocks_execution expected result.success to be False, but got True; InstallationResult reports success=True with a single echo step.
⏰ 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: security
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (8)
cortex/cli.py (2)
997-1034: LGTM! Config command implementation is correct.The
configcommand implementation follows established patterns:
- Proper subcommand routing and error handling
- Validates configuration keys before applying
- Uses
UserPreferences.load()andsave()for persistence- Uses
ApprovalMode.from_string()for validation- Provides clear user feedback
1684-1690: LGTM! CLI parser and dispatcher wiring is correct.The CLI argument parser setup and command dispatcher routing follow the established patterns in the codebase.
Also applies to: 1971-1972
cortex/coordinator.py (3)
179-195: LGTM! Approval checks are correctly implemented.The approval policy enforcement in
_execute_commandis correct:
- Checks
allow("shell_command")before execution- Prompts for confirmation when
requires_confirmation("shell_command")is True- Sets status to
FAILED(not SKIPPED) when blocked, which is appropriate- Provides clear error messages for debugging
249-252: LGTM! Rollback approval check is correct.The approval policy check in
_rollbackcorrectly prevents rollback commands from executing when the policy disallows shell commands.
327-330: LGTM! Verification approval check is correct.The approval policy check in
verify_installationcorrectly blocks verification commands when the policy disallows shell commands and records them as failed.cortex/approval_policy.py (3)
13-30: LGTM! ApprovalPolicy base class design is clean.The abstract base class provides a clear contract for policy implementations with well-defined methods for permission checking and confirmation requirements.
36-57: LGTM! Concrete policy implementations correctly enforce approval modes.The three policy classes correctly implement the tiered approval modes:
SuggestPolicy: Read-only, allows only "show" actionsAutoEditPolicy: Allows file edits and shell commands, but requires confirmation for shell commandsFullAutoPolicy: No restrictions, allows all actions without confirmationThe implementations align with the PR objectives.
63-74: LGTM! Factory function correctly instantiates policies.The
get_approval_policyfactory correctly maps eachApprovalModeto its corresponding policy implementation and provides appropriate error handling for unsupported modes.
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
7 times, most recently
from
8e4ab8a to
cee70ad
Compare
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
cee70ad to
7a9f249
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cortex/cli.py (1)
43-44:self.approval_policyis loaded but never used.The approval policy is loaded from user preferences and stored as an instance attribute, but it's never passed to
InstallationCoordinatorinstances created ininstall()(line 736),_execute_install()(line 1523), or_execute_multi_install()(line 1557). The coordinator defaults toFULL_AUTOwhen no policy is provided, ignoring the user's configured preference.Either pass the policy to the coordinator or remove this unused attribute.
🔎 Proposed fix for install() method (around line 736)
coordinator = InstallationCoordinator( commands=commands, descriptions=[f"Step {i + 1}" for i in range(len(commands))], timeout=300, stop_on_error=True, progress_callback=progress_callback, + approval_policy=self.approval_policy, )
🧹 Nitpick comments (5)
cortex/approval_policy.py (2)
36-57: Add docstrings to concrete policy classes.Per coding guidelines, docstrings are required for all public APIs. The concrete policy classes (
SuggestPolicy,AutoEditPolicy,FullAutoPolicy) lack class-level docstrings explaining their behavior.🔎 Proposed fix
class SuggestPolicy(ApprovalPolicy): + """Policy for suggest mode: only show actions are allowed, no execution.""" + def allow(self, action: ActionType) -> bool: return action == "show" def requires_confirmation(self, action: ActionType) -> bool: return False class AutoEditPolicy(ApprovalPolicy): + """Policy for auto-edit mode: allows all actions but requires confirmation for shell commands.""" + def allow(self, action: ActionType) -> bool: return action in {"show", "file_edit", "shell_command"} def requires_confirmation(self, action: ActionType) -> bool: return action == "shell_command" class FullAutoPolicy(ApprovalPolicy): + """Policy for full-auto mode: allows all actions without confirmation.""" + def allow(self, action: ActionType) -> bool: return True def requires_confirmation(self, action: ActionType) -> bool: return False
63-74: Add type hint to factory function parameter and ensure trailing newline.The
modeparameter has a type hint, but the file appears to be missing a trailing newline after line 74 (theValueError). Ensure the file ends with a single newline character.🔎 Proposed fix
raise ValueError(f"Unsupported approval mode: {mode}") +cortex/coordinator.py (3)
67-67: Add type hint forapproval_policyparameter.The parameter lacks a type hint. Consider adding
ApprovalPolicy | None = Nonefor clarity and IDE support.🔎 Proposed fix
+from cortex.approval_policy import ApprovalPolicy, get_approval_policy -from cortex.approval_policy import get_approval_policy ... progress_callback: Callable[[int, int, InstallationStep], None] | None = None, - approval_policy=None, + approval_policy: ApprovalPolicy | None = None, ):
87-100: Consider logging when falling back to default policy.The broad
except Exceptionsilently swallows any errors during preferences loading. While the fallback toFULL_AUTOis reasonable, consider logging a warning so operators can diagnose configuration issues.🔎 Proposed fix
try: prefs = UserPreferences.load() if hasattr(prefs, "approval_mode"): approval_mode = prefs.approval_mode except Exception: - pass + logger.warning("Failed to load user preferences, defaulting to FULL_AUTO mode") self.approval_policy = get_approval_policy(approval_mode)
320-320: Misleading comment: suggest mode behavior is handled elsewhere.The comment "Suggest mode should never report success" is not accurate for this location. The actual suggest mode handling happens in
_execute_command()(lines 189-194), not here. This comment may confuse future maintainers.🔎 Proposed fix
- # ❗ Suggest mode should never report success + # Check if all steps completed successfully all_success = all(s.status == StepStatus.SUCCESS for s in self.steps)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cortex/approval.pycortex/approval_policy.pycortex/cli.pycortex/confirm.pycortex/coordinator.pycortex/user_preferences.pydocs/approval_modes.mdtests/test_approval_policy.pytests/test_installation_coordinator_approval.py
✅ Files skipped from review due to trivial changes (1)
- docs/approval_modes.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_approval_policy.py
- cortex/approval.py
- cortex/user_preferences.py
- tests/test_installation_coordinator_approval.py
- cortex/confirm.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/approval_policy.pycortex/coordinator.py
🧬 Code graph analysis (3)
cortex/cli.py (4)
cortex/approval.py (2)
ApprovalMode(4-24)from_string(15-24)cortex/approval_policy.py (1)
get_approval_policy(63-74)cortex/user_preferences.py (3)
UserPreferences(15-55)load(23-42)save(44-55)cortex/first_run_wizard.py (1)
_print_error(746-748)
cortex/approval_policy.py (1)
cortex/approval.py (1)
ApprovalMode(4-24)
cortex/coordinator.py (4)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/approval_policy.py (1)
get_approval_policy(63-74)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (2)
UserPreferences(15-55)load(23-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (11)
cortex/approval_policy.py (2)
1-10: LGTM on imports and type alias.The import structure is clean, and
ActionTypeas aLiteraltype alias provides clear, self-documenting allowed action values.
13-30: LGTM on abstract base class design.The
ApprovalPolicyABC provides a clean contract withallow()andrequires_confirmation()methods. Type hints and docstrings are present. Storing themodein__init__enables policy introspection later.cortex/cli.py (5)
10-11: LGTM on imports.The new imports for
ApprovalModeandget_approval_policyare correctly placed and sorted.
997-1020: LGTM onconfig()method structure.The method follows the existing pattern used by
env()andnotify()for subcommand routing. Error handling is consistent with the rest of the CLI.
1022-1034: LGTM on_config_set()implementation.The method correctly:
- Validates the config key
- Uses
ApprovalMode.from_string()for safe parsing (which raisesValueErrorfor invalid modes)- Persists preferences via
prefs.save()- Provides user feedback
The
ValueErrorraised for unknown keys is properly caught by the parentconfig()method.
1684-1690: LGTM on CLI parser definition.The
configsubparser andsetsubcommand are correctly defined withkeyandvaluepositional arguments.
1971-1972: LGTM on command routing.The
configcommand is correctly routed tocli.config(args).cortex/coordinator.py (4)
12-15: LGTM on imports.The imports are correctly sorted alphabetically within the local import group.
196-202: Approval check forshell_commandmay be redundant in suggest mode.Lines 189-194 already return early if
self._explicit_policyand mode isSUGGEST. However,SuggestPolicy.allow("shell_command")returnsFalse, so the check on line 197 would also block execution. This is fine for defense-in-depth, but the flow could be clearer.
266-278: LGTM on rollback approval check.The rollback flow correctly checks approval before executing rollback commands. The early return prevents partial rollbacks if the policy disallows shell execution.
345-348: LGTM on verification approval check.The verification flow correctly checks approval before executing verification commands and logs when blocked.
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
2 times, most recently
from
a84e0e8 to
c551690
Compare
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
3 times, most recently
from
7f7b9a7 to
1d1a3fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/coordinator.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/coordinator.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/coordinator.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
cortex/coordinator.py
🧬 Code graph analysis (1)
cortex/coordinator.py (4)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/approval_policy.py (9)
get_approval_policy(63-74)allow(23-25)allow(37-38)allow(45-46)allow(53-54)requires_confirmation(28-30)requires_confirmation(40-41)requires_confirmation(48-49)requires_confirmation(56-57)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (2)
UserPreferences(15-55)load(23-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
| self._explicit_policy = approval_policy is not None | ||
| # 🔐 Load approval policy once | ||
| if approval_policy is not None: | ||
| self.approval_policy = approval_policy | ||
| else: | ||
| approval_mode = ApprovalMode.FULL_AUTO | ||
| try: | ||
| prefs = UserPreferences.load() | ||
| if hasattr(prefs, "approval_mode"): | ||
| approval_mode = prefs.approval_mode | ||
| except Exception: | ||
| pass | ||
|
|
||
| self.approval_policy = get_approval_policy(approval_mode) |
There was a problem hiding this comment.
Unsafe default: FULL_AUTO contradicts safe-by-default principles.
Lines 92-98 default to ApprovalMode.FULL_AUTO when preference loading fails. This is the most permissive mode and contradicts the retrieved learnings that specify "Dry-run by default for all installations" and "No silent sudo execution - require explicit user confirmation."
Additionally, the broad except Exception at line 97 silently swallows all errors (including file system issues, JSON decode errors, etc.) and falls back to the most dangerous mode.
Based on learnings, the safe default should be SUGGEST mode (most restrictive), not FULL_AUTO.
🔎 Proposed fix to default to SUGGEST mode
self._explicit_policy = approval_policy is not None
# 🔐 Load approval policy once
if approval_policy is not None:
self.approval_policy = approval_policy
else:
- approval_mode = ApprovalMode.FULL_AUTO
+ approval_mode = ApprovalMode.SUGGEST
try:
prefs = UserPreferences.load()
if hasattr(prefs, "approval_mode"):
approval_mode = prefs.approval_mode
except Exception:
+ # Fail safe: default to most restrictive mode
pass
self.approval_policy = get_approval_policy(approval_mode)🤖 Prompt for AI Agents
In cortex/coordinator.py around lines 87 to 100, change the unsafe fallback and
broad exception handling: set the initial fallback approval_mode to
ApprovalMode.SUGGEST instead of FULL_AUTO, and replace the blanket "except
Exception:" with handling only expected errors from loading preferences (e.g.,
FileNotFoundError, JSONDecodeError, ValueError) so other unexpected errors
surface; on preference-load failure for those expected cases, silently fall back
to SUGGEST (or optionally log the load error), and for unexpected exceptions let
them propagate or be logged explicitly before falling back.
| ] | ||
|
|
||
| self.rollback_commands: list[str] = [] | ||
| self._explicit_policy = approval_policy is not None |
There was a problem hiding this comment.
Design flaw: _explicit_policy flag undermines the approval system.
The _explicit_policy flag (line 87) creates a two-tier system where approval checks are only enforced when a policy is explicitly passed to the constructor. This design has several problems:
-
Breaks primary use case: The PR objectives specify CLI/config-based approval control. The flag bypasses approval for config-based usage, breaking the main feature.
-
No backward compatibility needed: This is a new feature, not a modification of existing behavior. There's no legacy behavior to preserve.
-
Security by obscurity: Developers must know to explicitly pass a policy to get security guarantees, rather than security being the default.
-
Inconsistent application: The flag is checked in
_execute_commandbut not in_rollbackorverify_installation, creating unpredictable behavior.
Recommendation: Remove the _explicit_policy flag entirely and always enforce the loaded approval policy. If programmatic bypass is truly needed for specific scenarios (e.g., internal testing), that should be achieved through a dedicated bypass mode or factory method, not by silently disabling security checks.
🔎 Proposed fix to remove _explicit_policy flag
self.rollback_commands: list[str] = []
- self._explicit_policy = approval_policy is not None
# 🔐 Load approval policy once
if approval_policy is not None:
self.approval_policy = approval_policy
else:
- approval_mode = ApprovalMode.FULL_AUTO
+ approval_mode = ApprovalMode.SUGGEST
try:
prefs = UserPreferences.load()
if hasattr(prefs, "approval_mode"):
approval_mode = prefs.approval_mode
except Exception:
pass
self.approval_policy = get_approval_policy(approval_mode)Then remove all references to self._explicit_policy throughout the file (as shown in previous fix suggestions).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/coordinator.py around line 87, the _explicit_policy flag undermines
approval enforcement; remove the flag and all logic that conditions security
checks on it so the loaded approval policy is always enforced. Delete the
attribute assignment, remove all references/branches that check
self._explicit_policy in methods like _execute_command, _rollback,
verify_installation (and any other methods), and ensure approval checks call the
policy unconditionally; if a programmatic bypass is required later, implement a
distinct explicit bypass mode or factory method instead of a silent flag. Update
or add unit tests to reflect the always-on policy behavior and adjust
constructor/initialization docs accordingly.
| def _execute_command(self, step: InstallationStep) -> bool: | ||
| step.status = StepStatus.RUNNING | ||
| step.start_time = time.time() | ||
| from cortex.approval import ApprovalMode |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant import.
ApprovalMode is already imported at line 12. This local import is unnecessary.
🔎 Proposed fix
def _execute_command(self, step: InstallationStep) -> bool:
- from cortex.approval import ApprovalMode
-
step.start_time = time.time()🤖 Prompt for AI Agents
In cortex/coordinator.py around line 182, remove the redundant local import
"from cortex.approval import ApprovalMode" (it's already imported at line 12);
delete that single-line import and ensure imports remain organized (run
linters/tests to confirm no remaining references depend on the local import).
cortex/coordinator.py
Outdated
| if self._explicit_policy and self.approval_policy.mode == ApprovalMode.SUGGEST: | ||
| step.status = StepStatus.SKIPPED | ||
| step.error = "Execution skipped in suggest mode" | ||
| step.end_time = time.time() | ||
| self._log("Suggest mode: execution skipped") | ||
| return True # Not a failure | ||
|
|
||
| # 🔐 Enforce approval ONLY when explicitly passed | ||
| if self._explicit_policy: | ||
| if not self.approval_policy.allow("shell_command"): | ||
| step.status = StepStatus.FAILED | ||
| step.error = "Shell execution disabled by approval policy" | ||
| step.end_time = time.time() | ||
| self._log(f"Execution blocked by approval policy: {step.command}") | ||
| return False | ||
|
|
||
| if self.approval_policy.requires_confirmation("shell_command"): | ||
| if not confirm_action("Execute planned shell commands?"): | ||
| step.status = StepStatus.FAILED | ||
| step.error = "User declined execution" | ||
| step.end_time = time.time() | ||
| self._log("Execution declined by user") | ||
| return False |
There was a problem hiding this comment.
Critical: Approval policy bypassed when loaded from user configuration.
Lines 189-211 only enforce approval checks when self._explicit_policy is True. This means:
-
SUGGEST mode is bypassed: If a user sets
approval_mode = suggestin their config file, commands will still execute because the skip logic (lines 189-194) only fires when an explicit policy was passed to the constructor. -
Confirmation prompts are bypassed: If a user configures AUTO_EDIT mode expecting confirmation prompts, lines 205-211 never fire unless the policy was explicitly passed, so commands execute without confirmation.
-
Security vulnerability: The approval system is completely bypassed for config-based usage, which is the primary use case according to the PR objectives.
The _explicit_policy flag fundamentally breaks the approval system. Policy enforcement should be unconditional regardless of how the policy was loaded.
🔎 Proposed fix to enforce approval checks unconditionally
step.start_time = time.time()
step.status = StepStatus.RUNNING
self._log(f"Executing: {step.command}")
- # 🚫 Suggest mode ONLY when explicitly passed
- if self._explicit_policy and self.approval_policy.mode == ApprovalMode.SUGGEST:
+ # 🚫 Skip execution in suggest mode
+ if self.approval_policy.mode == ApprovalMode.SUGGEST:
step.status = StepStatus.SKIPPED
step.error = "Execution skipped in suggest mode"
step.end_time = time.time()
self._log("Suggest mode: execution skipped")
return True # Not a failure
- # 🔐 Enforce approval ONLY when explicitly passed
- if self._explicit_policy:
- if not self.approval_policy.allow("shell_command"):
- step.status = StepStatus.FAILED
- step.error = "Shell execution disabled by approval policy"
- step.end_time = time.time()
- self._log(f"Execution blocked by approval policy: {step.command}")
- return False
-
- if self.approval_policy.requires_confirmation("shell_command"):
- if not confirm_action("Execute planned shell commands?"):
- step.status = StepStatus.FAILED
- step.error = "User declined execution"
- step.end_time = time.time()
- self._log("Execution declined by user")
- return False
+ # 🔐 Enforce approval policy
+ if not self.approval_policy.allow("shell_command"):
+ step.status = StepStatus.FAILED
+ step.error = "Shell execution disabled by approval policy"
+ step.end_time = time.time()
+ self._log(f"Execution blocked by approval policy: {step.command}")
+ return False
+
+ if self.approval_policy.requires_confirmation("shell_command"):
+ if not confirm_action(f"Execute command: {step.command}?"):
+ step.status = StepStatus.FAILED
+ step.error = "User declined execution"
+ step.end_time = time.time()
+ self._log("Execution declined by user")
+ return False
# Validate commandNote: Also updated the confirmation message to include the specific command for clarity.
🤖 Prompt for AI Agents
cortex/coordinator.py around lines 189 to 211: the approval checks are currently
guarded by self._explicit_policy which lets config-loaded policies be bypassed;
remove the self._explicit_policy conditional so the SUGGEST-mode skip (set
status SKIPPED, end_time, log and return True) and the approval.allow /
requires_confirmation checks run unconditionally for every loaded policy; when
prompting for confirmation include the specific command in the prompt (e.g.
"Execute planned shell command: {step.command}?"), preserve the existing
status/error/end_time assignments and logs for both skip, blocked, and
user-declined flows.
cortex/coordinator.py
Outdated
| if not self.approval_policy.allow("shell_command"): | ||
| self._log("Rollback blocked by approval policy") | ||
| return |
There was a problem hiding this comment.
Inconsistent approval enforcement between execution and rollback.
Lines 267-269 enforce the approval policy unconditionally, while _execute_command (lines 189-211) only enforces approval when self._explicit_policy is True. This creates inconsistent behavior:
- Commands execute without approval checks (when loaded from config)
- But rollback is blocked by approval checks
Once the critical issue in _execute_command is fixed to always enforce approval checks, this inconsistency will be resolved. The current rollback implementation is correct.
🤖 Prompt for AI Agents
In cortex/coordinator.py around lines 267-269, the rollback enforces approval
unconditionally while _execute_command (lines ~189-211) currently only checks
approval when self._explicit_policy is True, creating inconsistent behavior; fix
by changing _execute_command to always consult
self.approval_policy.allow("shell_command") before executing any shell command
(remove or bypass the self._explicit_policy gating), and on denial mirror the
rollback behavior (log "Rollback blocked by approval policy" style message and
abort the execution path) so both execution and rollback enforce approvals
consistently.
cortex/coordinator.py
Outdated
| if not self.approval_policy.allow("shell_command"): | ||
| verification_results[cmd] = False | ||
| self._log("Verification blocked by approval policy") | ||
| continue |
There was a problem hiding this comment.
Inconsistent approval enforcement between execution and verification.
Similar to the rollback flow, lines 345-348 unconditionally enforce the approval policy, while _execute_command only enforces approval when self._explicit_policy is True. This creates inconsistent behavior where:
- Installation commands may execute without approval checks (when policy loaded from config)
- But verification commands respect approval policy
Once the critical issue in _execute_command is fixed to always enforce approval checks, this inconsistency will be resolved. The current verification implementation is correct.
🤖 Prompt for AI Agents
In cortex/coordinator.py around lines 345-348, the verification flow currently
enforces the approval policy unconditionally which is correct; the actual
inconsistency comes from _execute_command only checking approval when
self._explicit_policy is True. Fix _execute_command so it always enforces the
approval policy for "shell_command" (call
self.approval_policy.allow("shell_command") and handle a False result the same
way verification does), remove the conditional gating on self._explicit_policy
for approval checks, and keep the verification block as-is to ensure both
execution and verification follow the same approval semantics.
a42a28b to
808494e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
cortex/coordinator.py (3)
182-182: Remove redundant import.
ApprovalModeis already imported at line 12. This local import is unnecessary and should be removed.🔎 Proposed fix
def _execute_command(self, step: InstallationStep) -> bool: - from cortex.approval import ApprovalMode - if self.approval_policy.mode == ApprovalMode.SUGGEST and (
67-67: Critical issues remain unaddressed from previous reviews.Lines 87-100 contain multiple critical issues previously flagged:
Line 92: FULL_AUTO default contradicts safe-by-default principles. Based on learnings, installations should be "dry-run by default" with "no silent sudo execution." Defaulting to the most permissive mode violates these principles.
Line 97: Broad exception handling silently fails to most permissive mode.
except Exceptioncatches all errors (filesystem, JSON decode, etc.) and silently falls back to FULL_AUTO without logging.Line 87:
_explicit_policyflag undermines the approval system. This creates a two-tier system where approval checks are only enforced when a policy is explicitly passed to the constructor, bypassing the primary use case (CLI/config-based approval control).Based on learnings, the safe default should be SUGGEST mode, not FULL_AUTO.
🔎 Recommended fixes from previous reviews
- self._explicit_policy = approval_policy is not None # 🔐 Load approval policy once if approval_policy is not None: self.approval_policy = approval_policy else: - approval_mode = ApprovalMode.FULL_AUTO + approval_mode = ApprovalMode.SUGGEST try: prefs = UserPreferences.load() if hasattr(prefs, "approval_mode"): approval_mode = prefs.approval_mode - except Exception: + except (FileNotFoundError, json.JSONDecodeError, ValueError): + # Fail safe: default to most restrictive mode pass self.approval_policy = get_approval_policy(approval_mode)Then remove all references to
self._explicit_policythroughout the file.Also applies to: 87-100
198-212: Critical: Approval policy bypassed for config-based usage.Lines 198-212 only enforce approval checks when
self._explicit_policyis True. This means:
SUGGEST mode bypassed: If a user sets
approval_mode = suggestin config, commands still execute because the skip logic at lines 184-192 also checks_explicit_policy.Confirmation prompts bypassed: If a user configures AUTO_EDIT mode expecting confirmation, lines 206-212 never fire unless the policy was explicitly passed.
Security vulnerability: The approval system is completely bypassed for config-based usage, which is the primary use case per PR objectives.
Based on learnings, approval checks should be unconditional.
🔎 Proposed fix to enforce approval unconditionally
# Validate command is_valid, error = self._validate_command(step.command) if not is_valid: step.status = StepStatus.FAILED step.error = error step.end_time = time.time() self._log(f"Command blocked: {step.command} - {error}") return False - # 🔐 Enforce approval ONLY when explicitly passed - if self._explicit_policy: - if not self.approval_policy.allow("shell_command"): - step.status = StepStatus.FAILED - step.error = "Shell execution disabled by approval policy" - step.end_time = time.time() - self._log(f"Execution blocked by approval policy: {step.command}") - return False - - if self.approval_policy.requires_confirmation("shell_command"): - if not confirm_action("Execute planned shell commands?"): - step.status = StepStatus.FAILED - step.error = "User declined execution" - step.end_time = time.time() - self._log("Execution declined by user") - return False + # 🔐 Enforce approval policy + if not self.approval_policy.allow("shell_command"): + step.status = StepStatus.FAILED + step.error = "Shell execution disabled by approval policy" + step.end_time = time.time() + self._log(f"Execution blocked by approval policy: {step.command}") + return False + + if self.approval_policy.requires_confirmation("shell_command"): + if not confirm_action(f"Execute command: {step.command}?"): + step.status = StepStatus.FAILED + step.error = "User declined execution" + step.end_time = time.time() + self._log("Execution declined by user") + return False try:Note: Also updated the confirmation message to include the specific command for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/coordinator.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/coordinator.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/coordinator.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
cortex/coordinator.py
🧬 Code graph analysis (1)
cortex/coordinator.py (4)
cortex/approval.py (1)
ApprovalMode(4-24)cortex/approval_policy.py (9)
get_approval_policy(63-74)allow(23-25)allow(37-38)allow(45-46)allow(53-54)requires_confirmation(28-30)requires_confirmation(40-41)requires_confirmation(48-49)requires_confirmation(56-57)cortex/confirm.py (1)
confirm_action(1-6)cortex/user_preferences.py (2)
UserPreferences(15-55)load(23-42)
🪛 GitHub Actions: CI
cortex/coordinator.py
[error] 185-185: During pytest run: AttributeError: 'InstallationCoordinator' object has no attribute '_policy_from_preferences'.
⏰ 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)
| try: | ||
| self._log(f"Rollback: {cmd}") | ||
| subprocess.run(cmd, shell=True, capture_output=True, timeout=self.timeout) | ||
| subprocess.run( | ||
| cmd, | ||
| shell=True, | ||
| capture_output=True, | ||
| timeout=self.timeout, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the coordinator.py file and examine the relevant sections
find . -name "coordinator.py" -type fRepository: cortexlinux/cortex
Length of output: 85
🏁 Script executed:
# Get the full context around lines 267-274 and understand the _rollback method
if [ -f "cortex/coordinator.py" ]; then
echo "=== Lines 250-290 of _rollback method ==="
sed -n '250,290p' cortex/coordinator.py | cat -n
fiRepository: cortexlinux/cortex
Length of output: 1760
🏁 Script executed:
# Search for approval policy checks in the file
if [ -f "cortex/coordinator.py" ]; then
echo "=== Searching for approval policy references ==="
rg "approval|_explicit_policy|SUGGEST|APPROVE" cortex/coordinator.py -n -A 2 -B 2
fiRepository: cortexlinux/cortex
Length of output: 2571
🏁 Script executed:
# Find the _execute_command method to understand how approval is checked
if [ -f "cortex/coordinator.py" ]; then
echo "=== Looking for _execute_command method ==="
rg "_execute_command" cortex/coordinator.py -n -A 10 -B 2
fiRepository: cortexlinux/cortex
Length of output: 1181
🏁 Script executed:
# Look for verify_installation method mentioned in the review comment
if [ -f "cortex/coordinator.py" ]; then
echo "=== Looking for verify_installation method ==="
rg "def verify_installation" cortex/coordinator.py -n -A 20
fiRepository: cortexlinux/cortex
Length of output: 1218
🏁 Script executed:
# Check for any docstrings or comments about rollback design
if [ -f "cortex/coordinator.py" ]; then
echo "=== Checking context and docstrings ==="
sed -n '255,280p' cortex/coordinator.py | cat -n
fiRepository: cortexlinux/cortex
Length of output: 1161
Missing approval policy enforcement in rollback and verify_installation methods.
The _rollback method (lines 267-274) executes rollback commands without checking the approval policy, creating inconsistent behavior:
_execute_commandrespects the approval policy when explicitly set (skipping execution in SUGGEST mode, blocking if not allowed)_rollbackandverify_installationunconditionally execute without any policy checks
If rollback should always execute regardless of approval mode (recommended for safety), document this design decision with a comment explaining why rollback is exempt from approval checks. If it should respect the policy for consistency, apply approval checks similar to _execute_command.
🤖 Prompt for AI Agents
In cortex/coordinator.py around lines 267-274 (and the verify_installation
method), the rollback and verification paths unconditionally invoke
subprocess.run and therefore bypass the approval-policy semantics enforced by
_execute_command; either (preferred for consistency) add the same
approval-policy checks used in _execute_command (i.e., consult the approval
mode, skip execution in SUGGEST mode and raise or block when policy disallows)
before running rollback/verify commands, or if rollback must always run for
safety, add a clear comment above these calls documenting that rollback/verify
are intentionally exempt from approval checks and why; apply the chosen approach
to both _rollback and verify_installation to ensure consistent behavior and
logging.
cortex/coordinator.py
Outdated
| if self._explicit_policy and self.approval_policy.mode == ApprovalMode.SUGGEST: | ||
| all_success = False | ||
| else: | ||
| self._log("Installation completed with errors") | ||
| all_success = all(s.status == StepStatus.SUCCESS for s in self.steps) | ||
|
|
||
| self._log( | ||
| "Installation completed successfully" | ||
| if all_success | ||
| else "Installation completed with errors" | ||
| ) |
There was a problem hiding this comment.
Inconsistent success determination tied to _explicit_policy flag.
Lines 316-319 create special handling for SUGGEST mode only when _explicit_policy is True. This leads to inconsistent behavior:
- SUGGEST mode via explicit policy →
all_success = False(line 317) - SUGGEST mode via config → success computed from step statuses (line 319)
This inconsistency stems from the broader _explicit_policy design flaw flagged in previous reviews. Once that flag is removed and approval checks are enforced unconditionally, the success determination should be based purely on step statuses.
🔎 Proposed fix after removing _explicit_policy
total_duration = time.time() - start_time
- if self._explicit_policy and self.approval_policy.mode == ApprovalMode.SUGGEST:
- all_success = False
- else:
- all_success = all(s.status == StepStatus.SUCCESS for s in self.steps)
+ all_success = all(s.status == StepStatus.SUCCESS for s in self.steps)
self._log(This makes success determination consistent: based purely on whether all steps succeeded.
🤖 Prompt for AI Agents
In cortex/coordinator.py around lines 316 to 325, the code currently forces
all_success = False when _explicit_policy is True and approval_policy.mode ==
ApprovalMode.SUGGEST, causing inconsistent behavior depending on how the policy
was set; change the logic to always compute success purely from step statuses
(all_success = all(s.status == StepStatus.SUCCESS for s in self.steps)) and
remove the special-case branch that checks _explicit_policy so the final log
("Installation completed successfully" vs "Installation completed with errors")
is determined solely by step outcomes.
808494e to
26e52ea
Compare
pavanimanchala53
force-pushed
the
feature/tiered-approval-modes
branch
from
26e52ea to
3464765
Compare
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@pavanimanchala53 PR doesn’t follow contributing.md (AI usage disclosure, docs, and demo video are missing). Please update these and converting the PR to draft until then.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@pavanimanchala53 CI checks failing.
|
3 participants
…d tests
Related Issue
Closes #267
Summary
This PR introduces tiered approval modes to control how Cortex executes actions:
Key Changes
ApprovalModeenum and centralized approval policy enginecortex config set approval-mode <mode>Motivation
Provides safer, more flexible execution control aligned with enterprise CLI expectations and the project’s security goals.
Testing
pytest--executeSummary by CodeRabbit
New Features
Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.