Conversation
worktrunk-bot
left a comment
There was a problem hiding this comment.
Thanks for adding install scripts — nice convenience for new users. A few issues to address:
worktrunk-bot
left a comment
There was a problem hiding this comment.
CI is failing with 2 test failures related to this PR:
test_readme_examples_are_in_sync— The README/docs content was updated but the auto-generated sections need re-syncing.test_command_pages_and_skill_files_are_in_sync—skills/worktrunk/reference/worktrunk.mdis out of sync with the docs.
To fix, run locally:
cargo test --test integration test_readme_examples_are_in_sync
cargo test --test integration test_command_pages_and_skill_files_are_in_syncThen commit the updated files.
This integrates the worktrunk-bot suggestions which I have reviewed. They are good additions. Co-authored-by: worktrunk-bot <w@worktrunk.dev>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The docs fixes look good — thanks for addressing those. One remaining issue from the code review suggestions being partially applied.
The review bot posted a suggestion on PR #1309 with start_line covering correct code (the git-wt elseif branch) along with the duplicate else block. Applying it would have removed a legitimate fallback path. Add explicit guidance to verify that every line in a multi-line suggestion range actually needs changing before posting. Closes #1310 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
thank you @snth ! great to have your name on the contributor list here too! (as I said on Twitter, come visit us in LA again). |
|
@worktrunk-bot can you stress test this approach — for example, when does Do a full scenario review of what users see depending on their setup. Does the TTY work well? If Separately: consider what it would take to have this work for |
|
Here's a thorough scenario analysis of the install script. Scenario matrix for
|
| # | Scenario | What happens | User experience |
|---|---|---|---|
| 1 | Happy path — macOS/Linux, default cargo location | Binary installed to ~/.cargo/bin/wt, first if branch runs, wt config shell install prompts via /dev/tty |
Smooth — user sees install output, then interactive shell config prompt |
| 2 | Custom CARGO_HOME (e.g. NixOS, custom setups) |
Binary goes to $CARGO_HOME/bin/wt, not $HOME/.cargo/bin/wt. First check fails. command -v wt also fails (new install, not on PATH yet). Falls through to warning. |
User sees success from upstream installer, then confusing warning saying wt not found. They have to figure out the path manually. |
| 3 | wt already on PATH (upgrade scenario) |
Old wt is on PATH. cargo-dist overwrites binary. $HOME/.cargo/bin/wt check succeeds (or command -v wt does). Shell install runs. |
Works, but wt config shell install will say "already configured" and skip — fine. |
| 4 | /dev/tty unavailable (Docker, CI, non-interactive) |
Binary installs fine. < /dev/tty fails with "No such device or address". wt config shell install errors out. |
User sees install success, then a cryptic error about /dev/tty. The shell config step fails, but the binary is installed. Not catastrophic but confusing. |
| 5 | wt conflicts with another binary (e.g. some systems have a wt for "wireless tools") |
command -v wt finds the wrong wt. But $HOME/.cargo/bin/wt check happens first and should succeed if cargo-dist used the default path. If not, command -v wt finds the wrong binary and runs wt config shell install which errors. |
Could run the wrong binary's command. Low probability since the $HOME/.cargo/bin check is first. |
| 6 | Upstream installer fails | curl ... | sh returns non-zero. The || { echo; exit 1; } catches it. Script exits. |
Clean — user sees "Installation failed." |
| 7 | No curl available | set -eu causes immediate exit on curl not found. |
User sees "curl: command not found" and the script exits. Clear enough, though a friendlier message could help. |
| 8 | Windows (Git Bash / WSL) | $OS is Windows_NT in Git Bash (set by Windows). WSL doesn't set it. Git Bash → redirected to PowerShell installer. WSL → installs normally (correct behavior). |
Works correctly for both cases. |
TTY / prompt behavior
The < /dev/tty redirect is the right approach for piped scripts. wt config shell install reads from stdin via io::stdin().read_line(), and when the script is piped (curl ... | sh), stdin is the pipe, not the terminal. The /dev/tty redirect restores terminal input for the prompt.
However, the prompt writes to stderr (io::stderr().flush()), which is good — stderr isn't captured by pipes, so the prompt text will always be visible.
One concern: if the upstream worktrunk-installer.sh (cargo-dist) also has interactive prompts, those would read from the pipe (stdin = the curl output) and get garbage. Looking at cargo-dist's installer behavior, it typically doesn't prompt interactively, so this should be fine in practice. But it's worth noting that only the last command in the script gets the /dev/tty redirect — the cargo-dist installer on line 14 runs with stdin still connected to the pipe.
Robustness improvements worth considering
-
CARGO_HOMEsupport — The biggest gap. Replace:if [ -x "$HOME/.cargo/bin/wt" ]; then "$HOME/.cargo/bin/wt" config shell install < /dev/tty
with:
_cargo_bin="${CARGO_HOME:-$HOME/.cargo}/bin" if [ -x "$_cargo_bin/wt" ]; then "$_cargo_bin/wt" config shell install < /dev/tty
-
Graceful
/dev/ttyfallback — For non-interactive environments:if [ -t 0 ] || [ -e /dev/tty ]; then "$_cargo_bin/wt" config shell install < /dev/tty else echo "" echo "Non-interactive environment detected." echo "Run 'wt config shell install' after restarting your shell." fi
-
Source the cargo env — cargo-dist's installer typically creates/updates
$HOME/.cargo/env. Sourcing it before thewtlookup would handle PATH updates:# shellcheck disable=SC1091 . "${CARGO_HOME:-$HOME/.cargo}/env" 2>/dev/null || true
Test suite ideas
Shell installer tests are inherently integration-heavy, but a reasonable test matrix could be:
# Test framework: simple shell scripts that assert on exit codes and output
# 1. Mock the upstream installer, verify wt binary is found
test_happy_path() # mock cargo-dist, place binary, assert shell install runs
test_custom_cargo_home() # CARGO_HOME=/tmp/cargo, assert binary found there
test_binary_not_found() # mock cargo-dist that doesn't place binary, assert warning
# 2. Environment detection
test_windows_detection() # OS=Windows_NT, assert redirects to PowerShell message
test_no_curl() # PATH without curl, assert error
# 3. TTY handling
test_piped_execution() # echo | sh install.sh, verify /dev/tty is used
test_no_tty() # Mock /dev/tty unavailable, verify graceful fallback
# 4. Idempotency
test_reinstall() # Run twice, second run should succeed cleanlyThese could live in a tests/ directory next to the install scripts, or be part of the integration test suite. The key challenge is mocking the upstream cargo-dist installer — you'd need to replace the curl URL with a local script that just places a mock binary.
https://worktrunk.dev | sh (bare domain as installer)
This pattern (e.g., curl https://get.volta.sh | sh or curl https://sh.rustup.rs | sh) works by having the web server return the install script as the default response for the root URL or a specific path.
What it would take:
-
Zola routing: The docs site would need to serve
install.shcontent at the root or a clean path. Since the docs use Zola, this is tricky — Zola serves HTML by default. Options:- A redirect rule at the CDN/hosting level: detect
curl/wgetuser-agents and serve the script instead of HTML. Netlify, Cloudflare Pages, and Vercel all support this via headers/edge functions. - A dedicated path like
https://worktrunk.dev/install.shalready works since it's indocs/static/. Thecurl -fsSL https://worktrunk.dev/install.sh | shcommand in the README is already the right pattern.
- A redirect rule at the CDN/hosting level: detect
-
User-agent detection (for bare
worktrunk.dev | sh): The server needs to distinguish browsers from curl. Common approach:If User-Agent contains "curl" or "wget" → serve install.sh as text/plain Otherwise → serve the HTML homepageThis is how
sh.rustup.rsworks. It requires a hosting platform that supports edge logic (Cloudflare Workers, Netlify Edge Functions, etc.). -
The current approach (
worktrunk.dev/install.sh) is already good — it's explicit, discoverable, and doesn't require server-side logic. The bare-domain pattern is slightly more memorable but adds infrastructure complexity. Given that the README already showscurl -fsSL https://worktrunk.dev/install.sh | sh, this seems like a nice-to-have rather than a blocker.
|
OK, can we start by:
|
…d tests - Move script installer below brew & winget in docs, label as experimental - Replace `curl | sh` pipe with temp file download (pipe swallows curl failures) - Source cargo env instead of hardcoding ~/.cargo/bin (supports custom CARGO_HOME) - Add graceful /dev/tty fallback for non-interactive environments - Add install.ps1 CARGO_HOME support and duplicate PATH prevention - Add test suite (25 tests) covering structure, Windows detection, download failures, PATH resolution, and non-interactive environments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Here's what I've done to address the feedback: 1. Moved install scripts below brew & winget, labeled as experimentalThe script installer is now in a collapsed 2. Removed hardcoded
|
|
All those
That doesn't work because immediately after install it won't be in the path. I add it to the path temporarily in order to allow the shell config script to run and then it prompts the user whether they want to install the shell integration. You could leave that whole section out as the installer already prints out what to run if you want shell integration. The suggested improvements in this comment are pretty good though and worth adding. |
worktrunk-bot
left a comment
There was a problem hiding this comment.
The install scripts are well-structured — temp file download in install.sh to catch failures, CARGO_HOME support, /dev/tty fallback, and Windows Terminal disambiguation in install.ps1 are all solid.
Two observations:
-
install_test.shisn't run in CI. It's a standalone script not referenced by any workflow orcargo test. These tests will drift as the install scripts evolve. Worth wiring into CI (even a simplesh docs/static/install_test.shstep in the test workflow), or calling from a Rust integration test. -
install.ps1still pipesirm | iexdirectly (line 11), whileinstall.shdownloads to a temp file specifically to catch download failures. In practiceirmthrows a terminating error on HTTP failures so this is lower-risk than the curl equivalent, but it's an asymmetry — the PS1 script doesn't get the same robustness improvement that motivated the temp-file approach ininstall.sh.
Tested on Ubuntu and Windows 11