feat: add dependency conflict resolver#75
Conversation
📝 WalkthroughWalkthroughAdds a Bash dependency-resolver script that checks package candidates, simulates APT installs, prints dependency and orphan-review output, and exits on broken or removal states. Also adds a mocked test script and a Makefile target to run it. ChangesDependency resolver CLI and test wiring
Sequence Diagram(s)sequenceDiagram
participant Makefile
participant "tests/dependency-resolver-tests.sh" as Tests
participant "scripts/cx-dep-resolver.sh" as Resolver
participant apt-cache
participant apt-get
participant apt-mark
participant dpkg-query
Makefile->>Tests: test-dep-resolver
Tests->>Resolver: --no-color cx-demo
Resolver->>apt-cache: policy / depends / show / search
Resolver->>apt-get: -s install
Resolver->>apt-mark: showauto
Resolver->>dpkg-query: -W
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 new dependency conflict resolver script, scripts/cx-dep-resolver.sh, along with a test suite and a new Makefile target to run the tests. The review feedback highlights several robustness and security improvements for the shell scripts, including preventing potential crashes when parsing options, avoiding subshell overhead in recursive functions, handling regex characters in package names safely, improving error detection from APT simulation output, and using secure temporary directories for test logs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| --tree-depth) | ||
| tree_depth="${2:-}" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
If --tree-depth is passed as the last argument without a value, shift 2 will attempt to shift more arguments than are left. In Bash, this returns a non-zero exit status, which under set -e will cause the script to crash immediately. Adding a check to ensure the argument is present prevents this crash.
| --tree-depth) | |
| tree_depth="${2:-}" | |
| shift 2 | |
| ;; | |
| --tree-depth) | |
| if [[ $# -lt 2 ]]; then | |
| bad "Option --tree-depth requires an argument" | |
| exit 2 | |
| fi | |
| tree_depth="$2" | |
| shift 2 | |
| ;; |
| candidate_exists() { | ||
| local package="$1" | ||
| "$APT_CACHE_BIN" policy "$package" 2>/dev/null | awk '/Candidate:/ {print $2; found=1} END {exit !found}' | grep -vq "(none)" | ||
| } |
There was a problem hiding this comment.
The current implementation of candidate_exists uses a pipeline with grep -vq. With set -o pipefail enabled, if grep exits early or if the package does not exist, the pipeline can return a non-zero exit status or trigger SIGPIPE issues. We can simplify this and make it more robust by performing the entire check within awk.
| candidate_exists() { | |
| local package="$1" | |
| "$APT_CACHE_BIN" policy "$package" 2>/dev/null | awk '/Candidate:/ {print $2; found=1} END {exit !found}' | grep -vq "(none)" | |
| } | |
| candidate_exists() { | |
| local package="$1" | |
| "$APT_CACHE_BIN" policy "$package" 2>/dev/null | awk '/Candidate:/ { found = ($2 && $2 != "(none)"); exit } END { exit !found }' | |
| } |
| "$APT_CACHE_BIN" depends "$package" 2>/dev/null | | ||
| awk '/^[[:space:]]*(PreDepends|Depends):/ {print $2}' | | ||
| sed 's/[<>|]//g' | | ||
| awk 'NF && !seen[$0]++' | | ||
| while read -r dependency; do | ||
| print_dependency_tree "$dependency" "$((depth - 1))" "$indent " "${seen},${package}" | ||
| done | ||
| } |
There was a problem hiding this comment.
Running recursive function calls inside a pipeline causes each level of recursion to execute in a nested subshell. This is highly inefficient and can lead to process exhaustion or unexpected behavior with variables. Additionally, with pipefail and set -e active, any failure in the pipeline can prematurely terminate the script. Reading the dependencies into an array using process substitution avoids these issues.
| "$APT_CACHE_BIN" depends "$package" 2>/dev/null | | |
| awk '/^[[:space:]]*(PreDepends|Depends):/ {print $2}' | | |
| sed 's/[<>|]//g' | | |
| awk 'NF && !seen[$0]++' | | |
| while read -r dependency; do | |
| print_dependency_tree "$dependency" "$((depth - 1))" "$indent " "${seen},${package}" | |
| done | |
| } | |
| local dependencies=() | |
| while read -r dependency; do | |
| dependencies+=("$dependency") | |
| done < <("$APT_CACHE_BIN" depends "$package" 2>/dev/null | | |
| awk '/^[[:space:]]*(PreDepends|Depends):/ {print $2}' | | |
| sed 's/[<>|]//g' | | |
| awk 'NF && !seen[$0]++' || true) | |
| for dependency in "${dependencies[@]}"; do | |
| print_dependency_tree "$dependency" "$((depth - 1))" "$indent " "${seen},${package}" | |
| done |
| if [[ "$prefix" != "$package" && -n "$prefix" ]]; then | ||
| alternatives=$("$APT_CACHE_BIN" search "^${prefix}" 2>/dev/null | | ||
| awk '{print $1}' | | ||
| grep -v "^${package}$" | |
There was a problem hiding this comment.
Using grep -v "^${package}$" can fail or behave unexpectedly if the package name contains regex special characters (such as + in g++ or . in node.js). Using grep -Fvx performs a safe, literal, exact-line match instead.
| grep -v "^${package}$" | | |
| grep -Fvx "${package}" | |
| simulation="$("$APT_GET_BIN" -s install "${packages[@]}" 2>&1 || true)" | ||
| removals="$(grep -E '^(Remv|The following packages will be REMOVED:)' <<<"$simulation" || true)" | ||
| held="$(grep -Ei 'held|kept back|changed held' <<<"$simulation" || true)" | ||
| broken="$(grep -Ei 'broken packages|unmet dependencies|conflicts with|but it is not going to be installed' <<<"$simulation" || true)" |
There was a problem hiding this comment.
If apt-get fails with a fatal error (such as E: ...), the script might exit with a success status (0) if the error message does not match the current broken regex. Including ^E: in the regex ensures that any standard APT error is correctly caught and treated as a failure.
| broken="$(grep -Ei 'broken packages|unmet dependencies|conflicts with|but it is not going to be installed' <<<"$simulation" || true)" | |
| broken="$(grep -Ei 'broken packages|unmet dependencies|conflicts with|but it is not going to be installed|^E:' <<<"$simulation" || true)" |
| "$ROOT_DIR/scripts/cx-dep-resolver.sh" --no-color danger-package >/tmp/cx-dep-danger.log 2>&1; then | ||
| echo "Expected resolver to fail when apt simulation removes packages" >&2 | ||
| cat /tmp/cx-dep-danger.log >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if PATH="$TMP_DIR:$PATH" \ | ||
| APT_GET_BIN="$TMP_DIR/apt-get" \ | ||
| APT_CACHE_BIN="$TMP_DIR/apt-cache" \ | ||
| APT_MARK_BIN="$TMP_DIR/apt-mark" \ | ||
| DPKG_QUERY_BIN="$TMP_DIR/dpkg-query" \ | ||
| "$ROOT_DIR/scripts/cx-dep-resolver.sh" --no-color missing-package >/tmp/cx-dep-missing.log 2>&1; then | ||
| echo "Expected resolver to fail when no candidate exists" >&2 | ||
| cat /tmp/cx-dep-missing.log >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Writing temporary log files directly to /tmp is insecure and can cause permission conflicts or failures on shared multi-user systems or concurrent CI runners. Since a secure temporary directory TMP_DIR is already created and cleaned up on exit, we should write the test logs there instead.
if PATH="$TMP_DIR:$PATH" \
APT_GET_BIN="$TMP_DIR/apt-get" \
APT_CACHE_BIN="$TMP_DIR/apt-cache" \
APT_MARK_BIN="$TMP_DIR/apt-mark" \
DPKG_QUERY_BIN="$TMP_DIR/dpkg-query" \
"$ROOT_DIR/scripts/cx-dep-resolver.sh" --no-color danger-package >"$TMP_DIR/cx-dep-danger.log" 2>&1; then
echo "Expected resolver to fail when apt simulation removes packages" >&2
cat "$TMP_DIR/cx-dep-danger.log" >&2
exit 1
fi
if PATH="$TMP_DIR:$PATH" \
APT_GET_BIN="$TMP_DIR/apt-get" \
APT_CACHE_BIN="$TMP_DIR/apt-cache" \
APT_MARK_BIN="$TMP_DIR/apt-mark" \
DPKG_QUERY_BIN="$TMP_DIR/dpkg-query" \
"$ROOT_DIR/scripts/cx-dep-resolver.sh" --no-color missing-package >"$TMP_DIR/cx-dep-missing.log" 2>&1; then
echo "Expected resolver to fail when no candidate exists" >&2
cat "$TMP_DIR/cx-dep-missing.log" >&2
exit 1
fi![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
🤖 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 `@scripts/cx-dep-resolver.sh`:
- Around line 187-190: The `--tree-depth` option handling in
`cx-dep-resolver.sh` is shifting before confirming an operand exists, so the
script dies with a shell error when the flag is last. Update the `--tree-depth)`
case to validate that `$2` is present before `shift 2`, and route missing or
invalid values through the existing non-negative-integer validation/error path.
Keep the fix localized to the `--tree-depth` branch so the script emits the
intended message instead of aborting from `shift`.
- Around line 93-99: The dependency traversal in print_dependency_tree should
tolerate unresolved packages instead of exiting when apt-cache depends fails
with an unknown or virtual package. Update the pipeline around "$APT_CACHE_BIN"
depends "$package" so that non-zero status from the producer is handled or
ignored for missing-package cases, while preserving the recursive call over
discovered dependencies. Keep the fix localized to the print_dependency_tree
flow and its dependency discovery loop.
In `@tests/dependency-resolver-tests.sh`:
- Around line 107-122: The failure-path log redirections in the dependency
resolver test use predictable /tmp paths, which can be hijacked on shared
runners. Update the shell test around the cx-dep-resolver.sh invocations to
create the danger-package and missing-package log files under the existing
TMP_DIR or with mktemp, and redirect stdout/stderr to those unique paths instead
of hardcoded /tmp/cx-dep-danger.log and /tmp/cx-dep-missing.log.
🪄 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: 2f230b50-9bef-4d08-a33c-ecc7708b5d22
📒 Files selected for processing (3)
Makefilescripts/cx-dep-resolver.shtests/dependency-resolver-tests.sh
|
Follow-up after the automated review: I pushed commit 32e1064 to address the three actionable items. The resolver now validates a missing/invalid --tree-depth operand before shifting, dependency-tree traversal tolerates unresolved/virtual packages, and the failure-path test logs are written under the existing TMP_DIR instead of predictable /tmp paths. Validation: |
1 participant
Fixes #50.
Summary
make test-dep-resolvertarget and a mocked shell test harness.Follow-up hardening
--tree-depthoperands before shifting.TMP_DIRinstead of predictable/tmppaths.Validation
bash -n scripts/cx-dep-resolver.sh tests/dependency-resolver-tests.shbash tests/dependency-resolver-tests.shmake test-dep-resolver