Fix runtime shell - #1199
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a runtime issue in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated Neovim configuration deployment to manage Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Mesa DescriptionTL;DRFixes the runtime shell. What changed?File summaries are not available. Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several changes, primarily focused on Neovim configuration and a fix for a systemd service. My review focuses on the Makefile changes, where I've identified opportunities to improve maintainability and consistency. The logic for setting up Neovim symlinks is duplicated across three make targets. I recommend extracting this into a common helper target. Additionally, I've suggested simplifying some of the new shell commands and improving consistency in path handling. The other changes in the PR appear to be correct.
| @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \ | ||
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi |
There was a problem hiding this comment.
This if block can be simplified. rm -rf is idempotent and will not error if the path doesn't exist, and it correctly handles files, directories, and symlinks. Replacing this block with a single command improves readability.
Additionally, the rm command on line 606 was missing an @ prefix, which would cause it to be printed during execution. The suggested change fixes this as well.
@rm -rf "$(HOME)/.config/nvim/lua"
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/init.lua" ~/.config/nvim/init.lua | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/lua" ~/.config/nvim/lua |
There was a problem hiding this comment.
For consistency and better reliability in Makefiles, it's recommended to use $(HOME) instead of ~. The if block you added just above uses $(HOME), and it's a good practice to follow throughout the file to avoid unexpected shell expansion issues.
@ln -sf "$(PWD)/home-manager/programs/neovim/lua" "$(HOME)/.config/nvim/lua"
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/init.lua" ~/.config/nvim/init.lua | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/lua" ~/.config/nvim/lua |
There was a problem hiding this comment.
For consistency and better reliability in Makefiles, it's recommended to use $(HOME) instead of ~. Using $(HOME) consistently avoids potential issues with shell expansion.
@ln -sf "$(PWD)/home-manager/programs/neovim/lua" "$(HOME)/.config/nvim/lua"
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Makefile">
<violation number="1" location="Makefile:606">
P2: `lua-check-neovim` now deletes and replaces the user's real `~/.config/nvim/lua` tree, so a validation command becomes destructive.</violation>
<violation number="2" location="Makefile:622">
P3: Inconsistent use of `~` vs `$(HOME)` within the same target. The `if` block above uses `$(HOME)` but this new `ln` command uses `~`. Use `$(HOME)` consistently for reliability — tilde expansion in Makefiles depends on the shell and can fail in certain quoting contexts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fi | ||
| @mkdir -p "$(HOME)/.config/nvim" | ||
| @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \ | ||
| rm -rf "$(HOME)/.config/nvim/lua"; \ |
There was a problem hiding this comment.
P2: lua-check-neovim now deletes and replaces the user's real ~/.config/nvim/lua tree, so a validation command becomes destructive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 606:
<comment>`lua-check-neovim` now deletes and replaces the user's real `~/.config/nvim/lua` tree, so a validation command becomes destructive.</comment>
<file context>
@@ -602,7 +602,11 @@ neovim-dev: ## Set up local Neovim development environment.
fi
@mkdir -p "$(HOME)/.config/nvim"
+ @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \
+ rm -rf "$(HOME)/.config/nvim/lua"; \
+ fi
@ln -sf "$(PWD)/home-manager/programs/neovim/init.lua" "$(HOME)/.config/nvim/init.lua"
</file context>
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/init.lua" ~/.config/nvim/init.lua | ||
| @ln -sf "$(PWD)/home-manager/programs/neovim/lua" ~/.config/nvim/lua |
There was a problem hiding this comment.
P3: Inconsistent use of ~ vs $(HOME) within the same target. The if block above uses $(HOME) but this new ln command uses ~. Use $(HOME) consistently for reliability — tilde expansion in Makefiles depends on the shell and can fail in certain quoting contexts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 622:
<comment>Inconsistent use of `~` vs `$(HOME)` within the same target. The `if` block above uses `$(HOME)` but this new `ln` command uses `~`. Use `$(HOME)` consistently for reliability — tilde expansion in Makefiles depends on the shell and can fail in certain quoting contexts.</comment>
<file context>
@@ -611,7 +615,11 @@ neovim-dev: ## Set up local Neovim development environment.
+ rm -rf "$(HOME)/.config/nvim/lua"; \
+ fi
@ln -sf "$(PWD)/home-manager/programs/neovim/init.lua" ~/.config/nvim/init.lua
+ @ln -sf "$(PWD)/home-manager/programs/neovim/lua" ~/.config/nvim/lua
@ln -sf "$(PWD)/home-manager/programs/neovim/nvim-pack-lock.json" ~/.config/nvim/nvim-pack-lock.json
@nvim --headless +"lua vim.pack.update()" +qa
</file context>
| @ln -sf "$(PWD)/home-manager/programs/neovim/lua" ~/.config/nvim/lua | |
| @ln -sf "$(PWD)/home-manager/programs/neovim/lua" "$(HOME)/.config/nvim/lua" |
There was a problem hiding this comment.
Pull request overview
Adjusts systemd service startup for the CrowdStrike Falcon sensor and streamlines the Neovim config/dev workflow by removing Treesitter textobjects support and ensuring the Lua runtime path is available in local development setups.
Changes:
- Run Falcon init script via an explicit bash interpreter in
ExecStartPre. - Remove
nvim-treesitter-textobjectsplugin and its Treesitter configuration. - Update Neovim dev/upgrade/check Make targets to symlink the repo
lua/directory into~/.config/nvim.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| named-hosts/matic/falcon.nix | Updates systemd ExecStartPre to run the init script with bash explicitly. |
| home-manager/programs/neovim/lua/config/treesitter.lua | Removes Treesitter textobjects configuration block. |
| home-manager/programs/neovim/lua/config/plugins.lua | Removes the nvim-treesitter-textobjects plugin from the plugin set. |
| home-manager/programs/neovim/init.lua | Reorders/ensures Lua module search paths include both init location and stdpath config. |
| Makefile | Adds lua/ symlinking for Neovim targets and clears existing ~/.config/nvim/lua before linking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| Type = "forking"; | ||
| PIDFile = "/run/falcond.pid"; | ||
| ExecStartPre = initScript; | ||
| ExecStartPre = "${pkgs.bash}/bin/bash ${initScript}"; |
| @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \ | ||
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi |
| @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \ | ||
| rm -rf "$(HOME)/.config/nvim/lua"; \ | ||
| fi |
| @if [ -L "$(HOME)/.config/nvim/lua" ] || [ -d "$(HOME)/.config/nvim/lua" ]; then \ | ||
| rm -rf "$(HOME)/.config/nvim/lua"; \ |
Summary by cubic
Fix Falcon startup by running the init script with
bash. Also ensure the Neovim dev environment loads the projectluaconfig correctly and remove the unusednvim-treesitter-textobjectsplugin.Bug Fixes
falcondsystemd unit: runExecStartPrevia${pkgs.bash}/bin/bash ${initScript}to use the correct shell.luaconfig dir (removing any existing one) andinit.luaadjusts package paths so local modules resolve.Dependencies
nvim-treesitter-textobjectsand its textobjects settings from Treesitter config.Written for commit 89a3122. Summary will update on new commits.