LLM-1459: auto-append ~/.local/bin to PATH on install#173
LLM-1459: auto-append ~/.local/bin to PATH on install#173
Conversation
Kimchi Code Review
Summary📊 Review Score: 72/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: no — No tests were added or modified alongside the install script changes. 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
kodburn
changed the title
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 72/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: no — No tests were added or modified alongside the install script changes.
📝 Found 5 issue(s). See inline comments for details.
| echo -e "${YELLOW}Note: ${INSTALL_DIR} may not be on your PATH.${NC}" | ||
| echo -e "${YELLOW}Note: ${dir} is not on your PATH.${NC}" | ||
| printf '\n# Kimchi CLI\nexport PATH="%s:$PATH"\n' "$dir" >> "$rc" | ||
| echo -e "${GREEN}✓ Added ${dir} to PATH in ${rc}${NC}" |
There was a problem hiding this comment.
The printf call writes $dir into the user's rc file inside double quotes without escaping embedded " characters. If KIMCHI_INSTALL_DIR or $HOME contains double quotes, the generated export PATH=... line will contain malformed shell syntax, which can cause shell startup failures or unexpected command execution when the rc file is sourced.
💡 Suggestion: Use printf '%q' to safely quote $dir when generating the rc file line, for example: printf '\n# Kimchi CLI\nexport PATH='%q':$PATH\n' "$dir".
| esac | ||
| [ -f "$rc" ] || return 1 | ||
| grep -qF "# Kimchi CLI" "$rc" && return 0 | ||
| } |
There was a problem hiding this comment.
path_already_configured returns 0 when it finds the generic marker # Kimchi CLI in the rc file, regardless of which directory was previously configured. If a user reinstalls kimchi to a different directory, the old marker prevents the new $INSTALL_DIR from being added to PATH.
💡 Suggestion: Make the marker specific to the directory, e.g., write # Kimchi CLI: $dir and grep for that exact line, or grep for the specific export PATH line containing the target directory.
| echo -e "${YELLOW}Note: ${INSTALL_DIR} may not be on your PATH.${NC}" | ||
| echo -e "${YELLOW}Note: ${dir} is not on your PATH.${NC}" | ||
| printf '\n# Kimchi CLI\nexport PATH="%s:$PATH"\n' "$dir" >> "$rc" | ||
| echo -e "${GREEN}✓ Added ${dir} to PATH in ${rc}${NC}" |
There was a problem hiding this comment.
The printf ... >> "$rc" append operation is not checked for failure. If the rc file becomes unwritable or the disk is full, the script will still print a green success message claiming the file was updated.
💡 Suggestion: Check the append result, for example: printf '...' >> "$rc" || { echo "Failed to update $rc" >&2; exit 1; }.
| # Returns 0 if the install dir is already on PATH or present in an active | ||
| # (non-commented) line in the rc file, or if we already wrote our marker. | ||
| path_already_configured() { | ||
| local rc="$1" dir="$2" |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
path_already_configured relies on the implicit return value of grep -qF. If a later maintainer adds a command after the grep, the function's return value would silently change, breaking callers.
💡 Suggestion: Add an explicit return 1 as the last statement in path_already_configured so the fallback behavior is clear and robust.
| local rc="$1" dir="$2" | ||
| case ":${PATH}:" in | ||
| *":${dir}:"*) return 0 ;; | ||
| esac |
There was a problem hiding this comment.
ℹ️🐛 Bug
In the case pattern *":${dir}:"*, glob metacharacters in $dir (such as *, ?, or [) are interpreted as pattern syntax, which can cause the PATH-containment check to return false negatives for unusual paths.
💡 Suggestion: Iterate over PATH entries exactly instead of using a case glob, for example with a for loop using IFS=':' to check each path element for an exact match.
1 participant
When installing to
~/.local/bin, the script previously only printed instructions for the user to manually add the directory to their PATH. This PR makes the installer do it automatically - writing the export line to~/.zshrcor~/.bashrc/.bash_profilewith a# Kimchi CLIcomment. The change is skipped if the directory is already on$PATHor the comment is already present.Kimchi Summary
What changed
The
install.shscript now automatically appends aPATHexport to the user's shell rc file (e.g.,~/.zshrc,~/.bashrc) when it installskimchito a directory that is not on the currentPATH. It also guards against duplicate entries across repeated installs.Why
Previously the script only printed a manual command for the user to copy and paste. Automating the edit reduces friction during first-time setup and makes re-installations safer.
Key changes
scripts/install.sh: Renamed internal flag fromNEEDS_PATH_HINTtoNEEDS_PATH_SETUP.scripts/install.sh: Addedpath_already_configured()helper to check whether the install directory is already onPATHor the rc file contains the# Kimchi CLImarker.scripts/install.sh: Addedadd_to_path()helper to idempotently append theexport PATHline to the rc file.scripts/install.sh: Forzshandbash, replaced printed manual instructions with direct rc-file modification viaadd_to_path();fishand unknown shells continue to show manual instructions.Impact
# Kimchi CLImarker.sourcetheir rc file for the change to take effect in the current session.