feat: cherry-pick qbm-qbit + notifiarr-branch-builder from #37#38
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughTwo new Bash scripts are added to the repository. ChangesNotifiarr Branch Builder and Deployment
qBittorrent Manager Lock-Based Launcher
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@notifiarr-branch-builder.sh`:
- Line 88: The conditional uses an unquoted variable; update the check that
references apt_reinstall (which triggers reinstall_notifiarr) to quote the
variable and the literal for consistency and safety—i.e., change the condition
that tests apt_reinstall to compare the quoted variable to the quoted string
"true" and keep the call to reinstall_notifiarr unchanged.
- Line 4: The PATH export should quote the expansion to be robust against
spaces; update the export command from export PATH=$PATH:/usr/local/go/bin to
export PATH="$PATH:/usr/local/go/bin" so the PATH variable expansion is quoted
(affecting the export of PATH).
- Line 120: The call to "sudo systemctl stop notifiarr" lacks error handling;
update notifiarr-branch-builder.sh to check the exit status of that command (or
call "systemctl is-active notifiarr" first) and handle failures consistently
with the rest of the script: if stop returns non-zero and the service remains
active, log an error message and exit non‑zero, otherwise log success or that
the service was already stopped; reference the "sudo systemctl stop notifiarr"
invocation when adding the conditional check and logging.
- Around line 30-31: The prompt shows "[Y/n]" (Y default) but the check uses if
[[ "$response" =~ ^[Yy] ]]; then which ignores empty input; update the
conditional around the read prompt (the read -p ... response and the if test) to
treat an empty response as yes by checking for either empty response or a Y/y
match (e.g., test -z "$response" or "$response" =~ ^[Yy]) so pressing Enter
selects the default "Y" behavior.
- Around line 97-113: The branch prompt currently always runs and ignores any
provided --branch option, and validation uses substring matching which can pick
partial names; update the logic so that if a CLI variable (e.g., BRANCH or a
parsed --branch) is set use that value and skip the interactive prompt,
otherwise prompt as before; replace the loose substring check against branches
with an exact-match check (e.g., compare against the list elements or use
anchored/line-exact matching) before calling git -C "$repo_dir" checkout
"$branch" (and still call handle_error on failure). Ensure you reference the
existing variables current_branch, choice, branches and the git checkout
invocation when implementing these changes.
In `@qbm-qbit.sh`:
- Line 43: The sleep invocation uses unquoted variable expansion (sleep
$QBQBM_SLEEP_TIME); update it to quote the variable (sleep "$QBQBM_SLEEP_TIME")
to follow shell best-practices and avoid word-splitting or unexpected behavior
when QBQBM_SLEEP_TIME contains spaces or is empty.
- Line 47: The QBQBM_QBIT_OPTIONS variable is unquoted and relies on
word-splitting, so change its declaration to build an array (e.g., qbit_opts=())
and initialize that array from the env value while intentionally allowing
splitting (use a shellcheck disable comment), then update the invocation to use
the array expansion ("${qbit_opts[@]}") instead of unquoted $QBQBM_QBIT_OPTIONS;
update references in qbm-qbit.sh (the variable QBQBM_QBIT_OPTIONS and the
command line that calls qbit_manage.py) to use the new array name so option
values containing spaces are passed correctly.
- Around line 10-11: The script currently sources ".env" from the current
working directory which can be wrong when the script is run from elsewhere;
change the .env lookup to be relative to the script directory by resolving the
script path (e.g., using BASH_SOURCE or $0 and dirname) and then source the .env
file from that directory instead of plain ".env" so functions and variables load
reliably regardless of CWD.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc6a7bd1-0dc6-4074-94f3-32adbe2db734
📒 Files selected for processing (2)
notifiarr-branch-builder.shqbm-qbit.sh
| #!/bin/bash | ||
|
|
||
| # Extend the PATH to include the go binary directory | ||
| export PATH=$PATH:/usr/local/go/bin |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Quote PATH expansion for robustness.
While PATH rarely contains spaces, quoting variable expansions is a shell scripting best practice.
♻️ Proposed fix
-export PATH=$PATH:/usr/local/go/bin
+export PATH="$PATH:/usr/local/go/bin"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export PATH=$PATH:/usr/local/go/bin | |
| export PATH="$PATH:/usr/local/go/bin" |
🤖 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 `@notifiarr-branch-builder.sh` at line 4, The PATH export should quote the
expansion to be robust against spaces; update the export command from export
PATH=$PATH:/usr/local/go/bin to export PATH="$PATH:/usr/local/go/bin" so the
PATH variable expansion is quoted (affecting the export of PATH).
| read -p "$tool is not installed. Do you want to install it? [Y/n] " response | ||
| if [[ "$response" =~ ^[Yy] ]]; then |
There was a problem hiding this comment.
Fix default behavior to match prompt convention.
The prompt uses [Y/n] notation suggesting "Y" is the default (accepted on Enter), but the regex ^[Yy] only matches explicit Y/y input. An empty response (Enter key) won't match and will not install the tool.
🔧 Proposed fix to accept empty input as yes
- read -p "$tool is not installed. Do you want to install it? [Y/n] " response
- if [[ "$response" =~ ^[Yy] ]]; then
+ read -p "$tool is not installed. Do you want to install it? [Y/n] " response
+ if [[ -z "$response" || "$response" =~ ^[Yy] ]]; then
eval "$install_cmd" || handle_error "Failed to install $tool."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| read -p "$tool is not installed. Do you want to install it? [Y/n] " response | |
| if [[ "$response" =~ ^[Yy] ]]; then | |
| read -p "$tool is not installed. Do you want to install it? [Y/n] " response | |
| if [[ -z "$response" || "$response" =~ ^[Yy] ]]; then | |
| eval "$install_cmd" || handle_error "Failed to install $tool." |
🤖 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 `@notifiarr-branch-builder.sh` around lines 30 - 31, The prompt shows "[Y/n]"
(Y default) but the check uses if [[ "$response" =~ ^[Yy] ]]; then which ignores
empty input; update the conditional around the read prompt (the read -p ...
response and the if test) to treat an empty response as yes by checking for
either empty response or a Y/y match (e.g., test -z "$response" or "$response"
=~ ^[Yy]) so pressing Enter selects the default "Y" behavior.
| sudo apt update && sudo apt install --reinstall notifiarr || handle_error "Failed to reinstall Notifiarr using apt." | ||
| } | ||
|
|
||
| [[ $apt_reinstall == true ]] && reinstall_notifiarr |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Quote variable in conditional for consistency.
While [[ ]] handles unquoted variables safely, quoting is still best practice.
♻️ Proposed fix
-[[ $apt_reinstall == true ]] && reinstall_notifiarr
+[[ "$apt_reinstall" == true ]] && reinstall_notifiarr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[ $apt_reinstall == true ]] && reinstall_notifiarr | |
| [[ "$apt_reinstall" == true ]] && reinstall_notifiarr |
🤖 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 `@notifiarr-branch-builder.sh` at line 88, The conditional uses an unquoted
variable; update the check that references apt_reinstall (which triggers
reinstall_notifiarr) to quote the variable and the literal for consistency and
safety—i.e., change the condition that tests apt_reinstall to compare the quoted
variable to the quoted string "true" and keep the call to reinstall_notifiarr
unchanged.
| # Branch handling and updating | ||
| current_branch=$(git -C "$repo_dir" rev-parse --abbrev-ref HEAD) | ||
| read -p "Do you want to use the current branch ($current_branch)? [Y/n] " choice | ||
| if [[ "$choice" =~ ^[Nn] ]]; then | ||
| branches=$(git -C "$repo_dir" branch -r | sed 's/origin\///;s/* //') | ||
| echo "Available branches:" | ||
| echo "$branches" | ||
| while true; do | ||
| read -p "Enter the branch name you want to use: " branch | ||
| if [[ $branches =~ $branch ]]; then | ||
| git -C "$repo_dir" checkout "$branch" || handle_error "Failed to checkout branch $branch." | ||
| break | ||
| else | ||
| echo "Invalid choice. Please select a valid branch." | ||
| fi | ||
| done | ||
| fi |
There was a problem hiding this comment.
Fix branch handling logic and --branch parameter usage.
Two issues with the branch selection:
-
The
--branchCLI parameter is accepted but completely ignored. Lines 98-113 always prompt interactively, overwriting any value set via--branch. This breaks expected CLI behavior. -
Line 106 uses substring matching which could select the wrong branch. For example, if branches include both "master" and "master-dev", entering "master" would match either.
🔧 Proposed fix to honor --branch parameter and improve validation
# Branch handling and updating
current_branch=$(git -C "$repo_dir" rev-parse --abbrev-ref HEAD)
-read -p "Do you want to use the current branch ($current_branch)? [Y/n] " choice
-if [[ "$choice" =~ ^[Nn] ]]; then
+
+# Honor --branch parameter if provided and different from current
+if [[ "$branch" != "master" && "$branch" != "$current_branch" ]]; then
+ # --branch was explicitly set via CLI
+ git -C "$repo_dir" checkout "$branch" || handle_error "Failed to checkout branch $branch."
+else
+ # Interactive branch selection
+ read -p "Do you want to use the current branch ($current_branch)? [Y/n] " choice
+ if [[ "$choice" =~ ^[Nn] ]]; then
branches=$(git -C "$repo_dir" branch -r | sed 's/origin\///;s/* //')
echo "Available branches:"
echo "$branches"
while true; do
read -p "Enter the branch name you want to use: " branch
- if [[ $branches =~ $branch ]]; then
+ # Use grep for exact line match instead of substring
+ if echo "$branches" | grep -qxF "$branch"; then
git -C "$repo_dir" checkout "$branch" || handle_error "Failed to checkout branch $branch."
break
else
echo "Invalid choice. Please select a valid branch."
fi
done
+ fi
fi🤖 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 `@notifiarr-branch-builder.sh` around lines 97 - 113, The branch prompt
currently always runs and ignores any provided --branch option, and validation
uses substring matching which can pick partial names; update the logic so that
if a CLI variable (e.g., BRANCH or a parsed --branch) is set use that value and
skip the interactive prompt, otherwise prompt as before; replace the loose
substring check against branches with an exact-match check (e.g., compare
against the list elements or use anchored/line-exact matching) before calling
git -C "$repo_dir" checkout "$branch" (and still call handle_error on failure).
Ensure you reference the existing variables current_branch, choice, branches and
the git checkout invocation when implementing these changes.
|
|
||
| # Service management | ||
| echo "Stopping notifiarr..." | ||
| sudo systemctl stop notifiarr |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding error handling for service stop.
While systemctl stop typically succeeds even if the service is already stopped, adding error handling would be more consistent with the rest of the script's approach.
♻️ Proposed fix
echo "Stopping notifiarr..."
-sudo systemctl stop notifiarr
+sudo systemctl stop notifiarr || echo "Warning: Failed to stop notifiarr service" >&2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo systemctl stop notifiarr | |
| echo "Stopping notifiarr..." | |
| sudo systemctl stop notifiarr || echo "Warning: Failed to stop notifiarr service" >&2 |
🤖 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 `@notifiarr-branch-builder.sh` at line 120, The call to "sudo systemctl stop
notifiarr" lacks error handling; update notifiarr-branch-builder.sh to check the
exit status of that command (or call "systemctl is-active notifiarr" first) and
handle failures consistently with the rest of the script: if stop returns
non-zero and the service remains active, log an error message and exit non‑zero,
otherwise log success or that the service was already stopped; reference the
"sudo systemctl stop notifiarr" invocation when adding the conditional check and
logging.
| if [ -f ".env" ]; then | ||
| source ".env" |
There was a problem hiding this comment.
Use script directory for .env path, not current working directory.
The relative path .env depends on the current working directory, not the script's location. If the script is invoked from a different directory (e.g., via cron with bash /path/to/qbm-qbit.sh), it will fail to find the .env file or load an unintended one from the working directory.
🔧 Proposed fix to make .env path relative to script directory
+# Get the script's directory
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
# Load environment variables from .env file if it exists
-if [ -f ".env" ]; then
- source ".env"
+if [ -f "$SCRIPT_DIR/.env" ]; then
+ source "$SCRIPT_DIR/.env"
fi🤖 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 `@qbm-qbit.sh` around lines 10 - 11, The script currently sources ".env" from
the current working directory which can be wrong when the script is run from
elsewhere; change the .env lookup to be relative to the script directory by
resolving the script path (e.g., using BASH_SOURCE or $0 and dirname) and then
source the .env file from that directory instead of plain ".env" so functions
and variables load reliably regardless of CWD.
| echo "sleeping for $QBQBM_SLEEP_TIME" | ||
| # Pause the script to wait for any pending operations (i.e. Starr Imports) | ||
|
|
||
| sleep $QBQBM_SLEEP_TIME |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Quote variable expansion for consistency.
While numeric values don't require quoting for word-splitting safety, quoting all variable expansions is a shell scripting best practice.
♻️ Proposed fix
-sleep $QBQBM_SLEEP_TIME
+sleep "$QBQBM_SLEEP_TIME"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sleep $QBQBM_SLEEP_TIME | |
| sleep "$QBQBM_SLEEP_TIME" |
🤖 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 `@qbm-qbit.sh` at line 43, The sleep invocation uses unquoted variable
expansion (sleep $QBQBM_SLEEP_TIME); update it to quote the variable (sleep
"$QBQBM_SLEEP_TIME") to follow shell best-practices and avoid word-splitting or
unexpected behavior when QBQBM_SLEEP_TIME contains spaces or is empty.
|
|
||
| # Execute qbit_manage with configurable options | ||
| echo "Executing Command" | ||
| "$QBQBM_VENV_PATH"/bin/python "$QBQBM_PATH_QBM"/qbit_manage.py "$QBQBM_QBIT_OPTIONS" --config-file "$QBQBM_CONFIG_PATH" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider using an array for options to handle edge cases.
The unquoted $QBQBM_QBIT_OPTIONS relies on word splitting to pass multiple arguments, which works for the current space-separated flags but breaks if any option value contains spaces.
♻️ More robust approach using arrays
Replace the variable definition (line 19) and usage (line 47):
-QBQBM_QBIT_OPTIONS=${QBIT_MANAGE_OPTIONS:-"-cs -re -cu -tu -ru -sl -r"}
+# shellcheck disable=SC2206
+QBQBM_QBIT_OPTIONS=(${QBIT_MANAGE_OPTIONS:-"-cs -re -cu -tu -ru -sl -r"})-"$QBQBM_VENV_PATH"/bin/python "$QBQBM_PATH_QBM"/qbit_manage.py "$QBQBM_QBIT_OPTIONS" --config-file "$QBQBM_CONFIG_PATH"
+"$QBQBM_VENV_PATH"/bin/python "$QBQBM_PATH_QBM"/qbit_manage.py "${QBQBM_QBIT_OPTIONS[@]}" --config-file "$QBQBM_CONFIG_PATH"Note: The shellcheck disable is needed because we intentionally want word splitting when initializing the array from the environment variable.
🤖 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 `@qbm-qbit.sh` at line 47, The QBQBM_QBIT_OPTIONS variable is unquoted and
relies on word-splitting, so change its declaration to build an array (e.g.,
qbit_opts=()) and initialize that array from the env value while intentionally
allowing splitting (use a shellcheck disable comment), then update the
invocation to use the array expansion ("${qbit_opts[@]}") instead of unquoted
$QBQBM_QBIT_OPTIONS; update references in qbm-qbit.sh (the variable
QBQBM_QBIT_OPTIONS and the command line that calls qbit_manage.py) to use the
new array name so option values containing spaces are passed correctly.
## Summary - Restores Pre-commit CI to green. Repo's main has been failing since 2026-04-18 because pre-commit-config pinned shfmt v3.13.1-1, but the repo files were formatted by an older shfmt version with different rules. - Pure formatting changes — no semantic effects. ## Changes - 4-space → 2-space indent - pipe-at-EOL → `\` continuation with pipe-at-BOL (`--binary-next-line` flag) - minor whitespace normalisation 12 `.sh` files touched; `shellcheck --severity=warning` clean across all. ## Test plan - [ ] Pre-commit CI passes (it's been red for 3 weeks; this should fix it) - [ ] shfmt --diff is empty repo-wide after merge - [ ] No behavioral changes (formatting only) ## Notes This unblocks #38 (qbm-qbit + notifiarr-branch-builder cherry-pick), which inherits the broken Pre-commit run from main. Co-authored-by: Claude <noreply@anthropic.com>
Cherry-picked from xseed-improvements branch (PR #37) — keeping only the qbittorrent-manage + notifiarr-deploy helpers, not the xseed.sh work superseded by qui-xseed.sh on main. Inline fixes during cherry-pick: - qbm-qbit.sh: aligned QBQBM_* var names to consumer line (was defining QBQBM_VENV_PATH etc. but invoking unset $VENV_PATH etc.) - qbm-qbit.sh: removed unused QBQBM_LOCK_TIME - qbm-qbit.sh: fixed $LOCK reference in remove_lock (was unset) - notifiarr-branch-builder.sh: replaced unicode smart quotes (“/”, –) with ASCII equivalents so bash parses correctly Co-Authored-By: Claude <noreply@anthropic.com>
bakerboy448
force-pushed
the
feat-qbm-qbit-notifiarr-from-xseed-improvements
branch
from
9816b42 to
f102ded
Compare
1 participant
Summary
Test plan
Summary by CodeRabbit