Post-install self-verification of CLI usability (#738)#215
Conversation
…correct PATH fix (#738)
Step 5 installed the tracebloc CLI and then told the user "open a new
terminal so it's on your PATH" — without ever proving a fresh terminal
would actually find it. That is exactly the cli#61 failure mode (binary
lands in ~/.local/bin, which a brand-new shell doesn't have on PATH),
left undetected until the customer hits it. The installer is the last
place to catch it.
After the install attempt, self-verify and report precisely:
- Probe `command -v tracebloc` in BOTH a fresh login shell ("$SHELL" -lic)
and a non-login shell ("$SHELL" -ic) — they read different startup files
(~/.profile vs ~/.bashrc), and cli#61 was "works in my login shell,
missing in a plain `bash` subshell".
- If found: confirm via `tracebloc version` and print a VERIFIED verdict.
The canonical `tracebloc dataset push ./data` next step stays in the
summary's "What to do next" — not duplicated here.
- If a fresh shell would NOT find it: print the EXACT shell-correct fix
for the user's actual $SHELL (zsh→~/.zshrc, bash+linux→~/.bashrc,
bash+darwin→~/.bash_profile, fish→fish_add_path + ~/.config/fish/config.fish,
else ~/.profile), not a generic "open a new terminal".
Stays NON-FATAL by design: the client is already connected by Step 5, so
the verification always returns 0 and is hardened against the orchestrator's
`set -e`. Mirrored in install-k8s.ps1 (RefreshPath is the faithful
"fresh terminal" probe on Windows, since the CLI installer edits the
user-scope registry PATH).
Tests: extend install-cli.bats (verified-command success, actionable
shell-correct PATH hint on miss, fish-specific fix, non-fatal under
`set -e`) and mirror in install-k8s.Tests.ps1 (Test-TraceblocCli:
verified verdict, actionable hint, non-fatal when RefreshPath throws).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
👋 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.) |
Two follow-ups so the Pester jobs go green (they were the only red checks on #215; Pester is green on develop, so this PR introduced both): - install-k8s.ps1: the new $TRACEBLOC_CLI_INSTALL_DIR ran Join-Path on $env:LOCALAPPDATA at top level. The Pester suite dot-sources this script, and on the Linux runner $env:LOCALAPPDATA is null — Join-Path throws on a null -Path, aborting BeforeAll and failing the whole container (0/65). Guard it; "" placeholder off Windows since the value is only used there. - install-k8s.Tests.ps1: add a `function tracebloc { }` stub so `Mock tracebloc` can bind. Pester v5 only mocks commands that already exist (cf. the existing kubectl/docker/helm/k3d stubs); without it the "fresh-shell success" test threw CommandNotFoundException — the lone windows-latest failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@LukasWodka — pushed a small follow-up ( 1. 2. Both Pester jobs are green now (+ Lint, bats, Unit tests, Static analysis). Marking ready for review. |
|
👋 Heads-up — Code review queue is at 13 / 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.) |
|
bugbot run |
Bugbot couldn't runBugbot is not enabled for your user on this team. Ask your team administrator to increase your team's hard limit for Bugbot seats or add you to the allowlist in the Cursor dashboard. |
|
bugbot run |
Bugbot couldn't runBugbot is not enabled for your user on this team. Ask your team administrator to increase your team's hard limit for Bugbot seats or add you to the allowlist in the Cursor dashboard. |
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Two non-blocking follow-ups from the review, both on scripts/lib/install-cli.sh — they bear on #738's core promise of correct, shell-specific guidance. CI is green and every failure mode here is non-fatal, so neither blocks merge.
Review follow-up on #215 (comment #2). The miss-branch hint printed a bare `export PATH=…` (fixes only the current shell) followed by `source ${rc}` on an rc that did not yet contain the line — so neither command persisted the fix, while the closing note implied ${rc} should hold it. The user is never told to write the line into the rc. Rewrite the guidance per-shell: - POSIX shells (zsh / bash / sh / dash): `echo '<export>' >> ${rc}` then `source ${rc}` — one copy-pasteable step that fixes THIS terminal and every new one. - fish: `fish_add_path "…"` already persists (a universal var) AND applies to the running shell, so drop the misleading `source ~/.config/fish/config.fish`. Tests (install-cli.bats): the zsh miss-path now asserts the `echo … >> ~/.zshrc` form; the fish case asserts no POSIX `export` and no `source`. bats 8/8 pass, shellcheck --severity=error gate clean, bash -n clean. NOTE: review comment #1 (fish fresh-shell probe using `command -v`, which fish's `command` builtin lacks) is NOT addressed here — it needs verification on a real fish and is tracked separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 9ec5a5d addressing review comment #2 (the PATH-fix guidance). The miss-branch hint now persists correctly, per shell:
✅ Update on review comment #1 — withdrawn after verification. I claimed the fresh-shell probe's |
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
LGTM. ✅
Reviewed the post-install CLI self-verification (#738):
- #2 — PATH-fix guidance (addressed in
9ec5a5d): POSIX shells nowecho '<export>' >> ~/.rc+source ~/.rc(persists for this terminal and new ones); fish uses its self-persistingfish_add_pathwith no bogussource.bats8/8,shellcheck --severity=errorgate clean,bash -nclean. - #1 — fish fresh-shell probe: verified against real fish 3.4.1 and 4.0.2 that
command -vresolves correctly and-licparses — non-issue, withdrawn.
Both review threads resolved, CI green. Design is sound: non-fatal after Step 5, two-shell (login + non-login) probe matching the cli#61 failure mode, and faithfully mirrored on Windows via RefreshPath. 👍
3 participants
Closes #738
Summary
After Step 5 installs the tracebloc CLI, the installer told the user "open a new terminal so it's on your PATH" without ever proving a fresh terminal would find it — the cli#61 failure mode (binary lands in
~/.local/bin, which a brand-new shell does not have on PATH), undetected until a customer hits it. This adds a post-install self-verification that probes a fresh shell fortraceblocand either prints a verified verdict or the exact shell-correct PATH fix for the user's actual$SHELL. Stays non-fatal (the client is already connected by Step 5). Mirrored on Windows.Related
Closes #738 · Part of #736 (install-journey epic)
Type of change
Test plan
Verified locally (macOS, bats 1.13.0):
bash -n scripts/lib/install-cli.sh— passesbats scripts/tests/install-cli.bats— 8/8 pass, including:tracebloc versionproof, asserts the old "open a new terminal" line is gone, asserts the dataset-push command is NOT duplicated here)export PATH="...local/bin:$PATH"line +source ~/.zshrcfor zsh)fish_add_path+~/.config/fish/config.fish, not a POSIXexport)set -ebats scripts/tests/summary.bats— 15/15 pass (unchanged; confirms next-steps stay consistent and the verified command is not duplicated)Pre-existing baseline failures (NOT introduced here):
validate_config,_extract_yaml_value, and two Docker-engine RHEL/Amazon-Linux tests fail identically on cleanorigin/develop(macOS env-specific); untouched by this PR.Needs CI:
scripts/tests/install-k8s.Tests.ps1) — pwsh is not installed on this machine, so the newTest-TraceblocCliDescribe block (verified verdict / actionable hint / non-fatal whenRefreshPaththrows) and the updatedInstall-TraceblocClicases were not run locally. Needs the Windows/Pester CI job to confirm.Deployment notes
None. Behavior change is limited to the install-time messaging of Step 5; no env vars, migrations, or rollout ordering.
Checklist
🤖 Generated with Claude Code