fix: handle NVIM environment variable in test script#35
fix: handle NVIM environment variable in test script#35greggh merged 2 commits intogreggh:mainfrom krishna-bala:krishna.fix-test-runner
Conversation
When Claude Code runs inside Neovim, the NVIM environment variable points to a socket path instead of the nvim executable. This caused the test script to fail with permission errors. The fix checks if NVIM is an executable file (not a socket) before using it, otherwise falls back to finding nvim in PATH. This allows tests to run properly both from within Claude Code/Neovim and from regular terminals. Added: - Test script to verify NVIM detection logic works correctly - Documentation in DEVELOPMENT.md explaining the behavior
WalkthroughA new section was added to the documentation explaining how the test script handles the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
scripts/test.sh (1)
20-26: Use POSIX‐compliant command lookup
whichis not guaranteed on all systems. Prefer the POSIX-standardcommand -v nvimand handle its exit status. For example:- NVIM=$(which nvim) + if ! NVIM=$(command -v nvim); then + echo "Error: nvim not found in PATH" + exit 1 + fiDEVELOPMENT.md (2)
190-194: Surround your Markdown lists with blank lines
Per MD032, add an empty line before and after the bullet list to improve rendering:... The test script will: - Use the `$NVIM` variable… - Fall back to finding `nvim`… - Display which nvim binary… ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
191-191: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
195-199: Ensure fenced code blocks are separated by blank lines
Per MD031, insert blank lines before and after yourbashblock:To verify the NVIM detection logic works correctly, you can run: ```bash ./scripts/test_nvim_detection.sh<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 196-196: Fenced code blocks should be surrounded by blank lines null (MD031, blanks-around-fences) </details> </details> </blockquote></details> <details> <summary>scripts/test_nvim_detection.sh (1)</summary><blockquote> `23-31`: **Avoid masking return values when exporting** ShellCheck SC2155 recommends separating assignment and export. Also switch `which` to `command -v`: ```diff - export NVIM="$(which nvim)" + if ! NVIM=$(command -v nvim); then + echo "Error: nvim not found in PATH" + exit 1 + fi + export NVIM🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 25-25: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DEVELOPMENT.md(1 hunks)scripts/test.sh(1 hunks)scripts/test_nvim_detection.sh(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
DEVELOPMENT.md
191-191: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
196-196: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🪛 Shellcheck (0.10.0)
scripts/test_nvim_detection.sh
[warning] 25-25: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (1)
scripts/test.sh (1)
15-19: Logic for selecting NVIM executable is sound
The added check correctly ensures$NVIMis only used when it’s set, executable, and not a socket, before falling back to PATH.
- Replace 'which' with POSIX-compliant 'command -v' in test scripts - Fix markdown formatting issues in DEVELOPMENT.md (add blank lines) - Separate variable declaration and assignment in test_nvim_detection.sh - Add robust absolute path resolution using readlink and BASH_SOURCE - Enhance error handling with 'set -euo pipefail' for strict mode - Replace unreliable $0 references in bash -c contexts These changes improve portability, reliability, and follow shell scripting best practices while addressing all linting and formatting issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
DEVELOPMENT.md (3)
188-188: Add comma for non-restrictive clause
Insert a comma before “which” to improve readability:
“…handles the$NVIMenvironment variable, which normally points to a socket file…”🧰 Tools
🪛 LanguageTool
[uncategorized] ~188-~188: Possible missing comma found.
Context: ...tically handles the$NVIMenvironment variable which normally points to a socket file ...(AI_HYDRA_LEO_MISSING_COMMA)
192-195: Backticknvimand clarify “binary”
Wrapnvimin backticks in the third bullet and consider explicitly calling it a “Neovim binary” for consistency:
“- Display whichnvimbinary is being used for transparency”
196-200: Note script execution permission
Recommend adding a note thatscripts/test_nvim_detection.shmust be executable (e.g.,chmod +x) or can be run viabashto avoid “permission denied” errors in some environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DEVELOPMENT.md(1 hunks)scripts/test.sh(1 hunks)scripts/test_nvim_detection.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/test.sh
- scripts/test_nvim_detection.sh
🧰 Additional context used
🪛 LanguageTool
DEVELOPMENT.md
[uncategorized] ~188-~188: Possible missing comma found.
Context: ...tically handles the $NVIM environment variable which normally points to a socket file ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (1)
DEVELOPMENT.md (1)
186-186: New subsection heading looks good
The “### Running Tests from Within Neovim/Claude Code” heading is clear and consistent with the existing style.
|
Another great one. Same thing, we'll get this going in a couple days. |
2 participants
Summary
$NVIMpoints to a socket instead of the nvim executableProblem
When running
make testfrom within claude-code.nvim, the test script would fail with permission errors because the$NVIMenvironment variable points to a Neovim socket (e.g.,/run/user/1000/nvim.xxxxx.0) instead of the actual nvim executable.Solution
The test script now checks if
$NVIMis:which nvimTest plan
test_nvim_detection.shscript that verifies all scenarios work correctlymake testfrom within claude-code.nvim - now works properlymake testfrom regular terminal - continues to work as before🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes