feat: implement system cloning and templating #657
Conversation
pratyush07-hub
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds System Cloning and Templating: a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as cortex/cli.py
participant ConfigMgr as ConfigManager
participant TemplateMgr as TemplateManager
participant FS as FileSystem
rect rgba(100, 150, 255, 0.5)
Note over User,FS: Template Creation Flow
User->>CLI: cortex template create my-template
CLI->>ConfigMgr: export_configuration()
ConfigMgr->>ConfigMgr: detect_installed_packages()\n+ detect_services()
ConfigMgr-->>CLI: config_state
CLI->>TemplateMgr: create_template(name, config_state, description)
TemplateMgr->>FS: mkdir ~/.cortex/templates/my-template/v1\nwrite template.yaml\nwrite metadata.json
TemplateMgr-->>CLI: template_path
CLI-->>User: Template saved: my-template-v1
end
rect rgba(150, 200, 100, 0.5)
Note over User,FS: Template Deployment Flow
User->>CLI: cortex template deploy my-template v1 --dry-run
CLI->>TemplateMgr: get_template(name, v1)
TemplateMgr->>FS: read template.yaml
TemplateMgr-->>CLI: template_config
CLI->>ConfigMgr: diff_configuration(template_config, current_state)
ConfigMgr-->>CLI: diff_result (packages, services, configs)
CLI->>ConfigMgr: import_configuration(diff_result, package_sources)
ConfigMgr->>ConfigMgr: _import_services()\n_update_packages()
ConfigMgr-->>CLI: deployment_summary
CLI-->>User: Deployment summary / success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @pratyush07-hub, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Cortex by introducing robust system cloning and templating capabilities. It allows users to define and manage system configurations as reusable, versioned templates, facilitating consistent deployments, disaster recovery, and streamlined team onboarding. The implementation focuses on capturing a comprehensive system state, including installed packages, user preferences, and systemd service states, and provides tools for differential updates and template portability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature for system cloning and templating. The implementation is comprehensive, covering the entire lifecycle of templates from creation to deployment, including versioning and import/export capabilities. The extension of ConfigManager to include systemd services is a thoughtful addition. The code is generally well-structured, and the inclusion of documentation and extensive unit tests is highly commendable.
My review highlights a few areas for improvement, primarily focusing on performance optimizations, enhancing robustness in the CLI, and improving maintainability. I've also identified a potentially unimplemented command-line argument. Addressing these points will further strengthen this already solid feature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 1975-1976: The public CLI handler method template lacks type
annotations; update the method signature for template to accept args:
argparse.Namespace and return -> int (i.e., def template(self, args:
argparse.Namespace) -> int) and ensure any return values inside template conform
to int so the function's declared return type matches actual returns; refer to
the template method in cortex/cli.py when making this change.
- Around line 2072-2076: The export command currently forces "v1" when version
is omitted (manager.export_template(name, version or "v1", args.file)); change
it to resolve the latest version instead of defaulting to v1: if version is
falsy, call the manager method that returns the latest template version (e.g.,
manager.get_latest_version(name) or manager.get_latest_template_version(name))
and pass that value into manager.export_template(name, resolved_version,
args.file); if no latest version is found, handle the error/print a message
instead of silently using "v1".
- Around line 3445-3449: The deploy subparser currently exposes --dry-run as a
flag (deploy_p) but defaults to executing changes; change the CLI so template
deploys are dry-run by default by replacing the parser flag with an explicit
execute flag (e.g., add_argument("--execute", action="store_true", help="Perform
changes; defaults to dry-run")) and update the deploy command handler to compute
dry_run = not args.execute (or check args.execute instead of args.dry_run) so
all installations/service changes are previewed unless --execute is passed;
update any references to args.dry_run in the handler to use the new logic.
- Around line 1983-1986: The usage branch for the "cortex template" command
currently calls self.console.print which doesn't exist and raises
AttributeError; replace that call with click.echo("[yellow]Usage: cortex
template [create|deploy|list|show|delete|export|import][/yellow]") (and add an
import for click if missing) so the usage prints reliably when no subcommand is
provided, leaving the rest of the handler unchanged.
- Around line 2108-2120: The current truthiness check on the dict returned by
config_manager.import_configuration causes success messages to print even when
imports failed; change the logic to inspect the returned summary dict (result)
explicitly—check for a success boolean (e.g., result.get("success") is True) or
absence of failure indicators (e.g., no result.get("errors") and no
result.get("failed") entries) before printing the success console messages and
returning 0; if the summary indicates errors, print an appropriate failure
message (including relevant result details) and return 1. Ensure you update the
block around config_manager.import_configuration(...) and the subsequent
console.print/return behavior to use this explicit success check.
In `@cortex/config_manager.py`:
- Around line 852-869: The _update_service function calls subprocess.run for
systemctl commands without a timeout which can hang; update these calls to pass
the existing timeout constant (e.g., CMD_TIMEOUT or whatever timeout constant is
used elsewhere in the module) and wrap each subprocess.run in try/except to
catch subprocess.TimeoutExpired, setting success = False when the timeout occurs
(and optionally logging the timeout). Ensure both the "enable" and "start"
branches use the timeout and handle TimeoutExpired to return failure.
In `@cortex/template_manager.py`:
- Around line 48-53: The code that computes the next version (collecting
versions = [d.name ...] and next_v = max(int(v[1:])...) ) fails for names like
"v1-import-..." because int(v[1:]) raises ValueError; update the logic in the
version-parsing locations (the block around template_dir/versions and the
similar blocks at the other mentioned spots) to extract only the leading digits
after the "v" (e.g. use a regex or parse until the first non-digit) and convert
that capture to int, ignoring non-matching names; then compute next_v =
max(found_ints)+1 (default 1 if none). Ensure you update all occurrences
referenced (the block using template_dir/versions and the similar logic at the
other two locations).
- Around line 199-224: The import currently uses zipfile.extractall() and trusts
metadata["name"]/["version"], allowing Zip Slip and path traversal; replace
extractall by iterating zipf.namelist()/infolist(), reject any member with
absolute paths or any '..' segments, compute the destination as Path(tmpdir) /
member and ensure dest.resolve().is_relative_to(Path(tmpdir).resolve()) (or
equivalent check) before writing, and write files safely creating parent dirs;
after loading metadata, sanitize/validate metadata["name"] and
metadata["version"] (e.g., allow only alphanumerics, dots, dashes or fail) and
ensure target_path = (self.base_dir / name / version).resolve() is under
self.base_dir.resolve() before creating dirs or copying files, otherwise raise
an error.
In `@README.md`:
- Line 197: Update the README table row for the "cortex template <cmd>" CLI
entry so it lists all supported subcommands; change the actions from "create,
deploy, list, show" to "create, deploy, list, show, delete, export, import"
(i.e., edit the text for the "cortex template <cmd>" table cell to include
delete/export/import).
🧹 Nitpick comments (5)
tests/unit/test_config_manager_services.py (1)
17-19: Isolate ConfigManager filesystem side effects in tests.
ConfigManager()writes to~/.cortex; patchPath.home()to a temp dir to avoid polluting developer machines or CI.🧪 Suggested fix
def setUp(self): - self.config_manager = ConfigManager() + import tempfile + + self._tmp_home = tempfile.TemporaryDirectory() + self._home_patch = patch("pathlib.Path.home", return_value=Path(self._tmp_home.name)) + self._home_patch.start() + self.addCleanup(self._home_patch.stop) + self.addCleanup(self._tmp_home.cleanup) + + self.config_manager = ConfigManager()tests/unit/test_template_manager.py (4)
6-6: Unused import:jsonThe
jsonmodule is imported but never used in this test file. All JSON operations are handled internally byTemplateManager.Suggested fix
-import json
29-40: Consider verifying mock interactions.The test correctly verifies versioning behavior. However, it doesn't verify that
export_configurationwas actually called. Addingmock_export.assert_called()would strengthen the test by confirming the expected interaction withConfigManager.Suggested enhancement
self.assertTrue((self.temp_dir / "test-template" / "v1" / "metadata.json").exists()) self.assertTrue((self.temp_dir / "test-template" / "v2" / "metadata.json").exists()) + self.assertEqual(mock_export.call_count, 2)
79-86: Weak assertion for imported version.The comment states the imported version should be new since
v1exists, butself.assertIn("v1", new_v)only verifies that "v1" is a substring. Based onimport_templatebehavior, whenv1exists, the new version should bev1-import-<timestamp>. Consider a more precise assertion.Suggested fix
# Import as new template new_name, new_v = self.template_manager.import_template(str(zip_path)) self.assertEqual(new_name, name) - # It'll be a new version since v1 exists - self.assertIn("v1", new_v) + # Since v1 exists, import should create versioned name with import suffix + self.assertTrue(new_v.startswith("v1-import-"), f"Expected 'v1-import-*' pattern, got '{new_v}'")
21-148: Consider additional test coverage.The test suite provides good coverage of core functionality. A few gaps to consider:
create_templatewithpackage_sourcesparameter: The optionalpackage_sourcesargument is never tested.delete_templatefor entire template: Line 122 tests deleting a nonexistent template, but there's no test for deleting an existing template without specifying a version (i.e., deleting all versions at once).Suggested additional tests
`@patch`("cortex.config_manager.ConfigManager.export_configuration") def test_create_template_with_package_sources(self, mock_export): """Test create_template with explicit package_sources.""" mock_export.return_value = "Success" v = self.template_manager.create_template( "source-test", "desc", package_sources=["apt", "snap"] ) self.assertEqual(v, "v1") # Verify package_sources was passed to export_configuration mock_export.assert_called_once() call_args = mock_export.call_args # Check that package_sources was passed (implementation dependent) `@patch`("cortex.config_manager.ConfigManager.export_configuration") def test_delete_entire_template(self, mock_export): """Test deleting an entire template with all versions.""" name = "full-delete-test" self.template_manager.create_template(name, "v1") self.template_manager.create_template(name, "v2") # Delete entire template (no version specified) self.assertTrue(self.template_manager.delete_template(name)) self.assertFalse((self.temp_dir / name).exists())
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2055-2063: The _parse_template_spec function incorrectly treats
any occurrence of "-v" as a version separator (breaking names like "dev-tools");
change the logic to only split on the last "-v" when it is immediately followed
by a digit (e.g., match "-v" + digit). Specifically, in _parse_template_spec
check for "-v" presence with name_spec.rfind("-v") then verify that there is a
character after the "-v" and that that character is a digit before slicing; when
valid return name_spec[:idx] and the version as name_spec[idx+2:], otherwise
fall through to return the whole spec as the name and None for version.
In `@cortex/config_manager.py`:
- Around line 895-905: The service enable/start subprocess.run calls in the
block that checks srv.get("enabled") and srv.get("active_state") use
self.DETECTION_TIMEOUT and omit text mode; change those runs to use
self.INSTALLATION_TIMEOUT and add text=True to subprocess.run so long-running
enable/start operations don't falsely fail and stdout/stderr are returned as
text (update the two subprocess.run invocations around the "systemctl enable"
and "systemctl start" calls accordingly).
- Around line 225-249: The enabled-state parsing in the try block that builds
enabled_map (using enabled_result from subprocess.run in the method containing
DETECTION_TIMEOUT) only treats state == "enabled" as enabled; update the check
that sets enabled_map[unit] so that units with state "enabled-runtime" are also
considered enabled (e.g., accept "enabled-runtime" or any variant that indicates
enabled) — modify the condition where state is evaluated before assigning
enabled_map[unit] to include "enabled-runtime" alongside "enabled".
In `@cortex/template_manager.py`:
- Around line 40-47: The create_template function uses the user-controlled name
directly to build template_dir, allowing path traversal or separator abuse;
before constructing template_dir (in create_template), validate and sanitize
name by rejecting empty strings, absolute paths, path separators, and any ".."
segments and enforce a safe whitelist (e.g. alphanumeric, hyphen, underscore) or
normalize it to a basename; if validation fails raise a ValueError. Update
references in create_template (template_dir, base_dir) to only use the
validated/sanitized name so no user input can escape base_dir.
♻️ Duplicate comments (2)
cortex/cli.py (2)
2162-2179: Deploy should fail when the import summary reports failures.
import_configuration()always returns a dict, so success is printed even with failures.🐛 Suggested fix
result = config_manager.import_configuration( str( manager.base_dir / template_data["name"] / template_data["version"] / "template.yaml" ), force=args.force, ) - if result: + failed = result.get("failed", []) + if failed: + self._print_error("Template deployment completed with failures") + for item in failed: + console.print(f" [red]✗[/red] {item}") + return 1 + + if result: console.print(" [green]✓[/green] Packages installed") console.print(" [green]✓[/green] Configurations applied") console.print(" [green]✓[/green] Services started") console.print(f"[green]✓[/green] System cloned successfully{target_label}\n") return 0
2192-2198: Default export should use the latest version, not hard-codedv1.
Otherwise users can unintentionally export stale templates when they omit a version.🛠️ Suggested fix
- path = manager.export_template(name, version or "v1", args.file) + if version is None: + latest = manager.get_template(name) + if not latest: + console.print(f"[red]Error:[/red] Template '{args.name}' not found.") + return 1 + version = latest["version"] + path = manager.export_template(name, version, args.file)
🧹 Nitpick comments (2)
tests/unit/test_config_manager_services.py (2)
6-14: Unused import:PathThe
Pathimport frompathlibon line 9 is never used in this file.🧹 Remove unused import
import os import sys import unittest -from pathlib import Path from unittest.mock import MagicMock, patch
94-138: Remove unnecessaryHardwareProfiler.profilemock.The
diff_configurationmethod doesn't callHardwareProfiler.profile(). The mock on line 98, its parameter on line 100, and the return value setup on line 102 are dead code that can be removed for clarity.🧹 Remove unused mock
`@patch`("cortex.config_manager.ConfigManager.detect_services") `@patch`("cortex.config_manager.ConfigManager.detect_apt_packages") `@patch`("cortex.config_manager.ConfigManager.detect_pip_packages") `@patch`("cortex.config_manager.ConfigManager.detect_npm_packages") - `@patch`("cortex.hwprofiler.HardwareProfiler.profile") def test_diff_configuration_with_services( - self, mock_profile, mock_npm, mock_pip, mock_apt, mock_srv + self, mock_npm, mock_pip, mock_apt, mock_srv ): - mock_profile.return_value = {"os": "24.04"} mock_apt.return_value = [] mock_pip.return_value = [] mock_npm.return_value = []
| def _parse_template_spec(self, name_spec: str) -> tuple[str, str | None]: | ||
| """Parse template name and version from spec (name:v1 or name-v1).""" | ||
| if ":" in name_spec: | ||
| parts = name_spec.split(":", 1) | ||
| return parts[0], parts[1] | ||
| elif "-v" in name_spec: | ||
| idx = name_spec.rfind("-v") | ||
| return name_spec[:idx], name_spec[idx + 1 :] | ||
| return name_spec, None |
There was a problem hiding this comment.
Avoid mis-parsing template names containing “-v”.
The current -v split triggers on any name with -v (e.g., dev-tools), producing a bogus version. Only treat -v as a version suffix when followed by a digit.
🔧 Suggested fix
- elif "-v" in name_spec:
- idx = name_spec.rfind("-v")
- return name_spec[:idx], name_spec[idx + 1 :]
+ elif "-v" in name_spec:
+ idx = name_spec.rfind("-v")
+ if idx != -1 and idx + 2 < len(name_spec) and name_spec[idx + 2].isdigit():
+ return name_spec[:idx], name_spec[idx + 1 :]
return name_spec, None📝 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.
| def _parse_template_spec(self, name_spec: str) -> tuple[str, str | None]: | |
| """Parse template name and version from spec (name:v1 or name-v1).""" | |
| if ":" in name_spec: | |
| parts = name_spec.split(":", 1) | |
| return parts[0], parts[1] | |
| elif "-v" in name_spec: | |
| idx = name_spec.rfind("-v") | |
| return name_spec[:idx], name_spec[idx + 1 :] | |
| return name_spec, None | |
| def _parse_template_spec(self, name_spec: str) -> tuple[str, str | None]: | |
| """Parse template name and version from spec (name:v1 or name-v1).""" | |
| if ":" in name_spec: | |
| parts = name_spec.split(":", 1) | |
| return parts[0], parts[1] | |
| elif "-v" in name_spec: | |
| idx = name_spec.rfind("-v") | |
| if idx != -1 and idx + 2 < len(name_spec) and name_spec[idx + 2].isdigit(): | |
| return name_spec[:idx], name_spec[idx + 1 :] | |
| return name_spec, None |
🤖 Prompt for AI Agents
In `@cortex/cli.py` around lines 2055 - 2063, The _parse_template_spec function
incorrectly treats any occurrence of "-v" as a version separator (breaking names
like "dev-tools"); change the logic to only split on the last "-v" when it is
immediately followed by a digit (e.g., match "-v" + digit). Specifically, in
_parse_template_spec check for "-v" presence with name_spec.rfind("-v") then
verify that there is a character after the "-v" and that that character is a
digit before slicing; when valid return name_spec[:idx] and the version as
name_spec[idx+2:], otherwise fall through to return the whole spec as the name
and None for version.
| try: | ||
| # 1. Get enabled status for all services (bulk) | ||
| enabled_map = {} | ||
| enabled_result = subprocess.run( | ||
| [ | ||
| "systemctl", | ||
| "list-unit-files", | ||
| "--type=service", | ||
| "--all", | ||
| "--no-legend", | ||
| "--no-pager", | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=self.DETECTION_TIMEOUT, | ||
| ) | ||
| if enabled_result.returncode == 0: | ||
| for line in enabled_result.stdout.strip().split("\n"): | ||
| if not line.strip(): | ||
| continue | ||
| parts = line.split() | ||
| if len(parts) >= 2: | ||
| unit = parts[0] | ||
| state = parts[1] | ||
| enabled_map[unit] = state == "enabled" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specified lines
fd cortex/config_manager.pyRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Read the specific lines mentioned in the review
head -n 249 cortex/config_manager.py | tail -n 25Repository: cortexlinux/cortex
Length of output: 996
🌐 Web query:
systemctl list-unit-files enabled-runtime state documentation
💡 Result:
- Command: systemctl list-unit-files --state=enabled-runtime
- Meaning: "enabled-runtime" shows unit files that are enabled only at runtime (enabled via transient symlinks in /run, not persisted to /etc), so the enablement lasts until reboot. (man.linuxreviews.org)
Example to list only services with runtime-only enablement:
systemctl list-unit-files --type=service --state=enabled-runtime. (linux-audit.com)
Citations:
- 1: https://man.linuxreviews.org/man1/systemctl.1.html?utm_source=openai
- 2: https://linux-audit.com/systemd/faq/how-to-see-all-enabled-services-with-systemctl/?utm_source=openai
Treat enabled-runtime units as enabled.
systemctl list-unit-files can report enabled-runtime for services enabled at runtime (via transient symlinks in /run). The current equality check treats these as disabled, when they should be considered enabled.
🔧 Suggested fix
- state = parts[1]
- enabled_map[unit] = state == "enabled"
+ state = parts[1]
+ enabled_map[unit] = state in {"enabled", "enabled-runtime"}🤖 Prompt for AI Agents
In `@cortex/config_manager.py` around lines 225 - 249, The enabled-state parsing
in the try block that builds enabled_map (using enabled_result from
subprocess.run in the method containing DETECTION_TIMEOUT) only treats state ==
"enabled" as enabled; update the check that sets enabled_map[unit] so that units
with state "enabled-runtime" are also considered enabled (e.g., accept
"enabled-runtime" or any variant that indicates enabled) — modify the condition
where state is evaluated before assigning enabled_map[unit] to include
"enabled-runtime" alongside "enabled".
| try: | ||
| if srv.get("enabled"): | ||
| cmd = ["sudo", "systemctl", "enable", name] | ||
| res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT) | ||
| if res.returncode != 0: | ||
| success = False | ||
|
|
||
| if srv.get("active_state") == "active": | ||
| cmd = ["sudo", "systemctl", "start", name] | ||
| res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT) | ||
| if res.returncode != 0: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "config_manager.py" -type fRepository: cortexlinux/cortex
Length of output: 88
🏁 Script executed:
sed -n '890,910p' ./cortex/config_manager.pyRepository: cortexlinux/cortex
Length of output: 857
🏁 Script executed:
rg "DETECTION_TIMEOUT|INSTALLATION_TIMEOUT" ./cortex/config_manager.py -n -B2 -A2Repository: cortexlinux/cortex
Length of output: 1888
🏁 Script executed:
sed -n '880,910p' ./cortex/config_manager.py | head -40Repository: cortexlinux/cortex
Length of output: 1284
🏁 Script executed:
rg "_update_service|systemctl" ./cortex/config_manager.py -n -B1 -A1Repository: cortexlinux/cortex
Length of output: 889
Use installation timeout for service enable/start operations.
Service enable and start operations are state-changing actions that can legitimately exceed the 30-second detection timeout. Use INSTALLATION_TIMEOUT (300 seconds) for these operations to prevent false failures. Additionally, add text=True to the subprocess calls for proper text output handling.
🛠️ Suggested fix
- res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT)
+ res = subprocess.run(
+ cmd,
+ capture_output=True,
+ text=True,
+ timeout=self.INSTALLATION_TIMEOUT,
+ )
...
- res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT)
+ res = subprocess.run(
+ cmd,
+ capture_output=True,
+ text=True,
+ timeout=self.INSTALLATION_TIMEOUT,
+ )📝 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.
| try: | |
| if srv.get("enabled"): | |
| cmd = ["sudo", "systemctl", "enable", name] | |
| res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT) | |
| if res.returncode != 0: | |
| success = False | |
| if srv.get("active_state") == "active": | |
| cmd = ["sudo", "systemctl", "start", name] | |
| res = subprocess.run(cmd, capture_output=True, timeout=self.DETECTION_TIMEOUT) | |
| if res.returncode != 0: | |
| try: | |
| if srv.get("enabled"): | |
| cmd = ["sudo", "systemctl", "enable", name] | |
| res = subprocess.run( | |
| cmd, | |
| capture_output=True, | |
| text=True, | |
| timeout=self.INSTALLATION_TIMEOUT, | |
| ) | |
| if res.returncode != 0: | |
| success = False | |
| if srv.get("active_state") == "active": | |
| cmd = ["sudo", "systemctl", "start", name] | |
| res = subprocess.run( | |
| cmd, | |
| capture_output=True, | |
| text=True, | |
| timeout=self.INSTALLATION_TIMEOUT, | |
| ) | |
| if res.returncode != 0: |
🤖 Prompt for AI Agents
In `@cortex/config_manager.py` around lines 895 - 905, The service enable/start
subprocess.run calls in the block that checks srv.get("enabled") and
srv.get("active_state") use self.DETECTION_TIMEOUT and omit text mode; change
those runs to use self.INSTALLATION_TIMEOUT and add text=True to subprocess.run
so long-running enable/start operations don't falsely fail and stdout/stderr are
returned as text (update the two subprocess.run invocations around the
"systemctl enable" and "systemctl start" calls accordingly).
| def create_template( | ||
| self, name: str, description: str, package_sources: list[str] | None = None | ||
| ) -> str: | ||
| """ | ||
| Create a new system template. | ||
| """ | ||
| template_dir = self.base_dir / name | ||
| template_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Validate template names before using them as paths.
name is user-controlled and used directly in the filesystem path, allowing ../ or separator abuse.
🔒 Suggested fix
def create_template(
self, name: str, description: str, package_sources: list[str] | None = None
) -> str:
"""
Create a new system template.
"""
+ if not re.match(r"^[a-zA-Z0-9._-]+$", name):
+ raise ValueError(f"Invalid template name: {name}")
template_dir = self.base_dir / name
template_dir.mkdir(parents=True, exist_ok=True)🤖 Prompt for AI Agents
In `@cortex/template_manager.py` around lines 40 - 47, The create_template
function uses the user-controlled name directly to build template_dir, allowing
path traversal or separator abuse; before constructing template_dir (in
create_template), validate and sanitize name by rejecting empty strings,
absolute paths, path separators, and any ".." segments and enforce a safe
whitelist (e.g. alphanumeric, hyphen, underscore) or normalize it to a basename;
if validation fails raise a ValueError. Update references in create_template
(template_dir, base_dir) to only use the validated/sanitized name so no user
input can escape base_dir.
CLA Verification PassedAll contributors have signed the CLA.
|
|
|
@pratyush07-hub Thanks for working on this. |
|
@Anshgrover23 , I have invested too much time on this and have implemented almost everything as mentioned in the issue. I’m confident that I can handle this. |
2 participants
Related Issue
Closes cortexlinux/cortex-ops#19
Summary
This PR adds system cloning and templating to Cortex, enabling full system state capture and saving it as versioned templates. These templates can be deployed to new systems with safe handling of configuration differences, supporting export/import and differential updates. The implementation includes documentation and unit tests with >80% coverage.
Demonstration
Screencast.from.2026-01-19.19-47-43.webm
AI Disclosure
Used Claude Opus 4.5 (Antigravity Coding Assistant) :
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.