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 1 commit
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
5 changes: 4 additions & 1 deletion cli/scripts/install.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ try {
$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)."
Write-Host "Added $InstallDir to user PATH."
}
if ($env:PATH -notlike "*$InstallDir*") {
$env:PATH = "$env:PATH;$InstallDir"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

@gemini-code-assist gemini-code-assist Bot Mar 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The -notlike check for the directory's existence in PATH is not fully robust. It can return a false positive if $InstallDir is a substring of another directory in PATH. For instance, if PATH includes C:\Program Files\synthorg-tools and $InstallDir is C:\Program Files\synthorg, the check would incorrectly assume the path is already present. A more reliable method is to split the PATH string by its separator and check for an exact match in the resulting array of paths. Note that the same potential issue exists on line 82 for the $UserPath check.

    if (($env:PATH -split ';') -notcontains $InstallDir) {
        $env:PATH = "$env:PATH;$InstallDir"
    }


& (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.
case ":${PATH}:" in
*":${INSTALL_DIR}:"*) ;;

@gemini-code-assist gemini-code-assist Bot Mar 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The check for INSTALL_DIR in PATH might fail if INSTALL_DIR has a trailing slash (e.g., INSTALL_DIR=/tmp/foo/) but PATH contains the path without it (e.g., /tmp/foo). It's good practice to normalize the path by removing any trailing slash before the check. You can do this directly within the case statement pattern using shell parameter expansion.

Suggested change
*":${INSTALL_DIR}:"*) ;;
*":${INSTALL_DIR%/}:"*) ;;

*)
echo "Warning: ${INSTALL_DIR} is not in your PATH."
echo "Add it by running:"
echo ""
echo " export PATH=\"${INSTALL_DIR}:\$PATH\""
echo ""
echo "To make it permanent, add that line to your shell profile (~/.bashrc, ~/.zshrc, etc.)."
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated
echo ""
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated
;;
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