fix(bootstrap): re-exec after a pull rewrites the script - #326
fix(bootstrap): re-exec after a pull rewrites the script#326evansenter wants to merge 2 commits into
Conversation
`./bootstrap.sh -p` could abort silently partway through. pull_latest runs `git pull origin main`, which rewrites bootstrap.sh in place whenever an update touches it. Bash reads scripts lazily by byte offset, so the interpreter's next read lands at a shifted position in new content and the run derails — no error, no exit code. Observed 2026-08-02: a `-p` run stopped right after `brew bundle` and never reached sync_dotfiles, leaving ~/.macos unlinked. The tell was that the brew call is wrapped in `|| echo "...(continuing)"` and that warning never printed — the code still executing was not the code on disk. pull_latest now checksums the script around the pull and, on a change, re-execs itself so the rest of the run comes from a fresh interpreter reading the new file from byte zero. DOTFILES_BOOTSTRAP_REEXECED guards against a pull loop. The exec happens inside pull_latest, which bash has already parsed in full, so it gets out before the corrupted read. Adds BOOTSTRAP_SOURCE_ONLY so tests can source bootstrap.sh for its function definitions without running an install, plus three behavioral tests that exercise the real pull_latest against a sandboxed copy and a stubbed git. All three fail without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WfpypRTFbZ7eZTWLyJX9y
There was a problem hiding this comment.
Code Review
Summary
Solid fix for a nasty silent-failure mode: pull_latest now checksums bootstrap.sh around the pull and re-execs a fresh interpreter when the pull rewrote the script. The design is correct on the details that matter: the exec happens inside an already-fully-parsed function, before any further byte-offset read of the stale file; DOTFILES_BOOTSTRAP_REEXECED prevents a pull loop; argv is preserved with the bash-3.2-safe empty-array expansion idiom; and since pull_latest is the first operation in phase 1, the re-exec loses no completed work. The confirmation prompt sits after the pull, so a non-force run still prompts exactly once. The three tests exercise the real pull_latest via BOOTSTRAP_SOURCE_ONLY sourcing against a sandboxed copy with a stubbed git, cover both directions plus the loop guard, and the PR body confirms they fail on revert. CI (Lint/Test/Hooks/Bootstrap) is green.
Verdict
APPROVE - Only minor suggestions, posted as inline comments; no Critical or Important issues.
Automated review by Claude Code (inline comments posted separately because the review-with-comments JSON payload was blocked by the sandbox command filter)
| pull_latest() { | ||
| if [[ -n "${DOTFILES_BOOTSTRAP_REEXECED:-}" ]]; then | ||
| echo "Already pulled before restarting; skipping." | ||
| return 0 |
There was a problem hiding this comment.
[Suggestion] Consider adding 'unset DOTFILES_BOOTSTRAP_REEXECED' in this guard branch once it has served its purpose. As written, the variable stays exported for the entire re-exec run and leaks into every child process bootstrap spawns (brew, npm, launchctl, the .macos script, etc.). Harmless today, but unsetting it here keeps the environment clean and would protect any future nested bootstrap invocation from silently skipping its own pull.
|
|
||
| # Tests source this file to exercise individual functions. `return` is legal at | ||
| # the top level of a sourced script and stops here, before any install work. | ||
| [[ "${BOOTSTRAP_SOURCE_ONLY:-}" == "1" ]] && return 0 |
There was a problem hiding this comment.
[Suggestion] Edge case: if someone executes the script (rather than sourcing it) with BOOTSTRAP_SOURCE_ONLY=1 in their environment, the top-level 'return 0' is an error in bash (return is only valid from a function or sourced script), and under set -e the run aborts with status 1 and a confusing message. That failure mode is arguably fine — it refuses to install rather than doing something wrong — but if you want it graceful, wrap it in an if-block whose body is: return 0 2>/dev/null || exit 0
Addresses both review suggestions on #326. Unset DOTFILES_BOOTSTRAP_REEXECED once it has served its purpose. It is exported, so otherwise it rides along into every child bootstrap spawns (brew, npm, launchctl, .macos) and would make a nested bootstrap run silently skip its own pull. Scope the BOOTSTRAP_SOURCE_ONLY early-return to genuinely sourced runs by also testing BASH_SOURCE[0] != $0. `return` from an executed script is an error that set -e turns into an abort, and the variable is inherited by the re-exec'd process — so a stray value in the environment could have broken a normal install. The review suggested `return 0 2>/dev/null || exit 0`, but that makes an executed run exit 0 having installed nothing; ignoring the variable outright is the safer failure mode. Both paths are now covered by tests that fail without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WfpypRTFbZ7eZTWLyJX9y
There was a problem hiding this comment.
Code Review
Summary
Correct fix for a nasty silent-failure mode: pull_latest checksums bootstrap.sh around the pull and re-execs a fresh interpreter when the pull rewrote the script. The details all check out: the exec fires inside an already-fully-parsed function before any further byte-offset read of the stale file; pull_latest is the first call in phase 1, so a re-exec loses no completed work and the confirmation prompt (which comes after the pull) still fires exactly once; argv is preserved with the bash-3.2-safe empty-array idiom; BOOTSTRAP_SCRIPT is captured before the cd; and the loop guard is unset immediately so it cannot leak into children. Five behavioral tests exercise the real pull_latest via BOOTSTRAP_SOURCE_ONLY sourcing against a sandboxed copy with a stubbed git, and the BASH_SOURCE != $0 check correctly restricts the top-level return to genuinely-sourced runs. CI (Lint/Test/Hooks/Bootstrap) is green on the head commit.
Previously Addressed (Filtered)
Both suggestions from the prior review round are implemented at this commit, each with a dedicated test:
- bootstrap.sh — unset DOTFILES_BOOTSTRAP_REEXECED after the guard so children do not inherit it → implemented (line 486, covered by test_pull_latest_unsets_reexec_marker)
- bootstrap.sh — stray BOOTSTRAP_SOURCE_ONLY=1 in the environment would abort an executed run at the top-level return → implemented via the BASH_SOURCE[0] != $0 check (covered by test_source_only_ignored_when_executed)
Verdict
APPROVE - No Critical or Important issues found; prior-round suggestions are fully addressed with test coverage.
Automated review by Claude Code
Problem
./bootstrap.sh -pcan abort silently partway through.pull_latestrunsgit pull origin main, which rewritesbootstrap.shin place whenever an update touches it. Bash reads scripts lazily by byte offset, so the interpreter's next read lands at a shifted position in new content and the run derails — no error, no exit code, no clue in the output.Observed on 2026-08-02 while updating this machine: a
-prun stopped right afterbrew bundleand never reachedsync_dotfiles, leaving~/.macosunlinked. The tell was that the brew call is wrapped in|| echo "…(continuing)"and that warning never printed — the code still executing was not the code on disk.cbe9b2dhad touchedbootstrap.shin that same pull.This is silent and non-deterministic: it only bites when an update happens to change
bootstrap.sh, which is exactly the run where you least want a half-finished install.Fix
pull_latestchecksums the script around the pull. On a change, it re-execs, handing the rest of the run to a fresh interpreter reading the new file from byte zero.exechappens insidepull_latest, which bash has already parsed in full, so we get out before the corrupted read can occur.DOTFILES_BOOTSTRAP_REEXECEDguards the second pass so it doesn't pull again and loop.BOOTSTRAP_SCRIPT/BOOTSTRAP_ARGSare captured at the top, before thecd, so the re-exec targets the physical script (pwd -P, so a symlinked invocation still resolves) with the original argv intact.Testing
Adds
BOOTSTRAP_SOURCE_ONLY— one line before the main block.returnis legal at the top level of a sourced script, so tests get every function definition with zero install side effects, without wrapping the main block in anifor duplicating logic into a mock.Three behavioral tests exercise the real
pull_latestagainst a sandboxed copy ofbootstrap.shand a stubbedgit:All three fail when
bootstrap.shis reverted (verified by stashing just that file).make checkpasses: shellcheck clean, 112 hook tests, 42 bootstrap tests../bootstrap.sh -fstill runs clean (exit 0) with the source-only guard in place.🤖 Generated with Claude Code
https://claude.ai/code/session_012WfpypRTFbZ7eZTWLyJX9y