feat(install.sh): uninstall + rc-file hygiene (#741)#63
Conversation
|
👋 Heads-up — Code review queue is at 17 / 8 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
cli#61 made the installer mutate the user's shell rc (appends a marked PATH block on the ~/.local/bin fallback). That created hygiene obligations the installer didn't meet — this closes them. - --uninstall: removes the tracebloc binary from the prefix (and the ~/.local/bin fallback) and strips ONLY our marked block from the rc, restoring it byte-identical. Portable editing (temp file + mv + awk; no sed -i, whose semantics differ GNU vs BSD/macOS). Idempotent. Scans all candidate rc files so it finds the block even if the user installed under a different shell. - Harden the append path: a symlinked / read-only / managed rc (chezmoi, dotfiles repo, Nix home-manager) is detected and the user is ADVISED with the exact line to add, instead of writing through the link or failing. $SHELL unset / exotic (csh, nu) now falls back to advice rather than guessing ~/.profile (which a non-login bash never reads). - Refactor the rc routing + path-line + append logic into shared, testable functions so install and uninstall can never drift on the marker string or target file. - Tests: scripts/tests/install.bats (new) — round-trip byte-identity, read-only/symlink advice, re-run idempotency, multi-rc uninstall. Verified locally: bash -n, sh -n, shellcheck -s sh (clean), bats 17/17. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8e1b8ee to
284439e
Compare
|
Rebased against current What changed during rebase:
Ready for review. |
|
Closing as stale/superseded (Lukas's call, board hygiene 2026-07-07): stacked on #61 which merged a month ago, predates the |
Closes #741.
Summary
#61 made the installer mutate the user's shell rc — it appends a marked PATH block (
# Added by the tracebloc CLI installer+ anexport PATH=…/fish_add_pathline) on the unprivileged~/.local/binfallback. That was the right fix, but mutating dotfiles creates obligations the installer didn't yet meet. This PR closes them: an uninstall path, and safety around managed/symlinked/read-only rc files.What changed (
scripts/install.sh)--uninstallflag — removes thetraceblocbinary from the prefix (and the~/.local/binfallback, so it cleans up regardless of which path install took) and strips only our marked block from the rc, restoring it byte-identical. Takes no network.mv+awk(the repo's existingsync-schema.shconvention). Nosed -i, whose-isemantics differ between GNU and BSD/macOS.\n+ marker + path line, so the block is effectively three lines. Stripping only the two visible lines orphans a blank line — the round-trip bats test caught this. The awk uses one-line lookbehind to drop a single preceding blank separator (only if it is blank — real content is kept)..zshrc,.bashrc,.bash_profile, fish config,.profile), so it finds the block even if the user installed under a different shell or has since switched shells.$SHELLunset / exotic (csh, tcsh, nu) now falls back to advice rather than guessing~/.profile(which a fresh non-login bash never reads).rc_for_shell), the PATH line (path_line_for_shell), the managed-rc probe (rc_is_managed), append (append_path_to_rc), and strip (strip_rc_block) are now shared, named functions, and the marker string is a singleRC_MARKERconstant. Install and uninstall cannot drift on the marker or the target file. Existing install idempotency (fix(install.sh): persist PATH to the shell rc (parity with install.ps1) #61's "skip if rc already references the dir") is preserved.Why
Per #741: mutating dotfiles without an uninstall leaves the line orphaned; and an append that writes through a symlink or a read-only/managed rc can fail silently or pollute version control. Both are addressed. The original report behind #61 came from a field deployment on Ubuntu 24.04.
Test plan
New
scripts/tests/install.bats(bats-core) — 17 tests, all green:--uninstallround-trips the rc byte-identical (with trailing newline, with a trailing blank line, and for an empty file). The one documented non-identical case — an rc whose last line lacks a trailing newline gains one (POSIX-correct, harmless) — is asserted explicitly.$SHELL→ generic advice, no wrong-file write;rc_for_shellreturns non-zero; known shells route correctly.--uninstallvia the real script — removes binary + block, clean no-op on a clean system, finds the block in a non-routed rc.The suite sources
install.shbehind aTRACEBLOC_INSTALL_SH_SOURCE_ONLYgate so the pure rc functions run without any download; the end-to-end tests execute the real script (uninstall takes no network).Verified locally / Needs CI
Verified locally:
bash -n scripts/install.sh— OKsh -n scripts/install.sh— OK (POSIX/bin/sh, no bashisms, matching the script's contract)shellcheck -s sh scripts/install.sh— clean (shellcheck 0.11.0)bats scripts/tests/install.bats— 17/17Needs CI:
build.ymlonly exercises the Go code — there is no shell-lint or bats step today, so neither #61's append nor this change is covered by CI. A small job (shellcheck+bats scripts/tests/) would guard the installer going forward. Not added here to avoid bundling CI infra into a behavior PR — happy to follow up if wanted.Notes
/bin/shthroughout (the script is piped viacurl … | shon distros that may lack bash).install.ps1(Windows) already persists PATH and is out of scope; onlyinstall.shmutates a Unix rc, so only it needs the uninstall/hygiene work.brew install bats-core/apt install bats).