fix: improve setup.sh reliability and fix dockerfile - #20
Conversation
- Move OS detection to the top of the file (was used before being set) - Add check_installed() helper to skip already-installed dependencies - Add safe_source() helper to avoid sourcing non-existent files - Remove 'npm' from brew dependency list (comes with node) - Define SNIPPET_LINK variable (was used undefined) - Replace broken $CONTINUE_ON_ERROR error handling with proper if/else - Add timeout (300s) to nvim headless commands - Use idempotent grep before appending to shell rc - Add existence check for monolisa-nerd-font directory - Quote all variable expansions consistently
- Fix --platform=x86-64 to --platform=linux/amd64 - Use apt + direct downloads instead of homebrew (inappropriate for Docker) - Extract docker-entrypoint.sh to a standalone file (heredoc in RUN is unreliable) - Add .dockerignore to reduce build context - Install neovim, lazygit, zoxide, fzf, gh via proper methods - Add DEBIAN_FRONTEND=noninteractive to prevent interactive prompts - Add timeout to nvim headless plugin installation - Use multi-stage RUN commands for better layer caching
Add deprecation notice at the top of prebuild.sh indicating it has been replaced by setup.sh and is no longer maintained.
Kailian-Jacy
left a comment
There was a problem hiding this comment.
🔍 Black-Box Review — PR #20
Reviewed via diff analysis,
bash -nsyntax checks,shellcheck, and runtime validation. No source code was read beyond the diff.
🔴 BLOCKER
1. Dockerfile: nvim-linux64.tar.gz asset no longer exists (dockerfile L40-44)
The Dockerfile downloads nvim-linux64.tar.gz and extracts to /opt/nvim-linux64/, but neovim latest release (v0.11.6+) renamed the asset to nvim-linux-x86_64.tar.gz and the extracted directory to nvim-linux-x86_64/. This means:
curl -fsSLwill 404 and fail the build- Even if the download somehow succeeds,
ln -s /opt/nvim-linux64/bin/nvimwill point to a non-existent path
Fix:
RUN curl -fsSL -o /tmp/nvim.tar.gz \
"https://github.com/neovim/neovim/releases/latest/download/nvim-linux-x86_64.tar.gz" \
&& tar -C /opt -xzf /tmp/nvim.tar.gz \
&& ln -s /opt/nvim-linux-x86_64/bin/nvim /usr/local/bin/nvim \
&& rm /tmp/nvim.tar.gz🟡 WARNING
2. setup.sh: timeout command unavailable on macOS (setup.sh L269, L277, L285)
timeout is a GNU coreutils command not available by default on macOS. The script installs dependencies via homebrew but does not include coreutils in INSTALL_DEPENDENCIES. On macOS, all three timeout "$NVIM_TIMEOUT" nvim ... calls will fail with command not found.
Suggestion: Add coreutils to deps, use gtimeout on macOS, or wrap with a fallback.
3. setup.sh: Non-idempotent — repeated appends to rc/profile files (setup.sh L116-121, L123, L164, L256-258)
Running setup.sh multiple times will keep appending duplicate entries to ~/.zshrc and ~/.zprofile:
- brew function definition (L116-121)
- PATH additions (L123)
- zoxide init (L164)
- OPENROUTER_API_KEY / DEEPSEEK_API_KEY / PATH lines (L256-258)
Only the source $DEFAULT_ENV_FILE_PATH line at L78 has a grep -qF guard. The rest will pile up.
Suggestion: Use grep -qF guards or write a marker comment and check for it.
4. setup.sh: macOS Homebrew path not sourced for Apple Silicon (setup.sh L97-102)
On Apple Silicon Macs, Homebrew installs to /opt/homebrew/. The script only adds the Linuxbrew path to $DEFAULT_ENV_FILE_PATH (L105). The macOS branch (L97-102) just checks if brew exists but doesn't add /opt/homebrew/bin/brew shellenv eval. If the user doesn't have brew in their PATH already, the check_installed brew on L99 will fail even after a successful install.
5. Dockerfile: Redundant COPY (dockerfile L75, L87)
COPY . /app/ at L75 already copies docker-entrypoint.sh into /app/. The second COPY docker-entrypoint.sh /app/docker-entrypoint.sh at L87 is redundant and creates an unnecessary layer. The chmod +x at L88 is also redundant since the file already has execute permission in the repo (100755).
6. Dockerfile: Missing luarocks (dockerfile)
setup.sh includes luarocks in its dependency list, but the Dockerfile doesn't install it. Some neovim plugins may require luarocks. If the Docker image should be functionally equivalent, luarocks should be added.
7. Dockerfile: curl piped to bash — supply chain risk (dockerfile L35, L56)
Two RUN instructions pipe curl output directly to bash:
- NodeSource setup (L35)
- zoxide install (L56)
This is a common pattern but carries supply chain risk. Consider pinning versions or verifying checksums.
8. .dockerignore: *.md glob makes README.md line redundant (.dockerignore L26-28)
*.md on L28 already covers README.md on L26. Not a bug, but slightly confusing.
9. Dockerfile: Running as root — no USER directive (dockerfile)
The entire Dockerfile runs and the entrypoint executes as root. For a dev-environment container this may be acceptable, but consider adding a non-root user for defense in depth.
🟢 GOOD
10. OS detection moved to the top of setup.sh — fixes the old bug where $OS was used before being set. The old expr substr approach was also fragile; the new [[ "$(uname -s)" == Linux* ]] is cleaner.
11. All shell scripts pass bash -n syntax validation. docker-entrypoint.sh passes shellcheck cleanly.
12. Added check_installed and safe_source helper functions — reduces repetition and improves readability.
13. Dependency pre-check loop (setup.sh L133-148) skips already-installed packages — much better UX on re-runs.
14. set -e + || { ... } error handling pattern with CONTINUE_ON_ERROR is correct and well-structured.
15. Adding --no-install-recommends in Dockerfile apt-get and cleaning /var/lib/apt/lists/* — good for image size.
16. DEBIAN_FRONTEND=noninteractive as ARG — proper practice for non-interactive Docker builds.
17. Font directory existence check before find (setup.sh L232) — prevents errors when font dir is absent.
18. SNIPPET_LINK made opt-in with empty default — avoids broken symlinks.
19. docker-entrypoint.sh as separate file with restart loop and configurable LISTEN_IP/LISTEN_PORT — much cleaner than the old inline heredoc approach.
20. EXPOSE ${LISTEN_PORT} uses the ARG variable — properly configurable.
📝 Summary
| Category | Count |
|---|---|
| 🔴 BLOCKER | 1 |
| 🟡 WARNING | 8 |
| 🟢 GOOD | 11 |
Verdict: The neovim asset name change is a build-breaker — the Docker image cannot be built as-is. Fix that and this is a solid improvement.
* feat: smarter and more flexible cmp setup - Add prefix match comparator: prefer exact prefix matches over fuzzy - Add locality-aware comparator: boost variables/fields, prefer local sources - Add explicit snippet expand function for reliable LuaSnip integration - Filter out LSP snippet items to avoid duplicates with LuaSnip - Enable use_show_condition for LuaSnip to respect snippet conditions - Limit buffer completions to visible buffers only (reduce noise) - Reorder sorting comparators for better relevance: recently_used > prefix_match > exact > locality > score > kind > proximity - Adjust source priorities: lower tabnine/yanky to reduce noise - Remove commented-out code blocks from cmdline mappings Addresses nvim-config#15 * fix: add nil check for entry.context in prefix_match_comparator entry1.context can be nil in some edge cases, causing a crash when accessing cursor_before_line. Add a guard check to return nil (fall through to next comparator) when context is unavailable. --------- Co-authored-by: Ubuntu <ubuntu@localhost.localdomain> Co-authored-by: OpenClaw Bot <bot@openclaw.dev>
Merge nvim-refine/main (44 commits) into nightly branch. Includes: - All merged PRs from nvim-refine (#20-#64) - Plugin cleanup, bug fixes, new features - Tests and documentation Conflicts resolved: - .gitignore: combined both sides - autocmds.lua: kept nightly version (nvim-runner plugin refactor) - keymaps.lua: kept nightly version (nvim-runner plugin integration) Note: commands.lua from refine contains legacy RunScript/RunTest definitions that are superseded by nvim-runner plugin on nightly. These will be cleaned up in a follow-up.
Summary
Improve setup.sh reliability, fix dockerfile issues, and deprecate prebuild.sh.
Changes
setup.sh
$OSwas used in conditional before being assignedcheck_installed()function — skip already-installed dependencies instead of reinstalling everythingsafe_source()function — prevent errors when sourcing non-existent filesnpmfrom brew dependency list — npm is not a brew formula, it comes withnodeSNIPPET_LINKvariable — was used undefined, causingbackup_and_linkto fail|| $CONTINUE_ON_ERRORpattern with proper if/else blocksgrep -qFbefore appending to avoid duplicatesdockerfile
--platform=x86-64→--platform=linux/amd64(correct Docker syntax)docker-entrypoint.shto standalone file — heredoc in DockerfileRUNis unreliable across BuildKit versions.dockerignore— reduce build context (exclude .git, fonts, neovim source, etc.)DEBIAN_FRONTEND=noninteractive— prevent interactive prompts during buildprebuild.sh
Testing
bash -n setup.sh— ✅ PASSbash -n docker-entrypoint.sh— ✅ PASS