Add configuration export/import system (Issue #33) #202
Conversation
Co-authored-by: danishirfan21 <44131991+danishirfan21@users.noreply.github.com>
…on, refactor methods Co-authored-by: danishirfan21 <44131991+danishirfan21@users.noreply.github.com>
Co-authored-by: danishirfan21 <44131991+danishirfan21@users.noreply.github.com>
Co-authored-by: danishirfan21 <44131991+danishirfan21@users.noreply.github.com>
…lidation Co-authored-by: danishirfan21 <44131991+danishirfan21@users.noreply.github.com>
…nagement-feature Add configuration export/import for system reproducibility with security enhancements
|
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. WalkthroughAdds a new ConfigManager (export/import/diff for apt/pip/npm) with CLI scaffold, YAML export format, compatibility checks, sandboxed/direct install paths, preferences persistence, unit tests, documentation and example config; updates requirements and modifies .gitignore patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CM as ConfigManager
participant Det as PackageDetectors
participant FS as FileSystem
participant SB as SandboxExecutor
rect rgba(220,235,255,0.6)
Note over User,CM: Export flow
User->>CM: export_configuration(output_path,...)
CM->>Det: detect_installed_packages(sources)
Det-->>CM: packages
CM->>FS: gather hardware & preferences
FS-->>CM: system data
CM->>FS: write YAML
FS-->>CM: output path
CM-->>User: export complete
end
rect rgba(235,245,220,0.6)
Note over User,CM: Dry-run import (preview)
User->>CM: import_configuration(path, dry_run=True)
CM->>FS: load YAML
FS-->>CM: config
CM->>CM: validate_compatibility(config)
CM->>CM: diff_configuration(config)
CM-->>User: dry-run diff
end
rect rgba(255,240,220,0.6)
Note over User,CM: Apply import
User->>CM: import_configuration(path, dry_run=False)
CM->>CM: validate & diff
alt package changes exist
CM->>SB: _install_package / _install_with_sandbox
SB-->>CM: install result
end
CM->>FS: save preferences
FS-->>CM: ack
CM-->>User: import summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive configuration management system for Cortex Linux, enabling reproducible system setups through YAML-based configuration export and import. The feature addresses Issue #33 by providing a robust solution for team collaboration, disaster recovery, and environment consistency.
Key Changes:
- Added multi-source package management supporting APT, PIP, and NPM package managers with automatic detection and version tracking
- Implemented safety features including version compatibility checking, OS compatibility warnings, path traversal protection for package identifiers, and optional SandboxExecutor integration
- Created comprehensive test suite with 32 passing tests covering 87%+ of the codebase, including package detection, configuration validation, import/export workflows, and error handling
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config_manager.py | Main implementation (922 lines) providing ConfigManager class with export, import, diff, and validation capabilities |
| src/test_config_manager.py | Comprehensive test suite (683 lines) with 32 unit tests using mocked system calls |
| CONFIGURATION.md | User documentation (589 lines) including usage guide, API reference, troubleshooting, and security considerations |
| examples/sample-config.yaml | Sample configuration file demonstrating YAML structure with realistic package examples |
| src/requirements.txt | Added pyyaml>=6.0.1 and packaging>=23.0 dependencies |
| .gitignore | Added Cortex-specific patterns (.cortex/, *.yaml.bak, /tmp/) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/config_manager.py (2)
38-43: Consider makingDEFAULT_SOURCESimmutable
DEFAULT_SOURCESis a mutable list at the class level, which can be mutated accidentally at runtime. Making it a tuple (or annotating asClassVar) would prevent subtle bugs and also satisfy the static analysis warning about mutable class attributes.Example:
- DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + DEFAULT_SOURCES = (SOURCE_APT, SOURCE_PIP, SOURCE_NPM)
222-237: Broadexcept Exceptionand silent failure may hide real issues
_load_preferencescatches a bareExceptionand then silentlypasses, returning{}. Similar broad exception handling patterns appear elsewhere in this file. While this is safe from a crash perspective, it makes real bugs (permissions, YAML parse issues, disk errors) hard to diagnose.Consider:
- Narrowing the exception types where feasible (e.g.,
FileNotFoundError,yaml.YAMLError,OSError).- At least logging the exception at debug level instead of silently swallowing it.
This keeps the UX resilient while still surfacing unexpected failures during development.
CONFIGURATION.md (1)
260-269: Minor Markdown lint cleanups (languages + bare URLs)A few small polish items flagged by markdownlint:
- The filename and tree examples use bare fenced blocks (
```) without a language; consider```textor```bashfor clarity.- The support section uses bare URLs; wrapping them in Markdown links improves readability and satisfies MD034.
These are non-blocking but easy wins for docs quality.
Also applies to: 316-321, 582-585
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)CONFIGURATION.md(1 hunks)examples/sample-config.yaml(1 hunks)src/config_manager.py(1 hunks)src/requirements.txt(1 hunks)src/test_config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/test_config_manager.py (3)
src/config_manager.py (12)
ConfigManager(19-781)detect_apt_packages(58-89)detect_pip_packages(91-122)detect_npm_packages(124-156)detect_installed_packages(158-192)export_configuration(251-312)validate_compatibility(314-362)diff_configuration(364-448)_compare_versions(450-472)_save_preferences(238-249)_load_preferences(222-236)_install_package(701-781)src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 markdownlint-cli2 (0.18.1)
CONFIGURATION.md
264-264: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
316-316: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
582-582: Bare URL used
(MD034, no-bare-urls)
583-583: Bare URL used
(MD034, no-bare-urls)
584-584: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.5)
src/test_config_manager.py
1-1: Shebang is present but file is not executable
(EXE001)
631-631: Unused method argument: mock_run
(ARG002)
src/config_manager.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
69-69: Starting a process with a partial executable path
(S607)
85-85: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
103-103: subprocess call: check for execution of untrusted input
(S603)
135-135: Starting a process with a partial executable path
(S607)
158-158: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
218-218: Consider moving this statement to an else block
(TRY300)
219-219: Do not catch blind exception: Exception
(BLE001)
233-234: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
255-255: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
285-285: Do not catch blind exception: Exception
(BLE001)
310-310: Consider moving this statement to an else block
(TRY300)
311-311: Do not catch blind exception: Exception
(BLE001)
312-312: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
350-352: try-except-pass detected, consider logging the exception
(S110)
350-350: Do not catch blind exception: Exception
(BLE001)
422-422: Do not catch blind exception: Exception
(BLE001)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
526-526: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
557-557: Do not catch blind exception: Exception
(BLE001)
558-558: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
558-558: Avoid specifying long messages outside the exception class
(TRY003)
564-564: Avoid specifying long messages outside the exception class
(TRY003)
645-645: Do not catch blind exception: Exception
(BLE001)
646-646: Use explicit conversion flag
Replace with conversion flag
(RUF010)
680-680: Do not catch blind exception: Exception
(BLE001)
681-681: Use explicit conversion flag
Replace with conversion flag
(RUF010)
749-749: Do not catch blind exception: Exception
(BLE001)
759-759: subprocess call: check for execution of untrusted input
(S603)
767-767: subprocess call: check for execution of untrusted input
(S603)
775-775: subprocess call: check for execution of untrusted input
(S603)
778-778: Do not catch blind exception: Exception
(BLE001)
916-916: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (5)
src/requirements.txt (1)
8-10: Config management dependencies look appropriateAdding
pyyamlandpackaginghere matches howConfigManageruses YAML parsing and robust version comparison; no issues from a dependency standpoint.examples/sample-config.yaml (1)
1-74: Sample configuration aligns with ConfigManager schemaThe YAML structure (metadata, hardware,
packages,preferences,environment_variables) matches whatConfigManagerexports and consumes, so this is a good canonical example for users..gitignore (1)
149-158: Ignore rules align with new configuration workflowThe added patterns (
*.out, editor swap files,.cortex/,*.yaml.bak,tmp/under the repo) make sense for the new config export/import feature and help keep local state and backups out of version control.src/config_manager.py (1)
721-747: The review comment is incorrect—the code is already correctThe
successattribute is defined as a@property(line 51-54 insrc/sandbox_executor.py), not a regular method. In Python, properties are accessed without parentheses. The current codereturn result.successat lines 731 and 739 insrc/config_manager.pycorrectly accesses the property without calling it as a method. The proposed fixresult.success()would be wrong for a property and would cause an error.The unit tests correctly mock this as
mock_result.success = True, which is the right approach for a property.Likely an incorrect or invalid review comment.
src/test_config_manager.py (1)
630-647: Review comment is partially incorrect—successis a property, not a methodThe review misidentifies
ExecutionResult.success. Insrc/sandbox_executor.py:54-57,successis defined as a@property, not a method:@property def success(self) -> bool: """Check if command executed successfully.""" return not self.blocked and self.exit_code == 0This means:
- The current implementation
return result.success(line 733 in_install_package) is correct- The current test mock
mock_result.success = Trueis correct- The review's suggestion to change it to
mock_result.success.return_value = Trueis incorrect—this syntax is for methods, not propertiesThe review does correctly identify one valid issue: the
mock_runparameter is unused (ARG002 lint warning). This can be fixed by either removing the@patch('subprocess.run')decorator or addingmock_run.assert_not_called()to verify the sandbox path avoids direct subprocess calls. However, the test's mock setup forsuccessrequires no changes.Likely an incorrect or invalid review comment.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CONFIGURATION.md (1)
414-423: Defineconfigvariable before callingdiff_configuration()in the Programmatic API example.The example at line 421 calls
manager.diff_configuration(config)butconfigis never defined in the snippet. Show how to load the config object first, either by reading and parsing an exported YAML file or by using the result fromexport_configuration().Apply this diff to fix the example:
# Import with dry-run result = manager.import_configuration( config_path='config.yaml', dry_run=True ) +# Load config for diff +import yaml + +with open('config.yaml', 'r') as f: + config = yaml.safe_load(f) + # Check diff diff = manager.diff_configuration(config) print(f"To install: {len(diff['packages_to_install'])}")This aligns the documentation with the actual
diff_configurationmethod signature and makes the snippet immediately runnable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONFIGURATION.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CONFIGURATION.md
264-264: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
316-316: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
582-582: Bare URL used
(MD034, no-bare-urls)
583-583: Bare URL used
(MD034, no-bare-urls)
584-584: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
CONFIGURATION.md (1)
1-589: Comprehensive documentation with clear structure and examples.The documentation is well-organized, covers all key features (export, import, diff, dry-run, selective operations), provides practical examples, and includes best practices, troubleshooting, and security considerations. The YAML format specification is clear, and the API reference matches the ConfigManager interface. Integration with SandboxExecutor is well-documented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/config_manager.py (2)
304-312: Validate output_path to prevent path traversal attacks.The method does not validate the
output_pathparameter. A malicious path like../../etc/sensitive-filecould overwrite files outside the intended directory, potentially compromising the system.Consider adding path validation:
def export_configuration(self, output_path: str, include_hardware: bool = True, include_preferences: bool = True, package_sources: List[str] = None) -> str: + # Validate output path to prevent path traversal + output_path_obj = Path(output_path).resolve() + # Ensure the resolved path doesn't escape intended boundaries + # Option 1: Restrict to current directory and subdirectories + try: + output_path_obj.relative_to(Path.cwd()) + except ValueError: + raise ValueError(f"Output path must be within current directory: {output_path}") + + # Option 2: Block suspicious patterns + if '..' in Path(output_path).parts: + raise ValueError(f"Output path contains invalid patterns: {output_path}") + ... # Write to file try: - output_path_obj = Path(output_path) output_path_obj.parent.mkdir(parents=True, exist_ok=True)Choose the validation strategy that best fits your security requirements.
554-558: Validate config_path to prevent reading unintended files.While less critical than write operations, the method does not validate
config_pathbefore reading. Malicious paths with..or symlinks could potentially expose sensitive file contents through error messages or if the YAML parsing reveals file structure.Add basic path validation:
# Load configuration try: + config_path_obj = Path(config_path).resolve() + # Block suspicious patterns + if '..' in Path(config_path).parts: + raise ValueError(f"Config path contains invalid patterns: {config_path}") + # Ensure file exists and is a regular file (not symlink to sensitive location) + if not config_path_obj.is_file(): + raise ValueError(f"Config path must be a regular file: {config_path}") + - with open(config_path, 'r') as f: + with open(config_path_obj, 'r') as f: config = yaml.safe_load(f)CONFIGURATION.md (2)
264-269: Add language specifier to code block.The fenced code block lacks a language specifier. Add
textto improve rendering and comply with markdown best practices.Apply this diff:
Name files descriptively: -``` +```text dev-machine-john.yaml production-web-server.yaml ml-training-gpu-rig.yaml team-baseline-2024-11.yaml ```
316-321: Add language specifier to code block.The fenced code block showing the directory tree lacks a language specifier. Add
textfor proper rendering.Apply this diff:
Maintain separate configs for different environments: -``` +```text configs/ ├── development.yaml ├── staging.yaml └── production.yaml ```
🧹 Nitpick comments (4)
src/config_manager.py (4)
1-1: Make file executable or remove shebang.The file has a shebang (
#!/usr/bin/env python3) but is not executable. Either make it executable withchmod +x src/config_manager.pyor remove the shebang if the file is intended to be imported as a module rather than executed directly.
42-42: Consider annotating DEFAULT_SOURCES with ClassVar.
DEFAULT_SOURCESis a mutable list defined at class level. While it's treated as a constant in the code, annotating it withtyping.ClassVarmakes the intent explicit and prevents accidental instance-level modifications.Apply this diff:
+from typing import ClassVar + class ConfigManager: ... - DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + DEFAULT_SOURCES: ClassVar[List[str]] = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM]
248-249: Preserve exception context when re-raising.When raising a new exception within an
exceptclause, useraise ... from eto preserve the original exception context for better debugging.Apply this diff:
except Exception as e: - raise RuntimeError(f"Failed to save preferences: {e}") + raise RuntimeError(f"Failed to save preferences: {e}") from e
648-649: Consider logging exceptions for better debugging.While catching broad exceptions prevents individual package failures from stopping the entire import, consider logging the exception details rather than just adding them to the failed list. This would help debugging without stopping the import process.
Example improvement:
except Exception as e: + import logging + logging.exception(f"Failed to install package {pkg['name']}") summary['failed'].append(f"{pkg['name']} ({str(e)})")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONFIGURATION.md(1 hunks)src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
69-69: Starting a process with a partial executable path
(S607)
85-85: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
103-103: subprocess call: check for execution of untrusted input
(S603)
135-135: Starting a process with a partial executable path
(S607)
158-158: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
218-218: Consider moving this statement to an else block
(TRY300)
219-219: Do not catch blind exception: Exception
(BLE001)
233-234: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
255-255: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
285-285: Do not catch blind exception: Exception
(BLE001)
310-310: Consider moving this statement to an else block
(TRY300)
311-311: Do not catch blind exception: Exception
(BLE001)
312-312: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
350-352: try-except-pass detected, consider logging the exception
(S110)
350-350: Do not catch blind exception: Exception
(BLE001)
422-422: Do not catch blind exception: Exception
(BLE001)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
526-526: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
557-557: Do not catch blind exception: Exception
(BLE001)
558-558: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
558-558: Avoid specifying long messages outside the exception class
(TRY003)
564-564: Avoid specifying long messages outside the exception class
(TRY003)
648-648: Do not catch blind exception: Exception
(BLE001)
649-649: Use explicit conversion flag
Replace with conversion flag
(RUF010)
683-683: Do not catch blind exception: Exception
(BLE001)
684-684: Use explicit conversion flag
Replace with conversion flag
(RUF010)
752-752: Do not catch blind exception: Exception
(BLE001)
762-762: subprocess call: check for execution of untrusted input
(S603)
770-770: subprocess call: check for execution of untrusted input
(S603)
778-778: subprocess call: check for execution of untrusted input
(S603)
781-781: Do not catch blind exception: Exception
(BLE001)
921-921: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
src/config_manager.py (8)
58-192: Package detection implementation looks solid.The package detection methods appropriately handle missing package managers, timeouts, and parsing errors. The deduplication strategy using
(name, source)tuples is efficient and correct.
314-362: Compatibility validation logic is well-designed.The version compatibility checks are appropriately strict (major version must match) while remaining flexible (lenient on minor versions and parsing errors). The OS mismatch warning without failure is a good UX decision.
364-448: Diff logic is comprehensive and robust.The method correctly identifies all package states (install/upgrade/downgrade/already installed) and preference changes. The fallback to treating comparison failures as upgrades is safe, and the warning generation for downgrades is helpful.
450-533: Version comparison implementation is robust.The two-tier approach (using
packaging.versionwhen available, with a numeric fallback) provides both accuracy and resilience. The fallback implementation correctly handles edge cases and is well-documented about its limitations.
651-684: Preferences import logic is clean and correct.The method appropriately handles both successful and failed preference saves, and correctly treats empty/missing preferences as a no-op rather than an error.
686-702: Package identifier validation is well-designed.The regex pattern effectively prevents command injection while supporting NPM scoped packages. The single-slash limit prevents path traversal attacks while allowing
@scope/packageformat. This is a strong security control.
787-927: CLI implementation is well-designed and user-friendly.The command-line interface provides clear subcommands with helpful output formatting. The downgrade tracking (lines 894-895) correctly displays downgrades separately from upgrades, addressing the concern from previous review comments. The use of emoji indicators and limited output (first 5 items) makes the output readable without overwhelming users.
724-750: The SandboxExecutor.execute() method accepts only string commands, not arrays—the refactoring suggestion is not feasible.The
execute()method is explicitly typed ascommand: strand the implementation processes commands as strings throughout (seesrc/sandbox_executor.py:455-456). The current code inconfig_manager.pyis correct: it validates identifiers before constructing command strings and relies onSandboxExecutor.validate_command()for further security validation.Likely an incorrect or invalid review comment.
CONFIGURATION.md (1)
1-592: Comprehensive and well-structured documentation.The documentation is thorough and user-friendly, covering installation, usage, best practices, troubleshooting, security considerations, and API reference. The examples are practical and the security guidance is valuable. The only minor issues are the missing language specifiers for two code blocks, which have already been flagged.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/config_manager.py (2)
304-305: Path traversal validation for output_path (duplicate).This concern was previously raised and you've already requested a separate PR to address path traversal validation for the
output_pathparameter. No additional action needed here.
569-571: Path traversal validation for config_path (duplicate).This concern was previously raised and you've already requested a separate PR to address path validation for the
config_pathparameter. No additional action needed here.
🧹 Nitpick comments (4)
src/config_manager.py (4)
42-42: Annotate mutable class attribute withClassVar.Per PEP 484, mutable class attributes should be annotated with
typing.ClassVarto distinguish them from instance attributes.Apply this diff:
+from typing import Dict, List, Optional, Any, Tuple, ClassVar + class ConfigManager: # ... - DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + DEFAULT_SOURCES: ClassVar[List[str]] = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM]
158-158: Use explicitOptionaltype hint.PEP 484 discourages implicit
Optional. Make the type annotation explicit.Apply this diff:
- def detect_installed_packages(self, sources: List[str] = None) -> List[Dict[str, Any]]: + def detect_installed_packages(self, sources: Optional[List[str]] = None) -> List[Dict[str, Any]]:
194-249: Consider adding debug logging for error cases.The broad exception handling is acceptable for these utility methods, but adding debug-level logging would help troubleshoot detection failures without changing the fallback behavior.
Example for
_detect_os_version:except Exception as e: logger.debug(f"Failed to detect OS version: {e}") return "unknown"
324-332: Validate types of required configuration fields.The method checks for field presence but not types. If
packagesis not a list orcortex_versionis not a string, downstream code could fail with confusing errors.Add type validation after presence checks:
if 'packages' not in config: return False, "Missing packages field in configuration" + + if not isinstance(config['packages'], list): + return False, "packages field must be a list" + if not isinstance(config['cortex_version'], str): + return False, "cortex_version field must be a string" + if not isinstance(config['os'], str): + return False, "os field must be a string"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
69-69: Starting a process with a partial executable path
(S607)
103-103: subprocess call: check for execution of untrusted input
(S603)
135-135: Starting a process with a partial executable path
(S607)
158-158: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
218-218: Consider moving this statement to an else block
(TRY300)
219-219: Do not catch blind exception: Exception
(BLE001)
233-234: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
255-255: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
285-285: Do not catch blind exception: Exception
(BLE001)
310-310: Consider moving this statement to an else block
(TRY300)
311-311: Do not catch blind exception: Exception
(BLE001)
312-312: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
350-352: try-except-pass detected, consider logging the exception
(S110)
350-350: Do not catch blind exception: Exception
(BLE001)
400-400: Do not catch blind exception: Exception
(BLE001)
484-484: Consider moving this statement to an else block
(TRY300)
485-485: Do not catch blind exception: Exception
(BLE001)
541-541: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
572-572: Do not catch blind exception: Exception
(BLE001)
573-573: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
573-573: Avoid specifying long messages outside the exception class
(TRY003)
579-579: Avoid specifying long messages outside the exception class
(TRY003)
663-663: Do not catch blind exception: Exception
(BLE001)
664-664: Use explicit conversion flag
Replace with conversion flag
(RUF010)
698-698: Do not catch blind exception: Exception
(BLE001)
699-699: Use explicit conversion flag
Replace with conversion flag
(RUF010)
742-742: Consider moving this statement to an else block
(TRY300)
743-743: Do not catch blind exception: Exception
(BLE001)
772-772: subprocess call: check for execution of untrusted input
(S603)
773-773: Consider moving this statement to an else block
(TRY300)
774-774: Do not catch blind exception: Exception
(BLE001)
937-937: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
src/config_manager.py (7)
58-156: LGTM: Package detection methods are well-implemented.The three detection methods handle timeouts appropriately, gracefully handle missing package managers, and return structured data. Silent error handling is appropriate for optional dependencies.
364-463: LGTM: Package categorization and diff logic are sound.The methods correctly categorize packages, handle version comparisons with fallback, and track downgrades separately (as addressed in the previous review).
465-548: LGTM: Version comparison logic is robust.The two-tier approach (packaging library with fallback) handles various version formats well. The edge case handling and documentation are thorough.
655-660: LGTM: Downgrade tracking is correctly implemented.The fix from the previous review is confirmed: packages are now correctly categorized as installed, upgraded, or downgraded based on their presence in the respective diff lists.
701-717: LGTM: Package identifier validation prevents command injection.The regex pattern correctly validates package names, supports NPM scoped packages, and prevents path traversal attacks. This is essential security hardening.
910-911: LGTM: Downgrade display in CLI output is correctly implemented.The CLI correctly displays downgraded packages separately, completing the fix from the previous review.
803-943: LGTM: CLI implementation is well-structured and user-friendly.The argument parsing, command routing, and output formatting provide a good user experience. Error handling is appropriate for a CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/config_manager.py (2)
302-312: Path traversal validation is still missing despite previous feedback.The past review comment indicated path traversal concerns for
output_path, but validation is not present in the current code. A malicious path like../../sensitive/filecould overwrite unintended files. While the developer acknowledged the feedback, the fix is not implemented in this version.Consider adding path validation:
# Write to file try: output_path_obj = Path(output_path) + # Prevent path traversal + if '..' in output_path_obj.parts: + raise RuntimeError("Invalid output path: path traversal detected") + # Optionally, ensure path is relative to a safe base directory output_path_obj.parent.mkdir(parents=True, exist_ok=True)Additionally, use exception chaining:
except Exception as e: - raise RuntimeError(f"Failed to export configuration: {e}") + raise RuntimeError(f"Failed to export configuration: {e}") from e
568-573: Path traversal validation is still missing for config_path despite previous feedback.The past review comment highlighted potential path traversal risks when loading configuration files. While reading is generally safer than writing, symbolic links or unusual path constructs could lead to reading unintended files. The developer acknowledged the feedback, but validation is not present in the current code.
Consider adding basic path validation:
# Load configuration try: + config_path_obj = Path(config_path).resolve() + # Ensure the resolved path exists and is a file + if not config_path_obj.is_file(): + raise RuntimeError("Configuration file not found or not accessible") - with open(config_path, 'r') as f: + with open(config_path_obj, 'r') as f: config = yaml.safe_load(f) except Exception as e: - raise RuntimeError(f"Failed to load configuration file: {e}") + raise RuntimeError(f"Failed to load configuration file: {e}") from eAlso use exception chaining for better debugging context.
🧹 Nitpick comments (3)
src/config_manager.py (3)
42-42: Make DEFAULT_SOURCES immutable or annotate as ClassVar.The mutable list
DEFAULT_SOURCESis a class attribute that could be accidentally modified, affecting all instances. Consider making it a tuple or annotating withtyping.ClassVarif mutability is required elsewhere.Apply this diff:
- DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + DEFAULT_SOURCES: tuple = (SOURCE_APT, SOURCE_PIP, SOURCE_NPM)Or if mutability is needed:
+from typing import ClassVar + class ConfigManager: ... - DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + DEFAULT_SOURCES: ClassVar[list] = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM]
158-158: Use explicit Optional type hint for the sources parameter.The
sourcesparameter has an implicitOptionalwhich violates PEP 484. Use explicitOptional[List[str]]or the newerList[str] | Nonesyntax.Apply this diff:
- def detect_installed_packages(self, sources: List[str] = None) -> List[Dict[str, Any]]: + def detect_installed_packages(self, sources: Optional[List[str]] = None) -> List[Dict[str, Any]]:
248-249: Use exception chaining for better error context.When raising a new exception from within an except block, use
raise ... from eto preserve the original exception context and traceback.Apply this diff:
except Exception as e: - raise RuntimeError(f"Failed to save preferences: {e}") + raise RuntimeError(f"Failed to save preferences: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
42-42: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
69-69: Starting a process with a partial executable path
(S607)
103-103: subprocess call: check for execution of untrusted input
(S603)
135-135: Starting a process with a partial executable path
(S607)
158-158: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
218-218: Consider moving this statement to an else block
(TRY300)
219-219: Do not catch blind exception: Exception
(BLE001)
233-234: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
255-255: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
285-285: Do not catch blind exception: Exception
(BLE001)
310-310: Consider moving this statement to an else block
(TRY300)
311-311: Do not catch blind exception: Exception
(BLE001)
312-312: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
350-352: try-except-pass detected, consider logging the exception
(S110)
350-350: Do not catch blind exception: Exception
(BLE001)
400-400: Do not catch blind exception: Exception
(BLE001)
484-484: Consider moving this statement to an else block
(TRY300)
485-485: Do not catch blind exception: Exception
(BLE001)
541-541: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
572-572: Do not catch blind exception: Exception
(BLE001)
573-573: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
573-573: Avoid specifying long messages outside the exception class
(TRY003)
579-579: Avoid specifying long messages outside the exception class
(TRY003)
663-663: Do not catch blind exception: Exception
(BLE001)
664-664: Use explicit conversion flag
Replace with conversion flag
(RUF010)
698-698: Do not catch blind exception: Exception
(BLE001)
699-699: Use explicit conversion flag
Replace with conversion flag
(RUF010)
742-742: Consider moving this statement to an else block
(TRY300)
743-743: Do not catch blind exception: Exception
(BLE001)
768-768: subprocess call: check for execution of untrusted input
(S603)
769-769: Consider moving this statement to an else block
(TRY300)
770-770: Do not catch blind exception: Exception
(BLE001)
933-933: Do not catch blind exception: Exception
(BLE001)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/config_manager.py (2)
250-311: Add basic path validation foroutput_pathto reduce path traversal risk
export_configurationwill happily write to any path, including things like../../etc/passwdif a caller passes that in. That’s low-risk for an interactive CLI user but becomes more dangerous if the API is ever driven by untrusted input (or scripts). A minimal hardening is to reject parent-directory components inoutput_path.You can do this inline:
- # Write to file - try: - output_path_obj = Path(output_path) + # Write to file + try: + output_path_obj = Path(output_path).expanduser() + # Basic defense against path traversal: disallow parent-directory segments. + if '..' in output_path_obj.parts: + raise ValueError( + "Parent directory components ('..') are not allowed in export output paths" + ) output_path_obj.parent.mkdir(parents=True, exist_ok=True) with open(output_path_obj, 'w') as f: yaml.safe_dump(config, f, default_flow_style=False, sort_keys=False)
549-603: Validateconfig_pathand YAML structure before using the configurationTwo related concerns in
import_configuration:
config_pathis used directly; a value like../../etc/shadowwould be read without any sanity checks.yaml.safe_loadcan returnNoneor a non-dict (e.g., a list), which will causevalidate_compatibilityto fail with aTypeErrorinstead of a clear “invalid config” error.You can address both with a small adjustment:
- # Load configuration - try: - with open(config_path, 'r') as f: - config = yaml.safe_load(f) - except Exception as e: - raise RuntimeError(f"Failed to load configuration file: {e}") + # Load configuration + try: + config_path_obj = Path(config_path).expanduser() + # Basic defense against path traversal: disallow parent-directory segments. + if '..' in config_path_obj.parts: + raise ValueError( + "Parent directory components ('..') are not allowed in configuration file paths" + ) + + with open(config_path_obj, 'r') as f: + config = yaml.safe_load(f) + except Exception as e: + raise RuntimeError(f"Failed to load configuration file: {e}") + + if not isinstance(config, dict): + raise RuntimeError( + "Invalid configuration file: expected a mapping at the top level" + )This keeps behavior for valid configs the same, but fails fast and explicitly on malformed YAML or suspicious paths.
🧹 Nitpick comments (3)
src/config_manager.py (3)
43-56: Ensure existing~/.cortexdirectory is not world-readable/writable
mkdir(mode=0o700, exist_ok=True)only enforces permissions on first creation; if~/.cortexalready exists with permissive bits, it’s left as-is. Given this directory can hold configuration and preferences, consider validating its mode (and optionally owner) on startup and warning or failing if it’s more permissive than0o700. This would better match the “0o700 with ownership validation” requirement.
807-815: Confirm CLI vs API defaults for including hardware in exports
export_configuration()defaultsinclude_hardware=True, but the CLI only includes hardware when--include-hardwareis explicitly passed; a bare:cortex export --output dev-machine.cortex.yamlwill not include hardware, which is the opposite of the programmatic default. If the intent is “hardware included by default,” consider flipping the CLI to
--no-hardwareor explicitly passinginclude_hardware=Truewhen neither--include-hardwarenor a disabling flag is set. Otherwise, documenting this divergence clearly in CONFIGURATION.md would avoid surprises.Also applies to: 840-853
1-7: Executable file without shebang: align permissions or add shebangRuff reports EXE002: the file is marked executable but has no shebang. Since you already have a
main()guarded byif __name__ == '__main__':, you have two options:
- If you don’t intend to run it directly: drop the executable bit in git:
chmod -x src/config_manager.py
- If you do want
./src/config_manager.pyto work, add a shebang as the first line:-""" +#!/usr/bin/env python3 +""" Configuration Manager for Cortex LinuxEither approach will satisfy the linter and make the execution model explicit.
Also applies to: 937-938
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: The file is executable but no shebang is present
(EXE002)
41-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
68-68: Starting a process with a partial executable path
(S607)
102-102: subprocess call: check for execution of untrusted input
(S603)
134-134: Starting a process with a partial executable path
(S607)
157-157: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
217-217: Consider moving this statement to an else block
(TRY300)
218-218: Do not catch blind exception: Exception
(BLE001)
232-233: try-except-pass detected, consider logging the exception
(S110)
232-232: Do not catch blind exception: Exception
(BLE001)
247-247: Do not catch blind exception: Exception
(BLE001)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
248-248: Avoid specifying long messages outside the exception class
(TRY003)
254-254: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
284-284: Do not catch blind exception: Exception
(BLE001)
309-309: Consider moving this statement to an else block
(TRY300)
310-310: Do not catch blind exception: Exception
(BLE001)
311-311: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
349-351: try-except-pass detected, consider logging the exception
(S110)
349-349: Do not catch blind exception: Exception
(BLE001)
399-399: Do not catch blind exception: Exception
(BLE001)
483-483: Consider moving this statement to an else block
(TRY300)
484-484: Do not catch blind exception: Exception
(BLE001)
540-540: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
571-571: Do not catch blind exception: Exception
(BLE001)
572-572: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
572-572: Avoid specifying long messages outside the exception class
(TRY003)
578-578: Avoid specifying long messages outside the exception class
(TRY003)
662-662: Do not catch blind exception: Exception
(BLE001)
663-663: Use explicit conversion flag
Replace with conversion flag
(RUF010)
697-697: Do not catch blind exception: Exception
(BLE001)
698-698: Use explicit conversion flag
Replace with conversion flag
(RUF010)
741-741: Consider moving this statement to an else block
(TRY300)
742-742: Do not catch blind exception: Exception
(BLE001)
767-767: subprocess call: check for execution of untrusted input
(S603)
768-768: Consider moving this statement to an else block
(TRY300)
769-769: Do not catch blind exception: Exception
(BLE001)
932-932: Do not catch blind exception: Exception
(BLE001)
- Extract argument parser setup to _setup_argument_parser() - Extract command handlers to separate functions (_handle_export_command, _handle_import_command, _handle_diff_command) - Extract result printing to _print_dry_run_results() and _print_import_results() - Extract package list printing to _print_package_list() - Reduce main() cognitive complexity from 73 to <15
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/config_manager.py (4)
31-41: Tighten typing forDEFAULT_SOURCES,sources, andpackage_sourcesThe runtime behavior is fine, but the current signatures conflict with PEP 484 (using
List[...]with aNonedefault) and Ruff’s hints. You can clarify intent and quiet the lints by annotating the mutable class attribute and making the optionals explicit:-from typing import Dict, List, Optional, Any, Tuple +from typing import Dict, List, Optional, Any, Tuple, ClassVar @@ - SOURCE_NPM = 'npm' - DEFAULT_SOURCES = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] + SOURCE_NPM = 'npm' + DEFAULT_SOURCES: ClassVar[List[str]] = [SOURCE_APT, SOURCE_PIP, SOURCE_NPM] @@ - def detect_installed_packages(self, sources: List[str] = None) -> List[Dict[str, Any]]: + def detect_installed_packages( + self, + sources: Optional[List[str]] = None, + ) -> List[Dict[str, Any]]: @@ - def export_configuration(self, - output_path: str, - include_hardware: bool = True, - include_preferences: bool = True, - package_sources: List[str] = None) -> str: + def export_configuration( + self, + output_path: str, + include_hardware: bool = True, + include_preferences: bool = True, + package_sources: Optional[List[str]] = None, + ) -> str:This keeps behavior identical while improving type clarity and matching the project’s Python 3.8+ target.
Also applies to: 157-170, 250-254
250-312: Consider basic validation or normalization ofoutput_pathbefore writing
export_configurationwrites directly tooutput_path(creating parent directories) with no validation. For typical CLI usage that’s fine, but it weakens the “path traversal protection” story ifoutput_pathever comes from untrusted input or higher-level tooling.At minimum, consider:
- Normalizing the path (
resolve()), and- Optionally rejecting obviously suspicious patterns when the caller indicates the path is user-supplied (e.g., starting with
/when you intend to stay under a base dir, or containing..when you don’t expect it).One possible lightweight helper:
+ def _normalize_output_path(self, output_path: str) -> Path: + """Normalize and return a safe-ish output path. + Currently just resolves the path; callers can layer stricter policies + (e.g., enforcing a base directory) if needed. + """ + return Path(output_path).expanduser().resolve() @@ - try: - output_path_obj = Path(output_path) + try: + output_path_obj = self._normalize_output_path(output_path) output_path_obj.parent.mkdir(parents=True, exist_ok=True)If you don’t want to constrain users today, even a normalization helper keeps the door open to enforce a base directory later without changing the public API.
549-573: Import path handling could be hardened slightly, but overall flow is goodThe high-level import flow (load YAML → validate compatibility unless
force→dry_rundiff → selective import + summary) is well-structured and clear.If this API is ever used with untrusted
config_pathvalues (e.g., from a higher-level service), you might want a small helper to normalize and possibly restrict where configs are read from (similar in spirit to the export-path note):@@ - def import_configuration(self, - config_path: str, + def import_configuration( + self, + config_path: str, @@ - # Load configuration - try: - with open(config_path, 'r') as f: + # Load configuration + try: + config_file = Path(config_path).expanduser() + with open(config_file, 'r') as f: config = yaml.safe_load(f)For a local CLI this is mostly defense-in-depth, so feel free to defer if your threat model doesn’t include untrusted paths.
Also applies to: 582-603
700-717: Harden_validate_package_identifierto block pip/npm path-based installs from configsThe current regex allows path-like identifiers such as
../pkgor./pkg(e.g.,"../evil"matches the leading..plus/<name>group). For pip/npm, that means a config can trigger installs from arbitrary filesystem paths:
pip3 install ../pkgnpm install -g ../evilThis undercuts the stated “path traversal / command injection protection” and was already raised in a prior review.
You can fix this by:
- Explicitly rejecting obvious path/traversal patterns up front.
- Allowing a single
/only when validating npm package names (for@scope/pkg), not versions.- Passing a flag from
_install_packageto indicate when slashes are allowed.For example:
- def _validate_package_identifier(self, identifier: str) -> bool: + def _validate_package_identifier(self, identifier: str, allow_slash: bool = False) -> bool: @@ - # Allow standard characters plus exactly one forward slash for NPM scoped packages - # This prevents path traversal (../, ../../, etc.) while allowing @scope/package - return bool(re.match(r'^[a-zA-Z0-9._:@=+\-]+(/[a-zA-Z0-9._\-]+)?$', identifier)) + # Quickly reject obvious path-like strings and traversal patterns. + if identifier.startswith(('.', '/', '~')): + return False + if '..' in identifier or '/.' in identifier: + return False + + # Allow standard characters plus, optionally, one forward slash for NPM scoped packages. + pattern = ( + r'^[a-zA-Z0-9._:@=+\-]+(/[a-zA-Z0-9._\-]+)?$' + if allow_slash + else r'^[a-zA-Z0-9._:@=+\-]+$' + ) + return bool(re.match(pattern, identifier)) @@ - # Validate package identifiers to prevent command injection - if not self._validate_package_identifier(name): + # Validate package identifiers to prevent command injection and path-based installs + if not self._validate_package_identifier( + name, + allow_slash=source == self.SOURCE_NPM, + ): return False - if version and not self._validate_package_identifier(version): + if version and not self._validate_package_identifier(version): return FalseThis keeps scoped npm packages working while preventing configs from smuggling local filesystem paths into your installation commands.
Also applies to: 772-795
🧹 Nitpick comments (3)
src/config_manager.py (3)
614-664: Package & preference import behavior is robust; consider minor UX/logging tweaks
_import_packagescorrectly uses the diff to drive installs, continues on errors, and now distinguishes install/upgrade/downgrade in the summary._import_preferencesis straightforward and updatespreferences_updatedwhile capturing failures.Two small optional improvements:
You may want to log or print a brief message when
_install_packageraises an exception; currently only the exception text is appended intosummary['failed']for that package, which is fine but could be complemented with a debug log hook later.The docstring for
_import_packagesstill mentions only'installed', 'upgraded', 'failed'in the summary; update it to include'downgraded'for accuracy.These are minor polish items; the core logic looks good.
Also applies to: 665-699
846-870: CLI UX is clear; consider including downgrades in dry-run detailsThe CLI helpers (
_setup_argument_parser,_print_dry_run_results,_print_import_results,_handle_*) are well-structured and produce readable output.One small UX enhancement: dry-run output currently shows install/upgrade lists but not the actual downgrade list, even though
difftrackspackages_to_downgrade. Since downgrades are potentially risky, it may be worth surfacing them explicitly:@@ def _print_dry_run_results(result: Dict[str, Any]) -> None: - if diff['preferences_changed']: + if diff['packages_to_downgrade']: + print(f"\n⬇️ Packages to downgrade: {len(diff['packages_to_downgrade'])}") + _print_package_list(diff['packages_to_downgrade']) + + if diff['preferences_changed']: print(f"\n⚙️ Preferences to change: {len(diff['preferences_changed'])}")This keeps the behavior consistent with the diff command and the import summary while making downgrade impact more visible in previews.
Also applies to: 872-888, 928-946
1-6: Executable bit vs. shebang mismatch (EXE002) confirmedThe issue is valid:
src/config_manager.pyis executable (permissions:-rwxr-xr-x) but lacks a shebang. The file contains a CLI entry point at lines 948-974 with amain()function andif __name__ == '__main__':guard, indicating it's designed to be executable as a script.Choose one option:
- Add a shebang so it can be invoked directly:
-""" +#!/usr/bin/env python3 +""" Configuration Manager for Cortex Linux
- Or remove the executable bit in git so it's treated purely as a module and run via
python -m cortex.config_manageror an entrypoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: The file is executable but no shebang is present
(EXE002)
41-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
68-68: Starting a process with a partial executable path
(S607)
102-102: subprocess call: check for execution of untrusted input
(S603)
134-134: Starting a process with a partial executable path
(S607)
157-157: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
217-217: Consider moving this statement to an else block
(TRY300)
218-218: Do not catch blind exception: Exception
(BLE001)
232-233: try-except-pass detected, consider logging the exception
(S110)
232-232: Do not catch blind exception: Exception
(BLE001)
247-247: Do not catch blind exception: Exception
(BLE001)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
248-248: Avoid specifying long messages outside the exception class
(TRY003)
254-254: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
284-284: Do not catch blind exception: Exception
(BLE001)
309-309: Consider moving this statement to an else block
(TRY300)
310-310: Do not catch blind exception: Exception
(BLE001)
311-311: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
349-351: try-except-pass detected, consider logging the exception
(S110)
349-349: Do not catch blind exception: Exception
(BLE001)
399-399: Do not catch blind exception: Exception
(BLE001)
483-483: Consider moving this statement to an else block
(TRY300)
484-484: Do not catch blind exception: Exception
(BLE001)
540-540: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
571-571: Do not catch blind exception: Exception
(BLE001)
572-572: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
572-572: Avoid specifying long messages outside the exception class
(TRY003)
578-578: Avoid specifying long messages outside the exception class
(TRY003)
662-662: Do not catch blind exception: Exception
(BLE001)
663-663: Use explicit conversion flag
Replace with conversion flag
(RUF010)
697-697: Do not catch blind exception: Exception
(BLE001)
698-698: Use explicit conversion flag
Replace with conversion flag
(RUF010)
741-741: Consider moving this statement to an else block
(TRY300)
742-742: Do not catch blind exception: Exception
(BLE001)
767-767: subprocess call: check for execution of untrusted input
(S603)
768-768: Consider moving this statement to an else block
(TRY300)
769-769: Do not catch blind exception: Exception
(BLE001)
968-968: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
src/config_manager.py (3)
57-89: Package detection & deduplication logic looks solidThe APT, pip, and npm detectors are straightforward, bounded by sensible timeouts, and degrade gracefully when tools aren’t present. The deduplication by
(name, source)plus sort-by-name indetect_installed_packagesis a good balance of correctness and simplicity for downstream diffing.Also applies to: 90-122, 123-156, 171-191
313-361: Compatibility & version comparison behavior is reasonableThe compatibility checks (required keys, major-version equality, and “current minor >= config minor”) match the documented expectations, and the combination of
packaging.versionwith a numeric fallback in_simple_version_compareis a sensible robustness choice.The numeric extraction in
_simple_version_comparewith padding to equal length handles most simple schemes well, and the docstring clearly calls out its limitations. No changes needed here.Also applies to: 464-487, 488-547
718-771: Sandbox/direct install flows are acceptable; subprocess usage is appropriately constrainedBoth
_install_with_sandboxand_install_directbuild fixed command templates and rely on validatedname/version, which is the right approach for mitigating shell injection. Timeouts are set, and failures are surfaced via return codes.Once
_validate_package_identifieris hardened as above, the subprocess calls will be in good shape from an input-safety standpoint.
- Add ClassVar to typing imports - Annotate DEFAULT_SOURCES as ClassVar[List[str]] - Fix function signatures to use Optional[List[str]] for Python 3.8 compatibility - Harden _validate_package_identifier with allow_slash parameter - Explicitly reject path-like inputs (./pkg, ../pkg, /abs, ~user, .., /.) - Only allow slash for NPM scoped packages when allow_slash=True - Update call sites to pass allow_slash=True only for NPM package names
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/config_manager.py (1)
250-312: Add basic validation/sanitization foroutput_pathandconfig_pathwhen used in less-trusted contextsBoth
export_configuration(output_path) andimport_configuration(config_path) pass user-supplied paths directly toPath(...)/open(...). For the current CLI use this is acceptable, but if these APIs are ever called with paths derived from less-trusted input (e.g., higher-level tools or services), it becomes easy to accidentally read/write outside intended directories (e.g.,../../etc/passwd).To make the APIs safer-by-default without over-constraining CLI usage, consider a small helper that rejects obviously suspicious paths and normalizes others, then use it in both methods:
@@ class ConfigManager: @@ def export_configuration(self, output_path: str, @@ - if package_sources is None: + if package_sources is None: package_sources = self.DEFAULT_SOURCES @@ - try: - output_path_obj = Path(output_path) + try: + output_path_obj = Path(output_path).expanduser() + # Basic safety: reject parent-directory traversal segments + if any(part == ".." for part in output_path_obj.parts): + raise ValueError("Parent-directory segments ('..') are not allowed in output_path") + output_path_obj.parent.mkdir(parents=True, exist_ok=True) @@ def import_configuration(self, config_path: str, @@ - # Load configuration - try: - with open(config_path, 'r') as f: + # Load configuration + try: + config_path_obj = Path(config_path).expanduser() + if any(part == ".." for part in config_path_obj.parts): + raise ValueError("Parent-directory segments ('..') are not allowed in config_path") + + with open(config_path_obj, 'r') as f: config = yaml.safe_load(f)This aligns with earlier path-traversal feedback while keeping the behavior simple and predictable for direct CLI use.
Also applies to: 549-573
🧹 Nitpick comments (3)
src/config_manager.py (3)
549-589: Fail fast with a clearer error when the loaded YAML is not a mapping
yaml.safe_loadcan return non-dict types (e.g., list, string,None). In that case, later calls likevalidate_compatibility(config)(which usesconfig.get(...)) ordiff_configuration(config)will raise attribute errors, and the CLI will surface a generic message like"'list' object has no attribute 'get'".Adding an explicit structure check right after loading makes the failure mode clearer:
- try: - with open(config_path, 'r') as f: - config = yaml.safe_load(f) + try: + with open(config_path, 'r') as f: + config = yaml.safe_load(f) + if not isinstance(config, dict): + raise ValueError( + f"Configuration root must be a mapping/object, got {type(config).__name__}" + )This keeps the existing
RuntimeError("Failed to load configuration file: ...")path but provides users with a much more actionable message when the YAML shape is wrong.
403-462: Surface downgrades more explicitly in dry‑run output
diff_configurationcorrectly trackspackages_to_downgradeand adds a warning line, and_print_import_resultsprints a separate downgraded count. In_print_dry_run_results, downgrades are only visible via the generic warnings section, which makes them easy to miss in large diffs.For better UX and consistency with real imports, consider rendering downgrades similarly to upgrades in the dry‑run output, for example:
@@ def _print_dry_run_results(result: Dict[str, Any]) -> None: - if diff['preferences_changed']: + if diff['packages_to_downgrade']: + print(f"\n⬇️ Packages to downgrade: {len(diff['packages_to_downgrade'])}") + _print_package_list(diff['packages_to_downgrade']) + + if diff['preferences_changed']: print(f"\n⚙️ Preferences to change: {len(diff['preferences_changed'])}")This doesn’t change any core logic, but makes downgrade actions as visible during dry runs as they are in the final import summary.
Also applies to: 859-883
1-6: Resolve Ruff EXE002 by either adding a shebang or dropping the executable bitStatic analysis reports this file as executable but without a shebang. Since it already defines a
main()andif __name__ == '__main__': main(), you can resolve EXE002 in one of two ways:
- If you intend direct execution (
./config_manager.py): add a shebang at the top of the file:-""" +#!/usr/bin/env python3 +""" Configuration Manager for Cortex Linux Handles export/import of system state for reproducibility.
- If you only expect
python -m cortex.config_manageror library use: leave the code as-is and remove the executable bit from the file in git (chmod -x src/config_manager.py).Either approach will satisfy the linter and make the intended usage clearer.
Also applies to: 961-987
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: The file is executable but no shebang is present
(EXE002)
68-68: Starting a process with a partial executable path
(S607)
102-102: subprocess call: check for execution of untrusted input
(S603)
134-134: Starting a process with a partial executable path
(S607)
217-217: Consider moving this statement to an else block
(TRY300)
218-218: Do not catch blind exception: Exception
(BLE001)
232-233: try-except-pass detected, consider logging the exception
(S110)
232-232: Do not catch blind exception: Exception
(BLE001)
247-247: Do not catch blind exception: Exception
(BLE001)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
248-248: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Do not catch blind exception: Exception
(BLE001)
309-309: Consider moving this statement to an else block
(TRY300)
310-310: Do not catch blind exception: Exception
(BLE001)
311-311: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
349-351: try-except-pass detected, consider logging the exception
(S110)
349-349: Do not catch blind exception: Exception
(BLE001)
399-399: Do not catch blind exception: Exception
(BLE001)
483-483: Consider moving this statement to an else block
(TRY300)
484-484: Do not catch blind exception: Exception
(BLE001)
540-540: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
571-571: Do not catch blind exception: Exception
(BLE001)
572-572: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
572-572: Avoid specifying long messages outside the exception class
(TRY003)
578-578: Avoid specifying long messages outside the exception class
(TRY003)
662-662: Do not catch blind exception: Exception
(BLE001)
663-663: Use explicit conversion flag
Replace with conversion flag
(RUF010)
697-697: Do not catch blind exception: Exception
(BLE001)
698-698: Use explicit conversion flag
Replace with conversion flag
(RUF010)
752-752: Consider moving this statement to an else block
(TRY300)
753-753: Do not catch blind exception: Exception
(BLE001)
778-778: subprocess call: check for execution of untrusted input
(S603)
779-779: Consider moving this statement to an else block
(TRY300)
780-780: Do not catch blind exception: Exception
(BLE001)
981-981: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
src/config_manager.py (2)
614-664: Package import flow and summary accounting look consistent and robustThe flow
_import_packages→_install_package→_install_with_sandbox/_install_directis coherent:
diff_configurationcategorizes install/upgrade/downgrade correctly._import_packagesmerges all three lists, updatesinstalled/upgraded/downgradedinsummary, and keeps going even when individual installs fail._install_packageuses_validate_package_identifierbefore constructing commands, and all subprocess calls are via argument lists or a sandboxed executor, avoiding shell injection.This is a good balance between safety, resilience, and clear reporting.
Also applies to: 783-809
700-809: Identifier validation effectively blocks path‑based pip/npm installs while preserving scoped packagesThe updated
_validate_package_identifierand its use in_install_packagenow:
- Reject path-like patterns (
./,../,/,~,..,/.) up front.- Allow exactly one
/only whenallow_slash=True, which you correctly apply only to npm package names (for@scope/package), not versions.- Restrict remaining characters to a safe alphanumeric set plus a small set of benign punctuation.
This directly addresses the risk of configs smuggling local filesystem paths into
pip3 install/npm install -gwhile keeping legitimate scoped and versioned packages working.Nice hardening step without complicating the call sites.
- Add _enforce_directory_security() method to check and fix directory security - Verify directory ownership matches current process uid/gid - Attempt to fix ownership with os.chown() if privileges allow - Enforce mode 0o700 (owner-only access) with os.chmod() - Verify chmod succeeded by re-checking actual permissions - Raise PermissionError with detailed message if security invariants fail - Run security checks even when directory pre-exists - Prevents security vulnerabilities from incorrect directory permissions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config_manager.py (1)
290-301: Add exception chaining for better error context.Line 301 raises a RuntimeError but doesn't chain the original exception. This loses the stack trace and makes debugging harder.
Apply this diff:
except Exception as e: - raise RuntimeError(f"Failed to save preferences: {e}") + raise RuntimeError(f"Failed to save preferences: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (2)
HardwareProfiler(15-439)profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: The file is executable but no shebang is present
(EXE002)
85-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-89: Avoid specifying long messages outside the exception class
(TRY003)
98-102: Avoid specifying long messages outside the exception class
(TRY003)
106-108: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
106-108: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Starting a process with a partial executable path
(S607)
155-155: subprocess call: check for execution of untrusted input
(S603)
187-187: Starting a process with a partial executable path
(S607)
270-270: Consider moving this statement to an else block
(TRY300)
271-271: Do not catch blind exception: Exception
(BLE001)
285-286: try-except-pass detected, consider logging the exception
(S110)
285-285: Do not catch blind exception: Exception
(BLE001)
300-300: Do not catch blind exception: Exception
(BLE001)
301-301: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Do not catch blind exception: Exception
(BLE001)
362-362: Consider moving this statement to an else block
(TRY300)
363-363: Do not catch blind exception: Exception
(BLE001)
364-364: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
364-364: Avoid specifying long messages outside the exception class
(TRY003)
402-404: try-except-pass detected, consider logging the exception
(S110)
402-402: Do not catch blind exception: Exception
(BLE001)
452-452: Do not catch blind exception: Exception
(BLE001)
536-536: Consider moving this statement to an else block
(TRY300)
537-537: Do not catch blind exception: Exception
(BLE001)
593-593: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
624-624: Do not catch blind exception: Exception
(BLE001)
625-625: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
625-625: Avoid specifying long messages outside the exception class
(TRY003)
631-631: Avoid specifying long messages outside the exception class
(TRY003)
715-715: Do not catch blind exception: Exception
(BLE001)
716-716: Use explicit conversion flag
Replace with conversion flag
(RUF010)
750-750: Do not catch blind exception: Exception
(BLE001)
751-751: Use explicit conversion flag
Replace with conversion flag
(RUF010)
805-805: Consider moving this statement to an else block
(TRY300)
806-806: Do not catch blind exception: Exception
(BLE001)
831-831: subprocess call: check for execution of untrusted input
(S603)
832-832: Consider moving this statement to an else block
(TRY300)
833-833: Do not catch blind exception: Exception
(BLE001)
1034-1034: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
src/config_manager.py (1)
18-1040: Overall implementation is solid and well-structured.The ConfigManager implementation is comprehensive with good security controls:
- ✅ Path traversal protection for package identifiers
- ✅ Directory permission enforcement (0o700)
- ✅ Command injection prevention via validation
- ✅ Downgrade tracking in import summary
- ✅ Version compatibility checking
- ✅ Robust error handling with timeouts
The code follows the PR objectives and acceptance criteria. Documentation is thorough with detailed docstrings.
- Display packages_to_downgrade section in _print_dry_run_results() - Show downgrade count and package list consistently with install/upgrade - Use ⬇️ emoji for visual consistency with import results
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config_manager.py (1)
84-89: Use exception chaining for clearer traceback context.The code raises
PermissionErrorwithout chaining the original exception, which can obscure the root cause in stack traces.Apply this pattern for better exception chaining:
except PermissionError: - raise PermissionError( + raise PermissionError( f"Directory {directory} is owned by uid={stat_info.st_uid}, " f"gid={stat_info.st_gid}, but process is running as uid={current_uid}, " f"gid={current_gid}. Insufficient privileges to change ownership." - ) + ) from NoneAnd at line 106-108:
if isinstance(e, PermissionError): raise - raise PermissionError( + raise PermissionError( f"Failed to enforce security on {directory}: {e}" - ) + ) from eAlso applies to: 106-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_manager.py (2)
src/hwprofiler.py (1)
profile(399-426)src/sandbox_executor.py (1)
success(52-54)
🪛 Ruff (0.14.5)
src/config_manager.py
1-1: The file is executable but no shebang is present
(EXE002)
85-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-89: Avoid specifying long messages outside the exception class
(TRY003)
98-102: Avoid specifying long messages outside the exception class
(TRY003)
106-108: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
106-108: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Starting a process with a partial executable path
(S607)
155-155: subprocess call: check for execution of untrusted input
(S603)
187-187: Starting a process with a partial executable path
(S607)
270-270: Consider moving this statement to an else block
(TRY300)
271-271: Do not catch blind exception: Exception
(BLE001)
285-286: try-except-pass detected, consider logging the exception
(S110)
285-285: Do not catch blind exception: Exception
(BLE001)
300-300: Do not catch blind exception: Exception
(BLE001)
301-301: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Do not catch blind exception: Exception
(BLE001)
362-362: Consider moving this statement to an else block
(TRY300)
363-363: Do not catch blind exception: Exception
(BLE001)
364-364: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
364-364: Avoid specifying long messages outside the exception class
(TRY003)
402-404: try-except-pass detected, consider logging the exception
(S110)
402-402: Do not catch blind exception: Exception
(BLE001)
452-452: Do not catch blind exception: Exception
(BLE001)
536-536: Consider moving this statement to an else block
(TRY300)
537-537: Do not catch blind exception: Exception
(BLE001)
593-593: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
624-624: Do not catch blind exception: Exception
(BLE001)
625-625: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
625-625: Avoid specifying long messages outside the exception class
(TRY003)
631-631: Avoid specifying long messages outside the exception class
(TRY003)
715-715: Do not catch blind exception: Exception
(BLE001)
716-716: Use explicit conversion flag
Replace with conversion flag
(RUF010)
750-750: Do not catch blind exception: Exception
(BLE001)
751-751: Use explicit conversion flag
Replace with conversion flag
(RUF010)
805-805: Consider moving this statement to an else block
(TRY300)
806-806: Do not catch blind exception: Exception
(BLE001)
831-831: subprocess call: check for execution of untrusted input
(S603)
832-832: Consider moving this statement to an else block
(TRY300)
833-833: Do not catch blind exception: Exception
(BLE001)
1038-1038: Do not catch blind exception: Exception
(BLE001)
|
@mikejmorgan-ai Please review |
Code Review FeedbackThanks for this contribution! Found some issues that need addressing before merge: 🔴 Critical (Must Fix)
🟡 High Priority
🟢 Medium (Can fix in follow-up)
Please address the Critical and High priority items, then I'll approve. Let me know if you need help! |
2 participants
Configuration Management System
Implements configuration export/import functionality for system reproducibility and team collaboration.
Closes #33
What's Included
Core Features
Package Management
Safety Features
Testing
Unit Tests
Manual Testing
Tested on Ubuntu 24.04 (GitHub Codespaces):
Documentation
Created comprehensive documentation:
Code Quality
Applied standard best practices:
packaginglibrary for robust version comparisonSecurity
Input validation:
Safe execution:
File permissions:
Usage
Export:
Import:
Team workflow:
Checklist
Files
src/config_manager.py(922 lines) - main implementationsrc/test_config_manager.py(683 lines) - test suiteCONFIGURATION.md(589 lines) - user documentationexamples/sample-config.yaml(74 lines) - example config.gitignore(49 lines) - git ignore rulessrc/requirements.txt(+2 dependencies: pyyaml, packaging)Total: 2,317 lines added
Ready for review. All tests passing.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores