fix(install): own ONE PATH block in the shell profile, never append a second (#433) - #434
Conversation
… second (#433) `scripts/install.sh` guarded its profile append with "does the rc mention $PREFIX", so an install with a DIFFERENT prefix appended another block every time and the profile grew without bound. The v0.10.1 validation box collected TEN blocks, each naming a temp dir that no longer existed. The polluter was our own test harness: scripts/tests/install-verify.sh mints a fresh mktemp --prefix per case and never sandboxed $HOME, so it wrote 3 blocks into the developer's real ~/.bash_profile per invocation. - Tag our block with a stable $PATH_MARKER and REPLACE it on a re-run instead of stacking a second one. A profile already polluted by an older installer collapses to one block on the next install. - Decide "is $PREFIX already handled?" from the rc with our block stripped out, so the answer comes from the user's own lines only — that is what makes the write idempotent. - Compare whole path COMPONENTS, not substrings. `grep -F "$PREFIX"` matched --prefix /opt/tb against an existing /opt/tb2 line and then claimed "already in your PATH config" for a directory that was on nobody's PATH. - Append when there is nothing of ours to clean up; only rewrite the file when a stale block must go. The rewrite truncates in place, so the inode, mode and owner survive and an rc symlinked into a dotfiles repo is written THROUGH rather than replaced by a regular file. - Removal only takes the line under the marker when it is shaped like a PATH op we wrote, so a dangling marker can never eat unrelated user content. - Quote the fish line (`fish_add_path "$PREFIX"`), matching the client installer's hint, so a prefix containing a space survives. - Sandbox $HOME in install-verify.sh and add 7 assertions: same prefix x3 → one block; three different prefixes → one block naming the newest; prefix already on PATH → no rc written at all; unrelated lines preserved byte-for-byte; dangling marker harmless; zsh and fish route to the right rc. A prefix already on $PATH still writes nothing (unchanged), and a $HOME prefix is still persisted even when it looks on-PATH — that hit can be session-only (Bugbot #392 r2), and it now costs at most one line rather than one per run. Fixes #433 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
bugbot run |
|
👋 Heads-up — Code review queue is at 32 / 30 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.) |
The first pass consolidated correctly but reached the replace path even when our
one block already said exactly what this run would write — so a re-install, and
every `tracebloc upgrade` (which re-execs this installer), rewrote the user's
profile byte-for-byte and reported "Updated the tracebloc PATH entry".
Recognise that case and leave the file completely alone: count our blocks, and
when there is exactly one whose PATH line already matches, report `present`
("already in your PATH config — nothing to add") without touching the rc.
Asserted: re-install with the same prefix leaves the rc byte-identical.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up commit: a same-prefix re-run (and therefore every bugbot run |
The redirection in replace_rc truncates before cat writes, so a write that died halfway (no space left, a vanishing mount) would leave the user's profile in pieces. Put the original contents back on failure — best effort, but far better than a half-written file we don't own. The caller already reports `failed` and prints the line to add by hand. Verified with a read-only rc: the file keeps its user content and its previous block, and the manual-add advice is printed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
One more hardening commit: bugbot run |
…ing it (Bugbot) Bugbot on #434: the marker alone was treated as proof of ownership, so a marker left dangling by a hand-edit directly above the USER's own PATH export would take that export with it on the next install — silently deleting a PATH entry we never added, while still reporting success. Removal now demands positive evidence about the directory the following line names (_tb_owns_dir): * a '$' anywhere -> never ours. We always write a literal, expanded path; "$HOME/mytools" is the user's own idiom. This kills the most realistic form of the bug. * == the prefix we are installing to now -> ours * a directory that no longer exists -> ours (the #433 cruft) * a directory still holding a tracebloc binary -> ours (a prior install) Anything else is the user's line: we drop only the orphaned marker comment and leave their PATH op alone. The bias is deliberate — for an installer editing a file it does not own, failing to clean one line is much cheaper than deleting a PATH entry someone depends on. The residue is bounded: at most one unmarked line per pathological cycle, never renewed growth. The pair-then-verdict pass goes through a temp file rather than a pipe (so the verdicts land in this shell) and rather than a heredoc (so the awk program needs no nested-expansion escaping). 3 new assertions, 23/23 in install-verify.sh: a dangling marker above the user's own PATH export keeps it, likewise with an unexpanded $HOME, and a block naming a vanished directory is still cleaned up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot's two findings are answered in the review threads: finding 1 (dangling marker) was valid and is fixed in 5dae22b — removal now requires positive proof the directory is ours; finding 2 (empty prefix) is refuted — a bare
bugbot run |
Bugbot read the bare `exit` in the BEGIN rule as exiting 0, i.e. 'prefix already listed', which would skip persistence. It does not: a bare exit in BEGIN still runs END, so the status is END's exit(found ? 0 : 1) = 1, the fail-safe direction. Verified 1 on BSD awk and by the POSIX rule. Making it `exit 1` costs nothing and means the intent cannot be misread by a later reader or a stricter awk, rather than resting on the fallthrough.
|
On the remaining Low finding ( Why it was already safe: a bare What changed anyway: the BEGIN rule now says Verified after the change: Resolving the thread on that basis. |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1183eeb. Configure here.
…a failed tidy-up (Bugbot)
Two Medium findings, both in the ownership/reporting logic.
1. _tb_owns_dir claimed ANY non-directory as ours, so a dangling marker above the
user's own literal-path line for a directory they hadn't created yet was
stripped as an owned block — deleting a PATH entry we never wrote, which is not
recoverable.
The marker we write now records the directory:
# Added by the tracebloc CLI installer (prefix: /opt/tracebloc)
export PATH="/opt/tracebloc:$PATH"
When the recorded prefix and the directory on the line below agree, we provably
wrote both halves, so the block is reclaimable even if that directory has since
been deleted — which is what keeps the #433 cleanup working, ten blocks and all.
$PATH_MARKER stays the stable BEGINNING of the line (matched literally via
index(), never as a whole line) so older blocks are still recognised.
A legacy marker that recorded nothing can now only be claimed when the
directory still holds a tracebloc binary, or is the prefix being installed to.
A legacy marker over a vanished directory is indistinguishable from the user's
own entry for a directory they plan to create, so we leave it alone. That gives
up auto-healing pre-#433 cruft in exactly one case; Lukas's profile was already
cleaned by hand, so that is a nice-to-have, whereas deleting someone's PATH line
is not.
Blocks we cannot claim are now left ENTIRELY intact, comment included, rather
than losing their marker: a dead line still labelled "Added by the tracebloc CLI
installer" tells the user what it is, a bare one doesn't.
2. A failed replace_rc reported `failed` even when the user's own line already
persisted the prefix and all we'd failed to do was drop a redundant block — so
the installer told them to hand-edit their profile while their PATH was in fact
correct. Split into `tidied`/`tidy_failed` (PATH is right; at most a cosmetic
note) versus `failed` (prefix genuinely not persisted; manual instruction
warranted), and the write decision now hinges on content equality (rc_same), so
a run that changes nothing writes nothing.
rc_same deliberately avoids cmp(1) — it lives in diffutils, which a minimal
container image can lack, and a missing tool must not silently turn "leave the
file alone" into "rewrite it every run".
Also restores the `mkdir -p "$(dirname "$rc")"` that the state-machine rewrite
dropped; without it fish's ~/.config/fish/config.fish could never be created. The
matrix caught it.
install-verify.sh: 29/29, with six new cases — legacy marker can't claim a missing
dir, nor one without our binary; a recorded-prefix block naming a vanished dir is
still cleaned; ten of them collapse to one; an unclaimable legacy block is kept
without churning the rc; a failed tidy-up never asks for a manual PATH line, while
a real failure still does.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both new Medium findings were valid and are fixed in a836089; details in the two review threads.
Also caught by my own matrix while reworking the state machine: the rewrite had dropped
bugbot run |
* fix(install): match a quoted fish path, spaces included rc_lists_dir field-split fish_add_path arguments on whitespace. Since #434 started writing fish_add_path "$PREFIX", a prefix containing spaces became two fields, matched neither, and the installer appended a SECOND block directly beneath an existing line naming the same directory -- while reporting that it had added a PATH entry that was already there. The read path now parses quotes like the ownership reader further up the file already does: a quoted argument is one path, spaces included, while an unquoted line still splits so several bare paths on one line keep working. Single quotes are handled too, which the old code also missed. Verified in both directions: the two new harness cases fail against the current installer (29 passed / 2 failed) and pass with this change (31 passed / 0 failed). Found by Bugbot on the develop->staging promotion (cli#438). * fix(install): take the FIRST fish_add_path, not the last A greedy /^.*fish_add_path/ strips through the LAST occurrence on the line, so an inline comment mentioning fish_add_path left the comment text as the path and missed the real argument -- reintroducing the exact bug this branch fixes. index() takes the first occurrence instead. Also restores the leading-whitespace strip that the greedy regex used to consume: without it substr() left ' "/path"', so the quote check saw a space and fell back to field-splitting, breaking the spaced-path fix. Caught by re-running the whole case set rather than only the new one. Bugbot, cli#439. * fix(install): take the FIRST fish_add_path, not the last A greedy /^.*fish_add_path/ strips through the LAST occurrence on the line, so an inline comment mentioning fish_add_path left the comment text as the path and missed the real argument -- reintroducing the exact bug this branch fixes. index() takes the first occurrence instead. Also restores the leading-whitespace strip that the greedy regex used to consume: without it substr() left ' "/path"', so the quote check saw a space and fell back to field-splitting, breaking the spaced-path fix. Caught by re-running the whole case set rather than only the new one. Bugbot, cli#439. * fix(install): tokenise the fish argument list properly Replaces three rounds of patching with one quote-aware tokenizer, because each patch fixed its own case and broke or missed another: * greedy .* strip lost the real argument to an inline comment * index() alone dropped the leading space, so the quote check failed and spaced paths regressed * taking only the first quoted argument missed the prefix when fish_add_path lists several directories The loop now walks the argument list: quoted tokens are one path (spaces included), bare tokens split on whitespace, flags are skipped, and a trailing # comment ends parsing. 18 direct variants and 33 harness cases green. Bugbot, cli#439. * fix(install): tokenise the fish argument list properly Replaces three rounds of patching with one quote-aware tokenizer, because each patch fixed its own case and broke or missed another: * greedy .* strip lost the real argument to an inline comment * index() alone dropped the leading space, so the quote check failed and spaced paths regressed * taking only the first quoted argument missed the prefix when fish_add_path lists several directories The loop now walks the argument list: quoted tokens are one path (spaces included), bare tokens split on whitespace, flags are skipped, and a trailing # comment ends parsing. 18 direct variants and 33 harness cases green. Bugbot, cli#439.

Summary
scripts/install.shappended a PATH block to the shell profile on runs it should have skipped, so profiles accumulated dead entries. The installer now owns exactly one PATH block per rc and replaces it on a re-run.Fixes #433
Root cause
Two things, and the second explains the "ten blocks" report.
1. The dedupe was per-prefix. The append was guarded by "does the rc mention
$PREFIX" — so a re-run with the same prefix was already idempotent (that landed in #65), but a run with a different prefix matched nothing and appended another block. Every install with a fresh--prefixadded a line, forever.2. Our own test harness was the polluter.
scripts/tests/install-verify.shmints a freshmktemp--prefixper case and never sandboxed$HOME, so it wrote into the developer's real~/.bash_profile. Measured against the pre-fix code:3 per run — run it three or four times while validating a release and you have ten blocks, each naming a temp dir that has since been cleaned up. That matches the reported machine exactly.
Repro (pre-fix)
The issue's repro re-evaluates
$(mktemp -d)each time, which is why it grows — a different prefix per run:What changed
scripts/install.sh$PATH_MARKERand replace it on a re-run instead of stacking a second one. Because the marker text is unchanged from previous releases, a profile already polluted by an older installer collapses to one block on the next install.$PREFIXalready handled?" from the rc with our block stripped out, so the answer comes from the user's own lines only. That is what makes the write idempotent rather than prefix-dependent.grep -F "$PREFIX"matched--prefix /opt/tbagainst an existing/opt/tb2line and then printed "already in your PATH config" for a directory that was on nobody's PATH — a silent "installed but not found in a new terminal".mv-ing a temp over the path, so inode/mode/owner survive and an rc symlinked into a dotfiles repo is written THROUGH instead of being replaced by a regular file._tb_owns_dirdemands evidence about the directory named on the following line — a$anywhere means never ours (we write literal, expanded paths;$HOME/mytoolsis the user's idiom), otherwise it must be the current$PREFIX, a directory that no longer exists (the Installer appends a PATH line to the shell profile on every run (no dedupe) — profiles accumulate dead entries #433 cruft), or one still holding atraceblocbinary. Anything else keeps the user's line and drops only the orphaned marker. The bias is deliberate: failing to clean one line is far cheaper than deleting a PATH entry someone depends on.fish_add_path "$PREFIX"), matching the client installer's hint, so a prefix containing a space survives.replacedstate so the message says "Updated the tracebloc PATH entry" rather than over-claiming "Added".tracebloc upgradere-execs this installer, so the same prefix comes back around on every upgrade; when our one block already says exactly what we'd write, the rc is left byte-identical and we report "already in your PATH config — nothing to add".scripts/tests/install-verify.sh$HOMEper case ($SBX/home) — the harness can no longer touch real dotfiles.SHELLdefaults to/bin/shso the target rc is~/.profileon both Linux and macOS and assertions don't branch per platform.Deliberately unchanged
A
$HOMEprefix is still persisted even when it already looks on-$PATH. That hit can be session-only (a one-offexport, direnv) which a new terminal won't have — the reviewed behaviour from Bugbot #392 r2. It now costs at most one line instead of one per run. A non-$HOMEprefix already on$PATH(the/usr/local/bindefault) writes nothing, as before — verified below.Test plan
Every gate this repo's
Installer (shell)job runs, locally:bash scripts/tests/install-verify.sh— 29 passed, 0 failed (12 pre-existing cosign/SHA/traversal cases still green, 17 new):Real end-to-end (actual download + SHA256 + cosign bootstrap and signature verify), throwaway
HOME, 3 runs with 3 different prefixes:Real end-to-end with the prefix already on
$PATH: install succeeds (checksum matches,cosign signature valid) and no shell profile is created at all — the only thing in the fakeHOMEis cosign's own.sigstore/TUF cache.Migration and safety, driven under
dash:/opt/tb2then/opt/tb(substring trap)/opt/tb, no false "already configured"a+b.c)sh, 3 differing prefixes eachReal
~/.bash_profileand~/.bashrcSHA-256 captured before and after the entire session — identical, never touched.CI on this PR is green, including the cross-repo Fresh-shell PATH guard on
ubuntu:24.04,debian:12,fedora:latestandopensuse/leap:15.6(added thee2elabel so it actually runs — it guards exactly the PATH-persistence class this PR touches).Not in scope
scripts/install.ps1already dedupes correctly (if ($existingEntries -notcontains $InstallPrefix)plus a whole-valueSetEnvironmentVariable), so Windows has no equivalent accumulation bug and is untouched.Bugbot
Four findings across two rounds, all four threads resolved. Final re-run: "no issues found."
Round 1
exitin awk'sBEGINstill runsEND, so the status isexit(found ? 0 : 1)= 1 ("not listed"), the fail-safe direction. Measured at 1 on BSD awk, GNU Awk 5.3.2, mawk and busybox awk.1183eebthen made itexit 1outright so no reader has to know that rule.Round 2 — both valid, both fixed in a836089:
Missing dirs treated as owned (Medium).
_tb_owns_dirclaimed any non-directory, so a user's literal path for a directory they hadn't created yet, sitting under a dangling marker, was deleted. The$-rule protected$HOME/binbut not that. The marker now records its prefix —# Added by the tracebloc CLI installer (prefix: <dir>)— so ownership is proven: when the recorded prefix and the directory on the line below agree, we wrote both halves, and the block stays reclaimable even once that directory is gone (which is what keeps the Installer appends a PATH line to the shell profile on every run (no dedupe) — profiles accumulate dead entries #433 collapse working).$PATH_MARKERremains the stable beginning of the line, matched literally viaindex(), so older blocks are still recognised. A legacy marker (recorded nothing) can now only be claimed when the directory still holds atraceblocbinary or is the prefix being installed to.Trade-off, stated explicitly: this gives up auto-healing pre-Installer appends a PATH line to the shell profile on every run (no dedupe) — profiles accumulate dead entries #433 cruft in exactly one case — a legacy block whose directory has vanished, which is indistinguishable from a user's entry for a directory they plan to create. Everything the fixed installer writes carries its prefix and stays fully self-healing, so the ten-block shape cannot recur. Lukas's profile was already cleaned by hand, so that auto-heal is a nice-to-have; deleting someone's PATH line is not recoverable. Safe side taken.
Related: a block we can't claim is now left entirely intact, comment included, rather than losing its marker — a dead line still labelled as ours tells the user what it is; a bare
export PATH=line just looks like their own.Redundant-block cleanup failure misreports (Medium). A failed
replace_rcreportedfailedeven when the user's own line already persisted the prefix, so the installer asked them to hand-edit a profile whose PATH was already correct. Split intotidied/tidy_failed(PATH is right — cosmetic note only, no manual instruction) versusfailed(prefix genuinely not persisted — instruction warranted). The write decision now hinges on content equality (rc_same), so a run that changes nothing writes nothing.rc_sameavoidscmp(1)deliberately: it ships in diffutils, which a minimal container image can lack, and a missing tool must not silently turn "leave the file alone" into "rewrite it every run".Found by my own matrix while reworking the state machine: the rewrite had dropped
mkdir -p "$(dirname "$rc")", so fish's~/.config/fish/config.fishcould never be created. Restored and covered by a test.Checklist
sh, no bashisms (dash -n+shellcheck --shell=shclean)scripts/developNote
Medium Risk
Changes how real dotfiles are edited on every install/upgrade; mistakes could drop user PATH lines, though ownership checks and extensive tests target that failure mode.
Overview
Fixes #433: repeated installs (especially with different
--prefixvalues) no longer stack multiple tracebloc PATH blocks in the shell rc.install.shreplaces append-only dedupe with a single owned block tagged by$PATH_MARKER, optionally with(prefix: …). Re-runs strip provably ours blocks via_tb_owns_dir/strip_tb_path_block, then append or in-placereplace_rc(symlink-safe).rc_lists_diruses whole path components instead ofgrep -F "$PREFIX". New outcomes includereplaced,tidied, andtidy_failedso messaging does not ask for manual PATH edits when the rc is already correct. Fish uses quotedfish_add_path "$PREFIX".install-verify.shsandboxes$HOMEper case and adds broad tests for idempotency, prefix churn, user-line preservation, dangling markers, legacy vs recorded-prefix cleanup, read-only rc behavior, and zsh/fish rc targets.Reviewed by Cursor Bugbot for commit a836089. Bugbot is set up for automated code reviews on this repo. Configure here.