Skip to content

fix: make install scripts usable immediately without terminal restart #433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Aureliolo merged 2 commits into from
Mar 15, 2026
Merged

fix: make install scripts usable immediately without terminal restart #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a9ed37
fix: make install scripts usable immediately without terminal restart
Aureliolo Mar 15, 2026
eaf1d74
fix: address 4 review findings from Gemini, CodeRabbit, and Greptile
Aureliolo Mar 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions cli/scripts/install.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,18 @@ try {
New-Item -ItemType Directory -Path $InstallDir -Force | Out-Null
Move-Item -Path (Join-Path $TmpDir $BinaryName) -Destination (Join-Path $InstallDir $BinaryName) -Force

# Add to PATH if not already there.
# Add to PATH if not already there (exact entry match, not substring).
$NormalizedInstallDir = $InstallDir.TrimEnd('\')
$UserPath = [Environment]::GetEnvironmentVariable("PATH", "User")
if ($UserPath -notlike "*$InstallDir*") {
[Environment]::SetEnvironmentVariable("PATH", "$UserPath;$InstallDir", "User")
Write-Host "Added $InstallDir to user PATH (restart your terminal to use 'synthorg' directly)."
$UserPathEntries = ($UserPath -split ';') | ForEach-Object { $_.TrimEnd('\') } | Where-Object { $_ }
if ($UserPathEntries -notcontains $NormalizedInstallDir) {
$NewUserPath = if ($UserPath) { "$UserPath;$InstallDir" } else { $InstallDir }
[Environment]::SetEnvironmentVariable("PATH", $NewUserPath, "User")
Write-Host "Added $InstallDir to user PATH."
}
$ProcessPathEntries = ($env:PATH -split ';') | ForEach-Object { $_.TrimEnd('\') } | Where-Object { $_ }
if ($ProcessPathEntries -notcontains $NormalizedInstallDir) {
$env:PATH = "$env:PATH;$InstallDir"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

& (Join-Path $InstallDir $BinaryName) version
Expand Down
15 changes: 15 additions & 0 deletions cli/scripts/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,19 @@ fi
echo ""
"${INSTALL_DIR}/${BINARY_NAME}" version
echo ""

# Warn if INSTALL_DIR is not in PATH (normalize trailing slash).
case ":${PATH}:" in
*":${INSTALL_DIR%/}:"*) ;;
Comment on lines +117 to +118

@greptile-apps greptile-apps Bot Mar 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PATH check only normalises INSTALL_DIR's trailing slash, not PATH entries

${INSTALL_DIR%/} strips a trailing slash from INSTALL_DIR before building the pattern, but individual entries in $PATH are not normalised. If the user's PATH contains the same directory with a trailing slash (e.g. /usr/local/bin/), the pattern *":/usr/local/bin:"* won't match :/usr/local/bin/:, and a spurious warning will be printed even though the binary is perfectly reachable.

A simple guard that covers this common case:

Suggested change
case ":${PATH}:" in
*":${INSTALL_DIR%/}:"*) ;;
case ":${PATH}:" in
*":${INSTALL_DIR%/}:"* | *":${INSTALL_DIR%/}/:"*) ;;

This matches both the normalised form and the same path with one trailing slash present in $PATH.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: cli/scripts/install.sh
Line: 117-118

Comment:
**PATH check only normalises INSTALL_DIR's trailing slash, not PATH entries**

`${INSTALL_DIR%/}` strips a trailing slash from `INSTALL_DIR` before building the pattern, but individual entries in `$PATH` are not normalised. If the user's `PATH` contains the same directory _with_ a trailing slash (e.g. `/usr/local/bin/`), the pattern `*":/usr/local/bin:"*` won't match `:/usr/local/bin/:`, and a spurious warning will be printed even though the binary is perfectly reachable.

A simple guard that covers this common case:
```suggestion
case ":${PATH}:" in
    *":${INSTALL_DIR%/}:"* | *":${INSTALL_DIR%/}/:"*) ;;
```
This matches both the normalised form and the same path with one trailing slash present in `$PATH`.

How can I resolve this? If you propose a fix, please make it concise.

*)
echo "Warning: ${INSTALL_DIR} is not in your PATH." >&2
echo "Add it by running:" >&2
echo "" >&2
echo " export PATH=\"${INSTALL_DIR}:\$PATH\"" >&2
echo "" >&2
echo "To make it permanent, add that line to your shell profile (~/.bashrc, ~/.bash_profile, ~/.profile, ~/.zshrc, etc.)." >&2
echo "" >&2
;;
esac
Comment on lines +116 to +128

@greptile-apps greptile-apps Bot Mar 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PATH check reflects the installer subprocess's PATH, not the user's interactive shell

When the script is run via curl … | bash, the spawned bash process inherits PATH from whatever invoked curl (e.g. a cron job, a CI runner, a minimal Docker container), not necessarily from the user's interactive login shell. This means:

  • A false positive is possible: the warning fires even though the directory is in the user's interactive ~/.profile-sourced PATH.
  • A false negative is possible: no warning fires because the parent process happened to have that directory in its PATH, even though the user's interactive shell does not.

This is an inherent limitation of piped installers and is hard to work around completely, but it may be worth adding a brief caveat to the warning message such as "(checked against the current process PATH)" so users aren't confused when which synthorg works in a new terminal despite the warning having fired.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: cli/scripts/install.sh
Line: 116-128

Comment:
**PATH check reflects the installer subprocess's PATH, not the user's interactive shell**

When the script is run via `curl … | bash`, the spawned bash process inherits `PATH` from whatever invoked `curl` (e.g. a cron job, a CI runner, a minimal Docker container), not necessarily from the user's interactive login shell. This means:

- A false positive is possible: the warning fires even though the directory _is_ in the user's interactive `~/.profile`-sourced `PATH`.
- A false negative is possible: no warning fires because the parent process happened to have that directory in its `PATH`, even though the user's interactive shell does not.

This is an inherent limitation of piped installers and is hard to work around completely, but it may be worth adding a brief caveat to the warning message such as `"(checked against the current process PATH)"` so users aren't confused when `which synthorg` works in a new terminal despite the warning having fired.

How can I resolve this? If you propose a fix, please make it concise.


echo "SynthOrg CLI installed successfully. Run 'synthorg init' to get started."
Loading