Add Installation Templates System for Common Development Stacks #201
Conversation
WalkthroughAdds a template subsystem and CLI template workflow: new template models, validator, manager, five built-in YAML templates, CLI template subcommands and a template-driven install path with hardware checks, import/export, docs, tests, and YAML runtime dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as Cortex CLI
participant TMgr as TemplateManager
participant HW as HardwareProfiler
participant PM as PackageManager
participant Exec as InstallationCoordinator
User->>CLI: cortex install --template <name>
CLI->>TMgr: load_template(name)
TMgr-->>CLI: Template / error
CLI->>TMgr: check_hardware_compatibility(template)
TMgr->>HW: profile_system()
HW-->>TMgr: hardware_info
TMgr-->>CLI: compatibility (ok/warnings)
alt warnings & interactive
CLI->>User: show warnings & prompt
User-->>CLI: confirm/cancel
end
CLI->>TMgr: generate_commands(template)
TMgr->>PM: resolve package commands (if needed)
PM-->>TMgr: package_commands
TMgr-->>CLI: installation_commands
CLI->>Exec: execute_commands(installation_commands, dry_run?/execute?)
Exec-->>CLI: success/failure (+ per-step results)
CLI->>CLI: record_installation_history(result, template)
CLI-->>User: report outcome (include rollback hints on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (14)
docs/TEMPLATES.md (2)
67-68: Fix bare URLs for better markdown rendering.The URLs on these lines should be wrapped in angle brackets
<>or converted to proper markdown links for better rendering and to comply with markdown linting standards.Based on learnings
Apply this diff:
**Access:** -- Apache: http://localhost -- phpMyAdmin: http://localhost/phpmyadmin +- Apache: <http://localhost> +- phpMyAdmin: <http://localhost/phpmyadmin>
318-331: Add language identifier to code fence.The code fence starting at line 319 should specify a language identifier (e.g.,
textorconsole) for proper syntax highlighting and to comply with markdown standards.Based on learnings
Apply this diff:
Output: -``` +```text 📋 Available Templates:cortex/templates/ml-ai.yaml (3)
25-33: Consider version pinning for ML libraries to ensure compatibility.Installing TensorFlow, PyTorch, and other ML libraries without version constraints can lead to:
- Dependency conflicts between TensorFlow and PyTorch
- Breaking changes in newer versions
- Incompatible transitive dependencies
- Non-reproducible installations
Consider specifying compatible version ranges:
- - command: pip3 install numpy pandas scipy matplotlib scikit-learn jupyter notebook + - command: pip3 install numpy pandas scipy matplotlib scikit-learn jupyter notebook description: Install core ML libraries requires_root: false - - command: pip3 install tensorflow torch torchvision torchaudio + - command: pip3 install tensorflow>=2.12.0,<3.0.0 torch>=2.0.0,<3.0.0 torchvision torchaudio description: Install deep learning frameworks requires_root: false
28-30: Add timeout or split large package installations.Installing TensorFlow and PyTorch together can take a very long time and consume significant bandwidth (multiple GB). Consider:
- Adding timeout handling
- Splitting into separate steps for better progress feedback
- Warning users about installation time and size
Split the installation for better UX:
- - command: pip3 install tensorflow torch torchvision torchaudio - description: Install deep learning frameworks + - command: pip3 install tensorflow + description: Install TensorFlow (this may take several minutes) + requires_root: false + - command: pip3 install torch torchvision torchaudio + description: Install PyTorch (this may take several minutes) requires_root: false
46-47: Simplify post-install verification commands.The complex shell commands with error redirection and fallbacks in post_install may fail silently or produce confusing output.
Simplify the commands:
- - echo "TensorFlow: $(python3 -c 'import tensorflow as tf; print(tf.__version__)' 2>/dev/null || echo 'installed')" - - echo "PyTorch: $(python3 -c 'import torch; print(torch.__version__)' 2>/dev/null || echo 'installed')" + - python3 -c "import tensorflow as tf; print(f'TensorFlow: {tf.__version__}')" || echo "TensorFlow: import failed" + - python3 -c "import torch; print(f'PyTorch: {torch.__version__}')" || echo "PyTorch: import failed"test/test_templates.py (1)
1-1: Make test file executable or remove shebang.The file has a shebang (
#!/usr/bin/env python3) but is not marked as executable. Either make it executable or remove the shebang.Make the file executable:
chmod +x test/test_templates.pyOr remove the shebang if the file is only meant to be run via pytest/unittest.
cortex/templates/mean.yaml (1)
1-64: Consider template refactoring to reduce duplication.The MEAN and MERN templates share significant structure:
- Same Node.js setup process (lines 13-21)
- Same MongoDB installation (lines 25-35)
- Similar verification commands
While templates should remain self-contained for clarity, consider documenting common patterns or creating template composition capabilities in future iterations to reduce maintenance burden.
cortex/templates/devops.yaml (1)
1-93: DevOps template structure looks solid; only minor consistency nitThe template fields (metadata,
packages,steps,hardware_requirements,post_install,verification_commands,metadata) line up with theTemplate/InstallationStepmodel and should round‑trip cleanly throughTemplate.from_dict/TemplateValidator. Therequires_rootflags also correctly mark privileged operations.One optional improvement: the
packageslist is more conceptual, while the actual installation is driven by the explicitsteps(e.g., Docker installed via the official repo asdocker-ce, notdocker.io). If you want history and UX to reflect the exact packages that were installed, you could either (a) alignpackageswith the concrete packages used in the steps, or (b) leavepackagesempty for step‑driven templates and let history derive packages from the commands instead. Not required, but it would remove a small source of confusion when inspecting installation records.cortex/cli.py (4)
299-304: Minor polish: avoid placeholder-free f-stringsThere are a couple of f-strings that don’t interpolate anything (e.g.,
print(f"\n[WARNING] Hardware Compatibility Warnings:")andprint("\n(Dry run mode - commands not executed)")vsprint(f"\n(Dry run mode - commands not executed)")). These trigger Ruff F541 and can just be plain strings:- print(f"\n[WARNING] Hardware Compatibility Warnings:") + print("\n[WARNING] Hardware Compatibility Warnings:") @@ - print(f"\n(Dry run mode - commands not executed)") + print("\n(Dry run mode - commands not executed)")This is cosmetic only, but it quiets the linter and clarifies intent.
Also applies to: 339-344
479-535: Improve robustness of interactivetemplate_createThe interactive
template_createflow works, but a couple of small tweaks would make it more robust:
- You re-import
TemplateandHardwareRequirementsinside the method even though they’re already imported at module level.- Converting hardware requirement inputs directly with
int()will raiseValueErroron non-numeric input and surface as a generic “Failed to create template” error.You could simplify and harden this block like:
- # Create template - from cortex.templates import Template, HardwareRequirements - template = Template( + # Create template + template = Template( name=name, description=description, version=version, author=author, packages=packages ) @@ - if min_ram or min_cores or min_storage: - hw_req = HardwareRequirements( - min_ram_mb=int(min_ram) if min_ram else None, - min_cores=int(min_cores) if min_cores else None, - min_storage_mb=int(min_storage) if min_storage else None - ) - template.hardware_requirements = hw_req + if min_ram or min_cores or min_storage: + try: + hw_req = HardwareRequirements( + min_ram_mb=int(min_ram) if min_ram else None, + min_cores=int(min_cores) if min_cores else None, + min_storage_mb=int(min_storage) if min_storage else None, + ) + template.hardware_requirements = hw_req + except ValueError: + self._print_error("Hardware requirements must be integers (MB/cores).") + return 1This avoids a surprising stack trace for a simple input typo and removes an unnecessary re-import.
619-640: Argparse wiring fortemplatesubcommands is correct; minor cleanupThe CLI wiring for
templatesubcommands andinstall --templatelooks good and should behave as documented. A couple of nits from static analysis:
template_list_parseris assigned but never used; you can drop the variable and just calladd_parser(...).- The
templatedispatch block is exhaustive; the finalelsebranch printingtemplate_parser.print_help()is fine.Example cleanup:
- # Template list - template_list_parser = template_subparsers.add_parser('list', help='List all available templates') + # Template list + template_subparsers.add_parser('list', help='List all available templates')Functionally everything is sound; this just reduces a tiny bit of dead code and silences Ruff’s F841 warning.
Also applies to: 649-673
59-76: Template-awareinstallflow is consistent with existing behaviorThe updated
installmethod short-circuits to_install_from_templatewhentemplateis provided, and the CLI entrypoint correctly passes an emptysoftwarestring only in that case:
- When
--templateis set,installdoes not touch the LLM path or thesoftwareargument, so the empty string is harmless.- When
softwareis given without--template, the original LLM-driven flow is preserved.There is a tiny bit of duplication (
InstallationHistory()andstart_timeare created even when immediately delegating to_install_from_template), but that’s just an efficiency nit and not worth complicating the control flow right now.Also applies to: 649-657
cortex/templates.py (2)
428-452: Reuse the existingPackageManagerinstead of instantiating a new one
TemplateManager.__init__already createsself._package_manager = PackageManager(), butgenerate_commandsignores it and instantiates a newPackageManager:elif template.packages: # Use package manager to generate commands pm = PackageManager() package_list = " ".join(template.packages) try: commands = pm.parse(f"install {package_list}")This is minor, but it means you re-run package manager detection and mappings unnecessarily. You can simplify and avoid redundant work by reusing the existing instance:
- elif template.packages: - # Use package manager to generate commands - pm = PackageManager() - package_list = " ".join(template.packages) + elif template.packages: + # Use the existing package manager to generate commands + pm = self._package_manager + package_list = " ".join(template.packages)Functionally it’s equivalent, but keeps initialization centralized and slightly reduces overhead when generating commands from templates.
151-156: AnnotateTemplateValidator.REQUIRED_FIELDS/REQUIRED_STEP_FIELDSasClassVarRuff correctly points out that the mutable class attributes in
TemplateValidatorshould be annotated asClassVarto make their intent clear and avoid confusion with instance fields:-from typing import Dict, List, Optional, Any, Set, Tuple +from typing import Dict, List, Optional, Any, Set, Tuple, ClassVar @@ class TemplateValidator: @@ - REQUIRED_FIELDS = ["name", "description", "version"] - REQUIRED_STEP_FIELDS = ["command", "description"] + REQUIRED_FIELDS: ClassVar[List[str]] = ["name", "description", "version"] + REQUIRED_STEP_FIELDS: ClassVar[List[str]] = ["command", "description"]This doesn’t change runtime behavior but makes the dataclass/linter story cleaner and documents that these are shared constants, not per-instance state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cortex/cli.py(7 hunks)cortex/templates.py(1 hunks)cortex/templates/README.md(1 hunks)cortex/templates/devops.yaml(1 hunks)cortex/templates/lamp.yaml(1 hunks)cortex/templates/mean.yaml(1 hunks)cortex/templates/mern.yaml(1 hunks)cortex/templates/ml-ai.yaml(1 hunks)docs/TEMPLATES.md(1 hunks)src/requirements.txt(1 hunks)test/test_templates.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cortex/templates.py (2)
src/hwprofiler.py (2)
HardwareProfiler(15-439)profile(399-426)cortex/packages.py (2)
PackageManager(23-452)PackageManagerType(16-20)
test/test_templates.py (1)
cortex/templates.py (16)
Template(54-148)TemplateManager(201-517)TemplateValidator(151-198)TemplateFormat(25-28)HardwareRequirements(32-40)InstallationStep(44-50)to_dict(67-106)from_dict(109-148)validate(158-198)load_template(241-261)save_template(263-293)list_templates(295-338)generate_commands(428-452)import_template(454-489)export_template(491-517)check_hardware_compatibility(340-426)
cortex/cli.py (3)
cortex/templates.py (12)
TemplateManager(201-517)Template(54-148)TemplateFormat(25-28)InstallationStep(44-50)load_template(241-261)list_templates(295-338)check_hardware_compatibility(340-426)generate_commands(428-452)HardwareRequirements(32-40)save_template(263-293)import_template(454-489)export_template(491-517)cortex/coordinator.py (6)
InstallationStep(19-32)execute(202-249)InstallationCoordinator(44-300)from_plan(78-124)StepStatus(10-15)verify_installation(251-273)installation_history.py (7)
InstallationHistory(68-653)_extract_packages_from_commands(212-244)record_installation(252-308)InstallationType(25-31)update_installation(310-362)InstallationStatus(34-39)rollback(461-586)
🪛 markdownlint-cli2 (0.18.1)
docs/TEMPLATES.md
67-67: Bare URL used
(MD034, no-bare-urls)
68-68: Bare URL used
(MD034, no-bare-urls)
319-319: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
cortex/templates.py
1-1: Shebang is present but file is not executable
(EXE001)
154-154: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
155-155: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
259-259: Consider moving this statement to an else block
(TRY300)
260-260: Do not catch blind exception: Exception
(BLE001)
261-261: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Use explicit conversion flag
Replace with conversion flag
(RUF010)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
315-316: try-except-pass detected, consider logging the exception
(S110)
315-315: Do not catch blind exception: Exception
(BLE001)
335-336: try-except-pass detected, consider logging the exception
(S110)
335-335: Do not catch blind exception: Exception
(BLE001)
467-467: Avoid specifying long messages outside the exception class
(TRY003)
485-485: Abstract raise to an inner function
(TRY301)
485-485: Avoid specifying long messages outside the exception class
(TRY003)
487-487: Consider moving this statement to an else block
(TRY300)
488-488: Do not catch blind exception: Exception
(BLE001)
489-489: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
489-489: Avoid specifying long messages outside the exception class
(TRY003)
489-489: Use explicit conversion flag
Replace with conversion flag
(RUF010)
506-506: Avoid specifying long messages outside the exception class
(TRY003)
test/test_templates.py
1-1: Shebang is present but file is not executable
(EXE001)
cortex/cli.py
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
301-301: f-string without any placeholders
Remove extraneous f prefix
(F541)
398-398: subprocess call with shell=True identified, security issue
(S602)
408-408: subprocess call with shell=True identified, security issue
(S602)
441-441: Consider moving this statement to an else block
(TRY300)
448-448: Do not catch blind exception: Exception
(BLE001)
451-451: Use explicit conversion flag
Replace with conversion flag
(RUF010)
474-474: Consider moving this statement to an else block
(TRY300)
475-475: Do not catch blind exception: Exception
(BLE001)
476-476: Use explicit conversion flag
Replace with conversion flag
(RUF010)
536-536: Do not catch blind exception: Exception
(BLE001)
537-537: Use explicit conversion flag
Replace with conversion flag
(RUF010)
552-552: Consider moving this statement to an else block
(TRY300)
553-553: Do not catch blind exception: Exception
(BLE001)
554-554: Use explicit conversion flag
Replace with conversion flag
(RUF010)
566-566: Consider moving this statement to an else block
(TRY300)
567-567: Do not catch blind exception: Exception
(BLE001)
568-568: Use explicit conversion flag
Replace with conversion flag
(RUF010)
624-624: Local variable template_list_parser is assigned to but never used
Remove assignment to unused variable template_list_parser
(F841)
🔇 Additional comments (4)
cortex/templates/README.md (1)
1-65: LGTM! Well-structured template directory documentation.This README provides a clear overview of the available templates, usage examples, and template format structure. The reference to the comprehensive TEMPLATES.md documentation is helpful.
test/test_templates.py (1)
27-451: LGTM! Comprehensive test coverage for template system.The test suite provides excellent coverage of the template system:
- Template creation and serialization (to_dict/from_dict)
- Validation with various error conditions
- Template management (load, save, list)
- Command generation from packages and steps
- Import/export functionality
- Hardware compatibility checking
- Multiple format support (YAML/JSON)
The tests are well-structured with proper setup/teardown and use temporary directories to avoid side effects.
cortex/templates/mean.yaml (1)
22-24: Verify npm global installation permissions.Same issue as MERN: global npm installations typically require root unless npm is configured with a user-level prefix.
This requires the same verification as the MERN template. The npm configuration should be checked to ensure
-ginstallations work without root.src/requirements.txt (1)
7-7: No security issues found; version constraint is secure.The specified constraint
pyyaml>=6.0.0is secure. All identified CRITICAL vulnerabilities (from 2019-2021) are in versions < 5.4 and have been patched; versions 6.0.0 and later are safe. The latest version is 6.0.3. The current constraint reasonably allows patch updates within the 6.0.x line.
cortex/templates/mern.yaml
Outdated
| - command: npm install -g create-react-app express-generator | ||
| description: Install React and Express generators globally | ||
| requires_root: false |
There was a problem hiding this comment.
🧩 Analysis chain
Verify npm global installation permissions.
Global npm installations (npm install -g) typically require elevated privileges unless npm is configured with a user-level prefix. With requires_root: false, this command may fail on many systems.
Run the following to check typical npm configuration and verify the command will work:
🏁 Script executed:
#!/bin/bash
# Check default npm prefix and permissions
echo "=== NPM global prefix ==="
npm config get prefix
echo -e "\n=== Checking if npm -g requires root ==="
# Test if user can write to global npm directory
NPM_PREFIX=$(npm config get prefix 2>/dev/null || echo "/usr/local")
if [ -w "$NPM_PREFIX/lib/node_modules" ]; then
echo "User can write to global npm directory"
else
echo "User CANNOT write to global npm directory - requires root"
fiLength of output: 136
🏁 Script executed:
#!/bin/bash
# Get full context of the MERN template
echo "=== Full mern.yaml content (lines 1-50) ==="
head -50 cortex/templates/mern.yaml
echo -e "\n=== Searching for npm configuration in template ==="
grep -n "npm" cortex/templates/mern.yaml || echo "No other npm references found"
echo -e "\n=== Checking if there are setup instructions ==="
find cortex/templates -name "*.md" -o -name "README*" | head -5Length of output: 1738
Change requires_root: false to requires_root: true for npm global installation.
The template installs Node.js via apt with requires_root: true (lines 20-22), which configures npm's global prefix at /usr/local owned by root. Subsequent npm install -g commands must also run as root. The mismatch at line 23-25 will cause the command to fail with permission errors.
- command: npm install -g create-react-app express-generator
description: Install React and Express generators globally
requires_root: true
🤖 Prompt for AI Agents
In cortex/templates/mern.yaml around lines 22 to 24 the npm global install
command is marked requires_root: false while previous steps configured npm
global prefix under /usr/local as root; change requires_root to true so the npm
install -g runs with root privileges and will succeed without permission errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
cortex/templates/ml-ai.yaml (1)
35-41: GPU/CUDA recommendation is descriptive only (no compatibility check).With
requires_gpu: falseandrequires_cuda: false, the hardware compatibility logic inTemplateManager.check_hardware_compatibilitywill never emit GPU/CUDA warnings for this template, even though the description recommends a GPU for deep learning. That’s fine if GPU is truly optional, but if you want the CLI to surface a warning when no suitable GPU/CUDA is detected (while still allowing users to continue), consider setting the GPU/CUDA flags and a minimal CUDA version so the checker can report that guidance programmatically.cortex/cli.py (5)
345-380: Add a progress callback for step-based template executions.When
template.stepsis present you build aplanand callInstallationCoordinator.from_plan(...)without aprogress_callback, so template-based installs with explicit steps won’t show per-step progress, while the fallback branch usingInstallationCoordinator(...)does.For consistent UX, consider wiring a progress callback into the
from_plancall similar to the non-steps branch, e.g., defining a localprogress_callbackand passing it as a keyword argument.
394-400: Post-install commands executed viashell=Trueon template content.
template.post_installentries are executed withsubprocess.run(cmd, shell=True). Given these commands are template-controlled (and potentially user-imported), this is powerful and expected, but it also means any imported template effectively has arbitrary shell execution rights when--executeis used.If you want a safer default, you could:
- Reserve
shell=Trueonly for commands that truly need shell features, or- Treat post-install purely as informational (e.g.,
echotext parsed and printed via Python) and move “real” commands intostepswhere you might later add richer controls.Not a blocker, but worth consciously documenting as part of the trust model for templates.
448-471: Template CLI commands use broadexcept Exception; consider narrowing or surfacing more detail.
template_list,template_create, andtemplate_import/template_exportall wrap their bodies inexcept Exception as e, which is flagged by static analysis and can hide useful distinctions between user errors (bad paths, invalid formats) and actual bugs.Consider:
- Catching more specific exceptions where possible (e.g.,
FileNotFoundError,ValueError,OSError), and- Letting unexpected exceptions bubble up to the outer
main()handler, or at least re‑raising after logging for debugging.This improves debuggability without changing behavior for expected user-facing failures.
618-619: Remove unusedtemplate_list_parservariable to satisfy linting.
template_list_parseris assigned but never used; Ruff flags this as F841. Since you don’t need the parser reference later, you can drop the binding:- # Template list - template_list_parser = template_subparsers.add_parser('list', help='List all available templates') + # Template list + template_subparsers.add_parser('list', help='List all available templates')
643-651: Ambiguous combination ofsoftwareand--templatearguments.In
main(), if bothargs.templateandargs.softwareare provided, theargs.templatebranch wins andsoftwareis ignored. That’s reasonable, but it’s implicit.If you expect users might accidentally mix modes, consider rejecting that combination explicitly (e.g., print an error when both are set) or documenting that
--templatetakes precedence.cortex/templates.py (2)
436-459: Reuse the existingPackageManagerinstance ingenerate_commands.You already construct
self._package_managerin__init__, butgenerate_commandsinstantiates a newPackageManagereach time. To avoid redundant detection and configuration, you can reuse the existing instance:- elif template.packages: - # Use package manager to generate commands - pm = PackageManager() - package_list = " ".join(template.packages) + elif template.packages: + # Use shared package manager to generate commands + pm = self._package_manager + package_list = " ".join(template.packages)The rest of the logic (parse + fallback) can remain unchanged.
299-324: Silent swallowing of malformed built-in templates.In
list_templates, malformed built-in templates are caught withexcept Exception: pass. This keeps listing robust if one template is broken, but it also hides which template failed and why.If you’d like more visibility without breaking UX, consider logging these exceptions at a debug or warning level (or at least including the filename in a diagnostic message) instead of fully suppressing them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.py(7 hunks)cortex/templates.py(1 hunks)cortex/templates/lamp.yaml(1 hunks)cortex/templates/mean.yaml(1 hunks)cortex/templates/mern.yaml(1 hunks)cortex/templates/ml-ai.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cortex/templates/lamp.yaml
- cortex/templates/mean.yaml
- cortex/templates/mern.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/templates.py (11)
Template(54-148)TemplateFormat(25-28)InstallationStep(44-50)load_template(241-261)list_templates(295-346)check_hardware_compatibility(348-434)generate_commands(436-460)HardwareRequirements(32-40)save_template(263-293)import_template(462-497)export_template(499-525)cortex/coordinator.py (5)
InstallationStep(19-32)execute(202-249)InstallationCoordinator(44-300)from_plan(78-124)verify_installation(251-273)installation_history.py (6)
_extract_packages_from_commands(212-244)record_installation(252-308)InstallationType(25-31)update_installation(310-362)InstallationStatus(34-39)rollback(461-586)
cortex/templates.py (2)
src/hwprofiler.py (1)
profile(399-426)cortex/packages.py (2)
PackageManager(23-452)PackageManagerType(16-20)
🪛 Ruff (0.14.5)
cortex/cli.py
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
399-399: subprocess call with shell=True identified, security issue
(S602)
435-435: Consider moving this statement to an else block
(TRY300)
445-445: Use explicit conversion flag
Replace with conversion flag
(RUF010)
468-468: Consider moving this statement to an else block
(TRY300)
469-469: Do not catch blind exception: Exception
(BLE001)
470-470: Use explicit conversion flag
Replace with conversion flag
(RUF010)
530-530: Do not catch blind exception: Exception
(BLE001)
531-531: Use explicit conversion flag
Replace with conversion flag
(RUF010)
546-546: Consider moving this statement to an else block
(TRY300)
547-547: Do not catch blind exception: Exception
(BLE001)
548-548: Use explicit conversion flag
Replace with conversion flag
(RUF010)
560-560: Consider moving this statement to an else block
(TRY300)
561-561: Do not catch blind exception: Exception
(BLE001)
562-562: Use explicit conversion flag
Replace with conversion flag
(RUF010)
618-618: Local variable template_list_parser is assigned to but never used
Remove assignment to unused variable template_list_parser
(F841)
cortex/templates.py
1-1: Shebang is present but file is not executable
(EXE001)
154-154: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
155-155: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
259-259: Consider moving this statement to an else block
(TRY300)
260-260: Do not catch blind exception: Exception
(BLE001)
261-261: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Use explicit conversion flag
Replace with conversion flag
(RUF010)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
322-324: try-except-pass detected, consider logging the exception
(S110)
322-322: Do not catch blind exception: Exception
(BLE001)
343-344: try-except-pass detected, consider logging the exception
(S110)
343-343: Do not catch blind exception: Exception
(BLE001)
475-475: Avoid specifying long messages outside the exception class
(TRY003)
493-493: Abstract raise to an inner function
(TRY301)
493-493: Avoid specifying long messages outside the exception class
(TRY003)
495-495: Consider moving this statement to an else block
(TRY300)
496-496: Do not catch blind exception: Exception
(BLE001)
497-497: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
497-497: Avoid specifying long messages outside the exception class
(TRY003)
497-497: Use explicit conversion flag
Replace with conversion flag
(RUF010)
514-514: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
cortex/templates.py (1)
295-346:list_templatescorrectly differentiates built-in vs user templates now.Loading built-ins directly from their files instead of going through
load_template()fixes the earlier misclassification when a user template shadowed a built-in name. The “built-in” vs “user” typing and path reporting now correctly reflect which file backs each template.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cortex/cli.py (3)
444-448: Consider more granular error handling and explicit f-string conversion.The broad exception tuple on line 444 might mask unexpected errors. While reasonable for a CLI tool, consider logging the exception type to aid debugging.
Apply this diff for better observability:
- except (RuntimeError, OSError, subprocess.SubprocessError) as e: + except (RuntimeError, OSError, subprocess.SubprocessError) as e: + logger.debug(f"Exception type: {type(e).__name__}") if install_id: history.update_installation(install_id, InstallationStatus.FAILED, str(e)) - self._print_error(f"Unexpected error: {str(e)}") + self._print_error(f"Unexpected error: {e!s}") return 1The
!sconversion flag (line 447) makes the string conversion explicit, as suggested by static analysis.
471-472: Optional: Add logging to broad exception handlers for debugging.The template management methods catch broad exceptions (lines 471, 533, 549, 563) to provide user-friendly error messages. While appropriate for a CLI, consider adding debug logging to preserve stack traces for troubleshooting.
Example pattern for one method (apply similarly to others):
+import logging + +logger = logging.getLogger(__name__) + except Exception as e: + logger.exception("Template operation failed") # Logs full traceback at DEBUG level - self._print_error(f"Failed to list templates: {str(e)}") + self._print_error(f"Failed to list templates: {e!s}") return 1Also use explicit
!sconversion flags as suggested by static analysis (lines 472, 533, 550, 564).Also applies to: 532-533, 549-550, 563-564
116-168: Consider extracting shared installation execution logic.The installation execution flow in both
install(lines 116-168) and_install_from_template(lines 347-432) contains significant duplication:
- Coordinator creation and execution
- Result handling (success/failure paths)
- History recording
- Progress callbacks
- Error messages
Consider extracting the common logic into a helper method:
def _execute_installation( self, commands: List[str], descriptions: List[str], install_id: Optional[str], history: InstallationHistory, software_name: str, verification_commands: Optional[List[str]] = None, post_install_commands: Optional[List[str]] = None, coordinator_plan: Optional[List[Dict[str, str]]] = None ) -> int: """Common installation execution logic.""" def progress_callback(current, total, step): status_emoji = "⏳" if step.status == StepStatus.SUCCESS: status_emoji = "✅" elif step.status == StepStatus.FAILED: status_emoji = "❌" print(f"\n[{current}/{total}] {status_emoji} {step.description}") print(f" Command: {step.command}") # Create coordinator if coordinator_plan: coordinator = InstallationCoordinator.from_plan(coordinator_plan, timeout=300, stop_on_error=True) else: coordinator = InstallationCoordinator( commands=commands, descriptions=descriptions, timeout=300, stop_on_error=True, progress_callback=progress_callback ) print("\nExecuting commands...") result = coordinator.execute() if result.success: # Run verification if provided if verification_commands: self._print_status("[*]", "Verifying installation...") verify_results = coordinator.verify_installation(verification_commands) # ... verification logic # Run post-install if provided if post_install_commands: self._print_status("[*]", "Running post-installation steps...") # ... post-install logic self._print_success(f"{software_name} ready!") print(f"\nCompleted in {result.total_duration:.2f} seconds") if install_id: history.update_installation(install_id, InstallationStatus.SUCCESS) print(f"\n📝 Installation recorded (ID: {install_id})") print(f" To rollback: cortex rollback {install_id}") return 0 else: # Failure handling... if install_id: history.update_installation(install_id, InstallationStatus.FAILED, result.error_message or "Installation failed") # ... error messages return 1Then both methods can call this helper, reducing duplication by ~50 lines while maintaining readability.
Also applies to: 347-432
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/templates.py (11)
TemplateManager(201-525)Template(54-148)TemplateFormat(25-28)InstallationStep(44-50)load_template(241-261)list_templates(295-346)check_hardware_compatibility(348-434)generate_commands(436-460)HardwareRequirements(32-40)import_template(462-497)export_template(499-525)cortex/coordinator.py (6)
InstallationStep(19-32)execute(202-249)InstallationCoordinator(44-300)from_plan(78-124)StepStatus(10-15)verify_installation(251-273)installation_history.py (6)
_extract_packages_from_commands(212-244)record_installation(252-308)InstallationType(25-31)update_installation(310-362)InstallationStatus(34-39)rollback(461-586)
🪛 Ruff (0.14.5)
cortex/cli.py
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
401-401: subprocess call with shell=True identified, security issue
(S602)
437-437: Consider moving this statement to an else block
(TRY300)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
470-470: Consider moving this statement to an else block
(TRY300)
471-471: Do not catch blind exception: Exception
(BLE001)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
532-532: Do not catch blind exception: Exception
(BLE001)
533-533: Use explicit conversion flag
Replace with conversion flag
(RUF010)
548-548: Consider moving this statement to an else block
(TRY300)
549-549: Do not catch blind exception: Exception
(BLE001)
550-550: Use explicit conversion flag
Replace with conversion flag
(RUF010)
562-562: Consider moving this statement to an else block
(TRY300)
563-563: Do not catch blind exception: Exception
(BLE001)
564-564: Use explicit conversion flag
Replace with conversion flag
(RUF010)
620-620: Local variable template_list_parser is assigned to but never used
Remove assignment to unused variable template_list_parser
(F841)
🔇 Additional comments (3)
cortex/cli.py (3)
13-13: LGTM! Clean template system integration.The import of template entities and the routing logic to delegate template-based installations to
_install_from_templateare well-structured. The optionaltemplateparameter maintains backward compatibility with existing LLM-based installations.Also applies to: 59-68
298-314: Hardware prompt handling improved.The error message for non-interactive environments is now clear and actionable, addressing the previous review feedback. The prompt correctly aborts installation when confirmation cannot be obtained, and provides helpful guidance to users.
396-401: Post-install commands now execute correctly.The duplicate execution issue from the previous review has been resolved. Post-install commands now run exactly once, eliminating redundant output and potential side effects.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cortex/cli.py (1)
652-656: Consider makingsoftwareparameter optional ininstall()method.The routing passes an empty string
""forsoftwarewhen using--template, which is awkward. Since the method already handles templates via a separate parameter, consider makingsoftwareoptional:- def install(self, software: str, execute: bool = False, dry_run: bool = False, template: Optional[str] = None): + def install(self, software: Optional[str] = None, execute: bool = False, dry_run: bool = False, template: Optional[str] = None):Then update the routing:
if args.command == 'install': if args.template: - return cli.install("", execute=args.execute, dry_run=args.dry_run, template=args.template) + return cli.install(execute=args.execute, dry_run=args.dry_run, template=args.template) else: # software is guaranteed to be set due to mutually_exclusive_group(required=True) return cli.install(args.software, execute=args.execute, dry_run=args.dry_run)This makes the API clearer and eliminates the placeholder empty string.
cortex/templates.py (3)
154-163: Annotate class-level constants withClassVarfor type safety.Static analysis correctly identifies that class attributes
REQUIRED_FIELDS,REQUIRED_STEP_FIELDS,ALLOWED_POST_INSTALL_COMMANDS, andDANGEROUS_SHELL_CHARSshould be annotated withtyping.ClassVarto indicate they are class-level constants, not instance attributes.+from typing import ClassVar + class TemplateValidator: """Validates template structure and content.""" - REQUIRED_FIELDS = ["name", "description", "version"] - REQUIRED_STEP_FIELDS = ["command", "description"] + REQUIRED_FIELDS: ClassVar[List[str]] = ["name", "description", "version"] + REQUIRED_STEP_FIELDS: ClassVar[List[str]] = ["command", "description"] # Allowed post_install commands (whitelist for security) - ALLOWED_POST_INSTALL_COMMANDS = { + ALLOWED_POST_INSTALL_COMMANDS: ClassVar[Set[str]] = { 'echo', # Safe echo commands } # Dangerous shell metacharacters that should be rejected - DANGEROUS_SHELL_CHARS = [';', '|', '&', '>', '<', '`', '\\'] + DANGEROUS_SHELL_CHARS: ClassVar[List[str]] = [';', '|', '&', '>', '<', '`', '\\']
321-323: Chain exception for better error diagnostics.Use
raise ... from eto preserve the original exception's traceback, which aids debugging when template loading fails.except Exception as e: - raise ValueError(f"Failed to load template {name}: {str(e)}") + raise ValueError(f"Failed to load template {name}: {str(e)}") from e
558-559: Chain exception for better error diagnostics.Use
raise ... from eto preserve the original exception's traceback when template import fails.except Exception as e: - raise ValueError(f"Failed to import template: {str(e)}") + raise ValueError(f"Failed to import template: {str(e)}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(7 hunks)cortex/templates.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/templates.py (2)
src/hwprofiler.py (1)
profile(399-426)cortex/packages.py (2)
PackageManager(23-452)PackageManagerType(16-20)
cortex/cli.py (1)
cortex/templates.py (12)
TemplateManager(263-587)Template(54-148)TemplateFormat(25-28)InstallationStep(44-50)load_template(303-323)list_templates(357-408)check_hardware_compatibility(410-496)generate_commands(498-522)HardwareRequirements(32-40)save_template(325-355)import_template(524-559)export_template(561-587)
🪛 Ruff (0.14.5)
cortex/templates.py
1-1: Shebang is present but file is not executable
(EXE001)
154-154: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
155-155: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
158-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
163-163: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
321-321: Consider moving this statement to an else block
(TRY300)
322-322: Do not catch blind exception: Exception
(BLE001)
323-323: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Use explicit conversion flag
Replace with conversion flag
(RUF010)
341-341: Avoid specifying long messages outside the exception class
(TRY003)
384-386: try-except-pass detected, consider logging the exception
(S110)
384-384: Do not catch blind exception: Exception
(BLE001)
405-406: try-except-pass detected, consider logging the exception
(S110)
405-405: Do not catch blind exception: Exception
(BLE001)
537-537: Avoid specifying long messages outside the exception class
(TRY003)
555-555: Abstract raise to an inner function
(TRY301)
555-555: Avoid specifying long messages outside the exception class
(TRY003)
557-557: Consider moving this statement to an else block
(TRY300)
558-558: Do not catch blind exception: Exception
(BLE001)
559-559: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
559-559: Avoid specifying long messages outside the exception class
(TRY003)
559-559: Use explicit conversion flag
Replace with conversion flag
(RUF010)
576-576: Avoid specifying long messages outside the exception class
(TRY003)
cortex/cli.py
294-294: f-string without any placeholders
Remove extraneous f prefix
(F541)
401-401: subprocess call with shell=True identified, security issue
(S602)
437-437: Consider moving this statement to an else block
(TRY300)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
470-470: Consider moving this statement to an else block
(TRY300)
471-471: Do not catch blind exception: Exception
(BLE001)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
536-536: Do not catch blind exception: Exception
(BLE001)
537-537: Use explicit conversion flag
Replace with conversion flag
(RUF010)
552-552: Consider moving this statement to an else block
(TRY300)
553-553: Do not catch blind exception: Exception
(BLE001)
554-554: Use explicit conversion flag
Replace with conversion flag
(RUF010)
566-566: Consider moving this statement to an else block
(TRY300)
567-567: Do not catch blind exception: Exception
(BLE001)
568-568: Use explicit conversion flag
Replace with conversion flag
(RUF010)
| self._print_status("[*]", "Running post-installation steps...") | ||
| print("\n[*] Post-installation information:") | ||
| for cmd in template.post_install: | ||
| subprocess.run(cmd, shell=True) |
There was a problem hiding this comment.
Post-install command validation is insufficient for shell injection protection.
While TemplateValidator._validate_post_install_commands now restricts commands to echo only, it explicitly allows command substitution patterns $(...) within echo commands (templates.py lines 191-196). This remains exploitable—a malicious template can execute arbitrary code via echo $(rm -rf /) or similar constructs, since the shell evaluates the substitution before echo receives it.
Recommended fix: Either:
- Block all command substitution by removing the special case for
echoat templates.py lines 191-196, OR - Avoid
shell=Trueentirely by parsing and printing echo messages directly in Python:
if template.post_install:
self._print_status("[*]", "Running post-installation steps...")
print("\n[*] Post-installation information:")
for cmd in template.post_install:
- subprocess.run(cmd, shell=True)
+ # Parse and print echo messages safely
+ if cmd.strip().startswith('echo '):
+ message = cmd.strip()[5:] # Remove 'echo ' prefix
+ # Strip quotes if present
+ message = message.strip('\'"')
+ print(f" {message}")
+ else:
+ print(f" [WARNING] Skipped non-echo command: {cmd[:50]}")This eliminates subprocess execution entirely for post-install informational messages.
🧰 Tools
🪛 Ruff (0.14.5)
401-401: subprocess call with shell=True identified, security issue
(S602)
🤖 Prompt for AI Agents
In cortex/cli.py around line 401, subprocess.run(cmd, shell=True) is used to
execute post-install commands which, combined with templates.py allowing echo
with $(...) substitution, permits shell injection; replace this by not invoking
the shell for informational echo commands: parse the post-install command list
and if the command is an echo (and only echo), extract and print the literal
message safely in Python (no shell), and for any non-allowed commands either
reject them during validation or, if you must run external commands, use
subprocess.run with a list of args and shell=False after strict validation;
alternatively remove the special-case allowance for echo in TemplateValidator so
all commands are blocked and subprocess.run is never called for untrusted
templates.
| if name not in templates: # Don't override built-in | ||
| try: | ||
| template = self.load_template(name) | ||
| if template: | ||
| templates[name] = { | ||
| "name": template.name, | ||
| "description": template.description, | ||
| "version": template.version, | ||
| "author": template.author, | ||
| "type": "user", | ||
| "path": str(template_file) | ||
| } | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
User templates that shadow built-ins are hidden in listings.
When a user creates a template with the same name as a built-in (e.g., ~/.cortex/templates/lamp.yaml), list_templates skips the user version (line 393) and only shows the built-in metadata. However, load_template (via _get_template_path) prefers user templates, so cortex install --template lamp will use the user's version while cortex template list displays the built-in's description and version.
This inconsistency can confuse users who edit or override built-in templates.
Suggested fix: Display both versions when shadowing occurs, or indicate which template is active:
for template_file in self.user_templates_dir.glob(f"*{ext}"):
name = template_file.stem
- if name not in templates: # Don't override built-in
- try:
- template = self.load_template(name)
- if template:
- templates[name] = {
- "name": template.name,
- "description": template.description,
- "version": template.version,
- "author": template.author,
- "type": "user",
- "path": str(template_file)
- }
- except Exception:
- pass
+ try:
+ template = self.load_template(name)
+ if template:
+ # Override built-in or add new user template
+ template_type = "user (overrides built-in)" if name in templates else "user"
+ templates[name] = {
+ "name": template.name,
+ "description": template.description,
+ "version": template.version,
+ "author": template.author,
+ "type": template_type,
+ "path": str(template_file)
+ }
+ except Exception:
+ passThis makes it clear which template will actually be used during installation.
🧰 Tools
🪛 Ruff (0.14.5)
405-406: try-except-pass detected, consider logging the exception
(S110)
405-405: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In cortex/templates.py around lines 393 to 406, the listing logic currently
skips user templates when a built-in with the same name exists which leads to
inconsistent behavior (install uses the user template but list shows the
built-in). Change the loop so that you do not silently skip user templates: if a
built-in entry exists for the same name, add a separate entry for the user
template (or augment the existing entry to include both variants) and mark which
one is active based on the same resolution used by
load_template/_get_template_path (e.g., add fields type="builtin" or "user" and
active=true for the path that load_template would return). Ensure the listing
returns both entries or a single entry with both variants and an explicit active
flag and path so users can see which template will be used.
|
Hi @aliraza556! This PR has merge conflicts with the latest main branch. Could you please rebase your branch? git fetch origin
git rebase origin/main
# resolve conflicts
git push --force-with-leaseThe template system looks excellent - ready to merge once conflicts are resolved! |
|
Please rebase onto main to resolve conflicts: |
|
@mikejmorgan-ai Sure |
|
@mikejmorgan-ai Done Please review and merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cortex/cli.py (1)
297-408: Fix minor f-string lint and hardenpost_installexecution against shell injectionTwo separate points in this block:
- Extraneous f-string (Ruff F541)
Line [300] usesprint(f"\n Packages:")with no interpolation. This triggers F541 and is trivially fixed:- print(f"\n Packages:") + print("\n Packages:")
subprocess.run(..., shell=True)on template-suppliedpost_installcommands remains unsafe
Lines [402–407] execute everytemplate.post_installentry viasubprocess.run(cmd, shell=True). Even with validator constraints, this is effectively running untrusted template content through a shell and is still vulnerable to injection (e.g., viaecho $(...)patterns allowed by the validator, as noted in a prior review).For post‑install informational messages, you can avoid invoking a shell entirely by parsing and printing the echo content in Python instead:
- # Run post-install commands once - if template.post_install: - self._print_status("[*]", "Running post-installation steps...") - print("\n[*] Post-installation information:") - for cmd in template.post_install: - subprocess.run(cmd, shell=True) + # Render post-install info without spawning a shell + if template.post_install: + self._print_status("[*]", "Running post-installation steps...") + print("\n[*] Post-installation information:") + for cmd in template.post_install: + stripped = cmd.strip() + if stripped.startswith("echo "): + # Remove leading "echo" and optional surrounding quotes + message = stripped[5:].strip() + if len(message) >= 2 and message[0] == message[-1] and message[0] in ("'", '"'): + message = message[1:-1] + print(f" {message}") + else: + print(f" [WARNING] Skipping non-echo post-install command: {cmd[:80]}")If you truly need to support non-echo commands, they should be validated much more strictly and executed with
shell=Falseusing argument lists rather than passing raw strings to the shell.#!/bin/bash # Verify remaining uses of shell=True on template-driven commands rg -n "post_install" -n cortex rg -n "shell=True" cortex
🧹 Nitpick comments (5)
cortex/cli.py (5)
65-82: Template routing ininstall()is functionally correct but adds to an already complex methodThe early
if template: return self._install_from_template(...)branch correctly preserves existing LLM-based behavior and routes template installs through their own workflow.However, Sonar is already flagging
install()for high cognitive complexity, and this change nudges it further. Consider extracting the LLM-based path into a dedicated helper (e.g.,_install_via_llm(...)) and keepinginstall()as a thin router between template and non-template flows. This would also let you avoid initializingInstallationHistoryandstart_timewhen the template branch is taken but never uses them.
322-444: Template installation flow is sound but very large; consider breaking into smaller helpersThe overall control flow in
_install_from_template()(load template → show metadata → hardware check with prompt → generate commands → history record → dry‑run/execute → verification → post‑install → history update) looks correct and user‑friendly.That said, Sonar flags this method for very high cognitive complexity. It’s now handling:
- Hardware compatibility prompting and non-interactive aborts
- Command generation and display
- Two execution strategies (template steps plan vs. raw commands)
- Verification reporting
- History bookkeeping and error handling
To make this maintainable, consider splitting into focused helpers, e.g.:
_prompt_hardware_acceptance(template, is_compatible, warnings, dry_run)_build_coordinator_for_template(template, commands)_run_template_verification(coordinator, template)_record_and_print_result(history, install_id, result, template)This would also let you share pieces (e.g., progress callbacks and history updates) with the LLM-based
install()path.
456-479:template_list()behavior looks good; keep broad exception catching minimalListing and formatting of templates (name/version/type/description, capped width) is straightforward and matches expectations. The broad
except Exception as ewithself._print_error(f"Failed to list templates: {str(e)}")is acceptable for a CLI surface, but if you find yourself duplicating this pattern across multiple template helpers, it may be worth centralizing into a small_handle_cli_error(context: str, exc: Exception)to reduce repetition and complexity.Also, to satisfy Ruff RUF010 you can simplify to
f"{e}"instead off"{str(e)}".
481-545: Interactivetemplate_create()flow is user-friendly; complexity is creeping upThe interactive creation flow (description/version/author → packages loop → optional hardware requirements → save) is clear and matches the PR’s goals. Numeric validation for hardware requirements with a targeted
ValueErrorhandler is a nice improvement.Sonar, however, flags this method for high cognitive complexity. To keep it manageable, consider extracting small helpers, for example:
_prompt_template_metadata(name) -> (description, version, author)_prompt_packages() -> List[str]_prompt_hardware_requirements() -> Optional[HardwareRequirements]This would also make it easier to add a non-interactive path in the future without overloading this single method.
546-575: Import/export helpers look correct and straightforward
template_import()andtemplate_export()are thin wrappers overTemplateManager, correctly wiring up file paths, names, formats, and user messaging. The behavior matches the documented CLI (cortex template import .../export ...).Only minor nit (optional): you could again simplify
str(e)inside the f-strings to just{e}or{e!s}to appease Ruff’s RUF010 and keep exception formatting consistent across methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(7 hunks)src/requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/requirements.txt
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 65-65: Refactor this function to reduce its Cognitive Complexity from 45 to the 15 allowed.
[failure] 481-481: Refactor this function to reduce its Cognitive Complexity from 27 to the 15 allowed.
[warning] 300-300: Add replacement fields or use a normal string instead of an f-string.
[failure] 277-277: Refactor this function to reduce its Cognitive Complexity from 85 to the 15 allowed.
🪛 Ruff (0.14.6)
cortex/cli.py
300-300: f-string without any placeholders
Remove extraneous f prefix
(F541)
407-407: subprocess call with shell=True identified, security issue
(S602)
443-443: Consider moving this statement to an else block
(TRY300)
453-453: Use explicit conversion flag
Replace with conversion flag
(RUF010)
476-476: Consider moving this statement to an else block
(TRY300)
477-477: Do not catch blind exception: Exception
(BLE001)
478-478: Use explicit conversion flag
Replace with conversion flag
(RUF010)
542-542: Do not catch blind exception: Exception
(BLE001)
543-543: Use explicit conversion flag
Replace with conversion flag
(RUF010)
558-558: Consider moving this statement to an else block
(TRY300)
559-559: Do not catch blind exception: Exception
(BLE001)
560-560: Use explicit conversion flag
Replace with conversion flag
(RUF010)
572-572: Consider moving this statement to an else block
(TRY300)
573-573: Do not catch blind exception: Exception
(BLE001)
574-574: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
cortex/cli.py (2)
807-811: Updated CLI examples correctly showcase the new template workflowThe additional examples in the help epilog for
cortex install --template lamp --executeand thecortex templatesubcommands are accurate and make the new functionality discoverable from--help. No issues here.
849-915: Template subcommand wiring and dispatch look correctThe
templatesubparser plus itslist/create/import/exportsubcommands are wired correctly, and the dispatch block underelif args.command == 'template':covers all four actions with a sensible fallback totemplate_parser.print_help().UX-wise,
cortex templatewith no action shows help and exits with status 1, which is consistent with the rest of the CLI.
|
@dhvll Could you review this PR? The templates system relates to your package manager work. Thanks! |
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1-17: Missingsubprocessimport causesNameErrorat runtime.Line 514 uses
subprocess.run(cmd, shell=True)but thesubprocessmodule is never imported. This will crash when executing post-install commands.Apply this diff to add the missing import:
import sys import os import argparse import time import logging +import subprocess from typing import List, Optional from datetime import datetime
♻️ Duplicate comments (1)
cortex/cli.py (1)
509-514: Security:shell=Truewith template-sourced commands enables command injection.The
post_installcommands from templates are executed withshell=True, which can execute arbitrary shell code. Even if templates restrict toecho, command substitution likeecho $(malicious_cmd)will execute.Parse and print echo messages directly in Python to eliminate shell execution:
# Run post-install commands once if template.post_install: self._print_status("[*]", "Running post-installation steps...") print("\n[*] Post-installation information:") for cmd in template.post_install: - subprocess.run(cmd, shell=True) + # Safely extract and print echo messages without shell + cmd_stripped = cmd.strip() + if cmd_stripped.startswith('echo '): + message = cmd_stripped[5:].strip('\'"') + print(f" {message}") + else: + print(f" [WARNING] Skipped non-echo post-install command")
🧹 Nitpick comments (3)
cortex/cli.py (3)
186-207: Redundant API key retrieval.When
templateisNone, lines 186-192 already fetch and validateapi_keyandprovider, but lines 204-207 fetch them again unconditionally after the template check.Remove the duplicate fetch since validation already occurred:
# If template is specified, use template system if template: return self._install_from_template(template, execute, dry_run) # Otherwise, use LLM-based installation - api_key = self._get_api_key() - if not api_key: - return 1 - provider = self._get_provider() + # api_key and provider already fetched above (lines 186-191) self._print_status("🧠", "Understanding request...")
384-561: Consider splitting this method to reduce complexity.SonarCloud flags Cognitive Complexity of 85 (limit: 15). This method handles template loading, hardware checks, command generation, execution, verification, and post-install steps.
Extract focused helper methods:
def _install_from_template(self, template_name: str, execute: bool, dry_run: bool): """Install from a template.""" template = self._load_and_validate_template(template_name) if not template: return 1 if not self._check_template_hardware(template, dry_run): return 1 commands = self._prepare_template_commands(template) if not commands: return 1 return self._execute_template_install(template, commands, execute, dry_run)This improves testability and maintainability.
611-618: Remove redundant local import.
Templateis already imported at line 17.HardwareRequirementsshould be added to the top-level import instead of importing locally.Update the top-level import and remove the local one:
-from cortex.templates import TemplateManager, Template, TemplateFormat, InstallationStep +from cortex.templates import TemplateManager, Template, TemplateFormat, InstallationStep, HardwareRequirementsThen remove line 611:
# Create template - from cortex.templates import Template, HardwareRequirements template = Template(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(7 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cortex/cli.py (4)
cortex/templates.py (12)
TemplateManager(263-587)Template(54-148)TemplateFormat(25-28)InstallationStep(44-50)load_template(303-323)list_templates(357-408)check_hardware_compatibility(410-496)generate_commands(498-522)HardwareRequirements(32-40)save_template(325-355)import_template(524-559)export_template(561-587)cortex/coordinator.py (6)
InstallationStep(40-53)execute(252-299)InstallationCoordinator(65-350)from_plan(99-145)StepStatus(31-36)verify_installation(301-323)cortex/validators.py (1)
validate_install_request(72-99)cortex/installation_history.py (6)
InstallationHistory(68-653)_extract_packages_from_commands(212-244)record_installation(252-308)InstallationType(25-31)update_installation(310-362)InstallationStatus(34-39)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 178-178: Refactor this function to reduce its Cognitive Complexity from 50 to the 15 allowed.
[failure] 588-588: Refactor this function to reduce its Cognitive Complexity from 27 to the 15 allowed.
[failure] 384-384: Refactor this function to reduce its Cognitive Complexity from 85 to the 15 allowed.
[warning] 407-407: Add replacement fields or use a normal string instead of an f-string.
🔇 Additional comments (3)
requirements.txt (1)
7-8: LGTM!The PyYAML dependency is appropriately added to support the new template system. Version 6.0+ is a reasonable baseline.
cortex/cli.py (2)
847-871: LGTM!The updated epilog with template command examples and environment variable documentation provides good user guidance.
890-895: The argparse configuration is valid—no issue here.The mutually exclusive group with
required=Trueand a positional argument usingnargs='?'is a supported pattern in Python's argparse. The positional argument becomes optional (can beNone), and argparse ensures that exactly one ofsoftwareor--templateis provided. The code at line 956–958 correctly handles both cases: checkingargs.templatefirst, then safely accessingargs.softwarewhen the template is not provided.
| elif args.command == 'template': | ||
| if args.template_action == 'list': | ||
| return cli.template_list() | ||
| elif args.template_action == 'create': | ||
| return cli.template_create(args.name) | ||
| elif args.template_action == 'import': | ||
| return cli.template_import(args.file_path, args.name) | ||
| elif args.template_action == 'export': | ||
| return cli.template_export(args.name, args.file_path, args.format) | ||
| else: | ||
| template_parser.print_help() | ||
| return 1 |
There was a problem hiding this comment.
Missing template subparser definition causes AttributeError at runtime.
The code handles args.command == 'template' and references args.template_action, args.name, args.file_path, args.format, and template_parser, but the template subparser was never defined in the argparse setup (lines 878-934).
Running cortex template list will fail with AttributeError: 'Namespace' object has no attribute 'template_action'.
Add the missing template subparser after line 906:
# Template command
template_parser = subparsers.add_parser('template', help='Manage installation templates')
template_subs = template_parser.add_subparsers(dest='template_action', help='Template actions')
template_subs.add_parser('list', help='List all available templates')
template_create_parser = template_subs.add_parser('create', help='Create a new template')
template_create_parser.add_argument('name', help='Template name')
template_import_parser = template_subs.add_parser('import', help='Import a template from file')
template_import_parser.add_argument('file_path', help='Path to template file')
template_import_parser.add_argument('--name', help='Optional name for imported template')
template_export_parser = template_subs.add_parser('export', help='Export a template to file')
template_export_parser.add_argument('name', help='Template name to export')
template_export_parser.add_argument('file_path', help='Output file path')
template_export_parser.add_argument('--format', choices=['yaml', 'json'], default='yaml', help='Output format')🤖 Prompt for AI Agents
In cortex/cli.py around lines 878–972 (add the new subparser after line 906),
the CLI references args.template_action, args.name, args.file_path, args.format
and template_parser but no 'template' subparser was defined, causing
AttributeError at runtime; add a 'template' top-level subparser named "template"
with its own subparsers (dest='template_action') and define the actions: 'list'
(no args), 'create' (positional name), 'import' (positional file_path and
optional --name), and 'export' (positional name, positional file_path, and
optional --format with choices ['yaml','json'] and default 'yaml'), so the code
paths in the handler can safely access those args and call the corresponding
cli.template_* methods.
🛠️ Refactor suggestion | 🟠 Major
Add graceful handling for missing template subcommand.
When user runs cortex template without a subcommand, args.template_action will be None, falling through to line 971 which tries to call template_parser.print_help() on an undefined variable.
Handle the no-subcommand case explicitly:
elif args.command == 'template':
+ if not args.template_action:
+ # template_parser needs to be defined in argparse setup
+ print("Please specify a template action: list, create, import, export")
+ return 1
if args.template_action == 'list':
return cli.template_list()
elif args.template_action == 'create':
return cli.template_create(args.name)
elif args.template_action == 'import':
return cli.template_import(args.file_path, args.name)
elif args.template_action == 'export':
return cli.template_export(args.name, args.file_path, args.format)
- else:
- template_parser.print_help()
- return 1🤖 Prompt for AI Agents
In cortex/cli.py around lines 961-972, args.template_action can be None when the
user runs "cortex template" with no subcommand, causing a fallback that
references template_parser which may be undefined; explicitly check for a
missing subcommand (if args.template_action is None) and in that branch call the
correct parser's print_help() (ensure you use the parser variable that is in
scope, e.g., the template subparser or the main parser if template_parser is not
defined), then return a non-zero exit code (e.g., return 1) to gracefully handle
the no-subcommand case.
2 participants
Overview
This PR implements a comprehensive installation template system that allows users to install pre-configured development stacks (LAMP, MEAN, MERN, ML/AI, DevOps) with a single command. The system supports template creation, validation, hardware compatibility checking, and sharing.
Features Implemented
Core Template System
Built-in Templates (5+)
Template Management
Hardware Compatibility
CLI Commands Added
Example Usage
Files Changed
New Files
cortex/templates.py- Core template system (519 lines)cortex/templates/lamp.yaml- LAMP stack templatecortex/templates/mean.yaml- MEAN stack templatecortex/templates/mern.yaml- MERN stack templatecortex/templates/ml-ai.yaml- ML/AI stack templatecortex/templates/devops.yaml- DevOps stack templatecortex/templates/README.md- Template directory documentationtest/test_templates.py- Comprehensive unit tests (452 lines)docs/TEMPLATES.md- Complete template documentation (571 lines)Modified Files
cortex/cli.py- Added template commands and integrationsrc/requirements.txt- Added PyYAML dependencyTesting
Documentation
Acceptance Criteria Met
Dependencies
pyyaml>=6.0.0to requirements🎯 Related Issues
Closes #44
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.