fix(switch): handle platform plugins and dotagents - #2283
Conversation
Entire-Checkpoint: a4d3b165d044
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds platform-aware ChangesNeovim platform support
Dotagents synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 237-238: Update the Makefile switch target so nvim-plugins-install
is no longer a parallel prerequisite; invoke it in the switch recipe after
nix-switch and the other prerequisite targets have completed, preserving the
existing switch behavior while serializing native-plugin installation.
In `@spec/make_switch_spec.sh`:
- Around line 7-9: Update the checks in the switch target tests to scope each
assertion to its exact Makefile target declaration rather than searching
arbitrary context ranges. Assert that switch depends on dotagents-switch-sync
and excludes dotagents-sync, dotagents-switch-sync depends on dotagents-prepare,
and the sync target preserves the expected skip variable; use target-aware
parsing or anchored extraction instead of fixed grep -A ranges.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed64f237-7e34-41e4-9ae0-ca7c6ff46784
📒 Files selected for processing (5)
Makefilehome-manager/programs/neovim/activate-build-plugins.shhome-manager/programs/neovim/default.nixspec/activate_neovim_spec.shspec/make_switch_spec.sh
| .PHONY: switch | ||
| switch: nix-switch services nvim-plugins-install dotagents-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. | ||
| switch: nix-switch services nvim-plugins-install dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Serialize native-plugin installation after nix-switch.
If make runs with -j, it can start nvim-plugins-install before nix-switch. The plugin scan can then find no new pack directory and exit successfully. The target does not retry after nix-switch completes.
Run nvim-plugins-install from the switch recipe after its prerequisites complete.
Proposed fix
-switch: nix-switch services nvim-plugins-install dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons.
+switch: nix-switch services dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons.
+ @$(MAKE) nvim-plugins-install
@$(MAKE) refresh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 237 - 238, Update the Makefile switch target so
nvim-plugins-install is no longer a parallel prerequisite; invoke it in the
switch recipe after nix-switch and the other prerequisite targets have
completed, preserving the existing switch behavior while serializing
native-plugin installation.
| It 'uses a switch-specific checked-in dotagents sync' | ||
| When run grep 'dotagents-switch-sync' "$MAKEFILE" | ||
| The output should include 'dotagents-switch-sync' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert target relationships, not only target text.
Line 8 searches the entire Makefile. It passes if dotagents-switch-sync exists, even when switch: no longer depends on it.
Line 13 checks the recipe text but not the dotagents-switch-sync: dotagents-prepare prerequisite. Line 18 checks the skip variable but not that switch: uses dotagents-switch-sync instead of dotagents-sync.
Scope each check to the exact target declaration. Assert the prerequisite edges and assert that switch: excludes dotagents-sync. Avoid fixed grep -A8 and grep -A2 ranges because unrelated line changes can make these checks unreliable.
Based on the supplied Makefile contract, switch includes dotagents-switch-sync in Makefile, Lines 237-239, and the sync targets use dotagents-prepare in Makefile, Lines 341-364.
Also applies to: 12-14, 17-19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/make_switch_spec.sh` around lines 7 - 9, Update the checks in the switch
target tests to scope each assertion to its exact Makefile target declaration
rather than searching arbitrary context ranges. Assert that switch depends on
dotagents-switch-sync and excludes dotagents-sync, dotagents-switch-sync depends
on dotagents-prepare, and the sync target preserves the expected skip variable;
use target-aware parsing or anchored extraction instead of fixed grep -A ranges.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| _LIB_EXT="so" ;; \ | ||
| *) _LIB_EXT=""; _TRIPLE=""; echo "fff.nvim: unsupported platform, skipping download" ;; \ | ||
| esac; \ | ||
| if [ -n "$$_LIB_EXT" ]; then \ |
There was a problem hiding this comment.
Cosmetic: This if [ -n "$$_LIB_EXT" ] gate wraps not just the download but also the [ ! -f "$$FFF_BINARY" ] / "fff.nvim binary already present" branch. On unsupported OSes we won't report a cached binary either. Consider moving the _LIB_EXT guard to wrap only the download block so the "already present" message still fires:
FFF_BINARY="$$FFF_DIR/target/libfff_nvim.$${_LIB_EXT:-so}"; \
if [ ! -f "$$FFF_BINARY" ]; then \
if [ -n "$$_LIB_EXT" ]; then \
...download...; \
fi; \
else \
echo "fff.nvim binary already present"; \
fiStrictly optional — not blocking.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 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="spec/make_switch_spec.sh">
<violation number="1" location="spec/make_switch_spec.sh:8">
P3: These new specs only check that particular substrings exist somewhere in the Makefile (or within a fixed `grep -A` window) rather than asserting the actual target/prerequisite relationships. For example, the first test would still pass even if `switch:` no longer listed `dotagents-switch-sync` as a prerequisite, since `dotagents-switch-sync` also appears in its own target declaration elsewhere in the file. Consider scoping each assertion to the exact target declaration line (e.g., anchoring the grep to `^switch:`) and asserting prerequisite edges directly instead of relying on fixed-size `-A` windows.</violation>
</file>
<file name="home-manager/programs/neovim/default.nix">
<violation number="1" location="home-manager/programs/neovim/default.nix:22">
P2: Changing libExt to dylib on Darwin is applied globally, so the telescope-fzf-native and vscode-diff presence checks now look for a `.dylib` that those builds never produce (they emit `libfzf.so` / `libvscode_diff*.so`, as keymaps.lua also expects). Result: on every `make switch` on macOS those two plugins are force-rebuilt (`make clean all`) even when nothing changed. Scope the dylib handling to fff.nvim's download only and keep `so` for the telescope-fzf-native and vscode-diff checks.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:238">
P2: Listing `nvim-plugins-install` as a parallel prerequisite of `switch` means that under `make -j`, it can run concurrently with (or before) `nix-switch` finishes, potentially finding no pack directory yet and exiting successfully without retrying. Consider running `nvim-plugins-install` explicitly from the `switch` recipe body after its prerequisites complete, rather than as a parallel prerequisite.</violation>
<violation number="2" location="Makefile:364">
P3: The new `dotagents-switch-sync` target duplicates the exact recipe already present in the CI branch of `dotagents-sync` (the `DOTAGENTS_SKIP_SYNC=1 ruler-prepare commands-sync skills-sync mcp-sync ruler-dotdirs-sync` invocation). This is the 'sync checked-in dotagents content only' behavior now used in two places, so any future change to which sub-targets a checked-in-only sync runs (or to the skip flag) must be applied twice and can easily drift. Consider extracting a shared helper target, e.g. `dotagents-sync-checked`, that both `dotagents-sync`'s CI branch and `dotagents-switch-sync` depend on, so the recipe lives in one place.</violation>
</file>
<file name="spec/activate_neovim_spec.sh">
<violation number="1" location="spec/activate_neovim_spec.sh:71">
P3: The two new tests are tautological: they grep the script source for the literal strings `aarch64` / `apple-darwin`, which exist verbatim in the implementation, so they pass unconditionally and provide false confidence. They can't detect a regression in the actual platform dispatch (e.g., a wrong arch/triple pairing, a dropped Darwin guard, or a broken download URL construction) because they assert the presence of a static string, not the resulting behavior. Consider asserting on the constructed output instead — for instance, running the arch/triple resolution logic and checking the produced URL/triple, or testing the branch decision — so these tests actually pin the Darwin behavior they claim to cover.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| pkgs.gcc | ||
| ]; | ||
| libExt = "so"; | ||
| libExt = if pkgs.stdenv.isDarwin then "dylib" else "so"; |
There was a problem hiding this comment.
P2: Changing libExt to dylib on Darwin is applied globally, so the telescope-fzf-native and vscode-diff presence checks now look for a .dylib that those builds never produce (they emit libfzf.so / libvscode_diff*.so, as keymaps.lua also expects). Result: on every make switch on macOS those two plugins are force-rebuilt (make clean all) even when nothing changed. Scope the dylib handling to fff.nvim's download only and keep so for the telescope-fzf-native and vscode-diff checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/default.nix, line 22:
<comment>Changing libExt to dylib on Darwin is applied globally, so the telescope-fzf-native and vscode-diff presence checks now look for a `.dylib` that those builds never produce (they emit `libfzf.so` / `libvscode_diff*.so`, as keymaps.lua also expects). Result: on every `make switch` on macOS those two plugins are force-rebuilt (`make clean all`) even when nothing changed. Scope the dylib handling to fff.nvim's download only and keep `so` for the telescope-fzf-native and vscode-diff checks.</comment>
<file context>
@@ -19,7 +19,7 @@ let
pkgs.gcc
];
- libExt = "so";
+ libExt = if pkgs.stdenv.isDarwin then "dylib" else "so";
in
{
</file context>
|
|
||
| .PHONY: switch | ||
| switch: nix-switch services nvim-plugins-install dotagents-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. | ||
| switch: nix-switch services nvim-plugins-install dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. |
There was a problem hiding this comment.
P2: Listing nvim-plugins-install as a parallel prerequisite of switch means that under make -j, it can run concurrently with (or before) nix-switch finishes, potentially finding no pack directory yet and exiting successfully without retrying. Consider running nvim-plugins-install explicitly from the switch recipe body after its prerequisites complete, rather than as a parallel prerequisite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 238:
<comment>Listing `nvim-plugins-install` as a parallel prerequisite of `switch` means that under `make -j`, it can run concurrently with (or before) `nix-switch` finishes, potentially finding no pack directory yet and exiting successfully without retrying. Consider running `nvim-plugins-install` explicitly from the `switch` recipe body after its prerequisites complete, rather than as a parallel prerequisite.</comment>
<file context>
@@ -200,37 +200,42 @@ nvim-plugins-install: ## Download/build missing Neovim native plugin binaries (f
.PHONY: switch
-switch: nix-switch services nvim-plugins-install dotagents-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons.
+switch: nix-switch services nvim-plugins-install dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons.
@$(MAKE) refresh
</file context>
| switch: nix-switch services nvim-plugins-install dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. | |
| switch: nix-switch services dotagents-switch-sync ## Apply Nix config, refresh services/plugins, and refresh agent daemons. | |
| @$(MAKE) nvim-plugins-install |
| MAKEFILE="$PWD/Makefile" | ||
|
|
||
| It 'uses a switch-specific checked-in dotagents sync' | ||
| When run grep 'dotagents-switch-sync' "$MAKEFILE" |
There was a problem hiding this comment.
P3: These new specs only check that particular substrings exist somewhere in the Makefile (or within a fixed grep -A window) rather than asserting the actual target/prerequisite relationships. For example, the first test would still pass even if switch: no longer listed dotagents-switch-sync as a prerequisite, since dotagents-switch-sync also appears in its own target declaration elsewhere in the file. Consider scoping each assertion to the exact target declaration line (e.g., anchoring the grep to ^switch:) and asserting prerequisite edges directly instead of relying on fixed-size -A windows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/make_switch_spec.sh, line 8:
<comment>These new specs only check that particular substrings exist somewhere in the Makefile (or within a fixed `grep -A` window) rather than asserting the actual target/prerequisite relationships. For example, the first test would still pass even if `switch:` no longer listed `dotagents-switch-sync` as a prerequisite, since `dotagents-switch-sync` also appears in its own target declaration elsewhere in the file. Consider scoping each assertion to the exact target declaration line (e.g., anchoring the grep to `^switch:`) and asserting prerequisite edges directly instead of relying on fixed-size `-A` windows.</comment>
<file context>
@@ -0,0 +1,21 @@
+MAKEFILE="$PWD/Makefile"
+
+It 'uses a switch-specific checked-in dotagents sync'
+When run grep 'dotagents-switch-sync' "$MAKEFILE"
+The output should include 'dotagents-switch-sync'
+End
</file context>
| .PHONY: dotagents-switch-sync | ||
| dotagents-switch-sync: dotagents-prepare ## Sync checked-in dotagents content during switch. | ||
| @echo "🔄 Syncing checked-in dotagents content..." | ||
| @$(MAKE) -C dotagents DOTAGENTS_SKIP_SYNC=1 ruler-prepare commands-sync skills-sync mcp-sync ruler-dotdirs-sync |
There was a problem hiding this comment.
P3: The new dotagents-switch-sync target duplicates the exact recipe already present in the CI branch of dotagents-sync (the DOTAGENTS_SKIP_SYNC=1 ruler-prepare commands-sync skills-sync mcp-sync ruler-dotdirs-sync invocation). This is the 'sync checked-in dotagents content only' behavior now used in two places, so any future change to which sub-targets a checked-in-only sync runs (or to the skip flag) must be applied twice and can easily drift. Consider extracting a shared helper target, e.g. dotagents-sync-checked, that both dotagents-sync's CI branch and dotagents-switch-sync depend on, so the recipe lives in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 364:
<comment>The new `dotagents-switch-sync` target duplicates the exact recipe already present in the CI branch of `dotagents-sync` (the `DOTAGENTS_SKIP_SYNC=1 ruler-prepare commands-sync skills-sync mcp-sync ruler-dotdirs-sync` invocation). This is the 'sync checked-in dotagents content only' behavior now used in two places, so any future change to which sub-targets a checked-in-only sync runs (or to the skip flag) must be applied twice and can easily drift. Consider extracting a shared helper target, e.g. `dotagents-sync-checked`, that both `dotagents-sync`'s CI branch and `dotagents-switch-sync` depend on, so the recipe lives in one place.</comment>
<file context>
@@ -334,13 +339,29 @@ upgrade-dev: ## Upgrade inside the Nix dev shell (mirrors CI).
+.PHONY: dotagents-switch-sync
+dotagents-switch-sync: dotagents-prepare ## Sync checked-in dotagents content during switch.
+ @echo "🔄 Syncing checked-in dotagents content..."
+ @$(MAKE) -C dotagents DOTAGENTS_SKIP_SYNC=1 ruler-prepare commands-sync skills-sync mcp-sync ruler-dotdirs-sync
.PHONY: codex-security-sync
</file context>
| The output should include 'aarch64' | ||
| End | ||
|
|
||
| It 'uses the Darwin fff.nvim target triple' |
There was a problem hiding this comment.
P3: The two new tests are tautological: they grep the script source for the literal strings aarch64 / apple-darwin, which exist verbatim in the implementation, so they pass unconditionally and provide false confidence. They can't detect a regression in the actual platform dispatch (e.g., a wrong arch/triple pairing, a dropped Darwin guard, or a broken download URL construction) because they assert the presence of a static string, not the resulting behavior. Consider asserting on the constructed output instead — for instance, running the arch/triple resolution logic and checking the produced URL/triple, or testing the branch decision — so these tests actually pin the Darwin behavior they claim to cover.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/activate_neovim_spec.sh, line 71:
<comment>The two new tests are tautological: they grep the script source for the literal strings `aarch64` / `apple-darwin`, which exist verbatim in the implementation, so they pass unconditionally and provide false confidence. They can't detect a regression in the actual platform dispatch (e.g., a wrong arch/triple pairing, a dropped Darwin guard, or a broken download URL construction) because they assert the presence of a static string, not the resulting behavior. Consider asserting on the constructed output instead — for instance, running the arch/triple resolution logic and checking the produced URL/triple, or testing the branch decision — so these tests actually pin the Darwin behavior they claim to cover.</comment>
<file context>
@@ -63,6 +63,16 @@ When run bash -c "grep 'uname -m' '$SCRIPT'"
+The output should include 'aarch64'
+End
+
+It 'uses the Darwin fff.nvim target triple'
+When run grep 'apple-darwin' "$SCRIPT"
+The output should include 'apple-darwin'
</file context>
| pkgs.gcc | ||
| ]; | ||
| libExt = "so"; | ||
| libExt = if pkgs.stdenv.isDarwin then "dylib" else "so"; |
There was a problem hiding this comment.
Perpetual rebuilds on macOS: libExt is threaded through activate-build-plugins.sh and used for all three plugins, but only fff.nvim actually publishes/produces a .dylib on Darwin. Both telescope-fzf-native and vscode-diff produce .so even on macOS:
telescope-fzf-native.nvim's upstream Makefile unconditionally setsTARGET := libfzf.soon non-Windows.keymaps.luain this repo also hard-codes.soin the presence check:home-manager/programs/neovim/lua/config/keymaps.lua:490—filereadable(fzf_dir .. "/build/libfzf.so")home-manager/programs/neovim/lua/config/keymaps.lua:510—glob(vsd_dir .. "/libvscode_diff*.so")
With libExt = "dylib", activate-build-plugins.sh will check libfzf.dylib / libvscode_diff*.dylib on Darwin, find nothing, force make -C "$fzf_dir" clean all and re-run build.sh on every make switch. The rebuild still produces .so, so the cache check will fail again next switch — a perpetual clean-rebuild loop.
Suggested fix: keep libExt = "so" globally and localize the Darwin extension inside activate-build-plugins.sh (and the Makefile nvim-plugins-install recipe) to the fff.nvim block only, e.g. a fff_ext variable derived from uname -s.
1 participant
Summary
.dylibextension, including Apple Silicon normalization, in activation and post-switch plugin paths.make switch, keeping optional external skill installation on the explicitmake syncpath.Validation
shellspec spec/activate_neovim_spec.sh spec/make_switch_spec.sh(21 examples, 0 failures)shellcheck home-manager/programs/neovim/activate-build-plugins.shmake nix-format-checkmake nix-lintmake nix-flake-checkreaches the Darwin configurations successfully but cannot build required x86_64-Linux derivations from this aarch64-Darwin host (docker.service.drv, exit 2).aarch64-apple-darwin.dylibreturns 200; the previously requestedarm64-unknown-linux-gnu.soreturns 404.Summary by cubic
Fixes platform handling for
fff.nvimby using the Darwin target and.dylibon macOS, and updates switch behavior to sync only checked-indotagentscontent after auto-initializing the submodule.fff.nvim: Normalize arch toaarch64/x86_64, useapple-darwinon macOS, and select.dylibvs.soin the Makefile and activation script.dotagents: Adddotagents-prepareto init the submodule;switchusesdotagents-switch-syncwithDOTAGENTS_SKIP_SYNC=1. Full external skill install remains onmake sync(skipped in CI)..dylib, and forswitchusingdotagents-switch-syncand submodule init.Written for commit 115440e. Summary will update on new commits.