Added cortex CLI messaging and documentation Fixes #11 #191
Conversation
WalkthroughAdds project packaging, repository ignores, a new Cortex CLI, an installation coordinator with execution/rollback/logging, test runner and extensive tests, and minor test import/path adjustments. No changes to existing public APIs beyond new modules and exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CortexCLI
participant Interp as CommandInterpreter
participant Coord as InstallationCoordinator
participant Shell as Shell
User->>CLI: install(software, execute=True|--dry-run)
activate CLI
CLI->>CLI: _get_api_key() / _get_provider()
CLI->>CLI: _animate_spinner("Planning...")
CLI->>Interp: Interpret "install <software>"
Interp-->>CLI: commands[]
alt commands produced
CLI->>Coord: new InstallationCoordinator(commands,...)
activate Coord
loop per step
Coord->>Shell: subprocess.run(command)
Shell-->>Coord: stdout/stderr, returncode
Coord->>CLI: progress_callback(index,total,step)
end
Coord-->>CLI: InstallationResult(success/failure)
deactivate Coord
CLI-->>User: Exit code (0/1)
else no commands
CLI-->>User: Exit code 1 (error)
end
deactivate CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (9)
cortex/__init__.py (1)
1-2: Exposemainis good; consider centralizing version definitionRe-exporting
mainfromcortex.cliand defining__version__are both useful. To avoid version drift betweencortex/__init__.pyandsetup.py, consider importing__version__intosetup.py(or reading it from a single version module) instead of duplicating the literal"0.1.0"in two places.test/run_all_tests.py (1)
1-22: Test runner wiring is correct and CLI-friendlyThe runner correctly adds the project root to
sys.path, discoverstest_*.pyunder thetestdirectory, and returns an appropriate exit code for the CLI--testflag. If you later wantLLM/test_interpreter.pyto run viacortex --test, you might either move it undertest/or expand discovery to additional directories.setup.py (1)
1-43: Make README/requirements loading robust to working directoryPackaging metadata and entry points look good. The
open("README.md", ...)andopen(os.path.join("LLM", "requirements.txt"), ...)calls assumesetup.pyis run from the project root. To make this robust (e.g., when tools invoke setup from another CWD), consider:-import os - -with open("README.md", "r", encoding="utf-8") as fh: +import os + +here = os.path.abspath(os.path.dirname(__file__)) + +with open(os.path.join(here, "README.md"), "r", encoding="utf-8") as fh: long_description = fh.read() -with open(os.path.join("LLM", "requirements.txt"), "r", encoding="utf-8") as fh: +with open(os.path.join(here, "LLM", "requirements.txt"), "r", encoding="utf-8") as fh: requirements = [line.strip() for line in fh if line.strip() and not line.startswith("#")]LLM/test_interpreter.py (1)
4-8: Interpreter tests align with implementation; tidy unused patch argumentsThe tests correctly target
openai.OpenAI/anthropic.Anthropicgiven the imports inCommandInterpreter._initialize_client, and the scenarios (success, failure, validation, context) match the production behavior.Ruff’s ARG002 warning about unused
mock_openai(e.g., intest_parse_empty_input) is purely cosmetic here since the decorator still performs the patch. If you want a clean lint run without losing the safety of the patch, you can either:
- Prefix unused parameters with
_:-@patch('openai.OpenAI') -def test_parse_empty_input(self, mock_openai): +@patch('openai.OpenAI') +def test_parse_empty_input(self, _mock_openai):
- Or add minimal assertions on the mocks in those tests.
cortex/test_coordinator.py (1)
1-352: Coordinator and Docker workflow tests are comprehensive; consider tightening timeout simulationThe coordinator test suite is very thorough: it exercises step lifecycle, stop vs. continue on error, rollback, logging, verification, JSON export, timing, and the higher-level
install_docker()helper. That gives good confidence incortex.coordinator.In
test_timeout_handling, you currently simulate a timeout with a genericException("Timeout"). To specifically cover thesubprocess.TimeoutExpiredbranch in_execute_command, you could instead raisesubprocess.TimeoutExpiredthere and optionally assert on the timeout error message. This is not required for correctness but would make the test more precise.cortex/cli.py (1)
62-145: Clarifyinstallflow around error handling and flag semanticsThe overall install orchestration is clear and preserves existing behavior, but there are a couple of points worth tightening:
executevsdry_run: when both areTrue, the current flow silently treats it as a dry-run because thedry_runcheck happens first. That’s reasonable, but it’s implicit. Either documenting this precedence or actively rejecting the combination with a message would avoid surprises.- Exception handling: catching
ValueError/RuntimeErrorexplicitly is good, but the broadexcept Exceptionrisks hiding programmer errors. Consider limiting the broad catch to a debug/verbose mode or at least re-raising after printing when running in a development environment.No functional bug here, but clarifying intent would make the behavior easier to reason about and maintain.
cortex/coordinator.py (3)
27-45: HardenInstallationStep.durationagainst falsy timestamps
duration()currently checksif self.start_time and self.end_time, which will treat0.0as “no timestamp”. It’s unlikely withtime.time(), but tests or future code that set timestamps explicitly could trip this.A more robust pattern is to check against
Noneexplicitly:- def duration(self) -> Optional[float]: - """Return the elapsed execution time for the step if available.""" - if self.start_time and self.end_time: - return self.end_time - self.start_time - return None + def duration(self) -> Optional[float]: + """Return the elapsed execution time for the step if available.""" + if self.start_time is not None and self.end_time is not None: + return self.end_time - self.start_time + return None
92-147: Be explicit aboutshell=Trueand broad exception handling in command execution
_execute_commandruns arbitrary commands via:result = subprocess.run( step.command, shell=True, capture_output=True, text=True, timeout=self.timeout, )combined with catching
Exceptionand returning a generic failure. Given thatcommandscan be LLM-generated (via the CLI + interpreter), this is a deliberately powerful but high‑risk surface:
shell=Truemeans the command string is interpreted by the user’s shell; if any upstream component ever concatenates user input into these commands, it becomes a shell‑injection vector.- A blanket
except Exceptionprotects the coordinator from crashing but can also hide unexpected programming errors or environment issues, making debugging harder.Suggestions:
- Clearly document, in this module’s docstring or class docstring, that
commandsare executed withshell=Trueand must be treated as fully trusted (or appropriately sandboxed) by the caller.- Where possible, prefer argument lists and
shell=Falsefor fixed, internal commands; keepshell=Trueonly where strictly needed.- Narrow the
except Exceptionto the specific error types you expect (e.g.,OSError,subprocess.SubprocessError), and consider logging or surfacing unexpected exceptions more explicitly.These changes would improve the security posture and debuggability of the coordinator without changing its external API.
274-312: Clarify or tighteninstall_dockerverification behaviorThe
install_dockerhelper is a nice convenience, and the base command list looks reasonable. Two small points to consider:
- Verification results from
coordinator.verify_installation(verify_commands)are currently ignored. Either:
- Use them to adjust the returned
InstallationResultor log a concise summary, or- Document that verification is best‑effort and only affects logging, not the returned status.
- All commands assume sufficient privileges (no
sudo), which is fine if you expectinstall_docker()to run as root, but it’s worth documenting in the docstring so callers know what environment is required.These tweaks would make the helper’s behavior more transparent to callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)LLM/test_interpreter.py(10 hunks)MANIFEST.in(1 hunks)cortex/__init__.py(1 hunks)cortex/cli.py(1 hunks)cortex/coordinator.py(1 hunks)cortex/test_coordinator.py(1 hunks)setup.py(1 hunks)test/run_all_tests.py(1 hunks)test/test_cli.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
cortex/__init__.py (1)
cortex/cli.py (1)
main(147-199)
cortex/test_coordinator.py (1)
cortex/coordinator.py (11)
InstallationCoordinator(58-271)InstallationStep(28-44)InstallationResult(48-55)StepStatus(17-24)install_docker(274-312)duration(40-44)execute(171-218)add_rollback_command(167-169)verify_installation(220-242)get_summary(244-266)export_log(268-271)
test/run_all_tests.py (1)
cortex/cli.py (1)
main(147-199)
LLM/test_interpreter.py (1)
LLM/interpreter.py (3)
CommandInterpreter(12-158)APIProvider(7-9)parse(131-145)
cortex/coordinator.py (1)
cortex/cli.py (1)
progress_callback(98-105)
cortex/cli.py (3)
LLM/interpreter.py (2)
CommandInterpreter(12-158)parse(131-145)cortex/coordinator.py (3)
InstallationCoordinator(58-271)StepStatus(17-24)execute(171-218)test/run_all_tests.py (1)
main(8-18)
test/test_cli.py (2)
cortex/cli.py (9)
CortexCLI(14-144)main(147-199)_get_api_key(22-28)_get_provider(30-36)_print_status(38-40)_print_error(42-44)_print_success(46-48)install(62-144)_animate_spinner(50-55)LLM/interpreter.py (1)
parse(131-145)
🪛 Ruff (0.14.4)
LLM/test_interpreter.py
82-82: Unused method argument: mock_openai
(ARG002)
cortex/coordinator.py
80-80: Avoid specifying long messages outside the exception class
(TRY003)
101-102: try-except-pass detected, consider logging the exception
(S110)
101-101: Do not catch blind exception: Exception
(BLE001)
112-112: subprocess call with shell=True identified, security issue
(S602)
141-141: Do not catch blind exception: Exception
(BLE001)
145-145: Use explicit conversion flag
Replace with conversion flag
(RUF010)
158-158: subprocess call with shell=True identified, security issue
(S602)
164-164: Do not catch blind exception: Exception
(BLE001)
165-165: Use explicit conversion flag
Replace with conversion flag
(RUF010)
228-228: subprocess call with shell=True identified, security issue
(S602)
238-238: Do not catch blind exception: Exception
(BLE001)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
cortex/cli.py
134-134: Consider moving this statement to an else block
(TRY300)
140-140: Use explicit conversion flag
Replace with conversion flag
(RUF010)
142-142: Do not catch blind exception: Exception
(BLE001)
143-143: Use explicit conversion flag
Replace with conversion flag
(RUF010)
187-187: subprocess call: check for execution of untrusted input
(S603)
test/test_cli.py
28-28: Unused method argument: mock_stderr
(ARG002)
48-48: Unused method argument: mock_stderr
(ARG002)
53-53: Unused method argument: mock_stdout
(ARG002)
🔇 Additional comments (3)
MANIFEST.in (1)
1-5: Packaging manifest looks appropriate for initial distributionREADME, LICENSE, core
LLMandcortexsources, andLLM/requirements.txtare all included, which is a solid baseline for sdist/wheel contents..gitignore (1)
1-30: Reasonable default Python.gitignoreThe ignore list covers typical Python build artifacts, venvs, and coverage outputs, which should keep the repo clean without hiding source files used here.
cortex/cli.py (1)
14-61: Helper methods and spinner implementation look solidThe CLI helpers (_get_api_key, _get_provider, printing helpers, spinner, and line clearing) are cohesive, well documented, and keep side effects localized to the CLI layer; no issues from a correctness perspective.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/test_cli.py (2)
26-30: Assert that an error was written to stderr.The
mock_stderrparameter is patched but never used in the assertion. When no API key is found,_get_api_key()calls_print_error(), which writes to stderr. Verify this behavior to make the test more robust and address the static analysis warning.Apply this diff:
@patch.dict(os.environ, {}, clear=True) @patch('sys.stderr') def test_get_api_key_not_found(self, mock_stderr): api_key = self.cli._get_api_key() self.assertIsNone(api_key) + mock_stderr.write.assert_called()
42-55: Print tests improved; optionally verify content.The assertions now properly verify that output was written, addressing the previous review concerns. For even stronger validation, consider using
assert_any_call()orassert_called_with()to check the actual formatted strings, but the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/test_cli.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_cli.py (2)
cortex/cli.py (9)
CortexCLI(14-144)main(147-199)_get_api_key(22-28)_get_provider(30-36)_print_status(38-40)_print_error(42-44)_print_success(46-48)install(62-144)_animate_spinner(50-55)LLM/interpreter.py (1)
parse(131-145)
🪛 Ruff (0.14.4)
test/test_cli.py
28-28: Unused method argument: mock_stderr
(ARG002)
🔇 Additional comments (2)
test/test_cli.py (2)
56-167: Comprehensive test coverage for install scenarios.The test suite thoroughly covers the various paths through the
install()method, including:
- Missing API key → exit 1
- Dry-run mode → exit 0 without execution
- Execute flag handling for both success and failure
- Empty command generation → exit 1
- Error handling for ValueError, RuntimeError, and generic exceptions → exit 1
The mocking strategy properly isolates the CLI logic from CommandInterpreter and InstallationCoordinator dependencies.
169-196: Main function tests validate CLI argument parsing.The tests properly verify that the
main()entry point correctly parses command-line arguments and dispatches toCortexCLI.install()with the appropriate flags. The use ofsys.argvpatching is the standard approach for testing argparse-based CLIs.
|
🎯 Closing for MVP Focus This issue is being closed to help the team focus on MVP-critical features (#1-45). This is NOT abandoned - it's an important feature we'll revisit after MVP completion. Timeline:
Want to work on this anyway? Tracking: Labeled as Thanks for understanding! 🚀 — Mike (@mikejmorgan-ai) |
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Tests
Chores