refactor: consolidate NixOS hosts into shared builder - #1473
Conversation
All NixOS hosts (matic, viper, ISOs) now inherit from hosts/nixos/default.nix instead of duplicating pkgs setup, user config, and nix settings. Adds isViper to lib/host.nix. Removes redundant environment.systemPackages from hosts with home-manager.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Mesa DescriptionTL;DRConsolidated NixOS host configurations into a shared builder in What changed?
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 refactors the NixOS configuration by introducing a more flexible and modular shared system builder in hosts/nixos/default.nix, which is now utilized by the matic and viper host configurations to reduce redundancy. Additionally, it introduces environment detection for the viper VM, updates the stateVersion for several hosts, and removes the isDev attribute. Feedback was provided regarding an orphaned comment in lib/host.nix that remained after the removal of isDev and should be deleted for better code clarity.
| # Install language server packages | ||
| isDev = true; | ||
|
|
||
| # Install language server package |
There was a problem hiding this comment.
This comment appears to be orphaned and misplaced after the removal of the isDev attribute. It should be removed to maintain code clarity.
# Get the node name for OpenClaw remote mode
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralized NixOS host construction into Changes
Sequence Diagram(s)sequenceDiagram
participant Flake as flake.nix
participant Builder as hosts/nixos/default.nix
participant Named as named-hosts/*/{default,iso}.nix
participant Nixpkgs as inputs.nixpkgs
Flake->>Builder: import with inputs, username, stateVersion
Named->>Builder: import with hostname, userInitialPassword, modules
Builder->>Nixpkgs: include inputs.nixpkgs.nixosModules and nixpkgs.pkgs
Builder->>Named: apply baseModule + conditional genericModules -> resulting NixOS module set
Builder->>Flake: returns constructed system/iso derivation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 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="hosts/nixos/default.nix">
<violation number="1" location="hosts/nixos/default.nix:10">
P2: Default `stateVersion` changed from `"24.05"` to `"24.11"`. NixOS `stateVersion` gates backward-compatibility migrations and should not change after initial installation. The `tests/eval.nix` evaluations for `eval-nixos-default` and `eval-nixos-runner` don't pass `stateVersion`, so they now evaluate `"24.11"` while production uses `"24.05"` — meaning tests no longer match deployed config. Consider keeping the default as `"24.05"` to match the existing hosts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| hostname ? "x86_64-linux", | ||
| isRunner ? false, | ||
| system ? "x86_64-linux", | ||
| stateVersion ? "24.11", |
There was a problem hiding this comment.
P2: Default stateVersion changed from "24.05" to "24.11". NixOS stateVersion gates backward-compatibility migrations and should not change after initial installation. The tests/eval.nix evaluations for eval-nixos-default and eval-nixos-runner don't pass stateVersion, so they now evaluate "24.11" while production uses "24.05" — meaning tests no longer match deployed config. Consider keeping the default as "24.05" to match the existing hosts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hosts/nixos/default.nix, line 10:
<comment>Default `stateVersion` changed from `"24.05"` to `"24.11"`. NixOS `stateVersion` gates backward-compatibility migrations and should not change after initial installation. The `tests/eval.nix` evaluations for `eval-nixos-default` and `eval-nixos-runner` don't pass `stateVersion`, so they now evaluate `"24.11"` while production uses `"24.05"` — meaning tests no longer match deployed config. Consider keeping the default as `"24.05"` to match the existing hosts.</comment>
<file context>
@@ -1,134 +1,151 @@
hostname ? "x86_64-linux",
isRunner ? false,
+ system ? "x86_64-linux",
+ stateVersion ? "24.11",
+ userExtraGroups ? [ ],
+ userInitialPassword ? null,
</file context>
| stateVersion ? "24.11", | |
| stateVersion ? "24.05", |
![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 the current code and only fix it if needed.
Inline comments:
In `@hosts/nixos/default.nix`:
- Around line 83-86: The file sets both GRUB and a systemd-boot option; remove
or reconcile the conflicting setting by either deleting the systemd-boot line
(boot.loader.systemd-boot.configurationLimit) if you intend to use GRUB
(boot.loader.grub.enable, boot.loader.grub.device,
boot.loader.grub.useOSProber), or conversely disable GRUB and keep the
systemd-boot configuration if you intend to use systemd-boot; update the file so
only the intended boot loader’s options remain.
In `@lib/host.nix`:
- Line 17: Remove or update the orphaned comment "# Install language server
package" that no longer applies; either delete it entirely or replace it with a
concise, accurate comment describing the following symbol (nodeName) or the
restored isDev logic if you intend to reintroduce it. Locate the stray comment
near the nodeName definition and ensure comments accurately reflect the
surrounding code (e.g., describe nodeName's purpose or restore isDev-related
logic if that was meant to be documented).
🪄 Autofix (Beta)
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
Run ID: 8bcb0bca-580f-41ab-a9f9-e9f9465797eb
📒 Files selected for processing (10)
flake.nixhosts/nixos/default.nixlib/host.nixnamed-hosts/matic/default.nixnamed-hosts/matic/iso.nixnamed-hosts/shared/linux-base.nixnamed-hosts/shared/live-iso.nixnamed-hosts/viper/default.nixnamed-hosts/viper/iso.nixtests/lib.nix
💤 Files with no reviewable changes (2)
- named-hosts/shared/live-iso.nix
- named-hosts/shared/linux-base.nix
| boot.loader.grub.enable = true; | ||
| boot.loader.grub.device = "/dev/sda"; | ||
| boot.loader.grub.useOSProber = true; | ||
| boot.loader.systemd-boot.configurationLimit = 10; |
There was a problem hiding this comment.
Conflicting boot loader configuration.
boot.loader.grub.enable = true and boot.loader.systemd-boot.configurationLimit are set together, but GRUB and systemd-boot are mutually exclusive boot loaders. The systemd-boot.configurationLimit setting has no effect when GRUB is enabled.
Either remove line 86 since it's ineffective with GRUB, or clarify the intent if this is meant as a fallback configuration.
Proposed fix
boot.loader.grub.enable = true;
boot.loader.grub.device = "/dev/sda";
boot.loader.grub.useOSProber = true;
- boot.loader.systemd-boot.configurationLimit = 10;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| boot.loader.grub.enable = true; | |
| boot.loader.grub.device = "/dev/sda"; | |
| boot.loader.grub.useOSProber = true; | |
| boot.loader.systemd-boot.configurationLimit = 10; | |
| boot.loader.grub.enable = true; | |
| boot.loader.grub.device = "/dev/sda"; | |
| boot.loader.grub.useOSProber = true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hosts/nixos/default.nix` around lines 83 - 86, The file sets both GRUB and a
systemd-boot option; remove or reconcile the conflicting setting by either
deleting the systemd-boot line (boot.loader.systemd-boot.configurationLimit) if
you intend to use GRUB (boot.loader.grub.enable, boot.loader.grub.device,
boot.loader.grub.useOSProber), or conversely disable GRUB and keep the
systemd-boot configuration if you intend to use systemd-boot; update the file so
only the intended boot loader’s options remain.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@named-hosts/matic/default.nix`:
- Around line 9-17: The matic host import call is missing an explicit
stateVersion, causing it to inherit the wrapper default; update the call where
you import ../../hosts/nixos for hostname "matic" (the matic host definition in
default.nix) to include a stateVersion attribute (e.g. stateVersion = "24.05" to
match talismand and runner, or "24.11" if matic needs the newer version) so the
host is pinned and won’t silently drift.
🪄 Autofix (Beta)
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
Run ID: ac28efac-3e05-4050-82bc-bc7ce5063800
📒 Files selected for processing (5)
home-manager/modules/xremap/default.nixhome-manager/programs/fish/functions/_pixelh_function.fishhosts/nixos/default.nixlib/host.nixnamed-hosts/matic/default.nix
💤 Files with no reviewable changes (1)
- home-manager/programs/fish/functions/_pixelh_function.fish
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/host.nix
- hosts/nixos/default.nix
| import ../../hosts/nixos { | ||
| inherit inputs username; | ||
| hostname = "matic"; | ||
| userExtraGroups = [ | ||
| "input" | ||
| "video" | ||
| "audio" | ||
| "docker" | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== matic host wrapper call =="
sed -n '1,80p' named-hosts/matic/default.nix
echo
echo "== shared host wrapper stateVersion handling =="
sed -n '1,220p' hosts/nixos/default.nix | rg -n -C2 'stateVersion|system\.stateVersion' || true
echo
echo "== other host/stateVersion pins =="
rg -n -C2 'system\.stateVersion|stateVersion\s*=' flake.nix hosts named-hostsRepository: shunkakinoki/dotfiles
Length of output: 3862
🏁 Script executed:
sed -n '135,145p' flake.nixRepository: shunkakinoki/dotfiles
Length of output: 481
🏁 Script executed:
# Check git history for matic's previous stateVersion
git log --all --oneline -- named-hosts/matic/default.nix | head -5
# Check if there are any comments in the file about stateVersion
rg -n 'stateVersion|state.version' named-hosts/matic/default.nix
# Check other named-hosts to see pattern
rg -n 'stateVersion|state.version' named-hosts/Repository: shunkakinoki/dotfiles
Length of output: 405
🏁 Script executed:
# Search for any stateVersion references in the entire flake context
rg 'stateVersion' flake.nix --color=never -n
# Check if matic's hardware-configuration might have stateVersion
rg 'stateVersion|system\.stateVersion' named-hosts/matic/
# Look at the actual wrapper again to understand the design
sed -n '1,20p' hosts/nixos/default.nixRepository: shunkakinoki/dotfiles
Length of output: 698
Explicitly pin stateVersion for matic in flake.nix.
matic does not pass stateVersion at its call site (unlike talismand and runner, which are both pinned to "24.05"). As a result, it uses the wrapper default of "24.11", creating inconsistency and exposing it to silent drift if the wrapper default changes. Pin matic to "24.05" to match the other hosts, or to "24.11" if it requires the newer version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@named-hosts/matic/default.nix` around lines 9 - 17, The matic host import
call is missing an explicit stateVersion, causing it to inherit the wrapper
default; update the call where you import ../../hosts/nixos for hostname "matic"
(the matic host definition in default.nix) to include a stateVersion attribute
(e.g. stateVersion = "24.05" to match talismand and runner, or "24.11" if matic
needs the newer version) so the host is pinned and won’t silently drift.
1 participant
Summary
hosts/nixos/default.nixwith amodulesparam instead of duplicating pkgs/user/nix setupnamed-hosts/shared/linux-base.nixandnamed-hosts/shared/live-iso.nix(absorbed into hosts/nixos base)environment.systemPackagesfrom hosts that have home-manager (kept for viper and ISOs with comments explaining why)isVipertolib/host.nix+nodeName+ testStructure
environment.systemPackagesTest plan
nix flake checkpasses on a NixOS hostnix build .#nixosConfigurations.matic.config.system.build.toplevelevaluatesnix build .#nixosConfigurations.viper.config.system.build.toplevelevaluatesSummary by cubic
Consolidated all NixOS hosts into a shared builder in
hosts/nixos/default.nixto remove duplicate config and standardize setup. Also added a Slack-only xremap block and silenced read-only pkg andnixpkgs.pkgswarnings.Refactors
modules,specialArgs,stateVersion,userExtraGroups,userInitialPassword; pinsnixpkgsin/etc/nix, disables channels, setsnix-path, and adds trusted users (root, username,@wheel).x86_64-linux/runner config withhome-managerwhenmodulesis null.nixpkgs.pkgsvia its own module; removedpkgsfromspecialArgs; passpkgstohome-manageronly where needed; importnixosModules.readOnlyPkgs.named-hosts/shared/linux-base.nixandnamed-hosts/shared/live-iso.nix.initialPassword); viper uses builder withinitialPasswordand minimal system packages (nohome-manager); ISOs use installer modules, minimal packages, setimage.fileName, clear getty help line, setinitialPassword.flake.nix: passesstateVersionforx86_64-linuxandrunner.lib/host.nix: restoredisDev, addedisViper, expandednodeName; tests updated.home-manager: added a Slack-specific xremap block to isolate app workarounds; removed_pixelh_function.fish.Migration
import ./hosts/nixos { username; stateVersion; modules = [ ... ]; }.environment.systemPackageson hosts withouthome-manager(e.g., viper, ISOs).userInitialPasswordfor ISOs or test VMs; not for real machines like matic.Written for commit b8e186d. Summary will update on new commits.