Conversation
Greptile SummaryRefactors update instruction logic to detect the actual install method (npm, brew, scoop, winget, apt, etc.) by inspecting the executable path at runtime, fixing the issue where users would see incorrect update commands. Adds comprehensive unit and integration tests across all platforms. Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: fd824d3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This would be great to have, it's a bit confusing for users to see instructions that they can't run every time they use infisical. |
|
@claude review this once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| func GetUpdateInstructions() string { | ||
| os := runtime.GOOS | ||
| switch os { | ||
| execPath, err := os.Executable() | ||
| if err != nil { | ||
| execPath = "" | ||
| } | ||
| if resolved, err := filepath.EvalSymlinks(execPath); err == nil { | ||
| execPath = resolved | ||
| } | ||
| return getUpdateInstructions(runtime.GOOS, execPath) | ||
| } |
There was a problem hiding this comment.
🟡 In CheckForUpdateWithWriter, the result of GetUpdateInstructions() is stored in updateInstructions and used for the empty-string guard, but then GetUpdateInstructions() is called a second time inside the fmt.Sprintf, discarding the stored value. This is a pre-existing issue, but this PR makes it more expensive by refactoring GetUpdateInstructions() to now call os.Executable() and filepath.EvalSymlinks() on every invocation — meaning two unnecessary syscalls happen on the second call. Fix: use the updateInstructions variable in the Sprintf call instead of calling GetUpdateInstructions() again.
Extended reasoning...
The double-call bug
In CheckForUpdateWithWriter (packages/util/check-for-update.go), GetUpdateInstructions() is called at line 60 and the result is stored in the updateInstructions variable. That variable is then used correctly on line 62 for the empty-string guard (if updateInstructions != ""). However, on line 63, the code calls GetUpdateInstructions() a second time inside the fmt.Sprintf instead of using the already-stored variable:
updateInstructions := GetUpdateInstructions() // line 60 – result stored
if updateInstructions \!= "" { // line 62 – stored var used correctly
msg = fmt.Sprintf("\n%s\n", GetUpdateInstructions()) // line 63 – called AGAIN\!
fmt.Fprintln(w, msg)
}The stored updateInstructions variable is effectively used only as a boolean check and never appears in the actual output.
Why existing code does not prevent it
This is a straightforward oversight: the developer wrote the guard correctly using the stored variable but forgot to reuse it in the format string one line later. No compiler warning or linter would catch this because the variable is used (in the if-condition), just not where it matters most.
Impact made worse by this PR
Before this PR, GetUpdateInstructions() was a trivial GOOS switch — essentially a compile-time constant lookup with no I/O. The redundant second call had negligible cost.
After this PR, GetUpdateInstructions() delegates to the new public wrapper that calls os.Executable() (a syscall) and filepath.EvalSymlinks() (which may involve multiple stat/readlink syscalls for each path component). These are cheap in absolute terms, but calling them twice per update check is purely wasteful — the result is deterministic during a single process invocation, so there is no reason to repeat the work.
Concrete proof
- User runs a version of infisical older than the latest release.
- CheckForUpdateWithWriter is called.
- GetUpdateInstructions() is invoked at line 60: os.Executable() + filepath.EvalSymlinks() are called; result, e.g., "To update, run: brew update && brew upgrade infisical", is stored in updateInstructions.
- The if-guard on line 62 passes (non-empty string).
- GetUpdateInstructions() is invoked again at line 63: os.Executable() + filepath.EvalSymlinks() are called a second time; the same string is returned and embedded in msg.
- The variable updateInstructions from step 3 is never used in the output.
How to fix
Change line 63 from:
msg = fmt.Sprintf("\n%s\n", GetUpdateInstructions())to:
msg = fmt.Sprintf("\n%s\n", updateInstructions)This is a one-word change with no behavioral difference, eliminating the redundant syscalls.
| case "darwin": | ||
| if isNpm { | ||
| return "To update, run: npm update -g @infisical/cli" | ||
| } | ||
| return "To update, run: brew update && brew upgrade infisical" |
There was a problem hiding this comment.
🟡 The darwin case in getUpdateInstructions() only checks for npm via a node_modules path substring, then unconditionally returns brew instructions for all other macOS installs — so users who installed via MacPorts, Nix, direct binary download, or any non-brew/non-npm method will see incorrect brew update instructions even if Homebrew is not installed. This is a pre-existing limitation the PR explicitly chose to maintain, but unlike Windows (which checks for scoop/winget path substrings), macOS has no homebrew path check before recommending brew.
Extended reasoning...
What the bug is: The getUpdateInstructions() darwin case (lines 183-187 of packages/util/check-for-update.go) performs exactly one check — whether the resolved executable path contains node_modules — and if not, unconditionally returns brew update instructions. There is no verification that the binary actually lives under a Homebrew prefix (e.g. /opt/homebrew, /usr/local/Cellar).
The specific code path:
The darwin switch arm checks isNpm first, and if false, immediately returns 'brew update && brew upgrade infisical' with no further path inspection. Any macOS user whose executable path does not contain node_modules falls straight through to the brew instruction, regardless of actual install method.
Why existing code does not prevent it: The PR intentionally chose not to add a homebrew path check for macOS, as explicitly documented in the PR description (macOS + brew -> brew update && brew upgrade infisical is the stated 'After' behavior). The previous code also defaulted to brew for all macOS installs; this PR improves only the npm detection case. This makes it a pre-existing design decision that the PR deliberately preserves.
Addressing the refutation: The refutation correctly notes that this is intentional behavior documented in the PR, that brew IS the dominant macOS install method, and that the PR does not make the situation worse for non-npm users — it only improves things. These points are valid. However, the inconsistency is still worth noting as a nit: Windows gets path-based detection for scoop (contains 'scoop') and winget (contains 'winget'), but macOS gets no analogous homebrew path check. Users who installed via MacPorts, Nix, pkgx, or direct binary download will see instructions they cannot run.
Step-by-step proof: A user installs infisical via nix-env -iA nixpkgs.infisical. The binary resolves to something like /nix/store/abc123-infisical/bin/infisical. The variable p is set to this lowercased path. It does not contain node_modules, so isNpm is false. The darwin branch falls through and returns 'To update, run: brew update && brew upgrade infisical'. The user may not have Homebrew installed at all, so this instruction is useless to them.
How to fix it: Mirror the Windows approach by checking for a Homebrew path prefix before defaulting. For example, check if the path contains '/homebrew/' or '/cellar/' before returning brew instructions, and return an empty string or generic message for unrecognized paths. This would be consistent with the pattern already established for Windows and avoids misleading non-brew macOS users.
3 participants
Description 📣
Fixes incorrect update instructions shown to users based on their install method. Previously, the update hint was hardcoded per OS — for example, Windows users who installed via npm would always see
scoop update infisicalregardless of how they actually installed the CLI.This PR refactors
GetUpdateInstructions()to detect the install method at runtime by inspecting the executable path viaos.Executable()+filepath.EvalSymlinks(). Symlink resolution is explicitly handled to cover macOS and Linux where package managers like npm create symlinks in$PATHpointing to the real binary insidenode_modules.Before:
After:
Type ✨
Tests 🛠️
Added unit tests covering all install method/OS combinations, and multi-OS integration tests that build a test helper binary, place it at real install paths (including via symlink to simulate npm's behavior), and assert the correct instruction is returned.
Run unit tests:
go test ./packages/util/ -run TestGetUpdateInstructions -vRun integration tests locally (macOS):
CI runs automatically on
ubuntu-latest,macos-latest, andwindows-latestvia.github/workflows/test-update-instructions.yml, covering npm, brew, scoop, winget, and apt detection on their respective platforms.Sample CI ran successfully: https://github.com/pangeran-bottor/cli/actions/runs/22317820293