[feature/git_helper-13] Fixed Update Check and Self-update#16
Merged
justinkumpe merged 1 commit intodevfrom Dec 31, 2025
Merged
[feature/git_helper-13] Fixed Update Check and Self-update#16justinkumpe merged 1 commit intodevfrom
justinkumpe merged 1 commit intodevfrom
Conversation
Reviewer's GuideImplements robust, non-interrupting update checks and a self-update mechanism in git-helper.sh, hardens version parsing, makes UI behavior TTY-aware with fallbacks, and prevents auto-execution when the script is sourced. Sequence diagram for the git-helper update check and self-update flowsequenceDiagram
actor User
participant GitHelper as GitHelperScript
participant GitHub as GitHubRaw
participant UI as UIHelpers
User->>GitHelper: Run cmd_check_updates
activate GitHelper
GitHelper->>GitHub: fetch_latest_version
activate GitHub
GitHub-->>GitHelper: latest_version or failure
deactivate GitHub
alt fetch_failed
GitHelper->>UI: ui_message_no_exit(title, "Failed to check for updates...")
UI-->>GitHelper: return
GitHelper-->>User: return 1
else latest_version_empty
GitHelper->>UI: ui_message_no_exit(title, "Could not retrieve version information...")
UI-->>GitHelper: return
GitHelper-->>User: return 1
else version_ok
GitHelper->>GitHelper: compare_versions(SCRIPT_VERSION, latest_version)
alt versions_equal
GitHelper->>UI: ui_message_no_exit(title, "You are running the latest version...")
UI-->>GitHelper: return
GitHelper-->>User: return 0
else current_older
GitHelper->>UI: ui_message_no_exit(title, "A new version is available...")
UI-->>GitHelper: return
GitHelper->>UI: ui_yesno(title, "Download and replace this script now?")
UI-->>GitHelper: user_choice
alt user_confirms
GitHelper->>GitHelper: update_script_from_remote
alt update_success
GitHelper->>UI: ui_message(title, "Update complete. Please rerun the script.")
UI-->>GitHelper: return
GitHelper-->>User: exit 0
else update_failure
GitHelper->>UI: ui_message_no_exit(title, "Update failed. Please try manual update.")
UI-->>GitHelper: return
GitHelper-->>User: return 1
end
else user_declines
GitHelper-->>User: return 0
end
else current_newer
GitHelper->>UI: ui_message_no_exit(title, "You are running a development version newer than latest release.")
UI-->>GitHelper: return
GitHelper-->>User: return 0
end
end
deactivate GitHelper
Flow diagram for git-helper UI behavior and TTY-aware messagingflowchart TD
A_start[Start message display] --> B_checkTool{UI_TOOL}
subgraph TTY_check
direction LR
T1[Check stdin is TTY]
end
B_checkTool -->|whiptail| C_whip[Call whiptail --textbox]
B_checkTool -->|dialog| D_dialog[Call dialog --textbox]
B_checkTool -->|text| E_textFallback[Call fallback_to_text]
C_whip --> F_whipOK{Return status}
D_dialog --> G_dialogOK{Return status}
F_whipOK -->|success| H_done[Return 0]
F_whipOK -->|failure| E_textFallback
G_dialogOK -->|success| H_done
G_dialogOK -->|failure| E_textFallback
E_textFallback --> I_setText[Set UI_TOOL = text]
I_setText --> J_echoTitle[echo title and file contents]
J_echoTitle --> T1
T1 -->|is TTY| K_prompt[read Press Enter to return to menu]
T1 -->|not TTY| H_done
K_prompt --> H_done
subgraph Message_types
direction LR
M1[ui_message] --> M2[prompt_exit_after_display]
M3[ui_message_no_exit] --> M4[No exit prompt]
end
H_done --> L_route{Message type}
L_route -->|ui_message| M1
L_route -->|ui_message_no_exit| M3
subgraph Exit_prompt
direction LR
P1[Check stdin is TTY]
P1 -->|not TTY| P2[Return without prompting]
P1 -->|is TTY| P3[ui_yesno Exit helper now]
P3 -->|Yes| P4[exit 0]
P3 -->|No| P2
end
M2 --> P1
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
compare_versions, treating any malformed version string asv1 < v2(return 1) could incorrectly trigger update prompts; consider returning a distinct non-comparison error code and handling that separately in the update flow. - Both
ui_message_no_exitandupdate_script_from_remotecreate temporary files without a trap-based cleanup on failure paths; consider adding atrapto ensure temp files are always removed even if intermediate commands fail. update_script_from_remotehard-depends onwgetwhile the rest of the update logic usescurl; you might want to reusecurlor provide acurlfallback so updates still work on systems withoutwget.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `compare_versions`, treating any malformed version string as `v1 < v2` (return 1) could incorrectly trigger update prompts; consider returning a distinct non-comparison error code and handling that separately in the update flow.
- Both `ui_message_no_exit` and `update_script_from_remote` create temporary files without a trap-based cleanup on failure paths; consider adding a `trap` to ensure temp files are always removed even if intermediate commands fail.
- `update_script_from_remote` hard-depends on `wget` while the rest of the update logic uses `curl`; you might want to reuse `curl` or provide a `curl` fallback so updates still work on systems without `wget`.
## Individual Comments
### Comment 1
<location> `git_helpers/git-helper.sh:441-442` </location>
<code_context>
return 0
fi
+
+ # Validate versions are in correct format
+ if ! [[ "$v1" =~ ^[0-9]+(\.[0-9]+)*$ ]] || ! [[ "$v2" =~ ^[0-9]+(\.[0-9]+)*$ ]]; then
+ return 1
+ fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Version comparison now rejects non-pure-numeric versions, which may mis-handle pre-release or suffixed versions.
This validation only accepts plain numeric versions (e.g. `1.2.3`) and treats anything with suffixes (`1.2.3-beta`, `1.2.3+build1`) as "older" by returning `1`. If `SCRIPT_VERSION` or the remote version ever includes pre-release or build metadata, this will wrongly indicate an update is needed. Please either relax the regex to allow and strip suffixes before comparison, or treat invalid formats as a separate error path instead of mapping them to the normal "older" result.
</issue_to_address>
### Comment 2
<location> `git_helpers/git-helper.sh:460-469` </location>
<code_context>
}
+# Download latest script from main branch and replace current file
+update_script_from_remote() {
+ if ! command -v wget >/dev/null 2>&1; then
+ echo "wget is required to update automatically." >&2
+ return 1
+ fi
+
+ local dest
+ dest=$(script_self_path)
+ local tmp
+ tmp=$(mktemp "${TMPDIR:-/tmp}/git-helper.update.XXXXXX")
+
+ local raw_url="https://raw.githubusercontent.com/$GITHUB_REPO/main/git_helpers/git-helper.sh"
+
+ # Use wget quietly; allow manual cleanup on failure
+ if ! wget -q -O "$tmp" "$raw_url"; then
+ rm -f "$tmp"
+ return 1
+ fi
+
+ # Basic sanity check to avoid clobbering with an empty file
+ if [[ ! -s "$tmp" ]]; then
+ rm -f "$tmp"
+ echo "Downloaded file is empty; aborting update." >&2
+ return 1
+ fi
+
+ # Preserve executable bit
+ chmod +x "$tmp" || true
+
+ mv "$tmp" "$dest"
+ return 0
+}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Automatic self-update lacks integrity checks and a rollback/backup strategy, which could be risky if the download is compromised or corrupted.
The update routine fully trusts the HTTPS download and overwrites the existing script via `mv`, so any transient corruption or network compromise could permanently break or backdoor the script. Please add at least basic safeguards: e.g., create a backup of `"$dest"` before replacing it, and perform simple validation on the new file (check for an expected `SCRIPT_VERSION` line and key functions like `main()`), or integrate checksum/signature verification here if available.
Suggested implementation:
```
# Download latest script from main branch and replace current file
update_script_from_remote() {
if ! command -v wget >/dev/null 2>&1; then
echo "wget is required to update automatically." >&2
return 1
fi
local dest
dest=$(script_self_path)
# Ensure we have a current script to back up
if [[ ! -f "$dest" ]]; then
echo "Current script path '$dest' does not exist; aborting update." >&2
return 1
fi
# Create a backup of the current script
local backup
backup=$(mktemp "${TMPDIR:-/tmp}/git-helper.backup.XXXXXX") || {
echo "Failed to create backup; aborting update." >&2
return 1
}
if ! cp "$dest" "$backup"; then
echo "Failed to back up existing script to '$backup'; aborting update." >&2
return 1
fi
local tmp
tmp=$(mktemp "${TMPDIR:-/tmp}/git-helper.update.XXXXXX") || {
echo "Failed to create temporary file for update; aborting." >&2
return 1
}
local raw_url="https://raw.githubusercontent.com/$GITHUB_REPO/main/git_helpers/git-helper.sh"
# Use wget quietly; allow manual cleanup on failure
if ! wget -q -O "$tmp" "$raw_url"; then
echo "Failed to download updated script from '$raw_url'." >&2
rm -f "$tmp"
return 1
fi
# Basic sanity check to avoid clobbering with an empty file
if [[ ! -s "$tmp" ]]; then
rm -f "$tmp"
echo "Downloaded file is empty; aborting update." >&2
return 1
fi
# Lightweight integrity/validity checks:
# - Script declares SCRIPT_VERSION
# - Script contains a main entrypoint
if ! grep -q 'SCRIPT_VERSION=' "$tmp"; then
echo "Downloaded script missing SCRIPT_VERSION; aborting update." >&2
rm -f "$tmp"
return 1
fi
if ! grep -q 'main()' "$tmp"; then
echo "Downloaded script missing main() entrypoint; aborting update." >&2
rm -f "$tmp"
return 1
fi
# Preserve executable bit on the new script
chmod +x "$tmp" || true
# Replace current script with the new one; roll back on failure
if mv "$tmp" "$dest"; then
# Keep backup for manual rollback; do not delete it automatically
return 0
else
echo "Failed to install updated script; restoring backup." >&2
rm -f "$tmp"
cp "$backup" "$dest" || {
echo "Critical: failed to restore backup from '$backup'." >&2
return 1
}
return 1
fi
}
```
1. If the script's main entrypoint is not actually named `main()`, adjust the `grep -q 'main()'` check to look for the correct function name or another strong indicator of a valid script.
2. If you have an existing checksum or signature verification mechanism, you should integrate it into `update_script_from_remote` after the download and before replacing the file, in addition to (or instead of) the simple `SCRIPT_VERSION`/`main()` checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+441
to
+442
| # Validate versions are in correct format | ||
| if ! [[ "$v1" =~ ^[0-9]+(\.[0-9]+)*$ ]] || ! [[ "$v2" =~ ^[0-9]+(\.[0-9]+)*$ ]]; then |
There was a problem hiding this comment.
issue (bug_risk): Version comparison now rejects non-pure-numeric versions, which may mis-handle pre-release or suffixed versions.
This validation only accepts plain numeric versions (e.g. 1.2.3) and treats anything with suffixes (1.2.3-beta, 1.2.3+build1) as "older" by returning 1. If SCRIPT_VERSION or the remote version ever includes pre-release or build metadata, this will wrongly indicate an update is needed. Please either relax the regex to allow and strip suffixes before comparison, or treat invalid formats as a separate error path instead of mapping them to the normal "older" result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve the git helper script’s update-check and self-update behavior, especially in non-interactive environments.
Bug Fixes:
Enhancements: