Add package conflict resolver#72
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a standalone resolver CLI (apt/scripts/resolve-conflicts.py) that detects APT Conflicts/Breaks and optional JSON rules, persists user “keep” choices, integrates into the ChangesPackage Conflict Resolver
Sequence DiagramsequenceDiagram
participant User
participant Resolver as resolve-conflicts.py
participant APT as apt-cache
participant RulesFile as Rules JSON
participant PrefsFile as Preferences JSON
User->>Resolver: resolve-conflicts docker.io podman --rules rules.json
Resolver->>APT: apt-cache show docker.io
APT-->>Resolver: Conflicts/Breaks fields
Resolver->>APT: apt-cache show podman
APT-->>Resolver: Conflicts/Breaks fields
Resolver->>RulesFile: load rules.json (optional)
RulesFile-->>Resolver: rule conflicts
Resolver->>PrefsFile: load saved preferences
PrefsFile-->>Resolver: previous choices (if any)
Resolver-->>User: display conflicts and options (exit 1)
User->>Resolver: --choice podman
Resolver->>PrefsFile: save docker.io|podman -> podman
Resolver-->>User: preference saved (exit 0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a package conflict resolver tool to detect and explain package conflicts before installation, integrating it into the cx CLI wrapper and providing comprehensive documentation and tests. Review feedback suggests optimizing the detection logic by avoiding redundant apt-cache calls, improving the robustness of JSON rule loading with better error handling, and enhancing performance by reducing file I/O operations when saving user preferences.
| requested = set(packages) | ||
| conflicts: dict[str, Conflict] = {} | ||
|
|
||
| for package in packages: |
There was a problem hiding this comment.
The loop iterates over the packages list, which may contain duplicates if the user provides the same package multiple times in the command line. This results in redundant apt-cache calls. Iterating over the requested set instead avoids this unnecessary overhead.
| for package in packages: | |
| for package in requested: |
| with path.open("r", encoding="utf-8") as handle: | ||
| data = json.load(handle) |
There was a problem hiding this comment.
The load_rule_conflicts function lacks error handling for JSON parsing and does not verify that the loaded data is a dictionary. If the rules file is malformed or contains an unexpected structure (e.g., a list at the top level), the script will crash with a JSONDecodeError or AttributeError. Consider adding a try-except block and a type check, similar to the implementation in load_preferences.
try:
with path.open("r", encoding="utf-8") as handle:
data = json.load(handle)
except (OSError, json.JSONDecodeError):
return []
if not isinstance(data, dict):
return []| for conflict in matching: | ||
| save_preference(preferences_path, conflict.key, args.choice) | ||
| preferences[conflict.key] = args.choice |
There was a problem hiding this comment.
Calling save_preference inside a loop causes multiple redundant file read/write operations if the user's choice matches multiple conflicts. It is more efficient to update the preferences dictionary in memory and perform a single write operation after the loop.
for conflict in matching:
preferences[conflict.key] = args.choice
# Save all updated preferences at once
preferences_path.parent.mkdir(parents=True, exist_ok=True)
with preferences_path.open("w", encoding="utf-8") as handle:
json.dump(preferences, handle, indent=2, sort_keys=True)
handle.write("\n")![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/resolve-conflicts-test.sh (1)
35-38: ⚡ Quick winInvoke the resolver via
python3for test portability.These calls currently depend on the script executable bit being preserved. Using
python3avoids false negatives in CI/local environments.Suggested patch
-OUTPUT="$("$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ +OUTPUT="$(python3 "$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ --rules "$RULES_FILE" \ --preferences "$PREFS_FILE" \ docker.io podman 2>&1)" @@ -"$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ +python3 "$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ --rules "$RULES_FILE" \ --preferences "$PREFS_FILE" \ --choice podman \ docker.io podman >/dev/null || true @@ -JSON_OUTPUT="$("$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ +JSON_OUTPUT="$(python3 "$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ --rules "$RULES_FILE" \ --preferences "$PREFS_FILE" \ --json \ docker.io podman || true)" @@ -NO_CONFLICT_OUTPUT="$("$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ +NO_CONFLICT_OUTPUT="$(python3 "$ROOT_DIR/apt/scripts/resolve-conflicts.py" \ --rules "$RULES_FILE" \ --preferences "$PREFS_FILE" \ nginx curl)"Also applies to: 53-57, 62-66, 69-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/resolve-conflicts-test.sh` around lines 35 - 38, Change the test invocations that call the resolver script directly to run it with python3 to avoid relying on the executable bit; specifically update the calls that execute "resolve-conflicts.py" (the occurrences that currently run "$ROOT_DIR/apt/scripts/resolve-conflicts.py" with various arguments such as --rules "$RULES_FILE" --preferences "$PREFS_FILE" and the package names) so they are prefixed with "python3" (i.e. run "python3 $ROOT_DIR/apt/scripts/resolve-conflicts.py ...") — apply the same change for the other occurrences noted (around the blocks at 53-57, 62-66, and 69-72) so tests invoke the script via python3 for portability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apt/scripts/resolve-conflicts.py`:
- Line 211: The call to load_rule_conflicts uses Path(args.rules) without
expanding ~, so pass an expanded path: replace Path(args.rules) with
Path(args.rules).expanduser() (i.e., call
load_rule_conflicts(Path(args.rules).expanduser(), args.packages)); also scan
for other uses of args.rules in this file and apply .expanduser() there to match
how preferences are handled.
- Around line 22-43: The parser currently uses APT_RELATION_RE.search(...)
inside parse_relation_names which only returns the first token per
comma-separated clause and misses alternatives like the second token in "pkg-a |
pkg-b"; change parse_relation_names to iterate all matches for each part (e.g.,
use APT_RELATION_RE.findall(...) or APT_RELATION_RE.finditer(...)) and add every
match to the names set so all package names in a relation expression are
captured; keep using the existing APT_RELATION_RE and the same part.strip()
logic, just collect all matches instead of taking only the first.
- Around line 104-106: When loading the optional rules file (the with
path.open(...) block that calls json.load), catch JSON parsing errors so a
malformed --rules file doesn't crash the script; wrap json.load(path) in a
try/except that handles json.JSONDecodeError (and optionally
ValueError/OSError), log a warning mentioning the --rules file path and error,
and set data to an empty/default rules structure so processing continues instead
of raising.
---
Nitpick comments:
In `@tests/resolve-conflicts-test.sh`:
- Around line 35-38: Change the test invocations that call the resolver script
directly to run it with python3 to avoid relying on the executable bit;
specifically update the calls that execute "resolve-conflicts.py" (the
occurrences that currently run "$ROOT_DIR/apt/scripts/resolve-conflicts.py" with
various arguments such as --rules "$RULES_FILE" --preferences "$PREFS_FILE" and
the package names) so they are prefixed with "python3" (i.e. run "python3
$ROOT_DIR/apt/scripts/resolve-conflicts.py ...") — apply the same change for the
other occurrences noted (around the blocks at 53-57, 62-66, and 69-72) so tests
invoke the script via python3 for portability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e1073de-c9ba-463f-8ef6-c9fb86f7560f
📒 Files selected for processing (5)
Makefileapt/README.mdapt/scripts/resolve-conflicts.pypackages/cx-core/debian/rulestests/resolve-conflicts-test.sh
dnesdan
force-pushed
the
codex/package-conflict-resolver-45
branch
from
e7e0bcc to
ea9d302
Compare
|
Updated this with the review fixes:\n\n- collect all package names in relation expressions like pkg-a | pkg-b\n- expand ~ in the rules path\n- malformed rules json now warns and keeps going\n- tests call the helper through python3\n- added coverage for malformed rules and relation parsing\n\nRe-ran:\n- python3 -m py_compile apt/scripts/resolve-conflicts.py\n- bash -n tests/resolve-conflicts-test.sh\n- make test-conflicts\n- git diff --check |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apt/scripts/resolve-conflicts.py`:
- Around line 51-53: The subprocess invocation uses "apt-cache" by name which is
a PATH-based execution risk; update the subprocess.run call (the line assigning
result in resolve-conflicts.py) to call the absolute binary (e.g.,
"/usr/bin/apt-cache") or resolve the path via shutil.which and fail if not
found, then use that absolute path in the argv list passed to subprocess.run to
eliminate PATH hijack risk.
- Around line 156-161: The save_preferences function currently writes directly
to the final path which can produce a truncated/corrupt JSON if interrupted;
change save_preferences to write atomically by creating a temporary file in the
same directory (use a unique temp filename based on the target Path), write JSON
to that temp file, flush and os.fsync the file descriptor to ensure data is on
disk, then os.replace (atomic rename) the temp file to the original path; ensure
the parent directory is created beforehand and clean up the temp file on errors.
- Around line 118-124: The code assumes `data` is a dict with a list under
"conflicts" and that each conflict is a dict, which causes AttributeError on
malformed JSON; update the logic in resolve-conflicts.py (the loop starting with
data.get("conflicts", [])) to first validate types: check that `data` is an
instance of dict, that `conflicts = data.get("conflicts")` is a list (or treat
non-list as absent), and for each item verify `isinstance(item, dict)` before
calling item.get or iterating its "packages"/"options"; if validation fails,
skip that entry (or treat rules as not provided) so --rules remains optional and
processing continues without raising.
In `@packages/cx-core/debian/rules`:
- Around line 38-40: The current install pre-check uses the raw argv length
check 'if [ "$$1" = "install" ] && [ "$$#" -gt 2 ]' which counts flags and
causes resolve-conflicts.py to be invoked when flags (like --yes) are present;
change the gate to count package operands only (non-flag args) before calling
python3 /usr/share/cx/resolve-conflicts.py. Implement a small loop in the cx
wrapper script that iterates over "$@" and increments a counter for args not
starting with '-' (or use getopt to parse), then use that counter (e.g.,
operands_count -gt 1) in the conditional instead of "$#"; keep the call to
resolve-conflicts.py and its argument expansion "$${@:2}" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb551b9c-3e81-4da6-a8f0-2f2acc97e443
📒 Files selected for processing (5)
Makefileapt/README.mdapt/scripts/resolve-conflicts.pypackages/cx-core/debian/rulestests/resolve-conflicts-test.sh
✅ Files skipped from review due to trivial changes (1)
- apt/README.md
|
Pushed a small follow-up in 4a3c4b0 for the review notes:
Local checks passing: python compile, bash syntax, git diff --check, and make test-conflicts. |
1 participant
Adds a small conflict resolver for planned package installs.
What changed:
--interactiveprompt for choosing the preferred packagecx resolve-conflicts ...Checks run:
python3 -m py_compile apt/scripts/resolve-conflicts.pybash -n tests/resolve-conflicts-test.shmake test-conflictsgit diff --checkgit diff --cached --checkCloses #45
Summary by CodeRabbit
New Features
resolve-conflictscommand available in the CLI.Documentation