Feature: Interactive Package Conflict Resolution + Saved Preferences(Issue #42) #203
Conversation
|
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 interactive package conflict detection/resolution to the Cortex CLI with persistent preference storage, a PreferencesManager (JSON/YAML + backup/import/export), a config subcommand, dependency-resolver integration in install flow, unit tests for UI/persistence, .gitignore updates, and a dev dependency on PyYAML. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant Resolver as DependencyResolver
participant Prefs as PreferencesManager
participant FS as Config Storage
User->>CLI: cortex install pkgA pkgB
CLI->>Resolver: detect_conflicts([pkgA,pkgB])
Resolver-->>CLI: conflicts list
alt No conflicts
CLI->>CLI: proceed with install
else Conflicts detected
CLI->>Prefs: get(conflict_key)
Prefs->>FS: read preferences file
FS-->>Prefs: saved_resolutions?
Prefs-->>CLI: saved resolution or None
alt Saved resolution exists
CLI->>CLI: apply saved preference (modify install plan)
else No saved resolution
CLI->>User: prompt _resolve_conflicts_interactive()
User->>CLI: selects (keep A / keep B / skip)
CLI->>CLI: build adjusted commands (inject removals if needed)
CLI->>User: ask to save preference (_ask_save_preference)
User->>CLI: confirm/save
CLI->>Prefs: set(conflict_key, resolution)
Prefs->>FS: save (create backup)
end
end
CLI->>User: [SUCCESS] Installation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request implements interactive package conflict resolution UI and persistent conflict-resolution preferences to address Issue #42. The PR includes significant additions to the codebase including comprehensive test coverage, documentation, and various automation scripts.
Key highlights:
- Adds interactive conflict resolution functionality with saved preferences
- Includes extensive test coverage (500+ lines of user preferences tests)
- Updates import paths across multiple test files for proper package structure
- Improves security with ReDoS-resistant regex patterns
- Adds comprehensive documentation and automation scripts
Reviewed changes
Copilot reviewed 21 out of 60 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/test_user_preferences.py |
New comprehensive test suite (501 lines) for user preferences system with 50+ test cases |
test/test_conflict_ui.py |
New test suite for interactive conflict resolution (76 lines, 5 tests) |
test/test_logging_system.py |
Updated import paths to use cortex. package structure |
test/test_llm_router.py |
Updated import paths and mock decorators for new structure |
test/test_installation_verifier.py |
Updated import paths for proper package imports |
test/test_installation_history.py |
Updated import paths for cortex package |
test/test_error_parser.py |
Updated import paths to cortex namespace |
test/test_context_memory.py |
Updated import paths with proper package structure |
cortex/installation_history.py |
Security improvements: ReDoS-resistant regex, clarified MD5 usage for non-cryptographic IDs |
cortex/dependency_resolver.py |
Security improvements: ReDoS-resistant regex patterns for version constraint removal |
scripts/* |
Multiple automation scripts for GitHub PR management, contributor onboarding, and bounty tracking |
docs/* |
Extensive new documentation including user guides, implementation summaries, API docs, and FAQs |
data/contributors.json |
New empty contributors tracking file |
bounties_pending.json, payments_history.json, pr_status.json, issue_status.json |
Removed tracking files (likely moved to gitignore) |
Comments suppressed due to low confidence (8)
cortex/installation_history.py:177
- Good security improvement: The regex pattern has been changed from
\(.*?\)to\([^)]*\)to prevent ReDoS (Regular Expression Denial of Service) attacks. The new pattern avoids nested quantifiers and backtracking, making it safer. This is a security best practice.
cortex/installation_history.py:256 - Good security clarification: The comment correctly clarifies that MD5 is being used for non-cryptographic purposes (generating unique IDs), not for security. This is appropriate for this use case. MD5 is fast and sufficient for collision-resistant ID generation when security is not a concern.
cortex/dependency_resolver.py:152 - Good security improvement: Similar to the other file, the regex pattern has been updated from
\(.*?\)to\([^)]*\)to prevent ReDoS attacks. This is a consistent security improvement across the codebase.
test/test_context_memory.py:24 - Import of 'Pattern' is not used.
Import of 'Suggestion' is not used.
test/test_error_parser.py:16 - Import of 'ErrorAnalysis' is not used.
test/test_installation_history.py:20 - Import of 'PackageSnapshot' is not used.
Import of 'InstallationRecord' is not used.
test/test_installation_verifier.py:16 - Import of 'VerificationTest' is not used.
test/test_llm_router.py:25 - Import of 'RoutingDecision' is not used.
💡 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: 0
🧹 Nitpick comments (14)
test/test_installation_verifier.py (1)
73-73: Remove redundant import.The
osmodule is already imported at the module level (line 8), making this import statement redundant.Apply this diff:
- import osdocs/USER_PREFERENCES_IMPLEMENTATION.md (1)
48-53: Minor Markdown hygiene for better tooling supportThe doc is thorough and matches the implemented system. A few small cleanups would make linters happier and improve readability:
- Add explicit languages to “fenced” code blocks that currently have none (e.g., the
~/.config/cortex/preferences.yamland file-structure snippets) to satisfy MD040.- Consider wrapping bare URLs in Markdown links (e.g.,
[link](https://...)) for the references section.- Optional: some style guides prefer
November 18, 2025,with a trailing comma; you can adjust or ignore depending on your house style.Also applies to: 356-365, 671-684, 736-737, 721-723
test/test_conflict_ui.py (1)
7-74: Conflict UI tests provide good coverage; only minor polish opportunitiesThe test scenarios align well with
_resolve_conflicts_interactiveand_ask_save_preference:
- Keep-first/keep-second paths assert the correct package ends up in
resolutions['remove']and that user-facing messages are printed.- Cancel path asserts
SystemExitand checks the cancellation message.- Save-preference test verifies
conflicts.saved_resolutionsis written with the sorted key and thatsave()is called.- Saved-preference test confirms the fast-path is used without prompting, and the output reflects the stored choice.
Only optional cleanups:
- Several patched arguments (
mock_input,mock_stdout) are not referenced directly; you could rename them to_mock_input/_mock_stdoutto quiet linters without changing behavior.- If you want to avoid constructing a real
CortexCLIin tests, you could inject a minimal stub just for conflict resolution, but this is not necessary given current scope.cortex/cli.py (5)
23-33: Preference manager initialization is reasonable; consider logging on load failureInitializing
PreferencesManagerin__init__and attempting a best-effortload()makes sense so the rest of the CLI can assume preferences are present. Swallowing all exceptions keeps the CLI usable if the config is corrupt or unreadable, but completely silent failures may make diagnosing broken configs harder.Consider logging a brief warning (to stderr or via a logger) when
load()fails, ideally including the config path, while still proceeding with defaults.Also applies to: 34-49
34-49: Credentials helpers are currently unused
_cred_path()and_load_creds()are defined but not yet wired into_get_api_key()or any other flow. If credential files are part of a planned follow-up, this is fine; otherwise, you might either:
- Integrate
_load_creds()into_get_api_key()as a secondary source after env vars, or- Remove these helpers for now to avoid dead code.
84-145: Interactive conflict resolution + saved preferences logic looks soundThe conflict UI and preference-saving logic are coherent and match the tests and
ConflictSettingsschema:
- Saved resolutions are keyed using
f"{min(pkg1, pkg2)}:{max(pkg1, pkg2)}", ensuring that a conflict betweenpkgA/pkgBis recognized regardless of order.- When a saved preference is present, the code computes the package to remove and prints a clear “Using saved preference: …” message without prompting, which is what the tests assert.
- New choices (1/2) correctly add the opposite package to
resolutions['remove']and delegate to_ask_save_preferenceif selected.- Choice
3cancels the installation viasys.exit(1), which matches the test expectations.Given this is used in an interactive CLI, exiting from here is acceptable, but if you ever want to reuse
_resolve_conflicts_interactiveprogrammatically (e.g., from a higher-level orchestration layer), you may eventually prefer raising a custom exception instead of callingsys.exit()directly.
159-223: DependencyResolver integration for conflict checks is safe but could log resolver failuresThe install flow’s conflict-check integration is conservative:
- It uses a simple heuristic (
target_package = software.split()[0]) to pick a package to resolve, which is acceptable as a first cut for the UI demo.- If
DependencyResolver.resolve_dependencies()finds conflicts, they’re passed through the interactive resolver and any chosen removals are prepended asapt-get removecommands unless already present.- Broadly catching
Exceptionaround the resolver and silently continuing avoids breaking installs when the resolver can’t handle a package, but you lose diagnostic visibility.Consider logging a warning when resolver calls fail (e.g., missing apt-cache data or unsupported packages), so users and developers can see why conflict checks were skipped while still proceeding with the install.
Also applies to: 204-223
402-513: Config subcommand behavior and value parsing are coherent; consider surfacing validation on setThe
config()method plus_parse_config_value()provide a straightforward interface:
listuseslist_all()and prints YAML plus config metadata.get/set/reset/validate/info/export/importmap cleanly onto thePreferencesManagerAPI._parse_config_value()handles booleans (true/false-like strings), ints, and simple comma-separated lists, and otherwise returns a string—sufficient for current preference types.Two optional improvements:
- After a successful
set, you could optionally runvalidate()and warn if the new value makes the config invalid (e.g.,auto_update.frequency_hours = 0), instead of relying on a separateconfig validatestep.- For
resetwithout a key, you currently ask for confirmation viainput("Continue? (y/n): "). That’s good; just be aware this introduces interactivity intoconfig resetthat might surprise scripts. A--forceflag at the argparse level could be a future enhancement if scripting becomes common.Overall, the implementation matches the documented CLI usage.
Also applies to: 514-533
cortex/user_preferences.py (4)
156-258: load()/save() behavior and backup handling are reasonable
PreferencesManager.load():
- Creates a default
UserPreferencesobject and immediately saves it if the config file doesn’t exist, ensuring subsequent operations always have a concrete config.- Uses
yaml.safe_load()and handles empty files by treating them as{}, which is robust.
save():
- Requires
_preferencesto be non-None (enforcing aload()first).- Optionally creates a timestamped backup using
with_suffix(".backup.<timestamp>")when a config already exists.- Dumps YAML with stable formatting (no flow style, no key sorting).
This is a solid, conservative approach. One optional enhancement would be to narrow the broad
except Exceptioninload()/save()-related helpers (_create_backup,load) to more specific exceptions where practical, but it’s not strictly necessary.
259-387: Dot-notation get/set/reset behavior is consistent; conflicts.saved_resolutions is handled correctlyKey points:
get()walks theUserPreferences.to_dict()structure using dot notation and returns the provideddefaultif any segment is missing. This is what the CLI and conflict UI expect.set()validates per top-level section:
verbosityandai.creativityrestricted to known enum values.- Type checks for booleans/ints/lists/dicts as appropriate (e.g.,
auto_update.frequency_hours,ai.max_suggestions,packages.default_sources,conflicts.saved_resolutions).- For
conflicts.saved_resolutions, requiring adictlines up with how_ask_save_preference()passes in a Python dict of normalized keys to package names.reset()either:
- Replaces the entire preferences object with defaults and saves, or
- For specific keys, derives the default value from a fresh
UserPreferences().to_dict()and then callsset(key, default_value)followed bysave().This all matches how
cortex.cli.configuses the manager. Only minor caveat: resetting a non-leaf key like"ai"would currently fail (sinceset()expects"ai.something"), but the documented and tested flows all use leaf keys (ai.model, etc.), so this is acceptable.
389-423: Validation rules are sensible and match the documented configuration optionsThe
validate()method checks:
verbosityis within theVerbosityLevelenum.ai.creativityis withinAICreativity.ai.modelis one of["claude-sonnet-4", "gpt-4", "gpt-4-turbo", "claude-3-opus"], which matches the docs.auto_update.frequency_hours >= 1.- At least one package source in
packages.default_sources.This provides clear, actionable error messages and is sufficient for the current config surface. If/when you expand models or strategies (e.g.
conflicts.default_strategybeyond"interactive"), this is a good place to enforce those constraints as well.
498-535: Embedded demo main() is fine but could be gated or moved laterThe
main()function at the bottom offers a handy demonstration of the preferences manager. It’s harmless in normal use (only runs underpython -m cortex.user_preferences), but in some projects, such demos are moved to separate examples or guarded in documentation.No need to change this now; just something to consider if the module grows further or if packaging policies prefer modules without executable demos.
test/test_user_preferences.py (2)
1-5: Resolve Ruff EXE001 by dropping the shebang (or making the file executable).Ruff correctly notes the shebang on Line 1 but the file isn’t executable and is already invoked via
python -m unittest/ the__main__guard. Simplest fix is to remove the shebang so the module just starts with the docstring.-#!/usr/bin/env python3 -""" -Unit tests for the User Preferences System -Tests all functionality of the PreferencesManager class -""" +""" +Unit tests for the User Preferences System +Tests all functionality of the PreferencesManager class +"""Also applies to: 499-501
132-415: PreferencesManager tests are thorough; consider adding coverage for conflicts.saved_resolutions.The suite here is strong: you cover default creation, load/save round‑trip, dot‑notation get/set, reset (all and specific), validation, backup creation, JSON export/import (valid and invalid), YAML parsing, basic concurrent access, invalid YAML, and directory creation. This gives very solid confidence in the manager’s behavior.
Given
PreferencesManager.sethas explicit handling forconflicts.saved_resolutions, it would be worth adding at least one positive round‑trip test (and optionally a type‑validation negative test) to keep the new conflict‑resolution preferences covered:class TestPreferencesManager(unittest.TestCase): @@ def test_import_invalid_json(self): """Test importing invalid JSON raises error""" @@ with self.assertRaises(ValueError): self.manager.import_json(import_path) + + def test_conflicts_saved_resolutions_roundtrip(self): + """Test saving and reloading conflicts.saved_resolutions""" + self.manager.load() + resolutions = {"example_conflict": "example_resolution"} + self.manager.set("conflicts.saved_resolutions", resolutions) + self.manager.save() + + reloaded = PreferencesManager(config_path=self.config_path) + reloaded.load() + self.assertEqual( + reloaded.get("conflicts.saved_resolutions"), + resolutions, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bounties_owed.csvis excluded by!**/*.csv
📒 Files selected for processing (21)
.gitignore(1 hunks)LLM/requirements.txt(1 hunks)bounties_pending.json(0 hunks)cortex/cli.py(12 hunks)cortex/dependency_resolver.py(2 hunks)cortex/installation_history.py(2 hunks)cortex/user_preferences.py(1 hunks)deploy_jesse_system (1).sh(0 hunks)deploy_jesse_system.sh(0 hunks)docs/USER_PREFERENCES_IMPLEMENTATION.md(1 hunks)issue_status.json(0 hunks)payments_history.json(0 hunks)pr_status.json(0 hunks)test/test_conflict_ui.py(1 hunks)test/test_context_memory.py(1 hunks)test/test_error_parser.py(1 hunks)test/test_installation_history.py(1 hunks)test/test_installation_verifier.py(1 hunks)test/test_llm_router.py(10 hunks)test/test_logging_system.py(1 hunks)test/test_user_preferences.py(1 hunks)
💤 Files with no reviewable changes (6)
- bounties_pending.json
- payments_history.json
- deploy_jesse_system (1).sh
- deploy_jesse_system.sh
- pr_status.json
- issue_status.json
🧰 Additional context used
🧬 Code graph analysis (4)
cortex/cli.py (3)
cortex/installation_history.py (1)
InstallationHistory(68-659)cortex/user_preferences.py (10)
PreferencesManager(144-495)load(200-226)get(259-282)save(228-257)list_all(485-495)get_config_info(465-483)reset(358-387)validate(389-422)export_json(424-440)import_json(442-463)cortex/dependency_resolver.py (2)
DependencyResolver(39-401)resolve_dependencies(217-282)
test/test_user_preferences.py (1)
cortex/user_preferences.py (24)
PreferencesManager(144-495)UserPreferences(96-141)VerbosityLevel(21-26)AICreativity(29-33)ConfirmationSettings(37-45)AutoUpdateSettings(49-56)AISettings(60-70)PackageSettings(74-82)to_dict(44-45)to_dict(55-56)to_dict(69-70)to_dict(81-82)to_dict(91-92)to_dict(108-120)from_dict(123-141)load(200-226)save(228-257)get(259-282)reset(358-387)validate(389-422)export_json(424-440)import_json(442-463)get_config_info(465-483)list_all(485-495)
test/test_conflict_ui.py (2)
cortex/cli.py (3)
CortexCLI(23-533)_resolve_conflicts_interactive(84-133)main(536-615)cortex/user_preferences.py (3)
get(259-282)save(228-257)main(498-531)
cortex/user_preferences.py (1)
cortex/cli.py (1)
main(536-615)
🪛 LanguageTool
docs/USER_PREFERENCES_IMPLEMENTATION.md
[style] ~737-~737: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...** 1.0 Last Updated: November 18, 2025 Author: Development Team **Revi...
(MISSING_COMMA_AFTER_YEAR)
🪛 markdownlint-cli2 (0.18.1)
docs/USER_PREFERENCES_IMPLEMENTATION.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
356-356: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
721-721: Bare URL used
(MD034, no-bare-urls)
722-722: Bare URL used
(MD034, no-bare-urls)
723-723: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.5)
cortex/cli.py
30-32: try-except-pass detected, consider logging the exception
(S110)
30-30: Do not catch blind exception: Exception
(BLE001)
46-48: try-except-pass detected, consider logging the exception
(S110)
46-46: Do not catch blind exception: Exception
(BLE001)
219-222: try-except-pass detected, consider logging the exception
(S110)
219-219: Do not catch blind exception: Exception
(BLE001)
510-510: Do not catch blind exception: Exception
(BLE001)
511-511: Use explicit conversion flag
Replace with conversion flag
(RUF010)
test/test_user_preferences.py
1-1: Shebang is present but file is not executable
(EXE001)
test/test_conflict_ui.py
18-18: Unused method argument: mock_input
(ARG002)
29-29: Unused method argument: mock_input
(ARG002)
39-39: Unused method argument: mock_input
(ARG002)
48-48: Unused method argument: mock_stdout
(ARG002)
48-48: Unused method argument: mock_input
(ARG002)
cortex/user_preferences.py
1-1: Shebang is present but file is not executable
(EXE001)
196-196: Consider moving this statement to an else block
(TRY300)
197-197: Do not catch blind exception: Exception
(BLE001)
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Use explicit conversion flag
Replace with conversion flag
(RUF010)
221-221: Consider moving this statement to an else block
(TRY300)
224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
224-224: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Use explicit conversion flag
Replace with conversion flag
(RUF010)
225-225: Do not catch blind exception: Exception
(BLE001)
226-226: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
226-226: Avoid specifying long messages outside the exception class
(TRY003)
226-226: Use explicit conversion flag
Replace with conversion flag
(RUF010)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
254-254: Consider moving this statement to an else block
(TRY300)
256-256: Do not catch blind exception: Exception
(BLE001)
257-257: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
257-257: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Use explicit conversion flag
Replace with conversion flag
(RUF010)
304-304: Abstract raise to an inner function
(TRY301)
304-304: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Abstract raise to an inner function
(TRY301)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Abstract raise to an inner function
(TRY301)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
316-316: Abstract raise to an inner function
(TRY301)
316-316: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Abstract raise to an inner function
(TRY301)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
320-320: Abstract raise to an inner function
(TRY301)
320-320: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Abstract raise to an inner function
(TRY301)
325-325: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Abstract raise to an inner function
(TRY301)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
330-330: Abstract raise to an inner function
(TRY301)
330-330: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Abstract raise to an inner function
(TRY301)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Abstract raise to an inner function
(TRY301)
337-337: Avoid specifying long messages outside the exception class
(TRY003)
342-342: Abstract raise to an inner function
(TRY301)
342-342: Avoid specifying long messages outside the exception class
(TRY003)
344-344: Abstract raise to an inner function
(TRY301)
344-344: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Abstract raise to an inner function
(TRY301)
351-351: Avoid specifying long messages outside the exception class
(TRY003)
353-353: Consider moving this statement to an else block
(TRY300)
356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
356-356: Avoid specifying long messages outside the exception class
(TRY003)
356-356: Use explicit conversion flag
Replace with conversion flag
(RUF010)
383-383: Avoid specifying long messages outside the exception class
(TRY003)
460-460: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (25)
test/test_installation_verifier.py (2)
7-10: LGTM! Import path setup is correct.The sys.path manipulation correctly enables importing the
cortexpackage from the test directory by adding the parent directory to the Python path.
12-12: Import path correctly updated.The change from
installation_verifiertocortex.installation_verifieraligns with the project's move to centralize modules under thecortexpackage..gitignore (1)
153-164: LGTM!The additions appropriately exclude generated artifacts (cleanup scripts, deployment scripts) and data files while preserving
contributors.json. The negation pattern!data/contributors.jsoncorrectly ensures that specific file is tracked.test/test_error_parser.py (1)
7-16: LGTM!The import path updates correctly standardize test imports to use the
cortex.*namespace, aligning with the broader repository restructuring.test/test_logging_system.py (1)
10-11: LGTM!The import path updates correctly align with the standardized
cortex.*namespace used across the test suite.test/test_context_memory.py (1)
16-24: LGTM!The import path updates correctly standardize module resolution to use the
cortex.*namespace, with a helpful comment explaining the sys.path adjustment.cortex/dependency_resolver.py (2)
150-152: Good security improvement!The regex change from
r'\s*\(.*?\)'tor'\s*\([^)]*\)'reduces ReDoS risk by using a negated character class instead of a quantifier, which is both safer and more performant.
166-167: Good security improvement!Consistent application of the safer regex pattern for version constraint removal.
test/test_llm_router.py (2)
15-25: LGTM!The import path updates correctly standardize module resolution to use the
cortex.*namespace.
247-247: LGTM!All patch decorators correctly updated to reference
cortex.llm_router.*, which is necessary for the mocks to target the correct module paths after the import restructuring.Also applies to: 279-279, 316-316, 351-351, 383-383, 425-426, 460-461, 502-502, 522-522
test/test_installation_history.py (1)
9-20: LGTM!The import path updates correctly align with the standardized
cortex.*namespace used across the test suite.LLM/requirements.txt (1)
3-3: PyYAML>=6.0 constraint is secure and appropriate.No widely reported direct security vulnerabilities affect PyYAML 6.0 or later, as known RCE issues were fixed in the 5.4+ line. The version constraint >=6.0 appropriately ensures that installed versions avoid historically vulnerable releases.
cortex/installation_history.py (2)
168-181: Dependency cleanup regex change looks correct and saferSwitching to
re.sub(r'\s*\([^)]*\)', '', dep)keeps the intended behavior (stripping version constraints likepkg (>= 1.2)) while avoiding potentially problematic patterns and aligns with similar resolver logic elsewhere. No functional issues spotted.
246-256: MD5-based ID generation is adequately documented for non-security useThe expanded docstring and inline comment make it clear MD5 is used only for non-cryptographic IDs, matching the actual usage. Given the timestamp + sorted package list and truncation to 16 hex chars, collision risk is acceptable for this context.
cortex/cli.py (4)
65-73: Standardized status and result messages improve UXSwitching to
_print_status,_print_error, and_print_success, and using consistent labels like[INFO],[SUCCESS], and[FAILED](including in theprogress_callbackand history messages) makes CLI output much easier to scan.No functional issues here; this is a solid readability/UX improvement.
Also applies to: 249-255, 270-299
135-145: Conflict preference persistence aligns with UserPreferences.conflicts.saved_resolutions
_ask_save_preference():
- Normalizes the conflict key using
min/max, matching the read path in_resolve_conflicts_interactive.- Retrieves the existing
conflicts.saved_resolutionsdict (or{}), updates it, and writes it back viaprefs_manager.set("conflicts.saved_resolutions", ...)followed byprefs_manager.save().This matches the
ConflictSettings.saved_resolutions: Dict[str, str]data model and the tests’ expectations for keys and values. No issues spotted.
224-245: History tracking integration remains correct after conflict handlingThe existing pattern of:
- Extracting packages from
commandsviahistory._extract_packages_from_commands(commands),- Recording the installation when
executeordry_runis enabled, and- Updating the record on success/failure or dry run
is preserved even with added conflict-removal commands inserted at the front. The additional
[INFO] Installation recorded (ID: …)messages in both success and failure paths are consistent and helpful.Also applies to: 236-237
550-555: Argparse wiring for the config subcommand is consistent and clearAdding the
configsubparser with the documented actions and wiring it throughmain()(including the updated epilog examples) is consistent with the rest of the CLI structure. Argument order (action, optionalkey, optionalvalue) matches howconfig()expects its parameters.No issues found here.
Also applies to: 582-589, 598-607
cortex/user_preferences.py (2)
21-107: Data models and defaults are coherent and align with documented configurationThe enums and dataclasses (
VerbosityLevel,AICreativity,ConfirmationSettings,AutoUpdateSettings,AISettings,PackageSettings,ConflictSettings,UserPreferences) are well-structured:
- Defaults match the documentation (e.g.,
verbosity=normal,ai.model="claude-sonnet-4", sensible confirmation and auto-update defaults).ConflictSettingsintroducesdefault_strategyandsaved_resolutionsin a way that matches howcortex.cli.CortexCLIreads/writesconflicts.saved_resolutions.UserPreferences.to_dict()andfrom_dict()provide a clean dict representation, suitable for YAML/JSON storage and CLI inspection.No structural issues identified here.
424-496: JSON import/export and config introspection align with the rest of the API
export_json()exports the current preferences viato_dict()to a JSON file, matching the YAML schema.import_json()loads JSON, constructs aUserPreferencesviafrom_dict(), validates it, and saves only if validation passes, which is a safe migration path.get_config_info()andlist_all()provide useful metadata and are exactly what the CLI’sconfig infoandconfig listcommands require.These operations appear correct and are coherent with the rest of the design.
test/test_user_preferences.py (5)
31-70: UserPreferences tests are aligned with the model and cover key paths.The defaults and
to_dict/from_dictassertions matchUserPreferencesand the enum values, and they exercise both serialization and partial override behavior; no changes needed here.
72-130: Dataclass default/serialization tests for settings look good.The tests for
ConfirmationSettings,AutoUpdateSettings,AISettings, andPackageSettingscorrectly assert defaults andto_dictstructure and give good safety against regressions in the config schema; nothing to fix.
417-432: Enum tests are precise and acceptable as-is.Verifying the exact
VerbosityLevelandAICreativitystring values is appropriate here and will catch accidental renames; just be aware these tests will need updating if new enum values or names are introduced.
434-476: Edge-case coverage for load/save behavior is solid.The tests for empty config files,
savewithout priorload, implicitloadonset, and nested key access nicely cover subtle behaviors around lazy initialization and yaml parsing. No issues spotted.
478-497: Customrun_tests()helper is fine but optional.The explicit test suite assembly and
run_tests()wrapper are correct and can be handy for local runs. Most runners will still rely on unittest discovery, so this is more of a convenience layer than a requirement; no changes needed.
|
🔥 CRITICAL PATH REVIEW Hi @Sahilbhatane! This PR is blocking our MVP completion. Urgent Review In Progress:
Payment Ready: Join Discord for payment coordination: We're prioritizing this merge! Thanks for the critical work. 🚀 |
|
@Sahilbhatane just keep files related to this issue in this PR. Don't need to restructure repository And add video implementation. And did you test it on your system? |
Sahilbhatane
force-pushed
the
issue-42
branch
from
64c8fbe to
3a775ca
Compare
video implementationUntitled.video.-.Made.with.Clipchamp.1.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
515-534: Epilog example usesllm.providerkey, which is not a defined preferenceThe CLI help shows:
cortex config set llm.provider openaiBut the preferences schema only defines keys like
ai.model,ai.creativity, etc., andPreferencesManager.set()will rejectllm.providerwith “Unknown preference key”.Update the example to a valid key (e.g.,
ai.model) or add anllmsection toUserPreferencesif that’s the intended schema.
♻️ Duplicate comments (1)
test/test_conflict_ui.py (1)
438-450: Aligntest_preference_validationwithPreferencesManager.validate()behaviorThis test assumes that setting
prefs.ai.max_suggestions = -999will causevalidate()to return at least one error. At present,validate()does not checkmax_suggestions, soerrorsremains empty and the assertion will fail.Once
validate()is extended to enforce a lower bound onmax_suggestions(e.g.,>= 1), this test will pass as written. If you decide not to constrainmax_suggestions, this test will need to be relaxed accordingly.
🧹 Nitpick comments (11)
cortex/user_preferences.py (2)
283-355:PreferencesManager.set()is very branch-heavy; consider splitting into helpersThe
set()method encodes all preference families (verbosity, confirmations, auto_update, ai, packages, conflicts, theme/language/timezone) in a single nestedif/elifchain, which Sonar flags for high cognitive complexity and will be hard to extend.Consider refactoring to dedicated helpers per top-level section, e.g.:
def set(self, key: str, value: Any) -> bool: if self._preferences is None: self.load() parts = key.split(".") if parts[0] == "verbosity": self._set_verbosity(value) elif parts[0] == "confirmations": self._set_confirmations(parts[1:], value) # ...This will make new preference sections (or extra validation rules) much easier to add and reason about.
193-199: Broadexcept Exceptionand re-wrapped errors hide root causesSeveral methods (
_create_backup,load,save,set,import_json) catch bareExceptionand immediately raise a newIOError/ValueErrorwith a stringified message, without preserving the original traceback (raise ... from e). This pattern makes debugging configuration problems harder and is what Ruff/Sonar are complaining about.It would be safer to:
- Narrow the caught exceptions where possible (e.g.,
OSError,yaml.YAMLError,json.JSONDecodeError).- Re-raise with
raise ... from eso the original stack is preserved.- Optionally log the error before rethrowing.
Functionality is fine as-is, but tightening this would improve diagnostics without changing behavior.
Also applies to: 222-225, 243-257, 353-355, 448-450
test/test_conflict_ui.py (1)
61-62: Remove unusedresultassignments inassertRaisesblocksIn
test_interactive_conflict_resolution_skipandtest_conflict_detected_triggers_ui,result = self.cli._resolve_conflicts_interactive(conflicts)is assigned but never used inside awith self.assertRaises(SystemExit)block.You can drop the assignment and just call the method:
- with self.assertRaises(SystemExit): - result = self.cli._resolve_conflicts_interactive(conflicts) + with self.assertRaises(SystemExit): + self.cli._resolve_conflicts_interactive(conflicts)This will clear the Ruff/Sonar warnings without changing test behavior.
Also applies to: 362-363
docs/IMPLEMENTATION_SUMMARY.md (1)
73-85: Minor markdownlint issues: add code fence languages and avoid emphasis-as-headingmarkdownlint is flagging:
- Fenced blocks without a language (e.g., the “Example Output” and workflow snippets).
- The “PR Title” line using bold emphasis instead of a proper heading.
To quiet this without changing meaning:
- Add languages, e.g.
text for console output blocks andbash for shell command examples.- Replace emphasized headings like
**"feat: ... "**with a Markdown heading (###), or leave as plain text.Purely cosmetic, but it will keep automated doc checks clean.
Also applies to: 219-235, 291-325
cortex/dependency_resolver.py (2)
226-246: Conflict detection can emit duplicate pairs for the same packages
conflict_patternsis symmetric for some packages (e.g., both'mysql-server': ['mariadb-server']and'mariadb-server': ['mysql-server']). With the current loop, a dependency set containing both can yield both('mysql-server', 'mariadb-server')and('mariadb-server', 'mysql-server').This can lead to duplicate conflict prompts in the CLI. Consider normalizing pairs (e.g.,
tuple(sorted((dep_name, conflicting)))) and storing them in asetbefore returning alist, so each conflict pair appears only once.
171-225:resolve_dependenciesis doing several distinct jobs and is over-complex
resolve_dependencies()currently:
- Fetches apt and predefined deps.
- Merges/deduplicates them.
- Optionally traverses transitive deps.
- Detects conflicts.
- Computes installation order.
- Populates and caches
DependencyGraph.Sonar’s complexity warning here is justified; future changes (e.g., more sources, richer conflict rules) will be hard to slot in. Consider extracting helpers like
_collect_direct_dependencies,_collect_transitive_dependencies, and_build_graphto keep this method focused on orchestration.cortex/cli.py (5)
31-38: Swallowing all exceptions when loading preferences hides configuration problemsIn
__init__,self.prefs_manager.load()is wrapped inexcept Exception: pass. If the YAML is malformed or the file is unreadable, this will silently skip loading, leaving_preferencesunset and deferring the real error to a later call.Prefer at least logging the failure, or restricting the catch to known benign cases (e.g., file-not-found) while surfacing real configuration errors early.
73-123: Conflict-resolution UI behavior looks correct; consider minor polishThe interactive flow correctly:
- Applies saved preferences first using the
min:maxconflict key.- Prompts the user with three options and loops on invalid input.
- Records packages to remove and optionally persists preferences.
Two small polish ideas:
- Tighten the type hint to
List[Tuple[str, str]]instead ofList[tuple].- Extract
"conflicts.saved_resolutions"into a module/class constant to avoid magic strings.Functionality otherwise aligns with the tests and issue requirements.
172-188: Dependency conflict integration is defensive but may fail silently and mis-identify target packageThe install flow now:
- Picks
target_package = software.split()[0].- Wraps the entire dependency resolution + conflict handling in a bare
except Exception: pass.Two implications:
- For natural-language requests (
"python 3.11 with pip"), only the first token is checked for conflicts, which may not match the actual apt package the interpreter chooses.- Any resolver failure (e.g., missing
dpkg/apt-cache) quietly disables conflict handling with no user feedback.Consider:
- Letting
CommandInterpretersurface the concrete package name(s) and using that instead ofsoftware.split()[0].- Narrowing the exception handling to expected system errors and emitting a
[INFO] Skipping conflict check: ...message when it’s disabled.
369-483:config()mixes many concerns and is over the usual complexity budget
config()currently handles eight subcommands, I/O formatting, interactive prompts, path handling, and error reporting in a single method. Sonar’s cognitive complexity warning is expected here.A simple refactor would be to delegate each action to a helper:
def config(self, action: str, key: Optional[str] = None, value: Optional[str] = None): try: if action == "list": return self._config_list() if action == "get": return self._config_get(key) # ... except ValueError as e: ...This keeps the public API unchanged, but makes each subcommand easier to test and evolve.
585-590:main()still emits emoji-style errors, contrary to the “[LABEL] only” output styleThe exception handlers in
main()print messages prefixed with❌, while the rest of the CLI intentionally uses[ERROR]/[SUCCESS]labels and the docs explicitly call out “No Emojis”.For consistency, consider switching these lines to use the same label style, e.g.:
- except KeyboardInterrupt: - print("\n❌ Operation cancelled by user", file=sys.stderr) + except KeyboardInterrupt: + print("\n[ERROR] Operation cancelled by user", file=sys.stderr)and similarly for the generic exception case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)cortex/cli.py(14 hunks)cortex/dependency_resolver.py(1 hunks)cortex/user_preferences.py(1 hunks)docs/IMPLEMENTATION_SUMMARY.md(1 hunks)test/test_conflict_ui.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_conflict_ui.py (3)
cortex/cli.py (4)
_resolve_conflicts_interactive(73-123)_ask_save_preference(125-141)config(369-482)main(510-590)cortex/user_preferences.py (9)
PreferencesManager(144-485)ConflictSettings(86-92)get(258-281)save(227-256)load(200-225)reset(356-383)validate(385-413)export_json(415-431)import_json(433-453)cortex/dependency_resolver.py (2)
DependencyResolver(37-274)resolve_dependencies(171-224)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 84-84: Define a constant instead of duplicating this literal "conflicts.saved_resolutions" 3 times.
[failure] 156-156: Define a constant instead of duplicating this literal "[INFO]" 4 times.
[failure] 369-369: Refactor this function to reduce its Cognitive Complexity from 37 to the 15 allowed.
test/test_conflict_ui.py
[warning] 62-62: Remove the unused local variable "result".
[warning] 363-363: Remove the unused local variable "result".
cortex/dependency_resolver.py
[failure] 171-171: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
cortex/user_preferences.py
[failure] 283-283: Refactor this function to reduce its Cognitive Complexity from 42 to the 15 allowed.
[failure] 356-356: Refactor this method to not always return the same value.
🪛 markdownlint-cli2 (0.18.1)
docs/IMPLEMENTATION_SUMMARY.md
73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
219-219: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.5)
cortex/cli.py
37-38: try-except-pass detected, consider logging the exception
(S110)
37-37: Do not catch blind exception: Exception
(BLE001)
188-189: try-except-pass detected, consider logging the exception
(S110)
188-188: Do not catch blind exception: Exception
(BLE001)
480-480: Do not catch blind exception: Exception
(BLE001)
481-481: Use explicit conversion flag
Replace with conversion flag
(RUF010)
test/test_conflict_ui.py
62-62: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
270-270: Unused method argument: mock_stdout
(ARG002)
283-283: Unused method argument: mock_stdout
(ARG002)
363-363: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
cortex/dependency_resolver.py
1-1: Shebang is present but file is not executable
(EXE001)
40-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
67-67: subprocess call: check for execution of untrusted input
(S603)
73-73: Consider moving this statement to an else block
(TRY300)
76-76: Do not catch blind exception: Exception
(BLE001)
cortex/user_preferences.py
1-1: Shebang is present but file is not executable
(EXE001)
196-196: Consider moving this statement to an else block
(TRY300)
197-197: Do not catch blind exception: Exception
(BLE001)
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Use explicit conversion flag
Replace with conversion flag
(RUF010)
220-220: Consider moving this statement to an else block
(TRY300)
223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Use explicit conversion flag
Replace with conversion flag
(RUF010)
224-224: Do not catch blind exception: Exception
(BLE001)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Use explicit conversion flag
Replace with conversion flag
(RUF010)
238-238: Avoid specifying long messages outside the exception class
(TRY003)
253-253: Consider moving this statement to an else block
(TRY300)
255-255: Do not catch blind exception: Exception
(BLE001)
256-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Use explicit conversion flag
Replace with conversion flag
(RUF010)
302-302: Abstract raise to an inner function
(TRY301)
302-302: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Abstract raise to an inner function
(TRY301)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Abstract raise to an inner function
(TRY301)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Abstract raise to an inner function
(TRY301)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
316-316: Abstract raise to an inner function
(TRY301)
316-316: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Abstract raise to an inner function
(TRY301)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Abstract raise to an inner function
(TRY301)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Abstract raise to an inner function
(TRY301)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Abstract raise to an inner function
(TRY301)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
333-333: Abstract raise to an inner function
(TRY301)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Abstract raise to an inner function
(TRY301)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Abstract raise to an inner function
(TRY301)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
342-342: Abstract raise to an inner function
(TRY301)
342-342: Avoid specifying long messages outside the exception class
(TRY003)
349-349: Abstract raise to an inner function
(TRY301)
349-349: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Consider moving this statement to an else block
(TRY300)
354-354: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Use explicit conversion flag
Replace with conversion flag
(RUF010)
379-379: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/test_conflict_ui.py (1)
282-297: Previous review concern addressed.The test now properly patches
builtins.inputto avoid blocking on the confirmation prompt. The unusedmock_inputparameter is required due to the@patchdecorator stacking order.
🧹 Nitpick comments (5)
cortex/user_preferences.py (4)
122-141:from_dictsilently ignores unknown keys and may raiseTypeErroron extra keys in nested dicts.The nested dataclass instantiations (e.g.,
ConfirmationSettings(**data.get("confirmations", {}))) will raiseTypeErrorif the loaded YAML/JSON contains unexpected keys. This can happen when users manually edit config files or when migrating from older versions with different schemas.Consider filtering keys to match dataclass fields:
+ @staticmethod + def _filter_dataclass_keys(data: Dict, dataclass_type) -> Dict: + """Filter dict to only include keys that are valid dataclass fields.""" + valid_keys = {f.name for f in dataclass_type.__dataclass_fields__.values()} + return {k: v for k, v in data.items() if k in valid_keys} + @classmethod def from_dict(cls, data: Dict[str, Any]) -> 'UserPreferences': """Create UserPreferences from dictionary""" - confirmations = ConfirmationSettings(**data.get("confirmations", {})) + confirmations = ConfirmationSettings(**cls._filter_dataclass_keys( + data.get("confirmations", {}), ConfirmationSettings))This improves forward/backward compatibility and prevents crashes on schema mismatches.
193-198: Add exception chaining for better traceability.Multiple
exceptblocks catchExceptionand re-raise without chaining (also flagged at lines 223-225, 255-256, 354). This loses the original traceback and makes debugging harder.try: import shutil shutil.copy2(self.config_path, backup_path) return backup_path - except Exception as e: - raise IOError(f"Failed to create backup: {str(e)}") + except Exception as e: + raise IOError(f"Failed to create backup: {e}") from eApply the same pattern (
raise ... from e) to the other locations for consistent error handling.
327-329: Consider adding range validation formax_suggestionsinset()to matchvalidate()logic.
set()only checks thatmax_suggestionsis an integer, butvalidate()(line 407-408) also requires it to be at least 1. This allows invalid values to be set and persisted before validation catches them.elif parts[1] == "max_suggestions" and not isinstance(value, int): raise ValueError("max_suggestions must be an integer") + elif parts[1] == "max_suggestions" and value < 1: + raise ValueError("max_suggestions must be at least 1")
403-405: Hardcoded model list may become stale.The
valid_modelslist is hardcoded invalidate(). As new models are released and old ones deprecated, this list will require manual updates.Consider extracting this to a module-level constant or making it configurable to simplify future maintenance.
test/test_conflict_ui.py (1)
248-251: Assertion at line 250 may be fragile due to YAML output format.The test asserts
'ai.model' in output, butyaml.dump()produces nested YAML withai:andmodel:on separate lines, not the dot-notation key. The current assertion might pass ifai.modelorgpt-4appears elsewhere, but it doesn't precisely validate the intended behavior.Consider asserting on the actual YAML structure or just the value:
output = mock_stdout.getvalue() - self.assertIn('ai.model', output) - self.assertIn('gpt-4', output) + self.assertIn('model: gpt-4', output)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)cortex/user_preferences.py(1 hunks)test/test_conflict_ui.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
test/test_conflict_ui.py (3)
cortex/cli.py (3)
_resolve_conflicts_interactive(73-123)_ask_save_preference(125-141)config(369-482)cortex/user_preferences.py (7)
ConflictSettings(86-92)get(258-281)save(227-256)reset(356-383)validate(385-416)export_json(418-434)import_json(436-456)cortex/dependency_resolver.py (2)
DependencyResolver(37-274)resolve_dependencies(171-224)
cortex/user_preferences.py (1)
logging_system.py (1)
info(211-213)
🪛 Ruff (0.14.5)
test/test_conflict_ui.py
62-62: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
270-270: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_input
(ARG002)
364-364: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
cortex/user_preferences.py
1-1: Shebang is present but file is not executable
(EXE001)
196-196: Consider moving this statement to an else block
(TRY300)
197-197: Do not catch blind exception: Exception
(BLE001)
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Use explicit conversion flag
Replace with conversion flag
(RUF010)
220-220: Consider moving this statement to an else block
(TRY300)
223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Use explicit conversion flag
Replace with conversion flag
(RUF010)
224-224: Do not catch blind exception: Exception
(BLE001)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Use explicit conversion flag
Replace with conversion flag
(RUF010)
238-238: Avoid specifying long messages outside the exception class
(TRY003)
253-253: Consider moving this statement to an else block
(TRY300)
255-255: Do not catch blind exception: Exception
(BLE001)
256-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Use explicit conversion flag
Replace with conversion flag
(RUF010)
302-302: Abstract raise to an inner function
(TRY301)
302-302: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Abstract raise to an inner function
(TRY301)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Abstract raise to an inner function
(TRY301)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Abstract raise to an inner function
(TRY301)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
316-316: Abstract raise to an inner function
(TRY301)
316-316: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Abstract raise to an inner function
(TRY301)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Abstract raise to an inner function
(TRY301)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Abstract raise to an inner function
(TRY301)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Abstract raise to an inner function
(TRY301)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
333-333: Abstract raise to an inner function
(TRY301)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Abstract raise to an inner function
(TRY301)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Abstract raise to an inner function
(TRY301)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
342-342: Abstract raise to an inner function
(TRY301)
342-342: Avoid specifying long messages outside the exception class
(TRY003)
349-349: Abstract raise to an inner function
(TRY301)
349-349: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Consider moving this statement to an else block
(TRY300)
354-354: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Use explicit conversion flag
Replace with conversion flag
(RUF010)
379-379: Avoid specifying long messages outside the exception class
(TRY003)
453-453: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
cortex/user_preferences.py (1)
385-416: Validation implementation looks complete.The
validate()method now includes themax_suggestionscheck (lines 407-408), addressing the previous review feedback. The validation covers all critical fields appropriately.test/test_conflict_ui.py (5)
15-16: Thesysimport is used on line 25.The static analysis flagged
sysas unused, but it's actually used forsys.path.insert(0, ...)on line 25. This is a false positive.
49-68: Test correctly validates cancel flow and SystemExit behavior.The test properly mocks user input, verifies the UI output, and checks that option 3 triggers
SystemExit. The unusedresultvariable (line 62) is acceptable since the test focuses on the exception behavior.
136-218: Preference saving tests are thorough and correctly validate the key format.Good coverage of save/skip flows, persistence across sessions, and multiple preferences. The tests correctly verify the
min:maxkey format used by the implementation.
439-451: Validation test correctly bypassesset()to test edge cases.The test directly modifies
prefs.ai.max_suggestions = -999to bypass the setter's type checking, then verifies thatvalidate()catches the invalid value. This is a valid testing approach for the validation layer.
35-47: Good test setup with proper isolation and cleanup.The use of temporary directories for config files and proper cleanup in
tearDownensures tests don't interfere with each other or leave artifacts.
Sahilbhatane
changed the title
reset to origin/main, no file structure changes, created separate issue for that... all test passed, used OPENAI API for testing on vs terminal (is same as ubuntu's terminal) (video). |
|
Did you tested on your machine before raising PR? |
yes, I always do, check the video for how i tested implemented functionalities |
Revert changes of dependency_resolver.py file unless those changes are related to your feature. |
|
that's just copied file from root folder, I can just rm the |
address all the reviews |
|
No changes done to the main |
|
I have mentioned all changes done in summary doc, please review that if any conflict with existing code then please specify it so I can understand and change it accordingly. |
|
@Sahilbhatane - Please respond to @dhvll l's questions:
Once you confirm and @dhvll approves, we'll merge immediately. Bounty ready for Issue #42. |
yeah he have added video implementation. |
|
Hi @Sahilbhatane! This PR has merge conflicts with the latest main branch. Could you please rebase? git fetch origin
git rebase origin/main
# resolve conflicts
git push --force-with-leaseGreat work on the conflict resolution UI - will merge as soon as it's rebased! |
Sure |
Sahilbhatane
force-pushed
the
issue-42
branch
from
144f64d to
e6d0b88
Compare
|
Please rebase onto main to resolve conflicts: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/test_conflict_ui.py (2)
282-297: Past review addressed:inputis now patched for reset test.The test correctly patches
builtins.inputto return'y', preventing the test from blocking on interactive input. The unusedmock_stdoutandmock_inputarguments are intentional for stdout capture and mock ordering.
385-399: Weak assertion already flagged in past review.The assertion at line 399 (
len(graph.conflicts) > 0 or mock_run.called) is too permissive and was previously flagged. Consider testing_detect_conflictsdirectly with controlled inputs for reliable coverage.
🧹 Nitpick comments (11)
test/test_conflict_ui.py (2)
60-68: Remove unusedresultvariable assignment.The
resultvariable is assigned but never used since the test expectsSystemExit. Static analysis correctly flags this.# Should exit on choice 3 with self.assertRaises(SystemExit): - result = self.cli._resolve_conflicts_interactive(conflicts) + self.cli._resolve_conflicts_interactive(conflicts)
362-364: Remove unusedresultvariable assignment.Same issue as line 62 - the
resultvariable is never used.# Should exit on choice 3 with self.assertRaises(SystemExit): - result = self.cli._resolve_conflicts_interactive(conflicts) + self.cli._resolve_conflicts_interactive(conflicts)cortex/user_preferences.py (4)
127-146: Consider handling unknown keys infrom_dict()for forward compatibility.If a config file contains keys not defined in the dataclass (e.g., from a newer version), the
**data.get("ai", {})unpacking will raiseTypeError. Consider filtering to known fields:@classmethod def from_dict(cls, data: Dict[str, Any]) -> 'UserPreferences': """Create UserPreferences from dictionary""" - confirmations = ConfirmationSettings(**data.get("confirmations", {})) - auto_update = AutoUpdateSettings(**data.get("auto_update", {})) - ai = AISettings(**data.get("ai", {})) - packages = PackageSettings(**data.get("packages", {})) - conflicts = ConflictSettings(**data.get("conflicts", {})) + def filter_fields(cls, d): + from dataclasses import fields + valid = {f.name for f in fields(cls)} + return {k: v for k, v in d.items() if k in valid} + + confirmations = ConfirmationSettings(**filter_fields(ConfirmationSettings, data.get("confirmations", {}))) + auto_update = AutoUpdateSettings(**filter_fields(AutoUpdateSettings, data.get("auto_update", {}))) + ai = AISettings(**filter_fields(AISettings, data.get("ai", {}))) + packages = PackageSettings(**filter_fields(PackageSettings, data.get("packages", {}))) + conflicts = ConflictSettings(**filter_fields(ConflictSettings, data.get("conflicts", {})))
288-359: High cognitive complexity inset()method.SonarCloud flags cognitive complexity of 42 (limit: 15). The method works correctly but could benefit from refactoring using a dispatch table pattern. This is a recommended improvement for maintainability but not blocking for this PR.
Consider extracting validators/setters into a dispatch table:
# Example refactor approach _SETTERS = { "verbosity": _set_verbosity, "confirmations": _set_confirmations, "ai": _set_ai, # ... } def set(self, key: str, value: Any) -> bool: parts = key.split(".") setter = self._SETTERS.get(parts[0]) if setter: return setter(self, parts, value) raise ValueError(f"Unknown preference key: {key}")
226-230: Add exception chaining for better debugging.When re-raising exceptions, use
from eto preserve the original stack trace. This applies to multiple locations in the file.except yaml.YAMLError as e: - raise ValueError(f"Invalid YAML in config file: {str(e)}") + raise ValueError(f"Invalid YAML in config file: {e}") from e except Exception as e: - raise IOError(f"Failed to load config file: {str(e)}") + raise IOError(f"Failed to load config file: {e}") from e
361-388: Method always returnsTrue- consider simplifying.SonarCloud notes that
reset()always returnsTruesince errors raise exceptions. Consider returningNoneor making it a void method, though this is a minor stylistic preference.cortex/cli.py (3)
35-38: Silent exception handling on preferences load.The bare
except Exception: passsilently ignores all errors when loading preferences. Consider logging a warning so users know if their config failed to load.try: self.prefs_manager.load() - except Exception: - pass + except Exception as e: + # Preferences load failed - continue with defaults + import logging + logging.warning(f"Failed to load preferences, using defaults: {e}")
83-84: Extract repeated literals to constants.The string
"conflicts.saved_resolutions"is used 3 times. Consider defining it as a class constant for maintainability.class CortexCLI: + CONFLICTS_SAVED_RESOLUTIONS_KEY = "conflicts.saved_resolutions" + def __init__(self): # ... def _resolve_conflicts_interactive(self, conflicts: List[tuple]) -> Dict[str, List[str]]: resolutions = {'remove': []} - saved_resolutions = self.prefs_manager.get("conflicts.saved_resolutions") or {} + saved_resolutions = self.prefs_manager.get(self.CONFLICTS_SAVED_RESOLUTIONS_KEY) or {}
369-482: High cognitive complexity inconfig()method.SonarCloud flags cognitive complexity of 37 (limit: 15). Consider extracting each action into a separate method (e.g.,
_config_list(),_config_get(), etc.) using a dispatch table pattern.docs/IMPLEMENTATION_SUMMARY.md (2)
73-84: Add language specifier to fenced code block.The code block showing example output should have a language specifier for proper rendering.
**Example Output:** -``` +```text ==================================================================== Package Conflicts Detected ====================================================================
219-234: Add language specifier to workflow diagram.The workflow diagram code block should have a language specifier.
**Workflow:** -``` +```text User runs: cortex install nginx ↓
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)cortex/cli.py(14 hunks)cortex/user_preferences.py(4 hunks)docs/IMPLEMENTATION_SUMMARY.md(1 hunks)test/test_conflict_ui.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (3)
cortex/user_preferences.py (1)
logging_system.py (1)
info(211-213)
test/test_conflict_ui.py (3)
cortex/cli.py (2)
_resolve_conflicts_interactive(73-123)_ask_save_preference(125-141)cortex/user_preferences.py (3)
ConflictSettings(91-97)get(263-286)save(232-261)dependency_resolver.py (1)
DependencyResolver(39-399)
cortex/cli.py (4)
cortex/user_preferences.py (10)
PreferencesManager(149-493)load(205-230)get(263-286)save(232-261)list_all(483-493)get_config_info(463-481)reset(361-388)validate(390-421)export_json(423-439)import_json(441-461)LLM/interpreter.py (1)
CommandInterpreter(12-158)dependency_resolver.py (2)
DependencyResolver(39-399)resolve_dependencies(215-280)cortex/coordinator.py (1)
StepStatus(31-36)
🪛 GitHub Actions: Cortex Automation
cortex/user_preferences.py
[error] 12-12: ModuleNotFoundError: No module named 'yaml' (PyYAML not installed) during test collection.
🪛 GitHub Check: SonarCloud Code Analysis
cortex/user_preferences.py
[failure] 361-361: Refactor this method to not always return the same value.
[failure] 288-288: Refactor this function to reduce its Cognitive Complexity from 42 to the 15 allowed.
test/test_conflict_ui.py
[warning] 364-364: Remove the unused local variable "result".
[warning] 62-62: Remove the unused local variable "result".
cortex/cli.py
[failure] 84-84: Define a constant instead of duplicating this literal "conflicts.saved_resolutions" 3 times.
[failure] 156-156: Define a constant instead of duplicating this literal "[INFO]" 4 times.
[failure] 369-369: Refactor this function to reduce its Cognitive Complexity from 37 to the 15 allowed.
🪛 markdownlint-cli2 (0.18.1)
docs/IMPLEMENTATION_SUMMARY.md
73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
219-219: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.6)
cortex/user_preferences.py
201-201: Consider moving this statement to an else block
(TRY300)
202-202: Do not catch blind exception: Exception
(BLE001)
203-203: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Use explicit conversion flag
Replace with conversion flag
(RUF010)
225-225: Consider moving this statement to an else block
(TRY300)
228-228: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
228-228: Avoid specifying long messages outside the exception class
(TRY003)
228-228: Use explicit conversion flag
Replace with conversion flag
(RUF010)
229-229: Do not catch blind exception: Exception
(BLE001)
230-230: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Use explicit conversion flag
Replace with conversion flag
(RUF010)
243-243: Avoid specifying long messages outside the exception class
(TRY003)
258-258: Consider moving this statement to an else block
(TRY300)
260-260: Do not catch blind exception: Exception
(BLE001)
261-261: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Use explicit conversion flag
Replace with conversion flag
(RUF010)
307-307: Abstract raise to an inner function
(TRY301)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
312-312: Abstract raise to an inner function
(TRY301)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Abstract raise to an inner function
(TRY301)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Abstract raise to an inner function
(TRY301)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
321-321: Abstract raise to an inner function
(TRY301)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Abstract raise to an inner function
(TRY301)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Abstract raise to an inner function
(TRY301)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Abstract raise to an inner function
(TRY301)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
333-333: Abstract raise to an inner function
(TRY301)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Abstract raise to an inner function
(TRY301)
338-338: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Abstract raise to an inner function
(TRY301)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
345-345: Abstract raise to an inner function
(TRY301)
345-345: Avoid specifying long messages outside the exception class
(TRY003)
347-347: Abstract raise to an inner function
(TRY301)
347-347: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Abstract raise to an inner function
(TRY301)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
356-356: Consider moving this statement to an else block
(TRY300)
359-359: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
359-359: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Use explicit conversion flag
Replace with conversion flag
(RUF010)
384-384: Avoid specifying long messages outside the exception class
(TRY003)
458-458: Avoid specifying long messages outside the exception class
(TRY003)
test/test_conflict_ui.py
62-62: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
270-270: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_input
(ARG002)
364-364: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
cortex/cli.py
37-38: try-except-pass detected, consider logging the exception
(S110)
37-37: Do not catch blind exception: Exception
(BLE001)
188-189: try-except-pass detected, consider logging the exception
(S110)
188-188: Do not catch blind exception: Exception
(BLE001)
480-480: Do not catch blind exception: Exception
(BLE001)
481-481: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (12)
test/test_conflict_ui.py (4)
49-68: Good test coverage for the cancel/skip path.The test correctly verifies that choosing option 3 triggers
SystemExitand validates the UI output contains expected package names and the cancel option.
152-164: Good validation of the min:max conflict key format.The test correctly verifies that conflict keys use the
min:maxformat (e.g.,'apache2:nginx'instead of'nginx:apache2'), ensuring consistent key generation regardless of the order packages are encountered.
439-451: Validation test now aligned with implementation.The test sets
max_suggestions = -999and expects validation errors. This now works correctly sincePreferencesManager.validate()includes the checkif self._preferences.ai.max_suggestions < 1at lines 412-413 inuser_preferences.py.
15-29: Test setup and imports are correct.The
sysimport is used at line 25 forsys.path.insert(). The test module correctly sets up the path and imports required classes. The Copilot comment about unusedsysimport is a false positive.cortex/user_preferences.py (3)
90-97: ConflictSettings dataclass is well-designed.The new
ConflictSettingsdataclass correctly usesfield(default_factory=dict)for mutable defaults and includes ato_dict()method for serialization. This integrates cleanly with the existing preference structure.
408-420: Validation logic is comprehensive and addresses past review.The
validate()method now includes the check forai.max_suggestions < 1at lines 412-413, addressing the previous review comment. The validation covers all critical configuration values.
11-13: I'm unable to access the repository due to cloning failures, which prevents me from verifying whether PyYAML is actually listed in the project's dependency files (requirements.txt, setup.py, pyproject.toml, etc.).Unable to verify PyYAML dependency claim due to repository access issues.
The review comment asserts that
PyYAMLis missing from dependency declarations, causing aModuleNotFoundErrorin the CI pipeline. However, without access to the actual repository files, I cannot independently confirm:
- Whether PyYAML is absent from requirements.txt, setup.py, or other dependency files
- The actual pipeline failure logs
- Whether this is a pre-existing issue or introduced by changes in this PR
Please manually verify by checking:
requirements.txtor equivalent dependency files- CI/pipeline logs showing the actual error
- Whether the
yamlimport at line 12 ofcortex/user_preferences.pyis actually causing failurescortex/cli.py (3)
83-98: Conflict resolution logic correctly handles saved preferences.The implementation uses
min:maxformat for conflict keys ensuring consistency. When a saved preference exists, it's automatically applied without prompting the user, which aligns with the acceptance criteria.
484-507: Config value parsing is reasonable.The
_parse_config_value()helper handles boolean strings, integers, and comma-separated lists. The behavior of treating'0'/'1'as booleans is standard CLI convention.
557-563: Config subcommand properly integrated with argparse.The new
configsubcommand is correctly wired with appropriate action choices, optional key/value arguments, and help text.docs/IMPLEMENTATION_SUMMARY.md (2)
1-6: Comprehensive implementation documentation.The document provides a thorough overview of the feature implementation, including file changes, feature breakdown, code examples, and testing instructions. This will be valuable for reviewers and future maintainers.
264-278: Repository access unavailable for verification.The shell script execution failed due to inability to clone the repository
cortexlinux/cortex. Without access to the actual codebase, I cannot verify whether:
- The
vim ↔ emacsconflicts exist in theconflict_patternsdictionary independency_resolver.py- The documented patterns match the implementation
The review comment's claim about documentation/code mismatch cannot be confirmed or refuted without examining the actual source files.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/test_conflict_ui.py (1)
385-399: Strengthen assertion intest_dependency_resolver_detects_conflicts.The final assertion:
self.assertTrue(len(graph.conflicts) > 0 or mock_run.called)passes whenever the subprocess mock is called, even if no conflicts are detected, so it doesn’t really validate conflict detection logic (as previously noted). Prefer asserting on the actual conflicts, e.g.:
- self.assertTrue(len(graph.conflicts) > 0 or mock_run.called) + self.assertTrue(mock_run.called) + self.assertIn(('nginx', 'apache2'), graph.conflicts)or directly unit‑test
_detect_conflictswith a controlled dependency list.cortex/cli.py (2)
172-189: Avoid swallowing dependency resolver failures; report them instead.This block:
try: graph = resolver.resolve_dependencies(target_package) ... except Exception: passmeans any resolver error (e.g., missing
apt-cache, permissions, bugs) will be completely silent and conflicts won’t be checked. That undermines the feature and was already flagged in a prior review.You should at least surface a warning and keep installing, e.g.:
- try: - graph = resolver.resolve_dependencies(target_package) - if graph.conflicts: - resolutions = self._resolve_conflicts_interactive(graph.conflicts) - - if resolutions['remove']: - for pkg_to_remove in resolutions['remove']: - if not any(f"remove {pkg_to_remove}" in cmd for cmd in commands): - commands.insert(0, f"sudo apt-get remove -y {pkg_to_remove}") - self._print_status("[INFO]", f"Added command to remove conflicting package: {pkg_to_remove}") - except Exception: - pass + try: + graph = resolver.resolve_dependencies(target_package) + if graph.conflicts: + resolutions = self._resolve_conflicts_interactive(graph.conflicts) + if resolutions['remove']: + for pkg_to_remove in resolutions['remove']: + if not any(f"remove {pkg_to_remove}" in cmd for cmd in commands): + commands.insert(0, f"sudo apt-get remove -y {pkg_to_remove}") + self._print_status("[INFO]", f"Added command to remove conflicting package: {pkg_to_remove}") + except Exception as e: + self._print_status("[INFO]", f"Could not check for conflicts for {target_package}: {e}")Optionally narrow the exception type if
DependencyResolverexposes its own error class.Also,
target_package = software.split()[0]is a coarse heuristic for natural‑language requests; consider deriving the primary package name from the parsedcommandsinstead for better accuracy.
369-482: Config command works but is brittle aroundyamland a bit monolithic.
The inline
import yamlin the"list"branch will raiseImportErrorif PyYAML isn’t installed. Given previous feedback, consider moving the import to module level with a guarded fallback (e.g., to JSON) socortex config listdoesn’t crash outright.The
config()method’s longif/elifladder plus embedded input prompts gives it high cognitive complexity (also flagged by Sonar). Splitting each action into a small helper (e.g.,_config_list(),_config_get(), …) would simplify reasoning and testing.For
action == "reset"withoutkey, you always prompt viainput("Continue? (y/n): "). That’s fine for interactive CLI use, but it complicates non‑interactive callers; a futureforceflag (plumbed from argparse) would make scripting easier while preserving safety for humans.Check current PyYAML installation recommendations for CLI tools that optionally render YAML, and whether a JSON fallback is considered a best practice when PyYAML is missing.
🧹 Nitpick comments (3)
cortex/cli.py (1)
54-61: Consider central constants for status labels.
"[INFO]"and other status labels are duplicated; Sonar already flagged this. A small module-level constant (e.g.,INFO = "[INFO]",ERROR = "[ERROR]", etc.) keeps the labels consistent and makes future changes easier.test/test_conflict_ui.py (2)
235-284: Tidy unused mock arguments in config tests.
mock_stdout/mock_inputare injected by@patchbut not used intest_config_list_commandandtest_config_reset_command, which Ruff flags. Either use them (e.g., inspect output) or rename to_stdout/_inputto make the intentional discard explicit and silence the warnings.
366-384:test_saved_preference_bypasses_uidoesn’t actually exercise the UI path.This test currently just inspects the stored dict and re‑implements the “if conflict_key in saved” logic inline, without calling
_resolve_conflicts_interactiveor asserting thatinput()isn’t called.To truly verify auto‑apply behavior, consider something like:
+ @patch('builtins.input') + def test_saved_preference_bypasses_ui(self, mock_input): + conflict_key = 'mariadb-server:mysql-server' + self.cli.prefs_manager.set('conflicts.saved_resolutions', { + conflict_key: 'mysql-server' + }) + self.cli.prefs_manager.save() + + conflicts = [('mysql-server', 'mariadb-server')] + result = self.cli._resolve_conflicts_interactive(conflicts) + + mock_input.assert_not_called() + self.assertIn('remove', result) + self.assertIn('mariadb-server', result['remove'])This would explicitly confirm that saved preferences bypass prompts and still yield the expected removal list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(14 hunks)test/test_conflict_ui.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_conflict_ui.py (3)
cortex/cli.py (5)
CortexCLI(23-507)_resolve_conflicts_interactive(73-123)_ask_save_preference(125-141)config(369-482)main(510-590)cortex/user_preferences.py (9)
PreferencesManager(149-493)ConflictSettings(91-97)get(263-286)save(232-261)load(205-230)reset(361-388)validate(390-421)export_json(423-439)import_json(441-461)dependency_resolver.py (1)
DependencyResolver(39-399)
🪛 GitHub Check: SonarCloud Code Analysis
test/test_conflict_ui.py
[warning] 364-364: Remove the unused local variable "result".
[warning] 62-62: Remove the unused local variable "result".
cortex/cli.py
[failure] 84-84: Define a constant instead of duplicating this literal "conflicts.saved_resolutions" 3 times.
[failure] 156-156: Define a constant instead of duplicating this literal "[INFO]" 4 times.
[failure] 369-369: Refactor this function to reduce its Cognitive Complexity from 37 to the 15 allowed.
🪛 Ruff (0.14.6)
test/test_conflict_ui.py
62-62: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
270-270: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_stdout
(ARG002)
284-284: Unused method argument: mock_input
(ARG002)
364-364: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
cortex/cli.py
37-38: try-except-pass detected, consider logging the exception
(S110)
37-37: Do not catch blind exception: Exception
(BLE001)
188-189: try-except-pass detected, consider logging the exception
(S110)
188-188: Do not catch blind exception: Exception
(BLE001)
480-480: Do not catch blind exception: Exception
(BLE001)
481-481: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
cortex/cli.py (4)
125-141:_ask_save_preferenceside effects look correct and match tests.The min/max key normalization and update pattern align with the tests and
ConflictSettings.saved_resolutionsdesign; persistence viasave()is consistent withPreferencesManager.
203-244: Install integration and history updates look consistent.Progress callback status mapping, history recording, and new info messages (including rollback hints) are coherent with existing patterns; the conflict‑driven removal commands integrate cleanly into the generated command list.
484-507:_parse_config_valueis reasonable for simple CLI values.Boolean/int/comma‑list parsing matches the documented behavior and keeps
config setergonomic. It’s a good fit for the current preference schema.
557-581: Newconfigsubcommand wiring is consistent with existing CLI design.Argument parsing and dispatch (
cli.config(args.action, args.key, args.value)) match the install/history/rollback pattern and align with the documented examples in the epilog.test/test_conflict_ui.py (2)
416-501: Preference persistence and validation tests look solid.The save/load, validation (including an explicit invalid case), nested‑key handling, reset‑to‑defaults, and JSON export/import tests exercise the main
PreferencesManagerbehaviors well and align with the implementation snippets.
32-328: Overall conflict‑UI and config command coverage is good.The tests around
_resolve_conflicts_interactive,_ask_save_preference,config list/get/set/reset/export/import, and integration with a temp config file provide good behavioral coverage and match the CLI implementation and acceptance criteria.
|
@dhvll Could you review this PR? Your package manager wrapper experience would be valuable for the conflict resolution feature. Thanks! |
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
cortex/cli.py (3)
223-241: Fix import path and avoid silently swallowing exceptions.Two issues:
Incorrect import path: Line 225 imports
from dependency_resolverbut the module is atcortex/dependency_resolver.pybased on the project structure.Silent exception handling (noted in previous review): The bare
except Exception: passhides failures like missingapt-cache, permission errors, or import failures.# Check for package conflicts using DependencyResolver - from dependency_resolver import DependencyResolver + from cortex.dependency_resolver import DependencyResolver resolver = DependencyResolver() target_package = software.split()[0] try: graph = resolver.resolve_dependencies(target_package) if graph.conflicts: resolutions = self._resolve_conflicts_interactive(graph.conflicts) if resolutions['remove']: for pkg_to_remove in resolutions['remove']: if not any(f"remove {pkg_to_remove}" in cmd for cmd in commands): commands.insert(0, f"sudo apt-get remove -y {pkg_to_remove}") self._print_status("[INFO]", f"Added command to remove conflicting package: {pkg_to_remove}") - except Exception: - pass + except ImportError: + self._print_status("[INFO]", "Dependency resolver not available, skipping conflict check") + except Exception as e: + self._print_status("[INFO]", f"Could not check for conflicts: {e}")This repeats feedback from the previous review regarding silent exception handling.
437-444: Handle missing PyYAML dependency gracefully.The
import yamlat line 439 will raiseImportErrorif PyYAML is not installed, crashing the CLI. This was noted in a previous review.+# At module level or with fallback: +try: + import yaml + HAS_YAML = True +except ImportError: + HAS_YAML = False + # In config() method, action == "list": if action == "list": prefs = self.prefs_manager.list_all() print("\n[INFO] Current Configuration:") print("=" * 60) - import yaml - print(yaml.dump(prefs, default_flow_style=False, sort_keys=False)) + if HAS_YAML: + print(yaml.dump(prefs, default_flow_style=False, sort_keys=False)) + else: + print(json.dumps(prefs, indent=2))
472-484: Interactiveinput()inconfig('reset')blocks non-interactive use.When
action == "reset"andkeyisNone, the method callsinput()which will block or crash in automated/test environments. Consider adding a--forceflag or ensuring tests mockinput.This was noted in a previous review. Options:
- Add a
forceparameter to skip confirmation- Document that tests must patch
input
🧹 Nitpick comments (4)
cortex/cli.py (2)
169-186: Extract duplicated literal and add null guard.The literal
"conflicts.saved_resolutions"is duplicated 3 times (SonarCloud flagged). Also,prefs_managermay beNone.Define a constant at module or class level:
CONFLICTS_SAVED_RESOLUTIONS_KEY = "conflicts.saved_resolutions"Then use it consistently:
+CONFLICTS_SAVED_RESOLUTIONS_KEY = "conflicts.saved_resolutions" + def _ask_save_preference(self, pkg1: str, pkg2: str, preferred: str): ... save = input("Save this preference for future conflicts? (y/N): ").strip().lower() - if save == 'y': + if save == 'y' and self.prefs_manager is not None: conflict_key = f"{min(pkg1, pkg2)}:{max(pkg1, pkg2)}" - saved_resolutions = self.prefs_manager.get("conflicts.saved_resolutions") or {} + saved_resolutions = self.prefs_manager.get(CONFLICTS_SAVED_RESOLUTIONS_KEY) or {} saved_resolutions[conflict_key] = preferred - self.prefs_manager.set("conflicts.saved_resolutions", saved_resolutions) + self.prefs_manager.set(CONFLICTS_SAVED_RESOLUTIONS_KEY, saved_resolutions) self.prefs_manager.save() print("Preference saved.")
421-534: Consider refactoringconfig()to reduce cognitive complexity.SonarCloud reports cognitive complexity of 37 (allowed: 15). The method handles 8 different actions with nested conditionals. Consider extracting each action into a private method.
def config(self, action: str, key: Optional[str] = None, value: Optional[str] = None): """Manage user preferences and configuration.""" handlers = { "list": self._config_list, "get": self._config_get, "set": self._config_set, "reset": self._config_reset, "validate": self._config_validate, "info": self._config_info, "export": self._config_export, "import": self._config_import, } handler = handlers.get(action) if not handler: self._print_error(f"Unknown config action: {action}") return 1 try: return handler(key, value) except ValueError as e: self._print_error(str(e)) return 1 except Exception as e: self._print_error(f"Configuration error: {e}") return 1docs/IMPLEMENTATION_SUMMARY.md (2)
9-9: Add language specifier to fenced code block.The code block at line 9 lacks a language specifier (markdownlint MD040).
Select action for Conflict 1 [1-3]: -``` +```text
144-159: Add language specifier to workflow diagram.The workflow diagram code block lacks a language specifier.
### Workflow: -``` +```text User runs: cortex install nginx ↓ DependencyResolver detects conflict with apache2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)cortex/cli.py(12 hunks)docs/IMPLEMENTATION_SUMMARY.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/user_preferences.py (8)
get(263-286)save(232-261)list_all(483-493)get_config_info(463-481)reset(361-388)validate(390-421)export_json(423-439)import_json(441-461)cortex/dependency_resolver.py (2)
DependencyResolver(39-399)resolve_dependencies(215-280)cortex/coordinator.py (1)
StepStatus(31-36)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 128-128: Define a constant instead of duplicating this literal "conflicts.saved_resolutions" 3 times.
[failure] 208-208: Define a constant instead of duplicating this literal "[INFO]" 4 times.
[failure] 421-421: Refactor this function to reduce its Cognitive Complexity from 37 to the 15 allowed.
🪛 markdownlint-cli2 (0.18.1)
docs/IMPLEMENTATION_SUMMARY.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
212-212: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.7)
cortex/cli.py
8-8: Redefinition of unused List from line 6
Remove definition: List
(F811)
8-8: Redefinition of unused Optional from line 6
Remove definition: Optional
(F811)
240-241: try-except-pass detected, consider logging the exception
(S110)
240-240: Do not catch blind exception: Exception
(BLE001)
532-532: Do not catch blind exception: Exception
(BLE001)
533-533: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
cortex/cli.py (3)
536-559: LGTM with minor note on '0'/'1' parsing.The parsing logic is reasonable. Note that
'0'and'1'are parsed as booleans before attempting integer conversion, so users cannot set literal integer0or1values. If this is intentional for config flags, the behavior is acceptable.
875-882: LGTM!The config subcommand is well-defined with appropriate argument handling and follows the existing CLI patterns.
904-905: LGTM!The config command routing integrates cleanly with the existing command dispatch pattern.
docs/IMPLEMENTATION_SUMMARY.md (1)
69-73: Verify documentation and code for vim/emacs conflict pattern consistency.The documentation at docs/IMPLEMENTATION_SUMMARY.md (lines 69-73) lists
vim ↔ emacsas a known conflict pattern, but this claim requires verification against the actualconflict_patternsdefinition incortex/dependency_resolver.pyto ensure documentation and implementation are aligned. Please confirm whether thevim ↔ emacsconflict is present in the code or if the documentation should be updated to match the actual implementation.
| def _resolve_conflicts_interactive(self, conflicts: List[tuple]) -> Dict[str, List[str]]: | ||
| """ | ||
| Interactively resolve package conflicts with user input. | ||
| Args: | ||
| conflicts: List of tuples (package1, package2) representing conflicts | ||
| Returns: | ||
| Dictionary with resolution actions (e.g., {'remove': ['pkgA']}) | ||
| """ | ||
| resolutions = {'remove': []} | ||
| saved_resolutions = self.prefs_manager.get("conflicts.saved_resolutions") or {} | ||
|
|
||
| print("\n" + "=" * 60) | ||
| print("Package Conflicts Detected") | ||
| print("=" * 60) | ||
|
|
||
| for i, (pkg1, pkg2) in enumerate(conflicts, 1): | ||
| conflict_key = f"{min(pkg1, pkg2)}:{max(pkg1, pkg2)}" | ||
| if conflict_key in saved_resolutions: | ||
| preferred = saved_resolutions[conflict_key] | ||
| to_remove = pkg2 if preferred == pkg1 else pkg1 | ||
| resolutions['remove'].append(to_remove) | ||
| print(f"\nConflict {i}: {pkg1} vs {pkg2}") | ||
| print(f" Using saved preference: Keep {preferred}, remove {to_remove}") | ||
| continue |
There was a problem hiding this comment.
Guard against uninitialized prefs_manager and invalid saved preferences.
Two issues in this segment:
self.prefs_manageris initialized asNone(line 51) but accessed directly at line 128 without null check.- If
saved_resolutions[conflict_key]is corrupted (not equal topkg1orpkg2), line 138 will incorrectly removepkg1.
def _resolve_conflicts_interactive(self, conflicts: List[tuple]) -> Dict[str, List[str]]:
...
resolutions = {'remove': []}
+ if self.prefs_manager is None:
+ saved_resolutions = {}
+ else:
- saved_resolutions = self.prefs_manager.get("conflicts.saved_resolutions") or {}
+ saved_resolutions = self.prefs_manager.get("conflicts.saved_resolutions") or {}
...
for i, (pkg1, pkg2) in enumerate(conflicts, 1):
conflict_key = f"{min(pkg1, pkg2)}:{max(pkg1, pkg2)}"
if conflict_key in saved_resolutions:
preferred = saved_resolutions[conflict_key]
+ if preferred not in (pkg1, pkg2):
+ print(f" Warning: Invalid saved preference '{preferred}', asking again...")
+ else:
- to_remove = pkg2 if preferred == pkg1 else pkg1
+ to_remove = pkg2 if preferred == pkg1 else pkg1
- resolutions['remove'].append(to_remove)
+ resolutions['remove'].append(to_remove)
- print(f"\nConflict {i}: {pkg1} vs {pkg2}")
+ print(f"\nConflict {i}: {pkg1} vs {pkg2}")
- print(f" Using saved preference: Keep {preferred}, remove {to_remove}")
+ print(f" Using saved preference: Keep {preferred}, remove {to_remove}")
- continue
+ continue🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 128-128: Define a constant instead of duplicating this literal "conflicts.saved_resolutions" 3 times.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 117 to 142, the code assumes self.prefs_manager is
initialized and trusts saved_resolutions entries, which can be None or
corrupted; add a guard that retrieves saved_resolutions only if
self.prefs_manager is not None and is the expected object (otherwise treat as
empty dict), and when a saved preference exists validate that its value exactly
equals pkg1 or pkg2 (and is a string) before using it; if validation fails, skip
applying the saved preference (print a warning or fall back to interactive
prompt) so you never remove the wrong package based on malformed data.
3 participants
Summary
Implements interactive package conflict resolution UI and persistent conflict-resolution preferences.cortex/cli.py.ConflictSettingstocortex/user_preferences.pyand persistconflicts.saved_resolutions.test/test_conflict_ui.pyexpanded to cover saving and auto-applying preferences..gitignoreto exclude generateddata/files while keepingcontributors.json.Type of Change
Checklist
Testing
python -m pytest test/test_conflict_ui.py— 5 tests passed (interactive resolution and saving behavior).python test/run_all_tests.py) passes (developer tests continue to succeed).OPENAI_API_KEY) and model access.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.