feat: 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. 📝 WalkthroughWalkthroughAdds a YAML-backed user preferences system and CLI Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CortexCLI
participant DR as DependencyResolver
participant PM as PreferencesManager
participant UI as Conflict UI
participant FS as FileSystem
User->>CLI: cortex install pkgA pkgB
CLI->>DR: request install plan
DR->>DR: detect dependency conflicts
alt conflicts found
DR-->>CLI: conflicts list
CLI->>PM: query saved preference
alt saved preference exists
PM-->>CLI: resolution
CLI->>CLI: adjust plan
else
CLI->>UI: present choices
UI->>User: prompt resolution
User-->>UI: selects option
UI-->>CLI: chosen resolution
CLI->>User: ask to save preference?
User-->>CLI: yes/no
alt save
CLI->>PM: save preference
PM->>FS: write YAML (atomic + backup)
end
CLI->>CLI: adjust plan
end
else
DR-->>CLI: plan OK
end
CLI-->>User: show final plan / execute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
f39ad6f to
e302d17
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cortex/licensing.py (1)
204-220: ASCII box borders are incomplete on dynamic content lines.Lines 210, 214, and 216 are missing the closing
│character, which will cause the upgrade prompt box to render with broken right edges in the terminal.Proposed fix to complete the box borders
┌─────────────────────────────────────────────────────────┐ │ ⚡ UPGRADE REQUIRED │ ├─────────────────────────────────────────────────────────┤ │ │ -│ '{feature_display}' requires Cortex {tier_display} +│ '{feature_display}' requires Cortex {tier_display:<15}│ │ │ │ ✅ Upgrade now: cortex upgrade │ │ │ -│ Plans start at {price}/month with 14-day free trial. +│ Plans start at {price}/month with 14-day free trial. │ │ │ -│ 🌐 {PRICING_URL} +│ 🌐 {PRICING_URL:<40}│ │ │ └─────────────────────────────────────────────────────────┘Note: The exact padding widths may need adjustment based on the maximum expected lengths of
feature_display,tier_display, andPRICING_URLto align properly with the 57-character box width.cortex/gpu_manager.py (2)
198-229: Avoid hard-coded NVIDIA PCI sysfs path (breaks on most machines).
Line 217 hard-codes0000:01:00.0, sopower_state/is_activebecomes unreliable when the dGPU sits elsewhere.Proposed fix
@@ def _detect_nvidia_gpu(self) -> GPUDevice | None: """Detect NVIDIA GPU with detailed info.""" returncode, stdout, _ = self._run_command( @@ memory = int(float(parts[1].strip())) if len(parts) > 1 else 0 - # Check power state - power_returncode, power_stdout, _ = self._run_command( - ["cat", "/sys/bus/pci/devices/0000:01:00.0/power/runtime_status"] - ) - power_state = power_stdout.strip() if power_returncode == 0 else "unknown" + # Check power state (best-effort; PCI address varies by machine) + power_state = "unknown" + for candidate in Path("/sys/bus/pci/devices").glob("*/power/runtime_status"): + power_returncode, power_stdout, _ = self._run_command(["cat", str(candidate)]) + if power_returncode == 0: + # Heuristic: first readable runtime_status is better than a wrong hard-coded one. + power_state = power_stdout.strip() + break return GPUDevice(
528-575:switchUX: includecomputein the “Specify a mode” / “Use: …” text.
mode_mapsupportscompute, but the user-facing hints don’t.Proposed fix
@@ elif action == "switch": if not mode: - cx_print("Specify a mode: integrated, hybrid, nvidia", "error") + cx_print("Specify a mode: integrated, hybrid, nvidia, compute", "error") return 1 @@ if not target_mode: - cx_print(f"Unknown mode: {mode}. Use: integrated, hybrid, nvidia", "error") + cx_print( + f"Unknown mode: {mode}. Use: integrated, hybrid, nvidia, compute", + "error", + ) return 1cortex/cli.py (1)
991-1110: Conflict detection should skip non-package installs (pip/shell pipelines), or it may prompt bogus removals.
After thepytorch-cpu ...rewrite,softwarebecomes a pip/&&pipeline, but Line 1085+ will still attemptresolver.resolve_dependencies()for tokens liketorch,numpy, etc. That can (a) waste time, (b) throw, or (c) detect irrelevant “conflicts” and injectsudo apt-get removecommands.Safer gating approach
@@ - # Detect package conflicts and apply interactive resolutions when possible. + # Detect apt package conflicts and apply interactive resolutions when possible. try: @@ - for token in software.split(): + # Only attempt apt conflict detection for simple "package list" requests. + if any(x in software for x in ("&&", "||", "|", ";")) or "pip" in software: + raise RuntimeError("Skipping apt conflict detection for non-apt install request") + + for token in software.split(): if not is_valid_package_token(token): continue graph = resolver.resolve_dependencies(token)
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 466-471: _get_prefs_manager currently returns whatever is in
self._prefs_manager even if it's None or the wrong type; change it to validate
the attribute: if self has no _prefs_manager or its value is None or not an
instance of PreferencesManager, instantiate PreferencesManager and assign it to
self._prefs_manager, then return it. Use isinstance(self._prefs_manager,
PreferencesManager) to check type and ensure a valid PreferencesManager is
always returned.
In `@tests/test_conflict_ui.py`:
- Around line 190-212: test_multiple_conflict_preferences can raise if
self.prefs_manager.get("conflicts.saved_resolutions") returns None; when
building the merged dict in the loop use a safe default (e.g., treat None as
{}), so update the logic around prefs_manager.get("conflicts.saved_resolutions")
in test_multiple_conflict_preferences (and any similar usage) to coalesce None
to an empty dict before merging and then call prefs_manager.set(...) and
prefs_manager.save() as before.
♻️ Duplicate comments (5)
.gitignore (1)
155-163: Home-directory patterns use literal~and won't match$HOMEpaths.The patterns on lines 160-161 (
~/.config/cortex/preferences.yamland~/.config/cortex/*.backup.*) treat~as a literal directory name, not the user's home directory. Since these config files live outside the repository, these rules have no effect and should be removed.🔧 Suggested fix
# User preferences and configuration .cortex/ *.yaml.bak -~/.config/cortex/preferences.yaml -~/.config/cortex/*.backup.* /tmp/ .envtests/test_conflict_ui.py (2)
276-292: Remove unusedinputpatch intest_config_reset_command.
The implementation ofconfig("reset")doesn’t prompt, so the patch is misleading noise.
376-391: Strengthentest_dependency_resolver_detects_conflictsassertions.
Right now it doesn’t verify conflict extraction, only that subprocess was called and a graph exists.Proposed fix
@@ graph = resolver.resolve_dependencies("nginx") - # Verify the resolver was called - self.assertTrue(mock_run.called) - # Verify graph object was created - self.assertIsNotNone(graph) + mock_run.assert_called() + self.assertIsNotNone(graph) + self.assertTrue(getattr(graph, "conflicts", None))cortex/cli.py (1)
534-556: TreatKeyboardInterruptas “don’t save” in_ask_save_preference()(don’t abort install).
Line 541 only handlesEOFError; Ctrl+C here can bubble up after the user already picked a resolution.Proposed fix
@@ - except EOFError: + except (EOFError, KeyboardInterrupt): returncortex/user_preferences.py (1)
263-265: Add error handling for invalid key paths inset().If a user provides an invalid key path like
"nonexistent.key",getattr(obj, part)on line 265 raises anAttributeErrorwith an unclear message. This was noted in a previous review.🔧 Suggested fix
# Navigate to parent object - for part in parts[:-1]: - obj = getattr(obj, part) + try: + for part in parts[:-1]: + obj = getattr(obj, part) + except AttributeError: + raise PreferencesError(f"Invalid preference key: '{key}'") from None # Set the final attribute attr_name = parts[-1] - current_value = getattr(obj, attr_name) + try: + current_value = getattr(obj, attr_name) + except AttributeError: + raise PreferencesError(f"Unknown preference: '{key}'") from None
🧹 Nitpick comments (12)
cortex/stdin_handler.py (1)
227-232: Clarify conditional precedence with explicit parentheses.The multi-line formatting makes the conditional expression's scope ambiguous. Due to Python's operator precedence, the ternary
if len(lines) > 1 else Falsebinds only to the equality check, not the entireandexpression. While functionally correct, the visual layout suggests otherwise.♻️ Suggested clarification
- if ( - "," in first_line and lines[0].count(",") == lines[1].count(",") - if len(lines) > 1 - else False - ): + if len(lines) > 1 and "," in first_line and lines[0].count(",") == lines[1].count(","): return "csv"This reordering makes the guard check explicit and avoids the ternary expression entirely.
tests/test_interpreter.py (1)
178-185: Unusedmock_cachesetup creates misleading test code.The
mock_cacheis created and passed to the constructor at lines 178-181, but immediately overwritten toNoneat line 185. This leaves the mock setup unused and makes the test logic confusing.If the intent is to disable caching entirely for determinism, simplify by passing
cache=Noneto the constructor directly:♻️ Suggested simplification
- mock_cache = Mock() - mock_cache.get_commands.return_value = None - - interpreter = CommandInterpreter(api_key=self.api_key, provider="openai", cache=mock_cache) + interpreter = CommandInterpreter(api_key=self.api_key, provider="openai", cache=None) interpreter.client = mock_client - # Disable semantic cache to make this test deterministic even if a prior run - # populated a persistent cache on disk. - interpreter.cache = Nonecortex/api_key_detector.py (1)
407-422: Security improvement approved, but update module docstring for consistency.The guard logic is sound: checking
CORTEX_DISABLE_CWD_DOTENV, ensuring HOME exists, and verifying CWD is under HOME prevents accidental pickup of.envfiles from untrusted directories. TheValueErrorfallback foris_relative_to()correctly handles cross-filesystem paths.However, the module docstring (lines 16-17) still lists ".env in current directory" as detection item 9 unconditionally. Consider updating it to note this is conditional, e.g., "(only when under $HOME)".
📝 Suggested docstring update
Detection order (highest priority first): 1. CORTEX_PROVIDER=ollama environment variable (for explicit Ollama mode) 2. API key environment variables: ANTHROPIC_API_KEY, OPENAI_API_KEY 3. Encrypted storage (~/.cortex/environments/cortex.json) - secure Fernet-encrypted storage 4. Cached key location (~/.cortex/.api_key_cache) 5. Saved Ollama provider preference in ~/.cortex/.env (CORTEX_PROVIDER=ollama) 6. API keys in ~/.cortex/.env 7. ~/.config/anthropic/credentials.json (Claude CLI location) 8. ~/.config/openai/credentials.json -9. .env in current directory +9. .env in current directory (only when CWD is under $HOME and not disabled via CORTEX_DISABLE_CWD_DOTENV)cortex/branding.py (1)
14-14: Consider removing unusedOptionalimport.The file uses the modern
X | Noneunion syntax throughout (e.g.,str | Noneon lines 144, 226, 296) rather thanOptional[X]. TheOptionalimport appears unused.Proposed fix
-from typing import Optional +# No typing imports needed - using built-in generics and union syntaxcortex/printer_setup.py (2)
223-242: Simplify printerstateparsing for readability/maintenance.
The nested conditional at Line 223–231 is hard to scan and easy to mis-edit.Proposed refactor
@@ name = parts[1] - state = ( - "idle" - if "is idle" in line - else ( - "printing" - if "printing" in line - else "disabled" if "disabled" in line else "unknown" - ) - ) + if "is idle" in line: + state = "idle" + elif "printing" in line: + state = "printing" + elif "disabled" in line: + state = "disabled" + else: + state = "unknown" devices.append(
347-410: Adddry_runparameter tosetup_printer()to align with project guidelines.The method performs a real system mutation via
lpadminwithout offering a preview-only mode. Per coding guidelines, "Use dry-run mode by default for all installation operations." Add adry_run: bool = Falseparameter; when True, return the commands that would execute instead of applying them. This matches the pattern used across the codebase ininstallation_history.rollback(),config_manager.import_configuration(),updater.update(), and similar methods.tests/test_conflict_ui.py (1)
15-29: Removesys.path.insert()from test files for cleaner imports.
CI already installs the package in editable mode (pip install -e .), making the sys.path manipulation unnecessary. This pattern appears across 25+ test files and is acknowledged in the docs as fragile. Consider removing all occurrences when refactoring the test suite.cortex/user_preferences.py (5)
58-86: Clarify the relationship betweenAISettings.modelandLLMSettings.model.Both
AISettings(line 62) andLLMSettings(line 85) define amodelfield with different defaults (claude-sonnet-4vsgpt-4). This could confuse users about which setting controls which behavior.Consider either:
- Adding docstrings clarifying the distinct purposes of each setting, or
- Consolidating into a single location if they serve the same purpose
126-128: Consider setting restrictive permissions on the config directory.When creating the config directory, the default permissions depend on the system umask. If the preferences file may contain sensitive data (e.g., conflict resolutions that reveal system packages), consider explicitly setting restrictive permissions.
config_dir.mkdir(parents=True, exist_ok=True, mode=0o700)
170-184: Handle unexpected keys in configuration data to avoidTypeErroron load.If the YAML file contains keys not present in the dataclass definitions (e.g., from manual editing or a newer version of Cortex), the
**spreadon lines 172-180 will pass them to the dataclass constructors, causing aTypeError.Consider filtering to known fields:
# Example for ConfirmationSettings known_fields = {f.name for f in dataclasses.fields(ConfirmationSettings)} confirmations_data = {k: v for k, v in data.get("confirmations", {}).items() if k in known_fields} confirmations=ConfirmationSettings(**confirmations_data),Alternatively, catch
TypeErrorand log a warning about unknown keys while falling back to defaults.
207-210: Consider handling backup creation failures gracefully.If
shutil.copy2fails (e.g., due to permissions or disk space), the entiresave()operation fails. Since the actual write uses an atomic pattern that protects the original file, a backup failure shouldn't necessarily abort the save.🔧 Suggested fix
# Create backup if file exists if self.config_path.exists(): backup_path = self.config_path.with_suffix(".yaml.bak") - shutil.copy2(self.config_path, backup_path) + try: + shutil.copy2(self.config_path, backup_path) + except OSError as e: + print(f"[WARNING] Could not create backup: {e}")
414-419: Consider removing or gating the main block for production.The
if __name__ == "__main__"block is useful for development testing but writes to the actual user config directory (~/.config/cortex/). Consider either removing it before release or adding a note that it modifies real user preferences.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
.github/workflows/automation.yml.gitignorecortex/api_key_detector.pycortex/benchmark.pycortex/branding.pycortex/cli.pycortex/config_manager.pycortex/context_memory.pycortex/doctor.pycortex/gpu_manager.pycortex/health_score.pycortex/install_parallel.pycortex/installation_history.pycortex/kernel_features/accelerator_limits.pycortex/kernel_features/kv_cache_manager.pycortex/licensing.pycortex/llm_router.pycortex/output_formatter.pycortex/printer_setup.pycortex/sandbox/docker_sandbox.pycortex/sandbox/sandbox_executor.pycortex/semver_resolver.pycortex/stdin_handler.pycortex/systemd_helper.pycortex/update_checker.pycortex/user_preferences.pycortex/version_manager.pycortex/wifi_driver.pydocs/IMPLEMENTATION_SUMMARY_ISSUE_42.mdrequirements-dev.txttests/integration/test_end_to_end.pytests/langchain_py313/test_basic_imports.pytests/test_api_key_detector.pytests/test_benchmark.pytests/test_conflict_ui.pytests/test_dependency_importer.pytests/test_env_loader.pytests/test_env_manager.pytests/test_gpu_manager.pytests/test_hardware_detection.pytests/test_health_score.pytests/test_interpreter.pytests/test_network_config.pytests/test_ollama_integration.pytests/test_parallel_llm.pytests/test_permission_manager.pytests/test_printer_setup.pytests/test_shell_env_analyzer.pytests/test_stdin_handler.pytests/test_systemd_helper.pytests/test_update_checker.pytests/test_updater.pytests/test_wifi_driver.py
💤 Files with no reviewable changes (12)
- cortex/kernel_features/accelerator_limits.py
- cortex/doctor.py
- tests/test_dependency_importer.py
- cortex/kernel_features/kv_cache_manager.py
- tests/test_env_manager.py
- cortex/installation_history.py
- tests/langchain_py313/test_basic_imports.py
- cortex/context_memory.py
- tests/test_network_config.py
- tests/test_env_loader.py
- cortex/sandbox/docker_sandbox.py
- tests/test_permission_manager.py
✅ Files skipped from review due to trivial changes (8)
- tests/test_shell_env_analyzer.py
- cortex/update_checker.py
- cortex/benchmark.py
- tests/test_api_key_detector.py
- cortex/systemd_helper.py
- cortex/wifi_driver.py
- tests/test_updater.py
- tests/test_health_score.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/automation.yml
- tests/test_hardware_detection.py
- cortex/sandbox/sandbox_executor.py
- cortex/config_manager.py
- requirements-dev.txt
- cortex/install_parallel.py
- tests/integration/test_end_to_end.py
- cortex/llm_router.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
tests/test_ollama_integration.pytests/test_update_checker.pycortex/stdin_handler.pycortex/version_manager.pytests/test_conflict_ui.pytests/test_gpu_manager.pytests/test_benchmark.pycortex/gpu_manager.pytests/test_interpreter.pycortex/health_score.pytests/test_stdin_handler.pytests/test_wifi_driver.pycortex/semver_resolver.pycortex/output_formatter.pycortex/api_key_detector.pycortex/branding.pycortex/printer_setup.pycortex/licensing.pycortex/user_preferences.pytests/test_printer_setup.pytests/test_parallel_llm.pycortex/cli.pytests/test_systemd_helper.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_ollama_integration.pytests/test_update_checker.pytests/test_conflict_ui.pytests/test_gpu_manager.pytests/test_benchmark.pytests/test_interpreter.pytests/test_stdin_handler.pytests/test_wifi_driver.pytests/test_printer_setup.pytests/test_parallel_llm.pytests/test_systemd_helper.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
.gitignore
🧬 Code graph analysis (10)
tests/test_update_checker.py (1)
cortex/update_checker.py (1)
UpdateCheckResult(102-111)
tests/test_conflict_ui.py (1)
cortex/cli.py (4)
InstallationCancelledError(61-62)_resolve_conflicts_interactive(472-532)_ask_save_preference(534-555)config(557-632)
tests/test_gpu_manager.py (1)
cortex/gpu_manager.py (3)
GPUDevice(46-56)GPUVendor(36-42)GPUState(60-76)
tests/test_benchmark.py (1)
cortex/benchmark.py (2)
BenchmarkReport(55-78)BenchmarkResult(44-51)
cortex/gpu_manager.py (3)
cortex/health_score.py (1)
_run_command(146-161)cortex/printer_setup.py (1)
_run_command(106-114)cortex/wifi_driver.py (1)
_run_command(193-208)
tests/test_interpreter.py (1)
tests/test_graceful_degradation.py (1)
cache(28-31)
cortex/health_score.py (3)
cortex/gpu_manager.py (1)
_run_command(131-139)cortex/printer_setup.py (1)
_run_command(106-114)cortex/wifi_driver.py (1)
_run_command(193-208)
tests/test_stdin_handler.py (1)
cortex/stdin_handler.py (1)
StdinData(37-50)
cortex/cli.py (2)
cortex/user_preferences.py (4)
format_preference_value(391-401)get(231-250)save(205-229)get_all_settings(370-372)cortex/first_run_wizard.py (1)
_print_error(778-780)
tests/test_systemd_helper.py (1)
cortex/systemd_helper.py (2)
ServiceConfig(104-123)_run_systemctl(162-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (51)
tests/test_systemd_helper.py (4)
7-20: LGTM!The import reorganization follows alphabetical ordering for
unittest.mockmembers, and the new constant imports (FAILURE_SOLUTIONS,SERVICE_STATE_EXPLANATIONS,SUB_STATE_EXPLANATIONS) are properly used in theTestServiceStateExplanationsandTestFailureSolutionstest classes.
26-34: LGTM!The single-line instantiation is clean and readable. The test adequately verifies the default values for
ServiceConfig.
133-139: LGTM!The single-line
MagicMockinstantiation is readable and the test properly verifies successfulsystemctlcommand execution.
141-149: LGTM!The test correctly handles the timeout scenario. The local
subprocessimport for accessingTimeoutExpiredis acceptable given it's only needed in this specific test.tests/test_update_checker.py (2)
13-20: LGTM!Import reordering is cosmetic and maintains proper alphabetical ordering. All imported symbols (
CACHE_TTL_SECONDS,ReleaseInfo,UpdateChecker,UpdateCheckResult,check_for_updates,should_notify_update) are used throughout the test file.
98-116: LGTM!The collapsed single-line patch is functionally equivalent and improves readability. The test fixture setup properly:
- Uses a temporary directory for isolation
- Patches both
CACHE_DIRandUPDATE_CACHE_FILE- Cleans up patches and temp directory in
tearDowncortex/stdin_handler.py (4)
139-147: LGTM!The MIDDLE truncation mode logic correctly preserves head and tail portions while inserting a clear truncation indicator. The single-line concatenation is readable and functionally equivalent.
151-156: LGTM!The byte-limit truncation correctly uses
errors="replace"to handle potential mid-character splits when slicing at arbitrary byte boundaries. This is an acceptable trade-off for size-limiting purposes.
234-236: LGTM!The container log detection using
any()with a generator expression is idiomatic and efficient.
238-240: LGTM!The system log detection follows the same idiomatic pattern as container log detection.
tests/test_benchmark.py (4)
11-21: LGTM — Import ordering improvements.The import reordering follows PEP 8 conventions with proper grouping (stdlib → third-party → local) and alphabetical ordering within blocks. No functional changes.
29-31: LGTM — Cleaner single-line formatting.The reformatted
BenchmarkResultinitializations are readable and consistent with other single-line usages in the file.Also applies to: 37-40
56-62: LGTM — Consistent formatting.The
BenchmarkReportinitialization formatting is appropriate for each context — single-line where concise and multi-line where readability benefits.Also applies to: 239-241
286-294: LGTM — Concise mock setup.The single-line
MagicMockconfigurations are clear and maintainable.tests/test_ollama_integration.py (2)
53-55: LGTM! Clean refactor to useshutil.which.Using
shutil.which()is the idiomatic Python approach for checking executable availability—cleaner and more efficient than spawning a subprocess. This also properly utilizes theshutilimport.
92-101: LGTM! Fixes the NameError by calling the correct function.The function now correctly delegates to
is_ollama_installed()instead of the undefined_ollama_installed(), which would have caused a runtimeNameError.cortex/api_key_detector.py (1)
399-405: LGTM!Good refactor to extract
Path.home()into a local variable, avoiding repeated calls. The explicit type annotation onlocationsimproves readability.cortex/licensing.py (4)
185-194: LGTM!The blank line formatting in the decorator follows PEP 8 conventions for readability.
233-240: LGTM!The reformatted print statement maintains correct ASCII box alignment with proper closing borders.
289-300: LGTM!The inline
json.dumps()call is clean and correctly serializes the license data. The structure includes all necessary fields for license validation.
326-330: LGTM!The function correctly returns the hostname with proper type annotation. The lazy import of
platformis acceptable for this utility function.cortex/output_formatter.py (1)
14-14: LGTM!The removal of
ListandTuplefrom typing imports is correct — the file uses Python 3.9+ built-in generic types (list[...],tuple[...]) throughout.tests/test_parallel_llm.py (1)
12-12: LGTM!The removal of
patchfrom imports is correct — the test file only usesMockfor creating mock objects and doesn't usepatchanywhere.cortex/version_manager.py (1)
17-17: LGTM!This is a minor formatting adjustment adding a blank line before the
UpdateChannelenum for better visual separation.cortex/branding.py (1)
319-323: LGTM!The multi-line reformatting of
cx_warningimproves readability without changing behavior.tests/test_stdin_handler.py (4)
10-10: LGTM!Import order is now alphabetical (
MagicMockbeforepatch), which is consistent with PEP 8 conventions.
191-192: LGTM!Consistent string literal style.
341-341: LGTM!Single-line
StdinDatainitialization is more concise for simple test cases.
479-480: Good modernization.Using
OSErrorinstead ofIOErroris correct — since Python 3.3,IOErroris an alias forOSError. Using the canonical class name is the preferred approach.tests/test_wifi_driver.py (1)
7-21: LGTM!The import reordering and addition of
BLUETOOTH_DRIVERSandDRIVER_DATABASEconstants are appropriate. The tests at lines 113-129 properly validate these newly exported constants from thecortex.wifi_drivermodule.tests/test_gpu_manager.py (4)
7-20: LGTM!Import reordering and addition of
APP_GPU_RECOMMENDATIONSandBATTERY_IMPACTconstants are appropriate. These constants are properly tested inTestBatteryImpact(lines 110-123) andTestAppGPURecommendations(lines 125-140).
50-55: LGTM!Single-line construction of
GPUDevicemaintains the same behavior and improves readability.
167-170: LGTM!Condensed
MagicMockinitialization is cleaner and functionally equivalent.
294-309: LGTM!The reformatted
GPUStateandGPUDeviceconstructions maintain identical behavior while improving code layout consistency..gitignore (2)
125-125: LGTM!Adding
.cursor/to the IDE ignore section is appropriate for users of the Cursor editor.
185-189: LGTM!The data file ignore pattern with negation (
!data/contributors.json) correctly excludes all JSON/CSV files in thedata/directory while preservingcontributors.jsonfor version control.cortex/health_score.py (4)
146-161: LGTM!The
_run_commandmethod follows the established pattern used across other modules (gpu_manager.py,printer_setup.py,wifi_driver.py) with proper exception handling forTimeoutExpired,FileNotFoundError, and generic exceptions.
310-313: LGTM!Single-line formatting for the
_run_commandcall maintains the same behavior.
483-484: LGTM!The condensed dict comprehension for
factorsis functionally equivalent and improves readability.
584-590: LGTM!The single-line conditional expression for
score_coloris cleaner while maintaining the same logic.cortex/semver_resolver.py (6)
143-147: LGTM!The tilde constraint logic is correct. The condition properly enforces that for
~1.2.3:
- Line 145-146 rejects versions below the constraint (
version < self.version)- Line 147 ensures the version stays within the same
major.minorrange
203-229: LGTM!The
_constraints_compatiblemethod signature reformatting maintains the same logic for detecting compatibility between version constraints.
401-422: LGTM!Single-line signature formatting for
add_dependencypreserves functionality.
442-504: LGTM!The
suggest_resolutionsmethod signature reformatting maintains the resolution strategy logic.
506-537: LGTM!The
_find_common_version_strategysignature reformatting preserves the heuristic logic for finding compatible versions.
699-700: LGTM!Condensed console output maintains the same behavior.
tests/test_printer_setup.py (1)
7-20: LGTM: tests updated to validate newly-exported constants + minor cleanup.Also applies to: 165-175
cortex/cli.py (1)
3259-3270: LGTM: CLI now accepts multi-packageinstallargs and passes them through cleanly.Also applies to: 3847-3855
cortex/user_preferences.py (3)
305-329: LGTM on validation logic.The validation method correctly checks bounds for
max_suggestions(1-20),frequency_hours(≥1), and validates language codes. The enum validation is effectively handled by the type coercion inset()which raisesPreferencesErrorfor invalid enum values.
331-368: LGTM on export/import error handling.Both
export_json()andimport_json()now have comprehensive error handling for file I/O and JSON parsing errors, with clear error messages wrapped inPreferencesError.
390-411: LGTM on CLI helper functions.The
format_preference_valueandprint_all_preferencesfunctions provide clean formatting for different value types and integrate well with the YAML-based storage.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def _get_prefs_manager(self) -> PreferencesManager: | ||
| """Get preferences manager instance.""" | ||
| if not hasattr(self, "_prefs_manager"): | ||
| self._prefs_manager = PreferencesManager() | ||
| return self._prefs_manager | ||
|
|
There was a problem hiding this comment.
Harden _get_prefs_manager() against _prefs_manager = None / wrong type.
Right now, if something sets self._prefs_manager = None, you’ll return None and fail later.
Proposed fix
def _get_prefs_manager(self) -> PreferencesManager:
"""Get preferences manager instance."""
- if not hasattr(self, "_prefs_manager"):
- self._prefs_manager = PreferencesManager()
+ if not isinstance(getattr(self, "_prefs_manager", None), PreferencesManager):
+ self._prefs_manager = PreferencesManager()
return self._prefs_manager🤖 Prompt for AI Agents
In `@cortex/cli.py` around lines 466 - 471, _get_prefs_manager currently returns
whatever is in self._prefs_manager even if it's None or the wrong type; change
it to validate the attribute: if self has no _prefs_manager or its value is None
or not an instance of PreferencesManager, instantiate PreferencesManager and
assign it to self._prefs_manager, then return it. Use
isinstance(self._prefs_manager, PreferencesManager) to check type and ensure a
valid PreferencesManager is always returned.
| def test_multiple_conflict_preferences(self): | ||
| """Test saving and retrieving multiple conflict preferences.""" | ||
| # Save multiple preferences (using min:max format) | ||
| resolutions = { | ||
| "apache2:nginx": "nginx", | ||
| "mariadb-server:mysql-server": "mariadb-server", | ||
| "emacs:vim": "vim", | ||
| } | ||
|
|
||
| for conflict, choice in resolutions.items(): | ||
| self.prefs_manager.set( | ||
| "conflicts.saved_resolutions", | ||
| {**self.prefs_manager.get("conflicts.saved_resolutions"), conflict: choice}, | ||
| ) | ||
|
|
||
| self.prefs_manager.save() | ||
|
|
||
| # Verify all preferences were saved | ||
| saved = self.prefs_manager.get("conflicts.saved_resolutions") | ||
| for conflict, choice in resolutions.items(): | ||
| self.assertIn(conflict, saved) | ||
| self.assertEqual(saved[conflict], choice) | ||
|
|
There was a problem hiding this comment.
Guard against None when merging conflict-resolution dicts.
If self.prefs_manager.get("conflicts.saved_resolutions") can ever be None, Line 202 will throw.
Proposed fix
- {**self.prefs_manager.get("conflicts.saved_resolutions"), conflict: choice},
+ {
+ **(self.prefs_manager.get("conflicts.saved_resolutions") or {}),
+ conflict: choice,
+ },🤖 Prompt for AI Agents
In `@tests/test_conflict_ui.py` around lines 190 - 212,
test_multiple_conflict_preferences can raise if
self.prefs_manager.get("conflicts.saved_resolutions") returns None; when
building the merged dict in the loop use a safe default (e.g., treat None as
{}), so update the logic around prefs_manager.get("conflicts.saved_resolutions")
in test_multiple_conflict_preferences (and any similar usage) to coalesce None
to an empty dict before merging and then call prefs_manager.set(...) and
prefs_manager.save() as before.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Sahilbhatane Kindly resolve conflicts.
7 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
Improvements
Chores / Tests
✏️ Tip: You can customize this high-level summary in your review settings.