feat(installer): add legacy migration and prevent recursive loops#12
Conversation
- Fix order of operations in install.bash to migrate AFTER asset copying - Ensure rsync --delete does not wipe user's custom modules during upgrade - Clean up .DS_Store artifacts
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 4 seconds. ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (1)
✨ 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 migrate_legacy function to move legacy configuration directories (.bashrc.d and .secrets.d) to the new $PREFIX and resolve recursive loop hazards in shell profiles. Key feedback includes the need for the migration function to respect the $DRY_RUN flag to prevent unintended filesystem modifications. Furthermore, it is recommended to replace the find and cp logic with rsync to safely preserve directory structures and ensure successful migration before deleting legacy data.
| fi | ||
|
|
||
| # @internal | ||
| migrate_legacy() { |
There was a problem hiding this comment.
The migrate_legacy function performs destructive operations (like rm -rf) and modifies the filesystem, but it does not respect the $DRY_RUN flag. This violates the expectation that --dry-run should not modify the system.
migrate_legacy() {
[[ "$DRY_RUN" -eq 1 ]] && return 0
| find "$legacy_rc_d" -type f -exec cp -p {} "$PREFIX/bashrc.d/" \; | ||
| rm -rf "$legacy_rc_d" |
There was a problem hiding this comment.
Using find -exec cp flattens the directory structure, which can lead to filename collisions and loss of organization if the legacy directory contains subdirectories. Additionally, the legacy directory is removed without explicitly verifying the success of the copy operation. Since rsync is already a dependency for this script, it is a more robust and efficient choice for migration.
rsync -a "$legacy_rc_d/" "$PREFIX/bashrc.d/" && rm -rf "$legacy_rc_d"
| find "$legacy_secrets_d" -type f -exec cp -p {} "$PREFIX/secrets.d/" \; | ||
| rm -rf "$legacy_secrets_d" |
There was a problem hiding this comment.
Similar to the .bashrc.d migration, using find -exec cp flattens the directory structure and the subsequent rm -rf is not explicitly guarded by the success of the copy operation.
rsync -a "$legacy_secrets_d/" "$PREFIX/secrets.d/" && rm -rf "$legacy_secrets_d"
There was a problem hiding this comment.
Pull request overview
Adds installer-side safeguards to improve upgrades from legacy layouts and reduce the chance of users ending up with broken shell startup behavior.
Changes:
- Introduces legacy migration from
~/.bashrc.dand~/.secrets.dinto the managed$PREFIXlayout. - Adds detection/recovery logic intended to prevent recursive sourcing loops from template-derived
~/.bashrc/~/.bash_profile. - Updates the installer’s shell-init wiring to source via deterministic (prefix-based) paths.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
install.bash |
Adds migrate_legacy migration + loop recovery logic; updates installation wiring/snippets. |
assets/.DS_Store |
Adds a macOS Finder metadata file (should not be tracked). |
Comments suppressed due to low confidence (1)
install.bash:567
- Legacy migration and loop-recovery are new behaviors but there’s no corresponding Bats coverage. Add tests that create
$HOME/.bashrc.dand$HOME/.secrets.dwith sample files and verify they end up under$PREFIX/...(and are removed from$HOME), and a test that simulates the loop-hazard dotfile and verifies it’s backed up and rewritten safely.
# @internal
migrate_legacy() {
local legacy_rc_d="$HOME/.bashrc.d"
local legacy_secrets_d="$HOME/.secrets.d"
if [[ -d "$legacy_rc_d" && ! -L "$legacy_rc_d" ]]; then
echo "Migrating legacy .bashrc.d to $PREFIX/bashrc.d..."
mkdir -p "$PREFIX/bashrc.d"
# Copy files, avoid error if empty
find "$legacy_rc_d" -type f -exec cp -p {} "$PREFIX/bashrc.d/" \;
rm -rf "$legacy_rc_d"
fi
if [[ -d "$legacy_secrets_d" && ! -L "$legacy_secrets_d" ]]; then
echo "Migrating legacy .secrets.d to $PREFIX/secrets.d..."
mkdir -p "$PREFIX/secrets.d"
chmod 700 "$PREFIX/secrets.d"
find "$legacy_secrets_d" -type f -exec cp -p {} "$PREFIX/secrets.d/" \;
rm -rf "$legacy_secrets_d"
fi
# Detect and fix "template as file" loop hazard
for f in "$HOME/.bashrc" "$HOME/.bash_profile"; do
if [[ -f "$f" && ! -L "$f" ]]; then
if grep -q "@file bashrc" "$f" || grep -q "@file bash_profile" "$f"; then
if [[ "$LINK_DOTFILES" -eq 0 ]]; then
echo "Detected recursive loop hazard in $f. Cleaning..."
backup_file "$f"
echo "# get-bashed: recovered from loop" > "$f"
fi
fi
fi
done
}
mkdir -p "$PREFIX"
export GET_BASHED_HOME="$PREFIX"
export GET_BASHED_VIMRC_MODE="$VIMRC_MODE"
copy_tree() {
local src="$1" dest="$2"
mkdir -p "$dest"
if [[ "${FORCE:-0}" -eq 1 ]]; then
rsync -a --delete "$src"/ "$dest"/
else
rsync -a "$src"/ "$dest"/
fi
}
# Copy base assets
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| migrate_legacy() { | ||
| local legacy_rc_d="$HOME/.bashrc.d" | ||
| local legacy_secrets_d="$HOME/.secrets.d" | ||
|
|
||
| if [[ -d "$legacy_rc_d" && ! -L "$legacy_rc_d" ]]; then | ||
| echo "Migrating legacy .bashrc.d to $PREFIX/bashrc.d..." | ||
| mkdir -p "$PREFIX/bashrc.d" | ||
| # Copy files, avoid error if empty | ||
| find "$legacy_rc_d" -type f -exec cp -p {} "$PREFIX/bashrc.d/" \; | ||
| rm -rf "$legacy_rc_d" | ||
| fi | ||
|
|
||
| if [[ -d "$legacy_secrets_d" && ! -L "$legacy_secrets_d" ]]; then | ||
| echo "Migrating legacy .secrets.d to $PREFIX/secrets.d..." | ||
| mkdir -p "$PREFIX/secrets.d" | ||
| chmod 700 "$PREFIX/secrets.d" | ||
| find "$legacy_secrets_d" -type f -exec cp -p {} "$PREFIX/secrets.d/" \; | ||
| rm -rf "$legacy_secrets_d" | ||
| fi |
There was a problem hiding this comment.
migrate_legacy performs filesystem writes (mkdir/cp/rm) even when --dry-run is set, which contradicts the earlier "Dry run enabled. No changes will be made." message. Add an early return when DRY_RUN=1 (or guard the call site) so legacy directories and dotfiles aren’t modified during dry runs.
| # Detect and fix "template as file" loop hazard | ||
| for f in "$HOME/.bashrc" "$HOME/.bash_profile"; do | ||
| if [[ -f "$f" && ! -L "$f" ]]; then | ||
| if grep -q "@file bashrc" "$f" || grep -q "@file bash_profile" "$f"; then | ||
| if [[ "$LINK_DOTFILES" -eq 0 ]]; then | ||
| echo "Detected recursive loop hazard in $f. Cleaning..." | ||
| backup_file "$f" | ||
| echo "# get-bashed: recovered from loop" > "$f" | ||
| fi |
There was a problem hiding this comment.
The recursive-loop recovery logic keys off the presence of @file bashrc / @file bash_profile anywhere in the user’s dotfiles, then backs up and replaces the entire file. This heuristic can trigger on legitimately customized files derived from the templates (or any file containing those strings), unexpectedly wiping active configuration. Consider detecting the actual recursive sourcing pattern (or comparing the file content to the shipped templates) and applying the minimal edit needed rather than clobbering the file.
| # shellcheck disable=SC2016 | ||
| BASHRC_LINE="# get-bashed: source modular bashrc" | ||
| # shellcheck disable=SC2016 | ||
| BASHRC_SNIP='if [[ -r "$HOME/.get-bashed/bashrc" ]]; then source "$HOME/.get-bashed/bashrc"; fi' | ||
| BASHRC_SNIP="if [[ -r \"$PREFIX/bashrc\" ]]; then source \"$PREFIX/bashrc\"; fi" | ||
| BASH_PROFILE_LINE="# get-bashed: source login bash_profile" | ||
| # shellcheck disable=SC2016 | ||
| BASH_PROFILE_SNIP='if [[ -r "$HOME/.get-bashed/bash_profile" ]]; then source "$HOME/.get-bashed/bash_profile"; fi' | ||
| BASH_PROFILE_SNIP="if [[ -r \"$PREFIX/bash_profile\" ]]; then source \"$PREFIX/bash_profile\"; fi" | ||
|
|
||
| ensure_block "$HOME/.bashrc" "$BASHRC_LINE" "$BASHRC_SNIP" | ||
| ensure_block "$HOME/.bash_profile" "$BASH_PROFILE_LINE" "$BASH_PROFILE_SNIP" |
There was a problem hiding this comment.
The .bashrc/.bash_profile snippet update is gated only on the marker line via ensure_block. If a user already has the marker from a previous install, changing the snippet (e.g., switching from $HOME/.get-bashed/... to an absolute $PREFIX/...) will not update the existing block, so the new deterministic/absolute-path behavior won’t take effect for upgrades or for users changing --prefix. Update ensure_block to replace the existing block when the marker exists but the snippet differs, or version the marker to force an update.
| cp -f "$REPO_DIR/vimrc" "$PREFIX/vimrc" | ||
| cp -f "$REPO_DIR/gitconfig" "$PREFIX/gitconfig" | ||
|
|
||
| migrate_legacy | ||
|
|
There was a problem hiding this comment.
The PR description mentions preserving custom user modules during upgrades when --force is used, but FORCE is parsed and never referenced anywhere in the installer (no behavior change occurs when passing --force). Either implement the described --force behavior (and document what it does) or remove the flag from usage/arg parsing to avoid misleading users.
~/.bashrc.dand~/.secrets.dfolders into the managed~/.get-bashedprefix.~/.bashrcor~/.bash_profileare copies of templates and recovers from infinite recursive sourcing loops.--forceis used.This solidifies the upgrade path and protects users from the 'broken terminal' state encountered during the PR 9 transition.