Skip to content

fix(tests): give path-persist a real red/green signal (off-PATH prefix + interactive per-shell) (#310)#313

Merged
divyasinghds merged 1 commit into
developfrom
fix/client-310-path-persist-signal
Jul 7, 2026
Merged

fix(tests): give path-persist a real red/green signal (off-PATH prefix + interactive per-shell) (#310)#313
divyasinghds merged 1 commit into
developfrom
fix/client-310-path-persist-signal

Conversation

@saadqbal

@saadqbal saadqbal commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Completes #310 (the remaining 🟠 item from @aptracebloc's #214 review; the Critical guard() bug + CI-config gaps landed in #311). Reworks path-persist.sh so it can actually distinguish a fixed installer from a broken one.

The problem

As merged in #214 the guard passed on a broken installer too:

  • Ran as root → cli install.sh installed to /usr/local/bin (on every PATH). The installer only persists a PATH entry for an off-PATH prefix, so no rc was ever written and every shell resolved the binary trivially. No signal.
  • The login × non-login matrix mis-modelled "open a new terminal": a non-interactive bash -c reads no rc file, and a login bash reads ~/.profile — not the ~/.bashrc the installer writes on Linux.

The fix

  • Off-PATH prefix. Pin INSTALL_PREFIX=$HOME/.local/bin (a $HOME dir the installer always persists, off the default root PATH), and precondition-check it isn't already on PATH — else bail rather than assert nothing.
  • Per-shell install. install.sh persists to a single rc keyed off $SHELL, so install once per shell with SHELL=bash/zsh/fish, then assert a fresh interactive shell of that kind (bash -ic / zsh -ic / fish -ic) — the one that reads that rc. The meaningless login dimension is dropped.
  • Per-shell negative control (bash/zsh): a fresh non-interactive shell must not resolve the binary; if it does, the prefix leaked onto the base PATH and a pass would be meaningless. Skipped for fish (fish_add_path writes a universal variable and fish -c still sources config.fish; the global precondition covers fish).

Test plan

  • Validated the resolution mechanics locally with an isolated binary name (the dev host has a real tracebloc on PATH): a fresh non-interactive shell does not resolve it, while a fresh interactive shell does and tracebloc version runs — i.e. resolution is attributable to rc persistence.
  • bash -n, shellcheck --severity=error (the gate) and --severity=warning (advisory), actionlint — all clean.
  • CI-only: the full distro × shell matrix (real cli binary download + per-distro zsh/fish installs, interactive-shell behaviour without a tty across 9 base images) runs on GitHub — not claimed to pass locally.

Fixes #310.

🤖 Generated with Claude Code


Note

Low Risk
Changes are limited to test scripts and workflow documentation; no production installer or runtime behavior is modified.

Overview
Reworks the PATH persist CI guard so it can fail on broken CLI installers instead of vacuously passing.

path-persist.sh now pins INSTALL_PREFIX=$HOME/.local/bin and aborts if that prefix is already on PATH, so install.sh must write rc persistence for the binary to be found. It downloads install.sh once, then for each of bash/zsh/fish runs the installer with SHELL set to that shell and checks a fresh interactive shell (-ic) resolves tracebloc and runs tracebloc version. The old login vs non-login matrix is removed. bash/zsh get a negative control: a non-interactive shell must not see the binary (detects prefix leaking onto base PATH).

installer-tests.yaml comments are updated to describe the off-PATH prefix, per-shell install, and interactive assertion model (references client#310).

Reviewed by Cursor Bugbot for commit bf5090e. Bugbot is set up for automated code reviews on this repo. Configure here.

…ix + interactive per-shell (#310)

As merged in #214 the path-persist guard could not distinguish a fixed
installer from a broken one:

- It ran as root, so cli/install.sh landed the binary in /usr/local/bin
  (on every PATH). The installer only PERSISTS a PATH entry for an
  off-PATH prefix, so no rc was ever written and every shell resolved the
  binary trivially — green on a broken installer too.
- The login × non-login matrix mis-modelled 'open a new terminal': a
  non-interactive `bash -c` reads NO rc file, and a *login* bash reads
  ~/.profile, not the ~/.bashrc the installer writes on Linux.

Rewrite to actually exercise persistence:

- Pin INSTALL_PREFIX=$HOME/.local/bin (a $HOME dir the installer always
  persists, off the default root PATH). Precondition-check it's not
  already on PATH, else bail rather than assert nothing.
- install.sh persists to ONE rc keyed off $SHELL, so install once per
  shell with SHELL=bash/zsh/fish, then assert a fresh INTERACTIVE shell of
  that kind (bash -ic / zsh -ic / fish -ic) — the one that reads that rc.
- Per-shell negative control (bash/zsh): a fresh NON-interactive shell
  must NOT resolve the binary; if it does, the prefix leaked onto the base
  PATH and a pass would be meaningless. (Skipped for fish: fish_add_path
  writes a universal var and `fish -c` still sources config.fish.)

Verified locally: with an isolated binary name (the dev host has a real
tracebloc on PATH), a non-interactive shell does NOT resolve it while a
fresh interactive shell does + `version` runs. bash -n, shellcheck
(error + warning), actionlint all clean. The full distro × shell matrix
is CI-only (real cli binary download + per-distro shell installs).

Fixes #310. Follow-up to #214.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal saadqbal self-assigned this Jul 7, 2026
@divyasinghds divyasinghds merged commit 50c6b2f into develop Jul 7, 2026
35 checks passed
@divyasinghds divyasinghds deleted the fix/client-310-path-persist-signal branch July 7, 2026 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants